Long agent turns are cut at exactly 30s on the live instance with "The connection dropped partway through." That is the #78 failure — the server's absolute WriteTimeout — which the #78 fix and its tests say is handled.
It never was, in production. slog-gin unconditionally replaces c.Writer with a wrapper that embeds the gin.ResponseWriterinterface, which has no Unwrap(). An http.ResponseController built from the handler's writer unwraps layer by layer looking for SetWriteDeadline, can't see through that wrapper, and returns ErrNotSupported — so the stream falls back to the server's 30s WriteTimeout. The first write past it fails, which cancels the request context and closes the socket under the client mid-frame; the browser's reader.read() throws, and the UI reports the drop. The tests passed because they host openEventStream on a bare gin.New() with no middleware.
The scan upload's read/write deadline extensions (seed_packet.go) fail the same way, with the errors discarded.
Visible in the container log as ERROR api: SSE write deadlines unavailable … error="feature not supported" at the start of every chat, and a POST /api/v1/agent/chat request line with response.latency of 30–50s.
The fix
internal/api/deadlines.go: captureController() runs first on the engine and stashes a controller built from gin's raw writer, before anything wraps it; responseController(c) hands it to handlers (falling back to c.Writer on a middleware-less test engine).
openEventStream and scanSeedPacket take their controller from responseController(c); the scan path now logs once instead of swallowing the error.
Regression tests run the stream through api.New() — the real stack, in the real order — and assert from the client side. TestEventStreamOutlivesWriteTimeoutBehindMiddleware fails on main with client read error after 0/3 frames: unexpected EOF, the exact live failure.
CLAUDE.md records the convention and the "test on gin.New()" trap.
## The bug
Long agent turns are cut at exactly 30s on the live instance with **"The connection dropped partway through."** That is the #78 failure — the server's absolute `WriteTimeout` — which the #78 fix and its tests say is handled.
It never was, in production. `slog-gin` unconditionally replaces `c.Writer` with a wrapper that embeds the `gin.ResponseWriter` *interface*, which has no `Unwrap()`. An `http.ResponseController` built from the handler's writer unwraps layer by layer looking for `SetWriteDeadline`, can't see through that wrapper, and returns `ErrNotSupported` — so the stream falls back to the server's 30s `WriteTimeout`. The first write past it fails, which cancels the request context and closes the socket under the client mid-frame; the browser's `reader.read()` throws, and the UI reports the drop. The tests passed because they host `openEventStream` on a bare `gin.New()` with no middleware.
The scan upload's read/write deadline extensions (`seed_packet.go`) fail the same way, with the errors discarded.
Visible in the container log as `ERROR api: SSE write deadlines unavailable … error="feature not supported"` at the start of every chat, and a `POST /api/v1/agent/chat` request line with `response.latency` of 30–50s.
## The fix
- `internal/api/deadlines.go`: `captureController()` runs **first** on the engine and stashes a controller built from gin's raw writer, before anything wraps it; `responseController(c)` hands it to handlers (falling back to `c.Writer` on a middleware-less test engine).
- `openEventStream` and `scanSeedPacket` take their controller from `responseController(c)`; the scan path now logs once instead of swallowing the error.
- Regression tests run the stream through `api.New()` — the real stack, in the real order — and assert from the client side. `TestEventStreamOutlivesWriteTimeoutBehindMiddleware` fails on `main` with `client read error after 0/3 frames: unexpected EOF`, the exact live failure.
- CLAUDE.md records the convention and the "test on `gin.New()`" trap.
`GOWORK=off go test ./...` green, gofmt clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Long agent turns were cut at exactly 30s on the live instance with "The
connection dropped partway through." — the #78 failure, which its tests
said was fixed. The tests host openEventStream on a bare gin.New(); in
production, slog-gin replaces c.Writer with a wrapper that embeds the
gin.ResponseWriter interface, which has no Unwrap, so the ResponseController
built from the handler's writer can't reach the connection and every
SetWriteDeadline returns ErrNotSupported. The stream fell back to the
server's absolute WriteTimeout; the first write past it failed, cancelled
the request context, and closed the socket under the client mid-frame.
The scan upload's read/write extensions failed the same way, with the
errors discarded.
captureController now runs first on the engine and stashes a controller
built before anything wraps the writer; openEventStream and scanSeedPacket
take it from responseController(c). The regression tests run the stream
through New() — the real stack, in the real order — and check from the
client side; the scan path logs once instead of swallowing the error.
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 03:08:04Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Same middleware/Unwrap rationale re-narrated in deadlines.go, agent.go, sse_deadline_test.go, and CLAUDE.md — secondary copies will drift; keep the full explanation only in deadlines.go and point back from the others
maintainability · flagged by 1 model
Duplicated mechanism explanation across four locations — The same slog-gin / Unwrap / ErrNotSupported failure-mode narrative is written out in near-identical prose in internal/api/deadlines.go:16-25, internal/api/agent.go:168-171, internal/api/sse_deadline_test.go:117-122, and CLAUDE.md:163-169. deadlines.go is where the mechanism actually lives, so it is the natural single home for the full explanation; agent.go already appends "(see deadlines.go)" yet still re-narrates th…
🪰 Gadfly · advisory
🟡 **Same middleware/Unwrap rationale re-narrated in deadlines.go, agent.go, sse_deadline_test.go, and CLAUDE.md — secondary copies will drift; keep the full explanation only in deadlines.go and point back from the others**
_maintainability · flagged by 1 model_
- **Duplicated mechanism explanation across four locations** — The same slog-gin / `Unwrap` / `ErrNotSupported` failure-mode narrative is written out in near-identical prose in `internal/api/deadlines.go:16-25`, `internal/api/agent.go:168-171`, `internal/api/sse_deadline_test.go:117-122`, and `CLAUDE.md:163-169`. `deadlines.go` is where the mechanism actually lives, so it is the natural single home for the full explanation; `agent.go` already appends "(see deadlines.go)" yet still re-narrates th…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Minor issues · 1 finding (0 with multi-model agreement)
1 single-model finding (lower confidence)
Finding
Where
Model
Lens
🟡
Same middleware/Unwrap rationale re-narrated in deadlines.go, agent.go, sse_deadline_test.go, and CLAUDE.md — secondary copies will drift; keep the full explanation only in deadlines.go and point back from the others
internal/api/agent.go:168
glm-5.2:cloud
maintainability
Per-model detailclaude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I reviewed this change through the security lens and verified the relevant code by reading internal/api/deadlines.go, api.go, and seed_packet.go at the checked-out state.
The change is a plumbing fix: a first-in-chain middleware (captureController) builds an http.ResponseController from gin's raw writer before slog-gin wraps it, and handlers pull it via responseController(c) to set read/write deadlines. Nothing here touches authn/authz, parsing of untrusted data, injection sinks, SSRF, deserialization, or secrets.
Things I checked specifically for security impact and found clean:
No new tainted-data path. The controller is built from c.Writer, not from any request-controlled value. c.Set/c.Get are request-scoped, and a fresh controller is created per request — no cross-request state leakage.
Middleware ordering is safe.captureController only does c.Set + c.Next(); placing it ahead of gin.Recovery() doesn't defeat panic recovery (Recovery's deferred recover still wraps all downstream handlers) and it doesn't short-circuit the auth/CSRF/admin guards that remain in place on their groups (seed-lots, /agent etc. are still behind requireAuth).
No secret leakage in the new log line.seed_packet.go:50 logs only the deadline error ("feature not supported"), no credentials or request content.
Upload endpoint remains bounded.scanSeedPacket still wraps the body in http.MaxBytesReader(…, scanUploadLimit) (seed_packet.go:54) and is behind requireAuth (api.go:186,194).
One thing I considered and concluded is not a material finding: the fix makes the scan read-deadline extension (15s → 60s) actually take effect, which genuinely lengthens how long a client can hold that connection. But the endpoint is authenticated, the deadlines are bounded (60s read / 120s write), and the body is byte-capped, so this is intended availability tuning, not an anonymous slow-loris vector. Verified via requireAuth on the seed-lots group and the MaxBytesReader cap.
Nothing in the security lens warrants a change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed this change through the Correctness lens and verified the mechanism against the checked-out code and dependencies rather than the diff alone.
What I checked and confirmed:
The captured controller reaches the real socket.captureController() runs as the first middleware (api.go:46) and builds http.NewResponseController(c.Writer) while c.Writer is still gin's own *responseWriter. gin v1.10.1's responseWriter implements Unwrap() (present since gin 1.9), so the controller unwraps gin's writer → the underlying *http.response, which supports SetWriteDeadline/SetReadDeadline. Storing it before sloggin (v1.15.0) wraps c.Writer is exactly what dodges the no-Unwrap wrapper. The deadline is set on the same connection the later writes flush through, so there's no target mismatch.
The middleware actually runs for the test routes.authEngine builds the engine via New() (auth_test.go:31), and streamFrames/the deadline test register /stream and /deadlinesafterNew() returns. gin applies engine-level Use() middleware (registered inside New) to routes added later, so captureController is in the chain for both. The routes are top-level, not under the requireAuth group, so the unauthenticated test GET reaches the handler — the assertions (got == 3, readErr/writeErr == nil) are meaningful, not silently skipped by a 401.
The regression test genuinely fails on main. Without captureController, openEventStream built the controller from the handler's c.Writer (the sloggin wrapper) → ErrNotSupported → stream falls back to the 300ms server WriteTimeout → cut after ~1 frame. So TestEventStreamOutlivesWriteTimeoutBehindMiddleware is a true regression guard, not a fixture that asserts its own assumptions.
No stale call sites.grep for NewResponseController/Set*Deadline shows the only two production handlers extending deadlines (agent.go, seed_packet.go) both now route through responseController(c); nothing else uses the old c.Writer pattern.
responseController fallback is sound. The type assertion on the stored value guards a missing/other key; the fallback to http.NewResponseController(c.Writer) only triggers on a middleware-less test engine, as documented.
The bareEngine() refactor preserves the first two tests' original bare-host behavior; the serverWriteTimeout/tick/frames values and the straddling logic are unchanged.
The seed_packet change (log the read-deadline error once, keep the second unchecked) is a behavior tweak with no correctness consequence — best-effort semantics are preserved.
No logic, unit, threshold, or formula errors in my lane.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
I reviewed this through the code cleanliness & maintainability lens and verified the change against the checked-out code (read deadlines.go, agent.go, seed_packet.go, api.go, auth_test.go, and grepped all captureController/responseController usages).
The change is clean and idiomatic for this codebase:
Single canonical explanation, with pointers. The mechanism (logging middleware embeds the gin.ResponseWriterinterface, no Unwrap) is documented once in deadlines.go:13-30; the four other touch points (agent.go:168, api.go:42, seed_packet.go:44, the test) each carry a short note that defers to it. That's the established verbose-narrative comment style here, not copy-paste rot.
Names are clear and consistent.captureController/responseController/responseControllerKey read plainly and the key is namespaced (pansy.responseController).
The c.Writer fallback is intentional and correctly scoped to middleware-less test engines (deadlines.go:38-48), matching the code comment and the bareEngine() helper the tests introduce.
The asymmetric error handling in scanSeedPacket (log on SetReadDeadline, discard on SetWriteDeadline, seed_packet.go:49-52) is deliberate and mirrors the same "report once, second call fails identically" pattern already used in openEventStream/write (agent.go:181,204) — consistent, not an oversight.
Test refactor is tidy. Threading r *gin.Engine into streamFrames and extracting bareEngine() avoids duplicating server setup, and the new middleware-stack test reuses the existing authEngine/localCfg helpers rather than hand-rolling a stack.
No dead code, no leaky abstraction, no duplication that should be shared but isn't. Nothing material in my lane.
⚡ Performance — No material issues found
I've verified the change against the checked-out code. Reviewing strictly through the Performance lens.
VERDICT: No material issues found
The change is deadline-plumbing, not a data path. I checked the three things that could matter for performance:
captureController runs on every request (internal/api/deadlines.go:31-36, wired at internal/api/api.go:46). Each request now does one http.NewResponseController(c.Writer) (a single tiny &ResponseController{rw} alloc) plus a c.Set map insert. This is on all routes, including static SPA assets and healthz. But NewResponseController is trivial, gin's Keys map is already allocated on most authenticated paths (requireAuth/requireAdmin call c.Set), and this is a low-traffic self-hosted app. This is a micro-optimization, not a justifiable regression — not flagging it as a finding.
The SSE hot path is unchanged in cost.openEventStream fetches the controller once via a single c.Get map lookup (agent.go:176), and write() still does exactly one SetWriteDeadline per frame (agent.go:204) — same per-frame work as before, just against a controller that now actually reaches the socket. No added per-frame allocation or work in the loop. The keepAlive goroutine (agent.go:213-238) is untouched.
No new N+1, unbounded growth, or blocking calls. The scan path (seed_packet.go) swaps http.NewResponseController for responseController(c) and adds one error log — no per-request cost change beyond that.
Nothing in this diff introduces an efficiency regression I can justify flagging.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this change through the error handling & edge cases lens, reading internal/api/deadlines.go, the openEventStream/write path in agent.go, the full scanSeedPacket in seed_packet.go, and the test wiring in auth_test.go/sse_deadline_test.go.
Nothing is materially wrong in my lane. The unhappy paths this diff introduces are handled soundly:
responseController(c) cannot return nil / cannot panic (deadlines.go:41). Both the stored-value path and the failed-type-assertion/missing-key fallback return a live *http.ResponseController (http.NewResponseController never returns nil). The double ok guard (c.Get + type assert) means a wrong-typed context value degrades to the fallback rather than a panic. Verified by reading the function.
The scan path now surfaces a previously-swallowed error (seed_packet.go:49-51) — a strict improvement in this lens. The remaining swallowed SetWriteDeadline (seed_packet.go:52) is a deliberate, documented no-double-report: for an http.ResponseController the read/write deadline setters resolve to the same underlying rwc, so the second call fails iff the first did. The claim holds; not a bug.
The per-frame _ = s.rc.SetWriteDeadline(...) (agent.go:204) is likewise intentionally unchecked — openEventStream already probes and logs once (agent.go:181-183), and the comment correctly notes a write past a dropped deadline returns nil anyway, so there is genuinely nothing actionable per-frame.
No new cleanup/defer/rollback obligation is introduced. captureController only does c.Set + c.Next(); the stored controller lives on the pooled gin.Context and is reset per request, so there's no cross-request leakage or lifetime edge case. Sitting ahead of gin.Recovery() is harmless — it does no post-Next() work that a downstream panic could strand.
Test unhappy-path wiring is real: authEngine (auth_test.go:20) routes through New(), so the new regression tests exercise the actual middleware order rather than asserting a mental model.
No off-by-one, nil, empty-collection, zero/negative, or overflow surface is touched by this change.
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
This PR is a pure deadline-management fix. Through the security lens:
captureController() added as first engine middleware: It only calls c.Set(responseControllerKey, ...) and c.Next(). It captures nothing from the request and grants no access. All authentication — h.requireAuth(), h.requireAdmin(), CSRF guard — is applied at the route-group level, not the engine level, so this ordering change doesn't touch any auth boundary.
Context key "pansy.responseController": The key is a string constant, properly namespaced. c.Get/c.Set on a gin context is never visible to the HTTP client — there's no path by which a caller can inject or read context values. The double type-assert in responseController() (v.(*http.ResponseController)) is safe; a failed assertion triggers the fallback rather than a panic.
Deadline extension as a DoS surface: The two handlers that call responseController — openEventStream and scanSeedPacket — are both behind h.requireAuth(). An authenticated user could keep connections alive up to 60s/120s rather than 30s, but that's within the same authorization scope they already have. The server's connection limits aren't changed.
Log messages in the new error paths (slog.Error in scanSeedPacket): the error value is from rc.SetReadDeadline, which is an internal net error ("feature not supported"). No user-controlled data flows into these log lines.
Fallback in responseController: The http.NewResponseController(c.Writer) fallback on a test engine without captureController reproduces the pre-existing (broken) behavior rather than introducing anything new, and only reachable on gin.New() engines — not in production.
Nothing in this diff creates an authn/authz gap, injection vector, or untrusted-data exposure.
🎯 Correctness — No material issues found
I have enough to decide. The draft's finding rests on two specific claims that don't hold up:
"For a 204 No Content response the net/http client sets resp.Body to http.NoBody" — This is wrong. Go's net/http documentation guarantees that Response.Body is always non-nil and backed by a real reader (an empty one for bodyless responses, but not http.NoBody). resp.Body.Close() is not a no-op for 204 responses.
"the race detector's happens-before graph does not include [the network round-trip] directly" — Also wrong in the relevant sense. Go's net/http internally uses sync.Mutex throughout the write path (server side) and read path (client side). Those are tracked happens-before edges. The chain is: readErr = … → (program-order in handler goroutine) → sync.Mutex.Unlock() in net/http response write → (happens-before, tracked by race detector) → sync.Mutex.Lock() in net/http response read, which is internal to Get() → (program-order in test goroutine) → read of readErr. The race detector follows this chain and will not flag it.
This exact pattern — shared variables written in a handler goroutine, read by the test goroutine after Get() or Body.Close() returns — is ubiquitous in Go's own standard-library HTTP tests, all of which run under -race. The pattern is safe in practice, and the finding's confidence rating of "high" with the "candidate race report" framing is not confirmable.
No material issues found.
The production change is correct. captureController is registered first in api.New, builds the http.ResponseController from gin's unwrappable raw writer before sloggin replaces c.Writer, stores it in the context, and responseController(c) retrieves it. All three call sites (openEventStream, scanSeedPacket) now use responseController(c). The new test TestEventStreamOutlivesWriteTimeoutBehindMiddleware runs through authEngine (which calls New), so captureController is present and the test actually exercises the production code path. TestResponseControllerReachesTheSocketBehindMiddleware correctly asserts that SetReadDeadline/SetWriteDeadline return nil through the full middleware stack. No issues survived verification.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The new abstraction is tidy: deadlines.go gives the pattern a single home, both call sites (agent.go:176, seed_packet.go:48) are updated consistently, and the bareEngine() extract is a clean de-duplication of a setup idiom that was repeated inline across three tests.
The comment block added to openEventStream (lines 168–171) restates roughly what deadlines.go already says in full, but it serves as a warning label at the exact call site that was silently regressed before, which the project's documentation conventions explicitly endorse for non-obvious production traps. It's not duplication that will bite anyone.
Nothing else to flag through this lens.
⚡ Performance — No material issues found
No material issues found
The two performance-relevant paths are both clean:
captureController() middleware — allocates one *http.ResponseController per request (a trivially small struct wrap) and stores it with a single c.Set call. This runs on every request including routes that never extend a deadline, but the cost is negligible: one allocation and one map write per request regardless of route.
responseController(c) / per-frame SSE — the controller is fetched from the context map exactly once, when openEventStream constructs the eventStream struct (s := &eventStream{c: c, rc: responseController(c)}). The per-frame hot loop (write) then calls s.rc.SetWriteDeadline(...) on the already-retrieved pointer — zero additional context lookups or allocations per frame. The design correctly avoids the obvious trap of re-fetching from the context inside the loop.
No N+1 patterns, no unbounded growth, no blocking on hot paths, no avoidable allocations in the SSE stream.
🧯 Error handling & edge cases — No material issues found
No material issues found.
The error-handling paths in this PR are clean under my lens:
captureController: Stores the controller before c.Next(), so it's available regardless of what downstream middleware does or whether they abort. No rollback or cleanup needed — there's nothing to undo when storing a value in the gin context.
responseController fallback: The double-ok check (one from c.Get, one from the type assertion) is robust against a non-*http.ResponseController value ever landing under that key. Returns a freshly-built controller in that case rather than panicking.
scanSeedPacket error handling: The asymmetry between logged SetReadDeadline and silently-discarded SetWriteDeadline is explicitly documented with the rationale (both calls fail through the same ErrNotSupported path, logging the second one would just repeat the first). For Go's standard HTTP server, a writer that supports read deadlines always supports write deadlines and vice versa — the same http.rwc implements both. The assumption holds.
openEventStream probe + suppress pattern: Checks deadline support once on open, logs if unsupported, then silently ignores per-frame deadline errors. This is correct — the probe already told the operator everything; repeating it per frame would flood the log for no new information.
write() mutex: The eventStream.write method holds s.mu for the full deadline-set + write-string + flush sequence. The keepAlive goroutine also uses write, so concurrent access to the ResponseController is properly serialized.
TestResponseControllerReachesTheSocketBehindMiddleware: Captures readErr/writeErr in the test goroutine and reads them after resp.Body.Close(). In Go's runtime, the net/http write path through the netpoller provides sufficient memory ordering between the handler goroutine and the caller of Get() — this pattern is used throughout the standard library's own tests and does not trigger the race detector.
Nothing in the unhappy paths or boundary conditions here looks broken or missing.
The security lens here: the controller is captured from c.Writer in the capture middleware and stored in the gin context by key. The controller is then used to set read/write deadlines. No untrusted input flows into this. The key is a constant. The deadline values are constants. No injection, no authz change.
One thing to check: captureController stores the controller in the gin context. gin context keys are a shared namespace, but the key is a package-private constant string. No collision risk exposed externally. The controller itself is not user-influenced.
The change doesn't alter authn/authz ordering — captureController runs before logging/recovery but after... actually it's the first middleware. It doesn't bypass requireAuth or CSRF; those are route-group middleware applied later. No security gap.
No material security issues found.
VERDICT: No material issues found
Reviewed internal/api/deadlines.go, internal/api/api.go, internal/api/seed_packet.go, internal/api/agent.go, and the test helpers. The change captures an http.ResponseController from gin's raw writer before logging middleware wraps it, stores it under a package-private constant key (responseControllerKey), and hands it to handlers that extend deadlines. No untrusted input taints the controller or the deadline values (all are time.Now().Add(<const>)). The key is not externally settable and lives in gin's per-request context, so there's no cross-request leakage or injection surface. captureController is added as the first engine middleware but does not bypass or reorder any authn/authz/CSRF middleware — csrfGuard, requireAuth, etc. are route-group middleware registered after it, so the authorization posture is unchanged. No secret/credential logging is introduced (only error values are logged). Nothing in the security lens is materially wrong.
🎯 Correctness — No material issues found
Verdict: No material issues found.
Reviewed through the Correctness lens. The fix is sound.
Key verifications:
captureController() runs first in api.New (api.go:46), capturing a controller from c.Writer before sloggin wraps it. Confirmed by reading api.go:38-46.
responseController(c) (deadlines.go:41-48) returns the stashed controller, falling back to a fresh http.NewResponseController(c.Writer) only when the key is absent — the bare-engine test path. Verified.
openEventStream (agent.go:176) and scanSeedPacket (seed_packet.go:48-52) both source their controller via responseController(c). Verified.
The mechanism claim is correct: an http.ResponseController unwraps via Unwrap(); a wrapper embedding the gin.ResponseWriterinterface (no Unwrap) blocks that, yielding ErrNotSupported. Building the controller ahead of the wrapper sidesteps it — the controller holds a reference to the underlying gin writer, which does unwrap to the net/http writer. This is the documented ResponseController behavior.
The new test TestEventStreamOutlivesWriteTimeoutBehindMiddleware uses authEngine → New → the real middleware order (auth_test.go:31), so it exercises the actual stack rather than a bare engine. Confirmed.
Scan-path deadline semantics unchanged: scanReadTimeout/scanWriteTimeout (60s/120s) applied as before, now through the working controller; the second call's error is deliberately not re-logged, which matches the comment (a writer that can't take deadlines fails identically on both). No behavior regression.
Constants/thresholds re-derived: server WriteTimeout=30s (main.go:65), ReadTimeout=15s (main.go:63); the scan extension to 60s/120s and the SSE per-frame sseWriteTimeout=30s (agent.go:134) are all relative time.Now().Add(...) per-write deadlines, which is exactly the correct shape to override an absolute server timeout. No unit/conversion errors.
No correctness defects introduced.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Duplicated mechanism explanation across four locations — The same slog-gin / Unwrap / ErrNotSupported failure-mode narrative is written out in near-identical prose in internal/api/deadlines.go:16-25, internal/api/agent.go:168-171, internal/api/sse_deadline_test.go:117-122, and CLAUDE.md:163-169. deadlines.go is where the mechanism actually lives, so it is the natural single home for the full explanation; agent.go already appends "(see deadlines.go)" yet still re-narrates the mechanism in lines 168-170, and sse_deadline_test.go:117-122 re-narrates it in full with no back-reference at all. When the middleware stack changes, four copies must be kept in step, and the three secondary ones are the ones most likely to drift and then mislead. Suggested fix: trim agent.go:168-171 and sse_deadline_test.go:117-122 to a one-line "see deadlines.go for why" and keep the full explanation only in deadlines.go (and the CLAUDE.md convention, which is the appropriate place for the rule itself). Verified by reading all four blocks.
Asymmetric error handling in seed_packet.go:49-52 — SetReadDeadline is checked and logged, while SetWriteDeadline is discarded with _ =. The inline comment (seed_packet.go:46-47) justifies this deliberately ("the second call can only fail the same way as the first"), and the same convention is used in agent.go:202-204. The asymmetry reads as a smell on first encounter but is intentional and documented; no change needed beyond keeping the comment.
The structural changes are clean: responseController/captureController are a small, well-named abstraction with a clear single responsibility; the tests reuse the existing authEngine/localCfg helpers rather than duplicating engine setup; bareEngine() is a tidy extraction of the previous inline gin.New(); and streamFrames taking the engine as a parameter is a low-churn generalization of the existing helper.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this change is neutral-to-slightly-positive:
responseController(c) is called once per request/stream, not per frame. The SSE hot path (eventStream.write at agent.go:204) reuses the controller stashed in s.rc at stream open (agent.go:176), so there is no per-frame context-map lookup or controller construction. The scan path (seed_packet.go:48) calls it once per upload.
captureController builds one http.ResponseController per request and stores it in the gin context (deadlines.go:33). This replaces the previous per-call http.NewResponseController(c.Writer) in openEventStream/scanSeedPacket, so allocation behavior is at worst unchanged and is now shared/cached rather than rebuilt at each deadline-extending call site.
The c.Get + double type-assertion in responseController (deadlines.go:42-46) is O(1) map lookup under the gin context mutex, once per request — not a hot-loop concern.
No new unbounded growth, no N+1, no blocking calls introduced on the SSE write path.
I verified by reading deadlines.go, api.go, agent.go:160-207, and grepping all responseController call sites — none sit inside a per-frame/per-write loop.
🧯 Error handling & edge cases — No material issues found
Through the error-handling / edge-case lens, the change is carefully constructed. I verified each unhappy path:
deadlines.go:41-47responseController handles both a missing key (c.Get → ok==false) and a wrong-typed value (inner type assertion fails) by falling back to http.NewResponseController(c.Writer). No nil deref — c.Get returns (nil, false) when absent, and both branches are guarded.
agent.go:181-183 the SSE probe logs a deadline failure once but deliberately continues (best-effort, documented), so a controller that can't reach the socket degrades to the server default rather than panicking or aborting.
seed_packet.go:49-52 the read-deadline error is now logged once; the write-deadline error is discarded with an explicit justification (same underlying ErrNotSupported cause). I checked: on a real http connection both setters are supported or neither is, so the "can only fail the same way" claim holds — no silent distinct write-deadline failure is introduced.
captureController builds the controller per-request before any wrapper, so no stale/nil controller is stashed; c.Next() is called so the chain always proceeds.
No swallowed errors that matter, no missing cleanup, no nil/empty/boundary hazards in the new code. The new tests' resp.Body.Close() error is ignored, which is standard test hygiene and not material.
Verdict: No material issues found.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The change correctly fixes a production bug where http.ResponseController couldn't unwrap through slog-gin's writer wrapper to reach the socket's deadline setters.
The new captureController() middleware captures a controller from the raw gin writer before any wrapping occurs, and stashes it under a package-private context key. No user-controlled input reaches the key, the controller, or the deadline calls.
responseController() safely falls back to http.NewResponseController(c.Writer) for bare test engines.
No new injection surfaces, SSRF vectors, secret leakage, authn/authz gaps, or unsafe deserialization introduced. The error logging in seed_packet.go is safe and does not expose sensitive data.
🎯 Correctness — No material issues found
Verdict: No material issues found
I've read the changed files (deadlines.go, api.go, agent.go, seed_packet.go, sse_deadline_test.go) and verified cross-file dependencies (auth_test.go, grep for remaining NewResponseController calls). The logic is sound:
captureController() runs first in the middleware chain and stores an http.ResponseController built from gin's native c.Writer, before sloggin wraps it in an interface-embedding type that blocks unwrapping.
responseController(c) retrieves that stored controller, falling back to http.NewResponseController(c.Writer) on bare engines (test-only path).
Both openEventStream and scanSeedPacket now use responseController(c), so their SetWriteDeadline/SetReadDeadline calls reach the underlying net.Conn in production.
The regression tests (TestEventStreamOutlivesWriteTimeoutBehindMiddleware, TestResponseControllerReachesTheSocketBehindMiddleware) exercise the real middleware stack via authEngine → New(), not the bare gin.New() that masked the bug.
Error handling in scanSeedPacket logs the first deadline error once rather than discarding both silently; the second call is intentionally ignored because it can only fail the same way (ErrNotSupported).
Constants and behavior re-derived from first principles:
http.ResponseController unwraps the writer it was built from layer-by-layer looking for SetWriteDeadline/SetReadDeadline.
gin's native writer implements Unwrap(); slog-gin's wrapper embeds the gin.ResponseWriter interface, which has no Unwrap().
Capturing the controller before the wrapper is therefore the correct fix, and the fallback for bare engines is safe because gin's native writer unwraps cleanly.
No logic bugs, incorrect constants, or semantic errors introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
The fix correctly moves ResponseController creation from per-handler (where it fails behind middleware wrappers) to a single per-request capture in captureController(). From a performance perspective:
No extra work on hot paths: responseController(c) is called once per SSE stream and once per scan upload; the controller is then reused. Per-frame SetWriteDeadline calls and their time.Now() overhead are unchanged from the pre-broken state.
No new allocations in loops: The middleware does a single http.NewResponseController and c.Set per HTTP request — negligible and necessary.
No N+1 or quadratic behavior introduced.
No unbounded growth or missing limits: The SSE stream already refreshes deadlines per frame, preventing unbounded blocking.
The architectural change (capture early, reuse) is strictly better than the prior fallback of building a broken controller on every call path.
🧯 Error handling & edge cases — No material issues found
No material issues found
Error handling: The diff correctly moves deadline-extension errors from silently swallowed (_ = ...) to logged once (slog.Error), both for SSE (agent.go:181) and scan (seed_packet.go:49). The per-frame SSE write (agent.go:204) and the second scan write (seed_packet.go:52) remain deliberately unchecked to avoid spam; the comments explain why (same controller, same failure mode), which is reasonable.
Edge cases:responseController (deadlines.go:41) safely falls back to http.NewResponseController(c.Writer) if the stored key is missing or the type assertion fails. No nil-dereference risk: captureController stores a non-nil controller, and the fallback path also receives gin's always-non-nil c.Writer. No integer overflow or boundary issues in the constants used.
Cleanup:captureController allocates no resources needing defer or rollback; it merely stores a pointer in the gin context.
The unhappy paths (missing middleware, unsupported writer) are handled with graceful fallbacks and explicit logging.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 1 finding (0 with multi-model agreement)
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | Same middleware/Unwrap rationale re-narrated in deadlines.go, agent.go, sse_deadline_test.go, and CLAUDE.md — secondary copies will drift; keep the full explanation only in deadlines.go and point back from the others | `internal/api/agent.go:168` | glm-5.2:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed this change through the security lens and verified the relevant code by reading `internal/api/deadlines.go`, `api.go`, and `seed_packet.go` at the checked-out state.
The change is a plumbing fix: a first-in-chain middleware (`captureController`) builds an `http.ResponseController` from gin's raw writer before `slog-gin` wraps it, and handlers pull it via `responseController(c)` to set read/write deadlines. Nothing here touches authn/authz, parsing of untrusted data, injection sinks, SSRF, deserialization, or secrets.
Things I checked specifically for security impact and found clean:
- **No new tainted-data path.** The controller is built from `c.Writer`, not from any request-controlled value. `c.Set`/`c.Get` are request-scoped, and a fresh controller is created per request — no cross-request state leakage.
- **Middleware ordering is safe.** `captureController` only does `c.Set` + `c.Next()`; placing it ahead of `gin.Recovery()` doesn't defeat panic recovery (Recovery's deferred recover still wraps all downstream handlers) and it doesn't short-circuit the auth/CSRF/admin guards that remain in place on their groups (`seed-lots`, `/agent` etc. are still behind `requireAuth`).
- **No secret leakage in the new log line.** `seed_packet.go:50` logs only the deadline error (`"feature not supported"`), no credentials or request content.
- **Upload endpoint remains bounded.** `scanSeedPacket` still wraps the body in `http.MaxBytesReader(…, scanUploadLimit)` (`seed_packet.go:54`) and is behind `requireAuth` (`api.go:186`,194).
One thing I considered and concluded is *not* a material finding: the fix makes the scan read-deadline extension (15s → 60s) actually take effect, which genuinely lengthens how long a client can hold that connection. But the endpoint is authenticated, the deadlines are bounded (60s read / 120s write), and the body is byte-capped, so this is intended availability tuning, not an anonymous slow-loris vector. Verified via `requireAuth` on the `seed-lots` group and the `MaxBytesReader` cap.
Nothing in the security lens warrants a change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed this change through the **Correctness** lens and verified the mechanism against the checked-out code and dependencies rather than the diff alone.
What I checked and confirmed:
- **The captured controller reaches the real socket.** `captureController()` runs as the *first* middleware (`api.go:46`) and builds `http.NewResponseController(c.Writer)` while `c.Writer` is still gin's own `*responseWriter`. gin v1.10.1's `responseWriter` implements `Unwrap()` (present since gin 1.9), so the controller unwraps gin's writer → the underlying `*http.response`, which supports `SetWriteDeadline`/`SetReadDeadline`. Storing it before `sloggin` (v1.15.0) wraps `c.Writer` is exactly what dodges the no-`Unwrap` wrapper. The deadline is set on the same connection the later writes flush through, so there's no target mismatch.
- **The middleware actually runs for the test routes.** `authEngine` builds the engine via `New()` (`auth_test.go:31`), and `streamFrames`/the deadline test register `/stream` and `/deadlines` *after* `New()` returns. gin applies engine-level `Use()` middleware (registered inside `New`) to routes added later, so `captureController` is in the chain for both. The routes are top-level, not under the `requireAuth` group, so the unauthenticated test `GET` reaches the handler — the assertions (`got == 3`, `readErr/writeErr == nil`) are meaningful, not silently skipped by a 401.
- **The regression test genuinely fails on `main`.** Without `captureController`, `openEventStream` built the controller from the handler's `c.Writer` (the sloggin wrapper) → `ErrNotSupported` → stream falls back to the 300ms server `WriteTimeout` → cut after ~1 frame. So `TestEventStreamOutlivesWriteTimeoutBehindMiddleware` is a true regression guard, not a fixture that asserts its own assumptions.
- **No stale call sites.** `grep` for `NewResponseController`/`Set*Deadline` shows the only two production handlers extending deadlines (`agent.go`, `seed_packet.go`) both now route through `responseController(c)`; nothing else uses the old `c.Writer` pattern.
- **`responseController` fallback is sound.** The type assertion on the stored value guards a missing/other key; the fallback to `http.NewResponseController(c.Writer)` only triggers on a middleware-less test engine, as documented.
- **The `bareEngine()` refactor** preserves the first two tests' original bare-host behavior; the `serverWriteTimeout`/`tick`/`frames` values and the straddling logic are unchanged.
The seed_packet change (log the read-deadline error once, keep the second unchecked) is a behavior tweak with no correctness consequence — best-effort semantics are preserved.
No logic, unit, threshold, or formula errors in my lane.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed this through the code cleanliness & maintainability lens and verified the change against the checked-out code (read `deadlines.go`, `agent.go`, `seed_packet.go`, `api.go`, `auth_test.go`, and grepped all `captureController`/`responseController` usages).
The change is clean and idiomatic for this codebase:
- **Single canonical explanation, with pointers.** The mechanism (logging middleware embeds the `gin.ResponseWriter` *interface*, no `Unwrap`) is documented once in `deadlines.go:13-30`; the four other touch points (`agent.go:168`, `api.go:42`, `seed_packet.go:44`, the test) each carry a short note that defers to it. That's the established verbose-narrative comment style here, not copy-paste rot.
- **Names are clear and consistent.** `captureController`/`responseController`/`responseControllerKey` read plainly and the key is namespaced (`pansy.responseController`).
- **The `c.Writer` fallback is intentional and correctly scoped** to middleware-less test engines (`deadlines.go:38-48`), matching the code comment and the `bareEngine()` helper the tests introduce.
- **The asymmetric error handling in `scanSeedPacket`** (log on `SetReadDeadline`, discard on `SetWriteDeadline`, `seed_packet.go:49-52`) is deliberate and mirrors the same "report once, second call fails identically" pattern already used in `openEventStream`/`write` (`agent.go:181,204`) — consistent, not an oversight.
- **Test refactor is tidy.** Threading `r *gin.Engine` into `streamFrames` and extracting `bareEngine()` avoids duplicating server setup, and the new middleware-stack test reuses the existing `authEngine`/`localCfg` helpers rather than hand-rolling a stack.
No dead code, no leaky abstraction, no duplication that should be shared but isn't. Nothing material in my lane.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've verified the change against the checked-out code. Reviewing strictly through the **Performance** lens.
## VERDICT: No material issues found
The change is deadline-plumbing, not a data path. I checked the three things that could matter for performance:
- **`captureController` runs on every request** (`internal/api/deadlines.go:31-36`, wired at `internal/api/api.go:46`). Each request now does one `http.NewResponseController(c.Writer)` (a single tiny `&ResponseController{rw}` alloc) plus a `c.Set` map insert. This is on *all* routes, including static SPA assets and `healthz`. But `NewResponseController` is trivial, gin's `Keys` map is already allocated on most authenticated paths (`requireAuth`/`requireAdmin` call `c.Set`), and this is a low-traffic self-hosted app. This is a micro-optimization, not a justifiable regression — not flagging it as a finding.
- **The SSE hot path is unchanged in cost.** `openEventStream` fetches the controller once via a single `c.Get` map lookup (`agent.go:176`), and `write()` still does exactly one `SetWriteDeadline` per frame (`agent.go:204`) — same per-frame work as before, just against a controller that now actually reaches the socket. No added per-frame allocation or work in the loop. The `keepAlive` goroutine (`agent.go:213-238`) is untouched.
- **No new N+1, unbounded growth, or blocking calls.** The scan path (`seed_packet.go`) swaps `http.NewResponseController` for `responseController(c)` and adds one error log — no per-request cost change beyond that.
Nothing in this diff introduces an efficiency regression I can justify flagging.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed this change through the **error handling & edge cases** lens, reading `internal/api/deadlines.go`, the `openEventStream`/`write` path in `agent.go`, the full `scanSeedPacket` in `seed_packet.go`, and the test wiring in `auth_test.go`/`sse_deadline_test.go`.
Nothing is materially wrong in my lane. The unhappy paths this diff introduces are handled soundly:
- **`responseController(c)` cannot return nil / cannot panic** (`deadlines.go:41`). Both the stored-value path and the failed-type-assertion/missing-key fallback return a live `*http.ResponseController` (`http.NewResponseController` never returns nil). The double `ok` guard (`c.Get` + type assert) means a wrong-typed context value degrades to the fallback rather than a panic. Verified by reading the function.
- **The scan path now surfaces a previously-swallowed error** (`seed_packet.go:49-51`) — a strict improvement in this lens. The remaining swallowed `SetWriteDeadline` (`seed_packet.go:52`) is a deliberate, documented no-double-report: for an `http.ResponseController` the read/write deadline setters resolve to the same underlying `rwc`, so the second call fails iff the first did. The claim holds; not a bug.
- **The per-frame `_ = s.rc.SetWriteDeadline(...)`** (`agent.go:204`) is likewise intentionally unchecked — `openEventStream` already probes and logs once (`agent.go:181-183`), and the comment correctly notes a write past a dropped deadline returns `nil` anyway, so there is genuinely nothing actionable per-frame.
- **No new cleanup/`defer`/rollback obligation** is introduced. `captureController` only does `c.Set` + `c.Next()`; the stored controller lives on the pooled `gin.Context` and is reset per request, so there's no cross-request leakage or lifetime edge case. Sitting ahead of `gin.Recovery()` is harmless — it does no post-`Next()` work that a downstream panic could strand.
- **Test unhappy-path wiring is real**: `authEngine` (`auth_test.go:20`) routes through `New()`, so the new regression tests exercise the actual middleware order rather than asserting a mental model.
No off-by-one, nil, empty-collection, zero/negative, or overflow surface is touched by this change.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
This PR is a pure deadline-management fix. Through the security lens:
- **`captureController()` added as first engine middleware**: It only calls `c.Set(responseControllerKey, ...)` and `c.Next()`. It captures nothing from the request and grants no access. All authentication — `h.requireAuth()`, `h.requireAdmin()`, CSRF guard — is applied at the route-group level, not the engine level, so this ordering change doesn't touch any auth boundary.
- **Context key `"pansy.responseController"`**: The key is a string constant, properly namespaced. `c.Get`/`c.Set` on a gin context is never visible to the HTTP client — there's no path by which a caller can inject or read context values. The double type-assert in `responseController()` (`v.(*http.ResponseController)`) is safe; a failed assertion triggers the fallback rather than a panic.
- **Deadline extension as a DoS surface**: The two handlers that call `responseController` — `openEventStream` and `scanSeedPacket` — are both behind `h.requireAuth()`. An authenticated user could keep connections alive up to 60s/120s rather than 30s, but that's within the same authorization scope they already have. The server's connection limits aren't changed.
- **Log messages in the new error paths** (`slog.Error` in `scanSeedPacket`): the error value is from `rc.SetReadDeadline`, which is an internal `net` error (`"feature not supported"`). No user-controlled data flows into these log lines.
- **Fallback in `responseController`**: The `http.NewResponseController(c.Writer)` fallback on a test engine without `captureController` reproduces the pre-existing (broken) behavior rather than introducing anything new, and only reachable on `gin.New()` engines — not in production.
Nothing in this diff creates an authn/authz gap, injection vector, or untrusted-data exposure.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I have enough to decide. The draft's finding rests on two specific claims that don't hold up:
1. **"For a 204 No Content response the `net/http` client sets `resp.Body` to `http.NoBody`"** — This is wrong. Go's `net/http` documentation guarantees that `Response.Body` is always non-nil and backed by a real reader (an empty one for bodyless responses, but not `http.NoBody`). `resp.Body.Close()` is not a no-op for 204 responses.
2. **"the race detector's happens-before graph does not include [the network round-trip] directly"** — Also wrong in the relevant sense. Go's `net/http` internally uses `sync.Mutex` throughout the write path (server side) and read path (client side). Those are tracked happens-before edges. The chain is: `readErr = …` → (program-order in handler goroutine) → `sync.Mutex.Unlock()` in net/http response write → (happens-before, tracked by race detector) → `sync.Mutex.Lock()` in net/http response read, which is internal to `Get()` → (program-order in test goroutine) → read of `readErr`. The race detector follows this chain and will not flag it.
This exact pattern — shared variables written in a handler goroutine, read by the test goroutine after `Get()` or `Body.Close()` returns — is ubiquitous in Go's own standard-library HTTP tests, all of which run under `-race`. The pattern is safe in practice, and the finding's confidence rating of "high" with the "candidate race report" framing is not confirmable.
---
No material issues found.
The production change is correct. `captureController` is registered first in `api.New`, builds the `http.ResponseController` from gin's unwrappable raw writer before `sloggin` replaces `c.Writer`, stores it in the context, and `responseController(c)` retrieves it. All three call sites (`openEventStream`, `scanSeedPacket`) now use `responseController(c)`. The new test `TestEventStreamOutlivesWriteTimeoutBehindMiddleware` runs through `authEngine` (which calls `New`), so `captureController` is present and the test actually exercises the production code path. `TestResponseControllerReachesTheSocketBehindMiddleware` correctly asserts that `SetReadDeadline`/`SetWriteDeadline` return nil through the full middleware stack. No issues survived verification.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
The new abstraction is tidy: `deadlines.go` gives the pattern a single home, both call sites (`agent.go:176`, `seed_packet.go:48`) are updated consistently, and the `bareEngine()` extract is a clean de-duplication of a setup idiom that was repeated inline across three tests.
The comment block added to `openEventStream` (lines 168–171) restates roughly what `deadlines.go` already says in full, but it serves as a warning label at the exact call site that was silently regressed before, which the project's documentation conventions explicitly endorse for non-obvious production traps. It's not duplication that will bite anyone.
Nothing else to flag through this lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The two performance-relevant paths are both clean:
- **`captureController()` middleware** — allocates one `*http.ResponseController` per request (a trivially small struct wrap) and stores it with a single `c.Set` call. This runs on every request including routes that never extend a deadline, but the cost is negligible: one allocation and one map write per request regardless of route.
- **`responseController(c)` / per-frame SSE** — the controller is fetched from the context map exactly once, when `openEventStream` constructs the `eventStream` struct (`s := &eventStream{c: c, rc: responseController(c)}`). The per-frame hot loop (`write`) then calls `s.rc.SetWriteDeadline(...)` on the already-retrieved pointer — zero additional context lookups or allocations per frame. The design correctly avoids the obvious trap of re-fetching from the context inside the loop.
No N+1 patterns, no unbounded growth, no blocking on hot paths, no avoidable allocations in the SSE stream.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
The error-handling paths in this PR are clean under my lens:
- **`captureController`**: Stores the controller before `c.Next()`, so it's available regardless of what downstream middleware does or whether they abort. No rollback or cleanup needed — there's nothing to undo when storing a value in the gin context.
- **`responseController` fallback**: The double-`ok` check (one from `c.Get`, one from the type assertion) is robust against a non-`*http.ResponseController` value ever landing under that key. Returns a freshly-built controller in that case rather than panicking.
- **`scanSeedPacket` error handling**: The asymmetry between logged `SetReadDeadline` and silently-discarded `SetWriteDeadline` is explicitly documented with the rationale (both calls fail through the same `ErrNotSupported` path, logging the second one would just repeat the first). For Go's standard HTTP server, a writer that supports read deadlines always supports write deadlines and vice versa — the same `http.rwc` implements both. The assumption holds.
- **`openEventStream` probe + suppress pattern**: Checks deadline support once on open, logs if unsupported, then silently ignores per-frame deadline errors. This is correct — the probe already told the operator everything; repeating it per frame would flood the log for no new information.
- **`write()` mutex**: The `eventStream.write` method holds `s.mu` for the full deadline-set + write-string + flush sequence. The `keepAlive` goroutine also uses `write`, so concurrent access to the `ResponseController` is properly serialized.
- **`TestResponseControllerReachesTheSocketBehindMiddleware`**: Captures `readErr`/`writeErr` in the test goroutine and reads them after `resp.Body.Close()`. In Go's runtime, the `net/http` write path through the netpoller provides sufficient memory ordering between the handler goroutine and the caller of `Get()` — this pattern is used throughout the standard library's own tests and does not trigger the race detector.
Nothing in the unhappy paths or boundary conditions here looks broken or missing.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
The security lens here: the controller is captured from `c.Writer` in the capture middleware and stored in the gin context by key. The controller is then used to set read/write deadlines. No untrusted input flows into this. The key is a constant. The deadline values are constants. No injection, no authz change.
One thing to check: `captureController` stores the controller in the gin context. gin context keys are a shared namespace, but the key is a package-private constant string. No collision risk exposed externally. The controller itself is not user-influenced.
The change doesn't alter authn/authz ordering — `captureController` runs before logging/recovery but after... actually it's the first middleware. It doesn't bypass `requireAuth` or CSRF; those are route-group middleware applied later. No security gap.
No material security issues found.
VERDICT: No material issues found
- Reviewed `internal/api/deadlines.go`, `internal/api/api.go`, `internal/api/seed_packet.go`, `internal/api/agent.go`, and the test helpers. The change captures an `http.ResponseController` from gin's raw writer before logging middleware wraps it, stores it under a package-private constant key (`responseControllerKey`), and hands it to handlers that extend deadlines. No untrusted input taints the controller or the deadline values (all are `time.Now().Add(<const>)`). The key is not externally settable and lives in gin's per-request context, so there's no cross-request leakage or injection surface. `captureController` is added as the first engine middleware but does not bypass or reorder any authn/authz/CSRF middleware — `csrfGuard`, `requireAuth`, etc. are route-group middleware registered after it, so the authorization posture is unchanged. No secret/credential logging is introduced (only `error` values are logged). Nothing in the security lens is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found.**
Reviewed through the Correctness lens. The fix is sound.
Key verifications:
- `captureController()` runs first in `api.New` (api.go:46), capturing a controller from `c.Writer` before `sloggin` wraps it. Confirmed by reading `api.go:38-46`.
- `responseController(c)` (deadlines.go:41-48) returns the stashed controller, falling back to a fresh `http.NewResponseController(c.Writer)` only when the key is absent — the bare-engine test path. Verified.
- `openEventStream` (agent.go:176) and `scanSeedPacket` (seed_packet.go:48-52) both source their controller via `responseController(c)`. Verified.
- The mechanism claim is correct: an `http.ResponseController` unwraps via `Unwrap()`; a wrapper embedding the `gin.ResponseWriter` *interface* (no `Unwrap`) blocks that, yielding `ErrNotSupported`. Building the controller ahead of the wrapper sidesteps it — the controller holds a reference to the underlying gin writer, which does unwrap to the net/http writer. This is the documented `ResponseController` behavior.
- The new test `TestEventStreamOutlivesWriteTimeoutBehindMiddleware` uses `authEngine` → `New` → the real middleware order (auth_test.go:31), so it exercises the actual stack rather than a bare engine. Confirmed.
- Scan-path deadline semantics unchanged: `scanReadTimeout`/`scanWriteTimeout` (60s/120s) applied as before, now through the working controller; the second call's error is deliberately not re-logged, which matches the comment (a writer that can't take deadlines fails identically on both). No behavior regression.
- Constants/thresholds re-derived: server `WriteTimeout`=30s (main.go:65), `ReadTimeout`=15s (main.go:63); the scan extension to 60s/120s and the SSE per-frame `sseWriteTimeout`=30s (agent.go:134) are all relative `time.Now().Add(...)` per-write deadlines, which is exactly the correct shape to override an absolute server timeout. No unit/conversion errors.
No correctness defects introduced.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Verdict: Minor issues
- **Duplicated mechanism explanation across four locations** — The same slog-gin / `Unwrap` / `ErrNotSupported` failure-mode narrative is written out in near-identical prose in `internal/api/deadlines.go:16-25`, `internal/api/agent.go:168-171`, `internal/api/sse_deadline_test.go:117-122`, and `CLAUDE.md:163-169`. `deadlines.go` is where the mechanism actually lives, so it is the natural single home for the full explanation; `agent.go` already appends "(see deadlines.go)" yet still re-narrates the mechanism in lines 168-170, and `sse_deadline_test.go:117-122` re-narrates it in full with no back-reference at all. When the middleware stack changes, four copies must be kept in step, and the three secondary ones are the ones most likely to drift and then mislead. Suggested fix: trim `agent.go:168-171` and `sse_deadline_test.go:117-122` to a one-line "see deadlines.go for why" and keep the full explanation only in `deadlines.go` (and the `CLAUDE.md` convention, which is the appropriate place for the rule itself). Verified by reading all four blocks.
- **Asymmetric error handling in `seed_packet.go:49-52`** — `SetReadDeadline` is checked and logged, while `SetWriteDeadline` is discarded with `_ =`. The inline comment (`seed_packet.go:46-47`) justifies this deliberately ("the second call can only fail the same way as the first"), and the same convention is used in `agent.go:202-204`. The asymmetry reads as a smell on first encounter but is intentional and documented; no change needed beyond keeping the comment.
The structural changes are clean: `responseController`/`captureController` are a small, well-named abstraction with a clear single responsibility; the tests reuse the existing `authEngine`/`localCfg` helpers rather than duplicating engine setup; `bareEngine()` is a tidy extraction of the previous inline `gin.New()`; and `streamFrames` taking the engine as a parameter is a low-churn generalization of the existing helper.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Through the performance lens, this change is neutral-to-slightly-positive:
- `responseController(c)` is called once per request/stream, not per frame. The SSE hot path (`eventStream.write` at `agent.go:204`) reuses the controller stashed in `s.rc` at stream open (`agent.go:176`), so there is no per-frame context-map lookup or controller construction. The scan path (`seed_packet.go:48`) calls it once per upload.
- `captureController` builds one `http.ResponseController` per request and stores it in the gin context (`deadlines.go:33`). This replaces the previous per-call `http.NewResponseController(c.Writer)` in `openEventStream`/`scanSeedPacket`, so allocation behavior is at worst unchanged and is now shared/cached rather than rebuilt at each deadline-extending call site.
- The `c.Get` + double type-assertion in `responseController` (`deadlines.go:42-46`) is O(1) map lookup under the gin context mutex, once per request — not a hot-loop concern.
- No new unbounded growth, no N+1, no blocking calls introduced on the SSE write path.
I verified by reading `deadlines.go`, `api.go`, `agent.go:160-207`, and grepping all `responseController` call sites — none sit inside a per-frame/per-write loop.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
Through the error-handling / edge-case lens, the change is carefully constructed. I verified each unhappy path:
- `deadlines.go:41-47` `responseController` handles both a missing key (`c.Get` → `ok==false`) and a wrong-typed value (inner type assertion fails) by falling back to `http.NewResponseController(c.Writer)`. No nil deref — `c.Get` returns `(nil, false)` when absent, and both branches are guarded.
- `agent.go:181-183` the SSE probe logs a deadline failure once but deliberately continues (best-effort, documented), so a controller that can't reach the socket degrades to the server default rather than panicking or aborting.
- `seed_packet.go:49-52` the read-deadline error is now logged once; the write-deadline error is discarded with an explicit justification (same underlying `ErrNotSupported` cause). I checked: on a real `http` connection both setters are supported or neither is, so the "can only fail the same way" claim holds — no silent distinct write-deadline failure is introduced.
- `captureController` builds the controller per-request before any wrapper, so no stale/nil controller is stashed; `c.Next()` is called so the chain always proceeds.
No swallowed errors that matter, no missing cleanup, no nil/empty/boundary hazards in the new code. The new tests' `resp.Body.Close()` error is ignored, which is standard test hygiene and not material.
Verdict: No material issues found.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
- The change correctly fixes a production bug where `http.ResponseController` couldn't unwrap through `slog-gin`'s writer wrapper to reach the socket's deadline setters.
- The new `captureController()` middleware captures a controller from the raw gin writer before any wrapping occurs, and stashes it under a package-private context key. No user-controlled input reaches the key, the controller, or the deadline calls.
- `responseController()` safely falls back to `http.NewResponseController(c.Writer)` for bare test engines.
- No new injection surfaces, SSRF vectors, secret leakage, authn/authz gaps, or unsafe deserialization introduced. The error logging in `seed_packet.go` is safe and does not expose sensitive data.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I've read the changed files (`deadlines.go`, `api.go`, `agent.go`, `seed_packet.go`, `sse_deadline_test.go`) and verified cross-file dependencies (`auth_test.go`, grep for remaining `NewResponseController` calls). The logic is sound:
- `captureController()` runs first in the middleware chain and stores an `http.ResponseController` built from gin's native `c.Writer`, before `sloggin` wraps it in an interface-embedding type that blocks unwrapping.
- `responseController(c)` retrieves that stored controller, falling back to `http.NewResponseController(c.Writer)` on bare engines (test-only path).
- Both `openEventStream` and `scanSeedPacket` now use `responseController(c)`, so their `SetWriteDeadline`/`SetReadDeadline` calls reach the underlying `net.Conn` in production.
- The regression tests (`TestEventStreamOutlivesWriteTimeoutBehindMiddleware`, `TestResponseControllerReachesTheSocketBehindMiddleware`) exercise the real middleware stack via `authEngine` → `New()`, not the bare `gin.New()` that masked the bug.
- Error handling in `scanSeedPacket` logs the first deadline error once rather than discarding both silently; the second call is intentionally ignored because it can only fail the same way (`ErrNotSupported`).
Constants and behavior re-derived from first principles:
- `http.ResponseController` unwraps the writer it was built from layer-by-layer looking for `SetWriteDeadline`/`SetReadDeadline`.
- gin's native writer implements `Unwrap()`; slog-gin's wrapper embeds the `gin.ResponseWriter` interface, which has no `Unwrap()`.
- Capturing the controller before the wrapper is therefore the correct fix, and the fallback for bare engines is safe because gin's native writer unwraps cleanly.
No logic bugs, incorrect constants, or semantic errors introduced.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
The fix correctly moves `ResponseController` creation from per-handler (where it fails behind middleware wrappers) to a single per-request capture in `captureController()`. From a performance perspective:
- **No extra work on hot paths**: `responseController(c)` is called once per SSE stream and once per scan upload; the controller is then reused. Per-frame `SetWriteDeadline` calls and their `time.Now()` overhead are unchanged from the pre-broken state.
- **No new allocations in loops**: The middleware does a single `http.NewResponseController` and `c.Set` per HTTP request — negligible and necessary.
- **No N+1 or quadratic behavior introduced**.
- **No unbounded growth or missing limits**: The SSE stream already refreshes deadlines per frame, preventing unbounded blocking.
The architectural change (capture early, reuse) is strictly better than the prior fallback of building a broken controller on every call path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
- **Error handling:** The diff correctly moves deadline-extension errors from silently swallowed (`_ = ...`) to logged once (`slog.Error`), both for SSE (`agent.go:181`) and scan (`seed_packet.go:49`). The per-frame SSE write (`agent.go:204`) and the second scan write (`seed_packet.go:52`) remain deliberately unchecked to avoid spam; the comments explain why (same controller, same failure mode), which is reasonable.
- **Edge cases:** `responseController` (deadlines.go:41) safely falls back to `http.NewResponseController(c.Writer)` if the stored key is missing or the type assertion fails. No nil-dereference risk: `captureController` stores a non-nil controller, and the fallback path also receives gin's always-non-nil `c.Writer`. No integer overflow or boundary issues in the constants used.
- **Cleanup:** `captureController` allocates no resources needing `defer` or rollback; it merely stores a pointer in the gin context.
The unhappy paths (missing middleware, unsupported writer) are handled with graceful fallbacks and explicit logging.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
The why-a-controller-can't-reach-the-socket story was told in full in
deadlines.go, agent.go, the test, and CLAUDE.md. It lives in deadlines.go
now; the others say what they need to and point there.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit f0aefb5378 into main2026-08-23 03:10:37 +00:00
steve
deleted branch fix/sse-deadlines-behind-middleware2026-08-23 03:10:37 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The bug
Long agent turns are cut at exactly 30s on the live instance with "The connection dropped partway through." That is the #78 failure — the server's absolute
WriteTimeout— which the #78 fix and its tests say is handled.It never was, in production.
slog-ginunconditionally replacesc.Writerwith a wrapper that embeds thegin.ResponseWriterinterface, which has noUnwrap(). Anhttp.ResponseControllerbuilt from the handler's writer unwraps layer by layer looking forSetWriteDeadline, can't see through that wrapper, and returnsErrNotSupported— so the stream falls back to the server's 30sWriteTimeout. The first write past it fails, which cancels the request context and closes the socket under the client mid-frame; the browser'sreader.read()throws, and the UI reports the drop. The tests passed because they hostopenEventStreamon a baregin.New()with no middleware.The scan upload's read/write deadline extensions (
seed_packet.go) fail the same way, with the errors discarded.Visible in the container log as
ERROR api: SSE write deadlines unavailable … error="feature not supported"at the start of every chat, and aPOST /api/v1/agent/chatrequest line withresponse.latencyof 30–50s.The fix
internal/api/deadlines.go:captureController()runs first on the engine and stashes a controller built from gin's raw writer, before anything wraps it;responseController(c)hands it to handlers (falling back toc.Writeron a middleware-less test engine).openEventStreamandscanSeedPackettake their controller fromresponseController(c); the scan path now logs once instead of swallowing the error.api.New()— the real stack, in the real order — and assert from the client side.TestEventStreamOutlivesWriteTimeoutBehindMiddlewarefails onmainwithclient read error after 0/3 frames: unexpected EOF, the exact live failure.gin.New()" trap.GOWORK=off go test ./...green, gofmt clean.🤖 Generated with Claude Code
🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 03:08:04Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -165,2 +165,4 @@// dropped connection. Hence a deadline set up front and refreshed per frame,// rather than anything checked after the fact.//// The controller comes from responseController, NOT from c.Writer: by the time🟡 Same middleware/Unwrap rationale re-narrated in deadlines.go, agent.go, sse_deadline_test.go, and CLAUDE.md — secondary copies will drift; keep the full explanation only in deadlines.go and point back from the others
maintainability · flagged by 1 model
Unwrap/ErrNotSupportedfailure-mode narrative is written out in near-identical prose ininternal/api/deadlines.go:16-25,internal/api/agent.go:168-171,internal/api/sse_deadline_test.go:117-122, andCLAUDE.md:163-169.deadlines.gois where the mechanism actually lives, so it is the natural single home for the full explanation;agent.goalready appends "(see deadlines.go)" yet still re-narrates th…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 1 finding (0 with multi-model agreement)
1 single-model finding (lower confidence)
internal/api/agent.go:168Per-model detail
claude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I reviewed this change through the security lens and verified the relevant code by reading
internal/api/deadlines.go,api.go, andseed_packet.goat the checked-out state.The change is a plumbing fix: a first-in-chain middleware (
captureController) builds anhttp.ResponseControllerfrom gin's raw writer beforeslog-ginwraps it, and handlers pull it viaresponseController(c)to set read/write deadlines. Nothing here touches authn/authz, parsing of untrusted data, injection sinks, SSRF, deserialization, or secrets.Things I checked specifically for security impact and found clean:
c.Writer, not from any request-controlled value.c.Set/c.Getare request-scoped, and a fresh controller is created per request — no cross-request state leakage.captureControlleronly doesc.Set+c.Next(); placing it ahead ofgin.Recovery()doesn't defeat panic recovery (Recovery's deferred recover still wraps all downstream handlers) and it doesn't short-circuit the auth/CSRF/admin guards that remain in place on their groups (seed-lots,/agentetc. are still behindrequireAuth).seed_packet.go:50logs only the deadline error ("feature not supported"), no credentials or request content.scanSeedPacketstill wraps the body inhttp.MaxBytesReader(…, scanUploadLimit)(seed_packet.go:54) and is behindrequireAuth(api.go:186,194).One thing I considered and concluded is not a material finding: the fix makes the scan read-deadline extension (15s → 60s) actually take effect, which genuinely lengthens how long a client can hold that connection. But the endpoint is authenticated, the deadlines are bounded (60s read / 120s write), and the body is byte-capped, so this is intended availability tuning, not an anonymous slow-loris vector. Verified via
requireAuthon theseed-lotsgroup and theMaxBytesReadercap.Nothing in the security lens warrants a change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed this change through the Correctness lens and verified the mechanism against the checked-out code and dependencies rather than the diff alone.
What I checked and confirmed:
captureController()runs as the first middleware (api.go:46) and buildshttp.NewResponseController(c.Writer)whilec.Writeris still gin's own*responseWriter. gin v1.10.1'sresponseWriterimplementsUnwrap()(present since gin 1.9), so the controller unwraps gin's writer → the underlying*http.response, which supportsSetWriteDeadline/SetReadDeadline. Storing it beforesloggin(v1.15.0) wrapsc.Writeris exactly what dodges the no-Unwrapwrapper. The deadline is set on the same connection the later writes flush through, so there's no target mismatch.authEnginebuilds the engine viaNew()(auth_test.go:31), andstreamFrames/the deadline test register/streamand/deadlinesafterNew()returns. gin applies engine-levelUse()middleware (registered insideNew) to routes added later, socaptureControlleris in the chain for both. The routes are top-level, not under therequireAuthgroup, so the unauthenticated testGETreaches the handler — the assertions (got == 3,readErr/writeErr == nil) are meaningful, not silently skipped by a 401.main. WithoutcaptureController,openEventStreambuilt the controller from the handler'sc.Writer(the sloggin wrapper) →ErrNotSupported→ stream falls back to the 300ms serverWriteTimeout→ cut after ~1 frame. SoTestEventStreamOutlivesWriteTimeoutBehindMiddlewareis a true regression guard, not a fixture that asserts its own assumptions.grepforNewResponseController/Set*Deadlineshows the only two production handlers extending deadlines (agent.go,seed_packet.go) both now route throughresponseController(c); nothing else uses the oldc.Writerpattern.responseControllerfallback is sound. The type assertion on the stored value guards a missing/other key; the fallback tohttp.NewResponseController(c.Writer)only triggers on a middleware-less test engine, as documented.bareEngine()refactor preserves the first two tests' original bare-host behavior; theserverWriteTimeout/tick/framesvalues and the straddling logic are unchanged.The seed_packet change (log the read-deadline error once, keep the second unchecked) is a behavior tweak with no correctness consequence — best-effort semantics are preserved.
No logic, unit, threshold, or formula errors in my lane.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
I reviewed this through the code cleanliness & maintainability lens and verified the change against the checked-out code (read
deadlines.go,agent.go,seed_packet.go,api.go,auth_test.go, and grepped allcaptureController/responseControllerusages).The change is clean and idiomatic for this codebase:
gin.ResponseWriterinterface, noUnwrap) is documented once indeadlines.go:13-30; the four other touch points (agent.go:168,api.go:42,seed_packet.go:44, the test) each carry a short note that defers to it. That's the established verbose-narrative comment style here, not copy-paste rot.captureController/responseController/responseControllerKeyread plainly and the key is namespaced (pansy.responseController).c.Writerfallback is intentional and correctly scoped to middleware-less test engines (deadlines.go:38-48), matching the code comment and thebareEngine()helper the tests introduce.scanSeedPacket(log onSetReadDeadline, discard onSetWriteDeadline,seed_packet.go:49-52) is deliberate and mirrors the same "report once, second call fails identically" pattern already used inopenEventStream/write(agent.go:181,204) — consistent, not an oversight.r *gin.EngineintostreamFramesand extractingbareEngine()avoids duplicating server setup, and the new middleware-stack test reuses the existingauthEngine/localCfghelpers rather than hand-rolling a stack.No dead code, no leaky abstraction, no duplication that should be shared but isn't. Nothing material in my lane.
⚡ Performance — No material issues found
I've verified the change against the checked-out code. Reviewing strictly through the Performance lens.
VERDICT: No material issues found
The change is deadline-plumbing, not a data path. I checked the three things that could matter for performance:
captureControllerruns on every request (internal/api/deadlines.go:31-36, wired atinternal/api/api.go:46). Each request now does onehttp.NewResponseController(c.Writer)(a single tiny&ResponseController{rw}alloc) plus ac.Setmap insert. This is on all routes, including static SPA assets andhealthz. ButNewResponseControlleris trivial, gin'sKeysmap is already allocated on most authenticated paths (requireAuth/requireAdmincallc.Set), and this is a low-traffic self-hosted app. This is a micro-optimization, not a justifiable regression — not flagging it as a finding.The SSE hot path is unchanged in cost.
openEventStreamfetches the controller once via a singlec.Getmap lookup (agent.go:176), andwrite()still does exactly oneSetWriteDeadlineper frame (agent.go:204) — same per-frame work as before, just against a controller that now actually reaches the socket. No added per-frame allocation or work in the loop. ThekeepAlivegoroutine (agent.go:213-238) is untouched.No new N+1, unbounded growth, or blocking calls. The scan path (
seed_packet.go) swapshttp.NewResponseControllerforresponseController(c)and adds one error log — no per-request cost change beyond that.Nothing in this diff introduces an efficiency regression I can justify flagging.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this change through the error handling & edge cases lens, reading
internal/api/deadlines.go, theopenEventStream/writepath inagent.go, the fullscanSeedPacketinseed_packet.go, and the test wiring inauth_test.go/sse_deadline_test.go.Nothing is materially wrong in my lane. The unhappy paths this diff introduces are handled soundly:
responseController(c)cannot return nil / cannot panic (deadlines.go:41). Both the stored-value path and the failed-type-assertion/missing-key fallback return a live*http.ResponseController(http.NewResponseControllernever returns nil). The doubleokguard (c.Get+ type assert) means a wrong-typed context value degrades to the fallback rather than a panic. Verified by reading the function.seed_packet.go:49-51) — a strict improvement in this lens. The remaining swallowedSetWriteDeadline(seed_packet.go:52) is a deliberate, documented no-double-report: for anhttp.ResponseControllerthe read/write deadline setters resolve to the same underlyingrwc, so the second call fails iff the first did. The claim holds; not a bug._ = s.rc.SetWriteDeadline(...)(agent.go:204) is likewise intentionally unchecked —openEventStreamalready probes and logs once (agent.go:181-183), and the comment correctly notes a write past a dropped deadline returnsnilanyway, so there is genuinely nothing actionable per-frame.defer/rollback obligation is introduced.captureControlleronly doesc.Set+c.Next(); the stored controller lives on the pooledgin.Contextand is reset per request, so there's no cross-request leakage or lifetime edge case. Sitting ahead ofgin.Recovery()is harmless — it does no post-Next()work that a downstream panic could strand.authEngine(auth_test.go:20) routes throughNew(), so the new regression tests exercise the actual middleware order rather than asserting a mental model.No off-by-one, nil, empty-collection, zero/negative, or overflow surface is touched by this change.
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
This PR is a pure deadline-management fix. Through the security lens:
captureController()added as first engine middleware: It only callsc.Set(responseControllerKey, ...)andc.Next(). It captures nothing from the request and grants no access. All authentication —h.requireAuth(),h.requireAdmin(), CSRF guard — is applied at the route-group level, not the engine level, so this ordering change doesn't touch any auth boundary.Context key
"pansy.responseController": The key is a string constant, properly namespaced.c.Get/c.Seton a gin context is never visible to the HTTP client — there's no path by which a caller can inject or read context values. The double type-assert inresponseController()(v.(*http.ResponseController)) is safe; a failed assertion triggers the fallback rather than a panic.Deadline extension as a DoS surface: The two handlers that call
responseController—openEventStreamandscanSeedPacket— are both behindh.requireAuth(). An authenticated user could keep connections alive up to 60s/120s rather than 30s, but that's within the same authorization scope they already have. The server's connection limits aren't changed.Log messages in the new error paths (
slog.ErrorinscanSeedPacket): the error value is fromrc.SetReadDeadline, which is an internalneterror ("feature not supported"). No user-controlled data flows into these log lines.Fallback in
responseController: Thehttp.NewResponseController(c.Writer)fallback on a test engine withoutcaptureControllerreproduces the pre-existing (broken) behavior rather than introducing anything new, and only reachable ongin.New()engines — not in production.Nothing in this diff creates an authn/authz gap, injection vector, or untrusted-data exposure.
🎯 Correctness — No material issues found
I have enough to decide. The draft's finding rests on two specific claims that don't hold up:
"For a 204 No Content response the
net/httpclient setsresp.Bodytohttp.NoBody" — This is wrong. Go'snet/httpdocumentation guarantees thatResponse.Bodyis always non-nil and backed by a real reader (an empty one for bodyless responses, but nothttp.NoBody).resp.Body.Close()is not a no-op for 204 responses."the race detector's happens-before graph does not include [the network round-trip] directly" — Also wrong in the relevant sense. Go's
net/httpinternally usessync.Mutexthroughout the write path (server side) and read path (client side). Those are tracked happens-before edges. The chain is:readErr = …→ (program-order in handler goroutine) →sync.Mutex.Unlock()in net/http response write → (happens-before, tracked by race detector) →sync.Mutex.Lock()in net/http response read, which is internal toGet()→ (program-order in test goroutine) → read ofreadErr. The race detector follows this chain and will not flag it.This exact pattern — shared variables written in a handler goroutine, read by the test goroutine after
Get()orBody.Close()returns — is ubiquitous in Go's own standard-library HTTP tests, all of which run under-race. The pattern is safe in practice, and the finding's confidence rating of "high" with the "candidate race report" framing is not confirmable.No material issues found.
The production change is correct.
captureControlleris registered first inapi.New, builds thehttp.ResponseControllerfrom gin's unwrappable raw writer beforeslogginreplacesc.Writer, stores it in the context, andresponseController(c)retrieves it. All three call sites (openEventStream,scanSeedPacket) now useresponseController(c). The new testTestEventStreamOutlivesWriteTimeoutBehindMiddlewareruns throughauthEngine(which callsNew), socaptureControlleris present and the test actually exercises the production code path.TestResponseControllerReachesTheSocketBehindMiddlewarecorrectly asserts thatSetReadDeadline/SetWriteDeadlinereturn nil through the full middleware stack. No issues survived verification.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The new abstraction is tidy:
deadlines.gogives the pattern a single home, both call sites (agent.go:176,seed_packet.go:48) are updated consistently, and thebareEngine()extract is a clean de-duplication of a setup idiom that was repeated inline across three tests.The comment block added to
openEventStream(lines 168–171) restates roughly whatdeadlines.goalready says in full, but it serves as a warning label at the exact call site that was silently regressed before, which the project's documentation conventions explicitly endorse for non-obvious production traps. It's not duplication that will bite anyone.Nothing else to flag through this lens.
⚡ Performance — No material issues found
No material issues found
The two performance-relevant paths are both clean:
captureController()middleware — allocates one*http.ResponseControllerper request (a trivially small struct wrap) and stores it with a singlec.Setcall. This runs on every request including routes that never extend a deadline, but the cost is negligible: one allocation and one map write per request regardless of route.responseController(c)/ per-frame SSE — the controller is fetched from the context map exactly once, whenopenEventStreamconstructs theeventStreamstruct (s := &eventStream{c: c, rc: responseController(c)}). The per-frame hot loop (write) then callss.rc.SetWriteDeadline(...)on the already-retrieved pointer — zero additional context lookups or allocations per frame. The design correctly avoids the obvious trap of re-fetching from the context inside the loop.No N+1 patterns, no unbounded growth, no blocking on hot paths, no avoidable allocations in the SSE stream.
🧯 Error handling & edge cases — No material issues found
No material issues found.
The error-handling paths in this PR are clean under my lens:
captureController: Stores the controller beforec.Next(), so it's available regardless of what downstream middleware does or whether they abort. No rollback or cleanup needed — there's nothing to undo when storing a value in the gin context.responseControllerfallback: The double-okcheck (one fromc.Get, one from the type assertion) is robust against a non-*http.ResponseControllervalue ever landing under that key. Returns a freshly-built controller in that case rather than panicking.scanSeedPacketerror handling: The asymmetry between loggedSetReadDeadlineand silently-discardedSetWriteDeadlineis explicitly documented with the rationale (both calls fail through the sameErrNotSupportedpath, logging the second one would just repeat the first). For Go's standard HTTP server, a writer that supports read deadlines always supports write deadlines and vice versa — the samehttp.rwcimplements both. The assumption holds.openEventStreamprobe + suppress pattern: Checks deadline support once on open, logs if unsupported, then silently ignores per-frame deadline errors. This is correct — the probe already told the operator everything; repeating it per frame would flood the log for no new information.write()mutex: TheeventStream.writemethod holdss.mufor the full deadline-set + write-string + flush sequence. ThekeepAlivegoroutine also useswrite, so concurrent access to theResponseControlleris properly serialized.TestResponseControllerReachesTheSocketBehindMiddleware: CapturesreadErr/writeErrin the test goroutine and reads them afterresp.Body.Close(). In Go's runtime, thenet/httpwrite path through the netpoller provides sufficient memory ordering between the handler goroutine and the caller ofGet()— this pattern is used throughout the standard library's own tests and does not trigger the race detector.Nothing in the unhappy paths or boundary conditions here looks broken or missing.
glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
The security lens here: the controller is captured from
c.Writerin the capture middleware and stored in the gin context by key. The controller is then used to set read/write deadlines. No untrusted input flows into this. The key is a constant. The deadline values are constants. No injection, no authz change.One thing to check:
captureControllerstores the controller in the gin context. gin context keys are a shared namespace, but the key is a package-private constant string. No collision risk exposed externally. The controller itself is not user-influenced.The change doesn't alter authn/authz ordering —
captureControllerruns before logging/recovery but after... actually it's the first middleware. It doesn't bypassrequireAuthor CSRF; those are route-group middleware applied later. No security gap.No material security issues found.
VERDICT: No material issues found
internal/api/deadlines.go,internal/api/api.go,internal/api/seed_packet.go,internal/api/agent.go, and the test helpers. The change captures anhttp.ResponseControllerfrom gin's raw writer before logging middleware wraps it, stores it under a package-private constant key (responseControllerKey), and hands it to handlers that extend deadlines. No untrusted input taints the controller or the deadline values (all aretime.Now().Add(<const>)). The key is not externally settable and lives in gin's per-request context, so there's no cross-request leakage or injection surface.captureControlleris added as the first engine middleware but does not bypass or reorder any authn/authz/CSRF middleware —csrfGuard,requireAuth, etc. are route-group middleware registered after it, so the authorization posture is unchanged. No secret/credential logging is introduced (onlyerrorvalues are logged). Nothing in the security lens is materially wrong.🎯 Correctness — No material issues found
Verdict: No material issues found.
Reviewed through the Correctness lens. The fix is sound.
Key verifications:
captureController()runs first inapi.New(api.go:46), capturing a controller fromc.Writerbeforeslogginwraps it. Confirmed by readingapi.go:38-46.responseController(c)(deadlines.go:41-48) returns the stashed controller, falling back to a freshhttp.NewResponseController(c.Writer)only when the key is absent — the bare-engine test path. Verified.openEventStream(agent.go:176) andscanSeedPacket(seed_packet.go:48-52) both source their controller viaresponseController(c). Verified.http.ResponseControllerunwraps viaUnwrap(); a wrapper embedding thegin.ResponseWriterinterface (noUnwrap) blocks that, yieldingErrNotSupported. Building the controller ahead of the wrapper sidesteps it — the controller holds a reference to the underlying gin writer, which does unwrap to the net/http writer. This is the documentedResponseControllerbehavior.TestEventStreamOutlivesWriteTimeoutBehindMiddlewareusesauthEngine→New→ the real middleware order (auth_test.go:31), so it exercises the actual stack rather than a bare engine. Confirmed.scanReadTimeout/scanWriteTimeout(60s/120s) applied as before, now through the working controller; the second call's error is deliberately not re-logged, which matches the comment (a writer that can't take deadlines fails identically on both). No behavior regression.WriteTimeout=30s (main.go:65),ReadTimeout=15s (main.go:63); the scan extension to 60s/120s and the SSE per-framesseWriteTimeout=30s (agent.go:134) are all relativetime.Now().Add(...)per-write deadlines, which is exactly the correct shape to override an absolute server timeout. No unit/conversion errors.No correctness defects introduced.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Duplicated mechanism explanation across four locations — The same slog-gin /
Unwrap/ErrNotSupportedfailure-mode narrative is written out in near-identical prose ininternal/api/deadlines.go:16-25,internal/api/agent.go:168-171,internal/api/sse_deadline_test.go:117-122, andCLAUDE.md:163-169.deadlines.gois where the mechanism actually lives, so it is the natural single home for the full explanation;agent.goalready appends "(see deadlines.go)" yet still re-narrates the mechanism in lines 168-170, andsse_deadline_test.go:117-122re-narrates it in full with no back-reference at all. When the middleware stack changes, four copies must be kept in step, and the three secondary ones are the ones most likely to drift and then mislead. Suggested fix: trimagent.go:168-171andsse_deadline_test.go:117-122to a one-line "see deadlines.go for why" and keep the full explanation only indeadlines.go(and theCLAUDE.mdconvention, which is the appropriate place for the rule itself). Verified by reading all four blocks.Asymmetric error handling in
seed_packet.go:49-52—SetReadDeadlineis checked and logged, whileSetWriteDeadlineis discarded with_ =. The inline comment (seed_packet.go:46-47) justifies this deliberately ("the second call can only fail the same way as the first"), and the same convention is used inagent.go:202-204. The asymmetry reads as a smell on first encounter but is intentional and documented; no change needed beyond keeping the comment.The structural changes are clean:
responseController/captureControllerare a small, well-named abstraction with a clear single responsibility; the tests reuse the existingauthEngine/localCfghelpers rather than duplicating engine setup;bareEngine()is a tidy extraction of the previous inlinegin.New(); andstreamFramestaking the engine as a parameter is a low-churn generalization of the existing helper.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this change is neutral-to-slightly-positive:
responseController(c)is called once per request/stream, not per frame. The SSE hot path (eventStream.writeatagent.go:204) reuses the controller stashed ins.rcat stream open (agent.go:176), so there is no per-frame context-map lookup or controller construction. The scan path (seed_packet.go:48) calls it once per upload.captureControllerbuilds onehttp.ResponseControllerper request and stores it in the gin context (deadlines.go:33). This replaces the previous per-callhttp.NewResponseController(c.Writer)inopenEventStream/scanSeedPacket, so allocation behavior is at worst unchanged and is now shared/cached rather than rebuilt at each deadline-extending call site.c.Get+ double type-assertion inresponseController(deadlines.go:42-46) is O(1) map lookup under the gin context mutex, once per request — not a hot-loop concern.I verified by reading
deadlines.go,api.go,agent.go:160-207, and grepping allresponseControllercall sites — none sit inside a per-frame/per-write loop.🧯 Error handling & edge cases — No material issues found
Through the error-handling / edge-case lens, the change is carefully constructed. I verified each unhappy path:
deadlines.go:41-47responseControllerhandles both a missing key (c.Get→ok==false) and a wrong-typed value (inner type assertion fails) by falling back tohttp.NewResponseController(c.Writer). No nil deref —c.Getreturns(nil, false)when absent, and both branches are guarded.agent.go:181-183the SSE probe logs a deadline failure once but deliberately continues (best-effort, documented), so a controller that can't reach the socket degrades to the server default rather than panicking or aborting.seed_packet.go:49-52the read-deadline error is now logged once; the write-deadline error is discarded with an explicit justification (same underlyingErrNotSupportedcause). I checked: on a realhttpconnection both setters are supported or neither is, so the "can only fail the same way" claim holds — no silent distinct write-deadline failure is introduced.captureControllerbuilds the controller per-request before any wrapper, so no stale/nil controller is stashed;c.Next()is called so the chain always proceeds.No swallowed errors that matter, no missing cleanup, no nil/empty/boundary hazards in the new code. The new tests'
resp.Body.Close()error is ignored, which is standard test hygiene and not material.Verdict: No material issues found.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
http.ResponseControllercouldn't unwrap throughslog-gin's writer wrapper to reach the socket's deadline setters.captureController()middleware captures a controller from the raw gin writer before any wrapping occurs, and stashes it under a package-private context key. No user-controlled input reaches the key, the controller, or the deadline calls.responseController()safely falls back tohttp.NewResponseController(c.Writer)for bare test engines.seed_packet.gois safe and does not expose sensitive data.🎯 Correctness — No material issues found
Verdict: No material issues found
I've read the changed files (
deadlines.go,api.go,agent.go,seed_packet.go,sse_deadline_test.go) and verified cross-file dependencies (auth_test.go, grep for remainingNewResponseControllercalls). The logic is sound:captureController()runs first in the middleware chain and stores anhttp.ResponseControllerbuilt from gin's nativec.Writer, beforeslogginwraps it in an interface-embedding type that blocks unwrapping.responseController(c)retrieves that stored controller, falling back tohttp.NewResponseController(c.Writer)on bare engines (test-only path).openEventStreamandscanSeedPacketnow useresponseController(c), so theirSetWriteDeadline/SetReadDeadlinecalls reach the underlyingnet.Connin production.TestEventStreamOutlivesWriteTimeoutBehindMiddleware,TestResponseControllerReachesTheSocketBehindMiddleware) exercise the real middleware stack viaauthEngine→New(), not the baregin.New()that masked the bug.scanSeedPacketlogs the first deadline error once rather than discarding both silently; the second call is intentionally ignored because it can only fail the same way (ErrNotSupported).Constants and behavior re-derived from first principles:
http.ResponseControllerunwraps the writer it was built from layer-by-layer looking forSetWriteDeadline/SetReadDeadline.Unwrap(); slog-gin's wrapper embeds thegin.ResponseWriterinterface, which has noUnwrap().No logic bugs, incorrect constants, or semantic errors introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
The fix correctly moves
ResponseControllercreation from per-handler (where it fails behind middleware wrappers) to a single per-request capture incaptureController(). From a performance perspective:responseController(c)is called once per SSE stream and once per scan upload; the controller is then reused. Per-frameSetWriteDeadlinecalls and theirtime.Now()overhead are unchanged from the pre-broken state.http.NewResponseControllerandc.Setper HTTP request — negligible and necessary.The architectural change (capture early, reuse) is strictly better than the prior fallback of building a broken controller on every call path.
🧯 Error handling & edge cases — No material issues found
No material issues found
_ = ...) to logged once (slog.Error), both for SSE (agent.go:181) and scan (seed_packet.go:49). The per-frame SSE write (agent.go:204) and the second scan write (seed_packet.go:52) remain deliberately unchecked to avoid spam; the comments explain why (same controller, same failure mode), which is reasonable.responseController(deadlines.go:41) safely falls back tohttp.NewResponseController(c.Writer)if the stored key is missing or the type assertion fails. No nil-dereference risk:captureControllerstores a non-nil controller, and the fallback path also receives gin's always-non-nilc.Writer. No integer overflow or boundary issues in the constants used.captureControllerallocates no resources needingdeferor rollback; it merely stores a pointer in the gin context.The unhappy paths (missing middleware, unsupported writer) are handled with graceful fallbacks and explicit logging.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.