Skip to content

feat(marketplace): poll RI Marketplace listings and record proceeds and AWS fee (backend slice of #27) - #1289

Merged
IAmJSD merged 10 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-27
Oct 26, 2026
Merged

IAmJSD merged 10 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-27

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

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_state as active or canceled. No path moved a listing to closed, 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:

  1. Migration and store (784c973d)
    • Migration 000144 adds purchase_history.listed_at TIMESTAMPTZ, listing_proceeds_received NUMERIC(12,2) and listing_fee_paid NUMERIC(12,2). All three are nullable, use IF NOT EXISTS, and get no backfill.
    • listing_price_schedule is 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.
    • The store gains three methods in internal/config/store_postgres_marketplace_settlement.go, plus the matching StoreInterface entries and mocks:
      • ListMarketplaceListingsToPoll
      • RecordMarketplaceListingActive
      • SettleMarketplaceListing
  2. Poller (2da0a1e9, plus test fix 5d3d1ccf)
    • api.Handler.PollMarketplaceListings in internal/api/handler_marketplace_poller.go.
    • Each listing is read with DescribeReservedInstancesListings through the existing listing-finder interface (marketplaceListingFinder, built by buildMarketplaceFinder). That interface gains one method. The ec2svc wrapper's DescribeMarketplaceListing returns only the state, and the poller needs the sold counts and price tiers too.
    • Requests are signed as the purchase's own account, through the same loadMarketplaceAWSConfig path that list and cancel use.
  3. Wiring (2d676224)
    • The existing finalize_revocations scheduled task (internal/server/handler_marketplace_poll.go) now runs the poll after the revocation repair.
    • That sweep fits best: it already reconciles purchase_history with the provider, and it is enabled by default on AWS Lambda at rate(1 hour). cleanup is opt-in and destructive, and a new task type would need new EventBridge rules, i.e. new infra.
    • The two steps are independent. One failing does not skip the other, and the task fails if either failed.
    • The task result keeps the revocation counts under their existing keys (Found/Finalized/Errored) and adds marketplace_listings.
  4. Relist clears the previous settlement (19a1558f, from review)
    • UpdatePurchaseHistoryListing and ResolveMarketplaceListingAttempt, the two statements that write a listing id, now null listed_at, listing_proceeds_received and listing_fee_paid in the same UPDATE when the listing id changes. Re-recording the id the row already holds (a cancel, or its retry) keeps them.
    • Without this, a relist (AWS has no price edit, so repricing is cancel and relist) that sold out or was canceled before the next hourly poll was never settled, because the settle guard refuses a row whose proceeds are already set.

What the poller records

  • Listing still active on AWS: listed_at is set to the listing's AWS CreateDate. 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.
  • Listing closed on AWS (all sold), or cancelled after 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 existing awsMarketplaceFeePercent).
    • Sale price = the instances AWS reports as 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.
    • A canceled listing with nothing sold settles at 0 / 0.
  • Instances sold but no single active USD tier with a price: the state is recorded and the proceeds and fee are left NULL instead of guessed. The row stays due, so a later poll can price it. Each run counts and logs it as unpriced.
  • AWS pending: no write.
  • A failure on one listing (credentials, AWS, DB): logged, counted as errored, and the run moves on to the next listing.

Which rows are polled: AWS rows with a listing that is active, plus closed/cancelled rows the poller has already seen (listed_at set) 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 with listed_at NULL 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 stamp listed_at itself, because neither the AWS cancel response nor the wrapper's DescribeMarketplaceListing (called only when the cancel fails) returns CreateDate. Closing the window would need an extra DescribeReservedInstancesListings call 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:

  • SettleMarketplaceListing applies only while all of these hold:
    • The row still holds that listing_id.
    • listing_proceeds_received IS NULL.
    • The state is active or already the same terminal state.
  • RecordMarketplaceListingActive applies only while the row holds that listing in active, and it never writes listing_state.

What this gives:

  • Two poller runs, or a retried run, record proceeds exactly once.
  • A cancel recorded by the cancel handler is never revived to active, and never turned into a closed sale.
  • A poll result for a listing the row no longer holds (the RI was relisted since) is ignored, and a row that is mid-claim (pending) is left alone.
  • A relist clears the previous listing's listed_at and 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):

