PER-11034: fix(cli-exec): stop percy exec spinning a CPU core while the child runs

packages/cli-exec/src/exec.js

@@ -1,3 +1,4 @@
+import os from 'os';
 import command from '@percy/cli-command';
 import logger from '@percy/logger';
 import start from './start.js';
@@ -160,16 +161,40 @@ async function* spawn(cmd, args, percy) {
       });
     }
 
-    proc.on('error', err => (error = err));
+    // wake the current wait as soon as the child errors or closes. A child killed by a signal
+    // closes with a null code, so report it the way shells do (128 + signal number); `| 0`
+    // keeps an unmapped signal at a non-zero 128 rather than NaN
+    let done = false;
+    let wake;
 
-    proc.on('close', code => {
-      closed = code;
+    proc.on('error', err => {
+      done = true;
+      error = err;
+      wake();
     });
 
-    // run until an event is triggered
+    proc.on('close', (code, signal) => {
+      done = true;
+      closed = code ?? 128 + (os.constants.signals[signal] | 0);
+      wake();
+    });
+
+    // run until an event is triggered. Waiting on the exit event rather than spinning on
+    // setImmediate keeps an idle child from costing a CPU core (percy/cli#2460), and the
+    // periodic wake lets an abort signal interrupt the generator promptly. Each tick waits on
+    // a fresh promise: racing one long-lived exit promise would retain a reaction per tick.
     /* eslint-disable-next-line no-unmodified-loop-condition */
-    while (closed == null && error == null) {
-      yield new Promise(r => setImmediate(r));
+    while (!done) {
+      let timer;
+
+      try {
+        yield new Promise(resolve => {
+          wake = resolve;
+          timer = setTimeout(resolve, 100);
+        });
+      } finally {
+        clearTimeout(timer);
+      }
     }
 
     if (error) throw error;

packages/cli-exec/test/exec.test.js

@@ -385,6 +385,23 @@ describe('percy exec', () => {
     ]));
   });
 
+  it('forwards a shell-style status when the command is killed by a signal', async () => {
+    // the child closes with a null code; this used to keep the wait loop running forever
+    await expectAsync(
+      exec(['--', 'node', '--eval', 'process.kill(process.pid, "SIGKILL")'])
+    ).toBeRejectedWithError(process.platform === 'win32' ? /^EEXIT: \d+$/ : 'EEXIT: 137');
+  });
+
+  it('does not spin the event loop while waiting for the command', async () => {
+    let immediates = spyOn(global, 'setImmediate').and.callThrough();
+    await exec(['--', 'node', '--eval', 'setTimeout(() => {}, 1000)']);
+
+    // the previous wait loop scheduled an immediate on every event loop turn, i.e. tens of
+    // thousands over a one second command (percy/cli#2460). The spy also counts unrelated
+    // callers such as percy startup, so the bound is deliberately loose.
+    expect(immediates.calls.count()).toBeLessThan(1000);
+  });
+
   it('provides the child process with a percy server address env var', async () => {
     let args = ['--no-warnings', '--input-type=module', '--loader=../../scripts/loader.js'];