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

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:

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.
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.

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

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.

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

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

Correcting my own re-triage: urgency/now was wrong

I raised this to priority/p1 + urgency/now earlier today on the premise that merging #1520 made the path reachable. That premise does not hold, and I am correcting it rather than leaving an inflated label in place.

An independent trace established the defect is real but still latent:

PartialSubscriptionFailureError has exactly one construction site repo-wide (providers/azure/recommendations_multi_subscription.go:374), inside the fan-out client.
The fan-out client is only constructed at providers/azure/provider.go:681 (step 3), reached solely when p.subscriptionID == "" and no AZURE_SUBSCRIPTION_ID target and no default resolves. GetRecommendationsClient returns the single-subscription client immediately when p.subscriptionID != "" (provider.go:641).
Both scheduler Azure paths pin a subscription: collectAzureAmbient is only called when AZURE_SUBSCRIPTION_ID != "" (scheduler.go:751-755 returns early on empty), and collectAzureForAccount hard-errors when acct.AzureSubscriptionID == "" (scheduler.go:782).
The remaining tolerateIncompleteSweep call sites cannot produce the error: 710/873 are GCP, and 935/951 are in fetchAndConvert, whose only three callers pass "aws".
Structurally, a pinned sweep can never be partial: that requires 0 < Succeeded < Attempted, impossible at Attempted == 1.

So #1520 shipped the fan-out client, but nothing on the collection path yet constructs an unpinned Azure provider. There is no live data loss to stop — which is what urgency/now asserted.

The rest of the analysis in my earlier comment stands unchanged: the eviction keyed on (provider, account_key) is genuinely coarser than the subscription-level failure unit, tolerateIncompleteSweep's NOTE genuinely understates the consequence as cosmetic, and the existing tests genuinely encode the hazard as intended behaviour.

Label change: priority/p1 → p2, urgency/now → this-sprint. Keeping it above its original this-quarter for one reason: a fix is in flight and nearly done, and landing the guarantee before the fan-out becomes reachable is far cheaper than retrofitting it afterwards — which is exactly what this issue's own body asks for ("Before (or as part of) the change that makes the fan-out reachable, decide explicitly how a partial sweep interacts with persistence").

Also correcting a fix suggestion I made. I proposed carrying the successfully-collected subscription set into the eviction predicate. That is not expressible today: recommendations has no subscription column — its identity is (provider, account_key) only (migration 000030, account_key generated from cloud_account_id), and RecommendationRecord carries no subscription. That approach would need a new column, a migration, population on insert, and threading the subscription through the record — a large blast radius on a path that is not yet reachable.

The shape being implemented instead uses a lever the code already documents for this case: keep a partially-swept account out of SucceededAccountIDs (which feeds only the eviction roster) while still counting it succeeded and upserting its recovered rows. Successful subscriptions get fresh data, un-queried ones keep their previous rows, and the account is not marked failed — so no blank dashboard, preserving the invariant SuccessfulCollect exists to protect.

fix(scheduler): withhold stale-row eviction after a partial sweep (#1708

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

Navigation Menu

Repository navigation

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

Summary

The hazard

Suggested direction

Provenance

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

The premise that no longer holds

The consequence is data loss, not a display artefact

Why the tests will not catch this

Label change

Correcting my own re-triage: urgency/now was wrong

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Summary

The hazard

Suggested direction

Provenance

Activity

cristim commented on Aug 3, 2026

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

The premise that no longer holds

The consequence is data loss, not a display artefact

Why the tests will not catch this

Label change

cristim commented on Aug 3, 2026

Correcting my own re-triage: urgency/now was wrong

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions