Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/jsc/bindings/BunProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1212,9 +1212,25 @@
if (!lexicalGlobalObject->inherits(Zig::GlobalObject::info()))
return false;
auto* globalObject = uncheckedDowncast<Zig::GlobalObject>(lexicalGlobalObject);
auto& vm = JSC::getVM(globalObject);

// A worker whose terminate() has been requested must not run its
// uncaught-exception machinery: the exception reaching here is either the
// TerminationException itself or was produced while it was pending, and the
// process->get / emit / call sequence below walks the JS heap and may call
// user code. With terminate() racing (possibly while the parent/process is
// already tearing down), that property walk has tripped
// ASSERT(object->structure() == this) in Structure::storedPrototype. Treat
// as handled so the Rust caller does not go on to dispatch more JS either.
// scriptExecutionStatus is Stopped exactly when has_requested_terminate()
// is set on the worker (or the VM is shutting down, which the Rust caller
// already short-circuits), so a main-thread node:vm watchdog does not trip
// this.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (Zig::GlobalObject::scriptExecutionStatus(globalObject, globalObject) != JSC::ScriptExecutionStatus::Running) [[unlikely]]
return true;

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

View check run for this annotation

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 1230 in src/jsc/bindings/BunProcess.cpp

View check run for this annotation

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
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

auto* process = globalObject->processObject();
auto& wrapped = process->wrapped();
auto& vm = JSC::getVM(globalObject);

// node parity (exitWithUndefinedFatalException): the internal fatal-exception
// handler is monkey-patchable as process._fatalException. If user code
Expand Down
56 changes: 56 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,62 @@
timeout,
);

// Regression: a nested worker whose grandchild's module fails to load reports
// its uncaught MODULE_NOT_FOUND via flush_logs → report_uncaught_exception →
// Bun__handleUncaughtException. When worker.terminate() and process.exit()
// land mid-dispatch, that handler would still lazily create `process` and do
// `process->get("_fatalException")` on a VM already asked to terminate, which
// tripped ASSERT(object->structure() == this) in Structure::storedPrototype.
// Debug-assert-only; the race is non-deterministic (~2/14 on
// release-asan-cov), so loop it.
test.skipIf(!isASAN)(
"terminating a worker while its grandchildren are reporting load errors does not assert",
async () => {
// Each subprocess run spawns a middle worker that creates grandchildren
// whose module load fails, then main terminates the middle worker and
// exits. The race window is between the grandchild's error dispatch and
// terminate_all_and_wait arming TerminationException.
const code = `
const { Worker } = require("node:worker_threads");
const middleSrc = \`
const { Worker, parentPort } = require("node:worker_threads");
for (let j = 0; j < 4; j++) {
const w = new Worker(new URL("./does-not-exist-xyzzy.mjs", import.meta.url));
w.on("error", () => {});

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

View check run for this annotation

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
Comment thread
robobun marked this conversation as resolved.
Outdated
}
parentPort.postMessage("spawned");
setInterval(() => {}, 1000);
\`;
const middle = new Worker(new URL(
"data:text/javascript;base64," + Buffer.from(middleSrc).toString("base64"),
));
middle.on("error", () => {});
middle.on("message", () => {
middle.terminate();
process.exit(0);
});
setTimeout(() => process.exit(0), 2000);
`;

for (let i = 0; i < 20; i++) {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stderr, stdout, exitCode, signalCode: proc.signalCode }).toEqual({
stderr: "",
stdout: "",
exitCode: 0,
signalCode: null,
});
}
},
timeout * 2,
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Regression: the per-VM c-ares channel was destroyed in deinit_runtime_state
// (RuntimeState drop) AFTER JSC teardown and RareData.file_polls drop.
// ares_destroy() synchronously fires EDESTRUCTION query callbacks and socket-
Expand Down
Loading