From b1887e7594ddac9dcc8bf65a429d72afd2a41a9a Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 06:55:04 +0200 Subject: [PATCH] test(uploads): pin the idle bound an unset App resolves to (#56) Round-2 gatekeeper finding: production never sets UploadIdleTimeout, so it always runs the zero-means-default branch, and nothing asserted what that branch resolved to (`idle = time.Hour` kept the suite green). Extract the resolution into App.uploadIdleTimeout(), expose it via export_test.go, and assert (&web.App{}) resolves to exactly 60s, a negative value too, and a set value is honoured. Verified red against the time.Hour mutant before implementing. Rename the socket test to what it observes: ...AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps. Co-Authored-By: Claude Opus 5.5 --- internal/web/export_test.go | 6 ++++++ internal/web/uploads.go | 14 ++++++++++---- internal/web/uploads_idle_handler_test.go | 15 ++++++++++++--- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/internal/web/export_test.go b/internal/web/export_test.go index aa9ae63..057cb44 100644 --- a/internal/web/export_test.go +++ b/internal/web/export_test.go @@ -1,5 +1,11 @@ package web +import "time" + // DefaultUploadIdleTimeout exposes the production idle bound (#56) to the black-box tests: // cmd/tapir never sets App.UploadIdleTimeout, so this constant IS the deployed value. const DefaultUploadIdleTimeout = defaultUploadIdleTimeout + +// UploadIdleTimeoutForTest exposes the bound an App resolves to (#56), so a test can pin the +// value the zero-value App -- production's -- actually uses rather than only the constant. +func (a *App) UploadIdleTimeoutForTest() time.Duration { return a.uploadIdleTimeout() } diff --git a/internal/web/uploads.go b/internal/web/uploads.go index 654620e..468378a 100644 --- a/internal/web/uploads.go +++ b/internal/web/uploads.go @@ -142,6 +142,15 @@ func (a *App) handleUploadPost(w http.ResponseWriter, r *http.Request) { // hostile one; a legitimate upload, however slow, keeps delivering bytes. const defaultUploadIdleTimeout = 60 * time.Second +// uploadIdleTimeout is the idle bound this App applies: UploadIdleTimeout when set, else +// defaultUploadIdleTimeout. cmd/tapir leaves it unset, so production runs the default. +func (a *App) uploadIdleTimeout() time.Duration { + if a.UploadIdleTimeout <= 0 { + return defaultUploadIdleTimeout + } + return a.UploadIdleTimeout +} + // idleBoundedBody bounds the SILENCE in an upload body, not its total duration (#56): every // read first pushes the connection's read deadline idle into the future, so a body that // keeps moving is never cut, and one that stops fails its read with a timeout. That read @@ -152,10 +161,7 @@ const defaultUploadIdleTimeout = 60 * time.Second // the body is returned unbounded and that is logged: a control that silently does not // apply is worse than one that says so. func (a *App) idleBoundedBody(w http.ResponseWriter, body io.ReadCloser) io.ReadCloser { - idle := a.UploadIdleTimeout - if idle <= 0 { - idle = defaultUploadIdleTimeout - } + idle := a.uploadIdleTimeout() rc := http.NewResponseController(w) if err := rc.SetReadDeadline(time.Now().Add(idle)); err != nil { a.logger().Warn("upload idle bound unavailable", "err", err) diff --git a/internal/web/uploads_idle_handler_test.go b/internal/web/uploads_idle_handler_test.go index 760b142..717c304 100644 --- a/internal/web/uploads_idle_handler_test.go +++ b/internal/web/uploads_idle_handler_test.go @@ -121,9 +121,18 @@ func TestDefaultUploadIdleTimeoutIsSixtySeconds(t *testing.T) { require.Equal(t, 60*time.Second, web.DefaultUploadIdleTimeout) } -// With UploadIdleTimeout unset the default applies: a body with 100 ms gaps between reads is -// accepted (a default in milliseconds or nanoseconds would cut it). -func TestUploadPost_AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGaps(t *testing.T) { +// The bound an App actually resolves to, not the constant: production's App leaves +// UploadIdleTimeout unset, so the zero value must resolve to exactly 60 s, and a set value +// must be used as given. +func TestAppUploadIdleTimeout_ResolvesUnsetToSixtySecondsAndHonoursASetValue(t *testing.T) { + require.Equal(t, 60*time.Second, (&web.App{}).UploadIdleTimeoutForTest()) + require.Equal(t, 60*time.Second, (&web.App{UploadIdleTimeout: -time.Second}).UploadIdleTimeoutForTest()) + require.Equal(t, 300*time.Millisecond, (&web.App{UploadIdleTimeout: 300 * time.Millisecond}).UploadIdleTimeoutForTest()) +} + +// With UploadIdleTimeout unset, a body with 100 ms gaps between reads is not cut (a resolved +// bound in milliseconds or nanoseconds would cut it). The exact value is pinned above. +func TestUploadPost_AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps(t *testing.T) { addr := newIdleBoundServer(t, 0) body, ct := multipartBody(t, bytes.Repeat([]byte("a"), 1000))