Repository navigation
Conversation
…cols The HTTP/1 path and the HTTP/2 and HTTP/3 path each had their own once-only flag and their own warning text. A process that timed out one request on each protocol printed the warning two times. Keep the one handler and flag in server_body.rs and use it from the HTTP/1 request path as well. should_add_timeout_handler_for_warning reads the same flag, so a later request does not register the handler again after the warning printed. Fixes #43016
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe change moves idle-timeout warning state and callback handling into ChangesIdle-timeout warning handling
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The timeout warning is emitted once across HTTP/1, HTTP/2, and HTTP/3, with no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 swap(true)-before-silent ordering the HTTP/1 path now inherits from the shared handler: both registration sites (should_add_timeout_handler_for_warning in mod.rs and the gate at server_body.rs:2950) already skip registering the callback when --silent is set, and silent is a process-wide CLI flag, so the handler can never run in a silent process and the latch-without-print case is unreachable.
Extended reasoning...
The Rust side of this diff is a small consolidation: two once-latches and two handler bodies become one, with the HTTP/1 path calling the shared server_body::on_timeout_for_idle_warn through a non-capturing closure shim. I traced the one semantic difference between the deleted and surviving handler (latching before the silent check) and confirmed it is unreachable because every registration site is gated on !silent, which cannot change during the process. The remaining concerns are the test-quality issues already posted inline (the HTTP/2 leg can pass without the stream reaching the server; the 10s wall-clock wait plus 30s per-test timeout), so a human still needs to weigh those before merge and I am not approving.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 1:10 AM PT - Sep 17th, 2026
✅ @robobun, your commit 352926d7d00e5dd89a6f490b8cdf6351e26c1bc5 passed in 🧪 To try this PR locally: bunx bun-pr 43017That installs a local version of the PR into your bun-43017 --bun |
There was a problem hiding this comment.
The follow-up commits address the point I raised about the HTTP/2 leg (the child now prints requests seen: h1,h2, so a client that fails before reaching the handler can no longer pass), and I found no bugs in the Rust change. One thing still needs a maintainer call rather than a bot: the test inherently waits out the default 10 s idle timer on two connections and sets a 30 s per-test ceiling, which the repo's test guidance treats as an exception to sign off on explicitly.
What was reviewed:
- The HTTP/1 closure in
mod.rsis byte-for-byte the shape already used byRespLike::on_timeout_warnforNewAppResponseinserver_body.rs, andshould_add_timeout_handler_for_warningstill gates onDEBUG && !silent && !has_idle_timeout, so dropping theDEBUGcheck inside the shared handler is safe. - Grepped for assertions on the old
[Bun.serve]: request timed outtext — none remain outside the new test. - Latching the flag with
swapbefore thesilentcheck is pre-existing behavior of the shared handler and unreachable when silent, since the handler is never registered then.
Extended reasoning...
Overview
The PR removes the HTTP/1-only copy of the dev-mode idle-timeout warning (NewServer::on_timeout_for_idle_warn and its per-monomorphization AtomicBool) from src/runtime/server/mod.rs and routes the HTTP/1 request path through the shared server_body::on_timeout_for_idle_warn / did_send_idletimeout_warning_once that HTTP/2 and HTTP/3 already used. The shared handler's text switches from the [Bun.serve]: pretty_errorln! form to bun_core::warn!("Bun.serve() timed out a request after 10 seconds. ...") followed by Output::flush(). A regression test at test/regression/issue/43016.test.ts spawns one process with development: true, http2: true, stalls one HTTP/1 POST and one h2c POST stream, and asserts exactly one warn: line and no [Bun.serve]: line. Since my prior review, commits 58cf4ea and 352926d changed only doc comments and the test.
Security risks
None. The change touches a once-per-process diagnostic latch and its message text. The user-data pointer passed to uWS is an opaque sentinel that the handler never dereferences, exactly as before. No user input is parsed, no allocation or lifetime changes.
Level of scrutiny
Low for the native change: the new closure in mod.rs (|_: *mut c_void, _: &mut uws_sys::NewAppResponse<SSL>| server_body::on_timeout_for_idle_warn()) is the same non-capturing shape already compiled at server_body.rs:370, and should_add_timeout_handler_for_warning() continues to require DEBUG, !silent, and !has_idle_timeout before registering it, so the handler losing its own DEBUG check does not widen when the warning fires. The swap(true, Relaxed) ordering is a single-flag latch with no data it publishes; Relaxed is fine. I grepped the tree for the old message text and found no remaining assertions to update. No CODEOWNERS entry covers the changed paths.
Other factors
My earlier inline comment about the HTTP/2 leg being unable to fail was addressed by a real commit (headers x-proto: h1/h2 are recorded in the handler and echoed on stdout, asserted with toBe). The remaining item is a judgment call, not a defect: the warning is only registered when idleTimeout is absent, so the test cannot be made faster than the hard-coded 10 s default and needs the 30 s per-test ceiling. test/CLAUDE.md says not to set timeouts and the root guidance allows it only for a rare outlier, so a maintainer should decide whether this belongs in the regression lane as written or be restructured. I could not run the test here because no debug build is present and building is not appropriate within a review run, so I am relying on the PR's claim that it fails on main and passes with the fix.
Problem
Bun.serveprints the idle timeout warning (warn: Bun.serve() timed out a request after 10 seconds. PassidleTimeoutto configure.) two times when one HTTP/1 request and one HTTP/2 or HTTP/3 request time out in the same process. The second copy reads[Bun.serve]: request timed out after 10 seconds.NewServer::on_timeout_for_idle_warn,src/runtime/server/mod.rs:661) and the HTTP/2 and HTTP/3 path (on_timeout_for_idle_warn,src/runtime/server/server_body.rs:1218) each had their own once-only flag and their own text.should_add_timeout_handler_for_warningread only the HTTP/1 flag.Fix
mod.rs. The HTTP/1 request path now registers the shared handler fromserver_body.rs, the same one theRespLike::on_timeout_warnclosures use for HTTP/2 and HTTP/3.should_add_timeout_handler_for_warningreads the shared flag. After the warning prints, no later request on any protocol registers the handler again.warn:text, the text the HTTP/1 path printed before, and flushes stderr. This matches the text that the regression test in Output: flush stderr after a warning #43004 expects.test/regression/issue/43016.test.ts(fails on main with two warnings, passes with the fix). Alsotest/js/bun/http/serve-http2.test.tsand the idle timeout tests inserve.test.ts.Background
idleTimeoutclosed a request. It is meant to print at most once per process, for all servers and all protocols.on_timeoutcallback of a response when the socket idle timer fires. The server registers the warning callback only while the flag is false and the user did not passidleTimeout.Fixes #43016
[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The Rust port of
Bun.servekept two copies of the dev-mode idle timeout warning handler, one for HTTP/1 inNewServerand one for HTTP/2 and HTTP/3 inserver_body, each with its own once-only flag and its own message text, so a process that timed out a request on each path printed the warning twice and the registration check only consulted the HTTP/1 flag. The fix removes the HTTP/1 copy and routes all three protocols through the singleserver_body::on_timeout_for_idle_warnhandler and its shared process-globalAtomicBool, with the message emitted once viabun_core::warn!. A regr…