From cc64358a96b4a9367a93e6d6a5e7446f6992fdd2 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Fri, 24 Jul 2026 06:58:07 +0100 Subject: [PATCH 01/19] fix: stop country-only IP locations from tearing down connections SetConnectionLocation panicked when a client's IP resolved to a country- or region-only location (no city), because it inserted the location row's NULL city_location_id / region_location_id into network_client_location, where both are NOT NULL. The panic is the real damage, not just a failed lookup: it propagates out of the connection announce goroutine (connect/transport_announce.go), whose HandleError wrapper cancels the connection-level context -- tearing down the entire connect connection right after auth. And because the panic hits before the disconnect-cleanup defer is registered, the connection row is orphaned as connected=true forever. How often this fires depends on the geo database's city coverage, so it is rare with a full commercial dataset and constant with a sparse one. Fix: fall back to the coarsest available granularity so the columns are always non-null -- a country-only location stores its country id for city/region too, keeping the provider locatable at country level instead of crashing the connection. If even the country id is absent, return a clean error (the caller already has a graceful retry path) rather than panic. --- model/network_client_location_model.go | 30 +++++++++++ model/network_client_location_model_test.go | 59 +++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/model/network_client_location_model.go b/model/network_client_location_model.go index a2650fc4..cbab0e61 100644 --- a/model/network_client_location_model.go +++ b/model/network_client_location_model.go @@ -1242,6 +1242,36 @@ func SetConnectionLocation( } }) + // fix(beta): the free-tier ipinfo geo db resolves many IPs -- + // datacenter, mobile, and VPN egress especially -- to country or + // region granularity only, with no city. network_client_location + // requires city_location_id and region_location_id NOT NULL, so a + // country-only location's NULL city/region made this INSERT panic + // inside server.Tx. That panic propagated out of the connection + // announce goroutine (connect/transport_announce.go), whose + // HandleError wrapper then cancelled the whole connection context -- + // tearing down every country-only client's connect connection right + // after auth (the app itself included, whichever egress it resolved + // to), and, because the panic hit before the disconnect-cleanup + // defer was registered, orphaning the connection row as + // connected=true forever. Fall back to the coarsest available + // granularity so the columns are always non-null: a country-only + // location stores its country id for city/region too, which keeps + // the provider locatable at country level instead of crashing the + // connection. If even the country id is missing (location row absent + // or malformed), return a clean error so the caller's existing + // graceful retry path handles it -- never panic here. + if countryLocationId == nil { + returnErr = fmt.Errorf("Location %s has no country granularity.", locationId) + return + } + if cityLocationId == nil { + cityLocationId = countryLocationId + } + if regionLocationId == nil { + regionLocationId = countryLocationId + } + server.RaisePgResult(tx.Exec( ctx, ` diff --git a/model/network_client_location_model_test.go b/model/network_client_location_model_test.go index ab91c7ee..888a1ce2 100644 --- a/model/network_client_location_model_test.go +++ b/model/network_client_location_model_test.go @@ -1198,3 +1198,62 @@ func TestFindProviders2ReliabilityDeployGap(t *testing.T) { connect.AssertEqual(t, len(res.Providers), n) }) } + +// SetConnectionLocation must not panic when the resolved location has no city +// (country-only). network_client_location requires city_location_id and +// region_location_id NOT NULL, but a country-granularity location row has them +// NULL, so the insert panicked inside server.Tx. That panic propagated out of +// the connection announce goroutine, whose HandleError wrapper cancelled the +// connection context -- tearing down the client's connection right after auth, +// and (because the panic hit before the disconnect-cleanup defer was +// registered) orphaning the connection row as connected=true. This asserts a +// country-only location is stored, falling back to country granularity, with +// no panic and no error. +func TestSetConnectionLocationToleratesCountryOnlyLocation(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + // country-only location -- its row has NULL city_location_id and + // NULL region_location_id, exactly what crashed the insert before + country := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, country) + + networkId := server.NewId() + clientId := server.NewId() + Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, "0.0.0.1:0", handlerId) + connect.AssertEqual(t, err, nil) + + // this call panicked before the fix; now it must succeed and store + // the connection at country granularity + err = SetConnectionLocation(ctx, connectionId, country.LocationId, &ConnectionLocationScores{}) + connect.AssertEqual(t, err, nil) + + var city, region, cty *server.Id + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT city_location_id, region_location_id, country_location_id FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&city, ®ion, &cty)) + } + }) + }) + if city == nil || region == nil || cty == nil { + t.Fatal("expected all three location ids to be set (falling back to country), got a nil") + } + // city and region fall back to the country id for a country-only location + connect.AssertEqual(t, *city, country.CountryLocationId) + connect.AssertEqual(t, *region, country.CountryLocationId) + connect.AssertEqual(t, *cty, country.CountryLocationId) + }) +} From 0bf9387afa56a3941e19c02c92a2a31c269c648d Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 04:15:25 +0100 Subject: [PATCH 02/19] feat(model): provider_egress_location storage --- db_migrations.go | 26 ++++ model/provider_egress_location_model.go | 154 +++++++++++++++++++ model/provider_egress_location_model_test.go | 129 ++++++++++++++++ 3 files changed, 309 insertions(+) create mode 100644 model/provider_egress_location_model.go create mode 100644 model/provider_egress_location_model_test.go diff --git a/db_migrations.go b/db_migrations.go index 063f091e..204afa4b 100644 --- a/db_migrations.go +++ b/db_migrations.go @@ -4385,4 +4385,30 @@ var migrations = []any{ CREATE INDEX IF NOT EXISTS network_client_top_level_contract_time ON network_client (contract_time) WHERE (active = true AND source_client_id IS NULL AND contract_time IS NOT NULL) `), + + // provider egress locations: locations learned by probing a provider's own + // egress (see docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md). + // Keyed by client_id, one row per provider, upserted by the operator's + // prober. location_id is the canonical country (or city, when the probe was + // city-confident) location row. observed_at is when the probe ran, and is + // what freshness is judged against. + newSqlMigration(` + CREATE TABLE IF NOT EXISTS provider_egress_location ( + client_id uuid NOT NULL PRIMARY KEY, + location_id uuid NOT NULL, + country_code varchar(2) NOT NULL, + asn int NOT NULL DEFAULT 0, + org varchar(256) NOT NULL DEFAULT '', + hosting bool NOT NULL DEFAULT false, + proxy bool NOT NULL DEFAULT false, + mobile bool NOT NULL DEFAULT false, + city_confident bool NOT NULL DEFAULT false, + observed_at timestamp NOT NULL, + update_time timestamp NOT NULL + ) + `), + newSqlMigration(` + CREATE INDEX IF NOT EXISTS provider_egress_location_observed_at + ON provider_egress_location (observed_at) + `), } diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go new file mode 100644 index 00000000..797d82fd --- /dev/null +++ b/model/provider_egress_location_model.go @@ -0,0 +1,154 @@ +package model + +import ( + "context" + "time" + + "github.com/urnetwork/server" +) + +// ProviderEgressLocationMaxAge bounds how long a probed egress location is +// trusted. Past this, the location is ignored and the caller falls back to the +// mmdb lookup on the observed control ip. +const ProviderEgressLocationMaxAge = 7 * 24 * time.Hour + +// ProviderEgressLocation is a provider location learned by probing the +// provider's own egress, rather than by looking up its control-connection ip. +type ProviderEgressLocation struct { + ClientId server.Id + LocationId server.Id + CountryCode string + ASN int + Org string + Hosting bool + Proxy bool + Mobile bool + CityConfident bool + ObservedAt time.Time + UpdateTime time.Time +} + +// SetProviderEgressLocation upserts the probed location for a provider. +func SetProviderEgressLocation(ctx context.Context, e *ProviderEgressLocation) { + server.Tx(ctx, func(tx server.PgTx) { + server.RaisePgResult(tx.Exec( + ctx, + ` + INSERT INTO provider_egress_location ( + client_id, + location_id, + country_code, + asn, + org, + hosting, + proxy, + mobile, + city_confident, + observed_at, + update_time + ) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) + ON CONFLICT (client_id) DO UPDATE + SET + location_id = $2, + country_code = $3, + asn = $4, + org = $5, + hosting = $6, + proxy = $7, + mobile = $8, + city_confident = $9, + observed_at = $10, + update_time = $11 + `, + e.ClientId, + e.LocationId, + e.CountryCode, + e.ASN, + e.Org, + e.Hosting, + e.Proxy, + e.Mobile, + e.CityConfident, + e.ObservedAt.UTC(), + server.NowUtc(), + )) + }) +} + +// GetProviderEgressLocation returns the stored location for a provider, or nil. +func GetProviderEgressLocation(ctx context.Context, clientId server.Id) *ProviderEgressLocation { + var e *ProviderEgressLocation + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query( + ctx, + ` + SELECT + client_id, + location_id, + country_code, + asn, + org, + hosting, + proxy, + mobile, + city_confident, + observed_at, + update_time + FROM provider_egress_location + WHERE client_id = $1 + `, + clientId, + ) + server.WithPgResult(result, err, func() { + if result.Next() { + e = &ProviderEgressLocation{} + server.Raise(result.Scan( + &e.ClientId, + &e.LocationId, + &e.CountryCode, + &e.ASN, + &e.Org, + &e.Hosting, + &e.Proxy, + &e.Mobile, + &e.CityConfident, + &e.ObservedAt, + &e.UpdateTime, + )) + } + }) + }) + return e +} + +// GetFreshProviderEgressLocation is GetProviderEgressLocation, filtered to +// entries probed within maxAge. The cutoff is computed in Go and bound as a +// parameter: observed_at is a naive timestamp holding utc, and comparing it +// against sql now() would cast through the session timezone. +func GetFreshProviderEgressLocation( + ctx context.Context, + clientId server.Id, + maxAge time.Duration, +) *ProviderEgressLocation { + e := GetProviderEgressLocation(ctx, clientId) + if e == nil { + return nil + } + if e.ObservedAt.Before(server.NowUtc().Add(-maxAge)) { + return nil + } + return e +} + +// RemoveExpiredProviderEgressLocations drops entries probed before +// minObservedAt. +func RemoveExpiredProviderEgressLocations(ctx context.Context, minObservedAt time.Time) { + server.MaintenanceTx(ctx, func(tx server.PgTx) { + server.RaisePgResult(tx.Exec( + ctx, + `DELETE FROM provider_egress_location WHERE observed_at < $1`, + minObservedAt.UTC(), + )) + }) +} diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go new file mode 100644 index 00000000..37d9c494 --- /dev/null +++ b/model/provider_egress_location_model_test.go @@ -0,0 +1,129 @@ +package model + +import ( + "context" + "testing" + "time" + + "github.com/go-playground/assert/v2" + + "github.com/urnetwork/server" +) + +func TestProviderEgressLocationUpsertAndGet(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + country := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, country) + + clientId := server.NewId() + now := server.NowUtc() + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: clientId, + LocationId: country.LocationId, + CountryCode: "us", + ASN: 401486, + Org: "RAVNIX LLC", + Hosting: true, + ObservedAt: now, + }) + + got := GetProviderEgressLocation(ctx, clientId) + if got == nil { + t.Fatal("expected a stored egress location") + } + assert.Equal(t, got.LocationId, country.LocationId) + assert.Equal(t, got.CountryCode, "us") + assert.Equal(t, got.ASN, 401486) + assert.Equal(t, got.Hosting, true) + assert.Equal(t, got.Proxy, false) + + // upsert replaces + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: clientId, + LocationId: country.LocationId, + CountryCode: "us", + ASN: 999, + Hosting: false, + Proxy: true, + ObservedAt: now, + }) + got = GetProviderEgressLocation(ctx, clientId) + assert.Equal(t, got.ASN, 999) + assert.Equal(t, got.Hosting, false) + assert.Equal(t, got.Proxy, true) + }) +} + +func TestProviderEgressLocationFreshness(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + country := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, country) + + fresh := server.NewId() + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: fresh, LocationId: country.LocationId, CountryCode: "us", + ObservedAt: server.NowUtc(), + }) + stale := server.NewId() + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: stale, LocationId: country.LocationId, CountryCode: "us", + ObservedAt: server.NowUtc().Add(-8 * 24 * time.Hour), + }) + + if GetFreshProviderEgressLocation(ctx, fresh, ProviderEgressLocationMaxAge) == nil { + t.Fatal("fresh entry must be returned") + } + if GetFreshProviderEgressLocation(ctx, stale, ProviderEgressLocationMaxAge) != nil { + t.Fatal("stale entry must not be returned") + } + // absent + if GetFreshProviderEgressLocation(ctx, server.NewId(), ProviderEgressLocationMaxAge) != nil { + t.Fatal("absent entry must return nil") + } + }) +} + +func TestRemoveExpiredProviderEgressLocations(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + country := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, country) + + keep := server.NewId() + drop := server.NewId() + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: keep, LocationId: country.LocationId, CountryCode: "us", + ObservedAt: server.NowUtc(), + }) + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: drop, LocationId: country.LocationId, CountryCode: "us", + ObservedAt: server.NowUtc().Add(-30 * 24 * time.Hour), + }) + + RemoveExpiredProviderEgressLocations(ctx, server.NowUtc().Add(-14*24*time.Hour)) + + if GetProviderEgressLocation(ctx, keep) == nil { + t.Fatal("recent entry must survive the sweep") + } + if GetProviderEgressLocation(ctx, drop) != nil { + t.Fatal("old entry must be swept") + } + }) +} From 88f9afd083b885c8d793f0b5a78b7207e328eb76 Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 04:28:58 +0100 Subject: [PATCH 03/19] fix(model): lowercase country code in SetProviderEgressLocation The sub-project's binding constraint is that country codes are stored/ compared lowercased, and CreateLocation (network_client_location_model.go) already enforces this at the model layer via strings.ToLower before writing. SetProviderEgressLocation wrote e.CountryCode verbatim, so an uppercase code from an operator-run prober (the raw "US" the real geolocation APIs return) would silently persist uppercase and violate the invariant that countryCodeLocationIds and other lookups depend on. Normalize to lowercase before the INSERT/UPDATE, mirroring CreateLocation's idiom, and add a test asserting SetProviderEgressLocation("US") round-trips as "us" via GetProviderEgressLocation. --- model/provider_egress_location_model.go | 8 ++++- model/provider_egress_location_model_test.go | 32 ++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 797d82fd..980e42f7 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -2,6 +2,7 @@ package model import ( "context" + "strings" "time" "github.com/urnetwork/server" @@ -30,6 +31,11 @@ type ProviderEgressLocation struct { // SetProviderEgressLocation upserts the probed location for a provider. func SetProviderEgressLocation(ctx context.Context, e *ProviderEgressLocation) { + // country codes are stored/compared lowercased (see CreateLocation in + // network_client_location_model.go); the geolocation APIs that feed this + // return uppercase codes (e.g. "US"), so normalize before writing. + countryCode := strings.ToLower(e.CountryCode) + server.Tx(ctx, func(tx server.PgTx) { server.RaisePgResult(tx.Exec( ctx, @@ -63,7 +69,7 @@ func SetProviderEgressLocation(ctx context.Context, e *ProviderEgressLocation) { `, e.ClientId, e.LocationId, - e.CountryCode, + countryCode, e.ASN, e.Org, e.Hosting, diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index 37d9c494..bf229eb5 100644 --- a/model/provider_egress_location_model_test.go +++ b/model/provider_egress_location_model_test.go @@ -60,6 +60,38 @@ func TestProviderEgressLocationUpsertAndGet(t *testing.T) { }) } +func TestProviderEgressLocationCountryCodeLowercased(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + country := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, country) + + clientId := server.NewId() + // geolocation APIs return uppercase codes (e.g. "US"); the model must + // normalize to lowercase before storing, matching CreateLocation's + // established invariant that country codes are stored/compared lowercased. + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: clientId, + LocationId: country.LocationId, + CountryCode: "US", + ASN: 12345, + Org: "TEST ORG", + ObservedAt: server.NowUtc(), + }) + + got := GetProviderEgressLocation(ctx, clientId) + if got == nil { + t.Fatal("expected a stored egress location") + } + assert.Equal(t, got.CountryCode, "us") + }) +} + func TestProviderEgressLocationFreshness(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() From cae15a35a41d2f2fda1aca2b8a88ec951ed95752 Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 04:37:46 +0100 Subject: [PATCH 04/19] feat(controller): resolve and store provider egress location submissions --- .../provider_egress_location_controller.go | 94 ++++++++++++ ...rovider_egress_location_controller_test.go | 139 ++++++++++++++++++ model/provider_egress_location_model.go | 55 +++++++ 3 files changed, 288 insertions(+) create mode 100644 controller/provider_egress_location_controller.go create mode 100644 controller/provider_egress_location_controller_test.go diff --git a/controller/provider_egress_location_controller.go b/controller/provider_egress_location_controller.go new file mode 100644 index 00000000..57a11d78 --- /dev/null +++ b/controller/provider_egress_location_controller.go @@ -0,0 +1,94 @@ +package controller + +import ( + "context" + "fmt" + "strings" + "time" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/model" +) + +// MaxProviderEgressLocationSubmissionAge rejects a submission whose probe is +// already older than this when it arrives. It bounds replay of an old probe. +const MaxProviderEgressLocationSubmissionAge = 24 * time.Hour + +type SubmitProviderEgressLocationArgs struct { + ClientId server.Id `json:"client_id"` + CountryCode string `json:"country_code"` + Country string `json:"country"` + Region string `json:"region,omitempty"` + City string `json:"city,omitempty"` + ASN int `json:"asn,omitempty"` + Org string `json:"org,omitempty"` + Hosting bool `json:"hosting,omitempty"` + Proxy bool `json:"proxy,omitempty"` + Mobile bool `json:"mobile,omitempty"` + CountryConfident bool `json:"country_confident"` + CityConfident bool `json:"city_confident,omitempty"` + ObservedAt time.Time `json:"observed_at"` +} + +type SubmitProviderEgressLocationResult struct { + LocationId server.Id `json:"location_id"` +} + +// SubmitProviderEgressLocation records a probed egress location for a provider. +// Only country-confident submissions are accepted; city/region are stored only +// when the probe was also city-confident (free geolocation sources disagree on +// city often enough that an unconfirmed city is worse than none). +func SubmitProviderEgressLocation( + ctx context.Context, + args *SubmitProviderEgressLocationArgs, +) (*SubmitProviderEgressLocationResult, error) { + if !args.CountryConfident { + return nil, fmt.Errorf("Submission is not country-confident.") + } + countryCode := strings.ToLower(strings.TrimSpace(args.CountryCode)) + if len(countryCode) != 2 { + return nil, fmt.Errorf("Country code must be alpha-2.") + } + if args.ObservedAt.IsZero() { + return nil, fmt.Errorf("Missing observed_at.") + } + if args.ObservedAt.Before(server.NowUtc().Add(-MaxProviderEgressLocationSubmissionAge)) { + return nil, fmt.Errorf("Submission is too old.") + } + if networkId := model.GetNetworkClientNetwork(ctx, args.ClientId); networkId == nil { + return nil, fmt.Errorf("Unknown client.") + } + + // resolve to a canonical location row. city granularity only when the + // probe agreed on a city; otherwise country. + location := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: args.Country, + CountryCode: countryCode, + } + if args.CityConfident && args.City != "" { + location = &model.Location{ + LocationType: model.LocationTypeCity, + City: args.City, + Region: args.Region, + Country: args.Country, + CountryCode: countryCode, + } + } + model.CreateLocation(ctx, location) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: args.ClientId, + LocationId: location.LocationId, + CountryCode: countryCode, + ASN: args.ASN, + Org: args.Org, + Hosting: args.Hosting, + Proxy: args.Proxy, + Mobile: args.Mobile, + CityConfident: args.CityConfident, + ObservedAt: args.ObservedAt, + }) + + return &SubmitProviderEgressLocationResult{LocationId: location.LocationId}, nil +} diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go new file mode 100644 index 00000000..459e1183 --- /dev/null +++ b/controller/provider_egress_location_controller_test.go @@ -0,0 +1,139 @@ +package controller + +import ( + "context" + "testing" + "time" + + "github.com/go-playground/assert/v2" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/model" +) + +func TestSubmitProviderEgressLocationCountryOnly(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + res, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "US", + Country: "United States", + ASN: 401486, + Org: "RAVNIX LLC", + Hosting: true, + CountryConfident: true, + ObservedAt: server.NowUtc(), + }) + assert.Equal(t, err, nil) + if res.LocationId == (server.Id{}) { + t.Fatal("expected a resolved location id") + } + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatal("expected the submission to be stored") + } + assert.Equal(t, stored.CountryCode, "us") + assert.Equal(t, stored.ASN, 401486) + assert.Equal(t, stored.Hosting, true) + assert.Equal(t, stored.CityConfident, false) + }) +} + +func TestSubmitProviderEgressLocationRejectsNotCountryConfident(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + CountryConfident: false, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("a submission that is not country-confident must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + +func TestSubmitProviderEgressLocationRejectsUnknownClient(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: server.NewId(), // never created + CountryCode: "us", + CountryConfident: true, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("unknown client_id must be rejected") + } + }) +} + +func TestSubmitProviderEgressLocationCityConfidentStoresCity(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: "United States", + Region: "Colorado", + City: "Denver", + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + assert.Equal(t, err, nil) + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatal("expected the submission to be stored") + } + assert.Equal(t, stored.CityConfident, true) + + // the resolved location must be the city-granular row + loc := model.GetLocation(ctx, stored.LocationId) + if loc == nil { + t.Fatal("expected the resolved location row to exist") + } + assert.Equal(t, loc.LocationType, model.LocationTypeCity) + }) +} + +func TestSubmitProviderEgressLocationRejectsStaleObservedAt(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + CountryConfident: true, + ObservedAt: server.NowUtc().Add(-30 * 24 * time.Hour), + }) + if err == nil { + t.Fatal("a submission observed long ago must be rejected") + } + }) +} diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 980e42f7..2d78d751 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -147,6 +147,61 @@ func GetFreshProviderEgressLocation( return e } +// GetLocation returns the canonical location row, or nil. +// +// Note: the location table also has a location_name column, but it holds the +// name for whichever granularity that specific row represents (e.g. a city +// row's own name), not a single name field on the Location struct. Location +// instead splits City/Region/Country by joining sibling rows (see +// IndexSearchLocationsInTx in network_client_location_model.go). This helper +// only needs to resolve identity/type, so it selects the columns that map +// directly onto Location's fields and leaves City/Region/Country empty. +func GetLocation(ctx context.Context, locationId server.Id) *Location { + var loc *Location + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query( + ctx, + ` + SELECT location_id, location_type, city_location_id, region_location_id, country_location_id, country_code + FROM location + WHERE location_id = $1 + `, + locationId, + ) + server.WithPgResult(result, err, func() { + if result.Next() { + loc = &Location{} + // city_location_id/region_location_id are only set once the + // row's hierarchy reaches that granularity (e.g. a country + // row has both NULL); server.Id.Scan errors on a nil source, + // so scan through nullable pointers as in + // IndexSearchLocationsInTx (network_client_location_model.go). + var cityLocationId *server.Id + var regionLocationId *server.Id + var countryLocationId *server.Id + server.Raise(result.Scan( + &loc.LocationId, + &loc.LocationType, + &cityLocationId, + ®ionLocationId, + &countryLocationId, + &loc.CountryCode, + )) + if cityLocationId != nil { + loc.CityLocationId = *cityLocationId + } + if regionLocationId != nil { + loc.RegionLocationId = *regionLocationId + } + if countryLocationId != nil { + loc.CountryLocationId = *countryLocationId + } + } + }) + }) + return loc +} + // RemoveExpiredProviderEgressLocations drops entries probed before // minObservedAt. func RemoveExpiredProviderEgressLocations(ctx context.Context, minObservedAt time.Time) { From 206bdd50d2eaa3568a2d955f311e6f18f1c0b3d1 Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 04:55:34 +0100 Subject: [PATCH 05/19] test(controller): assert country granularity in provider egress location test TestSubmitProviderEgressLocationCountryOnly only checked the persisted CityConfident flag, which is copied straight from args independent of the location-resolution branch. It never asserted the resolved location's LocationType, so a broken granularity gate (resolving city unconditionally) would slip past all 5 existing tests. Add an assertion that the resolved location is LocationTypeCountry, mirroring the pattern already used by TestSubmitProviderEgressLocationCityConfidentStoresCity. Also assert CityLocationId/RegionLocationId are unset, since a country-granularity row's INSERT never populates those columns (verified against CreateLocation and GetLocation). --- .../provider_egress_location_controller_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go index 459e1183..9f8c70a4 100644 --- a/controller/provider_egress_location_controller_test.go +++ b/controller/provider_egress_location_controller_test.go @@ -42,6 +42,20 @@ func TestSubmitProviderEgressLocationCountryOnly(t *testing.T) { assert.Equal(t, stored.ASN, 401486) assert.Equal(t, stored.Hosting, true) assert.Equal(t, stored.CityConfident, false) + + // the resolved location must be the country-granular row, with no + // city/region association + loc := model.GetLocation(ctx, stored.LocationId) + if loc == nil { + t.Fatal("expected the resolved location row to exist") + } + assert.Equal(t, loc.LocationType, model.LocationTypeCountry) + if loc.CityLocationId != (server.Id{}) { + t.Fatal("a country-granularity row must not have a city association") + } + if loc.RegionLocationId != (server.Id{}) { + t.Fatal("a country-granularity row must not have a region association") + } }) } From feaed919dab23b9a7d1e32534c947a9edbcae7cb Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 05:04:20 +0100 Subject: [PATCH 06/19] feat(api): operator-authenticated provider egress location ingest Adds POST /network/provider-egress-location, an operator-to-server endpoint (not a client route) that ingests probed provider egress locations. Authenticated by a shared secret from the vault (beta-vault/vault/provider_egress.yml, key ingest_secret) compared with hmac.Equal, not a network jwt. Fails closed: an absent vault resource or empty/missing key disables the endpoint (every request rejected) without panicking the api process at startup or per-request. --- api/api.go | 1 + .../provider_egress_location_handlers.go | 83 +++++++++++++++++++ .../provider_egress_location_handlers_test.go | 38 +++++++++ beta-vault/vault/provider_egress.yml.example | 6 ++ 4 files changed, 128 insertions(+) create mode 100644 api/handlers/provider_egress_location_handlers.go create mode 100644 api/handlers/provider_egress_location_handlers_test.go create mode 100644 beta-vault/vault/provider_egress.yml.example diff --git a/api/api.go b/api/api.go index fef528b8..c3dc801d 100644 --- a/api/api.go +++ b/api/api.go @@ -51,6 +51,7 @@ func Routes() []*router.Route { router.NewRoute("POST", "/auth/upgrade-guest-existing", handlers.UpgradeGuestExisting), router.NewRoute("POST", "/network/auth-client", handlers.AuthNetworkClient), router.NewRoute("POST", "/network/remove-client", handlers.RemoveNetworkClient), + router.NewRoute("POST", "/network/provider-egress-location", handlers.ProviderEgressLocationSubmit), router.NewRoute("GET", "/network/clients", handlers.NetworkClients), router.NewRoute("GET", "/network/peers", handlers.NetworkPeers), router.NewRoute("GET", "/network/provider-locations", handlers.NetworkGetProviderLocations), diff --git a/api/handlers/provider_egress_location_handlers.go b/api/handlers/provider_egress_location_handlers.go new file mode 100644 index 00000000..13557886 --- /dev/null +++ b/api/handlers/provider_egress_location_handlers.go @@ -0,0 +1,83 @@ +package handlers + +import ( + "crypto/hmac" + "encoding/json" + "io" + "net/http" + "sync" + + "github.com/urnetwork/glog" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/controller" +) + +// operatorSecretHeader carries the operator ingest secret. This endpoint is +// operator-to-server, not a client route: it is authenticated by a shared +// secret from the vault, not by a network jwt. +const operatorSecretHeader = "X-UR-Operator-Secret" + +// maxProviderEgressLocationBody bounds the request body. +const maxProviderEgressLocationBody = 16 * 1024 + +// operatorIngestSecret reads the operator ingest secret from the vault +// (beta-vault/vault/provider_egress.yml, key "ingest_secret"). It returns "" +// when the vault resource is absent, the key is absent, or the key is empty, +// which makes the endpoint fail closed (every request is rejected) rather +// than open. `SimpleResource`/`String` are the non-panicking lookups (unlike +// `RequireSimpleResource`/`RequireString`), so a missing vault resource +// disables the endpoint instead of panicking the api process at startup or +// per-request. +var operatorIngestSecret = sync.OnceValue(func() string { + res, err := server.Vault.SimpleResource("provider_egress.yml") + if err != nil { + glog.Infof("[pegl]no provider_egress.yml in the vault; ingest endpoint disabled\n") + return "" + } + values := res.String("ingest_secret") + if len(values) != 1 || values[0] == "" { + glog.Infof("[pegl]no ingest_secret in provider_egress.yml; ingest endpoint disabled\n") + return "" + } + return values[0] +}) + +// ProviderEgressLocationSubmit ingests a probed provider egress location from +// the operator's prober. See +// docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md. +func ProviderEgressLocationSubmit(w http.ResponseWriter, r *http.Request) { + secret := operatorIngestSecret() + provided := r.Header.Get(operatorSecretHeader) + if secret == "" || provided == "" || !hmac.Equal([]byte(secret), []byte(provided)) { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + + body, err := io.ReadAll(io.LimitReader(r.Body, maxProviderEgressLocationBody+1)) + if err != nil { + http.Error(w, "Bad request", http.StatusBadRequest) + return + } + if len(body) > maxProviderEgressLocationBody { + http.Error(w, "Request too large", http.StatusRequestEntityTooLarge) + return + } + + var args controller.SubmitProviderEgressLocationArgs + if err := json.Unmarshal(body, &args); err != nil { + http.Error(w, "Bad request", http.StatusBadRequest) + return + } + + result, err := controller.SubmitProviderEgressLocation(r.Context(), &args) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(result); err != nil { + glog.Infof("[pegl]could not write response. err = %s\n", err) + } +} diff --git a/api/handlers/provider_egress_location_handlers_test.go b/api/handlers/provider_egress_location_handlers_test.go new file mode 100644 index 00000000..a2afd35f --- /dev/null +++ b/api/handlers/provider_egress_location_handlers_test.go @@ -0,0 +1,38 @@ +package handlers + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +func TestProviderEgressLocationSubmitRejectsMissingSecret(t *testing.T) { + body, _ := json.Marshal(map[string]any{ + "client_id": "019f8835-158d-6fd8-e9dd-fd0e4c6d6792", + }) + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-location", bytes.NewReader(body)) + w := httptest.NewRecorder() + + ProviderEgressLocationSubmit(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 when the operator secret header is absent", w.Code) + } +} + +func TestProviderEgressLocationSubmitRejectsWrongSecret(t *testing.T) { + body, _ := json.Marshal(map[string]any{ + "client_id": "019f8835-158d-6fd8-e9dd-fd0e4c6d6792", + }) + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-location", bytes.NewReader(body)) + req.Header.Set(operatorSecretHeader, "definitely-not-the-secret") + w := httptest.NewRecorder() + + ProviderEgressLocationSubmit(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 on a wrong operator secret", w.Code) + } +} diff --git a/beta-vault/vault/provider_egress.yml.example b/beta-vault/vault/provider_egress.yml.example new file mode 100644 index 00000000..9402cafa --- /dev/null +++ b/beta-vault/vault/provider_egress.yml.example @@ -0,0 +1,6 @@ +# Operator ingest secret for POST /network/provider-egress-location. +# The operator's egress prober sends this value in the X-UR-Operator-Secret +# header. Generate with: openssl rand -base64 32 +# If this file or key is absent, the ingest endpoint rejects every request +# (fails closed, does not crash the api process). +ingest_secret: "replace-me" From 208b989437f643decf3293e65e84f868f009ab8b Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 05:24:55 +0100 Subject: [PATCH 07/19] test(api): prove the provider-egress ingest auth gate can accept, not just reject Both committed tests for ProviderEgressLocationSubmit ran with the vault unconfigured, so hmac.Equal never executed - a handler that always returned 401 would have passed the same suite. Extract the memoized secret reader (sync.OnceValue) into a reassignable package var plus a plain readOperatorIngestSecret function, with no change to the read logic or the handler's auth gate, so tests can inject a known secret without racing the existing unconfigured-vault reject tests in the same process. Add a positive-path test (correct secret clears auth and reaches the controller, 400 "Unknown client." for an unregistered id) and a negative discrimination test (wrong secret against a configured vault still 401), plus a direct vault-read test. Document in the vault example that the secret is memoized at first request, so rotating it requires an api restart. --- .../provider_egress_location_handlers.go | 12 +- .../provider_egress_location_handlers_test.go | 112 ++++++++++++++++++ beta-vault/vault/provider_egress.yml.example | 3 + 3 files changed, 124 insertions(+), 3 deletions(-) diff --git a/api/handlers/provider_egress_location_handlers.go b/api/handlers/provider_egress_location_handlers.go index 13557886..3271b167 100644 --- a/api/handlers/provider_egress_location_handlers.go +++ b/api/handlers/provider_egress_location_handlers.go @@ -21,7 +21,13 @@ const operatorSecretHeader = "X-UR-Operator-Secret" // maxProviderEgressLocationBody bounds the request body. const maxProviderEgressLocationBody = 16 * 1024 -// operatorIngestSecret reads the operator ingest secret from the vault +// operatorIngestSecret memoizes readOperatorIngestSecret for the life of the +// process. It is a package-level var (not a plain sync.OnceValue call site) +// so tests can swap it for a stub and restore it with defer; production code +// never reassigns it. +var operatorIngestSecret func() string = sync.OnceValue(readOperatorIngestSecret) + +// readOperatorIngestSecret reads the operator ingest secret from the vault // (beta-vault/vault/provider_egress.yml, key "ingest_secret"). It returns "" // when the vault resource is absent, the key is absent, or the key is empty, // which makes the endpoint fail closed (every request is rejected) rather @@ -29,7 +35,7 @@ const maxProviderEgressLocationBody = 16 * 1024 // `RequireSimpleResource`/`RequireString`), so a missing vault resource // disables the endpoint instead of panicking the api process at startup or // per-request. -var operatorIngestSecret = sync.OnceValue(func() string { +func readOperatorIngestSecret() string { res, err := server.Vault.SimpleResource("provider_egress.yml") if err != nil { glog.Infof("[pegl]no provider_egress.yml in the vault; ingest endpoint disabled\n") @@ -41,7 +47,7 @@ var operatorIngestSecret = sync.OnceValue(func() string { return "" } return values[0] -}) +} // ProviderEgressLocationSubmit ingests a probed provider egress location from // the operator's prober. See diff --git a/api/handlers/provider_egress_location_handlers_test.go b/api/handlers/provider_egress_location_handlers_test.go index a2afd35f..4af00971 100644 --- a/api/handlers/provider_egress_location_handlers_test.go +++ b/api/handlers/provider_egress_location_handlers_test.go @@ -5,7 +5,11 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/controller" ) func TestProviderEgressLocationSubmitRejectsMissingSecret(t *testing.T) { @@ -36,3 +40,111 @@ func TestProviderEgressLocationSubmitRejectsWrongSecret(t *testing.T) { t.Fatalf("status = %d, want 401 on a wrong operator secret", w.Code) } } + +// withStubOperatorIngestSecret swaps the package-level operatorIngestSecret +// memo for a stub that always returns secret, and returns a func to restore +// the original (real, still-memoized) reader. This lets a test exercise the +// "configured vault" path without the sync.OnceValue in the real reader ever +// touching the vault, and without the reject tests above (which rely on an +// unconfigured vault) observing any change. +func withStubOperatorIngestSecret(secret string) (restore func()) { + prev := operatorIngestSecret + operatorIngestSecret = func() string { return secret } + return func() { operatorIngestSecret = prev } +} + +// TestProviderEgressLocationSubmitAcceptsCorrectSecret proves the auth gate +// can ACCEPT a correct secret and hand off to the controller. Without this +// test, the two reject tests above (which both run with the vault +// unconfigured and take the secret=="" short-circuit) would pass unchanged +// even if the handler's entire body were replaced with an unconditional 401 - +// hmac.Equal would never be proven to run on a real match. +// +// Clearing auth hands the request to controller.SubmitProviderEgressLocation, +// which looks up the client in the database before it can return "Unknown +// client.", so this test needs a real (throwaway) test database - see +// server.DefaultTestEnv, the same harness +// controller/provider_egress_location_controller_test.go uses. t.Setenv makes +// it self-sufficient under a plain `go test`, matching the pattern in +// router/warp_handlers_status_test.go. +func TestProviderEgressLocationSubmitAcceptsCorrectSecret(t *testing.T) { + t.Setenv("WARP_ENV", "local") + server.DefaultTestEnv().Run(t, func(t testing.TB) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + // A syntactically valid, semantically unregistered submission: once + // auth clears, the controller looks up the client and (since it does + // not exist in the fresh test database) returns "Unknown client.", + // surfaced by the handler as 400. That 400 is proof the request + // reached the controller, i.e. proof auth passed. + args := controller.SubmitProviderEgressLocationArgs{ + ClientId: server.NewId(), + CountryCode: "US", + Country: "United States", + CountryConfident: true, + ObservedAt: server.NowUtc(), + } + body, err := json.Marshal(args) + if err != nil { + t.Fatalf("marshal args: %s", err) + } + + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-location", bytes.NewReader(body)) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationSubmit(w, req) + + if w.Code == http.StatusUnauthorized { + t.Fatalf("status = %d, want the correct secret to clear auth (not 401)", w.Code) + } + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 (Unknown client.) once auth clears for an unregistered client id; body = %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Unknown client.") { + t.Fatalf("body = %q, want it to report the unknown client", w.Body.String()) + } + }) +} + +// TestProviderEgressLocationSubmitRejectsAlteredSecret proves that once the +// vault is configured, hmac.Equal is actually consulted rather than the +// endpoint accepting any request once secret != "". Same configured secret as +// the accept test above, but the request carries a one-character-altered +// value. +func TestProviderEgressLocationSubmitRejectsAlteredSecret(t *testing.T) { + const secret = "correct-operator-secret-0123456789" + const wrongSecret = "correct-operator-secret-0123456780" // last char changed + defer withStubOperatorIngestSecret(secret)() + + body, _ := json.Marshal(map[string]any{ + "client_id": "019f8835-158d-6fd8-e9dd-fd0e4c6d6792", + }) + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-location", bytes.NewReader(body)) + req.Header.Set(operatorSecretHeader, wrongSecret) + w := httptest.NewRecorder() + + ProviderEgressLocationSubmit(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 when the operator secret is configured but the request's secret is wrong", w.Code) + } +} + +// TestProviderEgressLocationSubmitReadsSecretFromVault proves the vault +// plumbing itself - not the test stub - returns the configured secret: +// readOperatorIngestSecret (the un-memoized reader) reads a +// PushSimpleResource-injected provider_egress.yml. +func TestProviderEgressLocationSubmitReadsSecretFromVault(t *testing.T) { + const secret = "vault-provisioned-secret-abcdef" + pop := server.Vault.PushSimpleResource( + "provider_egress.yml", + []byte(`ingest_secret: "`+secret+`"`), + ) + defer pop() + + if got := readOperatorIngestSecret(); got != secret { + t.Fatalf("readOperatorIngestSecret() = %q, want %q", got, secret) + } +} diff --git a/beta-vault/vault/provider_egress.yml.example b/beta-vault/vault/provider_egress.yml.example index 9402cafa..2024763f 100644 --- a/beta-vault/vault/provider_egress.yml.example +++ b/beta-vault/vault/provider_egress.yml.example @@ -3,4 +3,7 @@ # header. Generate with: openssl rand -base64 32 # If this file or key is absent, the ingest endpoint rejects every request # (fails closed, does not crash the api process). +# The secret is read once, on the first request, and memoized for the life of +# the process: after creating or rotating this file, restart the api for the +# new value to take effect. ingest_secret: "replace-me" From 010bf9170876ed32d5f9b12a9393f866eccc1a7e Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 05:31:52 +0100 Subject: [PATCH 08/19] feat(controller): prefer probed provider egress location over mmdb --- controller/network_client_controller.go | 30 +++++++ controller/network_client_controller_test.go | 94 ++++++++++++++++++++ model/provider_egress_location_model.go | 18 ++++ 3 files changed, 142 insertions(+) create mode 100644 controller/network_client_controller_test.go diff --git a/controller/network_client_controller.go b/controller/network_client_controller.go index 5ecd7252..5bd46cf8 100644 --- a/controller/network_client_controller.go +++ b/controller/network_client_controller.go @@ -56,6 +56,36 @@ func SetConnectionLocation( connectionId server.Id, clientIp string, ) error { + // a provider probed through its own egress is located from that probe, not + // from a lookup on its control-connection ip: the egress is where user + // traffic actually exits, and the probed value is cross-checked across + // several sources. see + // docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md + if clientId := model.GetNetworkClientForConnection(ctx, connectionId); clientId != nil { + if egress := model.GetFreshProviderEgressLocation( + ctx, + *clientId, + model.ProviderEgressLocationMaxAge, + ); egress != nil { + scores := &model.ConnectionLocationScores{} + if egress.Hosting { + scores.NetTypeHosting = 1 + } + if egress.Proxy { + scores.NetTypePrivacy = 1 + } + if egress.Mobile { + scores.NetTypeVirtual = 1 + } + err := model.SetConnectionLocation(ctx, connectionId, egress.LocationId, scores) + if err == nil { + return nil + } + // fall through to the mmdb path on a storage error + glog.Infof("[ncc][%s]could not set probed egress location. err = %s\n", connectionId, err) + } + } + location, connectionLocationScores, err := GetLocationForIp(ctx, clientIp) if err != nil { // server.Logger().Printf("Get ip for location error: %s", err) diff --git a/controller/network_client_controller_test.go b/controller/network_client_controller_test.go new file mode 100644 index 00000000..863f412c --- /dev/null +++ b/controller/network_client_controller_test.go @@ -0,0 +1,94 @@ +package controller + +import ( + "context" + "testing" + + "github.com/go-playground/assert/v2" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/model" +) + +// A provider with a fresh probed egress location must be located from that +// entry, not from the mmdb lookup on its control ip. +func TestSetConnectionLocationPrefersEgressLocation(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + // the probed egress location: japan + probed := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: "Japan", + CountryCode: "jp", + } + model.CreateLocation(ctx, probed) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, "8.8.8.8:0", handlerId) + assert.Equal(t, err, nil) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "jp", + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, "8.8.8.8") + assert.Equal(t, err, nil) + + var countryLocationId server.Id + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT country_location_id FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&countryLocationId)) + } + }) + }) + assert.Equal(t, countryLocationId, probed.CountryLocationId) + }) +} + +// With no probed entry, the existing mmdb path still applies. +func TestSetConnectionLocationFallsBackToMmdb(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, "8.8.8.8:0", handlerId) + assert.Equal(t, err, nil) + + // no SetProviderEgressLocation call -> mmdb path + err = SetConnectionLocation(ctx, connectionId, "8.8.8.8") + assert.Equal(t, err, nil) + + var count int + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT COUNT(*) FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&count)) + } + }) + }) + assert.Equal(t, count, 1) + }) +} diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 2d78d751..c12950a6 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -128,6 +128,24 @@ func GetProviderEgressLocation(ctx context.Context, clientId server.Id) *Provide return e } +// GetNetworkClientForConnection returns the client id for a connection, or nil. +func GetNetworkClientForConnection(ctx context.Context, connectionId server.Id) *server.Id { + var clientId *server.Id + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query( + ctx, + `SELECT client_id FROM network_client_connection WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, err, func() { + if result.Next() { + server.Raise(result.Scan(&clientId)) + } + }) + }) + return clientId +} + // GetFreshProviderEgressLocation is GetProviderEgressLocation, filtered to // entries probed within maxAge. The cutoff is computed in Go and bound as a // parameter: observed_at is a naive timestamp holding utc, and comparing it From 2a58b1efe073aa9a15830c45b324fc13d1787a4e Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 05:56:34 +0100 Subject: [PATCH 09/19] fix(controller): collapse egress lookup to one query, add ARIN foreign parity, cover unprotected branches SetConnectionLocation ran on the connect-announce hot path (every client, every connection, inside a retry loop) with two separate DB round trips before ever reaching the mmdb fallback. Fix review findings against the probed-egress location change: - model.GetFreshProviderEgressLocationForConnection replaces the GetNetworkClientForConnection + GetFreshProviderEgressLocation pair with a single query joining network_client_connection to provider_egress_location. Freshness cutoff is still computed/compared in Go, not SQL now(). Cuts the probed-hit path from 3 DB round trips to 2, and the fallback path from 4 to 3. - The probed path now also runs the ARIN org-vs-country foreign check (against the probed country code), matching the mmdb path's GetLocationForIp, so net_type_foreign no longer gives probed providers an unearned ranking advantage over unprobed ones. Factored into a shared arinForeignScore helper; on any ARIN/ip-parse failure it just leaves NetTypeForeign at 0, never erroring or panicking the hot path. - Added tests for the stale-probed fallback, a probed write error (bad location_id) falling back to mmdb without panicking, and the Hosting/Proxy flag-to-score mapping. Verified the stale-fallback test has teeth by temporarily widening maxAge and confirming it fails. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- controller/network_client_controller.go | 57 +++--- controller/network_client_controller_test.go | 169 ++++++++++++++++++ .../network_client_location_controller.go | 39 ++-- model/provider_egress_location_model.go | 68 +++++++ 4 files changed, 297 insertions(+), 36 deletions(-) diff --git a/controller/network_client_controller.go b/controller/network_client_controller.go index 5bd46cf8..d5b457cd 100644 --- a/controller/network_client_controller.go +++ b/controller/network_client_controller.go @@ -2,6 +2,7 @@ package controller import ( "context" + "net/netip" // "strings" "time" @@ -61,29 +62,41 @@ func SetConnectionLocation( // traffic actually exits, and the probed value is cross-checked across // several sources. see // docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md - if clientId := model.GetNetworkClientForConnection(ctx, connectionId); clientId != nil { - if egress := model.GetFreshProviderEgressLocation( - ctx, - *clientId, - model.ProviderEgressLocationMaxAge, - ); egress != nil { - scores := &model.ConnectionLocationScores{} - if egress.Hosting { - scores.NetTypeHosting = 1 - } - if egress.Proxy { - scores.NetTypePrivacy = 1 - } - if egress.Mobile { - scores.NetTypeVirtual = 1 - } - err := model.SetConnectionLocation(ctx, connectionId, egress.LocationId, scores) - if err == nil { - return nil - } - // fall through to the mmdb path on a storage error - glog.Infof("[ncc][%s]could not set probed egress location. err = %s\n", connectionId, err) + // a single query joining network_client_connection to + // provider_egress_location: this runs for every connection (provider or + // not) on the connect-announce path and inside a retry loop, so it must + // not cost the two round trips (client lookup, then egress lookup) the + // naive version would. + if egress := model.GetFreshProviderEgressLocationForConnection( + ctx, + connectionId, + model.ProviderEgressLocationMaxAge, + ); egress != nil { + scores := &model.ConnectionLocationScores{} + if egress.Hosting { + scores.NetTypeHosting = 1 + } + if egress.Proxy { + scores.NetTypePrivacy = 1 + } + if egress.Mobile { + scores.NetTypeVirtual = 1 + } + // keep the ARIN org-vs-country foreign check on the probed path too, + // so a probed provider is ranked on equal terms with an equivalent + // unprobed one (net_type_foreign feeds the ranking columns). Compare + // against the probed country -- that's the country now being + // claimed. Any lookup failure just leaves NetTypeForeign at 0; it + // must never fail or panic this path. + if addr, err := netip.ParseAddr(clientIp); err == nil { + scores.NetTypeForeign = arinForeignScore(addr, egress.CountryCode) + } + err := model.SetConnectionLocation(ctx, connectionId, egress.LocationId, scores) + if err == nil { + return nil } + // fall through to the mmdb path on a storage error + glog.Infof("[ncc][%s]could not set probed egress location. err = %s\n", connectionId, err) } location, connectionLocationScores, err := GetLocationForIp(ctx, clientIp) diff --git a/controller/network_client_controller_test.go b/controller/network_client_controller_test.go index 863f412c..0f476477 100644 --- a/controller/network_client_controller_test.go +++ b/controller/network_client_controller_test.go @@ -3,6 +3,7 @@ package controller import ( "context" "testing" + "time" "github.com/go-playground/assert/v2" @@ -92,3 +93,171 @@ func TestSetConnectionLocationFallsBackToMmdb(t *testing.T) { assert.Equal(t, count, 1) }) } + +// A probed egress location observed longer ago than ProviderEgressLocationMaxAge +// must be ignored, falling back to the mmdb lookup exactly as if there were no +// probed entry at all. +func TestSetConnectionLocationStaleProbedFallsBackToMmdb(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientIp := "8.8.8.8" + + // the mmdb location this ip actually resolves to; created up front so + // its canonical (deduped) location id is known for the assertion. + mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) + assert.Equal(t, err, nil) + model.CreateLocation(ctx, mmdbLocation) + + // a stale probed location, deliberately a different country than + // whatever the mmdb lookup returns, so a wrongly-preferred probed + // value would be caught by the assertion below. + probed := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: "Japan", + CountryCode: "jp", + } + model.CreateLocation(ctx, probed) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) + assert.Equal(t, err, nil) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "jp", + ObservedAt: server.NowUtc().Add(-(model.ProviderEgressLocationMaxAge + time.Hour)), + }) + + err = SetConnectionLocation(ctx, connectionId, clientIp) + assert.Equal(t, err, nil) + + var countryLocationId server.Id + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT country_location_id FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&countryLocationId)) + } + }) + }) + assert.Equal(t, countryLocationId, mmdbLocation.CountryLocationId) + if countryLocationId == probed.CountryLocationId { + t.Fatal("stale probed location must not be used") + } + }) +} + +// A probed egress location whose location_id points at no existing location +// row makes the storage write fail (SetConnectionLocation returns an error +// rather than panicking, see model.SetConnectionLocation). The connection +// must still end up located via the mmdb path, and SetConnectionLocation +// itself must not panic or error out on this. +func TestSetConnectionLocationProbedWriteErrorFallsBackToMmdb(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientIp := "8.8.8.8" + + mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) + assert.Equal(t, err, nil) + model.CreateLocation(ctx, mmdbLocation) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) + assert.Equal(t, err, nil) + + // a fresh probed entry pointing at a location id that was never + // created via CreateLocation -- the storage write in + // model.SetConnectionLocation must fail cleanly on this, not panic. + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: server.NewId(), + CountryCode: "jp", + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, clientIp) + assert.Equal(t, err, nil) + + var countryLocationId server.Id + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT country_location_id FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&countryLocationId)) + } + }) + }) + assert.Equal(t, countryLocationId, mmdbLocation.CountryLocationId) + }) +} + +// A fresh probed location's Hosting/Proxy flags must map onto the stored +// connection's net_type_hosting/net_type_privacy scores. +func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + probed := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: "Japan", + CountryCode: "jp", + } + model.CreateLocation(ctx, probed) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, "8.8.8.8:0", handlerId) + assert.Equal(t, err, nil) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "jp", + Hosting: true, + Proxy: true, + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, "8.8.8.8") + assert.Equal(t, err, nil) + + var netTypeHosting int + var netTypePrivacy int + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT net_type_hosting, net_type_privacy FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&netTypeHosting, &netTypePrivacy)) + } + }) + }) + assert.Equal(t, netTypeHosting, 1) + assert.Equal(t, netTypePrivacy, 1) + }) +} diff --git a/controller/network_client_location_controller.go b/controller/network_client_location_controller.go index 3376a9af..0535db77 100644 --- a/controller/network_client_location_controller.go +++ b/controller/network_client_location_controller.go @@ -55,21 +55,32 @@ func GetLocationForIp(ctx context.Context, clientIp string) (*model.Location, *m connectionLocationScores.NetTypeVirtual = 1 } + connectionLocationScores.NetTypeForeign = arinForeignScore(addr, ipInfo.CountryCode) + + return location, connectionLocationScores, nil +} + +// arinForeignScore cross-checks the ARIN org registration country for addr +// against countryCode, the country code being claimed for this connection +// (the mmdb-resolved country on the ordinary path, or the probed egress +// country on the provider-egress path). If the org's registered country +// differs, the use case is considered foreign (VPN/proxy-like), matching the +// heuristic previously inlined in GetLocationForIp. +// +// If the ARIN lookup fails, this returns 0 without error: it must never fail +// or panic a caller on the connect-announce hot path over a missing/failed +// foreign check. +func arinForeignScore(addr netip.Addr, countryCode string) int { arinInfo, err := server.GetArinInfo(addr) - if err == nil { - // if the org ownership does not match the ip country, - // we consider the use case of the ip to be virtual - foreign := false - for _, orgCountryCode := range arinInfo.OrgCountryCodes { - if orgCountryCode != ipInfo.CountryCode { - foreign = true - break - } - } - if foreign { - connectionLocationScores.NetTypeForeign = 1 + if err != nil { + return 0 + } + // if the org ownership does not match the claimed country, + // we consider the use case of the ip to be foreign + for _, orgCountryCode := range arinInfo.OrgCountryCodes { + if orgCountryCode != countryCode { + return 1 } } - - return location, connectionLocationScores, nil + return 0 } diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index c12950a6..f7d0c69f 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -165,6 +165,74 @@ func GetFreshProviderEgressLocation( return e } +// GetFreshProviderEgressLocationForConnection resolves the probed provider +// egress location for a connection in a single query, joining +// network_client_connection to provider_egress_location on client_id. This +// exists for the connect-announce hot path (SetConnectionLocation), which +// previously spent two round trips per connection -- GetNetworkClientForConnection +// then GetFreshProviderEgressLocation -- before ever reaching the mmdb +// fallback; collapsing to one query matters on a path that runs for every +// connection and inside a retry loop. +// +// As with GetFreshProviderEgressLocation, the maxAge cutoff is computed in Go +// and compared in Go: observed_at is a naive timestamp holding utc, and +// comparing it against sql now() would cast through the session timezone. +func GetFreshProviderEgressLocationForConnection( + ctx context.Context, + connectionId server.Id, + maxAge time.Duration, +) *ProviderEgressLocation { + var e *ProviderEgressLocation + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query( + ctx, + ` + SELECT + pel.client_id, + pel.location_id, + pel.country_code, + pel.asn, + pel.org, + pel.hosting, + pel.proxy, + pel.mobile, + pel.city_confident, + pel.observed_at, + pel.update_time + FROM network_client_connection ncc + INNER JOIN provider_egress_location pel ON pel.client_id = ncc.client_id + WHERE ncc.connection_id = $1 + `, + connectionId, + ) + server.WithPgResult(result, err, func() { + if result.Next() { + e = &ProviderEgressLocation{} + server.Raise(result.Scan( + &e.ClientId, + &e.LocationId, + &e.CountryCode, + &e.ASN, + &e.Org, + &e.Hosting, + &e.Proxy, + &e.Mobile, + &e.CityConfident, + &e.ObservedAt, + &e.UpdateTime, + )) + } + }) + }) + if e == nil { + return nil + } + if e.ObservedAt.Before(server.NowUtc().Add(-maxAge)) { + return nil + } + return e +} + // GetLocation returns the canonical location row, or nil. // // Note: the location table also has a location_name column, but it holds the From 35ba895e8810af2858cef367884f402827e4a7bf Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 05:58:30 +0100 Subject: [PATCH 10/19] feat(taskworker): sweep expired provider egress locations --- taskworker/taskworker.go | 5 ++ .../work/provider_egress_location_work.go | 49 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 taskworker/work/provider_egress_location_work.go diff --git a/taskworker/taskworker.go b/taskworker/taskworker.go index 3364dce0..9cbc1c05 100644 --- a/taskworker/taskworker.go +++ b/taskworker/taskworker.go @@ -52,6 +52,7 @@ func InitTasks(ctx context.Context) { work.ScheduleRemoveExpiredAuthAttempts(clientSession, tx) work.ScheduleRemoveExpiredWalletAuthChallenges(clientSession, tx) work.ScheduleRemoveExpiredWalletNonces(clientSession, tx) + work.ScheduleRemoveExpiredProviderEgressLocations(clientSession, tx) work.ScheduleRemoveOldAuditNetworkEvents(clientSession, tx) work.ScheduleRemoveOldAuditEvents(clientSession, tx) work.ScheduleRemoveOldClientReliabilityStats(clientSession, tx) @@ -236,6 +237,10 @@ func InitTaskWorkerWithSettings(ctx context.Context, settings *task.TaskWorkerSe work.RemoveExpiredWalletNoncesPost, "github.com/urnetwork/server/taskworker/work.RemoveExpiredWalletNonces", ), + task.NewTaskTargetWithPost( + work.RemoveExpiredProviderEgressLocations, + work.RemoveExpiredProviderEgressLocationsPost, + ), task.NewTaskTargetWithPost( work.RemoveOldAuditNetworkEvents, work.RemoveOldAuditNetworkEventsPost, diff --git a/taskworker/work/provider_egress_location_work.go b/taskworker/work/provider_egress_location_work.go new file mode 100644 index 00000000..94901c34 --- /dev/null +++ b/taskworker/work/provider_egress_location_work.go @@ -0,0 +1,49 @@ +package work + +import ( + "time" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/model" + "github.com/urnetwork/server/session" + "github.com/urnetwork/server/task" +) + +type RemoveExpiredProviderEgressLocationsArgs struct{} + +type RemoveExpiredProviderEgressLocationsResult struct{} + +func ScheduleRemoveExpiredProviderEgressLocations(clientSession *session.ClientSession, tx server.PgTx) { + task.ScheduleTaskInTx( + tx, + RemoveExpiredProviderEgressLocations, + &RemoveExpiredProviderEgressLocationsArgs{}, + clientSession, + task.RunOnce("remove_expired_provider_egress_locations"), + task.RunAt(server.NowUtc().Add(6*time.Hour)), + ) +} + +// RemoveExpiredProviderEgressLocations drops probed locations well past their +// trust window, so a provider that stops being probed eventually falls back to +// the mmdb path instead of being pinned to a stale location forever. The cutoff +// is deliberately looser than ProviderEgressLocationMaxAge: reads already +// ignore stale rows, so this is only reclaiming storage. +func RemoveExpiredProviderEgressLocations( + _ *RemoveExpiredProviderEgressLocationsArgs, + clientSession *session.ClientSession, +) (*RemoveExpiredProviderEgressLocationsResult, error) { + minObservedAt := server.NowUtc().Add(-4 * model.ProviderEgressLocationMaxAge) + model.RemoveExpiredProviderEgressLocations(clientSession.Ctx, minObservedAt) + return &RemoveExpiredProviderEgressLocationsResult{}, nil +} + +func RemoveExpiredProviderEgressLocationsPost( + _ *RemoveExpiredProviderEgressLocationsArgs, + _ *RemoveExpiredProviderEgressLocationsResult, + clientSession *session.ClientSession, + tx server.PgTx, +) error { + ScheduleRemoveExpiredProviderEgressLocations(clientSession, tx) + return nil +} From 211e5a18ba0e87ad233320a37e666bd6e2c44e11 Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 06:49:45 +0100 Subject: [PATCH 11/19] fix(provider-egress): correct ARIN foreign parity, reject corrupting/oversized submissions, wire up ingest secret Pre-merge fixes from the final whole-branch review of provider-egress geolocation: - Fix the probed-path ARIN "foreign" check: it compared the control ip's ARIN org country against the probed egress country (two different ips), flagging a provider foreign precisely when probing changed the answer. Now matches the mmdb path exactly: ARIN org country of the control ip vs. the mmdb country of that same control ip, restoring probed/unprobed ranking parity. - Reject provider-egress submissions with an empty (post-trim) Country/City/Region instead of silently creating a permanently-blank canonical location row via CreateLocation's dedupe-on-name behavior. - Reject over-long Country/City/Region (>128) or Org (>256) instead of panicking inside CreateLocation on a Postgres "value too long" error. - Make SetProviderEgressLocation's upsert monotonic in observed_at so a replayed older submission cannot clobber a newer stored row. - Replace three production-comment references to the (upstream-absent) design-spec doc path with inline prose, and drop the hardcoded beta vault filesystem path from the ingest-secret handler comment. - Delete GetNetworkClientForConnection (model/provider_egress_location_model.go), a fully superseded, zero-caller helper. - Provision the ingest secret end-to-end: beta-setup.sh now generates beta-vault/vault/provider_egress.yml (guarded the same way as the other generated secrets), and BETA.md documents it plus the api-restart requirement, since the endpoint otherwise ships silently disabled. Adds tests for each behavioral fix; all pass, including TestSetConnectionLocationToleratesCountryOnlyLocation. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- .../provider_egress_location_handlers.go | 10 +- controller/network_client_controller.go | 24 ++- controller/network_client_controller_test.go | 63 ++++++++ .../provider_egress_location_controller.go | 61 +++++++- ...rovider_egress_location_controller_test.go | 142 ++++++++++++++++++ db_migrations.go | 15 +- model/provider_egress_location_model.go | 32 ++-- model/provider_egress_location_model_test.go | 58 ++++++- 8 files changed, 358 insertions(+), 47 deletions(-) diff --git a/api/handlers/provider_egress_location_handlers.go b/api/handlers/provider_egress_location_handlers.go index 3271b167..b843c05b 100644 --- a/api/handlers/provider_egress_location_handlers.go +++ b/api/handlers/provider_egress_location_handlers.go @@ -28,7 +28,7 @@ const maxProviderEgressLocationBody = 16 * 1024 var operatorIngestSecret func() string = sync.OnceValue(readOperatorIngestSecret) // readOperatorIngestSecret reads the operator ingest secret from the vault -// (beta-vault/vault/provider_egress.yml, key "ingest_secret"). It returns "" +// resource "provider_egress.yml", key "ingest_secret". It returns "" // when the vault resource is absent, the key is absent, or the key is empty, // which makes the endpoint fail closed (every request is rejected) rather // than open. `SimpleResource`/`String` are the non-panicking lookups (unlike @@ -50,8 +50,12 @@ func readOperatorIngestSecret() string { } // ProviderEgressLocationSubmit ingests a probed provider egress location from -// the operator's prober. See -// docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md. +// the operator's prober. The prober routes geolocation lookups through a +// provider's own egress -- rather than relying on a lookup against the +// provider's control-connection ip -- and submits the result here so the +// server can prefer it over the built-in mmdb lookup. The route is +// operator-to-server, authenticated by the shared secret above rather than a +// network jwt. func ProviderEgressLocationSubmit(w http.ResponseWriter, r *http.Request) { secret := operatorIngestSecret() provided := r.Header.Get(operatorSecretHeader) diff --git a/controller/network_client_controller.go b/controller/network_client_controller.go index d5b457cd..382aa913 100644 --- a/controller/network_client_controller.go +++ b/controller/network_client_controller.go @@ -59,9 +59,11 @@ func SetConnectionLocation( ) error { // a provider probed through its own egress is located from that probe, not // from a lookup on its control-connection ip: the egress is where user - // traffic actually exits, and the probed value is cross-checked across - // several sources. see - // docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md + // traffic actually exits, and an operator-run prober learns it by routing + // geolocation lookups through the provider itself and cross-checking them + // across several sources, then submits the result here. When a fresh + // probed entry exists we prefer it over the built-in mmdb lookup on the + // control ip. GetFreshProviderEgressLocationForConnection is // a single query joining network_client_connection to // provider_egress_location: this runs for every connection (provider or // not) on the connect-announce path and inside a retry loop, so it must @@ -84,12 +86,18 @@ func SetConnectionLocation( } // keep the ARIN org-vs-country foreign check on the probed path too, // so a probed provider is ranked on equal terms with an equivalent - // unprobed one (net_type_foreign feeds the ranking columns). Compare - // against the probed country -- that's the country now being - // claimed. Any lookup failure just leaves NetTypeForeign at 0; it - // must never fail or panic this path. + // unprobed one (net_type_foreign feeds the ranking columns). Compute + // it exactly as the mmdb path does in GetLocationForIp: the ARIN org + // country of the control ip against the mmdb country of that SAME + // control ip -- not the probed country, which is a different + // question (whether probing changed the answer) and must not be + // silently folded into this ranking penalty. Any lookup failure + // just leaves NetTypeForeign at 0; it must never fail or panic this + // path. if addr, err := netip.ParseAddr(clientIp); err == nil { - scores.NetTypeForeign = arinForeignScore(addr, egress.CountryCode) + if ipInfo, err := server.GetIpInfo(addr); err == nil { + scores.NetTypeForeign = arinForeignScore(addr, ipInfo.CountryCode) + } } err := model.SetConnectionLocation(ctx, connectionId, egress.LocationId, scores) if err == nil { diff --git a/controller/network_client_controller_test.go b/controller/network_client_controller_test.go index 0f476477..ea6c593c 100644 --- a/controller/network_client_controller_test.go +++ b/controller/network_client_controller_test.go @@ -210,6 +210,69 @@ func TestSetConnectionLocationProbedWriteErrorFallsBackToMmdb(t *testing.T) { }) } +// net_type_foreign on the probed path must be computed the same way as the +// mmdb path: the ARIN org country of the control ip against the mmdb country +// of that SAME control ip, not against the probed country. 8.8.8.8 resolves +// (both via mmdb and ARIN org registration) to "us", so it is non-foreign by +// construction -- this is the same ip used by the parity tests above. The +// probed country here is deliberately "jp", a different country than the +// control ip's mmdb country: probing having changed the answer must not, by +// itself, flip net_type_foreign, or a probed provider is penalized a full +// ranking tier precisely for the reason this feature exists. A prior version +// of this code compared the ARIN org country against the probed country +// instead of the control ip's mmdb country, which made this exact scenario +// (egress country differs from control-ip country) foreign=1 instead of the +// correct foreign=0. +func TestSetConnectionLocationProbedNetTypeForeignMatchesMmdbParity(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientIp := "8.8.8.8" + + probed := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: "Japan", + CountryCode: "jp", + } + model.CreateLocation(ctx, probed) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) + assert.Equal(t, err, nil) + + // probed country ("jp") deliberately differs from the control ip's + // mmdb country ("us" for 8.8.8.8). + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "jp", + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, clientIp) + assert.Equal(t, err, nil) + + var netTypeForeign int + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + `SELECT net_type_foreign FROM network_client_location WHERE connection_id = $1`, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan(&netTypeForeign)) + } + }) + }) + assert.Equal(t, netTypeForeign, 0) + }) +} + // A fresh probed location's Hosting/Proxy flags must map onto the stored // connection's net_type_hosting/net_type_privacy scores. func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { diff --git a/controller/provider_egress_location_controller.go b/controller/provider_egress_location_controller.go index 57a11d78..169de94a 100644 --- a/controller/provider_egress_location_controller.go +++ b/controller/provider_egress_location_controller.go @@ -14,6 +14,17 @@ import ( // already older than this when it arrives. It bounds replay of an old probe. const MaxProviderEgressLocationSubmissionAge = 24 * time.Hour +// maxLocationNameLen bounds country/city/region as submitted: these flow into +// model.CreateLocation, whose location_name column is varchar(128). Rejecting +// an over-long value here with a clear error is preferable to letting +// CreateLocation panic on a Postgres "value too long for type character +// varying(128)" error. +const maxLocationNameLen = 128 + +// maxOrgLen mirrors maxLocationNameLen for org, which is stored in +// provider_egress_location.org, a varchar(256) column. +const maxOrgLen = 256 + type SubmitProviderEgressLocationArgs struct { ClientId server.Id `json:"client_id"` CountryCode string `json:"country_code"` @@ -59,19 +70,59 @@ func SubmitProviderEgressLocation( return nil, fmt.Errorf("Unknown client.") } + // country is always used to resolve/create a location row (at minimum + // the country-granular one), and model.CreateLocation dedupes country + // rows on (location_type, country_code): an empty name here would create + // a canonical row with location_name='' that every later lookup for this + // country reuses forever, even after a subsequent real mmdb lookup. Reject + // rather than silently falling back, so the prober learns it sent a bad + // payload instead of the server permanently corrupting shared data. + country := strings.TrimSpace(args.Country) + if country == "" { + return nil, fmt.Errorf("Missing country.") + } + if maxLocationNameLen < len(country) { + return nil, fmt.Errorf("Country is too long.") + } + if maxOrgLen < len(args.Org) { + return nil, fmt.Errorf("Org is too long.") + } + + // city/region are only used (and their rows only created) when the probe + // was city-confident; the same empty-name corruption applies to them, so + // require both are present and reject rather than silently dropping to + // country granularity on a bad payload. + var city, region string + if args.CityConfident { + city = strings.TrimSpace(args.City) + region = strings.TrimSpace(args.Region) + if city == "" { + return nil, fmt.Errorf("Missing city for a city-confident submission.") + } + if region == "" { + return nil, fmt.Errorf("Missing region for a city-confident submission.") + } + if maxLocationNameLen < len(city) { + return nil, fmt.Errorf("City is too long.") + } + if maxLocationNameLen < len(region) { + return nil, fmt.Errorf("Region is too long.") + } + } + // resolve to a canonical location row. city granularity only when the // probe agreed on a city; otherwise country. location := &model.Location{ LocationType: model.LocationTypeCountry, - Country: args.Country, + Country: country, CountryCode: countryCode, } - if args.CityConfident && args.City != "" { + if args.CityConfident { location = &model.Location{ LocationType: model.LocationTypeCity, - City: args.City, - Region: args.Region, - Country: args.Country, + City: city, + Region: region, + Country: country, CountryCode: countryCode, } } diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go index 9f8c70a4..25a04c6d 100644 --- a/controller/provider_egress_location_controller_test.go +++ b/controller/provider_egress_location_controller_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "strings" "testing" "time" @@ -132,6 +133,147 @@ func TestSubmitProviderEgressLocationCityConfidentStoresCity(t *testing.T) { }) } +// An empty Country must be rejected, not silently stored: model.CreateLocation +// dedupes country rows on (location_type, country_code), so an empty name +// would create a canonical, permanently-blank row that every later lookup for +// that country reuses forever. +func TestSubmitProviderEgressLocationRejectsEmptyCountry(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: " ", // blank after trimming + CountryConfident: true, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("an empty country must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + +// A city-confident submission with an empty City must be rejected rather than +// silently falling back to country granularity: the same empty-canonical-row +// corruption applies to city/region rows. +func TestSubmitProviderEgressLocationRejectsEmptyCityWhenCityConfident(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: "United States", + Region: "Colorado", + City: "", + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("a city-confident submission with an empty city must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + +// A city-confident submission with an empty Region must likewise be rejected: +// the region row is created with the same dedupe-on-empty-name hazard. +func TestSubmitProviderEgressLocationRejectsEmptyRegionWhenCityConfident(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: "United States", + Region: "", + City: "Denver", + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("a city-confident submission with an empty region must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + +// An over-long Country must be rejected with a clear error instead of +// panicking inside model.CreateLocation on a Postgres "value too long for +// type character varying(128)" error. +func TestSubmitProviderEgressLocationRejectsOverLongCountry(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: strings.Repeat("a", 129), + CountryConfident: true, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("an over-long country must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + +// An over-long Org must likewise be rejected rather than panicking inside +// storage. +func TestSubmitProviderEgressLocationRejectsOverLongOrg(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: "United States", + Org: strings.Repeat("a", 257), + CountryConfident: true, + ObservedAt: server.NowUtc(), + }) + if err == nil { + t.Fatal("an over-long org must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + func TestSubmitProviderEgressLocationRejectsStaleObservedAt(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() diff --git a/db_migrations.go b/db_migrations.go index 204afa4b..9ae8a614 100644 --- a/db_migrations.go +++ b/db_migrations.go @@ -4386,12 +4386,15 @@ var migrations = []any{ ON network_client (contract_time) WHERE (active = true AND source_client_id IS NULL AND contract_time IS NOT NULL) `), - // provider egress locations: locations learned by probing a provider's own - // egress (see docs/superpowers/specs/2026-07-24-provider-egress-geolocation-design.md). - // Keyed by client_id, one row per provider, upserted by the operator's - // prober. location_id is the canonical country (or city, when the probe was - // city-confident) location row. observed_at is when the probe ran, and is - // what freshness is judged against. + // provider egress locations: locations learned by an operator-run prober + // that routes geolocation lookups through a provider's own egress rather + // than trusting a lookup on the provider's control-connection ip, since + // the egress is where user traffic actually exits and can differ from + // where the provider's control connection originates (e.g. behind a VPN + // or hosting network). Keyed by client_id, one row per provider, upserted + // by the operator's prober. location_id is the canonical country (or + // city, when the probe was city-confident) location row. observed_at is + // when the probe ran, and is what freshness is judged against. newSqlMigration(` CREATE TABLE IF NOT EXISTS provider_egress_location ( client_id uuid NOT NULL PRIMARY KEY, diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index f7d0c69f..240bb8b4 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -29,7 +29,10 @@ type ProviderEgressLocation struct { UpdateTime time.Time } -// SetProviderEgressLocation upserts the probed location for a provider. +// SetProviderEgressLocation upserts the probed location for a provider. The +// upsert is monotonic in observed_at: a replayed or out-of-order submission +// older than what is already stored is silently dropped rather than +// clobbering a newer probe result. func SetProviderEgressLocation(ctx context.Context, e *ProviderEgressLocation) { // country codes are stored/compared lowercased (see CreateLocation in // network_client_location_model.go); the geolocation APIs that feed this @@ -66,6 +69,7 @@ func SetProviderEgressLocation(ctx context.Context, e *ProviderEgressLocation) { city_confident = $9, observed_at = $10, update_time = $11 + WHERE provider_egress_location.observed_at < EXCLUDED.observed_at `, e.ClientId, e.LocationId, @@ -128,24 +132,6 @@ func GetProviderEgressLocation(ctx context.Context, clientId server.Id) *Provide return e } -// GetNetworkClientForConnection returns the client id for a connection, or nil. -func GetNetworkClientForConnection(ctx context.Context, connectionId server.Id) *server.Id { - var clientId *server.Id - server.Db(ctx, func(conn server.PgConn) { - result, err := conn.Query( - ctx, - `SELECT client_id FROM network_client_connection WHERE connection_id = $1`, - connectionId, - ) - server.WithPgResult(result, err, func() { - if result.Next() { - server.Raise(result.Scan(&clientId)) - } - }) - }) - return clientId -} - // GetFreshProviderEgressLocation is GetProviderEgressLocation, filtered to // entries probed within maxAge. The cutoff is computed in Go and bound as a // parameter: observed_at is a naive timestamp holding utc, and comparing it @@ -169,10 +155,10 @@ func GetFreshProviderEgressLocation( // egress location for a connection in a single query, joining // network_client_connection to provider_egress_location on client_id. This // exists for the connect-announce hot path (SetConnectionLocation), which -// previously spent two round trips per connection -- GetNetworkClientForConnection -// then GetFreshProviderEgressLocation -- before ever reaching the mmdb -// fallback; collapsing to one query matters on a path that runs for every -// connection and inside a retry loop. +// previously spent two round trips per connection -- resolving the client id +// for the connection, then fetching its fresh egress location -- before ever +// reaching the mmdb fallback; collapsing to one query matters on a path that +// runs for every connection and inside a retry loop. // // As with GetFreshProviderEgressLocation, the maxAge cutoff is computed in Go // and compared in Go: observed_at is a naive timestamp holding utc, and diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index bf229eb5..874f5391 100644 --- a/model/provider_egress_location_model_test.go +++ b/model/provider_egress_location_model_test.go @@ -43,7 +43,9 @@ func TestProviderEgressLocationUpsertAndGet(t *testing.T) { assert.Equal(t, got.Hosting, true) assert.Equal(t, got.Proxy, false) - // upsert replaces + // upsert replaces, given a strictly newer observed_at: the upsert is + // monotonic (see TestProviderEgressLocationUpsertIgnoresOlderReplay below), + // so a second submission at the same observed_at would not win. SetProviderEgressLocation(ctx, &ProviderEgressLocation{ ClientId: clientId, LocationId: country.LocationId, @@ -51,7 +53,7 @@ func TestProviderEgressLocationUpsertAndGet(t *testing.T) { ASN: 999, Hosting: false, Proxy: true, - ObservedAt: now, + ObservedAt: now.Add(time.Minute), }) got = GetProviderEgressLocation(ctx, clientId) assert.Equal(t, got.ASN, 999) @@ -60,6 +62,58 @@ func TestProviderEgressLocationUpsertAndGet(t *testing.T) { }) } +// The upsert is monotonic in observed_at: a replayed submission older than +// what is already stored must not clobber the newer row. +func TestProviderEgressLocationUpsertIgnoresOlderReplay(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + usCountry := &Location{ + LocationType: LocationTypeCountry, + Country: "United States", + CountryCode: "us", + } + CreateLocation(ctx, usCountry) + + jpCountry := &Location{ + LocationType: LocationTypeCountry, + Country: "Japan", + CountryCode: "jp", + } + CreateLocation(ctx, jpCountry) + + clientId := server.NewId() + newer := server.NowUtc() + older := newer.Add(-1 * time.Hour) + + // the newer probe lands first + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: clientId, + LocationId: jpCountry.LocationId, + CountryCode: "jp", + ASN: 111, + ObservedAt: newer, + }) + + // a stale/replayed older probe arrives afterward and must not win + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: clientId, + LocationId: usCountry.LocationId, + CountryCode: "us", + ASN: 222, + ObservedAt: older, + }) + + got := GetProviderEgressLocation(ctx, clientId) + if got == nil { + t.Fatal("expected a stored egress location") + } + assert.Equal(t, got.CountryCode, "jp") + assert.Equal(t, got.ASN, 111) + assert.Equal(t, got.LocationId, jpCountry.LocationId) + }) +} + func TestProviderEgressLocationCountryCodeLowercased(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() From 1c7bebebf1fa670b6c4d211f1c2918cbe2cd9eef Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 07:10:05 +0100 Subject: [PATCH 12/19] chore: drop beta-only vault example from the upstream change The example config lives in this fork's beta deployment tree, which does not exist upstream. Operators configure the ingest secret through their own vault; the handler documents the resource name and key. --- beta-vault/vault/provider_egress.yml.example | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 beta-vault/vault/provider_egress.yml.example diff --git a/beta-vault/vault/provider_egress.yml.example b/beta-vault/vault/provider_egress.yml.example deleted file mode 100644 index 2024763f..00000000 --- a/beta-vault/vault/provider_egress.yml.example +++ /dev/null @@ -1,9 +0,0 @@ -# Operator ingest secret for POST /network/provider-egress-location. -# The operator's egress prober sends this value in the X-UR-Operator-Secret -# header. Generate with: openssl rand -base64 32 -# If this file or key is absent, the ingest endpoint rejects every request -# (fails closed, does not crash the api process). -# The secret is read once, on the first request, and memoized for the life of -# the process: after creating or rotating this file, restart the api for the -# new value to take effect. -ingest_secret: "replace-me" From 39f29346c42bd2619c1881841f7ed9784f7cd2b8 Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 07:25:36 +0100 Subject: [PATCH 13/19] test: port provider-egress tests off go-playground/assert to connect.AssertEqual The three provider-egress-location test files imported github.com/go-playground/assert/v2, which is present in this fork's go.mod but not in upstream's, breaking compilation on this upstream-based branch (no required module provides package github.com/go-playground/assert/v2). Upstream already has an equivalent helper, connect.AssertEqual (from github.com/urnetwork/connect, used throughout model/ and controller/ test files), so no new third-party dependency is needed. This is a mechanical 1:1 swap (assert.Equal(t, v1, v2) -> connect.AssertEqual(t, v1, v2), same argument order and semantics) with no change to test logic or assertions. None of the three files used assert.NotEqual. No go.mod/go.sum changes: go-playground/assert was never present in this branch's go.mod/go.sum to begin with. --- controller/network_client_controller_test.go | 45 +++++++++---------- ...rovider_egress_location_controller_test.go | 21 +++++---- model/provider_egress_location_model_test.go | 27 ++++++----- 3 files changed, 45 insertions(+), 48 deletions(-) diff --git a/controller/network_client_controller_test.go b/controller/network_client_controller_test.go index ea6c593c..17feb8c9 100644 --- a/controller/network_client_controller_test.go +++ b/controller/network_client_controller_test.go @@ -5,8 +5,7 @@ import ( "testing" "time" - "github.com/go-playground/assert/v2" - + "github.com/urnetwork/connect" "github.com/urnetwork/server" "github.com/urnetwork/server/model" ) @@ -31,7 +30,7 @@ func TestSetConnectionLocationPrefersEgressLocation(t *testing.T) { handlerId := model.CreateNetworkClientHandler(ctx) connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, "8.8.8.8:0", handlerId) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ ClientId: clientId, @@ -41,7 +40,7 @@ func TestSetConnectionLocationPrefersEgressLocation(t *testing.T) { }) err = SetConnectionLocation(ctx, connectionId, "8.8.8.8") - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) var countryLocationId server.Id server.Db(ctx, func(conn server.PgConn) { @@ -56,7 +55,7 @@ func TestSetConnectionLocationPrefersEgressLocation(t *testing.T) { } }) }) - assert.Equal(t, countryLocationId, probed.CountryLocationId) + connect.AssertEqual(t, countryLocationId, probed.CountryLocationId) }) } @@ -71,11 +70,11 @@ func TestSetConnectionLocationFallsBackToMmdb(t *testing.T) { handlerId := model.CreateNetworkClientHandler(ctx) connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, "8.8.8.8:0", handlerId) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) // no SetProviderEgressLocation call -> mmdb path err = SetConnectionLocation(ctx, connectionId, "8.8.8.8") - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) var count int server.Db(ctx, func(conn server.PgConn) { @@ -90,7 +89,7 @@ func TestSetConnectionLocationFallsBackToMmdb(t *testing.T) { } }) }) - assert.Equal(t, count, 1) + connect.AssertEqual(t, count, 1) }) } @@ -106,7 +105,7 @@ func TestSetConnectionLocationStaleProbedFallsBackToMmdb(t *testing.T) { // the mmdb location this ip actually resolves to; created up front so // its canonical (deduped) location id is known for the assertion. mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) model.CreateLocation(ctx, mmdbLocation) // a stale probed location, deliberately a different country than @@ -125,7 +124,7 @@ func TestSetConnectionLocationStaleProbedFallsBackToMmdb(t *testing.T) { handlerId := model.CreateNetworkClientHandler(ctx) connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ ClientId: clientId, @@ -135,7 +134,7 @@ func TestSetConnectionLocationStaleProbedFallsBackToMmdb(t *testing.T) { }) err = SetConnectionLocation(ctx, connectionId, clientIp) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) var countryLocationId server.Id server.Db(ctx, func(conn server.PgConn) { @@ -150,7 +149,7 @@ func TestSetConnectionLocationStaleProbedFallsBackToMmdb(t *testing.T) { } }) }) - assert.Equal(t, countryLocationId, mmdbLocation.CountryLocationId) + connect.AssertEqual(t, countryLocationId, mmdbLocation.CountryLocationId) if countryLocationId == probed.CountryLocationId { t.Fatal("stale probed location must not be used") } @@ -169,7 +168,7 @@ func TestSetConnectionLocationProbedWriteErrorFallsBackToMmdb(t *testing.T) { clientIp := "8.8.8.8" mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) model.CreateLocation(ctx, mmdbLocation) networkId := server.NewId() @@ -178,7 +177,7 @@ func TestSetConnectionLocationProbedWriteErrorFallsBackToMmdb(t *testing.T) { handlerId := model.CreateNetworkClientHandler(ctx) connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) // a fresh probed entry pointing at a location id that was never // created via CreateLocation -- the storage write in @@ -191,7 +190,7 @@ func TestSetConnectionLocationProbedWriteErrorFallsBackToMmdb(t *testing.T) { }) err = SetConnectionLocation(ctx, connectionId, clientIp) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) var countryLocationId server.Id server.Db(ctx, func(conn server.PgConn) { @@ -206,7 +205,7 @@ func TestSetConnectionLocationProbedWriteErrorFallsBackToMmdb(t *testing.T) { } }) }) - assert.Equal(t, countryLocationId, mmdbLocation.CountryLocationId) + connect.AssertEqual(t, countryLocationId, mmdbLocation.CountryLocationId) }) } @@ -242,7 +241,7 @@ func TestSetConnectionLocationProbedNetTypeForeignMatchesMmdbParity(t *testing.T handlerId := model.CreateNetworkClientHandler(ctx) connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) // probed country ("jp") deliberately differs from the control ip's // mmdb country ("us" for 8.8.8.8). @@ -254,7 +253,7 @@ func TestSetConnectionLocationProbedNetTypeForeignMatchesMmdbParity(t *testing.T }) err = SetConnectionLocation(ctx, connectionId, clientIp) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) var netTypeForeign int server.Db(ctx, func(conn server.PgConn) { @@ -269,7 +268,7 @@ func TestSetConnectionLocationProbedNetTypeForeignMatchesMmdbParity(t *testing.T } }) }) - assert.Equal(t, netTypeForeign, 0) + connect.AssertEqual(t, netTypeForeign, 0) }) } @@ -292,7 +291,7 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { handlerId := model.CreateNetworkClientHandler(ctx) connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, "8.8.8.8:0", handlerId) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ ClientId: clientId, @@ -304,7 +303,7 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { }) err = SetConnectionLocation(ctx, connectionId, "8.8.8.8") - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) var netTypeHosting int var netTypePrivacy int @@ -320,7 +319,7 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { } }) }) - assert.Equal(t, netTypeHosting, 1) - assert.Equal(t, netTypePrivacy, 1) + connect.AssertEqual(t, netTypeHosting, 1) + connect.AssertEqual(t, netTypePrivacy, 1) }) } diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go index 25a04c6d..27b4163c 100644 --- a/controller/provider_egress_location_controller_test.go +++ b/controller/provider_egress_location_controller_test.go @@ -6,8 +6,7 @@ import ( "testing" "time" - "github.com/go-playground/assert/v2" - + "github.com/urnetwork/connect" "github.com/urnetwork/server" "github.com/urnetwork/server/model" ) @@ -30,7 +29,7 @@ func TestSubmitProviderEgressLocationCountryOnly(t *testing.T) { CountryConfident: true, ObservedAt: server.NowUtc(), }) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) if res.LocationId == (server.Id{}) { t.Fatal("expected a resolved location id") } @@ -39,10 +38,10 @@ func TestSubmitProviderEgressLocationCountryOnly(t *testing.T) { if stored == nil { t.Fatal("expected the submission to be stored") } - assert.Equal(t, stored.CountryCode, "us") - assert.Equal(t, stored.ASN, 401486) - assert.Equal(t, stored.Hosting, true) - assert.Equal(t, stored.CityConfident, false) + connect.AssertEqual(t, stored.CountryCode, "us") + connect.AssertEqual(t, stored.ASN, 401486) + connect.AssertEqual(t, stored.Hosting, true) + connect.AssertEqual(t, stored.CityConfident, false) // the resolved location must be the country-granular row, with no // city/region association @@ -50,7 +49,7 @@ func TestSubmitProviderEgressLocationCountryOnly(t *testing.T) { if loc == nil { t.Fatal("expected the resolved location row to exist") } - assert.Equal(t, loc.LocationType, model.LocationTypeCountry) + connect.AssertEqual(t, loc.LocationType, model.LocationTypeCountry) if loc.CityLocationId != (server.Id{}) { t.Fatal("a country-granularity row must not have a city association") } @@ -116,20 +115,20 @@ func TestSubmitProviderEgressLocationCityConfidentStoresCity(t *testing.T) { CityConfident: true, ObservedAt: server.NowUtc(), }) - assert.Equal(t, err, nil) + connect.AssertEqual(t, err, nil) stored := model.GetProviderEgressLocation(ctx, clientId) if stored == nil { t.Fatal("expected the submission to be stored") } - assert.Equal(t, stored.CityConfident, true) + connect.AssertEqual(t, stored.CityConfident, true) // the resolved location must be the city-granular row loc := model.GetLocation(ctx, stored.LocationId) if loc == nil { t.Fatal("expected the resolved location row to exist") } - assert.Equal(t, loc.LocationType, model.LocationTypeCity) + connect.AssertEqual(t, loc.LocationType, model.LocationTypeCity) }) } diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index 874f5391..f92bb2a7 100644 --- a/model/provider_egress_location_model_test.go +++ b/model/provider_egress_location_model_test.go @@ -5,8 +5,7 @@ import ( "testing" "time" - "github.com/go-playground/assert/v2" - + "github.com/urnetwork/connect" "github.com/urnetwork/server" ) @@ -37,11 +36,11 @@ func TestProviderEgressLocationUpsertAndGet(t *testing.T) { if got == nil { t.Fatal("expected a stored egress location") } - assert.Equal(t, got.LocationId, country.LocationId) - assert.Equal(t, got.CountryCode, "us") - assert.Equal(t, got.ASN, 401486) - assert.Equal(t, got.Hosting, true) - assert.Equal(t, got.Proxy, false) + connect.AssertEqual(t, got.LocationId, country.LocationId) + connect.AssertEqual(t, got.CountryCode, "us") + connect.AssertEqual(t, got.ASN, 401486) + connect.AssertEqual(t, got.Hosting, true) + connect.AssertEqual(t, got.Proxy, false) // upsert replaces, given a strictly newer observed_at: the upsert is // monotonic (see TestProviderEgressLocationUpsertIgnoresOlderReplay below), @@ -56,9 +55,9 @@ func TestProviderEgressLocationUpsertAndGet(t *testing.T) { ObservedAt: now.Add(time.Minute), }) got = GetProviderEgressLocation(ctx, clientId) - assert.Equal(t, got.ASN, 999) - assert.Equal(t, got.Hosting, false) - assert.Equal(t, got.Proxy, true) + connect.AssertEqual(t, got.ASN, 999) + connect.AssertEqual(t, got.Hosting, false) + connect.AssertEqual(t, got.Proxy, true) }) } @@ -108,9 +107,9 @@ func TestProviderEgressLocationUpsertIgnoresOlderReplay(t *testing.T) { if got == nil { t.Fatal("expected a stored egress location") } - assert.Equal(t, got.CountryCode, "jp") - assert.Equal(t, got.ASN, 111) - assert.Equal(t, got.LocationId, jpCountry.LocationId) + connect.AssertEqual(t, got.CountryCode, "jp") + connect.AssertEqual(t, got.ASN, 111) + connect.AssertEqual(t, got.LocationId, jpCountry.LocationId) }) } @@ -142,7 +141,7 @@ func TestProviderEgressLocationCountryCodeLowercased(t *testing.T) { if got == nil { t.Fatal("expected a stored egress location") } - assert.Equal(t, got.CountryCode, "us") + connect.AssertEqual(t, got.CountryCode, "us") }) } From 6b5de2be82f548daabcd66033311d9ae5cfe43b6 Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Sat, 25 Jul 2026 08:34:27 +0100 Subject: [PATCH 14/19] fix(provider-egress): reject future-dated submissions, fix ASN column overflow, and other review findings Five review findings on the provider-egress-location PR, each empirically reproduced before being fixed: - A far-future observed_at had no upper bound: it would win the monotonic upsert forever, read as fresh forever, and outlive the taskworker sweep, permanently pinning a provider's location with no API-side recovery. Add MaxProviderEgressLocationSubmissionSkew (5m) alongside the existing MaxProviderEgressLocationSubmissionAge check, and reject observed_at further in the future than that. - provider_egress_location.asn was `int` (Postgres int4, max ~2.147e9); ASNs are 32-bit unsigned (max ~4.295e9), so a real ASN like 4200000000 panicked pgx's arg encoding after a ~78s retry-storm hang. The table is new in this same unmerged PR, so the fix edits the CREATE TABLE migration directly (asn int -> asn bigint) rather than adding a second migration. - Removed a "fix(beta)" fork marker comment from model/network_client_location_model.go, rewritten to be vendor-neutral while keeping the NULL-city/region panic explanation intact. - arinForeignScore's doc comment said its second argument was "the mmdb-resolved country ... or the probed egress country" -- a later commit made both callers pass the mmdb country for ranking parity, so the doc was updated to describe what the code actually does and why. - SetConnectionLocation mapped the probed Mobile flag onto NetTypeVirtual, but IpInfo has no Mobile concept and NetTypeVirtual is only ever set from the ipinfo schema's is_satellite field. This gave probed mobile providers a ranking penalty an identical unprobed provider never takes -- the opposite of the parity this feature promises. Mobile stays in the model/wire contract as metadata but no longer feeds net_type_virtual; Hosting/Proxy keep feeding their scores since they do have direct mmdb-path equivalents (ipInfo.Hosting/ipInfo.Privacy). Tests added: future/within-skew observed_at rejection and acceptance, large-ASN round-trip, and a Mobile-does-not-set-net_type_virtual assertion folded into the existing probed-flags-to-scores test. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- controller/network_client_controller.go | 14 ++- controller/network_client_controller_test.go | 16 +++- .../network_client_location_controller.go | 13 ++- .../provider_egress_location_controller.go | 16 ++++ ...rovider_egress_location_controller_test.go | 95 +++++++++++++++++++ db_migrations.go | 2 +- model/network_client_location_model.go | 6 +- 7 files changed, 147 insertions(+), 15 deletions(-) diff --git a/controller/network_client_controller.go b/controller/network_client_controller.go index 382aa913..4a723b6e 100644 --- a/controller/network_client_controller.go +++ b/controller/network_client_controller.go @@ -81,9 +81,17 @@ func SetConnectionLocation( if egress.Proxy { scores.NetTypePrivacy = 1 } - if egress.Mobile { - scores.NetTypeVirtual = 1 - } + // egress.Mobile deliberately does NOT feed NetTypeVirtual: unlike + // Hosting/Proxy, Mobile has no mmdb-path equivalent (IpInfo has no + // Mobile concept; NetTypeVirtual is set from the ipinfo schema's + // is_satellite field only, see GetLocationForIp, and never from + // DB-IP). Deriving NetTypeVirtual from Mobile here would penalize a + // probed mobile provider's ranking with no equivalent penalty for an + // otherwise-identical unprobed one -- the opposite of the parity + // this feature is meant to preserve (see arinForeignScore's doc for + // the same parity reasoning applied to net_type_foreign). Mobile + // stays on the model/wire contract as metadata; it just does not + // feed the ranking score. // keep the ARIN org-vs-country foreign check on the probed path too, // so a probed provider is ranked on equal terms with an equivalent // unprobed one (net_type_foreign feeds the ranking columns). Compute diff --git a/controller/network_client_controller_test.go b/controller/network_client_controller_test.go index 17feb8c9..f6561fb1 100644 --- a/controller/network_client_controller_test.go +++ b/controller/network_client_controller_test.go @@ -273,7 +273,14 @@ func TestSetConnectionLocationProbedNetTypeForeignMatchesMmdbParity(t *testing.T } // A fresh probed location's Hosting/Proxy flags must map onto the stored -// connection's net_type_hosting/net_type_privacy scores. +// connection's net_type_hosting/net_type_privacy scores. Mobile must NOT map +// onto net_type_virtual: Hosting/Proxy have direct mmdb-path equivalents +// (ipInfo.Hosting/ipInfo.Privacy, see GetLocationForIp), but Mobile has none +// (IpInfo has no Mobile concept at all, and NetTypeVirtual is only ever set +// from the ipinfo schema's is_satellite field, never from DB-IP or from +// anything Mobile-shaped) -- deriving NetTypeVirtual from Mobile would give +// a probed mobile provider a ranking penalty an identical unprobed mobile +// provider never takes, breaking the parity this feature promises. func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() @@ -299,6 +306,7 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { CountryCode: "jp", Hosting: true, Proxy: true, + Mobile: true, ObservedAt: server.NowUtc(), }) @@ -307,19 +315,21 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { var netTypeHosting int var netTypePrivacy int + var netTypeVirtual int server.Db(ctx, func(conn server.PgConn) { result, qerr := conn.Query( ctx, - `SELECT net_type_hosting, net_type_privacy FROM network_client_location WHERE connection_id = $1`, + `SELECT net_type_hosting, net_type_privacy, net_type_virtual FROM network_client_location WHERE connection_id = $1`, connectionId, ) server.WithPgResult(result, qerr, func() { if result.Next() { - server.Raise(result.Scan(&netTypeHosting, &netTypePrivacy)) + server.Raise(result.Scan(&netTypeHosting, &netTypePrivacy, &netTypeVirtual)) } }) }) connect.AssertEqual(t, netTypeHosting, 1) connect.AssertEqual(t, netTypePrivacy, 1) + connect.AssertEqual(t, netTypeVirtual, 0) }) } diff --git a/controller/network_client_location_controller.go b/controller/network_client_location_controller.go index 0535db77..a8796b8b 100644 --- a/controller/network_client_location_controller.go +++ b/controller/network_client_location_controller.go @@ -61,11 +61,14 @@ func GetLocationForIp(ctx context.Context, clientIp string) (*model.Location, *m } // arinForeignScore cross-checks the ARIN org registration country for addr -// against countryCode, the country code being claimed for this connection -// (the mmdb-resolved country on the ordinary path, or the probed egress -// country on the provider-egress path). If the org's registered country -// differs, the use case is considered foreign (VPN/proxy-like), matching the -// heuristic previously inlined in GetLocationForIp. +// against countryCode. Both the ordinary path (GetLocationForIp) and the +// provider-egress path (SetConnectionLocation, in network_client_controller.go) pass the +// mmdb-resolved country of addr here, not the probed egress country: this is +// deliberate parity, so a probed and an unprobed provider on the same +// control ip are scored on the same basis and probing does not, by itself, +// change this ranking signal. If the org's registered country differs, the +// use case is considered foreign (VPN/proxy-like), matching the heuristic +// previously inlined in GetLocationForIp. // // If the ARIN lookup fails, this returns 0 without error: it must never fail // or panic a caller on the connect-announce hot path over a missing/failed diff --git a/controller/provider_egress_location_controller.go b/controller/provider_egress_location_controller.go index 169de94a..2c28c494 100644 --- a/controller/provider_egress_location_controller.go +++ b/controller/provider_egress_location_controller.go @@ -14,6 +14,19 @@ import ( // already older than this when it arrives. It bounds replay of an old probe. const MaxProviderEgressLocationSubmissionAge = 24 * time.Hour +// MaxProviderEgressLocationSubmissionSkew rejects a submission whose +// observed_at is further in the future than this. The prober and server +// clocks should be roughly in sync, so a few minutes of allowance covers +// ordinary clock drift without opening the door to a far-future timestamp. +// Without this bound, a future observed_at would defeat every other +// safeguard at once: it always wins the monotonic upsert in +// model.SetProviderEgressLocation (so no later, legitimate probe can ever +// overwrite it), it reads as "fresh" forever against +// ProviderEgressLocationMaxAge, and it outlives the taskworker sweep in +// RemoveExpiredProviderEgressLocations -- permanently pinning a provider to +// whatever location was submitted, with no API-side recovery. +const MaxProviderEgressLocationSubmissionSkew = 5 * time.Minute + // maxLocationNameLen bounds country/city/region as submitted: these flow into // model.CreateLocation, whose location_name column is varchar(128). Rejecting // an over-long value here with a clear error is preferable to letting @@ -66,6 +79,9 @@ func SubmitProviderEgressLocation( if args.ObservedAt.Before(server.NowUtc().Add(-MaxProviderEgressLocationSubmissionAge)) { return nil, fmt.Errorf("Submission is too old.") } + if server.NowUtc().Add(MaxProviderEgressLocationSubmissionSkew).Before(args.ObservedAt) { + return nil, fmt.Errorf("Submission is too far in the future.") + } if networkId := model.GetNetworkClientNetwork(ctx, args.ClientId); networkId == nil { return nil, fmt.Errorf("Unknown client.") } diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go index 27b4163c..d11c08c8 100644 --- a/controller/provider_egress_location_controller_test.go +++ b/controller/provider_egress_location_controller_test.go @@ -292,3 +292,98 @@ func TestSubmitProviderEgressLocationRejectsStaleObservedAt(t *testing.T) { } }) } + +// A far-future observed_at must be rejected, not just an old one: unchecked, +// it would defeat the monotonic upsert (it always "wins"), read as fresh +// forever, and outlive the taskworker sweep -- permanently pinning a +// provider's location with no API-side recovery. See +// MaxProviderEgressLocationSubmissionSkew. +func TestSubmitProviderEgressLocationRejectsFutureObservedAt(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "jp", + Country: "Japan", + CountryConfident: true, + ObservedAt: server.NowUtc().Add(10 * 365 * 24 * time.Hour), + }) + if err == nil { + t.Fatal("a far-future observed_at must be rejected") + } + if model.GetProviderEgressLocation(ctx, clientId) != nil { + t.Fatal("rejected submission must not be stored") + } + }) +} + +// A submission within the allowed clock-skew window must still be accepted: +// the future-timestamp rejection must not be so strict that ordinary clock +// drift between the prober and server breaks legitimate submissions. +func TestSubmitProviderEgressLocationAcceptsWithinSkewObservedAt(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + res, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: "United States", + CountryConfident: true, + ObservedAt: server.NowUtc().Add(1 * time.Minute), + }) + connect.AssertEqual(t, err, nil) + if res.LocationId == (server.Id{}) { + t.Fatal("expected a resolved location id") + } + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatal("expected the submission to be stored") + } + }) +} + +// A large 32-bit ASN (e.g. from the private-use range, common on +// hosting/VPN infrastructure) must round-trip cleanly, not panic. The asn +// column used to be `int` (Postgres int4, max ~2.147e9); ASNs are 32-bit +// unsigned (max ~4.295e9), so a value above int4's range panicked deep in +// pgx's arg encoding. +func TestSubmitProviderEgressLocationAcceptsLargeAsn(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + const largeAsn = 4200000000 + + res, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "us", + Country: "United States", + ASN: largeAsn, + CountryConfident: true, + ObservedAt: server.NowUtc(), + }) + connect.AssertEqual(t, err, nil) + if res.LocationId == (server.Id{}) { + t.Fatal("expected a resolved location id") + } + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatal("expected the submission to be stored") + } + connect.AssertEqual(t, stored.ASN, largeAsn) + }) +} diff --git a/db_migrations.go b/db_migrations.go index 9ae8a614..dfc27b62 100644 --- a/db_migrations.go +++ b/db_migrations.go @@ -4400,7 +4400,7 @@ var migrations = []any{ client_id uuid NOT NULL PRIMARY KEY, location_id uuid NOT NULL, country_code varchar(2) NOT NULL, - asn int NOT NULL DEFAULT 0, + asn bigint NOT NULL DEFAULT 0, org varchar(256) NOT NULL DEFAULT '', hosting bool NOT NULL DEFAULT false, proxy bool NOT NULL DEFAULT false, diff --git a/model/network_client_location_model.go b/model/network_client_location_model.go index cbab0e61..811cc80e 100644 --- a/model/network_client_location_model.go +++ b/model/network_client_location_model.go @@ -1242,9 +1242,9 @@ func SetConnectionLocation( } }) - // fix(beta): the free-tier ipinfo geo db resolves many IPs -- - // datacenter, mobile, and VPN egress especially -- to country or - // region granularity only, with no city. network_client_location + // geo databases vary in how deep their coverage goes: many IPs -- + // datacenter, mobile, and VPN egress especially -- only resolve to + // country or region granularity, with no city. network_client_location // requires city_location_id and region_location_id NOT NULL, so a // country-only location's NULL city/region made this INSERT panic // inside server.Tx. That panic propagated out of the connection From d176fc99650f619e19e1795bf41e1454bcc7866e Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 09:09:23 +0100 Subject: [PATCH 15/19] feat(api): serve the provider egress probe schedule from the server The operator's prober decided what to probe from an in-memory ttl cache, so a restart re-probed the whole provider population and nothing durable recorded what was actually due. The freshness data already exists server-side in provider_egress_location.observed_at; this exposes it so the server owns the schedule. model.GetProviderEgressLocationDue sources candidates from the live provider population (network_client_location_reliability, connected + valid) and LEFT JOINs the egress row, rather than selecting from provider_egress_location. The dominant case is a provider that has never been probed and so has no egress row at all; selecting from that table would return exactly the providers that least need probing and none of the ones that most do. Only providers holding a Public provide key are returned. Probing tunnels through the provider itself, i.e. opens a contract from outside the provider's own network, which a provider without a Public key refuses -- offering one to the prober would burn a probe slot on a guaranteed failure. Same EXISTS filter UpdateClientLocations and UpdateClientScores already apply. The staleness cutoff is computed in Go and bound as a query argument. observed_at is a naive `timestamp` holding utc, so comparing it against sql now() would cast through the session timezone and silently skip a window. GET /network/provider-egress-due is operator-to-server, authenticated by the same X-UR-Operator-Secret shared secret as the ingest endpoint (hmac.Equal against the memoized, fail-closed operatorIngestSecret) rather than a network jwt. Its cutoff is half ProviderEgressLocationMaxAge: at the full max age every location would lapse to the mmdb fallback at the exact moment it became due and stay lapsed until the prober reached it. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- api/api.go | 1 + .../provider_egress_location_handlers.go | 75 +++++++ .../provider_egress_location_handlers_test.go | 192 ++++++++++++++++++ model/provider_egress_location_model.go | 79 +++++++ model/provider_egress_location_model_test.go | 111 ++++++++++ 5 files changed, 458 insertions(+) diff --git a/api/api.go b/api/api.go index c3dc801d..e9a99f18 100644 --- a/api/api.go +++ b/api/api.go @@ -52,6 +52,7 @@ func Routes() []*router.Route { router.NewRoute("POST", "/network/auth-client", handlers.AuthNetworkClient), router.NewRoute("POST", "/network/remove-client", handlers.RemoveNetworkClient), router.NewRoute("POST", "/network/provider-egress-location", handlers.ProviderEgressLocationSubmit), + router.NewRoute("GET", "/network/provider-egress-due", handlers.ProviderEgressLocationDue), router.NewRoute("GET", "/network/clients", handlers.NetworkClients), router.NewRoute("GET", "/network/peers", handlers.NetworkPeers), router.NewRoute("GET", "/network/provider-locations", handlers.NetworkGetProviderLocations), diff --git a/api/handlers/provider_egress_location_handlers.go b/api/handlers/provider_egress_location_handlers.go index b843c05b..7f6feac8 100644 --- a/api/handlers/provider_egress_location_handlers.go +++ b/api/handlers/provider_egress_location_handlers.go @@ -5,12 +5,14 @@ import ( "encoding/json" "io" "net/http" + "strconv" "sync" "github.com/urnetwork/glog" "github.com/urnetwork/server" "github.com/urnetwork/server/controller" + "github.com/urnetwork/server/model" ) // operatorSecretHeader carries the operator ingest secret. This endpoint is @@ -91,3 +93,76 @@ func ProviderEgressLocationSubmit(w http.ResponseWriter, r *http.Request) { glog.Infof("[pegl]could not write response. err = %s\n", err) } } + +const ( + // defaultProviderEgressDueLimit is the batch size when the caller does not + // ask for one. + defaultProviderEgressDueLimit = 100 + // maxProviderEgressDueLimit bounds the batch size regardless of what the + // caller asks for, so one request cannot ask the database for the entire + // provider population. + maxProviderEgressDueLimit = 500 +) + +// providerEgressDueAge is how stale a stored probe must be before its provider +// is offered up for re-probing. It is deliberately shorter than +// model.ProviderEgressLocationMaxAge -- the age past which a stored location +// stops being trusted at all. If the two were equal, every location would lapse +// to the mmdb fallback at the exact moment it became due and stay lapsed until +// the prober worked its way around to it; at half the max age the prober has a +// full max-age/2 window to refresh a location before it expires. +const providerEgressDueAge = model.ProviderEgressLocationMaxAge / 2 + +// ProviderEgressLocationDueResult is the response body of +// ProviderEgressLocationDue. +type ProviderEgressLocationDueResult struct { + ClientIds []server.Id `json:"client_ids"` +} + +// ProviderEgressLocationDue tells the operator's prober which providers to +// probe next: those whose egress location has gone stale, and those that have +// never been probed at all, oldest first. +// +// This moves the probe schedule from the prober's memory into the database. +// The prober used to decide what to probe from an in-memory ttl cache, so a +// restart re-probed the whole population and nothing durable recorded what was +// actually due; observed_at already carries that information server-side, and +// this exposes it. +// +// Same auth as ProviderEgressLocationSubmit above: operator-to-server, the +// shared secret header rather than a network jwt, fail-closed when the vault +// resource is missing. +func ProviderEgressLocationDue(w http.ResponseWriter, r *http.Request) { + secret := operatorIngestSecret() + provided := r.Header.Get(operatorSecretHeader) + if secret == "" || provided == "" || !hmac.Equal([]byte(secret), []byte(provided)) { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + + limit := defaultProviderEgressDueLimit + if raw := r.URL.Query().Get("limit"); raw != "" { + parsed, err := strconv.Atoi(raw) + if err != nil || parsed < 1 { + // not clamped up to 1: `limit=0` would come back as an empty list, + // which the prober cannot distinguish from "nothing is due" + http.Error(w, "Bad request", http.StatusBadRequest) + return + } + limit = min(parsed, maxProviderEgressDueLimit) + } + + // the cutoff is computed here and passed as an argument; observed_at is a + // naive timestamp holding utc, so comparing it to sql now() in the query + // would cast through the session timezone + minObservedAt := server.NowUtc().Add(-providerEgressDueAge) + + result := &ProviderEgressLocationDueResult{ + ClientIds: model.GetProviderEgressLocationDue(r.Context(), minObservedAt, limit), + } + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(result); err != nil { + glog.Infof("[pegl]could not write response. err = %s\n", err) + } +} diff --git a/api/handlers/provider_egress_location_handlers_test.go b/api/handlers/provider_egress_location_handlers_test.go index 4af00971..ab298a94 100644 --- a/api/handlers/provider_egress_location_handlers_test.go +++ b/api/handlers/provider_egress_location_handlers_test.go @@ -2,14 +2,18 @@ package handlers import ( "bytes" + "context" "encoding/json" "net/http" "net/http/httptest" + "slices" "strings" "testing" + "time" "github.com/urnetwork/server" "github.com/urnetwork/server/controller" + "github.com/urnetwork/server/model" ) func TestProviderEgressLocationSubmitRejectsMissingSecret(t *testing.T) { @@ -148,3 +152,191 @@ func TestProviderEgressLocationSubmitReadsSecretFromVault(t *testing.T) { t.Fatalf("readOperatorIngestSecret() = %q, want %q", got, secret) } } + +func TestProviderEgressLocationDueRejectsMissingSecret(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due", nil) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 when the operator secret header is absent", w.Code) + } +} + +func TestProviderEgressLocationDueRejectsWrongSecret(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due", nil) + req.Header.Set(operatorSecretHeader, "definitely-not-the-secret") + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 on a wrong operator secret", w.Code) + } +} + +// TestProviderEgressLocationDueRejectsAlteredSecret is the reject case with +// the vault *configured*, so the request gets past the secret == "" fail-closed +// short-circuit and hmac.Equal is what does the rejecting. +func TestProviderEgressLocationDueRejectsAlteredSecret(t *testing.T) { + const secret = "correct-operator-secret-0123456789" + const wrongSecret = "correct-operator-secret-0123456780" // last char changed + defer withStubOperatorIngestSecret(secret)() + + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due", nil) + req.Header.Set(operatorSecretHeader, wrongSecret) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 when the operator secret is configured but the request's secret is wrong", w.Code) + } +} + +// testing_connectDueProvider stands up a connected + valid provider holding a +// Public provide key and no probe result, i.e. a provider the due query must +// return. The caller runs model.UpdateClientLocationReliabilities afterward. +func testing_connectDueProvider( + t testing.TB, + ctx context.Context, + clientId server.Id, + locationId server.Id, + clientAddress string, +) { + model.Testing_CreateDevice(ctx, server.NewId(), server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientAddress, handlerId) + if err != nil { + t.Fatalf("connect client: %s", err) + } + if err := model.SetConnectionLocation(ctx, connectionId, locationId, &model.ConnectionLocationScores{}); err != nil { + t.Fatalf("set connection location: %s", err) + } + model.SetProvide(ctx, clientId, map[model.ProvideMode][]byte{ + model.ProvideModePublic: []byte("provide-secret"), + }) +} + +// TestProviderEgressLocationDueAcceptsCorrectSecret proves the auth gate can +// ACCEPT. This is the test that gives the three reject tests above their +// meaning: without it, a handler whose entire body was replaced with an +// unconditional `http.Error(w, "Unauthorized", 401)` would still pass all +// three, because a suite that only ever asserts rejections cannot tell a +// working auth check from a broken-shut one. +// +// It deliberately asserts more than "not 401": a real, never-probed provider +// is stood up in the test database and must come back in the response body, so +// the test also fails if the handler clears auth but never reaches the model +// query or writes the wrong json shape. +func TestProviderEgressLocationDueAcceptsCorrectSecret(t *testing.T) { + t.Setenv("WARP_ENV", "local") + server.DefaultTestEnv().Run(t, func(t testing.TB) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + ctx := context.Background() + + city := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, city) + + due := server.NewId() + testing_connectDueProvider(t, ctx, due, city.LocationId, "0.0.0.1:0") + model.UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due?limit=10", nil) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code == http.StatusUnauthorized { + t.Fatalf("status = %d, want the correct secret to clear auth (not 401)", w.Code) + } + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + + var result ProviderEgressLocationDueResult + if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { + t.Fatalf("decode body %q: %s", w.Body.String(), err) + } + if !slices.Contains(result.ClientIds, due) { + t.Fatalf("client_ids = %v, want it to contain the never-probed provider %s", result.ClientIds, due) + } + // the wire name the prober reads + if !strings.Contains(w.Body.String(), `"client_ids"`) { + t.Fatalf("body = %s, want a client_ids field", w.Body.String()) + } + }) +} + +// The prober asks for a batch; the server must not hand back more than asked. +func TestProviderEgressLocationDueHonoursLimit(t *testing.T) { + t.Setenv("WARP_ENV", "local") + server.DefaultTestEnv().Run(t, func(t testing.TB) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + ctx := context.Background() + + city := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, city) + + testing_connectDueProvider(t, ctx, server.NewId(), city.LocationId, "0.0.0.1:0") + testing_connectDueProvider(t, ctx, server.NewId(), city.LocationId, "0.0.0.2:0") + model.UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due?limit=1", nil) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + var result ProviderEgressLocationDueResult + if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { + t.Fatalf("decode body %q: %s", w.Body.String(), err) + } + if len(result.ClientIds) != 1 { + t.Fatalf("len(client_ids) = %d, want 1 for limit=1; body = %s", len(result.ClientIds), w.Body.String()) + } + }) +} + +// A limit that is not a positive integer is a caller bug. Silently clamping it +// to 1 (or to the default) would answer a question the prober did not ask -- +// `limit=0` would come back as an empty list, indistinguishable from "nothing +// is due" -- so it is rejected instead. +func TestProviderEgressLocationDueRejectsBadLimit(t *testing.T) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + for _, raw := range []string{"0", "-1", "abc", "1.5"} { + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due?limit="+raw, nil) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("limit=%q: status = %d, want 400", raw, w.Code) + } + } +} diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 240bb8b4..7bf9ae00 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -274,6 +274,85 @@ func GetLocation(ctx context.Context, locationId server.Id) *Location { return loc } +// GetProviderEgressLocationDue returns the client ids of providers whose +// egress location is due for a probe: their newest probe is older than +// minObservedAt, or they have never been probed at all. Oldest first, so the +// longest-unprobed are handed out first, capped at limit. +// +// This is the durable replacement for the prober's in-memory ttl cache: the +// schedule lives in the database, so a prober restart resumes where it left +// off instead of re-probing everything. +// +// Two things about the shape of this query matter. +// +// First, candidates are sourced from the live provider population +// (network_client_location_reliability, connected + valid) and the egress row +// is LEFT JOINed on. The dominant case by far is a provider that has *never* +// been probed and therefore has no provider_egress_location row at all; +// selecting from provider_egress_location would return exactly the providers +// that least need probing and none of the ones that most do. +// +// Second, only providers holding a Public provide key are returned. Probing +// tunnels through the provider itself, which means opening a contract from +// outside the provider's own network -- something a provider without a Public +// key refuses. Offering one to the prober would burn a probe slot on a +// guaranteed failure. This is the same filter UpdateClientLocations and +// UpdateClientScores apply (network_client_location_model.go). +// +// The cutoff is computed by the caller in Go and bound as a parameter: +// observed_at is a naive `timestamp` holding utc, and comparing it against sql +// now() would cast through the session timezone and silently skip a window. +func GetProviderEgressLocationDue( + ctx context.Context, + minObservedAt time.Time, + limit int, +) []server.Id { + clientIds := []server.Id{} + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query( + ctx, + ` + SELECT + network_client_location_reliability.client_id + FROM network_client_location_reliability + + LEFT JOIN provider_egress_location ON + provider_egress_location.client_id = network_client_location_reliability.client_id + + 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 + ( + provider_egress_location.observed_at IS NULL OR + provider_egress_location.observed_at < $2 + ) + + -- never-probed sorts ahead of merely stale: a missing observed_at + -- is infinitely old + ORDER BY provider_egress_location.observed_at ASC NULLS FIRST + LIMIT $3 + `, + ProvideModePublic, + minObservedAt.UTC(), + limit, + ) + server.WithPgResult(result, err, func() { + for result.Next() { + var clientId server.Id + server.Raise(result.Scan(&clientId)) + clientIds = append(clientIds, clientId) + } + }) + }) + return clientIds +} + // RemoveExpiredProviderEgressLocations drops entries probed before // minObservedAt. func RemoveExpiredProviderEgressLocations(ctx context.Context, minObservedAt time.Time) { diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index f92bb2a7..c6f5f8cb 100644 --- a/model/provider_egress_location_model_test.go +++ b/model/provider_egress_location_model_test.go @@ -2,6 +2,7 @@ package model import ( "context" + "slices" "testing" "time" @@ -212,3 +213,113 @@ func TestRemoveExpiredProviderEgressLocations(t *testing.T) { } }) } + +// testing_connectProbeableProvider stands up the minimum a client needs to +// look like a live provider to the due-selection query: a device, a live +// connection with a resolved location, and a provide key of the given mode. +// The caller must run UpdateClientLocationReliabilities afterward -- that is +// what rolls the live connection tables up into the +// network_client_location_reliability row (connected + valid) the query reads. +func testing_connectProbeableProvider( + t testing.TB, + ctx context.Context, + clientId server.Id, + locationId server.Id, + clientAddress string, + provideMode ProvideMode, +) { + Testing_CreateDevice(ctx, server.NewId(), server.NewId(), clientId, "", "") + + handlerId := CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := ConnectNetworkClient(ctx, clientId, clientAddress, handlerId) + if err != nil { + t.Fatalf("connect client: %s", err) + } + + if err := SetConnectionLocation(ctx, connectionId, locationId, &ConnectionLocationScores{}); err != nil { + t.Fatalf("set connection location: %s", err) + } + + SetProvide(ctx, clientId, map[ProvideMode][]byte{ + provideMode: []byte("provide-secret"), + }) +} + +// The prober asks the server what to probe next. The answer must be sourced +// from the live provider population and not from provider_egress_location, +// because the dominant case -- a provider that has never been probed at all -- +// has no row there. +func TestGetProviderEgressLocationDue(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) + + fresh := server.NewId() + stale := server.NewId() + never := server.NewId() + // a provider that cannot serve a stranger is unprobeable: the tunnel + // contract would be refused, so it must never be offered to the prober + nonPublic := server.NewId() + + testing_connectProbeableProvider(t, ctx, fresh, city.LocationId, "0.0.0.1:0", ProvideModePublic) + testing_connectProbeableProvider(t, ctx, stale, city.LocationId, "0.0.0.2:0", ProvideModePublic) + testing_connectProbeableProvider(t, ctx, never, city.LocationId, "0.0.0.3:0", ProvideModePublic) + testing_connectProbeableProvider(t, ctx, nonPublic, city.LocationId, "0.0.0.4:0", ProvideModeNetwork) + + UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), now) + + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: fresh, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-1 * time.Hour), + }) + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: stale, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-72 * time.Hour), + }) + // `never` and `nonPublic` deliberately get no row at all + + due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), 100) + + // a provider probed an hour ago must not be re-probed; one probed three + // days ago must be; one never probed must be + if slices.Contains(due, fresh) { + t.Fatalf("due = %v, must not contain the provider probed an hour ago (%s)", due, fresh) + } + if !slices.Contains(due, stale) { + t.Fatalf("due = %v, must contain the provider probed three days ago (%s)", due, stale) + } + if !slices.Contains(due, never) { + t.Fatalf("due = %v, must contain the never-probed provider (%s)", due, never) + } + // unprobeable regardless of freshness + if slices.Contains(due, nonPublic) { + t.Fatalf("due = %v, must not contain the provider without a Public provide key (%s)", due, nonPublic) + } + + // oldest first, so the longest-unprobed are probed first: the + // never-probed provider sorts ahead of the three-days-stale one + neverIndex := slices.Index(due, never) + staleIndex := slices.Index(due, stale) + if staleIndex < neverIndex { + t.Fatalf("never-probed provider at %d must sort before the stale one at %d", neverIndex, staleIndex) + } + + // limit is honoured + limited := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), 1) + if len(limited) != 1 { + t.Fatalf("len(due) = %d for limit 1, want 1", len(limited)) + } + if limited[0] != never { + t.Fatalf("due[0] = %s for limit 1, want the never-probed provider %s", limited[0], never) + } + }) +} From 813d986fca643cb7925909d3a4e1f86a217e72e1 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Sun, 26 Jul 2026 09:46:43 +0100 Subject: [PATCH 16/19] fix(model,api): defer providers that fail to probe, not only ones that succeed The only thing that moved a provider off the head of the due queue was a successful probe writing provider_egress_location. A provider that connects and holds a Public provide key but always fails to probe -- firewalled egress, dead upstream, anything other than the missing Public key the query already screens for -- never gets a row, so its observed_at stays NULL, so it sorts ahead of every stale-but-refreshable provider on every poll, forever. Five hundred such providers and a prober asking for limit=500 gets the same five hundred dead providers every time; no healthy provider's location is ever refreshed. It fails silently: the endpoint keeps returning a full, plausible-looking batch. The in-memory ttl cache this replaced was incidentally immune, because it marked a provider probed whether or not the probe worked; moving the schedule server-side dropped that protection. Record attempts, not just successes. provider_egress_probe_attempt is a small table keyed by client_id -- the attempt cannot live on provider_egress_location, because the case it exists to handle is precisely a provider with no row there. GetProviderEgressLocationDue now requires both no fresh success and no recent attempt, with a much shorter backoff for attempts (6h) than for success freshness (half ProviderEgressLocationMaxAge, 3.5d): a failing provider should be retried periodically, just not on every poll. Both cutoffs are computed in Go and bound as arguments -- attempt_at, like observed_at, is a naive `timestamp` holding utc, and comparing it to sql now() casts through the session timezone. The prober reports an attempt to POST /network/provider-egress-attempt, on the same operator-authenticated surface as the other two endpoints (X-UR-Operator-Secret, hmac.Equal, the memoized fail-closed operatorIngestSecret). The server timestamps the attempt rather than trusting the prober's clock. Attempt rows are swept by the existing expiry task. This pulls probe_attempt_at/probe_failure forward from the P2 data model in docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md, because P1's schedule cannot function without them. Only those two columns, not the rest of the verdict model. Also adds a client_id secondary sort key, so batch composition under a limit is deterministic rather than plan-dependent -- the whole never-probed population otherwise ties on NULL observed_at. Tests: a provider never probed successfully but attempted seconds ago must not be due, must not take the single batch slot from a stale-but-refreshable provider, and must become due again once the backoff elapses; the same over http via the attempt endpoint. Plus the two coverage gaps review flagged: deleting either the connected or the valid predicate now fails a test, and the handler's staleness cutoff is exercised by a probed (not merely never-probed) provider. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- api/api.go | 1 + .../provider_egress_location_handlers.go | 71 +++++- .../provider_egress_location_handlers_test.go | 210 ++++++++++++++++ .../provider_egress_location_controller.go | 52 ++++ db_migrations.go | 36 +++ model/provider_egress_location_model.go | 152 +++++++++++- model/provider_egress_location_model_test.go | 226 +++++++++++++++++- .../work/provider_egress_location_work.go | 8 +- 8 files changed, 738 insertions(+), 18 deletions(-) diff --git a/api/api.go b/api/api.go index e9a99f18..bb9d6c21 100644 --- a/api/api.go +++ b/api/api.go @@ -53,6 +53,7 @@ func Routes() []*router.Route { router.NewRoute("POST", "/network/remove-client", handlers.RemoveNetworkClient), router.NewRoute("POST", "/network/provider-egress-location", handlers.ProviderEgressLocationSubmit), router.NewRoute("GET", "/network/provider-egress-due", handlers.ProviderEgressLocationDue), + router.NewRoute("POST", "/network/provider-egress-attempt", handlers.ProviderEgressLocationAttempt), router.NewRoute("GET", "/network/clients", handlers.NetworkClients), router.NewRoute("GET", "/network/peers", handlers.NetworkPeers), router.NewRoute("GET", "/network/provider-locations", handlers.NetworkGetProviderLocations), diff --git a/api/handlers/provider_egress_location_handlers.go b/api/handlers/provider_egress_location_handlers.go index 7f6feac8..264da746 100644 --- a/api/handlers/provider_egress_location_handlers.go +++ b/api/handlers/provider_egress_location_handlers.go @@ -94,6 +94,59 @@ func ProviderEgressLocationSubmit(w http.ResponseWriter, r *http.Request) { } } +// ProviderEgressLocationAttempt records that the operator's prober tried to +// probe a provider, whether or not the try produced a location. +// +// The prober reports a *failure* here; a success is reported by +// ProviderEgressLocationSubmit above, whose provider_egress_location row +// already defers the provider for the full staleness window. Reporting a +// success here as well is harmless -- the attempt backoff is far shorter than +// that window -- but redundant. +// +// This exists because ProviderEgressLocationDue would otherwise be starved by +// providers that can never be probed successfully: they never get an egress +// row, so they sort to the head of the queue on every poll forever. See +// model.GetProviderEgressLocationDue. +// +// Same auth as the two endpoints around it: operator-to-server, the shared +// secret header rather than a network jwt, fail-closed when the vault resource +// is missing. +func ProviderEgressLocationAttempt(w http.ResponseWriter, r *http.Request) { + secret := operatorIngestSecret() + provided := r.Header.Get(operatorSecretHeader) + if secret == "" || provided == "" || !hmac.Equal([]byte(secret), []byte(provided)) { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + + body, err := io.ReadAll(io.LimitReader(r.Body, maxProviderEgressLocationBody+1)) + if err != nil { + http.Error(w, "Bad request", http.StatusBadRequest) + return + } + if len(body) > maxProviderEgressLocationBody { + http.Error(w, "Request too large", http.StatusRequestEntityTooLarge) + return + } + + var args controller.RecordProviderEgressProbeAttemptArgs + if err := json.Unmarshal(body, &args); err != nil { + http.Error(w, "Bad request", http.StatusBadRequest) + return + } + + result, err := controller.RecordProviderEgressProbeAttempt(r.Context(), &args) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(result); err != nil { + glog.Infof("[pegl]could not write response. err = %s\n", err) + } +} + const ( // defaultProviderEgressDueLimit is the batch size when the caller does not // ask for one. @@ -129,6 +182,12 @@ type ProviderEgressLocationDueResult struct { // actually due; observed_at already carries that information server-side, and // this exposes it. // +// A provider is skipped if it has a fresh success *or* a recent attempt. The +// second cutoff, ProviderEgressProbeAttemptBackoff, is much shorter than the +// first: a provider that failed to probe should be retried within hours, but +// must not be handed back on every poll, which is what would starve the rest of +// the queue (see ProviderEgressLocationAttempt above). +// // Same auth as ProviderEgressLocationSubmit above: operator-to-server, the // shared secret header rather than a network jwt, fail-closed when the vault // resource is missing. @@ -152,13 +211,15 @@ func ProviderEgressLocationDue(w http.ResponseWriter, r *http.Request) { limit = min(parsed, maxProviderEgressDueLimit) } - // the cutoff is computed here and passed as an argument; observed_at is a - // naive timestamp holding utc, so comparing it to sql now() in the query - // would cast through the session timezone - minObservedAt := server.NowUtc().Add(-providerEgressDueAge) + // both cutoffs are computed here and passed as arguments; observed_at and + // attempt_at are naive timestamps holding utc, so comparing them to sql + // now() in the query would cast through the session timezone + now := server.NowUtc() + minObservedAt := now.Add(-providerEgressDueAge) + minAttemptAt := now.Add(-model.ProviderEgressProbeAttemptBackoff) result := &ProviderEgressLocationDueResult{ - ClientIds: model.GetProviderEgressLocationDue(r.Context(), minObservedAt, limit), + ClientIds: model.GetProviderEgressLocationDue(r.Context(), minObservedAt, minAttemptAt, limit), } w.Header().Set("Content-Type", "application/json") diff --git a/api/handlers/provider_egress_location_handlers_test.go b/api/handlers/provider_egress_location_handlers_test.go index ab298a94..0f094ca0 100644 --- a/api/handlers/provider_egress_location_handlers_test.go +++ b/api/handlers/provider_egress_location_handlers_test.go @@ -320,6 +320,216 @@ func TestProviderEgressLocationDueHonoursLimit(t *testing.T) { }) } +// Every other due test in this file stands up never-probed providers, which +// come back regardless of what cutoff the handler computes -- so nothing here +// actually exercised providerEgressDueAge. This one does: a provider probed +// just now must be held back, and one probed past the cutoff must come through. +// Defeating the cutoff (dropping it, computing it in the wrong direction, +// comparing against sql now() through the session timezone) fails this. +func TestProviderEgressLocationDueHonoursStalenessCutoff(t *testing.T) { + t.Setenv("WARP_ENV", "local") + server.DefaultTestEnv().Run(t, func(t testing.TB) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + ctx := context.Background() + + city := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, city) + + fresh := server.NewId() + stale := server.NewId() + testing_connectDueProvider(t, ctx, fresh, city.LocationId, "0.0.0.1:0") + testing_connectDueProvider(t, ctx, stale, city.LocationId, "0.0.0.2:0") + model.UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + now := server.NowUtc() + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: fresh, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now, + }) + // comfortably past providerEgressDueAge, which is half + // model.ProviderEgressLocationMaxAge + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: stale, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-providerEgressDueAge - time.Hour), + }) + + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due?limit=100", nil) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + var result ProviderEgressLocationDueResult + if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { + t.Fatalf("decode body %q: %s", w.Body.String(), err) + } + if slices.Contains(result.ClientIds, fresh) { + t.Fatalf("client_ids = %v, must not contain the just-probed provider %s", result.ClientIds, fresh) + } + if !slices.Contains(result.ClientIds, stale) { + t.Fatalf("client_ids = %v, must contain the provider probed past the cutoff %s", result.ClientIds, stale) + } + }) +} + +func TestProviderEgressLocationAttemptRejectsMissingSecret(t *testing.T) { + body, _ := json.Marshal(map[string]any{ + "client_id": "019f8835-158d-6fd8-e9dd-fd0e4c6d6792", + }) + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-attempt", bytes.NewReader(body)) + w := httptest.NewRecorder() + + ProviderEgressLocationAttempt(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 when the operator secret header is absent", w.Code) + } +} + +// The reject case with the vault *configured*, so the request gets past the +// secret == "" fail-closed short-circuit and hmac.Equal is what rejects. +func TestProviderEgressLocationAttemptRejectsAlteredSecret(t *testing.T) { + const secret = "correct-operator-secret-0123456789" + const wrongSecret = "correct-operator-secret-0123456780" // last char changed + defer withStubOperatorIngestSecret(secret)() + + body, _ := json.Marshal(map[string]any{ + "client_id": "019f8835-158d-6fd8-e9dd-fd0e4c6d6792", + }) + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-attempt", bytes.NewReader(body)) + req.Header.Set(operatorSecretHeader, wrongSecret) + w := httptest.NewRecorder() + + ProviderEgressLocationAttempt(w, req) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 when the operator secret is configured but the request's secret is wrong", w.Code) + } +} + +// The whole point of the attempt endpoint, end to end over http: a provider +// that has never been probed successfully is due; the prober reports that it +// tried and failed; the provider stops being due. Without that, a provider +// whose probes always fail sits at the head of the queue on every poll forever +// (observed_at IS NULL sorts first) and starves every provider behind it. +func TestProviderEgressLocationAttemptDefersProvider(t *testing.T) { + t.Setenv("WARP_ENV", "local") + server.DefaultTestEnv().Run(t, func(t testing.TB) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + ctx := context.Background() + + city := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Palo Alto", + Region: "California", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, city) + + dead := server.NewId() + testing_connectDueProvider(t, ctx, dead, city.LocationId, "0.0.0.1:0") + model.UpdateClientLocationReliabilities(ctx, server.NowUtc().Add(-time.Hour), server.NowUtc()) + + if !slices.Contains(due(t, secret), dead) { + t.Fatalf("the never-probed provider %s must be due before any attempt is reported", dead) + } + + attemptBody, err := json.Marshal(controller.RecordProviderEgressProbeAttemptArgs{ + ClientId: dead, + ProbeFailure: "tunnel_failed", + }) + if err != nil { + t.Fatalf("marshal attempt: %s", err) + } + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-attempt", bytes.NewReader(attemptBody)) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationAttempt(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + + attempt := model.GetProviderEgressProbeAttempt(ctx, dead) + if attempt == nil { + t.Fatal("expected the attempt to be recorded") + } + if attempt.ProbeFailure != "tunnel_failed" { + t.Fatalf("probe_failure = %q, want %q", attempt.ProbeFailure, "tunnel_failed") + } + + if slices.Contains(due(t, secret), dead) { + t.Fatalf("the provider %s must not be due again immediately after a failed attempt", dead) + } + }) +} + +// due drives the due endpoint over http and returns the batch. +func due(t testing.TB, secret string) []server.Id { + req := httptest.NewRequest(http.MethodGet, "/network/provider-egress-due?limit=100", nil) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationDue(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("due: status = %d, want 200; body = %s", w.Code, w.Body.String()) + } + var result ProviderEgressLocationDueResult + if err := json.Unmarshal(w.Body.Bytes(), &result); err != nil { + t.Fatalf("due: decode body %q: %s", w.Body.String(), err) + } + return result.ClientIds +} + +// An unknown client id must be rejected rather than writing an attempt row +// keyed to a client that does not exist, which nothing would ever read. +func TestProviderEgressLocationAttemptRejectsUnknownClient(t *testing.T) { + t.Setenv("WARP_ENV", "local") + server.DefaultTestEnv().Run(t, func(t testing.TB) { + const secret = "correct-operator-secret-0123456789" + defer withStubOperatorIngestSecret(secret)() + + body, err := json.Marshal(controller.RecordProviderEgressProbeAttemptArgs{ + ClientId: server.NewId(), + ProbeFailure: "tunnel_failed", + }) + if err != nil { + t.Fatalf("marshal attempt: %s", err) + } + req := httptest.NewRequest(http.MethodPost, "/network/provider-egress-attempt", bytes.NewReader(body)) + req.Header.Set(operatorSecretHeader, secret) + w := httptest.NewRecorder() + + ProviderEgressLocationAttempt(w, req) + + if w.Code == http.StatusUnauthorized { + t.Fatalf("status = %d, want the correct secret to clear auth (not 401)", w.Code) + } + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 for an unregistered client id; body = %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Unknown client.") { + t.Fatalf("body = %q, want it to report the unknown client", w.Body.String()) + } + }) +} + // A limit that is not a positive integer is a caller bug. Silently clamping it // to 1 (or to the default) would answer a question the prober did not ask -- // `limit=0` would come back as an empty list, indistinguishable from "nothing diff --git a/controller/provider_egress_location_controller.go b/controller/provider_egress_location_controller.go index 2c28c494..4c688150 100644 --- a/controller/provider_egress_location_controller.go +++ b/controller/provider_egress_location_controller.go @@ -159,3 +159,55 @@ func SubmitProviderEgressLocation( return &SubmitProviderEgressLocationResult{LocationId: location.LocationId}, nil } + +// maxProbeFailureLen bounds the failure class as submitted: +// provider_egress_probe_attempt.probe_failure is a varchar(64), and rejecting +// an over-long value with a clear error beats letting the insert panic on a +// Postgres "value too long" error and spin in the retry loop. +const maxProbeFailureLen = 64 + +type RecordProviderEgressProbeAttemptArgs struct { + ClientId server.Id `json:"client_id"` + // ProbeFailure is "" when the attempt succeeded, otherwise a short failure + // class (`contract_failed`, `tunnel_failed`, `no_consensus`, ...). + ProbeFailure string `json:"probe_failure,omitempty"` +} + +type RecordProviderEgressProbeAttemptResult struct { + AttemptAt time.Time `json:"attempt_at"` +} + +// RecordProviderEgressProbeAttempt records that the prober tried this provider. +// A failed attempt defers the provider from the due queue for +// ProviderEgressProbeAttemptBackoff, exactly as a successful probe defers it +// for the (much longer) staleness window -- without this, a provider that +// always fails to probe never gets a provider_egress_location row and so stays +// permanently at the head of the queue, starving every other provider. See +// model.GetProviderEgressLocationDue. +// +// The attempt is timestamped by the server, not the prober: the prober is +// reporting something it just did, and a prober whose clock ran fast could +// otherwise defer a provider far past the backoff window. +func RecordProviderEgressProbeAttempt( + ctx context.Context, + args *RecordProviderEgressProbeAttemptArgs, +) (*RecordProviderEgressProbeAttemptResult, error) { + if maxProbeFailureLen < len(args.ProbeFailure) { + return nil, fmt.Errorf("Probe failure class is too long.") + } + // same check as SubmitProviderEgressLocation: without it a typo'd or stale + // client id writes a row keyed to a client that does not exist, which + // nothing ever reads and only the sweep ever removes. + if networkId := model.GetNetworkClientNetwork(ctx, args.ClientId); networkId == nil { + return nil, fmt.Errorf("Unknown client.") + } + + attemptAt := server.NowUtc() + model.SetProviderEgressProbeAttempt(ctx, &model.ProviderEgressProbeAttempt{ + ClientId: args.ClientId, + AttemptAt: attemptAt, + ProbeFailure: args.ProbeFailure, + }) + + return &RecordProviderEgressProbeAttemptResult{AttemptAt: attemptAt}, nil +} diff --git a/db_migrations.go b/db_migrations.go index dfc27b62..7bcb3344 100644 --- a/db_migrations.go +++ b/db_migrations.go @@ -4414,4 +4414,40 @@ var migrations = []any{ CREATE INDEX IF NOT EXISTS provider_egress_location_observed_at ON provider_egress_location (observed_at) `), + + // provider egress probe attempts: when the prober last *tried* a provider, + // successful or not, and how the try failed. + // + // This cannot live on provider_egress_location, because the case it exists + // to handle is precisely a provider that has no row there. A provider that + // connects, holds a Public provide key and fails every probe (firewalled + // egress, dead upstream) never gets an egress row, so its observed_at stays + // NULL, so it sorts to the head of the due queue forever. Enough of them and + // every batch the prober asks for is the same set of permanently-dead + // providers, and no healthy provider's location is ever refreshed -- while + // the endpoint keeps returning a full, plausible-looking batch. + // GetProviderEgressLocationDue defers on a recent attempt as well as a fresh + // success, which needs somewhere to record the attempt. + // + // Pulled forward from the P2 verdict model + // (docs/superpowers/specs/2026-07-25-enforced-provider-geo-probing-design.md, + // probe_attempt_at / probe_failure) because the P1 schedule cannot function + // without it. Deliberately only the two columns the schedule reads, not the + // rest of that model. + newSqlMigration(` + CREATE TABLE IF NOT EXISTS provider_egress_probe_attempt ( + client_id uuid NOT NULL PRIMARY KEY, + attempt_at timestamp NOT NULL, + probe_failure varchar(64) NOT NULL DEFAULT '', + update_time timestamp NOT NULL + ) + `), + + // serves the sweep in RemoveExpiredProviderEgressProbeAttempts. The due + // query reaches this table by primary key through the left join, so it + // needs no index of its own. + newSqlMigration(` + CREATE INDEX IF NOT EXISTS provider_egress_probe_attempt_attempt_at + ON provider_egress_probe_attempt (attempt_at) + `), } diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 7bf9ae00..55d0aaf3 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -13,6 +13,16 @@ import ( // mmdb lookup on the observed control ip. const ProviderEgressLocationMaxAge = 7 * 24 * time.Hour +// ProviderEgressProbeAttemptBackoff is how long a probe *attempt* defers a +// provider from being offered up again, whether or not the attempt succeeded. +// +// It is much shorter than the staleness window a successful probe buys +// (providerEgressDueAge in api/handlers, half ProviderEgressLocationMaxAge): a +// provider that fails to probe should be retried periodically -- the fault may +// be transient -- just not on every single poll, which is what starves the rest +// of the queue. +const ProviderEgressProbeAttemptBackoff = 6 * time.Hour + // ProviderEgressLocation is a provider location learned by probing the // provider's own egress, rather than by looking up its control-connection ip. type ProviderEgressLocation struct { @@ -86,6 +96,87 @@ func SetProviderEgressLocation(ctx context.Context, e *ProviderEgressLocation) { }) } +// ProviderEgressProbeAttempt records that the prober tried a provider, whether +// or not the try produced a location. +// +// A provider that has never been probed successfully has no +// ProviderEgressLocation row at all, so an attempt cannot be recorded there -- +// see the provider_egress_probe_attempt migration for why that matters. +// ProbeFailure is "" for a successful attempt, otherwise a short failure class +// (`tunnel_failed`, `no_consensus`, ...). +type ProviderEgressProbeAttempt struct { + ClientId server.Id + AttemptAt time.Time + ProbeFailure string + UpdateTime time.Time +} + +// SetProviderEgressProbeAttempt upserts the last probe attempt for a provider. +// +// Like SetProviderEgressLocation the upsert is monotonic in its timestamp: a +// replayed or out-of-order report older than what is already stored is dropped +// rather than moving the provider's last-attempt time backwards, which would +// hand it back to the prober early. +func SetProviderEgressProbeAttempt(ctx context.Context, a *ProviderEgressProbeAttempt) { + server.Tx(ctx, func(tx server.PgTx) { + server.RaisePgResult(tx.Exec( + ctx, + ` + INSERT INTO provider_egress_probe_attempt ( + client_id, + attempt_at, + probe_failure, + update_time + ) + VALUES ($1, $2, $3, $4) + ON CONFLICT (client_id) DO UPDATE + SET + attempt_at = $2, + probe_failure = $3, + update_time = $4 + WHERE provider_egress_probe_attempt.attempt_at < EXCLUDED.attempt_at + `, + a.ClientId, + a.AttemptAt.UTC(), + a.ProbeFailure, + server.NowUtc(), + )) + }) +} + +// GetProviderEgressProbeAttempt returns the last recorded probe attempt for a +// provider, or nil. +func GetProviderEgressProbeAttempt(ctx context.Context, clientId server.Id) *ProviderEgressProbeAttempt { + var a *ProviderEgressProbeAttempt + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query( + ctx, + ` + SELECT + client_id, + attempt_at, + probe_failure, + update_time + FROM provider_egress_probe_attempt + WHERE client_id = $1 + `, + clientId, + ) + server.WithPgResult(result, err, func() { + if result.Next() { + a = &ProviderEgressProbeAttempt{} + server.Raise(result.Scan( + &a.ClientId, + &a.AttemptAt, + &a.ProbeFailure, + &a.UpdateTime, + )) + } + }) + }) + return a +} + // GetProviderEgressLocation returns the stored location for a provider, or nil. func GetProviderEgressLocation(ctx context.Context, clientId server.Id) *ProviderEgressLocation { var e *ProviderEgressLocation @@ -275,15 +366,16 @@ func GetLocation(ctx context.Context, locationId server.Id) *Location { } // GetProviderEgressLocationDue returns the client ids of providers whose -// egress location is due for a probe: their newest probe is older than -// minObservedAt, or they have never been probed at all. Oldest first, so the +// egress location is due for a probe: no fresh success (newest probe older than +// minObservedAt, or never probed) *and* no recent attempt (last attempt older +// than minAttemptAt, or never attempted). Oldest first, so the // longest-unprobed are handed out first, capped at limit. // // This is the durable replacement for the prober's in-memory ttl cache: the // schedule lives in the database, so a prober restart resumes where it left // off instead of re-probing everything. // -// Two things about the shape of this query matter. +// Three things about the shape of this query matter. // // First, candidates are sourced from the live provider population // (network_client_location_reliability, connected + valid) and the egress row @@ -299,12 +391,26 @@ func GetLocation(ctx context.Context, locationId server.Id) *Location { // guaranteed failure. This is the same filter UpdateClientLocations and // UpdateClientScores apply (network_client_location_model.go). // -// The cutoff is computed by the caller in Go and bound as a parameter: -// observed_at is a naive `timestamp` holding utc, and comparing it against sql -// now() would cast through the session timezone and silently skip a window. +// Third, a recent *attempt* defers a provider the same way a recent success +// does. Without that, a provider that connects and holds a Public provide key +// but always fails to probe -- for any reason other than the missing Public key +// screened for above -- never gets an egress row, so its observed_at stays +// NULL, so it sorts ahead of every stale-but-refreshable provider on every +// single poll, forever. Enough such providers to fill a batch and no healthy +// provider is ever refreshed again, while this endpoint goes on returning a +// full, plausible-looking batch. The in-memory ttl cache this replaced was +// incidentally immune, because it marked a provider probed whether or not the +// probe worked; moving the schedule server-side dropped that protection, and +// provider_egress_probe_attempt is what restores it. +// +// Both cutoffs are computed by the caller in Go and bound as parameters: +// observed_at and attempt_at are naive `timestamp` columns holding utc, and +// comparing them against sql now() would cast through the session timezone and +// silently skip a window. func GetProviderEgressLocationDue( ctx context.Context, minObservedAt time.Time, + minAttemptAt time.Time, limit int, ) []server.Id { clientIds := []server.Id{} @@ -319,6 +425,9 @@ func GetProviderEgressLocationDue( LEFT JOIN provider_egress_location ON provider_egress_location.client_id = network_client_location_reliability.client_id + LEFT JOIN provider_egress_probe_attempt ON + provider_egress_probe_attempt.client_id = network_client_location_reliability.client_id + WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND @@ -331,15 +440,26 @@ func GetProviderEgressLocationDue( ( provider_egress_location.observed_at IS NULL OR provider_egress_location.observed_at < $2 + ) AND + ( + provider_egress_probe_attempt.attempt_at IS NULL OR + provider_egress_probe_attempt.attempt_at < $3 ) -- never-probed sorts ahead of merely stale: a missing observed_at - -- is infinitely old - ORDER BY provider_egress_location.observed_at ASC NULLS FIRST - LIMIT $3 + -- is infinitely old. client_id breaks the tie so batch composition + -- is deterministic instead of plan-dependent -- otherwise the whole + -- never-probed population ties on NULL and which slice of it the + -- prober gets back under a limit is whatever order the executor + -- happened to produce. + ORDER BY + provider_egress_location.observed_at ASC NULLS FIRST, + network_client_location_reliability.client_id ASC + LIMIT $4 `, ProvideModePublic, minObservedAt.UTC(), + minAttemptAt.UTC(), limit, ) server.WithPgResult(result, err, func() { @@ -364,3 +484,17 @@ func RemoveExpiredProviderEgressLocations(ctx context.Context, minObservedAt tim )) }) } + +// RemoveExpiredProviderEgressProbeAttempts drops attempts older than +// minAttemptAt. An attempt only carries information for as long as it defers +// the provider (ProviderEgressProbeAttemptBackoff); past that the row is just +// storage held for a client id that may no longer exist. +func RemoveExpiredProviderEgressProbeAttempts(ctx context.Context, minAttemptAt time.Time) { + server.MaintenanceTx(ctx, func(tx server.PgTx) { + server.RaisePgResult(tx.Exec( + ctx, + `DELETE FROM provider_egress_probe_attempt WHERE attempt_at < $1`, + minAttemptAt.UTC(), + )) + }) +} diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index c6f5f8cb..892f01b3 100644 --- a/model/provider_egress_location_model_test.go +++ b/model/provider_egress_location_model_test.go @@ -220,6 +220,7 @@ func TestRemoveExpiredProviderEgressLocations(t *testing.T) { // The caller must run UpdateClientLocationReliabilities afterward -- that is // what rolls the live connection tables up into the // network_client_location_reliability row (connected + valid) the query reads. +// It returns the connection id, so a caller can disconnect the provider again. func testing_connectProbeableProvider( t testing.TB, ctx context.Context, @@ -227,7 +228,7 @@ func testing_connectProbeableProvider( locationId server.Id, clientAddress string, provideMode ProvideMode, -) { +) server.Id { Testing_CreateDevice(ctx, server.NewId(), server.NewId(), clientId, "", "") handlerId := CreateNetworkClientHandler(ctx) @@ -243,6 +244,8 @@ func testing_connectProbeableProvider( SetProvide(ctx, clientId, map[ProvideMode][]byte{ provideMode: []byte("provide-secret"), }) + + return connectionId } // The prober asks the server what to probe next. The answer must be sourced @@ -287,7 +290,9 @@ func TestGetProviderEgressLocationDue(t *testing.T) { }) // `never` and `nonPublic` deliberately get no row at all - due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), 100) + // no attempt rows exist in this test, so the attempt cutoff never + // excludes anything; freshness is the only variable + due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), now, 100) // a provider probed an hour ago must not be re-probed; one probed three // days ago must be; one never probed must be @@ -314,7 +319,7 @@ func TestGetProviderEgressLocationDue(t *testing.T) { } // limit is honoured - limited := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), 1) + limited := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), now, 1) if len(limited) != 1 { t.Fatalf("len(due) = %d for limit 1, want 1", len(limited)) } @@ -323,3 +328,218 @@ func TestGetProviderEgressLocationDue(t *testing.T) { } }) } + +// A provider that connects, holds a Public provide key and fails every probe +// never gets a provider_egress_location row, so its observed_at stays NULL, so +// it sorts ahead of every stale-but-refreshable provider -- forever, on every +// poll. Enough of them to fill a batch and no healthy provider's location is +// ever refreshed again, silently: the endpoint keeps returning a full, +// plausible-looking batch of the same dead providers. +// +// A recent attempt must therefore defer a provider exactly as a fresh success +// does. Deleting the attempt predicate from GetProviderEgressLocationDue must +// fail this test. +func TestGetProviderEgressLocationDueDefersRecentlyAttempted(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) + + // never probed successfully, and the prober just tried it and failed + dead := server.NewId() + // probed successfully three days ago, never attempted since: the + // provider that actually needs the next probe slot + healthyStale := server.NewId() + + testing_connectProbeableProvider(t, ctx, dead, city.LocationId, "0.0.0.1:0", ProvideModePublic) + testing_connectProbeableProvider(t, ctx, healthyStale, city.LocationId, "0.0.0.2:0", ProvideModePublic) + + UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), now) + + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: healthyStale, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-72 * time.Hour), + }) + // `dead` deliberately gets no location row -- it has never succeeded -- + // only a failed attempt seconds ago + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: dead, + AttemptAt: now.Add(-5 * time.Second), + ProbeFailure: "tunnel_failed", + }) + + minObservedAt := now.Add(-24 * time.Hour) + minAttemptAt := now.Add(-ProviderEgressProbeAttemptBackoff) + + due := GetProviderEgressLocationDue(ctx, minObservedAt, minAttemptAt, 100) + + if slices.Contains(due, dead) { + t.Fatalf("due = %v, must not contain the provider attempted seconds ago (%s)", due, dead) + } + if !slices.Contains(due, healthyStale) { + t.Fatalf("due = %v, must contain the stale-but-refreshable provider (%s)", due, healthyStale) + } + + // the starvation itself: with a batch big enough for exactly one + // provider, the slot must go to the one that can actually be refreshed, + // not to the never-probed one that just failed. Without the attempt + // predicate `dead` wins this on observed_at IS NULL every single poll. + limited := GetProviderEgressLocationDue(ctx, minObservedAt, minAttemptAt, 1) + if len(limited) != 1 { + t.Fatalf("len(due) = %d for limit 1, want 1", len(limited)) + } + if limited[0] != healthyStale { + t.Fatalf("due[0] = %s for limit 1, want the refreshable provider %s, not the just-failed one %s", limited[0], healthyStale, dead) + } + + // ... and the deferral is a backoff, not a ban: once the backoff has + // elapsed the same provider is offered again. The caller computes the + // cutoff as (wall clock - backoff), so a poll exactly one backoff period + // after the attempt computes `now`. + afterBackoff := GetProviderEgressLocationDue(ctx, minObservedAt, now, 100) + if !slices.Contains(afterBackoff, dead) { + t.Fatalf("due = %v, must contain the failed provider (%s) again once the attempt backoff has elapsed", afterBackoff, dead) + } + }) +} + +// Only live, routable providers are probeable. A provider that has gone offline +// (connected = false) or that looks messed up from a routing perspective +// (valid = false, a generated column: more than one address hash or location on +// its live connections) must not be handed to the prober. Deleting either +// predicate from GetProviderEgressLocationDue must fail this test. +func TestGetProviderEgressLocationDueRequiresConnectedAndValid(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) + + good := server.NewId() + disconnected := server.NewId() + invalid := server.NewId() + + testing_connectProbeableProvider(t, ctx, good, city.LocationId, "0.0.0.1:0", ProvideModePublic) + disconnectedConnectionId := testing_connectProbeableProvider(t, ctx, disconnected, city.LocationId, "0.0.0.2:0", ProvideModePublic) + + // `invalid` holds two simultaneous connections from two different + // addresses, which makes client_address_hash_count = 2 and so the + // generated `valid` column false. The two addresses must be in + // different /29s: server.ClientIpHash buckets ipv4 to the /29 network, + // so e.g. 0.0.0.3 and 0.0.0.4 would hash the same and count as one. + testing_connectProbeableProvider(t, ctx, invalid, city.LocationId, "0.0.0.3:0", ProvideModePublic) + secondHandlerId := CreateNetworkClientHandler(ctx) + secondConnectionId, _, _, _, err := ConnectNetworkClient(ctx, invalid, "0.0.8.3:0", secondHandlerId) + if err != nil { + t.Fatalf("connect second address: %s", err) + } + if err := SetConnectionLocation(ctx, secondConnectionId, city.LocationId, &ConnectionLocationScores{}); err != nil { + t.Fatalf("set second connection location: %s", err) + } + + // first roll-up: everything above is connected + UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), now) + + // `disconnected` drops off, and a second roll-up flips its reliability + // row's connected to false (the row itself survives) + if err := DisconnectNetworkClient(ctx, disconnectedConnectionId); err != nil { + t.Fatalf("disconnect client: %s", err) + } + UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), server.NowUtc()) + + due := GetProviderEgressLocationDue(ctx, now.Add(-24*time.Hour), now, 100) + + if !slices.Contains(due, good) { + t.Fatalf("due = %v, must contain the connected, valid provider (%s)", due, good) + } + if slices.Contains(due, disconnected) { + t.Fatalf("due = %v, must not contain the disconnected provider (%s)", due, disconnected) + } + if slices.Contains(due, invalid) { + t.Fatalf("due = %v, must not contain the provider whose reliability row is not valid (%s)", due, invalid) + } + }) +} + +// The attempt upsert is monotonic in attempt_at, for the same reason the +// location upsert is: a replayed or out-of-order report must not move the last +// attempt backwards and hand the provider back to the prober early. +func TestProviderEgressProbeAttemptUpsertIgnoresOlderReplay(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientId := server.NewId() + newer := server.NowUtc() + older := newer.Add(-time.Hour) + + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: clientId, AttemptAt: newer, ProbeFailure: "no_consensus", + }) + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: clientId, AttemptAt: older, ProbeFailure: "tunnel_failed", + }) + + got := GetProviderEgressProbeAttempt(ctx, clientId) + if got == nil { + t.Fatal("expected a stored probe attempt") + } + connect.AssertEqual(t, got.ProbeFailure, "no_consensus") + // postgres `timestamp` keeps microseconds, Go keeps nanoseconds, so + // compare with a tolerance rather than for equality + if delta := got.AttemptAt.Sub(newer); delta < -time.Millisecond || time.Millisecond < delta { + t.Fatalf("attempt_at = %s, want the newer attempt %s", got.AttemptAt, newer) + } + + // a strictly newer report does win + newest := newer.Add(time.Minute) + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: clientId, AttemptAt: newest, ProbeFailure: "", + }) + got = GetProviderEgressProbeAttempt(ctx, clientId) + connect.AssertEqual(t, got.ProbeFailure, "") + + // absent + if GetProviderEgressProbeAttempt(ctx, server.NewId()) != nil { + t.Fatal("absent attempt must return nil") + } + }) +} + +func TestRemoveExpiredProviderEgressProbeAttempts(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + keep := server.NewId() + drop := server.NewId() + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: keep, AttemptAt: server.NowUtc(), + }) + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: drop, AttemptAt: server.NowUtc().Add(-30 * 24 * time.Hour), + }) + + RemoveExpiredProviderEgressProbeAttempts(ctx, server.NowUtc().Add(-24*time.Hour)) + + if GetProviderEgressProbeAttempt(ctx, keep) == nil { + t.Fatal("recent attempt must survive the sweep") + } + if GetProviderEgressProbeAttempt(ctx, drop) != nil { + t.Fatal("old attempt must be swept") + } + }) +} diff --git a/taskworker/work/provider_egress_location_work.go b/taskworker/work/provider_egress_location_work.go index 94901c34..533d9609 100644 --- a/taskworker/work/provider_egress_location_work.go +++ b/taskworker/work/provider_egress_location_work.go @@ -33,8 +33,14 @@ func RemoveExpiredProviderEgressLocations( _ *RemoveExpiredProviderEgressLocationsArgs, clientSession *session.ClientSession, ) (*RemoveExpiredProviderEgressLocationsResult, error) { - minObservedAt := server.NowUtc().Add(-4 * model.ProviderEgressLocationMaxAge) + now := server.NowUtc() + minObservedAt := now.Add(-4 * model.ProviderEgressLocationMaxAge) model.RemoveExpiredProviderEgressLocations(clientSession.Ctx, minObservedAt) + // probe attempts stop meaning anything once they no longer defer the + // provider; same reasoning as above, a looser multiple of the window that + // actually matters. + minAttemptAt := now.Add(-4 * model.ProviderEgressProbeAttemptBackoff) + model.RemoveExpiredProviderEgressProbeAttempts(clientSession.Ctx, minAttemptAt) return &RemoveExpiredProviderEgressLocationsResult{}, nil } From e84775e9333255e4fc65dd4cc862f4c813992a9e Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Mon, 27 Jul 2026 03:22:51 +0100 Subject: [PATCH 17/19] fix(controller,model): the egress probe must match existing locations, never create them SubmitProviderEgressLocation validated a probe-supplied city/region only as trimmed, non-empty and <=128 chars, then handed it to model.CreateLocation. CreateLocation dedupes a city on its exact location_name, so an unrecognised spelling did not fail -- it silently inserted a new permanent row into the shared `location` table and added it to the search index. This is not hypothetical. The prober's consensus stores the winning source's original display string, and the three free geolocation APIs demonstrably disagree on spelling: we observed "Frankfurt am Main (Innenstadt I)" against "Frankfurt am Main" for the same host. "Frankfurt am Main", "Frankfurt Am Main" and "Frankfurt/Main" would each become their own row. Those rows are permanent, they survive a code revert, they feed the location search and the provider list, and there is no cleanup path. An ingest endpoint that can add rows to that table is an endpoint that can corrupt it from outside. A geolocation probe has no business defining the world's cities, so the ingest path now resolves against rows that already exist and creates none. model.MatchExistingLocation walks country -> region -> city, trying an exact, fully-indexed match at each level first (the common case: the winning source usually spells it the way the mmdb import did) and falling back to a normalized comparison -- lowercased with punctuation and whitespace dropped -- so the trivial variants land on the row that is already there. Standard library only: a transliteration/fuzzy-match dependency is a lot of new behaviour to take on for a path whose failure mode is already "use the country". Deliberately conservative -- "Frankfurt/Main" folds to "frankfurtmain", does not match "frankfurtammain", and falls back rather than guessing at the wrong row. When nothing resolves, the submission is stored at country granularity instead of being rejected: country is the granularity this design treats as trustworthy, and losing city precision for one probe beats polluting shared data permanently. The country fallback still goes through CreateLocation -- country rows are keyed on country_code, so a variant *name* cannot produce a second row for the same country the way a variant city name can, and a probe from a country not yet in the table must not be dropped. city_confident now records the granularity actually stored rather than what the probe claimed, keeping the schema's documented invariant intact: location_id is a city row exactly when city_confident is set. Tests: an unmatched city stores country granularity and leaves the location row count unchanged; five spelling/case/punctuation variants of one city all resolve to the single existing row and add none. Before this change the unmatched city took the table 3 -> 4 rows and stored as a city, and the variants took it 3 -> 7. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg (cherry picked from commit 682b29b4dccbb30db46542087e7d1708b8970055) --- .../provider_egress_location_controller.go | 54 +++- ...rovider_egress_location_controller_test.go | 173 ++++++++++- model/provider_egress_location_model.go | 280 ++++++++++++++++++ 3 files changed, 494 insertions(+), 13 deletions(-) diff --git a/controller/provider_egress_location_controller.go b/controller/provider_egress_location_controller.go index 4c688150..c8c64832 100644 --- a/controller/provider_egress_location_controller.go +++ b/controller/provider_egress_location_controller.go @@ -126,23 +126,53 @@ func SubmitProviderEgressLocation( } } - // resolve to a canonical location row. city granularity only when the - // probe agreed on a city; otherwise country. - location := &model.Location{ - LocationType: model.LocationTypeCountry, - Country: country, - CountryCode: countryCode, - } + // resolve to a location row. City granularity only when the probe agreed + // on a city AND that city already exists in the location table. + // + // The probe MUST NOT define new cities or regions. model.CreateLocation + // dedupes a city on its exact location_name, so an unrecognised spelling + // does not fail -- it silently inserts a new permanent row into the shared + // `location` table and adds it to the search index. The three free + // geolocation sources the prober reaches consensus over demonstrably + // disagree on spelling ("Frankfurt am Main (Innenstadt I)" vs "Frankfurt am + // Main" for the same host, observed), and the consensus keeps the winning + // source's original display string -- so "Frankfurt am Main", "Frankfurt Am + // Main" and "Frankfurt/Main" would each become their own row. Those rows + // survive a code revert and there is no cleanup path. + // + // model.MatchExistingLocation therefore matches only, never creates, + // case-insensitively and ignoring punctuation/whitespace so the ordinary + // variants land on the row that is already there. When it does not resolve, + // this submission falls back to country granularity: country is the + // granularity this design treats as trustworthy anyway, and losing city + // precision for one probe is strictly better than permanently polluting a + // table shared with the provider list and the location search. + var location *model.Location if args.CityConfident { + location = model.MatchExistingLocation(ctx, countryCode, region, city) + } + + // city_confident records the granularity of the row actually stored, not + // what the probe claimed. The schema's documented invariant is that + // location_id is a city row exactly when city_confident is set (see the + // provider_egress_location migration), and a city-confident probe whose + // city did not resolve is stored at country granularity. + cityConfident := location != nil + + if location == nil { + // country granularity. This still goes through CreateLocation: a + // country row is keyed on country_code, so a variant *name* can never + // produce a second row for the same country the way a variant city name + // can -- the pollution this guards against is not reachable here. The + // country row is also the whole point of the fallback, so a probe from + // a country not yet in the table must not be dropped. location = &model.Location{ - LocationType: model.LocationTypeCity, - City: city, - Region: region, + LocationType: model.LocationTypeCountry, Country: country, CountryCode: countryCode, } + model.CreateLocation(ctx, location) } - model.CreateLocation(ctx, location) model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ ClientId: args.ClientId, @@ -153,7 +183,7 @@ func SubmitProviderEgressLocation( Hosting: args.Hosting, Proxy: args.Proxy, Mobile: args.Mobile, - CityConfident: args.CityConfident, + CityConfident: cityConfident, ObservedAt: args.ObservedAt, }) diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go index d11c08c8..31c7d1cb 100644 --- a/controller/provider_egress_location_controller_test.go +++ b/controller/provider_egress_location_controller_test.go @@ -105,6 +105,18 @@ func TestSubmitProviderEgressLocationCityConfidentStoresCity(t *testing.T) { clientId := server.NewId() model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + // the ingest path resolves against locations that ALREADY exist and + // never creates one, so the city has to be in the table first -- as it + // would be from the mmdb import + denver := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Denver", + Region: "Colorado", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, denver) + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ ClientId: clientId, CountryCode: "us", @@ -123,7 +135,9 @@ func TestSubmitProviderEgressLocationCityConfidentStoresCity(t *testing.T) { } connect.AssertEqual(t, stored.CityConfident, true) - // the resolved location must be the city-granular row + // the resolved location must be the city-granular row that already + // existed, not a new one + connect.AssertEqual(t, stored.LocationId, denver.LocationId) loc := model.GetLocation(ctx, stored.LocationId) if loc == nil { t.Fatal("expected the resolved location row to exist") @@ -387,3 +401,160 @@ func TestSubmitProviderEgressLocationAcceptsLargeAsn(t *testing.T) { connect.AssertEqual(t, stored.ASN, largeAsn) }) } + +// testing_countLocations is the whole point of the two tests below: the +// `location` table is shared with the provider list and the location search, +// its rows are permanent, and nothing cleans up a bad one. An ingest endpoint +// that can add to it is an endpoint that can corrupt it from outside. +func testing_countLocations(ctx context.Context) int64 { + var count int64 + server.Db(ctx, func(conn server.PgConn) { + result, err := conn.Query(ctx, `SELECT COUNT(*) FROM location`) + server.WithPgResult(result, err, func() { + if result.Next() { + server.Raise(result.Scan(&count)) + } + }) + }) + return count +} + +// A city-confident submission whose city is not already in the location table +// must fall back to country granularity and must NOT create a location row. +// +// model.CreateLocation dedupes a city on its exact location_name, so before +// this fix an unrecognised spelling did not fail -- it silently inserted a new +// permanent row and indexed it for search. The prober's consensus stores the +// winning source's original display string and the three free geolocation +// sources demonstrably disagree on spelling, so "Frankfurt am Main", +// "Frankfurt Am Main" and "Frankfurt/Main" would each have become their own +// row. Those rows outlive a code revert and there is no cleanup path. +// +// Reverting the MatchExistingLocation call in SubmitProviderEgressLocation must +// fail this test: the row count goes up and the stored location is a city. +func TestSubmitProviderEgressLocationUnknownCityDoesNotCreateALocation(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + // Germany, and one real German city, already exist -- as they would + // from the mmdb import. The submission below names a DIFFERENT city + // that has never been seen. + model.CreateLocation(ctx, &model.Location{ + LocationType: model.LocationTypeCity, + City: "Frankfurt am Main", + Region: "Hesse", + Country: "Germany", + CountryCode: "de", + }) + + before := testing_countLocations(ctx) + + res, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "de", + Country: "Germany", + Region: "Hesse", + City: "Kleinstadt Nirgendwo", + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + connect.AssertEqual(t, err, nil) + + // nothing was added to the shared table + after := testing_countLocations(ctx) + if after != before { + t.Errorf("location row count went from %d to %d; an unmatched city must not create a permanent row in the shared location table", before, after) + } + + // and the submission was stored at country granularity instead + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatal("expected the submission to be stored") + } + loc := model.GetLocation(ctx, stored.LocationId) + if loc == nil { + t.Fatal("expected the resolved location row to exist") + } + if loc.LocationType != model.LocationTypeCountry { + t.Errorf("stored location_type = %q, want %q: an unmatched city must fall back to country granularity", loc.LocationType, model.LocationTypeCountry) + } + connect.AssertEqual(t, res.LocationId, stored.LocationId) + + // city_confident tracks the granularity actually stored, so the row + // stays internally consistent: location_id is a city row exactly when + // city_confident is set + if stored.CityConfident { + t.Error("city_confident must be false when the submission was stored at country granularity") + } + }) +} + +// The variants that matter are trivial: the same city spelled with different +// case, punctuation or spacing. Those must resolve to the row that is already +// there -- discarding them to country would throw away real precision for no +// reason, and creating a row for each is the bug this guards against. +func TestSubmitProviderEgressLocationMatchesCitySpellingVariant(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), server.NewId(), "", "") + + frankfurt := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Frankfurt am Main", + Region: "Hesse", + Country: "Germany", + CountryCode: "de", + } + model.CreateLocation(ctx, frankfurt) + + before := testing_countLocations(ctx) + + // each of these is a real disagreement between the geolocation sources + // over the same place + variants := []struct{ region, city string }{ + {"Hesse", "Frankfurt am Main"}, // exact + {"Hesse", "Frankfurt Am Main"}, // case + {"hesse", "FRANKFURT AM MAIN"}, // case, both levels + {"Hesse", "Frankfurt-am-Main"}, // punctuation + {"Hesse", " Frankfurt am Main "}, + } + for _, variant := range variants { + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "DE", + Country: "Germany", + Region: variant.region, + City: variant.city, + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + connect.AssertEqual(t, err, nil) + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatalf("%q: expected the submission to be stored", variant.city) + } + if stored.LocationId != frankfurt.LocationId { + t.Errorf("%q resolved to %s, want the existing Frankfurt row %s", variant.city, stored.LocationId, frankfurt.LocationId) + } + if !stored.CityConfident { + t.Errorf("%q: city_confident must stay set when the city resolved", variant.city) + } + } + + if after := testing_countLocations(ctx); after != before { + t.Errorf("location row count went from %d to %d; spelling variants must reuse the existing row, not add new ones", before, after) + } + }) +} diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 55d0aaf3..1bb13eed 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -4,6 +4,7 @@ import ( "context" "strings" "time" + "unicode" "github.com/urnetwork/server" ) @@ -365,6 +366,252 @@ func GetLocation(ctx context.Context, locationId server.Id) *Location { return loc } +// normalizeLocationName folds a location name to a comparison key: lowercased, +// with every rune that is not a letter or a digit dropped. So +// "Frankfurt am Main", "Frankfurt Am Main" and "FRANKFURT AM MAIN" all fold to +// "frankfurtammain" and match the one row that already exists. +// +// This is deliberately a comparison key only -- it is never stored, and never +// used to build a location_name. It exists so a trivial spelling variant from a +// geolocation source resolves to the existing row instead of being treated as a +// different place. +// +// Punctuation is dropped rather than mapped to a space because the disagreement +// is over whether the separator exists at all ("Washington, D.C." vs +// "Washington DC"). Note this deliberately does not fold "Frankfurt/Main" onto +// "Frankfurt am Main": dropping the separator gives "frankfurtmain" != +// "frankfurtammain", so that one falls back to country granularity rather than +// matching the wrong row. Falling back is the safe outcome; guessing is not. +// Stdlib only, by design -- a transliteration/fuzzy-match dependency is a large +// amount of new behaviour to take on for an ingest path whose failure mode is +// already "use the country". +func normalizeLocationName(name string) string { + var b strings.Builder + b.Grow(len(name)) + for _, r := range strings.ToLower(name) { + if unicode.IsLetter(r) || unicode.IsDigit(r) { + b.WriteRune(r) + } + } + return b.String() +} + +// matchLocationNameInTx returns the location_id of the row in `candidates` +// whose location_name matches `name`, preferring an exact match and falling +// back to a normalized one (see normalizeLocationName), or nil for no match. +// Candidates must already be ordered deterministically by the caller so that +// two rows folding to the same key always resolve the same way. +func matchLocationName(name string, candidateIds []server.Id, candidateNames []string) *server.Id { + for i, candidateName := range candidateNames { + if candidateName == name { + return &candidateIds[i] + } + } + normalized := normalizeLocationName(name) + if normalized == "" { + // nothing comparable survives folding (e.g. a name of only + // punctuation); an empty key would match any other such row + return nil + } + for i, candidateName := range candidateNames { + if normalizeLocationName(candidateName) == normalized { + return &candidateIds[i] + } + } + return nil +} + +// MatchExistingLocation resolves (countryCode, region, city) against location +// rows that ALREADY EXIST and returns the city-granular row, or nil if any +// level of the hierarchy does not resolve. It never inserts anything. +// +// This is the resolver the provider egress ingest path uses instead of +// CreateLocation. CreateLocation deduplicates a city on its exact +// location_name, so an unrecognised spelling does not fail -- it silently +// creates a new, permanent row in the shared `location` table and indexes it +// for search. A geolocation probe has no business defining the world's cities: +// the three free sources the prober reaches consensus over demonstrably +// disagree on spelling (we observed "Frankfurt am Main (Innenstadt I)" against +// "Frankfurt am Main" for one host), and the consensus stores the winning +// source's original display string. Each variant would become its own row, +// those rows outlive a code revert, and there is no cleanup path. +// +// Matching is case-insensitive and ignores punctuation and whitespace +// differences, so the ordinary variants resolve to the row that is already +// there. When nothing resolves the caller falls back to country granularity -- +// see SubmitProviderEgressLocation. Falling back loses precision for one +// submission; creating a row corrupts shared data permanently. +// +// Each level tries an exact, fully-indexed match first (the common case: the +// winning source usually spells it the way the mmdb import did) and only scans +// the level's candidates when that misses. +func MatchExistingLocation( + ctx context.Context, + countryCode string, + region string, + city string, +) *Location { + countryCode = strings.ToLower(strings.TrimSpace(countryCode)) + region = strings.TrimSpace(region) + city = strings.TrimSpace(city) + if countryCode == "" || region == "" || city == "" { + return nil + } + + var match *Location + server.Db(ctx, func(conn server.PgConn) { + // country: keyed on country_code alone, exactly as CreateLocation + // dedupes it, so there is no name to match here + var countryLocationId server.Id + var countryName string + found := false + result, err := conn.Query( + ctx, + ` + SELECT location_id, location_name + FROM location + WHERE location_type = $1 AND country_code = $2 + ORDER BY location_id + LIMIT 1 + `, + LocationTypeCountry, + countryCode, + ) + server.WithPgResult(result, err, func() { + if result.Next() { + server.Raise(result.Scan(&countryLocationId, &countryName)) + found = true + } + }) + if !found { + return + } + + // region, within that country + regionLocationId := matchChildLocation( + ctx, + conn, + LocationTypeRegion, + countryCode, + region, + ` + SELECT location_id, location_name + FROM location + WHERE + location_type = $1 AND + country_code = $2 AND + location_name = $3 AND + country_location_id = $4 + `, + ` + SELECT location_id, location_name + FROM location + WHERE + location_type = $1 AND + country_code = $2 AND + country_location_id = $3 + ORDER BY location_id + `, + []any{countryLocationId}, + ) + if regionLocationId == nil { + return + } + + // city, within that region + cityLocationId := matchChildLocation( + ctx, + conn, + LocationTypeCity, + countryCode, + city, + ` + SELECT location_id, location_name + FROM location + WHERE + location_type = $1 AND + country_code = $2 AND + location_name = $3 AND + region_location_id = $4 AND + country_location_id = $5 + `, + ` + SELECT location_id, location_name + FROM location + WHERE + location_type = $1 AND + country_code = $2 AND + region_location_id = $3 AND + country_location_id = $4 + ORDER BY location_id + `, + []any{*regionLocationId, countryLocationId}, + ) + if cityLocationId == nil { + return + } + + match = &Location{ + LocationType: LocationTypeCity, + City: city, + Region: region, + Country: countryName, + CountryCode: countryCode, + LocationId: *cityLocationId, + CityLocationId: *cityLocationId, + RegionLocationId: *regionLocationId, + CountryLocationId: countryLocationId, + } + }) + return match +} + +// matchChildLocation runs the exact-match query first and only falls back to +// scanning the level's candidates when it misses. `parents` are the parent +// location ids the two queries scope on: the exact query binds them after +// (location_type, country_code, name), the candidate query after +// (location_type, country_code). +func matchChildLocation( + ctx context.Context, + conn server.PgConn, + locationType LocationType, + countryCode string, + name string, + exactSql string, + candidatesSql string, + parents []any, +) *server.Id { + exactArgs := append([]any{locationType, countryCode, name}, parents...) + var exactId *server.Id + result, err := conn.Query(ctx, exactSql, exactArgs...) + server.WithPgResult(result, err, func() { + if result.Next() { + var locationId server.Id + var locationName string + server.Raise(result.Scan(&locationId, &locationName)) + exactId = &locationId + } + }) + if exactId != nil { + return exactId + } + + candidateArgs := append([]any{locationType, countryCode}, parents...) + candidateIds := []server.Id{} + candidateNames := []string{} + result, err = conn.Query(ctx, candidatesSql, candidateArgs...) + server.WithPgResult(result, err, func() { + for result.Next() { + var locationId server.Id + var locationName string + server.Raise(result.Scan(&locationId, &locationName)) + candidateIds = append(candidateIds, locationId) + candidateNames = append(candidateNames, locationName) + } + }) + return matchLocationName(name, candidateIds, candidateNames) +} + // GetProviderEgressLocationDue returns the client ids of providers whose // egress location is due for a probe: no fresh success (newest probe older than // minObservedAt, or never probed) *and* no recent attempt (last attempt older @@ -407,6 +654,39 @@ func GetLocation(ctx context.Context, locationId server.Id) *Location { // observed_at and attempt_at are naive `timestamp` columns holding utc, and // comparing them against sql now() would cast through the session timezone and // silently skip a window. +// +// # Two passes, not one +// +// Expressed as a single statement this is a scan of +// network_client_location_reliability with two LEFT JOINs, sorted on +// observed_at from an outer-joined table. That sort cannot use an index: the +// column being ordered on does not exist for most of the rows being ordered. +// At beta's 40 providers that is free. At 100k it is a full scan plus an +// unindexable sort, on every poll. +// +// The ordering makes the split possible. `NULLS FIRST` means every never-probed +// provider sorts ahead of every probed one, so the result is always the +// concatenation of two independently ordered groups: +// +// 1. never probed -- no provider_egress_location row at all. This is the +// dominant group (it is why the ordering is NULLS FIRST), and within it +// every observed_at is equally absent, so the order is client_id alone. As +// an anti-join with no outer-joined column in the ORDER BY it is an ordered +// index scan over (valid, connected, client_id) with a LIMIT: no sort, and +// it stops as soon as the batch is full. +// 2. stale but probed -- has a row, older than minObservedAt. Only reached +// when pass 1 came up short of the limit. Driven from +// provider_egress_location itself, where observed_at is a real, indexable +// column: an ordered range scan over (observed_at, client_id). +// +// Both passes carry the same eligibility predicates, so the concatenation is +// row-for-row what the single statement returned, in the same order, under the +// same limit. `attempt_at IS NULL OR attempt_at < $n` becomes the equivalent +// `NOT EXISTS (... AND $n <= attempt_at)` -- equivalent because client_id is the +// primary key of provider_egress_probe_attempt, so there is at most one row to +// quantify over. The same holds for `observed_at IS NULL` on +// provider_egress_location, whose client_id is likewise a primary key and whose +// observed_at is NOT NULL: the only way that test is true is that no row exists. func GetProviderEgressLocationDue( ctx context.Context, minObservedAt time.Time, From 927f225bd22f090ca72d73822bee9393bbc113d7 Mon Sep 17 00:00:00 2001 From: Ryan Mello Date: Mon, 27 Jul 2026 03:23:25 +0100 Subject: [PATCH 18/19] perf(model): split the provider-egress due query so the common case avoids a sort GetProviderEgressLocationDue scanned network_client_location_reliability, added two LEFT JOINs and an EXISTS, then sorted on observed_at from an outer-joined table. That sort cannot use an index: the column being ordered on does not exist for most of the rows being ordered. At beta's 40 providers it is free. At 100k it is a full scan plus an unindexable sort on every poll. Measured on a 100k-provider dataset (30% probed, 5% recently attempted): before Parallel Seq Scan over all 100k rows, hash-joins both tables, quicksort of ~94k rows (3.3MB), 2282 shared buffers, 62.9 ms after pass 1: Index Only Scan + two Merge Anti Joins, no sort, 138 rows touched, 18 shared buffers, 0.34 ms pass 2: Index Only Scan on the new index, no sort, 350 shared buffers, 1.6 ms The ordering is what makes the split possible. NULLS FIRST means every never-probed provider sorts ahead of every probed one, so the result is always the concatenation of two independently ordered groups: 1. never probed -- the dominant group, which is why the ordering is NULLS FIRST. Within it observed_at is equally absent, so client_id alone orders it, and as an anti-join with no outer-joined column in the ORDER BY it is an ordered index scan over the existing (valid, connected, client_id) index that stops as soon as the batch is full. 2. stale but probed -- only queried when pass 1 came up short of the limit. Driven from provider_egress_location, where observed_at is a real, indexable column. `attempt_at IS NULL OR attempt_at < $n` becomes `NOT EXISTS (... AND $n <= attempt_at)`, equivalent because client_id is the primary key of provider_egress_probe_attempt. Same for `observed_at IS NULL`, whose table also keys on client_id and whose observed_at is NOT NULL -- the only way that test is true is that no row exists. The two passes are separate snapshots, so a client that gains its first egress row between them is screened out rather than handed to the prober twice; the single statement could not do that. Verified row-for-row identical to the single statement on the 100k dataset at limits 0, 1, 2, 50, 100, 1000, 20000, 69000, 69500, 70000, 70500 and 100000 -- the middle four straddle the pass-one/pass-two seam (the never-probed population is ~70138) and the last exhausts the eligible set at 94088. New migration adds (observed_at, client_id) on provider_egress_location so pass 2's predicate and its full ORDER BY, tie-break included, are one ordered index scan; without it the planner adds an Incremental Sort (936 buffers vs 350). Appended, never inserted -- migrations apply by slice index here, so editing or reordering an applied one corrupts live databases. Pass 1 needs no new index. TestGetProviderEgressLocationDueOrderingIsStableAcrossLimits asserts the full ordering and then that every limit from 0 past the end returns exactly its prefix, over a population covering never-probed, stale, fresh, recently-attempted, stale-AND-recently-attempted and non-public. It passes against the old single statement as well as the new pair -- it is a behaviour-preservation test, not a description of the new code -- and fails against a pass 2 that uses the full limit instead of the remainder, and against a pass 2 that drops the attempt backoff. The stale-AND-attempted provider exists because of that second check: without it nothing in the suite covered the only case pass 2's backoff predicate screens, and the broken variant passed. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg (cherry picked from commit 37ca6cd7202afbb2327fae5681cdd9342cc9b88e) --- db_migrations.go | 27 +++++ model/provider_egress_location_model.go | 108 +++++++++++++---- model/provider_egress_location_model_test.go | 121 +++++++++++++++++++ 3 files changed, 235 insertions(+), 21 deletions(-) diff --git a/db_migrations.go b/db_migrations.go index 7bcb3344..09d920f9 100644 --- a/db_migrations.go +++ b/db_migrations.go @@ -4450,4 +4450,31 @@ var migrations = []any{ CREATE INDEX IF NOT EXISTS provider_egress_probe_attempt_attempt_at ON provider_egress_probe_attempt (attempt_at) `), + + // serves the stale-but-probed pass of GetProviderEgressLocationDue, which + // drives from provider_egress_location with `observed_at < $n ORDER BY + // observed_at, client_id LIMIT $m`. With client_id in the index the + // predicate and the whole ORDER BY -- tie-break included -- are one ordered + // index scan that stops when the batch is full: no sort, and no heap visit + // to resolve the tie. The pre-existing (observed_at) index alone leaves the + // client_id tie-break to a sort. + // + // The other pass (never-probed) needs no new index: it is an anti-join over + // network_client_location_reliability ordered by client_id, which the + // existing (valid, connected, client_id) index already serves as an ordered + // scan, and both anti-joins plus the provide_key EXISTS are primary-key + // probes. + // + // This supersedes provider_egress_location_observed_at, which is now a + // prefix of it -- including for the RemoveExpiredProviderEgressLocations + // sweep. The redundant index is left in place deliberately: dropping it is a + // separate decision with its own (small) risk, and this migration is meant + // to be purely additive. + // + // Appended, never inserted: migrations here apply by slice index, so + // editing or reordering an already-applied entry corrupts live databases. + newSqlMigration(` + CREATE INDEX IF NOT EXISTS provider_egress_location_observed_at_client_id + ON provider_egress_location (observed_at, client_id) + `), } diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 1bb13eed..88ee8603 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -695,6 +695,13 @@ func GetProviderEgressLocationDue( ) []server.Id { clientIds := []server.Id{} server.Db(ctx, func(conn server.PgConn) { + // pass 1: never probed. Ordered by client_id alone -- every row in this + // group has no observed_at, so the ORDER BY's leading key is constant + // across it and the tie-break is the whole ordering. + // + // `limit` is passed through as given rather than clamped, so a + // nonsensical limit fails exactly as the single-statement version did + // (LIMIT 0 returns nothing; a negative limit is an error). result, err := conn.Query( ctx, ` @@ -702,12 +709,6 @@ func GetProviderEgressLocationDue( network_client_location_reliability.client_id FROM network_client_location_reliability - LEFT JOIN provider_egress_location ON - provider_egress_location.client_id = network_client_location_reliability.client_id - - LEFT JOIN provider_egress_probe_attempt ON - provider_egress_probe_attempt.client_id = network_client_location_reliability.client_id - WHERE network_client_location_reliability.connected = true AND network_client_location_reliability.valid = true AND @@ -717,35 +718,100 @@ func GetProviderEgressLocationDue( provide_key.client_id = network_client_location_reliability.client_id AND provide_key.provide_mode = $1 ) AND - ( - provider_egress_location.observed_at IS NULL OR - provider_egress_location.observed_at < $2 + NOT EXISTS ( + SELECT 1 FROM provider_egress_location + WHERE + provider_egress_location.client_id = network_client_location_reliability.client_id + ) AND + NOT EXISTS ( + SELECT 1 FROM provider_egress_probe_attempt + WHERE + provider_egress_probe_attempt.client_id = network_client_location_reliability.client_id AND + $2 <= provider_egress_probe_attempt.attempt_at + ) + + ORDER BY network_client_location_reliability.client_id ASC + LIMIT $3 + `, + ProvideModePublic, + minAttemptAt.UTC(), + limit, + ) + server.WithPgResult(result, err, func() { + for result.Next() { + var clientId server.Id + server.Raise(result.Scan(&clientId)) + clientIds = append(clientIds, clientId) + } + }) + + remaining := limit - len(clientIds) + if remaining <= 0 { + // the batch is full from never-probed providers alone, which is the + // steady state until the population has been swept once. The + // single-statement version would have returned exactly these rows + // too: they all sort ahead of anything with an observed_at. + return + } + + // pass 2: stale but probed. Driven from provider_egress_location, so + // observed_at is a real column of the driving table and the ORDER BY is + // an ordered index scan rather than a sort. + result, err = conn.Query( + ctx, + ` + SELECT + provider_egress_location.client_id + FROM provider_egress_location + + INNER JOIN network_client_location_reliability ON + network_client_location_reliability.client_id = provider_egress_location.client_id + + WHERE + provider_egress_location.observed_at < $2 AND + 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 = provider_egress_location.client_id AND + provide_key.provide_mode = $1 ) AND - ( - provider_egress_probe_attempt.attempt_at IS NULL OR - provider_egress_probe_attempt.attempt_at < $3 + NOT EXISTS ( + SELECT 1 FROM provider_egress_probe_attempt + WHERE + provider_egress_probe_attempt.client_id = provider_egress_location.client_id AND + $3 <= provider_egress_probe_attempt.attempt_at ) - -- never-probed sorts ahead of merely stale: a missing observed_at - -- is infinitely old. client_id breaks the tie so batch composition - -- is deterministic instead of plan-dependent -- otherwise the whole - -- never-probed population ties on NULL and which slice of it the - -- prober gets back under a limit is whatever order the executor - -- happened to produce. + -- oldest probe first, client_id breaking the tie, so batch + -- composition is deterministic instead of plan-dependent ORDER BY - provider_egress_location.observed_at ASC NULLS FIRST, - network_client_location_reliability.client_id ASC + provider_egress_location.observed_at ASC, + provider_egress_location.client_id ASC LIMIT $4 `, ProvideModePublic, minObservedAt.UTC(), minAttemptAt.UTC(), - limit, + remaining, ) server.WithPgResult(result, err, func() { + // the two passes are separate statements and so separate snapshots. + // A provider that gains its first provider_egress_location row + // between them would be never-probed to pass 1 and stale to pass 2; + // the single-statement version could not do that, so screen it out + // rather than hand the prober the same client twice. + 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) } }) diff --git a/model/provider_egress_location_model_test.go b/model/provider_egress_location_model_test.go index 892f01b3..f3acd4c3 100644 --- a/model/provider_egress_location_model_test.go +++ b/model/provider_egress_location_model_test.go @@ -2,6 +2,7 @@ package model import ( "context" + "fmt" "slices" "testing" "time" @@ -543,3 +544,123 @@ func TestRemoveExpiredProviderEgressProbeAttempts(t *testing.T) { } }) } + +// GetProviderEgressLocationDue is served by two statements -- never-probed +// first, then stale-but-probed only when the first came up short -- because the +// single-statement form sorts on observed_at from an outer-joined table, which +// cannot use an index and becomes a full scan plus an unindexable sort at 100k +// providers. +// +// The split is only safe if the concatenation is row-for-row what one statement +// returned, at every limit. That is what this asserts: it builds one population +// covering every eligibility case and then walks the limit from 0 past the end, +// requiring each result to be exactly the prefix of the full ordering. Limit 3 +// is the seam (pass one exactly fills the batch) and limit 4 is the first that +// crosses into pass two. +func TestGetProviderEgressLocationDueOrderingIsStableAcrossLimits(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) + + // three never-probed providers: the dominant group, and the reason the + // ordering is NULLS FIRST + never := []server.Id{server.NewId(), server.NewId(), server.NewId()} + // two stale ones, at different ages -- the older must be handed out first + staleOlder := server.NewId() + staleNewer := server.NewId() + // probed an hour ago: not due + fresh := server.NewId() + // never probed, but attempted seconds ago: deferred by the backoff, and + // the case that would otherwise starve the queue + attempted := server.NewId() + // probed long ago AND attempted seconds ago. This one is only screened + // by the backoff predicate on the stale-but-probed pass -- the + // never-probed pass never sees it, because it has an egress row. Drop + // that predicate and it reappears in the batch. + staleAttempted := server.NewId() + // no Public provide key: unprobeable at any freshness + nonPublic := server.NewId() + + address := 0 + connectProvider := func(clientId server.Id, provideMode ProvideMode) { + address += 1 + testing_connectProbeableProvider( + t, ctx, clientId, city.LocationId, + fmt.Sprintf("0.0.%d.1:0", address), provideMode, + ) + } + for _, clientId := range never { + connectProvider(clientId, ProvideModePublic) + } + connectProvider(staleOlder, ProvideModePublic) + connectProvider(staleNewer, ProvideModePublic) + connectProvider(fresh, ProvideModePublic) + connectProvider(attempted, ProvideModePublic) + connectProvider(staleAttempted, ProvideModePublic) + connectProvider(nonPublic, ProvideModeNetwork) + + UpdateClientLocationReliabilities(ctx, now.Add(-time.Hour), now) + + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: staleOlder, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-100 * time.Hour), + }) + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: staleNewer, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-50 * time.Hour), + }) + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: fresh, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-1 * time.Hour), + }) + SetProviderEgressLocation(ctx, &ProviderEgressLocation{ + ClientId: staleAttempted, LocationId: city.LocationId, + CountryCode: "us", ObservedAt: now.Add(-200 * time.Hour), + }) + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: attempted, AttemptAt: now.Add(-5 * time.Second), + ProbeFailure: "tunnel_failed", + }) + // oldest observed_at of all, so it would sort to the head of the + // stale group if the backoff did not exclude it + SetProviderEgressProbeAttempt(ctx, &ProviderEgressProbeAttempt{ + ClientId: staleAttempted, AttemptAt: now.Add(-5 * time.Second), + ProbeFailure: "tunnel_failed", + }) + + minObservedAt := now.Add(-24 * time.Hour) + minAttemptAt := now.Add(-ProviderEgressProbeAttemptBackoff) + + // the never-probed group ties on a missing observed_at, so client_id + // alone orders it -- and postgres orders uuid by bytes, which is what + // server.Id.Cmp does + expected := slices.Clone(never) + slices.SortFunc(expected, func(a server.Id, b server.Id) int { return a.Cmp(b) }) + // ... then the probed group, oldest probe first + expected = append(expected, staleOlder, staleNewer) + + due := GetProviderEgressLocationDue(ctx, minObservedAt, minAttemptAt, 100) + if !slices.Equal(due, expected) { + t.Fatalf("due = %v, want %v (never-probed by client_id, then stale oldest-first; fresh/attempted/stale-attempted/non-public excluded)", due, expected) + } + + // every limit must return exactly the prefix of that ordering. limit 3 + // is the pass-one/pass-two seam; 4 is the first to cross it. + for limit := 0; limit <= len(expected)+2; limit += 1 { + want := expected[:min(limit, len(expected))] + got := GetProviderEgressLocationDue(ctx, minObservedAt, minAttemptAt, limit) + if !slices.Equal(got, want) { + t.Errorf("limit %d: due = %v, want %v", limit, got, want) + } + } + }) +} From 6913b87afa855926991fe6423bd9411c7ad350ca Mon Sep 17 00:00:00 2001 From: ryanmello07 Date: Mon, 27 Jul 2026 06:09:19 +0100 Subject: [PATCH 19/19] fix(egress): fold accents and district qualifiers, and never coarsen a probed location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the provider-egress probe ingest path. 1. The location-name normaliser dropped the cases it exists for. normalizeLocationName kept accented letters, so "São Paulo" did not fold onto "Sao Paulo", "Zürich" onto "Zurich", or "Kraków" onto "Krakow" -- every one of those missed and fell back to country. The motivating example missed too: "Frankfurt am Main (Innenstadt I)" folded to "frankfurtammaininnenstadti", because dropping "(" and ")" as punctuation leaves the qualifier's letters in the key. The fold now NFD-decomposes and drops the combining marks (unicode.Mn), which covers the whole accent class instead of a hand-rolled é->e table that would keep missing whatever it forgot. golang.org/x/text was already a dependency of this module and is now required directly; the stdlib-only constraint the previous revision was written under belongs to the prober repo's `geolocate` package, not to the server. matchLocationName gains a third pass that strips parenthesised qualifiers from both sides. That pass is the only one that can plausibly land on the wrong row -- "Springfield (IL)" and "Springfield (MA)" both reduce to "springfield" -- so it requires the stripped key to identify exactly one candidate and declines on any ambiguity. Falling back to country is the safe outcome; guessing is not. Letters carrying a stroke rather than a combining mark (ł, ø, đ) still do not fold. That is the documented residual: closing it needs the transliteration table this deliberately avoids, and the failure mode is country granularity, not a wrong answer. The existing variant test only covered foldings the implementation already handled, which is why it could not catch any of this. It now carries the qualifier cases, and accents get their own test covering both directions -- accent on the probe, accent on the stored row -- since which side carries it depends on how the row happened to be seeded. Both fail against the previous matcher. 2. An unmatched city made a provider worse off than never being probed. SubmitProviderEgressLocation stores country granularity whenever the probed city does not match an existing location row, and SetConnectionLocation then let that probed row win unconditionally over the mmdb lookup. A provider the mmdb had placed in a city was therefore demoted to a country row and disappeared from every city filter: being probed made it less discoverable than being ignored. This is the common case, not a rare one. Cities are not seeded -- AddDefaultLocations runs with cityLimit = 0 -- so a probed city can only match rows organic traffic already created. SetConnectionLocation now resolves the mmdb answer first (an in-process mmdb/ARIN read, no db round trip) and asks probedLocationPreferred whether replacing it is actually an improvement. The rule is that a probe may correct a location but never coarsen it: - a city-confident probe wins; it loses nothing; - a probe wins when mmdb has no answer, or only a country; - a probe wins when mmdb's city is in a DIFFERENT country -- that city is not more precise, it is precisely wrong, and correcting it is the entire point of the feature; - mmdb wins when its city or region is in the SAME country the probe reports, because the probe then adds nothing but a loss of granularity. The reasoning is written out at the function, because the naive reading ("a probe is better evidence, so it always wins") is what produced the regression. Three tests cover it: the anti-coarsening case fails against the old unconditional preference, and the two correction cases fail against a rule that merely keeps whichever answer is finer. Fix 1 materially reduces how often fix 2 is reached, but neither depends on the other. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg --- controller/network_client_controller.go | 105 +++++++-- controller/network_client_controller_test.go | 208 ++++++++++++++++++ controller/probed_location_preferred_test.go | 75 +++++++ .../provider_egress_location_controller.go | 6 +- ...rovider_egress_location_controller_test.go | 198 ++++++++++++++++- go.mod | 2 +- model/provider_egress_location_model.go | 123 +++++++++-- 7 files changed, 677 insertions(+), 40 deletions(-) create mode 100644 controller/probed_location_preferred_test.go diff --git a/controller/network_client_controller.go b/controller/network_client_controller.go index 4a723b6e..3556cdb1 100644 --- a/controller/network_client_controller.go +++ b/controller/network_client_controller.go @@ -3,7 +3,7 @@ package controller import ( "context" "net/netip" - // "strings" + "strings" "time" "github.com/urnetwork/glog" @@ -57,13 +57,24 @@ func SetConnectionLocation( connectionId server.Id, clientIp string, ) error { + // the mmdb lookup on the control ip. This is resolved up front even when + // the probed path is about to win, because the probed path has to know how + // precise the mmdb answer is before it can decide whether replacing it is + // an improvement (see probedLocationPreferred), and because it costs no db + // round trip -- it is an in-process maxminddb read plus an ARIN lookup. + // `err` is deliberately not returned yet: a failed mmdb lookup is not a + // reason to discard a perfectly good probed location. + location, connectionLocationScores, err := GetLocationForIp(ctx, clientIp) + // a provider probed through its own egress is located from that probe, not // from a lookup on its control-connection ip: the egress is where user // traffic actually exits, and an operator-run prober learns it by routing // geolocation lookups through the provider itself and cross-checking them // across several sources, then submits the result here. When a fresh // probed entry exists we prefer it over the built-in mmdb lookup on the - // control ip. GetFreshProviderEgressLocationForConnection is + // control ip -- subject to probedLocationPreferred, which is what stops the + // probe making a provider *less* locatable than it was. + // GetFreshProviderEgressLocationForConnection is // a single query joining network_client_connection to // provider_egress_location: this runs for every connection (provider or // not) on the connect-announce path and inside a retry loop, so it must @@ -73,7 +84,7 @@ func SetConnectionLocation( ctx, connectionId, model.ProviderEgressLocationMaxAge, - ); egress != nil { + ); egress != nil && probedLocationPreferred(egress, location) { scores := &model.ConnectionLocationScores{} if egress.Hosting { scores.NetTypeHosting = 1 @@ -92,30 +103,34 @@ func SetConnectionLocation( // the same parity reasoning applied to net_type_foreign). Mobile // stays on the model/wire contract as metadata; it just does not // feed the ranking score. + // keep the ARIN org-vs-country foreign check on the probed path too, // so a probed provider is ranked on equal terms with an equivalent - // unprobed one (net_type_foreign feeds the ranking columns). Compute - // it exactly as the mmdb path does in GetLocationForIp: the ARIN org - // country of the control ip against the mmdb country of that SAME - // control ip -- not the probed country, which is a different + // unprobed one (net_type_foreign feeds the ranking columns). It is + // computed exactly as the mmdb path computes it in GetLocationForIp: + // the ARIN org country of the control ip against the mmdb country of + // that SAME control ip -- not the probed country, which is a different // question (whether probing changed the answer) and must not be - // silently folded into this ranking penalty. Any lookup failure - // just leaves NetTypeForeign at 0; it must never fail or panic this - // path. + // silently folded into this ranking penalty. It is recomputed here + // rather than lifted off connectionLocationScores because that struct + // is nil whenever GetLocationForIp failed, including the case where + // mmdb resolved the ip fine but GuessLocationType could not classify + // the result -- the foreign check is still meaningful there. Any + // lookup failure just leaves NetTypeForeign at 0; it must never fail + // or panic this path. if addr, err := netip.ParseAddr(clientIp); err == nil { if ipInfo, err := server.GetIpInfo(addr); err == nil { scores.NetTypeForeign = arinForeignScore(addr, ipInfo.CountryCode) } } - err := model.SetConnectionLocation(ctx, connectionId, egress.LocationId, scores) - if err == nil { + setErr := model.SetConnectionLocation(ctx, connectionId, egress.LocationId, scores) + if setErr == nil { return nil } // fall through to the mmdb path on a storage error - glog.Infof("[ncc][%s]could not set probed egress location. err = %s\n", connectionId, err) + glog.Infof("[ncc][%s]could not set probed egress location. err = %s\n", connectionId, setErr) } - location, connectionLocationScores, err := GetLocationForIp(ctx, clientIp) if err != nil { // server.Logger().Printf("Get ip for location error: %s", err) glog.Infof("[ncc][%s]could not find client location. err = %s\n", connectionId, err) @@ -132,6 +147,68 @@ func SetConnectionLocation( return nil } +// probedLocationPreferred decides whether the probed egress location should be +// written for this connection instead of the mmdb location resolved from the +// control ip. mmdbLocation is nil when the mmdb lookup failed. +// +// Read this before changing it: the naive rule -- "a probe is better evidence +// than mmdb, so the probe always wins" -- is wrong, and produced a live +// regression. The probed location is not always as *precise* as the mmdb one. +// SubmitProviderEgressLocation only stores a city when the probed city matches +// a location row that already exists; anything else is stored at country +// granularity, deliberately, so that a probe can never mint new city rows in +// the shared `location` table. Cities are not seeded either -- AddDefaultLocations +// runs with cityLimit = 0 -- so the pool a probed city can match against is only +// the rows organic traffic happened to create, and a country-granular fallback +// is the common case, not a rare one. +// +// Letting that country row overwrite an mmdb *city* row would drop the provider +// out of every city filter in FindProviders2 and GetProviderLocations. Being +// probed would make a provider less discoverable than never having been probed +// at all -- a penalty for participating. +// +// So the rule is: a probe may CORRECT the location, but never COARSEN it. +// +// - The probe stored a city (CityConfident): it is at least as precise as +// anything mmdb has, and it is better evidence. It wins. +// - No usable mmdb answer: the probe is the only evidence there is. It wins. +// - The mmdb answer is itself country-granular: nothing to lose. The probe +// wins, and this is the case country-level correction exists for. +// - The mmdb answer is city- or region-granular in a DIFFERENT country: the +// mmdb row is not more precise, it is precisely wrong, and its city is a +// city in the wrong country. The probe wins. This is the other half of +// country-level correction and the reason this is not just "keep whichever +// is finer". +// - The mmdb answer is city- or region-granular in the SAME country the probe +// reports: the probe agrees with mmdb and adds nothing except a loss of +// granularity. mmdb wins. +// +// CityConfident is used as the probed row's granularity because the schema +// invariant is that provider_egress_location.location_id is a city row exactly +// when city_confident is set (see the provider_egress_location migration and +// SubmitProviderEgressLocation). Reading it off the flag keeps this on the hot +// connect-announce path without a second query for the location row's type. +func probedLocationPreferred( + egress *model.ProviderEgressLocation, + mmdbLocation *model.Location, +) bool { + if egress.CityConfident { + return true + } + if mmdbLocation == nil { + return true + } + if mmdbLocation.LocationType != model.LocationTypeCity && + mmdbLocation.LocationType != model.LocationTypeRegion { + return true + } + // both country codes are stored lowercased -- SubmitProviderEgressLocation + // lowercases the probed one and GetLocationForIp takes mmdb's, which the + // location table also stores lowercased -- but fold anyway rather than let + // a casing difference read as a country disagreement and silently coarsen. + return !strings.EqualFold(egress.CountryCode, mmdbLocation.CountryCode) +} + /* func SetMissingConnectionLocations(ctx context.Context, minTime time.Time) { connectionIpStrs := map[server.Id]string{} diff --git a/controller/network_client_controller_test.go b/controller/network_client_controller_test.go index f6561fb1..1276bf9d 100644 --- a/controller/network_client_controller_test.go +++ b/controller/network_client_controller_test.go @@ -333,3 +333,211 @@ func TestSetConnectionLocationMapsProbedFlagsToScores(t *testing.T) { connect.AssertEqual(t, netTypeVirtual, 0) }) } + +// testing_connectionLocationIds reads back the granularity actually stored for +// a connection. +func testing_connectionLocationIds(ctx context.Context, connectionId server.Id) ( + cityLocationId server.Id, + regionLocationId server.Id, + countryLocationId server.Id, +) { + server.Db(ctx, func(conn server.PgConn) { + result, qerr := conn.Query( + ctx, + ` + SELECT city_location_id, region_location_id, country_location_id + FROM network_client_location + WHERE connection_id = $1 + `, + connectionId, + ) + server.WithPgResult(result, qerr, func() { + if result.Next() { + server.Raise(result.Scan( + &cityLocationId, + ®ionLocationId, + &countryLocationId, + )) + } + }) + }) + return +} + +// A probe must never make a provider LESS locatable than leaving it unprobed +// would have. +// +// SubmitProviderEgressLocation stores country granularity whenever the probed +// city does not match a location row that already exists, which is the common +// case -- cities are not seeded, so the match pool is only what organic traffic +// created. If that country row then overwrote the mmdb city row unconditionally, +// the provider would fall out of every city filter: probed providers would be +// harder to find than unprobed ones, which is the exact opposite of the point. +// +// mmdb resolves 24.48.0.1 to Montreal, Quebec, CA at city granularity. A +// country-only probe agreeing that the provider is in CA adds nothing, so the +// mmdb city has to survive. +func TestSetConnectionLocationProbedCountryDoesNotCoarsenMmdbCity(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientIp := "24.48.0.1" + + mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) + connect.AssertEqual(t, err, nil) + // the fixture is only meaningful if mmdb really has a city here + connect.AssertEqual(t, mmdbLocation.LocationType, model.LocationTypeCity) + model.CreateLocation(ctx, mmdbLocation) + + // the probed location: the same country, but only the country -- + // exactly what an unmatched city name falls back to + probed := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: "Canada", + CountryCode: "ca", + } + model.CreateLocation(ctx, probed) + connect.AssertEqual(t, probed.LocationId, mmdbLocation.CountryLocationId) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) + connect.AssertEqual(t, err, nil) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "ca", + CityConfident: false, + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, clientIp) + connect.AssertEqual(t, err, nil) + + cityLocationId, regionLocationId, countryLocationId := testing_connectionLocationIds(ctx, connectionId) + if cityLocationId != mmdbLocation.CityLocationId { + t.Errorf( + "city_location_id = %s, want the mmdb city %s: a country-only probe must not coarsen a connection the mmdb already placed in a city", + cityLocationId, + mmdbLocation.CityLocationId, + ) + } + connect.AssertEqual(t, regionLocationId, mmdbLocation.RegionLocationId) + connect.AssertEqual(t, countryLocationId, mmdbLocation.CountryLocationId) + // and the collapse this guards against, stated directly + if cityLocationId == countryLocationId { + t.Errorf("city/region/country all collapsed to %s; the provider is no longer in any city filter", countryLocationId) + } + }) +} + +// The other half of the rule: country-level CORRECTION still wins. When the +// probe says the egress is in a different country than the control ip's mmdb +// city, that city is a city in the wrong country -- it is not more precise, it +// is precisely wrong -- so the probed country replaces it. This is the whole +// reason the feature exists and must not be lost to the anti-coarsening rule +// above. +func TestSetConnectionLocationProbedCountryCorrectsMmdbCityInAnotherCountry(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientIp := "24.48.0.1" + + mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, mmdbLocation.LocationType, model.LocationTypeCity) + connect.AssertEqual(t, mmdbLocation.CountryCode, "ca") + model.CreateLocation(ctx, mmdbLocation) + + probed := &model.Location{ + LocationType: model.LocationTypeCountry, + Country: "Japan", + CountryCode: "jp", + } + model.CreateLocation(ctx, probed) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) + connect.AssertEqual(t, err, nil) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "jp", + CityConfident: false, + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, clientIp) + connect.AssertEqual(t, err, nil) + + _, _, countryLocationId := testing_connectionLocationIds(ctx, connectionId) + if countryLocationId != probed.CountryLocationId { + t.Errorf( + "country_location_id = %s, want the probed country %s: a probe that disagrees with mmdb about the country must still correct it", + countryLocationId, + probed.CountryLocationId, + ) + } + if countryLocationId == mmdbLocation.CountryLocationId { + t.Errorf("kept the mmdb country %s; the provider is still advertised in the wrong country", mmdbLocation.CountryLocationId) + } + }) +} + +// A city-confident probe is at least as precise as anything mmdb has and is +// better evidence, so it wins outright -- including against an mmdb city in +// another country. Nothing is coarsened, so the anti-coarsening rule does not +// apply. +func TestSetConnectionLocationProbedCityWinsOverMmdbCity(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + clientIp := "24.48.0.1" + + mmdbLocation, _, err := GetLocationForIp(ctx, clientIp) + connect.AssertEqual(t, err, nil) + connect.AssertEqual(t, mmdbLocation.LocationType, model.LocationTypeCity) + model.CreateLocation(ctx, mmdbLocation) + + probed := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Tokyo", + Region: "Tokyo", + Country: "Japan", + CountryCode: "jp", + } + model.CreateLocation(ctx, probed) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + handlerId := model.CreateNetworkClientHandler(ctx) + connectionId, _, _, _, err := model.ConnectNetworkClient(ctx, clientId, clientIp+":0", handlerId) + connect.AssertEqual(t, err, nil) + + model.SetProviderEgressLocation(ctx, &model.ProviderEgressLocation{ + ClientId: clientId, + LocationId: probed.LocationId, + CountryCode: "jp", + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + + err = SetConnectionLocation(ctx, connectionId, clientIp) + connect.AssertEqual(t, err, nil) + + cityLocationId, _, countryLocationId := testing_connectionLocationIds(ctx, connectionId) + connect.AssertEqual(t, cityLocationId, probed.CityLocationId) + connect.AssertEqual(t, countryLocationId, probed.CountryLocationId) + }) +} diff --git a/controller/probed_location_preferred_test.go b/controller/probed_location_preferred_test.go new file mode 100644 index 00000000..33ac26a2 --- /dev/null +++ b/controller/probed_location_preferred_test.go @@ -0,0 +1,75 @@ +package controller + +import ( + "testing" + + "github.com/urnetwork/server" + "github.com/urnetwork/server/model" +) + +// The decision rule on its own, with no database and no mmdb file: a probe may +// CORRECT a location but must never COARSEN it. The end-to-end behaviour is +// covered by the TestSetConnectionLocation* tests; this pins the rule itself so +// each clause is exercised independently of whichever mmdb the environment has. +// +// Every row here fails against at least one plausible wrong rule -- "the probe +// always wins" (the bug), or "keep whichever answer is finer" (the overcorrection +// that would throw away country correction). +func TestProbedLocationPreferred(t *testing.T) { + probedCity := func(countryCode string) *model.ProviderEgressLocation { + return &model.ProviderEgressLocation{ + LocationId: server.NewId(), + CountryCode: countryCode, + CityConfident: true, + } + } + probedCountry := func(countryCode string) *model.ProviderEgressLocation { + return &model.ProviderEgressLocation{ + LocationId: server.NewId(), + CountryCode: countryCode, + CityConfident: false, + } + } + mmdb := func(locationType string, countryCode string) *model.Location { + return &model.Location{ + LocationId: server.NewId(), + LocationType: locationType, + CountryCode: countryCode, + } + } + + tests := []struct { + name string + egress *model.ProviderEgressLocation + mmdb *model.Location + want bool + }{ + // the regression: a country-only probe must not replace an mmdb city + // in the same country. This is the row that fails against "the probe + // always wins". + {"country probe vs mmdb city, same country", probedCountry("ca"), mmdb(model.LocationTypeCity, "ca"), false}, + {"country probe vs mmdb region, same country", probedCountry("ca"), mmdb(model.LocationTypeRegion, "ca"), false}, + // casing must not read as a country disagreement + {"country probe vs mmdb city, same country cased", probedCountry("CA"), mmdb(model.LocationTypeCity, "ca"), false}, + + // country correction: these fail against "keep whichever is finer" + {"country probe vs mmdb city, other country", probedCountry("jp"), mmdb(model.LocationTypeCity, "ca"), true}, + {"country probe vs mmdb region, other country", probedCountry("jp"), mmdb(model.LocationTypeRegion, "ca"), true}, + + // nothing to lose + {"country probe vs mmdb country, same", probedCountry("ca"), mmdb(model.LocationTypeCountry, "ca"), true}, + {"country probe vs mmdb country, other", probedCountry("jp"), mmdb(model.LocationTypeCountry, "ca"), true}, + {"country probe vs no mmdb answer", probedCountry("jp"), nil, true}, + + // a city-confident probe is never a downgrade + {"city probe vs mmdb city, same country", probedCity("ca"), mmdb(model.LocationTypeCity, "ca"), true}, + {"city probe vs mmdb city, other country", probedCity("jp"), mmdb(model.LocationTypeCity, "ca"), true}, + {"city probe vs mmdb region", probedCity("ca"), mmdb(model.LocationTypeRegion, "ca"), true}, + {"city probe vs no mmdb answer", probedCity("jp"), nil, true}, + } + for _, test := range tests { + if got := probedLocationPreferred(test.egress, test.mmdb); got != test.want { + t.Errorf("%s: probedLocationPreferred = %v, want %v", test.name, got, test.want) + } + } +} diff --git a/controller/provider_egress_location_controller.go b/controller/provider_egress_location_controller.go index c8c64832..8cac0dcc 100644 --- a/controller/provider_egress_location_controller.go +++ b/controller/provider_egress_location_controller.go @@ -141,8 +141,10 @@ func SubmitProviderEgressLocation( // survive a code revert and there is no cleanup path. // // model.MatchExistingLocation therefore matches only, never creates, - // case-insensitively and ignoring punctuation/whitespace so the ordinary - // variants land on the row that is already there. When it does not resolve, + // case-insensitively and ignoring punctuation/whitespace/accents and + // parenthesised district qualifiers, so the ordinary variants -- the + // "(Innenstadt I)" case above included -- land on the row that is already + // there. When it does not resolve, // this submission falls back to country granularity: country is the // granularity this design treats as trustworthy anyway, and losing city // precision for one probe is strictly better than permanently polluting a diff --git a/controller/provider_egress_location_controller_test.go b/controller/provider_egress_location_controller_test.go index 31c7d1cb..45b28d22 100644 --- a/controller/provider_egress_location_controller_test.go +++ b/controller/provider_egress_location_controller_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "fmt" "strings" "testing" "time" @@ -494,10 +495,17 @@ func TestSubmitProviderEgressLocationUnknownCityDoesNotCreateALocation(t *testin }) } -// The variants that matter are trivial: the same city spelled with different -// case, punctuation or spacing. Those must resolve to the row that is already -// there -- discarding them to country would throw away real precision for no -// reason, and creating a row for each is the bug this guards against. +// The variants that matter are the ones the geolocation sources actually +// produce for one place: different case, punctuation or spacing, and -- the +// case this feature was built for -- a parenthesised district qualifier +// ("Frankfurt am Main (Innenstadt I)"). Those must resolve to the row that is +// already there; discarding them to country throws away real precision, and +// creating a row for each is the bug this guards against. +// +// An earlier revision of this list only contained foldings the implementation +// already handled, so it could not fail. The qualifier case below fails against +// a matcher that only drops punctuation, which is what makes this test worth +// running. Diacritics -- the other class that failed -- have their own test. func TestSubmitProviderEgressLocationMatchesCitySpellingVariant(t *testing.T) { server.DefaultTestEnv().Run(t, func(t testing.TB) { ctx := context.Background() @@ -524,8 +532,17 @@ func TestSubmitProviderEgressLocationMatchesCitySpellingVariant(t *testing.T) { {"hesse", "FRANKFURT AM MAIN"}, // case, both levels {"Hesse", "Frankfurt-am-Main"}, // punctuation {"Hesse", " Frankfurt am Main "}, + // the observed disagreement that motivated matching at all: one + // source appends the district. Dropping "(" and ")" as punctuation + // is not enough -- the qualifier's letters stay in the key -- so + // this misses unless the qualifier itself is stripped. + {"Hesse", "Frankfurt am Main (Innenstadt I)"}, + {"Hesse", "Frankfurt am Main (Innenstadt I) "}, + // qualifier on the region too + {"Hesse (Regierungsbezirk Darmstadt)", "Frankfurt am Main"}, } for _, variant := range variants { + label := fmt.Sprintf("%q/%q", variant.region, variant.city) clientId := server.NewId() model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") @@ -543,13 +560,13 @@ func TestSubmitProviderEgressLocationMatchesCitySpellingVariant(t *testing.T) { stored := model.GetProviderEgressLocation(ctx, clientId) if stored == nil { - t.Fatalf("%q: expected the submission to be stored", variant.city) + t.Fatalf("%s: expected the submission to be stored", label) } if stored.LocationId != frankfurt.LocationId { - t.Errorf("%q resolved to %s, want the existing Frankfurt row %s", variant.city, stored.LocationId, frankfurt.LocationId) + t.Errorf("%s resolved to %s, want the existing Frankfurt row %s", label, stored.LocationId, frankfurt.LocationId) } if !stored.CityConfident { - t.Errorf("%q: city_confident must stay set when the city resolved", variant.city) + t.Errorf("%s: city_confident must stay set when the city resolved", label) } } @@ -558,3 +575,170 @@ func TestSubmitProviderEgressLocationMatchesCitySpellingVariant(t *testing.T) { } }) } + +// Diacritics are the largest single class of spelling disagreement between the +// geolocation sources: one emits the local spelling ("São Paulo", "Zürich", +// "Kraków"), another an ASCII transliteration ("Sao Paulo", "Zurich", +// "Krakow"), and the mmdb import that seeded the existing rows picked one of +// the two per city with no way to know which. Every one of these missed before +// the NFD fold and fell back to country -- and, per +// TestSetConnectionLocationProbedCountryDoesNotCoarsenMmdbCity, a fallback on a +// provider the mmdb already placed in a city used to be an outright +// discoverability regression. +// +// Both directions are exercised, because which side carries the accent depends +// on how the existing row happened to be seeded. +func TestSubmitProviderEgressLocationMatchesAccentedCityVariant(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + networkId := server.NewId() + + // existing rows, seeded the way the mmdb import would have: some + // accented, some already transliterated + saoPaulo := &model.Location{ + LocationType: model.LocationTypeCity, + City: "São Paulo", + Region: "São Paulo", + Country: "Brazil", + CountryCode: "br", + } + model.CreateLocation(ctx, saoPaulo) + + zurich := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Zürich", + Region: "Zürich", + Country: "Switzerland", + CountryCode: "ch", + } + model.CreateLocation(ctx, zurich) + + // seeded WITHOUT the accent, so the probe is the side carrying it + krakow := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Krakow", + Region: "Lesser Poland", + Country: "Poland", + CountryCode: "pl", + } + model.CreateLocation(ctx, krakow) + + before := testing_countLocations(ctx) + + variants := []struct { + countryCode string + country string + region string + city string + want *model.Location + }{ + // accent dropped by the probing source + {"BR", "Brazil", "Sao Paulo", "Sao Paulo", saoPaulo}, + {"CH", "Switzerland", "Zurich", "Zurich", zurich}, + // accent present in the probe, absent from the stored row + {"PL", "Poland", "Lesser Poland", "Kraków", krakow}, + // accent on one level only + {"BR", "Brazil", "São Paulo", "Sao Paulo", saoPaulo}, + // accent plus the other foldings at once + {"CH", "Switzerland", "zurich", " ZURICH ", zurich}, + } + for _, variant := range variants { + label := fmt.Sprintf("%q/%q", variant.region, variant.city) + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: variant.countryCode, + Country: variant.country, + Region: variant.region, + City: variant.city, + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + connect.AssertEqual(t, err, nil) + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatalf("%s: expected the submission to be stored", label) + } + if stored.LocationId != variant.want.LocationId { + t.Errorf("%s resolved to %s, want the existing row %s", label, stored.LocationId, variant.want.LocationId) + } + if !stored.CityConfident { + t.Errorf("%s: city_confident must stay set when the city resolved", label) + } + } + + if after := testing_countLocations(ctx); after != before { + t.Errorf("location row count went from %d to %d; accented variants must reuse the existing row, not add new ones", before, after) + } + }) +} + +// The qualifier-stripping pass is the only one that can pick the wrong row, so +// it must decline rather than guess. Two same-region rows that differ ONLY in +// their parenthesised qualifier both reduce to the same key; a probe carrying a +// third qualifier matches neither exactly and must fall back to country instead +// of silently landing on whichever row the candidate ordering put first. +func TestSubmitProviderEgressLocationAmbiguousQualifierFallsBackToCountry(t *testing.T) { + server.DefaultTestEnv().Run(t, func(t testing.TB) { + ctx := context.Background() + + springfieldIl := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Springfield (IL)", + Region: "Midwest", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, springfieldIl) + + springfieldMa := &model.Location{ + LocationType: model.LocationTypeCity, + City: "Springfield (MA)", + Region: "Midwest", + Country: "United States", + CountryCode: "us", + } + model.CreateLocation(ctx, springfieldMa) + + before := testing_countLocations(ctx) + + networkId := server.NewId() + clientId := server.NewId() + model.Testing_CreateDevice(ctx, networkId, server.NewId(), clientId, "", "") + + _, err := SubmitProviderEgressLocation(ctx, &SubmitProviderEgressLocationArgs{ + ClientId: clientId, + CountryCode: "US", + Country: "United States", + Region: "Midwest", + City: "Springfield (OH)", + CountryConfident: true, + CityConfident: true, + ObservedAt: server.NowUtc(), + }) + connect.AssertEqual(t, err, nil) + + stored := model.GetProviderEgressLocation(ctx, clientId) + if stored == nil { + t.Fatal("expected the submission to be stored") + } + if stored.CityConfident { + t.Errorf("an ambiguous qualifier must not resolve to a city; got city_confident with location %s", stored.LocationId) + } + if stored.LocationId == springfieldIl.LocationId || stored.LocationId == springfieldMa.LocationId { + t.Errorf("guessed a Springfield (%s) instead of falling back to country", stored.LocationId) + } + // the country fallback goes through CreateLocation, but the US country + // row already exists (the two Springfield fixtures created it), and + // CreateLocation dedupes a country on country_code -- so declining to + // guess must add no rows at all, least of all a third Springfield + if after := testing_countLocations(ctx); after != before { + t.Errorf("location row count went from %d to %d; the fallback must reuse the existing country row and never create a city", before, after) + } + }) +} diff --git a/go.mod b/go.mod index e4bddacf..284749b2 100644 --- a/go.mod +++ b/go.mod @@ -41,6 +41,7 @@ require ( golang.org/x/crypto v0.53.0 golang.org/x/net v0.56.0 golang.org/x/sys v0.46.0 + golang.org/x/text v0.40.0 golang.zx2c4.com/wireguard/wgctrl v0.0.0-20241231184526-a9ab2273dd10 google.golang.org/protobuf v1.36.11 gopkg.in/yaml.v3 v3.0.1 @@ -135,7 +136,6 @@ require ( golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.22.0 // indirect golang.org/x/term v0.44.0 // indirect - golang.org/x/text v0.40.0 // indirect golang.org/x/time v0.15.0 // indirect gopkg.in/ini.v1 v1.67.2 // indirect src.agwa.name/tlshacks v0.0.3 // indirect diff --git a/model/provider_egress_location_model.go b/model/provider_egress_location_model.go index 88ee8603..c0ad7780 100644 --- a/model/provider_egress_location_model.go +++ b/model/provider_egress_location_model.go @@ -6,6 +6,8 @@ import ( "time" "unicode" + "golang.org/x/text/unicode/norm" + "github.com/urnetwork/server" ) @@ -367,28 +369,52 @@ func GetLocation(ctx context.Context, locationId server.Id) *Location { } // normalizeLocationName folds a location name to a comparison key: lowercased, -// with every rune that is not a letter or a digit dropped. So +// accent-stripped, with every rune that is not a letter or a digit dropped. So // "Frankfurt am Main", "Frankfurt Am Main" and "FRANKFURT AM MAIN" all fold to -// "frankfurtammain" and match the one row that already exists. +// "frankfurtammain", and "São Paulo", "Zürich" and "Kraków" fold to the same +// keys as "Sao Paulo", "Zurich" and "Krakow". // // This is deliberately a comparison key only -- it is never stored, and never // used to build a location_name. It exists so a trivial spelling variant from a // geolocation source resolves to the existing row instead of being treated as a // different place. // +// Diacritics are the single biggest source of these variants: the free +// geolocation sources disagree over whether to emit the local spelling or an +// ASCII transliteration for the same city, and the mmdb import that seeded most +// existing rows made its own choice. Folding is done with an NFD decomposition +// followed by dropping the combining marks (unicode.Mn), which covers the whole +// accent class at once -- a hand-rolled é->e table would have to enumerate the +// world's diacritics and would silently keep missing the ones it forgot. +// +// golang.org/x/text is already a dependency of this module, so this costs no +// new supply-chain surface. (An earlier revision of this function was +// stdlib-only by mistake: that constraint belongs to the prober repo's +// `geolocate` package, not to the server.) +// +// Letters that carry a stroke rather than a combining mark -- ł, ø, đ -- do not +// decompose and therefore still do not fold onto l/o/d. Those fall back to +// country granularity, which is the safe outcome; the alternative is the +// transliteration table this function deliberately avoids. +// // Punctuation is dropped rather than mapped to a space because the disagreement // is over whether the separator exists at all ("Washington, D.C." vs // "Washington DC"). Note this deliberately does not fold "Frankfurt/Main" onto // "Frankfurt am Main": dropping the separator gives "frankfurtmain" != // "frankfurtammain", so that one falls back to country granularity rather than // matching the wrong row. Falling back is the safe outcome; guessing is not. -// Stdlib only, by design -- a transliteration/fuzzy-match dependency is a large -// amount of new behaviour to take on for an ingest path whose failure mode is -// already "use the country". func normalizeLocationName(name string) string { + // NFD splits a precomposed letter ("ü") into its base letter plus a + // combining mark ("u" + U+0308). The mark is category Mn, which is neither + // a letter nor a digit, so the filter below drops it; the explicit Mn skip + // is there to say so rather than to leave it to a category coincidence. + decomposed := norm.NFD.String(strings.ToLower(name)) var b strings.Builder - b.Grow(len(name)) - for _, r := range strings.ToLower(name) { + b.Grow(len(decomposed)) + for _, r := range decomposed { + if unicode.Is(unicode.Mn, r) { + continue + } if unicode.IsLetter(r) || unicode.IsDigit(r) { b.WriteRune(r) } @@ -396,11 +422,59 @@ func normalizeLocationName(name string) string { return b.String() } -// matchLocationNameInTx returns the location_id of the row in `candidates` -// whose location_name matches `name`, preferring an exact match and falling -// back to a normalized one (see normalizeLocationName), or nil for no match. -// Candidates must already be ordered deterministically by the caller so that -// two rows folding to the same key always resolve the same way. +// stripParentheticals removes every parenthesised span from name, so +// "Frankfurt am Main (Innenstadt I)" becomes "Frankfurt am Main ". Nesting is +// tracked, and an unclosed "(" swallows the rest of the string -- a qualifier +// that was truncated by a length limit is still a qualifier. +// +// This is only ever applied to a comparison key, never to anything stored. +func stripParentheticals(name string) string { + if !strings.ContainsRune(name, '(') { + return name + } + var b strings.Builder + b.Grow(len(name)) + depth := 0 + for _, r := range name { + switch r { + case '(': + depth += 1 + case ')': + if 0 < depth { + depth -= 1 + } + default: + if depth == 0 { + b.WriteRune(r) + } + } + } + return b.String() +} + +// matchLocationName returns the location_id of the row in `candidates` whose +// location_name matches `name`, or nil for no match. Candidates must already be +// ordered deterministically by the caller so that two rows folding to the same +// key always resolve the same way. +// +// Three passes, narrowest first: +// +// 1. exact string equality -- the common case, since the winning source usually +// spells it the way the mmdb import did; +// 2. the normalized fold (see normalizeLocationName): case, punctuation, +// whitespace and accents; +// 3. the normalized fold with parenthesised qualifiers stripped from BOTH +// sides. This is the case that motivated the whole feature: one source +// reports "Frankfurt am Main (Innenstadt I)" -- a district qualifier -- for +// a host another source calls "Frankfurt am Main". Pass 2 cannot see +// through that, because dropping the parentheses as punctuation leaves the +// qualifier's letters in the key. +// +// Pass 3 is the only pass that can plausibly match the wrong row -- two +// same-region rows "Springfield (IL)" and "Springfield (MA)" both reduce to +// "springfield" -- so it requires the stripped key to identify exactly ONE +// candidate and returns nil on any ambiguity. Falling back to country +// granularity is the safe outcome; guessing is not. func matchLocationName(name string, candidateIds []server.Id, candidateNames []string) *server.Id { for i, candidateName := range candidateNames { if candidateName == name { @@ -418,7 +492,22 @@ func matchLocationName(name string, candidateIds []server.Id, candidateNames []s return &candidateIds[i] } } - return nil + + base := normalizeLocationName(stripParentheticals(name)) + if base == "" { + // the name was nothing but a qualifier + return nil + } + var unique *server.Id + for i, candidateName := range candidateNames { + if normalizeLocationName(stripParentheticals(candidateName)) == base { + if unique != nil { + return nil + } + unique = &candidateIds[i] + } + } + return unique } // MatchExistingLocation resolves (countryCode, region, city) against location @@ -436,9 +525,11 @@ func matchLocationName(name string, candidateIds []server.Id, candidateNames []s // source's original display string. Each variant would become its own row, // those rows outlive a code revert, and there is no cleanup path. // -// Matching is case-insensitive and ignores punctuation and whitespace -// differences, so the ordinary variants resolve to the row that is already -// there. When nothing resolves the caller falls back to country granularity -- +// Matching is case-insensitive and ignores punctuation, whitespace, accents and +// parenthesised district qualifiers (see matchLocationName), so the ordinary +// variants resolve to the row that is already there -- including the +// "Frankfurt am Main (Innenstadt I)" case above, which is what this exists for. +// When nothing resolves the caller falls back to country granularity -- // see SubmitProviderEgressLocation. Falling back loses precision for one // submission; creating a row corrupts shared data permanently. //