From bfbdfde5dba548269b4cc3a2445a0c62d51cc6c5 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 13 Oct 2026 18:22:05 +0530 Subject: [PATCH 1/3] fix(cli-exec): wait on the child's exit event instead of spinning on setImmediate `percy exec` polled for the child's close with `yield new Promise(r => setImmediate(r))`, which keeps the event loop busy for the whole run of the wrapped command, so an idle test runner costs Percy roughly one CPU core (percy/cli#2460, PER-11034). Race the child's error/close event against a 100ms timer instead. The command still returns the moment the child exits, and the periodic wake keeps abort signals (SIGINT, SIGTERM) interrupting the generator as before. Co-Authored-By: Claude Opus 5.5 --- packages/cli-exec/src/exec.js | 16 ++++++++++------ packages/cli-exec/test/exec.test.js | 9 +++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/packages/cli-exec/src/exec.js b/packages/cli-exec/src/exec.js index 874bc7b82..911873813 100644 --- a/packages/cli-exec/src/exec.js +++ b/packages/cli-exec/src/exec.js @@ -160,16 +160,20 @@ async function* spawn(cmd, args, percy) { }); } - proc.on('error', err => (error = err)); - - proc.on('close', code => { - closed = code; + // settles as soon as the child errors or closes + let exited = new Promise(resolve => { + proc.on('error', err => resolve(error = err)); + proc.on('close', code => resolve(closed = code)); }); - // run until an event is triggered + // 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), while the + // periodic wake still lets an abort signal interrupt the generator promptly /* eslint-disable-next-line no-unmodified-loop-condition */ while (closed == null && error == null) { - yield new Promise(r => setImmediate(r)); + let timer; + yield Promise.race([exited, new Promise(r => (timer = setTimeout(r, 100)))]); + clearTimeout(timer); } if (error) throw error; diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index b29cac633..69e437b3d 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -385,6 +385,15 @@ describe('percy exec', () => { ])); }); + 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) + 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']; From a14f8db4a09ded25ad0a6bdff9327d31d1d0f505 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Tue, 13 Oct 2026 20:35:37 +0530 Subject: [PATCH 2/3] fix(cli-exec): end the wait when the child is killed by a signal A child terminated by a signal emits `close` with a null code, so a loop keyed on the exit code never ended. On master that hangs `percy exec` forever; with the previous commit it would also spin, because the settled exit promise wins every race. Track completion separately from the exit code and report a signal exit the way shells do (128 + signal number, e.g. 137 for SIGKILL). Addresses CodeRabbit review on #2462. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/cli-exec/src/exec.js | 18 ++++++++++++++---- packages/cli-exec/test/exec.test.js | 7 +++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/cli-exec/src/exec.js b/packages/cli-exec/src/exec.js index 911873813..f2d6b972d 100644 --- a/packages/cli-exec/src/exec.js +++ b/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,17 +161,26 @@ async function* spawn(cmd, args, percy) { }); } - // settles as soon as the child errors or closes + // settles 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) rather than waiting forever + let done = false; let exited = new Promise(resolve => { - proc.on('error', err => resolve(error = err)); - proc.on('close', code => resolve(closed = code)); + proc.on('error', err => { + [done, error] = [true, err]; + resolve(); + }); + + proc.on('close', (code, signal) => { + [done, closed] = [true, code ?? 128 + os.constants.signals[signal]]; + resolve(); + }); }); // 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), while the // periodic wake still lets an abort signal interrupt the generator promptly /* eslint-disable-next-line no-unmodified-loop-condition */ - while (closed == null && error == null) { + while (!done) { let timer; yield Promise.race([exited, new Promise(r => (timer = setTimeout(r, 100)))]); clearTimeout(timer); diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index 69e437b3d..cc8482655 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -385,6 +385,13 @@ 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)']); From 9244cb48b31752d973b68dc09f3cff53ec328718 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Wed, 14 Oct 2026 00:33:00 +0530 Subject: [PATCH 3/3] fix(cli-exec): wait on a fresh promise per tick and harden the exit status Address the Claude Code review on #2462: - Racing the long-lived exit promise against each 100ms timer attached a reaction to it per tick, retained until the child exited (about 62MB over 200k ticks in a standalone measurement). The exit handlers now resolve the current tick's promise directly. - Clear the wake timer in a finally block so an abort at the yield does not leave it behind. - `| 0` keeps a signal missing from os.constants.signals at status 128 instead of NaN. - Plain assignments instead of array destructuring, and a note that the setImmediate test bound is deliberately loose. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/cli-exec/src/exec.js | 43 ++++++++++++++++++----------- packages/cli-exec/test/exec.test.js | 3 +- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/packages/cli-exec/src/exec.js b/packages/cli-exec/src/exec.js index f2d6b972d..879be7647 100644 --- a/packages/cli-exec/src/exec.js +++ b/packages/cli-exec/src/exec.js @@ -161,29 +161,40 @@ async function* spawn(cmd, args, percy) { }); } - // settles 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) rather than waiting forever + // 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 exited = new Promise(resolve => { - proc.on('error', err => { - [done, error] = [true, err]; - resolve(); - }); + let wake; - proc.on('close', (code, signal) => { - [done, closed] = [true, code ?? 128 + os.constants.signals[signal]]; - resolve(); - }); + proc.on('error', err => { + done = true; + error = err; + wake(); + }); + + 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), while the - // periodic wake still lets an abort signal interrupt the generator promptly + // 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 (!done) { let timer; - yield Promise.race([exited, new Promise(r => (timer = setTimeout(r, 100)))]); - clearTimeout(timer); + + try { + yield new Promise(resolve => { + wake = resolve; + timer = setTimeout(resolve, 100); + }); + } finally { + clearTimeout(timer); + } } if (error) throw error; diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index cc8482655..92d341998 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -397,7 +397,8 @@ describe('percy exec', () => { 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) + // 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); });