From 077fdc2297a389356c7748c9a6c589981ef8992d Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 18:42:26 +0100 Subject: [PATCH] fix(config): disable plans left with no accounts when their account is deleted DeleteCloudAccount let the plan_accounts cascade remove an enabled plan's only target, leaving it enabled with zero accounts (the unscoped shape migration 000060 removed). Inside the existing transaction it now locks the account row first, so a writer attaching the account to a plan is waited for, then disables every enabled plan whose only plan_accounts row is this account, before the DELETE. A refused delete rolls the disable back with it. Closes #264 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 47 ++++++++ ...ostgres_delete_account_integration_test.go | 111 ++++++++++++++++++ .../config/store_postgres_pgxmock_test.go | 53 +++++++++ ...e_postgres_writer_race_integration_test.go | 2 +- 4 files changed, 212 insertions(+), 1 deletion(-) 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")