Repository navigation
Conversation
With Expect: 100-continue the HTTP/2 client held the request body until a 100 or a final status arrived. Nothing else ended the wait, so an origin that never answers the expectation got no DATA frame and the request ended only with the idle timeout. While a body is held, the session now arms its socket timer for the next tick. The first tick at least 1 s after the HEADERS sends the body as if 100 Continue had arrived.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe HTTP/2 client now tracks ChangesExpect Continue Timeout
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change addresses delayed HTTP/2 request bodies, and the inspected timeout paths retain connection monitoring. No concrete merge-blocking risk is established. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduction (bun 1.4.3 canary and a debug build of main, Linux x64):
Before: the origin gets no DATA frame, and there is no response after 10 s. The request ends only with the 360 s idle timeout. Tests: CI: the diff is green. Head One question needs a maintainer: keep the hold with this bound, or delete the hold so that the h2 client sends the body at once, as the h1 and h3 clients do. The Notes block of the description has the numbers for both. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the generalized ClientSession::enter in src/http/h2_client/ClientSession.rs: the closure result is a plain bool computed before the socket_ref_owed release, so the SAFETY comment still holds for the new on_timeout entry point, and a fail_all teardown inside release_held_bodies is handled by the same owed-ref path as the other entry points. The on_timeout == true early return in HTTPContext.rs leaves the socket alive with a freshly armed timer, so it does not strand a dead socket.
Extended reasoning...
Findings were reported inline (a confirmed idle-timeout regression for sibling streams plus two test nits), so this body only records what else was examined. I re-read the enter generalization against its SAFETY comment: result is produced by the closure before the owed socket ref is released, the return type is bool with no borrow of the session, and release_held_bodies calling fail_all follows the same teardown contract as on_close/on_data. I also traced the Handler::on_timeout change: when ClientSession::on_timeout returns true the session has already re-armed the socket timer in rearm_timeout, so skipping mark_socket_as_dead is correct; when it returns false the prior idle-timeout failure path is unchanged. The content-length concern was ruled out earlier in this run. A human should still weigh the confirmed idle-timeout finding and the maintainer question in the PR description (keep vs delete the hold).
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
test/js/web/fetch/fetch-http2-client.test.ts— nit: maintainers get no test that fails if the 1 s hold bound is dropped, so the clause that keepsExpectmeaningful is unverified.release_held_bodiesonly clears streams held forEXPECT_CONTINUE_TIMEOUT(ClientSession.rs:966-968); removing that comparison still passes every test. The existing "withholds the body until 100 arrives" test records client frames inseenbut never checks that the DATA came after the server wrote the 100. Fix: assert ordering against the server's own write, e.g. push a marker intoseenwhen the 100 is written at test line 1289 and requirefirstDatato be past it, so a release before the 100 fails the test.Extended reasoning...
REVIEW.md asks that deleting each load-bearing clause of a fix breaks at least one test. The new mechanism has two clauses: the
continue_timer_armedguard (covered by the sibling-reset test) and thenow.saturating_duration_since(since) >= EXPECT_CONTINUE_TIMEOUTcomparison at src/http/h2_client/ClientSession.rs:966-968. Without the comparison, a body queued a few milliseconds before a 4 s sweep tick is released on that tick, before the origin has had any chance to send 100, which defeats the expectation the header asks for. The two new tests (lines 1365 and 1436) use servers that never send 100, so they pass with or without the comparison. The pre-existing test at line 1263 sends the 100 after 20 ms and asserts onlyseen[0].type === 1,firstData > 0andseen[firstData].len === 20; these hold whether the DATA was released by the 100 or by a tick, becauseseencontains client-to-server frames only and has no record of when the 100 was written. The clause is therefore untested. Recording the server's write as a marker inseenand assertingfirstDatais past it makes the…Verification: nit. Triggering condition: a maintainer deletes or breaks the
>= EXPECT_CONTINUE_TIMEOUTcomparison; the suite stays green. Mechanism verified:release_held_bodies(src/http/h2_client/ClientSession.rs:962-971) clearsawaiting_continueonly whennow.saturating_duration_since(since) >= EXPECT_CONTINUE_TIMEOUT; that comparison is the sole thing that makes the constant's documented contract…
withRawH2Frames hands every client frame to a callback. withRawH2Server is now a wrapper over it, and the three Expect: 100-continue tests use it instead of their own copies of the parse loop. The hold test now records where the server wrote the 100 and asserts that the DATA frame comes after it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 4:38 PM PT - Sep 18th, 2026
✅ @robobun, your commit 88e06c638b48fe7bb6f1184a6996a6be57e50a44 passed in 🧪 To try this PR locally: bunx bun-pr 43385That installs a local version of the PR into your bun-43385 --bun |
|
On the finding in the review body (no test fails if the 1 s bound is dropped):
The two inline threads are answered and resolved. The comment length findings are fixed in 88e06c6. |
…xpect-continue-timeout
…xpect-continue-timeout
Problem
fetchPOST withexpect: 100-continueover HTTP/2 (protocol: "http2") sends HEADERS and holds the body until100 Continueor a final status arrives. If neither arrives, no DATA frame follows. Only the idle timeout ends the request (The operation timed out, code 23, after 360 s).100 Continueon HTTP/2 (ngx_http_test_expectreturns early for h2 streams), so this is the normal path there.write_requestsetsStream.awaiting_continue(src/http/h2_client/encode.rs:193). Onlydecode_header_blockclears it (src/http/h2_client/dispatch.rs:683).Fix
rearm_timeoutarms the session's socket timer for its next tick. On that tick,release_held_bodiessends each body held for at least 1 s and arms the idle timeout again.test/js/web/fetch/fetch-http2-client.test.tstime out without the fix. All 77 tests in the file pass on Linux, the fourExpecttests on Windows (debug builds).Background
Expect: 100-continueasks the origin for100 Continuebefore the client sends the body. After a final status instead, the client sends no body.ClientSessionis one HTTP/2 connection offetch, with oneStreamper request, on the HTTP thread.rearm_timeoutuses it as the session's idle timeout.Notes
Question for a maintainer: keep the hold, or delete it? This PR keeps it and puts a bound on it. The other option is to delete the hold (about 40 lines less in
encode.rsanddispatch.rs). Then the h2 client sends the body at once, as the h1 and h3 clients do, and no request waits. The cost is the case thatExpect: 100-continue with final status before 100 skips body uploadtests: an origin that rejects early no longer saves the upload. HTTP/2 flow control already limits that upload to one stream window (64 KB by default). If the answer is delete, I will replace this PR with that change.Report: a fuzz run found this (31 of 4,500 cells, every hang had this shape). There is no GitHub issue. It is not a regression: 1.4.2 and main behave the same.
Related, both closed without a merge and without a maintainer decision: #35781 (hold the body on the h1 client too) and #35788 (do not forward
Expectat all).A
100or a final status that arrives before the tick behaves as before.Measurements with this PR, against an h2 origin that never sends
100:The first tick comes 4 s after the loop gets its first socket, so a new process waits about 4 s. A request that starts when the previous one ends starts just after a tick, so it waits 4 s too. A caller deadline shorter than the wait (for example
AbortSignal.timeout(3000)) still fires first.A 1 s wait needs a fine timer that the HTTP thread does not have. On epoll and kqueue, uSockets has no
us_timer_t. The only clock is the timeout wheel, which is also what thetimeoutoption offetchuses.Why
continue_timer_armedis a field and is not derived when the timer fires: if the origin resets the held stream, nothing arms the timer again (RST_STREAM does not count as progress). The tick then finds no held stream. Without the field, that tick is taken for the idle timeout and fails every other request on the connection. The second new test covers this. With the field removed, its sibling request fails withThe operation timed out(code 23).The idle timeout does not run during a hold. It restarts from zero when the hold ends, as it does after any write. So an idle deadline slips by at most one hold.
The existing hold test (
withholds the body until 100 arrives) now asserts that the DATA frame comes after the server wrote the100. The threeExpecttests share one raw frame server helper,withRawH2Frames.Under
Expect: 100-continuethe h2 client does not sendcontent-length(the body can be abandoned). This PR does not change that, so a body that the timer releases also goes withoutcontent-length.Self-reviewed. The review agreed with a bound on the hold. It asked for the measured wait, the nginx fact, the related PRs, and the keep or delete question in this description. It also led to the second test and to the single pass in
rearm_timeout.[human-review] gate passed · iteration 1 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file