fix(purchases): approve now executes synchronously and parallel across recs (closes #372) - #373
Merged
cristim merged 2 commits intoMay 14, 2026
Conversation
…s recs (closes #372) Pre-fix, clicking Approve only flipped purchase_executions.status to "approved" and stopped. No scheduler in the Lambda deployment picked the row up, so the AWS RI / SP / Azure reservation / GCP CUD was never purchased and the approval never reached Purchase History. The historyExecutionStatuses list hid "approved" assuming the scheduler would materialize it, completing the invisibility. This change makes ApproveExecution (token path) and a new ApproveAndExecute (session path) drive the purchase synchronously inside the same HTTP invocation: 1. Atomic transition pending|notified -> approved via TransitionExecutionStatus, closing the race between two concurrent approves on the same execution. 2. ApprovedBy stamped on the returned row for attribution. 3. executeAndFinalize runs the AWS / Azure / GCP API calls and lands the row on "completed" or "failed". processPurchaseRecommendations now fans the per-rec API calls out across CUDLY_MAX_ACCOUNT_PARALLELISM goroutines via execution.FanOutWithConcurrency, so a multi-rec execution spanning providers (AWS RI + Azure reservation + GCP CUD) or services within a provider (EC2 + RDS + ElastiCache + OpenSearch) doesn't block on the slowest call. Aggregation stays single-threaded so writes to exec.Recommendations[i], purchaseErrors, and savePurchaseHistory remain race-free. Multi-account fan-out is unchanged: outer executeMultiAccount still parallelizes per-account, and per-account work now parallelizes per-rec. Concurrent approvals on different executions remain parallel by Lambda design - each HTTP invocation drives its own executeAndFinalize. handler_history.go: the historyExecutionStatuses comment is updated to reflect that "approved" is now a transient in-request state rather than something the scheduler is expected to process. Test updates: - internal/purchase/approvals_test.go rewritten to wire the new transition + execute chain. Empty Recommendations keeps the AWS call surface absent. - internal/purchase/coverage_extra_test.go ApproveHappyPath gains the mocks for the now-chained executeAndFinalize. - internal/api/handler_purchases_test.go + handler_test.go: token-path response shifts from "approved" to "completed" (post-execute state), plus two new tests that pin the session-path uses ApproveAndExecute and that an execute failure surfaces as 409. - internal/api/mocks_test.go, internal/testutil/mocks.go: new ApproveAndExecute mock methods on the purchase manager mocks. - internal/api/types.go, internal/server/interfaces.go: interface additions for ApproveAndExecute. The earlier in-dashboard approval feature (#286) shipped the button but not the execution chain, on the assumption the email-link scheduler would close the loop; that scheduler was never wired in the Lambda EventBridge module. This change makes the dashboard click and the email link both do the right thing in-process.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR changes approvals to a single synchronous approve+execute operation (handlers return "completed"), adds ApproveAndExecute to interfaces/mocks, refactors approval logic and tests, and parallelizes recommendation execution with serial aggregation and history saves. ChangesSynchronous Purchase Approval
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/purchase/coverage_extra_test.go (1)
279-297: ⚡ Quick winUse a non-empty
PlanIDfixture to harden this flow test.This setup currently validates
GetPurchasePlanwith"", which can mask plan-id propagation regressions in the approve→execute chain.♻️ Proposed test fixture hardening
+ planID := "plan-appv" exec := &config.PurchaseExecution{ ExecutionID: "exec-appv", + PlanID: planID, Status: "pending", ApprovalToken: "correct-token", @@ approved := &config.PurchaseExecution{ ExecutionID: "exec-appv", + PlanID: planID, Status: "approved", ApprovalToken: "correct-token", Recommendations: exec.Recommendations, } @@ - plan := &config.PurchasePlan{ID: "", Name: "test-plan"} - mockStore.On("GetPurchasePlan", ctx, "").Return(plan, nil) + plan := &config.PurchasePlan{ID: planID, Name: "test-plan"} + mockStore.On("GetPurchasePlan", ctx, planID).Return(plan, nil)🤖 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/coverage_extra_test.go` around lines 279 - 297, The test currently stubs mockStore.GetPurchasePlan with an empty PlanID which can mask regressions; change the PurchasePlan fixture (the plan variable) to have a non-empty ID (e.g., "plan-123"), update the mock expectation call to mockStore.On("GetPurchasePlan", ctx, plan.ID).Return(plan, nil), and ensure the exec fixture (exec) uses that same PlanID so the approve→execute path exercises real plan-id propagation; adjust any other mocks that assumed "" accordingly.
🤖 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/purchase/execution.go`:
- Around line 237-264: The closure that converts the key with strconv.Atoi and
aggregatePurchaseOutcomes both lack defensive checks; update the closure (the
anonymous func returning recPurchaseOutcome) to handle strconv.Atoi errors by
returning an error outcome (do not ignore the error), validate that the parsed
index is within bounds of exec.Recommendations before accessing it, and set
recPurchaseOutcome.err appropriately when parsing/bounds fail; in
aggregatePurchaseOutcomes, check each execution.Result's r.Err before using
r.Value (and treat r.Err as a failure by recording the error into
exec.Recommendations[i].Error and purchaseErrors) so you never dereference an
invalid/mismatched Value, using the recPurchaseOutcome fields (index, err) to
correlate and update exec.Recommendations safely.
---
Nitpick comments:
In `@internal/purchase/coverage_extra_test.go`:
- Around line 279-297: The test currently stubs mockStore.GetPurchasePlan with
an empty PlanID which can mask regressions; change the PurchasePlan fixture (the
plan variable) to have a non-empty ID (e.g., "plan-123"), update the mock
expectation call to mockStore.On("GetPurchasePlan", ctx, plan.ID).Return(plan,
nil), and ensure the exec fixture (exec) uses that same PlanID so the
approve→execute path exercises real plan-id propagation; adjust any other mocks
that assumed "" accordingly.
🪄 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: 5dcccf62-2d94-4cc2-89c9-f617fe508cef
📒 Files selected for processing (12)
internal/api/handler_history.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/handler_test.gointernal/api/mocks_test.gointernal/api/types.gointernal/purchase/approvals.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/server/interfaces.gointernal/testutil/mocks.go
Address CR feedback on PR #373: 1. **execution.go fan-out closure + aggregator: defensive error handling.** The closure that parses `key → index` discarded `strconv.Atoi`'s error and didn't bounds-check the parsed index, so a malformed key would silently target `exec.Recommendations[0]`. The aggregator likewise dereferenced `r.Value.index` without first checking `r.Err`, which would correlate a closure-level failure to whatever rec sits at the zero-valued index. Closure now returns an error on: - `strconv.Atoi` parse failure → `"invalid fan-out key %q: %w"` - `i` outside `[0, len(exec.Recommendations))` → bounds error Aggregator now: - Checks `r.Err` first and records it as an `"internal fan-out error"` in `purchaseErrors` (cannot correlate to a specific rec because the zero-valued outcome has no usable index). - Defence-in-depth bounds-check on `v.index` even when `r.Err == nil`, so a future refactor that shrinks `exec.Recommendations` between fan-out and aggregation can't corrupt the per-rec writes. Neither path is reachable in normal flow (`indexKeys()` always produces valid keys, and `exec.Recommendations` isn't mutated between the fan-out call and the aggregator). The checks pin invariants for future refactors. 2. **coverage_extra_test.go: non-empty PlanID fixture.** `TestProcessMessage_ApproveHappyPath` previously used `PlanID: ""` and stubbed `GetPurchasePlan(ctx, "")`. That would silently pass a regression where the approve→execute chain dropped or substituted the plan id. Switched both `exec` and `approved` fixtures to `PlanID: "plan-appv"` and updated the mock expectation to require the exact id. Verified: `go test ./internal/purchase/...` — 104 tests pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 14, 2026
Closed
cristim
added a commit
that referenced
this pull request
May 14, 2026
#395) (#412) PR #373 made approve synchronous, so executePurchase now runs inline from ApproveAndExecute. For direct-execute purchases created via the Opportunities "Purchase" button there is no associated plan and exec.PlanID is "". GetPurchasePlan(ctx, "") hits the UUID-typed purchase_plans.id column and crashes with SQLSTATE 22P02, leaving the execution stuck in approved status without ever running the purchase. executePurchase now short-circuits the plan/accounts fetch when PlanID is empty, synthesises a placeholder PurchasePlan{Name: "Direct purchase"} for the downstream history-record write + confirmation email, and falls through to the single-account path. updatePlanProgress gains a guard at the top so the post-execution progress update doesn't hit the same error. SavePurchaseExecution (store_postgres.go:719) and SavePurchaseHistory (store_postgres.go:1042) already handle empty PlanID by passing NULL to the nullable column, so the persistence layer is untouched. Adds TestManager_ApproveAndExecute_EmptyPlanID as a regression guard that asserts the empty-PlanID approval path never calls GetPurchasePlan / GetPlanAccounts / UpdatePurchasePlan and lands the execution at status "completed".
This was referenced May 14, 2026
cristim
added a commit
that referenced
this pull request
May 19, 2026
#454) * fix(purchases): reconstruct typed Details pointer for every service (closes #453) After #373 made approve synchronous, every AWS purchase from the dashboard failed with `invalid service details for <Service>`. executeSinglePurchase assigned a value-typed common.DatabaseDetails (and only when rec.Engine was non-empty); every AWS findOfferingID type-asserts a *pointer*, so the assertion failed and the error propagated up as "some purchases failed: [<resource>: purchase failed: failed to find offering: invalid service details for <Service>]" for every service. This change preserves the full ServiceDetails payload across the dashboard → DB → executePurchase boundary so per-rec platform, tenancy, AZ-config, plan-type etc. round-trip correctly: - pkg/common/service_details_codec.go: new Marshal/Decode helpers backed by a per-service-slug table. Empty payload on a known slug yields a zero- valued typed pointer (legacy-row graceful degradation); unknown slug returns nil; malformed JSON surfaces as an error. - internal/config/types.go: RecommendationRecord gains a json.RawMessage Details column persisted alongside the existing engine column. Stored as raw JSON because internal/config must not import pkg/common. - internal/scheduler/scheduler.go (convertRecommendations): marshals the per-rec ServiceDetails on collection. Marshal errors are logged and the row is persisted without Details so the graceful-degradation path applies. - internal/purchase/execution.go (executeSinglePurchase): replaces the value-typed DatabaseDetails block with DecodeServiceDetailsFor + applyEngineFallback. The fallback backfills Engine from the persisted column onto DB/Cache Details so legacy rows that lacked the full payload don't silently mis-purchase as the default engine. Per-service map of what Details type is now handed to the cloud client: - EC2 → *common.ComputeDetails (Platform/Tenancy/Scope) - RDS → *common.DatabaseDetails (Engine/AZConfig/InstanceClass) - ElastiCache → *common.CacheDetails (Engine/NodeType) - OpenSearch → *common.SearchDetails (forward-compat; client doesn't assert) - Redshift → *common.DataWarehouseDetails (forward-compat; client doesn't assert) - MemoryDB → nil (no *Details type; client doesn't assert) - Savings Plans (all variants) → *common.SavingsPlanDetails (PlanType/HourlyCommitment) Azure / GCP service clients don't type-assert rec.Details (they read rec.ResourceType / rec.Term / rec.Count directly), so they're unaffected by either the bug or the fix. Independent of #412 (empty plan_id, merged); the two fixes compose to unblock the full dashboard approve flow after #373. Tests: - internal/purchase/coverage_extra_test.go: TestManager_ExecuteSinglePurchase_LegacyEmptyDetails table-drives EC2/RDS/ElastiCache through executePurchase with an empty Details payload, captures the rec.Details handed to the mock service client, and asserts the concrete pointer type plus Engine backfill where applicable. TestApplyEngineFallback pins the helper's per-type behaviour. - pkg/common/service_details_codec_test.go: round-trip + legacy-empty + unknown-service + malformed-JSON + legacy-dash-form SP coverage. Test counts: go test ./... → 4454 pass across 37 packages. go vet ./... clean. * fix(purchases): apply CR nit — bytes.Equal for JSON null check Replace `string(raw) == "null"` with `bytes.Equal(raw, jsonNullBytes)` in the ServiceDetails codec to avoid the string conversion allocation on the read-back path (every purchase routes through DecodeServiceDetailsFor). Same treatment for MarshalServiceDetails' nil-pointer-folding step. CodeRabbit nit on PR #454. No behaviour change.
This was referenced May 19, 2026
This was referenced May 20, 2026
4 tasks
This was referenced May 22, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #372 (P0). Pre-fix, clicking Approve only flipped
purchase_executions.statustoapprovedand stopped. No scheduler in the Lambda deployment picked the row up, so the AWS RI / SP / Azure reservation / GCP CUD was never purchased and the approval never reached Purchase History. The History view'shistoryExecutionStatuseslist excludedapprovedon the assumption that the scheduler would materialize the row intopurchase_history, which completed the invisibility.This PR makes Approve run the purchase synchronously inside the same HTTP invocation:
pending|notified -> approvedvia the existingTransitionExecutionStatus. Two concurrent approves on the same execution see exactly one win; the loser gets a clean 409.completedorfailedbefore the HTTP response returns.Parallel-purchase guarantees (per follow-up reqs)
executeAndFinalize.executeMultiAccountstill spawns one goroutine per account up toCUDLY_MAX_ACCOUNT_PARALLELISM.processPurchaseRecommendationsfans out viaexecution.FanOutWithConcurrency, so a single approval that covers AWS RI + Azure reservation + GCP CUD (or EC2 + RDS + ElastiCache + OpenSearch within AWS) runs the cloud API calls concurrently. Aggregation stays single-threaded so writes toexec.Recommendations[i], error accumulation, andsavePurchaseHistoryare race-free regardless of how many recs are in flight.File-level changes
internal/purchase/approvals.go- newApproveAndExecute(atomic flip + execute), token-pathApproveExecutionnow delegates to it.internal/purchase/execution.go-processPurchaseRecommendationsparallelizes across recs; aggregation extracted toaggregatePurchaseOutcomes; smallnormalizePurchaseSource/selectedIndices/indexKeyshelpers added.internal/api/handler_purchases.go- session-authed branch now callsApproveAndExecuteinstead of doing an inline non-atomic flip; response returnscompleted(the post-execute state) instead ofapproved.internal/api/handler_history.go- obsolete "scheduler will materialize approved" comment updated.internal/api/types.go,internal/api/mocks_test.go,internal/server/interfaces.go,internal/testutil/mocks.go.Test plan
go test ./...passes (4440 / 4440)internal/purchase/approvals_test.gorewritten to cover the new transition + execute chain (happy path, ApprovedBy stamp, notified->approved, invalid token, empty token, not found, get error, transition fails, ApproveAndExecute bypasses token).internal/purchase/coverage_extra_test.go-TestProcessMessage_ApproveHappyPathextended with the now-chained mocks.internal/api/handler_purchases_test.go- existing token-path test asserts response is nowcompleted; two new tests pin (a) session-auth usesApproveAndExecuteand (b) execute failure surfaces as 409.completed, confirm it shows in Purchase History, confirm the RI shows up in AWS billing.Stranded rows
Any existing
purchase_executionsrow already stuck atstatus='approved'won't be auto-rescued by this PR. Operator action:UPDATE purchase_executions SET status='pending' WHERE status='approved'and re-click Approve. Filing a separate small chore to add a one-shot reaper for stranded rows.Out of scope (deferred to separate issues)
approvedreaper for legacy rows.ProcessScheduledPurchasescron still runs againstpending|notifiedonly - intentional; the synchronous path handles the common case and the scheduler stays as a backstop.Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests