[cristim]

Part of LeanerCloud/cloud-commitments-go#700.

Deploy order

Migration 000162 must be applied before this code runs. It adds a unique index on purchase_attempts (reservation_id) WHERE state = 'purchased', and the new store code turns a violation of that index into a clear "already recorded as purchased by another attempt" error. The migration number is re-checked against main at merge time and renumbered if another migration lands first.

What

• Re-pin cloud-commitments-go from 0b677e1 to 52d66feb, which includes go#710. go#710 makes the Redshift purchase path skip nodes that another purchase lineage has already recorded as purchased, and makes reconcile report such nodes as Unknown, never NotFound. To do that, the provider asks the attempt journal through the optional common.ClaimReader.
• PostgresStore.ClaimedByOthers(ctx, token, ids) returns the reservation IDs that a different idempotency token recorded as purchased:

  SELECT DISTINCT reservation_id FROM purchase_attempts
  WHERE state='purchased' AND reservation_id = ANY($2) AND idempotency_token <> $1

  A compile-time assertion pins that *PostgresStore implements common.ClaimReader.
• Migration 000162:
  • It fails loudly if purchase_attempts already holds duplicate purchased reservation IDs. The error names them and says how to resolve them. Only then does it create the partial unique index.
  • The down migration drops the index.
  • RecordAttempt turns a violation of the index (a race, or same-execution twins) into a clear error naming the masked token, instead of a raw 23505.
• internal/purchase: ManagerConfig.AttemptJournal now requires the claim reader (AttemptJournal = common.AttemptJournal + common.ClaimReader), so a journal that cannot answer claims cannot be wired in. The post-DB manager gets the Postgres store; the pre-DB manager keeps nil.

Operator note

If 000162's duplicate pre-check fails, schema_migrations is left dirty at 162. Resolve the duplicate rows (keep the true buyer, delete or reclassify the rest), set CUDLY_FORCE_MIGRATION_VERSION=161, then re-run the migration.

Tests

• Integration tests (real Postgres):
  • TestClaimedByOthers: only another token's purchased IDs are returned, non-purchased states are ignored, and the view is symmetric per token.
  • TestClaimedByOthers_EmptyIDsNoQuery.
  • TestRecordAttempt_PurchasedReservationUniqueAcrossTokens.
• Migration tests:
  • TestMigration_0162_UniquePurchasedReservation: the index blocks a second purchased row, and the down migration drops it.
  • TestMigration_0162_PreCheckNamesDuplicates.
• The internal/purchase fake journal implements the claim reader.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 53 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: feat/700-platform-claim-reader
• File: .coderabbit.yaml
• Commit: 2862901
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 03ce536d-5900-4fd2-8853-867bc709ab2b

Commits

Reviewing files that changed from the base of the PR and between 97f82af and 2862901.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 8

• go.mod
• internal/config/store_postgres_attempts.go
• internal/config/store_postgres_attempts_integration_test.go
• internal/database/postgres/migrations/000162_purchase_attempts_unique_purchased.down.sql
• internal/database/postgres/migrations/000162_purchase_attempts_unique_purchased.up.sql
• internal/database/postgres/migrations/000162_purchase_attempts_unique_purchased_test.go
• internal/purchase/manager.go
• internal/purchase/requested_at_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVED 2862901 by claude-qa-ui-4

