fix(model): stop advertising providers on stale egress-health evidence

model/provider_egress_health_model.go

@@ -132,6 +132,24 @@ func SetProviderEgressHealth(ctx context.Context, health *ProviderEgressHealth)
 	})
 }
 
+// ProviderEgressHealthMaxAge is how long an egress-health measurement is
+// treated as current. Past it the provider is indistinguishable from one never
+// measured, and the gate fails it closed.
+//
+// 24h, against ProviderEgressLocationMaxAge's 7 days, because the two decay at
+// different rates. Where a provider egresses from is a property of its network
+// and rarely changes; whether it still carries traffic is a property of the
+// moment and can change without warning or notice -- a provider that stops
+// forwarding stays connected and keeps accepting clients, so nothing else in
+// the system reveals it.
+//
+// The value is a floor on how bad the list can get, not a tuning knob: it is
+// the longest a provider can blackhole while still being advertised. Shortening
+// it shrinks that window at the cost of demanding more probe throughput, since
+// every gated provider must be re-measured inside it or it drops out of the
+// list.
+const ProviderEgressHealthMaxAge = 24 * time.Hour
+
 // ProviderEgressHealthCounts is the ok/total tally alone, for consumers that
 // only need to decide "did this provider carry traffic" in bulk. The heavy
 // fields (per-class results, failure name lists) are diagnostics and are left
@@ -155,9 +173,29 @@ type ProviderEgressHealthCounts struct {
 // A provider that has never been measured has no entry, exactly as
 // GetProviderEgressHealth returns nil for it. Never measured is not the same as
 // measured-unhealthy and the two must stay distinguishable to the caller.
+//
+// # Stale evidence is not evidence
+//
+// Only measurements newer than ProviderEgressHealthMaxAge are returned. A
+// provider whose measurement has aged out is absent from the map and therefore
+// fails passesHealth closed, exactly as a never-measured one does -- which is
+// the correct reading, because both mean "no current evidence this provider
+// carries traffic."
+//
+// Omitting this bound published blackholes. Health drives the gate but nothing
+// re-measured it on its own schedule: the due queue keyed re-probes off
+// provider_egress_location's age, so a provider with a fresh location was never
+// re-probed and its health tally sat unchanged for days. On beta that left
+// 98.6% of gated providers advertised on evidence older than six hours, and 12
+// of 12 sampled from the stalest cohort answered ok=0/131 when probed -- total
+// blackholes, still in the public list, because a measurement taken days ago
+// said they were fine. GetAllProviderEgressCountryCodes has always bounded its
+// half this way; this is the missing symmetry.
 func GetAllProviderEgressHealthCounts(ctx context.Context) map[server.Id]ProviderEgressHealthCounts {
 	healthCounts := map[server.Id]ProviderEgressHealthCounts{}
 
+	minMeasuredAt := server.NowUtc().Add(-ProviderEgressHealthMaxAge)
+
 	server.Db(ctx, func(conn server.PgConn) {
 		result, err := conn.Query(
 			ctx,
@@ -168,7 +206,9 @@ func GetAllProviderEgressHealthCounts(ctx context.Context) map[server.Id]Provide
 				ok_count,
 				total_count
 			FROM provider_egress_health
+			WHERE measured_at >= $1
 			`,
+			minMeasuredAt.UTC(),
 		)
 		server.WithPgResult(result, err, func() {
 			for result.Next() {

model/provider_egress_location_model.go

@@ -1081,6 +1081,109 @@ func GetProviderEgressLocationDueSharded(
 				clientIds = append(clientIds, clientId)
 			}
 		})
+
+		remaining = limit - len(clientIds)
+		if remaining <= 0 {
+			return
+		}
+
+		// pass 3: located and fresh, but its egress HEALTH has gone stale.
+		//
+		// Passes 1 and 2 both key off provider_egress_location, so a provider
+		// with a fresh location was never re-offered no matter how old its
+		// health tally was. That is the schedule that published blackholes:
+		// health decides whether a provider is advertised
+		// (providerCountFilter.passesHealth) but location decided when it was
+		// re-measured, and location outlives health by 7 days to 1. A provider
+		// probed once, then quietly stopping forwarding, kept its passing tally
+		// and its place in the list until its LOCATION aged out days later.
+		//
+		// Now that GetAllProviderEgressHealthCounts drops stale rows, this pass
+		// is what keeps the list populated rather than merely correct: without
+		// it, every gated provider would age out of the map after
+		// ProviderEgressHealthMaxAge and never be re-measured, and the list
+		// would drain to nothing.
+		//
+		// Scoped to providers that HAVE a health row which has aged out, not to
+		// every provider lacking fresh health. The difference matters: "no fresh
+		// health" is also true of a provider that has never been measured at
+		// all, and offering those here would re-probe a provider whose location
+		// was taken minutes ago purely because no health row accompanies it --
+		// which is what pass 1 and the attempt backoff already govern. It also
+		// silently broke TestGetProviderEgressLocationDue, whose fixtures write
+		// locations without health rows: every one of them became due.
+		//
+		// A provider with a location but no health row is therefore left to
+		// passes 1 and 2. It is excluded from the list meanwhile (passesHealth
+		// fails closed on a missing row) and is re-offered when its location
+		// goes stale, so it is not stranded -- only deferred.
+		//
+		// Ordered by client_id: the 6h attempt backoff, not the ordering, is
+		// what rotates the sweep across the population.
+		minMeasuredAt := server.NowUtc().Add(-ProviderEgressHealthMaxAge / 2)
+
+		result, err = conn.Query(
+			ctx,
+			`
+			SELECT
+				network_client_location_reliability.client_id
+			FROM network_client_location_reliability
+
+			WHERE
+				network_client_location_reliability.connected = true AND
+				network_client_location_reliability.valid = true AND
+				EXISTS (
+					SELECT 1 FROM provide_key
+					WHERE
+						provide_key.client_id = network_client_location_reliability.client_id AND
+						provide_key.provide_mode = $1
+				) AND
+				EXISTS (
+					SELECT 1 FROM provider_egress_health
+					WHERE
+						provider_egress_health.client_id = network_client_location_reliability.client_id AND
+						provider_egress_health.measured_at < $2
+				) AND
+				NOT EXISTS (
+					SELECT 1 FROM provider_egress_probe_attempt
+					WHERE
+						provider_egress_probe_attempt.client_id = network_client_location_reliability.client_id AND
+						$3 <= provider_egress_probe_attempt.attempt_at
+				) AND
+				-- the same shard partition as passes 1 and 2; see pass 1 for
+				-- why the modulo has to be normalised.
+				(
+					$5 <= 1 OR
+					((hashtext(network_client_location_reliability.client_id::text) % $5) + $5) % $5 = $6
+				)
+
+			ORDER BY network_client_location_reliability.client_id ASC
+			LIMIT $4
+			`,
+			ProvideModePublic,
+			minMeasuredAt.UTC(),
+			minAttemptAt.UTC(),
+			remaining,
+			shardCount,
+			shardIndex,
+		)
+		server.WithPgResult(result, err, func() {
+			// Same separate-snapshot hazard as pass 2, and additionally this
+			// pass overlaps pass 1 by construction: a never-probed provider has
+			// no health row either, so it satisfies this predicate too.
+			seen := map[server.Id]bool{}
+			for _, clientId := range clientIds {
+				seen[clientId] = true
+			}
+			for result.Next() {
+				var clientId server.Id
+				server.Raise(result.Scan(&clientId))
+				if seen[clientId] {
+					continue
+				}
+				clientIds = append(clientIds, clientId)
+			}
+		})
 	})
 	return clientIds
 }

model/provider_egress_location_model_test.go

@@ -791,3 +791,108 @@ func TestGetAllProviderEgressCountryCodes(t *testing.T) {
 		connect.AssertEqual(t, ok, false)
 	})
 }
+
+// A provider whose egress HEALTH has aged out must be re-offered even though
+// its location is still fresh.
+//
+// The two ages are independent and only one of them gated re-probing. Health
+// decides whether a provider is published (providerCountFilter.passesHealth),
+// but both of the original passes keyed off provider_egress_location, which is
+// trusted seven times longer. A provider probed once and then quietly going
+// dark kept its passing tally, was never re-offered, and stayed in the public
+// list for days -- observed on beta as 12 of 12 sampled providers answering
+// ok=0/131 while still advertised.
+func TestGetProviderEgressLocationDueOffersStaleHealth(t *testing.T) {
+	server.DefaultTestEnv().Run(t, func(t testing.TB) {
+		ctx := context.Background()
+		now := server.NowUtc()
+
+		city := &Location{
+			LocationType: LocationTypeCity,
+			City:         "Palo Alto",
+			Region:       "California",
+			Country:      "United States",
+			CountryCode:  "us",
+		}
+		CreateLocation(ctx, city)
+
+		staleHealth := server.NewId()
+		freshHealth := server.NewId()
+
+		testing_connectProbeableProvider(t, ctx, staleHealth, city.LocationId, "0.0.0.1:0", ProvideModePublic)
+		testing_connectProbeableProvider(t, ctx, freshHealth, city.LocationId, "0.0.0.2:0", ProvideModePublic)
+
+		UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), now)
+
+		// BOTH locations are fresh, so neither is due under the location-driven
+		// passes. Health age is the only variable.
+		for _, clientId := range []server.Id{staleHealth, freshHealth} {
+			SetProviderEgressLocation(ctx, &ProviderEgressLocation{
+				ClientId: clientId, LocationId: city.LocationId,
+				CountryCode: "us", ObservedAt: now.Add(-1 * time.Hour),
+			})
+		}
+
+		SetProviderEgressHealth(ctx, &ProviderEgressHealth{
+			ClientId:   staleHealth,
+			MeasuredAt: now.Add(-ProviderEgressHealthMaxAge),
+			OKCount:    100, Total: 100,
+		})
+		SetProviderEgressHealth(ctx, &ProviderEgressHealth{
+			ClientId:   freshHealth,
+			MeasuredAt: now.Add(-1 * time.Minute),
+			OKCount:    100, Total: 100,
+		})
+
+		due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), now, 100)
+
+		if !slices.Contains(due, staleHealth) {
+			t.Errorf("due = %v, must contain the provider whose health aged out (%s): "+
+				"its tally still gates the public list, so leaving it unmeasured advertises a provider nothing has checked in over %s",
+				due, staleHealth, ProviderEgressHealthMaxAge)
+		}
+		if slices.Contains(due, freshHealth) {
+			t.Errorf("due = %v, must not contain the provider measured a minute ago (%s): "+
+				"re-probing on fresh evidence spends the queue on providers that do not need it",
+				due, freshHealth)
+		}
+	})
+}
+
+// A provider that has a location but has NEVER been health-measured is left to
+// the location-driven passes, not offered by the health pass.
+//
+// Scoping matters: "lacks fresh health" is also true of a provider measured
+// never, and offering those would re-probe a provider whose location was taken
+// minutes ago purely because no health row accompanies it. Such a provider is
+// excluded from the list meanwhile (passesHealth fails closed on a missing row)
+// and returns via pass 2 when its location goes stale, so it is deferred rather
+// than stranded.
+func TestGetProviderEgressLocationDueLeavesNeverMeasuredToLocationPasses(t *testing.T) {
+	server.DefaultTestEnv().Run(t, func(t testing.TB) {
+		ctx := context.Background()
+		now := server.NowUtc()
+
+		city := &Location{
+			LocationType: LocationTypeCity,
+			City:         "Palo Alto",
+			Region:       "California",
+			Country:      "United States",
+			CountryCode:  "us",
+		}
+		CreateLocation(ctx, city)
+
+		noHealthRow := server.NewId()
+		testing_connectProbeableProvider(t, ctx, noHealthRow, city.LocationId, "0.0.0.1:0", ProvideModePublic)
+		UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), now)
+
+		SetProviderEgressLocation(ctx, &ProviderEgressLocation{
+			ClientId: noHealthRow, LocationId: city.LocationId,
+			CountryCode: "us", ObservedAt: now.Add(-1 * time.Hour),
+		})
+
+		if due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), now, 100); slices.Contains(due, noHealthRow) {
+			t.Errorf("due = %v, must not contain the never-measured provider with a fresh location (%s)", due, noHealthRow)
+		}
+	})
+}