Clear the SSE write deadline so an agent turn can outlive WriteTimeout #87

Merged

steve merged 3 commits from fix/sse-write-deadline into main 2026-07-22 02:04:23 +00:00

Conversation 6 Commits 3 Files Changed 2

+145 -1

3 Commits

Author	SHA1	Message	Date
steveandClaude Opus 4.8	95b9d611c6	Test the deadline is refreshed per-frame, not just extended (#87) ... Gadfly re-review: the regression test proved the stream outlives the server WriteTimeout, but its whole run was under a second — far below the 30s sseWriteTimeout — so it could NOT tell a per-frame refresh from a deadline set once at open. A revert to set-once would reintroduce the unbounded-block risk the per-frame refresh exists to prevent, and sail through the test. Make sseWriteTimeout a var (production never reassigns it) so a test can shrink it, and split into two focused cases sharing a streamFrames helper: - TestEventStreamOutlivesServerWriteTimeout — server WriteTimeout 300ms, frames straddling it, sseWriteTimeout left at 30s. Catches #78 (no deadline management → server cuts the stream). Huge margin, so CI slowness only makes it pass more surely — addresses the "timing margin tight" finding too. - TestEventStreamRefreshesDeadlinePerFrame — sseWriteTimeout shrunk to 400ms, 8 frames at a 100ms tick (800ms total, 4× margin per gap). A per-frame refresh delivers all 8; a set-once deadline expires mid-stream and cuts it. Verified each fails against its own regression: removing the per-write refresh gives 3/8 on the per-frame test (EOF at ~400ms); removing both deadline calls gives 0/3 on the outlives test. Both pass on the real code, 3× under -count. The "clear the deadline entirely" finding was already addressed by the previous commit (per-frame refresh); this covers it against reintroduction. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 20:49:57 -04:00
steveandClaude Opus 4.8	04cdf815df	Bound each SSE write instead of removing the deadline entirely ... Gadfly, security lens: clearing the write deadline outright traded the truncation bug for an unbounded one. With no deadline, a client that stops reading fills the socket buffer and blocks Write forever — pinning the agent run goroutine and this stream's mutex, which also takes down the keep-alive since it needs the same lock. The old 30s WriteTimeout at least bounded that. Refresh the deadline per frame instead: the stream as a whole is unbounded, but no single write is. That is the only shape that satisfies both ends, since an absolute deadline cuts long turns and no deadline cannot be recovered from. The probe stays in openEventStream so a writer that cannot take deadlines is reported once rather than once per frame; the per-write call deliberately ignores its error for the same reason. Also widened the test's timing margins, per the second finding. tick is now GREATER than writeTimeout, so every frame lands after the deadline has already expired and the margin only ever grows — a loaded CI runner pushes this toward passing rather than toward flaking. Sized the other way it would flake exactly when CI is busiest. Passes 3/3 with -count=3. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 18:23:31 -04:00
steveandClaude Opus 4.8	7088f382bc	Clear the SSE write deadline so a turn can outlive WriteTimeout (#78) ... http.Server.WriteTimeout is an ABSOLUTE deadline from when the request header was read, not an idle timeout. pansy sets it to 30s (cmd/pansy/main.go:65) while an agent turn is budgeted 4 minutes, so every turn over 30 seconds was cut mid-stream. The 4-minute runTimeout was unreachable; the real ceiling was 30 seconds. That also meant the keep-alive added in #73 could never do its job. It ticks at 20s, so it got exactly one tick before the connection it was pacing was destroyed underneath it — a mechanism built to survive long silences that was structurally incapable of surviving them. The failure is invisible from the handler: writes made after the deadline return err == nil and their bytes are discarded. There is no error to check and none to log, which is why this survived a review that specifically looked at the discarded write error. Only the client sees it, as an unexpected EOF, which web/src/lib/agent.ts reports as "The connection dropped partway through." — indistinguishable from a real network fault. Because it is invisible server-side, the test drives a real http.Server with a short WriteTimeout and asserts from the CLIENT side. Against the unfixed code it gets 1 of 4 frames and an unexpected EOF; a test that inspected the return of io.WriteString would have passed against the bug. gin 1.10.1's responseWriter implements Unwrap, so ResponseController reaches the underlying conn. A failure to clear the deadline IS worth logging: it means the stream will be cut and we cannot prevent it. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ	2026-07-21 18:14:12 -04:00