From 97b2a72e7f6b7ac0132289e13a2c9bd76ba33401 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 20:42:46 +0200 Subject: [PATCH] feat(purchases): idempotent re-drive for Azure/GCP (closes #639) Replace the AWS-only allRecsAWS predicate in RecoverStrandedApprovals with allRecsSafeToRedrive + recIsSafeToRedrive, extending the idempotent re-drive path to Azure reservation clients (compute, database, cache, cosmosdb, managedredis, search, synapse -- all use DoIdempotentPurchaseTwoStep from #729/#721) and GCP compute CUDs (server- side RequestId + deterministic name from the token, #654). Azure Savings Plans keeps the safe-fail path: the OrderAlias API uses time.Now().UnixNano() as the alias name with no server-side idempotency key, so a re-drive would create a duplicate savings plan. Tests: update three Azure safe-fail tests from reservations to savingsplans (the remaining unsafe service), add two new tests (AzureReservationRedrives, GCPRedrives) that exercise the expanded re-drive path end-to-end with mock provider calls. 191 tests pass; go build clean. --- internal/purchase/manager.go | 109 +++++++++----- internal/purchase/manager_test.go | 232 +++++++++++++++++++++++++----- 2 files changed, 274 insertions(+), 67 deletions(-) diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index d2b3414ae..4e92698e1 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -185,31 +185,66 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE return execErr } -// allRecsAWS reports whether every recommendation in the execution targets AWS. -// Empty provider ("") is treated as AWS because pre-multi-cloud rows predate the -// provider field and are all AWS. An execution with no recommendations returns -// false so it falls through to the safe-fail path (nothing to re-drive anyway). +// allRecsSafeToRedrive reports whether every recommendation in the execution +// can be safely re-driven without risking a double-purchase. A re-drive is safe +// when the underlying provider purchase API is idempotent under the +// DeriveIdempotencyToken(exec.ExecutionID, i) scheme used by execution.go. // -// Azure and GCP recs are excluded: Azure re-drive idempotency is blocked by issue -// #721 (#639), and GCP commitments are currently flagged unsupported (#640). Only -// call executeAndFinalize on an execution when this predicate returns true AND the -// execution has a non-empty ExecutionID (needed for DeriveIdempotencyToken). -func allRecsAWS(exec *config.PurchaseExecution) bool { +// Safe providers / services (issue #639): +// - AWS (all services): tag-guard or ClientToken deduplication (#636/#638). +// - Azure reservations (compute, relational-db, cache, nosql, memorydb, +// search, data-warehouse): DoIdempotentPurchaseTwoStep performs a +// tag-based lookup before purchasing (#729 / #721). +// - GCP compute (CUDs): server-side RequestId + deterministic name from +// the token (#654). +// +// NOT safe - safe-fail path preserved: +// - Azure savings-plans: the OrderAlias API uses time.Now().UnixNano() as +// the alias name; there is no server-side idempotency key and no +// tag-based lookup implemented yet. Re-driving would create a duplicate +// savings plan. +// +// Empty provider ("") is treated as AWS (pre-multi-cloud legacy rows). +// An execution with no recommendations returns false so it falls through to the +// safe-fail path (nothing to re-drive anyway). +func allRecsSafeToRedrive(exec *config.PurchaseExecution) bool { if len(exec.Recommendations) == 0 { return false } for _, rec := range exec.Recommendations { - if rec.Provider != "" && rec.Provider != "aws" { + if !recIsSafeToRedrive(rec) { return false } } return true } -// claimAndRedrive atomically claims a stranded AWS-only execution (by -// transitioning its status from "approved" to "running") and then re-drives it -// via executeAndFinalize. It is extracted from RecoverStrandedApprovals to keep -// that function's cyclomatic complexity within the gocyclo:10 limit. +// recIsSafeToRedrive reports whether a single recommendation can be safely +// re-driven. Extracted from allRecsSafeToRedrive to keep that function under +// the gocyclo budget and to make per-rec exclusions explicit. +func recIsSafeToRedrive(rec config.RecommendationRecord) bool { + switch rec.Provider { + case "", "aws": + // Empty provider is legacy AWS. All AWS services honour IdempotencyToken. + return true + case "azure": + // Azure savings-plans uses a timestamp-based alias name and has no + // server-side idempotency key, so a re-drive would create a duplicate. + // All other Azure services use DoIdempotentPurchaseTwoStep (#729). + return rec.Service != "savingsplans" && rec.Service != "savings-plans" + case "gcp": + // GCP compute CUDs use RequestId + deterministic name from the token (#654). + return true + default: + // Unknown provider: refuse to re-drive rather than risk a double-buy. + return false + } +} + +// claimAndRedrive atomically claims a stranded execution (by transitioning its +// status from "approved" to "running") and then re-drives it via +// executeAndFinalize. It is extracted from RecoverStrandedApprovals to keep that +// function's cyclomatic complexity within the gocyclo:10 limit. // // Returns (true, nil) when the claim was won and the re-drive completed (row is // now in a terminal state). A re-drive error is not fatal -- executeAndFinalize @@ -237,7 +272,7 @@ func (m *Manager) claimAndRedrive(ctx context.Context, exec *config.PurchaseExec // Update the local struct to reflect the committed DB state so // finalizeExecution starts from "running" rather than "approved". exec.Status = claimed.Status - logging.Infof("Recovering stranded AWS-only execution %s via idempotent re-drive (issue #632)", exec.ExecutionID) + logging.Infof("Recovering stranded execution %s via idempotent re-drive (issue #639)", exec.ExecutionID) if driveErr := m.executeAndFinalize(ctx, exec); driveErr != nil { logging.Errorf("Re-drive of stranded execution %s failed: %v", exec.ExecutionID, driveErr) // Persistence failures (ErrAuditLoss) are non-benign: the row was CAS-ed @@ -318,23 +353,27 @@ func (m *Manager) safeFail(ctx context.Context, exec *config.PurchaseExecution) } // RecoverStrandedApprovals finds executions stuck in the "approved" status past -// staleApprovedThreshold and either re-drives them idempotently (AWS-only -// executions with a durable ExecutionID) or drives them into a terminal "failed" -// state (mixed/Azure/GCP or legacy rows without a stable ExecutionID). +// staleApprovedThreshold and either re-drives them idempotently (executions +// where every rec is safe to re-drive and the row has a durable ExecutionID) +// or drives them into a terminal "failed" state (rows with unsafe recs or +// without a stable ExecutionID). // -// AWS-only path (issue #632 Option 5): all AWS executors derive a deterministic -// per-rec idempotency token via common.DeriveIdempotencyToken(exec.ExecutionID, i) -// at purchase time (execution.go:428). Re-driving with the same ExecutionID -// produces the same token, so AWS dedupes the second call and no double-purchase -// occurs. The row transitions from "approved" directly to "completed" (or -// "failed"/"partially_completed" on a genuine error), bypassing the manual Retry -// step required by the old safe-fail path. +// Idempotent re-drive path (issue #639): all AWS, Azure reservations, and GCP +// compute service clients derive or look up a deterministic idempotency key +// from DeriveIdempotencyToken(exec.ExecutionID, i). Re-driving with the same +// ExecutionID produces the same token, so the cloud provider dedupes the second +// call and no double-purchase occurs. The row transitions directly to "completed" +// (or "failed"/"partially_completed" on a genuine error), bypassing the manual +// Retry step required by the old safe-fail path. See allRecsSafeToRedrive for +// which provider/service combinations are eligible. // -// Safe-fail path (mixed/Azure/GCP/legacy): Azure two-step idempotency is blocked -// by issue #721; GCP is flagged unsupported (#640). Executions without a stable -// ExecutionID cannot derive tokens safely. These fall through to the original -// behaviour: the row is atomically transitioned to "failed" so it surfaces in -// History and can be Retry-ed by an operator after confirming the cloud-side state. +// Safe-fail path: Azure savings-plans recs are excluded because the OrderAlias +// API uses a timestamp-based alias name with no idempotency key. Executions +// without a stable ExecutionID (legacy rows) also fall through because +// DeriveIdempotencyToken("", i) would produce the same token set for every +// such row. These fall through to the original behaviour: the row is atomically +// transitioned to "failed" so it surfaces in History and can be Retry-ed by +// an operator after confirming the cloud-side state. // // The transition in the safe-fail path is atomic: TransitionExecutionStatus only // flips rows still in "approved", so if the original run finally completes between @@ -350,12 +389,12 @@ func (m *Manager) RecoverStrandedApprovals(ctx context.Context) (int, error) { for i := range stranded { exec := &stranded[i] - // AWS-only re-drive path (issue #632 Option 5): all AWS executors honour + // Idempotent re-drive path (issue #639): all recs honour // opts.IdempotencyToken via DeriveIdempotencyToken(exec.ExecutionID, i), - // so a second call with the same ExecutionID is a safe no-op on the AWS - // side. The ExecutionID must be non-empty to derive a unique token; an - // empty ID would map every legacy row to the same token set. - if allRecsAWS(exec) && exec.ExecutionID != "" { + // so a second call with the same ExecutionID is a safe no-op on the + // provider side. The ExecutionID must be non-empty to derive a unique + // token; an empty ID would map every legacy row to the same token set. + if allRecsSafeToRedrive(exec) && exec.ExecutionID != "" { counted, driveErr := m.claimAndRedrive(ctx, exec) if driveErr != nil { return recovered, driveErr diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index 5a5aea4d7..2afe531bb 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -319,12 +319,13 @@ func TestManager_ProcessScheduledPurchases_ExecutionFails(t *testing.T) { } // TestManager_RecoverStrandedApprovals_FailsStrandedRow is the regression test -// for issue #632 safe-fail path: an Azure execution flipped to "approved" whose -// synchronous purchase run was interrupted before it finalized must NOT stay -// permanently "approved". Azure re-drive idempotency is blocked by issue #721, so -// the recovery sweep drives the row into a terminal "failed" state with a clear -// error and does NOT re-run the purchase (no provider/service-client calls), -// eliminating any double-purchase risk for non-AWS providers. +// for issue #632 safe-fail path: an Azure Savings Plans execution flipped to +// "approved" whose synchronous purchase run was interrupted before it finalized +// must NOT stay permanently "approved". Azure Savings Plans re-drive is unsafe +// because the OrderAlias API uses a timestamp-based alias name with no server-side +// idempotency key (#639), so the recovery sweep drives the row into a terminal +// "failed" state with a clear error and does NOT re-run the purchase (no +// provider/service-client calls), eliminating double-purchase risk. func TestManager_RecoverStrandedApprovals_FailsStrandedRow(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) @@ -336,8 +337,8 @@ func TestManager_RecoverStrandedApprovals_FailsStrandedRow(t *testing.T) { PlanID: "plan-456", Status: "approved", Recommendations: []config.RecommendationRecord{ - // Azure provider: falls through to safe-fail path (issue #721 blocks re-drive). - {Provider: "azure", Service: "reservations", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 500.0, Selected: true, Purchased: false}, + // Azure Savings Plans: safe-fail path because OrderAlias has no idempotency key. + {Provider: "azure", Service: "savingsplans", ResourceType: "Compute", Region: "eastus", Count: 1, UpfrontCost: 500.0, Selected: true, Purchased: false}, }, } failedRow := stranded @@ -366,7 +367,7 @@ func TestManager_RecoverStrandedApprovals_FailsStrandedRow(t *testing.T) { assert.Equal(t, 1, recovered) require.NotNil(t, saved) - assert.Equal(t, "failed", saved.Status, "stranded Azure row must become terminally failed, never stay approved") + assert.Equal(t, "failed", saved.Status, "stranded Azure Savings Plans row must become terminally failed, never stay approved") assert.NotEmpty(t, saved.Error, "the failed row must carry a clear, operator-readable error") assert.Contains(t, saved.Error, "interrupted") assert.False(t, saved.Recommendations[0].Purchased, "recovery must not mark anything purchased") @@ -494,22 +495,187 @@ func TestManager_RecoverStrandedApprovals_AWSOnlyRedrives(t *testing.T) { mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed") } -// TestManager_RecoverStrandedApprovals_MixedAWSAzureSafeFails verifies that a -// stranded execution containing both AWS and Azure recommendations falls through -// to the safe-fail path rather than being re-driven. Azure re-drive idempotency -// is blocked by issue #721; a mixed execution must never be auto-re-driven. -func TestManager_RecoverStrandedApprovals_MixedAWSAzureSafeFails(t *testing.T) { +// TestManager_RecoverStrandedApprovals_AzureReservationRedrives verifies that a +// stranded Azure reservation execution (compute, database, cache, etc.) is +// re-driven via executeAndFinalize rather than failed. All Azure reservation +// service clients call DoIdempotentPurchaseTwoStep with opts.IdempotencyToken +// (PR #729 / issue #721), so a re-drive with the same ExecutionID is a safe +// no-op on the Azure side (#639). +func TestManager_RecoverStrandedApprovals_AzureReservationRedrives(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProvider := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + t.Cleanup(func() { mockFactory.AssertExpectations(t) }) + t.Cleanup(func() { mockProvider.AssertExpectations(t) }) + t.Cleanup(func() { mockServiceClient.AssertExpectations(t) }) + + stranded := config.PurchaseExecution{ + ExecutionID: "exec-azure-res-stranded", + PlanID: "plan-azure-res", + Status: "approved", + Recommendations: []config.RecommendationRecord{ + {Provider: "azure", Service: "compute", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 300.0, Selected: true, Purchased: false}, + }, + } + runningRow := stranded + runningRow.Status = "running" + + plan := &config.PurchasePlan{ + ID: "plan-azure-res", + Name: "Azure Reservation Plan", + RampSchedule: config.RampSchedule{ + CurrentStep: 0, + TotalSteps: 2, + }, + } + + mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). + Return([]config.PurchaseExecution{stranded}, nil) + // CAS claim: approved -> running before re-drive. + mockStore.On("TransitionExecutionStatus", ctx, "exec-azure-res-stranded", []string{"approved"}, "running"). + Return(&runningRow, nil) + mockStore.On("GetPurchasePlan", ctx, "plan-azure-res").Return(plan, nil).Twice() + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }). + Return(nil) + mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + + mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.Anything).Return(mockProvider, nil) + mockProvider.On("GetServiceClient", mock.Anything, common.ServiceCompute, "eastus").Return(mockServiceClient, nil) + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ + Success: true, + CommitmentID: "azure-res-idempotent-order-id", + }, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } + + recovered, err := manager.RecoverStrandedApprovals(ctx) + require.NoError(t, err) + assert.Equal(t, 1, recovered, "Azure reservation strand must be counted as recovered after re-drive") + + require.NotNil(t, saved) + assert.Equal(t, "completed", saved.Status, "successfully re-driven Azure reservation must be completed, not failed") + + // Provider was reached: re-drive called PurchaseCommitment exactly once. + mockServiceClient.AssertCalled(t, "PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")) + // CAS claim (approved -> running) was called; "failed" transition was not. + mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-azure-res-stranded", []string{"approved"}, "running") + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed") +} + +// TestManager_RecoverStrandedApprovals_GCPRedrives verifies that a stranded GCP +// compute execution is re-driven via executeAndFinalize rather than failed. The GCP +// compute client uses server-side RequestId + deterministic name from the +// IdempotencyToken (#654), so a re-drive with the same ExecutionID is a safe no-op +// on the GCP side (#639). +func TestManager_RecoverStrandedApprovals_GCPRedrives(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProvider := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + t.Cleanup(func() { mockEmail.AssertExpectations(t) }) + t.Cleanup(func() { mockFactory.AssertExpectations(t) }) + t.Cleanup(func() { mockProvider.AssertExpectations(t) }) + t.Cleanup(func() { mockServiceClient.AssertExpectations(t) }) + + stranded := config.PurchaseExecution{ + ExecutionID: "exec-gcp-stranded", + PlanID: "plan-gcp", + Status: "approved", + Recommendations: []config.RecommendationRecord{ + {Provider: "gcp", Service: "compute", ResourceType: "n2-standard-4", Region: "us-central1", Count: 2, UpfrontCost: 150.0, Selected: true, Purchased: false}, + }, + } + runningRow := stranded + runningRow.Status = "running" + + plan := &config.PurchasePlan{ + ID: "plan-gcp", + Name: "GCP CUD Plan", + RampSchedule: config.RampSchedule{ + CurrentStep: 0, + TotalSteps: 2, + }, + } + + mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). + Return([]config.PurchaseExecution{stranded}, nil) + // CAS claim: approved -> running before re-drive. + mockStore.On("TransitionExecutionStatus", ctx, "exec-gcp-stranded", []string{"approved"}, "running"). + Return(&runningRow, nil) + mockStore.On("GetPurchasePlan", ctx, "plan-gcp").Return(plan, nil).Twice() + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + var saved *config.PurchaseExecution + mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.PurchaseExecution) }). + Return(nil) + mockStore.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + + mockFactory.On("CreateAndValidateProvider", mock.Anything, "gcp", mock.Anything).Return(mockProvider, nil) + mockProvider.On("GetServiceClient", mock.Anything, common.ServiceCompute, "us-central1").Return(mockServiceClient, nil) + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")).Return(common.PurchaseResult{ + Success: true, + CommitmentID: "cud-idempotent-gcp", + }, nil) + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } + + recovered, err := manager.RecoverStrandedApprovals(ctx) + require.NoError(t, err) + assert.Equal(t, 1, recovered, "GCP strand must be counted as recovered after re-drive") + + require.NotNil(t, saved) + assert.Equal(t, "completed", saved.Status, "successfully re-driven GCP execution must be completed, not failed") + + // Provider was reached: re-drive called PurchaseCommitment exactly once. + mockServiceClient.AssertCalled(t, "PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")) + // CAS claim (approved -> running) was called; "failed" transition was not. + mockStore.AssertCalled(t, "TransitionExecutionStatus", ctx, "exec-gcp-stranded", []string{"approved"}, "running") + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, "failed") +} + +// TestManager_RecoverStrandedApprovals_MixedAWSAzureSPSafeFails verifies that a +// stranded execution containing AWS and Azure Savings Plans recommendations falls +// through to the safe-fail path rather than being re-driven. Azure Savings Plans +// uses a timestamp-based alias name with no idempotency key, so a mixed execution +// that includes at least one savings-plans rec must never be auto-re-driven. +func TestManager_RecoverStrandedApprovals_MixedAWSAzureSPSafeFails(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockFactory := new(MockProviderFactory) stranded := config.PurchaseExecution{ - ExecutionID: "exec-mixed", - PlanID: "plan-mixed", + ExecutionID: "exec-mixed-sp", + PlanID: "plan-mixed-sp", Status: "approved", Recommendations: []config.RecommendationRecord{ {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 100.0, Selected: true}, - {Provider: "azure", Service: "reservations", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 100.0, Selected: true}, + // Azure Savings Plans: not safe for re-drive; entire execution falls to safe-fail. + {Provider: "azure", Service: "savingsplans", ResourceType: "Compute", Region: "eastus", Count: 1, UpfrontCost: 100.0, Selected: true}, }, } failedRow := stranded @@ -517,7 +683,7 @@ func TestManager_RecoverStrandedApprovals_MixedAWSAzureSafeFails(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) - mockStore.On("TransitionExecutionStatus", ctx, "exec-mixed", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-mixed-sp", []string{"approved"}, "failed"). Return(&failedRow, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) @@ -531,25 +697,27 @@ func TestManager_RecoverStrandedApprovals_MixedAWSAzureSafeFails(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, recovered) - // No provider call: mixed execution falls through to safe-fail, not re-driven. + // No provider call: execution with savings-plans rec falls through to safe-fail. mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertExpectations(t) } -// TestManager_RecoverStrandedApprovals_PureAzureSafeFails verifies that a stranded -// execution whose every recommendation targets Azure falls through to the safe-fail -// path and is never re-driven (issue #721 guard against future regression). -func TestManager_RecoverStrandedApprovals_PureAzureSafeFails(t *testing.T) { +// TestManager_RecoverStrandedApprovals_AzureSavingsPlansSafeFails verifies that a +// stranded execution whose every recommendation targets Azure Savings Plans falls +// through to the safe-fail path and is never re-driven. The OrderAlias API uses +// a timestamp-based alias name with no server-side idempotency key, so re-driving +// would create a duplicate savings plan (#639). +func TestManager_RecoverStrandedApprovals_AzureSavingsPlansSafeFails(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore) mockFactory := new(MockProviderFactory) stranded := config.PurchaseExecution{ - ExecutionID: "exec-azure", - PlanID: "plan-azure", + ExecutionID: "exec-azure-sp", + PlanID: "plan-azure-sp", Status: "approved", Recommendations: []config.RecommendationRecord{ - {Provider: "azure", Service: "reservations", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 2, UpfrontCost: 400.0, Selected: true}, + {Provider: "azure", Service: "savingsplans", ResourceType: "Compute", Region: "eastus", Count: 2, UpfrontCost: 400.0, Selected: true}, }, } failedRow := stranded @@ -557,7 +725,7 @@ func TestManager_RecoverStrandedApprovals_PureAzureSafeFails(t *testing.T) { mockStore.On("GetStaleApprovedExecutions", ctx, staleApprovedThreshold). Return([]config.PurchaseExecution{stranded}, nil) - mockStore.On("TransitionExecutionStatus", ctx, "exec-azure", []string{"approved"}, "failed"). + mockStore.On("TransitionExecutionStatus", ctx, "exec-azure-sp", []string{"approved"}, "failed"). Return(&failedRow, nil) mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) @@ -571,7 +739,7 @@ func TestManager_RecoverStrandedApprovals_PureAzureSafeFails(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, recovered) - // Safe-fail: Azure-only execution must never reach the provider. + // Safe-fail: Azure Savings Plans execution must never reach the provider. mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) mockStore.AssertExpectations(t) } @@ -664,8 +832,8 @@ func TestManager_RecoverStrandedApprovals_SafeFail_ErrNotFoundIsBenign(t *testin ExecutionID: "exec-vanished", Status: "approved", Recommendations: []config.RecommendationRecord{ - // Azure provider falls through to safe-fail path. - {Provider: "azure", Service: "reservations", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 400.0, Selected: true}, + // Azure Savings Plans falls through to safe-fail path (no idempotency key). + {Provider: "azure", Service: "savingsplans", ResourceType: "Compute", Region: "eastus", Count: 1, UpfrontCost: 400.0, Selected: true}, }, } @@ -707,8 +875,8 @@ func TestManager_RecoverStrandedApprovals_SafeFail_ErrExecutionNotInExpectedStat ExecutionID: "exec-already-transitioned", Status: "approved", Recommendations: []config.RecommendationRecord{ - // Azure provider routes to safe-fail (not claimAndRedrive). - {Provider: "azure", Service: "reservations", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 400.0, Selected: true}, + // Azure Savings Plans routes to safe-fail (no idempotency key for re-drive). + {Provider: "azure", Service: "savingsplans", ResourceType: "Compute", Region: "eastus", Count: 1, UpfrontCost: 400.0, Selected: true}, }, }