From fc459ae167c0b1266c8e15b939cb98450230addf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 19:59:55 +0200 Subject: [PATCH 01/21] feat(marketplace): sell/cancel Standard RIs on AWS Marketplace - backend (closes #292) Add sell-on-Marketplace support for Standard Reserved Instances: - migration 000060: add offering_class, listing_id, listing_state to purchase_history - auth: sell-any and sell-own actions; sell-own added to DefaultUserPermissions - config: extend PurchaseHistoryRecord, StoreInterface with GetPurchaseHistoryByPurchaseID + UpdatePurchaseHistoryListing - ec2 client: CreateReservedInstancesListing, DescribeReservedInstancesListings, CancelReservedInstancesListing - api: handler_marketplace.go with marketplaceList/Cancel, authorizeSessionSell (sell-any/sell-own RBAC), default price schedule (95% of residual value) - router: POST /api/purchases/{id}/marketplace-list + /marketplace-cancel routes - all tests updated for the new interface methods and permission count --- internal/analytics/collector_test.go | 7 + internal/api/handler.go | 5 + internal/api/handler_marketplace.go | 332 ++++++++++++++++++ internal/api/router.go | 14 + internal/auth/service_group_test.go | 22 +- internal/auth/types.go | 20 ++ internal/auth/types_test.go | 7 +- internal/config/interfaces.go | 14 +- internal/config/store_postgres.go | 77 +++- .../config/store_postgres_pgxmock_test.go | 22 +- internal/config/types.go | 14 + ...chase_history_marketplace_listing.down.sql | 5 + ...urchase_history_marketplace_listing.up.sql | 18 + internal/mocks/stores.go | 8 +- internal/server/test_helpers_test.go | 3 + providers/aws/services/ec2/client.go | 131 +++++++ 16 files changed, 667 insertions(+), 32 deletions(-) create mode 100644 internal/api/handler_marketplace.go create mode 100644 internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql create mode 100644 internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index eeba52712..a6ed3ddcd 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -430,6 +430,13 @@ func (m *mockConfigStore) UpsertRIUtilizationCache(_ context.Context, _ string, return nil } +func (m *mockConfigStore) GetPurchaseHistoryByPurchaseID(_ context.Context, _ string) (*config.PurchaseHistoryRecord, error) { + return nil, nil +} +func (m *mockConfigStore) UpdatePurchaseHistoryListing(_ context.Context, _, _, _ string) error { + return nil +} + // strPtr is a test helper for *string fields. func strPtr(s string) *string { return &s } diff --git a/internal/api/handler.go b/internal/api/handler.go index 41e673366..e415214e3 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -83,6 +83,11 @@ type Handler struct { // armreservations-backed client. azureExchangeFactory func(subscriptionID string) azureExchangeClient + // Optional marketplace EC2 client factory injected by tests. When nil + // (the production default), buildMarketplaceEC2Client uses + // awsprovider.NewEC2ClientDirect. + marketplaceEC2Factory func(aws.Config) marketplaceEC2Client + // Optional account-resolver injection point used by the reshape // handler integration test. When nil (the production default), the // handler calls h.resolveAWSCloudAccountID which in turn invokes diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go new file mode 100644 index 000000000..c3b51dca8 --- /dev/null +++ b/internal/api/handler_marketplace.go @@ -0,0 +1,332 @@ +package api + +// handler_marketplace.go implements the Sell-on-Marketplace flow for Standard +// Reserved Instances (issue #292). +// +// Endpoints: +// POST /api/purchases/{id}/marketplace-list create a listing +// POST /api/purchases/{id}/marketplace-cancel cancel an active listing +// +// Both endpoints require the caller to hold sell-any:purchases (admin or a +// custom operator group) or sell-own:purchases for rows they purchased +// themselves. The purchase_id in the URL path is the AWS ReservedInstancesId +// stamped into purchase_history.purchase_id at completion. + +import ( + "context" + "encoding/json" + "fmt" + "strings" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/LeanerCloud/CUDly/pkg/logging" + awsprovider "github.com/LeanerCloud/CUDly/providers/aws" + ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" + "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/google/uuid" +) + +// marketplaceEC2Client is the narrow EC2 interface the marketplace handlers +// need. Using a minimal interface keeps test stubs small. +type marketplaceEC2Client interface { + CreateMarketplaceListing(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) + DescribeMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) + CancelMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) +} + +// buildMarketplaceEC2Client honours the injected factory for tests, falling +// back to the direct AWS SDK constructor in production. +func (h *Handler) buildMarketplaceEC2Client(cfg aws.Config) marketplaceEC2Client { + if h.marketplaceEC2Factory != nil { + return h.marketplaceEC2Factory(cfg) + } + return awsprovider.NewEC2ClientDirect(cfg) +} + +// MarketplacePriceTier is the JSON-decodable shape accepted in the request body. +type MarketplacePriceTier struct { + // TermMonths is the remaining-months count this tier covers. + TermMonths int64 `json:"term_months"` + // Price is the USD list price per unit for this tier. + Price float64 `json:"price"` +} + +// MarketplaceListRequest is the request body for POST .../marketplace-list. +// PriceSchedule is optional: when absent the handler computes a default from +// the row's upfront_cost, monthly_cost, term, and a 5% discount to attract +// buyers (documented in the response body so the caller can see what was used). +type MarketplaceListRequest struct { + PriceSchedule []MarketplacePriceTier `json:"price_schedule,omitempty"` +} + +// MarketplaceListResponse is the JSON response body for a successful listing. +type MarketplaceListResponse struct { + ListingID string `json:"listing_id"` + ListingState string `json:"listing_state"` + PriceSchedule []MarketplacePriceTier `json:"price_schedule"` + AWSFeePercent float64 `json:"aws_fee_percent"` + Note string `json:"note,omitempty"` +} + +// marketplaceList handles POST /api/purchases/{id}/marketplace-list. +// The {id} must be the purchase_history.purchase_id (AWS ReservedInstancesId). +func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctionURLRequest, purchaseID string) (any, error) { + session, err := h.requireSession(ctx, req) + if err != nil { + return nil, err + } + + if err := validateUUID(purchaseID); err != nil { + return nil, err + } + + // Look up the purchase_history row to validate offering_class + get metadata. + row, err := h.config.GetPurchaseHistoryByPurchaseID(ctx, purchaseID) + if err != nil { + return nil, fmt.Errorf("failed to look up purchase: %w", err) + } + if row == nil { + return nil, NewClientError(404, "purchase not found") + } + + // Only Standard RIs can be listed on the Marketplace. + if !strings.EqualFold(row.OfferingClass, "standard") { + return nil, NewClientError(400, "only Standard Reserved Instances can be listed on the AWS Marketplace; this purchase has offering_class="+row.OfferingClass) + } + + // Enforce sell-any / sell-own RBAC. + if err := h.authorizeSessionSell(ctx, session, row.CloudAccountID); err != nil { + return nil, err + } + + // Reject if a listing is already active to avoid duplicate listings. + if strings.EqualFold(row.ListingState, "active") { + return nil, NewClientError(409, fmt.Sprintf("an active marketplace listing %s already exists for this RI; cancel it first", row.ListingID)) + } + + // Decode optional body. + var body MarketplaceListRequest + if len(req.Body) > 0 { + if err := json.Unmarshal([]byte(req.Body), &body); err != nil { + return nil, NewClientError(400, "invalid request body: "+err.Error()) + } + } + + // Validate and normalise the price schedule. MonthlyCost is nullable + // (nil means the provider returned no recurring breakdown); treat absent + // as $0 recurring for the default price-schedule computation. + var monthlyCost float64 + if row.MonthlyCost != nil { + monthlyCost = *row.MonthlyCost + } + schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, row.Term, row.UpfrontCost, monthlyCost) + if err != nil { + return nil, NewClientError(400, err.Error()) + } + + // Build per-provider AWS config for the region the RI lives in. + cfg, err := h.loadAWSConfigWithRegion(ctx, row.Region) + if err != nil { + return nil, fmt.Errorf("failed to load AWS config: %w", err) + } + + ec2Client := h.buildMarketplaceEC2Client(cfg) + + awsSchedule := make([]ec2svc.MarketplacePriceTier, 0, len(schedule)) + for _, t := range schedule { + awsSchedule = append(awsSchedule, ec2svc.MarketplacePriceTier{ + Term: t.TermMonths, + Price: t.Price, + }) + } + + result, err := ec2Client.CreateMarketplaceListing(ctx, ec2svc.MarketplaceListingRequest{ + ReservedInstancesID: purchaseID, + ClientToken: uuid.New().String(), + PriceSchedule: awsSchedule, + }) + if err != nil { + // Preserve the AWS error message verbatim for 4xx errors (e.g. missing + // seller account, invalid listing) so the frontend can surface it. + logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) + return nil, NewClientError(502, "AWS marketplace listing failed: "+err.Error()) + } + + // Persist the listing ID and state. + if dbErr := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, result.ListingID, result.State); dbErr != nil { + // Log the error but return success to the caller — the listing was + // created in AWS; a DB write failure here must not be surfaced as a + // 5xx that could cause the user to create a duplicate listing. + logging.Errorf("marketplace: listing created (%s / %s) but DB update failed: %v", result.ListingID, result.State, dbErr) + } + + return &MarketplaceListResponse{ + ListingID: result.ListingID, + ListingState: result.State, + PriceSchedule: schedule, + AWSFeePercent: 12, + Note: "AWS charges a 12% transaction fee on the listing proceeds. Net proceeds = ListingPrice * 0.88.", + }, nil +} + +// marketplaceCancel handles POST /api/purchases/{id}/marketplace-cancel. +func (h *Handler) marketplaceCancel(ctx context.Context, req *events.LambdaFunctionURLRequest, purchaseID string) (any, error) { + session, err := h.requireSession(ctx, req) + if err != nil { + return nil, err + } + + if err := validateUUID(purchaseID); err != nil { + return nil, err + } + + row, err := h.config.GetPurchaseHistoryByPurchaseID(ctx, purchaseID) + if err != nil { + return nil, fmt.Errorf("failed to look up purchase: %w", err) + } + if row == nil { + return nil, NewClientError(404, "purchase not found") + } + + if err := h.authorizeSessionSell(ctx, session, row.CloudAccountID); err != nil { + return nil, err + } + + if !strings.EqualFold(row.ListingState, "active") { + return nil, NewClientError(409, "no active listing found for this RI; current state: "+row.ListingState) + } + + cfg, err := h.loadAWSConfigWithRegion(ctx, row.Region) + if err != nil { + return nil, fmt.Errorf("failed to load AWS config: %w", err) + } + + ec2Client := h.buildMarketplaceEC2Client(cfg) + + result, err := ec2Client.CancelMarketplaceListing(ctx, row.ListingID) + if err != nil { + logging.Warnf("marketplace: CancelReservedInstancesListing for listing %s failed: %v", row.ListingID, err) + return nil, NewClientError(502, "AWS cancel listing failed: "+err.Error()) + } + + if dbErr := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, result.ListingID, result.State); dbErr != nil { + logging.Errorf("marketplace: listing cancelled (%s) but DB update failed: %v", result.ListingID, dbErr) + } + + return map[string]string{"listing_id": result.ListingID, "listing_state": result.State}, nil +} + +// authorizeSessionSell returns nil when the session is permitted to perform a +// sell/marketplace action under the sell-any / sell-own RBAC rules. The +// cloudAccountID is the cloud account that owns the RI (used for sell-own to +// confirm the session's allowed accounts cover that account). Returns a 403 +// ClientError otherwise. +// +// sell-own semantics: a non-admin user can list/cancel RIs for cloud accounts +// they are permitted to access (allowed_accounts covers the account). This is +// intentionally looser than cancel-own (which checks the session UserID against +// created_by_user_id) because purchase_history rows lack a created_by_user_id. +func (h *Handler) authorizeSessionSell(ctx context.Context, session *Session, cloudAccountID *string) error { + if h.auth == nil { + return NewClientError(500, "authentication service not configured") + } + + // Admins are recognised by holding the full-access admin capability + // (auth migrated from role-based to group-membership-only, issue #907). + isAdmin, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionAdmin, auth.ResourceAll) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if isAdmin { + return nil + } + + hasAny, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionSellAny, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if hasAny { + return nil + } + + hasOwn, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionSellOwn, auth.ResourcePurchases) + if err != nil { + return fmt.Errorf("permission check failed: %w", err) + } + if !hasOwn { + return NewClientError(403, "permission denied: requires sell-any or sell-own on purchases") + } + + // sell-own: verify the session covers the cloud account that holds the RI. + // When cloudAccountID is nil (ambient/legacy row), deny for non-admins. + if cloudAccountID == nil { + return NewClientError(403, "permission denied: cannot sell an RI from an ambiguous (non-per-account) purchase row without sell-any") + } + return h.authorizeAllowedAccount(ctx, session, *cloudAccountID) +} + +// authorizeAllowedAccount returns nil when the session's allowed_accounts +// permit access to the given cloud account UUID. Returns 403 otherwise. +func (h *Handler) authorizeAllowedAccount(ctx context.Context, session *Session, cloudAccountID string) error { + if h.auth != nil { + // Admins are recognised by holding the full-access admin capability + // (auth migrated from role-based to group-membership-only, issue #907). + isAdmin, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionAdmin, auth.ResourceAll) + if err != nil { + return fmt.Errorf("admin permission check failed: %w", err) + } + if isAdmin { + return nil + } + } + allowed, err := h.getAllowedAccounts(ctx, session) + if err != nil { + return fmt.Errorf("failed to check allowed accounts: %w", err) + } + // Empty list means "no restriction" (the user has access to all accounts). + if len(allowed) == 0 { + return nil + } + for _, id := range allowed { + if id == "*" || id == cloudAccountID { + return nil + } + } + return NewClientError(403, "permission denied: purchase is in a cloud account not covered by your session's allowed accounts") +} + +// resolveMarketplacePriceSchedule returns a normalised price schedule for the +// given RI. When the caller supplied an explicit schedule it is validated and +// returned unchanged. When the caller omitted the schedule (nil / empty), a +// single-tier default is computed: total remaining value * 0.95 (5% discount +// to attract buyers; the 12% AWS fee is applied by the Marketplace on top). +// +// remainingMonths is rec.Term (AWS RI term in months), upfrontCost is the +// original upfront paid, monthlyCost is the ongoing recurring charge. +func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths int, upfrontCost, monthlyCost float64) ([]MarketplacePriceTier, error) { + if len(supplied) > 0 { + for i, t := range supplied { + if t.TermMonths <= 0 { + return nil, fmt.Errorf("price_schedule[%d]: term_months must be a positive integer", i) + } + if t.Price < 0 { + return nil, fmt.Errorf("price_schedule[%d]: price must be non-negative", i) + } + } + return supplied, nil + } + + // Default: spread (upfront + recurring) * 0.95 across remaining term. + if remainingMonths <= 0 { + remainingMonths = 1 // defensive: should not happen for an active RI + } + totalValue := upfrontCost + (monthlyCost * float64(remainingMonths)) + listPrice := totalValue * 0.95 + if listPrice < 0 { + listPrice = 0 + } + return []MarketplacePriceTier{ + {TermMonths: int64(remainingMonths), Price: listPrice}, + }, nil +} diff --git a/internal/api/router.go b/internal/api/router.go index 6eab313bd..81e5c5ffc 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -188,6 +188,12 @@ func (r *Router) registerRoutes() { // expected_refund_amount in the POST /revoke body. {PathPrefix: "/api/purchases/", PathSuffix: "/revoke/calculate", Method: "GET", Handler: r.calculateRevokeHandler, Auth: AuthUser}, + // RI Marketplace listing (issue #292). Session-authed only; the + // handler enforces the sell-any/sell-own RBAC matrix and checks + // that the purchase_history row has offering_class='standard'. + {PathPrefix: "/api/purchases/", PathSuffix: "/marketplace-list", Method: "POST", Handler: r.marketplaceListHandler, Auth: AuthUser}, + {PathPrefix: "/api/purchases/", PathSuffix: "/marketplace-cancel", Method: "POST", Handler: r.marketplaceCancelHandler, Auth: AuthUser}, + // Planned purchases endpoints (must come before generic /api/purchases/{id}). // All now AuthUser (PR-A of #660): handler-level requirePermission // is the actual gate for pause/resume/run/delete. @@ -595,6 +601,14 @@ func (r *Router) calculateRevokeHandler(ctx context.Context, req *events.LambdaF return r.h.calculateAzureRevoke(ctx, req, params["id"]) } +func (r *Router) marketplaceListHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.marketplaceList(ctx, req, params["id"]) +} + +func (r *Router) marketplaceCancelHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.marketplaceCancel(ctx, req, params["id"]) +} + func (r *Router) getPlannedPurchasesHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.getPlannedPurchases(ctx, req) } diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index 38a0126bc..7eeae6715 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -279,13 +279,14 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // 11 = 6 read/plan-author + delete:plans (PR-A #660) + // 12 = 6 read/plan-author + delete:plans (PR-A #660) // + update:purchases (PR-A #660) // + cancel-own:purchases (issue #46) // + retry-own:purchases (issue #47) - // + revoke-own:purchases (issue #290). + // + revoke-own:purchases (issue #290) + // + sell-own:purchases (issue #292). // NOTE: approve-own removed (issue #1407, four-eyes). - assert.Len(t, permissions, 11) + assert.Len(t, permissions, 12) mockStore.AssertExpectations(t) }) @@ -354,11 +355,11 @@ func TestService_GetUserPermissions(t *testing.T) { permissions, err := service.GetUserPermissions(ctx, "user-123") require.NoError(t, err) - // 11 standard-group (incl. delete:plans (PR-A #660) + update:purchases (PR-A #660) + // 12 standard-group (incl. delete:plans (PR-A #660) + update:purchases (PR-A #660) // + cancel-own (#46) + retry-own (#47) - // + revoke-own (#290):purchases; approve-own removed per #1407 four-eyes) - // + 1 group1 + 1 group2 = 13 - assert.Len(t, permissions, 13) + // + revoke-own (#290) + sell-own (#292):purchases; approve-own removed + // per #1407 four-eyes) + 1 group1 + 1 group2 = 14 + assert.Len(t, permissions, 14) mockStore.AssertExpectations(t) }) @@ -401,13 +402,14 @@ func TestService_GetUserPermissions(t *testing.T) { require.NoError(t, err) // Should have only the resolvable group's permissions; the missing // group is skipped. - // 11 = 6 read/plan-author + delete:plans (PR-A #660) + // 12 = 6 read/plan-author + delete:plans (PR-A #660) // + update:purchases (PR-A #660) // + cancel-own:purchases (issue #46) // + retry-own:purchases (issue #47) - // + revoke-own:purchases (issue #290). + // + revoke-own:purchases (issue #290) + // + sell-own:purchases (issue #292). // NOTE: approve-own removed (issue #1407, four-eyes). - assert.Len(t, permissions, 11) + assert.Len(t, permissions, 12) mockStore.AssertExpectations(t) }) diff --git a/internal/auth/types.go b/internal/auth/types.go index 7fc9451b9..dbe5d0026 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -461,6 +461,20 @@ const ( // escalating to admin. ActionRevokeOwn = "revoke-own" ActionRevokeAny = "revoke-any" + // ActionSellOwn / ActionSellAny gate the "Sell on Marketplace" button on + // Standard RI purchase history rows (issue #292). Mirror image of the + // cancel-{own,any} / retry-{own,any} / approve-{own,any} family: + // + // * RoleAdmin — implicit via {ActionAdmin, ResourceAll}; covers + // both verbs. + // * RoleUser — DefaultUserPermissions() adds sell-own:purchases so + // every user can list RIs they purchased themselves. + // * RoleReadOnly — neither verb. + // + // sell-any has no default non-admin grant; the constant exists so a + // custom operator group can list any Standard RI without admin escalation. + ActionSellOwn = "sell-own" + ActionSellAny = "sell-any" ) // Predefined resources. @@ -540,6 +554,12 @@ func DefaultUserPermissions() []Permission { // creator UUID matches. Legacy rows with NULL creator are out of reach // for non-admins (email-token paths have no revocation escape hatch). {Action: ActionRevokeOwn, Resource: ResourcePurchases}, + // sell-own:purchases — every authenticated user can list Standard + // RIs they purchased themselves on the AWS Marketplace (issue #292). + // The handler still requires the purchase_history row to carry + // offering_class = 'standard' and the cloud account to match the + // row's cloud_account_id before creating the listing. + {Action: ActionSellOwn, Resource: ResourcePurchases}, } } diff --git a/internal/auth/types_test.go b/internal/auth/types_test.go index c6ed9ebe0..7c222c6ca 100644 --- a/internal/auth/types_test.go +++ b/internal/auth/types_test.go @@ -21,8 +21,9 @@ func TestDefaultPermissions(t *testing.T) { // + cancel-own:purchases (issue #46) // + retry-own:purchases (issue #47) // + revoke-own:purchases (issue #290) - // NOTE: approve-own was removed (issue #1407, four-eyes) = 11. - assert.Len(t, perms, 11) + // + sell-own:purchases (issue #292) + // NOTE: approve-own was removed (issue #1407, four-eyes) = 12. + assert.Len(t, perms, 12) actions := make(map[string]bool) for _, p := range perms { @@ -44,6 +45,8 @@ func TestDefaultPermissions(t *testing.T) { // user permission. Self-approval requires an explicit custom-group grant. assert.False(t, actions[ActionApproveOwn+":"+ResourcePurchases], "approve-own must not be in DefaultUserPermissions (four-eyes, issue #1407)") + // sell-own:purchases must be a default user permission (issue #292). + assert.True(t, actions[ActionSellOwn+":"+ResourcePurchases]) }) t.Run("DefaultReadOnlyPermissions returns readonly access", func(t *testing.T) { diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 992c7f995..e63458d86 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -172,9 +172,11 @@ type StoreInterface interface { // across providers (aws/123 vs azure/123) from leaking the wrong rows. GetPurchaseHistoryFiltered(ctx context.Context, filter PurchaseHistoryFilter) ([]PurchaseHistoryRecord, error) // GetPurchaseHistoryByPurchaseID returns the single purchase_history row - // whose purchase_id matches. Returns (nil, nil) when no row is found. - // Used by the revoke endpoint to load the record before calling the - // provider cancel API (issue #290). + // whose purchase_id matches (AWS ReservedInstancesId / Azure reservation + // ID). Returns (nil, nil) when no row is found. Used by the revoke + // endpoint to load the record before calling the provider cancel API + // (issue #290) and by the marketplace-list handler to validate + // offering_class and look up the cloud account (issue #292). GetPurchaseHistoryByPurchaseID(ctx context.Context, purchaseID string) (*PurchaseHistoryRecord, error) // MarkPurchaseRevoked stamps revoked_at, revoked_via, and optionally // support_case_id on a purchase_history row identified by purchase_id. @@ -209,6 +211,12 @@ type StoreInterface interface { // finalize_revocations scheduled sweep calls this to retry the DB write. GetPurchaseHistoryInFlight(ctx context.Context) ([]*PurchaseHistoryRecord, error) + // UpdatePurchaseHistoryListing stamps the AWS marketplace listing_id and + // listing_state onto a purchase_history row. Called after + // CreateReservedInstancesListing succeeds (listing_state="active") and + // on subsequent poll/cancel transitions (issue #292). + UpdatePurchaseHistoryListing(ctx context.Context, purchaseID, listingID, listingState string) error + // RI Exchange history SaveRIExchangeRecord(ctx context.Context, record *RIExchangeRecord) error GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 2065a1d04..e6cc55752 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1638,8 +1638,8 @@ func (s *PostgresStore) SavePurchaseHistory(ctx context.Context, record *Purchas account_id, purchase_id, timestamp, provider, service, region, resource_type, count, term, payment, upfront_cost, monthly_cost, estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, - source, revocation_window_closes_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) + source, revocation_window_closes_at, offering_class + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) ` _, err := s.db.Exec(ctx, query, @@ -1662,6 +1662,7 @@ func (s *PostgresStore) SavePurchaseHistory(ctx context.Context, record *Purchas record.CloudAccountID, record.Source, record.RevocationWindowClosesAt, + nullStringFromString(record.OfferingClass), ) if err != nil { @@ -1671,13 +1672,35 @@ func (s *PostgresStore) SavePurchaseHistory(ctx context.Context, record *Purchas return nil } +// UpdatePurchaseHistoryListing stamps the marketplace listing fields onto a +// purchase_history row identified by its purchase_id (RI reservation ID). +// Called by the marketplace-list handler after CreateReservedInstancesListing +// succeeds (listing_id + listing_state="active") and by the status-poll path +// when AWS reports closed or cancelled. +func (s *PostgresStore) UpdatePurchaseHistoryListing(ctx context.Context, purchaseID, listingID, listingState string) error { + query := ` + UPDATE purchase_history + SET listing_id = $1, listing_state = $2 + WHERE purchase_id = $3 + ` + tag, err := s.db.Exec(ctx, query, listingID, listingState, purchaseID) + if err != nil { + return fmt.Errorf("failed to update listing state for purchase %s: %w", purchaseID, err) + } + if tag.RowsAffected() == 0 { + return fmt.Errorf("purchase %s not found in history", purchaseID) + } + return nil +} + // GetPurchaseHistory retrieves purchase history for an account. func (s *PostgresStore) GetPurchaseHistory(ctx context.Context, accountID string, limit int) ([]PurchaseHistoryRecord, error) { query := ` SELECT account_id, purchase_id, timestamp, provider, service, region, resource_type, count, term, payment, upfront_cost, monthly_cost, estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, - revocation_window_closes_at, revoked_at, revoked_via, support_case_id + revocation_window_closes_at, revoked_at, revoked_via, support_case_id, + offering_class, listing_id, listing_state FROM purchase_history WHERE account_id = $1 ORDER BY timestamp DESC @@ -1693,7 +1716,8 @@ func (s *PostgresStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([ SELECT account_id, purchase_id, timestamp, provider, service, region, resource_type, count, term, payment, upfront_cost, monthly_cost, estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, - revocation_window_closes_at, revoked_at, revoked_via, support_case_id + revocation_window_closes_at, revoked_at, revoked_via, support_case_id, + offering_class, listing_id, listing_state FROM purchase_history ORDER BY timestamp DESC LIMIT $1 @@ -1868,7 +1892,8 @@ func (s *PostgresStore) GetPurchaseHistoryFiltered( SELECT account_id, purchase_id, timestamp, provider, service, region, resource_type, count, term, payment, upfront_cost, monthly_cost, estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, - revocation_window_closes_at, revoked_at, revoked_via, support_case_id + revocation_window_closes_at, revoked_at, revoked_via, support_case_id, + offering_class, listing_id, listing_state FROM purchase_history%s ORDER BY timestamp DESC LIMIT $%d @@ -1883,10 +1908,13 @@ func (s *PostgresStore) GetPurchaseHistoryFiltered( // account_id, purchase_id, timestamp, provider, service, region, // resource_type, count, term, payment, upfront_cost, monthly_cost, // estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, -// revocation_window_closes_at, revoked_at, revoked_via, support_case_id +// revocation_window_closes_at, revoked_at, revoked_via, support_case_id, +// offering_class, listing_id, listing_state // -// The revocation columns were added in migration 000057. Queries must -// include them explicitly so the Scan targets stay in sync. +// The revocation columns were added in migration 000057; the marketplace +// listing columns (offering_class, listing_id, listing_state) in the #292 +// migration. Queries must include them explicitly so the Scan targets stay +// in sync. func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, args ...any) ([]PurchaseHistoryRecord, error) { rows, err := s.db.Query(ctx, query, args...) if err != nil { @@ -1897,7 +1925,7 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, records := make([]PurchaseHistoryRecord, 0) for rows.Next() { var record PurchaseHistoryRecord - var planID, planName, cloudAccountID sql.NullString + var planID, planName, cloudAccountID, offeringClass, listingID, listingState sql.NullString var monthlyCost sql.NullFloat64 var revocationWindowClosesAt, revokedAt *time.Time var revokedVia, supportCaseID sql.NullString @@ -1924,6 +1952,9 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, &revokedAt, &revokedVia, &supportCaseID, + &offeringClass, + &listingID, + &listingState, ) if err != nil { return nil, fmt.Errorf("failed to scan purchase history: %w", err) @@ -1954,6 +1985,15 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, if supportCaseID.Valid { record.SupportCaseID = supportCaseID.String } + if offeringClass.Valid { + record.OfferingClass = offeringClass.String + } + if listingID.Valid { + record.ListingID = listingID.String + } + if listingState.Valid { + record.ListingState = listingState.String + } records = append(records, record) } @@ -1966,14 +2006,16 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, // does not exist. The revocation-window columns (revocation_window_closes_at, // revoked_at, revoked_via, support_case_id) are read alongside the base // columns so the revoke endpoint can check idempotency without a second round -// trip (issue #290). +// trip (issue #290). The marketplace columns (offering_class, listing_id, +// listing_state) are read so the marketplace-list handler can validate +// offering_class and look up the cloud account (issue #292). func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purchaseID string) (*PurchaseHistoryRecord, error) { query := ` SELECT account_id, purchase_id, timestamp, provider, service, region, resource_type, count, term, payment, upfront_cost, monthly_cost, estimated_savings, plan_id, plan_name, ramp_step, cloud_account_id, revocation_window_closes_at, revoked_at, revoked_via, support_case_id, - revocation_in_flight + revocation_in_flight, offering_class, listing_id, listing_state FROM purchase_history WHERE purchase_id = $1 LIMIT 1 @@ -1992,6 +2034,7 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc var planID, planName, cloudAccountID sql.NullString var revocationWindowClosesAt, revokedAt *time.Time var revokedVia, supportCaseID sql.NullString + var offeringClass, listingID, listingState sql.NullString if err := rows.Scan( &r.AccountID, @@ -2016,6 +2059,9 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc &revokedVia, &supportCaseID, &r.RevocationInFlight, + &offeringClass, + &listingID, + &listingState, ); err != nil { return nil, fmt.Errorf("GetPurchaseHistoryByPurchaseID scan: %w", err) } @@ -2037,6 +2083,15 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc if supportCaseID.Valid { r.SupportCaseID = supportCaseID.String } + if offeringClass.Valid { + r.OfferingClass = offeringClass.String + } + if listingID.Valid { + r.ListingID = listingID.String + } + if listingState.Valid { + r.ListingState = listingState.String + } return &r, rows.Err() } diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index bd85724e5..5d0e3199b 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -745,6 +745,8 @@ func TestPGXMock_GetPurchaseHistory_Success(t *testing.T) { "estimated_savings", "plan_id", "plan_name", "ramp_step", "cloud_account_id", // revocation columns (issue #290) "revocation_window_closes_at", "revoked_at", "revoked_via", "support_case_id", + // marketplace columns (issue #292) + "offering_class", "listing_id", "listing_state", } rows := pgxmock.NewRows(cols). AddRow("acc-1", "pur-1", now, "aws", "ec2", "us-east-1", @@ -752,11 +754,16 @@ func TestPGXMock_GetPurchaseHistory_Success(t *testing.T) { sql.NullString{Valid: true, String: "plan-1"}, sql.NullString{Valid: true, String: "My Plan"}, 1, sql.NullString{Valid: true, String: "cloud-acct-1"}, - nil, nil, sql.NullString{}, sql.NullString{}). + // revocation columns (issue #290) + nil, nil, sql.NullString{}, sql.NullString{}, + // marketplace columns (issue #292) + sql.NullString{Valid: true, String: "standard"}, + sql.NullString{}, sql.NullString{}). AddRow("acc-1", "pur-2", now, "aws", "rds", "us-west-2", "db.t3.medium", 1, 3, "all-upfront", 200.0, 0.0, 100.0, sql.NullString{}, sql.NullString{}, 0, sql.NullString{}, - nil, nil, sql.NullString{}, sql.NullString{}) + nil, nil, sql.NullString{}, sql.NullString{}, + sql.NullString{}, sql.NullString{}, sql.NullString{}) mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).WillReturnRows(rows) records, err := store.GetPurchaseHistory(ctx, "acc-1", 10) @@ -772,12 +779,14 @@ func TestPGXMock_GetPurchaseHistory_Success(t *testing.T) { // purchaseHistoryCols lists the SELECT columns for purchase_history rows in // the order GetPurchaseHistoryFiltered scans them. Keep in sync with // queryPurchaseHistory in store_postgres.go (issue #290 added the 4 revocation -// columns at positions 18-21). +// columns at positions 18-21; issue #292 added the 3 marketplace columns at +// positions 22-24). var purchaseHistoryCols = []string{ "account_id", "purchase_id", "timestamp", "provider", "service", "region", "resource_type", "count", "term", "payment", "upfront_cost", "monthly_cost", "estimated_savings", "plan_id", "plan_name", "ramp_step", "cloud_account_id", "revocation_window_closes_at", "revoked_at", "revoked_via", "support_case_id", + "offering_class", "listing_id", "listing_state", } // purchaseHistoryRow builds a single AddRow tuple matching purchaseHistoryCols. @@ -788,6 +797,8 @@ func purchaseHistoryRow(now time.Time, provider, acct string) []interface{} { sql.NullString{}, sql.NullString{}, 0, sql.NullString{}, // revocation columns (issue #290): all null for non-revoked rows nil, nil, sql.NullString{}, sql.NullString{}, + // marketplace columns (issue #292): all null for unlisted rows + sql.NullString{}, sql.NullString{}, sql.NullString{}, } } @@ -1973,8 +1984,9 @@ func TestPGXMock_SavePurchaseHistory_Success(t *testing.T) { store := storeWith(mock) ctx := context.Background() - // 19 columns: original 18 + revocation_window_closes_at (issue #290). - mock.ExpectExec("INSERT INTO purchase_history").WithArgs(anyArgsCfg(19)...). + // 20 columns: original 18 + revocation_window_closes_at (issue #290) + // + offering_class (issue #292). + mock.ExpectExec("INSERT INTO purchase_history").WithArgs(anyArgsCfg(20)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) err := store.SavePurchaseHistory(ctx, &PurchaseHistoryRecord{ diff --git a/internal/config/types.go b/internal/config/types.go index 75b13addc..65f9e8113 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -787,6 +787,20 @@ type PurchaseHistoryRecord struct { // can detect and retry the MarkPurchaseRevoked write without re-calling Azure // (preventing a duplicate-refund error). RevocationInFlight bool `json:"revocation_in_flight,omitempty" dynamodbav:"revocation_in_flight,omitempty"` + + // OfferingClass records whether this commitment is a 'standard' or + // 'convertible' RI. NULL on pre-migration rows. The Sell-on-Marketplace + // button renders only when this equals "standard" (issue #292). + // Persisted in purchase_history via migration 000060. + OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` + // ListingID is the AWS ReservedInstancesListingId returned by + // CreateReservedInstancesListing. Empty when the RI has not been + // listed. Persisted in purchase_history via migration 000060. + ListingID string `json:"listing_id,omitempty" dynamodbav:"listing_id,omitempty"` + // ListingState mirrors the AWS marketplace listing state: "active", + // "cancelled", or "closed" (sold). Empty when not listed. + // Persisted in purchase_history via migration 000060. + ListingState string `json:"listing_state,omitempty" dynamodbav:"listing_state,omitempty"` } // RIExchangeRecord represents a record in the ri_exchange_history table. diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql new file mode 100644 index 000000000..dda98d245 --- /dev/null +++ b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql @@ -0,0 +1,5 @@ +-- Revert migration 000060 +ALTER TABLE purchase_history + DROP COLUMN IF EXISTS offering_class, + DROP COLUMN IF EXISTS listing_id, + DROP COLUMN IF EXISTS listing_state; diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql new file mode 100644 index 000000000..673749cb7 --- /dev/null +++ b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql @@ -0,0 +1,18 @@ +-- Migration 000060: add RI Marketplace listing columns to purchase_history. +-- +-- offering_class: 'convertible' or 'standard'; NULL for pre-migration rows. +-- The Sell button renders only when offering_class = 'standard' and the RI is +-- active with remaining term. +-- +-- listing_id: the AWS ReservedInstancesListingId returned by +-- CreateReservedInstancesListing. NULL when not listed. +-- listing_state: mirrors the AWS listing state (active/cancelled/closed). NULL +-- when not listed. +-- +-- All three columns are nullable and added with IF NOT EXISTS so the +-- migration is idempotent on re-apply. + +ALTER TABLE purchase_history + ADD COLUMN IF NOT EXISTS offering_class TEXT, + ADD COLUMN IF NOT EXISTS listing_id TEXT, + ADD COLUMN IF NOT EXISTS listing_state TEXT; diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 90aef78c2..e77f64397 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -389,7 +389,7 @@ func (m *MockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter return v, args.Error(1) } -// GetPurchaseHistoryByPurchaseID mocks the GetPurchaseHistoryByPurchaseID operation (issue #290). +// GetPurchaseHistoryByPurchaseID mocks the single-row lookup by purchase_id (issues #290, #292). func (m *MockConfigStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purchaseID string) (*config.PurchaseHistoryRecord, error) { args := m.Called(ctx, purchaseID) if args.Get(0) == nil { @@ -443,6 +443,12 @@ func (m *MockConfigStore) GetPurchaseHistoryInFlight(ctx context.Context) ([]*co return v, args.Error(1) } +// UpdatePurchaseHistoryListing mocks stamping listing_id and listing_state (issue #292). +func (m *MockConfigStore) UpdatePurchaseHistoryListing(ctx context.Context, purchaseID, listingID, listingState string) error { + args := m.Called(ctx, purchaseID, listingID, listingState) + return args.Error(0) +} + func (m *MockConfigStore) SaveRIExchangeRecord(ctx context.Context, record *config.RIExchangeRecord) error { args := m.Called(ctx, record) return args.Error(0) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index e2e304d00..6f89dd151 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -368,3 +368,6 @@ func (m *mockConfigStoreForHealth) UpdateGlobalConfigAtomic(_ context.Context, a } return cfg, nil } +func (m *mockConfigStoreForHealth) UpdatePurchaseHistoryListing(_ context.Context, _, _, _ string) error { + return nil +} diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 0e8166a77..983deeab8 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -28,6 +28,9 @@ type EC2API interface { GetReservedInstancesExchangeQuote(ctx context.Context, params *ec2.GetReservedInstancesExchangeQuoteInput, optFns ...func(*ec2.Options)) (*ec2.GetReservedInstancesExchangeQuoteOutput, error) AcceptReservedInstancesExchangeQuote(ctx context.Context, params *ec2.AcceptReservedInstancesExchangeQuoteInput, optFns ...func(*ec2.Options)) (*ec2.AcceptReservedInstancesExchangeQuoteOutput, error) CreateTags(ctx context.Context, params *ec2.CreateTagsInput, optFns ...func(*ec2.Options)) (*ec2.CreateTagsOutput, error) + CreateReservedInstancesListing(ctx context.Context, params *ec2.CreateReservedInstancesListingInput, optFns ...func(*ec2.Options)) (*ec2.CreateReservedInstancesListingOutput, error) + DescribeReservedInstancesListings(ctx context.Context, params *ec2.DescribeReservedInstancesListingsInput, optFns ...func(*ec2.Options)) (*ec2.DescribeReservedInstancesListingsOutput, error) + CancelReservedInstancesListing(ctx context.Context, params *ec2.CancelReservedInstancesListingInput, optFns ...func(*ec2.Options)) (*ec2.CancelReservedInstancesListingOutput, error) } // Client handles AWS EC2 Reserved Instances @@ -923,3 +926,131 @@ func normalizationFactorForInstanceType(instanceType string) float64 { } return exchange.NormalizationFactorForSize(parts[1]) } + +// MarketplaceListingRequest carries the parameters for +// CreateMarketplaceListing. PriceSchedule is required by the AWS API; a +// nil/empty slice is rejected before the outbound call is made. +type MarketplaceListingRequest struct { + // ReservedInstancesID is the AWS ReservedInstancesId to list. + ReservedInstancesID string + // ClientToken is a caller-supplied idempotency token (UUID). AWS dedupes + // CreateReservedInstancesListing calls sharing the same token for the same + // RI within a short window. + ClientToken string + // PriceSchedule is the per-month price schedule. At least one entry is + // required. Each entry specifies how many months the price applies and the + // list price per unit. + PriceSchedule []MarketplacePriceTier +} + +// MarketplacePriceTier represents one tier of the AWS RI Marketplace price +// schedule. Term is the number of months this price applies, counting down +// from the remaining RI term. Price is the per-unit listing price in USD. +type MarketplacePriceTier struct { + // Term is the remaining-months count this tier covers. + Term int64 + // Price is the USD list price per unit for this tier. + Price float64 +} + +// MarketplaceListingResult is returned by CreateMarketplaceListing and +// DescribeMarketplaceListing. +type MarketplaceListingResult struct { + // ListingID is the AWS ReservedInstancesListingId. + ListingID string + // State is the AWS listing state: active, cancelled, closed, pending-fulfillment, etc. + State string +} + +// CreateMarketplaceListing calls ec2:CreateReservedInstancesListing. +// Only Standard (not Convertible) RIs can be listed; AWS rejects requests +// for Convertible RIs with an explicit API error — the caller should gate on +// offering_class == "standard" before calling this to surface a cleaner error. +// AWS also requires the seller account to have a US bank account on file; +// that precondition is checked in the API handler so the UI can show a +// tailored message before making the API call. +func (c *Client) CreateMarketplaceListing(ctx context.Context, req MarketplaceListingRequest) (MarketplaceListingResult, error) { + if len(req.PriceSchedule) == 0 { + return MarketplaceListingResult{}, fmt.Errorf("price schedule must have at least one tier") + } + if req.ReservedInstancesID == "" { + return MarketplaceListingResult{}, fmt.Errorf("reserved instances ID is required") + } + + awsSchedule := make([]types.PriceScheduleSpecification, 0, len(req.PriceSchedule)) + for _, tier := range req.PriceSchedule { + tier := tier // capture loop var + awsSchedule = append(awsSchedule, types.PriceScheduleSpecification{ + Term: aws.Int64(tier.Term), + Price: aws.Float64(tier.Price), + CurrencyCode: types.CurrencyCodeValuesUsd, + }) + } + + input := &ec2.CreateReservedInstancesListingInput{ + ReservedInstancesId: aws.String(req.ReservedInstancesID), + ClientToken: aws.String(req.ClientToken), + InstanceCount: aws.Int32(1), + PriceSchedules: awsSchedule, + } + + out, err := c.client.CreateReservedInstancesListing(ctx, input) + if err != nil { + return MarketplaceListingResult{}, fmt.Errorf("CreateReservedInstancesListing failed: %w", err) + } + if len(out.ReservedInstancesListings) == 0 { + return MarketplaceListingResult{}, fmt.Errorf("CreateReservedInstancesListing returned empty response") + } + listing := out.ReservedInstancesListings[0] + state := "" + if listing.Status != "" { + state = string(listing.Status) + } + listingID := "" + if listing.ReservedInstancesListingId != nil { + listingID = aws.ToString(listing.ReservedInstancesListingId) + } + return MarketplaceListingResult{ListingID: listingID, State: state}, nil +} + +// DescribeMarketplaceListing polls the status of a single listing by ID. +// Returns an error when the listing is not found. +func (c *Client) DescribeMarketplaceListing(ctx context.Context, listingID string) (MarketplaceListingResult, error) { + out, err := c.client.DescribeReservedInstancesListings(ctx, &ec2.DescribeReservedInstancesListingsInput{ + ReservedInstancesListingId: aws.String(listingID), + }) + if err != nil { + return MarketplaceListingResult{}, fmt.Errorf("DescribeReservedInstancesListings failed: %w", err) + } + if len(out.ReservedInstancesListings) == 0 { + return MarketplaceListingResult{}, fmt.Errorf("listing %s not found", listingID) + } + listing := out.ReservedInstancesListings[0] + state := string(listing.Status) + resolvedID := "" + if listing.ReservedInstancesListingId != nil { + resolvedID = aws.ToString(listing.ReservedInstancesListingId) + } + return MarketplaceListingResult{ListingID: resolvedID, State: state}, nil +} + +// CancelMarketplaceListing calls ec2:CancelReservedInstancesListing to +// withdraw an active listing. Returns the updated listing state. +func (c *Client) CancelMarketplaceListing(ctx context.Context, listingID string) (MarketplaceListingResult, error) { + out, err := c.client.CancelReservedInstancesListing(ctx, &ec2.CancelReservedInstancesListingInput{ + ReservedInstancesListingId: aws.String(listingID), + }) + if err != nil { + return MarketplaceListingResult{}, fmt.Errorf("CancelReservedInstancesListing failed: %w", err) + } + if len(out.ReservedInstancesListings) == 0 { + return MarketplaceListingResult{}, fmt.Errorf("CancelReservedInstancesListing returned empty response") + } + listing := out.ReservedInstancesListings[0] + state := string(listing.Status) + resolvedID := "" + if listing.ReservedInstancesListingId != nil { + resolvedID = aws.ToString(listing.ReservedInstancesListingId) + } + return MarketplaceListingResult{ListingID: resolvedID, State: state}, nil +} From 4eff945668e742b23d5354b6ab9ffef7eb4b869f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 20:00:08 +0200 Subject: [PATCH 02/21] feat(marketplace): sell/cancel Standard RIs on AWS Marketplace - frontend (closes #292) Add Sell on Marketplace / Cancel listing buttons to purchase history rows: - types.ts: extend HistoryPurchase with offering_class, listing_id, listing_state - api/purchases.ts: createMarketplaceListing + cancelMarketplaceListing; re-export from api/index.ts - history.ts: canSellOnMarketplace / canCancelMarketplaceListing predicates; renderActionCell renders Sell/Cancel buttons for Standard RI completed rows; wireRowActionHandlers wires confirm-dialog + API + toast + reload for both buttons --- frontend/src/api/index.ts | 6 +- frontend/src/api/purchases.ts | 45 ++++++++++++++ frontend/src/history.ts | 113 +++++++++++++++++++++++++++++++++- frontend/src/types.ts | 12 ++++ 4 files changed, 173 insertions(+), 3 deletions(-) diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index e3fb66f73..e59ac15d9 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -141,9 +141,11 @@ export { runPlannedPurchase, deletePlannedPurchase, createPlannedPurchases, - revokePurchase + revokePurchase, + createMarketplaceListing, + cancelMarketplaceListing } from './purchases'; -export type { RetryPurchaseResult, RevokePurchaseResult } from './purchases'; +export type { RetryPurchaseResult, RevokePurchaseResult, MarketplacePriceTier, MarketplaceListResult } from './purchases'; // Re-export users functions export { diff --git a/frontend/src/api/purchases.ts b/frontend/src/api/purchases.ts index b0f19bb65..a965c74c2 100644 --- a/frontend/src/api/purchases.ts +++ b/frontend/src/api/purchases.ts @@ -168,3 +168,48 @@ export async function createPlannedPurchases(planId: string, count: number, star body: JSON.stringify({ count, start_date: startDate }) }); } + +// ── RI Marketplace (issue #292) ─────────────────────────────────────────────── + +export interface MarketplacePriceTier { + /** Number of months remaining this price tier covers. */ + term_months: number; + /** USD list price per unit for this tier. */ + price: number; +} + +export interface MarketplaceListResult { + listing_id: string; + listing_state: string; + price_schedule: MarketplacePriceTier[]; + aws_fee_percent: number; + note?: string; +} + +/** + * Create an AWS RI Marketplace listing for a Standard Reserved Instance. + * purchaseId is the purchase_history.purchase_id (AWS ReservedInstancesId). + * priceSchedule is optional: when omitted the backend computes a default. + */ +export async function createMarketplaceListing( + purchaseId: string, + priceSchedule?: MarketplacePriceTier[], +): Promise { + const body = priceSchedule && priceSchedule.length > 0 + ? JSON.stringify({ price_schedule: priceSchedule }) + : undefined; + return apiRequest(`/purchases/${purchaseId}/marketplace-list`, { + method: 'POST', + ...(body ? { body } : {}), + }); +} + +/** + * Cancel an active AWS RI Marketplace listing. + */ +export async function cancelMarketplaceListing(purchaseId: string): Promise<{ listing_id: string; listing_state: string }> { + return apiRequest<{ listing_id: string; listing_state: string }>( + `/purchases/${purchaseId}/marketplace-cancel`, + { method: 'POST' }, + ); +} diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 92678e7e3..4b659b830 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -593,6 +593,39 @@ function retryThresholdReached(p: HistoryPurchase): boolean { return (p.retry_attempt_n ?? 0) >= RETRY_THRESHOLD; } +// canSellOnMarketplace returns true when the current session is permitted +// to list the given completed history row on the AWS RI Marketplace +// (issue #292). UX gate only -- the backend authorizeSessionSell in +// internal/api/handler_marketplace.go remains the security boundary; a +// false-positive here surfaces as a 403 toast on click. +// +// Conditions: +// * row must be a completed purchase (status "completed" or absent); +// * offering_class must be "standard"; +// * no active listing already (listing_state != "active"); and +// * admin, or non-admin user (sell-own covers their own accounts -- +// we can't efficiently check per-account ownership client-side, so +// we show the button for all non-admin users and let the backend 403 +// when the account is out of scope). +function canSellOnMarketplace(p: HistoryPurchase): boolean { + const status = normalizeStatus(p).toLowerCase(); + if (status !== 'completed') return false; + if ((p.offering_class || '').toLowerCase() !== 'standard') return false; + if ((p.listing_state || '').toLowerCase() === 'active') return false; + const user = getCurrentUser(); + if (!user) return false; + return true; +} + +// canCancelMarketplaceListing returns true when there is an active listing +// that the current session can cancel. +function canCancelMarketplaceListing(p: HistoryPurchase): boolean { + if ((p.listing_state || '').toLowerCase() !== 'active') return false; + const user = getCurrentUser(); + if (!user) return false; + return true; +} + // shortExecID renders the first 8 chars of a UUID so inline lineage // links ("Retried as #abc12345") stay readable in the table cell. The // full ID is preserved in the data-history-status attribute so the @@ -615,7 +648,9 @@ function sameRowActions(btn: HTMLButtonElement): HTMLButtonElement[] { const cell = btn.closest('td') || btn.parentElement; if (!cell) return [btn]; return Array.from( - cell.querySelectorAll('.history-approve-btn, .history-cancel-btn, .history-revoke-btn'), + cell.querySelectorAll( + '.history-approve-btn, .history-cancel-btn, .history-revoke-btn, .history-marketplace-sell-btn, .history-marketplace-cancel-btn', + ), ); } @@ -705,6 +740,18 @@ function renderActionCell(p: HistoryPurchase): string { return ``; } + // Completed Standard RI rows (AWS): offer Sell on Marketplace (issue + // #292). Placed after the revoke check; the two are mutually exclusive + // in practice (revoke is Azure-only, marketplace is AWS Standard-RI-only). + if (p.purchase_id) { + if (canCancelMarketplaceListing(p)) { + return ``; + } + if (canSellOnMarketplace(p)) { + return ``; + } + } + return escapeHtml(p.plan_name || '-'); } @@ -1068,6 +1115,70 @@ function wireRowActionHandlers(container: HTMLElement): void { } }); }); + + // Wire Sell on Marketplace button (issue #292) + container.querySelectorAll('.history-marketplace-sell-btn[data-marketplace-sell-id]').forEach(btn => { + btn.addEventListener('click', async () => { + const id = btn.dataset['marketplaceSellId']; + if (!id) return; + const ok = await confirmDialog({ + title: 'List this RI on the AWS Marketplace?', + body: 'This will create a binding listing on the AWS Marketplace. AWS charges a 12% transaction fee on proceeds. Make sure your AWS account has a US bank account on file as a Marketplace seller. This action cannot be undone without cancelling the listing.', + confirmLabel: 'Sell on Marketplace', + destructive: false, + }); + if (!ok) return; + const rowActions = sameRowActions(btn); + rowActions.forEach(b => { b.disabled = true; }); + try { + await api.createMarketplaceListing(id); + } catch (sellError) { + console.error('Failed to list RI on Marketplace:', sellError); + const err = sellError as Error; + showToast({ message: `Failed to list on Marketplace: ${err.message || 'unknown error'}`, kind: 'error' }); + rowActions.forEach(b => { b.disabled = false; }); + return; + } + showToast({ message: 'RI listed on Marketplace successfully', kind: 'success', timeout: 5_000 }); + try { + await loadHistory(); + } catch (reloadError) { + console.error('Failed to reload history after Marketplace listing:', reloadError); + } + }); + }); + + // Wire Cancel listing button (issue #292) + container.querySelectorAll('.history-marketplace-cancel-btn[data-marketplace-cancel-id]').forEach(btn => { + btn.addEventListener('click', async () => { + const id = btn.dataset['marketplaceCancelId']; + if (!id) return; + const ok = await confirmDialog({ + title: 'Cancel this Marketplace listing?', + body: 'This will remove the listing from the AWS Marketplace. Any existing buyer negotiations will be cancelled. You can relist the RI at any time.', + confirmLabel: 'Cancel listing', + destructive: true, + }); + if (!ok) return; + const rowActions = sameRowActions(btn); + rowActions.forEach(b => { b.disabled = true; }); + try { + await api.cancelMarketplaceListing(id); + } catch (cancelError) { + console.error('Failed to cancel Marketplace listing:', cancelError); + const err = cancelError as Error; + showToast({ message: `Failed to cancel listing: ${err.message || 'unknown error'}`, kind: 'error' }); + rowActions.forEach(b => { b.disabled = false; }); + return; + } + showToast({ message: 'Marketplace listing cancelled', kind: 'success', timeout: 5_000 }); + try { + await loadHistory(); + } catch (reloadError) { + console.error('Failed to reload history after Marketplace cancel:', reloadError); + } + }); + }); } // isPendingRow returns true when a history row represents a purchase diff --git a/frontend/src/types.ts b/frontend/src/types.ts index add694e1f..aaa2e6a05 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -282,6 +282,18 @@ export interface HistoryPurchase { revoked_at?: string; // revoked_via: "direct-api" or "support-case". Absent unless revoked. revoked_via?: string; + + // OfferingClass is "standard" or "convertible" for EC2 RIs. Absent + // on non-EC2 rows and on rows written before migration 000060. + // The "Sell on Marketplace" button only renders when this equals + // "standard" (issue #292). + offering_class?: string; + // ListingID is the AWS ReservedInstancesListingId. Non-empty when an + // active or recently-closed marketplace listing exists (issue #292). + listing_id?: string; + // ListingState is the AWS marketplace listing state: "active", + // "cancelled", or "closed". Empty when not listed. + listing_state?: string; } // Savings Analytics types From 38266176a2eb66bb574142882e5bc98ef3a9c07b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:06:40 +0200 Subject: [PATCH 03/21] fix(history): address CR wave4 frontend marketplace findings - canSellOnMarketplace: gate on remaining term >= 1 month computed from purchase timestamp + total term; matured Standard RIs no longer show Sell - sell click handler: open pricing/schedule modal before calling createMarketplaceListing so users see RI summary, default list price, and 12% fee breakdown before confirming - lineage actions cell: build trailingActions[] first then combine with lineage[] so Sell/Cancel buttons render on retry-descendant rows --- frontend/src/history.ts | 105 ++++++++++++++++++++++++++++++++-------- 1 file changed, 84 insertions(+), 21 deletions(-) diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 4b659b830..3010e9064 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -614,6 +614,16 @@ function canSellOnMarketplace(p: HistoryPurchase): boolean { if ((p.listing_state || '').toLowerCase() === 'active') return false; const user = getCurrentUser(); if (!user) return false; + // Guard against listing a matured RI: compute remaining months from the + // purchase timestamp and the total term. term is in months; timestamp is + // the purchase date. We require at least 1 full month remaining. + const termMonths = typeof p.term === 'number' ? p.term : Number(p.term) || 0; + if (termMonths <= 0) return false; + const purchaseMs = new Date(p.timestamp).getTime(); + if (!Number.isFinite(purchaseMs)) return false; + const elapsedMonths = (Date.now() - purchaseMs) / (1000 * 60 * 60 * 24 * 30.4375); + const remainingMonths = termMonths - elapsedMonths; + if (remainingMonths < 1) return false; return true; } @@ -729,29 +739,31 @@ function renderActionCell(p: HistoryPurchase): string { // see "this is a retry" provenance. lineage.push(`↻ Retry #${p.retry_attempt_n}`); } - if (lineage.length > 0) { - return lineage.join(' '); - } - - // Completed Azure row within revocation window: show Revoke button - // (issue #290). Only Azure supports direct in-app revocation; AWS and - // GCP have no cancel API so the button is suppressed for those providers. - if (canRevokeCompletedRow(p) && p.purchase_id) { - return ``; - } - - // Completed Standard RI rows (AWS): offer Sell on Marketplace (issue - // #292). Placed after the revoke check; the two are mutually exclusive - // in practice (revoke is Azure-only, marketplace is AWS Standard-RI-only). + // Build the trailing action buttons so they compose with lineage links + // — a retry-descendant can also have an active listing that needs a + // Cancel button, or be an Azure row still inside its revoke window. + const trailingActions: string[] = []; if (p.purchase_id) { - if (canCancelMarketplaceListing(p)) { - return ``; + // Completed Azure row within revocation window: Revoke button (issue + // #290). Only Azure supports direct in-app revocation; AWS and GCP have + // no cancel API so the button is suppressed for those providers. + if (canRevokeCompletedRow(p)) { + trailingActions.push(``); } - if (canSellOnMarketplace(p)) { - return ``; + // Completed Standard RI rows (AWS): Cancel listing / Sell on Marketplace + // (issue #292). Mutually exclusive with revoke in practice (revoke is + // Azure-only, marketplace is AWS Standard-RI-only). + if (canCancelMarketplaceListing(p)) { + trailingActions.push(``); + } else if (canSellOnMarketplace(p)) { + trailingActions.push(``); } } + if (lineage.length > 0 || trailingActions.length > 0) { + return [...lineage, ...trailingActions].join(' '); + } + return escapeHtml(p.plan_name || '-'); } @@ -1116,18 +1128,69 @@ function wireRowActionHandlers(container: HTMLElement): void { }); }); - // Wire Sell on Marketplace button (issue #292) + // Wire Sell on Marketplace button (issue #292). + // Flow: pricing/schedule modal (RI summary + default price + 12% fee) → + // user confirms → createMarketplaceListing. We never skip the pricing + // modal (CR finding: going straight from confirmDialog to the API call + // denies the user informed consent about the price and fee). container.querySelectorAll('.history-marketplace-sell-btn[data-marketplace-sell-id]').forEach(btn => { btn.addEventListener('click', async () => { const id = btn.dataset['marketplaceSellId']; if (!id) return; + + // Look up the purchase record so we can show a meaningful price summary. + const purchase = lastPurchases.find(p => p.purchase_id === id); + + // Build a pricing modal body with RI summary and fee breakdown. + const bodyEl = document.createElement('div'); + bodyEl.className = 'marketplace-pricing-modal-body'; + + if (purchase) { + const termMonths = typeof purchase.term === 'number' ? purchase.term : Number(purchase.term) || 0; + const purchaseMs = new Date(purchase.timestamp).getTime(); + const elapsedMonths = Number.isFinite(purchaseMs) + ? (Date.now() - purchaseMs) / (1000 * 60 * 60 * 24 * 30.4375) + : 0; + const remainingMonths = Math.max(0, Math.round(termMonths - elapsedMonths)); + const upfront = purchase.upfront_cost ?? 0; + const monthly = purchase.monthly_cost ?? 0; + const totalValue = upfront + monthly * remainingMonths; + const listPrice = totalValue * 0.95; + const netProceeds = listPrice * 0.88; + + const summaryEl = document.createElement('dl'); + summaryEl.className = 'marketplace-pricing-summary'; + const addRow = (label: string, value: string): void => { + const dt = document.createElement('dt'); + dt.textContent = label; + const dd = document.createElement('dd'); + dd.textContent = value; + summaryEl.appendChild(dt); + summaryEl.appendChild(dd); + }; + addRow('RI ID', id); + addRow('Region', purchase.region || '-'); + addRow('Resource type', purchase.resource_type || '-'); + addRow('Remaining term', remainingMonths === 1 ? '1 month' : `${remainingMonths} months`); + addRow('Default list price', formatCurrency(listPrice)); + addRow('AWS fee (12%)', formatCurrency(listPrice * 0.12)); + addRow('Estimated net proceeds', formatCurrency(netProceeds)); + bodyEl.appendChild(summaryEl); + } + + const noteEl = document.createElement('p'); + noteEl.className = 'marketplace-pricing-note'; + noteEl.textContent = 'AWS charges a 12% transaction fee on proceeds. The default schedule prices the listing at 5% below remaining value. You can adjust pricing by contacting your administrator or modifying the schedule via the API. This action cannot be undone without cancelling the listing.'; + bodyEl.appendChild(noteEl); + const ok = await confirmDialog({ title: 'List this RI on the AWS Marketplace?', - body: 'This will create a binding listing on the AWS Marketplace. AWS charges a 12% transaction fee on proceeds. Make sure your AWS account has a US bank account on file as a Marketplace seller. This action cannot be undone without cancelling the listing.', - confirmLabel: 'Sell on Marketplace', + body: bodyEl, + confirmLabel: 'Confirm listing', destructive: false, }); if (!ok) return; + const rowActions = sameRowActions(btn); rowActions.forEach(b => { b.disabled = true; }); try { From df37173e865a0ef64ea5e80d29907ac2d600b298 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:06:56 +0200 Subject: [PATCH 04/21] fix(marketplace): address CR wave4 Go handler findings - Compute actual remaining months from purchase timestamp and total term via computeRemainingMonths; removes overpricing of older RIs in the default price schedule (was using full contract term) - Map AWS errors via mapAWSMarketplaceError: inspect smithy.APIError code and fault; client-fault errors return 4xx instead of blanket 502 - On DB failure after successful listing creation, attempt compensating rollback via CancelMarketplaceListing to prevent AWS/DB desync; return internal error rather than false success to the caller - Same DB-failure fix for cancel handler: return error on UpdatePurchaseHistoryListing failure so the caller knows the state is out of sync --- internal/api/handler_marketplace.go | 91 ++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 13 deletions(-) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index c3b51dca8..18b98fac1 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -15,8 +15,11 @@ package api import ( "context" "encoding/json" + "errors" "fmt" + "math" "strings" + "time" "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/pkg/logging" @@ -24,6 +27,7 @@ import ( ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" + smithy "github.com/aws/smithy-go" "github.com/google/uuid" ) @@ -113,6 +117,11 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio } } + // Compute actual remaining months from the purchase timestamp and total + // term so the default price schedule reflects real remaining value + // rather than the full contract term (which overprices older RIs). + remainingMonths := computeRemainingMonths(row.Timestamp, row.Term) + // Validate and normalise the price schedule. MonthlyCost is nullable // (nil means the provider returned no recurring breakdown); treat absent // as $0 recurring for the default price-schedule computation. @@ -120,7 +129,7 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio if row.MonthlyCost != nil { monthlyCost = *row.MonthlyCost } - schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, row.Term, row.UpfrontCost, monthlyCost) + schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, row.UpfrontCost, monthlyCost) if err != nil { return nil, NewClientError(400, err.Error()) } @@ -147,18 +156,21 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio PriceSchedule: awsSchedule, }) if err != nil { - // Preserve the AWS error message verbatim for 4xx errors (e.g. missing - // seller account, invalid listing) so the frontend can surface it. logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) - return nil, NewClientError(502, "AWS marketplace listing failed: "+err.Error()) + return nil, mapAWSMarketplaceError("AWS marketplace listing failed", err) } - // Persist the listing ID and state. + // Persist the listing ID and state. On DB failure, attempt a compensating + // rollback (cancel the just-created listing) to avoid a desync where the + // user sees success but the listing is invisible in subsequent renders. if dbErr := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, result.ListingID, result.State); dbErr != nil { - // Log the error but return success to the caller — the listing was - // created in AWS; a DB write failure here must not be surfaced as a - // 5xx that could cause the user to create a duplicate listing. - logging.Errorf("marketplace: listing created (%s / %s) but DB update failed: %v", result.ListingID, result.State, dbErr) + logging.Errorf("marketplace: listing created (%s / %s) but DB update failed: %v — attempting rollback", result.ListingID, result.State, dbErr) + if _, rollbackErr := ec2Client.CancelMarketplaceListing(ctx, result.ListingID); rollbackErr != nil { + logging.Errorf("marketplace: rollback cancel for listing %s also failed: %v", result.ListingID, rollbackErr) + } else { + logging.Warnf("marketplace: listing %s rolled back (cancelled) after DB failure", result.ListingID) + } + return nil, fmt.Errorf("listing created but could not be persisted; listing has been rolled back: %w", dbErr) } return &MarketplaceListResponse{ @@ -207,11 +219,15 @@ func (h *Handler) marketplaceCancel(ctx context.Context, req *events.LambdaFunct result, err := ec2Client.CancelMarketplaceListing(ctx, row.ListingID) if err != nil { logging.Warnf("marketplace: CancelReservedInstancesListing for listing %s failed: %v", row.ListingID, err) - return nil, NewClientError(502, "AWS cancel listing failed: "+err.Error()) + return nil, mapAWSMarketplaceError("AWS cancel listing failed", err) } + // The listing is already cancelled in AWS; there is no compensating rollback + // available if the DB write fails. Return an internal error so the caller + // knows the state is out of sync and can retry or contact an administrator. if dbErr := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, result.ListingID, result.State); dbErr != nil { - logging.Errorf("marketplace: listing cancelled (%s) but DB update failed: %v", result.ListingID, dbErr) + logging.Errorf("marketplace: listing cancelled in AWS (%s) but DB update failed: %v — state is out of sync", result.ListingID, dbErr) + return nil, fmt.Errorf("listing cancelled in AWS but could not be persisted: %w", dbErr) } return map[string]string{"listing_id": result.ListingID, "listing_state": result.State}, nil @@ -296,14 +312,63 @@ func (h *Handler) authorizeAllowedAccount(ctx context.Context, session *Session, return NewClientError(403, "permission denied: purchase is in a cloud account not covered by your session's allowed accounts") } +// computeRemainingMonths returns the number of whole months remaining on an RI +// given its purchase timestamp and total term in months. The result is floored +// at 1 so defensive callers always get a positive value. +func computeRemainingMonths(purchaseTime time.Time, termMonths int) int { + if purchaseTime.IsZero() || termMonths <= 0 { + return 1 + } + elapsed := time.Since(purchaseTime) + elapsedMonths := elapsed.Hours() / (24 * 30.4375) + remaining := float64(termMonths) - elapsedMonths + r := int(math.Floor(remaining)) + if r < 1 { + return 1 + } + return r +} + +// awsMarketplaceClientFaultCodes is the set of AWS error codes that represent +// client-side faults for Marketplace listing operations. These map to 4xx +// responses so the caller receives an actionable message. Server-side AWS +// errors remain 5xx. +var awsMarketplaceClientFaultCodes = map[string]bool{ + "InvalidReservedInstancesId": true, + "InvalidReservedInstancesId.NotFound": true, + "InvalidParameterValue": true, + "InvalidParameter": true, + "IncorrectState": true, + "InvalidReservedInstancesListingId": true, + "ReservedInstancesListingAlreadyExists": true, + "SellerNotRegistered": true, + "AuthFailure": true, + "UnauthorizedOperation": true, +} + +// 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. All other errors +// produce a 502 (AWS-side failure). +func mapAWSMarketplaceError(opMsg string, err error) error { + var apiErr smithy.APIError + if errors.As(err, &apiErr) { + if awsMarketplaceClientFaultCodes[apiErr.ErrorCode()] || apiErr.ErrorFault() == smithy.FaultClient { + return NewClientError(400, apiErr.ErrorMessage()) + } + } + return NewClientError(502, opMsg+": "+err.Error()) +} + // resolveMarketplacePriceSchedule returns a normalised price schedule for the // given RI. When the caller supplied an explicit schedule it is validated and // returned unchanged. When the caller omitted the schedule (nil / empty), a // single-tier default is computed: total remaining value * 0.95 (5% discount // to attract buyers; the 12% AWS fee is applied by the Marketplace on top). // -// remainingMonths is rec.Term (AWS RI term in months), upfrontCost is the -// original upfront paid, monthlyCost is the ongoing recurring charge. +// remainingMonths must be the actual remaining months (computed via +// computeRemainingMonths from purchase timestamp and total term — NOT the raw +// term field, which would overprice older RIs). func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths int, upfrontCost, monthlyCost float64) ([]MarketplacePriceTier, error) { if len(supplied) > 0 { for i, t := range supplied { From 4e34872359b04f91b5e81c90280cca086bbf6ea2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:07:07 +0200 Subject: [PATCH 05/21] fix(migrations): add settlement columns to 000060 marketplace migration Add listed_at, listing_price_schedule, listing_proceeds_received, and listing_fee_paid columns for marketplace settlement tracking. All nullable, consistent with existing offering_class/listing_id/listing_state nullability. Down migration updated to drop the new columns. --- ...00060_purchase_history_marketplace_listing.down.sql | 6 +++++- .../000060_purchase_history_marketplace_listing.up.sql | 10 +++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql index dda98d245..20a53aa56 100644 --- a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql +++ b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql @@ -2,4 +2,8 @@ ALTER TABLE purchase_history DROP COLUMN IF EXISTS offering_class, DROP COLUMN IF EXISTS listing_id, - DROP COLUMN IF EXISTS listing_state; + DROP COLUMN IF EXISTS listing_state, + DROP COLUMN IF EXISTS listed_at, + DROP COLUMN IF EXISTS listing_price_schedule, + DROP COLUMN IF EXISTS listing_proceeds_received, + DROP COLUMN IF EXISTS listing_fee_paid; diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql index 673749cb7..9b81ee49a 100644 --- a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql +++ b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql @@ -13,6 +13,10 @@ -- migration is idempotent on re-apply. ALTER TABLE purchase_history - ADD COLUMN IF NOT EXISTS offering_class TEXT, - ADD COLUMN IF NOT EXISTS listing_id TEXT, - ADD COLUMN IF NOT EXISTS listing_state TEXT; + ADD COLUMN IF NOT EXISTS offering_class TEXT, + ADD COLUMN IF NOT EXISTS listing_id TEXT, + ADD COLUMN IF NOT EXISTS listing_state TEXT, + ADD COLUMN IF NOT EXISTS listed_at TIMESTAMP WITH TIME ZONE, + ADD COLUMN IF NOT EXISTS listing_price_schedule JSONB, + ADD COLUMN IF NOT EXISTS listing_proceeds_received NUMERIC, + ADD COLUMN IF NOT EXISTS listing_fee_paid NUMERIC; From 31972f8a7208deb2b1e48ecb9a20a0fa5e07329b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:07:19 +0200 Subject: [PATCH 06/21] fix(ec2): validate non-empty listing ID from CreateReservedInstancesListing Return an error when AWS returns a listing with an empty ReservedInstancesListingId rather than propagating a blank ID that would silently make rollback and describe calls fail. --- providers/aws/services/ec2/client.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 983deeab8..8c1694b83 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -1006,9 +1006,9 @@ func (c *Client) CreateMarketplaceListing(ctx context.Context, req MarketplaceLi if listing.Status != "" { state = string(listing.Status) } - listingID := "" - if listing.ReservedInstancesListingId != nil { - listingID = aws.ToString(listing.ReservedInstancesListingId) + listingID := aws.ToString(listing.ReservedInstancesListingId) + if listingID == "" { + return MarketplaceListingResult{}, fmt.Errorf("CreateReservedInstancesListing returned a listing with an empty ID") } return MarketplaceListingResult{ListingID: listingID, State: state}, nil } From 31e14a213c517af1cdb77268da4080bf41a9c929 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 19:27:37 +0200 Subject: [PATCH 07/21] fix(pr-808): compile + CR findings + pre-commit Repair the stalled marketplace sell/cancel work: - Add the three missing marketplace methods to MockEC2Client so the ec2 package compiles against the extended EC2API interface (CreateReservedInstancesListing, DescribeReservedInstancesListings, CancelReservedInstancesListing). - Extract validateMarketplaceListRequest from marketplaceList to bring its cyclomatic complexity back under the gocyclo threshold. - Regenerate frontend/src/permissions.generated.ts to include the new sell-own:purchases permission (pre-commit codegen check). - Harden Describe/Cancel marketplace paths to fall back to the caller-supplied listing ID when AWS omits ReservedInstancesListingId, so downstream persistence never stores an empty ID (CR finding). --- frontend/src/permissions.generated.ts | 1 + internal/api/handler_marketplace.go | 50 +++++++++++++++-------- providers/aws/services/ec2/client.go | 12 +++--- providers/aws/services/ec2/client_test.go | 24 +++++++++++ 4 files changed, 64 insertions(+), 23 deletions(-) diff --git a/frontend/src/permissions.generated.ts b/frontend/src/permissions.generated.ts index 71105363d..142989e97 100644 --- a/frontend/src/permissions.generated.ts +++ b/frontend/src/permissions.generated.ts @@ -25,6 +25,7 @@ export const USER_PERMS: ReadonlySet = new Set([ 'delete:plans', 'retry-own:purchases', 'revoke-own:purchases', + 'sell-own:purchases', 'update:plans', 'update:purchases', 'view:history', diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 18b98fac1..47865e0e9 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -22,6 +22,7 @@ import ( "time" "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/LeanerCloud/CUDly/internal/config" "github.com/LeanerCloud/CUDly/pkg/logging" awsprovider "github.com/LeanerCloud/CUDly/providers/aws" ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" @@ -66,57 +67,72 @@ type MarketplaceListRequest struct { // MarketplaceListResponse is the JSON response body for a successful listing. type MarketplaceListResponse struct { - ListingID string `json:"listing_id"` - ListingState string `json:"listing_state"` - PriceSchedule []MarketplacePriceTier `json:"price_schedule"` - AWSFeePercent float64 `json:"aws_fee_percent"` - Note string `json:"note,omitempty"` + ListingID string `json:"listing_id"` + ListingState string `json:"listing_state"` + PriceSchedule []MarketplacePriceTier `json:"price_schedule"` + AWSFeePercent float64 `json:"aws_fee_percent"` + Note string `json:"note,omitempty"` } -// marketplaceList handles POST /api/purchases/{id}/marketplace-list. -// The {id} must be the purchase_history.purchase_id (AWS ReservedInstancesId). -func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctionURLRequest, purchaseID string) (any, error) { +// validateMarketplaceListRequest performs the pre-flight checks shared by the +// listing flow: session auth, UUID validation, row lookup, offering_class / +// RBAC / duplicate-listing gates, and optional body decode. It returns the +// validated history row and the decoded request body. Extracted from +// marketplaceList to keep that handler's cyclomatic complexity in check. +func (h *Handler) validateMarketplaceListRequest(ctx context.Context, req *events.LambdaFunctionURLRequest, purchaseID string) (*config.PurchaseHistoryRecord, MarketplaceListRequest, error) { + var body MarketplaceListRequest + session, err := h.requireSession(ctx, req) if err != nil { - return nil, err + return nil, body, err } if err := validateUUID(purchaseID); err != nil { - return nil, err + return nil, body, err } // Look up the purchase_history row to validate offering_class + get metadata. row, err := h.config.GetPurchaseHistoryByPurchaseID(ctx, purchaseID) if err != nil { - return nil, fmt.Errorf("failed to look up purchase: %w", err) + return nil, body, fmt.Errorf("failed to look up purchase: %w", err) } if row == nil { - return nil, NewClientError(404, "purchase not found") + return nil, body, NewClientError(404, "purchase not found") } // Only Standard RIs can be listed on the Marketplace. if !strings.EqualFold(row.OfferingClass, "standard") { - return nil, NewClientError(400, "only Standard Reserved Instances can be listed on the AWS Marketplace; this purchase has offering_class="+row.OfferingClass) + return nil, body, NewClientError(400, "only Standard Reserved Instances can be listed on the AWS Marketplace; this purchase has offering_class="+row.OfferingClass) } // Enforce sell-any / sell-own RBAC. if err := h.authorizeSessionSell(ctx, session, row.CloudAccountID); err != nil { - return nil, err + return nil, body, err } // Reject if a listing is already active to avoid duplicate listings. if strings.EqualFold(row.ListingState, "active") { - return nil, NewClientError(409, fmt.Sprintf("an active marketplace listing %s already exists for this RI; cancel it first", row.ListingID)) + return nil, body, NewClientError(409, fmt.Sprintf("an active marketplace listing %s already exists for this RI; cancel it first", row.ListingID)) } // Decode optional body. - var body MarketplaceListRequest if len(req.Body) > 0 { if err := json.Unmarshal([]byte(req.Body), &body); err != nil { - return nil, NewClientError(400, "invalid request body: "+err.Error()) + return nil, body, NewClientError(400, "invalid request body: "+err.Error()) } } + return row, body, nil +} + +// marketplaceList handles POST /api/purchases/{id}/marketplace-list. +// The {id} must be the purchase_history.purchase_id (AWS ReservedInstancesId). +func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctionURLRequest, purchaseID string) (any, error) { + row, body, err := h.validateMarketplaceListRequest(ctx, req, purchaseID) + if err != nil { + return nil, err + } + // Compute actual remaining months from the purchase timestamp and total // term so the default price schedule reflects real remaining value // rather than the full contract term (which overprices older RIs). diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 8c1694b83..229aea975 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -1027,9 +1027,9 @@ func (c *Client) DescribeMarketplaceListing(ctx context.Context, listingID strin } listing := out.ReservedInstancesListings[0] state := string(listing.Status) - resolvedID := "" - if listing.ReservedInstancesListingId != nil { - resolvedID = aws.ToString(listing.ReservedInstancesListingId) + resolvedID := aws.ToString(listing.ReservedInstancesListingId) + if resolvedID == "" { + resolvedID = listingID } return MarketplaceListingResult{ListingID: resolvedID, State: state}, nil } @@ -1048,9 +1048,9 @@ func (c *Client) CancelMarketplaceListing(ctx context.Context, listingID string) } listing := out.ReservedInstancesListings[0] state := string(listing.Status) - resolvedID := "" - if listing.ReservedInstancesListingId != nil { - resolvedID = aws.ToString(listing.ReservedInstancesListingId) + resolvedID := aws.ToString(listing.ReservedInstancesListingId) + if resolvedID == "" { + resolvedID = listingID } return MarketplaceListingResult{ListingID: resolvedID, State: state}, nil } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 14c3c893f..5f783e775 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -78,6 +78,30 @@ func (m *MockEC2Client) CreateTags(ctx context.Context, params *ec2.CreateTagsIn return args.Get(0).(*ec2.CreateTagsOutput), args.Error(1) } +func (m *MockEC2Client) CreateReservedInstancesListing(ctx context.Context, params *ec2.CreateReservedInstancesListingInput, optFns ...func(*ec2.Options)) (*ec2.CreateReservedInstancesListingOutput, error) { + args := m.Called(ctx, params) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*ec2.CreateReservedInstancesListingOutput), args.Error(1) +} + +func (m *MockEC2Client) DescribeReservedInstancesListings(ctx context.Context, params *ec2.DescribeReservedInstancesListingsInput, optFns ...func(*ec2.Options)) (*ec2.DescribeReservedInstancesListingsOutput, error) { + args := m.Called(ctx, params) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*ec2.DescribeReservedInstancesListingsOutput), args.Error(1) +} + +func (m *MockEC2Client) CancelReservedInstancesListing(ctx context.Context, params *ec2.CancelReservedInstancesListingInput, optFns ...func(*ec2.Options)) (*ec2.CancelReservedInstancesListingOutput, error) { + args := m.Called(ctx, params) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*ec2.CancelReservedInstancesListingOutput), args.Error(1) +} + func TestNewClient(t *testing.T) { t.Parallel() cfg := aws.Config{ From a8f542f9c16493909e29e2b649005b4a19cfc48a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 00:36:46 +0200 Subject: [PATCH 08/21] fix(marketplace): adapt to nullable PurchaseHistoryRecord.MonthlyCost After rebasing onto feat/multicloud-web-frontend, base PR #258 made PurchaseHistoryRecord.MonthlyCost a *float64 (nullable). The marketplace default price-schedule path passed it as a float64. Dereference it nil-safely at the call site, treating an absent monthly breakdown as a zero recurring contribution to residual value (per the nullable-not-zero convention: nil means "no breakdown recorded", which contributes nothing to the residual list price). --- internal/api/handler_marketplace.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 47865e0e9..c760af3d2 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -138,10 +138,10 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio // rather than the full contract term (which overprices older RIs). remainingMonths := computeRemainingMonths(row.Timestamp, row.Term) - // Validate and normalise the price schedule. MonthlyCost is nullable - // (nil means the provider returned no recurring breakdown); treat absent - // as $0 recurring for the default price-schedule computation. - var monthlyCost float64 + // Validate and normalise the price schedule. A nil MonthlyCost means the + // provider recorded no recurring breakdown (issue #258 made the field + // nullable); treat absent as zero recurring contribution to residual value. + monthlyCost := 0.0 if row.MonthlyCost != nil { monthlyCost = *row.MonthlyCost } From 6a17fb1bc74398464e39449dcc3cbe88d47e4aae Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 13:54:18 +0200 Subject: [PATCH 09/21] refactor(config): extract scanPurchaseHistoryRow to fix gocyclo budget queryPurchaseHistory reached cyclomatic complexity 11 (budget 10) after the nullable marketplace columns (offering_class, listing_id, listing_state) and the nullable monthly_cost handling were added to the per-row scan. Pull the scan plus nullable-column reconciliation into scanPurchaseHistoryRow so the parent function drops back under the limit. Behavior is unchanged. --- internal/config/store_postgres.go | 149 ++++++++++++++++-------------- 1 file changed, 81 insertions(+), 68 deletions(-) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index e6cc55752..e1f0e1fa0 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1924,81 +1924,94 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, records := make([]PurchaseHistoryRecord, 0) for rows.Next() { - var record PurchaseHistoryRecord - var planID, planName, cloudAccountID, offeringClass, listingID, listingState sql.NullString - var monthlyCost sql.NullFloat64 - var revocationWindowClosesAt, revokedAt *time.Time - var revokedVia, supportCaseID sql.NullString - - err := rows.Scan( - &record.AccountID, - &record.PurchaseID, - &record.Timestamp, - &record.Provider, - &record.Service, - &record.Region, - &record.ResourceType, - &record.Count, - &record.Term, - &record.Payment, - &record.UpfrontCost, - &monthlyCost, - &record.EstimatedSavings, - &planID, - &planName, - &record.RampStep, - &cloudAccountID, - &revocationWindowClosesAt, - &revokedAt, - &revokedVia, - &supportCaseID, - &offeringClass, - &listingID, - &listingState, - ) + record, err := scanPurchaseHistoryRow(rows) if err != nil { - return nil, fmt.Errorf("failed to scan purchase history: %w", err) + return nil, err } + records = append(records, record) + } - // Handle nullable monthly_cost: nil means "provider did not return a - // monthly breakdown"; 0.0 means "explicitly $0 recurring charge". - if monthlyCost.Valid { - v := monthlyCost.Float64 - record.MonthlyCost = &v - } + return records, rows.Err() +} - // Handle nullable strings - if planID.Valid { - record.PlanID = planID.String - } - if planName.Valid { - record.PlanName = planName.String - } - if cloudAccountID.Valid { - record.CloudAccountID = &cloudAccountID.String - } - record.RevocationWindowClosesAt = revocationWindowClosesAt - record.RevokedAt = revokedAt - if revokedVia.Valid { - record.RevokedVia = revokedVia.String - } - if supportCaseID.Valid { - record.SupportCaseID = supportCaseID.String - } - if offeringClass.Valid { - record.OfferingClass = offeringClass.String - } - if listingID.Valid { - record.ListingID = listingID.String - } - if listingState.Valid { - record.ListingState = listingState.String - } +// scanPurchaseHistoryRow scans a single purchase_history row and reconciles the +// nullable columns into a PurchaseHistoryRecord. It is pulled out of +// queryPurchaseHistory to keep that function under the cyclomatic limit. The +// column order must match the SELECT lists in GetPurchaseHistory / +// GetAllPurchaseHistory / GetPurchaseHistoryFiltered (base columns, then the +// revocation columns from issue #290, then the marketplace columns from #292). +func scanPurchaseHistoryRow(rows pgx.Rows) (PurchaseHistoryRecord, error) { + var record PurchaseHistoryRecord + var planID, planName, cloudAccountID, offeringClass, listingID, listingState sql.NullString + var monthlyCost sql.NullFloat64 + var revocationWindowClosesAt, revokedAt *time.Time + var revokedVia, supportCaseID sql.NullString - records = append(records, record) + if err := rows.Scan( + &record.AccountID, + &record.PurchaseID, + &record.Timestamp, + &record.Provider, + &record.Service, + &record.Region, + &record.ResourceType, + &record.Count, + &record.Term, + &record.Payment, + &record.UpfrontCost, + &monthlyCost, + &record.EstimatedSavings, + &planID, + &planName, + &record.RampStep, + &cloudAccountID, + &revocationWindowClosesAt, + &revokedAt, + &revokedVia, + &supportCaseID, + &offeringClass, + &listingID, + &listingState, + ); err != nil { + return PurchaseHistoryRecord{}, fmt.Errorf("failed to scan purchase history: %w", err) } - return records, rows.Err() + // Handle nullable monthly_cost: nil means "provider did not return a + // monthly breakdown"; 0.0 means "explicitly $0 recurring charge". + if monthlyCost.Valid { + v := monthlyCost.Float64 + record.MonthlyCost = &v + } + + // Handle nullable strings + if planID.Valid { + record.PlanID = planID.String + } + if planName.Valid { + record.PlanName = planName.String + } + if cloudAccountID.Valid { + record.CloudAccountID = &cloudAccountID.String + } + record.RevocationWindowClosesAt = revocationWindowClosesAt + record.RevokedAt = revokedAt + if revokedVia.Valid { + record.RevokedVia = revokedVia.String + } + if supportCaseID.Valid { + record.SupportCaseID = supportCaseID.String + } + if offeringClass.Valid { + record.OfferingClass = offeringClass.String + } + if listingID.Valid { + record.ListingID = listingID.String + } + if listingState.Valid { + record.ListingState = listingState.String + } + + return record, nil } // GetPurchaseHistoryByPurchaseID returns the single purchase_history row From a3a94fcefcaf9bc62cf5d77b362c1ea01bbb91fd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 15:56:58 +0200 Subject: [PATCH 10/21] fix(api/marketplace): prorate upfront cost to remaining term in default schedule When the caller omits a price schedule, the default was computing total value as (full_upfront + recurring * remaining_months). For an older RI this overprices the upfront component because only (remaining/original) of the upfront has not yet been amortized. Change to: upfront_remaining = upfront_cost * (remaining / original_term) total_value = upfront_remaining + recurring * remaining_months Pass row.Term as originalTerm through resolveMarketplacePriceSchedule. Addresses CR finding on handler_marketplace.go. --- internal/api/handler_marketplace.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index c760af3d2..0d7263c57 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -145,7 +145,7 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio if row.MonthlyCost != nil { monthlyCost = *row.MonthlyCost } - schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, row.UpfrontCost, monthlyCost) + schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, row.Term, row.UpfrontCost, monthlyCost) if err != nil { return nil, NewClientError(400, err.Error()) } @@ -379,13 +379,16 @@ func mapAWSMarketplaceError(opMsg string, err error) error { // resolveMarketplacePriceSchedule returns a normalised price schedule for the // given RI. When the caller supplied an explicit schedule it is validated and // returned unchanged. When the caller omitted the schedule (nil / empty), a -// single-tier default is computed: total remaining value * 0.95 (5% discount -// to attract buyers; the 12% AWS fee is applied by the Marketplace on top). +// single-tier default is computed: (upfront_remaining + future_recurring) * +// 0.95 (5% discount to attract buyers; the 12% AWS fee is applied by the +// Marketplace on top). // // remainingMonths must be the actual remaining months (computed via -// computeRemainingMonths from purchase timestamp and total term — NOT the raw +// computeRemainingMonths from purchase timestamp and total term -- NOT the raw // term field, which would overprice older RIs). -func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths int, upfrontCost, monthlyCost float64) ([]MarketplacePriceTier, error) { +// originalTerm is the full contract term in months, used to prorate the +// upfront cost to its remaining value. +func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths, originalTerm int, upfrontCost, monthlyCost float64) ([]MarketplacePriceTier, error) { if len(supplied) > 0 { for i, t := range supplied { if t.TermMonths <= 0 { @@ -398,11 +401,18 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM return supplied, nil } - // Default: spread (upfront + recurring) * 0.95 across remaining term. + // Default: spread (upfront_remaining + future_recurring) * 0.95 across + // remaining term. The upfront cost is prorated by (remaining/original) to + // avoid overpricing older RIs (a 12-month RI at month 6 retains only half + // the upfront value; using the full amount would overprice by ~2x). if remainingMonths <= 0 { remainingMonths = 1 // defensive: should not happen for an active RI } - totalValue := upfrontCost + (monthlyCost * float64(remainingMonths)) + upfrontRemaining := 0.0 + if originalTerm > 0 { + upfrontRemaining = upfrontCost * (float64(remainingMonths) / float64(originalTerm)) + } + totalValue := upfrontRemaining + (monthlyCost * float64(remainingMonths)) listPrice := totalValue * 0.95 if listPrice < 0 { listPrice = 0 From 9416b5e184813a677a992ce148c88e617098195f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 11:10:21 +0200 Subject: [PATCH 11/21] fix(marketplace): multi-count listing, handler tests, descope poller columns (closes #292 partial) Address adversarial-review gaps on the RI Marketplace sell/cancel PR: - Multi-count fix: CreateMarketplaceListing now lists every RI in the row (purchase_history.count) instead of a hardcoded InstanceCount=1, so a row of N Standard RIs lists all N. The EC2 client rejects a non-positive count before the outbound call; the handler passes row.Count (floored at 1 for legacy rows). Adds EC2 client tests for create (multi-count + guards), describe, and cancel via the SDK mock. - Tests: add handler_marketplace_test.go covering convertible -> 400, sell-own allow + deny (account scoping), duplicate active listing -> 409, AWS error mapping (client fault -> 400, server/unknown -> 502), DB-failure compensating rollback, and default-schedule proration math. - Descope poller: migration 000060 keeps only the columns the implemented flow reads/writes (offering_class, listing_id, listing_state). The settlement/poller columns (listed_at, listing_price_schedule, listing_proceeds_received, listing_fee_paid) are removed so the schema never carries columns nothing populates; they land with the poller in the #292 follow-up (#966). - Repair a stale Session.Role="admin" reference in a pre-existing purchases test (removed in the group-only auth migration) so the api test package compiles after rebasing onto the current base. --- internal/api/handler_marketplace.go | 9 + internal/api/handler_marketplace_test.go | 373 ++++++++++++++++++ ...chase_history_marketplace_listing.down.sql | 6 +- ...urchase_history_marketplace_listing.up.sql | 17 +- providers/aws/services/ec2/client.go | 10 +- providers/aws/services/ec2/client_test.go | 163 ++++++++ 6 files changed, 565 insertions(+), 13 deletions(-) create mode 100644 internal/api/handler_marketplace_test.go diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 0d7263c57..9530ae994 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -166,10 +166,19 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio }) } + // List every RI in the row: a row of N Standard RIs must list all N, not a + // single unit (issue #292 multi-count fix). Floor at 1 for legacy rows that + // somehow recorded a non-positive count so a valid Standard RI still lists. + instanceCount := int32(row.Count) + if instanceCount < 1 { + instanceCount = 1 + } + result, err := ec2Client.CreateMarketplaceListing(ctx, ec2svc.MarketplaceListingRequest{ ReservedInstancesID: purchaseID, ClientToken: uuid.New().String(), PriceSchedule: awsSchedule, + InstanceCount: instanceCount, }) if err != nil { logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go new file mode 100644 index 000000000..7d0d52ef4 --- /dev/null +++ b/internal/api/handler_marketplace_test.go @@ -0,0 +1,373 @@ +package api + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/LeanerCloud/CUDly/internal/config" + ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" + "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/aws" + smithy "github.com/aws/smithy-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// validMarketplacePurchaseID is a syntactically valid UUID accepted by +// validateUUID; the marketplace path keys on purchase_history.purchase_id. +const validMarketplacePurchaseID = "11111111-1111-1111-1111-111111111111" + +// stubMarketplaceEC2 is a configurable stub implementing marketplaceEC2Client. +// Each field, when set, overrides the corresponding method's behaviour and +// records the last request so tests can assert on what was sent to AWS. +type stubMarketplaceEC2 struct { + createFn func(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) + cancelFn func(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) + + lastCreateReq ec2svc.MarketplaceListingRequest + cancelCallCount int + lastCancelID string +} + +func (s *stubMarketplaceEC2) CreateMarketplaceListing(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { + s.lastCreateReq = req + if s.createFn != nil { + return s.createFn(ctx, req) + } + return ec2svc.MarketplaceListingResult{ListingID: "ril-default", State: "active"}, nil +} + +func (s *stubMarketplaceEC2) DescribeMarketplaceListing(_ context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) { + return ec2svc.MarketplaceListingResult{ListingID: listingID, State: "active"}, nil +} + +func (s *stubMarketplaceEC2) CancelMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) { + s.cancelCallCount++ + s.lastCancelID = listingID + if s.cancelFn != nil { + return s.cancelFn(ctx, listingID) + } + return ec2svc.MarketplaceListingResult{ListingID: listingID, State: "cancelled"}, nil +} + +// marketplaceTestAPIError is an smithy.APIError with a controllable fault so +// the error-mapping tests can exercise both the client- and server-fault paths. +type marketplaceTestAPIError struct { + code string + message string + fault smithy.ErrorFault +} + +func (e *marketplaceTestAPIError) Error() string { return e.code + ": " + e.message } +func (e *marketplaceTestAPIError) ErrorCode() string { return e.code } +func (e *marketplaceTestAPIError) ErrorMessage() string { return e.message } +func (e *marketplaceTestAPIError) ErrorFault() smithy.ErrorFault { return e.fault } + +// newMarketplaceHandler wires a Handler with the supplied mocks and stub EC2 +// client, pre-seeding the cached AWS config so loadAWSConfigWithRegion does not +// hit the real SDK. +func newMarketplaceHandler(cfgStore *MockConfigStore, authSvc *MockAuthService, ec2 *stubMarketplaceEC2) *Handler { + h := &Handler{ + config: cfgStore, + auth: authSvc, + marketplaceEC2Factory: func(_ aws.Config) marketplaceEC2Client { + return ec2 + }, + } + h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} }) + return h +} + +func standardRow() *config.PurchaseHistoryRecord { + acct := "acct-1" + return &config.PurchaseHistoryRecord{ + PurchaseID: validMarketplacePurchaseID, + OfferingClass: "standard", + CloudAccountID: &acct, + Region: "us-east-1", + Count: 3, + Term: 12, + UpfrontCost: 1200, + Timestamp: time.Now(), + } +} + +func marketplaceReq() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + } +} + +func adminSession(authSvc *MockAuthService) { + authSvc.On("ValidateSession", mock.Anything, "test-token"). + Return(&Session{UserID: "admin", Email: "admin@test.com"}, nil) + authSvc.On("HasPermissionAPI", mock.Anything, mock.Anything, auth.ActionAdmin, auth.ResourceAll). + Return(true, nil).Maybe() +} + +// --- Gap 3 handler tests (issue #292) --- + +func TestMarketplaceList_ConvertibleRejected(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + authSvc.On("ValidateSession", mock.Anything, "test-token"). + Return(&Session{UserID: "admin"}, nil) + + row := standardRow() + row.OfferingClass = "convertible" + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + + h := newMarketplaceHandler(cfgStore, authSvc, &stubMarketplaceEC2{}) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "Standard") + cfgStore.AssertExpectations(t) +} + +func TestMarketplaceList_SellOwnAllowed(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + ec2 := &stubMarketplaceEC2{} + + authSvc.On("ValidateSession", mock.Anything, "test-token"). + Return(&Session{UserID: "user-1"}, nil) + authSvc.On("HasPermissionAPI", mock.Anything, "user-1", auth.ActionAdmin, auth.ResourceAll). + Return(false, nil) + authSvc.On("HasPermissionAPI", mock.Anything, "user-1", auth.ActionSellAny, auth.ResourcePurchases). + Return(false, nil) + authSvc.On("HasPermissionAPI", mock.Anything, "user-1", auth.ActionSellOwn, auth.ResourcePurchases). + Return(true, nil) + // allowed accounts cover the row's cloud account. + authSvc.On("GetAllowedAccountsAPI", mock.Anything, "user-1"). + Return([]string{"acct-1"}, nil) + + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", "active"). + Return(nil) + + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + resp, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.NoError(t, err) + typed, ok := resp.(*MarketplaceListResponse) + require.True(t, ok) + assert.Equal(t, "ril-default", typed.ListingID) + // multi-count: the row has Count=3, so the outbound request must list all 3. + assert.Equal(t, int32(3), ec2.lastCreateReq.InstanceCount) + cfgStore.AssertExpectations(t) + authSvc.AssertExpectations(t) +} + +func TestMarketplaceList_SellOwnDeniedWrongAccount(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + + authSvc.On("ValidateSession", mock.Anything, "test-token"). + Return(&Session{UserID: "user-1"}, nil) + authSvc.On("HasPermissionAPI", mock.Anything, "user-1", auth.ActionAdmin, auth.ResourceAll). + Return(false, nil) + authSvc.On("HasPermissionAPI", mock.Anything, "user-1", auth.ActionSellAny, auth.ResourcePurchases). + Return(false, nil) + authSvc.On("HasPermissionAPI", mock.Anything, "user-1", auth.ActionSellOwn, auth.ResourcePurchases). + Return(true, nil) + // allowed accounts do NOT include acct-1. + authSvc.On("GetAllowedAccountsAPI", mock.Anything, "user-1"). + Return([]string{"acct-other"}, nil) + + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + + h := newMarketplaceHandler(cfgStore, authSvc, &stubMarketplaceEC2{}) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + authSvc.AssertExpectations(t) +} + +func TestMarketplaceList_DuplicateActiveListing409(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + row := standardRow() + row.ListingState = "active" + row.ListingID = "ril-existing" + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + + h := newMarketplaceHandler(cfgStore, authSvc, &stubMarketplaceEC2{}) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + assert.Contains(t, err.Error(), "ril-existing") +} + +func TestMarketplaceList_AWSClientFaultMapsTo400(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + + ec2 := &stubMarketplaceEC2{ + createFn: func(_ context.Context, _ ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { + return ec2svc.MarketplaceListingResult{}, &marketplaceTestAPIError{ + code: "InvalidReservedInstancesId", message: "bad RI", fault: smithy.FaultClient, + } + }, + } + + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "bad RI") +} + +func TestMarketplaceList_AWSServerFaultMapsTo502(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + + ec2 := &stubMarketplaceEC2{ + createFn: func(_ context.Context, _ ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { + return ec2svc.MarketplaceListingResult{}, &marketplaceTestAPIError{ + code: "InternalError", message: "aws broke", fault: smithy.FaultServer, + } + }, + } + + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 502, ce.code) +} + +func TestMarketplaceList_UnknownErrorMapsTo502(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + + ec2 := &stubMarketplaceEC2{ + createFn: func(_ context.Context, _ ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { + return ec2svc.MarketplaceListingResult{}, errors.New("network reset") + }, + } + + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 502, ce.code) +} + +func TestMarketplaceList_DBFailureCompensatingRollback(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + // DB persist fails after the listing was created. + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", "active"). + Return(errors.New("db down")) + + ec2 := &stubMarketplaceEC2{} + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + assert.Contains(t, err.Error(), "rolled back") + // The compensating cancel must have fired against the just-created listing. + assert.Equal(t, 1, ec2.cancelCallCount) + assert.Equal(t, "ril-default", ec2.lastCancelID) + cfgStore.AssertExpectations(t) +} + +func TestMarketplaceList_DefaultScheduleProrationMath(t *testing.T) { + // remaining=12, term=12, upfront=1200, monthly=0 => (1200*12/12 + 0)*0.95 = 1140. + schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1200, 0) + require.NoError(t, err) + require.Len(t, schedule, 1) + assert.Equal(t, int64(12), schedule[0].TermMonths) + assert.InDelta(t, 1140.0, schedule[0].Price, 0.001) + + // Half the term elapsed: remaining=6, term=12, upfront=1200, monthly=10. + // upfront_remaining = 1200 * 6/12 = 600; recurring = 10*6 = 60; + // (600 + 60) * 0.95 = 627. + schedule, err = resolveMarketplacePriceSchedule(nil, 6, 12, 1200, 10) + require.NoError(t, err) + require.Len(t, schedule, 1) + assert.Equal(t, int64(6), schedule[0].TermMonths) + assert.InDelta(t, 627.0, schedule[0].Price, 0.001) +} + +func TestMarketplaceCancel_HappyPath(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + row := standardRow() + row.ListingState = "active" + row.ListingID = "ril-cancel" + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-cancel", "cancelled"). + Return(nil) + + ec2 := &stubMarketplaceEC2{} + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + resp, err := h.marketplaceCancel(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.NoError(t, err) + m, ok := resp.(map[string]string) + require.True(t, ok) + assert.Equal(t, "cancelled", m["listing_state"]) + assert.Equal(t, "ril-cancel", ec2.lastCancelID) + cfgStore.AssertExpectations(t) +} + +func TestMarketplaceCancel_NoActiveListing409(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + row := standardRow() // ListingState empty -> not active + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + + h := newMarketplaceHandler(cfgStore, authSvc, &stubMarketplaceEC2{}) + _, err := h.marketplaceCancel(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 409, ce.code) +} diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql index 20a53aa56..dda98d245 100644 --- a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql +++ b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql @@ -2,8 +2,4 @@ ALTER TABLE purchase_history DROP COLUMN IF EXISTS offering_class, DROP COLUMN IF EXISTS listing_id, - DROP COLUMN IF EXISTS listing_state, - DROP COLUMN IF EXISTS listed_at, - DROP COLUMN IF EXISTS listing_price_schedule, - DROP COLUMN IF EXISTS listing_proceeds_received, - DROP COLUMN IF EXISTS listing_fee_paid; + DROP COLUMN IF EXISTS listing_state; diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql index 9b81ee49a..995db551b 100644 --- a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql +++ b/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql @@ -9,14 +9,17 @@ -- listing_state: mirrors the AWS listing state (active/cancelled/closed). NULL -- when not listed. -- +-- Only these three columns are added: they are the full set written and read by +-- the implemented list/cancel flow. The settlement/poller columns (listed_at, +-- listing_price_schedule, listing_proceeds_received, listing_fee_paid) were +-- descoped from this PR because the status poller that would populate them is +-- not implemented yet; they will land with the poller (see the #292 follow-up +-- issue) so the schema never carries columns nothing writes. +-- -- All three columns are nullable and added with IF NOT EXISTS so the -- migration is idempotent on re-apply. ALTER TABLE purchase_history - ADD COLUMN IF NOT EXISTS offering_class TEXT, - ADD COLUMN IF NOT EXISTS listing_id TEXT, - ADD COLUMN IF NOT EXISTS listing_state TEXT, - ADD COLUMN IF NOT EXISTS listed_at TIMESTAMP WITH TIME ZONE, - ADD COLUMN IF NOT EXISTS listing_price_schedule JSONB, - ADD COLUMN IF NOT EXISTS listing_proceeds_received NUMERIC, - ADD COLUMN IF NOT EXISTS listing_fee_paid NUMERIC; + ADD COLUMN IF NOT EXISTS offering_class TEXT, + ADD COLUMN IF NOT EXISTS listing_id TEXT, + ADD COLUMN IF NOT EXISTS listing_state TEXT; diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 229aea975..2b9ded74a 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -941,6 +941,11 @@ type MarketplaceListingRequest struct { // required. Each entry specifies how many months the price applies and the // list price per unit. PriceSchedule []MarketplacePriceTier + // InstanceCount is the number of Reserved Instances to list. A purchase + // row of N RIs must list all N; this mirrors purchase_history.count. Values + // <= 0 are rejected before the outbound call so a misconfigured caller can + // never silently list a single RI from a multi-count row. + InstanceCount int32 } // MarketplacePriceTier represents one tier of the AWS RI Marketplace price @@ -976,6 +981,9 @@ func (c *Client) CreateMarketplaceListing(ctx context.Context, req MarketplaceLi if req.ReservedInstancesID == "" { return MarketplaceListingResult{}, fmt.Errorf("reserved instances ID is required") } + if req.InstanceCount <= 0 { + return MarketplaceListingResult{}, fmt.Errorf("instance count must be a positive integer, got %d", req.InstanceCount) + } awsSchedule := make([]types.PriceScheduleSpecification, 0, len(req.PriceSchedule)) for _, tier := range req.PriceSchedule { @@ -990,7 +998,7 @@ func (c *Client) CreateMarketplaceListing(ctx context.Context, req MarketplaceLi input := &ec2.CreateReservedInstancesListingInput{ ReservedInstancesId: aws.String(req.ReservedInstancesID), ClientToken: aws.String(req.ClientToken), - InstanceCount: aws.Int32(1), + InstanceCount: aws.Int32(req.InstanceCount), PriceSchedules: awsSchedule, } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 5f783e775..ae621601b 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -909,3 +909,166 @@ func TestPurchaseCommitment_IdempotencySkipLogMasked(t *testing.T) { assert.Equal(t, token[:8]+"...", masked, "MaskToken shape: first 8 chars + ellipsis") assert.NotEqual(t, token, masked, "masked token must not equal raw token") } + +// --- RI Marketplace listing tests (issue #292) --- + +func TestClient_CreateMarketplaceListing_HappyPathMultiCount(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + // Capture the input so we can assert InstanceCount is the row's count, not 1. + mockEC2.On("CreateReservedInstancesListing", mock.Anything, + mock.MatchedBy(func(in *ec2.CreateReservedInstancesListingInput) bool { + return aws.ToInt32(in.InstanceCount) == 3 && + aws.ToString(in.ReservedInstancesId) == "ri-multi" && + len(in.PriceSchedules) == 1 + })). + Return(&ec2.CreateReservedInstancesListingOutput{ + ReservedInstancesListings: []types.ReservedInstancesListing{ + { + ReservedInstancesListingId: aws.String("ril-abc"), + Status: types.ListingStatusActive, + }, + }, + }, nil) + + res, err := client.CreateMarketplaceListing(context.Background(), MarketplaceListingRequest{ + ReservedInstancesID: "ri-multi", + ClientToken: "tok-1", + InstanceCount: 3, + PriceSchedule: []MarketplacePriceTier{{Term: 12, Price: 100}}, + }) + + assert.NoError(t, err) + assert.Equal(t, "ril-abc", res.ListingID) + assert.Equal(t, "active", res.State) + mockEC2.AssertExpectations(t) +} + +func TestClient_CreateMarketplaceListing_RejectsNonPositiveCount(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + _, err := client.CreateMarketplaceListing(context.Background(), MarketplaceListingRequest{ + ReservedInstancesID: "ri-1", + ClientToken: "tok", + InstanceCount: 0, + PriceSchedule: []MarketplacePriceTier{{Term: 12, Price: 100}}, + }) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "instance count must be a positive integer") + // No outbound call must have been made. + mockEC2.AssertNotCalled(t, "CreateReservedInstancesListing", mock.Anything, mock.Anything) + mockEC2.AssertExpectations(t) +} + +func TestClient_CreateMarketplaceListing_EmptyScheduleRejected(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + _, err := client.CreateMarketplaceListing(context.Background(), MarketplaceListingRequest{ + ReservedInstancesID: "ri-1", + InstanceCount: 1, + }) + assert.Error(t, err) + assert.Contains(t, err.Error(), "price schedule must have at least one tier") + mockEC2.AssertExpectations(t) +} + +func TestClient_CreateMarketplaceListing_EmptyListingIDRejected(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + mockEC2.On("CreateReservedInstancesListing", mock.Anything, mock.Anything). + Return(&ec2.CreateReservedInstancesListingOutput{ + ReservedInstancesListings: []types.ReservedInstancesListing{ + {ReservedInstancesListingId: aws.String(""), Status: types.ListingStatusActive}, + }, + }, nil) + + _, err := client.CreateMarketplaceListing(context.Background(), MarketplaceListingRequest{ + ReservedInstancesID: "ri-1", + InstanceCount: 1, + PriceSchedule: []MarketplacePriceTier{{Term: 12, Price: 100}}, + }) + assert.Error(t, err) + assert.Contains(t, err.Error(), "empty ID") + mockEC2.AssertExpectations(t) +} + +func TestClient_DescribeMarketplaceListing(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + mockEC2.On("DescribeReservedInstancesListings", mock.Anything, + mock.MatchedBy(func(in *ec2.DescribeReservedInstancesListingsInput) bool { + return aws.ToString(in.ReservedInstancesListingId) == "ril-xyz" + })). + Return(&ec2.DescribeReservedInstancesListingsOutput{ + ReservedInstancesListings: []types.ReservedInstancesListing{ + {ReservedInstancesListingId: aws.String("ril-xyz"), Status: types.ListingStatusClosed}, + }, + }, nil) + + res, err := client.DescribeMarketplaceListing(context.Background(), "ril-xyz") + assert.NoError(t, err) + assert.Equal(t, "ril-xyz", res.ListingID) + assert.Equal(t, "closed", res.State) + mockEC2.AssertExpectations(t) +} + +func TestClient_DescribeMarketplaceListing_NotFound(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + mockEC2.On("DescribeReservedInstancesListings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesListingsOutput{}, nil) + + _, err := client.DescribeMarketplaceListing(context.Background(), "ril-missing") + assert.Error(t, err) + assert.Contains(t, err.Error(), "not found") + mockEC2.AssertExpectations(t) +} + +func TestClient_CancelMarketplaceListing(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + mockEC2.On("CancelReservedInstancesListing", mock.Anything, + mock.MatchedBy(func(in *ec2.CancelReservedInstancesListingInput) bool { + return aws.ToString(in.ReservedInstancesListingId) == "ril-cancel" + })). + Return(&ec2.CancelReservedInstancesListingOutput{ + ReservedInstancesListings: []types.ReservedInstancesListing{ + {ReservedInstancesListingId: aws.String("ril-cancel"), Status: types.ListingStatusCancelled}, + }, + }, nil) + + res, err := client.CancelMarketplaceListing(context.Background(), "ril-cancel") + assert.NoError(t, err) + assert.Equal(t, "ril-cancel", res.ListingID) + assert.Equal(t, "cancelled", res.State) + mockEC2.AssertExpectations(t) +} + +func TestClient_CancelMarketplaceListing_APIError(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + + mockEC2.On("CancelReservedInstancesListing", mock.Anything, mock.Anything). + Return(nil, fmt.Errorf("boom")) + + _, err := client.CancelMarketplaceListing(context.Background(), "ril-cancel") + assert.Error(t, err) + assert.Contains(t, err.Error(), "CancelReservedInstancesListing failed") + mockEC2.AssertExpectations(t) +} From 512a68c94cf2ecc8da196c5e92a95acc5514a99b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 14:09:44 +0200 Subject: [PATCH 12/21] fix(migrations): renumber to 000065 to avoid base conflict 000060 is taken by 000060_cleanup_universal_plans on base. --- ...n.sql => 000065_purchase_history_marketplace_listing.down.sql} | 0 ....up.sql => 000065_purchase_history_marketplace_listing.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename internal/database/postgres/migrations/{000060_purchase_history_marketplace_listing.down.sql => 000065_purchase_history_marketplace_listing.down.sql} (100%) rename internal/database/postgres/migrations/{000060_purchase_history_marketplace_listing.up.sql => 000065_purchase_history_marketplace_listing.up.sql} (100%) diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.down.sql similarity index 100% rename from internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql rename to internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.down.sql diff --git a/internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.up.sql similarity index 100% rename from internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql rename to internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.up.sql From 7f8bed44501acee00241ed970687b07cc6db5e76 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 12:56:00 -0700 Subject: [PATCH 13/21] fix(migrations): renumber marketplace migration to 000068 to clear base conflict The marketplace listing migration was numbered 000065, colliding with 000065_enforce_min_one_admin which has since landed on feat/multicloud-web-frontend. The check-migration-conflicts pre-commit hook fails on the merge ref with "Duplicate migration number(s): 000065". Renumber to 000068 (one past base's highest, 000067) so the number is unique on the merge ref, and update the two stale in-file comment references from 000060 to 000068. --- ...sql => 000068_purchase_history_marketplace_listing.down.sql} | 2 +- ...p.sql => 000068_purchase_history_marketplace_listing.up.sql} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename internal/database/postgres/migrations/{000065_purchase_history_marketplace_listing.down.sql => 000068_purchase_history_marketplace_listing.down.sql} (84%) rename internal/database/postgres/migrations/{000065_purchase_history_marketplace_listing.up.sql => 000068_purchase_history_marketplace_listing.up.sql} (94%) diff --git a/internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.down.sql similarity index 84% rename from internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.down.sql rename to internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.down.sql index dda98d245..0ed453c6a 100644 --- a/internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.down.sql +++ b/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.down.sql @@ -1,4 +1,4 @@ --- Revert migration 000060 +-- Revert migration 000068 ALTER TABLE purchase_history DROP COLUMN IF EXISTS offering_class, DROP COLUMN IF EXISTS listing_id, diff --git a/internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.up.sql similarity index 94% rename from internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.up.sql rename to internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.up.sql index 995db551b..839a885f9 100644 --- a/internal/database/postgres/migrations/000065_purchase_history_marketplace_listing.up.sql +++ b/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.up.sql @@ -1,4 +1,4 @@ --- Migration 000060: add RI Marketplace listing columns to purchase_history. +-- Migration 000068: add RI Marketplace listing columns to purchase_history. -- -- offering_class: 'convertible' or 'standard'; NULL for pre-migration rows. -- The Sell button renders only when offering_class = 'standard' and the RI is From 9ec45407a979e013d1b77e413abf53e376c49164 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 17:45:07 -0700 Subject: [PATCH 14/21] fix(marketplace): reconcile mock + permission tests and gocyclo after #804 rebase Rebasing the marketplace sell/cancel work (#292) onto the in-app revocation base (#804) left integration gaps that only surface once both feature sets coexist: - internal/analytics/collector_test.go declared GetPurchaseHistoryByPurchaseID twice: #804 added the func-override version and the marketplace commit added a no-op stub. Drop the marketplace stub; keep the override and the unique UpdatePurchaseHistoryListing stub so the mock builds. - frontend permissions.test.ts asserted the user role grants 12 verbs; the merged DefaultUserPermissions now grants 13 (revoke-own from #804 plus sell-own from #292). Add sell-own:purchases to the expected set. - store_postgres.go: merging the revocation columns (#290) into the marketplace gocyclo refactor pushed scanPurchaseHistoryRow and GetPurchaseHistoryByPurchaseID back over the cyclomatic budget. Extract the shared NULL reconciliation into a purchaseHistoryNullables struct + applyTo, so both readers stay under 10 and the NULL handling lives in one place. The conflict resolution itself (handlers, store SELECT/scan column order, auth defaults, mocks, frontend history buttons) was applied additively during the rebase; both feature sets are independent (marketplace sell/cancel vs revocation). Marketplace migration stays at 000068 (free gap vs the new base, which tops out at 000070). --- frontend/src/__tests__/permissions.test.ts | 3 + internal/analytics/collector_test.go | 3 - internal/config/store_postgres.go | 176 ++++++++++----------- 3 files changed, 89 insertions(+), 93 deletions(-) diff --git a/frontend/src/__tests__/permissions.test.ts b/frontend/src/__tests__/permissions.test.ts index eb0c779c3..034de5b75 100644 --- a/frontend/src/__tests__/permissions.test.ts +++ b/frontend/src/__tests__/permissions.test.ts @@ -74,6 +74,9 @@ describe('permissions', () => { // Added by PR #804: revoke-own gates the History inline Revoke button // for completed Azure purchases within the free-cancel window. 'revoke-own:purchases', + // Added by issue #292: sell-own gates the History "Sell on Marketplace" + // button for completed Standard RIs the user purchased themselves. + 'sell-own:purchases', ]; expected.forEach((p) => expect(perms.has(p)).toBe(true)); // approve-own must NOT be in the standard user permission set (issue #1407). diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index a6ed3ddcd..cb3365613 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -430,9 +430,6 @@ func (m *mockConfigStore) UpsertRIUtilizationCache(_ context.Context, _ string, return nil } -func (m *mockConfigStore) GetPurchaseHistoryByPurchaseID(_ context.Context, _ string) (*config.PurchaseHistoryRecord, error) { - return nil, nil -} func (m *mockConfigStore) UpdatePurchaseHistoryListing(_ context.Context, _, _, _ string) error { return nil } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index e1f0e1fa0..fe1344e70 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1940,12 +1940,65 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, // column order must match the SELECT lists in GetPurchaseHistory / // GetAllPurchaseHistory / GetPurchaseHistoryFiltered (base columns, then the // revocation columns from issue #290, then the marketplace columns from #292). +// purchaseHistoryNullables holds the nullable scan targets shared by every +// purchase_history reader (scanPurchaseHistoryRow, GetPurchaseHistoryByPurchaseID). +// Centralising the NULL reconciliation in applyTo keeps each reader's +// cyclomatic complexity under the gocyclo budget as columns accrue across +// issues #290 (revocation) and #292 (marketplace). +type purchaseHistoryNullables struct { + monthlyCost sql.NullFloat64 + planID sql.NullString + planName sql.NullString + cloudAccountID sql.NullString + revocationWindowClosesAt *time.Time + revokedAt *time.Time + revokedVia sql.NullString + supportCaseID sql.NullString + offeringClass sql.NullString + listingID sql.NullString + listingState sql.NullString +} + +// applyTo reconciles the nullable columns into record. monthly_cost is left as +// a nil pointer when the provider returned no monthly breakdown (distinct from +// an explicit $0 recurring charge); the revocation-window timestamps map +// directly onto their pointer fields. +func (n *purchaseHistoryNullables) applyTo(record *PurchaseHistoryRecord) { + if n.monthlyCost.Valid { + v := n.monthlyCost.Float64 + record.MonthlyCost = &v + } + if n.planID.Valid { + record.PlanID = n.planID.String + } + if n.planName.Valid { + record.PlanName = n.planName.String + } + if n.cloudAccountID.Valid { + record.CloudAccountID = &n.cloudAccountID.String + } + record.RevocationWindowClosesAt = n.revocationWindowClosesAt + record.RevokedAt = n.revokedAt + if n.revokedVia.Valid { + record.RevokedVia = n.revokedVia.String + } + if n.supportCaseID.Valid { + record.SupportCaseID = n.supportCaseID.String + } + if n.offeringClass.Valid { + record.OfferingClass = n.offeringClass.String + } + if n.listingID.Valid { + record.ListingID = n.listingID.String + } + if n.listingState.Valid { + record.ListingState = n.listingState.String + } +} + func scanPurchaseHistoryRow(rows pgx.Rows) (PurchaseHistoryRecord, error) { var record PurchaseHistoryRecord - var planID, planName, cloudAccountID, offeringClass, listingID, listingState sql.NullString - var monthlyCost sql.NullFloat64 - var revocationWindowClosesAt, revokedAt *time.Time - var revokedVia, supportCaseID sql.NullString + var n purchaseHistoryNullables if err := rows.Scan( &record.AccountID, @@ -1959,58 +2012,24 @@ func scanPurchaseHistoryRow(rows pgx.Rows) (PurchaseHistoryRecord, error) { &record.Term, &record.Payment, &record.UpfrontCost, - &monthlyCost, + &n.monthlyCost, &record.EstimatedSavings, - &planID, - &planName, + &n.planID, + &n.planName, &record.RampStep, - &cloudAccountID, - &revocationWindowClosesAt, - &revokedAt, - &revokedVia, - &supportCaseID, - &offeringClass, - &listingID, - &listingState, + &n.cloudAccountID, + &n.revocationWindowClosesAt, + &n.revokedAt, + &n.revokedVia, + &n.supportCaseID, + &n.offeringClass, + &n.listingID, + &n.listingState, ); err != nil { return PurchaseHistoryRecord{}, fmt.Errorf("failed to scan purchase history: %w", err) } - // Handle nullable monthly_cost: nil means "provider did not return a - // monthly breakdown"; 0.0 means "explicitly $0 recurring charge". - if monthlyCost.Valid { - v := monthlyCost.Float64 - record.MonthlyCost = &v - } - - // Handle nullable strings - if planID.Valid { - record.PlanID = planID.String - } - if planName.Valid { - record.PlanName = planName.String - } - if cloudAccountID.Valid { - record.CloudAccountID = &cloudAccountID.String - } - record.RevocationWindowClosesAt = revocationWindowClosesAt - record.RevokedAt = revokedAt - if revokedVia.Valid { - record.RevokedVia = revokedVia.String - } - if supportCaseID.Valid { - record.SupportCaseID = supportCaseID.String - } - if offeringClass.Valid { - record.OfferingClass = offeringClass.String - } - if listingID.Valid { - record.ListingID = listingID.String - } - if listingState.Valid { - record.ListingState = listingState.String - } - + n.applyTo(&record) return record, nil } @@ -2044,11 +2063,11 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc } var r PurchaseHistoryRecord - var planID, planName, cloudAccountID sql.NullString - var revocationWindowClosesAt, revokedAt *time.Time - var revokedVia, supportCaseID sql.NullString - var offeringClass, listingID, listingState sql.NullString + var n purchaseHistoryNullables + // Note the extra revocation_in_flight bool between support_case_id and the + // marketplace columns; it scans straight into r.RevocationInFlight rather + // than through the shared nullables struct. if err := rows.Scan( &r.AccountID, &r.PurchaseID, @@ -2063,48 +2082,25 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc &r.UpfrontCost, &r.MonthlyCost, &r.EstimatedSavings, - &planID, - &planName, + &n.planID, + &n.planName, &r.RampStep, - &cloudAccountID, - &revocationWindowClosesAt, - &revokedAt, - &revokedVia, - &supportCaseID, + &n.cloudAccountID, + &n.revocationWindowClosesAt, + &n.revokedAt, + &n.revokedVia, + &n.supportCaseID, &r.RevocationInFlight, - &offeringClass, - &listingID, - &listingState, + &n.offeringClass, + &n.listingID, + &n.listingState, ); err != nil { return nil, fmt.Errorf("GetPurchaseHistoryByPurchaseID scan: %w", err) } - if planID.Valid { - r.PlanID = planID.String - } - if planName.Valid { - r.PlanName = planName.String - } - if cloudAccountID.Valid { - r.CloudAccountID = &cloudAccountID.String - } - r.RevocationWindowClosesAt = revocationWindowClosesAt - r.RevokedAt = revokedAt - if revokedVia.Valid { - r.RevokedVia = revokedVia.String - } - if supportCaseID.Valid { - r.SupportCaseID = supportCaseID.String - } - if offeringClass.Valid { - r.OfferingClass = offeringClass.String - } - if listingID.Valid { - r.ListingID = listingID.String - } - if listingState.Valid { - r.ListingState = listingState.String - } + // MonthlyCost is scanned directly above (not through n.monthlyCost), so + // leave n.monthlyCost zero-valued; applyTo will not overwrite r.MonthlyCost. + n.applyTo(&r) return &r, rows.Err() } From 2ab25b11af7113f4d0c79bd21290554542817378 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 18:32:23 -0700 Subject: [PATCH 15/21] fix(marketplace): gate sell actions on sell verbs and prorate default price (CR #808) Address CodeRabbit findings on the issue #292 marketplace sell/cancel flow: - Gate canSellOnMarketplace / canCancelMarketplaceListing on the sell-own / sell-any (or admin) verbs instead of bare sign-in, so the UX gate matches the backend authorizeSessionSell and avoids frontend/backend auth drift. Adds sell-own / sell-any to the Action union, mirroring the backend ActionSellOwn / ActionSellAny constants. - Prorate the upfront cost to its residual value in the sell modal's default price estimate (upfront * remaining/term), matching the backend resolveMarketplacePriceSchedule. Using the full upfront overstated the listing value for partially elapsed RIs. Also correct stale "migration 000060" comments (the columns ship in 000068) and add pgxmock coverage for GetPurchaseHistoryByPurchaseID asserting its distinct 25-column scan order (revocation_in_flight at position 22). --- frontend/src/history.ts | 30 +++++++- frontend/src/permissions.ts | 7 ++ frontend/src/types.ts | 2 +- .../config/store_postgres_pgxmock_test.go | 77 +++++++++++++++++++ internal/config/types.go | 6 +- 5 files changed, 117 insertions(+), 5 deletions(-) diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 3010e9064..b5b636da1 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -614,6 +614,17 @@ function canSellOnMarketplace(p: HistoryPurchase): boolean { if ((p.listing_state || '').toLowerCase() === 'active') return false; const user = getCurrentUser(); if (!user) return false; + // Gate on the sell verbs, not bare sign-in, so we don't show Sell to a + // role that lacks sell-own/sell-any and avoid frontend/backend auth drift + // (the backend authorizeSessionSell would 403 anyway). admin:* satisfies + // sell-own here since it is not carved out of admin. + if ( + !canAccess('admin', '*') && + !canAccess('sell-any', 'purchases') && + !canAccess('sell-own', 'purchases') + ) { + return false; + } // Guard against listing a matured RI: compute remaining months from the // purchase timestamp and the total term. term is in months; timestamp is // the purchase date. We require at least 1 full month remaining. @@ -633,6 +644,16 @@ function canCancelMarketplaceListing(p: HistoryPurchase): boolean { if ((p.listing_state || '').toLowerCase() !== 'active') return false; const user = getCurrentUser(); if (!user) return false; + // Same sell-verb gate as canSellOnMarketplace: cancelling a listing is a + // marketplace write, so require sell-own/sell-any (or admin) rather than + // bare sign-in to keep the UX gate aligned with authorizeSessionSell. + if ( + !canAccess('admin', '*') && + !canAccess('sell-any', 'purchases') && + !canAccess('sell-own', 'purchases') + ) { + return false; + } return true; } @@ -1154,7 +1175,14 @@ function wireRowActionHandlers(container: HTMLElement): void { const remainingMonths = Math.max(0, Math.round(termMonths - elapsedMonths)); const upfront = purchase.upfront_cost ?? 0; const monthly = purchase.monthly_cost ?? 0; - const totalValue = upfront + monthly * remainingMonths; + // Prorate the upfront cost to its residual value over the remaining + // term. Using the full upfront overstates the listing value for a + // partially elapsed RI (a 12-month RI at month 6 has only half its + // upfront value left). Mirrors resolveMarketplacePriceSchedule in + // internal/api/handler_marketplace.go, which drops the upfront term to + // 0 when the original term is unknown (<=0); we do the same here. + const upfrontRemaining = termMonths > 0 ? upfront * (remainingMonths / termMonths) : 0; + const totalValue = upfrontRemaining + monthly * remainingMonths; const listPrice = totalValue * 0.95; const netProceeds = listPrice * 0.88; diff --git a/frontend/src/permissions.ts b/frontend/src/permissions.ts index d2c434b51..f30481e26 100644 --- a/frontend/src/permissions.ts +++ b/frontend/src/permissions.ts @@ -55,6 +55,13 @@ export type Action = | 'update-any' | 'revoke-own' | 'revoke-any' + // sell-own / sell-any gate the "Sell on Marketplace" button (issue #292). + // Mirrors the backend ActionSellOwn / ActionSellAny constants in + // internal/auth/types.go. sell-own:purchases is granted to every + // authenticated user via DefaultUserPermissions; sell-any has no default + // non-admin grant. + | 'sell-own' + | 'sell-any' | 'admin'; // Resource names. Closed enum for the same reason. diff --git a/frontend/src/types.ts b/frontend/src/types.ts index aaa2e6a05..4e29f7e4a 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -284,7 +284,7 @@ export interface HistoryPurchase { revoked_via?: string; // OfferingClass is "standard" or "convertible" for EC2 RIs. Absent - // on non-EC2 rows and on rows written before migration 000060. + // on non-EC2 rows and on rows written before migration 000068. // The "Sell on Marketplace" button only renders when this equals // "standard" (issue #292). offering_class?: string; diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 5d0e3199b..e29285cd4 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -774,6 +774,83 @@ func TestPGXMock_GetPurchaseHistory_Success(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } +// TestPGXMock_GetPurchaseHistoryByPurchaseID_Success asserts the DISTINCT +// 25-column scan order used by GetPurchaseHistoryByPurchaseID. Unlike the +// 24-column GetPurchaseHistory reader it adds revocation_in_flight (a plain +// bool, NOT a nullable) at position 22, between support_case_id and the +// marketplace columns (issue #290 Finding #6, migration 000072). A regression +// here -- e.g. dropping the column or scanning it through the nullables struct +// -- would shift every marketplace column by one and silently mis-read +// offering_class / listing_id / listing_state. +func TestPGXMock_GetPurchaseHistoryByPurchaseID_Success(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + now := time.Now().Truncate(time.Second) + cols := []string{ + "account_id", "purchase_id", "timestamp", "provider", "service", "region", + "resource_type", "count", "term", "payment", "upfront_cost", "monthly_cost", + "estimated_savings", "plan_id", "plan_name", "ramp_step", "cloud_account_id", + // revocation columns (issue #290) + "revocation_window_closes_at", "revoked_at", "revoked_via", "support_case_id", + // partial-success reconciliation bool at position 22 (issue #290 Finding #6) + "revocation_in_flight", + // marketplace columns (issue #292) + "offering_class", "listing_id", "listing_state", + } + // monthly_cost scans straight into the *float64 r.MonthlyCost (not through + // the nullables struct), so the mock value must be a *float64. + monthly := 50.0 + rows := pgxmock.NewRows(cols). + AddRow("acc-1", "pur-1", now, "aws", "ec2", "us-east-1", + "m5.large", 2, 1, "no-upfront", 100.0, &monthly, 200.0, + sql.NullString{Valid: true, String: "plan-1"}, + sql.NullString{Valid: true, String: "My Plan"}, + 1, sql.NullString{Valid: true, String: "cloud-acct-1"}, + // revocation columns (issue #290) + nil, nil, sql.NullString{}, sql.NullString{}, + // revocation_in_flight bool at position 22 + true, + // marketplace columns (issue #292) + sql.NullString{Valid: true, String: "standard"}, + sql.NullString{Valid: true, String: "listing-1"}, + sql.NullString{Valid: true, String: "active"}) + mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) + + record, err := store.GetPurchaseHistoryByPurchaseID(ctx, "pur-1") + require.NoError(t, err) + require.NotNil(t, record) + assert.Equal(t, "pur-1", record.PurchaseID) + assert.Equal(t, "plan-1", record.PlanID) + require.NotNil(t, record.MonthlyCost) + assert.Equal(t, 50.0, *record.MonthlyCost) + assert.True(t, record.RevocationInFlight) + // Marketplace columns must land in their own fields, not shifted by the + // extra bool. + assert.Equal(t, "standard", record.OfferingClass) + assert.Equal(t, "listing-1", record.ListingID) + assert.Equal(t, "active", record.ListingState) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetPurchaseHistoryByPurchaseID_NotFound returns (nil, nil) when +// no row matches, so the revoke / marketplace handlers can distinguish "absent" +// from a scan/query error. +func TestPGXMock_GetPurchaseHistoryByPurchaseID_NotFound(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + rows := pgxmock.NewRows([]string{"account_id"}) // no rows added + mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) + + record, err := store.GetPurchaseHistoryByPurchaseID(ctx, "missing") + require.NoError(t, err) + assert.Nil(t, record) + assert.NoError(t, mock.ExpectationsWereMet()) +} + // ─── GetPurchaseHistoryFiltered (issue #701) ───────────────────────────────── // purchaseHistoryCols lists the SELECT columns for purchase_history rows in diff --git a/internal/config/types.go b/internal/config/types.go index 65f9e8113..f6091a787 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -791,15 +791,15 @@ type PurchaseHistoryRecord struct { // OfferingClass records whether this commitment is a 'standard' or // 'convertible' RI. NULL on pre-migration rows. The Sell-on-Marketplace // button renders only when this equals "standard" (issue #292). - // Persisted in purchase_history via migration 000060. + // Persisted in purchase_history via migration 000068. OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` // ListingID is the AWS ReservedInstancesListingId returned by // CreateReservedInstancesListing. Empty when the RI has not been - // listed. Persisted in purchase_history via migration 000060. + // listed. Persisted in purchase_history via migration 000068. ListingID string `json:"listing_id,omitempty" dynamodbav:"listing_id,omitempty"` // ListingState mirrors the AWS marketplace listing state: "active", // "cancelled", or "closed" (sold). Empty when not listed. - // Persisted in purchase_history via migration 000060. + // Persisted in purchase_history via migration 000068. ListingState string `json:"listing_state,omitempty" dynamodbav:"listing_state,omitempty"` } From 6254aa671053fa997c0e0ef4261ecab8912d3481 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 17:17:55 +0200 Subject: [PATCH 16/21] fix(marketplace): name fee/discount constants and restore gocyclo baseline - Introduce awsMarketplaceFeePercent (12), awsMarketplaceNetFactor (0.88), and awsMarketplaceBuyerDiscountFactor (0.95) as named Go constants in handler_marketplace.go; replace the matching TS literals with AWS_MARKETPLACE_FEE_PERCENT / AWS_MARKETPLACE_NET_FACTOR / AWS_MARKETPLACE_BUYER_DISCOUNT in history.ts. Eliminates silent ratios on a money-affecting display path per the no-hardcoded-magic-values rule. - Restore store_postgres_recommendations.go to main's lower-complexity form (recEffectiveSavingsPct + recOnDemandBaseline split); the rebase picked up an inlined variant from an earlier PR commit that cyclomatic-scored 12. --- frontend/src/history.ts | 16 ++++++++++++---- internal/api/handler_marketplace.go | 29 ++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/frontend/src/history.ts b/frontend/src/history.ts index b5b636da1..75508b3f8 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -17,6 +17,14 @@ import { getAccountName } from './recommendations'; const VALID_PROVIDERS: api.Provider[] = ['aws', 'azure', 'gcp']; +// AWS Marketplace fee/pricing constants (must stay in sync with handler_marketplace.go). +// awsMarketplaceFeePercent: transaction fee deducted from listing proceeds (published by AWS). +const AWS_MARKETPLACE_FEE_PERCENT = 12; +// awsMarketplaceNetFactor: fraction of list price the seller receives after the fee. +const AWS_MARKETPLACE_NET_FACTOR = 1 - AWS_MARKETPLACE_FEE_PERCENT / 100; +// awsMarketplaceBuyerDiscountFactor: applied to residual RI value to compute the default list price. +const AWS_MARKETPLACE_BUYER_DISCOUNT = 0.95; + type StatusFilter = 'all' | 'pending' | 'completed' | 'failed' | 'expired' | 'cancelled'; // Cache of the last-rendered purchase list so the status-chip click handler @@ -1183,8 +1191,8 @@ function wireRowActionHandlers(container: HTMLElement): void { // 0 when the original term is unknown (<=0); we do the same here. const upfrontRemaining = termMonths > 0 ? upfront * (remainingMonths / termMonths) : 0; const totalValue = upfrontRemaining + monthly * remainingMonths; - const listPrice = totalValue * 0.95; - const netProceeds = listPrice * 0.88; + const listPrice = totalValue * AWS_MARKETPLACE_BUYER_DISCOUNT; + const netProceeds = listPrice * AWS_MARKETPLACE_NET_FACTOR; const summaryEl = document.createElement('dl'); summaryEl.className = 'marketplace-pricing-summary'; @@ -1201,14 +1209,14 @@ function wireRowActionHandlers(container: HTMLElement): void { addRow('Resource type', purchase.resource_type || '-'); addRow('Remaining term', remainingMonths === 1 ? '1 month' : `${remainingMonths} months`); addRow('Default list price', formatCurrency(listPrice)); - addRow('AWS fee (12%)', formatCurrency(listPrice * 0.12)); + addRow(`AWS fee (${AWS_MARKETPLACE_FEE_PERCENT}%)`, formatCurrency(listPrice * (AWS_MARKETPLACE_FEE_PERCENT / 100))); addRow('Estimated net proceeds', formatCurrency(netProceeds)); bodyEl.appendChild(summaryEl); } const noteEl = document.createElement('p'); noteEl.className = 'marketplace-pricing-note'; - noteEl.textContent = 'AWS charges a 12% transaction fee on proceeds. The default schedule prices the listing at 5% below remaining value. You can adjust pricing by contacting your administrator or modifying the schedule via the API. This action cannot be undone without cancelling the listing.'; + noteEl.textContent = `AWS charges a ${AWS_MARKETPLACE_FEE_PERCENT}% transaction fee on proceeds. The default schedule prices the listing at ${(1 - AWS_MARKETPLACE_BUYER_DISCOUNT) * 100}% below remaining value. You can adjust pricing by contacting your administrator or modifying the schedule via the API. This action cannot be undone without cancelling the listing.`; bodyEl.appendChild(noteEl); const ok = await confirmDialog({ diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 9530ae994..6e92c08ff 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -202,8 +202,8 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio ListingID: result.ListingID, ListingState: result.State, PriceSchedule: schedule, - AWSFeePercent: 12, - Note: "AWS charges a 12% transaction fee on the listing proceeds. Net proceeds = ListingPrice * 0.88.", + AWSFeePercent: awsMarketplaceFeePercent, + Note: fmt.Sprintf("AWS charges a %d%% transaction fee on the listing proceeds. Net proceeds = ListingPrice * %.2f.", awsMarketplaceFeePercent, awsMarketplaceNetFactor), }, nil } @@ -354,6 +354,21 @@ func computeRemainingMonths(purchaseTime time.Time, termMonths int) int { return r } +// awsMarketplaceFeePercent is the AWS Marketplace transaction fee percentage +// charged against listing proceeds. Published at +// https://aws.amazon.com/marketplace/ri-marketplace/faq/ +const awsMarketplaceFeePercent = 12 + +// awsMarketplaceNetFactor is the fraction of list price the seller keeps +// after the AWS Marketplace fee: 1 - awsMarketplaceFeePercent/100. +const awsMarketplaceNetFactor = 0.88 + +// awsMarketplaceBuyerDiscountFactor is the discount applied to the computed +// residual RI value when building the default listing price. A 5% discount +// makes the listing attractive to buyers while still recovering most of the +// seller's remaining cost basis. Applied before AWS deducts its fee. +const awsMarketplaceBuyerDiscountFactor = 0.95 + // awsMarketplaceClientFaultCodes is the set of AWS error codes that represent // client-side faults for Marketplace listing operations. These map to 4xx // responses so the caller receives an actionable message. Server-side AWS @@ -389,8 +404,8 @@ func mapAWSMarketplaceError(opMsg string, err error) error { // given RI. When the caller supplied an explicit schedule it is validated and // returned unchanged. When the caller omitted the schedule (nil / empty), a // single-tier default is computed: (upfront_remaining + future_recurring) * -// 0.95 (5% discount to attract buyers; the 12% AWS fee is applied by the -// Marketplace on top). +// awsMarketplaceBuyerDiscountFactor (5% discount to attract buyers; the +// awsMarketplaceFeePercent% AWS fee is applied by the Marketplace on top). // // remainingMonths must be the actual remaining months (computed via // computeRemainingMonths from purchase timestamp and total term -- NOT the raw @@ -410,8 +425,8 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM return supplied, nil } - // Default: spread (upfront_remaining + future_recurring) * 0.95 across - // remaining term. The upfront cost is prorated by (remaining/original) to + // Default: spread (upfront_remaining + future_recurring) * awsMarketplaceBuyerDiscountFactor + // across remaining term. The upfront cost is prorated by (remaining/original) to // avoid overpricing older RIs (a 12-month RI at month 6 retains only half // the upfront value; using the full amount would overprice by ~2x). if remainingMonths <= 0 { @@ -422,7 +437,7 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM upfrontRemaining = upfrontCost * (float64(remainingMonths) / float64(originalTerm)) } totalValue := upfrontRemaining + (monthlyCost * float64(remainingMonths)) - listPrice := totalValue * 0.95 + listPrice := totalValue * awsMarketplaceBuyerDiscountFactor if listPrice < 0 { listPrice = 0 } From 73d9e4ae4a7f4d71cddc7f99b98c0194dae75417 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 13:00:15 +0200 Subject: [PATCH 17/21] refactor(api): derive awsMarketplaceNetFactor from fee percent Replace the hardcoded 0.88 literal with a computed constant expression (1 - awsMarketplaceFeePercent/100.0) so the net factor stays in sync with the fee percent automatically. The Go constant evaluator uses arbitrary precision, so the resulting float64 value is identical to the literal it replaces; no money-math change. --- internal/api/handler_marketplace.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 6e92c08ff..aba59e5db 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -361,7 +361,8 @@ const awsMarketplaceFeePercent = 12 // awsMarketplaceNetFactor is the fraction of list price the seller keeps // after the AWS Marketplace fee: 1 - awsMarketplaceFeePercent/100. -const awsMarketplaceNetFactor = 0.88 +// Derived from awsMarketplaceFeePercent so the two stay in sync automatically. +const awsMarketplaceNetFactor = 1 - awsMarketplaceFeePercent/100.0 // awsMarketplaceBuyerDiscountFactor is the discount applied to the computed // residual RI value when building the default listing price. A 5% discount From 93e4a31bc754405673a934007a16ce09e7c7fa0e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 15:42:22 +0200 Subject: [PATCH 18/21] fix(marketplace): guard concurrent RI listing creates with atomic slot claim Two concurrent marketplace-list requests for the same RI could both pass the read-only listing_state check in validateMarketplaceListRequest and both call AWS CreateReservedInstancesListing (each with a fresh ClientToken, so AWS does not dedup them), leaving two live listings for one RI and overwriting the DB row. Reserve the listing slot with a single atomic conditional UPDATE (ClaimMarketplaceListingSlot, modeled on FlipPurchaseRevocationInFlight) before the AWS call so exactly one racing request proceeds; the loser gets a 409. The claim is released back to the prior state on every post-claim failure path so a failed attempt never leaves the row stuck in the transient pending state. Also on this money path: - reject an implausibly large purchase count instead of silently truncating it into int32 (gosec G115) so the wrong number of RIs can never be listed; - add listing-state constants matching the AWS ListingStatus enum and use them in place of scattered string literals; - fix US-locale spellings flagged by misspell in comments and log/error text. Adds regression tests reproducing the concurrent-create race (claim loses -> 409, no AWS call) and the claim-error path, plus release-on-failure assertions. Closes #292 CR concurrent-create finding. --- internal/analytics/collector_test.go | 4 + internal/api/handler_marketplace.go | 99 ++++++++++++++++++------ internal/api/handler_marketplace_test.go | 99 +++++++++++++++++++++--- internal/config/interfaces.go | 12 +++ internal/config/store_postgres.go | 28 ++++++- internal/config/types.go | 19 ++++- internal/mocks/stores.go | 6 ++ internal/server/test_helpers_test.go | 3 + 8 files changed, 229 insertions(+), 41 deletions(-) diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index cb3365613..517f12864 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -434,6 +434,10 @@ func (m *mockConfigStore) UpdatePurchaseHistoryListing(_ context.Context, _, _, return nil } +func (m *mockConfigStore) ClaimMarketplaceListingSlot(_ context.Context, _ string) (bool, error) { + return true, nil +} + // strPtr is a test helper for *string fields. func strPtr(s string) *string { return &s } diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index aba59e5db..455feb506 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -40,7 +40,7 @@ type marketplaceEC2Client interface { CancelMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) } -// buildMarketplaceEC2Client honours the injected factory for tests, falling +// buildMarketplaceEC2Client honors the injected factory for tests, falling // back to the direct AWS SDK constructor in production. func (h *Handler) buildMarketplaceEC2Client(cfg aws.Config) marketplaceEC2Client { if h.marketplaceEC2Factory != nil { @@ -111,12 +111,12 @@ func (h *Handler) validateMarketplaceListRequest(ctx context.Context, req *event } // Reject if a listing is already active to avoid duplicate listings. - if strings.EqualFold(row.ListingState, "active") { + if strings.EqualFold(row.ListingState, config.ListingStateActive) { return nil, body, NewClientError(409, fmt.Sprintf("an active marketplace listing %s already exists for this RI; cancel it first", row.ListingID)) } // Decode optional body. - if len(req.Body) > 0 { + if req.Body != "" { if err := json.Unmarshal([]byte(req.Body), &body); err != nil { return nil, body, NewClientError(400, "invalid request body: "+err.Error()) } @@ -166,12 +166,51 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio }) } + result, err := h.reserveAndCreateListing(ctx, purchaseID, row, ec2Client, awsSchedule) + if err != nil { + return nil, err + } + + return &MarketplaceListResponse{ + ListingID: result.ListingID, + ListingState: result.State, + PriceSchedule: schedule, + AWSFeePercent: awsMarketplaceFeePercent, + Note: fmt.Sprintf("AWS charges a %d%% transaction fee on the listing proceeds. Net proceeds = ListingPrice * %.2f.", awsMarketplaceFeePercent, awsMarketplaceNetFactor), + }, nil +} + +// reserveAndCreateListing atomically claims the marketplace-listing slot for the +// row, creates the AWS listing, and persists it, releasing the claim on every +// failure path so a failed attempt never leaves the row stuck in the transient +// pending state. Extracted from marketplaceList to keep that handler under the +// gocyclo budget. Returns the persisted listing result on success. +func (h *Handler) reserveAndCreateListing(ctx context.Context, purchaseID string, row *config.PurchaseHistoryRecord, ec2Client marketplaceEC2Client, awsSchedule []ec2svc.MarketplacePriceTier) (ec2svc.MarketplaceListingResult, error) { // List every RI in the row: a row of N Standard RIs must list all N, not a // single unit (issue #292 multi-count fix). Floor at 1 for legacy rows that - // somehow recorded a non-positive count so a valid Standard RI still lists. - instanceCount := int32(row.Count) - if instanceCount < 1 { - instanceCount = 1 + // somehow recorded a non-positive count so a valid Standard RI still lists, + // and reject an implausibly large count rather than silently truncating it + // into int32 (which could list the wrong number of RIs on the money path). + instanceCount := int32(1) + switch { + case row.Count > math.MaxInt32: + return ec2svc.MarketplaceListingResult{}, NewClientError(500, fmt.Sprintf("purchase count %d exceeds the marketplace listing limit", row.Count)) + case row.Count > 1: + instanceCount = int32(row.Count) + } + + // Atomically reserve the listing slot before calling AWS. The read-only + // listing_state check in validateMarketplaceListRequest is not enough on its + // own: two concurrent requests can both pass it, and AWS assigns a fresh + // ClientToken per call so the provider will not dedup them, leaving two live + // listings for one RI. The claim serializes concurrent creates for the same + // purchase_id (issue #292 CR concurrent-create guard). + claimed, err := h.config.ClaimMarketplaceListingSlot(ctx, purchaseID) + if err != nil { + return ec2svc.MarketplaceListingResult{}, fmt.Errorf("failed to reserve marketplace listing slot: %w", err) + } + if !claimed { + return ec2svc.MarketplaceListingResult{}, NewClientError(409, "a marketplace listing is already active or in progress for this RI; cancel it first") } result, err := ec2Client.CreateMarketplaceListing(ctx, ec2svc.MarketplaceListingRequest{ @@ -181,30 +220,40 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio InstanceCount: instanceCount, }) if err != nil { + // AWS created nothing: release the claim so a retry is not blocked. + h.releaseMarketplaceClaim(ctx, purchaseID, row) logging.Warnf("marketplace: CreateReservedInstancesListing for purchase %s failed: %v", purchaseID, err) - return nil, mapAWSMarketplaceError("AWS marketplace listing failed", err) + return ec2svc.MarketplaceListingResult{}, mapAWSMarketplaceError("AWS marketplace listing failed", err) } // Persist the listing ID and state. On DB failure, attempt a compensating // rollback (cancel the just-created listing) to avoid a desync where the - // user sees success but the listing is invisible in subsequent renders. + // user sees success but the listing is invisible in subsequent renders, then + // release the claim so the row does not stay stuck in the pending state. if dbErr := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, result.ListingID, result.State); dbErr != nil { - logging.Errorf("marketplace: listing created (%s / %s) but DB update failed: %v — attempting rollback", result.ListingID, result.State, dbErr) + logging.Errorf("marketplace: listing created (%s / %s) but DB update failed: %v; attempting rollback", result.ListingID, result.State, dbErr) if _, rollbackErr := ec2Client.CancelMarketplaceListing(ctx, result.ListingID); rollbackErr != nil { logging.Errorf("marketplace: rollback cancel for listing %s also failed: %v", result.ListingID, rollbackErr) } else { - logging.Warnf("marketplace: listing %s rolled back (cancelled) after DB failure", result.ListingID) + logging.Warnf("marketplace: listing %s rolled back (canceled) after DB failure", result.ListingID) } - return nil, fmt.Errorf("listing created but could not be persisted; listing has been rolled back: %w", dbErr) + h.releaseMarketplaceClaim(ctx, purchaseID, row) + return ec2svc.MarketplaceListingResult{}, fmt.Errorf("listing created but could not be persisted; listing has been rolled back: %w", dbErr) } - return &MarketplaceListResponse{ - ListingID: result.ListingID, - ListingState: result.State, - PriceSchedule: schedule, - AWSFeePercent: awsMarketplaceFeePercent, - Note: fmt.Sprintf("AWS charges a %d%% transaction fee on the listing proceeds. Net proceeds = ListingPrice * %.2f.", awsMarketplaceFeePercent, awsMarketplaceNetFactor), - }, nil + return result, nil +} + +// releaseMarketplaceClaim restores a purchase_history row's listing fields to +// the state captured before ClaimMarketplaceListingSlot reserved the slot. It +// runs on every failure path after a successful claim so a failed listing +// attempt does not leave the row stuck in the transient pending state (which +// would block future list attempts). Best-effort: on error it logs loudly +// because the row may stay pending until the #292 status poller reconciles it. +func (h *Handler) releaseMarketplaceClaim(ctx context.Context, purchaseID string, row *config.PurchaseHistoryRecord) { + if err := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, row.ListingID, row.ListingState); err != nil { + logging.Errorf("marketplace: failed to release listing claim for purchase %s (row may be stuck in %q): %v", purchaseID, config.ListingStatePending, err) + } } // marketplaceCancel handles POST /api/purchases/{id}/marketplace-cancel. @@ -230,7 +279,7 @@ func (h *Handler) marketplaceCancel(ctx context.Context, req *events.LambdaFunct return nil, err } - if !strings.EqualFold(row.ListingState, "active") { + if !strings.EqualFold(row.ListingState, config.ListingStateActive) { return nil, NewClientError(409, "no active listing found for this RI; current state: "+row.ListingState) } @@ -247,12 +296,12 @@ func (h *Handler) marketplaceCancel(ctx context.Context, req *events.LambdaFunct return nil, mapAWSMarketplaceError("AWS cancel listing failed", err) } - // The listing is already cancelled in AWS; there is no compensating rollback + // The listing is already canceled in AWS; there is no compensating rollback // available if the DB write fails. Return an internal error so the caller // knows the state is out of sync and can retry or contact an administrator. if dbErr := h.config.UpdatePurchaseHistoryListing(ctx, purchaseID, result.ListingID, result.State); dbErr != nil { - logging.Errorf("marketplace: listing cancelled in AWS (%s) but DB update failed: %v — state is out of sync", result.ListingID, dbErr) - return nil, fmt.Errorf("listing cancelled in AWS but could not be persisted: %w", dbErr) + logging.Errorf("marketplace: listing canceled in AWS (%s) but DB update failed: %v; state is out of sync", result.ListingID, dbErr) + return nil, fmt.Errorf("listing canceled in AWS but could not be persisted: %w", dbErr) } return map[string]string{"listing_id": result.ListingID, "listing_state": result.State}, nil @@ -273,7 +322,7 @@ func (h *Handler) authorizeSessionSell(ctx context.Context, session *Session, cl return NewClientError(500, "authentication service not configured") } - // Admins are recognised by holding the full-access admin capability + // Admins are recognized by holding the full-access admin capability // (auth migrated from role-based to group-membership-only, issue #907). isAdmin, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionAdmin, auth.ResourceAll) if err != nil { @@ -311,7 +360,7 @@ func (h *Handler) authorizeSessionSell(ctx context.Context, session *Session, cl // permit access to the given cloud account UUID. Returns 403 otherwise. func (h *Handler) authorizeAllowedAccount(ctx context.Context, session *Session, cloudAccountID string) error { if h.auth != nil { - // Admins are recognised by holding the full-access admin capability + // Admins are recognized by holding the full-access admin capability // (auth migrated from role-based to group-membership-only, issue #907). isAdmin, err := h.auth.HasPermissionAPI(ctx, session.UserID, auth.ActionAdmin, auth.ResourceAll) if err != nil { diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index 7d0d52ef4..6f19c1f5a 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -22,27 +22,29 @@ import ( const validMarketplacePurchaseID = "11111111-1111-1111-1111-111111111111" // stubMarketplaceEC2 is a configurable stub implementing marketplaceEC2Client. -// Each field, when set, overrides the corresponding method's behaviour and +// Each field, when set, overrides the corresponding method's behavior and // records the last request so tests can assert on what was sent to AWS. type stubMarketplaceEC2 struct { createFn func(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) cancelFn func(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) lastCreateReq ec2svc.MarketplaceListingRequest + createCallCount int cancelCallCount int lastCancelID string } func (s *stubMarketplaceEC2) CreateMarketplaceListing(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { + s.createCallCount++ s.lastCreateReq = req if s.createFn != nil { return s.createFn(ctx, req) } - return ec2svc.MarketplaceListingResult{ListingID: "ril-default", State: "active"}, nil + return ec2svc.MarketplaceListingResult{ListingID: "ril-default", State: config.ListingStateActive}, nil } func (s *stubMarketplaceEC2) DescribeMarketplaceListing(_ context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) { - return ec2svc.MarketplaceListingResult{ListingID: listingID, State: "active"}, nil + return ec2svc.MarketplaceListingResult{ListingID: listingID, State: config.ListingStateActive}, nil } func (s *stubMarketplaceEC2) CancelMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) { @@ -51,7 +53,7 @@ func (s *stubMarketplaceEC2) CancelMarketplaceListing(ctx context.Context, listi if s.cancelFn != nil { return s.cancelFn(ctx, listingID) } - return ec2svc.MarketplaceListingResult{ListingID: listingID, State: "cancelled"}, nil + return ec2svc.MarketplaceListingResult{ListingID: listingID, State: config.ListingStateCancelled}, nil } // marketplaceTestAPIError is an smithy.APIError with a controllable fault so @@ -152,7 +154,9 @@ func TestMarketplaceList_SellOwnAllowed(t *testing.T) { cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(standardRow(), nil) - cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", "active"). + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", config.ListingStateActive). Return(nil) h := newMarketplaceHandler(cfgStore, authSvc, ec2) @@ -203,7 +207,7 @@ func TestMarketplaceList_DuplicateActiveListing409(t *testing.T) { adminSession(authSvc) row := standardRow() - row.ListingState = "active" + row.ListingState = config.ListingStateActive row.ListingID = "ril-existing" cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(row, nil) @@ -224,6 +228,11 @@ func TestMarketplaceList_AWSClientFaultMapsTo400(t *testing.T) { adminSession(authSvc) cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(standardRow(), nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) + // AWS create fails, so the claim must be released back to the unlisted state. + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "", ""). + Return(nil) ec2 := &stubMarketplaceEC2{ createFn: func(_ context.Context, _ ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { @@ -249,6 +258,11 @@ func TestMarketplaceList_AWSServerFaultMapsTo502(t *testing.T) { adminSession(authSvc) cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(standardRow(), nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) + // AWS create fails, so the claim must be released back to the unlisted state. + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "", ""). + Return(nil) ec2 := &stubMarketplaceEC2{ createFn: func(_ context.Context, _ ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { @@ -273,6 +287,11 @@ func TestMarketplaceList_UnknownErrorMapsTo502(t *testing.T) { adminSession(authSvc) cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(standardRow(), nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) + // AWS create fails, so the claim must be released back to the unlisted state. + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "", ""). + Return(nil) ec2 := &stubMarketplaceEC2{ createFn: func(_ context.Context, _ ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) { @@ -295,9 +314,15 @@ func TestMarketplaceList_DBFailureCompensatingRollback(t *testing.T) { adminSession(authSvc) cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(standardRow(), nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) // DB persist fails after the listing was created. - cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", "active"). + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", config.ListingStateActive). Return(errors.New("db down")) + // After the compensating cancel, the claim is released back to unlisted so + // the row is not left stuck in the pending state. + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "", ""). + Return(nil) ec2 := &stubMarketplaceEC2{} h := newMarketplaceHandler(cfgStore, authSvc, ec2) @@ -335,11 +360,11 @@ func TestMarketplaceCancel_HappyPath(t *testing.T) { adminSession(authSvc) row := standardRow() - row.ListingState = "active" + row.ListingState = config.ListingStateActive row.ListingID = "ril-cancel" cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). Return(row, nil) - cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-cancel", "cancelled"). + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-cancel", config.ListingStateCancelled). Return(nil) ec2 := &stubMarketplaceEC2{} @@ -349,7 +374,7 @@ func TestMarketplaceCancel_HappyPath(t *testing.T) { require.NoError(t, err) m, ok := resp.(map[string]string) require.True(t, ok) - assert.Equal(t, "cancelled", m["listing_state"]) + assert.Equal(t, config.ListingStateCancelled, m["listing_state"]) assert.Equal(t, "ril-cancel", ec2.lastCancelID) cfgStore.AssertExpectations(t) } @@ -371,3 +396,57 @@ func TestMarketplaceCancel_NoActiveListing409(t *testing.T) { require.True(t, ok) assert.Equal(t, 409, ce.code) } + +// TestMarketplaceList_ConcurrentClaimLoses409 reproduces the concurrent-create +// race (issue #292 CR): the pre-flight listing_state read passes, but a parallel +// request has already reserved the listing slot, so the atomic claim loses. The +// handler must return 409 and must NOT call AWS (creating a second live listing +// for one RI would be a real double-listing on the money path). This test fails +// on the pre-guard code, which called AWS unconditionally after the read-check. +func TestMarketplaceList_ConcurrentClaimLoses409(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + // A concurrent request already holds the slot: the atomic claim reports lost. + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(false, nil) + + ec2 := &stubMarketplaceEC2{} + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + // The guard must fire before AWS: no listing may be created when the claim loses. + assert.Equal(t, 0, ec2.createCallCount) + cfgStore.AssertExpectations(t) +} + +// TestMarketplaceList_ClaimErrorMapsToInternal verifies a claim DB error is +// surfaced (not swallowed) and blocks the AWS call, so a broken slot-reservation +// never silently proceeds to create an unguarded listing. +func TestMarketplaceList_ClaimErrorMapsToInternal(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(standardRow(), nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(false, errors.New("db down")) + + ec2 := &stubMarketplaceEC2{} + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reserve marketplace listing slot") + // A failed claim must not fall through to AWS. + assert.Equal(t, 0, ec2.createCallCount) + cfgStore.AssertExpectations(t) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index e63458d86..99aa659d1 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -217,6 +217,18 @@ type StoreInterface interface { // on subsequent poll/cancel transitions (issue #292). UpdatePurchaseHistoryListing(ctx context.Context, purchaseID, listingID, listingState string) error + // ClaimMarketplaceListingSlot atomically reserves the marketplace-listing + // slot for a purchase_history row so two concurrent marketplace-list + // requests cannot both proceed to create a duplicate AWS listing (issue + // #292). It transitions listing_state to ListingStatePending only when the + // row is not already listed or mid-listing, and reports whether this call + // won the claim: (true, nil) means the caller reserved the slot and must + // then persist the real listing on success or release the slot back to its + // prior state on failure; (false, nil) means another request already holds + // an active or pending listing (the caller maps this to a 409). Modeled on + // FlipPurchaseRevocationInFlight. + ClaimMarketplaceListingSlot(ctx context.Context, purchaseID string) (bool, error) + // RI Exchange history SaveRIExchangeRecord(ctx context.Context, record *RIExchangeRecord) error GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index fe1344e70..94a38b44d 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1675,8 +1675,8 @@ func (s *PostgresStore) SavePurchaseHistory(ctx context.Context, record *Purchas // UpdatePurchaseHistoryListing stamps the marketplace listing fields onto a // purchase_history row identified by its purchase_id (RI reservation ID). // Called by the marketplace-list handler after CreateReservedInstancesListing -// succeeds (listing_id + listing_state="active") and by the status-poll path -// when AWS reports closed or cancelled. +// succeeds (listing_id + listing_state="active") and by the status-poll path on +// the later closed/canceled transitions. func (s *PostgresStore) UpdatePurchaseHistoryListing(ctx context.Context, purchaseID, listingID, listingState string) error { query := ` UPDATE purchase_history @@ -1693,6 +1693,28 @@ func (s *PostgresStore) UpdatePurchaseHistoryListing(ctx context.Context, purcha return nil } +// ClaimMarketplaceListingSlot atomically reserves the marketplace-listing slot +// for a purchase_history row so two concurrent marketplace-list requests cannot +// both proceed to create a duplicate AWS listing (issue #292). The single +// conditional UPDATE transitions listing_state to ListingStatePending only when +// the row is not already active or pending, so exactly one racing request wins. +// It returns (true, nil) when this call reserved the slot and (false, nil) when +// the row is already active/pending (or absent); the caller maps false to a 409. +func (s *PostgresStore) ClaimMarketplaceListingSlot(ctx context.Context, purchaseID string) (bool, error) { + query := ` + UPDATE purchase_history + SET listing_state = $1 + WHERE purchase_id = $2 + AND lower(COALESCE(listing_state, '')) NOT IN ($3, $4) + ` + tag, err := s.db.Exec(ctx, query, + ListingStatePending, purchaseID, ListingStateActive, ListingStatePending) + if err != nil { + return false, fmt.Errorf("failed to claim marketplace listing slot for purchase %s: %w", purchaseID, err) + } + return tag.RowsAffected() == 1, nil +} + // GetPurchaseHistory retrieves purchase history for an account. func (s *PostgresStore) GetPurchaseHistory(ctx context.Context, accountID string, limit int) ([]PurchaseHistoryRecord, error) { query := ` @@ -1942,7 +1964,7 @@ func (s *PostgresStore) queryPurchaseHistory(ctx context.Context, query string, // revocation columns from issue #290, then the marketplace columns from #292). // purchaseHistoryNullables holds the nullable scan targets shared by every // purchase_history reader (scanPurchaseHistoryRow, GetPurchaseHistoryByPurchaseID). -// Centralising the NULL reconciliation in applyTo keeps each reader's +// Centralizing the NULL reconciliation in applyTo keeps each reader's // cyclomatic complexity under the gocyclo budget as columns accrue across // issues #290 (revocation) and #292 (marketplace). type purchaseHistoryNullables struct { diff --git a/internal/config/types.go b/internal/config/types.go index f6091a787..79e783fdd 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -797,12 +797,25 @@ type PurchaseHistoryRecord struct { // CreateReservedInstancesListing. Empty when the RI has not been // listed. Persisted in purchase_history via migration 000068. ListingID string `json:"listing_id,omitempty" dynamodbav:"listing_id,omitempty"` - // ListingState mirrors the AWS marketplace listing state: "active", - // "cancelled", or "closed" (sold). Empty when not listed. - // Persisted in purchase_history via migration 000068. + // ListingState mirrors the AWS marketplace listing state (see the + // ListingState* constants). Empty when not listed. Persisted in + // purchase_history via migration 000068. ListingState string `json:"listing_state,omitempty" dynamodbav:"listing_state,omitempty"` } +// AWS EC2 ReservedInstancesListing status values, mirroring the +// ec2types.ListingStatus enum. Stored verbatim in purchase_history.listing_state +// so the strings must match AWS exactly. ListingStatePending is additionally +// written by the marketplace-list handler as a transient claim that guards +// against concurrent listing creation (issue #292); the other three come +// straight from AWS. +const ( + ListingStateActive = "active" + ListingStatePending = "pending" + ListingStateCancelled = "cancelled" //nolint:misspell // AWS ListingStatus enum literal, not prose + ListingStateClosed = "closed" +) + // RIExchangeRecord represents a record in the ri_exchange_history table. type RIExchangeRecord struct { ID string `json:"id"` diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index e77f64397..52d590648 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -449,6 +449,12 @@ func (m *MockConfigStore) UpdatePurchaseHistoryListing(ctx context.Context, purc return args.Error(0) } +// ClaimMarketplaceListingSlot mocks the atomic listing-slot claim (issue #292). +func (m *MockConfigStore) ClaimMarketplaceListingSlot(ctx context.Context, purchaseID string) (bool, error) { + args := m.Called(ctx, purchaseID) + return args.Bool(0), args.Error(1) +} + func (m *MockConfigStore) SaveRIExchangeRecord(ctx context.Context, record *config.RIExchangeRecord) error { args := m.Called(ctx, record) return args.Error(0) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 6f89dd151..906a12908 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -371,3 +371,6 @@ func (m *mockConfigStoreForHealth) UpdateGlobalConfigAtomic(_ context.Context, a func (m *mockConfigStoreForHealth) UpdatePurchaseHistoryListing(_ context.Context, _, _, _ string) error { return nil } +func (m *mockConfigStoreForHealth) ClaimMarketplaceListingSlot(_ context.Context, _ string) (bool, error) { + return true, nil +} From 0d038b1b47d3cb9aa86ffdd6bf81aa863ce2aefd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 23:36:21 +0300 Subject: [PATCH 19/21] fix(marketplace): resolve 3 blocking defects in #808 RI sell flow Defect 1 -- migration number collision: Renumber marketplace migration from 000068 (already deployed, skipped by golang-migrate on prod) to 000084 (next free after main's 000083). Verified: main tops at 000083; #1277 uses 000082; no open PR holds 000084. All "000068" references in SQL comments and types.go updated to 000084. Defect 2 -- offering_class never written: CUDly's EC2 client hardwires OfferingClassTypeConvertible; savePurchaseHistory never stamped the field, so every row landed with offering_class=NULL and the Sell button never rendered. Fix: (a) stamp offering_class='convertible' in savePurchaseHistory for all AWS EC2 purchases so future CUDly-bought rows are correctly classified at write time; (b) add FetchOfferingClass to the marketplaceEC2Client interface and implement it via DescribeReservedInstances so the marketplace-list handler can lazily populate offering_class for pre-migration rows and for externally-created Standard RIs (purchased before CUDly existed); (c) add StampOfferingClass to ConfigStore + PostgresStore + mock to persist the fetched class so subsequent requests skip the extra AWS call; (d) a new populateOfferingClass helper in the handler ties it together -- it runs when offering_class is empty after validateMarketplaceListRequest and before the definitive "standard only" gate. How a 'standard' row comes to exist: an externally-created Standard RI will get its offering_class populated from AWS DescribeReservedInstances on the first POST .../marketplace-list call and the value persisted for future calls. Rows purchased by CUDly are stamped 'convertible' at write time (CUDly only ever buys Convertible EC2 RIs). Defect 3 -- $0 price guardrail gap: resolveMarketplacePriceSchedule accepted Price >= 0, allowing zero-dollar listings. Fix: reject Price <= 0 with a clear error; add a named constant awsMarketplaceMinPriceFloorFraction (5%) and a total-schedule floor check so a schedule that sums to less than 5% of the prorated residual value is also rejected with an explicit message. Extract floor check into checkSuppliedScheduleFloor and the class-check predicate into isKnownNonStandardOfferingClass to keep all touched functions below gocyclo 10. Regression tests: - TestMigration084_MarketplaceColumns: integration test proves the three new columns exist and round-trip after migrating a fresh DB through 000084. - TestMarketplaceList_EmptyOfferingClassFetchedStandard: proves an externally-created Standard RI (offering_class="") is listable after the lazy-populate path fetches and stamps 'standard' from AWS. - TestMarketplaceList_EmptyOfferingClassFetchedConvertible: proves the gate still rejects when AWS reports 'convertible' even if DB had no class. - TestResolveMarketplacePriceSchedule_ZeroPriceRejected: Price=0 rejected. - TestResolveMarketplacePriceSchedule_BelowFloorRejected: sub-floor rejected. --- internal/analytics/collector_test.go | 4 + internal/api/handler_marketplace.go | 101 +++++++++++++++- internal/api/handler_marketplace_test.go | 113 +++++++++++++++++- internal/config/interfaces.go | 8 ++ internal/config/store_postgres.go | 14 +++ internal/config/types.go | 6 +- ...hase_history_marketplace_listing.down.sql} | 0 ...rchase_history_marketplace_listing.up.sql} | 2 +- ...rchase_history_marketplace_listing_test.go | 66 ++++++++++ internal/mocks/stores.go | 6 + internal/purchase/execution.go | 8 ++ internal/server/test_helpers_test.go | 4 + providers/aws/services/ec2/client.go | 20 ++++ 13 files changed, 342 insertions(+), 10 deletions(-) rename internal/database/postgres/migrations/{000068_purchase_history_marketplace_listing.down.sql => 000084_purchase_history_marketplace_listing.down.sql} (100%) rename internal/database/postgres/migrations/{000068_purchase_history_marketplace_listing.up.sql => 000084_purchase_history_marketplace_listing.up.sql} (94%) create mode 100644 internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 517f12864..a988e7a87 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -438,6 +438,10 @@ func (m *mockConfigStore) ClaimMarketplaceListingSlot(_ context.Context, _ strin return true, nil } +func (m *mockConfigStore) StampOfferingClass(_ context.Context, _, _ string) error { + return nil +} + // strPtr is a test helper for *string fields. func strPtr(s string) *string { return &s } diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 455feb506..a1b4d4335 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -38,6 +38,11 @@ type marketplaceEC2Client interface { CreateMarketplaceListing(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) DescribeMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) CancelMarketplaceListing(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) + // FetchOfferingClass calls AWS DescribeReservedInstances to determine + // whether a given Reserved Instance is 'standard' or 'convertible'. Used + // when the purchase_history row has no offering_class stored (pre-migration + // 000084 rows and externally-created Standard RIs). + FetchOfferingClass(ctx context.Context, reservedInstancesID string) (string, error) } // buildMarketplaceEC2Client honors the injected factory for tests, falling @@ -100,8 +105,10 @@ func (h *Handler) validateMarketplaceListRequest(ctx context.Context, req *event return nil, body, NewClientError(404, "purchase not found") } - // Only Standard RIs can be listed on the Marketplace. - if !strings.EqualFold(row.OfferingClass, "standard") { + // Reject explicitly non-standard classes before the RBAC call (fast path). + // Empty offering_class is allowed here: the listing handler populates it + // lazily from AWS before performing the definitive check. + if isKnownNonStandardOfferingClass(row.OfferingClass) { return nil, body, NewClientError(400, "only Standard Reserved Instances can be listed on the AWS Marketplace; this purchase has offering_class="+row.OfferingClass) } @@ -158,6 +165,23 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio ec2Client := h.buildMarketplaceEC2Client(cfg) + // Populate offering_class from AWS when the DB row has none. This covers + // two cases: (1) pre-migration 000084 rows purchased by CUDly before this + // column existed, and (2) externally-created Standard RIs whose + // offering_class was never stamped. After population the value is persisted + // so subsequent requests do not require an extra AWS API call. + offeringClass := row.OfferingClass + if offeringClass == "" { + offeringClass, err = h.populateOfferingClass(ctx, purchaseID, ec2Client) + if err != nil { + return nil, err + } + } + // Definitive offering_class gate: only Standard RIs can be listed. + if !strings.EqualFold(offeringClass, "standard") { + return nil, NewClientError(400, "only Standard Reserved Instances can be listed on the AWS Marketplace; this purchase has offering_class="+offeringClass) + } + awsSchedule := make([]ec2svc.MarketplacePriceTier, 0, len(schedule)) for _, t := range schedule { awsSchedule = append(awsSchedule, ec2svc.MarketplacePriceTier{ @@ -244,6 +268,25 @@ func (h *Handler) reserveAndCreateListing(ctx context.Context, purchaseID string return result, nil } +// populateOfferingClass calls AWS DescribeReservedInstances to determine the +// offering class for an RI whose purchase_history row lacks one, then persists +// the result so subsequent requests are served from the DB. Returns the class +// string on success; returns an error if AWS reports no such RI. +// +// The DB stamp is best-effort: a write failure is logged but does not block the +// listing request because the in-memory class value is authoritative for this +// request. +func (h *Handler) populateOfferingClass(ctx context.Context, purchaseID string, ec2Client marketplaceEC2Client) (string, error) { + class, err := ec2Client.FetchOfferingClass(ctx, purchaseID) + if err != nil { + return "", fmt.Errorf("offering_class not set and could not fetch from AWS: %w", err) + } + if stampErr := h.config.StampOfferingClass(ctx, purchaseID, class); stampErr != nil { + logging.Errorf("marketplace: failed to persist offering_class %q for purchase %s: %v", class, purchaseID, stampErr) + } + return class, nil +} + // releaseMarketplaceClaim restores a purchase_history row's listing fields to // the state captured before ClaimMarketplaceListingSlot reserved the slot. It // runs on every failure path after a successful claim so a failed listing @@ -419,6 +462,16 @@ const awsMarketplaceNetFactor = 1 - awsMarketplaceFeePercent/100.0 // seller's remaining cost basis. Applied before AWS deducts its fee. const awsMarketplaceBuyerDiscountFactor = 0.95 +// awsMarketplaceMinPriceFloorFraction is the minimum ratio of prorated +// residual value that a caller-supplied price schedule must total across its +// tiers. At 5%, a schedule totalling less than 1/20 of what the default +// schedule would offer is rejected with a 400 so a user cannot accidentally +// (or maliciously) list an RI at $0 or a nominal amount. The default schedule +// uses awsMarketplaceBuyerDiscountFactor (95%) of residual; this floor is +// intentionally far below that to give sellers flexibility while preventing +// zero-price listings. +const awsMarketplaceMinPriceFloorFraction = 0.05 + // awsMarketplaceClientFaultCodes is the set of AWS error codes that represent // client-side faults for Marketplace listing operations. These map to 4xx // responses so the caller receives an actionable message. Server-side AWS @@ -462,16 +515,56 @@ func mapAWSMarketplaceError(opMsg string, err error) error { // term field, which would overprice older RIs). // originalTerm is the full contract term in months, used to prorate the // upfront cost to its remaining value. +// 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. +func isKnownNonStandardOfferingClass(class string) bool { + return class != "" && !strings.EqualFold(class, "standard") +} + +// checkSuppliedScheduleFloor returns a non-nil error when the total value of +// a caller-supplied price schedule falls below awsMarketplaceMinPriceFloorFraction +// of the computed residual value. Extracted from resolveMarketplacePriceSchedule +// to keep that function within the gocyclo budget. The check is skipped when +// residual is zero (fully-elapsed term or $0 RI) to avoid spurious rejections. +func checkSuppliedScheduleFloor(tiers []MarketplacePriceTier, remainingMonths, originalTerm int, upfrontCost, monthlyCost float64) error { + if remainingMonths <= 0 { + return nil + } + upfrontRemaining := 0.0 + if originalTerm > 0 { + upfrontRemaining = upfrontCost * (float64(remainingMonths) / float64(originalTerm)) + } + residual := upfrontRemaining + monthlyCost*float64(remainingMonths) + if residual <= 0 { + return nil + } + floor := residual * awsMarketplaceMinPriceFloorFraction + var total float64 + for _, t := range tiers { + total += t.Price * float64(t.TermMonths) + } + if total < floor { + return fmt.Errorf( + "total listing price (%.2f) is below the minimum floor (%.2f, %.0f%% of residual value %.2f); raise your price schedule or omit it to use the default", + total, floor, awsMarketplaceMinPriceFloorFraction*100, residual) + } + return nil +} + func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths, originalTerm int, upfrontCost, monthlyCost float64) ([]MarketplacePriceTier, error) { if len(supplied) > 0 { for i, t := range supplied { if t.TermMonths <= 0 { return nil, fmt.Errorf("price_schedule[%d]: term_months must be a positive integer", i) } - if t.Price < 0 { - return nil, fmt.Errorf("price_schedule[%d]: price must be non-negative", i) + if t.Price <= 0 { + return nil, fmt.Errorf("price_schedule[%d]: price must be positive (received %.4f); use a non-zero listing price", i, t.Price) } } + if err := checkSuppliedScheduleFloor(supplied, remainingMonths, originalTerm, upfrontCost, monthlyCost); err != nil { + return nil, err + } return supplied, nil } diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index 6f19c1f5a..b5373f1a6 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -25,8 +25,9 @@ const validMarketplacePurchaseID = "11111111-1111-1111-1111-111111111111" // Each field, when set, overrides the corresponding method's behavior and // records the last request so tests can assert on what was sent to AWS. type stubMarketplaceEC2 struct { - createFn func(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) - cancelFn func(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) + createFn func(ctx context.Context, req ec2svc.MarketplaceListingRequest) (ec2svc.MarketplaceListingResult, error) + cancelFn func(ctx context.Context, listingID string) (ec2svc.MarketplaceListingResult, error) + fetchClassFn func(ctx context.Context, reservedInstancesID string) (string, error) lastCreateReq ec2svc.MarketplaceListingRequest createCallCount int @@ -56,6 +57,14 @@ func (s *stubMarketplaceEC2) CancelMarketplaceListing(ctx context.Context, listi return ec2svc.MarketplaceListingResult{ListingID: listingID, State: config.ListingStateCancelled}, nil } +func (s *stubMarketplaceEC2) FetchOfferingClass(ctx context.Context, reservedInstancesID string) (string, error) { + if s.fetchClassFn != nil { + return s.fetchClassFn(ctx, reservedInstancesID) + } + // Default: return "standard" so tests that set offering_class="" still proceed. + return "standard", nil +} + // marketplaceTestAPIError is an smithy.APIError with a controllable fault so // the error-mapping tests can exercise both the client- and server-fault paths. type marketplaceTestAPIError struct { @@ -450,3 +459,103 @@ func TestMarketplaceList_ClaimErrorMapsToInternal(t *testing.T) { assert.Equal(t, 0, ec2.createCallCount) cfgStore.AssertExpectations(t) } + +// --- Regression tests for the three blocking defects fixed in this PR --- + +// TestResolveMarketplacePriceSchedule_ZeroPriceRejected verifies Defect 3: +// Price=0 must be rejected with a 400 so a user cannot list an RI for free. +// This is the fail-before/pass-after regression test (the old code accepted +// Price >= 0, so Price=0 was accepted; the fix requires Price > 0). +func TestResolveMarketplacePriceSchedule_ZeroPriceRejected(t *testing.T) { + _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 6, Price: 0}, + }, 6, 12, 1200, 0) + require.Error(t, err, "Price=0 must be rejected") + assert.Contains(t, err.Error(), "positive") +} + +// TestResolveMarketplacePriceSchedule_BelowFloorRejected verifies Defect 3: +// A schedule whose total value is below awsMarketplaceMinPriceFloorFraction of +// the residual must be rejected. The fixture has residual=$1200 (upfront $1200, +// no monthly, remaining=12 of 12 months) so the floor at 5% is $60; a price of +// $0.01/month for 12 months totals $0.12 and must be rejected. +func TestResolveMarketplacePriceSchedule_BelowFloorRejected(t *testing.T) { + _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 12, Price: 0.01}, + }, 12, 12, 1200, 0) + require.Error(t, err, "sub-floor schedule must be rejected") + assert.Contains(t, err.Error(), "floor") + assert.Contains(t, err.Error(), "residual") +} + +// TestMarketplaceList_EmptyOfferingClassFetchedStandard verifies Defect 2: +// When purchase_history.offering_class is empty (pre-migration rows and +// externally-created RIs), the handler fetches the class from AWS, persists +// it, and proceeds when AWS reports "standard". This is the sell path becoming +// reachable for externally-created Standard RIs. +func TestMarketplaceList_EmptyOfferingClassFetchedStandard(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + // Row has no offering_class (simulates pre-migration or externally-created RI). + row := standardRow() + row.OfferingClass = "" + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + // The handler must stamp the fetched class back to the DB. + cfgStore.On("StampOfferingClass", mock.Anything, validMarketplacePurchaseID, "standard"). + Return(nil) + cfgStore.On("ClaimMarketplaceListingSlot", mock.Anything, validMarketplacePurchaseID). + Return(true, nil) + cfgStore.On("UpdatePurchaseHistoryListing", mock.Anything, validMarketplacePurchaseID, "ril-default", config.ListingStateActive). + Return(nil) + + // AWS reports this RI is "standard". + ec2 := &stubMarketplaceEC2{ + fetchClassFn: func(_ context.Context, id string) (string, error) { + assert.Equal(t, validMarketplacePurchaseID, id) + return "standard", nil + }, + } + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + resp, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.NoError(t, err, "standard RI with empty offering_class must be listable after lazy-populate") + typed, ok := resp.(*MarketplaceListResponse) + require.True(t, ok) + assert.Equal(t, "ril-default", typed.ListingID) + cfgStore.AssertExpectations(t) +} + +// TestMarketplaceList_EmptyOfferingClassFetchedConvertible verifies Defect 2 +// guard: when AWS reports the RI is "convertible", the handler must reject +// with 400 even if offering_class was empty in the DB (not silently proceed). +func TestMarketplaceList_EmptyOfferingClassFetchedConvertible(t *testing.T) { + cfgStore := &MockConfigStore{} + authSvc := &MockAuthService{} + adminSession(authSvc) + + row := standardRow() + row.OfferingClass = "" + cfgStore.On("GetPurchaseHistoryByPurchaseID", mock.Anything, validMarketplacePurchaseID). + Return(row, nil) + // The handler stamps the fetched class back even when it is convertible. + cfgStore.On("StampOfferingClass", mock.Anything, validMarketplacePurchaseID, "convertible"). + Return(nil) + + ec2 := &stubMarketplaceEC2{ + fetchClassFn: func(_ context.Context, _ string) (string, error) { + return "convertible", nil + }, + } + h := newMarketplaceHandler(cfgStore, authSvc, ec2) + _, err := h.marketplaceList(context.Background(), marketplaceReq(), validMarketplacePurchaseID) + + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "Standard") + cfgStore.AssertExpectations(t) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 99aa659d1..abb7a395c 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -217,6 +217,14 @@ type StoreInterface interface { // on subsequent poll/cancel transitions (issue #292). UpdatePurchaseHistoryListing(ctx context.Context, purchaseID, listingID, listingState string) error + // StampOfferingClass writes the offering_class value to a purchase_history + // row identified by purchase_id. Called by the marketplace-list handler + // when offering_class is absent in the DB (pre-migration 000084 rows and + // externally-created Standard RIs): after fetching the class from AWS + // DescribeReservedInstances it is persisted so subsequent requests do not + // incur an extra AWS API call. + StampOfferingClass(ctx context.Context, purchaseID, offeringClass string) error + // ClaimMarketplaceListingSlot atomically reserves the marketplace-listing // slot for a purchase_history row so two concurrent marketplace-list // requests cannot both proceed to create a duplicate AWS listing (issue diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 94a38b44d..a046b17f8 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1693,6 +1693,20 @@ func (s *PostgresStore) UpdatePurchaseHistoryListing(ctx context.Context, purcha return nil } +// StampOfferingClass writes the offering_class value onto a purchase_history +// row identified by purchase_id. Used by the marketplace-list handler to +// lazily persist offering_class fetched from AWS DescribeReservedInstances for +// rows created before migration 000084 or for externally-created Standard RIs. +// A no-match (row not found) is treated as a non-fatal warning by callers. +func (s *PostgresStore) StampOfferingClass(ctx context.Context, purchaseID, offeringClass string) error { + query := `UPDATE purchase_history SET offering_class = $1 WHERE purchase_id = $2` + _, err := s.db.Exec(ctx, query, offeringClass, purchaseID) + if err != nil { + return fmt.Errorf("failed to stamp offering_class for purchase %s: %w", purchaseID, err) + } + return nil +} + // ClaimMarketplaceListingSlot atomically reserves the marketplace-listing slot // for a purchase_history row so two concurrent marketplace-list requests cannot // both proceed to create a duplicate AWS listing (issue #292). The single diff --git a/internal/config/types.go b/internal/config/types.go index 79e783fdd..426d10797 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -791,15 +791,15 @@ type PurchaseHistoryRecord struct { // OfferingClass records whether this commitment is a 'standard' or // 'convertible' RI. NULL on pre-migration rows. The Sell-on-Marketplace // button renders only when this equals "standard" (issue #292). - // Persisted in purchase_history via migration 000068. + // Persisted in purchase_history via migration 000084. OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` // ListingID is the AWS ReservedInstancesListingId returned by // CreateReservedInstancesListing. Empty when the RI has not been - // listed. Persisted in purchase_history via migration 000068. + // listed. Persisted in purchase_history via migration 000084. ListingID string `json:"listing_id,omitempty" dynamodbav:"listing_id,omitempty"` // ListingState mirrors the AWS marketplace listing state (see the // ListingState* constants). Empty when not listed. Persisted in - // purchase_history via migration 000068. + // purchase_history via migration 000084. ListingState string `json:"listing_state,omitempty" dynamodbav:"listing_state,omitempty"` } diff --git a/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.down.sql similarity index 100% rename from internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.down.sql rename to internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.down.sql diff --git a/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.up.sql similarity index 94% rename from internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.up.sql rename to internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.up.sql index 839a885f9..853f16d69 100644 --- a/internal/database/postgres/migrations/000068_purchase_history_marketplace_listing.up.sql +++ b/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.up.sql @@ -1,4 +1,4 @@ --- Migration 000068: add RI Marketplace listing columns to purchase_history. +-- Migration 000084: add RI Marketplace listing columns to purchase_history. -- -- offering_class: 'convertible' or 'standard'; NULL for pre-migration rows. -- The Sell button renders only when offering_class = 'standard' and the RI is diff --git a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go b/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go new file mode 100644 index 000000000..bf2da9a58 --- /dev/null +++ b/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go @@ -0,0 +1,66 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMigration084_MarketplaceColumns verifies that migration 000084 creates +// the three new columns (offering_class, listing_id, listing_state) on the +// purchase_history table. The test migrates a fresh DB up through the full +// migration set and asserts that the columns are present and writable. +// +// Fail-before/pass-after: running this test against a DB at version 083 (without +// 000084 applied) would fail because the columns do not exist; after 000084 the +// test passes. +func TestMigration084_MarketplaceColumns(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping test: could not setup postgres container: %v", err) + } + defer container.Cleanup(ctx) + + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", ""), + "migrations must apply cleanly through 000084") + + // Verify all three columns exist by inserting a row that references them. + // We insert into the minimal required columns plus the three new ones to + // confirm the DDL was applied; if any column is missing Postgres will error. + const insertSQL = ` + INSERT INTO purchase_history ( + account_id, purchase_id, timestamp, provider, service, region, + resource_type, count, term, payment, upfront_cost, + estimated_savings, plan_id, plan_name, ramp_step, source, + offering_class, listing_id, listing_state + ) VALUES ( + 'test-account', 'ri-084-test', $1, 'aws', 'ec2', 'us-east-1', + 't3.micro', 1, 12, 'All Upfront', 100.0, + 10.0, 'plan-084', 'test plan', 0, 'test', + 'standard', 'ril-084-test', 'active' + )` + + _, err = container.DB.Pool().Exec(ctx, insertSQL, time.Now()) + require.NoError(t, err, "INSERT referencing offering_class, listing_id, listing_state must succeed after migration 000084") + + // Verify the values round-trip correctly. + var offeringClass, listingID, listingState string + row := container.DB.Pool().QueryRow(ctx, ` + SELECT offering_class, listing_id, listing_state + FROM purchase_history WHERE purchase_id = 'ri-084-test'`) + require.NoError(t, row.Scan(&offeringClass, &listingID, &listingState)) + assert.Equal(t, "standard", offeringClass, "offering_class must round-trip") + assert.Equal(t, "ril-084-test", listingID, "listing_id must round-trip") + assert.Equal(t, "active", listingState, "listing_state must round-trip") +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 52d590648..ed446b09a 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -449,6 +449,12 @@ func (m *MockConfigStore) UpdatePurchaseHistoryListing(ctx context.Context, purc return args.Error(0) } +// StampOfferingClass mocks persisting the offering_class fetched from AWS (issue #292). +func (m *MockConfigStore) StampOfferingClass(ctx context.Context, purchaseID, offeringClass string) error { + args := m.Called(ctx, purchaseID, offeringClass) + return args.Error(0) +} + // ClaimMarketplaceListingSlot mocks the atomic listing-slot claim (issue #292). func (m *MockConfigStore) ClaimMarketplaceListingSlot(ctx context.Context, purchaseID string) (bool, error) { args := m.Called(ctx, purchaseID) diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 89ebce2fd..41885b019 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -785,6 +785,14 @@ func (m *Manager) savePurchaseHistory(ctx context.Context, exec *config.Purchase // Revoke button (issue #290). Azure-only in Phase 1; nil for AWS/GCP. RevocationWindowClosesAt: config.RevocationWindowClosesAtFor(rec.Provider, purchasedAt), } + // Stamp offering_class for AWS EC2 Reserved Instances. CUDly always + // purchases EC2 RIs with offering-class=Convertible (the EC2 client + // hard-wires OfferingClassTypeConvertible). Stamping at write time + // ensures the marketplace-sell guard can distinguish eligibility without + // an extra AWS round-trip for CUDly-purchased rows (issue #292). + if rec.Provider == string(common.ProviderAWS) && rec.Service == string(common.ServiceEC2) { + historyRecord.OfferingClass = "convertible" + } if err := m.config.SavePurchaseHistory(ctx, historyRecord); err != nil { logging.Errorf("Failed to save history: %v", err) return err diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 906a12908..52618d416 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -374,3 +374,7 @@ func (m *mockConfigStoreForHealth) UpdatePurchaseHistoryListing(_ context.Contex func (m *mockConfigStoreForHealth) ClaimMarketplaceListingSlot(_ context.Context, _ string) (bool, error) { return true, nil } + +func (m *mockConfigStoreForHealth) StampOfferingClass(_ context.Context, _, _ string) error { + return nil +} diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 2b9ded74a..267bdec84 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -1042,6 +1042,26 @@ func (c *Client) DescribeMarketplaceListing(ctx context.Context, listingID strin return MarketplaceListingResult{ListingID: resolvedID, State: state}, nil } +// FetchOfferingClass calls ec2:DescribeReservedInstances to fetch the +// offering_class ("standard" or "convertible") for a specific Reserved +// Instance ID. Used by the marketplace-list handler to lazily populate +// offering_class for rows created before migration 000084 or for externally- +// created (pre-CUDly) Standard RIs whose offering_class was never stamped. +// Returns an error when the RI is not found (e.g. it has expired or was +// exchanged) because a missing RI cannot be listed. +func (c *Client) FetchOfferingClass(ctx context.Context, reservedInstancesID string) (string, error) { + out, err := c.client.DescribeReservedInstances(ctx, &ec2.DescribeReservedInstancesInput{ + ReservedInstancesIds: []string{reservedInstancesID}, + }) + if err != nil { + return "", fmt.Errorf("DescribeReservedInstances(%s): %w", reservedInstancesID, err) + } + if len(out.ReservedInstances) == 0 { + return "", fmt.Errorf("reserved instance %s not found (may have expired or been exchanged)", reservedInstancesID) + } + return string(out.ReservedInstances[0].OfferingClass), nil +} + // CancelMarketplaceListing calls ec2:CancelReservedInstancesListing to // withdraw an active listing. Returns the updated listing state. func (c *Client) CancelMarketplaceListing(ctx context.Context, listingID string) (MarketplaceListingResult, error) { From 2dc0735ad6d400be5a727a5e534d9f6fc27a2420 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 23:46:44 +0300 Subject: [PATCH 20/21] fix(marketplace): renumber migration 000084 -> 000085 to avoid #1277 collision PR #1277 (cancelled->canceled rename) was concurrently renumbered to 000084 and merges before #808. Once it lands, main will hold 000084, so #808's 000084 would collide on the merge ref (pre-commit --all-files migration check) and give golang-migrate two version-84 migrations. Since #1277 takes the lower number and merges first, #808 moves to 000085. - git mv the up/down/test migration files 000084_* -> 000085_* - update the "-- Migration" header in the up.sql and "-- Revert migration" in the down.sql to 000085 - update every 000084 reference in comments: config/types.go (x3), config/interfaces.go, config/store_postgres.go, api/handler_marketplace.go (x2), providers/aws/services/ec2/client.go - rename TestMigration084_MarketplaceColumns -> TestMigration085_* and its fixture identifiers (ri-085-test, ril-085-test) Also fix the integration-test fixture surfaced by running it against a real testcontainer PG: plan_id is a UUID FK, so the literal 'plan-085' failed with SQLSTATE 22P02. The INSERT now lists only NOT-NULL-without-default columns plus the three new marketplace columns and omits plan_id/plan_name/ramp_step/ source (nullable or defaulted), so the test needs no purchase_plans fixture. Verified: migrations apply cleanly through version 85 and the three columns round-trip. --- internal/api/handler_marketplace.go | 4 +-- internal/config/interfaces.go | 2 +- internal/config/store_postgres.go | 2 +- internal/config/types.go | 6 ++-- ...hase_history_marketplace_listing.down.sql} | 2 +- ...rchase_history_marketplace_listing.up.sql} | 2 +- ...chase_history_marketplace_listing_test.go} | 32 +++++++++---------- providers/aws/services/ec2/client.go | 2 +- 8 files changed, 26 insertions(+), 26 deletions(-) rename internal/database/postgres/migrations/{000084_purchase_history_marketplace_listing.down.sql => 000085_purchase_history_marketplace_listing.down.sql} (84%) rename internal/database/postgres/migrations/{000084_purchase_history_marketplace_listing.up.sql => 000085_purchase_history_marketplace_listing.up.sql} (94%) rename internal/database/postgres/migrations/{000084_purchase_history_marketplace_listing_test.go => 000085_purchase_history_marketplace_listing_test.go} (66%) diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index a1b4d4335..40eb9bd0d 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -41,7 +41,7 @@ type marketplaceEC2Client interface { // FetchOfferingClass calls AWS DescribeReservedInstances to determine // whether a given Reserved Instance is 'standard' or 'convertible'. Used // when the purchase_history row has no offering_class stored (pre-migration - // 000084 rows and externally-created Standard RIs). + // 000085 rows and externally-created Standard RIs). FetchOfferingClass(ctx context.Context, reservedInstancesID string) (string, error) } @@ -166,7 +166,7 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio ec2Client := h.buildMarketplaceEC2Client(cfg) // Populate offering_class from AWS when the DB row has none. This covers - // two cases: (1) pre-migration 000084 rows purchased by CUDly before this + // two cases: (1) pre-migration 000085 rows purchased by CUDly before this // column existed, and (2) externally-created Standard RIs whose // offering_class was never stamped. After population the value is persisted // so subsequent requests do not require an extra AWS API call. diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index abb7a395c..542c914f2 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -219,7 +219,7 @@ type StoreInterface interface { // StampOfferingClass writes the offering_class value to a purchase_history // row identified by purchase_id. Called by the marketplace-list handler - // when offering_class is absent in the DB (pre-migration 000084 rows and + // when offering_class is absent in the DB (pre-migration 000085 rows and // externally-created Standard RIs): after fetching the class from AWS // DescribeReservedInstances it is persisted so subsequent requests do not // incur an extra AWS API call. diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index a046b17f8..b08bf08ea 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1696,7 +1696,7 @@ func (s *PostgresStore) UpdatePurchaseHistoryListing(ctx context.Context, purcha // StampOfferingClass writes the offering_class value onto a purchase_history // row identified by purchase_id. Used by the marketplace-list handler to // lazily persist offering_class fetched from AWS DescribeReservedInstances for -// rows created before migration 000084 or for externally-created Standard RIs. +// rows created before migration 000085 or for externally-created Standard RIs. // A no-match (row not found) is treated as a non-fatal warning by callers. func (s *PostgresStore) StampOfferingClass(ctx context.Context, purchaseID, offeringClass string) error { query := `UPDATE purchase_history SET offering_class = $1 WHERE purchase_id = $2` diff --git a/internal/config/types.go b/internal/config/types.go index 426d10797..641eb2e75 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -791,15 +791,15 @@ type PurchaseHistoryRecord struct { // OfferingClass records whether this commitment is a 'standard' or // 'convertible' RI. NULL on pre-migration rows. The Sell-on-Marketplace // button renders only when this equals "standard" (issue #292). - // Persisted in purchase_history via migration 000084. + // Persisted in purchase_history via migration 000085. OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` // ListingID is the AWS ReservedInstancesListingId returned by // CreateReservedInstancesListing. Empty when the RI has not been - // listed. Persisted in purchase_history via migration 000084. + // listed. Persisted in purchase_history via migration 000085. ListingID string `json:"listing_id,omitempty" dynamodbav:"listing_id,omitempty"` // ListingState mirrors the AWS marketplace listing state (see the // ListingState* constants). Empty when not listed. Persisted in - // purchase_history via migration 000084. + // purchase_history via migration 000085. ListingState string `json:"listing_state,omitempty" dynamodbav:"listing_state,omitempty"` } diff --git a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.down.sql similarity index 84% rename from internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.down.sql rename to internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.down.sql index 0ed453c6a..394681092 100644 --- a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.down.sql +++ b/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.down.sql @@ -1,4 +1,4 @@ --- Revert migration 000068 +-- Revert migration 000085 ALTER TABLE purchase_history DROP COLUMN IF EXISTS offering_class, DROP COLUMN IF EXISTS listing_id, diff --git a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.up.sql similarity index 94% rename from internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.up.sql rename to internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.up.sql index 853f16d69..44bf049af 100644 --- a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing.up.sql +++ b/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.up.sql @@ -1,4 +1,4 @@ --- Migration 000084: add RI Marketplace listing columns to purchase_history. +-- Migration 000085: add RI Marketplace listing columns to purchase_history. -- -- offering_class: 'convertible' or 'standard'; NULL for pre-migration rows. -- The Sell button renders only when offering_class = 'standard' and the RI is diff --git a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go b/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing_test.go similarity index 66% rename from internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go rename to internal/database/postgres/migrations/000085_purchase_history_marketplace_listing_test.go index bf2da9a58..5e5ba413a 100644 --- a/internal/database/postgres/migrations/000084_purchase_history_marketplace_listing_test.go +++ b/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing_test.go @@ -14,15 +14,15 @@ import ( "github.com/stretchr/testify/require" ) -// TestMigration084_MarketplaceColumns verifies that migration 000084 creates +// TestMigration085_MarketplaceColumns verifies that migration 000085 creates // the three new columns (offering_class, listing_id, listing_state) on the // purchase_history table. The test migrates a fresh DB up through the full // migration set and asserts that the columns are present and writable. // -// Fail-before/pass-after: running this test against a DB at version 083 (without -// 000084 applied) would fail because the columns do not exist; after 000084 the +// Fail-before/pass-after: running this test against a DB at version 084 (without +// 000085 applied) would fail because the columns do not exist; after 000085 the // test passes. -func TestMigration084_MarketplaceColumns(t *testing.T) { +func TestMigration085_MarketplaceColumns(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() @@ -33,34 +33,34 @@ func TestMigration084_MarketplaceColumns(t *testing.T) { defer container.Cleanup(ctx) require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", ""), - "migrations must apply cleanly through 000084") + "migrations must apply cleanly through 000085") // Verify all three columns exist by inserting a row that references them. - // We insert into the minimal required columns plus the three new ones to - // confirm the DDL was applied; if any column is missing Postgres will error. + // We insert the NOT-NULL-without-default columns plus the three new + // marketplace columns; if any of the new columns is missing Postgres will + // error. plan_id is intentionally omitted (nullable UUID FK to + // purchase_plans) so the test needs no plan fixture. const insertSQL = ` INSERT INTO purchase_history ( account_id, purchase_id, timestamp, provider, service, region, - resource_type, count, term, payment, upfront_cost, - estimated_savings, plan_id, plan_name, ramp_step, source, + resource_type, term, payment, offering_class, listing_id, listing_state ) VALUES ( - 'test-account', 'ri-084-test', $1, 'aws', 'ec2', 'us-east-1', - 't3.micro', 1, 12, 'All Upfront', 100.0, - 10.0, 'plan-084', 'test plan', 0, 'test', - 'standard', 'ril-084-test', 'active' + 'test-account', 'ri-085-test', $1, 'aws', 'ec2', 'us-east-1', + 't3.micro', 12, 'All Upfront', + 'standard', 'ril-085-test', 'active' )` _, err = container.DB.Pool().Exec(ctx, insertSQL, time.Now()) - require.NoError(t, err, "INSERT referencing offering_class, listing_id, listing_state must succeed after migration 000084") + require.NoError(t, err, "INSERT referencing offering_class, listing_id, listing_state must succeed after migration 000085") // Verify the values round-trip correctly. var offeringClass, listingID, listingState string row := container.DB.Pool().QueryRow(ctx, ` SELECT offering_class, listing_id, listing_state - FROM purchase_history WHERE purchase_id = 'ri-084-test'`) + FROM purchase_history WHERE purchase_id = 'ri-085-test'`) require.NoError(t, row.Scan(&offeringClass, &listingID, &listingState)) assert.Equal(t, "standard", offeringClass, "offering_class must round-trip") - assert.Equal(t, "ril-084-test", listingID, "listing_id must round-trip") + assert.Equal(t, "ril-085-test", listingID, "listing_id must round-trip") assert.Equal(t, "active", listingState, "listing_state must round-trip") } diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 267bdec84..94cb6f2f2 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -1045,7 +1045,7 @@ func (c *Client) DescribeMarketplaceListing(ctx context.Context, listingID strin // FetchOfferingClass calls ec2:DescribeReservedInstances to fetch the // offering_class ("standard" or "convertible") for a specific Reserved // Instance ID. Used by the marketplace-list handler to lazily populate -// offering_class for rows created before migration 000084 or for externally- +// offering_class for rows created before migration 000085 or for externally- // created (pre-CUDly) Standard RIs whose offering_class was never stamped. // Returns an error when the RI is not found (e.g. it has expired or was // exchanged) because a missing RI cannot be listed. From 1ae517590296fa79bccbdbaed94be4fbc9debc2b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 01:51:16 +0300 Subject: [PATCH 21/21] fix(marketplace): correct RI listing price-floor math and reachability Round-3 money-path review fixes for the Sell-on-Marketplace flow (#808). Price floor (was arithmetically wrong): - Enforce the floor PER TIER on the one-time sale Price. AWS PriceScheduleSpecification.Price is the lump-sum a buyer pays when TermMonths remain, not a per-month rate; the old code compared Price*TermMonths against the floor, over-crediting a cheap schedule up to ~term-fold so a $5 tier passed a $60 floor on a $1,200 residual. - Reject any tier whose TermMonths exceeds the RI's remaining months (was unvalidated and arbitrarily inflatable). - Per-instance basis: purchase_history.UpfrontCost is the row total for all Count instances, but Marketplace prices are per instance, so divide the residual by Count in both the floor and the default schedule. - Upfront-only residual: drop recurring (monthly) cost from the residual basis. In the RI Marketplace the buyer assumes recurring charges after transfer, so the seller recovers only the upfront remainder; including recurring overpriced partial-upfront RIs and made no-upfront RIs unpriceable. - Extract marketplaceResidualPerUnit as the shared per-unit basis so the default schedule and the floor cannot drift. Reachability: render the Sell button for completed AWS EC2 rows whose offering_class is empty (unknown), not just "standard". CUDly stamps "convertible" on its own EC2 purchases and externally-created Standard RIs arrive with an empty class until the backend lazily populates it; gating the UI on "standard" alone made the feature unreachable end to end. Provider and service are now checked so unknown-class non-EC2 rows never show the button. Nits: - Use SDK enum constants string(ec2types.OfferingClassType{Standard,Convertible}) instead of raw "standard"/"convertible" literals in the offering-class gate, the isKnownNonStandardOfferingClass predicate, and the purchase-history stamp. - Route populateOfferingClass errors through mapAWSMarketplaceError so an AWS client fault (e.g. InvalidReservedInstancesID.NotFound) surfaces as a 400, not a generic 500. Migration: renumber 000085 -> 000087 to stay above #1422's 000086 (main tops at pre-84; #1422 takes 000086, so this PR takes 000087). Regression tests (fail-before / pass-after verified): - TestResolveMarketplacePriceSchedule_BelowFloorRejected: a {12mo, $5} tier on a $1,200 per-unit residual (Count=1) is now rejected; passed under the old Price*TermMonths floor. - TestResolveMarketplacePriceSchedule_PerUnitFloor: on a $1,200 row-total with Count=3 the per-unit floor is $20, so $19 is rejected and $21 accepted. - TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected: a tier term beyond the remaining months is rejected. - history-marketplace-sell-button.test.ts: the Sell button renders for a completed AWS EC2 row with an empty offering_class and stays hidden for convertible, non-EC2, non-AWS, active-listing, and anonymous cases. - TestMigration087_MarketplaceColumns: fresh DB migrates cleanly through 000087 and the three columns round-trip. --- .../history-marketplace-sell-button.test.ts | 234 ++++++++++++++++++ frontend/src/history.ts | 16 +- internal/api/handler_marketplace.go | 164 +++++++----- internal/api/handler_marketplace_test.go | 77 ++++-- internal/config/interfaces.go | 2 +- internal/config/store_postgres.go | 2 +- internal/config/types.go | 6 +- ...hase_history_marketplace_listing.down.sql} | 2 +- ...rchase_history_marketplace_listing.up.sql} | 2 +- ...chase_history_marketplace_listing_test.go} | 20 +- internal/purchase/execution.go | 3 +- providers/aws/services/ec2/client.go | 2 +- 12 files changed, 430 insertions(+), 100 deletions(-) create mode 100644 frontend/src/__tests__/history-marketplace-sell-button.test.ts rename internal/database/postgres/migrations/{000085_purchase_history_marketplace_listing.down.sql => 000087_purchase_history_marketplace_listing.down.sql} (84%) rename internal/database/postgres/migrations/{000085_purchase_history_marketplace_listing.up.sql => 000087_purchase_history_marketplace_listing.up.sql} (94%) rename internal/database/postgres/migrations/{000085_purchase_history_marketplace_listing_test.go => 000087_purchase_history_marketplace_listing_test.go} (79%) diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts new file mode 100644 index 000000000..9da8ae1b1 --- /dev/null +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -0,0 +1,234 @@ +/** + * History inline "Sell on Marketplace" button tests (issue #292, FINDING D). + * + * Regression guard for the "dead Sell button" gap: canSellOnMarketplace used to + * require offering_class === 'standard', but nothing ever wrote a 'standard' + * row end-to-end -- CUDly stamps 'convertible' on its own EC2 purchases, and + * externally-created Standard RIs arrive with an EMPTY offering_class until the + * backend lazily populates it from AWS on the list call. So the button never + * rendered for the very case the feature targets. + * + * The fix shows the Sell button for completed AWS EC2 rows whose offering_class + * is 'standard' OR still unknown (empty), and lets the backend + * populateOfferingClass + gate (it 400s a fetched 'convertible') decide. The + * backend authorizeSessionSell remains the real security boundary; these tests + * only verify the UX gate. + * + * Tested matrix: + * 1. completed aws/ec2 row, offering_class 'standard' -> shown. + * 2. completed aws/ec2 row, offering_class EMPTY (unknown) -> shown (fix). + * 3. completed aws/ec2 row, offering_class 'convertible' -> hidden. + * 4. completed aws/ec2 row with an active listing -> hidden. + * 5. completed aws/rds row, offering_class EMPTY -> hidden. + * 6. completed azure/compute row, offering_class EMPTY -> hidden. + * 7. anonymous (no current user cached) -> hidden. + */ + +import { loadHistory } from '../history'; + +jest.mock('../api', () => ({ + getHistory: jest.fn(), + createMarketplaceListing: jest.fn(), + cancelMarketplaceListing: jest.fn(), +})); + +jest.mock('../navigation', () => ({ + switchTab: jest.fn(), +})); + +jest.mock('../utils', () => ({ + formatCurrency: jest.fn((val) => `$${val || 0}`), + formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), + formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), + escapeHtml: jest.fn((str) => str || ''), + escapeHtmlAttr: jest.fn((str) => str || ''), + amortizedMonthly: jest.fn((monthly) => monthly), + populateAccountFilter: jest.fn(() => Promise.resolve()), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn(), +})); + +jest.mock('../toast', () => ({ + showToast: jest.fn(), +})); + +jest.mock('../state', () => ({ + getCurrentUser: jest.fn(), + getCurrentProvider: jest.fn().mockReturnValue(''), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), + getAmortizeUpfront: jest.fn().mockReturnValue(false), + setAmortizeUpfront: jest.fn(), + subscribeAmortizeUpfront: jest.fn().mockReturnValue(() => {}), +})); + +import * as api from '../api'; +import { getCurrentUser } from '../state'; + +// Administrators group GUID -- mirrors ADMINISTRATORS_GROUP_ID in +// frontend/src/permissions.ts. Without this, isAdmin() returns false and +// canAccess('admin', '*') in canSellOnMarketplace's fallback branch does not +// grant. Seeded-group GUID, not the label string. +const ADMIN_GROUP_ID = '00000000-0000-5000-8000-000000000001'; +const ADMIN_USER = { id: 'admin-uuid', email: 'admin@example.com', groups: [ADMIN_GROUP_ID] }; + +// A recent purchase with plenty of term left so the remaining-months guard in +// canSellOnMarketplace (needs >= 1 month remaining) always passes. +const RECENT = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000).toISOString(); // -30 days + +function setupDOM(): void { + while (document.body.firstChild) document.body.removeChild(document.body.firstChild); + + const mkInput = (id: string): HTMLInputElement => { + const el = document.createElement('input'); + el.type = 'date'; + el.id = id; + return el; + }; + const mkSelect = (id: string): HTMLSelectElement => { + const el = document.createElement('select'); + el.id = id; + const opt = document.createElement('option'); + opt.value = ''; + opt.textContent = 'All'; + el.appendChild(opt); + return el; + }; + const mkDiv = (id: string): HTMLDivElement => { + const el = document.createElement('div'); + el.id = id; + return el; + }; + + document.body.appendChild(mkInput('history-start')); + document.body.appendChild(mkInput('history-end')); + document.body.appendChild(mkSelect('history-provider-filter')); + document.body.appendChild(mkSelect('history-account-filter')); + document.body.appendChild(mkDiv('history-summary')); + document.body.appendChild(mkDiv('history-list')); + document.body.appendChild(mkDiv('purchases-approval-queue')); +} + +// A completed AWS EC2 RI row with a long remaining term. Overrides tune the +// offering_class / provider / service / listing_state for each case. +function makeRow(overrides: Record) { + return { + purchase_id: 'ri-1', + timestamp: RECENT, + provider: 'aws', + service: 'ec2', + resource_type: 't3.large', + region: 'us-east-1', + count: 1, + term: 36, + upfront_cost: 1200, + estimated_savings: 300, + plan_name: '', + status: 'completed', + offering_class: 'standard', + ...overrides, + }; +} + +function sellIds(): (string | undefined)[] { + const list = document.getElementById('history-list')!; + const buttons = list.querySelectorAll('.history-marketplace-sell-btn'); + return Array.from(buttons).map((b) => b.dataset['marketplaceSellId']); +} + +describe('History inline Sell on Marketplace button (issue #292)', () => { + beforeEach(() => { + setupDOM(); + jest.clearAllMocks(); + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + }); + + test('shows Sell for a completed AWS EC2 row with offering_class=standard', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'ri-standard', offering_class: 'standard' })], + }); + + await loadHistory(); + + expect(sellIds()).toEqual(['ri-standard']); + }); + + test('shows Sell for a completed AWS EC2 row with EMPTY offering_class (the reachability fix)', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'ri-unknown', offering_class: '' })], + }); + + await loadHistory(); + + // The backend lazily populates + gates; the UI must surface the button so + // an externally-created Standard RI is actually listable end-to-end. + expect(sellIds()).toEqual(['ri-unknown']); + }); + + test('hides Sell for a known convertible RI', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'ri-convertible', offering_class: 'convertible' })], + }); + + await loadHistory(); + + expect(sellIds()).toEqual([]); + }); + + test('hides Sell when a listing is already active (Cancel is shown instead)', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'ri-active', offering_class: 'standard', listing_state: 'active', listing_id: 'ril-1' }), + ], + }); + + await loadHistory(); + + expect(sellIds()).toEqual([]); + }); + + test('hides Sell for a non-EC2 AWS row even when offering_class is unknown', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'rds-unknown', service: 'rds', offering_class: '' })], + }); + + await loadHistory(); + + expect(sellIds()).toEqual([]); + }); + + test('hides Sell for a non-AWS row even when offering_class is unknown', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [ + makeRow({ purchase_id: 'az-unknown', provider: 'azure', service: 'compute', offering_class: '' }), + ], + }); + + await loadHistory(); + + expect(sellIds()).toEqual([]); + }); + + test('hides Sell when no user is cached (anonymous)', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(null); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'ri-anon', offering_class: '' })], + }); + + await loadHistory(); + + expect(sellIds()).toEqual([]); + }); +}); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 75508b3f8..0bd0c160c 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -609,7 +609,14 @@ function retryThresholdReached(p: HistoryPurchase): boolean { // // Conditions: // * row must be a completed purchase (status "completed" or absent); -// * offering_class must be "standard"; +// * row must be an AWS EC2 RI (marketplace is EC2-only); +// * offering_class must be "standard" OR still unknown (empty). CUDly stamps +// "convertible" on its own EC2 purchases, but externally-created Standard +// RIs (and pre-migration rows) have an empty offering_class until the +// backend lazily populates it from AWS on the list call. The backend +// definitively gates -- it 400s a fetched "convertible" -- so we show the +// button for unknown-class EC2 rows and let the backend decide, otherwise +// the feature is unreachable end-to-end for the very case it targets; // * no active listing already (listing_state != "active"); and // * admin, or non-admin user (sell-own covers their own accounts -- // we can't efficiently check per-account ownership client-side, so @@ -618,7 +625,12 @@ function retryThresholdReached(p: HistoryPurchase): boolean { function canSellOnMarketplace(p: HistoryPurchase): boolean { const status = normalizeStatus(p).toLowerCase(); if (status !== 'completed') return false; - if ((p.offering_class || '').toLowerCase() !== 'standard') return false; + // Marketplace listing is AWS EC2 Standard-RI only. Gate on provider/service + // so unknown-class rows for non-EC2 providers never show the Sell button. + if ((p.provider || '').toLowerCase() !== 'aws') return false; + if ((p.service || '').toLowerCase() !== 'ec2') return false; + const offeringClass = (p.offering_class || '').toLowerCase(); + if (offeringClass !== 'standard' && offeringClass !== '') return false; if ((p.listing_state || '').toLowerCase() === 'active') return false; const user = getCurrentUser(); if (!user) return false; diff --git a/internal/api/handler_marketplace.go b/internal/api/handler_marketplace.go index 40eb9bd0d..fb0afd296 100644 --- a/internal/api/handler_marketplace.go +++ b/internal/api/handler_marketplace.go @@ -28,6 +28,7 @@ import ( ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" "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" smithy "github.com/aws/smithy-go" "github.com/google/uuid" ) @@ -41,7 +42,7 @@ type marketplaceEC2Client interface { // FetchOfferingClass calls AWS DescribeReservedInstances to determine // whether a given Reserved Instance is 'standard' or 'convertible'. Used // when the purchase_history row has no offering_class stored (pre-migration - // 000085 rows and externally-created Standard RIs). + // 000087 rows and externally-created Standard RIs). FetchOfferingClass(ctx context.Context, reservedInstancesID string) (string, error) } @@ -92,7 +93,8 @@ func (h *Handler) validateMarketplaceListRequest(ctx context.Context, req *event return nil, body, err } - if err := validateUUID(purchaseID); err != nil { + err = validateUUID(purchaseID) + if err != nil { return nil, body, err } @@ -145,14 +147,11 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio // rather than the full contract term (which overprices older RIs). remainingMonths := computeRemainingMonths(row.Timestamp, row.Term) - // Validate and normalise the price schedule. A nil MonthlyCost means the - // provider recorded no recurring breakdown (issue #258 made the field - // nullable); treat absent as zero recurring contribution to residual value. - monthlyCost := 0.0 - if row.MonthlyCost != nil { - monthlyCost = *row.MonthlyCost - } - schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, row.Term, row.UpfrontCost, monthlyCost) + // Validate and normalise the price schedule. Row-total UpfrontCost and the + // instance Count are passed so pricing is computed per instance; recurring + // (monthly) cost is deliberately excluded because the Marketplace buyer + // assumes recurring charges post-transfer (issue #292 money-path review). + schedule, err := resolveMarketplacePriceSchedule(body.PriceSchedule, remainingMonths, row.Term, row.Count, row.UpfrontCost) if err != nil { return nil, NewClientError(400, err.Error()) } @@ -166,7 +165,7 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio ec2Client := h.buildMarketplaceEC2Client(cfg) // Populate offering_class from AWS when the DB row has none. This covers - // two cases: (1) pre-migration 000085 rows purchased by CUDly before this + // two cases: (1) pre-migration 000087 rows purchased by CUDly before this // column existed, and (2) externally-created Standard RIs whose // offering_class was never stamped. After population the value is persisted // so subsequent requests do not require an extra AWS API call. @@ -178,7 +177,7 @@ func (h *Handler) marketplaceList(ctx context.Context, req *events.LambdaFunctio } } // Definitive offering_class gate: only Standard RIs can be listed. - if !strings.EqualFold(offeringClass, "standard") { + if !strings.EqualFold(offeringClass, string(ec2types.OfferingClassTypeStandard)) { return nil, NewClientError(400, "only Standard Reserved Instances can be listed on the AWS Marketplace; this purchase has offering_class="+offeringClass) } @@ -279,7 +278,11 @@ func (h *Handler) reserveAndCreateListing(ctx context.Context, purchaseID string func (h *Handler) populateOfferingClass(ctx context.Context, purchaseID string, ec2Client marketplaceEC2Client) (string, error) { class, err := ec2Client.FetchOfferingClass(ctx, purchaseID) if err != nil { - return "", fmt.Errorf("offering_class not set and could not fetch from AWS: %w", err) + // Route through mapAWSMarketplaceError so an AWS client fault (e.g. an + // InvalidReservedInstancesID.NotFound for an RI that has expired or been + // exchanged) surfaces as a 400, not a generic 500. Server-side AWS + // failures still map to 502. + return "", mapAWSMarketplaceError("offering_class not set and could not fetch from AWS", err) } if stampErr := h.config.StampOfferingClass(ctx, purchaseID, class); stampErr != nil { logging.Errorf("marketplace: failed to persist offering_class %q for purchase %s: %v", class, purchaseID, stampErr) @@ -306,7 +309,8 @@ func (h *Handler) marketplaceCancel(ctx context.Context, req *events.LambdaFunct return nil, err } - if err := validateUUID(purchaseID); err != nil { + err = validateUUID(purchaseID) + if err != nil { return nil, err } @@ -318,7 +322,8 @@ func (h *Handler) marketplaceCancel(ctx context.Context, req *events.LambdaFunct return nil, NewClientError(404, "purchase not found") } - if err := h.authorizeSessionSell(ctx, session, row.CloudAccountID); err != nil { + err = h.authorizeSessionSell(ctx, session, row.CloudAccountID) + if err != nil { return nil, err } @@ -503,56 +508,92 @@ func mapAWSMarketplaceError(opMsg string, err error) error { return NewClientError(502, opMsg+": "+err.Error()) } -// resolveMarketplacePriceSchedule returns a normalised price schedule for the -// given RI. When the caller supplied an explicit schedule it is validated and -// returned unchanged. When the caller omitted the schedule (nil / empty), a -// single-tier default is computed: (upfront_remaining + future_recurring) * -// awsMarketplaceBuyerDiscountFactor (5% discount to attract buyers; the -// awsMarketplaceFeePercent% AWS fee is applied by the Marketplace on top). -// -// remainingMonths must be the actual remaining months (computed via -// computeRemainingMonths from purchase timestamp and total term -- NOT the raw -// term field, which would overprice older RIs). -// originalTerm is the full contract term in months, used to prorate the -// upfront cost to its remaining value. // 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. func isKnownNonStandardOfferingClass(class string) bool { - return class != "" && !strings.EqualFold(class, "standard") + return class != "" && !strings.EqualFold(class, string(ec2types.OfferingClassTypeStandard)) +} + +// marketplaceResidualPerUnit returns the prorated per-instance upfront residual +// of an RI: the still-unconsumed portion of the upfront cost, divided by the +// instance count. This is the seller's recoverable basis on the RI Marketplace. +// +// Two money-path invariants (issue #292 review) are baked in here so every +// caller shares them: +// - Per instance: purchase_history.UpfrontCost is the row total for all +// `count` instances, but Marketplace prices are per instance, so divide by +// count. A legacy/corrupt count <= 0 is treated as 1 (matching +// reserveAndCreateListing's floor-at-1) so a single unit still prices. +// - Upfront only: recurring (monthly) cost is excluded because the buyer +// assumes recurring charges after transfer -- the seller recovers only the +// upfront remainder. Including it would overprice partial-upfront RIs and +// make no-upfront RIs unpriceable. +// +// Returns 0 when the term or upfront basis is non-positive (a $0 no-upfront RI +// or a fully-elapsed term), signaling callers to skip the per-unit price floor. +func marketplaceResidualPerUnit(remainingMonths, originalTerm, count int, upfrontCost float64) float64 { + if remainingMonths <= 0 || originalTerm <= 0 || upfrontCost <= 0 { + return 0 + } + units := count + if units <= 0 { + units = 1 + } + upfrontRemaining := upfrontCost * (float64(remainingMonths) / float64(originalTerm)) + return upfrontRemaining / float64(units) } -// checkSuppliedScheduleFloor returns a non-nil error when the total value of -// a caller-supplied price schedule falls below awsMarketplaceMinPriceFloorFraction -// of the computed residual value. Extracted from resolveMarketplacePriceSchedule -// to keep that function within the gocyclo budget. The check is skipped when -// residual is zero (fully-elapsed term or $0 RI) to avoid spurious rejections. -func checkSuppliedScheduleFloor(tiers []MarketplacePriceTier, remainingMonths, originalTerm int, upfrontCost, monthlyCost float64) error { +// checkSuppliedScheduleFloor validates a caller-supplied price schedule against +// two money-path guardrails (issue #292): +// 1. no tier's TermMonths may exceed the RI's remaining months -- an +// out-of-range term would inflate the tier's implied value arbitrarily; and +// 2. each tier's one-time sale Price must be at least +// awsMarketplaceMinPriceFloorFraction of the per-unit residual prorated to +// that tier's term. +// +// AWS's PriceScheduleSpecification.Price is the ONE-TIME sale price a buyer pays +// when TermMonths remain (NOT a per-month rate), so the floor is applied to +// Price directly -- never Price*TermMonths, which would over-credit a cheap +// schedule up to ~term-fold and let a near-zero listing slip past the floor. +// When the per-unit residual is zero (a $0 no-upfront RI) the price floor is +// skipped, but the term-range check still applies. +func checkSuppliedScheduleFloor(tiers []MarketplacePriceTier, remainingMonths, originalTerm, count int, upfrontCost float64) error { if remainingMonths <= 0 { return nil } - upfrontRemaining := 0.0 - if originalTerm > 0 { - upfrontRemaining = upfrontCost * (float64(remainingMonths) / float64(originalTerm)) - } - residual := upfrontRemaining + monthlyCost*float64(remainingMonths) - if residual <= 0 { - return nil - } - floor := residual * awsMarketplaceMinPriceFloorFraction - var total float64 - for _, t := range tiers { - total += t.Price * float64(t.TermMonths) - } - if total < floor { - return fmt.Errorf( - "total listing price (%.2f) is below the minimum floor (%.2f, %.0f%% of residual value %.2f); raise your price schedule or omit it to use the default", - total, floor, awsMarketplaceMinPriceFloorFraction*100, residual) + perUnit := marketplaceResidualPerUnit(remainingMonths, originalTerm, count, upfrontCost) + for i, t := range tiers { + if t.TermMonths > int64(remainingMonths) { + return fmt.Errorf( + "price_schedule[%d]: term_months (%d) exceeds the RI's remaining term (%d months)", + i, t.TermMonths, remainingMonths) + } + if perUnit <= 0 { + continue + } + proratedResidual := perUnit * (float64(t.TermMonths) / float64(remainingMonths)) + floor := proratedResidual * awsMarketplaceMinPriceFloorFraction + if t.Price < floor { + return fmt.Errorf( + "price_schedule[%d]: price (%.2f) is below the minimum floor (%.2f, %.0f%% of the %.2f per-unit residual for %d remaining months); raise the price or omit the schedule to use the default", + i, t.Price, floor, awsMarketplaceMinPriceFloorFraction*100, proratedResidual, t.TermMonths) + } } return nil } -func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths, originalTerm int, upfrontCost, monthlyCost float64) ([]MarketplacePriceTier, error) { +// resolveMarketplacePriceSchedule returns a normalised price schedule for the +// given RI. A caller-supplied schedule is validated (positive term/price, the +// term-range and per-unit price-floor guardrails in checkSuppliedScheduleFloor) +// and returned unchanged. When the caller omitted the schedule (nil / empty) a +// single-tier default is computed from the per-unit upfront residual. +// +// remainingMonths must be the actual remaining months (from computeRemainingMonths, +// NOT the raw term, which would overprice older RIs). originalTerm is the full +// contract term in months. count is the row's instance count (pricing is per +// instance). upfrontCost is the row-total upfront paid across all instances. +func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingMonths, originalTerm, count int, upfrontCost float64) ([]MarketplacePriceTier, error) { if len(supplied) > 0 { for i, t := range supplied { if t.TermMonths <= 0 { @@ -562,25 +603,22 @@ func resolveMarketplacePriceSchedule(supplied []MarketplacePriceTier, remainingM return nil, fmt.Errorf("price_schedule[%d]: price must be positive (received %.4f); use a non-zero listing price", i, t.Price) } } - if err := checkSuppliedScheduleFloor(supplied, remainingMonths, originalTerm, upfrontCost, monthlyCost); err != nil { + if err := checkSuppliedScheduleFloor(supplied, remainingMonths, originalTerm, count, upfrontCost); err != nil { return nil, err } return supplied, nil } - // Default: spread (upfront_remaining + future_recurring) * awsMarketplaceBuyerDiscountFactor - // across remaining term. The upfront cost is prorated by (remaining/original) to - // avoid overpricing older RIs (a 12-month RI at month 6 retains only half - // the upfront value; using the full amount would overprice by ~2x). + // Default: a single tier priced at the per-unit upfront residual discounted + // by awsMarketplaceBuyerDiscountFactor. Per instance (Count-divided) and + // upfront-only (recurring is the buyer's post-transfer obligation). The + // awsMarketplaceFeePercent% AWS fee is deducted by the Marketplace on top. + // The upfront cost is prorated by (remaining/original) so an older RI is not + // overpriced (a 12-month RI at month 6 retains only half its upfront value). if remainingMonths <= 0 { remainingMonths = 1 // defensive: should not happen for an active RI } - upfrontRemaining := 0.0 - if originalTerm > 0 { - upfrontRemaining = upfrontCost * (float64(remainingMonths) / float64(originalTerm)) - } - totalValue := upfrontRemaining + (monthlyCost * float64(remainingMonths)) - listPrice := totalValue * awsMarketplaceBuyerDiscountFactor + listPrice := marketplaceResidualPerUnit(remainingMonths, originalTerm, count, upfrontCost) * awsMarketplaceBuyerDiscountFactor if listPrice < 0 { listPrice = 0 } diff --git a/internal/api/handler_marketplace_test.go b/internal/api/handler_marketplace_test.go index b5373f1a6..c504b5ef0 100644 --- a/internal/api/handler_marketplace_test.go +++ b/internal/api/handler_marketplace_test.go @@ -346,21 +346,32 @@ func TestMarketplaceList_DBFailureCompensatingRollback(t *testing.T) { } func TestMarketplaceList_DefaultScheduleProrationMath(t *testing.T) { - // remaining=12, term=12, upfront=1200, monthly=0 => (1200*12/12 + 0)*0.95 = 1140. - schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1200, 0) + // remaining=12, term=12, count=1, upfront=1200 => per-unit residual + // 1200*12/12/1 = 1200; default price = 1200*0.95 = 1140. + schedule, err := resolveMarketplacePriceSchedule(nil, 12, 12, 1, 1200) require.NoError(t, err) require.Len(t, schedule, 1) assert.Equal(t, int64(12), schedule[0].TermMonths) assert.InDelta(t, 1140.0, schedule[0].Price, 0.001) - // Half the term elapsed: remaining=6, term=12, upfront=1200, monthly=10. - // upfront_remaining = 1200 * 6/12 = 600; recurring = 10*6 = 60; - // (600 + 60) * 0.95 = 627. - schedule, err = resolveMarketplacePriceSchedule(nil, 6, 12, 1200, 10) + // Half the term elapsed, count=1: remaining=6, term=12, upfront=1200. + // per-unit residual = 1200*6/12/1 = 600; recurring is excluded (the buyer + // assumes it post-transfer); default price = 600*0.95 = 570. + schedule, err = resolveMarketplacePriceSchedule(nil, 6, 12, 1, 1200) require.NoError(t, err) require.Len(t, schedule, 1) assert.Equal(t, int64(6), schedule[0].TermMonths) - assert.InDelta(t, 627.0, schedule[0].Price, 0.001) + assert.InDelta(t, 570.0, schedule[0].Price, 0.001) + + // Per-instance pricing: count=3 on the same $1200 row-total upfront divides + // the residual by the instance count, so the default per-unit price is a + // third of the count=1 price (1200/3=400 per-unit residual; 400*0.95=380), + // NOT the row-total (which would 3x-overprice each listed unit). + schedule, err = resolveMarketplacePriceSchedule(nil, 12, 12, 3, 1200) + require.NoError(t, err) + require.Len(t, schedule, 1) + assert.Equal(t, int64(12), schedule[0].TermMonths) + assert.InDelta(t, 380.0, schedule[0].Price, 0.001) } func TestMarketplaceCancel_HappyPath(t *testing.T) { @@ -469,25 +480,59 @@ func TestMarketplaceList_ClaimErrorMapsToInternal(t *testing.T) { func TestResolveMarketplacePriceSchedule_ZeroPriceRejected(t *testing.T) { _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ {TermMonths: 6, Price: 0}, - }, 6, 12, 1200, 0) + }, 6, 12, 1, 1200) require.Error(t, err, "Price=0 must be rejected") assert.Contains(t, err.Error(), "positive") } -// TestResolveMarketplacePriceSchedule_BelowFloorRejected verifies Defect 3: -// A schedule whose total value is below awsMarketplaceMinPriceFloorFraction of -// the residual must be rejected. The fixture has residual=$1200 (upfront $1200, -// no monthly, remaining=12 of 12 months) so the floor at 5% is $60; a price of -// $0.01/month for 12 months totals $0.12 and must be rejected. +// TestResolveMarketplacePriceSchedule_BelowFloorRejected verifies Defect A +// (money): the floor is enforced PER TIER on the one-time sale Price, not +// Price*TermMonths. A $5 one-time price on a $1,200 per-unit residual with 12 +// of 12 months remaining is 0.4% of residual and must be rejected (floor is 5% +// = $60). This FAILS on the pre-fix code, which multiplied Price*TermMonths +// (5*12 = $60) and compared against a $60 floor, letting it pass. func TestResolveMarketplacePriceSchedule_BelowFloorRejected(t *testing.T) { _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ - {TermMonths: 12, Price: 0.01}, - }, 12, 12, 1200, 0) - require.Error(t, err, "sub-floor schedule must be rejected") + {TermMonths: 12, Price: 5.0}, + }, 12, 12, 1, 1200) + require.Error(t, err, "a $5 one-time price on a $1,200 residual must be rejected by the per-tier floor") assert.Contains(t, err.Error(), "floor") assert.Contains(t, err.Error(), "residual") } +// TestResolveMarketplacePriceSchedule_PerUnitFloor verifies Defect B (money): +// the floor basis is PER INSTANCE. On a $1,200 row-total upfront with count=3, +// the per-unit residual is $400, so the 5% floor is $20/unit. A $19 one-time +// price is rejected; a $21 price is accepted -- proving the floor divides the +// row total by count instead of applying the $1,200 row total per unit. +func TestResolveMarketplacePriceSchedule_PerUnitFloor(t *testing.T) { + _, errBelow := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 12, Price: 19.0}, + }, 12, 12, 3, 1200) + require.Error(t, errBelow, "$19 is below the $20 per-unit floor (5%% of $400 per-unit residual)") + assert.Contains(t, errBelow.Error(), "per-unit residual") + + schedule, errAbove := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 12, Price: 21.0}, + }, 12, 12, 3, 1200) + require.NoError(t, errAbove, "$21 clears the $20 per-unit floor") + require.Len(t, schedule, 1) + assert.InDelta(t, 21.0, schedule[0].Price, 0.001) +} + +// TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected verifies the +// Defect A term-range guardrail: a tier whose TermMonths exceeds the RI's +// remaining months is rejected outright (an out-of-range term would let a +// caller arbitrarily inflate the tier's implied value). remaining=6, tier +// term=12 -> rejected. +func TestResolveMarketplacePriceSchedule_TermExceedsRemainingRejected(t *testing.T) { + _, err := resolveMarketplacePriceSchedule([]MarketplacePriceTier{ + {TermMonths: 12, Price: 1000}, + }, 6, 12, 1, 1200) + require.Error(t, err, "term_months exceeding remaining term must be rejected") + assert.Contains(t, err.Error(), "exceeds the RI's remaining term") +} + // TestMarketplaceList_EmptyOfferingClassFetchedStandard verifies Defect 2: // When purchase_history.offering_class is empty (pre-migration rows and // externally-created RIs), the handler fetches the class from AWS, persists diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 542c914f2..f2f726336 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -219,7 +219,7 @@ type StoreInterface interface { // StampOfferingClass writes the offering_class value to a purchase_history // row identified by purchase_id. Called by the marketplace-list handler - // when offering_class is absent in the DB (pre-migration 000085 rows and + // when offering_class is absent in the DB (pre-migration 000087 rows and // externally-created Standard RIs): after fetching the class from AWS // DescribeReservedInstances it is persisted so subsequent requests do not // incur an extra AWS API call. diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b08bf08ea..cff7b69c2 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1696,7 +1696,7 @@ func (s *PostgresStore) UpdatePurchaseHistoryListing(ctx context.Context, purcha // StampOfferingClass writes the offering_class value onto a purchase_history // row identified by purchase_id. Used by the marketplace-list handler to // lazily persist offering_class fetched from AWS DescribeReservedInstances for -// rows created before migration 000085 or for externally-created Standard RIs. +// rows created before migration 000087 or for externally-created Standard RIs. // A no-match (row not found) is treated as a non-fatal warning by callers. func (s *PostgresStore) StampOfferingClass(ctx context.Context, purchaseID, offeringClass string) error { query := `UPDATE purchase_history SET offering_class = $1 WHERE purchase_id = $2` diff --git a/internal/config/types.go b/internal/config/types.go index 641eb2e75..29b8c7ddf 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -791,15 +791,15 @@ type PurchaseHistoryRecord struct { // OfferingClass records whether this commitment is a 'standard' or // 'convertible' RI. NULL on pre-migration rows. The Sell-on-Marketplace // button renders only when this equals "standard" (issue #292). - // Persisted in purchase_history via migration 000085. + // Persisted in purchase_history via migration 000087. OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` // ListingID is the AWS ReservedInstancesListingId returned by // CreateReservedInstancesListing. Empty when the RI has not been - // listed. Persisted in purchase_history via migration 000085. + // listed. Persisted in purchase_history via migration 000087. ListingID string `json:"listing_id,omitempty" dynamodbav:"listing_id,omitempty"` // ListingState mirrors the AWS marketplace listing state (see the // ListingState* constants). Empty when not listed. Persisted in - // purchase_history via migration 000085. + // purchase_history via migration 000087. ListingState string `json:"listing_state,omitempty" dynamodbav:"listing_state,omitempty"` } diff --git a/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.down.sql b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing.down.sql similarity index 84% rename from internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.down.sql rename to internal/database/postgres/migrations/000087_purchase_history_marketplace_listing.down.sql index 394681092..66e09baa2 100644 --- a/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.down.sql +++ b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing.down.sql @@ -1,4 +1,4 @@ --- Revert migration 000085 +-- Revert migration 000087 ALTER TABLE purchase_history DROP COLUMN IF EXISTS offering_class, DROP COLUMN IF EXISTS listing_id, diff --git a/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.up.sql b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing.up.sql similarity index 94% rename from internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.up.sql rename to internal/database/postgres/migrations/000087_purchase_history_marketplace_listing.up.sql index 44bf049af..5daee4c42 100644 --- a/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing.up.sql +++ b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing.up.sql @@ -1,4 +1,4 @@ --- Migration 000085: add RI Marketplace listing columns to purchase_history. +-- Migration 000087: add RI Marketplace listing columns to purchase_history. -- -- offering_class: 'convertible' or 'standard'; NULL for pre-migration rows. -- The Sell button renders only when offering_class = 'standard' and the RI is diff --git a/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing_test.go b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go similarity index 79% rename from internal/database/postgres/migrations/000085_purchase_history_marketplace_listing_test.go rename to internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go index 5e5ba413a..0aa2609dc 100644 --- a/internal/database/postgres/migrations/000085_purchase_history_marketplace_listing_test.go +++ b/internal/database/postgres/migrations/000087_purchase_history_marketplace_listing_test.go @@ -14,15 +14,15 @@ import ( "github.com/stretchr/testify/require" ) -// TestMigration085_MarketplaceColumns verifies that migration 000085 creates +// TestMigration087_MarketplaceColumns verifies that migration 000087 creates // the three new columns (offering_class, listing_id, listing_state) on the // purchase_history table. The test migrates a fresh DB up through the full // migration set and asserts that the columns are present and writable. // -// Fail-before/pass-after: running this test against a DB at version 084 (without -// 000085 applied) would fail because the columns do not exist; after 000085 the +// Fail-before/pass-after: running this test against a DB at version 086 (without +// 000087 applied) would fail because the columns do not exist; after 000087 the // test passes. -func TestMigration085_MarketplaceColumns(t *testing.T) { +func TestMigration087_MarketplaceColumns(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) defer cancel() @@ -33,7 +33,7 @@ func TestMigration085_MarketplaceColumns(t *testing.T) { defer container.Cleanup(ctx) require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", ""), - "migrations must apply cleanly through 000085") + "migrations must apply cleanly through 000087") // Verify all three columns exist by inserting a row that references them. // We insert the NOT-NULL-without-default columns plus the three new @@ -46,21 +46,21 @@ func TestMigration085_MarketplaceColumns(t *testing.T) { resource_type, term, payment, offering_class, listing_id, listing_state ) VALUES ( - 'test-account', 'ri-085-test', $1, 'aws', 'ec2', 'us-east-1', + 'test-account', 'ri-087-test', $1, 'aws', 'ec2', 'us-east-1', 't3.micro', 12, 'All Upfront', - 'standard', 'ril-085-test', 'active' + 'standard', 'ril-087-test', 'active' )` _, err = container.DB.Pool().Exec(ctx, insertSQL, time.Now()) - require.NoError(t, err, "INSERT referencing offering_class, listing_id, listing_state must succeed after migration 000085") + require.NoError(t, err, "INSERT referencing offering_class, listing_id, listing_state must succeed after migration 000087") // Verify the values round-trip correctly. var offeringClass, listingID, listingState string row := container.DB.Pool().QueryRow(ctx, ` SELECT offering_class, listing_id, listing_state - FROM purchase_history WHERE purchase_id = 'ri-085-test'`) + FROM purchase_history WHERE purchase_id = 'ri-087-test'`) require.NoError(t, row.Scan(&offeringClass, &listingID, &listingState)) assert.Equal(t, "standard", offeringClass, "offering_class must round-trip") - assert.Equal(t, "ril-085-test", listingID, "listing_id must round-trip") + assert.Equal(t, "ril-087-test", listingID, "listing_id must round-trip") assert.Equal(t, "active", listingState, "listing_state must round-trip") } diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 41885b019..026f586e7 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -17,6 +17,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/provider" azureprovider "github.com/LeanerCloud/CUDly/providers/azure" gcpprovider "github.com/LeanerCloud/CUDly/providers/gcp" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/google/uuid" ) @@ -791,7 +792,7 @@ func (m *Manager) savePurchaseHistory(ctx context.Context, exec *config.Purchase // ensures the marketplace-sell guard can distinguish eligibility without // an extra AWS round-trip for CUDly-purchased rows (issue #292). if rec.Provider == string(common.ProviderAWS) && rec.Service == string(common.ServiceEC2) { - historyRecord.OfferingClass = "convertible" + historyRecord.OfferingClass = string(ec2types.OfferingClassTypeConvertible) } if err := m.config.SavePurchaseHistory(ctx, historyRecord); err != nil { logging.Errorf("Failed to save history: %v", err) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 94cb6f2f2..0e9af15e0 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -1045,7 +1045,7 @@ func (c *Client) DescribeMarketplaceListing(ctx context.Context, listingID strin // FetchOfferingClass calls ec2:DescribeReservedInstances to fetch the // offering_class ("standard" or "convertible") for a specific Reserved // Instance ID. Used by the marketplace-list handler to lazily populate -// offering_class for rows created before migration 000085 or for externally- +// offering_class for rows created before migration 000087 or for externally- // created (pre-CUDly) Standard RIs whose offering_class was never stamped. // Returns an error when the RI is not found (e.g. it has expired or was // exchanged) because a missing RI cannot be listed.