fix(config): let an account with only terminal executions be deleted (#1020)

* fix(config): let an account with only terminal executions be deleted

Migration 000053 made purchase_executions.cloud_account_id ON DELETE
RESTRICT, so one completed/failed/canceled/expired execution blocked
the account delete forever, and the handler answered with a 409 asking
the operator to cancel pending purchases that did not exist.

DeleteCloudAccount now detaches the account's terminal executions
(cloud_account_id = NULL, status in the new TerminalExecutionStatuses
allowlist) in its transaction before the DELETE, keeping them as
history. Live rows stay attached, so the FK still refuses the delete.
The deleteAccount preflight adds CountLiveExecutionsForAccount and
answers a 409 with reason "active_executions" for approved, running,
scheduled or paused rows, after the existing pending/notified 409.

Migration 000127 gives purchase_executions its own updated_at trigger
that keeps updated_at on a detach-only UPDATE, so deleting an account
does not pull old failures back into the plan health window or
restart the retention clock of canceled rows.

Closes #265

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(db): renumber executions-detach migration to 000128 after #905

#905 merged its own 000127 (never_started_legacy_ramps_to_new), so this
PR's migration moves to 000128. The down step in its test now targets the
real predecessor instead of a hard-coded 126.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): re-run the delete preflight after an FK-violation race

When DeleteCloudAccount hits SQLSTATE 23503 because a row raced past the
preflight, re-query so the client gets the structured pending (with
counts) or active 409 for that row. Fall back to the bare pending 409 if
the re-query fails or no longer sees a blocker (CodeRabbit on #1020).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: IAmJSD

internal/analytics/collector_test.go

@@ -299,6 +299,10 @@ func (m *mockConfigStore) CountPendingExecutionsForAccount(ctx context.Context,
 	return 0, nil
 }
 
+func (m *mockConfigStore) CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error) {
+	return 0, nil
+}
+
 func (m *mockConfigStore) ListPendingExecutionIDsForAccount(ctx context.Context, accountID string) ([]string, error) {
 	return nil, nil
 }

internal/api/handler_accounts.go

@@ -588,12 +588,12 @@ func (h *Handler) updateAccount(ctx context.Context, httpReq *events.LambdaFunct
 
 // deleteAccount handles DELETE /api/accounts/:id.
 //
-// Preflights the delete against pending/notified purchase_executions
-// (issue #606). Migration 000053 also enforces ON DELETE RESTRICT at the
-// DB level so a race that slips past the preflight still can't orphan a
-// pending execution — but the preflight gives the frontend a structured
-// 409 (with the count + execution_ids) so it can offer a Cancel-All-Then-
-// Delete affordance instead of surfacing a raw FK-violation 500.
+// Preflights the delete against the purchase_executions that still need the
+// account (preflightAccountDelete). Executions in a terminal status do not
+// block: DeleteCloudAccount detaches them (cloud_account_id = NULL) and keeps
+// them as history (issue #265). Migration 000053 also enforces ON DELETE
+// RESTRICT at the DB level, so a live execution that races past the
+// preflight still can't be orphaned.
 func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionURLRequest, id string) (any, error) {
 	if err := validateUUID(id); err != nil {
 		return nil, err
@@ -611,46 +611,26 @@ func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionU
 		return nil, err
 	}
 
-	// Preflight: refuse the delete if pending/notified executions still
-	// reference this account. The frontend uses the pending_count + the
-	// pending_execution_ids list to drive the Cancel-All-Then-Delete UX.
-	pendingCount, err := h.config.CountPendingExecutionsForAccount(ctx, id)
-	if err != nil {
-		return nil, fmt.Errorf("accounts: %w", err)
-	}
-	if pendingCount > 0 {
-		execIDs, listErr := h.config.ListPendingExecutionIDsForAccount(ctx, id)
-		if listErr != nil {
-			// Count succeeded but list failed — still surface the 409 so the
-			// operator gets a clear "cancel them first" message instead of
-			// the raw FK error from the eventual DB delete. The list payload
-			// is omitted; the frontend falls back to a generic message.
-			return nil, NewClientErrorWithDetails(409,
-				fmt.Sprintf("cannot delete account: %d pending purchase(s) must be canceled first", pendingCount),
-				map[string]any{
-					"pending_count": pendingCount,
-					"reason":        "pending_executions",
-				})
-		}
-		return nil, NewClientErrorWithDetails(409,
-			fmt.Sprintf("cannot delete account: %d pending purchase(s) must be canceled first", pendingCount),
-			map[string]any{
-				"pending_count":         pendingCount,
-				"pending_execution_ids": execIDs,
-				"reason":                "pending_executions",
-			})
+	if err := h.preflightAccountDelete(ctx, id); err != nil {
+		return nil, err
 	}
 
 	if err := h.config.DeleteCloudAccount(ctx, id); err != nil {
-		// Race: the preflight count was 0 but a pending execution row was
-		// inserted concurrently before we issued the DELETE. Migration 000053
-		// enforces ON DELETE RESTRICT, so Postgres raises SQLSTATE 23503
-		// (foreign_key_violation). Map this to the same structured 409 the
-		// preflight branch returns so the frontend's Cancel-All-Then-Delete
-		// affordance still kicks in — minus the count/ids, which we can't
-		// supply without re-querying (and the operator can just retry).
+		// Race: the preflight found no live execution but one was inserted
+		// concurrently before we issued the DELETE. Migration 000053 enforces
+		// ON DELETE RESTRICT, so Postgres raises SQLSTATE 23503
+		// (foreign_key_violation). Terminal executions never cause this:
+		// DeleteCloudAccount detaches them in the same transaction. Re-run the
+		// preflight so the client gets the same structured 409 (pending with
+		// counts and ids, or active), and fall back to a bare pending 409 if
+		// the re-query fails or no longer sees the blocker.
 		var pgErr *pgconn.PgError
 		if errors.As(err, &pgErr) && pgErr.Code == "23503" {
+			if perr := h.preflightAccountDelete(ctx, id); perr != nil {
+				if ce, ok := IsClientError(perr); ok {
+					return nil, ce
+				}
+			}
 			return nil, NewClientErrorWithDetails(409,
 				"cannot delete account: pending purchase(s) must be canceled first",
 				map[string]any{
@@ -663,6 +643,66 @@ func (h *Handler) deleteAccount(ctx context.Context, req *events.LambdaFunctionU
 	return nil, nil
 }
 
+// preflightAccountDelete refuses the delete with a structured 409 while any
+// purchase execution still needs the account:
+//
+//   - pending/notified rows (reason "pending_executions", issue #606): the
+//     frontend uses pending_count + pending_execution_ids to drive the
+//     Cancel-All-Then-Delete UX.
+//   - any other non-terminal row, e.g. approved, running, scheduled or
+//     paused (reason "active_executions", issue #265). These can't be
+//     canceled from the account page, so the operator has to let them
+//     finish, or revoke or delete them, first.
+//
+// Terminal rows are not counted: DeleteCloudAccount detaches them and keeps
+// them as history.
+func (h *Handler) preflightAccountDelete(ctx context.Context, id string) error {
+	pendingCount, err := h.config.CountPendingExecutionsForAccount(ctx, id)
+	if err != nil {
+		return fmt.Errorf("accounts: %w", err)
+	}
+	if pendingCount > 0 {
+		return h.pendingExecutionsConflict(ctx, id, pendingCount)
+	}
+
+	liveCount, err := h.config.CountLiveExecutionsForAccount(ctx, id)
+	if err != nil {
+		return fmt.Errorf("accounts: %w", err)
+	}
+	if liveCount > 0 {
+		return NewClientErrorWithDetails(409,
+			fmt.Sprintf("cannot delete account: %d in-progress purchase(s) (approved, running, scheduled or paused) still use it; "+
+				"wait for them to finish, or revoke or delete them, first", liveCount),
+			map[string]any{
+				"active_count": liveCount,
+				"reason":       "active_executions",
+			})
+	}
+	return nil
+}
+
+// pendingExecutionsConflict builds the issue #606 409 for pendingCount
+// pending/notified executions, with their IDs when they can be listed.
+func (h *Handler) pendingExecutionsConflict(ctx context.Context, id string, pendingCount int) error {
+	msg := fmt.Sprintf("cannot delete account: %d pending purchase(s) must be canceled first", pendingCount)
+	execIDs, listErr := h.config.ListPendingExecutionIDsForAccount(ctx, id)
+	if listErr != nil {
+		// Count succeeded but list failed — still surface the 409 so the
+		// operator gets a clear "cancel them first" message instead of
+		// the raw FK error from the eventual DB delete. The list payload
+		// is omitted; the frontend falls back to a generic message.
+		return NewClientErrorWithDetails(409, msg, map[string]any{
+			"pending_count": pendingCount,
+			"reason":        "pending_executions",
+		})
+	}
+	return NewClientErrorWithDetails(409, msg, map[string]any{
+		"pending_count":         pendingCount,
+		"pending_execution_ids": execIDs,
+		"reason":                "pending_executions",
+	})
+}
+
 // parseAndValidateCredentialsRequest performs all input validation that must
 // run before we touch the database: UUID format, permission, body parse,
 // credential_type allowlist, and per-type payload schema. Pulled out of

internal/api/handler_accounts_test.go

@@ -530,6 +530,131 @@ func TestDeleteAccount_PendingZero_DeletesNormally(t *testing.T) {
 	assert.True(t, deleteCalled, "delete should proceed when no pending executions")
 }
 
+// TestDeleteAccount_ActiveExecutions_Returns409 asserts the issue #265
+// preflight: with no cancelable pending/notified rows but an approved,
+// running, scheduled or paused row still on the account, the handler refuses
+// with a 409 that says why (reason "active_executions" + count) instead of
+// letting the FK raise the misleading "cancel pending purchases" 409.
+func TestDeleteAccount_ActiveExecutions_Returns409(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	store := setupAdminMock(ctx)
+	store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 0, nil }
+	var liveAccount string
+	store.CountLiveExecutionsForAccountFn = func(_ context.Context, id string) (int, error) {
+		liveAccount = id
+		return 2, nil
+	}
+	deleteCalled := false
+	store.DeleteCloudAccountFn = func(_ context.Context, _ string) error {
+		deleteCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	const id = "11111111-1111-1111-1111-111111111111"
+	result, err := handler.deleteAccount(ctx, adminRequest(""), id)
+	assert.Nil(t, result)
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected ClientError, got %v", err)
+	assert.Equal(t, 409, ce.code)
+	assert.Contains(t, ce.message, "2 in-progress purchase(s)")
+	assert.NotContains(t, ce.message, "canceled first", "these rows cannot be canceled; the message must not say to")
+	details := ce.Details()
+	require.NotNil(t, details)
+	assert.EqualValues(t, 2, details["active_count"])
+	assert.Equal(t, "active_executions", details["reason"])
+	assert.Equal(t, id, liveAccount, "the live count must be scoped to the account being deleted")
+	assert.False(t, deleteCalled, "DeleteCloudAccount must NOT be called while live executions exist")
+}
+
+// TestDeleteAccount_PendingTakesPrecedenceOverActive asserts the #606
+// Cancel-All-Then-Delete 409 still wins when cancelable rows exist, so the
+// frontend keeps getting the execution IDs it needs.
+func TestDeleteAccount_PendingTakesPrecedenceOverActive(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	store := setupAdminMock(ctx)
+	store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 1, nil }
+	store.ListPendingExecutionIDsForAccountFn = func(_ context.Context, _ string) ([]string, error) {
+		return []string{"exec-1"}, nil
+	}
+	liveCalled := false
+	store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) {
+		liveCalled = true
+		return 4, nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	_, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111")
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected ClientError, got %v", err)
+	assert.Equal(t, 409, ce.code)
+	assert.Equal(t, "pending_executions", ce.Details()["reason"])
+	assert.False(t, liveCalled, "the live count is not needed once pending rows already block")
+}
+
+// TestDeleteAccount_OnlyTerminalExecutions_Deletes asserts the issue #265
+// happy path at the handler: no pending and no live rows means the delete
+// goes through (DeleteCloudAccount detaches the terminal history itself).
+func TestDeleteAccount_OnlyTerminalExecutions_Deletes(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	store := setupAdminMock(ctx)
+	store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 0, nil }
+	liveCalled := false
+	store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) {
+		liveCalled = true
+		return 0, nil
+	}
+	deleteCalled := false
+	store.DeleteCloudAccountFn = func(_ context.Context, _ string) error {
+		deleteCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	result, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111")
+	require.NoError(t, err)
+	assert.Nil(t, result)
+	assert.True(t, liveCalled, "the live preflight must run before the delete")
+	assert.True(t, deleteCalled)
+}
+
+// TestDeleteAccount_LiveCountErr_Returns500 asserts a failing live count is
+// surfaced as an error (500) and never falls through to the delete.
+func TestDeleteAccount_LiveCountErr_Returns500(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	store := setupAdminMock(ctx)
+	store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) { return 0, nil }
+	store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) {
+		return 0, errors.New("db down")
+	}
+	deleteCalled := false
+	store.DeleteCloudAccountFn = func(_ context.Context, _ string) error {
+		deleteCalled = true
+		return nil
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	_, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111")
+	require.Error(t, err)
+	_, ok := IsClientError(err)
+	assert.False(t, ok, "a store failure must not be reported as a client error")
+	assert.Contains(t, err.Error(), "db down")
+	assert.False(t, deleteCalled)
+}
+
 // TestDeleteAccount_PendingListErr_StillReturns409 asserts that a failure
 // listing execution_ids doesn't downgrade the 409 to a 500 — the operator
 // still gets the structured count and the explicit "cancel first" ask,
@@ -598,6 +723,93 @@ func TestDeleteAccount_FKViolationRace_Returns409(t *testing.T) {
 	assert.Equal(t, "pending_executions", details["reason"])
 }
 
+// TestDeleteAccount_FKViolationRace_RequeriesPreflight asserts that after a
+// 23503 race the handler re-runs the preflight, so the client gets the
+// specific structured 409 for whatever row raced in: pending (with its count)
+// or active (approved/running/scheduled/paused), not a generic pending 409.
+func TestDeleteAccount_FKViolationRace_RequeriesPreflight(t *testing.T) {
+	cases := []struct {
+		name       string
+		pending    int
+		live       int
+		wantReason string
+		wantKey    string
+	}{
+		{"pending row raced in", 2, 0, "pending_executions", "pending_count"},
+		{"active row raced in", 0, 1, "active_executions", "active_count"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.Background()
+			mockAuth := new(MockAuthService)
+			setupAdminAuth(ctx, mockAuth)
+
+			store := setupAdminMock(ctx)
+			pendingCalls, liveCalls := 0, 0
+			// First call is the preflight (nothing yet); the second is the
+			// re-query after the FK violation, which sees the raced-in row.
+			store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) {
+				pendingCalls++
+				if pendingCalls == 1 {
+					return 0, nil
+				}
+				return tc.pending, nil
+			}
+			store.CountLiveExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) {
+				liveCalls++
+				if liveCalls == 1 {
+					return 0, nil
+				}
+				return tc.live, nil
+			}
+			store.DeleteCloudAccountFn = func(_ context.Context, _ string) error {
+				return &pgconn.PgError{Code: "23503"}
+			}
+			handler := &Handler{auth: mockAuth, config: store}
+
+			_, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111")
+			require.Error(t, err)
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "expected ClientError (409), got %T: %v", err, err)
+			assert.Equal(t, 409, ce.code)
+			details := ce.Details()
+			require.NotNil(t, details)
+			assert.Equal(t, tc.wantReason, details["reason"])
+			assert.Contains(t, details, tc.wantKey, "re-queried 409 carries the count")
+		})
+	}
+}
+
+// TestDeleteAccount_FKViolationRace_RequeryErrorFallsBack asserts that if the
+// re-query itself fails, the handler still returns the bare pending 409
+// rather than surfacing the re-query's 500.
+func TestDeleteAccount_FKViolationRace_RequeryErrorFallsBack(t *testing.T) {
+	ctx := context.Background()
+	mockAuth := new(MockAuthService)
+	setupAdminAuth(ctx, mockAuth)
+
+	store := setupAdminMock(ctx)
+	calls := 0
+	store.CountPendingExecutionsForAccountFn = func(_ context.Context, _ string) (int, error) {
+		calls++
+		if calls == 1 {
+			return 0, nil
+		}
+		return 0, errors.New("connection reset")
+	}
+	store.DeleteCloudAccountFn = func(_ context.Context, _ string) error {
+		return &pgconn.PgError{Code: "23503"}
+	}
+	handler := &Handler{auth: mockAuth, config: store}
+
+	_, err := handler.deleteAccount(ctx, adminRequest(""), "11111111-1111-1111-1111-111111111111")
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected ClientError (409), got %T: %v", err, err)
+	assert.Equal(t, 409, ce.code)
+	assert.Equal(t, "pending_executions", ce.Details()["reason"])
+}
+
 // TestDeleteAccount_NonFKError_StillReturns500 asserts that errors which
 // are NOT FK violations continue to bubble up as wrapped errors (500),
 // so we don't accidentally swallow real failures as 409s.

internal/config/interfaces.go

@@ -208,6 +208,13 @@ type StoreInterface interface {
 	// 409 with a count so the frontend can offer Cancel-All-Then-Delete UX
 	// instead of surfacing a raw constraint error. See issue #606.
 	CountPendingExecutionsForAccount(ctx context.Context, accountID string) (int, error)
+	// CountLiveExecutionsForAccount returns the number of purchase_executions
+	// referencing the given cloud account whose status is not in
+	// TerminalExecutionStatuses (pending, notified, approved, running,
+	// scheduled, paused, or any status not yet classified). These are the
+	// rows that block DeleteCloudAccount; terminal rows are detached and kept
+	// as history instead (issue #265).
+	CountLiveExecutionsForAccount(ctx context.Context, accountID string) (int, error)
 	// ListPendingExecutionIDsForAccount returns the execution IDs of all
 	// pending / notified executions referencing this account, used by the
 	// frontend's Cancel-All-Then-Delete flow when the operator opts to