diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index e7aaea8bd..7fc1378fd 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 a67dd2029..0a8405e0b 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 000000000..403951f5f --- /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") +}