test: cover the c-ares poll timer under jest.useFakeTimers() - #37940
test: cover the c-ares poll timer under jest.useFakeTimers()#37940robobun wants to merge 1 commit into
Conversation
|
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 (4)
WalkthroughDNS resolver polling timers now use real-time behavior when fake timers are active. Tests cover cancellation, timer counts, real-clock timeouts, and child-process behavior. ChangesDNS resolver timer behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:05 AM PT - Aug 13th, 2026
✅ @robobun, your commit 81a19e17fba882443549d64261f2ff6f66007010 passed in 🧪 To try this PR locally: bunx bun-pr 37940That installs a local version of the PR into your bun-37940 --bun |
|
Status: re-scoped to tests only, ready for review (head 81a19e1, based on current main). #37946 merged while this was open and contains the two source lines this PR originally carried, so the source change is gone from here. What remains is the regression coverage for the DNS resolver, which #37946 did not add: Verification: both tests pass on main at b7a777e with no source changes (debug build), and both fail on the released 1.4.0 binary ( The earlier source-fix history (three bot reviews with no findings, 179 green CI jobs including x64 ASAN and darwin 14 on the last source head) is in the PR body's details block. #37935 (Date header timer) is the remaining sibling and still applies to |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a two-line source change following the existing DnsSdConnection/GcRepeating pattern for internal pacing timers, with hermetic tests covering both halves of the fix.
What was reviewed:
allow_fake_timers()opt-out addition matches the siblingDnsSdConnectionentry;All::insert/removeroute onin_heap, so removal from the real heap under fake timers is unaffected.ForceRealTimeinadd_timer: theSome(now)path fromcheck_timeoutsalready receives real time from the heap drain, so only theNonepath (fromrequest_sent/request_completed) changes.- Tests are hermetic (local
127.0.0.1:0UDP socket, no external DNS), drain child pipes concurrently, and the fixture's second case specifically covers theForceRealTimehalf by pushing the mocked clock past uptime.
Extended reasoning...
Overview
Two source lines changed: Tag::DNSResolver added to the allow_fake_timers() opt-out list in src/event_loop/EventLoopTimer.rs, and Resolver::add_timer in src/runtime/dns_jsc/dns.rs switched from AllowMockedTime to ForceRealTime when computing the poll timer's deadline. A new spawned fixture and two tests are added to test/js/bun/test/test-timers.test.ts.
Security risks
None. No user-facing API change, no untrusted input handling, no crypto/auth. The change only affects which intrusive heap an internal 1s bookkeeping timer is placed in and which clock it reads.
Level of scrutiny
Low-to-medium. The change follows an established pattern precisely — DnsSdConnection (also DNS-related internal pacing) is already on the same opt-out list, and the PR description correctly explains why a real-heap timer must read the real clock (the real heap is drained against CLOCK_MONOTONIC, so a mocked-clock deadline would be either always-due or never-due). I traced All::insert (routes on fake_timers.is_active() && tag.allow_fake_timers()) and All::remove (routes on the node's in_heap field, not the tag), so a DNSResolver timer inserted into the real heap while fake timers are active is still removed correctly by remove_timer. I also confirmed check_timeouts passes the heap-drain's real now into add_timer(Some(&now)), so the re-arm path was already correct — only the initial arm from request_sent/request_completed was reading the mocked clock.
Other factors
Test quality is good: hermetic (local dgram socket on 127.0.0.1:0, never answered), the in-process test asserts ECANCELLED to prove the query was still pending when getTimerCount() was sampled, the child fixture is bounded by a spawn timeout below the test timeout so a pre-fix spin fails cleanly, and the fixture's second case (advance mocked clock years ahead, expect ETIMEOUT on real time) specifically exercises the ForceRealTime change — the PR notes it fails with the tag change alone. Pipes are drained concurrently. No behavior is being removed that users could have depended on: the poll timer only exists to let c-ares notice its own real-clock timeouts, so fake-timer enrollment never did anything but the observed spin/hang.
|
One follow-up push coming for the tests only (src unchanged): the in-process test will wait for ETIMEOUT after useRealTimers() instead of cancelling the query. On the unfixed binary, useRealTimers() drops the poll timer along with the rest of the fake heap and a query started under fake timers stays pending forever, so that variant is worth covering too (verified: released binary hits the test timeout, this branch gets ETIMEOUT in about a second). The fixture is unchanged. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Heads up: #37946 turns |
There was a problem hiding this comment.
LGTM — mechanical opt-out following the established DnsSdConnection/GcRepeating pattern.
What was reviewed:
Tag::DNSResolveradded to theallow_fake_timers()opt-out list;add_timerswitched toForceRealTime— both mirrordns_sd.rsexactly (same comment wording).- Verified
check_timeoutsre-arm already passes the real-clocknowfrom the heap drain, so only theNonecallers (request_sent/request_completed) needed the clock-mode change. - Tests use a local non-answering UDP socket (no external network),
port: 0, drain pipes concurrently, assertsignalCode === null; the fixture's second case proves theForceRealTimechange is load-bearing independent of the tag change.
Extended reasoning...
Overview
Two source lines: (1) add Tag::DNSResolver to the allow_fake_timers() => false match arm in src/event_loop/EventLoopTimer.rs, alongside the existing DnsSdConnection, GcRepeating, and QuicEndpoint internal-pacing opt-outs; (2) switch Resolver::add_timer's fallback clock read from AllowMockedTime to ForceRealTime in src/runtime/dns_jsc/dns.rs. Plus a new fixture and a describe block appended to test-timers.test.ts.
The mechanism is well-explained and I verified it against the code: All::insert at src/runtime/timer/mod.rs:679 routes to the fake heap only when tag.allow_fake_timers(), so adding DNSResolver to the opt-out list keeps the poll timer in the real heap. The real heap drains against the real clock, so the deadline must be computed from the real clock too — hence ForceRealTime. The check_timeouts re-arm path (dns.rs:4073) already receives now from the real-heap drain and passes it as Some(&now), so only the None callers needed the change. This is the identical shape and identical one-line comment as dns_sd.rs:116-118 for DnsSdConnection.
Security risks
None. No user input touched; this is internal timer-heap routing for a bookkeeping poll that calls ares_process_fd(BAD, BAD).
Level of scrutiny
Low-to-medium. The source change is two lines following an existing sibling pattern verbatim. The blast radius is limited to how the c-ares poll timer behaves under jest.useFakeTimers() — outside fake timers, ForceRealTime and AllowMockedTime return the same value and the heap routing is identical, so there is no behavior change for normal DNS resolution.
Other factors
Test coverage is strong and follows repo conventions: local dgram socket on 127.0.0.1:0 that never answers (hermetic, no external network), bunExe()/bunEnv, await using proc, concurrent pipe drain, signalCode === null asserted before exitCode. The fixture mirrors the adjacent test-timers-gc-spin-fixture.ts (same advance-mocked-clock-past-uptime trick). The 30s test timeout is justified because the pre-fix behavior spins until the 20s spawn timeout, and the passing case waits ~1s of real time for c-ares to give up. The fixture's second test isolates the ForceRealTime change (would fail with only the tag change), so both source lines are proven load-bearing. The PR body reports both new tests fail on the unfixed build and released binary, and the related DNS/fake-timers suites still pass.
The comment-cop bot flag was addressed (comment trimmed to one line matching dns_sd.rs, thread resolved). No outstanding human review comments.
…37946) ### Problem - `jest.useFakeTimers()` captures the runtime's own safety timeouts. With fake timers active, `Bun.spawn({ timeout })` never kills the child, `new SQL({ connectionTimeout })` / `new RedisClient(url, { connectionTimeout })` never time out, and `jest.getTimerCount()` counts these timers as if the test had created them. - `jest.useRealTimers()` (and `jest.clearAllTimers()`) then drains the fake heap and disarms them permanently: a child spawned with `timeout: 200` under fake timers runs to completion after the test is back on real time, and a Postgres connection attempt stays pending forever. The owner still believes it is armed, so nothing re-arms it. - `Bun.spawnSync({ timeout })` under fake timers has the same problem without the heap: its wait loop compared the deadline against the mocked clock, which cannot advance while the call blocks. - Cause: `Tag::allow_fake_timers()` in `src/event_loop/EventLoopTimer.rs` was an opt-out list, so every owner not on it (`SubprocessTimeout`, the Postgres/MySQL/Valkey connection, idle, lifetime and reconnect timers, `UpgradedDuplex`, `WindowsNamedPipe`, `DNSResolver`, `DateHeaderTimer`, the dev server timers) was routed into the fake heap by `All::insert` (`src/runtime/timer/mod.rs`), and each of them computed its deadline with `AllowMockedTime` to match. The fake timers PR left this list as a TODO ("should subprocess timeout? probably not"). - `FakeTimers::clear()` (`src/runtime/test_runner/timers/FakeTimers.rs`) only told `TimeoutObject` and `AbortSignal.timeout()` owners that their timer was gone; every other popped node was just marked `CANCELLED`. ### Fix - `allow_fake_timers()` becomes an allowlist of the timers a program schedules itself: `setTimeout`/`setInterval` (`TimeoutObject`), `AbortSignal.timeout()`, and `Bun.cron()` (documented as honoring fake timers). Everything else stays in the real heap and keeps running on the real clock, which is also what Jest does (its fake timers never reach Node's internal timeouts). - Every owner that moves to the real heap now arms with `ForceRealTime` (subprocess, Postgres, MySQL, Valkey, `UpgradedDuplex`, `WindowsNamedPipe`, c-ares poll timer, Date header timer, dev server sweep / visualizer). This is required, not optional: the real heap is drained against the real clock (`All::next`), so a mocked deadline there would be due immediately. It is the same convention the existing opt-outs (`BunTest`, `GcRepeating`, `StatWatcherScheduler`, `QuicEndpoint`, `DnsSdConnection`) already follow. - `spawnSync` compares its `timeout` against the real clock too (`js_bun_spawn_bindings.rs`, `SpawnSyncEventLoop.rs`), and only consults an `AbortSignal.timeout()` deadline that lives in the real heap; one in the fake heap is on the mocked clock and cannot fire during a blocking call anyway. Behavior without fake timers is unchanged. - `Bun.cron` is now the only non-JS owner the fake heap can drop, so `FakeTimers::clear()` collects dropped jobs and stops them the way `stop()` would (`CronJob::stop_dropped_from_fake_heap`), matching what happens to a dropped `setInterval`. Before, a job created under fake timers and then `useRealTimers()` kept the process alive forever with a timer that could never fire. The fallback arm is now a `debug_assert!`, so adding a tag to the allowlist without a release path fails loudly in debug builds. - Behavior change to be aware of: `advanceTimersByTime()` no longer fires runtime-internal timeouts (e.g. a SQL `connectionTimeout`); they elapse in real time. No test or doc relied on that; the cron tests that do rely on mocking are unaffected. Not changed here: the async `node:child_process` `timeout` option is implemented with a JS `setTimeout` in `src/js/node/child_process.ts`, so it is still faked like any other `setTimeout`. - Verified with `test/js/bun/test/fake-timers/fake-timers.test.ts` (new `runtime timeouts are not fake timers` and `Bun.cron() job dropped from the fake heap` blocks, 9 tests, including `node:child_process.spawnSync({ timeout })`, which hands its timeout to `Bun.spawnSync`). All 9 fail on the unfixed build (`getTimerCount()` is 1 or 2, spawn/spawnSync children exit normally, the cron child hangs until killed) and pass with the fix. - Also passing on the debug build: `test/js/bun/test/fake-timers/sinonjs`, `test-timers.test.ts`, `test/js/bun/cron/in-process-cron.test.ts` and `cron-local-time.test.ts`, `spawn-signal.test.ts` (covers `spawnSync` + `AbortSignal.timeout`), `spawnSync.test.ts`, `spawn-maxbuf.test.ts`, `spawnsync-isolated-event-loop.test.ts`, `spawnsync-no-microtask-drain.test.ts`, `test/js/sql/sql-connect-error-reporting.test.ts`, the fake-timer cases of `test/cli/test/isolation.test.ts`, `test/regression/issue/25869.test.ts`, `26284.test.ts`. `cargo clippy` on `bun_event_loop`, `bun_runtime`, `bun_sql_jsc` is clean, including `--target x86_64-pc-windows-msvc` for the `WindowsNamedPipe` change. - Overlap: #37935 (Date header timer) and #37940 (c-ares poll timer) each add one of these owners to the old opt-out list and flip its clock; this PR contains both of those source changes as part of the allowlist. Their tests still apply on top of this and are worth keeping; whichever lands second needs a small rebase of `allow_fake_timers()`. ### Background - `timer::All` keeps two intrusive heaps of `EventLoopTimer` nodes. The event loop drains `timers` against `CLOCK_MONOTONIC`. While `jest.useFakeTimers()` is active, `All::insert` diverts nodes whose `Tag` allows it into `fake_timers.timers`, which only the `jest.*` functions drain: each pop sets the mocked clock to the node's deadline and fires it; `useRealTimers()` / `clearAllTimers()` pop everything without firing. - `Tag` says which struct embeds a node (a `Subprocess`, a `PostgresSQLConnection`, a `TimeoutObject`, ...); `__bun_fire_timer` in `src/runtime/dispatch.rs` recovers the owner from it. Owners detect their own cancellation by checking `state == ACTIVE` before removing, which is why a foreign cancellation is silent rather than a crash. - `Timespec::now(AllowMockedTime)` returns the fake-timers clock when one is installed; `ForceRealTime` always reads the monotonic clock. A deadline must be computed with the clock of the heap it is inserted into. - `FakeTimers::clear()` returns the owners of the popped nodes so they can be released after the heap borrow ends (`TimeoutObject` drops its heap ref, `AbortSignal.timeout()` is cancelled through its signal, and now a `CronJob` is stopped); a node that is only unlinked leaves its owner holding event-loop refs for a timer that will never fire. - `spawnSync` runs the child on an isolated mini event loop and blocks the JS thread in a tick loop, so no `jest.advanceTimersByTime()` can run while it waits; the only clock that can make its timeout elapse is the real one. <details> <summary>Repro on the released binary (1.4.0)</summary> ```ts import { expect, jest, test } from "bun:test"; test("Bun.spawn timeout armed under fake timers, then useRealTimers()", async () => { jest.useFakeTimers(); const p = Bun.spawn(["sleep", "3"], { timeout: 200, killSignal: "SIGKILL" }); jest.useRealTimers(); await p.exited; expect(p.signalCode).toBe("SIGKILL"); // null: child ran for 3 s }); test("spawnSync timeout under fake timers", () => { jest.useFakeTimers(); const r = Bun.spawnSync(["sleep", "3"], { timeout: 200, killSignal: "SIGKILL" }); expect(r.exitedDueToTimeout).toBe(true); // undefined, and the call blocked for 3 s }); test("cron job dropped by useRealTimers", () => { // as a script: the process never exits, the job holds the loop open with a dead timer jest.useFakeTimers(); Bun.cron("* * * * *", () => {}); jest.useRealTimers(); }); ``` The SQL variant from the report (a `Bun.listen` that accepts and never answers, `connectionTimeout: 1`, let the connection get established, then `useRealTimers()`) stays pending past 4 s; with the fix it rejects with `ERR_POSTGRES_CONNECTION_TIMEOUT` after ~1 s. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 16 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 9 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/test/fake-timers/fake-timers.test.ts bun test v1.4.0 (ada0dd1) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [34.23ms] (pass) advanceTimersToNextTimer > one setTimeout [11.19ms] (pass) advanceTimersToNextTimer > setInterval [10.59ms] (pass) advanceTimersToNextTimer > sorted timeouts [18.70ms] (pass) advanceTimersToNextTimer > alternating intervals [12.93ms] (pass) advanceTimersByTime > setInterval [9.41ms] (pass) runOnlyPendingTimers > two setIntervals [12.40ms] (pass) runAllTimers > two setIntervals [12.89ms] (pass) getTimerCount > returns correct count of pending timers [10.69ms] (pass) getTimerCount > throws error if fake timers not active [5.09ms] (pass) clearAllTimers > clears all pending timers [6.90ms] (pass) clearAllTimers > throws error if fake timers not active [3.88ms] (pass) AbortSignal.timeout > pending signals stay alive while the fake heap holds their timer [653.89ms] (pass) AbortSignal.timeout > useRealTimers() releases the signals whose timers it dropped [696.70ms] (pass) AbortSign ... (truncated) release without fix: 11 FAILED bun test v1.4.0-canary.1 (da3851e) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [10.73ms] (pass) advanceTimersToNextTimer > one setTimeout [0.29ms] (pass) advanceTimersToNextTimer > setInterval [0.17ms] (pass) advanceTimersToNextTimer > sorted timeouts [0.22ms] (pass) advanceTimersToNextTimer > alternating intervals [0.13ms] (pass) advanceTimersByTime > setInterval [0.15ms] (pass) runOnlyPendingTimers > two setIntervals [0.17ms] (pass) runAllTimers > two setIntervals [0.15ms] (pass) getTimerCount > returns correct count of pending timers [0.12ms] (pass) getTimerCount > throws error if fake timers not active [0.11ms] (pass) clearAllTimers > clears all pending timers [0.10ms] (pass) clearAllTimers > throws error if fake timers not active [0.04ms] (pass) AbortSignal.timeout > pending signals stay alive while the fake heap holds their timer [12.11ms] 238 | test("useRealTimers() releases the signals whose timers it dropped", () => { 239 | const before = liveAbortSignals(); 240 | vi.useFakeTimers(); 241 | leakObservedTimeouts(); 242 | vi.useRealTimers(); 243 | expect(liveAbortSignals() - before).toBeLessThan(N * 0.1); ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/test/fake-timers/fake-timers.test.ts bun test v1.4.0 (ada0dd1) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [27.16ms] (pass) advanceTimersToNextTimer > one setTimeout [7.74ms] (pass) advanceTimersToNextTimer > setInterval [8.05ms] (pass) advanceTimersToNextTimer > sorted timeouts [13.95ms] (pass) advanceTimersToNextTimer > alternating intervals [8.95ms] (pass) advanceTimersByTime > setInterval [7.16ms] (pass) runOnlyPendingTimers > two setIntervals [7.82ms] (pass) runAllTimers > two setIntervals [8.66ms] (pass) getTimerCount > returns correct count of pending timers [7.35ms] (pass) getTimerCount > throws error if fake timers not active [4.26ms] (pass) clearAllTimers > clears all pending timers [4.93ms] (pass) clearAllTimers > throws error if fake timers not active [2.71ms] (pass) AbortSignal.timeout > pending signals stay alive while the fake heap holds their timer [619.24ms] (pass) AbortSignal.timeout > useRealTimers() releases the signals whose timers it dropped [609.59ms] (pass) AbortSignal.tim ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 827ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/1179] fetch hdrhistogram [hdrhistogram] up to date [2/1179] fetch lshpack [lshpack] up to date [3/1179] fetch lolhtml [lolhtml] up to date [4/1179] cc obj/vendor/zlib/deflate_slow.c.o [5/1179] cc obj/vendor/zlib/insert_string_roll.c.o [6/1179] cc obj/vendor/zlib/deflate_quick.c.o [7/1179] cc obj/vendor/zlib/insert_string.c.o [8/1179] cc obj/vendor/zlib/functable.c.o [9/1179] cc obj/vendor/zlib/deflate_rle.c.o [10/1179] cc obj/vendor/zlib/deflate_medium.c.o [11/1179] cc obj/vendor/zlib/uncompr.c.o [12/1179] cc obj/vendor/zlib/deflate_stored.c.o [13/1179] cc obj/vendor/zlib/inftrees.c.o [14/1179] cc obj/vendor/zlib/zutil.c.o [15/1179] cc obj/vendor/zlib/cpu_features.c.o [16/1179] gen generated_host_exports.rs generated_host_exports.rs: 93 exports (host=3, lazy=10, generic=80, rust=0); 239 extern-C blocks audited [16/1179] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Co ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/event_loop/EventLoopTimer.rs | 21 ++- src/event_loop/SpawnSyncEventLoop.rs | 6 +- src/runtime/api/bun/js_bun_spawn_bindings.rs | 13 +- src/runtime/api/cron.rs | 11 ++ src/runtime/bake/dev_server/hmr_socket.rs | 2 +- src/runtime/bake/dev_server/source_map_store.rs | 2 +- src/runtime/dns_jsc/dns.rs | 2 +- src/runtime/socket/UpgradedDuplex.rs | 2 +- src/runtime/socket/WindowsNamedPipe.rs | 2 +- src/runtime/test_runner/timers/FakeTimers.rs | 21 ++- src/runtime/timer/Timer.rs | 2 +- src/runtime/timer/mod.rs | 2 +- src/runtime/valkey_jsc/js_valkey.rs | 2 +- src/sql_jsc/mysql/JSMySQLConnection.rs | 4 +- src/sql_jsc/postgres/PostgresSQLConnection.rs | 4 +- test/js/bun/test/fake-timers/fake-timers.test.ts | 160 +++++++++++++++++++++++ 16 files changed, 225 insertions(+), 31 deletions(-) ``` </details> **gate history** · 1 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/event_loop/EventLoopTimer.rs 1 4 0 src/event_loop/SpawnSyncEventLoop.rs 1 4 0 src/runtime/api/bun/js_bun_spawn_bindings.rs 4 5 0 src/runtime/api/cron.rs 3 2 0 src/runtime/bake/dev_server/hmr_socket.rs 1 1 0 src/runtime/bake/dev_server/source_map_store.rs 2 1 0 src/runtime/dns_jsc/dns.rs 3 1 0 src/runtime/socket/UpgradedDuplex.rs 1 1 0 src/runtime/socket/WindowsNamedPipe.rs 1 1 0 src/runtime/test_runner/timers/FakeTimers.rs 2 7 0 src/runtime/timer/Timer.rs 3 1 0 src/runtime/timer/mod.rs 6 1 0 src/runtime/valkey_jsc/js_valkey.rs 1 1 0 src/sql_jsc/mysql/JSMySQLConnection.rs 1 2 0 src/sql_jsc/postgres/PostgresSQLConnection.rs 3 2 0 test/js/bun/test/fake-timers/fake-timers.test.ts 4 8 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
#37946 moved the resolver's 1s poll timer (Tag::DNSResolver) out of the fake timer heap and onto the real clock, but added no DNS coverage. These tests pin that behavior: an in-flight query is not counted by getTimerCount() and still times out after useRealTimers(), runAllTimers() returns while a query is in flight, and the timer is armed against the real clock even when the mocked clock has been pushed years ahead. All of them fail on bun 1.4.0 (getTimerCount() is 1, the query never settles, runAllTimers() spins until killed) and pass on main.
f45c3a2 to
81a19e1
Compare
robobun
changed the title
There was a problem hiding this comment.
LGTM — test-only addition covering the c-ares poll timer under jest.useFakeTimers(); the source fix already landed via #37946.
What was reviewed:
- Confirmed the diff is now test-only (source changes in
EventLoopTimer.rs/dns.rswere subsumed by #37946, which is on main). - Tests are hermetic (local
127.0.0.1UDP socket, no external DNS), clean up via try/finally, and drain child stdout/stderr/exited concurrently. - Fixture follows the existing
test-timers-gc-spin-fixture.tspattern in the same file; non-.test.tsname keeps it out of the default glob. - Checked the second fixture test guards the
ForceRealTimeclock read specifically (mocked clock pushed years ahead), not just the tag opt-out.
Extended reasoning...
Overview
This PR now adds only test coverage: a new describe block in test/js/bun/test/test-timers.test.ts and a spawned fixture test-timers-dns-resolver-fixture.ts. The two source-line fix it originally carried (adding Tag::DNSResolver to the fake-timer opt-out and switching Resolver::add_timer to ForceRealTime) has already merged as part of #37946, which converted allow_fake_timers() into an allowlist. I verified on the checked-out head that allow_fake_timers() is the allowlist form (TimeoutObject | AbortSignalTimeout | CronJob) and that add_timer reads ForceRealTime, so these tests exercise code that is already on main.
Security risks
None. The tests bind a UDP socket on 127.0.0.1:0 and point a dns.promises.Resolver at it; nothing answers, and no external network is contacted. No production code is touched.
Level of scrutiny
Low-to-medium. Test-only, follows the exact shape of the neighboring real timer heap is ticked against the real clock test (same spawn timeout / signalCode assertion / stdout JSON-line pattern). The in-process test correctly scopes useFakeTimers()/useRealTimers() in a try/finally and closes the UDP socket in an outer finally registered before the assertions. The child-process test uses await using on the spawn handle, drains all three streams with Promise.all, and asserts signalCode === null before exitCode === 0 so a spinning child produces a clear failure.
Other factors
- The fixture's first test keeps the query alive ~50 s (5 s × 10 tries, citing c-ares'
MAX_TIMEOUT_MS) so a regression to the pre-fix spin is caught by the 20 s spawn timeout rather than accidentally passing when c-ares gives up early. - The fixture's second test advances the mocked clock past any plausible uptime and then awaits a real
ETIMEOUT, which specifically guards theForceRealTimeread inadd_timer— a regression on that line alone would leave the deadline years out and hang this test. - The 30 s per-test timeout is higher than the repo guideline but is justified in-line: the child needs a 20 s bound to catch the pre-fix spin, and the passing path spends ~1 s of real time.
- The comment-cop bot's note on
dns.rswas addressed in an earlier push and is moot now that the source diff is gone. - CI on the earlier head (which included the source changes) was green on all lanes that ran; the current head only differs in tests.
1 participant
Re-scoped to tests only: #37946 landed while this was open and contains the two source lines this PR originally carried (
Tag::DNSResolverno longer fakeable,Resolver::add_timerarming withForceRealTime). It added no DNS coverage, so this PR now just pins that behavior for the resolver. The original description is in the details block at the bottom.Problem
jest.useFakeTimers(), the resolver's 1 s c-ares poll timer went into the fake heap:jest.getTimerCount()was1with a singledns.resolve*()(or, on Linux/macOS,dns.lookup()) in flight,jest.runAllTimers()spun at 100% CPU re-arming it, and a query started under fake timers never settled, even afterjest.useRealTimers()(which discards the fake heap, poll timer included).allow_fake_timers()is an allowlist,add_timerreads the real clock) this is fixed on main, but nothing in the tree exercises the DNS resolver under fake timers, so either half (the tag routing or the clockadd_timerreads) could regress silently.Change
test/js/bun/test/test-timers.test.ts, newdescribe("c-ares poll timer under useFakeTimers"), next to the existing real-heap-under-fake-timers test. Both tests use adns.promises.Resolverpointed at a local UDP socket that never answers, so nothing leaves the machine:getTimerCount()stays0with a query in flight, and afteruseRealTimers()the query still rejects withETIMEOUTabout a second later (c-ares floors the timeout at 250 ms; the poll runs once a second).test-timers-dns-resolver-fixture.ts, run as abun testchild:runAllTimers()returns immediately with a query kept alive for ~50 s (tries: 10, since c-ares caps a try at 5 s), and a second query started after the mocked clock was pushed years past uptime still rejects withETIMEOUTin real time. The second case fails ifadd_timerever reads the mocked clock again, even with the tag routing intact, because the deadline would land years out in the real heap. The child is bounded by a spawn timeout, and the test's own timeout is set above it so a spinning child is reaped rather than orphaned.resolve4()is enough: every request path except the Windows libuv lookup arms this one timer throughrequest_sent->add_timer(c-ares resolve/reverse/getaddrinfo/getnameinfo, the libc work-pool lookups, dns_sd on macOS), so one entry point covers the routing and the clock. A hermeticlookup()variant is not possible, since the libc and dns_sd backends always use the system resolver.Received: 1; the child spins until the spawn timeout kills it, 19.5 s of CPU in 20 s), and failed the same way on the debug build of the parent of the original fix before bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 landed. This is coverage for an already-merged fix, so there is no unfixed main build for these to fail against.Background
timer::Allkeeps two heaps ofEventLoopTimernodes. The event loop drainstimersagainstCLOCK_MONOTONIC; while fake timers are active,All::insertdiverts nodes whoseTag::allow_fake_timers()is true into the fake heap, which only thejest.*functions drain (each pop moves the mocked clock to the popped deadline).useRealTimers()/clearAllTimers()pop that heap without firing anything.ares_process_fd(ARES_SOCKET_BAD, ARES_SOCKET_BAD)once a second while anything is pending so c-ares gets to act on them. The node is armed byrequest_sent/request_completed(clock read inadd_timer) and re-armed fromcheck_timeoutswith thenowthe heap drain passes in, so both the heap it lands in and the clockadd_timerreads have to be the real ones.Original description (source fix, superseded by #37946)
Problem
jest.useFakeTimers(),jest.getTimerCount()reports1as soon as anode:dnsresolve*()query is in flight, with no user timers created. The same timer is armed by every other request the resolver sends (request_sentcallers indns.rs: c-aresgetaddrinfo/reverse/lookupService, the libc work-pool lookups, anddns_sd.rson macOS), sodns.lookup()on Linux and macOS shows the same count; only the Windows libuv lookup path does not use it.jest.runAllTimers()in that state does not return: the process sits at 100% CPU in a synchronous loop, so the per-test timeout cannot fire either. For a c-ares query it ends once c-ares gives the query up (5 s per try, timestries); for a libc or dns_sd lookup it cannot end on its own.dns.resolve*()started under fake timers never settles at all, and a laterjest.useRealTimers()does not help, since it discards the fake heap, poll timer included.Tag::DNSResolverwas not on theTag::allow_fake_timers()opt-out list insrc/event_loop/EventLoopTimer.rs, soAll::insertput the node in the fake heap;FakeTimers::execute_all_timersiswhile execute_next() {}, each pop rancheck_timeouts->ares_process_fd, c-ares judged the query by its own real clock, andadd_timerre-armed at mocked now + 1 s.add_timeralso readTimespec::now(AllowMockedTime)for the deadline.Fix (as originally proposed here; now on main via #37946)
Tag::DNSResolverto the opt-out list and read the clock withForceRealTimeinResolver::add_timer, because the real heap is drained against the real clock, so a mocked deadline there would be either already due or years away. Same treatmentDnsSdConnectionandGcRepeatingalready had; bun:test: cover Bun.serve's Date header timer under fake timers #37935 does the same for the Date header timer.[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file