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

[cristim]

Closes #1652

The defect

tolerateIncompleteSweep (internal/scheduler/scheduler.go) converted an Azure *PartialSubscriptionFailureError into a nil error so the subscriptions that did answer keep their data. That part is right. But it discarded the fact that the sweep was incomplete, and the account then landed in the succeeded-account roster that authorizes stale-row eviction.

Eviction is account-granular:

DELETE FROM recommendations
 WHERE collected_at < $1
   AND (provider, account_key) IN (…)

keyed on account_key, the CUDly account UUID. A sweep, however, fails one subscription at a time. A sweep that never queried subscription B returns no rows for B, so listing the account as succeeded deleted B's previous-cycle rows — its savings vanished and read as "nothing to buy" rather than "collection incomplete". That is the COR-03 hazard mergeServiceResults already warns about, landing on a money-adjacent table.

Reachability — please read before prioritising

This is not live today. The fan-out client is only built at providers/azure/provider.go step 3, which requires no subscription to be named, and both scheduler Azure paths pin one:

• collectAzureAmbient only runs when AZURE_SUBSCRIPTION_ID != "" (the caller returns early on empty) and passes it through;
• collectAzureForAccount hard-errors on an empty azure_subscription_id and passes it as Profile.

The other tolerateIncompleteSweep call sites cannot produce the error at all: two are GCP, two are in fetchAndConvert, whose only three callers pass "aws". PartialSubscriptionFailureError has exactly one construction site repo-wide, inside the fan-out client. A pinned sweep also cannot produce it structurally: partial requires 0 < Succeeded < Attempted, impossible at Attempted == 1.

So this lands the guarantee before the fan-out becomes reachable, which is the cheap ordering and what the issue itself asks for.

The fix

tolerateIncompleteSweep now returns (complete bool, err error). The flag threads through the per-provider collect functions into fanOutPerAccount, which counts a partially-swept account in SucceededCount but keeps it out of SucceededAccountIDs (recording it in a new IncompleteAccountIDs). Ambient paths route through a new ambientEvictionRoster(complete, accountID) that yields an empty roster when incomplete.

The narrowness is the point. Rows that were collected are still upserted by natural key, so the subscriptions that answered get fresh data, and the account is not marked failed — failing it would blank a whole account's dashboard over one flaky subscription, the opposite failure and the one the succeeded-account roster exists to prevent. Only the authority to DELETE is withheld. This reuses semantics UpsertRecommendations already documents for failed collections: "their stale rows stay — callers see older data with a banner rather than a blank section." Downstream needed no change: the eviction is already guarded by if len(successfulCollects) > 0.

Why not scope eviction to the queried subscriptions

That was the first shape considered, and it is not expressible today: the recommendations table has no subscription column — its identity is (provider, account_key) (migration 000030) and RecommendationRecord carries only CloudAccountID. It would need a new column, a migration, and the subscription threaded through the record, and would still need this same signal plumbed the same way. Larger blast radius, same prerequisite, for a path not yet reachable.

One subtlety

fetchAndConvert has a zero-results retry that re-queries the same scope. Completeness is combined with complete = complete && fallbackComplete rather than assigned, so a retry that happens to come back whole cannot launder away the first sweep's missing subscriptions and re-authorize eviction.

Tests

The existing account-level tests could not see this: they assert that a collection returning an error is excluded, while a partial sweep returns data plus an error — the one shape that is neither a clean success nor a failure. The new tests drive that sub-account granularity.

TestCollectAzure_PartialSweepIsNotEvictionEligible fails on pre-fix main:

=== RUN   TestCollectAzure_PartialSweepIsNotEvictionEligible
[WARN] azure recommendations incomplete: 1 of 3 subscriptions succeeded; not queried: sub-b, sub-c
    Error:    Should be empty, but was []
    Messages: a partial sweep must not enter the eviction roster, got []string{""}: 2 subscription(s)
              were never queried, so their previous-cycle rows would be deleted by the
              (provider, account_key) DELETE
--- FAIL: TestCollectAzure_PartialSweepIsNotEvictionEligible

It deliberately drives the ambient path, whose signature this PR does not change, so it compiles and fails against unmodified main. A fanOutPerAccount-level test could not demonstrate the pre-fix failure — this PR changes that function's fn signature, so such a test would fail to compile on main, proving nothing. One is included anyway (TestFanOutPerAccount_PartialSweepNotInSucceededAccountIDs) as post-fix coverage of the registered-account path, which is the common production shape.

