From 00c63e50a1c6a8a1ff0a3ca8b08aacb932dc6390 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:30:57 +0000 Subject: [PATCH 1/2] fix(purchase): mint a revocation token and send the executed email for partial runs A run that ended partially_completed bought part of its commitments, but the approve funnel returned before minting the revocation token, so the row could not be revoked by link and nobody got the executed email. transitionApproveAndExecute now mints the token for a saved partially_completed run too (IsCommittedPartialRun: partial status and no audit loss) and returns it with the run's error. Session approve, direct execute and run-now send the executed email for such a run before the 502, and the 502 message says the run partially completed. The token approve emails it when ApproveExecution returns a token with its error; its status mapping is unchanged. The executed email marks a partial run and lists only the commitments it bought when the row records them. Refs #532 Co-Authored-By: Claude Opus 5.5 --- .../api/executed_notification_partial_test.go | 223 ++++++++++++++++++ internal/api/handler_purchases.go | 118 ++++++--- ...dler_purchases_run_now_integration_test.go | 36 +++ internal/api/openapi.yaml | 8 +- internal/email/executed_notification_test.go | 28 +++ internal/email/sender.go | 4 + internal/email/templates.go | 21 +- internal/purchase/approvals.go | 49 +++- ...partial_run_revocation_integration_test.go | 67 ++++++ 9 files changed, 504 insertions(+), 50 deletions(-) create mode 100644 internal/api/executed_notification_partial_test.go create mode 100644 internal/purchase/partial_run_revocation_integration_test.go diff --git a/internal/api/executed_notification_partial_test.go b/internal/api/executed_notification_partial_test.go new file mode 100644 index 000000000..17ccde409 --- /dev/null +++ b/internal/api/executed_notification_partial_test.go @@ -0,0 +1,223 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #532: a partially_completed run bought part of its commitments, so it +// is revocable. The funnel mints a revocation token for it and the handler +// emails it like a completed run, then still answers 502 with the row status. + +const partialRunToken = "partial-revoke-token" + +var errPartialRun = errors.New("some purchases failed: m5.large: provider rejected the purchase") + +// partialRunRow is the row the funnel returns for a single-account partial +// run: one rec bought, one failed, completed_at stamped, the token's hash +// stored. +func partialRunRow(execID string) *config.PurchaseExecution { + accountID := "acct-1" + completed := time.Now().Add(-time.Minute) + return &config.PurchaseExecution{ + ExecutionID: execID, + Status: config.StatusPartiallyCompleted, + CompletedAt: &completed, + ApprovalToken: config.HashApprovalToken(partialRunToken), + Recommendations: []config.RecommendationRecord{ + {ID: "r1", Provider: "aws", Service: "ec2", ResourceType: "m5.xlarge", Region: "us-east-1", CloudAccountID: &accountID, UpfrontCost: 100, Savings: 10, Purchased: true}, + {ID: "r2", Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", CloudAccountID: &accountID, UpfrontCost: 50, Savings: 5}, + }, + TotalUpfrontCost: 150, + EstimatedSavings: 15, + } +} + +// assertPartialRunEmailed pins the executed email of a partial run: sent once, +// carrying the minted token (valid against the stored hash), marked partial, +// and listing only the commitment that was bought, with its own totals. +func assertPartialRunEmailed(t *testing.T, n *recordingExecutedNotifier, row *config.PurchaseExecution) { + t.Helper() + require.Equal(t, 1, n.calls, "a partial run must send the executed email") + assert.Equal(t, partialRunToken, n.captured.RevocationToken) + assert.NoError(t, validateRevokeToken(row, n.captured.RevocationToken), "the emailed token must revoke the row") + assert.True(t, n.captured.PartialPurchase) + require.Len(t, n.captured.Recommendations, 1, "only the bought commitment is listed") + assert.Equal(t, "m5.xlarge", n.captured.Recommendations[0].ResourceType) + assert.InDelta(t, 100.0, n.captured.TotalUpfrontCost, 0.001) + assert.InDelta(t, 10.0, n.captured.TotalSavings, 0.001) + assert.NotEmpty(t, n.captured.RevocationWindowClosesAt) +} + +func assertPartialRun502(t *testing.T, err error) { + t.Helper() + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %v", err) + assert.Equal(t, 502, ce.code) + assert.Contains(t, ce.message, "partially completed") + assert.Equal(t, config.StatusPartiallyCompleted, ce.Details()["status"]) +} + +func sessionApproveHandler(ctx context.Context, execID, contact string, final *config.PurchaseExecution, token string, runErr error) (*Handler, *recordingExecutedNotifier) { + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &contact}, nil) + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: "admin@example.com"}, nil) + mockAuth.grantAdminPurchaser() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveAndExecute", ctx, execID, "admin@example.com", (*string)(nil)).Return(final, token, runErr) + notifier := &recordingExecutedNotifier{} + return &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, emailNotifier: notifier}, notifier +} + +func TestExecutedNotification_PartialSessionApproveEmailsRevocationToken(t *testing.T) { + ctx := context.Background() + execID := "56789012-5678-5678-5678-56789012abcd" + row := partialRunRow(execID) + handler, notifier := sessionApproveHandler(ctx, execID, "contact@example.com", row, partialRunToken, errPartialRun) + + _, err := handler.approvePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}, execID, "") + assertPartialRun502(t, err) + assertPartialRunEmailed(t, notifier, row) +} + +// Nothing committed (failed), or the partial status never reached the store +// (audit loss): no revocable purchase exists, so no executed email. +func TestExecutedNotification_UncommittedFailureSendsNoEmail(t *testing.T) { + ctx := context.Background() + execID := "67890123-6789-6789-6789-67890123abcd" + failed := partialRunRow(execID) + failed.Status = "failed" + cases := map[string]struct { + final *config.PurchaseExecution + err error + }{ + "failed run": {failed, errors.New("AWS RI purchase failed")}, + "partial audit loss": {partialRunRow(execID), fmt.Errorf("%w: terminal save failed; original execution error: %w", config.ErrAuditLoss, errPartialRun)}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + handler, notifier := sessionApproveHandler(ctx, execID, "contact@example.com", tc.final, "", tc.err) + _, err := handler.approvePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}, execID, "") + require.Error(t, err) + assert.Zero(t, notifier.calls) + }) + } +} + +func TestExecutedNotification_PartialDirectExecuteEmailsRevocationToken(t *testing.T) { + ctx := context.Background() + execID := "78901234-7890-7890-7890-78901234abcd" + contact := "contact@example.com" + row := partialRunRow(execID) + + mockConfig := new(MockConfigStore) + mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contact}, nil + } + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &contact}, nil) + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("DirectExecute", ctx, execID, "admin@example.com", (*string)(nil)).Return(row, partialRunToken, errPartialRun) + notifier := &recordingExecutedNotifier{} + handler := &Handler{purchase: mockPurchase, config: mockConfig, emailNotifier: notifier} + + pending := &config.PurchaseExecution{ExecutionID: execID, Status: "pending", Recommendations: row.Recommendations} + _, err := handler.directExecutePurchase(ctx, &events.LambdaFunctionURLRequest{}, pending, &Session{Email: "admin@example.com", UserID: "admin-uid"}, nil) + assertPartialRun502(t, err) + assertPartialRunEmailed(t, notifier, row) +} + +// The token approve returns no row yet, so a token alongside the error is what +// marks a committed partial run; the email comes from the re-fetched row. The +// status mapping of this path is unchanged here. +func TestExecutedNotification_PartialTokenApproveEmailsRevocationToken(t *testing.T) { + ctx := context.Background() + execID := "89012345-8901-8901-8901-89012345abcd" + contact := "contact@example.com" + row := partialRunRow(execID) + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil).Once() + mockConfig.On("GetExecutionByID", ctx, execID).Return(row, nil).Once() + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &contact}, nil) + mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return(partialRunToken, errPartialRun) + notifier := &recordingExecutedNotifier{} + handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, emailNotifier: notifier} + + _, err := handler.approvePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}, execID, "valid-token") + require.ErrorIs(t, err, errPartialRun) + assertPartialRunEmailed(t, notifier, row) + mockConfig.AssertExpectations(t) +} + +func TestExecutedNotification_TokenApproveFailureWithoutTokenSendsNoEmail(t *testing.T) { + ctx := context.Background() + execID := "90123456-9012-9012-9012-90123456abcd" + contact := "contact@example.com" + + mockConfig := new(MockConfigStore) + exec := approvalTestExec(execID, contact, mockConfig) + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil).Once() + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{NotificationEmail: &contact}, nil) + mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() + mockPurchase := new(MockPurchaseManager) + mockPurchase.On("ApproveExecution", ctx, execID, "valid-token", contact).Return("", errors.New("AWS RI purchase failed")) + notifier := &recordingExecutedNotifier{} + handler := &Handler{purchase: mockPurchase, config: mockConfig, auth: mockAuth, emailNotifier: notifier} + + _, err := handler.approvePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}, execID, "valid-token") + require.Error(t, err) + assert.Zero(t, notifier.calls) + mockConfig.AssertExpectations(t) +} + +// A fan-out root records no per-rec Purchased flags (its per-account rows +// do), so its partial email lists every rec with the row totals; the partial +// notice carries the caveat. +func TestExecutedEmailCommitments(t *testing.T) { + row := partialRunRow("e1") + recs, upfront, savings := executedEmailCommitments(row) + assert.Len(t, recs, 1) + assert.InDelta(t, 100.0, upfront, 0.001) + assert.InDelta(t, 10.0, savings, 0.001) + + root := partialRunRow("e2") + for i := range root.Recommendations { + root.Recommendations[i].Purchased = false + } + recs, upfront, savings = executedEmailCommitments(root) + assert.Len(t, recs, 2) + assert.InDelta(t, 150.0, upfront, 0.001) + assert.InDelta(t, 15.0, savings, 0.001) + + completed := partialRunRow("e3") + completed.Status = config.StatusCompleted + recs, upfront, _ = executedEmailCommitments(completed) + assert.Len(t, recs, 2, "a completed row lists every rec, as before") + assert.InDelta(t, 150.0, upfront, 0.001) +} diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index ce5212ded..c62e4fa62 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -446,7 +446,7 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc // claim returns, the row it buys (issue #415). final, revocationToken, runErr := h.purchase.RunPlannedPurchaseNow(ctx, executionID, fourEyesActorIdentity(session), resolveCreatorUserID(session), h.runNowClaimCheck(session)) if runErr != nil { - return nil, executeFailureError(executionID, "started", final, runErr) + return nil, h.executeFailure(ctx, req, executionID, "started", final, revocationToken, session.Email, runErr) } // The shared execute funnel rotated the row's token into a revocation @@ -461,6 +461,18 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc }, nil } +// executeFailure answers a failed run-now, session approve or direct execute. +// A partially_completed run bought part of its commitments, so it gets the +// executed email with the revocation token the funnel minted for it, as a +// completed run does, before the 502 (issue #532). final is the row the +// funnel returned, carrying the per-rec outcome and completed_at. +func (h *Handler) executeFailure(ctx context.Context, req *events.LambdaFunctionURLRequest, executionID, action string, final *config.PurchaseExecution, revocationToken, executedBy string, err error) error { + if purchase.IsCommittedPartialRun(final, err) { + h.sendPurchaseExecutedEmail(ctx, req, final, revocationToken, executedBy) + } + return executeFailureError(executionID, action, final, err) +} + // Stale Azure prices are refused before purchase, even after a successful claim. func executeFailureError(executionID, action string, final *config.PurchaseExecution, err error) error { switch { @@ -476,6 +488,9 @@ func executeFailureError(executionID, action string, final *config.PurchaseExecu return claimedRowRefusalError(final, err) case errors.Is(err, purchase.ErrStaleAzurePricing): return NewClientError(409, err.Error()) + case purchase.IsCommittedPartialRun(final, err): + return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s partially completed: some commitments were purchased and can be revoked, others failed: %v", executionID, err), + map[string]any{"execution_id": executionID, "status": final.Status}) default: return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s failed: %v", executionID, err), map[string]any{"execution_id": executionID, "status": final.Status}) @@ -1042,26 +1057,36 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio // approveConstraintsForSession above read a snapshot; the funnel re-runs // it on the row its claim returns, the row it buys (issue #415). revocationToken, approveErr := h.purchase.ApproveExecution(ctx, execution.ExecutionID, token, actor, h.tokenApproveClaimCheck(tokenSession)) + // A token returned with an error is a partially_completed run: part of it + // committed and is revocable, so it is emailed like a completed run + // (issue #532). Any other error has nothing to email. + if approveErr != nil && revocationToken == "" { + return nil, tokenActionError(approveErr) + } + h.emailTokenApproveOutcome(ctx, req, execution, revocationToken, actor) if approveErr != nil { return nil, tokenActionError(approveErr) } - // Re-fetch the execution to pick up the FINAL state (status, - // completed_at, per-rec purchase results) ApproveAndExecute wrote -- - // the pre-approve `execution` struct is stale on those fields. Falling - // back to the stale struct on a re-fetch failure is a cosmetic - // degradation of the email content only: revocationToken above is - // already in hand either way, so the Revoke link is always valid. + return map[string]string{"status": "completed"}, nil +} + +// emailTokenApproveOutcome sends the executed email for a token approve that +// committed, in full or in part. It re-fetches the execution to pick up the +// FINAL state (status, completed_at, per-rec purchase results) +// ApproveAndExecute wrote -- the pre-approve `execution` struct is stale on +// those fields. Falling back to the stale struct on a re-fetch failure is a +// cosmetic degradation of the email content only: revocationToken is already +// in hand either way, so the Revoke link is always valid. Best-effort (issue +// #291): errors are logged inside sendPurchaseExecutedEmail and never +// propagate, since the purchase is already done. +func (h *Handler) emailTokenApproveOutcome(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, revocationToken, actor string) { emailExec, fetchErr := h.config.GetExecutionByID(ctx, execution.ExecutionID) if fetchErr != nil || emailExec == nil { logging.Warnf("approveViaToken[%s]: re-fetch for email failed (%v); using pre-approve snapshot", execution.ExecutionID, fetchErr) emailExec = execution } - // Best-effort post-execution notification email (issue #291). Mirrors - // the session-authed path; errors are logged inside sendPurchaseExecutedEmail - // and never propagate — the purchase is already done at this point. h.sendPurchaseExecutedEmail(ctx, req, emailExec, revocationToken, actor) - return map[string]string{"status": "completed"}, nil } // approvePurchaseViaSession is the session-authed branch of approvePurchase. @@ -1137,7 +1162,7 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam if approveErr != nil { logging.Errorf("purchase[%s]: approvePurchaseViaSession failed after %s: %v", execution.ExecutionID, time.Since(t0), approveErr) - return nil, executeFailureError(execution.ExecutionID, "approved", final, approveErr) + return nil, h.executeFailure(ctx, req, execution.ExecutionID, "approved", final, revocationToken, session.Email, approveErr) } logging.Infof("purchase[%s]: approvePurchaseViaSession completed in %s (auth=session)", @@ -3627,7 +3652,7 @@ func (h *Handler) directExecutePurchase(ctx context.Context, req *events.LambdaF if err != nil { logging.Errorf("purchase[%s]: directExecutePurchase failed after %s: %v", executionID, time.Since(t0), err) - return nil, executeFailureError(executionID, "direct-executed", final, err) + return nil, h.executeFailure(ctx, req, executionID, "direct-executed", final, revocationToken, session.Email, err) } logging.Infof("purchase[%s]: directExecutePurchase completed in %s", executionID, time.Since(t0)) @@ -3814,29 +3839,16 @@ func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.Lam return } - summaries := make([]email.RecommendationSummary, 0, len(execution.Recommendations)) - for i := range execution.Recommendations { - rec := &execution.Recommendations[i] - summaries = append(summaries, email.RecommendationSummary{ - Service: rec.Service, - ResourceType: rec.ResourceType, - Engine: rec.Engine, - Region: rec.Region, - Count: rec.Count, - MonthlySavings: rec.Savings, - Term: rec.Term, - Payment: rec.Payment, - UpfrontCost: rec.UpfrontCost, - }) - } + summaries, totalUpfront, totalSavings := executedEmailCommitments(execution) dashboardBase := h.resolveDashboardURL(req) data := email.NotificationData{ DashboardURL: dashboardBase, ExecutionID: execution.ExecutionID, - TotalSavings: execution.EstimatedSavings, - TotalUpfrontCost: execution.TotalUpfrontCost, + TotalSavings: totalSavings, + TotalUpfrontCost: totalUpfront, Recommendations: summaries, + PartialPurchase: execution.Status == config.StatusPartiallyCompleted, RecipientEmail: to, CCEmails: cc, // A fresh, dedicated revocation token minted by @@ -3864,6 +3876,52 @@ func (h *Handler) sendPurchaseExecutedEmail(ctx context.Context, req *events.Lam } } +// executedEmailCommitments lists the commitments the executed email reports, +// with their totals. A partially_completed row lists only the recs it bought +// when it records them per rec (issue #532), so the email does not present a +// failed rec as purchased. A multi-account root has no per-rec Purchased +// flags (its per-account rows do), so it lists every rec and the template's +// partial notice says some did not go through. +func executedEmailCommitments(execution *config.PurchaseExecution) (summaries []email.RecommendationSummary, upfront, savings float64) { + onlyPurchased := execution.Status == config.StatusPartiallyCompleted && anyPurchased(execution.Recommendations) + if !onlyPurchased { + upfront, savings = execution.TotalUpfrontCost, execution.EstimatedSavings + } + summaries = make([]email.RecommendationSummary, 0, len(execution.Recommendations)) + for i := range execution.Recommendations { + rec := &execution.Recommendations[i] + if onlyPurchased && !rec.Purchased { + continue + } + if onlyPurchased { + upfront += rec.UpfrontCost + savings += rec.Savings + } + summaries = append(summaries, email.RecommendationSummary{ + Service: rec.Service, + ResourceType: rec.ResourceType, + Engine: rec.Engine, + Region: rec.Region, + Count: rec.Count, + MonthlySavings: rec.Savings, + Term: rec.Term, + Payment: rec.Payment, + UpfrontCost: rec.UpfrontCost, + }) + } + return summaries, upfront, savings +} + +// anyPurchased reports whether any rec records a committed purchase. +func anyPurchased(recs []config.RecommendationRecord) bool { + for i := range recs { + if recs[i].Purchased { + return true + } + } + return false +} + // globalNotifyEmail returns the trimmed notification email from a GlobalConfig, // or "" when the config is nil or the field is unset. Extracted from // sendPurchaseExecutedEmail to keep that function under the cyclomatic limit. diff --git a/internal/api/handler_purchases_run_now_integration_test.go b/internal/api/handler_purchases_run_now_integration_test.go index fc14ceea0..0660e806c 100644 --- a/internal/api/handler_purchases_run_now_integration_test.go +++ b/internal/api/handler_purchases_run_now_integration_test.go @@ -125,6 +125,42 @@ func TestRunNow_PartialFailureIsServerErrorWithRowStatus(t *testing.T) { assert.NotNil(t, row.ExecutedAt) } +// A partial run-now bought one of two commitments (#532): it answers 502 with +// the row status, and it is emailed with a revocation token that revokes the +// stored row, listing only the commitment that was bought. +func TestRunNow_PartialRunEmailsRevocationToken(t *testing.T) { + f, service := newRunNowFixture(t, 2, twoRunNowRecs()) + notify := "ops@example.com" + global, err := f.store.GetGlobalConfig(f.ctx) + require.NoError(t, err) + global.NotificationEmail = ¬ify + require.NoError(t, f.store.SaveGlobalConfig(f.ctx, global)) + f.handler.auth.(*MockAuthService).On("GetUser", mock.Anything, mock.Anything).Return(nil, errors.New("no user")).Maybe() + notifier := &recordingExecutedNotifier{} + f.handler.emailNotifier = notifier + before := time.Now() + + _, runErr := f.runNow() + ce, ok := IsClientError(runErr) + require.True(t, ok, "expected a client-facing error, got %v", runErr) + assert.Equal(t, 502, ce.code) + assert.EqualValues(t, 2, service.calls.Load()) + row, err := f.store.GetExecutionByID(f.ctx, f.execution.ExecutionID) + require.NoError(t, err) + require.Equal(t, "partially_completed", row.Status) + + require.Equal(t, 1, notifier.calls, "a partial run must send the executed email") + token := notifier.captured.RevocationToken + require.NotEmpty(t, token) + assert.NoError(t, validateRevokeToken(row, token), "the emailed token must revoke the stored row") + require.NotNil(t, row.ApprovalTokenExpiresAt) + assert.WithinRange(t, *row.ApprovalTokenExpiresAt, + before.Add(config.RevocationWindow).Add(-time.Second), time.Now().Add(config.RevocationWindow).Add(time.Second)) + assert.True(t, notifier.captured.PartialPurchase) + require.Len(t, notifier.captured.Recommendations, 1, "only the bought commitment is listed") + assert.InDelta(t, 300.0, notifier.captured.TotalUpfrontCost, 0.001) +} + func TestRunNow_FourEyesDenialIsForbiddenAndLeavesRowUntouched(t *testing.T) { f, service := newRunNowFixture(t, 0, fanoutRecs()) require.NoError(t, f.store.SaveGlobalConfig(f.ctx, &config.GlobalConfig{RequireDifferentApprover: true})) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index ba01d7f22..9d979fd59 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -638,6 +638,8 @@ paths: execute_mode "direct" only. The claim was won and the purchase ran, but it failed or only partly completed. Money may have moved; the body details carry execution_id and the row's actual status. + A partially_completed run is revocable: the executed email with + its revocation link is sent as for a completed run. Check the execution in History before retrying. content: application/json: @@ -758,8 +760,10 @@ paths: Session mode only. The claim was won and the purchase ran, but it failed or only partly completed (a failed or partially_completed run). Money may have moved; the body details carry execution_id - and the row's actual status. Check the execution in History - before retrying. + and the row's actual status. A partially_completed run is + revocable: the executed email with its revocation link is sent as + for a completed run. Check the execution in History before + retrying. content: application/json: schema: diff --git a/internal/email/executed_notification_test.go b/internal/email/executed_notification_test.go index 7b6a895b2..41db6f099 100644 --- a/internal/email/executed_notification_test.go +++ b/internal/email/executed_notification_test.go @@ -152,3 +152,31 @@ func TestBuildExecutedNotificationSubject_SingleAndMulti(t *testing.T) { multiSubject := buildExecutedNotificationSubject(multi) assert.Equal(t, "[CUDly] Purchase executed (2 commitment(s))", multiSubject) } + +// A partially_completed run (issue #532) must not read as a full success in +// the subject or in either body half. +func TestRenderPurchaseExecutedNotification_Partial(t *testing.T) { + data := executedNotificationData() + data.PartialPurchase = true + + assert.Equal(t, "[CUDly] Purchase partially executed: ec2 m5.large in us-east-1", buildExecutedNotificationSubject(data)) + + text, err := RenderPurchaseExecutedNotificationEmail(data) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(text, "[CUDly] Purchase partially executed")) + assert.Contains(t, text, "Not every commitment in this purchase went through.") + assert.Contains(t, text, escapedToken, "the partial run stays revocable") + + html, err := RenderPurchaseExecutedNotificationEmailHTML(data) + require.NoError(t, err) + assert.Contains(t, html, "Purchase Partially Executed") + assert.Contains(t, html, "Not every commitment in this purchase went through.") + assert.NotContains(t, html, "were purchased successfully") + assert.Contains(t, html, "Revoke this purchase") + + full := executedNotificationData() + html, err = RenderPurchaseExecutedNotificationEmailHTML(full) + require.NoError(t, err) + assert.Contains(t, html, "were purchased successfully") + assert.NotContains(t, html, "Not every commitment") +} diff --git a/internal/email/sender.go b/internal/email/sender.go index 14b8bdf29..95e16a548 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -602,6 +602,10 @@ type NotificationData struct { // the purchase). Used in the post-execution notification body. // Empty omits the field. ExecutedBy string + // PartialPurchase marks a post-execution notification for a + // partially_completed run (issue #532): the template says some + // commitments were not purchased instead of claiming all of them were. + PartialPurchase bool } // RecommendationSummary is a simplified recommendation for email display. diff --git a/internal/email/templates.go b/internal/email/templates.go index ee712a9c4..1b0524715 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -940,9 +940,12 @@ func (s *Sender) SendPurchaseScheduledNotification(ctx context.Context, data Not // purchaseExecutedNotificationTemplate is the plain-text half of the // post-execution notification email. Rendered alongside // purchaseExecutedNotificationHTMLTemplate for multipart/alternative delivery. -const purchaseExecutedNotificationTemplate = `[CUDly] Purchase executed{{if .Recommendations}} ({{len .Recommendations}} commitment(s)){{end}} +const purchaseExecutedNotificationTemplate = `[CUDly] Purchase {{if .PartialPurchase}}partially {{end}}executed{{if .Recommendations}} ({{len .Recommendations}} commitment(s)){{end}} ============================================================================= -{{if .RequestedByEmail}} +{{if .PartialPurchase}} +Not every commitment in this purchase went through. See the execution in the +dashboard for which ones failed. +{{end}}{{if .RequestedByEmail}} Requested by: {{if .RequestedByName}}{{.RequestedByName}} <{{.RequestedByEmail}}>{{else}}{{.RequestedByEmail}}{{end}}{{if .RequestedAt}} at {{.RequestedAt}}{{end}} {{end}}{{if .ExecutedBy}} Executed by: {{.ExecutedBy}}{{if .ExecutedAt}} at {{.ExecutedAt}}{{end}} @@ -986,8 +989,8 @@ const purchaseExecutedNotificationHTMLTemplate = `
-

