Make request deadline extensions reach the socket behind the logging middleware #126
@@ -161,12 +161,11 @@ Conventions that follow from it:
|
|||||||
lives in `commitScope` where no caller can forget it.
|
lives in `commitScope` where no caller can forget it.
|
||||||
|
|
||||||
- **Request deadlines are extended through `responseController(c)`, never
|
- **Request deadlines are extended through `responseController(c)`, never
|
||||||
`http.NewResponseController(c.Writer)`.** The logging middleware wraps the
|
`http.NewResponseController(c.Writer)`.** A controller built in a handler
|
||||||
writer in a type with no `Unwrap`, so a controller built from a handler's
|
can't reach the socket — the logging middleware wraps the writer — so every
|
||||||
writer can't reach the socket: every `SetWriteDeadline`/`SetReadDeadline`
|
deadline call silently returns `ErrNotSupported`, in production only;
|
||||||
returns `ErrNotSupported` — in production only. `captureController` is the
|
`internal/api/deadlines.go` has the mechanism and why `captureController`
|
||||||
first middleware in `api.New` precisely so the controller exists before
|
must stay the first middleware. Corollary for tests: a deadline test must run
|
||||||
anything wraps the writer. Corollary for tests: a deadline test must run
|
|
||||||
through `New()`, not `gin.New()` — the #78 fix shipped fully tested on a bare
|
through `New()`, not `gin.New()` — the #78 fix shipped fully tested on a bare
|
||||||
engine and never worked on the live instance.
|
engine and never worked on the live instance.
|
||||||
|
|
||||||
|
|||||||
@@ -165,10 +165,8 @@ type eventStream struct {
|
|||||||
// dropped connection. Hence a deadline set up front and refreshed per frame,
|
// dropped connection. Hence a deadline set up front and refreshed per frame,
|
||||||
// rather than anything checked after the fact.
|
// rather than anything checked after the fact.
|
||||||
//
|
//
|
||||||
// The controller comes from responseController, NOT from c.Writer: by the time
|
// The controller comes from responseController, not from c.Writer — a
|
||||||
|
|
|||||||
// a handler runs, the logging middleware has wrapped the writer in something a
|
// controller built here can't reach the socket; deadlines.go says why.
|
||||||
// controller can't unwrap, and every deadline call fails — which is how this
|
|
||||||
// fix shipped, tested, and stayed broken live (see deadlines.go).
|
|
||||||
func openEventStream(c *gin.Context) *eventStream {
|
func openEventStream(c *gin.Context) *eventStream {
|
||||||
c.Header("Content-Type", "text/event-stream")
|
c.Header("Content-Type", "text/event-stream")
|
||||||
c.Header("Cache-Control", "no-cache")
|
c.Header("Cache-Control", "no-cache")
|
||||||
|
|||||||
@@ -112,18 +112,12 @@ func TestEventStreamRefreshesDeadlinePerFrame(t *testing.T) {
|
|||||||
// production middleware stack — which is where it was still broken.
|
// production middleware stack — which is where it was still broken.
|
||||||
//
|
//
|
||||||
// The two tests above passed while the deployed instance cut every agent turn
|
// The two tests above passed while the deployed instance cut every agent turn
|
||||||
// at exactly 30s with "The connection dropped partway through." The difference
|
// at exactly 30s: they host openEventStream on a bare engine, and it is the
|
||||||
// is the middleware: the logging middleware in New replaces c.Writer with a
|
// logging middleware in New that hides the socket from a ResponseController
|
||||||
// wrapper that embeds the gin.ResponseWriter INTERFACE, which has no Unwrap. An
|
// built in a handler (deadlines.go has the mechanism). So: the same scenario as
|
||||||
// http.ResponseController built from the handler's c.Writer unwraps layer by
|
// the first test, hosted on the engine New builds, in the order cmd/pansy runs
|
||||||
// layer looking for SetWriteDeadline, can't see past that wrapper, and returns
|
// it. Any future middleware that wraps the writer, or a reorder that puts one
|
||||||
// ErrNotSupported — putting the stream back on the server's absolute
|
// ahead of the controller capture, fails here.
|
||||||
// WriteTimeout. The first write past it fails, which cancels the request
|
|
||||||
// context and closes the socket under the client mid-frame.
|
|
||||||
//
|
|
||||||
// So: the same scenario as the first test, hosted on the engine New builds, in
|
|
||||||
// the order cmd/pansy runs it. Any future middleware that wraps the writer, or a
|
|
||||||
// reorder that puts one ahead of the controller capture, fails here.
|
|
||||||
func TestEventStreamOutlivesWriteTimeoutBehindMiddleware(t *testing.T) {
|
func TestEventStreamOutlivesWriteTimeoutBehindMiddleware(t *testing.T) {
|
||||||
got, err := streamFrames(t, authEngine(t, localCfg()), 300*time.Millisecond, 300*time.Millisecond, 3)
|
got, err := streamFrames(t, authEngine(t, localCfg()), 300*time.Millisecond, 300*time.Millisecond, 3)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
Reference in New Issue
Block a user
🟡 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