Conversation
…y name
A local `bun bd` debug build is ASAN-instrumented but the binary is named
`bun-debug`, not `bun-asan`, so `process.execPath.includes("bun-asan")`
returned false and the fixtures applied the tight non-ASAN RSS threshold. Under
`bun bd test test/js/web/timers/setTimeout.test.js` the three "doesn't leak
when clear/refresh/repeat" tests failed with "RSS grew by ~138 MB" against a
10 MB cap. CI was unaffected because its ASAN lane binary is literally named
`bun-asan`.
Probe the runtime the same way harness.ts already does, via
`require("bun:internal-for-testing").isASANEnabled()`, with the name check
as a fallback for Node and for release binaries without the internal testing
flag.
|
Updated 2:50 AM PT - Jul 22nd, 2026
✅ @robobun, your commit ba041e5b152ccb3ee055c038a05b1285da46ad99 passed in 🧪 To try this PR locally: bunx bun-pr 35081That installs a local version of the PR into your bun-35081 --bun |
|
Reproduced on main with: 3 fail before ("RSS grew by ~138 MB" vs a 10 MB cap), 3 pass after. Test-only change; no CI: the remaining failures on build 77545 are all retry-flaky and unrelated to this diff ( |
WalkthroughChangesASAN detection across leak fixtures
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/cjs-fixture-leak-small.js`:
- Line 10: Replace the ASAN fallback in the leak-detection checks with a
basename-only executable-path check, matching the established behavior in
test/harness.ts. Apply this change at test/cli/run/cjs-fixture-leak-small.js:10,
test/cli/run/esm-bug-leak-fixture.mjs:12,
test/cli/run/esm-fixture-leak-small.mjs:12,
test/cli/run/require-cache-bug-leak-fixture.js:10,
test/js/bun/http/server-fetch-string-leak-fixture.js:12,
test/js/bun/io/bun-write-leak-fixture.js:11,
test/js/node/http2/node-http2-memory-leak.js:12,
test/js/node/url/pathToFileURL-leak-fixture.js:11,
test/js/web/fetch/fetch-leak-test-fixture-2.js:18, and
test/js/web/timers/setInterval-leak-fixture.js:12, using the existing basename
utility rather than checking the full process.execPath.
In `@test/js/web/timers/setTimeout-clear-in-callback-leak-fixture.js`:
- Line 20: Update the fallback executable check in the timer leak fixture to
compare against the basename of process.execPath, matching the approach used by
test/harness.ts, so parent-directory names cannot trigger a false match.
🪄 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: 2f76a6f2-a8ef-460a-b513-018456ba2e7b
📒 Files selected for processing (11)
test/cli/run/cjs-fixture-leak-small.jstest/cli/run/esm-bug-leak-fixture.mjstest/cli/run/esm-fixture-leak-small.mjstest/cli/run/require-cache-bug-leak-fixture.jstest/js/bun/http/server-fetch-string-leak-fixture.jstest/js/bun/io/bun-write-leak-fixture.jstest/js/node/http2/node-http2-memory-leak.jstest/js/node/url/pathToFileURL-leak-fixture.jstest/js/web/fetch/fetch-leak-test-fixture-2.jstest/js/web/timers/setInterval-leak-fixture.jstest/js/web/timers/setTimeout-clear-in-callback-leak-fixture.js
There was a problem hiding this comment.
LGTM — mechanical test-fixture change that mirrors the isASAN probe already in test/harness.ts. The inline nit about async-context-fs-watch.js is optional (skip guard, not an RSS threshold — arguably outside the PR's stated scope).
Extended reasoning...
Overview
Eleven leak-test fixture files switch their isASAN detection from process.execPath.includes("bun-asan") to a try/catch around require("bun:internal-for-testing").isASANEnabled(), keeping the name check as a fallback. No src/ changes, no threshold values changed, no assertions weakened. The identical IIFE is applied 11 times.
Correctness checks
isASANEnabledexists:src/js/internal-for-testing.ts:387→jsFunction_isASANEnabledinsrc/jsc/bindings/InternalForTesting.cpp:66. It's the same probetest/harness.ts:58-61already uses, so the "same as harness.ts" comment is accurate..mjsfixtures: both already doconst require = createRequire(import.meta.url)before the new IIFE, sorequire("bun:internal-for-testing")is defined..jsfixtures with top-levelimport(pathToFileURL-leak-fixture.js,node-http2-memory-leak.js,fetch-leak-test-fixture-2.js): Bun allows mixedimport/requirein.js, and several of these files already userequireelsewhere.- Not a silent test weakening: on non-ASAN release builds,
isASANEnabled()returns false (or the require throws → falls through to name check → false), so the tight threshold is still selected. Only ASAN-instrumented binaries get the wide threshold — which was the intent of the original code. CI behavior is unchanged in both lanes. - Same-class sweep: grepped for remaining
bun-asanname checks intest/; the only fixture-level site left isasync-context-fs-watch.js:5(.endsWithvariant), which is a skip guard rather than an RSS threshold. Flagged as a nit inline. All other hits are comments or.test.tsfiles that importisASANfrom harness.
Security risks
None — test-fixture-only change, no user-facing surface.
Level of scrutiny
Low. Test infrastructure only, mechanical pattern replication of an established harness helper, no threshold or assertion changes.
Other factors
The one inline finding is an optional same-class nit outside the PR's declared scope; it doesn't block.
There was a problem hiding this comment.
LGTM — mechanical test-fixture change, identical 7-line probe applied to all 11 sites.
What was reviewed:
- Confirmed the IIFE matches
test/harness.ts'sdetectASAN()and thatisASANEnabled()exists as the referenced probe. - Checked
require()is in scope at each site: the two.mjsfixturescreateRequireabove the IIFE; the ESM-syntax.jsfixtures are Bun-only whererequireis a global (several already callrequire("bun:jsc")later). - Prior nit about
async-context-fs-watch.jswas answered (intentional exclusion — widening that skip would hide a passing test); CodeRabbit's basename-fallback note was also resolved (the fallback is dead underbunEnv).
Extended reasoning...
Overview
Test-infrastructure-only change touching 11 leak-test fixture .js/.mjs files under test/. Each file replaces const isASAN = process.execPath.includes("bun-asan") with an IIFE that first tries require("bun:internal-for-testing").isASANEnabled() and falls back to the pre-existing name check. No src/ changes; RSS thresholds themselves are unchanged. The pattern is copied verbatim from test/harness.ts:56-62 (verified).
Security risks
None. Test fixtures only; no user-facing code, no auth/crypto/permissions, no new inputs parsed.
Level of scrutiny
Low. This is a mechanical sweep of an identical snippet across 11 files whose only effect is selecting which of two pre-existing RSS thresholds applies. The change can only widen the threshold on binaries that are actually ASAN-instrumented (per the #if ASAN_ENABLED probe), so it cannot mask a real leak on release builds and does not alter CI behavior (CI's bun-asan binary already matched the old check). Worst case is a fixture that previously false-failed under bun bd now passes locally — which is the stated goal.
Other factors
- The two
.mjsfixtures bindrequireviacreateRequire(import.meta.url)before the new IIFE, so therequire("bun:internal-for-testing")call resolves. The remaining ESM-syntax fixtures (node-http2-memory-leak.js,pathToFileURL-leak-fixture.js,fetch-leak-test-fixture-2.js, etc.) are Bun-only and already rely on Bun's globalrequirein ESM elsewhere in the same file, so no new assumption is introduced. - The try/catch means Node.js dual-runtime fixtures fall through to the name check (correctly
falseunder Node), preserving prior behavior. - All three review threads (CodeRabbit ×2 on basename fallback, my prior note on the
.endsWithsibling inasync-context-fs-watch.js) are resolved with stated rationale; nothing outstanding. - PR description includes a before/after repro (
bun bd test setTimeout.test.js -t "doesn't leak when": 3 fail → 3 pass).
Review feedback: copy-pasting the detection IIFE into 11 fixtures is lockstep-edit #2 of the same snippet. harness.ts already knows whether the running binary is ASAN-instrumented (detectASAN()), and every one of these fixtures is spawned with env: bunEnv, so set BUN_TEST_IS_ASAN there once and have each fixture read process.env.BUN_TEST_IS_ASAN. One line per fixture, runtime-agnostic, survives tempdir copying, single source of truth.
There was a problem hiding this comment.
LGTM. The revised env-var approach (BUN_TEST_IS_ASAN set in bunEnv) is cleaner than the per-fixture probe — single source of truth in harness.ts's detectASAN(), no bun:internal-for-testing boilerplate, and it moots the basename-fallback concern entirely.
What was reviewed:
- Verified all 11 fixture spawn sites propagate
bunEnv(eitherenv: bunEnv,{...bunEnv, …}, or the.toRun()matcher which usesbunEnv), soBUN_TEST_IS_ASANreaches every fixture and there's no regression on CI'sbun-asanlane vs. the old name check. - Confirmed the new key survives the
BUN_DEBUG_*deletion loop in harness.ts and thatciEnv(snapshotted before it's set) is only used forbuildkite-agent secret get. - The earlier sibling-site note (
async-context-fs-watch.js) was resolved as an intentional exclusion.
Extended reasoning...
Overview
Test-infrastructure-only change: 11 leak fixtures previously detected ASAN via process.execPath.includes("bun-asan"), which is false for local bun bd builds (ASAN-instrumented but named bun-debug), causing false-positive leak failures locally. The final revision has test/harness.ts set bunEnv.BUN_TEST_IS_ASAN = "1" inside its existing if (isASAN) block, and each fixture reads process.env.BUN_TEST_IS_ASAN === "1". No src/ changes.
The PR went through two iterations — first a per-fixture require("bun:internal-for-testing").isASANEnabled() probe with a name-check fallback, then (commit ba041e5) the current env-var approach. The PR description is stale and still describes the first iteration, but the diff and commit message are clear.
Security risks
None. Test-only, no runtime code paths, no external input handling.
Level of scrutiny
Low. Mechanical replacement of a boolean detection expression across 11 fixture files plus a 4-line addition to bunEnv. The only correctness question is whether every fixture's spawn site actually passes bunEnv — otherwise the env var would be absent and the ASAN threshold would not be selected on CI's bun-asan lane (a regression vs. the old name check). I traced each fixture to its test driver: setTimeout.test.js, setInterval.test.js (via .toRun()), bun-write-leak.test.ts (via .toRun()), s3.leak.test.ts (...bunEnv — but that spawns a different s3-local fixture, not the io one), fetch-leak.test.ts (...bunEnv), pathToFileURL.test.ts (via .toRun()), node-http2.test.js (...bunEnv), leaks-test.test.ts (via .toRun()), and require-cache.test.ts (all four cli/run fixtures, env: bunEnv). The .toRun() matcher in harness.ts uses env: bunEnv. All covered.
Other factors
- CodeRabbit's basename concern on the earlier revision is moot: the name-check fallback is gone.
- My earlier note about
async-context-fs-watch.js(a same-class.endsWith("bun-asan")skip guard) was answered: intentionally excluded because widening it would hide a locally-passing test, and it's a TODO skip rather than a threshold selector. Reasonable. bunEnvspreadsprocess.env, so a nested test spawn would inherit the flag — correct, since a child of an ASAN binary is the same ASAN binary.- The Node dual-runtime fixtures correctly get
isASAN = falseunder Node (env var unset), which is right since Node isn't ASAN-instrumented.
|
Still reproducible on current main (e697804), this time through
The branch is 611 commits behind main and 7 of the other fixtures now conflict, in every case because main added the
Resolving each one means keeping main's |
|
Still reproducible on current main (4448a2e) through the repro in the PR description. A local
|
|
Still present on main at 0823e50: all eleven fixtures keep the This branch now conflicts with main in seven of the fixtures. #36429 (merged Jul 30) added an |
|
#41207 rewrites four of the fixtures that this PR changes: |
Problem
Eleven leak-test fixtures detect ASAN with
process.execPath.includes("bun-asan"). That is true on CI's ASAN lane (the binary is literally namedbun-asan) but false for a localbun bdbuild, which is ASAN-instrumented but namedbun-debug. So the fixtures apply the tight non-ASAN RSS threshold locally and report false leaks.Repro on main:
The 10 MB cap is compared against the ~138 MB ASAN-quarantine growth because
isASANis false; the 192 MB ASAN cap is never selected. CI does not see this becausebun-asanmatches the name check.Fix
test/harness.tsalready detects ASAN correctly viaisASANEnabled()frombun:internal-for-testing(a compile-time#if ASAN_ENABLEDprobe), and every one of these fixtures is spawned withenv: bunEnv(either directly or via thetoRunmatcher). So setbunEnv.BUN_TEST_IS_ASAN = "1"once inside harness.ts's existingif (isASAN)block, and have each fixture readprocess.env.BUN_TEST_IS_ASANinstead of re-detecting.This is the second lockstep edit of the same detection snippet across these eleven files (
fba43af684added the name check to each); centralizing it inbunEnvmeans there isn't a third. The env var is runtime-agnostic (works under Node for the dual-runtime fixtures) and survives the tempdir-copy pattern used bybun-write-leak.test.ts.The change only widens the RSS threshold on a binary the harness has confirmed is ASAN-instrumented, so a real leak still trips the tight threshold on release builds. CI behaviour is unchanged:
bun-asanwas already detected before, and still is.Verification
Under
./build/debug/bun-debug:bun bd test test/js/web/timers/setTimeout.test.js -t "doesn't leak when"now passes (3/3).Files touched (all with the identical one-line env read):
test/harness.ts(+BUN_TEST_IS_ASANinside the existingif (isASAN)block)test/js/web/timers/setTimeout-clear-in-callback-leak-fixture.jstest/js/web/timers/setInterval-leak-fixture.jstest/js/bun/http/server-fetch-string-leak-fixture.jstest/js/bun/io/bun-write-leak-fixture.jstest/js/web/fetch/fetch-leak-test-fixture-2.jstest/js/node/url/pathToFileURL-leak-fixture.jstest/js/node/http2/node-http2-memory-leak.jstest/cli/run/esm-bug-leak-fixture.mjstest/cli/run/require-cache-bug-leak-fixture.jstest/cli/run/cjs-fixture-leak-small.jstest/cli/run/esm-fixture-leak-small.mjstest/js/node/async_hooks/async-context/async-context-fs-watch.jsis intentionally excluded: it is a skip guard (process.exit(0)) rather than an RSS threshold, and the fixture already passes underbun bd, so widening the skip there would hide a passing test.This is a test-infrastructure fix with no
src/change.no test proof · iteration 0 · docs-only change; test-proof not applicable