Bound the npm audit retry budget by wall clock

The retry hardening added in #1885 bounded attempts but not time. npm's own
fetch-timeout defaults to five minutes and it retries internally, so three
"attempts" against a hanging advisory endpoint ran for 10m56s on job
100986651307, and a second audit step added 3m36s. The Frontend job was
cancelled 31s into type-check with all 1183 test files already passing, and a
cancelled job reports as a failed required check, so a green run blocked every
pull request. #1888 raised the job timeout to 40 minutes to unblock delivery;
this decides the policy instead.

Each attempt now runs under a hard wall-clock bound and the sequence stops at
a total deadline (60s and 240s by default). npm's internal retry loop is
disabled in favour of this one, since it was the hidden multiplier. The bound
is enforced by a watchdog subshell rather than timeout(1), which is not
present on every developer machine.

What happens when the endpoint stays unreachable is unchanged, because that
split was already right: the run fails when the change touches the dependency
graph and the answer is genuinely unknown, and warns without failing when it
does not, because the graph is then identical to a base commit that already
produced a passing answer. Any advisory at any severity still fails.

Also drops the production-only audit from the per-pull-request path. It audits
a subset of the same packages, so it reports a subset of the same advisories,
and because the complete audit fails the job on any finding, the production
step could only ever execute in the cases where it was already guaranteed
clean. The dev-versus-production split still runs for every npm workspace in
the scheduled security-scan job, where it informs rather than blocks delivery,
and Dependabot security updates remain the route for advisories published
against unchanged dependencies.

With the audit bounded to 4 minutes against an ~11 minute baseline, the job
timeout returns to 30: a stalled endpoint should surface as a warning, not be
absorbed by a budget large enough to hide it.
This commit is contained in:
rcourtman
2026-09-04 12:13:31 +01:00
parent 6a2efd8170
commit 5434868bd0
5 changed files with 205 additions and 24 deletions
+13 -10
View File
@@ -121,10 +121,11 @@ jobs:
needs: changes
if: needs.changes.outputs.code == 'true'
runs-on: ubuntu-24.04
# The two npm audit steps retry through registry outages (they burned
# 14m34s on 2026-09-04), which no longer fits a 25 minute budget: the job
# was cancelled mid type-check with all 1183 test files already passing.
timeout-minutes: 40
# Everything except the audit runs in ~11m. The audit is now bounded to a
# 4 minute wall-clock budget, so 30 leaves real headroom while staying a
# meaningful ceiling: an unbounded 40 would have absorbed the 2026-09-04
# stall instead of reporting it.
timeout-minutes: 30
env:
FRONTEND_DIR: frontend-modern
@@ -148,18 +149,20 @@ jobs:
# npm audit exits 1 both for a real advisory and for an unreachable
# advisory endpoint. The runner keeps the advisory verdict exactly as
# strict and only retries the endpoint being down; see the script header.
#
# Only the complete graph is audited here. `--omit=dev` audits a subset
# of these packages, so it can only report a subset of these advisories;
# and because the complete audit fails the job on any finding, the
# production step could only ever run in the cases where it was already
# guaranteed clean. The dev-versus-production split is still reported
# for every workspace by the scheduled npm-audit job in
# security-scan.yml, where it informs rather than blocks delivery.
- name: Audit complete frontend dependency graph
working-directory: frontend-modern
env:
NPM_AUDIT_REQUIRE_RESULT: ${{ needs.changes.outputs.frontend_deps }}
run: bash "$GITHUB_WORKSPACE/scripts/npm-audit-retry.sh" all
- name: Audit production frontend dependencies
working-directory: frontend-modern
env:
NPM_AUDIT_REQUIRE_RESULT: ${{ needs.changes.outputs.frontend_deps }}
run: bash "$GITHUB_WORKSPACE/scripts/npm-audit-retry.sh" production
# Whole-tree, not staged-only: the pre-commit formatter only ever sees
# staged files, so drift in untouched files is invisible to it. This is
# the backstop that keeps `make format` a no-op on a clean tree.
@@ -4002,26 +4002,44 @@ resolved package version and integrity that the release build will actually
consume.
Frontend dependency-security changes use their own proof route rather than
borrowing the local dev-runtime orchestration tests. The canonical
`.github/workflows/build-and-test.yml` frontend job must run both the complete
`npm audit` and the production-only `npm audit --omit=dev` after a clean
install, through `scripts/npm-audit-retry.sh`. That runner exists because
`.github/workflows/build-and-test.yml` frontend job must run the complete
`npm audit` after a clean install, through `scripts/npm-audit-retry.sh`. The
production-only `npm audit --omit=dev` is deliberately not on that
per-pull-request path: it audits a subset of the same packages, so it can only
report a subset of the same advisories, and because the complete audit fails
the job on any finding, the production step could only ever execute in the
cases where it was already guaranteed clean. The dev-versus-production split
is reported instead by the scheduled `npm-audit` job in
`.github/workflows/security-scan.yml`, which covers every npm workspace and
informs rather than blocks delivery. That runner exists because
`npm audit` exits non-zero both for a real advisory and for an unreachable
advisory endpoint: on 2026-09-03 registry.npmjs.org returned 503s and timeouts
for over an hour and no pull request could land, including changes that touch
no JavaScript. It separates the two and nothing else. A conclusive result is
acted on immediately and any vulnerability at any severity still fails, so a
severity threshold must never be introduced; only an unreachable endpoint is
retried. When retries are exhausted, the run fails if the change touches
retried. Retrying is bounded by wall clock and not by attempt count alone,
because npm's own `fetch-timeout` defaults to five minutes and it retries
internally: on 2026-09-04 three attempts against a hanging endpoint ran for
10m56s and cancelled the Frontend job at its own timeout with every test
already passing, so a green run was reported as a failed required check. Each
attempt is therefore bounded, npm's internal retry loop is disabled in favour
of the runner's own, and the sequence stops at a total deadline. That budget
must stay well inside the job timeout; it may never be raised to the point
where an unreachable endpoint can consume the job. When retries are exhausted,
by attempt count or by budget, the run fails if the change touches
`frontend-modern/package.json`, `frontend-modern/package-lock.json`, or the
runner itself, because then the answer is genuinely unknown and the runner may
never be relaxed under cover of its own tolerant mode, and warns without
failing when it does not, because the dependency graph is then identical to the base commit that
already produced a passing answer. Advisories published later against
unchanged dependencies are the responsibility of Dependabot security updates,
not of a per-pull-request audit. `scripts/tests/test-npm-audit-retry.sh` pins
unchanged dependencies are the responsibility of Dependabot security updates
and the scheduled scan, not of a per-pull-request audit.
`scripts/tests/test-npm-audit-retry.sh` pins
that split, including that a real advisory fails even when the tolerant mode
is active and that an unparseable or unrecognised report is never read as
clean. `frontend-modern/src/security/__tests__/dependencySecurity.test.ts`
is active, that an unparseable or unrecognised report is never read as
clean, and that neither a hung attempt nor an exhausted budget can outlive
its bound. `frontend-modern/src/security/__tests__/dependencySecurity.test.ts`
pins the known safe floors for advisories remediated by commit `6ba85a185`,
including DOMPurify `GHSA-55q2-fjhq-7xh7`, brace-expansion
`GHSA-mh99-v99m-4gvg` and `GHSA-rgw5-rvv9-x895`, and nanoid
@@ -3644,14 +3644,20 @@ func TestFrontendDependencySecurityAuditsAreRequired(t *testing.T) {
assertFileContainsAll(t, workflowPath,
`- name: Audit complete frontend dependency graph`,
`npm-audit-retry.sh" all`,
`- name: Audit production frontend dependencies`,
`npm-audit-retry.sh" production`,
// The runner may retry an unreachable advisory endpoint, but only a
// change that leaves the dependency graph untouched may proceed
// without a fresh result.
`NPM_AUDIT_REQUIRE_RESULT: ${{ needs.changes.outputs.frontend_deps }}`,
`frontend_deps: ${{ steps.filter.outputs.frontend_deps }}`,
)
// The production-only audit reports a subset of the complete audit's
// advisories and cannot gate anything the complete audit did not already
// fail on, so it is off the per-pull-request path. It must still run
// somewhere: the scheduled scan owns the dev-versus-production split.
assertFileContainsAll(t, repoFile(".github", "workflows", "security-scan.yml"),
`- name: Audit production dependencies`,
`npm audit --package-lock-only --omit=dev`,
)
// The gate itself must stay strict. An unreachable endpoint may be
// retried, but no severity threshold may be introduced that lets a real
// advisory through, and any vulnerability total must still fail.
@@ -3666,6 +3672,26 @@ func TestFrontendDependencySecurityAuditsAreRequired(t *testing.T) {
assertFileContainsAll(t, runnerPath,
`print("vulnerable" if total else "clean")`,
)
// Retrying must be bounded by wall clock, not by attempt count alone.
// npm's own fetch-timeout defaults to five minutes and it retries
// internally, so three unbounded attempts once ran for 10m56s and
// cancelled the Frontend job with every test already passing.
assertFileContainsAll(t, runnerPath,
`NPM_AUDIT_MAX_SECONDS`,
`NPM_AUDIT_ATTEMPT_TIMEOUT`,
`export npm_config_fetch_retries=0`,
`DEADLINE=`,
)
// The job budget has to stay above the audit budget by a wide margin, or
// a stalled endpoint reappears as a cancelled job rather than a warning.
workflow, err := os.ReadFile(workflowPath)
if err != nil {
t.Fatalf("read %s: %v", workflowPath, err)
}
frontendJob := workflowJobBlock(t, string(workflow), "frontend")
if !strings.Contains(frontendJob, "timeout-minutes: 30") {
t.Fatal("frontend job must keep a bounded timeout above the audit budget")
}
}
func TestReleaseCutGatesCriticalFrontendAndWindowsRuntimeProof(t *testing.T) {
+88 -4
View File
@@ -20,6 +20,16 @@
# change touches the dependency graph (NPM_AUDIT_REQUIRE_RESULT=true) and
# warns without failing when it does not.
#
# The retry budget is wall-clock, not just an attempt count, because attempt
# count alone does not bound anything: npm's own `fetch-timeout` defaults to
# five minutes and it retries internally, so a single `npm audit` against a
# hanging endpoint can sit for minutes before this script sees a verdict. On
# 2026-09-04 that produced a 10m56s audit step (two 5m00s attempts, then a
# 9s success) and cancelled the Frontend job at its 25m limit with every test
# already passing — a green run reported as a failed required check. So each
# attempt is bounded, npm's internal retry loop is disabled in favour of this
# one, and the whole sequence stops at a deadline.
#
# That last split is the whole safety argument. When package.json and
# package-lock.json are untouched, the audit answer for this change is the one
# the base commit already produced, so skipping it adds no risk from this
@@ -32,6 +42,9 @@
# NPM_AUDIT_ATTEMPTS attempts before giving up (default 3)
# NPM_AUDIT_RETRY_DELAY seconds before the first retry, doubled each
# time (default 15)
# NPM_AUDIT_ATTEMPT_TIMEOUT seconds one npm invocation may run (default 60)
# NPM_AUDIT_MAX_SECONDS total wall-clock budget for all attempts
# (default 240)
# NPM_AUDIT_REQUIRE_RESULT "true" to fail when no answer was obtained
# (default true — the safe default)
# NPM_AUDIT_CMD npm executable to invoke (test seam)
@@ -50,9 +63,52 @@ esac
ATTEMPTS="${NPM_AUDIT_ATTEMPTS:-3}"
DELAY="${NPM_AUDIT_RETRY_DELAY:-15}"
ATTEMPT_TIMEOUT="${NPM_AUDIT_ATTEMPT_TIMEOUT:-60}"
MAX_SECONDS="${NPM_AUDIT_MAX_SECONDS:-240}"
REQUIRE_RESULT="${NPM_AUDIT_REQUIRE_RESULT:-true}"
NPM_BIN="${NPM_AUDIT_CMD:-npm}"
# This script is the retry layer. npm's own fetch retry loop would multiply
# every attempt by an unbounded amount of hidden waiting, which is exactly
# what made a bounded-looking three attempts run for eleven minutes.
export npm_config_fetch_retries=0
export npm_config_fetch_timeout=$((ATTEMPT_TIMEOUT * 1000))
DEADLINE=$(( $(date +%s) + MAX_SECONDS ))
# Run one audit under a hard wall-clock bound, portably: `timeout` is not
# present on every developer machine, so a watchdog subshell kills the npm
# process if it outlives the limit. Blocking on `wait` for the real child
# avoids the zombie-liveness race that a `kill -0` poll would hit.
run_audit() {
local limit="$1" out="$2"
: >"${out}"
"${NPM_BIN}" audit --json "${SCOPE_ARGS[@]}" >"${out}" 2>/dev/null &
local npm_pid=$!
(
sleep "${limit}"
kill -TERM "${npm_pid}" 2>/dev/null
sleep 2
kill -KILL "${npm_pid}" 2>/dev/null
) >/dev/null 2>&1 &
local killer_pid=$!
wait "${npm_pid}" 2>/dev/null
local status=$?
kill -TERM "${killer_pid}" 2>/dev/null
wait "${killer_pid}" 2>/dev/null
# 143 = SIGTERM, 137 = SIGKILL: the watchdog fired. The report is then
# empty or truncated, which classify_report already reads as unreachable.
if [ "${status}" -eq 143 ] || [ "${status}" -eq 137 ]; then
return 124
fi
return 0
}
# Classify one audit run. Prints a verdict word on stdout:
# clean — audit completed, no vulnerabilities
# vulnerable — audit completed, vulnerabilities present
@@ -100,9 +156,25 @@ trap 'rm -f "${report_file}"' EXIT
attempt=1
delay="${DELAY}"
budget_exhausted=false
while [ "${attempt}" -le "${ATTEMPTS}" ]; do
echo "npm audit (${SCOPE}) attempt ${attempt}/${ATTEMPTS}"
"${NPM_BIN}" audit --json "${SCOPE_ARGS[@]}" >"${report_file}" 2>/dev/null
remaining=$(( DEADLINE - $(date +%s) ))
if [ "${remaining}" -le 0 ]; then
echo "npm audit (${SCOPE}): ${MAX_SECONDS}s retry budget exhausted before attempt ${attempt}"
budget_exhausted=true
break
fi
# Never let one attempt outlive the overall budget.
attempt_limit="${ATTEMPT_TIMEOUT}"
if [ "${attempt_limit}" -gt "${remaining}" ]; then
attempt_limit="${remaining}"
fi
echo "npm audit (${SCOPE}) attempt ${attempt}/${ATTEMPTS} (limit ${attempt_limit}s, ${remaining}s of budget left)"
if ! run_audit "${attempt_limit}" "${report_file}"; then
echo "npm audit (${SCOPE}): attempt ${attempt} exceeded ${attempt_limit}s and was stopped"
fi
verdict_output="$(classify_report <"${report_file}")"
verdict="$(printf '%s\n' "${verdict_output}" | head -1)"
summary="$(printf '%s\n' "${verdict_output}" | sed -n '2p')"
@@ -126,6 +198,12 @@ while [ "${attempt}" -le "${ATTEMPTS}" ]; do
esac
if [ "${attempt}" -lt "${ATTEMPTS}" ]; then
remaining=$(( DEADLINE - $(date +%s) ))
if [ "${delay}" -ge "${remaining}" ]; then
echo "npm audit (${SCOPE}): ${MAX_SECONDS}s retry budget exhausted"
budget_exhausted=true
break
fi
echo "retrying in ${delay}s"
sleep "${delay}"
delay=$((delay * 2))
@@ -133,10 +211,16 @@ while [ "${attempt}" -le "${ATTEMPTS}" ]; do
attempt=$((attempt + 1))
done
if [ "${budget_exhausted}" = "true" ]; then
gave_up="within its ${MAX_SECONDS}s retry budget"
else
gave_up="after ${ATTEMPTS} attempts"
fi
if [ "${REQUIRE_RESULT}" = "true" ]; then
echo "::error::npm audit (${SCOPE}) could not reach the advisory endpoint after ${ATTEMPTS} attempts, and this change touches the dependency graph, so the result cannot be assumed."
echo "::error::npm audit (${SCOPE}) could not reach the advisory endpoint ${gave_up}, and this change touches the dependency graph, so the result cannot be assumed."
exit 1
fi
echo "::warning::npm audit (${SCOPE}) could not reach the advisory endpoint after ${ATTEMPTS} attempts. This change does not touch package.json or package-lock.json, so the dependency graph is identical to the base commit that already passed; continuing without a fresh result."
echo "::warning::npm audit (${SCOPE}) could not reach the advisory endpoint ${gave_up}. This change does not touch package.json or package-lock.json, so the dependency graph is identical to the base commit that already passed; continuing without a fresh result."
exit 0
+50
View File
@@ -52,6 +52,8 @@ run_case() {
out="$(NPM_AUDIT_CMD="${npm_bin}" \
NPM_AUDIT_RETRY_DELAY=0 \
NPM_AUDIT_ATTEMPTS="${NPM_AUDIT_ATTEMPTS:-3}" \
NPM_AUDIT_ATTEMPT_TIMEOUT="${NPM_AUDIT_ATTEMPT_TIMEOUT:-60}" \
NPM_AUDIT_MAX_SECONDS="${NPM_AUDIT_MAX_SECONDS:-240}" \
NPM_AUDIT_REQUIRE_RESULT="${require}" \
bash "${SCRIPT}" all 2>&1)"
status=$?
@@ -117,6 +119,54 @@ run_case "unknown payload shape is not treated as clean" 1 \
"$(make_fake_npm npm-shape '{"metadata":{}}')" true \
"could not reach the advisory endpoint"
# A hung endpoint must be cut off per attempt rather than inheriting npm's
# own five-minute fetch timeout. This is the regression that cancelled the
# Frontend job on 2026-09-04: three "attempts" ran for eleven minutes.
hanging_npm="${WORK_DIR}/npm-hang"
cat > "${hanging_npm}" <<'SH'
#!/usr/bin/env bash
sleep 300
SH
chmod +x "${hanging_npm}"
started=$(date +%s)
NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=10 \
run_case "a hung audit is stopped at the per-attempt limit" 1 \
"${hanging_npm}" true "was stopped" "could not reach the advisory endpoint"
elapsed=$(( $(date +%s) - started ))
if [ "${elapsed}" -gt 30 ]; then
echo "FAIL: hung audit took ${elapsed}s; the per-attempt limit did not bound it"
failures=$((failures + 1))
else
echo "ok: hung audit bounded in ${elapsed}s"
fi
# The total budget, not just the attempt count, has to end the sequence, and
# an exhausted budget must still respect the fail-closed/fail-open split.
started=$(date +%s)
NPM_AUDIT_ATTEMPTS=50 NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=3 \
run_case "the wall-clock budget ends the retry sequence" 1 \
"${hanging_npm}" true "retry budget exhausted"
elapsed=$(( $(date +%s) - started ))
if [ "${elapsed}" -gt 25 ]; then
echo "FAIL: 50 attempts under a 3s budget took ${elapsed}s; the budget did not bound them"
failures=$((failures + 1))
else
echo "ok: wall-clock budget bounded 50 attempts in ${elapsed}s"
fi
# An exhausted budget is still tolerated when the dependency graph is unchanged.
NPM_AUDIT_ATTEMPTS=50 NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=3 \
run_case "an exhausted budget warns when dependencies unchanged" 0 \
"${hanging_npm}" false "::warning::" "retry budget"
# A real advisory must still fail even under a tight budget: the bound may
# only ever change what happens to an unreachable endpoint.
NPM_AUDIT_ATTEMPT_TIMEOUT=1 NPM_AUDIT_MAX_SECONDS=3 \
run_case "a vulnerability still fails under a tight budget" 1 \
"$(make_fake_npm npm-vuln-budget "${VULN}")" false \
"vulnerabilities present"
if [ "${failures}" -ne 0 ]; then
echo "${failures} test(s) failed"
exit 1