Skip to content

Commit 437190c

Browse files
authored
fix(purchases): record partial success and stamp target account on history (closes #642, #646) (#650)
* 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 * 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.
1 parent 1e751af commit 437190c

8 files changed

Lines changed: 626 additions & 35 deletions

File tree

‎frontend/src/__tests__/purchase-execution-toast.test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,8 @@ describe('handleFanOutExecute — fan-out path', () => {
361361
return {
362362
key: `key-${id}`,
363363
label: `Bucket ${id}`,
364+
provider: 'aws',
365+
service: `svc-${id}`,
364366
recs: [buildMinimalRec()],
365367
payment: 'all-upfront',
366368
capacityPercent,
@@ -414,6 +416,38 @@ describe('handleFanOutExecute — fan-out path', () => {
414416
expect(lastToastKind()).toBe('warning');
415417
});
416418

419+
test('#642 — partial fan-out failure names the submitted (still-actionable) buckets', async () => {
420+
(recs.getFanOutBuckets as jest.Mock).mockReturnValue([
421+
buildBucket('a'),
422+
buildBucket('b'),
423+
]);
424+
425+
// Bucket a submits cleanly (creates a pending execution); bucket b fails.
426+
(api.executePurchase as jest.Mock)
427+
.mockResolvedValueOnce({
428+
execution_id: 'exec-a',
429+
status: 'queued',
430+
email_sent: true,
431+
approval_recipient: 'alice@example.com',
432+
})
433+
.mockRejectedValueOnce(new Error('network down'));
434+
435+
const btn = setup();
436+
btn.click();
437+
await new Promise((r) => setTimeout(r, 0));
438+
439+
const msg = lastToastMessage();
440+
expect(msg).toContain('1 of 2');
441+
expect(msg).toContain('failed');
442+
// The orphaned-but-actionable pending execution must be surfaced by name so
443+
// the user knows which request is live (issue #642).
444+
expect(msg).toContain('Submitted (awaiting approval)');
445+
expect(msg).toContain('AWS svc-a@100%');
446+
// The failed bucket must NOT be listed as submitted.
447+
expect(msg).not.toContain('svc-b');
448+
expect(lastToastKind()).toBe('warning');
449+
});
450+
417451
test('Finding 2 — status === "failed" also counts as failure, recipient excluded', async () => {
418452
(recs.getFanOutBuckets as jest.Mock).mockReturnValue([
419453
buildBucket('a'),

‎frontend/src/app.ts‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,20 @@ async function handleExecutePurchase(): Promise<void> {
398398
}
399399
}
400400

401+
/**
402+
* fanOutBucketLabel renders a short human identifier for a fan-out bucket so a
403+
* partial-failure toast can name which buckets created pending executions
404+
* (issue #642). Uses the (provider/service @ capacity%) tuple — the same
405+
* fields the user chose in the modal — so the orphaned-but-actionable pending
406+
* requests are identifiable rather than hidden behind a bare count.
407+
*/
408+
function fanOutBucketLabel(b: FanOutBucket): string {
409+
const provider = (b.provider || '').toString().toUpperCase();
410+
const service = b.service || 'commitment';
411+
const cap = Number.isFinite(b.capacityPercent) ? `@${b.capacityPercent}%` : '';
412+
return `${provider} ${service}${cap}`.trim();
413+
}
414+
401415
/**
402416
* handleFanOutExecute submits one executePurchase POST per bucket.
403417
*
@@ -503,8 +517,28 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise<void> {
503517
]
504518
.slice(0, 3)
505519
.join('; ');
520+
// #642: on a partial fan-out failure the succeeded buckets created real
521+
// pending executions an approver can still act on. Name them so the user
522+
// knows which requests are live (and which to re-submit) rather than
523+
// leaving them as silent orphans behind a bare count.
524+
let submittedNote = '';
525+
if (succeeded > 0 && succeeded < results.length) {
526+
const submittedLabels: string[] = [];
527+
results.forEach((r, i) => {
528+
const ok =
529+
r.status === 'fulfilled' &&
530+
r.value.email_sent !== false &&
531+
r.value.status !== 'failed';
532+
if (ok && buckets[i]) submittedLabels.push(fanOutBucketLabel(buckets[i]));
533+
});
534+
if (submittedLabels.length > 0) {
535+
submittedNote = ` Submitted (awaiting approval): ${submittedLabels.slice(0, 5).join(', ')}${
536+
submittedLabels.length > 5 ? ` +${submittedLabels.length - 5} more` : ''
537+
}.`;
538+
}
539+
}
506540
showToast({
507-
message: `${succeeded} of ${results.length} submitted · ${failed} failed: ${failureMsgs}${failed > 3 ? ' (…)' : ''}`,
541+
message: `${succeeded} of ${results.length} submitted · ${failed} failed: ${failureMsgs}${failed > 3 ? ' (…)' : ''}${submittedNote}`,
508542
kind: failed === results.length ? 'error' : 'warning',
509543
timeout: null,
510544
});

‎frontend/src/history.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,11 @@ function statusBadgeHTML(status: string): string {
277277
return '<span class="badge badge-warning">In Progress</span>';
278278
case 'cancelled':
279279
return '<span class="badge badge-muted">Cancelled</span>';
280+
case 'partially_completed':
281+
// #642: some commitments succeeded, some failed. Not a clean success
282+
// and never "failed" (real commitments exist) — a distinct warning badge
283+
// so the user knows to read the description and reconcile the failures.
284+
return '<span class="badge badge-warning">Partial</span>';
280285
case 'failed':
281286
return '<span class="badge badge-danger">Failed</span>';
282287
case 'expired':

‎internal/api/handler_history.go‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,14 @@ func (h *Handler) getHistory(ctx context.Context, req *events.LambdaFunctionURLR
8282
// purchase_history rows (no duplicate). The execution row's PurchaseID is
8383
// the ExecutionID while a purchase_history row's is the CommitmentID, so
8484
// the keys never collide even when both happen to render.
85-
var historyExecutionStatuses = []string{"pending", "notified", "approved", "running", "paused", "completed", "failed", "expired", "cancelled"}
85+
//
86+
// "partially_completed" (issue #642) is loaded and ALWAYS synthesised: a
87+
// partial run committed some recs to purchase_history (those render from the
88+
// DB rows) and failed others. The synthesised execution row carries the
89+
// partial-failure marker and is flagged IsAuditGap so its execution-level
90+
// dollars are excluded from the dashboard totals — the committed dollars are
91+
// already counted via the per-rec purchase_history rows that succeeded.
92+
var historyExecutionStatuses = []string{"pending", "notified", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", "cancelled"}
8693

8794
// approvalExpiryWindow is how long a pending approval stays actionable
8895
// before the History view flips it to "expired". Aligns with the
@@ -231,6 +238,16 @@ func annotateHistoryRowByStatus(row *config.PurchaseHistoryRecord, exec config.P
231238
case "paused":
232239
row.Approver = approver
233240
row.StatusDescription = "purchase paused — resume or cancel from the plan"
241+
case "partially_completed":
242+
// #642: some recs committed, some failed. The committed recs are
243+
// surfaced via their own purchase_history rows; this synthesised row
244+
// is the audit flag for the failures. Flag IsAuditGap so the dashboard
245+
// excludes its execution-level dollars (the committed dollars are
246+
// counted on the per-rec purchase_history rows, not here) — same
247+
// double-count guard as the audit-gap completed case below.
248+
row.IsAuditGap = true
249+
annotateApproved(row, exec, approver)
250+
row.StatusDescription = "partially completed — some commitments succeeded, others failed: " + exec.Error
234251
case "completed":
235252
// Only audit-gap completed executions reach here (fetchExecutionsAsHistory
236253
// skips clean completed rows). exec.Error carries why the history write

‎internal/api/handler_history_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,51 @@ func TestHandler_getHistory_AuditGapCompletedVisible(t *testing.T) {
534534
assert.Equal(t, 0.0, resp.Summary.TotalMonthlySavings)
535535
}
536536

537+
// TestHandler_getHistory_PartiallyCompletedVisible is the issue #642 regression
538+
// guard. A "partially_completed" execution (some recs committed, some failed)
539+
// must be surfaced in History as a non-failed row carrying a clear description,
540+
// count as completed (money was committed), and have its execution-level
541+
// dollars excluded via IsAuditGap (the committed dollars are counted on the
542+
// per-rec purchase_history rows that actually saved, not here).
543+
func TestHandler_getHistory_PartiallyCompletedVisible(t *testing.T) {
544+
ctx := context.Background()
545+
mockStore := new(MockConfigStore)
546+
547+
partial := []config.PurchaseExecution{
548+
{
549+
ExecutionID: "partial-1",
550+
Status: "partially_completed",
551+
Error: "some purchases failed (partial success): c5.xlarge: offering not available",
552+
ScheduledDate: time.Now(),
553+
TotalUpfrontCost: 900.0,
554+
EstimatedSavings: 140.0,
555+
Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2", Region: "us-east-1"}},
556+
},
557+
}
558+
approver := "ops@example.com"
559+
mockStore.On("GetAllPurchaseHistory", ctx, 100).Return([]config.PurchaseHistoryRecord{}, nil)
560+
mockStore.On("GetExecutionsByStatuses", ctx, mock.Anything, mock.Anything).Return(partial, nil)
561+
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &approver}, nil)
562+
563+
mockAuth, req := adminHistoryReq(ctx)
564+
handler := &Handler{auth: mockAuth, config: mockStore}
565+
566+
result, err := handler.getHistory(ctx, req, map[string]string{})
567+
require.NoError(t, err)
568+
resp := result.(HistoryResponse)
569+
570+
require.Len(t, resp.Purchases, 1, "partially_completed execution must be surfaced in History (issue #642)")
571+
row := resp.Purchases[0]
572+
assert.Equal(t, "partial-1", row.PurchaseID)
573+
assert.Equal(t, "partially_completed", row.Status)
574+
assert.NotEqual(t, "failed", row.Status, "a partial run with real commitments must never read as failed (double-spend hazard)")
575+
assert.Contains(t, row.StatusDescription, "partially completed", "the partial outcome must be surfaced to the user")
576+
assert.True(t, row.IsAuditGap, "partial row must carry IsAuditGap so its execution-level dollars are excluded")
577+
assert.Equal(t, 1, resp.Summary.TotalCompleted, "money was committed, so it counts as completed")
578+
assert.Equal(t, 0.0, resp.Summary.TotalUpfront, "partial row must not contribute execution-level dollars (committed dollars come from purchase_history rows)")
579+
assert.Equal(t, 0.0, resp.Summary.TotalMonthlySavings)
580+
}
581+
537582
// TestHandler_getHistory_CompletedDBRowWithDescriptionStillCounts guards the
538583
// financial invariant that dollar exclusion keys off the explicit IsAuditGap
539584
// marker, NOT off StatusDescription being set. A real purchase_history row

0 commit comments

Comments
 (0)