Repository navigation
Conversation
|
Updated 2:38 AM PT - Oct 1st, 2026
✅ @robobun, your commit 4102d5bcb9917aadfd34f358f5a05226db8dff45 passed in 🧪 To try this PR locally: bunx bun-pr 41791That installs a local version of the PR into your bun-41791 --bun |
|
Status: reproduced on 1.4.3 canary with the report's script: Rebased onto main ( |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughBun now reports invalid IPC serialization formats as uncaught errors, identifies the peer and expected format, and closes the channel. Tests cover malformed frames and incompatible Node and Bun advanced serialization. The documentation describes the mismatch behavior. ChangesIPC decode errors
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is established. Invalid IPC formats now produce an explicit error instead of silently closing the channel. Merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — turns a silent channel death into a diagnosable error, with the remedy in the message.
What was reviewed:
report_undecodable_messageordering:close_socketdoes not clearself.owner(onlydetach()does), soowner_ref()/get_global_this()/peer()are still valid after the close and the SAFETY comment on the newunsafe { p.as_ref() }holds by the same invariant as the four neighboring methods.- The catch-all
Fail(_)arm is now exhaustive (Stopped | NotEnoughBytes), so no variant silently changed behavior. - Tests: both peer directions, both modes, real-Node and raw-frame paths;
isWindowsskips are justified inline and Windows is covered by the node-peer tests; subprocess pipes drained concurrently and output asserted before exit code.
Extended reasoning...
Overview
This PR changes finish_decode in src/runtime/ipc.rs so that IPCDecodeError::InvalidFormat — previously handled by a catch-all arm that only closed the socket — now closes the socket and then reports a descriptive uncaught exception via dispatch::fold. A new SendQueueOwner::peer() helper and PeerName Display enum name the other end of the channel in the error message. One sentence is added to docs/runtime/child-process.mdx, and three existing test files gain coverage: real Bun↔Node pairings in both directions gated on nodeExe(), POSIX raw-frame injection of Node's exact v8-serialized bytes, and updated assertions on three pre-existing invalid-frame tests.
Security risks
None. The change only adds an error report on a path that was already terminating the IPC channel; no new input parsing, no allocation sized by peer data, no auth/crypto/permissions surface. The one new unsafe block (unsafe { p.as_ref() }.pid()) is the identical pattern used four times immediately above it in the same impl for global_this, handle_ipc_close, handle_ipc_message, and this_jsvalue, guarded by the same "attached owner is live" invariant — close_socket does not detach the owner (only detach() clears self.owner), so the invariant holds at the new call site.
Level of scrutiny
Moderate. This is native runtime code with an unsafe block, so I traced the ordering: close_socket → socket_closed_notify schedules a deferred close notification but does not clear the owner or run JS synchronously, so owner_ref() and get_global_this() remain valid when report_undecodable_message runs. The error is constructed with global.create_error_instance(format_args!(...)) and folded through dispatch::fold, exactly mirroring the adjacent IPCDecodeError::Js arm. The previously catch-all Fail(_) arm is now explicitly Stopped | NotEnoughBytes, keeping the match exhaustive and confirming no other variant's behavior shifted.
Other factors
Test coverage is thorough and follows repo conventions closely: await using on spawns, Promise.all draining stdout/stderr/exited, output asserted before exit code, test.skipIf(!nodeExe()) for Node-dependent cases, and isWindows skips justified with a comment naming the libuv framing reason and cross-referencing the tests that do cover Windows. The change is small (~30 lines of Rust), self-contained, strictly additive to a failure path (working IPC never hits InvalidFormat), and no CODEOWNERS entries cover these files. The bug hunt ran to dry_streak with no findings and no outstanding review threads exist.
…ently When the bytes on an IPC channel are not a message in its serialization format (a Node.js peer on an "advanced" channel, or garbage), the decoder returned InvalidFormat and finish_decode closed the socket without a word. The receiver now closes the channel and then reports an uncaught exception that names the peer and, for "advanced", the serialization: "json" remedy. This matches the existing arm for a frame whose payload fails to deserialize.
7ae47fa to
cf6be49
Compare
| DecodeStep::Fail(IPCDecodeError::Js(err)) => { | ||
| crate::dispatch::fold(Err(*err)); | ||
| send_queue.close_socket(CloseReason::Failure, CloseFrom::User); | ||
| } | ||
| DecodeStep::Fail(_) => { | ||
| DecodeStep::Fail(IPCDecodeError::InvalidFormat) => { | ||
| send_queue.close_socket(CloseReason::Failure, CloseFrom::User); | ||
| report_undecodable_message(send_queue); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: nit (pre-existing): an uncaughtException handler sees child.connected as true for one undecodable-frame class and false for the other, so a send() inside it is accepted on one path and then the channel closes under it. The Js arm at src/runtime/ipc.rs:2293 reports before close_socket, while the new InvalidFormat arm at src/runtime/ipc.rs:2297 closes first. Fix: make both arms close before reporting so a handler always observes the channel disconnected; close_socket does not enter JS (it only enqueues a deferred task), so the pending exception is not consumed early by closing first. The comment's claim that the Js exception must be consumed before anything else runs is not what the code requires.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A peer sends a well-formed advanced frame whose payload fails JSValue::deserialize (src/runtime/ipc.rs:355) or fails ipc_restore_advanced_buffers (src/runtime/ipc.rs:357). The Js arm at src/runtime/ipc.rs:2293-2296 calls crate::dispatch::fold first, which runs the user's uncaughtException handler synchronously, and only then close_socket. Inside that handler is_connected() (src/runtime/ipc.rs:1093) is still true, so do_send (src/runtime/ipc_host.rs:116) accepts a message that start_message queues; the close that follows drains the queue through abort_unsent (src/runtime/ipc.rs:807), which never calls the user's send callback. The InvalidFormat arm at src/runtime/ipc.rs:2297-2300 closes first, so the same handler sees connected false and send() rejects with IPC_CHANNEL_CLOSED. close_socket (src/runtime/ipc.rs:1101) only sets state and calls schedule_deferred, which enqueues a task without running JS, so the ordering in the Js arm could match the new arm. The base branch had the same Js-arm ordering; the diff adds the second arm and the comment asserting the difference is required.
Verification: The comment at src/runtime/ipc.rs:2285-2292 says the Js arm reports first because "the pending exception is consumed before anything else runs", but that is a choice, not a constraint: close_socket (src/runtime/ipc.rs:1101-1146) only calls s.close(..) -> uws on_close -> socket_closed_notify, which sets socket = Closed and schedule_deferred(); none of that enters JS or consumes the exception.
There was a problem hiding this comment.
The deserialize arm has the same order as on main. This PR does not change it. The comment that called that order a requirement is gone in cb22a24.
To make both arms close first is a behaviour change for an existing path: a handler for Unable to deserialize data. would then see connected === false. I left that for a maintainer to decide. It is a two-line change if it is wanted.
Restore the original comment on the deserialize arm and keep one line on the new arm. Add the missing case: a child on a "json" channel whose parent sends a line that is not JSON.
A Node.js peer writes the line "undefined" when it sends an object whose toJSON() returns undefined. Use that to test the "json" report in both directions on every platform that has node.
| Mode::Json => global.create_error_instance(format_args!( | ||
| "{peer} sent an IPC message that is not valid JSON, so Bun closed the IPC channel." | ||
| )), |
There was a problem hiding this comment.
🟡 nit (optional): a user debugging a bad "json" IPC line gets only "not valid JSON" and never learns what was rejected or why. The parser's SyntaxError is discarded by global_this.clear_exception() at src/runtime/ipc.rs:533 before the InvalidFormat report is built, and the report at src/runtime/ipc.rs:2314-2316 echoes neither that message nor any part of the line. Fix: carry the cause into the report, e.g. keep the SyntaxError's message (or a bounded prefix of the offending line) alongside InvalidFormat and append it to the "json" error text, so the same report also covers the empty-line and lone-tag-byte rejections at ipc.rs:491-492.
Why this was flagged
A peer in serialization "json" writes a line such as "undefined\n" or "{not json\n"; it arrives through on_data2 -> decode_next_json -> json::decode_ipc_message (src/runtime/ipc.rs:456). to_js_by_parse_json throws a SyntaxError that names the bad token; the Err(JsError::Thrown) arm at src/runtime/ipc.rs:531-534 calls clear_exception() and returns InvalidFormat, so the cause is gone. finish_decode then calls report_undecodable_message (src/runtime/ipc.rs:2304), whose Mode::Json arm at 2314-2316 formats only the peer name and the fixed sentence "sent an IPC message that is not valid JSON, so Bun closed the IPC channel." The user sees a report (the base branch printed nothing) but cannot tell which line or which parse failure caused it; Node, whose behaviour the PR cites, surfaces the SyntaxError itself with the unexpected token. REVIEW.md asks that error messages carry the rejected value and the underlying cause; this one carries neither. No safeguard prevents it: the exception is cleared before any caller can read it.
Verification: nit. Any peer in serialization "json" writes a line that JSON.parse rejects. /home/claude/bun/src/runtime/ipc.rs:521 throws a SyntaxError; the Err(JsError::Thrown) arm at lines 531-535 does global_this.clear_exception(); return Err(IPCDecodeError::InvalidFormat);, so the parser's message is discarded. The message at 2314-2316 is built solely from {peer} and fixed text.
There was a problem hiding this comment.
The SyntaxError is cleared in json::decode_ipc_message on main too, so the report in this PR has no cause to show. To carry it (for example as the cause of the reported error) changes the shape of that error, and the shape is the open question on this PR. I will add it in the shape that a maintainer picks.
| 'a bun child of a node parent using serialization: "advanced" reports the mismatch', | ||
| async () => { | ||
| // Node's "advanced" serialization is v8's wire format, which Bun cannot decode. |
There was a problem hiding this comment.
🟡 nit (optional): the new subprocess tests run serially, so the suite takes longer than it needs to on the debug/ASAN lanes. sweep:^test\.skipIf\(!nodeExe\(\)\)\( Each new test in spawn.ipc.node-bun.test.ts:55 and spawn.ipc.bun-node.test.ts:46 spawns a node parent/child plus a bun process and shares no state with its neighbours. Fix: make every independent subprocess test in the two files test.concurrent.skipIf(!nodeExe())(...) (test/CLAUDE.md asks for concurrent tests whenever several tests in one file spawn processes), leaving the fixture-based first test alone if it must stay serial.
Why this was flagged
test/CLAUDE.md says to prefer test.concurrent when multiple tests in the same file spawn processes. The four tests added at test/js/bun/spawn/spawn.ipc.node-bun.test.ts:55 and :72 and test/js/bun/spawn/spawn.ipc.bun-node.test.ts:46 and :66 each spawn a node process that in turn spawns or is spawned by a bun process, and none of them share files, ports or globals. They are declared with plain test.skipIf(!nodeExe()), so each file's two-process startups run one after another; on debug+ASAN CI lanes (10-100x slower per test/CLAUDE.md) that is avoidable wall time. The base branch had only one serial test per file, so the cost is new with this change. No functional failure; this is a convention slip.
Verification: nit. Triggering condition: whenever the suite runs, the four new two-process tests execute one after another. Mechanism verified: test/js/bun/spawn/spawn.ipc.node-bun.test.ts:54 and :71 and test/js/bun/spawn/spawn.ipc.bun-node.test.ts:46 and :64 are all declared test.skipIf(!nodeExe())(...). test/CLAUDE.md:22 states "Prefer concurrent tests over sequential tests".
There was a problem hiding this comment.
Agreed. test.concurrent.skipIf(!nodeExe()) is the form the repo uses. I am not pushing for this alone, because a push restarts the CI run that this PR waits on. It goes in with the next push to this branch.
Problem
Bun.spawn(["node", ...], { ipc })defaults toserialization: "advanced", which only Bun speaks. The parent cannot decode the Node child's first message, and the channel dies silently. A malformed"json"line does the same.finish_decode(src/runtime/ipc.rs) closed the socket onIPCDecodeError::InvalidFormatand reported nothing. The arm beside it (an undeserializable payload) already raises an uncaught exception.Fix
InvalidFormatnow closes the channel, then raises an uncaught exception in the receiver. The message names the peer and, for"advanced", the remedy:use serialization: "json".test/js/bun/spawn/spawn.ipc*.test.ts(three files). Stock bun fails 10 of 28, the fix passes 28.Background
"advanced"frames are[u8 type][u32 LE length][JSC payload]. Node's are[u32 BE length][v8.serialize() payload], so the first byte is0x00and matches no frame type.node:child_processin Bun defaults to"json", which both runtimes share.Downsides
uncaughtExceptionlistener now exits 1 on the first undecodable message. On main it continued. A supervisor dies from one child's bad line, as on Node.Bun.spawnparent with a Node child that only replies. Node reads Bun's version packet as a 16 MB frame length and waits.Notes
uncaughtExceptionhandler the program continues, andconnectedis alreadyfalseinside the handler.onDisconnect, an'error'event, or an errorcodeon this exception (it has none)."json"mode still closes the channel on a malformed line (Node keeps it open and throwsSyntaxError). Only the silence changes. The OOM arm (IPC message is too long.) and theStoppedarm (VM shutting down) are unchanged.InvalidFormathas nothing pending, so it closes first.connectedstaystrue, the same as stock bun. Withserialization: "json": 2 echoes. The bytes01 02 00 00read as a big-endian u32 are 16,908,288.fs.writeSync(3)never reach the decoder there (a probe on a Windows build shows the parent sees only EOF), so the raw-byte tests are skipped on Windows with the reason. The tests with a real Node.js peer need no raw write and cover all four messages there:"advanced"through Node's own framing,"json"through a message whosetoJSON()returnsundefined, which Node writes as the lineundefined. The first version of the three files ran once on a Windows x64 debug build before the rebase. Since the rebase only Linux x64 ran locally, and Windows runs in CI.child_process_ipc.test.js,child_process_ipc_large_disconnect.test.js,bun-ipc-inherit.test.ts, 19test-child-process-*and 7test-cluster-*Node tests.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/spawn/spawn.ipc.test.ts, test/js/bun/spawn/spawn.ipc.bun-node.test.ts