From c135be801a86fa35a9caf056e9b4127753b19cae Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 30 Oct 2026 11:25:11 +1100 Subject: [PATCH] migrate: list every irreversible operation and test the list is complete Each gate had its own tests but nothing checked the set was complete; the registry's test found RunDesired passing AcceptBlocking and Restart through to planned statements, so desired-state execution now rejects every acknowledgement from the same list. --- docs/schemabot-integration.md | 7 + pkg/migrate/accept_blocking.go | 6 + pkg/migrate/copy_swap_restart.go | 8 + pkg/migrate/desired.go | 21 +-- pkg/migrate/irreversible_registry.go | 183 ++++++++++++++++++++++ pkg/migrate/irreversible_registry_test.go | 145 +++++++++++++++++ pkg/planner/destructive.go | 14 ++ pkg/planner/destructive_test.go | 2 + 8 files changed, 373 insertions(+), 13 deletions(-) create mode 100644 pkg/migrate/irreversible_registry.go create mode 100644 pkg/migrate/irreversible_registry_test.go diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 711cf370..7ee24518 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -639,6 +639,13 @@ and PostgreSQL `lock_timeout`). Contract points for the PostgreSQL adapter: rejects `AllowDestructive` (`ErrAllowDestructiveNotSupported`) and keeps refusing a destructive plan whole, so a declarative drop the operator approves runs as its own imperative statement. +- **`RunDesired` takes no acknowledgement.** Each of `Force`, `AcceptBlocking`, + `AllowDestructive`, `CopySwap.Restart`, and `CopySwap.BlockWrites` accepts a loss for one + statement the operator wrote, and a desired-state run executes statements the plan derived. + Setting any of them returns its own sentinel before any database work: + `ErrForceNotSupported`, `ErrAcceptBlockingNotSupported`, `ErrAllowDestructiveNotSupported`, + `ErrRestartNotSupported`, or `ErrBlockWritesNotSupported`. An adapter that reuses one + `Options` value for both front doors clears them before calling `RunDesired`. - **Plan-time verdicts are advisory; the executor re-resolves.** The stored verdict exists so operators learn about engine limitations at plan time, but it is estimate-based and stale by apply time. SchemaBot's own engines re-evaluate the refusal/policy predicate per statement diff --git a/pkg/migrate/accept_blocking.go b/pkg/migrate/accept_blocking.go index ad1883be..1d078ed7 100644 --- a/pkg/migrate/accept_blocking.go +++ b/pkg/migrate/accept_blocking.go @@ -30,6 +30,12 @@ var ( // where REINDEX TABLE needs a table — so there is no lock to // acknowledge. Nothing has executed. ErrAcceptBlockingWrongRelationKind = errors.New("the accepted statement names a relation of the wrong kind") + // ErrAcceptBlockingNotSupported is returned by [RunDesired] when + // Options.AcceptBlocking is set: the operator accepts one blocking + // statement they wrote, and desired-state execution runs statements the + // plan derived. + ErrAcceptBlockingNotSupported = errors.New( + "the accept-blocking acknowledgement applies to the imperative front door only; desired-state execution runs no blocking statement it was not told to") ) // AcceptedRefusal returns the refusal's in-process proof when the diff --git a/pkg/migrate/copy_swap_restart.go b/pkg/migrate/copy_swap_restart.go index 597b488f..c0bea550 100644 --- a/pkg/migrate/copy_swap_restart.go +++ b/pkg/migrate/copy_swap_restart.go @@ -1,6 +1,7 @@ package migrate import ( + "errors" "fmt" "log/slog" @@ -8,6 +9,13 @@ import ( "github.com/block/pg-sprite/pkg/statement" ) +// ErrRestartNotSupported is returned by [RunDesired] when +// Options.CopySwap.Restart is set: rerunning a desired schema resumes a +// stopped copy-and-swap, and discarding its progress is a decision about +// one statement the operator names on the imperative front door. +var ErrRestartNotSupported = errors.New( + "the restart acknowledgement applies to the imperative front door only; desired-state execution resumes a stopped copy-and-swap") + // checkRestartAck confirms that the restart acknowledgement names the // resolved target table, exactly as the force acknowledgement must: a // typed table name is what stops the flag from being left on a script and diff --git a/pkg/migrate/desired.go b/pkg/migrate/desired.go index 15e38e39..92904bb7 100644 --- a/pkg/migrate/desired.go +++ b/pkg/migrate/desired.go @@ -183,11 +183,12 @@ func (r DesiredResult) refused(ref verdict.Refusal, detail string) DesiredResult // the operational error. An error with a zero result means the pipeline // stopped before planning or executing anything. // -// Options.Force, Options.AllowDestructive and Options.CopySwap.BlockWrites -// are rejected: the declarative front door never runs a submitted form -// blind, refuses a destructive plan whole, and never takes a table offline -// for writes. A destructive, force-worthy or offline change belongs on the -// imperative front door where the operator states it explicitly. +// Every typed acknowledgement — Options.Force, AcceptBlocking, +// AllowDestructive, CopySwap.Restart and CopySwap.BlockWrites — is rejected +// with its own sentinel: each accepts a loss for one statement the operator +// wrote, and the declarative front door runs statements the plan derived. A +// destructive, blocking, force-worthy, restarted or offline change belongs +// on the imperative front door where the operator states it explicitly. // // Every planned statement runs back through [Run], so Run's note on the // pool's search_path applies to the pool passed here. @@ -197,14 +198,8 @@ func RunDesired(ctx context.Context, pool *pgxpool.Pool, req DesiredRequest, opt if err := opts.validate(); err != nil { return DesiredResult{}, err } - if opts.Force != "" { - return DesiredResult{}, ErrForceNotSupported - } - if opts.AllowDestructive != "" { - return DesiredResult{}, ErrAllowDestructiveNotSupported - } - if opts.CopySwap.BlockWrites != "" { - return DesiredResult{}, ErrBlockWritesNotSupported + if err := rejectDesiredAcks(opts); err != nil { + return DesiredResult{}, err } report, err := diffplan.Plan(ctx, pool, diffplan.Request{Schema: req.Schema, Desired: req.Desired}) if ref, ok := desiredShapeRefusal(err); ok { diff --git a/pkg/migrate/irreversible_registry.go b/pkg/migrate/irreversible_registry.go new file mode 100644 index 00000000..c811042f --- /dev/null +++ b/pkg/migrate/irreversible_registry.go @@ -0,0 +1,183 @@ +package migrate + +import ( + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/verdict" +) + +// This file is the irreversible-operations registry: every operation +// pg-sprite can run that cannot be undone, what it loses, and the typed +// acknowledgement that lifts its refusal on the imperative front door. Each +// acknowledgement names the resolved SCHEMA.TABLE, so one left on a script +// cannot reach another table. Desired-state execution takes none of them: +// it refuses a plan holding a destructive statement whole, and rejects +// every acknowledgement before any database work. The destructive operation +// shapes are not listed here but derived from the planner, the one answer +// both destructive gates use. TestIrreversibleOperationRegistryIsComplete +// fails when an acknowledgement or a destructive shape arrives without an +// entry. + +// ackKind is one typed acknowledgement the imperative front door takes. +// Its value is the migrate command's flag name. +type ackKind string + +const ( + ackForce ackKind = "force" + ackAcceptBlocking ackKind = "accept-blocking" + ackAllowDestructive ackKind = "allow-destructive" + ackRestart ackKind = "restart" + ackBlockWrites ackKind = "block-writes" +) + +// ackKinds is the closed set of acknowledgements, in the order RunDesired +// rejects them. +func ackKinds() []ackKind { + return []ackKind{ackForce, ackAcceptBlocking, ackAllowDestructive, ackRestart, ackBlockWrites} +} + +// value is the acknowledgement as opts carries it; empty means not given. +// The bool is false only for a kind this switch does not name, which the +// registry's completeness test rejects. +func (a ackKind) value(opts Options) (string, bool) { + switch a { + case ackForce: + return opts.Force, true + case ackAcceptBlocking: + return opts.AcceptBlocking, true + case ackAllowDestructive: + return opts.AllowDestructive, true + case ackRestart: + return opts.CopySwap.Restart, true + case ackBlockWrites: + return opts.CopySwap.BlockWrites, true + default: + return "", false + } +} + +// desiredErr is the sentinel RunDesired returns when the acknowledgement +// is given. +func (a ackKind) desiredErr() error { + switch a { + case ackForce: + return ErrForceNotSupported + case ackAcceptBlocking: + return ErrAcceptBlockingNotSupported + case ackAllowDestructive: + return ErrAllowDestructiveNotSupported + case ackRestart: + return ErrRestartNotSupported + case ackBlockWrites: + return ErrBlockWritesNotSupported + default: + return nil + } +} + +// rejectDesiredAcks returns the sentinel for the first acknowledgement opts +// carries, or nil when it carries none. +func rejectDesiredAcks(opts Options) error { + for _, a := range ackKinds() { + if v, _ := a.value(opts); v != "" { + return a.desiredErr() + } + } + return nil +} + +// loss is what an irreversible operation cannot give back. +type loss string + +const ( + // lossStructure: the planner marks the shape destructive — a dropped + // column's values, a constraint's or NOT NULL's guarantee, an index. + lossStructure loss = "data-or-structure" + // lossAvailability: the table's reads or writes stop for the length of + // the operation. + lossAvailability loss = "availability" + // lossProgress: a stopped copy-and-swap's shadow, checkpoint row, and + // write fence or replication slot. + lossProgress loss = "progress" +) + +// desiredHandling is what desired-state execution does about an entry. +type desiredHandling string + +const ( + // desiredRefusesPlan: the planner can derive the operation, and + // admission refuses any plan holding it, whole. + desiredRefusesPlan desiredHandling = "refuses-plan" + // desiredRejectsAck: no plan derives the operation on its own, and the + // acknowledgement that would allow it is rejected. + desiredRejectsAck desiredHandling = "rejects-ack" +) + +// irreversibleOp is one registry entry. +type irreversibleOp struct { + // key names the operation: a destructive shape, a blocking statement + // kind, the submitted form, or a copy-and-swap lifecycle action. + key string + // loss is what the operation cannot give back. + loss loss + // ack lifts the refusal on the imperative front door. + ack ackKind + // refusal is the reason the operation is refused with when ack is + // missing. Empty only for a progress loss: without the acknowledgement + // the run resumes, so nothing is discarded and nothing is refused. + refusal verdict.Reason + // desired is what desired-state execution does about it. + desired desiredHandling +} + +// irreversibleOps is the registry: one entry per destructive operation +// shape, derived from the planner, followed by the operations no shape +// classifies. +func irreversibleOps() []irreversibleOp { + var ops []irreversibleOp + for _, k := range planner.DestructiveOpKinds() { + ops = append(ops, irreversibleOp{ + key: "shape: " + statement.Op{Kind: k}.Describe(), + loss: lossStructure, + ack: ackAllowDestructive, + refusal: verdict.ReasonDestructiveChange, + desired: desiredRefusesPlan, + }) + } + return append(ops, + irreversibleOp{ + key: "blocking DROP INDEX", + loss: lossAvailability, + ack: ackAcceptBlocking, + refusal: verdict.ReasonIndexStatement, + desired: desiredRejectsAck, + }, + irreversibleOp{ + key: "blocking REINDEX", + loss: lossAvailability, + ack: ackAcceptBlocking, + refusal: verdict.ReasonIndexStatement, + desired: desiredRejectsAck, + }, + irreversibleOp{ + key: "submitted form that blocks the table", + loss: lossAvailability, + ack: ackForce, + refusal: verdict.ReasonRewriteRequired, + desired: desiredRejectsAck, + }, + irreversibleOp{ + key: "copy-and-swap restart", + loss: lossProgress, + ack: ackRestart, + desired: desiredRejectsAck, + }, + irreversibleOp{ + key: "copy-and-swap with the table's writes blocked", + loss: lossAvailability, + ack: ackBlockWrites, + refusal: verdict.ReasonCopyAndSwapRefused, + desired: desiredRejectsAck, + }, + ) +} diff --git a/pkg/migrate/irreversible_registry_test.go b/pkg/migrate/irreversible_registry_test.go new file mode 100644 index 00000000..ce8ce121 --- /dev/null +++ b/pkg/migrate/irreversible_registry_test.go @@ -0,0 +1,145 @@ +package migrate + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/plan" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/statement" + "github.com/block/pg-sprite/pkg/verdict" +) + +// withAck returns opts carrying value as acknowledgement a. It is written +// independently of ackKind.value, so a value that reads the wrong Options +// field fails the round trip below. +func withAck(t *testing.T, opts Options, a ackKind, value string) Options { + t.Helper() + switch a { + case ackForce: + opts.Force = value + case ackAcceptBlocking: + opts.AcceptBlocking = value + case ackAllowDestructive: + opts.AllowDestructive = value + case ackRestart: + opts.CopySwap.Restart = value + case ackBlockWrites: + opts.CopySwap.BlockWrites = value + default: + t.Fatalf("acknowledgement %q has no Options field in this test", a) + } + return opts +} + +// The registry lists every irreversible operation with the acknowledgement +// that lifts its refusal, and desired-state execution takes none of those +// acknowledgements. A new acknowledgement, or a new shape the planner marks +// destructive, fails here until it has an entry and a desired-state +// decision. +func TestIrreversibleOperationRegistryIsComplete(t *testing.T) { + acks := ackKinds() + ops := irreversibleOps() + destructive := planner.DestructiveOpKinds() + + t.Run("the walk is not vacuous", func(t *testing.T) { + require.NotEmpty(t, destructive) + require.Contains(t, destructive, statement.OpDropColumn) + assert.Len(t, acks, 5) + keys := make([]string, 0, len(ops)) + for _, op := range ops { + keys = append(keys, op.key) + } + assert.Contains(t, keys, "copy-and-swap restart") + assert.Len(t, ops, len(destructive)+5) + }) + + t.Run("every acknowledgement reads its own field and has its own sentinel", func(t *testing.T) { + sentinels := map[error]ackKind{} + for _, a := range acks { + got, ok := a.value(withAck(t, DefaultOptions(), a, "app.orders")) + require.True(t, ok, "acknowledgement %q is not classified", a) + assert.Equal(t, "app.orders", got, "acknowledgement %q reads another field", a) + for _, other := range acks { + if other != a { + v, _ := other.value(withAck(t, DefaultOptions(), a, "app.orders")) + assert.Empty(t, v, "acknowledgement %q also reads %q's field", other, a) + } + } + err := a.desiredErr() + require.Error(t, err, "acknowledgement %q has no desired-state sentinel", a) + prev, dup := sentinels[err] + assert.False(t, dup, "acknowledgements %q and %q share a sentinel", prev, a) + sentinels[err] = a + } + }) + + // Each acknowledgement accepts a loss for one statement the operator + // wrote; desired-state execution runs statements the plan derived, so + // it rejects every one before any database work, even when it names + // the right table. + t.Run("desired-state execution rejects every acknowledgement", func(t *testing.T) { + for _, a := range acks { + opts := withAck(t, DefaultOptions(), a, "public.t") + res, err := RunDesired(t.Context(), nil, DesiredRequest{Schema: "public"}, opts) + require.ErrorIs(t, err, a.desiredErr(), "acknowledgement %q", a) + assert.Equal(t, DesiredResult{}, res) + } + }) + + t.Run("every destructive shape has exactly one entry", func(t *testing.T) { + var shapes []string + for _, op := range ops { + if op.desired == desiredRefusesPlan { + shapes = append(shapes, op.key) + } + } + want := make([]string, 0, len(destructive)) + for _, k := range destructive { + want = append(want, "shape: "+statement.Op{Kind: k}.Describe()) + } + assert.Equal(t, want, shapes) + }) + + t.Run("every entry is gated and every acknowledgement gates an entry", func(t *testing.T) { + gated := map[ackKind]bool{} + for _, op := range ops { + assert.Contains(t, acks, op.ack, "entry %q", op.key) + gated[op.ack] = true + assert.Contains(t, []loss{lossStructure, lossAvailability, lossProgress}, op.loss, "entry %q", op.key) + assert.Contains(t, []desiredHandling{desiredRefusesPlan, desiredRejectsAck}, op.desired, "entry %q", op.key) + if op.loss == lossProgress { + assert.Empty(t, op.refusal, "entry %q: without the acknowledgement the run resumes", op.key) + continue + } + assert.Contains(t, verdict.Reasons(), op.refusal, "entry %q", op.key) + } + for _, a := range acks { + assert.True(t, gated[a], "acknowledgement %q gates no entry", a) + } + }) + + // The shapes desired-state execution can derive are refused at plan + // admission, whole, before any statement runs. + t.Run("desired-state admission refuses a destructive plan whole", func(t *testing.T) { + report := plan.Report{ + Schema: "app", + Table: "orders", + Statements: []plan.Statement{ + {SQL: `ALTER TABLE app.orders ADD COLUMN note text`}, + {SQL: `ALTER TABLE app.orders DROP COLUMN legacy_id`, Destructive: true}, + }, + } + res, ok := admitPlan(DesiredRequest{}, report) + require.False(t, ok) + assert.Equal(t, verdict.OutcomeRefused, res.Outcome) + assert.Equal(t, verdict.ReasonDestructiveChange, res.Reason) + for _, op := range ops { + if op.desired == desiredRefusesPlan { + assert.Equal(t, verdict.ReasonDestructiveChange, op.refusal, "entry %q", op.key) + } + } + }) +} diff --git a/pkg/planner/destructive.go b/pkg/planner/destructive.go index 802b847d..82f4c027 100644 --- a/pkg/planner/destructive.go +++ b/pkg/planner/destructive.go @@ -21,6 +21,20 @@ func destructiveOp(kind statement.OpKind) bool { return destructive } +// DestructiveOpKinds returns every operation shape the planner marks +// destructive, in statement.OpKinds() order. The front door's +// irreversible-operations registry derives its operation entries from it, +// so this classification stays the only list of destructive shapes. +func DestructiveOpKinds() []statement.OpKind { + var kinds []statement.OpKind + for _, k := range statement.OpKinds() { + if destructiveOp(k) { + kinds = append(kinds, k) + } + } + return kinds +} + // classifyDestructive is the explicit decision for every shape in // statement.OpKinds(). classified is false only for a shape this switch // does not name, which TestEveryOpKindIsClassifiedForDestructiveness diff --git a/pkg/planner/destructive_test.go b/pkg/planner/destructive_test.go index 05d7a313..b75db924 100644 --- a/pkg/planner/destructive_test.go +++ b/pkg/planner/destructive_test.go @@ -32,6 +32,8 @@ func TestEveryOpKindIsClassifiedForDestructiveness(t *testing.T) { statement.OpDropIndex, statement.OpDropNotNull, }, destructive) + assert.Equal(t, destructive, DestructiveOpKinds(), + "the exported set is the classification, in statement.OpKinds() order") } // A shape outside the classified set is treated as destructive, so the