diff --git a/internal/purchase/elasticache_duplicate_pool.go b/internal/purchase/elasticache_duplicate_pool.go new file mode 100644 index 000000000..fac6e5dfa --- /dev/null +++ b/internal/purchase/elasticache_duplicate_pool.go @@ -0,0 +1,63 @@ +package purchase + +import ( + "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +const elastiCacheDuplicatePoolRefusal = "refusing to dispatch: this purchase selects the same ElastiCache pool (region, node type, engine) twice for one account, and AWS would treat the second as already covered by the first, so remove the duplicate recommendation and start a new purchase, or ask an operator if this is a plan step" + +// hasDuplicateElastiCachePool reports whether two selected AWS ElastiCache recs share an account, +// region, node type and engine. Concurrent per-rec checks cannot see each other's purchase, so both buy. +// Term, payment and count are not in the key: the go library's pool key omits them too. +func hasDuplicateElastiCachePool(exec *config.PurchaseExecution) bool { + seen := map[string]bool{} + for _, i := range selectedIndices(exec.Recommendations) { + key, ok := elastiCachePoolKey(exec, exec.Recommendations[i]) + if !ok { + continue + } + if seen[key] { + return true + } + seen[key] = true + } + return false +} + +// elastiCachePoolKey returns the pool a rec buys into, or false for a rec the library refuses anyway. +func elastiCachePoolKey(exec *config.PurchaseExecution, rec config.RecommendationRecord) (string, bool) { + service, _ := mapServiceSlug(rec.Service) + if !isAWSElastiCache(rec.Provider, service) || rec.Count <= 0 { + return "", false + } + engine := elastiCacheBuyEngine(rec) + if engine == "" { + return "", false + } + account := "" + switch { + case rec.CloudAccountID != nil: + account = *rec.CloudAccountID + case exec.CloudAccountID != nil: + account = *exec.CloudAccountID + } + return strings.Join([]string{account, rec.Region, rec.ResourceType, engine}, "|"), true +} + +// elastiCacheBuyEngine is the engine the buy path sends, trimmed then normalized; empty when the +// details do not decode into cache details. +func elastiCacheBuyEngine(rec config.RecommendationRecord) string { + details, err := common.DecodeServiceDetailsFor(rec.Service, rec.Details) + if err != nil { + return "" + } + cache, ok := details.(*common.CacheDetails) + if !ok { + return "" + } + applyEngineFallback(cache, rec.Engine) + return common.NormalizeEngineName(strings.TrimSpace(cache.Engine)) +} diff --git a/internal/purchase/elasticache_duplicate_pool_test.go b/internal/purchase/elasticache_duplicate_pool_test.go new file mode 100644 index 000000000..46bf9bd85 --- /dev/null +++ b/internal/purchase/elasticache_duplicate_pool_test.go @@ -0,0 +1,119 @@ +package purchase + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func ecDupExec(recs ...config.RecommendationRecord) *config.PurchaseExecution { + return &config.PurchaseExecution{ + ExecutionID: "ec-dup", IdempotencyKey: "ec-dup-lineage", Status: "running", Recommendations: recs, + } +} + +func TestElastiCacheDuplicatePool_Detection(t *testing.T) { + base := func() config.RecommendationRecord { return ecRecWith(t, "redis", 2, "cache.r6g.large", ecRegion) } + with := func(f func(*config.RecommendationRecord)) config.RecommendationRecord { + r := base() + f(&r) + return r + } + unselected := with(func(r *config.RecommendationRecord) { r.Selected = false }) + memorydb := with(func(r *config.RecommendationRecord) { r.Service = "memorydb" }) + azure := with(func(r *config.RecommendationRecord) { r.Provider = "azure" }) + noAcct := withAccount(base(), "acct-A") + noAcct.CloudAccountID = nil + canonical := with(func(r *config.RecommendationRecord) { r.Service = "cache" }) + + cases := map[string]struct { + recs []config.RecommendationRecord + refused bool + }{ + "identical": {[]config.RecommendationRecord{base(), base()}, true}, + "different count": {[]config.RecommendationRecord{base(), with(func(r *config.RecommendationRecord) { r.Count = 5 })}, true}, + "different term and payment": {[]config.RecommendationRecord{base(), with(func(r *config.RecommendationRecord) { r.Term, r.Payment = 3, "all-upfront" })}, true}, + "untrimmed mixed-case engine": {[]config.RecommendationRecord{base(), ecRecWith(t, " Redis ", 2, "cache.r6g.large", ecRegion)}, true}, + "redis vs valkey": {[]config.RecommendationRecord{base(), ecRecWith(t, "valkey", 2, "cache.r6g.large", ecRegion)}, false}, + "other account": {[]config.RecommendationRecord{withAccount(base(), "acct-A"), withAccount(base(), "acct-B")}, false}, + "other region": {[]config.RecommendationRecord{base(), with(func(r *config.RecommendationRecord) { r.Region = "eu-west-1" })}, false}, + "unselected twin": {[]config.RecommendationRecord{base(), unselected}, false}, + "aws memorydb never counted": {[]config.RecommendationRecord{memorydb, memorydb}, false}, + "azure cache never counted": {[]config.RecommendationRecord{azure, azure}, false}, + "count zero skipped": {[]config.RecommendationRecord{with(func(r *config.RecommendationRecord) { r.Count = 0 }), with(func(r *config.RecommendationRecord) { r.Count = 0 })}, false}, + "empty engine skipped": {[]config.RecommendationRecord{ecRecWith(t, "", 2, "cache.r6g.large", ecRegion), ecRecWith(t, "", 2, "cache.r6g.large", ecRegion)}, false}, + "legacy slug vs canonical": {[]config.RecommendationRecord{base(), canonical}, true}, + "no account vs scoped twin": {[]config.RecommendationRecord{noAcct, withAccount(base(), "acct-X")}, true}, + "node type compared raw": {[]config.RecommendationRecord{base(), with(func(r *config.RecommendationRecord) { r.ResourceType = "cache.R6G.large" })}, false}, + "different node type": {[]config.RecommendationRecord{base(), with(func(r *config.RecommendationRecord) { r.ResourceType = "cache.r6g.xlarge" })}, false}, + "engine only on the rec column": {[]config.RecommendationRecord{base(), with(func(r *config.RecommendationRecord) { r.Details, r.Engine = nil, "redis" })}, true}, + "single rec": {[]config.RecommendationRecord{base()}, false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + exec := ecDupExec(tc.recs...) + if name == "no account vs scoped twin" { + acct := "acct-X" + exec.CloudAccountID = &acct + } + assert.Equal(t, tc.refused, hasDuplicateElastiCachePool(exec)) + }) + } +} + +func TestElastiCacheDuplicatePool_RefusedBeforeSubmission(t *testing.T) { + exec := ecDupExec(ecCacheRec(t, "elasticache"), ecCacheRec(t, "elasticache")) + run := runRedshiftFinalize(t, exec, nil, common.ServiceElastiCache, nil) + + require.Error(t, run.err) + assert.Equal(t, elastiCacheDuplicatePoolRefusal, run.err.Error()) + assert.True(t, isBeforeSubmission(run.err)) + assert.True(t, exec.NoSubmission) + assert.Equal(t, "failed", exec.Status) + assert.Empty(t, run.opts, "no PurchaseCommitment call") + require.NotEmpty(t, run.saves) + stored := run.saves[len(run.saves)-1].Error + assert.Equal(t, elastiCacheDuplicatePoolRefusal, stored) + assert.Equal(t, stored, SanitizeStoredError(stored)) +} + +func TestElastiCacheDuplicatePool_PlanStepRootRefusedAndSuppressionsReleased(t *testing.T) { + exec := ecDupExec(ecCacheRec(t, "elasticache"), ecCacheRec(t, "elasticache")) + exec.PlanID, exec.StepNumber = "plan-x", 1 + run := runRedshiftFinalize(t, exec, nil, common.ServiceElastiCache, nil) + + require.Error(t, run.err) + assert.Equal(t, elastiCacheDuplicatePoolRefusal, run.err.Error()) + assert.Empty(t, run.opts) + store := run.store // the vacuous-assertion guard resolves only plain identifiers + store.AssertCalled(t, "DeleteSuppressionsByExecutionTx") +} + +// Fan-out children hold only their own account's recs, so same-pool recs of different accounts never collide. +func TestElastiCacheDuplicatePool_FanOutDifferentAccountsNotRefused(t *testing.T) { + root := ecDupExec(withAccount(ecCacheRec(t, "elasticache"), "acct-A"), withAccount(ecCacheRec(t, "elasticache"), "acct-B")) + root.PlanID, root.StepNumber = "plan-x", 1 + m := &Manager{config: new(MockConfigStore)} + + require.NoError(t, m.preExecutionRefusal(context.Background(), root)) + children, err := scopeExecutionsByAccount(root, []config.CloudAccount{{ID: "acct-A"}, {ID: "acct-B"}}) + require.NoError(t, err) + require.Len(t, children, 2) + for _, child := range children { + assert.NoError(t, m.preExecutionRefusal(context.Background(), child)) + } +} + +func TestElastiCacheDuplicatePool_RetryRefusedAgain(t *testing.T) { + retry := ecDupExec(ecCacheRec(t, "elasticache"), ecCacheRec(t, "elasticache")) + retry.RetryAttemptN = 1 + run := runRedshiftFinalize(t, retry, nil, common.ServiceElastiCache, nil) + + require.Error(t, run.err) + assert.Equal(t, elastiCacheDuplicatePoolRefusal, run.err.Error()) + assert.Empty(t, run.opts) +} diff --git a/internal/purchase/elasticache_recent_check.go b/internal/purchase/elasticache_recent_check.go index fb8c4b883..cd47efd0f 100644 --- a/internal/purchase/elasticache_recent_check.go +++ b/internal/purchase/elasticache_recent_check.go @@ -38,6 +38,8 @@ func isAWSElastiCache(providerName string, service common.ServiceType) bool { // selected AWS ElastiCache recs will create, from the same per-rec tokens the // purchase path passes to the provider. A listed reservation with one of these // IDs came from this lineage, not from another purchase. +// Two same-pool recs of one execution cannot see each other here; hasDuplicateElastiCachePool +// refuses that selection before submission. func elastiCacheOwnReservationIDs(exec *config.PurchaseExecution, selected []int) map[string]bool { ids := make(map[string]bool) for _, i := range selected { diff --git a/internal/purchase/elasticache_recent_check_scenarios_test.go b/internal/purchase/elasticache_recent_check_scenarios_test.go index 693f851cc..a33a7247c 100644 --- a/internal/purchase/elasticache_recent_check_scenarios_test.go +++ b/internal/purchase/elasticache_recent_check_scenarios_test.go @@ -6,13 +6,16 @@ package purchase // marked "regression guard" pass on main too; the refusals fail there. import ( + "context" "testing" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/elasticache/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -168,17 +171,45 @@ func TestElastiCacheScenarios_SiblingDifferentPoolsBothBought(t *testing.T) { assert.Len(t, api.purchases(), 2) } -// Characterization test (D6, accepted gap, follow-up issue): two recs of the SAME -// pool in one execution get different tokens, their listings race, and both buy. -// A later fix must flip this assertion deliberately. -func TestElastiCacheScenarios_CharacterizationSamePoolTwiceInOneExecutionBothBought(t *testing.T) { +// runECFinalize drives recs through executeAndFinalize, the only path that runs +// preExecutionRefusal, over the real elasticache client on the shared fake API. +func runECFinalize(t *testing.T, api *fakeElastiCacheAPI, recs ...config.RecommendationRecord) (*config.PurchaseExecution, error) { + t.Helper() + store, sender, factory, prov := new(MockConfigStore), new(MockEmailSender), new(MockProviderFactory), new(MockProvider) + store.SavePurchaseExecutionFn = func(context.Context, *config.PurchaseExecution) error { return nil } + store.On("SavePurchaseHistory", mock.Anything, mock.Anything).Return(nil).Maybe() + sender.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil).Maybe() + sender.On("SendPurchaseFailedNotification", mock.Anything, mock.Anything).Return(nil).Maybe() + factory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(prov, nil).Maybe() + prov.On("GetServiceClient", mock.Anything, common.ServiceElastiCache, ecRegion).Return(provider.ServiceClient(newFakeElastiCacheClient(api)), nil).Maybe() + exec := &config.PurchaseExecution{ExecutionID: "exec-1867", IdempotencyKey: "lineage-1867", Status: "running", Recommendations: recs} + m := &Manager{config: store, email: sender, providerFactory: factory, credStore: awsAccessKeyCredStore()} + return exec, m.executeAndFinalize(context.Background(), exec) +} + +// #1867: two recs of the SAME pool in one execution would each get their own +// token and race their listings, so the selection is refused before submission. +func TestElastiCacheScenarios_SamePoolTwiceInOneExecutionIsRefused(t *testing.T) { api := &fakeElastiCacheAPI{} - _, errs, allRefused := runECExecution(t, api, ecCacheRec(t, "elasticache"), ecCacheRec(t, "elasticache")) + exec, err := runECFinalize(t, api, ecCacheRec(t, "elasticache"), ecCacheRec(t, "elasticache")) - assert.Empty(t, errs) - assert.False(t, allRefused) - assert.Len(t, api.purchases(), 2, "D6: the check cannot see a same-execution sibling") + require.Error(t, err) + assert.Equal(t, elastiCacheDuplicatePoolRefusal, err.Error()) + assert.Empty(t, api.purchases()) + assert.Equal(t, "failed", exec.Status) +} + +// Control: with the twin unselected there is no duplicate, so the one rec buys. +func TestElastiCacheScenarios_SamePoolTwinUnselectedBuysOnce(t *testing.T) { + api := &fakeElastiCacheAPI{} + twin := ecCacheRec(t, "elasticache") + twin.Selected = false + + _, err := runECFinalize(t, api, ecCacheRec(t, "elasticache"), twin) + + require.NoError(t, err) + assert.Len(t, api.purchases(), 1) } // Regression guard: a same-pool sibling's own reservation is listed (its purchase diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 7f6198bbd..b0bc530d6 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -45,6 +45,7 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftRecentBuyRefusalLegacy), regexp.QuoteMeta(redshiftSettleRefusal), regexp.QuoteMeta(redshiftDuplicateTupleRefusal), + regexp.QuoteMeta(elastiCacheDuplicatePoolRefusal), regexp.QuoteMeta(gcpDuplicateCheckFailedNote), regexp.QuoteMeta(gcpRecentCommitmentNote), regexp.QuoteMeta(elastiCacheDuplicateCheckFailedNote), diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 44729acf4..448cf59db 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -68,6 +68,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftRecentBuyRefusalLegacy, redshiftSettleRefusal, redshiftDuplicateTupleRefusal, + elastiCacheDuplicatePoolRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, @@ -224,7 +225,7 @@ func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { // The Redshift gate texts are fixed: an appended execution id or raw text must // not pass verbatim. func TestSanitizeStoredError_RedshiftGateRefusalsAreClosed(t *testing.T) { - for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, redshiftDuplicateTupleRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, elastiCacheRecentReservationNote} { + for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftFailedConflictRefusal, redshiftNoJournalRefusal, redshiftRecentBuyRefusal, redshiftSettleRefusal, redshiftDuplicateTupleRefusal, elastiCacheDuplicatePoolRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote, elastiCacheDuplicateCheckFailedNote, elastiCacheRecentReservationNote} { assert.Equal(t, text, SanitizeStoredError(text)) for _, variant := range []string{ text + " (execution 00000000-0000-0000-0000-000000000000, running)", diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index 3ee3f2a2f..98ba512ee 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -435,8 +435,8 @@ func (m *Manager) preExecutionRefusal(ctx context.Context, exec *config.Purchase if refusal := m.ladderExecutionRefusal(ctx, exec); refusal != nil { return beforeSubmission(refusal) } - if hasDuplicateRedshiftTuple(exec) { - return beforeSubmission(errors.New(redshiftDuplicateTupleRefusal)) + if refusal := duplicateSelectionRefusal(exec); refusal != nil { + return beforeSubmission(refusal) } // A Redshift retry reuses the first attempt's request time; only the journal lets go's dedupe tell its node from another's. if hasRedshiftRec(exec) && exec.RetryAttemptN > 0 && m.attemptJournal == nil { @@ -445,6 +445,17 @@ func (m *Manager) preExecutionRefusal(ctx context.Context, exec *config.Purchase return nil } +// duplicateSelectionRefusal refuses a selection two of whose recs would buy the same reservation. +func duplicateSelectionRefusal(exec *config.PurchaseExecution) error { + switch { + case hasDuplicateRedshiftTuple(exec): + return errors.New(redshiftDuplicateTupleRefusal) + case hasDuplicateElastiCachePool(exec): + return errors.New(elastiCacheDuplicatePoolRefusal) + } + return nil +} + // beforeSubmissionError marks a failure that happened before any cloud // purchase call was made for the execution: the code path itself proves no // commitment can exist, independent of the error text or of provider diff --git a/internal/purchase/requested_at_test.go b/internal/purchase/requested_at_test.go index 4de5e5710..1707d9d95 100644 --- a/internal/purchase/requested_at_test.go +++ b/internal/purchase/requested_at_test.go @@ -142,6 +142,7 @@ type redshiftFinalizeRun struct { stampedIDs []string opts []common.PurchaseOptions saves []config.PurchaseExecution + store *MockConfigStore err error } @@ -164,7 +165,7 @@ func runRedshiftFinalizeWithJournal(t *testing.T, exec *config.PurchaseExecution mockFactory := new(MockProviderFactory) mockProviderInst := new(MockProvider) mockServiceClient := new(MockServiceClient) - run := &redshiftFinalizeRun{} + run := &redshiftFinalizeRun{store: mockStore} var mu sync.Mutex mockStore.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { diff --git a/known_issues/42_elasticache_dedupe.md b/known_issues/42_elasticache_dedupe.md new file mode 100644 index 000000000..825f2d882 --- /dev/null +++ b/known_issues/42_elasticache_dedupe.md @@ -0,0 +1,31 @@ +# Known Issues: ElastiCache reserved-node duplicate protection + +> **Audit status (2026-11-09):** `2 needs triage ยท 2 resolved` + +## RESOLVED: a recent same-pool ElastiCache reservation could be bought again (#1863) + +**Files:** `internal/purchase/elasticache_recent_check.go` + +**Description:** Before each AWS ElastiCache purchase the platform lists the live reservations and refuses the rec when a recent reservation of the same region, node type and a compatible engine already covers it (a Redis reservation can also cover Valkey). Reservations created by the same execution are filtered out so a retry adopts its own purchase. This catches duplicates across executions only. + +**Status:** Resolved (#1863). + +## RESOLVED: two same-pool recs in one execution both bought (#1867) + +**Files:** `internal/purchase/elasticache_duplicate_pool.go`, `internal/purchase/manager.go` (`preExecutionRefusal`) + +**Description:** The per-rec checks run concurrently and drop the execution's own reservation ids, so neither saw the other and both bought. `hasDuplicateElastiCachePool` now refuses the whole selection before submission with a fixed, allowlisted text (`elastiCacheDuplicatePoolRefusal`). The key is account, region, node type (raw) and engine (trimmed, normalized); term, payment and count are not in it, so two same-pool recs with different terms are refused too. Only selected AWS ElastiCache recs count. Cross-execution duplicates stay the #1863 check's job. + +**Status:** Resolved (#1867). + +## LOW: Redis and Valkey of the same node type in one execution are both allowed + +**Description:** They are distinct pools, so the same-pool refusal lets both through, although a Redis reservation also covers Valkey. The result can be redundant spend. Refusing it would be an owner decision. + +**Status:** Needs triage. + +## LOW: a refused plan step is regenerated and refused every tick + +**Description:** A plan-step root with a duplicate pair ends failed with `no_submission` and releases its suppressions. If the plan regenerates the same duplicate recs, each tick yields a new refused row. No money moves. The Redshift same-tuple refusal behaves the same way since #1895. + +**Status:** Needs triage.