fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches

[IAmJSD]

What

Finishes #97. #244 (merged, Refs #97) moved Cloud SQL, Memorystore and Compute Engine onto the shared skumatch.InRegion / skumatch.HasToken rules. It deliberately left two pieces open:

1. Cloud Storage region polarity. skuMatchesStorageClass still returned true for a nil ServiceRegions (prices every region) while an empty non-nil slice matched none. It now calls skumatch.InRegion, the same rule as the other services.
2. Last-wins extraction. extractSQLPricingFromSKUs, memorystore extractPricingFromSKUs and extractStoragePricingFromSKUs assigned onDemand = price / commitment = price on every match, so whichever matching SKU came last in the catalog set the price that CommitmentPrice, OnDemandPrice and SavingsPercentage are computed from. They now collect through a new skumatch.SlotPrices. Agreeing duplicates are accepted. Two matching SKUs that feed the same slot at different prices return an ambiguous catalog: ... error, and the caller gets no pricing. This is the rule computeengine's rateSlot.set already applies.

Matching rules after this PR

Input	Before (Cloud Storage)	After (all four services)
ServiceRegions lists the region (case-insensitive)	match	match
ServiceRegions lists other regions only	no match	no match
ServiceRegions nil, no / non-GLOBAL GeoTaxonomy	match every region	no match
ServiceRegions empty [], no / non-GLOBAL GeoTaxonomy	no match	no match
ServiceRegions nil or empty, GeoTaxonomy.Type == "GLOBAL"	match (nil) / no match (empty)	match
Storage class STANDARD vs Standard-HA Storage ..., REGIONAL vs Multi-Regional Storage ...	no match (already anchored on "<class> storage ")	no match (now pinned by tests)

Matched SKUs per slot	Before (SQL / Memorystore / Storage)	After
one	that price	that price
several, same price	last	that price
several, different prices (either order)	last one in catalog order	error ambiguous catalog: <slot> SKU <id> ... but matching SKU <id> prices ...
Commit1Yr and Commit3Yr at different prices	last one	error (see #254 below)

Tier and machine-type tokens were already made whole-token by #244 (HasToken). This PR adds extractor-level tests showing that a db-n1-standard-16 / STANDARD_HA decoy SKU no longer affects the db-n1-standard-1 / STANDARD price in either order.

Behaviour change

• A Cloud Storage SKU with no region list and no GLOBAL taxonomy no longer prices the requested region.
• A catalog with conflicting matches for one slot now gives an error instead of a price that depends on catalog order. Today this includes a 1Yr and a 3Yr commitment SKU at different prices for the same tier. That case is fix(gcp): extractors conflate Commit1Yr and Commit3Yr SKUs into one commitment price #254 (extractors conflate the two terms). The fix for fix(gcp): extractors conflate Commit1Yr and Commit3Yr SKUs into one commitment price #254 filters by the requested term before collection, after which this rule only fires on a genuine conflict. Until then, failing closed replaces a silently order-dependent price.

Verification (local, macOS, fixtures only, no GCP calls)

• Fail-before / pass-after: with the three client.go files reverted to origin/main and the new tests kept, 3 subtests fail in cloudsql, 3 in memorystore and 6 in cloudstorage (nil-region match, ambiguous on-demand in both orders, 1Yr/3Yr conflict, GLOBAL empty slice). All pass with the fix.
• Mutation probes, each run against skumatch + cloudsql + memorystore + cloudstorage:
  • Error on any duplicate, even an agreeing one: 4 packages fail.
  • Never error: 4 packages fail.
  • Route SlotCommitment to SlotNone: 4 packages fail.
  • Storage len(ServiceRegions)==0 || short-circuit: cloudstorage fails.
  • Storage return true: cloudstorage fails.
  • One equivalent mutant (the guard deciding which agreeing SKU id is remembered) survived. That guard was removed as redundant.
• In providers/gcp: go build ./..., go vet ./..., go test ./..., GOWORK=off go test -race -short ./... all pass. golangci-lint v2.10.1 run --allow-parallel-runners ./... reports 0 issues. gocyclo -over 10 on every changed non-test .go file reports nothing. git-secrets (--register-aws, isolated HOME, fresh repo) is clean on all changed files. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.
• Not verified against the live Cloud Billing catalog. As fix(gcp): match SKUs by whole tier token and explicit region #244 notes, real Cloud SQL / Memorystore descriptions do not carry tier names, so those token-matched lookups find no SKUs on real data today. The region and ambiguity rules affect real data mainly through Cloud Storage.

Out of scope / overlap

• fix(gcp): extractors conflate Commit1Yr and Commit3Yr SKUs into one commitment price #254 (Commit1Yr vs Commit3Yr term plumbing): not done here. See Behaviour change.
• fix(gcp/computeengine): c2 pricing fails for legacy 'Commitment: Compute optimized' SKU names #393 / fix(gcp/computeengine): c4d commitment RAM priced in GBy.h never prices #394 (computeengine pricing.go c2 / c4d): untouched. Compute Engine already uses InRegion and fails closed on conflicting rates.
• fix(gcp): price tiered SKUs from the first paid tier #433 edits extract*PriceFromSKU and appends to skumatch.go. It does not overlap: the new type lives in skumatch/slotprices.go.
• fix(gcp/cloudstorage): apply the currency unification to the Cloud Storage extractor #424 replaces currency = "USD" in extractStoragePricingFromSKUs. This PR adds a var prices line directly above it, so whichever lands second gets a trivial one-line textual conflict.

Labels

Please mirror the issue's labels: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/bug, triaged.

Closes #97

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 10 billable files and costs up to $2.50.}

