Skip to content

fix(azure/savingsplans)!: refuse a malformed idempotency token before the purchase PUT - #495

Merged
cristim merged 3 commits into
mainfrom
fix/104-azure-sp-order-id-from-token
Oct 17, 2026
Merged

cristim merged 3 commits into
mainfrom
fix/104-azure-sp-order-id-from-token

Conversation

@cristim

@cristim cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Summary

Part 2 of 2 for #104 item A09-020 (part 1: #487, which added common.OrderIDFromToken and ErrInvalidIdempotencyToken).

Before this change, a non-empty idempotency token that could not be formatted as a GUID made the Azure Savings Plan purchase silently fall back to a fresh cudly-<ns> alias, so a re-drive of that purchase was no longer idempotent and could buy a second plan.

  1. build(azure): bump the providers/azure pkg pin 4566498f2a08 -> 7383c3a7db7e (the feat(common): add OrderIDFromToken, which fails loud on a malformed idempotency token #487 merge). Everything else the bump pulls in is additive (Commitment EC2 dimensions, recfilter EC2 dedupe, insurance line-item decode and paging, ladder docs); no azure changes were needed for it.
  2. fix(azure/savingsplans)!: PurchaseCommitment derives the alias name with OrderIDFromToken right after the commitment currency is resolved, before the purchase body and alias client are built, and returns a failed result wrapping ErrInvalidIdempotencyToken (token masked) before any PUT. common.ReservationOrderID and its test are deleted.
  3. test(azure/savingsplans): tighter test and corrected doc comment from review.

BREAKING CHANGE: common.ReservationOrderID is removed; use common.OrderIDFromToken. No caller remains in this repo, and the CLI, MCP and platform pin older pkg versions and call neither symbol.

Behavior: an empty token (the CLI path) still gets the timestamp fallback alias. Real producers (DeriveIdempotencyToken, 64-char hex) are unaffected.

Refs #104 (A06-006 refers to internal/server code that is not in this repo, so this PR does not close the issue). GCP's equivalent gap is #488.

Tests

TestPurchaseCommitment_MalformedTokenFailsBeforePUT (abc and 32 non-hex chars):

  • with a mock alias client: errors.Is(err, common.ErrInvalidIdempotencyToken), zero BeginCreate calls, raw token absent from the error;
  • with no injected alias client: still returns the token error, not "failed to create order alias client".

The existing deterministic-name and empty-token fallback tests pass.

Fail-before: ReservationOrderID no longer exists, so main's behavior was reproduced as a mutant (swallow the error and fall back to a fresh alias). It fails the test on the error assertion (client_test.go:395), not a panic.

Verification

make build, make vet, (cd providers/azure && GOWORK=off go build ./... && GOWORK=off go test -race -short ./services/savingsplans/...), (cd pkg && GOWORK=off go test -race -short ./common/...), pinned golangci-lint v2.10.1 on pkg and providers/azure (0 issues each), go mod tidy -diff clean, gofmt, and the commit/pre-push hooks pass. Unit-level evidence only.

Local review: two adversarial passes by a non-author reviewer (the same one that reviewed the plan and #487). Pass 1: stale doc, an overclaiming ordering comment, and a mutant that failed by panic (fixed in the third commit); pass 2: nothing actionable. No textual overlap with #491 (it edits client.go around lines 141-200; this PR around 348-390).

🤖 Generated with Claude Code

@cristim cristim added impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/xs Trivial / one-liner type/bug Defect triaged Item has been triaged labels Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 47 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 9cf86639-678a-4bd7-b852-49c66140748a
📥 Commits

Reviewing files that changed from the base of the PR and between 2bf23fb and 3ae04d7.

⛔ Files ignored due to path filters: 1
  • providers/azure/go.sum is excluded by !**/*.sum
📒 Files selected for processing: 5
  • pkg/common/tokens.go
  • pkg/common/tokens_test.go
  • providers/azure/go.mod
  • providers/azure/services/savingsplans/client.go
  • providers/azure/services/savingsplans/client_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ 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

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at a8e2e1c

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

Blocking findings: none.

Purchase path (providers/azure/services/savingsplans/client.go):

  • :357 derives the alias with common.OrderIDFromToken(token, "cudly-<ns>").
  • :358-361 returns before the body is built, before the alias client is created, and before BeginCreate (:390, the only PUT). So a malformed token causes zero HTTP calls.
  • The test asserts mock.calls == 0 and ErrorIs(ErrInvalidIdempotencyToken), and that the raw token is absent from the error.

Empty token: pkg/common/tokens.go:123-124 returns the fallback, so the CLI path keeps the timestamp alias. That is not idempotent: an empty-token retry can create a second order. This behaviour is unchanged and scoped to the CLI, as the body says.

Breaking change (!):

  • common.ReservationOrderID and its test are removed.
  • Its only caller was savingsplans/client.go, and nothing else in this repo references it (GCP uses IdempotencyGUID).
  • I grepped origin/main of cloud-commitments-cli, -platform and -mcp: 0 references, so a future pin bump breaks no consumer.

Pin and other purchase paths:

What was attacked and how (GOWORK=off, no Azure calls):

  • go vet ./... is clean in pkg and providers/azure. go test -race -count=1 ./... passes: 15 pkg packages and 13 azure packages.
  • Mutation probe: replacing the error return with a random-alias fallback fails TestPurchaseCommitment_MalformedTokenFailsBeforePUT (client_test.go:395). A "PUT before validating" mutant would trip the same test's calls == 0 assertion.

Gate: p3, no closing issue. Merge once CI is green.

cristim and others added 3 commits October 17, 2026 03:24
Move providers/azure from pkg 4566498 to 7383c3a so the
savingsplans purchase path can call common.OrderIDFromToken (#487).

Notable pkg changes pulled in (all additive):
- feat(common): OrderIDFromToken and ErrInvalidIdempotencyToken (#487)
- feat(common): EC2 platform, tenancy and scope dimensions on Commitment
- fix(recfilter): match EC2 commitments on those dimensions when deduplicating
- feat(insurance): decode plan line-item identity and read complete pages
- docs(ladder): document provider commitment precision at PurchaseLayer

Refs #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the purchase PUT

PurchaseCommitment silently turned a non-empty but malformed idempotency
token into a timestamp alias name, so a re-drive would PUT a second alias
and buy a second Savings Plan. Derive the alias name with
common.OrderIDFromToken right after the currency is resolved, before the
body and the alias client are built, and return the error without any PUT.
An empty token still falls back to the timestamp name (CLI path).

Remove the now caller-less common.ReservationOrderID and its test.

BREAKING CHANGE: common.ReservationOrderID was removed in favor of
common.OrderIDFromToken, which returns an error wrapping
common.ErrInvalidIdempotencyToken for a malformed non-empty token.

Refs #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e PurchaseCommitment doc

Name OrderIDFromToken in the PurchaseCommitment doc and document the
malformed-token error. Reword the comments to claim only what the test
proves, and give the mock a poller so the fallback mutant fails on an
assertion instead of a nil-pointer panic.

Refs #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim force-pushed the fix/104-azure-sp-order-id-from-token branch from a8e2e1c to 3ae04d7 Compare October 17, 2026 01:24
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Pure rebase onto main after go#490 merged. git range-diff of a8e2e1c against new head 3ae04d7 shows identical commits (=), so the CLEAN verdict carries. (claude-cc-reviewer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant