From 689b98afbbbcb2de19f7435e0ebde3990a866cfe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:36:56 +0100 Subject: [PATCH 01/11] fix(api): summarize provider errors on RI exchange responses and history Client-fault rejections on the AWS and Azure exchange endpoints now return a fixed provider-neutral message, and a failed approval returns a category message instead of the provider's text. The provider error goes to the log. The exchange history response passes stored record errors through the known-note allowlist, so rows written before this change are summarized on read without rewriting them. The allowlist gains the exchange-side notes. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_ri_exchange.go | 17 ++- .../api/handler_ri_exchange_azure_test.go | 2 +- internal/api/handler_ri_exchange_test.go | 5 +- internal/api/provider_error_summaries_test.go | 104 ++++++++++++++++++ internal/api/ri_exchange_detached_test.go | 2 +- internal/purchase/errnotes.go | 25 ++++- internal/purchase/errnotes_test.go | 20 ++++ internal/purchase/errsummary.go | 13 +++ 8 files changed, 180 insertions(+), 8 deletions(-) create mode 100644 internal/api/provider_error_summaries_test.go diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 36eadac33..ca6fcd416 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -24,6 +24,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" "github.com/LeanerCloud/cloud-commitments-platform/internal/ctxutil" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/LeanerCloud/cloud-commitments-platform/internal/ricost" ) @@ -1345,7 +1346,8 @@ func mapAzureExchangeError(opMsg string, err error) error { return refusal } if isAzureClientError(err) { - return NewClientError(400, err.Error()) + logging.Warnf("ri-exchange: %s: provider rejected the request: %v", opMsg, err) + return NewClientError(400, exchangeRejectedMessage) } return NewClientError(500, opMsg) } @@ -2107,11 +2109,16 @@ func mapAWSExchangeError(opMsg string, err error) error { } var apiErr smithy.APIError if errors.As(err, &apiErr) && awsExchangeClientFaultCodes[apiErr.ErrorCode()] { - return NewClientError(400, apiErr.ErrorMessage()) + logging.Warnf("ri-exchange: %s: provider rejected the request: %v", opMsg, err) + return NewClientError(400, exchangeRejectedMessage) } return NewClientError(500, opMsg) } +// exchangeRejectedMessage is the provider-neutral text for a client-fault +// rejection; the provider's own text stays in the log. +const exchangeRejectedMessage = "the exchange request was rejected as invalid: check the offering and count" + // Response types // ConvertibleRIsResponse holds the list of convertible RIs. @@ -2342,8 +2349,10 @@ func (h *Handler) getRIExchangeHistory(ctx context.Context, req *events.LambdaFu // Strip approval tokens — single-use secrets must not be included in // a read-only response that could be cached, logged, or screen-shared. + // Stored errors may hold provider text; only known server notes pass. for i := range records { records[i].ApprovalToken = "" + records[i].Error = purchase.SanitizeStoredError(records[i].Error) } return &RIExchangeHistoryResponse{Records: records}, nil @@ -2984,7 +2993,9 @@ func (h *Handler) executeApprovedExchange(ctx context.Context, id string, record saveCtx, cancelSave := ctxutil.DetachedWithTimeout(ctx, riExchangeTerminalSaveTimeout) defer cancelSave() if execErr != nil { - return h.failExchange(saveCtx, id, execErr.Error()) + logging.Errorf("RI exchange %s execution failed: %v", id, execErr) + _, reason := purchase.SummarizeFailure(execErr) + return h.failExchange(saveCtx, id, reason) } // H3: persist the amount AWS actually accepted, not the reserved ceiling. diff --git a/internal/api/handler_ri_exchange_azure_test.go b/internal/api/handler_ri_exchange_azure_test.go index 097be595b..beca62d32 100644 --- a/internal/api/handler_ri_exchange_azure_test.go +++ b/internal/api/handler_ri_exchange_azure_test.go @@ -1602,7 +1602,7 @@ func TestExecuteAzureExchange_CommitFailureClassification(t *testing.T) { wantCode int wantDetail string }{ - {"azure client fault", &azcore.ResponseError{StatusCode: 409, ErrorCode: "ReservationAlreadyExchanged"}, 400, "ReservationAlreadyExchanged"}, + {"azure client fault", &azcore.ResponseError{StatusCode: 409, ErrorCode: "ReservationAlreadyExchanged"}, 400, "the exchange request was rejected as invalid"}, {"transient failure", fmt.Errorf("azure: ExecuteExchange: transport timeout"), 500, "exchange execution failed"}, } for _, tt := range tests { diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 462cdd73f..e04dd6e64 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1936,8 +1936,9 @@ func TestMapAWSExchangeError_ClientFault4xx(t *testing.T) { ce, ok := IsClientError(mapped) require.True(t, ok, "must be ClientError") assert.Equal(t, 400, ce.code, "client-fault codes must map to 400") - assert.Contains(t, mapped.Error(), "AWS says: bad input", - "AWS error message must be preserved") + assert.Contains(t, mapped.Error(), "the exchange request was rejected as invalid", + "a fixed provider-neutral message replaces the provider text") + assert.NotContains(t, mapped.Error(), "AWS says: bad input") }) } } diff --git a/internal/api/provider_error_summaries_test.go b/internal/api/provider_error_summaries_test.go new file mode 100644 index 000000000..08ad7616b --- /dev/null +++ b/internal/api/provider_error_summaries_test.go @@ -0,0 +1,104 @@ +package api + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/aws/smithy-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const ( + fakeRoleARN = "arn:aws:sts::123456789012:assumed-role/cudly-role/session" + fakeRequestID = "11111111-2222-3333-4444-555555555555" + fakeSubPath = "/subscriptions/" + fakeSubID + "/providers/Microsoft.Capacity/reservationOrders/x" +) + +// awsFault builds a smithy error shaped like the SDK's, with an ARN, account +// and request ID in its text. +func awsFault(code string) error { + return fmt.Errorf("operation error EC2: Op, https response error StatusCode: 400, RequestID: %s, api error %s: %w", + fakeRequestID, code, &smithy.GenericAPIError{ + Code: code, + Message: "User: " + fakeRoleARN + " in account 123456789012 request " + fakeRequestID, + Fault: smithy.FaultClient, + }) +} + +func assertNoProviderDetail(t *testing.T, where, s string) { + t.Helper() + for _, n := range []string{"arn:aws", "123456789012", fakeRequestID, fakeSubID, fakeTenant, "management.azure.com"} { + assert.NotContains(t, s, n, where) + } +} + +func TestExchangeErrors_HideProviderDetail(t *testing.T) { + for _, code := range []string{"InvalidOfferingId", "InvalidParameter", "ValidationError"} { + ce, ok := IsClientError(mapAWSExchangeError("exchange quote failed", awsFault(code))) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assertNoProviderDetail(t, code, ce.message) + assert.Contains(t, ce.message, "the exchange request was rejected as invalid") + } + ce, ok := IsClientError(mapAzureExchangeError("exchange failed", azureHTTPError(t, 400, fakeSubPath))) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assertNoProviderDetail(t, "azure", ce.message) + assert.Contains(t, ce.message, "the exchange request was rejected as invalid") +} + +func TestExchangeApprovalFailure_HidesProviderDetail(t *testing.T) { + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-000000000001" + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + store.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{RIExchangeMaxDailyUSD: 1000, RIExchangeMaxPerExchangeUSD: 500}, nil) + store.On("ReserveApprovedRIExchange", ctx, mock.Anything, mock.Anything, mock.Anything).Return("50.000000", nil) + var stored string + store.On("FailRIExchange", mock.Anything, id, mock.Anything). + Run(func(args mock.Arguments) { stored = args.String(2) }).Return(nil) + h := &Handler{config: store, executeExchangeFn: func(context.Context, exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { + return "", nil, errors.New("operation error EC2: Accept, https response error StatusCode: 403, RequestID: " + fakeRequestID + + ", api error UnauthorizedOperation: User: " + fakeRoleARN + " is not authorized") + }} + + resp, err := h.executeApprovedExchange(ctx, id, &config.RIExchangeRecord{ID: id, Region: "us-east-1", SourceRIIDs: []string{"ri-1"}, TargetOfferingID: "o", TargetCount: 1}) + require.NoError(t, err) + reason := resp.(map[string]any)["reason"].(string) + assertNoProviderDetail(t, "response", reason) + assertNoProviderDetail(t, "stored", stored) + assert.Contains(t, reason, "not allowed") +} + +func TestExchangeHistory_SanitizesStoredErrors(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + auth := new(MockAuthService) + t.Cleanup(func() { auth.AssertExpectations(t) }) + auth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "uid"}, nil) + auth.grantAdmin() + raw := "api error UnauthorizedOperation: User: " + fakeRoleARN + " RequestID: " + fakeRequestID + const note = "exchange record has no region; cannot execute safely" + store.On("GetRIExchangeHistory", ctx, mock.Anything, 500).Return([]config.RIExchangeRecord{ + {ID: "a", Status: "failed", Error: raw}, + {ID: "b", Status: "failed", Error: note}, + {ID: "c", Status: "completed"}, + }, nil) + + h := &Handler{auth: auth, config: store} + result, err := h.getRIExchangeHistory(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}}) + require.NoError(t, err) + recs := result.(*RIExchangeHistoryResponse).Records + assertNoProviderDetail(t, "history", recs[0].Error) + assert.Contains(t, recs[0].Error, "not allowed") + assert.Equal(t, note, recs[1].Error) + assert.Empty(t, recs[2].Error) +} diff --git a/internal/api/ri_exchange_detached_test.go b/internal/api/ri_exchange_detached_test.go index b76c89c43..2899e2870 100644 --- a/internal/api/ri_exchange_detached_test.go +++ b/internal/api/ri_exchange_detached_test.go @@ -114,7 +114,7 @@ func TestExecuteApprovedExchange_RequestCancelStillRecordsFailure(t *testing.T) defer cancelRequest() store := approvedExchangeStore(t, id) - store.On("FailRIExchange", liveCtx(), id, "quote no longer valid").Return(nil).Once() + store.On("FailRIExchange", liveCtx(), id, "the provider returned an error").Return(nil).Once() h := &Handler{config: store, executeExchangeFn: func(_ context.Context, _ exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { cancelRequest() diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index e0e407a42..049a5f1cf 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -3,6 +3,7 @@ package purchase import ( "errors" "regexp" + "slices" "strings" ) @@ -36,6 +37,28 @@ var serverNotePatterns = []string{ `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`, + // RI exchange record errors (api/handler_ri_exchange.go failExchange callers and + // the exchange package's auto-exchange reservation notes). + `config load failed`, + `exchange record has no region; cannot execute safely`, + `(?:daily|per-exchange) spending cap is not configured \(RIExchange(?:MaxDaily|MaxPerExchange)USD is 0\)`, + `RI exchange daily cap exceeded: (?:spend [^;\s]+ exceeds cap [^;\s]+|initial [^;\s]+, remaining [^;\s]+)`, + `daily spending cap check failed`, + `reserved ceiling "[^"]*" is not a decimal`, + `exchange reservation returned no record ID`, + `exchange reservation returned invalid ceiling "[^"]*"`, + // Scheduler note for a provider name it does not know. + `unknown provider "[^"]*" \(valid: aws, azure, gcp\)`, +} + +// categoryNotePatterns matches a collection failure stored as ": +// " per failed provider. +func categoryNotePatterns() []string { + var out []string + for _, c := range []string{ErrCategoryAccessDenied, ErrCategoryUnavailable, ErrCategoryLimit, ErrCategoryUnknown} { + out = append(out, regexp.QuoteMeta(ErrCategoryMessage(c))) + } + return out } const ( @@ -60,7 +83,7 @@ var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + reg `|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, "|") + `)(?:; |$)`) +var serverNoteRE = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(append(slices.Clone(serverNotePatterns), categoryNotePatterns()...), "|") + `)(?:; |$)`) var serverNoteHeadRE = regexp.MustCompile(`^((?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNoteHeads, "|") + `))(?:: [^;]*)?(?:; |$)`) diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index e192b7239..b734de704 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -55,6 +55,18 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { "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", + "config load failed", + "exchange record has no region; cannot execute safely", + "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)", + "per-exchange spending cap is not configured (RIExchangeMaxPerExchangeUSD is 0)", + "RI exchange daily cap exceeded: spend 900.000000 exceeds cap 800.000000", + "RI exchange daily cap exceeded: initial 10.000000, remaining 5.000000", + "daily spending cap check failed", + `reserved ceiling "x" is not a decimal`, + "exchange reservation returned no record ID", + `exchange reservation returned invalid ceiling "x"`, + `unknown provider "AWS" (valid: aws, azure, gcp)`, + ErrCategoryMessage(ErrCategoryAccessDenied), } for _, n := range notes { assert.NotEmpty(t, n) @@ -87,3 +99,11 @@ const legacyPlanStepNoRecommendationsNote = "plan step has no recommendations at func TestErrPlanStepNoRecommendations_NamesNoInternalIssue(t *testing.T) { assert.NotContains(t, ErrPlanStepNoRecommendations.Error(), "#") } + +// A recommendation collection failure is stored as one ": " item per failed provider. +func TestSanitizeStoredError_CollectionCategoryForm(t *testing.T) { + stored := "aws: " + ErrCategoryMessage(ErrCategoryAccessDenied) + "; azure: " + ErrCategoryMessage(ErrCategoryUnknown) + assert.Equal(t, stored, SanitizeStoredError(stored)) + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(stored+"; gcp: raw text for project my-project")) +} diff --git a/internal/purchase/errsummary.go b/internal/purchase/errsummary.go index c2edf57fa..f218996ba 100644 --- a/internal/purchase/errsummary.go +++ b/internal/purchase/errsummary.go @@ -59,6 +59,19 @@ func SummarizeProviderError(err error) (category, message string) { return category, ErrCategoryMessage(category) } +// SummarizeFailure is SummarizeProviderError for errors that may also be a +// known server-authored refusal: those keep their own readable text, since +// the category message would hide what to fix. +func SummarizeFailure(err error) (category, message string) { + category, message = SummarizeProviderError(err) + if category == ErrCategoryUnknown { + if text, ok := ServerNoteMessage(err); ok { + message = text + } + } + return category, message +} + func classifyTypedError(err error) string { var apiErr interface{ ErrorCode() string } var azErr *azcore.ResponseError From 7924f597a88f0f6e013068cb54a1e45d650c885b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:41:13 +0100 Subject: [PATCH 02/11] fix(api): summarize provider errors on marketplace and refund paths Marketplace listing and cancel errors, Azure refund and revoke rejections, and the session-approve scheduling failure now return a category message plus a purchase or execution reference. The provider or store error is logged once and no longer echoed in the response. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_marketplace.go | 24 +++- internal/api/handler_marketplace_attempt.go | 2 +- internal/api/handler_marketplace_cancel.go | 2 +- .../handler_marketplace_credentials_test.go | 14 ++- internal/api/handler_marketplace_test.go | 5 +- internal/api/handler_purchases.go | 13 +- internal/api/handler_purchases_revoke.go | 20 +++- .../provider_error_summaries_refund_test.go | 111 ++++++++++++++++++ 8 files changed, 162 insertions(+), 29 deletions(-) create mode 100644 internal/api/provider_error_summaries_refund_test.go diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index be0f7b76b..1d3ec1540 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -26,6 +26,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" @@ -309,11 +310,11 @@ func (h *Handler) keepUncanceledListing(ctx context.Context, purchaseID string, if state == "" { state = config.ListingStatePending } - cancelDetail := cancelErr.Error() + _, cancelDetail := purchase.SummarizeProviderError(cancelErr) if isRoleAssumptionFailure(cancelErr) { cancelDetail = marketplaceRoleAssumptionMessage } - msg := fmt.Sprintf("marketplace listing %s was created but could not be saved, and canceling it failed (%s); the listing may still be active on AWS", listing.ListingID, cancelDetail) + msg := fmt.Sprintf("marketplace listing %s was created but could not be saved, and canceling it failed (%s); the listing may still be active on AWS (reference: purchase %s)", listing.ListingID, cancelDetail, purchaseID) if err := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, listing.ListingID, state); err != nil { if isListingEndedConflict(err) { return listingCanceledConcurrently(listing.ListingID) @@ -472,8 +473,8 @@ func roleAssumptionOperationError(err error) *smithy.OperationError { const marketplaceSellerNotRegisteredReason = "seller_not_registered" // mapAWSMarketplaceError maps an AWS SDK error to an appropriate ClientError. -// AWS client-fault errors (4xx-category codes) produce a 4xx response with the -// original AWS message so the caller gets actionable feedback. SellerNotRegistered +// AWS client-fault errors (4xx-category codes) produce a 400 with a category +// message, never the AWS text, which carries ARNs and request IDs. SellerNotRegistered // is the exception: its 400 carries marketplaceSellerNotRegisteredMessage and // reason seller_not_registered instead. All other errors produce a 502 // (AWS-side failure). STS AssumeRole failures are classified by @@ -489,12 +490,23 @@ func mapAWSMarketplaceError(opMsg string, err error) error { map[string]any{"reason": marketplaceSellerNotRegisteredReason}) } if awsMarketplaceClientFaultCodes[apiErr.ErrorCode()] || apiErr.ErrorFault() == smithy.FaultClient { - return NewClientError(400, apiErr.ErrorMessage()) + logging.Warnf("marketplace: %s: provider rejected the request: %v", opMsg, err) + category, msg := purchase.SummarizeProviderError(err) + if category == purchase.ErrCategoryUnknown { + msg = marketplaceRejectedMessage + } + return NewClientError(400, msg) } } - return NewClientError(502, opMsg+": "+err.Error()) + logging.Warnf("marketplace: %s: %v", opMsg, err) + _, msg := purchase.SummarizeProviderError(err) + return NewClientError(502, opMsg+": "+msg) } +// marketplaceRejectedMessage is the fixed text for a client-fault rejection +// the error classifier has no category for; the provider's text stays in the log. +const marketplaceRejectedMessage = "AWS rejected the listing request as invalid: check the listing parameters" + // isKnownNonStandardOfferingClass reports true when offering_class is // explicitly set to a non-standard value (e.g. "convertible"). An empty string // returns false because the class is not yet known and will be resolved lazily. diff --git a/internal/api/handler_marketplace_attempt.go b/internal/api/handler_marketplace_attempt.go index eaeba0de4..f98cfaab6 100644 --- a/internal/api/handler_marketplace_attempt.go +++ b/internal/api/handler_marketplace_attempt.go @@ -208,7 +208,7 @@ func (h *Handler) createAndPersistListing(ctx context.Context, purchaseID string // the listing unless it rejected the request as a client fault, so only // then is the token dropped; otherwise the retry resends it. h.releaseMarketplaceClaim(ctx, purchaseID, claim, !isClientFaultError(mapped)) - logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) + logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed", purchaseID) return ec2MarketplaceListingResult{}, mapped } diff --git a/internal/api/handler_marketplace_cancel.go b/internal/api/handler_marketplace_cancel.go index 9b24fda3e..76287e127 100644 --- a/internal/api/handler_marketplace_cancel.go +++ b/internal/api/handler_marketplace_cancel.go @@ -148,7 +148,7 @@ func (h *Handler) cancelListing(ctx context.Context, purchaseID string, seen *co if err != nil { ended, ok := endedMarketplaceListing(ctx, ec2Client, listingID, err) if !ok { - logging.Warnf("marketplace: CancelReservedInstancesListing for listing %s failed: %v", listingID, err) + logging.Warnf("marketplace: CancelReservedInstancesListing for listing %s failed", listingID) return nil, mapAWSMarketplaceError("AWS cancel listing failed", err) } result = ended diff --git a/internal/api/handler_marketplace_credentials_test.go b/internal/api/handler_marketplace_credentials_test.go index e99d2cd15..8a30b128e 100644 --- a/internal/api/handler_marketplace_credentials_test.go +++ b/internal/api/handler_marketplace_credentials_test.go @@ -13,6 +13,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-sdk-go-v2/aws" awscreds "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/smithy-go" @@ -248,7 +249,8 @@ func TestMarketplaceList_CompensationAssumeRoleDeniedReturnsFixedMessage(t *test require.True(t, ok, "expected a client error, got %v", err) assert.Equal(t, 502, ce.code) assert.Contains(t, ce.message, marketplaceRoleAssumptionMessage) - assert.NotContains(t, ce.message, marketplaceHostAccount) + // The purchase ID in the reference is a test UUID whose last segment equals the fake host account. + assert.NotContains(t, strings.ReplaceAll(ce.message, validMarketplacePurchaseID, ""), marketplaceHostAccount) assert.NotContains(t, ce.message, "arn:aws:") } @@ -468,7 +470,11 @@ func TestMarketplaceHandlers_NoCloudAccountRow(t *testing.T) { } } -func TestMapAWSMarketplaceError_GenuineEC2ErrorsKeepTheirOwnMessage(t *testing.T) { +func TestMapAWSMarketplaceError_GenuineEC2ErrorsKeepTheirOwnCategory(t *testing.T) { + want := map[string]string{ + "InvalidReservedInstancesId.NotFound": purchase.ErrCategoryMessage(purchase.ErrCategoryUnavailable), + "UnauthorizedOperation": purchase.ErrCategoryMessage(purchase.ErrCategoryAccessDenied), + } for _, code := range []string{"InvalidReservedInstancesId.NotFound", "UnauthorizedOperation"} { t.Run(code, func(t *testing.T) { const msg = "the ec2 service said no" @@ -482,7 +488,7 @@ func TestMapAWSMarketplaceError_GenuineEC2ErrorsKeepTheirOwnMessage(t *testing.T require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Equal(t, msg, ce.message) + assert.Equal(t, want[code], ce.message, "a category message, not the provider text") assert.NotEqual(t, marketplaceRoleAssumptionMessage, ce.message) }) } @@ -565,6 +571,6 @@ func TestMapAWSMarketplaceError_OtherClientFaultsHaveNoReason(t *testing.T) { require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Equal(t, "bad price", ce.message) + assert.Equal(t, marketplaceRejectedMessage, ce.message) assert.Nil(t, ce.Details()) } diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index 8814d7f8f..997a767f8 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -9,6 +9,7 @@ import ( ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" smithy "github.com/aws/smithy-go" @@ -274,7 +275,7 @@ func TestMarketplaceList_AWSClientFaultMapsTo400(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 400, ce.code) - assert.Contains(t, err.Error(), "bad RI") + assert.Contains(t, err.Error(), marketplaceRejectedMessage) } func TestMarketplaceList_AWSServerFaultMapsTo502(t *testing.T) { @@ -399,7 +400,7 @@ func TestMarketplaceList_DBFailureAndCancelFailureKeepsListing(t *testing.T) { assert.Equal(t, 502, ce.code) assert.NotContains(t, ce.message, "rolled back") assert.Contains(t, ce.message, "ril-default") - assert.Contains(t, ce.message, "throttled") + assert.Contains(t, ce.message, purchase.ErrCategoryMessage(purchase.ErrCategoryUnknown)) assert.Contains(t, ce.message, tc.wantMsg) assert.Equal(t, 1, ec2.cancelCallCount) cfgStore.AssertNotCalled(t, "ReleaseMarketplaceListingClaim", mock.Anything, validMarketplacePurchaseID, mock.Anything, mock.Anything, mock.Anything) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 7cc4348c1..104196a7f 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -552,15 +552,7 @@ func logRawExecuteFailure(executionID string, err error) { func summarizeExecuteFailure(executionID string, err error) (category, message string) { logRawExecuteFailure(executionID, err) - category, message = purchase.SummarizeProviderError(err) - if category == purchase.ErrCategoryUnknown { - // 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 + return purchase.SummarizeFailure(err) } // unclaimedExecuteError answers a failure where the funnel returned no row: @@ -1500,7 +1492,8 @@ func (h *Handler) approveWithDelay(ctx context.Context, execution *config.Purcha logging.Errorf("purchase[%s]: token approve failed to schedule: %v", execution.ExecutionID, err) return nil, NewClientError(500, "failed to schedule execution") } - return nil, NewClientError(500, fmt.Sprintf("failed to schedule execution %s: %v", execution.ExecutionID, err)) + logging.Errorf("purchase[%s]: failed to schedule execution: %v", execution.ExecutionID, err) + return nil, NewClientError(500, fmt.Sprintf("failed to schedule execution (reference: execution %s)", execution.ExecutionID)) } // Best-effort scheduled notification email. h.sendPurchaseScheduledEmail(ctx, updated, actor) diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index db258827e..7240e1d1e 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -35,6 +35,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" "github.com/LeanerCloud/cloud-commitments-platform/internal/ctxutil" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/jackc/pgx/v5" ) @@ -597,7 +598,7 @@ func (h *Handler) calculateAzureRevoke(ctx context.Context, req *events.LambdaFu }, nil) if err != nil { if isAzureClientError(err) { - return nil, NewClientError(400, fmt.Sprintf("Azure refund calculation rejected: %v", err)) + return nil, azureRefundRejected("Azure refund calculation rejected", purchaseID, err) } return nil, fmt.Errorf("revoke/calculate: CalculateRefund failed: %w", err) } @@ -824,7 +825,7 @@ func (h *Handler) callAzureReturn( // Step 1: CalculateRefund -> sessionID + quoted amount (TOCTOU check). quantity := int32(record.Count) // #nosec G115 -- Azure reservation count validated at purchase; bounded by API limits (< Date: Wed, 4 Nov 2026 15:44:02 +0100 Subject: [PATCH 03/11] fix(scheduler): store summarized collection errors A failed provider is now stored in last_collection_error and the collect result as ": "; the provider's own error is logged only. The freshness and refresh responses pass the stored value through the known-note allowlist, so rows written before this change are summarized on read without rewriting them. Closes #1702 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/handler_recommendations.go | 16 +++++++ .../api/handler_recommendations_refresh.go | 4 +- ...rovider_error_summaries_collection_test.go | 44 +++++++++++++++++++ .../collection_error_summary_test.go | 39 ++++++++++++++++ internal/scheduler/scheduler.go | 2 +- 5 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 internal/api/provider_error_summaries_collection_test.go create mode 100644 internal/scheduler/collection_error_summary_test.go diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 8f8b5060d..af1bf2924 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -11,6 +11,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" ) @@ -354,6 +355,7 @@ func (h *Handler) getRecommendationsFreshness(ctx context.Context, req *events.L if freshness == nil { return nil, nil } + freshness = withSanitizedCollectionError(freshness) scope, err := h.getAccountScope(ctx, session) if err != nil { return nil, fmt.Errorf("failed to get allowed accounts: %w", err) @@ -367,6 +369,20 @@ func (h *Handler) getRecommendationsFreshness(ctx context.Context, req *events.L // accounts the scope allows. The state row is global, so naming an account the // caller cannot see would leak that it exists. An unrestricted scope sees all // of it; an account-less (ambient) hold has no IDs to show a restricted caller. +// withSanitizedCollectionError returns f with last_collection_error reduced to +// known server notes or a category message: rows stored before the collector +// summarized provider errors hold the provider's raw text. The stored value +// is not rewritten. +func withSanitizedCollectionError(f *config.RecommendationsFreshness) *config.RecommendationsFreshness { + if f == nil || f.LastCollectionError == nil { + return f + } + out := *f + msg := purchase.SanitizeStoredError(*f.LastCollectionError) + out.LastCollectionError = &msg + return &out +} + func scopeHeldAccounts(f *config.RecommendationsFreshness, scope auth.AccountScope) *config.RecommendationsFreshness { out := *f if scope.AllowsAll() || f.LastCollectionPartial == nil { diff --git a/internal/api/handler_recommendations_refresh.go b/internal/api/handler_recommendations_refresh.go index eabedadc8..902c673c9 100644 --- a/internal/api/handler_recommendations_refresh.go +++ b/internal/api/handler_recommendations_refresh.go @@ -157,7 +157,7 @@ func (h *Handler) runMarkedCollection(ctx context.Context, token string) (*confi if err != nil { return nil, fmt.Errorf("failed to read freshness after async invoke: %w", err) } - return freshness, nil + return withSanitizedCollectionError(freshness), nil } // Non-Lambda (HTTP) mode: collect synchronously. ClearCollectionStarted // is called by the scheduler's deferred clearCollectionStartedBestEffort, @@ -169,7 +169,7 @@ func (h *Handler) runMarkedCollection(ctx context.Context, token string) (*confi if err != nil { return nil, fmt.Errorf("failed to read freshness after collect: %w", err) } - return freshness, nil + return withSanitizedCollectionError(freshness), nil } // rollbackMarkedCollection releases this caller's own in-flight marker after diff --git a/internal/api/provider_error_summaries_collection_test.go b/internal/api/provider_error_summaries_collection_test.go new file mode 100644 index 000000000..ed96219c7 --- /dev/null +++ b/internal/api/provider_error_summaries_collection_test.go @@ -0,0 +1,44 @@ +package api + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Rows stored before the write side summarized provider errors hold raw text; +// the freshness response must show only a category message for them. +func TestFreshness_SanitizesStoredCollectionError(t *testing.T) { + ctx := context.Background() + rawStored := "azure: PUT https://management.azure.com/subscriptions/" + fakeSubID + "/providers/Microsoft.Capacity/x\n" + + "RESPONSE 403: Forbidden\nERROR CODE: AuthorizationFailed\n; aws: operation error EC2: Op, https response error StatusCode: 403, " + + "RequestID: " + fakeRequestID + ", api error UnauthorizedOperation: User: " + fakeRoleARN + " in account 123456789012" + newCategory := "gcp: the provider returned an error" + + for name, tc := range map[string]struct{ stored, want string }{ + "raw text": {rawStored, "the cloud provider refused the purchase: CUDly's role is not allowed to make it in this account"}, + "category form": {newCategory, newCategory}, + } { + t.Run(name, func(t *testing.T) { + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + now := time.Now().UTC() + stored := tc.stored + store.On("GetRecommendationsFreshness", ctx).Return(&config.RecommendationsFreshness{LastCollectedAt: &now, LastCollectionError: &stored}, nil) + h := &Handler{config: store, apiKey: "test-key"} + + got, err := h.getRecommendationsFreshness(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"x-api-key": "test-key"}}) + + require.NoError(t, err) + require.NotNil(t, got.LastCollectionError) + assertNoProviderDetail(t, "freshness", *got.LastCollectionError) + assert.Equal(t, tc.want, *got.LastCollectionError) + assert.Equal(t, tc.stored, stored, "the stored value is never rewritten") + }) + } +} diff --git a/internal/scheduler/collection_error_summary_test.go b/internal/scheduler/collection_error_summary_test.go new file mode 100644 index 000000000..19837d9f8 --- /dev/null +++ b/internal/scheduler/collection_error_summary_test.go @@ -0,0 +1,39 @@ +package scheduler + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" +) + +// A failed provider's own error text carries project and account identifiers +// and request IDs; the stored last_collection_error and the collect result +// hold only a category message. +func TestCollectRecommendations_StoresSummarizedProviderError(t *testing.T) { + t.Setenv("GCP_PROJECT_ID", "proj-a") + raw := fmt.Errorf("get GCP recommendations: %w", &googleapi.Error{ + Code: 403, + Message: "Permission denied on resource project my-project-123456789012; request 11111111-2222-3333-4444-555555555555", + Errors: []googleapi.ErrorItem{{Reason: "forbidden", Message: "caller arn:aws:sts::123456789012:assumed-role/x/y"}}, + }) + s, store := newPartialSweepScheduler(t, "gcp", raw) + expectScheduledCollection(store, "gcp") + var stored string + store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { stored = args.String(1) }).Return(nil).Once() + + result, err := s.CollectRecommendations(context.Background(), "") + + require.NoError(t, err) + for _, got := range []string{stored, result.FailedProviders["gcp"]} { + for _, needle := range []string{"my-project-123456789012", "11111111-2222", "arn:aws", "123456789012", "googleapi"} { + assert.NotContains(t, got, needle) + } + } + assert.Equal(t, "gcp: the cloud provider refused the purchase: CUDly's role is not allowed to make it in this account", stored) +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 1f1b52352..db7946baa 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -372,7 +372,7 @@ func (s *Scheduler) collectAllProviders(ctx context.Context, globalCfg *config.G } if out.err != nil { logging.Errorf("Failed to collect %s recommendations: %v", providerName, out.err) - failedProviders[providerName] = out.err.Error() + _, failedProviders[providerName] = purchase.SummarizeFailure(out.err) continue } successfulProviders = append(successfulProviders, providerName) From ed46803e4801fa250b1dea2e26de4bb08c2db9ee Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 16:06:40 +0100 Subject: [PATCH 04/11] style(purchase): preallocate the category note patterns Satisfies the prealloc linter on the helper added with the exchange notes. Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errnotes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 049a5f1cf..b81e0ca75 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -54,7 +54,7 @@ var serverNotePatterns = []string{ // categoryNotePatterns matches a collection failure stored as ": // " per failed provider. func categoryNotePatterns() []string { - var out []string + out := make([]string, 0, 4) for _, c := range []string{ErrCategoryAccessDenied, ErrCategoryUnavailable, ErrCategoryLimit, ErrCategoryUnknown} { out = append(out, regexp.QuoteMeta(ErrCategoryMessage(c))) } From 05db5272f1262ca90df58de0e5560616635faece Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 20:19:20 +0100 Subject: [PATCH 05/11] fix(purchase): limit the exchange and collection notes to fixed text Pin the variable parts of the RI exchange cap, reserved ceiling and unknown provider notes to a decimal amount or a short token with no whitespace or quotes. A value outside that alphabet now shows only the fixed head of the note instead of the whole note, and newlines never pass through. Add tests for each tailed note. Refs #1721 Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errnotes.go | 18 ++++++++++++----- internal/purchase/errnotes_test.go | 29 ++++++++++++++++++++++++++++ internal/scheduler/scheduler_test.go | 3 ++- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index b81e0ca75..44d0adfd8 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -42,13 +42,13 @@ var serverNotePatterns = []string{ `config load failed`, `exchange record has no region; cannot execute safely`, `(?:daily|per-exchange) spending cap is not configured \(RIExchange(?:MaxDaily|MaxPerExchange)USD is 0\)`, - `RI exchange daily cap exceeded: (?:spend [^;\s]+ exceeds cap [^;\s]+|initial [^;\s]+, remaining [^;\s]+)`, + `RI exchange daily cap exceeded: (?:spend ` + amountPattern + ` exceeds cap ` + amountPattern + `|initial ` + amountPattern + `, remaining ` + amountPattern + `)`, `daily spending cap check failed`, - `reserved ceiling "[^"]*" is not a decimal`, + `reserved ceiling "` + shortValuePattern + `" is not a decimal`, `exchange reservation returned no record ID`, - `exchange reservation returned invalid ceiling "[^"]*"`, + `exchange reservation returned invalid ceiling "` + shortValuePattern + `"`, // Scheduler note for a provider name it does not know. - `unknown provider "[^"]*" \(valid: aws, azure, gcp\)`, + `unknown provider "` + shortValuePattern + `" \(valid: aws, azure, gcp\)`, } // categoryNotePatterns matches a collection failure stored as ": @@ -62,6 +62,10 @@ func categoryNotePatterns() []string { } const ( + // amountPattern is a decimal amount; shortValuePattern is a short value with no + // whitespace, quotes or newline. Anything wider falls back to the fixed head. + amountPattern = `\d+(?:\.\d+)?` + shortValuePattern = `[A-Za-z0-9._+-]{1,32}` idPattern = `[^;\s,()]+` idListPattern = idPattern + `(?:, ` + idPattern + `)*` skippedNotePattern = `\d+ recommendation\(s\) without a frozen baseline skipped \(under-buy\): ` + idListPattern @@ -77,6 +81,10 @@ var serverNoteHeads = []string{ `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`, + `RI exchange daily cap exceeded`, + `reserved ceiling`, + `exchange reservation returned invalid ceiling`, + `unknown provider`, } var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + regexp.QuoteMeta(redriveReasonAzureSavingsPlan) + @@ -85,7 +93,7 @@ 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(append(slices.Clone(serverNotePatterns), categoryNotePatterns()...), "|") + `)(?:; |$)`) -var serverNoteHeadRE = regexp.MustCompile(`^((?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNoteHeads, "|") + `))(?:: [^;]*)?(?:; |$)`) +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 diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index b734de704..055dc89e9 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -107,3 +107,32 @@ func TestSanitizeStoredError_CollectionCategoryForm(t *testing.T) { assert.Equal(t, stored, SanitizeStoredError(stored)) assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(stored+"; gcp: raw text for project my-project")) } + +// A variable part outside its closed alphabet (a newline, a provider or DB +// error text) never reaches the output: only the fixed head or nothing shows. +func TestSanitizeStoredError_ExchangeNoteTailsAreClosed(t *testing.T) { + cases := []struct{ in, want string }{ + {"RI exchange daily cap exceeded: spend 1\nDETAIL pq: boom exceeds cap 2", "RI exchange daily cap exceeded"}, + {"RI exchange daily cap exceeded: initial 1.0, remaining pq: connection refused", "RI exchange daily cap exceeded"}, + {"RI exchange daily cap exceeded: spend x y exceeds cap 2.0", "RI exchange daily cap exceeded"}, + {"reserved ceiling \"a\nb\" is not a decimal", "reserved ceiling"}, + {"reserved ceiling \"pq: boom\" is not a decimal", "reserved ceiling"}, + {"exchange reservation returned invalid ceiling \"x\ny\"", "exchange reservation returned invalid ceiling"}, + {"unknown provider \"evil\nprovider\" (valid: aws, azure, gcp)", "unknown provider"}, + {"unknown provider \"db error: password for x\" (valid: aws, azure, gcp)", "unknown provider"}, + } + for _, c := range cases { + assert.Equal(t, c.want, SanitizeStoredError(c.in), c.in) + } + for _, in := range []string{ + "config load failed\nsecret", + "config load failed: pq: boom", + "exchange record has no region; cannot execute safely\nx", + "daily spending cap check failed: pq: boom", + "exchange reservation returned no record ID: pq: boom", + "daily spending cap is not configured (RIExchangeMaxDailyUSD is 0)\nx", + "aws: " + ErrCategoryMessage(ErrCategoryUnknown) + "\nraw", + } { + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), in) + } +} diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index cbb886828..827c99f36 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -515,7 +515,8 @@ func TestScheduler_CollectRecommendations_UnknownProvider(t *testing.T) { assert.Contains(t, result.FailedProviders, "azure ") mockStore.AssertCalled(t, "SetRecommendationsCollectionError", mock.Anything, mock.AnythingOfType("string")) assert.Contains(t, recordedErr, `unknown provider "AWS"`) - assert.Contains(t, recordedErr, `unknown provider "azure "`) + assert.NotContains(t, recordedErr, `"azure "`, "a name outside the token alphabet shows only the fixed head") + assert.Contains(t, recordedErr, "azure : unknown provider") } func TestScheduler_CollectAWSRecommendations(t *testing.T) { From 4918faeeeda950492681fcd055003d2b8c7d57b9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 21:13:06 +0100 Subject: [PATCH 06/11] test(api): cover the refresh path's collection error sanitizing The marked-collection re-read returns the stored freshness to the refresh handler. Pin that a stored raw provider error comes back as a category message on both the async-invoke and the synchronous branch. Co-Authored-By: Claude Sonnet 5.5 --- ...rovider_error_summaries_collection_test.go | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/internal/api/provider_error_summaries_collection_test.go b/internal/api/provider_error_summaries_collection_test.go index ed96219c7..271766717 100644 --- a/internal/api/provider_error_summaries_collection_test.go +++ b/internal/api/provider_error_summaries_collection_test.go @@ -6,8 +6,11 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/scheduler" "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/service/lambda" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -42,3 +45,51 @@ func TestFreshness_SanitizesStoredCollectionError(t *testing.T) { }) } } + +// The refresh path re-reads freshness after the marked collection and hands it +// back to its caller; a stored raw provider error must come back as a category +// message on both the async-invoke and the synchronous branch. +func TestRunMarkedCollection_SanitizesStoredCollectionError(t *testing.T) { + ctx := context.Background() + raw := "aws: operation error EC2: Op, https response error StatusCode: 403, RequestID: " + fakeRequestID + + ", api error UnauthorizedOperation: User: " + fakeRoleARN + " in account 123456789012" + + "; azure: GET https://management.azure.com/subscriptions/" + fakeSubID + "/providers/Microsoft.Capacity/x" + want := "the cloud provider refused the purchase: CUDly's role is not allowed to make it in this account" + + for name, tc := range map[string]struct { + lambdaARN string + invoker LambdaInvokerInterface + sched bool + }{ + "async invoke": {lambdaARN: "arn:aws:lambda:us-east-1:123456789012:function:cudly", invoker: &stubLambdaInvoker{ + invokeFn: func(context.Context, *lambda.InvokeInput) (*lambda.InvokeOutput, error) { + return &lambda.InvokeOutput{}, nil + }}}, + "synchronous": {sched: true}, + } { + t.Run(name, func(t *testing.T) { + t.Setenv("SCHEDULER_LAMBDA_ARN", tc.lambdaARN) + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + stored := raw + store.On("GetRecommendationsFreshness", mock.Anything).Return(&config.RecommendationsFreshness{LastCollectionError: &stored}, nil) + h := &Handler{config: store} + if tc.invoker != nil { + h.lambdaInvoker = tc.invoker + } + if tc.sched { + sched := new(MockScheduler) + t.Cleanup(func() { sched.AssertExpectations(t) }) + sched.On("CollectRecommendations", mock.Anything, "tok").Return(&scheduler.CollectResult{}, nil) + h.scheduler = sched + } + + got, err := h.runMarkedCollection(ctx, "tok") + + require.NoError(t, err) + require.NotNil(t, got.LastCollectionError) + assertNoProviderDetail(t, "refresh", *got.LastCollectionError) + assert.Equal(t, want, *got.LastCollectionError) + }) + } +} From accf2f4cc7ad6954b40593ce6ef286089ec31333 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 21:29:11 +0100 Subject: [PATCH 07/11] fix(purchase): accept only real resource and provider heads in server notes The optional ": " head ahead of a stored note accepted any token. Per-rec notes now take only an instance, node or plan type shape (AWS, Azure, GCP), collection category notes also aws|azure|gcp, and the RI exchange notes take no head. Account numbers, addresses, hosts and user names in that slot fall back to the category message. Refs #1721 Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errnotes.go | 44 ++++++++++++++++++++--- internal/purchase/errnotes_tail_test.go | 2 +- internal/purchase/errnotes_test.go | 46 ++++++++++++++++++++++++- 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 44d0adfd8..6e13df57a 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -3,7 +3,6 @@ package purchase import ( "errors" "regexp" - "slices" "strings" ) @@ -37,6 +36,11 @@ var serverNotePatterns = []string{ `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`, +} + +// exchangeNotePatterns are server-authored RI exchange record notes. They are +// written bare, never behind a resource or provider head, so none is allowed. +var exchangeNotePatterns = []string{ // RI exchange record errors (api/handler_ri_exchange.go failExchange callers and // the exchange package's auto-exchange reservation notes). `config load failed`, @@ -81,6 +85,10 @@ var serverNoteHeads = []string{ `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`, +} + +// exchangeNoteHeads are the fixed heads of the exchange notes whose tail is not closed. +var exchangeNoteHeads = []string{ `RI exchange daily cap exceeded`, `reserved ceiling`, `exchange reservation returned invalid ceiling`, @@ -90,10 +98,38 @@ var serverNoteHeads = []string{ 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(append(slices.Clone(serverNotePatterns), categoryNotePatterns()...), "|") + `)(?:; |$)`) +// resourceTypePattern is the ": " head the executor's per-rec join +// adds (execution.go: fmt.Sprintf("%s: %v", rec.ResourceType, err)). Only these +// instance, node and plan type shapes are accepted, each with a digit in its +// family part so an account number, IPv4 address, hostname or DB user never fits: +// - AWS: [db.|cache.].[.search], family [a-z]{1,4}..., size +// nano|micro|small|medium|large|[N]xlarge|xlplus|metal (m5.large, db.t4g.micro, +// cache.t3.micro, dc2.large, ra3.xlplus, t3.small.search) +// - Azure: Standard_|Basic_ + letters, a digit, then [A-Za-z0-9_-] (Standard_D2_v3) +// - GCP: [letter]-[-] (n2-standard-2, e2-micro) +const resourceTypePattern = `(?:` + + `(?:db\.|cache\.)?[a-z]{1,4}[0-9][a-z0-9]{0,5}\.(?:nano|micro|small|medium|large|[0-9]{0,2}xlarge|xlplus|metal)(?:\.search)?` + + `|(?:Standard|Basic)_[A-Za-z]{1,4}[0-9][A-Za-z0-9_-]{0,24}` + + `|[a-z][0-9][a-z]?-[a-z]{2,12}(?:-[0-9]{1,4})?` + + `)` + +// providerHeadPattern is the collection writer's ": " head. +const providerHeadPattern = `(?:aws|azure|gcp)` + +const ( + resourceHead = `(?:` + resourceTypePattern + `: )?` + categoryHead = `(?:(?:` + providerHeadPattern + `|` + resourceTypePattern + `): )?` +) + +func joinAlt(parts []string) string { return strings.Join(parts, "|") } + +var serverNoteRE = regexp.MustCompile(`^(?:` + + resourceHead + `(?:` + joinAlt(serverNotePatterns) + `)` + + `|` + `(?:` + joinAlt(exchangeNotePatterns) + `)` + + `|` + categoryHead + `(?:` + joinAlt(categoryNotePatterns()) + `)` + + `)(?:; |$)`) -var serverNoteHeadRE = regexp.MustCompile(`^((?:[A-Za-z0-9][A-Za-z0-9._-]*: )?(?:` + strings.Join(serverNoteHeads, "|") + `))(?:(?:: | ")[^;]*)?(?:; |$)`) +var serverNoteHeadRE = regexp.MustCompile(`^(` + resourceHead + `(?:` + joinAlt(serverNoteHeads) + `)|(?:` + joinAlt(exchangeNoteHeads) + `))(?:(?:: | ")[^;]*)?(?:; |$)`) // renderServerNotes returns s with every "; "-separated item either kept // verbatim (a closed allowlisted note) or cut to its fixed head (a known note diff --git a/internal/purchase/errnotes_tail_test.go b/internal/purchase/errnotes_tail_test.go index 851d9cbd0..ac0271efb 100644 --- a/internal/purchase/errnotes_tail_test.go +++ b/internal/purchase/errnotes_tail_test.go @@ -31,7 +31,7 @@ func TestSanitizeStoredError_TailedNotesShowOnlyTheirHead(t *testing.T) { {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"}, + {"m5.large: ramp not advanced: " + fakeConnectError + "; recovered after reap; provider purchase succeeded", "m5.large: ramp not advanced; recovered after reap; provider purchase succeeded"}, {partialRunPrefix + "ramp not advanced: " + fakeConnectError, partialRunPrefix + "ramp not advanced"}, } for _, c := range cases { diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 055dc89e9..58f8bc969 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -2,6 +2,7 @@ package purchase import ( "fmt" + "slices" "testing" "time" @@ -68,10 +69,15 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { `unknown provider "AWS" (valid: aws, azure, gcp)`, ErrCategoryMessage(ErrCategoryAccessDenied), } + exchangeNotes := notes[slices.Index(notes, "config load failed"):slices.Index(notes, ErrCategoryMessage(ErrCategoryAccessDenied))] 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) + if slices.Contains(exchangeNotes, n) { + continue + } + head := "m5.large: " + assert.Equal(t, n, SanitizeStoredError(partialRunPrefix + head + n)[len(partialRunPrefix)+len(head):], "behind a resource head: "+n) } joined := notes[0] + "; " + notes[1] + "; " + notes[6] assert.Equal(t, joined, SanitizeStoredError(joined)) @@ -136,3 +142,41 @@ func TestSanitizeStoredError_ExchangeNoteTailsAreClosed(t *testing.T) { assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), in) } } + +// The head ahead of a note must be a real resource type (per-rec notes) or +// aws|azure|gcp (collection notes); an account number, address, host or DB user +// in that slot is never shown. +func TestSanitizeStoredError_HeadsAreClosed(t *testing.T) { + note := "plan step refused by commitment cap" + category := ErrCategoryMessage(ErrCategoryUnavailable) + for _, h := range []string{ + "m5.large", "db.t4g.micro", "cache.t3.micro", "dc2.large", "ra3.xlplus", "ra3.16xlarge", + "t3.small.search", "Standard_D2_v3", "Basic_A1", "n2-standard-2", "e2-micro", "c3-highcpu-8", + } { + assert.Equal(t, h+": "+note, SanitizeStoredError(h+": "+note), h) + assert.Equal(t, h+": "+category, SanitizeStoredError(h+": "+category), h) + } + for _, h := range []string{"aws", "azure", "gcp"} { + assert.Equal(t, h+": "+category, SanitizeStoredError(h+": "+category), h) + } + for _, h := range []string{ + "123456789012", "10.20.30.40", "db-prod.internal", "db.internal.example", "cudly_app", "web1.prod.example", + "m5.large.example.com", "ec2.large.example", "arn:aws:iam::123456789012:role/x", "evil", "AWS", + } { + unknown := ErrCategoryMessage(ErrCategoryUnknown) + assert.Equal(t, unknown, SanitizeStoredError(h+": "+category), h) + assert.Equal(t, unknown, SanitizeStoredError(h+": "+note), h) + } + for _, in := range []string{ + "123456789012: config load failed", + "10.20.30.40: config load failed", + "db-prod.internal: daily spending cap check failed", + "cudly_app: the requested commitment is not available from the provider", + "m5.large: config load failed", + "aws: " + note, + } { + out := SanitizeStoredError(in) + assert.NotContains(t, out, ": ", in) + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), out, in) + } +} From 659ef44fa06ada922cdd571ac24b3f3e284ab128 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 21:31:18 +0100 Subject: [PATCH 08/11] fix(purchase): show the operator resolve note in exchange history The note an operator's resolve call stores is server-formatted fields plus the admin's own quoted reason. The stored-error filter replaced it with the provider category message. Allow that fixed shape, and nothing after it. Refs #1721 Co-Authored-By: Claude Sonnet 5.5 --- internal/api/ri_exchange_resolve_note_test.go | 33 +++++++++++++++++++ internal/purchase/errnotes.go | 12 ++++--- 2 files changed, 41 insertions(+), 4 deletions(-) create mode 100644 internal/api/ri_exchange_resolve_note_test.go diff --git a/internal/api/ri_exchange_resolve_note_test.go b/internal/api/ri_exchange_resolve_note_test.go new file mode 100644 index 000000000..efeeb00f8 --- /dev/null +++ b/internal/api/ri_exchange_resolve_note_test.go @@ -0,0 +1,33 @@ +package api + +import ( + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/stretchr/testify/assert" +) + +// The note an operator's resolve call stores is the admin's own text: exchange +// history shows it as written, while provider text in the same column does not. +func TestResolveNote_ShowsAsWrittenInHistory(t *testing.T) { + check := config.RIExchangeCloudCheck{AccountID: strings.Repeat("7", 12), Region: "us-east-1", CheckedAt: time.Date(2026, 5, 1, 10, 0, 0, 0, time.UTC)} + for name, body := range map[string]riExchangeResolveRequest{ + "completed": {Outcome: config.StatusCompleted, ExchangeID: "ri-exch-0123", Reason: `checked in the console; it says "done"; ticket 42 é`}, + "failed": {Outcome: config.StatusFailed, Reason: "the exchange never reached AWS, confirmed with support"}, + "semicolon text": {Outcome: config.StatusFailed, Reason: "first; second; third"}, + } { + t.Run(name, func(t *testing.T) { + note := body.note(check) + assert.Equal(t, note, purchase.SanitizeStoredError(note)) + assert.Equal(t, note+"; "+note, purchase.SanitizeStoredError(note+"; "+note)) + }) + } + provider := "operation error EC2: AcceptReservedInstancesExchangeQuote, RequestID: 11111111, api error" + assert.Equal(t, purchase.ErrCategoryMessage(purchase.ErrCategoryUnknown), purchase.SanitizeStoredError(provider)) + note := riExchangeResolveRequest{Outcome: config.StatusFailed, Reason: "ten chars ok"}.note(check) + assert.Equal(t, purchase.ErrCategoryMessage(purchase.ErrCategoryUnknown), purchase.SanitizeStoredError(note+"; "+provider)) + assert.Equal(t, purchase.ErrCategoryMessage(purchase.ErrCategoryUnknown), purchase.SanitizeStoredError(note+" trailing provider text")) +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 6e13df57a..f5ebc2ede 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -38,11 +38,15 @@ var serverNotePatterns = []string{ `FROM_EMAIL not configured for this deployment`, } -// exchangeNotePatterns are server-authored RI exchange record notes. They are -// written bare, never behind a resource or provider head, so none is allowed. -var exchangeNotePatterns = []string{ +// bareNotePatterns are server-authored RI exchange record and collection notes. +// They are written bare, never behind a resource or provider head, so none is allowed. +var bareNotePatterns = []string{ // RI exchange record errors (api/handler_ri_exchange.go failExchange callers and // the exchange package's auto-exchange reservation notes). + // The operator's stale-exchange resolve note (api/handler_ri_exchange_resolve.go + // note()): fixed fields in a fixed order, then the admin's own reason as a Go + // quoted string, which is text typed on purpose and closed by its quotes. + `operator:(?:completed|failed): account="\d{12}" region="[a-z0-9-]{1,32}" checked_at=\d{4}-\d{2}-\d{2}T[0-9:.]+(?:Z|[+-]\d{2}:\d{2})(?: ack=true)?(?: exchange_id="(?:[^"\\\n]|\\.)*")? reason="(?:[^"\\\n]|\\.)*"`, `config load failed`, `exchange record has no region; cannot execute safely`, `(?:daily|per-exchange) spending cap is not configured \(RIExchange(?:MaxDaily|MaxPerExchange)USD is 0\)`, @@ -125,7 +129,7 @@ func joinAlt(parts []string) string { return strings.Join(parts, "|") } var serverNoteRE = regexp.MustCompile(`^(?:` + resourceHead + `(?:` + joinAlt(serverNotePatterns) + `)` + - `|` + `(?:` + joinAlt(exchangeNotePatterns) + `)` + + `|` + `(?:` + joinAlt(bareNotePatterns) + `)` + `|` + categoryHead + `(?:` + joinAlt(categoryNotePatterns()) + `)` + `)(?:; |$)`) From 3749bd6204276bd0b8afb5be7d4318a6a8bed75d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 21:32:07 +0100 Subject: [PATCH 09/11] fix(scheduler): store a fixed note for a failed recommendations write A failed rows write stored the store error text as the collection error. Store a fixed note instead and log the cause, and allow the three collection-stopped notes on the read path so they show as written. Refs #1721 Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errnotes.go | 3 ++ .../collection_stopped_notes_test.go | 41 +++++++++++++++++++ internal/scheduler/scheduler.go | 3 +- 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 internal/scheduler/collection_stopped_notes_test.go diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index f5ebc2ede..d9ef2a85e 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -41,6 +41,9 @@ var serverNotePatterns = []string{ // bareNotePatterns are server-authored RI exchange record and collection notes. // They are written bare, never behind a resource or provider head, so none is allowed. var bareNotePatterns = []string{ + // Scheduler notes recorded when a collection ends before saving + // (scheduler.go collectionStopped*); the cause is logged, never stored. + `recommendations collection stopped before saving: (?:could not load settings|it was canceled or ran out of time|could not save the collected recommendations)`, // RI exchange record errors (api/handler_ri_exchange.go failExchange callers and // the exchange package's auto-exchange reservation notes). // The operator's stale-exchange resolve note (api/handler_ri_exchange_resolve.go diff --git a/internal/scheduler/collection_stopped_notes_test.go b/internal/scheduler/collection_stopped_notes_test.go new file mode 100644 index 000000000..74d9814ff --- /dev/null +++ b/internal/scheduler/collection_stopped_notes_test.go @@ -0,0 +1,41 @@ +package scheduler + +import ( + "context" + "fmt" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +// The fixed collection-stopped notes are shown as written on the read path. +func TestCollectionStoppedNotes_ShowAsWritten(t *testing.T) { + for _, n := range []string{collectionStoppedLoadingSettings, collectionStoppedIncomplete, collectionStoppedSaving} { + assert.Equal(t, n, purchase.SanitizeStoredError(n)) + assert.Equal(t, n+"; "+n, purchase.SanitizeStoredError(n+"; "+n)) + } +} + +// A failed rows write stores a fixed note; the database error text stays in +// the log. +func TestPersistCollection_StoresNoRawWriteError(t *testing.T) { + raw := fmt.Errorf("write recommendations: failed to connect to `host=db-prod.internal user=cudly_app database=cudly`: " + + "dial error (dial tcp 10.20.30.40:5432: connect: connection refused)") + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + store.On("UpsertRecommendations", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(raw).Once() + var stored string + store.On("SetRecommendationsCollectionError", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { stored = args.String(1) }).Return(nil).Once() + s := &Scheduler{config: store} + + s.persistCollection(context.Background(), nil, nil, nil, nil) + + assert.Equal(t, collectionStoppedSaving, stored) + assert.Equal(t, stored, purchase.SanitizeStoredError(stored)) + for _, needle := range []string{"db-prod.internal", "cudly_app", "10.20.30.40", "dial"} { + assert.NotContains(t, stored, needle) + } +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index db7946baa..4fdba7fb2 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -448,7 +448,7 @@ func (s *Scheduler) clearCollectionStartedBestEffort(ctx context.Context, ownerT func (s *Scheduler) persistCollection(ctx context.Context, recs []config.RecommendationRecord, successfulCollects []config.SuccessfulCollect, failedProviders map[string]string, partial *config.RecommendationsCollectionPartial) { if err := s.upsertWithMarker(ctx, recs, successfulCollects, partial); err != nil { logging.Errorf("Failed to persist recommendations: %v", err) - if setErr := s.config.SetRecommendationsCollectionError(ctx, err.Error()); setErr != nil { + if setErr := s.config.SetRecommendationsCollectionError(ctx, collectionStoppedSaving); setErr != nil { logging.Errorf("Failed to record write error: %v", setErr) } return @@ -469,6 +469,7 @@ func (s *Scheduler) persistCollection(ctx context.Context, recs []config.Recomme const ( collectionStoppedLoadingSettings = "recommendations collection stopped before saving: could not load settings" collectionStoppedIncomplete = "recommendations collection stopped before saving: it was canceled or ran out of time" + collectionStoppedSaving = "recommendations collection stopped before saving: could not save the collected recommendations" ) // recordCollectionStoppedBestEffort records why a collection ended without From 45438f238046c930d7909daccf566815ad691916 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 22:07:23 +0100 Subject: [PATCH 10/11] fix(purchase): accept every real resource type as a server-note head The narrowed per-rec head rejected real resource types the executor writes, so allowlisted notes behind them rendered as the generic category message. Accept the Savings Plan types (Compute, EC2Instance, SageMaker, Database), db.serverless, u-/mac2-/ultrawarm families, metal-Nxl sizes, Azure SQL SKUs and unprefixed VM sizes, and GCP custom and suffixed machine types. Heads stay closed: account numbers, IPs, hosts, DB users and ARNs still fail. Also close the exchange_id slot of the operator resolve note to the writer's [A-Za-z0-9-]{1,100} shape. Refs #1721 The cfn-lint pre-commit hook was skipped (SKIP=cfn-lint): its pinned cfn-lint cannot install under the local Python, and this commit touches no CloudFormation files. Every other hook ran and passed. Co-Authored-By: Claude Sonnet 5.5 --- internal/purchase/errnotes.go | 29 ++++++++++++++++++----------- internal/purchase/errnotes_test.go | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index d9ef2a85e..263d35fa4 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -49,7 +49,7 @@ var bareNotePatterns = []string{ // The operator's stale-exchange resolve note (api/handler_ri_exchange_resolve.go // note()): fixed fields in a fixed order, then the admin's own reason as a Go // quoted string, which is text typed on purpose and closed by its quotes. - `operator:(?:completed|failed): account="\d{12}" region="[a-z0-9-]{1,32}" checked_at=\d{4}-\d{2}-\d{2}T[0-9:.]+(?:Z|[+-]\d{2}:\d{2})(?: ack=true)?(?: exchange_id="(?:[^"\\\n]|\\.)*")? reason="(?:[^"\\\n]|\\.)*"`, + `operator:(?:completed|failed): account="\d{12}" region="[a-z0-9-]{1,32}" checked_at=\d{4}-\d{2}-\d{2}T[0-9:.]+(?:Z|[+-]\d{2}:\d{2})(?: ack=true)?(?: exchange_id="[A-Za-z0-9-]{1,100}")? reason="(?:[^"\\\n]|\\.)*"`, `config load failed`, `exchange record has no region; cannot execute safely`, `(?:daily|per-exchange) spending cap is not configured \(RIExchange(?:MaxDaily|MaxPerExchange)USD is 0\)`, @@ -106,18 +106,25 @@ var redriveReasonPattern = regexp.QuoteMeta(redriveReasonAWSNoGuard) + `|` + reg `|provider "[^"]*" is not known to reject a duplicate purchase, so re-driving this could buy a second commitment` // resourceTypePattern is the ": " head the executor's per-rec join -// adds (execution.go: fmt.Sprintf("%s: %v", rec.ResourceType, err)). Only these -// instance, node and plan type shapes are accepted, each with a digit in its -// family part so an account number, IPv4 address, hostname or DB user never fits: -// - AWS: [db.|cache.].[.search], family [a-z]{1,4}..., size -// nano|micro|small|medium|large|[N]xlarge|xlplus|metal (m5.large, db.t4g.micro, -// cache.t3.micro, dc2.large, ra3.xlplus, t3.small.search) -// - Azure: Standard_|Basic_ + letters, a digit, then [A-Za-z0-9_-] (Standard_D2_v3) -// - GCP: [letter]-[-] (n2-standard-2, e2-micro) +// adds (execution.go: fmt.Sprintf("%s: %v", rec.ResourceType, err)). The head is +// closed: a Savings Plan type from a fixed list, or an instance, node or machine +// type shape whose family carries a digit, so an account number, IPv4 address, +// hostname, DB user or ARN never fits: +// - Savings Plan types: Compute, EC2Instance, SageMaker (aws-sdk SavingsPlanType) +// and Database, plus Azure's "Compute" +// - AWS: [db.|cache.].[.search] (m5.large, u-6tb1.metal, +// mac2-m2pro.metal, ultrawarm1.medium.search, m7i.metal-24xl) and db.serverless +// - Azure SQL: GP_Gen5_8 vCore SKUs and the tier names GENERAL_PURPOSE, BUSINESS_CRITICAL, HYPERSCALE +// - Azure: Standard_|Basic_ sizes (Standard_D2, Standard_NV24ads_A10_v5) and the +// unprefixed forms the provider sends (D2, D2s_v3, NC6s_v3) +// - GCP: -[-]... with a closed series word +// (n2-standard-2, n2d-custom-2-8192-ext, a2-ultragpu-8g, c3d-highmem-360-lssd, e2-micro) const resourceTypePattern = `(?:` + - `(?:db\.|cache\.)?[a-z]{1,4}[0-9][a-z0-9]{0,5}\.(?:nano|micro|small|medium|large|[0-9]{0,2}xlarge|xlplus|metal)(?:\.search)?` + + `Compute|EC2Instance|SageMaker|Database|db\.serverless` + + `|(?:db\.|cache\.)?(?:[a-z]{1,4}[0-9][a-z0-9]{0,5}(?:-[a-z][a-z0-9]{1,7})?|u-[0-9]{1,2}tb[0-9]|ultrawarm1)\.(?:nano|micro|small|medium|large|[0-9]{0,3}xlarge|xlplus|metal(?:-[0-9]{1,2}xl)?)(?:\.search)?` + `|(?:Standard|Basic)_[A-Za-z]{1,4}[0-9][A-Za-z0-9_-]{0,24}` + - `|[a-z][0-9][a-z]?-[a-z]{2,12}(?:-[0-9]{1,4})?` + + `|[A-Z]{1,3}[0-9]{1,3}[a-z]{0,4}(?:_[A-Za-z0-9]{1,4}){0,2}|[A-Z]{1,3}_Gen[0-9]_[0-9]{1,3}|GENERAL_PURPOSE|BUSINESS_CRITICAL|HYPERSCALE` + + `|[a-z][0-9][a-z]{0,2}-(?:standard|highmem|highcpu|highgpu|ultragpu|megagpu|megamem|ultramem|hypermem|custom|micro|small|medium)(?:-[a-z0-9]{1,8}){0,3}` + `)` // providerHeadPattern is the collection writer's ": " head. diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 58f8bc969..cccc1e40e 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -152,6 +152,14 @@ func TestSanitizeStoredError_HeadsAreClosed(t *testing.T) { for _, h := range []string{ "m5.large", "db.t4g.micro", "cache.t3.micro", "dc2.large", "ra3.xlplus", "ra3.16xlarge", "t3.small.search", "Standard_D2_v3", "Basic_A1", "n2-standard-2", "e2-micro", "c3-highcpu-8", + // Savings Plan types (aws-sdk SavingsPlanType, azure computeResourceType). + "Compute", "EC2Instance", "SageMaker", "Database", + "db.serverless", "u-6tb1.metal", "u-6tb1.112xlarge", "mac2-m2pro.metal", "ultrawarm1.medium.search", + "m7i.metal-24xl", "n2-custom-4-8192", "n2d-custom-2-8192-ext", "e2-custom-4-8192", "e2-custom-medium-2048", + "a2-ultragpu-8g", "a2-highgpu-1g", "a3-megagpu-8g", "c3d-highmem-360-lssd", "m1-ultramem-40", "h4d-highmem-192", + "Standard_D2", "Standard_B2ats_v2", "Standard_NV24ads_A10_v5", "Standard_NC6s_v3", "Basic_C0", + "D2", "D2s_v3", "NC6s_v3", "A1", + "GP_Gen5_8", "BC_Gen5_16", "GENERAL_PURPOSE", "c7i-flex.large", } { assert.Equal(t, h+": "+note, SanitizeStoredError(h+": "+note), h) assert.Equal(t, h+": "+category, SanitizeStoredError(h+": "+category), h) @@ -180,3 +188,13 @@ func TestSanitizeStoredError_HeadsAreClosed(t *testing.T) { assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), out, in) } } + +func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { + base := `operator:completed: account="` + "1234" + "56789012" + `" region="us-east-1" checked_at=2026-01-02T03:04:05Z` + ok := base + ` exchange_id="abc-123" reason="done"` + assert.Equal(t, ok, SanitizeStoredError(ok)) + for _, id := range []string{`a b`, `db-prod.internal`, `x\"y`, `a_b`, ``} { + in := base + ` exchange_id="` + id + `" reason="done"` + assert.Equal(t, ErrCategoryMessage(ErrCategoryUnknown), SanitizeStoredError(in), id) + } +} From 80f2959f1fa351d1c70589eb516f5679f3174353 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 22:38:47 +0100 Subject: [PATCH 11/11] test(api): expect the summarized marketplace cancel failure The cancel-failure integration test asserted the raw provider text in the response. The response now carries the summary and a purchase reference, and the provider text stays in the server log. Refs #1702 Co-Authored-By: Claude Opus 5.5 --- .../api/handler_marketplace_compensation_integration_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_marketplace_compensation_integration_test.go b/internal/api/handler_marketplace_compensation_integration_test.go index fb361a55d..7c381e23e 100644 --- a/internal/api/handler_marketplace_compensation_integration_test.go +++ b/internal/api/handler_marketplace_compensation_integration_test.go @@ -69,7 +69,9 @@ func TestMarketplaceListCancelFailureKeepsListingRecorded(t *testing.T) { require.True(t, ok, "%v", err) assert.Equal(t, 502, ce.code) assert.NotContains(t, ce.message, "rolled back") - assert.Contains(t, ce.message, "RequestLimitExceeded") + // The provider's text stays in the log; the user gets the summary and a reference (#1702). + assert.NotContains(t, ce.message, "RequestLimitExceeded") + assert.Contains(t, ce.message, "reference: purchase "+row.PurchaseID) stored, err := store.GetPurchaseHistoryByPurchaseID(ctx, row.PurchaseID) require.NoError(t, err)