[robobun]

Test-only change.

Problem

• bun bd test test/js/bun/shell/shell-blocking-pipe.test.ts on a debug + ASAN build fails the first test ("writing > send buffer size doesn't block the main thread") with this test timed out after 5000ms (8033ms on this machine; on main 87b26b5 it was seen failing at 5107ms and passing at 4879ms). The sibling test, which runs the same 1 MiB echo ... | cat pipeline, takes about 100ms. bun bd test is the workflow CLAUDE.md prescribes, so any change that adds to this file (io: POSIX pipe writers hold a ref on their parent from on_error until close() returns #34697 does, for example) runs into it when verified with a debug build. CI itself does not fail: scripts/runner.node.mjs passes --timeout=90000 (270000 on ASAN lanes) to every file, so there the ASAN lanes just spend the 6 to 8 seconds.
• The time is the generateHeapSnapshot("v8") call at line 17. A snapshot walks every live cell, and inside bun test the heap also holds the runner, the preload and harness: about 13.6k nodes and 44k edges, which the v8 snapshot builder takes 6 to 8 seconds to walk under debug + ASAN here (about 10ms in a release build). The pipeline is irrelevant to the cost: a v8 snapshot of an otherwise idle bun test heap costs the same.
• The call existed to exercise the memoryCost hooks of the shell's native classes (ParsedShellScript, ShellInterpreter). It asserted nothing, and because .text() only starts the command on a later microtask, it ran before any ShellInterpreter existed.

Fix

• The two pipe tests no longer take a snapshot; they check only the property in their names.
• The memoryCost coverage moves to test/js/bun/shell/shell-heap-snapshot-fixture.ts, spawned from the same test file, which is how v8-heap-snapshot.test.ts already takes its snapshots. The fixture's heap is about 2k nodes, so a snapshot costs about 0.2s under debug + ASAN; the new test takes 1.1 to 1.5s under bun bd test and about 60ms in release, independent of what the test runner has loaded.
• The fixture asserts what the hooks report at the three points where the objects exist: before the command starts, ParsedShellScript reports at least the 1 MiB script; while it runs, ShellInterpreter reports at least the script (the snapshot is taken synchronously after .then() starts the command, and a pipeline with a child process in it cannot finish until the event loop runs); after it finishes, whatever ShellInterpreter cells remain report far less than the script, because finish() resets the parse arena. It also checks the command's output survived the full GC a snapshot runs.
• Why this shape rather than the smaller alternatives:
  • Switching the existing call to the JSC format in place would fit the budget (one in-process JSC snapshot is 1.2 to 1.6s here) but keeps an assertion-free call whose cost tracks the runner's heap.
  • Making the assertions in-process does not fit: successive JSC snapshots inside bun test measured 1.3s, 1.7s, 2.1s, 3.9s, 5.2s as each one's output stays in the heap the next one walks, so three of them are back over the budget.
  • Exposing the two objects through bun:internal-for-testing and calling estimateShallowMemoryUsageOf on them would reach the same hook in microseconds and also run on Windows, but needs runtime changes plus a test that rebuilds the interpreter's construction protocol by hand; the fixture uses only the public $ API and the snapshot, like the native-size checks in heap-snapshot.test.ts. Happy to do the binding version instead if that is preferred.
• The JSC and v8 formats reach the shell through the same hook (JSCell::estimatedSizeInBytes -> generated estimatedSize -> memoryCost -> Rust memory_cost()), so the JSC format used by the fixture exercises the same shell code the v8 call did, and its output can be read directly.
• Verified:
  • bun bd test test/js/bun/shell/shell-blocking-pipe.test.ts: 3 pass, new test 1.1 to 1.5s. The unmodified file still times out under the same build on this machine.
  • USE_SYSTEM_BUN=1 bun test test/js/bun/shell/shell-blocking-pipe.test.ts: 3 pass.
  • With Interpreter::compute_estimated_size_for_gc temporarily not counting args, the new test fails with Expected: >= 1048576, Received: 3162; with the same change in ParsedShellScript::compute_estimated_size_for_gc it fails with Received: 1868. Both experiments were reverted; this PR touches no src/ file.

Background

• memoryCost / estimatedSize: a class declared in a *.classes.ts file with memoryCost: true gets a generated estimatedSize(cell, vm) entry in its JSC method table which calls the wrapped Rust object's memory_cost(). Both heap snapshot builders (and estimateShallowMemoryUsageOf) record that value as the cell's size. The GC itself reads a separately cached estimated_size(), so in the test suite a heap snapshot is what calls memory_cost().
• ParsedShellScript / ShellInterpreter: $`...` parses the script right away into a ParsedShellScript owned by the returned promise. The first then() (which is what await ends up calling) creates a ShellInterpreter, moves the parsed script into it and starts it. The parsed script is an arena holding the tokens, strings and AST, so it is at least as large as the script text; finish() resets the arena before resolving the promise, and the interpreter's JS wrapper can outlive that until the next GC. Neither object is reachable from JS.
• Test budgets: bun test defaults to 5s per test and bun bd test forwards its arguments unchanged, while the CI runner passes --timeout of 90s (270s with ASAN) per test. Whether bun bd test should mirror the runner is a separate question; this PR only makes this file cheap under either budget.

Snapshot timings measured for this PR (debug + ASAN build, 12 CPU container; release numbers from the baked release build)

heap	nodes	v8 format	jsc format
plain script, debug	~1.6k	760 to 1270ms	160 to 220ms
inside bun test, debug	~13.6k	5.9 to 7.6s	1.2 to 1.3s
plain script, release	~1.6k	6 to 10ms	8 to 10ms
inside bun test, debug, with the 1 MiB pipeline pending	~13.6k	same as idle	same as idle

Strings are truncated to 1024 characters by both builders, so the 1 MiB strings do not affect the cost; the node count does. Inside bun test the count is the same with or without the test file importing harness, since test/preload.ts imports it anyway.

Fixture phases under debug + ASAN: process start ~0.35s, parse + start + drain of the 1 MiB pipeline ~0.1s, three snapshots ~0.2 to 0.3s each. An earlier version of the fixture used summarizeByType from test/js/bun/util/heap.ts; that allocates an array per node, which survived into the following snapshots and roughly doubled the fixture's time, so the fixture sums node sizes directly.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d99eeed1-08d9-4c73-81a2-4e4fd811cb5f

📥 Commits

Reviewing files that changed from the base of the PR and between d3f975b and 41d232c.

📒 Files selected for processing (2)

• test/js/bun/shell/shell-blocking-pipe.test.ts
• test/js/bun/shell/shell-heap-snapshot-fixture.ts

---

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

[robobun]

^{Updated 7:05 AM PT - Aug 15th, 2026}

❌ @robobun, your commit 41d232c has some failures in Build #97645 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38940

That installs a local version of the PR into your bun-38940 executable, so you can run:

bun-38940 --bun

[robobun]

Status

• Reproduced on main with a debug + ASAN build: bun bd test test/js/bun/shell/shell-blocking-pipe.test.ts fails the first test with this test timed out after 5000ms (8033ms), while the sibling test with the same 1 MiB pipeline takes about 100ms. The time is the in-process generateHeapSnapshot("v8") walking the bun test heap (about 13.6k nodes, 6 to 8s under debug + ASAN, about 10ms in release). This bites bun bd test runs; CI lanes pass a much larger --timeout through the runner and only pay the time.
• This PR removes the snapshot from the pipe tests and moves the memoryCost coverage it was providing into shell-heap-snapshot-fixture.ts, a small-heap process spawned from the same file, with assertions on what ParsedShellScript and ShellInterpreter report. With the change the file passes under bun bd test in about 1.3s total and under the release build in under 0.3s.
• Test-only change. The PR description lists the alternatives considered (JSC format in place, in-process assertions, test-only bindings plus estimateShallowMemoryUsageOf) and why this shape was chosen; the bindings variant is available on request.
• Follow-up from review: the fixture picks the node stride from the snapshot's type, the same way test/js/bun/util/commonjs-module-heap-snapshot-fixture.cjs does.
• CI, build 97645 on the current head (41d232c): 177 of 179 jobs passed, which is every shard of every Linux lane (including x64-asan, the configuration the timeout came from), both Windows lanes and both macOS build jobs. The build is marked failed only because its two darwin 14 aarch64 - test-bun jobs expired without ever being assigned an agent, four times over six hours; the same lane is sitting unstarted or expired on essentially every other build from today, so a retrigger would just queue behind it. If a macOS result is wanted before merging, retrying those two jobs once that queue is draining is enough; the new test exercises the same echo ... | $(which cat) construction as the two tests in this file that already pass on macOS, and the sizes it checks are platform independent.

[claude]

LGTM — test-only de-flake that moves the heap snapshot into a fixture process and replaces an assertion-free call with real memoryCost checks.

What was reviewed:

• Verified Bun.generateHeapSnapshot() (default JSC path, BunObject.cpp:841) uses the Inspector format, so the fixture's hardcoded stride of 4 is correct; the sibling commonjs-module-heap-snapshot-fixture.cjs branches on type only because other callers may pass a GCDebugging snapshot.
• Confirmed the fixture pattern matches existing snapshot fixtures and the test follows harness conventions (bunExe/bunEnv, await using, concurrent pipe drain, exitCode last).
• Checked that a missing class name (indexOf → -1) yields size 0, which fails the >= scriptLength assertions rather than passing vacuously.
• The PR's mutation-test evidence (breaking compute_estimated_size_for_gc fails the new test) satisfies the "fails for the right reason" bar.

Extended reasoning...

Overview

Test-only change to test/js/bun/shell/shell-blocking-pipe.test.ts plus a new fixture shell-heap-snapshot-fixture.ts. The original test called generateHeapSnapshot("v8") inline, which under debug+ASAN walks ~13.6k nodes and blows the 5s default timeout. The fix removes the snapshot from the two pipe tests (they now check only what their names claim) and adds a third test that spawns a small-heap fixture process to take three JSC-format snapshots, asserting that ParsedShellScript and ShellInterpreter report ≥ 1 MiB while holding the script and far less afterward. No src/ files touched.

Security risks

None. Test code only; spawns bunExe() on a local fixture with bunEnv, no network, no untrusted input.

Level of scrutiny

Low-to-moderate. This is a de-flake of a known-flaky test with an unusually thorough PR description: root cause traced to snapshot cost scaling with heap size, timings tabulated, and the new assertions mutation-tested against compute_estimated_size_for_gc. The one non-obvious detail — hardcoding node stride 4 — I verified is correct for Bun.generateHeapSnapshot()'s default path (BunObject.cpp:841 constructs HeapSnapshotBuilder with no snapshot-type override, i.e. Inspector format). The existing heap.ts and commonjs-module-heap-snapshot-fixture.cjs branch on type because they may receive GCDebugging snapshots from bun:jsc; this fixture always calls the Bun. variant.

Other factors

• REVIEW.md's "never silently weaken" rule is satisfied: the original snapshot call asserted nothing and (per the PR) ran before any ShellInterpreter existed; the replacement is strictly stronger coverage.
• Harness conventions followed: await using proc, Promise.all([stdout, stderr, exited]), stderr and stdout asserted before exitCode, skipIf(isWindows) matching sibling tests, no sleeps.
• No CODEOWNERS entry covers test/js/bun/shell/.
• No prior reviewer comments on the PR.

[claude]

LGTM — test-only de-flake that moves the heap snapshot into a small-heap fixture process and replaces a no-op call with real assertions.

What was reviewed:

• Fixture's node layout (nodes[i+1] = size, nodes[i+2] = classNameIdx) and the GCDebugging ? 7 : 4 stride match test/js/bun/util/heap.ts and commonjs-module-heap-snapshot-fixture.cjs.
• Original snapshot call asserted nothing and ran before any ShellInterpreter existed; the new test asserts memoryCost at three lifecycle points and the author verified it fails when compute_estimated_size_for_gc stops counting the script.
• Spawn follows harness conventions: bunExe/bunEnv, await using, concurrent drain of stdout/stderr/exited, stderr and parsed values asserted before exitCode, fixture named *-fixture.ts, Windows skip consistent with sibling tests.

Extended reasoning...

Overview

Test-only change to test/js/bun/shell/shell-blocking-pipe.test.ts plus a new fixture shell-heap-snapshot-fixture.ts. The first pipe test was timing out under debug+ASAN because it called generateHeapSnapshot("v8") in-process, which walks the entire ~13.6k-node bun test heap (6-8s under debug+ASAN). The snapshot call asserted nothing and — because .text() starts the command on a later microtask — ran before any ShellInterpreter existed, so it wasn't exercising what it claimed to. The PR removes the snapshot from the pipe tests and adds a third test that spawns a small-heap fixture process, takes three JSC-format snapshots at the ParsedShellScript / ShellInterpreter lifecycle points, and asserts each reports (or stops reporting) the 1 MiB script via memoryCost().

Security risks

None. Test-only; no src/ changes, no network, no untrusted input.

Level of scrutiny

Low-to-medium. This is a de-flake of a debug-lane timeout that also strengthens coverage. I checked the snapshot-parsing details against the existing helpers: the node stride selection (type === "GCDebugging" ? 7 : 4) and the field offsets (id=0, size=1, classNameIdx=2) match NodeLayout in test/js/bun/util/heap.ts and the identical pattern in commonjs-module-heap-snapshot-fixture.cjs. The JSC and v8 snapshot formats reach the same estimatedSize → memoryCost hook, so switching to the JSC format for the assertions doesn't lose coverage. The fixture uses Bun.which("cat") to force a real child process into the pipeline so the command cannot complete before the synchronous "while running" snapshot — the same construction the sibling pipe tests already use.

Other factors

• Harness conventions are followed: bunExe()/bunEnv, await using proc, Promise.all over stdout/stderr/exited, assertions on stderr and parsed stdout before exitCode, fixture file named *-fixture.ts, test.skipIf(isWindows) matching the sibling tests that need system cat.
• The new assertions are meaningful (>= 1 MiB before/during, < 1 MiB after finish) and the PR description documents that temporarily removing the arena from compute_estimated_size_for_gc on either class makes the test fail with clear values, so the test can fail for the right reason.
• USE_SYSTEM_BUN=1 passing is expected here — this is a test refactor with no runtime behavior change, not a bugfix that must fail on the unfixed binary.
• No prior reviews from me on this PR; no outstanding reviewer comments; the one earlier note about hardcoding stride 4 was already addressed in commit 41d232c.

[robobun]

This change also removes a CI flake, so it is worth a look for that reason too.

The generateHeapSnapshot("v8") call at line 17 is the only heap snapshot that a darwin x64 shard takes inside a bun test --parallel worker. The snapshot stays registered in that worker's VM, and from then on the end of every GC walks the whole heap. When the same worker later runs test/js/bun/css/color.test.ts (2197 expect() calls in one test, one GC each under CI's BUN_GARBAGE_COLLECTOR_LEVEL=1), the test exceeds the 90 s limit: ansi-16 never emits a 256-color escape, "test timed out", 263 of the last 400 builds, darwin x64 only.

On an Intel Mac mini from the CI pool, the shard's batch reproduces it in three of four runs (232 s to 270 s). With this file's in-process snapshot taken out of the batch, the same test takes 2.6 s to 7 s. The measurements and the cause are in #34085 (comment).

[robobun]

Data from a second symptom of the same retained snapshot.

test/js/bun/yaml/yaml-test-suite.test.ts takes 22 to 40 s on the two darwin lanes and 0.1 to 1.0 s on the others. In those shards test/js/bun/util/heap-snapshot.test.ts also takes a snapshot inside the bun test --parallel worker (7 calls of Bun.generateHeapSnapshot()), and it sorts before the yaml files. So this PR removes the trigger for color.test.ts, and the yaml files stay slow until oven-sh/WebKit#656 lands.

The numbers and a replay on Linux are in oven-sh/WebKit#656 (comment) and in #46610.