feat(storage): durable merge gate requests for schema-mutating applies - #867
feat(storage): durable merge gate requests for schema-mutating applies#867aparajon wants to merge 4 commits into
Conversation
…lies An apply reaching terminal success changes its target's live schema, which stales the stored plan check state of every other open PR planning against that target. This adds the storage layer the refresh guardrail is built on: a check_refresh_requests outbox (one idempotent row per apply, claimed under a rotating lease with bounded attempts, heartbeat, coalescing of same-target pending rows, a sweep join over completed applies missing a request, and a stuck-processing terminator), plus the check-store reverse index and the conditional fail-closed flip (head-SHA guarded, in-flight-apply guarded) that the fan-out will use. The integration job budget grows to fit the added container-backed tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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_requestsoutbox/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.
…ion fails The operator terminalizes a task-less operation and re-derives the parent apply's state as two separate writes, so the parent briefly reads running after the operation is already failed. Poll for the derived state instead of asserting it in the same instant the operation turns failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aparajon
changed the title
The durable fan-out table is named for what it actuates — sibling changes' merge gates — rather than the GitHub Checks vocabulary. The originating change identity is now code-host neutral: provider (default github) plus a provider-scoped change_key string replace the GitHub-shaped pull_request integer, so changes on other code hosts can originate applies without a schema change. WebhookProviderGitHub generalizes to ProviderGitHub, shared by every table that attributes rows to a code host. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4a57268 to
5bce323
Compare
|
🤖 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
Non-blocking
General suggestions
The one thing that could have broken, verifiedThe lease state machine in merge_gate_requests.go — the interaction of Verified correct
This review was generated by Claude Code (claude-fable-5). |
|
🤖 Review from Morgan's AI agent. Holding — +1164/-66 introducing a new storage table and a lease/claim protocol is the large-core-change case in his standing bar, and it's currently What's genuinely good, and worth not losing in the rebase:
The rebase is structural, not textual — this is the part I'd plan for. This PR adds And it will need to join the canonicalization convention. Four PRs merged into that same package since this branch was cut (#1213, #1216, #1217, #1218) fold identity keys — repository, database name, database type, environment — at every write boundary, because MySQL's One finding that's independent of the rebase.
This is the fourth PR in flight adding an index to an existing growth table (#1196 on |
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 placed for the wrong reason. I held this on 2026-09-01 citing size and "a new storage table and lease machinery" — which is a description of the PR, not a finding. Since then I've audited this store's methods from above while reviewing #866, #940, #941 and #942, so the honest thing is to review the foundation directly rather than let it sit a fifth day. It holds up.
The claim path is correct. ClaimNext does FOR UPDATE SKIP LOCKED with LIMIT 1 inside a READ COMMITTED transaction, bumps attempts, clears retry_after, and mints a fresh lease_token — so two workers never block on the same row, and every subsequent write is lease-token-guarded. Reflecting the claim onto the scanned struct instead of reloading is justified exactly as the comment says: the row was held under FOR UPDATE for the whole transaction.
The timestamp precision is matched to the column types, and that's not an accident. retry_after is datetime and its predicate uses CurrentTimestamp(TimestampPrecisionDefault); lease_expires_at is datetime(6) and uses TimestampPrecisionMicrosecond. Getting this wrong is how lease code develops sub-second races that only appear under load, and someone clearly thought about the truncation. Worth saying, because it's invisible work.
The indexes cover the queries. idx_merge_gate_claimable (state, retry_after, lease_expires_at, created_at) fronts the claim predicate and idx_merge_gate_target fronts PendingForTarget. Importantly, state leads, and completed matches none of the three claimable disjuncts — so the claim scan stays bounded by the live set no matter how many completed rows accumulate. The UNIQUE KEY on apply_id is what makes Record's idempotent (bool, error) contract real rather than advisory.
Finding: the pending branch of the claimable predicate has no attempts < cap, and that becomes load-bearing one PR up. The other two disjuncts both guard attempts < MaxMergeGateAttempts; state = pending stands alone. At this layer that's unreachable — pending is only produced by Record, which starts at the column default of 0 — so there's no bug here. But ReopenForRetry arrives upstack and returns a failed request to pending, and #941's apply gate calls it on every drive attempt that finds a terminally-failed request. If that reset also clears attempts, the cap is bypassed by construction; if it doesn't, the uncapped pending branch makes it moot anyway. Either way the attempt cap stops binding once the reopen path exists. I raised the same thing from the other end on #941 — flagging it here too because this predicate is where it would be cheapest to fix, and because a reader auditing the cap will start at this function and conclude it's enforced.
Nit: req.LeaseExpiresAt is an application-clock estimate of a database-clock value, which the comment says plainly. It degrades safely — app clock behind the DB means a late heartbeat, a stolen lease, and a lease-token error on the next write rather than two workers both believing they own the row — so this is fine. It's only worth knowing that the field is advisory and shouldn't later be used for anything that needs to agree with the database's own expiry check.
Nit: nothing prunes completed rows. The hot path is insulated per the index note above, and FindCompletedAppliesMissingRequest is lookback-bounded, so this is about table growth rather than correctness. A retention story eventually, not now.
On the two files that look out of place: the test.yaml timeout-minutes: 10 → 15 is reasonable cover for 475 new lines of storage tests, and the WebhookProviderGitHub → ProviderGitHub rename is a genuine generalization — merge_gate_requests carries its own provider column defaulting to github, so the constant stops belonging to webhooks. That rename is also the whole explanation for the wide, shallow churn across a dozen test files, which is what made the diff look bigger than it is.
Merge state: this is DIRTY and has been for several days. My approval covers the content at 5bce323c; the conflict resolution is unreviewed, and since a rebase moves the head it may dismiss this approval anyway. Worth prioritizing the rebase — #868, #939, #940, #941 and #942 are all approved and stacked above this, so this branch is the only thing between that stack and merging.
Scope: I reviewed the store, the schema, the claimable predicate and the rename. I did not read the 475 test lines closely.
Not blocking.
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:
merge_gate_requeststable and store — a durable outbox mirroring thewebhook_eventslease pattern: one idempotent row per apply (Record), claimed under a rotating lease with bounded attempts (ClaimNextviaFOR UPDATE SKIP LOCKED),Heartbeat, lease-token-conditionalMarkCompleted/MarkFailed, same-target coalescing of pending rows (CompletePendingCoalesced), a sweep join that backfills completed applies missing a request (FindCompletedAppliesMissingRequest), and a stuck-processing terminator.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.GetByTarget(environment, database type, database) backed by a newidx_env_db, andMarkBlockedForFailedRefresh— a conditional, fail-closed flip that only lands when the stored head SHA is still current and no apply is in flight.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
mainas its base merges.🤖 Generated with Claude Code