refactor(scheduler): classify Advisor skips by SkipKinds, not reason prefixes

internal/scheduler/advisor_pricing_outage_integration_test.go

@@ -16,7 +16,10 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// Skip reasons as the go#663 Advisor pricing records them (see pricingOutageReasons).
+// The kind the library records for each skip below: a failed price lookup.
+var advisorOutageKinds = []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable, azureprovider.SkipPriceLookupUnavailable}
+
+// Skip reasons as the go#663 Advisor pricing records them.
 var advisorOutageSkips = []string{
 	`advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`,
 	`advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`,
@@ -76,13 +79,13 @@ func TestAzureAdvisorPricingOutage_HoldsEviction(t *testing.T) {
 
 	t.Run("outage keeps prior rows", func(t *testing.T) {
 		store := newStore(t)
-		collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips})
+		collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds})
 		require.Contains(t, storedAzureIDs(t, store), "prev-advisor-row")
 	})
 
 	t.Run("later good cycle replaces them", func(t *testing.T) {
 		store := newStore(t)
-		collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips})
+		collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds})
 		got := collectAzureAmbientCycle(t, store, []common.Recommendation{partialSweepRec()}, nil)
 		require.NotEmpty(t, got)
 		ids := storedAzureIDs(t, store)
@@ -113,7 +116,7 @@ func TestAzureAdvisorPricingOutage_FreshnessListsHeldAccount(t *testing.T) {
 	acct := config.CloudAccount{ID: uuid.NewString(), Name: "host", Provider: "azure", ExternalID: "sub-a", AzureSubscriptionID: "sub-a", Enabled: false}
 	require.NoError(t, store.CreateCloudAccount(ctx, &acct))
 
-	collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips})
+	collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds})
 	fresh, err := store.GetRecommendationsFreshness(ctx)
 	require.NoError(t, err)
 	require.NotNil(t, fresh.LastCollectionPartial)

internal/scheduler/azure_hold_carry_integration_test.go

@@ -162,7 +162,7 @@ func TestAzureHoldCarriedAcrossCycles(t *testing.T) {
 		store := config.NewPostgresStore(container.DB)
 		s := &Scheduler{config: store}
 		seedAdvisorRow(t, store)
-		collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips})
+		collectAzureAmbientCycle(t, store, nil, &azureprovider.PartialResultError{Skipped: advisorOutageSkips, Kinds: advisorOutageKinds})
 		s.persistCollection(ctx, nil, nil, map[string]string{"azure": "boom"}, nil)
 		fresh, err := store.GetRecommendationsFreshness(ctx)
 		require.NoError(t, err)

internal/scheduler/partial_skip_test.go

@@ -21,6 +21,23 @@ type skipErr struct {
 	reasons []string
 }
 
+// kindSkipErr is skipErr that also reports a kind per skip reason, as Azure's
+// PartialResultError does (cloud-commitments-go#670).
+type kindSkipErr struct {
+	skipErr
+	kinds []azureprovider.SkipKind
+}
+
+func (e *kindSkipErr) SkipKinds() []azureprovider.SkipKind { return e.kinds }
+
+// outageSkip is an Advisor skip whose price lookup did not complete, with a
+// reason text that matches no prefix the scheduler ever knew: the hold reads
+// the kind, never the text.
+func outageSkip(reason string) *kindSkipErr {
+	return &kindSkipErr{skipErr: skipErr{partial: true, count: 1, reasons: []string{reason}},
+		kinds: []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable}}
+}
+
 func (e *skipErr) Error() string         { return e.msg }
 func (e *skipErr) Partial() bool         { return e.partial }
 func (e *skipErr) SkippedCount() int     { return e.count }
@@ -126,28 +143,31 @@ func TestAzureFailureWithoutSkips_RecordsNoSkippedCount(t *testing.T) {
 	assert.True(t, gap.holdsEviction())
 }
 
-func TestHasPricingOutageSkip(t *testing.T) {
+func TestCountPricingOutageSkips(t *testing.T) {
+	const (
+		outage     = azureprovider.SkipPriceLookupUnavailable
+		unpriced   = azureprovider.SkipUnpriceable
+		incomplete = azureprovider.SkipSourceIncomplete
+	)
 	for name, tc := range map[string]struct {
-		reasons []string
-		want    bool
+		kinds []azureprovider.SkipKind
+		want  int
 	}{
-		"first failed row":                {[]string{"advisor: azure advisor: advisor compute reservation price: get: retail prices fetch failed"}, true},
-		"later rows":                      {[]string{"advisor: non-USD", "advisor: price lookup unavailable: x"}, true},
-		"invalid recommendation":          {[]string{"advisor: row is in EUR, not USD", "computeengine us-east1: non-USD"}, false},
-		"mid-string mention":              {[]string{"computeengine: price lookup unavailable: x"}, false},
-		"truncated walk is not an outage": {[]string{"advisor: azure advisor: recommendation list is incomplete"}, false},
-		"truncated walk plus outage":      {[]string{"advisor: azure advisor: recommendation list is incomplete", "advisor: azure advisor: get: retail prices fetch failed"}, true},
-		"prefix mid-string":               {[]string{"sub-1: advisor: price lookup unavailable: x"}, false},
-		"none":                            {nil, false},
+		"a failed price lookup":                 {[]azureprovider.SkipKind{outage}, 1},
+		"later rows of the same call":           {[]azureprovider.SkipKind{unpriced, outage, outage}, 2},
+		"invalid recommendations only":          {[]azureprovider.SkipKind{unpriced, unpriced}, 0},
+		"a truncated walk is not an outage":     {[]azureprovider.SkipKind{incomplete}, 0},
+		"a truncated walk plus an outage":       {[]azureprovider.SkipKind{incomplete, outage}, 1},
+		"an unrecognized kind is not an outage": {[]azureprovider.SkipKind{azureprovider.SkipKind("a_kind_added_later")}, 0},
+		"none":                                  {nil, 0},
 	} {
-		t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, hasPricingOutageSkip(tc.reasons)) })
+		t.Run(name, func(t *testing.T) { assert.Equal(t, tc.want, countPricingOutageSkips(tc.kinds)) })
 	}
 }
 
 // A pricing outage holds eviction; ordinary invalid-row skips do not.
 func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) {
-	gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1,
-		reasons: []string{"advisor: price lookup unavailable: x"}})
+	gap, err := tolerateIncompleteSweep("azure", outageSkip("advisor: price lookup unavailable: x"))
 	require.NoError(t, err)
 	assert.True(t, gap.holdsEviction())
 	assert.Equal(t, 1, gap.skippedRecommendations)
@@ -162,11 +182,11 @@ func TestTolerateSkipped_PricingOutageHoldsEviction(t *testing.T) {
 func TestPricingOutageGap_FanOutRecordsHeldAccounts(t *testing.T) {
 	accounts := []config.CloudAccount{{ID: "held", Name: "held"}, {ID: "plain", Name: "plain"}}
 	fn := func(_ context.Context, acct config.CloudAccount) ([]config.RecommendationRecord, *sweepGap, error) {
-		reason := "advisor: non-USD"
+		var skip error = &skipErr{partial: true, count: 1, reasons: []string{"advisor: non-USD"}}
 		if acct.ID == "held" {
-			reason = "advisor: price lookup unavailable: x"
+			skip = outageSkip("advisor: price lookup unavailable: x")
 		}
-		gap, err := tolerateIncompleteSweep("azure", &skipErr{partial: true, count: 1, reasons: []string{reason}})
+		gap, err := tolerateIncompleteSweep("azure", skip)
 		return []config.RecommendationRecord{{ID: acct.ID}}, gap, err
 	}
 
@@ -180,3 +200,48 @@ func TestPricingOutageGap_FanOutRecordsHeldAccounts(t *testing.T) {
 	assert.Equal(t, []string{"held"}, outcome.IncompleteAccountIDs)
 	assert.Equal(t, []string{"plain"}, outcome.SucceededAccountIDs)
 }
+
+// The hold decision reads only the typed kind. An outage skip whose reason text
+// matches no known prefix still holds, and a skip whose text looks like an
+// outage but is not of that kind does not: the library can reword its reasons
+// without silently stopping or starting the hold.
+func TestTolerateSkipped_HoldReadsTheKindNotTheText(t *testing.T) {
+	gap, err := tolerateIncompleteSweep("azure", outageSkip("the retail catalog said no"))
+	require.NoError(t, err)
+	assert.True(t, gap.holdsEviction(), "an outage with unfamiliar text still holds")
+	assert.True(t, gap.pricingUnavailable)
+
+	lookalike := &kindSkipErr{skipErr: skipErr{partial: true, count: 1, reasons: []string{"advisor: price lookup unavailable: x"}},
+		kinds: []azureprovider.SkipKind{azureprovider.SkipUnpriceable}}
+	gap, err = tolerateIncompleteSweep("azure", lookalike)
+	require.NoError(t, err)
+	assert.False(t, gap.holdsEviction(), "outage-looking text of another kind does not hold")
+
+	// Kinds beyond the skips name no skip: the library ignores them, so they
+	// must not count as an outage.
+	extra := &kindSkipErr{skipErr: skipErr{partial: true, count: 1, reasons: []string{"a"}},
+		kinds: []azureprovider.SkipKind{azureprovider.SkipUnpriceable, azureprovider.SkipPriceLookupUnavailable}}
+	gap, err = tolerateIncompleteSweep("azure", extra)
+	require.NoError(t, err)
+	assert.False(t, gap.holdsEviction(), "a kind with no skip behind it never holds")
+
+	short := &kindSkipErr{skipErr: skipErr{partial: true, count: 2, reasons: []string{"a", "b"}},
+		kinds: []azureprovider.SkipKind{azureprovider.SkipUnpriceable}}
+	gap, err = tolerateIncompleteSweep("azure", short)
+	require.NoError(t, err)
+	assert.False(t, gap.holdsEviction(), "a skip with no kind reads as unpriceable, never as an outage")
+}
+
+// The same through the multi-subscription failure that also lost a subscription.
+func TestGapFromPartialSubscriptions_HoldReadsTheKindNotTheText(t *testing.T) {
+	failed := []azureprovider.SubscriptionFailure{{SubscriptionID: "sub-b", Err: errors.New("403 authorization failed")}}
+	gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailureWithKinds(2, failed,
+		[]string{"the retail catalog said no"}, []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable}))
+	require.NoError(t, err)
+	assert.True(t, gap.forAccount().withHeld("acct-1").pricingUnavailable)
+
+	gap, err = tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed,
+		[]string{"advisor: price lookup unavailable: x"}))
+	require.NoError(t, err)
+	assert.False(t, gap.forAccount().withHeld("acct-1").pricingUnavailable, "no kinds recorded: the text alone never holds")
+}

internal/scheduler/partial_sweep.go

@@ -41,7 +41,7 @@ type sweepGap struct {
 	skippedOnly bool
 	reasons     map[string]int
 	// pricingUnavailable is set when a skip meant the price could not be
-	// fetched, so the previous cycle's rows are held (see hasPricingOutageSkip).
+	// fetched, so the previous cycle's rows are held (see recordSkips).
 	pricingUnavailable bool
 	// heldAccountIDs are the platform account UUIDs whose rows are held for
 	// that reason; the collector that knows the account fills them in.
@@ -66,6 +66,13 @@ type partialResult interface {
 	SkipReasons() []string
 }
 
+// skipKinder is implemented by Azure's PartialResultError: one kind per skip
+// reason, index-aligned (cloud-commitments-go#670). A provider without it, such
+// as GCP, reads every skip as azureprovider.SkipUnpriceable.
+type skipKinder interface {
+	SkipKinds() []azureprovider.SkipKind
+}
+
 // tolerateSkippedRecommendations keeps the recommendations returned alongside
 // a Partial() error and records how many were skipped. Any other error is
 // returned unchanged.
@@ -76,33 +83,31 @@ func tolerateSkippedRecommendations(providerName string, err error) (*sweepGap,
 	}
 	logging.Warnf("%s recommendations partial, keeping the rest: %v", providerName, err)
 	g := &sweepGap{reasons: map[string]int{}}
-	g.recordSkips(p.SkippedCount(), p.SkipReasons())
+	var kinds []azureprovider.SkipKind
+	if k, ok := p.(skipKinder); ok {
+		kinds = k.SkipKinds()
+	}
+	g.recordSkips(p.SkippedCount(), kinds)
 	g.skippedOnly = !g.pricingUnavailable
 	return g, nil
 }
 
-// trimSubscriptionPrefixes drops the "<subscription ID>: " that the Azure
-// multi-subscription client puts before each skip it aggregates
-// (recommendations_multi_subscription.go:399 at 61111428), so the reasons match
-// the same prefixes as a single-subscription sweep. Only a leading segment
-// followed by an Advisor skip is removed, so unprefixed reasons pass through.
-func trimSubscriptionPrefixes(reasons []string) []string {
-	out := make([]string, len(reasons))
-	for i, reason := range reasons {
-		out[i] = reason
-		if sub, rest, ok := strings.Cut(reason, ": "); ok && sub != "" && !strings.ContainsAny(sub, " :") && strings.HasPrefix(rest, "advisor: ") {
-			out[i] = rest
-		}
-	}
-	return out
-}
-
-// recordSkips adds dropped recommendations to the gap. A skip that means "the
-// price could not be fetched" says nothing about the row being invalid, so its
-// previous-cycle row must survive: it marks the gap pricingUnavailable.
-func (g *sweepGap) recordSkips(count int, reasons []string) {
+// recordSkips adds dropped recommendations to the gap. A skip of kind
+// SkipPriceLookupUnavailable means "the price could not be fetched", which says
+// nothing about the row being invalid, so its previous-cycle row must survive:
+// it marks the gap pricingUnavailable. The decision reads only the typed kinds,
+// never the reason text, so a wording change in the library cannot silently
+// stop holding rows. A truncated walk (SkipSourceIncomplete) and an unusable
+// row (SkipUnpriceable) do not hold: holding on a walk that is always
+// truncated would keep that account's stale rows forever.
+func (g *sweepGap) recordSkips(count int, kinds []azureprovider.SkipKind) {
 	g.skippedRecommendations += count
-	if outage := countPricingOutageSkips(trimSubscriptionPrefixes(reasons)); outage > 0 {
+	// Kinds are index-aligned with the skips, and the library ignores extra
+	// ones, so only the first count can name a skip.
+	if len(kinds) > count {
+		kinds = kinds[:count]
+	}
+	if outage := countPricingOutageSkips(kinds); outage > 0 {
 		g.pricingUnavailable = true
 		g.reasons[config.PartialReasonPricingUnavailable] += outage
 	}
@@ -119,42 +124,12 @@ func (g *sweepGap) withHeld(accountID string) *sweepGap {
 	return out
 }
 
-// Skip-reason prefixes cloud-commitments-go records when Azure Advisor rows
-// cannot be priced because the Retail Prices catalog is unreachable
-// (providers/azure/recommendations_advisor_pricing.go, priceAdvisorReservations,
-// go#663). The library exports no constant for them: RecordSkip stores
-// "<service>: <err>" strings. The first failed row is recorded with the
-// collection-failure wrapper "azure advisor: "; every later row of the call
-// gets "price lookup unavailable: ".
-var pricingOutageSkipPrefixes = []string{
-	"advisor: price lookup unavailable: ",
-	"advisor: azure advisor: ",
-}
-
-// advisorTruncatedSkipPrefix starts the skip recorded for an Advisor walk that
-// hit the page cap or a page error (collectAdvisorOrSkip,
-// providers/azure/recommendations.go:471 at 61111428). It shares the
-// "advisor: azure advisor: " prefix but is not a pricing outage: a walk over
-// the page cap is always truncated, so holding eviction would keep that
-// account's stale rows forever.
-const advisorTruncatedSkipPrefix = "advisor: azure advisor: recommendation list is incomplete"
-
-// hasPricingOutageSkip reports whether any skip reason is a pricing outage.
-func hasPricingOutageSkip(reasons []string) bool {
-	return countPricingOutageSkips(reasons) > 0
-}
-
-func countPricingOutageSkips(reasons []string) int {
+// countPricingOutageSkips counts the skips whose price lookup did not complete.
+func countPricingOutageSkips(kinds []azureprovider.SkipKind) int {
 	n := 0
-	for _, reason := range reasons {
-		if strings.HasPrefix(reason, advisorTruncatedSkipPrefix) {
-			continue
-		}
-		for _, prefix := range pricingOutageSkipPrefixes {
-			if strings.HasPrefix(reason, prefix) {
-				n++
-				break
-			}
+	for _, kind := range kinds {
+		if kind == azureprovider.SkipPriceLookupUnavailable {
+			n++
 		}
 	}
 	return n
@@ -187,7 +162,7 @@ func gapFromPartialSubscriptions(p *azureprovider.PartialSubscriptionFailureErro
 	// user warning only and does not affect eviction (skippedOnly stays false),
 	// but a pricing outage among them holds the account like a skips-only one.
 	if sk := p.Skips(); sk != nil {
-		g.recordSkips(sk.SkippedCount(), sk.SkipReasons())
+		g.recordSkips(sk.SkippedCount(), sk.SkipKinds())
 	}
 	for _, f := range p.Failed {
 		g.reasons[classifySweepCause(f.Err)]++

internal/scheduler/partial_sweep_test.go

@@ -292,7 +292,8 @@ func TestGapFromPartialSubscriptions_PricingOutageSkipsHoldAccount(t *testing.T)
 	for i, r := range outage {
 		prefixed[i] = "0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: " + r
 	}
-	gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailure(2, failed, prefixed))
+	gap, err := tolerateIncompleteSweep("azure", azureprovider.NewPartialSubscriptionFailureWithKinds(2, failed, prefixed,
+		[]azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable, azureprovider.SkipPriceLookupUnavailable}))
 	require.NoError(t, err)
 
 	held := gap.forAccount().withHeld("acct-1")
@@ -312,7 +313,8 @@ func TestTolerateIncompleteSweep_PrefixedPricingOutageSkipsHoldAccount(t *testin
 		`0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`,
 		`0f4c1a52-8d3e-4b7a-9c61-2e5d7a9b3c10: advisor: price lookup unavailable: azure advisor: advisor compute reservation price for "Standard_D2s_v3" in "westeurope" (1y): get: retail prices fetch failed: EOF`,
 	}
-	gap, err := tolerateIncompleteSweep("azure", &azureprovider.PartialResultError{Skipped: skips})
+	gap, err := tolerateIncompleteSweep("azure", &azureprovider.PartialResultError{Skipped: skips,
+		Kinds: []azureprovider.SkipKind{azureprovider.SkipPriceLookupUnavailable, azureprovider.SkipPriceLookupUnavailable}})
 	require.NoError(t, err)
 
 	held := gap.forAccount().withHeld("acct-1")