Conversation
…assert its numbers The child churned 80k signals with fixed sleeps between GCs and only its exit code was checked, which took 13s on the ASAN lane and 95s on a local debug build. The ASAN threshold (256 MB) was also above what the leak produces (~60 MB), and with ASAN's quarantine enabled RSS grows by everything the child allocates, so that lane could not fail. Now the child churns small batches after a warmup batch and reports how many AbortSignal wrappers survive a full GC plus the RSS growth per batch. The parent asserts both (every signal survives when the leak is back; RSS grows 23-34 MB at the release count against a 12 MB bound), checks stderr and the exit code, and the child runs with quarantine_size_mb=0 so RSS means the same thing under ASAN. Slow builds churn 1000 signals, release builds 40k; no sleeps and no per-test timeout.
|
Warning Review limit reached
Next review available in: 54 minutes 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 |
|
Status: ready for review. Test-only change, no Verified locally on linux x64:
Self-review is running; will update here if it turns anything up. Waiting on CI for the ASAN lane timing. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only rewrite that makes the #28756 leak test faster and its assertions meaningful.
What was reviewed:
- Harness/idiom conformance:
isDebug/isASANbranching,ASAN_OPTIONS=...:quarantine_size_mb=0join, andheapStats().objectTypeCounts.AbortSignalall match existing leak-test patterns in the repo. - Assertion strength: previously the ASAN bound (256 MB vs ~60 MB leak) was vacuous; now wrapper-count carries slow builds and RSS carries release, with the exit code asserted last.
- The remaining
setImmediatebetween GCs is not a sleep — it's the FinalizationRegistry callback turn, and the 300s timeout / 50–100ms sleeps are gone.
Extended reasoning...
Overview
This PR rewrites a single regression test file, test/regression/issue/28756.test.ts, which guards against the AbortSignal.timeout + util.aborted memory leak. No production code is touched. The rewrite (a) drops the workload on debug/ASAN builds from 80k signals + sleeps (~95s locally) to 1k signals with no sleeps (~2.5s), (b) moves pass/fail from the child's exit code to structured JSON that the parent asserts on, and (c) adds a direct heapStats().objectTypeCounts.AbortSignal wrapper-count assertion so slow builds can detect the leak without needing enough signals for RSS to separate.
Security risks
None. Test-only change; the child is spawned with bunExe() and bunEnv, no network, no filesystem writes, no untrusted input.
Level of scrutiny
Low-to-moderate. It's a leak test rewrite, so the main risks are (1) making the test vacuous, (2) making it flaky, or (3) breaking harness conventions. I checked each against REVIEW.md's "Tests reviewers reject" section:
- Not vacuous: the old ASAN bound of 256 MB could not fail (leak is ~60 MB). The new test asserts
leakedSignals < batchSize(leak producesbatchSize * batches) and RSS < 12 MB (leak produces 23–34 MB on release). The PR description documents that both assertions were exercised by simulating the leak and confirming failure. - Not a sleep-for-condition: the only remaining async wait is
setImmediatebetween two GCs, which the comment and PR description explain is the required event-loop turn for FinalizationRegistry callbacks — not a timing hack. The 50/100mssetTimeouts and 300s per-test timeout are removed. - RSS threshold branches on build type:
isASAN || isDebuggates the workload size, andASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0"].filter(Boolean).join(":")matches the exact pattern injson5.test.ts,archive.test.ts, and others. - Pipes drained concurrently,
stderrasserted empty, exit code asserted last.
Other factors
All helpers used (isDebug, isASAN, bunEnv.ASAN_OPTIONS join, Bun.unsafe.memoryFootprint on darwin, objectTypeCounts.AbortSignal) are already in use across multiple other leak tests, so there's no novel pattern here. The PR description records 20 debug+ASAN runs and 30 release runs with observed ranges, plus negative verification by re-introducing the leak two different ways. The comment block in the file records the measured numbers so future readers can see why 12 MB and < batchSize were chosen.
|
Updated 12:13 PM PT - Aug 13th, 2026
❌ @robobun, your commit 2871321 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38197That installs a local version of the PR into your bun-38197 --bun |
This branch has not been deployed
1 participant
Problem
test/regression/issue/28756.test.ts(leak test forAbortSignal.timeout()+util.aborted()causes unbounded memory growth #28756, fixed in Fix memory leak in AbortSignal.timeout() when listeners are removed #28761) takes 13s on the x64 ASAN lane and 95s under a localbun bd(debug+ASAN), versus 0.6-1.7s on the release lanes. The child churns 10 x 8000 signals and sleeps 50-100ms between GCs.baselineMB/finalMB/growthMBline is ignored.node:util, so on those builds a signal count that fits in the default test timeout is too small for an RSS bound to tell the two apart.Fix
aborted()called, listener removed) after one warmup batch, settles withgc, one event loop turn, gc, and prints JSON with the number ofAbortSignalwrappers that survived relative to the post-warmup count (bun:jscheapStats()) and the RSS growth after every batch.stderr === "", that every batch reported,leakedSignals < batchSize(a leak keeps every signal alive, the fixed build keeps 0), RSS growth below 12 MB with the per-batch trend in the failure message, and the exit code last.ASAN_OPTIONS=...:quarantine_size_mb=0, the same thing the other RSS based leak tests do, so freed memory is reused and RSS growth means retention on ASAN builds too.setImmediate) is required, not a delay: the{}passed toaborted()dies on the first GC, itsFinalizationRegistrycallback (a task) removes the listeneraborted()registered, and only the second GC can collect the signal. Verified by counting wrappers:gc, microtask, gcleaves all 1000 alive,gc, setImmediate, gcleaves 0. The 50/100ms sleeps and the 300s per test timeout are gone.AbortSignal.timeout()+util.aborted()causes unbounded memory growth #28756 did, or any other way, either keeps its wrapper alive (counted directly) or only its native object and timer alive (shows in RSS at the release count, 0.6-0.9 KB per signal, 23-34 MB at 40k signals against the 12 MB bound).Verification
bun bd test test/regression/issue/28756.test.ts(debug+ASAN): 94.8s before, 2.5-2.8s after (20 runs: RSS growth 0-3.1 MB,leakedSignals0 every time, slowest run 3.6s).USE_SYSTEM_BUN=1 bun test ...(release 1.4.0): 1.1s before, ~0.3s after (30 runs: RSS growth 2.0-3.1 MB,leakedSignals0 every time).40000 of 40000 AbortSignal wrappers survived GC, debug+ASAN1000 of 1000 ....RSS grew 32.0-34.0 MBandRSS grew 23.0 MB over 40000 signals (after batch 10: 3.0 MB, 20: 8.0 MB, ... 80: 23.0 MB)against the 12 MB bound; on debug+ASAN at 1000 signals both grow 0-3 MB and pass, which is the documented reason the wrapper count carries those builds.Background
util.aborted(signal, resource)registers an abort listener and aFinalizationRegistryentry keyed onresource; whenresourceis collected the callback removes the listener again.FinalizationRegistrycallbacks run as event loop tasks after the GC that found the dead target, so a microtask checkpoint is not enough to run them.JSAbortSignalOwner::isReachableFromOpaqueRoots); once the last listener is gone the wrapper is collectible and its destructor frees the native signal and its timer (Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC #35849, AbortSignal.timeout: keep the timer armed when the signal loses its observers #37666).AbortSignal.timeout()+util.aborted()causes unbounded memory growth #28756 was the older variant of this: the wrapper died but the timer held a reference on the native signal.heapStats().objectTypeCounts.AbortSignalfrombun:jscis the number of liveAbortSignalwrappers in the JS heap after the last collection.