Conversation
process.config.variables.asan is always 0 in Bun, so common.isASan never fired and Node tests that skip under ASAN ran on the ASAN lane. test-crypto-dh-leak.js measures RSS growth, and the ASAN quarantine holds the freed BIGNUMs, so the growth sits at the 20 MB limit and flakes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughASan detection now supports Node build variables and Bun runtime or executable checks. The result is computed lazily, cached, and exposed through the ChangesASan Detection
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This updates vendored Node test ASAN detection to use Bun runtime status lazily and cache the result, enabling ASAN-specific test behavior without an identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:40 AM PT - Sep 6th, 2026
❌ @robobun, your commit 7ae9833 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 41518That installs a local version of the PR into your bun-41518 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Loading bun:internal-for-testing evaluates about a dozen internal modules. Do it only when a test reads common.isASan.
|
Addressed the one review note in a391a91: the ASAN probe is now lazy, so the vendored tests no longer load |
There was a problem hiding this comment.
LGTM — the follow-up commit made the probe lazy, which addresses the eager bun:internal-for-testing load I flagged earlier.
What was reviewed:
getIsASan()is now called only via theget isASan()accessor, so the ~2800 tests that never read it don't pay the internal-module cost.- Confirmed
isASANEnabledis exported as a function fromsrc/js/internal-for-testing.ts:659, and all three consumers (test-crypto-dh-leak.js,test-crypto-secure-heap.js,test-v8-serialize-leak.js) access it ascommon.isASan, so the data-property → getter switch is compatible. - Fallback chain is sound:
process.config.variables.asan→ gated runtime probe →bun-asanbinary-name check viapath.basename; the empty catch is scoped and commented.
Extended reasoning...
Overview
This PR modifies only test/js/node/test/common/index.js, the vendored Node.js test harness. It changes common.isASan from an eager constant (process.config.variables.asan === 1, always false in Bun) to a lazy, memoized getter that probes isASANEnabled() from bun:internal-for-testing and falls back to inspecting the executable name for bun-asan. The goal is to let upstream Node tests that already guard on common.isASan (dh-leak, secure-heap, v8-serialize-leak) actually skip on Bun's ASAN CI lane, where quarantine inflates RSS past the test's fixed threshold.
Security risks
None. This is test-harness-only code with no effect on shipped runtime behavior. No untrusted input is parsed; the try/catch swallows only the expected "module gated" failure and is commented as such. No auth, crypto, or filesystem-write paths are touched.
Level of scrutiny
Low. The change is ~15 lines in a test support file, follows the vendored-test convention of marking Bun-specific deviations with a // Bun: comment, and uses the bun:internal-for-testing mechanism REVIEW.md explicitly endorses for making tests observable without production changes. My earlier review's one concern — that the probe ran at module load and would eagerly pull in the large exposedInternals graph for every one of the ~2800 parallel Node tests — was directly addressed in the second commit by moving evaluation behind a getter with memoization.
Other factors
I verified isASANEnabled exists as export const isASANEnabled: () => boolean in src/js/internal-for-testing.ts, so the typeof === 'function' guard and call are correct. All three in-tree consumers read the value as common.isASan (property access on the exported object), so converting from a data property to an accessor is transparent to them — destructuring would also work since getters fire on destructure. path is already required at the top of common/index.js, so the fallback introduces no new import. The change doesn't weaken any test on non-ASAN lanes: release/musl/Windows still run the leak assertions, which is the point.
|
Status: every test lane passed on build 110952, including the x64-asan lane where test-crypto-dh-leak.js now skips. The only red job is binary-size. It compares against main #110907 (ae7b8f4, #41330 builds JSC from source), and this branch is based on ee8f984, before that change. A test-only diff cannot change the binary size, so a rebase clears it. Ready for review. |
This branch has not been deployed
1 participant
Problem
test/js/node/test/parallel/test-crypto-dh-leak.jsfails on the Debian 13 x64-asan lane withAssertionError: before=276508672 after=299560960. The test asserts RSS growth below 20 MB across 50ksetPublicKeyandsetPrivateKeycalls. Builds 110810 and 110828 both saw about 22 MB.DH_set0_key, and ASAN keeps freed memory in quarantine instead of reusing it. Locally the same test shows 15 to 18 MB underbun bd, 7 MB withASAN_OPTIONS=quarantine_size_mb=1, and 4 MB on a release build.common.isASan. Bun'stest/js/node/test/common/index.js:311derives it fromprocess.config.variables.asan === 1, andBunProcess.cpp:2924always reports 0 (a 1 makes node-gyp build addons with-fsanitize=address). So the skip never fired.Fix
common.isASanis now a lazy getter. It asks the runtime throughisASANEnabledfrombun:internal-for-testing, the same probetest/harness.tsuses. The node test runner already setsBUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1, and debug builds expose the module unconditionally. Without the module it falls back to thebun-asanbinary name. The getter is lazy because loadingbun:internal-for-testingevaluates about a dozen internal modules, andrequire('../common')must stay cheap for every vendored test.common.isASan:test-crypto-secure-heap.jsnow skips under ASAN like Node, andtest-v8-serialize-leak.jstakes its ASAN branch with the same bound.bun bd test/js/node/test/parallel/test-crypto-dh-leak.jsprints1..0 # Skipped: ASan messes with memory measurements. The release binary still runs the test and passes.test-crypto-secure-heap.js,test-v8-serialize-leak.js, andtest-crypto-dh-odd-key.jspass underbun bd.Background
bun:internal-for-testingis a built-in module that is gated behindBUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1in release builds.isASANEnabled()is a side-effect-free#if ASAN_ENABLEDprobe insrc/jsc/bindings/InternalForTesting.cpp.no test proof · iteration 2 · no src or test change; test-proof not applicable