Skip to content

process: skip uncaught-exception dispatch on a terminating worker - #35678

Closed
robobun wants to merge 11 commits into
mainfrom
farm/409c20b8/uncaught-exception-termination-guard
Closed

process: skip uncaught-exception dispatch on a terminating worker#35678
robobun wants to merge 11 commits into
mainfrom
farm/409c20b8/uncaught-exception-termination-guard

process: skip uncaught-exception dispatch on a terminating worker

f539318
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jul 25, 2026 in 32m 8s

Code review found 3 potential issues

Found 4 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 3
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/jsc/bindings/BunProcess.cpp:1229-1230 return true inverts spin() entry-rejection shutdown gate
🟡 Nit test/js/web/workers/worker-terminate-lifetime.test.ts:143-145 Test fixture depends on Bun's mangled data-URL import.meta.url quirk
🟡 Nit src/jsc/bindings/BunProcess.cpp:1229-1230 Sibling Bun__handleUnhandledRejection / Bun__emitHandledPromiseEvent lack the same termination guard

Annotations

Check warning on line 1230 in src/jsc/bindings/BunProcess.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

return true inverts spin() entry-rejection shutdown gate

Returning `true` here inverts the shutdown gate for one caller the PR description lists as covered: `spin()`'s entry-rejection path (web_worker.rs:1113-1123) uses this return value as `if !handled { return self.shutdown() }`. When `terminate()` lands during `load_entry_point_for_web_worker` and the entry promise rejects, pre-PR went straight to `shutdown()` with `exit_code=1`; post-PR falls through to `dispatchOnline` / `fireEarlyMessages` / an unconditional `vm.tick()` with `exit_code` left at 

Check warning on line 145 in test/js/web/workers/worker-terminate-lifetime.test.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Test fixture depends on Bun's mangled data-URL import.meta.url quirk

The grandchild spawn `new URL("./does-not-exist-xyzzy.mjs", import.meta.url)` only works because Bun currently reports `import.meta.url` inside a data-URL worker as the mangled `file:///data:text/javascript;base64,...` — the very quirk the cited repro file `worker-dataurl-importmeta-url-mangled.mjs` is named after. Per WHATWG URL, resolving a relative reference against a `data:` base throws; when that quirk is fixed, `new URL(...)` throws synchronously at `j=0`, `middle.on("error", () => {})` sw

Check warning on line 1230 in src/jsc/bindings/BunProcess.cpp

See this annotation in the file changed.

@claude claude / Claude Code Review

Sibling Bun__handleUnhandledRejection / Bun__emitHandledPromiseEvent lack the same termination guard

The two immediately-adjacent siblings `Bun__handleUnhandledRejection` (line 1360) and `Bun__emitHandledPromiseEvent` (line 1382) share the exact `processObject()` (lazy-create) → `wrapped.emit()` shape being guarded here, and their Rust callers (`VirtualMachine.rs:3290` / `:1077`) check only `is_shutting_down()` — not `has_requested_terminate()` — so they're reachable in the same terminate() race window. Per REVIEW.md ('Fix the whole class in the same PR… If a site is intentionally excluded, say