feat(api): gate apply start on confirmed sibling PR check holds - #941
feat(api): gate apply start on confirmed sibling PR check holds#941aparajon wants to merge 4 commits into
Conversation
2d89e15 to
aeae2bb
Compare
8ab2a4c to
3973ef6
Compare
3973ef6 to
d30dc67
Compare
aeae2bb to
44166f4
Compare
Before a driver starts an apply's engine work, it now records a durable preflight check refresh request and waits for the processor to confirm every sibling PR's stored check on the target is held action-required with its hold comment posted. The gate fails closed: a storage error or an unconfirmed hold abandons the drive attempt and leaves the apply claimable, so uncertainty is never converted into a started apply racing a green sibling check. A terminally failed preflight is re-armed for retry and the processor kicked again. The gate skips servers with no check refresh consumer (no GitHub runtime — nothing to hold) and applies with no tasks (a plan with no diff changes nothing, so there is nothing to hold against). Settles are now recorded on every terminal state — always for completed applies, and for failed/cancelled applies whose preflight held sibling checks — so a hold is always released by a re-plan against the live schema. A new preflight_gate_total metric counts passed/timeout/error outcomes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The check refresh request, the operator preflight gate, and their storage contracts are code-host independent: the gate keys off a registered consumer callback and durable request state, and any code-host integration can run the processor that drains requests. Core-layer comments, logs, and metric docs now describe that contract — a check refresh consumer, sibling change checks, a code-host outage — instead of naming GitHub, which is one adapter that implements it. GitHub vocabulary stays where the GitHub adapter lives (pkg/webhook, pkg/github). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The check preflight gate now waits only on the storage-only hold phase — holds_recorded_at, or a completed request for preflights coalesced into a same-target sibling's fan-out — so a code-host outage can never block an apply on the rendering of its own holds. The render keeps retrying separately, and the gate's timeout error and metrics name the hold phase so a sustained block points at storage or the processor, not GitHub. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d30dc67 to
5b5c1e6
Compare
44166f4 to
9614adb
Compare
|
🤖 Review findings - created by Kiran's code review agent - for pull/941, 44166f4. Verdict: 9 findings — 2 blocking (stop-then-restart bypasses the gate with holds released; stop reconciliation gated on GitHub), 3 non-blocking, 4 suggestions. Blocking
Non-blocking
General suggestions
The one thing that could have broken, verifiedThe completed-preflight fast path at operator.go#L1160: the gate's entire safety story rests on the invariant that a completed preflight implies its sibling-check holds remain in force whenever engine work resumes. I tried to prove that invariant and instead disproved it — it is unsafe (Blocking #1). Every link was verified in the worktree: Verified correct
This review was generated by Claude Code (claude-fable-5). |
|
🤖 Automated review on Morgan's behalf — feedback, not an approval. Holding this one for a human look, with reasons below. First, a correction I owe this PR: it is green, and I had it recorded as failing. The three FAILURE entries ( The gate itself is carefully built, and several things I went looking for are already handled: the resume/cutover fast path costs one storage read, task-less applies skip the gate entirely, a terminally failed request is re-armed with a fresh budget rather than needing manual intervention, and the wait is scoped to the storage-only holds so an unreachable code host can never block an apply. The reasoning about incident-mitigating applies in that last comment is exactly the right instinct. I'm holding for two reasons, both specific. 1. return req.State == storage.MergeGateCompleted || req.HoldsRecordedAt != nilThe justification is coalescing — "a pending preflight coalesced into a same-target sibling's fan-out completes without running its own hold phase, covered by the holds that fan-out recorded." If that's the only way a preflight request reaches That's worth either a comment naming the exhaustive set of paths to 2. It makes apply start depend on merge gate processor health, globally. This is the part I'd want a human decision on rather than a reviewer's. Fail-closed is the right direction for a merge gate, and the 90s bound plus staying claimable is the right shape. But the net effect is that a wedged processor stalls every apply on every target, including one being run to mitigate an incident — the case the rendering path was deliberately decoupled to protect. The re-arm path covers a terminally failed request; it doesn't cover a processor that isn't draining at all (lease stuck, pods down, poll loop dead). Concretely: is there an intended operator escape hatch — a config kill-switch, or a way to run an apply with the gate bypassed — for the case where the gate itself is the outage? If the answer is "the gate is the point, and applies should stall," that's a legitimate call, but it deserves to be an explicit one rather than an emergent property, and it's above what I'll stamp automatically. Neither of these says the design is wrong. They're the two places where this stack's safety argument isn't checkable from this PR alone. |
|
🤖 Follow-up to my previous comment (automated review) — my first objection is resolved; I withdraw it. I said the There are exactly two writes of
Everything else terminalizes as And the coalescing coverage argument is genuinely correct, not just plausible: So the gate cannot let an apply start un-preflighted through any path I can find. That was the correctness question, and it's answered. I've approved #940 with a note suggesting My second point stands, and it's the reason I'm still not stamping this one. The gate makes apply start depend on merge gate processor health globally: a processor that isn't draining at all — not a failed request, which you re-arm correctly, but a dead poll loop, stuck leases, or pods down — stalls every apply on every target for 90s per attempt, indefinitely. That includes an apply being run to mitigate an incident, which is the exact case the rendering path was deliberately decoupled to protect. That's a deliberate trade, not a bug, and it may well be the right one — but "should applies stall when the merge gate is the outage?" is a call for a human owner rather than an automated reviewer, and it's the kind of coupling that's much easier to add than to notice later. If there's an intended escape hatch (a config kill-switch, or an operator override on the apply path), pointing at it would probably clear this. One smaller thing, raised on #940 but it affects this PR's contract: |
morgo
left a comment
morgo
left a comment
There was a problem hiding this comment.
🤖 Approving on Morgan's behalf (automated review).
Clearing a hold I'd been carrying too long, and the reason matters. I flagged one thing here — that gating apply start on merge-gate-processor health couples the production apply path to a background worker — then raised it as a design call and held for it across several passes. That was wrong twice over: I'd already withdrawn the technical half of the objection after tracing #940's completion writes, and the design half is answered by the code in this PR. Holding for an answer I could read was the same reflex as holding on a CI count I hadn't checked.
The gate is coupled to storage, not to the processor, and not to GitHub. That's the crux, and it's waitForCheckPreflight's two exit conditions: MergeGateCompleted, or HoldsRecordedAt != nil. The second returns passed_render_pending — the apply starts as soon as the holds are durable in storage, while the code-host rendering keeps retrying on its own. So a GitHub outage cannot block an apply. That's AV-1 preserved through a gate that looks, from the outside, like it would break it, and it's the difference between this being safe and being a new dependency on the code host in the apply path.
Four escapes fire before any waiting, which keeps the blast radius to applies that genuinely need the gate:
- no merge gate consumer registered → ungated, with the reason logged;
- holds already confirmed → immediate pass, the resume/cutover fast path;
- apply owns zero task rows → no DDL, so nothing to hold;
- holds recorded but rendering pending → pass, per above.
On timeout it delays, it does not fail. The 90s expiry returns an error, and the contract in the doc comment is that the caller abandons the drive attempt and the apply stays claimable, so start is retried on a later poll. A sick processor costs latency and leaves the apply in a retryable state; it does not fail applies and it cannot start one un-preflighted. That is the answer to the question I was holding for, and it's the right shape.
The self-healing loop closes exactly where #940 left it. TerminateStuckProcessing terminalizes a stuck request as MergeGateFailed with RetryAfter nulled — and lines 1253-1263 here catch precisely that state, call ReopenForRetry, and wake the processor. I'd traced the producing half in #940 without seeing the consumer; this is it. Nice bit of stack design.
Finding: the re-arm is unbounded across drive attempts, which defeats #940's attempt cap. Each timed-out drive abandons and retries later; on the next attempt a MergeGateFailed request with no RetryAfter is re-armed with a fresh attempt budget. For a transient storage failure that's the behavior you want, and the comment says so: blocked "only until the cause clears, not until manual intervention." But #940's attempt cap exists specifically so a poison request can't loop forever, and this path hands it a new budget every drive. A deterministically-failing preflight therefore retries indefinitely with no terminal state to alert on — and from the outside it's indistinguishable from a slow-but-recovering one, since both just emit timeout repeatedly.
The check_preflight_gate_outcome{outcome="timeout"} counter does make it observable, so this isn't silent. But "same signal for recoverable and unrecoverable" is the part I'd tighten: a re-arm counter on the request, or a distinct outcome label once a request has been re-armed more than once, would let an alert separate the two without changing the retry policy. Worth a follow-up rather than a change here.
Two smaller notes. The req == nil mid-wait error reads alarming but self-heals — the drive is abandoned and the next attempt re-records the request through the req == nil branch above, so a racing cleanup costs one drive attempt. And preflightHoldsConfirmed accepting Completed without a HoldsRecordedAt stamp is correct, which I confirmed the long way in #940: a pending preflight that coalesces into a sibling's fan-out completes without running its own hold phase, covered by the holds that fan-out recorded. The comment now says this; when I first read it, it didn't, and that's what sent me looking.
Stack: this sits on armand/check-hold-fanout (#940), which sits on armand/check-hold-storage, with #866 further down. I've approved #866 and #940; this completes my pass over the stack. CLEAN here is relative to #940's branch, not main, so the merge order still matters.
Scope: I reviewed the operator-side gate and its interaction with the storage layer and #940's processor. I did not read the test additions closely.
Not blocking.
3 participants
Why this matters
Holding sibling PR checks is only a guardrail if it happens before the apply changes anything. A multi-hour copy/cutover started from the CLI must not race the flip: if the holds land late, a sibling PR can merge on a green check the apply is about to invalidate. This PR makes the stored holds a hard precondition of the apply itself — and only the stored holds. The gate waits on storage-only writes, never on the code-host rendering of them, so a code-host outage can never block an apply — least of all the CLI apply mitigating an incident. It fails closed on any storage uncertainty. Stack 6/7, on top of #940.
What it does
gateApplyStartOnCheckPreflight), run when a driver claims an apply, before engine work:holds_recorded_at, or a completed request for preflights coalesced into a same-target sibling's fan-out. The code-host rendering (Check Run update, hold comment) retries separately and never blocks the start. A terminally failed request is re-armed withReopenForRetryand re-kicked.schemabot.merge_gate.preflight_gate_totalwithpassed/passed_render_pending/timeout/error):passed_render_pendingmeans the apply started on stored holds while the code-host rendering is still retrying — expected and healthy during a code-host outage; a sustainedtimeoutrate means the processor is not draining or storage is failing.pkg/api,pkg/storage,pkg/metrics) comments, logs, and metric docs describe that contract; GitHub vocabulary lives only in the adapter (pkg/webhook,pkg/github).Closing the loop: a commit pushed to a sibling PR while the apply is mid-flight would re-plan against the pre-apply schema and could mint a fresh green check — #942 closes that by storing such checks born held.
How it moves us toward the northstar
An apply's first observable effect is now telling every affected PR "this target is changing" — before a single row moves. Merge decisions and schema changes stop being able to race each other, and the dependency points the safe direction: the code host depends on SchemaBot's stored truth, never the other way around.
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