bundler: post Bun.build completions by ScriptExecutionContext id to survive worker.terminate() - #35158
bundler: post Bun.build completions by ScriptExecutionContext id to survive worker.terminate()#35158robobun wants to merge 7 commits into
Code review found 3 potential issues
Found 3 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/runtime/api/js_bundle_completion_task.rs:157-158 |
Fresh task-owned Loader has did_load_process=false, so load_process() re-reads OS environ on every Bun.build() |
| 🟡 Nit | test/bundler/bun-build-api.test.ts:1550-1551 |
Test fixture awaits worker 'message' without wiring 'error' to reject |
| 🟡 Nit | src/runtime/api/js_bundle_completion_task.rs:147-154 |
New early return in create_and_schedule_completion_task leaks plugins on clone_with_allocator failure |
Annotations
Check warning on line 158 in src/runtime/api/js_bundle_completion_task.rs
claude / Claude Code Review
Fresh task-owned Loader has did_load_process=false, so load_process() re-reads OS environ on every Bun.build()
The fresh `Loader::init(env_map)` has `did_load_process = false`, so on the bundle thread `configure_defines()` → `run_env_loader` → `env.load_process()` (transpiler.rs:786) now re-iterates OS `environ` and `map.put()`s every entry — where before this PR the VM's own loader early-returned. That's a redundant per-`Bun.build()` environ walk plus alloc-per-entry, and it clobbers any cloned-map value that differs from the OS value (e.g. `process.env.HTTP_PROXY = ...` set via JS, which `Bun__setEnvVa
Check warning on line 1551 in test/bundler/bun-build-api.test.ts
claude / Claude Code Review
Test fixture awaits worker 'message' without wiring 'error' to reject
The fixture's `await new Promise(res => w.once("message", res))` has no rejection path — REVIEW.md's 'Tests reviewers reject' section says to wire every failure event to reject the awaited promise. If the worker's eval source ever throws before `parentPort.postMessage("up")` (a future edit, or a Bun regression in worker eval), the subprocess hangs until the outer 45s/120s timeout with no diagnostic. One-line fix: `await new Promise((res, rej) => { w.once("message", res); w.once("error", rej); })
Check warning on line 154 in src/runtime/api/js_bundle_completion_task.rs
claude / Claude Code Review
New early return in create_and_schedule_completion_task leaks plugins on clone_with_allocator failure
The new `clone_with_allocator()?` inserts a fallible early return between receiving `plugins: Option<NonNull<Plugin>>` and storing it in the boxed task; on `Err` the `?` propagates and `plugins` (a `Copy` raw pointer with no `Drop`) leaks the C++ `JSBundlerPlugin` and its JSC `protect()` root. OOM-only path so impact is bounded, but the fix is trivial: destroy `plugins` on the error arm (or wrap it in a scopeguard armed before the clone), per REVIEW.md's "New early returns or fallible calls → re