Skip to content

test --changed: build no bundle for a config error and free the scan transpiler on every exit - #46810

Open
robobun wants to merge 1 commit into
mainfrom
robobun/30fa6e69/changed-scan-failure-leak
Open

robobun wants to merge 1 commit into
mainfrom
robobun/30fa6e69/changed-scan-failure-leak

Conversation

@robobun

@robobun robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • bun test --changed --define a-b=1 (a config error) prints warn: --changed: failed to build module graph (BuildFailed); running all tests. It then leaks 33560 bytes in 924 allocations, and up to 8 Bun Pool threads stay parked.
  • filter (src/runtime/cli/test/ChangedFilesFilter.rs) released the scan Transpiler only on success, and its Log never. BundleV2::scan_module_graph_from_cli (src/bundler/bundle_v2.rs) started its pool, then returned on the log error without teardown.
  • No user reported this: the review of test --changed: bind test files to the module graph by position, not by path #46583 found it.

Fix

  • The scan reads the log before BundleV2::init, so a config error starts no pool. Its other error exit tears the bundle down. A Drop guard in filter frees the scan log and the transpiler on every exit.
  • Correct because each owner releases what it acquired. The guard drops after the bundle.
  • Verified: 6 new cases in test/cli/test/test-changed.test.ts, with the new bundlerPoolsCreated of bun:internal-for-testing. Also ran test/bundler/bun-build-api.test.ts.
  • Self-reviewed: 14 concerns raised, 13 addressed. Kept the enqueue-error teardown, which one asked to drop.

Background

  • bun test --changed scans the module graph of the test files with the bundler. A failed scan runs every test.
  • The scan transpiler and log live in the CLI arena, which never runs Drop. Transpiler::deinit() is the explicit release, as in pack_command.rs.
  • Considered a log check in filter alone: the scan keeps an exit that leaves its pool alive.

Downsides

  • A successful bun test --changed runs 27 more instructions, once per process. The binary gains 1 host function (92 bytes).
  • No test runs the scan's enqueue-error teardown: only a failed allocation reaches it.
Notes

Reach. There is no issue for this. A release build prints the same output and exits 0. It keeps about 33 KB and up to min(8, cores) idle threads, once per process. The triggers are a --define or --drop key that is not an identifier and a --tsconfig-override file that does not exist, from the command line or from bunfig. What the change buys: the fallback gets its first tests, and test-changed.test.ts is leak-checked on the ASAN lane, so a test that reaches the fallback or logs a scan error no longer aborts there.

Measurements. Debug ASAN builds of main (179664e) and this branch, with the environment of the CI ASAN lane (BUN_DESTRUCT_VM_ON_EXIT=1, detect_leaks=1:abort_on_error=1, test/leaksan.supp):

main this branch
fallback exit, LeakSanitizer, 6 triggers 33560 to 33601 bytes in 920 to 924 allocations, exit 134 no report, exit 0
success exit with scan-log messages, LeakSanitizer 666 to 690 bytes in 5 allocations, exit 134 no report, exit 0
bundler pools made on the fallback (bundlerPoolsCreated) 1 0
fallback on a 12-core machine: Bun Pool threads, tasks 8 and 13 (release: 8 and 12) 0 and 4 (release: 0 and 4, plain bun test: 0 and 4)
fallback, allocations live at exit 1128 179 (run without --changed: 179)
enqueue-error exit (injected error, not committed) 32272 to 32318 bytes leaked, 8 to 10 Bun Pool threads after 3 s 0 bytes leaked, 0 threads

The 6 triggers: --define a-b=1, --drop=a-b, --tsconfig-override ./missing.json, --changed=HEAD --define a-b=1, bunfig [define] "a-b" = "1", bunfig external = "a*b*". Live allocations are malloc calls minus free calls from ASAN_OPTIONS=print_stats=1:atexit=1. The thread numbers are from /proc/self/task on Linux.

