fix(serve): stop when told to, at every stage of the lifecycle

[aparajon]

A SchemaBot instance had three ways to ignore being told to stop, and they
stacked: the serve command ran the server on a context of its own, so the
CLI's signal handler cancelled a context nobody was watching; the standalone
path installed its own handler only once the server was already built, leaving
the whole of startup uncovered; and the close path waited on background
goroutines with no bound, so one goroutine that never returned kept the process
alive indefinitely. Because the CLI traps SIGTERM for every command, Go's
default terminate-on-signal was already out of the way — so an instance
signalled mid-bootstrap did not stop, did not log, and was eventually killed.

An instance is now signal-scoped from before it starts building, and every wait
on the way down carries a bound and says what it walked away from.

Establishes AV-11 (an instance that is told to stop, stops). Upholds
AV-3 and AV-9; upholds OW-3, which is what decides the shape of the
driver drain below — an abandoned drive's claim is deliberately left registered,
because releasing it would invite a peer onto a target this instance has not let
go of. Left stale, it is reclaimed on the same window that covers any driver
that disappears.

Startup

An instance is 40 seconds into its storage bootstrap, against a database that
accepts connections and never answers, when the platform sends SIGTERM.

Before                                            After

┌──────────────────────────┐                      ┌──────────────────────────┐
│ SIGTERM at t+40s,        │                      │ SIGTERM at t+40s,        │
│ inside the storage boot  │                      │ inside the storage boot  │
└─────────────┬────────────┘                      └─────────────┬────────────┘
              │ CLI cancels its context                         │ CLI cancels its context
              ▼                                                 ▼
┌──────────────────────────┐                      ┌──────────────────────────┐
│ serve.Run built the      │                      │ serve.Run built the      │
│ server on a context of   │                      │ server on that context,  │
│ its own                  │  ✗ nothing watches   │ watcher installed first  │  ✓ signal honored
└─────────────┬────────────┘                      └─────────────┬────────────┘
              │ boot continues                                  │ boot returns
              ▼                                                 ▼
┌──────────────────────────┐                      ┌──────────────────────────┐
│ boot keeps retrying the  │                      │ boot reports the         │
│ unresponsive database    │                      │ cancellation, not the    │
│ for its 8m budget        │                      │ 5m bootstrap timeout     │
└─────────────┬────────────┘                      └─────────────┬────────────┘
              │ grace period expires                            │ Build fails
              ▼                                                 ▼
┌──────────────────────────┐                      ┌──────────────────────────┐
│ SIGKILL                  │  ✗                   │ exit 1 at t+40s          │  ✓
│ no shutdown logs at all  │                      │ with the cause logged    │
└──────────────────────────┘                      └──────────────────────────┘

Two independent gaps closed here, either of which alone would have been enough:
the command now passes the CLI's signal-scoped context to serve.Run, and
serve.Run installs its own watcher before Build rather than after it. The
embedding seam (Build, RegisterGRPC, Start, Close) still installs no
signal handler at all — a library that traps signals out from under its host is
a worse defect than the one being fixed — so an embedder keeps getting
cancellation as its only stop signal, and that now works.

Shutdown

An operator rolls the deployment while one driver is inside an engine call that
does not return.

Before                                             After

┌───────────────────────────┐                      ┌───────────────────────────┐
│ stop pending drops        │                      │ stop pending drops        │
│ wait: unbounded           │                      │ bound: 5s                 │
└─────────────┬─────────────┘                      └─────────────┬─────────────┘
              │                                                  │
              ▼                                                  ▼
┌───────────────────────────┐                      ┌───────────────────────────┐
│ stop durable dispatch     │                      │ stop durable dispatch     │
│ wait: unbounded           │                      │ bound: 10s                │
└─────────────┬─────────────┘                      └─────────────┬─────────────┘
              │                                                  │
              ▼                                                  ▼
┌───────────────────────────┐                      ┌───────────────────────────┐
│ reconcile pass            │                      │ reconcile pass            │
│ not waited on at all      │  ✗ storage closes    │ bound: 5s + 2s grace      │
└─────────────┬─────────────┘                      └─────────────┬─────────────┘
              │                                                  │
              ▼                                                  ▼
┌───────────────────────────┐                      ┌───────────────────────────┐
│ drain in-process work     │                      │ drain in-process work     │
│ wait: 25s                 │                      │ bound: 25s                │
└─────────────┬─────────────┘                      └─────────────┬─────────────┘
              │                                                  │
              ▼                                                  ▼
┌───────────────────────────┐                      ┌───────────────────────────┐
│ stop operator             │                      │ stop operator             │
│ wait: unbounded,          │                      │ bound: 10s, then          │
│ never returns             │  ✗ stuck here        │ abandon and log           │  ✓ claim left stale
└─────────────┬─────────────┘                      └─────────────┬─────────────┘
              │                                                  │
              ▼                                                  ▼
┌───────────────────────────┐                      ┌───────────────────────────┐
│ telemetry, then storage   │                      │ telemetry, then storage   │
│ never reached             │  ✗                   │ closed                    │  ✓
└───────────────────────────┘                      └───────────────────────────┘

Each bound is stated where it is declared, together with what its stage gives up
by expiring. None of them gives up anything another instance cannot redo or
reclaim: a claimed webhook delivery is redelivered, a repair pass that did not
finish is rerun by the next instance to start, a monitor recomputes its whole
view on its next pass, and an apply whose driver never returned is reclaimed by
a peer's stranded reaper. Waiting longer buys none of that back — it only delays
the exit the recovery is waiting on.

The worst case is now the sum of the stage bounds, a little over a minute,
against a previous worst case that did not exist. The in-process webhook drain
is by far the largest term and is unchanged here; the stage constants are what
make lowering the total a question that can now be answered.

Also in here

• A bootstrap that reaches the front of the advisory-lock queue with its
  deadline already consumed by the wait is a named, logged refusal rather than a
  pass that starts online DDL it cannot finish. It releases the lock to the next
  instance immediately, and the boot retry loop opens the next attempt on a
  fresh deadline.
• The sites that branch on ctx.Err() during the bootstrap now separate a
  caller that cancelled from a deadline that expired. The two mean different
  things to whoever reads the log: one is an instance that was told to stop, the
  other is a database that was too slow.
• pkg/drain holds the one bounded-wait helper the shutdown path uses, so no
  stage can reintroduce an unbounded Wait() by copying its neighbour.

No operator-facing surface changes: the PR comments, check summaries, and CLI
output are all untouched. What changes is the logs, which now name the cause of
every early exit.

This PR was prepared by Claude Code (Claude Opus 5).

[Copilot]

🟡 Changes recommended

Unresolved critical shutdown and API compatibility issues block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR makes server startup cancellation-aware and adds bounded shutdown drains.

Changes:

• Propagates signal-aware contexts through serve and schema bootstrap.
• Adds bounded waits for monitors, webhooks, reconciliation, and drivers.
• Adds cancellation, drain, lock-budget, and signal integration coverage.

File summaries

File	Review summary
pkg/webhook/durable_dispatch.go	Nit (1 vote): Timeout logs lack delivery or repository identity needed to triage abandoned claims.
pkg/webhook/durable_dispatch_drain_test.go	No final findings.
pkg/serve/serve.go	Critical (3 votes): GracefulStop is unbounded. Moderate (3 votes): Signal cause reporting is racy. Critical (1 vote): Database closure can still block indefinitely on abandoned queries.
pkg/serve/serve_shutdown_test.go	No final findings.
pkg/drain/drain.go	Critical (1 vote): The helper bounds only waiting, not work or resources, so storage closure can still hang.
pkg/drain/drain_test.go	No final findings.
pkg/cmd/serve_signal_integration_test.go	Moderate (2 votes): Binary compilation has no independent deadline.
pkg/cmd/commands/storage_integration_test.go	No final findings.
pkg/cmd/commands/serve.go	No final findings.
pkg/api/webhook_inbox_metrics.go	No final findings.
pkg/api/shutdown.go	Nit (1 vote): Missing focused coverage for bounded monitor shutdown.
pkg/api/resync_postgres_identity_integration_test.go	No final findings.
pkg/api/remote_deployment_health.go	No final findings.
pkg/api/pending_drops_cleaner.go	Moderate (1 vote): May log that the cleaner stopped after abandoning it, while it can race storage closure.
pkg/api/operator.go	Moderate (1 vote): Reaper goroutines can skip engine and claim cleanup. Moderate (2 votes): Abandoned drivers can race dependency closure and violate lease safety.
pkg/api/operator_stuck_applies.go	No final findings.
pkg/api/operator_shutdown_drain_test.go	No final findings.
pkg/api/mysql_shared_integration_test.go	No final findings.
pkg/api/ensure_schema.go	Critical (2 votes): The exported EnsureSchema signature is source-incompatible for external embedders.
pkg/api/ensure_schema_test.go	No final findings.
pkg/api/ensure_schema_postgres.go	No final findings.
pkg/api/ensure_schema_postgres_test.go	No final findings.
pkg/api/ensure_schema_postgres_pooling_integration_test.go	No final findings.
pkg/api/ensure_schema_postgres_integration_test.go	No final findings.
pkg/api/ensure_schema_lock_budget_test.go	No final findings.
pkg/api/ensure_schema_lock_budget_integration_test.go	No final findings.
pkg/api/ensure_schema_integration_test.go	No final findings.
pkg/api/canonicalize_postgres_identity_integration_test.go	No final findings.
docs/invariants.md	No final findings.

Review details

Suppressed comments (4)

pkg/api/operator.go:198

• recoveryWg also contains the stranded and retryable-expiry reaper goroutines started in StartOperator, so this is not a driver-only drain. If a reaper is the goroutine that ignores cancellation while all drivers have returned, this branch skips haltEnginesForShutdown and drainHeldClaims; an in-process engine can remain running and healthy claims can be left stale, unnecessarily blocking peers. Separate maintenance draining from driver draining or still run the cleanup for completed drivers (OW-3).

	if !drain.Wait(&s.recoveryWg, driverDrainTimeout) {
		s.logAbandonedDrives()
		return

pkg/api/pending_drops_cleaner.go:101

• drainMonitor can return after its timeout while the cleaner goroutine is still running, but the next line unconditionally logs pending drops cleaner stopped. During the failure mode this reports a false lifecycle state, and the still-running cleaner can race the storage close that follows. Return whether the wait completed and avoid the stopped message (or log abandonment) when it did not.

	s.drainMonitor(&s.pendingDropsWg, "pending_drops_cleaner")

pkg/api/shutdown.go:32

• This timeout branch is now shared by four monitor stop methods, but the added tests only exercise the operator-specific drain; no test blocks a monitor past cancellation and verifies the bounded return and log. A regression that restores an unbounded monitor wait would therefore pass. Add a focused test through a monitor stop method for this branch.

	if drain.Wait(wg, monitorDrainTimeout) {
		return
	}
	s.logger.Warn("background monitor did not return within the shutdown drain; the close continues without it and its next pass runs in whichever process starts next",
		"monitor", monitor,
		"drain_timeout", monitorDrainTimeout)

pkg/webhook/durable_dispatch.go:126

• On timeout this log has no delivery/event identity or repo/PR information, so an operator cannot identify which acknowledged inbox rows were left claimed or correlate their later redelivery. The driver timeout path enumerates abandoned apply identities; durable dispatch should snapshot and log the claimed delivery IDs before returning so AV-7 remains triageable.

	if !drain.Wait(&h.durableWebhookWg, durableWebhookDrainTimeout) {
		h.logger.Error("durable webhook deliveries did not return within the shutdown drain; their inbox rows stay claimed until the claim goes stale and the next process to run the pool redelivers them",
			"drain_timeout", durableWebhookDrainTimeout)
		return

• Files reviewed: 29/29 changed files
• Comments generated: 7
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.