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 <noreply@anthropic.com>
This commit is contained in:
+6
-3
@@ -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
|
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
|
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
|
(`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
|
and the slot is released; one that keeps moving is never cut. The handler never clears the
|
||||||
a clean end of body and stays armed after an error, because net/http drains an unread
|
deadline: an armed deadline also bounds net/http's drain of an unread remainder before it
|
||||||
remainder before replying. It needs `http.ResponseController` to reach the connection, so the
|
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
|
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
|
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.
|
bounded: a body that drips one byte per 59 s holds its slot for as long as it keeps that up.
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -172,15 +172,10 @@ type idleDeadlineBody struct {
|
|||||||
|
|
||||||
func (b *idleDeadlineBody) Read(p []byte) (int, error) {
|
func (b *idleDeadlineBody) Read(p []byte) (int, error) {
|
||||||
_ = b.rc.SetReadDeadline(time.Now().Add(b.idle))
|
_ = b.rc.SetReadDeadline(time.Now().Add(b.idle))
|
||||||
n, err := b.ReadCloser.Read(p)
|
// Never cleared here: net/http clears the read deadline itself before its background
|
||||||
if errors.Is(err, io.EOF) {
|
// read once the handler has the body, and an armed deadline after an error also bounds
|
||||||
// The body is complete: disarm, so the deadline cannot fire on the server's own
|
// net/http's drain of an unread remainder before it replies.
|
||||||
// background read while the handler is still probing the file. On any other error
|
return b.ReadCloser.Read(p)
|
||||||
// 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
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// abandoned reports, logs at INFO and answers 499, because the client has already gone: its request
|
// abandoned reports, logs at INFO and answers 499, because the client has already gone: its request
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import (
|
|||||||
"bytes"
|
"bytes"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"log/slog"
|
||||||
"mime/multipart"
|
"mime/multipart"
|
||||||
"net"
|
"net"
|
||||||
"net/http"
|
"net/http"
|
||||||
@@ -21,6 +22,7 @@ import (
|
|||||||
|
|
||||||
"git.d-ma.be/mathias/tapir/internal/metrics"
|
"git.d-ma.be/mathias/tapir/internal/metrics"
|
||||||
"git.d-ma.be/mathias/tapir/internal/uploads"
|
"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) {
|
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))
|
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")
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user