feat(azure): org-wide multi-subscription recommendation collection (closes #553) (#1520)

* feat(azure): org-wide multi-subscription recommendation collection

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

* fix(azure): singleflight the accounts cache instead of locking across 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.

* fix(azure): keep recommendation scope pinned and fix cache invalidation

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

* fix(scheduler): reject Azure accounts with no subscription ID

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.

* fix(azure): drop the accounts cache when the credential is swapped

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.

* fix(azure): validate AZURE_SUBSCRIPTION_ID before default resolution

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.

* fix(azure): report partial multi-subscription sweeps as partial

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.

* fix(scheduler): keep partial multi-subscription sweep data

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.

* chore(scheduler): use US spelling of "behavior" in two comments

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.

* fix(azure): report filter misses and lock credential swaps in fan-out

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

* fix(azure): validate configured subscription on the regions/service path

Addresses both CodeRabbit findings from the review round on PR #1520.
Both harden invariants that the previous commit established on one path
and left off the others, which reads as a global guarantee but is not.

1. Silent subscription retarget (providers/azure/provider.go)

resolveSubscriptionIDFromCtx -- which backs GetRegions and
GetServiceClient -- called getDefaultSubscriptionID without first
validating an explicitly configured subscription against the discovered
list. resolveDefaultSubscription deliberately falls through to its
single-visible-subscription rule when the configured target is not
found, so with an unreachable AZURE_SUBSCRIPTION_ID (or ProviderConfig
subscription) and exactly one visible subscription, the request was
silently retargeted to whichever subscription happened to be visible.
GetRecommendationsClient already guarded this via accountsContain under
a comment reading "This ordering is load-bearing"; the same
misconfiguration therefore errored on the recommendations path and
silently retargeted on the regions/service path.

GetServiceClient drives the resource enumeration recommendations are
computed from, and is also the purchase-execution path's service-client
lookup, so the operator could believe they were scoped to subscription A
while data came from B.

The guard is extracted into validateConfiguredSubscription and reused by
GetRecommendationsClient, so the two paths cannot drift apart again. It
covers both configuration axes (ProviderConfig field and the env var),
mirroring resolveDefaultSubscription's own priority order. Production
Azure flows pin AzureSubscriptionID (scheduler, registration and account
handlers) and short-circuit before this path, so the guard engages only
on the unpinned/ambient path where the retarget lived.

GetRegions' error wrapper no longer claims "no Azure subscriptions
found": the new inner error fires precisely when subscriptions were
found but the configured one is not among them.

2. Unsynchronized credProvider (providers/azure/provider.go)

SetCredentialProvider wrote p.credProvider outside accountsMu while
IsConfigured read it inside credOnce.Do -- the one swappable field left
outside the locking discipline the rest of the provider follows. Latent
today (SetCredentialProvider has no production callers), but a half-kept
invariant reads as a kept one. Published and read under accountsMu now,
matching cred and subscriptionsClient. It deliberately does not
invalidate the accounts cache: credProvider only feeds the lazy
resolution that runs when no credential was installed at all, so no
subscription list resolved under a different credential can exist yet.

Regression tests, both confirmed failing against the pre-fix code:

- TestAzureProvider_ResolveSubscription_InvisibleConfiguredTargetErrors
  uses an invisible configured target with exactly ONE visible
  subscription -- the case the fall-through hides. With two or more
  visible subscriptions getDefaultSubscriptionID returns "" and the
  pre-existing "multiple Azure subscriptions" error fires either way, so
  such a test would pass with or without the guard. Covers GetRegions
  and GetServiceClient, both configuration axes, and the honoured
  visible-target case. Pre-fix: 3 of 4 subtests fail with "An error is
  expected but got nil".
- TestAzureProvider_ConcurrentCredentialProviderSwapAndIsConfigured_NoDataRace
  loops over fresh providers because credOnce makes the read one-shot
  per instance. Pre-fix: WARNING: DATA RACE, write at provider.go:187
  against read at provider.go:280.

TestAzureProvider_GetRegions and
TestAzureProvider_GetServiceClient_WithSubscriptionLookup now clear
AZURE_SUBSCRIPTION_ID via the existing helper: their fixtures resolve
through the newly-guarded path, so an exported value on a developer
machine would fail them for an unrelated reason.

Author: cristim

cmd/multi_service_helpers.go

@@ -11,6 +11,7 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/common"
 	"github.com/LeanerCloud/CUDly/pkg/provider"
 	"github.com/LeanerCloud/CUDly/providers/aws/recommendations"
+	azureprovider "github.com/LeanerCloud/CUDly/providers/azure"
 	"github.com/aws/aws-sdk-go-v2/aws"
 	awsec2 "github.com/aws/aws-sdk-go-v2/service/ec2"
 )
@@ -99,7 +100,13 @@ func getAllAWSRegionsWithClient(ctx context.Context, ec2Client EC2ClientInterfac
 // discoverRegionsForService discovers regions that have recommendations for a specific service.
 func discoverRegionsForService(ctx context.Context, client provider.RecommendationsClient, service common.ServiceType) ([]string, error) {
 	recs, err := client.GetRecommendationsForService(ctx, service)
-	if err != nil {
+	if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil {
+		// Region discovery is best-effort: the subscriptions that answered
+		// still tell us where to look. Report the gap rather than dropping
+		// the discovered regions or failing outright.
+		AppLogger.Printf("  ⚠️  Region discovery incomplete: %d of %d Azure subscriptions succeeded\n",
+			partial.Succeeded, partial.Attempted)
+	} else if err != nil {
 		return nil, err
 	}
 
@@ -439,6 +446,18 @@ func fetchRecommendationsForRegion(
 	}
 
 	recs, err := recClient.GetRecommendations(ctx, &params)
+	if partial := azureprovider.AsPartialSubscriptionFailure(err); partial != nil {
+		// Keep the subscriptions that did answer, but say plainly that the
+		// sweep was incomplete: without this the operator would read a short
+		// list as "little to buy here" rather than "some subscriptions were
+		// never queried".
+		AppLogger.Printf("  ⚠️  Incomplete: %d of %d Azure subscriptions succeeded; %d could not be queried\n",
+			partial.Succeeded, partial.Attempted, len(partial.Failed))
+		for _, f := range partial.Failed {
+			AppLogger.Printf("      subscription %s: %v\n", f.SubscriptionID, f.Err)
+		}
+		return recs
+	}
 	if err != nil {
 		AppLogger.Printf("  ❌ Failed to fetch recommendations: %v\n", err)
 		return nil