fix(azure): bound concurrency in the multi-subscription recommendation fan-out

[cristim]

Summary

MultiSubscriptionRecommendationsClient.GetRecommendations (providers/azure/recommendations_multi_subscription.go) fanned out one goroutine per subscription with an unbounded errgroup, so a tenant with many subscriptions launched every subscription's collection at once. It now calls g.SetLimit(concurrency.MaxParallelismFromEnv()) (CUDLY_MAX_PARALLELISM, default 20, the same setting the rest of the collection tree uses).

This is an errgroup limit, not a second semaphore. The shared semaphore that bounds aggregate ARM calls is acquired only by the per-service goroutines inside each subscription client (recommendations.go goService) and released when they finish; the fan-out goroutines never hold it while waiting on the limit, so the two bounds cannot deadlock. The doc comment says so. Per-service slot holding is a separate concern (#8).

Closes #58

Tests

TestMultiSubscriptionRecommendationsClient_GetRecommendations_BoundsConcurrency: 24 subscriptions with the limit set to 3, a fake client tracking peak in-flight calls, asserting 1 < peak <= 3. Fails on origin/main ("24" is not less than or equal to "3"); removing SetLimit fails it the same way. Passes with -race -count=10.

Verification

make build, make vet, (cd providers/azure && GOWORK=off go test -race -short .), pinned golangci-lint v2.10.1 (0 issues), gofmt, and the commit/pre-push hooks pass.

Local review: non-author review of the diff, including where the shared semaphore is acquired and released, no findings.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements
  • Recommendation retrieval across multiple subscriptions now runs concurrently with a bounded number of subscriptions processed at once. This keeps multi-subscription requests responsive while limiting simultaneous work.

[cristim]

@coderabbitai review

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: e3f1b4d1-33ca-48e7-a73e-8a4e7ebf0a6f

📥 Commits

Reviewing files that changed from the base of the PR and between dcf1585 and 1940dfa.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

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: dd0f10f7-3766-46ef-8e3b-2574aff31497

📥 Commits

Reviewing files that changed from the base of the PR and between f57a838 and dcf1585.

📒 Files selected for processing: 2

• providers/azure/recommendations_multi_subscription.go
• providers/azure/recommendations_multi_subscription_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 multi-subscription recommendations client now limits concurrent subscription requests using concurrency.MaxParallelismFromEnv(). A test checks the limit across 24 subscriptions. The existing semaphore continues to bound aggregate ARM calls inside per-subscription clients.

Changes: Azure recommendations fan-out

Layer / File(s)	Summary
Bound subscription fan-out providers/azure/recommendations_multi_subscription.go, providers/azure/recommendations_multi_subscription_test.go	GetRecommendations sets an errgroup limit from concurrency.MaxParallelismFromEnv(). The documentation describes this limit alongside the existing ARM-call semaphore. A test checks that peak concurrency across 24 subscriptions is greater than 1 and no greater than 3 when the configured limit is 3.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to dcf15

The subscription fan-out is bounded as intended. No actionable merge-blocking risk remains after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Linked Issues check		Issue #58 requires a caller-independent bound for subscription fan-out, uses the CUDLY_MAX_PARALLELISM configuration, and includes a peak-concurrency test. GetRecommendations calls g.SetLimit(concurre…
Out of Scope Changes check		The production diff only adds the configured errgroup limit and updates its concurrency documentation. The test diff adds focused fake-client instrumentation and a regression test for issue #58. These…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: bounding concurrency in the Azure multi-subscription recommendation fan-out.

Full details: Linked Issues check

Explanation

Issue #58 requires a caller-independent bound for subscription fan-out, uses the CUDLY_MAX_PARALLELISM configuration, and includes a peak-concurrency test. GetRecommendations calls g.SetLimit(concurrency.MaxParallelismFromEnv()). The new test runs 24 subscriptions with a limit of 3 and asserts peak concurrency is at most 3 and greater than 1.

Full details: Out of Scope Changes check

Explanation

The production diff only adds the configured errgroup limit and updates its concurrency documentation. The test diff adds focused fake-client instrumentation and a regression test for issue #58. These changes support the linked issue and show no unrelated scope.

✨ Finishing Touches

📝 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: CLEAN at dcf15854f1c5e70e089dc46a9a920dfa3b26d539. Reviewer: claude-go-pr-merge-4 (merge driver, not the author). The PR body was treated as a claim.

The non-test diff adds one g.SetLimit(concurrency.MaxParallelismFromEnv()) call plus the doc comment.

What I attacked:

• A zero or invalid limit could hang the fan-out. It can't: MaxParallelismFromEnv (pkg/concurrency/concurrency.go:38-45) returns the env value only when n > 0 and otherwise DefaultMaxParallelism, so SetLimit never gets 0 or a negative.
• Deadlock between the two bounds. The shared semaphore is taken only inside the per-service leaf goroutines (providers/azure/recommendations.go:141-147, goService: Acquire and then a deferred Release). The per-subscription fan-out goroutines never hold it while they wait on the errgroup limit. A fan-out slot can wait on a semaphore slot, but never the reverse, so the doc comment's no-deadlock claim holds.

Evidence (macOS, exact SHA; fakes, no Azure call):

• TestMultiSubscriptionRecommendationsClient_GetRecommendations_BoundsConcurrency passes with -race -count=5.
• Mutation: removing SetLimit makes it FAIL with "24" is not less than or equal to "3", the same failure as on main.
• (cd providers/azure && GOWORK=off go test -race -short .) passes (249 tests), and make vet passes.

Labels mirror #58. CodeRabbit posted a summary. If it has not fully reviewed by merge time: CR waived: quota, adversarial review + local verification + green CI.

[cristim]

Merge gate: independent review + local verification (reviewed SHA 1940dfa)

Reviewed SHA: 1940dfa249d61527b4f16328fe9c7fe6f0443860 (head, unchanged at merge time).
Base: a7db6d5031ef33cb939fd0dd84e0e924d9bb20fe (main).
Reviewer: non-author. Diff reviewed as written; the plan's two adversarial passes were on an earlier revision of the base, not this diff.

Identity with the previously reviewed commit

This head is a pure rebase of dcf1585, the commit an earlier non-author review covered. Both produce stable patch-id ce8d4137551ecc18e97dc58d21e090204cef5186, so the reviewed content and the head content are byte-identical patches. The review below was done against the head itself, not inherited from the earlier one.

What was attacked

• Hang risk from g.SetLimit. concurrency.MaxParallelismFromEnv() returns DefaultMaxParallelism (20) on unset, unparseable and non-positive values (pkg/concurrency/concurrency.go:38-45), so the limit can never be 0 or negative. With golang.org/x/sync/errgroup, SetLimit(0) would block the first g.Go forever, so this was the sharpest failure mode; it is unreachable.
• Deadlock against the shared semaphore (the claim in the doc comment). The fan-out goroutines acquire nothing before sub.client.GetRecommendations (recommendations_multi_subscription.go:259-267), so a goroutine holding a semaphore permit never waits on an errgroup slot and no cycle exists. The two bounds are independent.
• Error isolation. Each goroutine still returns nil, so one subscription failing never cancels a sibling, and ctx.Err() is still propagated explicitly after g.Wait().
• Nested limits. The per-subscription clients apply their own caps downstream; an outer cap only delays admission, it never holds a permit while waiting.

Local verification (macOS, at the reviewed SHA)

• providers/azure: GOWORK=off go build ./... pass, GOWORK=off go vet . pass.
• GOWORK=off go test -race -short -count=10 -run TestMultiSubscriptionRecommendationsClient . ok (10 consecutive runs, so the peak-in-flight assertion is not a flake).
• Mutation probes, each reverted immediately and the tree confirmed clean afterwards:
  1. Remove the limit (origin/main behaviour, keeping the import used so the build survives): the new test fails with "24" is not less than or equal to "3".
  2. Ignore the env override and use the hard default 20: the test fails with "20" is not less than or equal to "3".

Honest coverage note: the evidence is fixture-based. The peak-in-flight test drives GetAllRecommendations through a fake subscription client, not a live ARM call. No real Azure account was used and none is needed: the property under test is the fan-out's own admission control, which does not depend on ARM responses. The test asserts the weaker bound (1 < peak <= 3) on purpose: it proves the fan-out is both bounded and still concurrent, so a change that serialised the sweep would not pass either.

CodeRabbit raised no actionable finding on this diff. CI is green on 1940dfa (14 checks).

Verdict: CLEAN. No actionable findings.