Skip to content

fix(gcp/computeengine): fail loud when a non-empty idempotency token cannot form a request ID - #496

Merged
cristim merged 2 commits into
mainfrom
fix/488-gcp-short-token-request-id
Oct 17, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/488-gcp-short-token-request-id

Conversation

@cristim

@cristim cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Summary

GCP twin of #104 A09-020. buildInsertRequest (providers/gcp/services/computeengine/client.go) set the commitment insert RequestId from common.IdempotencyGUID(token), which returns "" for a non-empty token that cannot form a GUID. A short all-hex token such as abc passed idempotentCommitmentName and then silently lost request-level idempotency.

  1. build(gcp): bump the providers/gcp pkg pin 498b74f31f01 -> 2bf23fbd8c2a (origin/main) to pick up common.OrderIDFromToken (feat(common): add OrderIDFromToken, which fails loud on a malformed idempotency token #487). The other pulled pkg commits need no gcp changes (build and tests pass).
  2. fix(gcp/computeengine): RequestId now comes from common.OrderIDFromToken(token, ""). An empty token still sends no RequestId (CLI path unchanged); a non-empty token that cannot form a GUID returns derive GCP request ID: ... wrapping common.ErrInvalidIdempotencyToken (token masked) before any Insert, and the purchase result reports the error with Success false. The package's unexported errInvalidIdempotencyToken is replaced by the shared sentinel (only this file and its test used it; cli, mcp and platform do not reference it).

Closes #488

Tests

TestPurchaseCommitment_UnformableRequestIDTokenFailsLoud (abc, 31 hex chars): errors.Is(err, common.ErrInvalidIdempotencyToken) and no Insert request recorded. abc passes idempotentCommitmentName (cud-abc), so this case exercises the new RequestId check. Existing 64-hex and empty-token tests pass.

  • Fail-before: with origin/main's client.go, both subtests fail ("An error is expected but got nil").
  • Mutation probe: swallowing the error (requestID = "") fails both subtests (independently reproduced by the reviewer).

Verification

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

Local review: one adversarial pass by a non-author reviewer (the one that reviewed #487 and #495), no actionable findings. No overlap with #307 (it edits client.go around lines 465 and 1171-1190; this PR around 870 and 1445).

🤖 Generated with Claude Code

cristim and others added 2 commits October 17, 2026 03:32
Moves pkg from 498b74f to 2bf23fb (12 pkg commits: OrderIDFromToken, EC2 dimensions, insurance line items, exchange, recfilter, ladder docs).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cannot form a request ID

A short all-hex token such as abc passed the commitment-name check but IdempotencyGUID returned empty, silently dropping the insert RequestId. Use common.OrderIDFromToken so the purchase fails before Insert, and replace the package-local sentinel with common.ErrInvalidIdempotencyToken. The error masks the token.

Closes #488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@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 3 billable files and costs up to $0.75.

  • 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 37 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: da9cfc98-1828-4277-bc40-dc54470f222b
📥 Commits

Reviewing files that changed from the base of the PR and between e735fdb and eed99d3.

⛔ Files ignored due to path filters: 1
  • providers/gcp/go.sum is excluded by !**/*.sum
📒 Files selected for processing: 3
  • providers/gcp/go.mod
  • providers/gcp/services/computeengine/client.go
  • providers/gcp/services/computeengine/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 eed99d3

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, focused on the money path. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Purchase path (providers/gcp/services/computeengine/client.go):

  • Malformed token: :733 calls buildInsertRequest, and on error it returns at ~:735-737 (result.Error set) before any Insert. :873-877 uses common.OrderIDFromToken(token, ""). A non-empty malformed token returns derive GCP request ID: %w, wrapping ErrInvalidIdempotencyToken with the token masked.
  • Empty token: returns "", so no RequestId is set. This is the non-idempotent CLI path, the same as Azure fix(azure/savingsplans)!: refuse a malformed idempotency token before the purchase PUT #495.
  • Valid token: RequestId is the derived GUID, a pure function of the token, so retries reuse the same ID.
  • Test: asserts ErrorIs, Success == false and zero insert requests for "abc" and a 31-character token.

Request ID format: IdempotencyGUID (pkg/common/tokens.go:98-106) lowercases the first 32 hex characters and formats them as a hyphenated 8-4-4-4-12 UUID, which GCP accepts. Not run against real GCP.

Pin: providers/gcp pins pkg 2bf23fbd8c2a, which contains #487 (7383c3a).

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

  • go vet ./... is clean. go test -race -count=1 ./... passes 821 in 9 packages.
  • Fail-before: with main's client.go, the new test fails ("An error is expected but got nil").
  • Mutation probe: replacing the error return with an empty requestId fails the same test.

Gate: labels are mirrored from #488. Merge once CI is green. (claude-cc-reviewer)

@cristim
cristim merged commit 8a85908 into main Oct 17, 2026
15 checks passed
@cristim
cristim deleted the fix/488-gcp-short-token-request-id branch October 17, 2026 01:47
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.

fix(gcp/computeengine): a short non-empty idempotency token silently drops the insert RequestId

1 participant