fix(scheduler): partial Azure sweep counts as success and evicts un-queried subscriptions' rows

[cristim]

Summary

tolerateIncompleteSweep (internal/scheduler/scheduler.go:906-925) converts an Azure *azure.PartialSubscriptionFailureError into scheduler-level success. Today that is a no-op at all five call sites (every Azure path pins a subscription, and GCP/AWS never return that error type), so this is latent — it becomes live the moment the org-wide fan-out from #1520 is actually wired up.

The hazard

Once the fan-out is reachable, an account whose sweep missed N subscriptions is recorded as succeeded. Two consequences follow from treating a partial sweep as a complete one:

1. Row eviction. The collection path replaces the account's stored recommendations with what the sweep returned. A sweep that never queried subscriptions X and Y returns no rows for them, so previously-collected rows for X and Y are deleted — the savings for those subscriptions silently vanish from the UI and from any downstream aggregate.
2. last_collection_error is cleared, so nothing on the account records that the sweep was incomplete.

This is exactly the COR-03 hazard that mergeServiceResults' own comment warns about (providers/azure/recommendations.go:276-280): a partial result persisted as if it were complete reads as a shrinking savings opportunity rather than a failed collection.

tolerateIncompleteSweep's NOTE mentions the last_collection_error consequence but not the eviction one, which is the more damaging of the two.

Suggested direction

Before (or as part of) the change that makes the fan-out reachable, decide explicitly how a partial sweep interacts with persistence. Options, roughly in increasing order of effort:

• Record the partial-failure detail on the account (a dedicated column, or keep last_collection_error populated) instead of clearing it, so the incompleteness survives into the data model.
• Make the persistence step merge rather than replace when the sweep is known-partial, so un-queried subscriptions keep their previous rows.
• Scope the replace to the subscriptions that were actually queried (PartialSubscriptionFailureError already carries Failed / FailedSubscriptionIDs(), so the queried set is derivable).

Whichever is chosen, extend tolerateIncompleteSweep's NOTE to state the eviction consequence, not only the last_collection_error one.

Provenance

Found during the adversarial review of #1520. Deliberately not fixed in that PR: the code path is unreachable today, and the fix is a persistence-semantics decision rather than a defect in the fan-out client.

[cristim]

Re-triage: p2 → p1, urgency this-quarter → now

An adversarial review of PR #1520 traced this path end to end against the actual SQL and found that this issue's "unreachable today" premise does not hold once #1520 merges, and that its stated consequence understates the damage.

The premise that no longer holds

This issue records the path as unreachable because "every Azure path pins a subscription". That is true only of the ambient path — collectAzureRecommendations guards it with if subscriptionID == "" { skip }, so ambient never fans out.

But collectAzureForAccount passes Profile: acct.AzureSubscriptionID, and resolveAzureSubscriptionID is AzureSubscriptionID → Profile with no env fallback. azure_subscription_id is VARCHAR(36) nullable (000011_cloud_accounts.up.sql:26), and the create path passes it through without a non-empty validation (handler_accounts.go:506; :296 fills it from onboarding, so it is usually set but not guaranteed).

So an enabled Azure account row with an empty subscription ID, AZURE_SUBSCRIPTION_ID unset, and ≥2 visible subscriptions does reach the multi-subscription fan-out from the scheduler. Unusual configuration, not an unreachable one. Via the CLI it is reachable unconditionally.

Caveat, stated rather than glossed: the reviewer did not exhaustively prove no non-empty validation exists elsewhere on the create path.

The consequence is data loss, not a display artefact

tolerateIncompleteSweep (scheduler.go:906) logs a partial failure and returns nil, so the account lands in succeededAccountIDs. All six call sites route through it — verified 6/6, none missed. UpsertRecommendations (internal/config/store_postgres_recommendations.go:78) then runs:

DELETE FROM recommendations
 WHERE collected_at < $1
   AND (provider, account_key) IN (SELECT p, a FROM unnest($2::text[], $3::uuid[]))

plus UPDATE recommendations_state SET last_collection_error = NULL.

Eviction is keyed on (provider, account_key) — the CUDly account UUID, not the Azure subscription. Before #1520, one Azure account row ≈ one subscription, so SuccessfulCollect's invariant held: a partially-failed provider preserved the failed accounts' previous-cycle rows. #1520 introduces a failure unit — the subscription — finer than the eviction guard's key. A sweep that misses subscription B still marks the account succeeded, so B's previous-cycle rows are deleted and the error is cleared. B's savings opportunities silently vanish and read as "nothing to buy".

That is silent, money-adjacent data loss on a savings-opportunity table, not the cosmetic dashboard effect the code's own NOTE describes.

Why the tests will not catch this

TestTolerateIncompleteSweep asserts NoError on a partial sweep — it encodes the hazard as intended behaviour. The account-level eviction tests at scheduler_test.go:1190+ assert the old invariant and still pass, because the breakage is at a sub-account granularity they never exercise.

Whoever fixes this needs a test at subscription granularity, or the fix merges green without demonstrating anything.

Label change

priority/p2 → p1, urgency/this-quarter → now. Severity and impact left as-is pending the fix author's own assessment.

Leaving the original text intact; this comment is the correction of record.