feat(storage): merge gate request kinds and an apply-in-flight check hold

[aparajon]

Why this matters

When an apply starts changing a target schema, every other open PR holding a green check against that same (environment, database type, database) target is now holding a verdict that may no longer be true — and nothing stops a merge from landing on it while the apply is still running. The existing merge gate guardrail only reacts after an apply completes. Closing the window before and during an apply requires new durable primitives, which this PR adds at the storage layer. Stack 4/7, on top of #866.

What it does

• Adds a kind column to merge_gate_requests, splitting the outbox into two request kinds:
  • settle — the existing post-terminal re-plan of sibling PR checks (all existing rows and writers).
  • preflight — a new request recorded before an apply's engine work starts, consumed by holding sibling PR checks action-required.
• The unique key becomes (apply_id, kind) so one apply can carry both requests through its lifecycle.
• Adds a holds_recorded_at stamp on the request: the preflight fan-out sets it once when every sibling hold is durably in place. It is a storage-only write — no code-host call in the path — so it lands even during a code-host outage, and it is the signal the operator gate starts the apply on (rather than full request completion, which includes the code-host rendering).
• New store operations the follow-up PRs consume:
  • MarkPreflightHoldsRecorded — lease-conditional, set-once stamp of holds_recorded_at.
  • ReopenForRetry — re-arms a terminally failed request so a gate can retry it.
  • ReopenTerminalPreflightsForActiveApplies — re-arms terminally failed preflights whose apply is still active, so a hold's code-host rendering keeps retrying after the apply has started on the stored holds.
  • FindTerminalAppliesWithPreflightMissingSettle — finds applies whose preflight held sibling checks but whose settle (the release) was never recorded.
  • HasActivePreflightedApplyOnTarget — detects a live hold on a target so a settle doesn't prematurely release it.
  • MarkBlockedForApplyInFlight — conditionally flips a stored check to blocked, refusing to touch rows owned by an in-progress apply or a moved head SHA (optimistic concurrency on the head SHA).

                     merge_gate_requests
 apply lifecycle      (apply_id, kind) UNIQUE
 ───────────────      ──────────────────────
 before engine   ──►  kind=preflight   hold sibling PR checks;
 work starts                           holds_recorded_at = the
      │                                storage-only "apply may start"
      │                                signal (consumed in follow-ups)
 terminal state  ──►  kind=settle      re-plan sibling PR checks,
                                       releasing the holds

How it moves us toward the northstar

Merging a PR should be safe exactly when its checks are green. This stack makes an in-flight apply on the same target visible in every sibling PR's checks, so git remains the interface and the check state is never a stale rendering of reality. This PR is pure storage; the processor fan-out and the operator gate build on it.

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 main as its base merges.

🤖 Generated with Claude Code

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/939, d72df1f.

Verdict: 8 findings — 1 blocking (upgrade-path kind='' backfill), 3 non-blocking, 4 suggestions.

Blocking

