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

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