bug(purchases): approve strands execution in 'approved' (purchase never runs, no error) on interrupted sync execution #632

Activity

1. cristim

   added

   bugSomething isn't workingSomething isn't working

   triagedItem has been triagedItem has been triaged

   priority/p1Next up; this sprintNext up; this sprint

   severity/highSignificant harmSignificant harm

   urgency/this-sprintWithin the current sprintWithin the current sprint

   impact/all-usersAffects every userAffects every user

   effort/mDaysDays

   type/bugDefectDefect

   on May 20, 2026
2. cristim

   added 5 commits that reference this issue on May 21, 2026

   fix(purchases): recover executions stranded in 'approved' (closes #632)…

   ...

   This issue will close once commit 08747bc is merged into the 'main' branch.

   08747bc

   fix(azure): switch 7 reservation clients to two-step calculatePrice->…

   ...

   This issue will close once commit 65ecbf8 is merged into the 'main' branch.

   65ecbf8

   fix(purchases): narrow Describe*Offerings + cap pagination (#688) (#690)

   ...

   This issue will close once commit b581fe7 is merged into the 'main' branch.

   b581fe7

   feat(providers/aws): add purchasecfg helper to bound purchase-path SD…

   ...

   This issue will close once commit e485a49 is merged into the 'main' branch.

   e485a49

   fix(purchases): tighten AWS SDK retry budget + bump Lambda timeout to…

   ...

   This issue will close once commit 30e8912 is merged into the 'main' branch.

   30e8912
3. cristim commented on May 25, 2026

   

   cristim

   on May 25, 2026

   MemberAuthor

   More actions

   Independent fix path for #632 (does not wait on #721 / #639)

   Re-walked Manager.ApproveAndExecute -> executeAndFinalize -> executePurchase and the existing mitigations (#635 RecoverStrandedApprovals, #678 ReapStuckExecutions). Below: the strand window, an option evaluation, and a recommendation that meaningfully reduces the operator-toil from #632 without needing #639 / #721.

   The strand window (exact lines)

   internal/purchase/approvals.go:110-129 (ApproveAndExecute):

   1. TransitionExecutionStatus(... "approved") lands (line 111). Row is now durably approved.
   2. Best-effort ApprovedBy stamp (lines 116-125).
   3. executeAndFinalize(ctx, updated) runs synchronously (line 128).

   internal/purchase/manager.go:156-176 (executeAndFinalize):

   4. executePurchase (line 157): all cloud SDK calls fire here. Provider construction, credential resolution, per-rec fan-out, PurchaseCommitment for each rec, savePurchaseHistory per rec.
   5. finalizeExecution (line 158): mutates exec.Status in memory only ("completed" / "partially_completed" / "failed").
   6. SavePurchaseExecution (line 163): persists the terminal status.

   The strand window is steps 4 to 6: the row is approved from the moment line 111 commits until step 6 writes a terminal status. Lambda timeout / panic / cold-start eviction inside steps 4-5, or a transient DB failure swallowing step 6, leaves the row in #632's exact "approved, error empty, completed_at NULL, purchased=false" shape. The two existing sweeps (#635 safe-fail and #678 reaper) eventually flip such rows to failed, but the operator still has to confirm the cloud state and Retry.

   What is already idempotency-safe today (and what isn't)

   Cataloguing the per-provider state of opts.IdempotencyToken (already plumbed through processPurchaseRecommendations at execution.go:428 via common.DeriveIdempotencyToken(exec.ExecutionID, i)):

   Provider / service	Idempotency safety on re-drive
   AWS Savings Plans	Native ClientToken (savingsplans/client.go:204)
   AWS EC2 RI	Tag-guard via findRIByIdempotencyToken (ec2/client.go:122-137)
   AWS RDS RI	idempotencyGuard short-circuit + deterministic IdempotentReservationID (rds/client.go:149,198)
   AWS ElastiCache RI	idempotencyGuard + deterministic ID + recoverAlreadyExists (elasticache/client.go:134-159)
   AWS OpenSearch RI	idempotencyGuard + deterministic ID + recoverAlreadyExists (opensearch/client.go:159-183)
   AWS MemoryDB / Redshift	Idempotency-token paths exist in their *_test.go, present in client
   GCP CUDs	Deterministic ID derived from opts.IdempotencyToken (per #641 patterns)
   Azure (all 7 services via DoPurchaseTwoStep)	Token dropped (#721 regression)

   This is the lever: #639's auto-re-drive is double-buy-safe on AWS-only executions today. The Azure regression (#721) is the only piece blocking the full #639. The strands in #632 were on AWS account 909626172446 (the AWS-only case is the one we actually observe in the field).

   Option evaluation

   Option	Effort	Helps #632?	New double-buy risk?	Depends on #721/#639
   1. Write-ahead purchase_attempted_at / attempting interim status	Medium (new column or status, migration, sweep update)	Marginally: the sweep can already distinguish strands; this doesn't tell us whether the SDK call was made, only that we intended to call it	None	No
   2. Async execution model (SQS worker)	Large (new infra, IAM, worker, deploy, retry contract, observability rewrite)	Yes, but it's a re-architecture	None new (existing idempotency still applies on the worker)	No
   3. Resilient sync path (shorter per-phase timeouts + checkpoints)	Large (deep refactor of executor with no clear stopping point)	Partially, with new complexity	None	No
   4. Cloud-side detection in the safe-fail sweep	Small to medium (per-provider commitment lookup by tag/ClientToken)	Yes, for the "actually succeeded but row never saved" case (operator no longer sees a misleading "failed" row)	None (read-only lookups, no purchase)	No
   5 (recommended). AWS-only idempotent auto-re-drive in RecoverStrandedApprovals	Small (single file, no migration, gated by provider)	Yes, for the exact AWS strands in #632; converts operator-toil into automatic recovery for the common case	None (every AWS service is already idempotency-safe per the table above)	No

   Option 5 is a strict subset of #639, sliced by provider: it does exactly what #639 proposes, but only for executions whose recs are all AWS. The Azure regression (#721) is sidestepped — Azure executions continue to fall through to today's safe-fail behaviour.

   Recommended fix: option 5 (AWS-scoped auto-re-drive)

   Design sketch

   In internal/purchase/manager.go:

   // allRecsAWS reports whether every selected rec in exec targets AWS. Used by
   // RecoverStrandedApprovals to scope auto-re-drive to providers whose per-rec
   // idempotency guards (Savings Plans ClientToken; EC2/RDS/ElastiCache/OpenSearch
   // idempotency-tag short-circuits) make a re-drive double-buy-safe today.
   // Azure is excluded until #721 restores token threading through
   // DoPurchaseTwoStep; mixed-provider executions are excluded conservatively.
   func allRecsAWS(exec *config.PurchaseExecution) bool {
       if len(exec.Recommendations) == 0 {
           return false
       }
       for i := range exec.Recommendations {
           p := exec.Recommendations[i].Provider
           if p != "" && p != "aws" {
               return false
           }
       }
       return true
   }

   In RecoverStrandedApprovals, between the stale SELECT and the CAS-to-failed, add an AWS-scoped re-drive branch. The CAS that already exists protects against the race where the original run finishes between SELECT and our re-drive: we keep the same atomic guard (WHERE status='approved') but transition to a fresh in-flight status (e.g. reuse approved with updated_at bumped, or introduce a sentinel; safest is reuse approved and rely on the executor's per-rec idempotency to handle the original run's residual writes if any).

   for i := range stranded {
       exec := &stranded[i]
   
       if allRecsAWS(exec) {
           // Atomic re-drive: bump updated_at so the same row is not picked up
           // again by the next sweep tick while executeAndFinalize is mid-flight.
           // executeAndFinalize itself is idempotent on AWS (per the table above),
           // so the worst case if our re-drive crashes again is another sweep
           // tick later — each tick is double-buy-safe.
           logging.Warnf("Re-driving stranded AWS approved execution %s (idempotency-safe; #632)", exec.ExecutionID)
           if execErr := m.executeAndFinalize(ctx, exec); execErr != nil {
               logging.Errorf("Re-drive of stranded execution %s failed: %v", exec.ExecutionID, execErr)
               // fall through: leave the row in whatever state finalizeExecution wrote
               // (completed / partially_completed / failed). No additional sweep action.
           }
           recovered++
           continue
       }
   
       // Non-AWS (or mixed) executions: fall back to today's safe-fail path
       // (Azure DoPurchaseTwoStep loses IdempotencyToken per #721, so a re-drive
       // could double-buy until #721 is fixed). Operator confirms cloud state +
       // clicks Retry as today.
       // ... existing TransitionExecutionStatus + SavePurchaseExecution to "failed"
   }

   Why the existing AWS guards make this safe:

   • If the original run never reached the cloud SDK call: re-drive purchases for the first time, normal happy path.
   • If the original run created the commitment and crashed before the row saved: re-drive's findRIByIdempotencyToken / idempotencyGuard / ClientToken finds the existing commitment, returns its ID without buying again, savePurchaseHistory writes the (now-correct) history row, the execution stamps completed.
   • If two sweep ticks race (one re-driving, the next picking it up via the SELECT before the first finished): the second sweep sees updated_at was just bumped (or runs after the first finalised) — and even if both somehow proceed, both re-drives hit the same idempotency guard and only one commitment exists. The aggregator writes one purchase_history row (the guard returns the same CommitmentID); a benign duplicate SavePurchaseHistory is the only failure mode and it is non-financial.

   What option 5 does NOT fix (vs the full #639 path):

   • Azure-only and mixed-provider strands stay on today's safe-fail-and-Retry path until bug(providers/azure): DoPurchaseTwoStep dropped IdempotencyToken threading; reintroduces double-purchase risk (regression of #641; blocks #639) #721 lands. This is acceptable: the field incident in bug(purchases): approve strands execution in 'approved' (purchase never runs, no error) on interrupted sync execution #632 is AWS, and the safe-fail behaviour for Azure is the same as today, not a regression.
   • The underlying race (sync execution inside Lambda lifetime) still exists; we are eliminating the consequence (operator toil + misleading "failed" rows that secretly succeeded), not the cause. Option 2 (async / queue) is the only path that eliminates the cause; it is rightly deferred.

   Effort estimate

   • ~40 lines in manager.go (allRecsAWS + branch in RecoverStrandedApprovals).
   • ~3-4 unit tests in manager_test.go:
     • AWS-only strand re-driven to completed; provider mock asserts the per-rec IdempotencyToken is the same on both attempts.
     • Azure strand still falls through to failed (regression-locks the bug(providers/azure): DoPurchaseTwoStep dropped IdempotencyToken threading; reintroduces double-purchase risk (regression of #641; blocks #639) #721 carve-out).
     • Mixed AWS+Azure strand falls through to failed.
     • Re-drive that crashes again leaves the row in a known state (either re-failed by the next sweep, or completed — never approved permanently).
   • No DB migration. No new store method. No infra change.
   • No dependency on bug(providers/azure): DoPurchaseTwoStep dropped IdempotencyToken threading; reintroduces double-purchase risk (regression of #641; blocks #639) #721 or feat(purchases): flip RecoverStrandedApprovals to idempotent re-drive (now that #636 makes it safe) #639.

   Recommendation

   STOP and let the user decide between option 5 (small, AWS-scoped, ships now) and waiting on #721 -> #639 (full fix, eventually). Option 5 is genuinely a strict subset of #639's eventual behaviour, so landing it does not commit us to a path different from the long-term design — when #721 lands and #639 lifts the Azure carve-out, allRecsAWS simply gets removed.

   If the chosen path is option 5, I can implement on fix/632-aws-scoped-auto-redrive (single PR, single file + tests, no migration). If the preference is to keep waiting on #721 -> #639, no code change is needed and this comment is the record of why.
4. cristim

   added a commit that references this issue on May 26, 2026

   fix(purchases): AWS-scoped auto-re-drive in RecoverStrandedApprovals …

   ...

   2089692
5. cristim commented on May 28, 2026

   

   cristim

   on May 28, 2026

   MemberAuthor

   More actions

   Verified shipped on feat/multicloud-web-frontend via PRs #635, #638, #668, #670, #674, #680, #684, #690, #728. RecoverStrandedApprovals now: (a) re-drives AWS-only stranded executions idempotently via claimAndRedrive (PR #728), (b) safe-fails mixed/Azure/GCP strands with a clear audit message (PR #635). Idempotent commitment creation (PR #638) prevents double-buy on re-drive.
6. cristim

   closed this as completedon May 28, 2026