Conversation
WalkthroughChangesAbortSignal leak test
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:04 AM PT - Jul 29th, 2026
❌ @robobun, your commit 862e37c has 3 failures in
🧪 To try this PR locally: bunx bun-pr 36206That installs a local version of the PR into your bun-36206 --bun |
![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 with strictly tighter assertions; the RSS-threshold nit below is a tidy-up, not a blocker (the heapStats AbortSignal count remains the load-bearing detector with ~1500× headroom).
Extended reasoning...
Overview
Test-only change to test/js/node/fs/abort-signal-leak-read-write-file{.test,-fixture}.ts. Drops the fixture loop from 100k → 5k iterations, replaces empty catch {} blocks with an explicit AbortError check, swaps .toRun() for Bun.spawn with concurrent stdout/stderr/exited draining and structured JSON assertions, and adds a 30s per-test timeout so debug+ASAN completes instead of silently timing out at 5s.
Security risks
None. No production code touched; the fixture writes only to a tmpdirSync() path and spawns nothing external.
Level of scrutiny
Low-to-medium. This is a leak-test refactor with no runtime code changes. The key question is whether the reduced iteration count still catches the #16788 regression — it does: 5k iterations × 3 signals would surface ~15,000 live AbortSignal objects against a threshold of 10, so the heapStats check fails loudly. The one weakened assertion is the trailing RSS cap (see inline nit), which was already a secondary backstop.
Other factors
- The rewrite is strictly stricter than before: previously all rejections were swallowed; now a non-
AbortErroroutcome fails the test and is surfaced in the JSON summary. - Follows repo test conventions:
bunEnv/bunExe,await using proc,Promise.allover stdout/stderr/exited, stderr/stdout asserted beforeexitCode. JSON.parse(stdout)is safe here — the fixture emits exactly oneconsole.log(JSON.stringify(...))line andbunEnvquiets debug logs.- PR description includes measured before/after timings across four build configurations, indicating the author actually ran it.
There was a problem hiding this comment.
LGTM — the vacuous RSS cap I flagged earlier was dropped in 1f20728, and the remaining heapStats AbortSignal-count check still has ~600× headroom against the #16788 regression at 2,000 iterations.
What was reviewed:
- Confirmed all three original code paths (pre-aborted readFile, pre-aborted writeFile, abort-during-writeFile) are preserved.
- The
.then(() => nonAbortErrors++, check)vs.catch(check)split correctly allows the race-y third case to resolve successfully without flagging. - Spawn wrapper drains stdout/stderr concurrently and asserts output before exit code per harness conventions; dead
isASANimport removed.
Extended reasoning...
Overview
Test-only change to test/js/node/fs/abort-signal-leak-read-write-file{.test,-fixture}.ts. The fixture's iteration count drops 100k → 2k, empty catch blocks become AbortError assertions, and the test wrapper switches from .toRun() to an explicit Bun.spawn that parses a JSON summary and asserts on stderr / AbortSignal count / non-abort-error count before the exit code. The absolute RSS cap was removed in the follow-up commit after my earlier note that it could no longer trip at the reduced iteration count.
Security risks
None. Pure test-infrastructure change with no runtime code touched.
Level of scrutiny
Low-to-moderate. The main REVIEW.md concern for this class of change is "never silently weaken an existing test". I checked each removed assertion:
- The 100k → 2k reduction: the load-bearing detector is
heapStats().objectTypeCounts.AbortSignal > 10. If #16788 regresses, 2,000 iterations × 3 signals ≈ 6,000 live signals vs. threshold 10 — still fails decisively. The extra 98k iterations added no detection headroom. - The dropped RSS cap: at 2k iterations the leak's RSS delta (~11 MB release / ~20 MB ASAN, per the author's measurement) sits inside normal jitter, so the cap was already vacuous. Dropping it with a stated reason satisfies the "every deletion needs a stated reason" rule; RSS stays in the JSON output for diagnostics.
- The empty
catch {}blocks were strengthened into anonAbortErrorscounter that fails the test if any rejection isn't an AbortError, and the pre-aborted cases now also fail if the promise unexpectedly resolves.
Other factors
- My prior inline comment is resolved by 1f20728.
- Harness conventions are followed:
bunEnv/bunExe,Promise.allover both pipes andproc.exited, stderr/stdout asserted before exit code, per-test 30s timeout instead of a file-wide default. - The abort-during-writeFile case correctly uses
.catch(check)(not.then(fail, check)) since the write may legitimately complete beforenextTickfires the abort — avoids a flake the stricter form would introduce. - No production code, no CODEOWNER paths, no outstanding reviewer comments.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the earlier RSS-cap nit was addressed in 1f20728/38b72e21 and the detector is now demonstrably live.
What was reviewed:
- New never-aborted-signal-with-listener path against
JSAbortSignalCustom.cppsemantics — it's the only shape thathasPendingActivity()pins, so the heapStats threshold can now fail. - Abort-later
writeFilepath uses.catch(mustAbort)(not.then(bump, mustAbort)), so a write that races ahead ofnextTickabort correctly counts as success rather than a non-abort error. - Spawn wrapper drains stdout/stderr/exited concurrently and asserts stderr/counts before exit code;
bunEnvkeeps debug logging off sostderr === ""holds underbun bd.
Extended reasoning...
Overview
Test-only change to test/js/node/fs/abort-signal-leak-read-write-file{.test,-fixture}.ts. Cuts the fixture from 100k iterations to 200 (debug/ASAN) / 2,000 (release), adds a never-aborted signal + abort listener path so the heapStats AbortSignal count can actually trip on a missing pending_activity_unref(), replaces empty catch {} blocks with an AbortError-name check, drops the now-unfalsifiable RSS cap (keeping RSS in the diagnostic JSON), and swaps .toRun() for an explicit Bun.spawn that parses the JSON summary and asserts stderr / counts / exit code in the recommended order.
Security risks
None. Test-only; no production code touched, no network, no untrusted input.
Level of scrutiny
Low-to-moderate. The risk with leak-test edits is silently weakening coverage. Here the opposite happened: the PR description shows the old heapStats assertion could not fail on the #16788 regression (every signal was aborted by GC time, so hasPendingActivity() never pinned the wrapper), and the new never-aborted-with-listener path was empirically verified to report ~200 live wrappers vs the threshold of 10 when pending_activity_unref() is removed. That satisfies REVIEW.md's "confirm deleting each load-bearing clause of your fix breaks at least one test" for the detector itself.
Other factors
- My earlier inline comment about the vacuous RSS cap was addressed (dropped, with measured justification that no RSS bound distinguishes leak from healthy at this iteration count).
- Checked the abort-later path for a false-positive:
.catch(mustAbort)(rather than.then(onFulfilled, mustAbort)) means a write that completes beforeprocess.nextTickfires is treated as success, not a non-abort error — correct. - Iteration scaling on
isDebug || isASANkeeps the fixture under the default 5s test timeout (~3.2s reported for debug+ASAN) without a per-test override. ?? 0onobjectTypeCounts.AbortSignalavoidsundefined > 10silently passing if the type key is absent.
The fixture ran 100,000 iterations, which cost ~17s on the Windows aarch64 lane and several minutes under debug+ASAN (so the test was silently timing out under 'bun bd test' locally). The primary assertion is the heapStats AbortSignal object count, which would report ~15,000 live signals at 5,000 iterations if the #16788 regression ever came back, versus the threshold of 10. Cut the loop to 5,000, keep all three code paths (pre-aborted readFile/writeFile and abort-during-writeFile), and additionally assert that every rejection is actually an AbortError rather than swallowing all errors. Replace the opaque .toRun() with an explicit Bun.spawn that surfaces the live-signal count, RSS, and any unexpected stderr on failure.
At the reduced iteration count the #16788 leak would only add tens of MB of RSS, so the 200/700 MB absolute cap could no longer fail on the bug it was written for. The heapStats AbortSignal count is the load-bearing detector; keep RSS in the JSON summary for diagnostics only. 2,000 iterations keeps the object-count headroom at ~600x while leaving the debug+ASAN subprocess comfortably under the per-test timeout.
Empirically verified that the existing heapStats AbortSignal count was never able to fail on the #16788 regression: JSAbortSignal wrappers are only pinned by hasPendingActivity() when the signal is not aborted AND has an abort listener, and every signal in the old fixture was aborted by the time GC ran. Add a never-aborted-signal-with-listener path to the loop. With pending_activity_unref() removed from ReadFile/WriteFile Drop, this reports ~200 live wrappers at 200 iterations vs ~3 with the fix in place, so the object-count assertion is now a real detector. Scale ITERATIONS by isDebug/isASAN (200 vs 2,000) instead of carrying a per-test timeout, matching the pattern used by other leak tests.
38b72e2 to
4090c23
Compare
There was a problem hiding this comment.
LGTM — test-only change that makes the detector actually fail on the regression it guards, and the earlier RSS-cap feedback was addressed. The inline note about the stale expectations.txt ASAN quarantine is worth cleaning up (otherwise the isASAN branch never runs in CI), but it's not a regression from today and doesn't block.
Extended reasoning...
Overview
Two test files: the abort-signal-leak-read-write-file fixture and its wrapper test. The fixture drops from 100k to 200/2,000 iterations (branched on isDebug || isASAN), adds a fourth loop path (never-aborted signal + listener) so heapStats().objectTypeCounts.AbortSignal can actually exceed the threshold when pending_activity_unref() is missing, replaces empty catch {} blocks with an AbortError name check, and drops the now-vacuous RSS cap (per my earlier comment, which the author addressed with measurements). The wrapper test replaces .toRun() with an explicit Bun.spawn that drains stdout/stderr concurrently, parses the JSON summary, and asserts stderr/counts before exitCode.
Security risks
None. Test-only; no production code, no untrusted input, no network.
Level of scrutiny
Low-to-moderate. It's test infrastructure, but leak tests are easy to make vacuous. The key claims check out: the author verified the new detector fires (203 vs 3) by reintroducing the leak; the abort-during-write path is race-tolerant (.catch(mustAbort) only fires on rejection, so a fast write that completes before nextTick doesn't spuriously bump nonAbortErrors); the never-aborted detector path can't reject, so no error handling is needed there; bunEnv keeps stderr clean for the toBe("") assertion.
Other factors
- The one open finding is a nit:
test/expectations.txt:88still quarantines this file on ASAN with a comment naming the old test title. Worst case the ASAN lane keeps skipping the file exactly as it does today — nothing regresses. - Harness conventions followed:
Promise.allon stdout/stderr/exited,bunEnv/bunExe, exit code asserted last, iteration count branched onisDebug/isASAN, no per-test timeout override. - Previous review round's feedback (RSS cap) was resolved with a reasoned reply and code change.
The entry was added when the RSS check tripped under ASAN's elevated baseline. That check is gone and the fixture now runs ~3s under debug+ASAN, so there is no reason to keep skipping the file there.
There was a problem hiding this comment.
LGTM — both earlier nits (the vacuous RSS cap and the stale ASAN quarantine) are addressed, and the detector now demonstrably fails on the reintroduced leak.
What was reviewed:
- Verified the new never-aborted-signal-with-listener path is what makes
heapStatspin the wrapper per thehasPendingActivitycontract — the old fixture's all-aborted signals could never trip it. - Checked the abort-during-
writeFilepath's.catch(mustAbort)correctly tolerates the write completing beforenextTickfires (4-byte write), sononAbortErrorswon't false-positive there. - Confirmed
bunEnv/bunExe, concurrent pipe drain, and exit-code-last ordering match harness conventions;JSON.parse(stdout)is safe since the fixture writes exactly one line.
Extended reasoning...
Overview
Test-only PR touching three files: rewrites abort-signal-leak-read-write-file-fixture.ts to use a heapStats-based detector that can actually fail on the #16788 regression, replaces the .toRun() wrapper in the test file with an explicit Bun.spawn that parses a JSON summary and asserts on stderr / AbortSignal count / non-abort-error count before the exit code, and removes the now-obsolete [ ASAN ] quarantine from test/expectations.txt. No production code is touched.
Security risks
None. The change is confined to a leak regression test and a CI quarantine list; there is no user-facing surface, no parsing of untrusted input, and no auth/crypto/permissions code.
Level of scrutiny
Low-to-moderate. This is test infrastructure with no runtime impact. The main risks for a change like this are (a) weakening the assertion the test was guarding, or (b) introducing flake. Both were examined: the PR description includes a direct verification that reintroducing the leak (pending_activity_unref() removed) yields numAbortSignalObjects: 203 against a threshold of 10, so the detector is strictly stronger than before (the old heapStats check was shown to stay at 3 under the same regression). The RSS cap that was dropped was already vacuous at the reduced iteration count, per the earlier review thread and the author's measurements. The un-quarantine is justified by the ~3s debug+ASAN timing and the file remaining in no-validate-leaksan.txt for LSan.
Other factors
Both prior review comments from earlier runs were addressed with follow-up commits and the threads are resolved. The test follows repo conventions: bunEnv/bunExe, Promise.all on stdout/stderr/exited to avoid pipe deadlock, ITERATIONS branched on isDebug || isASAN, stdout/stderr asserted before exit code. The .then(() => nonAbortErrors++, mustAbort) shape on the pre-aborted paths correctly counts an unexpected resolution as a failure, and the .catch(mustAbort) on the abort-during-write path correctly tolerates the write winning the race with process.nextTick. The fixture still uses tmpdirSync rather than tempDir, but that predates this PR and isn't a regression.
There was a problem hiding this comment.
LGTM — both earlier nits (vacuous RSS cap, stale ASAN quarantine) are addressed and the current diff is a clean test-only improvement.
What was reviewed:
- Fixture's new never-aborted-signal-with-listener path against
hasPendingActivity()semantics — it's the case that actually pins the wrapper, and the author empirically proved the threshold of 10 trips (203 live) withpending_activity_unref()removed. - Abort-later
writeFilepath uses.catch(mustAbort)without asserting rejection — correct, since a 4-byte write can legitimately complete beforenextTick. - Test wrapper follows harness conventions (
bunEnv/bunExe, concurrent pipe drain, stderr asserted beforeJSON.parseandexitCode);no-validate-leaksan.txtstill covers LSan so dropping theexpectations.txtline only removes the timeout-era skip.
Extended reasoning...
Overview
Test-only change touching three files: rewrites the fs.promises.readFile/writeFile AbortSignal leak fixture (abort-signal-leak-read-write-file-fixture.ts), replaces the .toRun() wrapper with an explicit Bun.spawn that parses a JSON summary (abort-signal-leak-read-write-file.test.ts), and removes one stale [ ASAN ] quarantine line from test/expectations.txt. No runtime code is touched.
Security risks
None. Test infrastructure only; no user-facing surface, no parsing of untrusted input, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. It's test-only, but leak-detector tests are easy to get subtly wrong (vacuous assertions, thresholds that never trip). The author did the right thing here: they reintroduced the #16788 leak locally by commenting out pending_activity_unref() in ReadFile::drop/WriteFile::drop, showed the old heapStats check couldn't fail (every signal was aborted by GC time, so hasPendingActivity() returned false regardless), and demonstrated the new never-aborted+listener path yields ~203 live wrappers vs ~3 healthy at 200 iterations. That's exactly the "prove the test fails for the right reason" bar from REVIEW.md.
Other factors
- I left two prior nits on this PR (RSS cap now vacuous at reduced iterations; stale ASAN quarantine). Both were addressed in follow-up commits (ca4226f dropped the RSS cap with measured justification; 0ff4d9c removed the quarantine). All threads are resolved.
- The wrapper follows CLAUDE.md's spawn pattern exactly:
Promise.all([stdout.text(), stderr.text(), exited]), asserts stderr first so any fixture crash surfaces beforeJSON.parsechokes, and checksexitCodelast. - The abort-during-write path deliberately does not assert rejection (only
.catch(mustAbort)) because a 4-byte write can race ahead ofprocess.nextTick— same tolerance the old fixture had, but now non-AbortError rejections are counted instead of swallowed. - Un-quarantining on ASAN is safe: the file remains in
no-validate-leaksan.txtfor LSan-at-exit, and the fixture now completes in ~3s under debug+ASAN, so the only way ASAN CI goes red is a real regression in the assertion — which is desirable. - Iteration scaling (
isDebug || isASAN ? 200 : 2_000) matches the pattern used by other leak tests in the suite and keeps the file under the default timeout without a per-test override.
Spawning the fixture under debug+ASAN carried ~2.5s of subprocess startup before the loop even ran, pushing the test past the default 5s timeout on loaded machines. The only reason for the subprocess was RSS isolation, and the RSS check is gone, so fold the detector into the test file and drop the fixture. The heapStats object-count assertion works identically in-process.
This file runs in the --parallel bucket on release CI where a worker executes several test files on one VM; wrappers left over from earlier files would count toward an absolute threshold. Use the same baseline + expectMaxObjectTypeCount pattern as pipeTo-signal-leak.test.ts.
There was a problem hiding this comment.
LGTM — test-only change; all three prior review points (RSS cap, stale ASAN quarantine, baseline-delta for parallel-bucket safety) are addressed in 862e37c.
What was reviewed:
- Detector loop uses never-aborted signals with a listener so
hasPendingActivity()actually pins leaked wrappers; author verified 102 vs ~2 with the fix reverted. .rejects.toMatchObjectcalls are awaited; the delayed-abort race accepts either outcome;expectMaxObjectTypeCounthandles GC+retry and asserts againstbaseline + 10.tempDir+usingfor cleanup; iteration count branches onisDebug || isASAN; fixture file and itsexpectations.txtentry both removed.
Extended reasoning...
Overview
Test-only PR touching three files: rewrites test/js/node/fs/abort-signal-leak-read-write-file.test.ts to run the AbortSignal leak check in-process instead of via a spawned 100k-iteration fixture, deletes the now-unused fixture, and removes the stale [ ASAN ] quarantine from test/expectations.txt. No production code is modified.
Security risks
None. This changes only test infrastructure — no runtime, auth, crypto, or user-facing code paths.
Level of scrutiny
Low-to-moderate. The bar for a test rewrite is (a) it still detects the regression it guards, (b) it doesn't flake, and (c) it doesn't silently weaken coverage. All three are satisfied: the PR description shows the new detector fails with 102 live wrappers when pending_activity_unref() is removed from ReadFile::drop and passes (~2) with it present, so the assertion is demonstrably live. The old fixture's heapStats check was shown to be vacuous (aborted signals aren't pinned by hasPendingActivity()), so this is a strict improvement in coverage, not a weakening. The three original behaviour paths (pre-aborted read, pre-aborted write, abort-during-write) are preserved as explicit AbortError assertions rather than swallowed via empty catch.
Other factors
Three rounds of review feedback from prior runs were all incorporated: the RSS check was dropped after measurement showed it couldn't distinguish the regression at the reduced iteration count; the stale expectations.txt entry was removed so the test actually runs on ASAN CI; and the absolute heapStats threshold was converted to a delta from a captured baseline via the shared expectMaxObjectTypeCount helper, matching the sibling pipeTo-signal-leak.test.ts pattern and avoiding parallel-bucket flake. The test uses tempDir/using for hermetic cleanup, awaits every .rejects, and scales iterations by isDebug || isASAN to stay under the default timeout without a per-test override. Straightforward, well-verified, and fully test-scoped.
|
The diff here is green: The red on builds 84358 and 84634 is unrelated to this test-only change:
Ready for a maintainer to merge past the unrelated red. |
This branch has not been deployed
1 participant
What
Cuts the wall time of
test/js/node/fs/abort-signal-leak-read-write-file.test.tsby ~18x on release lanes, makes it pass underbun bd testinstead of timing out, and turns theheapStatsAbortSignal check into an assertion that can actually fail on the regression it guards.Why
The existing fixture ran 100,000 iterations in a spawned subprocess and asserted two things:
heapStats().objectTypeCounts.AbortSignal <= 10andrss < 200 MB. While reviewing this I reintroduced the leak locally (removed thepending_activity_unref()fromReadFile/WriteFileDropinsrc/runtime/node/node_fs.rs) and ran the fixture: the AbortSignal count stayed at 3. AJSAbortSignalwrapper is only pinned byhasPendingActivity()when the signal is not aborted and has an abort listener (JSAbortSignalCustom.cpp), and every signal in the old fixture is aborted by the time GC runs, so the object-count assertion could never have failed on this leak. The RSS check at 100,000 iterations was the only live detector, and only by a marginal amount.The file was also timing out under
bun bd testlocally (the subprocess alone cost ~4 min under debug+ASAN against the 5s default), and was quarantined on ASAN CI for the same reason.How
abortlistener passed to bothreadFileandwriteFile. Withpending_activity_unref()removed fromReadFile::drop, the in-process test reports 102 live wrappers at 100 iterations vs ~2 with the fix in place, so theheapStatsthreshold of 10 is now a real detector.readFile, pre-abortedwriteFile, abort-during-writeFile) as single assertions that verify the rejection is anAbortError; the old fixture swallowed all errors with emptycatchblocks.isDebug || isASAN(100 vs 2,000) so debug+ASAN completes under the default test timeout without a per-test override.[ ASAN ]quarantine fromtest/expectations.txt.Verification
Commented out
signal.pending_activity_unref()inReadFile::dropand rebuilt:With a clean
src/:Timings
[stamp-90s] gate passed · iteration 3 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 3
evidence per changed file