• Migration 000162: CREATE UNIQUE INDEX IF NOT EXISTS on purchase_attempts' purchased reservation_id, after a read-only DO-block pre-check that names duplicates; the down is DROP INDEX IF EXISTS. There is no backfill or NOT NULL change, and existing duplicates fail the up loudly (the body documents the dirty-at-162 operator path). The index is built without CONCURRENTLY on a table that only exists since 000161 and is small, so the write lock is brief and readers are not blocked. Main ends at 000161; another local branch also uses 000162, so whichever merges second renumbers.
• Deploy order: the code tolerates the pre-migration schema. ClaimedByOthers reads only 000161 columns, and without the index the 23505 branch simply never fires. Old code on the new schema is also safe: a doubled purchased write fails and is reported as outcome unknown. Migrations run at app startup (internal/server/app.go:641), so code and schema deploy together. The body's "must be applied before" is stricter than the code needs.
• ClaimedByOthers (internal/config/store_postgres_attempts.go:106-125) returns only the IDs the caller passes that another token recorded as purchased (idempotency_token <> $1), on the pool, outside any tx. The caller passes node IDs from its own account's DescribeReservedNodes (execution.go:834), so it cannot reveal or hide another account's nodes. In go@52d66feb a read error before the send refuses with no purchase, and a race ends in a unique violation that becomes outcome unknown.
• Go pin 0b677e1..52d66feb: 5dcf711 (ec2/redshift PurchaseReconciler, not consumed yet; the ec2 and redshift refactors have the same truth tables) and 52d66fe (go#710, Redshift dedupe skips nodes claimed by other tokens, wired via ClaimReader). pkg is unchanged. go mod verify passes and go mod tidy leaves no diff.

CI run 42414809389 at this head: green. Local integration (testcontainers) passes: TestRecordAttempt_* (4), TestClaimedByOthers (and the empty-ids case), TestMigration_0161, TestMigration_0162 (2). Probes, each killed:

• the token filter removed, and <> changed to = (TestClaimedByOthers);
• the 23505 translation disabled (TestRecordAttempt_PurchasedReservationUniqueAcrossTokens);
• the down made a no-op (TestMigration_0162_UniquePurchasedReservation).

Non-blocking:

• Disabling the DO-block pre-check (IF false) still passes TestMigration_0162_PreCheckNamesDuplicates, likely because the migration error echoes the SQL text and Postgres's own unique error names the duplicate. Asserting a string only the RAISE produces ("Resolve the duplicate rows"), or naming two duplicates, would pin it. Without the pre-check the CREATE UNIQUE INDEX still fails closed.
• The 23505 message says "by another attempt", but the index also blocks the same token under another execution_id; in go@52d66feb that only happens in a race.
• The same AWS account registered twice gives two lineage keys, so each buys its own node, as go#700 intends.

[cristim]

APPROVED 2862901 by claude-darling

Cold review; I did not write this change. This is a money path with a migration. Two low test gaps, nothing blocking.

Migration 000162

• 162 is main's highest + 1 today (main at 97f82af). feat(ri-exchange): list and resolve stuck direct-execution rows (follow-up to #1829) #1836 also takes 162, so whichever merges second renumbers.
• Up (000162_..._up.sql:8-28): a DO-block pre-check raises an error naming any duplicate purchased reservation_id, then runs CREATE UNIQUE INDEX IF NOT EXISTS uq_purchase_attempts_purchased_reservation ... WHERE state='purchased'.
  • Both statements are guarded, so it's idempotent.
  • There are no column, NOT NULL or CHECK changes, and no backfill.
  • golang-migrate runs the file as one transaction (migration_transactional_test.go:3-15), so a duplicate that slips in between the check and the build still fails the unique build.
• Lock: a non-CONCURRENTLY build takes a SHARE lock on purchase_attempts only. That table is new in 161 and holds one row per Redshift attempt, so the cost is negligible. purchase_executions is untouched.
• Down: DROP INDEX IF EXISTS. Only the index goes; no data is lost.

Deploy order. The new code doesn't depend on 162.

• ClaimedByOthers reads only columns that 161 created, and the 23505 mapping simply never fires without the index.
• Migrations run in-process before the post-DB purchase manager is wired (app.go:753-765). On the pre-162 schema nothing errors and nothing fails open; only the race backstop is missing.
• Old code on the new schema fails closed: a cross-token recordPurchased hits 23505, which surfaces as ErrOutcomeUnknown.
• So the body's "must be applied before" overstates it. It's advisory, not needed for correctness.

ClaimedByOthers (store_postgres_attempts.go:106-125)

• It reads purchase_attempts with state='purchased' AND reservation_id = ANY($2) AND idempotency_token <> $1. It's keyed on the AWS ReservedNodeId plus the token, with no tuple scoping, by design: reservation IDs are unique (go#700).
• The IDs come from the account's own DescribeReservedNodes, so a match from another tenant is a real purchase of that node, and excluding it is correct with nothing leaked.
• NULLs are ruled out by purchase_attempts_reservation_iff_purchased.
• Lineages: the token is the stable lineage key plus the rec index (execution.go:833,947). Retry keeps the key and the rec order (handler_purchases.go:2706,2734-2781). So a redrive keeps its token and isn't excluded from its own claim; that double-buy path is closed.
• A lookup error makes the provider refuse before sending (go dedupe.go:122-126).

Go delta 0b677e1..52d66feb

• 5dcf711: EC2 and Redshift ReconcilePurchase, purchasecfg helpers, and Redshift's paymentShapes refactor.
• 52d66fe: the claim exclusion in guardAgainstRepurchase and adoptAfterUnknownOutcome.
• pkg is unchanged. The new ReconcilePurchase/PurchaseReconciler have no platform callers yet. The interface narrowing (manager.go:55-64) compiles against *PostgresStore and the fake. Non-Redshift providers only gain the unused reconcile methods.

Findings (low)

1. TestMigration_0162_PreCheckNamesDuplicates (000162_..._test.go:72-73) doesn't prove the pre-check runs. Replacing IF dups IS NOT NULL with IF false still passes, because golang-migrate's Error() echoes the whole SQL text, RAISE literal included, and the unique-index failure supplies rn-dup. Assert SQLSTATE P0001, or values: rn-dup..
2. No test makes ClaimedByOthers fail on a query error, so turning it into return nil, nil survives. The dedupe would then adopt rather than exclude, which costs no extra spend.

Probes (scoped, each reverted)

• Killed:
  • the token filter dropped;
  • the index made non-unique (config and migrations);
  • a wrong constraint name;
  • the down migration made a no-op.
• Survived:
  • the pre-check disabled (finding 1);
  • lookup error to nil (finding 2);
  • the state filter dropped, an equivalent mutant because of the CHECK.

CI at this head: green, run 42414809389, including Unit Tests, Integration Tests (migrations, config, purchase) and Lint Code.

[cristim]

Merge gate record for 2862901:

• Approvals at this SHA: claude-darling, claude-qa-ui-4.
• CI at this SHA is green: CI - Build & Test 42414809389, pre-commit 42414809294. 0 unresolved threads.
• Combo re-test against current main a393f03 (fix(ri-exchange): bound the direct-execute claim and provider call by one pre-save deadline #1841 also touches internal/config), run locally on macOS with the merged tree: build ok; unit tests for internal/config, internal/purchase and internal/database/... ok; integration (real Postgres) TestRecordAttempt*, TestClaimedByOthers*, TestPriorAttempts* and TestMigration_016* ok.
• Migration 000162 is still the next free number on main (main ends at 000161).
• Deploy order: migration 000162 runs before this code.
• darling's two low test gaps (pre-check assertion, lookup-error path) go to a follow-up PR.