[cristim]

Summary

• preSubmitChecks runs the ElastiCache recent-reservation check (S1) for AWS cache recommendations, before the GCP check and before the dispatch time is stamped. A recent same-pool reservation, or a listing that cannot rule one out, refuses the rec before submission, so the row ends as a before-submission refusal.
• The per-execution own-name sets become one pointer struct with nil-safe accessors, built once and passed through the fan-out, so a sibling rec's reservation in the same execution and a lost-response retry are adopted, not refused. The client and the rec take their region from rec.Region.

Verification

• Real-path tests over the fake ElastiCache API: a recent same-pool reservation refuses with zero purchase calls; an own listed reservation, a sibling's reservation and a retry next to a foreign reservation all reach purchase; only AWS ElastiCache lists reservations; a nil own-names pointer does not panic.
• Mutating the check call, the nil guards, the own-ID builder and the own-name wiring each fails a test. -race, internal/mocks, the purchase unit tests and golangci-lint pass locally on macOS; CI covers Linux and the integration suites.
• Four existing tests that run AWS cache recs through the real executor now expect the listing; one is strict.
• Scenario table and the integration run follow in S3.

Part of #1863

🤖 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 26 seconds.

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: feat/1863-elasticache-s2
• File: .coderabbit.yaml
• Commit: 6019153
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: ba11e140-2d72-47c4-af7e-cd148a6ea8b3

Commits

Reviewing files that changed from the base of the PR and between 55bdce2 and 6019153.

Files selected for processing: 5

• internal/purchase/azure_redis_routing_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/elasticache_recent_check.go
• internal/purchase/elasticache_recent_check_wiring_test.go
• internal/purchase/execution.go

• 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]

APPROVE @ 6019153 by claude-nibblets

Cold review of #1893 (Part of #1863, S2: wires the ElastiCache recent-reservation check from #1890 into the executor; +207/-7, money path; 0 behind main). I looked for ways it could refuse a legitimate buy, miss a duplicate, or break a retry.

Read: preSubmitChecks (internal/purchase/execution.go) now runs elastiCacheRecentRefusal first, before the GCP check and before withDispatchTime, so a refusal is a before-submission error and the dispatch time is not stamped for a refused rec. The service client is already built from rec.Region (execution.go:1287), so the listing covers the rec's own region. The two per-execution own-name sets are now one purchaseOwnNames built once in processPurchaseRecommendations from the selected indices and passed through the fan-out, with nil-safe accessors; ownElastiCacheID(opts) is the reservation ID this rec's purchase would create (empty when no token). So a sibling rec's reservation in the same execution and a lost-response retry (the rec's own ID already listed) are adopted rather than refused, and a retry next to a foreign reservation still reaches the library guard. The check adds a DescribeReservedCacheNodes call to every AWS ElastiCache buy and fails closed if it cannot list; I confirmed the IAM grants exist for it in the cross-account and target stacks (cloudformation/stacks/CUDly-CrossAccount/template.yaml:136, iac/federation/aws-cross-account and aws-target in both CFN and Terraform, and the CLI templates), so a correctly onboarded account does not start refusing.

Scoped probes (go test -run ElastiCache|PreSubmit|OwnNames|Wiring|GCP|RecentCommitment|AzureRedis|CoverageExtra in internal/purchase, scratch worktree at this head, baseline ok), each killed: the check not called (TestElastiCacheWiring_RecentSamePoolReservationRefusesBeforePurchase, TestPreSubmitChecks_ElastiCacheRefusesBeforeDispatchTimeIsStamped); own names not wired, and the builder dropping the ElastiCache set (TestElastiCacheWiring_SiblingOwnReservationDoesNotRefuse); nil guard removed (TestPreSubmitChecks_NilOwnNamesDoesNotPanic, which also panics as expected); own ID never built (TestElastiCacheWiring_RetryWithForeignReservationIsAdopted).

Non-blocking: any ElastiCache buy now needs a successful listing, so a transient throttle or an account whose role lacks the grant refuses the rec with the fixed "could not complete" note instead of buying; that is the intended fail-closed behaviour, and the note tells the operator to retry later or check the account.

CI at this head: all 42 checks pass (Unit Tests, Lint Code, Integration Tests (api, purchase), CI Success and pre-commit); 1 skipping (CodeRabbit).