Conversation
#40238 made bun_core::String own its WTF ref and release it on Drop. That closed three leaks that had open fix PRs with tests but no test on main: - SocketAddress: options.address on validation-error paths (#35230) - worker_threads: the cached Bun.main path at VM teardown (#38178) - bun:test: the bound test/describe function name (#38208) This adds only the tests from those branches. The LSan tests run on the ASAN lanes. The SocketAddress test measures RSS growth.
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 13 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 7:42 AM PT - Aug 24th, 2026
✅ @robobun, your commit fd4a62cf7fd9d432cc99db164ad6f0bbc204836c passed in 🧪 To try this PR locally: bunx bun-pr 40296That installs a local version of the PR into your bun-40296 --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 PR adding three leak regression tests salvaged from superseded fix PRs.
What was reviewed:
leaksan.supppath resolution from both test dirs (../../../leaksan.supp→test/leaksan.supp), and that the worker test correctly composes it with the extraWebCore::eventNamessuppression.- The pass/skip/todo/fail counts in the
bun-test.test.tsfixture — hand-counted 26/8/3/0 across all modifier variants; matches the assertion. - SocketAddress RSS test: warmup phase, per-path measurement, non-vacuous loop guard (
Object.keys(growth).length === 6), 20 MB threshold vs. ~64 MB expected on regression, ASAN quarantine disabled so RSS reflects live allocations. - Harness conventions:
bunEnvspread,Buffer.alloc().toString(),using/await using, concurrent pipe drain, combined-object assertions,skipIf(!isASAN || isWindows)gate, per-test timeouts with justifying comments.
Extended reasoning...
Overview
This PR adds three regression tests to existing test files, pinning string-leak fixes that landed as a side effect of #40238 (bun_core::String gaining a Drop impl). The tests were lifted from three now-closed fix PRs (#35230, #38178, #38208). No source code is touched.
test/js/node/net/socketaddress.spec.ts: RSS-growth test over sixSocketAddress/BlockListvalidation-error paths that populate an address string before throwing. Runs on all platforms in a spawned--smolsubprocess.test/js/node/worker_threads/worker-shutdown-post-leak.test.ts: LSan check (ASAN lanes only) that a worker's cachedBun.mainpath is released at VM teardown, for both ASCII and non-ASCII entry paths.test/js/bun/test/bun-test.test.ts: LSan check (ASAN lanes only) thatScopeFunctions::bindno longer leaks the bound function name, exercising everytest/describemodifier.
Security risks
None. Test-only additions that spawn the debug binary against local temp dirs; no network, no external inputs, no auth/crypto surface.
Level of scrutiny
Low-to-moderate. Test-only with no runtime behavior change — the risk is limited to CI flakiness or a vacuous test. I checked the specific things reviewers catch on leak tests per REVIEW.md:
- Fails for the right reason: PR description documents
USE_SYSTEM_BUN=1failure against a pre-#40238 canary (67 MB growth) and pass on current main. The two LSan tests were shown to fail on the unfixed tree in their original PRs. - Non-vacuous assertions: the SocketAddress test asserts
Object.keys(growth).length === 6so an empty result object can't pass; the bun-test fixture asserts exact result counts (I hand-verified 26 pass / 8 skip / 3 todo / 0 fail against the fixture body); the worker test asserts exact{stdout, stderr, exitCode}. - Threshold well below the unfixed leak: 20 MB vs ~64 MB, with ASAN quarantine disabled and a warmup phase to stabilize RSS.
- Harness conventions:
{...bunEnv, ...},Buffer.alloc(128*1024, "a").toString(),using/await using,Promise.allon stdout/stderr/exited,tempDirfrom harness,test.concurrentfor the independent worker subprocess pair, per-test timeouts with comments explaining LSan symbolization cost.
Other factors
- The
../../../leaksan.supprelative paths resolve correctly totest/leaksan.suppfrom bothtest/js/bun/test/andtest/js/node/worker_threads/. - The
Malloc=1+detect_leaks=1+ suppressions-file pattern matches the existing test in the same file and several others acrosstest/. - The exact-count assertion in
bun-test.test.tsis somewhat coupled to the reporter output format, but that's an intentional pin — a format change would break it visibly, not silently. - No prior review comments to address; CI is building.
The worker test was vacuous on current main: #39536 removed the bootstrap read of Bun.main, so a worker with an empty body never populates main_resolved_path. The bun:test modifier test measured a path that creates no WTF string since bind() takes a &'static str. The SocketAddress test now goes through the shared RSS helper and branches its bound on debug/ASAN builds. Fewer iterations with a larger string keep the leaked signal at about 50 MiB per path while the test runs in about 2.5 s under a debug ASAN build.
robobun
changed the title
There was a problem hiding this comment.
LGTM — test-only addition using the existing expectRssDeltaBelow harness helper. The one inline note is just PR-metadata hygiene.
Extended reasoning...
Overview
This PR adds a single leak-regression test to test/js/node/net/socketaddress.spec.ts. It spawns bun --smol -e <code> via the existing expectRssDeltaBelow harness helper and measures per-case RSS growth across six error paths where options.address is read into a native string before a later validator throws (four in SocketAddress's Options::from_js, one in init_js via pton reject, one in BlockList.addAddress). The child prints per-case deltas plus a final {deltaMiB: max} line, which the helper parses and asserts against {release: 20, debug: 30}.
Security risks
None. Test-only change; no source, no new dependencies, no network access (spawns the local bunExe() with bunEnv).
Level of scrutiny
Low-to-medium. It's a test addition to an existing spec file. I checked that: the expectRssDeltaBelow signature matches the call (cmd: string[], {release, debug}); the child's last stdout line is the {deltaMiB} object the helper's JSON.parse(...at(-1)) expects; the helper already branches on isASAN || isDebug and disables ASAN quarantine so the 30 MiB debug bound is realistic; the test uses Buffer.alloc(n, fill).toString() per repo convention; there's a 20-iteration warmup and Bun.gc(true) before each measurement window; and the 20/30 MiB thresholds sit well below the ~50 MiB an unfixed leak would produce per path.
Other factors
The PR originally added three tests but commit 3cd36e6 dropped two as vacuous — hence the inline nit about the stale title/description. That's a squash-commit-message concern only and doesn't affect the code being merged. No prior human reviewer comments to address.
There was a problem hiding this comment.
LGTM — thanks for retitling and syncing the description; the earlier concern is resolved.
What was reviewed:
- Traced each of the six cases against
Options::from_js/init_js/BlockList.addAddressinsrc/runtime/socket/SocketAddress.rs— every path readsoptions.addressinto aBunStringbefore the named validator throws. expectRssDeltaBelowcontract: last stdout line is the{deltaMiB}JSON, ASAN quarantine is disabled by the helper, and the 20/30 MiB bounds sit well below the ~50 MiB unfixed leak and above the reported 7 MiB fixed value.- Confirmed the helper is already used by ~9 other leak tests, so this follows the established pattern;
Buffer.alloc(n, fill).toString()andbig + iper-iteration match harness conventions.
Extended reasoning...
Overview
Test-only PR adding one leak-regression test to test/js/node/net/socketaddress.spec.ts. It spawns a child bun via the shared expectRssDeltaBelow harness helper and drives six SocketAddress/BlockList validation-error paths with a 512 KiB address string, asserting the max per-path RSS delta stays under 20 MiB (release) / 30 MiB (debug/ASAN). No source changes.
Security risks
None. The change is a test that spawns bun -e <code> with bunEnv and reads stdout; no network, no external data, no new API surface.
Level of scrutiny
Low-to-moderate. It is test-only and reuses an existing harness helper already in use by ~9 other leak tests. The main risks for leak tests are flakiness and vacuity, both of which the PR body addresses with concrete numbers: 7 MiB observed on a fixed debug+ASAN build vs 49–57 MiB on pre-#40238 builds, giving wide margin on both sides of the 20/30 MiB thresholds. The helper disables ASAN quarantine and branches the bound on isASAN || isDebug, which is exactly what REVIEW.md asks of leak tests.
I cross-checked the source ordering in SocketAddress.rs: Options::from_js reads address into a BunString at line ~72 before validating family, port, and flowlabel, so bad_family/bad_port/bad_flow_type/bad_flow_range all throw after the string ref exists. pton_reject and blocklist_bad_family cover the two remaining post-read throw sites named in the description. The child prints a per-case diagnostic line and then the {deltaMiB: max} line that expectRssDeltaBelow parses via .split("\n").at(-1), so the extra diagnostic line is harmless.
Other factors
My prior review comment (stale title/description) was addressed and the thread is resolved — the title and body now describe only this test, with a Notes block explaining why the two dropped tests were vacuous. The PR body records both bun bd test (67 pass) and USE_SYSTEM_BUN=1 on bun 1.4.0 (fails with 57 MiB), satisfying the fails-without-fix requirement. Harness idioms are followed: Buffer.alloc(n, fill).toString() instead of .repeat(), per-iteration big + i to defeat string dedup, warmup before measurement, Bun.gc(true) between windows, --smol.
This branch has not been deployed
1 participant
Problem
bun_core::Stringown its WTF ref and release it onDrop. One of the leaks it closed isoptions.addressinsrc/runtime/socket/SocketAddress.rs: every validation-error path after the address was read (bad family, bad port, bad flowlabel,ptonreject,BlockList.addAddresswith a bad family) left a +1 on the 128 KiB to 512 KiB string the caller passed in.Fix
test/js/node/net/socketaddress.spec.tsthat drives the six error paths with a 512 KiB address string and reports the largest per-path RSS delta.expectRssDeltaBelowfromharness(added in Make bun_core::String own its WTF ref #40238), so the ASAN quarantine handling and the debug/ASAN bound live in one place. Bounds: 20 MiB release, 30 MiB debug.Background
bun_core::Stringis the Rust mirror of the C++BunString. When it is WTF-backed it holds one ref on aWTF::StringImpl. Before Make bun_core::String own its WTF ref #40238 it was aCopyPOD with no destructor, so every +1 producer relied on the caller to callderef()by hand. Early returns through?skipped that call.expectRssDeltaBelowspawns bun with the ASAN quarantine disabled and reads adeltaMiBJSON line from stdout. The quarantine pins freed blocks, which would hide a fixed leak behind a large RSS.Notes
Bun.mainpath) and bun:test: stop leaking the name of every bound test/describe function #38208 (boundtest/describefunction name). Both were dropped after review.Bun.mainin node-kind workers, so a worker with an empty body never populatesmain_resolved_path. A pre-Make bun_core::String own its WTF ref #40238 ASAN build passes that test for both the ASCII and the non-ASCII case. A worker body that readsBun.mainfrom a non-ASCII path does report the leak on that build (74 bytes fromString::clone_utf8inBunObject_getter_main) and is clean on main. The ASCII case is an atom and is hidden by theleak:JSC::Identifier::fromStringsuppression. That pin belongs with the worker LSan table in worker: free the thread's event name table when the worker thread exits #38164, which touches the same file.bindinScopeFunctions.rstakes a&'static strand passes it toJSFunction::createwithout a clone.bun bd test test/js/node/net/socketaddress.spec.ts(67 pass, 1 skip, 0 fail).USE_SYSTEM_BUN=1 /usr/local/bin/bun test js/node/net/socketaddress.spec.ts -t "does not leak the address string"on bun 1.4.0 fails withReceived: 57.no test proof · iteration 1 · no src or test change; test-proof not applicable