Skip to content

Start the inspector at runtime on SIGUSR1 / process._debugProcess - #37336

Open
robobun wants to merge 9 commits into
mainfrom
farm/c42486d7/runtime-inspector
Open

Start the inspector at runtime on SIGUSR1 / process._debugProcess#37336
robobun wants to merge 9 commits into
mainfrom
farm/c42486d7/runtime-inspector

Conversation

@robobun

@robobun robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Sending SIGUSR1 to a running bun process (or calling process._debugProcess(pid), which also works on Windows) starts the inspector, matching Node.js. This works even when the JS thread is stuck in while (true) {} and never returns to the event loop.

Clean replacement for #34106 (and #26867 before it): one commit on current main, with every item deferred during the #34106 review folded in. Requires oven-sh/WebKit#287; WEBKIT_VERSION points at its preview build until it lands and gets repointed at the merged sha before this merges.

How it works

SIGUSR1 handler          sem_post, nothing else (async-signal-safe)
SignalInspector thread   sets a flag, VM::notifyNeedDebuggerBreak() on the main VM,
                         wakes the event loop
JSC                      services the trap at the next safe point in every tier:
                         LLInt/Baseline poll at loop back-edges, DFG/FTL get their
                         invalidation points patched by SignalSender (retried every
                         1ms), then calls the per-VM callback from WebKit#287
callback (JS thread)     starts the inspector if requested, drains queued CDP
                         messages, and calls Debugger::breakProgram() only if a
                         Debugger.pause was dispatched during the drain

After activation, the debugger thread fires the same trap whenever it queues a message for the target, so CDP keeps flowing into a busy loop and the debugger thread never blocks on the target. An idle target is handled by the event-loop wakeup instead (one relaxed load per tick on the hot path). No StopTheWorld, no polling traps, no bootstrap-pause state machine.

breakProgram() is needed because code that was running before activation was compiled without op_debug sites; the WebKit side adds the callback hook plus Debugger::isPauseAtNextOpportunitySet() so an in-flight step-over is not mistaken for a pause request.

Behaviour

  • kill -USR1 <pid> / process._debugProcess(pid) start the inspector and print the usual banner (default port 6499).
  • --inspect-port=[host:]port pre-selects where it listens (same forms as --inspect, 0 for a free port); --disable-sigusr1 leaves the signal untouched. Both also apply to bun build --compile binaries.
  • A user process.on("SIGUSR1") listener takes the signal over; removing the last listener restores whatever the process started with (the activation handler, or SIG_IGN under --inspect*), tracked as runtime_inspector::Sigusr1.
  • --inspect* processes ignore SIGUSR1. Platforms where JSC's GC owns SIGUSR1 (FreeBSD) are left untouched.
  • _debugProcess errors: ERR_MISSING_ARGS, ERR_INVALID_ARG_TYPE / ERR_INVALID_ARG_VALUE for anything that is not a positive int32 (same pid != (pid | 0) check as process.kill), a system error with .code/.syscall on POSIX, and Node's Win32 message text on Windows (vendored test-debug-process.js passes).
  • inspector.open() (landed on main since July) and this path now share Debugger::start_at_runtime, so both install Bun's inspector controller.

Verification

bun bd test test/js/bun/runtime-inspector/

Covers: idle and while(true) activation, Debugger.pause interrupting while(true) (12/12 runs on the debug ASAN build, so it is no longer skipped on sanitizer lanes), no double activation (acknowledged via a CDP round trip rather than a sleep), reconnect, self-signal, user listener precedence, hand-back to the activation handler and back to SIG_IGN under --inspect, --inspect* ignoring the signal, --disable-sigusr1, and the argument/error paths. All fail on a build without this change (_debugProcess is a stub there). test/js/node/inspector/inspector.test.ts still passes on the shared start_at_runtime path, and bun run rust:check-all is clean on all ten targets.


no test proof · iteration 14 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts test/js/bun/runtime-inspector/runtime-inspector.test.ts test/js/node/process/process.test.js

Sending SIGUSR1 to a running bun process (or calling process._debugProcess(pid),
which also works on Windows) starts the inspector, matching Node.js. This works
even when the JS thread is stuck in a loop that never returns to the event loop.

Mechanism:

  SIGUSR1 handler         sem_post only (async-signal-safe)
  SignalInspector thread  sets a flag, fires VM::notifyNeedDebuggerBreak on the
                          main VM, wakes the event loop
  JSC                     services the trap at the next safe point in any JIT
                          tier (SignalSender patches DFG/FTL invalidation
                          points) and calls the per-VM callback added in
                          oven-sh/WebKit#287
  callback (JS thread)    starts the inspector if requested, drains queued CDP
                          messages, and enters Debugger::breakProgram() when a
                          Debugger.pause was dispatched

CDP delivery after activation reuses the same trap, so the debugger thread
never blocks on the target. An idle target is handled by the event-loop wakeup
instead. Platforms where JSC's GC owns SIGUSR1 (FreeBSD) leave the signal alone.

