fix(purchases): atomic CAS cancel to close TOCTOU with concurrent approve (closes #671)

[cristim]

Summary

• Both cancel paths (cancelPurchaseViaSession in handler_purchases.go and CancelExecution in approvals.go) had a TOCTOU race: an out-of-transaction IsCancelable() check followed by an unconditional SavePurchaseExecutionTx upsert. A concurrent approve that transitioned the row to approved between the check and the upsert would be silently overwritten, leaving the DB at cancelled while the AWS/Azure/GCP commitment is real and billable.
• The approve path (ApproveAndExecute) already used the atomic TransitionExecutionStatus CAS guard; cancel now uses the same pattern via a new CancelExecutionAtomic store method.
• The misleading "optimistic-locking guard inside the tx" comment on handler_purchases.go:524 (which claimed a guard existed when it did not) is replaced with an accurate description.

What changed

internal/config/store_postgres.go - New CancelExecutionAtomic(ctx, tx, executionID, cancelledBy) method: conditional UPDATE ... WHERE status IN ('pending','notified') returning (cancelled bool, currentStatus string, err error). Zero rows affected -> follow-up SELECT to surface the racing status.

internal/config/interfaces.go - Add CancelExecutionAtomic to StoreInterface.

internal/api/handler_purchases.go - cancelPurchaseViaSession: replace unconditional SavePurchaseExecutionTx with CancelExecutionAtomic inside WithTx; 409 when CAS returns cancelled=false with the racing status in the body. Fix misleading comment.

internal/purchase/approvals.go - CancelExecution: same replacement. Returns a descriptive error with the racing status on CAS miss.

internal/mocks/stores.go, internal/api/mocks_test.go, internal/purchase/mocks_test.go, internal/scheduler/scheduler_test.go, internal/server/test_helpers_test.go, internal/analytics/collector_test.go - Add CancelExecutionAtomic to all mock implementations that satisfy StoreInterface.

internal/purchase/approvals_test.go, internal/api/handler_purchases_test.go, internal/purchase/coverage_extra_test.go - Update existing cancel tests to assert on CancelExecutionAtomic instead of SavePurchaseExecution. Add two new regression tests:

• TestManager_CancelExecution_RaceWithApprove (token path)
• TestHandler_cancelPurchase_Session_RaceWithApprove (session path)

Test plan

• go test ./internal/api/... ./internal/purchase/... ./internal/config/... ./internal/scheduler/... ./internal/server/... ./internal/analytics/... -count=1 -short - 2479 tests pass
• go build ./... - clean build
• go vet ./... - no issues
• TestManager_CancelExecution_RaceWithApprove confirms the token path returns a descriptive error when the CAS loses
• TestHandler_cancelPurchase_Session_RaceWithApprove confirms the session path returns 409 with racing status in body
• All pre-existing cancel tests updated and passing

Closes #671
Refs #632, #667, #669

Summary by CodeRabbit

• Bug Fixes
  • Improved purchase cancellation reliability by using atomic database operations, preventing data inconsistencies when cancellations race with concurrent approvals.
  • Enhanced concurrency handling to detect conflicting operations and return appropriate conflict responses.
  • Added audit tracking to record who cancelled each purchase execution.

[coderabbitai]

Warning

Rate limit exceeded

@cristim has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 2 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3bca4131-125b-4ebc-a1c2-502338adb42f

📥 Commits

Reviewing files that changed from the base of the PR and between aad614b and 892b13d.

📒 Files selected for processing (13)

• internal/analytics/collector_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/mocks_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/mocks/stores.go
• internal/purchase/approvals.go
• internal/purchase/approvals_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/mocks_test.go
• internal/scheduler/scheduler_test.go
• internal/server/test_helpers_test.go

📝 Walkthrough

Walkthrough

This PR introduces atomic execution cancellation to prevent a TOCTOU race where concurrent approve and cancel operations can silently overwrite an already-approved purchase status with "cancelled". Both session-authenticated (handler) and token-authenticated (manager) cancel paths now use conditional database updates instead of unconditional persistence.

Changes

Atomic Execution Cancellation to Fix TOCTOU Race

Layer / File(s)	Summary
Interface contract and method signature internal/config/interfaces.go	StoreInterface defines new CancelExecutionAtomic method: atomic status flip from pending/notified to cancelled within a transaction, returning (cancelled bool, currentStatus string, err error).
PostgresStore atomic cancellation implementation internal/config/store_postgres.go	CancelExecutionAtomic performs atomic UPDATE ... WHERE status IN ('pending','notified') RETURNING status; on zero-row update, queries GetExecutionByID to return current status and report what concurrent operation won the race.
Session-authenticated handler cancel path internal/api/handler_purchases.go	cancelPurchaseViaSession now uses CancelExecutionAtomic with cancelledBy from session email, runs single transaction with conditional suppression delete, and returns 409 with current status when atomic cancel reports zero rows affected.
Token-authenticated manager cancel path internal/purchase/approvals.go	CancelExecution rewritten to use atomic CancelExecutionAtomic instead of unconditional save; constructs nullable cancelled_by, conditionally deletes suppressions only on successful transition, returns early if concurrent operation already transitioned row.
Session-authenticated handler cancel tests internal/api/handler_purchases_test.go	Updated runSessionCancelAllowed to validate and capture atomic cancelledBy attribution; added TestHandler_cancelPurchase_Session_RaceWithApprove regression test; reworked deep-link bypass tests to exercise atomic cancel branch.
Token-authenticated manager cancel tests internal/purchase/approvals_test.go	Updated success and allowed-status tests to mock CancelExecutionAtomic and verify invocation; added TestManager_CancelExecution_RaceWithApprove regression test validating error includes racing status; asserts atomic method not called when status guard rejects or token expired.
Mock implementations across test suites internal/api/mocks_test.go, internal/mocks/stores.go, internal/purchase/mocks_test.go, internal/scheduler/scheduler_test.go, internal/analytics/collector_test.go, internal/server/test_helpers_test.go	Added CancelExecutionAtomic mock methods with testify delegation or hardcoded stubs across six test files to support atomic cancellation validation.
Integration test coverage alignment internal/purchase/coverage_extra_test.go	Updated TestProcessMessage_CancelHappyPath to expect CancelExecutionAtomic call with captured actor email, aligning message-processing cancel test with atomic flow.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly Related PRs

• LeanerCloud/CUDly#216: Modifies the session-based cancel branch selection in cancelPurchaseViaSession, overlapping the same handler method that this PR refactors to use CancelExecutionAtomic.
• LeanerCloud/CUDly#145: Updates the session-authenticated cancel flow to stamp CancelledBy and clear purchase_suppressions, mirroring the same operations that this PR now coordinates via atomic CancelExecutionAtomic.
• LeanerCloud/CUDly#648: Introduces IsCancelable() and status-matrix guards on the same cancellation paths that this PR protects with atomic conditional updates.

Suggested Labels

urgency/now, effort/m

Poem

🐰 A race was won by cunning flips,
Where "approved" met "cancelled" at the lips,
But now atomic guards stand fast,
No status overwrites the past!
Concurrent foes must lose the fight,
One victor claims the truthful right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: fixing a TOCTOU race condition in purchase cancellation by implementing atomic CAS, which directly aligns with the changeset's core objective.
Linked Issues check	✅ Passed	The PR fully addresses all coding requirements from #671: both cancel paths now use atomic CancelExecutionAtomic with conditional UPDATE; 409 responses surface racing status; misleading comment is fixed; regression tests for concurrent approve/cancel races are added.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to the TOCTOU race fix: mock additions across test suites support testing the new atomic method, and handler/business-logic updates implement the core CAS strategy without unrelated modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-671-cancel-toctou

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

internal/api/handler_purchases_test.go (1)

1607-1624: ⚡ Quick win

Assert suppression cleanup on successful atomic cancel.

This helper now pins CancelExecutionAtomic, but it doesn’t also pin the success-side suppression cleanup. Add an explicit DeleteSuppressionsByExecutionTx expectation + assertion here so the transactional contract is fully covered.

♻️ Suggested test tightening

 func runSessionCancelAllowed(t *testing.T, exec *config.PurchaseExecution, session *Session, hasAny, hasOwn bool) {
   t.Helper()
   handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, hasAny, hasOwn)

+  mockConfig.On("DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, cancelExecID).
+    Return(nil)
+
   var capturedCancelledBy *string
   mockConfig.On("CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything).
     Run(func(args mock.Arguments) {
       if v, ok := args.Get(3).(*string); ok {
         capturedCancelledBy = v
       }
     }).
     Return(true, "cancelled", nil)

   result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "")
   require.NoError(t, err)
   assert.Equal(t, "cancelled", result.(map[string]string)["status"])
   mockConfig.AssertCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything)
+  mockConfig.AssertCalled(t, "DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, cancelExecID)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_purchases_test.go` around lines 1607 - 1624, The test
sets an expectation for CancelExecutionAtomic but misses asserting the
corresponding success-path suppression cleanup; add a mock expectation on
mockConfig for DeleteSuppressionsByExecutionTx (matching the same
transaction/context and cancelExecID) before invoking handler.cancelPurchase,
return a successful result from that mock, and after the call assert that
mockConfig.AssertCalled(t, "DeleteSuppressionsByExecutionTx", ...) was invoked
so the transactional contract between CancelExecutionAtomic and
DeleteSuppressionsByExecutionTx is fully covered.

internal/purchase/approvals_test.go (1)

693-697: ⚡ Quick win

The race test should explicitly pin “no cleanup write” behavior.

The test currently verifies CAS miss/error text, but not the suppression-cleanup non-call. Add an explicit negative assertion for DeleteSuppressionsByExecutionTx (and make the purchase mock record that call path if needed) so this regression can’t silently drift.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/purchase/approvals_test.go` around lines 693 - 697, Add an explicit
negative expectation that DeleteSuppressionsByExecutionTx was NOT called in the
race test: after exercising the in-flight approve path and before/after
store.AssertExpectations(t), add an assertion on the purchase mock (or store
mock) to Verify/Assert that DeleteSuppressionsByExecutionTx was never invoked,
and if the mock currently doesn't record that method path, update the purchase
mock to include a no-call expectation/recorder for
DeleteSuppressionsByExecutionTx so the test fails if a cleanup write occurs;
keep the existing CAS miss/error assertions and CancelExecutionAtomic assertions
unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 536-548: The transaction error is being translated into a 409
client error; only the !cancelled path should return a 409. Change the outer
error handling for h.config.WithTx(...) so that if WithTx returns an error
(i.e., CancelExecutionAtomic or DeleteSuppressionsByExecutionTx failed inside
the transaction) you return/bubble an internal server error (5xx) or the
original error instead of NewClientError(409,...). Keep the existing
NewClientError(409,...) solely for the case when cancelled == false (the
concurrency/conflict case) and let real storage/tx errors surface as
server/internal errors.

---

Nitpick comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 1607-1624: The test sets an expectation for CancelExecutionAtomic
but misses asserting the corresponding success-path suppression cleanup; add a
mock expectation on mockConfig for DeleteSuppressionsByExecutionTx (matching the
same transaction/context and cancelExecID) before invoking
handler.cancelPurchase, return a successful result from that mock, and after the
call assert that mockConfig.AssertCalled(t, "DeleteSuppressionsByExecutionTx",
...) was invoked so the transactional contract between CancelExecutionAtomic and
DeleteSuppressionsByExecutionTx is fully covered.

In `@internal/purchase/approvals_test.go`:
- Around line 693-697: Add an explicit negative expectation that
DeleteSuppressionsByExecutionTx was NOT called in the race test: after
exercising the in-flight approve path and before/after
store.AssertExpectations(t), add an assertion on the purchase mock (or store
mock) to Verify/Assert that DeleteSuppressionsByExecutionTx was never invoked,
and if the mock currently doesn't record that method path, update the purchase
mock to include a no-call expectation/recorder for
DeleteSuppressionsByExecutionTx so the test fails if a cleanup write occurs;
keep the existing CAS miss/error assertions and CancelExecutionAtomic assertions
unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 83d37202-ae81-494a-a117-e143fee86af1

📥 Commits

Reviewing files that changed from the base of the PR and between 6fa5048 and aad614b.

📒 Files selected for processing (13)

• internal/analytics/collector_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/mocks_test.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/mocks/stores.go
• internal/purchase/approvals.go
• internal/purchase/approvals_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/mocks_test.go
• internal/scheduler/scheduler_test.go
• internal/server/test_helpers_test.go

[cristim]

CR follow-up (commit 892b13d)

Finding	File	Verdict	Action
Transaction errors coerced into 409	handler_purchases.go:548	Actionable (major)	Fixed: WithTx failures now return fmt.Errorf (5xx); only the !cancelled path returns NewClientError(409, ...)
Missing DeleteSuppressionsByExecutionTx expectation on success path	handler_purchases_test.go:1607-1624	Actionable (nitpick)	Fixed: added .On("DeleteSuppressionsByExecutionTx", ...) + AssertCalled in runSessionCancelAllowed; also added expectations to TestManager_CancelExecution, TestManager_CancelExecution_AllowsCancelableStatus, and TestProcessMessage_CancelHappyPath (all success-path cancel tests)
No negative assertion for DeleteSuppressionsByExecutionTx in race test	approvals_test.go:693-697	Actionable (nitpick)	Fixed: updated purchase MockConfigStore.DeleteSuppressionsByExecutionTx to call m.Called for testify tracking; added AssertNotCalled after AssertExpectations in TestManager_CancelExecution_RaceWithApprove

All 1892 tests pass; go build ./... clean.

@coderabbitai review

[coderabbitai]

@cristim: Thanks for the thorough follow-up — all three findings (major and nitpicks) look properly addressed based on your summary:

• ✅ 5xx/409 separation in handler_purchases.go — WithTx failures now correctly surface as server errors rather than being swallowed into a 409.
• ✅ DeleteSuppressionsByExecutionTx success-path expectations — added across runSessionCancelAllowed, TestManager_CancelExecution, TestManager_CancelExecution_AllowsCancelableStatus, and TestProcessMessage_CancelHappyPath.
• ✅ Negative assertion in race test — DeleteSuppressionsByExecutionTx mock now routes through m.Called and AssertNotCalled guards the race regression.

Triggering a review of commit 892b13d95 now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.