feat(node:worker_threads): implement markAsUncloneable#29424
Open
Arv1nt3 wants to merge 3 commits into
Open
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Implements worker_threads.markAsUncloneable(obj) matching Node.js
semantics: once an object is marked, any subsequent structured-clone
attempt (structuredClone, MessagePort.postMessage, Worker workerData,
BroadcastChannel.postMessage) throws a DOMException with
name === "DataCloneError". Primitives and ArrayBuffer /
SharedArrayBuffer / TypedArray / DataView / Buffer are a no-op, matching
the Node spec. Marking is irreversible and the marker is hidden from
Object.keys / JSON / for-in / Reflect via a JSC private-name identifier.
The native hook lives in a single place: CloneSerializer::dumpIfTerminal
in SerializedScriptValue.cpp, which every clone path funnels through, so
marked objects are caught at the root and when nested inside arrays,
objects, Maps, and Sets.
Unblocks undici 8.0.3+ on Bun. Before this change:
$ bun -e 'require("undici/lib/web/cache/cachestorage.js")'
TypeError: webidl.util.markAsUncloneable is not a function
After:
$ bun-debug -e 'require("undici/lib/web/cache/cachestorage.js"); console.log("ok")'
ok
Spec: https://nodejs.org/api/worker_threads.html#workermarkasuncloneableobject
Fixes oven-sh#29423
Arv1nt3
force-pushed
the
claude/worker_threads-markAsUncloneable
branch
from
6725688 to
bae0e3e
Compare
Contributor
WalkthroughAdds Node's worker_threads.markAsUncloneable support: introduces a builtin private name, a host function that tags objects as uncloneable, and a serialization-time check that causes structured-clone and postMessage paths to throw DataCloneError for tagged objects. Includes API exposure and tests, plus an undici regression test. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/js/node/worker_threads/markAsUncloneable-undici-regression.test.ts`:
- Around line 16-17: The TODO indicates this regression test should be moved
into the issue-specific regression folder; move the file currently named
markAsUncloneable-undici-regression.test.ts into
test/regression/issue/29423.test.ts (rename and relocate it) and remove or
update the TODO comment; ensure any imports or test-suite identifiers inside the
file still resolve after the move.
In `@test/js/node/worker_threads/markAsUncloneable.test.ts`:
- Around line 260-264: Add a conditional check to surface stderr when the child
process exitCode is non-zero: immediately before the existing
expect(exitCode).toBe(0) in the test (where stderr, stdout, and exitCode are
asserted), insert an if (exitCode !== 0) { expect(stderr).toBe(""); } so that
failing subprocesses print the full stderr diff; keep the existing assertions on
stderr and stdout intact and place this new guard just prior to the
expect(exitCode).toBe(0) line.
🪄 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: 62a7121f-0e3c-4e9d-a84c-0c94af2a6c54
📒 Files selected for processing (8)
src/bun.js/bindings/webcore/SerializedScriptValue.cppsrc/bun.js/bindings/webcore/Worker.cppsrc/bun.js/bindings/webcore/Worker.hsrc/js/builtins/BunBuiltinNames.hsrc/js/node/worker_threads.tstest/js/node/worker_threads/markAsUncloneable-undici-regression.test.tstest/js/node/worker_threads/markAsUncloneable.test.tstest/js/node/worker_threads/worker_threads.test.ts
Comment on lines
+16
to
+17
| // TODO(user): after filing the tracking issue, move this file to | ||
| // test/regression/issue/<N>.test.ts. |
Contributor
There was a problem hiding this comment.
Move this confirmed regression test under the issue regression folder.
This PR already fixes confirmed issue #29423, so the TODO should not ship here; place this file at test/regression/issue/29423.test.ts instead. As per coding guidelines, test/**/*.test.ts: “For tests related to a specific numbered GitHub Issue, place the test in test/regression/issue/${issueNumber}.test.ts with a real issue number”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/worker_threads/markAsUncloneable-undici-regression.test.ts`
around lines 16 - 17, The TODO indicates this regression test should be moved
into the issue-specific regression folder; move the file currently named
markAsUncloneable-undici-regression.test.ts into
test/regression/issue/29423.test.ts (rename and relocate it) and remove or
update the TODO comment; ensure any imports or test-suite identifiers inside the
file still resolve after the move.
- Move the undici regression test to test/regression/issue/29423.test.ts
(per repo convention for issue-linked tests) and drop the stale TODO.
- In markAsUncloneable.test.ts, build the child worker's file:// URL with
pathToFileURL(join(dir, "worker.mjs")).href in the parent test scope and
inject it via JSON.stringify, so paths with spaces or a Windows drive
letter work correctly and are properly percent-encoded.
- Add `if (exitCode !== 0) expect(stderr).toBe("")` before the final
exitCode assertion in the child-worker test so a non-zero exit surfaces
the full stderr diff in CI logs.
- Bump the 29423 regression test's per-test timeout to 120s via the 3rd
argument to test(...), so a cold-cache `bun install undici@^8.1.0` has
room to either succeed or hit its existing "install failed → skip"
branch instead of dying at the default 5s wall.
Verified locally with `bun bd test`:
- markAsUncloneable.test.ts: 25/25 pass (incl. child-worker at 2210ms)
- test/regression/issue/29423.test.ts: 1/1 pass (5921ms, real npm install)
No implementation or behavior changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/js/node/worker_threads/markAsUncloneable.test.ts`:
- Around line 1-274: The test "SharedArrayBuffer is unaffected by
markAsUncloneable (Node spec no-op)" is too strict by using expect(() =>
structuredClone(sab)).not.toThrow() — tighten it to only assert that
structuredClone does not throw a DataCloneError: catch the exception from
structuredClone(sab) (or use expect(...).not.toThrow(/DataCloneError/)) and
assert err?.name !== "DataCloneError"; update the assertion in that test (the
SharedArrayBuffer test that calls structuredClone(sab)) accordingly so unrelated
throws won't make the test flaky.
In `@test/regression/issue/29423.test.ts`:
- Around line 59-64: The current early-return on bun install failure (checking
installExit, installStdout, installStderr around the kUndiciVersion install)
causes the test to silently pass; change this to explicitly skip the test so CI
shows a skip instead of a green pass — for example, replace the return path with
a test.skip(...) or implement a test.skipIf(...) gated by a quick registry
reachability probe, and include the installExit/installStdout/installStderr
details in the skip message; apply the same change to the version-parse skip
branch (the branch around lines handling parsed version validation) so both
failure cases are surfaced as explicit skips rather than silent passes.
🪄 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: 7b52a9fe-fb9b-404f-9778-a30a99cfd15f
📒 Files selected for processing (2)
test/js/node/worker_threads/markAsUncloneable.test.tstest/regression/issue/29423.test.ts
Comment on lines
+59
to
+64
| if (installExit !== 0) { | ||
| console.warn( | ||
| `[markAsUncloneable-undici-regression] Skipping: 'bun install undici@${kUndiciVersion}' failed (exit ${installExit}).\nstdout: ${installStdout}\nstderr: ${installStderr}`, | ||
| ); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent pass on install failure — consider making the skip visible.
When bun install fails (e.g., offline CI, registry outage, or a real bun install regression), the test returns early and is reported as passed. A silent pass on a regression test can mask the very issue you're guarding against if, say, some future bun install bug starts returning non-zero across the board. Consider using test.skipIf keyed on a reachability probe, or at minimum preserve signal by running a second attempt or downgrading only to test.skip(...) so CI reports show the skip explicitly rather than a green check.
Same applies to the version-parse skip branch (Lines 77-89).
Not blocking — this is a deliberate tradeoff for an npm-network-dependent smoke test, and the console.warn does leave a breadcrumb.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/29423.test.ts` around lines 59 - 64, The current
early-return on bun install failure (checking installExit, installStdout,
installStderr around the kUndiciVersion install) causes the test to silently
pass; change this to explicitly skip the test so CI shows a skip instead of a
green pass — for example, replace the return path with a test.skip(...) or
implement a test.skipIf(...) gated by a quick registry reachability probe, and
include the installExit/installStdout/installStderr details in the skip message;
apply the same change to the version-parse skip branch (the branch around lines
handling parsed version validation) so both failure cases are surfaced as
explicit skips rather than silent passes.
… test Narrow the "SharedArrayBuffer is unaffected" test from a blanket expect(() => structuredClone(sab)).not.toThrow() to one that only fails on DataCloneError, matching the test's own comment. This follows up on CodeRabbit feedback on oven-sh#29424: if Bun's structuredClone(SAB) path ever legitimately rejects for an unrelated reason, the test now stays green as long as the rejection is not a DataCloneError — which is the only thing markAsUncloneable semantics actually pin down for SharedArrayBuffer. Verified locally with: bun bd test test/js/node/worker_threads/markAsUncloneable.test.ts → 25 pass / 0 fail (66 expect() calls)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements
worker_threads.markAsUncloneable(obj)to match Node.js behavior. Once marked,objcan no longer cross realms viastructuredClone,MessagePort.postMessage,WorkerworkerData, orBroadcastChannel.postMessage— those paths now throw aDOMExceptionwithname === "DataCloneError". Primitives andArrayBuffer/SharedArrayBuffer/ Buffer-like objects are a no-op per spec. The effect is irreversible, and the marker is invisible toObject.keys,JSON.stringify,for-in, andReflect.ownKeys.Spec: https://nodejs.org/api/worker_threads.html#workermarkasuncloneableobject
First shipped in Node.js v21.0.0.
Fixes #29423
Why
undici 8.0.3 removed its runtime feature probe for
markAsUncloneable(nodejs/undici#4968), and 8.1.0'slib/web/webidl/index.jsnow callsrequire("node:worker_threads").markAsUncloneableunconditionally. Every undici constructor that goes throughwebidl.util.markAsUncloneable(this)—CacheStorage,Response,Request,Headers,FormData,WebSocket,EventSource— throws at module-load time on Bun:This cascades into Next.js / Vitest / jsdom / cheerio apps running on Bun.
Repro before this PR:
$ bun -e 'require("undici/lib/web/cache/cachestorage.js"); new (require("undici")).CacheStorage' TypeError: webidl.util.markAsUncloneable is not a functionAfter:
$ ./build/debug/bun-debug -e 'require("undici/lib/web/cache/cachestorage.js"); console.log("ok")' okImplementation
isUncloneableinsrc/js/builtins/BunBuiltinNames.h.jsFunctionMarkAsUncloneableinsrc/bun.js/bindings/webcore/Worker.cpp, exposed to the built-innode:worker_threadsmodule via$newCppFunction. No-op on primitives,null,undefined,ArrayBuffer,SharedArrayBuffer, and anyJSArrayBufferView(coversBuffer, TypedArrays,DataView). Otherwise it tags the object viaputDirectwithDontEnum | DontDelete | ReadOnly, so the marker is invisible to userland and cannot be cleared.CloneSerializer::dumpIfTerminal(inSerializedScriptValue.cpp), placed before theisArray/ object-type dispatch. Every structured-clone entry point funnels through this function, so a marked object at the root or nested inside arrays, plain objects,Mapvalues, orSetentries is caught once and surfaced asSerializationReturnCode::DataCloneError, which the existing machinery converts to the appropriateDOMException.src/js/node/worker_threads.tsswaps thethrowNotImplementedstub for$newCppFunction("Worker.cpp", "jsFunctionMarkAsUncloneable", 1)and exports it on the default export, so it works viaimport { markAsUncloneable } from "node:worker_threads", the default export, CommonJSrequire("node:worker_threads"), and the bareworker_threadsspecifier.Test matrix
test/js/node/worker_threads/markAsUncloneable.test.ts— 25 tests, 66 assertions, all passing on a debug ASAN build:typeof === "function", surface exposure on default export / named / bare specifier / CJS.undefined.null,undefined, number, string, boolean,Symbol,BigInt); accepts functions.structuredClone(marked)throwsDataCloneErrorat the root and when nested inside arrays / plain objects /Mapvalues /Setentries; also when the value itself is an array at the root or nested.MessageChannel.port1.postMessage(marked)throwsDataCloneError.BroadcastChannel.postMessage(marked)throwsDataCloneError.new Worker(url, { workerData: marked })throwsDataCloneError.ArrayBuffer,SharedArrayBuffer,Buffer, TypedArrays,DataViewstay cloneable (spec no-op).JSON.stringify).Object.keys,JSON.stringify,for-in,Reflect.ownKeys,Object.getOwnPropertyNames, and spread.markAsUncloneableworks identically inside a worker thread.test/js/node/worker_threads/markAsUncloneable-undici-regression.test.ts— end-to-end regression thatbun installs realundici@^8.1.0into a tempDir (skips cleanly offline), verifies the installed version fromnode_modules/undici/package.json, and requires the deep subpathundici/lib/web/cache/cachestorage.js(which is what triggerswebidl.util.markAsUncloneable(this)). Asserts stdout isokand stderr contains nomarkAsUncloneable is not a function.test/js/node/worker_threads/worker_threads.test.tsgainsmarkAsUncloneableentries in the module-property assertion list.Scope
Intentionally tight: only
markAsUncloneable. No otherworker_threadsbehavior is modified; #15964 and related stability bugs remain open.Local verification
Linux x64 (Ubuntu 24):
bun run zig:check-allandbun bdOK;bun bd testformarkAsUncloneable.test.ts(25/25) andtest/regression/issue/29423.test.ts(1/1) OK.