feat(marketplace): sell/cancel Standard RIs on AWS Marketplace (closes #292) - #808
Merged
Conversation
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds AWS RI Marketplace listing and cancellation support for Standard RI history rows. It updates permissions, persistence, EC2 client calls, API routes and handlers, and the History UI to create and cancel listings. ChangesAWS RI Marketplace Listing Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant HistoryUI
participant FrontendAPI
participant Backend
participant EC2
participant Store
User->>HistoryUI: Click Sell on Marketplace
HistoryUI->>FrontendAPI: createMarketplaceListing(purchaseId, priceSchedule)
FrontendAPI->>Backend: POST /api/purchases/{id}/marketplace-list
Backend->>Store: GetPurchaseHistoryByPurchaseID
Backend->>EC2: CreateReservedInstancesListing
Backend->>Store: UpdatePurchaseHistoryListing
Backend-->>FrontendAPI: listing response
FrontendAPI-->>HistoryUI: reload history
User->>HistoryUI: Click Cancel listing
HistoryUI->>FrontendAPI: cancelMarketplaceListing(purchaseId)
FrontendAPI->>Backend: POST /api/purchases/{id}/marketplace-cancel
Backend->>EC2: CancelReservedInstancesListing
Backend->>Store: UpdatePurchaseHistoryListing
Backend-->>FrontendAPI: cancellation response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/history.ts (1)
610-624:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let retry-lineage markup suppress marketplace actions.
Line 610 returns as soon as lineage exists, so a completed Standard RI with
retry_attempt_n > 0never reaches Lines 617-623. That hides Sell/Cancel for successful retry descendants, even though they're still eligible completed purchases.Suggested direction
- if (lineage.length > 0) { - return lineage.join(' '); - } - - // Completed Standard RI rows: offer Sell on Marketplace (issue `#292`). - // The button is placed here (after lineage links) so it does not - // interfere with the Retry / Approve / Cancel affordances above. - if (p.purchase_id) { - if (canCancelMarketplaceListing(p)) { - return `<button type="button" class="btn-link history-marketplace-cancel-btn" data-marketplace-cancel-id="${escapeHtml(p.purchase_id)}">Cancel listing ${escapeHtml(p.listing_id || '')}</button>`; - } - if (canSellOnMarketplace(p)) { - return `<button type="button" class="btn-link history-marketplace-sell-btn" data-marketplace-sell-id="${escapeHtml(p.purchase_id)}">Sell on Marketplace</button>`; - } - } + const trailingActions: string[] = []; + if (p.purchase_id) { + if (canCancelMarketplaceListing(p)) { + trailingActions.push( + `<button type="button" class="btn-link history-marketplace-cancel-btn" data-marketplace-cancel-id="${escapeHtml(p.purchase_id)}">Cancel listing ${escapeHtml(p.listing_id || '')}</button>`, + ); + } else if (canSellOnMarketplace(p)) { + trailingActions.push( + `<button type="button" class="btn-link history-marketplace-sell-btn" data-marketplace-sell-id="${escapeHtml(p.purchase_id)}">Sell on Marketplace</button>`, + ); + } + } + + if (lineage.length > 0 || trailingActions.length > 0) { + return [...lineage, ...trailingActions].join(' '); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/history.ts` around lines 610 - 624, The early return when lineage.length > 0 prevents the marketplace buttons from being rendered for completed retry descendants; update the logic in the function handling the history row so that when lineage exists you append (or merge) the marketplace button markup instead of returning immediately — locate the lineage handling and the marketplace block using lineage, p.purchase_id, canCancelMarketplaceListing, canSellOnMarketplace and escapeHtml; either move the marketplace block above the return or build a combined string (lineage.join(' ') + marketplaceMarkup) so Sell on Marketplace / Cancel listing buttons are included for eligible purchases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/history.ts`:
- Around line 914-943: The flow skips the required pricing modal: after the
initial confirmDialog in the '.history-marketplace-sell-btn' click handler, open
the RI pricing/pricing-schedule modal (e.g., call a new or existing
showMarketplacePricingModal / showPricingModal function) to display the RI
summary, default per-month price schedule and 12% fee breakdown and allow the
user to adjust/select schedule types (use the schedule types exposed by
frontend/src/api/purchases.ts via api.getPricingSchedules or similar). Only if
the user confirms the pricing modal proceed to call
api.createMarketplaceListing(id); preserve the existing UI state handling
(disable/enable sameRowActions(btn) and call loadHistory() on success) and
handle errors exactly as existing catch blocks do.
- Around line 474-496: canSellOnMarketplace currently omits the required
remaining-term check; update the function (canSellOnMarketplace(p:
HistoryPurchase)) to return false unless the purchase has at least one month
remaining before maturity by inspecting the appropriate remaining-term field on
p (e.g., p.remaining_term, p.remaining_months, or p.termRemaining depending on
the model) and parsing/coercing string values to numbers if needed; add this
gate alongside the existing status/offering_class/listing_state checks so
matured Standard RIs (remaining < 1) do not show the Sell button.
In `@internal/api/handler_marketplace.go`:
- Line 147: The code currently maps every AWS marketplace error to HTTP 502 via
NewClientError("AWS marketplace listing failed: "+err.Error()) which incorrectly
classifies client-side validation/precondition failures; update the handlers in
internal/api/handler_marketplace.go to inspect the returned AWS error (check for
awserr.Error or smithy.APIError / error with Code() and/or HTTPStatusCode()
methods) and map its actual HTTP-level or SDK error code to an appropriate
status (e.g., 4xx for validation/precondition/auth errors, 409 for conflict, 404
for not found, 401/403 for auth/perm, otherwise 502 for server-side), then call
NewClientError with that mapped status and the error message; apply the same
change to the other occurrence that currently returns 502 so client-correctable
errors are propagated with the correct 4xx status.
- Around line 151-156: When UpdatePurchaseHistoryListing(...) fails after AWS
listing creation (the block using purchaseID, result.ListingID, result.State),
do not silently return success; instead treat it as an error: attempt a
compensating rollback of the created listing (call the appropriate marketplace
delete/cleanup method for result.ListingID), log both the DB error and the
rollback outcome, and return an internal error to the API caller; apply the same
change for the other occurrence referenced (the block around lines 207-209) so
DB persistence failures never yield a successful response while external state
exists.
- Around line 117-118: The default listing price uses row.Term as the remaining
months, which overprices older reservations; instead compute remainingMonths as
the actual remaining term (e.g., row.RemainingTerm or row.Term - row.UsedMonths,
or derive from start/end dates and now) and pass that into
resolveMarketplacePriceSchedule when body.PriceSchedule is empty; update the
code around the schedule assignment (the call to resolveMarketplacePriceSchedule
and the local variable schedule) to use this computed remainingMonths and apply
the same fix to the other occurrence referenced (lines ~299-310).
In
`@internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql`:
- Around line 15-18: The migration for table purchase_history adds
offering_class, listing_id, and listing_state but omits required marketplace
columns needed for lifecycle persistence; update the ALTER TABLE statement in
the migration to also ADD COLUMN IF NOT EXISTS listed_at TIMESTAMP WITH TIME
ZONE, listing_price_schedule JSONB (or appropriate type),
listing_proceeds_received NUMERIC (or appropriate money type), and
listing_fee_paid NUMERIC so the poller/sale path (references: table
purchase_history and columns offering_class, listing_id, listing_state) can
persist listing timestamps, price schedules, proceeds, and fees; ensure
nullability and default behavior match existing schema conventions.
In `@providers/aws/services/ec2/client.go`:
- Around line 970-979: Ensure the code never returns an empty ListingID: when
building MarketplaceListingResult (using variables listingID and
listing.ReservedInstancesListingId and listing.Status) validate that
aws.ToString(listing.ReservedInstancesListingId) yields a non-empty string and,
if empty, return a clear error instead of MarketplaceListingResult{ListingID:
"", ...}; apply the same validation & error-return logic in the other similar
blocks (the code paths referenced around the other two snippets) so downstream
handlers never persist an empty ListingID.
---
Outside diff comments:
In `@frontend/src/history.ts`:
- Around line 610-624: The early return when lineage.length > 0 prevents the
marketplace buttons from being rendered for completed retry descendants; update
the logic in the function handling the history row so that when lineage exists
you append (or merge) the marketplace button markup instead of returning
immediately — locate the lineage handling and the marketplace block using
lineage, p.purchase_id, canCancelMarketplaceListing, canSellOnMarketplace and
escapeHtml; either move the marketplace block above the return or build a
combined string (lineage.join(' ') + marketplaceMarkup) so Sell on Marketplace /
Cancel listing buttons are included for eligible purchases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b216fad4-4a77-4b3b-be24-d77f8f3b76d4
📒 Files selected for processing (24)
frontend/src/api/index.tsfrontend/src/api/purchases.tsfrontend/src/history.tsfrontend/src/types.tsinternal/analytics/collector_test.gointernal/api/handler.gointernal/api/handler_marketplace.gointernal/api/mocks_test.gointernal/api/router.gointernal/auth/service_group_test.gointernal/auth/service_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sqlinternal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sqlinternal/mocks/stores.gointernal/purchase/mocks_test.gointernal/scheduler/scheduler_test.gointernal/server/test_helpers_test.goproviders/aws/services/ec2/client.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
Fixed the compile/type issues (missing MockEC2Client methods for the extended EC2API interface), addressed the 7 CR findings (6 were already in prior commits and verified against HEAD; the Describe/Cancel empty-listing-ID fallback is now applied), and resolved the pre-commit failures (gocyclo on marketplaceList via extracting validateMarketplaceListRequest, and the stale permissions.generated.ts codegen) in 03ab7f5. @coderabbitai review |
Contributor
|
(ᴗ˳ᴗ) ✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
All 8 CodeRabbit findings from the 2026-05-28 review are resolved in commits a3cfc89 to 03ab7f5. Verification summary: frontend/src/history.ts
internal/api/handler_marketplace.go
internal/database/postgres/migrations/000060_...
providers/aws/services/ec2/client.go
Tests: @coderabbitai review |
Contributor
|
(ᴗ˳ᴗ) ✅ Actions performedReview triggered.
|
Member
Author
|
All 7 CR findings from the 2026-05-28 review (plus the outside-diff lineage composability note) are resolved and confirmed in the 2026-06-01T19:33 incremental review. No new findings were raised in that review pass. Verification summary: 7 inline threads - all confirmed LGTM by CR at 2026-06-01T19:33:
Outside-diff finding (lineage composability - history.ts:610-624): already fixed in the same commit range. trailingActions array is built before lineage join at history.ts:624-635 so retry descendants with eligible listings get Sell/Cancel buttons. Build and tests pass: go build ./... clean, go test ./internal/api/... 1355 passed, go test ./providers/aws/services/ec2/... 39 passed. @coderabbitai review |
Contributor
|
(ᴗ˳ᴗ) ✅ Actions performedReview triggered.
|
Member
Author
|
Rebased on Conflict resolution:
Verification:
All previous CR findings from the 2026-05-28 pass remain addressed (no logic changes to marketplace handlers, history.ts, ec2 client, or migration). The outside-diff-range note ( @coderabbitai review |
Contributor
|
The clean build + 3292 tests passing + frontend Jest green across all touched packages is a solid confidence signal. Triggering a fresh incremental review now to cover the rebase commits. (ᴗ˳ᴗ) 🧠 Learnings used✅ Actions performedReview triggered.
|
Member
Author
|
Rebased The prior round's 7 CR findings were addressed in the branch history:
Note: @coderabbitai review |
Contributor
|
(ᴗ˳ᴗ) 🧠 Learnings used✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_marketplace.go`:
- Around line 401-412: The default price schedule is using the full upfrontCost
instead of the prorated remaining upfront; update
resolveMarketplacePriceSchedule to accept the original term (e.g., add a
parameter originalTerm int64 or float64) and compute upfrontRemaining :=
upfrontCost * (float64(remainingMonths) / float64(originalTerm)), then set
totalValue := upfrontRemaining + (monthlyCost * float64(remainingMonths)) and
continue with listPrice := totalValue * 0.95 and clamping to >=0; also update
the call site in marketplaceList to pass row.Term into
resolveMarketplacePriceSchedule so the function has the original term available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a47024a4-0465-406d-b1b1-370d84820e04
📒 Files selected for processing (25)
frontend/src/api/index.tsfrontend/src/api/purchases.tsfrontend/src/history.tsfrontend/src/permissions.generated.tsfrontend/src/types.tsinternal/analytics/collector_test.gointernal/api/handler.gointernal/api/handler_marketplace.gointernal/api/mocks_test.gointernal/api/router.gointernal/auth/service_group_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sqlinternal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sqlinternal/mocks/stores.gointernal/purchase/mocks_test.gointernal/scheduler/scheduler_test.gointernal/server/test_helpers_test.goproviders/aws/services/ec2/client.goproviders/aws/services/ec2/client_test.go
💤 Files with no reviewable changes (15)
- internal/auth/types_test.go
- internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.down.sql
- providers/aws/services/ec2/client_test.go
- internal/database/postgres/migrations/000060_purchase_history_marketplace_listing.up.sql
- internal/server/test_helpers_test.go
- internal/config/interfaces.go
- internal/scheduler/scheduler_test.go
- internal/purchase/mocks_test.go
- internal/auth/service_group_test.go
- internal/auth/types.go
- internal/config/store_postgres_pgxmock_test.go
- internal/mocks/stores.go
- internal/config/types.go
- providers/aws/services/ec2/client.go
- internal/config/store_postgres.go
✅ Files skipped from review due to trivial changes (1)
- frontend/src/permissions.generated.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/api/mocks_test.go
- internal/api/router.go
- internal/api/handler.go
- frontend/src/types.ts
- frontend/src/api/purchases.ts
- frontend/src/history.ts
- frontend/src/api/index.ts
Member
Author
|
Pushed a follow-up fix: extracted @coderabbitai review |
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).
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.
…lt 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.
…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.
000060 is taken by 000060_cleanup_universal_plans on base.
…se 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.
…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).
… 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).
…eline - 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.
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.
…t 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.
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.
…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.
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.
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… spelling) (#1277) * fix(db): rename cancelled->canceled (expand-contract, migration 000089) Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088). * fix(db): restore correct cancel error paths lost in rebase conflict The rebase of 000089 onto current main incorrectly resolved two conflicts in handler_purchases.go: 1. cancelOrRecoverExecution: reverted fmt.Errorf (router->500) back to NewClientError(409,...), misclassifying retriable backend faults as caller faults (feedback_http_status_classification). 2. cancelPurchaseViaSession: took main's broad !IsCancelable() guard instead of the PR's narrower guardImmediatelyCancelable, allowing "scheduled" executions through the pending/notified-only CAS path and producing a misleading "concurrent operation" 409 instead of the clear "use the revoke endpoint" message. Fix: restore fmt.Errorf for the backend-failure branch; add guardCancelableViaSession that explicitly routes "scheduled" to the revoke endpoint before falling through to IsCancelable; rename residual cancelledBy -> canceledBy; remove extra blank line in types.go. Regression tests: TestHandler_deletePlannedPurchase_BackendErrorReturns5xx and TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke now pass.
This was referenced Jul 17, 2026
Merged
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… and consent modal (follow-up to #808) Mirror of the backend years-as-months fix on the frontend. purchase_history.term is stored in years (1 or 3), but frontend/src/history.ts consumed it as months in two places on the Sell-on-Marketplace path: - canSellOnMarketplace (~line 663): computed remainingMonths = term - elapsedMonths with term in years, so a 3-year RI was treated as 3 months and the Sell button vanished after ~3 months of elapsed time. - Consent/pricing modal (~line 1210): computed the residual with term in years, so the resale price summary shown to the user was ~1/3 of the real value (e.g. a 3yr RI 6 months in showed ~$0/underpriced instead of ~$2,850 on $3,600 upfront). Fix: convert term years->months at the boundary (termYears * 12) before computing remainingMonths/residual in both spots, and guard term <= 0. Correct the existing makeRow() test fixture from term:36 (36 years, nonsensical, masked the bug) to term:3. Add a modal-residual regression test asserting a 3yr RI ~6 months in shows ~30 months remaining and a ~$2,850 list price; it fails pre-fix (the gate hides the button) and passes post-fix. Parallels the backend TestMarketplaceList_TermYearsConvertedToMonths.
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… (follow-up to #808) (#1447) * fix(api/marketplace): convert RI term years->months in resale pricing (follow-up to #808) purchase_history.term is stored in years (1 or 3), confirmed by migration 000007 ("valid terms are 0, 1, or 3 (years)") and execution.go formatting it as "%dyr". The marketplaceList handler was passing row.Term unchanged to computeRemainingMonths (param: termMonths int) and resolveMarketplacePriceSchedule (param: originalTerm, documented as months), silently treating years as months. Impact for a 3-year RI sold 6 months in: pre-fix: remainingMonths = max(1, 3-6) = 1; default price ~= $1,140 (3600 * 1/3 * 0.95) post-fix: remainingMonths = 30; default price ~= $2,850 (3600 * 30/36 * 0.95) A caller-supplied {term_months: 30} schedule was also rejected pre-fix (30 > 1 remaining), preventing sellers from specifying the correct term. Fix: guard row.Term <= 0 (error, not silent fallback), then multiply by 12 at the boundary where years enter the pricing math and pass termMonths to both call sites. Update standardRow() in tests from Term:12 (nonsensical 12 years, accidentally masked the bug) to Term:3. Add TestComputeRemainingMonths unit test and TestMarketplaceList_TermYearsConvertedToMonths end-to-end regression test that fails pre-fix and passes post-fix. * fix(frontend/marketplace): convert RI term years->months in Sell gate and consent modal (follow-up to #808) Mirror of the backend years-as-months fix on the frontend. purchase_history.term is stored in years (1 or 3), but frontend/src/history.ts consumed it as months in two places on the Sell-on-Marketplace path: - canSellOnMarketplace (~line 663): computed remainingMonths = term - elapsedMonths with term in years, so a 3-year RI was treated as 3 months and the Sell button vanished after ~3 months of elapsed time. - Consent/pricing modal (~line 1210): computed the residual with term in years, so the resale price summary shown to the user was ~1/3 of the real value (e.g. a 3yr RI 6 months in showed ~$0/underpriced instead of ~$2,850 on $3,600 upfront). Fix: convert term years->months at the boundary (termYears * 12) before computing remainingMonths/residual in both spots, and guard term <= 0. Correct the existing makeRow() test fixture from term:36 (36 years, nonsensical, masked the bug) to term:3. Add a modal-residual regression test asserting a 3yr RI ~6 months in shows ~30 months remaining and a ~$2,850 list price; it fails pre-fix (the gate hides the button) and passes post-fix. Parallels the backend TestMarketplaceList_TermYearsConvertedToMonths.
cristim
added a commit
that referenced
this pull request
Jul 20, 2026
…ECT (#1472) GetActivePurchaseHistory SELECTed 21 columns but shares scanPurchaseHistoryRow, which scans 24 destinations (offering_class, listing_id, listing_state were added by #808). pgx/v5 Rows.Scan requires field count == destination count, so every real-Postgres call to this query failed with "number of field descriptions must equal number of destinations, got 21 and 24". Impact (all broken on real Postgres, silently in some paths): - dashboard commitment KPIs (fetchCommitmentPurchases swallows the error and renders 0 active commitments / $0 committed monthly / $0 YTD), - GET /api/inventory/commitments and /api/inventory/coverage (return errors), - the analytics collector's Collect (fails every run). #808 (marketplace) added the three columns to the scanner and to GetPurchaseHistory / GetAllPurchaseHistory / GetPurchaseHistoryFiltered but missed GetActivePurchaseHistory. This is the same class of defect #1221 fixed for the 17-vs-21 case. Fix: append offering_class, listing_id, listing_state so the column list is identical to the other purchase-history queries and matches the scanner. Update the pgxmock regex to pin the corrected column list so a future drop of these columns fails the test. Note: pgxmock cannot reproduce the field-count mismatch (it fabricates result columns), so the regression is only guarded via the SQL-text regex here; a shared "emitted SELECT column list == purchaseHistoryCols" assertion across all purchase-history queries is a worthwhile follow-up. Found by the adversarial (Fable) review sweep of recently-merged PRs.
9 tasks
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
…1493) resolveMarketplacePriceSchedule's default-schedule branch silently clamped a negative computed list price to 0 instead of rejecting it. A no-upfront RI (upfrontCost <= 0) or one with an unknown/elapsed term (originalTerm <= 0) makes marketplaceResidualPerUnit return 0, so the RI would be listed on AWS Marketplace for free instead of erroring out like the supplied-schedule branch already does for a non-positive price. The backend now returns an explicit error asking the caller to supply a price_schedule. The sell-consent-modal preview in frontend/src/history.ts had a parallel bug: it computed the preview price with a different formula than the backend (including recurring monthly cost, which the backend deliberately excludes, and not dividing by instance count), so it could show a nonzero preview price for exactly the case the backend now rejects, and diverge from the real listing price for multi-count RIs. The preview now mirrors marketplaceResidualPerUnit and resolveMarketplacePriceSchedule's default branch exactly, and shows "Default list price: unavailable (no upfront cost or unknown term)" instead of a fabricated or zero price when no default can be computed. Adds TestResolveMarketplacePriceSchedule_ZeroDefaultPriceRejected (backend) and two regression tests in history-marketplace-sell-button.test.ts (frontend): one pinning the per-unit formula against the old recurring-inclusive row-total formula for a multi-count RI, one asserting the no-upfront case shows "unavailable" and never "$0". Follow-up to #808, found during an adversarial review sweep.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
offering_class,listing_id,listing_statecolumns topurchase_historyPOST /api/purchases/{id}/marketplace-listandPOST /api/purchases/{id}/marketplace-cancelroutes withsell-any/sell-ownRBAC (mirroring the cancel/retry/approve family);sell-ownadded toDefaultUserPermissions; default price schedule is 95% of residual value (upfront + monthly * remaining months); EC2 API:CreateReservedInstancesListing,DescribeReservedInstancesListings,CancelReservedInstancesListinglisting_state == "active"; confirm-dialog + API call + toast + reload click handlers; all API types exported fromapi/index.tsTest plan
go test ./...- all 4966 tests pass across 38 packagesnpm testinfrontend/- all 2142 frontend tests passnpm run build- compiles with no TS errorsoffering_class=standardand verify "Sell on Marketplace" button appears; click it and confirm the API call is dispatchedlisting_state=activeand cancels correctlysell-ownusers can only sell RIs in their allowed accounts;sell-anyusers can sell anyScope / descope
This PR ships listing creation and cancellation for Standard RIs. Two parts of #292 are intentionally descoped to keep it landable and are tracked in follow-up #966:
active -> closed/sold transition, realized proceeds, AWS fee actually charged).Because the poller is not implemented, migration 000060 was reduced to 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) were removed so the schema never carries columns nothing populates; they will land with the poller in #966.Closes #292 partially; remainder tracked in #966.
Summary by CodeRabbit
Release Notes