http2: give Http2Stream its own per-stream idle timer#30308
Conversation
|
Updated 11:05 PM PT - Jun 17th, 2026
❌ @robobun, your commit 84d6bb5 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30308That installs a local version of the PR into your bun-30308 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
find-issues-bot flagged #19049 as potentially fixed by this PR — it isn't. That issue is a JSC GC-phase segfault when the entire |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds per-session and per-stream idle timer management to HTTP/2: imports timer utilities, implements per-stream timer lifecycle and refresh points in data paths, adjusts session timeout emission to avoid broadcasting to streams, and adds regression tests covering stream vs session timeout behavior. ChangesHTTP/2 Idle Timer Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/node/http2/node-http2.test.js:2088-2095— These tests rely on hardcoded sleeps (setTimeout(r, 500)/setTimeout(r, 400)) and tight timer thresholds (300ms / 150ms), which test/CLAUDE.md prohibits ("never wait for time to pass in tests"). The second test in particular can deterministicallyawait once(client, 'timeout')instead of sleeping 400ms and then assertingsessionFired.v === true; for the first test, consider widening the 300ms margin so a loaded ASAN runner can't legitimately fire the per-stream timer before the first data chunk refreshes it.Extended reasoning...
What this is
test/CLAUDE.md:21 states: "Do not write flaky tests. Unless explicitly asked, never wait for time to pass in tests. Always wait for the condition to be met instead." Both new tests in the
Http2Stream.setTimeout per-stream idle timerdescribe block use hardcodedawait new Promise(r => setTimeout(r, ...))sleeps paired with short timer thresholds, rather than awaiting a deterministic condition.How it can manifest
Test 2 (
session-level setTimeout does not cascade 'timeout' to tracked streams) armsclient.setTimeout(150), sleeps 400ms, then assertssessionFired.v === true. This is the avoidable case: the test is waiting for a positive event (the session'timeout') by sleeping past it. If the event loop stalls and the socket-idle timer hasn't fired within the 400ms window,sessionFired.vis stillfalseand the test fails spuriously.Test 1 (
does not fire on completed streams after a session-idle gap) armsreq.setTimeout(300, ...)on each request and later sleeps 500ms. The 500ms sleep itself is defensible — proving a negative ("the timer does not fire after the stream ends") inherently requires waiting past the armed duration. The tighter risk is the 300ms threshold: betweenreq.setTimeout(300, ...)and the firstpushToStreamcall that refreshes the timer via_unrefTimer(), a heavily-loaded ASAN CI runner could plausibly stall >300ms even on a warmed loopback connection, causing the per-stream timer to fire legitimately during the request and producing a false-positivetimeoutFiresentry.Why existing code doesn't prevent it
The file defines
ASAN_MULTIPLIER = isASAN ? 3 : 1, but it's only applied to one Jest test timeout (line 1583) — the new 300ms / 150ms / 500ms / 400ms constants are not scaled by it. There is no event-based synchronization for the session-timeout assertion in test 2.Step-by-step example (test 2 on a slow runner)
client.setTimeout(150)arms the socket idle timer.await new Promise(r => setTimeout(r, 400))is scheduled.- Under heavy load, the JS timer queue is delayed; the 400ms sleep resolves at, say, T+410ms wall-clock, but the socket's idle-timeout callback (which goes through libuv →
#onTimeout→emit('timeout')) hasn't run yet because the loop was blocked. expect(sessionFired.v).toBe(true)fails even though the implementation is correct.
The deterministic alternative —
await new Promise(r => client.once('timeout', r))followed byexpect(streamFired).toEqual([])— has no such race and also makes thesessionFiredflag unnecessary.Impact
Potential CI flakiness, particularly under ASAN or on contended runners. Not a production-code bug.
Suggested fix
- Test 2: replace the 400ms sleep +
sessionFired.vflag withawait new Promise(r => client.once('timeout', r)), then assertstreamFiredis empty. This removes the race entirely. - Test 1: the sleep is structurally necessary for a negative assertion, but widen the per-stream threshold (e.g.
req.setTimeout(1000, ...)with a ~1500ms idle) or scale byASAN_MULTIPLIERso a transient stall before the first data chunk can't trip it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/30307.test.ts`:
- Line 75: Replace the fixed 500ms sleeps in the test (the `await new Promise(r
=> setTimeout(r, 500));` occurrences) with awaiting the actual session timeout
event: locate the two occurrences in test/regression/issue/30307.test.ts and
change them to await the session/process/page timeout event (e.g. use
events.once(sessionOrPage, 'timeout') or sessionOrPage.waitForEvent('timeout')
depending on the test harness) so the test waits for the 'timeout' event itself
rather than sleeping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6365787a-6714-4aa1-b33a-78b8bf0f0048
📒 Files selected for processing (1)
test/regression/issue/30307.test.ts
|
CI flake summary across the three retriggers — all hit pre-existing Windows/ASAN flakes on code paths this PR does not touch:
Windows test-bun is at ~5% job failure rate across the last 40 builds; this PR has hit it twice on the same test. The actual test-proof for this PR is reproducible locally and clean: All 58 tests in |
|
Another unrelated Windows flake on retrigger #52033 (046d9e4):
Tally of flakes on this PR so far (5 builds, 0 signals related to http2.ts):
|
|
Build #52040 hit two more Windows flakes (different jobs, same build):
6 consecutive CI failures on this PR, zero related to
Not planning further retriggers — will leave it for maintainer review. The actual PR content verifies cleanly locally: Test-proof: |
|
Pleasae help to review! |
robobun
force-pushed
the
farm/6e45c4d0/http2-per-stream-timer
branch
from
9bc7582 to
bfbdb61
Compare
|
Rebased onto The bug is still present after the rewrite, just reshaped:
Re-applied on the new code:
Verification on the rebased branch: The rest of the |
robobun
force-pushed
the
farm/6e45c4d0/http2-per-stream-timer
branch
from
bfbdb61 to
9eb928d
Compare
robobun
force-pushed
the
farm/6e45c4d0/http2-per-stream-timer
branch
from
9eb928d to
936851e
Compare
robobun
force-pushed
the
farm/6e45c4d0/http2-per-stream-timer
branch
from
936851e to
f86ec37
Compare
req.setTimeout(ms, cb) on an Http2Stream delegated to
session.setTimeout, which registers the callback on the session and
arms the underlying socket's idle timer. Two problems fell out of that:
1. Each per-stream setTimeout call added its callback as a
once('timeout') listener on the shared session. The callbacks
accumulated across requests, so a single socket idle timeout fired
every still-pending per-stream callback at once, even for streams
that had already ended.
2. Both ServerHttp2Session and ClientHttp2Session #onTimeout handlers
broadcast 'timeout' to every stream still tracked by the parser via
forEachStream(emitTimeout), contrary to Node.js semantics where a
session-level idle timeout emits on the session only.
@fastify/reply-from attaches a per-Http2Stream requestTimeout, so on
Bun the warn path fired on every completed/live stream after an idle
gap >= 5s, producing FST_REPLY_FROM_HTTP2_REQUEST_TIMEOUT bursts in the
proxy log. The proxied response was still 200 OK because the retry
path succeeded, but the log flooded.
Give Http2Stream a real per-stream timer keyed off kTimeout from
internal/timers:
- setTimeout(ms, cb) installs a per-instance setTimeout.unref() that
fires _onTimeout -> emit('timeout'), mirroring Socket.prototype
.setTimeout (sets this.timeout, validates, returns this).
- _unrefTimer() refreshes it; called on stream read (pushToStream),
stream write (_write/_writev), and HEADERS receipt (streamHeaders) so
an active stream never times out.
- _destroy clears the timer so a stream that has already closed never
fires its callback.
Drop the parser.forEachStream(emitTimeout) cascade from both #onTimeout
handlers: session-level socket idle timeouts now emit 'timeout' on the
session only, matching Node.
Fixes #30307
robobun
force-pushed
the
farm/6e45c4d0/http2-per-stream-timer
branch
from
f86ec37 to
e518b49
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and I found nothing new this round — but this rewires Http2Stream timer semantics across ~10 lifecycle touch points in node:http2 (freshly rebased onto the #31584 rewrite), so it's worth a maintainer sign-off.
Extended reasoning...
Overview
This PR replaces Http2Stream.prototype.setTimeout's delegation to the session/socket timer with a real per-stream idle timer (kTimeout from internal/timers), adds _onTimeout/_unrefTimer, wires refresh points into pushToStream, _write/_writev, both streamHeaders handlers, respond(), pushStream(), and additionalHeaders(), disarms the timer in markStreamClosed() (covering close(), _destroy, and the deferred-destroy END_STREAM path), and drops the parser.forEachStream(emitTimeout) cascade from both session #onTimeout handlers. Three regression tests cover the accumulated-callback bug, the session→stream cascade, and the deferred-destroy buffered-body path.
Security risks
None identified. This is timer-lifecycle bookkeeping in a Node-compat module; no auth, crypto, parsing, or untrusted-input handling is touched. The new timer is .unref()'d so it cannot pin the event loop.
Level of scrutiny
Moderate-to-high. src/js/node/http2.ts is production-critical Node-compat code, and this is a behavioral change (not a mechanical fix) to per-stream timeout semantics that real frameworks (@fastify/reply-from) depend on. The change is well-scoped and now mirrors Node's setStreamTimeout/kUpdateTimer/closeStream placement closely, but the file was just rewritten in #31584 and this PR was rebased onto that — so the surrounding code is fresh and the interaction surface is non-trivial.
Other factors
This PR has been through five rounds of inline review on my end; every issue raised (timer not cleared in close(), missing HEADERS refresh points inbound and outbound, markStreamClosed consolidation for the END_STREAM deferred-destroy path, additionalHeaders() sibling, class-body field declarations, this.timeout introspection, test conventions) has been addressed and the threads are resolved. The bug-hunting system found nothing new on the current revision. The author reports all 166 test-http2-*.js Node compat tests and the timeout-specific subset pass, and the regression tests fail on master / pass on this branch. CI failures over the PR's history were unrelated Windows/ASAN flakes. Given the scope and the recent rebase onto a large rewrite, a human maintainer should give the final OK.
…ENOENT in type-export.test.ts, unrelated)
There was a problem hiding this comment.
All prior review feedback has been addressed and I found nothing further — the timer lifecycle now covers every close transition and every HEADERS/DATA activity point I could identify. That said, this reworks Http2Stream timeout semantics across ~10 call sites in a core node:http2 path, so it's worth a maintainer's final look rather than a bot sign-off.
Extended reasoning...
Overview
This PR replaces Http2Stream.setTimeout's delegation to the session/socket timer with a real per-stream idle timer (kTimeout), adds _unrefTimer() refresh points at every inbound/outbound HEADERS and DATA site (pushToStream, _write/_writev, both streamHeaders handlers, respond(), pushStream(), additionalHeaders()), disarms the timer in markStreamClosed() so every close transition (explicit RST, natural END_STREAM with deferred destroy, _destroy) clears it, and removes the session→stream forEachStream(emitTimeout) cascade from both #onTimeout handlers. Three regression tests in test/regression/issue/30307.test.ts cover the accumulated-callback bug, the session-cascade bug, and the deferred-destroy END_STREAM path.
Security risks
None identified. This is timer lifecycle management in a Node.js-compat module; no auth, crypto, permissions, or untrusted-input parsing is touched. The change strictly narrows when 'timeout' fires (from session-wide broadcast to per-stream idle), which if anything reduces the surface for unexpected callback invocation.
Level of scrutiny
Moderate-high. src/js/node/http2.ts is a production-critical Node.js compat surface used by real frameworks (@fastify/http-proxy is the reporter). The change is not mechanical: it required five review rounds to converge on correct timer-clear placement (close() → markStreamClosed()), complete refresh-point coverage (inbound HEADERS, then outbound HEADERS, then additionalHeaders()), and class-body field declarations for shape monomorphism. Each round surfaced a real edge case. The final diff looks correct and closely mirrors Node's setStreamTimeout/kUpdateTimer/closeStream placement, but the iteration history itself is evidence that this area is subtle enough to warrant a maintainer's sign-off rather than auto-approval.
Other factors
- All 11 of my prior inline comments are resolved and reflected in the current diff.
- The bug-hunting system found nothing on this revision.
- Author reports 166 Node
test-http2-*.jscompat tests and the fullnode-http2.test.jssuite pass; the regression test fails on master and passes on the branch. - CI failures across the PR's history were documented as unrelated Windows/ASAN flakes in other subsystems.
- No CODEOWNERS entry covers
src/js/node/http2.ts. - A community member explicitly requested maintainer review on 2026-05-18; the PR was rebased onto the post-#31584
node:http2rewrite on 2026-06-18 and re-verified.
|
Diff is green; the two CI failures since the rebase are unrelated darwin-agent flakes, each hitting a different test:
None of these touch I've used my one CI re-roll (84d6bb5) and won't keep pushing All 12 review threads are resolved. This needs a maintainer to merge (or re-run the darwin lanes); the remaining red is unrelated darwin flake. |
2 participants
Fixes #30307
Problem
@fastify/http-proxywith HTTP/2 reportsFST_REPLY_FROM_HTTP2_REQUEST_TIMEOUTwarnings on every idle gap ≥ 5 s, even though the proxied response is200 OK.@fastify/reply-fromattaches areq.setTimeout(requestTimeout, cb)on every outgoingHttp2Stream. On Bun that path was broken two ways.Cause
Http2Stream.prototype.setTimeoutdelegated tosession.setTimeout, which delegated to the underlying socket'ssetTimeout. Consequences:req.setTimeout(ms, cb)registeredcbas aonce('timeout')listener on the shared socket. The callbacks accumulated across requests — a single socket idle fire ran every still-pending per-stream callback at once, including callbacks for streams that had already ended. That's the 4-at-once burst the reporter saw after the first 5 s gap.ServerHttp2Session.#onTimeoutandClientHttp2Session.#onTimeoutadditionally calledparser.forEachStream(emitTimeout), broadcasting'timeout'to every stream the parser still tracked. Per Node.js, a session-level idle timeout emits'timeout'on the session only — streams manage their own timers.Fix
Give
Http2Streama real per-stream idle timer keyed onkTimeoutfrominternal/timers:setTimeout(ms, cb)installs a per-instancesetTimeout(...).unref()that fires_onTimeout→emit('timeout'). MirrorsSocket.prototype.setTimeoutvalidation (getTimerDuration,validateFunction,returns this)._unrefTimer()refreshes it; called on stream read (pushToStream) and stream write (_write,_writev) so an active stream never times out._destroyclears the timer so a stream that already closed never fires.Drop the
parser.forEachStream(emitTimeout)cascade from both session#onTimeouthandlers — session idle timeouts now emit on the session only, matching Node.Verification
Repro from the issue on the debug build — silent, all 200 OK:
All 166 Node
test-http2-*.jsparallel tests pass; the 5 timeout-specific ones (test-http2-timeouts,test-http2-session-timeout,test-http2-compat-server{request,response}-settimeout,test-http2-server-close-idle-connection) stay green.Two new tests in
test/js/node/http2/node-http2.test.js:does not fire on completed streams after a session-idle gap— exact shape of the bug report (fails on master withreq-1..req-4firing, passes on this branch)session-level setTimeout does not cascade 'timeout' to tracked streams