Purchase Executed

-

{{len .Recommendations}} commitment(s) were purchased successfully.

+

Purchase {{if .PartialPurchase}}Partially {{end}}Executed

+{{if .PartialPurchase}}

Not every commitment in this purchase went through. See the execution in the dashboard for which ones failed.

{{else}}

{{len .Recommendations}} commitment(s) were purchased successfully.

{{end}}
@@ -1093,12 +1096,16 @@ func (s *Sender) SendPurchaseExecutedNotification(ctx context.Context, data Noti // recommendation list is small enough to fit. Extracted so both Sender // and SMTPSender use the same subject format. func buildExecutedNotificationSubject(data NotificationData) string { + verb := "executed" + if data.PartialPurchase { + verb = "partially executed" + } if len(data.Recommendations) == 1 { r := data.Recommendations[0] - return fmt.Sprintf("[CUDly] Purchase executed: %s %s in %s", - r.Service, r.ResourceType, r.Region) + return fmt.Sprintf("[CUDly] Purchase %s: %s %s in %s", + verb, r.Service, r.ResourceType, r.Region) } - return fmt.Sprintf("[CUDly] Purchase executed (%d commitment(s))", len(data.Recommendations)) + return fmt.Sprintf("[CUDly] Purchase %s (%d commitment(s))", verb, len(data.Recommendations)) } // --------------------------------------------------------------------------- diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index b43544d12..91c3272b3 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -51,7 +51,9 @@ const directExecuteSkipReason = "direct-execute permission" // (see that function's doc comment for why this must be a return value // rather than something the caller re-fetches from the DB). Empty on // failure, and best-effort empty if the mint itself failed after an -// otherwise-successful approve. +// otherwise-successful approve. A partially_completed run returns its token +// together with its error (issue #532), so a non-empty token with an error +// means part of the purchase committed and is revocable. func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, actor string, check ClaimedRowCheck) (string, error) { t0 := time.Now() logging.Infof("purchase[%s]: ApproveExecution entry (auth=token actor=%q)", executionID, maskActor(actor)) @@ -383,10 +385,12 @@ func (m *Manager) RunPlannedPurchaseNow(ctx context.Context, executionID, actor // gate being reimplemented (or skipped) at a second call site. // // Returns the raw revocation token minted on a successful execute, or "" -// if the (best-effort) mint failed. Every caller emails a "purchase -// executed, revoke here" link, and since only the token's hash is stored -// (issue #103) a DB re-read can never yield a raw, emailable value, so the -// rotation lives here, in the one funnel all approve paths share. +// if the (best-effort) mint failed. A partially_completed run also gets one, +// returned alongside its error (see IsCommittedPartialRun, issue #532). +// Every caller emails a "purchase executed, revoke here" link, and since +// only the token's hash is stored (issue #103) a DB re-read can never yield +// a raw, emailable value, so the rotation lives here, in the one funnel all +// approve paths share. // // A non-empty skipReason stamps the executed_* audit fields once the CAS wins. // The returned row is nil when the CAS was never won. @@ -446,23 +450,46 @@ func (m *Manager) transitionApproveAndExecute(ctx context.Context, executionID, } logging.Infof("purchase[%s]: executing purchase synchronously", executionID) + revocationToken, execErr := m.runClaimedAndMint(ctx, updated, t0) + return updated, revocationToken, execErr +} + +// IsCommittedPartialRun reports whether an execute call that returned row and +// err ended partially_completed with that status saved: some commitments were +// bought, so the run is revocable and its executed email must go out like a +// completed run's (issue #532). An audit loss is excluded: the stored row is +// still "running", whatever the returned row says. +func IsCommittedPartialRun(row *config.PurchaseExecution, err error) bool { + return err != nil && row != nil && row.Status == config.StatusPartiallyCompleted && !errors.Is(err, config.ErrAuditLoss) +} + +// runClaimedAndMint runs the claimed row and, when the run committed anything +// (completed, or a saved partially_completed), mints its revocation token. It +// returns the raw token ("" when nothing committed or the best-effort mint +// failed) and the run's error, which a partial run keeps. +func (m *Manager) runClaimedAndMint(ctx context.Context, updated *config.PurchaseExecution, t0 time.Time) (string, error) { + executionID := updated.ExecutionID execErr := m.executeAndFinalize(ctx, updated) if execErr != nil { logging.Errorf("purchase[%s]: transitionApproveAndExecute failed after %s: %v", executionID, time.Since(t0), execErr) - return updated, "", execErr + if !IsCommittedPartialRun(updated, execErr) { + return "", execErr + } + } else { + logging.Infof("purchase[%s]: transitionApproveAndExecute completed in %s", executionID, time.Since(t0)) } - logging.Infof("purchase[%s]: transitionApproveAndExecute completed in %s", executionID, time.Since(t0)) - // Mint a fresh revocation token now that the purchase has committed. The - // pre-approve approval token must not double as the revocation token: on - // the token path it was just consumed to authorize this very approve + // Mint a fresh revocation token now that the purchase has committed, in + // full or in part (a partially_completed row is revocable, issue #532). + // The pre-approve approval token must not double as the revocation token: + // on the token path it was just consumed to authorize this very approve // action, and on every path its raw value is no longer recoverable once // hashed at rest (issue #103). See rotateApprovalToken's doc comment. revocationToken, mintErr := m.rotateApprovalToken(ctx, updated, config.RevocationWindow) if mintErr != nil { logging.Warnf("purchase[%s]: transitionApproveAndExecute: revocation token mint failed (best-effort): %v", executionID, mintErr) } - return updated, revocationToken, nil + return revocationToken, execErr } // CancelExecution cancels a pending execution. actor carries the email of diff --git a/internal/purchase/partial_run_revocation_integration_test.go b/internal/purchase/partial_run_revocation_integration_test.go new file mode 100644 index 000000000..f58777b52 --- /dev/null +++ b/internal/purchase/partial_run_revocation_integration_test.go @@ -0,0 +1,67 @@ +//go:build integration + +package purchase + +// Real PostgreSQL plus the ramp-step fixture's fake provider (labeled fixture; +// nothing here reaches a cloud API). Issue #532: a run that ends +// partially_completed bought part of its commitments, so the approve funnel +// mints it a revocation token as it does for a completed run. + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A fan-out root whose first account buys and whose other two fail on +// credentials ends partially_completed: the funnel returns the error, the row +// and a token whose hash is stored with the revocation window as its expiry. +func TestApproveAndExecute_PartialRunMintsRevocationToken(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + allowFailedNotification(f.manager.email.(*MockEmailSender)) + root := f.saveRootExecution(ctx, t, 3) + before := time.Now() + + final, token, err := f.manager.ApproveAndExecute(ctx, root.ExecutionID, "approver@example.invalid", nil, nil) + require.Error(t, err) + require.NotNil(t, final) + assert.True(t, IsCommittedPartialRun(final, err)) + require.NotEmpty(t, token, "a partial run is revocable, so it needs a revocation token") + assert.Equal(t, 1, f.purchaseCount()) + + row := rowStatus(ctx, t, f, root.ExecutionID) + assert.Equal(t, config.StatusPartiallyCompleted, row.Status) + assert.True(t, config.ApprovalTokenMatches(row.ApprovalToken, token), "the stored hash must match the returned token") + require.NotNil(t, row.ApprovalTokenExpiresAt) + assert.WithinRange(t, *row.ApprovalTokenExpiresAt, + before.Add(config.RevocationWindow).Add(-time.Second), time.Now().Add(config.RevocationWindow).Add(time.Second)) +} + +// Nothing bought: no revocation token, and the row keeps its approval token. +func TestApproveAndExecute_FailedRunMintsNoToken(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + allowFailedNotification(f.manager.email.(*MockEmailSender)) + broken := f.accounts[1].ID + row := &config.PurchaseExecution{ + PlanID: f.plan.ID, ExecutionID: uuid.New().String(), Status: "notified", StepNumber: 3, + ScheduledDate: time.Now(), CloudAccountID: &broken, ApprovalToken: config.HashApprovalToken("approval-tok"), + Recommendations: []config.RecommendationRecord{withAccount(rampStepRecommendation()[0], broken)}, + } + require.NoError(t, f.store.SavePurchaseExecution(ctx, row)) + + final, token, err := f.manager.ApproveAndExecute(ctx, row.ExecutionID, "approver@example.invalid", nil, nil) + require.Error(t, err) + require.NotNil(t, final) + assert.Equal(t, "failed", final.Status) + assert.False(t, IsCommittedPartialRun(final, err)) + assert.Empty(t, token) + assert.Zero(t, f.purchaseCount()) + assert.True(t, config.ApprovalTokenMatches(rowStatus(ctx, t, f, row.ExecutionID).ApprovalToken, "approval-tok")) +} From 09716248acf6473dc9a8f5774af12df4cde5aab3 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:33:26 +0000 Subject: [PATCH 2/2] test(purchase): pin that a stranded-approval re-drive loses to an in-flight sync run The stale-approved recovery (claimAndRedrive) claims only "approved" rows, and approve, direct execute and run-now hold their row "running" for the whole provider call (#829), so the CAS to "running" is the single claimer. Replay the race on real PostgreSQL: a re-drive that read the row as approved arrives mid-run, loses the claim and buys nothing. Refs #532 Co-Authored-By: Claude Opus 5.5 --- .../purchase/single_claim_integration_test.go | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/internal/purchase/single_claim_integration_test.go b/internal/purchase/single_claim_integration_test.go index a73cd91b4..e36f1a6d8 100644 --- a/internal/purchase/single_claim_integration_test.go +++ b/internal/purchase/single_claim_integration_test.go @@ -209,3 +209,52 @@ func TestSingleClaim_ExecutorArrivingMidApproveCannotBuyAgain(t *testing.T) { assert.Equal(t, 1, f.purchaseCount()) assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) } + +// Issue #532 item 4: the stranded-approval re-drive claims only "approved" +// rows, and every synchronous run (approve, direct execute, run-now) holds its +// row "running" for the whole provider call, so a re-drive that read the row +// as approved (a stale listing) loses the claim mid-run and buys nothing. +func TestSingleClaim_StrandedRedriveMidSyncRunCannotBuyAgain(t *testing.T) { + ctx := context.Background() + f := newRampStepFixture(ctx, t) + runs := map[string]func(id string) error{ + "approve": func(id string) error { + _, _, err := f.manager.ApproveAndExecute(ctx, id, "a@example.invalid", nil, nil) + return err + }, + "direct execute": func(id string) error { + _, _, err := f.manager.DirectExecute(ctx, id, "a@example.invalid", nil, nil) + return err + }, + "run now": func(id string) error { + _, _, err := f.manager.RunPlannedPurchaseNow(ctx, id, "a@example.invalid", nil, nil) + return err + }, + } + for name, run := range runs { + t.Run(name, func(t *testing.T) { + row := claimRaceRow(ctx, t, f, "notified") + before := f.purchaseCount() + var fired atomic.Bool + var midStatus string + var redriven bool + var redriveErr error + f.onPurchase = func() { + if fired.Swap(true) { + return + } + midStatus = rowStatus(ctx, t, f, row.ExecutionID).Status + c := rowStatusCopy(row) + c.Status = "approved" + redriven, redriveErr = f.manager.claimAndRedrive(ctx, c) + } + t.Cleanup(func() { f.onPurchase = nil }) + require.NoError(t, run(row.ExecutionID)) + assert.Equal(t, "running", midStatus, "the sync run holds the row running during the provider call") + require.NoError(t, redriveErr) + assert.False(t, redriven, "the re-drive must lose the claim") + assert.Equal(t, 1, f.purchaseCount()-before) + assert.Equal(t, "completed", rowStatus(ctx, t, f, row.ExecutionID).Status) + }) + } +}