From 8a08afd83fb640ed06b1345b1e4db6ac9b0d95b6 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Thu, 11 Jun 2026 01:07:33 -0700
Subject: [PATCH] fix(purchase): surface save error on failed-status execution
 record

The credential-resolution failure branch in executeForAccount discarded
the SavePurchaseExecution error (_ =), while the post-purchase save in
the same function treats a save failure as AUDIT LOSS. If the DB write
failed on the credential-failure path, the per-account "failed" row was
silently lost with no trace, leaving a gap in the History audit trail.

Handle the failure-path save exactly like the success path: when the
save fails, return an AUDIT LOSS error joined with the original
credential-resolution error so neither failure is masked and the save
error stays inspectable via errors.Is.

Regression test asserts both errors surface; it fails on the previous
code (save error silently dropped) and passes with the fix.

Closes #1184
---
 internal/purchase/execution.go      | 13 +++++-
 internal/purchase/execution_test.go | 65 +++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go
index a5a43b89..8d307414 100644
--- a/internal/purchase/execution.go
+++ b/internal/purchase/execution.go
@@ -229,8 +229,17 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha
 	if err != nil {
 		acctExec.Status = "failed"
 		acctExec.Error = err.Error()
-		_ = m.config.SavePurchaseExecution(ctx, &acctExec)
-		return false, fmt.Errorf("credential resolution failed for account %s: %w", account.ID, err)
+		credErr := fmt.Errorf("credential resolution failed for account %s: %w", account.ID, err)
+		// The failed row is as much a part of the audit trail as a success
+		// row: if persisting it fails, surface that loss exactly like the
+		// post-purchase save below does instead of silently dropping it.
+		if saveErr := m.config.SavePurchaseExecution(ctx, &acctExec); saveErr != nil {
+			return false, errors.Join(
+				fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, saveErr),
+				credErr,
+			)
+		}
+		return false, credErr
 	}
 
 	accountID := account.ExternalID
diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go
index 6eb8ad38..410fee62 100644
--- a/internal/purchase/execution_test.go
+++ b/internal/purchase/execution_test.go
@@ -663,6 +663,71 @@ func TestExecuteForAccount_CredentialFailure_MarksFailed(t *testing.T) {
 	}
 }
 
+// TestExecuteForAccount_CredentialFailure_SaveErrorSurfaced is the regression
+// guard for COR-10 (#1184): when credential resolution fails AND persisting
+// the per-account "failed" row also fails, the save failure must surface as an
+// AUDIT LOSS error alongside the credential error instead of being silently
+// discarded. The failed row is as much audit trail as a success row.
+func TestExecuteForAccount_CredentialFailure_SaveErrorSurfaced(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockConfigStore)
+	mockEmail := new(MockEmailSender)
+	mockFactory := new(MockProviderFactory)
+
+	// No credentials stored — credential resolution fails for the account.
+	credStore := &MockCredentialStore{
+		LoadRawFn: func(_ context.Context, _, _ string) ([]byte, error) {
+			return nil, nil
+		},
+	}
+
+	// Persisting the per-account "failed" row also fails.
+	saveErr := errors.New("db write failed")
+	mockStore.SavePurchaseExecutionFn = func(_ context.Context, _ *config.PurchaseExecution) error {
+		return saveErr
+	}
+
+	account := config.CloudAccount{
+		ID: "aaaaaaaa-0000-0000-0000-000000000001", Name: "aws-failing",
+		Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys",
+	}
+	plan := &config.PurchasePlan{
+		ID:           "plan-credfail-save",
+		Name:         "Credential Failure Save Error Plan",
+		RampSchedule: config.RampSchedule{CurrentStep: 0, TotalSteps: 1},
+	}
+	baseExec := &config.PurchaseExecution{
+		ExecutionID: "exec-credfail-save",
+		PlanID:      "plan-credfail-save",
+		Status:      "pending",
+		Recommendations: []config.RecommendationRecord{
+			{
+				Provider: "aws", Service: "ec2", ResourceType: "m5.large",
+				Region: "us-east-1", Count: 1, Selected: true,
+			},
+		},
+	}
+
+	manager := &Manager{
+		config:          mockStore,
+		email:           mockEmail,
+		providerFactory: mockFactory,
+		credStore:       credStore,
+	}
+
+	committed, err := manager.executeForAccount(ctx, baseExec, plan, account)
+	assert.False(t, committed)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "AUDIT LOSS: failed to save execution record for account "+account.ID)
+	assert.ErrorIs(t, err, saveErr, "the underlying save error must stay inspectable via errors.Is")
+	assert.Contains(t, err.Error(), "credential resolution failed for account "+account.ID,
+		"the original credential failure must not be masked by the save failure")
+
+	// No provider may ever be constructed when credentials fail to resolve.
+	mockFactory.AssertNotCalled(t, "CreateAndValidateProvider")
+	mockStore.AssertExpectations(t)
+}
+
 // TestExecuteMultiAccount_PartialFailure_IsolatesAccounts is the regression
 // guard for spec E-2: when one account (account-I) has invalid credentials and
 // another (account-V) has valid credentials, account-V's execution must