inspector: fix TestReporter ID collision between retroactive and live registration by hughescr · Pull Request #34221 · oven-sh/bun

hughescr · 2026-07-15T07:58:25Z

What does this PR do?

Fixes a TestReporter inspector-protocol bug where two tests/describes in
the same run can be reported under the same id.

TestReporter test/describe ids came from two independent counters:

The live-registration path (ScopeFunctions::call, which assigns an
id as each describe()/test() call is collected) used a function-local
static AtomicI32 that only advances while the agent is enabled.
The retroactive path (retroactively_report_discovered_tests, which
walks the already-discovered scope tree and reports everything found so
far when TestReporter.enable arrives after collection has already
started) seeded a fresh max_id at 0 on every call.

When enable lands mid-collection — some scopes already discovered,
others still being registered live — both paths run in the same process and
each hands out its own 1..N range, so two unrelated tests can end up
reported under the same id. Anything that keys state by id (e.g. an
inspector client correlating found/start/end events) silently
conflates the two.

The fix shares one counter instead of two. Debugger gains a
next_test_id_for_debugger: i32 field:

ScopeFunctions::call increments it directly (same-process field access —
no FFI involved there).
retroactively_report_discovered_tests takes the counter by value and
returns the advanced value through the
RuntimeHooks::retroactively_report_discovered_tests hook; the caller
(test_reporter_agent_enable) writes the return value back into the
field. The retroactive walk therefore resumes from wherever live
registration last left off instead of restarting at 0; the counter
crosses the hook by value rather than as a raw *mut i32 (the hook's
agent parameter is itself a raw pointer, per the unsafe call site).

Scope is intentionally narrow: one bug, one fix, four files
(Debugger.rs, VirtualMachine.rs, jsc_hooks.rs, ScopeFunctions.rs),
plus the regression test.

How did you verify your code works?

