fix(purchases): close double-buy holes in idempotency, atomic claim, and partial-success labelling

[cristim]

Summary

Three interlocking money-path defects that could cause DUPLICATE real cloud commitment purchases (AWS RI/SP, Azure reservation, GCP CUD). They share the same files, so they land together. Source review: docs/code-review/05-purchase-execution.md (findings C1/H1, C2/C3/M2, H2/M3).

The single-account synchronous approve path was the only one with the "claim the row atomically before touching the cloud" guard; the other two executor entry points lacked it, and idempotency identity was tied to a per-attempt mutable key.

What changed

#1012 — idempotency identity independent of the execution ID

• Idempotency token derived from exec.ExecutionID, which retry regenerates (fresh UUID per successor) and multi-account fan-out regenerates (fresh UUID per account) — so a re-drive of a "failed"-but-landed execution derived a different token and the provider guard couldn't match → double-buy.
• Added an idempotency_key column (migration 000066, nullable), generated once at first creation, INSERT-only (never rewritten by ON CONFLICT, lockstep with the existing created_by_user_id/retry_attempt_n pattern).
• Per-rec token now derives from idempotencyLineageKey(exec) (the key, with an ExecutionID fallback for legacy NULL rows).
• Retry copies the key verbatim onto the successor; each multi-account row seeds its key from rootKey + ":" + accountID (deterministic, replaces uuid.New()).

#1013 — single atomic claim-then-execute for all entry points

• New claimAndExecute: CAS ["approved","pending","notified"] -> "running" via the existing TransitionExecutionStatus; only the winner proceeds, a lost CAS is a benign ack/skip (mirrors the reaper's race model).
• handleExecutePurchase (SQS, at-least-once redelivery) and ProcessScheduledPurchases (overlapping cron ticks) now funnel through it.
• Cron loop gains a positive-allowlist status guard (M2: only pending/notified proceed).

#1014 — typed multi-account result; partial success is not "failed"

• executeMultiAccount returns a typed *multiAccountPartialError (≥1 account committed) vs errAllAccountsFailed (nothing committed), instead of an opaque joined string.
• finalizeExecution maps the partial sentinel to partially_completed, never failed.
• The root row is now always saved with its aggregate status (fixes H2's fragile !wasMultiAccount save-skip, and prevents the new claim from stranding the root in running).
• SQS/cron callers ack (no redeliver) when ≥1 account committed.

How each fix was verified

Regression tests in internal/purchase/money_path_regression_test.go replicate the real failing scenarios and were confirmed to fail on pre-fix code and pass after:

• TestRetryReusesIdempotencyToken / TestMultiAccountSeedsStablePerAccountKey — retry + fan-out reuse the identical provider token / per-account key (fail pre-fix: tokens differ).
• TestLegacyRowFallsBackToExecutionID — legacy NULL-key rows keep working.
• TestSQSRedeliveryDoesNotDoubleExecute — two deliveries of the same message run the cloud purchase exactly once (fails pre-fix: runs twice).
• TestMultiAccountPartialSuccessIsAcked — a partial multi-account run is acked (handler returns nil) and the root is recorded partially_completed (fails pre-fix: handler errors → redeliver).

Build/vet/test: go build ./..., go vet ./internal/purchase/... ./internal/api/... ./internal/config/..., and go test ./internal/purchase/... ./internal/api/... ./internal/config/... all pass (2439 tests across 6 packages after fold). go test -race ./internal/purchase/... clean.

DB-backed integration tests were exercised via pgxmock (column scan order updated for the new column); no live Postgres was required.

Closes #1012
Closes #1013
Closes #1014

Scope expanded

Additional findings from the FOLD-1037 review pass (docs/code-review/16-remaining-findings-plan.md) have been folded into this PR. All touch files already modified by the original commits. Regression tests confirm pre-fix failures.

05-H3 — fanout semaphore ignores context cancellation (internal/execution/fanout.go)

sem <- struct{}{} in the goroutine-launch loop blocked unconditionally even when the context was already cancelled. A large fan-out after a deadline expiry could hang indefinitely. Fixed with a select that records ctx.Err() on the result slot and continues.
Regression test: TestFanOut_ContextCancelled_BlockedSemaphore (maxConcurrency=1, second item gets context.Canceled immediately after cancel).

05-M1 — recs[:0] slice aliasing (internal/scheduler/scheduler.go, scheduler_overrides.go)

applySuppressionIndex and filterRecsByResolvedConfigs wrote out := recs[:0], aliasing the caller's backing array. Replaced with make([]config.RecommendationRecord, 0, len(recs)).
Regression tests: TestApplySuppressionIndex_DoesNotMutateCallerSlice, TestFilterRecsByResolvedConfigs_DoesNotMutateCallerSlice.

05-M4 — commitmentopts silent permissive fallback (internal/commitmentopts/service.go)

Validate returned true without logging when the provider was known but the service was absent in probe data. Now logs at Warn so operators can detect probe gaps or misconfigured service names.

05-L1 — unqueryable audit-gap marker (internal/purchase/execution.go)

recordHistoryAuditGap now prefixes the note with history_write_failed: + commitment ID, making these entries filterable in CloudWatch Insights.

05-L2 — getAWSAccountID sentinel string (internal/purchase/execution.go)

getAWSAccountID now returns ("", error) on all failure paths instead of the "unknown" sentinel; the caller decides the fallback. Existing tests updated.

05-L3 — normalizePurchaseSource decision documented (internal/purchase/execution.go)

Added explanatory comment: the purchase proceeds untagged on invalid source (by design) because failing the rec over a tag-only field is a worse outcome.

05-N1 — dead _ = now in reaper (internal/purchase/reaper.go)

Wired now into the sweep log line (RFC3339 timestamp) so the injected-clock parameter is actually used and the _ = now suppressors are removed.

05-N2 — single-threaded aggregator contract (internal/purchase/execution.go)

Added comment before the aggregatePurchaseOutcomes call documenting that the aggregation is intentionally serial.

05-N3 — sweep-threshold discrepancy documented (internal/purchase/manager.go)

Expanded staleApprovedThreshold godoc to cross-reference DefaultReapAfter in reaper.go and explain the 15m vs 10m difference.

Also closes #1069

Tracked separately (not implemented here):

• fix(purchases): updatePlanProgress non-atomic read-modify-write (05-L4) #1071 — updatePlanProgress non-atomic read-modify-write (05-L4); needs FOR UPDATE or atomic DB increment
• chore(purchases): finish-or-remove GetPendingExecutionsTx (dead after #1037 chose per-row CAS) #1072 — GetPendingExecutionsTx dead code chore (superseded by fix(purchases): close double-buy holes in idempotency, atomic claim, and partial-success labelling #1037's per-row CAS design, which is intentional and sound)

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Fixed duplicate purchases on retry by implementing stable idempotency tracking across execution attempts
  • Improved multi-account purchase handling to correctly process partial successes and prevent incorrect failure classification
  • Enhanced protection against duplicate execution from asynchronous message redelivery
  • Strengthened error handling and recovery for purchase execution failures

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 7 minutes and 55 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4b2f13a0-5b9f-459d-836e-c701e0bb2ca4

📥 Commits

Reviewing files that changed from the base of the PR and between 605b447 and d74e045.

📒 Files selected for processing (14)

• internal/commitmentopts/service.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/execution/fanout.go
• internal/execution/fanout_test.go
• internal/purchase/execution.go
• internal/purchase/execution_test.go
• internal/purchase/manager.go
• internal/purchase/reaper.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_overrides.go
• internal/scheduler/scheduler_overrides_test.go
• internal/scheduler/scheduler_suppressions_test.go
• pkg/exchange/reshape_crossfamily_test.go

📝 Walkthrough

Walkthrough

This PR resolves a critical double-buy risk in purchase execution retries and multi-account fan-out by introducing stable idempotency lineage (persisted IdempotencyKey field), atomic CAS claiming on all executor paths, and correct partial-success classification. It fixes issues #1012, #1013, and #1014.

Changes

Purchase execution idempotency and atomic claiming

Layer / File(s)	Summary
IdempotencyKey schema, storage persistence, and scanning internal/config/types.go, internal/database/postgres/migrations/000066_*, internal/config/store_postgres.go, internal/config/store_postgres_pgxmock_test.go	PurchaseExecution gains an IdempotencyKey field with forward/backward migrations. SavePurchaseExecutionTx generates and persists the key on first creation; all SELECT statements include idempotency_key in projections, and scanExecutionRows deserializes nullable keys with fallback to empty string for pre-migration rows.
Retry successor idempotency lineage propagation internal/api/handler_purchases.go	persistRetryExecution now copies the failed predecessor's stable IdempotencyKey (or falls back to ExecutionID for legacy rows) into the retry successor, ensuring the same idempotency identity across the lineage.
Stable per-recommendation idempotency token derivation via lineage key internal/purchase/execution.go	Introduces idempotencyLineageKey(exec) to return persisted IdempotencyKey with fallback to ExecutionID for legacy rows. processPurchaseRecommendations now derives per-resource tokens from the stable lineage key instead of directly from ExecutionID, keeping provider tokens unchanged across retries and multi-account runs.
Multi-account execution with partial-success semantics internal/purchase/execution.go, internal/purchase/execution_test.go	Refactors executeMultiAccount to aggregate per-account commit flags into typed sentinels (multiAccountPartialError for "some committed", errAllAccountsFailed for "all failed"). executeForAccount now returns (committed bool, err error) and marks per-account rows as partially_completed when purchases succeed partially. Unit test updated to assert the committed flag.
Atomic CAS claiming and finalization internal/purchase/manager.go	Introduces claimAndExecute as the universal CAS funnel that atomically transitions executable statuses to running, returning benign duplicate on CAS-loss races. Refactors executeAndFinalize to always persist the finalized root status. Updates finalizeExecution to recognize both single-account and multi-account partial sentinels and map them to partially_completed terminal status. Recovery comments updated to reflect deterministic idempotency-token behavior.
Scheduled-purchase execution loop with CAS claiming internal/purchase/manager.go	Updates ProcessScheduledPurchases to use claimAndExecute, treats multi-account partial successes as "executed" via isMultiAccountAckable, and collects claim/execution errors into the returned ProcessResult.
SQS handler wiring with claimAndExecute and ack logic internal/purchase/messages.go	handleExecutePurchase replaces explicit status checks with atomic claimAndExecute, returning nil (ACK) on benign CAS-loss or multi-account partial success, otherwise propagating the error for SQS redelivery.
Unit test updates for CAS claiming and error handling internal/purchase/coverage_extra_test.go, internal/purchase/manager_test.go, internal/purchase/messages_test.go	Tests now expect TransitionExecutionStatus CAS mocks with allowed source statuses and running target. Error assertions updated to check for config.ErrAuditLoss on save failures or config.ErrExecutionNotInExpectedStatus on wrong-status claims.
Comprehensive regression test suite for idempotency and double-protection internal/purchase/money_path_regression_test.go	Five new tests: (1) retry with fresh ExecutionID but stable IdempotencyKey reuses the same per-rec token, (2) legacy rows without IdempotencyKey fall back to ExecutionID tokens, (3) multi-account fan-out produces deterministic per-account keys across runs, (4) SQS redelivery where second CAS fails with expected-status error is benign, (5) multi-account partial success returns nil (ACK) and persists partially_completed status.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• LeanerCloud/CUDly#638: Refactors per-recommendation idempotency token generation to use persisted lineage key instead of only ExecutionID.
• LeanerCloud/CUDly#650: Extends partial-success handling to use the partially_completed status for both single-account and multi-account partial failures.
• LeanerCloud/CUDly#728: Related changes to manager.go's CAS claiming and executeAndFinalize persistence handling with config.ErrAuditLoss error propagation.

Poem

🐰 A lineage eternal, carved in stone,
No more UUID twins on each retry throne,
The CAS guards all pathways with atomic might,
Multi-account partial blooms in just light,
Stable idempotency—our double-buy blight, FIXED! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.26% 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
Title check	✅ Passed	The PR title accurately and concisely summarizes the three main changes: fixing idempotency token stability, adding atomic claims, and correcting partial-success labeling in the purchase system.
Linked Issues check	✅ Passed	All three linked issues (#1012, #1013, #1014) are fully addressed: idempotency token is now stable and propagated via IdempotencyKey field, atomic claim via claimAndExecute is implemented for all entry points, and multi-account partial success is correctly distinguished and labeled.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with the three linked issues: database schema migration for idempotency, handler/manager/message refactoring for atomic claims, execution logic for partial success, and comprehensive regression tests validating the fixes.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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/money-path-idempotency

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/config/store_postgres.go (1)

1251-1283: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

scanExecutionRows contract is now stricter than GetPlannedExecutions projection.

Line 1256 scans executed_by_user_id, executed_at, pre_approval_skip_reason, and idempotency_key, but the GetPlannedExecutions SELECT (Line 993-Line 999) still stops at approval_token_expires_at. This can fail at runtime with failed to scan execution on planned-purchase reads.

🛠️ Suggested fix

 func (s *PostgresStore) GetPlannedExecutions(ctx context.Context, statuses []string, limit int) ([]PurchaseExecution, error) {
@@
 	query := `
 		SELECT plan_id, execution_id, status, step_number, scheduled_date,
 		       notification_sent, approval_token, recommendations,
 		       total_upfront_cost, estimated_savings, completed_at, error, expires_at,
 		       cloud_account_id, source, approved_by, cancelled_by, capacity_percent,
 		       created_by_user_id, retry_execution_id, retry_attempt_n,
-		       approval_token_expires_at
+		       approval_token_expires_at,
+		       executed_by_user_id, executed_at, pre_approval_skip_reason,
+		       idempotency_key
 		FROM purchase_executions
 		WHERE status = ANY($1)
 		ORDER BY scheduled_date ASC NULLS LAST, id ASC
 		LIMIT $2
 	`

🤖 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/config/store_postgres.go` around lines 1251 - 1283, The
scanExecutionRows scanning code now expects columns executed_by_user_id,
executed_at, pre_approval_skip_reason and idempotency_key but the
GetPlannedExecutions SELECT projection stops at approval_token_expires_at;
update the GetPlannedExecutions SELECT to include executed_by_user_id,
executed_at, pre_approval_skip_reason and idempotency_key (matching
types/nullability) so rows.Scan in scanExecutionRows can map columns correctly
(or alternatively remove those extra scans from scanExecutionRows if they are
not needed) — locate the SELECT in GetPlannedExecutions and align its projected
columns with the rows.Scan call in scanExecutionRows.

🤖 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/config/store_postgres_pgxmock_test.go`:
- Around line 449-450: The test sets up both legacy (NULL) and non-NULL
idempotency_key scan paths but never asserts the resulting Exec object's
IdempotencyKey, so add explicit assertions: after the NULL-path scan (where the
diff passes nil for idempotency_key) assert exec.IdempotencyKey == nil (or empty
as your type expects), and after the non-NULL-path scan assert
exec.IdempotencyKey is non-nil and equals the original idempotency key value
used in the test; update the assertions around the scan calls that populate exec
(look for the variable name exec and the idempotency_key setup in the test) to
validate both outcomes.

---

Outside diff comments:
In `@internal/config/store_postgres.go`:
- Around line 1251-1283: The scanExecutionRows scanning code now expects columns
executed_by_user_id, executed_at, pre_approval_skip_reason and idempotency_key
but the GetPlannedExecutions SELECT projection stops at
approval_token_expires_at; update the GetPlannedExecutions SELECT to include
executed_by_user_id, executed_at, pre_approval_skip_reason and idempotency_key
(matching types/nullability) so rows.Scan in scanExecutionRows can map columns
correctly (or alternatively remove those extra scans from scanExecutionRows if
they are not needed) — locate the SELECT in GetPlannedExecutions and align its
projected columns with the rows.Scan call in scanExecutionRows.

🪄 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: 31780e21-0af8-45ff-bf82-0ce5f0403d30

📥 Commits

Reviewing files that changed from the base of the PR and between cbdc4be and 605b447.

📒 Files selected for processing (14)

• internal/api/handler_purchases.go
• internal/config/store_postgres.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/types.go
• internal/database/postgres/migrations/000066_purchase_executions_idempotency_key.down.sql
• internal/database/postgres/migrations/000066_purchase_executions_idempotency_key.up.sql
• internal/purchase/coverage_extra_test.go
• internal/purchase/execution.go
• internal/purchase/execution_test.go
• internal/purchase/manager.go
• internal/purchase/manager_test.go
• internal/purchase/messages.go
• internal/purchase/messages_test.go
• internal/purchase/money_path_regression_test.go

[cristim]

Addressed CR review findings (pass 1) plus the two CI gates:

• GetPlannedExecutions SELECT did not project the four trailing columns the shared scanExecutionRows now scans (executed_by_user_id, executed_at, pre_approval_skip_reason, idempotency_key), so the planned-purchase list path would fail at runtime with "failed to scan execution". Fixed in 1adfa8c and added TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns, which fails on the pre-fix 22-column projection and passes once aligned.
• pgxmock tests did not assert IdempotencyKey scan outcomes. Added explicit NULL-path (empty) and non-NULL-path (verbatim key) assertions in the GetExecutionByID tests, in 1adfa8c.
• CI: scanExecutionRows was at gocyclo 11 after the idempotency_key handling; extracted applyExecutionNullableTimes to bring it back under the budget (479c4a9). gofmt struct-field alignment in pkg/exchange/reshape_crossfamily_test.go (carried over from an earlier merge) unblocked the pre-commit hook on the merge ref (bab0a3b).

@coderabbitai review

[coderabbitai]

@cristim Thanks for the thorough pass-1 wrap-up. The follow-on fixes look well-targeted:

• Column projection gap in GetPlannedExecutions is a real runtime hazard — good catch and solid regression coverage with TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns.
• applyExecutionNullableTimes extraction is a clean way to keep scanExecutionRows under the cyclomatic budget without losing any of the new nullable-handling logic.
• gofmt carry-over on the merge ref is a common stumbling block; glad the pre-commit hook caught it.

Kicking off pass 2 now.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `CancelScheduledExecutionAtomic` (`WHERE status = 'scheduled'`) is distinct from `CancelExecutionAtomic` (`WHERE status IN ('pending','notified')`). The pre-fire-delay revoke path uses `CancelScheduledExecutionAtomic`; the 410 "window closed" / 409 "not pending" race outcomes must remain disambiguated. Using `CancelExecutionAtomic` for scheduled-execution revocation is a critical bug (the execution fires and the user is charged while the UI shows "cancelled").

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

✅ Action performed

Review finished.

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]

FOLD-1037 scope expansion: per-finding summary

The following FOLD-1037 findings from 16-remaining-findings-plan.md have been implemented in 3 additional commits (81ff5e3, c6cb85b, d74e045). All regression tests confirmed to fail on pre-fix code.

05-H3 (High) -- fanout semaphore ignores ctx cancellation (internal/execution/fanout.go:70-72): replaced unconditional sem<- with select+ctx.Done(); test: TestFanOut_ContextCancelled_BlockedSemaphore

05-M1 (Medium) -- recs[:0] slice aliasing in scheduler filters (scheduler.go:1144, scheduler_overrides.go:55): allocate make([]T,0,len(recs)) instead; tests: TestApplySuppressionIndex_DoesNotMutateCallerSlice, TestFilterRecsByResolvedConfigs_DoesNotMutateCallerSlice

05-M4 (Medium) -- commitmentopts silent permissive fallback on unknown service (service.go:163): add Warn log when provider known but service absent in probe data

05-L1 (Low) -- unqueryable audit-gap marker (recordHistoryAuditGap): prefix note with 'history_write_failed:' + commitment ID for CloudWatch filter support

05-L2 (Low) -- getAWSAccountID returns 'unknown' sentinel: changed to (string, error); caller logs Warn and proceeds with empty accountID; tests updated

05-L3 (Low) -- normalizePurchaseSource: added comment documenting 'proceed untagged' decision; behavior unchanged

05-N1 (Nit) -- dead _ = now in reaper: wired now into sweep log line (RFC3339), removed both suppressors

05-N2 (Nit) -- missing single-threaded aggregator contract: added comment before aggregatePurchaseOutcomes call

05-N3 (Nit) -- two stuck-execution thresholds undocumented: expanded staleApprovedThreshold godoc to cross-reference DefaultReapAfter

Tracked separately (NOT implemented here, by design):

• fix(purchases): updatePlanProgress non-atomic read-modify-write (05-L4) #1071: updatePlanProgress non-atomic RMW (05-L4) -- needs FOR UPDATE or atomic DB increment
• chore(purchases): finish-or-remove GetPendingExecutionsTx (dead after #1037 chose per-row CAS) #1072: GetPendingExecutionsTx dead code chore -- superseded by this PR's per-row CAS design (intentional, sound)

Closes #1069

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@cristim: Solid pass-3 bundle — the per-finding breakdown is exactly the right level of detail for a money-path PR. A few observations before I kick off the review:

• 05-H3 is the most consequential fix here: the unconditional sem <- in a fan-out goroutine would block forever after a context cancellation, leaking the goroutine and holding the semaphore slot — glad to see the select + ctx.Done() path and the accompanying test.
• 05-M1 (slice aliasing) is a subtle but real correctness hazard — recs[:0] shares the backing array, so later appends silently corrupt the caller's slice. The make([]T, 0, len(recs)) allocation is the right fix.
• 05-L2 promoting getAWSAccountID to (string, error) is a meaningful improvement — the "unknown" sentinel being silently propagated into provider API calls is the kind of thing that only surfaces as a confusing cloud-side error.
• Deferring #1071 (non-atomic RMW on updatePlanProgress) and #1072 (dead code superseded by the CAS design) is the correct call — both deserve their own targeted PRs.

Triggering pass 3 now.

[review]

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `CancelScheduledExecutionAtomic` (`WHERE status = 'scheduled'`) is distinct from `CancelExecutionAtomic` (`WHERE status IN ('pending','notified')`). The pre-fire-delay revoke path uses `CancelScheduledExecutionAtomic`; the 410 "window closed" / 409 "not pending" race outcomes must remain disambiguated. Using `CancelExecutionAtomic` for scheduled-execution revocation is a critical bug (the execution fires and the user is charged while the UI shows "cancelled").

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `canRevokeCompletedRow` in `frontend/src/history.ts` must check `canAccess` (not just `getCurrentUser()` truthiness) to align with peer predicates `canCancelPendingRow`, `canApprovePendingRow`, and `canRetryFailedRow`. `revoke-own` and `revoke-any` were added to the closed `Action` union to make future UX-vs-RBAC drift a compile error.

[cristim]

Addressed CR pass-1 findings (both stale -- resolved in FOLD commits):

• inline store_postgres_pgxmock_test.go:449 (IdempotencyKey assertion gap) -- stale: assertions added in 479c4a9 and 1adfa8c; CR itself marked this resolved.
• outside-diff store_postgres.go:993-1000 (GetPlannedExecutions missing 4 trailing columns) -- stale: fixed in commit 1adfa8c (fix(config): project idempotency_key in GetPlannedExecutions SELECT). The current branch projects all four columns (executed_by_user_id, executed_at, pre_approval_skip_reason, idempotency_key) in the SELECT.

Build, vet, and go test -race ./internal/purchase/... pass clean.

@coderabbitai review

[coderabbitai]

@cristim: Confirmed — both pass-1 items are stale artifacts now cleanly resolved in the FOLD commits. The column-projection gap in GetPlannedExecutions and the IdempotencyKey scan assertion gap in the pgxmock tests are both accounted for. Triggering pass 4 now.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `CancelScheduledExecutionAtomic` (`WHERE status = 'scheduled'`) is distinct from `CancelExecutionAtomic` (`WHERE status IN ('pending','notified')`). The pre-fire-delay revoke path uses `CancelScheduledExecutionAtomic`; the 410 "window closed" / 409 "not pending" race outcomes must remain disambiguated. Using `CancelExecutionAtomic` for scheduled-execution revocation is a critical bug (the execution fires and the user is charged while the UI shows "cancelled").

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-05T12:28:20.780Z
Learning: In LeanerCloud/CUDly, `internal/mocks/stores.go` is the canonical `MockConfigStore` (testify/mock embedding, Fn-override fields, isExpected guards). Per-package files in `internal/api`, `internal/purchase`, and `internal/scheduler` were reduced to a single type alias line: `type MockConfigStore = mocks.MockConfigStore`. Four intentionally-local mocks remain: `mockConfigStore` (lowercase, hook-field only) in `internal/analytics/collector_test.go`; `mockConfigStoreForHealth` in `internal/server/test_helpers_test.go`; and `mockConfigStoreForExchange` / per-scenario variants in `internal/server/handler_ri_exchange_test.go` and `handler_coverage_test.go`. A compile-time check `var _ config.StoreInterface = (*MockConfigStore)(nil)` lives in the shared mock.

✅ Action performed

Review finished.

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.