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
2 changed files with 85 additions and 0 deletions
Showing only changes of commit 7088f382bc - Show all commits
+18
View File
@@ -124,10 +124,28 @@ type eventStream struct {
// Headers go out before the first write and the stream is flushed immediately,
// so a proxy holding the response until it looks complete can't reintroduce
// exactly the silence streaming exists to remove.
//
// Clearing the write deadline is what makes a turn longer than the server's
// WriteTimeout possible at all (#78). That timeout is an ABSOLUTE deadline from
// when the request header was read, not an idle timeout, so a streaming response
// has to opt out of it explicitly or it is cut mid-turn no matter how recently
// it wrote. Without this the 4-minute runTimeout is unreachable and the
// keep-alive below tops out at one tick — it would be pacing a connection that
// is destroyed underneath it.
//
// This failure is INVISIBLE from in here: the writes after the deadline return
// err == nil and their bytes are dropped, so there is nothing to detect and
// report. Only the client sees it, as a truncated stream it reports as a dropped
// connection. That is why this is done up front rather than handled on write.
func openEventStream(c *gin.Context) *eventStream {
c.Header("Content-Type", "text/event-stream")
c.Header("Cache-Control", "no-cache")
c.Header("X-Accel-Buffering", "no")
// A failure here is worth saying out loud: it means this stream WILL be cut
// at WriteTimeout and we cannot stop it.
if err := http.NewResponseController(c.Writer).SetWriteDeadline(time.Time{}); err != nil {
slog.Error("api: cannot clear SSE write deadline; long turns will be truncated", "error", err)
}
c.Writer.Flush()
return &eventStream{c: c}
}
+67
View File
@@ -0,0 +1,67 @@
package api
import (
"bufio"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/gin-gonic/gin"
)
// TestEventStreamOutlivesWriteTimeout is the regression test for #78.
//
// http.Server.WriteTimeout is an ABSOLUTE deadline measured from when the
// request header was read — not an idle timeout — so a streaming response is cut
// once it passes, however recently the handler wrote. pansy sets it to 30s while
// an agent turn may run for minutes.
//
// This has to be driven through a real http.Server and asserted from the CLIENT
Review

🟡 Regression test never approaches the 30s deadline boundary, so it can't catch a regression to a single set-once deadline instead of the required per-frame refresh

error-handling · flagged by 1 model

  • internal/api/sse_deadline_test.go:20-45 — The regression test doesn't exercise the "refreshed per frame" mechanism that is the actual fix for #78. openEventStream sets the write deadline via the hardcoded 30s sseWriteTimeout (internal/api/agent.go:120) once, at stream open, and that constant isn't overridable by the test. The test only overrides srv.Config.WriteTimeout to 200ms (internal/api/sse_deadline_test.go:46) but total runtime is frames(3) * tick(300ms) ≈ 900ms — three ord…

🪰 Gadfly · advisory

🟡 **Regression test never approaches the 30s deadline boundary, so it can't catch a regression to a single set-once deadline instead of the required per-frame refresh** _error-handling · flagged by 1 model_ - `internal/api/sse_deadline_test.go:20-45` — The regression test doesn't exercise the "refreshed per frame" mechanism that is the actual fix for #78. `openEventStream` sets the write deadline via the hardcoded 30s `sseWriteTimeout` (`internal/api/agent.go:120`) once, at stream open, and that constant isn't overridable by the test. The test only overrides `srv.Config.WriteTimeout` to 200ms (`internal/api/sse_deadline_test.go:46`) but total runtime is `frames(3) * tick(300ms) ≈ 900ms` — three ord… <sub>🪰 Gadfly · advisory</sub>
// side, because the failure cannot be observed from the handler: writes made
// after the deadline return err == nil and their bytes are silently discarded.
// A test that checked the return of io.WriteString would pass against the bug.
func TestEventStreamOutlivesWriteTimeout(t *testing.T) {
// Long enough that the frames below straddle it, short enough to keep the
// test quick. Everything else is scaled off it.
const writeTimeout = 300 * time.Millisecond
const tick = 200 * time.Millisecond
const frames = 4 // last one lands at ~800ms, well past the deadline
gin.SetMode(gin.TestMode)
r := gin.New()
r.GET("/stream", func(c *gin.Context) {
s := openEventStream(c)
for i := 0; i < frames; i++ {
time.Sleep(tick)
s.send(chatEvent{Error: "frame"})
}
})
srv := httptest.NewUnstartedServer(r)
srv.Config.WriteTimeout = writeTimeout
srv.Start()
defer srv.Close()
resp, err := srv.Client().Get(srv.URL + "/stream")
if err != nil {
t.Fatalf("get: %v", err)
}
defer resp.Body.Close()
got := 0
sc := bufio.NewScanner(resp.Body)
for sc.Scan() {
if strings.HasPrefix(sc.Text(), "data: ") {
got++
}
}
// Read errors are the symptom, not an aside: against the bug the client gets
// an unexpected EOF partway through. Report it rather than only the count.
if err := sc.Err(); err != nil {
t.Errorf("client read error after %d/%d frames: %v", got, frames, err)
}
if got != frames {
t.Errorf("client received %d frames, want %d — the stream was cut at WriteTimeout", got, frames)
}
}