Handle SIGUSR1

[alii]

What does this PR do?

Implements a SIGUSR1 handler for starting the inspector.

Replaces #25474
Blocked by #25958

How did you verify your code works?

Some tests

---

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/bun.js/webcore/Request.zig`:
- Line 1093: The import reorder of FetchRedirect in Request.zig looks unrelated
to the SIGUSR1 work; either restore the original import ordering or document why
the change is intentional: revert the FetchRedirect import line back to its
previous position if it was an accidental auto-formatter/merge artifact, or add
a short comment in Request.zig and update the commit message explaining why the
import was moved (referencing FetchRedirect and the SIGUSR1 handler context) and
re-run the project's auto-formatter to ensure consistent ordering if needed.

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@test/js/bun/runtime-inspector/runtime-inspector.test.ts`:
- Around line 437-439: The assertion is checking for the wrong inspector text;
update the test that reads targetProc.stderr into stderr (in
runtime-inspector.test.ts) to assert that stderr does not contain the actual
inspector banner string "Bun Inspector" instead of "Debugger listening"—locate
the block using the stderr variable and the expect(stderr).not.toContain(...)
call and replace the asserted substring so the test targets the real inspector
banner.
- Around line 139-140: Remove the time‑based self‑exit calls (setTimeout(() =>
process.exit(0), 5000)) used to keep the process alive in the
runtime-inspector.test.ts fixtures and instead coordinate termination
explicitly: after invoking the helper that calls _debugProcess, wait for the
expected event/condition or the helper's completion and let the parent test
cleanup (await using + kill) end the child, or send a controlled signal to the
child when the test condition is met; replace each setTimeout occurrence
(including the other two occurrences mentioned) with an explicit await/Promise
that resolves on the desired condition or the parent's kill, and remove the
process.exit invocation.
- Around line 14-44: In waitForDebuggerListening, avoid creating concurrent
reader.read() calls by reusing a single pendingRead Promise across loop
iterations: introduce a pendingRead variable initialized to reader.read() before
the loop, race that pendingRead with the timeout each iteration, only clear
pendingRead and create a new reader.read() after you consume a non-timeout
result (or when result.done), and on timeout simply continue without calling
reader.read() again; reference reader.read() and pendingRead in your changes so
you preserve the Web Streams API contract and prevent overlapping reads.

[coderabbitai]

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 (1)

src/bun.js.zig (1)

178-190: Standalone boot path still ignores --disable-sigusr1.

boot() now threads the flag into VirtualMachine.init, but bootStandalone() still omits it, so standalone executables won’t honor the option. Please pass it through for parity and to avoid unexpected SIGUSR1 activation in compiled binaries.

✅ Proposed fix for bootStandalone()

         run = .{
             .vm = try VirtualMachine.initWithModuleGraph(.{
                 .allocator = arena.allocator(),
                 .log = ctx.log,
                 .args = ctx.args,
                 .graph = graph_ptr,
                 .is_main_thread = true,
                 .smol = ctx.runtime_options.smol,
                 .debugger = ctx.runtime_options.debugger,
                 .dns_result_order = DNSResolver.Order.fromStringOrDie(ctx.runtime_options.dns_result_order),
+                .disable_sigusr1 = ctx.runtime_options.disable_sigusr1,
             }),
             .arena = arena,
             .ctx = ctx,
             .entry_path = entry_path,
         };

🤖 Fix all issues with AI agents

In `@src/bun.js/bindings/BunProcess.cpp`:
- Around line 3873-3876: Add a null/validity check for the function pointer read
from the mapped view: after assigning LPTHREAD_START_ROUTINE threadProc =
*reinterpret_cast<LPTHREAD_START_ROUTINE*>(pFunc) and before calling
CreateRemoteThread, verify that threadProc is not null (and optionally that
pFunc was non-null) and bail out with appropriate cleanup (e.g.,
UnmapViewOfFile(pFunc) if not already done, set/return an error) if the pointer
is invalid; reference the variables threadProc and pFunc and the call to
CreateRemoteThread to locate where to insert the guard.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add null check for threadProc before use.

After reading from the mapped memory, threadProc could be null or contain invalid data if the target process's debug handler mapping was corrupted or improperly initialized. Consider validating the pointer before passing it to CreateRemoteThread.

🔧 Suggested fix

     LPTHREAD_START_ROUTINE threadProc = *reinterpret_cast<LPTHREAD_START_ROUTINE*>(pFunc);
     UnmapViewOfFile(pFunc);
     CloseHandle(hMapping);

+    if (!threadProc) {
+        throwVMError(globalObject, scope, makeString("Invalid debug handler for process "_s, pid));
+        return {};
+    }
+
     HANDLE hProcess = OpenProcess(PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION | PROCESS_VM_OPERATION | PROCESS_VM_WRITE | PROCESS_VM_READ, FALSE, pid);

🤖 Prompt for AI Agents

In `@src/bun.js/bindings/BunProcess.cpp` around lines 3873 - 3876, Add a
null/validity check for the function pointer read from the mapped view: after
assigning LPTHREAD_START_ROUTINE threadProc =
*reinterpret_cast<LPTHREAD_START_ROUTINE*>(pFunc) and before calling
CreateRemoteThread, verify that threadProc is not null (and optionally that
pFunc was non-null) and bail out with appropriate cleanup (e.g.,
UnmapViewOfFile(pFunc) if not already done, set/return an error) if the pointer
is invalid; reference the variables threadProc and pFunc and the call to
CreateRemoteThread to locate where to insert the guard.

[coderabbitai]

Actionable comments posted: 8

🤖 Fix all issues with AI agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts`:
- Around line 14-20: The test currently kills the child process with proc.kill()
but doesn't ensure deterministic shutdown or assert the child's exit code; add a
SIGTERM handler in the child fixture script (the snippet that writes pid and
prints "READY" and uses setInterval) that calls process.exit(0) (or another
deterministic code) on 'SIGTERM' so the process exits cleanly, and then after
calling proc.kill() in the test await proc.exited and assert proc.exitCode === 0
(or the chosen exit code); apply the same change to the other occurrence
referenced (lines ~61-67) so all child fixtures and tests check exit
deterministically.
- Around line 205-210: The test currently uses time-based waiting (Bun.sleep)
and a never-ending setInterval to keep the child alive; instead, remove the
timing sleep and replace the pattern with an immediate kill of the child after
it signals readiness (the code that writes pid and logs "READY"), add a SIGTERM
handler in the child (process.on('SIGTERM') -> process.exit(0) or similar) so
the process exits cleanly, and in the test assert the child's exit code (e.g.,
check child.exitCode or wait for exit and expect it to be 0); update the code
around the child process helper that writes pid/READY and the test logic that
currently calls Bun.sleep/kill to perform an immediate kill and assert the exit
code.
- Around line 401-440: The test uses an inline setTimeout in the --inspect-brk
fixture; remove the "setTimeout(() => process.exit(0), 500)" script and replace
it with a simple immediate exit script (e.g., "process.exit(0)") passed to spawn
via bunExe()/cmd so the test no longer relies on timing. Keep the existing logic
that reads stderr, sends SIGUSR1, calls proc.kill(), and waits on proc.exited,
but add an assertion to verify the process exit code (check proc.exitCode or the
resolved exit value) is 0 after awaiting proc.exited, and keep the existing
expectation that stderr matches two "Bun Inspector" occurrences.
- Around line 357-396: Replace the inline "setTimeout" script in the spawn cmd
so the child stays paused by --inspect-wait (e.g., use a no-op script instead of
setTimeout) and remove reliance on timers; after sending proc.kill() await
proc.exited and assert the child's exit status (check proc.exitCode or
proc.signal from the proc object) to verify the process terminated with the
expected code/signal; update the test around spawn/bunExe, proc, reader,
decoder, stderr, proc.kill, proc.exited and matches/expect to reflect these
changes.
- Around line 316-351: The test currently uses a timer-based shutdown inside the
spawned fixture (the setTimeout + setInterval in test.js) which we must remove;
instead modify the fixture (test.js) to install a SIGTERM handler (e.g.,
process.on('SIGTERM', () => process.exit(0))) and remove the
setTimeout/setInterval, then update the test harness around spawn/using proc to
send SIGTERM to the child PID (process.kill(pid, 'SIGTERM')) and wait for
proc.exited (and optionally fallback to kill if it doesn't exit), keeping the
existing SIGUSR1 check logic intact so the test asserts the inspector banner
only comes from --inspect. Ensure references to pid (from
Bun.file(.../pid).text()) and proc (the spawn result) are used for the new
signal flow.
- Around line 131-137: Replace the setTimeout-based shutdown in the SIGUSR1
handler with a next-tick exit: locate the process.on("SIGUSR1") handler where
the local variable count is incremented and "SIGNAL_"+count is logged, and when
count >= 3 call process.nextTick(() => process.exit(0)) (or equivalent next-tick
mechanism) instead of setTimeout(..., 100) so the process exits on the next tick
without using timers in the test.
- Around line 75-78: The SIGUSR1 handler registered via process.on("SIGUSR1")
currently uses setTimeout to call process.exit which causes timing flakiness;
replace the setTimeout(...) call in that handler with a next-tick exit (use
process.nextTick to invoke process.exit(0)) so the handler still logs
"USER_HANDLER_CALLED" and then exits synchronously on the next tick without
timer-based delays.
- Around line 272-300: Replace the ad-hoc setTimeout self-signal with next-tick
scheduling and assert the child exit code: remove the setTimeout(() =>
process.kill(process.pid, "SIGUSR1"), 50) and instead schedule the self-signal
using process.nextTick(() => process.kill(process.pid, "SIGUSR1")); also avoid
leaving a forever setInterval() — keep the process alive only as needed for the
signal (e.g., remove setInterval(() => {}, 1000) and rely on the test harness),
then after proc.kill() await proc.exited and verify the exit code on the proc
(check proc.exitCode or the resolved exit status) to assert the expected
termination.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add deterministic shutdown and assert exit code.

Killing the child without checking proc.exited can hide crashes. Add a SIGTERM handler in the fixture and assert the exit code after proc.kill().

🔧 Proposed fix

         fs.writeFileSync(path.join(process.cwd(), "pid"), String(process.pid));
         console.log("READY");
+        process.on("SIGTERM", () => process.exit(0));

         // Keep process alive
         setInterval(() => {}, 1000);

-    await proc.exited;
+    const exitCode = await proc.exited;

     expect(stderr).toContain("Bun Inspector");
     expect(stderr).toMatch(/ws:\/\/localhost:\d+\//);
+    expect(exitCode).toBe(0);

As per coding guidelines, always check exit codes.

Also applies to: 61-67

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
14 - 20, The test currently kills the child process with proc.kill() but doesn't
ensure deterministic shutdown or assert the child's exit code; add a SIGTERM
handler in the child fixture script (the snippet that writes pid and prints
"READY" and uses setInterval) that calls process.exit(0) (or another
deterministic code) on 'SIGTERM' so the process exits cleanly, and then after
calling proc.kill() in the test await proc.exited and assert proc.exitCode === 0
(or the chosen exit code); apply the same change to the other occurrence
referenced (lines ~61-67) so all child fixtures and tests check exit
deterministically.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Remove timer-based exit in SIGUSR1 handler.

Replace the setTimeout with a next-tick exit to avoid timing dependencies.

🔧 Proposed fix

         process.on("SIGUSR1", () => {
           console.log("USER_HANDLER_CALLED");
-          setTimeout(() => process.exit(0), 100);
+          queueMicrotask(() => process.exit(0));
         });

As per coding guidelines, avoid setTimeout in tests.

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
75 - 78, The SIGUSR1 handler registered via process.on("SIGUSR1") currently uses
setTimeout to call process.exit which causes timing flakiness; replace the
setTimeout(...) call in that handler with a next-tick exit (use process.nextTick
to invoke process.exit(0)) so the handler still logs "USER_HANDLER_CALLED" and
then exits synchronously on the next tick without timer-based delays.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Avoid setTimeout for third-signal shutdown.

Exit on the next tick after emitting SIGNAL_3 instead of using a timer.

🔧 Proposed fix

           console.log("SIGNAL_" + count);
           if (count >= 3) {
-            setTimeout(() => process.exit(0), 100);
+            queueMicrotask(() => process.exit(0));
           }
         });

As per coding guidelines, avoid setTimeout in tests.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let count = 0;
	process.on("SIGUSR1", () => {
	count++;
	console.log("SIGNAL_" + count);
	if (count >= 3) {
	setTimeout(() => process.exit(0), 100);
	}
	let count = 0;
	process.on("SIGUSR1", () => {
	count++;
	console.log("SIGNAL_" + count);
	if (count >= 3) {
	queueMicrotask(() => process.exit(0));
	}
	});

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
131 - 137, Replace the setTimeout-based shutdown in the SIGUSR1 handler with a
next-tick exit: locate the process.on("SIGUSR1") handler where the local
variable count is incremented and "SIGNAL_"+count is logged, and when count >= 3
call process.nextTick(() => process.exit(0)) (or equivalent next-tick mechanism)
instead of setTimeout(..., 100) so the process exits on the next tick without
using timers in the test.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid time-based sleep and assert clean exit.

Bun.sleep(100) is timing-based and can flake. Signal handling is synchronous here, so kill immediately and assert the exit code (add a SIGTERM handler to exit cleanly).

🔧 Proposed fix

         fs.writeFileSync(path.join(process.cwd(), "pid"), String(process.pid));
         console.log("READY");
+        process.on("SIGTERM", () => process.exit(0));

         // Keep process alive until test kills it
         setInterval(() => {}, 1000);

-    // Give a brief moment for any potential second banner to appear, then kill the process
-    // We can't wait indefinitely since a second banner should NOT appear
-    await Bun.sleep(100);
-
-    // Kill process and collect remaining stderr
-    proc.kill();
+    // Kill process and collect remaining stderr (signal handling is synchronous)
+    proc.kill();

     stderr += remainingStderr;

     // Should only see one "Bun Inspector" banner (two occurrences of the text, for header and footer)
     const matches = stderr.match(/Bun Inspector/g);
     expect(matches?.length ?? 0).toBe(2);
+    expect(exitCode).toBe(0);

As per coding guidelines, avoid timing-based waits and always check exit codes.

Also applies to: 251-264

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
205 - 210, The test currently uses time-based waiting (Bun.sleep) and a
never-ending setInterval to keep the child alive; instead, remove the timing
sleep and replace the pattern with an immediate kill of the child after it
signals readiness (the code that writes pid and logs "READY"), add a SIGTERM
handler in the child (process.on('SIGTERM') -> process.exit(0) or similar) so
the process exits cleanly, and in the test assert the child's exit code (e.g.,
check child.exitCode or wait for exit and expect it to be 0); update the code
around the child process helper that writes pid/READY and the test logic that
currently calls Bun.sleep/kill to perform an immediate kill and assert the exit
code.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Replace setTimeout self-signal with next-tick scheduling and verify exit code.

🔧 Proposed fix

-        setTimeout(() => {
-          process.kill(process.pid, "SIGUSR1");
-        }, 50);
+        queueMicrotask(() => {
+          process.kill(process.pid, "SIGUSR1");
+        });
+        process.on("SIGTERM", () => process.exit(0));

-    await proc.exited;
+    const exitCode = await proc.exited;

     expect(stderr).toContain("Bun Inspector");
+    expect(exitCode).toBe(0);

As per coding guidelines, avoid setTimeout in tests and always check exit codes.

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
272 - 300, Replace the ad-hoc setTimeout self-signal with next-tick scheduling
and assert the child exit code: remove the setTimeout(() =>
process.kill(process.pid, "SIGUSR1"), 50) and instead schedule the self-signal
using process.nextTick(() => process.kill(process.pid, "SIGUSR1")); also avoid
leaving a forever setInterval() — keep the process alive only as needed for the
signal (e.g., remove setInterval(() => {}, 1000) and rely on the test harness),
then after proc.kill() await proc.exited and verify the exit code on the proc
(check proc.exitCode or the resolved exit status) to assert the expected
termination.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Drop timer-based shutdown under --inspect; terminate explicitly.

Use a SIGTERM handler in the fixture and have the parent kill the process after signaling, so the test doesn’t rely on a timer.

🔧 Proposed fix

-        setTimeout(() => process.exit(0), 500);
+        process.on("SIGTERM", () => process.exit(0));
         setInterval(() => {}, 1000);

     // Send SIGUSR1 - should be ignored since RuntimeInspector is not installed
     process.kill(pid, "SIGUSR1");
+    proc.kill();

     const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);

As per coding guidelines, avoid setTimeout in tests.

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
316 - 351, The test currently uses a timer-based shutdown inside the spawned
fixture (the setTimeout + setInterval in test.js) which we must remove; instead
modify the fixture (test.js) to install a SIGTERM handler (e.g.,
process.on('SIGTERM', () => process.exit(0))) and remove the
setTimeout/setInterval, then update the test harness around spawn/using proc to
send SIGTERM to the child PID (process.kill(pid, 'SIGTERM')) and wait for
proc.exited (and optionally fallback to kill if it doesn't exit), keeping the
existing SIGUSR1 check logic intact so the test asserts the inspector banner
only comes from --inspect. Ensure references to pid (from
Bun.file(.../pid).text()) and proc (the spawn result) are used for the new
signal flow.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Remove setTimeout in --inspect-wait fixture and verify exit code.

🔧 Proposed fix

-    await using proc = spawn({
-      cmd: [bunExe(), "--inspect-wait", "-e", "setTimeout(() => process.exit(0), 500)"],
+    await using proc = spawn({
+      cmd: [
+        bunExe(),
+        "--inspect-wait",
+        "-e",
+        `
+          process.on("SIGTERM", () => process.exit(0));
+          setInterval(() => {}, 1000);
+        `,
+      ],
       env: bunEnv,
       stdout: "pipe",
       stderr: "pipe",
     });

-    await proc.exited;
+    const exitCode = await proc.exited;

     // Should only see one "Bun Inspector" banner (from --inspect-wait flag, not from SIGUSR1)
     // The banner has two occurrences of "Bun Inspector" (header and footer)
     const matches = stderr.match(/Bun Inspector/g);
     expect(matches?.length ?? 0).toBe(2);
+    expect(exitCode).toBe(0);

As per coding guidelines, avoid setTimeout in tests and always check exit codes.

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
357 - 396, Replace the inline "setTimeout" script in the spawn cmd so the child
stays paused by --inspect-wait (e.g., use a no-op script instead of setTimeout)
and remove reliance on timers; after sending proc.kill() await proc.exited and
assert the child's exit status (check proc.exitCode or proc.signal from the proc
object) to verify the process terminated with the expected code/signal; update
the test around spawn/bunExe, proc, reader, decoder, stderr, proc.kill,
proc.exited and matches/expect to reflect these changes.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Remove setTimeout in --inspect-brk fixture and verify exit code.

🔧 Proposed fix

-    await using proc = spawn({
-      cmd: [bunExe(), "--inspect-brk", "-e", "setTimeout(() => process.exit(0), 500)"],
+    await using proc = spawn({
+      cmd: [
+        bunExe(),
+        "--inspect-brk",
+        "-e",
+        `
+          process.on("SIGTERM", () => process.exit(0));
+          setInterval(() => {}, 1000);
+        `,
+      ],
       env: bunEnv,
       stdout: "pipe",
       stderr: "pipe",
     });

-    await proc.exited;
+    const exitCode = await proc.exited;

     // Should only see one "Bun Inspector" banner (from --inspect-brk flag, not from SIGUSR1)
     // The banner has two occurrences of "Bun Inspector" (header and footer)
     const matches = stderr.match(/Bun Inspector/g);
     expect(matches?.length ?? 0).toBe(2);
+    expect(exitCode).toBe(0);

As per coding guidelines, avoid setTimeout in tests and always check exit codes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	await using proc = spawn({
	cmd: [bunExe(), "--inspect-brk", "-e", "setTimeout(() => process.exit(0), 500)"],
	env: bunEnv,
	stdout: "pipe",
	stderr: "pipe",
	});
	const reader = proc.stderr.getReader();
	const decoder = new TextDecoder();
	let stderr = "";
	while ((stderr.match(/Bun Inspector/g) || []).length < 2) {
	const { value, done } = await reader.read();
	if (done) break;
	stderr += decoder.decode(value, { stream: true });
	}
	// Send SIGUSR1 - should be ignored since debugger is already active
	process.kill(proc.pid, "SIGUSR1");
	// Kill process since --inspect-brk would wait for connection
	// Signal processing is synchronous, so no sleep needed
	proc.kill();
	// Read any remaining stderr
	while (true) {
	const { value, done } = await reader.read();
	if (done) break;
	stderr += decoder.decode(value, { stream: true });
	}
	stderr += decoder.decode();
	reader.releaseLock();
	await proc.exited;
	// Should only see one "Bun Inspector" banner (from --inspect-brk flag, not from SIGUSR1)
	// The banner has two occurrences of "Bun Inspector" (header and footer)
	const matches = stderr.match(/Bun Inspector/g);
	expect(matches?.length ?? 0).toBe(2);
	});
	await using proc = spawn({
	cmd: [
	bunExe(),
	"--inspect-brk",
	"-e",
	`
	process.on("SIGTERM", () => process.exit(0));
	setInterval(() => {}, 1000);
	`,
	],
	env: bunEnv,
	stdout: "pipe",
	stderr: "pipe",
	});
	const reader = proc.stderr.getReader();
	const decoder = new TextDecoder();
	let stderr = "";
	while ((stderr.match(/Bun Inspector/g) || []).length < 2) {
	const { value, done } = await reader.read();
	if (done) break;
	stderr += decoder.decode(value, { stream: true });
	}
	// Send SIGUSR1 - should be ignored since debugger is already active
	process.kill(proc.pid, "SIGUSR1");
	// Kill process since --inspect-brk would wait for connection
	// Signal processing is synchronous, so no sleep needed
	proc.kill();
	// Read any remaining stderr
	while (true) {
	const { value, done } = await reader.read();
	if (done) break;
	stderr += decoder.decode(value, { stream: true });
	}
	stderr += decoder.decode();
	reader.releaseLock();
	const exitCode = await proc.exited;
	// Should only see one "Bun Inspector" banner (from --inspect-brk flag, not from SIGUSR1)
	// The banner has two occurrences of "Bun Inspector" (header and footer)
	const matches = stderr.match(/Bun Inspector/g);
	expect(matches?.length ?? 0).toBe(2);
	expect(exitCode).toBe(0);

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
401 - 440, The test uses an inline setTimeout in the --inspect-brk fixture;
remove the "setTimeout(() => process.exit(0), 500)" script and replace it with a
simple immediate exit script (e.g., "process.exit(0)") passed to spawn via
bunExe()/cmd so the test no longer relies on timing. Keep the existing logic
that reads stderr, sends SIGUSR1, calls proc.kill(), and waits on proc.exited,
but add an assertion to verify the process exit code (check proc.exitCode or the
resolved exit value) is 0 after awaiting proc.exited, and keep the existing
expectation that stderr matches two "Bun Inspector" occurrences.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/bun.js.zig`:
- Around line 189-190: bootStandalone omits the disable_sigusr1 option when
constructing Options for VirtualMachine.initWithModuleGraph; update the Options
passed in bootStandalone to include .disable_sigusr1 =
ctx.runtime_options.disable_sigusr1 so standalone executables respect the
--disable-sigusr1 flag (look for bootStandalone,
VirtualMachine.initWithModuleGraph, the Options struct, and
ctx.runtime_options.disable_sigusr1 to make the change).

