feat(github): store born-held checks while a preflighted apply changes the target

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: aparajon

pkg/metrics/metrics.go

@@ -1619,6 +1619,20 @@ func RecordCheckPreflightGateOutcome(ctx context.Context, database, environment,
 	)
 }
 
+// RecordMergeGatePlanTimeHold counts plan-time check writes stored born held:
+// the plan's verdict would have passed, but a preflighted apply is changing
+// the target, so storing a passing check would reopen the merge gate
+// mid-apply. The apply's settle fan-out re-plans held checks when it settles.
+// A sustained rate with no matching settle re-plans means holds are piling up
+// on the target — check the merge gate processor's logs.
+func RecordMergeGatePlanTimeHold(ctx context.Context, database, environment string) {
+	addCounter(ctx, "schemabot.merge_gate.plan_time_holds_total",
+		"Total plan-time check writes stored held because a preflighted apply was in flight on the target", "{hold}",
+		attribute.String("database", database),
+		EnvironmentAttribute(environment),
+	)
+}
+
 // 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

pkg/webhook/check_records.go

@@ -209,6 +209,45 @@ func (h *Handler) upsertPlanCheckRecord(ctx context.Context, client *ghclient.In
 		blockingReason = reviewTimeDeploymentDriftBlock.blockingReason
 	}
 
