feat(storage): durable merge gate requests for schema-mutating applies

[aparajon]

Why this matters: When an apply reaches terminal success, the target database's live schema has changed — and every other open PR that planned against that target is now holding stale check state. A stale green check on a tier-0 safety gate is the failure mode this workstream closes. This PR is the storage foundation of the merge gate guardrail (stack 1/7).

What it does:

• Adds the merge_gate_requests table and store — a durable outbox mirroring the webhook_events lease pattern: one idempotent row per apply (Record), claimed under a rotating lease with bounded attempts (ClaimNext via FOR UPDATE SKIP LOCKED), Heartbeat, lease-token-conditional MarkCompleted/MarkFailed, same-target coalescing of pending rows (CompletePendingCoalesced), a sweep join that backfills completed applies missing a request (FindCompletedAppliesMissingRequest), and a stuck-processing terminator.
• Rows carry code-host-neutral change identity — provider, repository, change_key (a PR number rendered as a string on GitHub; other providers use their own change handle) — so the core storage layer never assumes GitHub.
• Extends the check store with the reverse index the fan-out needs: GetByTarget (environment, database type, database) backed by a new idx_env_db, and MarkBlockedForFailedRefresh — a conditional, fail-closed flip that only lands when the stored head SHA is still current and no apply is in flight.
• Fits the Integration Tests CI budget to the grown container-backed suite.

