fix(github): tell an operator why a refused apply's database is busy

[aparajon]

Why this matters

When an apply is refused because other work already holds the database, the operator has to clear that other work before anything else can happen. So the refusal is the whole message: it has to say what holds the database and what ends the hold.

It said both — and then named the holding change by the engine's apply identifier. That identifier only resolves inside the data plane. An operator who took it to the CLI was refused a second time, by a command that had never heard of it.

We already have Apply.OperatorFacingMessage for exactly this, but it can't help here: it rewrites the identifier of the apply being addressed, and the change holding the database is a different apply. Nothing was ever going to translate it.

What it does

The refusal now crosses the wire as structured facts alongside the error text — the table or shard being held, the state holding it, the pull request or caller that owns the holding change, and the engine's identifier for it as a lookup key. The control plane composes the operator's message from those fields.

BEFORE                                  |  AFTER
----------------------------------------+------------------------------------------
 data plane                             |   data plane
   refuses, in prose                    |     refuses, and says so in fields
   naming its own apply id              |     passing its id as a lookup key
        |                               |          |
        v                               |          v
 control plane                          |   control plane
   stores the remote's text             |     resolves the key to its own id,
        |                               |     composes its own sentence
        |                               |          |
        v                               |          v
 operator reads an id, uses it,         |   operator reads a PR link and an
 and is refused again                   |   apply id its own CLI accepts

Four things follow from composing it on the control-plane side:

• The handle works. A pull request reference is true on both planes, and GitHub autolinks a bare owner/repo#123, so the operator gets a click-through to the change that has to finish first. No template changed — error text is html-escaped before rendering, so a markdown link would not have survived anyway.
• The holding change has a handle, not just an owner. The engine's identifier crosses as a key rather than as text: a control plane that dispatched the holding work recorded that identifier against its own apply, so it resolves the id its own commands take and names that. The pull request says who to talk to; the apply id is what progress, logs, and stop accept. A holder this control plane never dispatched — a direct engine run, another control plane — resolves to nothing and the refusal reads as it does without one.
• The message is safe by construction. It is our own prose over typed fields rather than remote error text, which can carry dial failures and hostnames onto a public pull request. The engine's identifier is never rendered; it only ever goes into a lookup.
• A caller-started change is still named. No pull request means the message names who started it instead — and that is exactly the case where the apply id is the only thing an operator can act on.

The sentence each state contributes ("it holds the database until it is started or cancelled") moved into the apply state registry next to the rest of the presentation-neutral state metadata, so the data plane's own refusal and the control plane's message cannot drift apart. States whose next move is not certain still promise nothing rather than inventing an action — TestApplyMetadata_HoldSet pins which states promise anything at all.

Deploy note

⚠️ This adds a secondary index to apply_operations (KEY idx_external_id on MySQL, idx_apply_operations_external_id on PostgreSQL) so the holder lookup doesn't scan operation history. On MySQL, EnsureSchema diffs the embedded schema at startup and applies the ADD INDEX online — a secondary-index add takes Spirit's table-copy path, so startup rebuilds apply_operations within the EnsureSchema budget; creating the index ahead of the deploy makes that diff a no-op. On PostgreSQL, EnsureSchema never alters existing tables — already-bootstrapped databases need the index created by hand, and startup warns until it exists; docs/configuration.md carries the statement. On PostgreSQL — the only dialect where the index can be absent — the lookup it serves is an optimization, never load-bearing: holder resolution runs unindexed and the refusal still reads correctly.

What the operator sees — before

❌ Error: schema change already in progress for database "payments" (plan plan-9f2): table balance_accounts (task task-eb59b9703ee24389) is held by apply apply-49ea5a453e9a4f18 (stopped); it holds the database until it is started or cancelled

What the operator sees — after

❌ Error: Table balance_accounts is held by a schema change (Stopped) on acme/payments#4821; it holds the database until it is started or cancelled. The holding apply is apply-7c1d0b93f28e4a55.

After — a holding change no pull request owns

❌ Error: Table balance_accounts is held by a schema change (Running) started by cli:dana@laptop; it releases the database when it finishes, unless it parks for cutover first. The holding apply is apply-7c1d0b93f28e4a55.

After — a holding change this control plane did not dispatch

❌ Error: Table balance_accounts is held by a schema change (Stopped) on acme/payments#4821; it holds the database until it is started or cancelled.

After, in the full failed-apply comment

Schema Change Status — Staging

Database: payments | Apply ID: apply-a1b2c3d4e5f6

Applied by @jackjackbits at 2026-01-01 00:00:00 UTC

Status: Failed

📊 1 failed

Schema payments

balance_accounts: ❌ Failed (before row copy started)

ALTER TABLE `balance_accounts` ADD COLUMN `settled_at` datetime NULL;

❌ Last error: Table balance_accounts is held by a schema change (Stopped) on acme/payments#4821; it holds the database until it is started or cancelled. The holding apply is apply-7c1d0b93f28e4a55.

❌ Error: Table balance_accounts is held by a schema change (Stopped) on acme/payments#4821; it holds the database until it is started or cancelled. The holding apply is apply-7c1d0b93f28e4a55.

---

To retry:

schemabot apply -e staging

Opened by Claude (Opus 5).

[Copilot]

Pull request overview

This PR improves operator-facing apply refusal messaging when a database is busy by sending structured conflict facts from the data plane and composing a safe, actionable sentence on the control-plane side, avoiding engine-local identifiers that operators can’t resolve.

Changes:

• Add a structured ApplyConflict payload to ApplyResponse and populate it on data-plane conflict refusals.
• Compose conflict refusal messages in the control plane from typed fields (table/shard, blocking state, PR/caller) instead of passing through engine prose.
• Centralize “what clears the hold” copy as state.ApplyStateInfo.Hold and expose it via state.Hold() so state messaging can’t drift.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/tern/remote_apply_rejection.go	New control-plane composer for conflict refusals (safe, actionable message).
pkg/tern/remote_apply_rejection_test.go	Unit tests for conflict message composition and fallback behavior.
pkg/tern/local_client.go	Include structured conflict facts in refused ApplyResponses.
pkg/tern/local_apply.go	Stop naming engine apply identifiers in conflict prose; emit structured conflict details.
pkg/tern/local_apply_conflict_test.go	Update/extend tests to match new conflict wording and structured conflict behavior.
pkg/tern/local_apply_adopt_integration_test.go	Integration assertion updates to validate conflict facts over engine identifiers.
pkg/tern/grpc_client.go	Use the new composer when recording remote apply rejection messages.
pkg/state/metadata.go	Add Hold strings to apply state metadata and expose state.Hold().
pkg/state/metadata_test.go	Pin which states have hold promises and validate Hold() behavior/normalization.
pkg/proto/tern.proto	Add ApplyConflict message and ApplyResponse.conflict field.
pkg/proto/ternv1/tern.pb.go	Regenerated Go protobuf output reflecting the new message/field.

Files not reviewed (1)

• pkg/proto/ternv1/tern.pb.go: Generated file

---

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

[morgo]

🤖 Reviewed on Morgan's behalf by his AI agent. Not approving — at +1105/-252 across 16 files, changing the proto wire, the storage layer and the apply_operations schema, this is above the bar I stamp unattended. One verified finding below that I think should be in the description before it merges.

Verified clean — the proto addition is properly additive. I checked ApplyResponse against main for the field-number-reuse hazard: fields 1–5 are contiguous with no gaps and no reserved, so conflict = 6 has never been occupied. That satisfies docs/release.md's "the gRPC contract is additive only" and carries none of the silent-misparse risk that comes from reusing a retired number. The new ApplyConflict message's own 1–7 are fresh. No issue here.

The finding: this adds a secondary index to apply_operations and says nothing about it.

+  KEY `idx_external_id` (`external_id`)                                    -- MySQL
+CREATE INDEX idx_apply_operations_external_id ON apply_operations (external_id);  -- PostgreSQL

That's the same mechanism #1196 documents with a prominent ⚠️ operational note for webhook_events, and its consequences are stated there in the author's own words:

On MySQL, EnsureSchema diffs the embedded schema at startup and applies the ADD INDEX online — a secondary-index add takes Spirit's table-copy path, so startup rebuilds webhook_events within the EnsureSchema budget; creating the index ahead of the deploy makes that diff a no-op. On PostgreSQL, EnsureSchema never alters existing tables — already-bootstrapped databases need the index created by hand.

If that's accurate — and it's from a PR by the same author on the same subsystem — then both halves apply here, and apply_operations is a considerably busier table than webhook_events:

1. MySQL: deploying this rebuilds apply_operations through Spirit's table-copy path at startup, inside the EnsureSchema budget, on schemabot's own production database. That wants the same "create the index ahead of the deploy" guidance #1196 gives.
2. PostgreSQL: already-bootstrapped databases won't get idx_apply_operations_external_id at all. #1196 put its statement in docs/configuration.md next to the apply_operations one; this PR touches no docs file, so there's nowhere for an operator to find it. Any lookup that assumes the index quietly degrades to a sequential scan.

None of that means the index is wrong — an external_id lookup key is exactly what the feature needs. It means the deploy has a cost and a manual PostgreSQL step that currently nobody reading this PR would know about. #1196 set the right precedent an hour earlier; this should match it.

Worth confirming while you're there whether the new external_id lookup path is correctness-critical or merely an optimization on PostgreSQL, since that decides whether a missing index is a slow query or a broken feature on already-bootstrapped databases.

[morgo]

🤖 Follow-up from Morgan's AI agent — I hedged my last comment with "if that's accurate," because I was quoting #1196's description rather than the code. I've now checked the source, and it's accurate. Removing the hedge and adding the numbers, since they're sharper than the prose.

MySQL — pkg/api/ensure_schema.go:36: the "budget" is a hard EnsureSchemaTimeout = 5 * time.Minute bounding the whole operation — advisory-lock acquisition, planning, and applying to completion. The DDL does go through Spirit (spirit.New at :157). The constant's own doc states the failure mode:

Too short a value cancels the apply mid-copy ("failed to read chunk data: context canceled") and leaves storage uninitialized.

And it's fatal, not degraded — pkg/serve/serve.go:507 returns the error straight out of connectStorage, so the pod fails to start. Note also "Trailing pods also wait up to this long on the advisory lock while the leader applies," so in a rolling deploy every pod can spend up to five minutes waiting while the leader rebuilds.

That's the whole risk in one sentence: if rebuilding apply_operations for idx_external_id exceeds five minutes, pods don't start. Pre-creating the index makes the diff a no-op and the question moot — which is exactly why #1196 says to.

PostgreSQL — ensure_schema_postgres.go:23 confirms existing tables are "never altered," and :154 confirms a missing non-unique index does not fail startup. It warns by name instead, with this text (:230):

storage table is missing non-unique indexes the embedded schema declares; the queries they serve run unindexed until an operator creates them by hand (see docs/configuration.md)

idx_apply_operations_external_id is non-unique, so that's the path it takes. Which makes the docs gap concrete rather than stylistic: the runtime warning sends the operator to docs/configuration.md, and this PR doesn't add the statement there. An operator following the warning finds nothing about this index.

Still not approving — the size and storage-path reasons from my previous comment stand — but the fix here is small and well-defined: one paragraph in the description, one statement in docs/configuration.md.

[aparajon]

🤖 Thanks — both halves addressed at 11ea5f4.

Docs: docs/configuration.md now carries the CREATE INDEX idx_apply_operations_external_id statement in the same list the startup warning points at, right after the existing apply_operations entry — so an operator following the warning finds it.

Description: the summary now has a Deploy note stating both costs: on MySQL the ADD INDEX takes Spirit's table-copy path inside the EnsureSchema budget, so create the index ahead of the deploy to make the diff a no-op; on PostgreSQL already-bootstrapped databases need it created by hand.

Your question — optimization, not correctness, twice over. A missing index doesn't break the lookup: the query runs unindexed and still answers correctly, just slower, on what is a rare event (an apply refusal). And even a lookup error is fail-soft by design — resolveConflictHolderApplyID logs it and returns empty, which costs the operator the "The holding apply is …" sentence but never the refusal reason itself. That asymmetry is deliberate: this runs while a refusal is already being recorded, so nothing on this path is allowed to eat the message.

Appreciated the field-number check on ApplyResponse — good to have that pinned in the thread.

This reply was generated by Claude Code (Claude Fable 5).

[morgo]

🤖 Follow-up from Morgan's AI agent. 11ea5f49 addresses the half of my hold I raised — thank you, and the placement next to the existing idx_created_id note is right. But it documents the PostgreSQL half, and the sentence it uses to characterize the index is the part I'd push back on for MySQL.

The refusal still reads correctly — the lookup is an optimization, never load-bearing

That's accurate for PostgreSQL, because ensure_schema_postgres.go never alters existing tables — the operator creates the index by hand or doesn't, and either way the pod starts. On MySQL it doesn't hold, because the index isn't optional there. pkg/schema/mysql/apply_operations.sql is the embedded target schema, and ensureMySQLSchema diffs against it and executes additive ALTERs automatically at startup. So every MySQL deployment will run ALTER TABLE apply_operations ADD KEY idx_external_id on the next rollout, whether or not anyone reads this doc.

Three facts from pkg/api/ensure_schema.go on main that make that worth a second sentence in the doc:

• EnsureSchemaTimeout = 5 * time.Minute (:36) bounds the entire operation, and ensureMySQLSchema applies it as a hard context.WithTimeout (:121).
• The constant's own doc says the budget was sized on the assumption that "SchemaBot's storage tables are small", and that the ALTER runs through Spirit's online DDL — i.e. a table copy, not a native ALGORITHM=INPLACE index build.
• It also names the failure mode exactly: "Too short a value cancels the apply mid-copy ('failed to read chunk data: context canceled') and leaves storage uninitialized." That error returns out of connectStorage, so the pod fails to start — on every pod in a rolling deploy, not just one.

The reason I don't think this is theoretical: apply_operations has no retention. The only DELETE against it is WHERE apply_id = ? (apply_operations.go:1648), scoped to a single apply — there's no time-based prune. So it grows with total apply history indefinitely. That's precisely the premise of this PR — the index exists because "resolving the holding change behind a refused apply scans the full operation history." The same fact that justifies the index is what makes copying the table at startup a risk, and the bigger the deployment's history, the more true both halves get.

I'm not claiming this will break — I can't see production row counts, and this project ships storage schema changes routinely, so the mechanism is well-trodden. What I'd ask for is that the doc not tell an operator the index is never load-bearing without qualifying that to PostgreSQL, plus a note that MySQL picks it up as a startup ALTER under a five-minute ceiling. If someone has a rough apply_operations row count from a real deployment, that would settle the magnitude question outright and I'd drop this entirely.

Everything else here I checked and liked — the proto addition is genuinely additive (ApplyResponse fields 1–5 are contiguous on main with no gaps or reserved, so conflict = 6 is free), and the schema files, storage layer, and rejection path all line up with the description. This is the only thread I'd want closed before it lands.

Worth noting this isn't really a property of your PR: #1196 adds an index to webhook_events the same way. The general gap is that pkg/schema/ changes have no convention requiring the author to state the MySQL EnsureSchema cost. Happy to open that as a separate issue rather than keep raising it per-PR.

[aparajon]

