feat(github): hold sibling PR checks and comment when an apply preflights a target - #940
feat(github): hold sibling PR checks and comment when an apply preflights a target#940aparajon wants to merge 3 commits into
Conversation
4df18d8 to
8506ecb
Compare
8ab2a4c to
3973ef6
Compare
8506ecb to
d72df1f
Compare
3973ef6 to
d30dc67
Compare
…ghts a target The check refresh processor consumes the new preflight request kind: before an apply changes a target schema, every open sibling PR with stored check state on that (environment, database type, database) target gets its check conditionally flipped to blocked (apply-in-flight) and a single explanatory PR comment, so a merge cannot land on a verdict the apply is about to invalidate. Checks owned by an in-progress apply and closed PRs are skipped; the flip is optimistic on the head SHA so a PR that moves mid-fan-out is never stomped, and the comment is idempotent via a hidden per-apply marker. Settle fan-outs release the holds by re-planning against the live schema, and defer without re-planning while a later preflighted apply is still active on the target. A release sweep backfills a settle for any terminal apply whose preflight held checks but whose settle was never recorded, so a hold can never outlive its apply. Consumer registration moves from handler construction to StartCheckRefreshProcessor: a registered consumer now means a processor is actually running to drain requests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fan-out Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…host render The hold phase flips sibling stored checks and stamps holds_recorded_at using storage alone, so the operator gate can start the apply during a code-host outage; the render phase (aggregate Check Run, hold comment) stays retryable without re-blocking the apply, and a re-arm sweep keeps a terminally failed render retrying while its apply is active. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d30dc67 to
5b5c1e6
Compare
|
🤖 Review findings - created by Kiran's code review agent - for pull/940, d30dc67. Verdict: 9 findings — 2 blocking (fail-open aggregate publish, broken comment dedup), 4 non-blocking, 3 suggestions. Blocking
Non-blocking
General suggestions
The one thing that could have broken, verifiedThe preflight-hold contract itself — "when the preflight request is marked completed, every sibling PR is un-mergeable" — because the #941 gate will trust that completion before starting an apply. Verified it is fail-open on both of its surfaces and sound on every other axis. Visible surface: the aggregate Check Run write's error is discarded at merge_gate.go:714 (finding 1), so completion does not imply the merge button is blocked. Stored surface: the hold has no write-path guard protecting it — it writes Verified correct
This review was generated by Claude Code (claude-fable-5). |
morgo
left a comment
morgo
left a comment
There was a problem hiding this comment.
🤖 Approving on Morgan's behalf (automated review).
A correction I owe this PR first: it is green, and an earlier automated sweep of mine had it recorded as failing. The three FAILURE entries (E2E Tests, LocalScale Tests, K8s E2E Tests) belong to a run that was cancelled wholesale — they're aggregate gate jobs failing because their matrix dependencies were cancelled, and the live run is green on those same three names. mergeStateStatus is CLEAN. My sweep counted non-SUCCESS as failure, which is why this sat from 2026-08-08 without a verdict from me. That was my error, not a CI problem.
The headline (+941/-52, 7 files) oversells the risk: 349 of those lines are tests, and the production change is concentrated in merge_gate.go (+409/-29) with small additive pieces elsewhere. I read the driver lifecycle, the hold phase, the render phase, and the store SQL rather than skimming, because the invariant this establishes is what #941's apply-start gate trusts.
The central invariant holds, and I checked it exhaustively rather than taking the comment's word. preflightHoldsConfirmed in #941 opens the gate on State == Completed even without a HoldsRecordedAt stamp, justified by a coalescing argument. There are exactly two writes of MergeGateCompleted: MarkCompleted, lease-guarded and reached only after a successful fan-out (which stamps the holds), and CompletePendingCoalesced, guarded WHERE state = MergeGatePending and called only on siblings from PendingForTarget(..., req.Kind, req.ID) captured before the fan-out began. Everything else terminalizes elsewhere — MarkFailed and, importantly, TerminateStuckProcessing set MergeGateFailed, not completed, and clear retry_after, which is exactly the shape #941's gate re-arms. So no sweep can open the gate on an un-preflighted apply.
The coalescing coverage argument is sound for a reason worth stating: holdStoredPRChecks flips every sibling check on the (environment, database type, database) target, so a hold is target-scoped rather than apply-scoped. One apply's fan-out genuinely covers a second apply's need on the same target, which is what makes completing the sibling without its own hold phase correct rather than merely convenient.
Other failure directions, all safe:
- Heartbeat failure doesn't complete the row — it's left processing for lease expiry and reclaim, on the stated grounds that re-planning is idempotent. That's the right call over "we did the work, record it anyway."
- Panic recovery treats a panic as retryable so the attempt cap makes a deterministic panic terminal, instead of crash-looping every replica on one poison request.
- A failed coalesce leaves the sibling pending to run its own redundant-but-safe fan-out.
MarkPreflightHoldsRecordedis set-once viaCOALESCE, so a retry after a partial render keeps the original stamp rather than moving it.
Finding: ensureCheckHoldComment's doc comment asserts a property that stops being true the moment #941 lands.
Comment failures fail the fan-out — the hold's explanation is part of the preflight contract, so the apply does not start until the operator-facing surface is complete.
In this tree that's accurate, because there's no apply-start gate yet. But #941 keys the gate on HoldsRecordedAt, which recordPreflightHolds stamps before the render phase runs — and its own comment says the gate must "never wait on the code-host rendering," with a passed_render_pending outcome for exactly this case. Since these land as a stack, the sentence arrives in main describing a guarantee the system deliberately doesn't provide. Worth rewording here rather than in #941, since this is where the split was introduced.
Hardening, narrow but pointed: CompletePendingCoalesced's SQL has no kind predicate — it takes an ID and completes whatever pending row it names. Correct today purely because the only caller filters by req.Kind upstream. Given that this statement is the one thing in the system that can mark a preflight Completed without a hold phase ever running, AND kind = ? is cheap insurance against a future caller reaching it from a settle's sibling list.
Nit: HasIssueCommentWithMarker inspects only the newest 100 comments; the doc is honest that an older occurrence means a possible duplicate, and a duplicate hold comment is harmless.
Scope, so the approval isn't read as broader than it is: I verified the driver lifecycle, hold and render paths, the block-reason definition, the client helper, and the merge-gate store SQL. I did not read the 349 test lines or the comment template closely.
Not blocking.
3 participants
Why this matters
A green check on a sibling PR is a promise that its plan still matches the target schema. The moment an apply starts changing that schema, the promise is stale — but today nothing on the PR says so, and the merge gate processor only knows how to re-plan after the apply finishes. This PR teaches the processor to consume preflight requests: hold every sibling PR's check action-required and explain why with a PR comment, before the apply's engine work begins. The fan-out runs in two phases so the hold itself never depends on the code host being reachable. Stack 5/7, on top of #939.
What it does
fanOutCheckPreflight):apply_in_flight_on_target), then stampsholds_recorded_at— the signal the operator gate (feat(api): gate apply start on confirmed sibling PR check holds #941) starts the apply on. No code-host call in this phase, so the holds land even when the code host is fully down.RenderCheckHold): tells the PR's author what is changing, on which target, by whom, and what to do next (wait for the apply to settle; checks re-plan automatically). Idempotent via a hidden per-apply marker, so webhook redeliveries and retries never double-post.ReopenTerminalPreflightsForActiveApplies, counted byschemabot.merge_gate.preflight_renders_rearmed_total), so a hold's Check Run and comment keep retrying until the code host recovers — nothing else would retry them once the apply has started.sweepPreflightedAppliesMissingSettle): settles are now the release valve for holds, so any apply that reaches a terminal state with a preflight but no settle (e.g. cancelled while queued, where no drive tail runs) gets a settle backfilled. A hold can never outlive its apply.StartMergeGateProcessor: a registered consumer now means "a processor is running and will drain requests", which the operator gate in the follow-up PR relies on.How it moves us toward the northstar
The PR's checks become an honest, live rendering of the target's state: "an apply is changing this schema right now" is visible where merges are decided, not buried in an operator log. The final PR in the stack turns this into a hard gate on apply start.
The chain: #867 (storage) → #868 (drive-tail recording) → #866 (settle re-plan processor) → #939 (request kinds + hold storage) → #940 (preflight hold fan-out) → #941 (apply-start gate) → #942 (plan-time holds). Merges bottom-up; each PR retargets to
mainas its base merges.🤖 Generated with Claude Code