feat(core): structured abort errors, per-instance network timeout, log redaction (PER-7855 phase 1/3)#2196
Closed
Shivanshu-07 wants to merge 1 commit intomasterfrom
Closed
feat(core): structured abort errors, per-instance network timeout, log redaction (PER-7855 phase 1/3)#2196Shivanshu-07 wants to merge 1 commit intomasterfrom
Shivanshu-07 wants to merge 1 commit intomasterfrom
Conversation
…g redaction (PER-7855)
Phase 1 of PER-7855 CLI QoS hardening — network refactors plus small wins:
R4 — Move `Network.TIMEOUT` from a static class field to a per-instance
`networkIdleWaitTimeout`, derived from PERCY_NETWORK_IDLE_WAIT_TIMEOUT in
the constructor. Concurrent pages with different env values no longer
overwrite each other's timeout.
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` (`utils.js:200, 212-213`). Upstream errors that
echo response bodies no longer leak AWS keys, URL-embedded credentials,
etc., to stderr or build logs.
R7 — Append actionable hint to network-idle timeout message: "Hint: set
PERCY_NETWORK_IDLE_WAIT_TIMEOUT to increase the budget, or allowlist slow
domains via the discovery config."
Implementation note: the deepened plan called for `_throwTimeoutError`
to throw `AbortError`, but `error.name === 'AbortError'` is checked by
`discovery.js:520`, `percy.js:347`, and `snapshot.js:472` — all of which
treat aborts as "snapshot cancelled" rather than as errors. The
network-idle timeout uses a plain `Error` with `code`/`reason`
properties; only the explicit browser-cancellation path uses
`AbortError`.
Tests added: 6 new specs (SC6 per-instance timeout, R5 AbortCodes
shape, SC8 redactSecrets fixtures for AWS keys + URL-embedded creds).
Existing idle-timeout assertions in `discovery.test.js` updated for
the new hint message and removed the `Network.TIMEOUT` reset infra
that the static-field refactor obviates.
Origin: 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
Phase 2 next: per-port lockfile (PER-7855)
Co-Authored-By: Claude Opus 4.7 (1M context, extended thinking) <[email protected]>
This was referenced Apr 27, 2026
feat(core): SIGINT/SIGTERM graceful drain + unhandled-rejection redaction (PER-7855 phase 3/3)
#2198
Closed
Contributor
Author
|
Closing in favor of consolidated PR #2199, which contains all three commits (the same content) so review can happen against a single diff. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of PER-7855 — proactive CLI hardening (no incident driving it; YAGNI applies). Network refactors plus three small wins, isolated to
core/src/network.js+core/src/utils.jsfor low-risk first-PR.Network.TIMEOUTstatic class field → per-instancenetworkIdleWaitTimeout. Concurrent pages with different env values no longer overwrite each other.AbortCodesenum (ABORTED,TIMEOUT_NETWORK_IDLE). Aborted-request throw atnetwork.js:138carries{code, reason}via the existingAbortErrorclass. The consumer at:529preferserror.code === 'ABORTED'; legacy string-match clauses retained for BC.redactSecrets()wraps the warn/debug logs inexecuteDomainValidation(utils.js:200, 212-213). Upstream errors that echo response bodies no longer leak AWS keys, URL-embedded credentials, etc., to stderr or build logs.Implementation Note
The deepened plan called for `_throwTimeoutError` to throw `AbortError`, but that broke a chain of consumers — `error.name === 'AbortError'` is checked at `discovery.js:520`, `percy.js:347`, and `snapshot.js:472`, all of which treat aborts as "snapshot cancelled" rather than as errors. The network-idle timeout uses a plain `Error` with `code`/`reason` properties; only the explicit browser-cancellation path uses `AbortError`. This learning is captured in the plan (
docs/plans/2026-04-27-001-feat-per-7855-cli-qos-hardening-plan.md).Tests
Test run on this branch: 690 specs, 28 pre-existing failures (21 `Unit / Install Chromium` with `TypeError: Cannot read properties of undefined (reading 'set')`, 5 `runDoctorOnFailure`, 1 `API Server when the server is disabled`, 1 flaky `Snapshot with browsers`). None touch any file in this PR. Confirmed pre-existing on master via stash + run.
Test plan
Unit / Networktests pass (4 new specs)Unit / Utils > redactSecrets > SC8fixtures pass (2 new specs)Discovery > idle timeouttests pass with the hintnetwork.js:529swallow pathPost-Deploy Monitoring & Validation
Origin / Plan
🤖 Generated with Claude Opus 4.7 (1M context, extended thinking) via Claude Code