feat(github): re-plan sibling PR checks when an apply changes a target schema

[aparajon]

Why this matters: When an apply reaches terminal success, every other open PR with stored plan check state against that (environment, database type, database) target is holding a verdict computed against a schema that no longer exists. This is acute for CLI-driven applies with no PR surface — nothing on GitHub even hints the target moved — and a stale green check on a tier-0 safety gate is exactly the failure this guardrail closes. Stack 3/7, on top of #868 (drive-tail recording) and #867 (storage).

What it does:

• The webhook processor drains the durable merge gate requests the drive tails record. Each pass: sweep for completed applies missing a request (the outbox backstop), terminate requests wedged past the attempt cap, then claim pending requests under a rotating lease (FOR UPDATE SKIP LOCKED) and fan out.
• The drive tail's recorded-notifier kicks the driver awake, so a request drains immediately after the apply completes instead of waiting out the poll interval; the kick is an in-memory hint layered over the durable row, so losing it (pod boundary, stopped driver) only costs poll latency.
• The fan-out re-plans each sibling PR's stored check at its current head, appending an attribution note (re-planned: schema for <db> in <env> changed (apply <id> by <caller>)) so a reviewer knows why an unchanged PR was re-planned. It skips the originating change, rows owned by an in-flight apply (a started apply stays authoritative), closed PRs, targets the PR no longer manages, and rows a newer head already superseded.
• Fail closed: a failed re-plan durably flips the stored check to a blocking schema_changed_replan_failed conclusion (head-SHA + not-in-flight conditional write) with a fixed sanitized summary — and the request still completes, because the block is a durable outcome, not a retry.
• Same-target pending requests coalesce into a single fan-out: a re-plan against the live schema covers every schema change recorded before it started.

        sweep (completed applies missing a request)
          │                                  ┌── kick (drive tail) ──┐
pending ──┴─▶ claim (lease) ◀── poll tick ──┴────────────────────────┘
                  │
                  └─▶ fan out over checks on (env, type, db)
                        ├─ sibling PR, head current ─▶ re-plan + attribution
                        ├─ re-plan fails ───────────▶ flip check blocking (fail closed)
                        ├─ in-flight apply row ─────▶ untouched (apply authoritative)
                        └─ originator / closed / superseded ─▶ skip (logged + counted)

• Observability: per-PR outcome and per-request outcome counters, stuck-termination counter, and triage-complete logs on every skip and failure path.
• Integration tests cover the sweep backfill, fan-out + originator skip + attribution, the fail-closed flip, the in-flight guard, coalescing, and the kick draining without a poll tick.

