fix(review): rebase only at the merge boundary, and stop repair jobs jumping the queue (#9497, #9498, #9499) - #9511
Merged
Conversation
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
7 tasks
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…jumping the queue (#9497, #9498, #9499) The staleness rebase fired in the READINESS gate -- before the CI-pending wait, the gate verdict, the disposition plan, and the merge-train check. Since the default branch only advances when a PR merges, "N commits ahead" means "N unrelated PRs merged since your head", and every merge wakes a cohort of siblings that each ran this check. Production measured 7 PRs rebased in 12 seconds, 44s after an unrelated merge, with ci.yml's cancel-in-progress killing their running jobs; one cohort was force-rebased three times in 2.5 hours. A joint rebase also resets that cohort to zero-behind simultaneously, so they stay phase-locked and re-cross the threshold together rather than settling. And running before the plan meant a PR about to be auto-closed, on red CI, or held for manual review was rebased first and that CI run thrown away. The threshold now shares maybeForceFreshRebase's call site, inheriting the three guards the readiness path had none of: imminent merge only, clean mergeable state only, and the 3-per-PR-per-24h cap. That also kills the self-feeding loop -- no speculative rebase means no stale surface, which means no repair-priority re-gate. isRegateRepairExhausted keyed its 5-attempt budget on repo#pr#headSha, so a successful rebase minted a new SHA and reset the budget to zero -- the identical bug MAX_FRESH_REBASE_FORCES already fixed for the fresh-rebase counter, whose comment explains why SHA-keying makes a cap unreachable. Re-keyed to repo#pr. Note this resets existing budgets once on deploy, since historical audit rows carry the old SHA-suffixed target. Two more agent-regate-pr producers omitted prCreatedAt, so jobClaimSortKey fell back to a legacy base (~9.5e11) that sorts AHEAD of every real 2026 PR (~1.78e12) -- silently preempting older contributor work. Threaded through the open-PR reconciler, the surface-disposition reconciler and the contributor-cap wake. GitHub's workflow-scope refusal is now classified as permanent for a diff shape rather than retried: update_branch merges the base in, so any workflow change on the default branch since the fork makes the merge a workflow write even when the PR touches none -- 48 of 82 failures in one 7-day window, one PR retried nine times.
…usal, and simplify two unreachable arms (#9497, #9498, #9499) prCreatedAt is now passed unconditionally where its source is NOT NULL (pull_requests.created_at, and GitHub's PR payload) -- the guarded spread had an absent arm nothing could reach. Adds the no-stored-default-branch skip, the cap-exceeded audit naming whichever trigger fired, and an executor-level regression for the workflow-scope refusal being audited but never paged.
JSONbored
force-pushed
the
fix/speculative-rebase-and-ordering
branch
from
July 28, 2026 02:19
ef61467 to
1fa5553
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9511 +/- ##
==========================================
- Coverage 89.55% 88.65% -0.90%
==========================================
Files 843 843
Lines 110135 110139 +4
Branches 26207 26209 +2
==========================================
- Hits 98635 97648 -987
- Misses 10238 11520 +1282
+ Partials 1262 971 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…9499) With no local PR row there is no createdAt to thread, so the wake goes out without the sort hint rather than being skipped -- the same fail-open posture as the bare cooldown key beside it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stops unrelated merges from rebasing and re-running CI on other PRs, and stops repair jobs jumping the contributor queue.
Closes #9497
Closes #9498
Partially addresses #9499 (Phase 1; see Scope)
#9497 — the rebase was speculative
The staleness rebase fired in the readiness gate — before the CI-pending wait, the gate verdict, the disposition plan, and the merge-train check.
The default branch only advances when a PR merges, so "N commits ahead" literally means "N unrelated PRs merged since your head". Every merge wakes a cohort of siblings, each of which ran this check. Production measured:
ci.yml'scancel-in-progress: truekilling their in-flight runs, which is the reported "re-runs CI during the middle of their runs".The correctly-gated sibling has always been
maybeForceFreshRebase: imminent-merge only,mergeableState === "clean"only, capped 3 per PR per 24h keyed on PR number. The threshold now shares that call site and inherits all three guards, so a PR is rebased only when it is the PR about to merge.It also kills the self-feeding loop: no speculative rebase → no stale surface → no repair-priority re-gate.
#9499 Phase 1 — repair jobs were jumping the queue
The repair budget reset on every rebase.
isRegateRepairExhaustedkeyed its 5-attempt budget onrepo#pr#headSha, and a successful rebase mints a new SHA — so the budget reset to zero and the repair path could re-run indefinitely on a PR whose head kept moving. This is the identical bugMAX_FRESH_REBASE_FORCESalready fixed for the fresh-rebase counter, whose own comment explains at length why SHA-keying makes a cap unreachable. Re-keyed torepo#pr.Three producers omitted
prCreatedAt.jobClaimSortKeysortsagent-regate-prrows by PRcreatedAtso contributor work drains oldest-first; without it the key falls back toLEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber(~9.5e11), which sorts ahead of every real 2026 PR (~1.78e12). So the open-PR reconciler, the surface-disposition reconciler and the contributor-cap wake each silently preempted genuinely older work. All three now thread it.#9498 — a permanent failure was retried
GitHub's workflow-scope refusal is now classified as permanent for a diff shape rather than retried. It is not limited to PRs that touch workflow files:
update_branchmerges the base into the head, so any workflow change on the default branch since the PR forked makes the resulting merge a workflow write. In one 7-day window this was 48 of 82update_branchfailures across 14 PRs — and 4 of the 5 worst offenders touched no workflow file themselves, with one PR retried nine times. Audited (so it stays diagnosable), never paged.A correction to #9499 as filed
The issue claims
maybeEnqueueSiblingRegateForMergedPrhas no coalesce key, so "three merges six seconds apart fan out 45 jobs for the same 15 siblings". That is wrong:agent-regate-prjobs already coalesce onagent-regate-pr:${repo}#${pr}(queue-common.ts). I have not "fixed" it, and I have not claimed credit for it. The audit conflated the queue coalesce key with the per-producer cooldown keys that the neighbouring wake producers carry.Scope
#9499's Phase 2 (an explicit focus gate serializing actuation) is not here. It is a genuine behaviour change rather than a tightening — strict global FIFO means one slow PR delays everything behind it, which is precisely why the merge train was built overlap-scoped — and it should be measured after Phase 1 lands. #9499 stays open for that.
Migration note
Re-keying the repair budget resets existing budgets once on deploy, because historical audit rows carry the old SHA-suffixed target key. That is a one-time widening of the cap, not a loss.
Validation
npx tsc --noEmit -p tsconfig.json— cleanqueue-2/3/4,surface-repair-priority,surface-disposition-reconciler,pr-reconciliation,merge-failure,agent-action-executorRegressions — three
queue-4tests asserted the rebase fires during the review pass, which is the defect. They now assert the opposite (no rebase, and no compare call to pay for one), each with the production evidence inline.New merge-boundary coverage (
queue-3): the threshold rebases when far enough ahead; does not when below it; makes no compare call at all when unconfigured (zero added cost); skips when the repo has no stored default branch; and the cap-exceeded audit names whichever trigger fired rather than hardcoding the freshness window.#9498: the classifier recognises four real message shapes, rejects five neighbouring ones (including the sibling
update_branchclassifiers, asserted non-overlapping), and an executor-level regression proves the refusal is audited but never paged.Two guarded spreads were made unconditional after checking their sources are non-null (
pull_requests.created_atisNOT NULL; GitHub's PR payload always carriescreated_at) — the absent arm was unreachable, so simplifying beat testing it.