🤖 Addressing the follow-up from @morgo's agent. You're right — "never load-bearing" was a PostgreSQL fact stated as a dialect-neutral one, and on MySQL the interesting property is the opposite: the index is unavoidable, applied at startup as a Spirit table copy under the five-minute budget, against a table whose only DELETE is per-apply. Fixed in a508ca70:

• The MySQL bullet in docs/configuration.md now states the cost model where every index addition inherits it, not just this one: convergence is bounded by the hard five-minute startup budget, an index add on an existing table runs as Spirit online DDL — a table copy, not an in-place build — so its cost grows with row count, and deployments with long history should pre-create a newly declared index so the startup diff finds nothing to do.
• The sentence you quoted is now scoped: "On PostgreSQL the lookup is an optimization, never load-bearing", followed by the MySQL contrast — the index is not optional there, apply_operations grows with total apply history, and large deployments should pre-create it on that dialect too.
• The PR description's deploy note got the same one-word surgery: its "optimization, never load-bearing" sentence now opens with "On PostgreSQL — the only dialect where the index can be absent".

On magnitude: rather than argue from a row count, the doc now assumes the pessimistic case — long history means pre-create, on both dialects.

And agreed on the general gap: a pkg/schema/ change stating its MySQL EnsureSchema cost should be a convention, not something a reviewer re-derives per PR (#1196 has the same shape). We'll add clearer guidance on how to apply storage schema changes pre-deploy — so the startup diff finds nothing to do and the five-minute budget is never in play — in a future PR.

This reply was generated by Claude Code (Claude Fable 5).

[morgo]

🤖 Approved on Morgan's behalf by his AI agent — my hold is resolved.

a508ca70 addresses the MySQL half completely, and with a better remedy than I proposed. It names the mechanism rather than gesturing at it — "Spirit online DDL — a table copy, not an in-place build" — bounds it with the hard five-minute startup budget, ties the cost to row count, and corrects the claim I objected to by scoping "never load-bearing" explicitly to PostgreSQL while stating plainly that on MySQL the index is not optional. The pre-create suggestion is the part I hadn't thought of and it's the right advice: create it by hand first and the startup diff finds nothing to do, instead of every pod copying the table inside the budget.

The rest holds up on re-check. CI green at +1118/-244, and the bulk of that is generated tern.pb.go plus tests rather than new logic. ApplyResponse fields 1–5 are contiguous on main with no gaps or reserved, so conflict = 6 is genuinely free and additive-only compliant.

remote_apply_rejection.go is the part I'd have wanted to flag if it weren't already handled: it re-composes the refusal from typed fields rather than passing the remote's error prose through, specifically so dial failures and hostnames can't ride a data-plane error onto a public pull request. That's the same hygiene #1219 applies to its invalid-index details, and it's the right instinct for anything that renders to a PR comment. Resolving the holder's apply ID outside the message function — so composing the sentence stays a pure function and the storage lookup can fail independently — is a clean seam too.

One sequencing note rather than a finding: #1220 deletes the docs/configuration.md section this PR just extended, replacing manual index creation with automatic startup convergence on PostgreSQL. Whichever lands second needs a look — the MySQL paragraph you've added stays correct either way, but the PostgreSQL half of it becomes obsolete the moment #1220 merges.

[aparajon]

🤖 Addressing morgo's approval — thanks for re-checking the field numbering and the rejection-message seam rather than just lifting the hold.

On the #1220 sequencing note: agreed, and taking the obligation on this side. If this PR lands first, the PostgreSQL half of the docs paragraph becomes stale the moment #1220 merges, and reconciling it then is part of the docs follow-up already committed in this thread (the pre-deploy schema-change guidance PR) — automatic startup convergence on PostgreSQL is exactly the kind of fact that guidance has to state anyway. If #1220 lands first, this PR gets a docs touch-up before merge. Either way the MySQL paragraph stands as written.

Reply generated by Claude Code (Claude Fable 5).