Skip to content

Recover from a 421 on the evidence rather than on the cache's age - #154

Open
mpretty-cyro wants to merge 1 commit into
session-foundation:devfrom
mpretty-cyro:fix/swarm-invalidation-on-421
Open

Recover from a 421 on the evidence rather than on the cache's age#154
mpretty-cyro wants to merge 1 commit into
session-foundation:devfrom
mpretty-cyro:fix/swarm-invalidation-on-421

Conversation

@mpretty-cyro

Copy link
Copy Markdown
Collaborator

The defect

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 for any cache younger than that, then re-read the same _swarm_cache entry. The retry
therefore went to another node in the list that had just rejected us, redirect_retry_count (1) was
exhausted, and every later request for that account did the same until the cache aged out.

User-visible result: a client can be unable to interact with a swarm for up to two hours, with no
recovery except waiting out cache_expiration or a full clear_cache(). The log line even said it —
"refreshing swarm if stale" — staleness measured by the clock while we were holding direct proof.

Why evicting the swarm cache entry doesn't fix it

Worth stating, because it's the obvious-looking fix and it is a no-op:

  • _swarm_cache is only ever written with swarm::get_swarm(swarm_pubkey, _all_swarms) — a pure
    function of _all_swarms.
  • _all_swarms is swarm::generate_swarms(_snode_cache), grouping nodes by the swarm_id each node
    carried in the pool fetch.
  • Both writers of _all_swarms replace _swarm_cache wholesale in the same breath.

So the swarm cache can never disagree with the pool; erasing an entry recomputes the identical answer.
What a 421 disproves is the pool snapshot, and only a refresh replaces that.

The 421 body does carry the correct swarm, and is deliberately not used: it would take one node's
unauthenticated word for swarm membership — the node that just rejected us — where a pool refresh
establishes it from an intersection of cache_num_nodes_to_use_for_refresh (3) nodes. It would also
break the memo relationship above.

The change

SnodePool::invalidate_swarm() — evidence-driven, alongside the existing age-driven
refresh_if_needed. _handle_421_retry calls it instead.

What stops a 421 storm becoming a refresh storm. A rejection arriving after we already refreshed
on one is the refresh telling us it didn't help, so each successive one waits twice as long, capped at
cache_expiration: immediate, 1m, 2m, 4m, 8m, 16m, 32m — about 7 refreshes in the first hour, then
flattening toward today's behaviour. Quiet for twice the interval being held resets to the base delay.
The backoff is global rather than per-swarm on purpose: a refresh replaces the pool every swarm is
derived from, so a rejection for swarm B right after a refresh prompted by swarm A is equally
unhelpful to act on. A separate pool-age floor stops us refreshing a pool something else just fetched.

Worst-case outage goes from 2h to ~1 min.

Tests

New [network][invalidate_swarm] case. It cannot fail against the pre-fix tree (it calls a function
that doesn't exist there), so the old behaviour was reproduced by mutation instead — reducing
invalidate_swarm to refresh_if_needed({}, cb), exactly what _handle_421_retry used to do, fails
REQUIRE(debug_refresh_in_progress()). Deleting the backoff term fails
CHECK_FALSE(debug_refresh_in_progress()).

Full suite green (135 cases). utils/format.sh verify clean.

Notes for review

  • Request::swarm_pubkey is populated only by the client via the C API, and a request without it
    is refused a redirect outright — so this is inert for any caller that leaves it null. iOS sets it
    for .snode destinations; worth confirming as the Session 2.0 client wires up its request paths.
  • Applies cleanly to client, which carries the identical defect.

A 421 tells us, authoritatively, that the swarm we resolved for an account
is wrong.  `_handle_421_retry` answered it with `refresh_if_needed`, which
decides on `cache_expiration` (2h) and so declines on any cache younger
than that, and then re-read the same `_swarm_cache` entry - so the retry
went to another node in the list that had just rejected us, and every later
request for that account did the same until the cache aged out.

Evicting the swarm cache entry alone cannot fix it: the entry is only ever
a memo of `swarm::get_swarm(pubkey, _all_swarms)`, so it recomputes the
identical answer.  What the rejection disproves is the pool snapshot the
swarms were generated from, and only a refresh replaces that.

`invalidate_swarm` refreshes on that evidence, with a backoff that doubles
each time another rejection arrives after a refresh we already ran for one,
so a node rejecting everything costs a handful of refreshes over a couple
of hours rather than one every minute for as long as it keeps it up.
@mpretty-cyro
mpretty-cyro marked this pull request as ready for review September 11, 2026 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant