Repository navigation
refactor(aws): page Cost Explorer through one capped helper and cap the unbounded loops - #462
Merged
Merged
Conversation
Contributor
Member
Author
|
@coderabbitai review |
Contributor
|
Member
Author
Independent review verdict: CLEAN at d074459Reviewer: 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. Each call site keeps its behaviour:
Helper and cap:
What was attacked and how (providers/aws,
Gate: p3 with its own labels, no closing issue. Merge once CI is green, in its turn in the go merge order. |
This was referenced Oct 16, 2026
Member
Author
|
CI is green on head |
cristim
force-pushed
the
refactor/35-ce-pagination-helper
branch
from
October 16, 2026 20:44
d074459 to
51855eb
Compare
Member
Author
…he unbounded loops Add paginateCE, a shared NextPageToken loop that checks ctx.Err() (returned raw) on each iteration, enforces a page cap, and ends on a nil or empty token. Hitting the cap is an error, never partial results reported as complete. Migrate the three loops that had no cap: fetchCoveragePaged (100 pages), GetRIUtilization (100 pages) and fetchDailyCoverage (maxRecommendationPages, 20, and now with a per-page ctx check). The 100-page caps are runaway guards, not size limits: no Cost Explorer input sets MaxResults, so CE picks the page size. The already-capped sites move over separately. Refs #35 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cristim
force-pushed
the
refactor/35-ce-pagination-helper
branch
from
October 16, 2026 20:54
51855eb to
8906050
Compare
Member
Author
IAmJSD
added a commit
to IAmJSD/cloud-commitments-go
that referenced
this pull request
Oct 16, 2026
fetchGroupedDayCoverage was the only Cost Explorer token loop in the package left uncapped after LeanerCloud#462/LeanerCloud#463. Route it through paginateCE with maxCoveragePages so endless fresh tokens or a token cycle stop at the cap with an error; keep the repeated-token check as a fast failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cristim
added a commit
that referenced
this pull request
Oct 17, 2026
* fix(aws): batch daily coverage sparkline fetch per region AttachDailyUsageHistory issued one GetReservationCoverage request per distinct (service, region, instance type), so CE requests scaled with SKU count. Group recommendations by (service, region). Buckets with more distinct types than lookback days now issue one region-wide request per day grouped by INSTANCE_TYPE (CE rejects DAILY granularity combined with GroupBy, so a single grouped request cannot return a daily series) and fan each type's series out to its recommendations. Smaller buckets keep the existing per-type DAILY request, so a bucket never costs more than min(types, lookback days) requests plus continuation pages. Per-type series, missing-day zeros and nil-for-no-data semantics match the per-type path; a failed grouped fetch leaves the bucket's usage history nil instead of reporting zero coverage. Closes #85 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(aws): stop grouped coverage paging on a repeated page token If Cost Explorer returned the same NextPageToken twice, the grouped per-day loop re-requested the same page until ctx was canceled. Return an error instead (CodeRabbit finding on #464). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(aws): cap grouped daily coverage paging through paginateCE fetchGroupedDayCoverage was the only Cost Explorer token loop in the package left uncapped after #462/#463. Route it through paginateCE with maxCoveragePages so endless fresh tokens or a token cycle stop at the cap with an error; keep the repeated-token check as a fast failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Cristian Magherusan-Stanciu <cristim@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is PR 1 of 2 for #35. The AWS provider has 9 Cost Explorer pagination loops in
providers/aws/recommendations, all hand-rolled. Three of them had no page cap:coverage.gofetchCoveragePaged;utilization.go;usage_history.go, which also had no context check.A Cost Explorer response that kept returning a next token made them loop forever.
New file:
ce_paginate.gopaginateCE(ctx, maxPages, what, page)runs one capped loop. It checksctx.Err()first and returns it unwrapped, because an existing test requiresEqual(context.Canceled). Then it checks the cap, then fetches. A nil or empty token ends the loop. The cap error is<what>: pagination cap reached after N pages, and partial results are never returned as complete. The callback keeps each site's own accumulation and error wrapping.Per-site caps
These are runaway guards, not size limits. No Cost Explorer input sets
MaxResults, so Cost Explorer chooses the page size.maxCoveragePagesandmaxUtilizationPages, both 100. The guard is generous because one over-cap pool aborts the wholeGetRICoverageMapsurvey (coverage.go:190-200).maxRecommendationPages(20). It uses a 7-day lookback, one SKU and no GroupBy, so it is a single page in practice. It now checks ctx every page.fetchDailyCoveragekeeps its "failed to get daily coverage for ..." prefix.Behaviour change: these three loops now fail loud past 100, 100 and 20 pages respectively. Before, they were unbounded.
Out of scope here
client.go,sp_coverage.go,sp_coverage_series.go,ondemand_series.go,sp_commitment_rate.go.collection_sp.gois deliberately excluded. It merges partial pages as per-scope errors, so failing the whole call would change its behaviour.ratelimiter.go(fix(aws/recs): honor canceled context on RateLimiter.Wait first attempt #432) andparser_ri.go(fix(aws): parse Cost Explorer EstimatedBreakEvenInMonths into BreakEvenMonths #452) are untouched.Refs #35
Verification (macOS, Go 1.26.9)
Commands
(cd providers/aws && GOWORK=off go test -race -short ./...)passes, and existing tests are unchanged.make buildandmake vetpass.Tests
context.Canceled), and callback error propagation.TestFetchCoveragePaged_CapsEndlessTokens,TestGetRIUtilization_CapsEndlessTokensandTestGetDailyUsagePcts_CapsEndlessTokens. Each asserts the literal cap and the call count. The mock fails with "runaway pagination" after 500 calls, so a regression fails fast instead of hanging CI.TestGetDailyUsagePcts_PreCanceledContext.Fail-before: on the original loops, all four site tests time out at 15s.
Mutation probes by the independent reviewer, in scratch copies. Each made a test fail within 2s:
>instead of>=at the cap;Review: two plan passes and two diff passes by the same non-author reviewer. Pass 1 led to the per-site caps of 100, the bounded mock with literal-cap assertions, and the error wording. No actionable findings remain on
fbdc9a0.Fakes only; no AWS call.
🤖 Generated with Claude Code
Summary by CodeRabbit