[IAmJSD]

What was still broken

Most of #267 was already handled by earlier merges, so this PR covers only what was left:

• New duplicates: fix(purchase): save purchase history once per commitment #776 (migration 000108, which addressed fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704) makes SavePurchaseHistory skip an existing (provider, account_id, purchase_id) and adds a partial unique index for rows created after the migration.
• The literal wedge: fix(marketplace): ambiguous AWS errors and crashes between create and persist can leave a duplicate or a stuck pending row #525 (c67bbb5) rewrote ClaimMarketplaceListingSlot to QueryRow ... RETURNING, so the RowsAffected() == 1 check the issue quotes is gone. On main, identical legacy duplicates no longer return 409 forever (the ClaimReleaseReclaimDoesNotWedge subtest passes on main too).

Rows saved before 000108 can still share a purchase_id. Deleting them is a pending owner decision (cristim on #704), so this PR deletes no rows and adds no migration. With such rows, main still gets these wrong:

1. GetPurchaseHistoryByPurchaseID uses LIMIT 1 with no ORDER BY, so it returns whichever duplicate the planner finds first. The marketplace-list and revoke handlers read listing state, offering class and account from that row.
2. ClaimMarketplaceListingSlot joins prev to the target on purchase_id, so every duplicate is joined to every other one. When the duplicates' listing state has diverged (for example the earliest row is active and a later twin was saved without it):
   • the claim succeeds on the twin, so the handler can send a second CreateReservedInstancesListing for an RI that is already listed
   • the returned PriorState and ListingID can come from a different row than the one claimed, and the release later writes that PriorState back

Fix (internal/config/store_postgres.go)

• GetPurchaseHistoryByPurchaseID orders by created_at, id, so it always returns the earliest row. That is the row fix(purchase): save purchase history once per commitment #776's "first row wins" kept.
• ClaimMarketplaceListingSlot:
  • prev locks only the earliest row (ORDER BY created_at, id LIMIT 1 FOR UPDATE), and the update joins on id. The claim, its prior state and its listing id all belong to that one row.
  • A NOT EXISTS refuses the claim while any duplicate records an active listing.
  • The other writers (UpdatePurchaseHistoryListing, ReleaseMarketplaceListingClaim, revocation) still match on purchase_id, so a recorded listing covers every duplicate. The release only matches rows that are pending with the caller's token, which now means only the earliest row.

Concurrency: concurrent claims all lock the same earliest row first. created_at and id never change, so the row prev picks stays the same while a claim waits. The claim holds one row lock and only reads the twin rows, so it adds no new lock-order deadlock with the all-rows UPDATEs. One race is the same as on main: if a claim and UpdatePurchaseHistoryListing interleave, the update overwrites the claim's pending state.

Verification

All evidence is local integration evidence against real Postgres (testcontainers, -tags integration, which CI runs). Since merging main, the new test takes its database from testhelpers.RequireMigratedDatabase, the package's shared container (#1165), like the other internal/config integration tests. Nothing touched real AWS. All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.

The new test TestPurchaseHistoryLegacyDuplicates_Postgres (internal/config/store_postgres_purchase_duplicates_integration_test.go) seeds a legacy duplicate with created_at before the index cutoff. The earliest row is stored second on disk, so an unordered lookup returns the later row.

go test -tags integration -run 'TestPurchaseHistoryLegacyDuplicates_Postgres|TestMarketplaceListingClaim_PostgresSemantics' -count=1 -v ./internal/config/

Subtest	origin/main store code	this PR
LookupReturnsEarliestRow	FAIL	PASS
ClaimRefusedWhenEarliestRowIsListed	FAIL (claims, leaves a row pending)	PASS
ClaimRefusedWhenAnyDuplicateIsListed	FAIL (claims, PriorState active taken from the twin)	PASS
ClaimReportsEarliestRowState	FAIL (wrong prior state/listing id; release overwrites)	PASS
ClaimReleaseReclaimDoesNotWedge	PASS (the wedge was already fixed by #525)	PASS
ConcurrentClaimWaitsAndResumes	FAIL*	PASS
ParallelClaimsShareOneAttempt (8 goroutines)	PASS	PASS
existing TestMarketplaceListingClaim_PostgresSemantics	PASS	PASS

* ConcurrentClaimWaitsAndResumes holds an open transaction that marks only the earliest row pending, which is the shape of the new claim. Its failure on main is not evidence of a race on main, because on main a real claim marks every duplicate. It is here to check the new locking.

Mutation probes (each reverted afterwards). Each one fails at least one subtest:

Mutation	Failing subtests
Drop the twin NOT EXISTS	ClaimRefusedWhenAnyDuplicateIsListed
Drop the lookup ORDER BY	LookupReturnsEarliestRow
ORDER BY created_at DESC in prev	ClaimReportsEarliestRowState, ConcurrentClaimWaitsAndResumes
Drop LIMIT 1 in prev, so every duplicate is claimed	ClaimReportsEarliestRowState

Other checks

• go test -tags integration ./internal/config/ -count=1: ok (340.9s)
• go test ./internal/config/ ./internal/mocks/ -count=1: ok
• go build ./..., go vet ./internal/config/ and go vet -tags integration ./internal/config/: clean
• golangci-lint v2.10.1 run --build-tags integration ./internal/config/...: 0 issues
• gocyclo -over 10 -ignore "_test\.go" internal/config/store_postgres.go: silent
• scripts/gosec-hook.sh on the changed files: exit 0
• git-secrets: full scan in an isolated fresh repo with the repo's patterns is clean, and scripts/test-git-secrets-allowlist.sh gives 42 PASS, 0 FAIL
• git diff --check: clean

Not covered: I did not run against a deployed database or a real AWS Marketplace listing, and I don't know how many legacy duplicates production holds.

Out of scope / left for a decision

• The issue suggests a full unique index on purchase_history(purchase_id) after a dedupe migration. That needs the legacy duplicates removed or rewritten, which is the pending owner decision on fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704, so this PR adds no migration.
• MarkPurchaseRevoked, FlipPurchaseRevocationInFlight, ClearRevocationInFlight, UpdatePurchaseHistoryListing and StampOfferingClass still match every row with the purchase_id. The duplicates describe the same commitment, so stamping all of them keeps them consistent. MarkPurchaseRevoked only touches rows with revoked_at IS NULL.
• The 000108 key is (provider, account_id, purchase_id), while these single-row helpers key on purchase_id alone. AWS RI IDs and Azure reservation IDs are globally unique, so I did not change that.
• Legacy duplicates still double count spend and savings in history and YTD views (noted in fix(purchase): save purchase history once per commitment #776).
• While a claim is pending, only the earliest row shows pending; its twins keep their old listing state until the next all-rows update. This only affects how history and list views show those twins.
• The marketplace cancel handler checks only the earliest row's listing state, while the claim also refuses when any twin is active. No current writer leaves only a twin active (the listing update and the release write every duplicate together), so this is theoretical; it belongs with the dedupe decision on fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704.

Note for whoever merges: this repo's squash message lists the commit messages, and the first commit's message (53b3c03) ends with a closing keyword for issue 267. Please delete that line from the squash message (or merge with an explicit message), because this PR only partly fixes that issue and it should stay open.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, triaged, urgency/this-quarter, impact/few, effort/m, type/bug.

Refs #267 (partial: the full purchase_id unique index and duplicate cleanup wait on the owner decision on #704)

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

VERDICT: APPROVE 53b3c03

Reviewed: full diff origin/main...53b3c03 (1 commit, 2 files, +298/-6). Rebase check: git merge-tree against origin/main b6a0649 is clean; every test below ran on the merged tree (973f447f).

Findings (none blocking):

• nit PR body - the sentence "fix(purchase): save purchase history once per commitment #776 (migration 000108, closes fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704)" is parsed by GitHub as a closing keyword: closingIssuesReferences is [704]. fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704 is already CLOSED, so merging does nothing to it, but the PR shows as closing fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704 when the intent is "Refs fix(db): duplicate purchase_history.purchase_id rows wedge the marketplace listing slot #267 (partial)". Rephrase it, for example "fix(purchase): save purchase history once per commitment #776, which resolved fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704".
• nit internal/api/handler_marketplace.go:376 - the cancel handler checks only the earliest row's listing_state, while the claim (internal/config/store_postgres.go:2414) also refuses when any twin is active. If only a later twin were active, list would get 409 from the claim and cancel would get 409 "no active listing", so the user could do neither. No current writer produces that state: UpdatePurchaseHistoryListing and the release write every duplicate together, so this is theoretical. Noting it for the eventual dedupe decision.
• nit (documented out of scope) - legacy duplicates are still counted twice by the readers that return rows: GetActivePurchaseHistory (handler_dashboard.go:641, handler_inventory.go:116, analytics/collector.go:96) and the history/YTD totals. FinalizeInFlightRevocations (internal/purchase/finalize_revocations.go:46) reports a doubled Found/Finalized count. Its second MarkPurchaseRevoked is an idempotent no-op. These wait on the fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704 dedupe decision, which the PR body acknowledges.
• nit process: the largest commit is 304 changed lines, under the 400 cap.

Attacked:

• SQL ordering and tie-break: created_at is TIMESTAMPTZ NOT NULL and id is the UUID PK (000001_initial_schema.up.sql:102,119), so ORDER BY created_at, id is total and deterministic, and no NULLS LAST case applies. prev and the lookup use the same key (store_postgres.go:2401, 2859), so the handler validates the same row it claims.
• Concurrency: prev locks one row, the twin NOT EXISTS is a plain read, and the all-rows writers must take the earliest-row lock, so no new lock cycle appears. If UpdatePurchaseHistoryListing commits first, the EPQ recheck sees the earliest row as active and refuses the claim.
• Readers of purchase_history by purchase_id: GetPurchaseHistoryByPurchaseID callers (handler_marketplace.go:199,362, handler_purchases_revoke.go:180,596), plus the release, revoke and in-flight writers. They all match every duplicate, so state stays uniform.
• Transitional rows left pending by the old claim (all duplicates carrying the same token): the new claim resumes on the earliest row, and the release still matches every pending row with that token.
• Mutation probes (fresh scratch export, real Postgres; each change was reverted and the restore checked with cmp):
  • dropped the lookup ORDER BY: LookupReturnsEarliestRow FAILS
  • dropped the twin NOT EXISTS: ClaimRefusedWhenAnyDuplicateIsListed FAILS
  • prev ORDER BY created_at DESC: ClaimReportsEarliestRowState and ConcurrentClaimWaitsAndResumes FAIL

Local results on the merged tree:

• go vet -tags integration ./internal/config/: clean
• go test -tags integration -run 'TestPurchaseHistoryLegacyDuplicates_Postgres|TestMarketplaceListingClaim_PostgresSemantics|PurchaseHistory|Marketplace' ./internal/config/ (testcontainers Postgres): ok (27s)

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 53b3c03

Findings (none blocking):

• [non-blocking] PR metadata - The PR is linked to close fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704, but the body says "Refs fix(db): duplicate purchase_history.purchase_id rows wedge the marketplace listing slot #267 (partial)".
  • This PR does not do fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704's dedupe migration, full unique index or root-retry refusal.
  • If fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704 is still open, merging closes it. Unlink it, or confirm fix(purchase): save purchase history once per commitment #776 already closed it.
• [non-blocking, disclosed] While a claim is pending, legacy duplicates diverge in history and list views.
  • Only the earliest row goes pending. The twins keep their old state until UpdatePurchaseHistoryListing rewrites every row (internal/config/store_postgres.go:2343-2360).
  • This is cosmetic.
• [non-blocking] A twin that is active while the earliest row is pending with a token gets a 409 until the next all-rows update. This requires the claim-vs-Update race the body lists as unchanged from main.

Attacked:

• Root cause: the claim's prev joined on purchase_id, which joined every duplicate to every other one. The fix:
  • locks only the earliest row (store_postgres.go:2397-2403);
  • updates by p.id = prev.id (:2411);
  • refuses when any twin is active (:2414-2418);
  • makes the lookup use ORDER BY created_at, id (:2859), the same row SavePurchaseHistory's "first wins" keeps.
• Lock order: the claim locks one row and only reads the twins, so it cannot form a cycle with the all-rows UPDATEs. created_at and id are immutable, so the earliest row cannot change while a claim waits.
• Mixed-version deploy:
  • An old instance's claim stamps every row with token A.
  • New code resumes A on the earliest row.
  • Release (purchase_id + token) clears all rows.
• Callers:
  • handler_marketplace_attempt.go:120 (claim).
  • Lookups at handler_marketplace.go:199 and :362, and handler_purchases_revoke.go:180 and :596.
  • mocks/stores.go. There are no other implementers.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All were caught:
  • M1, the twin NOT EXISTS dropped: ClaimRefusedWhenAnyDuplicateIsListed FAILS.
  • M2, the lookup ORDER BY dropped: LookupReturnsEarliestRow FAILS.
  • M3, join on p.purchase_id = $2: ClaimReportsEarliestRowState and ConcurrentClaimWaitsAndResumes FAIL.
  • M4, FOR UPDATE dropped in prev: ConcurrentClaimWaitsAndResumes FAILS.
  • M5, ORDER BY created_at DESC in prev: ClaimReportsEarliestRowState and ConcurrentClaimWaitsAndResumes FAIL.
• Main drift: merge-tree with origin/main is clean, and store_postgres.go has not changed on main since the base. Main added migrations 000131-000133, and 000133 touches purchase_history, so I also ran the tests on a merged export.
• No migration added. The highest on main is 000133.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • gofmt -l on both changed files: clean.
  • go build ./...: ok.
  • go vet on config and mocks, and with -tags integration on config: ok.
  • go test -count=1 ./internal/config/ ./internal/mocks/ ./internal/api/: ok.
  • go test -tags integration -count=1 -run 'TestPurchaseHistoryLegacyDuplicates_Postgres|TestMarketplaceListingClaim_PostgresSemantics' ./internal/config/: all 7 subtests and the existing claim test PASS.
• Fail-before, with the new test file on base:
  • LookupReturnsEarliestRow, ClaimRefusedWhenEarliestRowIsListed, ClaimRefusedWhenAnyDuplicateIsListed, ClaimReportsEarliestRowState and ConcurrentClaimWaitsAndResumes FAIL.
  • ClaimReleaseReclaimDoesNotWedge and ParallelClaimsShareOneAttempt pass. This matches the PR's table, including its note that fix(marketplace): ambiguous AWS errors and crashes between create and persist can leave a duplicate or a stuck pending row #525 already fixed the wedge.
• Merged export: build and vet ok, config and mocks unit tests ok, and the same integration run passes.
• Not run: the full -tags integration ./internal/config/ package.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Follow-up to VERDICT: APPROVE and Independent review 2 (both on 53b3c03). New head: 1dcdc2d.

Main merged in (merge commit, no rebase), no conflicts. After merging, main's TestIntegrationTestsShareOnePostgresContainer (#1165) failed on this PR's new test because it started its own container (store_postgres_purchase_duplicates_integration_test.go:29: testhelpers.RequirePostgresContainer).

• Fixed in internal/config/store_postgres_purchase_duplicates_integration_test.go:28-33. The test now gets its own migrated database from testhelpers.RequireMigratedDatabase(ctx, t, getTestMigrationsPath()), the same way setupTestContainerDB does, and it no longer imports migrations or runs a cleanup of its own. The test logic is unchanged.

Findings

• PR metadata / fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704 (both reviews): the body already says "which addressed fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704", and closingIssuesReferences is []. There's no sidebar link either: the timeline has no connected events. One closing keyword was left: the first commit's message (53b3c03) ends with a closing line for issue 267, and this repo's squash message is COMMIT_MESSAGES, so a default squash would close that issue. I can't amend a pushed commit, so the body now asks whoever merges to delete that line from the squash message. The new commit says Refs only.
• Pending state leaves twins showing their old state (review 2, cosmetic): no code change. It's now listed under the body's "Out of scope".
• Twin active while the earliest row is pending gets a 409 (review 2): no change. It needs the claim-vs-Update race the body already lists as unchanged from main.
• Cancel handler checks only the earliest row (internal/api/handler_marketplace.go, verdict nit): left alone, because no current writer can produce that state. It's noted in the body for the fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704 dedupe decision.
• Double counting by the row readers / FinalizeInFlightRevocations (verdict nit): this was already documented as out of scope, waiting on fix(purchase): skip duplicate purchase_history insert on re-drive and add a unique index #704.
• Commit size: still under the cap. The new commit changes 11 lines.

Verification on the merged tree (1dcdc2d), with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null:

• go build ./..., go vet ./internal/config/ and go vet -tags integration ./internal/config/: clean. gofmt -l: clean.
• TestIntegrationTestsShareOnePostgresContainer FAILS on the bare merge (the offender above) and PASSES with the fix.
• go test -tags integration -run 'TestPurchaseHistoryLegacyDuplicates_Postgres|TestMarketplaceListingClaim_PostgresSemantics|TestIntegrationTestsShareOnePostgresContainer' ./internal/config/: all 7 subtests and both other tests PASS.
• Mutation probe on the shared DB (I dropped the lookup's ORDER BY, then restored it and checked with cmp): LookupReturnsEarliestRow FAILS.
• Full go test -tags integration -timeout 20m -count=1 ./internal/config/, skipping TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck (a known failure on main, fixed in fix(config): compare resolve checked_at at the database's microsecond precision #1252): ok (282s). The first attempt hit a 60s template-creation timeout while the host was under load (environmental); the rerun was clean.
• go test ./internal/config/ ./internal/mocks/ -count=1: ok.
• golangci-lint v2.10.1 on ./internal/config/..., with and without --build-tags integration: 0 issues. gocyclo -over 10 on store_postgres.go: silent. git diff --check: clean.
• git-secrets: I ran the repo's setup in an isolated fresh repo with both changed files tracked: clean. A planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

The PR stays a draft. The govulncheck failures (GO-2026-6809) aren't related to this PR and are covered by #1236.

🤖 Generated with Claude Code