fix(agent): guard the hosted child completion dispatch - #3492
Conversation
runHostedChildLifecycle wrapped the terminal dispatch on the failure path but not on the success path. When persisting the completion event threw, the error escaped into runHostedChildExecutionLifecycle's outer catch, which re-resolved it as an execution failure. A child that had succeeded was reported failed with the caller's executionFailedCode and the persistence error as its message, the adapter received two terminal dispatches (completed, then failed), and onLifecycleError was never called even when supplied. Guard the success path the same way the failure path is guarded. With no handler the error still propagates, preserving the existing contract test. With a handler it is reported once, the terminal state is not dispatched a second time, and the outcome carries CHILD_FINALIZATION_FAILED so a run whose work finished but whose record did not survive is distinguishable from one that failed to execute. Completion usage is preserved. Refusing to report success when the terminal state cannot be persisted stays as it was; durable-child-fork-execution.ts:882-889 applies the same policy on the setup path. Refs veryfront/veryfront-issue-inbox#420
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHosted child lifecycle finalization now handles completion persistence failures with a dedicated error code, lifecycle error reporting, and failed results that preserve execution usage. Tests cover hook failures, execution finalization failures, and duplicate terminal dispatch prevention. ChangesHosted child finalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b60795f032
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw lifecycleError; | ||
| } | ||
|
|
||
| await options.onLifecycleError(lifecycleError); |
There was a problem hiding this comment.
When onLifecycleError rejects while handling a failed completion dispatch, this await throws into runHostedChildExecutionLifecycle's outer catch. That path relabels the callback error with executionFailedCode and invokes adapter.failed, recreating the exact completed-then-failed double dispatch this change is intended to prevent. Catch failures from this observability callback and preserve the original CHILD_FINALIZATION_FAILED result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — real, and fixed in 2297b7d.
If onLifecycleError rejected, it propagated out of runHostedChildLifecycle into the outer catch, which relabels with executionFailedCode and dispatches adapter.failed — exactly the completed-then-failed double dispatch this change exists to prevent.
Contained it, matching the existing precedent at durable-child-fork-execution.ts:884-888, where a failing onLifecycleError already must not displace the authoritative terminal state.
Added "keeps the finalization outcome when onLifecycleError itself throws", and verified it is load-bearing: with the inner guard removed it fails, with it in place 16/16 pass.
…utcome A rejecting observability callback propagated out of runHostedChildLifecycle into runHostedChildExecutionLifecycle's outer catch, which relabels with executionFailedCode and dispatches adapter.failed — recreating the completed-then-failed double dispatch this guard exists to prevent. Contain it, matching durable-child-fork-execution.ts:884-888, where a failing onLifecycleError already must not displace the authoritative terminal state.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/hosted/child-lifecycle.test.ts (1)
253-255: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert one
onLifecycleErrorinvocation.The test confirms that
failedis not dispatched. It does not confirm thatonLifecycleErrorruns once. Count callback invocations and assert1; otherwise, duplicate lifecycle-error reports can pass this test.Proposed test update
diff --git a/src/agent/hosted/child-lifecycle.test.ts b/src/agent/hosted/child-lifecycle.test.ts --- a/src/agent/hosted/child-lifecycle.test.ts +++ b/src/agent/hosted/child-lifecycle.test.ts @@ -233,35 +233,37 @@ it("keeps the finalization outcome when onLifecycleError itself throws", async () => { const calls: string[] = []; + let lifecycleErrorCalls = 0; const adapter: HostedChildLifecycleAdapter = { completed: () => { calls.push("completed"); throw new Error("persist failed"); }, failed: () => { calls.push("failed"); }, }; const result = await runHostedChildLifecycle({ adapter, execute: async () => "ok", resolveErrorState: () => ({ status: "failed", terminalErrorCode: "STREAM_ERROR", terminalErrorMessage: "boom", }), onLifecycleError: () => { + lifecycleErrorCalls += 1; throw new Error("reporting failed"); }, }); // A failing observability callback must not relabel the outcome or trigger // a second terminal dispatch. assertEquals(calls, ["completed"]); + assertEquals(lifecycleErrorCalls, 1); assertEquals(result.status, "failed"); assertEquals( result.terminalState.terminalErrorCode, HOSTED_CHILD_FINALIZATION_FAILED_CODE, ); assertEquals(result.terminalState.terminalErrorMessage, "persist failed"); });Also applies to: 258-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/hosted/child-lifecycle.test.ts` around lines 253 - 255, Update the test’s onLifecycleError callback in the relevant lifecycle-error scenario to count invocations, then assert that the callback was called exactly once alongside the existing assertion that failed was not dispatched. Preserve the callback’s reporting-failure behavior while adding coverage against duplicate lifecycle-error reports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/agent/hosted/child-lifecycle.test.ts`:
- Around line 253-255: Update the test’s onLifecycleError callback in the
relevant lifecycle-error scenario to count invocations, then assert that the
callback was called exactly once alongside the existing assertion that failed
was not dispatched. Preserve the callback’s reporting-failure behavior while
adding coverage against duplicate lifecycle-error reports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65c99016-e18a-40e7-8f4d-af19519e3f34
📒 Files selected for processing (2)
src/agent/hosted/child-lifecycle.test.tssrc/agent/hosted/child-lifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/agent/hosted/child-lifecycle.ts
Also regenerate the api reference source links, which shift again with the guard added in 2297b7d.
|
@coderabbitai Applied — Also regenerated |
|
Tip For best results, initiate chat on the files or code changes.
Regenerating ✏️ Learnings added
You are interacting with an AI system. |
Fixes veryfront/veryfront-issue-inbox#420.
Problem
runHostedChildLifecycleguards the terminal dispatch on the failure path but not on the success path:When persisting the completion event throws, that escapes into
runHostedChildExecutionLifecycle's outer catch, which re-resolves it throughresolveHostedChildExecutionErrorStateas though the child's execution had failed.Reachable in production:
createDurableRunEventSinkrethrows on every failure (durable-run-event-sink.ts:128-131) — flush timeout, oversized event, API 5xx.Probe on a child that succeeded, with
adapter.completedthrowing a flush error:Three defects from one missing guard:
onLifecycleErrornever fires even when supplied, the adapter gets two terminal dispatches, and a persistence failure is labelled an execution failure.What this does not change
Refusing to report success when the terminal state cannot be persisted is deliberate —
durable-child-fork-execution.ts:882-889applies the same policy on the setup path. Thefailedoutcome is not the bug and is preserved.runHostedChildLifecyclealso still throws when noonLifecycleErroris supplied. That is asserted by the pre-existing test"still throws lifecycle hook errors on successful completion"(added with the file in #3123), and it still passes unchanged.Change
Guard the success dispatch symmetrically with the failure path. When a handler is supplied:
onLifecycleError, onceCHILD_FINALIZATION_FAILEDinstead of the caller'sexecutionFailedCode, so "work finished, record did not survive" is distinguishable from "failed to execute"usageHOSTED_CHILD_FINALIZATION_FAILED_CODEis a new exported constant. It is deliberately not added tohostedChildTerminalErrorCodes, sinceshouldSkipHostedChildTerminalPersistencekeys off that set and this state must still be persisted.Tests
Three added, and I verified they are load-bearing. With the guard reverted (constant kept, so the failure is behavioural rather than a missing export): all 3 new tests fail, all 12 pre-existing ones pass. With the guard: 15/15.
Also ran the two downstream consumers —
durable-child-fork-execution.test.tsanddefault-invoke-agent-tool.test.ts— 14 passed / 35 steps, green.deno lintanddeno check src/agent/index.tsclean.Deliberately out of scope
The failure path has the same double-dispatch twin: if its dispatch throws and no
onLifecycleErroris supplied, it rethrows into the outer catch, which re-dispatches. Left alone here because changing that rethrow is a contract change for existing callers, and this PR is scoped to the success path as filed. Noted on veryfront/veryfront-issue-inbox#420 so it is not lost.Summary by CodeRabbit
Bug Fixes
Documentation