fix(purchases): AWS-scoped auto-re-drive in RecoverStrandedApprovals (partial #632) - #728
Conversation
…(partial #632) Refs #632 (full Azure inclusion blocked by #721 -> #639). AWS-only strands now re-drive idempotently; mixed/Azure strands fall through to safe-fail-and-Retry. - Add allRecsAWS predicate: true when every rec has Provider=="aws" or Provider=="" (empty = legacy pre-multi-cloud rows). Returns false for an empty Recommendations slice so zero-rec executions safe-fail. - Add AWS-only + non-empty ExecutionID branch in RecoverStrandedApprovals: calls executeAndFinalize directly, reusing the existing idempotency tokens derived via DeriveIdempotencyToken(exec.ExecutionID, i) at execution.go:428. AWS dedupes on the same token, so a second call is safe even if the first created a commitment. - Mixed/Azure/GCP or legacy rows without a stable ExecutionID continue to use the atomic TransitionExecutionStatus->failed path. - Update existing FailsStrandedRow test to use an Azure rec so it stays on the safe-fail path (test intent preserved, provider changed). - Add 4 new tests: AWSOnlyRedrives, MixedAWSAzureSafeFails, PureAzureSafeFails, LegacyNoExecutionIDSafeFails.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughRefactors recovery of purchase executions stuck in ChangesStranded Approval Recovery with AWS Redrive Path
Sequence Diagram(s)sequenceDiagram
participant Manager
participant Store
participant Executor
Manager->>Store: TransitionExecutionStatus(id, approved -> running) (CAS)
alt CAS success
Manager->>Executor: executeAndFinalize(execution)
Executor-->>Manager: execution result
Manager->>Store: SavePurchaseExecution (may wrap ErrAuditLoss)
else CAS failure
Manager->>Store: GetExecutionByID(id) probe to detect race
alt Already transitioned or missing
Note right of Manager: treat as benign, no recovered increment
else Store error
Note right of Manager: abort sweep on store error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/manager.go`:
- Around line 274-289: This AWS re-drive branch currently lacks an atomic
ownership step so two workers can both call executeAndFinalize for the same
approved row; add a CAS-style claim before invoking executeAndFinalize in the
allRecsAWS(exec) && exec.ExecutionID != "" branch: call a new or existing atomic
claim method (e.g., m.claimExecution(ctx, exec.ID or exec.ExecutionID) returning
(bool, error)) right after the logging/info line, proceed to executeAndFinalize
only if the claim returns true, and if the claim returns false or an error,
log/continue without calling executeAndFinalize; ensure the claim persists
ownership such that subsequent SavePurchaseExecution/updatePlanProgress from
executeAndFinalize are only done by the claimer.
- Around line 211-228: When handling the result of
m.config.TransitionExecutionStatus in RecoverStrandedApprovals, treat a CAS "row
vanished" as a benign race-loss by checking errors.Is(txErr, config.ErrNotFound)
(or equivalent exported sentinel) before calling GetExecutionByID; if that check
is true return (false, nil). In other words, add an early branch after obtaining
txErr that returns (false, nil) when errors.Is(txErr, config.ErrNotFound),
otherwise keep the existing read-back probe using m.config.GetExecutionByID and
the current logging/propagation behavior.
🪄 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: 91b7aeb8-4aec-4ab7-9122-f37371da599e
📒 Files selected for processing (2)
internal/purchase/manager.gointernal/purchase/manager_test.go
…d as benign in safe-fail (CR #728) Finding 1: RecoverStrandedApprovals previously called executeAndFinalize on AWS-only stranded rows without claiming ownership first. Two concurrent sweeps (or a late original completion racing the re-drive) could both call executeAndFinalize on the same approved row, risking a double-purchase. Fix: extract claimAndRedrive helper that calls TransitionExecutionStatus ("approved" -> "running") as an atomic CAS claim before the re-drive. Only the winner of the CAS proceeds; ErrNotFound and ErrExecutionNotInExpectedStatus are both treated as benign race-losses (log and skip). Real DB errors propagate. Extracting the helper also brings RecoverStrandedApprovals back within gocyclo:10. Finding 2: safeFail's TransitionExecutionStatus error path called GetExecutionByID even when the error was config.ErrNotFound, which indicates the row has already been deleted, making the follow-up probe redundant. Fix: add an early branch that returns (false, nil) immediately on ErrNotFound, skipping the unnecessary round-trip. Both fixes reuse the established TransitionExecutionStatus CAS primitive. Tests: SafeFail_ErrNotFoundIsBenign asserts no GetExecutionByID call on ErrNotFound; AWSClaimLost_NoRedrive asserts executeAndFinalize is not reached when the CAS claim is lost. Existing AWSOnlyRedrives test updated to expect the new CAS claim call (approved -> running).
CR #728 findings addressedBoth findings from the 2026-05-26T04:52:40Z review have been resolved in commit d0addd8. Finding 1 (Major): CAS claim before AWS re-drive
The claim logic was extracted into a New test: Finding 2 (Minor):
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/manager.go`:
- Around line 204-207: The helper currently swallows all errors from
executeAndFinalize, but executeAndFinalize can return persistence/audit failures
(e.g., when SavePurchaseExecution fails after CAS to running) which must not be
treated as benign; update executeAndFinalize (or its callers) to return a
distinguishable error type or sentinel for persistence/audit failures and change
the caller in manager.go to propagate or handle that error (do not return
(false, nil) for persistence failures), ensuring the execution row is either
retried/requeued or the persistence error is returned up the stack; add a
regression test that simulates SavePurchaseExecution failing after the CAS to
running and asserts the manager does not drop the row as still running but
instead surfaces/handles the persistence error.
🪄 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: bf07239c-6275-4983-b90f-a5fa83675134
📒 Files selected for processing (2)
internal/purchase/manager.gointernal/purchase/manager_test.go
… (CR #728 round 2) When SavePurchaseExecution failed after the CAS-to-running CAS succeeded, claimAndRedrive returned (false, nil), silently dropping the stranded-in- running row. Add config.ErrAuditLoss sentinel and wrap the save error with it in executeAndFinalize; claimAndRedrive now propagates that sentinel so the sweep surfaces the error rather than losing the row. Benign provider/rec errors still return (false, nil) as before -- those paths already result in a persisted terminal status via finalizeExecution.
CR #728 round-2 fix: propagate persistence failures from
|
|
Triggering the full review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/purchase/manager.go (1)
163-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap audit loss on every terminal-state persistence failure.
The
execErr == nilguard is too narrow. IfexecutePurchasereturns a provider error orpartialPurchaseErrorandSavePurchaseExecutionthen fails, the terminal state was never persisted, so the row is still stranded inrunning.claimAndRedriveonly propagates errors matchingconfig.ErrAuditLoss, so those paths are still silently dropped.Please mark any
SavePurchaseExecutionfailure here asErrAuditLossand preserve the original execution error as context, then add a regression test for the non-nilexecErrcase.Possible fix
if !wasMultiAccount { if err := m.config.SavePurchaseExecution(ctx, exec); err != nil { logging.Errorf("AUDIT LOSS: failed to save execution status: %v", err) - if execErr == nil { - // Wrap with ErrAuditLoss so callers (e.g. claimAndRedrive) can - // distinguish a persistence failure -- where the row is left - // stranded in "running" -- from a benign provider/rec error - // where finalizeExecution already committed a terminal status. - execErr = fmt.Errorf("%w: %w", config.ErrAuditLoss, err) - } + if execErr != nil { + execErr = fmt.Errorf("%w: terminal execution error %v; save failed: %v", config.ErrAuditLoss, execErr, err) + } else { + execErr = fmt.Errorf("%w: %w", config.ErrAuditLoss, err) + } } }🤖 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/manager.go` around lines 163 - 171, The SavePurchaseExecution failure handling should always be wrapped as an audit-loss error regardless of execErr: when m.config.SavePurchaseExecution(ctx, exec) returns an error, replace the conditional guard and set execErr = fmt.Errorf("%w: %w", config.ErrAuditLoss, originalErr) while preserving any existing execErr as context (e.g., include execErr in the wrapped error or chain it so the original provider/partialPurchaseError isn't lost); update the code in the SavePurchaseExecution/error block (referencing m.config.SavePurchaseExecution, execErr, and config.ErrAuditLoss) and add a regression test that simulates executePurchase returning a non-nil provider/partialPurchaseError and then SavePurchaseExecution failing to assert the final propagated error matches or wraps config.ErrAuditLoss and contains the original execErr context.
🤖 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/manager_test.go`:
- Around line 739-756: The mocks for side-effectful operations are not
constrained to run exactly once, weakening the test; update the expectations to
include .Once() on the side-effectful calls: add .Once() to
mockStore.TransitionExecutionStatus(...), mockStore.SavePurchaseHistory(...),
mockEmail.SendPurchaseConfirmation(...), mockStore.SavePurchaseExecution(...),
and mockServiceClient.PurchaseCommitment(...) so each of those expectations must
be invoked exactly once (leave non-side-effect mocks like GetPurchasePlan,
GetCallerIdentity, CreateAndValidateProvider, GetServiceClient as appropriate).
---
Duplicate comments:
In `@internal/purchase/manager.go`:
- Around line 163-171: The SavePurchaseExecution failure handling should always
be wrapped as an audit-loss error regardless of execErr: when
m.config.SavePurchaseExecution(ctx, exec) returns an error, replace the
conditional guard and set execErr = fmt.Errorf("%w: %w", config.ErrAuditLoss,
originalErr) while preserving any existing execErr as context (e.g., include
execErr in the wrapped error or chain it so the original
provider/partialPurchaseError isn't lost); update the code in the
SavePurchaseExecution/error block (referencing m.config.SavePurchaseExecution,
execErr, and config.ErrAuditLoss) and add a regression test that simulates
executePurchase returning a non-nil provider/partialPurchaseError and then
SavePurchaseExecution failing to assert the final propagated error matches or
wraps config.ErrAuditLoss and contains the original execErr context.
🪄 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: 8021bbf5-32f5-4721-aaf0-896930994f21
📒 Files selected for processing (3)
internal/config/errors.gointernal/purchase/manager.gointernal/purchase/manager_test.go
| mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-persist-fail", []string{"approved"}, "running"). | ||
| Return(&runningRow, nil) | ||
| mockStore.On("GetPurchasePlan", ctx, "plan-aws-persist-456").Return(plan, nil).Once() | ||
| mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) | ||
| mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) | ||
| // SavePurchaseExecution fails AFTER the CAS-to-running succeeded. | ||
| mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). | ||
| Return(saveErr) | ||
|
|
||
| mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{ | ||
| Account: aws.String("123456789012"), | ||
| }, nil) | ||
| mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) | ||
| mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) | ||
| mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ | ||
| Success: true, | ||
| CommitmentID: "ri-persist-fail-12345", | ||
| }, nil) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In github.com/stretchr/testify/mock v1.11.1, does mock.On(...).Return(...)withoutOnce()/Twice()/Times(n)permit multiple calls while still passingAssertExpectations?
💡 Result:
Yes. In testify/mock v1.11.1, if you set an expectation with mock.On(...).Return(...) and you do not add .Once(), .Twice(), or .Times(n), then the expectation is allowed to match and return repeatedly, and it will still pass AssertExpectations as long as the method/arguments match at least once. Reason (v1.11.1 behavior): the Call struct has a Repeatability field where the comments state that 0 means “to always return the value” [1]. The expectation satisfaction check in checkExpectation only fails on repeatability when call.Repeatability > 0; when Repeatability is 0, it does not enforce an upper bound [1]. AssertExpectations iterates over expectations and fails only when a non-optional, never-called expectation remains (call.totalCalls == 0 and not optional) or when repeatability is violated [1]. Practical implication: - If you write m.On("DoSomething", args...).Return(ret...) with no .Once/.Twice/.Times, it can be invoked multiple times and still satisfy AssertExpectations. - If you want to ensure it’s called only N times (or exactly once), you must set .Times(n)/.Once()/.Twice() because that sets Repeatability to a positive number that checkExpectation will use to fail when the repeatability condition is exceeded [1].
Citations:
Constrain side-effectful mock expectations to .Once() in internal/purchase/manager_test.go (lines 739-756).
testify/mock expectations defined as On(...).Return(...) without .Once()/.Times(n) can match multiple times and still pass AssertExpectations, so this test can be weakened against double-execution. Add .Once() to the side-effectful calls (transition/purchase/redrive/email/storage) to enforce the “no double-redrive/no double-purchase” contract.
Proposed tightening
mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-persist-fail", []string{"approved"}, "running").
- Return(&runningRow, nil)
+ Return(&runningRow, nil).Once()
mockStore.On("GetPurchasePlan", ctx, "plan-aws-persist-456").Return(plan, nil).Once()
-mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil)
-mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil)
+mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Once()
+mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil).Once()
// SavePurchaseExecution fails AFTER the CAS-to-running succeeded.
mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).
- Return(saveErr)
+ Return(saveErr).Once()
mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{
Account: aws.String("123456789012"),
- }, nil)
-mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil)
-mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil)
+ }, nil).Once()
+mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil).Once()
+mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil).Once()
mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{
Success: true,
CommitmentID: "ri-persist-fail-12345",
- }, nil)
+ }, nil).Once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-persist-fail", []string{"approved"}, "running"). | |
| Return(&runningRow, nil) | |
| mockStore.On("GetPurchasePlan", ctx, "plan-aws-persist-456").Return(plan, nil).Once() | |
| mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) | |
| mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) | |
| // SavePurchaseExecution fails AFTER the CAS-to-running succeeded. | |
| mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). | |
| Return(saveErr) | |
| mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{ | |
| Account: aws.String("123456789012"), | |
| }, nil) | |
| mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) | |
| mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil) | |
| mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ | |
| Success: true, | |
| CommitmentID: "ri-persist-fail-12345", | |
| }, nil) | |
| mockStore.On("TransitionExecutionStatus", ctx, "exec-aws-persist-fail", []string{"approved"}, "running"). | |
| Return(&runningRow, nil).Once() | |
| mockStore.On("GetPurchasePlan", ctx, "plan-aws-persist-456").Return(plan, nil).Once() | |
| mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil).Once() | |
| mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil).Once() | |
| // SavePurchaseExecution fails AFTER the CAS-to-running succeeded. | |
| mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). | |
| Return(saveErr).Once() | |
| mockSTS.On("GetCallerIdentity", ctx, mock.AnythingOfType("*sts.GetCallerIdentityInput")).Return(&sts.GetCallerIdentityOutput{ | |
| Account: aws.String("123456789012"), | |
| }, nil).Once() | |
| mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil).Once() | |
| mockProvider.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(mockServiceClient, nil).Once() | |
| mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ | |
| Success: true, | |
| CommitmentID: "ri-persist-fail-12345", | |
| }, nil).Once() |
🤖 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/manager_test.go` around lines 739 - 756, The mocks for
side-effectful operations are not constrained to run exactly once, weakening the
test; update the expectations to include .Once() on the side-effectful calls:
add .Once() to mockStore.TransitionExecutionStatus(...),
mockStore.SavePurchaseHistory(...), mockEmail.SendPurchaseConfirmation(...),
mockStore.SavePurchaseExecution(...), and
mockServiceClient.PurchaseCommitment(...) so each of those expectations must be
invoked exactly once (leave non-side-effect mocks like GetPurchasePlan,
GetCallerIdentity, CreateAndValidateProvider, GetServiceClient as appropriate).
…only on success (CR #728 round 3) The previous guard (`if execErr == nil`) only wrapped SavePurchaseExecution failures as ErrAuditLoss when executePurchase had itself succeeded. When executePurchase returned a provider error or partialPurchaseError AND SavePurchaseExecution then failed, the save failure was silently discarded: the row remained stranded in "running" with no terminal status persisted, and claimAndRedrive returned (false, nil), hiding the problem. Fix: always wrap SavePurchaseExecution failure as ErrAuditLoss. When execErr != nil, chain the original error as the innermost %w so errors.As/errors.Is callers can still reach it for diagnosis. When execErr == nil, keep the existing two-verb wrap (ErrAuditLoss + save err). Regression test: RecoverStrandedApprovals_AWSRedrive_ExecAndPersistBothFail simulates GetPurchasePlan failing (execErr != nil) followed by SavePurchaseExecution also failing. Asserts that the sweep surfaces a non-nil error, that errors.Is(err, config.ErrAuditLoss) holds, and that the original execution error remains reachable via errors.Is.
CR #728 round-3 fix: wrap ErrAuditLoss on every terminal save failureWhat was wrongThe previous Fix
The second Regression test
Asserts:
The pre-existing All 188 package tests pass. @coderabbitai review |
|
Triggering the full review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/purchase/manager.go (1)
271-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShort-circuit
ErrExecutionNotInExpectedStatusbefore the read-back probe.
TransitionExecutionStatusalready does its ownGetExecutionByIDand wraps the benign CAS-loss case withconfig.ErrExecutionNotInExpectedStatus. Re-reading here can turn that race into a hard sweep error if the second read flakes or the row disappears after the CAS rejection.claimAndRedriveandinternal/purchase/reaper.goalready treat this sentinel as terminally benign.Suggested fix
updated, txErr := m.config.TransitionExecutionStatus(ctx, exec.ExecutionID, []string{"approved"}, "failed") if txErr != nil { // ErrNotFound means the row vanished between the stale SELECT and // this CAS attempt (e.g. deleted by an operator or a concurrent // sweep already claimed and deleted it). That is a benign race-loss: // there is nothing left to fail, and the caller should not be // charged with an error. if errors.Is(txErr, config.ErrNotFound) { logging.Warnf("Skipping recovery of %s (row no longer exists, benign race-loss): %v", exec.ExecutionID, txErr) return false, nil } + if errors.Is(txErr, config.ErrExecutionNotInExpectedStatus) { + logging.Warnf("Skipping recovery of %s (already transitioned out of approved): %v", exec.ExecutionID, txErr) + return false, nil + } + // Distinguish benign races (row already left the "approved" // state - concurrent sweep handled it, or the original run // finished after the LIST snapshot) from real store🤖 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/manager.go` around lines 271 - 297, TransitionExecutionStatus can return the sentinel config.ErrExecutionNotInExpectedStatus for a benign CAS-loss; before performing the expensive/read-back probe, check if txErr is errors.Is(txErr, config.ErrExecutionNotInExpectedStatus) and treat it as a benign race by logging and returning (false, nil) (same behavior as claimAndRedrive/internal/purchase/reaper.go uses), only falling back to the GetExecutionByID probe for other txErr values so you don't convert a benign CAS rejection into a hard sweep error.
🤖 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.
Outside diff comments:
In `@internal/purchase/manager.go`:
- Around line 271-297: TransitionExecutionStatus can return the sentinel
config.ErrExecutionNotInExpectedStatus for a benign CAS-loss; before performing
the expensive/read-back probe, check if txErr is errors.Is(txErr,
config.ErrExecutionNotInExpectedStatus) and treat it as a benign race by logging
and returning (false, nil) (same behavior as
claimAndRedrive/internal/purchase/reaper.go uses), only falling back to the
GetExecutionByID probe for other txErr values so you don't convert a benign CAS
rejection into a hard sweep error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7916170-cd7d-4ed0-8729-05cd263288a9
📒 Files selected for processing (2)
internal/purchase/manager.gointernal/purchase/manager_test.go
…ail before read-back (CR #728 round 4) TransitionExecutionStatus already wraps the benign CAS-loss case with config.ErrExecutionNotInExpectedStatus when the row exists but its status has moved out of "approved". Re-reading via GetExecutionByID after receiving this sentinel is unnecessary: the sentinel itself already confirms the row left the target state. The probe can turn the race into a hard sweep error if the second read flakes or the row disappears between the CAS rejection and the read. Add a short-circuit returning (false, nil) for ErrExecutionNotInExpectedStatus immediately after the existing ErrNotFound short-circuit, mirroring the pattern already used by claimAndRedrive (manager.go:231) and the purchase reaper (reaper.go:186), both of which treat this sentinel as terminally benign. Regression test: TestManager_RecoverStrandedApprovals_SafeFail_ErrExecutionNotInExpectedStatusIsBenign asserts GetExecutionByID is never called and the sweep returns (0, nil).
CR round-4 fix: short-circuit
|
|
What's right about it:
Triggering the full review now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Refs #632. Full Azure inclusion blocked on #721 → #639; this PR ships the AWS-only subset so the field-observed strand class (all AWS) is auto-recovered today.
What changed
internal/purchase/manager.go:allRecsAWS(exec *config.PurchaseExecution) boolpredicate — returns true when every rec hasProvider == 'aws'orProvider == ''(empty = pre-multi-cloud legacy) AND at least one rec exists.RecoverStrandedApprovals: whenallRecsAWS(exec) && exec.ExecutionID != ''→ re-drives viaexecuteAndFinalize. The existingDeriveIdempotencyToken(exec.ExecutionID, i)atexecution.go:428produces the same token on re-drive, making the second AWS API call a safe no-op via the per-executor idempotency machinery (feat(purchases): extend IdempotencyToken to all commitment executors (AWS RDS/ElastiCache/MemoryDB/OpenSearch/Redshift, Azure, GCP) — blocks #639 #641).safeFailhelper extracted fromRecoverStrandedApprovalsto keep cyclomatic complexity at 10 (pre-commit hook enforces gocyclo:10). The safe-fail path for mixed / Azure-only / legacy executions is unchanged.Tests (4 new + 1 updated)
TestManager_RecoverStrandedApprovals_AWSOnlyRedrives— re-drive path, row ends ascompleted.TestManager_RecoverStrandedApprovals_MixedAWSAzureSafeFails— mixed execution falls through to safe-fail.TestManager_RecoverStrandedApprovals_PureAzureSafeFails— Azure-only falls through to safe-fail.TestManager_RecoverStrandedApprovals_LegacyNoExecutionIDSafeFails— empty ExecutionID falls through.TestManager_RecoverStrandedApprovals_FailsStrandedRowupdated fromProvider: awstoProvider: azureso it stays on the safe-fail path (original intent preserved).1457 tests pass across purchase + api.
go vet/ gofmt / golangci-lint clean.Scope guard
Azure-only and mixed strands deliberately fall through to today's safe-fail-and-Retry. Once #721 (Azure DoPurchaseTwoStep idempotency regression) lands, #639 can flip the predicate to all-providers.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation