diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index db578bacd9c..8c27f4336d4 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -1829,10 +1829,11 @@ jobs: # Pending-check staleness bound (invariant across candidate PRs, computed # once): ignore a check stuck far past any legitimate runtime. The bound # must sit ABOVE real check durations here — review-pr can take ~50m and - # review-address is capped at 120m — so an active run keeps blocking and - # is never aged out mid-flight (which would enqueue the PR against a live - # check and double-process the feedback). - PENDING_STALE_MIN=240 + # a review-address JOB runs up to its 300-minute cap — so an active run + # keeps blocking and is never aged out mid-flight (which would enqueue + # the PR against a live check and double-process the feedback). 330 holds + # a 30-minute margin over that cap. + PENDING_STALE_MIN=330 PENDING_CUTOFF="$(date -u -d "${PENDING_STALE_MIN} minutes ago" +%Y-%m-%dT%H:%M:%SZ)" # Repetition-guard cutoff for the stale-base update marker (invariant @@ -2573,7 +2574,7 @@ jobs: if: |- ${{ needs.review-scan.outputs.has_targets == 'true' }} runs-on: 'ubuntu-latest' - timeout-minutes: 150 + timeout-minutes: 300 permissions: contents: 'read' strategy: @@ -2586,7 +2587,11 @@ jobs: # 7th PR waited 81 minutes for a slot it could have had immediately. # 5 halves that tail while staying a real bound — the point of the cap # is that a backlog cannot open an unbounded number of agent runs at - # once, not the specific number. + # once, not the specific number. The 300-minute job cap raises the + # worst-case hold to 5 runner-hours per slot (25 across the fleet) and + # holds the per-PR head-write concurrency group for the same window. + # The cap itself does not lengthen the queueing tail above; the raised + # 120-minute budget does, for the PRs that exhaust it. max-parallel: 5 matrix: target: '${{ fromJSON(needs.review-scan.outputs.targets) }}' @@ -3317,7 +3322,7 @@ jobs: echo '--- feedback.md ---' cat "${WORKDIR}/feedback.md" - # The agent below runs for up to 80 minutes and the verification gate adds + # The agent below runs for up to 130 minutes and the verification gate adds # more, but nothing reaches the PR thread until "Push and report" at the # very end: a maintainer who just engaged takeover sees silence and cannot # tell a working round from a stuck one. The agent's output already @@ -3374,13 +3379,38 @@ jobs: # revalidation in prepare) — no agent run, no marker, no comment. if: |- ${{ steps.prepare.outputs.stale != 'true' }} - # Bound the agent well below the 150-minute job timeout so a runaway agent - # fails THIS step (not the whole job), leaving the always() verify and - # report steps time to run and post a handoff. A job-level timeout would - # cancel those steps too and leave the loop silent. The 80-minute primary - # attempt plus one 20-minute repair leaves ~50 minutes for setup, two - # verification passes, and reporting. - timeout-minutes: 80 + # Bound the agent below the job timeout so a runaway agent fails THIS + # step (not the whole job), leaving the always() verify and report + # steps time to run and post a handoff. A job-level timeout would + # cancel those steps too and leave the loop silent. + # + # This step timeout is the BACKSTOP for a runaway that ignores the + # agent's own timer; QWEN_TIMEOUT_MS below is the real budget. + # Invariant: budget <= backstop - margin, where the margin covers + # the internal kill path (SIGTERM, 10s grace, SIGKILL, marker write). + # + # Measured on run 30646547838: + # + # setup (12 steps, ends at 'Post autofix status comment') 5-7m + # Triage and address #8005 round 9 50m03s (its own timer) + # #8211 12m45s + # Verification gate #8211 22m48s + # push + report + finalize 3-4s + # + # Setup runs in EARLIER steps, so it never competes with the agent + # for this cap. Worst-case budget: + # + # setup 7 + # Triage and address 130 (120 budget + 10 margin) + # Verification gate 60 (2.6x the measured 22m48s) + # Repair 20 + # Repair verification 60 + # report 3 + # ------------------------------- + # worst case 280 => job timeout 300, and the job runs + # on ubuntu-latest, whose own ceiling + # is 360. + timeout-minutes: 130 env: PR: '${{ env.PR }}' ISSUE: '${{ env.ISSUE }}' @@ -3389,6 +3419,17 @@ jobs: OPENAI_MODEL: '${{ vars.QWEN_AUTOFIX_MODEL || vars.QWEN_PR_REVIEW_MODEL }}' NO_PROXY: '127.0.0.1,localhost,::1' QWEN_HOME: '${{ runner.temp }}/qwen-autofix-review-home' + # The primary attempt's real budget: 120m, with a 10-minute margin + # under the 130-minute step backstop above. The margin covers the + # internal kill path (SIGTERM, 10s grace, SIGKILL, marker write); + # if the step cap fires first, `agent-timeout` is never written and + # the report step misclassifies the round as a crash. + # QWEN_AUTOFIX_TIMEOUT_MS can only LOWER the fallback without a code + # change: the run block clamps it to the 7,200,000 ms ceiling + # (BUDGET_CAP_MS, the fallback itself), so raising the budget still + # requires editing this default, BUDGET_CAP_MS, and the step backstop, + # while a misconfigured variable degrades to a warning, not a misreport. + QWEN_TIMEOUT_MS: '${{ vars.QWEN_AUTOFIX_TIMEOUT_MS || 7200000 }}' CONFLICT: '${{ steps.prepare.outputs.conflict }}' BASE: 'main' SETTINGS_JSON: |- @@ -3441,6 +3482,35 @@ jobs: # write-capable collaborators); keep AUTOFIX_OPENAI_API_KEY a # low-privilege, quota-bounded, rotatable key. git config core.hooksPath .husky + # Clamp the override to the budget ceiling: a repo variable past + # 7,200,000 ms (120m) would arm the timer past the 130-minute step + # backstop, the cap would fire first, and the round would be + # misreported as a crash. Malformed values fall back to the same + # ceiling (run-agent.mjs's own || handles the empty/NaN case). + # The {1,8} width bound keeps 10# inside int64: a 19+ digit value + # wraps negative in (( )) and slips past the comparison unclamped. + # 10# forces base-10: a zero-padded value is octal in (( )) and would + # error past the guard the same way. + # A FLOOR, not just a ceiling — and the floor guards the likelier + # mistake. Every comment here, the PR body and the operator message + # all speak in MINUTES; this one variable wants MILLISECONDS. A + # maintainer told to "raise the agent time budget" who sets + # QWEN_AUTOFIX_TIMEOUT_MS=120 arms a 120 ms timer: every round + # SIGTERMs instantly, writes agent-timeout, and reports "ran out of + # time (timeout (120ms))" until TIMEOUT_WINDOW_CAP trips and AutoFix + # stops on the PR — advising the human to raise the budget they just + # raised, with no ::warning:: anywhere in that loop. 60000 rejects + # every minutes-shaped value (1..999) and every 0/000, which the + # bare regex admitted while the message claimed positivity. + BUDGET_CAP_MS=7200000 + BUDGET_FLOOR_MS=60000 + if [[ ! "${QWEN_TIMEOUT_MS}" =~ ^[0-9]{1,8}$ ]] || + (( 10#${QWEN_TIMEOUT_MS} < BUDGET_FLOOR_MS )) || + (( 10#${QWEN_TIMEOUT_MS} > BUDGET_CAP_MS )); then + echo "::warning::QWEN_TIMEOUT_MS=${QWEN_TIMEOUT_MS} is not an integer of MILLISECONDS in [${BUDGET_FLOOR_MS}, ${BUDGET_CAP_MS}] (120 means 120ms, not 120 minutes); clamping to ${BUDGET_CAP_MS}" + QWEN_TIMEOUT_MS="${BUDGET_CAP_MS}" + fi + export QWEN_TIMEOUT_MS # Trusted staged copy in the mirrored layout — resolves # ../SKILL.md to the trusted staged SKILL, never the PR branch's. node "${RUNNER_TEMP}/autofix-skill/scripts/run-agent.mjs" \ @@ -3456,6 +3526,14 @@ jobs: if: |- ${{ always() && steps.prepare.outputs.stale != 'true' }} continue-on-error: true + # Unbounded until now, and the largest consumer in the job (22m48s + # measured on #8211). Left unbounded it eats the job timeout, and a + # JOB timeout cancels the always() reporters — the silent round this + # design exists to prevent. Bounded here it degrades to the ordinary + # verification-failure path instead: continue-on-error keeps the job + # alive, 'Finalize verification' sees an empty outcome, falls through + # its case to exit 1, and the always() report step posts. + timeout-minutes: 60 run: |- bash "${RUNNER_TEMP}/run-autofix-review-verification.sh" @@ -3560,6 +3638,8 @@ jobs: if: |- ${{ always() && steps.repair.outputs.attempted == 'true' }} continue-on-error: true + # Same bound as the first pass, for the same reason. + timeout-minutes: 60 run: |- bash "${RUNNER_TEMP}/run-autofix-review-verification.sh" @@ -3686,8 +3766,10 @@ jobs: # Salvage a race-lost push instead of discarding the run. The # per-PR head-write concurrency group serialises THIS repo's # workflows, but it cannot stop the PR author (or anything on the - # fork side) pushing during the agent's ~50-minute window — - # observed twice in one day (#7983, #7985): a one-shot push died + # fork side) pushing during the agent's ~120-minute window. The + # stated budget widened it from ~50m, so a race-lost push is that + # much likelier and the retry loop below stays bounded at 3 merges. + # Observed twice in one day (#7983, #7985): a one-shot push died # `fetch first` and a full verified agent run was thrown away. # On rejection, fetch the moved head and MERGE it into the local # line (merge, not rebase: the agent's own conflict-resolution @@ -4215,7 +4297,7 @@ jobs: # this is usually a one-off; the last automatic attempt names # the real fix (split the PR or raise the budget). CAUSE="ran out of time before finishing (${AGENT_TIMEOUT})" - LAST_FIX="a human should split the PR or raise the agent time budget, then re-arm" + LAST_FIX="a human should split the PR or raise the agent time budget and its step backstop, then re-arm" elif [[ -z "${DETAIL_FILE}" ]]; then CAUSE="crashed before it could evaluate the feedback" LAST_FIX="a human should take over this PR" @@ -4429,7 +4511,7 @@ jobs: # failed differently (a gate rejection landing on a window # that already carries the cap — the exact rollout state # of #7929/#7846). - HEADLINE="🤖 AutoFix stopped: this counting window now contains ${TIMEOUT_N} time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is ${TIMEOUT_N} full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget), then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." + HEADLINE="🤖 AutoFix stopped: this counting window now contains ${TIMEOUT_N} time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is ${TIMEOUT_N} full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." fi fi fi diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index c3050958d5a..866dbb0e703 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -384,9 +384,10 @@ describe('qwen-autofix workflow', () => { expect(reviewScanJob).toContain('EFF_WM="${CREATED_WM}"'); expect(reviewScanJob).toContain('echo "targets=[]" >> "${GITHUB_OUTPUT}"'); expect(reviewScanJob).toContain('active checks in flight; skipping until'); - // Staleness bound must sit above legitimate check runtimes (review-address is - // capped at 120m) so an active run is never aged out mid-flight. - expect(reviewScanJob).toContain('PENDING_STALE_MIN=240'); + // Staleness bound must sit above legitimate check runtimes (a review-address + // job runs up to its 300-minute cap) so an active run is never aged out + // mid-flight. + expect(reviewScanJob).toContain('PENDING_STALE_MIN=330'); // The staleness filter itself, including the comparison operator: a check only // blocks if its start is newer than the cutoff. Asserting `> $cut` too means a // flipped comparison (which would age out live checks → double-processing) is @@ -5829,7 +5830,7 @@ describe('qwen-autofix workflow', () => { it('salvages a race-lost push by merging the moved head instead of discarding the run', () => { // A one-shot push dies `fetch first` whenever anything pushes to the PR - // head during the agent's ~50-minute window (observed twice in one day, + // head during the agent's ~120-minute window (observed twice in one day, // #7983/#7985 — a full verified agent run thrown away each time). The // per-PR head-write concurrency group cannot prevent this: it only // serialises THIS repo's workflows, not the PR author or the fork side. @@ -6564,7 +6565,7 @@ describe('qwen-autofix workflow', () => { it('announces a working round up front and closes the same status comment', () => { // The whole point: the live run link reaches the thread BEFORE the - // 80-minute agent step, not after it. Without this the PR is silent from + // 130-minute agent step, not after it. Without this the PR is silent from // takeover until "Push and report", so a working round and a stuck one // look identical. expect(postStatusCommentStep.length).toBeGreaterThan(0); @@ -8317,20 +8318,179 @@ describe('qwen-autofix workflow', () => { rmSync(dir, { recursive: true, force: true }); }); - it('replays the handoff decision and terminal-round transitions under bash', () => { - // The primary + repair steps are bounded below the 150-minute job timeout - // so a runaway - // agent fails the STEP, not the job, leaving the always() report step time to - // run (a job-level timeout would cancel that step too and go silent). - // 150 is the review-address job timeout (unique; other jobs use 5/15/180). - expect(workflow).toContain('timeout-minutes: 150'); + it('bounds every long step so the round fits under the job timeout', () => { + // Every long step is bounded BELOW the job timeout so a runaway fails its + // own STEP, leaving the always() report step time to run — a job-level + // timeout cancels that step too and the round goes silent. + // + // The invariant is the SUM, not any single number: identify the long + // steps by what they EXECUTE, not by whether they already carry a bound, + // so a new step running one of these two scripts shows up here even if + // added without a bound. Cheap setup/report steps run other scripts and + // are covered by the SETUP_AND_REPORT_MIN reserve instead. const addressStep = workflow.match( /- name: 'Triage and address'[\s\S]*?(?=\n {6}- name: )/, )?.[0] ?? ''; - expect(addressStep).toContain('timeout-minutes: 80'); - expect(repairDeterministicRejectionStep).toContain('timeout-minutes: 20'); + // review-address is the LAST job in the file, so the terminator has to + // accept end-of-input as well as the next job header. + const jobBlock = + workflow.match( + /\n {2}review-address:\n[\s\S]*?(?=\n {2}\w[\w-]*:\n|$)/, + )?.[0] ?? ''; + expect(jobBlock).toBeTruthy(); + const jobCapMin = Number( + jobBlock.match(/\n {4}timeout-minutes: (\d+)/)?.[1], + ); + expect(Number.isFinite(jobCapMin)).toBe(true); + const stepBlocks = jobBlock.split(/\n {6}- name: /).slice(1); + const longSteps = stepBlocks.filter((b) => + /node [^\n]*run-agent\.mjs|bash [^\n]*run-autofix-review-verification\.sh/.test( + b, + ), + ); + // Primary + repair agent steps and their two verification gates. + expect(longSteps).toHaveLength(4); + const stepCaps = longSteps.map((b) => + Number(b.match(/\n {8}timeout-minutes: (\d+)/)?.[1]), + ); + for (const cap of stepCaps) { + expect(Number.isFinite(cap)).toBe(true); + } + // The setup/report steps (Prepare, Push, Finalize) are NOT bounded at + // runtime — this reserve is an ASSUMPTION that they stay under 25m + // (measured 5-7m + 3-4s), not a proven headroom. A hung gh call in any + // of them can still eat the job timeout. + const SETUP_AND_REPORT_MIN = 25; + const worstCaseMin = + stepCaps.reduce((a, b) => a + b, 0) + SETUP_AND_REPORT_MIN; + expect(worstCaseMin).toBeLessThanOrEqual(jobCapMin); + expect(jobCapMin).toBeLessThanOrEqual(360); + + // The pending-check staleness bound (review-scan) must sit ABOVE this job + // cap, or a live review-address run ages out of HAS_PENDING_CHECKS + // mid-flight and the PR is enqueued against its own still-running check. + const pendingStaleMin = Number( + reviewScanJob.match(/PENDING_STALE_MIN=(\d+)/)?.[1], + ); + expect(Number.isFinite(pendingStaleMin)).toBe(true); + expect(pendingStaleMin).toBeGreaterThan(jobCapMin); + + // continue-on-error makes bounding the verification gates a graceful + // degrade: a timed-out gate falls through to the report step. + for (const name of ['Verification gate', 'Repair verification gate']) { + const gate = + jobBlock.match( + new RegExp(`- name: '${name}'[\\s\\S]*?(?=\\n {6}- name: )`), + )?.[0] ?? ''; + expect(gate, `${name} is missing from review-address`).toBeTruthy(); + expect(gate).toContain('continue-on-error: true'); + } + + // QWEN_TIMEOUT_MS is the budget that actually ends a round; the step + // timeout is only a backstop. Derive both from the workflow and assert + // the margin so the pair cannot drift silently. + const stepCapMin = Number(addressStep.match(/timeout-minutes: (\d+)/)?.[1]); + const budgetMs = Number( + addressStep.match( + /QWEN_TIMEOUT_MS: '\$\{\{[^}]*\|\|\s*(\d+)\s*\}\}'/, + )?.[1], + ); + expect(Number.isFinite(stepCapMin)).toBe(true); + expect(Number.isFinite(budgetMs)).toBe(true); + const marginMin = stepCapMin - budgetMs / 60000; + // Under the cap, or the cap fires first and the internal kill path never + // writes `agent-timeout` — the file the report step reads to tell a + // timeout apart from a crash. + expect(marginMin).toBeGreaterThanOrEqual(1); + // The repair attempt stays inside its own smaller step the same way. + const repairCapMin = Number( + repairDeterministicRejectionStep.match(/timeout-minutes: (\d+)/)?.[1], + ); + const repairMs = Number( + repairDeterministicRejectionStep.match(/QWEN_TIMEOUT_MS: '(\d+)'/)?.[1], + ); + expect(repairCapMin - repairMs / 60000).toBeGreaterThanOrEqual(1); + + // The run block clamps QWEN_TIMEOUT_MS to a RANGE so a misconfigured repo + // variable degrades to a warning, not a misreport. Both bounds, because + // the floor guards the likelier mistake: this variable is the one place + // that wants milliseconds while everything around it says minutes. + expect(addressStep).toContain('BUDGET_CAP_MS=7200000'); + expect(addressStep).toContain('BUDGET_FLOOR_MS=60000'); + expect(addressStep).toMatch( + /QWEN_TIMEOUT_MS.*MILLISECONDS in \[.*\].*clamping to/, + ); + // The clamp ceiling must stay under the step backstop with the SAME margin + // rule as the fallback above, or the cap fires first and the internal kill + // path never writes `agent-timeout`. + const clampMs = Number(addressStep.match(/BUDGET_CAP_MS=(\d+)/)?.[1]); + expect(Number.isFinite(clampMs)).toBe(true); + expect(clampMs / 60000).toBeLessThanOrEqual(stepCapMin - 1); + // The clamp ceiling IS the fallback budget: raising the default below the + // || without raising BUDGET_CAP_MS would be silently pulled back down (with + // a ::warning::) on every run. + expect(budgetMs).toBeLessThanOrEqual(clampMs); + + // Replay the ACTUAL clamp block so the guard condition is executed, not + // merely string-matched: a flipped operator, a dropped width bound, or a + // missing 10# must fail here rather than survive green. + const clampBlock = addressStep.match( + /BUDGET_CAP_MS=\d+\n[\s\S]*?export QWEN_TIMEOUT_MS/, + )?.[0]; + expect(clampBlock).toBeTruthy(); + const runClamp = (value) => + execFileSync( + 'bash', + ['-c', `${clampBlock}\nprintf '%s' "$QWEN_TIMEOUT_MS"`], + { env: { ...process.env, QWEN_TIMEOUT_MS: value }, encoding: 'utf8' }, + ) + .trim() + .split('\n') + .pop(); + // In-range values pass through untouched; both bounds are inclusive. + expect(runClamp('60000')).toBe('60000'); + expect(runClamp('7200000')).toBe('7200000'); + // Over-cap, malformed, zero-padded (octal), and int64-overflowing values + // all clamp to the ceiling instead of reaching run-agent.mjs unclamped. + expect(runClamp('9999999')).toBe('7200000'); + expect(runClamp('abc')).toBe('7200000'); + expect(runClamp('09999999')).toBe('7200000'); + expect(runClamp('9223372036854775808')).toBe('7200000'); + + // The FLOOR, and the reason it exists: every comment, the PR body and the + // operator message speak in minutes while this variable wants + // milliseconds. `120` is not a contrived input — it is what a maintainer + // told to "raise the agent time budget" types. Unclamped it arms a 120 ms + // timer, so every round SIGTERMs instantly and reports a timeout until + // TIMEOUT_WINDOW_CAP stops AutoFix on the PR, advising the human to raise + // the budget they just raised, with no warning anywhere in that loop. + for (const minutesShaped of ['1', '30', '60', '120', '999']) { + expect(runClamp(minutesShaped)).toBe('7200000'); + } + // `0` and `000` passed the bare regex while the message claimed the value + // had to be positive. + expect(runClamp('0')).toBe('7200000'); + expect(runClamp('000')).toBe('7200000'); + // Just under and just over the floor, so the boundary is pinned in both + // directions rather than only from inside. + expect(runClamp('59999')).toBe('7200000'); + expect(runClamp('60001')).toBe('60001'); + + // The message has to name the units, since a units confusion is the whole + // failure mode this branch exists for. + const warned = execFileSync('bash', ['-c', `${clampBlock}\n:`], { + env: { ...process.env, QWEN_TIMEOUT_MS: '120' }, + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + }); + expect(warned).toContain('::warning::'); + expect(warned).toContain('MILLISECONDS'); + expect(warned).toContain('120 means 120ms, not 120 minutes'); + }); + + it('replays the handoff decision and terminal-round transitions under bash', () => { // Replay the ACTUAL POST_HANDOFF decision extracted from the workflow so the // state transitions are exercised, not merely string-matched. const decision = reviewAddressReportStep.match(