ci(browser): load the running-tests reporter by absolute path

A config imported from another directory (the workflow-artifacts test copies
it to a temp root) resolved './running-tests-reporter.mjs' against that root
under CI and failed to load it. Resolve it from this module instead.

.github/workflows/viewerjs-check.yml

@@ -27,6 +27,15 @@ concurrency:
 # coverage shards 7m34s/6m04s/5m22s (tests 6m38s/4m53s/4m04s), merge 54s, firefox 7m57s/5m21s (tests
 # 7m08s/4m34s), webkit 8m16s/5m03s (tests 7m26s/3m58s); a cache miss adds up to 5m to npm ci, which
 # the 20-minute limits absorb.
+# Hangs (#425): a job killed at its limit keeps no log and skips its upload steps, so each browser
+# test step has its own timeout-minutes (18) below the job's 20. At the step limit, SIGTERM makes
+# tools/vendor/running-tests-reporter.mjs print the tests still running and hand over to Playwright's
+# interrupt, which writes the blob report; the always() uploads then run.
+# Measured on main (runs 40725289094, 40719375442, 40700439399): setup before the test step <= 52s,
+# uploads <= 35s; test steps up to 14m42s (webkit shard 1/2) and 13m05s for coverage (shard 1/3:
+# 12m53s, node units and the viewer suite before 11.8m of workflows), so coverage is the tight one
+# in its 18 minutes. This holds only when the npm cache hits: a cold cache adds up to 5m to npm ci,
+# and then the job limit can still end the job before the step limit does.
 jobs:
   frontend-coverage:
     name: frontend coverage (shard ${{ matrix.shard }}/3)
@@ -79,6 +88,7 @@ jobs:
       - if: matrix.shard == 3
         run: npm run --prefix tools/vendor test
       - name: Page workflows with coverage, shard ${{ matrix.shard }}/3
+        timeout-minutes: 18
         run: npm run --prefix tools/vendor test:coverage:shard -- ${{ matrix.shard }}/3
       - name: Upload raw coverage
         uses: actions/upload-artifact@v7
@@ -87,6 +97,7 @@ jobs:
           path: coverage/js/raw/
           if-no-files-found: error
           retention-days: 3
+      # Traces and the blob report (test-results/blob/) of a failed, timed-out or cancelled step.
       - name: Upload Chromium and touch traces
         if: always()
         uses: actions/upload-artifact@v7
@@ -211,7 +222,9 @@ jobs:
           key: npm-${{ runner.os }}-playwright-v1.64.1-${{ hashFiles('tools/vendor/package-lock.json') }}
       - run: npm ci --prefix tools/vendor --ignore-scripts
       - name: Page workflows in ${{ matrix.browser }}, shard ${{ matrix.shard }}/2
+        timeout-minutes: 18
         run: npm run --prefix tools/vendor test:workflows -- --project ${{ matrix.browser }} --shard ${{ matrix.shard }}/2
+      # Traces and the blob report (test-results/blob/) of a failed, timed-out or cancelled step.
       - name: Upload browser traces
         if: always()
         uses: actions/upload-artifact@v7

TESTING.md

@@ -262,6 +262,31 @@ watchdog dump, is a failure like any other: read the dump, find the blocking
 call, fix it. Never raise a limit or rerun to get green; a limit changes only
 with new measurements written next to it.
 
