[IAmJSD]

Root cause

The stuck-purchase reaper (internal/purchase/reaper.go) listed approved and running rows in one SELECT, both at reapAfter (PURCHASE_APPROVED_REAP_AFTER, default 10m). RecoverStrandedApprovals (run from ProcessScheduledPurchases) only sees approved rows older than staleApprovedThreshold (15m), and it only runs once per process sweep (process_scheduled_purchases_schedule, default rate(1 hour)). So when both sweeps were scheduled, the reaper failed a stranded approval at about 10m, and the idempotent re-drive (#639) never got to see it. The user got a failure instead of the purchase they approved.

Fix

• ReapStuckExecutions now runs two passes:
  • running rows at reapAfter, as before. Recovery never re-drives running rows, so the reaper is still their only backstop.
  • approved rows at approvedReapAfter(reapAfter) = max(reapAfter, minApprovedReapAfter).
• minApprovedReapAfter = staleApprovedThreshold + processSweepInterval (1h) + processSweepSlack (5m), which is 1h20m. It is derived from staleApprovedThreshold, so moving the recovery threshold moves the reaper floor with it. An operator-configured reapAfter above the floor is used unchanged. The floor can only delay the approved pass, never speed it up.
• For an approved row, the reap annotation ("reaped after Nm in approved state") now reports the threshold of the pass that listed it.
• Comments are updated in manager.go and handler.go. The terraform variable descriptions are updated too: the stale "Note (fix(purchase): reaper fails stranded approvals before process recovery can re-drive them #894)" caveat is removed from both enable_process_scheduled_purchases_schedule variables, and purchase_approved_reap_after now explains the approved floor. Only description strings changed in terraform; no defaults or resources changed.

The per-row CAS is unchanged (it still accepts approved|running -> failed), so reapOne and its callers keep the same signature and race handling.

Closes #894

Verification

All of this is unit/mock evidence: MockConfigStore with recorded ListStuckExecutions arguments. No real database, Lambda or EventBridge run was involved, and no real cloud calls were made. Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because the local ~/.aws/config default profile is invalid on this machine.

New regression tests are in internal/purchase/reaper_ordering_test.go. Fail-before, with reaper.go reverted to origin/main and the two tests that reference the new helper set aside:

$ go test ./internal/purchase/ -run 'TestReapStuckExecutions_(ApprovedRows|ShortReap|LongReap|PassesCover)' -count=1
--- FAIL: TestReapStuckExecutions_ApprovedRowsWaitForStrandedRecovery
        Error: "10m0s" is not greater than "1h15m0s"
        Messages: approved rows must outlive the stranded-approval recovery window
--- FAIL: TestReapStuckExecutions_ShortReapAfterDoesNotBeatRecovery
        Error: "2m0s" is not greater than "1h15m0s"
FAIL

LongReapAfterIsHonoredForApproved and PassesCoverStuckStatuses are invariants. They pass both before and after the fix, and they guard against over-correcting. ApprovedAnnotationUsesApprovedThreshold and ApprovedListStoreError use the new helper, so they don't compile against the old code.

Pass-after (this branch):

go build ./...                                                       # ok
go vet ./internal/purchase/ ./internal/server/                       # ok
go test ./internal/purchase/ ./internal/server/ -count=1             # ok, ok
go test -race ./internal/purchase/ -run Reap -count=1                # ok
go test ./internal/mocks/ -count=1                                   # ok (repo-wide test guards)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/purchase/... ./internal/server/...   # 0 issues
gocyclo -over 10 internal/purchase/reaper.go internal/purchase/manager.go internal/server/handler.go  # silent
terraform fmt -check terraform/modules/compute/aws/lambda terraform/environments/aws                 # ok
git diff --check                                                     # ok
scripts/test-git-secrets-allowlist.sh                                # 42 PASS, 0 FAIL
git secrets --scan (fresh git init copy of tracked files, isolated HOME)                            # clean

Mutation probes, each reverted afterwards:

• approvedReapAfter returns reapAfter: ApprovedRowsWaitForStrandedRecovery and ShortReapAfterDoesNotBeatRecovery fail.
• The approved pass uses reapAfter: the same two fail, plus ApprovedAnnotationUsesApprovedThreshold.

The existing reaper_test.go cases now stub both passes through expectStuckLists. TerminalStatusNotTouched checks every pass and the union of statuses. A new test checks that an error on the approved pass also propagates.

Not covered locally: terraform validate and tflint on the description-only terraform edits (no init was run). There is also no end-to-end run with real EventBridge schedules showing a stranded approval being re-driven by the process sweep instead of reaped.

Out of scope / notes

• processSweepInterval is a Go constant that matches the terraform default rate(1 hour). An operator who sets process_scheduled_purchases_schedule slower than rate(1 hour) can still have approved rows reaped before recovery runs. A full fix would pass the process cadence (or an explicit approved threshold) through to the Lambda environment. I left that out to keep this change small.
• On deployments that don't schedule the process sweep at all, stranded approved rows are now failed after about 1h20m instead of 10m. That is the trade-off the issue asks for, so the recovery path gets a chance to run where it is scheduled.
• Making the process sweep default-on (it is opt-in since fix(lambda): schedule process, notification and finalize sweeps #949 / fix(iac): several scheduled tasks are not scheduled; delayed purchases never fire on GCP/Azure #712) is not changed here.
• The reaper's CAS still accepts running as a source status for a row it listed as approved. If recovery claims the row in the gap between the reaper's SELECT and its CAS, the reaper can still fail the re-driven row. This race existed before, and the executor's existing reaped-row recovery path (saveOrRecoverTerminal) handles it. Narrowing the CAS per pass would change the reapOne contract that other tests use, so I left it.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/high, urgency/this-quarter, impact/few, effort/s, type/bug, triaged.

🤖 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: e821b257-e07f-4902-aba9-ca8b5756f405

📥 Commits

Reviewing files that changed from the base of the PR and between b56fe1f and bf2a604.

📒 Files selected for processing: 7

• internal/purchase/manager.go
• internal/purchase/reaper.go
• internal/purchase/reaper_ordering_test.go
• internal/purchase/reaper_test.go
• internal/server/handler.go
• terraform/environments/aws/variables.tf
• terraform/modules/compute/aws/lambda/variables.tf

+----------------------------------------------------------+
| My fitness tracker wants me to tackle 10,000 bugs a day. |
+----------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      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 bf2a604: draft: blocks ready

No blocking code finding. One non-blocking race worth a follow-up, plus gate items still open (CI pending, PR is a draft).

Findings

1. NON-BLOCKING, internal/purchase/reaper.go:225 (reapOne CAS): TransitionExecutionStatus(..., stuckStatuses, failed, nil) still accepts approved|running for rows from either pass. Scenario: the approved pass lists a row older than 1h20m, then a late process sweep claims it (claimAndRedrive, manager.go:569, approved -> running with fresh updated_at) and starts re-driving; the reaper's CAS then matches running and fails a row that is mid-purchase. Provider idempotency tokens (#639) should stop a double buy, but the row would read failed while the commitment may exist. This CAS widening predates the PR, but the PR makes the overlap more likely, since a delayed sweep is now what reaches rows near the 1h20m boundary. Fix: pass pass.statuses to the CAS so an approved-pass row only flips from approved. I did not reproduce the final row state; I did not trace what saveOrRecoverTerminal does when the row is already failed.
2. NON-BLOCKING, reaper.go:103: processSweepInterval is a Go constant tied to the terraform default rate(1 hour). A slower operator schedule can still lose approved rows to the reaper. The PR body already states this; it is a follow-up (pass the cadence or an explicit approved threshold via env).

What I attacked

• Double execution: the reaper only ever writes failed; it never calls the executor. Delaying it cannot cause a second submission. Recovery (manager.go:706) and the reaper use the same column (updated_at, store_postgres.go:1633 and :1669), so the 15m/1h20m ordering compares like with like. Worst-case first recoverable age is 15m + 1h = 1h15m; the floor is 1h20m, leaving the 5m slack.
• Lost purchases: approved rows are still reaped, just later (never earlier); the floor is max(reapAfter, 1h20m), so a larger operator value is unchanged.
• Running rows: the running pass uses reapAfter unchanged (reaper.go reapPasses). Mutation probe: running pass switched to the approved threshold -> ApprovedRowsWaitForStrandedRecovery, ShortReapAfterDoesNotBeatRecovery, ApprovedAnnotationUsesApprovedThreshold fail (reverted).
• Mutation probe: approvedReapAfter returns reapAfter -> ApprovedRowsWaitForStrandedRecovery and ShortReapAfterDoesNotBeatRecovery fail (reverted). Worktree clean afterwards.
• Terraform: diff of the 2 files is description strings only (variables.tf x2); no defaults, types or resources changed.
• Error handling: a list error in either pass returns the wrapped error; rows reaped in the first pass stay reaped and counted in no returned result (result is nil on error), same as before for a single list failure. Acceptable.
• Scope: 82 additions in reaper.go, +123 test file, comments in manager.go/handler.go; files under 500 lines. Small enough for the 400-line additions cap.
• Tests at this SHA: go test ./internal/purchase/ ./internal/server/ -count=1 -> ok, ok (with isolated AWS config env). I did not re-run fail-before against the parent code; the PR body's fail-before output was not independently reproduced beyond the two mutation probes above.
• Labels: PR carries priority/p2, triaged, severity/high, urgency/this-quarter, impact/few, effort/s, type/bug; identical to #894.

Gate status (origin/main Review gate)

• Independent adversarial review naming the SHA: this comment.
• Findings resolved: none blocking; finding 1 is a follow-up or a one-line fix at the author's choice.
• CI on the SHA: not green yet. At review time Lint Code passed; Unit Tests, Integration Tests, E2E, pre-commit, Validate Terraform and the rest were pending.
• CodeRabbit: skipped (draft); it is optional given this review plus local verification.
• Local verification: unit/mock evidence only (MockConfigStore with recorded ListStuckExecutions arguments). No real database or EventBridge run showing a stranded approval re-driven instead of reaped. Labeled as such by the author; I agree it is a remaining real-account gap.
• Verdict, SHA and evidence recorded on the PR: yes, here.

Blocks ready

• CI green on bf2a604.
• PR taken out of draft by the author.
• Decide finding 1 (narrow the CAS now, or file the follow-up); any new commit restarts the gate.

[cristim]

Merge-driver triage of the independent review at bf2a604: no blocking findings. Finding 1 (reaper CAS accepts approved|running regardless of the listing pass) predates this PR and is tracked as follow-up #1043. Finding 2 (hardcoded 1h sweep interval) is already noted in the PR. Once CI is green on this SHA I will mark the PR ready (owner-authorized) and merge under the gate; any new commit restarts it.

[cristim]

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

• Independent review (fresh cold-brief agent, non-author) of bf2a604: no blocking findings (fix(purchase): let stranded-approval recovery run before the reaper fails approved rows #1042 (review)). Mutation probes on the approved/running thresholds were caught (2 and 3 failing tests). Finding 1 (reaper CAS status set) predates this PR and is follow-up fix(purchase): reaper CAS should only reap rows still in the status they were listed under #1043; finding 2 (hardcoded sweep interval) is documented in the PR.
• CI on bf2a604: latest CI - Build & Test and pre-commit runs completed/success; CI Success and Run pre-commit hooks pass. No main change since CI ran overlaps this PR's directories (checked by the merge driver).
• Local verification: unit/mock evidence with recorded ListStuckExecutions arguments; fail-before shown in the PR body, pass-after re-run by the reviewer (go test ./internal/purchase/ ./internal/server/). No real database, Lambda or EventBridge run.
• Terraform changes are description strings only (verified by the reviewer).
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from fix(purchase): reaper fails stranded approvals before process recovery can re-drive them #894.

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