feat(inspector): runtime activation via SIGUSR1 / process._debugProcess - #34106
Closed
robobun wants to merge 17 commits into
Closed
feat(inspector): runtime activation via SIGUSR1 / process._debugProcess#34106robobun wants to merge 17 commits into
robobun wants to merge 17 commits into
Conversation
Collaborator
Author
alii
reviewed
Jul 14, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesRuntime inspector activation is added through SIGUSR1 on POSIX and Runtime inspector activation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@src/jsc/bindings/BunProcess.cpp`:
- Around line 4290-4361: Update Process_functionDebugProcess to use
throwSystemError for all platform-specific failures instead of plain
throwVMError strings, matching the structured error behavior of
Process_functionKill. Preserve the existing failure conditions and messages
while supplying the appropriate underlying system error codes so callers can
branch on error.code.
- Around line 4313-4357: Synchronize the debug-handler handoff between the
mapping publisher and the Windows reader in the relevant debug-thread setup code
and the shown OpenFileMappingW path. Ensure the named mapping is not published,
or is not consumed, until start_debug_thread_proc has been written and a ready
state is visible; have the reader validate readiness before calling
CreateRemoteThread, preserving the existing cleanup and error paths.
In `@src/jsc/RuntimeInspector.rs`:
- Around line 370-415: Update the install function around CreateFileMappingW and
MAPPING_HANDLE.store so the named mapping is not discoverable until
start_debug_thread_proc has been written and the view unmapped. Ensure
concurrent process._debugProcess(pid) callers cannot observe an uninitialized or
null threadProc, while preserving the existing failure cleanup behavior.
In `@src/runtime/cli/run_command.rs`:
- Around line 961-966: Update boot_standalone’s InitOptions construction to
forward ctx.runtime_options.disable_sigusr1 and ctx.runtime_options.inspect_port
into VirtualMachine::init_with_module_graph, preserving the existing cli_dupe
conversion for the optional inspect port.
In `@src/runtime/jsc_hooks.rs`:
- Line 525: Add an adjacent `// SAFETY:` comment before the inline unsafe
expression in the `debugger` branch, explaining why dereferencing `vm` is valid
at that point. Keep the condition and surrounding control flow unchanged.
🪄 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: d49ba7f0-af84-4abe-a0f3-2944484a65f5
📒 Files selected for processing (18)
scripts/build/deps/webkit.tssrc/jsc/RuntimeInspector.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunDebugger.cppsrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/vm/Semaphore.cppsrc/jsc/event_loop.rssrc/jsc/lib.rssrc/options_types/context.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/repl_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/test_command.rssrc/runtime/jsc_hooks.rstest/js/bun/runtime-inspector/runtime-inspector-posix.test.tstest/js/bun/runtime-inspector/runtime-inspector-windows.test.tstest/js/bun/runtime-inspector/runtime-inspector.test.tstest/js/node/process/process.test.js
💤 Files with no reviewable changes (1)
- test/js/node/process/process.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/bindings/BunProcess.cpp (1)
1575-1581: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the SIGUSR1 inspector handler on listener removal.
src/jsc/bindings/BunProcess.cppuninstalls the runtime-inspector SIGUSR1 handler when a user listener is added, but there’s no matching reinstall when the last listener is removed, so SIGUSR1 activation stays disabled for the rest of the process.🤖 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/bindings/BunProcess.cpp` around lines 1575 - 1581, Update the SIGUSR1 listener-removal path near Bun__Sigusr1Handler__uninstall() to reinstall the runtime-inspector SIGUSR1 handler when the final user listener is removed. Preserve the existing uninstall behavior while a user listener remains active, and invoke the established inspector-handler installation mechanism only after listener removal leaves no user handler.test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts (1)
12-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame timer-leak issue as
runtime-inspector-posix.test.ts.This
readStreamUntilis an identical copy of the one in the POSIX test file and shares the same unhandled-rejection risk from the never-clearedtimeoutpromise. See the consolidated comment for the fix.🤖 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/runtime-inspector-windows.test.ts` around lines 12 - 36, Update the Windows test helper readStreamUntil to clear or otherwise settle the timeout created for each read race once the stream condition is met or the reader completes, matching the fix applied to the POSIX test helper and preventing leftover timer rejections.
🤖 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 `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts`:
- Around line 12-31: Update readStreamUntil to retain the timeout handle and
ensure it is cleared when the stream condition is met or reader.read() fails.
Also attach handling for the timeout promise’s rejection so the losing promise
cannot produce an unhandled rejection after successful completion.
---
Outside diff comments:
In `@src/jsc/bindings/BunProcess.cpp`:
- Around line 1575-1581: Update the SIGUSR1 listener-removal path near
Bun__Sigusr1Handler__uninstall() to reinstall the runtime-inspector SIGUSR1
handler when the final user listener is removed. Preserve the existing uninstall
behavior while a user listener remains active, and invoke the established
inspector-handler installation mechanism only after listener removal leaves no
user handler.
In `@test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts`:
- Around line 12-36: Update the Windows test helper readStreamUntil to clear or
otherwise settle the timeout created for each read race once the stream
condition is met or the reader completes, matching the fix applied to the POSIX
test helper and preventing leftover timer rejections.
🪄 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: e4707f58-25a6-4a70-bbe1-72a3353bb479
📒 Files selected for processing (5)
src/jsc/RuntimeInspector.rssrc/jsc/bindings/BunProcess.cpptest/js/bun/runtime-inspector/runtime-inspector-posix.test.tstest/js/bun/runtime-inspector/runtime-inspector-windows.test.tstest/js/bun/runtime-inspector/runtime-inspector.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts`:
- Around line 22-41: Extract the duplicated readStreamUntil and hasBanner
helpers into a shared runtime-inspector harness module, preserving the existing
timer cleanup behavior. In
test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts lines 22-41, move
the helpers to the shared module and import them; in
test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts lines 22-41,
remove the duplicate implementations and import the same helpers.
🪄 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: 194f9c48-8c27-4ab6-b207-14b51c056c46
📒 Files selected for processing (7)
src/jsc/RuntimeInspector.rssrc/jsc/bindings/BunProcess.cppsrc/runtime/jsc_hooks.rstest/js/bun/http/serve-response-stream-sink-leak.test.tstest/js/bun/runtime-inspector/runtime-inspector-posix.test.tstest/js/bun/runtime-inspector/runtime-inspector-windows.test.tstest/js/bun/runtime-inspector/runtime-inspector.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/RuntimeInspector.rs
Collaborator
Author
|
CI status at 807d3d7 (build #73549, rebased onto main 8215078): the new runtime-inspector tests pass on every lane that has reported. Remaining failures are unrelated to this diff:
The WebKit bump ( Deferred to a follow-up (tracked in resolved review threads):
|
Port of #26867 to the Rust runtime with a simplified architecture: SIGUSR1 posts to an async-signal-safe semaphore; a dedicated thread fires notifyNeedDebuggerBreak on the main VM; JSC's SignalSender interrupts the VM (all tiers via InvalidationPoint patching) and VMTraps::handleTraps invokes a per-VM callback that activates the inspector and, when a pause is requested, enters Debugger::breakProgram(). Requires oven-sh/WebKit#287 (VM::setDebuggerTrapCallback + idempotent Debugger::attach + DebuggerCallFrame scope guard + disconnectFrontend ordering).
…cache rollback - install_debugger_trap_callback moved to request_inspector_activation() (init_runtime_state fires before vm.jsc_vm is written; the previous call passed a null VM*). - configure_sigusr1_handler bails when g_wtfConfig.sigThreadSuspendResume is SIGUSR1 (FreeBSD), so the GC suspend/resume handler is left intact. - activate_inspector disables the runtime transpiler cache only after Debugger::create succeeds. - Un-skip the CDP pause test and drop the ASAN skip on basic activation.
… fixes - Process_functionDebugProcess now throws ERR_MISSING_ARGS / INVALID_ARG_VALUE and a system error with .code/.syscall on kill() failure. - Free the semaphore if the SignalInspector thread fails to spawn. - Drop exact-empty-stderr assertions (ASAN/debug builds emit benign output). - readStreamUntil races each read() against the timeout so a silent child still fails with the accumulated output.
- Windows _debugProcess: throwSystemError with uv_translate_sys_error for all Win32 failures; reject null threadProc (install race) as ENOENT. - jsc_hooks: SAFETY comment for the debugger.is_some() unsafe. - RuntimeInspector: drop the dead Windows uninstall() (file-mapping stays for process lifetime; no POSIX-style user-listener uninstall on Windows). - runtime-inspector-windows.test.ts: drain the existing stderr reader to EOF instead of .text() on an already-locked stream. - runtime-inspector-posix.test.ts: use --inspect=0 / --inspect-wait=0 / --inspect-brk=0 to avoid port 6499 collisions; make the self-signal test actually self-signal via setImmediate.
- Windows _debugProcess: throw plain Error with the FormatMessageW string (matches Node's winapi_strerror; fixes test-debug-process.js). - Un-skip the infinite-loop banner test; add per-step 20s timeouts to the CDP pause test so a hang reports which step rather than a blank 60s. - serve-response-stream-sink-leak: widen slack to 3 MB. The WebKit bump in this PR carries the PerformPromiseThenOneHandler async-context bailout (oven-sh/WebKit 234d8b38), which nudges per-request commit slightly above the previous 2 MB threshold on Windows.
The CDP pause test times out on x64-asan release waiting for the Debugger.paused event (after all CDP responses arrive). The banner-only infinite-loop test covers trap delivery on ASAN; release lanes cover the full CDP pipeline. Also replace the 128+signum exit-code assertion with signalCode as per the repo convention.
The previous pin carried oven-sh/WebKit 234d8b38 (PerformPromiseThenOneHandler async-context bailout), which increased per-promise allocation enough to trip serve-response-stream-sink-leak, node-net connect-leak, and the binary-size check. Rebase the WebKit branch back onto Bun main's current WebKit pin (c9ad5813) with just the cmake 4.4 quoting fix cherry-picked on top, so the bump carries only the 49-line setDebuggerTrapCallback patch. Also: - Revert the serve-response-stream-sink-leak threshold widen (no longer needed). - runtime-inspector-windows: make the self-debug test actually call _debugProcess(process.pid) on itself, mirroring the POSIX self-signal fix.
Bun's signalCode name lookup maps macOS's SIGUSR1 (30) to SIGPWR (the Linux name for 30), so assert on the numeric exit code derived from os.constants instead.
…45d) Main bumped WEBKIT_VERSION to 4895f45d in #34009. Re-rebase oven-sh/WebKit#287 onto that commit (previously it was c9ad5813 with the cmake fix cherry-picked) so the preview build matches main's WebKit plus just the 49-line setDebuggerTrapCallback patch. Resolved conflicts in serve-response-stream-sink-leak.test.ts by taking main's version (the threshold widen in this PR was reverted anyway).
robobun
force-pushed
the
farm/c42486d7/runtime-inspector-v2
branch
from
July 16, 2026 01:33
0cfacdc to
807d3d7
Compare
Member
|
@robobun get this mergable in a new pr, close this one |
Collaborator
Author
|
Working on it. Will squash into a clean branch, address the deferred follow-ups from review, and open a new PR. This one will be closed once the replacement is up. |
Collaborator
Author
|
Superseded by #37336: same design, rebased as a single commit on current main, with the follow-ups from this review folded in (pause-vs-step distinction, disconnect on busy targets, SIGUSR1 hand-back, standalone binaries, docs, shared test helpers, and reuse of the new |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Node.js-compatible runtime inspector activation:
kill -USR1 <pid>(orprocess._debugProcess(pid)) attaches Chrome DevTools to a running Bun process, including one stuck inwhile (true) {}.Rework of #26867, ported to the Rust runtime with a simpler architecture.
Architecture
Compared to #26867 this drops StopTheWorld,
STW_CONTEXT_SWITCH, thepauseFlagsstate machine, bootstrap-pause synthetic events, and the weak-symbol hooks. Only the target VM is interrupted; the debugger thread never blocks.Trap delivery
op_check_trapsis emitted at every loop back-edge. LLInt and Baseline JIT pollm_trapBitsthere directly. DFG and FTL compile it to anInvalidationPoint(NodeMustGenerate, survives optimization), which JSC'sSignalSenderpatches to a halt from another thread, retrying every 1 ms until the trap is handled. This makes delivery reliable across all tiers withoutusePollingTraps.Node.js behaviors matched
process._debugProcess(pid)sends SIGUSR1 on POSIX, uses abun-debug-handler-<pid>file mapping +CreateRemoteThreadon Windows--inspect-port=<port>pre-configures the port (0 for random)--disable-sigusr1leaves SIGUSR1 at its default actionprocess.on('SIGUSR1', ...)listener takes precedenceDebugger.pauseinterrupts tight loopsWebKit dependency
Requires oven-sh/WebKit#287:
VM::setDebuggerTrapCallback(fn): per-VM hook invoked fromhandleTraps(NeedDebuggerBreak)on the VM's owning thread at a safe pointDebugger::attach()idempotent underUSE(BUN_JSC_ADDITIONS)DebuggerCallFrame::scope()ignores the scope register when the frame was not compiled with debugger opcodesJSGlobalObjectInspectorController::disconnectFrontend()defers agent teardown until the last frontend disconnectsWEBKIT_VERSIONis pinned to the preview build for now; once #287 merges the pin moves to the resultingautobuild-<sha>.Verification
On FreeBSD (and any platform where JSC's GC suspend/resume signal is SIGUSR1) the handler is not installed so GC stack scanning is unaffected.
Closes #26867.
Co-authored-by: Alistair Smith hi@alistair.sh
no test proof · iteration 8 · 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-windows.test.ts test/js/bun/runtime-inspector/runtime-inspector.test.ts test/js/node/process/process.test.js