diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 2da993ed6bb..c94b39d8e49 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -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 \ @@ -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' @@ -1656,7 +1679,7 @@ jobs: { printf '%s\n\n' '' 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 @@ -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 \ @@ -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' @@ -3089,7 +3149,7 @@ jobs: else printf '%s\n\n' '' 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_' diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index d7d9153d626..46cbcef7d66 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -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=''", - ); + expect(finalizeStep).toContain("MARKER=''"); expect(finalizeStep).toContain( "LEGACY_MARKER=''", ); @@ -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. @@ -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, { @@ -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', () => {