node:inspector: make Session.post() without a callback fire-and-forget - #36018
node:inspector: make Session.post() without a callback fire-and-forget#36018robobun wants to merge 3 commits into
Conversation
Node's Session.post() always returns undefined. When no callback is passed the reply is dropped entirely, so a command error (unsupported method, precondition failure) never surfaces as a synchronous throw. Bun was both throwing on command errors and returning the result object synchronously, which breaks APM/profiling tools that send fire-and-forget commands like HeapProfiler.enable on startup. The existing profiler tests that relied on the synchronous return and throw are moved onto the callback form so they keep asserting the same command results and errors.
|
Warning Review limit reached
Next review available in: 10 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 (3)
Comment |
|
Updated 7:04 PM PT - Jul 26th, 2026
✅ @robobun, your commit 3f650dda6bbc33d5a19f1b7a4f6dbb569cd76283 passed in 🧪 To try this PR locally: bunx bun-pr 36018That installs a local version of the PR into your bun-36018 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested Node compat fix.
What was reviewed:
post()still runs#handleMethodsynchronously for its side effects; only the return value / throw is dropped when no callback is supplied. Argument validation andERR_INSPECTOR_NOT_CONNECTEDstill throw synchronously.node:inspector/promisesalways passes a callback viasuper.post, so it's unaffected.- Checked other in-tree
session.post()callers (repl.js,internal/repl/*, node parallel tests) — none rely on the old sync return/throw. - The removed
Nonexistent.domainsync-throw assertion is subsumed by the new fire-and-forget test, which also covers the with-callback error path.
Extended reasoning...
Overview
Removes the no-callback branch of Session.post() in src/js/node/inspector.ts that synchronously threw command errors and returned result objects. Node's contract is that post() always returns undefined and routes replies to the stored callback (or drops them if none). #handleMethod still runs for its side effects; only the return handling changed. The callback branch is byte-identical to before, just refactored from an if/else to an early return.
Test changes: inspector-profiler.test.ts migrates ~20 tests from the old sync-return form to a postAsync() helper (callback → Promise) so they keep asserting the same command results/errors. inspector.test.ts drops one now-invalid sync-throw assertion and adds a new test covering the three fire-and-forget cases (unsupported method, success, failed precondition) plus the with-callback error path.
Security risks
None. Pure Node-compat behavior change to an in-process inspector session; no auth, crypto, network, or untrusted-input parsing is touched.
Level of scrutiny
Low-to-medium. The runtime change is a ~15-line simplification (mostly deletion) in a built-in JS module; the callback path is unchanged. The bulk of the diff is mechanical test migration from session.post(x) → await postAsync(session, x).
Other factors
- Verified
node:inspector/promises(super.post(method, params, cb)) always supplies a callback and is unaffected. - Grepped for other
session.post()call sites:src/js/node/repl.jsandsrc/js/internal/repl/*either pass callbacks or discard the return value;test/js/node/test/parallel/*andtest/js/node/test/fixtures/worker-name.jsall use the callback form or fire-and-forget with no return-value dependency. - The new test would fail on main (
ERR_INSPECTOR_COMMAND -32601) per the PR description, satisfying the fails-without-fix requirement. - No prior human or bot reviews to reconcile.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate-finder: #34719 (and its follow-up #35396) do carry the same |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix.
What was reviewed
post()no-callback branch now returnsundefinedand drops command errors;ERR_INSPECTOR_NOT_CONNECTEDand arg validation still throw synchronously (unchanged, above the edit).- Checked other callers of the sync form:
repl.js(Runtime.enable/disable, return value unused),inspector.promises.ts(always passes a callback), and the node-test-parallel fixtures — none depend on the removed sync return/throw. - Profiler tests migrated to a
postAsynchelper preserve the same assertions;.resolves/.rejectsare all awaited. Fire-and-forgetsession.post("Profiler.enable"/"start")calls in those tests still work because#handleMethodruns synchronously for side effects.
Extended reasoning...
Overview
Removes the no-callback branch in Session.post() (src/js/node/inspector.ts) that synchronously threw command errors and returned result objects. Now post() without a callback runs #handleMethod for its side effects and returns undefined, matching Node's fire-and-forget contract. Test updates: a new regression test in inspector.test.ts covering unsupported-method / success / precondition-failure / callback-still-delivers-error, plus a mechanical migration of ~20 tests in inspector-profiler.test.ts from the sync return/throw form onto a postAsync callback wrapper.
Security risks
None. This is a JS-only change to node:inspector's in-process Session dispatch. No auth, crypto, network, or untrusted-input parsing is touched.
Level of scrutiny
Low-to-moderate. The runtime change is a ~15-line simplification that deletes a branch and de-indents the callback path unchanged. The test churn is larger but purely mechanical (sync → callback form). I traced every other consumer of Session.post in the repo — src/js/node/repl.js, src/js/node/inspector.promises.ts, and the four node-test-parallel/fixture files — and none rely on the removed sync semantics; the fire-and-forget calls in the profiler test file still work because #handleMethod executes synchronously before post() returns.
Other factors
- The comment-cop bot's feedback was addressed (long comment trimmed to one line in 3f650dd).
- The duplicate-finder flagged #34719/#35396; the author noted this is the standalone fix extractable from those larger overhauls. That's a landing-order decision for maintainers, not a correctness concern.
- Verified the removed
expect(() => session.post("Nonexistent.domain")).toThrow(...)assertion (which asserted the old, Node-incompatible behavior) is replaced by an equivalent callback-form assertion in the new test. - All
.resolves/.rejectsin the migrated tests are awaited (no vacuous assertions).
|
Diff is ready. Locally both ASAN debug and release builds pass all 68 tests across The internal gate's release build step keeps failing on infrastructure (first a |
2 participants
Repro
post("HeapProfiler.enable")returned: undefinedBUG: sync throw -> ERR_INSPECTOR_COMMAND -32601: 'HeapProfiler.enable' wasn't foundreturned: undefinedpost("Profiler.enable")undefined{}undefinedCause
Session.post()insrc/js/node/inspector.tshad a no-callback branch that synchronously threw whatever#handleMethodreturned as an error and returned the result object on success. Node never does either:post()dispatches the message, stores the callback (if any) keyed on the message id, and returnsundefined. The reply is routed to the stored callback or dropped if none was registered. Only argument validation andERR_INSPECTOR_NOT_CONNECTEDthrow synchronously.This means an APM/profiling tool that does
session.post("HeapProfiler.enable")fire-and-forget (a no-op in Node) crashes the app in Bun with an uncaught error.Fix
Drop the no-callback branch.
#handleMethodstill runs synchronously for its side effects (enabling the profiler, installing console hooks, etc.), but its return value is discarded when no callback was supplied andpost()returnsundefined.The profiler tests that relied on the synchronous return value or synchronous throw are moved onto the callback form so they keep asserting the same command results and errors.
Verification
New test
Session.post() without a callback is fire-and-forgetfails on main (ERR_INSPECTOR_COMMAND -32601) and passes with this change.