[cristim]

Problem

Closes #1144 (review finding PERF-02, P2)

fetchCoveragePage and fetchUtilizationPage in providers/aws/recommendations/ called the Cost Explorer SDK without acquiring the shared concurrency semaphore. The scheduler's coverage-enrichment phase (AttachDailyUsageHistory, looping per (service, region, resourceType) tuple from each of the 6 concurrent service goroutines) therefore escaped the CUDLY_MAX_PARALLELISM cap that was installed specifically to protect Lambda memory and Cost Explorer rate limits - worst case roughly 6 x min(N, 20) uncapped concurrent CE calls on top of the capped 20. It also made the GetAllRecommendations docstring invariant ("the Acquire/Release boundary lives around the individual CE SDK call") false for a third of the CE call sites.

Fix

Wrap both SDK calls in concurrency.Acquire/concurrency.Release, holding the slot only across the outbound call (so slots are free during rate-limit backoff waits), exactly matching the existing fetchRIPageWithRetry and fetchSPPageWithRetry pattern. All four CE call sites in the package now honor the cap, so the GetAllRecommendations docstring needed no change. Docstrings on both fetchers document the boundary.

Tests

New concurrency_cap_test.go:

• TestFetchCoveragePage_RespectsSharedSemaphore and TestFetchUtilizationPage_RespectsSharedSemaphore replicate the real scenario: a saturated capacity-1 shared semaphore on the context (the scheduler's collection path under load) plus cancellation, asserting the CE mock is never reached and context.Canceled is surfaced. Both FAIL on the pre-fix code (verified via git stash: "An error is expected but got nil", mock called) and pass post-fix.
• TestFetchPages_ReleaseSemaphoreSlot guards the Acquire/Release pairing: after successful fetches the slot must be available again (no leak).

Verification run:

• go test ./recommendations/ (from providers/aws/): 334 passed
• go vet ./recommendations/: clean
• go build ./... in both the root module and providers/aws: success

Note: go test -race flags a pre-existing data race on the shared rateLimiter (Reset from concurrent service sweeps, e.g. TestGetAllRecommendations_IncludesSavingsPlans); it reproduces identically on origin/main without this change and is out of scope here.

Summary by CodeRabbit

• Tests

  • Added comprehensive test suite for AWS recommendation fetching with concurrency constraints, ensuring API requests respect parallelism limits and handle capacity saturation correctly.

• Refactor

  • Updated AWS Cost Explorer API request handling to apply concurrency controls, preventing resource exhaustion during parallel operations.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4dc46be6-7692-4077-867a-22c7dee7eb36

📥 Commits

Reviewing files that changed from the base of the PR and between 7705441 and ce2e7f2.

📒 Files selected for processing (3)

• providers/aws/recommendations/concurrency_cap_test.go
• providers/aws/recommendations/coverage.go
• providers/aws/recommendations/utilization.go

📝 Walkthrough

Walkthrough

This PR adds semaphore-based concurrency gating to AWS Cost Explorer API calls in fetchCoveragePage and fetchUtilizationPage functions, wrapping the GetReservationCoverage and GetReservationUtilization SDK calls with acquire/release primitives to enforce the shared parallelism cap established by the scheduler. The test suite verifies saturation prevents calls and normal capacity correctly releases the semaphore slot.

Changes

Semaphore integration for Cost Explorer calls

Layer / File(s)	Summary
Saturation behavior verification providers/aws/recommendations/concurrency_cap_test.go	Mock Cost Explorer with call counters and helper fullSemaphoreCtx that pre-acquires the sole semaphore slot and cancels the context. Tests TestFetchCoveragePage_RespectsSharedSemaphore and TestFetchUtilizationPage_RespectsSharedSemaphore verify that both functions return context.Canceled without invoking their SDK methods when the semaphore is saturated.
Coverage fetch semaphore integration providers/aws/recommendations/coverage.go	Adds concurrency package import and wraps the GetReservationCoverage SDK call in fetchCoveragePage with concurrency.Acquire and concurrency.Release, holding the semaphore slot only during the outbound API request while preserving rate-limiter and retry logic.
Utilization fetch semaphore integration providers/aws/recommendations/utilization.go	Imports pkg/concurrency and wraps the GetReservationUtilization SDK call in fetchUtilizationPage with semaphore acquire/release, with error handling for acquisition failures. Existing rate-limiter reset/wait behavior is preserved.
Semaphore lifecycle validation providers/aws/recommendations/concurrency_cap_test.go	Test TestFetchPages_ReleaseSemaphoreSlot performs sequential coverage and utilization fetches under normal capacity (available semaphore) and verifies both SDK methods are invoked once each and the semaphore slot is released afterward via successful TryAcquire.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• LeanerCloud/CUDly#270: Both PRs are related because they introduce/use the shared pkg/concurrency semaphore by wrapping AWS Cost Explorer leaf calls with concurrency.Acquire/Release (main PR in fetchCoveragePage/fetchUtilizationPage, retrieved PR in scheduler/leaf concurrency primitives that set up the same cap mechanism).

Suggested labels

bug, urgency/this-sprint

Poem

🐰 Two API calls were running free,
Unbounded, wild, with no decree,
Now Acquire and Release stand guard,
The semaphore cap's no longer hard,
Lambda breathes easier now, you see! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: adding semaphore caps to coverage and utilization Cost Explorer calls, which is the primary fix described in the changeset.
Linked Issues check	✅ Passed	The PR fully addresses the requirements in issue #1144: it wraps fetchCoveragePage and fetchUtilizationPage CE calls with concurrency.Acquire/Release, adds comprehensive tests validating semaphore behavior, and aligns all four CE call sites with the shared concurrency cap.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the issue objective: updates to coverage.go and utilization.go add semaphore wrapping, concurrency_cap_test.go provides required regression tests, with no extraneous modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/perf-02-coverage-semaphore

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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]

Adversarial review

Reviewed against feedback_semaphore_at_api_call.md, feedback_rate_limiter_per_call.md, feedback_ctx_cancel_terminal.md, feedback_no_sleep_in_tests.md, feedback_no_hardcoded_magic_values.md, the package's existing fetchRIPageWithRetry / fetchSPPageWithRetry pattern, and the original PERF-02 finding in #1144. The fix is surgical and correct.

Verified (no change needed)

• Semaphore placement is at the API call, not the goroutine. Acquire is held only across the outbound GetReservationCoverage / GetReservationUtilization call, with Release immediately after — exactly matching feedback_semaphore_at_api_call.md and the in-package fetchRI/fetchSP pattern. The rate-limiter Wait (potentially up to 30s of backoff) runs OUTSIDE the slot, so the cap reflects real IO, not wall time.
• All four CE call sites in the package are now uniformly capped (RI purchase, SP purchase, coverage, utilization). The GetAllRecommendations docstring invariant ("Acquire/Release lives around the individual CE SDK call") is now true for all sites, no docstring update needed.
• Cancel semantics match feedback_ctx_cancel_terminal.md. semaphore.Acquire(ctx, 1) returns ctx.Err() on cancellation; the PR wraps with %w, preserving errors.Is(err, context.Canceled) so callers / scheduler can short-circuit. Verified via the regression test's assert.ErrorIs(t, err, context.Canceled).
• Acquire / Release pairing balanced on every path. Release is called unconditionally after the SDK call (success OR error), and the retry loop re-Acquires on each iteration — TestFetchPages_ReleaseSemaphoreSlot proves no leak via sem.TryAcquire(1) after a successful fetch.
• Tests use atomic.Int32 + t.Cleanup, no time.Sleep — clean per feedback_no_sleep_in_tests.md. Both regression tests verified pre-fix-fail / post-fix-pass via git stash per the PR body, and the package passes locally with -race on Coverage|Utilization subset (35/35) and full (334/334).
• Cap value (CUDLY_MAX_PARALLELISM=20) is documented + env-overridable in pkg/concurrency/concurrency.go:33, not a hidden magic number per feedback_no_hardcoded_magic_values.md.
• CE shares one rate-limit budget per account (it's per-account, not per-API), so the single shared semaphore across coverage/utilization/RI/SP is correct, not a starvation hazard.

Findings (non-blocking, file follow-ups)

1. RateLimiter.Wait at retryCount == 0 skips ctx check (pre-existing, exposed by this PR's test scaffolding). ratelimiter.go:62-65 returns nil immediately when retryCount == 0 without consulting ctx.Done(). The regression test TestFetchCoveragePage_RespectsSharedSemaphore happens to pass solely because concurrency.Acquire catches the cancellation a few lines later — but if anyone reorders the retry loop so the SDK call lands before the Acquire, a cancelled-from-the-start context would NOT be caught at the rate-limiter layer. Trivial fix: short-circuit on ctx.Err() even when retryCount==0. Out of scope here; filing follow-up.

2. Pre-existing rateLimiter race acknowledged in the PR body — already tracked in fix(aws/recs): data race on shared rateLimiter in fetchRIPageWithRetry #1263 (and the older bug(aws/recommendations): RateLimiter.Reset race after #269 parallelised GetAllRecommendations #271). No new race introduced by this PR; feedback_rate_limiter_per_call.md applies to those issues, not to this diff. Mentioned for the reviewer's audit trail only.

3. No defer on concurrency.Release — matches the existing in-package pattern in fetchRI/fetchSP for retry-loop tightness, so the consistency is correct (don't fix here). The only failure mode this would catch is an SDK panic, which the AWS SDK Go v2 doesn't do in normal operation. Mentioned for completeness; no follow-up.

Failing CI (unrelated, pre-existing on main)

• Lint Code — 2875 pre-existing issues across internal/*; none in this diff.
• Integration Tests — internal/config Postgres tests; unrelated to providers/aws/recommendations.
• Security Scanning — npm advisories in frontend (webpack, ws, svgo); not Go.
• CI Success — aggregator of the above.

These don't gate this PR's correctness; UNSTABLE is repo-wide noise, not introduced here.

Verdict

LGTM. Ready to merge once the unrelated repo-level CI failures are resolved or accepted.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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]

Rebased onto origin/main (73166c2). Clean rebase. Gates: go build, go vet, gocyclo -over 10, golangci-lint (0 new issues). Tests: providers/aws/recommendations (1 pass, all ok).

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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]

Merged to main (rebased onto current main, CLEAN + all CI green; closes #1144). hold the concurrency slot only around the outbound CE call, not the whole coverage/utilization sweep (semaphore-at-API-call).