probe system checkpoint - #2
Merged
Merged
Conversation
consensus() picked country and city winners with (n == bestN && c < best), which meant an exact vote tie was decided by lexicographic order of the country/city string rather than by which sources actually agreed. With 2 sources voting US and 2 voting CA, the smaller string "ca" would win and be reported CountryConfident=true — a coin flip presented as agreement. Add SourcePriority (ip.pn > freeipapi > ipinfo, lower rank = more trusted) and sourceRank(), and use them as the primary tie-break for both country and city winner selection: among candidates with equal vote counts, the one backed by the most-trusted contributing source wins. Lexicographic order is now only a last-resort tiebreaker when ranks also match. Vote-count comparison and the >= MinSources / >= 2 confidence thresholds are unchanged. Also add regression tests that construct genuine 2-vs-2 ties (both sides clearing threshold) where the lexicographically larger answer comes from the higher-priority sources, so the tests fail against the old lexicographic-only logic and pass against the fix. The existing 1-vs-1 disagreement test only exercised the below-threshold path and never caught this bug.
…llback
Free geolocation APIs are known to switch ASN between a JSON number and
a quoted string. Both parseIpPn (int) and parseFreeIpApi (string) were
strictly typed, so the "wrong" JSON shape failed json.Unmarshal for the
whole payload and threw away an otherwise-good country/city result,
costing that source its consensus vote. Both now decode asn as
json.RawMessage and route it through a shared parseASNValue helper that
accepts a raw number, a quoted number, or a quoted "AS"-prefixed form,
degrading to ASN: 0 on anything else instead of failing the parse.
Also strengthens TestSourcesTable to assert the exact source Name
values ("ip.pn", "freeipapi", "ipinfo") and that they match the keys
in consensus.go's SourcePriority map, since consensus tie-breaking is
keyed on those exact strings and a typo would silently fall back to
the unknown-source rank with no test failure. Adds table-driven
coverage for parseASNOrg's undertested fallback paths (bare org, org
that merely starts with "AS", AS-only, empty) and for the new ASN type
tolerance in both parseIpPn and parseFreeIpApi.
…urrently locate() fans out to 3 sources in goroutines so total latency is bounded by the slowest single source, not the sum of all sources. No existing test asserted this: patching locate() to fetch sequentially left all TestLocate* tests passing. Add TestLocateRunsSourcesConcurrently, which times locate() against 3 httptest servers that each sleep 200ms before responding, and asserts elapsed time stays well under the ~600ms a sequential fan-out would take. Verified the test fails (603ms) against a temporary sequential patch and passes (200ms) against the real concurrent implementation.
…ame, fix doc drift Final whole-branch review via mutation testing found ProbedAt and SourceResult.Name were completely unguarded: deleting either line let the whole suite pass silently, which would have made B (the ingestion server) reject 100% of submissions and destroyed source-priority tie-breaking, respectively. Strengthen TestLocateAllAgree to assert both. Also: guard Locate/locate against a nil *http.Client (previously panicked inside a spawned goroutine, unrecoverable by A2's prober process) with a test; clarify Locate's doc comment that a nil error does not mean the result is usable (callers must check CountryConfident); fix a stale "wired up in a later task" comment now that sources.go exists; note that the SourcePriority tie-break is currently unreachable with only 3 sources (exists so a 4th source can be added safely); and document that ASN plurality ties break by numeric order, not source priority, deviating from the design spec's "0 if none/tie" wording. No consensus/fetch/parse logic changed - test additions, comments, and a 2-line nil-client guard only.
Adds PinnedTLSConfig, SPKIPin, and ErrPinMismatch: the primitive that lets a later geolocation lookup made through an untrusted provider's tunnel detect a MITM'd response instead of silently trusting it.
…oned PinnedTLSConfig's VerifyPeerCertificate closure captured the *tls.Config variable and read cfg.ServerName at call time. The idiomatic per-host pattern -- clone := template.Clone(); clone.ServerName = host -- copies the func value but not the closure's underlying config pointer, so the check kept reading the template's empty ServerName, found no pin-map entry, and silently returned nil. A malicious provider MITMing its own geolocation lookup would sail through ordinary CA validation with no error and no log: pinning that appears healthy while doing nothing. Fix: build the verifier from an explicit, immutable host parameter instead of mutable config state. - PinVerifier(pins, host) closes over its own normalized copy of pins and host -- nothing a Clone() can decouple it from. - PinnedTLSConfigForHost(pins, host) is the new per-connection convenience: ServerName + MinVersion + a PinVerifier-built callback, safe to Clone() freely since the verifier ignores the config's ServerName field entirely. - PinnedTLSConfig(pins) keeps its old signature/behavior for existing callers and tests, now implemented via the same checkPin() helper (one implementation of the pin logic, not two). It remains clone-unsafe as a mutable template, but the trap is now inert: if its verifier is ever invoked with an empty ServerName -- exactly what happens when a clone's ServerName is set instead of the original's -- it fails closed with the new ErrPinHostUnknown rather than silently skipping the check. Added a regression test that reproduces the exact reported failure (Clone() + set ServerName on the clone + wrong-key cert) and confirmed it fails against the pre-fix closure before verifying it passes against the fix. Also added PinnedTLSConfigForHost coverage and an SPKI pin-stability-across-reissuance test (same key, new serial/CN/validity -> identical pin). geolocate/ package untouched. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Builds Tunnel.Open/HTTPClient/Close on top of connect.NewApiMultiClientGenerator + CreateTunWithDefaults + NewRemoteUserNatMultiClientWithDefaults, mirroring urnetwork/proxy socks/main.go. httpClientOverDialer takes the pin map directly (not a prebuilt *tls.Config) and builds a fresh PinnedTLSConfigForHost(pins, host) inside DialTLSContext per connection, avoiding the clone-and-mutate fail-open Task 1's review found. Adds a client-layer pinning regression test (CA-signed leaf certs + SSL_CERT_FILE override) that proves a wrong-key cert for a pinned host is rejected through the real client, not just the verifier. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Absolute paths (/root/urnetwork/...) only resolve on the machine they were written on. Use ../connect and ../glog, matching the convention in urnetwork/proxy's go.mod, so the repo builds for anyone who checks the sibling modules out alongside it.
…se, and tidy transport Fixes six review findings against the provider-pinned tunnel used to geolocate a provider through its own egress: - FIX 1 (important): DialTLSContext now refuses any https host absent from the pin map (ErrPinHostUnknown), instead of silently connecting unpinned. Enforced at the tunnel/client layer, not in pinning.go's checkPin, so checkPin's documented "unpinned host passes" contract for other callers is untouched. Open() also refuses a nil/empty Config.Pins (ErrPinsRequired). Host/key lookups strip a stray :port suffix (normalizeHost, used consistently by normalizePins, checkPin, and the new allowlist check) so a key like "ipinfo.io:443" cannot silently miss its intended host. - FIX 2 (important): added an offline Open/Close lifecycle test that asserts goroutine count returns to baseline after Close() and that a second Close() is safe. - FIX 3 (minor): Tunnel.Close() now calls mc.Close() explicitly (was stored and never read) instead of relying on async context-cancel teardown; Close() is now idempotent via sync.Once. - FIX 4 (minor): dropped the dead TLSHandshakeTimeout/IdleConnTimeout transport fields (never applied, since DialTLSContext owns the handshake and DisableKeepAlives means nothing is ever idle) and bound the manual HandshakeContext call with the real timeout instead. - FIX 5 (minor): the tun-read pump goroutine now logs before exiting on a read error, matching proxy/socks/main.go, instead of dying silently. - FIX 6 (minor): the client now refuses redirects (CheckRedirect -> http.ErrUseLastResponse), since a geolocation probe has no legitimate reason to follow one off a pinned host. geolocate/ is untouched. All 13 pre-existing providertunnel tests still pass; added 4 new top-level tests (8 subtests) covering nil/empty pins, a typo'd host key, a port-suffixed key (both the correct- and wrong-pin cases), an absent-from-populated-map host, Open's pins guard, and the Open/Close lifecycle -- verified green under -race. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…robedAt TestSubmitPostsContractShape previously asserted only 4 of 13 submitBody JSON fields, so renaming asn/org/hosting (etc.) silently passed CI while the server would unmarshal the field as its zero value with no error anywhere. Strengthen the test to assert presence (per omitempty semantics) and a round-tripped, distinctive value for every field, add a city-not-confident case covering the omitted-city/region/booleans shape, and add a teeth-check confirming tag-rename mutations fail the test. Also remove Submit's loc.ProbedAt.IsZero() -> time.Now().UTC() fallback. Fabricating an "observed now" timestamp would defeat the server's age check and could permanently pin a stale/wrong location by winning its monotonic upsert against later genuine probes and evading the expiry sweep. Add ErrMissingProbedAt, refused before any network call, mirroring the existing ErrNotConfident pattern.
Scheduler.Run probes providers with at most Concurrency tunnels open at once (a hard instantaneous cap via a semaphore held across each probe's full lifetime, not just an average) and skips any provider successfully probed within CacheTTL. Only successful probes are cached, so a failed probe is retried on the next run. Now is injectable so TTL expiry is testable without sleeping. Also fixes a pre-existing data race in the shared stubSubmitter test helper: Submit incremented s.calls/s.last with no lock. That was safe under Task 4's single-goroutine tests but races once schedule_test.go shares one stubSubmitter across Scheduler.Run's concurrent probes; -race caught it. Added a mutex.
Wires providertunnel, geolocate, ingest, and prober into the operator-facing binary: enumerate providers via /network/find-providers2 (verified against the real server types), probe each one's egress location through a pinned tunnel, and submit results to the ingest endpoint. Deviates from the brief in two places called out by the task: parseByJwtClientId uses the proven gojwt-based approach from proxy/socks/main.go instead of manual base64 decoding, and geolocatePins() ships with real, verified SPKI pins (captured and cross-checked against providertunnel.SPKIPin at runtime) instead of empty ones, since providertunnel.Open refuses to run with no pins. Also documents, in main.go and the README, that providertunnel's pin check is leaf-only: the captured intermediate-CA pins are recorded per the brief but do not currently protect against a leaf rotation, since checkPin never inspects the rest of the chain.
checkPin previously only hashed and compared rawCerts[0], the leaf, so the intermediate-CA pins already embedded in cmd/egress-prober/main.go were inert and every routine leaf rotation (Let's Encrypt ~90 days, across three pinned hosts roughly monthly) broke probing until someone manually re-captured and redeployed the leaf pin. checkPin now accepts a match against ANY certificate in the presented chain -- leaf or intermediate -- iterating rawCerts and skipping any certificate that fails to parse, but still failing closed with ErrPinMismatch if nothing in the chain matches. This survives routine leaf rotation (the new leaf still chains to the same pinned intermediate) while still rejecting a MITM presenting a chain-valid cert from a different issuer, which is the actual threat model for these untrusted-provider lookups. Trade-off: pinning an intermediate trusts that CA, not one specific certificate, for the pinned host. All existing behavior is preserved exactly: empty rawCerts rejects, an unknown host still falls through to the tunnel-layer allowlist rejection, an empty allow-list for a present host still rejects, host matching stays case-insensitive and :port-normalized, chain verification stays on (InsecureSkipVerify never set), MinVersion unchanged. Updates doc comments on the pin functions, cmd/egress-prober/main.go's pin table comment, and README.md's "Certificate pinning" section, all of which previously and correctly documented the intermediate pin as inert -- that caveat is now obsolete and is corrected to describe the new chain-wide behavior and its trade-off instead. Adds two tests built on a self-signed CA -> intermediate -> leaf chain: one where only the intermediate's pin is allowed (the rotation scenario -- confirmed to fail against a leaf-only revert of checkPin), and one where neither leaf nor intermediate matches (must still reject). All 20 tests in providertunnel pass under -race; full repo go test/build/vet/gofmt clean. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
… rawCerts (C1) CRITICAL: checkPin matched SPKI pins against rawCerts -- the certificate message the PEER sent, which is entirely attacker-controlled. Go's TLS stack puts rawCerts[1:] into an intermediates pool and only uses what path building needs; it never restricts rawCerts to what the verified path actually contains. An attacker holding a leaf for a pinned host issued by ANY CA in the trust store could chain-verify via their own path and then pad the Certificate message with the real, publicly downloadable pinned intermediate as inert dead weight -- checkPin would find its SPKI sitting unused in rawCerts and accept. Since certificate pinning is the only thing preventing an untrusted provider from forging its egress country, this reduced pinning to no protection at all. Fix: checkPin now matches against verifiedChains -- the path(s) crypto/tls actually validated -- with a rawCerts fallback used only when verifiedChains is empty (direct unit-test calls; unreachable in production because InsecureSkipVerify is never set on any *tls.Config this package builds, confirmed by reading crypto/tls's verifyServerCertificate). All existing pin invariants are preserved (empty rawCerts rejects, unpinned host passes, empty allow-list rejects, case/port normalization, ErrPinsRequired, InsecureSkipVerify never set). Teeth-check: TestCheckPinBypassViaDeadWeightIntermediate, a real TLS handshake reproducing the bypass, FAILS against the old rawCerts-based logic and PASSES against the fix (see A2-task-6-report.md). TestCheckPinAcceptsRotatedLeafThroughRealHandshake covers the intended leaf-rotation/intermediate-pin case through the same real-handshake path. Also, in cmd/egress-prober and prober (Important/minor findings from the same review): - I1: listProviders enumerated only providers the server's OWN geo database already believes are "best available" (US), the inverse of what a location-correcting prober needs, and was weighted-random/shuffled per call. Now enumerates every location with providers via GET /network/provider-locations and unions POST /network/find-providers2 results per location_id, skipping (and logging) any location whose query fails rather than aborting the pass. Verified field names against the server source (model.FindLocationsResult/FindProviders2Args/ FindProvidersProvider). - I2: Scheduler.Run now logs each per-provider probe failure (provider id + error), capped at the first 10 distinct error messages per pass plus a suppression notice, so a broken -platform-url/jwt/pin is debuggable from a VPS log instead of an opaque failed=N count. - I3: egress-prober now exits non-zero when the provider list can't be fetched, and when a single-shot pass (-interval 0) submits nothing while recording failures -- so a permanently broken prober no longer reports success to cron/systemd forever. The long-running loop still retries through transient failures rather than exiting. - M1: -interval < 0 is now rejected at startup instead of degenerating into a sleepless retry loop. - M2: Scheduler.probed now evicts entries older than CacheTTL on each Run, so a long-lived process doesn't accumulate one entry per provider ever probed. - M3: README no longer links a design-spec path that doesn't exist in the server repo; replaced with a prose design summary. - M5: -probe-timeout <= 0 is now rejected at startup (a non-positive value silently disabled both the client and TLS handshake timeouts). M4 (geolocate consensus Country can be empty) is explicitly out of scope per instructions and untouched.
… names it (M4) ipinfo (parseIpInfo) returns only an alpha-2 country code, never a human-readable name, by design. A quorum of ipinfo plus any other source that also happened to supply no name yielded CountryConfident == true with Country == "", which the server rejects (400 "Missing country.") since an empty canonical location name would corrupt its location table. This surfaced as an unexplained submission failure rather than bad data, but it's a real gap: closing it here. Add geolocate/countries.go: a standard-library-only, compact map from all 249 currently-assigned ISO-3166-1 alpha-2 codes to English short country names (18 entries use a common-usage override instead of the ISO literal inverted "X, Y of" form, e.g. KR -> "South Korea" not "Korea, Republic of"; see the file's doc comment). consensus() in consensus.go now falls back to this table only when no source supplied a name for the winning country code; a source-supplied name still always wins, and a code outside the table leaves Country empty (no placeholder), preserving today's safe reject-rather-than-corrupt behavior for unknown codes. Also investigated the same class of gap for city/region (server also rejects an empty region on a city-confident submission). Found it's structurally possible but has no fallback available: unlike country codes, region has no compact global code->name table to key from with data geolocate already has (ISO 3166-2 is per-country, not a compact global map, and sources give region only as free text with no subdivision code). No fix applied there; documented as a finding in .superpowers/sdd/A2-task-6-report.md (gitignored, not part of this commit). Tests added to geolocate/consensus_test.go and geolocate/countries_test.go cover: table-fallback fill-in, source-name-wins-over-table, and an unknown code staying empty (no placeholder). Confirmed via git-stash teeth-check that TestConsensusCountryNameFallbackFromTable fails against the pre-fix consensus.go and passes after. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
consensus() was setting CityConfident=true whenever >= 2 sources agreed on a city, even if no source supplied a Region. The receiving server rejects any city-confident submission with an empty region, which fails the ENTIRE POST -- including a perfectly good, independently-confident country result that the server would gladly have stored. CityConfident now requires city agreement AND a non-empty resolved Region. When no agreeing source named a region, City/Region stay empty and CityConfident stays false, degrading cleanly to country granularity (which the server accepts) instead of fabricating a placeholder region or having the whole submission rejected. Adds the regression test (city agreement, no region -> not confident, country unaffected) and the happy-path complement (city agreement with a region -> still confident). All existing tests still pass unchanged.
Open called connect.CreateTunWithDefaults, which inherits DefaultDnsResolverSettings with EnableLocalDns: true. When in-tunnel DoH produced nothing within its budget -- flaky provider, blocked 1.1.1.1:443, or a tunnel still coming up -- DohCache.resolve fell back to a plaintext port-53 query issued from the HOST's dialer, so the operator's own server IP asked a public resolver, in the clear, for "ipinfo.io" / "ip.pn" / "free.freeipapi.com". The TCP that followed still egressed through the tunnel, so the location verdict was never wrong and no test could see it -- but the operator's infrastructure must never be visible to the geolocation providers, and this happened with no log line and no error. Open now builds its tun with CreateTunWithResolver and an in-tunnel-only resolver: EnableRemoteDoh plus the RemoteDoh*/RemoteDns* server lists, every other toggle off and the Local* lists empty. This mirrors connect's own DefaultUpgradeMuxSettings, which disables the same toggles because they "would resolve off-tunnel or in the clear". A probe that cannot resolve in-tunnel now fails and is retried next pass instead of leaking. Tests: TestOpenUsesInTunnelOnlyDnsResolution captures what the real Open path asks for (reverting to CreateTunWithDefaults fails it), and assertInTunnelOnlyResolver reflects over every field of connect's DnsResolverSettings so a future sibling toggle cannot go unasserted. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…cord (F2)
consensus() could return CountryConfident with an empty Country (an agreed
code outside the ISO-3166-1 table -- XK, A1/A2/AP are all real codes free
geolocation APIs emit, and ipinfo never supplies a name, so it is always one
of the two votes for free) or with a non-alpha-2 code (normalizeCountry only
lowercases and trims, so two sources reporting "USA" passed straight
through). The server hard-rejects both ("Missing country." / "Country code
must be alpha-2."), and the scheduler caches successes only -- so such a
provider was re-probed and re-rejected on every pass, forever, burning a
tunnel, three lookups and a round-trip each time.
CountryConfident now means what CityConfident already means: a complete,
usable record. An unnameable or non-alpha-2 code degrades to
not-country-confident with the fields left empty, exactly as the no-majority
path does. No name is fabricated from the code.
ingest.Submit gets the matching last-gate check (ErrIncompleteCountry): it
owns the wire contract, and a doomed POST is not a one-off cost.
Tests: TestConsensusUnknownCountryCodeDegradesToNotConfident replaces
TestConsensusCountryNameUnknownCodeStaysEmpty, which asserted the broken
behavior; TestConsensusNonAlpha2CountryCodeDegradesToNotConfident and
TestSubmitRefusesIncompleteCountry cover the rest. Two existing ingest tests
now build a complete country record, since they test rejection surfacing and
the ProbedAt guard, not this one.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
-by-jwt and -operator-secret took os.Getenv(...) as their flag DEFAULT, and flag.PrintDefaults renders a non-zero default as `(default %q)`. Every flag.Usage() call therefore echoed both secrets verbatim to stderr: any missing required flag, a bad -interval or -probe-timeout, any parse error, and plain -h, which an operator runs routinely. Under the systemd deployment the README recommends that lands in journald; in CI it lands in build logs. It also inverted README.md's own advice, which presents these env vars as the way to keep secrets out of logs and ps. Both flags now default to "" and read their env var after Parse (envFallback), so the value can never reach the usage renderer. An explicit flag still wins, and the help text still documents that each env var is supported. Tests: TestHelpDoesNotPrintSecrets and TestMissingFlagUsageDoesNotPrintSecrets build and run the real binary with both secrets in the environment and assert neither appears in its output; the second also asserts the env fallback still supplies both values (only -api-url is reported missing). Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…e (F4)
fetchSource wrapped every request in context.WithTimeout(ctx,
PerSourceTimeout), a package var the CLI never set, so the effective bound
was always min(5s, -probe-timeout): raising -probe-timeout could not have any
effect and the operator's only latency knob was inert.
That 5s is structurally tight here. Every probe runs over a COLD tunnel --
providertunnel.Open returns before the multiclient has reached the platform,
and the tunnel is closed again after each provider -- so within one per-source
budget a source must complete session establishment, an in-tunnel DoH
resolution (TCP+TLS+h2 to the DoH server), and then TCP+TLS to the
geolocation host. connect's own defaults budget 30s for the dial alone. If 5s
is short in practice the symptom is total: <2 sources respond, ErrNoConsensus
every time, nothing ever cached, the same failure every pass.
Adds LocateOptions{PerSourceTimeout} plus LocateWithOptions; the zero value
falls back to the PerSourceTimeout package default, so Locate is unchanged.
The CLI now passes *probeTimeout.
Tests: TestLocatePerSourceTimeoutFromOptions asserts the option both raises
the bound above the package default (the real defect's shape) and lowers it,
and that a zero LocateOptions still uses the default.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…(F5) The three human-readable fields resolved by three different rules: the country name and the region were last-writer-wins, the city's display casing was first-writer-wins. Which rendering survived therefore depended on nothing but a source's index in the results slice -- with ip.pn reporting Region "Colorado" and ipinfo reporting "CO", ipinfo's "CO" won because it sorts later in `sources`, i.e. the least-trusted source's rendering beat the most-trusted one's. That is not cosmetic: the server feeds these into model.CreateLocation, which dedupes and stores location_name canonically and permanently, so a bad pick is durable. All three now go through displayField, which keeps the rendering from the most-trusted contributing source using the same SourcePriority order that already decides the verdict (ties keep the first offer, so the result does not depend on map iteration order). An empty value is never recorded, so a trusted source that omitted a field cannot blank out one that supplied it. Tests: TestConsensusDisplayFieldsPreferHigherPrioritySource brackets ip.pn between lower-priority sources so both old rules pick wrong on all three fields; TestConsensusDisplayFieldsIgnoreEmptyFromTrustedSource covers the omitted-field case. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Probing one named provider is how you answer "is this provider's egress really where we think it is" without waiting for a full pass, and it is what proved the design end to end: run from a US host against a Spanish provider, it reported Spain (ASN 8560, IONOS SE) rather than the host's own US/RAVNIX, with tcpdump showing zero packets to any geolocation API. Skipped unless MANUAL_PROBE_PROVIDER is set, since it needs a live provider, a real jwt and real egress. Build for a remote host with `go test -c -o manualprobe ./cmd/egress-prober`. It prints each source's individual answer next to the consensus, which is what makes a disagreement legible: in that run ip.pn said GB/London while the other two said ES, and the 2-of-3 country rule outvoted it. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…tring
A real probe of a German provider returned city "" from ip.pn, "Frankfurt am
Main (Innenstadt I)" from freeipapi and "Frankfurt am Main" from ipinfo. Two
sources agreed on Frankfurt in substance, but city agreement required an exact
normalized string match, so consensus saw two different cities, discarded the
city AND the region with it, and the provider fell back to country
granularity.
Add geolocate/placename.go: a small, lexical, stdlib-only matcher. It strips
parenthetical groups (depth-counted, so nested/unclosed/stray shapes cannot
panic), lowercases, folds Latin-1 Supplement and Latin Extended-A accents to
ASCII through an explicit table (golang.org/x/text is not available here and
full normalization is far more machinery than this needs), drops combining
marks so decomposed input is not split into two tokens, turns every other
non-alphanumeric rune into a separator, and splits. Two names match when their
token sequences are equal or one is a proper TOKEN prefix of the other -- never
an arbitrary substring, never a suffix, so "Frankfurt" matches "Frankfurt am
Main" while "York" does not match "New York". An empty name matches nothing,
including another empty name: two silent sources have not agreed on a city.
Wire it into consensus for both city and region. Region gets the same
treatment because it is the same class of problem and CityConfident requires a
non-empty region, so merging city variants alone would still leave results
discarded. Grouping is PAIRWISE -- the match relation is not transitive, since
"Frankfurt" matches both "Frankfurt am Main" and "Frankfurt Oder" which do not
match each other -- which is equivalent to the group forming a chain under the
token-prefix order, so building the candidate group around each result in turn
enumerates every maximal group.
The canonical display name is the SHORTEST agreeing variant by token count,
rendered from that source's original string so casing and diacritics survive
("Logroño", not "logrono"). The shortest variant is exactly the assertion every
agreeing source supports; a longer one asserts specificity that not all of them
confirmed, and the server canonicalizes and permanently stores the name it is
given. Token-count ties fall back to SourcePriority, consistent with how the
other display fields already resolve.
Unchanged: the >= 2 city threshold, CityConfident still requiring both a
non-empty city and a non-empty region, country/ASN/flag logic,
CountryConfident semantics, and every struct field. normalizeCity is deleted
with its last caller.
Known and tested consequence: a bare prefix from one source is read as a less
specific description of the same place, so "Kansas" merges with "Kansas City"
and "Frankfurt (Oder)" merges with "Frankfurt am Main". The second is genuinely
lossy -- see .superpowers/sdd/placename-report.md, which records it and the
minimal fix rather than hiding it.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
c14cbcf stripped every parenthetical unconditionally, which merged two genuinely different cities: "Frankfurt (Oder)" reduced to "Frankfurt", which then prefix-matched "Frankfurt am Main". Consensus published a confident city "Frankfurt" with whichever region the more-trusted source reported -- in the regression test added here, "Brandenburg" while the other source said Hesse. The server canonicalizes and permanently stores that name, so the bad merge is durable and hard to walk back. Parenthesized text in these feeds plays two incompatible roles. After a multi-token name it is a SUBDIVISION ("Frankfurt am Main (Innenstadt I)") that only some sources emit, so it must be dropped or agreement is lost. After a single token it is a DISAMBIGUATOR ("Frankfurt (Oder)", a real city ~90km away in a different Land), so dropping it destroys the only thing that tells the two cities apart. Nothing lexical distinguishes the two roles -- it needs a gazetteer -- so use the token count before the group as the proxy and break the tie toward the safe failure. The failure modes are not symmetric. A false merge publishes a wrong location that is stored as canonical; a false split only degrades that provider to country granularity, which is safe and self-corrects on the next probe. A rule that can only ever make matching stricter is therefore the right trade, and it is accepted that it occasionally splits something that could legitimately have merged ("Paris (75)" vs "Paris, France"), now documented in a test. PlaceDisplay applies the identical reduction, so the two directions stay in sync in BOTH: a dropped subdivision never reaches the stored name, and a corroborated disambiguator always does -- two sources that both say "Frankfurt (Oder)" now publish "Frankfurt (Oder)", not "Frankfurt". Tokens are counted the way PlaceTokens segments them, not by whitespace, so "Frankfurt-am-Main (Innenstadt)" counts three and drops its group. Nested groups inherit their parent's fate, an unclosed "(" swallows the tail when dropping and is kept verbatim when retaining, and a ")" with no opener is ordinary text. Still an accepted consequence, deliberately not chased further: a bare "Frankfurt" prefix-matches "Frankfurt (Oder)". A source that wrote no qualifier supplied no signal to disambiguate with, and the canonical-shortest rule keeps the published name at "Frankfurt" -- the most any source asserted. Everything from the original brief still holds: the three Frankfurt am Main variants match pairwise, "York" does not match "New York", diacritics survive into the stored name, and CityConfident still requires both a non-empty city and a non-empty region. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…ted certs The fleet had been failing every provider with no_consensus. Two unrelated external changes landed close together and each removed one source: ip.pn https://ip.pn/json now 404s. The endpoint moved to https://api.i.pn/json -- a DIFFERENT host, so it needs its own pins rather than a rotation of the old ones. The response schema is unchanged, so parseIpPn is untouched: status/country/countryCode/city/regionName/asn/mobile/ proxy/hosting are all still present and verified against a live response. ipinfo.io certificate rotated; BOTH leaf and intermediate changed, so the pin failed closed. That left ONE working source against MinSources = 2, so no provider could reach consensus. Routine rotation is expected -- the problem is that it is SILENT: a pinned source that fails closed just shrinks the source set, it does not raise anything. TestEveryGeolocationSourceHostHasPins closes the half of this that is checkable offline: the source table and the pin table must agree on the host set. The unit tests could not have caught the original bug because they point sources at httptest servers and never exercise a pin. Cert drift on a correctly-named host still needs a live check. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
The hardcoded geolocatePins() map is gone. The prober now fetches the pins the server observed -- GET /network/geolocation-source-pins, operator secret, the same auth as the due list -- at startup, and re-fetches every -pin-refresh-interval (default 1h; the server re-observes every 6h). On 2026-08-02 that map took the whole fleet offline: ip.pn moved its json endpoint to api.i.pn and ipinfo.io rotated both its leaf and its intermediate, so two of three sources failed closed at once and every provider came back no_consensus. Nothing raised, because a pinned source that fails closed is indistinguishable from a source that did not answer. Routine CA changes now self-heal within six hours. # Fail-closed is the whole point, so it is enforced in four places The geolocation lookup is issued THROUGH the provider under test. The pin is what stops that provider substituting a certificate and forging its own apparent location, which is the entire reason the probe exists. A prober that fell back to unpinned would keep producing location data that looks fine and is worthless -- strictly worse than one that stops, and the same failure class as the outage above. - Startup fetch fails -- unreachable, 404, 401, 500, or a 200 with an empty table -- and the process refuses to start. No fallback to unpinned, to an empty map (providertunnel.Open still refuses that; it is the last-ditch guard, not the only one), or to any built-in set. ingest.GeolocationPins therefore has NO "the server does not implement this" sentinel beside ErrDueUnsupported and ErrAttemptUnsupported: those exist so the prober can carry on without the server's help, and the equivalent here is exactly what must never happen. - A source host in geolocate.SourceHosts() with no usable pin in the served set is a hard error, not a silently-unpinned host. This is not pedantry: providertunnel's checkPin returns nil for a host that is not a key in the map, so a partial set does not merely leave that source unprotected -- it probes it UNPINNED, and the source goes on answering normally. Half a pin (an empty leaf or intermediate) counts as no pin. - A refresh failure keeps the last good set and logs; it never blanks it and never unpins. The startup fetch and the refresh are deliberately two call sites rather than one helper with a tolerate-failure flag, because that flag is the whole difference between them. Keeping a stale set is safe on its own terms: the pins were observed by the server on a direct WebPKI-validated connection, so an old one still rejects a substituted certificate. - A host the server serves that is NOT a geolocation source is dropped, with a log line. The pin map is also providertunnel's allowlist, and a set fetched over the network must not be able to widen it. # Ordering and refresh The confinement self-check still runs first, before anything touches the network, and the pin fetch comes after parseByJwtClientId -- so nothing about this change moves a network call ahead of the check that this host cannot reach a geolocation api directly. Verified against the built binary: the check passes, then the pin fetch runs. Pins are read from the pinSet on every tunnel Open rather than baked into tunnelCfg once, so an hourly refresh reaches the next provider instead of the next restart. get() returns a copy so a refresh cannot mutate the map a tunnel is mid-handshake against. # Tests TestEveryGeolocationSourceHostHasPins keeps its name and its intent -- it was added after the outage -- and now asserts the runtime gate rather than a constant: a served set missing any source host is refused. TestGeolocatePins- CoverEveryGeolocationHost folded into it. TestEgressHealthDestinationsAreNot- Pinned now builds the map the way the prober does, from a deliberately hostile served set that offers a pin for every health destination, and it got stronger for it. Teeth-check (TestAWrongServedPinFailsClosedRatherThanProbingUnpinned) runs the real chain end to end: a served pin set -> the real ingest client -> the real validation gate -> the real providertunnel verifier -> a REAL TLS handshake against a chain-valid certificate correctly named for the source host, so only the pin can reject it. With the correct pin the request completes and reaches the far side; with a wrong one it is refused with ErrPinMismatch and the far side is never reached -- the assertion is that nothing arrived, because a pin failure that still delivered the request would be decoration on an unpinned probe. The provider's multiclient tunnel is the one simulated link; it cannot be stood up in a unit test, and the pin check is the same code either way. Deployment order: the server's pin endpoint must be live before this prober starts, or it will (correctly) refuse to run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
The built test binary had no .exe suffix, and Windows exec.Command refuses an extensionless path (LookPath only tries PATHEXT extensions), so all six binary-driving tests failed environmentally. runProberWithSecretsInEnv also discarded the exec error, so those failures surfaced as content assertions against empty output instead of naming the real problem; it now fails fast on any non-ExitError, matching runProber and runProberWithJwt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TestMain's trust injection relies on crypto/x509's root_unix.go honoring SSL_CERT_FILE, which only happens on Linux; Windows and macOS use their platform verifiers, so all eight handshake-level tests failed there with "certificate signed by unknown authority" -- an environmental failure that reads exactly like a fail-closed regression in the pinning code. They now skip with an explanatory message off Linux; the verifier-level tests (which call VerifyPeerCertificate directly) still run everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The allowlist and the certificate pins are enforced in DialTLSContext, which only https traffic reaches; net/http routes http:// URLs through DialContext, which was the raw tunnel dialer with no gate. Any future caller or source-table edit producing an http URL would have ridden the provider's tunnel in cleartext -- unpinned, un-allowlisted, forgeable by the provider being measured -- with every test green, because TestHTTPClientUsesSuppliedDialer encoded the bypass as the contract. The transport now refuses the dial with ErrPlainHTTPRefused before any bytes traverse the tunnel; the dialer-wiring test proves its property via https instead (the supplied dialer observably invoked; the handshake failure against a plaintext stub is irrelevant to the wiring), which also frees it from the Linux-only trust-injection harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… failure The steady-state figure divided post-warmup bytes by the post-warmup window with no lower bound on that window. A transfer that stalls across the warmup boundary and then bursts -- the shape a windowed tunnel transport produces when a refill lands just after the boundary -- got the tail's bytes divided by the tail's few-millisecond spread and published with WarmupExcluded=true: 120 MB/s for a link truly carrying ~19 MB/s in the regression test here (17x observed in review). A steady figure now requires at least MinSteadyDuration (= WarmupDuration) of window; anything narrower reports the warmup-inclusive lower bound, which was measured accurate for exactly this shape. The lower-bound crossover moves from ~32 MiB/s to ~16 MiB/s; README updated. TestMeasureExcludesWarmup now paces its bulk phase (~640ms) instead of dumping it, so its steady window outlasts the floor and its 3x assertion holds by construction (stall = 1.5s >= 2x the fast phase). Separately, the totalElapsed <= 0 fallback claimed "no bytes transferred" while up to 16 MiB had moved; only the DURATION was unmeasurable (real on Windows dev machines, whose ~0.5ms clock tick can swallow a loopback transfer -- the cause of the TestMeasureTargetSendsHeaders flake, 3-in-5 on this machine). That path now returns a distinct ErrUnmeasuredDuration, the header test serves full-size streams like its siblings, and the byte-cap test tolerates the unmeasurable-duration case it can legitimately hit from a memory-backed stub. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both findings are the package's own stated defect class -- inability to verify read as evidence of confinement -- reached through gaps the existing guards could not see: - A filtering resolver's answer (0.0.0.0, ::, loopback, link-local, multicast) was accepted as the host's dialable address; the only record filter was 'parses as an ip'. On Linux 0.0.0.0 dials loopback, so with nothing listening the refusal counted as real evidence of confinement, and with any local service on the port it produced ErrNotConfined with a misleading diagnosis. Non-global-unicast records now take the same unresolved path as a non-ip record; a genuine record mixed into the same answer is kept. - A cancelled or already-expired parent context made every dial fail in microseconds with a context error, each counted as 'refused' -- the vacuous pass the MinTimeout floor guards against, reached via the context the floor cannot inspect. Verify now returns ErrInterrupted the moment the caller's context dies mid-check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scheduler.Run had no ctx.Done() path: after a SIGTERM it kept iterating every remaining provider, and providertunnel.Open builds a full netstack before ever consulting the context, so each one got a real tunnel constructed and torn down just so its probe could fail instantly on the dead context. A 500-provider batch reported hundreds of spurious failures, and single-shot mode exited 1 with 'submitted nothing and recorded N failure(s)' when the truth was that the operator pressed Ctrl-C. The spawn loop now checks ctx.Err() first (select chooses randomly among ready cases, so the explicit check must precede it) and selects on the semaphore versus ctx.Done(), accounting the unspawned remainder as Skipped with one log line. On the cmd side, NotifyContext kept the signal handler registered until main returned, so a second Ctrl-C during the wind-down was silently swallowed and the operator could not force-quit. context.AfterFunc(ctx, stop) restores default signal behavior the moment the first signal lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
listProviders skipped each failed find-providers2 call by design -- one hiccup out of hundreds of locations must not abort a pass -- but when every call failed it returned ([], nil), which flowed into the 'nothing to do (no providers, no failures)' exit-0 carve-out. An external cron then saw permanent silent success from a prober accomplishing nothing, the exact outcome the exit-code contract promises cannot happen. The divergence is realistic: provider-locations is an unauthenticated GET while find-providers2 is an authenticated POST, so a broken route or revoked jwt fails only the second. Zero successes across a non-empty location list is now an error naming the count and the last cause; partial failure keeps its skip-and-continue behavior, pinned by its own test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
egressHealthOptions derived the per-request bound from the SAMPLED round count (5 rounds of 6) and the -egress-health-all path then multiplied it by the FULL-TABLE round count (14 rounds of 10), so the shipped default drew 2.80x -probe-timeout: 2m48s of health budget at the 60s default, on top of geolocation, putting a blackholing provider at ~3.8x per probe. That is the ~4x regression egressHealthOptions' own comment says the arithmetic exists to prevent -- a 100-provider batch at -concurrency 4 back to ~95 min against a 1h -interval -- arriving through the default configuration. TestEgressHealthAddsAtMostOneProbeTimeout only pinned the non-default sampled path, so nothing caught it. The geometry is now chosen once, by the same flag that selects the run: egressHealthOptions takes allDestinations and sizes rounds from the requests and concurrency that run will actually use, so both paths spend exactly one -probe-timeout. egresshealth exports AllConcurrency and RoundsForAllDestinations for it, and BudgetForAllDestinations is reimplemented on top of them so the two cannot drift. The new test pins the shipped path (verified failing at 2.80x against the old arithmetic); flag help and README updated to say the per-request slice is shorter for the full table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Country and city each require two agreeing sources; the net-type flags were OR-ed across sources and the ASN was adopted on a plurality of one, so a single one of three free apis -- compromised, or merely wrong for an afternoon -- could set Proxy or Hosting, and dictate ASN/Org, on every provider the fleet probes, indistinguishable downstream from a value all three agreed on. Both now need MinSources votes and otherwise stay unset, which is the direction the rest of consensus already fails in: silent about what it does not know rather than confident about what one source said. TestConsensusFlagsOr is replaced by TestConsensusFlagsRequireCorroboration, and TestLocateAllAgree's single-source Proxy assertion is inverted -- both encoded the old contract. Adds the single-rogue-source table test (one source lying about country, flags and ASN at once) and the uncorroborated- ASN case, which is the 2-vs-1 resilience claim the suite asserted only implicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fallback scanned peer-controlled rawCerts whenever verifiedChains was empty -- which is the dead-weight-intermediate bypass itself: an attacker pads the wire chain with a legitimately pinned certificate that was never on the validated path. It was unreachable in production for exactly one reason: nothing in this package sets InsecureSkipVerify on the exported, MUTABLE *tls.Config values PinnedTLSConfig and PinnedTLSConfigForHost hand out. That is a convention held in place by a forty-line comment, and one debugging line elsewhere would have re-armed the full bypass with every test still green. It existed so the verifier-level tests could avoid standing up a handshake. They now build the verified chain themselves via a two-line chainOf helper, which costs nothing and lets the branch go. Added the regression test that fails closed on an empty verifiedChains even when the presented certificate's pin matches -- verified failing (accepted the unverified cert) before the branch was removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e lists Three edges of the same client: - Submit was the only method not mapping 401 to ErrUnauthorized, so the CLI's remediation advice never fired for it. The gap is reachable: against a server without the due endpoint the prober falls back to enumeration, which authenticates with the byJwt, so a wrong operator secret let the pass proceed and surfaced only as a per-provider submit_failed. - GeolocationPins wrapped its transport error with %s, flattening the cause, so errors.Is(err, context.Canceled) could not distinguish a cancelled startup from a genuinely unreachable server -- while the 401 case beside it kept its sentinel with %w. - The two egress-health failure-name lists were joined uncapped; a heavy-failure run names ~26 destinations (400+ chars) and these submissions are fire-and-forget, so a length rejection drops the health signal after one deduplicated log line. Both are now bounded by MaxNameListLen, and the probe_failure truncation is rune-safe (shared truncateUTF8) rather than a byte slice that could emit U+FFFD the first time a non-ASCII value reaches it. The server's column widths for the two name lists are UNCONFIRMED -- the cap is defensive, not a mirror of a known limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three ways the log-once machinery inverted under real inputs: - Both dedup maps are keyed on the full error text, and ingest.ErrRejected embeds up to 4096 bytes of server response body. A body carrying a request id or a timestamp -- ordinary -- makes every message distinct, so the gate logged one line per provider per pass (the flood it exists to prevent) and the map grew an entry per probe, forever, in a process built to run for months. A shared shouldLogOnce helper now caps both at maxLoggedDistinctErrors, the bound the scheduler already applies to its own error detail. - The tunnel-failure error wrapped in the provider id, so a fleet-wide identical failure (a wrong -platform-url, a revoked jwt -- the doc comment's own examples) read as one distinct error per provider and filled all ten of the scheduler's detail slots with copies of a single failure mode, suppressing genuinely different ones. The id is already in the log line's provider= field. - A due batch containing the same id twice opened two simultaneous tunnels to that provider, because recentlyProbed only becomes true after a probe completes. The enumeration path de-duplicates before Run sees it; the due list is whatever the server sent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MinTimeBudget (1s) sat below DefaultTimeout (5s), so a probe with 1-5s of deadline left passed hasTimeBudget, spent a 16 MiB deployment-wide reservation, and then had its read killed by the PARENT deadline rather than the measurement's own cap. readStream correctly classifies that as a failure, so the byte budget was charged and nothing was recorded -- and the provider logged failed(context deadline exceeded) instead of the SkipNoTime this situation has a dedicated string for. hasTimeBudget now takes the duration actually needed (the sampler's per-target cap, floored at MinTimeBudget). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
normalizePins overwrote on collision, so "ipinfo.io" and "IPINFO.IO:443" -- which normalize to the same host -- silently dropped one set in map iteration order, nondeterministically. A dropped pin is a probe that fails closed against the legitimate host after a rotation the surviving entry does not cover. They are merged now, which is also the safe direction for the allowlist since the caller had already permitted both keys. Open also stored cfg.Pins by reference, so a caller mutating its map after Open would race the per-dial reads in DialTLSContext. The cmd layer happens to hand over a fresh copy per Open; the copy here makes the package safe by construction instead of by the caller remembering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
probeHosts claimed to be "every third-party host this process reaches through a tunnel" while excluding the bandwidth CDN target, which is both third-party and configurable. A jail that lets the prober reach it directly is the same defect as one that lets it reach a geolocation api, and an operator translating probeHosts into -confinement-address entries would never have learned it existed. It is included now unless -skip-bandwidth is set (a host this process will not touch is not evidence about anything), and a custom -bandwidth-cdn-url follows. The operator's own api host stays out, deliberately: it is not third-party and a deployment may legitimately allow it. A negative -cache-ttl made recentlyProbed always false, silently disabling the enumeration cache, while every other duration flag fails fast. It now exits 2 naming the flag, and points at 0 as the way to disable the cache on purpose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- prober: the deferred closeTunnel error was discarded, so a tunnel
failing to release its netstack was completely invisible; it is now
logged through its own deduplication gate (separate from the health and
attempt gates so a noisy teardown cannot consume their budget) and
still never fails the probe.
- egresshealth: Check drew a sample and then threw it away whenever
AllDestinations was set; the flag is checked first now.
- cmd tests: TestMain removes the ~36 MB compiled prober that buildProber
deliberately keeps outside t.TempDir, instead of leaving one per run in
the system temp directory.
- cmd tests: TestNewProberReportsAttempts now also asserts HealthResults
is wired -- without it the health check still runs and still logs, so
every existing assertion passed while results reached no server.
- prober tests: TestProbeOneTunnelFailureIsReported wires no reporter and
asserted nothing about reporting; renamed to say what it actually
covers and to point at the attempt_test.go case that does.
- providertunnel tests: dropped a dead 'var _ = tls.Config{}'.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
go.mod declared github.com/urnetwork/urnetwork-operator-proxy while the repository is urnetwork/operator-proxy, so nothing outside a replace directive could ever import this module -- 'go get' resolves the module path against the repo that serves it, and that path is served by nothing. The mismatch is invisible today only because every consumer is inside this repo. Mechanical: the module line plus every internal import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The committed go.sum did not satisfy the current connect main, so a fresh clone of this repo plus its siblings failed at 'go build ./...' with "updates to go.mod needed; to update it: go mod tidy" -- before a single line of this code ran. The pinned indirect versions (pion, quic-go, btree) are refreshed to what connect actually resolves today. The workflow is what keeps that from recurring. It clones the two sibling modules the replace directives point at (a lone checkout cannot build, and the failure names nothing about the real cause), then verifies go.mod and go.sum are current, builds, vets, checks gofmt, and runs the suite with -race -count=1. Two things only this job can do: -race needs cgo, which is unavailable on the Windows machine this branch was developed on, and the eight providertunnel handshake tests inject their test CA via SSL_CERT_FILE, which crypto/x509 honors on Linux only -- everywhere else they skip. This is the first CI in the repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review of the previous commit found the absolute MinSteadyDuration floor was the wrong shape in both directions. It gave up too much: requiring a 500ms post-warmup window puts the steady-path ceiling at ~16 MiB/s, so every provider between there and ~32 MiB/s started reporting the warmup-depressed lower bound -- the first band the 8-stream rewrite unlocked -- and that degradation is invisible server-side, because ingest submits only the rate and the byte count. And it caught too little: it only rejects NARROW tails. A long stall followed by a tail wider than 500ms still divided the tail's bytes by the tail's own spread; review measured 5.1x inflation still published as steady, with a residual bound of ~9x at the default timeout. The window must instead cover at least 1/MaxSteadyInflation of the transfer. Excluding the warmup can raise a rate by at most totalElapsed/steadyElapsed, so bounding that ratio directly bounds how far a steady figure may exceed the whole-transfer aggregate -- the physical ceiling for bytes that demonstrably moved in that wall clock. Inflation is capped at 4x whatever the stall's width, and the ceiling rises to ~24 MiB/s theoretical / ~21 measured. The old ~32 was partly illusory: at 31 MiB/s the steady window is already under 20ms. The new test uses a tail of ~1.3s -- far past any absolute floor -- and asserts the inflation bound directly; verified failing against the absolute floor and passing against the ratio. The existing burst test's unguarded negative sleep is clamped so it cannot silently stop testing a stall. Stale ~32 MiB/s comments corrected, and the Sample doc now records that WarmupExcluded never reaches the server. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The cap added in the previous commit was permanent. These gates live on the Prober, which lives for the whole process -- months -- so ten transient errors (a burst of 503s whose bodies carry request ids) burned the budget forever, and a LATER fault breaking every provider, such as a rotated operator secret answering 401 on every attempt report, then logged nothing at all. That is the silent failure the logging exists to prevent, reintroduced by the fix for the flood. The comment claimed this matched the scheduler's reasoning. It did not: the scheduler's map is local to one Run and re-arms every pass. The gates now do the same -- Scheduler.Run resets them at the start of each pass -- and each reset reports how many distinct errors the finished pass withheld, which the prober never did (the scheduler already does). The loose (mutex, map) argument pairs are now one errGate type, so a caller cannot pair the wrong mutex with the wrong map. Regression test drives a full budget of transient errors, resets, then a new fleet-wide fault; verified logging zero lines for that fault when the reset is a no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t on element boundaries Three more findings from the review of this branch. The cancellation fix stopped the scheduler spawning but left the exit condition alone, so the symptom it named in its own commit message survived: probes already in flight still fail on the dead context and land in sum.Failed, so a single-shot pass interrupted with Ctrl-C still exited 1 claiming it 'submitted nothing and recorded N failure(s)'. An interrupted pass now logs the interruption and exits zero -- nothing about the fleet was learned either way. The provider-list fetch path above it had the same misdiagnosis and the same guard. The egress-health name lists were cut at a byte offset, so the last element was usually a fragment: review measured a 131-name list cut at 512 bytes ending in 'kernel-org-mirror' when the real destination is 'kernel-org-mirrors' -- indistinguishable from a genuine name, so a query for providers failing that destination silently returns nothing. They now cut on element boundaries and carry a '+N more' marker, because with 73% of names dropped a reader must be able to tell a truncated list from a short one. truncateUTF8 also guards a non-positive limit rather than panicking. bandwidth's hasTimeBudget doc now records that the guard cannot fire in the shipped wiring -- nothing on the cmd path puts a deadline on the scheduler's context -- so it is future-proofing, not a live fix, and says what to weigh when a per-provider deadline does arrive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n review geolocate: requiring two votes for the net-type flags made hosting and mobile PERMANENTLY false. Only ip.pn's parser populates them (freeipapi carries proxy; ipinfo carries none), so the bar was one no source set could clear -- a silent total loss of the signal, worse than the single-source risk it was meant to close. The threshold is now the corroboration actually available: two votes where two sources can express the flag, one where only one can. Parsers declare capability (FlagSupport) so 'cannot report' is never counted as a vote against. The existing test passed only because it hand-built a shape no parser emits; the new test drives the REAL parsers, and fails against a flat threshold. cmd: -egress-health-all left 4.29s per request at the shipped 60s default -- below the 6s this package's own DefaultConcurrency comment rejects as 'cold-start timeouts charged to providers as blackholes', and far below the 10s documented floor. Fixing the total budget had merely relocated the breakage. The full table cannot fit a viable per-request floor in 60s (14 rounds x 10s = 140s), so the prober now refuses to start when the derived value is below the floor and names the -probe-timeout that would work, and the default is sampling, which clears the floor at 12s. NOTE: this reverses an intentional default -- the full table is still how concurrency gets exercised, it just needs -probe-timeout raised to match. confinement: RFC1918/ULA/CGNAT answers are global unicast, so the previous filter let them through -- and AdGuard's Custom IP mode and split-horizon corporate resolvers answer with a LAN address, not 0.0.0.0. If anything on that address serves 443 the dial SUCCEEDS and the prober refuses to start on a correctly confined host: a false accusation that takes the deployment down. Also, Addresses shared one resolution budget across all hosts, so a resolver hanging on the first left the rest 'unresolved' and the caller proceeded on a degraded pass -- the same vacuous-pass shape Verify refuses on. And Verify no longer discards a check that genuinely finished when the context dies after the last dial. providertunnel: Open's pin-map copy had zero coverage (reverting it left the suite green); now tested. egresshealth: BudgetForAllDestinations was vestigial and its docs prescribed the bug this branch removed -- deleted, docs corrected. CI: sibling clones are pinned to SHAs, since tracking connect main unpinned makes every open PR red when connect moves; added permissions, timeout and concurrency. README documents the sibling checkout the build requires. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fix every finding from the PR #1 branch review
The server's due queue hands work out but never claims it: its dedupe only bites once an attempt row lands, which is at submit time, minutes after a batch went out. So every prober polling inside that window receives the SAME rows. N probers repeat one prober's work and throughput does not rise as hosts are added -- six edge servers buy nothing over one. -shard-count / -shard-index claim one slice. The server shards on a hash of client_id (urnetwork/server#430), so the slices are disjoint and need no locks, leases or new columns. Defaults are the single-prober case and send nothing new: below a shard count of 2 the parameters are omitted entirely, so an unsharded deployment issues the identical request it always did and this still works against a server that predates them. An out-of-range shard is rejected at startup and again in the client, for the same reason -due-limit already is: the server answers 400, but a prober quietly probing nothing on every pass looks exactly like a fleet that is already fully probed. Also corrects the -due-limit help, which claimed the server's maximum was a fixed 500. It is now configurable per deployment (provider_egress_due.yml), and 500 is only the fallback.
Take one shard of the due queue instead of all of it
Two things that belong to the fork, not to a public upstream repo: - .gitignore held nothing but `.superpowers/`, a local agent-tooling scratch directory. That is a property of one contributor's machine, not of this project. Replaced with the artifacts this repo actually produces -- the egress-prober binary the README tells you to build into the working tree, plus go test output -- so the file earns its place instead of leaking a toolchain nobody else runs. - The CI workflow triggered on pushes to `main` and `beta`. `beta` is a branch on the fork; upstream has no such branch, so the trigger was dead configuration that also advertised the fork's layout. Pushes to main plus every pull request is the upstream-correct set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
No description provided.