test: re-enable the LeakSanitizer exit check for the bake dev tests that no longer leak - #39293
test: re-enable the LeakSanitizer exit check for the bake dev tests that no longer leak#39293robobun wants to merge 1 commit into
Conversation
…hat no longer leak The two bake blocks in test/no-validate-leaksan.txt excluded ten dev server test files from the ASAN lane's exit-time leak check. Eight of them exit clean under that check now (BUN_DESTRUCT_VM_ON_EXIT=1, detect_leaks=1, test/leaksan.supp; four runs each on a debug ASAN build), so they are removed: dev-and-prod, esm, hot, react-spa, sourcemap, ssg-pages-router, react-response and server-sourcemap. bundle.test.ts and css.test.ts stay listed; the comment now names the one leaking case (or two) in each and the PR that fixes it, so the entries can be removed when those land. Three entries named files that do not exist, so they excluded nothing: test/bake/dev/deinitialization.test.ts (the file is test/bake/deinitialization.test.ts and has always been checked), test/bake/dev/react-cookies.test.ts (request-cookies.test.ts, likewise) and test/cli/install/bun-repl.test.ts (deleted with the old REPL).
|
Status: reproduced and fixed in this PR.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe LSAN exemption file removes the ChangesLSAN exemption maintenance
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized test-coverage change re-enables leak checks for files verified clean while retaining exclusions for two files with known leaks. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — tightens the LSan suppression list, which is self-verifying via this PR's own ASAN lane.
What was reviewed:
- Confirmed the three removed dead entries (
dev/deinitialization.test.ts,dev/react-cookies.test.ts,cli/install/bun-repl.test.ts) do not exist on disk and the eight re-enabled files do. - Checked
scripts/runner.node.mjs— the list is consumed by exact path match, so removals only add coverage; nothing is loosened. - The two kept entries and their new comment match the leak reports and fix PRs cited in the description.
Extended reasoning...
Overview
This PR edits a single test-infrastructure file, test/no-validate-leaksan.txt, which is an opt-out list read by scripts/runner.node.mjs to decide which test files skip the exit-time LeakSanitizer check on the ASAN CI lane. It removes eight bake dev-server test files that no longer leak, removes three entries that name nonexistent files (dead no-ops), and replaces two vague block comments with a precise per-entry comment naming the remaining leak, its allocation site, and the PR that fixes it.
Security risks
None. This is a plaintext suppression list consumed only by the CI test runner; no runtime code, auth, or user-facing surface is touched.
Level of scrutiny
Low. The change only removes suppressions, which strictly tightens CI — if any of the eight re-enabled files still leaks, this PR's own ASAN run fails and blocks merge, so the change is self-verifying. The three dead entries were verified against the filesystem (all three paths are indeed missing; the corrected paths test/bake/deinitialization.test.ts and test/bake/dev/request-cookies.test.ts exist and were already running under the check). No entries are added other than the two that were already present (bundle.test.ts, css.test.ts), so nothing new is being exempted from checking.
Other factors
The PR description is unusually thorough: 4× runs per re-enabled file under the exact runner environment, a positive control (the two kept files still report leaks under the same harness, proving the check actually ran), and cross-references to the in-flight fix PRs (#38004, #39197). The runner reads this file line-by-line with comment/blank filtering and matches by exact relative path, so the edit cannot have unintended side effects on other entries. The one operational risk — that re-enabling LSan on eight subprocess-heavy bake files slows the ASAN lane — is acknowledged with timing data and is a CI-tuning concern, not a correctness one.
|
Updated 9:49 PM PT - Aug 15th, 2026
✅ @robobun, your commit 623abc24599a951781e737c26f725c2b9cce1b1c passed in 🧪 To try this PR locally: bunx bun-pr 39293That installs a local version of the PR into your bun-39293 --bun |
Problem
test/no-validate-leaksan.txtswitches off the ASAN lane's exit-time leak check for the files it lists. Its two bake blocks listed ten dev server test files: the "Watcher Thread" block (entries excluded since the check was wired into CI in ci: instrument being able to run leaksanitizer #21142, grouped under that heading in a09dc2f) and the "leaks related to the dev server closing on process exit" block (ssg 3 #22138). Those files have never run under the check.scripts/runner.node.mjs:BUN_DESTRUCT_VM_ON_EXIT=1,ASAN_OPTIONS=...detect_leaks=1...,LSAN_OPTIONS=...suppressions=test/leaksan.supp),dev-and-prod,dev/esm,dev/hot,dev/react-spa,dev/sourcemap,dev/ssg-pages-router,dev/react-responseanddev/server-sourcemapexit 0 with no LeakSanitizer output, 4 of 4 runs each (table below).dev/bundle.test.ts, "removing 'use client' from a component with a pending resolution failure":Direct leak of 336 byte(s)fromBundleV2::enqueue_server_component_generated_file(src/bundler/bundle_v2.rs:3752), theServerComponentParseTaskbundler: free the ServerComponentParseTask after it generates its file #38004 frees. The other 20 cases in the file are clean.dev/css.test.ts, "changing html file with link tag works" and "css import before create":Direct leak of 30 byte(s)fromWatcher::add_directoryviaDirectoryWatchStore::insert(src/runtime/bake/dev_server/mod.rs:1447), the evicted watch entry path watcher: free the owned path of evicted watchlist entries #39197 frees. The other 13 cases are clean.test/bake/dev/deinitialization.test.ts(the file istest/bake/deinitialization.test.ts; no file ever existed at the listed path),test/bake/dev/react-cookies.test.ts(added in the same commit asrequest-cookies.test.ts, which is the file that exists), andtest/cli/install/bun-repl.test.ts(deleted in fa3a30f). The two real files have been running under the check in CI all along; bake: fix use-after-free in ~DevServerSourceProvider during VM teardown #34035 is a CI failure ofrequest-cookies.test.tsunderBUN_DESTRUCT_VM_ON_EXIT.Fix
bundle.test.tsandcss.test.ts, under a comment that names the leaking case in each, the allocation site and size, and the PR that fixes it, so each entry can be removed with its fix. watcher: free the owned path of evicted watchlist entries #39197 already removes thecss.test.tsline on its branch; whichever lands second needs a one-line rebase of this block. bundler: free the ServerComponentParseTask after it generates its file #38004 does not touch this file, sobundle.test.tsshould be removed when it lands.css.test.tsandbundle.test.tsleaks above from inside the dev server child processes, so a clean run means the check ran and found nothing, not that it was skipped.dev/production.test.ts(bun build --appstill leaks its transpilers, bake: drop the production build's transpilers and framework projection #38233; bake: exit a successful bun build --app through the build VM #38241 documents that entry) and the bake files listed in the generic blocks near the top of the file (ecosystem,html,plugins,stress,vfile,import-meta-inline,incremental-graph-edge-deletion), which were not audited here../build/debug/bun-debug test <file>under the three variables above, on main at f0f6b2c. Results are in the table below. Every non-vendor/entry remaining in the file exists on disk.Background
abort_on_error=1a report aborts the process. Bun disables it by default;scripts/runner.node.mjsturns it back on for every test file not intest/no-validate-leaksan.txtand also setsBUN_DESTRUCT_VM_ON_EXIT=1, which tears the JSC VM down before exit so memory still owned by live JS objects is freed rather than reported.test/bake/bake-harness.ts). The test process's environment is inherited, so the check runs in each dev server process: at the end of a case the harness stops the server, waits untilDevServerhas been dropped, and callsprocess.exit(0); a leak report there makes the server exit by SIGABRT and the case fails withDevServer exited with signal SIGABRT. This is how the two remaining entries fail, and the leak coverage the removed entries were withholding.isBucketCandidaterequires the file to be leak-checked);sourcemap,react-responseandserver-sourcemapbecome eligible there, as they already are on the other lanes.Probe results (debug ASAN build of main at f0f6b2c, runner environment)
Cost of the check on the debug build:
sourcemap.test.ts11s without the environment, 18s with it;dev-and-prod.test.ts(12 dev server processes) 37s and 79s. The existing checked bake files show the release ASAN equivalent intest/expected-durations.json(deinitialization.test.ts: 2.2s default, 8.0s asan).Leak report from bundle.test.ts, bun frames only:
Leak report from css.test.ts (css-13; css-14 is identical), bun frames only: