Skip to content

fix(triage): retry a transient npm ci before blaming the PR for it #7884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 1 commit into from
Jul 28, 2026
+255 −17
Merged

fix(triage): retry a transient npm ci before blaming the PR for it #7884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 74 additions & 14 deletions .github/workflows/qwen-triage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,12 +1036,26 @@ jobs:

set +e
{
printf '%s\n' '$ npm ci --prefer-offline --no-audit --progress=false'
runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \
npm ci --prefer-offline --no-audit --progress=false
install_status=$?
# Retried once for the same reason as the verify lane: npm can
# exec a dependency's binary before its own write is closed and
# take an ETXTBSY that has nothing to do with the PR, and this
# lane blames the PR for any install failure. Unconditional, so
# nothing the PR can print decides whether it gets a retry. The
# build stays single-shot: compile errors are deterministic.
install_attempt=1
while :; do
printf '%s\n' '$ npm ci --prefer-offline --no-audit --progress=false'
runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \
npm ci --prefer-offline --no-audit --progress=false
install_status=$?
if [ "$install_status" -eq 0 ] || [ "$install_attempt" -ge 2 ]; then
break
fi
printf '\n%s\n' "npm ci failed with exit code ${install_status}; retrying once."
install_attempt=$((install_attempt + 1))
done
if [ "$install_status" -ne 0 ]; then
printf '\n%s\n' "npm ci failed with exit code ${install_status}."
printf '\n%s\n' "npm ci failed with exit code ${install_status} after ${install_attempt} attempts."
else
printf '\n%s\n' '$ npm run build'
runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \
Expand Down Expand Up @@ -1638,8 +1652,17 @@ jobs:
} > "$BODY_FILE"
elif [ -n "${PREPARE_FAILURE_PHASE:-}" ]; then
PREPARE_LOG="$(find tmux-results -name 'prepare.log' 2>/dev/null | head -1 || true)"
# Initialised, not merely defaulted at use: an inherited value
# would otherwise tell a reader that a single-shot `npm run
# build` had "failed twice in a row" — the same false
# accusation, from the other direction.
PREPARE_ATTEMPTS=''
case "$PREPARE_FAILURE_PHASE" in
install) PREPARE_COMMAND='npm ci' ;;
install)
PREPARE_COMMAND='npm ci'
# Retried once in the prepare step; see the verify lane.
PREPARE_ATTEMPTS=' twice in a row'
;;
build) PREPARE_COMMAND='npm run build' ;;
*)
PREPARE_COMMAND='install/build'
Expand All @@ -1656,7 +1679,7 @@ jobs:
{
printf '%s\n\n' '<!-- qwen-triage:tmux -->'
printf '**tmux real-user testing: fail** - [workflow run](%s)\n\n' "$RUN_URL"
printf 'The PR app could not be launched because `%s` failed before the tmux session started. This is treated as a PR failure verdict rather than an infrastructure failure.\n\n' "$PREPARE_COMMAND"
printf 'The PR app could not be launched because `%s` failed%s before the tmux session started. This is treated as a PR failure verdict rather than an infrastructure failure.\n\n' "$PREPARE_COMMAND" "${PREPARE_ATTEMPTS:-}"
if [ -n "${PREPARE_LOG_NOTE:-}" ]; then
printf '%s\n\n' "$PREPARE_LOG_NOTE"
fi
Expand Down Expand Up @@ -2183,12 +2206,37 @@ jobs:

set +e
{
printf '%s\n' '$ npm ci --prefer-offline --no-audit --progress=false'
runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \
npm ci --prefer-offline --no-audit --progress=false
install_status=$?
# `npm ci` can die on a race that has nothing to do with the PR:
# npm writes a dependency's binary and that package's install
# script execs it before the write is closed, so Linux answers
# ETXTBSY. Observed on esbuild in run 30319209722, which turned
# it into a `fail` verdict against a PR that never touched the
# dependency. One clean retry absorbs that class — `npm ci`
# removes node_modules before installing, so the second attempt
# cannot inherit the half-written file.
#
# Unconditional by design. Gating the retry on the log would
# read text the PR's own lifecycle scripts can write, which is
# exactly the forgeable-signal mistake the verdict logic below
# was rewritten to avoid. A deterministically broken tree just
# fails twice and still earns `fail`; the second attempt is only
# ever paid on a path that is already failing. The build is not
# retried: a compile error is deterministic, so a second run
# would only double the cost of an honest failure.
install_attempt=1
while :; do
printf '%s\n' '$ npm ci --prefer-offline --no-audit --progress=false'
runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \
npm ci --prefer-offline --no-audit --progress=false
install_status=$?
if [ "$install_status" -eq 0 ] || [ "$install_attempt" -ge 2 ]; then
break
fi
printf '\n%s\n' "npm ci failed with exit code ${install_status}; retrying once."
install_attempt=$((install_attempt + 1))
done
if [ "$install_status" -ne 0 ]; then
printf '\n%s\n' "npm ci failed with exit code ${install_status}."
printf '\n%s\n' "npm ci failed with exit code ${install_status} after ${install_attempt} attempts."
else
printf '\n%s\n' '$ npm run build'
runuser -u node -- env -u GITHUB_OUTPUT -u GITHUB_STATE -u GITHUB_ENV -u GITHUB_PATH -u GITHUB_STEP_SUMMARY \
Expand Down Expand Up @@ -3060,8 +3108,20 @@ jobs:
# so it must not overwrite a previous round's real report.
[ "${VERDICT:-}" = 'infra-error' ] && WEAK_BODY=true
PREPARE_LOG="$(find verify-results -name 'prepare.log' 2>/dev/null | head -1 || true)"
# Initialised, not merely defaulted at use: an inherited value
# would otherwise tell a reader that a single-shot `npm run
# build` had "failed twice in a row" — the same false
# accusation, from the other direction.
PREPARE_ATTEMPTS=''
case "$PREPARE_FAILURE_PHASE" in
install) PREPARE_COMMAND='npm ci' ;;
install)
PREPARE_COMMAND='npm ci'
# The prepare step retries the install once, so reaching
# this phase means two consecutive failures. Say so: the
# sentence below blames the PR for it, and "failed twice in
# a row" is the part that earns the accusation.
PREPARE_ATTEMPTS=' twice in a row'
;;
build) PREPARE_COMMAND='npm run build' ;;
*)
PREPARE_COMMAND='install/build'
Expand Down Expand Up @@ -3089,7 +3149,7 @@ jobs:
else
printf '%s\n\n' '<!-- qwen-triage:verify-substantive -->'
printf '**Sandboxed verification: fail** - [workflow run](%s)\n\n' "$RUN_URL"
printf 'The PR could not be built because `%s` failed before any verification started. This is treated as a PR failure verdict rather than an infrastructure failure.\n\n' "$PREPARE_COMMAND"
printf 'The PR could not be built because `%s` failed%s before any verification started. This is treated as a PR failure verdict rather than an infrastructure failure.\n\n' "$PREPARE_COMMAND" "${PREPARE_ATTEMPTS:-}"
fi
emit_block 'Install/build log' "$PREPARE_LOG" 20000
printf '%s\n' '— _Qwen Code · sandboxed verification_'
Expand Down
184 changes: 181 additions & 3 deletions scripts/tests/qwen-triage-workflow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,7 @@ describe('qwen-triage tmux workflow', () => {

const finalizeStep = step('Finalize triage status comment');
// Runs on both outcomes and edits the SAME marker comment (no second post).
expect(finalizeStep).toContain(
"MARKER='<!-- qwen-triage lifecycle -->'",
);
expect(finalizeStep).toContain("MARKER='<!-- qwen-triage lifecycle -->'");
expect(finalizeStep).toContain(
"LEGACY_MARKER='<!-- qwen-triage stage=status -->'",
);
Expand Down Expand Up @@ -1561,6 +1559,11 @@ describe('qwen-triage verify publish fidelity', () => {
});
expect(real).toContain('treated as a PR failure');
expect(real).toContain('npm ci');
// The install is retried, so this sentence is blaming the PR for two
// consecutive failures and has to say which. Without the count a
// reader cannot tell this verdict from the single-shot one that
// mis-blamed a PR for an ETXTBSY race.
expect(real).toContain('failed twice in a row');

// The build arm of the phase mapping was never rendered by any test,
// so a typo in that command name would have shipped unnoticed.
Expand All @@ -1571,6 +1574,23 @@ describe('qwen-triage verify publish fidelity', () => {
});
expect(buildPhase).toContain('npm run build');
expect(buildPhase).not.toContain('`npm ci` failed');
// The build is single-shot, so the retry clause must not leak onto it.
expect(buildPhase).not.toContain('twice in a row');

// Same arm, but with the clause pre-seeded in the environment. This
// is what makes the explicit `PREPARE_ATTEMPTS=''` load-bearing
// rather than decorative: defaulting it at the point of use (
// `${PREPARE_ATTEMPTS:-}`) does nothing against an inherited value,
// and the result would be a report claiming a single-shot build had
// failed twice.
const seededBuild = render(dir, {
NAME: 'buildfail-seeded',
VERDICT: 'fail',
PREPARE_FAILURE_PHASE: 'build',
PREPARE_ATTEMPTS: ' twice in a row',
});
expect(seededBuild).toContain('npm run build');
expect(seededBuild).not.toContain('twice in a row');

// An unrecognized phase must degrade, not mislabel.
const unknownPhase = render(dir, {
Expand Down Expand Up @@ -2044,6 +2064,164 @@ describe('qwen-triage verify round-3 hardening', () => {
}
});

// Run 30319209722 reported `fail` against a PR whose only crime was that
// npm exec'd esbuild's binary before its own write was closed (ETXTBSY),
// so the install is now retried once.
//
// Asserting that structurally — "the step contains a loop" — would pass
// on a loop that never retries and equally on one that never stops, which
// are the two ways this can actually be wrong. So run the real step text:
// `runuser` is stubbed to drop its own arguments and exec the rest (which
// keeps the genuine `env -u ...` stripping in the path under test),
// `chown` and `curl` are no-ops, and `npm` fails a set number of times.
const runPrepare = (jobName, { failures, resultsDir }) => {
const script = stepIn(jobName, 'Install and build PR app')
.match(/run: \|-\n([\s\S]*)$/)?.[1]
.replace(/^ {10}/gm, '');
expect(script).toBeTruthy();
const dir = mkdtempSync(join(tmpdir(), 'prepare-retry-'));
try {
const work = join(dir, 'work');
mkdirSync(work, { recursive: true });
const calls = join(dir, 'npm-ci-calls');
writeFileSync(calls, '');
writeFileSync(
join(dir, 'runuser'),
[
'#!/usr/bin/env bash',
'while [ "$#" -gt 0 ] && [ "$1" != \'--\' ]; do shift; done',
'shift || true',
'exec "$@"',
].join('\n'),
{ mode: 0o755 },
);
writeFileSync(
join(dir, 'npm'),
[
'#!/usr/bin/env bash',
// Only `ci` is counted/failed: `run build` shares this stub and
// must stay a success, or an install-phase assertion could pass
// because the BUILD failed instead.
'if [ "$1" = ci ]; then',
' printf "ci\\n" >> "$NPM_CI_CALLS"',
' n=$(wc -l < "$NPM_CI_CALLS" | tr -d " ")',
' if [ "$n" -le "$NPM_CI_FAILURES" ]; then',
' echo "npm error ETXTBSY" >&2',
' exit 1',
' fi',
'fi',
'exit 0',
].join('\n'),
{ mode: 0o755 },
);
for (const noop of ['chown', 'curl']) {
writeFileSync(join(dir, noop), '#!/usr/bin/env bash\nexit 0\n', {
mode: 0o755,
});
}
const out = join(dir, 'step-output');
writeFileSync(out, '');
const res = spawnSync('bash', ['-c', script], {
cwd: work,
encoding: 'utf8',
env: {
...process.env,
PATH: `${dir}:${process.env.PATH}`,
NPM_CI_CALLS: calls,
NPM_CI_FAILURES: String(failures),
RUNNER_TEMP: dir,
GITHUB_WORKSPACE: work,
GITHUB_OUTPUT: out,
GITHUB_STEP_SUMMARY: '/dev/null',
},
});
return {
status: res.status,
stderr: res.stderr,
output: readFileSync(out, 'utf8'),
attempts: readFileSync(calls, 'utf8').trim()
? readFileSync(calls, 'utf8').trim().split('\n').length
: 0,
log: readFileSync(join(dir, resultsDir, 'prepare.log'), 'utf8'),
};
} finally {
rmSync(dir, { recursive: true, force: true });
}
};

for (const [jobName, resultsDir] of [
['verify', 'verify-results'],
['tmux-testing', 'tmux-results'],
]) {
it(`retries a transient npm ci once in the ${jobName} lane`, () => {
// One ETXTBSY-style failure then success: the run must continue to
// the build with no verdict at all. This is the arm the bug lives in
// — before the retry it emitted verdict=fail here.
const flaky = runPrepare(jobName, { failures: 1, resultsDir });
expect(flaky.attempts).toBe(2);
expect(flaky.output).not.toContain('verdict=');
expect(flaky.log).toContain('retrying once');
expect(flaky.log).toContain('$ npm run build');

// A tree that is genuinely broken still fails, and the retry is
// bounded: exactly two attempts, not an unbounded loop.
const broken = runPrepare(jobName, { failures: 99, resultsDir });
expect(broken.attempts).toBe(2);
expect(broken.output).toContain('verdict=fail');
expect(broken.output).toContain('failure_phase=install');
expect(broken.log).toContain('after 2 attempts');
// The build must not have run once the install gave up.
expect(broken.log).not.toContain('$ npm run build');

// Control: a healthy install must not pay for the retry at all.
const clean = runPrepare(jobName, { failures: 0, resultsDir });
expect(clean.attempts).toBe(1);
expect(clean.output).not.toContain('verdict=');
expect(clean.log).not.toContain('retrying once');
});
}

// The build is deliberately NOT retried — a compile error is
// deterministic, so a second run would only double the cost of an honest
// failure. Pin that asymmetry so a future "retry everything" edit is a
// decision rather than an accident.
it('does not retry the build in either lane', () => {
for (const jobName of ['verify', 'tmux-testing']) {
const prepare = stepIn(jobName, 'Install and build PR app');
const build = prepare.slice(prepare.indexOf('$ npm run build'));
expect(build).not.toContain('while :;');
expect(build).not.toContain('build_attempt');
}
});

// The verify comment's retry clause is rendered for real by the publish
// fidelity suite. There is no equivalent render harness for the tmux
// comment, so its copy of the same wiring is pinned structurally: the
// clause must be set ONLY on the install arm (the build is single-shot)
// and must actually reach the sentence that blames the PR.
it('threads the retry count into both lanes fail copy', () => {
for (const [jobName, stepName] of [
['publish-verify', 'Post verification report comment'],
['publish-tmux', 'Post tmux result comment'],
]) {
const publish = stepIn(jobName, stepName);
const install = publish.indexOf("PREPARE_COMMAND='npm ci'");
const build = publish.indexOf("PREPARE_COMMAND='npm run build'");
expect(install).toBeGreaterThan(-1);
expect(build).toBeGreaterThan(install);
// Assigned between the install arm and the build arm — i.e. inside
// the install arm and nowhere else.
const attempts = publish.indexOf("PREPARE_ATTEMPTS=' twice in a row'");
expect(attempts).toBeGreaterThan(install);
expect(attempts).toBeLessThan(build);
expect(publish.slice(build)).not.toContain('PREPARE_ATTEMPTS=');
// ...and consumed by the PR-blaming sentence, not left dangling.
expect(publish).toMatch(
/failed%s[\s\S]*?treated as a PR failure verdict[\s\S]*?"\$\{PREPARE_ATTEMPTS:-\}"/,
);
}
});

// skipped / n-a upload no artifact, so their download always fails; their
// own reason must still reach the comment.
it('answers skipped and docs-only before the download-failure branch', () => {
Expand Down
Loading