diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index a5a43b89..bfe0306d 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -229,8 +229,12 @@ 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) + // Audit-trail save: the failed row is as much a part of the audit + // trail as a success row, so a save failure must surface (AUDIT LOSS) + // instead of being silently dropped. Extracted to a helper to keep + // executeForAccount under the gocyclo:10 budget. + return false, m.persistFailedAccountExecution(ctx, &acctExec, account, credErr) } accountID := account.ExternalID @@ -288,6 +292,22 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha return committed, nil } +// persistFailedAccountExecution writes a per-account execution row that the +// caller has already stamped with Status="failed" + Error, and joins any save +// failure with the originating cause via errors.Join so neither error is +// silently dropped (the row is as much audit trail as a success row; issue +// #1184 / COR-10). Extracted from executeForAccount so the new branch keeps +// that function under the gocyclo:10 budget. +func (m *Manager) persistFailedAccountExecution(ctx context.Context, acctExec *config.PurchaseExecution, account config.CloudAccount, cause error) error { + if saveErr := m.config.SavePurchaseExecution(ctx, acctExec); saveErr != nil { + return errors.Join( + fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, saveErr), + cause, + ) + } + return cause +} + // resolveSingleAccountProvider derives per-account credentials for the // single-account execution path. Returns (nil, "", nil) when no account can be // identified (ambient credentials are used in that case), or when no recs are 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