From 60c6d7996d0e93a61450b4179c4cca5e8a88b05b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 09:58:26 +1000 Subject: [PATCH 01/22] Recover from a 421 on the evidence rather than on the cache's age 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. --- include/session/network/snode_pool.hpp | 10 ++ src/network/session_network.cpp | 18 ++-- src/network/snode_pool.cpp | 80 ++++++++++++++ tests/test_snode_pool.cpp | 143 +++++++++++++++++++++++++ 4 files changed, 243 insertions(+), 8 deletions(-) diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 787e2c582..22c0194f2 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -76,6 +76,14 @@ class SnodePool : public std::enable_shared_from_this { const std::vector& in_use_nodes, std::function on_refresh_complete = nullptr); + // Re-resolves `swarm_pubkey` after a node we believed was in its swarm has told us the account + // isn't there. Where `refresh_if_needed` decides on the cache's age, this decides on that + // rejection, so a mapping we have direct proof is wrong can be replaced long before + // `cache_expiration`. `on_complete` runs once the swarm can be resolved again. + virtual void invalidate_swarm( + session::network::x25519_pubkey swarm_pubkey, + std::function on_complete = nullptr); + virtual void get_swarm( session::network::x25519_pubkey swarm_pubkey, bool ignore_strike_count, @@ -109,6 +117,8 @@ class SnodePool : public std::enable_shared_from_this { // Refresh logic std::chrono::system_clock::time_point _last_snode_cache_update; + std::chrono::system_clock::time_point _last_evidence_refresh; + std::chrono::seconds _evidence_refresh_backoff{0}; std::optional _current_snode_cache_refresh_id; int _snode_cache_refresh_failure_count = 0; std::vector _refresh_candidate_nodes; diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 7d160fc41..474ed48cf 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -821,24 +821,26 @@ void Network::_handle_421_retry( "421 Misdirected Request for a request with no swarm"); } - // If we got a 421 it means our snode cache is outdated (because the swarm the destination node - // belongs to doesn't match our cache anymore) log::info( cat, - "Request {} received 421 from node {}, refreshing swarm if stale.", + "Request {} received 421 from node {}, re-resolving its swarm.", original_request.request_id, original_dest_node->to_string()); auto failed_node_copy = *original_dest_node; - std::vector nodes_to_exclude = _router->get_all_used_nodes(); - _snode_pool->refresh_if_needed( - std::move(nodes_to_exclude), + auto swarm_pubkey = *original_request.swarm_pubkey; + + // A node in the swarm we resolved has told us the account isn't in its swarm, which is proof + // the mapping is wrong now rather than a reason to suspect it might be; asking for a refresh + // by age (`refresh_if_needed`) would decline for as long as `cache_expiration`, leaving every + // request for this account failing until then. + _snode_pool->invalidate_swarm( + swarm_pubkey, [this, + swarm_pubkey, req_to_retry = std::move(original_request), cb = std::move(final_callback), failed_node = failed_node_copy] { - auto swarm_pubkey = *req_to_retry.swarm_pubkey; - _snode_pool->get_swarm( swarm_pubkey, false, diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index d4f61ad9a..273a0a0a2 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -42,6 +42,18 @@ namespace { const std::chrono::seconds STRIKE_EXPIRY = 48h; const std::chrono::seconds SAVE_THROTTLE = 5min; + + // A swarm rejection is evidence against the pool snapshot the swarm was derived from, so it is + // only worth acting on once that snapshot is old enough for a refresh to plausibly return + // something different. + const std::chrono::seconds EVIDENCE_REFRESH_MIN_POOL_AGE = 1min; + + // A rejection arriving *after* we already refreshed on one is the refresh telling us it didn't + // help - the network hasn't settled, or the node is lying - so each successive one waits twice + // as long, up to `cache_expiration`. A single 3-node refresh per minute sustained indefinitely + // is not an acceptable resting state for a client on a bad connection, and without this it is + // exactly what a persistent rejection source would produce. + const std::chrono::seconds EVIDENCE_REFRESH_BASE_BACKOFF = 1min; } // namespace SnodePool::SnodePool( @@ -1113,6 +1125,74 @@ void SnodePool::refresh_if_needed( }); } +void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, std::function on_complete) { + _loop->call([this, swarm_pubkey, cb = std::move(on_complete)]() mutable { + if (_suspended) { + log::info(cat, "Ignoring swarm invalidation as pool is suspended."); + + if (cb) + cb(); + return; + } + + // The swarm cache only ever memoises `swarm::get_swarm(pubkey, _all_swarms)`, and + // `_all_swarms` is generated from the pool, so dropping the single entry would recompute + // the same rejected answer. What the rejection actually disproves is the pool snapshot, + // and a refresh is the only thing that replaces it - clearing the swarm cache as it goes. + auto now = std::chrono::system_clock::now(); + + if (!_current_snode_cache_refresh_id) { + // The backoff is deliberately global rather than per-swarm: a refresh replaces the pool + // every swarm is derived from, so a rejection for one swarm is exactly as unhelpful to + // act on right after a refresh as a second rejection for the swarm that prompted it + auto pool_age = now - _last_snode_cache_update; + auto since_last = now - _last_evidence_refresh; + + // Quiet for twice the interval we were holding means whatever caused the last run is + // over, so the next incident starts from the base delay rather than inheriting it + if (since_last > 2 * _evidence_refresh_backoff) + _evidence_refresh_backoff = 0s; + + if (pool_age < EVIDENCE_REFRESH_MIN_POOL_AGE || since_last < _evidence_refresh_backoff) + log::info( + cat, + "Swarm {} was rejected, but the pool is {}s old and the last refresh on a " + "rejection was {}s ago (backoff {}s); leaving it alone.", + swarm_pubkey.hex(), + std::chrono::duration_cast(pool_age).count(), + std::chrono::duration_cast(since_last).count(), + _evidence_refresh_backoff.count()); + else { + _evidence_refresh_backoff = std::min( + std::chrono::seconds{_config.cache_expiration}, + (_evidence_refresh_backoff == 0s ? EVIDENCE_REFRESH_BASE_BACKOFF + : _evidence_refresh_backoff * 2)); + _last_evidence_refresh = now; + + log::info( + cat, + "Swarm {} was rejected by a node we had in it, refreshing the pool (next " + "rejection-driven refresh no sooner than {}s from now).", + swarm_pubkey.hex(), + _evidence_refresh_backoff.count()); + _refresh_snode_cache(); + } + } + + if (!cb) + return; + + // `_refresh_snode_cache` runs inline here (we are on the loop thread) and can decline to + // start, so it is the refresh id - not the call above - that says whether there is anything + // to wait for. A callback queued against a refresh that never started never runs, and on + // this path that would strand the request that triggered the rejection. + if (_current_snode_cache_refresh_id) + _after_snode_cache_refresh.push_back(std::move(cb)); + else + cb(); + }); +} + std::vector SnodePool::get_unused_nodes( size_t count, const std::vector& exclude_nodes) { // Kick of a cache refresh in the background if needed (call_soon to ensure it is scheduled diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index d470ed193..05073eff0 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -69,6 +69,30 @@ class TestSnodePool : public SnodePool { // loop thread void update_cache(std::vector nodes) { _update_cache("test", std::move(nodes)); } + bool debug_refresh_in_progress() { + return _loop->call_get([this] { return _current_snode_cache_refresh_id.has_value(); }); + } + + // Backdates the pool snapshot so the age-based policies can be exercised without waiting + void debug_age_pool(std::chrono::seconds by) { + _loop->call_get([this, by] { _last_snode_cache_update -= by; }); + } + + void debug_age_evidence_refresh(std::chrono::seconds by) { + _loop->call_get([this, by] { _last_evidence_refresh -= by; }); + } + + std::chrono::seconds debug_evidence_backoff() { + return _loop->call_get([this] { return _evidence_refresh_backoff; }); + } + + // Runs `fn` on the loop thread, which is what makes `get_swarm` / `invalidate_swarm` resolve + // inline rather than being queued: a test observing them from off the loop has no ordering + // guarantee at all + void debug_run_on_loop(std::function fn) { + _loop->call_get([fn = std::move(fn)] { fn(); }); + } + void debug_on_refresh_complete(std::vector> raw_results) { auto total_requests = static_cast(raw_results.size()); _loop->call_get([&] { @@ -77,6 +101,19 @@ class TestSnodePool : public SnodePool { } }; +// `TestSnodePool` stubs out `refresh_if_needed` so tests don't kick off real refreshes; the swarm +// invalidation test needs the real age-based policy to compare against, so it uses this instead +class TestSnodePoolAgePolicy : public TestSnodePool { + public: + using TestSnodePool::TestSnodePool; + + void refresh_if_needed( + const std::vector& in_use_nodes, + std::function on_refresh_complete = nullptr) override { + SnodePool::refresh_if_needed(in_use_nodes, std::move(on_refresh_complete)); + } +}; + // Encodes nodes the way the storage server returns them, so they can be fed to // `_on_refresh_complete`: 51 bytes per node, all multi-byte fields big-endian std::vector to_snode_cache_bin(const std::vector& nodes) { @@ -336,3 +373,109 @@ TEST_CASE("Network", "[network][refresh_min_cache_size]") { snode_pool->debug_on_refresh_complete({to_snode_cache_bin(enough)}); CHECK(snode_pool->size() == 12); } + +TEST_CASE("Network", "[network][invalidate_swarm]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 12, + .cache_min_swarm_size = 3, + .cache_num_nodes_to_use_for_refresh = 1, + .cache_min_num_refresh_presence_to_include_node = 1, + .cache_node_strike_threshold = 3}; + + auto ed_pk_before = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; + auto ed_pk_after = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; + + // Two snapshots of the same two swarms, with an entirely different set of nodes serving them. + // Which swarm the pubkey lands in doesn't matter; what matters is that the correct answer + // changed and no node is common to both. + auto pool_snapshot = [](const std::vector& ed_pk, uint8_t subnet) { + std::vector nodes; + + for (uint16_t i = 0; i < 12; ++i) + nodes.emplace_back(service_node{ + ed25519_pubkey::from_bytes(ed_pk), + oxen::quic::ipv4{"192.168.{}.{}"_format(subnet, i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + static_cast(i < 6 ? 0 : 1)}); + + return nodes; + }; + auto nodes_before = pool_snapshot(ed_pk_before, 0); + auto nodes_after = pool_snapshot(ed_pk_after, 1); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->update_cache(nodes_before); + snode_pool->debug_age_pool(10min); + + auto swarm_pubkey = x25519_pubkey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000000"); + + std::vector rejected_swarm, recovered_swarm; + bool age_policy_started_refresh = false, recovery_ran = false; + + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + rejected_swarm = std::move(nodes); + }); + + // Asking by age is what the 421 path used to do, and on a cache this far short of + // `cache_expiration` it declines - which is what left the disproven mapping in place + snode_pool->refresh_if_needed({}); + age_policy_started_refresh = snode_pool->debug_refresh_in_progress(); + + snode_pool->invalidate_swarm(swarm_pubkey, [&] { + recovery_ran = true; + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + recovered_swarm = std::move(nodes); + }); + }); + }); + + REQUIRE_FALSE(rejected_swarm.empty()); + CHECK_FALSE(age_policy_started_refresh); + + // The rejection has to produce a refresh, and the retry has to wait for it rather than being + // handed the answer that was just rejected + REQUIRE(snode_pool->debug_refresh_in_progress()); + CHECK_FALSE(recovery_ran); + + snode_pool->update_cache(nodes_after); + REQUIRE(recovery_ran); + REQUIRE_FALSE(recovered_swarm.empty()); + + for (const auto& node : recovered_swarm) + CHECK(std::ranges::find(rejected_swarm, node) == rejected_swarm.end()); + + // A rejection that arrives after we already refreshed on one has to be held off, or a node that + // rejects everything keeps a client refreshing from three nodes forever + CHECK(snode_pool->debug_evidence_backoff() == 1min); + snode_pool->debug_age_pool(10min); + snode_pool->debug_run_on_loop([&] { snode_pool->invalidate_swarm(swarm_pubkey); }); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // Once it has elapsed the next one runs, and the one after it waits twice as long again + snode_pool->debug_age_evidence_refresh(90s); + snode_pool->debug_run_on_loop([&] { snode_pool->invalidate_swarm(swarm_pubkey); }); + CHECK(snode_pool->debug_refresh_in_progress()); + CHECK(snode_pool->debug_evidence_backoff() == 2min); + + // Staying quiet for twice the interval means the next incident starts from the base delay + // rather than inheriting an escalated one + snode_pool->update_cache(nodes_after); + snode_pool->debug_age_pool(10min); + snode_pool->debug_age_evidence_refresh(10min); + snode_pool->debug_run_on_loop([&] { snode_pool->invalidate_swarm(swarm_pubkey); }); + CHECK(snode_pool->debug_refresh_in_progress()); + CHECK(snode_pool->debug_evidence_backoff() == 1min); +} From 329f8b9d209e2a51830a9ad2c1e39af2c8fde93d Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 14 Sep 2026 09:15:48 +1000 Subject: [PATCH 02/22] Follow a 421's redirect instead of refreshing the whole pool 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`. --- include/session/network/session_network.hpp | 5 +- include/session/network/snode_pool.hpp | 19 +++ src/network/session_network.cpp | 138 ++++++++++++-------- src/network/snode_pool.cpp | 92 +++++++++++++ tests/test_snode_pool.cpp | 124 ++++++++++++++++++ 5 files changed, 325 insertions(+), 53 deletions(-) diff --git a/include/session/network/session_network.hpp b/include/session/network/session_network.hpp index 9c8f07eb9..cc495e9c8 100644 --- a/include/session/network/session_network.hpp +++ b/include/session/network/session_network.hpp @@ -116,7 +116,10 @@ class Network : public std::enable_shared_from_this { void _recalculate_status(); void _update_status(ConnectionStatus new_status); void _update_network_state(const std::string& body); - void _handle_421_retry(Request original_request, network_response_callback_t final_callback); + void _handle_421_retry( + Request original_request, + std::optional response_body, + network_response_callback_t final_callback); void _resync_clock( std::optional original_request, network_response_callback_t request_callback); diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 22c0194f2..25995fc69 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -84,6 +84,18 @@ class SnodePool : public std::enable_shared_from_this { session::network::x25519_pubkey swarm_pubkey, std::function on_complete = nullptr); + // Records the swarm a node named when it rejected a request for `swarm_pubkey`, so the mapping + // can be corrected without refreshing the whole pool - a swarm change would otherwise have + // every client of that swarm fetch the full node list, where nothing made them before. + // + // Only the node pubkeys are taken from the response, and only ones that resolve against the + // pool we fetched ourselves, so a redirect can reach registered service nodes we already know + // and nothing else. Returns false when the redirect is unusable, which is the caller's cue to + // fall back to `invalidate_swarm`. + virtual bool record_swarm_redirect( + session::network::x25519_pubkey swarm_pubkey, + const std::vector& swarm_node_keys); + virtual void get_swarm( session::network::x25519_pubkey swarm_pubkey, bool ignore_strike_count, @@ -108,6 +120,13 @@ class SnodePool : public std::enable_shared_from_this { std::vector>> _all_swarms; std::unordered_map>> _swarm_cache; + + // Swarms a node has redirected us to, which `get_swarm` prefers over its own calculation. Kept + // apart from `_swarm_cache` deliberately: that is a memo of `_all_swarms` and stays one, where + // these are claims from outside that outrank it until the pool is refreshed. The count bounds + // how far a disagreement between nodes can bounce us before we go and refresh instead. + std::unordered_map, uint8_t>> + _swarm_overrides; std::map> _snode_strikes; bool _strikes_flush_scheduled = false; diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 474ed48cf..24d4babce 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -1,6 +1,7 @@ #include "session/network/session_network.hpp" #include +#include #include #include @@ -482,7 +483,7 @@ void Network::send_request(Request request, network_response_callback_t callback // cache, the original request might succeed after this refresh so we should // just automatically retry if (final_status_code == 421) { - _handle_421_retry(std::move(original_req), std::move(cb)); + _handle_421_retry(std::move(original_req), std::move(body), std::move(cb)); return; } @@ -779,8 +780,41 @@ void Network::_update_network_state(const std::string& body) { // MARK: Specific Error Handling +// Pulls the node pubkeys out of a 421 body. Only the pubkeys: the rest of each record is contact +// information we deliberately don't read, so a redirect can't put us in touch with anything the +// pool hasn't already told us about. +static std::vector redirect_node_keys(const std::optional& body) { + std::vector keys; + + if (!body) + return keys; + + try { + auto json = nlohmann::json::parse(*body); + + if (!json.contains("snodes") || !json["snodes"].is_array()) + return keys; + + for (const auto& entry : json["snodes"]) { + if (!entry.contains("pubkey_ed25519") || !entry["pubkey_ed25519"].is_string()) + continue; + + auto hex = entry["pubkey_ed25519"].get(); + + if (hex.size() == 64 && oxenc::is_hex(hex)) + keys.push_back(ed25519_pubkey::from_hex(hex)); + } + } catch (const std::exception& e) { + log::debug(cat, "Could not read a swarm redirect from the 421 body: {}", e.what()); + } + + return keys; +} + void Network::_handle_421_retry( - Request original_request, network_response_callback_t final_callback) { + Request original_request, + std::optional response_body, + network_response_callback_t final_callback) { if (original_request.retry_count >= config.redirect_retry_count) { log::error( cat, @@ -830,56 +864,56 @@ void Network::_handle_421_retry( auto failed_node_copy = *original_dest_node; auto swarm_pubkey = *original_request.swarm_pubkey; - // A node in the swarm we resolved has told us the account isn't in its swarm, which is proof - // the mapping is wrong now rather than a reason to suspect it might be; asking for a refresh - // by age (`refresh_if_needed`) would decline for as long as `cache_expiration`, leaving every - // request for this account failing until then. - _snode_pool->invalidate_swarm( - swarm_pubkey, - [this, - swarm_pubkey, - req_to_retry = std::move(original_request), - cb = std::move(final_callback), - failed_node = failed_node_copy] { - _snode_pool->get_swarm( - swarm_pubkey, - false, - [this, - req_to_retry = std::move(req_to_retry), - cb = std::move(cb), - failed_node](swarm::swarm_id_t, std::vector swarm_nodes) { - // Extract a single random index from the vector indices, but excluding - // the index of the failing node: - size_t new_target; - auto out = std::ranges::sample( - std::views::iota(0, static_cast(swarm_nodes.size())) | - std::views::filter([&](int i) { - return swarm_nodes[i] != failed_node; - }), - &new_target, - 1, - csrng); - - if (out == &new_target) - return cb( - false, - false, - ERROR_MISDIRECTED_REQUEST, - {content_type_plain_text}, - "421 Misdirected Request, but no other nodes in swarm to " - "retry"); - - log::info( - cat, - "Request {} retrying 421 error on new node {}.", - req_to_retry.request_id, - swarm_nodes[new_target].to_string()); - auto final_request = req_to_retry; - final_request.retry_count++; - final_request.destination = std::move(swarm_nodes[new_target]); - this->send_request(std::move(final_request), std::move(cb)); - }); - }); + auto retry_on_resolved_swarm = [this, + swarm_pubkey, + req_to_retry = std::move(original_request), + cb = std::move(final_callback), + failed_node = failed_node_copy]() mutable { + _snode_pool->get_swarm( + swarm_pubkey, + false, + [this, req_to_retry = std::move(req_to_retry), cb = std::move(cb), failed_node]( + swarm::swarm_id_t, std::vector swarm_nodes) { + // Extract a single random index from the vector indices, but excluding + // the index of the failing node: + size_t new_target; + auto out = std::ranges::sample( + std::views::iota(0, static_cast(swarm_nodes.size())) | + std::views::filter( + [&](int i) { return swarm_nodes[i] != failed_node; }), + &new_target, + 1, + csrng); + + if (out == &new_target) + return cb( + false, + false, + ERROR_MISDIRECTED_REQUEST, + {content_type_plain_text}, + "421 Misdirected Request, but no other nodes in swarm to " + "retry"); + + log::info( + cat, + "Request {} retrying 421 error on new node {}.", + req_to_retry.request_id, + swarm_nodes[new_target].to_string()); + auto final_request = req_to_retry; + final_request.retry_count++; + final_request.destination = std::move(swarm_nodes[new_target]); + this->send_request(std::move(final_request), std::move(cb)); + }); + }; + + // A node that rejects a request for an account usually names the swarm it actually belongs to. + // Taking its word (bounded - see `record_swarm_redirect`) fixes the one mapping we know is + // wrong, where refreshing has every client of a changed swarm fetch the full node list for it. + if (_snode_pool->record_swarm_redirect(swarm_pubkey, redirect_node_keys(response_body))) + return retry_on_resolved_swarm(); + + // No usable redirect, so the pool itself is the only thing that can put this right + _snode_pool->invalidate_swarm(swarm_pubkey, std::move(retry_on_resolved_swarm)); } void Network::_resync_clock( diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 273a0a0a2..d41ce44d2 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -54,6 +54,10 @@ namespace { // is not an acceptable resting state for a client on a bad connection, and without this it is // exactly what a persistent rejection source would produce. const std::chrono::seconds EVIDENCE_REFRESH_BASE_BACKOFF = 1min; + + // Two nodes disagreeing about who owns a swarm can redirect us back and forth; after this many + // in a row without an intervening pool refresh, stop believing them and go and refresh. + constexpr uint8_t MAX_CONSECUTIVE_SWARM_REDIRECTS = 3; } // namespace SnodePool::SnodePool( @@ -889,6 +893,11 @@ void SnodePool::_update_cache(std::string refresh_id, std::vector _snode_cache = std::move(nodes); _all_swarms = swarm::generate_swarms(_snode_cache); _swarm_cache.clear(); + + // The pool these redirects were correcting has been replaced, so they have outlived what + // they were for. Keeping them would let one node's claim outlast the ground truth that + // would have overruled it. + _swarm_overrides.clear(); _last_snode_cache_update = std::chrono::system_clock::now(); // Reset all failure and refresh-in-progress state @@ -970,6 +979,7 @@ void SnodePool::clear_cache() { _snode_cache = {}; _all_swarms = {}; _swarm_cache = {}; + _swarm_overrides = {}; _disk_loop->call([path = _snode_cache_file_path] { SnodePool::_clear_disk_cache(path); }); }); @@ -1193,6 +1203,77 @@ void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, std::function& swarm_node_keys) { + return _loop->call_get([this, swarm_pubkey, &swarm_node_keys] { + if (_snode_cache.empty() || _all_swarms.empty()) + return false; + + // Resolving the names against our own pool is what makes a redirect safe to act on: the + // responding node chooses which registered nodes we end up talking to, but it cannot invent + // a node or point us at an address of its own choosing, because we never read the contact + // details it sent + std::unordered_set named{swarm_node_keys.begin(), swarm_node_keys.end()}; + std::vector resolved; + + for (const auto& node : _snode_cache) + if (named.count(node.remote_pubkey)) + resolved.push_back(node); + + if (resolved.size() < _config.cache_min_swarm_size) { + log::debug( + cat, + "Ignoring redirect for {}: only {}/{} named nodes are in our pool.", + swarm_pubkey.hex(), + resolved.size(), + swarm_node_keys.size()); + return false; + } + + // A redirect naming the swarm we already calculated is the node contradicting itself - it + // rejected the request and then pointed us back at the nodes we asked through. Refusing it + // sends the caller off to refresh the pool, which is the only thing that can help + auto computed = swarm::get_swarm(swarm_pubkey, _all_swarms); + if (computed.second.size() == resolved.size()) { + std::unordered_set computed_keys; + for (const auto& node : computed.second) + computed_keys.insert(node.remote_pubkey); + + if (std::ranges::all_of(resolved, [&computed_keys](const auto& node) { + return computed_keys.count(node.remote_pubkey) > 0; + })) { + log::debug( + cat, + "Ignoring redirect for {}: it names the swarm we already calculated.", + swarm_pubkey.hex()); + return false; + } + } + + auto& [nodes, redirects] = _swarm_overrides[swarm_pubkey]; + + if (++redirects > MAX_CONSECUTIVE_SWARM_REDIRECTS) { + log::warning( + cat, + "Dropping redirects for {} after {} in a row; refreshing the pool instead.", + swarm_pubkey.hex(), + MAX_CONSECUTIVE_SWARM_REDIRECTS); + _swarm_overrides.erase(swarm_pubkey); + return false; + } + + log::info( + cat, + "Redirected to a swarm of {} nodes for {} ({} of at most {}).", + resolved.size(), + swarm_pubkey.hex(), + redirects, + MAX_CONSECUTIVE_SWARM_REDIRECTS); + nodes = std::move(resolved); + return true; + }); +} + std::vector SnodePool::get_unused_nodes( size_t count, const std::vector& exclude_nodes) { // Kick of a cache refresh in the background if needed (call_soon to ensure it is scheduled @@ -1304,6 +1385,17 @@ void SnodePool::get_swarm( return nodes; }; + // A node that rejected a request for this account told us where it belongs, which beats + // anything we can work out from a pool we now know is behind. There is no swarm id to give + // back - a redirect names members, not an id - and nothing downstream reads it. + if (auto it = _swarm_overrides.find(swarm_pubkey); it != _swarm_overrides.end()) { + const auto& swarm_nodes = it->second.first; + + return cb( + swarm::INVALID_SWARM_ID, + (ignore_strike_count ? swarm_nodes : filter_by_strikes(swarm_nodes))); + } + // Check the in-memory swarm cache first if (auto it = _swarm_cache.find(swarm_pubkey); it != _swarm_cache.end()) { const auto& swarm_nodes = it->second.second; diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 05073eff0..87c0e3994 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -479,3 +479,127 @@ TEST_CASE("Network", "[network][invalidate_swarm]") { CHECK(snode_pool->debug_refresh_in_progress()); CHECK(snode_pool->debug_evidence_backoff() == 1min); } + +TEST_CASE("Network", "[network][swarm_redirect]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 12, + .cache_min_swarm_size = 3, + .cache_num_nodes_to_use_for_refresh = 1, + .cache_min_num_refresh_presence_to_include_node = 1, + .cache_node_strike_threshold = 3}; + + // A redirect is resolved by node pubkey, so every node needs its own + auto key_for = [](uint16_t i) { + return ed25519_pubkey::from_hex( + "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46a{:02x}"_format(i)); + }; + std::vector snode_cache; + + for (uint16_t i = 0; i < 12; ++i) + snode_cache.emplace_back(service_node{ + key_for(i), + oxen::quic::ipv4{"192.168.0.{}"_format(i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + static_cast(i < 6 ? 0 : 1)}); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->update_cache(snode_cache); + + // Old enough that the fallback would refresh, so "no refresh happened" below means the redirect + // was taken rather than the pool being too fresh to bother + snode_pool->debug_age_pool(10min); + + auto swarm_pubkey = x25519_pubkey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000000"); + + std::vector calculated; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + calculated = std::move(nodes); + }); + }); + REQUIRE(calculated.size() == 6); + + // Everything the calculation didn't pick, which is what a real redirect would be naming + std::vector elsewhere; + for (const auto& node : snode_cache) + if (std::ranges::find(calculated, node) == calculated.end()) + elsewhere.push_back(node); + REQUIRE(elsewhere.size() == 6); + + // Membership is the claim in every comparison below; the pool is reshuffled on each refresh and + // a redirect is resolved in pool order, so neither side has an order worth asserting + auto sorted = [](std::vector nodes) { + std::ranges::sort(nodes); + return nodes; + }; + auto keys_of = [](const std::vector& nodes, size_t count) { + std::vector keys; + for (size_t i = 0; i < count && i < nodes.size(); ++i) + keys.push_back(nodes[i].remote_pubkey); + return keys; + }; + + // Nodes we've never heard of can't redirect us anywhere - this is what stops a response putting + // us in touch with something that isn't a registered service node + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, {key_for(200), key_for(201)})); + + // Too few of the named nodes resolve to be a swarm + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 2))); + + // Naming the swarm we already calculated is the node contradicting itself + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); + + // A usable redirect is taken, and - the point of all this - no pool refresh is started for it + REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + std::vector redirected; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + redirected = std::move(nodes); + }); + }); + CHECK(sorted(redirected) == + sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 4))); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // Nodes that keep disagreeing must not bounce us forever; after the third we stop believing + // them and the calculated swarm is back, for the caller to fall back on refreshing the pool + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 5))); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 5))); + + std::vector after_giving_up; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + after_giving_up = std::move(nodes); + }); + }); + CHECK(sorted(after_giving_up) == sorted(calculated)); + + // A refreshed pool is ground truth again, so redirects correcting the old one are dropped + REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + snode_pool->update_cache(snode_cache); + + std::vector after_refresh; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + after_refresh = std::move(nodes); + }); + }); + CHECK(sorted(after_refresh) == sorted(calculated)); +} From 152a46e1efd94d2a871b202630f1cbd1ace6f31e Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 10:18:51 +1000 Subject: [PATCH 03/22] Expire snode strikes where it matters, not only where it's reported `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. --- include/session/network/snode_pool.hpp | 5 ++ src/network/snode_pool.cpp | 67 ++++++++++++------------ tests/test_snode_pool.cpp | 71 ++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 34 deletions(-) diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 25995fc69..436cd632a 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -144,6 +144,11 @@ class SnodePool : public std::enable_shared_from_this { std::vector> _snode_refresh_results; std::vector> _after_snode_cache_refresh; + // Counts a node's strikes that haven't expired yet. `record_node_failure` only appends, so the + // raw vector answers a different question - every strike the node has ever collected. + size_t _active_strike_count(const ed25519_pubkey& key) const; + bool _node_struck_out(const ed25519_pubkey& key) const; + // Disk I/O functions void _load_from_disk(); static void _clear_disk_cache(const std::filesystem::path& path); diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index d41ce44d2..d4b9d7848 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -985,6 +985,25 @@ void SnodePool::clear_cache() { }); } +size_t SnodePool::_active_strike_count(const ed25519_pubkey& key) const { + auto it = _snode_strikes.find(key); + + if (it == _snode_strikes.end()) + return 0; + + auto threshold = sysclock_now_s() - STRIKE_EXPIRY; + + return std::ranges::count_if(it->second, [threshold](auto t) { return t > threshold; }); +} + +bool SnodePool::_node_struck_out(const ed25519_pubkey& key) const { + auto strikes = _active_strike_count(key); + + // The `strikes > 0` is what keeps a threshold of 0 meaning "drop a node on its first strike"; + // comparing straight against 0 also drops every node that has never failed at all + return (strikes > 0 && strikes >= _config.cache_node_strike_threshold); +} + void SnodePool::record_node_failure(const service_node& node, bool permanent) { record_node_failure(node.remote_pubkey, permanent); } @@ -992,18 +1011,17 @@ void SnodePool::record_node_failure(const service_node& node, bool permanent) { void SnodePool::record_node_failure(const ed25519_pubkey& key, bool permanent) { _loop->call([this, key, permanent] { auto now = sysclock_now_s(); + auto& stamps = _snode_strikes[key]; + std::erase_if(stamps, [threshold = now - STRIKE_EXPIRY](auto t) { return t <= threshold; }); - if (permanent) - for (int i = 0; i < _config.cache_node_strike_threshold; ++i) - _snode_strikes[key].push_back(now); - else - _snode_strikes[key].push_back(now); + // A permanent failure has to strike the node out whatever the threshold is - looping up to + // a threshold of 0 records nothing and leaves the node in rotation + auto strikes = (permanent ? std::max(1, _config.cache_node_strike_threshold) : 1); - log::trace( - cat, - "Recorded strike for node {}, total: {}", - key.hex(), - _snode_strikes[key].size()); + for (uint16_t i = 0; i < strikes; ++i) + stamps.push_back(now); + + log::trace(cat, "Recorded strike for node {}, total: {}", key.hex(), stamps.size()); // Throttle persisting the strikes to disk to at most every X minutes if (!_strikes_flush_scheduled && !_suspended) { @@ -1031,22 +1049,8 @@ uint16_t SnodePool::node_strike_count(const service_node& node) { } uint16_t SnodePool::node_strike_count(const ed25519_pubkey& key) { - return _loop->call_get([this, &key] { - auto it = _snode_strikes.find(key); - if (it == _snode_strikes.end()) - return uint16_t{0}; - - const auto& stamps = it->second; - - const auto threshold = sysclock_now_s() - STRIKE_EXPIRY; - - uint16_t count = 0; - for (auto t : stamps) - if (t > threshold) - count++; - - return count; - }); + return _loop->call_get( + [this, &key] { return static_cast(_active_strike_count(key)); }); } void SnodePool::clear_node_strikes() { @@ -1090,9 +1094,7 @@ void SnodePool::refresh_if_needed( in_use_keys.insert(node.remote_pubkey); for (const auto& node : _snode_cache) { - auto it = _snode_strikes.find(node.remote_pubkey); - if (it != _snode_strikes.end() && - it->second.size() >= _config.cache_node_strike_threshold) + if (_node_struck_out(node.remote_pubkey)) continue; // If the caller considers the node as already in use then it wouldn't be @@ -1319,9 +1321,7 @@ std::vector SnodePool::get_unused_nodes( continue; // Skip nodes with too many failures - auto it = _snode_strikes.find(node.remote_pubkey); - if (it != _snode_strikes.end() && - it->second.size() >= _config.cache_node_strike_threshold) + if (_node_struck_out(node.remote_pubkey)) continue; // Skip nodes whos IP addresses are in the exclusion list @@ -1357,8 +1357,7 @@ void SnodePool::get_swarm( std::ranges::shuffle(nodes, csrng); auto get_strike_count = [this](const service_node& node) -> size_t { - auto it = _snode_strikes.find(node.remote_pubkey); - return (it != _snode_strikes.end() ? it->second.size() : 0); + return _active_strike_count(node.remote_pubkey); }; // Partition into below-threshold and above-thresold. This keeps the shuffled order of diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 87c0e3994..3518f0363 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -93,6 +93,23 @@ class TestSnodePool : public SnodePool { _loop->call_get([fn = std::move(fn)] { fn(); }); } + // Backdates every recorded strike so `STRIKE_EXPIRY` can be crossed without a 48h wait + void debug_age_strikes(std::chrono::seconds by) { + _loop->call_get([this, by] { + for (auto& [key, stamps] : _snode_strikes) + for (auto& stamp : stamps) + stamp -= by; + }); + } + + // Every strike on record, expired or not - which is what the decision sites used to count + size_t debug_recorded_strikes(const ed25519_pubkey& key) { + return _loop->call_get([this, &key] { + auto it = _snode_strikes.find(key); + return (it == _snode_strikes.end() ? size_t{0} : it->second.size()); + }); + } + void debug_on_refresh_complete(std::vector> raw_results) { auto total_requests = static_cast(raw_results.size()); _loop->call_get([&] { @@ -603,3 +620,57 @@ TEST_CASE("Network", "[network][swarm_redirect]") { }); CHECK(sorted(after_refresh) == sorted(calculated)); } + +TEST_CASE("Network", "[network][strike_expiry]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 5min, + .cache_min_lifetime = 5min, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 0, + .cache_min_swarm_size = 0, + .cache_num_nodes_to_use_for_refresh = 0, + .cache_min_num_refresh_presence_to_include_node = 0, + .cache_node_strike_threshold = 3}; + + // Strikes are keyed by node pubkey, so these have to differ per node or striking one strikes + // every node sharing its key + std::vector> ed_pks{ + "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes, + "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes, + "e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5"_hexbytes, + "7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f"_hexbytes}; + std::vector snode_cache; + + for (uint16_t i = 0; i < ed_pks.size(); ++i) + snode_cache.emplace_back(service_node{ + ed25519_pubkey::from_bytes(ed_pks[i]), + oxen::quic::ipv4{"192.168.0.{}"_format(i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + 0}); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->reset_state_with_cache(snode_cache); + + snode_pool->record_node_failure(snode_cache[0], true); + snode_pool->record_node_failure(snode_cache[1], true); + REQUIRE(snode_pool->node_strike_count(snode_cache[0]) == 3); + REQUIRE(snode_pool->get_unused_nodes(4).size() == 2); + + // Once the strikes are older than `STRIKE_EXPIRY` the node has to become selectable again; a + // node dropped for a blip two days ago is not evidence about the node now + snode_pool->debug_age_strikes(49h); + CHECK(snode_pool->node_strike_count(snode_cache[0]) == 0); + CHECK(snode_pool->get_unused_nodes(4).size() == 4); + + // ... and a fresh failure starts from one rather than stacking onto the expired ones, which is + // also what stops the vector growing for the life of the process + snode_pool->record_node_failure(snode_cache[0]); + CHECK(snode_pool->node_strike_count(snode_cache[0]) == 1); + CHECK(snode_pool->debug_recorded_strikes(snode_cache[0].remote_pubkey) == 1); +} From 1159595138ddd6329c651ab6a644980fa7e54b7e Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 10:50:40 +1000 Subject: [PATCH 04/22] Drop a cached edge node once it has been struck out 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. --- .../network/routing/onion_request_router.hpp | 1 + include/session/network/snode_pool.hpp | 7 ++ src/network/routing/onion_request_router.cpp | 35 ++++++- src/network/snode_pool.cpp | 8 ++ tests/test_onion_request_router.cpp | 92 +++++++++++++++++++ 5 files changed, 140 insertions(+), 3 deletions(-) diff --git a/include/session/network/routing/onion_request_router.hpp b/include/session/network/routing/onion_request_router.hpp index 1dbbbcee3..da17f3c40 100644 --- a/include/session/network/routing/onion_request_router.hpp +++ b/include/session/network/routing/onion_request_router.hpp @@ -168,6 +168,7 @@ class OnionRequestRouter : public IRouter, public std::enable_shared_from_this { virtual void record_node_failure(const ed25519_pubkey& key, bool permanent = false); uint16_t node_strike_count(const service_node& node); uint16_t node_strike_count(const ed25519_pubkey& key); + + // Whether the node has collected enough unexpired strikes to be kept out of node selection. + // Callers that pick a node by some other route - a cached one, say - need this to apply the + // same bar `get_unused_nodes` does, rather than comparing a raw count to a threshold they'd + // have to know about. + virtual bool node_struck_out(const service_node& node); + virtual bool node_struck_out(const ed25519_pubkey& key); void clear_node_strikes(); // Checks if the pool is empty or stale and triggers a refresh if needed diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index 99f6c4549..9915dd453 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -595,9 +595,37 @@ void OnionRequestRouter::_finish_setup() { } } +// 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). +// +// Being struck out costs it the cached-edge-node role, not its place in the pool: it stays a node +// like any other and `get_unused_nodes` can pick it 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 - that is a second change of first hop, not a +// return to a stable one. +void OnionRequestRouter::_drop_struck_cached_edge_nodes() { + auto snode_pool = _snode_pool.lock(); + + if (!snode_pool) + return; + + std::erase_if(_cached_edge_nodes, [&snode_pool](const auto& cached) { + if (!snode_pool->node_struck_out(cached.node)) + return false; + + log::debug( + cat, + "Dropping cached edge node {}, it has been struck out.", + cached.node.to_string()); + return true; + }); +} + void OnionRequestRouter::_pre_build_paths_if_needed() { if (!_config.disable_pre_build_paths) { log::info(cat, "Pre-building initial paths."); + _drop_struck_cached_edge_nodes(); std::vector edge_nodes = _cached_edge_nodes; if (_config.single_path_mode) { @@ -2128,13 +2156,14 @@ void OnionRequestRouter::_rotate_path(const std::string& path_id, PathCategory c } // Get enough nodes for the path (if the edge node has been used for longer than the cache - // duration then we should create an entirely new path, otherwise we should try to reuse the - // edge node) + // duration, or has been struck out since we connected to it, then we should create an entirely + // new path, otherwise we should try to reuse the edge node) auto now = std::chrono::system_clock::now(); auto rotate_at = (std::chrono::steady_clock::now() + _config.path_rotation_frequency); std::vector rotated_path_nodes; - if (now > path.edge_first_connected_at + _config.edge_node_cache_duration) + if (now > path.edge_first_connected_at + _config.edge_node_cache_duration || + snode_pool->node_struck_out(edge_node)) rotated_path_nodes = snode_pool->get_unused_nodes(_config.path_length, nodes_to_exclude); else { rotated_path_nodes = diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index d4b9d7848..fe312431d 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1053,6 +1053,14 @@ uint16_t SnodePool::node_strike_count(const ed25519_pubkey& key) { [this, &key] { return static_cast(_active_strike_count(key)); }); } +bool SnodePool::node_struck_out(const service_node& node) { + return node_struck_out(node.remote_pubkey); +} + +bool SnodePool::node_struck_out(const ed25519_pubkey& key) { + return _loop->call_get([this, &key] { return _node_struck_out(key); }); +} + void SnodePool::clear_node_strikes() { // Use 'call_get' to force this to be synchronous _loop->call_get([this] { diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 8913315d8..910d0f217 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -50,6 +50,20 @@ class TestOnionRequestRouter { return 0; } + static void set_cached_edge_nodes( + std::shared_ptr router, std::vector nodes) { + router->_cached_edge_nodes = std::move(nodes); + } + + static std::vector cached_edge_nodes( + std::shared_ptr router) { + return router->_cached_edge_nodes; + } + + static void drop_struck_cached_edge_nodes(std::shared_ptr router) { + router->_drop_struck_cached_edge_nodes(); + } + static void build_path( std::shared_ptr router, PathCategory category, @@ -832,4 +846,82 @@ TEST_CASE("Network", "[network][onion_request_router][check_request_queue_timeou CHECK(result.timeout); } +TEST_CASE("Network", "[network][onion_request_router][cached_edge_nodes]") { + const auto node_strike_threshold = 3; + config::SnodePool pool_config = { + .cache_directory = std::nullopt, + .fallback_snode_pool_path = std::nullopt, + .cache_expiration = std::chrono::minutes{5}, + .cache_min_lifetime = std::chrono::minutes{5}, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .seed_nodes = {}, + .cache_min_size = 0, + .cache_min_swarm_size = 0, + .cache_num_nodes_to_use_for_refresh = 3, + .cache_min_num_refresh_presence_to_include_node = 2, + .cache_node_strike_threshold = node_strike_threshold}; + config::OnionRequestRouter config = { + file_server::DEFAULT_CONFIG, + std::nullopt, + std::chrono::days{10}, + opt::netid::Target::testnet, + {}, + network::opt::retry_delay{50ms, 200ms}, + 3, + 3, + 10, + 10min, + node_strike_threshold, + true, + true, + {{PathCategory::standard, 1}}}; + + auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; + auto ed_pk2 = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; + auto healthy = service_node{ + ed25519_pubkey::from_bytes(ed_pk), + oxen::quic::ipv4{"127.0.0.1"}, + 20001, + 30001, + {2, 11, 0}, + 0}; + auto struck = service_node{ + ed25519_pubkey::from_bytes(ed_pk2), + oxen::quic::ipv4{"127.0.0.2"}, + 20002, + 30002, + {2, 11, 0}, + 0}; + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + auto transport = std::make_shared(); + auto router = + std::make_shared(config, loop, disk_loop, snode_pool, transport); + + auto now = std::chrono::system_clock::now(); + TestOnionRequestRouter::set_cached_edge_nodes( + router, {cached_edge_node{healthy, now}, cached_edge_node{struck, now}}); + + // Both are well inside `edge_node_cache_duration`, which is the only thing that used to matter + TestOnionRequestRouter::drop_struck_cached_edge_nodes(router); + CHECK(TestOnionRequestRouter::cached_edge_nodes(router).size() == 2); + + // A cached edge node is forced as a path's first hop without going through `get_unused_nodes`, + // so nothing else would apply the strike filter to it. Striking it out has to cost it the + // cached-edge role outright - skipping it while keeping the entry would hand the role back when + // the strikes expire, long after we moved to another edge node. + snode_pool->SnodePool::record_node_failure(struck, true); + REQUIRE(snode_pool->node_struck_out(struck)); + + // Only the cached-edge role is lost; nothing here touches `_snode_cache`, so it stays a node + // like any other and `get_unused_nodes` can pick it again when its strikes expire + TestOnionRequestRouter::drop_struck_cached_edge_nodes(router); + auto remaining = TestOnionRequestRouter::cached_edge_nodes(router); + REQUIRE(remaining.size() == 1); + CHECK(remaining.front().node == healthy); +} } // namespace session::network From 6446f67a4c2d431dc1bdda95eb8a0bac00cb4056 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 10:46:00 +1000 Subject: [PATCH 05/22] Tell callers when the snode pool could not be refreshed `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. --- include/session/network/session_network.hpp | 2 +- include/session/network/snode_pool.hpp | 14 +++- src/network/routing/onion_request_router.cpp | 52 ++++++++++-- src/network/routing/session_router_router.cpp | 20 ++++- src/network/session_network.cpp | 48 +++++++++-- src/network/snode_pool.cpp | 75 +++++++++++------ tests/test_onion_request_router.cpp | 2 +- tests/test_snode_pool.cpp | 81 +++++++++++++++++-- 8 files changed, 240 insertions(+), 54 deletions(-) diff --git a/include/session/network/session_network.hpp b/include/session/network/session_network.hpp index cc495e9c8..2cc2c237b 100644 --- a/include/session/network/session_network.hpp +++ b/include/session/network/session_network.hpp @@ -128,7 +128,7 @@ class Network : public std::enable_shared_from_this { const uint8_t index, const service_node& node, const uint8_t total_requests); - void _on_clock_resync_complete(const uint8_t total_requests); + void _on_clock_resync_complete(); Request _preprocess_request(Request request); }; diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 6eb5ed73a..64515c565 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -42,6 +42,12 @@ class empty_file_exception : public std::runtime_error { class SnodePool : public std::enable_shared_from_this { public: using network_fetcher_t = std::function; + + // `refreshed` is false when the pool could not be refreshed at all - suspended, no candidate + // nodes, no fetcher. Callers that retry their work on this callback have to check it: without + // it the only way to signal "that didn't happen" was to never call back, which leaves whatever + // the caller set up beforehand set up forever. + using refresh_callback_t = std::function; using fetcher_connectivity_check_t = std::function; SnodePool( @@ -81,15 +87,14 @@ class SnodePool : public std::enable_shared_from_this { // Checks if the pool is empty or stale and triggers a refresh if needed virtual void refresh_if_needed( const std::vector& in_use_nodes, - std::function on_refresh_complete = nullptr); + refresh_callback_t on_refresh_complete = nullptr); // Re-resolves `swarm_pubkey` after a node we believed was in its swarm has told us the account // isn't there. Where `refresh_if_needed` decides on the cache's age, this decides on that // rejection, so a mapping we have direct proof is wrong can be replaced long before // `cache_expiration`. `on_complete` runs once the swarm can be resolved again. virtual void invalidate_swarm( - session::network::x25519_pubkey swarm_pubkey, - std::function on_complete = nullptr); + session::network::x25519_pubkey swarm_pubkey, refresh_callback_t on_complete = nullptr); // Records the swarm a node named when it rejected a request for `swarm_pubkey`, so the mapping // can be corrected without refreshing the whole pool - a swarm change would otherwise have @@ -149,7 +154,7 @@ class SnodePool : public std::enable_shared_from_this { int _snode_cache_refresh_failure_count = 0; std::vector _refresh_candidate_nodes; std::vector> _snode_refresh_results; - std::vector> _after_snode_cache_refresh; + std::vector _after_snode_cache_refresh; // Counts a node's strikes that haven't expired yet. `record_node_failure` only appends, so the // raw vector answers a different question - every strike the node has ever collected. @@ -186,6 +191,7 @@ class SnodePool : public std::enable_shared_from_this { const bool use_direct_fetcher, const uint8_t total_requests); void _update_cache(std::string refresh_id, std::vector nodes); + void _run_pending_refresh_callbacks(bool refreshed); }; } // namespace session::network diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index 9915dd453..de0b947aa 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -331,7 +331,12 @@ OnionRequestRouter::OnionRequestRouter( } if (snode_pool->size() == 0) - snode_pool->refresh_if_needed({}, [weak_self = weak_from_this()] { + snode_pool->refresh_if_needed({}, [weak_self = weak_from_this()](bool refreshed) { + // Setup finishes either way: not finishing leaves the router permanently unusable, + // where finishing with an empty pool just means the first request refreshes again + if (!refreshed) + log::warning(cat, "Finishing router setup without a refreshed snode pool."); + if (auto self = weak_self.lock()) self->_loop->call([weak_self] { if (auto self = weak_self.lock()) @@ -1188,15 +1193,50 @@ void OnionRequestRouter::_build_path( snode_pool->refresh_if_needed( nodes_to_exclude, - [weak_self = weak_from_this(), - this, - category, - initiating_req_id, - nodes_to_exclude]() { + [weak_self = weak_from_this(), this, category, initiating_req_id, nodes_to_exclude]( + bool refreshed) { auto self = weak_self.lock(); if (!self) return; + // Rebuilding without a refresh lands back in this same branch with the same + // too-few nodes, so the queued requests would wait on a loop that cannot end + if (!refreshed) { + log::error( + cat, + "[Request {}]: Cannot build a path, the snode pool could not be " + "refreshed.", + initiating_req_id.value_or("internal")); + _update_status(); + + auto queue_it = _request_queues.find(category); + if (queue_it == _request_queues.end()) { + log::critical( + cat, + "No request queue for category '{}'.", + to_string(category, _config.single_path_mode)); + return; + } + + if (!queue_it->second->is_empty()) { + auto to_fail = queue_it->second->pop_all(); + log::error( + cat, + "Failing {} queued requests for '{}' paths; the snode pool " + "could not be refreshed.", + to_fail.size(), + to_string(category, _config.single_path_mode)); + + for (const auto& [req, cb] : to_fail) + cb(false, + false, + -1, + {content_type_plain_text}, + "Failed to refresh the snode pool to build a path."); + } + return; + } + log::info( cat, "[Request {}]: SnodePool refresh complete, retrying path build.", diff --git a/src/network/routing/session_router_router.cpp b/src/network/routing/session_router_router.cpp index cf1606d05..5dd80ecbd 100644 --- a/src/network/routing/session_router_router.cpp +++ b/src/network/routing/session_router_router.cpp @@ -131,11 +131,19 @@ void SessionRouter::_init() { return; if (snode_pool->size() == 0) - snode_pool->refresh_if_needed({}, [weak_self, this] { + snode_pool->refresh_if_needed({}, [weak_self, this](bool refreshed) { auto self = weak_self.lock(); if (!self) return; + // Setup finishes either way: not finishing leaves the router + // permanently unusable, where finishing with an empty pool just means + // the first request refreshes again + if (!refreshed) + log::warning( + cat, + "Finishing router setup without a refreshed snode pool."); + _loop->call([weak_self] { if (auto self = weak_self.lock()) self->_finish_setup(); @@ -458,11 +466,19 @@ void SessionRouter::_send_proxy_request(Request request, network_response_callba [weak_self = weak_from_this(), this, req = std::move(request), - cb = std::move(callback)]() { + cb = std::move(callback)](bool refreshed) { auto self = weak_self.lock(); if (!self) return; + if (!refreshed) + return cb( + false, + false, + -1, + {content_type_plain_text}, + "Failed to refresh the snode pool to find a proxy."); + auto snode_pool = _snode_pool.lock(); if (!snode_pool) return cb( diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 24d4babce..1fdce62f1 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -410,8 +410,19 @@ void Network::get_random_nodes( std::vector nodes_to_exclude = _router->get_all_used_nodes(); return _snode_pool->refresh_if_needed( - nodes_to_exclude, - [this, count, cb = std::move(cb)] { get_random_nodes(count, cb); }); + nodes_to_exclude, [this, count, cb = std::move(cb)](bool refreshed) { + // Retrying without a refresh would re-enter this same branch and recurse + // until the stack gives out + if (!refreshed) { + log::warning( + cat, + "Cannot get {} random nodes: the pool could not be refreshed.", + count); + return cb({}); + } + + get_random_nodes(count, cb); + }); } cb(unused_nodes); }); @@ -868,7 +879,17 @@ void Network::_handle_421_retry( swarm_pubkey, req_to_retry = std::move(original_request), cb = std::move(final_callback), - failed_node = failed_node_copy]() mutable { + failed_node = failed_node_copy](bool swarm_resolved) mutable { + // Without a re-resolve the swarm cache still holds the mapping the 421 disproved, so the + // retry would go straight back to the swarm that just rejected us + if (!swarm_resolved) + return cb( + false, + false, + ERROR_MISDIRECTED_REQUEST, + {content_type_plain_text}, + "421 Misdirected Request, and the swarm could not be re-resolved"); + _snode_pool->get_swarm( swarm_pubkey, false, @@ -910,7 +931,7 @@ void Network::_handle_421_retry( // Taking its word (bounded - see `record_swarm_redirect`) fixes the one mapping we know is // wrong, where refreshing has every client of a changed swarm fetch the full node list for it. if (_snode_pool->record_swarm_redirect(swarm_pubkey, redirect_node_keys(response_body))) - return retry_on_resolved_swarm(); + return retry_on_resolved_swarm(true); // No usable redirect, so the pool itself is the only thing that can put this right _snode_pool->invalidate_swarm(swarm_pubkey, std::move(retry_on_resolved_swarm)); @@ -969,7 +990,20 @@ void Network::_resync_clock( // Refresh the snode pool if needed to ensure we have the most up-to-date cache std::vector nodes_to_exclude = _router->get_all_used_nodes(); - _snode_pool->refresh_if_needed(std::move(nodes_to_exclude), [this, request_id] { + _snode_pool->refresh_if_needed(std::move(nodes_to_exclude), [this, request_id](bool refreshed) { + // `_current_clock_resync_id` was set before this call, so giving up without clearing it + // blocks every later resync attempt for the life of the process and strands everything + // queued behind it. Completing with no results takes the existing "resync finished, + // successful or not" path, which does both and leaves the offset we already had alone - + // an old offset beats none, and this says nothing about whether it was right. + if (!refreshed) { + log::warning( + cat, + "[Request {}] Abandoning clock resync: the snode pool could not be refreshed.", + request_id); + return _on_clock_resync_complete(); + } + // Pick the random nodes we want to use for retrying (these won't change for this resync // attempt) auto resync_nodes = @@ -1076,11 +1110,11 @@ void Network::_launch_next_clock_out_of_sync_request( // If we've received all the results then we need to process them and complete the // resync if (_clock_resync_results.size() >= total_requests) - _on_clock_resync_complete(total_requests); + _on_clock_resync_complete(); }); } -void Network::_on_clock_resync_complete(const uint8_t /*total_requests*/) { +void Network::_on_clock_resync_complete() { auto raw_results = std::move(_clock_resync_results); auto refresh_id = std::move(*_current_clock_resync_id); diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index fe312431d..ac4e2a8e0 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -355,6 +355,11 @@ void SnodePool::_refresh_snode_cache(std::optional request_id_opt) _loop->call([this, request_id_opt] { if (_suspended) { log::info(cat, "Ignoring refresh as pool is suspended."); + + // Anything queued against a refresh we are declining to start has nothing left to wait + // for, unless one is already in flight and still owns it + if (!_current_snode_cache_refresh_id) + _run_pending_refresh_callbacks(false); return; } @@ -432,6 +437,7 @@ void SnodePool::_refresh_snode_cache(std::optional request_id_opt) (use_seed_nodes ? "No seed nodes are configured!" : "Found no nodes and decided not to use seed nodes!")); _current_snode_cache_refresh_id.reset(); + _run_pending_refresh_callbacks(false); return; } @@ -586,6 +592,7 @@ void SnodePool::_launch_next_refresh_request( cat, "[Request {}] No fetcher available, aborting refresh.", target_request_id); _current_snode_cache_refresh_id.reset(); _refresh_candidate_nodes.clear(); + _run_pending_refresh_callbacks(false); return; } @@ -910,28 +917,35 @@ void SnodePool::_update_cache(std::string refresh_id, std::vector SnodePool::_perform_cache_write(path, cache); }); - // Trigger any callbacks - // - // These must be moved out of the member before being run: a callback can re-enter the pool - // and register another post-refresh callback (`get_swarm` does exactly that if the cache is - // still empty), which would reallocate the vector we're iterating and leave us calling - // through a freed `std::function`. Anything registered while we're here accumulates in the - // now-empty member and runs after the next refresh instead of being silently discarded. - if (!_after_snode_cache_refresh.empty()) { - auto callbacks = std::move(_after_snode_cache_refresh); - _after_snode_cache_refresh.clear(); // A moved-from vector is valid but unspecified + _run_pending_refresh_callbacks(true); + }); +} - log::debug(cat, "Executing {} post-refresh callbacks.", callbacks.size()); +void SnodePool::_run_pending_refresh_callbacks(bool refreshed) { + if (_after_snode_cache_refresh.empty()) + return; - for (const auto& cb : callbacks) { - try { - cb(); - } catch (const std::exception& e) { - log::error(cat, "Exception thrown in a post-refresh callback: {}", e.what()); - } - } + // These must be moved out of the member before being run: a callback can re-enter the pool and + // register another post-refresh callback (`get_swarm` does exactly that if the cache is still + // empty), which would reallocate the vector we're iterating and leave us calling through a + // freed `std::function`. Anything registered while we're here accumulates in the now-empty + // member and runs after the next refresh instead of being silently discarded. + auto callbacks = std::move(_after_snode_cache_refresh); + _after_snode_cache_refresh.clear(); // A moved-from vector is valid but unspecified + + log::debug( + cat, + "Executing {} post-refresh callbacks ({}).", + callbacks.size(), + (refreshed ? "refreshed" : "refresh did not happen")); + + for (const auto& cb : callbacks) { + try { + cb(refreshed); + } catch (const std::exception& e) { + log::error(cat, "Exception thrown in a post-refresh callback: {}", e.what()); } - }); + } } // MARK: Public Functions @@ -1074,10 +1088,13 @@ void SnodePool::clear_node_strikes() { } void SnodePool::refresh_if_needed( - const std::vector& in_use_nodes, std::function on_refresh_complete) { + const std::vector& in_use_nodes, refresh_callback_t on_refresh_complete) { _loop->call([this, in_use_nodes, cb = std::move(on_refresh_complete)] { if (_suspended) { log::info(cat, "Ignoring refresh as pool is suspended."); + + if (cb) + cb(false); return; } @@ -1141,17 +1158,19 @@ void SnodePool::refresh_if_needed( } else _refresh_snode_cache(); else if (!already_running && cb) - cb(); + // Nothing needed refreshing, which is the answer the caller wanted rather than a + // failure to get one + cb(true); }); } -void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, std::function on_complete) { +void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, refresh_callback_t on_complete) { _loop->call([this, swarm_pubkey, cb = std::move(on_complete)]() mutable { if (_suspended) { log::info(cat, "Ignoring swarm invalidation as pool is suspended."); if (cb) - cb(); + cb(false); return; } @@ -1209,7 +1228,8 @@ void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, std::functionget_swarm(swarm_pubkey, ignore_strike_count, std::move(cb)); }); diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 910d0f217..380dd2315 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -168,7 +168,7 @@ namespace { void refresh_if_needed( const std::vector& /*in_use_nodes*/, - std::function /*on_refresh_complete*/ = nullptr) override { + refresh_callback_t /*on_refresh_complete*/ = nullptr) override { func_called("refresh_if_needed"); // Do nothing (don't want to trigger a cache refresh) } diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 3518f0363..63df35675 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -32,11 +32,11 @@ class TestSnodePool : public SnodePool { void refresh_if_needed( const std::vector& /*in_use_nodes*/, - std::function /*on_refresh_complete*/ = nullptr) override { + refresh_callback_t /*on_refresh_complete*/ = nullptr) override { // Do nothing (don't want to trigger a cache refresh) } - void debug_queue_post_refresh_callback(std::function cb) { + void debug_queue_post_refresh_callback(refresh_callback_t cb) { _loop->call_get([this, cb = std::move(cb)]() mutable { _after_snode_cache_refresh.push_back(std::move(cb)); }); @@ -110,6 +110,14 @@ class TestSnodePool : public SnodePool { }); } + // Reaches past the no-op override above to the real age-based policy, on the loop thread so it + // resolves inline + void debug_refresh_if_needed(refresh_callback_t cb) { + _loop->call_get([this, cb = std::move(cb)]() mutable { + SnodePool::refresh_if_needed({}, std::move(cb)); + }); + } + void debug_on_refresh_complete(std::vector> raw_results) { auto total_requests = static_cast(raw_results.size()); _loop->call_get([&] { @@ -126,7 +134,7 @@ class TestSnodePoolAgePolicy : public TestSnodePool { void refresh_if_needed( const std::vector& in_use_nodes, - std::function on_refresh_complete = nullptr) override { + refresh_callback_t on_refresh_complete = nullptr) override { SnodePool::refresh_if_needed(in_use_nodes, std::move(on_refresh_complete)); } }; @@ -317,12 +325,12 @@ TEST_CASE("Network", "[network][update_cache]") { // what a deferred `get_swarm` does when the refresh left the cache empty) rather than // invalidating the vector it's iterating std::vector callbacks_run; - snode_pool->debug_queue_post_refresh_callback([&] { + snode_pool->debug_queue_post_refresh_callback([&](bool) { callbacks_run.push_back(0); - snode_pool->debug_queue_post_refresh_callback([&] { callbacks_run.push_back(3); }); + snode_pool->debug_queue_post_refresh_callback([&](bool) { callbacks_run.push_back(3); }); }); - snode_pool->debug_queue_post_refresh_callback([&] { callbacks_run.push_back(1); }); - snode_pool->debug_queue_post_refresh_callback([&] { callbacks_run.push_back(2); }); + snode_pool->debug_queue_post_refresh_callback([&](bool) { callbacks_run.push_back(1); }); + snode_pool->debug_queue_post_refresh_callback([&](bool) { callbacks_run.push_back(2); }); REQUIRE(snode_pool->debug_remove_post_refresh_callback_spare_capacity()); snode_pool->update_cache({}); CHECK(callbacks_run == std::vector{0, 1, 2}); @@ -450,8 +458,9 @@ TEST_CASE("Network", "[network][invalidate_swarm]") { snode_pool->refresh_if_needed({}); age_policy_started_refresh = snode_pool->debug_refresh_in_progress(); - snode_pool->invalidate_swarm(swarm_pubkey, [&] { + snode_pool->invalidate_swarm(swarm_pubkey, [&](bool refreshed) { recovery_ran = true; + CHECK(refreshed); snode_pool->get_swarm( swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { recovered_swarm = std::move(nodes); @@ -674,3 +683,59 @@ TEST_CASE("Network", "[network][strike_expiry]") { CHECK(snode_pool->node_strike_count(snode_cache[0]) == 1); CHECK(snode_pool->debug_recorded_strikes(snode_cache[0].remote_pubkey) == 1); } + +TEST_CASE("Network", "[network][refresh_callback_contract]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .seed_nodes = {}, + .cache_min_size = 0, + .cache_min_swarm_size = 0, + .cache_num_nodes_to_use_for_refresh = 0, + .cache_min_num_refresh_presence_to_include_node = 0, + .cache_node_strike_threshold = 3}; + auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; + std::vector snode_cache; + + for (uint16_t i = 0; i < 5; ++i) + snode_cache.emplace_back(service_node{ + ed25519_pubkey::from_bytes(ed_pk), + oxen::quic::ipv4{"192.168.0.{}"_format(i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + 0}); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + + auto run = [&snode_pool] { + std::pair result{false, true}; // {called, refreshed} + snode_pool->debug_refresh_if_needed( + [&result](bool refreshed) { result = {true, refreshed}; }); + return result; + }; + + // An empty cache and no seed nodes means the refresh cannot even start. The callback still has + // to run: callers set state up before calling (`_resync_clock` sets its in-progress id) and + // never hearing back leaves that set for the life of the process. + auto [called, refreshed] = run(); + CHECK(called); + CHECK_FALSE(refreshed); + + // Nothing needing a refresh is the answer the caller asked for, not a failure to get one + snode_pool->update_cache(snode_cache); + std::tie(called, refreshed) = run(); + CHECK(called); + CHECK(refreshed); + + // ... and a suspended pool reports that it didn't refresh rather than going quiet + snode_pool->suspend(); + std::tie(called, refreshed) = run(); + CHECK(called); + CHECK_FALSE(refreshed); +} From 9c274741cbe4f567d079fd64481c49651ae1fe3e Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 15:45:09 -0300 Subject: [PATCH 06/22] Follow a 421's redirect for each account a batch rejects 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. --- include/session/network/session_network.hpp | 11 +- .../session/network/session_network_types.h | 4 +- .../session/network/session_network_types.hpp | 61 +++++- src/network/session_network.cpp | 150 ++++++++------- src/network/session_network_types.cpp | 160 ++++++++++++++-- tests/test_network_swarm.cpp | 181 ++++++++++++++++++ 6 files changed, 469 insertions(+), 98 deletions(-) diff --git a/include/session/network/session_network.hpp b/include/session/network/session_network.hpp index 2cc2c237b..cb1ce2099 100644 --- a/include/session/network/session_network.hpp +++ b/include/session/network/session_network.hpp @@ -115,10 +115,17 @@ class Network : public std::enable_shared_from_this { void _close_connections(); void _recalculate_status(); void _update_status(ConnectionStatus new_status); - void _update_network_state(const std::string& body); + void _update_network_state(const nlohmann::json& json); + + // An account's redirect is left empty where there was none to take. + response::swarm_rejections _take_swarm_redirects( + const Request& request, const nlohmann::json* json, int16_t status_code); + void _refresh_if_unredirected(const response::swarm_rejections& rejections); void _handle_421_retry( Request original_request, - std::optional response_body, + response::swarm_rejections rejections, + std::vector> headers, + std::optional body, network_response_callback_t final_callback); void _resync_clock( diff --git a/include/session/network/session_network_types.h b/include/session/network/session_network_types.h index a830cdebb..f312d5381 100644 --- a/include/session/network/session_network_types.h +++ b/include/session/network/session_network_types.h @@ -66,7 +66,9 @@ typedef struct { // The account whose swarm this request addresses, as 64 hex chars of its X25519 pubkey — the // same value passed to session_network_get_swarm() to choose the destination node. Null for a // request aimed at a node rather than about an account (a cache refresh, a clock resync) or at - // a server, which no swarm membership applies to. + // a server, which no swarm membership applies to. For a `batch` or `sequence` request each + // sub-request's account is read from the body instead, and this is used only if the body + // can't be parsed. // // Set it whenever there is one: a storage server rejects a request whose pubkey is outside its // swarm with a 421, and recovering means re-resolving this account's swarm. Left null, such a diff --git a/include/session/network/session_network_types.hpp b/include/session/network/session_network_types.hpp index 266cbf12b..3b255321c 100644 --- a/include/session/network/session_network_types.hpp +++ b/include/session/network/session_network_types.hpp @@ -2,7 +2,9 @@ #include #include +#include #include +#include #include #include "session/network/key_types.hpp" @@ -166,15 +168,16 @@ struct Request { /// Any extra request details which may modify the structure of the request. RequestDetails details; - /// The account whose swarm this request addresses, when it addresses one: the X25519 pubkey of - /// a session ID, group ID, etc. Set it for anything sent to a swarm member *about* that - /// account, and leave it unset for a request merely aimed at a node (a snode cache refresh, a - /// clock resync), which no swarm membership applies to. + /// The accounts this request addresses: one entry per sub-request of a `batch` or `sequence`, + /// in order, or a single entry for any other request. Each is the 32-byte pubkey an account's + /// swarm is computed from (a session ID or group ID without its prefix); an entry is empty for + /// a sub-request that isn't about an account. A single entry applies to every sub-request. + /// Leave it empty for a request merely aimed at a node (a snode cache refresh, a clock resync). /// - /// Required to recover from a 421: the storage server rejects a request whose pubkey is not in - /// its swarm, and recovering means re-resolving the swarm of *this account*, which cannot be - /// derived from the node we happened to ask. - std::optional swarm_pubkey; + /// Required to recover from a 421: it is what says whose swarm the redirect a 421 carries is + /// for. The account a storage server names in a 421 is only checked against it, never trusted + /// in its place. + std::vector> swarm_pubkeys; /// The time the request was created, this is used primarily for determining whether the /// `overall_timeout` has been exceeded. @@ -266,9 +269,51 @@ using network_response_callback_t = std::function> headers, std::optional response)>; +/// The accounts of each sub-request in a pregenerated `batch`/`sequence` body, in the form +/// `Request::swarm_pubkeys` takes, for a request whose body was built by the caller. Returns +/// nullopt when `endpoint` is neither or the body can't be parsed. +std::optional>> batch_request_accounts( + std::string_view endpoint, std::span body); + namespace response { std::optional> parse_text_error(std::string_view body); std::optional find_uniform_batch_error(std::string_view body); + + /// As `find_uniform_batch_error`, for a body that has already been parsed. + std::optional uniform_batch_error(const nlohmann::json& json); + + /// One result of a `batch`/`sequence` response, or the whole of any other response. + struct subresponse { + /// The result's own status code; nullopt for a response that isn't a batch, whose status + /// is the one it came back with. + std::optional code; + + /// The result's body when it is a JSON object. Points into the parsed response. + const nlohmann::json* body = nullptr; + }; + + /// The sub-responses of a parsed response: one per entry of `results` for a batch or sequence, + /// otherwise the response itself as the only one. The returned bodies point into `json`. + std::vector subresponses(const nlohmann::json& json); + + /// The accounts a response's 421s rejected. + struct swarm_rejections { + /// Each rejected account, with the ed25519 pubkeys of the swarm its 421 redirected it to: + /// empty when the 421 named no swarm. An account with several 421s takes the first. + std::unordered_map> accounts; + + /// Whether a 421 could not be attributed to any account. + bool unattributed = false; + }; + + /// Finds the 421s in a storage server response. `json` is its parsed body, or null when the + /// body isn't JSON. `status_code` is the response's own status, which is the only status a + /// response that isn't a batch has. `accounts` is the request's `swarm_pubkeys`, which each + /// 421 is attributed by; a 421 naming a different account than its position does is ignored. + swarm_rejections find_swarm_rejections( + const nlohmann::json* json, + int16_t status_code, + std::span> accounts); } // namespace response struct OnionPathMetadata { diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 1fdce62f1..0e7c8d73c 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -468,20 +468,34 @@ void Network::send_request(Request request, network_response_callback_t callback const auto dest_is_snode = std::holds_alternative(original_req.destination); + // Parsed once for everything below, since a retrieve's body can be large + std::optional json; + if (body) + if (auto parsed = nlohmann::json::parse(*body, nullptr, false); + !parsed.is_discarded()) + json = std::move(parsed); + // If we got a successful response (with a body) and the request was sent to a // service node then we should update the network state based on the response // (Note: we don't want to do this for server requests because they could // include values in different formats, eg. the "Session Network" API returns // `t` in seconds) - if (success && body && dest_is_snode) - _update_network_state(*body); + if (success && json && dest_is_snode) + _update_network_state(*json); int16_t final_status_code = status_code; - if (body) - if (auto uniform_error = response::find_uniform_batch_error(*body)) + if (json) + if (auto uniform_error = response::uniform_batch_error(*json)) final_status_code = *uniform_error; + // Taken from every snode response rather than only a 421 one: a batch can be + // rejected for some accounts and answered for others + response::swarm_rejections rejections; + if (dest_is_snode) + rejections = _take_swarm_redirects( + original_req, json ? &*json : nullptr, status_code); + // If we got a 406 from a snode, or a 425 from a server, then the device clock // is out of sync so we need to kick off a clock resync request if ((final_status_code == ERROR_NOT_ACCEPTABLE && dest_is_snode) || @@ -494,10 +508,17 @@ void Network::send_request(Request request, network_response_callback_t callback // cache, the original request might succeed after this refresh so we should // just automatically retry if (final_status_code == 421) { - _handle_421_retry(std::move(original_req), std::move(body), std::move(cb)); + _handle_421_retry( + std::move(original_req), + std::move(rejections), + std::move(headers), + std::move(body), + std::move(cb)); return; } + _refresh_if_unredirected(rejections); + // For debugging purposes we want to add a log if this was a successful request // after we did an automatic retry if (original_req.retry_count > 0) @@ -715,37 +736,22 @@ Request Network::_preprocess_request(Request request) { return request; } -void Network::_update_network_state(const std::string& body) { +void Network::_update_network_state(const nlohmann::json& json) { try { - auto json = nlohmann::json::parse(body); const nlohmann::json* target_json = &json; // If it was a batch/sequence request then take the one with the highest "t" value as that // would have been the one which was returned last - if (json.contains("results") && json["results"].is_array()) { - log::trace(cat, "Parsing batch response for latest network state."); - - int64_t max_t = -1; - const nlohmann::json* latest_body = nullptr; - - for (const auto& result : json["results"]) { - if (!result.is_object() || !result.contains("body") || !result["body"].is_object()) - continue; - - const auto& result_body = result["body"]; - - if (result_body.contains("t") && result_body["t"].is_number()) { - int64_t current_t = result_body["t"].get(); + int64_t max_t = -1; + for (const auto& sub : response::subresponses(json)) { + if (!sub.body) + continue; - if (current_t > max_t) { - max_t = current_t; - latest_body = &result_body; - } + if (auto t = sub.body->find("t"); t != sub.body->end() && t->is_number()) + if (auto current_t = t->get(); current_t > max_t) { + max_t = current_t; + target_json = sub.body; } - } - - if (latest_body) - target_json = latest_body; } // Update hardfork/softfork versions @@ -791,46 +797,40 @@ void Network::_update_network_state(const std::string& body) { // MARK: Specific Error Handling -// Pulls the node pubkeys out of a 421 body. Only the pubkeys: the rest of each record is contact -// information we deliberately don't read, so a redirect can't put us in touch with anything the -// pool hasn't already told us about. -static std::vector redirect_node_keys(const std::optional& body) { - std::vector keys; - - if (!body) - return keys; +// A node that rejects a request for an account usually names the swarm it actually belongs to. +// Taking its word (bounded - see `record_swarm_redirect`) fixes the one mapping we know is wrong, +// where refreshing has every client of a changed swarm fetch the full node list for it. +response::swarm_rejections Network::_take_swarm_redirects( + const Request& request, const nlohmann::json* json, int16_t status_code) { + auto rejections = response::find_swarm_rejections(json, status_code, request.swarm_pubkeys); - try { - auto json = nlohmann::json::parse(*body); - - if (!json.contains("snodes") || !json["snodes"].is_array()) - return keys; - - for (const auto& entry : json["snodes"]) { - if (!entry.contains("pubkey_ed25519") || !entry["pubkey_ed25519"].is_string()) - continue; + for (auto& [account, keys] : rejections.accounts) + if (!keys.empty() && !_snode_pool->record_swarm_redirect(account, keys)) + keys.clear(); - auto hex = entry["pubkey_ed25519"].get(); - - if (hex.size() == 64 && oxenc::is_hex(hex)) - keys.push_back(ed25519_pubkey::from_hex(hex)); - } - } catch (const std::exception& e) { - log::debug(cat, "Could not read a swarm redirect from the 421 body: {}", e.what()); - } + return rejections; +} - return keys; +// With no usable redirect the pool itself is the only thing that can put a rejection right, and +// one refresh re-resolves every account at once +void Network::_refresh_if_unredirected(const response::swarm_rejections& rejections) { + for (const auto& [account, keys] : rejections.accounts) + if (keys.empty()) + return _snode_pool->invalidate_swarm(account); } void Network::_handle_421_retry( Request original_request, - std::optional response_body, + response::swarm_rejections rejections, + std::vector> headers, + std::optional body, network_response_callback_t final_callback) { if (original_request.retry_count >= config.redirect_retry_count) { log::error( cat, "Request {} received 421 but exceeded max retry count.", original_request.request_id); + _refresh_if_unredirected(rejections); return final_callback( false, false, @@ -851,12 +851,12 @@ void Network::_handle_421_retry( "Received 421 from a non-service-node destination"); // A 421 says the account we asked about is not in this node's swarm, so recovering means - // re-resolving *that account's* swarm. Nothing about the node we asked can tell us which - // account that was, so a request that did not record one cannot be redirected. - if (!original_request.swarm_pubkey) { + // re-resolving *that account's* swarm; without knowing which account that was, there is + // nothing to re-resolve. + if (rejections.accounts.empty()) { log::warning( cat, - "Request {} received 421 but carries no swarm pubkey to re-resolve.", + "Request {} received 421 but names no account to re-resolve.", original_request.request_id); return final_callback( false, @@ -866,6 +866,19 @@ void Network::_handle_421_retry( "421 Misdirected Request for a request with no swarm"); } + // A retry goes to a single swarm, which a batch spanning several accounts doesn't have. Their + // redirects are already taken, so the next request for each goes to the right place. + if (rejections.accounts.size() > 1 || rejections.unattributed) { + log::info( + cat, + "Request {} received 421s for {} accounts; not retrying it as a whole.", + original_request.request_id, + rejections.accounts.size()); + _refresh_if_unredirected(rejections); + return final_callback( + false, false, ERROR_MISDIRECTED_REQUEST, std::move(headers), std::move(body)); + } + log::info( cat, "Request {} received 421 from node {}, re-resolving its swarm.", @@ -873,7 +886,7 @@ void Network::_handle_421_retry( original_dest_node->to_string()); auto failed_node_copy = *original_dest_node; - auto swarm_pubkey = *original_request.swarm_pubkey; + auto [swarm_pubkey, redirect] = *rejections.accounts.begin(); auto retry_on_resolved_swarm = [this, swarm_pubkey, @@ -927,13 +940,9 @@ void Network::_handle_421_retry( }); }; - // A node that rejects a request for an account usually names the swarm it actually belongs to. - // Taking its word (bounded - see `record_swarm_redirect`) fixes the one mapping we know is - // wrong, where refreshing has every client of a changed swarm fetch the full node list for it. - if (_snode_pool->record_swarm_redirect(swarm_pubkey, redirect_node_keys(response_body))) + if (!redirect.empty()) return retry_on_resolved_swarm(true); - // No usable redirect, so the pool itself is the only thing that can put this right _snode_pool->invalidate_swarm(swarm_pubkey, std::move(retry_on_resolved_swarm)); } @@ -1835,8 +1844,13 @@ LIBSESSION_C_API void session_network_send_request( std::nullopt, request_id}; - if (params->swarm_pubkey_hex) - request.swarm_pubkey = x25519_pubkey::from_hex({params->swarm_pubkey_hex, 64}); + std::optional>> batch_accounts; + if (request.body) + batch_accounts = batch_request_accounts(request.endpoint, *request.body); + if (batch_accounts) + request.swarm_pubkeys = std::move(*batch_accounts); + else if (params->swarm_pubkey_hex) + request.swarm_pubkeys = {x25519_pubkey::from_hex({params->swarm_pubkey_hex, 64})}; auto cpp_callback = [c_cb = callback, c_ctx = ctx]( bool success, diff --git a/src/network/session_network_types.cpp b/src/network/session_network_types.cpp index 4b40e1325..163fa0be6 100644 --- a/src/network/session_network_types.cpp +++ b/src/network/session_network_types.cpp @@ -73,35 +73,157 @@ std::optional> response::parse_text_error(std::string_v return std::nullopt; } +namespace { + inline auto cat = log::Cat("network"); + + const nlohmann::json* field( + const nlohmann::json& j, + std::string_view key, + bool (nlohmann::json::*is_kind)() const noexcept) { + auto it = j.find(key); + return it != j.end() && ((*it).*is_kind)() ? &*it : nullptr; + } + + std::optional account_in(const nlohmann::json& obj) { + auto pk = field(obj, "pubkey", &nlohmann::json::is_string); + if (!pk) + return std::nullopt; + + // Prefixed with the network id, except on testnet + auto hex = pk->get(); + if (hex.size() == 66) + hex.remove_prefix(2); + return x25519_pubkey::maybe_from_hex(hex); + } + + // Only the pubkeys: the rest of each record is contact information we deliberately don't read, + // so a redirect can't put us in touch with anything the pool hasn't already told us about. + std::vector redirect_keys(const nlohmann::json& body) { + std::vector keys; + + if (auto snodes = field(body, "snodes", &nlohmann::json::is_array)) + for (const auto& snode : *snodes) + if (auto pk = field(snode, "pubkey_ed25519", &nlohmann::json::is_string)) + if (auto key = ed25519_pubkey::maybe_from_hex(pk->get())) + keys.push_back(*key); + + return keys; + } + + std::optional account_by_position( + std::span> accounts, size_t index) { + if (accounts.size() == 1) + return accounts.front(); + return index < accounts.size() ? accounts[index] : std::nullopt; + } +} // namespace + std::optional response::find_uniform_batch_error(std::string_view body) { try { - auto json = nlohmann::json::parse(body); + return uniform_batch_error(nlohmann::json::parse(body)); + } catch (...) { /* Do nothing */ + } - // If it wasn't a batch response then just handle the non-batch status code - if (json.contains("results") && json["results"].is_array() && !json["results"].empty()) { - int16_t first_status_code = -1; + return std::nullopt; +} + +std::optional response::uniform_batch_error(const nlohmann::json& json) { + std::optional uniform; - for (const auto& result : json["results"]) { - if (!result.contains("code") || !result["code"].is_number()) - return std::nullopt; + // A response that isn't a batch has no per-result codes, so its own status stands + for (const auto& sub : subresponses(json)) { + if (!sub.code || (*sub.code >= 200 && *sub.code <= 299)) + return std::nullopt; + if (uniform && *uniform != *sub.code) + return std::nullopt; + uniform = sub.code; + } - // If we got a success then we can just use the original status code - int16_t code = result["code"].get(); - if (code >= 200 && code <= 299) - return std::nullopt; + return uniform; +} - if (first_status_code == -1) - first_status_code = code; - else if (first_status_code != code) - return std::nullopt; - } +std::vector response::subresponses(const nlohmann::json& json) { + auto results = field(json, "results", &nlohmann::json::is_array); + if (!results) + return {{std::nullopt, json.is_object() ? &json : nullptr}}; - return first_status_code; + std::vector subs; + subs.reserve(results->size()); + + for (const auto& result : *results) { + auto& sub = subs.emplace_back(); + if (auto code = field(result, "code", &nlohmann::json::is_number)) + sub.code = code->get(); + sub.body = field(result, "body", &nlohmann::json::is_object); + } + + return subs; +} + +std::optional>> batch_request_accounts( + std::string_view endpoint, std::span body) { + if (endpoint != "batch" && endpoint != "sequence") + return std::nullopt; + + auto json = nlohmann::json::parse(body.begin(), body.end(), nullptr, false); + auto requests = field(json, "requests", &nlohmann::json::is_array); + if (!requests) + return std::nullopt; + + std::vector> accounts; + accounts.reserve(requests->size()); + + for (const auto& req : *requests) { + auto& account = accounts.emplace_back(); + if (auto params = field(req, "params", &nlohmann::json::is_object)) + account = account_in(*params); + } + + return accounts; +} + +response::swarm_rejections response::find_swarm_rejections( + const nlohmann::json* json, + int16_t status_code, + std::span> accounts) { + swarm_rejections result; + + // A body that isn't JSON still leaves the response's own status to act on, just with no + // redirect to go with it + auto subs = json ? subresponses(*json) : std::vector{{}}; + + for (size_t i = 0; i < subs.size(); ++i) { + const auto& [code, sub_body] = subs[i]; + if (code.value_or(status_code) != ERROR_MISDIRECTED_REQUEST) + continue; + + // Attributed by position in our own request, never by the account the 421 names: that is + // the responding node's word, and taking it would let any node we talk to redirect any + // account it likes - not just the ones it was asked about - onto nodes of its choosing + auto account = account_by_position(accounts, i); + if (!account) { + result.unattributed = true; + continue; } - } catch (...) { /* Do nothing */ + + if (auto echoed = sub_body ? account_in(*sub_body) : std::nullopt; + echoed && *echoed != *account) { + log::warning( + cat, + "Ignoring a 421 for {} that names {} as the rejected account.", + account->hex(), + echoed->hex()); + result.unattributed = true; + continue; + } + + // Several sub-requests for one account (a retrieve per namespace, say) all name its swarm, + // so the first is as good as any + if (auto [it, inserted] = result.accounts.try_emplace(*account); inserted && sub_body) + it->second = redirect_keys(*sub_body); } - return std::nullopt; + return result; } } // namespace session::network diff --git a/tests/test_network_swarm.cpp b/tests/test_network_swarm.cpp index 887ea581e..1cd0df5c1 100644 --- a/tests/test_network_swarm.cpp +++ b/tests/test_network_swarm.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -198,3 +199,183 @@ TEST_CASE("Swarm", "[network][swarm][get_swarm]") { "05000000000000000000000000000000000000000000000000fffffffffffffffe", swarms) == 0); } + +namespace { +using json = nlohmann::json; + +x25519_pubkey account(char c) { + return x25519_pubkey::from_hex(std::string(64, c)); +} + +std::vector nodes(std::string_view chars) { + std::vector keys; + for (char c : chars) + keys.push_back(ed25519_pubkey::from_hex(std::string(64, c))); + return keys; +} + +json rejection(std::optional echoed, std::string_view node_chars) { + auto snodes = json::array(); + for (char c : node_chars) + snodes.push_back({{"pubkey_ed25519", std::string(64, c)}, {"ip", "10.0.0.1"}}); + + json body = {{"snodes", std::move(snodes)}, {"swarm", "abc"}}; + if (echoed) + body["pubkey"] = *echoed; + return body; +} + +json batch(std::vector> results) { + auto list = json::array(); + for (auto& [code, body] : results) + list.push_back({{"code", code}, {"body", std::move(body)}}); + return {{"results", std::move(list)}}; +} + +response::swarm_rejections rejections( + const json& body, + int16_t status_code, + std::vector> accounts = {}) { + return response::find_swarm_rejections(&body, status_code, accounts); +} + +const std::string A = "05" + std::string(64, 'a'); +const std::string B = "03" + std::string(64, 'b'); +} // namespace + +TEST_CASE("Swarm", "[network][swarm][batch_error]") { + CHECK(response::find_uniform_batch_error(batch({{421, {}}, {421, {}}}).dump()) == 421); + CHECK_FALSE(response::find_uniform_batch_error(batch({{421, {}}, {406, {}}}).dump())); + CHECK_FALSE(response::find_uniform_batch_error(batch({{421, {}}, {200, {}}}).dump())); + CHECK_FALSE(response::find_uniform_batch_error(batch({}).dump())); + CHECK_FALSE(response::find_uniform_batch_error(rejection(A, "12").dump())); + CHECK_FALSE(response::find_uniform_batch_error("not json")); +} + +TEST_CASE("Swarm", "[network][swarm][rejections]") { + SECTION("A plain 421 is attributed to the request's account") { + auto r = rejections(rejection(A, "123"), 421, {account('a')}); + CHECK_FALSE(r.unattributed); + REQUIRE(r.accounts.size() == 1); + CHECK(r.accounts.at(account('a')) == nodes("123")); + } + + SECTION("An account echoed in testnet form, without a prefix, still matches") { + auto r = rejections(rejection(std::string(64, 'a'), "12"), 421, {account('a')}); + REQUIRE(r.accounts.count(account('a'))); + CHECK(r.accounts.at(account('a')) == nodes("12")); + } + + SECTION("A 421 needn't name its account") { + auto r = rejections( + batch({{421, rejection(std::nullopt, "12")}, {421, rejection(std::nullopt, "34")}}), + 421, + {account('a'), account('b')}); + CHECK_FALSE(r.unattributed); + REQUIRE(r.accounts.size() == 2); + CHECK(r.accounts.at(account('a')) == nodes("12")); + CHECK(r.accounts.at(account('b')) == nodes("34")); + } + + SECTION("Nothing is found in a response that isn't a 421") { + auto r = rejections(json{{"t", 1}}, 200, {account('a')}); + CHECK(r.accounts.empty()); + CHECK_FALSE(r.unattributed); + } + + SECTION("A batch can reject some accounts and answer others") { + auto r = rejections( + batch({{421, rejection(A, "12")}, + {200, json{{"messages", json::array()}}}, + {421, rejection(B, "34")}}), + 200, + {account('a'), account('c'), account('b')}); + CHECK_FALSE(r.unattributed); + REQUIRE(r.accounts.size() == 2); + CHECK(r.accounts.at(account('a')) == nodes("12")); + CHECK(r.accounts.at(account('b')) == nodes("34")); + } + + SECTION("Several rejections of one account are one redirect") { + auto r = rejections( + batch({{421, rejection(A, "12")}, {421, rejection(A, "12")}}), 421, {account('a')}); + REQUIRE(r.accounts.size() == 1); + CHECK(r.accounts.at(account('a')) == nodes("12")); + } + + SECTION("A single account applies to every sub-request") { + auto r = rejections( + batch({{421, rejection(std::nullopt, "12")}, {421, rejection(std::nullopt, "12")}}), + 421, + {account('a')}); + REQUIRE(r.accounts.size() == 1); + CHECK(r.accounts.at(account('a')) == nodes("12")); + } + + SECTION("A sequence that stopped early is still attributed by position") { + auto r = rejections( + batch({{200, json::object()}, {421, rejection(std::nullopt, "34")}}), + 200, + {account('a'), account('b'), account('c')}); + REQUIRE(r.accounts.size() == 1); + CHECK(r.accounts.at(account('b')) == nodes("34")); + } + + // The account a 421 names is the responding node's word; taking it would let any node redirect + // any account it likes, not just those it was asked about + SECTION("The account a 421 names is never trusted in place of the request's") { + auto r = rejections(rejection(A, "12"), 421, {account('b')}); + CHECK(r.accounts.empty()); + CHECK(r.unattributed); + + r = rejections( + batch({{421, rejection("05" + std::string(64, 'c'), "12")}, + {421, rejection(B, "34")}}), + 421, + {account('a'), account('b')}); + CHECK(r.unattributed); + REQUIRE(r.accounts.size() == 1); + CHECK(r.accounts.at(account('b')) == nodes("34")); + + CHECK(rejections(rejection(A, "12"), 421).accounts.empty()); + } + + SECTION("A sub-request with no account leaves its 421 unattributed") { + auto r = rejections( + batch({{200, json::object()}, {421, rejection(std::nullopt, "12")}}), + 200, + {account('a'), std::nullopt}); + CHECK(r.accounts.empty()); + CHECK(r.unattributed); + } + + SECTION("A 421 whose body isn't JSON has no redirect") { + std::vector> accounts{account('a')}; + auto r = response::find_swarm_rejections(nullptr, 421, accounts); + REQUIRE(r.accounts.size() == 1); + CHECK(r.accounts.at(account('a')).empty()); + CHECK(response::find_swarm_rejections(nullptr, 200, accounts).accounts.empty()); + } +} + +TEST_CASE("Swarm", "[network][swarm][batch_request_accounts]") { + SECTION("A pregenerated batch body gives each sub-request's account") { + auto body = json{{"requests", + {{{"method", "retrieve"}, {"params", {{"pubkey", A}}}}, + {{"method", "info"}, {"params", json::object()}}, + {{"method", "retrieve"}, {"params", {{"pubkey", B}}}}}}} + .dump(); + auto bytes = std::span{reinterpret_cast(body.data()), body.size()}; + + auto accounts = batch_request_accounts("batch", bytes); + REQUIRE(accounts); + CHECK(*accounts == + std::vector>{account('a'), std::nullopt, account('b')}); + CHECK(batch_request_accounts("sequence", bytes) == accounts); + CHECK_FALSE(batch_request_accounts("retrieve", bytes)); + + std::string_view junk = "not json"; + CHECK_FALSE(batch_request_accounts( + "batch", {reinterpret_cast(junk.data()), junk.size()})); + } +} From 16be3674a734f7394dc59bf15e558bed81ba0aad Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 15:56:32 -0300 Subject: [PATCH 07/22] Act on a destination's status only when its response decrypted 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. --- .../session/network/session_network_types.hpp | 8 ++ src/network/routing/onion_request_router.cpp | 86 +++++++------ src/network/routing/session_router_router.cpp | 55 +++++---- src/network/session_network_types.cpp | 9 ++ tests/test_onion_request_router.cpp | 116 +++++++++++++++++- 5 files changed, 203 insertions(+), 71 deletions(-) diff --git a/include/session/network/session_network_types.hpp b/include/session/network/session_network_types.hpp index 3b255321c..52e6e6b5b 100644 --- a/include/session/network/session_network_types.hpp +++ b/include/session/network/session_network_types.hpp @@ -30,6 +30,7 @@ constexpr int16_t ERROR_FAILED_TO_QUEUE_REQUEST = -10008; constexpr int16_t ERROR_INVALID_DESTINATION = -10009; constexpr int16_t ERROR_FAILED_GENERATE_ONION_PAYLOAD = -10010; constexpr int16_t ERROR_FAILED_TO_GET_STREAM = -10011; +constexpr int16_t ERROR_UNAUTHENTICATED_RESPONSE = -10012; constexpr int16_t ERROR_BUILD_TIMEOUT = -10100; constexpr int16_t ERROR_REQUEST_CANCELLED = -10200; constexpr int16_t ERROR_UNKNOWN = -11000; @@ -282,6 +283,13 @@ namespace response { /// As `find_uniform_batch_error`, for a body that has already been parsed. std::optional uniform_batch_error(const nlohmann::json& json); + /// The status to report for a proxied response that could not be decrypted, whose status + /// therefore came from whichever node on the way answered rather than from the destination. + /// Statuses that are acted on as the destination's verdict (406, 421, 425) become + /// `ERROR_UNAUTHENTICATED_RESPONSE`, since any node on the path could have sent them; anything + /// else is returned unchanged. + int16_t undecrypted_status(int16_t status); + /// One result of a `batch`/`sequence` response, or the whole of any other response. struct subresponse { /// The result's own status code; nullopt for a response that isn't a batch, whose status diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index de0b947aa..c3f0ad044 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -1634,51 +1634,49 @@ void OnionRequestRouter::_send_on_path( return; } - auto decryption_callback = [weak_self = weak_from_this(), - this, - parser = std::move(parser), - path_id = path.id, - original_request = std::move(request), - cb = std::move(callback)]( - bool success, - bool timeout, - int16_t status, - auto headers, - auto response) { - auto self = weak_self.lock(); - if (!self) - return; + auto decryption_callback = + [weak_self = weak_from_this(), + this, + parser = std::move(parser), + path_id = path.id, + original_request = std::move(request), + cb = std::move(callback)]( + bool success, bool timeout, int16_t status, auto headers, auto response) { + auto self = weak_self.lock(); + if (!self) + return; - try { - if (!success) - throw std::runtime_error{response.value_or("Unknown request failure")}; - if (timeout) - throw std::runtime_error{response.value_or("Timed out")}; - if (!response) - throw std::runtime_error{"Unexpected empty response"}; - - onionreq::DecryptedResponse decrypted = parser->decrypted_response(*response); - _handle_transport_response( - path_id, - std::move(original_request), - true, - false, - decrypted.status_code, - std::move(decrypted.headers), - std::move(decrypted.body), - std::move(cb)); - } catch (const std::exception& e) { - _handle_transport_response( - path_id, - std::move(original_request), - false, - timeout, - status, - std::move(headers), - std::move("Failed to handle onion response due to error: {}"_format(e.what())), - std::move(cb)); - } - }; + try { + if (!success) + throw std::runtime_error{response.value_or("Unknown request failure")}; + if (timeout) + throw std::runtime_error{response.value_or("Timed out")}; + if (!response) + throw std::runtime_error{"Unexpected empty response"}; + + onionreq::DecryptedResponse decrypted = parser->decrypted_response(*response); + _handle_transport_response( + path_id, + std::move(original_request), + true, + false, + decrypted.status_code, + std::move(decrypted.headers), + std::move(decrypted.body), + std::move(cb)); + } catch (const std::exception& e) { + _handle_transport_response( + path_id, + std::move(original_request), + false, + timeout, + response::undecrypted_status(status), + std::move(headers), + "Failed to handle onion response (status {}) due to error: {}"_format( + status, e.what()), + std::move(cb)); + } + }; transport->send_request(std::move(onion_request), std::move(decryption_callback)); } diff --git a/src/network/routing/session_router_router.cpp b/src/network/routing/session_router_router.cpp index 5dd80ecbd..f8f54d814 100644 --- a/src/network/routing/session_router_router.cpp +++ b/src/network/routing/session_router_router.cpp @@ -539,32 +539,35 @@ void SessionRouter::_send_proxy_request(Request request, network_response_callba request.time_remaining(), request.overall_timeout}; - auto proxy_callback = - [parser = std::move(parser), cb = std::move(callback)]( - bool success, bool timeout, int16_t status, auto headers, auto response) { - try { - if (!success) - throw std::runtime_error{response.value_or("Unknown request failure")}; - if (timeout) - throw std::runtime_error{response.value_or("Timed out")}; - if (!response) - throw std::runtime_error{"Unexpected empty response"}; - - onionreq::DecryptedResponse decrypted = parser->decrypted_response(*response); - cb(true, - false, - decrypted.status_code, - std::move(decrypted.headers), - std::move(decrypted.body)); - } catch (const std::exception& e) { - cb(false, - timeout, - status, - std::move(headers), - "Failed to handle proxied request response due to error: {}"_format( - e.what())); - } - }; + auto proxy_callback = [parser = std::move(parser), cb = std::move(callback)]( + bool success, + bool timeout, + int16_t status, + auto headers, + auto response) { + try { + if (!success) + throw std::runtime_error{response.value_or("Unknown request failure")}; + if (timeout) + throw std::runtime_error{response.value_or("Timed out")}; + if (!response) + throw std::runtime_error{"Unexpected empty response"}; + + onionreq::DecryptedResponse decrypted = parser->decrypted_response(*response); + cb(true, + false, + decrypted.status_code, + std::move(decrypted.headers), + std::move(decrypted.body)); + } catch (const std::exception& e) { + cb(false, + timeout, + response::undecrypted_status(status), + std::move(headers), + "Failed to handle proxied request response (status {}) due to error: {}"_format( + status, e.what())); + } + }; // Now that we have a service_node destination we can send a direct request _send_direct_request(std::move(proxy_request), std::move(proxy_callback)); diff --git a/src/network/session_network_types.cpp b/src/network/session_network_types.cpp index 163fa0be6..f64e6a0d6 100644 --- a/src/network/session_network_types.cpp +++ b/src/network/session_network_types.cpp @@ -118,6 +118,15 @@ namespace { } } // namespace +int16_t response::undecrypted_status(int16_t status) { + switch (status) { + case ERROR_NOT_ACCEPTABLE: + case ERROR_MISDIRECTED_REQUEST: + case ERROR_TOO_EARLY: return ERROR_UNAUTHENTICATED_RESPONSE; + default: return status; + } +} + std::optional response::find_uniform_batch_error(std::string_view body) { try { return uniform_batch_error(nlohmann::json::parse(body)); diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 380dd2315..d668b0e46 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -78,6 +78,14 @@ class TestOnionRequestRouter { return router->_find_valid_path(request); } + static void send_on_path( + std::shared_ptr router, + OnionPath& path, + Request request, + network_response_callback_t callback) { + router->_send_on_path(path, std::move(request), std::move(callback)); + } + static void handle_transport_response( std::shared_ptr router, std::string path_id, @@ -218,8 +226,19 @@ namespace { func_called("remove_failure_listeners"); } - void send_request(Request /*request*/, network_response_callback_t /*callback*/) override { + // Answered the way a node on the path does when it fails a request itself: a status of its + // own and a plaintext body, which is nothing the destination encrypted + std::optional> plaintext_reply; + + void send_request(Request /*request*/, network_response_callback_t callback) override { func_called("send_request"); + if (plaintext_reply) + callback( + false, + false, + plaintext_reply->first, + {content_type_plain_text}, + plaintext_reply->second); } }; @@ -924,4 +943,99 @@ TEST_CASE("Network", "[network][onion_request_router][cached_edge_nodes]") { REQUIRE(remaining.size() == 1); CHECK(remaining.front().node == healthy); } + +TEST_CASE("Network", "[network][onion_request_router][undecrypted_response]") { + config::SnodePool pool_config = { + .cache_directory = std::nullopt, + .fallback_snode_pool_path = std::nullopt, + .cache_expiration = std::chrono::minutes{5}, + .cache_min_lifetime = std::chrono::minutes{5}, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .seed_nodes = {}, + .cache_min_size = 0, + .cache_min_swarm_size = 0, + .cache_num_nodes_to_use_for_refresh = 3, + .cache_min_num_refresh_presence_to_include_node = 2, + .cache_node_strike_threshold = 3}; + config::OnionRequestRouter config = { + file_server::DEFAULT_CONFIG, + std::nullopt, + std::chrono::days{10}, + opt::netid::Target::testnet, + {}, + network::opt::retry_delay{50ms, 200ms}, + 3, + 3, + 10, + 10min, + 3, + true, + true, + {{PathCategory::standard, 1}}}; + + auto node = [](std::string_view pk_hex, uint16_t port) { + return service_node{ + ed25519_pubkey::from_hex(pk_hex), + oxen::quic::ipv4{"127.0.0.1"}, + port, + static_cast(port + 10000), + {2, 11, 0}, + 0}; + }; + auto target = node("4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7", 20001); + auto path_nodes = std::vector{ + node("5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876", 20002), + node("e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5", 20003), + node("7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f", 20004)}; + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + auto transport = std::make_shared(); + auto router = + std::make_shared(config, loop, disk_loop, snode_pool, transport); + TestOnionRequestRouter::set_paths( + router, PathCategory::standard, {OnionPath{"Test", path_nodes}}); + + auto send_with_reply = [&](int16_t status) { + transport->plaintext_reply.emplace(status, "Forged by a node on the path"); + auto request = Request{ + "AAAA", target, "retrieve", to_vector("{}"), RequestCategory::standard, 10s}; + auto* path = TestOnionRequestRouter::find_valid_path(router, request); + REQUIRE(path); + + std::optional result; + TestOnionRequestRouter::send_on_path( + router, + *path, + request, + [&result]( + bool success, + bool timeout, + int16_t status_code, + std::vector> headers, + std::optional response) { + result = {success, timeout, status_code, headers, response}; + }); + REQUIRE(result); + return *result; + }; + + // Each of these is acted on as the destination's verdict (a swarm redirect, a clock resync), + // which only the destination can give; a node on the path sending one mustn't pass for it + for (int16_t status : {ERROR_MISDIRECTED_REQUEST, ERROR_NOT_ACCEPTABLE, ERROR_TOO_EARLY}) { + CAPTURE(status); + auto result = send_with_reply(status); + CHECK_FALSE(result.success); + CHECK(result.status_code == ERROR_UNAUTHENTICATED_RESPONSE); + CHECK(result.response.value_or("").find("status {}"_format(status)) != std::string::npos); + } + + // Anything else a node on the path says about its own failure is reported as it was + auto result = send_with_reply(502); + CHECK_FALSE(result.success); + CHECK(result.status_code == 502); +} } // namespace session::network From 6499e95e842696bf66c6ae4f6e02f930b87955af Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 16:15:01 -0300 Subject: [PATCH 08/22] Follow a swarm redirect without checking it against our own grouping 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. --- src/network/snode_pool.cpp | 20 ------------ tests/test_snode_pool.cpp | 64 ++++++++++++++++++-------------------- 2 files changed, 31 insertions(+), 53 deletions(-) diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index ac4e2a8e0..9408ceff3 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1260,26 +1260,6 @@ bool SnodePool::record_swarm_redirect( return false; } - // A redirect naming the swarm we already calculated is the node contradicting itself - it - // rejected the request and then pointed us back at the nodes we asked through. Refusing it - // sends the caller off to refresh the pool, which is the only thing that can help - auto computed = swarm::get_swarm(swarm_pubkey, _all_swarms); - if (computed.second.size() == resolved.size()) { - std::unordered_set computed_keys; - for (const auto& node : computed.second) - computed_keys.insert(node.remote_pubkey); - - if (std::ranges::all_of(resolved, [&computed_keys](const auto& node) { - return computed_keys.count(node.remote_pubkey) > 0; - })) { - log::debug( - cat, - "Ignoring redirect for {}: it names the swarm we already calculated.", - swarm_pubkey.hex()); - return false; - } - } - auto& [nodes, redirects] = _swarm_overrides[swarm_pubkey]; if (++redirects > MAX_CONSECUTIVE_SWARM_REDIRECTS) { diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 63df35675..52d59b0f8 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -583,51 +583,49 @@ TEST_CASE("Network", "[network][swarm_redirect]") { // Too few of the named nodes resolve to be a swarm CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 2))); - // Naming the swarm we already calculated is the node contradicting itself - CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); + auto current_swarm = [&] { + std::vector swarm; + snode_pool->debug_run_on_loop([&] { + snode_pool->get_swarm( + swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { + swarm = std::move(nodes); + }); + }); + return sorted(std::move(swarm)); + }; + auto overridden = sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 4)); // A usable redirect is taken, and - the point of all this - no pool refresh is started for it REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); CHECK_FALSE(snode_pool->debug_refresh_in_progress()); - - std::vector redirected; - snode_pool->debug_run_on_loop([&] { - snode_pool->get_swarm( - swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { - redirected = std::move(nodes); - }); - }); - CHECK(sorted(redirected) == - sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 4))); + CHECK(current_swarm() == overridden); CHECK_FALSE(snode_pool->debug_refresh_in_progress()); - // Nodes that keep disagreeing must not bounce us forever; after the third we stop believing - // them and the calculated swarm is back, for the caller to fall back on refreshing the pool - CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 5))); + // How the named swarm relates to the one we sent to says nothing, since swarms can be + // rearranged any way at all: a node that has left the overridden swarm names its current + // members, and one in it naming the calculated swarm is how a wrong redirect gets put right CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); - CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 5))); + CHECK(current_swarm() == overridden); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); + CHECK(current_swarm() == sorted(calculated)); - std::vector after_giving_up; - snode_pool->debug_run_on_loop([&] { - snode_pool->get_swarm( - swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { - after_giving_up = std::move(nodes); - }); - }); - CHECK(sorted(after_giving_up) == sorted(calculated)); + // Every redirect counts, though, so two sets of nodes that keep disagreeing can't bounce us + // between them forever; after the third we stop believing them and the calculated swarm is + // back, for the caller to fall back on refreshing the pool + CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + CHECK(current_swarm() == sorted(calculated)); // A refreshed pool is ground truth again, so redirects correcting the old one are dropped REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); snode_pool->update_cache(snode_cache); - - std::vector after_refresh; - snode_pool->debug_run_on_loop([&] { - snode_pool->get_swarm( - swarm_pubkey, true, [&](swarm::swarm_id_t, std::vector nodes) { - after_refresh = std::move(nodes); - }); - }); - CHECK(sorted(after_refresh) == sorted(calculated)); + CHECK(current_swarm() == sorted(calculated)); + + // ... and the calculated swarm itself, or part of it, is followed like any other + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); + CHECK(current_swarm() == sorted(calculated)); + auto remaining = std::vector(calculated.begin() + 1, calculated.end()); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(remaining, 5))); + CHECK(current_swarm() == sorted(remaining)); } TEST_CASE("Network", "[network][strike_expiry]") { From 190f017a4b008edd9d026a13b6050460e9bb6fff Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 16:25:56 -0300 Subject: [PATCH 09/22] Check the pool can now serve a caller before retrying it 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. --- src/network/routing/onion_request_router.cpp | 31 ++- src/network/session_network.cpp | 17 +- tests/test_onion_request_router.cpp | 197 +++++++++++++------ 3 files changed, 168 insertions(+), 77 deletions(-) diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index c3f0ad044..c828f1f21 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -1199,14 +1199,25 @@ void OnionRequestRouter::_build_path( if (!self) return; - // Rebuilding without a refresh lands back in this same branch with the same - // too-few nodes, so the queued requests would wait on a loop that cannot end - if (!refreshed) { + // Rebuilding lands back in this same branch unless the pool can now fill a + // path, and `refreshed` includes nothing having needed refreshing, which calls + // back inline - so rebuilding without checking first recurses until the stack + // gives out + std::optional cannot_build; + if (!refreshed) + cannot_build = "the snode pool could not be refreshed"; + else if (auto pool = _snode_pool.lock(); + !pool || + pool->get_unused_nodes(_config.path_length, nodes_to_exclude).size() < + _config.path_length) + cannot_build = "the snode pool has too few usable nodes"; + + if (cannot_build) { log::error( cat, - "[Request {}]: Cannot build a path, the snode pool could not be " - "refreshed.", - initiating_req_id.value_or("internal")); + "[Request {}]: Cannot build a path, {}.", + initiating_req_id.value_or("internal"), + *cannot_build); _update_status(); auto queue_it = _request_queues.find(category); @@ -1222,17 +1233,17 @@ void OnionRequestRouter::_build_path( auto to_fail = queue_it->second->pop_all(); log::error( cat, - "Failing {} queued requests for '{}' paths; the snode pool " - "could not be refreshed.", + "Failing {} queued requests for '{}' paths; {}.", to_fail.size(), - to_string(category, _config.single_path_mode)); + to_string(category, _config.single_path_mode), + *cannot_build); for (const auto& [req, cb] : to_fail) cb(false, false, -1, {content_type_plain_text}, - "Failed to refresh the snode pool to build a path."); + "Cannot build a path: {}."_format(*cannot_build)); } return; } diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 0e7c8d73c..baeeccaf1 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -411,8 +411,6 @@ void Network::get_random_nodes( return _snode_pool->refresh_if_needed( nodes_to_exclude, [this, count, cb = std::move(cb)](bool refreshed) { - // Retrying without a refresh would re-enter this same branch and recurse - // until the stack gives out if (!refreshed) { log::warning( cat, @@ -421,7 +419,20 @@ void Network::get_random_nodes( return cb({}); } - get_random_nodes(count, cb); + // `refreshed` includes nothing having needed refreshing, which calls back + // inline, so asking again of a pool that can't supply `count` would + // re-enter this branch until the stack gives out + auto nodes = _snode_pool->get_unused_nodes(count); + if (nodes.size() < count) { + log::warning( + cat, + "Cannot get {} random nodes: the pool has only {} to give.", + count, + nodes.size()); + return cb({}); + } + + cb(std::move(nodes)); }); } cb(unused_nodes); diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index d668b0e46..778c608d6 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -174,11 +174,21 @@ namespace { SnodePool::record_node_failure(key, permanent); } + // When set, `refresh_if_needed` calls back with it straight away - as the real one does, on + // the loop thread, when nothing needs refreshing - instead of never calling back. Only the + // first `max_refresh_callbacks` calls do, so code that asks again on every callback stops + // there instead of recursing until the stack gives out. + std::optional refresh_result; + int max_refresh_callbacks = 20; + void refresh_if_needed( const std::vector& /*in_use_nodes*/, - refresh_callback_t /*on_refresh_complete*/ = nullptr) override { + refresh_callback_t on_refresh_complete = nullptr) override { func_called("refresh_if_needed"); - // Do nothing (don't want to trigger a cache refresh) + // Don't trigger a real cache refresh + if (refresh_result && on_refresh_complete && + get_call_count("refresh_if_needed") <= max_refresh_callbacks) + on_refresh_complete(*refresh_result); } void get_swarm( @@ -249,6 +259,60 @@ namespace { std::vector> headers; std::optional response; }; + + config::SnodePool test_pool_config() { + return {.cache_directory = std::nullopt, + .fallback_snode_pool_path = std::nullopt, + .cache_expiration = std::chrono::minutes{5}, + .cache_min_lifetime = std::chrono::minutes{5}, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .seed_nodes = {}, + .cache_min_size = 0, + .cache_min_swarm_size = 0, + .cache_num_nodes_to_use_for_refresh = 3, + .cache_min_num_refresh_presence_to_include_node = 2, + .cache_node_strike_threshold = 3}; + } + + config::OnionRequestRouter test_router_config() { + return {.file_server_config = file_server::DEFAULT_CONFIG, + .cache_directory = std::nullopt, + .edge_node_cache_duration = std::chrono::days{10}, + .netid = opt::netid::Target::testnet, + .seed_nodes = {}, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .path_length = 3, + .path_strike_threshold = 3, + .path_build_retry_limit = 10, + .path_rotation_frequency = 10min, + .node_strike_threshold = 3, + .disable_pre_build_paths = true, + .single_path_mode = true, + .min_path_counts = {{PathCategory::standard, 1}}}; + } + + service_node test_node(std::string_view pk_hex, uint16_t port) { + return service_node{ + ed25519_pubkey::from_hex(pk_hex), + oxen::quic::ipv4{"127.0.0.1"}, + port, + static_cast(port + 10000), + {2, 11, 0}, + 0}; + } + + network_response_callback_t record_into(std::optional& result) { + return [&result]( + bool success, + bool timeout, + int16_t status_code, + std::vector> headers, + std::optional response) { + result = {success, timeout, status_code, headers, response}; + }; + } } // namespace TEST_CASE("Network", "[network][onion_request_router][handle_errors]") { @@ -945,80 +1009,40 @@ TEST_CASE("Network", "[network][onion_request_router][cached_edge_nodes]") { } TEST_CASE("Network", "[network][onion_request_router][undecrypted_response]") { - config::SnodePool pool_config = { - .cache_directory = std::nullopt, - .fallback_snode_pool_path = std::nullopt, - .cache_expiration = std::chrono::minutes{5}, - .cache_min_lifetime = std::chrono::minutes{5}, - .enforce_subnet_diversity = false, - .retry_delay = network::opt::retry_delay{50ms, 200ms}, - .netid = opt::netid::Target::testnet, - .seed_nodes = {}, - .cache_min_size = 0, - .cache_min_swarm_size = 0, - .cache_num_nodes_to_use_for_refresh = 3, - .cache_min_num_refresh_presence_to_include_node = 2, - .cache_node_strike_threshold = 3}; - config::OnionRequestRouter config = { - file_server::DEFAULT_CONFIG, - std::nullopt, - std::chrono::days{10}, - opt::netid::Target::testnet, - {}, - network::opt::retry_delay{50ms, 200ms}, - 3, - 3, - 10, - 10min, - 3, - true, - true, - {{PathCategory::standard, 1}}}; - - auto node = [](std::string_view pk_hex, uint16_t port) { - return service_node{ - ed25519_pubkey::from_hex(pk_hex), - oxen::quic::ipv4{"127.0.0.1"}, - port, - static_cast(port + 10000), - {2, 11, 0}, - 0}; - }; - auto target = node("4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7", 20001); + auto target = + test_node("4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7", 20001); auto path_nodes = std::vector{ - node("5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876", 20002), - node("e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5", 20003), - node("7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f", 20004)}; + test_node("5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876", 20002), + test_node("e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5", 20003), + test_node("7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f", 20004)}; auto loop = std::make_shared(); auto disk_loop = std::make_shared(); - auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + auto snode_pool = std::make_shared(test_pool_config(), loop, disk_loop); auto transport = std::make_shared(); - auto router = - std::make_shared(config, loop, disk_loop, snode_pool, transport); - TestOnionRequestRouter::set_paths( - router, PathCategory::standard, {OnionPath{"Test", path_nodes}}); + auto router = std::make_shared( + test_router_config(), loop, disk_loop, snode_pool, transport); + + // The router is driven on the loop, as its own code is, and checked from here + loop->call_get([&] { + TestOnionRequestRouter::set_paths( + router, PathCategory::standard, {OnionPath{"Test", path_nodes}}); + }); auto send_with_reply = [&](int16_t status) { transport->plaintext_reply.emplace(status, "Forged by a node on the path"); auto request = Request{ "AAAA", target, "retrieve", to_vector("{}"), RequestCategory::standard, 10s}; - auto* path = TestOnionRequestRouter::find_valid_path(router, request); - REQUIRE(path); + bool have_path = false; std::optional result; - TestOnionRequestRouter::send_on_path( - router, - *path, - request, - [&result]( - bool success, - bool timeout, - int16_t status_code, - std::vector> headers, - std::optional response) { - result = {success, timeout, status_code, headers, response}; - }); + loop->call_get([&] { + if (auto* path = TestOnionRequestRouter::find_valid_path(router, request)) { + have_path = true; + TestOnionRequestRouter::send_on_path(router, *path, request, record_into(result)); + } + }); + REQUIRE(have_path); REQUIRE(result); return *result; }; @@ -1038,4 +1062,49 @@ TEST_CASE("Network", "[network][onion_request_router][undecrypted_response]") { CHECK_FALSE(result.success); CHECK(result.status_code == 502); } + +TEST_CASE("Network", "[network][onion_request_router][build_path_too_few_nodes]") { + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(test_pool_config(), loop, disk_loop); + auto transport = std::make_shared(); + auto router = std::make_shared( + test_router_config(), loop, disk_loop, snode_pool, transport); + + // Run on the loop, as the router's own code is: that also puts it after the router's setup, + // which asks for the (empty) pool to be refreshed and mustn't be counted below + std::optional result; + loop->call_get([&] { + snode_pool->reset_calls(); + + auto queue = std::make_shared(loop); + TestOnionRequestRouter::set_request_queues(router, {{PathCategory::standard, queue}}); + queue->add( + Request{"AAAA", + test_node( + "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7", + 20001), + "retrieve", + to_vector("{}"), + RequestCategory::standard, + 10s}, + record_into(result)); + + // A path needs three nodes and the pool can only offer one, while saying nothing needs + // refreshing - which is a pool too concentrated in a few subnets to fill a path, say + snode_pool->mock_unused_nodes = std::vector{test_node( + "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876", 20002)}; + snode_pool->refresh_result = true; + + TestOnionRequestRouter::build_path(router, PathCategory::standard, "AAAA"); + }); + + // Asking for a refresh once and then giving up; without checking that the pool can now fill a + // path, every "refreshed" rebuilt straight into another refresh request, and only the mock's + // limit stops it + CHECK(snode_pool->get_call_count("refresh_if_needed") == 1); + REQUIRE(result); + CHECK_FALSE(result->success); + CHECK(result->response.value_or("").find("too few usable nodes") != std::string::npos); +} } // namespace session::network From 35387a982501f8864984258dc3255b19d9eb912b Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 16:55:28 -0300 Subject: [PATCH 10/22] Keep following swarm redirects past the limit, and ask for a refresh 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. --- include/session/network/snode_pool.hpp | 4 +-- src/network/snode_pool.cpp | 42 ++++++++++++++++---------- tests/test_snode_pool.cpp | 28 ++++++++++++++--- 3 files changed, 52 insertions(+), 22 deletions(-) diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 64515c565..4ebd252aa 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -135,8 +135,8 @@ class SnodePool : public std::enable_shared_from_this { // Swarms a node has redirected us to, which `get_swarm` prefers over its own calculation. Kept // apart from `_swarm_cache` deliberately: that is a memo of `_all_swarms` and stays one, where - // these are claims from outside that outrank it until the pool is refreshed. The count bounds - // how far a disagreement between nodes can bounce us before we go and refresh instead. + // these are claims from outside that outrank it until the pool is refreshed. The count is the + // account's redirects since that refresh, which past a limit also ask for the next one. std::unordered_map, uint8_t>> _swarm_overrides; std::map> _snode_strikes; diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 9408ceff3..a1f327810 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -55,9 +56,12 @@ namespace { // exactly what a persistent rejection source would produce. const std::chrono::seconds EVIDENCE_REFRESH_BASE_BACKOFF = 1min; - // Two nodes disagreeing about who owns a swarm can redirect us back and forth; after this many - // in a row without an intervening pool refresh, stop believing them and go and refresh. - constexpr uint8_t MAX_CONSECUTIVE_SWARM_REDIRECTS = 3; + // An account redirected more than this many times within one pool generation is being bounced + // between nodes that disagree, or its swarm has moved on too far for our pool to keep up. A + // redirect is still the best information we have, so past it each one is still followed, and + // also asks for the refresh that can settle it. Legitimate rearrangements count too (nothing + // resets this short of a refresh), hence the headroom. + constexpr uint8_t SWARM_REDIRECTS_BEFORE_REFRESH = 8; } // namespace SnodePool::SnodePool( @@ -1261,25 +1265,31 @@ bool SnodePool::record_swarm_redirect( } auto& [nodes, redirects] = _swarm_overrides[swarm_pubkey]; + nodes = std::move(resolved); + + // Saturating, because counting carries on past the limit for as long as the refresh it + // asks for is throttled, and wrapping would stop it asking + if (redirects < std::numeric_limits::max()) + ++redirects; + + log::info( + cat, + "Redirected to a swarm of {} nodes for {} (redirect {} since the pool was " + "refreshed).", + nodes.size(), + swarm_pubkey.hex(), + redirects); - if (++redirects > MAX_CONSECUTIVE_SWARM_REDIRECTS) { + if (redirects > SWARM_REDIRECTS_BEFORE_REFRESH) { log::warning( cat, - "Dropping redirects for {} after {} in a row; refreshing the pool instead.", + "{} has been redirected {} times since the pool was refreshed; asking for a " + "refresh.", swarm_pubkey.hex(), - MAX_CONSECUTIVE_SWARM_REDIRECTS); - _swarm_overrides.erase(swarm_pubkey); - return false; + redirects); + invalidate_swarm(swarm_pubkey); } - log::info( - cat, - "Redirected to a swarm of {} nodes for {} ({} of at most {}).", - resolved.size(), - swarm_pubkey.hex(), - redirects, - MAX_CONSECUTIVE_SWARM_REDIRECTS); - nodes = std::move(resolved); return true; }); } diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 52d59b0f8..764c143db 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -609,11 +609,24 @@ TEST_CASE("Network", "[network][swarm_redirect]") { CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); CHECK(current_swarm() == sorted(calculated)); - // Every redirect counts, though, so two sets of nodes that keep disagreeing can't bounce us - // between them forever; after the third we stop believing them and the calculated swarm is - // back, for the caller to fall back on refreshing the pool - CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + // Two sets of nodes that disagree, each redirecting to the other + auto bounce = [&](int times) { + for (int i = 0; i < times; ++i) + REQUIRE(snode_pool->record_swarm_redirect( + swarm_pubkey, i % 2 == 0 ? keys_of(elsewhere, 4) : keys_of(calculated, 6))); + }; + + // Every redirect counts, but none is refused for it - refusing would only fail the request. + // Up to the limit nothing else happens... + bounce(5); + CHECK(current_swarm() == overridden); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // ... and past it the redirect is still followed, but also asks for a refresh, which the pool + // being old enough lets start + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(calculated, 6))); CHECK(current_swarm() == sorted(calculated)); + CHECK(snode_pool->debug_refresh_in_progress()); // A refreshed pool is ground truth again, so redirects correcting the old one are dropped REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); @@ -626,6 +639,13 @@ TEST_CASE("Network", "[network][swarm_redirect]") { auto remaining = std::vector(calculated.begin() + 1, calculated.end()); CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(remaining, 5))); CHECK(current_swarm() == sorted(remaining)); + + // Past the limit with the refresh throttled - the pool was only just refreshed - redirects are + // still followed, rather than leaving the account on a swarm already known to be wrong + bounce(6); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); + CHECK(current_swarm() == overridden); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); } TEST_CASE("Network", "[network][strike_expiry]") { From 78c55b9ebccfc3548b7978b206ddc3ea9068d129 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 17:12:08 -0300 Subject: [PATCH 11/22] Follow a redirect with the nodes of it we know, and refresh for the rest 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. --- src/network/session_network.cpp | 6 ++++-- src/network/snode_pool.cpp | 20 +++++++++++++------- tests/test_snode_pool.cpp | 17 ++++++++++++++--- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index baeeccaf1..3662479a6 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -904,8 +904,10 @@ void Network::_handle_421_retry( req_to_retry = std::move(original_request), cb = std::move(final_callback), failed_node = failed_node_copy](bool swarm_resolved) mutable { - // Without a re-resolve the swarm cache still holds the mapping the 421 disproved, so the - // retry would go straight back to the swarm that just rejected us + // Without a re-resolve there is nothing better than the mapping the 421 disproved - a + // redirect naming any node we know would have been followed instead - so a retry would go + // straight back to the swarm that just rejected us. The refresh has been asked for, and + // the account's next rejection asks again. if (!swarm_resolved) return cb( false, diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index a1f327810..788bd0fea 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1254,16 +1254,21 @@ bool SnodePool::record_swarm_redirect( if (named.count(node.remote_pubkey)) resolved.push_back(node); - if (resolved.size() < _config.cache_min_swarm_size) { + if (resolved.empty()) { log::debug( cat, - "Ignoring redirect for {}: only {}/{} named nodes are in our pool.", + "Ignoring redirect for {}: none of the {} named nodes are in our pool.", swarm_pubkey.hex(), - resolved.size(), swarm_node_keys.size()); return false; } + // Named nodes we don't have are ones our pool is too old to know, and when that leaves too + // few to work with only a refresh can fill them in; the ones we do have are still in the + // right swarm, where the swarm we had just rejected us, so they are followed meanwhile + const bool pool_missing_members = + resolved.size() < named.size() && resolved.size() < _config.cache_min_swarm_size; + auto& [nodes, redirects] = _swarm_overrides[swarm_pubkey]; nodes = std::move(resolved); @@ -1280,13 +1285,14 @@ bool SnodePool::record_swarm_redirect( swarm_pubkey.hex(), redirects); - if (redirects > SWARM_REDIRECTS_BEFORE_REFRESH) { + if (redirects > SWARM_REDIRECTS_BEFORE_REFRESH || pool_missing_members) { log::warning( cat, - "{} has been redirected {} times since the pool was refreshed; asking for a " - "refresh.", + "Asking for a refresh for {}: {}.", swarm_pubkey.hex(), - redirects); + pool_missing_members + ? "its redirect names nodes our pool doesn't have" + : "{} redirects since the pool was refreshed"_format(redirects)); invalidate_swarm(swarm_pubkey); } diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 764c143db..ce2dd0fee 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -580,9 +580,6 @@ TEST_CASE("Network", "[network][swarm_redirect]") { // us in touch with something that isn't a registered service node CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, {key_for(200), key_for(201)})); - // Too few of the named nodes resolve to be a swarm - CHECK_FALSE(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 2))); - auto current_swarm = [&] { std::vector swarm; snode_pool->debug_run_on_loop([&] { @@ -646,6 +643,20 @@ TEST_CASE("Network", "[network][swarm_redirect]") { CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 4))); CHECK(current_swarm() == overridden); CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // A redirect naming nodes we mostly haven't heard of is followed with the ones we have - they + // are the right swarm, where the one we had just rejected us - and asks for the refresh that + // can fill in the rest (from a fresh count, so the limit plays no part) + snode_pool->update_cache(snode_cache); + snode_pool->debug_age_pool(10min); + snode_pool->debug_age_evidence_refresh(10min); + auto partial = keys_of(elsewhere, 2); + partial.push_back(key_for(200)); + partial.push_back(key_for(201)); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, partial)); + CHECK(current_swarm() == + sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 2))); + CHECK(snode_pool->debug_refresh_in_progress()); } TEST_CASE("Network", "[network][strike_expiry]") { From fd6321f505c624220526bf2545d53e1de5c76dd4 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 17:21:02 -0300 Subject: [PATCH 12/22] Report too few usable nodes with a named error code 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. --- include/session/network/session_network_types.hpp | 1 + src/network/routing/onion_request_router.cpp | 2 +- src/network/routing/session_router_router.cpp | 4 ++-- tests/test_onion_request_router.cpp | 1 + 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/session/network/session_network_types.hpp b/include/session/network/session_network_types.hpp index 52e6e6b5b..cf29731fc 100644 --- a/include/session/network/session_network_types.hpp +++ b/include/session/network/session_network_types.hpp @@ -31,6 +31,7 @@ constexpr int16_t ERROR_INVALID_DESTINATION = -10009; constexpr int16_t ERROR_FAILED_GENERATE_ONION_PAYLOAD = -10010; constexpr int16_t ERROR_FAILED_TO_GET_STREAM = -10011; constexpr int16_t ERROR_UNAUTHENTICATED_RESPONSE = -10012; +constexpr int16_t ERROR_INSUFFICIENT_NODES = -10013; constexpr int16_t ERROR_BUILD_TIMEOUT = -10100; constexpr int16_t ERROR_REQUEST_CANCELLED = -10200; constexpr int16_t ERROR_UNKNOWN = -11000; diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index c828f1f21..0c379b173 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -1241,7 +1241,7 @@ void OnionRequestRouter::_build_path( for (const auto& [req, cb] : to_fail) cb(false, false, - -1, + ERROR_INSUFFICIENT_NODES, {content_type_plain_text}, "Cannot build a path: {}."_format(*cannot_build)); } diff --git a/src/network/routing/session_router_router.cpp b/src/network/routing/session_router_router.cpp index f8f54d814..19240cfc1 100644 --- a/src/network/routing/session_router_router.cpp +++ b/src/network/routing/session_router_router.cpp @@ -475,7 +475,7 @@ void SessionRouter::_send_proxy_request(Request request, network_response_callba return cb( false, false, - -1, + ERROR_INSUFFICIENT_NODES, {content_type_plain_text}, "Failed to refresh the snode pool to find a proxy."); @@ -492,7 +492,7 @@ void SessionRouter::_send_proxy_request(Request request, network_response_callba return cb( false, false, - -1, + ERROR_INSUFFICIENT_NODES, {content_type_plain_text}, "SnodePool refresh failed."); diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 778c608d6..4a3953559 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -1105,6 +1105,7 @@ TEST_CASE("Network", "[network][onion_request_router][build_path_too_few_nodes]" CHECK(snode_pool->get_call_count("refresh_if_needed") == 1); REQUIRE(result); CHECK_FALSE(result->success); + CHECK(result->status_code == ERROR_INSUFFICIENT_NODES); CHECK(result->response.value_or("").find("too few usable nodes") != std::string::npos); } } // namespace session::network From c038a7be9c12579c79c77eec3bdc6f0a5cb3aceb Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 17:37:18 -0300 Subject: [PATCH 13/22] Actually suspend the QUIC transport, and close without blaming nodes 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). --- src/network/session_network.cpp | 13 ++-- src/network/transport/quic_transport.cpp | 43 +++++++------ tests/CMakeLists.txt | 1 + tests/test_quic_transport.cpp | 80 ++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 22 deletions(-) create mode 100644 tests/test_quic_transport.cpp diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 3662479a6..307ba7e85 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -293,12 +293,14 @@ void Network::suspend() { _loop->call_get([this] { _suspended = true; + // The router before the transport, so that it has let go of its path builds and failure + // listeners before the transport closes the connections they're attached to if (_snode_pool) _snode_pool->suspend(); - if (_transport) - _transport->suspend(); if (_router) _router->suspend(); + if (_transport) + _transport->suspend(); _close_connections(); }); @@ -651,10 +653,13 @@ void Network::download(DownloadRequest request) { // MARK: Internal Logic void Network::_close_connections() { - if (_transport) - _transport->close_connections(); + // The router first, for the same reason as in `suspend`: otherwise each edge connection the + // transport closes fires a failure listener into a live router, which retires the path and + // starts rebuilding it if (_router) _router->close_connections(); + if (_transport) + _transport->close_connections(); _recalculate_status(); log::info(cat, "Closed all connections."); diff --git a/src/network/transport/quic_transport.cpp b/src/network/transport/quic_transport.cpp index f90f72007..a86fb4bc3 100644 --- a/src/network/transport/quic_transport.cpp +++ b/src/network/transport/quic_transport.cpp @@ -51,7 +51,7 @@ QuicTransport::~QuicTransport() { void QuicTransport::suspend() { // Use 'call_get' to force this to be synchronous _loop->call_get([this] { - if (!_suspended) + if (_suspended) return; _suspended = true; @@ -156,34 +156,41 @@ void QuicTransport::_recreate_endpoint() { } void QuicTransport::_close_connections() { - // Explicitly close all connections then reset the endpoint + // Taken out before the endpoint goes. Resetting it destroys it on the spot (close_conns() + // only defers), and its destructor closes every connection with code 0 through the handler + // that fails whatever is pending on it - so left in place, our own close would reach each + // pending verification as an error code, which is what gets a node struck. Answering copies + // also keeps a callback that calls back into the transport from changing the maps being walked. + auto verifications = std::exchange(_pending_verification_callbacks, {}); + auto requests = std::exchange(_pending_requests, {}); + if (_endpoint) _endpoint->close_conns(); _endpoint.reset(); - // Cancel any pending verifications (they can't succeed once the connection is closed) - for (const auto& [pubkey, callbacks] : _pending_verification_callbacks) + // Clear all storage of requests, paths and connections so that we are in a fresh state on + // relaunch + _active_connection_ids.clear(); + _available_stream_ids.clear(); + _update_status(ConnectionStatus::disconnected); + + // Answered only now the transport is fully closed, so anything a callback asks of it fails + // cleanly, without an error code, rather than meeting half-closed state. A verification gets + // no error code either: a close we asked for is no evidence against the node. + for (const auto& [pubkey, callbacks] : verifications) for (const auto& callback : callbacks) - callback(false, -1); + callback(false, std::nullopt); - // Cancel any pending requests (they can't succeed once the connection is closed) - for (const auto& [pubkey, pupkey_requests] : _pending_requests) - for (const auto& [info, callback] : pupkey_requests) + for (const auto& [pubkey, pubkey_requests] : requests) + for (const auto& [info, callback] : pubkey_requests) callback( false, false, - ERROR_NETWORK_SUSPENDED, + (_suspended ? ERROR_NETWORK_SUSPENDED : ERROR_CONNECTION_CLOSED), {content_type_plain_text}, - "QuickTransport is suspended."); + (_suspended ? "QuickTransport is suspended." + : "QuickTransport closed its connections.")); - // Clear all storage of requests, paths and connections so that we are in a fresh state on - // relaunch - _active_connection_ids.clear(); - _available_stream_ids.clear(); - _pending_verification_callbacks.clear(); - _pending_requests.clear(); - - _update_status(ConnectionStatus::disconnected); log::info(cat, "Closed all connections."); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2ed0d1af5..3f0cd7f9a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -44,6 +44,7 @@ if(ENABLE_NETWORKING) list(APPEND LIB_SESSION_UTESTS_SOURCES test_network_swarm.cpp) list(APPEND LIB_SESSION_UTESTS_SOURCES test_onionreq.cpp) list(APPEND LIB_SESSION_UTESTS_SOURCES test_onion_request_router.cpp) + list(APPEND LIB_SESSION_UTESTS_SOURCES test_quic_transport.cpp) list(APPEND LIB_SESSION_UTESTS_SOURCES test_snode_pool.cpp) endif() diff --git a/tests/test_quic_transport.cpp b/tests/test_quic_transport.cpp new file mode 100644 index 000000000..52464e236 --- /dev/null +++ b/tests/test_quic_transport.cpp @@ -0,0 +1,80 @@ +#include +#include +#include +#include + +using namespace session::network; +using namespace std::literals; + +namespace { +std::shared_ptr make_transport(std::shared_ptr loop) { + return std::make_shared( + config::QuicTransport{ + .handshake_timeout = 1s, .keep_alive = 10s, .disable_mtu_discovery = false}, + std::move(loop)); +} + +service_node test_node() { + auto pubkey = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"sv; + return service_node{ + ed25519_pubkey::from_hex(pubkey), + oxen::quic::ipv4{"127.0.0.1"}, + 20001, + 30001, + {2, 11, 0}, + 0}; +} +} // namespace + +TEST_CASE("Network", "[network][quic_transport][suspend]") { + auto loop = std::make_shared(); + auto transport = make_transport(loop); + transport->suspend(); + + std::promise status; + transport->send_request( + Request{"AAAA", test_node(), "info", std::nullopt, RequestCategory::standard, 1s}, + [&status, answered = false](bool, bool, int16_t status_code, auto, auto) mutable { + if (!std::exchange(answered, true)) + status.set_value(status_code); + }); + + // Refused outright rather than attempted: a suspended transport sends nothing + auto answer = status.get_future(); + REQUIRE(answer.wait_for(5s) == std::future_status::ready); + CHECK(answer.get() == ERROR_NETWORK_SUSPENDED); +} + +TEST_CASE("Network", "[network][quic_transport][close_connections]") { + auto loop = std::make_shared(); + auto transport = make_transport(loop); + + // Verified and closed within one loop job, so nothing from the network can settle the + // verification before the close does + int answers = 0; + bool success = true; + std::optional error_code; + loop->call_get([&] { + transport->verify_connectivity( + test_node(), + 3s, + "AAAA", + RequestCategory::standard, + [&](bool s, std::optional e) { + ++answers; + success = s; + error_code = e; + }); + transport->close_connections(); + }); + + // Anything the close deferred has run by the time a later job does + loop->call_get([] {}); + + // One answer, from the close, and with no error code: a close we asked for is no evidence + // against the node, which is what an error code would say - the router strikes a node whose + // verification fails with one + CHECK(answers == 1); + CHECK_FALSE(success); + CHECK_FALSE(error_code); +} From 8ee229314810b8d01e8d653b70eb3887e211e527 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 19:33:30 -0300 Subject: [PATCH 14/22] Abandon a snode pool refresh when the pool is suspended 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. --- src/network/snode_pool.cpp | 32 ++++++++-- tests/test_snode_pool.cpp | 124 +++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 5 deletions(-) diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 788bd0fea..842d6ea48 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -361,9 +361,8 @@ void SnodePool::_refresh_snode_cache(std::optional request_id_opt) log::info(cat, "Ignoring refresh as pool is suspended."); // Anything queued against a refresh we are declining to start has nothing left to wait - // for, unless one is already in flight and still owns it - if (!_current_snode_cache_refresh_id) - _run_pending_refresh_callbacks(false); + // for: suspending abandons any refresh in progress, so none can answer it later + _run_pending_refresh_callbacks(false); return; } @@ -464,7 +463,9 @@ void SnodePool::_launch_next_refresh_request( refreshing_from_seed_nodes, use_direct_fetcher, total_requests] { - if (!_current_snode_cache_refresh_id) + // A retry scheduled for a refresh that has since been abandoned or replaced mustn't touch + // the one in its place - its candidates in particular + if (_current_snode_cache_refresh_id != request_id) return; const auto target_request_id = "{}-{}"_format(request_id, index); @@ -570,7 +571,7 @@ void SnodePool::_launch_next_refresh_request( "trying again in {}ms.", target_request_id, delay.count()); - _loop->call_later(delay, [weak_self = weak_from_this(), this] { + _loop->call_later(delay, [weak_self = weak_from_this(), this, request_id] { // We need to wait until after the `call_later` to reset the `refresh_id` (and clear // previous results) as if we don't then additional refreshes could be triggered // during the delay @@ -578,6 +579,11 @@ void SnodePool::_launch_next_refresh_request( if (!self) return; + // Abandoned (by a suspend) or replaced during the delay: resetting the id would + // wipe out whichever refresh has it now, and there is nothing of ours to restart + if (_current_snode_cache_refresh_id != request_id) + return; + _current_snode_cache_refresh_id.reset(); _snode_refresh_results.clear(); _refresh_snode_cache(); @@ -959,6 +965,22 @@ void SnodePool::suspend() { _loop->call_get([this] { _suspended = true; + // A refresh in progress can only fail from here on, since every fetch is refused while + // suspended, so it is abandoned rather than left to work its way through the candidates, + // and whatever waits on it is answered now. The failures it built up are dropped too: + // they say nothing about the network we resume on. + if (_current_snode_cache_refresh_id) { + log::info( + cat, + "[Request {}] Abandoning cache refresh for suspension.", + *_current_snode_cache_refresh_id); + _current_snode_cache_refresh_id.reset(); + _refresh_candidate_nodes.clear(); + _snode_refresh_results.clear(); + _snode_cache_refresh_failure_count = 0; + _run_pending_refresh_callbacks(false); + } + // Force a strike write immediately if we had one scheduled if (_strikes_flush_scheduled) _disk_loop->call([path = _strikes_file_path, strikes = _snode_strikes] { diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index ce2dd0fee..de732ef60 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -1,5 +1,6 @@ #include #include +#include #include "utils.hpp" @@ -73,6 +74,37 @@ class TestSnodePool : public SnodePool { return _loop->call_get([this] { return _current_snode_cache_refresh_id.has_value(); }); } + void debug_start_refresh() { + _loop->call_get([this] { _refresh_snode_cache(); }); + } + + std::optional debug_refresh_id() { + return _loop->call_get([this] { return _current_snode_cache_refresh_id; }); + } + + size_t debug_refresh_candidate_count() { + return _loop->call_get([this] { return _refresh_candidate_nodes.size(); }); + } + + void debug_clear_refresh_candidates() { + _loop->call_get([this] { _refresh_candidate_nodes.clear(); }); + } + + int debug_refresh_failure_count() { + return _loop->call_get([this] { return _snode_cache_refresh_failure_count; }); + } + + void debug_set_refresh_failure_count(int count) { + _loop->call_get([this, count] { _snode_cache_refresh_failure_count = count; }); + } + + // Launches one of the refresh `request_id`'s fetches, as its retries do + void debug_launch_refresh_request(const std::string& request_id) { + _loop->call_get([this, request_id] { + _launch_next_refresh_request(request_id, 0, false, true, 1); + }); + } + // Backdates the pool snapshot so the age-based policies can be exercised without waiting void debug_age_pool(std::chrono::seconds by) { _loop->call_get([this, by] { _last_snode_cache_update -= by; }); @@ -768,3 +800,95 @@ TEST_CASE("Network", "[network][refresh_callback_contract]") { CHECK(called); CHECK_FALSE(refreshed); } + +TEST_CASE("Network", "[network][suspend_refresh]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 1, + .cache_min_swarm_size = 1, + .cache_num_nodes_to_use_for_refresh = 1, + .cache_min_num_refresh_presence_to_include_node = 1, + .cache_node_strike_threshold = 3}; + + std::vector snode_cache; + for (uint16_t i = 0; i < 4; ++i) + snode_cache.emplace_back(service_node{ + ed25519_pubkey::from_hex( + "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46a{:02x}"_format( + i)), + oxen::quic::ipv4{"192.168.0.{}"_format(i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + 0}); + + // The default fetcher never answers, so a refresh stays in progress until the test ends it + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->update_cache(snode_cache); + + SECTION("Suspending abandons a refresh in progress, and answers what waits on it") { + snode_pool->debug_start_refresh(); + REQUIRE(snode_pool->debug_refresh_in_progress()); + + std::optional answer; + snode_pool->debug_queue_post_refresh_callback( + [&answer](bool refreshed) { answer = refreshed; }); + snode_pool->debug_set_refresh_failure_count(3); + + snode_pool->suspend(); + CHECK(answer == false); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + CHECK(snode_pool->debug_refresh_failure_count() == 0); + } + + SECTION("A fetch belonging to an abandoned refresh leaves the next refresh alone") { + snode_pool->debug_start_refresh(); + auto abandoned = snode_pool->debug_refresh_id(); + REQUIRE(abandoned); + snode_pool->suspend(); + snode_pool->resume(); + + snode_pool->debug_start_refresh(); + auto current = snode_pool->debug_refresh_id(); + REQUIRE(current); + REQUIRE(current != abandoned); + auto candidates = snode_pool->debug_refresh_candidate_count(); + + snode_pool->debug_launch_refresh_request(*abandoned); + CHECK(snode_pool->debug_refresh_id() == current); + CHECK(snode_pool->debug_refresh_candidate_count() == candidates); + } + + SECTION("An abandoned refresh's retry after running out of candidates restarts nothing") { + // Running out schedules a retry of the whole refresh, after the retry delay + auto run_out_then_suspend = [&] { + snode_pool->debug_start_refresh(); + auto id = snode_pool->debug_refresh_id(); + REQUIRE(id); + snode_pool->debug_clear_refresh_candidates(); + snode_pool->debug_launch_refresh_request(*id); + snode_pool->suspend(); + snode_pool->resume(); + }; + auto past_retry_delay = [] { std::this_thread::sleep_for(500ms); }; + + // Once it was the refresh that used to restart itself here + run_out_then_suspend(); + past_retry_delay(); + CHECK_FALSE(snode_pool->debug_refresh_in_progress()); + + // ... and with another refresh started by the time it fires, that one is left running + run_out_then_suspend(); + snode_pool->debug_start_refresh(); + auto current = snode_pool->debug_refresh_id(); + REQUIRE(current); + past_retry_delay(); + CHECK(snode_pool->debug_refresh_id() == current); + } +} From 561054cb2ade8c1adff796d08e8cc336d49f13dc Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 19:48:13 -0300 Subject: [PATCH 15/22] Give the older snode pool tests the config they were written for 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. --- tests/test_snode_pool.cpp | 91 ++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 49 deletions(-) diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index de732ef60..f56b12376 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -201,18 +201,12 @@ std::vector to_snode_cache_bin(const std::vector& nodes TEST_CASE("Network", "[network][get_unused_nodes]") { session::network::config::SnodePool pool_config = { - std::nullopt, - std::nullopt, - std::chrono::minutes{5}, - std::chrono::minutes{5}, - false, // enforce_subnet_diversity - network::opt::retry_delay{50ms, 200ms}, - opt::netid::Target::testnet, - {}, - 0, - 0, - 3, // cache_node_strike_threshold - false}; + .cache_expiration = std::chrono::minutes{5}, + .cache_min_lifetime = std::chrono::minutes{5}, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_node_strike_threshold = 3}; auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; auto ed_pk2 = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; auto ed_pk3 = "e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5"_hexbytes; @@ -297,20 +291,31 @@ TEST_CASE("Network", "[network][get_unused_nodes]") { std::sort(unused_nodes.begin(), unused_nodes.end()); CHECK(unused_nodes == remaining); + // ... but only once they have passed it: ordinary strikes below the threshold leave a node in + // use (strikes are per pubkey, so these apply to every node sharing the struck node's key) + snode_pool->reset_state_with_cache(snode_cache); + auto struck_key = snode_cache[0].remote_pubkey; + auto uses_struck_key = [&struck_key](const service_node& n) { + return n.remote_pubkey == struck_key; + }; + auto with_struck_key = std::ranges::count_if(snode_cache, uses_struck_key); + + snode_pool->record_node_failure(snode_cache[0]); + snode_pool->record_node_failure(snode_cache[0]); + CHECK(std::ranges::count_if(snode_pool->get_unused_nodes(20), uses_struck_key) == + with_struck_key); + + snode_pool->record_node_failure(snode_cache[0]); + CHECK(std::ranges::none_of(snode_pool->get_unused_nodes(20), uses_struck_key)); + // Should exclude nodes which have the same subnet pool_config = { - std::nullopt, - std::nullopt, - std::chrono::minutes{5}, - std::chrono::minutes{5}, - true, // enforce_subnet_diversity - network::opt::retry_delay{50ms, 200ms}, - opt::netid::Target::testnet, - {}, - 0, - 0, - 3, // cache_node_strike_threshold - false}; + .cache_expiration = std::chrono::minutes{5}, + .cache_min_lifetime = std::chrono::minutes{5}, + .enforce_subnet_diversity = true, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_node_strike_threshold = 3}; snode_pool = std::make_shared(pool_config, loop, disk_loop); snode_pool->reset_state_with_cache(snode_cache); unused_nodes = snode_pool->get_unused_nodes(20); @@ -325,18 +330,12 @@ TEST_CASE("Network", "[network][get_unused_nodes]") { TEST_CASE("Network", "[network][update_cache]") { session::network::config::SnodePool pool_config = { - std::nullopt, - std::nullopt, - 5min, - 5min, - false, // enforce_subnet_diversity - network::opt::retry_delay{50ms, 200ms}, - opt::netid::Target::testnet, - {}, - 0, - 0, - 3, // cache_node_strike_threshold - false}; + .cache_expiration = 5min, + .cache_min_lifetime = 5min, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_node_strike_threshold = 3}; auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; std::vector snode_cache; @@ -380,19 +379,13 @@ TEST_CASE("Network", "[network][update_cache]") { TEST_CASE("Network", "[network][refresh_min_cache_size]") { session::network::config::SnodePool pool_config = { - std::nullopt, - std::nullopt, - 5min, - 5min, - false, // enforce_subnet_diversity - network::opt::retry_delay{50ms, 200ms}, - opt::netid::Target::testnet, - {}, - 12, // cache_min_size - 0, - 0, - 3, // cache_node_strike_threshold - false}; + .cache_expiration = 5min, + .cache_min_lifetime = 5min, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_size = 12, + .cache_node_strike_threshold = 3}; auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; std::vector snode_cache; From c84039c1ba336426122cb70645a79763509bb009 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 19:53:37 -0300 Subject: [PATCH 16/22] Test that rotating a path drops a struck-out edge 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. --- tests/test_onion_request_router.cpp | 58 ++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 4a3953559..20ac44c7c 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -25,7 +25,7 @@ class TestOnionRequestRouter { std::shared_ptr router, PathCategory category, std::vector paths) { - router->_paths.emplace(category, paths); + router->_paths[category] = std::move(paths); } static std::vector get_paths( @@ -86,6 +86,13 @@ class TestOnionRequestRouter { router->_send_on_path(path, std::move(request), std::move(callback)); } + static void rotate_path( + std::shared_ptr router, + const std::string& path_id, + PathCategory category) { + router->_rotate_path(path_id, category); + } + static void handle_transport_response( std::shared_ptr router, std::string path_id, @@ -200,8 +207,12 @@ namespace { // Do nothing (don't want to trigger a cache refresh) } + // The `count` of each `get_unused_nodes` call, which the mock result doesn't reflect + std::vector unused_node_counts; + std::vector get_unused_nodes( size_t count, const std::vector& exclude = {}) override { + unused_node_counts.push_back(count); if (check_should_ignore_and_log_call("get_unused_nodes")) return {}; @@ -1108,4 +1119,49 @@ TEST_CASE("Network", "[network][onion_request_router][build_path_too_few_nodes]" CHECK(result->status_code == ERROR_INSUFFICIENT_NODES); CHECK(result->response.value_or("").find("too few usable nodes") != std::string::npos); } + +TEST_CASE("Network", "[network][onion_request_router][rotate_path]") { + auto key1 = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"sv; + auto key2 = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"sv; + auto key3 = "e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5"sv; + auto key4 = "7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f"sv; + auto edge = test_node(key1, 20001); + auto path_nodes = std::vector{edge, test_node(key2, 20002), test_node(key3, 20003)}; + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(test_pool_config(), loop, disk_loop); + auto transport = std::make_shared(); + auto config = test_router_config(); + auto router = + std::make_shared(config, loop, disk_loop, snode_pool, transport); + snode_pool->mock_unused_nodes = + std::vector{test_node(key2, 20012), test_node(key3, 20013), test_node(key4, 20014)}; + + // Rotates a path whose edge node was connected just now, so well inside + // `edge_node_cache_duration`, and returns what the rotation asked the pool for. The first + // request says which it chose: a whole new path, or the rest of one that keeps the edge node. + auto rotate = [&] { + std::vector counts; + loop->call_get([&] { + auto now = std::chrono::system_clock::now(); + TestOnionRequestRouter::set_paths( + router, PathCategory::standard, {OnionPath{"Test", path_nodes, now, now}}); + snode_pool->unused_node_counts.clear(); + TestOnionRequestRouter::rotate_path(router, "Test", PathCategory::standard); + counts = snode_pool->unused_node_counts; + }); + return counts; + }; + + auto kept = rotate(); + REQUIRE_FALSE(kept.empty()); + CHECK(kept.front() == config.path_length - 1u); + + // Struck out since we connected to it, the edge node is replaced along with the rest + snode_pool->record_node_failure(edge, true); + auto replaced = rotate(); + REQUIRE_FALSE(replaced.empty()); + CHECK(replaced.front() == config.path_length); +} } // namespace session::network From aeac6b6d9dfaf596b57791d44dd9bbe078a94c95 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 20:13:48 -0300 Subject: [PATCH 17/22] Check the pool's age on every get_swarm, not only the first per account 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. --- src/network/snode_pool.cpp | 14 +++++---- tests/test_snode_pool.cpp | 61 +++++++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 842d6ea48..4c7320034 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1397,6 +1397,14 @@ void SnodePool::get_swarm( log::trace(cat, "{} called for {}.", __PRETTY_FUNCTION__, swarm_pubkey.hex()); _loop->call([this, swarm_pubkey, ignore_strike_count, cb = std::move(callback)] { + // Trigger a non-blocking background refresh if the data is stale. Up here so that it runs + // for every call: answers from an override or the swarm cache below return early, and + // they are most of them. + _loop->call_soon([weak_self = weak_from_this()] { + if (auto self = weak_self.lock()) + self->refresh_if_needed({}); + }); + auto filter_by_strikes = [this](std::vector nodes) -> std::vector { // Shuffle everything to start with @@ -1473,12 +1481,6 @@ void SnodePool::get_swarm( return; } - // Trigger a non-blocking background refresh if the data is stale - _loop->call_soon([weak_self = weak_from_this()] { - if (auto self = weak_self.lock()) - self->refresh_if_needed({}); - }); - // Perform the swarm calculation using our local copy of the data auto swarm = swarm::get_swarm(swarm_pubkey, _all_swarms); _swarm_cache[swarm_pubkey] = swarm; diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index f56b12376..5b57f49fe 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -31,10 +31,15 @@ class TestSnodePool : public SnodePool { }); } + // How often the pool was asked to check whether it needs refreshing; read it only after a + // loop job has run, since the checks are scheduled onto the loop + int refresh_checks = 0; + void refresh_if_needed( const std::vector& /*in_use_nodes*/, refresh_callback_t /*on_refresh_complete*/ = nullptr) override { - // Do nothing (don't want to trigger a cache refresh) + // Don't trigger a cache refresh + ++refresh_checks; } void debug_queue_post_refresh_callback(refresh_callback_t cb) { @@ -684,6 +689,60 @@ TEST_CASE("Network", "[network][swarm_redirect]") { CHECK(snode_pool->debug_refresh_in_progress()); } +TEST_CASE("Network", "[network][get_swarm_checks_age]") { + session::network::config::SnodePool pool_config{ + .cache_expiration = 2h, + .cache_min_lifetime = 2s, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .cache_min_swarm_size = 3, + .cache_node_strike_threshold = 3}; + + std::vector snode_cache; + for (uint16_t i = 0; i < 6; ++i) + snode_cache.emplace_back(service_node{ + ed25519_pubkey::from_hex( + "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46a{:02x}"_format( + i)), + oxen::quic::ipv4{"192.168.0.{}"_format(i)}, + static_cast(20000 + i), + static_cast(30000 + i), + {2, 11, 0}, + static_cast(i < 3 ? 0 : 1)}); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->update_cache(snode_cache); + + auto swarm_pubkey = x25519_pubkey::from_hex( + "0000000000000000000000000000000000000000000000000000000000000000"); + + // The check is scheduled onto the loop, so it has run by the time a later loop job does + auto checks_for_one_get_swarm = [&] { + snode_pool->debug_run_on_loop([&] { + snode_pool->refresh_checks = 0; + snode_pool->get_swarm(swarm_pubkey, true, [](auto, auto) {}); + }); + snode_pool->debug_run_on_loop([] {}); + return snode_pool->refresh_checks; + }; + + // Worked out from the pool and cached... + CHECK(checks_for_one_get_swarm() == 1); + + // ... then answered from that cache, and from an override, each of which returns before the + // swarm is worked out, and neither of which may skip checking whether the pool is stale + CHECK(checks_for_one_get_swarm() == 1); + + std::vector redirect; + for (const auto& node : snode_cache) + redirect.push_back(node.remote_pubkey); + REQUIRE(snode_pool->record_swarm_redirect(swarm_pubkey, redirect)); + CHECK(checks_for_one_get_swarm() == 1); +} + TEST_CASE("Network", "[network][strike_expiry]") { session::network::config::SnodePool pool_config{ .cache_expiration = 5min, From 044ef5164ac645eff01d84df03b272958d7a5f2c Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 20:15:11 -0300 Subject: [PATCH 18/22] Fall back on the C API's swarm pubkey for sub-requests naming no account 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. --- include/session/network/session_network_types.hpp | 9 ++++++--- src/network/session_network.cpp | 10 +++++++--- src/network/session_network_types.cpp | 6 +++++- tests/test_network_swarm.cpp | 5 +++++ 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/include/session/network/session_network_types.hpp b/include/session/network/session_network_types.hpp index cf29731fc..efecd7e01 100644 --- a/include/session/network/session_network_types.hpp +++ b/include/session/network/session_network_types.hpp @@ -272,10 +272,13 @@ using network_response_callback_t = std::function response)>; /// The accounts of each sub-request in a pregenerated `batch`/`sequence` body, in the form -/// `Request::swarm_pubkeys` takes, for a request whose body was built by the caller. Returns -/// nullopt when `endpoint` is neither or the body can't be parsed. +/// `Request::swarm_pubkeys` takes, for a request whose body was built by the caller. A +/// sub-request whose params name no account gets `fallback` instead: the account the caller said +/// the request is for. Returns nullopt when `endpoint` is neither or the body can't be parsed. std::optional>> batch_request_accounts( - std::string_view endpoint, std::span body); + std::string_view endpoint, + std::span body, + std::optional fallback = std::nullopt); namespace response { std::optional> parse_text_error(std::string_view body); diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 307ba7e85..1270771db 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -1862,13 +1862,17 @@ LIBSESSION_C_API void session_network_send_request( std::nullopt, request_id}; + std::optional swarm_pubkey; + if (params->swarm_pubkey_hex) + swarm_pubkey = x25519_pubkey::from_hex({params->swarm_pubkey_hex, 64}); + std::optional>> batch_accounts; if (request.body) - batch_accounts = batch_request_accounts(request.endpoint, *request.body); + batch_accounts = batch_request_accounts(request.endpoint, *request.body, swarm_pubkey); if (batch_accounts) request.swarm_pubkeys = std::move(*batch_accounts); - else if (params->swarm_pubkey_hex) - request.swarm_pubkeys = {x25519_pubkey::from_hex({params->swarm_pubkey_hex, 64})}; + else if (swarm_pubkey) + request.swarm_pubkeys = {*swarm_pubkey}; auto cpp_callback = [c_cb = callback, c_ctx = ctx]( bool success, diff --git a/src/network/session_network_types.cpp b/src/network/session_network_types.cpp index f64e6a0d6..7f5af921a 100644 --- a/src/network/session_network_types.cpp +++ b/src/network/session_network_types.cpp @@ -170,7 +170,9 @@ std::vector response::subresponses(const nlohmann::json& } std::optional>> batch_request_accounts( - std::string_view endpoint, std::span body) { + std::string_view endpoint, + std::span body, + std::optional fallback) { if (endpoint != "batch" && endpoint != "sequence") return std::nullopt; @@ -186,6 +188,8 @@ std::optional>> batch_request_accounts( auto& account = accounts.emplace_back(); if (auto params = field(req, "params", &nlohmann::json::is_object)) account = account_in(*params); + if (!account) + account = fallback; } return accounts; diff --git a/tests/test_network_swarm.cpp b/tests/test_network_swarm.cpp index 1cd0df5c1..fa9dc0019 100644 --- a/tests/test_network_swarm.cpp +++ b/tests/test_network_swarm.cpp @@ -374,6 +374,11 @@ TEST_CASE("Swarm", "[network][swarm][batch_request_accounts]") { CHECK(batch_request_accounts("sequence", bytes) == accounts); CHECK_FALSE(batch_request_accounts("retrieve", bytes)); + // A sub-request naming no account takes the one the caller gave for the whole request, + // while those that name their own keep it + CHECK(batch_request_accounts("batch", bytes, account('c')) == + std::vector>{account('a'), account('c'), account('b')}); + std::string_view junk = "not json"; CHECK_FALSE(batch_request_accounts( "batch", {reinterpret_cast(junk.data()), junk.size()})); From dbb8aebea5d2802ddc05f68501f204ecbb632818 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 24 Sep 2026 20:16:22 -0300 Subject: [PATCH 19/22] Keep repeated swarm redirect logging out of the default log 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. --- src/network/snode_pool.cpp | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 4c7320034..0d345a51b 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1219,7 +1219,7 @@ void SnodePool::invalidate_swarm(x25519_pubkey swarm_pubkey, refresh_callback_t _evidence_refresh_backoff = 0s; if (pool_age < EVIDENCE_REFRESH_MIN_POOL_AGE || since_last < _evidence_refresh_backoff) - log::info( + log::debug( cat, "Swarm {} was rejected, but the pool is {}s old and the last refresh on a " "rejection was {}s ago (backoff {}s); leaving it alone.", @@ -1299,7 +1299,7 @@ bool SnodePool::record_swarm_redirect( if (redirects < std::numeric_limits::max()) ++redirects; - log::info( + log::debug( cat, "Redirected to a swarm of {} nodes for {} (redirect {} since the pool was " "refreshed).", @@ -1307,14 +1307,28 @@ bool SnodePool::record_swarm_redirect( swarm_pubkey.hex(), redirects); - if (redirects > SWARM_REDIRECTS_BEFORE_REFRESH || pool_missing_members) { + if (pool_missing_members) { log::warning( cat, - "Asking for a refresh for {}: {}.", - swarm_pubkey.hex(), - pool_missing_members - ? "its redirect names nodes our pool doesn't have" - : "{} redirects since the pool was refreshed"_format(redirects)); + "Asking for a refresh for {}: its redirect names nodes our pool doesn't have.", + swarm_pubkey.hex()); + invalidate_swarm(swarm_pubkey); + } else if (redirects > SWARM_REDIRECTS_BEFORE_REFRESH) { + // Past the limit every redirect asks again, for as long as the refresh stays + // throttled, so only crossing it is worth a warning + if (redirects == SWARM_REDIRECTS_BEFORE_REFRESH + 1) + log::warning( + cat, + "Asking for a refresh for {}: {} redirects since the pool was refreshed.", + swarm_pubkey.hex(), + redirects); + else + log::debug( + cat, + "Asking again for a refresh for {}: {} redirects since the pool was " + "refreshed.", + swarm_pubkey.hex(), + redirects); invalidate_swarm(swarm_pubkey); } From acad948095f3852eba8e50a52ef3fb313ee5048f Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 12:13:15 +1000 Subject: [PATCH 20/22] Ask for a refresh whenever a redirect resolves to too few nodes 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. --- src/network/snode_pool.cpp | 20 ++++++++++++-------- tests/test_snode_pool.cpp | 12 ++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 0d345a51b..3dc99999e 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1285,11 +1285,12 @@ bool SnodePool::record_swarm_redirect( return false; } - // Named nodes we don't have are ones our pool is too old to know, and when that leaves too - // few to work with only a refresh can fill them in; the ones we do have are still in the - // right swarm, where the swarm we had just rejected us, so they are followed meanwhile - const bool pool_missing_members = - resolved.size() < named.size() && resolved.size() < _config.cache_min_swarm_size; + // Too few of the named nodes resolved to call the result a swarm: either our pool is too + // old to know the rest, or the redirect isn't describing one. Only a refresh settles + // which, and the nodes we do have still beat the swarm that just rejected us, so they are + // followed meanwhile. Deliberately not also requiring that some names were unknown - a + // redirect naming one node we do know is the shape with no stale pool to explain it. + const bool too_few_resolved = resolved.size() < _config.cache_min_swarm_size; auto& [nodes, redirects] = _swarm_overrides[swarm_pubkey]; nodes = std::move(resolved); @@ -1307,11 +1308,14 @@ bool SnodePool::record_swarm_redirect( swarm_pubkey.hex(), redirects); - if (pool_missing_members) { + if (too_few_resolved) { log::warning( cat, - "Asking for a refresh for {}: its redirect names nodes our pool doesn't have.", - swarm_pubkey.hex()); + "Asking for a refresh for {}: only {} of the {} nodes its redirect named are " + "ones we know.", + swarm_pubkey.hex(), + nodes.size(), + named.size()); invalidate_swarm(swarm_pubkey); } else if (redirects > SWARM_REDIRECTS_BEFORE_REFRESH) { // Past the limit every redirect asks again, for as long as the refresh stays diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 5b57f49fe..1a9eeb4b2 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -687,6 +687,18 @@ TEST_CASE("Network", "[network][swarm_redirect]") { CHECK(current_swarm() == sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 2))); CHECK(snode_pool->debug_refresh_in_progress()); + + // A redirect naming too few nodes to be a swarm asks for the refresh whether or not any of the + // names were unknown. One naming a single node we do know has no stale pool to explain it, and + // is the only thing serving the account until a refresh drops it, so it is the case that most + // wants one + snode_pool->update_cache(snode_cache); + snode_pool->debug_age_pool(10min); + snode_pool->debug_age_evidence_refresh(10min); + CHECK(snode_pool->record_swarm_redirect(swarm_pubkey, keys_of(elsewhere, 1))); + CHECK(current_swarm() == + sorted(std::vector(elsewhere.begin(), elsewhere.begin() + 1))); + CHECK(snode_pool->debug_refresh_in_progress()); } TEST_CASE("Network", "[network][get_swarm_checks_age]") { From d81d252e4d9ee50dacf993e947b53e83177aee60 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 16:09:22 +1000 Subject: [PATCH 21/22] Silence the disable_mtu_discovery deprecation until the pin can move 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. --- src/network/transport/quic_transport.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/network/transport/quic_transport.cpp b/src/network/transport/quic_transport.cpp index a86fb4bc3..a743944db 100644 --- a/src/network/transport/quic_transport.cpp +++ b/src/network/transport/quic_transport.cpp @@ -148,11 +148,24 @@ void QuicTransport::_recreate_endpoint() { // The optional must CONTAIN the option to have any effect: libquic's optional-taking // handle_ep_opt overload does nothing when the optional is empty, so a default-constructed // std::optional{} silently leaves discovery enabled. + // + // TODO: replace with quic::opt::max_udp_payload::minimum(), which is the same value under the + // name that isn't deprecated. That needs a newer libquic than this pin carries, and moving the + // pin needs the dependency handling in external/CMakeLists.txt moved onto session-deps first - + // the newer session-router defines oxen::quic itself, which collides with our own definition of + // it. Both of those come over with the Session client work, and this goes when they do. + // + // Until then the deprecated name is the only spelling that compiles against both this pin and + // the newer liboxenquic the Linux CI stages take from the system, which deprecates it under + // -Werror. The replacement does not exist in the pinned copy at all. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" _endpoint = quic::Endpoint::endpoint( *_loop, quic::Address{}, (_config.disable_mtu_discovery ? std::make_optional() : std::nullopt)); +#pragma GCC diagnostic pop } void QuicTransport::_close_connections() { From 588286d3f527147661e7c3e6c04855b9a1fb4a27 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 28 Sep 2026 16:25:54 +1000 Subject: [PATCH 22/22] Include what the test helpers use tests/utils.hpp uses std::condition_variable, std::mutex and the lock guards without including or , and test_config_userprofile.cpp calls std::this_thread::sleep_for without . 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. --- tests/test_config_userprofile.cpp | 1 + tests/utils.hpp | 2 ++ 2 files changed, 3 insertions(+) diff --git a/tests/test_config_userprofile.cpp b/tests/test_config_userprofile.cpp index 9133e38a8..1c72e2eb6 100644 --- a/tests/test_config_userprofile.cpp +++ b/tests/test_config_userprofile.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include "utils.hpp" diff --git a/tests/utils.hpp b/tests/utils.hpp index 047a3ba77..120637abf 100644 --- a/tests/utils.hpp +++ b/tests/utils.hpp @@ -5,7 +5,9 @@ #include #include +#include #include +#include #include #include #include