Also: --inspect-port and --disable-sigusr1 flags; a user SIGUSR1 listener takes
over the signal and hands it back when removed; inspector.open() and this path
now share Debugger::start_at_runtime.

Replaces #26867 and #34106. Requires oven-sh/WebKit#287.
To be replaced with the main autobuild once #287 lands.
Comment thread src/jsc/Debugger.rs Outdated
Comment thread src/jsc/Debugger.rs Outdated
Comment thread src/jsc/Debugger.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/jsc/VM.rs Outdated
Comment thread src/jsc/bindings/BunDebugger.cpp Outdated
Comment thread src/jsc/bindings/BunDebugger.cpp Outdated
Comment thread src/jsc/bindings/BunDebugger.cpp Outdated
Comment thread src/jsc/bindings/BunDebugger.cpp Outdated
Comment thread src/jsc/bindings/BunProcess.cpp Outdated
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 314100c9-0500-410e-9c94-1518bd1cd031

📥 Commits

Reviewing files that changed from the base of the PR and between 388ab9e and 192eb9e.

📒 Files selected for processing (4)
  • src/jsc/bindings/BunProcess.cpp
  • test/js/bun/runtime-inspector/helpers.ts
  • test/js/bun/runtime-inspector/runtime-inspector.test.ts
  • test/js/node/process/process.test.js

Walkthrough

The PR adds runtime inspector activation through SIGUSR1 and process._debugProcess, configurable ports, platform-specific activation paths, debugger-break integration, CLI propagation, tests, and documentation. It also updates the default WebKit build identifier.

Changes

Runtime inspector

