Repository navigation
feat(marketplace): poll RI Marketplace listings and record proceeds and AWS fee (backend slice of #27) - #1289
Conversation
…writes Migration 000148 adds the purchase_history settlement columns that 000087 deferred (listed_at, listing_proceeds_received, listing_fee_paid). listing_price_schedule already exists via 000105 (LeanerCloud#525) and is left alone, including on rollback. The store gains the three statements the RI Marketplace status poller needs: the scan of listings to poll and two status-guarded UPDATEs. The guards (same listing id, active or same terminal state, proceeds not yet recorded) decide whether a poll result applies. Concurrent polls therefore record proceeds once, and a cancel recorded by the cancel handler is never revived or turned into a sale. Real-Postgres tests cover the races. Refs LeanerCloud#27 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PollMarketplaceListings reads each listing due for a poll from AWS (DescribeReservedInstancesListings through the existing listing-finder interface, signed as the purchase's own account like list and cancel). While a listing is active it records its CreateDate as listed_at. When the listing closes, or is canceled after a partial sale, it records the terminal state with the seller's net proceeds and the AWS fee: the instances AWS reports sold, times the active price tier, minus the 12% fee, in whole cents. When instances sold but no single active USD tier exists, the state is recorded and the proceeds are left unset for a later poll instead of guessed. A failure on one listing is counted and the run moves on to the next. Refs LeanerCloud#27 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The vacuous-assertion guard cannot resolve f.store as a receiver. Give the two tests their own *MockConfigStore and use the name-only AssertNotCalled, which reads the call log and fails on any call. Refs LeanerCloud#27 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tions sweep The finalize_revocations task now also runs the RI Marketplace status poller after the revocation repair. Both reconcile purchase_history with the provider on the same hourly schedule, so no new schedule or infrastructure is needed. The two steps are independent: one failing does not skip the other, and the task fails if either did. The task's result keeps the revocation counts under their existing keys and adds the poll counts under marketplace_listings. Refs LeanerCloud#27 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
collector_test.go takes main's LeanerCloud#1254 version: its hand-rolled ConfigStore mock now embeds mocks.MockConfigStore, which this branch already extends with ListMarketplaceListingsToPoll, RecordMarketplaceListingActive and SettleMarketplaceListing, so the three local stubs the branch had added are no longer needed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…148 -> 000144 golang-migrate only applies versions above the database's current one, so PRs must merge in migration-number order. 000140-000144 had been taken by PRs that are now held on owner decisions (LeanerCloud#1239, LeanerCloud#1241, LeanerCloud#1253). The ready PRs move to 000140-000145 so they can merge ahead of them, and the held ones will take the next free numbers when decided. Rename only: the SQL statements are unchanged, and the test version constants and the comments naming the migration follow the new number. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Renumbered migration 000148 -> 000144 so it can merge ahead of PRs held on owner decisions (#1239, #1241, #1253, which will take the next free numbers when decided). Rename only; SQL unchanged. New head: The origin/main merge had one conflict, in internal/analytics/collector_test.go. It takes main's #1254 version: the hand-rolled ConfigStore mock now embeds mocks.MockConfigStore, which this branch already extends with ListMarketplaceListingsToPoll, RecordMarketplaceListingActive and SettleMarketplaceListing, so the three local stubs are no longer needed. The test name follows the number: TestMigration148_ListingSettlementColumns is now TestMigration144_ListingSettlementColumns. Merge order for the ready PRs: #1240 (000140) -> #1302 (000141) -> #1268 (000142) -> #1287 (000143) -> #1289 (000144) -> #1116 (000145). The PR body's migration-number paragraph is updated to match. Checks on this head (local): build, vet (plain, integration, e2e), golangci-lint v2.10.1 with and without the integration tag and gocyclo -over 10 on the changed packages, |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES Blocking:
Non-blocking:
Attacked: I read the store guards, the poller and the sweep wiring against issue #27, then traced the list, claim and cancel store paths for relist interactions. That found the blocking case, which I proved with a temporary real-Postgres integration test (deleted afterwards). Fail-before probes: making unknown prices settle at $0 fails Body-stale:
Reviewer: rE5. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
…recorded Repricing an RI on the Marketplace means cancel and relist, since AWS has no price edit. A relisted row kept the earlier listing's listed_at, proceeds and fee, and only RecordMarketplaceListingActive cleared them, which needs a poll to see the new listing active first. A relist that sold out (or was canceled) before the next hourly poll was therefore never settled: the settle guard refuses rows whose proceeds are set, so the row stayed active with the old 0.00 and was polled and skipped every hour, or showed the old listing's settlement as the new one's. UpdatePurchaseHistoryListing and ResolveMarketplaceListingAttempt, the two statements that write a listing id, now null the three columns in the same UPDATE when the id changes. Re-recording the id the row already holds (a cancel or its retry) keeps them. The new real-Postgres test runs the review's exact sequence plus the canceled-within-the-hour and resolved-attempt variants. Refs LeanerCloud#27 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressing rE5's REQUEST_CHANGES on Blocking: relist keeps the previous listing's
Non-blocking: cancel before the first poll leaves Non-blocking: migration numbering. Unchanged at 000144. main is now at 000141 (#1240 and #1302 merged). The order is #1268 (142) → #1287 (143) → #1289 (144) → #1116 (145), and the body says so. Non-blocking: derived proceeds. Unchanged. It is still flagged as an owner decision in the body. Body-stale. Both sentences are rewritten. The "relisted ... left alone" bullet now says a poll result for a listing the row no longer holds is ignored, and that the relist itself clears the old settlement. The active-refresh bullet now says the old settlement is cleared when the relist writes the new id, whether or not a poll sees it active. The body also gains commit 4, the known window, the new verification block, and a note that the combined-tree run was on the earlier head Checks on
No AWS API was called. |
LeanerCloud#1460 replaced the hand-written mockConfigStoreForHealth with the shared mocks.MockConfigStore, which already has ListMarketplaceListingsToPoll, RecordMarketplaceListingActive and SettleMarketplaceListing, so take main's test_helpers_test.go and drop this branch's stub methods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rE8. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
Summary
Backend first slice of #27. Until now, once an RI was listed on the AWS RI Marketplace, nothing told CUDly when AWS sold it. The list and cancel endpoints write
listing_stateas active or canceled. No path moved a listing toclosed, and none recorded what the sale brought in. Migration 000087 deliberately left out the settlement columns until a poller existed to write them.This PR adds that poller and re-adds the settlement columns. The seller-account precondition check and its UI are left for the next slice (see "Out of scope" and "Frontend follow-up").
Refs #27
What changed
The work is split into reviewable commits:
784c973d)purchase_history.listed_at TIMESTAMPTZ,listing_proceeds_received NUMERIC(12,2)andlisting_fee_paid NUMERIC(12,2). All three are nullable, useIF NOT EXISTS, and get no backfill.listing_price_scheduleis also named in the issue, but migration 000105 (fix(marketplace): ambiguous AWS errors and crashes between create and persist can leave a duplicate or a stuck pending row #525) already added it for the in-flight listing attempt. 000144 does not add it, and its down migration does not drop it.internal/config/store_postgres_marketplace_settlement.go, plus the matchingStoreInterfaceentries and mocks:ListMarketplaceListingsToPollRecordMarketplaceListingActiveSettleMarketplaceListing2da0a1e9, plus test fix5d3d1ccf)api.Handler.PollMarketplaceListingsininternal/api/handler_marketplace_poller.go.DescribeReservedInstancesListingsthrough the existing listing-finder interface (marketplaceListingFinder, built bybuildMarketplaceFinder). That interface gains one method. Theec2svcwrapper'sDescribeMarketplaceListingreturns only the state, and the poller needs the sold counts and price tiers too.loadMarketplaceAWSConfigpath that list and cancel use.2d676224)finalize_revocationsscheduled task (internal/server/handler_marketplace_poll.go) now runs the poll after the revocation repair.purchase_historywith the provider, and it is enabled by default on AWS Lambda atrate(1 hour).cleanupis opt-in and destructive, and a new task type would need new EventBridge rules, i.e. new infra.Found/Finalized/Errored) and addsmarketplace_listings.19a1558f, from review)UpdatePurchaseHistoryListingandResolveMarketplaceListingAttempt, the two statements that write a listing id, now nulllisted_at,listing_proceeds_receivedandlisting_fee_paidin the same UPDATE when the listing id changes. Re-recording the id the row already holds (a cancel, or its retry) keeps them.What the poller records
listed_atis set to the listing's AWSCreateDate. An earlier listing's settlement is not carried over: it is cleared when the relist writes the new listing id (see commit 4), whether or not a poll ever sees the new listing active. The active refresh also clears it, which only matters for rows relisted before this PR.closedon AWS (all sold), orcancelledafter a partial sale: the terminal state is recorded, and with it:listing_proceeds_received: what the seller receives, net of the AWS fee.listing_fee_paid: the AWS fee, 12% of the sale price (the existingawsMarketplaceFeePercent).sold× the price of the listing's single active USD price tier. The math is done in whole cents, so proceeds + fee equals the sale price exactly.unpriced.pending: no write.errored, and the run moves on to the next listing.Which rows are polled: AWS rows with a listing that is
active, plusclosed/cancelledrows the poller has already seen (listed_atset) whose proceeds are not recorded yet. The second group covers a cancel that the cancel handler recorded before the poller saw the listing end, so partial-sale proceeds are still captured. Canceled rows withlisted_atNULL are not polled: legacy rows, and the window below. There is one row per(purchase_id, listing_id), so legacy duplicate rows (#267) cost one AWS call.Known window: a listing canceled through CUDly before the first poll after it was listed (up to one hour, the sweep interval) never gets
listed_at, so it is never polled and a partial sale inside that window is not recorded. The cancel handler cannot stamplisted_atitself, because neither the AWS cancel response nor the wrapper'sDescribeMarketplaceListing(called only when the cancel fails) returnsCreateDate. Closing the window would need an extraDescribeReservedInstancesListingscall in the cancel path. I left that out to keep this slice small.Concurrency
The store's status-guarded UPDATEs decide whether a poll result applies, not the poller's copy of the row:
SettleMarketplaceListingapplies only while all of these hold:listing_id.listing_proceeds_received IS NULL.activeor already the same terminal state.RecordMarketplaceListingActiveapplies only while the row holds that listing inactive, and it never writeslisting_state.What this gives:
closedsale.pending) is left alone.listed_atand settlement in the same UPDATE that writes the new listing id, so the new listing settles even if it ends before any poll sees it active.Postgres re-checks these WHERE clauses on the new row version after a lock wait, and the real-Postgres tests below cover both deterministic lock-order races and parallel storms. The task's advisory lock also prevents concurrent runs of the same task, but correctness does not depend on it.
Verification
All of this is unit/mock/fixture evidence plus local real-Postgres integration tests (testcontainers). No AWS API was called. The poller is tested only with fakes of the listing-finder interface and of
DescribeReservedInstancesListings.Commands run on this branch (Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):After the review fix (
19a1558f, on main merged at527ae6b2):TestMarketplaceListingRelist_Postgres(new, real Postgres) runs the review's sequence: L1 active,RecordMarketplaceListingActive, cancel, settle 0/0,ClaimMarketplaceListingSlot,UpdatePurchaseHistoryListing(L2, active), settle L2closed. It also covers L2 canceled within the hour, a recovered pending attempt resolved to L2 (ResolveMarketplaceListingAttempt), and a same-id re-record that keeps the settlement. Before the fix, three of the four subtests fail: the relist keeps L1'slisted_atand 0/0, the L2 settle returnsapplied=false, the row staysactive, and it is still inListMarketplaceListingsToPoll. After the fix, all four pass.Fail-before / pass-after (mutation probes; each mutation applied alone, then reverted):
listing_proceeds_received IS NULLSettleIsGuarded,SecondSettleWaitsAndSkips,ParallelSettlesRecordOnceSettleIsGuarded,CancelCommittingBeforeCloseWins,ParallelCancelAndPollKeepCancellisting_idguardSettleIsGuardedActiveRefreshIsGuarded,CancelCommittingBeforeActiveRefreshWinsTestMigration144_ListingSettlementColumns(settlement write errors)listing_price_scheduleTestMigration144_ListingSettlementColumnsTestPollMarketplaceListings_UnknownPriceIsNotGuessedClosedRecordsProceedsAndFee,CanceledAfterPartialSale,TestMarketplaceSettlementAmountsTestPollMarketplaceListings_CanceledAfterPartialSaleTestActiveUSDListingPriceTestPollMarketplaceListings_FailuresAreCountedAndTheRunGoesOnpendingtreated like activeTestPollMarketplaceListings_AWSPendingNeedsNoWrite19a1558fcode)RelistClosesBeforeFirstPoll,RelistCanceledBeforeFirstPoll,ResolvedAttemptDropsOldSettlementhandleFinalizeRevocationsTestFinalizeSweep_PollsMarketplaceListings,_RevocationFailureStillPolls,_PollFailureFailsTheTaskCombined-tree verification (run on the earlier head
2d676224, before the review fix). This branch was tested together with the other queued branches on a local, never-pushed tree:queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Things to check / judgment calls
DescribeReservedInstancesListingsreports the sold instance counts and the price tiers, but not the money AWS actually disbursed or the fee it actually charged. Those appear only in the seller disbursement reports. The recorded values are sold × active-tier price, minus 12%.finalize_revocations. Operators who setenable_finalize_revocations_schedule = falsealso disable the marketplace poll. On AWS Fargate and CloudFormation,finalize_revocationsis not scheduled (a known gap inscheduled_task_coverage_test.go), so the poll does not run there either. The Terraform variable descriptions still say "audit repair"; I did not touch Terraform.erroredon every run. The same goes for a terminal row that stays unpriceable (counted asunpriced). There is no back-off column.Out of scope (next slices)
SellerNotRegisteredas a 400 throughawsMarketplaceClientFaultCodes.listed_at/listing_proceeds_received/listing_fee_paidin the purchase history API.GetPurchaseHistory*does not select them yet. That is left out deliberately to avoid conflicts with the open PRs (fix(config): use the earliest row when purchase_history duplicates a purchase_id #1103, fix(history): set cloud_account_id on every purchase_history insert #1244) that touch those queries instore_postgres.go. The new store code is in its own file for the same reason.Frontend follow-up for the UI owner
Nothing under
frontend/is changed. Once the history API exposes the new columns (next backend slice):listing_state = closed, with the listing date (listed_at), net proceeds (listing_proceeds_received) and the AWS fee (listing_fee_paid). Label them as estimates derived from the listing price (see above).cancelledwith non-zero proceeds, show the partial sale.closed/cancelledwith NULL proceeds, show "proceeds pending / unavailable".Labels
A maintainer please mirror #27's triage labels:
priority/p2,severity/low,urgency/eventually,impact/few,effort/l,type/feat,triaged.🤖 Generated with Claude Code