From 899b3be30ffd00c1cb5e14953391e9d857c23e08 Mon Sep 17 00:00:00 2001
From: Salim Habash <salim@habash.net>
Date: Wed, 27 May 2026 00:41:57 -0700
Subject: [PATCH] fix(bun-polyfill): resolve proc.exited on spawn failure too
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Codex review pass 2 flagged a [P2]: Node emits 'error' but not 'exit'
when the spawned binary is missing, so the lifecycle Promise that only
listened to 'exit' hangs forever. `await proc.exited` waits until the
caller's own timeout fires, which on a missing `bun`, `powershell`, or
similar burns the full 10 s the DPAPI helper allows.

Listen to both 'exit' and 'error' on the lifecycle Promise so spawn
failures resolve immediately. Also add 'close' as a third resolve
trigger on the pipe drains for the same reason — the stdio streams may
fire 'close' without 'end' or 'error' when the process never started.

Regression test asserts proc.exited resolves (not 'TIMEOUT') when the
binary doesn't exist.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 browse/src/bun-polyfill.cjs      | 22 ++++++++++++++++------
 browse/test/bun-polyfill.test.ts | 24 ++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/browse/src/bun-polyfill.cjs b/browse/src/bun-polyfill.cjs
index 275d92a7e..bc7236e52 100644
--- a/browse/src/bun-polyfill.cjs
+++ b/browse/src/bun-polyfill.cjs
@@ -104,8 +104,12 @@ globalThis.Bun = {
       const chunks = [];
       const done = new Promise((resolve) => {
         stream.on('data', (chunk) => chunks.push(chunk));
+        // Any terminal event resolves: 'end' on normal close, 'error' on a
+        // stream-level error, 'close' as the belt-and-suspenders for spawn
+        // failures where Node fires 'close' but neither 'end' nor 'error'.
         stream.once('end', resolve);
-        stream.once('error', resolve);   // exit event carries the real status
+        stream.once('error', resolve);
+        stream.once('close', resolve);
       });
       return { done, chunks };
     };
@@ -131,11 +135,17 @@ globalThis.Bun = {
       proc.once('error', () => {
         if (exitStatus === undefined) exitStatus = 1;
       });
-      Promise.all([
-        new Promise((r) => proc.once('exit', r)),
-        stdoutDrain.done,
-        stderrDrain.done,
-      ]).then(() => resolveExited(exitStatus !== undefined ? exitStatus : 0));
+      // Wait for either 'exit' (normal child lifecycle) or 'error' (spawn
+      // failure — Node fires error without exit when the binary is missing).
+      // Either path resolves the lifecycle promise; without listening to both
+      // a spawn error hangs `await proc.exited` until the consumer's own
+      // timeout fires.
+      const lifecycle = new Promise((r) => {
+        proc.once('exit', r);
+        proc.once('error', r);
+      });
+      Promise.all([lifecycle, stdoutDrain.done, stderrDrain.done])
+        .then(() => resolveExited(exitStatus !== undefined ? exitStatus : 0));
     });
 
     // Replay buffered output as a fresh Web ReadableStream. `start()` awaits
diff --git a/browse/test/bun-polyfill.test.ts b/browse/test/bun-polyfill.test.ts
index c9309208b..6b7ae78a9 100644
--- a/browse/test/bun-polyfill.test.ts
+++ b/browse/test/bun-polyfill.test.ts
@@ -97,6 +97,30 @@ describe('bun-polyfill', () => {
     expect(result.stdout.toString().trim()).toBe('ready:0');
   });
 
+  // Spawn-failure case: Node emits 'error' but not 'exit' when the binary
+  // is missing, so listening only for 'exit' hangs `await proc.exited`
+  // forever. The lifecycle promise must resolve on either event.
+  test('Bun.spawn proc.exited resolves on spawn failure (missing binary)', async () => {
+    const result = Bun.spawnSync(['node', '-e', `
+      require('${polyfillPath}');
+      (async () => {
+        const p = Bun.spawn(['this-binary-does-not-exist-zzz-' + Date.now()], {
+          stdio: ['ignore', 'pipe', 'pipe']
+        });
+        const code = await Promise.race([
+          p.exited,
+          new Promise((_, r) => setTimeout(() => r(new Error('timeout')), 3000))
+        ]).catch(() => 'TIMEOUT');
+        console.log('exit:' + code);
+      })();
+    `], { stdout: 'pipe', stderr: 'pipe' });
+    // Anything other than 'TIMEOUT' (and ideally a non-zero number) means the
+    // lifecycle promise resolved on the spawn error.
+    const out = result.stdout.toString().trim();
+    expect(out).not.toBe('exit:TIMEOUT');
+    expect(out).toMatch(/^exit:\d+$/);
+  });
+
   // Regression for the pipe-blocking case: if the child writes more than the
   // OS pipe buffer (~16-64 KB) and the polyfill doesn't drain eagerly, the
   // child blocks in write() and `exit` never fires. 1 MB is well past every