node:http: measure the keep-alive idle period on the monotonic clock - #37974
node:http: measure the keep-alive idle period on the monotonic clock#37974robobun wants to merge 5 commits into
Conversation
The server records when a connection's last response finished and, when the socket timer fires, grants whatever is left of the keep-alive budget from that mark. Both readings came from Date.now(), which bun:test's setSystemTime() (and fake timers) override inside the engine: a clock moved back re-armed the idle connection for the budget plus the jump, a pinned clock doubled the budget, and a clock moved ahead closed the connection as soon as the timer fired. Add internal/timers.monotonicNowMs(), a Rust binding returning the real monotonic clock in ms, and take both readings with it.
|
Status: fix pushed (fb9cfb2); CI is green apart from one lane that fails on main as well.
|
WalkthroughThe change adds a monotonic millisecond timer binding. HTTP keep-alive timing uses this binding instead of ChangesMonotonic keep-alive timing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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/js/node/http/node-http-server-timeouts.test.ts`:
- Around line 258-260: Update the socket handling around the
Promise.withResolvers call so the error listener rejects closed with the emitted
socket error instead of swallowing it. Keep the close listener resolving on
normal closure, ensuring every socket failure event causes the probe to fail.
🪄 Autofix
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: a4c4258f-7a03-4c9d-9e3a-cc96bca2305d
📒 Files selected for processing (4)
src/js/internal/timers.tssrc/js/node/_http_server.tssrc/runtime/timer/Timer.rstest/js/node/http/node-http-server-timeouts.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a two-site clock swap with a matching Rust binding, following the existing timerClockMs pattern.
What was reviewed
- Both keep-alive idle readings (write at
onResponseFinishHandleSocket, read atonSocketTimeoutTimerExpired) now use the samemonotonicNowMs()clock, so the delta stays well-defined;ForceRealTimematches the heap the socket timer drains against. - Confirmed the removed
DateNowlocal has no remaining callers in_http_server.ts, andkeepAliveTimeoutBuffer: 0is a validcreateServeroption. - Tests: lower-bound-only assertions (no upper bound to flake under ASAN),
setSystemTime()reset infinally, kept serial because the mock is process-wide — the forward-skew bound (≥350ms) cleanly separates the unfixed ~200ms close.
Extended reasoning...
Overview
Replaces Date.now() with a new monotonicNowMs() binding at the two sites in src/js/node/_http_server.ts that measure the keep-alive idle interval (kKeepAliveIdleStart write on response finish, and the remaining computation when the socket timer fires). Adds the binding in src/runtime/timer/Timer.rs (Timespec::now(ForceRealTime).ms()) alongside the existing timerClockMs, exports it from src/js/internal/timers.ts, and removes the now-dead DateNow capture. Two regression tests added to test/js/node/http/node-http-server-timeouts.test.ts.
Security risks
None. This changes only which clock a subtraction reads; no new inputs, no parsing, no auth/crypto surface.
Level of scrutiny
Low-to-medium. The runtime change is a one-for-one substitution of the clock source at two call sites that already agreed with each other; both now read monotonicNowMs(), so the delta is still coherent. The Rust binding is a four-line clone of the neighbouring timer_clock_ms with ForceRealTime instead of AllowMockedTime, wired via the same $newRustFunction("runtime/timer/Timer.rs", "internal_bindings.*", 0) pattern already used for timerClockMs in internal-for-testing.ts. The PR description explains why ForceRealTime is the right mode (it is the clock the real timer heap is drained against, and setSystemTime() leaves the socket timer on that heap), and that this matches Node's libuv-loop-time behaviour.
Other factors
- Verified via grep that
DateNowhas no remaining references in_http_server.tsafter removal. - The new tests assert lower bounds only (
>= KEEP_ALIVE_MS - 150), so debug/ASAN slowness cannot cause false failures; the forward-skew case discriminates cleanly (unfixed ~140-200ms vs bound 350ms) and the backward-skew case times out unfixed.setSystemTime()is reset infinally, and the tests are deliberately serial with a comment explaining the process-wide state. - The PR notes the
Timer.rs/internal/timers.tshunks are byte-identical to #37960, so merge order is irrelevant. - Author reports the full keep-alive/timeout ported-test suite (17 files) plus the rest of this file pass on the debug build, and clippy/oxlint are clean.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
An idle keep-alive close is a clean FIN, so a reset or any other socket error now fails the probe instead of being timed as a close. Also fix the SLOW_RESPONSE_MS comment: the timer fires KEEP_ALIVE_MS minus SLOW_RESPONSE_MS into the second idle period, not SLOW_RESPONSE_MS.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 6:58 AM PT - Aug 13th, 2026
❌ @robobun, your commit fb9cfb2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37974That installs a local version of the PR into your bun-37974 --bun |
|
Not a duplicate of #37960: that PR fixes the SQL connect-retry budget, this one fixes node:http's keep-alive idle expiry; they share only the new |
There was a problem hiding this comment.
LGTM — a two-site Date.now() → monotonicNowMs() swap for the keep-alive idle mark, plus the trivial Rust binding it calls.
What was reviewed:
- Both readings of
kKeepAliveIdleStart(write at response-finish, read at timer-fire) now use the same monotonic clock; the removedDateNowcapture has no remaining uses in_http_server.ts. monotonic_now_msmirrors the siblingtimer_clock_msbinding, differing only inForceRealTimevsAllowMockedTime;$newRustFunctionis the established codegen path.- New tests reset
setSystemTime()infinally, reject on socket error, and stay serial because earlier tests in the file time themselves withDate.now().
Extended reasoning...
Overview
Four files. src/runtime/timer/Timer.rs adds a 6-line #[bun_jsc::host_fn] returning Timespec::now(ForceRealTime).ms(), placed directly beside the existing timer_clock_ms binding it copies. src/js/internal/timers.ts exports it via $newRustFunction. src/js/node/_http_server.ts replaces DateNow() with monotonicNowMs() at the only two sites that touch kKeepAliveIdleStart and drops the now-dead const DateNow = Date.now capture (grep confirms no other uses). Two regression tests are added to the existing keep-alive timeouts file.
Security risks
None. No user input reaches the new binding (it takes no arguments), and the _http_server.ts change is a clock-source swap with no control-flow change. Nothing auth-, crypto-, or permissions-related.
Level of scrutiny
Low-to-medium. The production change is mechanical: the two timestamps are only ever subtracted from one another, so as long as both come from the same monotonic source the arithmetic is unchanged. ForceRealTime is the correct choice — verified in bun_core/util.rs that it bypasses the mocked-time hook while AllowMockedTime consults it — and matches the clock the real timer heap is drained against, so the remaining computation now agrees with the timer that fires it. The new Rust host fn follows the exact shape of its neighbor.
Other factors
The bug-hunting pass found nothing. CodeRabbit's one concern (test probe swallowing socket errors) was addressed in 733f24d and the thread is resolved. The comment-cop bot's flags were about doc-comment length, not code, and are resolved. The tests wire error to reject, restore setSystemTime() in finally before any assertion can leak state, and use lower-bound-only assertions (>= 350ms) that the PR description shows fail on the unfixed build (~140-200ms for the forward-skew case; test timeout for the backward-skew case). No CODEOWNERS cover these paths. The shared Timer.rs/internal/timers.ts hunks are noted as byte-identical to #37960 so merge order doesn't matter.
|
Heads-up from #37960 (the SQL side): after a self-review pass it now binds |
Both the mark and the clock read when the timer fires are whole milliseconds, so a timer armed after the mark always measures at least the full budget when it fires. Before, a mark taken just after arming could round the other way and re-arm the connection for one more millisecond; with fake timers enabled in the meantime that re-arm would land in the fake heap and the connection would never be closed. Also say once, on the internal/timers export, which clocks the mocks touch, instead of repeating it on the binding.
The pinned case carries an upper bound: measured against a Date.now() that never advances, the unfixed server re-arms for the whole budget once more, so it closes no sooner than two budgets on any machine. The fake-timers case is what pins the choice of clock: a mark taken on the mocked monotonic clock (which starts over at zero) strands the connection just like one taken on Date.now() does. The idle time is now measured from just before each res.end() and read after the mocks are undone, so it can only over-report the server's own measurement, and every probe resets the mocks first so one that times out cannot leak them into the next.
|
Pushed eaa9a9f and fb9cfb2 after a review pass over the first revision:
|
There was a problem hiding this comment.
LGTM — swaps the two Date.now() reads in the keep-alive idle bookkeeping for a real monotonic clock, which is the textbook fix for interval measurement.
What was reviewed:
monotonic_now_msmirrors the adjacenttimer_clock_msbinding exactly, differing only inForceRealTimevsAllowMockedTime.DateNowhad no other callers in_http_server.ts;internal/timersis already loaded vianode:net, so the new require is a cache hit.- Moving the
kKeepAliveIdleStartmark before the timer arm is sound (avoids a 1ms rounding re-arm) and does not change the leave-timer-armed fast path. - Tests: probe rejects on socket error, mocks reset at both entry and
finally, and the< 2×budgetbound is safe because the unfixed floor is exactly two timer fires.
The three open comment-cop pings are the same doc-comment lines the author already justified in the earlier round (and the Timer.rs/internal/timers.ts ones are kept byte-identical to #37960's Rust hunk on purpose).
Extended reasoning...
Overview
The PR fixes a bug where node:http's server-side keep-alive idle expiry was measured with Date.now(), so bun:test's setSystemTime() / useFakeTimers() decided when (or whether) idle connections closed. The fix adds a monotonicNowMs binding (Timespec::now(ForceRealTime).ms()) in src/runtime/timer/Timer.rs, exports it from src/js/internal/timers.ts, and uses it at the two sites in src/js/node/_http_server.ts that record and read the idle-start mark. The DateNow = Date.now capture is deleted (no other callers). Four new tests in test/js/node/http/node-http-server-timeouts.test.ts cover clock-forward, clock-pinned, clock-back, and fake-timers-after-response.
Security risks
None. No user input is parsed; the change reads a monotonic clock instead of the wall clock in an internal timer-bookkeeping path. No auth, crypto, path, or network-parsing surface is touched.
Level of scrutiny
Moderate. The keep-alive path in _http_server.ts is hot (the whole leave-the-timer-armed design from #33879 exists to keep it cheap), but the runtime change is two clock-read call sites plus moving one assignment above the timer arm. The Rust binding is 8 lines that mirror the existing timer_clock_ms immediately above it, differing only in ForceRealTime. $newRustFunction codegen resolves internal_bindings.monotonicNowMs → monotonic_now_ms the same way it does for timerClockMs, so a mismatch would be a build error. The per-response cost stays one native clock read, and internal/timers is already loaded transitively via node:net, so the top-level require adds no module-load work.
The mark reorder (take kKeepAliveIdleStart before arming the timer) is a small, well-explained refinement: both readings are whole milliseconds, so a timer armed after the mark provably measures ≥ total on fire and closes instead of re-arming for a rounding millisecond. In the leave-armed fast path no timer is armed at all, so ordering is irrelevant there.
Other factors
- Tests await the socket
closeevent (no sleeps), reject onerror(fixed after CodeRabbit's note in 733f24d), reset mocks at probe entry and infinallyso a timed-out probe cannot poison later tests, and are kept serial because the earlier tests in the file time themselves withDate.now(). The one upper-bound assertion (< 2×KEEP_ALIVE_MSfor the pinned-clock case) is justified: the unfixed build always takes at least two full timer intervals to close, independent of machine speed, so the fixed build only has to beat that floor. - Cross-PR: #37960 now binds
monotonicNowMsdirectly insql/shared.tsand no longer editsinternal/timers.ts, but itsTimer.rshunk is still identical to this one, so either merge order stays clean. Theinternal/timersexport here is appropriate for_http_server.tssince that module is already in thenode:netload graph. - The three unresolved
github-actionscomment-cop pings are the automated length heuristic re-firing on the latest push; the author already responded to the identical earlier round explaining these are doc comments on a new export (not workaround justifications) and are kept verbatim to stay merge-clean with #37960. I agree — themonotonicNowMsJSDoc names exactly which clocks it must not be confused with, which is the non-obvious part a future reader would otherwise undo.
1 participant
Problem
node:httpserver's keep-alive idle expiry followsDate.now(), so bun:test'ssetSystemTime()anduseFakeTimers()(which overridesDate.nowas well) decide when, or whether, an idle kept-alive connection is closed. Reproduced on 1.4.0 withkeepAliveTimeout = 500,keepAliveTimeoutBuffer = 0: clock moved back an hour after the response, the connection is still open when the test times out; clock pinned before the request (the usualsetSystemTime(new Date("2020-01-01"))), it closes after 1000ms instead of 500; clock moved ahead an hour after a late second response, it closes ~200ms after that response instead of 500;useFakeTimers()called after the response, it is never closed.src/js/node/_http_server.ts.onResponseFinishHandleSocketleaves the socket timer armed across responses and only recordssocket[kKeepAliveIdleStart] = DateNow(); when the timer fires,onSocketTimeoutTimerExpiredcomputesremaining = socket.timeout - (DateNow() - idleStart)and re-arms forremainingwhen it is positive. Clock moved back:remainingis the budget plus the jump. Pinned: the full budget once more. Moved ahead: negative, so a fire that predates the idle period closes the connection. Fake timers: the re-arm'ssetTimeoutlands in the fake heap, where nothing fires it.Date.nowat module load does not help:setSystemTime()setsJSGlobalObject::overridenDateNow, whichDate.now()consults in every tier.useFakeTimers()also freezesperformance.now(),process.hrtime()andBun.nanoseconds(), so builtin JS had no clock to measure this with.Fix
src/runtime/timer/Timer.rs:internal_bindings.monotonicNowMs(), returningTimespec::now(ForceRealTime).ms(), next to the existingtimerClockMsbinding (which reads the mockable clock on purpose, for ported Node timer tests).src/js/internal/timers.tsexports it asmonotonicNowMs, with the rationale on the export._http_server.tstakes the idle mark and the fire-time reading with it, and takes the mark before arming the timer. Both readings are whole milliseconds, so a timer armed after the mark always measures at least the full budget when it fires; before, a mark taken just after arming could round the other way and re-arm for one more millisecond (harmless on real timers, a stranded connection if fake timers were enabled in between). Per-response cost is still one native clock read.ForceRealTimerather than the timer subsystem'sAllowMockedTimeclock:keepAliveTimeoutis a real-time budget, and the socket timer it is reconciled against is a real timer whenever it was armed before any mocking started, which is the case for every connection a test suite's earlier tests leave behind.ForceRealTimeis the clock that real heap is drained against and nothing in bun:test can move it.AllowMockedTimewould read as the fake clock (which starts at zero whenuseFakeTimers()is called) against a mark taken on the real one, and re-arm the connection for roughly the machine's uptime; the fourth test below fails with exactly that variant (verified by building it). The same property covers production: the wall clock can be stepped by NTP, the monotonic clock cannot. It also matches Node, whose keep-alive expiry runs on libuv's loop time, which neitherDatemocking nor jest's fake timers touch.useFakeTimers()enabled before the response, the socket timer itself is a plainsetTimeoutand still lands in the fake heap (builtin JS has no real-time timer yet; bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 did this for native timers). With a real-clock mark, a fake fire that comes before the budget has really elapsed re-arms once for the remainder, so such a connection may take two advances instead of one; nothing in the tree depends on either.sql/shared.tsdirectly, so theinternal/timersexport is only added here. The binding code is identical in both; only its doc comment differs (each points at its own consumer), so the conflict for whichever lands second is that comment and nothing else. This branch merges cleanly into current main.test/js/node/http/node-http-server-timeouts.test.ts, each measuring the idle time from just before the server's ownres.end()to the client'sclose, so the number can only over-report the server's measurement. Clock moved ahead after a second response answered 300ms late: unfixed ~140-200ms against a>= 350bound, fixed ~500. Clock pinned before the request: unfixed 1001-1004ms against a< 1000bound (the unfixed server re-arms for the full budget, so two budgets is a floor independent of machine speed), fixed ~500-530, and the>= 350bound as well. Clock moved back, and fake timers enabled after the response: unfixed never closes (test timeout), fixed ~500. Each probe resets the mocks before it starts and after it ends, so a timed-out probe cannot leak into the next one. All four fail on 1.4.0 individually and together; the file passes repeatedly on the debug build.node-http.test.ts,node-http-res-settimeout-unref.test.ts, and the portedtest-http-keep-alive-timeout*,test-http-server-keep-alive-*,test-http-server-*-timeout-*,test-http-set-timeout*.jssuites (17 files).cargo clippy -p bun_runtime,rustfmt,oxlintand prettier are clean.Background
idleStart + budgetre-arms once for the remainder; the next fire closes the connection.setSystemTime(d)pinsDate.now()atd(it does not advance) until reset; it touches no other clock.useFakeTimers()pinsDate.now(),performance.now(),process.hrtime()andBun.nanoseconds(), installs a mocked monotonic clock starting at zero that onlyjest.advanceTimersByTime()and friends move, and diverts newsetTimeouts into a heap that only those calls drain; timers armed before the call stay in the real heap and keep firing.Timespec::now(mode)is bun's monotonic clock (CLOCK_MONOTONIC, QPC on Windows).AllowMockedTimereturns the mocked clock while fake timers are installed;ForceRealTimenever does. The event loop drains the real timer heap againstForceRealTime.$newRustFunction(file, symbol, argc)is how builtin JS calls into Rust: codegen emits a thunk per call site that calls the named function directly, so a misnamed Rust function is a compile error.Measured close times (ms after the last response)
Earlier revision of this PR
The first revision took the mark after arming the timer and shipped only the moved-ahead and moved-back cases. Review added the pinned and fake-timers cases, which is what surfaced the rounding re-arm and the
ForceRealTimeargument above.[review] gate passed · iteration 0 · 4 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 node:http server measured keep-alive idle time with Date.now(), both when recording the idle start after each response and when the timer fired to compute the remaining budget, so bun:test's setSystemTime() or fake timers could delay, advance, or double the idle expiry of a kept-alive socket. The fix adds a monotonicNowMs binding backed by the real-time timer clock, exported from internal/timers, and uses it at both sites so the idle measurement is immune to wall-clock mocking. The idle start is also recorded before the timer is armed, so a timer that fires measures at least the full bu…