test(require-cache): skip the leak fixtures under debug builds - #38148
test(require-cache): skip the leak fixtures under debug builds#38148robobun wants to merge 1 commit into
Conversation
The three "don't leak" describe blocks load their modules 55 to 100k times with workloads sized for release builds. Under a debug build one load of the larger modules takes 1-7 s, so every fixture overruns its timeout, and because the tests are concurrent bun test does not kill the timed-out children, which keep running (holding the inherited stdio) for up to half an hour after the run ends. The RSS bounds are also not meaningful under the debug build's ASAN quarantine. CI has no debug lane; the release and release+ASAN lanes still run the fixtures.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 5:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 68792c9 has some failures in 🧪 To try this PR locally: bunx bun-pr 38148That installs a local version of the PR into your bun-38148 --bun |
There was a problem hiding this comment.
LGTM — test-only change gating three RSS-leak describe blocks behind skipIf(isDebug), matching the established pattern.
What was reviewed:
isDebugisBun.version.includes("debug")(harness.ts:27); CI's release and release-asan lanes are unaffected, so no coverage is lost.- Same pattern already used for RSS-leak tests in
password.test.ts,node-tls-getpeercert-leak.test.ts,html-rewriter-leak.test.ts, and others. - The skip carries an inline comment naming the observed failure (timeouts + orphaned children + ASAN-quarantine bounds), satisfying the "every skip needs a comment with the observed failure" rule; the old one-line "extra slow in debug builds" comment it replaces was less informative.
- The existing
isBroken && isIntelMacOSgate on the first block is preserved via theskipSourceLeakChecksconst.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/run/require-cache.test.ts, adding describe.skipIf(isDebug) to three memory-leak describe blocks and importing isDebug from harness. The first block folds the new condition with its pre-existing isBroken && isIntelMacOS gate into a named const so the describe line stays within print width and the 230-line body isn't reindented. A six-line comment above the const explains the debug-build failure mode (per-load takes 1–7 s, fixtures overrun timeouts, concurrent tests leave orphaned children holding stdio for up to 30 min, ASAN quarantine makes RSS bounds meaningless). The stale one-liner // These tests are extra slow in debug builds is removed in favor of that comment. No runtime code, fixtures, or CI config are touched.
Security risks
None. Test-only change to skip conditions; no production code, no inputs, no new dependencies.
Level of scrutiny
Low. This is a mechanical application of an existing repo convention: skipIf(isDebug) on RSS-threshold leak tests already appears in password.test.ts, node-tls-getpeercert-leak.test.ts, html-rewriter-leak.test.ts, buffer.test.js, gc-controller-cadence.test.ts, and bun-types.test.ts. I verified isDebug in harness is Bun.version.includes("debug"), so it is true only for the Debug build type (bun bd) and false on both CI lanes (release and release-asan) — CI coverage of the eight leak assertions is unchanged. The functional require.cache tests at the top of the file are untouched and still run on debug builds.
Other factors
REVIEW.md requires that every skip carry a comment naming the observed failure; the added comment does this precisely (timeouts, orphaned children, quarantine-invalid bounds) and notes CI is unaffected. The PR description independently verifies before/after behavior on both debug (bun bd test: 7 fail + 5 orphans → 3 pass / 8 skip, no orphans) and release (USE_SYSTEM_BUN=1: 11 pass unchanged). The bug-hunting system found no issues. No prior reviewer comments to address. The change is small, self-contained, and reversible.
|
Status
|
Problem
bun bd test test/cli/run/require-cache.test.tsdoes not come back.bun testitself exits after ~94 s with 7 failures, every one of themthis test timed out after 60000ms/30000ms/20000ms(the two standalone CJS fixtures additionally print--fail--once they finish, reported as 2 errors between tests), but it leaves the five leak fixtures it spawned running as orphans. They inherit stdio, so anything reading the run's output (a pipe, a tool,tee) waits until the last of them finishes; theimport()long-export-names fixture alone needs about 33 minutes on a debug build.test/cli/run/require-cache.test.ts:53,:284,:313) load a module 55 to 100k times with sizes chosen for release builds. Per load on a debug build here, measured standalone: 0.8 s (require()of the 1.1 MB long-names module, 550 loads), 1.2 s / 1.5 s (require()/import()of the 20k-call module, 500 loads each), 6.5 s (import()of the 1.2 MB long-export-names module, 300 loads). The release binary does the whole file in 14 to 18 s. The two standalone CJS fixtures do not fit either:require-cache-bug-leak-fixture.jstakes 32 s alone against a 20 s test timeout,cjs-fixture-leak-small.js28 s alone against 30 s (it timed out in the concurrent run, and fails its bound regardless, next bullet), andesm-fixture-leak-small.mjsseveral minutes against 30 s.bun testkills a timed-out test's subprocesses only when the timed-out group holds a single test (src/runtime/test_runner/Execution.rs:301,handle_timeout); this file is adescribe.concurrent, where the children cannot be attributed to one test, so nothing is killed, and at the end of the file the tracker is cleared without killing (src/runtime/cli/test_command.rs:3392). The tests' ownawait using procnever runs because the test body is still parked onawait proc.exitedwhen the timeout fires.bun-asanbinary name, so underbun-debug(also ASAN) they apply the release bounds and reportleaked: "168 MB"against 120 MB and"62 MB"against 48 MB with nothing leaking (ASAN quarantine); the inline fixtures use the harnessisASANbounds of 320 to 400 MB, which a leak only clears after hundreds of loads, i.e. the minutes-long workload above.Fix
describe.skipIf(isDebug)on the three leak describe blocks (the first one keeps its existingisBroken && isIntelMacOScondition, folded into a const so the describe line still fits the print width and the 230-line body is not re-indented). The functionalrequire.cachetests in the file are unchanged and still run everywhere.test/js/bun/util/password.test.ts,test/js/node/tls/node-tls-getpeercert-leak.test.ts,test/js/workerd/html-rewriter-leak.test.ts), for the same two reasons (time and quarantine).isDebugisBun.version.includes("debug"), and the-debugsuffix comes from the Debug build type (src/bun_core/Global.rs:489,scripts/build/rust.ts:455). CI's ASAN lane is therelease-asanprofile (Release build type,scripts/build/profiles.ts), so it and the release lanes keep running all eight leak tests exactly as before; no lane sees a difference, and the durations/allowlist files need no change.bun testshould reap the children of timed-out concurrent tests is a runtime question theExecution.rscomment already records as a deliberate trade-off; this PR does not change runtime behavior.bun bd test test/cli/run/require-cache.test.ts: before,bun testexits after 93.8 s with 7 fail and 5bun-debug run ... fixtureprocesses left behind (reparented to pid 1, killed by hand); after, 8.2 s, 3 pass / 8 skip / 0 fail, no processes left.USE_SYSTEM_BUN=1 bun test test/cli/run/require-cache.test.ts(release 1.4.0): 11 pass in 18 s before and after; every leak fixture runs and prints its RSS figures, so the release/CI path is not affected by the change.x64-asantest lane:bun test v1.4.0-canary.1 (68792c97a)ran the file unskipped, 11 pass in 52 s, with the four inline fixtures printingRSS diff 46 / 220 / 127 / 151 MB(i.e.isDebugis false on the release-asan build, as claimed above).esm-fixture-leak-small.mjs) and module loader: report module source text to the GC as extra memory #38135 (adds a describe block) change different lines; the only overlap is theharnessimport list, and the skip applies equally to their versions of the fixtures.Background
bun bdbuilds thedebugprofile: Debug build type, ASAN on, and the debug WebKit prebuilt (JSC with its assertions enabled), which is what makes a single transpile+evaluate of a 1 MB module take seconds. CI has no lane built this way; its sanitizer lane isrelease-asan(optimized code plus ASAN), which is several times slower than release, not a hundred times.bun testtimeouts and subprocesses: while a group of tests runs,bun testtracks the processes they spawn (ProcessAutoKiller). When a test times out it kills them only if the group contains that one test; in adescribe.concurrentgroup the tracked set mixes every running test's children, so it is left alone. A timed-out test's body is not cancelled either; it stays suspended, soawait usingcleanup in it never runs.Per-load timings and the unfixed run
Debug build (
bun bd, linux x64, this container), each fixture'sindex.jsloaded standalone with--smoland the transpiler cache disabled, 5 loads each:require()long export namesrequire()20k callsawait import()20k callsimport()long export namesrequire-cache-bug-leak-fixture.js(AST)leaked: "168 MB"then--fail--cjs-fixture-leak-small.js(file paths)leaked: "62 MB"then--fail--Debug-sized variant of the four inline loops (5 warm-up + 10 measured loads, nothing retained, the four processes running concurrently on the debug build as they would under
describe.concurrent):require()long export namesawait import()20k callsimport()long export namesrequire()20k callsRelease (
bun1.4.0) for comparison: theimport()long-export-names loop is about 58 ms per load, and the cost grows roughly 2.3x per doubling of the export count (5k: 17 ms, 10k: 42 ms, 20k: 101 ms, 40k: 233 ms), so the debug figure is debug-build slowness, not an algorithmic problem in the loader.Unfixed run,
bun-debug test test/cli/run/require-cache.test.tswith output redirected to a file:Ran 11 tests across 1 file. [93.78s], 4 pass, 7 fail, 2 errors: all 7 failures are timeouts, and the 2 errors aredon't leak the AST > via require()anddon't leak file paths > via require()printingleaked: "157 MB"/"66 MB"and--fail--after their tests had already timed out. Immediately afterwardspsshowed fivebun-debug runfixture processes with ppid 1 still running (the four inline fixtures andesm-fixture-leak-small.mjs).The one leak test that passed quickly on debug,
don't leak the AST > via import(), does so because its fixture evicts by afile://key that never matches the cache, so it loads the module once; that is a separate pre-existing problem withesm-bug-leak-fixture.mjsand is being handled on its own, it is not what this PR addresses.