From 70bd1308c455955d25ea69ffec91065cd7efc41c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 21 May 2026 14:45:24 +0200 Subject: [PATCH 1/2] fix(purchases): record partial success and stamp target account on history Partial-failure runs (some recs commit, others fail) were marked the whole execution "failed" and skipped the success notification despite real commitments written to purchase_history with Purchased=true, inviting a re-approve that double-buys the already-purchased recs (#642). The single-account path also stamped the ambient AWS host account on history regardless of the resolved target account or provider (#646). - #642: introduce a "partially_completed" outcome. When at least one rec committed, never mark the row "failed"; record partially_completed, send the confirmation for the recs that purchased, and surface the row in History (flagged IsAuditGap so its execution-level dollars are excluded - the committed dollars come from the per-rec purchase_history rows). Applies to both the single-account path (via a partialPurchaseError sentinel that finalizeExecution maps) and the multi-account fan-out path. - #642 frontend: on a partial fan-out failure, name the buckets that created pending executions so the still-actionable requests are not silent orphans; add a distinct "Partial" history badge. - #646: resolve the target account's ExternalID from the resolved account and stamp that on purchase_history, falling back to the ambient AWS STS identity only when no target account can be identified. Fixes assume-role AWS and direct-execute Azure/GCP stamping. Regression tests: single-account + multi-account partial success (partially_completed status + notification sent), correct account stamping for AWS and Azure, History row synthesis with dollar-exclusion, and the fan-out partial toast naming submitted buckets. Closes #642 Closes #646 --- .../purchase-execution-toast.test.ts | 34 ++ frontend/src/app.ts | 36 ++- frontend/src/history.ts | 5 + internal/api/handler_history.go | 19 +- internal/api/handler_history_test.go | 45 +++ internal/purchase/execution.go | 154 ++++++++-- internal/purchase/execution_test.go | 290 ++++++++++++++++++ internal/purchase/manager.go | 24 +- 8 files changed, 574 insertions(+), 33 deletions(-) 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..4ad497b01 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,21 +199,36 @@ 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- @@ -161,13 +236,13 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha // // 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) { +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 +250,35 @@ 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 + return nil, "", nil } 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 +481,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..9194f28d3 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1379,3 +1379,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() } } From f9eaf910a84b91512b2046dcbd137b3e2041e701 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 12:04:41 +0200 Subject: [PATCH 2/2] fix(purchases): error when target account is unresolved, never fall back to ambient When a single-account purchase resolves a concrete cloudAccountID but GetCloudAccount returns a nil account (the account does not exist), resolveSingleAccountProvider returned (nil, "", nil). The caller then treated the empty target ID as "truly ambient" and fell back to the host AWS STS identity, purchasing and stamping purchase_history against the wrong account (#646). Surface a descriptive error instead so the caller's no-ambient-fallback contract holds once a target account is known. Add a regression test covering the not-found path. --- internal/purchase/execution.go | 13 +++++++-- internal/purchase/execution_test.go | 43 +++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 4ad497b01..f59244885 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -234,8 +234,10 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha // 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. +// 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 @@ -258,7 +260,12 @@ func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config 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 { diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 9194f28d3..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) {