Drop a cached edge node once it has been struck out - #156
Closed
mpretty-cyro wants to merge 2 commits into
Closed
mpretty-cyro wants to merge 2 commits into
mpretty-cyro wants to merge 2 commits into
Conversation
`STRIKE_EXPIRY` was applied in `node_strike_count()` - which nothing in this repo calls - and on the disk load. Every place that actually decides something counted the raw vector instead, and `record_node_failure` only ever appends, so a node struck during a two-minute outage stayed out of path building, swarm answers and refresh candidates for the life of the process, and its timestamp vector grew without bound. Restarting was the only thing that cleared it, because loading is the one path that filtered. Counting is now in one place, used by all four, and a node's expired strikes are dropped when it collects a new one. Two things this uncovered, both only reachable with a strike threshold of 0, which is what `tests/test_snode_pool.cpp` has been running with: - comparing the count against the threshold with a bare `>=` excludes every node in the pool, including ones that have never failed, so a threshold of 0 has to keep meaning "drop a node on its first strike". - a permanent failure looped up to the threshold, recording no strikes at all and leaving a node we know is gone in rotation.
A cached edge node is handed to `_build_path` as a forced first hop, so it is the one node in a path that never passes the strike filter `get_unused_nodes` applies to the rest, and the only thing that dropped it was `edge_node_cache_duration` (10 days). So a node we already had evidence was unreachable got another path built onto it on every launch and every resume, and a path rotation carried it into the replacement path. It loses the cached-edge role rather than its place in the pool: it stays a node like any other and can be picked again once its strikes expire. Keeping the entry and merely skipping it would hand the role back at that expiry, by which point we have been running on a different edge node for two days - a second change of first hop, not a return to a stable one.
mpretty-cyro
marked this pull request as ready for review
September 11, 2026 04:24
Collaborator
Author
|
Folded into #155 as a second commit rather than kept as a stacked PR. It depended on #155's |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
A cached edge node is handed to
_build_pathas a forced first hop, so it is the one node in a paththat never passes the strike filter
get_unused_nodesapplies to the rest. The only thing thatdropped it was
edge_node_cache_duration(10 days).So a node we already had evidence was unreachable — including one struck by
force_remove_node— gotanother path built onto it on every launch and every resume (
_pre_build_paths_if_neededrunsfrom
_finish_setup()and fromresume()), and a path rotation carried it into the replacement path.The change
The entry is dropped from
_cached_edge_nodesonce the node is struck out, at both reuse sites.Scope: it loses the cached-edge role, not its place in the pool. Nothing here touches
_snode_cache— it stays a node like any other andget_unused_nodescan pick it again once itsstrikes expire. It simply has no special claim to being our first hop any more.
Erasing rather than skipping-but-keeping is deliberate.
_cached_edge_nodesis written once, by_load_from_disk, and never again, so a retained entry stays a candidate for the whole process andwould reclaim the role when its strikes expire — 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, which
is the opposite of what the 10-day stickiness is for.
Severity
Small, and smaller than it first looked: the connection-failure retry already passes the failed edge
node as
nodes_to_exclude_, so it is excluded from the next attempt. The real cost is one wastedbuild plus one retry delay per launch, self-correcting immediately. Happy for this to be dropped if
it isn't felt to be worth the surface.
Tests
New
[network][onion_request_router][cached_edge_nodes]case. Mutation-verified: making the predicatenever erase fails
REQUIRE(remaining.size() == 1)with2 == 1.Full suite green (136 cases).
utils/format.sh verifyclean. Applies cleanly toclient.