fix(scheduler): record partial multi-subscription sweeps in collection state

[IAmJSD]

Root cause

tolerateIncompleteSweep (internal/scheduler/scheduler.go) kept the rows of a partial multi-subscription sweep and withheld stale-row eviction, but it recorded the incompleteness only with logging.Warnf and returned a bare complete bool. Nothing reached recommendations_state, so GET /api/recommendations/freshness (which the dashboard reads) showed a sweep that covered 1 of 10 subscriptions exactly like full coverage. The code comment at the old scheduler.go:1007-1012 described this as follow-up work.

Fix

• tolerateIncompleteSweep now returns (*sweepGap, error). A nil gap means the sweep was complete. A non-nil gap is a sanitized summary built from PartialSubscriptionFailureError (Azure/GCP) or IncompleteRecommendationsError (AWS). Eviction semantics are unchanged: gap != nil keeps the account out of the eviction roster, exactly as !complete did.
• The gap is passed up through the per-account and ambient collectors, fanOutPerAccount (new accountOutcome.Gap, merged across incomplete accounts), the three provider collectors and collectAllProviders. That produces a *config.RecommendationsCollectionPartial with one entry per provider that had an incomplete sweep.
• persistCollection records it with the new SetRecommendationsCollectionPartial, after UpsertRecommendations, the same way last_collection_error is recorded.
• It clears on the next complete sweep. UpsertRecommendations and ReplaceRecommendations now also set last_collection_partial = NULL, and the scheduler only writes it again when the collection was partial.
• Total failure behavior is unchanged. A hard error still fails the provider and goes into last_collection_error, and is never reported as partial. The new tests check this.

New file internal/scheduler/partial_sweep.go holds the gap type and the cause classifier.

API shape

GET /api/recommendations/freshness (and the freshness returned by the refresh handler) gains last_collection_partial. The key is always present: null when the last persisted collection was complete.

"last_collection_partial": {
  "providers": [
    {
      "provider": "azure",
      "incomplete_accounts": 1,
      "attempted_scopes": 10,
      "failed_scopes": 9,
      "failed_details": 0,
      "reasons": { "permission_denied": 7, "throttled": 2 }
    }
  ]
}

• A "scope" is an Azure subscription, a GCP project, or an AWS service/region collection scope. attempted_scopes is 0 for AWS because AWS does not report it. failed_details only applies to AWS.
• reasons keys come from a closed set: permission_denied, throttled, timeout, canceled, not_found, provider_unavailable, other. They are derived from azcore/googleapi/gRPC/smithy error codes and statuses.
• No raw provider error text and no subscription/project IDs go into the response. recommendations_state is a single global row that every view:recommendations user reads, while accounts are access-scoped, so naming failed subscriptions there would leak their existence to users who are not authorized for them. The failed IDs and raw errors stay in the existing WARN log line. This is a deliberate departure from the issue's "failed subscription IDs" bullet. If the maintainer wants IDs surfaced, it needs a per-viewer, scope-filtered endpoint, which is out of scope here.
• openapi.yaml: the freshness path was not documented at all. This PR adds /api/recommendations/freshness plus the RecommendationsFreshness, RecommendationsCollectionPartial and PartialProviderSweep schemas (with 401/403).

Migration notes

• 000128_recommendations_state_partial: ALTER TABLE recommendations_state ADD COLUMN IF NOT EXISTS last_collection_partial JSONB (nullable, so existing deployments read as complete). The down migration drops it.
• 000128 was chosen because main's highest is 000126 and open PR fix(config): let an account with only terminal executions be deleted #1020 adds 000127. If this merges before fix(config): let an account with only terminal executions be deleted #1020, fix(config): let an account with only terminal executions be deleted #1020's 000127 would sit below an already-applied 128 and golang-migrate would not apply it to databases already at 128. Merge fix(config): let an account with only terminal executions be deleted #1020 first, or renumber whichever lands second.
• Migration test 000128_recommendations_state_partial_test.go uses previousMigrationVersion, so it does not depend on 127 existing.

Frontend follow-up (not in this PR; frontend/ untouched)

• In frontend/src/recommendations.ts, render a "Partial collection" banner or badge when freshness.last_collection_partial is non-null. Example copy: "Azure: 9 of 10 subscriptions could not be queried (permission denied ×7, throttled ×2). Savings shown may be understated." Use attempted_scopes when it is > 0; otherwise show failed_scopes alone (AWS).
• Keep it visually distinct from the last_collection_error banner. Partial data is still valid, and the post-refresh path at recommendations.ts:445 currently treats last_collection_error as "refresh failed". That is the main reason this is a separate field rather than text in last_collection_error.
• Add the field to the freshness type and the test fixtures (last_collection_partial: null).

Tests

• Scheduler, scheduled (cron, tokenless) path via CollectRecommendations(ctx, ""):
  • a strict subset of subscriptions fails: the surviving rec is upserted and SetRecommendationsCollectionPartial gets the exact expected summary;
  • a complete sweep never writes the marker;
  • a hard failure still sets last_collection_error and never sets the marker.
• Audit finding A16-007: the partial/complete/hard-error cases now run for AWS, Azure and GCP ambient paths, plus registered-account AWS (self-account) and GCP (ADC). Previously only Azure was covered.
• Unit tests for the classifier, gap merge/forAccount (no input mutation), appendPartialProvider, and a sanitization test that the JSON contains no subscription IDs, tenant IDs, ARNs or raw error text.
• Store (integration, testcontainers): record → survives an error write → cleared by Upsert → re-record → cleared by Replace → nil clears. Also a unit test that undecodable JSON reads as nil instead of failing the freshness read.
• API: the freshness handler serializes the field, and serializes it as null when the collection was complete.
• Migration 000128 up/down test.

Verification

All Go tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. No real cloud calls were made.

• go build ./..., go vet ./..., go vet -tags integration ./...: clean.
• Unit tests pass for ./internal/scheduler/, ./internal/config/, ./internal/api/, ./internal/mocks/, ./internal/server/, ./internal/analytics/ and ./internal/database/postgres/migrations/.
• Integration tests pass for the full ./internal/database/postgres/migrations/ package (152s), the full ./internal/scheduler/ package, and ./internal/config/ -run 'Recommendation|Freshness|Collection'.
• golangci-lint v2.10.1 (--allow-parallel-runners) on the touched packages: 0 issues. gocyclo -over 10 on changed non-test files: none.
• git-secrets 1.3.0 (--register-aws, isolated HOME, temp copy with .gitallowed) on all changed files: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• Fail-before: the new tests do not compile against main because the API they use does not exist there. Mutation probes stand in as behavioral fail-before. Each of the following was killed by at least one test:
  • fanOut dropping the gap;
  • the GCP per-account, GCP ambient and AWS fetchAndConvert collectors returning nil;
  • the AWS fallback-merge being dropped;
  • the ambient path skipping forAccount;
  • tolerateIncompleteSweep returning nil for a partial (this is the pre-fix behavior);
  • appendPartialProvider being dropped;
  • persistCollection skipping the write (also the pre-fix behavior);
  • the classifier ignoring error codes;
  • attemptedScopes being zeroed;
  • UpsertRecommendations not clearing the column (killed by the integration test).
• Not verified: a real Azure/GCP/AWS partial sweep against live accounts. The evidence is mocks and fixtures that match the provider error types.

Out of scope

• Frontend rendering (see follow-up above).
• Failed-subscription IDs in the API (see API shape).
• Azure PartialResultError (per-service partials). tolerateIncompleteSweep did not handle it before this PR and still does not.
• The current GCP provider never emits a partial error: per-region failures return nil when at least one call succeeds. The GCP path is wired and tested so it works if it starts to.
• known_issues/ has no entry for fix(scheduler): surface partial multi-subscription sweeps in collection state so the dashboard shows them as incomplete #87, so there is nothing to move.

Closes #87

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review verdict: CLEAN at 1d2c9e2, with a merge-order constraint

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

⚠️ Merge order: the migration runner is golang-migrate (m.Up(), migrations/migrate.go:62). It only applies versions above the DB's current version. Open PR #1020 adds 000127. If this PR (000128) merges first, databases already at 128 would silently skip #1020's 000127. Either merge #1020 first or renumber whichever lands second. The PR body already notes this.

Consumers of the new partial flag. None of them is on a money path:

• handler_recommendations.go:286 (freshness GET) echoes LastCollectionPartial to the UI.
• handler_recommendations_refresh.go:76-177, handler_recommendations.go:406, handler_ri_exchange.go:1718 (reshape banner) and scheduler.go:1191-1552 (cache TTL, cold start) read only LastCollectedAt or LastCollectionStartedAt.
• Auto purchase, ladder, scheduled purchase and RI-exchange auto read neither the flag nor any completeness signal. So partial data cannot newly gate or unblock a purchase.

Stale rows: eviction is unchanged. An incomplete account is kept out of SucceededAccountIDs (scheduler.go:639-645), so its old rows are kept with their old collected_at. That was already the behaviour; the flag now makes it visible.

Migration 000128:

• A nullable JSONB column added with ADD COLUMN IF NOT EXISTS. NULL means complete.
• The down migration drops the column, which is safe because the data is derived.

OpenAPI: the nullable last_collection_partial field is always present and matches the emitted JSON.

Note (non-blocking): persistCollection (scheduler.go:418-437) runs an Upsert (which clears partial) and then a separate Set, so the two writes aren't atomic. If two sweeps overlap, an older sweep's partial can land after a newer complete one. The flag is advisory and no money path reads it, and the PR documents the behaviour.

What was attacked and how:

• go vet ./internal/... is clean. go test -race -count=1 on scheduler, config and api passes 4619.
• -tags integration -v -run 'Recommendations|Partial' on migrations, config and scheduler against Docker Postgres: 149 PASS, 0 SKIP.
• Mutation probes, both killed:
  • Never recording partial fails TestCollectRecommendations_PartialSweepRecordedInState.
  • Counting an incomplete account as succeeded gives 4 failures.

Gate:

• Labels are mirrored from fix(scheduler): surface partial multi-subscription sweeps in collection state so the dashboard shows them as incomplete #87.
• Merge only after fix(config): let an account with only terminal executions be deleted #1020 (or renumber).
• Needs the author to mark the draft ready, then green CI and CodeRabbit.

[IAmJSD]

CI fix for TestApproveAzurePricingRequiresMigrationAndRecollection (new head 474842dc03584de9080b89961d917e7042f8570d).

Root cause: the test pins the schema to 000111 before its final UpsertRecommendations. This PR makes the upsert also reset recommendations_state.last_collection_partial, which only exists from 000128, so the upsert failed with SQLSTATE 42703. Reproduced locally with the same error.

Fix: test-only. The pin moves to 128, following the test's existing 000111 precedent, and the comment now says why. Production semantics are unchanged. I grepped every other MigrateToVersion user outside the migrations package (internal/api/nullable_purchase_cost_integration_test.go pins 100/101; none of them touch the recommendations store) plus the migration tests. 000100_hash_approval_tokens_at_rest_test.go builds a store only after migrating to latest. Nothing else is affected.

Evidence (AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE set to /dev/null; no cloud calls):

• go test -tags integration ./internal/purchase -count=1 passes (all 6 subtests of the failing test now run). The 000100 migration test and the nullable purchase cost integration test also pass.
• go vet -tags integration ./... is clean, and golangci-lint v2.10.1 --build-tags integration reports 0 issues.
• git-secrets is clean (fresh git init temp repo, isolated HOME).
• No conflicts with origin/main.

🤖 Generated with Claude Code

[cristim]

Head moved to 474842d. The reviewed 1d2c9e2 is unchanged (range-diff =). The one added commit, "test(purchase): migrate to 000128 before the Azure pricing recollection upsert", is test-only: internal/purchase/azure_pricing_integration_test.go now migrates the test DB to 128 instead of 111, so the upsert sees the new last_collection_partial column. I reviewed it myself: no production change. The CLEAN verdict carries to 474842d, and the merge-order constraint still stands (#1020 / 000127 first). (claude-cc-reviewer)