priority lanes for rebase-cascade pushes, escalate on stack promotion - #50
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Claude finished @jnasbyupgrade's task in 6m 56s —— View job Claude finished reviewing PR #50Todo list
Note: this PR's own commit ( Findings1. (Moderate) Same-SHA self-cancellation can poison This is different from (and does not invalidate) the routine case — a fresh push cancelling a run for the previous, now-stale SHA — which is harmless because branch protection only ever evaluates the current head SHA. It's specifically the same-SHA cancellation this PR newly introduces (via the 2. (Minor-moderate) 3. (Minor) Every What looks solid
As the PR body notes, the low-priority-lane path and the escalate path aren't exercised by CI on this PR itself — worth the manual check on a real stacked PR the author already flagged, and finding #1 above is exactly the kind of interaction that manual check might not surface unless the retarget happens to race an in-flight low-priority run. |
6026d8b to
d7afa05
Compare
…tion gh stack rebase cascades a push to every PR above the one actually changed, at the SAME priority as real new commits - a stacked PR's own runner budget gets crowded out by rebase noise from pushes that introduce no new content. bin/patch_id_hash tells the two apart via a base-independent content hash; a synchronize push whose hash matches the last one observed for this PR routes to a small shared low-priority lane instead of the per-PR high-priority one, still running the full matrix either way (a rebase can break things, and merges wait on a clean run regardless) - just possibly later. A base retarget (edited, gh stack promoting this PR to the bottom of its stack) or a draft PR marked ready escalates to an immediate high-priority run bypassing whatever lane its last push landed in, unless a full run already exists for that exact SHA. ci.yml's pull_request trigger previously had no edited/ready_for_review in its types, so neither of those could fire at all before now. claude-code-review.yml gets the equivalent skip on pure rebase pushes (nothing new to review), restricted to synchronize so it never suppresses the review on ready_for_review/opened/reopened. Reapplied onto the current ci.yml/claude-code-review.yml structure (docs-only gate, PG-matrix derivation, pg-upgrade-test/pg-tle-test draft gating) rather than the stale branch point this originally carried.
a2e6ea0 to
2865308
Compare
The high-priority concurrency group for test/pg-upgrade-test/pg-tle-test was keyed only on PR number (or SHA), shared across every leg of that job's own matrix - with cancel-in-progress true, a job's second leg to start cancelled its own first leg instead of only ever cancelling a PRIOR push's run of the SAME leg. Confirmed live on this PR's own first push: most PostgreSQL-version/pg_tle legs came back cancelled, directly contradicting the "still runs the full matrix either way" design intent. Now includes matrix.pg (test, pg-tle-test) / matrix.old_pg+matrix.new_pg (pg-upgrade-test) in the high-priority group name, so sibling legs of one push get distinct groups while a same-leg rerun on a later push still supersedes the earlier one. The low-priority lane is deliberately left without this split: cancel-in-progress is false there, so a shared name only serializes legs through the lane instead of cancelling them.
No content change - bin/patch_id_hash's base...head patch-id should come out identical to the previous push's, so the changes job should route this synchronize push to the shared low-priority lane instead of the per-PR high-priority one. See the top-of-file CI PRIORITY comment.
The claude-code-review on this PR's original commit already flagged that "Cancel a superseded low-priority run for this SHA" always fires against the exact SHA the new escalated run is about to test (edited/ ready_for_review never change head.sha) - cancelling that other run's all-checks-passed job leaves a competing non-success conclusion for the same check name on the same SHA the escalated run is separately about to report success for. That finding was carried over unfixed into this rebuild; dropping the step entirely removes the race (at the cost of a redundant, already-superseded run finishing rather than being killed early, which was only ever a compute optimization). Also documents a second, more fundamental issue found by actually pushing an unchanged-patch-id follow-up commit: GitHub Actions' concurrency default only keeps one PENDING job per group and cancels the rest rather than queueing them (queue: max would fix it, but it can't combine with a conditionally-true cancel-in-progress, and queue doesn't take an expression) - so the low-priority lane's shared group name, as designed, drops most of a push's own matrix legs instead of deferring them, contradicting the "still gets the FULL matrix either way" guarantee this feature is built around. Left as a flagged open issue rather than guessing at a redesign.
|
Claude finished @jnasbyupgrade's task in 5m 4s —— View job Claude finished reviewing PR #50Todo list
Scope note: Findings1. (Significant) The low-priority lane's shared concurrency group will cancel most of its own matrix legs — turning "runs later" into "reports failure," and poisoning the
The consequence goes a step further than the "KNOWN OPEN ISSUE" comment at
Since this is exactly the Previously flagged items — status
What looks solid
|
Flagged twice by claude-code-review (both times as low-severity/likely acceptable) with no decision recorded anywhere - a plain title/body edit (edited, no base change) now runs a real lint job, since edited was added to pull_request.types for the escalate check. Accepting it: lint is cheap (no PostgreSQL container) and gating it would duplicate the changes job's own docs_only computation on what's meant to be the always-on baseline check.
|
Claude finished @jnasbyupgrade's task in 1m 42s —— View job Claude finished reviewing PR #50Todo list
Note: the This push
This resolves the earlier Minor finding (" Open item carried forwardThe prior review's Significant finding is still unresolved, and still only tracked via the The low-priority lane's shared concurrency group ( This is the common path the PR is built around (rebase cascade → low priority → later promoted to bottom of stack → retarget), so it's worth resolving — or an explicit call to merge with it open, tracked as a followup — before relying on this in production. The comment itself already frames the options (accept weaker/no concurrency cap, split low-priority into a dedicated job with What looks solid (unchanged from prior passes)
|
docs_only_diff, check_run_exists, and ci_priority pull the changes job's docs-only diff, escalate-check, and patch-id compare/persist logic out of ci.yml's run: blocks, matching this repo's existing convention (patch_id_hash, test_existing, ...) of keeping anything non-trivial testable/runnable on its own instead of only inline in a workflow. pg_binary_upgrade and verify_pgtle_smoke do the same for pg-upgrade-test's binary pg_upgrade invocation and pg-tle-test's post-install smoke check - both about to be duplicated across a high/low-priority job-ID split, so extracting them first keeps that split's diff smaller.
… leg queue: max (letting up to 100 runs queue in a concurrency group instead of cancelling all but one) cannot combine with a cancel-in-progress that could evaluate true at runtime - which the previous single-job-per-test-type design, with a priority-conditional concurrency: block, required. Confirmed live: pushing an unchanged-patch-id follow-up cancelled most of that job's own matrix legs instead of queueing them. Split test, pg-upgrade-test, and pg-tle-test into high/low-priority job-ID pairs instead: the high-priority job keeps today's per-PR-per-leg group and an unconditional cancel-in-progress: true; the low-priority job gets a literal cancel-in-progress: false plus queue: max, which is only legal because it's now a job a real push can never enter. While at it, key the low-priority group by the matrix leg itself (e.g. ci-test-lowprio-<pg>) instead of a hashed 0-2 lane number shared across unrelated legs - this preserves full within-push matrix parallelism (each leg queues independently) while still being "low priority" (only competes with other low-priority runs of that exact same leg, repo-wide). This makes the lane computation entirely redundant, so it's removed along with the changes job's lane output.
Orientation for a first-time reader landing on ci.yml cold: what each job proves, the docs-only gate, the draft-PR reduction, and the priority-lane mechanism, with a job-graph diagram and a priority-routing decision-flow diagram. The detailed rationale still lives in ci.yml's own inline comments; this is deliberately just the map, not the territory.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @jnasbyupgrade's task in 5m 38s —— View job Claude finished reviewing PR #50Todo list
Previously flagged item — statusLow-priority lane's shared concurrency group cancelling its own matrix legs (prior "Significant" finding): Fixed. New finding1. (Significant)
The Checks API lists a job's check-run as soon as its parent workflow run is created, in That's exactly the moment the escalate step ( This wouldn't manifest for a retarget that happens well after a normal high-priority run has already completed (the common case when nothing unusual is going on), which may be why the PR body's live-test check didn't hit it — but it's precisely the rebase-cascade → low-priority → promoted-to-bottom-of-stack path this feature is built around, where the retarget racing an in-flight/queued run is the expected case, not an edge case. Suggested fix: filter on completion in the jq select, e.g. What looks solid
|
…ists" GitHub creates a job's check-run (status "queued") as soon as its workflow run starts, well before its needs: are satisfied - so all-checks-passed's own check-run exists in queued/in_progress state for a SHA's entire matrix runtime, which for a low-priority push (queue: max) can be a long time. Filtering only on .name meant a base retarget arriving while that low-priority run was still queued would see a "queued" all-checks-passed and wrongly conclude there was nothing to escalate - exactly the rebase-cascade-then-promoted-to-bottom-of-stack scenario this feature exists for, not an edge case. Add a status == "completed" filter so only a finished run counts. Caught by Claude Code Review on this PR.
|
Claude finished @jnasbyupgrade's task in 1m 20s —— View job Claude finished reviewing PR #50Todo list
This push
Previously flagged item — status"
This is exactly the right fix — a check-run in What looks solid (carried forward, unchanged since prior passes)
No new findings on this pass. All previously-flagged issues across this PR's review history are now resolved. |
"every other heavy job" in the draft-PR paragraph read as "every second job" (alternating) rather than "each of the remaining heavy jobs" - reworded for clarity. ci.yml's top-of-file comment and the new workflows/README.md had grown into two full copies of the same narrative (job-by-job test strategy, and the CI-priority-lane/escalation rationale), which will drift out of sync the next time only one gets edited. Reallocated so each topic has one source of truth: the overview narrative lives in README.md (which already has the table/diagrams for it), ci.yml's top comment is now a short pointer plus what's genuinely specific to this file, and each low-priority job's own inline comment remains the source of truth for its concurrency-group mechanics. Preserved the one implementation detail that was only stated in the removed text (why `ready_for_review` skips the `escalate` step's existing-run check) by moving it next to the `escalate` step itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @jnasbyupgrade's task in 1m 56s —— View job Claude finished reviewing PR #50Todo list
Note: the This push
New finding1. (Minor-moderate)
The Mermaid diagram ( But So as of this push, Suggested fix: split the What looks solid
|
…run check The Escalation section and its diagram stated ready_for_review goes through the same has-a-full-run-already-happened check as a base retarget, but the escalate step (ci.yml) only ever fires for edited with a base change - it never runs for ready_for_review at all, which gets priority: high unconditionally from the diff step's own default. Split the prose and diagram into the two actual distinct rules.
|
Claude finished @jnasbyupgrade's task in 1m 0s —— View job Claude finished reviewing PR #50Todo list
This push
This directly resolves the Minor-moderate finding from the previous review round ("README.md's 'Escalation' section states
This now agrees exactly with Open itemsNone. This was the last outstanding finding from the review history (same-SHA self-cancellation, low-priority-lane concurrency cancelling its own matrix legs, What looks solid (unchanged from prior passes)
|

gh stack rebasecascades a push to every PR above the one actually changed, at the SAME priority as real new commits - a stacked PR's own runner budget gets crowded out by rebase noise from pushes that introduce no new content.bin/patch_id_hash(viabin/ci_priority) tells the two apart via a base-independent content hash (git patch-id --stable, robust to the context-line shifts a rebase causes); a synchronize push whose hash matches the last one observed for this PR routes to a low-priority job ID instead of the per-PR high-priority one.test,pg-upgrade-test, andpg-tle-testeach now exist as two job IDs (e.g.test/test-lowprio) rather than one job with a priority-conditionalconcurrency:block:queue: max(letting up to 100 runs queue in a group instead of cancelling all but one) cannot combine with acancel-in-progressthat could evaluatetrueat runtime, which the original single-job design required and which is why its low-priority lane didn't actually preserve "runs the full matrix either way" (most of a push's own legs got cancelled instead of queued). The low-priority job's concurrency group is now keyed by the matrix leg itself (e.g.ci-test-lowprio-<pg>), shared across every PR's low-priority pushes for that same leg, instead of the small hashed 0-2 lane the first version of this PR used. Verified live: a multi-leg low-priority push now runs/queues its full matrix successfully with no unexpected cancellations, and a same-SHA base retarget correctly escalates to an immediate high-priority run - which ran concurrently with, and independent of, the low-priority run it bypassed.A base retarget (
editedwith abasechange - the signalgh stacksends when a PR is promoted to the bottom of its stack) or a draft PR marked ready escalates to an immediate high-priority run bypassing whatever lane its last push landed in, unless a full run already exists for that exact head SHA (bin/check_run_exists, wrapping the Checks API).Several sizeable inline
run:blocks (the docs-only diff, the escalate check, the patch-id priority decision, the pg_upgrade binary invocation, the pg_tle post-install smoke check) are now standalonebin/scripts, matching this repo's existing convention (bin/patch_id_hash,bin/test_existing,bin/compare_fresh_vs_update,bin/assert_fs_clean) - each runnable/testable on its own, which also keeps the high/low-priority job-ID split's diff smaller since pg-upgrade-test's and pg-tle-test's duplicated steps are correspondingly shorter..github/workflows/README.mdgives a first-time reader a concise map of the jobs, the docs-only gate, the draft-PR reduction, and the priority-lane mechanism, with two Mermaid diagrams (job graph, priority-routing decision flow).claude-code-review.ymlgets the equivalent skip on pure rebase pushes (nothing new to review), restricted tosynchronizeso it never suppresses the review onready_for_review/opened/reopened- unaffected by this change; it doesn't reference job IDs or the lane mechanism.Both workflows share one
actions/cachenamespace (keyed per-PR) for the observed patch-id, so whichever runs first on a given push primes the cache for the other.