mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-11 09:46:05 +00:00
feat(ci): size-aware default timeout for the PR review job (#8377)
The review-pr job ran every PR with a fixed 180-minute default budget. A medium PR (#8241, +1577/-57) exhausted it and was killed at exactly 180 minutes without ever posting its review, while the same review can finish in ~90 minutes on a less loaded runner. When the caller does not pass an explicit --timeout, size the default budget by the diff (additions + deletions): small PRs (<= 300 lines) keep the proven 180 minutes, and anything larger gets the full 240 cap. An explicit --timeout=N still wins, and a failed size lookup falls back to 180 rather than failing the review. Raise the job-level timeout from 260 to 300 so the 240 budget plus the shared retry and comment posting fit with headroom.
This commit is contained in:
Shaojin Wen
GitHub
parent
4379755474
commit
563f744329
2 changed files with 87 additions and 9 deletions
56
.github/workflows/qwen-code-pr-review.yml
vendored
56
.github/workflows/qwen-code-pr-review.yml
vendored
|
|
@ -347,7 +347,10 @@ jobs:
|
|||
startsWith(github.event.review.body, '@qwen-code /review ') ||
|
||||
startsWith(github.event.review.body, format('@qwen-code /review{0}', '\n'))) &&
|
||||
needs.authorize.outputs.should_review == 'true'))
|
||||
timeout-minutes: 260
|
||||
# 300 (not 260): the per-review budget auto-scales to 240 minutes for any
|
||||
# non-small PR (see "Run review"), and the shared retry budget plus comment
|
||||
# posting need headroom above that within the job-level cap.
|
||||
timeout-minutes: 300
|
||||
runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}'
|
||||
permissions:
|
||||
contents: 'read'
|
||||
|
|
@ -415,12 +418,17 @@ jobs:
|
|||
set -euo pipefail
|
||||
DEFAULT_TIMEOUT_MINUTES=180
|
||||
TIMEOUT_MINUTES="$DEFAULT_TIMEOUT_MINUTES"
|
||||
# Tracks whether the caller chose a timeout explicitly (workflow_dispatch
|
||||
# input or a /review --timeout=N comment). When false, "Run review"
|
||||
# replaces this default with a PR-size-aware tier instead.
|
||||
TIMEOUT_EXPLICIT=false
|
||||
TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}"
|
||||
|
||||
if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
|
||||
PR_NUMBER="${{ github.event.inputs.pr_number }}"
|
||||
REVIEW_MODE="${{ github.event.inputs.review_mode }}"
|
||||
TIMEOUT_MINUTES="${{ github.event.inputs.timeout_minutes || '180' }}"
|
||||
TIMEOUT_EXPLICIT=true
|
||||
elif [ "${{ github.event_name }}" = "issue_comment" ]; then
|
||||
if ! printf '%s\n' "$TRIGGER_COMMAND" | grep -Eq '^@qwen-code[[:space:]]+/review([[:space:]]|$)'; then
|
||||
echo "should_run=false" >> "$GITHUB_OUTPUT"
|
||||
|
|
@ -449,9 +457,11 @@ jobs:
|
|||
case "$token" in
|
||||
--timeout=*)
|
||||
TIMEOUT_MINUTES="${token#--timeout=}"
|
||||
TIMEOUT_EXPLICIT=true
|
||||
;;
|
||||
timeout=*)
|
||||
TIMEOUT_MINUTES="${token#timeout=}"
|
||||
TIMEOUT_EXPLICIT=true
|
||||
;;
|
||||
esac
|
||||
done
|
||||
|
|
@ -463,6 +473,7 @@ jobs:
|
|||
echo "pr_number=$PR_NUMBER"
|
||||
echo "review_mode=$REVIEW_MODE"
|
||||
echo "timeout_minutes=$TIMEOUT_MINUTES"
|
||||
echo "timeout_explicit=$TIMEOUT_EXPLICIT"
|
||||
} >> "$GITHUB_OUTPUT"
|
||||
|
||||
- name: 'Setup Node.js for hosted review'
|
||||
|
|
@ -493,6 +504,7 @@ jobs:
|
|||
PR_NUMBER: '${{ steps.context.outputs.pr_number }}'
|
||||
REVIEW_MODE: '${{ steps.context.outputs.review_mode }}'
|
||||
TIMEOUT_MINUTES: '${{ steps.context.outputs.timeout_minutes }}'
|
||||
TIMEOUT_EXPLICIT: '${{ steps.context.outputs.timeout_explicit }}'
|
||||
# Per-run agent home so this review's session/memory cannot leak into
|
||||
# the next on the reused self-hosted workspace (reset in "Clean stale
|
||||
# agent state"). Must match the QWEN_HOME computed there.
|
||||
|
|
@ -713,6 +725,37 @@ jobs:
|
|||
fail "timeout_minutes must not exceed 240 minutes"
|
||||
fi
|
||||
|
||||
# Size-aware default timeout. A fixed 180-minute default times out
|
||||
# non-trivial PRs whose deep review (chunked passes, two reverse-audit
|
||||
# rounds, test-efficacy probes that run real builds) legitimately
|
||||
# needs longer — e.g. #8241 (+1577) died on the clock at 180, and the
|
||||
# same PR had finished in ~90 minutes on a less loaded runner, so the
|
||||
# variance alone argues for a wide budget. When the caller did not
|
||||
# pass an explicit --timeout, give any non-small PR (> 300 changed
|
||||
# lines, additions + deletions) the full 240 cap; small PRs keep the
|
||||
# proven 180. An explicit --timeout always wins. A size lookup failure
|
||||
# falls back to the 180 default rather than failing the review.
|
||||
EFFECTIVE_TIMEOUT_MINUTES="$TIMEOUT_MINUTES"
|
||||
if [ "${TIMEOUT_EXPLICIT:-false}" != "true" ]; then
|
||||
PR_SIZE_LINES=''
|
||||
if PR_SIZE_DATA="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json additions,deletions --jq '.additions + .deletions' 2>/dev/null)"; then
|
||||
case "$PR_SIZE_DATA" in
|
||||
''|*[!0-9]*) ;;
|
||||
*) PR_SIZE_LINES="$PR_SIZE_DATA" ;;
|
||||
esac
|
||||
fi
|
||||
if [ -n "$PR_SIZE_LINES" ]; then
|
||||
if [ "$PR_SIZE_LINES" -le 300 ]; then
|
||||
EFFECTIVE_TIMEOUT_MINUTES=180
|
||||
else
|
||||
EFFECTIVE_TIMEOUT_MINUTES=240
|
||||
fi
|
||||
echo "PR #${PR_NUMBER} changed ${PR_SIZE_LINES} lines; auto timeout ${EFFECTIVE_TIMEOUT_MINUTES} minutes."
|
||||
else
|
||||
echo "Could not determine PR #${PR_NUMBER} size; using default ${EFFECTIVE_TIMEOUT_MINUTES} minutes."
|
||||
fi
|
||||
fi
|
||||
|
||||
if ! PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')"; then
|
||||
fail "Failed to determine state for PR #${PR_NUMBER}."
|
||||
fi
|
||||
|
|
@ -733,7 +776,10 @@ jobs:
|
|||
export QWEN_CI_REVIEW_REPO="$REPO"
|
||||
export QWEN_CI_REVIEW_PR_NUMBER="$PR_NUMBER"
|
||||
export QWEN_CI_REVIEW_EXPECTED_HEAD_SHA="$EXPECTED_HEAD_SHA"
|
||||
echo "expected_head_sha=$EXPECTED_HEAD_SHA" >> "$GITHUB_OUTPUT"
|
||||
{
|
||||
echo "expected_head_sha=$EXPECTED_HEAD_SHA"
|
||||
echo "effective_timeout_minutes=$EFFECTIVE_TIMEOUT_MINUTES"
|
||||
} >> "$GITHUB_OUTPUT"
|
||||
|
||||
PROMPT="/review ${REVIEW_URL}"
|
||||
if [ "$REVIEW_MODE" = "comment" ]; then
|
||||
|
|
@ -745,7 +791,7 @@ jobs:
|
|||
MODEL_ARGS=(--model "$OPENAI_MODEL")
|
||||
fi
|
||||
|
||||
QWEN_TIMEOUT="$TIMEOUT_MINUTES"
|
||||
QWEN_TIMEOUT="$EFFECTIVE_TIMEOUT_MINUTES"
|
||||
|
||||
# One attempt of the qwen review. Sets OUTCOME (success | retryable |
|
||||
# quota | timeout | fatal), plus REASON/KIND for the failure paths.
|
||||
|
|
@ -901,7 +947,9 @@ jobs:
|
|||
FAILURE_REASON: "${{ steps.review.outputs.failure_reason || 'Run review failed. See workflow logs for details.' }}"
|
||||
PR_NUMBER: '${{ steps.context.outputs.pr_number }}'
|
||||
RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}'
|
||||
TIMEOUT_MINUTES: '${{ steps.context.outputs.timeout_minutes }}'
|
||||
# The size-aware budget actually used by "Run review" (falls back to
|
||||
# the raw context value if the review step failed before setting it).
|
||||
TIMEOUT_MINUTES: '${{ steps.review.outputs.effective_timeout_minutes || steps.context.outputs.timeout_minutes }}'
|
||||
run: |-
|
||||
pr_data="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')" || {
|
||||
echo "Could not verify PR #${PR_NUMBER}; skipping fallback comment." >> "$GITHUB_STEP_SUMMARY"
|
||||
|
|
|
|||
|
|
@ -346,7 +346,7 @@ describe('qwen resolve workflow', () => {
|
|||
const contextStep = step(reviewJob, 'Resolve PR context');
|
||||
const runStep = step(reviewJob, 'Run review');
|
||||
|
||||
expect(reviewJob).toContain('timeout-minutes: 260');
|
||||
expect(reviewJob).toContain('timeout-minutes: 300');
|
||||
expect(contextStep).toContain('DEFAULT_TIMEOUT_MINUTES=180');
|
||||
expect(contextStep).toContain('case "$token" in');
|
||||
expect(contextStep).toContain('--timeout=*)');
|
||||
|
|
@ -355,10 +355,39 @@ describe('qwen resolve workflow', () => {
|
|||
expect(contextStep).toContain('TIMEOUT_MINUTES="${token#timeout=}"');
|
||||
expect(runStep).toContain('if [ "${#TIMEOUT_MINUTES}" -gt 3 ]; then');
|
||||
expect(runStep).toContain('timeout_minutes must not exceed 240 minutes');
|
||||
expect(runStep).toContain('QWEN_TIMEOUT="$TIMEOUT_MINUTES"');
|
||||
expect(runStep).toContain('QWEN_TIMEOUT="$EFFECTIVE_TIMEOUT_MINUTES"');
|
||||
expect(runStep).not.toContain('QWEN_TIMEOUT=$((TIMEOUT_MINUTES - 5))');
|
||||
});
|
||||
|
||||
it('tiers the default review timeout by PR size unless overridden', () => {
|
||||
const contextStep = step(reviewJob, 'Resolve PR context');
|
||||
const runStep = step(reviewJob, 'Run review');
|
||||
|
||||
// The context step records whether the caller chose a timeout explicitly.
|
||||
expect(contextStep).toContain('TIMEOUT_EXPLICIT=false');
|
||||
expect(contextStep).toContain('TIMEOUT_EXPLICIT=true');
|
||||
expect(contextStep).toContain('echo "timeout_explicit=$TIMEOUT_EXPLICIT"');
|
||||
expect(runStep).toContain(
|
||||
"TIMEOUT_EXPLICIT: '${{ steps.context.outputs.timeout_explicit }}'",
|
||||
);
|
||||
|
||||
// Auto-tiering only applies without an explicit --timeout, keys off
|
||||
// additions + deletions, and never exceeds the 240 cap: small PRs keep 180,
|
||||
// anything larger gets the full 240.
|
||||
expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES="$TIMEOUT_MINUTES"');
|
||||
expect(runStep).toContain(
|
||||
'if [ "${TIMEOUT_EXPLICIT:-false}" != "true" ]; then',
|
||||
);
|
||||
expect(runStep).toContain('--json additions,deletions');
|
||||
expect(runStep).toContain('if [ "$PR_SIZE_LINES" -le 300 ]; then');
|
||||
expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES=180');
|
||||
expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES=240');
|
||||
expect(runStep).not.toContain('EFFECTIVE_TIMEOUT_MINUTES=210');
|
||||
expect(runStep).toContain(
|
||||
'echo "effective_timeout_minutes=$EFFECTIVE_TIMEOUT_MINUTES"',
|
||||
);
|
||||
});
|
||||
|
||||
it('tells maintainers how to retry timed-out reviews with more time', () => {
|
||||
const runStep = step(reviewJob, 'Run review');
|
||||
const fallbackStep = step(reviewJob, 'Post fallback comment on failure');
|
||||
|
|
@ -371,6 +400,9 @@ describe('qwen resolve workflow', () => {
|
|||
expect(runStep).toContain('[ "$qwen_status" -eq 137 ]');
|
||||
expect(fallbackStep).toContain('FAILURE_KIND:');
|
||||
expect(fallbackStep).toContain('TIMEOUT_MINUTES:');
|
||||
expect(fallbackStep).toContain(
|
||||
"TIMEOUT_MINUTES: '${{ steps.review.outputs.effective_timeout_minutes || steps.context.outputs.timeout_minutes }}'",
|
||||
);
|
||||
expect(fallbackStep).toContain('@qwen-code /review --timeout=240');
|
||||
expect(fallbackStep).toContain(
|
||||
'This run already used the maximum 240 minute timeout.',
|
||||
|
|
@ -425,9 +457,7 @@ describe('qwen resolve workflow', () => {
|
|||
expect(runStep).toContain(
|
||||
'QWEN_CI_REVIEW_EXPECTED_HEAD_SHA="$EXPECTED_HEAD_SHA"',
|
||||
);
|
||||
expect(runStep).toContain(
|
||||
'echo "expected_head_sha=$EXPECTED_HEAD_SHA" >> "$GITHUB_OUTPUT"',
|
||||
);
|
||||
expect(runStep).toContain('echo "expected_head_sha=$EXPECTED_HEAD_SHA"');
|
||||
expect(fallbackStep).toContain('EXPECTED_HEAD_SHA:');
|
||||
expect(fallbackStep).toContain(
|
||||
'Skipping fallback comment: PR #${PR_NUMBER} is ${pr_state}.',
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue