test(api): pin the submit dedupe as a predicate, not a 1000-row page (closes #266)

[IAmJSD]

Summary

The bug in #266 is already fixed on main by #761 (commit 27c1a30, "make the purchase submit duplicate guard atomic", Refs #758). This PR adds no production change. It adds the regression tests that pin the fix, so #266 can close with evidence.

Root cause (as reported) and current state

• Before fix(api): make the purchase submit duplicate guard atomic #761: persistExecutionAndSuppressions read GetPendingExecutionsTx (status IN ('pending','notified') AND (expires_at IS NULL OR expires_at > NOW()) ORDER BY scheduled_date ASC LIMIT 1000 FOR UPDATE) and scanned the page in Go with matchDuplicateInList. With more than 1000 earlier-scheduled pending rows, the first submit fell off the page and an identical resubmit was inserted. That meant two approvable executions, so the commitment could be bought twice.
• On main now: the submit transaction takes pg_advisory_xact_lock('purchase-submit:'+creator) and then calls ListRecentSubmitsTx. That is a predicate query with no LIMIT: source = 'cudly-web' AND status IN (pending, notified, approved, running, completed, partially_completed) AND scheduled_date >= now-2m AND created_by_user_id IS NOT DISTINCT FROM $creator. matchDuplicateInList then compares the recomputed idempotency key, all inside the same transaction as the insert. The read is bounded by the rows that match, not by a page size. It no longer row-locks unrelated pending rows.

Predicate vs the Go scan (current code)

Dedupe component	SQL (ListRecentSubmitsTx)	Go (matchDuplicateInList)	Pinned by
Source = web	source = $3	ex.Source != PurchaseSourceWeb → skip	TestListRecentSubmitsTx_Predicate, unit table, key-semantics "not web-submitted"
Status (pending/notified/approved/running/completed/partially_completed; failed/canceled/cancelled/expired ignored)	status IN (submitDedupeStatuses)	— (SQL only)	TestListRecentSubmitsTx_Predicate, existing TestPersistExecution_RetryAfterRowLeftPending
Window start, inclusive (now − 2 min)	scheduled_date >= $1	ScheduledDate.Before(cutoff) → skip	TestListRecentSubmitsTx_Predicate (boundary row), unit table (boundary), key-semantics "outside the window"
Creator (NULL == "")	created_by_user_id IS NOT DISTINCT FROM $2	exCreator != creatorID → skip	TestListRecentSubmitsTx_Predicate, existing TestPersistExecution_DedupeIsPerCreator, unit table
Key: provider, service, region, resource type, engine, account, count, term, payment, AZ, platform, tenancy, capacity (and creator)	— (recomputed in Go from persisted recs)	purchaseIdempotencyKey(...) == key	TestPersistExecution_DedupeKeySemantics (each field changed alone; provider/payment case and whitespace are still duplicates)

The key itself is not pushed into SQL. purchase_executions.idempotency_key is the lineage anchor (migration 000066), not the submit fingerprint. Storing the fingerprint would need a new column plus a backfill from JSONB recs, and correctness does not need it because the read is already a bounded predicate.

Migration

None. ListRecentSubmitsTx uses the existing idx_purchase_executions_scheduled_date (000002) for the window range, and the per-creator advisory lock serializes submits. No index or migration number is claimed, so this PR does not interact with the ordering of #1020 (000127) or #1022 (000128).

Tests added

• internal/api/handler_purchases_submit_dedupe_integration_test.go (real Postgres via testcontainers):
  • TestPersistExecution_DedupeSeesPastThousandPendingRows: 1,100 plan-style pending rows (an hour earlier, non-web), plus 500 web rows from the same creator inside the window with a different key, plus the first submit. The identical resubmit collapses onto the first submit (1,601 rows, not 1,602).
  • TestPersistExecution_DedupeKeySemantics: 18 cases, each changing exactly one dedupe component.
  • TestListRecentSubmitsTx_Predicate: exact returned set per clause, including the inclusive window boundary and a NULL creator.
• internal/api/handler_purchases_dedupe_test.go: TestMatchDuplicateInList clause table (unit).

Verification

Run locally on macOS with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No cloud calls and no purchases.

• go build ./..., go vet ./..., go vet -tags integration ./...: clean
• go test -count=1 ./internal/api/ ./internal/config/ ./internal/mocks/: ok
• go test -tags integration -count=1 ./internal/api/: ok (whole package, 320s)
• golangci-lint v2.10.1 --allow-parallel-runners on ./internal/api/..., with and without --build-tags integration: 0 issues
• gocyclo: no non-test .go file changed
• git-secrets 1.3.0 --scan of both new files in a fresh repo with an isolated HOME: clean. bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL

Fail-before evidence: I temporarily put back the pre-#761 read (h.config.GetPendingExecutionsTx(ctx, tx) in place of ListRecentSubmitsTx) and TestPersistExecution_DedupeSeesPastThousandPendingRows fails. With main's code it passes.

Mutation probes (each applied alone, then reverted):

Probe	Result
SQL: drop source clause	killed (TestListRecentSubmitsTx_Predicate)
SQL: drop status IN (...)	killed (_Predicate, RetryAfterRowLeftPending)
SQL: drop window clause	killed (_Predicate)
SQL: >= → > on the window	killed (_Predicate)
SQL: drop creator clause	killed (_Predicate)
SQL: add LIMIT 100 page cap	killed (DedupeSeesPastThousandPendingRows)
Go: drop source check	killed (TestMatchDuplicateInList)
Go: drop window check	killed
Go: make the window boundary exclusive	killed
Go: ignore the key	killed
Go: drop the creator check	survives, as an equivalent mutant. The creator is hashed into purchaseIdempotencyKey, so the key comparison already separates creators.

Out of scope

• Removing the now-dead GetPendingExecutionsTx (interface, store and three mocks) and findDuplicatePendingExecution (which still scans the capped GetPendingExecutions page, but only its test calls it). This is tracked in chore(api): remove dead submit-dedupe code and lock the retry-successor path #760, together with the per-creator lock for persistRetrySuccessorTx.
• GCP dedupe wiring (fix(gcp): wire the recent-commitment duplicate checker into the platform #682).

Closes #266

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 29a05403-14fb-4b48-ad92-50e4e94681ca

📥 Commits

Reviewing files that changed from the base of the PR and between e3b4aa9 and 9e34d25.

📒 Files selected for processing: 2

• internal/api/handler_purchases_dedupe_test.go
• internal/api/handler_purchases_submit_dedupe_integration_test.go

+-----------------------------------------------------------+
| GPU shortage? The only thing I fear is a carrot shortage. |
+-----------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ 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.}

[cristim]

Independent review of 9e34d25: draft: blocks ready

No BLOCKING code findings. The PR is test-only (+251/-0, two test files).

#266 is already fixed on main (claim verified). persistExecutionAndSuppressions calls ListRecentSubmitsTx at internal/api/handler_purchases.go:3118, not GetPendingExecutionsTx. The query (internal/config/store_postgres.go:1772-1798) takes pg_advisory_xact_lock('purchase-submit:'+creator) and filters on source, submitDedupeStatuses, scheduled_date >= since and creator (IS NOT DISTINCT FROM). It has no LIMIT and no FOR UPDATE. This is the fix #266 itself proposed ("push the predicate into SQL"). matchDuplicateInList (handler_purchases.go:3070) compares the recomputed key in the same tx as the insert. Closing #266 is justified.

Findings

• NON-BLOCKING, handler_purchases_submit_dedupe_integration_test.go (DedupeSeesPastThousandPendingRows): a SQL LIMIT 1000 on the dedupe query survives. Only 500 rows match the predicate (the 1,100 plan rows are excluded by source), so a 1000-row page still holds them all. The test pins "unrelated rows do not count", not "no cap on matching rows". The PR body's probe used LIMIT 100. Fix: seed more than 1000 matching rows (same creator, web, in window, other key), or soften the test comment and PR text. A real 1000-matching-rows case needs more than 1000 web submits by one creator in 2 minutes, so this is theoretical.
• NON-BLOCKING: dead GetPendingExecutionsTx and findDuplicatePendingExecution remain (internal/config/interfaces.go:598-601). Already tracked in #760 and stated in the PR.

What I attacked (real Postgres via testcontainers, worktree at this SHA)

• All new integration tests plus RetryAfterRowLeftPending and ConcurrentIdenticalSubmits pass. The unit TestMatchDuplicateInList passes (10 tests).
• Probe 1, revert to the paged GetPendingExecutionsTx read at handler_purchases.go:3118: DedupeSeesPastThousandPendingRows FAILS, and so do RetryAfterRowLeftPending approved/running/completed/partially_completed. The test is not vacuous. The first submit lands past the 1000 earliest rows (1,100 plan rows an hour earlier plus 500 web rows).
• Probe 2, SQL LIMIT 1000: SURVIVES (see finding above).
• Probe 3, window >= to >: TestListRecentSubmitsTx_Predicate FAILS (boundary row).
• All probes reverted; the worktree is clean.
• Consistency with submitDedupeStatuses (store_postgres.go:1760): the status set in TestListRecentSubmitsTx_Predicate and RetryAfterRowLeftPending matches it. failed, canceled, cancelled and expired are excluded. I did not re-test the #925 frontend toast, which is out of scope for a test-only diff.
• Labels: all six triage labels plus triaged mirror #266. The PR body says Closes #266.

Gate status (origin/main Review gate)

• Independent review at this SHA: this comment.
• CI: all checks were pending when I looked, including Unit, Integration, Lint and pre-commit. Not yet green on this SHA.
• CodeRabbit skipped because the PR is a draft. I did not ping it.
• Local verification: done above on macOS. The PR body's own evidence is the author's claim.

Blocks ready

1. Mark the PR ready for review (it is still a draft).
2. CI green on 9e34d25 (Unit, Integration, Lint, pre-commit). Fork runs may need maintainer approval.
3. Optional: close the LIMIT 1000 gap above, or soften the test comment and PR text on that point. Any new commit restarts the gate.

[cristim]

Gate record for 9e34d25 (origin/main CLAUDE.md "Review gate"):

• Independent review of 9e34d25: no blocking findings.
  • The claim holds: fix(config): purchase dedupe scans a 1000-row page of pending rows, not a predicate #266 is already fixed on main by fix(api): make the purchase submit duplicate guard atomic #761. handler_purchases.go:3118 calls ListRecentSubmitsTx (store_postgres.go:1772-1798), which takes an advisory lock and applies the source/status/window/creator predicate with no LIMIT. Closing fix(config): purchase dedupe scans a 1000-row page of pending rows, not a predicate #266 is justified.
  • Non-blocking: a SQL LIMIT 1000 probe survives, because only 500 rows match the predicate. Seeding more than 1000 matching rows would catch it.
  • Non-blocking: dead GetPendingExecutionsTx is tracked in chore(api): remove dead submit-dedupe code and lock the retry-successor path #760.
• CI on 9e34d25: CI Success and Run pre-commit hooks pass, on a fresh pull_request run (38972702381) against current main.
• Local verification (macOS, testcontainers Postgres):
  • Reverting to the paged GetPendingExecutionsTx fails DedupeSeesPastThousandPendingRows and the RetryAfterRowLeftPending variants.
  • Changing the window >= to > fails the predicate test.
  • Test-only change.
• CR skipped: draft and quota-limited; covered by the independent review plus local verification.
• Labels mirrored from fix(config): purchase dedupe scans a 1000-row page of pending rows, not a predicate #266.

Marking ready (owner-authorized) and merging at 9e34d25 with --match-head-commit.