From 1ff5fcbe6b0df60c7cc32ed6a6ec44a39e15296c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 13:27:57 +0100 Subject: [PATCH 01/12] feat(purchase): summarize provider purchase errors into fixed categories Add a closed set of failure categories (access denied, not available, limit reached, unknown) with fixed user messages. SummarizeProviderError classifies a live error by its type, and SanitizeStoredError rewrites already-stored error strings that carry provider text, leaving server-authored notes untouched. Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errsummary.go | 156 +++++++++++++++++++++++++++ internal/purchase/errsummary_test.go | 86 +++++++++++++++ 2 files changed, 242 insertions(+) create mode 100644 internal/purchase/errsummary.go create mode 100644 internal/purchase/errsummary_test.go diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go new file mode 100644 index 000000000..d4201566b --- /dev/null +++ b/internal/purchase/errsummary.go @@ -0,0 +1,156 @@ +package purchase + +import ( + "errors" + "net/http" + "strings" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "google.golang.org/api/googleapi" +) + +// Closed set of purchase failure categories shown to users in place of the +// provider's own error text, which carries ARNs, account IDs and request IDs. +const ( + ErrCategoryAccessDenied = "access_denied" + ErrCategoryUnavailable = "not_available" + ErrCategoryLimit = "limit_reached" + ErrCategoryUnknown = "unknown" +) + +var errCategoryMessages = map[string]string{ + ErrCategoryAccessDenied: "the cloud provider refused the purchase: CUDly's role is not allowed to make it in this account", + ErrCategoryUnavailable: "the requested commitment is not available from the provider", + ErrCategoryLimit: "a provider limit was reached", + ErrCategoryUnknown: "the provider returned an error", +} + +const partialRunPrefix = "some purchases failed (partial success): " + +// ErrCategoryMessage returns the fixed user message for a category. +func ErrCategoryMessage(category string) string { + if m, ok := errCategoryMessages[category]; ok { + return m + } + return errCategoryMessages[ErrCategoryUnknown] +} + +// IsDefiniteRefusal reports whether the provider clearly refused the +// purchase, so nothing was bought. +func IsDefiniteRefusal(category string) bool { + return category == ErrCategoryAccessDenied || category == ErrCategoryUnavailable || category == ErrCategoryLimit +} + +// SummarizeProviderError classifies a live error by type and returns its +// category and fixed message. +func SummarizeProviderError(err error) (category, message string) { + category = classifyTypedError(err) + if category == ErrCategoryUnknown && err != nil { + category = classifyText(err.Error()) + } + return category, ErrCategoryMessage(category) +} + +func classifyTypedError(err error) string { + var apiErr interface{ ErrorCode() string } + var azErr *azcore.ResponseError + var gErr *googleapi.Error + switch { + case errors.As(err, &apiErr): + if c := classifyText(apiErr.ErrorCode()); c != ErrCategoryUnknown { + return c + } + case errors.As(err, &azErr): + if c := classifyText(azErr.ErrorCode); c != ErrCategoryUnknown { + return c + } + return categoryForStatus(azErr.StatusCode) + case errors.As(err, &gErr): + return categoryForStatus(gErr.Code) + } + var httpErr interface{ HTTPStatusCode() int } + if errors.As(err, &httpErr) { + return categoryForStatus(httpErr.HTTPStatusCode()) + } + return ErrCategoryUnknown +} + +func categoryForStatus(code int) string { + switch code { + case http.StatusUnauthorized, http.StatusForbidden: + return ErrCategoryAccessDenied + case http.StatusNotFound: + return ErrCategoryUnavailable + case http.StatusTooManyRequests: + return ErrCategoryLimit + } + return ErrCategoryUnknown +} + +var ( + deniedNeedles = []string{"accessdenied", "unauthorized", "authorizationfailed", "forbidden", + "encoded authorization", "statuscode: 401", "statuscode: 403", "response 401", "response 403", "error 403"} + limitNeedles = []string{"limitexceeded", "quota", "throttl", "toomanyrequests", "statuscode: 429", "response 429", "error 429"} + unavailNeedles = []string{"capacity", "offeringnotfound", "notfound", "not available", + "notavailable", "statuscode: 404", "response 404", "error 404"} + // Substrings that only appear in provider-authored error text. + providerMarkers = []string{"operation error", "api error", "statuscode:", "requestid", "arn:aws", + "encoded authorization", "accessdenied", "unauthorizedoperation", "authorizationfailed", + "googleapi", "error code:", "response 401", "response 403", "response 404", "response 429", "response 5"} +) + +func containsAny(lower string, needles []string) bool { + for _, n := range needles { + if strings.Contains(lower, n) { + return true + } + } + return false +} + +func classifyText(s string) string { + lower := strings.ToLower(s) + switch { + case containsAny(lower, deniedNeedles): + return ErrCategoryAccessDenied + case containsAny(lower, limitNeedles): + return ErrCategoryLimit + case containsAny(lower, unavailNeedles): + return ErrCategoryUnavailable + } + return ErrCategoryUnknown +} + +func hasProviderMarker(s string) bool { + return containsAny(strings.ToLower(s), providerMarkers) +} + +// SummarizeStoredError returns the category of a stored error string, or "" +// when the string carries no provider text (a server-authored note). +func SummarizeStoredError(s string) string { + if !hasProviderMarker(s) { + return "" + } + return classifyText(s) +} + +// SanitizeStoredError rewrites provider error text in an already-stored +// error string into its category message. Strings and "; "-joined segments +// without provider markers pass through unchanged. +func SanitizeStoredError(s string) string { + if !hasProviderMarker(s) { + return s + } + prefix := "" + if strings.HasPrefix(s, partialRunPrefix) { + prefix = partialRunPrefix + s = strings.TrimPrefix(s, partialRunPrefix) + } + segs := strings.Split(s, "; ") + for i, seg := range segs { + if hasProviderMarker(seg) { + segs[i] = ErrCategoryMessage(classifyText(seg)) + } + } + return prefix + strings.Join(segs, "; ") +} diff --git a/internal/purchase/errsummary_test.go b/internal/purchase/errsummary_test.go new file mode 100644 index 000000000..e6b8daef6 --- /dev/null +++ b/internal/purchase/errsummary_test.go @@ -0,0 +1,86 @@ +package purchase + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/aws/smithy-go" + "github.com/stretchr/testify/assert" + "google.golang.org/api/googleapi" +) + +const fakeAWS403 = "operation error EC2: PurchaseReservedInstancesOffering, https response error StatusCode: 403, " + + "RequestID: 11111111-2222-3333-4444-555555555555, api error UnauthorizedOperation: You are not authorized to perform this operation. " + + "User: arn:aws:sts::123456789012:assumed-role/cudly-role/session is not authorized. " + + "Encoded authorization failure message: AbCdEfGhIjKlMnOpQrStUvWxYz0123456789" + +func TestSanitizeStoredError_AWS403(t *testing.T) { + out := SanitizeStoredError(fakeAWS403) + assert.Equal(t, ErrCategoryMessage(ErrCategoryAccessDenied), out) + for _, bad := range []string{"arn:", "123456789012", "RequestID", "Encoded"} { + assert.NotContains(t, out, bad) + } +} + +func TestSanitizeStoredError_PassThrough(t *testing.T) { + for _, s := range []string{ + "history_write_failed: db timeout", + "recovered after reap; provider purchase succeeded", + "ramp not advanced: ladder step missing", + "reaped after 30m in running state, executor did not complete", + "refusing to execute retry attempt 2: AWS EC2 and Redshift purchases have no provider-side duplicate guard", + ErrStaleAzurePricing.Error(), + "stranded in approved state", + "", + } { + assert.Equal(t, s, SanitizeStoredError(s)) + } +} + +func TestSanitizeStoredError_Samples(t *testing.T) { + cases := map[string]string{ + "RESPONSE 403: 403 Forbidden\nERROR CODE: AuthorizationFailed": ErrCategoryAccessDenied, + "googleapi: Error 403: Required permission denied, forbidden": ErrCategoryAccessDenied, + "operation error EC2: X, api error InsufficientInstanceCapacity: none": ErrCategoryUnavailable, + "operation error RDS: X, api error ReservedDBInstanceQuotaExceeded: no": ErrCategoryLimit, + "operation error EC2: X, api error SomethingOdd: boom": ErrCategoryUnknown, + } + for in, cat := range cases { + assert.Equal(t, ErrCategoryMessage(cat), SanitizeStoredError(in), in) + } +} + +func TestSanitizeStoredError_PartialRun(t *testing.T) { + in := partialRunPrefix + "rec a: " + fakeAWS403 + "; rec b failed locally; " + "api error LimitExceeded: x arn:aws:iam::123456789012:role/r" + out := SanitizeStoredError(in) + assert.True(t, strings.HasPrefix(out, partialRunPrefix)) + assert.Contains(t, out, "rec b failed locally") + assert.Contains(t, out, ErrCategoryMessage(ErrCategoryLimit)) + assert.NotContains(t, out, "123456789012") + assert.NotContains(t, out, "arn:") +} + +func TestSummarizeProviderError_Typed(t *testing.T) { + cases := []struct { + err error + cat string + }{ + {&smithy.GenericAPIError{Code: "UnauthorizedOperation"}, ErrCategoryAccessDenied}, + {&smithy.GenericAPIError{Code: "AccessDenied"}, ErrCategoryAccessDenied}, + {&smithy.GenericAPIError{Code: "ReservedInstancesLimitExceeded"}, ErrCategoryLimit}, + {&smithy.GenericAPIError{Code: "InsufficientInstanceCapacity"}, ErrCategoryUnavailable}, + {&azcore.ResponseError{ErrorCode: "AuthorizationFailed", StatusCode: 403}, ErrCategoryAccessDenied}, + {&googleapi.Error{Code: 404}, ErrCategoryUnavailable}, + {fmt.Errorf("wrapped: %w", &googleapi.Error{Code: 403}), ErrCategoryAccessDenied}, + {errors.New("boom"), ErrCategoryUnknown}, + } + for _, c := range cases { + cat, msg := SummarizeProviderError(c.err) + assert.Equal(t, c.cat, cat, c.err.Error()) + assert.Equal(t, ErrCategoryMessage(c.cat), msg) + assert.NotEmpty(t, msg) + } +} From 12021c963e9f97b55bd54c5ceed8e18bf00b31c5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 13:30:42 +0100 Subject: [PATCH 02/12] fix(api): show summarized purchase failures and keep provider details in logs When a provider refuses a purchase, the execute response, the history rows, the purchase details and the retry 409 now show a short category message plus the execution ID instead of the provider's own error text. The full error is written to the server log, and the stored error column is unchanged. Rows already stored are summarized when read. The session approve test that expected the raw run error text now expects the category message and the execution reference. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_history.go | 6 +- internal/api/handler_purchases.go | 47 ++++++-- internal/api/purchase_failure_summary_test.go | 100 ++++++++++++++++++ .../api/token_approve_failure_message_test.go | 10 +- 4 files changed, 148 insertions(+), 15 deletions(-) create mode 100644 internal/api/purchase_failure_summary_test.go diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 13c674fb0..0ed7b4a0d 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -428,7 +428,7 @@ func annotateHistoryRowByStatus(row *config.PurchaseHistoryRecord, exec config.P case "pending", "notified": row.Approver = approver case "failed": - row.StatusDescription = exec.Error + row.StatusDescription = purchase.SanitizeStoredError(exec.Error) case "expired": row.StatusDescription = "approval link expired (not approved within 7 days)" case config.StatusCanceled, config.LegacyStatusCanceled: @@ -482,7 +482,7 @@ func annotateInFlightOrAuditGapRow(row *config.PurchaseHistoryRecord, exec confi // double-count guard as the audit-gap completed case below. row.IsAuditGap = true annotateApproved(row, exec, approver) - row.StatusDescription = "partially completed — some commitments succeeded, others failed: " + exec.Error + row.StatusDescription = "partially completed — some commitments succeeded, others failed: " + purchase.SanitizeStoredError(exec.Error) case "completed": // Only audit-gap completed executions reach here (fetchExecutionsAsHistory // skips clean completed rows). exec.Error carries why the history write @@ -494,7 +494,7 @@ func annotateInFlightOrAuditGapRow(row *config.PurchaseHistoryRecord, exec confi row.IsAuditGap = true annotateApproved(row, exec, approver) if exec.Error != "" { - row.StatusDescription = "purchase completed but its history record could not be saved: " + exec.Error + row.StatusDescription = "purchase completed but its history record could not be saved: " + purchase.SanitizeStoredError(exec.Error) } } } diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 764f38756..4e3bf89de 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -527,20 +527,40 @@ func executeFailureError(executionID, action string, final *config.PurchaseExecu } switch { case errors.Is(err, config.ErrAuditLoss): - return NewClientError(500, fmt.Sprintf("execution %s ran but its final status could not be saved: %v", executionID, err)) + logRawExecuteFailure(executionID, err) + return NewClientError(500, fmt.Sprintf("execution %s ran but its final status could not be saved (reference: execution %s)", executionID, executionID)) case errors.Is(err, purchase.ErrClaimedRowRefused): 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}) + category, msg := summarizeExecuteFailure(executionID, err) + return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s partially completed: some commitments were purchased and can be revoked, others failed: %s (reference: execution %s)", executionID, msg, executionID), + map[string]any{"execution_id": executionID, "status": final.Status, "category": category}) default: - return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s failed: %v", executionID, err), - map[string]any{"execution_id": executionID, "status": final.Status}) + category, msg := summarizeExecuteFailure(executionID, err) + return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s failed: %s (reference: execution %s)", executionID, msg, executionID), + map[string]any{"execution_id": executionID, "status": final.Status, "category": category}) } } +// logRawExecuteFailure keeps the provider's own error text in the server log +// only; responses carry a category message and the execution ID. +func logRawExecuteFailure(executionID string, err error) { + logging.Errorf("purchase[%s]: execution failed: %v", executionID, err) +} + +func summarizeExecuteFailure(executionID string, err error) (category, message string) { + logRawExecuteFailure(executionID, err) + category, message = purchase.SummarizeProviderError(err) + if category == purchase.ErrCategoryUnknown { + // Server-authored refusals (for example "re-fetch the recommendation") + // carry no provider text and stay readable. + message = purchase.SanitizeStoredError(err.Error()) + } + return category, message +} + // unclaimedExecuteError answers a failure where the funnel returned no row: // a lost claim race or a vanished row is a 409, anything else is a plain error. func unclaimedExecuteError(executionID, action string, err error) error { @@ -2638,7 +2658,7 @@ func checkRetryEligibilityGates(failedExec *config.PurchaseExecution, req *event if hint := resolveOpsHint(failedExec.Error); hint != "" { return NewClientErrorWithDetails(409, "this failure is operator-fixable; retrying without changing configuration will fail again", - map[string]any{"ops_hint": hint, "failure_reason": failedExec.Error}) + map[string]any{"ops_hint": hint, "failure_reason": purchase.SanitizeStoredError(failedExec.Error)}) } // Threshold soft-block (Q2). force=true (set by the frontend @@ -3076,6 +3096,17 @@ func (h *Handler) tryGetSession(ctx context.Context, req *events.LambdaFunctionU return session } +// sanitizedRecommendationErrors copies recs with provider text in Error +// replaced, leaving the stored slice untouched. +func sanitizedRecommendationErrors(recs []config.RecommendationRecord) []config.RecommendationRecord { + out := make([]config.RecommendationRecord, len(recs)) + copy(out, recs) + for i := range out { + out[i].Error = purchase.SanitizeStoredError(out[i].Error) + } + return out +} + // buildPurchaseDetailsResponse builds the response map for a purchase execution. func buildPurchaseDetailsResponse(execution *config.PurchaseExecution, planName string) map[string]any { response := map[string]any{ @@ -3087,7 +3118,7 @@ func buildPurchaseDetailsResponse(execution *config.PurchaseExecution, planName "scheduled_date": execution.ScheduledDate.Format("2006-01-02"), "total_upfront_cost": execution.TotalUpfrontCost, "estimated_savings": execution.EstimatedSavings, - "recommendations": execution.Recommendations, + "recommendations": sanitizedRecommendationErrors(execution.Recommendations), } if execution.NotificationSent != nil { @@ -3097,7 +3128,7 @@ func buildPurchaseDetailsResponse(execution *config.PurchaseExecution, planName response["completed_at"] = execution.CompletedAt.Format("2006-01-02T15:04:05Z") } if execution.Error != "" { - response["error"] = execution.Error + response["error"] = purchase.SanitizeStoredError(execution.Error) } return response } diff --git a/internal/api/purchase_failure_summary_test.go b/internal/api/purchase_failure_summary_test.go new file mode 100644 index 000000000..3533bc65a --- /dev/null +++ b/internal/api/purchase_failure_summary_test.go @@ -0,0 +1,100 @@ +package api + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + fakeEncodedBlob = "AbCdEfGhIjKlMnOpQrStUvWxYz0123456789LongEncodedBlob" + fakeAWSDenied = "operation error EC2: PurchaseReservedInstancesOffering, https response error StatusCode: 403, " + + "RequestID: 11111111-2222-3333-4444-555555555555, api error UnauthorizedOperation: You are not authorized. " + + "User: arn:aws:sts::123456789012:assumed-role/cudly-role/session. " + + "Encoded authorization failure message: " + fakeEncodedBlob +) + +var providerTextNeedles = []string{"arn:aws", "123456789012", "Encoded authorization", fakeEncodedBlob, "RequestID"} + +func assertNoProviderText(t *testing.T, where, s string) { + t.Helper() + for _, n := range providerTextNeedles { + assert.NotContains(t, s, n, where) + } +} + +// One realistic provider denial must stay out of every user-facing surface. +func TestProviderDenial_NeverReachesUserSurfaces(t *testing.T) { + const execID = "11111111-1111-1111-1111-111111111111" + raw := errors.New(fakeAWSDenied) + rec := config.RecommendationRecord{Provider: "aws", Service: "rds", Error: fakeAWSDenied} + + t.Run("direct execute 502", func(t *testing.T) { + err := executeFailureError(execID, "executed", &config.PurchaseExecution{Status: "failed"}, raw) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 502, ce.code) + body, mErr := json.Marshal(ce.Details()) + require.NoError(t, mErr) + assertNoProviderText(t, "message", ce.Error()) + assertNoProviderText(t, "details", string(body)) + assert.Contains(t, ce.Error(), execID) + assert.Equal(t, "access_denied", ce.Details()["category"]) + }) + + t.Run("history failed row", func(t *testing.T) { + row := executionToHistoryRow(config.PurchaseExecution{ + ExecutionID: execID, Status: "failed", Error: "IAM denied: " + fakeAWSDenied, + }, "", "") + assertNoProviderText(t, "status_description", row.StatusDescription) + assertNoProviderText(t, "ops_hint", row.OpsHint) + assert.Equal(t, "Grant the deploy role missing IAM permission, then retry", row.OpsHint, + "the hint still resolves from the raw text") + }) + + t.Run("purchase details", func(t *testing.T) { + exec := &config.PurchaseExecution{ExecutionID: execID, Status: "failed", Error: fakeAWSDenied, + Recommendations: []config.RecommendationRecord{rec}} + resp := buildPurchaseDetailsResponse(exec, "plan") + body, err := json.Marshal(resp) + require.NoError(t, err) + assertNoProviderText(t, "details body", string(body)) + assert.Equal(t, fakeAWSDenied, exec.Error, "stored row is not mutated") + assert.Equal(t, fakeAWSDenied, exec.Recommendations[0].Error, "stored rec is not mutated") + }) + + t.Run("retry 409 failure_reason", func(t *testing.T) { + creator := retryCallerID + failed := &config.PurchaseExecution{ExecutionID: retryExecID, Status: "failed", + Error: "AssumeRole error: IAM denied: " + fakeAWSDenied, CreatedByUserID: &creator} + handler, _, _ := buildSessionRetryHandler(t, failed, &Session{UserID: retryCallerID}, false, true) + _, err := handler.retryPurchase(t.Context(), sessionRetryReq(), retryExecID) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + body, mErr := json.Marshal(ce.Details()) + require.NoError(t, mErr) + assertNoProviderText(t, "retry details", string(body)+ce.Error()) + assert.NotEmpty(t, ce.Details()["failure_reason"]) + }) +} + +func TestExecuteFailureError_PartialAndAuditLossHideProviderText(t *testing.T) { + for _, status := range []string{"partially_completed", "failed"} { + err := executeFailureError("e1", "executed", &config.PurchaseExecution{Status: status}, + errors.New("some purchases failed (partial success): "+fakeAWSDenied)) + ce, ok := IsClientError(err) + require.True(t, ok) + assertNoProviderText(t, status, ce.Error()) + } + err := executeFailureError("e1", "executed", &config.PurchaseExecution{Status: "failed"}, + errors.Join(config.ErrAuditLoss, errors.New(fakeAWSDenied))) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 500, ce.code) + assertNoProviderText(t, "audit loss", ce.Error()) +} diff --git a/internal/api/token_approve_failure_message_test.go b/internal/api/token_approve_failure_message_test.go index 2cbd78f95..ff9ac3816 100644 --- a/internal/api/token_approve_failure_message_test.go +++ b/internal/api/token_approve_failure_message_test.go @@ -101,9 +101,9 @@ func TestTokenApprove_FailureBodyOmitsRunErrorText(t *testing.T) { } } -// The session caller holds approve-* scoped to the execution and keeps the -// detailed message. -func TestSessionApprove_FailureKeepsRunErrorText(t *testing.T) { +// The session caller gets the execution reference and a category message; the +// provider's own text (ARNs, account IDs) stays in the server log. +func TestSessionApprove_FailureShowsSummaryNotRunErrorText(t *testing.T) { ctx := context.Background() execID := "c1234567-c123-c123-c123-c1234567abcd" for _, tc := range tokenFailureCases()[:3] { @@ -113,7 +113,9 @@ func TestSessionApprove_FailureKeepsRunErrorText(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok, "expected a client error, got %v", err) assert.Equal(t, tc.code, ce.code) - assert.Contains(t, ce.message, otherAcctName) + assert.NotContains(t, ce.message, "arn:aws") + assert.NotContains(t, ce.message, otherAcctID) + assert.Contains(t, ce.message, execID) }) } } From 88cd56f7d8f69a47958c1668c1fd4aeec0b9acc5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 13:42:58 +0100 Subject: [PATCH 03/12] fix(history): show the duplicate-purchase warning only for unknown outcomes A failed EC2 or Redshift row carried a warning that the earlier attempt may already have bought the commitment, even when the provider gave a definite refusal (access denied, not available, limit reached). In those cases nothing was bought, so the warning is now left out. It stays for failures with an unknown outcome. Retry eligibility is unchanged. Closes #1688 Refs #1694 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_history.go | 8 +++++++- internal/api/history_duplicate_hint_test.go | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 internal/api/history_duplicate_hint_test.go diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 0ed7b4a0d..d2b03091c 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -412,8 +412,14 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma // fixing the config makes Retry work). Same call and input as the // retry handler's gate so the two cannot disagree (issue #161). if reason := purchase.RedriveRefusalReason(&exec); reason != "" { - row.OpsHint = reason row.RedriveUnsafe = true + // A definite provider refusal means nothing was bought, so the + // duplicate-purchase warning would mislead. + if purchase.IsDefiniteRefusal(purchase.SummarizeStoredError(exec.Error)) { + row.OpsHint = "" + } else { + row.OpsHint = reason + } } } annotateHistoryRowByStatus(&row, exec, approver) diff --git a/internal/api/history_duplicate_hint_test.go b/internal/api/history_duplicate_hint_test.go new file mode 100644 index 000000000..d16f509fb --- /dev/null +++ b/internal/api/history_duplicate_hint_test.go @@ -0,0 +1,18 @@ +package api + +import ( + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +func TestHistoryRow_DuplicateGuardHint(t *testing.T) { + ec2 := []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}} + denied := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: fakeAWSDenied, Recommendations: ec2}, "", "") + assert.False(t, strings.Contains(denied.OpsHint, "duplicate guard"), "definite refusal hides the warning") + unknown := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: "purchase outcome unknown", Recommendations: ec2}, "", "") + assert.Contains(t, unknown.OpsHint, "duplicate guard") + assert.True(t, unknown.RedriveUnsafe) +} From 276483111dcfb2aef77f36513936d3fbac469988 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 14:23:54 +0100 Subject: [PATCH 04/12] fix(history): keep the duplicate-purchase warning unless every rec was refused The warning was dropped when the stored error contained any definite provider refusal. A failed run joins one error per rec, so a run where one rec was refused and another ended with an unknown outcome lost the warning even though the second rec may have bought a commitment. The gate now requires every "; "-joined segment to be a definite provider refusal. Refs #1688 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_history.go | 6 +++--- internal/api/history_duplicate_hint_test.go | 6 ++++++ internal/purchase/errsummary.go | 16 ++++++++++------ internal/purchase/errsummary_test.go | 15 +++++++++++++++ 4 files changed, 34 insertions(+), 9 deletions(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index d2b03091c..f4b5f8b82 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -413,9 +413,9 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma // retry handler's gate so the two cannot disagree (issue #161). if reason := purchase.RedriveRefusalReason(&exec); reason != "" { row.RedriveUnsafe = true - // A definite provider refusal means nothing was bought, so the - // duplicate-purchase warning would mislead. - if purchase.IsDefiniteRefusal(purchase.SummarizeStoredError(exec.Error)) { + // A definite provider refusal on every rec means nothing was + // bought, so the duplicate-purchase warning would mislead. + if purchase.IsDefiniteStoredRefusal(exec.Error) { row.OpsHint = "" } else { row.OpsHint = reason diff --git a/internal/api/history_duplicate_hint_test.go b/internal/api/history_duplicate_hint_test.go index d16f509fb..874dc76d3 100644 --- a/internal/api/history_duplicate_hint_test.go +++ b/internal/api/history_duplicate_hint_test.go @@ -15,4 +15,10 @@ func TestHistoryRow_DuplicateGuardHint(t *testing.T) { unknown := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: "purchase outcome unknown", Recommendations: ec2}, "", "") assert.Contains(t, unknown.OpsHint, "duplicate guard") assert.True(t, unknown.RedriveUnsafe) + + // One refused rec does not prove that another, whose outcome is unknown, bought nothing. + mixed := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: "t4g.nano: purchase outcome unknown; t4g.micro: " + fakeAWSDenied, Recommendations: ec2}, "", "") + assert.Contains(t, mixed.OpsHint, "duplicate guard", "a mixed failure keeps the warning") + allDenied := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: "t4g.nano: " + fakeAWSDenied + "; t4g.micro: " + fakeAWSDenied, Recommendations: ec2}, "", "") + assert.NotContains(t, allDenied.OpsHint, "duplicate guard", "every rec refused hides the warning") } diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index d4201566b..8827e9c53 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -125,13 +125,17 @@ func hasProviderMarker(s string) bool { return containsAny(strings.ToLower(s), providerMarkers) } -// SummarizeStoredError returns the category of a stored error string, or "" -// when the string carries no provider text (a server-authored note). -func SummarizeStoredError(s string) string { - if !hasProviderMarker(s) { - return "" +// IsDefiniteStoredRefusal reports whether every "; "-joined segment of a stored +// error is a definite provider refusal. One unknown or server-authored segment +// means that rec's outcome is not proven, so the answer is false. +func IsDefiniteStoredRefusal(s string) bool { + s = strings.TrimPrefix(s, partialRunPrefix) + for _, seg := range strings.Split(s, "; ") { + if !hasProviderMarker(seg) || !IsDefiniteRefusal(classifyText(seg)) { + return false + } } - return classifyText(s) + return s != "" } // SanitizeStoredError rewrites provider error text in an already-stored diff --git a/internal/purchase/errsummary_test.go b/internal/purchase/errsummary_test.go index e6b8daef6..4299677d0 100644 --- a/internal/purchase/errsummary_test.go +++ b/internal/purchase/errsummary_test.go @@ -84,3 +84,18 @@ func TestSummarizeProviderError_Typed(t *testing.T) { assert.NotEmpty(t, msg) } } + +func TestIsDefiniteStoredRefusal(t *testing.T) { + cases := map[string]bool{ + fakeAWS403: true, + "t4g.nano: " + fakeAWS403 + "; t4g.micro: " + fakeAWS403: true, + partialRunPrefix + "t4g.nano: " + fakeAWS403: true, + "t4g.nano: purchase outcome unknown; t4g.micro: " + fakeAWS403: false, + "purchase outcome unknown": false, + "history_write_failed: commitment purchased": false, + "": false, + } + for in, want := range cases { + assert.Equal(t, want, IsDefiniteStoredRefusal(in), in) + } +} From 37b5e6a334da3e1ad6c0e67381390c118618a51c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:20:16 +0100 Subject: [PATCH 05/12] fix(purchase): pass through only known server notes in stored errors Stored and live error text now fails closed: only allowlisted server notes (and known refusal sentinels) are shown as written; any other text becomes one category message for the whole remainder. Categories come from explicit code fields only, and the duplicate-purchase hint is dropped only when every rec is AWS and every segment is an AWS 4xx with a listed pre-send code. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_history.go | 6 +- internal/api/handler_purchases.go | 8 +- internal/api/handler_purchases_test.go | 4 +- internal/purchase/errnotes.go | 81 ++++++++++++ internal/purchase/errnotes_test.go | 78 ++++++++++++ internal/purchase/errsummary.go | 170 +++++++++++++++++-------- internal/purchase/errsummary_test.go | 34 +---- internal/purchase/manager.go | 10 +- 8 files changed, 297 insertions(+), 94 deletions(-) create mode 100644 internal/purchase/errnotes.go create mode 100644 internal/purchase/errnotes_test.go diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index f4b5f8b82..a6a8af665 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -413,9 +413,9 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma // retry handler's gate so the two cannot disagree (issue #161). if reason := purchase.RedriveRefusalReason(&exec); reason != "" { row.RedriveUnsafe = true - // A definite provider refusal on every rec means nothing was - // bought, so the duplicate-purchase warning would mislead. - if purchase.IsDefiniteStoredRefusal(exec.Error) { + // Only a proven pre-send AWS refusal on every rec means nothing was + // bought; for any other outcome the duplicate-purchase warning stays. + if purchase.IsDefiniteAWSRefusal(exec.Recommendations, exec.Error) { row.OpsHint = "" } else { row.OpsHint = reason diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4e3bf89de..b2c5db01b 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -554,9 +554,11 @@ func summarizeExecuteFailure(executionID string, err error) (category, message s logRawExecuteFailure(executionID, err) category, message = purchase.SummarizeProviderError(err) if category == purchase.ErrCategoryUnknown { - // Server-authored refusals (for example "re-fetch the recommendation") - // carry no provider text and stay readable. - message = purchase.SanitizeStoredError(err.Error()) + // Only known server-authored refusals (for example "re-fetch the + // recommendation") stay readable; any other text is the category message. + if text, ok := purchase.ServerNoteMessage(err); ok { + message = text + } } return category, message } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 51220cb5a..c199c5e8c 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2771,7 +2771,7 @@ func TestHandler_getPurchaseDetails_WithTimestamps(t *testing.T) { ScheduledDate: scheduledDate, NotificationSent: ¬ificationSent, CompletedAt: &completedAt, - Error: "some error", + Error: "ramp not advanced: some error", } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) @@ -2793,7 +2793,7 @@ func TestHandler_getPurchaseDetails_WithTimestamps(t *testing.T) { assert.Equal(t, "completed", resultMap["status"]) assert.NotNil(t, resultMap["notification_sent"]) assert.NotNil(t, resultMap["completed_at"]) - assert.Equal(t, "some error", resultMap["error"]) + assert.Equal(t, "ramp not advanced: some error", resultMap["error"]) } // Tests for executePurchase diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go new file mode 100644 index 000000000..1f158754e --- /dev/null +++ b/internal/purchase/errnotes.go @@ -0,0 +1,81 @@ +package purchase + +import ( + "errors" + "regexp" + "strings" +) + +// serverNotePatterns lists every server-authored string stored into +// purchase_executions.error or a recommendation's error (enumerated from the +// assignments in purchase/execution.go, manager.go, reaper.go, recover_reaped.go, +// notifications.go, plan_step_attach.go, stranded_redrive.go, step_cap.go, +// due_age_cap.go, claim.go, ladder_execution_recheck.go and the api retry +// handler's email reasons). Anything else in a stored error is treated as +// provider text. Each pattern is anchored; free tails stop at the next "; ". +// The em dash in the reaper note is written as \x{2014}. +var serverNotePatterns = []string{ + `history_write_failed: commitment [^;\s]+ purchased but its history record failed to save`, + `recovered after reap; provider purchase succeeded`, + `ramp not advanced: [^;]*`, + `reaped after \d+m in [a-z_]+ state \x{2014} executor did not complete(?:; safe to retry)?`, + `execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck \(issue #632\)\. (?:Verify on the cloud provider that no commitment was created, then Retry\.|Retry is refused for this purchase because a repeat could buy a second commitment\. Verify on the cloud provider whether the commitment was created; if it was not, start a new purchase\.)`, + `refusing to execute retry attempt \d+: (?:` + redriveReasonPattern + `)`, + `stale Azure recommendation pricing; refresh recommendations and submit a new purchase(?:: [^;]*)?`, + `(?:execution [^:\s]+: )?plan step has no recommendations attached, so it cannot buy anything \(platform#631: plan steps cannot yet carry recommendations\)`, + `plan step refused by commitment cap(?:: [^;]*)?`, + `scheduled purchase skipped: it was due too long ago(?:: it was due at [^;]*\(SCHEDULED_PURCHASE_MAX_OVERDUE\); nothing was submitted, retry it if the purchase is still wanted)?`, + `purchase refused after claim: the claimed execution no longer passes the authorization checks(?:: [^;]*)?`, + `refusing to execute ladder purchase: [^;]*`, + `refusing to execute: ladder tranche lookup failed: [^;]*`, + `scheduled plan step reached its scheduled date without recommendations; no purchase was submitted`, + `no current recommendation matched plan [^;\s]+ step \d+ by [^;]* \(\d+-day grace after the plan date\)(?:; ` + skippedNotePattern + `)?`, + `target already met: target \d+, committed \d+(?:; ` + skippedNotePattern + `)?; \d+ failed row\(s\) of unknown outcome counted as committed`, + `selected recommendations do not resolve to a single cloud account: (?:\d+ selected recommendation\(s\) target \d+ cloud accounts \([^;]*\); submit one purchase per account|\d+ selected recommendation\(s\) carry no cloud_account_id while \d+ target account [^;]*)`, + `purchase was not successful`, + `EC2 recommendation for \S+ has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, + `zonal RI offering needs an availability zone; re-fetch the recommendation`, + `email notifier not configured for this deployment`, + `no notification email set in Settings \x{2192} General(?: and no account contact emails configured)?`, + `FROM_EMAIL not configured for this deployment`, +} + +const skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): [^;]*` + +var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + regexp.QuoteMeta(redriveReasonAzureSavingsPlan) + + `|provider "[^"]*" is not known to reject a duplicate purchase, so re-driving this could buy a second commitment` + +// A rec's ": " head, as the writer's per-rec join adds it. +var serverNoteRE = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNotePatterns, "|") + `)(?:; |$)`) + +// onlyServerNotes reports whether every "; "-separated item of s is an +// allowlisted server note. Matching consumes whole notes, so a "; " inside a +// provider message can never smuggle a tail through. +func onlyServerNotes(s string) bool { + for s != "" { + m := serverNoteRE.FindString(s) + if m == "" { + return false + } + s = s[len(m):] + } + return true +} + +// ServerNoteMessage returns err's text when it is a known server-authored +// refusal (a sentinel or an allowlisted note), so live responses can show it. +func ServerNoteMessage(err error) (string, bool) { + if err == nil { + return "", false + } + for _, sentinel := range []error{ErrPlanStepNoRecommendations, ErrStaleAzurePricing, ErrStepCommitmentCap, + ErrDueRowTooOld, ErrClaimedRowRefused, errAmbiguousAccountScope} { + if errors.Is(err, sentinel) { + return err.Error(), true + } + } + if onlyServerNotes(err.Error()) { + return err.Error(), true + } + return "", false +} diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go new file mode 100644 index 000000000..539a2a210 --- /dev/null +++ b/internal/purchase/errnotes_test.go @@ -0,0 +1,78 @@ +package purchase + +import ( + "fmt" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +// Every server-authored string stored into purchase_executions.error or a +// recommendation's error must come back verbatim. +func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { + fresh := &config.PurchaseExecution{} + recordHistoryAuditGap(fresh, "c-1", fmt.Errorf("db down")) + retry := &config.PurchaseExecution{RetryAttemptN: 2, Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}}} + azSP := &config.PurchaseExecution{RetryAttemptN: 1, Recommendations: []config.RecommendationRecord{{Provider: "azure", Service: "savingsplans"}}} + gcpRetry := &config.PurchaseExecution{RetryAttemptN: 1, Recommendations: []config.RecommendationRecord{{Provider: "weird"}}} + notes := []string{ + fresh.Error, + recoveredAfterReapNote, + "ramp not advanced: ladder step missing", + "reaped after 30m in running state — executor did not complete", + "reaped after 30m in approved state — executor did not complete; safe to retry", + strandedRecoveryError(&config.PurchaseExecution{}), + strandedRecoveryError(retry), + armedRedriveRefusal(retry).Error(), + armedRedriveRefusal(azSP).Error(), + armedRedriveRefusal(gcpRetry).Error(), + ErrStaleAzurePricing.Error(), + ErrPlanStepNoRecommendations.Error(), + fmt.Errorf("execution %s: %w", "00000000-0000-0000-0000-000000000000", ErrPlanStepNoRecommendations).Error(), + fmt.Errorf("%w: plan has no max_step_commitment set (auto-purchase of steps with recommendations is refused until one is set)", ErrStepCommitmentCap).Error(), + fmt.Errorf("%w: step total commitment 10.00 exceeds max_step_commitment 5.00", ErrStepCommitmentCap).Error(), + dueRowAgeRefusal(&config.PurchaseExecution{ScheduledDate: time.Now().Add(-100 * time.Hour)}, time.Hour, time.Now()).Error(), + fmt.Errorf("%w: %w", ErrClaimedRowRefused, fmt.Errorf("permission changed")).Error(), + "refusing to execute ladder purchase: its ladder run was canceled", + "refusing to execute ladder purchase: its tranche is paused", + "refusing to execute: ladder tranche lookup failed: timeout", + "scheduled plan step reached its scheduled date without recommendations; no purchase was submitted", + "no current recommendation matched plan 00000000-0000-0000-0000-000000000000 step 2 by 2026-01-02 (3-day grace after the plan date)", + "no current recommendation matched plan p step 2 by 2026-01-02 (3-day grace after the plan date); " + skippedNote([]string{"a\x1fb", "c"}), + "target already met: target 10, committed 12; 0 failed row(s) of unknown outcome counted as committed", + "target already met: target 10, committed 12; " + skippedNote([]string{"a"}) + "; 1 failed row(s) of unknown outcome counted as committed", + fmt.Errorf("%w: 2 selected recommendation(s) target 2 cloud accounts (a, b); submit one purchase per account", errAmbiguousAccountScope).Error(), + fmt.Errorf("%w: 1 selected recommendation(s) carry no cloud_account_id while 2 target account a", errAmbiguousAccountScope).Error(), + "purchase was not successful", + "EC2 recommendation for m5.large has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", + "zonal RI offering needs an availability zone; re-fetch the recommendation", + "email notifier not configured for this deployment", + "no notification email set in Settings → General and no account contact emails configured", + "no notification email set in Settings → General", + "FROM_EMAIL not configured for this deployment", + } + for _, n := range notes { + assert.NotEmpty(t, n) + assert.Equal(t, n, SanitizeStoredError(n), n) + assert.Equal(t, n, SanitizeStoredError(partialRunPrefix + "x: " + n)[len(partialRunPrefix)+3:], "behind a resource head: "+n) + } + joined := notes[0] + "; " + notes[1] + "; " + notes[6] + assert.Equal(t, joined, SanitizeStoredError(joined)) + assert.Equal(t, partialRunPrefix+joined, SanitizeStoredError(partialRunPrefix+joined)) +} + +func TestSanitizeStoredError_UnknownTextFailsClosed(t *testing.T) { + for _, in := range []string{ + "some unlisted text", + "history_write_failed: commitment c purchased; and then provider text", + "ramp not advanced: x; some tail", + "purchase failed: boom", + "send failed: SES said no", + "failed to get plan: db", + } { + out := SanitizeStoredError(in) + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), out, in) + } +} diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index 8827e9c53..9eb2fe2db 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -3,9 +3,12 @@ package purchase import ( "errors" "net/http" + "regexp" "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "google.golang.org/api/googleapi" ) @@ -35,18 +38,12 @@ func ErrCategoryMessage(category string) string { return errCategoryMessages[ErrCategoryUnknown] } -// IsDefiniteRefusal reports whether the provider clearly refused the -// purchase, so nothing was bought. -func IsDefiniteRefusal(category string) bool { - return category == ErrCategoryAccessDenied || category == ErrCategoryUnavailable || category == ErrCategoryLimit -} - -// SummarizeProviderError classifies a live error by type and returns its -// category and fixed message. +// SummarizeProviderError classifies a live error by its type or explicit code +// fields and returns its category and fixed message. func SummarizeProviderError(err error) (category, message string) { category = classifyTypedError(err) if category == ErrCategoryUnknown && err != nil { - category = classifyText(err.Error()) + category = classifyStoredText(err.Error()) } return category, ErrCategoryMessage(category) } @@ -55,17 +52,25 @@ func classifyTypedError(err error) string { var apiErr interface{ ErrorCode() string } var azErr *azcore.ResponseError var gErr *googleapi.Error + var authErr *azidentity.AuthenticationFailedError switch { + case errors.As(err, &authErr): + return ErrCategoryAccessDenied case errors.As(err, &apiErr): - if c := classifyText(apiErr.ErrorCode()); c != ErrCategoryUnknown { + if c := classifyCode(apiErr.ErrorCode()); c != ErrCategoryUnknown { return c } case errors.As(err, &azErr): - if c := classifyText(azErr.ErrorCode); c != ErrCategoryUnknown { + if c := classifyCode(azErr.ErrorCode); c != ErrCategoryUnknown { return c } return categoryForStatus(azErr.StatusCode) case errors.As(err, &gErr): + for _, item := range gErr.Errors { + if c := classifyCode(item.Reason); c != ErrCategoryUnknown { + return c + } + } return categoryForStatus(gErr.Code) } var httpErr interface{ HTTPStatusCode() int } @@ -88,15 +93,9 @@ func categoryForStatus(code int) string { } var ( - deniedNeedles = []string{"accessdenied", "unauthorized", "authorizationfailed", "forbidden", - "encoded authorization", "statuscode: 401", "statuscode: 403", "response 401", "response 403", "error 403"} - limitNeedles = []string{"limitexceeded", "quota", "throttl", "toomanyrequests", "statuscode: 429", "response 429", "error 429"} - unavailNeedles = []string{"capacity", "offeringnotfound", "notfound", "not available", - "notavailable", "statuscode: 404", "response 404", "error 404"} - // Substrings that only appear in provider-authored error text. - providerMarkers = []string{"operation error", "api error", "statuscode:", "requestid", "arn:aws", - "encoded authorization", "accessdenied", "unauthorizedoperation", "authorizationfailed", - "googleapi", "error code:", "response 401", "response 403", "response 404", "response 429", "response 5"} + deniedCodes = []string{"accessdenied", "unauthorized", "authorizationfailed", "forbidden"} + limitCodes = []string{"limitexceeded", "quota", "throttl", "toomanyrequests", "ratelimit"} + unavailCodes = []string{"capacity", "offeringnotfound", "notfound", "notavailable"} ) func containsAny(lower string, needles []string) bool { @@ -108,53 +107,122 @@ func containsAny(lower string, needles []string) bool { return false } -func classifyText(s string) string { - lower := strings.ToLower(s) +// classifyCode classifies one short error code or reason, never free text. +func classifyCode(code string) string { + lower := strings.ToLower(code) switch { - case containsAny(lower, deniedNeedles): + case containsAny(lower, deniedCodes): return ErrCategoryAccessDenied - case containsAny(lower, limitNeedles): + case containsAny(lower, limitCodes): return ErrCategoryLimit - case containsAny(lower, unavailNeedles): + case containsAny(lower, unavailCodes): return ErrCategoryUnavailable } return ErrCategoryUnknown } -func hasProviderMarker(s string) bool { - return containsAny(strings.ToLower(s), providerMarkers) +// Explicit code fields in the providers' own error formats. Words elsewhere +// (URLs, resource names, bodies) never count. +var ( + awsCodeRE = regexp.MustCompile(`api error ([A-Za-z0-9_.]+):`) + awsStatusRE = regexp.MustCompile(`StatusCode: (\d{3})`) + azCodeRE = regexp.MustCompile(`ERROR CODE: ([A-Za-z0-9_.-]+)`) + azStatusRE = regexp.MustCompile(`RESPONSE (\d{3}):`) + gcpReasonRE = regexp.MustCompile(`Reason: ([A-Za-z]+), Message:`) + gcpErrorRE = regexp.MustCompile(`googleapi: Error (\d{3}): [^\n]*?(?:, ([A-Za-z]+))?(?:\n|$)`) + definiteAWSRE = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9._-]*: )?operation error [^\n]*StatusCode: (4\d{2}),[^\n]*api error ([A-Za-z0-9_.]+): [^\n]*$`) +) + +func statusOf(re *regexp.Regexp, s string) int { + m := re.FindStringSubmatch(s) + if m == nil { + return 0 + } + n := 0 + for _, c := range m[1] { + n = n*10 + int(c-'0') + } + return n } -// IsDefiniteStoredRefusal reports whether every "; "-joined segment of a stored -// error is a definite provider refusal. One unknown or server-authored segment -// means that rec's outcome is not proven, so the answer is false. -func IsDefiniteStoredRefusal(s string) bool { - s = strings.TrimPrefix(s, partialRunPrefix) - for _, seg := range strings.Split(s, "; ") { - if !hasProviderMarker(seg) || !IsDefiniteRefusal(classifyText(seg)) { - return false +// classifyStoredText classifies a stored error string from its explicit code +// fields only; no code field means unknown. +func classifyStoredText(s string) string { + for _, re := range []*regexp.Regexp{awsCodeRE, azCodeRE, gcpErrorRE, gcpReasonRE} { + if c := classifyCode(lastGroup(re, s)); c != ErrCategoryUnknown { + return c + } + } + for _, re := range []*regexp.Regexp{awsStatusRE, azStatusRE, gcpErrorRE} { + if c := categoryForStatus(statusOf(re, s)); c != ErrCategoryUnknown { + return c } } - return s != "" + return ErrCategoryUnknown } -// SanitizeStoredError rewrites provider error text in an already-stored -// error string into its category message. Strings and "; "-joined segments -// without provider markers pass through unchanged. -func SanitizeStoredError(s string) string { - if !hasProviderMarker(s) { - return s +// lastGroup returns the last capture group of re's first match, or "". +func lastGroup(re *regexp.Regexp, s string) string { + m := re.FindStringSubmatch(s) + if len(m) < 2 { + return "" } - prefix := "" - if strings.HasPrefix(s, partialRunPrefix) { - prefix = partialRunPrefix - s = strings.TrimPrefix(s, partialRunPrefix) + return m[len(m)-1] +} + +// definiteAWSCodes are the AWS error codes returned before anything is +// created: refused credentials, throttling and rejected parameters. +var definiteAWSCodes = map[string]bool{ + "AccessDenied": true, "AccessDeniedException": true, "UnauthorizedOperation": true, "AuthFailure": true, + "UnrecognizedClientException": true, "InvalidClientTokenId": true, "ExpiredToken": true, "ExpiredTokenException": true, + "Throttling": true, "ThrottlingException": true, "RequestLimitExceeded": true, "TooManyRequestsException": true, + "LimitExceeded": true, "ReservedInstanceLimitExceeded": true, "ReservedInstancesLimitExceeded": true, + "ReservedDBInstanceQuotaExceeded": true, "ReservedCacheNodeQuotaExceeded": true, + "InvalidParameterValue": true, "InvalidParameterCombination": true, "InvalidParameter": true, + "ValidationError": true, "ValidationException": true, "MissingParameter": true, + "InvalidReservedInstancesOfferingId": true, "ReservedInstancesOfferingNotFound": true, + "ReservedDBInstancesOfferingNotFound": true, "ReservedCacheNodesOfferingNotFound": true, +} + +func definiteAWSSegment(seg string) bool { + if strings.Contains(seg, "outcome unknown") { + return false + } + m := definiteAWSRE.FindStringSubmatch(seg) + return len(m) == 3 && definiteAWSCodes[m[2]] +} + +// IsDefiniteAWSRefusal reports whether a stored error proves nothing was +// bought: every rec is AWS and every "; "-separated segment is an AWS 4xx +// with a listed pre-send code. Azure, GCP, 5xx, a missing code or a server +// note anywhere means the outcome is not proven, so the answer is false. +func IsDefiniteAWSRefusal(recs []config.RecommendationRecord, stored string) bool { + if len(recs) == 0 { + return false + } + for i := range recs { + if recs[i].Provider != "" && recs[i].Provider != "aws" { + return false + } } - segs := strings.Split(s, "; ") - for i, seg := range segs { - if hasProviderMarker(seg) { - segs[i] = ErrCategoryMessage(classifyText(seg)) + stored = strings.TrimPrefix(stored, partialRunPrefix) + for _, seg := range strings.Split(stored, "; ") { + if !definiteAWSSegment(seg) { + return false } } - return prefix + strings.Join(segs, "; ") + return stored != "" +} + +// SanitizeStoredError turns a stored error string into text safe to show: it +// passes through only when every item is an allowlisted server note, and +// otherwise the whole remainder becomes one category message. The partial-run +// prefix is kept. +func SanitizeStoredError(s string) string { + rest := strings.TrimPrefix(s, partialRunPrefix) + prefix := s[:len(s)-len(rest)] + if onlyServerNotes(rest) { + return s + } + return prefix + ErrCategoryMessage(classifyStoredText(rest)) } diff --git a/internal/purchase/errsummary_test.go b/internal/purchase/errsummary_test.go index 4299677d0..409d70cef 100644 --- a/internal/purchase/errsummary_test.go +++ b/internal/purchase/errsummary_test.go @@ -25,21 +25,6 @@ func TestSanitizeStoredError_AWS403(t *testing.T) { } } -func TestSanitizeStoredError_PassThrough(t *testing.T) { - for _, s := range []string{ - "history_write_failed: db timeout", - "recovered after reap; provider purchase succeeded", - "ramp not advanced: ladder step missing", - "reaped after 30m in running state, executor did not complete", - "refusing to execute retry attempt 2: AWS EC2 and Redshift purchases have no provider-side duplicate guard", - ErrStaleAzurePricing.Error(), - "stranded in approved state", - "", - } { - assert.Equal(t, s, SanitizeStoredError(s)) - } -} - func TestSanitizeStoredError_Samples(t *testing.T) { cases := map[string]string{ "RESPONSE 403: 403 Forbidden\nERROR CODE: AuthorizationFailed": ErrCategoryAccessDenied, @@ -57,10 +42,8 @@ func TestSanitizeStoredError_PartialRun(t *testing.T) { in := partialRunPrefix + "rec a: " + fakeAWS403 + "; rec b failed locally; " + "api error LimitExceeded: x arn:aws:iam::123456789012:role/r" out := SanitizeStoredError(in) assert.True(t, strings.HasPrefix(out, partialRunPrefix)) - assert.Contains(t, out, "rec b failed locally") - assert.Contains(t, out, ErrCategoryMessage(ErrCategoryLimit)) + assert.Equal(t, partialRunPrefix+ErrCategoryMessage(ErrCategoryAccessDenied), out) assert.NotContains(t, out, "123456789012") - assert.NotContains(t, out, "arn:") } func TestSummarizeProviderError_Typed(t *testing.T) { @@ -84,18 +67,3 @@ func TestSummarizeProviderError_Typed(t *testing.T) { assert.NotEmpty(t, msg) } } - -func TestIsDefiniteStoredRefusal(t *testing.T) { - cases := map[string]bool{ - fakeAWS403: true, - "t4g.nano: " + fakeAWS403 + "; t4g.micro: " + fakeAWS403: true, - partialRunPrefix + "t4g.nano: " + fakeAWS403: true, - "t4g.nano: purchase outcome unknown; t4g.micro: " + fakeAWS403: false, - "purchase outcome unknown": false, - "history_write_failed: commitment purchased": false, - "": false, - } - for in, want := range cases { - assert.Equal(t, want, IsDefiniteStoredRefusal(in), in) - } -} diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index c3630c68d..1f75a81b0 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -587,6 +587,12 @@ func RedriveRefusalReason(exec *config.PurchaseExecution) string { return "" } +// Fixed refusal texts, shared with the stored-error allowlist (errnotes.go). +const ( + redriveReasonAWSNoGuard = "AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" + redriveReasonAzureSavingsPlan = "Azure savings plans have no provider-side duplicate guard, so re-driving this purchase would buy a second savings plan that cannot be canceled" +) + // recRedriveRefusalReason returns the reason a single recommendation cannot be // safely re-driven, or "" when it can. Extracted from RedriveRefusalReason to // keep that function under the gocyclo budget and to make per-rec exclusions @@ -601,7 +607,7 @@ func recRedriveRefusalReason(rec config.RecommendationRecord) string { // "purchase outcome unknown". The commitment may exist, so a second // attempt must not be made until someone has checked the account. if awsPurchaseHasNoDuplicateGuard(rec.Service) { - return "AWS EC2 and Redshift purchases have no provider-side duplicate guard, and a lost response means the earlier attempt may already have bought the commitment. Check EC2 Reserved Instances or Redshift Reserved Nodes in the target account and region; if it is missing, start a new purchase instead of retrying" + return redriveReasonAWSNoGuard } return "" case "azure": @@ -610,7 +616,7 @@ func recRedriveRefusalReason(rec config.RecommendationRecord) string { // re-drive may create a duplicate. All other Azure services use // DoIdempotentPurchaseTwoStep (#729). if rec.Service == "savingsplans" || rec.Service == "savings-plans" { - return "Azure savings plans have no provider-side duplicate guard, so re-driving this purchase would buy a second savings plan that cannot be canceled" + return redriveReasonAzureSavingsPlan } return "" case "gcp": From 4d3756599c369c4731893f9c45ad3037b1114449 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:20:33 +0100 Subject: [PATCH 06/12] test(purchase): cover codeless Azure, GCP and AWS failure shapes Real azcore and azidentity errors, googleapi errors and AWS messages with a semicolon, checked on the stored, live and history paths. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/history_duplicate_hint_test.go | 29 +++ internal/api/purchase_failure_summary_test.go | 65 +++++++ internal/purchase/errsummary_provider_test.go | 181 ++++++++++++++++++ 3 files changed, 275 insertions(+) create mode 100644 internal/purchase/errsummary_provider_test.go diff --git a/internal/api/history_duplicate_hint_test.go b/internal/api/history_duplicate_hint_test.go index 874dc76d3..d145463f4 100644 --- a/internal/api/history_duplicate_hint_test.go +++ b/internal/api/history_duplicate_hint_test.go @@ -22,3 +22,32 @@ func TestHistoryRow_DuplicateGuardHint(t *testing.T) { allDenied := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: "t4g.nano: " + fakeAWSDenied + "; t4g.micro: " + fakeAWSDenied, Recommendations: ec2}, "", "") assert.NotContains(t, allDenied.OpsHint, "duplicate guard", "every rec refused hides the warning") } + +func TestHistoryRow_DuplicateHintKeptOutsideDefiniteAWSRefusals(t *testing.T) { + const gcpRate = "commitment creation failed: googleapi: Error 403: Rate Limit Exceeded, rateLimitExceeded" + const gcpNotFound = "commitment creation failed: googleapi: Error 404: operations/op-1 was not found, notFound" + azure500 := azureHTTPError(t, 500, "/providers/Microsoft.Capacity/reservationOrders/x").Error() + azure429 := "PUT https://management.azure.com/x\nRESPONSE 429: 429 Too Many\nERROR CODE: TooManyRequests" + aws503 := strings.Replace(fakeAWSDenied, "StatusCode: 403", "StatusCode: 503", 1) + ec2 := config.RecommendationRecord{Provider: "aws", Service: "ec2"} + azSP := config.RecommendationRecord{Provider: "azure", Service: "savingsplans"} + gcp := config.RecommendationRecord{Provider: "gcp", Service: "compute"} + cases := []struct { + name string + recs []config.RecommendationRecord + err string + }{ + {"gcp rate limit", []config.RecommendationRecord{gcp, ec2}, gcpRate}, + {"gcp not found", []config.RecommendationRecord{gcp, ec2}, gcpNotFound}, + {"azure 500 on a capacity url", []config.RecommendationRecord{azSP}, azure500}, + {"azure 429", []config.RecommendationRecord{azSP}, azure429}, + {"aws 5xx", []config.RecommendationRecord{ec2}, aws503}, + {"aws without a code", []config.RecommendationRecord{ec2}, "operation error EC2: X, https response error StatusCode: 403"}, + {"non-aws rec beside a definite aws refusal", []config.RecommendationRecord{ec2, gcp}, fakeAWSDenied}, + } + for _, c := range cases { + row := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: c.err, Recommendations: c.recs}, "", "") + assert.True(t, row.RedriveUnsafe, c.name) + assert.Contains(t, row.OpsHint, "duplicate guard", c.name) + } +} diff --git a/internal/api/purchase_failure_summary_test.go b/internal/api/purchase_failure_summary_test.go index 3533bc65a..666466f1e 100644 --- a/internal/api/purchase_failure_summary_test.go +++ b/internal/api/purchase_failure_summary_test.go @@ -3,8 +3,12 @@ package api import ( "encoding/json" "errors" + "io" + "net/http" + "strings" "testing" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -56,6 +60,16 @@ func TestProviderDenial_NeverReachesUserSurfaces(t *testing.T) { "the hint still resolves from the raw text") }) + t.Run("partially completed and audit-gap rows", func(t *testing.T) { + for _, status := range []string{"partially_completed", "completed"} { + row := executionToHistoryRow(config.PurchaseExecution{ + ExecutionID: execID, Status: status, Error: "t4g.nano: " + fakeAWSDenied, + }, "", "") + assert.NotEmpty(t, row.StatusDescription, status) + assertNoProviderText(t, status+" status_description", row.StatusDescription) + } + }) + t.Run("purchase details", func(t *testing.T) { exec := &config.PurchaseExecution{ExecutionID: execID, Status: "failed", Error: fakeAWSDenied, Recommendations: []config.RecommendationRecord{rec}} @@ -98,3 +112,54 @@ func TestExecuteFailureError_PartialAndAuditLossHideProviderText(t *testing.T) { assert.Equal(t, 500, ce.code) assertNoProviderText(t, "audit loss", ce.Error()) } + +const ( + fakeSubID = "00000000-0000-0000-0000-000000000000" + fakeTenant = "11111111-1111-1111-1111-111111111111" +) + +func azureHTTPError(t *testing.T, status int, path string) error { + t.Helper() + req, err := http.NewRequest(http.MethodPut, "https://management.azure.com"+path, nil) + require.NoError(t, err) + return runtime.NewResponseError(&http.Response{ + StatusCode: status, Status: http.StatusText(status), Request: req, + Header: http.Header{"Content-Type": {"text/html"}}, + Body: io.NopCloser(strings.NewReader("gateway error")), + }) +} + +func assertNoAzureText(t *testing.T, where, s string) { + t.Helper() + for _, n := range []string{fakeSubID, fakeTenant, "management.azure.com", "gateway", "ERROR CODE"} { + assert.NotContains(t, s, n, where) + } +} + +func TestCodelessAzureError_NeverReachesUserSurfaces(t *testing.T) { + for _, status := range []int{400, 409, 412} { + raw := azureHTTPError(t, status, "/subscriptions/"+fakeSubID+"/providers/Microsoft.BillingBenefits/savingsPlanOrderAliases/x") + rec := config.RecommendationRecord{Provider: "azure", Service: "savingsplans", Error: raw.Error()} + ce, ok := IsClientError(executeFailureError("e1", "executed", &config.PurchaseExecution{Status: "failed"}, raw)) + require.True(t, ok) + assertNoAzureText(t, "live", ce.Error()) + row := executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: raw.Error(), Recommendations: []config.RecommendationRecord{rec}}, "", "") + assert.NotEmpty(t, row.StatusDescription) + assertNoAzureText(t, "history", row.StatusDescription) + resp, err := json.Marshal(buildPurchaseDetailsResponse(&config.PurchaseExecution{Status: "failed", Error: raw.Error(), Recommendations: []config.RecommendationRecord{rec}}, "plan")) + require.NoError(t, err) + assertNoAzureText(t, "details", string(resp)) + } +} + +func TestAzureIdentityText_NeverReachesUserSurfaces(t *testing.T) { + raw := "ClientSecretCredential authentication failed. POST https://login.microsoftonline.com/" + fakeTenant + "/oauth2/v2.0/token\n" + + "RESPONSE 400: 400 Bad Request\n{\"error_description\":\"AADSTS700016: Application not found. Trace ID: 22222222 Correlation ID: 33333333\"}" + ce, ok := IsClientError(executeFailureError("e1", "executed", &config.PurchaseExecution{Status: "failed"}, errors.New(raw))) + require.True(t, ok) + for _, s := range []string{ce.Error(), executionToHistoryRow(config.PurchaseExecution{Status: "failed", Error: raw}, "", "").StatusDescription} { + for _, n := range []string{fakeTenant, "login.microsoftonline.com", "AADSTS", "Trace ID", "Correlation ID"} { + assert.NotContains(t, s, n) + } + } +} diff --git a/internal/purchase/errsummary_provider_test.go b/internal/purchase/errsummary_provider_test.go new file mode 100644 index 000000000..55660ea56 --- /dev/null +++ b/internal/purchase/errsummary_provider_test.go @@ -0,0 +1,181 @@ +package purchase + +import ( + "context" + "io" + "net/http" + "strings" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" +) + +const ( + fakeSub = "00000000-0000-0000-0000-000000000000" + fakeTenant = "11111111-1111-1111-1111-111111111111" +) + +var providerLeakNeedles = []string{fakeSub, fakeTenant, "management.azure.com", "login.microsoftonline.com", + "Trace ID", "Correlation ID", "AADSTS", "123456789012", "gateway"} + +func assertNoProviderLeak(t *testing.T, where, s string) { + t.Helper() + for _, n := range providerLeakNeedles { + assert.NotContains(t, s, n, where) + } +} + +func azureResponseError(t *testing.T, status int, path, contentType, body string) error { + t.Helper() + req, err := http.NewRequest(http.MethodPut, "https://management.azure.com"+path, nil) + require.NoError(t, err) + resp := &http.Response{ + StatusCode: status, Status: http.StatusText(status), Request: req, + Header: http.Header{"Content-Type": {contentType}}, + Body: io.NopCloser(strings.NewReader(body)), + } + return runtime.NewResponseError(resp) +} + +type fakeTransport struct{ status int } + +func (f fakeTransport) Do(r *http.Request) (*http.Response, error) { + body := `{"error":"unauthorized_client","error_description":"AADSTS700016: Application not found in tenant ` + fakeTenant + `. Trace ID: 22222222 Correlation ID: 33333333"}` + return &http.Response{StatusCode: f.status, Status: "400 Bad Request", Request: r, + Header: http.Header{"Content-Type": {"application/json"}}, Body: io.NopCloser(strings.NewReader(body))}, nil +} + +func aadError(t *testing.T) error { + t.Helper() + cred, err := azidentity.NewClientSecretCredential(fakeTenant, fakeSub, "fake-secret", &azidentity.ClientSecretCredentialOptions{ + ClientOptions: azcore.ClientOptions{Transport: fakeTransport{status: 400}}, + DisableInstanceDiscovery: true, + }) + require.NoError(t, err) + _, err = cred.GetToken(context.Background(), policy.TokenRequestOptions{Scopes: []string{"https://management.azure.com/.default"}}) + require.Error(t, err) + return err +} + +func TestAzureCodelessErrors_StoredAndLiveAreSanitized(t *testing.T) { + for _, status := range []int{400, 409, 412} { + err := azureResponseError(t, status, "/subscriptions/"+fakeSub+"/providers/Microsoft.Capacity/reservationOrders", "text/html", "gateway error") + require.Contains(t, err.Error(), "ERROR CODE UNAVAILABLE") + stored := SanitizeStoredError(err.Error()) + assertNoProviderLeak(t, "stored", stored) + assert.NotEmpty(t, stored) + _, live := SummarizeProviderError(err) + assertNoProviderLeak(t, "live", live) + _, liveStored := SummarizeProviderError(err) + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), liveStored) + } +} + +func TestAzureIdentityFailure_StoredAndLiveAreSanitized(t *testing.T) { + err := aadError(t) + var afe *azidentity.AuthenticationFailedError + require.ErrorAs(t, err, &afe) + require.Contains(t, err.Error(), "AADSTS700016") + assertNoProviderLeak(t, "stored", SanitizeStoredError(err.Error())) + cat, msg := SummarizeProviderError(err) + assert.Equal(t, ErrCategoryAccessDenied, cat) + assertNoProviderLeak(t, "live", msg) +} + +// A 500 whose URL names Microsoft.Capacity is an unknown outcome, never "not available". +func TestAzure500OnCapacityURL_IsUnknown(t *testing.T) { + err := azureResponseError(t, 500, "/providers/Microsoft.Capacity/reservationOrders/x", "application/json", `{"error":{"code":"InternalServerError","message":"boom"}}`) + cat, _ := SummarizeProviderError(err) + assert.Equal(t, ErrCategoryUnknown, cat) + assert.Equal(t, ErrCategoryUnknown, classifyStoredText(err.Error())) +} + +func TestClassifyStoredText_IgnoresWordsOutsideCodeFields(t *testing.T) { + for _, s := range []string{ + "PUT https://management.azure.com/providers/Microsoft.Capacity/x\nRESPONSE 500: 500 Internal\nERROR CODE: InternalServerError", + "something about quota and forbidden and notfound", + "operation error EC2: X, https response error StatusCode: 500, api error InternalError: capacity forbidden", + } { + assert.Equal(t, ErrCategoryUnknown, classifyStoredText(s), s) + } +} + +func TestGoogleAPIErrors_ClassifyByCodeAndReason(t *testing.T) { + rate := &googleapi.Error{Code: 403, Message: "Rate Limit Exceeded", Errors: []googleapi.ErrorItem{{Reason: "rateLimitExceeded", Message: "Rate Limit Exceeded"}}} + cat, _ := SummarizeProviderError(rate) + assert.Equal(t, ErrCategoryLimit, cat) + assert.Equal(t, ErrCategoryLimit, classifyStoredText(rate.Error())) + nf := &googleapi.Error{Code: 404, Message: "operations/op-1 was not found", Errors: []googleapi.ErrorItem{{Reason: "notFound", Message: "operations/op-1 was not found"}}} + assert.Equal(t, ErrCategoryUnavailable, classifyStoredText(nf.Error())) + multi := &googleapi.Error{Code: 403, Message: "m", Errors: []googleapi.ErrorItem{{Reason: "rateLimitExceeded", Message: "a"}, {Reason: "x", Message: "b"}}} + assert.Equal(t, ErrCategoryLimit, classifyStoredText(multi.Error())) +} + +func TestSanitizeStoredError_WholeRemainderReplaced(t *testing.T) { + const awsWithSemicolon = "operation error EC2: X, https response error StatusCode: 400, api error InvalidParameterValue: offering rejected; reserved for account 123456789012 in us-east-1" + for _, in := range []string{ + awsWithSemicolon, + "t4g.nano: " + awsWithSemicolon, + "t4g.nano: " + awsWithSemicolon + "; t4g.micro: " + awsWithSemicolon, + "history_write_failed: commitment c-1 purchased but its history record failed to save; " + awsWithSemicolon, + "plain provider words with no marker at all", + } { + out := SanitizeStoredError(in) + assert.NotContains(t, out, "123456789012", in) + assert.NotContains(t, out, "reserved for", in) + assert.NotEmpty(t, out) + assert.Equal(t, 1, strings.Count(out, "the provider"), in) + } + out := SanitizeStoredError(partialRunPrefix + "t4g.nano: " + awsWithSemicolon) + assert.True(t, strings.HasPrefix(out, partialRunPrefix)) + assert.NotContains(t, out, "123456789012") + assert.Equal(t, "", SanitizeStoredError("")) +} + +func TestIsDefiniteAWSRefusal(t *testing.T) { + aws := func(status, code string) string { + return "operation error EC2: X, https response error StatusCode: " + status + ", RequestID: r, api error " + code + ": msg" + } + azure429 := "PUT https://management.azure.com/x\nRESPONSE 429: 429 Too Many\nERROR CODE: TooManyRequests" + gcp := "commitment creation failed: googleapi: Error 403: Rate Limit Exceeded, rateLimitExceeded" + cases := []struct { + name string + recs []string + err string + want bool + }{ + {"aws 403 denied", []string{"aws"}, aws("403", "UnauthorizedOperation"), true}, + {"legacy empty provider", []string{""}, aws("403", "AccessDenied"), true}, + {"aws throttle 400", []string{"aws"}, aws("400", "Throttling"), true}, + {"aws invalid offering", []string{"aws"}, aws("400", "InvalidParameterValue"), true}, + {"per-rec segments", []string{"aws", "aws"}, "a: " + aws("403", "AccessDenied") + "; b: " + aws("400", "Throttling"), true}, + {"partial prefix", []string{"aws"}, partialRunPrefix + "a: " + aws("403", "AccessDenied"), true}, + {"5xx", []string{"aws"}, aws("503", "AccessDenied"), false}, + {"no code", []string{"aws"}, "operation error EC2: X, https response error StatusCode: 403", false}, + {"unlisted code", []string{"aws"}, aws("400", "SomethingOdd"), false}, + {"azure rec", []string{"azure"}, azure429, false}, + {"azure text on aws rec", []string{"aws"}, azure429, false}, + {"gcp rec", []string{"gcp"}, gcp, false}, + {"mixed recs", []string{"aws", "gcp"}, aws("403", "AccessDenied"), false}, + {"server note", []string{"aws"}, "history_write_failed: commitment c purchased but its history record failed to save", false}, + {"unknown outcome wrapper", []string{"aws"}, "purchase outcome unknown: the request may have been sent; reconcile before retrying: " + aws("400", "Throttling"), false}, + {"one unknown segment", []string{"aws", "aws"}, "a: " + aws("403", "AccessDenied") + "; b: purchase outcome unknown", false}, + {"semicolon in message", []string{"aws"}, aws("400", "InvalidParameterValue") + "; reserved elsewhere", false}, + {"no recs", nil, aws("403", "AccessDenied"), false}, + {"empty", []string{"aws"}, "", false}, + } + for _, c := range cases { + var recs []config.RecommendationRecord + for _, p := range c.recs { + recs = append(recs, config.RecommendationRecord{Provider: p}) + } + assert.Equal(t, c.want, IsDefiniteAWSRefusal(recs, c.err), c.name) + } +} From b851ab54bf2d8bf2bf31c31ca5bc4228a8692f0b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:23:57 +0100 Subject: [PATCH 07/12] test(purchase): name the provider-detail test helpers plainly Co-Authored-By: Claude Opus 5.5 --- internal/purchase/errsummary_provider_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/purchase/errsummary_provider_test.go b/internal/purchase/errsummary_provider_test.go index 55660ea56..f484146f5 100644 --- a/internal/purchase/errsummary_provider_test.go +++ b/internal/purchase/errsummary_provider_test.go @@ -22,12 +22,12 @@ const ( fakeTenant = "11111111-1111-1111-1111-111111111111" ) -var providerLeakNeedles = []string{fakeSub, fakeTenant, "management.azure.com", "login.microsoftonline.com", +var providerDetailNeedles = []string{fakeSub, fakeTenant, "management.azure.com", "login.microsoftonline.com", "Trace ID", "Correlation ID", "AADSTS", "123456789012", "gateway"} -func assertNoProviderLeak(t *testing.T, where, s string) { +func assertNoProviderDetail(t *testing.T, where, s string) { t.Helper() - for _, n := range providerLeakNeedles { + for _, n := range providerDetailNeedles { assert.NotContains(t, s, n, where) } } @@ -69,10 +69,10 @@ func TestAzureCodelessErrors_StoredAndLiveAreSanitized(t *testing.T) { err := azureResponseError(t, status, "/subscriptions/"+fakeSub+"/providers/Microsoft.Capacity/reservationOrders", "text/html", "gateway error") require.Contains(t, err.Error(), "ERROR CODE UNAVAILABLE") stored := SanitizeStoredError(err.Error()) - assertNoProviderLeak(t, "stored", stored) + assertNoProviderDetail(t, "stored", stored) assert.NotEmpty(t, stored) _, live := SummarizeProviderError(err) - assertNoProviderLeak(t, "live", live) + assertNoProviderDetail(t, "live", live) _, liveStored := SummarizeProviderError(err) assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), liveStored) } @@ -83,10 +83,10 @@ func TestAzureIdentityFailure_StoredAndLiveAreSanitized(t *testing.T) { var afe *azidentity.AuthenticationFailedError require.ErrorAs(t, err, &afe) require.Contains(t, err.Error(), "AADSTS700016") - assertNoProviderLeak(t, "stored", SanitizeStoredError(err.Error())) + assertNoProviderDetail(t, "stored", SanitizeStoredError(err.Error())) cat, msg := SummarizeProviderError(err) assert.Equal(t, ErrCategoryAccessDenied, cat) - assertNoProviderLeak(t, "live", msg) + assertNoProviderDetail(t, "live", msg) } // A 500 whose URL names Microsoft.Capacity is an unknown outcome, never "not available". From 1c2df3026b87f1cb0d58979db562ddba03e6d066 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 16:54:19 +0100 Subject: [PATCH 08/12] fix(purchase): show only the fixed head of server notes Allowlisted notes no longer pass a free-text tail. Tails are pinned to closed formats (digits, ISO dates, id lists, fixed phrases). A note whose tail can carry a wrapped store or config error (ramp not advanced, ladder tranche lookup, ladder global config, stale Azure pricing, commitment cap and claim refusals) now shows only its fixed head when the tail is not a closed format, instead of the whole item or the category fallback. Refs #1721 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_purchases_test.go | 2 +- internal/api/purchase_failure_notes_test.go | 57 +++++++++++++++ internal/purchase/errnotes.go | 78 ++++++++++++++------- internal/purchase/errnotes_tail_test.go | 51 ++++++++++++++ internal/purchase/errnotes_test.go | 7 +- internal/purchase/errsummary.go | 4 +- 6 files changed, 167 insertions(+), 32 deletions(-) create mode 100644 internal/api/purchase_failure_notes_test.go create mode 100644 internal/purchase/errnotes_tail_test.go diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index c199c5e8c..9faa713b9 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -2793,7 +2793,7 @@ func TestHandler_getPurchaseDetails_WithTimestamps(t *testing.T) { assert.Equal(t, "completed", resultMap["status"]) assert.NotNil(t, resultMap["notification_sent"]) assert.NotNil(t, resultMap["completed_at"]) - assert.Equal(t, "ramp not advanced: some error", resultMap["error"]) + assert.Equal(t, "ramp not advanced", resultMap["error"]) } // Tests for executePurchase diff --git a/internal/api/purchase_failure_notes_test.go b/internal/api/purchase_failure_notes_test.go new file mode 100644 index 000000000..5149b3a22 --- /dev/null +++ b/internal/api/purchase_failure_notes_test.go @@ -0,0 +1,57 @@ +package api + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const fakeConnectErr = "failed to connect to `host=db.internal.example user=cudly_app database=cudly`: " + + "dial error (dial tcp 10.0.0.5:5432: connect: connection refused)" + +func assertNoConnectText(t *testing.T, where, s string) { + t.Helper() + for _, n := range []string{"db.internal.example", "cudly_app", "10.0.0.5", "dial tcp"} { + assert.NotContains(t, s, n, where) + } +} + +func TestTailedNotesWithStoreErrors_NeverReachUserSurfaces(t *testing.T) { + notes := []string{ + "ramp not advanced: " + fakeConnectErr, + "refusing to execute: ladder tranche lookup failed: " + fakeConnectErr, + "refusing to execute ladder purchase: global config unreadable: " + fakeConnectErr, + purchase.ErrStaleAzurePricing.Error() + ": cannot read the latest collection state to confirm Azure prices are current: " + fakeConnectErr, + } + for _, note := range notes { + name := note[:strings.Index(note, ":")] + t.Run(name, func(t *testing.T) { + row := executionToHistoryRow(config.PurchaseExecution{ExecutionID: "e-1", Status: "failed", Error: note}, "", "") + assertNoConnectText(t, "status_description", row.StatusDescription) + assert.NotEmpty(t, row.StatusDescription) + + exec := &config.PurchaseExecution{ExecutionID: "e-1", Status: "failed", Error: note} + body, err := json.Marshal(buildPurchaseDetailsResponse(exec, "plan")) + require.NoError(t, err) + assertNoConnectText(t, "details", string(body)) + + creator := retryCallerID + failed := &config.PurchaseExecution{ExecutionID: retryExecID, Status: "failed", CreatedByUserID: &creator, + Error: "FROM_EMAIL not configured for this deployment; " + note} + handler, _, _ := buildSessionRetryHandler(t, failed, &Session{UserID: retryCallerID}, false, true) + _, rErr := handler.retryPurchase(t.Context(), sessionRetryReq(), retryExecID) + ce, ok := IsClientError(rErr) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + raw, mErr := json.Marshal(ce.Details()) + require.NoError(t, mErr) + assertNoConnectText(t, "retry 409", string(raw)+ce.Error()) + assert.NotEmpty(t, ce.Details()["failure_reason"]) + }) + } +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 1f158754e..081937809 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -12,26 +12,24 @@ import ( // notifications.go, plan_step_attach.go, stranded_redrive.go, step_cap.go, // due_age_cap.go, claim.go, ladder_execution_recheck.go and the api retry // handler's email reasons). Anything else in a stored error is treated as -// provider text. Each pattern is anchored; free tails stop at the next "; ". +// provider text. Each pattern is anchored and closed: no pattern matches free text or a newline. // The em dash in the reaper note is written as \x{2014}. var serverNotePatterns = []string{ `history_write_failed: commitment [^;\s]+ purchased but its history record failed to save`, `recovered after reap; provider purchase succeeded`, - `ramp not advanced: [^;]*`, `reaped after \d+m in [a-z_]+ state \x{2014} executor did not complete(?:; safe to retry)?`, `execution was approved but its purchase run was interrupted before completing and never finalized; failed by the recovery sweep so it is not silently stuck \(issue #632\)\. (?:Verify on the cloud provider that no commitment was created, then Retry\.|Retry is refused for this purchase because a repeat could buy a second commitment\. Verify on the cloud provider whether the commitment was created; if it was not, start a new purchase\.)`, `refusing to execute retry attempt \d+: (?:` + redriveReasonPattern + `)`, - `stale Azure recommendation pricing; refresh recommendations and submit a new purchase(?:: [^;]*)?`, + `stale Azure recommendation pricing; refresh recommendations and submit a new purchase(?:: the latest collection state is missing, so Azure prices cannot be confirmed current)?`, `(?:execution [^:\s]+: )?plan step has no recommendations attached, so it cannot buy anything \(platform#631: plan steps cannot yet carry recommendations\)`, - `plan step refused by commitment cap(?:: [^;]*)?`, - `scheduled purchase skipped: it was due too long ago(?:: it was due at [^;]*\(SCHEDULED_PURCHASE_MAX_OVERDUE\); nothing was submitted, retry it if the purchase is still wanted)?`, - `purchase refused after claim: the claimed execution no longer passes the authorization checks(?:: [^;]*)?`, - `refusing to execute ladder purchase: [^;]*`, - `refusing to execute: ladder tranche lookup failed: [^;]*`, + `plan step refused by commitment cap(?:: plan has no max_step_commitment set \(auto-purchase of steps with recommendations is refused until one is set\)|: step total commitment \d+\.\d{2} exceeds max_step_commitment \d+\.\d{2})?`, + `scheduled purchase skipped: it was due too long ago(?:: it was due at \d{4}-\d{2}-\d{2}T[0-9:.]+Z, [0-9a-z.]+ ago, which is more than the [0-9a-z.]+ limit \(SCHEDULED_PURCHASE_MAX_OVERDUE\); nothing was submitted, retry it if the purchase is still wanted)?`, + `purchase refused after claim: the claimed execution no longer passes the authorization checks`, + `refusing to execute ladder purchase: (?:global config unavailable|ladder planning is switched off|ladder execution is switched off|its tranche status is unknown|its tranche is [a-z_]+|its ladder config no longer exists|its ladder config is disabled|ladder config mode is "[a-z_]*", not auto_approve|its ladder run no longer exists|its ladder run was canceled)`, `scheduled plan step reached its scheduled date without recommendations; no purchase was submitted`, - `no current recommendation matched plan [^;\s]+ step \d+ by [^;]* \(\d+-day grace after the plan date\)(?:; ` + skippedNotePattern + `)?`, + `no current recommendation matched plan [^;\s]+ step \d+ by \d{4}-\d{2}-\d{2} \(\d+-day grace after the plan date\)(?:; ` + skippedNotePattern + `)?`, `target already met: target \d+, committed \d+(?:; ` + skippedNotePattern + `)?; \d+ failed row\(s\) of unknown outcome counted as committed`, - `selected recommendations do not resolve to a single cloud account: (?:\d+ selected recommendation\(s\) target \d+ cloud accounts \([^;]*\); submit one purchase per account|\d+ selected recommendation\(s\) carry no cloud_account_id while \d+ target account [^;]*)`, + `selected recommendations do not resolve to a single cloud account: (?:\d+ selected recommendation\(s\) target \d+ cloud accounts \(` + idListPattern + `\); submit one purchase per account|\d+ selected recommendation\(s\) carry no cloud_account_id while \d+ target account ` + idPattern + `)`, `purchase was not successful`, `EC2 recommendation for \S+ has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, `zonal RI offering needs an availability zone; re-fetch the recommendation`, @@ -40,7 +38,23 @@ var serverNotePatterns = []string{ `FROM_EMAIL not configured for this deployment`, } -const skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): [^;]*` +const ( + idPattern = `[^;\s,()]+` + idListPattern = idPattern + `(?:, ` + idPattern + `)*` + skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): ` + idListPattern +) + +// serverNoteHeads are server notes whose tail can carry a wrapped store or +// config error. Only the fixed head is ever shown; the tail is dropped. +var serverNoteHeads = []string{ + `ramp not advanced`, + `refusing to execute: ladder tranche lookup failed`, + `refusing to execute ladder purchase: global config unreadable`, + `stale Azure recommendation pricing; refresh recommendations and submit a new purchase`, + `plan step refused by commitment cap`, + `purchase refused after claim: the claimed execution no longer passes the authorization checks`, + `scheduled purchase skipped: it was due too long ago`, +} var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + regexp.QuoteMeta(redriveReasonAzureSavingsPlan) + `|provider "[^"]*" is not known to reject a duplicate purchase, so re-driving this could buy a second commitment` @@ -48,22 +62,37 @@ var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + reg // A rec's ": " head, as the writer's per-rec join adds it. var serverNoteRE = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNotePatterns, "|") + `)(?:; |$)`) -// onlyServerNotes reports whether every "; "-separated item of s is an -// allowlisted server note. Matching consumes whole notes, so a "; " inside a -// provider message can never smuggle a tail through. -func onlyServerNotes(s string) bool { +var serverNoteHeadRE = regexp.MustCompile(`^((?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNoteHeads, "|") + `))(?:: [^;]*)?(?:; |$)`) + +// renderServerNotes returns s with every "; "-separated item either kept +// verbatim (a closed allowlisted note) or cut to its fixed head (a known note +// whose tail is not closed). ok is false when any item is neither. Matching +// consumes whole notes, so a "; " inside a provider message cannot smuggle a +// tail through. +func renderServerNotes(s string) (string, bool) { + var out strings.Builder for s != "" { - m := serverNoteRE.FindString(s) - if m == "" { - return false + if m := serverNoteRE.FindString(s); m != "" { + out.WriteString(m) + s = s[len(m):] + continue } - s = s[len(m):] + loc := serverNoteHeadRE.FindStringSubmatchIndex(s) + if loc == nil { + return "", false + } + out.WriteString(s[loc[2]:loc[3]]) + if strings.HasSuffix(s[:loc[1]], "; ") { + out.WriteString("; ") + } + s = s[loc[1]:] } - return true + return out.String(), true } -// ServerNoteMessage returns err's text when it is a known server-authored -// refusal (a sentinel or an allowlisted note), so live responses can show it. +// ServerNoteMessage returns fixed server text for a known server-authored +// refusal so live responses can show it. A sentinel yields only its own text, +// never the wrapped chain; any other error must be allowlisted notes. func ServerNoteMessage(err error) (string, bool) { if err == nil { return "", false @@ -74,8 +103,5 @@ func ServerNoteMessage(err error) (string, bool) { return err.Error(), true } } - if onlyServerNotes(err.Error()) { - return err.Error(), true - } - return "", false + return renderServerNotes(err.Error()) } diff --git a/internal/purchase/errnotes_tail_test.go b/internal/purchase/errnotes_tail_test.go new file mode 100644 index 000000000..1b1ccdc4c --- /dev/null +++ b/internal/purchase/errnotes_tail_test.go @@ -0,0 +1,51 @@ +package purchase + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +const fakeConnectError = "failed to connect to `host=db.internal.example user=cudly_app database=cudly`: " + + "dial error (dial tcp 10.0.0.5:5432: connect: connection refused)" + +var connectNeedles = []string{"db.internal.example", "cudly_app", "10.0.0.5", "dial", "connect"} + +func assertNoConnectDetail(t *testing.T, where, s string) { + t.Helper() + for _, n := range connectNeedles { + assert.NotContains(t, s, n, where) + } +} + +// A note whose tail is not a closed format shows only its fixed head. +func TestSanitizeStoredError_TailedNotesShowOnlyTheirHead(t *testing.T) { + cases := []struct{ in, want string }{ + {"ramp not advanced: " + fakeConnectError, "ramp not advanced"}, + {"ramp not advanced: " + fakeConnectError + "\nsecond line", "ramp not advanced"}, + {"refusing to execute: ladder tranche lookup failed: " + fakeConnectError, "refusing to execute: ladder tranche lookup failed"}, + {"refusing to execute ladder purchase: global config unreadable: " + fakeConnectError, "refusing to execute ladder purchase: global config unreadable"}, + {ErrStaleAzurePricing.Error() + ": cannot read the latest collection state to confirm Azure prices are current: " + fakeConnectError, ErrStaleAzurePricing.Error()}, + {ErrStaleAzurePricing.Error() + ": Azure recommendation \"r-1\" no longer has the approved price", ErrStaleAzurePricing.Error()}, + {ErrStepCommitmentCap.Error() + ": recommendation \"r-1\" is not priced in USD, so it cannot be compared against the USD cap", ErrStepCommitmentCap.Error()}, + {ErrClaimedRowRefused.Error() + ": " + fakeConnectError, ErrClaimedRowRefused.Error()}, + {"x: ramp not advanced: " + fakeConnectError + "; recovered after reap; provider purchase succeeded", "x: ramp not advanced; recovered after reap; provider purchase succeeded"}, + {partialRunPrefix + "ramp not advanced: " + fakeConnectError, partialRunPrefix + "ramp not advanced"}, + } + for _, c := range cases { + out := SanitizeStoredError(c.in) + assertNoConnectDetail(t, c.in, out) + assert.Equal(t, c.want, out, c.in) + } +} + +func TestSanitizeStoredError_NoPatternMatchesANewline(t *testing.T) { + for _, in := range []string{ + "purchase refused after claim: the claimed execution no longer passes the authorization checks\narn:aws:iam::123456789012:role/x", + "refusing to execute ladder purchase: its tranche is paused\narn:aws:iam::123456789012:role/x", + "selected recommendations do not resolve to a single cloud account: 1 selected recommendation(s) carry no cloud_account_id while 2 target account a\nb", + "no current recommendation matched plan p step 2 by 2026-01-02\n(3-day grace after the plan date)", + } { + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), in) + } +} diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 539a2a210..a8e366396 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -20,7 +20,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { notes := []string{ fresh.Error, recoveredAfterReapNote, - "ramp not advanced: ladder step missing", + "ramp not advanced", "reaped after 30m in running state — executor did not complete", "reaped after 30m in approved state — executor did not complete; safe to retry", strandedRecoveryError(&config.PurchaseExecution{}), @@ -34,10 +34,11 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { fmt.Errorf("%w: plan has no max_step_commitment set (auto-purchase of steps with recommendations is refused until one is set)", ErrStepCommitmentCap).Error(), fmt.Errorf("%w: step total commitment 10.00 exceeds max_step_commitment 5.00", ErrStepCommitmentCap).Error(), dueRowAgeRefusal(&config.PurchaseExecution{ScheduledDate: time.Now().Add(-100 * time.Hour)}, time.Hour, time.Now()).Error(), - fmt.Errorf("%w: %w", ErrClaimedRowRefused, fmt.Errorf("permission changed")).Error(), + ErrClaimedRowRefused.Error(), "refusing to execute ladder purchase: its ladder run was canceled", "refusing to execute ladder purchase: its tranche is paused", - "refusing to execute: ladder tranche lookup failed: timeout", + "refusing to execute: ladder tranche lookup failed", + "refusing to execute ladder purchase: global config unreadable", "scheduled plan step reached its scheduled date without recommendations; no purchase was submitted", "no current recommendation matched plan 00000000-0000-0000-0000-000000000000 step 2 by 2026-01-02 (3-day grace after the plan date)", "no current recommendation matched plan p step 2 by 2026-01-02 (3-day grace after the plan date); " + skippedNote([]string{"a\x1fb", "c"}), diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index 9eb2fe2db..61ec62971 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -221,8 +221,8 @@ func IsDefiniteAWSRefusal(recs []config.RecommendationRecord, stored string) boo func SanitizeStoredError(s string) string { rest := strings.TrimPrefix(s, partialRunPrefix) prefix := s[:len(s)-len(rest)] - if onlyServerNotes(rest) { - return s + if out, ok := renderServerNotes(rest); ok { + return prefix + out } return prefix + ErrCategoryMessage(classifyStoredText(rest)) } From 569f832927c196fcf237a7e1fb8ac0018a9482d4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 16:58:51 +0100 Subject: [PATCH 09/12] fix(purchase): return only a sentinel's own text ServerNoteMessage and the stale-pricing responses now return the sentinel's fixed text when errors.Is matches, instead of the wrapped chain, which may end in a store or provider error. The full chain is still logged by the caller. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_purchases.go | 4 ++-- internal/api/purchase_failure_notes_test.go | 24 +++++++++++++++++++++ internal/purchase/errnotes.go | 2 +- internal/purchase/errnotes_tail_test.go | 22 +++++++++++++++++++ 4 files changed, 49 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index b2c5db01b..0fbe3328d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -532,7 +532,7 @@ func executeFailureError(executionID, action string, final *config.PurchaseExecu case errors.Is(err, purchase.ErrClaimedRowRefused): return claimedRowRefusalError(final, err) case errors.Is(err, purchase.ErrStaleAzurePricing): - return NewClientError(409, err.Error()) + return NewClientError(409, purchase.ErrStaleAzurePricing.Error()) case purchase.IsCommittedPartialRun(final, err): category, msg := summarizeExecuteFailure(executionID, err) return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s partially completed: some commitments were purchased and can be revoked, others failed: %s (reference: execution %s)", executionID, msg, executionID), @@ -1081,7 +1081,7 @@ func tokenActionError(err error) error { case errors.Is(err, purchase.ErrClaimedRowRefused): return claimedRowRefusalError(nil, err) case errors.Is(err, purchase.ErrStaleAzurePricing): - return NewClientError(409, err.Error()) + return NewClientError(409, purchase.ErrStaleAzurePricing.Error()) case errors.Is(err, purchase.ErrInvalidApprovalToken), errors.Is(err, config.ErrApprovalTokenChanged): return NewClientError(403, "invalid approval token") case errors.Is(err, purchase.ErrApprovalTokenExpired): diff --git a/internal/api/purchase_failure_notes_test.go b/internal/api/purchase_failure_notes_test.go index 5149b3a22..4604b8cc8 100644 --- a/internal/api/purchase_failure_notes_test.go +++ b/internal/api/purchase_failure_notes_test.go @@ -2,6 +2,8 @@ package api import ( "encoding/json" + "errors" + "fmt" "strings" "testing" @@ -55,3 +57,25 @@ func TestTailedNotesWithStoreErrors_NeverReachUserSurfaces(t *testing.T) { }) } } + +func TestExecuteFailure_SentinelWrappingProviderErrorShowsOnlySentinelText(t *testing.T) { + for _, sentinel := range []error{purchase.ErrStaleAzurePricing, purchase.ErrClaimedRowRefused} { + err := fmt.Errorf("%w: %w", sentinel, errors.New(fakeConnectErr)) + _, msg := summarizeExecuteFailure("e-1", err) + assert.Equal(t, sentinel.Error(), msg) + out := executeFailureError("e-1", "executed", &config.PurchaseExecution{Status: "failed"}, err) + require.Error(t, out) + // Non-client errors render as a generic 500 (handler.go), so only + // structured client errors can carry text to the caller. + if ce, ok := IsClientError(out); ok { + assertNoConnectText(t, "live", ce.Error()) + } + } +} + +func TestTokenActionError_StalePricingShowsOnlySentinelText(t *testing.T) { + err := tokenActionError(fmt.Errorf("%w: %w", purchase.ErrStaleAzurePricing, errors.New(fakeConnectErr))) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, purchase.ErrStaleAzurePricing.Error(), ce.Error()) +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 081937809..e7b4d070b 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -100,7 +100,7 @@ func ServerNoteMessage(err error) (string, bool) { for _, sentinel := range []error{ErrPlanStepNoRecommendations, ErrStaleAzurePricing, ErrStepCommitmentCap, ErrDueRowTooOld, ErrClaimedRowRefused, errAmbiguousAccountScope} { if errors.Is(err, sentinel) { - return err.Error(), true + return sentinel.Error(), true } } return renderServerNotes(err.Error()) diff --git a/internal/purchase/errnotes_tail_test.go b/internal/purchase/errnotes_tail_test.go index 1b1ccdc4c..851d9cbd0 100644 --- a/internal/purchase/errnotes_tail_test.go +++ b/internal/purchase/errnotes_tail_test.go @@ -1,6 +1,8 @@ package purchase import ( + "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -49,3 +51,23 @@ func TestSanitizeStoredError_NoPatternMatchesANewline(t *testing.T) { assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), in) } } + +// A sentinel yields only its own text, never the wrapped chain. +func TestServerNoteMessage_SentinelReturnsOnlyItsOwnText(t *testing.T) { + for _, sentinel := range []error{ErrPlanStepNoRecommendations, ErrStaleAzurePricing, ErrStepCommitmentCap, + ErrDueRowTooOld, ErrClaimedRowRefused, errAmbiguousAccountScope} { + for _, err := range []error{ + fmt.Errorf("%w: %w", sentinel, errors.New(fakeConnectError)), + fmt.Errorf("%w: cannot read the latest collection state to confirm Azure prices are current: %w", sentinel, errors.New(fakeConnectError)), + fmt.Errorf("execution e-1: %w: %s", sentinel, fakeConnectError), + } { + msg, ok := ServerNoteMessage(err) + assert.True(t, ok) + assert.Equal(t, sentinel.Error(), msg) + assertNoConnectDetail(t, sentinel.Error(), msg) + } + } + msg, ok := ServerNoteMessage(errors.New("ramp not advanced: " + fakeConnectError)) + assert.True(t, ok) + assert.Equal(t, "ramp not advanced", msg) +} From 5704b52d2d02d0e579cf25f059c9dfd2c0a21a4e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 17:06:52 +0100 Subject: [PATCH 10/12] fix(history): keep Retry hidden for refused AWS purchases A failed row with a definite AWS refusal now keeps a fixed ops_hint instead of an empty one. The History UI hides Retry when ops_hint is set, so clearing it showed a Retry button that always got a 409. The hint says AWS refused the purchase before it was made and carries no duplicate-purchase warning. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_history.go | 7 ++++++- internal/api/purchase_failure_notes_test.go | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index a6a8af665..72c06674a 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -415,8 +415,9 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma row.RedriveUnsafe = true // Only a proven pre-send AWS refusal on every rec means nothing was // bought; for any other outcome the duplicate-purchase warning stays. + // ops_hint stays set either way: the History UI hides Retry on it. if purchase.IsDefiniteAWSRefusal(exec.Recommendations, exec.Error) { - row.OpsHint = "" + row.OpsHint = definiteRefusalOpsHint } else { row.OpsHint = reason } @@ -426,6 +427,10 @@ func executionToHistoryRow(exec config.PurchaseExecution, approver, createdByEma return row } +// definiteRefusalOpsHint replaces the duplicate-purchase warning when AWS +// refused the purchase before making it. +const definiteRefusalOpsHint = "AWS refused this purchase before it was made; start a new purchase after fixing the cause" + // annotateHistoryRowByStatus fills in Approver + StatusDescription on the // row based on exec.Status. Split from executionToHistoryRow to keep the // switch below under the gocyclo threshold. diff --git a/internal/api/purchase_failure_notes_test.go b/internal/api/purchase_failure_notes_test.go index 4604b8cc8..38d0d83e7 100644 --- a/internal/api/purchase_failure_notes_test.go +++ b/internal/api/purchase_failure_notes_test.go @@ -79,3 +79,18 @@ func TestTokenActionError_StalePricingShowsOnlySentinelText(t *testing.T) { require.True(t, ok) assert.Equal(t, purchase.ErrStaleAzurePricing.Error(), ce.Error()) } + +// A refused AWS purchase keeps ops_hint (the History UI hides Retry on it) but +// carries no duplicate-purchase warning. +func TestHistoryRow_DefiniteAWSRefusalKeepsOpsHintWithoutDuplicateWarning(t *testing.T) { + denied := "operation error EC2: X, https response error StatusCode: 403, RequestID: r, api error UnauthorizedOperation: msg" + row := executionToHistoryRow(config.PurchaseExecution{ + ExecutionID: "e-1", Status: "failed", Error: denied, + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}}, + }, "", "") + assert.True(t, row.RedriveUnsafe) + assert.NotEmpty(t, row.OpsHint) + assert.NotContains(t, strings.ToLower(row.OpsHint), "duplicate guard") + assert.NotEqual(t, purchase.RedriveRefusalReason(&config.PurchaseExecution{ + Recommendations: []config.RecommendationRecord{{Provider: "aws", Service: "ec2"}}}), row.OpsHint) +} From 9b2a69dab7c8694315947a0b2d79bfe109911735 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 17:09:51 +0100 Subject: [PATCH 11/12] fix(purchase): read every GCP reason and keep post-insert waits unconfirmed Stored errors are now classified from every "Reason:" line, not only the first, so a recognized reason in second place is found. A failure while waiting on a GCP operation after the Insert was accepted ("commitment creation failed: ...") is never a definite category: it shows that the outcome could not be confirmed, live and stored, and the duplicate-purchase hint stays. Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errsummary.go | 35 ++++++++++++++----- internal/purchase/errsummary_provider_test.go | 28 +++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index 61ec62971..b601abf43 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -28,6 +28,14 @@ var errCategoryMessages = map[string]string{ ErrCategoryUnknown: "the provider returned an error", } +// outcomeUnconfirmedMessage replaces the category message when the provider had +// already accepted the request and only the wait for its result failed. +const outcomeUnconfirmedMessage = "the provider accepted the request but its outcome could not be confirmed; check the account before trying again" + +// postInsertWaitMarker is the cloud-commitments-go GCP wrapper around a failed +// wait on an operation whose Insert was accepted. +const postInsertWaitMarker = "commitment creation failed: " + const partialRunPrefix = "some purchases failed (partial success): " // ErrCategoryMessage returns the fixed user message for a category. @@ -41,6 +49,9 @@ func ErrCategoryMessage(category string) string { // SummarizeProviderError classifies a live error by its type or explicit code // fields and returns its category and fixed message. func SummarizeProviderError(err error) (category, message string) { + if err != nil && strings.Contains(err.Error(), postInsertWaitMarker) { + return ErrCategoryUnknown, outcomeUnconfirmedMessage + } category = classifyTypedError(err) if category == ErrCategoryUnknown && err != nil { category = classifyStoredText(err.Error()) @@ -148,9 +159,14 @@ func statusOf(re *regexp.Regexp, s string) int { // classifyStoredText classifies a stored error string from its explicit code // fields only; no code field means unknown. func classifyStoredText(s string) string { + if strings.Contains(s, postInsertWaitMarker) { + return ErrCategoryUnknown + } for _, re := range []*regexp.Regexp{awsCodeRE, azCodeRE, gcpErrorRE, gcpReasonRE} { - if c := classifyCode(lastGroup(re, s)); c != ErrCategoryUnknown { - return c + for _, code := range lastGroups(re, s) { + if c := classifyCode(code); c != ErrCategoryUnknown { + return c + } } } for _, re := range []*regexp.Regexp{awsStatusRE, azStatusRE, gcpErrorRE} { @@ -161,13 +177,13 @@ func classifyStoredText(s string) string { return ErrCategoryUnknown } -// lastGroup returns the last capture group of re's first match, or "". -func lastGroup(re *regexp.Regexp, s string) string { - m := re.FindStringSubmatch(s) - if len(m) < 2 { - return "" +// lastGroups returns the last capture group of every match of re in s. +func lastGroups(re *regexp.Regexp, s string) []string { + var out []string + for _, m := range re.FindAllStringSubmatch(s, -1) { + out = append(out, m[len(m)-1]) } - return m[len(m)-1] + return out } // definiteAWSCodes are the AWS error codes returned before anything is @@ -224,5 +240,8 @@ func SanitizeStoredError(s string) string { if out, ok := renderServerNotes(rest); ok { return prefix + out } + if strings.Contains(rest, postInsertWaitMarker) { + return prefix + outcomeUnconfirmedMessage + } return prefix + ErrCategoryMessage(classifyStoredText(rest)) } diff --git a/internal/purchase/errsummary_provider_test.go b/internal/purchase/errsummary_provider_test.go index f484146f5..70486e485 100644 --- a/internal/purchase/errsummary_provider_test.go +++ b/internal/purchase/errsummary_provider_test.go @@ -2,6 +2,8 @@ package purchase import ( "context" + "errors" + "fmt" "io" "net/http" "strings" @@ -179,3 +181,29 @@ func TestIsDefiniteAWSRefusal(t *testing.T) { assert.Equal(t, c.want, IsDefiniteAWSRefusal(recs, c.err), c.name) } } + +func TestClassifyStoredText_ReadsEveryGoogleReason(t *testing.T) { + multi := &googleapi.Error{Code: 403, Message: "m", Errors: []googleapi.ErrorItem{{Reason: "x", Message: "a"}, {Reason: "rateLimitExceeded", Message: "b"}}} + assert.Contains(t, multi.Error(), "Reason: rateLimitExceeded") + assert.Equal(t, ErrCategoryLimit, classifyStoredText(multi.Error())) + cat, _ := SummarizeProviderError(multi) + assert.Equal(t, ErrCategoryLimit, cat) +} + +// After an accepted Insert the commitment may exist, so a failed wait is never +// a definite category. +func TestPostInsertWaitFailure_IsUnconfirmed(t *testing.T) { + for _, g := range []*googleapi.Error{ + {Code: 404, Message: "operations/op-1 was not found", Errors: []googleapi.ErrorItem{{Reason: "notFound", Message: "operations/op-1 was not found"}}}, + {Code: 403, Message: "denied", Errors: []googleapi.ErrorItem{{Reason: "forbidden", Message: "denied"}}}, + } { + err := fmt.Errorf("%w: commitment creation failed: %w", errors.New("retry: permanent error, do not retry"), g) + cat, msg := SummarizeProviderError(err) + assert.Equal(t, ErrCategoryUnknown, cat) + assert.Contains(t, msg, "outcome could not be confirmed") + assert.Equal(t, ErrCategoryUnknown, classifyStoredText(err.Error())) + assert.Contains(t, SanitizeStoredError(err.Error()), "outcome could not be confirmed") + assert.Contains(t, SanitizeStoredError(partialRunPrefix+"x: "+err.Error()), "outcome could not be confirmed") + assert.NotContains(t, SanitizeStoredError(err.Error()), "not available") + } +} From 5bc1aad2e08d8d25556ce2dd2613258c39d340cd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 17:54:45 +0100 Subject: [PATCH 12/12] style(purchase): preallocate the matched code groups Co-Authored-By: Claude Opus 5.5 --- internal/purchase/errsummary.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index b601abf43..c2edf57fa 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -179,8 +179,9 @@ func classifyStoredText(s string) string { // lastGroups returns the last capture group of every match of re in s. func lastGroups(re *regexp.Regexp, s string) []string { - var out []string - for _, m := range re.FindAllStringSubmatch(s, -1) { + matches := re.FindAllStringSubmatch(s, -1) + out := make([]string, 0, len(matches)) + for _, m := range matches { out = append(out, m[len(m)-1]) } return out