child_process: publish diagnostics_channel events for node:child_process spawn/fork/exec/execFile - #30080
child_process: publish diagnostics_channel events for node:child_process spawn/fork/exec/execFile#30080robobun wants to merge 1 commit into
Conversation
WalkthroughAdds Changeschild_process diagnostics_channel instrumentation
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 4:48 PM PT - Jun 17th, 2026
❌ @robobun, your commit 308c1f7 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30080That installs a local version of the PR into your bun-30080 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs and the implementation follows the existing dgram.ts lazy-channel pattern with hasSubscribers gating, but since this inserts new logic (including a require on first call) directly into the ChildProcess.prototype.spawn hot path, it's worth a quick human sanity check before merging.
Extended reasoning...
Overview
This PR wires node:child_process into diagnostics_channel for Node.js compatibility (issue #30067), enabling ecosystem instrumentation like Sentry's childProcessIntegration and OpenTelemetry. It touches:
src/js/node/child_process.ts(~40 production lines): two lazy channel getters at module scope, plus publish calls insideChildProcess.prototype.spawnfor the plain'child_process'channel and the'child_process.spawn'tracing channel (start/end/error).test/js/node/child_process/child_process.test.ts: seven new tests coveringspawn/fork/exec/execFile, the negativespawnSynccase, and the tracing success/ENOENT paths.test/js/node/test/parallel/test-diagnostics-channel-child-process.js: the upstream Node test added verbatim, including the EACCES branch (skipped on Windows).
Security risks
None identified. The change only publishes references to the already-user-visible ChildProcess instance, the normalized options object, and caught spawn errors to opt-in subscribers. No new inputs are parsed and no privilege boundaries are crossed.
Level of scrutiny
Moderate. The diff is small, self-contained, and mirrors the existing udp.socket pattern in src/js/node/dgram.ts (lazy require of diagnostics_channel, hasSubscribers gate before allocating the payload). However, the publish calls and the unconditional getChildProcessChannel() / getChildProcessSpawnTracingChannel() invocations sit directly in ChildProcess.prototype.spawn, which executes for every async subprocess (spawn, fork, exec, execFile). A regression here would be very visible, so a human glance at the hot-path placement and the error-branch ordering (publishing error both on the handled ENOENT/EACCES/EAGAIN/EMFILE/ENFILE path and on the rethrow path) seems prudent.
Other factors
- The bug-hunting system found no issues.
- Test coverage is solid: both Bun-native tests and the upstream Node parallel test are included, and the PR description shows the gate proof (6/7 fail without the fix).
- CI was still building at review time, so green CI hasn't been confirmed yet.
- No CODEOWNERS entries cover these paths.
27fa3fd to
cd20771
Compare
There was a problem hiding this comment.
LGTM — focused Node.js compat addition with hasSubscribers gating and good test coverage.
Extended reasoning...
Overview
This PR adds diagnostics_channel event publishing to node:child_process, matching Node.js semantics so ecosystem instrumentation (Sentry, OpenTelemetry) can observe subprocess lifecycle. It touches three files: ~40 lines added to src/js/node/child_process.ts (lazy channel init + four publish sites inside ChildProcess.prototype.spawn), seven new tests in child_process.test.ts, and a verbatim port of Node's upstream test-diagnostics-channel-child-process.js.
Security risks
None. The change only emits observability events containing the ChildProcess instance and the already-normalized internal options object — data the caller already has access to. No new inputs are parsed, no command construction is altered, and the spawn path itself is unchanged when no subscribers exist.
Level of scrutiny
Low-to-moderate. While child_process is a core module, the change is purely additive and defensive: every publish is gated on hasSubscribers, so the no-subscriber path has effectively zero overhead and no behavioral change. The lazy-init pattern is copied directly from src/js/node/dgram.ts:193. I verified that Bun's Channel.prototype.publish (diagnostics_channel.ts:141) wraps subscriber callbacks in try/catch and defers errors to process.nextTick(reportError), so a throwing subscriber cannot disrupt the spawn flow. The error-path coverage is correct: both the handled-error-code branch (ENOENT/EACCES/EAGAIN/EMFILE/ENFILE) and the rethrow branch publish to .error before continuing.
Other factors
No CODEOWNERS apply to these paths. No outstanding human review comments. Test coverage is thorough: spawn/fork/exec/execFile each verified, spawnSync verified to not publish (Node parity), tracing start/end on success and start/error on ENOENT, plus the upstream Node parallel test covering EACCES on non-Windows. The bug-hunting system found no issues. The one minor semantic divergence from Node — publishing distinct object literals to start/end/error rather than threading a single mutable context via traceSync — is inconsequential for the stated ecosystem use cases and is what the upstream test actually validates.
|
can we please take a look at this again some time soon? This is a really big issue for sentry users who use bun with child processes. |
|
@Jarred-Sumner Can someone please get on this some time soon? It would be really really appreciated to have this "fixed"/added. |
…ics_channel events
Matches Node semantics: async spawn/fork/exec/execFile publish {process} on
the "child_process" channel and {process, options} / {process} / {process, error}
on the "child_process.spawn" tracingChannel. spawnSync does not publish.
Ecosystem instrumentation (Sentry childProcessIntegration, OpenTelemetry auto-
instrumentations) can now observe subprocess lifecycle under Bun.
Fixes #30067
cd20771 to
308c1f7
Compare
|
Rebased onto current main to resolve the merge conflict (the test file now keeps both the new fd-stdio tests from #32067 and the diagnostics_channel tests; the src change applied cleanly). No logic changes from the previously reviewed version. Re-verified locally:
PR is now mergeable; CI is re-running on the rebased commit. Also closes #32472 (duplicate report). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/js/node/child_process.ts (1)
1463-1497: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider deduplicating the error publication.
The
cpSpawn.error.publish({ process: this, error: ex })call appears in both branches of the catch block (lines 1488–1490 and 1492–1494). You could lift it before theifcondition to avoid duplication:♻️ Proposed refactor
} catch (ex) { + if (cpSpawn.error.hasSubscribers) { + cpSpawn.error.publish({ process: this, error: ex }); + } if ( ex != null && typeof ex === "object" && Object.hasOwn(ex, "code") && (ex.code === "EACCES" || ex.code === "EAGAIN" || ex.code === "EMFILE" || ex.code === "ENFILE" || ex.code === "ENOENT") ) { this.#handle = null; ex.syscall = "spawn " + this.spawnfile; ex.spawnargs = Array.prototype.slice.$call(this.spawnargs, 1); process.nextTick(() => { this.emit("error", ex); this.emit("close", (ex as SystemError).errno ?? -1); }); if (ex.code === "EMFILE" || ex.code === "ENFILE") { this.#stdioOptions[0] = "undefined"; this.#stdioOptions[1] = "undefined"; this.#stdioOptions[2] = "undefined"; } - if (cpSpawn.error.hasSubscribers) { - cpSpawn.error.publish({ process: this, error: ex }); - } } else { - if (cpSpawn.error.hasSubscribers) { - cpSpawn.error.publish({ process: this, error: ex }); - } throw ex; } }🤖 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/js/node/child_process.ts` around lines 1463 - 1497, The cpSpawn.error.publish() call with its hasSubscribers check is duplicated in both the if and else branches of the error handling logic. Move the entire if (cpSpawn.error.hasSubscribers) block that publishes the error outside and after the conditional statement that checks for specific error codes (EACCES, EAGAIN, EMFILE, ENFILE, ENOENT), so that the error is published regardless of which branch executes. This should be positioned after the outer if/else block but before the final throw statement in the else branch.
🤖 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/js/node/child_process.ts`:
- Around line 1463-1497: The cpSpawn.error.publish() call with its
hasSubscribers check is duplicated in both the if and else branches of the error
handling logic. Move the entire if (cpSpawn.error.hasSubscribers) block that
publishes the error outside and after the conditional statement that checks for
specific error codes (EACCES, EAGAIN, EMFILE, ENFILE, ENOENT), so that the error
is published regardless of which branch executes. This should be positioned
after the outer if/else block but before the final throw statement in the else
branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fcf99b3-c006-474d-bab8-d319163b33ff
📒 Files selected for processing (3)
src/js/node/child_process.tstest/js/node/child_process/child_process.test.tstest/js/node/test/parallel/test-diagnostics-channel-child-process.js
There was a problem hiding this comment.
LGTM — small additive Node-compat change that mirrors the existing dgram.ts diagnostics_channel pattern, fully gated by hasSubscribers, with both Bun tests and the vendored upstream Node test.
Extended reasoning...
Overview
This PR wires node:child_process into diagnostics_channel, publishing to channel('child_process') and tracingChannel('child_process.spawn') (start/end/error) from inside ChildProcess.prototype.spawn(). The implementation in src/js/node/child_process.ts adds two lazy channel getters and four publish sites (~50 lines). The remaining ~230 lines are tests: seven new cases in child_process.test.ts covering spawn/fork/exec/execFile/spawnSync and the tracing success/ENOENT paths, plus the upstream test-diagnostics-channel-child-process.js vendored verbatim.
Security risks
None. The change only emits observability events to in-process subscribers; no new external I/O, no privilege changes, no new user-controlled input parsing. The options object exposed on the start event is the already-normalized internal spawn options — the same data the caller supplied — and is only handed to subscribers the process itself registered.
Level of scrutiny
Low-to-moderate. child_process is a hot path, but every publish is guarded by hasSubscribers, so the unobserved case adds only a cached property read per spawn — no allocation, no require. The lazy-init + hasSubscribers gate is copied directly from the existing udp.socket channel in src/js/node/dgram.ts, so there is established precedent in this codebase. The tracing channel is published manually to the .start/.end/.error sub-channels rather than via traceSync; the payload shapes match what the upstream Node test asserts, and that test passes here.
Other factors
No CODEOWNERS cover these files. There are no outstanding review comments — the only timeline activity is bot noise and two community pings asking for the PR to land. The bug-hunting pass found nothing. The change is purely additive (no existing behavior modified outside the new publish calls), self-contained, and well-tested, so I'm comfortable approving without human review.
|
Closing in favor of #32628. It publishes the |
2 participants
Fixes #30067.
Fixes #32472.
Summary
Publishes events on the
diagnostics_channelmodule whennode:child_processcreates a subprocess, matching Node.js semantics. Ecosystem instrumentation (Sentry childProcessIntegration, OpenTelemetry auto-instrumentations) can now observe subprocess lifecycle under Bun.Channels published
Matches Node's
lib/internal/child_process.js:channel('child_process'){ process }tracingChannel('child_process.spawn').start{ process, options }Bun.spawn()tracingChannel('child_process.spawn').end{ process }tracingChannel('child_process.spawn').error{ process, error }spawnSync/execSync/execFileSyncdo not publish — matches Node, which also skips sync variants.Implementation
src/js/node/child_process.ts:getChildProcessChannel,getChildProcessSpawnTracingChannel) so we don't loaddiagnostics_channeluntil the first spawn. Same patterndgram.tsuses forudp.socket.hasSubscribers(andstart.hasSubscribers/end.hasSubscribers/error.hasSubscribersfor the tracing channel) so there's no payload allocation when nobody's listening.child_processfires at the top ofChildProcess.prototype.spawn— Node fires it in theChildProcessconstructor, but Bun's publicnew ChildProcess()takes no args, sospawn(options)is the natural equivalent.startfires beforeBun.spawn();endfires on success;errorfires in the catch, on both the handled node-error-codes path and the rethrow path.Tests
test/js/node/child_process/child_process.test.ts— seven new tests underdescribe("diagnostics_channel '…'"):spawn(),fork(),exec(),execFile()each publish one{ process }to'child_process'spawnSync()publishes nothing (parity with Node)start+endon success,start+error(witherror.code === 'ENOENT') on missing binarytest/js/node/test/parallel/test-diagnostics-channel-child-process.js— added the upstream Node test verbatim, exercising both success and ENOENT/EACCES paths.Verification
With the fix (PASS):
Without the fix (FAIL — gate proof):
(the lone pass is
does not publish for spawnSync, which is trivially satisfied when nothing publishes anywhere)