[cristim]

Summary

#251 capped four of the five uncapped Azure pager loops. The fifth was GetExistingCommitments in providers/azure/services/savingsplans/client.go. It looped on pager.More() with no page cap and no ctx.Err() check, so a pager that never ends spun forever, and a cancelled context was only noticed inside the SDK call.

The fix mirrors #251's pattern: a per-package constant and an inline check, with the same error wording.

• maxSavingsPlanPages = 100. That is the same value as fix(azure): cap the remaining Azure pager walks #251's maxSubscriptionPages, and more headroom than its 20 and 50 caps. Savings plans are listed per billing scope, so real volumes are small, and the cap only stops a runaway pager.
• Each page checks ctx.Err() first and returns a wrapped "context canceled during pagination" error.
• Past the cap, it returns "pagination cap (100 pages) reached".
• Every error return yields nil, never partial results.

Tests are in a new file, client_pager_cap_test.go. Open PR #453 owns client_test.go in this package, so the new file avoids touching it. It uses a small multi-page fake, because the existing mockListAllPager only serves one page. Five cases:

• An endless pager errors at the cap after exactly 100 fetches.
• Cancelling mid-walk returns context.Canceled.
• An already-cancelled ctx returns before the first fetch.
• A 3-page listing returns everything.
• Exactly 100 pages succeeds, with 100 fetches.

Closes #247

Verification (macOS, Go 1.26.9)

• (cd providers/azure && GOWORK=off go test -race -short ./...) passes.
• make build and make vet pass.
• golangci-lint v2.10.1 (the CI pin) reports 0 issues.
• gofmt is clean, and gocyclo does not flag GetExistingCommitments.
• Fail-before: the endless-pager and cancel cases fail on the original client.go.
• Mutation probes by the independent reviewer, in a scratch copy: dropping the ctx check, a >= cap comparison, and returning the partial slice on the cap or ctx error each fail the test.
• Review: two passes by a non-author reviewer. Pass 1 added the pre-cancelled-ctx and exact-cap call-count assertions. No actionable findings remained.

Fake pager only; no Azure call.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Azure savings plan listings now stop promptly if a request is canceled and return an error when pagination would exceed 100 pages. Listings that finish within the limit continue to return their results, including listings spanning exactly 100 pages. Cancellation before the first page request also returns an error without fetching a page.

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

• 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 7 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 67 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: f25228da-89e3-4d19-a991-4b764a277667

📥 Commits

Reviewing files that changed from the base of the PR and between 787bee2 and 1425af9.

📒 Files selected for processing: 2

• providers/azure/services/savingsplans/client.go
• providers/azure/services/savingsplans/client_pager_cap_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 5e15286b-9998-4d79-ad76-1cbd955ada93

📥 Commits

Reviewing files that changed from the base of the PR and between 6113015 and 787bee2.

📒 Files selected for processing: 1

• providers/azure/services/savingsplans/client_pager_cap_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The Azure savings-plan inventory pager checks for context cancellation before each page request and returns an error if pagination would exceed 100 pages. Tests cover cancellation, page limits, and results across multiple pages.

Changes: Savings-plan pagination

Layer / File(s)	Summary
Pagination limits and validation providers/azure/services/savingsplans/client.go, providers/azure/services/savingsplans/client_pager_cap_test.go	The pager enforces a 100-page limit and checks for context cancellation before each page request. Tests cover cancellation, pagination up to the limit, and multi-page results.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 787be

No merge-blocking issue is identified for the savings-plan pagination change; normal checks remain appropriate.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #247 requires a cap, a per-iteration ctx.Err() check, and a test for each of five Azure pager loops. This PR implements those requirements for GetExistingCommitments in `providers/azure/serv…	Implement and test the required cap and per-iteration cancellation behavior for the other four pager loops listed in #247, including the documented partial-results warning behavior for the Advisor loop. Alternatively, update the linked issu…
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main changes: adding a cap to the Azure savings-plan inventory pager and honoring context cancellation.
Out of Scope Changes check		The source change targets the savings-plan pager listed in issue #247. The added tests verify the requested cap, cancellation behavior, successful pagination, and nil results on errors. The reviewed c…

Full details: Out of Scope Changes check

Explanation

The source change targets the savings-plan pager listed in issue #247. The added tests verify the requested cap, cancellation behavior, successful pagination, and nil results on errors. The reviewed changes show no unrelated work.

Full details: Linked Issues check

Explanation

Issue #247 requires a cap, a per-iteration ctx.Err() check, and a test for each of five Azure pager loops. This PR implements those requirements for GetExistingCommitments in providers/azure/services/savingsplans/client.go and adds focused tests. The reviewed changes do not address the other four issue locations: managed Redis SKUs, provider locations, account subscriptions, and Advisor recommendations.

Resolution

Implement and test the required cap and per-iteration cancellation behavior for the other four pager loops listed in #247, including the documented partial-results warning behavior for the Advisor loop. Alternatively, update the linked issue scope if this PR intentionally delivers only the savings-plan loop.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

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

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

[coderabbitai]

⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

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]

Independent review verdict: CLEAN at 6113015

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. The guard fails closed: hitting the cap never returns a truncated inventory to the duplicate-purchase guard.

Verified:

• Cap. providers/azure/services/savingsplans/client.go:157-158 returns nil and "failed to list savings plans: pagination cap (%d pages) reached". The wording matches fix(azure): cap the remaining Azure pager walks #251 (33f329d). pkg/recfilter/dedupe.go:48-50 propagates the error.
• Cancellation. ctx.Err() is checked before every page (:154-156), wrapped with %w, and returns nil.
• Boundary. The check is pageIndex > 100, evaluated only when More() is true. So exactly 100 pages succeed and the 101st errors, and both cases are tested.

Notes (non-blocking):

1. dedupe.go:48-50 returns recs, nil, err, so the original recommendations come back alongside the error. A caller that ignored the error would buy unfiltered recommendations. The callers of the wrapper at dedupe.go:291 were not checked here.
2. providers/azure/ri_utilization.go:104 still loops for pager.More() with no cap and no ctx check. It is reporting only, not on the purchase path.

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

• go vet ./... is clean. go test -race -count=1 ./... passes 13 packages with 0 FAIL.
• Fail-before: with main's client.go plus the constant, 3 subtests fail: endless pager, cancel mid-walk, and pre-cancelled ctx.
• Mutation probes, both killed:
  • >= instead of > fails the endless and exactly-cap subtests.
  • Returning the partial slice at the cap fails the endless subtest.

Gate: labels are on the PR (no closing issue). Merge once CI is green, after the go PRs ahead of it in the merge queue.

[cristim]

Follow-up on note 1 (checked by claude-cc-reviewer): the only real-money consumer of AdjustRecommendationsForExisting is cloud-commitments-cli. Its CSV path (cmd/multi_service.go:885-899, checkDuplicatesForCSVRegion) refuses to purchase on a non-dry run when the check errors, and only a dry run continues with un-deduplicated counts. The non-CSV path (cmd/multi_service_helpers.go:808) handles the error separately. So a pager-cap error fails closed end to end. Platform does not call it on main.

[cristim]

CI on 6113015 failed one job: Lint Code (providers/azure) reported misspell cancelled at client_pager_cap_test.go:70, in a test name added during review. Fixed in 170d535. The pinned lint now reports 0 issues on the package and the savingsplans tests pass. The test name changed and nothing else, so no code changed.

[cristim]

CI is green on head 170d535: pre-commit 38950041213 and Build & Test 38950041306.

[cristim]

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

[cristim]

Correction to my previous comment: the head 787bee2 has one commit beyond the reviewed 6113015. The reviewed commit itself is identical (=, now 812db4f). The extra commit, 787bee2 "test(azure): use the US spelling the misspell linter enforces", renames one subtest string (already-cancelled → already-canceled) in client_pager_cap_test.go and changes no code. I reviewed this delta myself: it is test-name-only and has no effect on behaviour. The CLEAN verdict holds at 787bee2. (claude-cc-reviewer)

[cristim]

Pure rebase onto main after go#422 merged. git range-diff of 787bee2 against new head d2cd02f shows both commits identical (=), so the CLEAN verdict carries. (claude-cc-reviewer)

[cristim]

Pure rebase onto main after go#438 merged. git range-diff of d2cd02f against new head 1425af9 shows both commits identical (=), so the CLEAN verdict carries. (claude-cc-reviewer)