fix(agents): emit terminal stream END only after finalize_turn - #3120
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to fe05cb3. Security Overview
Detected Code Changes
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee9e4e067c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a69290df84
ℹ️ 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".
jordan-umusu
left a comment
There was a problem hiding this comment.
LGTM on the current impl., but gentle pushback on the shape:
I think terminal END emission should be folded into the existing finalize_turn_activity, with AgentSessionService.finalize_turn enforcing the following invariant:
When finalize_turn returns, this run no longer owns the session’s active DB pointers and its captured Redis stream contains END.
The method should clear and commit the DB pointers, then emit END. If either step fails, it raises and Temporal retries the entire activity. The DB operation is
idempotent, and duplicate END frames are harmless (the current impl. can already do this).
|
@jordan-umusu Addressed in fe05cb3.
Could you take another look? |
| # Preserve the two former 10-second operation budgets when DB | ||
| # cleanup and Redis completion run inside one activity. | ||
| start_to_close_timeout=timedelta( | ||
| seconds=20 if emit_terminal_done else 10 |
There was a problem hiding this comment.
nit: remove the branching, this likely doesnt buy much
Summary
Backend root fix for ENG-1554: the final assistant message streams in fully, then vanishes at end of turn until reload.
The race: the executor loopback persists the final assistant rows (tagged
curr_run_id) and then emits the stream END token, butfinalize_turn— which clearscurr_run_idand thereby makes the rows visible toGET /sessions/{id}/vercel— only runs later in the workflowfinally. A client refetch triggered by END lands in that window and gets a snapshot with the just-finished turn hidden.This PR establishes the invariant that a client-observable terminal END implies the canonical DB snapshot is publishable: persist terminal state →
finalize_turn(compare-and-clear by run id) → emit END to the captured stream id.The frontend defense-in-depth layer (content-aware adopt guard) is #3119; regression origin and full RCA are in the Linear issue.
Changes
done()on Redis; it closes only external sinks viadone_external().curr_run_idremains set because the turn is still active, and a resumed turn uses the possibly rotated stream id._finalize_turn, and then calls the existing idempotentemit_session_doneactivity. An ambiguous retry may append a duplicate END, which clients already tolerate. If finalization exhausts its retries, END is still attempted so the client does not hang indefinitely.Temporal compatibility
FINALIZE_TURN_PATCHremains gated withworkflow.patched(...). Histories without that marker must not schedule the newerfinalize_turnactivity.APPROVAL_STREAM_V2_PATCHremains at its original command boundary asworkflow.deprecate_patch(...). This is a replay bridge for histories that recorded the v2 marker and is covered by a real history-replay regression test. Keep the bridge until those histories have drained.emit_session_donecommand and are not replay-compatible with that boundary; verify that no such executions remain RUNNING before rollout.REMINT_SCOPE_TOKENS_PATCHremains gated because approval waits are unbounded, so affected histories cannot be assumed to have drained.durable-agent-terminal-end-after-finalize-v1marker from an earlier revision of this PR was removed. It never reachedmain, so there are no mainline histories carrying it to preserve.Tests
curr_run_id IS NULLand the final rows throughlist_messages(include_active=False).durable-agent-approval-stream-v2, plus existing suspended and completed replay coverage.Summary by cubic
Fixes ENG-1554: emit terminal END only after
finalize_turnso the final assistant message stays visible at end-of-turn. The workflow now owns stream completion; the executor only closes external sinks.finalize_turn_activitywhenFINALIZE_TURN_WITH_END_PATCHis present: capture the active stream id, compare-and-clear by run id, then emit END. If a legacy worker runs this activity (returns no result), the workflow falls back toemit_session_done. If the combined finalize activity fails, we do not emit END.FINALIZE_TURN_PATCHis recorded, the workflow finalizes (DB-only) and then emits END separately.done()on Redis; it closes only external sinks. Approval pauses only close external sinks; deduplicated. Errors still stream inline immediately.emit_session_errorandemit_session_cancelledpersist their frames (and stream notices when needed) but never calldone(); the workflow emits terminal END after finalize. Cancellations persist the marker before END.APPROVAL_STREAM_V2_PATCHis retained viaworkflow.deprecate_patch(...); the executor’sdefer_done_on_approvalflag is removed.Written for commit fe05cb3. Summary will update on new commits.