go build ./... && go vet ./... && go vet -tags integration ./...            # clean
go test -count=1 ./internal/api/                                             # ok
go test -count=1 ./internal/config/                                          # ok
go test -count=1 ./internal/mocks/                                           # ok (incl. vacuous-assertion guard)
go test -count=1 ./internal/server/                                          # ok
go test -count=1 ./internal/analytics/                                       # ok
go test -count=1 ./internal/database/postgres/migrations/                    # ok
golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration \
  ./internal/{api,config,mocks,server,analytics}/ ./internal/database/postgres/migrations/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>                # nothing over 10
go test -tags integration -count=1 -timeout 60m -run 'TestMarketplaceListingSettlement_Postgres' ./internal/config/        # ok (9 subtests)
go test -tags integration -count=1 -timeout 20m -run 'TestMigration144_' ./internal/database/postgres/migrations/          # ok

After the review fix (19a1558f, on main merged at 527ae6b2):

go build ./... && go vet ./... && go vet -tags integration ./... && (cd tests/e2e && go vet -tags e2e ./...)   # clean
golangci-lint v2.10.1 run, with and without --build-tags integration, on api/config/mocks/server/migrations    # 0 issues
gocyclo -over 10 on the changed non-test files                                                                 # nothing over 10
go test -race -short -count=1 ./internal/{api,config,mocks,server}/... ./internal/database/postgres/migrations/...   # ok
go test -race -tags integration -count=1 ./internal/config/ ./internal/database/postgres/migrations/ ./internal/server/   # ok (config 201s, migrations 278s, server 55s)

TestMarketplaceListingRelist_Postgres (new, real Postgres) runs the review's sequence: L1 active, RecordMarketplaceListingActive, cancel, settle 0/0, ClaimMarketplaceListingSlot, UpdatePurchaseHistoryListing(L2, active), settle L2 closed. 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's listed_at and 0/0, the L2 settle returns applied=false, the row stays active, and it is still in ListMarketplaceListingsToPoll. After the fix, all four pass.

Fail-before / pass-after (mutation probes; each mutation applied alone, then reverted):

Mutation Failing tests
Settle without listing_proceeds_received IS NULL SettleIsGuarded, SecondSettleWaitsAndSkips, ParallelSettlesRecordOnce
Settle without the state guard SettleIsGuarded, CancelCommittingBeforeCloseWins, ParallelCancelAndPollKeepCancel
Settle without the listing_id guard SettleIsGuarded
Active refresh without the state guard ActiveRefreshIsGuarded, CancelCommittingBeforeActiveRefreshWins
000144 up file emptied TestMigration144_ListingSettlementColumns (settlement write errors)
000144 down also drops listing_price_schedule TestMigration144_ListingSettlementColumns
Unknown price recorded as $0 TestPollMarketplaceListings_UnknownPriceIsNotGuessed
Proceeds recorded gross (fee not deducted) ClosedRecordsProceedsAndFee, CanceledAfterPartialSale, TestMarketplaceSettlementAmounts
Partial sales on a cancel ignored TestPollMarketplaceListings_CanceledAfterPartialSale
First active tier wins when two are active TestActiveUSDListingPrice
One listing's failure aborts the run TestPollMarketplaceListings_FailuresAreCountedAndTheRunGoesOn
AWS pending treated like active TestPollMarketplaceListings_AWSPendingNeedsNoWrite
Relist without clearing the old settlement (the pre-19a1558f code) RelistClosesBeforeFirstPoll, RelistCanceledBeforeFirstPoll, ResolvedAttemptDropsOldSettlement
Task dispatch back to handleFinalizeRevocations TestFinalizeSweep_PollsMarketplaceListings, _RevocationFailureStillPolls, _PollFailureFailsTheTask

Combined-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/main a14dc8f2 plus these heads, merged in this order:

Branch Tested head
fix/issue-337 ff3420c
fix/issue-169 7d75eb5
fix/issue-342 66b221f
fix/issue-310 5e11c21
fix/issue-326 5f0ea59
fix/issue-243 365e959
fix/issue-343 6c94036
fix/issue-312 d2848a8
fix/issue-316 acd60dd
fix/issue-339 75ea9b9
fix/issue-339-commitment-type 73bdb19
fix/issue-353 5d25fcd
fix/issue-313 3156bc4
fix/issue-357 53d9e58
fix/issue-368 0ba727d
fix/issue-419 61648c9
fix/issue-111 b04c372
fix/issue-686 0ad90b7
fix/issue-674 793095d
fix/issue-1055 4ec96f1
fix/issue-559 dfa9525
fix/issue-558 034eed4
fix/issue-874 1720539
fix/issue-666 1e120da
fix/issue-668 fb26345
fix/issue-568 ce04173
fix/issue-311 e54f250
fix/issue-177 0cbb88a
fix/issue-151 c21377c
fix/issue-166 41fb77b
fix/issue-27 2d67622
fix/issue-323 69288ae
fix/issue-59 36463e8
fix/issue-43 35e4069
fix/issue-158 6f76f8a
fix/issue-104 bb00491
fix/issue-55 971ef1b
fix/issue-361 89ac220
fix/issue-120 b458719
fix/issue-36 42bac6e
fix/issue-525 57c97dd
fix/issue-712 e5bcda0
fix/issue-72 6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; 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:

  • Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
    • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
    • internal/config passes except TestPostgresStoreDB_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).
    • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
  • What the combined run caught, each fixed on its own branch before this PR opened:
    • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
    • fix/issue-316 rejecting legacy-keyed plans;
    • fix/issue-120 versus main's global-filter test.
  • Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns 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.
  • Nothing was applied to any cloud.

Things to check / judgment calls

Out of scope (next slices)

Frontend follow-up for the UI owner

Nothing under frontend/ is changed. Once the history API exposes the new columns (next backend slice):

  • Show "Sold" for 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).
  • For cancelled with non-zero proceeds, show the partial sale.
  • For closed/cancelled with NULL proceeds, show "proceeds pending / unavailable".
  • Seller-account precondition: before the Sell dialog submits, show a tailored message when the account is not a registered RI Marketplace seller (US bank account on file). This depends on the precondition-check endpoint from the next slice.

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

IAmJSD and others added 4 commits October 23, 2026 06:34
…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>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy effort/l Weeks triaged Item has been triaged severity/low Minor harm urgency/eventually No deadline impact/few Limited audience type/feat New capability labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

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>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE cd13d824abafcd3ebb972011bcd33fa4dd90d006

Blocking:

  • none