+	// A preflighted apply changing this target right now means the schema this
+	// plan was computed against is mid-change, so a passing verdict cannot
+	// prove the schema the merge would land on. Storing it would reopen the
+	// merge gate the preflight fan-out closed: the check is born held instead,
+	// and the apply's settle fan-out re-plans it against the settled schema,
+	// releasing the hold. Verdicts that already block (changes, plan errors,
+	// drift) keep their more specific reason. A storage failure fails closed
+	// by failing the write — never by assuming the target is quiet.
+	errorMessage := ""
+	if conclusion == checkConclusionSuccess {
+		activeHold, err := h.service.Storage().MergeGateRequests().HasActivePreflightedApplyOnTarget(ctx, environment, schema.Type, schema.Database)
+		if err != nil {
+			metrics.RecordStatusCheckOperation(ctx, metrics.StatusCheckOperation{
+				Operation:    "plan_check_recorded",
+				Repository:   repo,
+				Database:     schema.Database,
+				DatabaseType: schema.Type,
+				Environment:  environment,
+				Status:       "error",
+			})
+			return headSHA, nil, fmt.Errorf("check for an active preflighted apply before storing plan check state repo %s pr %d environment %s database_type %s database %s: %w",
+				repo, pr, environment, schema.Type, schema.Database, err)
+		}
+		if activeHold {
+			conclusion = checkConclusionActionRequired
+			blockingReason = applyInFlightBlock.blockingReason
+			errorMessage = applyInFlightBlock.message
+			changeSummary = clampDriftSummary(fmt.Sprintf("held: an apply in flight is changing %s in %s", schema.Database, environment))
+			h.logger.Info("plan check born held: an active preflighted apply is changing the target; the apply's settle fan-out will re-plan this check when it settles",
+				"repo", repo,
+				"pr", pr,
+				"head_sha", headSHA,
+				"environment", environment,
+				"database_type", schema.Type,
+				"database", schema.Database)
+			metrics.RecordMergeGatePlanTimeHold(ctx, schema.Database, environment)
+		}
+	}
+
 	check := &storage.Check{
 		Repository:     repo,
 		PullRequest:    pr,
@@ -220,6 +259,7 @@ func (h *Handler) upsertPlanCheckRecord(ctx context.Context, client *ghclient.In
 		Status:         checkStatusCompleted,
 		Conclusion:     conclusion,
 		BlockingReason: blockingReason,
+		ErrorMessage:   errorMessage,
 		ChangeSummary:  changeSummary,
 	}
 	if err := h.service.Storage().Checks().UpsertPlanResult(ctx, check, drift.planDriftState()); err != nil {

pkg/webhook/merge_gate_integration_test.go

@@ -22,9 +22,12 @@ import (
 	"database/sql"
 	"encoding/json"
 	"fmt"
+	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -34,6 +37,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/block/schemabot/pkg/api"
+	ghclient "github.com/block/schemabot/pkg/github"
 	"github.com/block/schemabot/pkg/state"
 	"github.com/block/schemabot/pkg/storage"
 )
@@ -884,3 +888,89 @@ func TestE2ECheckSettleDefersToActivePreflightedApply(t *testing.T) {
 	assert.Equal(t, applyInFlightBlock.blockingReason, stillHeld.BlockingReason,
 		"the active apply's holds must survive an earlier apply's settle")
 }
+
+// TestE2ECheckPlanBornHeldDuringActivePreflightedApply verifies plan-time
+// merge gate awareness: while a preflighted apply is changing a target, a
+// sibling change that plans against that target must not mint a fresh
+// passing check — the preflight fan-out already held the checks that existed
+// when the apply started, and a new plan (a pushed commit or a manual plan
+// command) would otherwise sidestep those holds. A plan whose verdict would
+// pass is stored born held with the apply-in-flight blocking reason instead,
+// and the apply's settle fan-out re-plans it like any other held check. The
+// plan comment still posts normally: the hold changes the stored verdict,
+// not the plan UX.
+func TestE2ECheckPlanBornHeldDuringActivePreflightedApply(t *testing.T) {
+	clearMergeGateRequests(t)
+	dbName := "webhook_mergegate_bornheld"
+	svc := setupE2EService(t, dbName)
+
+	// The target already matches the PR schema, so the plan finds no changes
+	// and its verdict would pass.
+	ctx := t.Context()
+	appDSN := strings.Replace(e2eTargetDSN, "/target_test", "/"+dbName, 1) + "&multiStatements=true"
+	db, err := sql.Open("mysql", appDSN)
+	require.NoError(t, err)
+	_, err = db.ExecContext(ctx, "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")
+	require.NoError(t, err)
+	_ = db.Close()
+
+	// Another change's apply is mid-flight on the target with a recorded
+	// preflight, meaning its holds are (or are about to be) in force.
+	apply := seedApplyWithLock(t, svc, dbName, state.Apply.Running, 2)
+	recordRefreshRequest(t, svc, &storage.MergeGateRequest{
+		ApplyID:         apply.ID,
+		Kind:            storage.MergeGateKindPreflight,
+		ApplyIdentifier: apply.ApplyIdentifier,
+		Environment:     "staging",
+		DatabaseType:    "mysql",
+		DatabaseName:    dbName,
+		Repository:      "octocat/hello-world",
+		ChangeKey:       "2",
+		RequestedBy:     apply.Caller,
+	})
+
+	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;",
+	}
+	result := setupFakeGitHubForPlan(t, mux, schemaFiles, schemabotConfig, dbName)
+
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelError}))
+	installClient := ghclient.NewInstallationClient(client, logger)
+	factory := &fakeClientFactory{client: installClient}
+	h := NewHandler(svc, factory, nil, logger)
+
+	req := buildWebhookRequest(t, webhookPayloadOpts{
+		comment: "schemabot plan -e staging",
+		isPR:    true,
+	}, nil)
+
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, req)
+	require.Equal(t, http.StatusOK, rr.Code)
+
+	// The plan comment posts as usual.
+	select {
+	case body := <-result.comments:
+		assert.Contains(t, body, "No schema changes detected")
+	case <-time.After(10 * time.Second):
+		t.Fatal("timed out waiting for plan comment")
+	}
+
+	// The stored check is born held, not passing.
+	check, err := svc.Storage().Checks().Get(ctx, "octocat/hello-world", 1, "staging", "mysql", dbName)
+	require.NoError(t, err)
+	require.NotNil(t, check, "expected a stored check record for the plan")
+	assert.False(t, check.HasChanges)
+	assert.Equal(t, checkStatusCompleted, check.Status)
+	assert.Equal(t, checkConclusionActionRequired, check.Conclusion)
+	assert.Equal(t, applyInFlightBlock.blockingReason, check.BlockingReason)
+	assert.Contains(t, check.ChangeSummary, "held: an apply in flight is changing "+dbName+" in staging")
+}