Skip to content

fix(scheduler): hold pre-review branch updates while current-head checks are in flight - #1937

Merged
seonghobae merged 1 commit into
mainfrom
fix/scheduler-hold-update-while-checks-in-flight
Sep 5, 2026
Merged

fix(scheduler): hold pre-review branch updates while current-head checks are in flight#1937
seonghobae merged 1 commit into
mainfrom
fix/scheduler-hold-update-while-checks-in-flight

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Fixes the self-cancelling loop measured in #1935.

What changes

  • scripts/ci/pr_review_merge_scheduler_core.py: new has_in_flight_check_runs(pr) (one expression over the existing latest_check_runs / running_check_state), and one guard in the pre-review branch of inspect_pr: when the PR is behind, unreviewed, and any newest current-head check run is still QUEUED/IN_PROGRESS/PENDING/…, the decision is wait instead of update_branch. The reason string keeps the existing prefix so log consumers that match on branch is outdated before review dispatch still match.
  • No age cap on the hold, deliberately: a cap would restart the loop this change removes. A check that never finishes keeps the head where it is; the update resumes on the next heartbeat after every newest check run is terminal.
  • tests/test_pr_review_merge_scheduler.py: contract test with the positive case (queued + in-progress + one success → wait, both reason fragments), the negative control (all terminal → update_branch, prefix unchanged, hold fragment absent), and the empty-rollup case.
  • CLAUDE.md: the governance paragraph said the scheduler updates a branch only after approval; it now describes both update paths (post-approval, and pre-review when no check is in flight).
  • CHANGELOG.md entry.

Why this is the smallest root-cause change

Under the current queue a scheduler run executes 2h+ after it was created, picks one behind PR from its scan (its own PR or another one — a codex/originweave-mv3-owner run updated #1857), and merges main into that head before dispatching any review (core.py:4774-4792, UPDATE_BRANCHES true for pull_request_target at pr-review-merge-scheduler.yml:139). With main moving every ~28 minutes almost every completing run finds a behind PR, so the scheduler's completion rate becomes the PR cancellation rate. Each push cancels every queued check on the old head — 22/28 on #1926, 21/30 on #1484 with 8 successes orphaned — and requeues the PR at the back. Reconstructed at merged_at, 76 of the 77 PRs merged here since 2026-09-04 had 0/12 required contexts satisfied.

The guard removes the pre-review self-cancellation without touching the post-approval update, which main's required_status_checks.strict=true still requires. Alternatives considered: dropping pull_request_target from UPDATE_BRANCHES (only makes the schedule/push sweeps do the same thing less often); an age cap on the hold (reintroduces the loop); dispatching the review on the behind head (wasted under strict=true, since the post-approval update changes the SHA and invalidates a same-head approval).

Not claimed

Verification

Full gate on this head with GITHUB_ACTIONS=true (the env gate at core.py:4172 is what makes these tests run the same code CI runs): 2893 passed / 1 skipped / 21 subtests, coverage report --fail-under=100 → 100%, interrogate → 100%; negative control: the new test fails (1 failed) with origin/main's pr_review_merge_scheduler_core.py swapped in.

Chicken-and-egg note for the merger: this PR's own required contexts are produced by the queue this change unblocks; its head cannot self-validate. Same category as #1932 and #1926.

🤖 Generated with Claude Code

…cks are in flight

A scheduler run that executes after a 2h+ queue wait finds its scanned PR
behind main and merges main into the head before any review is dispatched
(inspect_pr, pre-review path). That push cancels every queued check on the
old head (22/28 on #1926, 21/30 on #1484) and requeues the PR at the back,
so under a saturated queue no head ever finishes its checks: 76 of the 77
PRs merged since 2026-09-04 had 0/12 required contexts satisfied at merge.

has_in_flight_check_runs() reuses latest_check_runs()/running_check_state();
the pre-review path now decides "wait" while any newest current-head check
run is still queued or running. No age cap on purpose: a cap would restart
the loop. The post-approval update path is unchanged (main is strict=true).

CLAUDE.md described only the post-approval update; it now names both paths.

Refs #1935

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 3a1c8211-33df-48b5-a9db-d8eea8561e27

📥 Commits

Reviewing files that changed from the base of the PR and between 3f88e13 and d05856f.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • CLAUDE.md
  • scripts/ci/pr_review_merge_scheduler_core.py
  • tests/test_pr_review_merge_scheduler.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae merged commit 7f4c5e3 into main Sep 5, 2026
6 of 16 checks passed
@seonghobae
seonghobae deleted the fix/scheduler-hold-update-while-checks-in-flight branch September 5, 2026 14:17
seonghobae added a commit that referenced this pull request Sep 5, 2026
…trator-integration

Conflict: tests/test_pr_review_merge_scheduler.py -- both sides appended
tests at end of file (this branch: two SchedulerAdmissionGate reconcile
cases; main: the #1935 pre-review update hold). Kept both.

Gate after merge: 2930 passed, coverage 100%, interrogate 100%.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant