Skip to content

fix(purchases): dispatch AWS-scoped stranded-approval recovery in reap task (closes #632) - #958

Closed
cristim wants to merge 1 commit into
feat/multicloud-web-frontendfrom
fix/632-wire-recovery-sweep
Closed

fix(purchases): dispatch AWS-scoped stranded-approval recovery in reap task (closes #632)#958
cristim wants to merge 1 commit into
feat/multicloud-web-frontendfrom
fix/632-wire-recovery-sweep

Conversation

@cristim

@cristim cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member

Summary

Closes #632 (the remaining gap after PR #728, which was explicitly partial).

PR #728 added RecoverStrandedApprovals (issue #632 Option 5: idempotent AWS re-drive of executions stranded in approved by an interrupted synchronous run) but never wired it into the runtime. The only scheduled stuck-purchase sweep dispatched from dispatchTask was ReapStuckExecutions (#678), which unconditionally fails stranded rows for visibility. So the re-drive-to-completion path was dead code: in production a stranded AWS approval was always failed at the reap threshold rather than idempotently re-driven to completion.

Change

  • Wire RecoverStrandedApprovals into the reap_stuck_purchases scheduled task (handleReapStuckPurchases), running it before the reaper so AWS-only strands complete instead of being failed, while the reaper remains the durable safety net for anything still stuck (mixed/Azure/GCP/legacy, or AWS rows that couldn't be re-driven).
  • A recovery-sweep error is logged but not propagated, so it can never block the reaper.
  • Add RecoverStrandedApprovals to PurchaseManagerInterface and the test mock.

Safety

  • RecoverStrandedApprovals is age-guarded (15m staleApprovedThreshold > the 10m default reap threshold), CAS-claims approved -> running before re-driving, and uses per-rec idempotency tokens, so it cannot double-execute an in-flight approval or a row another sweep already claimed. All of this was already built and unit-tested in fix(purchases): AWS-scoped auto-re-drive in RecoverStrandedApprovals (partial #632) #728; this PR only activates it.

Tests

  • New TestHandleReapStuckPurchasesRunsRecoveryFirst: asserts (a) recovery runs before the reaper, and (b) a recovery error does not stop the reaper.
  • go build ./internal/..., go vet, and go test ./internal/server/ ./internal/purchase/ ./internal/testutil/ all green.

Note

The immediate UX symptom in the QA sheet (re-clicking Approve on a still-approved row returns 409 cannot be approved (status=approved)) is mitigated by this change: stranded rows now reach a terminal, retry-able state via the scheduled sweep instead of requiring manual DB intervention. A separate follow-up could make the approve handler treat a stale approved row as re-drivable inline, but that is out of scope here.

…p task (closes #632)

PR #728 added RecoverStrandedApprovals (issue #632 Option 5: idempotent
AWS re-drive of executions stranded in "approved" by an interrupted sync
run) but never wired it into the runtime. The only scheduled stuck-purchase
sweep was ReapStuckExecutions (#678), which unconditionally fails stranded
rows for visibility, so the re-drive-to-completion path was dead code: in
production a stranded AWS approval was always failed at the reap threshold
rather than idempotently re-driven.

Wire RecoverStrandedApprovals into the reap_stuck_purchases scheduled task,
running it before the reaper so AWS-only strands (15m staleApprovedThreshold,
CAS-claimed, idempotency-token guarded) complete rather than being failed,
while the reaper remains the durable safety net for anything still stuck.
A recovery-sweep error is logged but not propagated so it can never block
the reaper.

Add RecoverStrandedApprovals to PurchaseManagerInterface and the test mock,
plus a regression test asserting the recovery-then-reaper ordering and that
a recovery error does not stop the reaper.
@cristim cristim added bug Something isn't working triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/all-users Affects every user effort/m Days type/bug Defect labels Jun 4, 2026
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5b1f5ccc-5b50-4bf9-8de8-07b2e32d68f9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/632-wire-recovery-sweep

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

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 23 minutes and 10 seconds before sending another message.

@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

Closing: the premise of this PR is incorrect. RecoverStrandedApprovals is NOT dead code on feat/multicloud-web-frontend — it is already wired into ProcessScheduledPurchases (manager.go:388), which runs as the process_scheduled_purchases scheduled task with error-isolation. Adding a second invocation inside reap_stuck_purchases is redundant and could run the sweep concurrently from two tasks. Issue #632's durable re-drive + reaper safety net is already delivered and live; no code change needed here.

@cristim cristim closed this Jun 4, 2026
@cristim
cristim deleted the fix/632-wire-recovery-sweep branch June 4, 2026 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort/m Days impact/all-users Affects every user priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant