ci: quarantine worker transfer-terminate tests on ASAN; widen node-net mimalloc threshold - #34686
Conversation
…t mimalloc threshold Three test files have been turning unrelated PR builds red on main: - test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js SIGABRTs on the debian 13 x64-asan lane with JSC's "ASSERTION FAILED: !scope.exception() || !result" in getOwnPropertyDescriptor. worker.terminate() lands while a process.* lazy PropertyCallback builder is in JS; tryClearException() refuses to clear the TerminationException, so the builder returns with it pending. Tracked in #34095; fix PRs #33966 and #33418. Release lanes are unaffected (e.g. builds 75570, 75604). - test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts is the bun-owned amplified form of the above and SIGABRTs with the same assertion on the same lane (e.g. builds 75597, 75604, 75606). Both are quarantined with [ ASAN ] so they keep running on every release lane. Remove both entries once either fix PR lands. - test/js/node/net/node-net.test.ts "should not leak when connect({path}) fails synchronously on a reused handle" asserts the mimalloc page delta across two equal runs is < 10. Since #34009 JSC shares mimalloc, and on aarch64 release lanes (alpine 3.23, debian 13, ubuntu 25.04) its heap adds +10..13 of noise over the measured run (builds 75570, 75589, 75562). The socket leak this test guards against shows +25, so the threshold is raised to 18: well clear of both the observed noise ceiling and the leak signal. The file stays in the run on every lane. test-net-connect-memleak.js (the fourth file in the original report) is linux-x64-musl only and handled by #34631.
|
Test-infra only; no source changes, so no fail-before/pass-after proof applies. Diff is green. Across both CI runs (builds 75622 and 75654), none of the three files this PR touches appear in any failure annotation: both worker tests are correctly skipped on x64-asan and Remaining red on both builds is the same two
Everything else was retry-passed flake. Ready for a maintainer to merge. Self-review corrections (in 2936941): the stress test's CI crashes are all |
|
Warning Review limit reached
Next review available in: 5 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 (2)
Comment |
|
Updated 11:46 PM PT - Jul 18th, 2026
❌ @robobun, your commit 2936941 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34686That installs a local version of the PR into your bun-34686 --bun |
There was a problem hiding this comment.
LGTM — test-infra only, no runtime code touched.
What was reviewed:
- Both new
[ ASAN ]entries inexpectations.txtname real files, match the existing format/modifier, and carry the required comment naming the tracked issue (#34095) and fix PRs. - Substring matching won't over-quarantine: neither path is a prefix of any other test file.
- The mimalloc threshold widening (10 → 18) still sits well below the +25-page leak signal the test guards against, so the assertion can still fail for the right reason.
Extended reasoning...
Overview
Two files changed, both in test/:
test/expectations.txt: adds two[ ASAN ]quarantine entries fortest-worker-message-port-transfer-terminate.jsandworker-transfer-terminate-stress.test.ts, with a detailed comment block explaining the JSC!scope.exception() || !resultassertion, the tracking issue (#34095), and the open fix PRs (#33966, #33418).test/js/node/net/node-net.test.ts: raises one leak-test threshold fromtoBeLessThan(10)totoBeLessThan(18)and updates the accompanying comment to explain the JSC-shares-mimalloc noise since #34009.
No source code is modified; this is purely CI quarantine + a de-flake threshold adjustment.
Security risks
None. No runtime code, no auth/crypto/permissions, no user-facing behavior change.
Level of scrutiny
Low. This is a mechanical test-infra change that follows the file's own header rules exactly: whole-file crashes go in expectations.txt with a modifier and comment; a single flaky case is fixed in-file rather than quarantining the whole file. Both patterns are applied correctly here.
I verified:
- Both quarantined test files exist at the stated paths.
- The
[ ASAN ]modifier and[ CRASH ]expectation match the surrounding entries' format exactly, so the runner's substring parser will pick them up on the ASAN lane only (release lanes keep running both). - The entries are full unique paths, so substring matching won't accidentally quarantine sibling tests.
- The threshold change preserves the test's purpose per REVIEW.md's de-flaking rule ("keep asserting the property the original assertion protected"): the guarded leak produces +25 pages on release, the observed noise ceiling is +13, and 18 sits between them with clear separation on both sides.
Other factors
The PR body is thorough — it cites the Buildkite survey (builds 75470–75621), per-lane hit counts, the exact assertion text, and a per-lane filtering verification against scripts/runner.node.mjs. Every skip/weakening carries a stated reason and a removal condition, satisfying the repo's "never silently weaken" rule. No prior human reviews or outstanding comments to address.
… for darwin noise Self-review surfaced two factual corrections: - The stress test's CI crashes are all ExceptionScope.h:61 assertNoException (6/6 observed), not the JSObject.cpp:3936 path tracked in #34095. PR #33966's Verification section reports that path still reproducing at ~1/4000 workers AFTER its lazy-builder fix. Give the stress-test entry its own tracker (#34690) and stop claiming it is removable with the vendored-test entry. - The error-only annotation sweep missed a style=warning (retry-passed) hit on darwin 26 aarch64 at delta=14 in build 75589, the very build the previous comment cited. Raise the threshold to 20 so the margin above the observed noise ceiling matches the margin below the +25 leak signal, and correct the comment.
There was a problem hiding this comment.
LGTM — test-infra only, no source changes.
What was reviewed:
- Both
[ ASAN ]quarantine entries follow the file's format, cite tracking issues (#34095/#34690) and removal conditions, and are scoped so release lanes keep running the tests. - The mimalloc threshold widening (10→20) still sits below the +25 leak signal the test guards against, so the assertion can still fail for the right reason.
- Checked substring-match semantics in
expectations.txt— the two new paths don't prefix-match any unrelated test files.
Extended reasoning...
Overview
Two-file test-infra change: (1) test/expectations.txt gains two [ ASAN ] quarantine entries for test-worker-message-port-transfer-terminate.js and worker-transfer-terminate-stress.test.ts, both of which SIGABRT on the debian-13 x64-asan lane with a known JSC TerminationException-during-lazy-PropertyCallback assertion; (2) test/js/node/net/node-net.test.ts widens one leak-regression threshold from < 10 to < 20 mimalloc pages and updates the explanatory comment.
Security risks
None. No runtime, native, or built-in JS code is touched — only a test expectations list and a numeric threshold in a test assertion.
Level of scrutiny
Low. This is a CI-flake quarantine PR following the exact patterns already established in both files. The expectations.txt header explicitly documents when file-level quarantine is appropriate (whole-file crash/hang) vs. in-file skip (single case), and the PR applies each correctly: the two crashing worker tests get [ ASAN ] file-level entries (they SIGABRT the whole process), while the single flaky node-net case is fixed in-file by adjusting the threshold rather than quarantining the entire 40+-test file.
Other factors
- Per REVIEW.md's "never silently weaken a test" rule: both quarantine entries carry detailed comments naming the observed assertion, build numbers, tracking issues, fix PRs, and explicit removal conditions. The stress-test entry even documents that it is not removable with the first entry (separate root cause per #33966's findings).
- The threshold change preserves the test's ability to catch the regression it guards: the comment and PR body state the actual leak produces +25 pages (release) / +163 (debug+ASAN), and observed JSC-shared-mimalloc noise tops out around +13–14. A threshold of 20 keeps clear separation on both sides.
- Minor: the PR description says "raising the threshold to 18" but the code (and the second commit message) landed on 20 to accommodate darwin noise — the code is internally consistent and the discrepancy is just stale PR-body text.
- Entries match by substring per the file header; both new paths are full and specific enough not to accidentally quarantine sibling tests.
- No prior reviews from me on this PR; no outstanding human reviewer comments.
|
The test runs an Same signature as the stress-test entry you're adding here, so it likely belongs under the same #34690 tracker. Worth adding a third |
Audited every `[ ASAN ]` entry in `test/expectations.txt` and every entry in `test/no-validate-exceptions.txt` against a release-asan build at 9dc6c37. Each test was run under four configs (bare ASAN / +validateExceptionChecks / +LeakSanitizer / full CI), and every removal candidate was re-verified 3x. ### `test/expectations.txt` (11 `[ ASAN ]` entries removed) No test in this file reproduces an AddressSanitizer heap error anymore. Removed entries: | Test | Was | Now | |---|---|---| | `worker_threads/worker_threads.test.ts` | CRASH (bad free) | bare clean; flaky LSAN leak 1/4 → stays in `no-validate-leaksan.txt` | | `worker_threads/worker_destruction.test.ts` | CRASH (bad free) | bare clean; test-body timeout under `BUN_DESTRUCT_VM_ON_EXIT` → stays in `no-validate-leaksan.txt` | | `node/watch/fs.watch.test.ts` | CRASH (bad free) | bare clean; test-body timeout under `BUN_DESTRUCT_VM_ON_EXIT` → added to `no-validate-leaksan.txt` | | `test-worker-unref-from-message-during-exit.js` | CRASH (use-after-poison) | stable clean 3/3 under full CI config | | `test-fs-watch.js` | CRASH (use-after-poison) | stable clean 5/5 | | `test-fs-watch-recursive-watch-file.js` | CRASH (use-after-poison) | stable clean 5/5 | | `test-fs-promises-watch.js` | CRASH (use-after-poison) | stable clean 5/5 | | `cli/test/parallel.test.ts` | TIMEOUT | stable clean 3/3, ~22s under full config | | `cli/test/isolation.test.ts` | TIMEOUT | stable clean 3/3, ~6s under full config | | `bun/io/bun-write-leak.test.ts` | LEAK | stable clean 3/3, ~2s | | `test-net-error-twice.js` | SKIP (slow write) | stable clean 3/3, ~0.5s | Left in place: the two `transfer-terminate` entries added earlier today (#34686, known ~1/4000 flake); the next-pages / next-auth / napi / inspect / tls-sql / spawn / type-export entries (still fail under at least one config or could not be verified locally); the four `[ LEAK ]` entries that hit their 5s test-body timeout under ASAN. ### `test/no-validate-exceptions.txt` (63 lines removed) 60 entries now pass 3x under `BUN_JSC_validateExceptionChecks=1` on a release-asan build, plus 2 entries for files that no longer exist (`cli/install/bun-repl.test.ts` removed in fa3a30f, `node/test/system-ca/test-native-root-certs.test.mjs`), plus one orphaned section header. The `vendor/elysia/*` entries are left as-is (repo is cloned in CI via `test/vendor.json`, not present locally). ### `test/no-validate-leaksan.txt` Added `test/js/node/watch/fs.watch.test.ts` (test-body timeout under `BUN_DESTRUCT_VM_ON_EXIT`, no sanitizer report). Removed `test-fs-watch.js`, `test-fs-watch-recursive-watch-file.js`, and `test-fs-promises-watch.js` (stable clean 5/5 under LSAN). ### Remaining unchecked-exception sites in Bun code The still-failing entries in `no-validate-exceptions.txt` cluster around these throw → unchecked pairs in `src/jsc/`: | Throw | Unchecked at | Repro | |---|---|---| | `NapiClass.cpp:120` finishCreation | `JSObject::defineOwnNonIndexProperty` | 33× napi tests, `require-cache.test.ts` | | `napi_create_function` napi.cpp:954 | `napi_set_named_property` :598 etc. | addon Init() pattern (#32911) | | `defaultBunSQLObject` BunObject.cpp:319 | itself | `BunObject.test.ts`, `import-meta.test.js`, `resolve.test.ts` | | `JSObject::putInlineSlow` | `Process_functionDlopen` BunProcess.cpp:397 | napi `4_object_factory`, `5_function_factory` | | `jsString` | `jsFunctionWrap` NodeModuleModule.cpp:220 | `node-module-module.test.js` | | `jsSubstring` | `jsFunctionNodeModuleModuleConstructor` NodeModuleModule.cpp:172 | `module-resolve-filename-paths.test.js` | | `isArraySlowInline` | `determineSpecificType` ErrorCode.cpp:348 | `isArray-proxy-crash.test.ts` | | `importModuleInner` NodeVM.cpp:303 | `moduleLoaderImportModuleInner` NodeVM.cpp:1688 | `test-vm-module-referrer-realm.mjs` | | `JSGenericTypedArrayView::create` | `jsPublicKeyObjectPrototype_export` :40 | `node-crypto.test.js` | | `convertDictionaryToJS` JSURLPatternInit.cpp:148 | `convertURLPatternInputToJS` JSURLPatternResult.cpp:89 | `urlpattern.test.ts` | | `normalizeCryptoAlgorithmParameters` SubtleCrypto.cpp:135 | `JSDOMPromiseDeferred::reject` :194/:159 | webcrypto tests | | `evaluateWithScopeExtension` JSInjectedScriptHost.cpp:120 | `...PrototypeFunctionEvaluateWithScopeExtension` :275 | `inspect.test.ts` (WebKit) | | `JSOrderedHashTable::getImpl` | `executeBoundCall` Interpreter.cpp:1223 | next-pages tests (WebKit) |
What
Stops three test files from turning unrelated PR builds red on main. All three are pre-existing failures that the CI annotation tagger marks
[pre-existing]on every PR that hits them.Surveyed builds 75470–75621 via the Buildkite
/annotationsAPI:test-worker-message-port-transfer-terminate.jsJSObject.cpp:3936(11/12) /JSCJSValuePropertyInlines.h:51(1/12)worker-transfer-terminate-stress.test.tsExceptionScope.h:61 assertNoException(6/6)node-net.test.ts(one case)Worker transfer-terminate (ASAN quarantine)
Two distinct crash paths, both x64-asan only, release lanes unaffected:
Vendored test (
test-worker-message-port-transfer-terminate.js):worker.terminate()lands while aprocess.*lazyPropertyCallbackbuilder (stdout/stderr/stdin/nextTick/mainModule, viasetupWorkerStdio) is in JS;tryClearException()refuses to clear theTerminationException, so the builder returns with it pending andreifyStaticPropertyreports the slot found, tripping:Tracked in #34095; fix PRs #33966 and #33418. Remove this entry once either lands. Verbatim upstream port we do not edit.
Stress test (
worker-transfer-terminate-stress.test.ts): the bun-owned 8×10-worker amplification of the above, but on CI it only ever hits:6/6 observed (builds 75493/75495/75514/75597/75604/75606). #33966's Verification section reports this path still reproducing at ~1/4000 workers after its lazy-builder fix ("termination landing later in the bootstrap, after the stdio builders have completed"), so it is not the same bug. Tracked separately in #34690; this entry is not removable with the vendored-test entry.
node-net.test.ts threshold
Only one case fails:
should not leak when connect({path}) fails synchronously on a reused handle. It asserts the mimalloc page delta across two equal 8000-iteration runs is< 10.Since #34009 JSC shares mimalloc, and on aarch64/darwin release lanes its heap adds up to +14 of noise over the measured run even after the equal-size warmup (observed deltas across
style=error+style=warningannotations: 10, 10, 10, 10, 10, 10, 10, 11, 11, 11, 11, 11, 12, 12, 12, 13, 13, 14). The native-socket leak this test guards against shows +25 per 8000 iterations on release, so raising the threshold to 20 keeps clear separation from both the observed noise ceiling and the leak signal. The file stays in the run everywhere; per theexpectations.txtheader, a single broken case is fixed in-file, not quarantined.Not in this PR
test/js/node/test/parallel/test-net-connect-memleak.js(the fourth file in the original report) fails only onlinux-x64-muslwith the onGC/FinalizationRegistry timing issue and is handled by #34631 (which also covers itstest-gc-http-client*siblings) and #33045.Verification
Parsing
test/expectations.txtwith the runner's own filtering (replicated fromscripts/runner.node.mjsgetRelevantTests):Build 75622 on this branch: neither worker test appeared in the x64-asan lane's annotations and
node-net.test.tsdid not appear on any lane, so the filtering and threshold are working. The remaining red on that build was pre-existing (node-http-server-socket-end-drain.test.tson darwin 14 x64,test-net-connect-memleak.json alpine x64; both owned by other sessions) plus a darwin 26 tart-VM provisioning failure.No source changes; this is test-infra only, so there is no fail-before/pass-after to capture.
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.