+Browser jobs (#425): GitHub keeps no log of a job killed at its limit and skips
+its upload steps, so the test step in each browser job in `viewerjs-check.yml`
+has its own `timeout-minutes` (18, below the job's 20). At the step limit
+GitHub sends SIGTERM, which would end Playwright without any report; under `CI`
+`tools/vendor/running-tests-reporter.mjs` catches the first one, prints
+`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/`. 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
+`npm ci`, and the job limit can then still win), and coverage shard 1 (12m53s
+measured) is the tight one in its 18-minute step. Local runs keep the `list`
+reporter only; reproduce with `CI=1 timeout -s TERM 40 npm run --prefix
+tools/vendor test:workflows -- --project=firefox`.
+
 ### After a push
 
 Follow the run for the commit you pushed, never an older one:

tests/vendor/browser-server-fixture.test.mjs

@@ -2,12 +2,14 @@ import test from 'node:test';
 import assert from 'node:assert/strict';
 import { createServer } from 'node:net';
 import { spawn } from 'node:child_process';
-import { once } from 'node:events';
+import { EventEmitter, once } from 'node:events';
 import { readFileSync } from 'node:fs';
+import { isAbsolute } from 'node:path';
 import { fileURLToPath, pathToFileURL } from 'node:url';
 import { setTimeout as delay } from 'node:timers/promises';
 import { startBrowserServer, createServerSupervisor, attachServerStderr, forwardPrefixedLines, SLOW_RESET_PREFIX } from '../../tools/vendor/start-browser-server.mjs';
-import { workflowWorkers } from '../../tools/vendor/test-workers.mjs';
+import { reporters, workflowWorkers } from '../../tools/vendor/test-workers.mjs';
+import RunningTestsReporter from '../../tools/vendor/running-tests-reporter.mjs';
 
 const fakeServer = fileURLToPath(new URL('./fake-browser-server.mjs', import.meta.url));
 const fake = (...extra) => ({ command: process.execPath, args: port => [fakeServer, '--port', String(port), ...extra] });
@@ -255,3 +257,57 @@ test('worker count follows PM_TEST_WORKERS, then one in CI, then a quarter of th
     assert.throws(() => workflowWorkers({ env: { PM_TEST_WORKERS: 'many' }, cpus: 8 }), /PM_TEST_WORKERS must be a positive integer/);
     assert.throws(() => workflowWorkers({ env: { PM_TEST_WORKERS: '0' }, cpus: 8 }), /PM_TEST_WORKERS must be a positive integer/);
 });
+
+test('CI adds the running-tests reporter and a blob report under test-results/blob/, which the trace upload covers; local runs keep list', () => {
+    assert.equal(reporters({ env: {}, name: 'workflows' }), 'list');
+    const [list, running, blob] = reporters({ env: { CI: '1' }, name: 'workflows' });
+    assert.deepEqual(list, ['list']);
+    // Absolute: a config imported from another directory (workflow-artifacts.test.mjs) still finds it.
+    assert.ok(isAbsolute(running[0]) && running[0].endsWith('running-tests-reporter.mjs'), running[0]);
+    assert.deepEqual(blob, ['blob', { outputDir: './test-results/blob/workflows' }]);
+});
+
+function fakeHost() {
+    const host = new EventEmitter();
+    Object.assign(host, { pid: 4242, signals: [], cwd: () => '/repo/tools/vendor' });
+    host.kill = (pid, signal) => host.signals.push([pid, signal]);
+    return host;
+}
+
+const spec = (id, title, line) => ({ id, title, location: { file: '/repo/tests/browser/a.spec.mjs', line },
+    parent: { project: () => ({ name: 'firefox' }) } });
+
+test('the running-tests reporter names only the tests that never ended, relative to rootDir', () => {
+    let clock = 1_000;
+    const output = [];
+    const host = fakeHost();
+    const reporter = new RunningTestsReporter({ now: () => clock, write: text => output.push(text), host });
+    reporter.onBegin({ rootDir: '/repo/tests/browser' });
+    const done = spec('1', 'finishes', 10);
+    const stuck = spec('2', 'hangs on a fetch', 20);
+    reporter.onTestBegin(done, { retry: 0 });
+    reporter.onTestBegin(stuck, { retry: 0 });
+    clock += 500;
+    reporter.onTestEnd(done, { retry: 0 });
+    clock += 41_500;
+    reporter.onEnd({ status: 'interrupted' });
+    assert.equal(output.join(''), '\nStill running when the run ended (interrupted): 1\n'
+        + '  [firefox] › a.spec.mjs:20 › hangs on a fetch (running 42.0s)\n');
+    assert.equal(host.listenerCount('SIGTERM'), 0, 'the run is over: SIGTERM gets its default again');
+    output.length = 0;
+    reporter.onTestEnd(stuck, { retry: 0 });
+    reporter.onEnd({ status: 'passed' });
+    assert.deepEqual(output, [], 'a run where every test ended prints nothing');
+});
+
+test('on the first SIGTERM the reporter prints the running tests and hands over to Playwright\'s SIGINT', () => {
+    const output = [];
+    const host = fakeHost();
+    const reporter = new RunningTestsReporter({ now: () => 0, write: text => output.push(text), host });
+    reporter.onBegin({ rootDir: '/repo/tests/browser' });
+    reporter.onTestBegin(spec('2', 'hangs on a fetch', 20), { retry: 0 });
+    host.emit('SIGTERM');
+    assert.match(output.join(''), /Still running when the run ended \(SIGTERM\): 1\n {2}\[firefox\] › a\.spec\.mjs:20 › hangs on a fetch/);
+    assert.deepEqual(host.signals, [[4242, 'SIGINT']], 'Playwright\'s own interrupt ends the run and writes the blob');
+    assert.equal(host.listenerCount('SIGTERM'), 0, 'a second SIGTERM kills the process as usual');
+});

