fix(model): count only providers holding a Public provide key - #408
Closed
Ryanmello07 wants to merge 5 commits into
Closed
fix(model): count only providers holding a Public provide key#408Ryanmello07 wants to merge 5 commits into
Ryanmello07 wants to merge 5 commits into
Conversation
A provider without a provide_mode=3 key cannot accept a contract from a client outside its own network, so counting it advertises supply nobody can reach. On beta this reported 39 US providers when 2 were reachable. Filters both UpdateClientLocations and UpdateClientScores: GetProviderLocations gates on loadLocationStables as well, so fixing only the first leaves the symptom in place. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…e score filter
Review follow-ups to "count only providers holding a Public provide key".
1. The location *group* source query in UpdateClientScores was still bare
`connected = true AND valid = true`. It fills locationGroupClientScores ->
the clientScoreLocationGroup* redis keys -> loadClientScores ->
FindProviders2 whenever a spec carries a LocationGroupId, so a user
selecting a promoted group (e.g. "Strong Privacy Laws") still received
providers that cannot accept their contract, and CreateContract rejected
them with NoPermission. Same EXISTS predicate as the other two queries.
2. The UpdateClientScores filter had no test of its own -- every existing
test that reaches it gives all its clients a Public key, so deleting the
clause broke nothing. Two new tests set up connected+valid providers that
are identical except for the Public provide key:
- TestUpdateClientScoresCountsOnlyPublicProviders covers the per-location
query through loadLocationStables (the gate GetProviderLocations uses)
and loadClientScores.
- TestUpdateClientScoresGroupCountsOnlyPublicProviders covers the group
query through the group cache.
Both were verified to fail with their respective EXISTS clause removed.
Because UpdateClientScores inner joins client_connection_reliability_score
here, each provider also gets a reliability score row
(AddClientReliabilityStats + UpdateClientReliabilityScores at
server.NowUtc(), not NowUtc().Add(time.Hour), which produces no
lookback_index = 0 row), and the fixture levels both clients'
independent_reliability_weight so a single sample's ~0.016 weight does not
fail the strict minimums for both and make the stability assertion pass
for the wrong reason.
3. The SQL comments claimed the destination "must hold a key for exactly
that mode". That is not an invariant the code holds:
resolveNonCompanionProvideMode lets a Stream-only destination settle a
cross-network contract as a companion stream. Reworded to state the
intent -- a companion-only destination is a return path, not general
provider supply, so excluding it from provider counts is deliberate.
No SQL change.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…ders discoverable to their own network
Two production risks in this change, found by a backward-compatibility
audit of the PR.
1. Country-only clients were counted three times.
A client whose geo lookup resolved neither a city nor a region is stored
with city_location_id = region_location_id = country_location_id --
SetConnectionLocation writes the coarsest available id into the NOT NULL
city/region columns. Both fan-out loops then walked city, region and
country unconditionally, so one such client added 3 to its own country's
provider count and was inserted three times into its country's scoring
map. The inflation is worst exactly where geo resolution is coarsest:
datacenter, mobile and VPN egress.
Both loops now go through the distinct set of location ids, so a client
counts once per location it is actually in. A genuinely city-granular
client still rolls up into its region and its country unchanged -- that
half is asserted explicitly, because a dedupe keyed on the client rather
than on (client, location) would silently stop city clients counting
toward their country, a worse regression than the one being fixed.
(The per-location and per-group scoring maps are keyed by client id, so
they already absorbed the repeat; the count was the live defect. The
loops are made explicit anyway so the two stay in step.)
2. Network-only providers vanished from their own network's discovery.
The Public-only filter is right for the public provider count -- a
stranger genuinely cannot use a ProvideModeNetwork provider -- but wrong
for the candidate pool. Such a provider serves same-network sources today
via the working CreateContractNoEscrow path and is discoverable to them
now; filtering the pool to Public made it undiscoverable to exactly the
users it exists for. That is a live regression for anyone running
providers for their own organisation.
The pool must also not hand a cross-network caller a provider whose
contract will be refused, so this is not a revert:
- UpdateClientLocations keeps the Public-only filter. That number is
shown to everyone and should reflect what a stranger can reach.
- The UpdateClientScores source queries now admit Public or Network --
exactly the two modes GetProvideRelationship can return, hence
exactly what CreateContract can accept -- and record NetworkOnly on
each ClientScore.
- FindProviders2 filters at request time: a candidate is eligible if it
is publicly usable, or if its NetworkId equals the requesting
session's network. This cannot be baked into the cache: the client
score redis entries are keyed by (forceMinimum, rankMode, locationId,
callerLocationId) with no network component, so one cached set is
shared by callers from every network.
NetworkOnly is stored negated deliberately. The score cache is gob
encoded with a 5h ttl, so entries written before the field existed decode
with the zero value; the zero value therefore has to mean "publicly
usable", or every provider would be treated as network-only until the
cache turned over.
The ClientFilter exported alongside the samples still counts only
publicly usable providers. It is read solely by loadLocationStables,
which decides the public Stable flag GetProviderLocations publishes -- a
public surface, so it follows the same rule as the provider count. A
location whose only supply is network-only is not stable and reports no
providers.
No Stream carve-out: resolveNonCompanionProvideMode does return
companion=true for a Stream-only destination, but that dead-ends at
CreateCompanionTransferEscrow, which requires a pre-existing
reverse-direction origin contract, so a Stream-only destination can never
bootstrap a cross-network session. The inline comments that implied
Stream was the only excluded case are corrected.
Tests: a country-only provider counted exactly once and a city provider
still counted at all three granularities, for both UpdateClientLocations
and UpdateClientScores; and all four visibility cases in FindProviders2
(Public and Network-only providers x same-network and other-network
callers). The two existing pool assertions that encoded the Public-only
pool are updated to the new two-tier contract; their location-stability
assertions are unchanged and still pass.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
`/network/provider-locations` (UpdateClientLocations) counts only providers holding a Public provide key: GetProvideRelationship returns ProvideModePublic for a cross-network pair, so without a Public key a provider can only ever serve requesters inside its own network. As the owner put it -- "for provider list only do public. network ones can only be used by their own network so they are basically private." `/stats/providers-map` (GetProvidersMap) and CountProviderCountries applied no provide-mode filter at all. Once the provide-mode change ships, those two public numbers report supply that is not in the list users can pick from -- the map's per-region counts and the country count both exceed /network/provider-locations, in the same deployment, for the same population. The country count is worse than an inflated number: a country whose only providers are network-only or hold no provide key at all counts as a covered country while offering a stranger nothing. Both now apply the same `EXISTS (SELECT 1 FROM provide_key WHERE client_id = ... AND provide_mode = $N)` predicate UpdateClientLocations uses, mirrored exactly rather than varied, so every public provider number answers one question: what a stranger can actually reach. TestProviderStatsCountOnlyPublicProviders covers both surfaces in one run (Errorf, not Fatalf, on the first so the second is still exercised): a Public provider in au counts, a Network-only and a keyless provider in jp do not, so the map has no jp entry and the country count is 1 not 2. TestGetProvidersMapAggregatesRegions' helper now gives its providers a Public key, since every provider it builds is meant to count. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg (cherry picked from commit d06fc68)
…test that could not fail
Two review findings on the provide-mode filtering work.
1. Deleting either provide-mode predicate left CI green.
Both UpdateClientScores source queries carry
`EXISTS (... provide_mode IN (Public, Network))`, and nothing failed when
either one was removed. The existing provide-mode tests build Public and
Network-only providers, both of which the predicate admits, so a
deletion changes nothing they look at.
What a deletion actually does is refill the candidate pool with the
entire consumer fleet: every client registers ProvideMode_Stream on
connect (connect/transfer_contract_manager.go), and a Stream-only client
can never settle a contract -- resolveNonCompanionProvideMode can
resolve it as a companion, but that dead-ends at
CreateCompanionTransferEscrow, which needs a pre-existing reverse origin
contract. That is precisely the 39-advertised-against-2-usable failure
this work exists to fix.
Two tests now build the populations that would flood back in -- a
Stream-only provider and a keyless one, alongside a Public and a
Network-only provider in the same location -- and assert the pool holds
exactly the two that can serve a contract, each tagged correctly. One
covers the per-location query, one the location-group query, so a
deletion in either statement produces its own signal:
per-location predicate deleted -> per-location test fails
("candidate pool holds 4 clients,
want exactly the 2 that can settle
a contract"), group test passes
group predicate deleted -> the reverse
2. TestUpdateClientScoresCountsEachClientOncePerLocation was vacuous.
Its per-location accumulator is a map keyed by client id, so a repeated
client is absorbed whether or not the loop dedupes: it returned 1 before
the fix and 1 after, and could not fail.
Deleted, and replaced with
TestUpdateClientScoresRollsCityProviderUpToRegionAndCountry, which
asserts the property the code CAN violate -- that a city-granular
provider reaches its region's and its country's pools, and that a
provider does not leak into another country's. Membership is asserted by
client id rather than by count, so it also catches leakage. Keying the
dedupe on the client instead of on (client, location) -- the tempting
simplification, since the map already absorbs repeats -- empties the
region and country pools and fails it.
Also corrects the scope claimed for the 3x dedupe. The commit message
for that change describes the triple-count as a live defect; that is
true on beta, which has SetConnectionLocation's coarsest-granularity
fallback, but not against upstream/main, which has no country-only
fallback and raises on the NULL city instead, so no row with
city = region = country is written there today. The fallback arrives
upstream with PR urnetwork#407, which now carries its own copy of the dedupe.
distinctIds, the counting loop and the fan-out test all say so.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Ryanmello07
added a commit
to Ryanmello07/server
that referenced
this pull request
Jul 27, 2026
…finement Brings in the review fixes for PR urnetwork#408: tests that fail when either UpdateClientScores provide-mode predicate is deleted, and the replacement for the vacuous per-location dedupe test. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg # Conflicts: # model/network_client_location_model_test.go
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A provider without a
provide_mode=3key cannot accept a contract from a client outside its own network, so counting it advertises unreachable supply. Measured on a beta deployment: 39 US providers advertised, 2 reachable.Filters both
UpdateClientLocationsandUpdateClientScores, sinceGetProviderLocationsgates onloadLocationStables(populated byUpdateClientScores) as well — filtering only the first leaves the symptom in place.New test
TestUpdateClientLocationsCountsOnlyPublicProviders: three connected+valid clients in the same country, only one holding a Public key. Fails withClientCount = 3before the change, passes with 1 after.🤖 Generated with Claude Code
https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg