fix: seven correctness and lifecycle findings from the bidi-streaming review - #5
Merged
Conversation
runAsync() carried only the TerminalException and SerializationException arms
of run(), missing its `catch (Throwable) => handleRunFailure()` branch. Two
consequences:
- a non-terminal failure inside the closure escaped runAsync() synchronously,
so the caller never received the DurableFuture the interface promises --
the "control returns immediately so the run can be composed concurrently"
contract broke precisely in the failure case;
- runAsync() took no RunOptions, so a per-run retry policy (attempt counting,
give-up-as-terminal) was unavailable for async runs.
Both now share a single runFuture() body: it journals the RunCommand, executes
the closure at most once and proposes the result, returning the future. run()
awaits it, runAsync() hands it back. handleRunFailure() returns that future
instead of awaiting internally, so the exhausted-attempts branch resolves
identically for both callers.
Sharing the body also removes ~40 lines that had already been duplicated once
and were free to drift again.
Implicit cancellation sent the built-in CANCEL signal to every call the handler had ever issued whose invocation id was known -- including calls that returned long ago. A workflow making 500 sequential calls and then being cancelled appended 500 SendSignalCommands, 499 of them targeting finished invocations, so the emitted commands grew with the invocation's length rather than with the number of children actually running. trackedInvocationIdCompletions (list<int>) becomes trackedCallCompletions (invocationId completion id => result completion id). raiseCancellation() now skips any call whose result completion has already landed: a finished call has nothing left to tear down. Also releases the proposed-run stash when the slice ends. sysEnd(), notifyError() and writeSuspension() route through a new markClosed() that clears pendingRunResults, so a ProposeRunCompletionAck arriving after the invocation ended cannot promote a stale value into the completion table, and a VM whose acks never arrive does not hold the values for its lifetime. Finally, the ack is decoded with its own ProposeRunCompletionAck::decode() instead of Notification::decode(). The two are different message types that happened to agree on field 1; a dedicated decoder keeps them from being coupled through that coincidence. The JournalBuilder docblock claiming the SDK ignores the ack was stale -- over streaming the ack IS the resolution.
Three issues in the bidi streaming host. Limits were hardcoded constants: 100_000 connections, 100_000 per IP, 100_000 concurrent, 3600s stream and connection idle. The values are right for the Restate runtime -- a single trusted peer holding many long-lived, deliberately idle streams from one IP, which amphp's own defaults reject -- but they are not a general hardening posture, and the endpoint also starts without identity verification. With the per-IP ceiling that high, anything else reaching the endpoint can hold many idle connections for an hour each. They move to a ServerLimits value object with the same defaults, passable to the constructor, every ceiling validated positive. README documents when to tighten them. Forked workers returned from listen() instead of exiting, so every statement after listen(), plus each registered shutdown function and destructor, ran once per worker. They now exit(0). The reap loop ran after the parent's own runServer() with no try/finally: if the parent threw (bind failure, driver error) the children survived it, still holding the port through SO_REUSEPORT. It is now in a finally, extracted to stopWorkers(). Also gitignores graphify-out/, a 13 MB generated tree that was one `git add -A` away from being committed.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 3 medium |
🟢 Metrics 27 complexity · 0 duplication
Metric Results Complexity 27 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
CI resolves dependencies fresh (composer.lock is gitignored for a library), so it
picked up phpstan/phpstan-phpunit 2.0.18 while local vendor/ still had 2.0.16.
The newer release reports `staticMethod.alreadyNarrowedType` for
assertInstanceOf() calls whose subject already has that declared return type --
12 of them, 11 pre-dating this branch. It is a latent failure on main, not
something this branch introduced, but the PR has to be green.
The assertions were tautologies: the return types already guarantee them. Rather
than suppress the rule, each is replaced by something the type system does NOT
know:
- the async-call variants now assert each future owns a distinct result
completion id (2, 4, 6), which is what "three separate calls were journaled"
actually means;
- the call-handle variants assert the invocation-id/result id pairs, pinning
both the allocation order and that no ids are shared;
- context metadata asserts random() is one seeded instance per invocation while
logger() is rebuilt per call so its replay state cannot go stale;
- the awakeable and runAsync cases keep only their meaningful assertion, plus
an isReady() check showing a proposed run stays unresolved until the runtime
acks it.
Two tests that previously asserted nothing beyond "the return type is the return
type" now carry real coverage.
The child branch called runServer() and then exit(0), but the call was not guarded. If a child's runServer() threw (bind failure, driver error) the exception unwound out of listen() in that process, running its destructors and shutdown functions -- exactly the lifecycle contamination exit(0) was added to prevent, and the same gap the parent path had before its try/finally. The child now always exits: cleanly on a normal stop, or with a message on STDERR and status 1 when it could not serve, so a supervisor can tell a failed worker from a clean shutdown. Reported by gitar-bot on #5.
The badge was never activated -- a commented-out block with a literal CODACY_PROJECT_ID placeholder. Codacy is not wired into the repo at all (no workflow, no config); it analysed pushes through a GitHub App installed on the account. With that integration going away, the placeholder is dead weight.
Code Review ✅ Approved 1 resolved / 1 findingsFixes seven correctness and lifecycle issues from the bidi-streaming review, including worker lifecycle handling and retry policies. No issues found. ✅ 1 resolved✅ Edge Case: Child worker exit not guarded if runServer throws
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Post-merge review of
fix/bidi-streaming-cancellation(merged as #4) surfaced seven issues that are all still live onmain. This fixes them. No behaviour change for the happy path — every default is preserved.runAsync()was missingrun()'s non-terminal failure branchlisten()instead of exitingfinallyNotificationDetails
runAsync()dropped the retry-policy path. It carried only theTerminalExceptionandSerializationExceptionarms ofrun(), missingcatch (Throwable) => handleRunFailure(). A transient failure inside the closure escapedrunAsync()synchronously, so the caller never got theDurableFuturethe interface promises — the "compose it concurrently" contract broke exactly in the failure case. It also took noRunOptions, so per-run retry policies were unavailable for async runs. Both now share onerunFuture()body;handleRunFailure()returns the future rather than awaiting internally. This also removes ~40 lines already duplicated once.Cancellation cancelled finished children.
trackedInvocationIdCompletionsonly ever grew, so a workflow making 500 sequential calls and then being cancelled appended 500SendSignalCommands — 499 targeting invocations that had returned. It becomes ainvocationIdCompletionId => resultCompletionIdmap, andraiseCancellation()skips any call whose result already landed. Emitted commands are now proportional to in-flight children, not to invocation length.Server limits are now a value object. 100 000 connections / per-IP / concurrent and 3600 s idle are correct for the Restate runtime — one trusted peer, many long-lived deliberately idle streams from one IP, which amphp's defaults reject — but they are not a general hardening posture, and the endpoint starts without identity verification by default. Same values, now in
ServerLimits, passable to the constructor, each validated positive. README says when to tighten.Worker lifecycle. Children returned from
listen(), so all post-listen()code, shutdown functions and destructors ran once per worker; they nowexit(0). The reap loop sat after the parent'srunServer()with notry/finally, so a parent that threw left children holding the port viaSO_REUSEPORT; it is now in afinally.Proposal stash.
sysEnd(),notifyError()andwriteSuspension()route throughmarkClosed(), which clearspendingRunResults. A lateProposeRunCompletionAckcan no longer promote a stale value into the completion table.Ack decoding.
ProposeRunCompletionAckgets its own decoder instead of borrowingNotification::decode(); the two are different message types that merely agreed on field 1.Also gitignores
graphify-out/(13 MB generated tree, onegit add -Afrom being committed).Test plan
composer test:unit— 529 passing (was 511); 18 new across 5 files['inv-A', 'inv-B']where['inv-B']is correct, and the post-sysEnd()ack resurrected a completioncomposer stan— PHPStan level max oversrc+tests, no errors, no baseline entries or ignores addedcomposer sast— Psalm taint analysis, no errorscomposer cs— 0 / 247make conformance) — needs Docker + AVX2 + JDK 21; label this PRconformanceto run it in CINot covered
The multi-worker fork path (
runServer,detectWorkers) still has no automated test — exercising it would fork the PHPUnit process. It needs an integration/smoke test, which is out of scope here.