feat(schema): key provider_bandwidth on (client_id, source) for two active targets - #418
Open
Ryanmello07 wants to merge 24 commits into
Open
Conversation
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.
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.
…ion 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).
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.
… 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.
…n 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
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.
…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.
… 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…t 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…, 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg (cherry picked from commit 682b29b)
…voids 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
(cherry picked from commit 37ca6cd)
…a probed location
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
transfer_escrow already records real bytes delivered per contract as a byproduct of billing. Deriving throughput from it costs nothing additional and cannot be gamed selectively -- a provider cannot inflate real user traffic without actually being fast for real users. Excludes companion (return-traffic) contracts: a client's return leg settles with the client as destination, which would otherwise misread ordinary users as fast providers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Appends three migrations and the storage path for a measured bandwidth
figure.
- provider_bandwidth: one row per provider, keyed on client_id, tagged with
the source that produced it (passive aggregation of settled bytes, or an
active sample). A new measurement overwrites the old one -- this is the
current figure a consumer reads, not a history, and keeping every sample
would grow without bound for a value only ever read as "the current one".
Indexed on window_end for staleness scans; single-provider lookups go
through the primary key.
- provider_egress_location gains verdict / verdict_reason / assurance. All
three are additive with safe defaults ('unverified' / '' / 'direct'), so
existing rows read as an unjudged direct probe and every existing reader
of the table is unaffected. Nothing writes a non-default verdict yet.
- StoreProviderBandwidth upserts through the client_id primary key, so both
sources write one figure per provider.
The migrations are appended at the tail of the slice and never inserted:
migrations apply by slice index (`for i := DbVersion(ctx); i < upTo; i++`),
so editing or reordering an applied entry corrupts a live database. Every
statement is IF NOT EXISTS, making a re-run -- or a duplicated merge
resolution -- a no-op.
SetProviderEgressLocation names every column explicitly, which bypasses the
new columns' DEFAULTs, so it normalizes an empty Verdict to 'unverified' and
an empty Assurance to 'direct' the same way it already lowercases the country
code. Without that, the additive-with-safe-defaults claim would be false for
every row the current ingest path writes: it computes no judgement, and Go's
zero value '' is a legal varchar. Both readers are updated -- the plain
getter and GetFreshProviderEgressLocationForConnection, which is the
connect-announce hot path -- so neither returns a zero-valued verdict that
looks like a real one.
Tests: the table and the three columns exist (both failed first against the
un-migrated schema); StoreProviderBandwidth round-trips and a second
measurement replaces the first in place; the write path stores the defaults
for an unjudged submission and an explicit judgement verbatim.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Active bandwidth probing pulls real data through a provider's tunnel, which is a real paid contract: a zero-cost balance code does not make it free, because the payout planner sums paid and unpaid traffic identically before computing payouts. Probe bytes therefore need a spend limit unconditionally, on every deployment, not only where payouts happen to be live. The budget is split into fixed, non-overlapping UTC hourly buckets rather than a rolling trailing window. A reservation goes to the earliest bucket with room, starting from the current hour; if the current hour is full the probe is deferred to a later one rather than rejected, up to a 24-hour lookahead, after which the deployment's daily budget is genuinely exhausted and the probe is refused. Fixed buckets are what make "defer to the next hour" well-defined -- there needs to be a discrete boundary to wait for. A caller that reserves budget it cannot ultimately use (the provider went offline between reserving and dialing) releases it explicitly via CancelProviderBandwidthReservation, and a reaper drops ledger rows whose bucket is safely in the past. The reservation is a plain read-then-insert in one transaction, with no row locking. SELECT ... FOR UPDATE would not help: the contended quantity is a bucket's aggregate, and an empty bucket has no row to lock -- serializing properly would need a materialized per-bucket row or an advisory lock, which this ledger shape deliberately doesn't have. Two concurrent reservations can overshoot the ceiling by at most their combined byte counts, which is acceptable for a coarse deployment-wide spend cap fed by a small number of prober processes. MaxActiveBandwidthProbesPerBucket = 40 is derived from the population this probes, not picked arbitrarily. Active sampling only ever runs against providers with no passive bandwidth history -- the trickle of newly-joined providers before their first settled contract on a mature deployment, and effectively the whole fleet on a new one. 40/hour x 5 MB per probe = 200 MB/hour, 4.8 GB/day worst case. One value chosen to behave sensibly at both scales rather than a per-environment knob; tuned, not structural, and worth revisiting against real production data. The two migrations are appended at the tail (535 -> 537 entries, zero deletions in the diff) and every statement is IF NOT EXISTS, since migrations apply by slice index and reordering an applied entry corrupts live databases.
Three operator-secret-gated routes the active bandwidth probe needs: GET /network/provider-bandwidth-test?bytes=N POST /network/provider-bandwidth-result POST /network/provider-bandwidth-reserve The download endpoint is the target the prober pulls through a provider's tunnel to measure its throughput. It streams min(N, 5MB) from a small repeating block through an io.LimitReader, so the byte count is never allocated, and the clamp keeps a single request from being an open-ended resource commitment. A `bytes` that is absent, malformed, or non-positive falls back to 1MiB: `bytes=0` parses cleanly, and an empty body would hand the prober a zero-byte sample to divide by. The result endpoint stores the measurement with Source "active" and window_start == window_end (an active probe is a point measurement, not an aggregate over a window). A non-positive rate or sample size is rejected before anything is written -- a zero measurement is not usable and must never overwrite a real figure. The reserve endpoint wraps ReserveProviderBandwidthSlot. That function is built for a caller that can set a RunAt, so when the current hour is full it succeeds with a reservation in a later bucket; the prober measures over a tunnel it already has open, right now, so later-hour budget is useless to it and a 200 would let it spend the whole daily ceiling inside one hour. A deferred reservation is therefore cancelled again and the request answered 429 with Retry-After, which the prober treats as "skip this provider". All three reuse the existing operator auth unchanged: same X-UR-Operator-Secret header, same constant-time hmac.Equal compare, same fail-closed memoized vault read as the egress location ingest.
…argets The active probe measures two independent targets per provider -- the operator's own download endpoint and a public CDN -- and the two figures are the point. A provider that prioritises one path and not the other is invisible in a single number and obvious in a pair. Keyed on client_id alone the two overwrite each other on every pass, so only one target could be stored at all; averaging them into the one row would lose the same signal more quietly. The source becomes part of the key rather than each target getting its own columns, because a further target then needs no migration: passive, active-operator and active-cdn are three rows in one shape, and a fourth would be a fourth row. Per-target columns would need a migration each. Migration is a strict tail append and is idempotent (DROP CONSTRAINT IF EXISTS paired with the ADD). No backfill: provider_bandwidth is empty on every deployment -- the active prober is its first writer and ships with this change -- so re-keying cannot orphan or collide with an existing row. The result endpoint now carries the source and VALIDATES it against the known active set instead of hardcoding one tag. Two failures are closed off. An unrecognised source is not a harmless label: it writes a row under a tag nothing will ever read or replace, while the submitter goes on looking like it works. And "passive" is refused specifically -- that figure is derived server-side from bytes the provider has already been paid to carry, which is exactly what makes it ungameable, so accepting a submitted one would let this endpoint overwrite a derived figure with an asserted one, through a secret that now travels over a provider-controlled path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
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.
P2 Task 6, server half, widened to two bandwidth targets.
Branched off
feat/provider-bandwidth-two-targets-upstream's parentfeat/provider-bandwidth-endpoint-upstream(#417), so the diff carries P0+P1+T1+T2+T4+T5 — only the tip commit is new. Merge #407 → #416 → #417 first.Why
The active bandwidth probe measures two independent targets per provider — the operator's own
/network/provider-bandwidth-testandhttps://speed.cloudflare.com/__down— and the two figures are the point. A provider that prioritises the operator's own path while starving the public internet is invisible in one combined number and obvious in a pair.provider_bandwidth's primary key wasclient_idalone, so the two measurements overwrote each other on every pass and only one target could ever be stored. Averaging them into the one row would lose the same signal more quietly.Changes
Migration (appended, idempotent):
Strict tail append (
git diff --statondb_migrations.go: 34 insertions, zero deletions — exactly one new entry at the tail);DROP … IF EXISTSpaired with theADDmakes a re-run a no-op. No backfill —provider_bandwidthis empty on every deployment (the active prober is its first writer and ships with this change), so re-keying cannot orphan or collide with an existing row. Same shape as the existingclient_connection_reliability_scorere-key precedent in this file.Source constants split:
active→active-operator/active-cdn, pluspassive. A third target later needs a constant and nothing else — no migration, no new column. That generality is why the source is a key column rather than per-target columns, which would need a migration per target.StoreProviderBandwidthupserts on(client_id, source).POST /network/provider-bandwidth-resultcarriessourceand validates it against the known active set.Why
passiveis rejected by the endpoint, not just unknown stringsThe submittable set is deliberately the active subset.
passiveis derived server-side from bytes the provider has already been paid to carry, which is exactly what makes it ungameable; accepting a submittedpassiverow would let this endpoint overwrite that derived figure with an asserted one — through a secret that now travels over a provider-controlled path (the operator download target is reached through the provider tunnel).An unrecognised source is not a harmless label either: keyed on
(client_id, source), it writes a row nothing will ever read or replace while the submitter goes on looking like it works.Budget note
Two targets consume two reservations per provider, so
MaxActiveBandwidthProbesPerBucket = 40now admits 20 providers/hour rather than 40. Deliberately not retuned here — it is a tuned value to revisit with real data. A full fleet sweep therefore takes twice as many hours, which is the intended trade: the hourly bucket is what keeps fleet-wide measurement from becoming one expensive pass.Tests
go build ./...,go vet ./...clean on this branch. Against the local stack:go test ./model -run 'TestStoreProviderBandwidth|TestProviderBandwidthTableExists|TestComputePassiveProviderBandwidth|TestReserveProviderBandwidthSlot'— 8/8 passgo test ./api/handlers -run TestProviderBandwidth— 19/19 passgo test . -run TestApplyDbMigrations— passNew:
TestStoreProviderBandwidthKeepsEverySourceSeparate,TestProviderBandwidthResultStoresTheTwoTargetsSeparately,TestProviderBandwidthResultRejectsUnknownSource(table-driven over absent / empty / the pre-splitactivetag / a typo / wrong case /passive).Teeth-checked, not assumed:
client_idalone →--- FAIL: TestProviderBandwidthResultStoresTheTwoTargetsSeparately--- FAIL: TestProviderBandwidthResultRejectsUnknownSource