1. The new kind column has no DEFAULT and no backfill, so the live-table upgrade backfills every pre-existing row with kind='' — invisible to every kind-scoped query and re-triggering the settle fan-out for recent applies. merge_gate_requests.sql:4 is `kind` varchar(20) NOT NULL, while every other post-creation column on this table (lines 9-12: provider/repository/change_key/requested_by) carries a DEFAULT for exactly this ALTER-a-live-table path. EnsureSchema applies the diff via Spirit, which forces non-strict sql_mode on its connections, so MySQL backfills existing rows with the implicit '' — a value Record itself rejects and that no kind-scoped read matches: GetByApplyAndKind, PendingForTarget, and the sweep join r.kind = ? bound to settle. Failure scenario: base stack (feat(storage): durable merge gate requests for schema-mutating applies #867/feat(api): record durable merge gate requests at apply drive tails #868/feat(github): re-plan sibling PR checks when an apply changes a target schema #866) deploys first and an apply completes at T0 (settle row recorded); feat(storage): merge gate request kinds and an apply-in-flight check hold #939 deploys and the ALTER makes that row kind=''; the next sweep pass finds every apply completed within the 6h lookback "missing" a settle, Record inserts a duplicate row (the unique key is now (apply_id, kind), and ('', id) != ('settle', id)), and the sibling re-plan fan-out runs a second time per target — duplicate GitHub check churn — while legacy '' rows stay claimable via kind-agnostic ClaimNext but can never coalesce or be found again. Fix: NOT NULL DEFAULT 'settle' or a one-time backfill.

Non-blocking

1. ReopenForRetry re-arms retryable-failed rows too, contradicting its own contract and defeating backoff/attempt caps. The predicate at merge_gate_requests.go:97 is WHERE id = ? AND state = ? bound to failed only, but MarkFailed marks retryable failures with the same state='failed' (non-nil retry_after, per merge_gate.go:301-303), while the interface contract at storage.go:345 promises "false when it was not terminally failed". Scenario: the feat(api): gate apply start on confirmed sibling PR check holds #941 preflight gate polls during a GitHub outage and calls ReopenForRetry on a failed-retryable row (attempts=2 of 3, inside its backoff window) — each call zeroes attempts and clears retry_after, so the row never accumulates attempts toward the cap and never honors backoff. Fix at this layer: add AND retry_after IS NULL (terminal failures have NULL retry_after), or fix the contract doc. TestMergeGateStore_ReopenForRetry covers only pending and terminal-failed rows, never retryable-failed.

2. Drive-by test edit shrinks the retry_after skew margin from -time.Minute to -time.Second and deletes the comment explaining the DB-clock-skew guard, reintroducing the flake it prevented. merge_gate_requests_test.go:204 is now past := time.Now().Add(-time.Second); the base had -time.Minute plus a two-line comment ("place it far enough in the past to be immune to client/server skew"), removed in this PR's delta. retry_after is a second-precision datetime (merge_gate_requests.sql:18) written from the client clock but compared against the MySQL server clock in the claimable predicate; MySQL rounds fractional seconds (eating up to ~0.5s of the 1s margin), so any DB clock lagging the test client by ≥1s (Docker VM drift after host sleep, slow CI container) leaves retry_after in the DB's future — ClaimNext returns nil and require.NotNil(t, reclaimed) at line 216 flakes intermittently. Revert to -time.Minute and restore the comment.

3. PendingForTarget's new kind predicate has no discriminating test — dropping AND kind = ? keeps the whole suite green. The predicate at merge_gate_requests.go:207 is documented at storage.go:360-362 ("Kind-scoped because a preflight fan-out … does not do a settle's work"), yet the sole test call at merge_gate_requests_test.go:294 only ever sees settle-kind rows (the helper hardcodes MergeGateKindSettle; the three recorded rows differ only by environment). If a future refactor drops the predicate or mis-binds the arg, tests stay green, and once feat(github): hold sibling PR checks and comment when an apply preflights a target #940 lands a settle fan-out could coalesce-complete a pending preflight on the same target without ever holding the sibling checks it was recorded to hold. Fix: record a pending preflight row on the same target and assert it is absent from the settle-kind pending list (and vice versa).

General suggestions

• Efficiency: FindTerminalAppliesWithPreflightMissingSettle filters a.state IN (…) plus an a.updated_at range, but no index on applies contains updated_at — once feat(github): hold sibling PR checks and comment when an apply preflights a target #940 wires the sweep, each tick scans essentially every terminal apply ever recorded (idx_state_created_id serves only the state prefix), unlike the sibling FindCompletedAppliesMissingRequest, which is bounded by idx_completed_at_state. Cheap fix in this PR since the schema is self-bootstrapping: add an index covering (state, updated_at) or plain (updated_at). (Keying on completed_at instead is not equivalent: it is NULL for failed/cancelled applies, which this sweep must include.)
• Stale apply attribution on overlapping holds: the skipAlreadyHeld predicate at checks.go:582-584 matches on blocking_reason alone — a per-kind constant, with the apply identifier only in change_summary. When apply B preflights a target whose sibling checks are still held for finished apply A (reachable by design: an earlier settle defers to an active newer preflighted apply), the row is skipped untouched, so the visible check output keeps naming apply A while B runs, until B's settle re-plans. Check stays fail-closed blocking and feat(github): hold sibling PR checks and comment when an apply preflights a target #940's hold comment still posts per apply identifier, so this is triage/UX only — consider comparing (reason, apply identifier) or refreshing change_summary on same-reason holds.
• Test gap in TestCheckStore_MarkBlockedForApplyInFlight (merge_gate_requests_test.go:636): no case flips a row already blocked for a different non-empty reason, so a mutant predicate like (blocking_reason IS NULL OR blocking_reason = '') — skip any already-blocked row — survives all existing assertions, yet in feat(github): hold sibling PR checks and comment when an apply preflights a target #940 it would silently skip a failed-refresh-blocked sibling (flipped=false), suppressing the hold announcement. Add a case seeding blocking_reason='check_refresh_failed' and asserting flipped=true with the reason rewritten.
• Rename backslide: this PR's delta rewrites the base's "merge gate recording" back to the retired "refresh recording" at storage.go:389 and merge_gate_requests_test.go:316 — the latter doubly inconsistent since the next line uses the PR's own new "settle" vocabulary. (The "refresh re-plan" comment near storage.go:434 is fine — it documents the inherited MarkBlockedForFailedRefresh method.)

The one thing that could have broken, verified

Rewiring the existing settle outbox to be kind-scoped over an already-live table: EnsureSchema ALTERs merge_gate_requests in place — adding kind varchar(20) NOT NULL with no DEFAULT and widening the unique key from (apply_id) to (apply_id, kind) — while every read path this PR touches (the r.kind='settle' sweep join, GetByApplyAndKind, kind-scoped PendingForTarget coalescing) becomes blind to any row without kind='settle'. For a fresh database I verified it sound: Record enforces the kind enum, the (apply_id, kind) unique key preserves idempotency (settle duplicate rejected, same apply's preflight inserts as a distinct row, per test), both production writers pass Settle explicitly (drive tail pkg/api/operator.go and the backstop sweep at merge_gate.go:180), and insert/scan column order matches. I also verified git cat-file -e origin/main:pkg/schema/mysql/merge_gate_requests.sql fails — the table is not on main yet, so a from-scratch deploy creates it fresh with kind. The unsafe half is exactly the stack's own bottom-up deploy plan (Blocking #1): if #867/#868/#866 deploy first and accumulate rows, #939's ALTER backfills them with MySQL's implicit kind='' (Spirit forces non-strict sql_mode), producing duplicate settle re-records and double fan-outs for applies inside the 6h sweep lookback, and permanently orphaning legacy rows from every kind-scoped query. Proven safe by adding DEFAULT 'settle' (matching the four other post-creation columns on the same table) or a one-time UPDATE … SET kind='settle' WHERE kind='' backfill.

Verified correct

• CI effectively green at head d72df1f: latest run passes all 32 checks (3 stale fail rows are from a superseded duplicate run); go build ./... and go vet pass locally at head.
• Record validates kind before insert, rejecting empty and unknown kinds (merge_gate_requests.go:38-40) with table-driven negative tests — nothing can write kind='' going forward.
• Both production Record writers set Kind=MergeGateKindSettle explicitly (drive tail and backstop sweep); no other non-test MergeGateRequest constructions exist at head.
• Duplicate-key idempotency survives the unique-key change to (apply_id, kind): isDuplicateKeyError → recorded=false, with a test proving a settle duplicate is rejected while the same apply's preflight inserts as a distinct row.
• Record's INSERT column/placeholder/arg counts and order match (11 each), and scanMergeGateRequestInto's scan order matches mergeGateColumns with Kind third in both.
• markBlockedConditional with skipAlreadyHeld=false is byte-identical to the pre-PR MarkBlockedForFailedRefresh UPDATE (same SET list, head-SHA optimistic-concurrency guard, apply-owned exclusion), with error context re-wrapped at both callers — behavior-preserving refactor; the pre-existing TestCheckStore_MarkBlockedForFailedRefresh is retained.
• The skipAlreadyHeld predicate is NULL-safe and empty-string-safe: (blocking_reason IS NULL OR blocking_reason != ?) flips never-blocked (NULL) and ''-reason rows; TestCheckStore_MarkBlockedForApplyInFlight covers flip, already-held, stale-head, and apply-owned paths.
• PendingForTarget's added kind scope is threaded to its single production caller with req.Kind (merge_gate.go:279), so a settle fan-out can never coalesce-complete a preflight or vice versa once preflight writers arrive.
• FindCompletedAppliesMissingRequest's new AND r.kind = ? join term correctly narrows suppression to settle rows (a preflight-only completed apply should get a settle backfilled), and cross-sweep double-recording is idempotent via the unique key.
• FindTerminalAppliesWithPreflightMissingSettle binds args in placeholder order, joins on the unique (apply_id, kind) key (at most one preflight row per apply — no fan-out duplication), and keying the window on a.updated_at rather than completed_at is deliberate and safe (completed_at is NULL for failed/cancelled applies; updated_at can only widen the window).
• HasActivePreflightedApplyOnTarget reuses the pre-existing nonTerminalApplyStatePredicate and binds args in order; deliberately ignoring the preflight's own state fails in the conservative direction (checks stay held while an active apply exists), consistent with the fail-closed check-state rule; the target filter matches the idx_merge_gate_target index prefix.
• ReopenForRetry's clear-set (lease fields, retry_after, completed_at, attempts=0) makes the row match the claimable predicate's pending arm exactly; the ReopenForRetry-vs-ClaimNext race is safe (both single conditional statements, either ordering consistent).
• The GetByApplyID → GetByApplyAndKind rename left no stale call sites (remaining GetByApplyID hits are all TaskStore, a different interface); all 12 integration-test call-site migrations pass the settle kind matching what the code under test records; no 5-arg PendingForTarget callers remain.
• New store methods unconsumed in this PR (ReopenForRetry, MarkBlockedForApplyInFlight, HasActivePreflightedApplyOnTarget, FindTerminalAppliesWithPreflightMissingSettle) are deliberate stack layering per the PR body, each shipping with integration tests here — consistent with the add-new-alongside-old small-PR convention. ClaimNext/TerminateStuckProcessing/Heartbeat/MarkCompleted were correctly left kind-unaware.
• SQL schema conventions otherwise honored (canonical SHOW CREATE TABLE tail, backticked identifiers, unique key widened in place, no leftover redundant index); untyped string constants for the two kinds follow the repo's stored-enum convention; no forbidden "migration" terminology; TEMPLATES.md needs no regeneration (no rendered-output strings changed).
• New integration tests follow repo conventions (testify require/assert, t.Context(), scenario comments, specific-value assertions, no time.Sleep readiness waits); interface additions break no test doubles (all doubles embed the interface).

This review was generated by Claude Code (claude-fable-5).