fix(agent): don't peek past a completed stream on a half-open socket (#56) - #215
Open
justrach wants to merge 1 commit into
Open
fix(agent): don't peek past a completed stream on a half-open socket (#56)#215justrach wants to merge 1 commit into
justrach wants to merge 1 commit into
Conversation
…56) The SSE reader's post-line reader.peekByte() ran BEFORE the terminal-event check, which then breaks on [DONE]/response.completed. So on the final line the peek's result was discarded anyway, but its fill could block forever on a half-open socket (server gone, no FIN) that the idle-stall watchdog doesn't guard — hanging an already-complete turn. That is part of #56's "reconnect doesn't reconnect". Move the peek to AFTER the terminal-event break, matching the loop's documented intent ("stop instead of waiting for the socket to close"). A completed response now breaks before peeking; the non-terminal path is byte-for-byte unchanged (peek -> if (!more) break -> toss the buffered '\n'). Safe by inspection: on a terminal line the loop already broke at the isStreamEnd check, so `more` was never read there — skipping the peek only removes a useless, hang-prone fill. Verified: zig build test green + the streaming PTY suite (test-pty-codex-ws/repl/markdown/spinner) green — no regression in the hot read path. Scope: this closes the post-completion subcase. The mid-stream half-open subcase (peekByte blocking on a NON-terminal line) still needs a watchdog-guarded peek plus a half-open-socket mock harness to verify — left as a follow-up. Fix-B (retryable stalls via watchdogError, #134) is already done. Pre-existing zig-fmt debt in this file (unrelated one-liners) left untouched to avoid churn. Refs #56 Co-Authored-By: blackfloofie <265516171+blackfloofie@users.noreply.github.com>
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
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.
Problem
The SSE streaming reader's post-line probe,
reader.peekByte()(agent_stream.zig), ran before the terminal-event check that breaks on[DONE]/response.completed. On the final line of a response the peek's result is discarded anyway (the loop breaks at the terminal check), but its fill can block forever on a half-open socket — server gone, no FIN/RST — which the idle-stall watchdog does not guard. That hangs an already-complete turn: part of #56's "reconnect doesn't reconnect".Fix
Move the peek to after the terminal-event break, which matches this loop's own documented intent — "stop instead of waiting for the socket to close". A completed response now breaks before it ever peeks; the non-terminal path is byte-for-byte unchanged (
peek → if (!more) break → toss the buffered '\n').Safe by inspection: on a terminal line the loop already broke at the
isStreamEndcheck, somorewas never read there — moving the peek only removes a useless, hang-prone fill on completed responses.Verification
zig build testgreen.test-pty-codex-ws,test-pty-repl,test-pty-markdown,test-pty-spinner(these stream model output through this exact reader), so no regression in the hot read path.I explored a mock hang-test but it can't reliably distinguish old-from-new: a complete SSE line always leaves its terminating
\nbuffered (that's the bytereader.toss(1)consumes), so the oldpeekBytereads from the buffer without blocking in every reproducible case. A test both versions pass isn't a meaningful guard, so I rely on the inspection argument + regression suite instead.Scope (honest)
peekByteblocking on a non-terminal line — still needs a watchdog-guarded peek (astreamPeekTaskselect-arm) plus a half-open-socket mock harness (accept, send partial SSE, then network-drop without closing) to verify. Left as a follow-up.watchdogError: Esc→Interrupted, deadline→HungRequest/StreamStalled) is already implemented and tested (Stream stall incorrectly reported as user Esc interruption #134).Pre-existing
zig fmtdebt in this file (unrelated one-liners) left untouched to avoid churn.Refs #56