[cristim]

Part of #107. This follows #696, which added the AttemptJournal contract.

The Redshift reserved-node purchase uses PurchaseOptions.AttemptJournal when the caller sets one along with an idempotency token. With no journal or no token, behaviour is exactly as before.

What

• chore(aws): bumps providers/aws's pkg pin to the feat(common): add a purchase attempt journal contract #696 merge.

• Before any AWS call:

  • require RequestedAt;
  • require an ExecutionID;
  • read PriorAttempts;
  • adopt a recorded reservation ID only if it is among the owned matching nodes (more than one distinct recorded ID refuses);
  • otherwise use the existing window guard, recording a purchased row when it adopts;
  • refuse when this execution already has an unresolved attempt;
  • refuse when another execution's unresolved attempt was sent under a different or empty offering ID, since the window filters by offering;
  • then the existing upfront check;
  • then write sending. If that write fails, the purchase is refused and nothing is sent.

  Every refusal before the send wraps ErrRefusedBeforePurchase, except the unresolved-attempt refusals, which wrap ErrOutcomeUnknown.

• After the single purchase call, all writes use context.WithoutCancel plus the existing 30s budget:

  • a returned node is recorded purchased;
  • an error or an empty response goes through the existing classification and post-error adoption. Otherwise it is recorded outcome_unknown, and the classified error is returned (a 4xx passes through unchanged).
  • If a purchased write fails, the result is never Success. CommitmentID keeps the node ID, ExistingCommitment is false, the error text starts with the node ID, and it wraps ErrOutcomeUnknown.

• The offering ID is written once per attempt. Tokens in errors and logs are masked.

• The comments in client.go and dedupe.go describe the journal.

Tests (journal_test.go)

• Coverage: one test per refusal, adoption and write branch, against a fake journal that enforces the transition table and the write-once offering ID. Both rules have their own tests.
• Mocks: no-send tests assert AssertNotCalled, because the shared mocks are optional.
• Unchanged path: the existing suite covers the no-journal path.

Verification

• Fail-first: at the commit before the flow (f37c718), 20 journal tests fail.
• Mutation probes: each of 22 probes failed the named test, including:
  • send without the sending write;
  • Success after a failed purchased write;
  • dropping the offering-change or same-execution refusal;
  • using the caller's context after the send;
  • adopting a recorded ID that is not among the matches;
  • a lax or overwriting fake journal.
• Checks: with GOWORK=off, build, vet and go test ./services/redshift/... ./internal/... pass. golangci-lint (the CI version) reports 0 issues, and gitleaks finds no leaks.
• Review: a separate agent reviewed the change adversarially: APPROVED at 2649b40, and APPROVED again at 5f629e6 after its three Low findings were fixed.
• Not verified: no live AWS account was used. The flow is exercised with SDK mocks.

Follow-up: #700 (skip nodes another lineage recorded).

🤖 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 30 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/107-redshift-attempt-journal
• File: .coderabbit.yaml
• Commit: 727fea7
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 31b44e63-5f6a-4d31-b6fe-64ddf498265c

Commits

Reviewing files that changed from the base of the PR and between 8d06fde and 727fea7.

Files ignored due to path filters: 1

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

Files selected for processing: 4

• providers/aws/go.mod
• providers/aws/services/redshift/client.go
• providers/aws/services/redshift/dedupe.go
• providers/aws/services/redshift/journal_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 5f629e6 by claude-cc-libs

Cold, non-author review against the closed go-107 plan and the #696 contract (pkg/common/attempt_journal.go at 6b6d481), including tests, race, mutation probes and lint.

Blocking: none.

Read this first: the settle period is the caller's job. go reads no clock and records carry no timestamp (attempt_journal.go:20-29), so it can't judge a settle window. When another execution has a Sending or OutcomeUnknown record for the same offering and no matching node is visible in the window, the retry buys (redshift/dedupe.go:229-246, test journal_test.go:304-319). The only thing that stops a double buy while the first node isn't visible yet is the platform waiting the settle period before it re-dispatches a possibly-sent token under a new execution. That matches the plan (R2-1, L10) and is documented at attempt_journal.go:68-70 and dedupe.go:41-48. The platform side needs its own check when it lands.

Non-blocking

1. Another token's records are trusted. dedupe.go:175-190 doesn't filter priors by IdempotencyToken. A faulty journal that returns another token's Purchased record whose node is in the window gets adopted. That under-buys rather than double-buys, and the node must still match the offering, count and window. Suggest skipping a prior whose token differs.
2. A recorded buy is refused for good after an offering change. adoptRecordedNode (dedupe.go:213-218) matches on the current offering ID. That's safe (no buy) but needs a manual step. The plan accepted it (pass 24).
3. Concurrent dispatches. Two dispatches under different execution IDs both record Sending and both send. The journal is documented as a record, not a lock (attempt_journal.go:57-70). Under the same execution ID, the store rejects Sending to Sending (journal_test.go:124-137).

Checked

