[IAmJSD]

What

Two follow-ups left by #761 (atomic submit dedupe, #758):

1. Race: the retry successor bypassed the submit dedupe. persistRetrySuccessorTx inserts a cudly-web pending row for the retrying user with the failed row's recommendations and capacity. That is the same fingerprint an identical fresh submit by that user carries. It took no per-creator submit lock and ran no duplicate check, so a retry racing an identical submit (or arriving just after one) left two approvable pending rows for the same purchase.
2. Dead code. findDuplicatePendingExecution and GetPendingExecutionsTx have had no production caller since fix(api): make the purchase submit duplicate guard atomic #761 moved executePurchase onto ListRecentSubmitsTx.

Root cause and fix

The retry transaction (now writeRetrySuccessorTx, called from persistRetrySuccessorTx) does this:

1. First statement: ListRecentSubmitsTx(creator, now-2m). This takes the same pg_advisory_xact_lock('purchase-submit:'+creator) at the same point as persistExecutionAndSuppressions, then reads the creator's live web submits. The creator is the successor's CreatedByUserID (the retrier), and is empty for API-key sessions, exactly as on the submit side.
2. Then the ladder tranche guard, successor insert and link CAS, all unchanged.
3. After the link CAS: if a recent row has the successor's fingerprint (purchaseIdempotencyKey + matchDuplicateInList, the same predicate the submit uses), the tx rolls back. The caller gets 409 an identical purchase was submitted moments ago as execution X; act on it instead of retrying Y with details.duplicate_execution_id = X. No suppressions are written and no approval email is sent.

Why the lock alone is not enough: with the lock and no check, a submit that commits first is followed by a retry that still inserts. The mutation probe below shows this. Why the check alone is not enough: without the lock, both sides read before either commits. The probe below shows this too.

Design notes:

