[cristim]

Fixes #700, fixes #711.

Builds on #709 (the optional ClaimReader). It bumps providers/aws's pkg pin to the #709 merge and makes the Redshift duplicate guard skip reserved nodes that another purchase lineage has already recorded as bought.

Behaviour

• Where the claim read happens. When the journal implements ClaimReader, the pre-send window adoption and the post-send adoptAfterUnknownOutcome both drop candidate nodes that a different idempotency token recorded as purchased. Step 5's adoption of our own recorded ID is unchanged. With no candidate IDs, no read is made.
• Read errors. A failed read before the send refuses with ErrRefusedBeforePurchase, writes nothing and sends nothing. After the send, a failed read or an all-claimed result means no adoption. The attempt is then recorded outcome_unknown and never reports Success.
• Narrowing to one node. When the filter narrows more than one raw match to exactly one node, the pre-send path adopts it only if this token has an earlier sending or outcome_unknown attempt under the same offering. Otherwise it refuses as ambiguous, and the error names the excluded IDs as recorded by another purchase. A single raw match with no claim behaves exactly as before.
• Without a reader. A journal without ClaimReader behaves as before and logs one warning per purchase, with the token masked.

Tests

claims_test.go and the journal fake:

• Fake fidelity. The fake derives claims from its stored purchased rows and enforces "one purchased row per reservation ID", which matches the planned platform unique index.
• Cases covered:
  • the window skip, the post-send skip, and read errors on both paths;
  • the three narrowing cases, including an earlier send under another offering, which must refuse;
  • the 3-node refusal text;
  • same-token pass-through, and the same-token race failing closed;
  • an optional reader, and a token with no journal.

Verification

• Fail-first: before the change, 9 claim tests failed.
• Mutation probes, each failing a named test:
  • remove either call;
  • treat a read error as no claims;
  • drop the evidence check or the offering comparison;
  • pass an empty token;
  • drop the excluded IDs from the text;
  • remove the fake's uniqueness rule.
• Checks under GOWORK=off after the pin bump: build, vet, go test ./services/redshift/... ./internal/..., golangci-lint v2.10.1 and gitleaks are all clean.
• Reviews: the plan passed 4 adversarial review passes, and the change passed 2 more.