Tests. Debug ASAN build of this branch: 27 pass in 5 of 5 runs, and once more under the environment of the CI ASAN lane. Release build of this branch: 26 pass, 1 skipped (the case that only LeakSanitizer can see). A debug ASAN build of main with only the getter added: the 4 config error cases fail with bundlerPools: "1", the syntax error case fails with 690 byte(s) leaked in 5 allocation(s), and the control (a scan with no config error makes 1 pool) passes. Plain main has no getter, so there the cases that read it fail with bundlerPools: undefined. test/bundler/bun-build-api.test.ts on the release build of this branch: 89 pass, 0 fail. On the local debug ASAN build 3 of its cases reach their time limit (two bytecode cases at 5 s, "can be called thousands of times" at 180 s).

The tests count pools and not threads. A pool thread takes its Bun Pool name when it first runs, so a count of names reads 0 on a loaded machine while the pool exists. bundlerPoolsCreated reads the existing POOL_GENERATION counter, which BundleV2::init is the only code to advance.

Why the check is in the scan. A has_errors() return in filter before the call passes the same tests. The scan would keep its own init, then check, then return Err with the pool alive: an exit that no input reaches and that leaks when one does. With the check above init, the function that starts the pool refuses the log, there is one check, and an Err of the scan leaves no bundle for any caller. init adds an error only for code splitting with a format that is not ESM and for a bytecode order file. bun test sets neither, so the copy of the check after init is deleted.

Left as they are, on purpose.

Other open pull requests on these lines.

Release builds of main and this branch.

  • Successful bun test --changed: the body of filter runs 1159 then 1157 instructions, the body of the scan 120 then 119, and the one new call, Log::clear_and_free, runs 30 with 0 frees. That is +27 (a gdb nexti and stepi count, two runs each, same result).
  • .text 82164617 to 82164873 (+256), .bun_builtins 2710563 to 2710620 (+57), file size 107730656 for both. filter 8239 to 8261 bytes, the scan 843 to 858, jsPoolsCreated 92 (size -A and nm -S).
  • strace is not installed here, so there is no clone3 count.

Design alternatives beyond the one in Background: impl Drop for BundleV2 (every bundle of all five drivers pays a flag and a branch, and it cannot join tasks without the accounting in #40438), a Box for the scan transpiler (the shape of #35312, it has no defect, the guard is the shape that #44968 landed for this call pattern in pack_command.rs), and a guard shared with pack_command.rs (that one must not free a log).


no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/test-changed.test.ts

The scan checks the log before it builds a bundle, so a config error
starts no worker pool, and it tears its bundle down when enqueueing
fails. `filter` owns the scan transpiler and its log through a guard
that releases both on every exit.

`bundlerPoolsCreated` in `bun:internal-for-testing` reads how many
bundler thread pools the process made, so the tests can tell a scan
that built a bundle from one that did not.
@coderabbitai

coderabbitai Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 22 seconds.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 179664e and a1c94ef.

Files selected for processing: 7
  • src/bundler/ThreadPool.rs
  • src/bundler/bundle_v2.rs
  • src/js/internal-for-testing.ts
  • src/runtime/api/JSBundler.bind.ts
  • src/runtime/api/JSBundler.rs
  • src/runtime/cli/test/ChangedFilesFilter.rs
  • test/cli/test/test-changed.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:32 PM PT - Oct 24th, 2026

✅ @robobun, your commit a1c94ef9a667e2b71b7a12748d90a05e5cb8101f passed in Build #132232! 🎉


🧪   To try this PR locally:

bunx bun-pr 46810

That installs a local version of the PR into your bun-46810 executable, so you can run:

bun-46810 --bun

@robobun

robobun commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on debug ASAN builds of main with the LeakSanitizer options of the CI ASAN lane. bun test --changed --define a-b=1 prints the fallback warning, then reports 33560 bytes leaked in 924 allocations and keeps 8 Bun Pool threads on a 12-core machine. A syntax error in a scanned file leaks 666 to 690 bytes on the success exit. With this branch both runs report nothing and the fallback makes no pool. The fix and 6 test cases are in this PR (#46810).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

robobun added a commit that referenced this pull request Oct 24, 2026
…hanges

The teardown on the error exits of `scan_module_graph_from_cli` is now
in #46810, which checks the log before `BundleV2::init`. The drain
helper called `wait_for_parse()` after an enqueue error, where the scan
counter can stay above zero. #43428 moves that counter first.

What stays here: `get_worker_slow` inserts a complete `Worker`, and
`worker_pool()` checks its pointer in release builds.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants