Conversation
Implements the DedicatedWorkerGlobalScope#close() API from the HTML spec. Before this, calling self.close() inside a Worker threw TypeError. Now it requests termination of the worker event loop on the next tick; any task already queued (e.g. an immediately-preceding postMessage) still runs to completion before the worker exits. Fixes #29186
|
Updated 7:34 PM PT - Apr 11th, 2026
❌ @robobun, your commit 8eb3c0e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29189That installs a local version of the PR into your bun-29189 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a JSC host function Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29186.test.ts`:
- Around line 11-19: The tests call cleanStderr and then assert
expect(cleanStderr(stderr)).toBe(""), which is brittle under CI/ASAN; remove
those empty-stderr assertions in the failing regression test(s) and instead
assert the spawned process exit code (use expect(exitCode).toBe(0) or the
appropriate non-zero code) as the primary failure condition; if you need to
validate specific stderr content, replace the empty-string check with a targeted
assertion (e.g., expect(cleanStderr(stderr)).toContain("specific error text")).
Update occurrences referencing cleanStderr in the affected tests (the
regression/issue/*.test.ts files) – replace the empty stderr assertions at the
noted spots with exitCode-based checks or targeted contains checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 977129b9-bb8d-495d-b762-8b4afed5ad1c
📒 Files selected for processing (4)
src/bun.js/bindings/ZigGlobalObject.lut.txtsrc/bun.js/bindings/webcore/Worker.cppsrc/bun.js/bindings/webcore/Worker.htest/regression/issue/29186.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Node.js worker_threads has no global close() — calling it throws ReferenceError. Delete the property from the worker global object when the worker kind is Node so typeof close === "undefined" there, matching Node exactly. Web workers still see it via the LUT. Also drop the brittle empty-stderr assertions in the regression test; exitCode is the primary signal. Add a test that verifies close is absent on a node:worker_threads Worker and that calling it throws ReferenceError.
There was a problem hiding this comment.
The node:worker_threads compatibility issue I raised has been addressed — the fix deletes close from Node-kind workers in the initializeWorker hook, and the new test suite covers all three cases (Web Worker termination, typeof checks for both kinds, and main-thread no-op). LGTM.
Extended reasoning...
Overview
This PR adds / to the WHATWG . Changes span (new host function and helper), (declaration), (registration), (Node-kind property removal), and a regression test file.
Security risks
No security-sensitive code paths are touched. The function only requests termination of the calling worker — it has no capability to affect other workers or the main thread. No auth, crypto, or permission code is involved.
Level of scrutiny
Moderate: this touches the worker lifecycle and global property table. The key correctness concern (leaking into workers) was caught and fixed. The register-then-delete approach for Node workers is slightly inelegant but sound and consistent with how the rest of the LUT works. The fix correctly avoids setting , matching the spec's "current task finishes" semantics.
Other factors
The regression test is comprehensive — it covers the Web Worker termination path, typeof guards on both worker kinds, and the main-thread no-op. The CodeRabbit feedback about brittle stderr assertions was also addressed in the follow-up commit. No outstanding reviewer comments remain unresolved.
There was a problem hiding this comment.
LGTM — the node:worker_threads close-exposure bug I raised in the previous review has been fixed by deleting the property from Node-kind worker globals.
Extended reasoning...
Overview
This PR adds self.close() (WHATWG DedicatedWorkerGlobalScope#close) to Bun's Worker global scope. Changes span Worker.cpp, Worker.h, ZigGlobalObject.lut.txt, ZigGlobalObject.cpp, and a regression test.
Security risks
None. This implements a well-defined browser API; the termination path it invokes (WebWorker__notifyNeedTermination) already existed and is used by terminate().
Level of scrutiny
Moderate — the change touches C++ global object initialization and worker lifecycle. However, the fix for the node:worker_threads exposure (deleting the property on Node-kind workers in the initializeWorker hook) is the correct and minimal approach, matching how WebWorker__dispatchError already discriminates between worker kinds.
Other factors
The prior concern about node:worker_threads workers incorrectly getting close() was addressed in e27e7c9. The regression test now covers four cases: Web Worker termination semantics, typeof detection in Web Workers, absence in node:worker_threads (including ReferenceError on call), and main-thread no-op. No bugs were found by the automated review.
Previously a pre-fix run produced JSON.parse EOF errors because the worker threw TypeError, main rejected, and stdout stayed empty. Assert exitCode + stderr via toMatchObject first so the diff shows the real reason; the JSON.parse check runs after and still catches wrong shapes.
There was a problem hiding this comment.
The node:worker_threads concern I raised has been addressed — LGTM.
Extended reasoning...
Overview
This PR adds the WHATWG DedicatedWorkerGlobalScope#close() API to Bun's Web Worker global scope. Changes touch Worker.cpp/Worker.h (new notifyNeedTermination() method and jsFunctionSelfClose host function), ZigGlobalObject.lut.txt (LUT registration), ZigGlobalObject.cpp (Node-worker cleanup), and a regression test file.
Security risks
None. The function only signals the worker's own event loop to stop; it cannot affect other workers or the main thread in a destructive way beyond what terminate() already permits.
Level of scrutiny
Medium. The implementation follows the established jsFunctionPostMessage pattern exactly, and the Node-worker isolation fix (deleting close from the global for Kind::Node workers) is correct and well-tested. The pre-existing thread-safety note in the bug report is real but predates this PR and is unrelated to the correctness of the new API.
Other factors
- My prior inline comment about
closeleaking intonode:worker_threadsworkers was resolved in commit e27e7c9 with a targetedDeletePropertycall ininitializeWorker. - Four regression tests cover: message delivery before termination, feature-detection in Web Workers, absence in Node workers (including
ReferenceErroron call), and main-thread no-op. - The pre-existing
unref()thread-safety issue innotifyNeedTerminationis already acknowledged via a// TODOcomment in Zig and is not introduced by this PR.
setRefInternal(false) called parent_poll_ref.unref() non-atomically, so any caller on the worker thread (process.exit, and now self.close) raced with the parent event loop's refcount. ASAN and multi-distro CI lanes tripped on it after self.close() made the path user-reachable. Drop the unref here entirely. The parent ref is released later during deinit() via unrefConcurrently, which is the safe cross-thread path. The author had already flagged this line with TODO(@190n) delete.
There was a problem hiding this comment.
LGTM — both prior concerns (Node-worker close exposure and the cross-thread unref race) have been addressed in the follow-up commits.
Extended reasoning...
Overview
This PR adds the WHATWG DedicatedWorkerGlobalScope#close() API to Bun's Web Worker global scope. It touches ZigGlobalObject.cpp, ZigGlobalObject.lut.txt, Worker.cpp, Worker.h, web_worker.zig, and a new regression test.
Prior concerns (now resolved)
My earlier reviews raised two issues:
- Node worker exposure: The LUT-registered
closewas unconditionally visible tonode:worker_threadsworkers. Fixed in e27e7c9 by deleting the property from Node-kind worker globals duringinitializeWorker. - Cross-thread unref race:
notifyNeedTerminationpreviously calledsetRefInternal(false)(non-thread-safe) from the worker thread. Fixed by removing that call entirely — the parent KeepAlive ref is now released only throughunrefConcurrently()insidedeinit(), which is the established safe cross-thread path.
Security risks
None. This adds a self-termination API (no auth, no crypto, no privilege escalation). The main thread no-op path is safe.
Level of scrutiny
Moderate. The change touches the Worker event loop and cross-thread lifecycle, which are sensitive paths. However, the fix is small, follows existing patterns (mirrors jsFunctionPostMessage, uses the same WebWorker__getParentWorker lookup), and the thread-safety issue was proactively fixed rather than left as a TODO.
Other factors
Four regression tests cover: message-before-close delivery, Web Worker feature detection, absence of close in node:worker_threads, and main-thread no-op. CodeRabbit's stderr-assertion concern was also addressed. The implementation is consistent with Node.js and Deno behavior.
This reverts commit 26da1dd.
1 participant
Fixes #29186.
Repro
Before:
After:
Cause
Bun never registered the WHATWG
DedicatedWorkerGlobalScope#closeAPI on theworker global scope —
self.closewasundefined, so the first thing a workerscript did when trying to terminate itself was throw
TypeError. Both Node.jsand Deno expose it, and it's a commonly expected shutdown path.
Fix
jsFunctionSelfClosehost function inwebcore/Worker.cppmodeled on
jsFunctionPostMessage. It looks up the parentWorker*viaWebWorker__getParentWorkerand, when called from inside a worker, asksthe worker's event loop to stop on its next tick via a new
Worker::notifyNeedTermination()method that wraps the existingWebWorker__notifyNeedTerminationZig export.closeas aFunction 0global inZigGlobalObject.lut.txt.close()is a silent no-op, matchinghow
postMessagebehaves in that context today.Per spec, the task currently running when
self.close()is called finishesnormally, so an immediately-preceding
self.postMessage(...)still reachesthe parent — this is the behavior covered by the regression test.
Verification
test/regression/issue/29186.test.tsasserts:self.postMessage('message'); self.close();delivers the message to theparent and the worker exits cleanly (
closeevent, exit code 0, noTypeError).typeof self.close === 'function'(and the bareclosebinding too).close()on the main thread is a no-op.All three tests pass on this branch and fail on stock 1.3.11.