Controls pin both other directions, since under-evicting is a quieter bug than over-evicting: a complete sweep must stay eviction-eligible (or stale rows accumulate forever), and a hard error must still fail the account.

TestTolerateIncompleteSweep asserted only NoError on a partial sweep — it encoded this hazard as intended behaviour. It now pins the completeness half of the contract too.

The store-layer half of the chain was already covered by TestPostgresStore_UpsertRecommendations_PartialCollect ("not in roster → rows preserved"), so the guarantee is now tested end to end.

Comment corrected

The function's NOTE framed the discarded signal as cosmetic — "so the dashboard shows the sweep as partial". The same signal fed the eviction roster, so it was data loss, not display. Rewritten to say so, since a future reader would otherwise trust it.

Deliberately out of scope

• last_collection_error is still cleared on a partial sweep. It is a single global row (recommendations_state WHERE id = 1), not per-account, so carrying one account's partial sweep in it is a schema/UX decision rather than a mechanical fix; with eviction withheld the residue is observability, not lost rows. Documented at the source — worth a follow-up issue.
• ReplaceRecommendations does DELETE FROM recommendations with no scoping at all, so a partial sweep through it would wipe every account. It has no production caller (the only non-test reference is the interface declaration), so it is a latent landmine rather than a live path. Flagged, not touched.

Verification

• go build ./... exit 0; go vet ./... exit 0 — vet is what caught the stale test call sites go build skips.
• go test ./... exit 0 (28 ok, 0 fail).
• -race on internal/scheduler, internal/config, providers/azure (+ all 11 azure sub-packages): exit 0, 0 data races.
• golangci-lint at the CI-pinned v2.10.1: baseline on main 0 issues / exit 0; branch 0 issues / exit 0. (An interim run surfaced 9 issues — 8 British spellings misspell enforces as US, plus one unparam — fixed by hand rather than --fix.)
• gocyclo -over 10, CI's exact invocation: exit 0. Touched functions max out at 9 (fetchAndConvert).
• Full pre-commit hook suite passed; no --no-verify.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a041f6c4-4a1c-4394-890a-d99a5cb3922b

📥 Commits

Reviewing files that changed from the base of the PR and between 34d85bc and c67c3d6.

📒 Files selected for processing (3)

• internal/scheduler/partial_sweep_eviction_test.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_test.go

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Adversarial review — verdict: safe to merge

Independent review with fresh context, against a clean detached extract of c67c3d670a4a219db59123d5fd20438965792bdd (base 34d85bc1). Note CodeRabbit reports "Review skipped" on this PR, so this is a substitute for that pass, not a confirmation of it.

Everything below was executed. Commands and output are quoted.

---

The hazard I went looking for first, and did not find

The fix works by returning an empty eviction roster for a partial sweep. That is only safe if an empty roster means "evict nothing" and not "evict unscoped". UpsertRecommendations (internal/config/store_postgres_recommendations.go:92) guards the DELETE:

if len(successfulCollects) > 0 {
        ... DELETE FROM recommendations WHERE collected_at < $1 AND (provider, account_key) IN (...)
}

An empty roster skips the statement entirely. There is no unscoped-delete fallback. This also covers the all-accounts-partial case on the registered path, where SucceededAccountIDs comes back nil while SucceededCount > 0 (so errAllAccountsFailed correctly does not fire).

1. Withheld in every partial case, and only in partial cases

tolerateIncompleteSweep now returns (complete bool, error) over three exhaustive branches — err == nil → (true, nil); non-partial error → (false, err); partial → (false, nil) after logging. All 6 production call sites capture and propagate it (scheduler.go:725, 745, 839, 908, 997, 1013); none discards. All 3 fanOutPerAccount call sites (AWS closure, collectAzureForAccount, collectGCPForAccount) carry the new 3-value signature, and the AWS closure forwards all three returns.

ambientEvictionRoster(true, id) returns []string{id}, byte-identical to the pre-fix []string{id} — so a complete sweep still evicts. That direction is pinned by TestCollectAzure_CompleteSweepStaysEvictionEligible and, on the registered path, by assert.Equal(t, []string{"acct-complete"}, outcome.SucceededAccountIDs), which pins inclusion and exclusion in one assertion.

3. The pre-fix test genuinely fails, and the labelling is honest

Copied the new test file onto unmodified main (34d85bc1), removed only the fanOutPerAccount case, and ran it:

=== RUN   TestCollectAzure_PartialSweepIsNotEvictionEligible
    Error:    Should be empty, but was []
    Messages: a partial sweep must not enter the eviction roster, got []string{""}: 2 subscription(s)
              were never queried, so their previous-cycle rows would be deleted by the
              (provider, account_key) DELETE
--- FAIL: TestCollectAzure_PartialSweepIsNotEvictionEligible
--- PASS: TestCollectAzure_CompleteSweepStaysEvictionEligible
--- PASS: TestCollectAzure_HardSweepErrorStillFailsTheAccount
FAIL    github.com/LeanerCloud/CUDly/internal/scheduler

Exactly one test flips, and it is the one that describes the bug — []string{""} as reported. The other two pass on both sides, which is correct: one is a control against over-correcting, the other a non-regression pin.

The caveat in the PR description is accurate, and I verified it rather than taking it: the full file cannot compile against main —

vet: internal/scheduler/partial_sweep_eviction_test.go:159:77: cannot use fn (variable of type func(_ context.Context...

— because the fix changes fanOutPerAccount's fn signature. So that test genuinely cannot demonstrate a pre-fix failure, and it is not presented as if it could.

6. TestTolerateIncompleteSweep is a real contract pin

Before, the partial case asserted only assert.NoError(...) — which encoded the hazard as intended behaviour. The rewrite pins all three completeness branches (False on partial, False on hard error, True on nil). Not an assertion that passes either way.

4 & 5. Field consumers

SucceededAccountIDs is read in production at exactly three sites — scheduler.go:501, 806, 879 — all of them the return recs, outcome.SucceededAccountIDs, nil that feeds the eviction roster. Nothing else consumes it, so withholding an account has no second effect.

IncompleteAccountIDs is written but never read in production code; its only reader is the new test's assertion. It is currently inert — a future foothold for surfacing partial sweeps in recommendations_state, which the code comment names as follow-up.

---

One finding: the && combination is correct but untested, and currently unreachable in its dangerous form

complete = complete && fallbackComplete (fetchAndConvert) is the right operator, and the comment above it correctly identifies that assigning would let a zero-results retry launder away the first sweep's missing subscriptions.

I mutated it to the exact regression the comment warns about and re-ran the package:

MUTATED: '&&' -> plain assignment
ok  github.com/LeanerCloud/CUDly/internal/scheduler  224.009s     # mutant SURVIVES

The whole suite stays green, so that line has no coverage. The reason is structural rather than an oversight: fetchAndConvert's only callers are AWS (scheduler.go:643, 759, 770), while tolerateIncompleteSweep detects only *azureprovider.PartialSubscriptionFailureError, whose single construction site is providers/azure/recommendations_multi_subscription.go. On the AWS path complete is therefore always err == nil, and any non-nil error returns before the fallback — so today complete && fallbackComplete is equivalent to fallbackComplete, and no test can distinguish them through the current wiring.

Not blocking: the code is correct, and the line is defensive against the day fetchAndConvert is reused for a provider that can report partials. Worth a direct unit test on fetchAndConvert (partial first sweep + zero rows + complete retry) so a future edit to = cannot ship silently.

Verified separately

ReplaceRecommendations (unscoped DELETE FROM recommendations, filed as LeanerCloud/cloud-commitments-platform#159) has no production caller — confirmed. Every reference is the interface declaration (internal/config/interfaces.go:342), the implementation, three mocks, or a test. It stays latent, not live.

AsPartialSubscriptionFailure uses errors.As, so it is wrapping-safe; and if a wrap ever did defeat it, the result is (false, err) — the account fails and is excluded from the roster. Fail-closed either way.

Test / CI state

• Extract: go build ./... clean; go test ./internal/scheduler/ → ok ... 167.217s.
• CI on this head: Lint Code, pre-commit, E2E, Docker, all Terraform validates, both IAM/role parity jobs, GCP secret scope, Snyk — all green. Unit / Integration / Security Scanning were still running when I checked; I did not treat them as passing.

Could not verify

Whether the un-queried subscriptions' rows survive end-to-end against a real Postgres — the assertions here are on the roster, and the DELETE guard is verified by reading UpsertRecommendations plus its existing account-scoped-eviction tests, not by driving scheduler → store in one integration test.

---

Bottom line: safe to merge. The fix withholds eviction in exactly the partial case, leaves the complete case evicting as before, and cannot degrade into an unscoped delete. The single follow-up worth having is a test for the && line.