feat: developer screen for diagnosing connection freezes - #467
Open
Ryanmello07 wants to merge 20 commits into
Open
feat: developer screen for diagnosing connection freezes#467Ryanmello07 wants to merge 20 commits into
Ryanmello07 wants to merge 20 commits into
Conversation
Records the read-only investigation into the multi-exit freeze reports (DNS/websites hanging until reconnect or browser restart) and the plan to fix it. Five ranked causes, all read out of source and none observed at runtime: UDP flows get no teardown signal (ipOosRst is TCP-only, so DNS and QUIC blackhole silently where TCP would get a RST); TCP retransmits are suppressed up to ~30s while a stalled-but-not-dead exit holds the flow; per-site affinity degrades to per-IP whenever the DNS mux cannot observe a query; affinity is computed once at flow creation, so a late hostname splits a site across exits; and the 2-minute idle timeout is far more aggressive than VPN norms. Also records what the investigation ruled out -- providers call DialContext and terminate TCP, so exits are split-TCP user-space NATs, not packet forwarders. That means a TCP connection cannot survive an exit change by design, and PMTUD blackholing is largely not the culprit despite the absent MSS clamp. Decisions: multi-IP becomes an orthogonal toggle so Web and Streaming share one UI (Auto untouched); every fix ships behind a runtime toggle, since the ranking is unverified reasoning and shipping five silent fixes would leave us unable to tell which mattered; the developer section ships in all builds rather than debug-only. Phase 0 blocks the rest: no Go tests run in CI anywhere today and there is no Go toolchain or JDK locally, so nothing in the data path can be verified until that exists. (cherry picked from commit 40e87b3)
Spreading across several exit IPs used to be implied by picking Web, with a "Fixed IP" switch expressing the opposite. That framing made the two modes look like they differed in IP behavior when they actually differ in how providers are selected -- both can spread across exits, and both can pin to one. Inverts the switch to "Multiple IPs" so the choice is stated positively and reads as its own option rather than a qualifier on Web. Web and Streaming now present identically; Auto keeps the switch disabled because it balances across both windows and ignores an explicit window size, so there is nothing there for the switch to act on. Auto's own behavior is unchanged. The stored representation is untouched -- a window pinned to exactly one is still the single-exit case -- so an existing profile reads back the same way and no migration is needed. Leaves the now-unused fixed_ip string in place: strings.xml is generated upstream, and deleting an upstream key invites a conflict on the next localizations regen for no benefit. Verified locally: compileGithubDebugKotlin passes, strings.xml has no duplicate keys and parses. (cherry picked from commit 3002aac)
Six reliability fixes ship default-on, and the code cannot tell which one matters for a given user's freeze -- the ranking behind them is reasoning, not measurement. This is what turns that into something testable. Each fix gets a switch that takes effect on the next packet rather than on reconnect. That is the whole point: reconnecting to change a setting destroys the freeze being investigated, so the toggles have to work while it is happening. Every switch off restores exactly the behaviour that shipped before that fix, and "Reset to shipped defaults" puts everything back, so an experiment can always be undone. The exit readout lists each provider channel with the flow count pinned to it, which is how the affinity work becomes visible: a site split across exits reads as flows spread over several rows instead of collected on one. Drop kills a single exit and leaves the others working -- the failure the teardown fixes actually address. Shuffle replaces all of them at once, which is useful for forcing re-selection but looks nothing like a real outage. Stall makes an exit swallow packets without acknowledging or erroring, reproducing the neither-healthy-nor-dead state the tcp collapse bound exists for, which otherwise only occurs when a provider misbehaves at exactly the wrong moment. A plain Settings section above Version info in every flavor, not a hidden gesture and not BuildConfig.DEBUG-gated: these are meant to be reached while a connection is misbehaving, and hunting for a secret tap target at that moment is the wrong experience. Disconnected, the section says so rather than showing defaults that are not in force. Verified locally: all four flavors compile, strings.xml has 486 keys with no duplicates, and no unescaped apostrophes. (cherry picked from commit 5e612ae)
Device timings with curl phase breakdown reframe the problem. DNS is not the bottleneck -- 1.5ms warm, 190ms cold. Exit selection is not either; the warm pass is fast across all eight destinations. The cost is the TLS handshake at ~510ms against a ~90ms tcp handshake, making every new connection roughly 10x its off-vpn cost. Records why that bucket is so large: providers are split-tcp, so ConnectionState.SynAck answers locally and the provider does not dial the real server until traffic arrives. The provider->server connect therefore lands inside the TLS phase. ~510ms is that connect plus TLS round trips across two legs, not a protocol defect. Also records a hypothesis that did not survive checking -- that the transport pays a round trip per packet instead of windowing, which would have explained the multi-packet certificate flight. transfer.go has an ack window, a 32-deep pack buffer and a 256KiB in-flight floor. Written down so the same guess is not made again. Phase 6 lists four candidates, each to be toggled and measured rather than assumed: tls resumption across exits, provider-side connection reuse, destination-aware exit selection, and racing first contact. Separately, github.com cold showed a normal handshake followed by a 5.5s stall in the response body. That is the freeze, caught in a measurement for the first time, and it belongs to the Phase 1 work rather than here. (cherry picked from commit 30ab292)
Running the same hosts over plain http and https settles it. Http, with no handshake at all, pays the same ~600ms between tcp connect and first byte that https spends in its tls window; the request after the handshake costs only ~120ms. Tls is not expensive -- it just happens to occupy the window where the provider dials the destination and relays the first exchange. Client↔provider is healthy at ~120ms. The ~600ms sits on the provider→server leg, roughly three round trips, which is more than a bare tcp connect should need. Reorders phase 6 accordingly: provider-side connection reuse and destination-aware exit selection first, plus a new item to check whether contract setup or policy work delays the provider's dial. Tls session resumption is struck out. Also records that `openssl s_client -reconnect` reports New for every connection under tls 1.3 regardless of whether resumption worked -- the ticket arrives after the handshake and -reconnect does not wait for it. Noted so the false negative is not treated as a finding later. (cherry picked from commit 8ea0429)
Three sequential connections to the same destination held ttfb-conn flat at 559 / 571 / 521 ms, while conn itself fell from 213ms to 100ms as the client↔provider path settled. The provider→server leg is not reused between flows at all -- every new flow pays the full dial. That confirms provider-side connection pooling as the lever, from measurement rather than inference. Promotes the pre-dial overhead check ahead of building the pool. ~550ms is far more than a dial to an anycast host should cost, and pooling would mask that overhead rather than remove it. (cherry picked from commit 0fba286)
Device testing showed the collapse-hold toggle made no observable difference to a stalled exit: with it on or off, the browser stayed frozen until it was refreshed. Releasing a retransmit hands it to the same exit that is swallowing everything, so nothing changes -- recovery needs the exit removed from the window, and nothing classified it as failed inside the 30s ack timeout. This is the control for the fix that does that. On, an exit holding unacknowledged sends for 3s is dropped and its flows re-race onto a working one. Off restores the previous behaviour exactly, so the Stall button can be used to A/B it against a live freeze. Verified locally: all four flavors compile against the new SDK binding, strings.xml has 488 keys with no duplicates and parses. (cherry picked from commit 9a2cc87)
Two changes driven by testing. The timing controls are values now, not switches. How long to wait before giving up on an exit trades recovery speed against dropping one that was slow but alive, and that balance differs per connection -- so it wants tuning per user rather than a number guessed once. Send stall, tcp collapse hold, and both idle timeouts each cycle through presets on tap. The first preset of every list is 0, which reproduces the behaviour that shipped before the fix it controls, so each can still be A/B'd against a live freeze. The tools also move out of the bottom of Settings onto their own screen, reached from a Developer row. They act on the live connection while something is going wrong, and the set is expected to grow; burying them under everything else was already awkward at seven controls. Also merges the leaderboard duplicate-key fix, which was sitting unmerged in its own PR -- so the crash was still present in every build tested so far. Verified locally: all four flavors compile, strings.xml has 493 keys with no duplicates and parses. (cherry picked from commit a44775c)
With a full window every row rendered as the same string. Client ids are ULIDs, so their leading characters encode creation time, and the channels in a window open within milliseconds of each other -- a leading substring throws away exactly the part that differs. Six exits all read "019fa806", which makes Drop and Stall unusable since you cannot tell which one you are acting on. Uses the tail, where the random component is. (cherry picked from commit b02e3ec)
Measurements are placed first because they are what the rest of the screen is for. Blast radius and recovery time make a candidate falsifiable: one that moves neither did not work, however good the reasoning behind it was. "Never came back" is shown next to recovery time so a change that abandons flows instead of recovering them cannot hide behind a better average. The test suite runs probes through the live tunnel. The run to do first is to start it, stall the exit carrying the flows, and watch whether the next results fail in milliseconds or hang for the full timeout -- fast failures mean teardown is reaching the client and the delay is browser backoff, hangs mean it is not. Probe failure text is shown verbatim for that reason. Results poll during a run rather than only at the end, since the interesting moment is the one right after an exit is stalled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 7d7a34e)
These are build artifacts committed on the fork branch for device testing. They are not part of the change and have no place upstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gomobile only emits a Kotlin property for Go methods named Get*, so GetProbeResults reads as .probeResults but ProbeSuiteRunning does not -- it stays a call. The other bindings take arguments or return void, so this was the only site that got it wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 736fb2f)
Every probe reported a 15s timeout regardless of the tunnel's health, so the control could only mislead. Two harness bugs, neither in the data path: the dns probe used Go's pure-go resolver, which has no server list on android and falls back to [::1]:53, and the http probes resolve through a tun built without resolver settings. The measurements above are unaffected -- they are collected from real traffic rather than from probes, and remain the way a candidate fix is judged. The Go probe suite is left in the sdk to be fixed rather than deleted, but nothing reaches it from the app until it measures something real. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit e5d8ed6)
The previous commit removed the probe block by slicing from its opening comment to resetReliability, which swallowed the toggle and timing setters sitting between them. Only the probe members should have gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit fa7fea5)
The counters change while the screen is up -- a stalled exit takes seconds to be detected and removed -- and the actions here refreshed immediately, which captured the state before anything had happened. During device testing a stale zero read as "no provider failures" rather than "nothing measured yet" and was nearly taken as evidence that detection was broken. Also records Phase 8: the mainnet stall/drop results, the stall-detection bug they exposed, and the pre-existing false positive found reviewing its fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 336867e)
The bound responsible for the churn: how long a provider still acknowledging our sends may return nothing before being dropped. Off leaves only the unambiguous signal, a provider that acknowledges nothing at all, and is the comparison point for how much of the churn this branch accounts for. Nothing above ~30s is offered. The bound is compared against an age derived from stat buckets that are dropped once past the 30s stats window, so a larger value never fires and is silently identical to off -- it would present as more grace while measuring nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 7a03858)
Losing an exit kills every connection pinned to it. In testing, four removals out of 25 accounted for 92% of all connection loss, the worst taking out 484 at once -- a 35s stall and a laggy recovery. Removal rate was never the thing being felt; concentration was. Unlimited restores the shipped behaviour and is the comparison point. Lower values cut the worst event further at the cost of splitting more sites across exits, so a site sees more than one egress IP -- which is why this is a knob rather than a fixed value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 931750c)
The default moved to 16 once recovery times were measured against teardown sizes; the comment still said 64. Records why the higher presets exist: they barely move the common case (median teardown was 36 flows) and are there to A/B the affinity cost rather than as serious candidates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit bfebe44)
beta-104 to beta-106: removals 1.92/min to 0.40/min, median gap 18.5s to 60.5s, over a 128 minute session. Reasons are now attributable: 41 no-receive-ack, 10 no-receive-syn -- the earlier claim that all removals came from the receive branch was inference, and the syn branch is about a fifth. Records the teardown size distribution against reported stall durations, which is what set the flow cap at 16 rather than 64: the median teardown was 36 flows, so a permissive bound would not have touched any of the 15s stalls. Also records a 68s stall against a 2-flow teardown that none of this explains, with the two leads that came out of triage -- the tunnel dropping all ICMP, and the window running below target more often than at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 879d3c6)
The cap holds on device: teardowns 13-16, never above the bound, on both the affinity and race paths. Worst single failure 484 -> 16, blast radius 3.9. Recovery time barely moved while blast radius fell 30x, which rules out connection rebuild as the cost. 56% of traffic is QUIC, and RFC 9000 requires QUIC endpoints not to terminate on ICMP -- so UdpTeardownSignal is correctly built and structurally inert for the majority of flows. The corollary is better: QUIC keys on Connection ID rather than the 4-tuple, so those flows can survive an exit dying, and the re-race mechanism for that already exists. Records the suspicion that the blackhole fix partly undid itself: raising the receive bound 5s -> 20s cut removals 78% but quadrupled how long a flow black-holes before rescue, and QUIC's PTO doubles per loss. The recovery clock starts at removal, so that period is invisible in the metric and felt by the user. Includes the A/B to settle it, which needs no new build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit af7dd07)
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.
Adds a Developer screen with runtime reliability controls, failure injection, measurements, and an in-app probe suite.
This does not compile against
mainyet — it is up for review, not merging. It depends on:ProbeResult,ProbeSuiteConfig,StartProbeSuite)ReliabilitySettings,Exit,DropExit,StallExit), which live in a fork-only SDK file and have not been upstreamed. That is the real blocker; sdk#133 alone is not enough.Why
VPN freezes here have been diagnosed by how long they felt in a browser. That confounds what the tunnel did with what the browser decided to do about it — a browser sits on its own backoff after a reset, so a working fix and a useless one produce the same number. Several fixes that were correct in isolation changed nothing in use, and nothing could tell them apart.
This screen makes a candidate falsifiable:
Also makes multi-IP an explicit connect option so the streaming and web modes present the same UI.
Review notes
strings.xmlis generated — the header saysGenerated by @urnetwork/localizations (gen/generate.mjs). DO NOT EDIT.The strings here were added by hand and would be clobbered by the next generation run. They need to go through the generator before this could merge; I did not want to guess at that pipeline.takeLast(8)rather thantake(8): client ids are ULIDs, and channels in a window open milliseconds apart, so a leading substring renders every row identical and makes Drop/Stall unusable.docs/superpowers/record the device measurements behind this, including hypotheses that were disproved (TLS, windowing, exit churn). Happy to drop them from the PR if they do not belong in-tree.🤖 Generated with Claude Code