From the adversarial review of PR #55 (finding 5), recorded as a limitation in ADR-033 (amended 2026-10-03).
cmd/tapir sets ReadHeaderTimeout (10 s) only; there is no ReadTimeout or IdleTimeout. A body sent very slowly therefore holds one of the 4 total / 2 per-user receive slots (#47) and a spool file indefinitely. Two slow bodies take a user's two slots; two such users make every upload answer "busy". The attacker needs a valid session (auth runs before the body is read). Not verified: whether Nginx Proxy Manager buffers request bodies and so shields tapir.
Why not just set ReadTimeout: it covers the whole body, so it would cut a legitimate 200 MiB upload on a slow link. The right control is a minimum-rate / idle-read deadline on the upload body (reset on each read, e.g. http.NewResponseController(w).SetReadDeadline extended per chunk, or a reader wrapper that fails after N seconds without a byte).
Red test first: a body reader that stalls for longer than the idle bound makes Accept return a client-read error (ErrClientRead) and frees its slot; a steady slow-but-moving body is accepted. Review: control code, adversarial pass by execution. Milestone v0.32.0, risk/medium.
From the adversarial review of PR #55 (finding 5), recorded as a limitation in ADR-033 (amended 2026-10-03).
`cmd/tapir` sets `ReadHeaderTimeout` (10 s) only; there is no `ReadTimeout` or `IdleTimeout`. A body sent very slowly therefore holds one of the 4 total / 2 per-user receive slots (#47) and a spool file indefinitely. Two slow bodies take a user's two slots; two such users make every upload answer "busy". The attacker needs a valid session (auth runs before the body is read). Not verified: whether Nginx Proxy Manager buffers request bodies and so shields tapir.
**Why not just set `ReadTimeout`:** it covers the whole body, so it would cut a legitimate 200 MiB upload on a slow link. The right control is a minimum-rate / idle-read deadline on the upload body (reset on each read, e.g. `http.NewResponseController(w).SetReadDeadline` extended per chunk, or a reader wrapper that fails after N seconds without a byte).
**Red test first:** a body reader that stalls for longer than the idle bound makes `Accept` return a client-read error (`ErrClientRead`) and frees its slot; a steady slow-but-moving body is accepted. **Review:** control code, adversarial pass by execution. Milestone v0.32.0, risk/medium.
---
_Created via git-mcp on behalf of @mathias_
mathias
added this to the v0.32.0 - audio upload follow-ups milestone 2026-10-03 19:13:11 +00:00
Mode: PR mode. CI run 9219 on the head sha is success. The local gate (task check) exited 0 when I ran it. Both are green, and neither covers the finding below.
The control holds under every stall I could construct
I ran these over a real socket through metrics.HTTPMiddleware(app.Router()), with idle = 300 ms and MaxReceivesPerUser = 1. Each stall ended about 300 ms after its last byte, and the next upload from the same user got the slot (303):
no body byte at all: 499
stall inside the first boundary, the title value, or the file part headers: 499
stall in the middle of the file data: 499
stall after the file data but before the final --: 499
full body sent, but Content-Length declares 1 or 50 000 more bytes: 303 sent after net/http's drain hits the deadline (~301 ms)
too large (MaxBytes 1000, then a stall with under 256 KiB declared as remaining): 413 after ~301 ms
busy (429), then a stall during the drain: 429 after ~301 ms
chunked body with the terminating 0\r\n\r\n never sent: ended, and the slot was released
Expect: 100-continue, then a stall: ended after ~300 ms
HTTP/2 over TLS (httptest with EnableHTTP2): the stall ended after ~303 ms. The per-stream deadline works there, and production listens on HTTP/1 only.
a legitimate upload whose probe takes 4x idle: 303, the job was submitted and the probe ctx was not cancelled. I tried this with the final bytes split off by 0, 1, 2, 4 and 6 bytes, and with a chunked body.
Why it is blocked: the production value of the control is not asserted anywhere
cmd/tapir/main.go never sets App.UploadIdleTimeout, so production always runs defaultUploadIdleTimeout. Both new tests set the timeout explicitly, and no test goes through the zero-means-default path. I mutated the code and ran go test ./internal/web/ ./internal/metrics/:
Mutation
Suite
remove statusWriter.Unwrap
RED (...StallsAndReleasesItsReceiveSlot)
remove the per-read re-arm
RED (...SlowButMovingBodyIsAccepted)
defaultUploadIdleTimeout = time.Nanosecond
green
defaultUploadIdleTimeout = 60 * time.Millisecond
green. With the server left at the default, a legitimate upload sent in 100 ms chunks gets 499: every real upload breaks.
defaultUploadIdleTimeout = 24 * time.Hour
green. The control is silently disabled in production.
remove the upload idle bound unavailable Warn
green. This fail-open signal is the whole safeguard for "a control that silently does not apply", and nothing asserts it.
remove the EOF SetReadDeadline(time.Time{})
green. See note 2.
How to reproduce the 60 ms row: change the const, then add a socket test that builds the server as in newIdleBoundServer but does not set UploadIdleTimeout, and sends a 50 kB multipart body in 5 kB writes 100 ms apart. It gets 499 instead of 303, while the rest of the suite stays green.
Fix (small): add (a) a socket test that runs with UploadIdleTimeout unset, pinning the default: a moving body is accepted, and a stall longer than the default is cut. Exporting the const through export_test.go and asserting == 60*time.Second is enough for the "is cut" half. Add (b) a test that a writer which cannot set a deadline (e.g. httptest.NewRecorder) produces the upload idle bound unavailable log line.
Notes (not blocking on their own)
The deadline is not always cleared at the end of the body. The multipart reader can end the file part without the body ever returning EOF, for example when the trailing \r\n arrives in a later segment. When that happens the deadline stays armed while ffprobe and the DB work run. Measured: with the tail split by 1 or 2 bytes and a probe slower than idle, the client still gets 303 and the job is submitted, but net/http's post-handler drain hits the expired deadline and the connection is closed instead of kept alive. The impact is low. But the sentence in DECISIONS.md, "the deadline is cleared only on a clean end of body", describes the reader's EOF, which is not the same as the multipart end.
Removing the EOF clear changes nothing observable.connReader.startBackgroundRead (net/http server.go:741) already does SetReadDeadline(time.Time{}) before it starts the background read, and it runs synchronously on the body's EOF. The comment says the clear exists so the deadline "cannot fire on the server's own background read". Either cite net/http's own clear and keep this as belt-and-braces, or remove it. As written, the comment claims something the code does not have to do.
Not checked
I ran no live deploy and no NPM/ingress body buffering.
The 1 byte per 59 s drip is declared out of scope, so I did not probe it.
I did not measure goroutine counts directly. "Ended" means the server answered or closed the connection, and the slot release was confirmed by the next upload succeeding.
## pr-gatekeeper verdict: BLOCKED (PR #63 @ b106b0d, closes #56)
Mode: PR mode. CI run 9219 on the head sha is `success`. The local gate (`task check`) exited 0 when I ran it. Both are green, and neither covers the finding below.
### The control holds under every stall I could construct
I ran these over a real socket through `metrics.HTTPMiddleware(app.Router())`, with idle = 300 ms and `MaxReceivesPerUser = 1`. Each stall ended about 300 ms after its last byte, and the next upload from the same user got the slot (303):
- no body byte at all: 499
- stall inside the first boundary, the title value, or the file part headers: 499
- stall in the middle of the file data: 499
- stall after the file data but before the final `--`: 499
- full body sent, but Content-Length declares 1 or 50 000 more bytes: 303 sent after net/http's drain hits the deadline (~301 ms)
- too large (MaxBytes 1000, then a stall with under 256 KiB declared as remaining): 413 after ~301 ms
- busy (429), then a stall during the drain: 429 after ~301 ms
- chunked body with the terminating `0\r\n\r\n` never sent: ended, and the slot was released
- `Expect: 100-continue`, then a stall: ended after ~300 ms
- HTTP/2 over TLS (httptest with EnableHTTP2): the stall ended after ~303 ms. The per-stream deadline works there, and production listens on HTTP/1 only.
- a legitimate upload whose probe takes 4x idle: 303, the job was submitted and the probe ctx was not cancelled. I tried this with the final bytes split off by 0, 1, 2, 4 and 6 bytes, and with a chunked body.
### Why it is blocked: the production value of the control is not asserted anywhere
`cmd/tapir/main.go` never sets `App.UploadIdleTimeout`, so production always runs `defaultUploadIdleTimeout`. Both new tests set the timeout explicitly, and no test goes through the zero-means-default path. I mutated the code and ran `go test ./internal/web/ ./internal/metrics/`:
| Mutation | Suite |
|---|---|
| remove `statusWriter.Unwrap` | RED (`...StallsAndReleasesItsReceiveSlot`) |
| remove the per-read re-arm | RED (`...SlowButMovingBodyIsAccepted`) |
| `defaultUploadIdleTimeout = time.Nanosecond` | **green** |
| `defaultUploadIdleTimeout = 60 * time.Millisecond` | **green**. With the server left at the default, a legitimate upload sent in 100 ms chunks gets **499**: every real upload breaks. |
| `defaultUploadIdleTimeout = 24 * time.Hour` | **green**. The control is silently disabled in production. |
| remove the `upload idle bound unavailable` Warn | **green**. This fail-open signal is the whole safeguard for "a control that silently does not apply", and nothing asserts it. |
| remove the EOF `SetReadDeadline(time.Time{})` | **green**. See note 2. |
How to reproduce the 60 ms row: change the const, then add a socket test that builds the server as in `newIdleBoundServer` but does **not** set `UploadIdleTimeout`, and sends a 50 kB multipart body in 5 kB writes 100 ms apart. It gets 499 instead of 303, while the rest of the suite stays green.
**Fix (small):** add (a) a socket test that runs with `UploadIdleTimeout` unset, pinning the default: a moving body is accepted, and a stall longer than the default is cut. Exporting the const through `export_test.go` and asserting `== 60*time.Second` is enough for the "is cut" half. Add (b) a test that a writer which cannot set a deadline (e.g. `httptest.NewRecorder`) produces the `upload idle bound unavailable` log line.
### Notes (not blocking on their own)
1. **The deadline is not always cleared at the end of the body.** The multipart reader can end the file part without the body ever returning EOF, for example when the trailing `\r\n` arrives in a later segment. When that happens the deadline stays armed while ffprobe and the DB work run. Measured: with the tail split by 1 or 2 bytes and a probe slower than idle, the client still gets 303 and the job is submitted, but net/http's post-handler drain hits the expired deadline and the connection is closed instead of kept alive. The impact is low. But the sentence in DECISIONS.md, "the deadline is cleared only on a clean end of body", describes the reader's EOF, which is not the same as the multipart end.
2. **Removing the EOF clear changes nothing observable.** `connReader.startBackgroundRead` (net/http server.go:741) already does `SetReadDeadline(time.Time{})` before it starts the background read, and it runs synchronously on the body's EOF. The comment says the clear exists so the deadline "cannot fire on the server's own background read". Either cite net/http's own clear and keep this as belt-and-braces, or remove it. As written, the comment claims something the code does not have to do.
### Not checked
- I ran no live deploy and no NPM/ingress body buffering.
- The 1 byte per 59 s drip is declared out of scope, so I did not probe it.
- I did not measure goroutine counts directly. "Ended" means the server answered or closed the connection, and the slot release was confirmed by the next upload succeeding.
Mode: PR mode. CI run 9264 on fd74140 is success, and task check exited 0 when I ran it with 0 FAIL lines. Neither covers the one remaining gap below.
What I re-ran
I re-ran the whole round-1 probe suite on fd74140 through metrics.HTTPMiddleware(app.Router()), and the results are identical to round 1. Every stall ended about 300 ms after its last byte and released the slot: no body, first boundary, title, part headers, file data, before the final --, Content-Length declaring 1 or 50k more bytes, too-large drain (413), busy drain (429), chunked body with no terminator, Expect: 100-continue, and HTTP/2. A legitimate upload whose probe runs 4x idle still gets 303 and the job is submitted. That holds with the end of the body split off by 0/1/2/4/6 bytes and with a chunked body, and the probe context was never cancelled. So removing the end-of-body clear did not reopen the background-read case: MaxBytesReader caches the EOF, so the wrapper cannot re-arm after net/http has cleared the deadline. A production-default server (timeout unset) accepts a body sent in 100 ms gaps.
Flip table, round 2 (PR suite only, go test ./internal/web/ ./internal/metrics/)
Mutation
Suite
remove statusWriter.Unwrap
RED
remove the per-read re-arm
RED
default = time.Nanosecond / 60 * time.Millisecond
RED (both new tests)
default = 24 * time.Hour / 59 * time.Second
RED (TestDefaultUploadIdleTimeoutIsSixtySeconds)
remove the upload idle bound unavailable Warn
RED
if idle <= 0 { return body } (unset means unbounded)
RED, but only incidentally: the warn test happens to leave the timeout unset
if idle <= 0 { idle = time.Hour } (the zero-means-default branch ignores the const)
green
The blocking finding
internal/web/uploads.go, the idle <= 0 branch of idleBoundedBody. Production runs exactly this branch, because cmd/tapir never sets UploadIdleTimeout, and nothing asserts what that branch produces. Run the last mutation in the table and production's bound becomes one hour, 60x weaker, with the suite green.
The test TestUploadPost_AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGaps passes under that mutation. Its name says "applies the default", but its assertion only shows that the unset bound is longer than about 100 ms. TestDefaultUploadIdleTimeoutIsSixtySeconds pins the const, not the value an unset App actually uses. This is the gap AGENT.md rule 9 describes: the name claims more than the assertion proves.
Repro: replace idle = defaultUploadIdleTimeout with idle = time.Hour and run go test -p 1 ./internal/web/. It stays green.
Fix (small), either option:
(a) Extract the resolution into func (a *App) uploadIdleTimeout() time.Duration, expose it in export_test.go, and assert (&web.App{}).UploadIdleTimeoutForTest() == 60*time.Second. You can drop or keep the const-only test.
(b) Rename the socket test to what it observes, e.g. ...AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps. That alone leaves the mutation green, so (a) is the actual fix.
Not checked
Same as round 1: nothing ran on the live cluster, I did not check ingress body buffering, and the declared 1 byte per 59 s drip is out of scope.
## pr-gatekeeper verdict, round 2: BLOCKED (PR #63 @ fd74140, closes #56)
Mode: PR mode. CI run 9264 on fd74140 is `success`, and `task check` exited 0 when I ran it with 0 FAIL lines. Neither covers the one remaining gap below.
### What I re-ran
I re-ran the whole round-1 probe suite on fd74140 through `metrics.HTTPMiddleware(app.Router())`, and the results are identical to round 1. Every stall ended about 300 ms after its last byte and released the slot: no body, first boundary, title, part headers, file data, before the final `--`, Content-Length declaring 1 or 50k more bytes, too-large drain (413), busy drain (429), chunked body with no terminator, `Expect: 100-continue`, and HTTP/2. A legitimate upload whose probe runs 4x idle still gets 303 and the job is submitted. That holds with the end of the body split off by 0/1/2/4/6 bytes and with a chunked body, and the probe context was never cancelled. So removing the end-of-body clear did not reopen the background-read case: `MaxBytesReader` caches the EOF, so the wrapper cannot re-arm after net/http has cleared the deadline. A production-default server (timeout unset) accepts a body sent in 100 ms gaps.
### Flip table, round 2 (PR suite only, `go test ./internal/web/ ./internal/metrics/`)
| Mutation | Suite |
|---|---|
| remove `statusWriter.Unwrap` | RED |
| remove the per-read re-arm | RED |
| default `= time.Nanosecond` / `60 * time.Millisecond` | RED (both new tests) |
| default `= 24 * time.Hour` / `59 * time.Second` | RED (`TestDefaultUploadIdleTimeoutIsSixtySeconds`) |
| remove the `upload idle bound unavailable` Warn | RED |
| `if idle <= 0 { return body }` (unset means unbounded) | RED, but only incidentally: the warn test happens to leave the timeout unset |
| **`if idle <= 0 { idle = time.Hour }`** (the zero-means-default branch ignores the const) | **green** |
### The blocking finding
`internal/web/uploads.go`, the `idle <= 0` branch of `idleBoundedBody`. Production runs exactly this branch, because `cmd/tapir` never sets `UploadIdleTimeout`, and nothing asserts what that branch produces. Run the last mutation in the table and production's bound becomes one hour, 60x weaker, with the suite green.
The test `TestUploadPost_AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGaps` passes under that mutation. Its name says "applies the default", but its assertion only shows that the unset bound is longer than about 100 ms. `TestDefaultUploadIdleTimeoutIsSixtySeconds` pins the const, not the value an unset `App` actually uses. This is the gap AGENT.md rule 9 describes: the name claims more than the assertion proves.
Repro: replace `idle = defaultUploadIdleTimeout` with `idle = time.Hour` and run `go test -p 1 ./internal/web/`. It stays green.
**Fix (small), either option:**
- (a) Extract the resolution into `func (a *App) uploadIdleTimeout() time.Duration`, expose it in `export_test.go`, and assert `(&web.App{}).UploadIdleTimeoutForTest() == 60*time.Second`. You can drop or keep the const-only test.
- (b) Rename the socket test to what it observes, e.g. `...AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps`. That alone leaves the mutation green, so (a) is the actual fix.
### Not checked
Same as round 1: nothing ran on the live cluster, I did not check ingress body buffering, and the declared 1 byte per 59 s drip is out of scope.
Mode: PR mode. CI run 9278 on b1887e7 is success. I ran task check myself and it exited 0, with 0 FAIL lines.
Flip table (PR suite only):
Mutation
Suite
unset resolves to 1 h
RED (TestAppUploadIdleTimeout_...)
default 60 ms
RED (3 tests)
a set value is ignored
RED (socket stall test and resolver test)
unset means unbounded
RED
remove Unwrap
RED
remove the per-read re-arm
RED
remove the warning
RED
Probe suite (same as rounds 1 and 2): every stall was cut about 300 ms after its last byte and released the slot. That covers no body, the boundary, the title, the part headers, the file data, before the final --, the post-reply drains (CL overstated, 413, 429), chunked with no terminator, Expect: 100-continue, and HTTP/2. A legitimate upload with a probe taking 4x idle got 303, the job was queued and the context was not cancelled. A production-default server accepts 100 ms gaps.
Residuals, accepted:
The 1 byte per 59 s drip, declared in DECISIONS.md.
A mutant where idleBoundedBody stops calling the resolver only for the unset case and re-implements it with another value still passes. No test can see that without a 60 s wait. The handler is shown to use the resolver for set values (the "set value ignored" mutant goes red).
Keep-alive is lost when the final CRLF arrives late and the probe outlasts idle. The upload is unaffected; this is documented.
Not checked: live deploy, and ingress body buffering.
## pr-gatekeeper verdict, round 3: APPROVED, merged (PR #63 @ b1887e7, closes #56)
Mode: PR mode. CI run 9278 on b1887e7 is `success`. I ran `task check` myself and it exited 0, with 0 FAIL lines.
**Flip table** (PR suite only):
| Mutation | Suite |
|---|---|
| unset resolves to 1 h | RED (`TestAppUploadIdleTimeout_...`) |
| default 60 ms | RED (3 tests) |
| a set value is ignored | RED (socket stall test and resolver test) |
| unset means unbounded | RED |
| remove `Unwrap` | RED |
| remove the per-read re-arm | RED |
| remove the warning | RED |
**Probe suite** (same as rounds 1 and 2): every stall was cut about 300 ms after its last byte and released the slot. That covers no body, the boundary, the title, the part headers, the file data, before the final `--`, the post-reply drains (CL overstated, 413, 429), chunked with no terminator, `Expect: 100-continue`, and HTTP/2. A legitimate upload with a probe taking 4x idle got 303, the job was queued and the context was not cancelled. A production-default server accepts 100 ms gaps.
**Residuals, accepted:**
- The 1 byte per 59 s drip, declared in DECISIONS.md.
- A mutant where `idleBoundedBody` stops calling the resolver *only* for the unset case and re-implements it with another value still passes. No test can see that without a 60 s wait. The handler is shown to use the resolver for set values (the "set value ignored" mutant goes red).
- Keep-alive is lost when the final CRLF arrives late and the probe outlasts idle. The upload is unaffected; this is documented.
**Not checked:** live deploy, and ingress body buffering.
Released as v0.33.1 on merge commit 813e679. v0.33.0, the #61 language picker, was already on main, so this is the next patch release. I briefly pushed it as v0.32.2 by mistake and deleted that tag within a minute. If its tag build produced an image :v0.32.2, that image is stray and nothing deploys it. CD deploys :813e679 through infra.
Released as **v0.33.1** on merge commit 813e679. v0.33.0, the #61 language picker, was already on main, so this is the next patch release. I briefly pushed it as v0.32.2 by mistake and deleted that tag within a minute. If its tag build produced an image `:v0.32.2`, that image is stray and nothing deploys it. CD deploys `:813e679` through infra.
---
_Created via git-mcp on behalf of @mathias_
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.
From the adversarial review of PR #55 (finding 5), recorded as a limitation in ADR-033 (amended 2026-10-03).
cmd/tapirsetsReadHeaderTimeout(10 s) only; there is noReadTimeoutorIdleTimeout. A body sent very slowly therefore holds one of the 4 total / 2 per-user receive slots (#47) and a spool file indefinitely. Two slow bodies take a user's two slots; two such users make every upload answer "busy". The attacker needs a valid session (auth runs before the body is read). Not verified: whether Nginx Proxy Manager buffers request bodies and so shields tapir.Why not just set
ReadTimeout: it covers the whole body, so it would cut a legitimate 200 MiB upload on a slow link. The right control is a minimum-rate / idle-read deadline on the upload body (reset on each read, e.g.http.NewResponseController(w).SetReadDeadlineextended per chunk, or a reader wrapper that fails after N seconds without a byte).Red test first: a body reader that stalls for longer than the idle bound makes
Acceptreturn a client-read error (ErrClientRead) and frees its slot; a steady slow-but-moving body is accepted. Review: control code, adversarial pass by execution. Milestone v0.32.0, risk/medium.Created via git-mcp on behalf of @mathias
pr-gatekeeper verdict: BLOCKED (PR #63 @
b106b0d, closes #56)Mode: PR mode. CI run 9219 on the head sha is
success. The local gate (task check) exited 0 when I ran it. Both are green, and neither covers the finding below.The control holds under every stall I could construct
I ran these over a real socket through
metrics.HTTPMiddleware(app.Router()), with idle = 300 ms andMaxReceivesPerUser = 1. Each stall ended about 300 ms after its last byte, and the next upload from the same user got the slot (303):--: 4990\r\n\r\nnever sent: ended, and the slot was releasedExpect: 100-continue, then a stall: ended after ~300 msWhy it is blocked: the production value of the control is not asserted anywhere
cmd/tapir/main.gonever setsApp.UploadIdleTimeout, so production always runsdefaultUploadIdleTimeout. Both new tests set the timeout explicitly, and no test goes through the zero-means-default path. I mutated the code and rango test ./internal/web/ ./internal/metrics/:statusWriter.Unwrap...StallsAndReleasesItsReceiveSlot)...SlowButMovingBodyIsAccepted)defaultUploadIdleTimeout = time.NanoseconddefaultUploadIdleTimeout = 60 * time.MilliseconddefaultUploadIdleTimeout = 24 * time.Hourupload idle bound unavailableWarnSetReadDeadline(time.Time{})How to reproduce the 60 ms row: change the const, then add a socket test that builds the server as in
newIdleBoundServerbut does not setUploadIdleTimeout, and sends a 50 kB multipart body in 5 kB writes 100 ms apart. It gets 499 instead of 303, while the rest of the suite stays green.Fix (small): add (a) a socket test that runs with
UploadIdleTimeoutunset, pinning the default: a moving body is accepted, and a stall longer than the default is cut. Exporting the const throughexport_test.goand asserting== 60*time.Secondis enough for the "is cut" half. Add (b) a test that a writer which cannot set a deadline (e.g.httptest.NewRecorder) produces theupload idle bound unavailablelog line.Notes (not blocking on their own)
\r\narrives in a later segment. When that happens the deadline stays armed while ffprobe and the DB work run. Measured: with the tail split by 1 or 2 bytes and a probe slower than idle, the client still gets 303 and the job is submitted, but net/http's post-handler drain hits the expired deadline and the connection is closed instead of kept alive. The impact is low. But the sentence in DECISIONS.md, "the deadline is cleared only on a clean end of body", describes the reader's EOF, which is not the same as the multipart end.connReader.startBackgroundRead(net/http server.go:741) already doesSetReadDeadline(time.Time{})before it starts the background read, and it runs synchronously on the body's EOF. The comment says the clear exists so the deadline "cannot fire on the server's own background read". Either cite net/http's own clear and keep this as belt-and-braces, or remove it. As written, the comment claims something the code does not have to do.Not checked
pr-gatekeeper verdict, round 2: BLOCKED (PR #63 @
fd74140, closes #56)Mode: PR mode. CI run 9264 on
fd74140issuccess, andtask checkexited 0 when I ran it with 0 FAIL lines. Neither covers the one remaining gap below.What I re-ran
I re-ran the whole round-1 probe suite on
fd74140throughmetrics.HTTPMiddleware(app.Router()), and the results are identical to round 1. Every stall ended about 300 ms after its last byte and released the slot: no body, first boundary, title, part headers, file data, before the final--, Content-Length declaring 1 or 50k more bytes, too-large drain (413), busy drain (429), chunked body with no terminator,Expect: 100-continue, and HTTP/2. A legitimate upload whose probe runs 4x idle still gets 303 and the job is submitted. That holds with the end of the body split off by 0/1/2/4/6 bytes and with a chunked body, and the probe context was never cancelled. So removing the end-of-body clear did not reopen the background-read case:MaxBytesReadercaches the EOF, so the wrapper cannot re-arm after net/http has cleared the deadline. A production-default server (timeout unset) accepts a body sent in 100 ms gaps.Flip table, round 2 (PR suite only,
go test ./internal/web/ ./internal/metrics/)statusWriter.Unwrap= time.Nanosecond/60 * time.Millisecond= 24 * time.Hour/59 * time.SecondTestDefaultUploadIdleTimeoutIsSixtySeconds)upload idle bound unavailableWarnif idle <= 0 { return body }(unset means unbounded)if idle <= 0 { idle = time.Hour }(the zero-means-default branch ignores the const)The blocking finding
internal/web/uploads.go, theidle <= 0branch ofidleBoundedBody. Production runs exactly this branch, becausecmd/tapirnever setsUploadIdleTimeout, and nothing asserts what that branch produces. Run the last mutation in the table and production's bound becomes one hour, 60x weaker, with the suite green.The test
TestUploadPost_AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGapspasses under that mutation. Its name says "applies the default", but its assertion only shows that the unset bound is longer than about 100 ms.TestDefaultUploadIdleTimeoutIsSixtySecondspins the const, not the value an unsetAppactually uses. This is the gap AGENT.md rule 9 describes: the name claims more than the assertion proves.Repro: replace
idle = defaultUploadIdleTimeoutwithidle = time.Hourand rungo test -p 1 ./internal/web/. It stays green.Fix (small), either option:
func (a *App) uploadIdleTimeout() time.Duration, expose it inexport_test.go, and assert(&web.App{}).UploadIdleTimeoutForTest() == 60*time.Second. You can drop or keep the const-only test....AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps. That alone leaves the mutation green, so (a) is the actual fix.Not checked
Same as round 1: nothing ran on the live cluster, I did not check ingress body buffering, and the declared 1 byte per 59 s drip is out of scope.
pr-gatekeeper verdict, round 3: APPROVED, merged (PR #63 @
b1887e7, closes #56)Mode: PR mode. CI run 9278 on
b1887e7issuccess. I rantask checkmyself and it exited 0, with 0 FAIL lines.Flip table (PR suite only):
TestAppUploadIdleTimeout_...)UnwrapProbe suite (same as rounds 1 and 2): every stall was cut about 300 ms after its last byte and released the slot. That covers no body, the boundary, the title, the part headers, the file data, before the final
--, the post-reply drains (CL overstated, 413, 429), chunked with no terminator,Expect: 100-continue, and HTTP/2. A legitimate upload with a probe taking 4x idle got 303, the job was queued and the context was not cancelled. A production-default server accepts 100 ms gaps.Residuals, accepted:
idleBoundedBodystops calling the resolver only for the unset case and re-implements it with another value still passes. No test can see that without a 60 s wait. The handler is shown to use the resolver for set values (the "set value ignored" mutant goes red).Not checked: live deploy, and ingress body buffering.
Released as v0.33.1 on merge commit
813e679. v0.33.0, the #61 language picker, was already on main, so this is the next patch release. I briefly pushed it as v0.32.2 by mistake and deleted that tag within a minute. If its tag build produced an image:v0.32.2, that image is stray and nothing deploys it. CD deploys:813e679through infra.Created via git-mcp on behalf of @mathias