refactor(aws): share the purchase idempotency guard, starting with ElastiCache

[cristim]

Summary

PR A of 2 for #10. The AWS RI purchase idempotency guard (look up an existing reservation by token before purchasing, and adopt it when the purchase returns AlreadyExists) is copied verbatim into four service clients. This adds one shared implementation and migrates ElastiCache; PR B migrates MemoryDB, RDS and OpenSearch.

New providers/aws/internal/purchaseguard:

• type Finder func(ctx) (id string, found bool, err error)
• Guard(ctx, token, label, find): empty token skips the lookup; a lookup error fails loud ("refusing to purchase to avoid a possible double-buy", wrapped with %w); a found reservation is logged with the token masked and returned for adoption.
• Recover[F error](ctx, token, label, purchaseErr, find): matches the service's AlreadyExists fault with errors.As on the pointer type F (including %w-wrapped faults), then looks the reservation up; a lookup error or a miss returns not-recovered, so the original purchase error stands (unchanged behavior).

ElastiCache keeps its two methods as 5-line wrappers, so call sites, call order and all existing elasticache tests are unchanged.

One accepted wording change: the guard error now reads "before ElastiCache reservation purchase" (was "before ElastiCache purchase"); only "refusing to purchase" is asserted, and the old phrase appears nowhere in this repo's tests or in the CLI, MCP or platform repos. Redshift's guard is structurally different and stays separate; the same sentence also exists in Azure reservations, EC2 and Redshift, out of scope here.

Refs #10

Tests

guard_test.go asserts exact log and error text (golden strings copied from the old copies), the masked token and the absence of the raw token, the empty-token paths, lookup-error wrapping, found and not-found, and Recover's matching, adoption, swallowed lookup error and miss. Recover-with-lookup-error and the exact text were not covered by any existing test.

Mutation probes (scratch copies), each caught: wrong fault type in the elasticache wrapper (TestClient_PurchaseCommitment_Idempotent_AlreadyExistsRecovers), dropping either empty-token return, swallowing the lookup error, negating found, dropping the errors.As check, adopting on lookup error or miss, removing MaskToken, and removing the Recover call from the wrapper. The reviewer re-ran the wrong-type and adopt-on-miss probes independently.

Verification

make build, make vet, (cd providers/aws && GOWORK=off go test -race -short ./internal/purchaseguard/... ./services/elasticache/...), pinned golangci-lint v2.10.1 (0 issues), gofmt, and the commit/pre-push hooks (including make test-unit) pass. Unit-level evidence only; no AWS call.

Local review: the plan had two adversarial passes by a non-author reviewer before code; the diff had one more by the same reviewer (golden strings byte-identical apart from the accepted label, errors.As semantics, call order), no actionable findings.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Reliability
  • Purchase requests with the same identifier can reuse an existing reservation instead of creating a duplicate.
  • If a purchase reports that a reservation already exists, the existing reservation can be recovered.
  • Lookup failures are reported to help prevent potentially duplicate purchases.

[cristim]

@coderabbitai review

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 020e6f77-dfe9-4670-9d9f-937de0330495

📥 Commits

Reviewing files that changed from the base of the PR and between 0a87b9b and f396755.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: a678546b-66fd-4a99-9da6-6c2faa1be6ce

📥 Commits

Reviewing files that changed from the base of the PR and between 8316c61 and bafecc6.

📒 Files selected for processing: 3

• providers/aws/internal/purchaseguard/guard.go
• providers/aws/internal/purchaseguard/guard_test.go
• providers/aws/services/elasticache/client.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The pull request adds shared helpers to check purchase reservations and recover from matching duplicate-purchase errors. The ElastiCache client now delegates its reservation checks and recovery to these helpers.

Changes: Purchase idempotency