• Window. since = RequestedAt - redshiftClockSkew (5 minutes, dedupe.go:27, :50), inclusive (:87) with no upper bound. It's unchanged by this PR, and dedupe_test.go:81 and :110 pin the edge.
• Offering change after an unrecorded send.
  • The window filters on the current offering (dedupe.go:81), and refuseUnresolvedPriors refuses with ErrOutcomeUnknown and no send when the prior's offering is empty or different (:239-242).
  • The same execution refuses whatever the offering (:235-238), and a recorded buy is refused after an offering change (:217).
  • Tests: journal_test.go:291-302 and :321-336.
• Nil journal. journalActive requires both a token and a journal (dedupe.go:143-145). A nil journal falls through to main's guardAgainstRepurchase (:153-154), recordSending does nothing (:252-254), and the post-send path is main's (client.go:175-199). The existing dedupe suite pins it, and journal_test.go:520-529 covers an empty token with a journal set.
• Recording the reservation ID. It's recorded on a fresh buy (dedupe.go:306-311), on adopt-after-unknown (:313-314) and on the pre-send window adopt (:184-187). A retry adopts the recorded ID only if it still matches (:180-182, :208-223). A failed write after a buy returns Success=false with the ID named and ErrOutcomeUnknown wrapped (:280-289).
• Failure paths.
  • A PriorAttempts error refuses (:175-178), and so does a Sending write error (:259-262).
  • Post-send writes use WithoutCancel with a 30s timeout (:268-275).
  • A zero RequestedAt refuses before any journal call (:167-170), and two distinct recorded IDs refuse (:209-211).
• Pin. Only pkg moves, to 6b6d481 (providers/aws/go.mod:6), the feat(common): add a purchase attempt journal contract #696 merge on main. There are no replace directives.

Mutation probes: 23 run, 23 killed. They included skipping the possibly-sent refusal, the offering-change branch, the own-execution branch, the window skew, the zero RequestedAt check, every recording site, the Sending write error, the post-send ctx, the distinct-ID check and the ExecutionID requirement. Each was reverted.

Commands (from providers/aws, under the review lock): go build ./..., go test -p 2 ./services/redshift/... -count=1, go test -race ./services/redshift/, go vet and golangci-lint all pass, with 0 lint issues.

[cristim]

APPROVED 5f629e6 by claude-qa-ui-3

Cold review of the full diff (money path), checked against the closed go#107 plan. Its authoritative flow is in Revision 13, as amended by R14 to R16.

Pin. providers/aws now pins pkg at 6b6d481e8298, which is #696's merge commit on main.

Flow vs. the R13 table (dedupe.go: journalPreflight, refuseUnresolvedPriors, recordSending, finishJournaled; client.go: PurchaseCommitment, readyToSend)

• Journal active iff there is a token and a journal (journalActive). Otherwise guardBeforePurchase falls through to today's guardAgainstRepurchase or no guard. recordSending is a no-op and the post-send branch is today's code. A nil journal keeps today's behavior.
• Steps 2-4. A zero RequestedAt gives errRequestedAtRequired before any journal call (R14 placement). An empty ExecutionID gives ErrRefusedBeforePurchase. A PriorAttempts error gives ErrRefusedBeforePurchase.
• Step 5. More than one distinct Purchased ID refuses. A single recorded ID is adopted only when findMatchingNodes lists it. A lookup error or a missing ID refuses. Adoption writes nothing (R1-5).
• Step 6. A window adoption writes Purchased. A failed write gives Success=false, keeps CommitmentID, clears ExistingCommitment, names the node ID in the error, and wraps ErrOutcomeUnknown (R1-3).
• Steps 7/7b. This execution's own sending/outcome_unknown row refuses with ErrOutcomeUnknown. Another execution's unresolved row refuses when its OfferingID differs from the current offering. An empty OfferingID also differs, so 7b fails closed (R16). An offering change after an unrecorded send therefore can't buy a second node.
• Steps 8-9. The approved-upfront check runs, then the Sending write on the caller's ctx. A failed write refuses before the send: no record, no buy.
• Step 11.
  • A returned node writes Purchased.
  • An error or empty response tries adoptAfterUnknownOutcome, then writes Purchased or outcome_unknown.
  • Post-send writes use WithoutCancel plus recoverLookupTimeout.
  • A failed outcome_unknown write is appended to the original error.

Attacks

• Journal write failures: covered before and after the send, and on both adoption paths.
• Concurrent attempts: two executions with the same token and the same offering can both proceed. That relies on the caller's claim and settle wait, which the findMatchingNodes doc now states generically, per the plan's public-wording rule.
• Clock skew: past redshiftClockSkew, a recorded node falls outside the window and refuses (step 5). An unresolved own attempt refuses (step 7). The only open case is another execution under the same offering, which is the documented settle contract.
• Malformed prior: a Purchased prior with an empty ID can never be among the matches, so it refuses.

Mutation probes (each fails named tests)