Or wait 59 minutes for your next included review.

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

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8d7d0185-99b6-454a-bf2f-83c5121f7094

📥 Commits

Reviewing files that changed from the base of the PR and between 011be8f and c99bd7f.

📒 Files selected for processing: 10

• providers/gcp/internal/skumatch/skumatch.go
• providers/gcp/internal/skumatch/slotprices.go
• providers/gcp/internal/skumatch/slotprices_test.go
• providers/gcp/services/cloudsql/ambiguity_test.go
• providers/gcp/services/cloudsql/client.go
• providers/gcp/services/cloudstorage/ambiguity_test.go
• providers/gcp/services/cloudstorage/client.go
• providers/gcp/services/cloudstorage/client_test.go
• providers/gcp/services/memorystore/ambiguity_test.go
• providers/gcp/services/memorystore/client.go

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

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

[cristim]

Independent review verdict: CLEAN at c99bd7f

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

Blocking findings: none.

Matching rules:

• Region matching: skumatch.InRegion (skumatch.go:20-30) matches a SKU with named regions only on a case-insensitive region hit. A region-less SKU (nil or empty, treated the same) matches only if GeoTaxonomy.Type == "GLOBAL".
• Ambiguity: slotprices.go:30-38 (setOnce) returns "ambiguous catalog" when a second SKU fills the same slot at a different price. A duplicate at the same price is accepted.
• No regression for multi- or dual-region: multi-region (ServiceRegions ["us"]) and dual-region SKUs still match by name. Only region-less, non-GLOBAL SKUs are dropped.

Service	On ambiguity	Region-less, non-GLOBAL SKU
cloudsql (client.go:445)	error from extractor → getSQLPricing	not matched
memorystore (client.go:413)	same	not matched
cloudstorage (client.go:307)	same	not matched (via InRegion)

Scope of the error:

• The error is per GetOfferingDetails call (cloudstorage client.go:175-176), so it affects only the one recommendation, not the run.
• Compute Engine already fails closed through its own rateSlot.set.
• No other first-match or last-wins loop remains in services.

