Skip to content

priority lanes for rebase-cascade pushes, escalate on stack promotion - #50

Merged
jnasbyupgrade merged 17 commits into
masterfrom
ci-priority-lanes
Aug 10, 2026
Merged

priority lanes for rebase-cascade pushes, escalate on stack promotion#50
jnasbyupgrade merged 17 commits into
masterfrom
ci-priority-lanes

Conversation

@jnasbyupgrade

@jnasbyupgrade jnasbyupgrade commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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 (via bin/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, and pg-tle-test each now exist as two job IDs (e.g. test/test-lowprio) rather than one job with a priority-conditional concurrency: block: queue: max (letting up to 100 runs queue in a group instead of cancelling all but one) cannot combine with a cancel-in-progress that could evaluate true at 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 (edited with a base change - the signal gh stack sends 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 standalone bin/ 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.md gives 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.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 - unaffected by this change; it doesn't reference job IDs or the lane mechanism.

Both workflows share one actions/cache namespace (keyed per-PR) for the observed patch-id, so whichever runs first on a given push primes the cache for the other.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a965af53-acb1-4cfe-aad3-42e8b35fe118

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files)
  • Understand request (code review via /code-review skill)
  • Review CI/workflow logic (priority lanes, escalation)
  • Review other changed files (Makefile, bin/, test/)
  • Post final review

View job run

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 6m 56s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files)
  • Read .github/workflows/ci.yml and .github/workflows/claude-code-review.yml in full
  • Read bin/patch_id_hash
  • Trace the escalate/cancel/priority-lane logic end-to-end for correctness
  • Post review feedback

