fix(tern): complete a deployment-keyed apply only when its generation manifest is satisfied

[aparajon]

Why this matters

When a schema change fans out across shards, the control plane sends the work to the data plane one operation at a time: one dispatch per shard, plus a finalizer that applies the VSchema at the end. All of those dispatches land on one shared apply on the data plane.

The bug: the data plane decided that shared apply was "complete" by looking only at the operations that had arrived so far. If the first shard finished before its siblings were dispatched, the apply was marked completed — and because a completed apply refuses new work, every late-arriving sibling was then turned away. The apply reported success while most of the work never ran.

The rule this PR enforces: an apply completes only when all of its shards and finalizers are done.

What it does

Every dispatch now declares up front the full list of operations its deployment will send — its generation manifest. The data plane stores that list on the apply at creation and holds the apply open until the list is satisfied:

before: completion inferred from whatever has arrived

  dispatch shard -80  ──▶  apply created
  shard -80 finishes  ──▶  apply marked COMPLETED        ✗ too early
  dispatch shard 80-  ──▶  refused: apply is terminal
  dispatch finalizer  ──▶  refused: apply is terminal
                           "success", but 2 of 3 operations never ran

after: completion declared up front

  dispatch shard -80  ──▶  apply created, manifest stored:
                           { -80, 80-, finalizer }
  shard -80 finishes  ──▶  apply stays RUNNING (80- and finalizer missing)
  dispatch shard 80-  ──▶  attaches (named in the manifest)
  dispatch finalizer  ──▶  attaches (named in the manifest)
  all three terminal  ──▶  apply COMPLETED

The manifest is enforced fail-closed at three points:

1. Completion hold — the apply cannot take a whole-generation verdict (completed or reverted) while any operation named in the manifest is missing or unfinished. A held apply logs the missing keys, and schemabot.apply_manifest_hold_total counts sustained holds so a dispatcher that died mid-generation is visible instead of silent.
2. Attach gate — a dispatch for an operation not named in the stored manifest is refused. Undeclared work would be work the completion check never waits for.
3. Creation gate — a dispatch whose manifest omits its own operation is refused outright, rather than creating an apply that could never complete.

Failures are never held: if any operation fails, the apply reports the failure immediately instead of waiting for siblings that may never arrive. Terminal stays terminal — nothing reopens a completed apply. An apply with no stored manifest keeps today's behavior, so old and new planes can be deployed in either order.

Retries follow the same honesty rule: a deliberate retry redispatches only its own operation (successful siblings never dispatch again), so a retry gets its own operation-scoped remote apply declaring just that one operation — it can never wait on siblings that will not come.

The manifest also settles a long-standing ambiguity: a dispatch carrying a single namespace's VSchema could be either that namespace's finalizer (one of several in a sharded change) or the whole deployment's finalizer (a VSchema-only change). The two are indistinguishable on arrival, but the manifest names the dispatcher's actual keys, so the data plane now adopts whichever shape was declared.

Each deployment declares only its own operations, and each data plane gates only on what was declared to it:

  control plane
  plan → operations grouped by deployment

    deployment A ── manifest { ks/-80/t, ks/80-/t, ks/group_finalizer }
    deployment B ── manifest { ks/t }
    deployment C ── manifest { ks/t }
          │                 │                 │
          ▼                 ▼                 ▼
    data plane A      data plane B      data plane C
    holds its apply   holds its apply   holds its apply
    to A's manifest   to B's manifest   to C's manifest

  the control plane aggregates the per-deployment applies into the
  overall rollout verdict — no data plane ever waits on another
  deployment's operations

How it moves us toward the northstar

In the target architecture the control plane composes the full operation set for a schema change, and each data plane reconciles against that declared set. This PR carries the declared set across the plane boundary for the first time: the data plane now completes against what was declared, not what happened to arrive. The natural next step is to materialize every declared operation row when the keyed apply is created, so a later dispatch simply claims its row instead of attaching a new one.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

Adds deployment generation manifests so Tern data planes wait for every declared shard and finalizer before completing an apply.

Changes:

• Sends and persists per-deployment operation manifests.
• Gates apply creation, attachment, and completion against the manifest.
• Adds schema, telemetry, and integration coverage.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/tern/sharded_apply_dispatch_test.go	Tests manifest-aware finalizer scope.
pkg/tern/local_dispatch_finalizer_integration_test.go	Tests VSchema-only keyed dispatches.
pkg/tern/local_dispatch_attach_integration_test.go	Tests manifest persistence and attach gates.
pkg/tern/local_client.go	Validates, stores, and enforces manifests.
pkg/tern/grpc_client.go	Sends deployment operation manifests.
pkg/tern/grpc_client_test.go	Tests manifest construction.
pkg/storage/types.go	Adds manifest storage helpers.
pkg/storage/types_test.go	Tests manifest helper behavior.
pkg/storage/internal/sqlstore/applies.go	Persists and loads manifests.
pkg/schema/postgres/applies.sql	Adds the PostgreSQL manifest column.
pkg/schema/mysql/applies.sql	Adds the MySQL manifest column.
pkg/proto/ternv1/tern.pb.go	Updates generated protocol bindings.
pkg/proto/tern.proto	Adds the manifest request field.
pkg/metrics/metrics.go	Adds refusal and completion-hold metrics.
pkg/api/operator.go	Holds completion for missing operations.
pkg/api/operator_test.go	Tests manifest-gated state projection.
pkg/api/ensure_schema_postgres_test.go	Updates PostgreSQL schema expectations.

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.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1076, e27aa06.

Verdict: 3 findings — 1 blocking (Reverted bypasses the manifest hold), 2 non-blocking (log field gap, retry co-rotation test coverage).

Blocking

• pkg/api/operator.go:2339 — The new manifest-completeness hold only guards state.IsState(derived, state.Apply.Completed); it does not also guard state.Apply.Reverted. state.DeriveApplyState (pkg/state/apply.go:136-139) returns Apply.Reverted as soon as counts[Apply.Reverted] > 0 — a single reverted child operation, not unanimity across the manifest — and this check runs before the counts[Apply.Completed] == total check. Because pkg/tern/local_control_reconcile.go's adoptableEngineTerminalStates lets one shard's operation independently adopt Reverted ("a multi-operation drive owns only its operation," line 106) while sibling operations from the same generation manifest (ExpectedOperationKeys) have never attached, a 3-shard deployment-keyed apply can have one shard revert and be written terminal (Reverted, completed_at stamped) while the other two manifest-declared shards were never touched — exactly the premature-completion bug this PR fixes for Completed, reproduced for Reverted. TestUpdateApplyStateFromOperations_ManifestGatesCompletion covers Completed (held) and Failed (passes through, intentionally) but has no equivalent case for Reverted, confirming this is untested as well as unguarded.

Non-blocking

• pkg/tern/local_client.go:2562 — The creation-gate refusal log ("Apply: dispatch's generation manifest does not name its own operation") hand-lists plan_id, operation_key, idempotency_key, manifest instead of calling apply.LogAttrs(), even though a fully-populated *storage.Apply (Database, DatabaseType, Deployment, Environment, Repository, PullRequest, Caller) is in scope at that point. This drops database, database_type, environment, and deployment from the one log line an operator would grep during an incident to find which target is sending malformed dispatches. The sibling gates added in this same PR — validateDispatchAgainstManifest's Warn (lines ~2259-2273) and refuseAttachToTerminalApply (lines ~2298-2303) — both correctly use apply.LogAttrs(), making this an inconsistent outlier within the PR and a violation of AGENTS.md's "build these with the LogAttrs() helpers... rather than hand-listing identifiers."
• pkg/tern/grpc_client_test.go:7053-7103, 7004-7045 — No test jointly exercises the retry (Attempt > 0) key-rotation and manifest-rotation for the same scope object. TestGenerationOperationKeys (grpc_client_test.go:7053-7103, including its retry case at line 7100 setting scope.operation.Attempt = 1) only asserts scope.generationOperationKeys(). TestRemoteApplyIdempotencyKey_GenerationRotation (grpc_client_test.go:7004-7045) only asserts remoteApplyIdempotencyKey(...). Neither test calls both functions on the same scope. If a future edit desyncs the two Attempt > 0 guards (grpc_client.go:1478 in generationOperationKeys, grpc_client.go:1593 in remoteApplyIdempotencyKey) — e.g. one changes to Attempt >= 1 or gets an extra condition — both suites would keep passing individually while a retried remote apply could be created under a manifest it can never satisfy, permanently holding it Running under the new completion-hold gate with only schemabot.apply_manifest_hold_total ticking as the operator-visible signal. The code is correct today; this is a coverage gap on the PR's most safety-critical invariant, not an existing bug.

The one thing that could have broken, verified

The retry key/manifest co-rotation mechanism in pkg/tern/grpc_client.go: on a deliberate retry (operation.Attempt > 0), remoteApplyIdempotencyKey() (line 1593) must rotate to an operation-scoped idempotency key, and generationOperationKeys() (line 1478) must simultaneously narrow the manifest to that operation's own key alone. If only one of the two rotated — key stayed deployment-scoped while the manifest narrowed, or vice versa — a retry's fresh remote apply would be created with a manifest declaring siblings that already succeeded on the old apply and will never redispatch to the new one, permanently holding the retried apply Running under the completion-hold gate with no operator-visible failure beyond a slow-ticking hold metric.

Verified: both guards are textually identical today — if s.operation.Attempt > 0 (generationOperationKeys, line 1478) and if scope.operation.Attempt > 0 (remoteApplyIdempotencyKey, line 1593) — so there is no drift right now, and I confirmed by direct read that they are two independent conditions in two different functions, not derived from one shared predicate/helper, so nothing in the code itself prevents them from drifting apart in a future edit. The joint test-coverage gap called out above was confirmed, not refuted, by the verify phase: grepping both function names across grpc_client_test.go shows zero tests calling both remoteApplyIdempotencyKey and generationOperationKeys on the same scope object, including the retry-focused assertions in each respective test. So: the mechanism is safe as written today, but the two Attempt > 0 conditions are not structurally protected from drifting apart — only a joint test would catch that, and no such test exists.

Verified correct

• The completion hold's if state.IsState(derived, state.Apply.Completed) gate (operator.go:2327-2341) correctly excludes Failed from waiting on manifest completeness, matching the PR's documented intent that a failed generation must not wait for siblings that may never dispatch — confirmed by direct read of the state-derivation order and the accompanying comment (Failed is excluded on purpose; Reverted is excluded by omission, see Blocking above).
• The creation gate correctly runs BEFORE any row is created: operationKey is derived at local_client.go:2551, the !apply.AllowsOperationKey(operationKey) check and early return happen at lines 2561-2571, and CreateWithTasksAndOperations isn't called until line 2593 — a malformed dispatch (manifest omitting its own key) is refused before any row exists, so nothing is stranded wedged-open.
• apply.MissingExpectedOperationKeys and apply.AllowsOperationKey (pkg/storage/types.go ~468-501) are nil-safe: nil apply, nil/empty manifest all resolve to allow/no-missing-keys, and MissingExpectedOperationKeys returns keys in manifest order (deterministic), not attach order.

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

[aparajon]

🤖 All three findings are addressed in 21aac54:

1. Reverted verdict bypassing the manifest hold (blocking) — the hold now gates both whole-generation verdicts via a manifestGatedVerdict() helper: neither completed nor reverted can land while manifest-declared operations are missing. Failure verdicts still pass through unheld, since a failed generation must not wait on siblings that may never dispatch. Covered by two new cases in TestUpdateApplyStateFromOperations_ManifestGatesCompletion.
2. Creation-gate refusal log — now built with apply.LogAttrs() like the sibling gates.
3. Joint co-rotation coverage — TestRetryRotatesKeyAndManifestTogether drives both remoteApplyIdempotencyKey and generationOperationKeys() on the same scope across the attempt boundary, pinning that a retry rotates the key and narrows the manifest in the same step.

This reply was posted by Claude Code (Claude Fable 5) on Armand's behalf.