test(glob): cut leak.test.ts iterations ~3-10x by disabling ASAN quarantine - #36086
test(glob): cut leak.test.ts iterations ~3-10x by disabling ASAN quarantine#36086robobun wants to merge 3 commits into
Conversation
…antine The four RSS-growth probes each ran 100k scan()/scanSync() iterations. Under a debug+ASAN build that is ~60s per subprocess, so the async scan tests hit their own 60s timeout and the file ran ~62s locally (11s on darwin release CI, 7s on Windows release CI). Most of that work was spent saturating ASAN's freed-allocation quarantine (~360 MB plateau observed) so the 400 MB ASAN threshold would hold. Passing quarantine_size_mb=0 to the subprocess drops the noise floor to ~15 MB under ASAN and ~5-7 MB on release, which lets a single 30 MB bound separate the fixed case from a reintroduced #29379 leak (65-90 MB at 10k iterations under ASAN, >= 40 MB at 30k on darwin release where PathBuffer is 1 KB). While here: - pipe stdout/stderr and assert the growth number directly instead of only checking the exit code, so a failure reports the observed MB - fold the four near-identical bodies into one parameterised loop - halve the warmup and trim the per-test timeout to 30s
WalkthroughChangesGlob leak tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: CI shows one hard failure, Ready for review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup that fixes debug+ASAN timeouts while preserving CI leak coverage. One inline nit about local macOS debug builds potentially slipping under the 30 MB bound at 10k iters; CI is unaffected so it's non-blocking.
Extended reasoning...
Overview
This PR rewrites test/js/bun/glob/leak.test.ts to run ~3-10× faster by disabling the ASAN quarantine (quarantine_size_mb=0) on the spawned subprocess, which collapses the RSS noise floor enough that a single 30 MB threshold works across build variants. Iteration counts drop from 100k to 10k (ASAN/debug) or 30k (release), the four copy-pasted test bodies are folded into a parameterised loop, and assertions now report the observed growth via a piped-stdout {stderr, growthMB, exitCode} object instead of a bare exit-code check. No src/** changes.
Security risks
None. Test-only change; the subprocess runs a fixed inline script under bunEnv in a tempDir, and the only new env var is a well-known ASAN tuning knob appended to any existing ASAN_OPTIONS.
Level of scrutiny
Low-to-moderate. The risk with leak-test tuning is silently weakening detection. The PR description shows the reintroduced #29379 leak still fails all four cases at 65-89 MB vs the 30 MB bound on the configurations that run in CI (Linux debug+ASAN, macOS/Linux/Windows release), so CI coverage is preserved. The inline nit flags that macOS local bun bd (debug, 1 KB PathBuffer, 10k iters) lands at ~13-26 MB — at or below the threshold — but that gap does not exist on any CI lane, so a regression cannot escape to main.
Other factors
The refactor follows harness conventions the repo enforces: {...bunEnv, ...} spread, concurrent pipe drain via Promise.all, combined-object assertion before the numeric bound, tempDir with using, and test.concurrent retained. The previous version of this file timed out under debug+ASAN (62 s vs 60 s limit), so this is strictly an improvement even accounting for the local-macOS nit. The ASAN_OPTIONS join with .filter(Boolean) correctly handles an unset base value. Approving with the nit left inline for the author to optionally address.
macOS PathBuffer is 1 KB, so 10k iterations of the #29379 leak only reach ~13-26 MB locally and could slip under the 30 MB bound. Gate the 10k reduction on !isMacOS so macOS keeps 30k regardless of build type; CI (Linux ASAN, macOS/Windows release) is unchanged.
There was a problem hiding this comment.
LGTM — the macOS iteration-count nit from the previous review is addressed in ab74dd7 (&& !isMacOS), and no new issues turned up on re-review.
What was reviewed:
- Confirmed the four original test names/patterns/fixtures are preserved 1:1 in the parameterised loop.
ASAN_OPTIONScomposition viafilter(Boolean).join(":")handles both set and unsetbunEnv.ASAN_OPTIONS.- Checked the macOS-debug 30k-vs-30s-timeout concern: ruled out (release macOS did 100k in ~11s; 30k under debug+ASAN stays well inside 30s, and CI has no macOS debug lane anyway).
- NaN from garbage stdout would still fail via
toBeLessThan(30), soexpect.any(Number)isn't a hole.
Extended reasoning...
Overview
Test-only refactor of test/js/bun/glob/leak.test.ts: cuts iteration counts 3-10× by disabling ASAN's freed-allocation quarantine on the subprocess (so RSS deltas measure the actual leak instead of quarantine saturation), collapses four copy-pasted test bodies into one parameterised for-loop over a cases table, replaces the branched isASAN ? 400 : 100 MB threshold with a single 30 MB bound, and improves failure diagnostics by piping stdout/stderr and asserting {stderr, growthMB, exitCode} before the threshold check. No src/** changes.
Delta since my previous review
I previously flagged that isASAN || isDebug ? 10_000 : 30_000 would give macOS debug/ASAN builds only 10k iterations, which at macOS's 1 KB PathBuffer would let a reintroduced #29379 leak (~13-26 MB) slip under the 30 MB threshold on local bun bd runs. Commit ab74dd7 addresses this exactly as suggested: (isASAN || isDebug) && !isMacOS ? 10_000 : 30_000, with the comment updated to explain why. Verified isMacOS is exported from test/harness.ts:20. The inline thread is resolved.
Security risks
None. Test-only; spawns bunExe() with a fixed inline script in a hermetic tempDir, no network, no untrusted input.
Level of scrutiny
Moderate — this is a leak-regression test being loosened (fewer iterations, tighter threshold, halved timeout), so the key question is whether detection sensitivity is preserved. The PR description demonstrates it empirically (reintroducing the #29379 leak fails all four tests at 65-89 MB vs the 30 MB bound), and the per-platform arithmetic checks out: Linux 10k × ≥4 KB, Windows 30k × ~96 KB, and now macOS 30k × ~1.3 KB all comfortably exceed 30 MB when leaking while the fixed noise floor (~7-15 MB with quarantine off) stays well below it. That satisfies REVIEW.md's "bound well below the unfixed leak" rule on every lane.
Other factors
- Harness conventions are followed:
using tempDir,await using proc,{...bunEnv, ...}spread, concurrent pipe drain viaPromise.all,test.concurrentfor independent subprocesses, stdout/stderr asserted before/alongside exitCode. - The one candidate raised this run (macOS debug/ASAN 30k iters vs the halved 30s timeout) was examined and ruled out: the old file ran 100k on macOS release in ~11s, so 30k on a debug+ASAN arm64 Mac lands well under 30s, and no CI lane runs macOS debug anyway — worst case is a slightly slower local run.
Number(stdout.trim())on unexpected output yieldsNaN, which passesexpect.any(Number)but failstoBeLessThan(30), so the assertion chain still fails closed.
macOS debug/ASAN now runs 30k iterations and its wall time there is unmeasured; extrapolating from the 17s linux-debug@10k datapoint puts the async pair near the previous 30s budget. The speedup in this PR comes from the iteration cut and quarantine_size_mb=0, not from lowering the ceiling, so restore the original 60s to avoid introducing a local macOS timeout.
There was a problem hiding this comment.
LGTM — the macOS iteration-count nit from my earlier pass is addressed in ab74dd7 (&& !isMacOS), and nothing else turned up on re-review.
What was reviewed
- Verified all four original test names/patterns/fixtures are preserved and the parameterised loop is behaviour-equivalent to the four bodies it replaces.
- Checked the
ASAN_OPTIONScomposition matches the repo idiom ([bunEnv.ASAN_OPTIONS, ...].filter(Boolean).join(":"), same as html-rewriter-leak / json5 / resolve-dns tests) and that pipes are drained concurrently viaPromise.all. - Confirmed the NaN-on-crash path still fails: if the child dies before printing,
exitCode: 0/stderr: ""mismatch trips the first assertion, andNaN < 30is false sotoBeLessThanwould also fail.
Extended reasoning...
Overview
Test-only rewrite of test/js/bun/glob/leak.test.ts. The four copy-pasted RSS-growth probes (100k iters, isASAN ? 400 : 100 MB threshold, stdio: inherit, bare expect(exitCode).toBe(0)) are folded into one parameterised for-of over a cases table. Iteration count drops to 10k on Linux/Windows debug/ASAN and 30k elsewhere; ASAN's freed-allocation quarantine is disabled on the subprocess so a single 30 MB threshold works across all build variants; stdout/stderr are now piped and the observed growth is asserted directly. No src/** changes.
Security risks
None. Pure test refactor; the only new external input is an ASAN_OPTIONS env var passed to a Bun subprocess the test itself spawns.
Level of scrutiny
Moderate for a test-only change, because the repo guidance is explicit that weakening a leak test's detection margin is a real regression. The two things that needed checking were (a) does the reduced iteration count + tighter threshold still catch the #29379 leak on every build/platform combination, and (b) does the refactor preserve the exact test names and inputs. The PR description shows empirical fail output (65-89 MB vs 30 MB) with the leak reintroduced on Linux debug+ASAN, and my earlier concern about the one uncovered combination (macOS debug/ASAN local at 10k iters × 1 KB PathBuffer ≈ 13-26 MB) was fixed in ab74dd7 by keeping macOS at 30k unconditionally.
Other factors
- The change tightens the assertion surface relative to the old file: it now asserts
stderr === "",exitCode === 0, and the numeric growth in a combined object (better failure diagnostics), where the old version only checked exit code with inherited stdio. - Subprocess handling follows REVIEW.md's rules:
{...bunEnv, ...}spread,Promise.all([stdout.text(), stderr.text(), exited])to avoid pipe-buffer deadlock,await using proc. - The
ASAN_OPTIONScomposition pattern is copied verbatim from several existing tests in the suite, so it's a known-good idiom rather than novel. - Harness imports (
isDebug,isMacOS,isASAN) all exist intest/harness.ts. - Per-test 60s ceiling is retained (commit 65b3b14), so a genuine hang is still caught; the speedup comes purely from fewer iterations.
|
Updated 5:43 AM PT - Jul 27th, 2026
❌ @robobun, your commit 65b3b14 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36086That installs a local version of the PR into your bun-36086 --bun |
1 participant
test/js/bun/glob/leak.test.tsruns four RSS-growth probes, each spawning a subprocess that callsscan()/scanSync()100 000 times. Under a debug+ASAN build that is ~60 s per subprocess, so the two asyncscantests hit their own 60 s timeout and the file fails locally (it shows up as 11 s on the darwin-14-aarch64 release lane in build 83134).Almost all of that work was spent saturating ASAN's freed-allocation quarantine so the 400 MB ASAN threshold would hold: the observed RSS delta plateaus at ~360 MB around 20-25 k iterations and the remaining 75-80 k buy nothing. Passing
quarantine_size_mb=0to the subprocess drops the ASAN noise floor to ~15 MB (~5-7 MB on release), which lets a single 30 MB bound separate "fixed" from "leaking" on every build variant.Change
PathBufferis only 1 KB there, so aBox<GlobWalker>leak at 10 k would only reach ~13-26 MB locally. Warmup 1000 -> 500.ASAN_OPTIONS=...:quarantine_size_mb=0on the subprocess so the RSS delta measures the leak rather than the quarantine, and replace the per-buildisASAN ? 400 : 100threshold with a single 30 MB bound.{stderr, growthMB, exitCode}plusgrowthMB < 30instead of the bareexpect(exitCode).toBe(0), so a failure reports the observed growth.Verification
Wall-clock before/after:
bun bd test(debug+ASAN, linux)Leak detection is preserved: reintroducing the #29379 leak (forgetting the
Box<GlobWalker>in__scan_sync/ thepath_bufbox inWalkTask::then) makes all four tests fail with 65-89 MB against the 30 MB threshold.Test-only change; no
src/**diff.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.