From 21d90139efe1484c67accca21320ad3a7dd08125 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 10:18:51 +1000 Subject: [PATCH 1/9] 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 787e2c582..ba25de359 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -115,6 +115,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 d4f61ad9a..bb9c5e13b 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -963,6 +963,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); } @@ -970,18 +989,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) { @@ -1009,22 +1027,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() { @@ -1068,9 +1072,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 @@ -1158,9 +1160,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 @@ -1196,8 +1196,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 d470ed193..57052dc43 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -69,6 +69,23 @@ class TestSnodePool : public SnodePool { // loop thread void update_cache(std::vector nodes) { _update_cache("test", std::move(nodes)); } + // 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([&] { @@ -336,3 +353,57 @@ 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][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 ef30d0293ccece89774021945848697e2d70ceea Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 11 Sep 2026 10:50:40 +1000 Subject: [PATCH 2/9] 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 bb9c5e13b..9643078a8 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1031,6 +1031,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 02283e9c353bd86c27e1bd90535632f45dad02a6 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 09:56:33 +1000 Subject: [PATCH 3/9] Apply the strike bar through node_struck_out at the last two sites Both compared a count against a threshold directly, so both missed the `strikes > 0` guard: at a threshold of 0 the path-repair check queues every node in every path on each failure, and get_swarm's partition puts every node in the over-threshold group, returning an empty swarm when cache_min_swarm_size is also 0. get_swarm keeps get_strike_count for the stable-sort key below; only the partition predicate becomes the boolean. The router's own config::OnionRequestRouter::node_strike_threshold is now unread. It is a public field, so removing it is left for its own change. --- src/network/routing/onion_request_router.cpp | 3 +-- src/network/snode_pool.cpp | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index 9915dd453..9fb5381bb 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -1748,8 +1748,7 @@ void OnionRequestRouter::_handle_transport_response( for (const auto& node : path.nodes) { auto node_key = ed25519_pubkey::from_bytes(node.view_remote_key()); - if (snode_pool->node_strike_count(node_key) >= - _config.node_strike_threshold) + if (snode_pool->node_struck_out(node_key)) nodes_to_repair.push_back(node_key); } diff --git a/src/network/snode_pool.cpp b/src/network/snode_pool.cpp index 9643078a8..4431e8961 100644 --- a/src/network/snode_pool.cpp +++ b/src/network/snode_pool.cpp @@ -1210,8 +1210,8 @@ void SnodePool::get_swarm( // Partition into below-threshold and above-thresold. This keeps the shuffled order of // each set: auto over_nodes = std::ranges::stable_partition( - nodes.begin(), nodes.end(), [&](const auto& node) { - return get_strike_count(node) < _config.cache_node_strike_threshold; + nodes.begin(), nodes.end(), [this](const auto& node) { + return !_node_struck_out(node.remote_pubkey); }); auto under_count = nodes.size() - over_nodes.size(); From 84fbb5d78d0c16b7063f630e3a90d82a24ea2fcb Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 09:56:33 +1000 Subject: [PATCH 4/9] Give a path built with a new edge node its own connected-at stamp The rebuild branch replaces every node including the edge, but the new path inherited the previous edge node's edge_first_connected_at. A replacement built after the old edge had served out edge_node_cache_duration was therefore already expired on arrival, so the next rotation discarded it too and no edge node ever kept its stickiness. --- src/network/routing/onion_request_router.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index 9fb5381bb..e96b2c798 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -2161,8 +2161,11 @@ void OnionRequestRouter::_rotate_path(const std::string& path_id, PathCategory c 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 || - snode_pool->node_struck_out(edge_node)) + bool new_edge = + (now > path.edge_first_connected_at + _config.edge_node_cache_duration || + snode_pool->node_struck_out(edge_node)); + + if (new_edge) rotated_path_nodes = snode_pool->get_unused_nodes(_config.path_length, nodes_to_exclude); else { rotated_path_nodes = @@ -2186,7 +2189,10 @@ void OnionRequestRouter::_rotate_path(const std::string& path_id, PathCategory c } OnionPath new_path{ - new_path_id, std::move(rotated_path_nodes), now, path.edge_first_connected_at}; + new_path_id, + std::move(rotated_path_nodes), + now, + (new_edge ? now : path.edge_first_connected_at)}; // Send /info request to verify path before rotating Request info_request{ From 6159c91a606e876f9c22650a92ce0228881cfa1b Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 09:56:33 +1000 Subject: [PATCH 5/9] Name the fields in the SnodePool test configs The three positional setups were one short of the struct, so every value from cache_num_nodes_to_use_for_refresh on landed a field early and the trailing comments named the wrong ones. The values are carried over exactly as they resolve today, which means cache_node_strike_threshold stays 0: these cases have never run against the production threshold, and changing that wants a look at each expectation rather than a rename. --- tests/test_snode_pool.cpp | 76 ++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 57052dc43..0ca894535 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -124,18 +124,19 @@ 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_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 = 0, + .cache_node_strike_threshold = 0}; auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; auto ed_pk2 = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; auto ed_pk3 = "e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5"_hexbytes; @@ -248,18 +249,19 @@ 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_directory = std::nullopt, + .fallback_snode_pool_path = std::nullopt, + .cache_expiration = 5min, + .cache_min_lifetime = 5min, + .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 = 0, + .cache_node_strike_threshold = 0}; auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; std::vector snode_cache; @@ -303,19 +305,19 @@ 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_directory = std::nullopt, + .fallback_snode_pool_path = std::nullopt, + .cache_expiration = 5min, + .cache_min_lifetime = 5min, + .enforce_subnet_diversity = false, + .retry_delay = network::opt::retry_delay{50ms, 200ms}, + .netid = opt::netid::Target::testnet, + .seed_nodes = {}, + .cache_min_size = 12, + .cache_min_swarm_size = 0, + .cache_num_nodes_to_use_for_refresh = 0, + .cache_min_num_refresh_presence_to_include_node = 3, + .cache_node_strike_threshold = 0}; auto ed_pk = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; std::vector snode_cache; From 5b553943fec7b205ee90686a7c22f801f427cf0a Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 09:56:33 +1000 Subject: [PATCH 6/9] State what holds rather than what changed, in five comments The _active_strike_count one had gone stale against this branch: prune-then- append means the stored vector is no longer every strike ever collected. The rest narrate the diff or repeat the PR description. --- include/session/network/snode_pool.hpp | 4 ++-- src/network/routing/onion_request_router.cpp | 12 +++--------- tests/test_onion_request_router.cpp | 8 +++----- tests/test_snode_pool.cpp | 2 +- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index e1ed6f860..0b3ea26ea 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -122,8 +122,8 @@ 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. + // Strikes are only pruned when the node collects a new one, so the stored vector can still + // hold expired stamps - count them out before deciding anything. size_t _active_strike_count(const ed25519_pubkey& key) const; bool _node_struck_out(const ed25519_pubkey& key) const; diff --git a/src/network/routing/onion_request_router.cpp b/src/network/routing/onion_request_router.cpp index e96b2c798..ab030d0f8 100644 --- a/src/network/routing/onion_request_router.cpp +++ b/src/network/routing/onion_request_router.cpp @@ -595,15 +595,9 @@ 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. +// A cached edge node is forced as a path's first hop, so it never passes the strike filter +// `get_unused_nodes` applies to the rest; erasing rather than skipping is what stops it reclaiming +// the role when its strikes expire. void OnionRequestRouter::_drop_struck_cached_edge_nodes() { auto snode_pool = _snode_pool.lock(); diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 910d0f217..5e348e4d8 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -906,14 +906,12 @@ TEST_CASE("Network", "[network][onion_request_router][cached_edge_nodes]") { 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 + // Both are well inside `edge_node_cache_duration`, so only a strike can drop them 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. + // Erasing rather than skipping is what stops a struck-out node reclaiming the cached-edge + // role when its strikes expire. snode_pool->SnodePool::record_node_failure(struck, true); REQUIRE(snode_pool->node_struck_out(struck)); diff --git a/tests/test_snode_pool.cpp b/tests/test_snode_pool.cpp index 0ca894535..e9f3c483d 100644 --- a/tests/test_snode_pool.cpp +++ b/tests/test_snode_pool.cpp @@ -78,7 +78,7 @@ class TestSnodePool : public SnodePool { }); } - // Every strike on record, expired or not - which is what the decision sites used to count + // Every strike on record, expired or not size_t debug_recorded_strikes(const ed25519_pubkey& key) { return _loop->call_get([this, &key] { auto it = _snode_strikes.find(key); From 5c98bb0586de604e47560d1759366883544b03f1 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 09:56:33 +1000 Subject: [PATCH 7/9] Drop virtual from node_struck_out Nothing overrides it - the tests call it on the concrete type - and being virtual puts it ahead of the existing virtuals in the vtable. --- include/session/network/snode_pool.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/session/network/snode_pool.hpp b/include/session/network/snode_pool.hpp index 0b3ea26ea..917d5a2ca 100644 --- a/include/session/network/snode_pool.hpp +++ b/include/session/network/snode_pool.hpp @@ -74,8 +74,8 @@ class SnodePool : public std::enable_shared_from_this { // 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); + bool node_struck_out(const service_node& node); + 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 From 7279db54253e3520cbafcb08bd8717a6a1401335 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 10:29:44 +1000 Subject: [PATCH 8/9] Remove the router's copy of the strike threshold Nothing reads config::OnionRequestRouter::node_strike_threshold now that the path-repair check asks node_struck_out. It was always a second copy of the pool's cache_node_strike_threshold, filled from the same place and kept in step by convention, so a caller that set one and not the other got no warning. --- include/session/network/routing/onion_request_router.hpp | 1 - src/network/session_network.cpp | 1 - tests/test_onion_request_router.cpp | 6 ------ 3 files changed, 8 deletions(-) diff --git a/include/session/network/routing/onion_request_router.hpp b/include/session/network/routing/onion_request_router.hpp index da17f3c40..78291069f 100644 --- a/include/session/network/routing/onion_request_router.hpp +++ b/include/session/network/routing/onion_request_router.hpp @@ -31,7 +31,6 @@ namespace config { uint8_t path_strike_threshold; uint8_t path_build_retry_limit; std::chrono::minutes path_rotation_frequency; - uint8_t node_strike_threshold; bool disable_pre_build_paths; bool single_path_mode; std::unordered_map min_path_counts; diff --git a/src/network/session_network.cpp b/src/network/session_network.cpp index 7d160fc41..509d38f0a 100644 --- a/src/network/session_network.cpp +++ b/src/network/session_network.cpp @@ -113,7 +113,6 @@ namespace { main_config.onionreq_path_strike_threshold, main_config.onionreq_path_build_retry_limit, main_config.onionreq_path_rotation_frequency, - main_config.cache_node_strike_threshold, main_config.onionreq_disable_pre_build_paths, main_config.onionreq_single_path_mode, main_config.onionreq_min_path_counts}; diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 5e348e4d8..57338a7dd 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -259,7 +259,6 @@ TEST_CASE("Network", "[network][onion_request_router][handle_errors]") { 3, 10, 10min, - node_strike_threshold, true, true, {{PathCategory::standard, 1}}}; @@ -570,7 +569,6 @@ TEST_CASE("Network", "[network][onion_request_router][build_path]") { 3, 10, 10min, - 3, true, true, {{PathCategory::standard, 1}}}; @@ -621,7 +619,6 @@ TEST_CASE("Network", "[network][onion_request_router][find_valid_path]") { 3, 10, 10min, - 3, true, false, {{PathCategory::standard, 1}}}; @@ -696,7 +693,6 @@ TEST_CASE("Network", "[network][onion_request_router][find_valid_path]") { 3, 10, 10min, - 3, true, true, // single path mode {{PathCategory::standard, 1}}}; @@ -731,7 +727,6 @@ TEST_CASE("Network", "[network][onion_request_router][check_request_queue_timeou 3, 10, 10min, - 3, true, false, {{PathCategory::standard, 1}}}; @@ -873,7 +868,6 @@ TEST_CASE("Network", "[network][onion_request_router][cached_edge_nodes]") { 3, 10, 10min, - node_strike_threshold, true, true, {{PathCategory::standard, 1}}}; From e4ce5f664ca1c16eafadb8469b8baf85f574a9ef Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Thu, 17 Sep 2026 10:29:44 +1000 Subject: [PATCH 9/9] Cover the pre-build wiring and the struck-edge rebuild _drop_struck_cached_edge_nodes was only ever exercised directly, so removing its one production call site left the suite green. _rotate_path's struck-edge check had no coverage at all, and it is the half that stops a live path re-adopting an edge node struck since it was built. Both cases fail if the code they cover is reverted. --- tests/test_onion_request_router.cpp | 141 ++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/tests/test_onion_request_router.cpp b/tests/test_onion_request_router.cpp index 57338a7dd..78777e8ed 100644 --- a/tests/test_onion_request_router.cpp +++ b/tests/test_onion_request_router.cpp @@ -98,6 +98,26 @@ class TestOnionRequestRouter { std::move(decrypted_body), std::move(callback)); } + + static void pre_build_paths_if_needed(std::shared_ptr router) { + router->_pre_build_paths_if_needed(); + } + + static void rotate_path( + std::shared_ptr router, + std::string path_id, + PathCategory category) { + router->_rotate_path(path_id, category); + } + + static std::optional pending_rotation_path( + std::shared_ptr router, std::string old_path_id) { + for (const auto& [_, pending] : router->_pending_rotation_paths) + if (pending.old_path_id == old_path_id) + return pending.new_path; + + return std::nullopt; + } }; namespace detail { @@ -916,4 +936,125 @@ TEST_CASE("Network", "[network][onion_request_router][cached_edge_nodes]") { REQUIRE(remaining.size() == 1); CHECK(remaining.front().node == healthy); } +namespace { + config::SnodePool strike_pool_config(uint16_t threshold) { + 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 = threshold}; + } + + config::OnionRequestRouter strike_router_config(bool disable_pre_build_paths) { + 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, + .disable_pre_build_paths = disable_pre_build_paths, + .single_path_mode = true, + .min_path_counts = {{PathCategory::standard, 1}}}; + } + + service_node strike_test_node(std::vector ed_pk, uint16_t n) { + return service_node{ + ed25519_pubkey::from_bytes(ed_pk), + oxen::quic::ipv4{"127.0.0.{}"_format(n)}, + static_cast(20000 + n), + static_cast(30000 + n), + {2, 11, 0}, + 0}; + } +} // namespace + +TEST_CASE("Network", "[network][onion_request_router][pre_build_paths]") { + auto pool_config = strike_pool_config(3); + auto config = strike_router_config(false); + auto key1 = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; + auto key2 = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; + auto key3 = "7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f"_hexbytes; + auto healthy = strike_test_node(key1, 1); + auto struck = strike_test_node(key2, 2); + + auto loop = std::make_shared(); + auto disk_loop = std::make_shared(); + auto snode_pool = std::make_shared(pool_config, loop, disk_loop); + snode_pool->mock_unused_nodes = std::vector{ + strike_test_node(key3, 11), strike_test_node(key1, 12), strike_test_node(key2, 13)}; + 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}}); + snode_pool->SnodePool::record_node_failure(struck, true); + REQUIRE(snode_pool->node_struck_out(struck)); + + // Pre-building is the only production caller of the drop, so going through it is what proves + // the two are wired together + TestOnionRequestRouter::pre_build_paths_if_needed(router); + + auto remaining = TestOnionRequestRouter::cached_edge_nodes(router); + REQUIRE(remaining.size() == 1); + CHECK(remaining.front().node == healthy); +} + +TEST_CASE("Network", "[network][onion_request_router][rotate_path]") { + auto pool_config = strike_pool_config(3); + auto config = strike_router_config(true); + auto key1 = "4cb76fdc6d32278e3f83dbf608360ecc6b65727934b85d2fb86862ff98c46ab7"_hexbytes; + auto key2 = "5ea34e72bb044654a6a23675690ef5ffaaf1656b02f93fb76655f9cbdbe89876"_hexbytes; + auto key3 = "7b633fa6fb462b90db6f0f50384190ce7715e31b7aa93d87dbd7e94e33d4251f"_hexbytes; + auto key4 = "e17a692033200ae41350df9709754edde7343e2cf2f23e88f993319e0720e5e5"_hexbytes; + auto struck = strike_test_node(key1, 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->mock_unused_nodes = std::vector{ + strike_test_node(key2, 11), strike_test_node(key3, 12), strike_test_node(key4, 13)}; + auto transport = std::make_shared(); + auto router = + std::make_shared(config, loop, disk_loop, snode_pool, transport); + + // Fresh enough that `edge_node_cache_duration` cannot be what triggers the rebuild + auto now = std::chrono::system_clock::now(); + auto edge_connected_at = now - std::chrono::days{5}; + TestOnionRequestRouter::set_paths( + router, + PathCategory::standard, + {OnionPath{ + "P1", + {struck, strike_test_node(key2, 2), strike_test_node(key3, 3)}, + now, + edge_connected_at}}); + + snode_pool->SnodePool::record_node_failure(struck, true); + REQUIRE(snode_pool->node_struck_out(struck)); + + TestOnionRequestRouter::rotate_path(router, "P1", PathCategory::standard); + + auto rotated = TestOnionRequestRouter::pending_rotation_path(router, "P1"); + REQUIRE(rotated.has_value()); + CHECK(std::ranges::find(rotated->nodes, struck) == rotated->nodes.end()); + + // An all-new path starts the edge clock now; inheriting the old stamp would make the + // replacement expire early and rotate again + CHECK(rotated->edge_first_connected_at == rotated->created_at); + CHECK(rotated->edge_first_connected_at > edge_connected_at); +} } // namespace session::network