Closes#56 (risk/medium, so PR lane per rule 5; control code, so it needs an adversarial pass by execution per rule 8).
Mechanism: every read of the upload body calls http.ResponseController.SetReadDeadline(now + UploadIdleTimeout) (default 60 s). A stalled body fails its read with a timeout, Accept returns and the receive slot is released. A body that keeps moving is never cut. ReadTimeout was rejected because it would cut a slow 200 MiB upload.
Found by execution (each was red first):
metrics.statusWriter had no Unwrap, so through the production stack (metrics.HTTPMiddleware(app.Router())) SetReadDeadline returned ErrNotSupported and the bound silently did not apply. Added Unwrap. If a writer still can't set a deadline, the handler logs WARN upload idle bound unavailable.
Clearing the deadline on any read error left net/http's pre-reply drain of the unread remainder (under 256 KiB) unbounded, so the stalled connection hung after Accept returned. The deadline is now cleared only on a clean io.EOF.
Tests (internal/web/uploads_idle_handler_test.go, real socket, production middleware):
TestUploadPost_AConnectionReadDeadlineEndsABodyThatStallsAndReleasesItsReceiveSlot: half the body, then silence. The server ends the request within 3 s and the same user's next upload (MaxReceivesPerUser=1) gets 303.
TestUploadPost_TheReadDeadlineIsReArmedOnEveryReadSoASlowButMovingBodyIsAccepted: a ~1.2 s drip against a 300 ms bound gets 303.
Not covered (residual): a body that drips 1 byte every 59 s still holds its slot. This is a minimum-rate gap, recorded in ADR-033. A stalled request ends as upload abandoned (499 / connection closed), not as a 400 "cut off" page: the read error cancels the request context.
task check exit 0; -race -count=3 on the new tests is green.
Closes #56 (risk/medium, so PR lane per rule 5; control code, so it needs an adversarial pass by execution per rule 8).
**Mechanism:** every read of the upload body calls `http.ResponseController.SetReadDeadline(now + UploadIdleTimeout)` (default 60 s). A stalled body fails its read with a timeout, `Accept` returns and the receive slot is released. A body that keeps moving is never cut. `ReadTimeout` was rejected because it would cut a slow 200 MiB upload.
**Found by execution (each was red first):**
1. `metrics.statusWriter` had no `Unwrap`, so through the production stack (`metrics.HTTPMiddleware(app.Router())`) `SetReadDeadline` returned ErrNotSupported and the bound **silently did not apply**. Added `Unwrap`. If a writer still can't set a deadline, the handler logs WARN `upload idle bound unavailable`.
2. Clearing the deadline on *any* read error left net/http's pre-reply drain of the unread remainder (under 256 KiB) unbounded, so the stalled connection hung after Accept returned. The deadline is now cleared only on a clean `io.EOF`.
**Tests** (`internal/web/uploads_idle_handler_test.go`, real socket, production middleware):
- `TestUploadPost_AConnectionReadDeadlineEndsABodyThatStallsAndReleasesItsReceiveSlot`: half the body, then silence. The server ends the request within 3 s and the same user's next upload (MaxReceivesPerUser=1) gets 303.
- `TestUploadPost_TheReadDeadlineIsReArmedOnEveryReadSoASlowButMovingBodyIsAccepted`: a ~1.2 s drip against a 300 ms bound gets 303.
**Not covered (residual):** a body that drips 1 byte every 59 s still holds its slot. This is a minimum-rate gap, recorded in ADR-033. A stalled request ends as `upload abandoned` (499 / connection closed), not as a 400 "cut off" page: the read error cancels the request context.
`task check` exit 0; `-race -count=3` on the new tests is green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
A body that stopped arriving held one of the receive slots (#47) and a
spool file indefinitely. Every read of the upload body now pushes the
connection read deadline UploadIdleTimeout (default 60 s) ahead, so a
stalled body fails its read, Accept returns and the slot is released,
while a slow but moving body is never cut (ReadTimeout would cut it).
Two things found by running it over a socket:
- metrics.statusWriter had no Unwrap, so http.ResponseController could
not reach the connection and the bound silently did not apply in the
production handler stack. It now unwraps; a writer that still cannot
set a deadline logs "upload idle bound unavailable".
- clearing the deadline on any read error left net/http's pre-reply
drain of the unread remainder unbounded, so the connection hung. The
deadline is now cleared only on a clean io.EOF.
ADR-033 limitation amended. A 1-byte-per-59-s drip is still unbounded.
Closes#56
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
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>
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.
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 <noreply@anthropic.com>
Resolution is now func (a *App) uploadIdleTimeout() time.Duration. It returns UploadIdleTimeout when that is > 0, otherwise defaultUploadIdleTimeout. idleBoundedBody calls it.
export_test.go exposes it as (*App).UploadIdleTimeoutForTest().
New test TestAppUploadIdleTimeout_ResolvesUnsetToSixtySecondsAndHonoursASetValue asserts:
(&web.App{}) resolves to exactly 60*time.Second
UploadIdleTimeout: -1s also resolves to 60s
UploadIdleTimeout: 300ms resolves to 300ms
Renamed ...AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGaps to ...AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps (rule 9). Its comment now points to the new test for the exact value.
I kept the const-only test.
Mutant evidence (actually run): I wrote the test first, then implemented uploadIdleTimeout() as if a.UploadIdleTimeout <= 0 { return time.Hour }. Running go test ./internal/web/ gave RED on the new test only (expected: 1m0s, actual: 1h0m0s). With the real implementation (return defaultUploadIdleTimeout) it is green.
Gate:task check exited 0 with 0 FAIL lines, and the commit was chained with && on the gate.
## Round-2 BLOCK addressed (b1887e7)
This is option (a) from comment 15146.
- Resolution is now `func (a *App) uploadIdleTimeout() time.Duration`. It returns `UploadIdleTimeout` when that is > 0, otherwise `defaultUploadIdleTimeout`. `idleBoundedBody` calls it.
- `export_test.go` exposes it as `(*App).UploadIdleTimeoutForTest()`.
- New test `TestAppUploadIdleTimeout_ResolvesUnsetToSixtySecondsAndHonoursASetValue` asserts:
- `(&web.App{})` resolves to exactly `60*time.Second`
- `UploadIdleTimeout: -1s` also resolves to 60s
- `UploadIdleTimeout: 300ms` resolves to 300ms
- Renamed `...AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGaps` to `...AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps` (rule 9). Its comment now points to the new test for the exact value.
- I kept the const-only test.
**Mutant evidence (actually run):** I wrote the test first, then implemented `uploadIdleTimeout()` as `if a.UploadIdleTimeout <= 0 { return time.Hour }`. Running `go test ./internal/web/` gave **RED** on the new test only (`expected: 1m0s, actual: 1h0m0s`). With the real implementation (`return defaultUploadIdleTimeout`) it is green.
**Gate:** `task check` exited 0 with 0 FAIL lines, and the commit was chained with `&&` on the gate.
Not merged. Waiting for the reviewer's verdict.
---
_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.
Closes #56 (risk/medium, so PR lane per rule 5; control code, so it needs an adversarial pass by execution per rule 8).
Mechanism: every read of the upload body calls
http.ResponseController.SetReadDeadline(now + UploadIdleTimeout)(default 60 s). A stalled body fails its read with a timeout,Acceptreturns and the receive slot is released. A body that keeps moving is never cut.ReadTimeoutwas rejected because it would cut a slow 200 MiB upload.Found by execution (each was red first):
metrics.statusWriterhad noUnwrap, so through the production stack (metrics.HTTPMiddleware(app.Router()))SetReadDeadlinereturned ErrNotSupported and the bound silently did not apply. AddedUnwrap. If a writer still can't set a deadline, the handler logs WARNupload idle bound unavailable.io.EOF.Tests (
internal/web/uploads_idle_handler_test.go, real socket, production middleware):TestUploadPost_AConnectionReadDeadlineEndsABodyThatStallsAndReleasesItsReceiveSlot: half the body, then silence. The server ends the request within 3 s and the same user's next upload (MaxReceivesPerUser=1) gets 303.TestUploadPost_TheReadDeadlineIsReArmedOnEveryReadSoASlowButMovingBodyIsAccepted: a ~1.2 s drip against a 300 ms bound gets 303.Not covered (residual): a body that drips 1 byte every 59 s still holds its slot. This is a minimum-rate gap, recorded in ADR-033. A stalled request ends as
upload abandoned(499 / connection closed), not as a 400 "cut off" page: the read error cancels the request context.task checkexit 0;-race -count=3on the new tests is green.🤖 Generated with Claude Code
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.
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 <noreply@anthropic.com>Round-2 BLOCK addressed (
b1887e7)This is option (a) from comment 15146.
func (a *App) uploadIdleTimeout() time.Duration. It returnsUploadIdleTimeoutwhen that is > 0, otherwisedefaultUploadIdleTimeout.idleBoundedBodycalls it.export_test.goexposes it as(*App).UploadIdleTimeoutForTest().TestAppUploadIdleTimeout_ResolvesUnsetToSixtySecondsAndHonoursASetValueasserts:(&web.App{})resolves to exactly60*time.SecondUploadIdleTimeout: -1salso resolves to 60sUploadIdleTimeout: 300msresolves to 300ms...AnUnsetIdleTimeoutAppliesTheDefaultAndAcceptsABodyWithSubSecondGapsto...AnUnsetIdleTimeoutDoesNotCutABodyWithSubSecondGaps(rule 9). Its comment now points to the new test for the exact value.Mutant evidence (actually run): I wrote the test first, then implemented
uploadIdleTimeout()asif a.UploadIdleTimeout <= 0 { return time.Hour }. Runninggo test ./internal/web/gave RED on the new test only (expected: 1m0s, actual: 1h0m0s). With the real implementation (return defaultUploadIdleTimeout) it is green.Gate:
task checkexited 0 with 0 FAIL lines, and the commit was chained with&&on the gate.Not merged. Waiting for the reviewer's verdict.
Created via git-mcp on behalf of @mathias