feat(azure): org-wide multi-subscription recommendation collection (closes #553) - #1520
feat(azure): org-wide multi-subscription recommendation collection (closes #553)#1520cristim wants to merge 10 commits into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAzure subscription discovery now uses a thread-safe cache with invalidation and independent returned slices. Recommendation client resolution supports pinned, single-subscription, and multi-subscription modes. Multi-subscription results are fetched concurrently, merged, and surfaced with typed partial-failure errors. Scheduler and command flows retain successful partial results. ChangesAzure subscription-aware recommendations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AzureProvider
participant SubscriptionAPI
participant MultiSubscriptionRecommendationsClient
participant RecommendationClients
Caller->>AzureProvider: GetRecommendationsClient
AzureProvider->>SubscriptionAPI: Discover accessible subscriptions
SubscriptionAPI-->>AzureProvider: Subscription accounts
AzureProvider-->>Caller: Single- or multi-subscription client
Caller->>MultiSubscriptionRecommendationsClient: GetRecommendations
MultiSubscriptionRecommendationsClient->>RecommendationClients: Query subscriptions concurrently
RecommendationClients-->>MultiSubscriptionRecommendationsClient: Recommendations and errors
MultiSubscriptionRecommendationsClient-->>Caller: Merged recommendations and partial-failure status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/provider.go (1)
280-373: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid holding
accountsMuduring the ARM list call
fetchAccountsLockedcallspager.NextPage(ctx)under the write lock, so any slow or hung subscription lookup blocks every concurrent caller that reaches account discovery. Add an internal timeout here, or move the fetch outside the mutex and only lock around cache population.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 280 - 373, Update getOrFetchAccounts and fetchAccountsLocked so the ARM subscription listing does not run while accountsMu is held. Fetch accounts outside the mutex, then reacquire the lock to re-check cachedAccounts and populate it only if still empty, returning the existing cached result otherwise; preserve cloneAccounts behavior and propagate fetch errors unchanged.
🧹 Nitpick comments (1)
providers/azure/provider.go (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the subscription-cache logic into its own file.
getOrFetchAccounts,cloneAccounts,InvalidateAccountsCache, andfetchAccountsLockedform a cohesive, self-contained unit (~100 lines) added to an already large file (content in this diff runs past line 635). As per coding guidelines, Go/TS files should be kept under 500 lines; extracting this caching subsystem into a sibling file (e.g.providers/azure/accounts_cache.go) would restore compliance with low mechanical risk and improve cohesion.Also applies to: 270-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 99 - 109, The subscription-cache subsystem should be moved out of the large provider file into a sibling Go file. Extract getOrFetchAccounts, cloneAccounts, InvalidateAccountsCache, fetchAccountsLocked, and the accountsMu/cachedAccounts state together, preserving their behavior and existing access from GetAccounts, GetServiceClient, and GetRecommendationsClient.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@providers/azure/provider.go`:
- Around line 280-373: Update getOrFetchAccounts and fetchAccountsLocked so the
ARM subscription listing does not run while accountsMu is held. Fetch accounts
outside the mutex, then reacquire the lock to re-check cachedAccounts and
populate it only if still empty, returning the existing cached result otherwise;
preserve cloneAccounts behavior and propagate fetch errors unchanged.
---
Nitpick comments:
In `@providers/azure/provider.go`:
- Around line 99-109: The subscription-cache subsystem should be moved out of
the large provider file into a sibling Go file. Extract getOrFetchAccounts,
cloneAccounts, InvalidateAccountsCache, fetchAccountsLocked, and the
accountsMu/cachedAccounts state together, preserving their behavior and existing
access from GetAccounts, GetServiceClient, and GetRecommendationsClient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9598f7d5-1fb8-4c30-8e08-d7e1d1739c53
📒 Files selected for processing (4)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
6a8155f to
bdd3260
Compare
|
@coderabbitai full review Force-pushed a fix for the concurrency finding on |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 35 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
providers/azure/provider_test.go (1)
1398-1407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
runtime.Gosched()nudge doesn't guarantee the followers are on the cold-cache path.Nothing blocks until the followers actually reach
getOrFetchAccounts, soclose(gated.release)can fire while some goroutines haven't started. Those late callers then hit an already-warm cache, and a naive per-caller-fetch implementation would still count 1 — the regression this test exists to catch can slip through. Gating each follower on an explicit "entered" signal (e.g. a second counter/channel incremented beforeGetAccounts, plus a barrier) makes it deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1398 - 1407, Replace the runtime.Gosched-based coordination in the concurrent GetAccounts test with an explicit follower-entry barrier: have each follower signal an entered counter/channel immediately before invoking GetAccounts, wait until all followers have entered the cold-cache path, then close gated.release. Keep the existing worker synchronization and assertions unchanged so late goroutines cannot observe a warmed cache.providers/azure/accounts_cache.go (1)
43-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDetach the shared cache fill from the caller’s context.
getOrFetchAccountspasses the singleflight leader’sctxintofetchAccounts, so one canceled request can abort the shared ARM list call for every waiter and leave the cache cold. Usecontext.WithoutCancel(ctx)here, or retry once oncontext.Canceled.providers/azure/accounts_cache.go:43-68🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/accounts_cache.go` around lines 43 - 68, Update the shared fetch in getOrFetchAccounts so fetchAccounts receives a context detached from caller cancellation, using context.WithoutCancel(ctx). Keep the singleflight cache population and error propagation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/provider.go`:
- Around line 457-470: Update the recommendation client resolution around
p.subscriptionID and getOrFetchAccounts to honor the subscription ID resolved by
the provider’s default-subscription logic, including AZURE_SUBSCRIPTION_ID,
before falling back to all visible accounts. Reuse the existing
resolveSubscriptionIDFromCtx or equivalent resolved-default path so
recommendations remain scoped consistently with GetServiceClient and GetRegions,
while preserving explicit p.subscriptionID handling and multi-account fan-out
only when no default is resolved.
In `@providers/azure/recommendations_multi_subscription_test.go`:
- Around line 43-47: Update the doc comment for withFakeSubscriptionClients to
state that it provides fake clients by subscriptionID from the supplied map,
rather than describing ordered allocation by account order. Keep the
cleanup/restoration behavior documentation accurate.
---
Nitpick comments:
In `@providers/azure/accounts_cache.go`:
- Around line 43-68: Update the shared fetch in getOrFetchAccounts so
fetchAccounts receives a context detached from caller cancellation, using
context.WithoutCancel(ctx). Keep the singleflight cache population and error
propagation unchanged.
In `@providers/azure/provider_test.go`:
- Around line 1398-1407: Replace the runtime.Gosched-based coordination in the
concurrent GetAccounts test with an explicit follower-entry barrier: have each
follower signal an entered counter/channel immediately before invoking
GetAccounts, wait until all followers have entered the cold-cache path, then
close gated.release. Keep the existing worker synchronization and assertions
unchanged so late goroutines cannot observe a warmed cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94f3473b-66c2-4c25-83fe-3c5fefbdfe43
📒 Files selected for processing (5)
providers/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
Adversarial review pass: 6 confirmed findings, all fixedReviewed with a focus on cross-subscription data bleed, since this PR is the first Azure path that can return more than one subscription's data from a single client. Six confirmed issues, each with a regression test verified to fail with its fix reverted and pass with it in place. Commits: 20d3e53 (azure), 671ced2 (scheduler). 1.
|
|
🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
Cache the ARM subscriptions list on AzureProvider (getOrFetchAccounts, double-checked locking, InvalidateAccountsCache for tests) so repeated GetAccounts/GetServiceClient/GetRecommendationsClient calls in one run do not each re-issue the subscriptions.List API call. Add MultiSubscriptionRecommendationsClient, which fans recommendation collection out across every subscription visible to the authenticated principal via errgroup, isolating per-subscription errors and only failing when every subscription fails. GetRecommendationsClient now returns this fan-out client when no subscription is pinned and 2+ are discovered, bringing Azure to parity with the AWS provider's automatic whole-organization recommendation coverage. Pinned-subscription and single-subscription paths are unchanged. Closes #553
… the ARM fetch getOrFetchAccounts held accountsMu.Lock() for the entire subscriptions.List network round-trip via fetchAccountsLocked, so every reader of the cached subscription list blocked behind a slow or hung ARM call. Move the fetch into golang.org/x/sync/singleflight so concurrent cold-cache callers coalesce into one in-flight request, taking accountsMu only to read or populate cachedAccounts. Add a deterministic regression test that stages N goroutines through a channel-gated fake fetch and asserts exactly one underlying ARM call.
Adversarial review of the org-wide fan-out. Scoping first. GetRecommendationsClient ignored the resolved default subscription. Before fan-out existed, an unpinned provider resolved the default via resolveSubscriptionIDFromCtx, which honours AZURE_SUBSCRIPTION_ID and errors when the scope is ambiguous. The fan-out path replaced that with "2+ visible subscriptions means fan out", so a caller that pinned a subscription through the environment silently started receiving every other visible subscription's recommendations. Restore the precedence: a resolvable default still yields a single-subscription client, a configured AZURE_SUBSCRIPTION_ID naming a subscription this principal cannot see is an error, and only a genuinely unnamed scope fans out. MultiSubscriptionRecommendationsClient ignored params.AccountFilter. The AWS provider applies that filter to every recommendation it returns (filterByAccounts in providers/aws/service_client.go). It was moot while an Azure client could only ever cover its own subscription, but a client covering every visible subscription has to honour it or a request scoped to one account comes back carrying others'. Filter before the fan-out so subscriptions outside the filter are never queried, and error rather than return an empty slice when the filter matches nothing. Then the subscription-list cache. InvalidateAccountsCache was a no-op against an in-flight fetch: a fetch that started before the invalidation still published its pre-invalidation snapshot afterwards, so the next read was served from cache and handed back exactly the data the caller asked to discard, with no second ARM call. A caller arriving in that window also joined the still in-flight pre-invalidation call through the constant singleflight key. Both are closed by an accountsGen counter that keys the singleflight call and gates the cache write. singleflight hands the leader's error to every waiter, so a caller whose own context was still live inherited a cancellation raised by whichever unrelated caller won the leader election. Retry once as leader when our own context is still healthy; our own cancellation stays terminal, so this cannot spin on a dead context. The unchecked type assertion on singleflight's untyped result now returns an error instead of panicking. Test fixes: the context-cancellation test passed with the guard it named deleted, because fakes that return ctx.Err() make every subscription fail and the all-failed guard produces the same error either way; its fakes now ignore the cancelled context so only the post-Wait ctx.Err() check can produce the error. The new cache tests read AZURE_SUBSCRIPTION_ID from the ambient environment and now neutralise it, the concurrent cold-cache test waits for every follower to enter GetAccounts rather than relying on a runtime.Gosched nudge, and the constructor test asserts the built client is actually stored. Each behaviour has a regression test verified to fail with its fix reverted. Refs #553
collectAzureForAccount tags every recommendation it returns with the registered account's UUID, so the provider has to be pinned to that account's subscription. It built the provider from acct.AzureSubscriptionID without checking it was set, and validateCloudAccountRequest does not require the field, so an azure row created without one produced an unpinned provider. That was harmless while an unpinned provider resolved a single subscription or errored. With the org-wide fan-out it instead collects from every subscription the credential can see, and those results are then filed under this one account, where anyone authorised for it can read them. Reject the misconfigured row by name instead.
SetCredential is used in production, not only in tests: the scheduler and purchase-execution paths construct the provider and then install per-account federated credentials. The subscription-list cache added in this PR records what the previous credential could see, so leaving it in place across a swap would report subscriptions the new principal may have no access to, and GetRecommendationsClient would fan out across them. Every caller today installs the credential before the first accounts fetch, so this is a guard against a future reordering rather than a live leak. SetSubscriptionsClient gets the same treatment for the same reason.
31b45cb to
3e07ed2
Compare
|
Rebased onto History was rewritten, so this is a force-push: Since every commit SHA changed, the previous review no longer corresponds to anything on this branch. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
|
Status update: head is The two nitpicks from the 16:17Z review were both addressed in The full review requested at 18:46Z was acknowledged but no review has landed since 16:17Z, so re-requesting. Please do a full pass, not an incremental one, since the branch was rebased after that review. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
providers/azure/recommendations_multi_subscription.go (1)
189-202: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffPartial failures are only visible in logs.
A sweep where 9 of 10 subscriptions fail returns
nilerror, so downstream aggregation/reporting treats the result as complete org-wide coverage. Consider returning the covered/failed subscription counts (or a partial-error type likeinternal/purchase'smultiAccountPartialError) so callers can annotate reports as incomplete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription.go` around lines 189 - 202, Update the multi-subscription recommendation aggregation around the errs loop and its failures check so any mix of successful and failed subscriptions returns an explicit partial-failure signal, not a nil error. Include covered and failed subscription counts, or reuse the existing multiAccountPartialError pattern from internal/purchase, while preserving the all-failed error behavior and collected successful results.providers/azure/provider_test.go (1)
1614-1624: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoverage gap: env target invisible with exactly one visible subscription.
This subtest pins the two-subscription case, where no default resolves and the fail-loud branch fires. The one-visible-subscription variant takes the opposite path (
resolveDefaultSubscriptionmarks the lone sub default, so Step 1 returns a client for a subscription the caller never named). Adding that case would pin the behavior either way — see the related comment onproviders/azure/provider.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1614 - 1624, Add a subtest alongside the existing invisible-subscription test that sets AZURE_SUBSCRIPTION_ID to an unseen value while configuring exactly one visible subscription, then call GetRecommendationsClient and assert the intended behavior from resolveDefaultSubscription: it must not return a client for the unnamed visible subscription, and should preserve the explicit-target error contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/provider.go`:
- Around line 504-520: The subscription target validation occurs too late
because resolveDefaultSubscription can select the sole visible subscription
first. In providers/azure/provider.go lines 504-520, validate
AZURE_SUBSCRIPTION_ID against accounts before calling getDefaultSubscriptionID
and return the existing misconfiguration error when it is invisible; in
providers/azure/provider_test.go lines 1614-1624, add coverage for an invisible
target with exactly one visible subscription and assert the error outcome.
---
Nitpick comments:
In `@providers/azure/provider_test.go`:
- Around line 1614-1624: Add a subtest alongside the existing
invisible-subscription test that sets AZURE_SUBSCRIPTION_ID to an unseen value
while configuring exactly one visible subscription, then call
GetRecommendationsClient and assert the intended behavior from
resolveDefaultSubscription: it must not return a client for the unnamed visible
subscription, and should preserve the explicit-target error contract.
In `@providers/azure/recommendations_multi_subscription.go`:
- Around line 189-202: Update the multi-subscription recommendation aggregation
around the errs loop and its failures check so any mix of successful and failed
subscriptions returns an explicit partial-failure signal, not a nil error.
Include covered and failed subscription counts, or reuse the existing
multiAccountPartialError pattern from internal/purchase, while preserving the
all-failed error behavior and collected successful results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bc56d4d-e406-434b-a35f-2a232a3bfe7a
📒 Files selected for processing (7)
internal/scheduler/scheduler.gointernal/scheduler/scheduler_test.goproviders/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
CodeRabbit finding. GetRecommendationsClient consulted getDefaultSubscriptionID before validating the configured target, and resolveDefaultSubscription's rule 3 marks a lone visible subscription as the default even when a target was configured and did not match it. So with AZURE_SUBSCRIPTION_ID naming a subscription the credential cannot see and exactly one subscription visible, that one subscription was returned as the default and the fail-loud check further down was unreachable. An operator who typo'd the variable, or whose credential lost access to the intended subscription, silently collected against the wrong subscription instead of getting an error: a misconfiguration answered with a plausible wrong result. Validate the target against the discovered accounts first, via a direct membership check rather than IsDefault, so it fails loud regardless of how many subscriptions are visible. A target that IS visible is honoured directly. The previous test only covered two visible subscriptions, where rule 3 never fires; the new subtest pins the single-visible-subscription case and fails against the old ordering.
CodeRabbit's Major finding: confirmed and fixed in
|
Response to the review on
|
Pre-merge check for any partial-failure change to
|
| Line | Path |
|---|---|
| 689 | Azure org-wide GetAllRecommendations |
| 708 | GCP org-wide GetAllRecommendations |
| 801 | Azure per-account GetAllRecommendations |
| 869 | GCP per-account GetAllRecommendations |
| 896 | generic per-provider GetAllRecommendations |
| 911 | generic default term/payment fallback GetRecommendations |
Lines 689, 801, and the 896/911 generic path are the routes that actually reach
the Azure multi-subscription client, so they are not hypothetical.
cmd/multi_service_helpers.go:441 is already handled in the in-flight work via
asPartialSubscriptionFailure, but that call site alone is not sufficient
coverage. The scheduler is the path that runs unattended, which makes it the
one where silently dropped data is least likely to be noticed.
Suggested verification before merge
- Every call site above either inspects the error with
errors.Asand keeps the
partial results, or is deliberately left failing closed with a stated reason. - A regression test on the scheduler path: a sweep where a strict subset of
subscriptions fails still records the successful subscriptions'
recommendations, rather than returning zero rows.
Also worth confirming the AWS and GCP recommendation clients, which implement
the same interface, are unaffected by the new error semantics.
mergeSubscriptionResults returned a nil error whenever at least one subscription succeeded, so 3 of 50 subscriptions failing handed back 47 subscriptions' recommendations indistinguishably from a complete sweep. The only signal was a log line, which no programmatic caller reads. On a path whose output is persisted and rendered as a savings opportunity, an under-collected sweep then reads as a shrinking opportunity rather than a failed one. Add PartialSubscriptionFailureError, returned ALONGSIDE the successful subscriptions' recommendations, carrying the attempted/succeeded counts and every failed subscription with its cause. Unwrap exposes the per-subscription causes so errors.Is still matches through it. Failing the whole sweep on one transient error would be worse than a partial result, so the data is still returned; callers opt in to keeping it by inspecting the error with errors.As, and a caller that treats any non-nil error as fatal now fails loud instead of silently under-reporting. Both outcomes beat the previous silent success. Updated the two call sites that can reach the fan-out (the CLI paths, which are the only ones that construct an unpinned Azure provider) to keep the partial data and report the gap. The scheduler's Azure paths always pin a subscription -- collectAzureAmbient via AZURE_SUBSCRIPTION_ID and collectAzureForAccount via the account's subscription -- so they never build the fan-out client and need no change. Tests assert the distinguishing property directly: a sweep where a subscription was never queried and a complete sweep that found nothing both produce an empty slice, and only the error tells them apart. Verified failing against the old nil-error contract.
Partial-failure signal added (
|
Second-order effect of returning a typed partial-failure error. Every
recommendation-fetch site in the scheduler has the shape
recs, err := recClient.GetAllRecommendations(ctx)
if err != nil { return nil, ... }
which discards recs. Correct for a real error, but for a partial sweep it
turned one flaky subscription out of fifty into a total collection outage:
strictly worse than the silent under-collection the typed error exists to
prevent. The diff that introduced it looked like a pure improvement in
isolation and only misbehaved at callers that were not re-read.
Add tolerateIncompleteSweep and route every fetch site through it, so the
policy lives in one place instead of relying on each call site to remember
it. A partial sweep logs a warning naming the subscriptions that were not
queried and returns nil, so the data that was collected is kept and
persisted. Any other error is returned unchanged.
Also export AsPartialSubscriptionFailure from the azure provider so the
scheduler and the CLI share one definition of the check rather than each
hand-rolling errors.As, and add FailedSubscriptionIDs for the log lines.
Recording the incompleteness in the state table's last_collection_error, so
the dashboard shows the sweep as partial rather than merely smaller, needs a
partial-note threaded through collectProviderRecommendations and its three
per-provider implementations. Left as follow-up rather than bolted on here;
the warning names the failed subscriptions in the meantime.
Tests drive fetchAndConvert with a client returning recommendations plus a
partial-failure error and assert the data survives, that a genuine error
still fails loud, and that the helper is narrow. Verified failing with the
tolerance removed.
Second-order regression from the partial-failure error, fixed (
|
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 38 minutes. |
The Lint Code job pins golangci-lint v2.10.1 and .golangci.yml sets misspell locale to US, so the British spelling in these two comments failed the lint gate and cascaded to the CI Success aggregate: internal/scheduler/scheduler.go:910 internal/scheduler/scheduler_test.go:766 Both are comment prose, so this changes no identifier, no database column name and no string any test asserts on. Scoped deliberately to the two lines the linter flagged. 49 further occurrences of the British spelling remain elsewhere in the tree and were left alone: they are pre-existing, unrelated to this branch, and a repo-wide rewrite here would bury a two-word fix under a diff touching dozens of files.
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 26 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 6 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
providers/azure/recommendations_multi_subscription.go (2)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate package doc comment.
providers/azure/provider.goalready carries thePackage azuredoc; a second one in this file means tooling picks an arbitrary winner. Demote it to a plain file comment (// Org-wide (multi-subscription) recommendations fan-out client.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription.go` around lines 1 - 3, Replace the package doc comment in the multi-subscription recommendations file with a plain file comment, leaving the existing Package azure documentation in provider.go as the sole package-level doc.
79-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Error()panics on an emptyFailedslice.
e.Failed[0].Erris unguarded.mergeSubscriptionResultsnever produces an emptyFailed, but the type and its fields are exported, so a zero-value or externally constructed instance panics the moment it is formatted — typically inside a log/error-wrapping path.🛡️ Guard the first-cause read
func (e *PartialSubscriptionFailureError) Error() string { + if len(e.Failed) == 0 { + return fmt.Sprintf("azure recommendations incomplete: %d of %d subscriptions succeeded", + e.Succeeded, e.Attempted) + } return fmt.Sprintf( "azure recommendations incomplete: %d of %d subscriptions succeeded; %d failed (%s): %v", e.Succeeded, e.Attempted, len(e.Failed), strings.Join(e.FailedSubscriptionIDs(), ", "), e.Failed[0].Err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription.go` around lines 79 - 84, Update PartialSubscriptionFailureError.Error to guard access to e.Failed[0].Err when Failed is empty. Preserve the existing first-failure detail for non-empty slices, while returning a safe error message for zero-value or externally constructed instances without panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/provider.go`:
- Around line 169-197: Synchronize credential and subscriptions-client swaps
with the accounts cache mutex to eliminate races and mixed snapshots. Update
SetCredential and SetSubscriptionsClient to assign their fields while holding
accountsMu, and update fetchAccounts to snapshot cred and subscriptionsClient
under RLock before using them; retain cache invalidation after the synchronized
publication.
---
Nitpick comments:
In `@providers/azure/recommendations_multi_subscription.go`:
- Around line 1-3: Replace the package doc comment in the multi-subscription
recommendations file with a plain file comment, leaving the existing Package
azure documentation in provider.go as the sole package-level doc.
- Around line 79-84: Update PartialSubscriptionFailureError.Error to guard
access to e.Failed[0].Err when Failed is empty. Preserve the existing
first-failure detail for non-empty slices, while returning a safe error message
for zero-value or externally constructed instances without panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70fdd710-c9da-42f4-9388-8eedd2dcbff2
📒 Files selected for processing (8)
cmd/multi_service_helpers.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.goproviders/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
Three review findings on the org-wide multi-subscription fan-out. 1. A partially-satisfied AccountFilter silently narrowed the sweep. A filter matching NO accessible subscription already errored, but one matching some and missing others returned the matched subscriptions' recommendations with a nil error. An operator scoping a sweep to sub-A and sub-B whose principal has lost Reader on sub-B therefore read sub-B as "no savings available" rather than "never queried" -- the exact confusion PartialSubscriptionFailureError exists to prevent, and which selectSubscriptions' own doc comment says must not happen. selectSubscriptions now iterates the filter rather than the discovered subscriptions, so every requested entry is accounted for as matched or missed, and the misses are folded into the partial-failure error as ErrSubscriptionNotAccessible. They count towards Attempted and Failed; the subscriptions that did answer still return their data, so one revoked subscription does not become a total collection outage. The filter is also deduplicated, so a repeated entry is neither queried twice nor double-counted. 2. PartialSubscriptionFailureError.Error() indexed Failed[0] unguarded. The type and its fields are exported, so a zero-value or externally-constructed instance panicked when formatted -- an error that crashes when logged fails exactly when the message is needed. 3. SetCredential and SetSubscriptionsClient wrote cred/subscriptionsClient as plain assignments and only then took accountsMu to invalidate, while fetchAccounts read both from a goroutine running on behalf of every caller that joined the single-flight. Both fields are now published inside one accountsMu critical section (via invalidateAccountsCacheLocked, since RWMutex is not reentrant) and snapshotted together under RLock, which also closes the window where a fetch pairs the old client with the new credential. The remaining in-package reads go through the same accessors so the guarantee holds rather than covering only one path. No production caller is concurrent today, so this was latent. Both behavioral fixes are covered by tests confirmed to fail on the pre-fix code: the partial-filter test with "An error is expected but got nil", and the swap test with a data race reported by -race. Refs #1652, #1653, #1654
|
Pushed Fixed1. A partially-satisfied
2. 3. Unsynchronized Both behavioral fixes carry regression tests confirmed to fail on the pre-fix code, not merely to pass on the fixed code:
Disclosed (not "fixed" — the code stays as scaffolding)The PR body now states plainly that this diff is a behavioral no-op in production today, naming all three unreachable integration surfaces rather than just the first: the fan-out client itself, the Azure partial-handling in I verified each claim against the code rather than taking the review's word for it — including the one place the reviewer's summary was imprecise: Not deleting the code: it is scaffolding for the follow-up that wires it up. But it should not read as shipped either. Deferred (filed, not fixed here)
GatesAll run locally, exit codes reported rather than inferred from empty output:
The golangci run used the exact CI invocation (repo root, no path arguments) at the version @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff stat ==='
git diff --stat origin/main...HEAD || git diff --stat HEAD~1..HEAD
echo '=== Changed files ==='
git diff --name-only origin/main...HEAD || git diff --name-only HEAD~1..HEAD
echo '=== Azure symbol outlines ==='
for f in \
providers/azure/recommendations_multi_subscription.go \
providers/azure/accounts_cache.go \
providers/azure/provider.go \
internal/scheduler/scheduler.go \
cmd/multi_service_helpers.go \
internal/purchase/execution.go
do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
echo '=== Recommendation-client construction and calls ==='
rg -n -C 3 --glob '*.go' \
'(NewAzureProvider|GetRecommendationsClient\s*\(|GetAllRecommendations\s*\(|GetRecommendations\s*\(|tolerateIncompleteSweep|AsPartialSubscriptionFailure|selectSubscriptions|ErrSubscriptionNotAccessible|SetCredential\s*\(|SetSubscriptionsClient\s*\()' \
providers internal cmd pkgLength of output: 173995 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Multi-subscription implementation ==='
sed -n '35,395p' providers/azure/recommendations_multi_subscription.go
echo '=== Account cache implementation ==='
sed -n '40,305p' providers/azure/accounts_cache.go
echo '=== Azure provider synchronization and client resolution ==='
sed -n '90,255p' providers/azure/provider.go
sed -n '525,615p' providers/azure/provider.go
echo '=== Scheduler partial-error policy and Azure construction ==='
sed -n '670,825p' internal/scheduler/scheduler.go
sed -n '890,965p' internal/scheduler/scheduler.go
echo '=== Regression tests for new filter/error behavior ==='
sed -n '135,455p' providers/azure/recommendations_multi_subscription_test.go
echo '=== Regression tests for concurrency / credential swaps ==='
sed -n '1360,1675p' providers/azure/provider_test.go
echo '=== Full diff, compact context ==='
git diff --unified=20 origin/main...HEAD -- \
providers/azure/recommendations_multi_subscription.go \
providers/azure/accounts_cache.go \
providers/azure/provider.go \
internal/scheduler/scheduler.go \
cmd/multi_service_helpers.go \
| sed -n '1,2200p'Length of output: 149404 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== All AzureProvider mutable-field accesses ==='
rg -n -C 4 --glob '*.go' \
'p\.(cred|credErr|credOnce|credProvider|subscriptionsClient|subscriptionID|cachedAccounts|accountsGen)\b' \
providers/azure
echo '=== IsConfigured and initialization tests ==='
sed -n '250,360p' providers/azure/provider.go
sed -n '115,245p' providers/azure/provider_test.go
rg -n -C 5 --glob '*test.go' \
'(IsConfigured|SetCredentialProvider|SetCredential).*([Cc]oncurrent|[Rr]ace)|([Cc]oncurrent|[Rr]ace).*(IsConfigured|SetCredentialProvider|SetCredential)' \
providers/azure
echo '=== PR body and current head ==='
git rev-parse --short HEAD
gh pr view 1520 --repo LeanerCloud/CUDly --json title,body,headRefOid,mergeStateStatus,statusCheckRollup \
--jq '{title, headRefOid, mergeStateStatus, checks: [.statusCheckRollup[] | {name, conclusion: .conclusion}], body}'Length of output: 26961
One issue remains:
I did not identify additional blockers in the reported AccountFilter or cache fixes. 🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
Summary
AzureProvider(getOrFetchAccounts, single-flight underaccountsMu sync.RWMutex) soGetAccounts,GetServiceClient, andGetRecommendationsClientdon't each re-issue thesubscriptions.ListAPI call within one logical runMultiSubscriptionRecommendationsClient, which fans recommendation collection out across every subscription visible to the authenticated principal (errgroup, per-subscription error isolation, all-attempted-failed guard, typed partial-failure error)GetRecommendationsClientreturns the fan-out client when no subscription is pinned and 2+ subscriptions are discovered. Pinned-subscription and single-discovered-subscription paths are unchanged.Nothing in this 1787-line diff changes what CUDly does when deployed as it is currently wired. The fan-out is scaffolding for a follow-up that connects it; it is not a shipped feature. All three integration surfaces it adds are unreachable:
MultiSubscriptionRecommendationsClientitself. It is only returned byGetRecommendationsClienton the unpinned path, and every AzureGetRecommendationsClientcall site reaches it through a pinned provider:internal/scheduler/scheduler.go:685(collectAzureAmbient) — the subscription comes fromAZURE_SUBSCRIPTION_ID, and the caller at:752returns early when that is empty, so the provider is always pinned.internal/scheduler/scheduler.go:799(collectAzureForAccount) — guarded by the explicitacct.AzureSubscriptionID == ""fail-fast at:782.internal/purchase/execution.go:476builds an Azure provider fromaccount.AzureSubscriptionIDwith no empty-guard, but that provider is used for purchase execution only — it never reachesGetRecommendationsClient.pkg/provider/registry.go:82-93stores provider factories, not instances, so there is no shared unpinned provider a caller could pick up either.The Azure partial-failure handling in
cmd/multi_service_helpers.go(:103-111and:449-459). Dead:recClientin that file is always the AWS adapter (cmd/multi_service.go:122), anddiscoverRegionsForServiceis reachable only via an AWSDescribeRegionsfailure. These branches add acmd→providers/azureimport for code no input can reach, and carry nocmd-level tests.tolerateIncompleteSweep(internal/scheduler/scheduler.go:906-925). A no-op at all five of its call sites: every Azure path pins a subscription, and GCP/AWS never return*azure.PartialSubscriptionFailureError.What would wire it up: a caller that constructs
AzureProviderwithoutAzureSubscriptionID/Profile— e.g. an org-wide collection run for a service principal holding Reader across many subscriptions, rather than today's per-registered-account loop. That change is deliberately out of scope here: it is a collection-model decision with its own persistence consequences (tracked in #1652), not a mechanical follow-on.Reading the test suite alone would reasonably suggest the feature is live. It is not. This section exists so a future maintainer does not have to rediscover that.
Why fan-out instead of a single org-wide API call
AWS Cost Explorer's
AccountScope=Linkedcovers an entire organization in one API call. Azure has no equivalent — the Consumption Reservation Recommendations and Advisor APIs are subscription-scoped — so achieving org-wide coverage requires calling the per-subscriptionRecommendationsClientAdapteronce per subscription and aggregating client-side. This mirrors the design proposed in #561 (closed as stale; reimplemented fresh against currentmainhere since that branch's diff no longer applies cleanly).What changed (per file)
providers/azure/provider.goaccountsMu sync.RWMutex/cachedAccounts/accountsGen/accountsSFfieldsgetOrFetchAccounts(single-flight cache, returns a defensive copy viacloneAccountsso a caller mutating the returned slice can't corrupt the cache) andfetchAccounts(the actual ARM call, extracted from the previousGetAccountsbody)GetAccountsdelegates togetOrFetchAccounts— identical external behavior, now cache-backedInvalidateAccountsCache()for tests/long-lived callers that need a forced refreshGetRecommendationsClient: pinned subscription unchanged; unpinned path resolves accounts via the cache, validates an explicitly configuredAZURE_SUBSCRIPTION_IDagainst the discovered list before any default resolution, and returnsMultiSubscriptionRecommendationsClientonly when the scope is genuinely ambiguousSetCredential/SetSubscriptionsClientpublish the swapped field underaccountsMuand invalidate in the same critical section;fetchAccountssnapshots both fields underRLockproviders/azure/recommendations_multi_subscription.go(new)MultiSubscriptionRecommendationsClientimplementingprovider.RecommendationsClientNewMultiSubscriptionRecommendationsClientrejects an empty account list and fails loud if building any per-subscription client errorsGetRecommendationsfans out via errgroup; each subscription's error is isolated (logged, doesn't cancel siblings); explicit post-Waitctx.Err()check propagates parent-context cancellation; if every subscription fails, returns a wrapped error instead of a silently empty resultPartialSubscriptionFailureErrorreturns the successful subscriptions' data alongside a typed error naming what was missed, so an incomplete sweep is distinguishable from a complete one that found nothingselectSubscriptionshonoursAccountFilter, and reports filter entries that match no accessible subscription (ErrSubscriptionNotAccessible) rather than silently narrowing the sweepGetRecommendations; adding a bound at this layer for callers that supply no semaphore is tracked in fix(azure): bound concurrency in the multi-subscription recommendations fan-out #1653Tests —
providers/azure/provider_test.go,providers/azure/recommendations_multi_subscription_test.goAZURE_SUBSCRIPTION_IDerrors)Known follow-ups (filed, not fixed here)
accountsis not deduped by subscription ID, so a duplicate would double-count that subscription's savings (unverified whether ARM can emit duplicates)Test plan
go build ./...go vet ./providers/azure/... ./internal/scheduler/... ./cmd/go test -race -count=1 ./providers/azure/...go test -race -count=1 ./internal/scheduler/... ./cmd/...golangci-lint run --timeout=10mat the CI-pinnedv2.10.1— 0 issuesgocyclo -over 10onproviders/azure internal/scheduler cmd— cleangofmt -l/goimports -lclean on touched filesGetRecommendationsClientwithout pinning a subscription — verify recommendations from both subscriptions are returned. Not run: no such path exists in the deployed product yet (see the no-op section above), so this is exercised by unit tests only.Closes #553
Refs #473, #561, #1652, #1653, #1654
Summary by CodeRabbit