diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 3b4d7d460..cd26ddf40 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3921,6 +3921,8 @@ func (s *PostgresStore) UpdateCloudAccount(ctx context.Context, account *CloudAc // If an approved account_registrations row points at this account, it is reset to // 'pending' in the same transaction so the admin can re-approve through the normal // flow instead of being left with a dead-end "Approved (account pending link)" row. +// Enabled plans whose only target is this account are disabled in the same +// transaction (issue #264); see disableSoleAccountPlansTx. func (s *PostgresStore) DeleteCloudAccount(ctx context.Context, id string) error { tx, err := s.db.Begin(ctx) if err != nil { @@ -3928,6 +3930,16 @@ func (s *PostgresStore) DeleteCloudAccount(ctx context.Context, id string) error } defer rollbackTx(ctx, tx, "DeleteCloudAccount") + // Lock the account row before reading which plans target it. A writer + // attaching the account to a plan holds a KEY SHARE lock on this row + // (the plan_accounts FK check) until it commits, so this waits for it and + // the plan disable below sees its rows. Without the lock the DELETE would + // still wait for that writer, but only after the plans were picked, and + // its cascade would leave that plan enabled with no accounts. + if _, err = tx.Exec(ctx, `SELECT 1 FROM cloud_accounts WHERE id = $1 FOR UPDATE`, id); err != nil { + return fmt.Errorf("failed to lock cloud account: %w", err) + } + // Reset any linked approved registration first (explicit NULL so we don't // rely on the FK's ON DELETE SET NULL behavior). if _, err = tx.Exec(ctx, ` @@ -3959,6 +3971,11 @@ func (s *PostgresStore) DeleteCloudAccount(ctx context.Context, id string) error return fmt.Errorf("failed to detach terminal executions: %w", err) } + disabled, err := disableSoleAccountPlansTx(ctx, tx, id) + if err != nil { + return err + } + tag, err := tx.Exec(ctx, `DELETE FROM cloud_accounts WHERE id = $1`, id) if err != nil { return fmt.Errorf("failed to delete cloud account: %w", err) @@ -3970,9 +3987,39 @@ func (s *PostgresStore) DeleteCloudAccount(ctx context.Context, id string) error if err = tx.Commit(ctx); err != nil { return fmt.Errorf("failed to commit cloud account deletion: %w", err) } + if len(disabled) > 0 { + logging.Infof("deleting cloud account %s disabled %d plan(s) left with no target accounts: %v", id, len(disabled), disabled) + } return nil } +// disableSoleAccountPlansTx disables every enabled plan whose only +// plan_accounts row is accountID, and returns their IDs (issue #264). The +// account delete cascades those rows away, and a plan with no accounts is the +// unscoped "universal plan" shape migration 000060 removed: the scheduler +// would keep creating executions for it with no account to buy in. Disabling +// rather than refusing the delete keeps the plan and its history for the +// admin to re-target and re-enable. Plans already disabled are not rewritten, +// so their updated_at (the plan edit token) does not move. +func disableSoleAccountPlansTx(ctx context.Context, tx pgx.Tx, accountID string) ([]string, error) { + rows, err := tx.Query(ctx, ` + UPDATE purchase_plans p + SET enabled = false + WHERE p.enabled + AND EXISTS (SELECT 1 FROM plan_accounts pa WHERE pa.plan_id = p.id AND pa.account_id = $1) + AND NOT EXISTS (SELECT 1 FROM plan_accounts pa WHERE pa.plan_id = p.id AND pa.account_id <> $1) + RETURNING p.id::text + `, accountID) + if err != nil { + return nil, fmt.Errorf("failed to disable plans targeting only this account: %w", err) + } + ids, err := pgx.CollectRows(rows, pgx.RowTo[string]) + if err != nil { + return nil, fmt.Errorf("failed to disable plans targeting only this account: %w", err) + } + return ids, nil +} + // ListCloudAccounts returns accounts matching the filter, with credentials_configured derived. func (s *PostgresStore) ListCloudAccounts(ctx context.Context, filter CloudAccountFilter) ([]CloudAccount, error) { query := ` diff --git a/internal/config/store_postgres_delete_account_integration_test.go b/internal/config/store_postgres_delete_account_integration_test.go index a0077908e..13f9716a8 100644 --- a/internal/config/store_postgres_delete_account_integration_test.go +++ b/internal/config/store_postgres_delete_account_integration_test.go @@ -15,6 +15,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" "github.com/google/uuid" "github.com/jackc/pgx/v5/pgconn" "github.com/stretchr/testify/assert" @@ -195,3 +196,113 @@ func TestTerminalExecutionStatuses_CoverSchema(t *testing.T) { sort.Strings(classified) assert.Equal(t, schema, classified) } + +// seedDeletePlan inserts a plan in the given enabled state targeting accounts. +func seedDeletePlan(ctx context.Context, t *testing.T, store *PostgresStore, enabled bool, accounts ...string) string { + t.Helper() + var id string + require.NoError(t, store.db.QueryRow(ctx, + `INSERT INTO purchase_plans (name, enabled, auto_purchase) VALUES ('delete-264', $1, true) RETURNING id::text`, + enabled).Scan(&id)) + for _, acct := range accounts { + _, err := store.db.Exec(ctx, `INSERT INTO plan_accounts (plan_id, account_id) VALUES ($1, $2)`, id, acct) + require.NoError(t, err) + } + return id +} + +func storedPlanState(ctx context.Context, t *testing.T, store *PostgresStore, planID string) (enabled bool, accounts int, updatedAt time.Time) { + t.Helper() + require.NoError(t, store.db.QueryRow(ctx, ` + SELECT p.enabled, (SELECT COUNT(*) FROM plan_accounts pa WHERE pa.plan_id = p.id), p.updated_at + FROM purchase_plans p WHERE p.id = $1`, planID).Scan(&enabled, &accounts, &updatedAt)) + return enabled, accounts, updatedAt +} + +// Issue #264: a plan whose only account is deleted must not stay enabled with +// zero plan_accounts rows (the unscoped shape migration 000060 removed). Plans +// that keep another account, plans already disabled and plans on other +// accounts are left as they were. +func TestDeleteCloudAccount_DisablesPlansLeftWithoutAccounts(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + gone, kept := seedDeleteAccount(ctx, t, store), seedDeleteAccount(ctx, t, store) + + soleGone := seedDeletePlan(ctx, t, store, true, gone) + shared := seedDeletePlan(ctx, t, store, true, gone, kept) + alreadyOff := seedDeletePlan(ctx, t, store, false, gone) + other := seedDeletePlan(ctx, t, store, true, kept) + // A plan with no accounts yet (createPlan writes the plan, then its + // accounts) is not one this delete orphaned. + unassigned := seedDeletePlan(ctx, t, store, true) + _, _, offBefore := storedPlanState(ctx, t, store, alreadyOff) + _, _, otherBefore := storedPlanState(ctx, t, store, other) + _, _, sharedBefore := storedPlanState(ctx, t, store, shared) + + require.NoError(t, store.DeleteCloudAccount(ctx, gone)) + + enabled, n, _ := storedPlanState(ctx, t, store, soleGone) + assert.Zero(t, n, "the cascade removed the plan's only account") + assert.False(t, enabled, "a plan left with no accounts must be disabled") + + enabled, n, at := storedPlanState(ctx, t, store, shared) + assert.Equal(t, 1, n) + assert.True(t, enabled, "a plan that keeps another account stays enabled") + assert.True(t, at.Equal(sharedBefore), "a plan that keeps another account must not be rewritten") + + enabled, _, at = storedPlanState(ctx, t, store, alreadyOff) + assert.False(t, enabled) + assert.True(t, at.Equal(offBefore), "an already disabled plan must not be rewritten") + + enabled, n, at = storedPlanState(ctx, t, store, other) + assert.Equal(t, 1, n) + assert.True(t, enabled, "a plan on another account is untouched") + assert.True(t, at.Equal(otherBefore), "a plan on another account must not be rewritten") + + enabled, _, _ = storedPlanState(ctx, t, store, unassigned) + assert.True(t, enabled, "a plan that never targeted the deleted account is untouched") +} + +// A refused delete rolls the plan disable back with it. +func TestDeleteCloudAccount_RefusedDeleteKeepsPlanEnabled(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + acct := seedDeleteAccount(ctx, t, store) + plan := seedDeletePlan(ctx, t, store, true, acct) + seedDeleteAccountExec(ctx, t, store, acct, "running") + + require.Error(t, store.DeleteCloudAccount(ctx, acct)) + + enabled, n, _ := storedPlanState(ctx, t, store, plan) + assert.True(t, enabled, "the disable must roll back with the refused delete") + assert.Equal(t, 1, n) +} + +// A plan that gains the account in a transaction still open when the delete +// starts is disabled too: the delete locks the account row before it picks +// the plans, so it waits for that writer and sees its committed rows instead +// of cascading them away afterwards and leaving the plan enabled. +func TestDeleteCloudAccount_DisablesPlanThatGainedAccountConcurrently(t *testing.T) { + ctx := context.Background() + store := setupLadderStore(ctx, t) + gone, kept := seedDeleteAccount(ctx, t, store), seedDeleteAccount(ctx, t, store) + plan := seedDeletePlan(ctx, t, store, true, kept) + + writer, err := store.db.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = writer.Rollback(context.Background()) }) + _, err = writer.Exec(ctx, `DELETE FROM plan_accounts WHERE plan_id = $1`, plan) + require.NoError(t, err) + _, err = writer.Exec(ctx, `INSERT INTO plan_accounts (plan_id, account_id) VALUES ($1, $2)`, plan, gone) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- store.DeleteCloudAccount(ctx, gone) }() + testhelpers.WaitForLockWait(ctx, t, store.db, "%cloud_accounts%") + require.NoError(t, writer.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) + + enabled, n, _ := storedPlanState(ctx, t, store, plan) + assert.Zero(t, n) + assert.False(t, enabled, "the plan's only account is gone, so it must be disabled") +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 894897eb4..35eafdd6b 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1777,9 +1777,11 @@ func TestPGXMock_DeleteCloudAccount_Success(t *testing.T) { ctx := context.Background() mock.ExpectBegin() + expectLockDeletedAccount(mock) mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 0)) expectDetachTerminalExecutions(mock) + expectDisableSoleAccountPlans(mock) mock.ExpectExec("DELETE FROM cloud_accounts").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 1)) mock.ExpectCommit() @@ -1794,9 +1796,11 @@ func TestPGXMock_DeleteCloudAccount_WithLinkedRegistration(t *testing.T) { ctx := context.Background() mock.ExpectBegin() + expectLockDeletedAccount(mock) mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 1)) expectDetachTerminalExecutions(mock) + expectDisableSoleAccountPlans(mock) mock.ExpectExec("DELETE FROM cloud_accounts").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 1)) mock.ExpectCommit() @@ -1811,9 +1815,11 @@ func TestPGXMock_DeleteCloudAccount_NotFound(t *testing.T) { ctx := context.Background() mock.ExpectBegin() + expectLockDeletedAccount(mock) mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 0)) expectDetachTerminalExecutions(mock) + expectDisableSoleAccountPlans(mock) mock.ExpectExec("DELETE FROM cloud_accounts").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 0)) mock.ExpectRollback() @@ -1832,12 +1838,59 @@ func expectDetachTerminalExecutions(mock pgxmock.PgxPoolIface) { WillReturnResult(pgxmock.NewResult("UPDATE", 2)) } +// expectLockDeletedAccount expects DeleteCloudAccount's first statement: the +// account row lock taken before the plans are picked (issue #264). +func expectLockDeletedAccount(mock pgxmock.PgxPoolIface) { + mock.ExpectExec(`SELECT 1 FROM cloud_accounts WHERE id = \$1 FOR UPDATE`).WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) +} + +// expectDisableSoleAccountPlans expects the issue #264 disable of enabled +// plans whose only account is the deleted one. +func expectDisableSoleAccountPlans(mock pgxmock.PgxPoolIface) { + mock.ExpectQuery(`UPDATE purchase_plans p\s+SET enabled = false\s+WHERE p.enabled`).WithArgs(pgxmock.AnyArg()). + WillReturnRows(pgxmock.NewRows([]string{"id"}).AddRow("plan-1")) +} + +func TestPGXMock_DeleteCloudAccount_LockFailsRollsBack(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectBegin() + mock.ExpectExec("SELECT 1 FROM cloud_accounts").WithArgs("acct-id").WillReturnError(errors.New("boom")) + mock.ExpectRollback() + + err := store.DeleteCloudAccount(ctx, "acct-id") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to lock cloud account") +} + +func TestPGXMock_DeleteCloudAccount_DisablePlansFailsRollsBack(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectBegin() + expectLockDeletedAccount(mock) + mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + expectDetachTerminalExecutions(mock) + mock.ExpectQuery("UPDATE purchase_plans").WithArgs("acct-id").WillReturnError(errors.New("boom")) + mock.ExpectRollback() + + err := store.DeleteCloudAccount(ctx, "acct-id") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to disable plans targeting only this account") +} + func TestPGXMock_DeleteCloudAccount_DetachFailsRollsBack(t *testing.T) { mock := newMock(t) store := storeWith(mock) ctx := context.Background() mock.ExpectBegin() + expectLockDeletedAccount(mock) mock.ExpectExec("UPDATE account_registrations").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("UPDATE", 0)) mock.ExpectExec("UPDATE purchase_executions").WithArgs(pgxmock.AnyArg(), TerminalExecutionStatuses). diff --git a/internal/config/store_postgres_writer_race_integration_test.go b/internal/config/store_postgres_writer_race_integration_test.go index 695b3531e..379734e18 100644 --- a/internal/config/store_postgres_writer_race_integration_test.go +++ b/internal/config/store_postgres_writer_race_integration_test.go @@ -83,7 +83,7 @@ func TestAccountDeleteWaitsForCommandAccountShare(t *testing.T) { f := newRampFixture(ctx, t, 1, 1, 3) command := testhelpers.HoldCommandFootprint(ctx, t, f.store.db, f.planID) done := inBackground(func() error { return f.store.DeleteCloudAccount(ctx, f.accounts[0]) }) - testhelpers.WaitForLockWait(ctx, t, f.store.db, "%DELETE FROM cloud_accounts%") + testhelpers.WaitForLockWait(ctx, t, f.store.db, "%FROM cloud_accounts WHERE id = $1 FOR UPDATE%") var n int require.NoError(t, command.QueryRow(ctx, `SELECT count(*) FROM plan_accounts WHERE plan_id = $1`, f.planID).Scan(&n)) require.Equal(t, 1, n, "the command still sees its account while the delete waits")