review: outage bandwidth leak staging mirror (closed, stale base) - #2
Closed
full-bars wants to merge 78 commits into
Closed
review: outage bandwidth leak staging mirror (closed, stale base)#2full-bars wants to merge 78 commits into
full-bars wants to merge 78 commits into
Conversation
Pulls in the peer-discovery / device-remote-rpc refactor, memory-budget tracking, egress rework, ip_assoc/ip_block_action, contract stats, and DoH/DNS pluggable-transport hardening from upstream. Conflict resolution: upstream deletes provider/ entirely (main.go, sn.go, sn_evm.go, ss58.go, Dockerfile, community-provider systemd examples) as part of moving subnet/token-payout functionality to the new Bittensor subnet repo (urfoundation/sn). We don't depend on that (Bittensor-based provider incentives are out of scope for our custom-server work), so we take upstream's deletion rather than keep it as a diverging fork. Also removed .github/workflows/provider-release.yml, which built and published releases from provider/ and now has nothing to build. Per direction: our branch should add to main, not diverge from it - custom-server-specific work (configurable server endpoint, wallet auth, etc.) stays layered on top of upstream, not forked away from it.
…e workflow migration
…eta/custom-server
Third and last link in unblocking the Android fork's CI. The chain is
android -> sdk -> connect, and all three forks have to move from
upstream together: the Android post-quantum merge needed SDK APIs, and
those SDK APIs in turn need connect symbols this branch did not have.
gomobile bind failed against the old connect with:
device_local.go:4616: undefined: connect.WindowTypeAuto
device_local.go:4622: unknown field PostQuantumEncryption in struct
literal of type connect.PerformanceProfile
device_local.go:1074: multi.PeerIdentities undefined
device_local.go:2589: multi.AddPeerIdentityChangeCallback undefined
device_local.go:2541: settings.OverrideAllowDirect undefined
Brings in 12 upstream commits: the post-quantum encryption and peer
identity work in ip_remote_multi_client.go and transfer_encrypt.go, the
forceAllowDirect/WindowTypeAuto fix from PR urnetwork#187, and the rolling ip
security and blocker data updates.
Merged with no conflicts. Verified all five symbols the SDK references
now resolve in ip_remote_multi_client.go, and this branch's own 19
commits (the provider release workflow and miner network selection work)
are intact.
Not compiled -- no Go toolchain available locally; the Android CI build
is the first real check.
No Go tests ran in CI anywhere in this stack. connect's only workflow is provider-release.yml, which triggers on go.mod/go.sum changes, and the SDK workflow builds Android and iOS without ever testing. The suite here is substantial and network-free -- the multi-client window type, pqe, identity and grid-leak tests all run as plain unit tests -- so it was only ever a matter of running it. This blocks the VPN reliability work: those changes are in the data path, and there is no Go toolchain on the dev machine, so CI is the only way to verify them. Landing the workflow first establishes a known baseline before any behaviour changes. Clones glog beside the checkout for the one local replace in go.mod. Uses -count=1 so a green run always means the tests actually executed rather than being served from the result cache.
The first run established the baseline: github.com/urnetwork/connect --
the package all the reliability work touches -- passes in ~9 minutes,
along with blocker and every other package. One package fails, and it
failed on a clean checkout before any behaviour change of ours:
extender_test.go:98 Get "https://localhost/hello": EOF does not equal <nil>
--- FAIL: TestExtender (1.22s)
TestExtender self-signs a cert, starts a TLS server and an extender
listener on :1442, then waits a fixed 1s (extender_test.go:71) before
issuing the request. EOF is what racing a not-yet-listening socket looks
like, so this reads as a startup race that a loaded runner loses rather
than a defect in the extender itself -- though that is inference from the
source, not something reproduced locally (no Go toolchain here).
Splitting it out rather than deleting or silently skipping it: the gate
now blocks on the packages this work actually changes, while extender
still runs and still reports, so the flake stays visible and can be fixed
on its own terms instead of being buried behind an excluded path.
When a flow's exit is removed, tcp flows get a rst and the application reconnects immediately. udp flows got nothing: ipOosRst returns false for every protocol but tcp, so the flow went silent and stalled until the application's own timeout. ip_packet.go carried a standing `TODO if udp quic, look at what should happen` for exactly this. Two things ride on udp. dns queries in flight when the exit changes are simply lost. And quic (udp 443), which browsers use for most large sites, sees its source ip change mid-session with no notice -- the session hangs rather than failing over to tcp, and commonly only clears on a browser restart. That is the closest match in the code to the reported symptom of sites freezing until the browser is restarted, and it is specifically why tcp does not show the same behavior. Adds ipOosUnreachable, building an icmp destination-unreachable addressed back to the source as if from the destination, embedding the original ip header plus the 8 transport bytes rfc 792 requires so the source can match the error to its socket. v4 checksums the message alone; v6 uses the ipv6 pseudo header like tcp and udp. Deliberately host/address unreachable (EHOSTUNREACH) rather than port unreachable (ECONNREFUSED). The destination port is fine -- the path through the exit is what vanished. Resolvers and quic stacks treat EHOSTUNREACH as a transient path failure and retry, while ECONNREFUSED can make a resolver mark a server dead; against the tunnel's own fixed dns address that would be a worse failure than the freeze being fixed. Routed through a teardownSourcePacket helper so all three source-direction teardown sites (rstFlow, removeClient, and the sendPacket error path) get it consistently. The destination-direction rsts are untouched. Behind MultiClientSettings.UdpTeardownSignal, default on, with a test asserting that off reproduces the previous behavior exactly. Not verified at runtime -- no Go toolchain locally, and the freeze itself has never been reproduced under observation. Whether this is the cause the user is hitting is still to be confirmed by checking whether stuck traffic is quic or tcp.
TcpCollapsePrevention drops a sender's pure retransmits on the premise that a packet committed to a client will either be delivered reliably or the client will be dropped. That premise holds while a client is healthy or plainly dead. It fails in between: a client that stalls without yet being declared dead keeps the flow's sequence state pinned, so canUpdateSequence rejects every retransmit for as long as failure detection takes -- bounded by BlackholeTimeout 5s, StatsWindowMaxUnhealthyDuration 15s and AckTimeout 30s. For that whole window the sender's only recovery mechanism is being discarded and the connection is frozen with no way to self-heal. The existing `TODO it's still not clear why one client might stop working occasionally` sits directly above this logic. Tracks when the sequence state last advanced and, once a flow has sat at the same state past TcpCollapseMaxHold, admits a retransmit and restarts the window. One retransmit per window rather than the whole backlog, so collapse prevention still does its job during an ordinary stall while a genuinely stuck flow keeps a path to recovery. Default 1500ms: past the ~200ms-1s of a first tcp rto, so a healthy flow's retransmits are still collapsed, and far under the 30s this exists to preempt. 0 restores the previous behavior exactly, which a test asserts rather than assumes. Applies to every platform building from this branch, not just android -- the stall it addresses is not android specific. Not verified at runtime: no Go toolchain locally, and the freeze has never been reproduced under observation. Whether C1 or C2 is the cause being hit is still open, which is what the per-fix toggles are for.
Per-site affinity depends on the dns mux having observed a plaintext query, so it can map a destination ip back to a hostname and pin a site's flows to one client by base domain. When it has no name, affinity falls back to the bare destination ip for ports 80/53/443. That fallback fires more often than it looks. The mux sees nothing when the app runs its own doh (chrome secure dns, android private dns) or when the os answers from cache -- the long-ttl case ReverseTtl's own comment calls out. A cdn-hosted site spans many ips, so per-ip affinity splits one page load across the window, and sites that bind a session to an ip break in a way that looks random. IpAssoc already solves the underlying problem: it clusters co-active destination ips into sites, and is what the block-action decision path uses to union a destination's cluster. This reuses it as the affinity fallback, keying on a stable cluster representative rather than the bare ip, so a site's ips group again even with no dns visibility. The representative is the minimum member, not whichever the map yields first -- map iteration order is randomized in go, so an unstable pick would hand back a different key per call and group nothing. Tested directly, since that failure would be silent. GetClusterAddrs is a lock-free atomic load, so it is safe to call with the parent stateLock that affinityIpPathsWithLock already holds. A known server name still always wins; this only touches the no-name path. Cluster membership evolves, so a flow created before a cluster forms keys on the bare ip and a later one keys on the representative. That affects grouping for new flows only and never migrates an established flow -- migrating one would cause exactly the breakage this fixes. C4 addresses the same race for the server-name path. Behind MultiClientSettings.ClusterAffinityFallback, default on.
TestClusterAffinityRepresentativeIsStable asserted 151.101.1.140 as the minimum of the set. netip.Addr.Less orders byte-wise, not lexically by string form -- the first octets are 93, 93 and 151, so the minimum is 93.184.216.7 and 151.101.1.140 is in fact the largest, despite sorting first as a string. The implementation was right; the expectation was written from string intuition. The order-independence assertions the test exists for all passed. Spelling the byte-wise ordering out in the comment so the next reader does not repeat it.
The cluster fallback added the first settings access to
affinityIpPathsWithLock, which until then was a pure function of the
config and the path. Five fixtures across the suite construct a bare
&RemoteUserNatMultiClient{} and call it directly -- encoding exactly that
contract -- so the new access nil-panicked
TestMultiClientServerNameAffinity.
Guarding in the function rather than adding settings to five fixtures:
the fixtures are right about what this function depends on, and the
guard keeps it callable the way the rest of the suite already calls it.
An unset settings falls back to plain per-ip affinity, which is the
pre-existing behavior.
Adds a regression test that names the invariant, so the next settings
access here fails on a clear assertion instead of a panic in an unrelated
test.
SequenceIdleTimeout tore down any flow idle for 2 minutes, tcp included. A tcp connection is routinely idle between requests -- ssh sessions, websockets, push channels -- and the application still considers it open. Traditional vpns hold tcp nat state for 5-30 minutes, so a 2 minute bound resets connections users have every reason to think are alive, and the teardown is not free: the flow gets a rst and whatever was using it has to reconnect. Udp has no equivalent notion of an open connection and its mappings are conventionally short lived, so it keeps the tighter bound. Splitting the two lets tcp match vpn norms without holding udp state longer than it is worth. TcpSequenceIdleTimeout defaults to 600s and applies at all three places the idle bound is read (waitForIdleUpdate and the v4/v6 reap paths). 0 falls back to SequenceIdleTimeout, restoring the previous single-value behavior, which a test asserts. The helper tolerates a nil path, following the lesson from the cluster affinity fallback: this file's helpers get called from lightweight fixtures, and a nil deref there surfaces as a panic in an unrelated test.
Implements the consult-only donor fallback: when a new flow's own affinity
group has no donor, read the destination-scoped groups an earlier nameless
flow would have joined, and inherit its client without joining those groups.
Extracts destinationAffinityIpPathWithLock so the registration path and the
bridge can never build the key differently, and inheritAffinityClient{4,6}
WithLock so the donor filter exists once per version.
Deliberately does not touch invalidateServerNames: that would take the
parent stateLock from a mux callback that can run inline on the egress
path, and would have to mutate live flows' affinity bookkeeping.
Held on this branch rather than beta/custom-server because it has no tests
and has never been compiled. Do not merge as-is.
Audit of C1 found the code choice made the fix a no-op on the one platform
reporting the freeze.
C1 used host unreachable (v4 code 1 / v6 code 3) on the reasoning that the
port is fine and only the path died. That is semantically truer and it does
not work. Linux maps ICMP_HOST_UNREACH to {EHOSTUNREACH, fatal=0} in
icmp_err_convert, and __udp4_lib_err discards a non-fatal error for any
socket without IP_RECVERR:
if (!inet_test_bit(RECVERR, sk)) {
if (!harderr || sk->sk_state != TCP_ESTABLISHED)
goto out;
__udp6_lib_err is the same, and ICMPV6_ADDR_UNREACH is likewise non-fatal.
Chrome's quic sockets and the android resolver do not set IP_RECVERR, so
the teardown signal never reached the application on android at all, while
still working on ios and windows -- a platform split nothing in the code or
tests reflected. Port unreachable is {ECONNREFUSED, fatal=1} on both
families and is delivered.
The reason host unreachable was picked -- that ECONNREFUSED might make a
resolver mark the tunnel's fixed dns address dead -- turns out not to
apply. IpMux.Receive short-circuits isLocalDestination traffic to the
internal stack, so the mux-terminated resolver flows never reach this
teardown path.
Also moves the version check ahead of building the embedded datagram.
ipOosUdpPacket panics on an unsupported version, so ipOosUnreachable
panicked where it was written to return false, unlike ipOosRst which checks
first. Not reachable in production, since paths come from
ParseIpPathWithPayload, but the graceful return was clearly intended.
Two owner-directed changes, both live-tunable from the developer menu: ProbeSampleHostCount: one qualification pass now dials the ENTIRE embedded health-host table by default (0 = all; a positive value restores a rotating block, 4 was the old width). Every sampled hostname is resolved -- the 3-name truncation is gone with the economics that justified it: a pass's probes are all in flight together against one timeout, so width costs kilobytes, never wall time. The verdict stays fraction-based (60%), so coverage widens without moving the bar. AffinityStickyPastCap: affinity-group inheritance is exempt from the flow cap. The veto was splitting a busy site's egress ip exactly when the site was busiest -- flow n+1 of a video session was refused its donor, raced onto a different exit, and services that bind sessions or signed media urls to the client ip (video cdns do) then rejected the strays. The cap still gates every race and rebind placement, so an exit can exceed it only by the growth of sites it already hosts, never by collecting new ones. false restores the veto, the A/B comparison point. domainAffinityAliases: cdn constellations collapse to one affinity group (googlevideo/ytimg/ggpht -> youtube.com, and the equivalent for x, meta, tiktok, netflix, twitch, reddit). The sticky cap fix alone cannot hold a video session together, because the manifest domain and the media domain are different eTLD+1 groups that can anchor on different exits -- and the signed media urls carry the client ip the MANIFEST was fetched from. One group, one exit, one egress ip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Exits() walks two maps (windows, then each window's client set), so every readout shuffled the rows and the developer screen's exit list visibly jumped positions on each refresh. Sort by window type then client id: stable while membership is stable, and a membership change moves only the rows it must. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four packages, each behind its own A/B knob, aimed at the one split source the ip-consistency work left open: our own suspicion. G-2 warning causes: the resize warning now carries WHY (draining / starved / unhealthy) alongside the shared bool, exposed through the exit readout. Before this every warned exit displayed as "draining", which made benches read as retirements. G-1 group-follow (QuarantineGroupFollow, default on): a quarantined exit keeps inheriting new flows from sites already living on it, while new sites, races, and rebinds still avoid it. Gated on receive freshness (GroupFollowReceiveFreshness, 10s): a receive-silent bench gets no fresh flows. Field motivation: five quarantines in six minutes on 2026-08-03, every one acquitted -- each scattered its sites' egress ips for nothing. The heartbeat gains follow=followed/scattered and the metrics gain the same pair, so the field can falsify it. G-6 load-scaled corroboration (BlackholeLoadCorroboration, default 8): the soft no-receive verdict's distinct-destination requirement becomes max(MinBlackholeDestinations, flows/8) -- a 24-flow exit needs 3 silent destinations, not 2. Every one of the false benches above was a 22-24-flow exit. Hard evidence paths untouched; 0 restores the flat bar. G-4a destination-exit attribution: DestinationExits() joins the live flow table to destination ips -- which exit carries each ip RIGHT NOW, pull-model so re-races and rebinds read as their current exit. This is the Local statistics join the app renders. G-5 (bench backfill) was found already implemented: setQuarantined wakes the owning window's resize via resizeWake, shipped with the A+B demote work. Nothing to add. Also: bare fixture channels identify by their args client id, so tests can distinguish exits without a full client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The field regression (2026-08-03, ~04:40-04:43): three loaded exits
executed in three minutes on "no ack progress 3s + liveness probe
timeout 1.5s" -- one had been acquitted on receive progress 15s
earlier, another was wide-probe-proven. Three independent providers do
not go identically silent for the same 4.5s; one cellular blip does.
The stall path outran the 5s uplink gate, the same storm mechanism the
receive verdicts were gated against in phase A, reintroduced by the
ported busy-probe path -- whose probe rides the same possibly-dead
uplink it investigates and so can never exculpate a phone-side outage.
The gate: a stall conviction is admissible only while some OTHER
window client shows receive progress (or its own probe ack) inside the
judged interval. Silence everywhere is the phone's silence: the
verdict is held, unlogged state untouched, the stall clock carries, and
a real stall convicts on the first pass after the uplink proves out.
An in-depth review (subagent, 2026-08-03) of the whole G delta then
found five defects, all fixed here:
1. G-1 group-follow was UNREACHABLE: the receive-freshness gate
demanded evidence a quarantined exit structurally cannot have (the
benching verdicts require a silent 30s window; any receive lifts
the bench). Regated on quarantine-episode age -- follow through the
first GroupFollowWindow (45s) of a bench, covering the observed
false-positive range while stopping before the ~60s
drain-to-conviction zone. Setting renamed to match
(GroupFollowWindow / GroupFollowWindowMillis).
2. The group ledger overcounted scatters: one flow placement makes
several inherit calls (one per group + fallbacks) and the per-call
count booked a scatter even when a later group donated. The inherit
functions now return their verdicts and the caller books ONE event
per flow (bookGroupLedger).
3. Held passes kept incrementing busyProbeSendFailures -- probes fired
while the verdict was inadmissible are evidence about the phone,
and two of them would convict instantly once the gate opened,
executing the exact exits the hold protects. The held branch resets
the run; a genuinely dead exit still convicts via probe timeout on
the first open pass.
4. The stall hold counted per-pass into a per-episode counter
(verdictsHeldUplinkStale), making it unreadable. Latched per stall
episode (stallHoldCounted), which also bounds the held log line to
one per episode.
5. The resize ulimit branch labeled a merely-full exit "starved". It
is warnCapacity ("capacity") now; starved means failing dials.
Plus the review's cheap improvement to the gate: a sibling's own
liveness-probe ack counts as uplink proof, so a window with several
simultaneously stalled exits resolves -- one acquittal proves the
uplink for judging the rest.
Accepted trade, documented: a window whose siblings are all idle holds
a dead exit until the ~30s no-send-ack/AckTimeout paths reap it. Safe
direction; the alternative was executing healthy exits on phone blips.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
migrateClientFlows runs the removal-time rebind's proactive half while the exit is STILL ALIVE: established quic moves to live replacements now (same partition, candidate order, and affinity-group cohesion as rebindFlowsWithLock), tcp and anything unplaceable stays and finishes naturally, and nothing is ever torn down. The resize drain branch runs it once per drain (per-channel latch) through a window seam, so a lifetime retirement hands its movable flows off at drain START and the eventual close finds little or nothing to kill. New flows already avoid the draining exit and inherit the moved group members on the replacement, so the group's donor flips for free. MigrateExit(clientId) runs it on demand -- the developer-menu drill. Gated by the existing QuicRebindOnExitLoss switch; off restores the pre-change drain exactly. [rel] event=migrate carries the counts. The quarantine-escalation consumer from the plan is deliberately dropped: a bench that sustains to execution is receive-dead, its tcp is not finishing naturally either way, and the removal-time rebind already moves the quic at execution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The tracker's entries close on the next provider-originated ingress for the destination -- and after a migration the OLD exit is alive and delivering until the server validates the new path, so every entry closed milliseconds later as a fake ~0s recovery and a fake rebindsAccepted: the two headline metrics this program is judged by, corrupted by the mechanism built to improve them. Migration arms nothing now; the [rel] migrate line is its field signal. Also: remaining counts flows genuinely still riding the exit (the pre-sweep subtraction counted stale book entries as stayers), and the seam wiring is pinned by source anchors -- deleting the two wiring lines previously failed no test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RouteOverride.Pin holds matching traffic to a single egress. For host rules that is the matched cluster; for app rules it is every flow the app owns, which is the case that needed new plumbing: a FlowOwnerLookupFunc seam (android: one getConnectionOwnerUid binder call per NEW flow, cached per flow key, never per packet) resolves the owning PINNED app, and all of that app's flows join one app:<id> affinity group. An app's api session and its cdn destinations then share one exit and one egress ip -- the fix for apps whose images fail to load behind a multi-exit vpn, where signed urls are issued against one ip and fetched from another. A pinned flow's inheritance also follows a benched donor for the whole episode rather than the follow window: a pin is a request for stability over everything short of removal. Warned donors still refuse -- a pin is not a license to board a retiring or unhealthy exit. Pin never changes routing (local/remote), only placement, and the zero value is off so rules stored before this field read back unpinned. Also adds doordash's cdn to the constellation table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…groups Five of the review's findings land here (2026-08-03): - the app group now REPLACES a pinned flow's domain groups instead of joining alongside them. A pinned flow that also joined youtube.com donated its app-chosen exit to unrelated youtube traffic, dragging strangers onto the pinned app's exit and oscillating a site's group between exits -- a regression of the one-site-one-egress-ip property inflicted on users who pinned nothing relevant. - the flow-owner cache is two TYPED maps, not a sync.Map: boxing the flow key into allocated on every egress packet, in a file that keeps that path allocation-free everywhere else. Escape analysis is clean now. - cached answers are never REFRESHED. The answer is consumed only at flow creation, so the old ttl refresh burned a platform call per long-lived flow per 5 minutes to compute a result nothing reads -- on the single-threaded tun reader. - answers are generation-tagged, so one landing after a rules change (the resolver runs unlocked, by design) cannot resurrect a removed pin. - the pinned follow window is bounded at 3x the ordinary window instead of 24h. Unbounded following was self-defeating: a soft verdict only executes against a flowless exit and a quarantine only lifts on one, so an endless stream of pinned flows kept a failing exit both un-executed and un-released. Plus the cross-version convergence the review found missing: the affinity maps are per-ip-version, so a dual-stack pinned app took one exit for v4 and another for v6 -- two egress ips, exactly what the feature exists to prevent. appPinClients records an app's placement across versions and donates it when the version's own group is empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The verification pass found two of the previous round's fixes did not hold up, both here: 1. The cross-version convergence never fired in the dual-stack case it was written for. An app's placement was recorded only when affinity inheritance produced it -- but an app's FIRST flow of each ip version is placed by the async RACE, which commits through bindClientFlow and recorded nothing. So v4 and v6 opening together (how a phone actually connects) still took two exits. The update now carries pinAppId and bindClientFlow records the placement, as does the rebind's assign, so a moved flow takes the app's canonical exit with it. 2. Removing the group contamination left the destination bridge (affinityFallbackIpPathsWithLock) outranking the app pin. The bridge places by destination ip, which on a shared cdn address is a STRANGER's exit -- and the record that followed then rewrote the app's canonical exit to the stranger's, dragging the whole app onto it. Worse than what it replaced, and the pinned user pays. The app donor is now consulted first and the bridge is skipped entirely for app-pinned flows; the record only ever captures a placement the app itself produced. Also from that pass: the donor honors the flow cap when sticky affinity is off (it silently ignored the A/B state), records clear when the pin rules change (they described a pin set that no longer exists, and held a strong reference to a dead channel), and the Pin docs in both repos stop claiming things the code does not do -- a host pin never formed a group, and no pin follows a bench without limit. The two lock sections in SetFlowOwnerLookup are deliberately not nested: flowOwnerLock is documented as a leaf and this would have been the only inversion in the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shipped per-app pinning with no field-observable signal, and it showed immediately: with three apps pinned on device, the rules were readable from disk and nothing had crashed, but nothing in a capture could say whether the android-to-go wiring engaged or whether the apps had actually converged. That is the one standing rule of this work, broken on the feature whose whole value is a behavior the owner cannot see directly. pin_lookup fires when the platform installs or clears the resolver -- it distinguishes 'no apps pinned' from 'the lookup never reached the go side', which is otherwise indistinguishable from silence. pins=<apps>/<exits> rides the heartbeat: apps with a placement, and distinct exits holding them. 3/1 is three pinned apps sharing an exit, 3/3 is three apps each on their own, 0/0 with rules present is the mechanism never engaging. AppPins() exposes the same for a readout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Field diagnosis 2026-08-03: a pinned app's 46 flows sat on an exit that went receive-silent at 19:27:10 and was not executed until 19:28:05 -- the full sustained-evidence window. The teardown was not the freeze; the 55-second HOLD was. The flows-are-sacred invariant protects an exit from a false positive, and it should, but nothing was protecting the flows from waiting out that protection. The resize pass's quarantine branch now runs the same migration the lifetime drain runs (G-3's migrateClientFlows), once per quarantine EPISODE. A quarantine is receive-silence by construction, so every flow on the exit is already getting nothing: established quic moves in about a packet interval, tcp cannot move and stays to finish or die with the exit exactly as before, and nothing is torn down. An acquitted exit keeps taking new flows -- a rebound quic flow is not harmed by having moved. This is the quarantine consumer I explicitly dropped when G-3 shipped, on the reasoning that a sustained bench is receive-dead anyway and the removal-time rebind already moves the quic at execution. That ignored the minute of hostage-taking before the execution, which is the part the user actually feels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four parallel reviews of the quarantine/migration area. The headline is that af76497 -- shipped an hour ago to fix a one-minute app freeze -- does not run at all, and would have been wrong if it did. DOES NOT RUN. The call sat in resize's quarantine branch, which is inside `if healthy` and `unhealthyDuration < StatsWindowWarnUnhealthy (5s)`, and behind the dialStarved branch. Whenever the resize goroutine is blocked in expand() -- WindowExpandTimeout 15s, and the field logs show `create client args expired` constantly -- the window closes and never reopens, because unhealthyDuration only grows and the latch only sets when the branch runs. The incident log proves it: its four "unhealthy removal demoted to warning" lines come from the UNHEALTHY branch, so the quarantine branch was never reached, and no event=migrate line appears anywhere. 34 connections died that the hand-off existed to save. The hand-off now runs from the VERDICT (detectBlackhole, right after setQuarantined succeeds, beside the resizeWake that was already there), through a channel seam. It cannot be preempted by a classification or lose a race with a blocked goroutine. The resize site stays as a backstop sharing the same per-episode latch. WOULD HAVE BEEN WRONG. Emptying a benched exit drops its flow count to zero, and verdictAction executes any soft verdict against a flowless exit -- inside one 1.25s poll -- while the flows just moved were the only traffic that could produce the receive ack that acquits it. So the rescue became the conviction, and the commit's claim that "a false positive is acquitted and keeps taking new flows" was false for exactly the all-quic exits the feature targets. verdictAction now takes emptiedByMigration: flowlessness WE caused falls through to the expiry bound instead of executing, so the episode still matures and a genuine recovery still releases it. Also from the same reviews: - the benched-exit rebind fallback was unreachable. It gathers only warned exits (the ordered offer is empty precisely because everything is warned), and usable() rejected warned exits -- so the fallback could never place one flow, and "0 rebound, 33 torn down" was structural. rebindFlowsWithLock gains a last-resort tier: after the strict passes, warned and over-cap candidates are admitted least-loaded-first, because a live suspect exit beats a destroyed connection. This also stops sticky affinity's own consequence -- the groups that grew past the cap are the ones no candidate had headroom for, so the biggest sites were split across three exits or torn down. - sendPacket's error path did a bare Store(nil) on a snapshot taken before the lock was released, clobbering a concurrent migration, leaving the flow booked on the replacement with a nil client (invisible to the idle reaper), and handing the app a teardown for a flow that had moved. Now a CompareAndSwap; a failed swap means someone else moved the flow and there is nothing to reset. - rebindFlowsWithLock recorded an app pin per assigned flow, so a split group left the app recorded on the LAST, least-preferred fragment -- the smallest one -- and pulled every later flow of the app onto it. First writer per app now wins. - event=migrate logged only successes, so a hand-off that moved nothing left no trace: the unfalsifiability trap this project keeps falling into. It is unconditional now and carries cause=bench|drain|action, movable=, and candidates=, which is what makes "the exit was benched and my app still hung" answerable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Logging only; no behavior changes. The question this answers: stable providers fail probes at times, and the owner's hypothesis is that the PHONE is the one that was absent -- asleep, backgrounded, or between networks. Every provider is reached through that same uplink, so a device that steps out fails every probe at once, and provider by provider that is indistinguishable from a bad pool. probe_sweep_result (new) is the discriminator, because correlation is the whole signal and a per-exit line cannot show it: [rel] event=probe_sweep_result passed=0 failed=6 silent=0 scheduled=6 uplinkstale=1 Emitted once when the last pass of a sweep lands, tallied by the passes themselves. passed=0 across the whole sweep with the uplink stale is one fact about the device; failures scattered across sweeps with the uplink fresh is a fact about providers. `silent` is a third outcome (a pass that asked nothing at all) that the pass/fail split would otherwise hide inside failures. The per-probe line gains the same discrimination at provider grain: recvage (seconds since THIS exit last received anything, -1 never), dns (hostnames it carried an answer back for), transport, flows, and uplinkstale. A provider with fresh receive and live flows that answers no probes is a target-side or egress-policy story; one whose last receive is minutes old is very likely simply gone. event=warn (new) closes the gap the last capture hit: 6 of 15 exits sat out of new-flow selection and nothing in the log could say why, because drain/starved/capacity only existed behind V(1), which is compiled off in the field. Now every cause transition logs from/to with flows, sources, and dial failures -- transitions only, since the resize pass rewrites this state every pass for every client. Salvage note: this batch started from work recovered after a crash. Two of its fields were kept only after being fixed. `sendage` read a lastSendAckTime field that does not exist (it did not compile), and is dropped rather than inventing a timestamp for it. `dns` was declared, documented and logged but never assigned -- it would have printed 0 on every line forever, and its own comment reads 0 as "the provider is not there", so it would have manufactured evidence for the very hypothesis under test. It is now threaded from the resolution stage where the count actually exists. Writing the tally test caught a real defect in the tally: `done == scheduled` fires again on any extra landing, so one sweep could report twice and read as two sweeps. Latched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Field capture 2026-08-04: providers on consumer devices leave the network mid-session. Exits that had answered 122-127/127 probes went to 0/127 -- including the dns resolution stage -- and stayed that way for 25+ minutes, while sitting in the window classified healthy, because a flowless corpse's idle stats look fine. Each one stayed SELECTABLE until real app traffic bound to it and ate the ~10-30s of dead syns that convicts; the capture ends with a wake-up burst convicting one in 10s, 10s during which those connections were black holes. ProbeSilenceWarnStreak (default 2, 0 off): after that many consecutive probe passes answered with total silence -- zero stage-B answers AND zero dns resolutions, so zero evidence of life across the whole target table -- the resize pass warns the exit out of new-flow placement with the new warnSilent cause, and the size math backfills a replacement, exactly like a dial-starved exit. Placement only, by construction: - probes stay non-punitive for removal, which stays traffic-based - any evidence of life acquits: a probe answer or a dns resolution clears the streak at the recording site, and return traffic newer than the latest silent pass clears it at the read site -- a provider that was merely asleep is warned for a few minutes, never executed - gated like starvation on having somewhere else to go: the sole exit of a window is never warned into a dead end - ordered after the quarantine branch, so a quarantined exit that is also probe-silent still runs its bench-time migration hand-off Observability: the probe line gains streak=, and the existing event=warn line reports the transition (to=silent). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The field-validated reliability stack (flows-are-sacred phases A-G, provider qualification prober, per-app/site exit pinning, provider-churn placement compensation, forensic [rel] observability) merged onto upstream main. Reconciliations against upstream's concurrent work, described in the PR: - ip_remote_multi_client.go carries our implementation, with upstream's batch-receive fast path (cd87ea5) reimplemented on it -- including a probe-interception guard the fast path needs here -- and upstream's icmp echo gate, removal receive queue, network-peer destination, and sparse send-drop counters ported in - upstream's borrowed-IpPath zero-alloc committed-ingress optimization is NOT carried (one test skipped); flow paths are defensively owned at update creation instead - upstream tests of replaced internals (DegradedMode/CPing-busy-stale, ping admission, generator-deadline API, transport migrator control gate, reconcile/reset-budget internals) are removed or adapted; the equivalent behaviors are covered by this stack's own suites - both sides' opening-contract fixes kept: mib(1) initial and the network-peer initial Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hardening Suite-driven fixes after the merge commit: - tun.go is upstream's file untouched (a merge-side inversion had taken the pre-GRO copy) - contract sizing defers to upstream's escrow policy: kib(16) public initial + the network-peer mib(1); the fork's mib(1)-for-all bump and its test stay fork-side - OrderedClients gains the fixed-destination fallback: a warned sole network peer is still offered (the race-level benched fallback's judgment, applied at this seam too) - a refused tcp dial still answers RST+ACK; the dial-rejection test now asserts the classified icmp dial-failure signal for other errors - lifecycle hardening ported from upstream's stall suite: Close detaches retired owner references, the stale-flow teardown deletes only its own generation, a closed multi-client rejects new flow generations, the platform deregistration precedes the (possibly Pion-slow) local client teardown and channel Close no longer waits behind it - source anchors updated for the factored receive path (probe intercept pinned on BOTH the per-packet and batch dispatches) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A cross-checking review of the checkpoint found three regressions of upstream's newest work that test deletion had masked, plus a leak: - server-initiated resident migration restored: the MultiClientGeneratorTransportMigrator capability, the channel's transportMigrator wiring, and the control-source-gated TransferResidentMigrate handling in clientReceive (with its test back); ApiMultiClientGenerator.MigrateClientTransport is live again - SetPerformanceProfile equivalence guard restored (performanceProfilesEqual): re-applying an equal profile no longer shuffles every window client (three upstream tests back) - windowMinSatisfied restored and wired into the resize monitor events: a fixed destination counts warned clients toward the minimum, so a warned sole selected peer does not report "connecting" for its whole session (upstream test file back) - connectctl defaults restored to the public endpoints (the fork's test server had leaked into the diff) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Acquiring a contract blocks the send sequence, and web traffic restarts every new destination at sequence 0, so the kib(16) opener (about nine usable packets) cost every new domain two blocking negotiations before its second contract. A provider log measured ~40 opening acquisitions in 13 minutes across ten destinations at 80ms-2.4s each -- a several-second stall on a new domain. mib(1) covers essentially any single web response in the opening contract. The tradeoff is a larger pre-settlement exposure to an unproven peer, bounded by the unchanged mib(128) ceiling; the escrow tests now pin the new policy (policy-relative asserts), and the coverage test for the opener returns. Self-contained and easy to drop if the small opener is preferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The three installer/manager scripts fetched releases and self-updates from Ryanmello07/connect. Upstream they must use urnetwork/connect. This is a correctness fix as much as a link fix. All three already select the release asset by the `urnetwork-provider-` name prefix, which is what UPSTREAM publishes (urnetwork-provider-<version>.tar.gz under v* tags). The fork publishes a single `miner.tar.gz` under a `beta-custom-server-latest` tag, which none of those selectors match -- so pointed at the fork the scripts could not resolve an asset at all. Verified against the live upstream API: the latest release resolves to urnetwork-provider-2026.3.23-895075980.tar.gz, matching the prefix all three scripts look for, and both the API and raw.githubusercontent URLs return 200. Also dropped the "Beta fork: installs the custom client build from Ryanmello07/connect" banner line, which has no meaning upstream.
…eckpoint-upstream reliability checkpoint: the flows-are-sacred stack, field-validated
…idth during control-API outages When the control API is unreachable, providers keep creating contracts, expanding client windows and retransmitting against an endpoint that cannot authorize anything. The work has nowhere to go, so it is spent bandwidth. On metered links a sustained outage can consume a large share of a monthly allowance while serving no client. Add a shared degradation signal driven by the two round-trips that actually touch the backend (platform auth and contract OOB). It requires backendDegradedFailThreshold consecutive failures with no intervening success, and the last failure must be within backendDegradedWindow, so isolated timeouts on a busy provider never trip it. Any successful round-trip clears it immediately, so recovery is not on a timer. While degraded: - skip CreateContract; every request is an OOB round-trip that cannot succeed - start contract retries at the backed-off interval instead of the fast first retry, composing with the existing nextCreateContractRetryInterval backoff - do not expand the multi-client window; each added client needs its own contract Also rate-limit the four log lines that flood under the same fault ([t]auth error, [contract]oob err, [ts]->error, [r]drop) to one per minute with a suppressed count, via a small shared logThrottle. These are emitted per sequence per retry, so during an outage they are the dominant log volume and can push out the lines needed to diagnose it. Each falls back to -v=1 so no detail is lost when the level is raised.
📝 WalkthroughWalkthroughThis PR adds reliability controls across transfer, IP, multi-client, probing, transport, and TUN paths. It also adds extensive tests for degradation, packet signaling, flow migration, provider qualification, observability, reconnect behavior, and recovery metrics. ChangesTransfer and packet signaling
Multi-client reliability
Transport and runtime support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
Closing: opened against a stale fork main, so the diff swept in ~78 upstream commits from urnetwork#190 that are not part of this change. Fork main is now synced to 9dc9531. Reopening as a fresh PR so the diff is just the 7 files that went upstream. |
full-bars
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
net_http_doh.go (1)
870-883: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrefer a completed fresh result before serving stale data.
At Lines 874-879, both
fl.doneandctx.Done()can be ready. Go can select the canceled-context branch and returnstaleAddrseven when the flight already completed with fresh records or authoritative NXDOMAIN.Before serving stale data in the cancellation branch, check
fl.doneagain without blocking. Add a concurrent regression test for this race.Proposed fix
case <-ctx.Done(): + select { + case <-fl.done: + return fl.addrs, fl.authoritative + default: + } if 0 < len(staleAddrs) { return serveStale() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net_http_doh.go` around lines 870 - 883, Update the !leader resolution wait around fl.done and ctx.Done so the cancellation branch performs a non-blocking check of fl.done before serving staleAddrs, returning the completed flight’s fresh addresses and authoritative status when available. Preserve existing stale and cancellation behavior when the flight has not completed, and add a concurrent regression test covering this race.ip_remote_multi_client.go (1)
11062-11068: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the blackhole poll interval against a zero
BlackholeTimeout.The loop paces itself with
time.After(self.settings.BlackholeTimeout / 4). Every other timeout in this file treats 0 as "disabled", andBlackholeReceiveTimeoutdocuments 0 as disabling its check. If an operator or a fixture setsBlackholeTimeoutto 0 to disable the send-bound verdict, this timer becomes zero-duration and the goroutine spins, callingWindowStats(which coalesces stat buckets under the channel lock) as fast as the scheduler allows. That burns a core per channel and contends the receive path.🐛 Proposed fix: floor the poll interval
+ pollTimeout := self.settings.BlackholeTimeout / 4 + if pollTimeout <= 0 { + pollTimeout = time.Second + } select { case <-self.ctx.Done(): return case <-self.client.Done(): return - case <-time.After(self.settings.BlackholeTimeout / 4): + case <-time.After(pollTimeout): }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client.go` around lines 11062 - 11068, Update the blackhole polling loop around self.settings.BlackholeTimeout and time.After so a zero BlackholeTimeout cannot produce a zero-duration timer or busy loop. Apply the existing timeout-disabled semantics by using a positive fallback poll interval when the configured value is zero, while preserving cancellation through self.ctx.Done() and self.client.Done().
🧹 Nitpick comments (22)
ip_remote_multi_client_observability.go (2)
451-465: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment does not match the behavior; rendered values are escaped a second time.
relEventalways passes each value throughrelValue, so the already-renderedchange.fromandchange.toare re-quoted. For the current bool and duration fields the output is unchanged, so there is no visible defect today. If a string field is added toReliabilitySettings, a value containing a space renders asfrom="\"a b\""in the diff line but askey="a b"in the banner, and the two lines stop agreeing.Either correct the comment, or build the line directly so the values pass through unmodified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_observability.go` around lines 451 - 465, Update relSettingsDiffLines so rendered change.from and change.to values are not passed through relEvent and relValue for a second escaping; build the setting diff line directly while preserving the existing field/from/to output and matching banner rendering.
740-762: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe semaphore is per call, not shared with the prober loop.
The comment states that the passes run "behind the same bounded semaphore the prober loop uses".
semis allocated insideProbeAllExits, so each invocation gets its own budget ofproberConcurrency. Repeated taps of the developer-menu button, or a tap that overlaps a background sweep, put more concurrent probe traffic on the wire than the stated bound.Either share the client-level semaphore with
runProber, or correct the comment to describe a per-invocation bound.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_observability.go` around lines 740 - 762, Update ProbeAllExits and the related runProber coordination so both paths use the same client-level semaphore, rather than allocating sem inside each ProbeAllExits invocation. Preserve teardown cancellation and semaphore release behavior, and ensure overlapping manual probes and background sweeps share the proberConcurrency limit.transfer_test.go (1)
960-982: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the post-fork assertions.
Two points on this block:
- The
selectonplain.ctx.Done()only proves that the plain lane was not retired before this instant. If the intermediary fork retires the predecessor asynchronously, this check passes even for a broken implementation. The same applies to the direct force-stream sequence, whose retirement is asserted only indirectly throughviaCount == 1.- The block no longer asserts the total per-destination sequence count. After the multi-hop send, the destination should hold exactly two sequences: the plain lane and the intermediary force-stream lane. Adding that assertion catches a leaked direct force-stream sequence explicitly instead of relying on the force-stream-only filter.
♻️ Proposed additional assertion
if viaCount != 1 { t.Fatalf("force-stream lane sequences = %d, want the intermediary replacement only", viaCount) } + total := 0 + for id := range client.sendBuffer.sendSequences { + if id.Destination == destination { + total += 1 + } + } + if total != 2 { + t.Fatalf("peer sequences = %d, want the plain lane plus the intermediary force-stream lane", total) + } }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transfer_test.go` around lines 960 - 982, Strengthen the post-fork assertions around sendSequences: wait for the asynchronous retirement of the original direct force-stream sequence and assert that plain.ctx remains active, rather than checking Done only once. Also count all sequences for destination and require exactly two total entries—plain plus the intermediary force-stream lane—while retaining the existing intermediary-count validation.transfer_contract_size_test.go (1)
46-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the usable fraction from
ContractFillFractioninstead of hard-coding0.8.
0.8duplicatesSendBufferSettings.ContractFillFraction. If that default changes, this test keeps passing while its stated premise ("~80% of the contract is usable") no longer holds.♻️ Proposed refactor
- // ~80% of the contract is usable, so compare against the usable budget - usable := ByteCount(float64(settings.InitialContractTransferByteCount) * 0.8) + // only ContractFillFraction of the contract is usable, so compare against + // the usable budget + fillFraction := DefaultSendBufferSettings().ContractFillFraction + usable := ByteCount(float32(settings.InitialContractTransferByteCount) * fillFraction)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transfer_contract_size_test.go` around lines 46 - 47, Update the usable budget calculation near the `usable` variable to derive the fraction from `SendBufferSettings.ContractFillFraction` rather than hard-coding `0.8`; preserve the existing multiplication by `settings.InitialContractTransferByteCount` and ByteCount conversion.backend_degraded_test.go (1)
143-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test hard-codes a three-failure streak against
backendDegradedFailThreshold.
TestBackendDegraded_StreakSurvivesGapInsideWindowperforms exactly threenoteBackendFailurecalls and then asserts the count equalsbackendDegradedFailThreshold. If the threshold changes, the assertion fails for a reason unrelated to the behavior under test. Drive the loop from the constant instead.♻️ Proposed refactor to derive the streak from the constant
- noteBackendFailure() - noteBackendFailure() - // A gap well inside the window: still the same outage. - lastBackendFailNano.Store(time.Now().Add(-backendDegradedWindow / 2).UnixNano()) - - noteBackendFailure() + for i := 0; i < backendDegradedFailThreshold-1; i++ { + noteBackendFailure() + } + // A gap well inside the window: still the same outage. + lastBackendFailNano.Store(time.Now().Add(-backendDegradedWindow / 2).UnixNano()) + + noteBackendFailure() if got := consecutiveBackendFails.Load(); got != backendDegradedFailThreshold { t.Fatalf("consecutive failures = %d, want %d (streak reset inside the window)", got, backendDegradedFailThreshold) } if !isBackendDegraded() { - t.Fatal("not degraded after 3 failures spanning a gap inside the window") + t.Fatal("not degraded after a full streak spanning a gap inside the window") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend_degraded_test.go` around lines 143 - 160, Update TestBackendDegraded_StreakSurvivesGapInsideWindow to perform the failure streak using backendDegradedFailThreshold rather than exactly three noteBackendFailure calls. Preserve the intentional gap before the final failure and keep the existing count and degraded-state assertions.transfer_contract_prewarm_test.go (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestPrewarmOpeningContractCanBeDisabledasserts only the value it just assigned.The test sets
PrewarmOpeningContract = falseand then asserts the field isfalse. It never callsprewarmOpeningContract, so it cannot detect a regression in the disabled path.No test in this file exercises
prewarmOpeningContractitself. That gap hides the two behaviors that matter: theSendNoContractearly return, and theContractKeythe prewarm request uses. The key is currently wrong for network peers — see the comment ontransfer.golines 6721-6729.Add a test that calls
prewarmOpeningContractwith the flag off and asserts no contract request is issued, and one that calls it with the flag on and asserts the requestedContractKeymatches the keyupdateContractlater uses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transfer_contract_prewarm_test.go` around lines 15 - 22, Replace the value-only assertion in TestPrewarmOpeningContractCanBeDisabled with coverage of prewarmOpeningContract: configure the disabled flag, invoke it with a SendNoContract request, and assert no contract request is issued. Add an enabled-path test that invokes prewarmOpeningContract and verifies the request’s ContractKey matches the key produced by updateContract, including network peers.transfer_lane_test.go (1)
177-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fixed count of four contract results per lane is coupled to the contract-size defaults changed in this PR.
Each lane sends
n= 48 messages. The test feeds exactly four contract results per lane. The number of contracts a lane consumes depends onInitialContractTransferByteCountand the ramp, and this PR changes that default. If the ramp shrinks later, the test fails with "lane starvation", which does not name the real cause.Consider serving contract results on demand, or add a comment that states why four is sufficient for
nmessages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transfer_lane_test.go` around lines 177 - 194, Decouple contract-result provisioning in the lane test from the hard-coded four iterations in the HandleControlFrame setup. Ensure each lane receives contract results on demand, or document and derive the sufficient count from the lane’s 48-message workload and contract-size settings so future ramp changes cannot cause misleading lane-starvation failures.transfer.go (1)
3396-3399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe 50ms contract-wait log is unthrottled at default verbosity.
This PR throttles
[contract]oob errand[r]dropbecause an outage makes them per-sequence and per-retry. This log has the same shape. While the backend is degraded, every sequence blocks forCreateContractTimeoutand then emits one line at default verbosity. Opening acquisitions are documented at ~260ms, so the line also fires on every new destination in normal operation.Apply the existing
logThrottlehere, or keep the unconditional line only above a higher threshold and log the 50ms band atV(1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transfer.go` around lines 3396 - 3399, Throttle the contract-wait log in the flow around addContractWaitTime so the 50ms-and-above message does not emit unconditionally at default verbosity. Reuse the existing logThrottle mechanism, or move this band to V(1) while retaining unconditional logging only for longer waits; preserve the current message fields and wait-time accounting.transfer_encrypt.go (1)
1614-1678: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCap the resend interval by the remaining window, not the whole window.
interval = min(2*interval, window)lets the interval reachTlsTimeout. After that growth the next wake can occur after the deadline, so the loop performs one extra sleep and then returns without sending. The behavior is safe, but a cap against the remaining time todeadlinekeeps the last resends inside the window.Also consider a reused
time.Timerinstead oftime.Afterper iteration, matching the timer-reuse pattern used elsewhere in this repository.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transfer_encrypt.go` around lines 1614 - 1678, Update resendIdentityProofForEstablishment so the backoff interval is capped by the remaining duration until deadline rather than the full window, preventing a final sleep from extending past the establishment window; preserve the existing deadline check and resend behavior. Replace the per-iteration time.After call with a reusable time.Timer, following the repository’s established timer-reset and cleanup pattern.transport_platform_test.go (1)
366-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the doc comment with what the test asserts, and drop the duplicate Close.
Two small points:
- The comment states the re-dial "takes the reconnect fast path (hadConnection semantics)". The test asserts only that the re-dial beats the 60s backoff and that H1 is re-elected. It does not observe the fast path. Either remove that clause or assert on the fast-path slot state.
testingPlatformTransportalready registerst.Cleanup(transport.Close), sodefer transport.Close()on Line 378 closes the transport twice.♻️ Proposed cleanup
// TestKickSkipsDialFailureBackoff: a kick that arrives while the transport is -// waiting out a failed-dial backoff re-dials immediately instead of waiting, -// and the re-dial takes the reconnect fast path (hadConnection semantics). +// waiting out a failed-dial backoff re-dials immediately instead of waiting. func TestKickSkipsDialFailureBackoff(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() platform := newTestingPlatformServer(t) settings := testingPlatformTransportSettings() // make the backoff long enough that only a kick can plausibly beat it settings.ReconnectTimeout = 60 * time.Second transport := testingPlatformTransport(t, ctx, platform.url, settings) - defer transport.Close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transport_platform_test.go` around lines 366 - 378, Update TestKickSkipsDialFailureBackoff’s doc comment to describe only the observed immediate re-dial and H1 re-election, removing the unverified “reconnect fast path (hadConnection semantics)” clause. Remove the redundant defer transport.Close() because testingPlatformTransport already registers cleanup.trace_test.go (1)
43-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
returnedflag; it cannot be false.Line 51 sets
returned = trueunconditionally, so the check on Line 54 is dead. The comment on Line 52-53 already states the real assertion correctly: reaching the end of the function is the test. Keep the comment and delete the flag.♻️ Proposed simplification
var wg sync.WaitGroup wg.Add(1) - returned := false HandleError( func() { defer wg.Done() panic(errors.New("boom")) }, wg.Done, ) - returned = true // reaching here at all is the assertion: the pre-fix behavior was a // process-killing repanic out of HandleError's recover - if !returned { - t.Fatal("unreachable") - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trace_test.go` around lines 43 - 56, Remove the returned flag and the unreachable if !returned assertion from the test around HandleError. Keep the existing comment explaining that reaching the end verifies HandleError does not repanic, so the test succeeds by completing normally.tun_test.go (1)
621-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
Tun.WriteBatchin the bridge
Tun.WriteBatchhas no test or production call site. UpdatebridgeTunBatchto calldst.WriteBatch(packets[:n])and add focused assertions for GRO, shard locking, and checksum offload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tun_test.go` around lines 621 - 631, Update bridgeTunBatch to invoke dst.WriteBatch(packets[:n]) for each batch instead of bypassing the batch API. Add focused tests covering GRO behavior, shard locking, and checksum offload through this bridge path, preserving existing transfer and throughput assertions.ip_remote_multi_client.go (3)
3730-3754: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing the last-resort mode explicitly instead of mutating a captured flag.
usableandheadroomread the capturedlastResortvariable, which the last-resort pass flips at line 3904 and restores at line 3925. The predicates therefore behave differently depending on hidden state that is set 150 lines away. A reader who auditsusablein isolation cannot tell which rule is active.Passing the mode as a parameter (
usable(c, lastResort)andheadroom(c, lastResort)) would make each call site state its own rule, and would remove the need for the restore at line 3925.Also applies to: 3896-3926
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client.go` around lines 3730 - 3754, The captured lastResort state should not control usable and headroom implicitly across distant code. Update usable and headroom to accept the last-resort mode explicitly, pass the appropriate value at every call site including the last-resort pass, and remove the mutation and restoration of lastResort around that pass.
2069-2113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a compile-time or reflective guard against
ReliabilitySettingsFromfield drift.
ReliabilitySettingsFromcopies about thirty fields by hand. A field added toReliabilitySettingsbut omitted here reads as the zero value, which silently disables the knob on every override write. The current defense is one assertion per knob spread across several test files, so a new knob has no automatic coverage.A reflective test that walks
ReliabilitySettingsfields and asserts each one is non-zero afterReliabilitySettingsFrom(DefaultMultiClientSettings())would catch the omission for every future field at once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client.go` around lines 2069 - 2113, Add a reflective test covering ReliabilitySettingsFrom that iterates every field in ReliabilitySettings and verifies the corresponding value is non-zero when called with DefaultMultiClientSettings(). Use this guard to detect any field omitted from the manual mapping, while preserving the existing per-knob tests and nil-settings behavior.
2880-3195: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the shared body of the v4 and v6 branches in
sendUpdate.The two branches are about 150 lines each and differ only in the key type and the two path maps. They now carry identical logic for the teardown goroutine, affinity joining, the app-pin donor, the destination bridge, and the group ledger. The comments already acknowledge this by pointing at "the v4 twin" five times.
A generic helper parameterized over the key type (or a small interface over the two map pairs) would make a divergence between the twins impossible. This is a large mechanical change, so it is reasonable to defer it out of this mirror PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client.go` around lines 2880 - 3195, Defer this refactor; no code change is required for the current PR. If addressed later, extract the duplicated sendUpdate v4/v6 branch logic into a shared helper parameterized by the protocol-specific key and affinity map types, preserving the existing teardown, affinity joining, app-pin donor, destination bridge, and ledger behavior.ip_remote_multi_client_stall_test.go (1)
1218-1218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider naming the nil callback arguments.
Nine consecutive bare
nilarguments map positionally toreliabilitySettingsFunc,uplinkGateFunc,reliabilityMetricsFunc,flowCountFunc,resizeWakeFunc,migrateFlowsFunc,providerQualifiedFunc,receivingSiblingsFunc, andqualificationRefreshFunc. If the constructor's parameter order changes, this call still compiles and the test wires a different seam than intended.One
nilper line with a trailing comment naming the parameter would make a reorder visible in review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_stall_test.go` at line 1218, The constructor call in the test should make the nine callback arguments explicit by placing each nil on its own line with a trailing comment naming reliabilitySettingsFunc, uplinkGateFunc, reliabilityMetricsFunc, flowCountFunc, resizeWakeFunc, migrateFlowsFunc, providerQualifiedFunc, receivingSiblingsFunc, and qualificationRefreshFunc, preserving their current order.ip_remote_multi_client_bind_flow_test.go (1)
638-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making this source anchor less dependent on exact indentation.
The block is delimited by
strings.Index(block, "\n\t\t\t\t}"), which encodes a specific nesting depth indetectBlackhole. If that block is re-nested — for example by wrapping it in one moreif— the slice truncates at the wrong place and the test fails with "the verdict does not run the hand-off", which points at the wrong cause.Matching on the enclosing statement text, or asserting that
migrateFlows()appears aftersetQuarantined(reason)and before the} else {that follows it, would keep the intent without pinning the indentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_bind_flow_test.go` around lines 638 - 648, Update the source inspection in the detectBlackhole test around the setQuarantined anchor so block extraction does not depend on the exact indentation of its closing brace. Instead, bound the assertion using stable statement text—such as requiring migrateFlows() after setQuarantined(reason) and before the following } else {—while preserving the existing hand-off validation.ip_remote_multi_client_generator_deadline_test.go (1)
98-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the deadline assertions.
These assertions allow a call to block for almost five seconds. That permits a large regression beyond the configured 50 ms deadline or canceled-context path. Use a CI-tolerant bound near the configured deadline, such as one second.
Proposed test change
- if elapsed := time.Now().Sub(startTime); 5*time.Second < elapsed { + if elapsed := time.Since(startTime); time.Second < elapsed {Also applies to: 130-132, 154-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_generator_deadline_test.go` around lines 98 - 100, Replace the five-second elapsed-time thresholds in the deadline assertions of the relevant test cases with a CI-tolerant bound near the configured 50 ms deadline, such as one second. Update the checks around the affected abandon and canceled-context paths while preserving their existing failure messages and deadline behavior.ip_remote_multi_client_rotation_test.go (1)
313-315: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the
"} else"lookup before slicing.
strings.Index(rest, "} else")returns-1when the marker is absent. Line 315 then slicesrest[:-1]and panics. The neighbouring helpernextWarningCallalready fails witht.Fatalfin the same situation. A refactor ofresizethat renames or reformats the branch would produce a panic stack instead of the intended diagnostic message.🛡️ Proposed guard
drainAt := strings.Index(body, `printStats("client drain")`) + if drainAt < 0 { + t.Fatal("could not find the drain branch in resize") + } rest := body[drainAt:] - drainBranch := rest[:strings.Index(rest, "} else")] + elseAt := strings.Index(rest, "} else") + if elseAt < 0 { + t.Fatal("could not find the end of the drain branch") + } + drainBranch := rest[:elseAt]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_rotation_test.go` around lines 313 - 315, Guard the strings.Index lookup for the "} else" marker in the drainBranch extraction, and call t.Fatalf with a clear diagnostic when the marker is absent before slicing rest. Follow the existing failure behavior used by nextWarningCall, while preserving the current drainBranch extraction when the marker is found.ip_remote_multi_client_rebind_test.go (1)
203-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
MaxFlowsPerExiton the fixture settings instead of replacingparent.settings.
rebindTestParentbuilds its own settings object and setsQuicRebindOnExitLoss = rebindon it. Line 207 replaces that object with a freshDefaultMultiClientSettings(), so the explicit toggle is discarded and these two tests now depend on the shipped default value ofQuicRebindOnExitLoss. If the default ever flips tofalse, the rebind assertions fail with a misleading message instead of testing the intended behavior. The same pattern appears at lines 334-338.A fixture option that mutates the settings the parent already holds keeps the toggle explicit:
♻️ Proposed adjustment
- settings := DefaultMultiClientSettings() - settings.MaxFlowsPerExit = 1 - fullCandidate := rebindTestCandidate(settings) - parent, dying, forwarded, _ := rebindTestParent(t, true, []*multiClientChannel{fullCandidate}) - parent.settings = settings + parent, dying, forwarded, _ := rebindTestParent(t, true, nil) + parent.settings.MaxFlowsPerExit = 1 + fullCandidate := rebindTestCandidate(parent.settings) + parent.rebindCandidatesFunc = func(*multiClientChannel) []*multiClientChannel { + return []*multiClientChannel{fullCandidate} + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_rebind_test.go` around lines 203 - 207, Update the test setup around rebindTestParent and the matching fixture at lines 334-338 to mutate the parent’s existing settings by setting MaxFlowsPerExit to 1, rather than replacing parent.settings with a new settings object. Preserve the explicit QuicRebindOnExitLoss value configured by rebindTestParent.ip_remote_multi_client_prober.go (1)
641-653: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAn early-returning pass suppresses the sweep-result line for the whole sweep.
sweepTally.recordruns only at line 653. The two early returns before it — the teardown case at line 644 and theclient.IsDone()case at line 650 — return without recording.self.donethen never reachesself.scheduled, sorecordnever returnsokand theprobe_sweep_resultline is never emitted for that sweep.One exit that dies between the plan and its turn on the semaphore is enough. The file comment describes this line as the only view of correlated failure, so losing it on the exact device conditions the mechanism targets (a sleeping phone dropping exits) removes the diagnostic when it matters most.
Recording those returns as
silentkeeps the sweep accountable:🐛 Proposed fix sketch
select { case sem <- struct{}{}: case <-self.ctx.Done(): return } defer func() { <-sem }() + result := probeResult{} if client.IsDone() { - return + // a pass that never ran asked nothing: the silent bucket + } else { + result = self.probeProviderPass(client) } - result := self.probeProviderPass(client) if line, ok := sweepTally.record(result); ok {The teardown return at line 644 can stay unrecorded, because the parent is closing and no line is expected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_prober.go` around lines 641 - 653, Update the client.IsDone() early-return path in the sweep worker to record a silent result through sweepTally.record before returning, ensuring the sweep still reaches self.scheduled and can emit probe_sweep_result. Leave the semaphore-acquisition teardown return guarded by self.ctx.Done() unchanged and unrecorded.ip_remote_multi_client_prober_test.go (1)
264-267: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe 1s
ProbeTimeoutgives stage B a hard budget on a loaded runner.
ProbeTimeoutbounds both stages. Stage A must expire before the fallback runs, and then stage B has the same 1s to receive every literal SynAck. If the deadline fires first,probeExitunregisters the probe flows at line 667 ofip_remote_multi_client_probe.go, so the answers this test delivers afterwards match nothing andresult.Passedis false. The failure would read as a broken fallback rather than as a slow runner.The polling in
waitForProbeFlowsplus in-process packet handling normally finishes in microseconds, so this is a tail risk rather than a routine failure. A larger timeout costs the test only the extra stage-A wait it already pays:♻️ Proposed adjustment
- parent.settings.ProbeTimeout = 1 * time.Second + parent.settings.ProbeTimeout = 2 * time.Second🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ip_remote_multi_client_prober_test.go` around lines 264 - 267, Increase parent.settings.ProbeTimeout in this test to provide sufficient budget for fallback stage B on loaded runners, while preserving the requirement that stage A times out before fallback begins. Keep the wait-for-fallback behavior and packet-answer flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend_degraded_test.go`:
- Around line 12-15: Update resetBackendDegraded and the affected tests to wait
for all clients, transports, and out-of-band callbacks to finish before
resetting or asserting the process-wide backend counters. Do not rely on
Client.Cancel alone; synchronize completion of in-flight CreateContract
callbacks, including noteBackendSuccess, before calling resetBackendDegraded or
checking counter values.
In `@ip_packet_icmp_parse_test.go`:
- Around line 127-132: Update the test setup around ipOosUnreachable to capture
and validate its ok result before indexing packet; fail the test clearly and
return when construction fails, then retain the existing ICMP type mutation and
rejection assertion for successful builds.
In `@ip_remote_multi_client_busy_probe_test.go`:
- Around line 180-198: Update TestBusyProbeUnsendableRunResetsBetweenEpisodes to
add a receivingSibling() to the busyProbeTestWindow fixture, ensuring
convictSendStalls evaluates the unsendable probe result instead of the
sibling-corroboration hold path. After the first convictSendStalls assertion,
verify busyProbeSendFailures is nonzero, then keep the addSendAck(1440) and
final zero-value assertion to confirm the completed episode resets the failures.
In `@ip_remote_multi_client_observability.go`:
- Around line 115-151: Update relValue to safely handle typed nil values before
invoking Error or String on the error and fmt.Stringer cases. Add a
reflect-based relIsNilPointer helper covering nil-capable kinds, and use it to
return the existing nil representation instead of calling methods on typed nil
pointers, preserving relValue’s never-panic contract.
In `@ip_remote_multi_client_probe.go`:
- Around line 1160-1173: Update the early-return paths in sendProbe and
SendDetailedWithAck to return pooled legacy frame buffers before exiting: when
frame.Raw is false, call MessagePoolReturn(frame.MessageBytes), then preserve
the existing stalled and client == nil return values. Keep the default v2 Raw:
true behavior unchanged and apply the guard only to these identified early
returns.
In `@ip_remote_multi_client.go`:
- Around line 4972-4986: Update NotifyNetworkChanged to reset uplinkCapLogged
while closing the uplink epoch, matching the reset performed by
notifySchedulerPause. Keep the existing uplink state updates and network-change
notification behavior unchanged.
- Around line 11789-11792: Add a nil check for ipPacketFromProvider.IpPacket
immediately after the type assertion in the FromFrame success path before
accessing PacketBytes, and handle the nil case by skipping or safely rejecting
the frame. Preserve processing for frames with a non-nil IpPacket.
- Around line 4459-4536: Remove the duplicate MessagePoolReturn call in the
failed-send path around client.SendWithAck: retain the existing !sent cleanup
and eliminate the later else branch that returns p.packet again. Ensure each
failed race attempt releases the shared packet exactly once, or route the
attempt through sendMultiClientRaceAttempt if that helper is the established
ownership mechanism.
In `@ip.go`:
- Around line 1843-1849: Update the UDP dial-failure handling around
classifyDialFailure to suppress receivePacket when self.ctx.Err() is non-nil,
matching the TCP teardown behavior; only send the unreachable signal for active
contexts, then return without generating output during cancellation.
In `@log_throttle_test.go`:
- Around line 82-97: Update the concurrent emission collection in the test
around the allowed channel to retain and sum each winner’s reported suppressed
count instead of discarding the received values. Add that accumulated count to
the later suppressed value returned by th.Allow, then assert the combined total
equals callers-1 so the check is independent of report timing.
In `@net_http_doh.go`:
- Around line 837-843: Update the stale-result handling around QueryResult and
staleUsable so staleAddrs contains only addresses whose individual expiration
remains within dohStaleServeBound; do not rely on the result’s latest expiration
alone. Use this filtered address set for stale serving and cache pruning,
preserve normal fresh-result behavior, and add a test covering a result with
mixed address TTLs where the older address is excluded.
In `@protocol/transfer.proto`:
- Around line 549-562: Update DeliverEncryptedControl to validate every
non-empty epoch_id before epoch routing, using ULID validation stronger than
IdFromBytes’ length-only check; reject malformed or non-canonical values instead
of treating them as unset, while preserving empty epoch_id as the legacy
behavior.
In `@reliability_metrics.go`:
- Around line 370-386: Correct the doc comment for
reliabilityMetrics.destinationReachable to state that pending is checked only
after acquiring pendingLock; remove the claim that the common empty case
performs a lock-free racy check. Leave the locking and function behavior
unchanged.
In `@transfer_contract_manager.go`:
- Around line 293-294: Update the documentation for
InitialNetworkPeerContractTransferByteCount to reflect that its default now
matches InitialContractTransferByteCount at mib(1). Remove or revise the stale
statements describing network contracts exhausting 16 KiB and public/friends
streams retaining a smaller initial contract, while preserving any
still-accurate behavior details.
In `@transfer_encrypt.go`:
- Around line 2056-2106: Update receivePeerIdentityProofForEpoch so a late proof
cannot promote an epoch canceled by establishmentTimeoutWatcher. Before clearing
identityFailed or evaluating the proof, detect that e has been canceled or its
outboxLoop control path is no longer active and reject the proof or create a
fresh epoch instead; ensure Cipher() never exposes AEAD state from the canceled
e.
In `@transfer_lane_test.go`:
- Around line 226-229: Update the ctx.Done() branch in the receive loop of the
transfer lane test to fail the test instead of returning successfully when
cancellation occurs before all expected messages are received. Use the test’s
existing failure mechanism, then preserve the receive path and subsequent
sequence-map assertions.
In `@transfer.go`:
- Around line 6721-6729: Update the ContractKey construction in
prewarmOpeningContract to include NetworkPeer: self.networkPeer, matching the
keys used by updateContract and TakeContract. Also pass self.networkPeer to the
contract-size calculation so contractByteCount requests the correct size for
network-peer contracts.
- Around line 5495-5553: Bound the synchronous receiveCallback work in
ReceiveSequence.flushDeliver so acknowledgment delivery cannot be delayed beyond
the configured AckTimeout, or add a saturation test that explicitly enforces the
60-second limit. Preserve deliver-before-ack ordering and ensure the test
exercises a callback lasting past the timeout without allowing sender expiry and
retransmission.
In `@tun_test.go`:
- Around line 675-691: Update both the sender loop and the receiver loop around
the visible received-drain logic to check ctx before each bounded read or write
iteration. If the context is canceled, report the context error through the
existing error channel and return immediately, ensuring the overall test timeout
interrupts slow-but-progressing transfers instead of waiting for per-step
deadlines.
In `@tun.go`:
- Around line 798-804: Update the single-entry fast path in Tun.WriteBatch so
non-IPv4 packets follow the batch behavior and are silently skipped rather than
returning syscall.EAFNOSUPPORT. Preserve normal writing for supported packets
and the existing empty-batch behavior.
---
Outside diff comments:
In `@ip_remote_multi_client.go`:
- Around line 11062-11068: Update the blackhole polling loop around
self.settings.BlackholeTimeout and time.After so a zero BlackholeTimeout cannot
produce a zero-duration timer or busy loop. Apply the existing timeout-disabled
semantics by using a positive fallback poll interval when the configured value
is zero, while preserving cancellation through self.ctx.Done() and
self.client.Done().
In `@net_http_doh.go`:
- Around line 870-883: Update the !leader resolution wait around fl.done and
ctx.Done so the cancellation branch performs a non-blocking check of fl.done
before serving staleAddrs, returning the completed flight’s fresh addresses and
authoritative status when available. Preserve existing stale and cancellation
behavior when the flight has not completed, and add a concurrent regression test
covering this race.
---
Nitpick comments:
In `@backend_degraded_test.go`:
- Around line 143-160: Update TestBackendDegraded_StreakSurvivesGapInsideWindow
to perform the failure streak using backendDegradedFailThreshold rather than
exactly three noteBackendFailure calls. Preserve the intentional gap before the
final failure and keep the existing count and degraded-state assertions.
In `@ip_remote_multi_client_bind_flow_test.go`:
- Around line 638-648: Update the source inspection in the detectBlackhole test
around the setQuarantined anchor so block extraction does not depend on the
exact indentation of its closing brace. Instead, bound the assertion using
stable statement text—such as requiring migrateFlows() after
setQuarantined(reason) and before the following } else {—while preserving the
existing hand-off validation.
In `@ip_remote_multi_client_generator_deadline_test.go`:
- Around line 98-100: Replace the five-second elapsed-time thresholds in the
deadline assertions of the relevant test cases with a CI-tolerant bound near the
configured 50 ms deadline, such as one second. Update the checks around the
affected abandon and canceled-context paths while preserving their existing
failure messages and deadline behavior.
In `@ip_remote_multi_client_observability.go`:
- Around line 451-465: Update relSettingsDiffLines so rendered change.from and
change.to values are not passed through relEvent and relValue for a second
escaping; build the setting diff line directly while preserving the existing
field/from/to output and matching banner rendering.
- Around line 740-762: Update ProbeAllExits and the related runProber
coordination so both paths use the same client-level semaphore, rather than
allocating sem inside each ProbeAllExits invocation. Preserve teardown
cancellation and semaphore release behavior, and ensure overlapping manual
probes and background sweeps share the proberConcurrency limit.
In `@ip_remote_multi_client_prober_test.go`:
- Around line 264-267: Increase parent.settings.ProbeTimeout in this test to
provide sufficient budget for fallback stage B on loaded runners, while
preserving the requirement that stage A times out before fallback begins. Keep
the wait-for-fallback behavior and packet-answer flow unchanged.
In `@ip_remote_multi_client_prober.go`:
- Around line 641-653: Update the client.IsDone() early-return path in the sweep
worker to record a silent result through sweepTally.record before returning,
ensuring the sweep still reaches self.scheduled and can emit probe_sweep_result.
Leave the semaphore-acquisition teardown return guarded by self.ctx.Done()
unchanged and unrecorded.
In `@ip_remote_multi_client_rebind_test.go`:
- Around line 203-207: Update the test setup around rebindTestParent and the
matching fixture at lines 334-338 to mutate the parent’s existing settings by
setting MaxFlowsPerExit to 1, rather than replacing parent.settings with a new
settings object. Preserve the explicit QuicRebindOnExitLoss value configured by
rebindTestParent.
In `@ip_remote_multi_client_rotation_test.go`:
- Around line 313-315: Guard the strings.Index lookup for the "} else" marker in
the drainBranch extraction, and call t.Fatalf with a clear diagnostic when the
marker is absent before slicing rest. Follow the existing failure behavior used
by nextWarningCall, while preserving the current drainBranch extraction when the
marker is found.
In `@ip_remote_multi_client_stall_test.go`:
- Line 1218: The constructor call in the test should make the nine callback
arguments explicit by placing each nil on its own line with a trailing comment
naming reliabilitySettingsFunc, uplinkGateFunc, reliabilityMetricsFunc,
flowCountFunc, resizeWakeFunc, migrateFlowsFunc, providerQualifiedFunc,
receivingSiblingsFunc, and qualificationRefreshFunc, preserving their current
order.
In `@ip_remote_multi_client.go`:
- Around line 3730-3754: The captured lastResort state should not control usable
and headroom implicitly across distant code. Update usable and headroom to
accept the last-resort mode explicitly, pass the appropriate value at every call
site including the last-resort pass, and remove the mutation and restoration of
lastResort around that pass.
- Around line 2069-2113: Add a reflective test covering ReliabilitySettingsFrom
that iterates every field in ReliabilitySettings and verifies the corresponding
value is non-zero when called with DefaultMultiClientSettings(). Use this guard
to detect any field omitted from the manual mapping, while preserving the
existing per-knob tests and nil-settings behavior.
- Around line 2880-3195: Defer this refactor; no code change is required for the
current PR. If addressed later, extract the duplicated sendUpdate v4/v6 branch
logic into a shared helper parameterized by the protocol-specific key and
affinity map types, preserving the existing teardown, affinity joining, app-pin
donor, destination bridge, and ledger behavior.
In `@trace_test.go`:
- Around line 43-56: Remove the returned flag and the unreachable if !returned
assertion from the test around HandleError. Keep the existing comment explaining
that reaching the end verifies HandleError does not repanic, so the test
succeeds by completing normally.
In `@transfer_contract_prewarm_test.go`:
- Around line 15-22: Replace the value-only assertion in
TestPrewarmOpeningContractCanBeDisabled with coverage of prewarmOpeningContract:
configure the disabled flag, invoke it with a SendNoContract request, and assert
no contract request is issued. Add an enabled-path test that invokes
prewarmOpeningContract and verifies the request’s ContractKey matches the key
produced by updateContract, including network peers.
In `@transfer_contract_size_test.go`:
- Around line 46-47: Update the usable budget calculation near the `usable`
variable to derive the fraction from `SendBufferSettings.ContractFillFraction`
rather than hard-coding `0.8`; preserve the existing multiplication by
`settings.InitialContractTransferByteCount` and ByteCount conversion.
In `@transfer_encrypt.go`:
- Around line 1614-1678: Update resendIdentityProofForEstablishment so the
backoff interval is capped by the remaining duration until deadline rather than
the full window, preventing a final sleep from extending past the establishment
window; preserve the existing deadline check and resend behavior. Replace the
per-iteration time.After call with a reusable time.Timer, following the
repository’s established timer-reset and cleanup pattern.
In `@transfer_lane_test.go`:
- Around line 177-194: Decouple contract-result provisioning in the lane test
from the hard-coded four iterations in the HandleControlFrame setup. Ensure each
lane receives contract results on demand, or document and derive the sufficient
count from the lane’s 48-message workload and contract-size settings so future
ramp changes cannot cause misleading lane-starvation failures.
In `@transfer_test.go`:
- Around line 960-982: Strengthen the post-fork assertions around sendSequences:
wait for the asynchronous retirement of the original direct force-stream
sequence and assert that plain.ctx remains active, rather than checking Done
only once. Also count all sequences for destination and require exactly two
total entries—plain plus the intermediary force-stream lane—while retaining the
existing intermediary-count validation.
In `@transfer.go`:
- Around line 3396-3399: Throttle the contract-wait log in the flow around
addContractWaitTime so the 50ms-and-above message does not emit unconditionally
at default verbosity. Reuse the existing logThrottle mechanism, or move this
band to V(1) while retaining unconditional logging only for longer waits;
preserve the current message fields and wait-time accounting.
In `@transport_platform_test.go`:
- Around line 366-378: Update TestKickSkipsDialFailureBackoff’s doc comment to
describe only the observed immediate re-dial and H1 re-election, removing the
unverified “reconnect fast path (hadConnection semantics)” clause. Remove the
redundant defer transport.Close() because testingPlatformTransport already
registers cleanup.
In `@tun_test.go`:
- Around line 621-631: Update bridgeTunBatch to invoke
dst.WriteBatch(packets[:n]) for each batch instead of bypassing the batch API.
Add focused tests covering GRO behavior, shard locking, and checksum offload
through this bridge path, preserving existing transfer and throughput
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e21ac61d-0734-4e26-bdb3-615407bfea61
⛔ Files ignored due to path filters (1)
protocol/transfer.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (83)
backend_degraded_gate_test.gobackend_degraded_test.goframe_protobuf.goframe_protobuf_test.goip.goip_block_action.goip_dial_failure_test.goip_packet.goip_packet_icmp_parse_test.goip_packet_rst_sequence_test.goip_packet_unreachable_test.goip_path_allocation_test.goip_probe_targets.goip_remote_multi_client.goip_remote_multi_client_bind_flow_test.goip_remote_multi_client_blackhole_test.goip_remote_multi_client_busy_probe_test.goip_remote_multi_client_cluster_affinity_test.goip_remote_multi_client_collapse_hold_test.goip_remote_multi_client_degraded_test.goip_remote_multi_client_dev_controls_test.goip_remote_multi_client_dial_failure_test.goip_remote_multi_client_effective_tier_test.goip_remote_multi_client_flow_cap_test.goip_remote_multi_client_generator_deadline_test.goip_remote_multi_client_idle_timeout_test.goip_remote_multi_client_late_server_name_test.goip_remote_multi_client_liveness_test.goip_remote_multi_client_migrate_test.goip_remote_multi_client_network_peer_test.goip_remote_multi_client_observability.goip_remote_multi_client_observability_test.goip_remote_multi_client_pqe_test.goip_remote_multi_client_probe.goip_remote_multi_client_probe_silence_test.goip_remote_multi_client_probe_test.goip_remote_multi_client_prober.goip_remote_multi_client_prober_test.goip_remote_multi_client_quarantine_test.goip_remote_multi_client_rebind_test.goip_remote_multi_client_rotation_test.goip_remote_multi_client_send_stall_test.goip_remote_multi_client_stall_test.goip_remote_multi_client_stall_watchdog_test.goip_remote_multi_client_test.goip_tcp_dial_failure_test.golog_throttle.golog_throttle_test.gomemory_budget_test.gomulti_client_recovery_kernel_test.gonet_http.gonet_http_doh.gonet_http_doh_test.gonet_http_reconnect_test.gonet_tls.goprotocol/transfer.protoreliability_metrics.goreliability_metrics_test.goscripts/Provider_Install_Linux.shscripts/Provider_Install_Win32.ps1scripts/urnet-tools.ps1trace.gotrace_test.gotransfer.gotransfer_contract_manager.gotransfer_contract_manager_test.gotransfer_contract_prewarm_test.gotransfer_contract_size_test.gotransfer_control.gotransfer_encrypt.gotransfer_encrypt_epoch_test.gotransfer_encrypt_test.gotransfer_lane_test.gotransfer_route_manager.gotransfer_route_manager_test.gotransfer_test.gotransport.gotransport_p2p_webrtc.gotransport_p2p_webrtc_pc.gotransport_p2p_webrtc_test.gotransport_platform_test.gotun.gotun_test.go
💤 Files with no reviewable changes (4)
- scripts/Provider_Install_Linux.sh
- ip_remote_multi_client_liveness_test.go
- scripts/Provider_Install_Win32.ps1
- ip_remote_multi_client_degraded_test.go
Comment on lines
+12
to
+15
| func resetBackendDegraded() { | ||
| consecutiveBackendFails.Store(0) | ||
| lastBackendFailNano.Store(0) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all writers of the degraded counters and their call sites.
rg -nP -C4 '\b(noteBackendFailure|noteBackendSuccess)\s*\(' --type=goRepository: full-bars/connect
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== candidate files =='
git ls-files | rg '(^|/)(backend_degraded_test\.go|.*backend.*\.go)$' || true
printf '%s\n' '== counter declarations and references =='
rg -n -C3 'consecutiveBackendFails|lastBackendFailNano|noteBackend(Failure|Success)|CreateContract' --glob '*.go' . || trueRepository: full-bars/connect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== backend_degraded_test.go =='
cat -n backend_degraded_test.go
printf '%s\n' '== relevant CreateContract callback =='
sed -n '1268,1332p' transfer_contract_manager.go
printf '%s\n' '== client shutdown and callback ownership =='
rg -n -C5 'func \(.*\) (Cancel|Done)|func NewClient|go .*Contract|control.*callback|callbacks|Callback' --glob '*.go' client.go transfer_contract_manager.go transport.goRepository: full-bars/connect
Length of output: 25387
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== client lifecycle definitions =='
rg -n -C8 'func NewClient|func \(.*\) Cancel\(|func \(.*\) Done\(\)|type Client struct' --glob '*.go' . | head -n 300
printf '%s\n' '== SendControl implementations and callback dispatch =='
rg -n -C8 'func \(.*\) SendControl|SendControlWithCtx|callback\(.*result|callback\(.*err|callbacks' --glob '*.go' . | head -n 500
printf '%s\n' '== test clients without explicit cancellation =='
rg -n -C4 'NewClient\(' --glob '*_test.go' . | head -n 500Repository: full-bars/connect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== Client construction and shutdown =='
sed -n '655,735p' transfer.go
sed -n '1880,1970p' transfer.go
printf '%s\n' '== OOB interfaces and test implementations =='
rg -n -C5 'type (OutOfBandControl|OutOfBandControlWithCtx)|NewNoContractClientOob|SendControl\(' --glob '*.go' . | head -n 400
printf '%s\n' '== all direct counter writers =='
rg -n -C3 'noteBackendFailure|noteBackendSuccess' --glob '*.go' transport.go transfer_contract_manager.goRepository: full-bars/connect
Length of output: 38178
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== OOB type declarations and constructors =='
rg -n -C10 'OutOfBandControl|NoContractClientOob|ApiOutOfBandControl|OobResultFunction' --glob '*.go' . | head -n 500
printf '%s\n' '== exact callback dispatch sites =='
rg -n -C12 'callback\(resultFrames|callback\(.*err|resultCallback|OobResultFunction' --glob '*.go' . | head -n 500Repository: full-bars/connect
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== OOB implementation =='
sed -n '38,145p' transfer_oob_control.go
printf '%s\n' '== asynchronous test OOB callback =='
sed -n '63,115p' transfer_control_oob_test.go
printf '%s\n' '== NewNoContractClientOob declaration =='
rg -n -C8 'NewNoContractClientOob' . --hidden
printf '%s\n' '== parallel test execution =='
rg -n 't\.Parallel\(\)|T\.Parallel\(\)' --glob '*_test.go' . || trueRepository: full-bars/connect
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== NoContractClientOob callback behavior =='
sed -n '141,166p' transfer_oob_control.go
printf '%s\n' '== API callback context behavior =='
rg -n -C12 'func \(.*BringYourApi.*ConnectControl|ConnectControlWithCtx|NewApiCallback|go .*Result|callback\.Result' net_http.go transfer_oob_control.go --glob '*.go' | head -n 500
printf '%s\n' '== test helpers that return clients and their cleanup =='
rg -n -C6 'func (testingNewClient|newTestEncryptionSession|attachBudgetTestPeer|.*NewClient.*\()' --glob '*_test.go' . | head -n 500Repository: full-bars/connect
Length of output: 36007
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== asynchronous OOB tests and cleanup =='
sed -n '1,220p' transfer_control_oob_test.go
printf '%s\n' '== CreateContract calls in tests =='
rg -n -C8 '\.CreateContract\(' --glob '*_test.go' . | head -n 500
printf '%s\n' '== clients constructed with non-test OOB controls =='
rg -n -C4 'NewClient\([^,]+,[^,]+,[^N][^,]*Oob|NewApiOutOfBandControl|ApiOutOfBandControl' --glob '*_test.go' . | head -n 500Repository: full-bars/connect
Length of output: 39754
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== API OOB use in tests =='
rg -n 'NewApiOutOfBandControl|ApiOutOfBandControl|NewClient\(.*clientOob|NewClientWithTag\(.*clientOob' --glob '*_test.go' . || true
printf '%s\n' '== custom OOB clients in tests =='
rg -n -C2 'NewClient\(' --glob '*_test.go' . |
awk 'BEGIN{RS=""; ORS="\n\n"} /NewClient\(/ && !/NewNoContractClientOob/ {print}' |
head -n 300
printf '%s\n' '== direct CreateContract production call graph =='
rg -n -C10 'CreateContract\(' transfer.go transfer_contract_manager.go transfer_oob_control.goRepository: full-bars/connect
Length of output: 5697
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== platform transport setup in tests =='
rg -n -C5 'NewPlatform|PlatformTransport|TransportGenerator|Connect\(|Auth\(' --glob '*_test.go' . | head -n 500
printf '%s\n' '== test cleanup omissions near client construction =='
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('.').glob('*_test.go'):
lines = p.read_text(errors='ignore').splitlines()
for i, line in enumerate(lines):
if 'NewClient(' in line and 'NewNoContractClientOob' in ''.join(lines[i:min(i+5, len(lines))]):
block = '\n'.join(lines[i:min(i+18, len(lines))])
if not re.search(r'defer .*Cancel|defer .*Close|t\.Cleanup', block):
print(f'{p}:{i+1}')
PYRepository: full-bars/connect
Length of output: 35914
Wait for asynchronous client callbacks before resetting counters. Client.Cancel does not wait for an in-flight CreateContract callback, whose success path can call noteBackendSuccess after cancellation. Ensure all clients, transports, and OOB callbacks finish before these tests reset or assert the process-wide counters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend_degraded_test.go` around lines 12 - 15, Update resetBackendDegraded
and the affected tests to wait for all clients, transports, and out-of-band
callbacks to finish before resetting or asserting the process-wide backend
counters. Do not rely on Client.Cancel alone; synchronize completion of
in-flight CreateContract callbacks, including noteBackendSuccess, before calling
resetBackendDegraded or checking counter values.
Comment on lines
+127
to
+132
| // wrong icmp type: flip dest-unreachable to echo request | ||
| packet, _ := ipOosUnreachable(udpTestPath(4)) | ||
| packet[Ipv4HeaderSizeWithoutExtensions] = 8 | ||
| if _, ok := ipParseIcmpUnreachable(packet); ok { | ||
| t.Error("accepted an icmp echo") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check the build result before indexing the packet.
Line 128 discards the ok return. If ipOosUnreachable ever returns false, packet is nil and Line 129 panics with an index error instead of reporting a clear failure. Every other build site in this file asserts ok first.
💚 Proposed fix
- packet, _ := ipOosUnreachable(udpTestPath(4))
+ packet, ok := ipOosUnreachable(udpTestPath(4))
+ if !ok {
+ t.Fatal("build failed")
+ }
packet[Ipv4HeaderSizeWithoutExtensions] = 8📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // wrong icmp type: flip dest-unreachable to echo request | |
| packet, _ := ipOosUnreachable(udpTestPath(4)) | |
| packet[Ipv4HeaderSizeWithoutExtensions] = 8 | |
| if _, ok := ipParseIcmpUnreachable(packet); ok { | |
| t.Error("accepted an icmp echo") | |
| } | |
| // wrong icmp type: flip dest-unreachable to echo request | |
| packet, ok := ipOosUnreachable(udpTestPath(4)) | |
| if !ok { | |
| t.Fatal("build failed") | |
| } | |
| packet[Ipv4HeaderSizeWithoutExtensions] = 8 | |
| if _, ok := ipParseIcmpUnreachable(packet); ok { | |
| t.Error("accepted an icmp echo") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ip_packet_icmp_parse_test.go` around lines 127 - 132, Update the test setup
around ipOosUnreachable to capture and validate its ok result before indexing
packet; fail the test clearly and return when construction fails, then retain
the existing ICMP type mutation and rejection assertion for successful builds.
Comment on lines
+180
to
+198
| func TestBusyProbeUnsendableRunResetsBetweenEpisodes(t *testing.T) { | ||
| stallTimeout := 20 * time.Millisecond | ||
|
|
||
| client := busyProbeTestChannel(t, func(timeout time.Duration, ackCallback func(error)) (bool, error) { | ||
| return false, nil | ||
| }) | ||
| stallPast(client, stallTimeout) | ||
|
|
||
| window := busyProbeTestWindow(40*time.Millisecond, client) | ||
| AssertEqual(t, window.convictSendStalls(stallTimeout), false) | ||
|
|
||
| // the exit delivers: the episode is over | ||
| client.addSendAck(1440) | ||
| AssertEqual(t, client.sendStalled(stallTimeout), false) | ||
|
|
||
| client.stateLock.Lock() | ||
| AssertEqual(t, client.busyProbeSendFailures, 0) | ||
| client.stateLock.Unlock() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This test does not exercise the episode reset it describes.
busyProbeTestWindow(40*time.Millisecond, client) builds the window with no receiving sibling. convictSendStalls therefore takes the sibling-corroboration hold branch, which calls resetBusyProbeSendFailures() and zeroes busyProbeSendFailures before the probe result is ever consulted. The assertion at line 196 then passes because of the hold, not because addSendAck ended the episode.
TestBusyProbeConvictsOnTwoUnsendable adds receivingSibling() for exactly this reason and says so in its fixture note.
💚 Proposed fix: add the receiving sibling and assert the intermediate state
- window := busyProbeTestWindow(40*time.Millisecond, client)
+ window := busyProbeTestWindow(40*time.Millisecond, client, receivingSibling())
AssertEqual(t, window.convictSendStalls(stallTimeout), false)
+
+ // the first unsendable probe is recorded, so the reset below is observable
+ client.stateLock.Lock()
+ AssertEqual(t, client.busyProbeSendFailures, 1)
+ client.stateLock.Unlock()
// the exit delivers: the episode is over
client.addSendAck(1440)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestBusyProbeUnsendableRunResetsBetweenEpisodes(t *testing.T) { | |
| stallTimeout := 20 * time.Millisecond | |
| client := busyProbeTestChannel(t, func(timeout time.Duration, ackCallback func(error)) (bool, error) { | |
| return false, nil | |
| }) | |
| stallPast(client, stallTimeout) | |
| window := busyProbeTestWindow(40*time.Millisecond, client) | |
| AssertEqual(t, window.convictSendStalls(stallTimeout), false) | |
| // the exit delivers: the episode is over | |
| client.addSendAck(1440) | |
| AssertEqual(t, client.sendStalled(stallTimeout), false) | |
| client.stateLock.Lock() | |
| AssertEqual(t, client.busyProbeSendFailures, 0) | |
| client.stateLock.Unlock() | |
| } | |
| func TestBusyProbeUnsendableRunResetsBetweenEpisodes(t *testing.T) { | |
| stallTimeout := 20 * time.Millisecond | |
| client := busyProbeTestChannel(t, func(timeout time.Duration, ackCallback func(error)) (bool, error) { | |
| return false, nil | |
| }) | |
| stallPast(client, stallTimeout) | |
| window := busyProbeTestWindow(40*time.Millisecond, client, receivingSibling()) | |
| AssertEqual(t, window.convictSendStalls(stallTimeout), false) | |
| // the first unsendable probe is recorded, so the reset below is observable | |
| client.stateLock.Lock() | |
| AssertEqual(t, client.busyProbeSendFailures, 1) | |
| client.stateLock.Unlock() | |
| // the exit delivers: the episode is over | |
| client.addSendAck(1440) | |
| AssertEqual(t, client.sendStalled(stallTimeout), false) | |
| client.stateLock.Lock() | |
| AssertEqual(t, client.busyProbeSendFailures, 0) | |
| client.stateLock.Unlock() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ip_remote_multi_client_busy_probe_test.go` around lines 180 - 198, Update
TestBusyProbeUnsendableRunResetsBetweenEpisodes to add a receivingSibling() to
the busyProbeTestWindow fixture, ensuring convictSendStalls evaluates the
unsendable probe result instead of the sibling-corroboration hold path. After
the first convictSendStalls assertion, verify busyProbeSendFailures is nonzero,
then keep the addSendAck(1440) and final zero-value assertion to confirm the
completed episode resets the failures.
Comment on lines
+115
to
+151
| func relValue(value any) string { | ||
| switch typed := value.(type) { | ||
| case nil: | ||
| return "-" | ||
| case string: | ||
| return relQuote(typed) | ||
| case bool: | ||
| if typed { | ||
| return "1" | ||
| } | ||
| return "0" | ||
| // time.Duration and Id are both Stringers, so they must precede the | ||
| // Stringer case below or they would render in their own spellings | ||
| case time.Duration: | ||
| return strconv.FormatInt(typed.Milliseconds(), 10) | ||
| case Id: | ||
| return relExitId(typed) | ||
| case int: | ||
| return strconv.Itoa(typed) | ||
| case int32: | ||
| return strconv.FormatInt(int64(typed), 10) | ||
| case int64: | ||
| return strconv.FormatInt(typed, 10) | ||
| case uint32: | ||
| return strconv.FormatUint(uint64(typed), 10) | ||
| case uint64: | ||
| return strconv.FormatUint(typed, 10) | ||
| case float64: | ||
| return strconv.FormatFloat(typed, 'f', 2, 64) | ||
| case error: | ||
| return relQuote(typed.Error()) | ||
| case fmt.Stringer: | ||
| return relQuote(typed.String()) | ||
| default: | ||
| return relQuote(fmt.Sprintf("%v", value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A typed nil pointer breaks the "never panics" contract.
case nil matches only an untyped nil interface. A non-nil interface that holds a nil pointer, for example a *someError that is nil or a nil fmt.Stringer, falls through to case error or case fmt.Stringer. The call then dereferences the nil receiver and panics inside a log line. The doc comment above relEvent states that formatting must never panic.
Guard the two method-calling cases with a reflect-based nil check.
🛡️ Proposed guard
case error:
+ if relIsNilPointer(typed) {
+ return "-"
+ }
return relQuote(typed.Error())
case fmt.Stringer:
+ if relIsNilPointer(typed) {
+ return "-"
+ }
return relQuote(typed.String())// relIsNilPointer reports whether an interface value holds a nil pointer,
// which would panic when its method is called.
func relIsNilPointer(value any) bool {
rv := reflect.ValueOf(value)
switch rv.Kind() {
case reflect.Ptr, reflect.Interface, reflect.Map, reflect.Slice, reflect.Func:
return rv.IsNil()
}
return false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ip_remote_multi_client_observability.go` around lines 115 - 151, Update
relValue to safely handle typed nil values before invoking Error or String on
the error and fmt.Stringer cases. Add a reflect-based relIsNilPointer helper
covering nil-capable kinds, and use it to return the existing nil representation
instead of calling methods on typed nil pointers, preserving relValue’s
never-panic contract.
Comment on lines
+1160
to
+1173
| // a stalled exit swallows the packet, matching SendDetailedWithAck exactly | ||
| // (including its position after the frame build): the probe is reported | ||
| // sent and never answered, which is precisely what a stalled provider does | ||
| // to it. This is also how the exclusion tests model a never-answering exit | ||
| // without a transport. | ||
| if self.stalled.Load() { | ||
| return true | ||
| } | ||
|
|
||
| // bare fixture channels have no underlying client; refuse rather than | ||
| // panic, the same convention ClientId and Tier follow | ||
| if self.client == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the frame build and pool-return discipline in the send paths sendProbe mirrors.
set -euo pipefail
# The mirrored production send path
ast-grep run --pattern $'func (self *multiClientChannel) SendDetailedWithAck($$$) {
$$$
}' --lang go ip_remote_multi_client.go || true
# The frame builder: does it wrap a pooled buffer for non-raw frames?
rg -nP -C 10 'func ipPacketToProviderFrame\s*\(' --type=go
# Every pool return / stalled early-exit pairing
rg -nP -C 4 'MessagePoolReturn\(|stalled\.Load\(\)' --type=go -g '!**/*_test.go'Repository: full-bars/connect
Length of output: 155
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files '*ip_remote_multi_client*.go' '*ip_remote_multi_client_probe.go' '*provider*frame*' | sed -n '1,120p'
printf '%s\n' '--- relevant symbols ---'
rg -n -C 12 'func .*SendDetailedWithAck|func .*sendProbe|ipPacketToProviderFrame|MessagePoolReturn|stalled\.Load' --glob '*.go' .Repository: full-bars/connect
Length of output: 50374
🏁 Script executed:
set -euo pipefail
files="$(git ls-files | grep -E '(^|/)ip_remote_multi_client[^/]*\.go$|(^|/)ip_remote[^/]*\.go$')"
printf '%s\n' "$files"
while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
rg -n -C 18 'SendDetailedWithAck|sendProbe|ipPacketToProviderFrame|MessagePoolReturn|stalled\.Load' "$file" || true
done <<< "$files"
printf '\n--- frame builder definitions ---\n'
rg -l 'func ipPacketToProviderFrame' --glob '*.go' . | while IFS= read -r file; do
rg -n -C 25 'func ipPacketToProviderFrame' "$file"
doneRepository: full-bars/connect
Length of output: 37117
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- probe packet ownership ---'
rg -n -C 28 'func probePacket|func probeCourtesyRstPacket|MessagePool(Get|Copy|Return)' ip_remote_multi_client_probe.go ip_remote_multi_client_prober.go
printf '%s\n' '--- protocol defaults and frame marshaling ---'
rg -n -C 18 'DefaultProtocolVersion|func ProtoMarshal|func MessagePool(Get|Copy|ShareReadOnly|Return)|type Frame struct' --glob '*.go' . | sed -n '1,900p'
printf '%s\n' '--- send ownership documentation and tests ---'
rg -n -C 12 'Raw: *true|frame\.Raw|sendProbe|ipPacketToProviderFrame|MessagePoolCounts|poolOutstanding' ip_remote_multi_client_probe*.go ip_remote_multi_client.go frame*.go transfer_pool_balance_test.goRepository: full-bars/connect
Length of output: 50374
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- exact marshaling and pool definitions ---'
rg -l '^func (ProtoMarshal|MessagePool(Get|Copy|ShareReadOnly|Return))\b|^const DefaultProtocolVersion\b|DefaultProtocolVersion *=' --glob '*.go' . |
while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
rg -n -C 20 '^func (ProtoMarshal|MessagePool(Get|Copy|ShareReadOnly|Return))\b|^const DefaultProtocolVersion\b|DefaultProtocolVersion *=' "$file"
done
printf '%s\n' '--- protocol-version settings ---'
rg -n -C 8 'ProtocolVersion:' ip_remote_multi_client.go ip_remote_multi_client_probe*.go ip.go | sed -n '1,260p'Repository: full-bars/connect
Length of output: 7964
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- ProtoMarshal completion ---'
sed -n '725,770p' message_pool.go
printf '%s\n' '--- packet builder ownership ---'
rg -n -C 24 '^func ipOos(Tcp|Udp|Icmp)Packet|^func ipOos.*Sequence|MessagePoolGet\(' --glob '*.go' . | sed -n '1,700p'
printf '%s\n' '--- protocol-version test coverage ---'
rg -n -C 15 'ProtocolVersion.*[01]|ProtocolVersion: *1|sendProbe\(' --glob '*_test.go' . | sed -n '1,500p'Repository: full-bars/connect
Length of output: 42378
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- exact probe packet constructors ---'
rg -n -C 22 'func ipOos(Tcp|Udp|Rst).*|func ipOos.*Packet' --glob '*.go' . | sed -n '1,500p'
printf '%s\n' '--- all sendProbe callers and packet cleanup ---'
rg -n -C 18 'sendProbe\(' ip_remote_multi_client_probe.go ip_remote_multi_client_prober.go ip_remote_multi_client_probe*.go
printf '%s\n' '--- packet ownership checks ---'
rg -n -C 10 'MessagePoolCheck\(parsedPacket\.packet\)|MessagePoolReturn\(parsedPacket\.packet\)|MessagePoolReturn\(packet\)' ip_remote_multi_client_probe.go ip_remote_multi_client_prober.goRepository: full-bars/connect
Length of output: 32760
Release legacy frame buffers on early returns. For ProtocolVersion < 2, ipPacketToProviderFrame creates a pooled frame.MessageBytes. The stalled and client == nil returns in sendProbe, plus the stalled return in SendDetailedWithAck, skip MessagePoolReturn(frame.MessageBytes). Guard these returns with if !frame.Raw; default v2 uses Raw: true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ip_remote_multi_client_probe.go` around lines 1160 - 1173, Update the
early-return paths in sendProbe and SendDetailedWithAck to return pooled legacy
frame buffers before exiting: when frame.Raw is false, call
MessagePoolReturn(frame.MessageBytes), then preserve the existing stalled and
client == nil return values. Keep the default v2 Raw: true behavior unchanged
and apply the guard only to these identified early returns.
Comment on lines
+226
to
+229
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case r := <-receives: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The ctx.Done() branch ends the test as a pass.
If the context is cancelled before the counters reach 3*n, the loop returns and the test reports success without verifying delivery or the sequence-map assertions at lines 247-282. Fail the test on that branch so a cancellation cannot mask a starvation.
🐛 Proposed fix
case <-ctx.Done():
- return
+ t.Fatalf(
+ "context cancelled before completion: receives=%d/%d acks=%d/%d",
+ receiveCount, 3*n, ackCount, 3*n,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| select { | |
| case <-ctx.Done(): | |
| return | |
| case r := <-receives: | |
| select { | |
| case <-ctx.Done(): | |
| t.Fatalf( | |
| "context cancelled before completion: receives=%d/%d acks=%d/%d", | |
| receiveCount, 3*n, ackCount, 3*n, | |
| ) | |
| case r := <-receives: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transfer_lane_test.go` around lines 226 - 229, Update the ctx.Done() branch
in the receive loop of the transfer lane test to fail the test instead of
returning successfully when cancellation occurs before all expected messages are
received. Use the test’s existing failure mechanism, then preserve the receive
path and subsequent sequence-map assertions.
Comment on lines
+5495
to
5553
|
|
||
| // buffer for a combined dispatch: a Peer identity change (contract | ||
| // rotation) is a batch boundary because one callback carries one peer | ||
| peerEqual := func(a Peer, b Peer) bool { | ||
| return a.ProvideMode == b.ProvideMode && | ||
| a.Principal == b.Principal && | ||
| slices.Equal(a.Roles, b.Roles) | ||
| } | ||
| if 0 < len(self.deliverItems) && !peerEqual(peer, self.deliverPeer) { | ||
| self.flushDeliver() | ||
| } | ||
| self.deliverPeer = peer | ||
| self.deliverItems = append(self.deliverItems, item) | ||
| self.deliverFrames = append(self.deliverFrames, appFrames...) | ||
| if receiveDeliverBatchMaxFrames <= len(self.deliverFrames) { | ||
| self.flushDeliver() | ||
| } | ||
| } | ||
|
|
||
| // flushDeliver dispatches the buffered head items' app frames in one receive | ||
| // callback, then sends their acks (deliver-before-ack, as the per-item path | ||
| // did) and returns their pool buffers. The batch is taken out of the sequence | ||
| // fields BEFORE the callback runs: a callback panic (e.g. a resident tearing | ||
| // down mid-control-processing) then loses the un-acked batch — the sender | ||
| // resends and a healthy sequence reprocesses — instead of the exit-path flush | ||
| // re-delivering a half-processed batch or acking frames whose processing | ||
| // failed. | ||
| func (self *ReceiveSequence) flushDeliver() { | ||
| if len(self.deliverItems) == 0 { | ||
| return | ||
| } | ||
| items := slices.Clone(self.deliverItems) | ||
| frames := slices.Clone(self.deliverFrames) | ||
| peer := self.deliverPeer | ||
| clear(self.deliverItems) | ||
| self.deliverItems = self.deliverItems[:0] | ||
| clear(self.deliverFrames) | ||
| self.deliverFrames = self.deliverFrames[:0] | ||
|
|
||
| // pool buffers return exactly once even when the callback panics | ||
| defer func() { | ||
| for _, item := range items { | ||
| item.messagePoolReturn() | ||
| } | ||
| }() | ||
|
|
||
| if 0 < len(frames) { | ||
| // all items of one sequence share the client's receive callback | ||
| items[0].receiveCallback( | ||
| self.source, | ||
| appFrames, | ||
| frames, | ||
| peer, | ||
| ) | ||
| } | ||
| if item.ack { | ||
| self.sendAck(item.sequenceNumber, item.messageId, false, item.tag, item.unwrapped) | ||
| for _, item := range items { | ||
| if item.ack { | ||
| self.sendAck(item.sequenceNumber, item.messageId, false, item.tag, item.unwrapped) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the delivery-batch flush points and any test that measures ack latency under saturation.
rg -nP -C5 '\bflushDeliver\s*\(|receiveDeliverBatchMaxFrames' --type=go
rg -nP -C3 'AckTimeout' --type=go -g '*_test.go'Repository: full-bars/connect
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(transfer\.go|.*_test\.go|go\.mod|.*config.*)$' | head -200
printf '%s\n' '--- batching symbols ---'
rg -n -P -C5 'flushDeliver|receiveDeliverBatchMaxFrames|deliverItems|deliverFrames' .
printf '%s\n' '--- ack timeout symbols ---'
rg -n -P -C5 'AckTimeout|ackTimeout|ack.*timeout|resend|retransmit' . -g '*.go' -g '*.yaml' -g '*.yml' -g '*.json' -g '*.md' | head -500Repository: full-bars/connect
Length of output: 45897
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- receive sequence structure ---'
sed -n '4680,4750p' transfer.go
sed -n '4800,4890p' transfer.go
sed -n '5110,5230p' transfer.go
printf '%s\n' '--- receive and ack paths ---'
sed -n '5350,5585p' transfer.go
printf '%s\n' '--- exact timeout references ---'
rg -n -i -P -C4 'acktimeout|ack.?timeout|ack.*deadline|unacked.*timeout|sendAck|acknowledge' . -g '*.go' | head -800
printf '%s\n' '--- transfer tests related to callback/backpressure/batching ---'
rg -n -P -C5 'batch|callback|backpressure|flushDeliver|receiveHead|AckTimeout|ack' transfer_*_test.go | head -1000Repository: full-bars/connect
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- ack-window and resend symbols ---'
rg -n -P -C4 'sequenceAckWindow|windowTimeout|resend|SendSequence|sendSequence|ackWindow|unacked|sendAck' transfer.go transfer_*.go -g '*.go' | head -1200
printf '%s\n' '--- timeout-like declarations in transfer files ---'
rg -n -P 'const .*Timeout|[A-Za-z]+Timeout|time\.Duration|time\.After|NewTimer|Reset\(' transfer.go transfer_*.go -g '*.go' | head -800
printf '%s\n' '--- focused ack-window test ---'
sed -n '1,280p' transfer_ack_window_test.go
printf '%s\n' '--- focused callback backpressure test ---'
sed -n '1,180p' transfer_callback_backpressure_test.goRepository: full-bars/connect
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- SendSequence declarations and methods ---'
rg -n '^type SendSequence|^func .*SendSequence|resendQueue|MinResendInterval|MaxResendInterval|NewRttWindow|rttWindow' transfer.go transfer_*.go -g '*.go'
printf '%s\n' '--- sender settings declarations and defaults ---'
rg -n -P -C8 'MinResendInterval|MaxResendInterval|ResendQueue|AckTimeout|SendStallTimeout' --glob '*.go' --glob '!**/*_test.go' .
printf '%s\n' '--- focused resend implementation ---'
rg -n -P -C12 'resendQueue\.|Resend|resend|rttWindow|RttWindow' transfer.go transfer_buffer.go transfer_rtt.go | head -1000
printf '%s\n' '--- receive loop with line numbers ---'
sed -n '4930,5235p' transfer.goRepository: full-bars/connect
Length of output: 50374
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re
src = Path("transfer.go").read_text()
batch = re.search(r"const\s+receiveDeliverBatchMaxFrames\s*=\s*(\d+)", src)
ack = re.search(r"AckTimeout:\s*([0-9]+)\s*\*\s*time\.Second", src)
flush = src[src.index("func (self *ReceiveSequence) flushDeliver()"):]
callback = flush.index("items[0].receiveCallback")
ack_call = flush.index("self.sendAck", callback)
deadline = re.search(
r"itemAckTimeout\s*:=\s*item\.sendTime\.Add\(self\.sendBufferSettings\.AckTimeout\)",
src,
)
print({
"batch_frame_cap": int(batch.group(1)) if batch else None,
"default_ack_timeout_seconds": int(ack.group(1)) if ack else None,
"callback_before_ack": callback < ack_call,
"sender_deadline_expression_present": bool(deadline),
"callback_has_local_timeout": bool(re.search(r"receiveCallback.*(?:Timer|Timeout|Deadline)", flush)),
})
PYRepository: full-bars/connect
Length of output: 324
Bound callback-induced acknowledgment delay. flushDeliver runs receiveCallback synchronously before sendAck, with no timeout. A callback lasting beyond the default 60-second AckTimeout can cause sender expiry and retransmission. Add a bound or a saturation test that enforces this limit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transfer.go` around lines 5495 - 5553, Bound the synchronous receiveCallback
work in ReceiveSequence.flushDeliver so acknowledgment delivery cannot be
delayed beyond the configured AckTimeout, or add a saturation test that
explicitly enforces the 60-second limit. Preserve deliver-before-ack ordering
and ensure the test exercises a callback lasting past the timeout without
allowing sender expiry and retransmission.
Comment on lines
+6721
to
+6729
| self.client.ContractManager().CreateContract( | ||
| ContractKey{ | ||
| Destination: self.destination, | ||
| IntermediaryIds: self.intermediaryIds, | ||
| CompanionContract: self.companionContract, | ||
| ForceStream: self.forceStream, | ||
| EncryptionRole: self.encryptionRole, | ||
| EncryptionCompanion: self.encryptionCompanion, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
prewarmOpeningContract builds a ContractKey without NetworkPeer, so the prewarmed contract lands in a different queue.
ContractKey includes NetworkPeer (see transfer_contract_manager.go lines 37-65), and it is the map key for destinationContracts. Both contract requests in updateContract set it: nextContract at lines 3265-3274 and the retry loop at lines 3345-3353 pass NetworkPeer: self.networkPeer.
When self.networkPeer is true, the prewarm request opens a queue under a key that TakeContract never uses. Three consequences follow:
- The opening send still blocks for a full control round trip, so prewarming provides no benefit for network peers.
- The returned contract is orphaned until
ContractQueueExpireTimeoutreclaims it. This is the outcome the function comment says it avoids. contractByteCountsizes the request withoutisNetworkPeerContract, so the request asks for the wrong size.
🐛 Proposed fix to align the prewarm key with the acquisition key
self.client.ContractManager().CreateContract(
ContractKey{
Destination: self.destination,
IntermediaryIds: self.intermediaryIds,
CompanionContract: self.companionContract,
ForceStream: self.forceStream,
+ NetworkPeer: self.networkPeer,
EncryptionRole: self.encryptionRole,
EncryptionCompanion: self.encryptionCompanion,
},
self.contractSeqIndex,
ByteCount(float32(self.sendBufferSettings.MinMessageByteCount)/self.sendBufferSettings.ContractFillFraction),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transfer.go` around lines 6721 - 6729, Update the ContractKey construction in
prewarmOpeningContract to include NetworkPeer: self.networkPeer, matching the
keys used by updateContract and TakeContract. Also pass self.networkPeer to the
contract-size calculation so contractByteCount requests the correct size for
network-peer contracts.
Comment on lines
+675
to
+691
| // drain exactly totalBytes, so neither side needs a half-close. | ||
| // The deadline is refreshed per bounded step so it bounds a | ||
| // stall in the stack, not the whole transfer: under -race plus | ||
| // host load the full stream legitimately outlasts any single | ||
| // fixed deadline while still making progress. | ||
| received := int64(0) | ||
| for received < totalBytes { | ||
| _ = conn.SetReadDeadline(time.Now().Add(55 * time.Second)) | ||
| step := min(totalBytes-received, int64(1024*1024)) | ||
| n, err := io.CopyN(io.Discard, conn, step) | ||
| received += n | ||
| if err != nil { | ||
| recvErr <- err | ||
| return | ||
| } | ||
| } | ||
| recvDone <- n | ||
| recvDone <- received |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A slow-but-progressing transfer can outrun the package test timeout.
The per-chunk deadlines bound a full stall. They do not bound a partial stall. A stream that delivers one chunk every 54 seconds refreshes both deadlines forever, and neither loop observes ctx. The 300-second context is only read in the select after the write loop completes, so it cannot interrupt the loops.
Worst case for one attempt is 1024 write chunks plus 128 read steps at 55 seconds each. Three attempts run in sequence. The test would then hit the go test binary timeout and abort the whole package instead of failing cleanly.
Check ctx in both loops so the overall cap actually applies.
💚 Proposed fix to enforce the overall cap
written := int64(0)
for written < totalBytes {
+ select {
+ case <-ctx.Done():
+ return 0, fmt.Errorf("overall cap reached after writing %d/%d bytes", written, totalBytes)
+ default:
+ }
chunk := payloadApply the same guard to the receiver loop:
received := int64(0)
for received < totalBytes {
+ select {
+ case <-ctx.Done():
+ recvErr <- fmt.Errorf("overall cap reached after reading %d/%d bytes", received, totalBytes)
+ return
+ default:
+ }
_ = conn.SetReadDeadline(time.Now().Add(55 * time.Second))Also applies to: 704-717
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tun_test.go` around lines 675 - 691, Update both the sender loop and the
receiver loop around the visible received-drain logic to check ctx before each
bounded read or write iteration. If the context is canceled, report the context
error through the existing error channel and return immediately, ensuring the
overall test timeout interrupts slow-but-progressing transfers instead of
waiting for per-step deadlines.
Comment on lines
+798
to
+804
| func (self *Tun) WriteBatch(packets [][]byte) (int, error) { | ||
| if len(packets) == 0 { | ||
| return 0, nil | ||
| } | ||
| if len(packets) == 1 { | ||
| return self.write(packets[0], nil) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
WriteBatch reports non-IPv4 packets differently depending on batch size.
For a single non-IPv4 packet, Line 802-804 delegates to write, which returns 0, syscall.EAFNOSUPPORT. For the same packet inside a larger batch, Line 840-843 skips it and WriteBatch returns nil. The caller therefore learns about an unsupported packet only when the batch happens to contain exactly one entry.
Pick one contract. Silent skipping is reasonable for a batch API, but then the single-packet fast path must not surface the error either.
🐛 Proposed fix: make the fast path match the batch contract
if len(packets) == 1 {
- return self.write(packets[0], nil)
+ // match the batch contract below: an unsupported packet is skipped,
+ // not surfaced as an error
+ n, err := self.write(packets[0], nil)
+ if err == syscall.EAFNOSUPPORT {
+ return 0, nil
+ }
+ return n, err
}Alternatively, document the asymmetry on the exported method and keep the current behavior.
Also applies to: 836-843
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tun.go` around lines 798 - 804, Update the single-entry fast path in
Tun.WriteBatch so non-IPv4 packets follow the batch behavior and are silently
skipped rather than returning syscall.EAFNOSUPPORT. Preserve normal writing for
supported packets and the existing empty-batch behavior.
This was referenced Aug 5, 2026
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
3 participants
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.
Closed: opened against a stale fork main, so the diff swept in unrelated upstream commits. Superseded by #3.