Conversation
BasePooledConnection bounded connect retries with Date.now(), which bun:test's setSystemTime() pins (and useFakeTimers() freezes), so a pool pointed at a server that is not accepting connections redialed forever instead of failing after connectionTimeout, and a clock moved ahead ended the budget after one attempt. Add a monotonicNowMs() binding (Timespec::now(ForceRealTime)) exported from internal/timers and use it for connectStartedAt and the budget check.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 5:17 AM PT - Aug 13th, 2026
✅ @robobun, your commit b3995f231cddcfe0761a236c0d2044820445d2d9 passed in 🧪 To try this PR locally: bunx bun-pr 37960That installs a local version of the PR into your bun-37960 --bun |
|
Status: fix pushed (b3995f2), waiting on CI. Reproduced on the released 1.4.0 binary with CI: the earlier heads were green on every lane that ran (179 of 181 on fc80581, 177 on 2d69e81 with the macOS aarch64 jobs queued behind a backlogged lane). b3995f2 is the revision to judge: it rebinds the clock directly in Review: automated reviews found nothing to change (the latest one covers b3995f2); the three comment-length threads are answered and resolved. #37974 shares the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused clock swap with a matching binding and tests for both directions.
Checked: the new monotonic_now_ms binding mirrors the existing timer_clock_ms shape and $newRustFunction call site; the two Date.now() sites in BasePooledConnection are the only ones on the retry-budget path and the 0 sentinel still holds. The added mysqlHandshakeV10/mysqlOkPacket/mysqlReadPackets imports exist in wire-frames.ts, and every new test resets setSystemTime() in finally. The top-level require("internal/timers") in sql/shared.ts is a leaf module (no cycle).
Extended reasoning...
Overview
Swaps the SQL pool's connect-retry budget from Date.now() to a new unmockable monotonic clock so bun:test's setSystemTime() cannot freeze or skip it. Touches four files: a 12-line Rust host function (monotonic_now_ms) alongside the existing timer_clock_ms in src/runtime/timer/Timer.rs; an export in src/js/internal/timers.ts; two call sites plus a comment in src/js/internal/sql/shared.ts; and four new tests plus a MySQL greet helper in test/js/sql/sql-connect-error-reporting.test.ts.
Security risks
None. The new binding is internal-only ($newRustFunction codegen, not user-visible), takes no arguments, and just reads Timespec::now(ForceRealTime).ms(). No user input reaches it. The test file's new MySQL helper reuses existing wire-frames.ts builders against a local net.Server.
Level of scrutiny
Low-to-moderate. The Rust side is a near-copy of the sibling timer_clock_ms binding with only the mock-mode flag changed — no allocation, no JS re-entry, no unsafe. The JS change is a two-call-site clock substitution that preserves the connectStartedAt === 0 sentinel and all backoff/budget arithmetic. internal/timers only requires internal/validators, so the new top-level require in sql/shared.ts introduces no cycle.
Other factors
Tests cover both failure modes (pinned clock → budget still elapses; forward jump → still retries) for both adapters that share BasePooledConnection, and each restores setSystemTime() in finally. The PR description documents that the pinned tests hang and the jump tests fail after one attempt on the unfixed build, and that the full 19-test file plus adjacent SQL/fake-timer suites pass on the debug build. No prior reviewer comments to address.
|
#37974 (the node:http keep-alive side of this) adds the same |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs — the Date.now() → monotonicNowMs() swap is correct and well-tested. Deferring to a human because it adds a new native binding and changes user-observable behavior (mocked time no longer affects the SQL connect-retry budget), and the comment-cop feedback on the doc comments is a style call for a maintainer.
What was reviewed:
monotonic_now_msmirrors the adjacenttimer_clock_msbinding withForceRealTime; the$newRustFunctionname/argc match the existing pattern.- The
connectStartedAt === 0sentinel remains safe — CLOCK_MONOTONIC/QPC won't return 0 in practice. - New test imports (
mysqlHandshakeV10,mysqlOkPacket,mysqlReadPackets) exist inwire-frames.ts;setSystemTime()is reset in everyfinally.
Extended reasoning...
Overview
Replaces Date.now() with a new unmockable monotonic clock (monotonicNowMs) for measuring the SQL connect-retry budget in BasePooledConnection. Four files: a ~10-line Rust host function in src/runtime/timer/Timer.rs (sibling to the existing timer_clock_ms, differing only in TimespecMockMode::ForceRealTime vs AllowMockedTime), a one-line $newRustFunction export in src/js/internal/timers.ts, two call-site swaps in src/js/internal/sql/shared.ts, and four new regression tests in test/js/sql/sql-connect-error-reporting.test.ts.
Security risks
None. The new binding reads a clock and returns a number; no user input is parsed, no allocations sized from external data, no auth/TLS/permission paths touched.
Level of scrutiny
Moderate. The mechanical change is small and follows an established pattern exactly (verified against the neighboring timerClockMs binding in internal-for-testing.ts). However, it (1) adds a new native→JS binding surface that #37974 already plans to reuse, (2) is a deliberate behavior change — setSystemTime() / advanceTimersByTime() no longer influence the SQL retry budget — which the PR description argues for convincingly but is still a maintainer-level decision, and (3) has outstanding comment-cop bot feedback on three doc comments. Those comments read to me as legitimate "why this clock and not Date.now()" documentation rather than workaround justification, but that's a repo style judgment.
Other factors
- CI showed a ❌ on fc80581 followed by a
ci: retriggercommit; current status is not confirmed green from the timeline. - Verified the
0sentinel forconnectStartedAtis still safe with the monotonic clock (starts at boot on all platforms, not at epoch). - Verified the newly-imported
mysqlHandshakeV10/mysqlOkPacket/mysqlReadPacketshelpers exist intest/js/sql/wire-frames.ts. - Tests correctly restore process-global state (
setSystemTime()with no args) infinallyblocks and cover both failure directions (pinned clock → never elapses; jumped clock → elapses too early) for both adapters sharingBasePooledConnection. require("internal/timers")fromsql/shared.tsintroduces no circular dependency (it only pulls ininternal/validators).
|
Confirmed from this side: as of 2d69e81 the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The clock swap itself is small and correct, but since it introduces a new internal binding (monotonicNowMs, also picked up by #37974) and fixes a semantics decision — the connect-retry budget now ignores mocked time — a human sign-off would still be worthwhile. Two of the three comment-cop flags (on the timers.ts and Timer.rs doc comments) are also still open; they read as API docs rather than workaround justifications, so likely false positives, but worth a look.
What was reviewed:
monotonic_now_msmirrors the adjacenttimer_clock_msbinding withForceRealTimeinstead ofAllowMockedTime; the$newRustFunctionsymbol name matches the existing camelCase→snake_case convention.- The
0sentinel onconnectStartedAtis unchanged and still safe with the monotonic clock; both call sites (#beginConnecting,#canKeepRetrying) now read the same clock. - New tests restore
setSystemTime()infinallyon every path; the importedmysqlHandshakeV10/mysqlOkPacket/mysqlReadPacketshelpers exist inwire-frames.ts.
Extended reasoning...
Overview
Swaps Date.now() for a new monotonicNowMs() binding in BasePooledConnection's connect-retry budget (src/js/internal/sql/shared.ts), so bun:test's setSystemTime() / useFakeTimers() can no longer freeze or prematurely exhaust it. Adds the Rust host function in src/runtime/timer/Timer.rs (a one-line variant of the existing timer_clock_ms using TimespecMockMode::ForceRealTime), exports it from src/js/internal/timers.ts, and adds four tests (postgres × mysql × pinned/jumped clock) to test/js/sql/sql-connect-error-reporting.test.ts.
Security risks
None. No user-controlled input reaches the new binding; it takes zero arguments and returns a number. No auth/crypto/permissions surface touched.
Level of scrutiny
Medium. The implementation is mechanical (two call sites swap Date.now() → monotonicNowMs(); the Rust binding is a near-copy of its neighbor), but this is a deliberate semantics decision: the connect-retry budget is now decoupled from mocked time. The PR argues convincingly that this aligns with the native connect timeout and the real-clock backoff timers, and #37974 will reuse the same binding for node:http — so the shape of this internal API is worth a human confirming once.
Other factors
- Two comment-cop flags remain on the doc comments in
timers.ts:7-12andTimer.rs:600-605(the third, onshared.ts, was addressed in 2d69e81). These look like false positives — they document why the API exists, not why a workaround is acceptable — but they're technically unresolved. - CI on
fc805815reported failures, followed by a retrigger commit; status on the latest head isn't visible in the timeline. - Tests cover both failure directions (budget never elapses under a pinned clock; budget ends early under a forward jump) for both adapters, and restore process-global
setSystemTime()state infinally. The0sentinel for "not connecting" is preserved and remains safe with a monotonic clock.
…ry-budget-monotonic-clock
…ding Requiring internal/timers from internal/sql/shared pulled validators, internal/shared and primordials into every Bun.SQL load for one native function; bind it where it is used instead, like the adapters do. Expose the binding through bun:internal-for-testing and assert that useFakeTimers() neither restarts nor advances it, which is the only thing separating it from the mockable timerClockMs next to it.
|
Update after a self-review pass (b3995f2): |
There was a problem hiding this comment.
LGTM — targeted swap of Date.now() for an unmockable monotonic clock in the SQL connect-retry budget.
What was reviewed:
- The new
monotonic_now_msbinding mirrors the siblingtimer_clock_msexactly, only swappingAllowMockedTime→ForceRealTime; the$newRustFunctionsymbol path matches the existing camelCase→snake_case convention. - Checked the
connectStartedAt === 0sentinel still works with a monotonic clock (CLOCK_MONOTONIC/QPC ms since boot; effectively never 0 in practice, and the previousDate.now()had the same theoretical edge). - The four new SQL tests reset
setSystemTime()infinally, cover both pinned and jumped-ahead directions for both adapters, and reuse the existingwire-frames.tshelpers; the fake-timers test asserts the mockable clocks do move whilemonotonicNowMs()does not.
Extended reasoning...
Overview
Replaces Date.now() with a new monotonicNowMs() Rust binding (Timespec::now(ForceRealTime).ms()) at the two sites in src/js/internal/sql/shared.ts that measure the connect-retry budget (#beginConnecting records the start, #canKeepRetrying compares against it). Adds the binding in src/runtime/timer/Timer.rs alongside the existing timer_clock_ms, exposes it for tests via internal-for-testing.ts, and adds one binding-level test in fake-timers.test.ts plus four end-to-end tests (postgres/mysql × pinned/jumped) in sql-connect-error-reporting.test.ts.
Security risks
None. The change reads a monotonic clock instead of wall-clock time for an internal deadline; no user input flows into the new binding, and the binding takes no arguments.
Level of scrutiny
Low-to-medium. The Rust hunk is a near-verbatim copy of the neighbouring timer_clock_ms with one enum variant flipped; the JS change is a two-line clock substitution that leaves the sentinel, backoff, and budget arithmetic unchanged. The behavioural change (mocked time no longer influences the retry budget) is intentional, well-argued in the description, and consistent with #37946's treatment of the native connection timeouts.
Other factors
- The
$newRustFunctionsymbol name follows the same camelCase→snake_case mapping as the existingtimerClockMs→timer_clock_ms, so codegen will resolve it. - The
0sentinel forconnectStartedAtis safe with a monotonic clock: the value only reads 0 within the first millisecond after boot, no worse than the previousDate.now()edge at the epoch. - Tests are hermetic (local TCP servers,
setSystemTime()reset infinally), cover both failure directions for both drivers, and the author verified they fail on the released binary and pass on the debug build. - All three comment-cop threads are resolved: the
shared.tsfield comment was shortened in 2d69e81, and the remaining doc comments on the new binding were justified and kept. No prior reviews from me on this PR.
|
Follow-up to the note above: review on #37974 pointed out two things in the shared doc comments, so they changed there and the hunks are no longer byte-identical (the code lines still are, so any conflict is comment-only). In case you want to line them up again, this is the text #37974 now carries:
/**
* The real monotonic clock in whole milliseconds, for deadlines the runtime's
* own JS keeps (a mark recorded now and compared against when a timer fires).
* No JS-visible clock will do: bun:test's `setSystemTime()` overrides
* `Date.now()` inside the engine, and `useFakeTimers()` also overrides
* `performance.now()` and `process.hrtime()`, so a deadline measured with any
* of them freezes or jumps along with the mock.
*/
/// `require("internal/timers").monotonicNowMs()`, documented there. Always
/// the real clock, i.e. the one the real timer heap is drained against,
/// whatever bun:test has mocked.
The two corrections behind it: |
|
Saw that this PR now binds the function from sql/shared.ts directly and no longer touches internal/timers.ts, so disregard the internal/timers.ts part of the comment above. The only remaining overlap with #37974 is the Timer.rs doc comment on the (otherwise identical) binding; whichever lands second keeps its own wording, no coordination needed. |
This branch has not been deployed
1 participant
Problem
connectionTimeoutelapses. That budget is measured withDate.now():connectStartedAt = Date.now()in#beginConnectingandDate.now() - this.connectStartedAt < connectionTimeoutin#canKeepRetrying(src/js/internal/sql/shared.ts).setSystemTime()pinsDate.now()at one value (anduseFakeTimers()freezes it), so the budget never elapses: the pool redials once a second for as long as the test runs andconnect()/ the waiting queries never reject. On 1.4.0,setSystemTime(new Date("2020-01-01"))plusconnectionTimeout: 0.25against an accept-and-destroynet.Serveris still pending after 4s with 8 to 12 connections dialed.setSystemTime()moved ahead during a connect cycle makes the next failure look like the whole budget is gone, so the pool gives up after a single attempt with the default 30s budget.Date.nowat module load does not help; the override lives in the engine (JSGlobalObject::overridenDateNow, consulted byDate.now()in every tier).performance.now(),process.hrtime()andBun.nanoseconds()are frozen byuseFakeTimers()too (Bun__readOriginTimer), so builtin JS had no unmockable clock to measure an internal deadline with.Fix
src/runtime/timer/Timer.rs: newinternal_bindings.monotonicNowMs()returningTimespec::now(ForceRealTime).ms(), next to the existingtimerClockMsbinding (which reads the mockable clock on purpose, for ported Node timer tests). node:http: measure the keep-alive idle period on the monotonic clock #37974 (thenode:httpkeep-alive side of the same problem) adds the identical hunk, so either PR can land second without a rebase.sql/shared.tsbinds it with$newRustFunctionwhere it is used (the waypostgres.ts/mysql.tsbind their natives) and records / comparesconnectStartedAtwith it. Sentinel (0= not connecting), backoff and budget are unchanged. An earlier revision routed this through arequire("internal/timers"), which would have pulledinternal/timers,validators,internal/sharedandprimordialsinto everyBun.SQLload for one function; the direct binding keeps the SQL module graph as it is.connectionTimeoutis a real-time budget (how long to wait for the server to come up), the same thing the native connect timeout measures, and the backoff timers that drive the retries run on the real clock undersetSystemTime().ForceRealTimeis the clock the event loop drains the real timer heap with, so the budget and the timers now agree. This is also what Node users get: Node's internals read theDateNowprimordial, which jest/sinon fake timers (they replace the globalDate) cannot reach; bun:test mocks inside the engine, so the internal clock has to be explicit.setSystemTime(),advanceTimersByTime()) no longer expires or extends the connect-retry budget; it elapses in real time, the same call bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 made for the native connection timeouts. Nothing in the tree relied on the old coupling (no test combinesBun.SQLwith mocked time, and the docs describe the retry window only in terms ofconnectionTimeout).setTimeoutinshared.tsis still a regular JS timer, so underuseFakeTimers()it lands in the fake heap (a real-time timer for builtin JS is a separate change), and_http_server.ts's keep-alive bookkeeping is node:http: measure the keep-alive idle period on the monotonic clock #37974.test/js/sql/sql-connect-error-reporting.test.ts(4 new, postgres and mysql each since both adapters shareBasePooledConnection): with the clock pinned,connect()still rejects withERR_*_CONNECTION_FAILEDonce the budget elapses; with the clock jumped a day ahead while the first failure is being reported, the pool still retries and connects on the second attempt. On the unfixed build the pinned tests hang until the test timeout and the jump tests reject after one attempt.test/js/bun/test/fake-timers/fake-timers.test.ts(1 new, through atimerInternals.monotonicNowMsexport inbun:internal-for-testing): underuseFakeTimers()+advanceTimersByTime(1h)the mockabletimerClockMs()andperformance.now()read exactly 1h whilemonotonicNowMs()neither restarted from 0 nor moved by the hour.setSystemTime()never touches the Rust mock clock, so this is the one test that pinsForceRealTime: with the binding switched toAllowMockedTimeit fails (restarted: true, advancedByTheHour: true), and on the released binary it fails because the export does not exist.sql-close-pending-connection,postgres-failed-connection-resurrection,sql-onconnect-onclose-throw,sql-mysql-duplicate-auth-switch,test/js/web/timers/setTimeout.test.js(thetimerClockMstest passes; its three leak fixtures fail locally on any debug+ASAN build because the fixture detects ASAN by binary name, which test: surface ASAN status to leak fixtures via bunEnv #35081 addresses),test-timers-ordering.js, and a real connect +select 1against a local postgres.Background
BasePooledConnection.handleClosekeeps the slot pending and redials after 40ms, 80ms, ... (capped at 1s) while queries are waiting, untilconnectionTimeout(default 30s) has passed sinceconnectStartedAt; bothhandleCloseand the backoff timer callback re-check the budget through#canKeepRetrying.setSystemTime(d)setsoverridenDateNowon the global object; from then onDate.now()returns exactlyduntil it is reset, it does not advance, and nothing on the Rust side changes.useFakeTimers()additionally installs the Rust mock clock (read byTimespec::now(AllowMockedTime),performance.now()andtimerClockMs(), all restarting from 0) and routessetTimeoutinto a heap that onlyjest.advanceTimersByTime()and friends drain.Timespec::now(mode)is bun's monotonic clock (CLOCK_MONOTONIC, QPC on Windows).AllowMockedTimereturns the mock clock while one is installed;ForceRealTimenever does.$newRustFunction(file, symbol, argc)is how builtin JS modules call into Rust: codegen emits one thunk per distinct symbol that calls the named function directly, so several modules may bind the same function, and a missing or misnamed Rust function is a compile error.bun:internal-for-testingis the test-only module for exposing such bindings to tests.