Restore smol-gated retain-with-limit reset for the module loader transpile arena - #31855
Open
Jarred-Sumner wants to merge 6 commits into
Open
Jarred-Sumner wants to merge 6 commits into
Jarred-Sumner wants to merge 6 commits into
Conversation
Collaborator
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The substantive change here is small (~20 lines in ModuleLoader::reset_arena plus the new test — the other ~200 files are autofix.ci formatting), but it changes arena memory-management semantics on the hot per-require() path, the description notes an outstanding RSS/time benchmark still to be run, and CI is currently red on macOS aarch64 build-rust, so this warrants a human look before merging.
Extended reasoning...
Overview
Despite touching 206 files, the real change is confined to src/jsc/ModuleLoader.rs (reset_arena now gates on vm.smol: full reset() under --smol, reset_retain_with_limit(8 MiB) otherwise, plus debug-only scoped_log! branch tags) and a new subprocess stress test in test/js/bun/resolve/load-same-js-file-a-lot.test.ts. The remaining ~200 files are the [autofix.ci] apply automated fixes commit — trailing-newline removal, import reordering, derive-macro collapsing, and match-arm reflow with no semantic content.
Security risks
None identified. The change is internal allocator-reset policy with no user-controlled inputs, no auth/crypto/permissions surface, and the debug log is compiled out of release builds.
Level of scrutiny
Medium-high. reset_arena runs after every synchronous module transpile, so this is a hot, memory-management-sensitive path. Switching from unconditional heap destroy/new to cap-gated retain is well-motivated (matches every other per-cycle arena call site and the MimallocArena doc), but memory-retention policy changes deserve human eyes for lifetime/UAF implications and RSS regression risk under --smol vs default. The PR description itself flags an outstanding verify-phase action — the before/after RSS+time benchmark required by the work order hasn't been recorded yet.
Other factors
- CI is failing: robobun reports
build-rustfailed on 🍎 aarch64 for commit 3bab1a9 (the pre-autofix commit); unclear if the autofix commit resolved it. - The new test is thorough (exercises both smol/default branches, the over-cap recycle path via oversized parse-error modules, and asserts differentially on debug-log branch tags), which is reassuring for correctness.
- No prior human reviews on the PR.
Contributor
There was a problem hiding this comment.
I didn't find any bugs, but the core change here is a memory-management policy switch in the module loader's per-transpile hot path (plus a loosened RSS threshold in inspect-error-leak.test.js), and the description notes an outstanding benchmark step — worth a human look before merging.
Extended reasoning...
Overview
Despite the 207-file diff, the substantive change is small: ModuleLoader::reset_arena in src/jsc/ModuleLoader.rs now gates between arena.reset() (under --smol) and arena.reset_retain_with_limit(8 MiB) (default), replacing the previous unconditional reset(). A new subprocess stress test in load-same-js-file-a-lot.test.ts asserts the branch taken via debug-only scoped logs, and inspect-error-leak.test.js raises its non-ASAN RSS-growth threshold from 10 MB to 20 MB to accommodate the bounded arena retention. The remaining ~200 files are pure autofix.ci formatting noise (trailing-newline removal, rustfmt derive/import reordering) with no semantic content.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing is touched; the change adjusts an internal allocator-reset policy.
Level of scrutiny
Medium-high. This runs after every synchronous module transpile (every require()), so it sits on a hot, correctness-sensitive path. Switching from destroy/recreate to retain-with-limit changes when arena memory is reclaimed; while the pattern matches other call sites and the description explains the rationale well, memory-lifecycle changes in the module loader deserve human sign-off. The PR also intentionally loosens a leak-test threshold and explicitly calls out an outstanding verify-phase action (the before/after RSS+time benchmark for reports2/24-fix.md), both of which a maintainer should acknowledge.
Other factors
The bug-hunting system found nothing, the new test is thorough (covers both modes, the over-cap recycle path via parse errors, and uses differential log assertions on debug builds), and the formatting churn is benign. The change itself looks correct and well-motivated; deferral is purely because hot-path allocator policy and a relaxed leak threshold are the kind of trade-offs a human reviewer should explicitly approve.
…spile arena ModuleLoader::reset_arena unconditionally destroyed and recreated the per-transpile mimalloc heap after every module, diverging from the intended behavior: a full reset only under --smol, and an 8 MiB cap-gated retain otherwise. The unconditional destroy/new round-trip pays mi_heap_destroy teardown plus page re-commit and re-zero on every transpile cycle; every other per-cycle arena call site (bundler chunk generation, installer, renamer, bake router, the transpiler store hook) already uses reset_retain_with_limit(8 MiB), and the MimallocArena doc comment documents why the 'segment cache keeps pages warm anyway' rationale for unconditional reset was wrong. reset_arena now reads vm.smol and calls reset() under --smol (lowest steady-state RSS) and reset_retain_with_limit(8 * 1024 * 1024) otherwise, matching the other call sites, and emits a BUN_DEBUG_ModuleLoader scoped log of the branch taken (free_all/retained/recycled; debug builds only, compiled out of release). Tested with a subprocess stress test that require()s many distinct small modules (require forces the synchronous transpile path through reset_arena; dynamic import routes through the concurrent transpiler store and skips it) plus oversized 150k-statement modules with a trailing syntax error (the parse-error path parks the arena un-reset, so it is the only path where reset_arena sees an over-cap arena and recycles), with forced GC interleaved, in both default and --smol modes; on debug builds the test counts the branch-tag logs differentially (default: retained>=iters, recycled>=brokenCount, zero free_all; --smol: free_all only), so a revert to unconditional reset or an inverted smol gate fails. Outstanding verify-phase action: run the transpile-heavy before/after RSS+time benchmark the work order requires and record the numbers in reports2/24-fix.md.
…le-arena retention
Jarred-Sumner
force-pushed
the
claude/complex-24-restore-smol-gated-retain-with-limit-res
branch
from
June 5, 2026 03:31
8470cf4 to
cda54bc
Compare
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughModuleLoader now chooses an arena reset strategy based on the VM's ChangesModule loader arena reset policy
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
Contributor
There was a problem hiding this comment.
I didn't find any bugs and the implementation matches the established reset_retain_with_limit(8 MiB) pattern used elsewhere, but this changes hot-path memory retention for every synchronous module load, doubles the inspect-error-leak RSS threshold, and the PR notes the before/after RSS+time benchmark is still outstanding — worth a human sign-off on the perf/RSS tradeoff.
Extended reasoning...
Overview
This PR changes ModuleLoader::reset_arena in src/jsc/ModuleLoader.rs from an unconditional arena.reset() (mi_heap_destroy + mi_heap_new on every synchronous transpile) to a smol-gated policy: full reset under --smol, and reset_retain_with_limit(8 MiB) otherwise. It adds a debug-only scoped_log! of the branch taken, a new subprocess stress test in load-same-js-file-a-lot.test.ts that asserts the branch counts on debug builds, and raises the non-ASAN RSS threshold in inspect-error-leak.test.js from 10 MB to 20 MB to accommodate the now-intentional ≤8 MiB retained heap.
Security risks
None. This is allocator-lifecycle policy — no untrusted input handling, auth, or data exposure surface. The retained heap is bounded by the existing 8 MiB cap and is the same primitive already used by the bundler, installer, renamer, bake router, and the transpiler-store hook.
Level of scrutiny
Medium-high. The diff is small and the pattern is well-established (I verified reset_retain_with_limit(8 * 1024 * 1024) is used identically at 7+ other call sites, and jsc_hooks.rs already does the same smol gate), and the prior code's own PERF comment said this was the intended end state. But this runs on every synchronous require()/transpile cycle and changes steady-state RSS for every Bun process, so it's a runtime-wide perf/memory policy change rather than a mechanical fix.
Other factors
- The PR description explicitly lists an outstanding action: "run the transpile-heavy before/after RSS+time benchmark the work order requires." That benchmark is the stated justification for having deferred this change in the first place.
- The
inspect-error-leak.test.jsthreshold is doubled (10 → 20 MB non-ASAN). The rationale is sound (≤8 MiB bounded retention can land on either side of the baseline sample), but loosening a leak-detection threshold deserves a human glance. - CI shows a
streams-leak.test.tsfailure on one platform. It may well be flake, but it's a memory/leak test failing on a PR that changes memory-retention policy, so I'd rather not auto-approve past it. - The implementation itself looks correct:
vm.smolis read before theas_mut()borrow,reset_retain_with_limitreturnsbool(true=retained/false=recycled) as the log expects, andscoped_log!is compiled out of release builds.
Contributor
There was a problem hiding this comment.
Thanks — both nits are addressed and the change now exactly mirrors the existing smol-gated pattern in jsc_hooks.rs; deferring to a human only because this alters per-transpile arena retention on a hot path and the RSS/time benchmark called out in the description is still outstanding.
Extended reasoning...
Overview
The PR changes ModuleLoader::reset_arena (src/jsc/ModuleLoader.rs) from an unconditional arena.reset() to a vm.smol-gated branch: reset() under --smol, reset_retain_with_limit(8 MiB) otherwise, plus a debug-only scoped_log! of the branch taken. A new subprocess stress test in load-same-js-file-a-lot.test.ts exercises both modes and (on debug builds) asserts the branch counts via BUN_DEBUG_ModuleLoader. Two earlier nits I raised — an unjustified 10→20 MB RSS-threshold raise in inspect-error-leak.test.js and a bare expect(stderr).toBe("") in the non-debug branch — were both addressed in 8502792; the threshold file is no longer in the diff.
Security risks
None. This is allocator-lifecycle policy with no input parsing, auth, or external I/O surface. The added scoped_log! is compiled out of release builds.
Level of scrutiny
Moderate-to-high. The Rust delta is ~10 effective lines and is a direct copy of the established pattern: jsc_hooks.rs:2145-2180 already has the identical if smol { reset() } else { reset_retain_with_limit(8 * 1024 * 1024) } branch for the transpiler-store arena, and the same 8 MiB constant appears at the bundler chunk generator, installer, renamer, bake router, and workspace-map call sites. reset_retain_with_limit returns bool (retained vs recycled), matching the new code's use. The previous code's own comment said this switch was the intended end state pending benchmarking. So correctness risk is low — but it is a behavior change on the synchronous module-transpile hot path that raises steady-state RSS by up to ~8 MiB per VM, and the PR description explicitly lists the before/after RSS+time benchmark as an outstanding action. That trade-off is one a human should sign off on.
Other factors
The unrelated bunx.test.ts CI failure on commit 74a487f is in package-install code, not module loading, and predates the latest fix commit. The new test is thorough (covers retained/recycled/free_all branches differentially, uses require() to force the synchronous reset path, and uses oversized parse-error fixtures to reach the over-cap recycle branch). Given the hot-path memory-policy nature plus the self-noted outstanding benchmark, deferring to a human reviewer rather than auto-approving.
…mmediately in destructOnExit The bounded 8 MiB transpile-arena retention legitimately raises RSS in inspect-error-leak.test.js (CI measured 11 MB growth on the glibc x64 lanes), so restore the 20 MB non-ASAN threshold together with its explanatory comment. Zig__GlobalObject__destructOnExit now sets a termination request before the final collectNow(), mirroring WebWorker__teardownJSCVM. Without it, NapiEnv::mustDeferFinalizers() returns true during that last sweep, so every napi finalizer is enqueued as a NapiFinalizerTask on an event loop that never ticks again; LSan reports each never-drained task allocation as a direct leak (32 B per live napi_wrap), which aborted the inner subprocess of test/regression/issue/30205.test.ts (exit 134) on the x64-asan lane. Reproduced with the CI release-asan artifacts: a control binary built from the merge base without the arena change fails at the same rate with the identical leak stack, so the leak pre-dates this PR; the termination request makes the finalizers run inline during the final GC, matching the worker-teardown path.
Collaborator
Author
|
Two CI fixes pushed in e3719e8: inspect-error-leak.test.js threshold (re: the earlier review nit calling the 10→20 MB raise unjustified): the raise is required, not a leak. 30205.test.ts exit-134 on x64-asan: not caused by this PR — and now fixed at the root. The inner |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/resolve/load-same-js-file-a-lot.test.ts`:
- Around line 107-125: The test lacks meaningful assertions for release builds,
so non-debug runs do not actually validate the arena reset-policy regression
fix. The else block starting at line 123 only checks for empty stderr and zero
exitCode, which the pre-fix behavior would still satisfy. Either add a skip or
failure condition at the test level when isDebug is false to make this
explicitly debug-only instrumentation, or add release-observable invariant
checks within the else block that distinguish between the retain versus reset
arena policies, ensuring that USE_SYSTEM_BUN=1 builds would fail if the arena
policy behavior regresses.
🪄 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: e25c8e38-ab79-458f-aad0-ee2bda929a6a
📒 Files selected for processing (4)
src/jsc/ModuleLoader.rssrc/jsc/bindings/ZigGlobalObject.cpptest/js/bun/resolve/load-same-js-file-a-lot.test.tstest/js/bun/util/inspect-error-leak.test.js
Comment on lines
+107
to
+125
| expect(stdout).toContain(`total=${iters + 149999} caught=${brokenCount}`); | ||
| if (isDebug) { | ||
| const logs = stdout + stderr; | ||
| const occurrences = (needle: string) => logs.split(needle).length - 1; | ||
| if (smol) { | ||
| expect(occurrences("reset_arena: free_all")).toBeGreaterThanOrEqual(iters + brokenCount); | ||
| expect(occurrences("reset_arena: retained")).toBe(0); | ||
| expect(occurrences("reset_arena: recycled")).toBe(0); | ||
| } else { | ||
| // Each oversized parse failure must trip the over-cap recycle; if the | ||
| // broken fixture stops clearing the cap, this fails rather than | ||
| // silently losing branch coverage. | ||
| expect(occurrences("reset_arena: retained")).toBeGreaterThanOrEqual(iters); | ||
| expect(occurrences("reset_arena: recycled")).toBeGreaterThanOrEqual(brokenCount); | ||
| expect(occurrences("reset_arena: free_all")).toBe(0); | ||
| } | ||
| } else { | ||
| expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); | ||
| } |
Contributor
There was a problem hiding this comment.
Non-debug runs don’t actually assert the reset-policy regression.
Outside debug builds this only checks the happy-path totals plus { stderr: "", exitCode: 0 }. The pre-fix behavior would still satisfy those assertions, so USE_SYSTEM_BUN=1 can go green without exercising the retain-vs-reset change. Either make this explicitly debug-only instrumentation coverage, or add a release-observable invariant that distinguishes the two arena policies.
As per coding guidelines, "Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - your test is invalid if it passes with USE_SYSTEM_BUN=1".
🤖 Prompt for 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.
In `@test/js/bun/resolve/load-same-js-file-a-lot.test.ts` around lines 107 - 125,
The test lacks meaningful assertions for release builds, so non-debug runs do
not actually validate the arena reset-policy regression fix. The else block
starting at line 123 only checks for empty stderr and zero exitCode, which the
pre-fix behavior would still satisfy. Either add a skip or failure condition at
the test level when isDebug is false to make this explicitly debug-only
instrumentation, or add release-observable invariant checks within the else
block that distinguish between the retain versus reset arena policies, ensuring
that USE_SYSTEM_BUN=1 builds would fail if the arena policy behavior regresses.
Collaborator
Author
There was a problem hiding this comment.
This is intentional: the branch taken (free_all vs retained/recycled) is only observable via the BUN_DEBUG_ModuleLoader scoped logger, which is compiled out of release builds — there is nothing for a release binary to assert beyond clean output and exit 0, so the non-debug branch is a smoke test. The arena-policy assertions run on every debug lane in CI.
Contributor
There was a problem hiding this comment.
@Jarred-Sumner, understood — since BUN_DEBUG_ModuleLoader and the scoped reset_arena log lines are compiled out of release builds entirely, there is no release-observable signal to distinguish the two arena policies, making the non-debug block a correct smoke test by design. The policy correctness is covered by debug CI lanes. Makes sense, I'll note this for future reviews.
✏️ Learnings added
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31855
File: test/js/bun/resolve/load-same-js-file-a-lot.test.ts:107-125
Timestamp: 2026-06-05T07:21:39.156Z
Learning: In oven-sh/bun, `BUN_DEBUG_ModuleLoader` (and other `BUN_DEBUG_<Tag>` scoped loggers) are compiled out of release builds entirely. Tests that assert arena-policy branch coverage (e.g., `reset_arena: free_all`, `reset_arena: retained`, `reset_arena: recycled` log lines) using these loggers are correctly gated on `isDebug`. The non-debug (`else`) branch of such tests is intentionally a smoke test (assert clean stdout output + `{ stderr: "", exitCode: 0 }`); there is nothing release-observable that distinguishes the two arena policies. Do NOT flag the non-debug branch as "insufficient" or require a `USE_SYSTEM_BUN=1`-failing invariant for this class of tests — the policy assertions run on every debug lane in CI. Applies to: `test/js/bun/resolve/load-same-js-file-a-lot.test.ts` and similar tests that gate branch-coverage assertions on `isDebug`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 31817
File: test/js/bun/net/socket.test.ts:1589-1594
Timestamp: 2026-06-04T22:27:41.981Z
Learning: In `test/js/bun/net/socket.test.ts` (oven-sh/bun), subprocess-based abort-regression tests (e.g., "Bun.listen with an invalid socket handler throws ERR_INVALID_ARG_TYPE instead of aborting") intentionally do NOT assert `expect(stderr).toBe("")`. The no-abort contract is fully encoded by `expect(stdout).toBe(<expected lines>)` and `expect(exitCode).toBe(0)`: a reintroduced abort produces a non-zero exit and stdout that does not match the expected error lines. Debug and ASAN builds may write benign diagnostics to stderr, so the established pattern in this file is to destructure-and-discard stderr with `void stderr` rather than asserting it empty. Do NOT flag this omission as a missing assertion.
Learnt from: robobun
Repo: oven-sh/bun PR: 31785
File: test/bundler/bundler_react_compiler.test.ts:42-68
Timestamp: 2026-06-04T02:01:29.478Z
Learning: In oven-sh/bun bundler test files under `test/bundler/` (e.g., `bundler_feature_flag.test.ts`, `bundler_react_compiler.test.ts`), the conventional way to iterate CLI/API backends is a plain `for (const backend of ["cli", "api"] as const)` loop inside `describe("bundler", ...)`. Do not recommend converting this to `describe.each()` or `test.each()` in these files — the flat `for...of` loop is the intentional, established convention for backend matrices in the bundler test suite.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - your test is invalid if it passes with USE_SYSTEM_BUN=1
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/internal/repl/await.js:71-86
Timestamp: 2026-06-04T23:12:09.051Z
Learning: In oven-sh/bun PR `#31827`, `src/js/internal/repl/await.js` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/repl/await.js`. The `registerVariableDeclarationIdentifiers` function inside `processTopLevelAwait` does not handle null elements (array elisions), `RestElement`, or `AssignmentPattern` nodes — but this is an upstream Node.js bug (verified on Node v26.3.0: `processTopLevelAwait("let [,,x] = await a;")` throws "Cannot read properties of null (reading 'type')"). Rest and default cases work correctly in both. Do NOT suggest patching this function in Bun; any fix must go to nodejs/node first. The file is kept byte-close to upstream to enable clean future syncs.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.test.{ts,tsx} : Tests must be hermetic and leave no resources behind - use `using`/`await using` or try/finally for cleanup registered before assertions
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31825
File: test/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs:18-20
Timestamp: 2026-06-05T01:57:55.903Z
Learning: In oven-sh/bun, `test/js/node/test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs` is a verbatim byte-identical copy of Node.js v26.3.0 `test/parallel/test-fs-cp-async-dereference-force-false-silent-fail.mjs`. The upstream `cp()` call intentionally omits `force: false` even though the filename and comment mention it — this is an upstream inconsistency. Do NOT suggest adding `force: false` to this test; any fix must go upstream to nodejs/node first. The file is kept unmodified to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/from.ts:289-310
Timestamp: 2026-06-05T02:31:36.078Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/from.ts` is a verbatim line-for-line port of Node.js v26.3.0 `lib/internal/streams/iter/from.js`. In `normalizeAsyncSource`, the async-iterable branch (corresponding to Node upstream lines ~337-362) intentionally yields pre-batched `Uint8Array[]` arrays as-is and accumulates normalized chunks without `FROM_BATCH_SIZE` chunking — only the sync paths (Node upstream lines ~210-252) apply `FROM_BATCH_SIZE` sub-slicing. Do NOT flag the async branch's lack of `FROM_BATCH_SIZE` bounding as a bug; any fix must go to nodejs/node first. The file is kept verbatim to enable clean future syncs.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.test.{ts,tsx} : For multi-file tests, prefer `tempDir` with `Bun.spawn` over single-file `-e` tests
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to test/bundler/**/*.test.{ts,tsx} : Use `itBundled` helper for bundler and transpiler tests
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31826
File: src/js/internal/streams/iter/broadcast.ts:146-147
Timestamp: 2026-06-05T02:31:21.560Z
Learning: In oven-sh/bun PR `#31826`, `src/js/internal/streams/iter/broadcast.ts` is a verbatim byte-close port of Node.js v26.3.0 `lib/internal/streams/iter/broadcast.js`. The `#error` field is intentionally initialized to `null` (not `undefined`), and all checks such as `if (self.#error)` and `if (this.#ended || this.#error)` are truthy checks that match upstream lines 90, 182, 199, and 320 exactly. As a consequence, falsy cancellation reasons (e.g., `cancel(0)`, `cancel("")`, `cancel(false)`) behave identically in Node — they do NOT trigger rejection and instead resolve as `{ done: true }`. The vendored upstream tests assert this behavior. Do NOT suggest changing these to `!== undefined` / `!== null` checks in Bun; any fix must go to nodejs/node first. The file is kept byte-close to enable clean future syncs.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: src/js/node/inspector.ts:106-113
Timestamp: 2026-06-05T01:04:19.604Z
Learning: In oven-sh/bun (PR `#31823`), `Runtime.consoleAPICalled` notifications emitted by the in-process `inspector.Session` intentionally omit `params.stackTrace`. The only upstream test that dereferences `notification.params.stackTrace.callFrames[0]` is `test-inspector-console-top-frame.js`, which is guarded by `common.skipIfInspectorDisabled()`. Because this PR sets `process.features.inspector = false`, that test is always skipped when running under Bun and never exercises the in-process session. Synthesizing CDP-shaped `CallFrame` objects from JS is non-trivial (no reliable `scriptId` mapping), so `stackTrace` is deferred until a real consumer requires it. Do not flag the absence of `params.stackTrace` in the in-process `Runtime.consoleAPICalled` payload as a bug.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31823
File: test/js/node/test/parallel/test-inspector.js:315-315
Timestamp: 2026-06-04T22:08:18.155Z
Learning: In oven-sh/bun, `test/js/node/test/parallel/test-inspector.js` is a verbatim byte-identical sync from upstream Node.js (v26.3.0 `test/parallel/test-inspector.js`). Do not suggest modifications to this file—including fixing apparent bugs like the duplicate `${expectedExitCode}` placeholder on line ~315 (should be `${exitCode}`) that originates in the upstream source. Any fixes must go to nodejs/node first. The file is kept unmodified to enable clean diffable future syncs.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/packages/bun-types/**/*.d.ts : TypeScript type declarations in `packages/bun-types/**/*.d.ts` do not require a debug build - run tests directly with system Bun
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31825
File: test/js/node/test/common/fs.js:24-24
Timestamp: 2026-06-05T01:57:45.487Z
Learning: In oven-sh/bun, `test/js/node/test/common/fs.js` is a verbatim copy of Node.js v26.3.0's `test/common/fs.js`. Do not suggest modifications to this file — including the apparent bug on line ~24 where `entry2.name` is dereferenced in an `assert()` message before `entry2` existence is confirmed (should logically be `entry1.name`). This matches the upstream source exactly and is kept identical to enable clean diffable future syncs. Any fixes must go to nodejs/node first.
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/codegen/generate-classes.ts:659-663
Timestamp: 2026-06-05T03:54:16.251Z
Learning: In oven-sh/bun PR `#31822`, `src/codegen/generate-classes.ts` is intentionally performing a mechanical 1:1 rename from `Zig::GlobalObject` to `Bun::GlobalObject` in generated C++ code. The generated constructor `call()` template already used `reinterpret_cast<Zig::GlobalObject*>(lexicalGlobalObject)` before this PR, while `construct()` already used `defaultGlobalObject()`. Do not flag the `call()` path's continued `reinterpret_cast<Bun::GlobalObject*>` as a PR `#31822` regression; changing it to `defaultGlobalObject()` is a behavioral change that belongs in a focused follow-up.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-05T00:36:36.107Z
Learning: Applies to **/*.test.{ts,tsx} : Use `bunEnv` with spread operator when modifying environment variables in tests - never mutate the shared object directly
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/codegen/generate-classes.ts:773-775
Timestamp: 2026-06-05T03:54:14.779Z
Learning: In oven-sh/bun PR `#31822`, `src/codegen/generate-classes.ts` generated C++ thunks such as `${typeName}__getConstructor` and `${typeName}__create*` already used `Zig::GlobalObject*` before the rename. Changing those generated C++ signatures to `Bun::GlobalObject*` is a mechanical namespace/type-spelling rename, not an ABI narrowing from `JSC::JSGlobalObject*`. Do not flag this specific Zig::GlobalObject* → Bun::GlobalObject* change as a generic-global caller mismatch solely because the Zig/Rust imports use `JSGlobalObject*`.
Learnt from: robobun
Repo: oven-sh/bun PR: 31822
File: src/jsc/bindings/BunProcess.cpp:391-394
Timestamp: 2026-06-05T03:54:21.949Z
Learning: In oven-sh/bun rename-only PRs that mechanically migrate JSC binding identifiers from `Zig::GlobalObject` to `Bun::GlobalObject`, do not flag pre-existing direct downcasts such as `static_cast<Zig::GlobalObject*>` becoming `static_cast<Bun::GlobalObject*>` as requiring `defaultGlobalObject()` or node:vm hardening. Treat those behavior-preserving casts as out of scope unless the PR changes their behavior or touches an explicit TODO requiring hardening.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31783
File: src/crash_handler/lib.rs:932-934
Timestamp: 2026-06-05T01:49:48.445Z
Learning: In oven-sh/bun `src/crash_handler/lib.rs`, the unsupported-libuv crash banner is intentionally gated on the process-global `bun_analytics::features::unsupported_uv_function` counter while the function name is read from the thread-local `UNSUPPORTED_UV_FUNCTION` and falls back to `"<unknown>"`. This matches the original `crash_handler.zig` behavior (around lines 273-274). The previous thread-local-only gate was only a `TODO(port)` stand-in, and the cross-thread `"<unknown>"` banner is a known accepted quirk. Do not flag this global-counter gate as a bug.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 31827
File: src/js/node/repl.js:1297-1307
Timestamp: 2026-06-04T23:12:14.305Z
Learning: In oven-sh/bun, `src/js/node/repl.js` is a verbatim port of Node.js v26.3.0 `lib/repl.js`. Do not suggest fixes for bugs that originate in the upstream source (e.g., the `dw.length - up.length` NaN depth bug in `_memory()` around line 1300-1302 of the ported file). The vendored REPL tests assert upstream behavior, so patching divergences would break those tests. Any fixes must go upstream to nodejs/node first.
Learnt from: robobun
Repo: oven-sh/bun PR: 27056
File: test/bundler/standalone.test.ts:281-324
Timestamp: 2026-02-16T04:26:25.185Z
Learning: In Bun test files that exercise Bun.build(), assertions for configuration-validation errors thrown synchronously by JSBundler.fromJS() (via globalThis.throwInvalidArguments()) should use toThrow, e.g., expect(() => Bun.build({...})).toThrow()). Do not use rejects.toThrow() since rejections occur only for asynchronous build errors.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 27385
File: test/js/bun/http/tls-keepalive.test.ts:115-140
Timestamp: 2026-02-24T21:02:00.725Z
Learning: In Bun's test suites, avoid adding tests for trivial environment/fixture script validation (e.g., checking if env vars exist) within test fixtures. Focus test coverage on actual behavior being tested (e.g., TLS keepalive, memory leaks) rather than auxiliary fixture validation. If a test file is primarily for fixtures, skip or limit tests that validate simple JS behavior like if (!env) throw; prioritize meaningful end-to-end or unit behavior instead.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:21:42.189Z
Learning: In Bun's test runner, describe.concurrent is supported (since Bun v1.2.23). Use describe.concurrent/test.concurrent for concurrent tests. Be aware of limitations: expect.assertions() and expect.hasAssertions() are not supported; toMatchSnapshot() is not supported (toMatchInlineSnapshot() is); and beforeAll/afterAll hooks are not executed concurrently. The broader guideline to prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent remains valid and should be applied to test files such as test/cli/run/concurrency-filter.test.ts and similar test files.
Learnt from: LawoodDev
Repo: oven-sh/bun PR: 27855
File: test/cli/run/concurrency-filter.test.ts:32-32
Timestamp: 2026-03-06T16:22:55.570Z
Learning: In test/cli/run/concurrency-filter.test.ts and similar test files, timing-sensitive tests that assert on wall-clock elapsed time to verify concurrency behavior (e.g., expect(elapsed).toBeGreaterThan(800)) must remain in a sequential describe block rather than describe.concurrent. Running such tests concurrently can cause CPU contention and skew timing assertions, leading to flaky results. The guideline to prefer describe.concurrent does NOT apply for timing-based correctness verification.
Learnt from: robobun
Repo: oven-sh/bun PR: 28214
File: test/regression/issue/18115.test.ts:1-158
Timestamp: 2026-03-18T15:19:38.407Z
Learning: In Bun test files, when a resource like tempDir is a DisposableString implementing both Symbol.dispose (sync) and Symbol.asyncDispose, prefer plain using over await using. Do not recommend converting to await using for tempDir in Bun test files. This keeps tests idiomatic and avoids unnecessary async disposal. If a resource only supports asyncDispose, use await using.
Learnt from: robobun
Repo: oven-sh/bun PR: 28425
File: test/regression/issue/28422.test.ts:65-79
Timestamp: 2026-03-22T10:12:05.719Z
Learning: In oven-sh/bun test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}, follow CLAUDE.md by asserting the command exit code LAST—after all other assertions such as stdout/stderr checks and filesystem validation. Do not assert exitCode earlier than those checks. Also, avoid asserting stdout for commands like bun install whose output can vary between runs.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 28863
File: scripts/build/deps/webkit.ts:149-161
Timestamp: 2026-04-04T19:43:49.607Z
Learning: When reviewing Node/TypeScript code that uses `node:path.join()`, do not treat a later path segment that starts with `/` as a Windows/absolute-path override bug. `path.join()` concatenates segments and normalizes; it only resets the root when using `path.resolve()` (e.g., when it encounters an absolute-looking segment). Therefore, patterns like `join(base, "/relPath")` or `join(homedir(), env.slice(1))` where `env.slice(1)` becomes `"/WebKit"` are expected to produce `base/relPath` (cross-platform). Only flag cases where `path.resolve()` (or other root-resetting logic) is used in a way that could unintentionally ignore the base path.
Learnt from: robobun
Repo: oven-sh/bun PR: 28923
File: test/regression/issue/28921.test.ts:0-0
Timestamp: 2026-04-06T19:19:08.790Z
Learning: In oven-sh/bun tests, prefer `tempDir` (from the `harness` module) over `tempDirWithFiles` when using the `using` statement for automatic cleanup. `tempDirWithFiles(...)` returns a plain `string`, so `using tempDirWithFiles(...)` is effectively a no-op and will not trigger disposal/cleanup. `tempDir` returns a `DisposableString` that implements `Symbol.dispose`, so it will correctly trigger cleanup on scope exit.
Learnt from: robobun
Repo: oven-sh/bun PR: 29050
File: test/regression/issue/29042.test.ts:60-94
Timestamp: 2026-04-08T21:22:00.840Z
Learning: In this repo’s Bun environment, `Bun.RedisClient` does not implement `Symbol.dispose` or `Symbol.asyncDispose`, so you cannot rely on `using` / `await using` for automatic cleanup. When creating a `Bun.RedisClient` in tests, close it explicitly with `try/finally`, calling `client.close()` in the `finally` block.
Learnt from: robobun
Repo: oven-sh/bun PR: 29322
File: test/js/web/workers/worker-terminate-after-exit.test.ts:38-43
Timestamp: 2026-04-15T01:57:52.469Z
Learning: In oven-sh/bun test files (matching `test/**/*.test.ts`), when you spawn a subprocess in a bun:test and you assert on its exit code, follow the CLAUDE.md house style: write `if (exitCode !== 0) { expect(stderr).toBe(""); }` immediately before `expect(exitCode).toBe(0)`. This is intentional so that, on failure, bun:test surfaces the full `stderr` content in the diff output. Do not replace this with a custom/second assertion that formats stderr into the exit-code expectation (e.g., `expect(exitCode, \\`stderr: ${stderr}\\`).toBe(0)` or any single-assertion equivalent).
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: test/js/bun/util/v8-heap-snapshot-large-strings.test.ts:4-152
Timestamp: 2026-04-17T02:55:14.338Z
Learning: In oven-sh/bun, do not enforce the `test/regression/issue/${issueNumber}.test.ts` placement rule based solely on PR descriptions that include a speculative GitHub issue link like “might fix `#NNNNN`” without a confirmed regression (e.g., no verifying stack trace/reproduction). If the issue is not confirmed per CLAUDE.md (“confirmed numbered issue” only), the test should be placed next to the closest related existing test file for the affected feature/module (e.g., alongside `test/js/bun/util/v8-heap-snapshot.test.ts`) and should not be flagged as a guideline violation. Likewise, tests that validate a broader behavioral invariant (e.g., V8-matching 1024-char string truncation in heap snapshots) are not purely issue regressions and should live with the feature’s existing test suite rather than under `test/regression/issue/`.
Learnt from: robobun
Repo: oven-sh/bun PR: 29426
File: test/js/node/tls/node-tls-root-certs-concurrent-init.test.ts:80-82
Timestamp: 2026-04-18T00:50:38.905Z
Learning: In oven-sh/bun Jest/Bun test files under `test/js/` that spawn subprocesses using `bunEnv` from the `harness` module, it’s safe and intentional to assert `expect(stderr).toBe("")` unconditionally. `bunEnv` sets `BUN_DEBUG_QUIET_LOGS=1`, which suppresses ASAN/debug-build stderr noise, so an unexpected stderr value should fail the test and show useful diagnostics. Do not gate `expect(stderr).toBe("")` behind `if (exitCode !== 0)` for these `bunEnv`-based subprocess tests—follow the established pattern used in similar tests (e.g., `test/js/node/tls/test-use-system-ca.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29450
File: test/js/bun/resolve/bun-main-entry-point.test.ts:0-0
Timestamp: 2026-04-18T13:16:42.650Z
Learning: In oven-sh/bun tests under `test/js/bun/resolve/` and `test/cli/hot/`, for `--hot` subprocess cases where the subprocess is intentionally terminated via `proc.kill()` (SIGTERM), do not assert the numeric exit code after `await proc.exited`. Treat the exit code as platform-dependent (e.g., SIGTERM may yield `null`/no stable code) and as non-indicative of correctness. Instead, rely on the regression guard that asserts stdout content (e.g., `waitForLine("GEN 4\n")`); failures like stale-slice/use-after-free should manifest as early stdout closure that makes `waitForLine` throw. This matches the approach used in `test/cli/hot/hot.test.ts`.
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:80-82
Timestamp: 2026-04-21T09:47:19.303Z
Learning: In Bun JavaScript/TS tests under `test/js/bun/**` that run runtime subprocesses by spawning `bunExe()` with `bunEnv`, do not add strict `expect(stderr).toBe("")` assertions. In debug ASAN builds, stderr will include `WARNING: ASAN interferes with JSC signal handlers…` on every JS-process launch and it is not suppressed by `bunEnv` / `BUN_DEBUG_QUIET_LOGS=1`. Use the regression guards that are already effective for this area: assert an exact match on `stdout` and `expect(exitCode).toBe(0)`. If you must validate stderr, follow the repo’s filter-based convention: ignore/filter out lines starting with `"WARNING: ASAN interferes"`. If stdout + exitCode provide sufficient coverage, leaving stderr unchecked is acceptable.
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:133-142
Timestamp: 2026-04-21T09:54:56.748Z
Learning: When testing `bun build` subprocesses in `test/js/bun/**/*.test.ts`, it is acceptable to assert `expect(stderr).toBe("")` (or otherwise expect no stderr noise). `bun build` is compiler-only and does not start a JS VM, so it should not emit the ASAN warning about interfering with JSC signal handlers. Only JS-executing subprocesses (e.g., `bun -e`, running built output like `bun out.js`) are expected to produce that warning, so do not treat empty-stderr assertions as brittle specifically for `bun build` in these tests.
Learnt from: robobun
Repo: oven-sh/bun PR: 29564
File: test/regression/issue/29513.test.ts:51-51
Timestamp: 2026-04-22T02:58:30.645Z
Learning: In oven-sh/bun TypeScript test files, it is acceptable to use `Bun.sleep(0)` specifically as a macrotask barrier to deterministically drain the pending microtask queue before asserting. Do NOT flag `Bun.sleep(0)` as a timing-wait violation. The “do not use setTimeout/Bun.sleep in tests” guideline is intended to prevent load-sensitive wall-clock delays (e.g., `Bun.sleep(100)` or other timing windows). Use `Bun.sleep(0)` only when you need to observe a fully settled Promise/microtask chain (e.g., after deferred resolution and multiple internal `.then()` hops) where a single `await Promise.resolve()` would not advance far enough; `Bun.sleep(0)` resumes in a later macrotask after pending microtasks complete, without relying on elapsed time.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:10.896Z
Learning: In oven-sh/bun code reviews, do not recommend adding standalone regression tests that depend on setting `BUN_JSC_validateExceptionChecks=1` to exercise JSC throw-scope/exception-scope validator paths (e.g., PropertyCallback/reify interactions like `reifyAllStaticProperties`). Per `CLAUDE.md`, tests are expected to pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. Instead, treat this class of validator coverage issue as covered by: (1) the x64-asan CI shard that enables the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that hit pre-existing throw-scope assertion failures unrelated to the change under review. If helpful, add an in-source comment pointing to the specific existing exerciser (e.g., the relevant `tsgo/bun-types` test) to document the intent without relying on the env var.
Learnt from: robobun
Repo: oven-sh/bun PR: 29656
File: test/js/bun/s3/s3-path-double-free.test.ts:49-61
Timestamp: 2026-04-23T23:39:21.333Z
Learning: In Bun test files under `test/js/bun/**/*.test.ts`, prefer `test.each()` over `describe.each()` when each parameter value results in a single `test`/`it` assertion. Using `describe.each()` to wrap a single `test` adds unnecessary nesting. Only use `describe.each()` when you need multiple `test`/`it` blocks per parameter value.
Learnt from: robobun
Repo: oven-sh/bun PR: 29820
File: test/js/node/process/process-execve.test.ts:47-52
Timestamp: 2026-04-28T11:35:58.257Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when a test uses the `tempDir` fixture and spawns a subprocess via `await using proc = Bun.spawn(...)` (i.e., the embedded script runs as a spawned subprocess), do not recommend adding a fixture-level or embedded-script `setTimeout` watchdog to prevent hangs. The `await using` scope exit should terminate the subprocess automatically, and Bun test per-test timeouts already bound execution time. Also, avoid embedded `setTimeout` watchdog patterns that violate Bun’s “no setTimeout in tests” guideline. If the worker/subprocess exits silently without posting, rely on the test’s stdout/exitCode assertions plus Bun’s outer timeout rather than a watchdog, even when the embedded fixture script uses `worker_threads` or other async constructs.
Learnt from: robobun
Repo: oven-sh/bun PR: 29874
File: test/js/web/websocket/websocket-proxy-tunnel-upgrade-leak.test.ts:15-16
Timestamp: 2026-04-28T21:34:23.491Z
Learning: In oven-sh/bun, when a test is intentionally validating native refcount leak detection using Bun debug-only instrumentation (e.g., `BUN_DEBUG_alloc=1` and `[alloc] new(...)/destroy(...)` log lines produced only by debug builds when `Environment.enable_logs` is set), use `test.skipIf(!isDebug)` as the correct/intentional guard. Do not flag this `test.skipIf(!isDebug)` as a guideline violation for this class of tests. The debug-only `[alloc] ...` lines are absent in release and ASAN builds, and there is no equivalent observable system-Bun hook to assert a leak when only debug-build instrumentation exists (so the `USE_SYSTEM_BUN=1` rule in `CLAUDE.md` does not apply in this situation).
Learnt from: robobun
Repo: oven-sh/bun PR: 29876
File: test/js/bun/ffi/cc.test.ts:0-0
Timestamp: 2026-04-29T00:09:18.937Z
Learning: In oven-sh/bun tests, when using the `harness` module’s `tempDir`, prefer the overload that accepts an optional second argument: `tempDir(prefix, fileTree)`, where `fileTree` is an object in the same shape as `tempDirWithFiles` (e.g., `{ "file.c": "..." }`). This creates a disposable temp directory pre-populated with files. If the `tempDir` file-tree overload is available, don’t recommend a separate manual `fs.writeFile`/write step for pre-populating files (e.g., when using `using dir = tempDir("prefix", { ... })`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29876
File: test/js/bun/ffi/cc.test.ts:205-231
Timestamp: 2026-04-29T00:24:38.784Z
Learning: In oven-sh/bun’s Bun test files under test/js/bun/, do not treat explicit per-test timeouts as a guideline violation when the test is an RSS-leak regression that spawns a subprocess and performs many iterations (subprocess-heavy leak tests). For these cases, Bun’s default per-test timeout (5s locally) is insufficient—especially under debug+ASAN where these tests may take ~5–14s—so reviewers should expect and accept an explicit, larger per-test timeout (e.g., 60_000). Concretely, tests like the cc() option-string leak test (test/js/bun/ffi/cc.test.ts) and glob-leak tests (e.g., test/js/bun/glob/leak.test.ts) should be reviewed as exceptions: allow explicit timeouts when the intent is to cover RSS-leak/subprocess-heavy regression workloads.
Learnt from: robobun
Repo: oven-sh/bun PR: 29919
File: test/js/bun/util/filesystem_router.test.ts:613-628
Timestamp: 2026-05-02T00:35:55.819Z
Learning: In oven-sh/bun tests under test/js/bun/**, prefer strict stderr assertions like `expect(stderr).toBe("")` for subprocesses spawned with `bunExe()` when you pass a `bunEnv` that already propagates `ASAN_OPTIONS=allow_user_segv_handler=1` from the parent `bun bd` build environment (this suppresses the `WARNING: ASAN interferes with JSC signal handlers` message). On CI ASAN lanes where `isASAN` is true, `bunEnv` sets `isASAN` explicitly as well—so strict stderr expectations are still safe. Only relax/skip strict stderr assertions (e.g., avoid `toBe("")`) when `ASAN_OPTIONS=allow_user_segv_handler=1` is *not* propagated into the subprocess environment.
Learnt from: robobun
Repo: oven-sh/bun PR: 30115
File: test/js/bun/glob/scan.test.ts:877-882
Timestamp: 2026-05-02T17:49:10.214Z
Learning: In oven-sh/bun regression tests for UAFs tied to Bun’s threadpool/event-loop interaction (e.g., WalkTask pending activity), keep the intended repro timing: use `Bun.sleepSync(N)` inside a spawned subprocess to hold the JS event loop without yielding/draining pending tasks, then trigger `Bun.gc(true)` (after the threadpool task has been given time to complete `run()`), and finally drive the result with the corresponding `for await`/iterator consumption to make the UAF observable. Do not replace `Bun.sleepSync(N)` with `await Bun.sleep(0)` or any other event-loop-yielding construct, since it can drain pending concurrent tasks and cause callbacks/`then()` work to run before the GC call, making the bug unobservable. This “sleepSync → gc(true) → for await” sequence is the correct 3-step UAF repro pattern for this bug class.
Learnt from: robobun
Repo: oven-sh/bun PR: 30142
File: test/js/bun/http/bun-serve-html-abort-leak-fixture.ts:28-38
Timestamp: 2026-05-03T01:29:10.031Z
Learning: In oven-sh/bun tests/fixtures that spawn subprocesses with `BUN_DEBUG_alloc` (or `BUN_DEBUG_ALL`) set to a non-zero value (e.g., `"1"`), the `[alloc]` log scope is effectively enabled at runtime for all `bun.new`/`bun.destroy`-allocated types. Because the runtime check in `src/output.zig` forces `really_disable = false` when `BUN_DEBUG_<tagname>` is not `"0"`, such fixtures may emit `[alloc] new(T)` / `[alloc] destroy(T)` lines even when `T` does not declare `log_allocations = true`. In this context, do not flag missing `log_allocations` declarations as a bug in the test fixture or the involved fixture types.
Learnt from: robobun
Repo: oven-sh/bun PR: 30153
File: test/bundler/plugin-sync-exception-fallback.test.ts:75-91
Timestamp: 2026-05-03T01:53:50.441Z
Learning: In this repo’s Bun test files that use `Bun.spawn`, don’t “parse/assert stdout before checking `exitCode`” when the expected failure mode is a crash (e.g., SIGSEGV or UBSan abort) that may produce empty stdout. Parsing/validating empty stdout first can mask the more useful signal/stderr. Instead, assert the spawned-process result by including `stdout` in the object passed to `toMatchObject` alongside `exitCode`, `signalCode`, and `stderr`, so stdout/stderr/signal all appear together in the failure diff (same pattern as `test/bundler/plugin-error-nested-throw.test.ts`).
Learnt from: robobun
Repo: oven-sh/bun PR: 29922
File: test/js/bun/resolve/bust-dir-cache-leak.test.ts:1-72
Timestamp: 2026-05-03T07:04:37.720Z
Learning: In oven-sh/bun, when reviewing Jest/Bun test additions under test/js/bun/resolve/, apply the rule “add new tests to an existing test file” only if an existing test file in the same target directory already covers the same feature area.
If no existing test file covers the specific feature being tested (e.g., the lifecycle of resolver-cache/DirInfo/BSSMap cache slots), it is acceptable and preferred to create a new dedicated test file for discoverability (e.g., bust-dir-cache-leak.test.ts), rather than forcing the tests into an unrelated file.
Do not flag the creation of a new *.test.ts file in test/js/bun/resolve/ as a violation when the feature under test is not already covered by another existing test file in that directory.
Learnt from: robobun
Repo: oven-sh/bun PR: 30245
File: test/regression/issue/19650.test.ts:9-30
Timestamp: 2026-05-04T20:27:55.527Z
Learning: In oven-sh/bun test files, prefer using flat `test.concurrent.each([...])` when you want every parameterized test case to run fully concurrently across the entire parameter matrix. By contrast, `describe.each([...])` executes its describe blocks sequentially; while tests inside each describe block may be `test.concurrent`, concurrency is limited to within that block rather than across the whole matrix.
Learnt from: robobun
Repo: oven-sh/bun PR: 30118
File: test/js/node/zlib/zlib-writestate-detached.test.ts:78-90
Timestamp: 2026-05-04T20:37:57.348Z
Learning: In this Bun repository, do not flag code in Bun subprocess fixtures/tests where `console.log(...)` (or similar synchronous stdout/stderr writes) is immediately followed by `process.exit(n)` as a potential output-loss problem. Bun’s `process.exit()` flushes stdout and stderr synchronously before exiting (per the implementation in `src/runtime/node/process/exit.zig`), so `console.log` + `process.exit` is considered a safe, established Bun convention.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.796Z
Learning: When reviewing JS/TS regex literals in Bun test files under `test/js/bun/`, don’t flag `\\` or `\b` as “bad escaping” if they’re intentionally matching literal backslashes used in Windows named-pipe paths (e.g., `\\.\pipe\name`). In JS regex literals, `\\` represents two literal backslashes, `\.` matches a literal dot, and `\b` (backslash-backslash-b) means a literal backslash followed by `b`, not the `\b` word-boundary escape.
Learnt from: robobun
Repo: oven-sh/bun PR: 30268
File: test/js/bun/net/named-pipe-listen-error.test.ts:137-137
Timestamp: 2026-05-05T02:16:13.255Z
Learning: When reviewing JavaScript/TypeScript regex literals, treat `\b` as an escaped backslash followed by `b` (i.e., it matches a literal backslash and then `b`), not the regex word-boundary metacharacter. The word-boundary metacharacter is an unescaped `\b` in the source code (i.e., `\b` in the pattern string/literal syntax), which has word-boundary semantics.
So: do not flag `\b` inside a regex as a word-boundary issue by default. Only flag `\b` when the intent is to match a literal backslash+`b` and word-boundary semantics would be incorrect. Example: `/^\\\.\\pipe\\/` (as written) matches the Windows named-pipe prefix `\\.\pipe\`.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:467-495
Timestamp: 2026-05-05T15:02:03.877Z
Learning: In oven-sh/bun test files under `test/**/*.test.ts`, when verifying that a test was NOT executed (for example, it was filtered out by `pathIgnorePatterns`), assert the absence of the test name string (e.g., `expect(stderr).not.toContain("explicit test")`) rather than asserting that the filename is absent. Bun may echo the filename in its `"The following filters did not match any test files:"` error output even when no tests ran, so filename-based assertions can be misleading.
Learnt from: robobun
Repo: oven-sh/bun PR: 30306
File: test/js/web/fetch/blob-write.test.ts:88-96
Timestamp: 2026-05-06T01:36:05.893Z
Learning: TempDir must be invoked with two arguments in test harness code: basename: string and filesOrAbsolutePathToCopyFolderFrom: DirectoryTree | string. Calls like tempDir("foo") should be flagged as invalid. tempDirWithFiles("name", {}) is a permitted pattern in existing tests (e.g., test/js/web/fetch/blob-write.test.ts line 55) when the result is assigned with const (not using) and consistent with the file's conventions. Apply this rule to test files across the repository (oven-sh/bun), and do not flag compliant const-based patterns that follow the established usage.
Learnt from: robobun
Repo: oven-sh/bun PR: 30350
File: test/cli/test/bun-test.test.ts:1319-1324
Timestamp: 2026-05-07T06:52:44.159Z
Learning: In oven-sh/bun TypeScript test files under `test/**/*.test.ts`, when the test constructs the snapshot input by intentionally `.filter()`-ing raw stderr to only the reporter-generated status/output lines (e.g., lines matching `/^\((pass|fail|skip|todo)\)/`, `^ ...` explanation lines, and `AssertionError:` lines), do not require `normalizeBunSnapshot` for that snapshot. In this design, the `.filter()` is what stabilizes the snapshot across `Execution.Result` variants; adding `normalizeBunSnapshot` would unnecessarily retain extra output (stack traces, repeated failures block, summaries), making snapshots ~3x larger and more fragile. Accept the local convention of small ad-hoc `.replace()` regex normalization for volatile timing fragments (e.g., stripping `[{d}ms]` and `after {d}ms` timeout text) where applied consistently within the same test suite.
Learnt from: jgoyvaerts
Repo: oven-sh/bun PR: 30410
File: test/js/bun/http/bun-serve-routes.test.ts:721-745
Timestamp: 2026-05-08T20:24:48.518Z
Learning: For this repo’s Bun/CLI tests under `test/js/bun/**`, follow the rule from `CLAUDE.md`: do not add explicit per-test timeouts (e.g., the 3rd argument to `test()`), including in performance/timing or scaling regression tests. Bun already applies its own timeouts, and adding per-test timeouts will likely interfere with the intended measurement. Only suggest adding explicit timeouts if the target file already uses them and they are explicitly required for correctness. The known exceptions are `test/js/bun/ffi/cc.test.ts` and `test/js/bun/glob/leak.test.ts` (RSS-leak, subprocess-heavy tests where timeouts may be necessary).
Learnt from: robobun
Repo: oven-sh/bun PR: 30414
File: test/js/bun/util/throw-bad-toPrimitive.test.ts:17-17
Timestamp: 2026-05-09T01:26:42.041Z
Learning: In oven-sh/bun test files under test/js/bun/**, enforce `bunExe()` + `-e` only for short inline one-liners (where the subprocess entry point is a single-string expression). If the subprocess entry-point is a fixture file (i.e., the entry point requires module-level `import` declarations and/or references `import.meta.dir`), use the established fixture pattern instead: `[bunExe(), path.join(import.meta.dir, "fixture.ts")]`. Do not flag this fixture pattern as a guideline violation (it matches existing usage across the test suite).
Learnt from: majiayu000
Repo: oven-sh/bun PR: 25687
File: test/bundler/issue-25675.test.ts:1-4
Timestamp: 2026-05-16T17:15:07.036Z
Learning: For Bun bundler tests, if a test file imports or uses `itBundled` / `expectBundled`, it must live under `./test/bundler/` (e.g., `test/bundler/**`). These helpers include a runtime guard that checks the call stack for `test/bundler/` and will throw with “All bundler tests must be placed in ./test/bundler/…”. Do not suggest moving such tests to `test/regression/…`, even for issue-specific/regression cases, because they will fail at runtime.
Learnt from: robobun
Repo: oven-sh/bun PR: 30936
File: test/bundler/transpiler/runtime-transpiler.test.ts:225-225
Timestamp: 2026-05-17T19:03:05.577Z
Learning: This repo (oven-sh/bun) does not enforce Biome lint rules in CI because there is no root Biome config (`biome.json` or `.biome*`). Therefore, during code review do not suggest adding `// biome-ignore` (or similar) suppression comments for Biome rule violations.
Additionally, in test files under `test/bundler/transpiler/`, do not “fix” switch-case code by wrapping intentionally-bare (unwrapped) `const` declarations in `{}` blocks when the test is specifically asserting TDZ/const-inlining behavior across sibling cases (e.g., regression tests like issue `#30932`). Adding a `{}` block can interfere with the const-prefix inliner and the single-use substitution pass, causing the test to miss the intended failure mode.
Learnt from: robobun
Repo: oven-sh/bun PR: 30975
File: test/js/bun/resolve/import-defer.test.ts:36-44
Timestamp: 2026-05-18T06:47:10.280Z
Learning: In oven-sh/bun Jest/Vitest-style test files under test/js/bun/resolve/ (e.g., *.test.ts) that spawn subprocesses using bunEnv, keep an unconditional `expect(stderr).toBe("")` assertion. Place it BEFORE any stdout-related assertions and BEFORE `expect(exitCode).toBe(0)`. Do not change it to a conditional pattern like `if (exitCode !== 0) { expect(stderr).toBe(""); }`—bunEnv sets `BUN_DEBUG_QUIET_LOGS=1` to suppress noisy ASAN/debug output, and the unconditional check helps catch unexpected stderr even when the process exits with code 0.
Learnt from: robobun
Repo: oven-sh/bun PR: 30284
File: test/cli/test/path-ignore-patterns.test.ts:343-375
Timestamp: 2026-05-21T07:56:03.036Z
Learning: In oven-sh/bun test files (Bun test), both `test.each` and `describe.each` are acceptable idioms for parameterized tests. Do not treat `test.each` as a guideline violation in favor of `describe.each`. Use `test.each` when each parameter entry corresponds to a single test body and no nested `test()` blocks are needed; use `describe.each` when you want grouped/structured test suites per parameter set.
Learnt from: robobun
Repo: oven-sh/bun PR: 31201
File: scripts/strip-long-rs-comments.ts:72-74
Timestamp: 2026-05-22T05:32:25.972Z
Learning: In this repo (oven-sh/bun), .gitattributes enforces LF line endings for tracked files, so CR characters from CRLF inputs should not be present. When reviewing TypeScript code that reads text and splits lines (e.g., using `split("\n")`), don’t flag CRLF/"trailing `\r`" concerns as issues, since tracked inputs are expected to contain only `\n` line endings.
Learnt from: robobun
Repo: oven-sh/bun PR: 31270
File: test/js/bun/css/nested-vendor-prefix-duplication.test.ts:113-120
Timestamp: 2026-05-23T14:52:47.580Z
Learning: In Bun/JS test files under `test/js/bun/**`, when a test spawns a subprocess and then reads an output file that the subprocess is supposed to generate, assert the subprocess result (both `exitCode` and `stderr`) together *before* attempting to read the output file. Prefer a combined assertion like `expect({ exitCode, stderr }).toEqual({ exitCode: 0, stderr: "" })` so that failures in `exitCode`/`stderr` surface clearly and don’t get masked by a subsequent “file not found” when the output file was never produced. This is an intentional exception to any general guideline that defers `exitCode` assertions until after filesystem reads.
Learnt from: robobun
Repo: oven-sh/bun PR: 31273
File: test/js/bun/jsonc/jsonc.test.ts:195-195
Timestamp: 2026-05-23T15:10:12.956Z
Learning: In Bun test files under `test/js/bun/**`, avoid adding explicit per-test timeouts except for pathological-input performance regression tests that run a subprocess with a `killSignal: "SIGKILL"` (e.g., tests that validate worst-case/slow inputs under debug+ASAN). For these tests, add an explicit outer test timeout (e.g., `90_000`) that is larger than the subprocess `timeout` option. The subprocess `timeout` is the real hang guard; the outer timeout is only a safety margin to prevent premature failures on slow CI lanes.
Learnt from: robobun
Repo: oven-sh/bun PR: 31514
File: test/js/sql/sqlite-sql.test.ts:5155-5163
Timestamp: 2026-05-28T17:06:44.390Z
Learning: When writing/updating tests that use `bun:sqlite` (oven-sh/bun) to round-trip the Unicode code point `\uFFFE`, account for SQLite’s bind-time behavior: SQLite drops `\uFFFE` during `sqlite3_bind_text16` UTF-16 → UTF-8 conversion, so the stored value becomes zero bytes and reads back as an empty string (`""`). Therefore, tests asserting round-trip behavior of `\uFFFE` should expect `""` (not `"\uFFFE"`). Do not change the expectation or framing to treat `\uFFFE` as preserved or leniently replaced—this is explicitly a SQLite-level drop.
Learnt from: robobun
Repo: oven-sh/bun PR: 31661
File: test/cli/run/env.test.ts:598-598
Timestamp: 2026-06-01T17:43:01.365Z
Learning: In Bun test files, when asserting that a subprocess produced no stderr (e.g., `expect(stderr).toBe("")`), do not add noise-filtering like `.filter(line => !line.startsWith("WARNING: ASAN interferes"))`. After PR `#30412`, Bun subprocesses no longer emit this ASAN startup warning across build variants (debug/ASAN/release), so the plain `toBe("")` assertion is correct for all CI configurations.
Learnt from: robobun
Repo: oven-sh/bun PR: 31661
File: test/cli/run/env.test.ts:598-600
Timestamp: 2026-06-01T17:43:14.469Z
Learning: In Bun test files under `test/**/*.test.ts`, when you spawn a subprocess and expect it to produce **empty stderr**, it’s acceptable to assert stderr unconditionally with `expect(result.stderr.toString('utf8')).toBe('')` before asserting `expect(result.exitCode).toBe(0)`. This avoids checking stderr twice while still showing stderr in the failure diff if stderr is non-empty. Use the conditional pattern (assert stderr only when `result.exitCode !== 0`) when stderr may include known-benign output that is only acceptable under certain failure/special cases (e.g., ASAN startup noise or other stderr exemptions).
Learnt from: robobun
Repo: oven-sh/bun PR: 31694
File: test/js/node/fs/fs-path-length.test.ts:168-170
Timestamp: 2026-06-02T09:34:04.212Z
Learning: In bun:test files, do not flag `expect(async () => await somePromise).toThrow("message")` as incorrect. bun:test’s `.toThrow(...)` supports async functions by inspecting the returned promise; a rejecting async fn with a matching message should pass and a non-matching message should fail. The alternative `await expect(promise).rejects.toThrow(...)` is also valid, but it is not required for bun:test.
Learnt from: EffortlessSteven
Repo: oven-sh/bun PR: 31729
File: test/js/bun/util/arraybuffersink.test.ts:66-123
Timestamp: 2026-06-02T20:41:52.089Z
Learning: For oven-sh/bun tests covering SharedArrayBuffer/resizable-ArrayBuffer snapshot boundary behavior in synchronous “sink” implementations (e.g., `ArrayBufferSink`, and similarly `FileSink` and `ResumableSink`), avoid using concurrency/worker-based mutation after `write()` returns to validate snapshot correctness. Since `ArrayBufferSink.write(chunk)` is fully synchronous (bytes are already copied into the sink buffer before it returns), post-write mutation will pass for both old and new code and does not prove the fix; race-based Worker tests also tend to be timing/Atomics-sensitive and are considered flaky in this repo. Instead, follow the pattern in `test/js/bun/util/arraybuffersink.test.ts`: use guard bytes around the view (e.g., `0xff`) and assert that `sink.end()` output contains only the exact intended view range (no data outside the view), which validates the snapshot boundary without any concurrency.
Learnt from: EffortlessSteven
Repo: oven-sh/bun PR: 31729
File: test/js/bun/s3/s3.test.ts:1805-1870
Timestamp: 2026-06-02T20:42:36.426Z
Learning: For Bun JS tests covering S3/“sink” behavior that copies SharedArrayBuffer/resizable-ArrayBuffer bytes into owned storage before `write()` returns, don’t rely on post-dispatch mutation to prove the UB fix: a post-write mutation will land after the relevant read in both the old (UB) and new (safe snapshot) cases. Instead, write behavior-preserving tests that validate the uploaded view range precisely (e.g., the slice boundaries are exactly correct and no guard/extra bytes leak), demonstrating the snapshot captured the intended slice—without attempting timing-sensitive concurrent Worker mutation races.
Learnt from: EffortlessSteven
Repo: oven-sh/bun PR: 31776
File: test/js/bun/ffi/cc.test.ts:399-427
Timestamp: 2026-06-03T19:45:00.193Z
Learning: In oven-sh/bun Bun/FFI test files, when using a multi-test fixture directory managed by a `beforeAll`/`afterAll` lifecycle (i.e., the temp `dir` is created/assigned in `beforeAll` and removed in `afterAll` and must live across multiple `it` blocks), prefer `tempDirWithFiles(prefix, fileTree)` over `tempDir(prefix, fileTree)`. In this lifecycle, `using`/`Symbol.dispose` automatic-disposal from `tempDir` can’t be relied on because the directory must outlive individual `it` blocks, so using `tempDir` adds no useful behavior and can confuse intent.
Also, do NOT flag `tempDirWithFiles(prefix, fileTree)` as a guideline violation inside these `beforeAll`/`afterAll` blocks—`tempDirWithFiles` is the correct primitive for multi-test fixture directories.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31835
File: test/js/workerd/html-rewriter.test.js:835-835
Timestamp: 2026-06-05T07:13:12.642Z
Learning: In oven-sh/bun test files, follow the `Buffer.alloc(count, fill).toString()` performance guideline only for building large repetitive *binary* buffers (where repeatedly allocating string data via `Buffer`/conversion matters). Do not treat `String.prototype.repeat()` as a violation when it is used solely to create a plain string that is immediately consumed by string-to-bytes APIs such as `TextEncoder.encode()` (or other APIs that accept a string and convert internally). In particular, if `str.repeat(n)` is passed directly to `TextEncoder.encode()` (or a similar string-to-bytes API), it should be considered idiomatic/correct and must not be flagged as a `Buffer.alloc(...).toString()` guideline violation.
…e finalizers Setting a termination request in Zig__GlobalObject__destructOnExit made NapiEnv::mustDeferFinalizers() return false, so napi finalizers ran inline while the heap was sweeping during the final collectNow(). That is exactly what the deferral mechanism exists to prevent: a finalizer that calls back into JS touches cells mid-sweep and trips 'ASSERTION FAILED: vm().currentThreadIsHoldingAPILock() => vm().heap.mutatorState() != MutatorState::Sweeping' (JSC::JSCell::validateIsNotSweeping), failing test/napi/node-napi-tests/test/js-native-api/test_cannot_run_js under BUN_DESTRUCT_VM_ON_EXIT. Revert that line. Fix the leak it was papering over at the right layer instead: deferred NapiFinalizerTasks that a GC enqueued but the event loop never drained are now released in __bun_release_task_at_shutdown (before destructOnExit, env still alive), the existing per-tag shutdown release point. The box is dropped without dispatching the addon callback, matching the policy of NapiFinalizerTask::schedule's shutdown branch. Also reword the inspect-error-leak threshold comment to say why that test is affected by the arena retention policy.
Comment on lines
+1191
to
+1204
| // Deferred napi finalizers enqueued by a GC that the loop never got | ||
| // to drain. Running the addon callback this late is not safe (it may | ||
| // call back into JS), so drop the box without dispatching — same | ||
| // policy as `NapiFinalizerTask::schedule`'s shutdown branch. The drop | ||
| // releases the `Ref<NapiEnv>` while the env is still alive (we run | ||
| // before `destructOnExit`); the addon's external data is reclaimed by | ||
| // the OS at process exit. | ||
| task_tag::NapiFinalizerTask => { | ||
| // SAFETY: `task.ptr` is the `Box<NapiFinalizerTask>` from | ||
| // `NapiFinalizerTask::schedule` (`heap::into_raw`); the loop will | ||
| // never dispatch it, so we hold the sole reference. | ||
| drop(unsafe { bun_core::heap::take(task.ptr.cast::<NapiFinalizerTask>()) }); | ||
| true | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 The 53c583d commit message says the 30205 LSan flake "is now handled by releasing queued NapiFinalizerTasks in __bun_release_task_at_shutdown", but this arm runs at release_queued_tasks_for_shutdown (VirtualMachine.rs:1579) — before destructOnExit's collectNow() (:1581) where your e3719e8 root-cause says the leaked allocations originate, and on the bun test exit path on_exit() is never called so schedule() routes those boxes into rare_data.cleanup_hooks (napi_body.rs:4280-4285), not event_loop.tasks. The arm itself is fine, but it likely doesn't cover the path the reverted setHasTerminationRequest() fix did — worth re-running 30205.test.ts on x64-asan against 53c583d (the robobun results above are for e3719e8).
Extended reasoning...
What changed and what it claims
53c583d reverted the e3719e8 vm.setHasTerminationRequest() line in Zig__GlobalObject__destructOnExit (because it tripped validateIsNotSweeping in test_cannot_run_js) and added this task_tag::NapiFinalizerTask arm to __bun_release_task_at_shutdown, with the commit/PR comment: "The leak is now handled by releasing queued NapiFinalizerTasks in __bun_release_task_at_shutdown instead."
The arm itself is correct and harmless — it drops a Box<NapiFinalizerTask> and releases its NapiEnvRef while the env is still alive, symmetric with the adjacent FetchTasklet/AsyncFSTask arms. The concern is only whether it actually intercepts the allocations LSan flagged in 30205.
Ordering: this arm runs before the leak point
__bun_release_task_at_shutdown is reached only via EventLoop::release_queued_tasks_for_shutdown (event_loop.rs:801), called at VirtualMachine.rs:1579. Zig__GlobalObject__destructOnExit — and its final collectNow() — runs at :1581, after the drain. The author's own e3719e8 root-cause states the leaked NapiFinalizerTask boxes are allocated "in napi_internal_enqueue_finalizer during destructOnExit's final collectNow()". Anything allocated there is after this arm has already walked event_loop.tasks.
Where those boxes actually go on the bun test exit path
30205.test.ts runs under bun test --isolate. The test-runner exit paths (test_command.rs:2942, parallel/runner.rs:712) set vm.is_shutting_down = true and call global_exit() without calling on_exit(), so has_run_cleanup_hooks stays false. During destructOnExit's collectNow(), NapiFinalizerTask::schedule() (napi_body.rs:4266-4289) therefore evaluates:
is_main_thread→ true (sweep runs on the JS thread)vm.is_shutting_down()→ truevm.has_run_cleanup_hooks()→ false
…and takes the push_cleanup_hook branch at :4280-4285 — heap::into_raw(self) is stashed as a raw ctx pointer in rare_data.cleanup_hooks. That list is never walked again (on_exit() never ran), and when rare_data is dropped during teardown the Vec<CleanupHook> storage is freed but the raw ctx pointers are orphaned → LSan direct leak, matching the reported Direct leak of 32000 byte(s) in 1000 object(s). The new arm drains event_loop.tasks; it never touches rare_data.cleanup_hooks.
This also resolves the apparent contradiction the refutation raised: e3719e8's setHasTerminationRequest() worked because it flips mustDeferFinalizers() to false, so finalizers ran inline during the sweep and napi_internal_enqueue_finalizer → schedule() was never reached — a different mechanism that bypassed the push_cleanup_hook path entirely. And it explains why LSan reported a direct leak rather than "reachable via static-rooted VM": the boxes weren't in self.tasks, they were behind raw pointers in a Vec that gets freed.
What about tasks enqueued before shutdown?
For NapiFinalizerTasks that landed in event_loop.tasks via the enqueue_task branch (:4288) before is_shutting_down flipped — the only window the new arm does cover — without this arm they'd return false, get re-queued (event_loop.rs:810-816), and stay in self.tasks, a field of the never-dealloc'd static-rooted VM box. Per the codebase's own comments (event_loop.rs:796-800, 822-827) LSan treats those as reachable and does not flag them. So the arm changes nothing LSan-observable on that path either.
Step-by-step proof for 30205
bun test --isolatefinishes;test_command.rs:2942setsis_shutting_down=trueand callsglobal_exit().on_exit()is not called →has_run_cleanup_hooks=false.global_exit()reaches VirtualMachine.rs:1579:release_queued_tasks_for_shutdown()walksevent_loop.tasks. AnyNapiFinalizerTaskalready there is dropped by the new arm. ✅- VirtualMachine.rs:1581:
Zig__GlobalObject__destructOnExit→collectNow(). GC sweeps 1000NapiRef-backed objects; each callsnapi_internal_enqueue_finalizer→Finalizer::enqueue→NapiFinalizerTask::schedule(). schedule():is_shutting_down=true,has_run_cleanup_hooks=false→heap::into_raw(self)pushed intorare_data.cleanup_hooksas a rawctxpointer.- The new
__bun_release_task_at_shutdownarm already ran in step 2; it never sees these. - Cleanup hooks are never walked again.
rare_datais dropped; the 1000 rawctxpointers are orphaned. LSan:Direct leak of 32000 byte(s) in 1000 object(s)fromnapi_internal_enqueue_finalizer— same stack the author observed.
Impact & suggestion
The 30205 flake is pre-existing (the author confirmed it reproduces on a control binary without the arena change), so this PR doesn't regress anything — hence nit. But the verified-working fix was reverted and the stated replacement operates on a different queue at an earlier point than where the analysis (and the author's own root-cause) places the leak. The robobun results in this PR are for e3719e8, not 53c583d. Worth either re-verifying 30205.test.ts on x64-asan against 53c583d, or — if the push_cleanup_hook path is indeed the culprit — having the bun test exit path set has_run_cleanup_hooks=true (or call on_exit()) so schedule() takes the drop(self) branch at :4276 instead.
This was referenced Sep 17, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 24, 2026
…hs (#43840) ### Problem - `src/codegen/generate-classes.ts` still has the DOMJIT emitter (C++ signatures, `WithoutTypeChecks` wrappers, result-type asserts, Rust thunks). None of it can run: `define()` in `class-definitions.ts` has set `DOMJIT = undefined` on each field since #14005 (2024-09), and all 31 `*.classes.ts` files go through `define()`. - Nothing is left for it to bind: #35002 deleted each Rust `*_without_type_checks` fast path, and #36756 and #36903 removed the C++ leftovers. - A `DOMJIT:` option in a `.classes.ts` file does nothing. Five exist. ### Fix - Delete the emitter, the option type, the strip in `define()`, the five ignored blocks, and the stale notes next to them: 6 files, 273 lines removed. - The generated output is the same except for 94 empty `#if BUN_DEBUG`/`#endif` pairs in `ZigGeneratedClasses.h` and three DOMJIT-only `#include`s in `ZigGeneratedClasses.cpp`. `generated_classes.rs` is byte-identical. - Verified: the generator run before and after, `bun bd`, `tsc -p src`, and the tests in the Notes. - Self-reviewed: 13 concerns raised, 13 addressed (Notes). ### Background - DOMJIT is the JavaScriptCore fast path that lets the JIT call a host function with unboxed, type-checked arguments. The generator could emit a C++ signature and a Rust thunk for each method. - The hand-written DOMJIT users (`Buffer.alloc`, `performance.now`, `bun:ffi`) do not use this generator. They stay. - Earlier sweeps (#39249, #41169) called this removal a design call. This PR asks for that call alone. ### Downsides - To turn generated DOMJIT on again, a person must restore these paths from git history and write the Rust fast paths again. - No runtime cost found. Checked: the generated `.cpp`, `.h` and `.rs` differ only as Fix says. <details><summary>Notes</summary> **Removed** - `generate-classes.ts`: `DOMJITName`, `argTypeName`, `DOMJITType`, `DOMJITFunctionDeclaration`, `DOMJITFunctionDefinition`, `domJITTypeCheckFields`, `RustDOMJITArgType`. Also the `DOMJIT` branches in `zigExportName`, `propRow`, `renderDecls`, the `expectedResultType` asserts in the host-function wrapper, both Rust thunk loops, and the `DOMJITAbstractHeap.h`, `FrameTracers.h`, `DFGAbstractHeap.h` includes of the generated prologue. The destructures that named `DOMJIT` also lose the unused `cache` and `value` bindings. - `class-definitions.ts`: the `DOMJIT?:` option type and the two `.map()` calls in `define()` that erased it. - Ignored live blocks: `Crypto.randomUUID`, `Crypto.timingSafeEqual` (`crypto.classes.ts`), `ServerWebSocket.publishText`, `publishBinary` (`server.classes.ts`), `TextDecoder.decode` (`encoding.classes.ts`). - Stale notes: the commented-out `// DOMJIT: {` blocks with their crash notes on `sendText`, `sendBinary` (2023) and `getRandomValues` (#13470, 2024-08), and three orphan "DOMJIT fast path" comments in `src/runtime/webcore/Crypto.rs` whose functions #35002 deleted. **Self-review, and what changed because of it** - The first draft mixed this design call with 22 log scopes and seven fields. It now ships alone, with its history in the body. - The leftover DOMJIT notes (`Crypto.rs`, the commented-out blocks) are folded in. - Three deletions that open PRs carry were dropped (#40232, #41385). - Five items that open PRs use were taken out of the held branch (#43283, #31855, #42819, #39222, #37518). Two `builtins.d.ts` lines were dropped too: `src/codegen/replacements.ts` defines `$ImportKindLabelToId`, so that declaration is live. **History** - #13470 (2024-08) turned DOMJIT off for `getRandomValues`. #14005 (2024-09) added the strip in `define()` as the repair for the #14001 segfault. #35224 found the cause (the wrappers returned `{ result }` with a null exception slot) and tried to repair the generated wrappers. A stale-PR cleanup closed it with no maintainer comment. #35002 deleted the Rust fast paths, so the option cannot come back without new native code. **Kept on purpose** - The hand-written `DomCall` path for `bun:ffi` (`src/jsc/host_fn.rs`, `src/runtime/ffi`), the C++ DOMJIT signatures in `JSBuffer.cpp`, `JSPerformance.cpp`, `NodeVM.cpp`, `JSSQLStatement.cpp`, and `test/js/bun/jsc/domjit.test.ts`. **Tests (debug build)** - `test/js/web/encoding/text-decoder.test.js` 127 pass, `test/js/web/web-globals.test.js` 23 pass, `test/js/bun/util/randomUUIDv5.test.ts` 40 pass, `test/js/bun/websocket/websocket-server.test.ts -t sendBinary` 5 pass. - `websocket-server.test.ts -t "publish|send"`: 44 pass, 4 time out near 19 s under debug+ASAN. A debug binary built from main fails the same 4. - `test/js/bun/jsc/domjit.test.ts`: 40 pass, 10 time out at the 100k-iteration sizes. A debug binary built from main gives the same 40/10. **The rest of this sweep** - Relink of the debug build with `-Wl,--gc-sections`, then the DWARF line table of the result: 2,290 of 31,118 Rust `fn`s have no live line. After `cargo check` on six targets only three `pub fn`s had no caller anywhere. The 96 trait impls with no caller are the ones #43664 kept on purpose. - clang `-fsyntax-only -Wunused-function -Wunused-template -Wunused-member-function -Wunused-macros` over bun's 177 C/C++ translation units (the build passes `-Wno-unused-function`): ten hits. Open PRs delete them, or an `#if` uses them. - oxlint `no-unused-vars` over `src/js`, `src/codegen`, `scripts`, `packages`. cargo's `unused_dependencies` lint over four targets. A scan for commented-out blocks (62 lines in the repo). Nothing new that is certain. - Each deletion was compared with the diffs of the 36 open dead-code PRs. Left out because an open PR has it: `Bun__napi_get_version` (#40232), two unused generator locals (#41385). **Verified and held for the next run** (branch `robobun/9a0817f5/dead-code-scopes-fields`, 25 files, 81 lines removed) - 20 `declare_scope!` scopes that nothing logs to: `JSC`, `STR`, `Bundle` and `scan_counter` (outer pair), `Store`, `hot_reloader`, `CLI`, `LibUVBackend`, `ResolveInfoRequest`, `GetHostByAddrInfoRequest`, `CAresNameInfo`, `GetNameInfoRequest`, `CAresReverse`, `CAresLookup`, `quic_session`, `PathWatcherManager`, `S3Client`, `S3Stat`, `AWS`, `uws` (`uws_sys/socket.rs`). rustc does not lint an item that another crate's macro expands. - Fields: `Runtime::Features.jsx_optimization_inline` with the local `can_be_inlined`, `DebugOptions.output_file`, `ArchiveIterator.filter`, `PackageManager.total_scripts`, `CommandLineArguments.lockfile`, `ArgumentsSlice::_vm`. Also `struct_Channeldata` and two empty modules in `napi_body.rs`. - It passed `rust:check-all` (12 targets), release and `--cfg bun_debug --cfg bun_asan` checks on linux, windows and darwin, and `cargo check --tests` before the trim below. The trimmed commit passes `cargo check` on linux. - Taken out because an open PR uses the item: `BundleOptions.code_coverage` (#43283 reads it), scope `ModuleLoader` (#31855), scope `PROCESS` (#42819, #39222), `impl Clone for JsPoster` and its vtable slot (#37518 rewrites the vtable). **Follow-up candidates, not verified dead** - `Parser Options.preserve_unused_imports_ts` is never `true`. tsconfig `importsNotUsedAsValues` is parsed into `preserve_imports_not_used_as_values` but never reaches the parser, in the released binary too. This looks like a missing feature. - `completions/bun-cli.json` (4,513 lines) and `misctools/generate-cli-completions.ts` (728 lines): nothing in the repo reads the JSON, but feature PRs still edit it by hand. - `bench/snippets/runner-entrypoint.js` (244 lines): no reference, first line says "this isn't done yet", last real change 2023-05. - Ten `impl_timer_owner!` accessors have no caller because `dispatch.rs` recovers the owner with its own `owner!` macro. Which mechanism stays is a design call. - `mordant-baseline.toml` still counts about 170 `unused_pub` findings (`sys/lib.rs` 56, `libuv_sys/libuv.rs` 41, `errno/windows_errno.rs` 31). `bun run rust:mordant` names them. </details>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ModuleLoader::reset_arena unconditionally destroyed and recreated the per-transpile mimalloc heap after every module, diverging from the intended behavior: a full reset only under --smol, and an 8 MiB cap-gated retain otherwise. The unconditional destroy/new round-trip pays mi_heap_destroy teardown plus page re-commit and re-zero on every transpile cycle; every other per-cycle arena call site (bundler chunk generation, installer, renamer, bake router, the transpiler store hook) already uses reset_retain_with_limit(8 MiB), and the MimallocArena doc comment documents why the 'segment cache keeps pages warm anyway' rationale for unconditional reset was wrong. reset_arena now reads vm.smol and calls reset() under --smol (lowest steady-state RSS) and reset_retain_with_limit(8 * 1024 * 1024) otherwise, matching the other call sites, and emits a BUN_DEBUG_ModuleLoader scoped log of the branch taken (free_all/retained/recycled; debug builds only, compiled out of release). Tested with a subprocess stress test that require()s many distinct small modules (require forces the synchronous transpile path through reset_arena; dynamic import routes through the concurrent transpiler store and skips it) plus oversized 150k-statement modules with a trailing syntax error (the parse-error path parks the arena un-reset, so it is the only path where reset_arena sees an over-cap arena and recycles), with forced GC interleaved, in both default and --smol modes; on debug builds the test counts the branch-tag logs differentially (default: retained>=iters, recycled>=brokenCount, zero free_all; --smol: free_all only), so a revert to unconditional reset or an inverted smol gate fails. Outstanding verify-phase action: run the transpile-heavy before/after RSS+time benchmark the work order requires and record the numbers in reports2/24-fix.md.
Verification
Implemented and verified on a unified integration branch: full debug build (linux-x64, ASAN), cargo check across the workspace, and the affected test files run against the debug build (failures cross-checked against main's build to exclude pre-existing issues). Each change was reviewed twice (compile/API correctness and GC/concurrency/semantics lenses) with findings repaired before landing.