apply completes ──▶ merge_gate_requests (pending)              [#868: drive tail]
                        │ ClaimNext (SKIP LOCKED, lease)
                        ▼
                    processing ──▶ completed / failed          [#866: processor]
                        ▲
   sweep backfill ──────┘   (completed applies missing a request)

How it moves us toward the northstar: Declarative schema GitOps is only safe if stored check state always reflects the live target. A durable, at-least-once merge gate outbox means no schema mutation — GitHub-driven or CLI-driven — can leave a sibling PR holding a stale verdict.

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

[Copilot]

Pull request overview

Introduces the storage-layer foundation for durable “check refresh” requests, enabling at-least-once re-planning of stored check state for sibling PRs whenever an apply successfully mutates a target’s live schema.

Changes:

• Added a durable check_refresh_requests outbox/table + MySQL store implementing claim/lease/heartbeat/completion/failure semantics with coalescing and backfill sweep support.
• Extended stored check state with a target-wide reverse index (GetByTarget) and a head-SHA-conditional, fail-closed flip for refresh failures (MarkBlockedForFailedRefresh).
• Added integration tests for the new store and adjusted CI timeout to accommodate the expanded integration suite.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/storage/types.go	Defines check refresh request types, states, and attempt budget constant.
pkg/storage/storage.go	Extends storage interfaces with CheckRefreshRequestStore and new check-store methods.
pkg/storage/mysqlstore/storage.go	Wires the new check refresh request store into the MySQL storage implementation.
pkg/storage/mysqlstore/checks.go	Implements GetByTarget and MarkBlockedForFailedRefresh in the MySQL check store.
pkg/storage/mysqlstore/check_refresh_requests.go	Implements the durable check refresh request MySQL store (record/claim/lease/heartbeat/complete/fail/sweep).
pkg/storage/mysqlstore/check_refresh_requests_test.go	Adds integration tests covering request lifecycle semantics and the new check-store APIs.
pkg/storage/errors.go	Adds store-level sentinel errors for not-found and lease-loss cases.
pkg/schema/mysql/checks.sql	Adds idx_env_db to support target-wide check lookups.
pkg/schema/mysql/check_refresh_requests.sql	Adds the check_refresh_requests table and supporting indexes.
pkg/api/handlers_test.go	Updates storage mock to satisfy the expanded Storage interface.
.github/workflows/test.yaml	Increases workflow timeout to fit the expanded integration test runtime.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/867, 5bce323.

Verdict: 9 findings — 1 blocking (rebase-and-port onto main's sqlstore layout + full CI), 6 non-blocking, 2 suggestions.

Blocking

1. The PR targets a pre-refactor storage layout; it needs a rebase onto main with an internal/sqlstore port, a pkg/schema/postgres twin, and a green full CI run before merge. Since this PR's base, main (refactor(storage): depend on storage.Storage instead of *mysqlstore.Storage #947/feat(storage): add PostgreSQL EnsureSchema bootstrapper #946) moved the storage implementation into dialect-generic pkg/storage/internal/sqlstore — origin/main:pkg/storage/mysqlstore/mysqlstore.go is now a one-file shim (type Storage = sqlstore.Storage with var _ storage.Storage = (*Storage)(nil)) — and added 13 per-table pkg/schema/postgres/*.sql twins plus internal/sqlstore/parity_test.go cross-dialect coverage. This PR implements the store as package mysqlstore (merge_gate_requests.go#L3) with a mysql-only schema file, and widens the required interfaces — storage.go#L66 MergeGateRequests() MergeGateRequestStore, storage.go#L183 GetByTarget, storage.go#L192 MarkBlockedForFailedRefresh — with no sqlstore implementation. git merge-tree already predicts a content conflict in pkg/storage/internal/sqlstore/storage.go, and full CI never ran at this head (only DCO/Semgrep/zizmor completed; 3 of 32 checks). Failure scenario: resolved without a real port, internal/sqlstore.Storage no longer satisfies storage.Storage → repo-wide compile failure; a mechanical file-move compiles but leaves no pkg/schema/postgres/merge_gate_requests.sql and no parity coverage, so the durable merge-gate outbox silently has no PostgreSQL path (parity suite fails, or the table is absent on PG deployments and every Record() errors at runtime).

Non-blocking

1. MergeGateRequestStore omits the attempt-refunding Release its webhook template has, so shutdown-interrupted claims permanently consume retry budget. ClaimNext burns an attempt at claim time (merge_gate_requests.go#L149 SET state = ?, attempts = attempts + 1, ...), the claim predicate stops reclaiming at attempts < ? (merge_gate_requests.go#L90-L91, cap 5), and TerminateStuckProcessing then terminally fails the row — which the backfill sweep cannot rescue (it requires r.id IS NULL, merge_gate_requests.go#L284). The sibling WebhookEventStore.Release doc (storage.go#L283-L288) states the exact rationale: "an interrupted claim must not consume retry budget, or repeated deploy restarts could terminally fail a delivery". MergeGateRequestStore copied everything but that. Failure scenario: a long fan-out in flight during routine rolling deploys is claim-interrupted 5 times with zero real failures → the request terminally fails, the fan-out never runs, and sibling PRs keep stale green merge gates — the exact tier-0 failure this workstream exists to close. No consumer exists at this head, so adding Release alongside the feat(github): re-plan sibling PR checks when an apply changes a target schema #866 processor is a safe follow-up.

2. The contract and backfill sweep cover only fully-completed applies, so a failed multi-op apply that already executed some DDL mutates the live schema without ever generating a merge gate request — undocumented as a scope exclusion. The sweep filters WHERE a.state = ? bound to Completed (merge_gate_requests.go#L282), and a multi-op apply with one completed op and one failed op derives to failed (apply.go#L99 if counts[Apply.Failed] > 0 wins over #L117). Failure scenario: op1 ALTER TABLE lands, op2 fails → live schema changed, apply state failed, invisible to both feat(api): record durable merge gate requests at apply drive tails #868's completed-only recording and this sweep → siblings planned against the pre-apply schema keep stale green checks — contradicting the PR body's "no schema mutation ... can leave a sibling PR holding a stale verdict". May be an accepted stack scope decision, but nothing documents it; at minimum the contract at storage.go#L315 should state the exclusion.

3. MarkBlockedForFailedRefresh unconditionally overwrites blocking_reason, laundering away the review-time deployment-drift sentinel. checks.go#L541 sets blocking_reason = ? with only head-SHA and in-progress-apply guards (#L546-L547); a drift-blocked row matches and is relabeled. The sentinel is load-bearing twice: UpsertPlanResult's not-evaluated CASE guard keys every column on it (checks.go#L131), and the apply-start drift refusal compares the exact sentinel (webhook/apply_check_records.go#L79). Failure scenario: a drift-blocked row gets relabeled by a failed re-plan → subsequent not-evaluated writes can clear the block without any rollup re-run, and the apply-start refusal no longer fires. Fail-closed initially (row stays blocked), but the drift invariant — "only a write that re-ran the rollup rewrites those columns" — is silently broken. No caller exists until feat(github): re-plan sibling PR checks when an apply changes a target schema #866; the guard belongs in this primitive before feat(github): re-plan sibling PR checks when an apply changes a target schema #866 merges.

4. MarkBlockedForFailedRefresh lacks the newer-applies guard its siblings carry, so a same-head completed apply can be stomped and its apply_id marker erased. The only guards are AND head_sha = ? and AND NOT (status = ? AND apply_id IS NOT NULL) with checkStatusInProgress (checks.go#L546-L547), and #L537 does SET apply_id = NULL. Failure scenario: the feat(github): re-plan sibling PR checks when an apply changes a target schema #866 processor reads a row via GetByTarget; the PR's own apply starts and completes (CompleteForApply writes completed/success + apply_id at the same head); the processor's failed re-plan then stomps it to blocked and nulls apply_id. The mis-flip itself is fail-closed, but the nulled marker means MarkStalePlanSuccessful's apply_id IS NULL guard (checks.go#L249) can later convert an applied-then-removed change to success by cleanup alone. Siblings CompleteForApply/MarkActionRequiredForApply carry a newer.id > ? NOT EXISTS guard for exactly this TOCTOU class (checks.go#L366, #L435).

5. MarkBlockedForFailedRefresh infers "flipped" purely from RowsAffected with no 0-rows re-read, so under production changed-rows semantics an idempotent retry is indistinguishable from "row preserved". checks.go#L561 return rows > 0, nil. The package's own convention doc (mysql_test.go#L51-L54) warns that production connections report changed rows, and this same PR's mergeGateLeaseResult (merge_gate_requests.go#L322) plus the pre-existing MarkStalePlanSuccessful both re-read on 0 rows to disambiguate. Failure scenario: the processor flips a check, commits, crashes before posting the GitHub check-run update; the retry writes identical values → RowsAffected 0 → (false, nil); a caller treating false as "skip the GitHub update" leaves GitHub showing the stale green verdict while the DB says blocked. TestCheckStore_MarkBlockedForFailedRefresh runs only on the clientFoundRows testDB so cannot catch this.

6. The merge gate lease tests never run under production changed-rows semantics, leaving the mergeGateLeaseResult owned-but-unchanged fallback untested. The fallback branch if currentToken.Valid && currentToken.String == leaseToken { return nil } (merge_gate_requests.go#L338) is reachable only under changed-rows semantics; the "same-token retry is a no-op" assertion (merge_gate_requests_test.go#L217) passes trivially on the clientFoundRows connection without exercising it. The webhook analog pins SET timestamp and opens testDSNChangedRows for exactly these paths (TestWebhookEventStore_TerminalWritesAreIdempotentOnRetry, TestWebhookEventStore_HeartbeatTreatsUnchangedMatchingLeaseAsSuccess); grep shows zero testDSNChangedRows uses in merge_gate_requests_test.go. Correct today, but a future edit to mergeGateLeaseResult could regress the production path silently.

General suggestions

• PendingForTarget/CompletePendingCoalesced rely on caller ordering discipline encoded only in a comment (storage.go#L333-L337): the query has no created-at/id cutoff (merge_gate_requests.go#L180), so a processor that lists after its re-plan snapshot could coalesce-complete a request recorded mid-fan-out whose schema change never triggered a refresh. A cutoff parameter captured at claim time (max id / claim timestamp) would enforce the invariant at the storage layer instead of by feat(github): re-plan sibling PR checks when an apply changes a target schema #866-caller discipline.
• Field mergeGatees breaks the field-matches-store-name convention — storage.go#L27 mergeGatees *mergeGateRequestStore (also lines 46, 112); every sibling field is named after its store (controlRequests, webhookEvents, checks). mergeGateRequests would match, and the irregular name will be missed by grep sweeps for the ongoing merge-gate rename that each stacked PR carries.

The one thing that could have broken, verified

The lease state machine in merge_gate_requests.go — the interaction of mergeGateClaimablePredicate (pending OR failed+retry_after-elapsed+under-cap OR processing+lease-expired+under-cap, #L87-L103), ClaimNext's two-step FOR UPDATE SKIP LOCKED claim with token rotation (#L105-L174), and mergeGateLeaseResult's 0-rows token-recheck fallback (#L322). A flaw here silently drops or double-runs merge gate fan-outs, i.e. leaves stale green tier-0 checks — the exact failure the workstream exists to close. Proved sound three ways: (a) line-by-line diff against the battle-tested webhook_events.go analog — the only deltas are intentional (no started_at, no Release (non-blocking #1), a single failed state keyed on retry_after NULL-ness whose terminal branch is provably unclaimable); (b) direct verification of the concurrency invariants — the claim tx holds the row lock through commit so the post-commit struct reflection cannot drift, reclaim always rotates lease_token so a stale owner's write affects 0 rows and the token re-read returns ErrMergeGateLeaseLost, and TerminateStuckProcessing NULLs the token so a zombie driver's late heartbeat also loses; (c) executing the full new integration suite against a real MySQL 8.0 testcontainer at this exact head (ok github.com/block/schemabot/pkg/storage/mysqlstore 9.7s) — which matters because CI never ran it. The residual risk is not the state machine but the landing surface: none of this code exists in main's new internal/sqlstore layer (the blocking finding); the remaining proof obligation is the rebase-and-port plus a green full 32-check run.

Verified correct

• CI status fact: only 3 of 32 checks (DCO, Semgrep, zizmor) completed at head 5bce323 — the full suite never executed, so every no-behavior-change claim below was re-verified locally rather than taken from CI. The PR is also currently CONFLICTING with main (git merge-tree predicts content conflicts in pkg/storage/internal/sqlstore/storage.go and pkg/webhook/durable_check_run_test.go).
• All 11 new integration tests (9 TestMergeGateStore_* plus TestCheckStore_GetByTargetSpansRepositories and TestCheckStore_MarkBlockedForFailedRefresh) pass locally against a real MySQL 8.0 testcontainer at this head.
• Full toolchain verification at this head: go build ./..., go vet ./..., go vet -tags=integration ./..., go vet -tags=e2e ./... all pass — the Storage interface widening at storage.go#L66 reached every implementer, including the api mockStorage and all webhook test fakes via interface embedding.
• The WebhookProviderGitHub → ProviderGitHub rename is a pure compile-time constant rename with identical value "github"; grep over the whole worktree returns zero stale references across the 40+ call sites.
• The claimable predicate is internally consistent with the single-failed-state design: 5 placeholders bound in exact arg order with correct timestamp precisions; terminal failure (retry_after NULL) is never claimable; both reclaim branches are gated on attempts < MaxMergeGateAttempts; each branch has a dedicated integration test.
• TerminateStuckProcessing (merge_gate_requests.go#L303) is a field-for-field mirror of the webhook version, including lease/retry_after clearing, COALESCE'd completed_at, and token NULLing so a zombie's late heartbeat gets lease-lost.
• SQL/scan consistency: mergeGateColumns order matches Record's INSERT list and scanMergeGateRequestInto's Scan order; nullable columns use sql.Null*; lease_expires_at datetime(6) vs NOW(6) and retry_after datetime vs second-precision NOW() match webhook conventions; table ENGINE/CHARSET/COLLATE matches all 13 sibling schema files.
• Schema plumbing is automatic: both the test harness (applyTestSchema) and production readEmbeddedSchemaFiles glob the embedded pkg/schema/mysql dir, and clearTables iterates SHOW TABLES — merge_gate_requests.sql needs no registration and cannot leak state between tests.
• Indexes cover every new query with no redundancy: idx_merge_gate_apply UNIQUE(apply_id) backs Record's duplicate-key idempotency, GetByApplyID, and the sweep's join; idx_merge_gate_claimable backs ClaimNext; idx_merge_gate_target backs PendingForTarget; the new checks idx_env_db backs the repo-less GetByTarget and is not a left-prefix of any existing composite.
• MarkBlockedForFailedRefresh's mechanics are sound per its stated contract: 13 placeholders = 13 args in order; the flip direction is fail-closed (blocking), never converting uncertainty into a pass; all three predicate branches are covered by its test (the gaps above are guards/semantics beyond that contract).
• GetByTarget's repo-less semantics (CLI/gRPC applies carry no repository) are tested cross-repository in TestCheckStore_GetByTargetSpansRepositories.
• FindCompletedAppliesMissingRequest's LEFT JOIN ... r.id IS NULL cannot duplicate applies (apply_id UNIQUE), correctly excludes NULL completed_at via the interval comparison, and reuses the shared apply scan helpers; each exclusion is covered by its test.
• The rewritten integration/operator_test.go assertion preserves the parent-failed invariant via polling with state.IsState (the AGENTS.md-mandated comparison) and correctly loosens an assertion that raced the operator's separate parent-state re-derivation write.
• Stacked-consumer fit: feat(github): re-plan sibling PR checks when an apply changes a target schema #866's processor calls exactly and only the methods this PR defines, and feat(api): record durable merge gate requests at apply drive tails #868's drive tail uses Record/GetByApplyID with the documented recorded=false idempotency contract; no stack-order inversion — this head builds standalone.
• The CI timeout bump (test.yaml 10 → 15 min) matches the genuinely added container-backed test work (a 475-line MySQL-testcontainer suite), not flake-masking; no in-test poll deadlines were increased.

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