[claude]

This test sets an explicit timeout of 30000ms, which violates the CLAUDE.md testing rules.

From test/CLAUDE.md:

CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

Suggested change

	30000,
	);

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts`:
- Around line 191-197: The inline snapshot compares raw output which is
platform-specific; update the assertion to use normalizeBunSnapshot to normalize
the output before snapshotting by replacing
expect(output).toMatchInlineSnapshot(...) with
expect(normalizeBunSnapshot(output)).toMatchInlineSnapshot(...), and ensure
normalizeBunSnapshot is imported/available in this test file (refer to
normalizeBunSnapshot and toMatchInlineSnapshot and the output variable to locate
the change).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Normalize snapshot output before inline snapshot.

Line 191-197 snapshots raw output; use normalizeBunSnapshot to avoid platform-specific noise.

✅ Suggested change

-import { bunEnv, bunExe, isASAN, isWindows, tempDir } from "harness";
+import { bunEnv, bunExe, isASAN, isWindows, normalizeBunSnapshot, tempDir } from "harness";

-    expect(output).toMatchInlineSnapshot(`
+    expect(normalizeBunSnapshot(output)).toMatchInlineSnapshot(`
       "READY
       SIGNAL_1
       SIGNAL_2
       SIGNAL_3
       "
     `);

As per coding guidelines: Use normalizeBunSnapshot to normalize snapshot output in tests.

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector-posix.test.ts` around lines
191 - 197, The inline snapshot compares raw output which is platform-specific;
update the assertion to use normalizeBunSnapshot to normalize the output before
snapshotting by replacing expect(output).toMatchInlineSnapshot(...) with
expect(normalizeBunSnapshot(output)).toMatchInlineSnapshot(...), and ensure
normalizeBunSnapshot is imported/available in this test file (refer to
normalizeBunSnapshot and toMatchInlineSnapshot and the output variable to locate
the change).

[claude]

CLAUDE.md violation: Explicit test timeout should be removed.

Per test/CLAUDE.md:

CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

Suggested change

	);
	);

[claude]

CLAUDE.md violation: Using Bun.sleep() to wait for an arbitrary time in tests is discouraged.

Per test/CLAUDE.md:

CRITICAL: Do not write flaky tests. Do not use setTimeout in tests. Instead, await the condition to be met. You are not testing the TIME PASSING, you are testing the CONDITION.

Consider restructuring this test to avoid the arbitrary sleep. For example, since we're testing that a second banner does NOT appear, this may be an acceptable tradeoff given the nature of the test (proving a negative). If intentional, consider adding a comment explaining why this is necessary.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@test/js/bun/runtime-inspector/runtime-inspector.test.ts`:
- Line 318: Remove the explicit per-test timeout value (the trailing 30000) from
the test declaration in runtime-inspector.test.ts; locate the failing test call
that supplies 30000 as the third argument (likely a test(...) or it(...)
invocation) and delete that timeout parameter so the test relies on Bun's
built-in timeout handling (or move any needed extended timeout to the test
runner configuration instead).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Consider removing the explicit test timeout.

As per coding guidelines: "Do not set a timeout on tests. Bun already has timeouts configured." The 30000ms timeout here may conflict with Bun's built-in test timeout configuration.

If this test genuinely requires extended time, consider configuring it at the test runner level rather than per-test.

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector.test.ts` at line 318, Remove
the explicit per-test timeout value (the trailing 30000) from the test
declaration in runtime-inspector.test.ts; locate the failing test call that
supplies 30000 as the third argument (likely a test(...) or it(...) invocation)
and delete that timeout parameter so the test relies on Bun's built-in timeout
handling (or move any needed extended timeout to the test runner configuration
instead).

[claude]

CLAUDE.md violation: Explicit test timeout.

The test specifies a timeout of 30000ms as the third argument. Per test/CLAUDE.md:

CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

Suggested change

	30000,
	);

[claude]

CLAUDE.md violation: Using Bun.sleep() to wait for time to pass.

Per test/CLAUDE.md:

Do not write flaky tests. Unless explicitly asked, never wait for time to pass in tests. Always wait for the condition to be met instead of waiting for an arbitrary amount of time.

This is a tricky case since the test is verifying something does NOT happen (no second banner). Consider an alternative approach such as collecting all stderr after killing the process, rather than waiting a fixed time before killing.

[claude]

CLAUDE.md violation: Using Bun.sleep() in polling loop.

Per test/CLAUDE.md:

Do not write flaky tests. Unless explicitly asked, never wait for time to pass in tests.

Consider having the target script output "READY" to stdout after writing the PID file (before entering the infinite loop), then wait for that signal instead of polling with sleep:

fs.writeFileSync(path.join(process.cwd(), "pid"), String(process.pid));
console.log("READY");
while (true) {}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@test/js/bun/runtime-inspector/runtime-inspector.test.ts`:
- Around line 165-205: The stderrReader is not guaranteed to be released if the
banner wait times out; wrap the reading loop and subsequent inspector checks
(the while loop that reads from stderrReader, the second spawn/expect blocks
that rely on its output) in a try/finally and call stderrReader.releaseLock() in
the finally block (mirroring the waitForDebuggerListening pattern) so the reader
is always released even on errors; reference stderrReader, stderrDecoder,
stderr, the banner wait loop, and the second spawn (debug2) when making the
change.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Add try/finally for stderrReader cleanup to match waitForDebuggerListening pattern.