• Lock order. The advisory lock is taken before the ladder tranche row lock. The submit path takes only the advisory lock and then inserts. The reconciler takes tranche, then execution rows, and never takes the advisory lock. So no path holds tranche or row locks while waiting on the advisory lock, and no new cycle exists. guardLadderTrancheForRetry's comment is updated to say this.
• Why the check runs after the link CAS. Two same-user retries of the same failed row now also serialize on the advisory lock. Checking after the CAS means the loser still gets the existing, more specific already retried; act on its descendant 409, not the duplicate 409. Existing tests that assert that message pass unchanged.
• Retry vs submit outcome. If the retry wins, the submit collapses onto the successor, as it already did on main for a committed successor. If the submit wins, the retry gets a 409 and the failed row stays unlinked, so it can still be retried after the 2-minute window. I chose a refusal over collapsing the retry onto the submit's row because that row has a fresh lineage idempotency_key, not the predecessor's (fix(plans): refuse Add Purchases and account changes on managed plans #1012). Linking the failed row to it would misstate the lineage.
• Fail closed. If the successor's fingerprint cannot be derived (undecodable EC2/compute Details), the retry fails with a 500 rather than skip the check. The same recs already fail key derivation on the submit side.
• Status window. The retry uses the submit's status set (pending, notified, approved, running, completed, partially_completed). A retry within 2 minutes of an identical completed direct buy by the same user is refused. That is the same double-buy that fix(api): make the purchase submit duplicate guard atomic #761 collapses on submit.

Dead code removed: findDuplicatePendingExecution; GetPendingExecutionsTx from config.StoreInterface, PostgresStore, mocks.MockConfigStore, the internal/server fake (mockConfigStoreForHealth) and the internal/analytics fake (mockConfigStore). Its two lines in the created_by_system integration test are dropped; the other readers there still cover the flag. Before removing it I grepped the repo and the diffs of all 80 open PRs. None reference GetPendingExecutionsTx, findDuplicatePendingExecution, persistRetrySuccessorTx, ListRecentSubmitsTx or matchDuplicateInList.

Test move: TestFindDuplicatePendingExecution is deleted. TestMatchDuplicateInList (added in #1030) already covers its window, creator, source and rec-set cases. The two cases it did not cover are now in TestMatchDuplicateInList_ReturnsTheMatchingRow: the scan returns the matching row by ID from a mixed list, and an undecodable candidate is skipped without ending the scan. The "lookup error is surfaced" case now runs against the live path in TestPersistExecution_RecentSubmitsErrorIsSurfaced.

Mock change: MockConfigStore.ListRecentSubmitsTx with neither ListRecentSubmitsTx nor GetPendingExecutions registered now returns no rows instead of panicking. Without that, 19 existing retry unit tests would each need a stub, because the retry path now reads recent submits. Submit tests that register GetPendingExecutions behave as before.

No migration. No frontend change. The 409 is a normal client error, and the retry button already surfaces client-error messages.

Verification

All local on macOS, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. No cloud calls, purchases or email.

Race test on real Postgres (testcontainers, Postgres 16, migrations to 000133): internal/api/handler_purchases_retry_submit_race_integration_test.go, TestRetrySuccessorRacesFreshSubmit. Each round seeds a failed web execution owned by a real user. It then runs persistRetryExecution (the production successor builder) and persistExecutionAndSuppressions for an identical fresh submit by the same user concurrently. Rounds alternate which side starts first. Each round asserts:

• exactly one pending row;
• no 40P01 on either side;
• the outcome is consistent: either the retry succeeded and the submit returned it as the duplicate, or the submit inserted and the retry got the duplicate 409.

There are two modes:

• forced interleaving: 8 rounds. Each tx is held at its INSERT until both arrive (300ms cap), and the leader starts 50ms early. The test asserts both orders occurred (retry won 4, submit won 4).
• free race: 100 rounds, with a random lag under 0.5ms (for example, retry won 37, submit won 63).

go test -tags integration -count=5 -v -run TestRetrySuccessorRacesFreshSubmit ./internal/api/   # ok, 5/5 runs

Fail before (handler file swapped for origin/main's, test unchanged): both modes FAIL in round 0 with expected: 1, actual: 2 pending rows.

Mutation probes (each applied alone, then reverted):

Probe	Result
origin/main handler (no lock, no check)	killed: 2 pending rows, round 0 (both modes)
lock kept, duplicate check removed	killed: 2 pending rows in submit-first rounds (round 1 forced, round 0 free)
check kept, recent submits read in a separate tx (lock released before insert)	killed: 2 pending rows (round 0 forced, round 8 free)
unit, origin/main handler	RefusesIdenticalLiveSubmit and RecentSubmitsErrorFailsRetry fail

Other tests (one package at a time):

• go test -short -count=1 for ./internal/api/, ./internal/config/, ./internal/mocks/ (repo-wide test guards), ./internal/server/, ./internal/analytics/, ./internal/purchase/: ok
• go test -race -short -count=1 -run 'Retry|Dedupe|Duplicate|PersistExecution' ./internal/api/: ok
• go test -tags integration -count=1 ./internal/api/ (whole package, 270s, including the existing retry, ladder-reconcile, managed-plan race and submit-dedupe integration tests): ok
• go test -tags integration -count=1 -run CreatedBySystem ./internal/config/: ok
• New unit tests in internal/api/handler_purchases_retry_dedupe_test.go (mock store): an identical live submit gets a 409 with duplicate_execution_id, no suppressions and no email; a different live submit lets the retry through; a read error fails the retry before any write.

Static checks:

• go build ./..., go vet ./..., go vet -tags integration ./internal/...: clean
• golangci-lint v2.10.1 --allow-parallel-runners on api/config/mocks/server/analytics, with and without --build-tags integration: 0 issues
• gocyclo -over 10 -ignore "_test\.go" internal/: silent (writeRetrySuccessorTx is 9)
• gosec hook (v2.28.0) on the changed packages: clean
• git-secrets 1.3.0 --scan of every changed file, run in a fresh git init repo with an isolated HOME and current main's setup-git-secrets.sh and .gitallowed: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL

Evidence level: real Postgres through a test harness, plus mock-store unit tests. I did not run this against the deployed stack or through the browser UI.

Out of scope

• The other inserters of execution rows (scheduler and plan steps) are not web-source and do not go through the submit dedupe. That is unchanged.
• docs/audits/codebase-audit-2026-09-02.md still mentions the removed functions (A01-015, A04-005). I left it alone because it is a dated audit record.
• A stray // executePurchase handles direct purchase execution from recommendations comment sits above matchDuplicateInList. It is pre-existing, so I did not touch it, to keep the diff clear of sec(api): bound update-any by its Constraints on planned-purchase run #1192.

Maintainer: please mirror the issue's triage labels on this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/s.

Closes #760

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 11 billable files and costs up to $2.75.}

Or wait 8 minutes for your next included review.

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: main
• File: .coderabbit.yaml
• Commit: 5d4ab6b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2d4e4650-3267-46a8-9306-311372f1fc38

Commits

Reviewing files that changed from the base of the PR and between 5d4ab6b and f30184e.

Files selected for processing: 11

• internal/analytics/collector_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_dedupe_test.go
• internal/api/handler_purchases_guards_test.go
• internal/api/handler_purchases_retry_dedupe_test.go
• internal/api/handler_purchases_retry_submit_race_integration_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_created_by_system_integration_test.go
• internal/mocks/stores.go
• internal/server/test_helpers_test.go

• 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 2 (claude-fleet, cold): APPROVE f30184e

Findings (none blocking):

• [non-blocking] internal/api/handler_purchases.go writeRetrySuccessorTx (head ~2602-2611) - No test pins the order "link CAS before the dupe check".
  • Moving checkRetryDuplicateSubmit above if !linked (probe P6) keeps every unit and integration test green.
  • Under that change, a retry that loses the race while a duplicate also exists would return the "identical purchase" 409 instead of "already retried".
  • Both are 409s with no write, so money is safe. Only the ordering the doc comment promises is unpinned.
• [non-blocking] Same function - No test pins "per-creator submit lock before the tranche lock". Swapping them (P3) keeps the full Retry|Ladder integration set green.
  • I checked for a lock cycle either way and found none.
  • Only the submit path (handler_purchases.go:3397) and this retry transaction take the advisory lock.
  • The reconciler (purchase/ladder_reconcile.go:127, store_postgres_ladder_resolve.go:94) takes tranche locks and never the advisory lock.
  • persistExecutionAndSuppressions always opens its own WithTx, never inside a transaction that holds a tranche lock.
  • So the claim holds, but nothing tests it.
• [non-blocking, test-only] internal/mocks/stores.go ListRecentSubmitsTx - When neither ListRecentSubmitsTx nor GetPendingExecutions is registered, it now returns (nil, nil) silently.
  • Before, the unregistered call failed the test.
  • A future submit-path unit test could skip dedupe unnoticed. The PR does this on purpose.
• [non-blocking] The new 409 detail key duplicate_execution_id is not read by the frontend. history.ts and api/client.ts show the message generically, so there is no link to the existing execution.

Dead-code claim: confirmed.

• GetPendingExecutionsTx and findDuplicatePendingExecution appear nowhere in head or the merged tree (Go, mocks, fakes or frontend).
• The only remaining mentions are in comments in the new tests and in a docs/audits file.
• There is no .On("GetPendingExecutionsTx") or AssertCalled.
• The merged tree (origin/main 00a3f40 plus head, merge-tree clean) builds, which proves no Go caller remains.

Against #760:

• In base, persistRetrySuccessorTx took no lock and ran no check.
• The new race test fails on base: both subtests leave 2 pending rows in round 0.
• So the bug is real, and this fixes it at the right layer.
• ListRecentSubmitsTx already covers approved, running and completed, so the retry also sees in-flight submits.

Attacked:

• Concurrency probes on the testcontainers harness, using a scratch test that was removed afterwards:
  • Two concurrent retries by one user, of two different failed rows with identical recs, under the insert barrier, for 6 rounds: PASS.
    • Exactly 1 pending row and one 409.
    • Exactly 1 linked failed row. The loser rolls back and stays retryable.
    • No 40P01.
  • A retry by user X racing an identical submit by user Y, for 20 rounds: PASS. Both rows insert, with no false dedupe across creators and no deadlock.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • P1, the dupe check dropped: the unit RefusesIdenticalLiveSubmit and both integration race subtests FAIL.
  • P2, recent submits read in a separate transaction, so the lock is released: the forced and free races FAIL (the free race at round 34).
  • P3, the tranche lock taken before the submit lock: no failure (gap above).
  • P4, the matcher ignores Source: TestMatchDuplicateInList and _ReturnsTheMatchingRow FAIL.
  • P5, the dupe error not mapped to 409: RefusesIdenticalLiveSubmit FAILS.
  • P6, the dupe check before the link CAS: no failure (gap above).
• Edge cases:
  • A creator-less successor (admin API key with 4-eyes off) uses the shared purchase-submit: lock and IS NOT DISTINCT FROM NULL, the same as the submit path.
  • The failed predecessor is never in recent, because failed is excluded from submitDedupeStatuses.
  • The successor cannot match itself, because recent is read before the insert.

Verified locally on the merged tree, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• Build, vet and format: go build ./..., go vet and go vet -tags integration on api, config, mocks, server and analytics are OK. gofmt -l on the 11 changed files is clean.
• Unit tests: go test -count=1 on those 5 packages: ok.
• Integration tests:
  • -tags integration -run 'Retry|Ladder|Submit|PersistExecution' ./internal/api: ok. That includes the existing ladder, reconcile and managed-race retry tests, now under the new lock.
  • -tags integration -run 'CreatedBySystem|RecentSubmits' ./internal/config: ok.
  • -race -tags integration -count=3 -run TestRetrySuccessorRacesFreshSubmit: ok, with no data race. The forced case ran 4/4 in each order every run. The free race had the retry winning 18-24 of 100.
• Fail-before:
  • RefusesIdenticalLiveSubmit and RecentSubmitsErrorFailsRetry FAIL on base, and so does the integration race test.
  • The tests that only move existing coverage pass on base, as expected.
• No migration, frontend or terraform changes.

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

[cristim]

VERDICT: APPROVE f30184e

Independent adversarial review of PR #1217 (closes #760) at f30184e, base main. Delta reviewed: git diff origin/main...f30184eb.

The fix addresses both halves of #760. writeRetrySuccessorTx (internal/api/handler_purchases.go:2571) calls ListRecentSubmitsTx as its first statement, which takes the same purchase-submit:<creator> advisory lock as persistExecutionAndSuppressions (internal/config/store_postgres.go:1812). It then checks the successor's fingerprint against the rows it read, using the same purchaseIdempotencyKey + matchDuplicateInList predicate as the submit. The creator is newExecution.CreatedByUserID (resolveCreatorUserID(session)), which matches what the submit side keys on. Only failed rows are retryable (the link CAS), and failed is not in submitDedupeStatuses, so the predecessor never matches itself. The dead findDuplicatePendingExecution and GetPendingExecutionsTx are removed from the interface, the Postgres store, the shared mock and both package fakes. No Go references remain.

Findings (none blocking):

• nit internal/api/handler_purchases.go:2585 - The PR says the duplicate check runs after the link CAS so that a lost same-row retry race still gets "already retried". No test pins this. Moving the check to right after the read (before the guard and the CAS) passes every unit test. If someone later reorders it, a concurrent double-retry gets the less specific duplicate 409.
• nit internal/api/handler_purchases.go:2605 - The fail-closed branch for an undecodable fingerprint is untested. Mutating it to return nil (skip the check) passes every unit test. That branch is now the only thing keeping an undecodable-details retry from inserting unchecked.
• nit internal/api/handler_purchases_retry_dedupe_test.go:21 - Every fixture has the retrier equal to the original creator. Keying the lock on failedExec.CreatedByUserID instead of the successor's creator survives the tests. With retry-any, that would lock the wrong user's submits. One test where a different user retries would cover it.
• nit internal/mocks/stores.go:1802 - ListRecentSubmitsTx now returns no rows when neither method is registered, where it used to panic. A submit test that forgets its GetPendingExecutions stub now quietly sees "no duplicate" instead of failing. This is acceptable for the reason the PR gives, but it removes a tripwire.

Attacked:

• Lock order: the advisory lock comes before the tranche lock. The submit path takes only the advisory lock. No new lock cycle with the reconciler. The integration race test asserts no 40P01.
• Status set: the predecessor (failed) is excluded by the SQL. An identical completed buy by the same user within 2 minutes is refused on purpose, matching the submit.
• API-key sessions: an empty creator gives IS NOT DISTINCT FROM NULL on both sides. Consistent.
• Error mapping: errors.Is / errors.As survive the WithTx wrapping. The 409 carries duplicate_execution_id.
• Build and vet, including -tags integration, are clean on api and config. go test -short passes for internal/api (3929 tests), internal/config and internal/mocks.
• Integration (testcontainers Postgres): go test -tags integration -count=2 -run 'TestRetrySuccessorRacesFreshSubmit|Retry' ./internal/api/ ok (102s).
• Mutation probes (each reverted, file restored byte-identical):
  • Remove the duplicate check, unit: TestHandler_retryPurchase_RefusesIdenticalLiveSubmit FAILS (killed).
  • Remove the duplicate check, integration: TestRetrySuccessorRacesFreshSubmit FAILS in both modes, round 1, with 2 pending rows (killed).
  • Move the check before the CAS: all tests pass (survived, nit 1).
  • Skip the check when the key cannot be derived: all tests pass (survived, nit 2).
  • Key the lock on the original creator: all tests pass (survived, nit 3).

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_purchases.go:2606 (checkRetryDuplicateSubmit): the fail-closed key derivation means that a failed row whose EC2/compute Details cannot be decoded can no longer be retried (500). Before this PR it could. Rows like that come only from corrupt Details, since legacy and absent Details decode to "", and such a row would probably fail again downstream anyway. Still, it is a new way to fail on the retry path. The 500's message doesn't tell the operator why, so a 409 or 422 naming the undecodable rec might be kinder.
• The retry refusal also fires against a completed identical direct buy by the same user within 2 minutes. That matches the submit side (fix(api): make the purchase submit duplicate guard atomic #761) and is the safer choice on a money path. Once the window passes, the same retry goes through, which is also how the submit behaves.
• The MockConfigStore.ListRecentSubmitsTx fallback now returns nil, nil when neither expectation is registered. Before, this panicked, so no existing test can be silently weakened. A new submit-path test that forgets the stub will see "no duplicate" rather than a failure, though.

Attacked:

• Lock order. Retry: advisory purchase-submit:<creator>, then the ladder tranche FOR UPDATE, the successor insert and the failed-row link CAS. Submit: advisory lock, then insert. Reconciler: tranche, then rows, never the advisory lock. No path waits on the advisory lock while holding a tranche or execution row lock, so no new cycle.
• Checked the creator derivation (resolveCreatorUserID, nil for non-UUID/API-key sessions) and that the source (web) matches the submit side. ListRecentSubmitsTx's status set excludes failed, so the predecessor never matches itself. A rolled-back duplicate leaves the failed row unlinked, with no suppressions and no email.
• Fail-before on real Postgres: with only the checkRetryDuplicateSubmit call disabled (lock kept), TestRetrySuccessorRacesFreshSubmit fails in both modes with expected: 1, actual: 2 pending rows in round 1 (submit-first). Restored afterwards.
• Pass-after: go test -race -tags integration -run 'TestRetrySuccessorRacesFreshSubmit|TestRetry' passes the race test (forced: retry won 4, submit won 4; free: 15/85) and all the existing retry integration tests. Three ladder retry subtests (RefusesSettledLadderTranche/completed, FiredLadderTrancheStillRetries, NonLadderKeysRetryNormally/empty) failed on this loaded machine only at container start (a ryuk reaper in the "removing" state, or the wait-for-ready deadline). Rerun one at a time, all three pass.
• Also ran:
  • go build ./...;
  • go vet ./..., with and without -tags integration, plus the e2e vet;
  • go test -race -short -count=1 on api, config, mocks, server, analytics and purchase;
  • golangci-lint, with and without integration tags: 0 issues;
  • gocyclo: silent;
  • secrets-scan: clean.

Body-stale:

• "test-git-secrets-allowlist.sh: 42 PASS" is 137 now that main has grown. Immaterial.

_{Reviewer: rB2. 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]

Local verification at f30184eb27bc2dc62ae7384fec6331dae12016e0 merged onto main (26004a6d), together with #974, #1106, #1151, #1193, #1199, #1212, #1217, #1218, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1272, #1275, #1283, #1286, #1293, #1299, #1300, #1313 in that order (combined tree d3d95c83, 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 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on f30184eb (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly f30184eb27bc2dc62ae7384fec6331dae12016e0.