fix(vitess): gate stored-plan applies on recorded VSchema deletions and mutations

[aparajon]

Why this matters

Plan time already treats a VSchema removal (a deleted vindex or routing entry) and an in-place vindex mutation (same name, different routing behavior) as unsafe changes requiring the same operator opt-in as destructive DDL, because either changes Vitess query routing the moment the VSchema is applied. But that verdict lived only in the live plan response: persisting a plan kept just the changed flag for artifact attachment and dropped the deletion and mutation records, so the stored-plan apply gate checked DDL alone. An apply created from a stored plan could remove or rewire vindexes and routing entries without anyone acknowledging the risk, on every Vitess-family engine.

What it does

Persists the gate-facing VSchema change-metadata into stored plans and teaches the stored-plan unsafe gates — at queueing and at dispatch admission — to read it.

• Both plan persistence sites — the gRPC path and the local data-plane path — persist the same subset through one storage helper (VSchemaPlanMetadata): the vschema_changed flag plus the recorded vschema_deletions and vschema_mutations. Display-only metadata such as the rendered diff is deliberately not persisted.
• The stored plan gains UnsafeVSchemaChanges() alongside UnsafeDDLChanges(), and the queueing gate now rejects on either, with the rejection naming the namespace and the operator-facing reason for each removal or mutation.
• Dispatch admission on the data plane runs a VSchema counterpart of its DDL gate, so a dispatched VSchema-only operation — whose scope carries no table DDL — still requires the opt-in. The dispatch's VSchema changes carry the namespace's persisted change-metadata on the wire (new engine-agnostic metadata field on the TableChange proto), so a deployment that materializes the plan from the dispatch request runs the same gate as one reading its own stored plan.

Two safety properties worth calling out:

• Fail closed on ambiguity. A stored plan that changes its VSchema but carries no persisted change-metadata has no deletion or mutation record to consult, a record that cannot be decoded may hide one, and metadata without a VSchema document is a divergent record — all are rejected with guidance to re-plan rather than assumed additive. Each record key fails closed independently, so one corrupt key never hides the other's disclosures.
• Sibling shards cannot clear the record. A sharded keyspace's per-shard changes share one namespace record; the persisted and wire metadata merge key by key with first write winning, so a sibling shard's change never wipes what another already contributed.

stored-plan apply without allow_unsafe (queueing and dispatch admission)
  ├─ deletions/mutations recorded → rejected; reason names the removed or mutated vindex/routing entry
  ├─ metadata missing/corrupt     → rejected; re-plan to record it (fail closed)
  ├─ metadata without a document  → rejected; divergent record (fail closed)
  └─ changed flag only            → additive VSchema change, queues normally

How it moves us toward the northstar

The northstar is that a schema change is equally safe no matter which surface executes it: every destructive action requires an explicit, auditable operator opt-in whether the apply comes from a live plan or a stored one. This closes the remaining path where a VSchema removal or mutation could apply unacknowledged, making the stored plan a faithful carrier of the plan-time safety verdict instead of a lossy copy.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR closes a safety gap for Vitess-family engines by persisting gate-relevant VSchema change metadata (the vschema_changed flag plus recorded vschema_deletions) into stored plans, and extending the stored-plan apply gate to block applies on unsafe VSchema removals unless the operator explicitly opts in.

Changes:

• Persist VSchema gate metadata into NamespacePlanData.Metadata via a shared helper (storage.VSchemaPlanMetadata) from both local-plan and gRPC plan persistence paths.
• Add stored-plan inspection for unsafe VSchema changes (Plan.UnsafeVSchemaChanges) and extend the stored-plan gate to reject on unsafe DDL or unsafe VSchema changes when allow_unsafe is not set.
• Add targeted unit/integration-style tests covering persistence and gate behavior, including fail-closed behavior on missing/corrupt VSchema metadata.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/tern/local_client.go	Refactors namespace plan-data construction and persists VSchema plan metadata during local plan storage.
pkg/tern/local_client_plan_namespaces_test.go	Tests that local plan persistence stores VSchema metadata and doesn’t let sibling shard changes wipe it.
pkg/storage/types.go	Extends stored namespace plan data with a persisted Metadata map for gate-facing plan metadata.
pkg/storage/plan_vschema.go	Introduces persisted VSchema metadata keys, extraction helper, and stored-plan unsafe VSchema detection.
pkg/storage/plan_vschema_test.go	Unit tests for metadata extraction and stored-plan unsafe VSchema detection (including fail-closed cases).
pkg/api/vschema_stored_plan_gate_test.go	Verifies ExecuteApply blocks/permits stored-plan applies based on persisted VSchema deletions and opt-in.
pkg/api/proto_helpers.go	Persists VSchema metadata in the gRPC plan persistence path using the shared helper.
pkg/api/proto_helpers_test.go	Tests that proto→storage namespace conversion persists gate metadata and omits it for DDL-only changes.
pkg/api/plan_handlers.go	Extends stored-plan unsafe gate to reject on unsafe VSchema changes (in addition to unsafe DDL).
pkg/api/handlers_test.go	Updates stored-plan fixtures to include vschema_changed metadata where VSchema artifacts exist.

---

💡 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/1084, 0bb53ef.

