From 216267348d7b10185cbf4098ef0cf408f911c086 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Wed, 28 Oct 2026 10:47:03 -0700 Subject: [PATCH] ci(browser): forward SIGTERM and SIGINT from frontend-coverage.mjs to its child In the coverage job npm signals only frontend-coverage.mjs. It blocked in spawnSync with no listener, so Playwright never saw a step timeout. It now runs each step's child with async spawn, in its own process group, and forwards SIGTERM and SIGINT to the running child. Step order, exit codes and the "Command failed (N)" message are unchanged. Locally, in the pinned image, `timeout -s TERM` on test:coverage printed the still-running tests and wrote both blob reports. TESTING.md records that a SIGTERM to npm alone stops at npm's `sh -c`, and that delivery through `docker exec` in CI is not yet observed. Refs #425 --- TESTING.md | 10 ++++- tests/vendor/frontend-coverage-cli.test.mjs | 49 ++++++++++++++++++++- tools/vendor/frontend-coverage.mjs | 35 +++++++++++---- 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/TESTING.md b/TESTING.md index 801a9377..a46e1a93 100644 --- a/TESTING.md +++ b/TESTING.md @@ -270,7 +270,15 @@ GitHub sends SIGTERM, which would end Playwright without any report; under `CI` `Still running when the run ended (SIGTERM): N` and one line per unfinished test (path relative to the suite's `rootDir`), then sends Playwright its own SIGINT, so the workers stop, the run ends interrupted and the blob report is -written to `tools/vendor/test-results/blob/`. That directory and the traces +written to `tools/vendor/test-results/blob/`. In the coverage job npm signals +only `frontend-coverage.mjs`, which forwards SIGTERM and SIGINT to the child it +is running (started in its own process group, so a group-wide signal still +arrives once). Delivery is proven locally in the pinned image for a SIGTERM to +the whole process group (`timeout -s TERM`). A SIGTERM to npm alone +(`timeout --foreground`) reaches nothing: npm forwards it to the `sh -c` that +runs the script, and that shell exits without passing it on. Delivery in CI, +where the step runs through `docker exec`, is not yet observed; the next real +step timeout will show it. That directory and the traces upload with `if: always()`; `npx playwright merge-reports` opens the blob. Each test keeps the 30 s default and there is no `globalTimeout`. Two limits of this: it assumes the npm cache hits (a cold cache adds up to 5 minutes to diff --git a/tests/vendor/frontend-coverage-cli.test.mjs b/tests/vendor/frontend-coverage-cli.test.mjs index 8243181b..d926f72c 100644 --- a/tests/vendor/frontend-coverage-cli.test.mjs +++ b/tests/vendor/frontend-coverage-cli.test.mjs @@ -1,9 +1,11 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { EventEmitter } from 'node:events'; +import { setTimeout as delay } from 'node:timers/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { plan, runCoverage } from '../../tools/vendor/frontend-coverage.mjs'; +import { plan, runCoverage, runNode } from '../../tools/vendor/frontend-coverage.mjs'; const steps = argv => plan(argv).map(({ step, ...options }) => (Object.keys(options).length ? { step, ...options } : step)); @@ -91,3 +93,46 @@ test('shard 1 runs the node units into the shared raw dir, then the viewer suite assert.ok(calls[1].args.includes(join(paths.tools, 'playwright.config.mjs'))); assert.deepEqual(calls[2].args.slice(-2), ['--shard', '1/3']); }); + +// #425: npm and a CI step timeout signal only frontend-coverage.mjs; its running child must get it. +function hostWithSignals() { + return new EventEmitter(); +} + +test('runNode resolves on exit 0, rejects with the exit code otherwise and leaves no signal listener', async () => { + const host = hostWithSignals(); + await runNode(['-e', 'process.exit(0)'], {}, { host }); + await assert.rejects(runNode(['-e', 'process.exit(3)'], {}, { host }), /Command failed \(3\): -e process\.exit\(3\)/); + assert.equal(host.listenerCount('SIGTERM') + host.listenerCount('SIGINT'), 0); +}); + +test('runNode forwards SIGTERM to the running child, which can report before it exits', async t => { + const root = mkdtempSync(join(tmpdir(), 'pm-coverage-signal-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const ready = join(root, 'ready'); + const seen = join(root, 'seen'); + const child = `const fs = require('node:fs'); +process.on('SIGTERM', () => { fs.writeFileSync(${JSON.stringify(seen)}, 'SIGTERM'); process.exit(7); }); +fs.writeFileSync(${JSON.stringify(ready)}, ''); setInterval(() => {}, 1000);`; + const host = hostWithSignals(); + const running = runNode(['-e', child], {}, { host }); + while (!existsSync(ready)) await delay(20); + host.emit('SIGTERM'); + await assert.rejects(running, /Command failed \(7\)/); + assert.equal(readFileSync(seen, 'utf8'), 'SIGTERM', 'the child handled the forwarded signal'); + assert.equal(host.listenerCount('SIGTERM'), 0); +}); + +test('runNode forwards SIGINT and starts the child in its own process group', async () => { + const host = hostWithSignals(); + const kills = []; + let options; + const fake = new EventEmitter(); + fake.kill = signal => { kills.push(signal); queueMicrotask(() => fake.emit('exit', null, signal)); }; + const running = runNode(['x.mjs'], {}, { host, spawnChild: (command, args, spawnOptions) => { options = spawnOptions; return fake; } }); + assert.equal(options.detached, true, 'a group-wide signal reaches the child only through forwarding, once'); + host.emit('SIGINT'); + await assert.rejects(running, /Command failed \(null\): x\.mjs/); + assert.deepEqual(kills, ['SIGINT']); + assert.equal(host.listenerCount('SIGINT'), 0); +}); diff --git a/tools/vendor/frontend-coverage.mjs b/tools/vendor/frontend-coverage.mjs index 30de4152..3ade3bb5 100644 --- a/tools/vendor/frontend-coverage.mjs +++ b/tools/vendor/frontend-coverage.mjs @@ -1,7 +1,7 @@ import { existsSync, readFileSync, readdirSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join, relative } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; -import { spawnSync } from 'node:child_process'; +import { spawn } from 'node:child_process'; import { repositoryRoot } from './vendor-viewerjs.mjs'; import { discoverSources, checkCoverage } from '@comfyassets/toolkit/testing/coverage'; @@ -10,10 +10,27 @@ const metrics = ['lines', 'branches', 'functions', 'statements']; const SHARD = /^([1-9]\d*)\/([1-9]\d*)$/; const USAGE = 'usage: frontend-coverage.mjs [--update-baseline] | --shard I/N | --merge-only [--update-baseline]'; -function runNode(args, env = {}) { - const result = spawnSync(process.execPath, args, { cwd: repositoryRoot, stdio: 'inherit', - env: { ...process.env, ...env } }); - if (result.status !== 0) throw new Error(`Command failed (${result.status}): ${args.join(' ')}`); +const FORWARDED_SIGNALS = ['SIGTERM', 'SIGINT']; + +// Runs one step's node child and waits for it. npm and a CI step timeout signal only this process, +// so SIGTERM and SIGINT are forwarded to the running child (#425): Playwright then reports the tests +// still running and writes its blob. The child gets its own process group, so a signal sent to the +// whole group (Ctrl-C, `timeout`) still reaches it exactly once, through this forwarding. +export function runNode(args, env = {}, { spawnChild = spawn, host = process } = {}) { + return new Promise((resolve, reject) => { + const child = spawnChild(process.execPath, args, { cwd: repositoryRoot, stdio: 'inherit', + env: { ...process.env, ...env }, detached: true }); + const forward = signal => () => child.kill(signal); + const handlers = FORWARDED_SIGNALS.map(signal => [signal, forward(signal)]); + for (const [signal, handler] of handlers) host.on(signal, handler); + const done = () => { for (const [signal, handler] of handlers) host.removeListener(signal, handler); }; + child.once('error', error => { done(); reject(error); }); + child.once('exit', code => { + done(); + if (code === 0) resolve(); + else reject(new Error(`Command failed (${code}): ${args.join(' ')}`)); + }); + }); } function defaultPaths() { @@ -112,17 +129,17 @@ export async function runCoverage({ argv = process.argv.slice(2), run = runNode, 'node-units'() { const tests = readdirSync(join(root, 'tests/js')).filter(file => file.endsWith('.test.js')) .map(file => `tests/js/${file}`); - run(['--test', ...tests], { NODE_V8_COVERAGE: raw }); + return run(['--test', ...tests], { NODE_V8_COVERAGE: raw }); }, viewer() { - run([playwright, 'test', '--config', join(tools, 'playwright.config.mjs')]); + return run([playwright, 'test', '--config', join(tools, 'playwright.config.mjs')]); }, workflows({ shard }) { - run([playwright, 'test', '--config', join(tools, 'playwright.workflows.config.mjs'), + return run([playwright, 'test', '--config', join(tools, 'playwright.workflows.config.mjs'), '--project', 'chromium', '--project', 'touch', ...(shard ? ['--shard', shard] : [])], { PM_JS_COVERAGE: raw }); }, report() { - run([join(tools, 'node_modules/c8/bin/c8.js'), 'report', '--all', '--src', sourceRoot, + return run([join(tools, 'node_modules/c8/bin/c8.js'), 'report', '--all', '--src', sourceRoot, '--include', 'web/js/**/*.js', '--include', 'web/js/**/*.mjs', '--temp-directory', raw, '--reports-dir', reportRoot, '--reporter', 'text', '--reporter', 'json-summary', '--reporter', 'html']); },