fix(streaming): fence superseded streams out of the delta sink (single-writer, #65991) - #66005
fix(streaming): fence superseded streams out of the delta sink (single-writer, #65991)#66005HexLab98 wants to merge 2 commits into
Conversation
…e-writer) When the stale-stream detector reconnects past a stream whose socket abort raced (the close never actually stopped the old worker), the superseded stream and the retry's stream both write deltas into the same turn. The persisted transcript is then two coherent responses interleaved token-by-token — de-interleaving the stored text by alternation yields two complete, independent answers to the same prompt, which is a dual-writer race in the harness, not a model/context failure (NousResearch#65991). The interrupt path already positively cancels before force-closing (NousResearch#6600), but the stale-kill path relies only on the socket abort, and nothing fenced late chunks from a superseded stream out of the shared delta sink. Enforce a single-writer invariant on the sink itself, guarded by attempt id rather than only socket state: every streaming attempt (chat_completions, anthropic_messages, and bedrock paths) claims a monotonic writer token before it begins consuming its stream. A newer claim supersedes any older one, so the consume loop bails the instant it is superseded and _fire_stream_delta / _fire_reasoning_delta / _record_streamed_assistant_text drop chunks from a stale writer. The token is stored per-thread, so a thread that never claimed (a non-streaming delta caller) is never fenced — the guard can only ever drop a superseded stream, never the single legitimate writer. Discards are counted and logged sparsely so a real provider problem stays visible instead of being silently swallowed.
…eams Assert that a superseded stream (older writer token, other thread) is fenced from the delta sink, the active writer is never fenced, a non-claiming thread is never treated as a writer, and the real consume loop stops the instant it is superseded — so two streams can never interleave into one turn (NousResearch#65991).
1c139a2 to
4170cb6
Compare
Related to #65991: this is the focused implementation of the retry-path single-writer fence for persisted transcript integrity. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Summary
Fix: fences superseded streams out of the delta sink (single-writer guarantee, fixes #65991).
Prevents multiple streaming responses from interleaving in the history when the same agent is used rapidly. The "fence" ensures only the most recent stream writes to history.
Clean, targeted fix. No security concerns.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Single-writer SSE streaming fix (285 lines). Fences superseded streams out of the delta sink.
Checked diff:
api_key="test-key"is a test fixture, not a real credential- No security concerns
Looks good. No blocking issues.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary (Re-review)
Verdict: LGTM / Comment
Fix implements single-writer fencing for superseded streams in the delta sink. Prior COMMENT review confirmed. No issues found.
|
Merged through #66284 with your authorship preserved via rebase: both of your single-writer commits landed on main, then the same ownership fence was extended to the Codex Responses SSE path during the sibling audit. The linked #65991 is closed as completed. Thank you @HexLab98 — the monotonic writer-token design was the right fix. This original PR is only being closed because the widened replacement has landed, not because the contribution was rejected. |
Fixes #65991
Problem
When stream recovery retries while the prior SSE stream is still delivering
(the stale-stream / reconnect path), the superseded stream is not positively
terminated. Both the old and the new stream write deltas into the same turn,
so the persisted transcript is two coherent responses interleaved
token-by-token — de-interleaving the stored text by alternation yields two
complete, independently fluent answers to the same prompt. That signature is a
dual-writer race in the harness, not a model/context failure.
Root cause
In
agent/chat_completion_helpers.py::interruptible_streaming_api_call():_request_cancelled["value"] = Truebeforeforce-closing so the worker recognizes its own forced close and exits (fix(agent,gateway): voice interrupts + cascading interrupt hang #6600).
The stale-kill path does not — it only calls
_close_request_client_once("stale_stream_kill")and resets the timer, so thesuperseded worker's death is only as reliable as the socket abort, which can
race / miss the live stream.
sink: late chunks from the superseded stream still flowed through
_fire_stream_delta/_current_streamed_assistant_textalongside the newattempt's chunks.
Fix
Enforce a single-writer invariant on the sink itself, guarded by attempt id
(not only socket state):
chat_completions,anthropic_messages, andbedrockpaths) claims a monotonic writer token via_claim_stream_writer()right before it begins consuming its stream. A newerclaim supersedes any older one.
_fire_stream_delta/_fire_reasoning_delta/_record_streamed_assistant_textdrop chunks from a stale writer (coveringthe tool-suppressed raw-callback path too).
threading.local), so a thread that neverclaimed (a non-streaming delta caller) is never fenced — the guard can only
ever drop a superseded stream, never the single legitimate writer.
_stream_writer_dropped) and logged sparsely so a realprovider problem stays visible instead of being silently swallowed.
Meets the issue's acceptance criteria
transcript records exactly one response for the turn (delta sink guarded by
attempt id, not only by socket state). ✔
asserts single-writer output. ✔
Tests
tests/run_agent/test_stream_single_writer_65991.py(4 tests): supersededwriter fenced across threads, active writer never fenced, non-claiming thread
never treated as a writer, and the real consume loop stops the instant it is
superseded.
scripts/run_tests.sh tests/run_agent/test_stream_single_writer_65991.py tests/agent/test_cascading_interrupt_6600.py tests/run_agent/test_partial_stream_finish_reason.py→ all pass.tests/run_agent/test_streaming.py→ 41 passed, 2 skipped, no new failures(4 pre-existing failures are environmental: the optional
anthropicpackageis not installed).
Useful log line for reproduction
Grep the agent log for the fence firing (and the stale-stream reconnects that
trigger it):