tests/vendor/ci-workflow.test.mjs

@@ -247,6 +247,26 @@ test('browser job timeouts keep a real margin over measured runs', () => {
     }
 });
 
+// A hang (#425) must end inside the test step, before the job limit, so the log and the always()
+// uploads survive: the step's timeout-minutes < the job's.
+const BROWSER_TEST_STEP = /npm run --prefix tools\/vendor test:(workflows|coverage)/;
+
+test('each browser test step stops itself before its job limit and its traces upload anyway', () => {
+    for (const id of browserJobIds) {
+        const body = allJobs.get(id);
+        const jobMinutes = Number(/^ {4}timeout-minutes: (\d+)$/m.exec(body)[1]);
+        const jobSteps = steps(body);
+        const testAt = jobSteps.findIndex(step => BROWSER_TEST_STEP.test(step));
+        const testStep = jobSteps[testAt];
+        const stepMinutes = Number(/^ {8}timeout-minutes: (\d+)$/m.exec(testStep)?.[1]);
+        assert.ok(stepMinutes > 0, `${id}: the test step sets its own timeout-minutes`);
+        assert.ok(jobMinutes - stepMinutes >= 2, `${id}: the step limit leaves time for setup and the uploads`);
+        const upload = jobSteps.slice(testAt + 1).find(step => /^ {10}path: tools\/vendor\/test-results\/$/m.test(step));
+        assert.ok(upload, `${id} uploads tools/vendor/test-results/ (traces and blob report) after the tests`);
+        assert.match(upload, /^ {8}if: always\(\)$/m, `${id} uploads them when the step failed or timed out`);
+    }
+});
+
 // Every job is bounded (#350): a hang ends at a measured limit instead of GitHub's 6-hour default.
 const GATING_WORKFLOWS = ['test.yml', 'code-quality.yml', 'viewerjs-check.yml'];
 const BOUNDED_WORKFLOWS = [...GATING_WORKFLOWS, 'portability.yml'];

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);
+});

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']);
         },

tools/vendor/playwright.config.mjs

@@ -1,4 +1,5 @@
 import { defineConfig } from '@playwright/test';
+import { reporters } from './test-workers.mjs';
 
 export default defineConfig({
     testDir: './tests',
@@ -8,7 +9,7 @@ export default defineConfig({
     workers: 2,
     forbidOnly: Boolean(process.env.CI),
     retries: process.env.CI ? 1 : 0,
-    reporter: 'list',
+    reporter: reporters({ name: 'viewer' }),
     use: { browserName: 'chromium', headless: true, trace: 'retain-on-failure' },
     projects: [
         { name: 'desktop', use: { viewport: { width: 1280, height: 800 } } },

tools/vendor/playwright.workflows.config.mjs

@@ -1,5 +1,5 @@
 import { defineConfig, devices } from '@playwright/test';
-import { workflowWorkers } from './test-workers.mjs';
+import { reporters, workflowWorkers } from './test-workers.mjs';
 
 // No global webServer: tests/browser/fixtures.mjs starts one server per worker
 // on an OS-chosen port. Files run in parallel; tests inside a file keep their order.
@@ -11,7 +11,7 @@ export default defineConfig({
     fullyParallel: false,
     forbidOnly: Boolean(process.env.CI),
     retries: 0,
-    reporter: 'list',
+    reporter: reporters({ name: 'workflows' }),
     use: { trace: 'retain-on-failure', screenshot: 'only-on-failure' },
     projects: [
         { name: 'chromium', outputDir: './test-results/workflows/chromium', use: { browserName: 'chromium' } },

tools/vendor/running-tests-reporter.mjs

@@ -0,0 +1,58 @@
+import { writeSync } from 'node:fs';
+import { relative } from 'node:path';
+
+// Names the tests still running when a run is cut off (#425). A browser test step that reaches its
+// timeout-minutes gets SIGTERM, which ends Playwright at once: no reporter's onEnd runs, so neither
+// `list` nor the blob report says what was running. On the first SIGTERM this reporter prints every
+// started test that has not ended, then turns the signal into SIGINT, Playwright's own interrupt,
+// which stops the workers, ends the run as interrupted (non-zero exit) and writes the blob report.
+// A second SIGTERM gets the default behaviour: the process dies.
+export default class RunningTestsReporter {
+    constructor({ now = () => Date.now(), write = text => writeSync(1, text), host = process } = {}) {
+        this.now = now;
+        this.write = write;
+        this.host = host;
+        this.rootDir = host.cwd();
+        this.running = new Map();
+        this.onSigterm = () => {
+            this.host.removeListener('SIGTERM', this.onSigterm);
+            this.report('SIGTERM');
+            this.host.kill(this.host.pid, 'SIGINT');
+        };
+    }
+
+    printsToStdio() {
+        return false;
+    }
+
+    onBegin(config) {
+        this.rootDir = config.rootDir;
+        this.host.on('SIGTERM', this.onSigterm);
+    }
+
+    onTestBegin(test, result) {
+        this.running.set(`${test.id}#${result.retry}`, { test, startedAt: this.now() });
+    }
+
+    onTestEnd(test, result) {
+        this.running.delete(`${test.id}#${result.retry}`);
+    }
+
+    describe({ test, startedAt }) {
+        const project = test.parent?.project()?.name;
+        const where = `${relative(this.rootDir, test.location.file)}:${test.location.line}`;
+        const seconds = ((this.now() - startedAt) / 1000).toFixed(1);
+        return `${project ? `[${project}] › ` : ''}${where} › ${test.title} (running ${seconds}s)`;
+    }
+
+    report(reason) {
+        if (!this.running.size) return;
+        const lines = [...this.running.values()].map(entry => `  ${this.describe(entry)}`);
+        this.write(`\nStill running when the run ended (${reason}): ${this.running.size}\n${lines.join('\n')}\n`);
+    }
+
+    onEnd(result) {
+        this.host.removeListener('SIGTERM', this.onSigterm);
+        this.report(result.status);
+    }
+}

tools/vendor/test-workers.mjs

@@ -1,4 +1,8 @@
 import { availableParallelism } from 'node:os';
+import { fileURLToPath } from 'node:url';
+
+// Absolute, so a config that imports this one from another directory still finds it.
+const RUNNING_TESTS_REPORTER = fileURLToPath(new URL('./running-tests-reporter.mjs', import.meta.url));
 
 const MAX_LOCAL_WORKERS = 8;
 const CPUS_PER_WORKER = 4;
@@ -16,3 +20,10 @@ export function workflowWorkers({ env = process.env, cpus = availableParallelism
     if (env.CI) return 1;
     return Math.max(1, Math.min(MAX_LOCAL_WORKERS, Math.floor(cpus / CPUS_PER_WORKER)));
 }
+
+// Under CI, running-tests-reporter.mjs names the tests a step timeout's signal cut off (`list`
+// cannot: they never end), and a blob report beside the traces is uploaded with them.
+export function reporters({ env = process.env, name } = {}) {
+    if (!env.CI) return 'list';
+    return [['list'], [RUNNING_TESTS_REPORTER], ['blob', { outputDir: `./test-results/blob/${name}` }]];
+}