[cristim]

Closes #1867

What

Two selected AWS ElastiCache recommendations for the same pool inside one execution both bought. Each rec gets its own index-derived idempotency token, so the library's guard sees two different reservation ids, and the per-rec recent-reservation checks (#1863) run concurrently under the fan-out and drop the execution's own reservation ids from their listing, so neither sees the other's purchase.

The whole selection is now refused before submission (the same shape as the Redshift duplicate-tuple refusal from #1895): hasDuplicateElastiCachePool runs in preExecutionRefusal, the single funnel every executor entry point goes through, and returns a beforeSubmission error with a fixed, allowlisted note. The row ends failed with no_submission stamped.

• Pool = account, region, node type (raw, as the library keys it) and engine (the engine the buy path sends, trimmed then normalized). Term, payment and count are not part of it, as in the go library's pool key.
• Only selected recs count; Azure cache and AWS memorydb never do; a rec with Count <= 0 or no usable engine is skipped (the library refuses those anyway).
• The refusal text does not tell the user to edit the selection, because a plan-step root has none: "remove the duplicate recommendation and start a new purchase, or ask an operator if this is a plan step".

Decisions made with safe defaults (owner can override)

• Refuse the whole selection rather than keep one rec or sum the counts: both change what the user approved.
• Different terms of the same pool are refused too (the issue asks for one rec per node type, region and engine pool per run).
• Redis plus Valkey of the same node type in one execution is still allowed, although Redis covers Valkey one way in the library, so this is redundant spend that is not blocked here. Noted in known_issues/42_elasticache_dedupe.md.
• Plan-step churn: a refusal releases the plan's suppressions, so if the same duplicate pair is regenerated, each tick yields a new refused row. No money moves; Redshift behaves the same after fix(purchase): refuse two identical Redshift recs in one execution #1895.

How it is proven

• Detection table (identical, different count, term and payment, engine whitespace and case, redis vs valkey, other account, other region, unselected twin, memorydb and Azure cache, Count 0, empty engine, legacy elasticache vs canonical cache slug, rec account vs execution account, node type case) and an end-to-end test through executeAndFinalize: zero purchase calls, status failed, no_submission stamped, the stored note equals the fixed text and survives SanitizeStoredError.
• A plan-step root is refused and its suppressions are released; a fan-out root with the same pool for different accounts is not refused; a retry of a refused row is refused again.
• The characterization test that asserted both recs buy is flipped (it now drives executeAndFinalize, because the old path cannot see the refusal), with a control where an unselected twin buys once.
• Six mutation probes, each killed: remove the hook, drop the trim, add term to the key, count unselected recs, count other services, skip the Count <= 0 rule.
• Plan reviewed twice and the implementation reviewed by an independent reviewer; lint gate (golangci-lint on internal/purchase, 0 findings in changed files; gocyclo clean); the whole purchase package passes on the rebased tree.

Notes

Two commits (203 and 80 changed lines). Cross-execution duplicates remain the #1863 recent check's job.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/1867-elasticache-same-pool
• File: .coderabbit.yaml
• Commit: 51105e7
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3ee59a64-f3c7-4d43-a22e-b46bee6fa09a

Commits

Reviewing files that changed from the base of the PR and between a4dd0f3 and 51105e7.

Files selected for processing: 9

• internal/purchase/elasticache_duplicate_pool.go
• internal/purchase/elasticache_duplicate_pool_test.go
• internal/purchase/elasticache_recent_check.go
• internal/purchase/elasticache_recent_check_scenarios_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go
• internal/purchase/manager.go
• internal/purchase/requested_at_test.go
• known_issues/42_elasticache_dedupe.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

CHANGES REQUESTED 8ec320a by claude-nibblets

The production change is sound (details below), but Unit Tests and CI Success are red at this head, and the failure is in this PR's own new test file.

Blocker: TestNoUnfailableMockAssertions (the repo's vacuous-assertion guard, internal/mocks) reports one assertion site it cannot check: internal/purchase/elasticache_duplicate_pool_test.go:92:2 run.store.AssertCalled(t, "..."), "receiver is not a plain identifier (e.g. a field or nested selector). Review by hand." This is the same class of failure as #1881 had (fixed there by taking the mock into a local first): add store := run.store before the assertion and call store.AssertCalled(...) on it. Nothing else fails (Lint Code and Integration Tests (purchase) pass).

Review of the change itself (closes #1867; +266/-10, money path; 0 behind main): two selected AWS ElastiCache recs for the same pool in one execution both bought, because each rec gets its own index-derived token and the per-rec recent-reservation checks run concurrently and drop the execution's own reservation ids. hasDuplicateElastiCachePool (internal/purchase/elasticache_duplicate_pool.go) keys each selected AWS ElastiCache rec by account (its own, else the execution's), region, node type and the engine the buy path sends (details decoded, engine fallback applied, trimmed, normalized), skips a rec with Count <= 0 or no usable engine, and duplicateSelectionRefusal (manager.go) runs it in preExecutionRefusal next to the Redshift check from #1895, as a beforeSubmission error with a fixed note that is on the stored-error allowlist, so the row ends no_submission before any provider call. Term, payment and count are deliberately not in the key, matching the go library's pool key; the PR states the decisions (refuse the whole selection, different terms of one pool refused, Redis plus Valkey still allowed and recorded in known_issues/42).

Scoped probes (go test -run ElastiCache|ErrNote|ServerNote|Sanitiz|PreExecution|RequestedAt|Redshift in internal/purchase, scratch worktree at this head, baseline ok), each killed: duplicate-pool check not called (four tests incl. TestElastiCacheScenarios_SamePoolTwiceInOneExecutionIsRefused); account dropped from the key (TestElastiCacheDuplicatePool_Detection, _FanOutDifferentAccountsNotRefused; my first version did not compile and was redone); engine dropped; node type dropped; unselected recs counted (also TestElastiCacheScenarios_SamePoolTwinUnselectedBuysOnce); zero-count rec counted; engine not trimmed or normalized; allowlist entry dropped (TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim).

I will re-approve at the head that fixes the guard failure once Unit Tests is green.