From fd741408557e64c524f46a894d0f3d27c97dddcb Mon Sep 17 00:00:00 2001 From: Mathias Date: Sun, 4 Oct 2026 23:10:51 +0200 Subject: [PATCH] test(uploads): pin the deployed idle bound and its unavailable warning (#56) Gatekeeper review of PR #63 (comment 15139): - the 60 s default is what production runs (cmd/tapir never sets UploadIdleTimeout) and no test covered it; 1 ns, 60 ms or 24 h all left the suite green. Pinned via export_test.go, plus a socket test with the timeout unset that accepts a body with 100 ms gaps (60 ms mutant red). - the "upload idle bound unavailable" warning, the only signal that the control did not apply, is now asserted (removing it goes red). - the deadline clear on io.EOF was redundant (net/http resets the deadline before its background read) and is removed; ADR-033 text corrected (the part can end before the body, so "cleared on clean EOF" did not hold). Co-Authored-By: Claude Opus 5.5 --- DECISIONS.md | 9 ++++-- internal/web/export_test.go | 5 +++ internal/web/uploads.go | 13 +++----- internal/web/uploads_idle_handler_test.go | 38 +++++++++++++++++++++++ 4 files changed, 53 insertions(+), 12 deletions(-) create mode 100644 internal/web/export_test.go diff --git a/DECISIONS.md b/DECISIONS.md index 618ee6b..b72da0d 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1465,9 +1465,12 @@ an hour of Swedish *was* an unknown here; it is now measured, see decision 8.) 2026-10-04)**: a `ReadTimeout` would cut legitimate 200 MiB uploads on slow links, so instead every read of the upload body re-arms the connection's read deadline 60 s ahead (`App.UploadIdleTimeout`). A body that stops delivering bytes fails its read, Accept returns - and the slot is released; one that keeps moving is never cut. The deadline is cleared only on - a clean end of body and stays armed after an error, because net/http drains an unread - remainder before replying. It needs `http.ResponseController` to reach the connection, so the + and the slot is released; one that keeps moving is never cut. The handler never clears the + deadline: an armed deadline also bounds net/http's drain of an unread remainder before it + replies, and net/http resets it itself before its background read. A deadline still armed + while the file is probed (the multipart part can end before the body's last bytes arrive) + costs at most keep-alive reuse of that connection, not the upload. The 60 s default is the + deployed value (cmd/tapir does not set it) and is pinned by a test. It needs `http.ResponseController` to reach the connection, so the metrics middleware's writer implements `Unwrap`; where a deadline cannot be set the handler logs `upload idle bound unavailable` rather than silently running unbounded. Still not bounded: a body that drips one byte per 59 s holds its slot for as long as it keeps that up. diff --git a/internal/web/export_test.go b/internal/web/export_test.go new file mode 100644 index 0000000..aa9ae63 --- /dev/null +++ b/internal/web/export_test.go @@ -0,0 +1,5 @@ +package web + +// 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 diff --git a/internal/web/uploads.go b/internal/web/uploads.go index 3ed817e..654620e 100644 --- a/internal/web/uploads.go +++ b/internal/web/uploads.go @@ -172,15 +172,10 @@ type idleDeadlineBody struct { func (b *idleDeadlineBody) Read(p []byte) (int, error) { _ = b.rc.SetReadDeadline(time.Now().Add(b.idle)) - n, err := b.ReadCloser.Read(p) - if errors.Is(err, io.EOF) { - // The body is complete: disarm, so the deadline cannot fire on the server's own - // background read while the handler is still probing the file. On any other error - // it stays armed: net/http drains an unread remainder before replying, and that - // drain must stay bounded too. - _ = b.rc.SetReadDeadline(time.Time{}) - } - return n, err + // Never cleared here: net/http clears the read deadline itself before its background + // read once the handler has the body, and an armed deadline after an error also bounds + // net/http's drain of an unread remainder before it replies. + return b.ReadCloser.Read(p) } // abandoned reports, logs at INFO and answers 499, because the client has already gone: its request diff --git a/internal/web/uploads_idle_handler_test.go b/internal/web/uploads_idle_handler_test.go index 1e54b59..760b142 100644 --- a/internal/web/uploads_idle_handler_test.go +++ b/internal/web/uploads_idle_handler_test.go @@ -9,6 +9,7 @@ import ( "bytes" "errors" "fmt" + "log/slog" "mime/multipart" "net" "net/http" @@ -21,6 +22,7 @@ import ( "git.d-ma.be/mathias/tapir/internal/metrics" "git.d-ma.be/mathias/tapir/internal/uploads" + "git.d-ma.be/mathias/tapir/internal/web" ) func multipartBody(t *testing.T, content []byte) (body []byte, contentType string) { @@ -112,3 +114,39 @@ func TestUploadPost_TheReadDeadlineIsReArmedOnEveryReadSoASlowButMovingBodyIsAcc require.Equal(t, http.StatusSeeOther, readStatus(t, c, 3*time.Second)) } + +// cmd/tapir never sets UploadIdleTimeout, so the default is the value production runs. +// Shorter and legitimate uploads on a slow link are cut; much longer and the bound is moot. +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) { + addr := newIdleBoundServer(t, 0) + body, ct := multipartBody(t, bytes.Repeat([]byte("a"), 1000)) + + c := rawUpload(t, addr, ct, len(body)) + const chunks = 5 + step := len(body)/chunks + 1 + for i := 0; i < len(body); i += step { + time.Sleep(100 * time.Millisecond) + _, err := c.Write(body[i:min(i+step, len(body))]) + require.NoError(t, err) + } + + require.Equal(t, http.StatusSeeOther, readStatus(t, c, 3*time.Second)) +} + +// A writer that cannot set a read deadline leaves the body unbounded; that must be said in +// the log, not happen silently. The in-process recorder is such a writer. +func TestUploadPost_AWriterWithoutReadDeadlineSupportLogsThatTheIdleBoundIsUnavailable(t *testing.T) { + app, _ := newUploadApp(t, audioOK, 200<<20) + buf := &lockedBuf{} + app.Log = slog.New(slog.NewTextHandler(buf, nil)) + + do(t, app, uploadReq(t, "t", "a.m4a", []byte("x"))) + + require.Contains(t, buf.String(), "upload idle bound unavailable") +}