How it moves us toward the northstar: Stored check state now tracks the live target no matter where a schema change comes from — GitHub PR, CLI, or rollback — so merge gates never pass on a stale plan. The follow-up PRs (#939–#941) extend this processor to also hold sibling checks before an apply starts, closing the window while the apply is still running.

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

Adds a durable “check refresh” guardrail so that when an apply successfully changes a target’s live schema, SchemaBot re-plans (or fail-closes) other open PRs whose stored plan checks were computed against the old schema—covering PR, CLI/gRPC, and rollback-driven applies.

Changes:

• Introduces check_refresh_requests outbox + processor that leases, heartbeats, coalesces same-target requests, and fans out re-plans to sibling PRs (fail-closed on re-plan failure with a sanitized message).
• Records refresh requests from the operator drive tail on terminal success, plus a backstop sweep for crash gaps.
• Extends stored plan-check upsert to support refresh attribution notes, adds target-wide check lookup/indexing, and adds metrics + integration tests.

Reviewed changes

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

Show a summary per file

File	Description
pkg/webhook/vschema_only_check_integration_test.go	Updates test calls for new upsertPlanCheckRecord signature.
pkg/webhook/handler.go	Adds check refresh processor lifecycle fields and defaults.
pkg/webhook/check_runs.go	Adds a fixed fail-closed blocking reason/message for refresh re-plan failures.
pkg/webhook/check_refresh.go	Implements the check refresh processor (sweep, lease/heartbeat, fan-out, fail-closed path).
pkg/webhook/check_refresh_integration_test.go	End-to-end integration tests for drive-tail recording, sweep backfill, fan-out, fail-closed behavior, in-flight guard, and coalescing.
pkg/webhook/check_records.go	Adds refresh attribution support and introduces errPlanCheckHeadStale for racing-head detection.
pkg/storage/types.go	Adds CheckRefreshRequest type + constants and max-attempts budget.
pkg/storage/storage.go	Extends storage interfaces with CheckRefreshRequestStore, CheckStore.GetByTarget, and MarkBlockedForFailedRefresh.
pkg/storage/mysqlstore/storage.go	Wires MySQL storage to expose CheckRefreshRequests().
pkg/storage/mysqlstore/checks.go	Implements target-wide check lookup and fail-closed update with head-SHA + in-flight guards.
pkg/storage/mysqlstore/check_refresh_requests.go	Implements durable request recording/claiming/heartbeat/completion/failure/sweep queries.
pkg/storage/mysqlstore/check_refresh_requests_test.go	Integration tests for request leasing semantics, retry/terminal behavior, sweep selection, coalescing, and check-store helpers.
pkg/storage/errors.go	Adds durable check refresh request lease/not-found errors.
pkg/serve/serve.go	Starts/stops the check refresh processor alongside other server background work.
pkg/schema/mysql/checks.sql	Adds an index to support target-wide check lookups.
pkg/schema/mysql/check_refresh_requests.sql	Adds the new durable check_refresh_requests table.
pkg/metrics/metrics.go	Adds metrics for recording sources, record failures, PR fan-out outcomes, and request-level outcomes/terminations.
pkg/api/operator.go	Records refresh requests from operator terminal-success transitions (before control-request cleanup).
pkg/api/handlers_test.go	Updates mock storage to satisfy the extended storage interface.

---

💡 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/866, 6bc0242.

Verdict: 9 findings — 3 blocking (fan-out panics escape the recover, coalescing drops the originator re-plan, dead stale-head guard), 4 non-blocking, 2 suggestions.

Blocking

1. safeFanOutMergeGate's recover only covers the calling goroutine; the per-PR wg.Go fan-out goroutines have no recover, so a panic in refreshPRPlanForTarget crashes the process — the advertised crash-loop protection is inert for exactly the work it exists to contain. The only recover() in the file is deferred on the parent goroutine (merge_gate.go#L392-L401) before return h.fanOutMergeGate(ctx, req); the per-PR re-plans run on child goroutines via wg.Go(func() { … h.refreshPRPlanForTarget(ctx, req, check) … }) (merge_gate.go#L448-L456) with no recover of their own — Go recover only intercepts panics on the goroutine where the deferred call runs. Failure scenario: one poison sibling PR whose re-plan deterministically panics crashes the entire server, taking down all webhook processing; the request row stays processing with an expired lease, another replica reclaims it and panics too — a fleet-wide crash-loop bounded only by the 5-attempt cap, the exact scenario the comment at merge_gate.go#L386-L390 ("a driver panic would otherwise crash-loop every replica on the same poison request") claims to prevent. Contrast durable_dispatch.go#L308, whose safeProcessDurableWebhookEvent processes on the same goroutine so its recover works; the existing goroutine-local helpers handler.go#L958 (recoverPanic) / handler.go#L979 (goSafe) show the shape of the fix.

2. Coalescing completes sibling requests from a different originating PR even though the driven fan-out skipped its own originator — permanently dropping the re-plan of the driven request's originating PR against the sibling's schema change, i.e. the stale-green check this guardrail exists to close. Siblings are captured by target only (merge_gate.go#L278; the query at merge_gate_requests.go#L181 never compares Repository/ChangeKey), the driven fan-out skips exactly its own originator (merge_gate.go#L420 if isOriginatingChange(check, req) {), and every captured sibling is then completed unconditionally (merge_gate.go#L361). Failure scenario: PR#2's apply A completes (kick lost to a pod restart, a documented loss mode), PR#3's apply B completes on the same target before the next 30s tick; the drain claims A (ClaimNext orders ORDER BY created_at, id, merge_gate_requests.go#L123, so the unsafe order is the guaranteed order), skips PR#2 as originator, and coalesces B — whose own fan-out would NOT have skipped PR#2. PR#2's stored check, computed before B mutated the schema, keeps passing forever: requests are one-per-apply and B is completed, so nothing recovers it. The only coalescing test uses CLI-style requests with empty ChangeKey (matches nothing), so this case is untested. Fix direction: only coalesce siblings with the same (Repository, ChangeKey) as the driven request, or include the driven request's originator in the fan-out when a pending sibling has a different originator.

3. The merge-gate re-plan's skipped_superseded head-stale guard is structurally dead (cache-hit tautology), so a stale head-A re-plan can overwrite a racing synchronize's head-B check row via the head-unconditional UpsertPlanResult, wedging the PR's aggregate check. commandBootstrap attaches a request-scoped PR-info cache (bootstrap.go#L47); discovery seeds it (schema.go#L68 prInfo, err := ic.FetchPullRequest(...), schema.go#L100 HeadSHA: prInfo.HeadSHA); the guard's re-fetch at check_records.go#L165 is a cache hit, so if prInfo.HeadSHA != headSHA (check_records.go#L177) compares the discovery snapshot against itself and the errors.Is(err, errPlanCheckHeadStale) branch at merge_gate.go#L566 is unreachable from this path. UpsertPlanResult's WHERE has no head condition (checks.go#L163 guards only apply-owned in-progress rows). Failure scenario: developer pushes head B while the merge gate is mid-re-plan of head A; the synchronize auto-plan stores the head-B row, then the merge gate's upsert overwrites it with head-A data; the aggregate folds the stale row as a blocking placeholder ("Awaiting results for the latest commit", check_aggregate.go#L204) and the PR wedges until a manual re-plan or push. Fails closed, but the advertised TOCTOU protection does not exist from this path, and a stale driver overwrites newer state — against the repo's conditional-update review bar. The dead comparison pre-existed in base, but the merge-gate path, the sentinel, and the skipped_superseded reliance on it are this PR's delta. The repo already documents the exact fix: plan.go#L75-L77 "Use FetchPullRequestNoCache: the cached FetchPullRequest used by discovery would return the discovery-time HeadSHA, masking the race." — do a NoCache re-check before the upsert, or add a head-conditional UpsertPlanResult variant.

Non-blocking

1. driveClaimedMergeGate treats driver-context cancellation (pod shutdown) as fan-out failure, burning retry budget on every deploy that catches a claimed request. merge_gate.go#L298 if fanErr != nil { has no ctx.Err() check, and the WithoutCancel finishCtx (merge_gate.go#L295) guarantees the MarkFailed write lands despite cancellation — consuming the attempt ClaimNext already charged. The durable-webhook sibling in the same package handles exactly this via a Release refund (durable_dispatch.go#L216-L227), but MergeGateRequestStore has no Release method (only WebhookEventStore does, storage.go#L288). Under deploy churn, five interruptions terminally fail a request that never got one real attempt; the sweep cannot backfill it because FindCompletedAppliesMissingRequest requires r.id IS NULL. Also: the lease-lost branches of MarkFailed/MarkCompleted (merge_gate.go#L310, #L344) emit no events_total outcome, unlike every other terminal path. Non-blocking because the fix needs a new storage-layer Release method (stack follow-up), mitigated by the 5-attempt budget and 1-minute retry delay.

2. Run-context cancellation is inert for per-PR fan-out work, so the heartbeat contract comment is false past bootstrap and shutdown can block for many minutes. refreshPRPlanForTarget switches to prCtx from commandBootstrap (merge_gate.go#L491), rooted in context.Background() with a 2-minute timeout (bootstrap.go#L46); neither the wg.Go bodies nor refreshPRPlanForTarget re-check runCtx (the only runCtx-using step, resolveRepoWebhookInstallation, short-circuits on installation-cache hit), so the comment "On lease loss it cancels the run context so in-flight work stops" (merge_gate.go#L637) does not hold. Consequences: after lease loss the reclaiming driver re-plans the same PRs while the old driver's detached re-plans keep running (doubled GitHub/plan load; a late transient failure can flip a freshly-green check blocked — fail-closed but confusing); on shutdown StopMergeGateProcessor → mergeGateWg.Wait (merge_gate.go#L116) blocks up to ~2min × ceil(N/3) at mergeGatePRConcurrency = 3 (merge_gate.go#L47) — a 30-PR target ≈ 20 minutes, past pod termination grace, so the pod is SIGKILLed mid-write. Fix: derive the per-PR context from runCtx (preserving the timeout), or at minimum correct the comment.

3. No test covers refreshPRPlanForTarget's documented skip paths or the driver's failed-fanout retry branch. Zero test hits for errPlanCheckHeadStale/superseded, closed-PR skip (merge_gate.go#L505), or not-managed skip (merge_gate.go#L516); the seven integration tests cover record/sweep/replan/fail-closed/in-flight/coalesce/kick only, and the fail-closed test asserts completion, never the MarkFailed-with-retryAfter branch (merge_gate.go#L298-L309). The superseded path is the fragile one: it depends on the stale-head error keeping its %w wrap of the sentinel (check_records.go#L187, a plain non-wrapping error until this PR) — a refactor dropping the %w silently converts every benign head race into blockCheckForFailedRefresh, durably flipping healthy sibling PRs to action_required, and no test fails. The PR body documents these behaviors, and AGENTS.md ("Tests must prove documented behavior") requires focused tests or an explicit why-not callout. A test that stubs a slow plan and lands a newer-head write mid-re-plan would also expose Blocking Fix MySQL health checks to use TCP and add failure log capture #3.

4. The docs Blocking Reasons table is not updated with the new stable schema_changed_replan_failed blocking reason. This PR adds the value at check_runs.go#L118 but touches no docs; the table at docs/check-runs.md#L541 — the reference the doc says exists "for logs, metrics, and operator triage" — omits it, so an operator triaging the new fail-closed flip finds nothing. The table already omitted two pre-existing reasons (managed_dir_missing_config, review_time_deployment_drift); consider adding all three.

General suggestions

• Sustained GitHub unavailability during sibling schema discovery fails the check closed at merge_gate.go#L526 and completes the request with 4 of 5 claim attempts unused, so the PR author must manually re-plan. This is deliberate, precedented fail-closed design (client-level retryGitHubUnavailableRead already absorbs single 503s, and the code's own contract sanctions fail-closed for discovery uncertainty), but consider routing ghclient.IsUnavailableError discovery errors to the retryable path (return err) so a one-minute retry clears them instead of a durable action_required flip.
• Lease-owner strings keep the pre-rename feature name: merge_gate.go#L751 return fmt.Sprintf("%s/%d/check-refresh", hostname, os.Getpid()) and merge_gate_integration_test.go#L34, while the table, metrics, and logs all say merge gate. Cosmetic, but the stale token lands in the persisted lease_owner column operators grep during triage.

The one thing that could have broken, verified

The fan-out's concurrent stored-check writes racing other writers on the same row — in-flight applies, synchronize auto-plans, other merge-gate drivers. The in-flight-apply race is proven safe at the storage layer, not the snapshot layer: both UpsertPlanResult UPDATE branches carry AND NOT (status = ? AND apply_id IS NOT NULL) (checks.go#L136, #L163) and MarkBlockedForFailedRefresh adds AND head_sha = ? (checks.go#L546-L547), so a stale GetByTarget snapshot cannot stomp a started apply and the fail-closed flip always loses to a newer head or a claiming apply. But the success-path write is NOT protected as advertised: tracing schemaResult.HeadSHA (cached FetchPullRequest at schema.go#L68) into upsertPlanCheckRecord's guard (check_records.go#L165, same request-scoped cache from bootstrap.go#L47; the NoCache fetch at merge_gate.go:500 does not seed it) shows the head re-check compares the discovery snapshot against itself — the superseded branch is unreachable and the head-unconditional UpsertPlanResult can overwrite a mid-re-plan synchronize's newer-head row (Blocking #3). The remaining safety is the aggregate's fail-closed stale-head placeholder, which wedges the PR instead of passing it stale. Definitive proof either way: a test that stubs a slow plan, lands a synchronize write for a new head during it, and asserts the stored row still carries the new head — today it would fail.

Verified correct

• CI effectively green: latest run passes all 32 checks including all matrix shards; the 3 stale "fail" rows (unexpanded matrix names, 0s duration) come from a superseded duplicate run.
• Attempt-cap alignment: maxMergeGateAttempts aliases storage.MaxMergeGateAttempts (merge_gate.go#L52); ClaimNext increments attempts transactionally and the claimable predicate requires attempts < cap, so the driver's terminal decision goes terminal on exactly the attempt after which the store stops handing the row out.
• Lease lifecycle: MarkCompleted/MarkFailed/Heartbeat are lease-token-conditional and idempotent; every finish path treats ErrMergeGateLeaseLost/NotFound as yield-to-other-driver; finishCtx = WithTimeout(WithoutCancel(ctx), 5s) so terminal bookkeeping survives driver cancellation.
• Heartbeat correctness: transient store errors keep the lease until expiry and retry next tick; intentional stop is distinguished via hbCtx.Err(); heartbeatErr is written strictly before the goroutine's deferred close(done) and read only after <-done — proper happens-before, no data race.
• Fan-out success + heartbeat failure is fail-safe: the driver skips MarkCompleted and leaves the row processing for lease-expiry reclaim rather than completing under uncertain ownership; re-planning the same PRs is idempotent-safe.
• Sweep parity and idempotency: sweepMergeGateRequests builds requests from the same apply fields as the drive tail, both scoped to state=completed only; Record is idempotent per apply via the unique apply_id key; the sweep query is index-covered and bounded by the 6h lookback.
• Coalescing ordering is sound for same-originator coverage: siblings are captured before the fan-out starts, a pending request only exists after its apply's terminal write, and CompletePendingCoalesced is pending-conditional so a concurrently-claimed sibling is finished by its own lifecycle. (The differing-originator gap is Blocking Initial port #2.)
• The drain loop cannot tight-spin: it stops on ctx error, empty claim, or claim error; MarkFailed sets retry_after one minute out; a row wedged by a failed MarkFailed stays leased until expiry.
• Kick wiring: mergeGateKick is a buffered-1 channel allocated in NewHandlerWithDispatch so KickMergeGate is safe before Start and a mid-pass kick is retained; OnMergeGateRecorded registration matches hasMergeGateConsumer's gate so GitHub-less servers record nothing and no pending rows leak.
• Lifecycle wiring: both single- and multi-app webhook runtimes wire start/stop, Start/Close nil-guard the hooks, the processor stops before storage closes, and StartMergeGateProcessor is idempotent and registers on the WaitGroup under the mutex so Start cannot race Stop's Wait.
• The fail-closed flip is head-SHA-conditional and sanitized: MarkBlockedForFailedRefresh refusals map to skipped_superseded; the stored block carries only the fixed message with the raw cause in server logs; caller-influenced text is routed through clampDriftSummary per the no-untrusted-markdown rule.
• isMergeGateNotManagedError matches only determinate typed not-managed answers (merge_gate.go#L697) — none can wrap a transient GitHub failure; the PR-state fetch before discovery uses FetchPullRequestNoCache and its failure stays retryable, not flipped.
• isOriginatingChange handles CLI/gRPC applies correctly: empty ChangeKey matches nothing, so CLI-originated requests exclude no PR and every sibling is re-planned.
• Aggregate and participant sentinel rows can never enter the fan-out: GetByTarget filters on a real (environment, database_type, database_name) while aggregate rows carry the sentinel in both type and name; requests reject empty type/name at Record.
• Reuse/altitude on the re-plan path is right: the fan-out reuses commandBootstrap, createManagedSchemaRequestFromPR, attachServerEnvironments, executePlanProtoWithTransientRetry, reviewTimeDrift, upsertPlanCheckRecord, and updateAggregateCheck rather than forking the plan pipeline; the only signature change (refreshNote) is threaded through all four call sites and is a no-op for pre-existing callers passing "".
• Deleted-line audit clean: the only deleted/rewritten non-test lines are serve.go doc comments (re-established), the upsertPlanCheckRecord signature + stale-head error (%w sentinel added, behavior-preserving for base callers), and a test header comment; the base drive-tail test is retained unmodified, so no coverage was lost.
• Metrics helpers follow repo conventions (addCounter/addCounterN, EnvironmentAttribute, actionable outcome comments); go 1.26 so sync.WaitGroup.Go is available.
• Conventions honored: driver/drive vocabulary throughout, no "migration"/"worker" in new code, sweep logs use apply.LogAttrs(), no internal numeric row IDs logged, commit scopes match AGENTS.md.
• The six new integration tests assert on specific values (exact blocking reason, message, attribution substring, attempts counts, untouched originator fields), use t.Context(), require/assert, and EventuallyWithT with the shared poll deadline — no raw sleeps.
• safeFanOutMergeGate's recover does convert same-goroutine fan-out panics (GetByTarget, target filtering, goroutine launch) into retryable failures — the containment gap is specifically the wg.Go child goroutines (Blocking Dependency Dashboard #1).

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