Snode pool: 421 recovery on evidence, strike expiry, and reporting failed refreshes - #154
Merged
mpretty-cyro merged 22 commits intoSep 28, 2026
Conversation
A 421 tells us, authoritatively, that the swarm we resolved for an account is wrong. `_handle_421_retry` answered it with `refresh_if_needed`, which decides on `cache_expiration` (2h) and so declines on any cache younger than that, and then re-read the same `_swarm_cache` entry - so the retry went to another node in the list that had just rejected us, and every later request for that account did the same until the cache aged out. Evicting the swarm cache entry alone cannot fix it: the entry is only ever a memo of `swarm::get_swarm(pubkey, _all_swarms)`, so it recomputes the identical answer. What the rejection disproves is the pool snapshot the swarms were generated from, and only a refresh replaces that. `invalidate_swarm` refreshes on that evidence, with a backoff that doubles each time another rejection arrives after a refresh we already ran for one, so a node rejecting everything costs a handful of refreshes over a couple of hours rather than one every minute for as long as it keeps it up.
mpretty-cyro
marked this pull request as ready for review
September 11, 2026 03:05
Refreshing on a 421 recovers the account, but it means a swarm change has every client of that swarm fetch the full node list - 51 bytes per node from each of `cache_num_nodes_to_use_for_refresh` nodes - where nothing made them fetch it at all before. The backoff added with `invalidate_swarm` bounds how often one client repeats that; it does nothing about the aggregate. A node rejecting a request for an account usually names the swarm it actually belongs to, which corrects the one mapping we know is wrong without fetching anything. `get_swarm` prefers such a redirect over its own calculation until the pool is refreshed, and the refresh stays as the fallback for a 421 that carries no usable redirect. Only the node pubkeys are read from the response, and only ones that resolve against the pool we fetched ourselves, so a redirect reaches registered service nodes we already know about and nothing else - it cannot invent a node or name an address of its own choosing. It can still choose which of those nodes we talk to, so: a redirect naming the swarm we already calculated is refused, fewer than `cache_min_swarm_size` resolved names is refused, three in a row without an intervening refresh is refused, and a pool refresh drops every override it was correcting. The redirects are kept out of `_swarm_cache`, which stays a pure memo of `_all_swarms`.
Collaborator
Author
|
Pushed a second commit ( What changed. The point stands that refreshing the pool on a 421 means a swarm change has every client of that swarm fetch the full node list (51 bytes/node from each of So the redirect is now the primary path: a node rejecting a request for an account usually names the swarm it actually belongs to, and On safety — this was the objection I'd originally raised against using the 421 body, and taking only pubkeys and resolving them against our own pool answers it: a redirect can reach registered service nodes we already know about and nothing else. It can still choose which of those we talk to, so it's bounded:
Overrides are kept out of One thing worth a second opinion: a redirect reports New |
`STRIKE_EXPIRY` was applied in `node_strike_count()` - which nothing in this repo calls - and on the disk load. Every place that actually decides something counted the raw vector instead, and `record_node_failure` only ever appends, so a node struck during a two-minute outage stayed out of path building, swarm answers and refresh candidates for the life of the process, and its timestamp vector grew without bound. Restarting was the only thing that cleared it, because loading is the one path that filtered. Counting is now in one place, used by all four, and a node's expired strikes are dropped when it collects a new one. Two things this uncovered, both only reachable with a strike threshold of 0, which is what `tests/test_snode_pool.cpp` has been running with: - comparing the count against the threshold with a bare `>=` excludes every node in the pool, including ones that have never failed, so a threshold of 0 has to keep meaning "drop a node on its first strike". - a permanent failure looped up to the threshold, recording no strikes at all and leaving a node we know is gone in rotation.
A cached edge node is handed to `_build_path` as a forced first hop, so it is the one node in a path that never passes the strike filter `get_unused_nodes` applies to the rest, and the only thing that dropped it was `edge_node_cache_duration` (10 days). So a node we already had evidence was unreachable got another path built onto it on every launch and every resume, and a path rotation carried it into the replacement path. It loses the cached-edge role rather than its place in the pool: it stays a node like any other and can be picked again once its strikes expire. Keeping the entry and merely skipping it would hand the role back at that expiry, by which point we have been running on a different edge node for two days - a second change of first hop, not a return to a stable one.
`refresh_if_needed` had three ways to do nothing and say nothing: a suspended pool, no candidate nodes, and no fetcher. The callback was simply never invoked, and every caller sets its own state up before calling, so "never" is not a delay - it is permanent: - `_resync_clock` sets `_current_clock_resync_id` first, so a dropped callback makes every later clock resync short-circuit as "already in progress" and strands everything queued behind it. - both routers finish setup from the callback, so they never finish it. - `get_random_nodes` and the path builder retry from the callback, so their callers wait on something that will not arrive. The callback now takes whether a refresh actually happened, which is the smallest thing that lets a caller tell "the pool is fine" from "we could not find out". Nothing needing a refresh reports true - that is the answer the caller wanted, not a failure to get one. Each caller had to be given a failing branch, and two of them would have spun rather than hung if the callback had simply always been invoked: `get_random_nodes` re-enters itself, and the path builder rebuilds into the same too-few-nodes branch. `SnodePool` has no C API surface, so this is internal only.
This was referenced Sep 21, 2026
mpretty-cyro
changed the title
Issue: a batch or sequence request that gets a 421 carries its redirect inside each sub-response, but the redirect was only read from the top level of the body. Every batched 421 therefore fell back to a full pool refresh - the very thundering herd the redirect exists to prevent, on the path Session polls through. Separately, a batch that is rejected for some accounts and answered for others (as coalescing requests for several accounts into one batch to their shared swarm will produce) never reached the 421 handling at all, because its results aren't uniformly 421. Solution: the redirects are taken from every snode response, per account. `Request::swarm_pubkey` becomes `swarm_pubkeys`, one account per sub-request in order, and each 421 is attributed to the account at its position; the legacy C API fills this in by parsing the pregenerated batch body. Session storage server 2.12 also echoes the rejected account as `pubkey` in the 421 body, but that is the responding node's word, so it is only checked against the position: a 421 naming a different account is ignored. Trusting it instead would let any node the client talks to install a redirect for any account - a group everyone is in, say - rather than only the ones it was asked about. A rejection of a single account still retries the whole request on its (possibly redirected) swarm. A batch rejected for several accounts is not retried - a retry goes to one swarm - but its redirects are recorded, so the next request for each account goes to the right place. A rejection that names no usable swarm falls back to one pool refresh for the whole response. Because the redirects are now recorded before anything else happens to the response, a 421 on a request whose retry budget is already spent no longer discards its redirect either. The batch walk shared by `find_uniform_batch_error`, `_update_network_state` and the redirect extraction is now one `response::subresponses`, and the response body is parsed once rather than separately by each. Forward-porting to `client`: Core's `_swarm_request` needs to fill in `swarm_pubkeys` from the accounts it builds each body for.
Issue: an onion or proxied request's response is encrypted by the destination, and only a response that decrypts is the destination's answer. When decryption fails - as it does for anything a node along the path sends back on its own - the onion router and the session-router proxy both passed the outer HTTP status through unchanged. Network acts on some statuses as the destination's verdict: - 421 re-resolves the account's swarm, refreshing the whole pool when there is no usable redirect, and retries; - 406 resyncs the clock against the snode network; - 425 resyncs the clock against a server. So any node on the path could force a pool refresh or a clock resync just by answering with one of those statuses in plaintext. With 421 recovery now refreshing on evidence rather than on the pool's age, that is a cheap way to make a client fetch the full node list. Solution: when a response can't be decrypted, those three statuses are reported as a new ERROR_UNAUTHENTICATED_RESPONSE instead. Every other status passes through as before: the same path also carries timeouts and transport failures, and the onion router's path-failure handling matches on the statuses nodes along the path return, none of which is one of these three. The error message now carries the status the node sent, since the reported status no longer does. Requests sent to a storage server through session-router tunnels, and by the direct router, are unaffected: they aren't body-encrypted, their status is the destination's own over an authenticated connection, and they never reach either decryption path.
Issue: a redirect was refused as "the node contradicting itself" when it named the swarm calculated from our pool. That comparison measures a redirect against exactly the data the 421 says is out of date. Swarms can be rearranged any way at all - members join or leave, a swarm is disbanded and its members scattered, a new one is formed from nodes anywhere in the pool - so the account's actual swarm can relate to our grouping in any way: the same, a part of it, overlapping, or disjoint. How a redirect compares with it says nothing about whether the redirect is right. In particular, once a redirect had been followed to swarm T, a node in T correctly naming the calculated swarm - which is how a wrong redirect gets put right - was refused, and since refused redirects don't count towards the limit, the account stayed pinned to T until a pool refresh, which invalidate_swarm declines while the pool is under a minute old or its backoff is active. Solution: the comparison is gone. A redirect is followed whenever enough of the nodes it names resolve against our pool. What bounds a redirect is unchanged and never depended on the comparison: only nodes already in the pool can be named, and every redirect is dropped at the next pool refresh.
Issue: refresh_if_needed reports true both when a refresh ran and when nothing needed refreshing, and in the latter case calls back straight away - inline, since Network, the pool and the routers share one loop. Two callers retried on true without checking that they could now get what they needed: - get_random_nodes called itself again; - _build_path rebuilt the path. get_unused_nodes applies subnet diversity, exclusions and strikes, none of which refresh_if_needed's count of usable nodes does, so a pool can be healthy by refresh_if_needed's measure and still unable to supply the caller. Each retry then landed back in the same branch with nothing changed, recursing until the stack gave out: get_random_nodes from the C API with a count the pool can't meet, _build_path on a pool whose usable nodes sit in too few /24s to fill a path, such as a small testnet or devnet. This predates the refresh callback reporting a result - it only stopped the retry when a refresh could not be started at all. Solution: both check before retrying, as the session-router proxy selection already did, so each makes at most one refresh request. get_random_nodes calls back with an empty list, per its documented contract, and _build_path fails the queued requests for the category through the same path as a refresh that could not start, with messages that now say which it was. The onion router test's snode pool can now call back from refresh_if_needed, up to a limit so that code which asks again on every callback stops rather than overflowing the stack. Against the unfixed _build_path the new test sees 21 refresh requests and the queued request never answered; with the fix it sees one, and the request failed.
Issue: after MAX_CONSECUTIVE_SWARM_REDIRECTS (3) redirects for an account, the next was refused and the account's override erased, for the caller to fall back on refreshing the pool. But that refresh goes through invalidate_swarm's throttle, which declines while the pool is under a minute old or its backoff is active - exactly when a network-wide rearrangement makes hitting the limit likely. When it declined, the request failed, the account went back to the swarm calculated from the pool (the one the first 421 had already disproved), and with the entry erased the count started over from zero, so the limit never held. The count was also never reset by anything but a refresh, so "consecutive" was wrong, and legitimate changes to a swarm's membership spent it just as disagreeing nodes did. Solution: the limit only ever asks for a refresh; it never refuses a redirect, since refusing one only guarantees the request fails. Past it, each further redirect is followed as usual and also asks invalidate_swarm for a refresh, so the refresh happens at the first redirect its throttle allows, and until then the account stays on its latest redirect. Renamed to SWARM_REDIRECTS_BEFORE_REFRESH for what it now does, and raised to 8: a genuine bounce between disagreeing nodes still reaches it within a few requests, while ordinary rearrangements, which take one or two redirects an account, don't. The count saturates rather than wrapping, since it keeps climbing for as long as the refresh stays throttled.
Issue: a redirect was refused unless at least cache_min_swarm_size of the nodes it named were in our pool. That is the realistic way a redirect can't be followed in full: a swarm rearranged partly out of nodes that registered after our last pool refresh. The nodes we do know are still members of the account's actual swarm, but refusing the redirect left the account on the swarm that had just rejected it, with the request falling back to invalidate_swarm - which declines while the pool is under a minute old or its backoff is active, and when it declines fails the request outright. Solution: a redirect is followed with whichever of its nodes are in our pool, and refused only when none are. When some of the named nodes are unknown and that leaves fewer than cache_min_swarm_size, it also asks invalidate_swarm for a refresh, through the same throttle as the redirect limit, to fill in the rest. A small swarm whose members are all known doesn't count, since the pool isn't missing anything. Failing the request when the swarm can't be re-resolved is kept, and now says why: it is reached only when no node the redirect named is one we know, so a retry could only go back to the swarm that rejected us.
Issue: when a path couldn't be built, or the session router couldn't find a proxy, because the snode pool couldn't supply enough usable nodes, the failure was reported with a bare -1. Network failures are otherwise reported with the named ERROR_* codes, which is what lets a caller (through the C API included) tell what went wrong; -1 only says that something did. Solution: a new ERROR_INSUFFICIENT_NODES (-10013) for all three sites - the path builder failing its queued requests, and proxy selection after a pool refresh that failed or left no usable node.
Issue: QuicTransport::suspend() returned early unless the transport was
already suspended - the guard was inverted - so it never did anything and
_suspended was never set. In production that mostly didn't show, because
Network::suspend closed the transport's connections straight afterwards,
which resets its endpoint: requests during a suspension still failed at
once, just with -1 ("Network is invalid") rather than
ERROR_NETWORK_SUSPENDED. The defect was the wrong error code and a
suspended flag nothing could rely on. This is on dev as well.
Fixing the guard makes the transport's suspend() close its connections
itself, and exposes how that close treated the nodes it closed on:
- The connections are closed synchronously by the endpoint's destructor
when _close_connections() resets it (close_conns() only defers, and its
deferred job finds the endpoint gone). Each close runs the transport's
handler with code 0, and _fail_connection passes that code to every
pending edge verification. The router strikes an edge node whose
verification fails with an error code other than a handshake timeout, and
0 counts - so a close we asked for struck every edge node with a
verification in flight. Verifications with no connection yet were
cancelled with -1, to the same effect.
- _fail_connection also fires the transport's failure listeners, and the
router's listener retires the path and starts rebuilding it.
Both reach the router while it is still live, because Network::suspend
suspended the transport before the router, and Network::_close_connections
(the public close_connections() route, on dev too) closes the transport
before the router.
Solution:
- suspend() returns early only when already suspended.
- _close_connections() takes the pending verifications and requests out of
their maps before resetting the endpoint, so the destructor's closes find
nothing to fail, and answers the copies once the transport is fully
closed: verifications with no error code, since our own close is no
evidence against a node, and requests with ERROR_NETWORK_SUSPENDED when
suspended or ERROR_CONNECTION_CLOSED when not (they were told "suspended"
either way). Answering copies rather than walking the members also means
a callback that calls back into the transport - the router's path rebuild
calling verify_connectivity, say - can't insert into the maps being
walked; such a call finds no endpoint and fails cleanly, without an error
code. That is a reason to keep this shape even if the code-0 close is
ever avoided some other way.
- Network::suspend suspends the router before the transport, and
Network::_close_connections closes the router before the transport, so
the router has dropped its path builds and failure listeners before the
transport closes the connections they are attached to.
Tests: a request sent to a suspended transport is refused with
ERROR_NETWORK_SUSPENDED (with the inverted guard it went out and failed on
the network with -1), and a verification in flight when the transport is
closed is answered exactly once, with no error code (before, it got 0 from
the destructor's close).
Issue: SnodePool::suspend() set its flag and flushed strikes, and left any refresh in progress to carry on. With the transport and router both refusing requests while suspended, every fetch that refresh made failed at once, and its retry paths - a failed fetch moving to the next candidate, a rejected set of results being fetched again - don't check for suspension, so it worked its way through the candidate list at the retry delay (capped at 5s, three chains in parallel): about an hour for a mainnet-sized pool, waking the process to do nothing useful on Android and desktop (iOS freezes it, so there the timers never fire). Whatever waited on the refresh was answered only when the candidates ran out, and the failures it counted up slowed the first refreshes after resume. Solution: suspend() abandons a refresh in progress - resetting its id, candidates, results and failure count - and answers everything waiting on it with false, as the router and transport already do with their own work in flight. Treating a suspension as a fresh start drops genuine pre-suspend failures too; that is deliberate, since they say nothing about the network we resume on. Nothing can be in flight while suspended any more, so _refresh_snode_cache's suspended branch answers its waiters unconditionally. Retries belonging to an abandoned refresh can still fire afterwards, so: - _launch_next_refresh_request does nothing unless its refresh is still the current one; before, it only checked that some refresh was, and would take candidates from whichever refresh had replaced it; - the "ran out of candidates" retry captures its refresh id and does nothing when that refresh is gone. Before, it reset the id and started a new refresh unconditionally, which could wipe out a refresh started after resume. This was the one place a refresh restarted itself; after a suspension, the next refresh now comes from the pool's ordinary callers asking again - the router's path pre-build on resume, via get_unused_nodes, or a clock resync. A refresh invalidate_swarm started has already stepped its backoff when it is abandoned, so after a short suspension the first rejection-driven refresh can be declined, for at most the current backoff interval. The throttle resets itself after quiet for twice that interval, so a longer suspension carries nothing over.
Issue: [get_unused_nodes], [update_cache] and [refresh_min_cache_size] built their config::SnodePool positionally, with trailing comments naming fields the values no longer went into. Each list was one entry short of the struct and misaligned by two, so the value commented as cache_node_strike_threshold went two fields early and the trailing false filled a numeric field as 0: - in [get_unused_nodes] (both of its configs) and [update_cache], the 3 went into cache_num_nodes_to_use_for_refresh and the false into cache_min_num_refresh_presence_to_include_node; - in [refresh_min_cache_size], which also sets cache_min_size, the 3 went into cache_min_num_refresh_presence_to_include_node and the false into cache_node_strike_threshold. Every one of them therefore ran with a strike threshold of 0 - a node out on its first strike. None of the three tests' outcomes depended on that, or on the refresh settings the values landed in: TestSnodePool makes refresh_if_needed a no-op, none of them reaches a refresh through get_swarm on an empty pool, and [refresh_min_cache_size] feeds _on_refresh_complete a single response, for which the presence requirement doesn't apply. But the one strike check, in [get_unused_nodes], uses permanent failures, which strike a node out at any threshold, so nothing checked the threshold itself. Solution: all four configs are designated, with the values the comments meant - cache_node_strike_threshold 3, plus cache_min_size 12 where marked - and everything else they didn't annotate left 0. So besides the threshold, cache_num_nodes_to_use_for_refresh goes from 3 to 0 in the short lists and the presence requirement from 3 to 0 in [refresh_min_cache_size]; for the reasons above, neither has any effect on these tests. [get_unused_nodes] now also checks the threshold: a node with two ordinary strikes is still picked, and with a third it isn't. At the old threshold of 0 that check fails, since two strikes already exclude the node.
Issue: _rotate_path keeps a path's edge node across rotations - so a client's first hop stays stable - unless it has been in use longer than edge_node_cache_duration, or (since the cached edge node change) it has been struck out. Nothing tested the struck-out case; the existing test covers only the edge nodes cached from a previous run. Solution: [onion_request_router][rotate_path] rotates a path whose edge node was connected just now, so the ten-day rule can't be what decides, and checks what the rotation asks the pool for: the rest of a path keeping the edge node (path_length - 1 nodes) when it isn't struck, a whole new path (path_length) once it is. The mock pool now records the count of each get_unused_nodes call, since its canned result doesn't reflect it. Without the struck-out condition the second rotation asks for path_length - 1 and the test fails. TestOnionRequestRouter::set_paths used emplace, which leaves a category's paths alone if it already has some, so setting them twice on one router silently kept the first set. It now replaces them, which is what its name says and what the new test needs; no existing test set them twice.
Issue: get_swarm schedules a background refresh_if_needed, the check that refreshes a pool past cache_expiration, only on the path that works a swarm out from the pool. Answers from the swarm cache return before it, and so, since the redirect commits, do answers from an override. So each account triggered the check once per pool generation, on its first lookup, and never again while its swarm was cached - and an overridden account, whose override only a refresh clears, never did at all. Refreshes still happened, because path rotation and building call get_unused_nodes, which schedules the same check, but only thanks to that unrelated caller. The cached case predates this branch; the override case widened it. Solution: the check is scheduled at the top of get_swarm, so every call makes it. It costs nothing when the pool is fresh or a refresh is already running. A test counts the checks: one each for a swarm worked out from the pool, one answered from the cache and one answered from an override; before, the last two made none.
Issue: session_network_send_request takes each sub-request's account from a pregenerated batch or sequence body, and uses the caller's swarm_pubkey_hex only when the body can't be parsed at all. A sub-request whose params carry no pubkey got an empty entry even when the caller had said which account the request was for. With 421s attributed strictly by position in the request, a 421 for that sub-request could then be attributed to no account, and the redirect it carried was lost. Solution: batch_request_accounts takes a fallback account, which fills any sub-request whose params name none; accounts parsed from the body are kept, since they can differ per sub-request. The C API passes swarm_pubkey_hex as the fallback.
Issue: every accepted swarm redirect logged at info, and once an account is past SWARM_REDIRECTS_BEFORE_REFRESH every further redirect warned and asked invalidate_swarm for a refresh, whose "leaving it alone" line for a throttled refresh was also at info. During a bounce between disagreeing nodes that is several lines per request for as long as the backoff holds, up to its two-hour cap - useful once, noise after that. Solution: the per-redirect line and invalidate_swarm's decline line are debug. The limit warning fires only on the redirect that crosses it, and later ones log at debug. The warning for a redirect naming nodes our pool doesn't have is unchanged. It can repeat too - every request for that account produces one while the refresh it asks for is throttled - and the same first-then-debug treatment would apply, but that waits for evidence that it is noisy in practice.
Member
|
Follow-up commits from review are now on this branch, on top of the original five. This summarizes 421 recoveryRedirects replace most refreshes. The 421 body names the swarm the account now belongs to. Only Changes from the design described above:
Batches and sequences are handled per account. A batch's 421s sit in its sub-results, and each Attribution is by position, from the request itself.
Only an encrypted status is the destination's. When an onion or proxied response can't be
Refresh callbacks
Suspend
Smaller changes
TestsFrom the original five commits: Added by the follow-ups: 149 cases, all green; Forward-porting to
|
jagerman
approved these changes
Sep 24, 2026
A redirect asked for a pool refresh only when it named nodes we don't have *and* that left fewer than cache_min_swarm_size of them. The first half reads "our pool is behind", which is a fair guess for an honest redirect - a rearrangement pulls in nodes registered since our last refresh, so some names don't resolve - but it is exactly backwards for a dishonest one: a node naming only itself leaves nothing unresolved, so the pool looks fine and no refresh is asked for, while the account is pinned to a single node until one happens for another reason. Dropping that half changes one shape - a redirect naming fewer than cache_min_swarm_size nodes, all of which we know. Once enough resolve the condition is false either way, so a redirect naming a node or two we don't happen to have alongside a full swarm still asks for nothing. The redirect is followed either way; this only decides whether a refresh is also requested, and that request goes through invalidate_swarm's throttle.
The Linux CI stages build against the system liboxenquic, which deprecates quic::opt::disable_mtu_discovery in favour of max_udp_payload::minimum(), and -Werror makes that fatal. The vendored libquic this repo pins has no max_udp_payload at all, so the deprecated name is the only spelling that compiles against both - there is nothing to migrate to here yet. Moving the pin is not a pin change: the newer session-router defines oxen::quic through its own session-deps, which collides with the definition in external/CMakeLists.txt, so the dependency handling has to move onto session-deps first. That comes over with the Session client work, and the TODO goes with it.
tests/utils.hpp uses std::condition_variable, std::mutex and the lock guards without including <condition_variable> or <mutex>, and test_config_userprofile.cpp calls std::this_thread::sleep_for without <thread>. Newer standard libraries pull all three in through something else, so this builds everywhere except the older libstdc++ on the Debian 12 CI stage, which is where it fails. Both predate this branch. They surfaced now because every Linux stage used to fail compiling the network library, before it got as far as the tests.
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
2 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.
Five commits against
dev, each independently reviewable and any of them droppable. They're togetherbecause they all sit in
src/network/and touch the same few files, so splitting them into separatePRs just moved the conflict resolution to merge time.
1–2. A 421 couldn't invalidate the mapping it proved was wrong
A 421 says authoritatively that the swarm we resolved for an account is wrong.
_handle_421_retryanswered it with
refresh_if_needed, which decides oncache_expiration(2h) and so declines forany cache younger than that, then re-read the same
_swarm_cacheentry — so the retry went to anothernode in the list that had just rejected us,
redirect_retry_count(1) was exhausted, and every laterrequest for that account did the same until the cache aged out. A client could be unable to interact
with a swarm for up to two hours, recoverable only by waiting or a full
clear_cache().Evicting the swarm cache entry is a no-op, which is worth stating since it's the obvious-looking fix:
_swarm_cacheonly ever holdsswarm::get_swarm(pubkey, _all_swarms),_all_swarmsisgenerate_swarms(_snode_cache), and both writers of_all_swarmsreplace_swarm_cachein the samebreath. It can never disagree with the pool. What a 421 disproves is the pool snapshot.
Commit 1 adds
invalidate_swarm— evidence-driven, alongside the age-drivenrefresh_if_needed—with a backoff that doubles each time another rejection arrives after a refresh we already ran for one
(immediate, 1m, 2m, 4m… capped at
cache_expiration), so a node rejecting everything costs a handfulof refreshes over a couple of hours rather than one a minute.
Commit 2 makes the redirect the primary path, after review feedback that refreshing on every 421
means a swarm change has every client of that swarm fetch the full node list (51 bytes/node from each
of
cache_num_nodes_to_use_for_refreshnodes) where nothing made them fetch it before. A rejectingnode usually names the swarm the account actually belongs to;
get_swarmnow prefers that until thepool is refreshed, with the refresh kept as the fallback when a 421 carries no usable redirect.
Only pubkeys are read from the response, and only ones resolving against the pool we fetched
ourselves, so a redirect reaches registered service nodes we already know and nothing else. It can
still choose which of those, so: a redirect naming the swarm we already calculated is refused, fewer
than
cache_min_swarm_sizeresolved names is refused, three in a row without an intervening refreshis refused, and a pool refresh drops every override it was correcting. Overrides are kept out of
_swarm_cache, which stays a pure memo of_all_swarms.INVALID_SWARM_ID— it names members, not an id, and the new id isn't derivablefrom a stale pool. Harmless today (the C API discards the swarm id,
_handle_421_retryignores it) butit would matter if a consumer started reading it.
3. Strikes never expired where it counted
STRIKE_EXPIRY(48h) was applied innode_strike_count()— which nothing in this repo calls, it'sclient-facing API — and on the disk load. Every place that actually decides something counted the
raw vector:
refresh_if_needed's usable-node count,get_unused_nodes' skip, andget_swarm'sfilter_by_strikes. Sincerecord_node_failureonly appends, a node struck three times during atwo-minute outage stayed out of path building, swarm answers and refresh candidates for the life of
the process, with its vector growing unbounded. Restarting was the only cure, because loading is the
one path that filtered.
Lopsided in a telling way:
_perform_strikes_writealready filtered bySTRIKE_EXPIRYon the way outand
_load_from_diskfiltered again on the way in. Both ends of persistence honoured expiry; only thein-memory decision sites didn't. (So this changes nothing about what lands on disk.)
Two further defects fell out, both only reachable at
cache_node_strike_threshold == 0— which is whattests/test_snode_pool.cpphas been running with — and both caught by the existing suite:>=against the threshold excludes every node in the pool, including ones that neverfailed (
0 >= 0). The old code dodged it only by requiring the map lookup to succeed first.for (i = 0; i < threshold; ++i)is zeroiterations — leaving a node we know is gone in full rotation. Now
max(1, threshold).48h after its last strike instead of never. Intended — it may have recovered, and gets struck out again
on first use — but it is a change.
4. The cached edge node never had its strikes consulted
OnionRequestRouterkeeps sticky edge nodes so a client's first hop is stable across sessions, andhands one to
_build_pathas a forced first hop — so it's the one node in a path that never passesthe strike filter
get_unused_nodesapplies to the rest. Onlyedge_node_cache_duration(10 days)ever dropped it, so a node we had evidence was unreachable got another path built onto it on every
launch and every resume.
It now loses the cached-edge role once struck out — not its place in the pool; nothing here touches
_snode_cache, soget_unused_nodescan pick it again when its strikes expire. Dropped rather thanskipped because
_cached_edge_nodesis written once at load and never again, so a retained entry wouldreclaim the role at expiry, by which point we'd been on a different edge node for two days.
Lowest-value commit of the five — the retry already excludes the failed edge node, so the real cost
is one wasted build plus a retry per launch. Easy to drop.
5.
refresh_if_neededcould silently never call backThree ways to do nothing and say nothing — suspended pool, no candidate nodes, no fetcher. Callers set
their state up before calling, so "never" is permanent, not slow:
_resync_clock_current_clock_resync_idfirst, so every later clock resync short-circuits as "already in progress" and its queue is stranded_finish_setup()never runs — the router never becomes usableget_random_nodes, path build, proxy selectionThe callback now carries whether a refresh happened; "nothing needed refreshing" reports
true, sincethat's the answer the caller wanted. The flag does real work rather than being decoration — simply
always invoking the callback would turn two of these hangs into spins, as
get_random_nodesre-entersitself (and
Loop::callruns inline on the loop thread, so that's stack recursion) and the pathbuilder rebuilds into the same branch.
SnodePoolhas no C API surface, so this is internal only. The judgement calls are the review-worthypart: both routers still finish setup (not finishing leaves them permanently unusable), and
_resync_clockroutes into the existing_on_clock_resync_complete(), which resets the in-progress id,fails the queued requests, and leaves the existing offset alone — an old offset beats none.
Tests
Four new cases —
[invalidate_swarm],[swarm_redirect],[strike_expiry],[refresh_callback_contract],[cached_edge_nodes]— 139 total, all green.utils/format.sh verifyclean.Where a test couldn't fail against the pre-fix tree (it calls a function that didn't exist), the old
behaviour was reproduced by mutation instead and the failing assertion recorded.
[strike_expiry]is agenuine fail-before-fix:
2 == 4and4 == 1against unfixed code.Forward-port
client(b0a523fd) carries all of these. Commits 1–2 and 4 apply cleanly; 3 conflicts only on thesysclock_now_s()→clock_now_s()rename and 5 on the_loop->call→_jq.calljob-queue refactor.