• Skipping steps 7/7b fails OfferingChangedUnresolvedPriorRefuses and SameExecUnresolvedPriorRefusesNoSend.
• 7b failing open on an empty prior offering fails OfferingChangedUnresolvedPriorRefuses.
• Step 7 never firing fails SameExecUnresolvedPriorRefusesNoSend.
• Buying when the Sending write fails fails SendingWriteErrorRefusesNoSend.
• A failed Purchased write reporting success fails PurchasedWriteErrorIsOutcomeUnknownWithID, AdoptWriteErrorNotSuccess and PostSendAdoptWriteErrorNotSuccess.
• Adopting a recorded ID that isn't among the matches fails RecordedIDNotMatchedRefuses.
• Post-send writes on the caller's ctx fail CancelledCtxAfterSendStillRecords.
• The journal never active fails 5 TestJournal_* tests.
• outcome_unknown never written fails UnknownOutcomeRecorded, UnknownWriteErrorStillOutcomeUnknown and AmbiguousResponseRecordedUnknown.
• Conflicting recorded IDs not refused fails ConflictingRecordedIDsRefuses.

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

• go mod verify, build and go vet ./services/redshift/ are OK.
• go test passes for ./services/redshift/ and ./internal/....
• golangci-lint v2.10.1 reports 0 issues on ./services/redshift/....

[cristim]

APPROVED ced337a by claude-cc-libs

Merge-only delta since my approval at 5f629e6.

• ced337a is a merge commit with parents 5f629e6 (the head I approved) and 409a1f3 (main, go#698).
• git show --remerge-diff ced337a is empty, so the merge adds no resolution of its own.
• git diff 5f629e68 ced337a touches only providers/aws/ladder/commitment_rates.go, commitment_rates_test.go and commitment_rates_sdk_test.go, which are go#698's files. None is under providers/aws/services/redshift or touches the journal path, so my earlier review stands.
• CI at ced337a: 14 checks SUCCESS, 1 NEUTRAL.

[cristim]

APPROVED ced337a by claude-qa-ui-3

Merge-only delta since my approval at 5f629e6.

Checked

• ced337a has parents 5f629e6 (my approved head) and 409a1f3 (main, feat(ladder): give Compute SP its Cost Explorer commitment rate #698). git show --remerge-diff is empty, so there are no conflict resolutions.
• git diff 5f629e68 ced337a5 is byte-identical to main's own change since the merge base: three files under providers/aws/ladder/ (commitment_rates*.go). None of them overlaps services/redshift or the pkg pin.
• CI at ced337a: 14 checks pass and 1 is skipped.

My evidence at 5f629e6 carries over unchanged: the tests, vet, lint and 11 mutation probes, all killed.

[cristim]

Merge gate at ced337a:

• Approvals at this SHA: claude-cc-libs and claude-qa-ui-3. These are merge-only deltas over their approvals at 5f629e6.
• CI green, 0 unresolved threads, merge state CLEAN.
• The branch contains current main (409a1f3, feat(ladder): give Compute SP its Cost Explorer commitment rate #698). git show --remerge-diff ced337a is empty, and the incoming files don't overlap the Redshift change.
• Local verification at 5f629e6:
  • with GOWORK=off, build, vet and go test ./services/redshift/... ./internal/... pass;
  • golangci-lint and gitleaks are clean;
  • 20 journal tests failed before the change;
  • each of 22 deletion probes, plus 3 for the review fixes, fails a named test.
• Adversarial local review: two passes, both APPROVED.
• No live AWS account was used.

[cristim]

APPROVED 727fea7 by claude-cc-libs

Merge-only delta since my approval at ced337a.

• 727fea7 is a merge commit with parents ced337a (the head I approved) and 8d06fde (main, go#689).
• git show --remerge-diff 727fea70 is empty, so the merge adds no resolution of its own.
• git diff ced337a5 727fea70 touches only CHANGELOG.md, pkg/common/types.go and pkg/common/types_test.go, which are go#689's files. providers/aws still pins pkg at 6b6d481, and nothing under providers/aws/services/redshift changes, so my earlier review stands.
• CI at 727fea7: 14 checks SUCCESS, 1 NEUTRAL.

[cristim]

APPROVED 727fea7 by claude-qa-ui-3

Merge-only delta since my approval at ced337a.

Checked

• 727fea7 has parents ced337a (my approved head) and 8d06fde (main, feat(common): add Commitment.CostKnown #689). git show --remerge-diff is empty, so there are no conflict resolutions.
• git diff ced337a5 727fea70 is byte-identical to main's own change since the merge base. That change is feat(common): add Commitment.CostKnown #689's Commitment.CostKnown: pkg/common/types.go, pkg/common/types_test.go and the CHANGELOG. None of it overlaps services/redshift.
• providers/aws still pins pkg at 6b6d481e8298, so the Redshift code builds against the same pkg as before.
• CI at 727fea7: 14 checks pass and 1 is skipped.

My evidence at 5f629e6 carries over unchanged: the tests, vet, lint and 11 mutation probes, all killed.

[cristim]

Merge gate at 727fea7: claude-cc-libs and claude-qa-ui-3 posted merge-only deltas at this SHA. CI is green, 0 unresolved threads, CLEAN. The head merges main 8d06fde (#689: CHANGELOG and pkg/common only) into ced337a; remerge-diff is empty. Local verification is as recorded at ced337a.