Settle spawnAsync on close so stdout/stderr are fully read

.bumpy/spawn-async-stdio-race.md

@@ -0,0 +1,10 @@
+---
+varlock: patch
+"@varlock/1password-plugin": patch
+"@varlock/bitwarden-plugin": patch
+"@varlock/keepass-plugin": patch
+"@varlock/pass-plugin": patch
+"@varlock/proton-pass-plugin": patch
+---
+
+Fix CLI calls occasionally losing stdout/stderr when the command exits quickly under load, which could surface as a vague "unknown" error instead of the real one

packages/utils/src/exec-helpers.ts

@@ -11,6 +11,9 @@ export class ExecError extends Error {
   }
 }
 
+/** how long to wait for stdio to drain after the child exits, see below */
+const CLOSE_GRACE_PERIOD_MS = 500;
+
 export function spawnAsyncHelper(
   command: string,
   args: Array<string>,
@@ -36,7 +39,12 @@ export function spawnAsyncHelper(
     childProcess.on('error', (err) => {
       reject(err);
     });
-    childProcess.on('exit', (exitCode, signal) => {
+    let settled = false;
+    let graceTimer: ReturnType<typeof setTimeout> | undefined;
+    const settle = (exitCode: number | null, signal: NodeJS.Signals | null) => {
+      if (settled) return;
+      settled = true;
+      clearTimeout(graceTimer);
       // A child killed by a signal exits with a null code, so only an explicit 0 counts as
       // success. Otherwise whatever partial stdout arrived before the kill would be returned.
       if (exitCode === 0) {
@@ -48,6 +56,19 @@ export function spawnAsyncHelper(
           stderrData || (signal ? `command terminated by ${signal}` : ''),
         ));
       }
+    };
+    // `exit` can fire before stdout/stderr are drained, so settling there could drop output
+    // (eg. an empty stderr on failure). `close` fires once the stdio streams have ended.
+    childProcess.on('close', (exitCode, signal) => settle(exitCode, signal));
+    // But if the child leaves a grandchild (eg. a background daemon) holding the stdio pipes,
+    // `close` never fires. After `exit`, give the streams a short grace period to drain, then
+    // settle with whatever output arrived and stop waiting on the pipes.
+    childProcess.on('exit', (exitCode, signal) => {
+      graceTimer = setTimeout(() => {
+        settle(exitCode, signal);
+        childProcess.stdout?.destroy();
+        childProcess.stderr?.destroy();
+      }, CLOSE_GRACE_PERIOD_MS);
     });
   });
 

packages/utils/test/exec-helpers.test.ts

@@ -37,4 +37,46 @@ describe('spawnAsync', () => {
     expect((result as ExecError).exitCode).not.toBe(0);
     expect((result as ExecError).data).toContain('SIGTERM');
   });
+
+  // `exit` can fire before the stdio pipes are drained, so settling on it used to drop
+  // output under load (eg. bitwarden saw an empty stderr and could not tell the vault was
+  // locked). To make that gap deterministic, the child exits immediately and a grandchild
+  // sharing its stdio writes the output a little later, before the pipes close.
+  const delayedWriteScript = (fd: 1 | 2, text: string, exitCode: number) => `
+    const { spawn } = require('node:child_process');
+    spawn(process.execPath, ['-e', ${JSON.stringify(`setTimeout(() => require('node:fs').writeSync(${fd}, ${JSON.stringify(text)}), 100)`)}], { stdio: 'inherit' }).unref();
+    process.exit(${exitCode});
+  `;
+
+  test('rejection carries stderr written after the child exited but before the pipes closed', async () => {
+    const result = await spawnAsync(
+      process.execPath,
+      ['-e', delayedWriteScript(2, 'Vault is locked.', 1)],
+    ).catch((err) => err);
+
+    expect(result).toBeInstanceOf(ExecError);
+    expect((result as ExecError).exitCode).toBe(1);
+    expect((result as ExecError).data).toBe('Vault is locked.');
+  });
+
+  test('resolves with stdout written after the child exited but before the pipes closed', async () => {
+    const output = await spawnAsync(process.execPath, ['-e', delayedWriteScript(1, 'LATE_OUTPUT', 0)]);
+    expect(output).toBe('LATE_OUTPUT');
+  });
+
+  test('does not hang when a grandchild keeps the stdio pipes open after the child exits', async () => {
+    // the child spawns a detached grandchild that inherits stdout/stderr and outlives it,
+    // like a CLI that starts a background daemon. `close` never fires in that case.
+    const startAt = Date.now();
+    const output = await spawnAsync(process.execPath, [
+      '-e', `
+      const { spawn } = require('node:child_process');
+      const gc = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 10000)'], { stdio: 'inherit', detached: true });
+      gc.unref();
+      process.stdout.write('DONE');
+    `,
+    ]);
+    expect(output).toBe('DONE');
+    expect(Date.now() - startAt).toBeLessThan(5000);
+  });
 });