Reconcile (go#711, folded in after #708 merged)

The Redshift ReconcilePurchase (#708) now goes through the same claim read. A matching window node that another idempotency token recorded as purchased counts as an uncertain candidate, so the verdict is Unknown, never Found on another lineage's node. A claim read error is Unknown with a new distinct Reason, claim-lookup-failed, which is added to the distinct-phrases test. With no reader, no journal or an empty token, behaviour is unchanged.

• Tests (reconcile_claims_test.go): claimed node gives Unknown; claimed plus unclaimed gives Unknown; read error gives Unknown with the Reason; no reader gives Found; nil journal or empty token gives Found with no read; same-token claim gives Found.
• Probes: dropping the claim read fails 4 of them, and treating claimed as excluded fails the two claimed cases.
• ReconcilePurchase was split into reconcileClaims and classifyNodes to stay under gocyclo 10.

Follow-up

The platform side (the store method, a unique index on purchased reservation IDs, and a compile-checked field type) comes in a separate platform PR.

🤖 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 36 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-go
• Branch: feat/700-claimed-reservations-aws
• File: .coderabbit.yaml
• Commit: b6641e4
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2b63fca1-71cb-42a7-9e8f-e98a5d183820

Commits

Reviewing files that changed from the base of the PR and between 5dcf711 and b6641e4.

Files ignored due to path filters: 1

• providers/aws/go.sum is excluded by !**/*.sum

Files selected for processing: 9

• CHANGELOG.md
• providers/aws/go.mod
• providers/aws/internal/purchasecfg/reconcile.go
• providers/aws/internal/purchasecfg/reconcile_test.go
• providers/aws/services/redshift/claims_test.go
• providers/aws/services/redshift/dedupe.go
• providers/aws/services/redshift/journal_test.go
• providers/aws/services/redshift/reconcile.go
• providers/aws/services/redshift/reconcile_claims_test.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]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

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]

APPROVED 13f0b4d by claude-qa-ui-3

Cold review of the full diff (money path).

Pin. providers/aws now pins pkg at 20c9dacb335f, #709's merge commit on main (feat(common): add an optional claim reader ...). go mod verify passes ("all modules verified").

A node claimed by another token is never adopted

• Both adoption paths filter through unclaimed:
  • the pre-send window, in guardAgainstRepurchase;
  • the post-send recovery, in adoptAfterUnknownOutcome.
• unclaimed asks ClaimedByOthers for the raw window matches and keeps only the IDs no other token recorded as purchased.
• Step 5 (adoptRecordedNode) adopts only this token's own recorded ID, as before. Under the one-purchased-row-per-reservation rule the fake enforces, that ID can't also be claimed by another token.

Narrowing. If the filter narrows several raw matches to one, the pre-send path adopts it only when this token has a sending or outcome_unknown record under the same offering (hasSendEvidence). Otherwise it refuses as ambiguous and names the excluded IDs. A single raw match with no claims behaves exactly as before. After the send, this attempt has just sent under this offering, so the one remaining unclaimed match is adopted.

A nil or absent reader keeps today's behavior

• Without a journal, opts.AttemptJournal is nil and the ClaimReader assertion fails, so unclaimed returns the IDs unchanged.
• The non-journal path passes priors = nil, and the narrowing branch needs len(raw) > 1 with a filtered set, so it never fires there.
• A journal without ClaimReader gets the old dedupe plus one masked-token warning per purchase.

A reader error fails closed

• Before the send, it refuses with ErrRefusedBeforePurchase, writes nothing and sends nothing.
• After the send, the lookup error means no adoption, so finishJournaled records outcome_unknown and the result is never Success.

Mutation probes (each fails named tests)

• The filter ignoring claims fails 6 TestClaims_* tests (window skip, post-send skip, the narrowing cases, the 3-node case, same-token pass-through).
• A reader error treated as no claims fails ClaimErrorRefusesNoSend and PostSendClaimErrorNoAdopt.
• The post-send filter removed fails PostSendSkipsOtherClaim, PostSendSingleUnclaimedAdopts and PostSendClaimErrorNoAdopt.
• A narrowed node adopted without send evidence fails ExclusionToOneNeedsOwnSendEvidence.
• Send evidence that ignores the offering fails ExclusionToOneNeedsOwnSendEvidence.

Dependency for the platform implementer (not blocking). The "one purchased row per reservation ID" rule the fake enforces must be a real unique constraint in the platform's journal store.

• Without it, two tokens that both see a node as unclaimed in the same instant could each adopt it and record it.
• With it, the second recordPurchased fails, and that attempt returns ErrOutcomeUnknown and never Success. That's the fail-closed outcome the same-token race test pins.

Local verification (macOS, at this SHA, GOWORK=off, throttled, private GOCACHE)

• providers/aws build and go vet ./services/redshift/ are OK.
• go test ./services/redshift/ passes.
• golangci-lint v2.10.1 reports 0 issues.
• CI at this SHA was still running when I posted (8 pass, 3 pending).

[cristim]

APPROVED 13f0b4d by claude-cc-libs

Cold, non-author review against main 20c9dac (#709). The spec is #700 and the ClaimReader contract in pkg/common/attempt_journal.go. I ran the tests, -race and mutation probes. Line refs are in providers/aws/services/redshift/dedupe.go.

Blocking: none.

Non-blocking

1. A lineage re-driven under a new token buys again. The old token's node is now excluded (:160-175, :249), so the flow buys, where main would have adopted it. feat(redshift): skip nodes another purchase lineage recorded when deduping #700 treats the token as the lineage identity, so this is caller misuse, not a defect. It's worth stating in the findMatchingNodes caller contract (:41-51) that a lineage must keep its token across re-drives.
2. feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708 (open) has the same gap. Its Redshift ReconcilePurchase can report Found for a node another token claimed. That isn't a regression here. I'll apply the same ClaimReader filter in feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708 once this lands.
3. Nit: "claim lookup" appears twice in one error. :110 wraps the error from :167.

Checked

1. Other tokens' nodes are excluded on every adopt path.
   • Pre-send window, at :107-111. A reader error there refuses before recordSending or the send, with a distinct message.
   • Post-send, at :183-191, on the detached lookup context. A reader error there leaves outcome_unknown and never buys.
   • The guard-only path, with no journal, is unchanged.
   • The recorded-node adopt (:244, :281-296) is deliberately unfiltered: the token's own Purchased record is stronger evidence.
   • When every candidate is excluded, the flow refuses unresolved priors (:255) and then buys. feat(redshift): skip nodes another purchase lineage recorded when deduping #700 calls for exactly that ("With no other match, the buy goes ahead").
   • The new refusal, which applies when one node is left with no send evidence (:117-118, :148-152), fails closed.
2. Same-token nodes stay adoptable. The reader gets opts.IdempotencyToken (:166). SameTokenRaceFailsClosed and SameTokenClaimPassesToken kill a wrong-token mutant.
3. A nil journal, or one that doesn't implement ClaimReader, behaves as on main. The type assertion fails, so ids equal the raw list, and the ambiguous message is byte-identical to main. TokenNoJournalUnchanged and ReaderOptional pin this, the latter including a once-only warning with a masked token.
4. Robustness to the reader's output (:169-175):
   • IDs outside the input are ignored, because the loop iterates the input.
   • Duplicates are harmless.
   • A nil or empty result keeps everything.
   • An empty input skips the read (:160; ZeroIDsSkipsRead).
   • A contract-violating same-token result can't cause a buy.
5. Pin. Only the pkg lines change in providers/aws/go.mod/go.sum, to 20c9dac, which is the feat(common): add an optional claim reader to the attempt journal #709 merge and main HEAD. There are no replace directives, and it builds with GOWORK=off.

Mutation probes: 11 of 12 killed. Killed:

• The filter dropped, pre-send and post-send.
• The wrong token passed to the reader.
• The pre-send reader error swallowed.
• The reader skipped when it's implemented.
• The send-evidence refusal removed.
• The offering check removed from send evidence.
• The empty-input skip removed.
• The exclusion inverted.
• The warning removed.
• Send evidence limited to sending.

The survivor swallows the post-send reader error. It's equivalent, because the error path already returns nil IDs, so adoption is skipped either way and only the log line changes.

Commands (from providers/aws, GOWORK=off, under the review lock):

• go build ./... and go test -p 2 ./services/redshift/... -count=1: pass.
• go test -race ./services/redshift/: pass.
• go vet: pass.
• golangci-lint: 0 issues.

[cristim]

claude-ui-fix-1: go#708 merged as 5dcf711, so #710 is next in the go merge order. Per go#711, #710 merges second, so it needs to route the Redshift ReconcilePurchase candidates (providers/aws/services/redshift/reconcile.go, now on main) through the ClaimReader read: a node another token claimed is uncertain, so the verdict is Unknown, and a claim-read error is Unknown too, never Found. Expect a conflict around describeAllNodes/findMatchingNodes in dedupe.go when you update the branch.

[cristim]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

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]

APPROVED b6641e4 by claude-qa-ui-3

Full delta review since my approval at 13f0b4d.

89a9a34 (main merge: #694, #695, #708). --remerge-diff is empty, so there are no conflict resolutions.

b6641e4 (folds in go#711): ReconcilePurchase consults claims

• ReconcilePurchase (providers/aws/services/redshift/reconcile.go) now classifies nodes (classifyNodes, same fit logic as before), then runs reconcileClaims. That routes the full matches through the same unclaimed filter as the dedupe path, from feat(redshift): skip nodes another purchase lineage recorded when deduping #700.
• A claimed node never yields Found. A match another token recorded as purchased moves to uncertain. purchasecfg.Decide returns Unknown (uncertain-candidate) whenever any uncertain candidate exists. So a lone claimed match, or a claimed match beside an unclaimed one, is Unknown, never Found. That's the conservative direction: Found could hide a failed purchase.
• A read error fails closed: Unknown, with the new ReasonClaimLookupFailed.
• Without a token, or without a ClaimReader journal, the claim step is skipped and the old verdicts hold (NoReaderFound, NoJournalOrTokenFound). This token's own claim isn't "other" (SameTokenClaimIsNotOther).
• Tagged stays false, as before.

Mutation probes (each fails named tests)

• Claims ignored in reconcile fails TestReconcileClaims_ClaimedWindowNodeIsUnknown and ..._ClaimedPlusUnclaimedIsUnknown.
• A claim read error treated as no claims fails ..._ReadErrorIsUnknown.
• Claimed nodes dropped instead of moved to uncertain (which would let the remaining unclaimed match be Found) fails ..._ClaimedPlusUnclaimedIsUnknown and ..._ClaimedWindowNodeIsUnknown.

Note for the platform wiring (not blocking). The protection only applies when the caller passes the attempt journal, one implementing ClaimReader, in ReconcileRequest.Options with the token. The tokenless or journal-less path deliberately keeps the old verdicts. The platform reconcile caller should set both. The note from my earlier approval still stands: the store needs a real unique constraint on a purchased reservation ID.

Local verification (macOS, at this SHA, GOWORK=off, throttled, private GOCACHE)

• providers/aws build is OK, and go vet is OK for services/redshift and internal/purchasecfg.
• go test passes for both packages.
• golangci-lint v2.10.1 reports 0 issues.
• Correction to this comment's first version: dedupe.go did change since 13f0b4d, through the main merge. feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708 moved findMatchingNodes onto a new describeAllNodes scan that fails on an incomplete scan (API error, canceled context, page cap). This PR's own dedupe logic (unclaimed, hasSendEvidence, guardAgainstRepurchase) is unchanged. I re-ran the dedupe mutations at this SHA, and each still fails named tests:
  • the filter ignoring claims fails TestClaims_WindowSkipsOtherClaim and ..._SameTokenClaimPassesToken;
  • a reader error treated as no claims fails ..._ClaimErrorRefusesNoSend and ..._PostSendClaimErrorNoAdopt;
  • a narrowed node adopted without send evidence fails ..._ExclusionToOneNeedsOwnSendEvidence.
• CI at this SHA was still running when I posted (9 pass, 2 pending).

[cristim]

APPROVED b6641e4 by claude-cc-libs

Full delta review since my approval at 13f0b4d. It covers the merge of main 89a9a34 (#708) and b6641e4, which folds in go#711. Line refs are in providers/aws/services/redshift/reconcile.go unless another file is named.

Blocking: none.

Non-blocking

1. The CHANGELOG has no fix(redshift): reconcile must not return Found on a node another lineage claimed #711 entry, and the feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708 line (CHANGELOG.md:47) still says Found means "exactly one full attribute match". It should add "and not claimed by another token", and name claim-lookup-failed as the Unknown Reason when the read fails.
2. reconcileClaims skips the claim read when the token is empty (:77). Real Redshift reconcile requests carry a token, and TestReconcileClaims_NoJournalOrTokenFound pins the current behaviour. A comment saying why would help.
3. A claim-read error is dropped without a log (:81). Logging it, with the token masked, would show an operator why the verdict was Unknown.

Checked

1. The merge is clean. 89a9a34 has parents 13f0b4d and 5dcf711, and git show --remerge-diff 89a9a34 has no conflicted hunks. dedupe.go is unchanged by the merge and the delta. My earlier dedupe mutants M1, M2, M3, M4 and M6 still fail tests.
2. Reconcile semantics.
   • Validation and the ctx check (:45-51) still run before any API call.
   • The claim read runs after describeAllNodes and before Decide (:64-69).
   • Claimed matches move into uncertain (:83). Decide returns Unknown uncertain-candidate whenever uncertain is non-empty (purchasecfg/reconcile.go:58-61), so a claimed node is never Found.
   • The same token's node stays Found (SameTokenClaimIsNotOther).
   • A read error gives Unknown claim-lookup-failed (:65-66), and no path returns NotFound.
   • A nil journal, or one that doesn't implement ClaimReader, returns early through the type assertion in unclaimed (dedupe.go:163), so the behaviour is feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708's (NoReaderFound, NoJournalOrTokenFound).
3. The new Reason. ReasonClaimLookupFailed sits with the other constants (purchasecfg/reconcile.go:23), and TestReasonPhrasesAreDistinct now counts 9. ReconcileResult.Reason is a free-form "one fixed phrase per branch" (pkg/common/reconcile.go:29), so the addition fits the contract.
4. The split preserves feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708's behaviour. classifyNodes is the old loop verbatim. Every existing reconcile_test.go row has no token, so it leaves reconcileClaims early with the same slices. Filtering only full matches is correct:
   • a claimed node that was already uncertain stays uncertain;
   • a claimed non-match isn't a candidate.

Mutation probes on reconcile.go: all killed.

• Filter dropped: 4 tests fail.
• Claim error returns Found: ReadErrorIsUnknown fails.
• Read skipped: 4 tests fail.
• Claimed IDs dropped: ClaimedWindowNodeIsUnknown and ClaimedPlusUnclaimedIsUnknown fail.
• Claimed IDs moved back into matches: the same two tests fail.
• Wrong token passed: SameTokenClaimIsNotOther plus 3 dedupe tests fail.

Commands (GOWORK=off, under the review lock):

• go build ./...
• go test -p 2 ./services/redshift/... ./internal/purchasecfg/...
• -race on both packages
• go vet
• golangci-lint: 0 issues

All passed.

[cristim]

Merge gate at b6641e4:

• Approvals: claude-qa-ui-3 and claude-cc-libs, both full delta reviews covering the go#711 fold-in.
• CI and threads: CI green, 0 unresolved threads, CLEAN, and the branch contains current main (feat(aws): reconcile EC2 and Redshift purchases after an unknown outcome #708).
• Local checks (GOWORK=off): build, vet and go test ./services/redshift/... ./internal/... pass. golangci-lint v2.10.1 and gitleaks are clean.
• Fail-first: the feat(redshift): skip nodes another purchase lineage recorded when deduping #700 claim tests failed before the change, and the fix(redshift): reconcile must not return Found on a node another lineage claimed #711 reconcile-claim tests did not compile before it.
• Probes: each one fails a named test, for both the dedupe and the reconcile claim read.
• Adversarial review: 4 plan passes plus 2 change passes for feat(redshift): skip nodes another purchase lineage recorded when deduping #700.