[cristim]

Closes #1743

Classify Azure Advisor partial-skip results by the typed SkipKinds() from cloud-commitments-go (go#681) instead of matching reason-text prefixes. Only SkipPriceLookupUnavailable holds stale rows; SkipSourceIncomplete (truncated Advisor walk) does not.

Missing or short kinds read as unpriceable; extra kinds are ignored (clamped to the skip count).

Verification:

• scheduler unit and integration (outage, hold-carry) suites pass locally
• the new kind-based tests fail against main's prefix implementation
• 3 mutation probes killed; golangci-lint --new-from-rev=origin/main: 0 issues
• 2 staged-diff review passes by the same cold reviewer: clean

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 48 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: refactor/scheduler-skipkinds
• File: .coderabbit.yaml
• Commit: 677a62a
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: cbffd281-d0ec-455f-b00b-8ade5e6c10aa

Commits

Reviewing files that changed from the base of the PR and between 676cfe8 and 677a62a.

Files selected for processing: 5

• internal/scheduler/advisor_pricing_outage_integration_test.go
• internal/scheduler/azure_hold_carry_integration_test.go
• internal/scheduler/partial_skip_test.go
• internal/scheduler/partial_sweep.go
• internal/scheduler/partial_sweep_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVE @ 677a62a by claude-nibblets

Cold review of #1885 (closes #1743, +130/-83, scheduler). It swaps the partial-sweep "hold the previous cycle's rows" decision from reason-text prefixes to the typed SkipKinds(). One wrong direction here means stale recommendations kept forever, or rows evicted during a pricing outage, so I checked both ways.

Read (internal/scheduler/partial_sweep.go): only SkipPriceLookupUnavailable counts as an outage (countPricingOutageSkips); a truncated walk (SkipSourceIncomplete) and an unusable row (SkipUnpriceable) do not hold, which is the "always-truncated walk would keep stale rows forever" case the old code excluded by prefix. Kinds are index-aligned with the skips, extra ones are clamped to the count, and missing or short ones read as unpriceable (no hold). A provider without SkipKinds (GCP, via the skipKinder interface) reads every skip as unpriceable. Both the single-subscription path (tolerateSkippedRecommendations) and the multi-subscription path (gapFromPartialSubscriptions) read the kinds, so the old trimSubscriptionPrefixes workaround is gone.

Checked against the library: the pinned providers/azure (52d66feb616b, on main) assigns SkipPriceLookupUnavailable when the error is a failed lookup (IsCollectionFailure) and SkipUnpriceable otherwise, and SkipSourceIncomplete only for the truncated Advisor walk (recommendations.go:503), so the first failed row (the old "advisor: azure advisor:" wrapper) and later rows both get the outage kind, matching what the removed prefixes matched.

Scoped probes (go test -run Partial|Skip|Sweep|Hold|Outage|Advisor|Kind in internal/scheduler, scratch worktree at this head; unit and the integration Outage/HoldCarry baselines pass), each killed: truncated walk holding rows (TestCountPricingOutageSkips); unpriceable row holding rows (TestCountPricingOutageSkips, TestGapFromPartialSubscriptions_HoldReadsTheKindNotTheText, TestTolerateSkipped_HoldReadsTheKindNotTheText); extra kinds not clamped (TestTolerateSkipped_HoldReadsTheKindNotTheText); single-subscription kinds ignored (four tests incl. TestTolerateSkipped_PricingOutageHoldsEviction); multi-subscription kinds ignored (TestGapFromPartialSubscriptions_HoldReadsTheKindNotTheText and _PricingOutageSkipsHoldAccount).

Non-blocking: the failure direction for a library that returns fewer kinds than skips is "no hold", i.e. rows can be evicted during a real outage; that is safe only because the pin carries kinds for every Azure skip. A future downgrade of the azure pin, or a new skip path that forgets a kind, would silently stop holding; the tests pin the reading but not the pin.

Main has one commit this branch lacks (#1872, internal/purchase) and nothing in internal/scheduler or go.mod, so the branch is not in conflict with it. CI at this head: Unit Tests, Lint Code and Run pre-commit hooks pass (34 pass, 4 pending when I looked).