Conversation
…cesses The IPC guide, the child process reference and the Bun.spawn JSDoc said advanced-mode IPC supports everything structuredClone supports. Platform objects are deliberately not sent across the process boundary: the receiving process gets an empty object in their place, which is also what Node.js does. Say so, and list the values that do round-trip.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: docs, JSDoc and a test pinning the documented behavior; no runtime change. Ready for a maintainer ( What is in the PR now:
How the documented behavior was verified:
CI on the earlier revisions: every job that ran passed. Build 95987 was marked failed because the two The guides audit in #38441 does not touch either of the two doc lines changed here. |
There was a problem hiding this comment.
LGTM — docs-only correction to the IPC serialization description.
What was reviewed:
- Verified the claim against
SerializedScriptValue.cpp:1405-1411: non-transferable Bun classes are written asObjectTag+TerminatorTag(empty object) whenSerializationForCrossProcessTransfer::Yes. - Confirmed
test/js/node/cluster.test.tsasserts{}for BunFile and BlockList over IPC, matching the new prose. - The three edited locations (two
.mdxguides, two JSDoc blocks inbun.d.ts) use identical wording and preserve the existing MDN link and "no ownership transfer" caveat.
Extended reasoning...
Overview
This PR rewords one sentence in each of three places — docs/guides/process/ipc.mdx, docs/runtime/child-process.mdx, and two JSDoc blocks in packages/bun-types/bun.d.ts — that previously claimed advanced-mode IPC "supports everything structuredClone supports". The new text lists concrete types that do round-trip and states that platform objects (Blob, Bun.file(), net.BlockList) arrive as {}, matching Node.js. No runtime, type-signature, or test changes.
Security risks
None. Prose and JSDoc comments only; no executable code, no type surface changes (the send(message: any): void signature is untouched).
Level of scrutiny
Low. This is a documentation accuracy fix. I verified the underlying runtime behavior directly: CloneSerializer at src/jsc/bindings/webcore/SerializedScriptValue.cpp:1405-1411 writes ObjectTag + TerminatorTag for any StructuredCloneableSerialize class that isn't isForTransfer when the cross-process flag is set, and test/js/node/cluster.test.ts has two tests ("cloneable and non-transferable not-equals" for BunFile and BlockList) asserting the empty-object result. The PR description also includes an empirical probe showing Bun and Node v26.3.0 produce identical output, backing the "as it does in Node.js" claim.
Other factors
The wording is consistent across all three locations, keeps the MDN structured-clone link, and retains the existing "does not support transferring ownership" sentence. The list of supported types (strings, numbers, plain objects/arrays, Date, RegExp, Map, Set, Error, typed arrays, ArrayBuffer) matches what the JSC serializer handles and what the author's probe confirmed round-trips. No prior reviews or outstanding comments on the PR.
…the bun-only claim
Values structuredClone rejects (URL, Headers, functions) make send() throw
a DataCloneError over IPC too, so "platform objects arrive as {}" was too
broad; only Blob (File, Bun.file()) and net.BlockList degrade to an empty
object. Scope the sentence to values inside a message, since handles are
passed through a separate argument.
The two JSDoc blocks also still said IPC only works with other bun
processes, which the serialization option right below them contradicts.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the four reworded passages accurately describe the cross-process serializer's behavior. Deferring only because packages/bun-types/bun.d.ts is CODEOWNER-covered, so a human sign-off is required.
What was reviewed
- Verified the empty-object claim against
SerializedScriptValue.cpp(ObjectTag+TerminatorTagfor non-transferable Bun classes underSerializationForCrossProcessTransfer::Yes) and thetransferable: falsedeclarations for Blob and BlockList in their.classes.tsfiles. - Confirmed the
.d.tsedits are JSDoc-only; no type signatures changed. - Checked the "only other
buninstances" removal is consistent with the existingserializationJSDoc and the Bun↔Node IPC docs section.
Extended reasoning...
Overview
This PR rewords one sentence each in docs/guides/process/ipc.mdx and docs/runtime/child-process.mdx, and updates two JSDoc blocks (ipc option and Subprocess.send) in packages/bun-types/bun.d.ts. All four edits replace the claim that advanced-mode IPC "supports everything structuredClone supports" with a more accurate description: values structuredClone accepts arrive intact, values it rejects throw DataCloneError, and Blob/Bun.file()/net.BlockList arrive as {} (matching Node.js). The .d.ts change also drops the stale "only compatible with other bun instances" claim in favor of pointing at serialization: "json" for Node interop.
Security risks
None. Pure prose/JSDoc; no runtime code, no type signatures, no build inputs.
Level of scrutiny
Low. The behavior being documented is deliberate (introduced in #19351), asserted by existing tests (test/js/node/cluster.test.ts, test/js/web/workers/structuredClone-classes.test.ts), and I verified the mechanism directly in SerializedScriptValue.cpp:1405-1411 — non-transferable Bun classes are written as ObjectTag+TerminatorTag when m_forTransfer == SerializationForCrossProcessTransfer::Yes. Both Blob (response.classes.ts) and BlockList (sockets.classes.ts) declare transferable: false. The "as in Node.js" qualifier is scoped only to the Blob/BlockList row, which the PR's probe output supports.
Other factors
The .d.ts edits touch only comment text — no interface members, parameter types, or return types change — so the bun-types integration test is unaffected (and the PR reports it passing). The removed "another instance of bun" clause was already contradicted by the neighbouring serialization JSDoc and the "IPC between Bun & Node.js" docs section, so this brings the JSDoc into line with the rest of the docs. I would have approved outright, but packages/bun-types/ is listed in .github/CODEOWNERS, so per policy I'm deferring for the owner's sign-off.
Values structuredClone clones keep their types, values it rejects make send() throw DataCloneError in both directions, and Blob, Bun.file() and net.BlockList arrive as empty objects. The existing coverage only checked Bun.file() through node:cluster; this covers Bun.spawn in both directions so the docs and the serializer cannot drift apart silently.
There was a problem hiding this comment.
LGTM — docs/JSDoc corrections plus a test that pins the described contract; no runtime code touched.
What was reviewed:
- Verified the doc claim against
SerializedScriptValue.cpp:1405-1411— cross-process serialization of a non-isForTransferBun class writesObjectTag+TerminatorTag(empty object), andresponse.classes.tsmarks Blobtransferable: false. - Checked the new test for flakiness:
onMessageis armed beforespawn()and before eachsend(),onExitis wired to reject the raced promise, andexitedEarly.catch(() => {})prevents an unhandled rejection when the child exits normally at the end. - The "only other
buninstances" JSDoc removal is consistent with the neighbouringserializationJSDoc and the existing bun↔node IPC test.
Extended reasoning...
Overview
This PR touches four files with no runtime code changes:
- Two docs pages (
docs/guides/process/ipc.mdx,docs/runtime/child-process.mdx) — one sentence each, rewording the advanced-mode IPC description from "supports everything structuredClone supports" to the accurate contract: structuredClone-accepted values keep their types, structuredClone-rejected values throwDataCloneError, andBlob/Bun.file()/net.BlockListdegrade to{}. packages/bun-types/bun.d.ts— the same rewording in theipcoption andSubprocess.send()JSDoc, plus replacing the stale "only compatible with otherbuninstances" claim with the real constraint (advanced needs bun↔bun; use"json"for Node.js).test/js/bun/spawn/spawn.ipc.test.ts— a new test that exercises every branch of the documented contract in both directions (child→parent and parent→child).
Security risks
None. This is documentation and a read-only test; no runtime, parser, or serializer code is modified.
Level of scrutiny
Low-to-medium. The docs edits are prose only and I confirmed the underlying behavior against the source: SerializedScriptValue.cpp:1405-1411 writes ObjectTag+TerminatorTag for any StructuredCloneableSerialize whose isForTransfer is false when SerializationForCrossProcessTransfer::Yes, and response.classes.ts:150 sets transferable: false for Blob. The "as in Node.js" clause is scoped only to the Blob/BlockList sentence, matching the probe output in the PR description (Node delivers {} for URL/Headers where Bun throws, so parity is not claimed there).
The new test warranted a closer read for the flakiness patterns REVIEW.md calls out. It awaits real conditions (no sleeps), wires onExit into a rejected promise raced against every awaited message so an early child crash surfaces as a clear failure, arms onMessage before each send so no message can be lost to a race, registers exitedEarly.catch(() => {}) up front so the deliberate exit at the end does not produce an unhandled rejection, uses await using for cleanup, and asserts exact object shapes plus constructor identity for the degraded values. If process.send(URL) unexpectedly succeeded instead of throwing, the extra message would resolve fromChild with the wrong shape and the toEqual on kind: "from-child" would fail — so the DataCloneError assertion is not vacuous.
Other factors
- The behavior being documented is already asserted by
test/js/node/cluster.test.tsandtest/js/web/workers/structuredClone-classes.test.tsper the PR description; this PR adds directBun.spawnIPC coverage in the file that owns that surface. - The
.d.tschange is JSDoc-only (no type signatures altered), so the bun-types integration test is the relevant gate and the PR reports it passing. - No prior human or claude[bot] reviews on this PR; only bot status comments.
Problem
docs/guides/process/ipc.mdx:57,docs/runtime/child-process.mdx:304and theipc/Subprocess.sendJSDoc inpackages/bun-types/bun.d.tssay advanced-mode IPC supports everythingstructuredClonesupports.Blob(includingFileandBun.file()) or anet.BlockListinside a message arrives in the other process as{}, whilestructuredCloneof the same value returns a working clone. This came up while auditing the IPC guide.CloneSerializerwritesObjectTag+TerminatorTag(an empty object) for any Bun class not markedtransferablewhen serializing for a cross-process transfer (src/jsc/bindings/webcore/SerializedScriptValue.cpp:1405-1411);BlobandBlockListare the two such classes (src/runtime/webcore/response.classes.ts,src/runtime/socket/sockets.classes.ts). Introduced on purpose in js: fix serialization of non-transferable objects #19351.test/js/node/cluster.test.ts("cloneable and non-transferable not-equals", BunFile and BlockList) andtest/js/web/workers/structuredClone-classes.test.tsassert the empty object.Blob,Fileandnet.BlockListsent throughchild_processIPC arrive as{}in bothjsonandadvancedmode (probe below).bunprocesses. That has been false sinceserialization: "json"landed: theserializationJSDoc a few lines below, the "IPC between Bun & Node.js" section ofchild-process.mdx, andtest/js/bun/spawn/spawn.ipc.bun-node.test.tsall have Bun talking to a Node child.Fix
structuredCloneand name the Blob/BlockList deviation from it: the valuesstructuredCloneaccepts arrive with their types intact, the values it rejects makesend()throw aDataCloneError(verified forURL,Headers,Response,FormData,AbortSignal,MessagePortand a nested function, from bothprocess.sendandsubprocess.send), and aBlob/Bun.file()/net.BlockListinside a message arrives as{}, as in Node.js.net.Server/net.Sockethandles are passed through the separate handle argument (cluster: port Node's cluster and child_process handle-passing suites (+43 upstream tests; cluster 54 → 85) and implement what they expose — round-robin fd handoff, SCHED_NONE shared handles, UDP clustering, IPC handle passing #31829), which this text does not describe.SharedArrayBuffer,KeyObject) are deliberately not listed either way.buninstances" sentence in theipcJSDoc with the real constraint (the default"advanced"format can only be read by anotherbunprocess; use"json"for Node.js) and drop the same clause fromsend(), matching the neighbouringdisconnect()JSDoc.test/js/bun/spawn/spawn.ipc.test.tsthat pins the documented contract throughBun.spawn({ ipc })in both directions:Date/Map/Uint8Arraykeep their types,URLand a nested function makeprocess.sendandsubprocess.sendthrowDataCloneError, andBlob,Bun.file(),net.BlockListand a nestedBlobarrive as plain empty objects. Existing coverage only checkedBun.file()throughnode:cluster. Since this PR changes no runtime code, the test passes before and after it; it exists so a later change to the serializer (for example one of the open PRs that touch cross-process serialization) fails this test and updates the docs along with it.{}forURL/Headerswhere Bun throws, so the text does not claim Node parity for those).bun bd test test/js/bun/spawn/spawn.ipc.test.tspasses (14 tests, including the new one);bun test test/integration/bun-types/bun-types.test.tspasses with the JSDoc edits.Background
jsonusesJSON.stringify;advanced(the default forBun.spawn({ ipc })and forchild_processwithserialization: "advanced") uses the structured clone serializer with a cross-process flag set..classes.tsfiles. That declaration also says whether the class may cross a process boundary (transferable). A file-backed Blob serializes as the sending process's path or raw file descriptor number (src/runtime/webcore/blob/Store.rs:177) and a BlockList serializes as a handle to the native object inside the sending process (src/runtime/node/net/BlockList.rs:411), so neither is marked transferable; in-processstructuredClone,postMessageandbun:jscserialization do not set the cross-process flag and still clone them.structuredClone, which is why unsupported values throw the sameDataCloneErrorover IPC that they throw in-process.DataCloneError) for the two Bun classes mirrors what V8's serializer produces for these values in Node'schild_processIPC.Probe 1: round-trips. A child sends each value with
process.send, the parent prints what it receivedSame two files run under both runtimes (
bun probe-parent.mjs advanced/node probe-parent.mjs advanced);bunFileis only sent under Bun. Bun 1.4.0 and Node v26.3.0 print the same lines.Probe 2: structuredClone vs IPC for platform objects (advanced mode)
subprocess.send()from aBun.spawn({ ipc })parent behaves the same asprocess.send()above:URLand a nested function throwDataCloneError, a nestedBlobis delivered as{"b":{}}.