🐛 fix(http): bound the gap in a request body - #2182
Merged
Merged
Conversation
Nothing bounded how long a client could hold a handler. A request that stopped arriving occupied it until the connection died, and every lock that handler held was held for the same span. One stalled PATCH could therefore block the reclaim of every other expired upload session, since the pass takes each session's gate in turn. The edge now fails a request body that goes thirty seconds without a frame, with the wait starting again at each frame. The gap is the right thing to bound because a large upload over a slow link is making progress and has to finish: a total bound would have to exceed the slowest legitimate transfer, which leaves it far too loose to catch a stall, and it would cut across the throughput floor that already governs long transfers from the other direction. Thirty seconds is the span peryx already allows an upstream it is reading from, applied to the other end. The wrapper forwards the length the inner body declares. Wrapping with the one tower-http provides broke every chunked upload, because it drops the size hint that Content-Range is checked against, which turned each ranged chunk into a rejected one. Six tests said so. A cut chunk leaves the session recorded at the bytes that reached disk, which the append path already did for a client that disconnects, so the client resumes at that offset or cancels. The reclaim pass now waits one bound rather than as long as the client stays connected, and a test measures that wait.
Merging this PR will not alter performance
Comparing Footnotes
|
This was referenced Sep 4, 2026
gaborbernat
added a commit
that referenced
this pull request
Sep 4, 2026
An upload whose body failed answered 502 with "upstream transfer failed". No upstream serves a request body: the bytes were coming from the client and peryx is the server. The stall bound #2182 added made it reachable with the client still connected, so a client that paused for thirty seconds was told something upstream of peryx had gone wrong. 502 also carries a meaning to intermediaries, a bad response from an upstream server, which invites a retry against a different backend for a condition no backend change reaches. A stall now answers 408, which says the server gave up waiting for a message that never arrived, and names the offset a resumable session stands at so the client knows where to continue. Anything else the body ends with answers 400, since repeating it unchanged fails the same way. Both come from the shared classification rather than from each call site reading an opaque error. Four tests asserted the old status. Their intent survives unchanged: the stall pair asserted a cut chunk must not read as accepted, which 408 satisfies, and the monolithic case was named for the gateway status it happened to produce rather than for a gateway being involved.
This was referenced Sep 4, 2026
gaborbernat
added a commit
that referenced
this pull request
Sep 4, 2026
An upload whose body failed answered 502 with "upstream transfer failed". No upstream serves a request body: the bytes were coming from the client and peryx is the server. The stall bound #2182 added made it reachable with the client still connected, so a client that paused for thirty seconds was told something upstream of peryx had gone wrong. 502 also carries a meaning to intermediaries, a bad response from an upstream server, which invites a retry against a different backend for a condition no backend change reaches. A stall now answers 408, which says the server gave up waiting for a message that never arrived, and names the offset a resumable session stands at so the client knows where to continue. Anything else the body ends with answers 400, since repeating it unchanged fails the same way. Both come from the shared classification rather than from each call site reading an opaque error. Four tests asserted the old status. Their intent survives unchanged: the stall pair asserted a cut chunk must not read as accepted, which 408 satisfies, and the monolithic case was named for the gateway status it happened to produce rather than for a gateway being involved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nothing bounded how long a client could hold a handler. A request that stopped arriving occupied it until the connection died, and any lock that handler took stayed taken for as long. The upload path shows the cost:
reclaim_idletakes each expired session's gate in turn, so one stalledPATCHblocked the reclaim of every other expired session behind it.The edge now fails a request body that goes thirty seconds without a frame, and the wait starts again at each frame. ⏱️ The gap is the thing to bound, because a large upload over a slow link is making progress and has to finish. A total bound would have to exceed the slowest legitimate transfer, which leaves it far too loose to catch a stall, and no single value serves both an upload chunk and a multi-gigabyte layer. A gap bound needs no such split. Silence means the same thing on every route, so this stays one number in one place rather than a per-handler patchwork, and thirty seconds is the span peryx already allows an upstream it is reading from, turned around to face the sender.
It composes with the throughput floor from PR #2149 by governing a different stream on a different axis. That floor bounds bytes arriving from an upstream peryx reads; this bounds bytes arriving from a client peryx serves. Neither counts total elapsed time, so a transfer that keeps delivering trips neither. A total inbound bound would have overridden the floor outright, cutting the 2 GiB layer that floor exists to let finish.
A client sees the status the append path already answers for a body that fails mid-chunk, and the session it was filling stands at the bytes that reached disk, so it can read that offset back with a status
GET, send the rest, or cancel. One wart rides along. That status is502carrying a message about an upstream transfer, which never fitted a client-side body failure and which a connected client can now read. Correcting it means changing how each ecosystem classifies a body error, the patchwork this issue exists to avoid, so it belongs in its own change.Wrapping with the body
tower-httpships broke every chunked upload:TimeoutBodydrops the size hint thatContent-Rangechecks against, turning each ranged chunk into a rejected one. 🔍 Six existing tests caught it. The wrapper here forwards that hint, which is why it exists rather than coming from the crate.A test measures the reclaim case rather than arguing it. It stalls a
PATCHon an expired session, runs a pass, and pins the delay to one stall bound instead of the client's connection lifetime, with both sessions reclaimed at the end. A client that trickles rather than stalls still evades this, the way an outbound trickle evaded the read timeout until #2149 added a floor on top of it, and that case wants the same second bound and an issue of its own.Closes #2176