Note: this PR's own commit (576a111) only touches ci.yml, claude-code-review.yml, and bin/patch_id_hash — that's what I focused the review on. The other files in the "changed files" list (Makefile, bin/compare_fresh_vs_update, bin/test_existing, the test_existing.sql/* guards, test/README.md, test/install/load.sql) belong to earlier commits already merged from lower PRs in this gh stack and aren't this PR's own contribution, so I didn't re-review them here.


Findings

1. (Moderate) Same-SHA self-cancellation can poison all-checks-passed for a commit that otherwise has a healthy run
ci.yml:176-200 ("Cancel a superseded low-priority run for this SHA") fires when the escalate path (base retarget via edited, or ready_for_review) kicks in. Critically, edited/ready_for_review don't change head.sha — so this cancels another in-progress workflow run for the exact same commit the new escalated run is about to test. That cancelled run's own all-checks-passed job (ci.yml:775-777, if: always()) still executes, sees cancelled in needs.*.result, and deliberately exit 1s (ci.yml:798-801) — writing a failure conclusion for the check named all-checks-passed against the same SHA the escalated run is separately about to report success for. Two check-runs with the identical name on the identical commit is exactly the scenario where GitHub's branch-protection required-check resolution can pick the wrong (more-recently-completed) conclusion, risking a merge block on an otherwise-green commit purely from this internal race.

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 edited/ready_for_review additions to pull_request.types at ci.yml:108, which didn't fire this workflow at all before) that creates the risk.

Fix this →

2. (Minor-moderate) actions: write is silently downgraded to read-only for fork PRs
ci.yml is a plain pull_request trigger (runs for any contributor, not just jnasbyupgrade like claude-code-review.yml's gated pull_request_target). GitHub forces GITHUB_TOKEN to read-only for pull_request runs triggered from a fork, regardless of the permissions: block requesting actions: write (ci.yml:135). So for an external fork PR, the cancel call at ci.yml:197 (gh api -X POST .../cancel) will 403. It's swallowed by || true so the workflow won't break, but the escalate-path cleanup silently no-ops for anyone outside the base repo. Worth a one-line comment noting this is same-repo-only, so it's not mistaken for a working guarantee later.

3. (Minor) Every edited event now runs a real lint job, even a trivial title/description edit
edited was added to pull_request.types (ci.yml:108) to catch base retargets for the escalate check, but lint (ci.yml:352) has no needs: [changes] / docs_only gate — it always runs a full checkout + make lint, and the changes job itself always runs its cache restore/save too. A plain title/body edit with no base change now costs a real lint run + changes job on every save, not just on meaningful triggers. Likely an acceptable tradeoff (lint is cheap, per the file's own top comment), but flagging in case it wasn't a deliberate choice for every edit rather than just base retargets.


What looks solid

  • bin/patch_id_hash's use of git patch-id --stable over base...head is a sound way to get a rebase-invariant content hash, and the design is fail-safe by construction: a false "looks like a rebase" classification still runs the full matrix (just later/lower-priority), so a misclassification can't skip real testing.
  • The changes job's own fetch-depth: 0 checkout and claude-code-review.yml's explicit git fetch origin <head-sha> after a full base-branch checkout both correctly ensure base...head has a resolvable merge-base for patch_id_hash/git diff.
  • claude-code-review.yml's content-check gate correctly restricts the skip to synchronize only, so opened/reopened/ready_for_review always get reviewed regardless of patch-id — matches the stated intent exactly.
  • The escalate step's own failure mode (an all-checks-passed API query erroring out) safely defaults to escalate=true, consistent with the fail-safe-to-full-priority philosophy used throughout.

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.

@jnasbyupgrade
jnasbyupgrade force-pushed the pgtle-update-and-report branch 4 times, most recently from 6026d8b to d7afa05 Compare August 9, 2026 21:23
Base automatically changed from pgtle-update-and-report to master August 10, 2026 16:29
…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.
@jnasbyupgrade jnasbyupgrade changed the title CI: priority lanes for rebase-cascade pushes, escalate on stack promotion priority lanes for rebase-cascade pushes, escalate on stack promotion Aug 10, 2026
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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files)
  • Run /code-review skill per repo custom instructions
  • Post final review

View job run

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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files)
  • Run /code-review skill per repo custom instructions
  • Post final review

View job run

@jnasbyupgrade
jnasbyupgrade marked this pull request as draft August 10, 2026 17:06
@jnasbyupgrade
jnasbyupgrade marked this pull request as ready for review August 10, 2026 17:07
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 5m 4s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files, prior review comments)
  • Diff this PR against origin/master and re-read ci.yml/claude-code-review.yml/bin/patch_id_hash in full
  • Check how the prior review's findings (priority lanes for rebase-cascade pushes, escalate on stack promotion #50 review from 2026-08-06) were addressed by the follow-up commits
  • Trace the low-priority-lane concurrency design end-to-end, including the interaction with escalate and all-checks-passed
  • Post review feedback

Scope note: claude-code-review.yml and bin/patch_id_hash are unchanged since the prior review (verified via git log), so I didn't re-review them — that review's "What looks solid" notes for both still stand. This pass focuses on ci.yml, which changed across commits 14e4cb1 and f42599b in response to that review.


Findings

1. (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 escalate dedup check

ci.yml:408-413 (and the same pattern in pg-upgrade-test/pg-tle-test) gives the low-priority path one shared group name (ci-test-lowprio-<lane>) across every leg of a single push's own matrix, with cancel-in-progress: false. GitHub Actions' concurrency semantics keep only one job running and one queued per group — any additional legs requesting the same group are cancelled outright, not queued behind it. The author's own f42599b commit message confirms this live: "pushing an unchanged-patch-id follow-up here cancelled most of this job's own matrix legs instead of queueing them."

The consequence goes a step further than the "KNOWN OPEN ISSUE" comment at ci.yml:395-407 states:

  • all-checks-passed (ci.yml:830-860) has if: always() and fails (exit 1) if any needs.*.result is cancelled. So a low-priority-routed push whose matrix has more than ~2 legs (e.g. test's ~9 PG-major legs) will almost always get a failing all-checks-passed, not just a delayed one — directly contradicting the PR's central premise ("still gets the full matrix either way... this repo won't merge without a clean run regardless").
  • The escalate step (ci.yml:143-167) only checks existence of an all-checks-passed check-run for the SHA, not its conclusion. Once that false failure is recorded, a later stack-promotion retarget (edited with a base change) for the same SHA will see count > 0 and set escalate=false, treating the SHA as "already covered" — permanently skipping the fresh high-priority run that would actually fix it. Recovery then requires a real new commit (which changes the SHA/patch-id) or a manual re-run; the automation doesn't self-heal.

Since this is exactly the gh stack flow the PR is built around (rebase cascade → low priority → later promoted to bottom of stack → retarget), this isn't a rare edge case — it's the common path this feature targets. Worth resolving (or at least explicitly deciding to accept, per the author's own note that it "needs a real decision... before relying on this path") before merging, since as shipped the feature can make an otherwise-green rebase-only push register as broken.

Fix this →


Previously flagged items — status

  • Same-SHA self-cancellation poisoning all-checks-passed (prior finding Trigger functions should accept an expression #1): Fixed. The "Cancel a superseded low-priority run for this SHA" step is removed entirely in f42599b, along with the actions: write permission it needed. This trades away a compute optimization (a superseded run finishes instead of being killed early) for removing the race — reasonable given the fail-safe philosophy used everywhere else in this design.
  • actions: write silently downgraded on fork PRs (prior finding Syntax error in SQL #2): No longer applicable — the step needing that permission is gone.
  • edited now runs a real lint job even for a trivial title/description edit (prior finding Update to latest pgxntool #3): Still present, unchanged. lint (ci.yml:354) still has no docs_only/needs gate. As before, likely an acceptable tradeoff (lint is cheap) but flagging again since it wasn't addressed either way.

What looks solid

  • 14e4cb1's fix (scoping the high-priority concurrency groups per matrix leg via matrix.pg/matrix.old_pg+matrix.new_pg) is correct and necessary — without it, siblings of the same push's own matrix would cancel each other, which the commit confirms was actually observed.
  • The escalate step's fail-safe defaults (any gh api error → escalate=true; any computation failure → priority=high) are consistent with the rest of the file's philosophy and correctly reasoned.
  • Live-testing the actual behavior (the empty follow-up push in 51b7227, and the matrix-cancellation discovery documented in f42599b) surfaced real bugs that pure code-reading likely wouldn't have caught — good practice, and it's how finding Trigger functions should accept an expression #1 above was found in the first place.

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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 42s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files, prior review comments)
  • Diff this push (f15c875) against the state reviewed previously (14e4cb1/f42599b)
  • Confirm claude-code-review.yml and bin/patch_id_hash are unchanged since the last review
  • Post review feedback

Note: the /code-review skill invocation errored in this environment, so this pass was done by direct diff/file review instead (same scope as intended).


This push

f15c875 only adds a 7-line comment to ci.yml's lint: job (ci.yml:356-362) recording, as a deliberate decision, why lint runs unconditionally on a plain title/description edited event. No behavioral change — confirmed via git show f15c875. claude-code-review.yml and bin/patch_id_hash are unchanged since the previous review round (14e4cb1/f42599b).

This resolves the earlier Minor finding ("edited now runs a real lint job even for a trivial edit") by explicitly accepting the tradeoff rather than fixing it — reasonable, since lint is cheap and gating it would duplicate changes' own docs_only computation on what's meant to be the always-on baseline check.

Open item carried forward

The prior review's Significant finding is still unresolved, and still only tracked via the KNOWN OPEN ISSUE comment at ci.yml:402-414:

The low-priority lane's shared concurrency group (test at ci.yml:415-420, pg-upgrade-test at 516-521, pg-tle-test at 667-672) gives every leg of one push's own matrix the same group name with cancel-in-progress: false. GitHub Actions only keeps one job running + one queued per group; extra legs requesting the same group get cancelled, not queued (confirmed live per the comment, from f42599b's testing). Since all-checks-passed (ci.yml:862-867) fails on any cancelled result, a low-priority-routed push with more than ~2 matrix legs will have most legs cancelled and fail all-checks-passed — not just run later, contradicting the PR's core premise. Worse, escalate (ci.yml:143-167) only checks existence of an all-checks-passed check-run for the SHA, not its conclusion, so a later stack-promotion retarget for that same SHA will see this false failure and skip escalation, treating the SHA as already covered.

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 queue: max, or something else); it just hasn't been decided yet.

What looks solid (unchanged from prior passes)

  • bin/patch_id_hash's rebase-invariant content hash via git patch-id --stable, and the overall fail-safe-to-full-priority/high-priority design, remain sound.
  • claude-code-review.yml's synchronize-only skip gate is correctly scoped.
  • The per-matrix-leg high-priority concurrency group fix (14e4cb1) is correct.

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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files, prior review comments)
  • Run /code-review skill per repo custom instructions
  • Post final review

View job run

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files, prior review comments)
  • Run /code-review skill per repo custom instructions
  • Post final review

View job run

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files, prior review comments)
  • Run /code-review skill per repo custom instructions
  • Post final review

View job run

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude is reviewing this PR

  • Gather context (PR body, diff, changed files, prior review comments)
  • Run /code-review skill per repo custom instructions
  • Post final review

View job run

@jnasbyupgrade
jnasbyupgrade changed the base branch from master to tmp-retarget-target August 10, 2026 19:40
@jnasbyupgrade
jnasbyupgrade changed the base branch from tmp-retarget-target to master August 10, 2026 19:41
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 5m 38s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files, prior review comments)
  • Diff this push against the last fully-reviewed state (f15c875) — 7 commits, +587/-138 across ci.yml, the new bin/ scripts, and .github/workflows/README.md
  • Re-verify the previously-flagged "Significant" low-priority-lane concurrency bug against the new job-ID split
  • Review new standalone bin/ scripts and .github/workflows/README.md
  • Post final review

Previously flagged item — status

Low-priority lane's shared concurrency group cancelling its own matrix legs (prior "Significant" finding): Fixed. test/pg-upgrade-test/pg-tle-test are now genuinely split into -lowprio job IDs (ci.yml:462, 670, 896) with per-leg-only concurrency groups (ci-test-lowprio-<pg> etc., no PR number), cancel-in-progress: false, queue: max. Each leg now queues independently instead of the whole push's matrix colliding on one shared group — matches the PR body's live-verification claim and is consistent with all-checks-passed's needs: list correctly including both the high- and low-priority job IDs (ci.yml:974), so a skipped sibling contributes skipped, not cancelled, to that check.


New finding

1. (Significant) bin/check_run_exists doesn't filter on status, so the escalate check can treat a still-queued/in-progress low-priority run as "already covered" and silently skip the promotion-retarget escalation

bin/check_run_exists:23-24 queries the Checks API and counts any check-run named all-checks-passed for the SHA, regardless of .status (queued/in_progress/completed) — only .name is filtered. The doc comment even says "(any conclusion)", but conclusion is null for a check-run that hasn't reached completed yet, so this reads as "any conclusion, including none yet."

The Checks API lists a job's check-run as soon as its parent workflow run is created, in status: "queued", well before its needs: are satisfied — this is the grey "Queued" entry visible immediately in a PR's Checks tab for any job blocked on dependencies. Since all-checks-passed (ci.yml:973) depends on all 8 other jobs via needs:, its own check-run exists in queued/in_progress state for the entire duration a push's matrix is still running — which, for a low-priority push, could be a long time (queue: max lets it wait behind up to 100 other repo-wide runs for the same leg, by design).

That's exactly the moment the escalate step (ci.yml:170-185) is meant to matter: a gh stack promotion (base retarget via edited) firing while the PR's last push is still sitting in the low-priority queue for that same head SHA. As coded, check_run_exists will report true for that SHA (the queued check-run already "exists"), so escalate=false — the retarget is treated as "nothing to escalate," and the PR keeps waiting on the slow, possibly-still-queued low-priority run instead of jumping to an immediate high-priority run. This directly contradicts the top-of-file comment's stated intent ("UNLESS a full run already exists... there's nothing to gain by re-running it") — a queued run isn't a full run; there's everything to gain by escalating past it.

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. select(.name == \"$name\" and .status == \"completed\"), so only a finished run counts as "already covered."

Fix this →


What looks solid

  • The job-ID split's fix for the concurrency-cancellation bug is correct and matches the PR body's live-verification claims.
  • bin/ci_priority, bin/docs_only_diff, bin/pg_binary_upgrade, and bin/verify_pgtle_smoke are faithful, correctly-scoped extractions of the previous inline run: blocks — no behavioral changes spotted, and each is independently testable as intended. verify_pgtle_smoke's use of make print-EXTENSION_count_nulls_VERSION correctly relies on pgxntool/base.mk's generic print-% target.
  • .github/workflows/README.md accurately reflects the current ci.yml/claude-code-review.yml behavior, including the corrected per-leg low-priority queuing design — good first-read orientation, and both Mermaid diagrams match the actual job graph and priority/escalation logic.
  • claude-code-review.yml and bin/patch_id_hash are unchanged since the last review round; those "what looks solid" notes still stand.

…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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 20s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files, prior review comments)
  • Diff this push (07fee43) against the last fully-reviewed state
  • Re-verify the previously-flagged check_run_exists status-filter finding
  • Review any other new changes in this push
  • Post final review

This push

07fee43 is the only substantive commit since the last review round — the six commits before it (0f52747 through 34a963f) are all empty follow-up pushes used for live-testing the priority-lane/escalate behavior, confirmed via git show --stat (no file changes in any of them).

Previously flagged item — status

"check_run_exists doesn't filter on status, so a queued/in-progress low-priority run is treated as 'already covered'" (prior Significant finding): Fixed.

bin/check_run_exists:35 now filters the jq select on .status == "completed" in addition to .name:

--jq "[.check_runs[] | select(.name == \"$name\" and .status == \"completed\")]"

This is exactly the right fix — a check-run in queued/in_progress state no longer counts as "already exists," so a base-retarget arriving while the SHA's last (low-priority) run is still queued now correctly falls through to escalate=true instead of wrongly treating the in-flight run as sufficient. The doc comment at the top of the file was also updated to state the completed-only intent and explain why (mirroring the reasoning from the prior review finding). Verified the escalate step in ci.yml:180 calls this script unchanged — no other wiring needed to change for the fix to take effect.

What looks solid (carried forward, unchanged since prior passes)

  • The job-ID split's fix for the low-priority-lane concurrency-cancellation bug (per-leg-only groups, queue: max) is correct and matches the PR body's live-verification claims.
  • bin/ci_priority, bin/docs_only_diff, bin/pg_binary_upgrade, bin/verify_pgtle_smoke, and bin/patch_id_hash remain faithful, independently-testable extractions with no behavioral changes.
  • .github/workflows/README.md accurately reflects current behavior, including the corrected priority/escalation design.
  • claude-code-review.yml is unchanged since earlier review rounds; its synchronize-only skip gate remains correctly scoped.

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>
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 56s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files, prior review comments)
  • Diff this push (d60c3da) against the last fully-reviewed state (07fee43)
  • Confirm no functional changes (comment/docs reorganization only)
  • Cross-check the relocated narrative against the actual escalate step logic
  • Post review feedback

Note: the /code-review skill invocation didn't return usable output in this environment, so this pass was done by direct diff/file review instead (same scope as intended).


This push

d60c3da is docs/comments-only: no if:/run:/concurrency: lines changed, confirmed by diffing only comment blocks in ci.yml and prose in README.md. It (a) fixes "every other heavy job" → "the other heavy jobs" (the original read as alternating jobs, not "each remaining one"), and (b) removes the narrative duplication between ci.yml's top-of-file comment and the new README.md by making README.md the one source of truth for the job-strategy/priority-lane narrative.

New finding

1. (Minor-moderate) README.md's "Escalation" section (and its diagram) now states something the code doesn't do for ready_for_review, while the comment this same commit moved into ci.yml states the correct, narrower rule right next to it

README.md:99-106 ("Escalation"):

A base retarget (...) or ready_for_review bypasses whatever priority its last push landed in and forces an immediate high-priority run - unless a full all-checks-passed run already exists for that exact head SHA (bin/check_run_exists), in which case there's nothing to gain by re-running it.

The Mermaid diagram (README.md:94-96) encodes the same thing: both retarget["base retarget (edited) / ready_for_review"] feed into esc{full run already exists for this SHA?}.

But bin/check_run_exists is only ever invoked from the escalate step (ci.yml:113), which is gated if: ... github.event.action == 'edited' && github.event.changes.base != null (ci.yml:104-107) — it never fires for ready_for_review at all. This commit's own relocated comment right at that step (ci.yml:99-102) says so explicitly: "ready_for_review needs no equivalent check here: it isn't edited, so it never enters this step at all - the diff step's default ... already gives it priority: high unconditionally." The commit message even calls out preserving this exact nuance ("why ready_for_review skips the escalate step's existing-run check").

So as of this push, ci.yml's own comment (accurate) and README.md's "Escalation" section + diagram (inaccurate for the ready_for_review case) disagree on the same rule — the opposite of this commit's stated goal of giving each topic one non-drifting source of truth. A reader following only README.md would wrongly conclude that re-marking a draft PR ready is a no-op when a full run already exists for that SHA, when it actually always forces an immediate high-priority run regardless.

Suggested fix: split the README.md Escalation sentence (and the diagram's retarget node) into two branches — base retarget: escalate unless a full run already exists; ready_for_review: always escalates unconditionally (with the same "its prior run, if any, was necessarily the reduced draft-time one" reasoning the original top-of-file comment used to state).

Fix this →


What looks solid

  • The wording fix ("every other heavy job" → "the other heavy jobs") correctly resolves the ambiguity — the original genuinely read as alternating-job skipping.
  • The de-duplication itself is well-executed everywhere else: each job's own -lowprio comment still carries its own concurrency-group mechanics (not moved), only the repeated top-level narrative was consolidated, and the escalate step's own comment correctly gained the ready_for_review-has-no-check nuance that used to live only in the removed top-of-file block.
  • All previously-flagged issues across this PR's review history (same-SHA self-cancellation, low-priority-lane concurrency cancelling its own matrix, check_run_exists not filtering on status) remain resolved — this push didn't touch any of that code.

…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.
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 0s —— View job


Claude finished reviewing PR #50

Todo list

  • Gather context (PR body, diff, changed files, prior review comments)
  • Diff this push (7ba00e2) against the last fully-reviewed state (d60c3da)
  • Confirm the fix matches the previously-flagged README/ci.yml mismatch
  • Post review feedback

This push

7ba00e2 is a docs-only, single-file change (.github/workflows/README.md, +10/-6) — confirmed via git diff --stat d60c3da..HEAD. No other file changed.

This directly resolves the Minor-moderate finding from the previous review round ("README.md's 'Escalation' section states ready_for_review goes through the same existing-run check as a base retarget, which the code doesn't do"):

  • The prose now correctly splits into two rules: a base retarget escalates unless a full all-checks-passed run already exists for that SHA (via bin/check_run_exists); ready_for_review always escalates unconditionally, with no existing-run check, because its prior run (if any) was necessarily the reduced draft-time one.
  • The Mermaid diagram now matches: retarget["base retarget (edited)"] still feeds into the esc{full run already exists?} decision, while a new standalone ready[ready_for_review] --> hijob edge bypasses that check entirely.

This now agrees exactly with ci.yml's actual escalate step logic (gated to action == 'edited' && changes.base != null, so it never fires for ready_for_review, which gets priority: high unconditionally from the diff step's own default) — verified ci.yml and bin/check_run_exists are unchanged since the last review round.

Open items

None. This was the last outstanding finding from the review history (same-SHA self-cancellation, low-priority-lane concurrency cancelling its own matrix legs, check_run_exists not filtering on status, and this README/code mismatch) — all are now resolved.

What looks solid (unchanged from prior passes)

  • The job-ID split's fix for the low-priority-lane concurrency-cancellation bug (per-leg-only groups, queue: max) remains correct.
  • bin/check_run_exists's status == "completed" filter remains correct.
  • bin/ci_priority, bin/docs_only_diff, bin/pg_binary_upgrade, bin/verify_pgtle_smoke, and bin/patch_id_hash remain faithful, independently-testable extractions.
  • claude-code-review.yml's synchronize-only skip gate remains correctly scoped.

@jnasbyupgrade
jnasbyupgrade merged commit 689589b into master Aug 10, 2026
11 checks passed
@jnasbyupgrade
jnasbyupgrade deleted the ci-priority-lanes branch August 10, 2026 22:36
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