Repository navigation
Conversation
…hannel
`subprocess.send(() => {})` was accepted: it returned true, invoked the send
callback with a null error, and then tore the IPC channel down asynchronously.
Messages queued behind it were never delivered and later sends failed with
ERR_IPC_CHANNEL_CLOSED.
Node's guard in `target._send` rejects anything whose `typeof` is not
string/object/number/boolean. Bun spelled that guard with `JSValue::is_object()`,
which is JSC's "is a JSObject" and is also true for functions, so callables slipped
through. `JSON.stringify` then mapped the function to `undefined`, which reaches
Rust as a null WTF::String (the `Empty` tag, not `Dead`), and the existing `Dead`
check missed it. The result was a zero-length JSON frame on the wire, which the
peer rejects as malformed and answers by closing the socket. A node child in the
same position dies with `SyntaxError: Unexpected end of JSON input`.
- exclude callables from the message type guard, so `typeof`-equivalence holds
- treat an empty `JSON.stringify` result as a serialization failure: it can never
be a value we can send, and writing it poisons the channel
- return the serialization error from `serialize_and_send` rather than flattening
it to a generic transport failure, so `do_send` can throw ERR_INVALID_ARG_TYPE
(node semantics) and propagate a pending JS exception instead of replacing it
with `TypeError: process.send() failed` and then reporting success to the
send callback
|
Updated 11:47 PM PT - Jul 5th, 2026
❌ @robobun, your commit 6878f0d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33383That installs a local version of the PR into your bun-33383 --bun |
|
Warning Review limit reached
Next review available in: 1 minute 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 (2)
WalkthroughThis PR changes IPC serialization error handling in the JSC/runtime layer from an enum-based ChangesIPC Send Error Handling Refactor
Sequence Diagram(s)sequenceDiagram
participant do_send
participant SendQueue
do_send->>SendQueue: serialize_and_send(message)
SendQueue-->>do_send: Err(IPCSerializationError) or Ok(SerializeAndSendResult)
do_send->>do_send: validate message / map error
Related PRs: None identified. Suggested labels: rust, ipc, tests, needs-review Suggested reviewers: None identified. Poem: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/ipc.rs`:
- Around line 1378-1395: The backoff flag in SendQueue#serializeAndSend is
computed too early and can become stale because serialize(...) may run user JS
via toJSON and re-enter IPC. Recompute indicate_backoff only after
serialize(...) returns, immediately before start_message(...) and the final
result selection, so the return value reflects the current waiting_for_ack and
queue state. Keep the rest of the SendQueue#serializeAndSend flow unchanged,
just move the backoff check to the post-serialization point.
- Around line 286-295: The IPCSerializationError::pending_exception method is
dropping OutOfMemory by returning no pending exception, which causes later send
paths to replace it with a generic failure. Update the IPCSerializationError
handling so OutOfMemory is preserved either by returning JsError::OutOfMemory
from pending_exception or by treating it as a fatal OOM before the generic
process.send()/sendInternal() fallback allocates a new error. Make sure the
change is applied alongside the other IPCSerializationError variants in this
match so downstream callers keep the original OOM signal.
In `@src/runtime/ipc_host.rs`:
- Around line 74-77: The doc comment above the `JSValue::is_object()` check in
`ipc_host` is too long and must be trimmed to three lines. Shorten the comment
while preserving the key point about Node’s `target._send` type guard and
excluding callables because `JSValue::is_object()` includes functions. Keep the
wording concise and verify the final comment stays within the repo’s 3-line
limit.
In `@src/runtime/node/node_cluster_binding.rs`:
- Around line 121-138: The send failure path in node_cluster_binding’s helper
that calls serialize_and_send leaves the newly registered StrongOptional
callback entry behind, so clean up the just-inserted seq from the pending
callback map before returning on Err. Update the error arm around
serialize_and_send to mirror the ACK cleanup logic used with swap_remove,
ensuring the callback ref is removed and the entry does not remain unsettled;
apply this in the helper that builds the ex object and schedules
__jsc_host_impl_.
🪄 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: 1381bff8-38e2-4ba9-b27e-3b101152dc4f
📒 Files selected for processing (4)
src/jsc/ipc.rssrc/runtime/ipc_host.rssrc/runtime/node/node_cluster_binding.rstest/js/node/child_process/child_process_ipc.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- propagate OOM from serialization as JsError::OutOfMemory rather than flattening it into `TypeError: process.send() failed` with `syscall: "write"`. With every IPCSerializationError variant now fully handled, that generic transport error is dead on this path, so delete it. - report a message the type guard accepts but JSON cannot represent (toJSON() returning undefined) as ERR_INVALID_ARG_VALUE "could not be serialized to JSON" instead of reusing the type-guard message, which claimed an object was not an object. - read the send queue state for the backoff result after serialization, which can run toJSON and re-enter. - release the cluster callback entry when serialize_and_send fails: nothing was enqueued, so no ack would ever settle it and the Strong leaked.
|
Addressed all five review findings in e99d188. Each one held up against the code, so all five are fixed rather than waved off. OOM was being flattened into a fake write error. This had a knock-on effect worth calling out: with all four The error for
Used the centralized The cluster callback entry leaked on send failure. Confirmed: the only Backoff was read before serialization, which can run Comment length. Trimmed, plus a second 4-line comment I had added in Re-verified: |
`serialize_and_send` runs `JSON.stringify`, which can invoke a user `toJSON` that re-enters `sendHelperChild` and takes a second `&mut` to the same static. Cleaning up the callback entry through the outer `singleton` binding kept that borrow live across the call, which `child_singleton`'s aliasing contract forbids. Re-borrow in the error arm so the outer borrow ends before serialization, as it did before.
|
Pushed 9a281a9, which fixes a real aliasing bug the previous round introduced, and documents why the red lane on build 68602 is not this PR. The aliasing bug (fixed)The callback-entry cleanup I added in e99d188 cleaned up through the outer
Fixed by re-borrowing in the error arm, so the outer borrow's last use is back to Err(err) => {
// Nothing was enqueued, so no ack will ever settle the callback registered above.
child_singleton().callbacks.swap_remove(&seq);
Build 68602Three annotations, one red lane:
An exception-check violation: a This PR cannot cause it. The test requires only Locally the test is green 73/73 on a main-built ASAN binary (25 serial + 48 under 6-way contention) and 30/30 on this branch's build, which is what you would expect: the lane that caught it runs an ASAN-instrumented JSC, and the extra slowdown is what widens the termination window. I did not spend the retrigger on it — this push re-runs CI anyway. Verification on 9a281a9 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/ipc.rs (1)
196-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate stale
.failurereferences after removing the variant.
SerializeAndSendResult::Failureis gone, but the OOM comments still sayserializeAndSendreturns.failure. Update both comments to describe the newErr(IPCSerializationError::OutOfMemory)path. As per coding guidelines, comments should carry durable, accurate non-obvious content.Proposed comment update
- // Propagate OOM so serializeAndSend - // returns `.failure` instead of silently discarding the Result. + // Propagate OOM as `Err(IPCSerializationError::OutOfMemory)`.Also applies to: 443-447, 612-616
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/ipc.rs` around lines 196 - 199, Update the stale OOM comments that still mention SerializeAndSendResult::Failure, since that variant was removed. In the comments around serializeAndSend and the related IPC send paths (including the references near SerializeAndSendResult and the later OOM handling sites), replace any “returns .failure” wording with the current Err(IPCSerializationError::OutOfMemory) behavior. Keep the comments accurate and durable by describing the error-based path instead of the removed enum variant.Source: Coding guidelines
src/runtime/node/node_cluster_binding.rs (1)
226-239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrop the
ipc_databorrow beforeserialize_and_send
serialize_and_sendcan re-enter via usertoJSON(), so this keeps a mutable borrow alive across a call that may invokesendHelperagain on the same subprocess. Re-fetch the queue in the error path, assend_helper_childdoes, instead of reusing the pre-call borrow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/node/node_cluster_binding.rs` around lines 226 - 239, The mutable ipc_data borrow is still held across serialize_and_send in the send path, which can re-enter through user toJSON() and conflict with a nested sendHelper call on the same subprocess. Update the logic in node_cluster_binding.rs around the serialize_and_send call so the queue borrow is dropped before invoking it, following the pattern used by send_helper_child, and in the error path re-fetch ipc_data.internal_msg_queue before removing the callback with seq instead of reusing the pre-call borrow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/jsc/ipc.rs`:
- Around line 196-199: Update the stale OOM comments that still mention
SerializeAndSendResult::Failure, since that variant was removed. In the comments
around serializeAndSend and the related IPC send paths (including the references
near SerializeAndSendResult and the later OOM handling sites), replace any
“returns .failure” wording with the current
Err(IPCSerializationError::OutOfMemory) behavior. Keep the comments accurate and
durable by describing the error-based path instead of the removed enum variant.
In `@src/runtime/node/node_cluster_binding.rs`:
- Around line 226-239: The mutable ipc_data borrow is still held across
serialize_and_send in the send path, which can re-enter through user toJSON()
and conflict with a nested sendHelper call on the same subprocess. Update the
logic in node_cluster_binding.rs around the serialize_and_send call so the queue
borrow is dropped before invoking it, following the pattern used by
send_helper_child, and in the error path re-fetch ipc_data.internal_msg_queue
before removing the callback with seq instead of reusing the pre-call borrow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d1ba771-a6c5-4df8-8a9c-9e766cbb6062
📒 Files selected for processing (4)
src/jsc/ipc.rssrc/runtime/ipc_host.rssrc/runtime/node/node_cluster_binding.rstest/js/node/child_process/child_process_ipc.test.js
…nd_send
`send_helper_primary` had the same problem just fixed in `send_helper_child`: cleaning
up the callback entry through the pre-call `ipc_data` binding held the
`&mut SendQueue` across `serialize_and_send`, which runs `toJSON` and can re-enter
`sendHelper` on the same subprocess. `Subprocess::ipc`'s contract forbids exactly that
("callers do not hold the borrow across JS re-entry that touches `ipc_data` itself").
Re-borrow in the error arm so the outer borrow's last use is the send call again.
Also drop two comments still describing the removed `SerializeAndSendResult::Failure`.
|
Fixed both findings in 6878f0d. The second one is a correction to something I got wrong last round.
|
There was a problem hiding this comment.
Both prior findings are addressed and nothing new turned up this round; deferring to a human since this reworks the shared IPC send path (return-type change on serialize_and_send, removed Failure/process.send() failed fallback, re-entrant borrow handling in the cluster helpers) and that's more than I should rubber-stamp.
Extended reasoning...
Overview
Fixes subprocess.send() / process.send() accepting non-JSON-serializable values (functions, toJSON() → undefined) and then tearing down the IPC channel asynchronously. Changes span four files: src/jsc/ipc.rs (removes SerializeAndSendResult::Failure, adds IPCSerializationError::as_js_error() + From<JsError>, switches json::serialize to out.is_empty(), moves the backoff read after serialization, changes serialize_and_send to return Result<_, IPCSerializationError>), src/runtime/ipc_host.rs (adds is_sendable_message() with the !is_callable() fix, replaces the generic process.send() failed error with typed propagation and a distinct ERR_INVALID_ARG_VALUE for JSON-unrepresentable objects), src/runtime/node/node_cluster_binding.rs (both send helpers now match on Result, clean up the registered callback on Err, and re-borrow the singleton/subprocess IPC to avoid holding a &mut across re-entrant toJSON), and four new tests in child_process_ipc.test.js.
Security risks
None identified. This is input validation on the IPC send path — it tightens the guard (functions now rejected) and stops writing malformed frames to the peer. No auth/crypto/permissions code touched.
Level of scrutiny
Medium-high. The change is well-reasoned and thoroughly tested, but it's not mechanical: it changes a public function's return type, removes an enum variant and the associated process.send() failed fallback (on the argued basis that write errors never flow through this path), and touches the send queue used by child_process, Bun.spawn IPC, and node:cluster. The re-entrancy/aliasing handling in node_cluster_binding.rs needed two follow-up commits (9a281a9 for send_helper_child, 6878f0d for send_helper_primary) to keep the documented SAFETY contracts on child_singleton() / Subprocess::ipc() true — the final shape looks right, but that's exactly the kind of subtlety a maintainer should confirm.
Other factors
Both of my earlier inline findings (self-contradictory ERR_INVALID_ARG_TYPE message for { toJSON: () => undefined }; child_singleton() borrow extended across serialize_and_send) were addressed, and all CodeRabbit threads are resolved. Test coverage is solid: exact error code + message assertions for each case, plus a channel round-trip afterward proving the queue is untouched. The PR description documents USE_SYSTEM_BUN=1 failure and bun bd pass, and re-verified after each fix commit. No new findings from the bug-hunting system.
Status: diff is green, CI is red on infra only. Needs a maintainer to merge.Build 68621 on
The single failed job never ran a test: The two remaining annotations are also infra: Why none of this is the diffThree builds, three different infra failures, and the one thing that looked real is now disproved:
The 68602 asan SIGABRT was an exception-check violation ( I spent the one retrigger on 68614. Not pushing another; the darwin artifact download is failing on that lane regardless of what is in the branch. What's in the PR
Fixed by excluding callables from the guard, failing serialization on an empty Review caught three things beyond the original bug, all fixed and all worth a look since they touch shared state: OOM was being flattened into a fake VerificationFour new tests in Both review bots have signed off and every thread is resolved. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-05, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
|
Reopened as #43005 against current main. |
1 participant
Repro
subprocess.send()of a valueJSON.stringifymaps toundefined(a function, asymbol, a
toJSON()returningundefined) was accepted: it returnedtrueandcalled the send callback with a
nullerror. The IPC channel was then torn downasynchronously —
'error'(ERR_IPC_CHANNEL_CLOSED) and'disconnect'fire, awell-formed message sent immediately after (while
connected === true) is neverdelivered, and later sends fail. Node throws
ERR_INVALID_ARG_TYPEsynchronously andthe channel keeps working.
Same for
process.send()in the child, in bothjsonandadvancedserialization.Cause
Node's guard lives in
target._send:do_sendmirrors it, but spellstypeof message === "object"asJSValue::is_object(). That is JSC's is aJSObjectpredicate, which is alsotrue for functions, so callables slip past the guard that exists to stop them.
json::serializethen callsJSON.stringify, which returnsundefinedfor afunction. That reaches Rust as a null
WTF::String, andBun::toStringRefmaps anull string to the
Emptytag — notDead, which is the only tag the existingguard checked. So the value was serialized to a zero-length JSON frame (
"\n")and written to the channel.
The peer's decoder rejects an empty payload as
InvalidFormatand closes thesocket. With a node child on the other end, it dies outright:
Separately, when serialization throws (cyclic structure, throwing
toJSON,DataCloneErrorin advanced mode),serialize_and_sendflattened the error to ageneric
Failure, anddo_sendbuilt a freshTypeError: process.send() failedon top of the still-pending exception. The observable result was
send()throwingthe real error and the send callback firing with
null, i.e. reporting successfor a message that was never sent.
Fix
do_send: exclude callables from the message type guard, restoringtypeof-equivalence with node. Functions, symbols and bigints now all throwERR_INVALID_ARG_TYPEsynchronously, in both serialization modes and from bothsubprocess.send()andprocess.send().json::serialize: treat an emptyJSON.stringifyresult as a serializationfailure.
JSON.stringifynever returns""for a value it can represent, so anempty result always means the message is not serializable — writing it anyway is
what poisons the channel.
serialize_and_sendreturns theIPCSerializationErrorinstead of flattening itto
Failure, sodo_sendcan report each cause honestly:OutOfMemoryErrorrather than a fake write errortoJSON()returningundefined) throwsERR_INVALID_ARG_VALUE: The "message" argument could not beserialized to JSON
With every variant now handled, the old
TypeError: process.send() failedwithsyscall: "write"is dead and removed. It was never a transport error in the firstplace: a real write failure closes the socket inside
continue_sendand neverreturns through
serialize_and_send.Serialization already ran before anything was enqueued, so a rejected message leaves
the queue, the callback and the channel untouched.
Two adjacent fixes in the same error arm:
node_cluster_binding's send helpersregister a
StrongOptionalcallback keyed byseqbefore serializing, and nothingremoved it when the send failed (no ack arrives for a message that was never
enqueued), so they now release it; and the backoff flag is read after serialization,
which can run
toJSONand re-enter.Verification
test/js/node/child_process/child_process_ipc.test.jsgains four tests coveringsubprocess.send(),process.send()in the child,toJSON()returningundefined,and the false-success callback on a cyclic message. Each asserts the exact error, and
that the channel still round-trips a message afterwards.
All four fail on
mainand pass with the fix:USE_SYSTEM_BUN=1 bun test(before)The repro above now exits 0, matching node.
test/js/node/child_process/,test/js/bun/spawn/IPC tests,test/js/node/cluster.test.tsand node's portedtest-child-process-send-*.js(includingtest-child-process-send-type-error.js)are unchanged.