Added a deterministic regression test to
test/cli/inspect/test-reporter.test.ts ("assigns unique test IDs when
TestReporter.enable lands mid-collection") alongside the existing
retroactive-reporting test in the same file.

The existing test in that file enables TestReporter only after
collection has fully finished (it waits for a test to start executing
first), so the live-registration path never runs again within that test —
the two counters never actually interleave, and it can't catch this bug.

The new test creates a genuine mid-collection window by pausing inside an
async describe() callback. describe() callbacks aren't invoked
inline — describe() only registers a scope and enqueues its callback, and
every enqueued callback (including one from a sibling describe() called
earlier at the top level) runs only once collection's step loop gets to it.
So a plain top-level await between two describe() calls doesn't
actually pause collection with useful partial state — neither callback has
run yet by that point. Pausing inside describe("suite B", async () => { ... })'s own body does: by the time that callback starts, describe("suite A", ...) (a synchronous sibling registered earlier) has already run to
completion, registering two real tests, while suite B's own test is not yet
registered.

TestReporter.enable is sent during that window, so suite A's describe + 2
tests go through the retroactive path and suite B's test goes through the
live path in the same run. The test asserts every reported id across both
paths is unique (via a raw, non-deduplicating list of found ids -- the
Map-keyed bookkeeping used elsewhere would silently coalesce a collision),
and that every end event's id corresponds to an actual test, not a
describe.

# Fails deterministically on unpatched Bun (verified against system Bun 1.3.14):
USE_SYSTEM_BUN=1 bun test -t "mid-collection" test/cli/inspect/test-reporter.test.ts
# ✗ ... this test timed out after 5000ms.
#   (pre-fix, suite B's live-registered test collides with an id the
#   retroactive walk already used, so the runner never observes the
#   expected number of distinct `found` ids and the wait hangs; the
#   `end` ids happen to stay distinct in this deterministic run, so that
#   wait is not what fails)
# Reproduced across 3 consecutive runs.

# Passes against this fix:
bun bd test test/cli/inspect/test-reporter.test.ts
#  2 pass
#  0 fail
# Reproduced across 3 consecutive runs (~0.9-1.1s each).

Also verified:

bun bd test test/cli/inspect/ (full inspector suite) — 38 pass, 4 todo,
0 fail, no regressions.
cd src/js && bun x tsc --noEmit — error set is byte-for-byte identical
(file, line, col, TS code) to unmodified main, 1628/1628 — no new
TypeScript errors from the test-file change.
A full non-quiet rebuild (bun scripts/build.ts --profile=debug, no
--quiet/positional args so ninja's cargo output isn't suppressed) after
touching all four changed Rust files shows a clean recompile with no
warnings.

Found while investigating #34218 (unrelated root cause — filed separately per its "different bugs, different triggers" scope note; sibling PRs: #34219, and the writer-layer fix).

https://claude.ai/code/session_013DexKQwDASzoRVDFS8Jtyn

… registration TestReporter test/describe IDs came from two independent counters: the live-registration path (ScopeFunctions::call, assigning IDs as describe()/ test() calls are collected) used a function-local `static AtomicI32`, while the retroactive path (retroactively_report_discovered_tests, which walks the already-discovered scope tree when TestReporter.enable arrives after collection has started) seeded a fresh `max_id` at 0 on every call. When `enable` landed mid-collection - some scopes already discovered, others still being registered live - both paths were active in the same run and handed out overlapping 1..N ranges, so two unrelated tests could end up reported under the same id. Consumers that key state by id (this repo's own inspector-backed test aggregation included) silently conflate the two. Fix: share one counter. `Debugger` gains a `next_test_id_for_debugger: i32` field; `ScopeFunctions::call` increments it directly (in-process), and `retroactively_report_discovered_tests` takes/returns it by value through the `RuntimeHooks::retroactively_report_discovered_tests` hook (the caller writes the returned value back into the field) so the walk resumes from wherever live registration left off instead of restarting at 0. The counter crosses that hook by value rather than as a raw `*mut i32`. Adds a deterministic regression test: TestReporter.enable is sent while collection is paused inside an async describe() callback (a genuine mid-collection window - describe() callbacks aren't invoked inline, so a plain top-level await between two describe() calls doesn't actually pause collection, but pausing inside a describe() body does, since a sibling describe() registered earlier by then has already run to completion). This reliably collides pre-fix: the retroactive walk hands out ids 1-4 (suite A's describe, its 2 tests, and suite B's still-empty describe), while the live counter sits untouched at 0, so the next live-registered test (suite B's own test) gets id 1 - colliding with the describe that the retroactive walk already assigned id 1 to (empirically confirmed on unpatched Bun 1.3.14: raw found ids [1,2,3,4,1]). Fails deterministically on unpatched Bun (verified against system Bun 1.3.14) and passes 3/3 against this fix.

claude

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

coderabbitai · 2026-07-15T08:05:53Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: af8ae25f-4fc3-40a2-a995-4696cf05e5de

📥 Commits

Reviewing files that changed from the base of the PR and between 45c27b2 and e3c18a8.

📒 Files selected for processing (1)

src/runtime/jsc_hooks.rs

Walkthrough

Changes

The debugger now shares test ID allocation between live collection and retroactive reporting. The runtime hook passes and returns the counter, while inspector regression coverage verifies unique IDs when TestReporter is enabled mid-collection.

Suggested reviewers: cirospaciari, robobun

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: fixing TestReporter ID collisions between retroactive and live registration.
Description check	✅ Passed	The description includes the required purpose and verification sections and gives detailed, relevant implementation and test coverage.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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/cli/inspect/test-reporter.test.ts`:
- Around line 469-470: Add a diagnostic check immediately before the exitCode
assertion in the subprocess test: when proc.exited is nonzero, assert the
captured stderr is empty so the failure exposes the subprocess error output.
Preserve the existing expect(exitCode).toBe(0) assertion and use the stderr
value already provided by proc.

🪄 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: 38b69fb4-9eea-4f91-b236-4462c1377175

📥 Commits

Reviewing files that changed from the base of the PR and between be77b65 and b8a8aec.

📒 Files selected for processing (5)

src/jsc/Debugger.rs
src/jsc/VirtualMachine.rs
src/runtime/jsc_hooks.rs
src/runtime/test_runner/ScopeFunctions.rs
test/cli/inspect/test-reporter.test.ts

…e assertion Reorder the mid-collection TestReporter test so behavioral assertions (the rawFoundIds uniqueness check) surface before the bare exit-code comparison, and gate the exit-code assertion with a stderr-empty check so a failing child's stderr gets printed via the failed expectation instead of being silently swallowed. This matches the exit-code-last ordering adopted on sibling PR oven-sh#34219.

* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...

* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...

hughescr · 2026-07-31T00:24:22Z

Housekeeping for reviewers: I've just merged latest main (bbe3f6a262) into this branch — a clean merge (commit 92976b6b21, no conflicts), as a normal merge commit with no force-push, so existing review comments and their anchors are intact. bun bd test test/cli/inspect/test-reporter.test.ts is green after the merge.

Since I have four related-but-independent PRs open (#34219, #34220, #34221, #34222), a quick map so nobody has to reconstruct it: no two of them touch the same source file. The only overlap anywhere is test coverage — #34219, #34220, and this PR each extend test/cli/inspect/test-reporter.test.ts, so whichever lands first leaves a test-file-only conflict in the other(s); I'll rebase and resolve promptly whenever anything merges. This PR and #34220 don't overlap even in tests, and #34222 shares nothing with the other three. Any merge order works.

@hughescr

…ctive paths (#36522) Adopts the fix from #34221 by @hughescr. ### Problem `TestReporter` test/describe IDs came from two independent counters: - the live-registration path (`ScopeFunctions::call`, which assigns an ID as each `describe()`/`test()` is collected) used a function-local `static AtomicI32` that only advances while the agent is enabled - the retroactive path (`retroactively_report_discovered_tests`, which walks the already-discovered scope tree when `TestReporter.enable` arrives after collection has started) seeded a fresh local `max_id` at `0` on every call When `enable` lands mid-collection (some scopes already discovered, others still being registered live) both paths run in the same process and each hands out its own `1..N` range, so two unrelated tests can be reported under the same ID. Inspector clients keying state by ID (correlating `found`/`start`/`end` events) silently conflate them. ### Fix Move the counter to a `next_test_id: i32` field on `TestReporterAgent` (matching `HTTPServerAgent::next_server_id` / `ErasedAgentSlot::sequence`): - `ScopeFunctions::call` increments it directly through its `&mut Debugger` borrow - `retroactively_report_discovered_tests` takes the counter by value and returns the advanced value through the `RuntimeHooks` hook; `test_reporter_agent_enable` writes the return back into the field Both paths now draw from the same monotone counter. Single JS thread, so no synchronization needed. ### Verification New test in `test/cli/inspect/test-reporter.test.ts` creates a genuine mid-collection window by pausing inside an async `describe()` callback (suite A's sync callback has already registered 2 tests; suite B's `test()` call has not yet run). `TestReporter.enable` is sent in that window so suite A + suite B's describe go through the retroactive walk and `test B1` goes through the live path in the same run. The test asserts every `found` ID across both paths is unique via a raw non-deduped list (the `Map`-keyed session bookkeeping would coalesce a collision). ``` USE_SYSTEM_BUN=1 bun test test/cli/inspect/test-reporter.test.ts -t "mid-collection" # (fail) ... timed out after 5000ms (Map never reaches 5 distinct keys) bun bd test test/cli/inspect/test-reporter.test.ts # 2 pass, 0 fail ``` Supersedes #34221 (same fix, adopted with co-author credit).  --- **[review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/inspect/test-reporter.test.ts bun test v1.4.0 (532af42) test/cli/inspect/test-reporter.test.ts: (pass) TestReporter inspector protocol > retroactively reports tests when TestReporter.enable is called after tests are discovered [1618.23ms] killed 1 dangling process (fail) TestReporter inspector protocol > assigns unique test IDs when TestReporter.enable lands mid-collection [5001.59ms] ^ this test timed out after 5000ms. 1 pass 1 fail 7 expect() calls Ran 2 tests across 1 file. [8.88s] error: script "bd" exited with code 1 __F:1:S:0 release without fix: 1 FAILED bun test v1.4.0-canary.1 (1498d7b) test/cli/inspect/test-reporter.test.ts: (pass) TestReporter inspector protocol > retroactively reports tests when TestReporter.enable is called after tests are discovered [58.29ms] killed 1 dangling process (fail) TestReporter inspector protocol > assigns unique test IDs when TestReporter.enable lands mid-collection [5000.27ms] ^ this test timed out after 5000ms. 1 pass 1 fail 7 expect() calls Ran 2 tests across 1 file. [5.26s] __F:1:S:0 ``` </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/cli/inspect/test-reporter.test.ts bun test v1.4.0 (532af42) test/cli/inspect/test-reporter.test.ts: (pass) TestReporter inspector protocol > retroactively reports tests when TestReporter.enable is called after tests are discovered [1671.03ms] (pass) TestReporter inspector protocol > assigns unique test IDs when TestReporter.enable lands mid-collection [1602.82ms] 2 pass 0 fail 19 expect() calls Ran 2 tests across 1 file. [5.40s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 692ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/6] cc obj/packages/bun-usockets/src/context.c.o [2/6] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 239 extern-C blocks audited [2/6] 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 Compiling�[0m bun_install v0.0.0 (/workspace/bun/src/install) �[1m�[92m Compiling�[0m bun_jsc v0.0.0 (/workspace/bun/src/jsc) �[1m�[92m Compiling�[0m bun_ast_jsc v0.0.0 (/workspace/bun/src/ast_jsc) �[1m�[92m Compiling�[0m bun_js_parser_jsc v0.0.0 (/workspace/bun/src/js_parser_jsc) �[1m�[92m Compiling�[0m bun_semver_jsc v0.0.0 (/workspace/bun/src/semver_jsc) �[1m�[92m Compiling�[0m bun_sys_jsc v0.0.0 (/workspace/bun/src/sys_jsc) �[1m�[92m Compiling�[0m bun_sql_jsc v0.0.0 (/workspace/bun/src/sql_jsc) �[1m�[92m Compiling�[0m bun_sourcemap_jsc v0.0.0 (/workspace/bun/src/sourcemap_jsc) �[1m�[92m Compiling�[0m bun_css_jsc ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/Debugger.rs | 10 ++- src/jsc/VirtualMachine.rs | 8 +- src/runtime/jsc_hooks.rs | 24 ++--- src/runtime/test_runner/ScopeFunctions.rs | 5 +- test/cli/inspect/test-reporter.test.ts | 145 ++++++++++++++++++++++++++++++ 5 files changed, 174 insertions(+), 18 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/Debugger.rs 4 4 0 src/jsc/VirtualMachine.rs 2 4 0 src/runtime/jsc_hooks.rs 4 5 0 src/runtime/test_runner/ScopeFunctions.rs 4 4 0 test/cli/inspect/test-reporter.test.ts 3 3 0 ``` </details>  --------- Co-authored-by: Craig R. Hughes <craig.git@rungie.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Jarred-Sumner · 2026-08-01T07:08:32Z

Fixed in #36522, thanks

claude Bot reviewed Jul 15, 2026

View reviewed changes

robobun mentioned this pull request Jul 15, 2026

TestReporter inspector events are silently, permanently lost if the process exits before the debugger thread finishes delivering them #34218

Open

coderabbitai Bot reviewed Jul 15, 2026

View reviewed changes

Comment thread test/cli/inspect/test-reporter.test.ts Resolved

hughescr added 3 commits July 15, 2026 23:55

hughescr mentioned this pull request Jul 31, 2026

inspector: drain queued frontend messages to the debugger thread before exit #34219

Closed

This was referenced Jul 31, 2026

inspector: distinguish backpressure and drops in the debugger WebSocket writer #34220

Open

websocket: fix idleTimeout: 0 to actually disable the idle timeout #34222

Closed

robobun mentioned this pull request Jul 31, 2026

inspector: share TestReporter test ID counter between live and retroactive paths #36522

Merged

Jarred-Sumner closed this Aug 1, 2026

Navigation Menu

Repository navigation

inspector: fix TestReporter ID collision between retroactive and live registration - #34221

inspector: fix TestReporter ID collision between retroactive and live registration#34221
hughescr wants to merge 4 commits into
oven-sh:mainfrom
hughescr:pr/testreporter-id-fix

hughescr commented Jul 15, 2026

Uh oh!

claude Bot left a comment

Uh oh!

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

hughescr commented Jul 31, 2026

Uh oh!

Jarred-Sumner commented Aug 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hughescr commented Jul 15, 2026

What does this PR do?

How did you verify your code works?

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

coderabbitai Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hughescr commented Jul 31, 2026

Uh oh!

Jarred-Sumner commented Aug 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading