diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index c5e778425..5439daa20 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -361,6 +361,8 @@ describe('handleFanOutExecute — fan-out path', () => { return { key: `key-${id}`, label: `Bucket ${id}`, + provider: 'aws', + service: `svc-${id}`, recs: [buildMinimalRec()], payment: 'all-upfront', capacityPercent, @@ -414,6 +416,38 @@ describe('handleFanOutExecute — fan-out path', () => { expect(lastToastKind()).toBe('warning'); }); + test('#642 — partial fan-out failure names the submitted (still-actionable) buckets', async () => { + (recs.getFanOutBuckets as jest.Mock).mockReturnValue([ + buildBucket('a'), + buildBucket('b'), + ]); + + // Bucket a submits cleanly (creates a pending execution); bucket b fails. + (api.executePurchase as jest.Mock) + .mockResolvedValueOnce({ + execution_id: 'exec-a', + status: 'queued', + email_sent: true, + approval_recipient: 'alice@example.com', + }) + .mockRejectedValueOnce(new Error('network down')); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + const msg = lastToastMessage(); + expect(msg).toContain('1 of 2'); + expect(msg).toContain('failed'); + // The orphaned-but-actionable pending execution must be surfaced by name so + // the user knows which request is live (issue #642). + expect(msg).toContain('Submitted (awaiting approval)'); + expect(msg).toContain('AWS svc-a@100%'); + // The failed bucket must NOT be listed as submitted. + expect(msg).not.toContain('svc-b'); + expect(lastToastKind()).toBe('warning'); + }); + test('Finding 2 — status === "failed" also counts as failure, recipient excluded', async () => { (recs.getFanOutBuckets as jest.Mock).mockReturnValue([ buildBucket('a'), diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 5407b8b38..f9359ab47 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -398,6 +398,20 @@ async function handleExecutePurchase(): Promise { } } +/** + * fanOutBucketLabel renders a short human identifier for a fan-out bucket so a + * partial-failure toast can name which buckets created pending executions + * (issue #642). Uses the (provider/service @ capacity%) tuple — the same + * fields the user chose in the modal — so the orphaned-but-actionable pending + * requests are identifiable rather than hidden behind a bare count. + */ +function fanOutBucketLabel(b: FanOutBucket): string { + const provider = (b.provider || '').toString().toUpperCase(); + const service = b.service || 'commitment'; + const cap = Number.isFinite(b.capacityPercent) ? `@${b.capacityPercent}%` : ''; + return `${provider} ${service}${cap}`.trim(); +} + /** * handleFanOutExecute submits one executePurchase POST per bucket. * @@ -503,8 +517,28 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise { ] .slice(0, 3) .join('; '); + // #642: on a partial fan-out failure the succeeded buckets created real + // pending executions an approver can still act on. Name them so the user + // knows which requests are live (and which to re-submit) rather than + // leaving them as silent orphans behind a bare count. + let submittedNote = ''; + if (succeeded > 0 && succeeded < results.length) { + const submittedLabels: string[] = []; + results.forEach((r, i) => { + const ok = + r.status === 'fulfilled' && + r.value.email_sent !== false && + r.value.status !== 'failed'; + if (ok && buckets[i]) submittedLabels.push(fanOutBucketLabel(buckets[i])); + }); + if (submittedLabels.length > 0) { + submittedNote = ` Submitted (awaiting approval): ${submittedLabels.slice(0, 5).join(', ')}${ + submittedLabels.length > 5 ? ` +${submittedLabels.length - 5} more` : '' + }.`; + } + } showToast({ - message: `${succeeded} of ${results.length} submitted · ${failed} failed: ${failureMsgs}${failed > 3 ? ' (…)' : ''}`, + message: `${succeeded} of ${results.length} submitted · ${failed} failed: ${failureMsgs}${failed > 3 ? ' (…)' : ''}${submittedNote}`, kind: failed === results.length ? 'error' : 'warning', timeout: null, }); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index f4d878587..9287bde6b 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -277,6 +277,11 @@ function statusBadgeHTML(status: string): string { return 'In Progress'; case 'cancelled': return 'Cancelled'; + case 'partially_completed': + // #642: some commitments succeeded, some failed. Not a clean success + // and never "failed" (real commitments exist) — a distinct warning badge + // so the user knows to read the description and reconcile the failures. + return 'Partial'; case 'failed': return 'Failed'; case 'expired': diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index c7c30dba6..0d7e74d13 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -82,7 +82,14 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR // purchase_history rows (no duplicate). The execution row's PurchaseID is // the ExecutionID while a purchase_history row's is the CommitmentID, so // the keys never collide even when both happen to render. -var historyExecutionStatuses = []string{"pending", "notified", "approved", "running", "paused", "completed", "failed", "expired", "cancelled"} +// +// "partially_completed" (issue #642) is loaded and ALWAYS synthesised: a +// partial run committed some recs to purchase_history (those render from the +// DB rows) and failed others. The synthesised execution row carries the +// partial-failure marker and is flagged IsAuditGap so its execution-level +// dollars are excluded from the dashboard totals — the committed dollars are +// already counted via the per-rec purchase_history rows that succeeded. +var historyExecutionStatuses = []string{"pending", "notified", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", "cancelled"} // approvalExpiryWindow is how long a pending approval stays actionable // before the History view flips it to "expired". Aligns with the @@ -231,6 +238,16 @@ func annotateHistoryRowByStatus(row *config.PurchaseHistoryRecord, exec config.P case "paused": row.Approver = approver row.StatusDescription = "purchase paused — resume or cancel from the plan" + case "partially_completed": + // #642: some recs committed, some failed. The committed recs are + // surfaced via their own purchase_history rows; this synthesised row + // is the audit flag for the failures. Flag IsAuditGap so the dashboard + // excludes its execution-level dollars (the committed dollars are + // counted on the per-rec purchase_history rows, not here) — same + // double-count guard as the audit-gap completed case below. + row.IsAuditGap = true + annotateApproved(row, exec, approver) + row.StatusDescription = "partially completed — some commitments succeeded, others failed: " + exec.Error case "completed": // Only audit-gap completed executions reach here (fetchExecutionsAsHistory // skips clean completed rows). exec.Error carries why the history write diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index abae0f2a4..4e1c14281 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -534,6 +534,51 @@ func TestHandler_getHistory_AuditGapCompletedVisible(t *testing.T) { assert.Equal(t, 0.0, resp.Summary.TotalMonthlySavings) } +// TestHandler_getHistory_PartiallyCompletedVisible is the issue #642 regression +// guard. A "partially_completed" execution (some recs committed, some failed) +// must be surfaced in History as a non-failed row carrying a clear description, +// count as completed (money was committed), and have its execution-level +// dollars excluded via IsAuditGap (the committed dollars are counted on the +// per-rec purchase_history rows that actually saved, not here). +func TestHandler_getHistory_PartiallyCompletedVisible(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + partial := []config.PurchaseExecution{ + { + ExecutionID: "partial-1", + Status: "partially_completed", + Error: "some purchases failed (partial success): c5.xlarge: offering not available", + ScheduledDate: time.Now(), + TotalUpfrontCost: 900.0, + EstimatedSavings: 140.0, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1"}}, + }, + } + approver := "ops@example.com" + mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(partial, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approver}, nil) + + mockAuth, req := adminHistoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.getHistory(ctx, req, map[string]string{}) + require.NoError(t, err) + resp := result.(HistoryResponse) + + require.Len(t, resp.Purchases, 1, "partially_completed execution must be surfaced in History (issue #642)") + row := resp.Purchases[0] + assert.Equal(t, "partial-1", row.PurchaseID) + assert.Equal(t, "partially_completed", row.Status) + assert.NotEqual(t, "failed", row.Status, "a partial run with real commitments must never read as failed (double-spend hazard)") + assert.Contains(t, row.StatusDescription, "partially completed", "the partial outcome must be surfaced to the user") + assert.True(t, row.IsAuditGap, "partial row must carry IsAuditGap so its execution-level dollars are excluded") + assert.Equal(t, 1, resp.Summary.TotalCompleted, "money was committed, so it counts as completed") + assert.Equal(t, 0.0, resp.Summary.TotalUpfront, "partial row must not contribute execution-level dollars (committed dollars come from purchase_history rows)") + assert.Equal(t, 0.0, resp.Summary.TotalMonthlySavings) +} + // TestHandler_getHistory_CompletedDBRowWithDescriptionStillCounts guards the // financial invariant that dollar exclusion keys off the explicit IsAuditGap // marker, NOT off StatusDescription being set. A real purchase_history row diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 1a41e1f85..f59244885 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -63,23 +63,67 @@ func (m *Manager) executePurchase(ctx context.Context, exec *config.PurchaseExec } // Single-account (legacy) path. - accountID := m.getAWSAccountID(ctx) - provCfg, err := m.resolveSingleAccountProvider(ctx, exec) + return false, m.executeSingleAccount(ctx, exec, plan) +} + +// executeSingleAccount runs the legacy single-account purchase path: resolve +// per-account credentials + the target account id, execute the selected recs, +// then notify and classify the outcome. Returns nil on full success, a +// *partialPurchaseError when some recs committed and others failed (#642), or a +// plain error when nothing committed. Split out of executePurchase to keep that +// function under the gocyclo budget. +func (m *Manager) executeSingleAccount(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan) error { + provCfg, targetAccountID, err := m.resolveSingleAccountProvider(ctx, exec) if err != nil { - return false, err + return err + } + // #646: stamp the resolved target account on history, not the ambient + // AWS host account. resolveSingleAccountProvider returns the target's + // ExternalID (the provider-appropriate account identifier for AWS / + // Azure / GCP). Only fall back to the ambient AWS STS identity when no + // target account could be identified (truly-ambient AWS single-account + // execution where credentials are inherited from the host). + accountID := targetAccountID + if accountID == "" { + accountID = m.getAWSAccountID(ctx) } totalSavings, totalUpfront, purchaseErrors := m.processPurchaseRecommendations(ctx, exec, plan, accountID, provCfg) - if len(purchaseErrors) > 0 { - return false, fmt.Errorf("some purchases failed: %v", purchaseErrors) + if len(purchaseErrors) > 0 && !anyRecPurchased(exec.Recommendations) { + // Nothing committed — a clean total failure. + return fmt.Errorf("some purchases failed: %v", purchaseErrors) + } + + // At least one rec committed (full or partial success): the confirmation + // must go out for the recs that purchased. buildPurchaseConfirmationData + // only lists Purchased recs, so a partial run notifies on exactly those. + if notifyErr := m.sendPurchaseNotification(ctx, exec, plan, totalSavings, totalUpfront); notifyErr != nil { + logging.Errorf("Failed to send confirmation: %v", notifyErr) } - if err := m.sendPurchaseNotification(ctx, exec, plan, totalSavings, totalUpfront); err != nil { - logging.Errorf("Failed to send confirmation: %v", err) + if len(purchaseErrors) > 0 { + // #642: partial success. Real commitments were written to + // purchase_history with Purchased=true; never mark the row "failed" + // (it would invite a re-approve that double-buys the purchased recs). + // Return a sentinel so finalizeExecution records "partially_completed". + return &partialPurchaseError{errors: purchaseErrors} } + return nil +} - return false, nil +// anyRecPurchased reports whether at least one recommendation in the slice +// was marked Purchased=true by the aggregator (a real commitment was created +// on the cloud provider). It is the gate that distinguishes a partial success +// (#642 — some recs committed, some failed) from a total failure (nothing +// committed, e.g. credential resolution failed before any rec ran). +func anyRecPurchased(recs []config.RecommendationRecord) bool { + for i := range recs { + if recs[i].Purchased { + return true + } + } + return false } // executeMultiAccount fans out executePurchase across all plan accounts in parallel. @@ -126,10 +170,26 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha accountID := account.ExternalID totalSavings, totalUpfront, purchaseErrors := m.processPurchaseRecommendations(ctx, &acctExec, plan, accountID, provCfg) - if len(purchaseErrors) > 0 { + // #642: a per-account run can be a partial success — some recs committed + // to purchase_history (Purchased=true) while others failed. Marking the + // row "failed" in that case is wrong (it hides the real commitments and + // invites a re-approve that double-buys). Record "partially_completed" + // instead so the row reflects reality, and still send the confirmation + // for the recs that did purchase. + partial := len(purchaseErrors) > 0 && anyRecPurchased(acctExec.Recommendations) + switch { + case partial: + now := time.Now() + acctExec.Status = "partially_completed" + // Append so any audit-gap note already stamped by + // aggregatePurchaseOutcomes (issue #621) survives alongside the + // per-rec failure list. + acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) + acctExec.CompletedAt = &now + case len(purchaseErrors) > 0: acctExec.Status = "failed" - acctExec.Error = strings.Join(purchaseErrors, "; ") - } else { + acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) + default: now := time.Now() acctExec.Status = "completed" acctExec.CompletedAt = &now @@ -139,35 +199,52 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha return fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, err) } - if len(purchaseErrors) > 0 { - return fmt.Errorf("some purchases failed: %v", purchaseErrors) + // Send the confirmation whenever at least one rec committed (full or + // partial success). buildPurchaseConfirmationData only lists Purchased + // recs, so a partial run notifies on exactly the recs that fired. + if len(purchaseErrors) == 0 || partial { + if err := m.sendPurchaseNotification(ctx, &acctExec, plan, totalSavings, totalUpfront); err != nil { + logging.Errorf("Failed to send confirmation for account %s: %v", account.ID, err) + } } - if err := m.sendPurchaseNotification(ctx, &acctExec, plan, totalSavings, totalUpfront); err != nil { - logging.Errorf("Failed to send confirmation for account %s: %v", account.ID, err) + // Surface the per-rec failures to the multi-account aggregator so the + // overall run reflects that not everything succeeded, even though the + // authoritative per-account row was already saved as partially_completed + // (not failed) and the confirmation already went out. + if len(purchaseErrors) > 0 { + return fmt.Errorf("some purchases failed: %v", purchaseErrors) } return nil } // resolveSingleAccountProvider derives per-account credentials for the -// single-account execution path. Returns (nil, nil) when no account can be +// 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 // selected (approval-only flows where credentials are not needed). // +// The second return value is the resolved target account's ExternalID — the +// provider-appropriate account identifier (AWS account number, Azure +// subscription, GCP project) that the caller stamps onto purchase_history +// (#646). It is "" when no target account could be identified, signalling the +// caller to fall back to the ambient AWS STS identity. +// // The account is taken from exec.CloudAccountID when set (plan-with-single- // account executions), or derived from the shared cloud_account_id on the // recommendations when all selected recs agree on exactly one account (direct- // execute purchases where PlanID is empty and exec.CloudAccountID is nil). // -// A non-nil error means credentials were found but could not be resolved; the -// caller must NOT fall back to ambient credentials on error. -func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config.PurchaseExecution) (*provider.ProviderConfig, error) { +// A non-nil error means a target account was identified but could not be +// resolved (lookup failed, the account does not exist, or credentials could +// not be derived); the caller must NOT fall back to ambient credentials on +// error, as that would purchase/stamp against the wrong account (#646). +func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config.PurchaseExecution) (*provider.ProviderConfig, string, error) { // Skip credential resolution when nothing is selected. Approval-only // flows may carry an account ID on the recs solely for the contact-email // gate; attempting resolution there would fail on accounts with no // provider field set and add a pointless DB round-trip. if len(selectedIndices(exec.Recommendations)) == 0 { - return nil, nil + return nil, "", nil } cloudAccountID := exec.CloudAccountID @@ -175,21 +252,40 @@ func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config cloudAccountID = singleCloudAccountIDFromRecs(exec.Recommendations) } if cloudAccountID == nil { - return nil, nil + return nil, "", nil } account, err := m.config.GetCloudAccount(ctx, *cloudAccountID) if err != nil { - return nil, fmt.Errorf("credential resolution failed for account %s: %w", *cloudAccountID, err) + return nil, "", fmt.Errorf("credential resolution failed for account %s: %w", *cloudAccountID, err) } if account == nil { - return nil, nil + // A target account was identified but does not exist in config. + // Returning ("") here would let the caller fall back to ambient AWS + // credentials and purchase/stamp against the wrong account (#646), so + // surface an error instead; the contract above forbids ambient + // fallback once a target account ID is known. + return nil, "", fmt.Errorf("credential resolution failed for account %s: account not found", *cloudAccountID) } provCfg, err := m.resolveAccountProvider(ctx, *account) if err != nil { - return nil, fmt.Errorf("credential resolution failed for account %s: %w", *cloudAccountID, err) + return nil, "", fmt.Errorf("credential resolution failed for account %s: %w", *cloudAccountID, err) } - return provCfg, nil + return provCfg, account.ExternalID, nil +} + +// partialPurchaseError is the sentinel returned by the single-account +// execution path when at least one rec purchased and at least one failed +// (#642). finalizeExecution detects it via errors.As and records the row as +// "partially_completed" rather than "failed", so the real commitments stay +// visible and a re-approve can't double-buy them. The successful recs' +// confirmation email has already been sent by the time this error surfaces. +type partialPurchaseError struct { + errors []string +} + +func (e *partialPurchaseError) Error() string { + return "some purchases failed (partial success): " + strings.Join(e.errors, "; ") } // resolveAccountProvider returns a *ProviderConfig with a pre-authenticated provider @@ -392,11 +488,22 @@ func recordHistoryAuditGap(exec *config.PurchaseExecution, commitmentID string, // status_description. Only a generic, user-safe note reaches the UI. logging.Errorf("history audit gap for commitment %s: %v", commitmentID, histErr) note := fmt.Sprintf("commitment %s purchased but its history record failed to save", commitmentID) - if exec.Error == "" { - exec.Error = note - } else { - exec.Error += "; " + note + exec.Error = appendErrNote(exec.Error, note) +} + +// appendErrNote joins note onto an existing error string with "; ", +// returning note alone when existing is empty. Shared by the audit-gap and +// partial-failure paths so a successful-rec audit gap (#621) and a per-rec +// failure list (#642) within the same execution are both preserved on +// exec.Error rather than one clobbering the other. +func appendErrNote(existing, note string) string { + if existing == "" { + return note + } + if note == "" { + return existing } + return existing + "; " + note } // normalizePurchaseSource canonicalizes exec.Source for downstream tag diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 74743d6a4..f2b8a96de 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1241,6 +1241,49 @@ func TestExecutePurchase_SingleAccount_CredResolutionError(t *testing.T) { mockStore.AssertExpectations(t) } +// TestExecutePurchase_SingleAccount_AccountNotFound asserts that when a target +// account ID is known but GetCloudAccount returns (nil, nil) (the account does +// not exist), execution surfaces an error rather than silently falling back to +// ambient credentials and purchasing/stamping against the wrong account (#646). +func TestExecutePurchase_SingleAccount_AccountNotFound(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + acctID := "missing-acct" + exec := &config.PurchaseExecution{ + ExecutionID: "exec-acct-missing", + PlanID: "", + Recommendations: []config.RecommendationRecord{ + { + Provider: "azure", + Service: "compute", + ResourceType: "Standard_B1ls", + Region: "eastus", + Count: 1, + Selected: true, + CloudAccountID: &acctID, + }, + }, + } + + // GetCloudAccount returns no error but a nil account (not found). + mockStore.On("GetCloudAccount", ctx, acctID).Return(nil, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + _, err := manager.executePurchase(ctx, exec) + require.Error(t, err) + assert.Contains(t, err.Error(), "credential resolution failed for account "+acctID) + assert.Contains(t, err.Error(), "account not found") + + mockStore.AssertExpectations(t) +} + // TestSingleCloudAccountIDFromRecs covers the helper that derives a shared // cloud_account_id from a recommendation slice. func TestSingleCloudAccountIDFromRecs(t *testing.T) { @@ -1379,3 +1422,293 @@ func TestManager_ExecuteAndFinalize_HistorySaveFailure_StaysVisible(t *testing.T mockStore.AssertExpectations(t) mockFactory.AssertExpectations(t) } + +// TestManager_ExecuteAndFinalize_SingleAccount_PartialSuccess is the issue #642 +// regression guard for the single-account path. A two-rec direct purchase where +// rec A succeeds and rec B fails must: +// - NOT be marked "failed" (rec A committed a real purchase; failing the row +// invites a re-approve that double-buys A). +// - be recorded as "partially_completed". +// - still SEND the confirmation email (for the rec that did purchase). +// - persist a purchase_history row only for the succeeded rec. +func TestManager_ExecuteAndFinalize_SingleAccount_PartialSuccess(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockSTS := new(MockSTSClient) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + exec := &config.PurchaseExecution{ + ExecutionID: "exec-partial-single", + PlanID: "", + StepNumber: 1, + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300.0, Savings: 50.0, Selected: true}, + {Provider: "aws", Service: "ec2", ResourceType: "c5.xlarge", Region: "us-east-1", Count: 1, UpfrontCost: 600.0, Savings: 90.0, Selected: true}, + }, + } + + // History row must be saved ONLY for the rec that succeeded (m5.large). + mockStore.On("SavePurchaseHistory", ctx, mock.MatchedBy(func(r *config.PurchaseHistoryRecord) bool { + return r.ResourceType == "m5.large" + })).Return(nil).Once() + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + // Confirmation must still go out (the issue's core complaint). + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil).Once() + // Ambient STS only consulted as a fallback (no account resolved here). + mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{ + Account: aws.String("123456789012"), + }, nil).Maybe() + + mockFactory.On("CreateAndValidateProvider", ctx, "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", ctx, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + // m5.large succeeds; c5.xlarge fails. + mockServiceClient.On("PurchaseCommitment", ctx, + mock.MatchedBy(func(r common.Recommendation) bool { return r.ResourceType == "m5.large" }), + mock.AnythingOfType("common.PurchaseOptions"), + ).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil).Once() + mockServiceClient.On("PurchaseCommitment", ctx, + mock.MatchedBy(func(r common.Recommendation) bool { return r.ResourceType == "c5.xlarge" }), + mock.AnythingOfType("common.PurchaseOptions"), + ).Return(common.PurchaseResult{}, errors.New("offering not available")).Once() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + stsClient: mockSTS, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.executeAndFinalize(ctx, exec) + require.Error(t, err, "a partial failure must surface an error so the run isn't reported as a clean success") + + assert.Equal(t, "partially_completed", exec.Status, "partial success must never be 'failed' (double-spend hazard on the committed rec)") + require.NotNil(t, exec.CompletedAt, "a partial run is terminal for its committed recs") + assert.NotEmpty(t, exec.Error, "the per-rec failure must be recorded on the row") + assert.Contains(t, exec.Error, "offering not available") + + // The succeeded rec must be flagged Purchased; the failed one carries its error. + assert.True(t, exec.Recommendations[0].Purchased, "m5.large committed") + assert.Equal(t, "ri-ok", exec.Recommendations[0].PurchaseID) + assert.False(t, exec.Recommendations[1].Purchased, "c5.xlarge did not commit") + assert.NotEmpty(t, exec.Recommendations[1].Error) + + mockStore.AssertExpectations(t) + mockEmail.AssertExpectations(t) +} + +// TestExecuteForAccount_PartialSuccess is the issue #642 regression guard for +// the multi-account fan-out path: within a single account, one rec commits and +// another fails. The per-account execution record must be saved as +// "partially_completed" (NOT "failed"), carry the failure in Error, and the +// confirmation must still go out for the committed rec. +func TestExecuteForAccount_PartialSuccess(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + const acctID = "aaaaaaaa-0000-0000-0000-000000000001" + account := config.CloudAccount{ID: acctID, Name: "Prod", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"} + + baseExec := &config.PurchaseExecution{ + ExecutionID: "exec-base", + PlanID: "plan-x", + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300.0, Savings: 50.0, Selected: true}, + {Provider: "aws", Service: "ec2", ResourceType: "c5.xlarge", Region: "us-east-1", Count: 1, UpfrontCost: 600.0, Savings: 90.0, Selected: true}, + }, + } + plan := &config.PurchasePlan{ID: "plan-x", Name: "Plan X"} + + var savedExec *config.PurchaseExecution + mockStore.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + c := *e + savedExec = &c + return nil + } + mockStore.On("SavePurchaseHistory", ctx, mock.MatchedBy(func(r *config.PurchaseHistoryRecord) bool { + return r.ResourceType == "m5.large" + })).Return(nil).Once() + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil).Once() + + credStore := &MockCredentialStore{ + LoadRawFn: func(_ context.Context, _, _ string) ([]byte, error) { + return []byte(`{"access_key_id":"AKIAIOSFODNN7EXAMPLE","secret_access_key":"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"}`), nil + }, + } + + mockFactory.On("CreateAndValidateProvider", ctx, "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", ctx, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) + mockServiceClient.On("PurchaseCommitment", ctx, + mock.MatchedBy(func(r common.Recommendation) bool { return r.ResourceType == "m5.large" }), + mock.AnythingOfType("common.PurchaseOptions"), + ).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil).Once() + mockServiceClient.On("PurchaseCommitment", ctx, + mock.MatchedBy(func(r common.Recommendation) bool { return r.ResourceType == "c5.xlarge" }), + mock.AnythingOfType("common.PurchaseOptions"), + ).Return(common.PurchaseResult{}, errors.New("offering not available")).Once() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + credStore: credStore, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.executeForAccount(ctx, baseExec, plan, account) + require.Error(t, err, "the per-rec failure must surface to the aggregator") + + require.NotNil(t, savedExec, "per-account record must be saved") + assert.Equal(t, "partially_completed", savedExec.Status, "partial success must never be 'failed' (double-spend hazard)") + require.NotNil(t, savedExec.CompletedAt) + assert.Contains(t, savedExec.Error, "offering not available") + + mockStore.AssertExpectations(t) + mockEmail.AssertExpectations(t) +} + +// TestManager_ExecutePurchase_SingleAccount_StampsTargetAccount is the issue +// #646 regression guard: the single-account path must stamp the resolved +// target account's ExternalID on purchase_history, not the ambient AWS host +// account. Covers a managed AWS account (ExternalID is the target account +// number, distinct from the host STS identity) and a direct-execute Azure +// account (must NOT inherit the AWS host account or "unknown"). The fix uses +// account.ExternalID regardless of auth mode, so access_keys exercises the +// same single-account stamping path the assume-role case would. +func TestManager_ExecutePurchase_SingleAccount_StampsTargetAccount(t *testing.T) { + const hostAccount = "999999999999" // ambient AWS STS identity — must NOT be stamped + + cases := []struct { + name string + provider string + externalID string + service string + expectedType common.ServiceType + region string + resource string + }{ + { + name: "assume-role AWS stamps target ExternalID not host", + provider: "aws", + externalID: "111111111111", + service: "ec2", + expectedType: common.ServiceEC2, + region: "us-east-1", + resource: "m5.large", + }, + { + name: "direct-execute Azure stamps subscription not AWS host", + provider: "azure", + externalID: "azure-sub-222", + service: "compute", + expectedType: common.ServiceCompute, + region: "eastus", + resource: "Standard_D2s_v3", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockSTS := new(MockSTSClient) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + mockCredStore := &MockCredentialStore{ + LoadRawFn: func(_ context.Context, _, _ string) ([]byte, error) { + // AWS access_keys path reads stored key JSON; Azure + // managed_identity never reaches here. + if tc.provider == "aws" { + return []byte(`{"access_key_id":"AKIAIOSFODNN7EXAMPLE","secret_access_key":"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"}`), nil + } + return nil, nil + }, + } + + acctID := "acct-" + tc.name + exec := &config.PurchaseExecution{ + ExecutionID: "exec-stamp-" + tc.name, + PlanID: "", + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{ + { + Provider: tc.provider, + Service: tc.service, + ResourceType: tc.resource, + Region: tc.region, + Count: 1, + UpfrontCost: 100.0, + Savings: 20.0, + Selected: true, + CloudAccountID: &acctID, + }, + }, + } + + account := &config.CloudAccount{ + ID: acctID, + Provider: tc.provider, + ExternalID: tc.externalID, + } + switch tc.provider { + case "aws": + account.AWSAuthMode = "access_keys" + case "azure": + account.AzureAuthMode = "managed_identity" + account.AzureSubscriptionID = "sub-internal" + } + + mockStore.On("GetCloudAccount", ctx, acctID).Return(account, nil) + + // Capture the stamped AccountID on the history row. + var stampedAccount string + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")). + Run(func(args mock.Arguments) { + stampedAccount = args.Get(1).(*config.PurchaseHistoryRecord).AccountID + }).Return(nil).Once() + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + // Ambient STS — if the host account leaks onto history this is where + // it would come from; it must NOT be the stamped value. + mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")). + Return(&sts.GetCallerIdentityOutput{Account: aws.String(hostAccount)}, nil).Maybe() + + mockFactory.On("CreateAndValidateProvider", ctx, tc.provider, mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", ctx, tc.expectedType, tc.region).Return(mockServiceClient, nil) + mockServiceClient.On("PurchaseCommitment", ctx, + mock.AnythingOfType("common.Recommendation"), + mock.AnythingOfType("common.PurchaseOptions"), + ).Return(common.PurchaseResult{Success: true, CommitmentID: "commit-1"}, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + stsClient: mockSTS, + providerFactory: mockFactory, + credStore: mockCredStore, + dashboardURL: "https://dashboard.example.com", + } + + _, err := manager.executePurchase(ctx, exec) + require.NoError(t, err) + + assert.Equal(t, tc.externalID, stampedAccount, + "history must be stamped with the target account ExternalID (#646)") + assert.NotEqual(t, hostAccount, stampedAccount, + "history must NOT be stamped with the ambient AWS host account") + assert.NotEqual(t, "unknown", stampedAccount) + + mockStore.AssertExpectations(t) + }) + } +} diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 11d919773..c814fa038 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -3,6 +3,7 @@ package purchase import ( "context" + "errors" "fmt" "time" @@ -126,13 +127,28 @@ func NewManager(cfg ManagerConfig) *Manager { // finalizeExecution sets the status and completion time on an execution based on the error. func (m *Manager) finalizeExecution(exec *config.PurchaseExecution, execErr error) { - if execErr != nil { - exec.Status = "failed" - exec.Error = execErr.Error() - } else { + var partial *partialPurchaseError + switch { + case execErr == nil: completedAt := time.Now() exec.Status = "completed" exec.CompletedAt = &completedAt + case errors.As(execErr, &partial): + // #642: at least one rec committed a real purchase while others + // failed. Never mark such a row "failed" — the commitments are real + // and a re-approve would double-buy them. Record the partial outcome + // and stamp CompletedAt so the row reads as terminal (the successful + // recs are done), with the per-rec failures preserved in Error. + // Append rather than overwrite so any audit-gap note already stamped + // by aggregatePurchaseOutcomes (a successful rec whose history write + // failed, issue #621) is not lost. + completedAt := time.Now() + exec.Status = "partially_completed" + exec.Error = appendErrNote(exec.Error, execErr.Error()) + exec.CompletedAt = &completedAt + default: + exec.Status = "failed" + exec.Error = execErr.Error() } }