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 f80f3554e..95ec54647 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}) @@ -1044,26 +1059,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. @@ -1139,7 +1164,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)", @@ -3737,7 +3762,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)) @@ -3924,29 +3949,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 @@ -3974,6 +3986,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 60a542f9c..fbcaf4a10 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -893,6 +893,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: @@ -1013,8 +1015,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 de013ad0f..9ffbfe40e 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 // OutcomeUnknown switches the purchase-failed email to the outcome-unknown // variant, for a row a recovery sweep failed after its purchase run was // interrupted: the commitment may already exist, so the email asks the diff --git a/internal/email/templates.go b/internal/email/templates.go index 70b05b7e9..b7a1be813 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -957,9 +957,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}} @@ -1003,8 +1006,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}} |
| @@ -1110,12 +1113,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 61bee1662..530760d2c 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -52,7 +52,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)) @@ -384,10 +386,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, or when the audit stamp @@ -452,23 +456,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")) +} 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) + }) + } +} |