Layer / File(s) Summary
Runtime inspector configuration
src/options_types/context.rs, src/runtime/cli/*, src/jsc/VirtualMachine.rs, src/runtime/jsc_hooks.rs, src/jsc/lib.rs
Adds --inspect-port and --disable-sigusr1, forwards both settings into VM initialization, and configures main-thread signal handling.
Cross-platform activation and debugger startup
src/jsc/RuntimeInspector.rs, src/jsc/VM.rs, src/jsc/event_loop.rs, src/jsc/Debugger.rs, src/jsc/bindings/vm/Semaphore.cpp
Adds POSIX signal and semaphore activation, Windows remote-thread activation, VM debugger-break notification, event-loop activation, and shared runtime debugger startup.
Debugger trap and process activation bridge
src/jsc/bindings/BunDebugger.cpp, src/jsc/bindings/BunProcess.cpp
Processes runtime activation at debugger traps, manages inspector connections and pause loops, restores signal handling, and implements process._debugProcess(pid).
Inspector validation and documentation
test/js/bun/runtime-inspector/*, test/js/node/process/process.test.js, docs/runtime/debugger.mdx, docs/snippets/cli/run.mdx
Adds helper utilities and tests for signals, process debugging, CDP behavior, error handling, and disabled signal handling. Documents the activation methods and CLI options.

WebKit preview dependency

Layer / File(s) Summary
WebKit build identifier
scripts/build/deps/webkit.ts
Changes the default WEBKIT_VERSION to autobuild-preview-pr-287-d55f967e.

Possibly related PRs

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes runtime inspector activation through SIGUSR1 and process._debugProcess.
Description check ✅ Passed The description explains the implementation, behavior, dependencies, and verification steps in detail, covering both required template topics.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

Comment thread test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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 `@scripts/build/deps/webkit.ts`:
- Line 6: Keep WEBKIT_VERSION pinned to the PR `#287` preview while that PR
remains open; once it merges or closes, replace the preview identifier with the
resulting merged WebKit commit SHA so prebuilt downloads continue using an
available release.

In `@src/jsc/bindings/BunProcess.cpp`:
- Around line 4680-4685: Update the _debugProcess argument handling around pid
to validate the original numeric value is within the signed int32 range before
calling toInt32. Reject values outside that range, including values that would
wrap during conversion, while preserving the existing positive-integer
validation and exception handling.

In `@src/jsc/RuntimeInspector.rs`:
- Around line 333-341: The Windows debug-handler mapping name is duplicated
across Rust and C++; define the format once near the Rust mapping-name
construction and expose it through an extern "C" accessor or shared constant,
then update BunProcess.cpp at lines 4716-4717 to use that shared definition
instead of the hardcoded L"bun-debug-handler-%d" literal. Ensure both producer
and consumer generate the identical per-process mapping name.

In `@src/runtime/jsc_hooks.rs`:
- Around line 579-581: Validate the inspect-port option during CLI parsing in
Arguments.rs before storing it as raw bytes, requiring a numeric value within
the supported port range. Reject invalid values with a parse error that includes
the original rejected value, while preserving valid values for the
runtime_inspector activation path.

In `@test/js/bun/runtime-inspector/helpers.ts`:
- Around line 130-149: Update cdpClient to track both promise resolution and
rejection for each pending request, and reject when an incoming CDP response
contains msg.error instead of resolving it. Preserve normal response resolution
and event handling, while ensuring protocol error details propagate to callers
through the returned send promise.
🪄 Autofix

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: 58c1279d-affb-45b0-9d2d-3eadb618af69

📥 Commits

Reviewing files that changed from the base of the PR and between 827475e and f366e4e.

📒 Files selected for processing (22)
  • docs/runtime/debugger.mdx
  • docs/snippets/cli/run.mdx
  • scripts/build/deps/webkit.ts
  • src/jsc/Debugger.rs
  • src/jsc/RuntimeInspector.rs
  • src/jsc/VM.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/BunDebugger.cpp
  • src/jsc/bindings/BunProcess.cpp
  • src/jsc/bindings/vm/Semaphore.cpp
  • src/jsc/event_loop.rs
  • src/jsc/lib.rs
  • src/options_types/context.rs
  • src/runtime/cli/Arguments.rs
  • src/runtime/cli/repl_command.rs
  • src/runtime/cli/run_command.rs
  • src/runtime/cli/test_command.rs
  • src/runtime/jsc_hooks.rs
  • test/js/bun/runtime-inspector/helpers.ts
  • test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts
  • test/js/bun/runtime-inspector/runtime-inspector.test.ts
  • test/js/node/process/process.test.js
💤 Files with no reviewable changes (1)
  • test/js/node/process/process.test.js

Comment thread scripts/build/deps/webkit.ts
Comment thread src/jsc/bindings/BunProcess.cpp Outdated
Comment thread src/jsc/RuntimeInspector.rs Outdated
Comment thread src/runtime/jsc_hooks.rs Outdated
Comment thread test/js/bun/runtime-inspector/helpers.ts
Comment thread src/jsc/bindings/BunProcess.cpp Outdated
- Record the SIGUSR1 disposition chosen at startup (runtime_inspector::Sigusr1)
  and re-apply that, rather than only the activation handler, when the last
  user SIGUSR1 listener is removed. Under --inspect* the signal now stays
  ignored after a listener add/remove cycle instead of falling back to the
  default action. --disable-sigusr1 leaves the inherited disposition alone,
  as before this change.
- process._debugProcess validates the pid the same way process.kill does, so
  values toInt32 would wrap (2 ** 32 + 1 -> 1) and fractions are rejected
  instead of signalling an unrelated process.
- Test helper cdpClient rejects on CDP error responses; the user-listener test
  accumulates stdout across signals; the Debugger.pause test runs on ASAN
  builds too (passes 12/12 locally on the debug ASAN build).
- --inspect-port documents the [host:]port form it shares with --inspect.
- Shorter comments throughout.
Comment thread src/jsc/RuntimeInspector.rs
Comment thread src/jsc/RuntimeInspector.rs
Comment thread src/runtime/jsc_hooks.rs
Comment thread test/js/bun/runtime-inspector/runtime-inspector.test.ts
Comment thread test/js/bun/runtime-inspector/helpers.ts Outdated
… inspectee

On the ASAN lanes the runner sets BUN_JSC_validateExceptionChecks, and a
process answering Runtime.evaluate (or pausing) aborts inside JSC's
InjectedScript on the unchecked getOwnNonIndexPropertyNames scope in the
prebuilt WebKit, the same gap that keeps test/cli/inspect/inspect.test.ts in
test/no-validate-exceptions.txt. Every CDP round trip in these files timed out
there as a result. Strip the flag for the inspected processes only, as
test/js/node/inspector/inspector.test.ts does; the test process, the
signalling children and the error-path children still run with it.

spawnTarget now kills the child if it fails before handing it to the caller.
Comment thread test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts
Comment thread test/js/bun/runtime-inspector/helpers.ts Outdated
Comment thread test/js/bun/runtime-inspector/helpers.ts Outdated
…orting ready

The two user-listener targets printed their pid before calling process.on, so
a signal sent right after spawnTarget returned could still reach the activation
handler. debugProcess drains stdout as well, and the banner helpers type the
stderr pipe rather than stdout.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/jsc/RuntimeInspector.rs (1)

229-235: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle sigemptyset and sigaction failures.

Line 234 discards the result of sigaction. If installation fails, SIGUSR1 can retain its prior disposition while configure records StartInspector or Ignore. Return failure from platform::apply, retain the previous disposition on failure, and report the syscall error.

🤖 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/RuntimeInspector.rs` around lines 229 - 235, Update the SIGUSR1 setup
in platform::apply to check the return values from sigemptyset and sigaction,
preserving the previous signal disposition when either syscall fails. Propagate
failure from platform::apply and report the corresponding OS error instead of
recording StartInspector or Ignore as successfully applied.

Source: Coding guidelines

src/jsc/Debugger.rs (1)

605-642: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Restore debugger startup state after thread-spawn failure.

If Debugger::create fails, Line 628 clears only vm.debugger. Debugger::create has already set HAS_CREATED_DEBUGGER and has_started_debugger. Later SIGUSR1, process._debugProcess, and inspector.open() attempts then fail can_start_at_runtime() permanently.

Roll back these flags in Debugger::create before returning the spawn error. Preserve a retryable state.

🤖 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/Debugger.rs` around lines 605 - 642, Update Debugger::create failure
handling in start_at_runtime so a thread-spawn error rolls back all startup
state, including HAS_CREATED_DEBUGGER and has_started_debugger, before returning
the error. Keep vm.debugger cleared and preserve can_start_at_runtime() so later
SIGUSR1, process._debugProcess, or inspector.open() attempts can retry
successfully.

Source: Coding guidelines

test/js/bun/runtime-inspector/helpers.ts (1)

33-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Cancel the pending stream read on timeout.

When timeout wins Promise.race, reader.read() remains pending. waitForBanner then calls reader.releaseLock() at line 72. That call can throw and replace the useful timeout error.

Cancel and await the pending read path before returning the timeout error.

As per coding guidelines, every timeout path must complete the operation and cancel protocols.

🤖 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 `@test/js/bun/runtime-inspector/helpers.ts` around lines 33 - 42, Update the
waitForBanner read loop to track when timeout wins Promise.race, cancel the
pending reader.read() operation, and await its completion before propagating the
timeout error. Ensure reader.releaseLock() runs only after the cancellation
protocol completes, while preserving normal stream completion behavior.

Source: Coding guidelines

🤖 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 `@docs/snippets/cli/run.mdx`:
- Around line 141-143: Update the --inspect-port ParamField description to
document URL-prefix input alongside the existing [host:]port form, including an
example such as localhost:4000/prefix. Preserve the current default-port and
free-port details.

In `@test/js/bun/runtime-inspector/helpers.ts`:
- Around line 126-131: Update the subprocess assertion around Promise.all to
report whether stderr is empty using a strict stderr === "" check, replacing the
weaker stderr.includes("error:") condition while preserving the existing stdout
and exitCode assertions.

In `@test/js/bun/runtime-inspector/runtime-inspector.test.ts`:
- Around line 126-145: Update the test around the Bun.spawn call and invalid-PID
output assertion to pipe stderr, await proc.exited, and assert the child’s
stderr is empty. After validating stdout, assert the strongest completion
invariant by requiring proc.exitCode === 0, preserving the existing output
snapshot.

---

Outside diff comments:
In `@src/jsc/Debugger.rs`:
- Around line 605-642: Update Debugger::create failure handling in
start_at_runtime so a thread-spawn error rolls back all startup state, including
HAS_CREATED_DEBUGGER and has_started_debugger, before returning the error. Keep
vm.debugger cleared and preserve can_start_at_runtime() so later SIGUSR1,
process._debugProcess, or inspector.open() attempts can retry successfully.

In `@src/jsc/RuntimeInspector.rs`:
- Around line 229-235: Update the SIGUSR1 setup in platform::apply to check the
return values from sigemptyset and sigaction, preserving the previous signal
disposition when either syscall fails. Propagate failure from platform::apply
and report the corresponding OS error instead of recording StartInspector or
Ignore as successfully applied.

In `@test/js/bun/runtime-inspector/helpers.ts`:
- Around line 33-42: Update the waitForBanner read loop to track when timeout
wins Promise.race, cancel the pending reader.read() operation, and await its
completion before propagating the timeout error. Ensure reader.releaseLock()
runs only after the cancellation protocol completes, while preserving normal
stream completion behavior.
🪄 Autofix

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: 9ed2b61d-957c-4547-91ff-0619f1dd77d1

📥 Commits

Reviewing files that changed from the base of the PR and between f366e4e and 388ab9e.

📒 Files selected for processing (14)
  • docs/runtime/debugger.mdx
  • docs/snippets/cli/run.mdx
  • src/jsc/Debugger.rs
  • src/jsc/RuntimeInspector.rs
  • src/jsc/VM.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/BunDebugger.cpp
  • src/jsc/bindings/BunProcess.cpp
  • src/options_types/context.rs
  • src/runtime/cli/Arguments.rs
  • src/runtime/jsc_hooks.rs
  • test/js/bun/runtime-inspector/helpers.ts
  • test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts
  • test/js/bun/runtime-inspector/runtime-inspector.test.ts
💤 Files with no reviewable changes (1)
  • src/options_types/context.rs

Comment thread docs/snippets/cli/run.mdx
Comment thread test/js/bun/runtime-inspector/helpers.ts Outdated
Comment thread test/js/bun/runtime-inspector/runtime-inspector.test.ts
runSnippet drains stdout and stderr and returns them with the exit code, so
debugProcess and the argument-validation tests assert on all three instead of
stdout alone.
@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

On the three "outside the diff" review items:

  • sigaction return value in RuntimeInspector.rs: not checking it on purpose. With a fixed valid signal number and a valid struct the call cannot fail (EINVAL is only for bad signal numbers / SIGKILL / SIGSTOP), and the existing signal installs in BunProcess.cpp treat it the same way.
  • Rolling back HAS_CREATED_DEBUGGER when the debugger thread fails to spawn: that is Debugger::create, which is main's inspector.open() code and behaves identically there today. After a failed spawn the runtime path prints failed to start the inspector and later signals are ignored, which is the safe outcome under resource exhaustion. Making it retryable is a separate change to the existing code, not something this PR alters.
  • readStreamUntil timeout leaving a read() pending: in Bun (checked against both the current release and this build) releaseLock() does not throw with a pending read, it rejects that read instead, and Promise.race has already attached a handler to it, so the timeout error is what surfaces. Nothing to change.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this revision and the bug hunting system found no new issues; all prior inline findings from earlier runs are addressed (30ef98e, 388ab9e, 9a5d8dd). Given the scope — a new cross-thread trap/signal path into JSC, Windows CreateRemoteThread plumbing, and a dependency on an unmerged WebKit change (WEBKIT_VERSION still points at the PR-287 preview build) — a human review is still warranted before this lands.

What was reviewed:

  • Async-signal-safety of the SIGUSR1 handler (only sem_post) and Semaphore::wait EINTR/KERN_ABORTED retry loops.
  • onDebuggerTrap / runWhilePaused re-entrancy: inPauseLoop gating, snapshot-under-lock before doDisconnect mutates the map, and the anyPaused guard against re-entering breakProgram.
  • _debugProcess argument validation (matches process.kill's pid | 0 check) and Windows handle cleanup on each error path.
  • SIGUSR1 hand-back ordering in onDidChangeListeners and the gc_owns_sigusr1 FreeBSD carve-out.
Extended reasoning...

Overview

This PR wires runtime inspector activation via SIGUSR1 / process._debugProcess(pid), matching Node.js. It adds a new src/jsc/RuntimeInspector.rs module (POSIX semaphore + dedicated thread; Windows named file mapping + CreateRemoteThread), a per-VM NeedDebuggerBreak trap callback in BunDebugger.cpp that connects/drains/pauses from inside a busy JS loop, a real Process_functionDebugProcess in BunProcess.cpp, two new CLI flags (--inspect-port, --disable-sigusr1) plumbed through four command entry points, EINTR-safe Bun::Semaphore::wait, docs, and a full test suite. It also refactors the existing inspector.open() path onto a shared start_at_runtime helper.

Security risks

The Windows _debugProcess path opens the target with PROCESS_CREATE_THREAD | PROCESS_VM_* and calls CreateRemoteThread at a function pointer read from a named mapping. This is Node's own protocol and is gated by OS process ACLs, but it is still remote-thread injection into another process and merits a maintainer glance. The POSIX side installs a process-wide SIGUSR1 handler at VM init unless --disable-sigusr1 is passed; the handler is correctly async-signal-safe (single sem_post), and the FreeBSD/JSC-GC-owns-SIGUSR1 case is checked. _debugProcess argument validation rejects non-positive-int32 pids before any syscall.

Level of scrutiny

High. This is ~650 lines of new native code coordinating a signal handler, a dedicated thread, JSC VM traps, and the debugger connection state machine across three threads (signal → SignalInspector → JS), on all three platforms. BunDebugger.cpp gains a new re-entrancy dimension (onDebuggerTrap running inside arbitrary JS via VMTraps, guarded by inPauseLoop). The change also hooks every main-thread event-loop tick (check_and_activate_inspector). None of this is mechanical.

Other factors

  • WEBKIT_VERSION is pointed at autobuild-preview-pr-287-d55f967e, an unmerged WebKit PR. The description says it will be repointed before merge; a human should confirm that has happened.
  • This is the third iteration of the feature (replaces #34106 and #26867), so the design has already had review cycles, but the implementation is fresh on current main.
  • Test coverage is thorough (idle/busy activation, pause-in-while(true), reconnect, listener precedence/hand-back, --inspect* interaction, --disable-sigusr1, error paths on both platforms) and every prior review comment on this PR is resolved.
  • The Semaphore::wait change also affects existing callers of Bun::Semaphore (now retries on EINTR/KERN_ABORTED instead of returning false), which is almost certainly a fix but is a behaviour change outside this feature.

@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

On the Semaphore::wait() note above: the only other caller is SigintWatcher, whose thread does ASSERT(success) on the result and otherwise treats a return as "SIGINT arrived". Before this change an interrupted sem_wait / KERN_ABORTED made wait() return false, which in a release build would have been dispatched as a SIGINT; retrying on EINTR is what that caller wanted as well. uninstall() still wakes the thread through a real signal(), so its shutdown path is unaffected.

@robobun

robobun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 6:39 PM PT - Aug 10th, 2026

@robobun, your commit 192eb9e has 2 failures in Build #91781 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37336

That installs a local version of the PR into your bun-37336 executable, so you can run:

bun-37336 --bun

@alii

alii commented Aug 11, 2026

Copy link
Copy Markdown
Member

Went through oven-sh/WebKit#287 against this branch (192eb9e) and requested changes there: oven-sh/WebKit#287 (review). The JSC-side problems are on that PR; this is the list of things that land on this side, either because the fix belongs here or because a test here is the only thing that would pin the WebKit hunk. All from reading, none of it run.

  1. onDebuggerTrap uses debugger->isPauseAtNextOpportunitySet() to mean "a Debugger.pause was dispatched during the drain" (BunDebugger.cpp:1207-1209). That bit is also set by stepIntoStatement(), step-over off the end of a program, the blackbox defer path, didAwait, and breakProgram itself, so after a step-into from a trap pause the next CDP message re-enters breakProgram() and re-pauses somewhere arbitrary, reported as the finished step. The flag that means what the comment says is InspectorDebuggerAgent::pauseOnNextStatementEnabled(), and debug-helpers.h already has JSC::debuggerAgent(globalObject) to reach it, so this can be auto* agent = JSC::debuggerAgent(globalObject); agent && agent->pauseOnNextStatementEnabled() with no WebKit change. I've asked for the Debugger.h accessor to be dropped.

  2. The pre-attach in doConnect (BunDebugger.cpp:158-163) is what the idempotent Debugger::attach() hunk exists for, and I don't think it's needed: onDebuggerTrap drains every connection before it reads globalObject->debugger(), and Debugger.enable attaches synchronously on that thread during the drain. What it does add is an attach that nothing ever detaches (Runtime-only sessions keep the microtask fast path off and ShadowChicken on for the rest of the process) and it turns Debugger.pause-without-enable from ignored into a silent hang. Delete it, and the WebKit PR can put upstream attach() back verbatim. Worth one run of the runtime-inspector tests against an asserts-on WebKit after to make sure nothing double-attaches.

  3. Idle activation never retires the trap bit. check_and_activate_inspector keys off ACTIVATION_REQUESTED and never touches vm.traps(); handleTraps is only reached from bytecode poll sites and C++ VM entry masks NeedDebuggerBreak out. So after kill -USR1 on a mostly idle process (and after every post-activation connect/disconnect, which re-fire it) SignalSender keeps suspending/resuming the main thread every 1ms until some JS happens to hit a loop hint. The idle fixture in the tests ticks a 1s timer, which hides it. Fix: have the idle path call vm.traps().handleTrapsIfNeeded(JSC::VMTraps::NeedDebuggerBreak) instead of re-checking the flag, so idle and busy activation go through the same callback and the bit gets cleared.

  4. onDebuggerTrap can recurse into itself. The debugger thread re-fires the trap per queued message (BunDebugger.cpp:537), the bit is re-armed while the callback is still running, and any JS the dispatch runs (injected script, Runtime.evaluate of user code with a loop or a call) polls it and re-enters onDebuggerTrap mid-dispatch. I've asked for JSC to coalesce that in handleTraps, but until it does a re-entrancy guard here is cheap. Same site also has no DeferTermination and no exception post-condition, so a process.exit()/worker terminate racing the drain gets swallowed and a stray exception resurfaces at the next op_check_traps. The tests strip BUN_JSC_validateExceptionChecks from the inspectee; with it on, two callbacks in one handleTraps loop should assert.

  5. Windows. !ENABLE(SIGNAL_BASED_VM_TRAPS) there, so usePollingTraps is forced, invalidateCodeBlocksOnStack compiles to nothing, and DFG emits CheckTraps (modelled as side-effect free) instead of InvalidationPoint. The callback then runs CDP dispatch or a whole nested pause loop inside a live DFG/FTL frame and returns into it. _debugProcess on a busy optimized loop on Windows is not safe until the WebKit side changes. The BUSY_LOOP tests pass there because an empty while (true) {} has nothing hoisted to get wrong; a loop that reads array elements or object properties and then has Runtime.evaluate mutate them from the pause is the case that would show it.

  6. LLInt function prologue and wasm IPInt prologue both service the trap on a frame that isn't built yet (JS: before op_enter, locals not zeroed; wasm: stale topCallFrame). breakProgram() from there plus a scopeChain read is a wild pointer once the block has debug opcodes. JSC-side fix, but the test that would catch it lives here: pause inside nested function calls (not top-level program code) with captured locals, under BUN_JSC_useDFGJIT=0 and default tiers, and assert on scopeChain and evaluateOnCallFrame. The current pause test pauses top-level code where the scope register and callee->scope() are both the global scope, so it can't tell the DebuggerCallFrame.cpp change from a revert.

  7. Stepping from a trap-initiated pause doesn't work and the docs don't say so. The paused frame has no op_debug, so step-over/out arm m_pauseOnCallFrame on a frame that will never report a return event: Step behaves as Continue, stepping mode stays enabled (blocks DFG tier-up for the realm), and if the frontend disconnects in that state the next session inherits it. More generally the hook buys pause + evaluate for code that was already running, not breakpoints, debugger; or stepping, until that code is re-entered after an idle tick, and in the while (true) {} case idle never comes so setBreakpointByUrl resolves and never fires. debugger.mdx says "matching Node.js"; it should carry that caveat, and a test should pin "breakpoint on the pre-activation hot frame does not fire until re-entry".

  8. Every trap fire jettisons every DFG/FTL frame on the stack before the callback runs, and this fires per CDP batch, so autocomplete or getProperties against a busy target is a rolling deopt even when nothing pauses. Asked for the WebKit side to let the callback own that decision; nothing to do here except not fire when the queue was already non-empty.

Tests I'd add before this merges, since right now reverting DebuggerCallFrame.cpp:159, the disconnectFrontend reorder, or the attach early-return in WebKit#287 passes everything in both repos: (a) the nested-function scopeChain/evaluateOnCallFrame pause from 6, both tier configs; (b) activate, pause, resume, define and call a new function, pause again; (c) stepInto from a trap pause then an unrelated CDP message, assert no second Debugger.paused; (d) pipelined CDP requests without awaiting each, including a Runtime.evaluate that calls functions, to exercise the re-entrancy in 4; (e) two frontends, disconnect one, the other keeps working; and one full run with BUN_JSC_validateExceptionChecks=1 left on for the inspectee.

Comment on lines +158 to +176
const pending = new Map<number, PromiseWithResolvers<any>>();
ws.onmessage = event => {
const msg = JSON.parse(event.data as string);
if (msg.id === undefined) {
onEvent?.(msg);
return;
}
const request = pending.get(msg.id);
pending.delete(msg.id);
if (msg.error) request?.reject(new Error(`CDP error: ${JSON.stringify(msg.error)}`));
else request?.resolve(msg);
};
return function send(method: string, params: Record<string, unknown> = {}): Promise<any> {
const id = nextId++;
const request = Promise.withResolvers<any>();
pending.set(id, request);
ws.send(JSON.stringify({ id, method, params }));
return withTimeout(`response to ${method}`, request.promise);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 cdpClient sets ws.onmessage but never ws.onclose/ws.onerror, so if the inspected target crashes mid-request the pending promise only fails via withTimeout after 20s with a generic "Timed out waiting for response to " instead of surfacing the close reason — REVIEW.md's "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise". Add ws.onclose/ws.onerror handlers that reject and clear every entry in pending.

Extended reasoning...

What the bug is

cdpClient() at test/js/bun/runtime-inspector/helpers.ts:158-176 builds a request/response layer over the inspector WebSocket:

export function cdpClient(ws: WebSocket, onEvent?: (msg: any) => void) {
  let nextId = 1;
  const pending = new Map<number, PromiseWithResolvers<any>>();
  ws.onmessage = event => { ... };
  return function send(method, params = {}) {
    const id = nextId++;
    const request = Promise.withResolvers<any>();
    pending.set(id, request);
    ws.send(JSON.stringify({ id, method, params }));
    return withTimeout(`response to ${method}`, request.promise);
  };
}

Only ws.onmessage is set. Nothing wires ws.onclose or ws.onerror to the pending map. connectInspector() does set ws.onerror, but that handler's reject is a no-op once ws.onopen has already resolved the connection promise, so it provides no coverage during CDP traffic — and ws.onclose is never set at all.

REVIEW.md's "Tests reviewers reject" section states:

Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise

This is a direct match for that rule.

Step-by-step proof

  1. A test calls send("Runtime.evaluate", { expression: "6 * 7" }). A PromiseWithResolvers is created and stored in pending under id 1; ws.send(...) writes the request; the caller awaits withTimeout("response to Runtime.evaluate", request.promise, 20_000).
  2. The inspected target aborts before responding — e.g. exactly the scenario this PR documents at helpers.ts:77-82 (inspecteeEnv comment): under BUN_JSC_validateExceptionChecks, answering Runtime.evaluate runs JSC's InjectedScript, whose unchecked exception scope aborts the process. Or the target is SIGKILLed, or crashes for any other reason.
  3. The debugger-thread WebSocket server dies with the target; the client-side ws receives a close frame (or the TCP connection resets).
  4. ws.onclose is unset → nothing runs. ws.onerror (from connectInspector) calls reject on an already-settled promise → no-op. ws.onmessage never fires again.
  5. request.promise in pending is never settled. pending still holds the entry.
  6. 20 seconds later, withTimeout's timer fires and rejects with "Timed out after 20000ms waiting for response to Runtime.evaluate".
  7. The test fails with a misleading timeout message instead of "WebSocket closed: ", and 20s of wall-clock is burned per in-flight request.

Why existing code doesn't prevent it

withTimeout(20_000) is the only bound. It is a safety-net fallback, not the failure-event wiring the rule requires — the rule's point is that a close/error is an observable condition that should reject immediately, whereas the timeout only proves "nothing happened for 20s". connectInspector's onerror is spent once the connection resolves. Nothing else touches pending on close.

Impact

Bounded — this is failure-path only. On the happy path nothing changes; withTimeout guarantees the awaited promise settles within 20s so tests do not hang to the test timeout. But on a systematic failure (a broken build where every target aborts on the first CDP round trip), each affected describe.concurrent test burns 20s and reports a generic timeout instead of the actual close reason, which makes the CI failure harder to diagnose. The known trigger (InjectedScript exception-check abort) is already worked around via inspecteeEnv, so this only bites on unexpected target crashes. Hence nit, not blocking.

Fix

Add close/error handlers that flush pending:

const pending = new Map<number, PromiseWithResolvers<any>>();
const fail = (why: string) => {
  for (const [, req] of pending) req.reject(new Error(`CDP connection ${why}`));
  pending.clear();
};
ws.onclose = e => fail(`closed (${e.code} ${e.reason || ""})`);
ws.onerror = e => fail(`errored: ${(e as ErrorEvent).message ?? e}`);
ws.onmessage = event => { ... };

This surfaces the close reason immediately and lets the test fail with a diagnostic message the moment the target dies, instead of 20s later with "Timed out".

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, agreed on all eight. Plan, so you can redirect early if any of it is wrong:

WebKit#287 (same PR, new revision, then the pin here moves to its preview):

  • VMTraps: llint_check_stack_and_vm_traps and the IPInt check_stack_and_vm_traps service NonDebuggerAsyncEvents only, so the debugger bit is only taken at op_check_traps (loop headers) or from the idle loop. One consequence worth knowing: busy code that recurses without looping (fib(45)) now keeps the bit armed until it reaches a loop or returns to the event loop, and while it is armed SignalSender keeps installing and jettisoning. If you would rather keep prologue servicing and rely on the tightened scope() predicate instead, it is a one-line switch; I am going with the mask as asked.
  • handleTraps callback bracket: re-entrancy flag that masks NeedDebuggerBreak while the callback runs (so a re-fire is coalesced by the outer loop), DeferTerminationForAWhile around the call, releaseAssertNoExceptionExceptTermination after it, contract documented on setDebuggerTrapCallback together with the idle-loop requirement and the "pause + evaluate, not breakpoints/stepping, until re-entry" limitation.
  • Polling builds (Windows): invalidateCodeBlocksOnStack un-gated; handleCheckTraps emits CheckTraps followed by an InvalidationPoint for linked plans, and the callback path in polling mode always jettisons the stack before returning, so the frame exits at that point instead of continuing with hoisted state. In signal mode with a callback installed the whole-stack pre-walk is skipped (the frame that was running optimized code has already exited through the trap breakpoints and the rest are parked at calls), which is the per-batch jettison you flagged; upstream behaviour is unchanged when no callback is installed. Making CheckTraps clobber the world would cost every loop on Windows, so I am not doing that.
  • DebuggerCallFrame::scope() and ShadowChicken share one helper: debugging opcodes, valid scope register, bytecode offset past op_enter, read as a JSValue, dynamicDowncast<JSScope>, null falls through. Comment rewritten around the OSR-exit and prologue hazards.
  • isPauseAtNextOpportunitySet() dropped; the attach/early-return hunks reverted to upstream.
  • Under BUN_JSC_ADDITIONS, step next/over/out on a frame without debugging opcodes degrade to m_pauseAtNextOpportunity instead of arming m_pauseOnCallFrame, and detach() resets stepping mode, the immediate/eventual/async pause state and m_currentCallFrame once the last global object goes away.

This side:

  • onDebuggerTrap keys off debuggerAgent(globalObject)->pauseOnNextStatementEnabled(), the doConnect pre-attach goes away, and the idle path becomes traps().handleTrapsIfNeeded(NeedDebuggerBreak) driven by a "trap fired" flag that every firing site sets, so idle and busy activation share the callback and the bit is retired on a quiet process after connect/disconnect/message delivery too. Re-entrancy guard in the callback as well, and the debugger thread only fires when its queue went from empty to non-empty.
  • docs: the limitation paragraph in debugger.mdx.
  • tests: your (a) through (e), the pre-activation breakpoint one from 7, the Windows-shaped one from 5 (optimized loop reading array/object state that a Runtime.evaluate from the pause mutates), plus the attach double-check against the asserts-on debug WebKit that bun bd already links.
  • On the validateExceptionChecks run: today the inspectee dies on the first Runtime.evaluate regardless of this PR (getOwnNonIndexPropertyNames -> JSObjectInlines::get, the gap that keeps test/cli/inspect/inspect.test.ts in no-validate-exceptions.txt), so the run as described cannot get past message one. I will see whether that gap and the two RELEASE_AND_RETURN ones noted in inspector.test.ts are small enough to fold into console.timeLog() does not work #287; if they are, the inspectee keeps the flag on and the strip in helpers.ts goes away, otherwise I will report what the run shows up to that point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants