diff --git a/model/provider_egress_health_model.go b/model/provider_egress_health_model.go index e84e8c22..d6008c2b 100644 --- a/model/provider_egress_health_model.go +++ b/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() { diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index ba9956d1..f6f35127 100644 --- a/model/provider_egress_location_model.go +++ b/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 } diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index da18e559..fd9f81bd 100644 --- a/model/provider_egress_location_model_test.go +++ b/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) + } + }) +}