[cristim]

Fixes #699.

Problem

managedredis.GetExistingCommitments built held commitments from the Consumption ReservationDetail usage feed. That feed has no purchase date or quantity, so every Redis commitment had a zero StartDate and a Count of 0. recfilter dropped them as old, and even a kept one added nothing. A Redis reservation bought in the last day therefore never reduced the matching recommendation.

Fix

Redis now reads held reservations from the Reservations API, through the same shared path compute uses:

• It calls internal/reservations ListAppliedReservations and StrictCommitment, with ReservedResourceTypeRedisCache.
• StartDate comes from the purchase date and Count from the quantity. Region and SKU come from the reservation itself.
• Canceled, expired, failed and past-expiry reservations are excluded, and so are Shared reservations billed to another subscription.
• An incomplete live row fails the lookup instead of being skipped, the same as compute. That covers a missing SKU, location, quantity or purchase date, an unattributable scope, and an expiry falling today. It's in the CHANGELOG.

The old seam (ReservationsDetailsPager and SetReservationsPager) is replaced by SetInventoryFactories, with a nil fallback to the default factories for both constructors. The CHANGELOG lists that as a source break; nothing outside managedredis used it.

Tests

• Round trip (TestHeldReservation_RoundTripReducesRecommendation):

  1. A raw Consumption recommendation (Legacy and Modern payloads) goes through convertRecommendation.
  2. PurchaseCommitment for it is captured.
  3. The held reservation's SKU and location are built from the captured body.
  4. The recommendation and the reservation go through recfilter.NewDuplicateChecker(24).

  The held reservation reduces the recommendation. The test also asserts that the purchase's location equals the recommendation's region.

• Table (TestHeldReservation_Table):

  • These reduce the recommendation: Shared billed to this subscription, Single scope, Creating, PendingBilling, purchased yesterday.
  • These don't: another subscription, Canceled, Expired, Succeeded past expiry, another region, another SKU, purchased three days ago.
  • These must fail with an error naming the field: missing quantity, expiry today.

• Variant pin: one held reservation reduces only the first term/payment variant (P1Y upfront 1, P1Y monthly 2, P3Y 2 and 2). The test pins today's behaviour and names the known over-buy, fix(recfilter): one held commitment reduces only the first term/payment variant #703.

• Default inventory: TestNewClient_DefaultInventoryFactories, for both constructors.

Verification

• Fail-before: at main, the new test file doesn't compile, because the seam doesn't exist. Feeding the old converter's output through the checker fails the round-trip, table and variant tests.
• Mutation probes, each failing named tests:
  • wrong reservation type;
  • no default-factories fallback;
  • StrictCommitment error ignored;
  • old converter restored;
  • the error-wrapper prefix dropped.
• Checks: with GOWORK=off, build, vet and go test ./services/... in providers/azure pass. golangci-lint at the CI version reports 0 issues, and gitleaks finds no leaks.
• Review: a separate agent reviewed the plan adversarially in 3 passes and the commits once (APPROVED). Its two Low findings are fixed in b95124e.

Not verified (no live Azure account)

• Whether the Reservations API returns sku.name and location in exactly the case the Consumption API uses. There is no normalisation, matching compute.
• Whether reservation quantity and recommendation count use the same units for sharded Premium caches.
• Held coverage older than about a day is assumed to be netted out of Azure's own recommendations, the same semantics as AWS.

Follow-ups

• fix(recfilter): one held commitment reduces only the first term/payment variant #703: variants share one held budget.
• fix(azure): Redis purchases buy in the client region, not the recommendation's region #704: Redis purchases are made in the client's region, not the recommendation's region.
• fix(azure): Cosmos DB, SQL, Search and Synapse reservations never dedupe recommendations #705: Cosmos DB, SQL, Search and Synapse have the same defect.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[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 53 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: fix/699-redis-held-dedupe
• File: .coderabbit.yaml
• Commit: b95124e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c98e29cc-5549-40ff-877e-7bdfa317b6e9

Commits

Reviewing files that changed from the base of the PR and between c83aef8 and b95124e.

Files selected for processing: 6

• CHANGELOG.md
• providers/azure/services/managedredis/client.go
• providers/azure/services/managedredis/client_test.go
• providers/azure/services/managedredis/held_reservation_dedupe_test.go
• providers/azure/services/managedredis/pagination_cap_test.go
• providers/azure/services/managedredis/slot_wiring_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

[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 b95124e by claude-qa-ui-4

• providers/azure/services/managedredis/client.go:212-232: GetExistingCommitments now reads the Reservations API through the shared reservations.ListAppliedReservations + StrictCommitment(..., ReservedResourceTypeRedisCache, ...), as compute does (compute/client.go:291-314), instead of the Consumption ReservationDetail shape. The field mapping (services/internal/reservations/inventory.go:232-318, strict.go) is unchanged by this PR.

Commitment field	armreservations source	Nil handling
StartDate	Properties.PurchaseDate (inventory.go:274); StartDateIsDay=true (:277)	nil on a live row is an error
Count	Properties.Quantity (:271)	nil or <=0 on a live row is an error
ResourceType	SKU.Name (:268)	blank is an error
Region	Location (:265)	blank is an error
State	ProvisioningState plus ExpiryDate (:288)
Type filter	ReservedResourceType = RedisCache	nil is an error (strict.go:29)

PurchaseDate is date-only, so StartDateIsDay keeps the reservation counted until 24h after the purchase day (pkg/recfilter/dedupe.go:134-136), which errs toward suppression.

• Dedupe: different SKUs or regions are not suppressed. Canceled, Expired, Succeeded past expiry, Failed, BillingFailed, Split and Merged are excluded. A Shared reservation billed to another subscription is skipped (strict.go:61-65). A partial match (held 2, recommended 5) leaves 3, with the money fields scaled.
• With go#694: duplicate checking runs per client (dedupe.go:94-99); the cache client's converter is still the Consumption one with no StartDate, so it suppresses nothing and the two inventories are never summed.

Runs (GOWORK=off, GOMAXPROCS=4, -p 2): build and vet pass; go test passes for managedredis, internal/reservations, compute and cache; golangci-lint reports 0 issues. 16 probes, all killed by managedredis tests:

• StartDate dropped or shifted 2 days;
• StartDateIsDay false;
• Count dropped, constant 1 or doubled;
• SKU lowercased;
• wrong region;
• Cancelled or Expired mapped to active;
• the past-expiry check removed;
• PendingBilling mapped to canceled;
• the type filter changed to VirtualMachines;
• a StrictCommitment error skipped with continue;
• the list error swallowed;
• nil factories.

Non-blocking:

• held_reservation_dedupe_test.go:26-31 computes fixtureToday at package init, so a run that crosses midnight UTC can fail the "expires today" and "purchased yesterday" cases.
• managedredis/client.go:30: the maxReservationsPages comment still describes reservation-detail pagination; the cap is now per reservation order (inventory.go:105-107).
• Already filed: fix(azure): Redis purchases buy in the client region, not the recommendation's region #704 (purchases use the client region, so a rec in another region is never matched), fix(recfilter): one held commitment reduces only the first term/payment variant #703 (no term in the dedupe key), fix(azure): Cosmos DB, SQL, Search and Synapse reservations never dedupe recommendations #705 (cache, cosmosdb and database still use the Consumption shape).

[cristim]

APPROVED b95124e by claude-cc-libs

Cold, non-author review of the change against main (c83aef8), including tests and mutation probes. The PR moves managedredis inventory to the Reservations API path compute uses, which is the source #699 recommended. It does not patch the ReservationDetail usage row.

Blocking: none.

Non-blocking

1. New ways for Redis dedupe to fail (services/managedredis/client.go:212-232). An incomplete live reservation fails the inventory read (strict.go:108-127), and so does a date-only expiryDate of today (strict.go:176-183). The identity also needs read access on Microsoft.Capacity reservations, which ReservationDetail didn't. On failure, recfilter.AdjustRecommendationsForExisting (pkg/recfilter/dedupe.go:57-59) returns an error, so Redis dedupe stops for that run instead of silently passing. This is deliberate, matches compute and is in the CHANGELOG. Consumers should expect a lookup that used to succeed to error now.
2. Term and payment aren't part of the dedupe key (dedupe.go:146), and the held budget is used up by the first matching variant (:299-305). This predates the PR, TestHeldReservation_VariantPin pins it, and fix(recfilter): one held commitment reduces only the first term/payment variant #703 tracks it.
3. Region and SKU compare as exact strings with no case normalization (dedupe.go:206, :292). Compute has the same exposure.
4. Five services still read ReservationDetail: database, cosmosdb, search, synapse and cache. fix(azure): Cosmos DB, SQL, Search and Synapse reservations never dedupe recommendations #705 tracks them.
5. Possible midnight flake. held_reservation_dedupe_test.go:27 computes fixtureToday once at package init, while StrictCommitment reads time.Now, so a run that crosses UTC midnight could flake the "expires today" row.

Checked

• Fields against armreservations v1.1.0 (models.go): PurchaseDate (:591), Quantity (:594), ExpiryDate (:577), ProvisioningState (:588), ReservedResourceType (:608), Location (:1182), SKU (:1188). RedisCache is defined at constants.go:510.
  • inventory.go:269-273 sets StartDate from PurchaseDate (StartDateIsDay). That's the fixed purchase date, not EffectiveDateTime and not a moving usage date.
  • Count is the reserved Quantity (inventory.go:266-267).
  • Order IDs are deduped (inventory.go:198-209), so there are no per-day duplicate rows.
• Region now comes from r.Location (inventory.go:260-262), not the client region. That resolves the fix(azure): commitments fabricate State "active" and Region from fields the SDK omits #74 fabrication concern for Redis.
• Over-suppression:
  • The table test confirms these are not suppressed: a different SKU or region, Canceled, Expired, Succeeded past its expiry, a reservation purchased three days ago (outside the 24h lookback), and a Shared reservation billed to another subscription.
  • Creating, PendingBilling and a reservation purchased yesterday are suppressed.
• Composition:
  • git merge-tree against fix(azure): make managedredis the only Azure Redis recommendation producer #702 (d8e1a11) and against fix(azure/cache): refuse Redis purchases so only managedredis buys them #694 (824e845) is conflict-free in both cases.
  • fix(azure): make managedredis the only Azure Redis recommendation producer #702 changes only the recommendation pricer in managedredis/client.go, not the inventory path.
  • The Service label is ServiceMemoryDB on both the recommendation and the commitment (client.go:221).
• Tests: the round-trip test uses the real convertRecommendation, appendPage, the purchase request body, StrictCommitment and recfilter.NewDuplicateChecker(24). Main's shape (StartDate and Count zeroed) fails it.

Mutation probes: 9 of 9 killed. They covered dropping StartDate, dropping StartDateIsDay, dropping Count, setting Count=1, taking StartDate from EffectiveDateTime or from ExpiryDate, taking Region from the client region, main's shape, and a ServiceCache label.

Commands

• From providers/azure: go build ./... and go test -p 2 ./services/managedredis/ . -count=1: pass.
• From pkg: go test ./recfilter/: pass.
• golangci-lint on managedredis: 0 issues.

[cristim]

Merge gate at b95124e:

• Approvals at this SHA: claude-cc-libs and claude-qa-ui-4.
• CI and state: CI green, 0 unresolved threads, merge state CLEAN, and the branch contains current main.
• Local verification (GOWORK=off):
  • build, vet and go test ./services/... in providers/azure pass; lint and gitleaks are clean;
  • the new tests don't compile on main, and the old converter fails the round-trip, table and variant tests;
  • five mutation probes each fail a named test.
• Unverified without a live Azure account: SKU and location case, and Premium shard units. Both are stated in the body.