Verdict: 4 findings — 2 blocking (missing apply-time VSchema gate, fail-open on artifact/metadata divergence), 2 non-blocking (test coverage gaps).

Blocking

• pkg/tern/local_client.go#L2304 (also L2468): The apply-time safety re-check rejectUnsafeDDLChangesWithoutOptIn only inspects scope.ddlChanges (table DDL) and was never given a VSchema counterpart, so plan.UnsafeVSchemaChanges() is enforced solely at the pkg/api queueing checkpoint and never re-checked where changes are actually applied. On a dispatched group_finalizer (VSchema-only) operation, deriveDispatchScope sets scope.ddlChanges = nil, so the gate trivially passes and an unsafe VSchema deletion/vindex mutation can apply on that shard without allow_unsafe=true.
• pkg/storage/plan_vschema.go#L81: UnsafeVSchemaChanges() only walks VSchemaNamespaces(), which is gated on Artifacts[vschema.json] != "" (types.go#L393), not on the new Metadata field this PR introduces. A namespace with Metadata[vschema_changed]=true plus real deletions/mutations but an empty/unset artifact is silently excluded rather than failing closed — contradicting the function's own "fails closed on uncertainty" doc comment, and inconsistent with namespacesFromApplyRequest's hard-error for the identical ambiguity shape. Today's two write paths happen to keep artifact and metadata coupled, but no test proves the gate's behavior when they diverge — the PR's riskiest input shape is unverified.

Non-blocking

• pkg/tern/local_client.go#L1974: namespacesFromApplyRequest (sibling of namespacesFromEngineChanges) never populates NamespacePlanData.Metadata, and TableChange proto has no metadata field to carry it — so a plan materialized from a dispatch request always fails closed with the generic "re-plan" reason instead of the itemized deletions/mutations detail. Compounds the missing-gate finding above; worth fixing alongside it rather than separately.
• pkg/storage/plan_vschema_test.go#L115: No test sets both PlanMetadataVSchemaDeletions and PlanMetadataVSchemaMutations undecodable simultaneously; works today since the two checks are independent unconditional appends, but a future refactor adding an early return between them would silently hide one key's corruption with no test to catch it.

General suggestions

• pkg/tern/local_client_plan_namespaces_test.go#L67: The sibling-shard merge tests only ever have one shard already carrying the full expected result (a superset), so a regression that short-circuits the per-key merge loop on the first shard with any metadata — dropping a key that only a different shard contributes (e.g. shard A → deletions only, shard B → mutations only) — would pass unnoticed. Add a case with two shards each contributing a disjoint new key.

The one thing that could have broken, verified

The riskiest mechanism is the Metadata: sc.Metadata → Metadata: maps.Clone(sc.Metadata) change in planResultToProtoChanges (local_client.go, first-shard-of-a-namespace branch). Before the fix, the first shard's proto Metadata aliased the same map as engine.SchemaChangeResult.Metadata; the very next branch does an in-place protoSC.Metadata[key] = value when merging a second shard, which would have mutated result.Changes[0].Metadata by side effect — corrupting the same underlying map that namespacesFromEngineChanges reads immediately afterward for the storage-persistence path. Confirmed engine.SchemaChangeResult.Metadata is a plain reference-type map and that result.Changes is read again post-mutation in the same request, so this is a real aliasing bug fix, not cosmetic.

Verified correct

• namespacesFromEngineChanges's own merge loop always allocates a fresh map before copying — never aliases sc.Metadata — so it never had the aliasing risk fixed above.
• namespacesWithShardPlans (sqlstore/plans.go) passes nsData.Metadata straight through because it only reshapes already-merged shard data; no caller invokes it on not-yet-merged per-shard sources.
• UnsafeVSchemaChanges()/unsafeVSchemaRecords() independently fail-closed on undecodable deletions vs. mutations (no short-circuit between the two checks), consistent with feat(vitess): treat in-place vindex mutations as unsafe changes #1081's apitypes-layer pattern; duplication across the storage/apitypes layers is acceptable since the two operate on distinct structs.
• The ambiguous-changed-flag short-circuit in unsafeVSchemaRecords correctly reports unsafe (via the generic ambiguous record) rather than under-reporting safe, even when deletions/mutations keys are present but the top-level flag is missing/false.

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

[aparajon]

🤖 All four addressed in c05f24a:

1. Apply-time gate: dispatch admission now runs rejectUnsafeVSchemaChangesWithoutOptIn beside the DDL gate at both the create and attach paths, so a VSchema-only operation (nil ddlChanges) can no longer apply an unsafe VSchema change without opt-in.
2. Divergence fail-closed: UnsafeVSchemaChanges now walks the union of document-bearing and metadata-bearing namespaces; metadata without a VSchema document is rejected as a divergent record, with a test for that shape.
3. Materialized plans: the TableChange proto gained an engine-agnostic metadata map; the remote VSchema dispatch attaches the persisted change-metadata and namespacesFromApplyRequest merges it in, so a materialized plan runs the same gate with the itemized reasons instead of always failing closed generically. (This also keeps fix 1 from rejecting additive multi-deployment dispatches.)
4. Tests: added both-keys-undecodable and disjoint-key sibling merge (deletions-only + mutations-only shards) cases per your suggestions.

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