fix(ci): read the CI patrol's status rollup one PR at a time - #10861
Conversation
Asking for statusCheckRollup inside the PR search makes one GraphQL call whose cost grows with matched PRs times their check runs. The repository outgrew it: the call returns HTTP 504 above ~30 matches, and every scheduled patrol run has failed since. The same search without the rollup answers in a second, and per-PR reads are small. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8
|
|
|
Thanks for the PR! Template looks good ✓ Problem: This is an observed failure, not theory. The Direction: Clearly aligned. The patrol is the component that reruns flaky-red CI jobs, so while it is down every flake failure stays red until someone reruns it by hand. Restoring it is unambiguously in scope, and it touches no auth/sandbox/model-selection/telemetry/release surface. Size: Not applicable — the change is confined to Approach: The scope feels right. Dropping the rollup from the list query and reading it per PR is the minimal fix, and skipping (with a stderr note) a PR whose rollup can't be read — instead of failing the whole scan — is the right tradeoff. No unrelated edits or drive-by refactors; nothing to cut. Risk: No elevated risk signals — neither changed file matches the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是已观测到的故障，不是理论问题。 方向：明确对齐。patrol 正是重跑 flake 变红 CI job 的组件，它停摆期间每个 flake 失败都会一直红着，直到有人手工重跑。恢复它显然在范围内，且不涉及 auth/沙箱/模型选择/遥测/发布等敏感面。 规模：不适用——改动仅限 方案：范围合理。把 rollup 从 list 查询中移除、改为逐个 PR 读取是最小修复；对读取失败的 rollup 跳过（并在 stderr 记录）而非让整次扫描失败，也是正确取舍。无无关改动或顺手重构，无可砍内容。 风险：无升级风险信号——两个改动文件均不匹配高回滚率相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe implementation is the minimal version of the fix, and it follows the existing code's idiom. The new per-PR read reuses the exact The new test pins the load-bearing contract: No sequence diagram or files table — this is a focused two-file change. TestingThis is an unattended CI run, so I did not run the PR's code; evidence below is the PR's own CI (via the API) plus my read-only verification of the premise against the live GitHub API. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The ubuntu unit suite ( The one red check, Beyond the PR's own CI, I verified the behavior against the live GitHub API (read-only queries): the list query with The only claim that can't be checked pre-merge is the end-to-end "the patrol reaches 中文说明代码审查实现是该修复的最小版本，且沿用了现有代码的惯用写法。新的逐 PR 读取复用了 新增测试钉住了关键契约： 无时序图、无文件清单表——这是一个聚焦的双文件改动。 测试这是无人值守 CI 运行，因此我未运行 PR 的代码；以下证据来自 PR 自身的 CI（经 API 读取）以及我针对线上 GitHub API 对前提做的只读验证。 （上方表格为 PR 自身 CI 检查，由 finalize job 在 CI 结束后原地更新。） ubuntu 单测套件（ 唯一的红色检查 除 PR 自身 CI 外，我还针对线上 GitHub API 做了只读验证：带 唯一无法在合入前验证的是端到端的"patrol 能进入 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal, well-evidenced fix for a real, verified outage; approval deferred only until the PR's own CI lands green. Stepping back: this restores a piece of CI infrastructure that has been fully down — 30/30 scheduled patrol runs failing — and the patrol is the mechanism that reruns flaky-red jobs. The problem is real and I reproduced it live. The fix is the minimal correct approach: drop the rollup from the list query, read it per PR, skip-and-log on a failed read — and it matches the existing The one reservation is unrelated to the diff. The Every change in the diff is necessary; nothing to cut. In six months this reads clearly — the inline comment explains exactly why the rollup moved out of the list query. Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净、最小、证据充分的修复，针对一次真实且已验证的故障；仅因 PR 自身 CI 尚未变绿而暂缓批准。 退一步看：这恢复的是一整套已完全停摆的 CI 基础设施——patrol 定时运行 30/30 全部失败，而它正是重跑 flake 变红 job 的机制。问题真实存在，我已在线上复现。修复是最小且正确的方案：把 rollup 从 list 查询中移除、逐个 PR 读取、读取失败则跳过并记录——且沿用了现有 唯一的保留意见与 diff 无关： diff 中每一处改动都是必要的，无可砍内容。六个月后回看依然清晰——内联注释完整解释了 rollup 为何移出 list 查询。 批准暂缓，等待 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
One call per PR is fine; 66 of them in series is 46 seconds against a ten-minute job budget, and that cost grows with the open-PR count — the same growth that broke the single bulk query in the first place. Five at a time brings a 66-PR scan to about ten seconds, and the results stay in their original order. Claude-Session: https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head aa32e3ed.
- The failure mode is real and documented with numbers (504 beyond ~30 matches, 100 straight failed scans), and the fix is the right shape: the bulk rollup becomes per-PR small reads, bounded at 5 concurrent so a 66-PR scan stays inside the job's budget without bursting the API, and a single unreadable PR is skipped with a stderr note instead of failing the whole patrol — the next scan picks it up.
- Index handoff is race-free (the
next++claim happens in the synchronous slice; results land at their original positions so ordering is stable), and skipped entries are dropped viafilter(Boolean)rather than surfacing as undefined. The new test pins both the search-query shape (nostatusCheckRollupin the list) and the per-PR fetch behavior. - No prior reviews or threads; CI has no failures on this head. Per the channel convention the call is on the review itself.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Approval blockers: none.
Scope: .github/scripts/ci-flaky-rerun.mjs (full file read) and scripts/tests/ci-flaky-rerun.test.js (new tests reviewed). Cross-file callers of prList (prs(), prsWithMarkers()) and their downstream consumers (selectCandidateTargets, resetSuccessfulFailures) traced.
What I checked
Core change — prList split:
statusCheckRollupis absent from the list query args (--json number,isDraft,baseRefName,headRefOid); the per-PR fetch viagh pr view <N> --json statusCheckRolluprestores the field on each object before it is returned.detailed[i] = { ...pr, statusCheckRollup }correctly merges the base fields with the fetched rollup; slots where the fetch threw remainundefinedand are removed byfilter(Boolean).- Order preservation:
filter(Boolean)is a stable filter on the pre-indexed array, so callers receive PRs in the same order the list query returned them. The concurrency test asserts this ([1..12]).
Concurrency worker pattern:
- JavaScript is single-threaded;
next++is synchronous and executes without interruption betweenawaityield points. Everyi = next++in the for-loop yields a unique index. No race condition. - Five workers claim indices 0-4 synchronously before any
awaitfires. Subsequent iterations are claimed in the for-loop update expression, also synchronously. Correct work-stealing for the JS event loop model. Math.min(PR_DETAIL_CONCURRENCY, prs.length)correctly handles the empty-list and small-list edge cases.
prsWithMarkers() coverage:
- This caller also flows through the updated
prList, soresetSuccessfulFailuresreceives PRs withstatusCheckRolluppopulated.succeededAfter(pr, state)readspr.statusCheckRollup— it gets the correct value. A PR whose rollup fetch fails is skipped; the reset logic won't fire for it, but it remains in future scans. Acceptable per the PR's stated design.
Rate-limit arithmetic:
- At 66 matched PRs: 1 list call + 66 view calls = 67 calls per scan. With
PR_DETAIL_CONCURRENCY = 5the wall time is approximately ceil(66/5) x 0.7 s ~10 s, well within the 10-minute job budget. The 5000-call/hour core limit easily accommodates a few scans per hour.
Test validity:
keeps the status rollup out of the PR search query: verifiesstatusCheckRollupis absent from the list--jsonarg, that PR 8 (error path) is dropped, and that exactly 3ghcalls are made. The test can fail if the field is re-added to the list query.bounds how many PR detail reads run at once: mock usessetTimeout(resolve, 1)to create real yield points; peak inFlight of 5 is structurally guaranteed (5 workers each incrementinFlightsynchronously before their firstawait). The order assertion covers the index-based sparse array.
No issues found in:
- Error text propagation from skipped PRs to stderr.
currentPr(not changed, still fetchesstatusCheckRollupinline for the freshness check in scan).- Callers in the scan and reset entry points.
Cross-check: No prior reviews or inline comments from other reviewers to compare against.
Unreviewed dimensions: Working tree unavailable; test suite was read but not executed locally. The logic is fully traceable statically and the test mocks structurally guarantee the observed behaviors.
Reviewed with AI assistance.
|
Released in v0.23.0. |
5 participants
What this PR does
This PR stops the CI Failure Patrol from asking GitHub for every matched PR's status rollup in one query. The PR search now returns only the cheap fields, and
statusCheckRollupis read per PR afterwards. A PR whose rollup cannot be read is skipped with a note on stderr instead of failing the whole scan — the next run picks it up.Why it's needed
Qwen CI Failure Patrolhas failed 100 consecutive scheduled runs (every run in the last ~22 hours; the most recent is 33699092403). Every one dies the same way, before doing any work:statusCheckRollupexpands every check run of every matched PR, so the list query is a single GraphQL call costing matched PRs × check runs. That is what times out, not the page size. Measured against the live API just now — 66 PRs currently match the search, and each carries 34–86 check runs:--limit 1000/200/100/50--limit 40--limit 30/20/10--limit 100gh pr view N --json statusCheckRollup)Lowering
--limitis not a fix: it fails at 40 and would silently drop matched PRs below that. The threshold moves with the repo's open-PR count and each PR's check count, so the query was always going to cross it — it has now.The patrol is what reruns CI jobs that failed on a flake, so while it is down every flaky red PR stays red until someone reruns it by hand.
Reviewer Test Plan
How to verify
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/ci-flaky-rerun.test.js scripts/tests/ci-flaky-rerun-workflow.test.js— 42 passing, including the new contract that pinsstatusCheckRollupout of the list query, merges the per-PR rollups, and skips a PR whose rollup read throws.gh pr list --repo QwenLM/qwen-code --base main --state open --search "status:failure updated:>=$(date -u -d '7 days ago' +%F)" --json number,isDraft,baseRefName,headRefOid,statusCheckRollup --limit 50returns HTTP 504; dropstatusCheckRollupand the same search answers immediately.Qwen CI Failure Patrolrun reachAct on classified PR failuresinstead of dying inClassify stale PR failures.Evidence (Before & After)
Before: 100/100 scheduled runs failed at
Classify stale PR failureswith HTTP 504 from the GraphQL endpoint; theAct on classified PR failuresjob never ran.After: the list query drops the rollup and each rollup is a separate small read. At the current 66 matched PRs the scan makes 1 + 66 calls at ~0.7s each, about 45 seconds, well inside the job.
Tested on
Environment (optional)
Script contract tests plus live
ghqueries against this repository.Risk & Scope
prsWithMarkers()sharesprListand gets the same treatment; it was not the query that failed. Nothing else in the patrol's classification or rerun logic changes.prListreturns the same shape, minus PRs whose rollup could not be read.Linked Issues
None — found while auditing red workflows on
main.中文说明
本 PR 做了什么
本 PR 让 CI Failure Patrol 不再在一次查询里向 GitHub 索取所有匹配 PR 的 status rollup。PR 搜索现在只返回廉价字段，
statusCheckRollup改为随后逐个 PR 读取。读取失败的 PR 会被跳过并在 stderr 记录一行，而不是让整次扫描失败 —— 下一次运行会重新处理它。为什么需要
Qwen CI Failure Patrol已经连续 100 次定时运行全部失败（最近约 22 小时内的每一次；最新一次是 33699092403）。每一次都在开始干活之前以同样的方式失败：statusCheckRollup会展开每个匹配 PR 的每一个 check run，因此这个 list 查询是一次 GraphQL 调用，其代价等于「匹配 PR 数 × 各自的 check run 数」。超时的是它，而不是分页大小。刚刚针对线上 API 的实测 —— 当前有 66 个 PR 匹配该搜索，每个带有 34–86 个 check run：--limit 1000/200/100/50--limit 40--limit 30/20/10--limit 100gh pr view N --json statusCheckRollup）调小
--limit不是修复：它在 40 就失败，而且会静默丢弃阈值以下的匹配 PR。这个阈值随仓库未关闭 PR 数量和每个 PR 的 check 数量移动，所以这个查询迟早会越过它 —— 现在越过了。Patrol 正是负责对因 flake 失败的 CI job 重跑的组件，所以它停摆期间，每一个 flake 变红的 PR 都会一直红着，直到有人手工重跑。
Reviewer Test Plan
如何验证
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/ci-flaky-rerun.test.js scripts/tests/ci-flaky-rerun-workflow.test.js—— 42 个通过，其中包含新增契约：statusCheckRollup不出现在 list 查询里、逐个 PR 的 rollup 会被合并、读取抛错的 PR 会被跳过。gh pr list --repo QwenLM/qwen-code --base main --state open --search "status:failure updated:>=$(date -u -d '7 days ago' +%F)" --json number,isDraft,baseRefName,headRefOid,statusCheckRollup --limit 50返回 HTTP 504；去掉statusCheckRollup后同样的搜索立即返回。Qwen CI Failure Patrol，预期它能进入Act on classified PR failures，而不是死在Classify stale PR failures。Evidence（修复前后）
修复前：100/100 次定时运行在
Classify stale PR failures处以 GraphQL 端点的 HTTP 504 失败；Act on classified PR failures从未运行。修复后：list 查询不再带 rollup，每个 rollup 是一次独立的小请求。按当前 66 个匹配 PR 计算，一次扫描发出 1 + 66 次调用，每次约 0.7 秒，合计约 45 秒，远在 job 时限之内。
测试环境
Environment（可选）
脚本契约测试，以及针对本仓库的线上
gh查询。风险与范围
prsWithMarkers()共用prList，因此同样受益；它不是失败的那个查询。Patrol 的分类与重跑逻辑没有任何改动。prList返回结构不变，只是不再包含 rollup 读取失败的 PR。关联 Issue
无 —— 在排查
main上变红的 workflow 时发现。https://claude.ai/code/session_01MCE9CXnMVrX4fUxHpQoUr8