From cc6704de2fbda06bb4808febf0519ef5bb206b28 Mon Sep 17 00:00:00 2001 From: Alistair Smith Date: Mon, 5 Jan 2026 17:08:08 +0000 Subject: [PATCH] Address PR review comments for runtime inspector - Add PID validation in Windows tests to fail early on invalid PID file content - Add diagnostic logging for Windows file mapping failures - Remove log() call from signal handler context for async-signal-safety - Remove unused previous_action variable from POSIX signal handler --- src/bun.js/event_loop/RuntimeInspector.zig | 13 ++++++------- .../runtime-inspector-windows.test.ts | 4 ++++ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/bun.js/event_loop/RuntimeInspector.zig b/src/bun.js/event_loop/RuntimeInspector.zig index 1ca6df7c28..972d465d25 100644 --- a/src/bun.js/event_loop/RuntimeInspector.zig +++ b/src/bun.js/event_loop/RuntimeInspector.zig @@ -29,10 +29,9 @@ var installed: std.atomic.Value(bool) = std.atomic.Value(bool).init(false); var inspector_activation_requested: std.atomic.Value(bool) = std.atomic.Value(bool).init(false); fn requestInspectorActivation() void { - const vm = VirtualMachine.getMainThreadVM() orelse { - log("No main thread VM available", .{}); - return; - }; + // Note: This function may be called from signal handler context on POSIX, + // so we must only use async-signal-safe operations here. + const vm = VirtualMachine.getMainThreadVM() orelse return; inspector_activation_requested.store(true, .release); vm.eventLoop().wakeup(); @@ -94,8 +93,6 @@ pub fn isInstalled() bool { } const posix = if (Environment.isPosix) struct { - var previous_action: std.posix.Sigaction = undefined; - fn signalHandler(_: c_int) callconv(.c) void { // This handler runs in signal context, so we can only do async-signal-safe operations. // Set the atomic flag and wake the event loop. @@ -112,7 +109,7 @@ const posix = if (Environment.isPosix) struct { .flags = std.posix.SA.RESTART, }; - std.posix.sigaction(std.posix.SIG.USR1, &act, &previous_action); + std.posix.sigaction(std.posix.SIG.USR1, &act, null); return true; } @@ -211,11 +208,13 @@ const windows = if (Environment.isWindows) struct { _ = UnmapViewOfFile(ptr); return true; } else { + log("MapViewOfFile failed", .{}); _ = bun.windows.CloseHandle(handle); mapping_handle = null; return false; } } else { + log("CreateFileMappingW failed for bun-debug-handler-{d}", .{pid}); return false; } } diff --git a/test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts b/test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts index fa5cf7ee39..8948c74971 100644 --- a/test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts +++ b/test/js/bun/runtime-inspector/runtime-inspector-windows.test.ts @@ -39,6 +39,7 @@ describe.skipIf(!isWindows)("Runtime inspector Windows file mapping", () => { reader.releaseLock(); const pid = parseInt(await Bun.file(join(String(dir), "pid")).text(), 10); + expect(pid).toBeGreaterThan(0); // Use _debugProcess which uses file mapping on Windows await using debugProc = spawn({ @@ -130,6 +131,7 @@ describe.skipIf(!isWindows)("Runtime inspector Windows file mapping", () => { reader.releaseLock(); const pid = parseInt(await Bun.file(join(String(dir), "pid")).text(), 10); + expect(pid).toBeGreaterThan(0); // Set up stderr reader to wait for debugger to start const stderrReader = targetProc.stderr.getReader(); @@ -225,6 +227,8 @@ describe.skipIf(!isWindows)("Runtime inspector Windows file mapping", () => { const pid1 = parseInt(await Bun.file(join(String(dir), "pid-1")).text(), 10); const pid2 = parseInt(await Bun.file(join(String(dir), "pid-2")).text(), 10); + expect(pid1).toBeGreaterThan(0); + expect(pid2).toBeGreaterThan(0); // Activate inspector in both await using debug1 = spawn({