Layer / File(s)	Summary
Shared guard and recovery helpers providers/aws/internal/purchaseguard/*	Guard skips lookup for an empty token, returns lookup errors, and reports existing reservations. Recover looks up a reservation after a matching purchase error. Tests cover these cases, lookup failures, and log messages.
ElastiCache integration providers/aws/services/elasticache/client.go	The client delegates reservation checks and duplicate-purchase recovery to the shared helpers, using findReservationByID for lookups.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to bafec

This refactor consolidates purchase idempotency checks without a visible behavior change beyond a log wording update. No actionable merge-blocking risk was found.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the shared AWS purchase idempotency guard and its initial ElastiCache migration.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

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

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent review verdict: CLEAN at bafecc6

Reviewer: fresh Claude agent (Sonnet 5.5, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none. Neither guard path can double-buy, and neither fails open.

Behaviour is equivalent. I compared the old inline code (origin/main elasticache/client.go:238-273) with purchaseguard/guard.go:20-56 plus the wrappers, case by case:

• Empty token: both skip the lookup.
• Lookup error: both fail loud, wrapped with %w. Only the wording changed.
• Found or not found: identical.
• AlreadyExists: matched both directly and %w-wrapped.
• Other error types: not recovered.
• AlreadyExists, then a lookup error or a miss: the original error stands.
• Found with an empty ID: identical.
• Cancelled context: the guard refuses, and recovery reports not recovered.

Generics:

• ReservedCacheNodeAlreadyExistsFault has pointer receivers (SDK types/errors.go:1358-1373), and the wrapper instantiates the pointer type.
• A value-type instantiation does not compile.
• A wrong pointer type is caught by TestClient_PurchaseCommitment_Idempotent_AlreadyExistsRecovers.

Call sites:

• The guard runs at client.go:158, before the purchase call at :178.
• Recovery runs at :180, in the error branch only.
• Token masking: MaskToken (pkg/common/tokens.go:78) cannot panic and never logs the full token. A test asserts that the raw token is absent from the log.

Mutation probes (each reverted):

• All of these are caught:
  • fail-open on a lookup error;
  • ignoring a found reservation;
  • recovering on any error;
  • recovering on a miss;
  • dropping the empty-token shortcut in Guard;
  • dropping the empty-token shortcut in Recover;
  • the wrong fault type.
• A value type as F does not compile.
• "Recover on any error" and the Recover empty-token shortcut are caught only by the new unit tests. The ElastiCache suite alone does not catch them.

Local verification: in providers/aws, go build ./..., go vet ./... and go test -race ./... -count=1 pass. Local golangci-lint was blocked by another session's lock, so lint relies on the CI Lint Code job.

Non-blocking notes:

• The wording change is safe. "before ElastiCache purchase" appears nowhere in the cli, mcp or platform repos.
• Recover drops the lookup error silently, the same as before. A debug log would help.
• Generics are justified: there are four copies today, and PR B is the second caller.

Gate:

• Labels match ARCH-03: AWS RI purchase skeleton (idempotency guard etc.) copy-pasted across 5-6 service clients #10.
• The branch is BEHIND. I am rebasing it with update-branch and will carry this verdict only if the range-diff is =.
• CI must be green at the final head.

[cristim]

Pure rebase onto main with gh pr update-branch --rebase. git range-diff shows both commits as = (167bcf0→459a6dc, bafecc6→572dd63). The CLEAN verdict above carries to 572dd63, and CI is now running at this head. (claude-cc-reviewer)

[cristim]

Second pure rebase after #473 merged. git range-diff shows both commits as = (459a6dc→7609dd7, 572dd63→ee2d508), and main gained no changes under elasticache/ or internal/. The CLEAN verdict carries to ee2d508. (claude-cc-reviewer)

[cristim]

Third pure rebase, after #430 merged. git range-diff shows both commits as =, and main gained nothing under elasticache/ or providers/aws/internal. The CLEAN verdict carries to 48b4183. (claude-cc-reviewer)

[cristim]

Fourth pure rebase, after #431 merged. git range-diff shows both commits =, and main gained nothing under elasticache/ or providers/aws/internal. The CLEAN verdict carries to 4ca5692. (claude-cc-reviewer)

[cristim]

Fifth pure rebase (claude-go-pr-merge-3, which took over this claim from claude-cc-reviewer). I ran gh pr update-branch --rebase onto main 8afb655, after #430, #431 and #432 merged.

git range-diff <base>..4ca56926 origin/main..0a87b9b8:

1:  a62dbf7 = 1:  0a79c97 refactor(aws): add a shared purchase idempotency guard
2:  4ca5692 = 2:  0a87b9b refactor(aws/elasticache): use the shared purchase idempotency guard

Both commits are unchanged, so the CLEAN verdict carries to 0a87b9b8334a13885a6d3aecca35ba34928a1731.

Local verification (macOS; fixtures and mocks, no AWS call, no purchase):

• At 4ca56926: in providers/aws, GOWORK=off go test -race -short ./... exits 0.
• At 0a87b9b8: in providers/aws, go build ./..., go vet ./... and GOWORK=off go test -race -short ./... all exit 0.

[cristim]

Merge evidence at f3967554a4c3fd04956a6390c4d869ecf8b3923e (claude-go-pr-merge-3).

Pure update. gh pr update-branch --rebase onto main 364131a, after #433 merged. #433 touches only gcp. git range-diff 8afb655..0a87b9b8 origin/main..f3967554:

1:  0a79c97 = 1:  f4f2799 refactor(aws): add a shared purchase idempotency guard
2:  0a87b9b = 2:  f396755 refactor(aws/elasticache): use the shared purchase idempotency guard

The independent CLEAN verdict (at bafecc6b, carried through rebases with = range-diffs) applies to this head.

CI green on f3967554. Build & Test run 39060423760 and pre-commit run 39060423788.

Local verification at f3967554 (macOS; fixtures and mocks, no AWS call, no purchase): in providers/aws, go build ./..., go vet ./... and GOWORK=off go test -race -short ./... all exit 0.

Gate items.

• No open blocking findings.
• Labels match ARCH-03: AWS RI purchase skeleton (idempotency guard etc.) copy-pasted across 5-6 service clients #10, which this PR references.
• Head unchanged, mergeability CLEAN.
• CodeRabbit skipped its own review ("no reviewable changes"). The repo gate is covered by the independent review, exact-revision local verification and green CI.