From 5860fe1fff3efb72486d4342b6436e61357d4c8c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 02:00:21 +0100 Subject: [PATCH 01/15] feat(db): record operator settlements on purchase executions (#999) Migration 000143 adds outcome_settled_at, outcome_settled_by and outcome_settlement to purchase_executions, with a CHECK that they are set together. They hold an operator's audited settlement of a failed execution whose outcome is unknown. Settling never changes status or the ramp ledger's view of the row. Co-Authored-By: Claude Opus 5.5 --- ...ase_executions_outcome_settlement.down.sql | 8 +++ ...chase_executions_outcome_settlement.up.sql | 21 ++++++ ...hase_executions_outcome_settlement_test.go | 68 +++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql create mode 100644 internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql create mode 100644 internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go diff --git a/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql new file mode 100644 index 000000000..46b11cc0e --- /dev/null +++ b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.down.sql @@ -0,0 +1,8 @@ +-- Reverse 000143 (issue #999). Lossy: operator settlements are dropped, so a +-- settled failed row blocks its managed plan's delete again. +ALTER TABLE purchase_executions + DROP CONSTRAINT IF EXISTS purchase_executions_outcome_settlement_complete; +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS outcome_settlement, + DROP COLUMN IF EXISTS outcome_settled_by, + DROP COLUMN IF EXISTS outcome_settled_at; diff --git a/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql new file mode 100644 index 000000000..55894f5d4 --- /dev/null +++ b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement.up.sql @@ -0,0 +1,21 @@ +-- Migration 000143 (issue #999): an operator's audited settlement of a failed +-- execution whose outcome is unknown (failed without no_submission proof). +-- +-- Settling never changes status and never makes the row count as unbought: +-- the ramp ledger (holdsCapacity, ramp_ledger_holds) and the retention trigger +-- still treat it as possibly bought. It only lets a managed plan's delete +-- detach the row, and it stops the row from being retried (a retry of a row +-- detached from its plan would buy with no plan). +-- +-- outcome_settled_by is the acting principal as text (a users.id, or the admin +-- API key sentinel), so the audit survives the user being deleted. +-- outcome_settlement is the complete, fixed-format record the store wrote. +ALTER TABLE purchase_executions + ADD COLUMN IF NOT EXISTS outcome_settled_at TIMESTAMPTZ NULL, + ADD COLUMN IF NOT EXISTS outcome_settled_by TEXT NULL, + ADD COLUMN IF NOT EXISTS outcome_settlement TEXT NULL; + +ALTER TABLE purchase_executions + ADD CONSTRAINT purchase_executions_outcome_settlement_complete + CHECK ((outcome_settled_at IS NULL) = (outcome_settled_by IS NULL) + AND (outcome_settled_at IS NULL) = (outcome_settlement IS NULL)); diff --git a/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go new file mode 100644 index 000000000..5d78393a9 --- /dev/null +++ b/internal/database/postgres/migrations/000143_purchase_executions_outcome_settlement_test.go @@ -0,0 +1,68 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000143 up, down and +// the all-or-nothing CHECK (#999). +func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + const version = 143 + prev := previousMigrationVersion(t, version) + + columns := func() int { + var n int + require.NoError(t, pool.QueryRow(ctx, ` + SELECT count(*) FROM information_schema.columns + WHERE table_name = 'purchase_executions' + AND column_name IN ('outcome_settled_at', 'outcome_settled_by', 'outcome_settlement')`).Scan(&n)) + return n + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + require.Zero(t, columns(), "before 000143 the columns must not exist") + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations) + VALUES ('00000000-0000-0000-0000-0000000009a1', 'failed', 1, now(), '[]')`) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + require.Equal(t, 3, columns()) + var settled bool + require.NoError(t, pool.QueryRow(ctx, `SELECT outcome_settled_at IS NOT NULL FROM purchase_executions + WHERE execution_id = '00000000-0000-0000-0000-0000000009a1'`).Scan(&settled)) + require.False(t, settled, "existing rows start unsettled") + + _, err = pool.Exec(ctx, `UPDATE purchase_executions + SET outcome_settled_at = now(), outcome_settled_by = 'admin-api-key', outcome_settlement = 'operator:settled' + WHERE execution_id = '00000000-0000-0000-0000-0000000009a1'`) + require.NoError(t, err, "a complete settlement is valid") + for name, set := range map[string]string{ + "time only": `outcome_settled_at = now()`, + "no record": `outcome_settled_at = now(), outcome_settled_by = 'u'`, + "no actor": `outcome_settled_at = now(), outcome_settlement = 's'`, + "record only": `outcome_settlement = 's'`, + } { + _, err = pool.Exec(ctx, `INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations) + VALUES (gen_random_uuid(), 'failed', 1, now(), '[]')`) + require.NoError(t, err) + _, err = pool.Exec(ctx, `UPDATE purchase_executions SET `+set+` WHERE outcome_settled_at IS NULL AND outcome_settlement IS NULL`) + require.Error(t, err, "CHECK must reject a partial settlement (%s)", name) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + require.Zero(t, columns(), "down must drop the columns") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + require.Equal(t, 3, columns(), "up must re-apply cleanly after down") +} From d75cf1faf7ee5d2942fdbece55d14fe4c49eb80a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 02:00:21 +0100 Subject: [PATCH 02/15] feat(plans): settle a failed-with-submission execution so a managed plan can be deleted (#999) SettleFailedExecution records an operator's acknowledged, reasoned settlement of a failed plan execution with no no_submission proof. It locks the plan FOR SHARE, then the row and its retry-chain tail FOR UPDATE, and refuses while the chain may still buy: a tail outside the terminal allowlist or a failed tail that can still be retried. Replaying the same request returns the stored settlement. The row stays failed and keeps counting as possibly bought in the ramp ledger. requireManagedDeleteTx skips a settled failed row, and LinkRetryExecutionAtomic refuses one, since a retry of a row detached from its deleted plan would buy with no plan. Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 4 +- .../config/store_postgres_execution_settle.go | 233 ++++++++++++++++++ .../store_postgres_execution_settle_test.go | 40 +++ .../config/store_postgres_managed_guard.go | 24 +- 4 files changed, 296 insertions(+), 5 deletions(-) create mode 100644 internal/config/store_postgres_execution_settle.go create mode 100644 internal/config/store_postgres_execution_settle_test.go diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c90c71ab5..1f0f8b6b6 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1193,7 +1193,8 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, // // Returns (true, nil) when this call won the race and wrote the linkage, // (false, nil) when zero rows were affected because a concurrent retry -// already claimed the row (or it is no longer 'failed'), and (false, err) +// already claimed the row (or it is no longer 'failed', or an operator +// settled it: SettleFailedExecution, issue #999), and (false, err) // on a real DB error. Must be called inside the same tx that inserts the // successor row, after that insert, so the FK on retry_execution_id is // satisfied. @@ -1205,6 +1206,7 @@ func (s *PostgresStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, WHERE execution_id = $1 AND status = 'failed' AND retry_execution_id IS NULL + AND outcome_settled_at IS NULL ` tag, err := tx.Exec(ctx, q, executionID, retryExecutionID) if err != nil { diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go new file mode 100644 index 000000000..e22a907c3 --- /dev/null +++ b/internal/config/store_postgres_execution_settle.go @@ -0,0 +1,233 @@ +package config + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/jackc/pgx/v5" +) + +// Outcomes of an operator settle that the store distinguishes. The handler +// maps them to 404, 400 and 409. +var ( + ErrExecutionSettleNotFound = errors.New("execution not found") + ErrExecutionSettleInvalid = errors.New("invalid execution settle request") + ErrExecutionSettleConflict = errors.New("execution cannot be settled") +) + +// executionSettleReasonMax bounds the stored record; the column has no CHECK. +const executionSettleReasonMax = 1000 + +// ExecutionSettleRequest asks to settle a failed plan execution whose outcome +// is unknown. Actor is the acting principal's id. Acknowledge must be true: +// the operator states they checked the cloud account and accept that the row +// will never be retried. +type ExecutionSettleRequest struct { + ExecutionID string + Actor string + Reason string + Acknowledge bool +} + +// ExecutionSettlement is the stored settlement. Replayed is true when an +// identical earlier request had already settled the row and nothing was +// written. +type ExecutionSettlement struct { + ExecutionID string + Status string + SettledAt time.Time + SettledBy string + Settlement string + Replayed bool +} + +// settleTarget is the locked row SettleFailedExecution judges. +type settleTarget struct { + status string + noSubmission bool + retryID *string + planID *string + settledAt *time.Time + settledBy *string + settlement *string +} + +// SettleFailedExecution records an operator's audited settlement of a failed +// plan execution that has no no_submission proof, so it stops blocking the +// delete of its editor-managed plan (requireManagedDeleteTx). It never changes +// the status and never makes the row count as unbought: the ramp ledger and +// the retention trigger keep treating it as possibly bought. A settled row can +// no longer be retried (LinkRetryExecutionAtomic), because a retry of a row +// whose plan was deleted would buy with no plan. +// +// Locks, in the order the editor command and DeletePurchasePlan use: the plan +// row FOR SHARE, the execution FOR UPDATE, then its retry-chain tail FOR +// UPDATE. It refuses while the chain may still buy (a tail that is not +// terminal, a failed tail that can still be retried, or a chain longer than +// LadderChainMaxDepth). +func (s *PostgresStore) SettleFailedExecution(ctx context.Context, req ExecutionSettleRequest) (*ExecutionSettlement, error) { + if err := validateSettleRequest(req); err != nil { + return nil, err + } + var res *ExecutionSettlement + err := s.WithTx(ctx, func(tx pgx.Tx) error { + target, err := lockSettleTarget(ctx, tx, req.ExecutionID) + if err != nil { + return err + } + if target.settledAt != nil { + res, err = replaySettlement(req, target) + return err + } + if checkErr := checkSettleTarget(target); checkErr != nil { + return checkErr + } + tail, err := lockSettleChainTail(ctx, tx, req.ExecutionID, target.retryID) + if err != nil { + return err + } + record := settlementRecord(req, tail) + res = &ExecutionSettlement{ExecutionID: req.ExecutionID, Status: target.status, SettledBy: req.Actor, Settlement: record} + return tx.QueryRow(ctx, `UPDATE purchase_executions + SET outcome_settled_at = NOW(), outcome_settled_by = $2, outcome_settlement = $3 + WHERE execution_id = $1 AND outcome_settled_at IS NULL + RETURNING outcome_settled_at`, req.ExecutionID, req.Actor, record).Scan(&res.SettledAt) + }) + if err != nil { + return nil, err + } + return res, nil +} + +func validateSettleRequest(req ExecutionSettleRequest) error { + switch { + case !req.Acknowledge: + return fmt.Errorf("%w: the operator must acknowledge the unknown outcome", ErrExecutionSettleInvalid) + case req.Actor == "": + return fmt.Errorf("%w: settling needs an actor", ErrExecutionSettleInvalid) + case req.Reason == "" || len(req.Reason) > executionSettleReasonMax: + return fmt.Errorf("%w: reason must be 1 to %d bytes", ErrExecutionSettleInvalid, executionSettleReasonMax) + } + return nil +} + +// lockSettleTarget takes the plan lock, then the execution lock, and re-reads +// the row under both. A row whose plan changed in between (deleted, so +// plan_id went NULL) is refused rather than judged against the wrong plan. +func lockSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*settleTarget, error) { + var planID *string + err := tx.QueryRow(ctx, `SELECT plan_id::text FROM purchase_executions WHERE execution_id = $1`, executionID).Scan(&planID) + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrExecutionSettleNotFound + } + if err != nil { + return nil, fmt.Errorf("read execution %s: %w", executionID, err) + } + if planID == nil { + return nil, fmt.Errorf("%w: only an execution of a purchase plan can be settled", ErrExecutionSettleConflict) + } + if _, lockErr := tx.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR SHARE`, *planID); lockErr != nil { + return nil, fmt.Errorf("lock plan %s: %w", *planID, lockErr) + } + var t settleTarget + err = tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, plan_id::text, + outcome_settled_at, outcome_settled_by, outcome_settlement + FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, executionID).Scan( + &t.status, &t.noSubmission, &t.retryID, &t.planID, &t.settledAt, &t.settledBy, &t.settlement) + if err != nil { + return nil, fmt.Errorf("lock execution %s: %w", executionID, err) + } + if t.planID == nil || *t.planID != *planID { + return nil, fmt.Errorf("%w: the execution's plan changed while settling", ErrExecutionSettleConflict) + } + return &t, nil +} + +// checkSettleTarget allows only the rows that block a managed delete for an +// unknown outcome: failed without a no_submission proof. +func checkSettleTarget(t *settleTarget) error { + if t.status != StatusFailed { + return fmt.Errorf("%w: execution is %s; only a failed execution can be settled", ErrExecutionSettleConflict, t.status) + } + if t.noSubmission { + return fmt.Errorf("%w: execution is proven never submitted and needs no settlement", ErrExecutionSettleConflict) + } + return nil +} + +// settleChainTail is the last execution of the retry chain from the target. +type settleChainTail struct { + id string + status string +} + +// lockSettleChainTail follows retry_execution_id from the target to the tail +// and locks the tail. It refuses while the chain may still buy: a tail outside +// the terminal allowlist, a failed tail that can still be retried (no +// no_submission proof and not settled itself), or a chain cut short by the +// depth cap or changed between the walk and the lock. +func lockSettleChainTail(ctx context.Context, tx pgx.Tx, executionID string, next *string) (settleChainTail, error) { + tail, err := walkSettleChain(ctx, tx, executionID, next) + if err != nil || tail.id == executionID { + return tail, err + } + var retryID *string + var noSubmission, settled bool + err = tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, outcome_settled_at IS NOT NULL + FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, tail.id).Scan(&tail.status, &noSubmission, &retryID, &settled) + if err != nil { + return tail, fmt.Errorf("lock execution %s: %w", tail.id, err) + } + if retryID != nil { + return tail, fmt.Errorf("%w: the retry chain changed while settling", ErrExecutionSettleConflict) + } + retryable := tail.status == StatusFailed && !noSubmission && !settled + if !ladderResolvableNodeStatus(tail.status) || retryable { + return tail, fmt.Errorf("%w: retry %s is %s and may still buy or be retried; let it finish or settle it first", ErrExecutionSettleConflict, tail.id, tail.status) + } + return tail, nil +} + +// walkSettleChain reads the retry chain from the target, unlocked, and returns +// its last node; the target itself when it was never retried. +func walkSettleChain(ctx context.Context, tx pgx.Tx, executionID string, next *string) (settleChainTail, error) { + tail := settleChainTail{id: executionID, status: StatusFailed} + for depth := 0; next != nil; depth++ { + if depth >= LadderChainMaxDepth { + return tail, fmt.Errorf("%w: retry chain is longer than %d executions", ErrExecutionSettleConflict, LadderChainMaxDepth) + } + node, err := readChainNode(ctx, tx, *next, false) + if err != nil { + return tail, err + } + tail, next = settleChainTail{id: node.ExecutionID, status: node.Status}, node.RetryExecutionID + } + return tail, nil +} + +// settlementRecord is the exact string stored in outcome_settlement. Fields are +// fixed and quoted, so a repeat of the same request compares equal and a +// different one cannot. +func settlementRecord(req ExecutionSettleRequest, tail settleChainTail) string { + return fmt.Sprintf("operator:settled_unknown_outcome: ack=true chain_tail=%s tail_status=%s reason=%q", + tail.id, tail.status, req.Reason) +} + +// replaySettlement answers a request for a row that is already settled: the +// stored settlement when this actor sent the same reason, else a conflict. +func replaySettlement(req ExecutionSettleRequest, t *settleTarget) (*ExecutionSettlement, error) { + if t.settledBy == nil || t.settlement == nil { + return nil, fmt.Errorf("execution %s has an incomplete settlement", req.ExecutionID) + } + if *t.settledBy != req.Actor || !settlementHasReason(*t.settlement, req.Reason) { + return nil, fmt.Errorf("%w: already settled by %s at %s", ErrExecutionSettleConflict, *t.settledBy, t.settledAt.UTC().Format(time.RFC3339)) + } + return &ExecutionSettlement{ExecutionID: req.ExecutionID, Status: t.status, SettledAt: *t.settledAt, SettledBy: *t.settledBy, Settlement: *t.settlement, Replayed: true}, nil +} + +func settlementHasReason(record, reason string) bool { + suffix := fmt.Sprintf(" reason=%q", reason) + return len(record) >= len(suffix) && record[len(record)-len(suffix):] == suffix +} diff --git a/internal/config/store_postgres_execution_settle_test.go b/internal/config/store_postgres_execution_settle_test.go new file mode 100644 index 000000000..9018e7fc6 --- /dev/null +++ b/internal/config/store_postgres_execution_settle_test.go @@ -0,0 +1,40 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestBlocksManagedDelete(t *testing.T) { + cases := []struct { + status string + noSubmission bool + settled bool + blocks bool + }{ + {StatusFailed, false, false, true}, + {StatusFailed, false, true, false}, + {StatusFailed, true, false, false}, + {"pending", false, true, true}, + {"running", false, true, true}, + {StatusCompleted, false, false, false}, + {StatusPartiallyCompleted, false, false, false}, + {StatusCanceled, false, false, false}, + {StatusExpired, false, false, false}, + {"some_future_status", false, true, true}, + } + for _, tc := range cases { + e := &PurchaseExecution{Status: tc.status, NoSubmission: tc.noSubmission} + require.Equal(t, tc.blocks, blocksManagedDelete(e, tc.settled), "%+v", tc) + } +} + +func TestSettlementRecordReplayMatchesOnlyTheSameReason(t *testing.T) { + req := ExecutionSettleRequest{Reason: `checked "eu" account`} + record := settlementRecord(req, settleChainTail{id: "tail", status: StatusCompleted}) + require.Equal(t, `operator:settled_unknown_outcome: ack=true chain_tail=tail tail_status=completed reason="checked \"eu\" account"`, record) + require.True(t, settlementHasReason(record, req.Reason)) + require.False(t, settlementHasReason(record, "account")) + require.False(t, settlementHasReason(record, `checked "eu" account2`)) +} diff --git a/internal/config/store_postgres_managed_guard.go b/internal/config/store_postgres_managed_guard.go index fc4b0064b..bcb734307 100644 --- a/internal/config/store_postgres_managed_guard.go +++ b/internal/config/store_postgres_managed_guard.go @@ -123,21 +123,37 @@ func sameAccountSet(a, b []string) bool { // hold or buy capacity: the FK would detach it (ON DELETE SET NULL) and it // would then buy with no plan. Judged under the caller's plan lock, so a root // inserted by a command that committed first is seen. Settled history -// (completed, canceled, expired, failed without submission) does not block. +// (completed, canceled, expired, failed without submission) does not block, +// and neither does a failed row with an unknown outcome that an operator +// settled (SettleFailedExecution): it can no longer be retried and keeps its +// failed status, so detaching it hides nothing. func requireManagedDeleteTx(ctx context.Context, tx pgx.Tx, planID string) error { - rows, err := tx.Query(ctx, `SELECT status, no_submission FROM purchase_executions WHERE plan_id = $1`, planID) + rows, err := tx.Query(ctx, `SELECT status, no_submission, outcome_settled_at IS NOT NULL FROM purchase_executions WHERE plan_id = $1`, planID) if err != nil { return fmt.Errorf("failed to read plan executions: %w", err) } defer rows.Close() for rows.Next() { var e PurchaseExecution - if err := rows.Scan(&e.Status, &e.NoSubmission); err != nil { + var settled bool + if err := rows.Scan(&e.Status, &e.NoSubmission, &settled); err != nil { return fmt.Errorf("failed to scan plan execution: %w", err) } - if holdsCapacity(&e) && e.Status != StatusCompleted && e.Status != StatusPartiallyCompleted { + if blocksManagedDelete(&e, settled) { return fmt.Errorf("%w: plan %s", ErrManagedPlanLiveExecutions, planID) } } return rows.Err() } + +// blocksManagedDelete reports whether a row of a managed plan may still hold +// or buy capacity that no operator has accounted for. +func blocksManagedDelete(e *PurchaseExecution, settled bool) bool { + switch { + case !holdsCapacity(e), e.Status == StatusCompleted, e.Status == StatusPartiallyCompleted: + return false + case e.Status == StatusFailed: + return !settled + } + return true +} From 981a04f4c0a78a91f9cc02b9af4e71e90164d379 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 02:00:21 +0100 Subject: [PATCH 03/15] test(plans): prove settle, delete and retry on real Postgres, including races (#999) Covers the issue scenario (a failed row whose retry completed blocks the managed delete until settled; the ledger is unchanged), every refusal, chains of unknown outcomes, replay, the retry CAS, and the three orderings that matter: a retry that links first, a delete that holds the plan first, and a delete that queues behind a settle. Co-Authored-By: Claude Opus 5.5 --- ...tgres_execution_settle_integration_test.go | 217 ++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 internal/config/store_postgres_execution_settle_integration_test.go diff --git a/internal/config/store_postgres_execution_settle_integration_test.go b/internal/config/store_postgres_execution_settle_integration_test.go new file mode 100644 index 000000000..67be9d2ad --- /dev/null +++ b/internal/config/store_postgres_execution_settle_integration_test.go @@ -0,0 +1,217 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/require" +) + +const settleActor = "00000000-0000-0000-0000-0000000000a9" + +// managedSettleFixture is a managed 4-step ramp plan with one account. +func managedSettleFixture(ctx context.Context, t *testing.T) *rampFixture { + t.Helper() + f := newRampFixture(ctx, t, 1, 1, 4) + _, err := f.store.db.Exec(ctx, `UPDATE purchase_plans SET ramp_schedule = ramp_schedule || '{"dates_managed":true}' WHERE id = $1`, f.planID) + require.NoError(t, err) + return f +} + +func (f *rampFixture) settle(ctx context.Context, execID, reason string) (*ExecutionSettlement, error) { + return f.store.SettleFailedExecution(ctx, ExecutionSettleRequest{ExecutionID: execID, Actor: settleActor, Reason: reason, Acknowledge: true}) +} + +func (f *rampFixture) settled(ctx context.Context, t *testing.T, execID string) bool { + t.Helper() + var settled bool + require.NoError(t, f.store.db.QueryRow(ctx, `SELECT outcome_settled_at IS NOT NULL FROM purchase_executions WHERE execution_id = $1`, execID).Scan(&settled)) + return settled +} + +// A failed row with an unknown outcome blocks the managed delete until an +// operator settles it; then the delete detaches it without changing it, and the +// ledger still counts it as possibly bought (issue #999). +func TestSettleFailedExecutionLetsManagedDeleteProceed(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + rec := baselineRec(f.accounts[0], "m5.large", 4) + failed := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, false, rec) + retry := f.retryInOneTx(ctx, t, failed, uuid.NewString(), StatusCompleted) + retry.Recommendations = []RecommendationRecord{rec} + require.NoError(t, f.store.SavePurchaseExecution(ctx, retry)) + ledger := f.committed(ctx, t) + + require.ErrorIs(t, f.store.DeletePurchasePlan(ctx, f.planID), ErrManagedPlanLiveExecutions) + + res, err := f.settle(ctx, failed.ExecutionID, "checked the account: retry bought it") + require.NoError(t, err) + require.False(t, res.Replayed) + require.Equal(t, settleActor, res.SettledBy) + require.Contains(t, res.Settlement, "chain_tail="+retry.ExecutionID+" tail_status=completed") + require.Equal(t, ledger, f.committed(ctx, t), "settling must not change what the ledger counts") + + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) + got, err := f.store.GetExecutionByID(ctx, failed.ExecutionID) + require.NoError(t, err) + require.Equal(t, StatusFailed, got.Status, "the settled row keeps its failed status") + require.False(t, got.NoSubmission, "settling is not a no_submission proof") + require.Empty(t, got.PlanID) + require.True(t, f.settled(ctx, t, failed.ExecutionID), "the audit record survives the delete") +} + +func TestSettleFailedExecutionRefusals(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + acct := &f.accounts[0] + conflict := map[string]string{ + "completed": f.row(ctx, t, 1, acct, StatusCompleted).ExecutionID, + "pending": f.row(ctx, t, 1, acct, "pending").ExecutionID, + "canceled": f.row(ctx, t, 1, acct, StatusCanceled).ExecutionID, + "failed no_submission": f.rowWithRecs(ctx, t, 1, acct, StatusFailed, true).ExecutionID, + } + for _, tail := range []string{"pending", "running", StatusFailed} { + head := f.row(ctx, t, 2, acct, StatusFailed) + f.retryInOneTx(ctx, t, head, uuid.NewString(), tail) + conflict["retry tail "+tail] = head.ExecutionID + } + adHoc := &PurchaseExecution{ExecutionID: uuid.NewString(), Status: StatusFailed, StepNumber: 1} + require.NoError(t, f.store.SavePurchaseExecution(ctx, adHoc)) + conflict["no plan"] = adHoc.ExecutionID + + for name, id := range conflict { + _, err := f.settle(ctx, id, "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleConflict, name) + require.False(t, f.settled(ctx, t, id), name) + } + _, err := f.settle(ctx, uuid.NewString(), "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleNotFound) + + failed := f.row(ctx, t, 3, acct, StatusFailed) + for name, req := range map[string]ExecutionSettleRequest{ + "no ack": {ExecutionID: failed.ExecutionID, Actor: settleActor, Reason: "checked"}, + "no actor": {ExecutionID: failed.ExecutionID, Reason: "checked", Acknowledge: true}, + "no reason": {ExecutionID: failed.ExecutionID, Actor: settleActor, Acknowledge: true}, + } { + _, err := f.store.SettleFailedExecution(ctx, req) + require.ErrorIs(t, err, ErrExecutionSettleInvalid, name) + } + require.False(t, f.settled(ctx, t, failed.ExecutionID)) +} + +// A failed tail blocks its predecessor's settle only while it can still be +// retried: once the tail is settled, the head can be too. +func TestSettleFailedExecutionChainOfUnknownOutcomes(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + head := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + tail := f.retryInOneTx(ctx, t, head, uuid.NewString(), StatusFailed) + + _, err := f.settle(ctx, head.ExecutionID, "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleConflict) + _, err = f.settle(ctx, tail.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.ErrorIs(t, f.store.DeletePurchasePlan(ctx, f.planID), ErrManagedPlanLiveExecutions, "the unsettled head still blocks") + _, err = f.settle(ctx, head.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) +} + +func TestSettleFailedExecutionReplayAndRetry(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + first, err := f.settle(ctx, failed.ExecutionID, "operator checked the account") + require.NoError(t, err) + + again, err := f.settle(ctx, failed.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.True(t, again.Replayed) + require.True(t, first.SettledAt.Equal(again.SettledAt), "a replay writes nothing") + _, err = f.settle(ctx, failed.ExecutionID, "a different reason entirely") + require.ErrorIs(t, err, ErrExecutionSettleConflict) + + successor := &PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: f.planID, Status: "pending", StepNumber: 1} + require.NoError(t, f.store.WithTx(ctx, func(tx pgx.Tx) error { + require.NoError(t, f.store.SavePurchaseExecutionTx(ctx, tx, successor)) + linked, linkErr := f.store.LinkRetryExecutionAtomic(ctx, tx, failed.ExecutionID, successor.ExecutionID) + require.NoError(t, linkErr) + require.False(t, linked, "a settled row must not be retried") + return nil + })) +} + +// A retry that links the row first wins: the settle waits on the row lock, +// then sees the live successor and refuses. +func TestSettleWaitsForRetryAndRefusesItsLiveSuccessor(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + retryTx, err := f.store.db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = retryTx.Rollback(ctx) }) + successor := &PurchaseExecution{ExecutionID: uuid.NewString(), PlanID: f.planID, Status: "pending", StepNumber: 1} + require.NoError(t, f.store.SavePurchaseExecutionTx(ctx, retryTx, successor)) + linked, err := f.store.LinkRetryExecutionAtomic(ctx, retryTx, failed.ExecutionID, successor.ExecutionID) + require.NoError(t, err) + require.True(t, linked) + + done := make(chan error, 1) + go func() { + _, settleErr := f.settle(ctx, failed.ExecutionID, "operator checked the account") + done <- settleErr + }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FOR UPDATE%") + require.NoError(t, retryTx.Commit(ctx)) + require.ErrorIs(t, testhelpers.Await(t, done), ErrExecutionSettleConflict) + require.False(t, f.settled(ctx, t, failed.ExecutionID)) +} + +// A delete that holds the plan lock first wins: the settle waits, then finds +// the row detached and refuses instead of settling it against no plan. +func TestSettleWaitsForDeleteAndRefusesDetachedRow(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 1, 4) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + del := testhelpers.HoldCommandFootprint(ctx, t, f.store.db, f.planID) + + done := make(chan error, 1) + go func() { + _, settleErr := f.settle(ctx, failed.ExecutionID, "operator checked the account") + done <- settleErr + }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FOR SHARE%") + _, err := del.Exec(ctx, `DELETE FROM purchase_plans WHERE id = $1`, f.planID) + require.NoError(t, err) + require.NoError(t, del.Commit(ctx)) + require.ErrorIs(t, testhelpers.Await(t, done), ErrExecutionSettleConflict) + require.False(t, f.settled(ctx, t, failed.ExecutionID)) +} + +// A delete that queues behind a settle sees the committed settlement. +func TestDeleteWaitsForSettleAndSeesIt(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + settleTx, err := f.store.db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = settleTx.Rollback(ctx) }) + // The settle's own lock footprint and write, paused before commit. + _, err = settleTx.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR SHARE`, f.planID) + require.NoError(t, err) + _, err = settleTx.Exec(ctx, `UPDATE purchase_executions SET outcome_settled_at = now(), + outcome_settled_by = $2, outcome_settlement = 'operator:settled_unknown_outcome' WHERE execution_id = $1`, failed.ExecutionID, settleActor) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- f.store.DeletePurchasePlan(ctx, f.planID) }() + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FOR UPDATE%") + require.NoError(t, settleTx.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) +} From 73ac5a13aa7c99d2f5b7dcc4cc9561a5af26620b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 02:00:21 +0100 Subject: [PATCH 04/15] feat(api): add POST /api/purchases/{id}/settle for unknown-outcome failures (#999) The endpoint needs execute:purchases, account access to the execution and the grant's constraints against what the execution would have bought (strict scope), a 10-300 character reason and acknowledge_unknown_outcome: true. It logs the settlement with the actor and credential kind. Retry's lost-race 409 now says the row may have been settled. Marks the known issue resolved. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_execution_settle.go | 157 ++++++++++++++++++ ...ndler_execution_settle_integration_test.go | 112 +++++++++++++ internal/api/handler_execution_settle_test.go | 45 +++++ internal/api/handler_purchases.go | 4 +- internal/api/router.go | 8 + known_issues/35_legacy_guard_followups.md | 8 +- 6 files changed, 330 insertions(+), 4 deletions(-) create mode 100644 internal/api/handler_execution_settle.go create mode 100644 internal/api/handler_execution_settle_integration_test.go create mode 100644 internal/api/handler_execution_settle_test.go diff --git a/internal/api/handler_execution_settle.go b/internal/api/handler_execution_settle.go new file mode 100644 index 000000000..0676f40e8 --- /dev/null +++ b/internal/api/handler_execution_settle.go @@ -0,0 +1,157 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "strings" + "time" + "unicode/utf8" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +const ( + executionSettleReasonMin = 10 + executionSettleReasonMax = 300 + executionSettleBodyMax = 2048 +) + +// executionSettleRequest is the body of POST /api/purchases/{id}/settle. +type executionSettleRequest struct { + Reason string `json:"reason"` + Acknowledge bool `json:"acknowledge_unknown_outcome"` +} + +type executionSettleResponse struct { + ExecutionID string `json:"execution_id"` + Status string `json:"status"` + SettledAt string `json:"outcome_settled_at"` + SettledBy string `json:"outcome_settled_by"` + Settlement string `json:"outcome_settlement"` + Replayed bool `json:"replayed"` +} + +// executionSettleStore is the part of the store the endpoint needs. +type executionSettleStore interface { + SettleFailedExecution(ctx context.Context, req config.ExecutionSettleRequest) (*config.ExecutionSettlement, error) +} + +// settleExecution records an operator's audited settlement of a failed plan +// execution whose outcome is unknown (issue #999), so it stops blocking the +// delete of its editor-managed plan. The row stays failed and keeps counting +// as possibly bought; it can no longer be retried. Gated like a purchase: +// execute:purchases, account access to the execution, and the grant's account +// constraints with strict scope. +func (h *Handler) settleExecution(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) { + if err := validateUUID(id); err != nil { + return nil, err + } + session, err := h.requirePermission(ctx, req, auth.ActionExecute, auth.ResourcePurchases) + if err != nil { + return nil, err + } + body, err := decodeExecutionSettle(req.Body) + if err != nil { + return nil, err + } + if authErr := h.authorizeExecutionSettle(ctx, session, id); authErr != nil { + return nil, authErr + } + store, ok := h.config.(executionSettleStore) + if !ok { + return nil, NewClientError(503, "Execution settle storage is unavailable") + } + res, err := store.SettleFailedExecution(ctx, config.ExecutionSettleRequest{ + ExecutionID: id, Actor: session.UserID, Reason: body.Reason, Acknowledge: body.Acknowledge, + }) + if err != nil { + return nil, mapExecutionSettleError(err) + } + if !res.Replayed { + logging.Infof("purchase execution settled by operator: execution=%s actor=%q via=%s acknowledged_unknown_outcome=true settlement=%q", + id, session.UserID, credentialKind(ctx, session), res.Settlement) + } + return executionSettleResponse{ + ExecutionID: res.ExecutionID, Status: res.Status, SettledAt: res.SettledAt.UTC().Format(time.RFC3339), + SettledBy: res.SettledBy, Settlement: res.Settlement, Replayed: res.Replayed, + }, nil +} + +// authorizeExecutionSettle checks account access to the execution, then the +// execute:purchases constraints against what the execution would have bought +// (its recommendations' providers, services, regions and total commitment) +// and its accounts. StrictScope makes a dimension the execution leaves +// unknown, such as the provider of a step with no recommendations, require an +// unconstrained grant. +func (h *Handler) authorizeExecutionSettle(ctx context.Context, session *Session, id string) error { + if err := h.requireExecutionAccess(ctx, session, id); err != nil { + return err + } + execution, err := h.config.GetExecutionByID(ctx, id) + if errors.Is(err, config.ErrNotFound) || (err == nil && execution == nil) { + return errNotFound + } + if err != nil { + return fmt.Errorf("failed to get execution: %w", err) + } + accounts, err := h.executionAccounts(ctx, execution, nil) + if err != nil { + return err + } + ids := make([]string, 0, len(accounts)) + for i := range accounts { + ids = append(ids, accounts[i].ID) + } + sets := purchaseConstraintSets(execution.Recommendations) + if len(sets) == 0 { + sets = []auth.PermissionConstraints{{}} + } + for i := range sets { + sets[i].AccountIDs, sets[i].StrictScope = ids, true + } + return h.requirePermissionConstraints(ctx, session, auth.ActionExecute, auth.ResourcePurchases, sets) +} + +func mapExecutionSettleError(err error) error { + switch { + case errors.Is(err, config.ErrExecutionSettleNotFound): + return errNotFound + case errors.Is(err, config.ErrExecutionSettleInvalid): + return NewClientError(400, err.Error()) + case errors.Is(err, config.ErrExecutionSettleConflict): + return NewClientError(409, err.Error()) + } + return err +} + +func decodeExecutionSettle(raw string) (executionSettleRequest, error) { + var body executionSettleRequest + if len(raw) > executionSettleBodyMax { + return body, NewClientError(400, "Settle body is too large") + } + decoder := json.NewDecoder(strings.NewReader(raw)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&body); err != nil { + return body, NewClientError(400, "Invalid settle body") + } + if err := decoder.Decode(new(any)); err != io.EOF { + return body, NewClientError(400, "Expected one settle body") + } + body.Reason = strings.TrimSpace(body.Reason) + if !body.Acknowledge { + return body, NewClientError(400, "acknowledge_unknown_outcome must be true: this purchase may have been submitted; check the cloud account first") + } + if n := utf8.RuneCountInString(body.Reason); n < executionSettleReasonMin || n > executionSettleReasonMax { + return body, NewClientError(400, fmt.Sprintf("reason must be %d to %d characters", executionSettleReasonMin, executionSettleReasonMax)) + } + if hasControlRune(body.Reason) { + return body, NewClientError(400, "reason must not contain control characters") + } + return body, nil +} diff --git a/internal/api/handler_execution_settle_integration_test.go b/internal/api/handler_execution_settle_integration_test.go new file mode 100644 index 000000000..2e3812592 --- /dev/null +++ b/internal/api/handler_execution_settle_integration_test.go @@ -0,0 +1,112 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const settleBody = `{"reason":"checked the subscription: nothing was bought","acknowledge_unknown_outcome":true}` + +var purchaserPerms = []auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, +} + +func (f *createConcurrencyFixture) settledAt(ctx context.Context, t *testing.T, id string) *time.Time { + t.Helper() + var at *time.Time + require.NoError(t, f.pool.QueryRow(ctx, `SELECT outcome_settled_at FROM purchase_executions WHERE execution_id = $1`, id).Scan(&at)) + return at +} + +// The issue #999 scenario through the HTTP surface: a failed row with an +// unknown outcome makes the managed delete 409 until a purchaser settles it. +func TestSettleUnblocksManagedPlanDeleteOverHTTP(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + setPlanManaged(ctx, t, f, true) + id := f.saveRow(ctx, t, "failed", false, time.Now(), true) + requireLiveExecutionsRefusal(t, f.deletePlan(ctx, f.handler)) + + h := limitedHandler(f, purchaserPerms, nil) + h.auth.(*MockAuthService).On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil) + req := managedGuardRequest(settleBody) + req.Headers["X-CSRF-Token"] = "test-csrf" + req.RequestContext.HTTP.Method = "POST" + req.RequestContext.HTTP.Path = "/api/purchases/" + id + "/settle" + resp, err := h.HandleRequest(ctx, req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode, resp.Body) + var got executionSettleResponse + require.NoError(t, json.Unmarshal([]byte(resp.Body), &got)) + require.Equal(t, id, got.ExecutionID) + require.Equal(t, "failed", got.Status) + require.Equal(t, "00000000-0000-0000-0000-000000000001", got.SettledBy) + require.Contains(t, got.Settlement, `reason="checked the subscription: nothing was bought"`) + require.False(t, got.Replayed) + + require.NoError(t, f.deletePlan(ctx, f.handler)) + row, err := f.store.GetExecutionByID(ctx, id) + require.NoError(t, err) + require.Equal(t, "failed", row.Status) + require.NotNil(t, f.settledAt(ctx, t, id)) +} + +func TestSettleAuthorizationAndValidation(t *testing.T) { + ctx := context.Background() + f := newCreateConcurrencyFixture(ctx, t) + setPlanManaged(ctx, t, f, true) + id := f.saveRow(ctx, t, "failed", false, time.Now(), true) + completed := f.saveRow(ctx, t, "completed", false, time.Now(), true) + var acct string + require.NoError(t, f.pool.QueryRow(ctx, `SELECT account_id::text FROM plan_accounts WHERE plan_id = $1`, f.planID).Scan(&acct)) + constrained := func(c auth.PermissionConstraints) []auth.Permission { + return []auth.Permission{{Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &c}} + } + cases := []struct { + name string + perms []auth.Permission + scoped []string + id string + body string + status int + }{ + {"admin without execute", []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, nil, id, settleBody, http.StatusForbidden}, + {"out of account scope", purchaserPerms, []string{"other-account"}, id, settleBody, http.StatusNotFound}, + {"execute constrained to another account", constrained(auth.PermissionConstraints{AccountIDs: []string{"other-account"}}), nil, id, settleBody, http.StatusForbidden}, + {"execute constrained to another provider", constrained(auth.PermissionConstraints{AccountIDs: []string{acct}, Providers: []string{"aws"}}), nil, id, settleBody, http.StatusForbidden}, + {"no acknowledgement", purchaserPerms, nil, id, `{"reason":"checked the subscription"}`, http.StatusBadRequest}, + {"short reason", purchaserPerms, nil, id, `{"reason":"ok","acknowledge_unknown_outcome":true}`, http.StatusBadRequest}, + {"unknown field", purchaserPerms, nil, id, `{"reason":"checked the subscription","acknowledge_unknown_outcome":true,"force":true}`, http.StatusBadRequest}, + {"completed row", purchaserPerms, nil, completed, settleBody, http.StatusConflict}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := limitedHandler(f, tc.perms, tc.scoped) + _, err := h.settleExecution(ctx, managedGuardRequest(tc.body), tc.id) + var ce *clientError + if errors.As(err, &ce) { + require.Equal(t, tc.status, ce.code, ce.message) + } else { + require.True(t, IsNotFoundError(err) && tc.status == http.StatusNotFound, "got %v", err) + } + require.Nil(t, f.settledAt(ctx, t, tc.id), "a refused settle writes nothing") + }) + } + + t.Run("constrained to the plan's account", func(t *testing.T) { + h := limitedHandler(f, constrained(auth.PermissionConstraints{AccountIDs: []string{acct}}), nil) + _, err := h.settleExecution(ctx, managedGuardRequest(settleBody), id) + require.NoError(t, err) + }) +} diff --git a/internal/api/handler_execution_settle_test.go b/internal/api/handler_execution_settle_test.go new file mode 100644 index 000000000..d4e7e942d --- /dev/null +++ b/internal/api/handler_execution_settle_test.go @@ -0,0 +1,45 @@ +package api + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/require" +) + +func TestDecodeExecutionSettle(t *testing.T) { + ok, err := decodeExecutionSettle(`{"reason":" checked the account ","acknowledge_unknown_outcome":true}`) + require.NoError(t, err) + require.Equal(t, "checked the account", ok.Reason) + + for name, body := range map[string]string{ + "empty": ``, + "no ack": `{"reason":"checked the account"}`, + "ack false": `{"reason":"checked the account","acknowledge_unknown_outcome":false}`, + "short": `{"reason":" short ","acknowledge_unknown_outcome":true}`, + "long": `{"reason":"` + strings.Repeat("x", executionSettleReasonMax+1) + `","acknowledge_unknown_outcome":true}`, + "control": `{"reason":"checked the\naccount","acknowledge_unknown_outcome":true}`, + "unknown field": `{"reason":"checked the account","acknowledge_unknown_outcome":true,"x":1}`, + "two bodies": `{"reason":"checked the account","acknowledge_unknown_outcome":true}{}`, + "too large": strings.Repeat(" ", executionSettleBodyMax+1), + } { + _, err := decodeExecutionSettle(body) + var ce *clientError + require.True(t, errors.As(err, &ce), name) + require.Equal(t, 400, ce.code, name) + } +} + +func TestMapExecutionSettleError(t *testing.T) { + require.True(t, IsNotFoundError(mapExecutionSettleError(config.ErrExecutionSettleNotFound))) + for sentinel, code := range map[error]int{config.ErrExecutionSettleInvalid: 400, config.ErrExecutionSettleConflict: 409} { + var ce *clientError + require.True(t, errors.As(mapExecutionSettleError(fmt.Errorf("%w: detail", sentinel)), &ce)) + require.Equal(t, code, ce.code) + } + other := errors.New("db down") + require.Equal(t, other, mapExecutionSettleError(other), "an internal error is not a client error") +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index af7c61b96..8f5b49084 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -2599,8 +2599,10 @@ func (h *Handler) persistRetrySuccessorTx(ctx context.Context, failedExec, newEx map[string]any{}) } if lostRace { + // Zero rows linked: a concurrent retry won, or an operator settled the + // row (issue #999), which retires it from retry for good. return NewClientErrorWithDetails(409, - fmt.Sprintf("execution %s was already retried; act on its descendant instead", failedExec.ExecutionID), + fmt.Sprintf("execution %s was already retried or settled; reload it", failedExec.ExecutionID), map[string]any{}) } return fmt.Errorf("failed to save retry execution: %w", err) diff --git a/internal/api/router.go b/internal/api/router.go index 9b5ba7b4e..3641ba592 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -186,6 +186,10 @@ func (r *Router) registerRoutes() { // AuthUser: router-level gate; per-handler requirePermission is the // real security boundary. {PathPrefix: "/api/purchases/", PathSuffix: "/revoke", Method: "POST", Handler: r.revokePurchaseHandler, Auth: AuthUser}, + // Settle a failed plan execution whose outcome is unknown (issue #999), + // so it stops blocking the delete of its editor-managed plan. The + // handler enforces execute:purchases, account access and constraints. + {PathPrefix: "/api/purchases/", PathSuffix: "/settle", Method: "POST", Handler: r.settleExecutionHandler, Auth: AuthUser}, // GET /api/purchases/{id}/revoke/calculate: returns the Azure refund quote // (amount + currency) for the two-step quote-then-confirm revoke UX // (issue #290 Finding #4). No state mutation; result used to populate @@ -624,6 +628,10 @@ func (r *Router) cancelPurchaseHandler(ctx context.Context, req *events.LambdaFu return r.h.cancelPurchase(ctx, req, params["id"], token) } +func (r *Router) settleExecutionHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.settleExecution(ctx, req, params["id"]) +} + func (r *Router) retryPurchaseHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.retryPurchase(ctx, req, params["id"]) } diff --git a/known_issues/35_legacy_guard_followups.md b/known_issues/35_legacy_guard_followups.md index 4b800b0ff..a68883f35 100644 --- a/known_issues/35_legacy_guard_followups.md +++ b/known_issues/35_legacy_guard_followups.md @@ -1,6 +1,6 @@ # Known Issues: follow-ups from the legacy-writer guards (#631) -> **Audit status (2026-10-16):** `2 needs triage · 0 resolved` +> **Audit status (2026-10-22):** `1 needs triage · 1 resolved` ## LOW: cleanup lambda deletes by expires_at with no status predicate @@ -14,7 +14,7 @@ **Status:** Needs triage. -## MEDIUM: a failed-with-submission row blocks deleting a managed plan forever +## ~~MEDIUM: a failed-with-submission row blocks deleting a managed plan forever~~ — RESOLVED **Files:** `internal/config/store_postgres_managed_guard.go` (`requireManagedDeleteTx`), `internal/config/store_postgres_ramp_baseline.go` (`holdsCapacity`) @@ -24,4 +24,6 @@ **Issue:** #999 (follow-up of #631) -**Status:** Needs triage. +**Status:** ✔️ Resolved + +**Resolved by:** an explicit, audited operator settle, `POST /api/purchases/{id}/settle` (`SettleFailedExecution`, migration 000143). It needs `execute:purchases`, account access and the grant's constraints, a reason and `acknowledge_unknown_outcome: true`, and refuses while the row's retry chain may still buy. The row stays `failed` and keeps counting as possibly bought in the ramp ledger; once settled it can no longer be retried and no longer blocks `requireManagedDeleteTx`. Delete itself gained no flag. From d783c23d64d1d85c2a9c6a472cfb4be7b05e6722 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 00:19:39 +0000 Subject: [PATCH 05/15] fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= From 3dba21d07442828e298e8052250645a52f10d910 Mon Sep 17 00:00:00 2001 From: Buddy Date: Wed, 28 Oct 2026 00:58:31 +0000 Subject: [PATCH 06/15] docs(api): describe audited purchase execution settlement --- internal/api/openapi.yaml | 83 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index a7782101b..0b96eaf00 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1454,6 +1454,89 @@ paths: '500': $ref: '#/components/responses/InternalError' + /api/purchases/{id}/settle: + parameters: + - $ref: '#/components/parameters/ResourceID' + post: + operationId: settlePurchaseExecution + tags: [Purchases] + summary: Record an operator settlement of an unknown purchase outcome + description: > + Records an audited settlement of a failed plan execution after the + operator checks the cloud account and acknowledges its unknown outcome. + Requires execute permission on purchases, execution account access and + matching permission constraints. The row remains failed and counts as + possibly bought; settlement permanently prevents retry and can unblock + deletion of its managed plan. A retry chain that may still buy is + refused. Repeating an identical settlement returns the stored result. + parameters: + - $ref: '#/components/parameters/CSRFToken' + requestBody: + required: true + content: + application/json: + schema: + type: object + additionalProperties: false + required: [reason, acknowledge_unknown_outcome] + properties: + reason: + type: string + minLength: 10 + maxLength: 300 + description: > + Trimmed explanation without control characters, at most + 1000 UTF-8 bytes. The entire request body is limited to 2048 bytes. + acknowledge_unknown_outcome: + type: boolean + enum: [true] + responses: + '200': + description: Settlement recorded or an identical settlement replayed + content: + application/json: + schema: + type: object + required: [execution_id, status, outcome_settled_at, outcome_settled_by, outcome_settlement, replayed] + properties: + execution_id: + type: string + format: uuid + status: + type: string + enum: [failed] + outcome_settled_at: + type: string + format: date-time + outcome_settled_by: + type: string + outcome_settlement: + type: string + replayed: + type: boolean + '400': + $ref: '#/components/responses/BadRequest' + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + '404': + $ref: '#/components/responses/NotFound' + '409': + description: Execution cannot be settled, or an earlier settlement differs + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '500': + $ref: '#/components/responses/InternalError' + '503': + description: Settlement storage is unavailable + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + /api/purchases/{id}/revoke: parameters: - name: id From 7d83bc1e461b30c75634978bd1a07ab9bfd83997 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 21:18:11 +0100 Subject: [PATCH 07/15] chore(db): renumber the outcome-settlement migration to 000157 000156 collides with #1239 (000154-000156). The fleet migration-numbering authority assigned 000157 to this PR; #1239 merges and migrates first. Pre-commit: cfn-lint skipped (its Python env cannot build on this machine; no CloudFormation file is touched). Refs #999 Co-Authored-By: Claude Opus 5.5 --- ... 000157_purchase_executions_outcome_settlement.down.sql} | 2 +- ...=> 000157_purchase_executions_outcome_settlement.up.sql} | 2 +- ...> 000157_purchase_executions_outcome_settlement_test.go} | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) rename internal/database/postgres/migrations/{000156_purchase_executions_outcome_settlement.down.sql => 000157_purchase_executions_outcome_settlement.down.sql} (84%) rename internal/database/postgres/migrations/{000156_purchase_executions_outcome_settlement.up.sql => 000157_purchase_executions_outcome_settlement.up.sql} (94%) rename internal/database/postgres/migrations/{000156_purchase_executions_outcome_settlement_test.go => 000157_purchase_executions_outcome_settlement_test.go} (96%) diff --git a/internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql similarity index 84% rename from internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement.down.sql rename to internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql index d86ebccf4..7c1fba2c7 100644 --- a/internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement.down.sql +++ b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql @@ -1,4 +1,4 @@ --- Reverse 000156 (issue #999). Lossy: operator settlements are dropped, so a +-- Reverse 000157 (issue #999). Lossy: operator settlements are dropped, so a -- settled failed row blocks its managed plan's delete again. ALTER TABLE purchase_executions DROP CONSTRAINT IF EXISTS purchase_executions_outcome_settlement_complete; diff --git a/internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql similarity index 94% rename from internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement.up.sql rename to internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql index 33cbe86fe..e98412894 100644 --- a/internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement.up.sql +++ b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql @@ -1,4 +1,4 @@ --- Migration 000156 (issue #999): an operator's audited settlement of a failed +-- Migration 000157 (issue #999): an operator's audited settlement of a failed -- execution whose outcome is unknown (failed without no_submission proof). -- -- Settling never changes status and never makes the row count as unbought: diff --git a/internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement_test.go b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go similarity index 96% rename from internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement_test.go rename to internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go index 26d1c7bba..498d50111 100644 --- a/internal/database/postgres/migrations/000156_purchase_executions_outcome_settlement_test.go +++ b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go @@ -11,14 +11,14 @@ import ( "github.com/stretchr/testify/require" ) -// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000156 up, down and +// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000157 up, down and // the all-or-nothing CHECK (#999). func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { ctx := context.Background() container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool, path := container.DB.Pool(), getMigrationsPath() - const version = 156 + const version = 157 prev := previousMigrationVersion(t, version) columns := func() int { @@ -31,7 +31,7 @@ func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { } require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) - require.Zero(t, columns(), "before 000156 the columns must not exist") + require.Zero(t, columns(), "before 000157 the columns must not exist") _, err := pool.Exec(ctx, ` INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations) VALUES ('00000000-0000-0000-0000-0000000009a1', 'failed', 1, now(), '[]')`) From f9fd6c010fe54353027a34b6a776658f6a42a71e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 21:18:26 +0100 Subject: [PATCH 08/15] fix(config): run the settlement code before its migration is applied main deploys on merge while prod migrations are applied separately. The retry link (LinkRetryExecutionAtomic) and the managed-plan delete guard read outcome_settled_at, so before 000157 every purchase retry and every managed-plan delete would fail with "column does not exist". settlementSchemaReady reads schema_migrations; until 000157 is applied both paths use their pre-settlement queries (no row can be settled yet) and settling returns ErrExecutionSettleNotReady (503). The schema read is shared with the Azure pricing check. An integration test rolls back to the migration below 000157 and pins all three paths; without the gate it fails with the missing column. Pre-commit: cfn-lint skipped (its Python env cannot build on this machine; no CloudFormation file is touched). Refs #999 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_execution_settle.go | 2 + internal/api/openapi.yaml | 4 +- internal/config/store_postgres.go | 10 +++- .../config/store_postgres_execution_settle.go | 8 +++ ...tgres_execution_settle_integration_test.go | 47 ++++++++++++++++ .../config/store_postgres_managed_guard.go | 9 ++- .../config/store_postgres_recommendations.go | 10 +--- .../config/store_postgres_schema_version.go | 47 ++++++++++++++++ .../store_postgres_schema_version_test.go | 55 +++++++++++++++++++ 9 files changed, 180 insertions(+), 12 deletions(-) create mode 100644 internal/config/store_postgres_schema_version.go create mode 100644 internal/config/store_postgres_schema_version_test.go diff --git a/internal/api/handler_execution_settle.go b/internal/api/handler_execution_settle.go index 0676f40e8..92667924c 100644 --- a/internal/api/handler_execution_settle.go +++ b/internal/api/handler_execution_settle.go @@ -126,6 +126,8 @@ func mapExecutionSettleError(err error) error { return NewClientError(400, err.Error()) case errors.Is(err, config.ErrExecutionSettleConflict): return NewClientError(409, err.Error()) + case errors.Is(err, config.ErrExecutionSettleNotReady): + return NewClientError(503, err.Error()) } return err } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 98d4d7307..66d46afee 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1558,7 +1558,9 @@ paths: '500': $ref: '#/components/responses/InternalError' '503': - description: Settlement storage is unavailable + description: >- + Settlement is unavailable: its storage is down, or the database + migration that adds the settlement columns is not applied yet. content: application/json: schema: diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 2f66549c8..1193d437d 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1285,6 +1285,13 @@ func (s *PostgresStore) SavePurchaseExecutionTx(ctx context.Context, tx pgx.Tx, // successor row, after that insert, so the FK on retry_execution_id is // satisfied. func (s *PostgresStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID, retryExecutionID string) (linked bool, err error) { + // Before the settlement migration no row can be settled (#999). + notSettled := "" + if ready, readyErr := settlementSchemaReady(ctx, tx); readyErr != nil { + return false, readyErr + } else if ready { + notSettled = "AND outcome_settled_at IS NULL" + } q := ` UPDATE purchase_executions SET retry_execution_id = $2, @@ -1292,8 +1299,7 @@ func (s *PostgresStore) LinkRetryExecutionAtomic(ctx context.Context, tx pgx.Tx, WHERE execution_id = $1 AND status = 'failed' AND retry_execution_id IS NULL - AND outcome_settled_at IS NULL - ` + ` + notSettled tag, err := tx.Exec(ctx, q, executionID, retryExecutionID) if err != nil { return false, fmt.Errorf("failed to link retry execution: %w", err) diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go index e22a907c3..9eb1dc919 100644 --- a/internal/config/store_postgres_execution_settle.go +++ b/internal/config/store_postgres_execution_settle.go @@ -15,6 +15,8 @@ var ( ErrExecutionSettleNotFound = errors.New("execution not found") ErrExecutionSettleInvalid = errors.New("invalid execution settle request") ErrExecutionSettleConflict = errors.New("execution cannot be settled") + // ErrExecutionSettleNotReady: the settlement migration is not applied yet. + ErrExecutionSettleNotReady = errors.New("execution settlement is not available until the database migration is applied") ) // executionSettleReasonMax bounds the stored record; the column has no CHECK. @@ -73,6 +75,12 @@ func (s *PostgresStore) SettleFailedExecution(ctx context.Context, req Execution } var res *ExecutionSettlement err := s.WithTx(ctx, func(tx pgx.Tx) error { + if ready, err := settlementSchemaReady(ctx, tx); err != nil || !ready { + if err != nil { + return err + } + return ErrExecutionSettleNotReady + } target, err := lockSettleTarget(ctx, tx, req.ExecutionID) if err != nil { return err diff --git a/internal/config/store_postgres_execution_settle_integration_test.go b/internal/config/store_postgres_execution_settle_integration_test.go index 67be9d2ad..30522f946 100644 --- a/internal/config/store_postgres_execution_settle_integration_test.go +++ b/internal/config/store_postgres_execution_settle_integration_test.go @@ -5,8 +5,13 @@ package config import ( "context" + "os" + "regexp" + "strconv" "testing" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" "github.com/google/uuid" "github.com/jackc/pgx/v5" @@ -215,3 +220,45 @@ func TestDeleteWaitsForSettleAndSeesIt(t *testing.T) { require.NoError(t, settleTx.Commit(ctx)) require.NoError(t, testhelpers.Await(t, done)) } + +// migrationBelow is the highest migration on disk below version, so a test can +// roll back exactly one migration whatever gaps the numbering has. +func migrationBelow(t *testing.T, version int) uint { + t.Helper() + entries, err := os.ReadDir(getTestMigrationsPath()) + require.NoError(t, err) + re := regexp.MustCompile(`^(\d{6})_.*\.up\.sql$`) + best := 0 + for _, e := range entries { + if m := re.FindStringSubmatch(e.Name()); m != nil { + if n, _ := strconv.Atoi(m[1]); n < version && n > best { + best = n + } + } + } + require.NotZero(t, best) + return uint(best) +} + +// main deploys on merge while prod migrations are applied separately, so this +// code must work on a schema without the settlement columns (#999): delete +// and retry behave as before, and settling is refused instead of erroring. +func TestSettlementPathsBeforeTheMigration(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + rec := baselineRec(f.accounts[0], "m5.large", 4) + failed := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, false, rec) + + conn, ok := f.store.db.(*database.Connection) + require.True(t, ok) + require.NoError(t, migrations.MigrateToVersion(ctx, conn.Pool(), getTestMigrationsPath(), migrationBelow(t, settlementMigrationVersion))) + var cols int + require.NoError(t, conn.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns WHERE table_name = 'purchase_executions' AND column_name = 'outcome_settled_at'`).Scan(&cols)) + require.Zero(t, cols, "precondition: the settlement columns are gone") + + require.ErrorIs(t, f.store.DeletePurchasePlan(ctx, f.planID), ErrManagedPlanLiveExecutions, + "the delete guard works and still refuses, rather than failing on a missing column") + _, err := f.settle(ctx, failed.ExecutionID, "checked the account") + require.ErrorIs(t, err, ErrExecutionSettleNotReady) + f.retryInOneTx(ctx, t, failed, uuid.NewString(), StatusCompleted) // the retry link still works +} diff --git a/internal/config/store_postgres_managed_guard.go b/internal/config/store_postgres_managed_guard.go index d541f16df..b90b9ff72 100644 --- a/internal/config/store_postgres_managed_guard.go +++ b/internal/config/store_postgres_managed_guard.go @@ -137,7 +137,14 @@ func sameAccountSet(a, b []string) bool { // settled (SettleFailedExecution): it can no longer be retried and keeps its // failed status, so detaching it hides nothing. func requireManagedDeleteTx(ctx context.Context, tx pgx.Tx, planID string) error { - rows, err := tx.Query(ctx, `SELECT status, no_submission, outcome_settled_at IS NOT NULL FROM purchase_executions WHERE plan_id = $1`, planID) + // Before the settlement migration no row can be settled (#999). + settledCol := "false" + if ready, err := settlementSchemaReady(ctx, tx); err != nil { + return err + } else if ready { + settledCol = "outcome_settled_at IS NOT NULL" + } + rows, err := tx.Query(ctx, `SELECT status, no_submission, `+settledCol+` FROM purchase_executions WHERE plan_id = $1`, planID) if err != nil { return fmt.Errorf("failed to read plan executions: %w", err) } diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index cfc297312..4d78be030 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -3,7 +3,6 @@ package config import ( "context" "encoding/json" - "errors" "fmt" "math" "strings" @@ -407,16 +406,11 @@ func (s *PostgresStore) checkAzurePricingMigration(ctx context.Context, required if !required { return nil } - var version int - var dirty bool - err := s.db.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty) - if errors.Is(err, pgx.ErrNoRows) { - return ErrAzurePricingNotReady - } + version, dirty, found, err := readSchemaVersion(ctx, s.db) if err != nil { return fmt.Errorf("check Azure pricing migration: %w", err) } - if version < azurePricingMigrationVersion || dirty { + if !found || version < azurePricingMigrationVersion || dirty { return ErrAzurePricingNotReady } return nil diff --git a/internal/config/store_postgres_schema_version.go b/internal/config/store_postgres_schema_version.go new file mode 100644 index 000000000..0ca8259c4 --- /dev/null +++ b/internal/config/store_postgres_schema_version.go @@ -0,0 +1,47 @@ +package config + +import ( + "context" + "errors" + "fmt" + + "github.com/jackc/pgx/v5" +) + +// settlementMigrationVersion is the migration that adds the outcome-settlement +// columns to purchase_executions (issue #999). +const settlementMigrationVersion = 157 + +// rowQuerier is satisfied by both the store's connection and a pgx.Tx, so a +// schema check can run inside the caller's transaction. +type rowQuerier interface { + QueryRow(ctx context.Context, sql string, args ...any) pgx.Row +} + +// readSchemaVersion returns the applied migration version and its dirty flag. +// found is false when schema_migrations has no row (nothing applied yet). +func readSchemaVersion(ctx context.Context, q rowQuerier) (version int, dirty, found bool, err error) { + err = q.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty) + if errors.Is(err, pgx.ErrNoRows) { + return 0, false, false, nil + } + if err != nil { + return 0, false, false, fmt.Errorf("read schema version: %w", err) + } + return version, dirty, true, nil +} + +// settlementSchemaReady reports whether the settlement columns exist. main +// deploys on merge while prod migrations are applied separately, so the code +// must run against a schema without them: the retry link and the managed +// delete guard fall back to their pre-settlement queries, and settling is +// refused (ErrExecutionSettleNotReady). A later version means the columns +// exist even if that later migration is dirty; at exactly this version a +// dirty flag means it may not have finished. +func settlementSchemaReady(ctx context.Context, q rowQuerier) (bool, error) { + version, dirty, found, err := readSchemaVersion(ctx, q) + if err != nil || !found { + return false, err + } + return version > settlementMigrationVersion || (version == settlementMigrationVersion && !dirty), nil +} diff --git a/internal/config/store_postgres_schema_version_test.go b/internal/config/store_postgres_schema_version_test.go new file mode 100644 index 000000000..dc51d7474 --- /dev/null +++ b/internal/config/store_postgres_schema_version_test.go @@ -0,0 +1,55 @@ +package config + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakeVersionRow struct { + version int + dirty bool + err error +} + +func (r fakeVersionRow) Scan(dest ...any) error { + if r.err != nil { + return r.err + } + *dest[0].(*int) = r.version + *dest[1].(*bool) = r.dirty + return nil +} + +type fakeVersionQuerier struct{ row fakeVersionRow } + +func (q fakeVersionQuerier) QueryRow(context.Context, string, ...any) pgx.Row { return q.row } + +// main deploys before prod migrations are applied, so the settlement paths +// must know whether the columns exist (#999). +func TestSettlementSchemaReady(t *testing.T) { + t.Parallel() + v := settlementMigrationVersion + cases := []struct { + name string + row fakeVersionRow + want bool + }{ + {"nothing applied", fakeVersionRow{err: pgx.ErrNoRows}, false}, + {"before the migration", fakeVersionRow{version: v - 1}, false}, + {"at the migration", fakeVersionRow{version: v}, true}, + {"at the migration, dirty", fakeVersionRow{version: v, dirty: true}, false}, + {"after it, even with a later dirty migration", fakeVersionRow{version: v + 1, dirty: true}, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := settlementSchemaReady(context.Background(), fakeVersionQuerier{tc.row}) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} From 1e902905add7d44da9bfb449161d797d71a08e9e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 01:20:07 +0100 Subject: [PATCH 09/15] fix(api): keep the settler identity out of the settle conflict response Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_execution_settle.go | 9 +++++++-- .../store_postgres_execution_settle_test.go | 20 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go index 9eb1dc919..4ecf92fea 100644 --- a/internal/config/store_postgres_execution_settle.go +++ b/internal/config/store_postgres_execution_settle.go @@ -6,6 +6,7 @@ import ( "fmt" "time" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/jackc/pgx/v5" ) @@ -224,13 +225,17 @@ func settlementRecord(req ExecutionSettleRequest, tail settleChainTail) string { } // replaySettlement answers a request for a row that is already settled: the -// stored settlement when this actor sent the same reason, else a conflict. +// stored settlement when this actor sent the same reason, else a conflict. The +// conflict goes back to the caller, who may not be the earlier settler, so it +// names only the time; the earlier settler is logged server-side. func replaySettlement(req ExecutionSettleRequest, t *settleTarget) (*ExecutionSettlement, error) { if t.settledBy == nil || t.settlement == nil { return nil, fmt.Errorf("execution %s has an incomplete settlement", req.ExecutionID) } if *t.settledBy != req.Actor || !settlementHasReason(*t.settlement, req.Reason) { - return nil, fmt.Errorf("%w: already settled by %s at %s", ErrExecutionSettleConflict, *t.settledBy, t.settledAt.UTC().Format(time.RFC3339)) + at := t.settledAt.UTC().Format(time.RFC3339) + logging.Infof("execution settle refused: execution %s was already settled by %s at %s", req.ExecutionID, *t.settledBy, at) + return nil, fmt.Errorf("%w: already settled at %s", ErrExecutionSettleConflict, at) } return &ExecutionSettlement{ExecutionID: req.ExecutionID, Status: t.status, SettledAt: *t.settledAt, SettledBy: *t.settledBy, Settlement: *t.settlement, Replayed: true}, nil } diff --git a/internal/config/store_postgres_execution_settle_test.go b/internal/config/store_postgres_execution_settle_test.go index 9018e7fc6..b6833c5da 100644 --- a/internal/config/store_postgres_execution_settle_test.go +++ b/internal/config/store_postgres_execution_settle_test.go @@ -1,7 +1,9 @@ package config import ( + "strings" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -38,3 +40,21 @@ func TestSettlementRecordReplayMatchesOnlyTheSameReason(t *testing.T) { require.False(t, settlementHasReason(record, "account")) require.False(t, settlementHasReason(record, `checked "eu" account2`)) } + +// The conflict text is the 409 body, and the caller may not be the earlier +// settler, so it must not carry that settler's principal ID. +func TestReplaySettlementConflictOmitsTheEarlierSettler(t *testing.T) { + at := time.Date(2026, 11, 4, 12, 0, 0, 0, time.UTC) + by := "settler-principal-7f3a" + record := settlementRecord(ExecutionSettleRequest{Reason: "checked the account"}, settleChainTail{id: "e1", status: "completed"}) + target := &settleTarget{status: "failed", settledAt: &at, settledBy: &by, settlement: &record} + for name, req := range map[string]ExecutionSettleRequest{ + "another actor": {ExecutionID: "e1", Actor: "someone-else", Reason: "checked the account"}, + "another reason": {ExecutionID: "e1", Actor: by, Reason: "a different reason"}, + } { + _, err := replaySettlement(req, target) + require.ErrorIs(t, err, ErrExecutionSettleConflict, name) + require.False(t, strings.Contains(err.Error(), by), "%s: %q names the earlier settler", name, err.Error()) + require.Contains(t, err.Error(), "2026-11-04T12:00:00Z", name) + } +} From f464387d7031a361b75ebb8b83661df5ea3ee8a5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:09:25 +0100 Subject: [PATCH 10/15] fix(config): replay detached settlements and refuse unsettled failed chain tails An identical repeat settle request now replays the stored result after the plan was deleted, instead of failing the detached-plan check first. A request that does not match the stored settlement, or any unsettled detached row, still conflicts. A failed retry-chain tail is now treated as possibly retryable whenever it is not itself settled, including one with a no_submission proof, because retry still accepts such a tail and a successor of a deleted plan would run as a direct purchase. Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_execution_settle.go | 35 ++++++++++++++----- ...tgres_execution_settle_integration_test.go | 33 +++++++++++++++++ 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go index 4ecf92fea..d91578078 100644 --- a/internal/config/store_postgres_execution_settle.go +++ b/internal/config/store_postgres_execution_settle.go @@ -135,7 +135,7 @@ func lockSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*sett return nil, fmt.Errorf("read execution %s: %w", executionID, err) } if planID == nil { - return nil, fmt.Errorf("%w: only an execution of a purchase plan can be settled", ErrExecutionSettleConflict) + return readDetachedSettleTarget(ctx, tx, executionID) } if _, lockErr := tx.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR SHARE`, *planID); lockErr != nil { return nil, fmt.Errorf("lock plan %s: %w", *planID, lockErr) @@ -154,6 +154,25 @@ func lockSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*sett return &t, nil } +// readDetachedSettleTarget serves a row with no plan. Only an already settled +// one is returned, so an identical repeat request still replays after the plan +// was deleted; anything else cannot be settled. The settlement never changes +// once written, so no lock is needed. +func readDetachedSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*settleTarget, error) { + var t settleTarget + err := tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, + outcome_settled_at, outcome_settled_by, outcome_settlement + FROM purchase_executions WHERE execution_id = $1`, executionID).Scan( + &t.status, &t.noSubmission, &t.retryID, &t.settledAt, &t.settledBy, &t.settlement) + if err != nil { + return nil, fmt.Errorf("read execution %s: %w", executionID, err) + } + if t.settledAt == nil { + return nil, fmt.Errorf("%w: only an execution of a purchase plan can be settled", ErrExecutionSettleConflict) + } + return &t, nil +} + // checkSettleTarget allows only the rows that block a managed delete for an // unknown outcome: failed without a no_submission proof. func checkSettleTarget(t *settleTarget) error { @@ -174,25 +193,25 @@ type settleChainTail struct { // lockSettleChainTail follows retry_execution_id from the target to the tail // and locks the tail. It refuses while the chain may still buy: a tail outside -// the terminal allowlist, a failed tail that can still be retried (no -// no_submission proof and not settled itself), or a chain cut short by the -// depth cap or changed between the walk and the lock. +// the terminal allowlist, a failed tail that is not itself settled (retry +// accepts even a no_submission tail), or a chain cut short by the depth cap or +// changed between the walk and the lock. func lockSettleChainTail(ctx context.Context, tx pgx.Tx, executionID string, next *string) (settleChainTail, error) { tail, err := walkSettleChain(ctx, tx, executionID, next) if err != nil || tail.id == executionID { return tail, err } var retryID *string - var noSubmission, settled bool - err = tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, outcome_settled_at IS NOT NULL - FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, tail.id).Scan(&tail.status, &noSubmission, &retryID, &settled) + var settled bool + err = tx.QueryRow(ctx, `SELECT status, retry_execution_id::text, outcome_settled_at IS NOT NULL + FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, tail.id).Scan(&tail.status, &retryID, &settled) if err != nil { return tail, fmt.Errorf("lock execution %s: %w", tail.id, err) } if retryID != nil { return tail, fmt.Errorf("%w: the retry chain changed while settling", ErrExecutionSettleConflict) } - retryable := tail.status == StatusFailed && !noSubmission && !settled + retryable := tail.status == StatusFailed && !settled if !ladderResolvableNodeStatus(tail.status) || retryable { return tail, fmt.Errorf("%w: retry %s is %s and may still buy or be retried; let it finish or settle it first", ErrExecutionSettleConflict, tail.id, tail.status) } diff --git a/internal/config/store_postgres_execution_settle_integration_test.go b/internal/config/store_postgres_execution_settle_integration_test.go index 30522f946..39ea9ea2b 100644 --- a/internal/config/store_postgres_execution_settle_integration_test.go +++ b/internal/config/store_postgres_execution_settle_integration_test.go @@ -152,6 +152,39 @@ func TestSettleFailedExecutionReplayAndRetry(t *testing.T) { })) } +// Once the plan is deleted the settled row is detached, yet an identical +// repeat request still replays the stored result; a different one conflicts. +func TestSettleReplayAfterPlanDelete(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + failed := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + first, err := f.settle(ctx, failed.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) + + again, err := f.settle(ctx, failed.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.True(t, again.Replayed) + require.True(t, first.SettledAt.Equal(again.SettledAt)) + _, err = f.settle(ctx, failed.ExecutionID, "a different reason entirely") + require.ErrorIs(t, err, ErrExecutionSettleConflict) +} + +// A failed tail with a no_submission proof can still be retried, so it keeps +// its predecessor's settlement refused. +func TestSettleRefusesChainWhoseTailIsFailedNoSubmission(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + head := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) + tail := f.retryInOneTx(ctx, t, head, uuid.NewString(), StatusFailed) + _, err := f.store.db.Exec(ctx, `UPDATE purchase_executions SET no_submission = true WHERE execution_id = $1`, tail.ExecutionID) + require.NoError(t, err) + + _, err = f.settle(ctx, head.ExecutionID, "operator checked the account") + require.ErrorIs(t, err, ErrExecutionSettleConflict) + require.False(t, f.settled(ctx, t, head.ExecutionID)) +} + // A retry that links the row first wins: the settle waits on the row lock, // then sees the live successor and refuses. func TestSettleWaitsForRetryAndRefusesItsLiveSuccessor(t *testing.T) { From 9bd9d309868c4bbbf6c7951fad0c2f8cda85f1c2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:11:50 +0100 Subject: [PATCH 11/15] fix(db): guard settled executions and split the settlement CHECK validation - 000157 adds a BEFORE UPDATE trigger that rejects linking a retry to a settled execution, so the "a settled row is never retried" rule holds for any writer, including a binary from before the settlement code. - 000157 down now locks purchase_executions and refuses while any settlement exists, so a reversal cannot silently discard audited settlements. The check and the drops share one DO block. - The completeness CHECK is added NOT VALID in 000157 to avoid a table scan under ACCESS EXCLUSIVE; new migration 000158 validates it (same split as 000121/000122). Both migrations are renumbered together at merge. settlementSchemaReady keeps gating on 000157 only. The columns, the trigger and the CHECK (enforced on new writes even when NOT VALID) all exist after 000157, and 000158 only validates existing rows, which the code never relies on, so requiring it would disable settling for no safety gain. Co-Authored-By: Claude Opus 5.5 --- ...ase_executions_outcome_settlement.down.sql | 28 +++++-- ...chase_executions_outcome_settlement.up.sql | 25 ++++++- ...hase_executions_outcome_settlement_test.go | 74 ++++++++++++++++++- ...ase_executions_outcome_settlement.down.sql | 9 +++ ...chase_executions_outcome_settlement.up.sql | 4 + 5 files changed, 127 insertions(+), 13 deletions(-) create mode 100644 internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql create mode 100644 internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql diff --git a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql index 7c1fba2c7..40039197b 100644 --- a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql +++ b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql @@ -1,8 +1,20 @@ --- Reverse 000157 (issue #999). Lossy: operator settlements are dropped, so a --- settled failed row blocks its managed plan's delete again. -ALTER TABLE purchase_executions - DROP CONSTRAINT IF EXISTS purchase_executions_outcome_settlement_complete; -ALTER TABLE purchase_executions - DROP COLUMN IF EXISTS outcome_settlement, - DROP COLUMN IF EXISTS outcome_settled_by, - DROP COLUMN IF EXISTS outcome_settled_at; +-- Reverse 000157 (issue #999). Refuses while any settlement exists: dropping +-- the columns would let a settled row be retried and block its plan's delete +-- again. The lock and the check share one DO block, so no settlement can land +-- between them. +DO $$ +BEGIN + LOCK TABLE purchase_executions IN ACCESS EXCLUSIVE MODE; + IF EXISTS (SELECT 1 FROM purchase_executions WHERE outcome_settled_at IS NOT NULL) THEN + RAISE EXCEPTION 'cannot reverse 000157: purchase_executions has operator settlements'; + END IF; + DROP TRIGGER IF EXISTS purchase_executions_refuse_retry_of_settled ON purchase_executions; + DROP FUNCTION IF EXISTS purchase_executions_refuse_retry_of_settled(); + ALTER TABLE purchase_executions + DROP CONSTRAINT IF EXISTS purchase_executions_outcome_settlement_complete; + ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS outcome_settlement, + DROP COLUMN IF EXISTS outcome_settled_by, + DROP COLUMN IF EXISTS outcome_settled_at; +END +$$; diff --git a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql index e98412894..8b088d854 100644 --- a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql +++ b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql @@ -18,4 +18,27 @@ ALTER TABLE purchase_executions ALTER TABLE purchase_executions ADD CONSTRAINT purchase_executions_outcome_settlement_complete CHECK ((outcome_settled_at IS NULL) = (outcome_settled_by IS NULL) - AND (outcome_settled_at IS NULL) = (outcome_settlement IS NULL)); + AND (outcome_settled_at IS NULL) = (outcome_settlement IS NULL)) + NOT VALID; +-- NOT VALID still checks every new write and skips the table scan under the +-- ACCESS EXCLUSIVE lock; 000158 validates it. + +-- A settled row is never retried. LinkRetryExecutionAtomic enforces it, and +-- this trigger holds it for any other writer, such as a binary from before +-- this migration during a rollout or rollback. +CREATE OR REPLACE FUNCTION purchase_executions_refuse_retry_of_settled() +RETURNS TRIGGER AS $$ +BEGIN + IF OLD.retry_execution_id IS NULL AND NEW.retry_execution_id IS NOT NULL + AND (OLD.outcome_settled_at IS NOT NULL OR NEW.outcome_settled_at IS NOT NULL) THEN + RAISE EXCEPTION 'execution % is settled and cannot be retried', OLD.execution_id + USING ERRCODE = 'check_violation'; + END IF; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS purchase_executions_refuse_retry_of_settled ON purchase_executions; +CREATE TRIGGER purchase_executions_refuse_retry_of_settled + BEFORE UPDATE OF retry_execution_id ON purchase_executions + FOR EACH ROW EXECUTE FUNCTION purchase_executions_refuse_retry_of_settled(); diff --git a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go index 498d50111..21e5ec3d2 100644 --- a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go +++ b/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go @@ -11,15 +11,21 @@ import ( "github.com/stretchr/testify/require" ) -// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000157 up, down and -// the all-or-nothing CHECK (#999). +// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000157 and its +// VALIDATE follow-up 000158: up, down and the all-or-nothing CHECK (#999). func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { ctx := context.Background() container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool, path := container.DB.Pool(), getMigrationsPath() - const version = 157 + const version, validateVersion = 157, 158 prev := previousMigrationVersion(t, version) + validated := func() bool { + var ok bool + require.NoError(t, pool.QueryRow(ctx, `SELECT convalidated FROM pg_constraint + WHERE conname = 'purchase_executions_outcome_settlement_complete'`).Scan(&ok)) + return ok + } columns := func() int { var n int @@ -39,6 +45,7 @@ func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) require.Equal(t, 3, columns()) + require.False(t, validated(), "000157 adds the CHECK NOT VALID to avoid a table scan under lock") var settled bool require.NoError(t, pool.QueryRow(ctx, `SELECT outcome_settled_at IS NOT NULL FROM purchase_executions WHERE execution_id = '00000000-0000-0000-0000-0000000009a1'`).Scan(&settled)) @@ -61,8 +68,67 @@ func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { require.Error(t, err, "CHECK must reject a partial settlement (%s)", name) } + _, err = pool.Exec(ctx, `UPDATE purchase_executions SET outcome_settled_at = NULL, outcome_settled_by = NULL, + outcome_settlement = NULL`) + require.NoError(t, err) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, validateVersion)) + require.True(t, validated(), "000158 validates the CHECK") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) require.Zero(t, columns(), "down must drop the columns") - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, validateVersion)) require.Equal(t, 3, columns(), "up must re-apply cleanly after down") } + +// The database itself refuses to link a retry to a settled row, so a binary +// from before the settlement code cannot undo "a settled row is never retried". +func TestMigration_OutcomeSettlementRefusesRetryLink(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool := container.DB.Pool() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 157)) + + insert := func(id, settled string) { + _, err := pool.Exec(ctx, `INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations, + outcome_settled_at, outcome_settled_by, outcome_settlement) + VALUES ($1, 'failed', 1, now(), '[]', `+settled+`)`, id) + require.NoError(t, err) + } + const settledID, openID, succ1, succ2 = "00000000-0000-0000-0000-0000000009b1", "00000000-0000-0000-0000-0000000009b2", + "00000000-0000-0000-0000-0000000009b3", "00000000-0000-0000-0000-0000000009b4" + insert(settledID, `now(), 'u', 's'`) + insert(openID, `NULL, NULL, NULL`) + insert(succ1, `NULL, NULL, NULL`) + insert(succ2, `NULL, NULL, NULL`) + + link := `UPDATE purchase_executions SET retry_execution_id = $2 WHERE execution_id = $1` + _, err := pool.Exec(ctx, link, settledID, succ1) + require.Error(t, err, "linking a retry to a settled row must fail in the database") + _, err = pool.Exec(ctx, link, openID, succ2) + require.NoError(t, err, "an unsettled failed row can still be retried") +} + +// Down refuses to drop settlements, which would let a settled row be retried +// and its plan's delete refused again; the refusal leaves the schema intact. +func TestMigration_OutcomeSettlementDownRefusesWithSettlements(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + prev := previousMigrationVersion(t, 157) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 157)) + _, err := pool.Exec(ctx, `INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations, + outcome_settled_at, outcome_settled_by, outcome_settlement) + VALUES (gen_random_uuid(), 'failed', 1, now(), '[]', now(), 'u', 's')`) + require.NoError(t, err) + + require.Error(t, migrations.MigrateToVersion(ctx, pool, path, prev), "down must refuse while a settlement exists") + var cols, triggers int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns + WHERE table_name = 'purchase_executions' AND column_name LIKE 'outcome_settle%'`).Scan(&cols)) + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM pg_trigger + WHERE tgname = 'purchase_executions_refuse_retry_of_settled'`).Scan(&triggers)) + require.Equal(t, 3, cols, "a refused down leaves the columns") + require.Equal(t, 1, triggers, "a refused down leaves the trigger") +} diff --git a/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql new file mode 100644 index 000000000..3980db132 --- /dev/null +++ b/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql @@ -0,0 +1,9 @@ +-- A validated constraint cannot be un-validated; re-add it NOT VALID, as the +-- state 000157 leaves (same pattern as 000122). +ALTER TABLE purchase_executions + DROP CONSTRAINT purchase_executions_outcome_settlement_complete; +ALTER TABLE purchase_executions + ADD CONSTRAINT purchase_executions_outcome_settlement_complete + CHECK ((outcome_settled_at IS NULL) = (outcome_settled_by IS NULL) + AND (outcome_settled_at IS NULL) = (outcome_settlement IS NULL)) + NOT VALID; diff --git a/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql new file mode 100644 index 000000000..462af2c74 --- /dev/null +++ b/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql @@ -0,0 +1,4 @@ +-- Validate the CHECK that 000157 added NOT VALID (issue #999); same split as +-- 000121/000122. VALIDATE scans without blocking writes. +ALTER TABLE purchase_executions + VALIDATE CONSTRAINT purchase_executions_outcome_settlement_complete; From bcacaa8fd96750850c5032e3857bcd93f0dbd5b6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:12:13 +0100 Subject: [PATCH 12/15] docs(known-issues): name the settlement migration without a stale number The #999 entry cited migration 000143; the settlement migration is 000157 on this branch and is renumbered at merge, so refer to it by name. Co-Authored-By: Claude Opus 5.5 --- known_issues/resolved/35_legacy_guard_followups.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/known_issues/resolved/35_legacy_guard_followups.md b/known_issues/resolved/35_legacy_guard_followups.md index 04a506209..379e63f20 100644 --- a/known_issues/resolved/35_legacy_guard_followups.md +++ b/known_issues/resolved/35_legacy_guard_followups.md @@ -30,4 +30,4 @@ **Status:** ✔️ Resolved -**Resolved by:** an explicit, audited operator settle, `POST /api/purchases/{id}/settle` (`SettleFailedExecution`, migration 000143). It needs `execute:purchases`, account access and the grant's constraints, a reason and `acknowledge_unknown_outcome: true`, and refuses while the row's retry chain may still buy. The row stays `failed` and keeps counting as possibly bought in the ramp ledger; once settled it can no longer be retried and no longer blocks `requireManagedDeleteTx`. Delete itself gained no flag. +**Resolved by:** an explicit, audited operator settle, `POST /api/purchases/{id}/settle` (`SettleFailedExecution`, the outcome-settlement migration, 000157 on this branch and renumbered at merge). It needs `execute:purchases`, account access and the grant's constraints, a reason and `acknowledge_unknown_outcome: true`, and refuses while the row's retry chain may still buy. The row stays `failed` and keeps counting as possibly bought in the ramp ledger; once settled it can no longer be retried and no longer blocks `requireManagedDeleteTx`. Delete itself gained no flag. From bd4013463ef610ec68726128aef73f93c4e6c5b9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 15:12:32 +0100 Subject: [PATCH 13/15] fix(config): allow settling a failed execution that was never submitted A failed execution carrying a no_submission proof is still accepted by retry, so a retry chain whose tail is such a row could never be settled: the head was refused while the tail was unsettled, and the tail was refused as needing no settlement. Settling it is safe because nothing was submitted, and it only stops further retries. The other refusals are unchanged: non-failed status, rows without a plan, and a chain that may still buy. Co-Authored-By: Claude Opus 5.5 --- internal/api/openapi.yaml | 5 ++- .../config/store_postgres_execution_settle.go | 32 ++++++-------- ...tgres_execution_settle_integration_test.go | 43 +++++++++++++++---- 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 66d46afee..f4abc0793 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1494,8 +1494,9 @@ paths: Requires execute permission on purchases, execution account access and matching permission constraints. The row remains failed and counts as possibly bought; settlement permanently prevents retry and can unblock - deletion of its managed plan. A retry chain that may still buy is - refused. Repeating an identical settlement returns the stored result. + deletion of its managed plan. A failed execution proven never + submitted can be settled too, to stop it being retried. A retry chain + that may still buy is refused. Repeating an identical settlement returns the stored result. parameters: - $ref: '#/components/parameters/CSRFToken' requestBody: diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go index d91578078..b4ed0d6db 100644 --- a/internal/config/store_postgres_execution_settle.go +++ b/internal/config/store_postgres_execution_settle.go @@ -48,18 +48,17 @@ type ExecutionSettlement struct { // settleTarget is the locked row SettleFailedExecution judges. type settleTarget struct { - status string - noSubmission bool - retryID *string - planID *string - settledAt *time.Time - settledBy *string - settlement *string + status string + retryID *string + planID *string + settledAt *time.Time + settledBy *string + settlement *string } // SettleFailedExecution records an operator's audited settlement of a failed -// plan execution that has no no_submission proof, so it stops blocking the -// delete of its editor-managed plan (requireManagedDeleteTx). It never changes +// plan execution, so it stops blocking the delete of its editor-managed plan +// (requireManagedDeleteTx). It never changes // the status and never makes the row count as unbought: the ramp ledger and // the retention trigger keep treating it as possibly bought. A settled row can // no longer be retried (LinkRetryExecutionAtomic), because a retry of a row @@ -141,10 +140,10 @@ func lockSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*sett return nil, fmt.Errorf("lock plan %s: %w", *planID, lockErr) } var t settleTarget - err = tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, plan_id::text, + err = tx.QueryRow(ctx, `SELECT status, retry_execution_id::text, plan_id::text, outcome_settled_at, outcome_settled_by, outcome_settlement FROM purchase_executions WHERE execution_id = $1 FOR UPDATE`, executionID).Scan( - &t.status, &t.noSubmission, &t.retryID, &t.planID, &t.settledAt, &t.settledBy, &t.settlement) + &t.status, &t.retryID, &t.planID, &t.settledAt, &t.settledBy, &t.settlement) if err != nil { return nil, fmt.Errorf("lock execution %s: %w", executionID, err) } @@ -160,10 +159,10 @@ func lockSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*sett // once written, so no lock is needed. func readDetachedSettleTarget(ctx context.Context, tx pgx.Tx, executionID string) (*settleTarget, error) { var t settleTarget - err := tx.QueryRow(ctx, `SELECT status, no_submission, retry_execution_id::text, + err := tx.QueryRow(ctx, `SELECT status, retry_execution_id::text, outcome_settled_at, outcome_settled_by, outcome_settlement FROM purchase_executions WHERE execution_id = $1`, executionID).Scan( - &t.status, &t.noSubmission, &t.retryID, &t.settledAt, &t.settledBy, &t.settlement) + &t.status, &t.retryID, &t.settledAt, &t.settledBy, &t.settlement) if err != nil { return nil, fmt.Errorf("read execution %s: %w", executionID, err) } @@ -173,15 +172,12 @@ func readDetachedSettleTarget(ctx context.Context, tx pgx.Tx, executionID string return &t, nil } -// checkSettleTarget allows only the rows that block a managed delete for an -// unknown outcome: failed without a no_submission proof. +// checkSettleTarget allows only a failed row. One proven never submitted is +// allowed too: retry still accepts it, so settling is how it stops being retried. func checkSettleTarget(t *settleTarget) error { if t.status != StatusFailed { return fmt.Errorf("%w: execution is %s; only a failed execution can be settled", ErrExecutionSettleConflict, t.status) } - if t.noSubmission { - return fmt.Errorf("%w: execution is proven never submitted and needs no settlement", ErrExecutionSettleConflict) - } return nil } diff --git a/internal/config/store_postgres_execution_settle_integration_test.go b/internal/config/store_postgres_execution_settle_integration_test.go index 39ea9ea2b..c03101d1d 100644 --- a/internal/config/store_postgres_execution_settle_integration_test.go +++ b/internal/config/store_postgres_execution_settle_integration_test.go @@ -76,10 +76,9 @@ func TestSettleFailedExecutionRefusals(t *testing.T) { f := managedSettleFixture(ctx, t) acct := &f.accounts[0] conflict := map[string]string{ - "completed": f.row(ctx, t, 1, acct, StatusCompleted).ExecutionID, - "pending": f.row(ctx, t, 1, acct, "pending").ExecutionID, - "canceled": f.row(ctx, t, 1, acct, StatusCanceled).ExecutionID, - "failed no_submission": f.rowWithRecs(ctx, t, 1, acct, StatusFailed, true).ExecutionID, + "completed": f.row(ctx, t, 1, acct, StatusCompleted).ExecutionID, + "pending": f.row(ctx, t, 1, acct, "pending").ExecutionID, + "canceled": f.row(ctx, t, 1, acct, StatusCanceled).ExecutionID, } for _, tail := range []string{"pending", "running", StatusFailed} { head := f.row(ctx, t, 2, acct, StatusFailed) @@ -170,19 +169,47 @@ func TestSettleReplayAfterPlanDelete(t *testing.T) { require.ErrorIs(t, err, ErrExecutionSettleConflict) } -// A failed tail with a no_submission proof can still be retried, so it keeps -// its predecessor's settlement refused. -func TestSettleRefusesChainWhoseTailIsFailedNoSubmission(t *testing.T) { +// A never-submitted failed row can still be retried, so it must be settleable: +// otherwise a head whose tail is such a row could never be settled. +func TestSettleFailedNoSubmissionChain(t *testing.T) { ctx := context.Background() f := managedSettleFixture(ctx, t) head := f.row(ctx, t, 1, &f.accounts[0], StatusFailed) tail := f.retryInOneTx(ctx, t, head, uuid.NewString(), StatusFailed) _, err := f.store.db.Exec(ctx, `UPDATE purchase_executions SET no_submission = true WHERE execution_id = $1`, tail.ExecutionID) require.NoError(t, err) + ledger := f.committed(ctx, t) _, err = f.settle(ctx, head.ExecutionID, "operator checked the account") - require.ErrorIs(t, err, ErrExecutionSettleConflict) + require.ErrorIs(t, err, ErrExecutionSettleConflict, "the unsettled tail may still be retried") require.False(t, f.settled(ctx, t, head.ExecutionID)) + _, err = f.settle(ctx, tail.ExecutionID, "operator checked the account") + require.NoError(t, err) + _, err = f.settle(ctx, head.ExecutionID, "operator checked the account") + require.NoError(t, err) + require.Equal(t, ledger, f.committed(ctx, t), "settling must not change what the ledger counts") + + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) + successor := &PurchaseExecution{ExecutionID: uuid.NewString(), Status: "pending", StepNumber: 1} + require.NoError(t, f.store.WithTx(ctx, func(tx pgx.Tx) error { + require.NoError(t, f.store.SavePurchaseExecutionTx(ctx, tx, successor)) + linked, linkErr := f.store.LinkRetryExecutionAtomic(ctx, tx, tail.ExecutionID, successor.ExecutionID) + require.NoError(t, linkErr) + require.False(t, linked, "a settled tail must not be retried") + return nil + })) +} + +// A never-submitted row on its own is settleable and stays never-submitted. +func TestSettleFailedNoSubmissionRow(t *testing.T) { + ctx := context.Background() + f := managedSettleFixture(ctx, t) + row := f.rowWithRecs(ctx, t, 1, &f.accounts[0], StatusFailed, true) + _, err := f.settle(ctx, row.ExecutionID, "operator checked the account") + require.NoError(t, err) + got, err := f.store.GetExecutionByID(ctx, row.ExecutionID) + require.NoError(t, err) + require.True(t, got.NoSubmission) } // A retry that links the row first wins: the settle waits on the row lock, From 2861f4d3dd4173a9265e4fb4716d175516b86366 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 16:13:49 +0100 Subject: [PATCH 14/15] docs(config): describe the ledger and audit tag for never-submitted settlements SettleFailedExecution now says it counts the row in the ledger exactly as before, which is possibly bought unless the row was never submitted. The settlementRecord doc notes that its fixed audit tag also covers a never-submitted row and stays fixed so replays compare equal. Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres_execution_settle.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/config/store_postgres_execution_settle.go b/internal/config/store_postgres_execution_settle.go index b4ed0d6db..97b83e60b 100644 --- a/internal/config/store_postgres_execution_settle.go +++ b/internal/config/store_postgres_execution_settle.go @@ -58,9 +58,9 @@ type settleTarget struct { // SettleFailedExecution records an operator's audited settlement of a failed // plan execution, so it stops blocking the delete of its editor-managed plan -// (requireManagedDeleteTx). It never changes -// the status and never makes the row count as unbought: the ramp ledger and -// the retention trigger keep treating it as possibly bought. A settled row can +// (requireManagedDeleteTx). It never changes the status or no_submission, so +// the ramp ledger and the retention trigger count the row exactly as before +// (possibly bought, unless it was never submitted). A settled row can // no longer be retried (LinkRetryExecutionAtomic), because a retry of a row // whose plan was deleted would buy with no plan. // @@ -233,7 +233,8 @@ func walkSettleChain(ctx context.Context, tx pgx.Tx, executionID string, next *s // settlementRecord is the exact string stored in outcome_settlement. Fields are // fixed and quoted, so a repeat of the same request compares equal and a -// different one cannot. +// different one cannot. The "settled_unknown_outcome" tag also covers a +// never-submitted row; it stays fixed so replays keep comparing equal. func settlementRecord(req ExecutionSettleRequest, tail settleChainTail) string { return fmt.Sprintf("operator:settled_unknown_outcome: ack=true chain_tail=%s tail_status=%s reason=%q", tail.id, tail.status, req.Reason) From f0b496750ebabb773f05598d909ee3ad0ce30b91 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 16:33:23 +0100 Subject: [PATCH 15/15] chore(db): renumber the outcome-settlement migrations to 000155 and 000156 Migration numbers are set at merge time. main is at 000154, so the settlement migration becomes 000155 and its VALIDATE follow-up 000156. This renames the five files and updates settlementMigrationVersion, the migration tests' versions, the SQL comments and the known-issues record. Number-only change. Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_schema_version.go | 2 +- ...ase_executions_outcome_settlement.down.sql} | 4 ++-- ...chase_executions_outcome_settlement.up.sql} | 4 ++-- ...hase_executions_outcome_settlement_test.go} | 18 +++++++++--------- ...ase_executions_outcome_settlement.down.sql} | 2 +- ...chase_executions_outcome_settlement.up.sql} | 2 +- .../resolved/35_legacy_guard_followups.md | 2 +- 7 files changed, 17 insertions(+), 17 deletions(-) rename internal/database/postgres/migrations/{000157_purchase_executions_outcome_settlement.down.sql => 000155_purchase_executions_outcome_settlement.down.sql} (87%) rename internal/database/postgres/migrations/{000157_purchase_executions_outcome_settlement.up.sql => 000155_purchase_executions_outcome_settlement.up.sql} (95%) rename internal/database/postgres/migrations/{000157_purchase_executions_outcome_settlement_test.go => 000155_purchase_executions_outcome_settlement_test.go} (94%) rename internal/database/postgres/migrations/{000158_validate_purchase_executions_outcome_settlement.down.sql => 000156_validate_purchase_executions_outcome_settlement.down.sql} (89%) rename internal/database/postgres/migrations/{000158_validate_purchase_executions_outcome_settlement.up.sql => 000156_validate_purchase_executions_outcome_settlement.up.sql} (73%) diff --git a/internal/config/store_postgres_schema_version.go b/internal/config/store_postgres_schema_version.go index 0ca8259c4..731feba5d 100644 --- a/internal/config/store_postgres_schema_version.go +++ b/internal/config/store_postgres_schema_version.go @@ -10,7 +10,7 @@ import ( // settlementMigrationVersion is the migration that adds the outcome-settlement // columns to purchase_executions (issue #999). -const settlementMigrationVersion = 157 +const settlementMigrationVersion = 155 // rowQuerier is satisfied by both the store's connection and a pgx.Tx, so a // schema check can run inside the caller's transaction. diff --git a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement.down.sql similarity index 87% rename from internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql rename to internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement.down.sql index 40039197b..aec64696d 100644 --- a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.down.sql +++ b/internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement.down.sql @@ -1,4 +1,4 @@ --- Reverse 000157 (issue #999). Refuses while any settlement exists: dropping +-- Reverse 000155 (issue #999). Refuses while any settlement exists: dropping -- the columns would let a settled row be retried and block its plan's delete -- again. The lock and the check share one DO block, so no settlement can land -- between them. @@ -6,7 +6,7 @@ DO $$ BEGIN LOCK TABLE purchase_executions IN ACCESS EXCLUSIVE MODE; IF EXISTS (SELECT 1 FROM purchase_executions WHERE outcome_settled_at IS NOT NULL) THEN - RAISE EXCEPTION 'cannot reverse 000157: purchase_executions has operator settlements'; + RAISE EXCEPTION 'cannot reverse 000155: purchase_executions has operator settlements'; END IF; DROP TRIGGER IF EXISTS purchase_executions_refuse_retry_of_settled ON purchase_executions; DROP FUNCTION IF EXISTS purchase_executions_refuse_retry_of_settled(); diff --git a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement.up.sql similarity index 95% rename from internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql rename to internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement.up.sql index 8b088d854..4fe592bf7 100644 --- a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement.up.sql +++ b/internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement.up.sql @@ -1,4 +1,4 @@ --- Migration 000157 (issue #999): an operator's audited settlement of a failed +-- Migration 000155 (issue #999): an operator's audited settlement of a failed -- execution whose outcome is unknown (failed without no_submission proof). -- -- Settling never changes status and never makes the row count as unbought: @@ -21,7 +21,7 @@ ALTER TABLE purchase_executions AND (outcome_settled_at IS NULL) = (outcome_settlement IS NULL)) NOT VALID; -- NOT VALID still checks every new write and skips the table scan under the --- ACCESS EXCLUSIVE lock; 000158 validates it. +-- ACCESS EXCLUSIVE lock; 000156 validates it. -- A settled row is never retried. LinkRetryExecutionAtomic enforces it, and -- this trigger holds it for any other writer, such as a binary from before diff --git a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go b/internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement_test.go similarity index 94% rename from internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go rename to internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement_test.go index 21e5ec3d2..d9ee4bc12 100644 --- a/internal/database/postgres/migrations/000157_purchase_executions_outcome_settlement_test.go +++ b/internal/database/postgres/migrations/000155_purchase_executions_outcome_settlement_test.go @@ -11,14 +11,14 @@ import ( "github.com/stretchr/testify/require" ) -// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000157 and its -// VALIDATE follow-up 000158: up, down and the all-or-nothing CHECK (#999). +// TestMigration_PurchaseExecutionsOutcomeSettlement covers 000155 and its +// VALIDATE follow-up 000156: up, down and the all-or-nothing CHECK (#999). func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { ctx := context.Background() container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool, path := container.DB.Pool(), getMigrationsPath() - const version, validateVersion = 157, 158 + const version, validateVersion = 155, 156 prev := previousMigrationVersion(t, version) validated := func() bool { var ok bool @@ -37,7 +37,7 @@ func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { } require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) - require.Zero(t, columns(), "before 000157 the columns must not exist") + require.Zero(t, columns(), "before 000155 the columns must not exist") _, err := pool.Exec(ctx, ` INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations) VALUES ('00000000-0000-0000-0000-0000000009a1', 'failed', 1, now(), '[]')`) @@ -45,7 +45,7 @@ func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, version)) require.Equal(t, 3, columns()) - require.False(t, validated(), "000157 adds the CHECK NOT VALID to avoid a table scan under lock") + require.False(t, validated(), "000155 adds the CHECK NOT VALID to avoid a table scan under lock") var settled bool require.NoError(t, pool.QueryRow(ctx, `SELECT outcome_settled_at IS NOT NULL FROM purchase_executions WHERE execution_id = '00000000-0000-0000-0000-0000000009a1'`).Scan(&settled)) @@ -72,7 +72,7 @@ func TestMigration_PurchaseExecutionsOutcomeSettlement(t *testing.T) { outcome_settlement = NULL`) require.NoError(t, err) require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, validateVersion)) - require.True(t, validated(), "000158 validates the CHECK") + require.True(t, validated(), "000156 validates the CHECK") require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) require.Zero(t, columns(), "down must drop the columns") @@ -87,7 +87,7 @@ func TestMigration_OutcomeSettlementRefusesRetryLink(t *testing.T) { container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool := container.DB.Pool() - require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 157)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 155)) insert := func(id, settled string) { _, err := pool.Exec(ctx, `INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations, @@ -116,8 +116,8 @@ func TestMigration_OutcomeSettlementDownRefusesWithSettlements(t *testing.T) { container := testhelpers.RequirePostgresContainer(ctx, t) t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) pool, path := container.DB.Pool(), getMigrationsPath() - prev := previousMigrationVersion(t, 157) - require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 157)) + prev := previousMigrationVersion(t, 155) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 155)) _, err := pool.Exec(ctx, `INSERT INTO purchase_executions (execution_id, status, step_number, scheduled_date, recommendations, outcome_settled_at, outcome_settled_by, outcome_settlement) VALUES (gen_random_uuid(), 'failed', 1, now(), '[]', now(), 'u', 's')`) diff --git a/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql b/internal/database/postgres/migrations/000156_validate_purchase_executions_outcome_settlement.down.sql similarity index 89% rename from internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql rename to internal/database/postgres/migrations/000156_validate_purchase_executions_outcome_settlement.down.sql index 3980db132..6988bfabc 100644 --- a/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.down.sql +++ b/internal/database/postgres/migrations/000156_validate_purchase_executions_outcome_settlement.down.sql @@ -1,5 +1,5 @@ -- A validated constraint cannot be un-validated; re-add it NOT VALID, as the --- state 000157 leaves (same pattern as 000122). +-- state 000155 leaves (same pattern as 000122). ALTER TABLE purchase_executions DROP CONSTRAINT purchase_executions_outcome_settlement_complete; ALTER TABLE purchase_executions diff --git a/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql b/internal/database/postgres/migrations/000156_validate_purchase_executions_outcome_settlement.up.sql similarity index 73% rename from internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql rename to internal/database/postgres/migrations/000156_validate_purchase_executions_outcome_settlement.up.sql index 462af2c74..9ff474186 100644 --- a/internal/database/postgres/migrations/000158_validate_purchase_executions_outcome_settlement.up.sql +++ b/internal/database/postgres/migrations/000156_validate_purchase_executions_outcome_settlement.up.sql @@ -1,4 +1,4 @@ --- Validate the CHECK that 000157 added NOT VALID (issue #999); same split as +-- Validate the CHECK that 000155 added NOT VALID (issue #999); same split as -- 000121/000122. VALIDATE scans without blocking writes. ALTER TABLE purchase_executions VALIDATE CONSTRAINT purchase_executions_outcome_settlement_complete; diff --git a/known_issues/resolved/35_legacy_guard_followups.md b/known_issues/resolved/35_legacy_guard_followups.md index 379e63f20..7b5b40da3 100644 --- a/known_issues/resolved/35_legacy_guard_followups.md +++ b/known_issues/resolved/35_legacy_guard_followups.md @@ -30,4 +30,4 @@ **Status:** ✔️ Resolved -**Resolved by:** an explicit, audited operator settle, `POST /api/purchases/{id}/settle` (`SettleFailedExecution`, the outcome-settlement migration, 000157 on this branch and renumbered at merge). It needs `execute:purchases`, account access and the grant's constraints, a reason and `acknowledge_unknown_outcome: true`, and refuses while the row's retry chain may still buy. The row stays `failed` and keeps counting as possibly bought in the ramp ledger; once settled it can no longer be retried and no longer blocks `requireManagedDeleteTx`. Delete itself gained no flag. +**Resolved by:** an explicit, audited operator settle, `POST /api/purchases/{id}/settle` (`SettleFailedExecution`, the outcome-settlement migration 000155, validated by 000156). It needs `execute:purchases`, account access and the grant's constraints, a reason and `acknowledge_unknown_outcome: true`, and refuses while the row's retry chain may still buy. The row stays `failed` and keeps counting as possibly bought in the ramp ledger; once settled it can no longer be retried and no longer blocks `requireManagedDeleteTx`. Delete itself gained no flag.