fix(config): disable plans left with no accounts when their account is deleted

internal/config/store_postgres.go

@@ -3921,13 +3921,25 @@ 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 {
 		return fmt.Errorf("failed to begin transaction: %w", err)
 	}
 	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 := `

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")
+}

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).

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")