If the timeout throws an error at line 186, stderrReader.releaseLock() on line 205 won't be called. While await using targetProc will handle process cleanup, it's good practice to ensure the reader is properly released.

♻️ Suggested refactor for consistent cleanup

       // Start reading stderr before triggering debugger
       const stderrReader = targetProc.stderr.getReader();
       const stderrDecoder = new TextDecoder();
       let stderr = "";
 
       // Call _debugProcess the first time
       await using debug1 = spawn({
         cmd: [bunExe(), "-e", `process._debugProcess(${pid})`],
         env: bunEnv,
         stdout: "pipe",
         stderr: "pipe",
       });
       const debug1Stderr = await debug1.stderr.text();
       expect(debug1Stderr).toBe("");
       expect(await debug1.exited).toBe(0);
 
-      // Wait for the full debugger banner (header + content + footer) with timeout
-      const bannerStartTime = Date.now();
-      const bannerTimeout = 30000;
-      while ((stderr.match(/Bun Inspector/g) || []).length < 2) {
-        if (Date.now() - bannerStartTime > bannerTimeout) {
-          throw new Error(`Timeout waiting for inspector banner. Got: "${stderr}"`);
+      try {
+        // Wait for the full debugger banner (header + content + footer) with timeout
+        const bannerStartTime = Date.now();
+        const bannerTimeout = 30000;
+        while ((stderr.match(/Bun Inspector/g) || []).length < 2) {
+          if (Date.now() - bannerStartTime > bannerTimeout) {
+            throw new Error(`Timeout waiting for inspector banner. Got: "${stderr}"`);
+          }
+          const { value, done } = await stderrReader.read();
+          if (done) break;
+          stderr += stderrDecoder.decode(value, { stream: true });
         }
-        const { value, done } = await stderrReader.read();
-        if (done) break;
-        stderr += stderrDecoder.decode(value, { stream: true });
+      } finally {
+        stderrReader.releaseLock();
       }
 
       // Call _debugProcess again - inspector should not activate twice
       ...
-
-      // Release the reader and kill the target
-      stderrReader.releaseLock();
       targetProc.kill();

🤖 Prompt for AI Agents

In `@test/js/bun/runtime-inspector/runtime-inspector.test.ts` around lines 165 -
205, The stderrReader is not guaranteed to be released if the banner wait times
out; wrap the reading loop and subsequent inspector checks (the while loop that
reads from stderrReader, the second spawn/expect blocks that rely on its output)
in a try/finally and call stderrReader.releaseLock() in the finally block
(mirroring the waitForDebuggerListening pattern) so the reader is always
released even on errors; reference stderrReader, stderrDecoder, stderr, the
banner wait loop, and the second spawn (debug2) when making the change.

[claude]

CLAUDE.md violation: Exit code is checked without first checking stderr/stdout.

Per CLAUDE.md:

When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure.

Suggested change

	expect(await debug1.exited).toBe(0);
	const debug1Stderr = await debug1.stderr.text();
	expect(debug1Stderr).toBe("");
	expect(await debug1.exited).toBe(0);

[claude]

CLAUDE.md violation: Exit code is checked without first checking stderr/stdout.

Per CLAUDE.md:

When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure.

Suggested change

	expect(await debug2.exited).toBe(0);
	const debug2Stderr = await debug2.stderr.text();
	expect(debug2Stderr).toBe("");
	expect(await debug2.exited).toBe(0);

[alii]

Replacing with #26867