From 1c2a9807fc274aad4c5b38368484509f8e5690ad Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 01:42:43 +0100 Subject: [PATCH] fix(config): cancel a deleted plan's open step rows in the same transaction purchase_executions.plan_id is ON DELETE SET NULL, and token approval applies the plan's max_step_commitment and the approve-own plan-step rule only while plan_id is set. A pending, notified or paused step row left open when its plan was deleted became a plan-less row that a token approver with no approve verb could approve past the plan's cap. DeletePurchasePlan now cancels those rows (and releases their suppressions) inside its transaction, before the plan row goes, so no committed state ever has an approvable row orphaned from its plan. Scheduled, approved, running and settled rows are left as they were. Closes #862 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 27 +++++++ .../config/store_postgres_pgxmock_test.go | 23 +++++- ...e_postgres_plan_delete_integration_test.go | 76 +++++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 internal/config/store_postgres_plan_delete_integration_test.go diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index e7aaea8b..7fc1378f 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -880,6 +880,9 @@ func (s *PostgresStore) DeletePurchasePlan(ctx context.Context, planID string) e return err } } + if err := cancelOpenPlanExecutionsTx(ctx, tx, planID); err != nil { + return err + } if _, err := tx.Exec(ctx, `DELETE FROM purchase_plans WHERE id = $1`, planID); err != nil { return fmt.Errorf("failed to delete purchase plan: %w", err) } @@ -887,6 +890,30 @@ func (s *PostgresStore) DeletePurchasePlan(ctx context.Context, planID string) e }) } +// cancelOpenPlanExecutionsTx cancels the plan's step rows that can still be +// approved or run (pending, notified, paused) in the delete's transaction. +// purchase_executions.plan_id is ON DELETE SET NULL (migration 000033), and +// token approval applies the plan's max_step_commitment and the approve-own +// plan-step rule only while plan_id is set (approveConstraintsForSession), so +// a row left open would become approvable past its plan's cap (issue #862). +// The rows' suppressions go with them, as on the other cancel paths. +func cancelOpenPlanExecutionsTx(ctx context.Context, tx pgx.Tx, planID string) error { + const q = ` + WITH canceled AS ( + UPDATE purchase_executions + SET status = $2, updated_at = NOW() + WHERE plan_id = $1 + AND status IN ('pending', 'notified', 'paused') + RETURNING execution_id + ) + DELETE FROM purchase_suppressions + WHERE execution_id IN (SELECT execution_id FROM canceled)` + if _, err := tx.Exec(ctx, q, planID, StatusCanceled); err != nil { + return fmt.Errorf("failed to cancel open executions of purchase plan: %w", err) + } + return nil +} + // buildListPlansQuery returns the SQL query and args for ListPurchasePlans. // // When accountIDs is empty the query returns all plans without an unassigned diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index a67dd202..0a8405e0 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -649,7 +649,10 @@ func TestPGXMock_DeletePurchasePlan_Success(t *testing.T) { ctx := context.Background() expectLockedUnmanagedPlan(mock) - mock.ExpectExec("DELETE").WithArgs(pgxmock.AnyArg()). + mock.ExpectExec(`(?s)UPDATE purchase_executions.*WHERE plan_id = \$1.*'pending', 'notified', 'paused'.*DELETE FROM purchase_suppressions`). + WithArgs("plan-id", StatusCanceled). + WillReturnResult(pgxmock.NewResult("DELETE", 0)) + mock.ExpectExec("DELETE FROM purchase_plans").WithArgs("plan-id"). WillReturnResult(pgxmock.NewResult("DELETE", 1)) mock.ExpectCommit() @@ -658,6 +661,24 @@ func TestPGXMock_DeletePurchasePlan_Success(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } +// TestPGXMock_DeletePurchasePlan_CancelFailureKeepsPlan pins issue #862's +// ordering: the plan row is deleted only after its open step rows were +// canceled in the same transaction, so a failed cancel leaves both in place. +func TestPGXMock_DeletePurchasePlan_CancelFailureKeepsPlan(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + expectLockedUnmanagedPlan(mock) + mock.ExpectExec("UPDATE purchase_executions").WithArgs("plan-id", StatusCanceled). + WillReturnError(errors.New("db down")) + mock.ExpectRollback() + + err := store.DeletePurchasePlan(ctx, "plan-id") + require.ErrorContains(t, err, "cancel open executions") + assert.NoError(t, mock.ExpectationsWereMet()) +} + func TestPGXMock_DeletePurchasePlan_NotFound(t *testing.T) { mock := newMock(t) store := storeWith(mock) diff --git a/internal/config/store_postgres_plan_delete_integration_test.go b/internal/config/store_postgres_plan_delete_integration_test.go new file mode 100644 index 00000000..403951f5 --- /dev/null +++ b/internal/config/store_postgres_plan_delete_integration_test.go @@ -0,0 +1,76 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// TestDeletePurchasePlan_CancelsOpenStepRows covers issue #862: plan_id is ON +// DELETE SET NULL, and token approval applies the plan's step cap only while +// plan_id is set, so a step row still open when its plan is deleted must not +// survive as an approvable, plan-less row. +func TestDeletePurchasePlan_CancelsOpenStepRows(t *testing.T) { + ctx := context.Background() + f := newRampFixture(ctx, t, 1, 0, 4) + acct := &f.accounts[0] + + open := map[string]*PurchaseExecution{} + for _, status := range []string{"pending", "notified", "paused"} { + open[status] = f.row(ctx, t, 1, acct, status) + } + settled := map[string]*PurchaseExecution{} + for _, status := range []string{"scheduled", "approved", StatusCompleted, StatusFailed} { + settled[status] = f.row(ctx, t, 1, acct, status) + } + suppress := func(execID string) { + require.NoError(t, f.store.CreateSuppression(ctx, &PurchaseSuppression{ + ExecutionID: execID, AccountID: *acct, Provider: "aws", Service: "ec2", + Region: "us-east-1", ResourceType: "m5.large", SuppressedCount: 1, + ExpiresAt: time.Now().Add(24 * time.Hour), + })) + } + suppress(open["pending"].ExecutionID) + suppress(settled["scheduled"].ExecutionID) + + other := &PurchasePlan{Name: "other " + uuid.NewString()[:8], Services: map[string]ServiceConfig{}} + require.NoError(t, f.store.CreatePurchasePlan(ctx, other)) + sibling := &PurchaseExecution{ + ExecutionID: uuid.NewString(), IdempotencyKey: uuid.NewString(), PlanID: other.ID, + Status: "pending", StepNumber: 1, ScheduledDate: time.Now(), + } + require.NoError(t, f.store.SavePurchaseExecution(ctx, sibling)) + + require.NoError(t, f.store.DeletePurchasePlan(ctx, f.planID)) + + for status, exec := range open { + got, err := f.store.GetExecutionByID(ctx, exec.ExecutionID) + require.NoError(t, err) + require.Equal(t, StatusCanceled, got.Status, "%s row of the deleted plan stays approvable", status) + require.Empty(t, got.PlanID, "%s row is detached from the deleted plan", status) + _, err = f.store.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"pending", "notified", "paused"}, "running", nil) + require.ErrorIs(t, err, ErrExecutionNotInExpectedStatus, "%s row can still be claimed for approval", status) + } + for status, exec := range settled { + got, err := f.store.GetExecutionByID(ctx, exec.ExecutionID) + require.NoError(t, err) + require.Equal(t, status, got.Status, "%s row is not an open step row and keeps its status", status) + } + got, err := f.store.GetExecutionByID(ctx, sibling.ExecutionID) + require.NoError(t, err) + require.Equal(t, "pending", got.Status, "another plan's row is untouched") + + var pendingSup, scheduledSup int + require.NoError(t, f.store.db.QueryRow(ctx, + `SELECT COUNT(*) FROM purchase_suppressions WHERE execution_id = $1`, open["pending"].ExecutionID).Scan(&pendingSup)) + require.NoError(t, f.store.db.QueryRow(ctx, + `SELECT COUNT(*) FROM purchase_suppressions WHERE execution_id = $1`, settled["scheduled"].ExecutionID).Scan(&scheduledSup)) + require.Zero(t, pendingSup, "the canceled row's suppression is released") + require.Equal(t, 1, scheduledSup, "a row that was not canceled keeps its suppression") +}