Caveat (non-blocking, disclosed as #254): the extractors don't filter by term. Where a catalog has both 1yr and 3yr SKUs for one tier, commitment pricing for that tier now errors (fail-closed) instead of taking the last price seen. On live data that could drop commitment pricing for some Cloud Storage tiers until #254 lands.

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

• go vet ./... is clean. go test -race -count=1 ./... passes 836 in 9 packages.
• Fail-before: with the three client.go files reverted, cloudsql fails 3 subtests, memorystore 3 and cloudstorage 6.
• Mutation probes, both killed:
  • Disabling the ambiguity guard fails skumatch, cloudsql, cloudstorage and memorystore.
  • Accepting region-less SKUs fails the same four. computeengine has no region-less test (a coverage note).

Gate:

• Labels are mirrored from fix(gcp): SKU matching treats a nil region list as global and a substring as identity #97.
• Needs the author to mark the draft ready, then green CI, then its turn in the go merge order.

[cristim]

Independent adversarial review: CLEAN at c99bd7fb4a20c71eef6a857872211b900ddebc38

Reviewer: an independent agent that did not write this change (Sonnet, high effort), cold brief, full diff. The PR body was treated as a claim.

Blocking findings: none.

Trade-off for the owner to see before merging (non-blocking): the Cloud SQL and Memorystore match predicate is only skumatch.HasToken(sku.Description, tier) && InRegion (cloudsql/client.go:480, memorystore/client.go:448). Commit1Yr and Commit3Yr both feed SlotCommitment, and no term filter exists yet (#254). SlotPrices.Add now errors when one slot gets two different prices. So any catalog that lists a 1-year and a 3-year CUD SKU for the same tier at different prices returns ambiguous catalog. Loose tier tokens can also match several capacity, engine or availability variants.

• Before: an arbitrary last-wins price, which could be the wrong term.
• After: no pricing for that rec.

Callers handle this safely. fillSQLPricing and fillRedisPricing (cloudsql/client.go:519, memorystore/client.go:487) log and return, leaving OnDemandCost and SavingsPercentage unset. getXPricing also rejects a zero on-demand or commitment price, so there is no zero-price, 100%-savings path. This matches the repo's fail-loud rule and the PR body's stated stance. In practice, though, Cloud SQL and Memorystore commitment pricing may be unavailable until #254 adds term filtering. That could not be confirmed offline against a live catalog.

What was attacked:

• Cloud Storage region change. skuMatchesStorageClass now uses skumatch.InRegion (pkg skumatch.go:~18-27), so nil or empty regions match only when GeoTaxonomy.Type == "GLOBAL". Regional SKUs list their regions. Multi-region SKUs carry codes like us, so they never matched a concrete region anyway. Cloud Storage has no CUD SKUs, so getStoragePricing already errored on commitment pricing before this PR.
• Float equality. Prices come deterministically from units and nanos, so agreeing duplicates are not falsely flagged.
• Errors. The error carries only public catalog SKU IDs, descriptions and prices, and it is only logged.
• SlotPrices. It is local to each call, so there is no shared state. Nil or empty input gives zero prices, which callers reject.
• Current main. git merge-tree is clean. The merged tree passes go test -race -short ./... and go vet in providers/gcp, including after fix(gcp/computeengine): price c2 commitments listed under the legacy 'Commitment:' SKU names #482 (computeengine) and fix(gcp/memorystore): list Redis SKUs from the Cloud Memorystore for Redis billing service #483 (memorystore service ID).

Mutation probes (scratch copy, reverted):

• Last-wins SlotPrices: TestSlotPrices_Add and the cloudsql, memorystore and cloudstorage ambiguity tests fail.
• Nil ServiceRegions matching: TestInRegion, TestSkuMatchesTier, TestSkuMatchesStorageClass and the region tests fail.
• A 1e-9 tolerance in the equality check: the skumatch, cloudsql and memorystore tests fail. Cloud Storage's do not, a minor gap in how tightly they pin equality.

Local verification (macOS, exact SHA c99bd7f)

Fixture evidence; no GCP call:

• Fail-before: with the three client.go files restored to the merge-base, these fail: cloudsql TestExtractPricing_AmbiguousMatches, memorystore TestExtractPricing_AmbiguousMatches, and cloudstorage TestExtractStoragePricing_RegionAndAmbiguity plus TestSkuMatchesStorageClass.
• Pass-after: (cd providers/gcp && GOWORK=off go test -race -short ./...), make build and make vet pass. Fork CI on this head is green (pre-commit 38971209077, Build & Test 38971209234).

Labels mirror #97.

Needed before merge

@IAmJSD:

1. Rebase onto main (the ruleset requires up-to-date branches). It merges cleanly.
2. Mark the PR ready for review.

@cristim: please confirm you accept the trade-off above, which leaves no Cloud SQL or Memorystore commitment pricing until #254 instead of a possibly wrong-term price. Otherwise #254 should land first.

[cristim]

Merge driver status: blocked on an owner decision. The independent review is CLEAN at c99bd7fb, but it raised a trade-off for the owner to see before merging. Cloud SQL and Memorystore pricing fail closed when the catalog lists both 1-year and 3-year commitment SKUs, until the term filter in #254 exists. @cristim, please confirm that fail-closed behavior is acceptable, or ask for #254 first. Once you confirm, I will update the branch with main and merge.