Non-blocking:

  • Merge order (migration 000148). This must merge after every PR holding 000140-000147, including fix(analytics): keep unknown usage and commitment unknown in the savings rollups #1287 (147). Otherwise a production database at version 148 will never apply the lower-numbered files when they merge later (golang-migrate only moves forward). The alternative is to renumber whichever of those is still open.
  • internal/api/handler_marketplace_poller.go:84-104: the price relies on AWS still marking one schedule tier Active after the listing is closed/cancelled. I couldn't verify offline that DescribeReservedInstancesListings does this for ended listings. If AWS deactivates every tier when a listing ends, every real sale lands in unpriced. That fails safe (no fabricated number), but every such row is re-polled hourly forever, each time with a credential resolution, an AWS call and a warning, and none of them ever settles. Recording the active tier price on the row during the active polls, then using it at settlement, would remove the dependency. Worth checking against one real closed listing before relying on these columns.
  • handler_marketplace_poller.go:117-121: a closed listing whose InstanceCounts report 0 sold (incomplete data, since closed means everything sold) is settled at $0 / $0 and never polled again. That's the fabricated-zero pattern fix(money): zero-vs-absent conflation across frontend/internal money paths [platform part] #151 is about. Consider treating closed && SoldCount == 0 as unpriced.
  • Small hole in "the row stays due": when AWS omits CreateDate and the poller never saw the listing while it was active, an unpriced settle leaves listed_at NULL. The row is then neither active nor matched by the listed_at IS NOT NULL branch, so it is never re-polled. It's unlikely, since AWS normally returns CreateDate.
  • Legacy duplicate rows (fix(db): duplicate purchase_history.purchase_id rows wedge the marketplace listing slot #267) for the same (purchase_id, listing_id) each get the same proceeds written, because the UPDATE matches all of them. Any future SUM over listing_proceeds_received must dedupe.
  • The body already lists the remaining owner calls: proceeds derived as sold × active price minus 12% (not the disbursement report); the poll is disabled wherever finalize_revocations is not scheduled (Fargate/CloudFormation) or is turned off; no per-run cap.

Attacked:

  • Tenant and credential scope: every AWS read goes through loadMarketplaceAWSConfig(row), the same path list and cancel use, signed as the row's own cloud account (or the host-account check when there is none). A row can only read listings visible to its own account. Store writes are keyed on purchase_id plus AWS's globally unique listing_id. I traced the guards: settle needs proceeds IS NULL and state in (active, same terminal); the active refresh never writes state. The sweep wiring runs both steps, errors.Joins their errors, and keeps the revocation result keys through the embedded pointer. The migration adds nullable columns only, so it is metadata-only, and the down migration leaves 000105's listing_price_schedule in place.
  • Fail-before (mutations, each reverted): (1) I dropped listing_proceeds_received IS NULL from the settle UPDATE, and SettleIsGuarded, SecondSettleWaitsAndSkips and ParallelSettlesRecordOnce failed against real Postgres. (2) I recorded an unknown price as $0, and TestPollMarketplaceListings_UnknownPriceIsNotGuessed failed. (3) I recorded proceeds gross, and ClosedRecordsProceedsAndFee, CanceledAfterPartialSale and TestMarketplaceSettlementAmounts failed.
  • Ran: go build ./..., go vet ./..., go vet -tags integration ./... and tests/e2e go vet -tags e2e are clean. go test -race -short -count=1 passes for api, config, server, mocks, analytics and migrations. go test -race -tags integration passes for TestMarketplaceListingSettlement_Postgres (9/9 subtests). -tags integration passes for TestMigration148_ListingSettlementColumns, after 3 testcontainers startup timeouts caused by VM load. golangci-lint v2.10.1 reports 0 issues on the changed packages with and without --build-tags integration. gocyclo -over 10 on the changed non-test files is silent.

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.

IAmJSD and others added 2 commits October 26, 2026 08:46
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>
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

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: f328e644. The branch first merged origin/main (529b6e38); then one commit renames the up/down/test files and updates the test's version constant and the comments that name the number (including the self-referencing header comments in the .sql files; no statement changed).

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, go test -race -short and go test -race -tags integration on the changed packages (including the migrations package's up/down round trip and number guards), secrets scan and git-secrets allowlist test. The six renumbered branches were also merged onto origin/main in that order in one scratch tree, and go test -race -tags integration ./internal/database/postgres/migrations/ passed there, so the chain applies up and down cleanly together.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES f328e64431af9ca936f166593a708a75059db851

Blocking:

  • internal/config/store_postgres_marketplace_settlement.go:107-114 (with UpdatePurchaseHistoryListing, store_postgres.go:2328): a relisted row keeps the previous listing's listed_at / listing_proceeds_received / listing_fee_paid, because the list path never clears them, and SettleMarketplaceListing refuses to write while listing_proceeds_received IS NOT NULL. The only thing that clears them is RecordMarketplaceListingActive, which needs the poller to see the new listing as active first. Concrete failure, using the standard repricing flow (AWS has no price edit, so you cancel and relist): L1 is listed and the poller sees it active (listed_at set). The user cancels it and the next poll settles it at 0/0. The user relists the RI at a lower price as L2 (ClaimMarketplaceListingSlot allows a canceled row). L2 sells out before the next hourly poll. The poller then reads L2 closed, the settle UPDATE matches 0 rows because proceeds = 0 is not NULL, and the run counts it as Skipped. The row stays listing_state = active with L1's 0.00 proceeds, it is polled and skipped every hour forever, and L2's sale is never recorded. I reproduced this against real Postgres with a throwaway integration test that calls the real store methods (seed L1 active, then RecordActive, cancel, settle 0/0, claim, UpdatePurchaseHistoryListing(L2, active), settle L2 closed). Result: applied=false state=active proceeds=0, and the row was still in ListMarketplaceListingsToPoll. A variant of the same bug: if L2 is canceled within the hour instead, the row is cancelled with L1's listed_at and proceeds set, so it is never polled, and L1's settlement is shown as L2's. Fix idea: clear the three columns whenever a new listing_id is written (in UpdatePurchaseHistoryListing when listing_id changes, or in the claim), and add a store test for relist-then-settle without an intervening active poll. The existing checkActiveRefresh "relisted RI" case only covers the path where the poller sees the relist active first.

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 TestPollMarketplaceListings_UnknownPriceIsNotGuessed, and dispatching back to handleFinalizeRevocations fails all three TestFinalizeSweep_* tests. Both were restored. Ran go build ./..., go vet ./... and go vet -tags integration ./... (clean), go test -race -short -count=1 on internal/{api,config,mocks,server} and migrations (ok), and go test -race -tags integration -run 'TestMarketplaceListingSettlement_Postgres|TestMigration144_' on config and migrations (ok).

Body-stale:

  • "A row that has been relisted ... is left alone" and "Any settlement left on the row by an earlier listing of the same RI is cleared" are only true when the poller sees the new listing active before it ends (see Blocking).

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.

IAmJSD and others added 2 commits October 26, 2026 12:49
…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>
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Addressing rE5's REQUEST_CHANGES on f328e644. New head: 19a1558f (fix commit on top of a git merge origin/main, merge commit 527ae6b2). No migration added or renumbered; still 000144.

Blocking: relist keeps the previous listing's listed_at / proceeds / fee. Fixed.

  • UpdatePurchaseHistoryListing and ResolveMarketplaceListingAttempt are the two statements that write a listing id. Both now set listed_at, listing_proceeds_received and listing_fee_paid to CASE WHEN COALESCE(listing_id, '') = <new id> THEN <column> END in the same UPDATE. The right-hand sides read the row's old listing_id, so the check and the clear are one atomic write that covers every list path: the normal list record, the resumed-attempt record, the rollback-cancel record and the cancel endpoint's pending-attempt recovery. Re-recording the id the row already holds (a cancel, or a cancel retry) keeps the settlement.
  • I did not put the clear in ClaimMarketplaceListingSlot. A claim that is later released (ReleaseMarketplaceListingClaim) restores the prior state, so clearing there would lose L1's settlement when no relist happens. The claim also touches only the earliest duplicate row.
  • New real-Postgres test TestMarketplaceListingRelist_Postgres (internal/config/store_postgres_marketplace_relist_integration_test.go):
    • RelistClosesBeforeFirstPoll: your exact sequence (L1 active, RecordMarketplaceListingActive, cancel, settle 0/0, claim, UpdatePurchaseHistoryListing(L2, active), settle L2 closed). It now ends with applied=true, state closed, L2's listed_at, proceeds 1760 and fee 240, and the row is no longer in ListMarketplaceListingsToPoll.
    • RelistCanceledBeforeFirstPoll: L2 canceled within the hour. The row is cancelled with no listed_at and no proceeds (L1's 0/0 is no longer shown as L2's), and an L2 cancel settlement applies.
    • ResolvedAttemptDropsOldSettlement: the same clear on the ResolveMarketplaceListingAttempt path.
    • SameListingKeepsSettlement: a same-id re-record keeps the settlement.
  • Fail-before: on f328e644 plus the test only, the first three subtests fail. The relist keeps L1's listed_at and 0/0, the L2 settle returns applied=false, the row stays active with proceeds 0, and it is still in the poll list. Pass-after: all four pass on 19a1558f.

Non-blocking: cancel before the first poll leaves listed_at NULL. Documented in the PR body as a known window. The cancel handler has no CreateDate to stamp. The AWS cancel response and the wrapper's DescribeMarketplaceListing (called only when the cancel fails) return only the id and state. Closing the window would need an extra DescribeReservedInstancesListings call in the cancel path, which I kept out of this slice.

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 2d676224.

Checks on 19a1558f (Linux, Go 1.26.9, AWS env pointed at /dev/null, testcontainers Postgres):

  • go build ./..., go vet ./..., go vet -tags integration ./... and tests/e2e go vet -tags e2e ./...: clean.
  • golangci-lint v2.10.1 on api/config/mocks/server/migrations, with and without --build-tags integration: 0 issues.
  • gocyclo -over 10 on the changed non-test files: silent.
  • go test -race -short -count=1 on ./internal/{api,config,mocks,server}/... and ./internal/database/postgres/migrations/...: ok.
  • go test -race -tags integration -count=1 on ./internal/config/, ./internal/database/postgres/migrations/ and ./internal/server/: all ok, full packages (config, migrations, server).
  • secrets-scan: "clean; planted key flagged". scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.

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>
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 4e646f56e1b87e94ca6940c2b58452a97b9e781d

Blocking:

  • none

Non-blocking:

  • internal/config/store_postgres_marketplace_settlement.go:33-37 (the WHERE in ListMarketplaceListingsToPoll): a row that holds a real listing id in listing_state = 'pending' is never polled. The list paths write that state when AWS reports no status and the follow-up describe fails (createdListingState, handler_marketplace_attempt.go:196), when AWS itself says pending (reconcileAttempt / createAndPersistListing store listing.State verbatim), and in keepUncanceledListing. RecordMarketplaceListingActive also never writes listing_state. So such a row stays pending for good: it is never moved to active, never settled, and its sale is never recorded. That state existed before this PR, but the poller is where it would get reconciled. Worth a follow-up: poll pending rows that have a listing id and let a poll that sees AWS active promote them, guarded on the same listing id.
  • Same class as the "Known window" in the body: a listing that the list or cancel path records directly as closed/cancelled under a new listing id (recordDeadListing, handler_marketplace_attempt.go:257, or ResolveMarketplaceListingAttempt resolving a recovered attempt) gets listed_at NULL. That NULL comes from the new CASE-clear, so the row is never selected. A partial or full sale in that path is not recorded. It is rare, and it is consistent with the documented trade-off. I mention it because the body only names the cancel-within-the-hour case.
  • Owner decisions the body raises, which I agree need the acting owner and not a reviewer: proceeds and fee are derived (sold × active tier price, less 12%), not read from disbursement reports. The poll runs only where finalize_revocations is scheduled (not on Fargate or CloudFormation). Turning off that schedule also turns off the poll.
  • Still open at this head, raised first by an earlier review (on cd13d82), which I confirmed against the current code:
    • The price depends on AWS still marking one tier Active after the listing has ended (handler_marketplace_poller.go:84-104). If it does not, every real sale lands in unpriced and is re-polled every hour.
    • A closed listing that reports 0 sold settles at $0/$0 and is never polled again (handler_marketplace_poller.go:117-121).
    • Legacy duplicate rows for one (purchase_id, listing_id) all receive the same proceeds, so any future SUM must dedupe.

Attacked:

  • Read the migration, the three store statements and the two relist CASE-clears. SET expressions read the old listing_id, so the clear only fires on a new id. Also read the poller, the server wiring, and the loadMarketplaceAWSConfig path with the partial rows the poller hands it.
  • Checked the settle and active guards against these races: cancel vs poll, double poll, relist vs poll, and a claim mid-relist (row pending, so it is left alone). Checked the cents math for proceeds + fee == gross. Checked that a nil embedded *FinalizeResult marshals cleanly.
  • Migration 000144 is nullable with no default (metadata-only, no table rewrite). Its down drops only its own three columns, and 000144 is the next free number above main's 000143.
  • Fail-before 1: reverse-applied only the non-test part of 19a1558 (the relist clear). TestMarketplaceListingRelist_Postgres then fails in RelistClosesBeforeFirstPoll, RelistCanceledBeforeFirstPoll and ResolvedAttemptDropsOldSettlement, and passes once restored.
  • Fail-before 2: a mutation recording proceeds gross fails ClosedRecordsProceedsAndFee, CanceledAfterPartialSale and TestMarketplaceSettlementAmounts.
  • Ran: go build ./..., go vet ./... and go vet -tags integration ./... (clean). go test -race -short -count=1 on api, config, mocks, server and migrations (ok). go test -race -tags integration -count=1 on config (258s), migrations (244s) and server (46s), all ok. golangci-lint v2.10.1 with and without integration tags: 0 issues. gocyclo -over 10 on the new non-test files: clean.

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.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 4e646f56e1b87e94ca6940c2b58452a97b9e781d merged onto main (df0de7e7), together with #1289, #1459 in that order (combined tree 2f911bc2, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 14:32
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 26, 2026 14:32
@IAmJSD
IAmJSD merged commit 731b23a into LeanerCloud:main Oct 26, 2026
41 checks passed
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

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 4e646f56, green CI, and the combined-tree verification above. Squash-merged at exactly 4e646f56e1b87e94ca6940c2b58452a97b9e781d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/l Weeks impact/few Limited audience priority/p2 Backlog-worthy severity/low Minor harm triaged Item has been triaged type/feat New capability urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant