diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index bc2cb237c..d59c7a9a4 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -1547,3 +1547,58 @@ func RecordMergeGateRecordFailure(ctx context.Context, database, environment str EnvironmentAttribute(environment), ) } + +// RecordMergeGatePROutcome counts per-PR outcomes of merge gate fan-out. +// Outcomes: +// - "refreshed": the PR's stored check state was re-planned against the +// mutated target schema. +// - "blocked_replan_failed": the re-plan failed, so the stored check was +// flipped to blocking (fail closed) — check the server logs for the +// re-plan error and re-plan the PR once the cause is fixed. +// - "skipped_in_flight": the PR's check row is owned by an in-flight apply, +// which stays authoritative; the row was left untouched. +// - "skipped_pr_closed": the PR is closed, so its stored checks no longer +// gate anything. +// - "skipped_not_managed": the PR no longer manages schema for the target +// (config removed or database dropped from the PR's schema files). +// - "skipped_superseded": a racing write (a synchronize that re-planned a +// newer head, or an apply that claimed the row) landed first and is +// authoritative; the refresh yielded to it. +func RecordMergeGatePROutcome(ctx context.Context, repository, database, environment, outcome string) { + addCounter(ctx, "schemabot.merge_gate.pr_refreshes_total", + "Total per-PR outcomes of merge gate fan-out after a target schema changed", "{refresh}", + attribute.String("repository", repository), + attribute.String("database", database), + EnvironmentAttribute(environment), + attribute.String("outcome", outcome), + ) +} + +// RecordMergeGateEventOutcome counts terminal outcomes of driving one +// durable merge gate request. Outcomes: +// - "completed": the fan-out refreshed (or safely skipped) every sibling PR. +// - "failed_retrying": the fan-out failed and the request will be retried. +// - "failed_terminal": the fan-out failed on its final attempt — sibling PR +// stored checks for the target may remain stale until their PRs re-plan. +// Check the server logs for the failing PR and re-plan it. +// - "lease_lost": the drive lost its lease mid-fan-out; another driver +// re-drives the request (re-planning the same PRs again is safe). +func RecordMergeGateEventOutcome(ctx context.Context, database, environment, outcome string) { + addCounter(ctx, "schemabot.merge_gate.events_total", + "Total terminal outcomes of driving durable merge gate requests", "{event}", + attribute.String("database", database), + EnvironmentAttribute(environment), + attribute.String("outcome", outcome), + ) +} + +// RecordMergeGateTerminatedStuck counts merge gate requests terminated +// by the stuck-processing sweep: rows wedged past the attempt cap with an +// expired lease (a driver hard-killed on its final attempt). Each terminated +// request means sibling PR stored checks for its target may remain stale — +// find the request's target in the server logs and re-plan the affected PRs. +func RecordMergeGateTerminatedStuck(ctx context.Context, terminated int64) { + addCounterN(ctx, terminated, "schemabot.merge_gate.terminated_stuck_total", + "Total merge gate requests terminated by the stuck-processing sweep", "{request}", + ) +} diff --git a/pkg/serve/serve.go b/pkg/serve/serve.go index 6d1a557e3..86f5574b2 100644 --- a/pkg/serve/serve.go +++ b/pkg/serve/serve.go @@ -90,6 +90,8 @@ type webhookRuntime struct { stopDurableWebhookDispatch func() drainInProcessWebhookWork func(context.Context) reconcileMissingSummaryComments func(context.Context) + startMergeGateProcessor func(context.Context) + stopMergeGateProcessor func() } func (r webhookRuntime) StartMissingSummaryReconciliation(ctx context.Context, logger *slog.Logger) { @@ -535,10 +537,11 @@ func (s *Server) MetricsHandler() http.Handler { } // Start launches the server's background work: the operator driver pool -// (dispatches queued applies and recovers stale ones), the remote-deployment -// health monitor, the webhook inbox monitor (emits durable-inbox depth/backlog -// metrics), and the pending-drops cleaner — all of which run until ctx is -// canceled or Close is called. It also kicks off a one-shot missing-summary +// (dispatches queued applies and recovers stale ones), the merge gate +// processor (re-plans sibling PR checks after a target's schema changes), the +// remote-deployment health monitor, the webhook inbox monitor (emits +// durable-inbox depth/backlog metrics), and the pending-drops cleaner — all of +// which run until ctx is canceled or Close is called. It also kicks off a one-shot missing-summary // reconciliation that, once started, runs to completion independently of ctx (it // repairs interrupted terminal comments and must not be cut short by a request // context); it runs before the operator so recovered applies attach observers @@ -548,6 +551,9 @@ func (s *Server) Start(ctx context.Context) { if s.webhook.startDurableWebhookDispatch != nil { s.webhook.startDurableWebhookDispatch(ctx) } + if s.webhook.startMergeGateProcessor != nil { + s.webhook.startMergeGateProcessor(ctx) + } s.svc.StartOperator(ctx) s.svc.StartRemoteDeploymentHealthMonitor(ctx) s.svc.StartWebhookInboxMonitor(ctx) @@ -555,7 +561,8 @@ func (s *Server) Start(ctx context.Context) { } // Close releases the resources the Server owns and returns all cleanup errors -// encountered, joined together. It stops the pending-drops cleaner, stops the +// encountered, joined together. It stops the pending-drops cleaner and the +// merge gate processor, stops the // operator (before closing the gRPC client it built, see below), shuts down // telemetry, closes that gRPC fallback client, and closes the service. svc.Close // stops the health monitor and closes the service's clients and storage (the @@ -564,6 +571,9 @@ func (s *Server) Start(ctx context.Context) { // after Start. func (s *Server) Close() error { s.svc.StopPendingDropsCleaner() + if s.webhook.stopMergeGateProcessor != nil { + s.webhook.stopMergeGateProcessor() + } if s.webhook.stopDurableWebhookDispatch != nil { s.webhook.stopDurableWebhookDispatch() } @@ -775,6 +785,8 @@ func buildSingleAppWebhookRuntime(serverConfig *api.ServerConfig, svc *api.Servi drainInProcessWebhookWork: handler.DrainInProcessWebhookWork, handler: handler, reconcileMissingSummaryComments: handler.ReconcileMissingSummaryComments, + startMergeGateProcessor: handler.StartMergeGateProcessor, + stopMergeGateProcessor: handler.StopMergeGateProcessor, }, nil } @@ -853,6 +865,8 @@ func buildMultiAppWebhookRuntime(serverConfig *api.ServerConfig, svc *api.Servic drainInProcessWebhookWork: handler.DrainInProcessWebhookWork, handler: handler, reconcileMissingSummaryComments: handler.ReconcileMissingSummaryComments, + startMergeGateProcessor: handler.StartMergeGateProcessor, + stopMergeGateProcessor: handler.StopMergeGateProcessor, }, nil } diff --git a/pkg/webhook/check_records.go b/pkg/webhook/check_records.go index c6ef0a88d..049372e17 100644 --- a/pkg/webhook/check_records.go +++ b/pkg/webhook/check_records.go @@ -2,6 +2,7 @@ package webhook import ( "context" + "errors" "fmt" "github.com/block/schemabot/pkg/apitypes" @@ -58,12 +59,18 @@ func (o reviewDriftOutcome) planDriftState() storage.PlanDriftState { } } +// errPlanCheckHeadStale reports that a plan check record was not stored +// because the PR head advanced past the planned head. The newer head's own +// plan is authoritative, so callers racing a synchronize (for example the +// merge gate fan-out) treat this as a benign skip rather than a failure. +var errPlanCheckHeadStale = errors.New("plan head is no longer the PR's current head") + // storePlanCheckRecord stores per-database check state after a plan is generated. // The state is used internally by the aggregate check to compute its overall status. // No per-database GitHub Check Run is created — only the aggregate is visible on the PR. // Returns the commit SHA used for the plan. Failures are non-fatal. func (h *Handler) storePlanCheckRecord(ctx context.Context, client *ghclient.InstallationClient, repo string, pr int, schema *ghclient.SchemaRequestResult, planResp *apitypes.PlanResponse, environment string, drift reviewDriftOutcome) (string, error) { - headSHA, _, err := h.upsertPlanCheckRecord(ctx, client, repo, pr, schema, planResp, environment, drift) + headSHA, _, err := h.upsertPlanCheckRecord(ctx, client, repo, pr, schema, planResp, environment, drift, "") return headSHA, err } @@ -71,7 +78,7 @@ func (h *Handler) storePlanCheckRecord(ctx context.Context, client *ghclient.Ins // plan and then reconciles same-head apply-owned stored check state when the manual // plan proves the target already matches the PR schema. func (h *Handler) storeManualPlanCheckRecord(ctx context.Context, client *ghclient.InstallationClient, repo string, pr int, schema *ghclient.SchemaRequestResult, planResp *apitypes.PlanResponse, environment string, drift reviewDriftOutcome) (string, bool, error) { - headSHA, check, err := h.upsertPlanCheckRecord(ctx, client, repo, pr, schema, planResp, environment, drift) + headSHA, check, err := h.upsertPlanCheckRecord(ctx, client, repo, pr, schema, planResp, environment, drift, "") if err != nil { return headSHA, false, err } @@ -134,7 +141,13 @@ func planCheckConclusion(hasChanges, hasPlanErrors, driftBlocked bool) string { } } -func (h *Handler) upsertPlanCheckRecord(ctx context.Context, client *ghclient.InstallationClient, repo string, pr int, schema *ghclient.SchemaRequestResult, planResp *apitypes.PlanResponse, environment string, drift reviewDriftOutcome) (string, *storage.Check, error) { +// upsertPlanCheckRecord verifies the planned head is still the PR's current +// head (returning errPlanCheckHeadStale when it is not) and upserts the +// per-database stored check state for the plan. refreshNote, when non-empty, +// is a pre-sanitized attribution line the merge gate fan-out appends to the +// stored change summary so the Change column says why an unchanged PR was +// re-planned; ordinary plan writes pass "". +func (h *Handler) upsertPlanCheckRecord(ctx context.Context, client *ghclient.InstallationClient, repo string, pr int, schema *ghclient.SchemaRequestResult, planResp *apitypes.PlanResponse, environment string, drift reviewDriftOutcome, refreshNote string) (string, *storage.Check, error) { headSHA := schema.HeadSHA if headSHA == "" { metrics.RecordStatusCheckOperation(ctx, metrics.StatusCheckOperation{ @@ -170,8 +183,8 @@ func (h *Handler) upsertPlanCheckRecord(ctx context.Context, client *ghclient.In Environment: environment, Status: "stale", }) - return headSHA, nil, fmt.Errorf("skip stale plan check record for repo %s pr %d environment %s database_type %s database %s: plan head SHA %s no longer matches current head SHA for PR %s", - repo, pr, environment, schema.Type, schema.Database, headSHA, prInfo.HeadSHA) + return headSHA, nil, fmt.Errorf("skip stale plan check record for repo %s pr %d environment %s database_type %s database %s: plan head SHA %s no longer matches current head SHA %s: %w", + repo, pr, environment, schema.Type, schema.Database, headSHA, prInfo.HeadSHA, errPlanCheckHeadStale) } hasChanges := planResp.HasChanges() @@ -184,8 +197,14 @@ func (h *Handler) upsertPlanCheckRecord(ctx context.Context, client *ghclient.In // the block rides on BlockingReason + Conclusion so a stored drift block is // legible and durable across write paths. changeSummary := summarizePlanChanges(schema, planResp, environment) + if refreshNote != "" { + changeSummary = appendRefreshNote(changeSummary, refreshNote) + } blockingReason := "" if driftBlocked { + // The drift summary owns the Change column when drift blocks: it names + // the deployment divergence the operator must resolve, which supersedes + // refresh attribution. changeSummary = drift.summary blockingReason = reviewTimeDeploymentDriftBlock.blockingReason } diff --git a/pkg/webhook/check_runs.go b/pkg/webhook/check_runs.go index b2a0ea5ad..a72f08136 100644 --- a/pkg/webhook/check_runs.go +++ b/pkg/webhook/check_runs.go @@ -108,6 +108,17 @@ var reviewTimeDeploymentDriftBlock = checkBlockReason{ message: "One or more deployments differ from the reviewed plan, or could not be confirmed to match it; reconcile the deployment drift or replan once the deployments match before this check can pass.", } +// schemaChangedReplanFailedBlock is used when the live schema of a check's +// target changed after its plan was computed (another apply reached terminal +// success there) and the merge gate fan-out could not re-plan the PR. The +// stored check fails closed: a plan computed against a schema that no longer +// exists must not keep passing. The raw re-plan error stays in the server +// logs; only this fixed message is rendered on the PR. +var schemaChangedReplanFailedBlock = checkBlockReason{ + blockingReason: "schema_changed_replan_failed", + message: "The live schema for this database changed after this plan was computed, and SchemaBot could not re-plan the PR against it. Re-run `schemabot plan` (or push a new commit) before this check can pass; see server logs for the re-plan failure.", +} + // noAllowedConfiguredEnvironmentsBlock is used when schema files changed but // the server-configured environments for the database do not overlap this // service's allowed_environments. SchemaBot cannot safely plan the schema diff --git a/pkg/webhook/handler.go b/pkg/webhook/handler.go index a2f648192..652459311 100644 --- a/pkg/webhook/handler.go +++ b/pkg/webhook/handler.go @@ -173,6 +173,20 @@ type Handler struct { webhookReconcileGrace time.Duration webhookReconcileMaxPages int + // Merge gate processor lifecycle (see merge_gate.go). The intervals + // have package defaults set at construction; tests override them directly. + mergeGatePollInterval time.Duration + mergeGateLeaseDuration time.Duration + mergeGateSweepLookback time.Duration + mergeGateMu sync.Mutex + mergeGateStop chan struct{} + mergeGateCancel context.CancelFunc + mergeGateWg sync.WaitGroup + // mergeGateKick wakes the driver to run a pass now instead of waiting + // for the next poll tick; buffered so one pending kick coalesces any + // number of concurrent recordings. + mergeGateKick chan struct{} + logger *slog.Logger priorEnvCheckMaxAttempts int priorEnvCheckRetryInterval time.Duration @@ -258,6 +272,10 @@ func NewHandlerWithDispatch(service *api.Service, ghClients github.ClientSet, we webhookReconcileLookback: defaultWebhookReconcileLookback, webhookReconcileGrace: defaultWebhookReconcileGrace, webhookReconcileMaxPages: defaultWebhookReconcileMaxPages, + mergeGatePollInterval: defaultMergeGatePollInterval, + mergeGateLeaseDuration: defaultMergeGateLeaseDuration, + mergeGateSweepLookback: defaultMergeGateSweepLookback, + mergeGateKick: make(chan struct{}, 1), priorEnvCheckMaxAttempts: defaultPriorEnvCheckMaxAttempts, priorEnvCheckRetryInterval: defaultPriorEnvCheckRetryInterval, } @@ -344,6 +362,12 @@ func NewHandlerWithDispatch(service *api.Service, ghClients github.ClientSet, we obs.OnTerminal(apply, tasks) return nil } + + // Wake the merge gate processor as soon as a drive tail records a + // request, so sibling PR checks re-plan without waiting for the next + // poll tick. The durable request row stays the source of truth: a + // kick lost to a pod boundary only costs poll latency. + service.OnMergeGateRecorded = h.KickMergeGate } return h diff --git a/pkg/webhook/merge_gate.go b/pkg/webhook/merge_gate.go new file mode 100644 index 000000000..54ad784b6 --- /dev/null +++ b/pkg/webhook/merge_gate.go @@ -0,0 +1,752 @@ +// merge_gate.go drives the merge gate guardrail: when an apply reaches +// terminal success on a (environment, database type, database) target, every +// other open PR with stored plan check state against that target planned +// against a schema that no longer exists. The operator drive tail (and a +// backstop sweep here) records a durable merge_gate_requests row, and this +// processor consumes it: it finds the sibling PRs' stored check state through +// the checks reverse index, re-plans each PR against the live schema, and — +// when a re-plan fails — fails the stored check closed so a stale plan can +// never keep passing. CLI/gRPC applies carry no PR surface at all, so this +// processor is the only path that keeps their targets' sibling PR checks +// honest. +package webhook + +import ( + "context" + "errors" + "fmt" + "os" + "runtime/debug" + "sync" + "time" + + "github.com/block/schemabot/pkg/api" + ghclient "github.com/block/schemabot/pkg/github" + "github.com/block/schemabot/pkg/metrics" + "github.com/block/schemabot/pkg/storage" + "github.com/block/schemabot/pkg/webhook/action" +) + +const ( + defaultMergeGatePollInterval = 30 * time.Second + defaultMergeGateLeaseDuration = 2 * time.Minute + + // defaultMergeGateSweepLookback bounds the backstop sweep over completed + // applies missing a merge gate request. The sweep exists for the crash window + // between an apply's terminal write and the drive tail's recording, plus + // process downtime; requests are unique per apply, so re-sweeping the same + // window is a no-op. + defaultMergeGateSweepLookback = 6 * time.Hour + + // mergeGateRetryDelay is the pause before a failed fan-out is reclaimed. + mergeGateRetryDelay = time.Minute + + // mergeGatePRConcurrency bounds how many sibling PRs one fan-out + // re-plans at once, so a hot target with many open PRs cannot saturate the + // GitHub API or the plan engine. + mergeGatePRConcurrency = 3 + + // maxMergeGateAttempts aliases the store's claim ceiling so the drive + // records a retryable failure as terminal on the same attempt at which + // ClaimNext would stop handing the request out. + maxMergeGateAttempts = storage.MaxMergeGateAttempts +) + +// Per-PR fan-out outcomes recorded via metrics.RecordMergeGatePROutcome. +const ( + mergeGateOutcomeRefreshed = "refreshed" + mergeGateOutcomeBlockedReplan = "blocked_replan_failed" + mergeGateOutcomeSkippedInFlight = "skipped_in_flight" + mergeGateOutcomeSkippedPRClosed = "skipped_pr_closed" + mergeGateOutcomeSkippedNotManaged = "skipped_not_managed" + mergeGateOutcomeSkippedSuperseded = "skipped_superseded" +) + +// StartMergeGateProcessor starts the background driver that consumes +// durable merge gate requests. Idempotent; StopMergeGateProcessor stops +// it and waits for the in-flight pass to finish. +func (h *Handler) StartMergeGateProcessor(ctx context.Context) { + if h.mergeGateStore() == nil { + h.logger.Warn("merge gate processor not started: storage is unavailable; sibling PR checks will go stale after applies until it recovers") + return + } + + h.mergeGateMu.Lock() + if h.mergeGateStop != nil { + h.mergeGateMu.Unlock() + h.logger.Info("merge gate processor already running") + return + } + stop := make(chan struct{}) + driverCtx, cancel := context.WithCancel(ctx) + h.mergeGateStop = stop + h.mergeGateCancel = cancel + // Register on the WaitGroup while the mutex is held so Start cannot race a + // concurrent Stop's Wait. + h.mergeGateWg.Go(func() { + h.mergeGateDriver(driverCtx, stop) + }) + h.mergeGateMu.Unlock() + + h.logger.Info("merge gate processor started", + "interval", h.mergeGatePollInterval, + "lease_duration", h.mergeGateLeaseDuration, + "sweep_lookback", h.mergeGateSweepLookback) +} + +// StopMergeGateProcessor stops the merge gate driver and waits for the +// in-flight pass to finish its current drive. +func (h *Handler) StopMergeGateProcessor() { + h.mergeGateMu.Lock() + if h.mergeGateStop == nil { + h.mergeGateMu.Unlock() + h.logger.Debug("merge gate processor stop requested but it is not running") + return + } + stop := h.mergeGateStop + cancel := h.mergeGateCancel + h.mergeGateStop = nil + h.mergeGateCancel = nil + h.mergeGateMu.Unlock() + + close(stop) + if cancel != nil { + cancel() + } + h.mergeGateWg.Wait() + h.logger.Info("merge gate processor stopped") +} + +// KickMergeGate wakes the merge gate driver to run a pass now instead +// of waiting for the next poll tick. Non-blocking: when a kick is already +// pending, the coming pass drains the new request too. The durable request +// row is the source of truth — a kick that lands with no driver running, or +// on a different pod than the one that will claim the request, only costs +// poll latency, never the refresh. +func (h *Handler) KickMergeGate() { + select { + case h.mergeGateKick <- struct{}{}: + default: + } +} + +func (h *Handler) mergeGateDriver(ctx context.Context, stop <-chan struct{}) { + owner := mergeGateLeaseOwner() + ticker := time.NewTicker(h.mergeGatePollInterval) + defer ticker.Stop() + + h.logger.Debug("merge gate driver started", "lease_owner", owner) + h.runMergeGatePass(ctx, owner) + + for { + select { + case <-stop: + h.logger.Debug("merge gate driver stopping") + return + case <-ctx.Done(): + h.logger.Debug("merge gate driver context cancelled") + return + case <-h.mergeGateKick: + h.logger.Debug("merge gate driver woken by recorded-request kick") + h.runMergeGatePass(ctx, owner) + case <-ticker.C: + h.runMergeGatePass(ctx, owner) + } + } +} + +// runMergeGatePass runs one full processor pass: backfill requests the +// drive tails missed, terminalize requests wedged past the attempt cap, then +// claim and drive requests until none remain claimable. +func (h *Handler) runMergeGatePass(ctx context.Context, owner string) { + h.sweepMergeGateRequests(ctx) + h.terminateStuckMergeGateRequests(ctx) + h.drainMergeGateRequests(ctx, owner) +} + +// sweepMergeGateRequests backfills merge gate requests for completed applies +// that have none — the applies table is the outbox, so a pod crash between an +// apply's terminal write and its drive-tail recording cannot lose the fan-out. +func (h *Handler) sweepMergeGateRequests(ctx context.Context) { + store := h.mergeGateStore() + applies, err := store.FindCompletedAppliesMissingRequest(ctx, h.mergeGateSweepLookback) + if err != nil { + h.logger.Error("merge gate sweep failed to find completed applies missing a merge gate request; drive-tail gaps stay unbackfilled until the next pass", "error", err) + return + } + for _, apply := range applies { + recorded, err := store.Record(ctx, &storage.MergeGateRequest{ + ApplyID: apply.ID, + ApplyIdentifier: apply.ApplyIdentifier, + Environment: apply.Environment, + DatabaseType: apply.DatabaseType, + DatabaseName: apply.Database, + Repository: apply.Repository, + ChangeKey: storage.ChangeKeyForPullRequest(apply.PullRequest), + RequestedBy: apply.Caller, + }) + if err != nil { + // Each apply's backfill is independent; a failed one is retried on + // the next sweep pass. + h.logger.Error("merge gate sweep failed to backfill a merge gate request for a completed apply", + append(apply.LogAttrs(), "error", err)...) + metrics.RecordMergeGateRecordFailure(ctx, apply.Database, apply.Environment) + continue + } + if !recorded { + // A drive tail recorded it between the sweep query and this insert. + h.logger.Debug("merge gate sweep found the merge gate request already recorded", + apply.LogAttrs()...) + continue + } + h.logger.Info("merge gate sweep backfilled a merge gate request the drive tail did not record", + apply.LogAttrs()...) + metrics.RecordMergeGateRecorded(ctx, apply.Database, apply.Environment, metrics.MergeGateSourceSweep) + } +} + +// terminateStuckMergeGateRequests terminalizes requests wedged in +// processing past the attempt cap with an expired lease, so a poison request +// cannot be reclaimed forever. Each terminated request means sibling PR +// stored checks for its target may remain stale until their PRs re-plan. +func (h *Handler) terminateStuckMergeGateRequests(ctx context.Context) { + terminated, err := h.mergeGateStore().TerminateStuckProcessing(ctx, "merge gate request exceeded its attempt cap with an expired lease") + if err != nil { + h.logger.Error("merge gate stuck-processing sweep failed; wedged requests stay unterminalized until the next pass", "error", err) + return + } + if terminated == 0 { + return + } + h.logger.Error("merge gate requests terminated after exceeding their attempt cap; sibling PR stored checks for their targets may remain stale until those PRs re-plan", + "terminated", terminated) + metrics.RecordMergeGateTerminatedStuck(ctx, terminated) +} + +// drainMergeGateRequests claims and drives requests until none remain +// claimable, so a backlog is worked down within a single tick. It stops on the +// first empty claim or claim error — a storage error must not spin a tight +// loop — and on context cancellation. +func (h *Handler) drainMergeGateRequests(ctx context.Context, owner string) { + for { + if ctx.Err() != nil { + return + } + if !h.driveNextMergeGate(ctx, owner) { + return + } + } +} + +// driveNextMergeGate claims and drives at most one request. It reports +// whether a request was claimed, so the drain loop knows whether to continue. +func (h *Handler) driveNextMergeGate(ctx context.Context, owner string) (claimed bool) { + store := h.mergeGateStore() + req, err := store.ClaimNext(ctx, owner, h.mergeGateLeaseDuration) + if err != nil { + h.logger.Error("merge gate driver failed to claim a request", "lease_owner", owner, "error", err) + return false + } + if req == nil { + h.logger.Debug("merge gate driver found no request to claim") + return false + } + + h.logger.Info("merge gate driver claimed a request", + "lease_owner", owner, + "apply_id", req.ApplyIdentifier, + "environment", req.Environment, + "database_type", req.DatabaseType, + "database", req.DatabaseName, + "origin_repo", req.Repository, + "origin_change", req.ChangeKey, + "requested_by", req.RequestedBy, + "attempts", req.Attempts) + + h.driveClaimedMergeGate(ctx, store, req) + return true +} + +// driveClaimedMergeGate runs the fan-out → heartbeat → finish lifecycle for +// a freshly claimed request, coalescing pending sibling requests for the same +// target once the fan-out succeeds. +func (h *Handler) driveClaimedMergeGate(ctx context.Context, store storage.MergeGateRequestStore, req *storage.MergeGateRequest) { + // Capture the pending siblings before the fan-out starts: the fan-out + // re-plans against the live schema, so it covers every schema change + // recorded before it began. A request recorded mid-fan-out is not covered + // and stays pending for the next drain. + siblings, err := store.PendingForTarget(ctx, req.Environment, req.DatabaseType, req.DatabaseName, req.ID) + if err != nil { + // Coalescing is an optimization: without the sibling list each pending + // request runs its own fan-out, which re-plans the same PRs again — + // wasteful but safe. + h.logger.Warn("merge gate driver could not list pending sibling requests; siblings will fan out on their own", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, "error", err) + siblings = nil + } + + runCtx, cancelRun := context.WithCancel(ctx) + stopHeartbeat := h.startMergeGateHeartbeat(runCtx, req, cancelRun) + fanErr := h.safeFanOutMergeGate(runCtx, req) + heartbeatErr := stopHeartbeat() + cancelRun() + + finishCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() + + if fanErr != nil { + retryAfter := (*time.Time)(nil) + if req.Attempts < maxMergeGateAttempts { + due := time.Now().Add(mergeGateRetryDelay) + retryAfter = &due + } else { + h.logger.Error("merge gate request exhausted its retry budget and is now terminal; sibling PR stored checks for the target may remain stale until those PRs re-plan", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, + "attempts", req.Attempts, "error", fanErr) + } + if err := store.MarkFailed(finishCtx, req.ID, req.LeaseToken, fanErr.Error(), retryAfter); err != nil { + if errors.Is(err, storage.ErrMergeGateLeaseLost) || errors.Is(err, storage.ErrMergeGateNotFound) { + h.logger.Warn("merge gate driver lost the request lease before recording failure; another driver owns the request", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName) + return + } + h.logger.Error("merge gate driver failed to record the fan-out failure", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, "error", err) + return + } + outcome := "failed_terminal" + if retryAfter != nil { + outcome = "failed_retrying" + } + metrics.RecordMergeGateEventOutcome(finishCtx, req.DatabaseName, req.Environment, outcome) + h.logger.Warn("merge gate driver recorded a fan-out failure", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, + "retry", retryAfter != nil, "error", fanErr) + return + } + if heartbeatErr != nil { + // The fan-out reported success but the lease heartbeat failed, so + // ownership is uncertain. Do not mark it completed — leave the row + // processing so lease expiry hands it to another driver. Re-planning + // the same PRs again is safe. + h.logger.Warn("merge gate driver skipped completion because the request lease heartbeat failed; leaving the request for reclaim", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, "error", heartbeatErr) + metrics.RecordMergeGateEventOutcome(finishCtx, req.DatabaseName, req.Environment, "lease_lost") + return + } + if err := store.MarkCompleted(finishCtx, req.ID, req.LeaseToken); err != nil { + if errors.Is(err, storage.ErrMergeGateLeaseLost) || errors.Is(err, storage.ErrMergeGateNotFound) { + h.logger.Warn("merge gate driver lost the request lease before recording completion; another driver owns the request", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName) + return + } + h.logger.Error("merge gate driver failed to mark the request completed", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, "error", err) + return + } + metrics.RecordMergeGateEventOutcome(finishCtx, req.DatabaseName, req.Environment, "completed") + h.logger.Info("merge gate driver completed the request", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName) + + for _, sibling := range siblings { + coalesced, err := store.CompletePendingCoalesced(finishCtx, sibling.ID) + if err != nil { + // Each sibling completes independently; a failed coalesce leaves + // the sibling pending, so its own fan-out (a redundant but safe + // re-plan) finishes it. + h.logger.Warn("merge gate driver failed to coalesce a covered sibling request; it will run its own fan-out", + "apply_id", req.ApplyIdentifier, "sibling_apply_id", sibling.ApplyIdentifier, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "error", err) + continue + } + if !coalesced { + h.logger.Debug("merge gate sibling request no longer pending; its own lifecycle finishes it", + "apply_id", req.ApplyIdentifier, "sibling_apply_id", sibling.ApplyIdentifier, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName) + continue + } + h.logger.Info("merge gate driver coalesced a sibling request covered by this fan-out", + "apply_id", req.ApplyIdentifier, "sibling_apply_id", sibling.ApplyIdentifier, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName) + } +} + +// safeFanOutMergeGate runs the fan-out with panic recovery: the row stays +// processing until its lease expires and is then claimable again, so a driver +// panic would otherwise crash-loop every replica on the same poison request. A +// recovered panic is a retryable failure, so the attempt cap makes a +// deterministic panic terminal instead. +func (h *Handler) safeFanOutMergeGate(ctx context.Context, req *storage.MergeGateRequest) (err error) { + defer func() { + if r := recover(); r != nil { + h.logger.Error("merge gate driver recovered from panic during fan-out", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, + "panic", fmt.Sprintf("%v", r), "stack", string(debug.Stack())) + err = fmt.Errorf("panic during merge gate fan-out for apply %s: %v", req.ApplyIdentifier, r) + } + }() + return h.fanOutMergeGate(ctx, req) +} + +// fanOutMergeGate re-plans every sibling PR whose stored check state +// targets the request's (environment, database type, database). A returned +// error means at least one PR was neither refreshed nor safely failed closed, +// so the request must be retried; re-planning already-refreshed PRs on that +// retry is safe. +func (h *Handler) fanOutMergeGate(ctx context.Context, req *storage.MergeGateRequest) error { + // Aggregate rows never match: their database type and name are the + // aggregate sentinel, not a real target. + checks, err := h.service.Storage().Checks().GetByTarget(ctx, req.Environment, req.DatabaseType, req.DatabaseName) + if err != nil { + return fmt.Errorf("list stored check state for target %s/%s in %s: %w", + req.DatabaseType, req.DatabaseName, req.Environment, err) + } + + var targets []*storage.Check + for _, check := range checks { + if isOriginatingChange(check, req) { + // The originating PR's own apply lifecycle already updated its + // stored check state; re-planning it here would be redundant. + h.logger.Debug("merge gate skipping the originating PR", + "apply_id", req.ApplyIdentifier, "repo", check.Repository, "pr", check.PullRequest, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName) + continue + } + targets = append(targets, check) + } + if len(targets) == 0 { + h.logger.Info("merge gate found no sibling PR check state for the target", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName) + return nil + } + + h.logger.Info("merge gate fanning out to sibling PRs whose plans predate the schema change", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, + "requested_by", req.RequestedBy, "sibling_prs", len(targets)) + + sem := make(chan struct{}, mergeGatePRConcurrency) + var wg sync.WaitGroup + var mu sync.Mutex + var errs []error + for _, check := range targets { + wg.Go(func() { + sem <- struct{}{} + defer func() { <-sem }() + if err := h.refreshPRPlanForTarget(ctx, req, check); err != nil { + mu.Lock() + errs = append(errs, err) + mu.Unlock() + } + }) + } + wg.Wait() + return errors.Join(errs...) +} + +// refreshPRPlanForTarget re-plans one sibling PR's stored check state against +// the target's live schema. It returns nil when the PR was refreshed or safely +// skipped (closed, in-flight-owned, no longer managed, or superseded by a +// racing write), and nil when a re-plan failure was durably failed closed. A +// returned error means the PR was neither refreshed nor failed closed, so the +// request must be retried. +func (h *Handler) refreshPRPlanForTarget(ctx context.Context, req *storage.MergeGateRequest, check *storage.Check) error { + repo, pr := check.Repository, check.PullRequest + + if check.Status == checkStatusInProgress { + // A started apply remains authoritative for its stored check state; a + // re-plan here would fight the in-flight apply's own lifecycle. The + // apply's terminal update (or stale-check reconciliation) refreshes it. + h.logger.Info("merge gate leaving in-flight apply-owned check state untouched", + "apply_id", req.ApplyIdentifier, "repo", repo, "pr", pr, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "check_apply_id", check.ApplyID, + "check_head_sha", check.HeadSHA) + metrics.RecordMergeGatePROutcome(ctx, repo, req.DatabaseName, req.Environment, mergeGateOutcomeSkippedInFlight) + return nil + } + + // Driver work runs outside any HTTP request, so resolve the App + // installation from config the same way repo-level webhook dispatch does. + installationID, err := h.resolveRepoWebhookInstallation(ctx, repo) + if err != nil { + return fmt.Errorf("resolve installation for merge gate of %s#%d (target %s/%s in %s, apply %s): %w", + repo, pr, req.DatabaseType, req.DatabaseName, req.Environment, req.ApplyIdentifier, err) + } + prCtx, cancel, client, err := h.commandBootstrap(repo, installationID) + defer cancel() + if err != nil { + return fmt.Errorf("bootstrap merge gate of %s#%d (target %s/%s in %s, apply %s): %w", + repo, pr, req.DatabaseType, req.DatabaseName, req.Environment, req.ApplyIdentifier, err) + } + + // A GitHub failure here is uncertainty, not staleness: keep the request + // retryable rather than guessing at the PR's state. + prInfo, err := client.FetchPullRequestNoCache(prCtx, repo, pr) + if err != nil { + return fmt.Errorf("verify PR state for merge gate of %s#%d (target %s/%s in %s, apply %s): %w", + repo, pr, req.DatabaseType, req.DatabaseName, req.Environment, req.ApplyIdentifier, err) + } + if prInfo.IsClosed() { + h.logger.Info("merge gate skipping closed PR; its stored checks no longer gate a merge", + "apply_id", req.ApplyIdentifier, "repo", repo, "pr", pr, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "merged", prInfo.Merged) + metrics.RecordMergeGatePROutcome(ctx, repo, req.DatabaseName, req.Environment, mergeGateOutcomeSkippedPRClosed) + return nil + } + + schemaResult, err := h.createManagedSchemaRequestFromPR(prCtx, client, repo, pr, req.Environment, req.DatabaseName, action.Plan) + if err != nil { + if isMergeGateNotManagedError(err) { + // A determinate answer, not uncertainty: the PR's current head no + // longer manages this target here, so there is nothing to re-plan. + h.logger.Info("merge gate skipping PR that no longer manages the target", + "apply_id", req.ApplyIdentifier, "repo", repo, "pr", pr, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "reason", err) + metrics.RecordMergeGatePROutcome(ctx, repo, req.DatabaseName, req.Environment, mergeGateOutcomeSkippedNotManaged) + return nil + } + return h.blockCheckForFailedRefresh(prCtx, client, req, check, + fmt.Errorf("discover schema config: %w", err)) + } + if err := h.attachServerEnvironments(schemaResult, req.Environment); err != nil { + if isMergeGateNotManagedError(err) { + h.logger.Info("merge gate skipping PR whose target environment is no longer configured", + "apply_id", req.ApplyIdentifier, "repo", repo, "pr", pr, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "reason", err) + metrics.RecordMergeGatePROutcome(ctx, repo, req.DatabaseName, req.Environment, mergeGateOutcomeSkippedNotManaged) + return nil + } + return h.blockCheckForFailedRefresh(prCtx, client, req, check, + fmt.Errorf("validate schema environments: %w", err)) + } + + prNumber := int32(pr) + planReq := api.PlanRequest{ + Database: schemaResult.Database, + Environment: req.Environment, + Type: schemaResult.Type, + SchemaFiles: schemaResult.SchemaFiles, + Repository: repo, + PullRequest: &prNumber, + HeadSHA: &schemaResult.HeadSHA, + SchemaPath: schemaResult.SchemaPath, + SourceTrusted: true, + } + planProto, planResp, err := h.executePlanProtoWithTransientRetry(prCtx, planReq, repo, pr) + if err != nil { + return h.blockCheckForFailedRefresh(prCtx, client, req, check, + fmt.Errorf("re-plan against changed schema: %w", err)) + } + + // Roll up every deployment's diff against the refreshed plan so drift on a + // non-primary deployment fails the check closed, exactly as at review time. + drift := h.reviewTimeDrift(prCtx, planReq, planProto, planResp.Deployment, repo, pr) + + sha, _, err := h.upsertPlanCheckRecord(prCtx, client, repo, pr, schemaResult, planResp, req.Environment, drift, mergeGateNote(req)) + if err != nil { + if errors.Is(err, errPlanCheckHeadStale) { + // A racing synchronize replaced the PR head mid-refresh; its own + // auto-plan against the new head is authoritative. + h.logger.Info("merge gate superseded by a newer PR head; the newer head's plan is authoritative", + "apply_id", req.ApplyIdentifier, "repo", repo, "pr", pr, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "planned_head_sha", schemaResult.HeadSHA) + metrics.RecordMergeGatePROutcome(ctx, repo, req.DatabaseName, req.Environment, mergeGateOutcomeSkippedSuperseded) + return nil + } + return h.blockCheckForFailedRefresh(prCtx, client, req, check, + fmt.Errorf("store refreshed plan check record: %w", err)) + } + h.updateAggregateCheck(prCtx, client, repo, pr, sha) + + h.logger.Info("merge gate re-planned sibling PR against the changed schema", + "apply_id", req.ApplyIdentifier, "repo", repo, "pr", pr, "head_sha", sha, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "requested_by", req.RequestedBy, + "has_changes", planResp.HasChanges()) + metrics.RecordMergeGatePROutcome(ctx, repo, req.DatabaseName, req.Environment, mergeGateOutcomeRefreshed) + return nil +} + +// blockCheckForFailedRefresh fails a sibling PR's stored check state closed +// after its refresh re-plan failed: the plan on record was computed against a +// schema that no longer exists, so it must not keep passing. The raw cause is +// logged server-side with full identifiers; the stored check carries only the +// fixed sanitized block message. Returns nil once the block is durable (or a +// racing write superseded it) and an error when the flip itself failed, so the +// request is retried. +func (h *Handler) blockCheckForFailedRefresh(ctx context.Context, client *ghclient.InstallationClient, req *storage.MergeGateRequest, check *storage.Check, cause error) error { + h.logger.Error("merge gate re-plan failed; failing the stored check closed", + "repo", check.Repository, "pr", check.PullRequest, "check_head_sha", check.HeadSHA, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "apply_id", req.ApplyIdentifier, + "requested_by", req.RequestedBy, "error", cause) + + blocked := *check + blocked.Status = checkStatusCompleted + blocked.Conclusion = checkConclusionActionRequired + blocked.HasChanges = true + blocked.BlockingReason = schemaChangedReplanFailedBlock.blockingReason + blocked.ErrorMessage = schemaChangedReplanFailedBlock.message + blocked.ChangeSummary = clampDriftSummary(fmt.Sprintf( + "schema for %s in %s changed (apply %s); re-plan failed — see server logs", + req.DatabaseName, req.Environment, req.ApplyIdentifier)) + flipped, err := h.service.Storage().Checks().MarkBlockedForFailedRefresh(ctx, &blocked) + if err != nil { + return fmt.Errorf("fail stored check closed for %s#%d (target %s/%s in %s, apply %s) after re-plan failure: %w", + check.Repository, check.PullRequest, req.DatabaseType, req.DatabaseName, + req.Environment, req.ApplyIdentifier, err) + } + if !flipped { + // The head-SHA condition (or the in-flight apply-owned guard) refused + // the write: a racing synchronize re-planned a newer head, or an apply + // claimed the row. Either way the newer write is authoritative. + h.logger.Info("merge gate fail-closed flip superseded by a racing write; the newer stored check state is authoritative", + "apply_id", req.ApplyIdentifier, "repo", check.Repository, "pr", check.PullRequest, + "environment", req.Environment, "database_type", req.DatabaseType, + "database", req.DatabaseName, "check_head_sha", check.HeadSHA) + metrics.RecordMergeGatePROutcome(ctx, check.Repository, req.DatabaseName, req.Environment, mergeGateOutcomeSkippedSuperseded) + return nil + } + h.updateAggregateCheck(ctx, client, check.Repository, check.PullRequest, check.HeadSHA) + metrics.RecordMergeGatePROutcome(ctx, check.Repository, req.DatabaseName, req.Environment, mergeGateOutcomeBlockedReplan) + return nil +} + +// startMergeGateHeartbeat extends the request lease on a fixed cadence +// while the fan-out runs, so a fan-out spanning many PRs is not reclaimed +// mid-flight. On lease loss it cancels the run context so in-flight work +// stops. The returned join function stops the heartbeat and reports the +// heartbeat failure (nil when the lease was held for the whole run). +func (h *Handler) startMergeGateHeartbeat(ctx context.Context, req *storage.MergeGateRequest, cancelRun context.CancelFunc) func() error { + hbCtx, stop := context.WithCancel(ctx) + done := make(chan struct{}) + var heartbeatErr error // written once before close(done) + interval := h.mergeGateLeaseDuration / 3 + if interval <= 0 { + interval = 10 * time.Second + } + go func() { + defer close(done) + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-hbCtx.Done(): + return + case <-ticker.C: + if err := h.mergeGateStore().Heartbeat(hbCtx, req.ID, req.LeaseToken, h.mergeGateLeaseDuration); err != nil { + if hbCtx.Err() != nil { + // The heartbeat was stopped intentionally mid-call; the + // interrupted call is not a lease failure. + h.logger.Debug("merge gate heartbeat interrupted by intentional stop; not a lease failure", + "apply_id", req.ApplyIdentifier) + return + } + if errors.Is(err, storage.ErrMergeGateLeaseLost) || errors.Is(err, storage.ErrMergeGateNotFound) { + // Lease loss and a deleted row are both terminal for this + // run: the result has nowhere to land, so stop instead of + // finishing work that cannot be recorded. + h.logger.Warn("merge gate heartbeat lost the request lease; driver will stop", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, "error", err) + heartbeatErr = err + cancelRun() + return + } + // A transient store error is not lease loss: the lease is + // still ours until it expires, so keep working and retry on + // the next tick. + h.logger.Warn("merge gate heartbeat failed; will retry", + "apply_id", req.ApplyIdentifier, "environment", req.Environment, + "database_type", req.DatabaseType, "database", req.DatabaseName, "error", err) + } + } + } + }() + return func() error { + stop() + <-done + return heartbeatErr + } +} + +// isMergeGateNotManagedError reports whether a schema discovery error is a +// determinate "this PR does not manage the target here" answer — safe to skip +// — as opposed to uncertainty (GitHub unavailability, storage failure), which +// must fail closed or retry. +func isMergeGateNotManagedError(err error) bool { + var dbNotFound *ghclient.DatabaseNotFoundError + var outsideAllowedDirs *schemaConfigOutsideAllowedDirsError + var dbNotConfigured *api.DatabaseNotConfiguredError + var envNotConfigured *environmentNotConfiguredError + return errors.Is(err, ghclient.ErrNoConfig) || + errors.As(err, &dbNotFound) || + errors.As(err, &outsideAllowedDirs) || + errors.As(err, &dbNotConfigured) || + errors.As(err, &envNotConfigured) +} + +// isOriginatingChange reports whether a stored check belongs to the change +// that originated the merge gate request. CLI/gRPC applies carry no +// originating change (empty ChangeKey) and therefore match nothing. +func isOriginatingChange(check *storage.Check, req *storage.MergeGateRequest) bool { + if req.ChangeKey == "" { + return false + } + return check.Repository == req.Repository && + storage.ChangeKeyForPullRequest(check.PullRequest) == req.ChangeKey +} + +// mergeGateNote renders the attribution line appended to a refreshed +// check's stored change summary, so the aggregate's Change column says why an +// unchanged PR was re-planned. RequestedBy is caller-influenced text, so the +// note is sanitized for markdown-table rendering and clamped to the column +// width. +func mergeGateNote(req *storage.MergeGateRequest) string { + return clampDriftSummary(fmt.Sprintf("re-planned: schema for %s in %s changed (apply %s by %s)", + req.DatabaseName, req.Environment, req.ApplyIdentifier, req.RequestedBy)) +} + +// appendRefreshNote joins a plan's own change summary with the refresh +// attribution note, keeping the combined value within the stored column width. +func appendRefreshNote(changeSummary, note string) string { + if changeSummary == "" { + return note + } + return clampDriftSummary(changeSummary + " · " + note) +} + +func (h *Handler) mergeGateStore() storage.MergeGateRequestStore { + if h.service == nil || h.service.Storage() == nil { + return nil + } + return h.service.Storage().MergeGateRequests() +} + +func mergeGateLeaseOwner() string { + hostname, err := os.Hostname() + if err != nil || hostname == "" { + hostname = "unknown-host" + } + return fmt.Sprintf("%s/%d/check-refresh", hostname, os.Getpid()) +} diff --git a/pkg/webhook/merge_gate_integration_test.go b/pkg/webhook/merge_gate_integration_test.go index facfd2e07..37ed24a23 100644 --- a/pkg/webhook/merge_gate_integration_test.go +++ b/pkg/webhook/merge_gate_integration_test.go @@ -4,8 +4,11 @@ // success on a (environment, database type, database) target, every other open // PR with stored check state against that target planned against a schema that // no longer exists. These tests exercise the durable merge gate request lifecycle -// against the real webhook harness, starting with recording at the operator -// drive tail. +// end to end against the real webhook harness: recording at the operator drive +// tail, the backstop sweep, the sibling PR fan-out with attribution, the +// fail-closed flip when a re-plan fails, the in-flight apply guard, +// same-target request coalescing, and the recorded-request kick that drains +// without waiting for a poll tick. package webhook @@ -23,10 +26,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/block/schemabot/pkg/api" "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/storage" ) +const mergeGateTestLeaseOwner = "check-refresh-test-driver" + // clearMergeGateRequests empties the shared merge_gate_requests table. // The table is cross-test shared state: apply drive tails in earlier tests // record requests the processor never drains there, and this test's drain @@ -40,6 +46,37 @@ func clearMergeGateRequests(t *testing.T) { require.NoError(t, err) } +// recordRefreshRequest records a pending refresh request directly, standing in +// for the operator drive tail so the processor side can be exercised in +// isolation. +func recordRefreshRequest(t *testing.T, svc *api.Service, req *storage.MergeGateRequest) *storage.MergeGateRequest { + t.Helper() + recorded, err := svc.Storage().MergeGateRequests().Record(t.Context(), req) + require.NoError(t, err) + require.True(t, recorded) + return req +} + +// seedRefreshTargetCheck stores per-database plan check state for a PR against +// the given target, as an earlier plan would have recorded it. +func seedRefreshTargetCheck(t *testing.T, svc *api.Service, pr int, env, dbName, status, conclusion, changeSummary string) *storage.Check { + t.Helper() + check := &storage.Check{ + Repository: "octocat/hello-world", + PullRequest: pr, + HeadSHA: "abc123", + Environment: env, + DatabaseType: "mysql", + DatabaseName: dbName, + HasChanges: true, + Status: status, + Conclusion: conclusion, + ChangeSummary: changeSummary, + } + require.NoError(t, svc.Storage().Checks().Upsert(t.Context(), check)) + return check +} + // TestE2EMergeGateRecordedOnApplyTerminalSuccess drives a real apply // through the webhook command path to terminal success and verifies the // operator drive tail durably records a merge gate request for the apply's @@ -140,3 +177,377 @@ func TestE2EMergeGateRecordedOnApplyTerminalSuccess(t *testing.T) { t.Fatal("timed out waiting for the drive tail to invoke the merge gate recorded-notifier") } } + +// TestE2EMergeGateSweepBackfillsMissedApply verifies the outbox backstop: a +// completed apply with no refresh request (a pod crash between the terminal +// write and the drive-tail recording) is found by the processor's sweep and +// its request backfilled with full attribution, so the fan-out is never lost. +func TestE2EMergeGateSweepBackfillsMissedApply(t *testing.T) { + clearMergeGateRequests(t) + dbName := "webhook_mergegate_sweep" + // The sweep reads and writes storage only, so the lighter storage-backed + // service is enough — no target database, tern client, or operator. + svc := setupE2EServiceWithConfig(t, &api.ServerConfig{}) + ctx := t.Context() + + lock := &storage.Lock{ + DatabaseName: dbName, + DatabaseType: "mysql", + Repository: "octocat/hello-world", + PullRequest: 1, + Owner: "octocat/hello-world#1", + } + require.NoError(t, svc.Storage().Locks().Acquire(ctx, lock)) + lock, err := svc.Storage().Locks().Get(ctx, dbName, "mysql") + require.NoError(t, err) + t.Cleanup(func() { + _ = svc.Storage().Locks().ForceRelease(context.WithoutCancel(t.Context()), dbName, "mysql") + }) + + apply := &storage.Apply{ + ApplyIdentifier: fmt.Sprintf("apply_mergegate_sweep_%d", time.Now().UnixNano()), + LockID: lock.ID, + PlanID: 1, + Database: dbName, + DatabaseType: "mysql", + Repository: "octocat/hello-world", + PullRequest: 1, + Environment: "staging", + Caller: "cli:sweeper@host", + InstallationID: 12345, + Engine: "spirit", + State: state.Apply.Completed, + } + applyID, err := svc.Storage().Applies().Create(ctx, apply) + require.NoError(t, err) + apply.ID = applyID + completedAt := time.Now() + apply.CompletedAt = &completedAt + require.NoError(t, svc.Storage().Applies().Update(ctx, apply)) + + h := newE2EHandler(t, svc, gh.NewClient(nil)) + h.sweepMergeGateRequests(ctx) + + gateReq, err := svc.Storage().MergeGateRequests().GetByApplyID(ctx, applyID) + require.NoError(t, err) + require.NotNil(t, gateReq, "the sweep must backfill a refresh request for a completed apply that has none") + assert.Equal(t, apply.ApplyIdentifier, gateReq.ApplyIdentifier) + assert.Equal(t, "staging", gateReq.Environment) + assert.Equal(t, dbName, gateReq.DatabaseName) + assert.Equal(t, "cli:sweeper@host", gateReq.RequestedBy) + assert.Equal(t, storage.MergeGatePending, gateReq.State) + + // Recording is idempotent per apply: a second sweep pass over the same + // window must not duplicate or reset the request. + h.sweepMergeGateRequests(ctx) + again, err := svc.Storage().MergeGateRequests().GetByApplyID(ctx, applyID) + require.NoError(t, err) + require.NotNil(t, again) + assert.Equal(t, gateReq.ID, again.ID) +} + +// TestE2EMergeGateReplansSiblingPRAndSkipsOriginator verifies the fan-out: +// after an apply on another PR changes a target's live schema, a sibling PR +// with stored plan check state on that target is re-planned against the live +// schema at its current head, and the refreshed check's change summary carries +// the attribution (which apply, by whom) so a reviewer knows why an unchanged +// PR was re-planned. The originating PR's own check state is left to its apply +// lifecycle and never re-planned. +func TestE2EMergeGateReplansSiblingPRAndSkipsOriginator(t *testing.T) { + clearMergeGateRequests(t) + dbName := "webhook_mergegate_fanout" + svc := setupE2EService(t, dbName) + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\n", dbName) + schemaFiles := map[string]string{ + "users.sql": "CREATE TABLE `users` (\n `id` bigint unsigned NOT NULL AUTO_INCREMENT,\n `name` varchar(255) NOT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;", + } + setupFakeGitHubForPlan(t, mux, schemaFiles, schemabotConfig, dbName) + + // The sibling PR (#1) holds plan check state from before the schema + // change. The originating PR (#2) has check state on the same target; the + // fake GitHub serves no fixtures for it, so any attempt to re-plan it + // would fail the drain loudly instead of passing silently. + seedRefreshTargetCheck(t, svc, 1, "staging", dbName, + checkStatusCompleted, checkConclusionActionRequired, "1 table created") + originator := seedRefreshTargetCheck(t, svc, 2, "staging", dbName, + checkStatusCompleted, checkConclusionActionRequired, "originator summary") + + h := newE2EHandler(t, svc, client) + + applyIdentifier := fmt.Sprintf("apply_mergegate_fanout_%d", time.Now().UnixNano()) + gateReq := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000001, + ApplyIdentifier: applyIdentifier, + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + Repository: "octocat/hello-world", + ChangeKey: "2", + RequestedBy: "cli:tester@host", + }) + + h.drainMergeGateRequests(t.Context(), mergeGateTestLeaseOwner) + + // The sibling PR's stored check state was re-planned against the live + // schema at its current head, with the attribution note appended. + refreshed, err := svc.Storage().Checks().Get(t.Context(), "octocat/hello-world", 1, "staging", "mysql", dbName) + require.NoError(t, err) + require.NotNil(t, refreshed) + assert.Equal(t, "abc123", refreshed.HeadSHA) + assert.True(t, refreshed.HasChanges) + assert.Contains(t, refreshed.ChangeSummary, "re-planned: schema for "+dbName+" in staging changed") + assert.Contains(t, refreshed.ChangeSummary, applyIdentifier) + assert.Contains(t, refreshed.ChangeSummary, "cli:tester@host") + assert.Empty(t, refreshed.BlockingReason) + + // The originating PR's stored check state is untouched. + originatorAfter, err := svc.Storage().Checks().Get(t.Context(), "octocat/hello-world", 2, "staging", "mysql", dbName) + require.NoError(t, err) + require.NotNil(t, originatorAfter) + assert.Equal(t, originator.ChangeSummary, originatorAfter.ChangeSummary) + assert.Equal(t, originator.Status, originatorAfter.Status) + assert.Equal(t, originator.Conclusion, originatorAfter.Conclusion) + assert.Empty(t, originatorAfter.BlockingReason) + + // The request itself is terminal-successful. + finished, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), gateReq.ApplyID) + require.NoError(t, err) + require.NotNil(t, finished) + assert.Equal(t, storage.MergeGateCompleted, finished.State) +} + +// TestE2EMergeGateReplanFailureFailsCheckClosed verifies the fail-closed +// guarantee: when a sibling PR's re-plan fails (here: its schema files no +// longer parse), its stored check state is durably flipped to a blocking +// conclusion with a fixed sanitized message — a plan computed against a schema +// that no longer exists must not keep passing — and the request still +// completes because the block is a durable outcome, not a retry. +func TestE2EMergeGateReplanFailureFailsCheckClosed(t *testing.T) { + clearMergeGateRequests(t) + dbName := "webhook_mergegate_failclosed" + svc := setupE2EService(t, dbName) + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\n", dbName) + schemaFiles := map[string]string{ + "users.sql": "CREATE TABLE `users` (broken", + } + setupFakeGitHubForPlan(t, mux, schemaFiles, schemabotConfig, dbName) + + seedRefreshTargetCheck(t, svc, 1, "staging", dbName, + checkStatusCompleted, checkConclusionSuccess, "no changes") + + h := newE2EHandler(t, svc, client) + + applyIdentifier := fmt.Sprintf("apply_mergegate_failclosed_%d", time.Now().UnixNano()) + gateReq := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000002, + ApplyIdentifier: applyIdentifier, + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + RequestedBy: "cli:tester@host", + }) + + h.drainMergeGateRequests(t.Context(), mergeGateTestLeaseOwner) + + blocked, err := svc.Storage().Checks().Get(t.Context(), "octocat/hello-world", 1, "staging", "mysql", dbName) + require.NoError(t, err) + require.NotNil(t, blocked) + assert.Equal(t, checkStatusCompleted, blocked.Status) + assert.Equal(t, checkConclusionActionRequired, blocked.Conclusion) + assert.True(t, blocked.HasChanges) + assert.Equal(t, schemaChangedReplanFailedBlock.blockingReason, blocked.BlockingReason) + assert.Equal(t, schemaChangedReplanFailedBlock.message, blocked.ErrorMessage) + assert.Contains(t, blocked.ChangeSummary, "re-plan failed — see server logs") + assert.Contains(t, blocked.ChangeSummary, applyIdentifier) + + finished, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), gateReq.ApplyID) + require.NoError(t, err) + require.NotNil(t, finished) + assert.Equal(t, storage.MergeGateCompleted, finished.State) +} + +// TestE2EMergeGateLeavesInFlightApplyCheckUntouched verifies that a started +// apply remains authoritative: a sibling PR whose stored check state is owned +// by an in-flight apply (status in_progress with an apply id) is never +// re-planned or flipped — the apply's own terminal update refreshes it — and +// the request completes without touching GitHub for that PR. +func TestE2EMergeGateLeavesInFlightApplyCheckUntouched(t *testing.T) { + clearMergeGateRequests(t) + dbName := "webhook_mergegate_inflight" + // The in-flight guard fires on stored check state alone, so the lighter + // storage-backed service is enough — no target database or operator. + svc := setupE2EServiceWithConfig(t, &api.ServerConfig{}) + + // No GitHub fixtures at all: the in-flight guard fires before any GitHub + // call, so a fetch attempt for this PR fails the drain loudly. + client := gh.NewClient(nil) + server := httptest.NewServer(http.NewServeMux()) + t.Cleanup(server.Close) + client.BaseURL, _ = url.Parse(server.URL + "/") + + inFlight := seedRefreshTargetCheck(t, svc, 1, "staging", dbName, + checkStatusInProgress, "", "apply in flight") + inFlight.ApplyID = 424242 + require.NoError(t, svc.Storage().Checks().Upsert(t.Context(), inFlight)) + + h := newE2EHandler(t, svc, client) + + gateReq := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000003, + ApplyIdentifier: fmt.Sprintf("apply_mergegate_inflight_%d", time.Now().UnixNano()), + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + RequestedBy: "cli:tester@host", + }) + + h.drainMergeGateRequests(t.Context(), mergeGateTestLeaseOwner) + + untouched, err := svc.Storage().Checks().Get(t.Context(), "octocat/hello-world", 1, "staging", "mysql", dbName) + require.NoError(t, err) + require.NotNil(t, untouched) + assert.Equal(t, checkStatusInProgress, untouched.Status) + assert.Equal(t, int64(424242), untouched.ApplyID) + assert.Equal(t, "apply in flight", untouched.ChangeSummary) + assert.Empty(t, untouched.BlockingReason) + + finished, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), gateReq.ApplyID) + require.NoError(t, err) + require.NotNil(t, finished) + assert.Equal(t, storage.MergeGateCompleted, finished.State) +} + +// TestE2EMergeGateCoalescesPendingSiblingRequests verifies coalescing: two +// applies completing on the same target need only one fan-out, because a +// re-plan against the live schema covers every schema change recorded before +// it started. The drain drives the older request and completes the younger one +// without ever claiming it. +func TestE2EMergeGateCoalescesPendingSiblingRequests(t *testing.T) { + clearMergeGateRequests(t) + dbName := "webhook_mergegate_coalesce" + // Coalescing is pure request-lifecycle behavior in storage, so the lighter + // storage-backed service is enough — no target database or operator. + svc := setupE2EServiceWithConfig(t, &api.ServerConfig{}) + + client := gh.NewClient(nil) + server := httptest.NewServer(http.NewServeMux()) + t.Cleanup(server.Close) + client.BaseURL, _ = url.Parse(server.URL + "/") + + h := newE2EHandler(t, svc, client) + + first := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000004, + ApplyIdentifier: fmt.Sprintf("apply_mergegate_coalesce_a_%d", time.Now().UnixNano()), + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + RequestedBy: "cli:tester@host", + }) + second := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000005, + ApplyIdentifier: fmt.Sprintf("apply_mergegate_coalesce_b_%d", time.Now().UnixNano()), + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + RequestedBy: "cli:tester@host", + }) + + h.drainMergeGateRequests(t.Context(), mergeGateTestLeaseOwner) + + driven, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), first.ApplyID) + require.NoError(t, err) + require.NotNil(t, driven) + assert.Equal(t, storage.MergeGateCompleted, driven.State) + assert.Equal(t, 1, driven.Attempts, "the older request runs the fan-out") + + coalesced, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), second.ApplyID) + require.NoError(t, err) + require.NotNil(t, coalesced) + assert.Equal(t, storage.MergeGateCompleted, coalesced.State) + assert.Equal(t, 0, coalesced.Attempts, "the younger request is coalesced, never claimed") +} + +// TestE2EMergeGateKickDrainsWithoutTick verifies the recorded-request kick: +// a request recorded while the processor sleeps between polls is drained as +// soon as the drive tail's notifier fires, so sibling PR checks re-plan +// without waiting out the poll interval. The poll interval is set far beyond +// the test deadline, so only the kick can explain the drain. +func TestE2EMergeGateKickDrainsWithoutTick(t *testing.T) { + clearMergeGateRequests(t) + dbName := "webhook_mergegate_kick" + // The request lifecycle is storage-only when no sibling checks exist, so + // the lighter storage-backed service is enough — no target database or + // operator. + svc := setupE2EServiceWithConfig(t, &api.ServerConfig{}) + + client := gh.NewClient(nil) + server := httptest.NewServer(http.NewServeMux()) + t.Cleanup(server.Close) + client.BaseURL, _ = url.Parse(server.URL + "/") + + h := newE2EHandler(t, svc, client) + require.NotNil(t, svc.OnMergeGateRecorded, + "the handler registers the drive-tail kick on the service at construction") + + // A sentinel recorded before start is drained by the driver's startup + // pass; its completion means the driver is parked on the (hour-long) + // ticker, so nothing but a kick can drain the next request. + sentinel := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000006, + ApplyIdentifier: fmt.Sprintf("apply_mergegate_kick_sentinel_%d", time.Now().UnixNano()), + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + RequestedBy: "cli:tester@host", + }) + + h.mergeGatePollInterval = time.Hour + h.StartMergeGateProcessor(t.Context()) + t.Cleanup(h.StopMergeGateProcessor) + + require.EventuallyWithT(t, func(collect *assert.CollectT) { + got, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), sentinel.ApplyID) + if !assert.NoError(collect, err) || !assert.NotNil(collect, got) { + return + } + assert.Equal(collect, storage.MergeGateCompleted, got.State) + }, webhookIntegrationPollDeadline, 100*time.Millisecond, + "the startup pass drains the sentinel request") + + kicked := recordRefreshRequest(t, svc, &storage.MergeGateRequest{ + ApplyID: 91000007, + ApplyIdentifier: fmt.Sprintf("apply_mergegate_kick_%d", time.Now().UnixNano()), + Environment: "staging", + DatabaseType: "mysql", + DatabaseName: dbName, + RequestedBy: "cli:tester@host", + }) + // Wake the driver through the same registration the drive tail uses. + svc.OnMergeGateRecorded() + + require.EventuallyWithT(t, func(collect *assert.CollectT) { + got, err := svc.Storage().MergeGateRequests().GetByApplyID(t.Context(), kicked.ApplyID) + if !assert.NoError(collect, err) || !assert.NotNil(collect, got) { + return + } + assert.Equal(collect, storage.MergeGateCompleted, got.State) + }, webhookIntegrationPollDeadline, 100*time.Millisecond, + "the kick drains the request without a poll tick") +} diff --git a/pkg/webhook/vschema_only_check_integration_test.go b/pkg/webhook/vschema_only_check_integration_test.go index 28a447d7a..329aa30c4 100644 --- a/pkg/webhook/vschema_only_check_integration_test.go +++ b/pkg/webhook/vschema_only_check_integration_test.go @@ -94,7 +94,7 @@ func TestUpsertPlanCheckRecord_VSchemaOnlyPlanRequiresApply(t *testing.T) { }}, } - gotSHA, check, err := h.upsertPlanCheckRecord(ctx, installClient, repo, pr, schema, planResp, env, reviewDriftOutcome{}) + gotSHA, check, err := h.upsertPlanCheckRecord(ctx, installClient, repo, pr, schema, planResp, env, reviewDriftOutcome{}, "") require.NoError(t, err) assert.Equal(t, headSHA, gotSHA) require.NotNil(t, check) @@ -114,7 +114,7 @@ func TestUpsertPlanCheckRecord_VSchemaOnlyPlanRequiresApply(t *testing.T) { Changes: []*apitypes.SchemaChangeResponse{{Namespace: "boardgames_sharded"}}, } - _, check, err := h.upsertPlanCheckRecord(ctx, installClient, repo, pr, schema, planResp, env, reviewDriftOutcome{}) + _, check, err := h.upsertPlanCheckRecord(ctx, installClient, repo, pr, schema, planResp, env, reviewDriftOutcome{}, "") require.NoError(t, err) require.NotNil(t, check) assert.False(t, check.HasChanges)