feat(core): CLI QoS hardening — drain, lockfile, structured errors, redaction (PER-7855)

[Shivanshu-07]

Summary

Consolidated PER-7855 — proactive CLI hardening (no incident driving it; YAGNI applies). Three logically separable units packaged as one PR with three commits so the diff stays reviewable while the change history reflects the original phased risk-sequencing.

Commit	Topic	Touches
1️⃣ 36bf4b4e	network refactors + redaction + idle-timeout hint (R4/R5/R6/R7)	core/src/{network,utils}.js
2️⃣ 590f845d	per-port lockfile with stale-lock reclaim (R2)	core/src/{lock,percy}.js (new file)
3️⃣ f3261353	SIGINT/SIGTERM drain + unhandled-rejection redaction + bonus child-tree-kill fix (R1/R3)	cli-command/, cli-exec/, cli-snapshot/, core/src/{server,browser}.js

Commit 1 — network refactors

• R4 Move Network.TIMEOUT from a static class field to a per-instance networkIdleWaitTimeout. Concurrent pages with different env values no longer overwrite each other.
• R5 Export AbortCodes enum (ABORTED, TIMEOUT_NETWORK_IDLE). Throws from Network#send for aborted requests now carry {code, reason} via the existing AbortError class. The consumer at network.js:529 prefers error.code === 'ABORTED'; legacy string-match clauses retained for BC.
• R6 Wrap redactSecrets() around the warn/debug logs in executeDomainValidation so upstream errors that echo response bodies don't leak AWS keys, URL-embedded credentials, etc.
• R7 Append actionable hint to network-idle timeout: Hint: set PERCY_NETWORK_IDLE_WAIT_TIMEOUT to increase the budget, or allowlist slow domains via the discovery config.

Implementation note — the _throwTimeoutError path uses a plain Error with code/reason (not AbortError), because error.name === 'AbortError' is checked at discovery.js:520, percy.js:347, and snapshot.js:472 and would silently swallow the timeout as if it were a deliberate cancel. Only the explicit browser-cancellation path uses AbortError.

Commit 2 — per-port lockfile

• R2 New core/src/lock.js: acquireLock({port}) writes ~/.percy/agent-<port>.lock atomically via wx. Payload {pid, port, startedAt}; mode 0o600 on the file, 0o700 on the parent dir.
• LockHeldError carries {meta, lockPath} so the refusal message can name the live pid + lock path for manual cleanup.
• Stale-lock reclaim via process.kill(pid, 0) liveness probe: ESRCH = dead → reclaim; EPERM = alive-but-foreign → refuse; self-pid → reclaim (we cannot conflict with ourselves).
• Reclaim is unlink + retry-wx, not rename-based: Windows CI is pinned to Node 14 (.github/workflows/windows.yml:15) where fs.renameSync over an existing target is unreliable.
• Percy.start() acquires the lock as the first step inside try {, before any expensive setup; registers process.on('exit') synchronous unlink as last-chance cleanup.
• Percy.stop() releases the lock in the finally block (idempotent).
• Backwards compatibility: when the lock is held, the catch maps LockHeldError to the legacy Percy is already running or the port X is in use message string (downstream tooling may grep for it) AND also log.errors the actionable detail.

Commit 3 — graceful drain + unhandled-rejection redaction

• R1 New module-level shutdownState bag exposed to commands as ctx.shutdown so they can call percy.stop(ctx.shutdown.forced) for graceful-on-first-signal, force-on-second-signal behavior.
• First SIGINT/SIGTERM: log ${signal} received, draining (press Ctrl-C again to force)..., arm 30s drain timer.
• Second signal (or 30s timer): flip forced=true, arm 5s hard-exit safety timer.
• Production exit codes: SIGINT→130, SIGTERM→143 via process.exit only when definition.exitOnError is true; tests with exitOnError: false preserve the legacy clean-resolution.
• R3 Global unhandledRejection / uncaughtException handlers, attached exactly once. Stack trace routed through redactSecrets() so CDP rejections that include serialized page-script bodies, Authorization headers, or cookie strings cannot leak. activeContext.runFailed=true ensures non-zero exit even when the rejection is non-fatal.
• Bonus Fixed the existing POSIX child-tree leak in core/src/browser.js:207. The previous this.process.kill('SIGKILL') targeted only the lead Chromium pid despite detached: true at :266, leaving renderer/utility/zygote children orphaned on every kill. Fix matches Puppeteer / Playwright convention: taskkill /pid <pid> /T /F on Windows, process.kill(-pid, 'SIGKILL') on POSIX (negative-pid signals the process group). Falls back to lead-pid kill on either path's error.
• HTTP server graceful drain: Server.close() becomes async with drainMs (default 5s), uses Node 18.2+ closeIdleConnections/closeAllConnections with Node 14 fallback.

Tests

• 23 net-new specs across the three commits:
  • 6 in core/test/unit/{network,utils}.test.js (SC6 per-instance timeout, R5 AbortCodes shape, SC8 redactSecrets fixtures)
  • 13 in core/test/unit/lock.test.js (SC3 stale reclaim, SC4 live-foreign refusal, SC5 multi-port, EPERM-as-alive, corrupt-payload recovery, mkdir-p, mode bits on POSIX, release idempotency, re-acquire after release)
  • 4 in cli-command/test/shutdown.test.js (SIGINT→130, SIGTERM→143, shutdown.forced transition, redactSecrets path for unhandled rejections)
• Updated core/test/discovery.test.js, cli-command/test/command.test.js, cli-exec/test/exec.test.js for the new "draining" announcement on stderr, the removal of the legacy "Stopping percy..." log on graceful interrupts, and the AbortCodes/idle-hint message changes.
• Test infrastructure: added ~/.percy/agent-* to mockfs $bypass (lock files use real fs); _resetShutdownForTest() exported from @percy/cli-command for spec isolation; module-level shutdown state auto-resets at the start of each runCommandWithContext; try/finally in runCommandWithContext ensures per-run signal listeners are always removed (eliminates pre-existing MaxListenersExceededWarning).

Test run on this branch (sequential, per workspace)

Workspace	Specs	Pass	Fail	Notes
@percy/core	703	676	27	Same 27 pre-existing failures as master baseline (21 install Chromium, 5 runDoctorOnFailure, 1 API server when disabled). All 19 new specs pass.
@percy/cli-command	62	62	0	All 4 new shutdown specs pass; 1 pre-existing test ("handles interrupting generator actions") still passes after assertion update.
@percy/cli-exec	33	33	0	2 tests updated for Phase 3 behavior changes (drain announcement, removed force-stop log).

⚠ Important — running tests: @percy/core and @percy/cli-exec both bind port 5338 via Percy.start(). With Phase 2's lockfile, running these two suites in parallel will fail the second-to-acquire with LockHeldError — that is the lockfile working as designed, refusing concurrent same-port starts across processes. Run the workspace test suites sequentially, or set distinct PERCY_SERVER_PORT per worker if you parallelize CI. Same for any developer running lerna run --parallel test.

(Pre-existing in cli-snapshot/test/file.test.js: 4 failures from a mockfs/dynamic-import() resolver issue unrelated to this PR; identical on master.)

Test plan

• CI green on Linux + macOS + Windows modulo the 27 pre-existing failures
• Manual: trigger network-idle timeout, confirm hint appears in stderr
• Manual: rm -rf ~/.percy/, percy start, kill -9, percy start again — second succeeds via stale-lock reclaim
• Manual: percy start in two terminals on the same port — second refuses with the actionable message naming pid + lock path
• Manual: percy start --port 5338 and percy start --port 5339 concurrently — both succeed
• Manual: percy start, Ctrl-C → drain message + clean exit 130
• Manual: percy start, Ctrl-C, Ctrl-C again → forced exit ≤ 2s, no orphan Chromium (ps -A | grep -i chrom empty)
• Manual: kill -TERM <pid> → exit 143
• Windows manual: console Ctrl+C → drain message + clean exit (SIGINT works on Windows; SIGTERM is documented as best-effort because Windows can't deliver it gracefully)

Risks

Risk	Mitigation
Tests that emit process.emit('SIGINT') and expect empty stderr	Updated to expect the drain announcement
Tests that expect Stopping percy... log on signal interrupt	Updated — graceful drain doesn't force-stop, so that log doesn't fire
process.exit(130) in test mode kills the test runner	Production-only via definition.exitOnError gate
Browser child-tree kill change (POSIX negative-pid) might fail in containers	Fallback to lead-pid SIGKILL preserves at-least-as-good-as-before behavior
Drain hangs at Percy.stop(false)	5s hard-exit safety timer after second signal / 30s drain timeout
Lock file leaks on hard kill (SIGKILL)	Reclaimed on next start via process.kill(pid, 0)
Multi-user host: another user's pid happens to match	Treated as alive (EPERM); user must manually delete the file (path is in the refusal message)
Restricted CI without writable $HOME	acquireLock propagates EACCES via the catch path with an actionable message; future ticket can add tmpdir fallback if real users hit this
Concurrent same-port test workers	Phase 2 refuses with LockHeldError by design; document running tests sequentially or per-worker PERCY_SERVER_PORT
secretPatterns.yml doesn't cover Cookie:/JSESSIONID/custom-auth	SC8 acceptance covers stated categories only; yml augmentation is a separate ticket

Post-Deploy Monitoring & Validation

• Logs to watch (24h–1 week post-merge):
  • Orphaned Chromium reports in #percy-cli Slack — should drop to zero (Phase 3 bonus + R1)
  • Any LockHeldError in build logs — expected to drop after legitimate stale locks self-reclaim once
  • Build logs containing unredacted Authorization: / AKIA* / URL credentials — should drop to zero (R6 + R3)
  • Any [REDACTED] markers in unhandled-rejection logs — confirms the new redaction path is live
  • PERCY_NETWORK_IDLE_WAIT_TIMEOUT related support tickets — should decrease as users hit the new hint
• Validation checks:
  • ls ~/.percy/ after a clean percy start && percy stop — should be empty
  • ps -A | grep -i chrom after a SIGINT'd percy start — must be empty (POSIX) / tasklist | findstr chrome empty (Windows)
• Failure signal(s) / rollback trigger:
  • Reports of stuck builds that never exit on Ctrl-C — drain hang
  • Chromium children accumulating after Ctrl-C
  • "Percy is already running" errors when no Percy is running and ~/.percy/agent-X.lock is present (PID-reuse false positive on long-running hosts)
• Validation window & owner: 1 week post-merge; @shivanshu.si

Origin / Plan

• Origin requirements: docs/brainstorms/2026-04-24-per-7855-cli-qos-hardening-requirements.md
• Plan: docs/plans/2026-04-27-001-feat-per-7855-cli-qos-hardening-plan.md

This PR consolidates the previously-staged drafts: #2196 (Phase 1), #2197 (Phase 2), #2198 (Phase 3) — those will be closed in favor of this single PR.

---

🤖 Generated with Claude Opus 4.7 (1M context, extended thinking) via Claude Code

[amandeepsingh333]

Review: CLI QoS Hardening (PER-7855)

Thorough and well-documented PR. The three-commit split is clean and the PR description is excellent. I have a few findings across the new code — mostly defense-in-depth and a couple of correctness concerns.

Summary of findings:

#	Severity	File	Issue
1	MEDIUM	browser.js	execSync with string interpolation — use execFileSync to avoid shell injection risk
2	MEDIUM	lock.js	Race-loser JSON.parse can crash on partially-written files from a concurrent winner
3	MEDIUM	command.js	process.exit() bypasses the finally block that clears activeContext and removes signal handlers
4	MEDIUM	server.js	Forced-drain setTimeout is never cleared on the happy path, causing a deferred no-op closeAllConnections call
5	MEDIUM	shutdown.test.js	PERCY_EXIT_WITH_ZERO_ON_ERROR env var in the signal-exit path has no test coverage
6	LOW	lock.js	Port 0 passes validation but produces a meaningless lockfile
7	LOW	network.js	Network.TIMEOUT static field removal could break external SDK consumers

Overall the design is solid — the lock reclaim, drain state machine, and process-tree kill are well thought through. The findings above are mostly edge-case hardening. Nice work on the test suite (23 new specs + updated assertions).

[amandeepsingh333]

MEDIUM — shell injection risk with execSync

this.process.pid is always a number from child_process.spawn, so this is safe today, but execSync runs through a shell by default, meaning any future bug where pid isn't a safe integer becomes a command injection vector.

Suggestion: use execFileSync which bypasses the shell entirely:

import { execFileSync } from 'child_process';
// ...
execFileSync('taskkill', ['/pid', String(this.process.pid), '/T', '/F'], { stdio: 'ignore' });

This matches the defense-in-depth standard already applied elsewhere in this PR (e.g., the lockfile port validation).

[Shivanshu-07]

Done in fe20045 — switched to execFileSync('taskkill', ['/pid', String(pid), '/T', '/F']) so the pid is passed via argv (no shell). Thanks for catching this.

[amandeepsingh333]

MEDIUM — JSON.parse in race-loser path can throw SyntaxError

Between the unlinkSync above and this second wx write, a concurrent process could win the race and create a lock file. If that winner is killed mid-write (or writes atomically but the file is still being flushed), readFileSync here gets a truncated/partial JSON payload, and JSON.parse throws a SyntaxError that propagates as an unhandled exception instead of a graceful LockHeldError.

The earlier stale-lock read (around line 118) already has a try/catch around JSON.parse for exactly this case — apply the same treatment here:

if (err.code === 'EEXIST') {
  let winner;
  try {
    winner = JSON.parse(readFileSync(path, 'utf-8'));
  } catch {
    winner = { pid: '?', port, startedAt: 'unknown' };
  }
  throw new LockHeldError(winner, path);
}

[Shivanshu-07]

Done in fe20045 — wrapped JSON.parse in try/catch with a placeholder {pid:'?', port, startedAt:'unknown'} fallback for the LockHeldError, mirroring the earlier stale-lock read. Now a partially-written winner doesn't surface as SyntaxError.

[amandeepsingh333]

LOW — port 0 passes validation but produces a misleading lockfile

n < 0 excludes negatives but allows port 0. Port 0 means "OS picks an ephemeral port" — a lockfile named agent-0.lock would be created, but the actual bound port would be different. Two processes both requesting port 0 would contend on the same lock even though the OS gives them different ports.

If dynamic-port mode is intentionally supported, the lock should be skipped for port 0. If not, consider n <= 0:

if (!Number.isInteger(n) || n <= 0 || n > 65535) {

(The Percy constructor likely defaults to 5338 so this is low-risk, but worth documenting the intent.)

[Shivanshu-07]

Done in fe20045 — tightened to n <= 0 || n > 65535 so port 0 is rejected. Percy doesn't use ephemeral ports today; rejecting it avoids the agent-0.lock contention you described. If we ever want dynamic-port support we can revisit.

[amandeepsingh333]

MEDIUM — process.exit() bypasses the finally block

process.exit() is synchronous and terminates immediately, which means the finally block in runCommandWithContext (which clears activeContext and removes signal handlers) will not run. The lockfile's process.on('exit') handler does fire (synchronous cleanup), so the lock is released. But:

1. In test environments where process.exit is stubbed (as in shutdown.test.js), the stub throws, which does unwind through finally — so tests pass but production behavior differs.
2. activeContext is never cleared, which is fine for a real exit but could matter if process.exit is stubbed to not actually exit.

Consider setting process.exitCode and returning, or restructuring so the finally block runs before the exit:

if (shutdownState.signal && err.signal && definition.exitOnError) {
  let signalCode = shutdownState.signal === 'SIGINT' ? 130 : 143;
  let percyExitWithZeroOnError = process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR === 'true';
  process.exitCode = percyExitWithZeroOnError ? 0 : signalCode;
  return; // let finally block run, then process exits with exitCode
}

Alternatively, if the hard exit is intentional, add a brief comment explaining why it's safe to skip finally here.

[Shivanshu-07]

Done in fe20045 — replaced process.exit(...) with process.exitCode = ...; return; so the surrounding finally block runs (clearing activeContext, removing per-run signal handlers). The unref'd timers don't keep the loop alive, so the process exits cleanly. Tests updated to assert process.exitCode instead of stubbing process.exit.

[amandeepsingh333]

MEDIUM — forced timer is never cleared on the happy path

When closed wins the Promise.race, the setTimeout inside forced still fires after drainMs and calls closeAllConnections() (or iterates #sockets) on an already-closed server. This is likely a no-op, but calling socket.destroy() on already-destroyed sockets could throw in edge cases.

Suggestion: capture and clear the timer:

let forcedTimer;
let forced = new Promise(resolve => {
  forcedTimer = setTimeout(() => {
    if (typeof this.closeAllConnections === 'function') {
      this.closeAllConnections();
    } else {
      this.#sockets.forEach(socket => socket.destroy());
    }
    resolve();
  }, drainMs).unref();
});

await Promise.race([closed, forced]);
clearTimeout(forcedTimer);
await closed;

[Shivanshu-07]

Done in fe20045 — captured the timer handle and clearTimeout(forcedTimer) after the race. The graceful path no longer leaves a deferred no-op call running.

[amandeepsingh333]

MEDIUM — missing test for PERCY_EXIT_WITH_ZERO_ON_ERROR in signal path

The production code at command.js:338 has a PERCY_EXIT_WITH_ZERO_ON_ERROR override that changes the exit code to 0 on signal, but no test covers this branch. If someone refactors the string comparison or moves the env-var read, CI pipelines relying on this behavior would silently break.

Suggestion — add a test case here:

it('exits with 0 on SIGINT when PERCY_EXIT_WITH_ZERO_ON_ERROR is true', async () => {
  process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR = 'true';
  let runner = makeRunner();
  let promise = runner();
  await new Promise(r => setImmediate(r));
  process.emit('SIGINT');
  await promise.catch(() => {});

  expect(exitSpy).toHaveBeenCalledWith(0);
  delete process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR;
});

[Shivanshu-07]

Done in fe20045 — added it('honors PERCY_EXIT_WITH_ZERO_ON_ERROR=true on SIGINT') that sets the env var, emits SIGINT, and asserts process.exitCode === 0. The afterEach also delete process.env.PERCY_EXIT_WITH_ZERO_ON_ERROR so it doesn't leak.

[amandeepsingh333]

LOW — Network.TIMEOUT static field removal may break external consumers

The deleted static TIMEOUT = undefined was used in tests within this repo (Network.TIMEOUT = undefined), which suggests it was part of the semi-public API. External SDK consumers or plugins that read/write Network.TIMEOUT to customize the timeout will silently see their override ignored (the field no longer exists; the per-instance networkIdleWaitTimeout takes its place).

Consider adding a deprecated getter/setter that logs a one-time warning and delegates to a default value:

static get TIMEOUT() {
  // Deprecated: per-instance timeout replaces static field (PER-7855)
  return undefined;
}
static set TIMEOUT(val) {
  logger('core:discovery').warn(
    'Network.TIMEOUT is deprecated; set PERCY_NETWORK_IDLE_WAIT_TIMEOUT env var instead.'
  );
}

Or if the field was truly internal-only, document the breaking change in the changelog.

[Shivanshu-07]

Done in fe20045 — added a static TIMEOUT getter that returns undefined and a setter that logs a one-time logger('core:discovery').warn pointing at PERCY_NETWORK_IDLE_WAIT_TIMEOUT. External consumers see a clear deprecation message instead of silently dropped overrides.