Push notification for session-router connections - #152
Open
jagerman wants to merge 21 commits into
Open
Conversation
_fail_connection moved the listeners into a local, erased the map entry, and then iterated the erased entry: a dereference of an invalidated iterator into a moved-from vector. No listener ever fired.
set_network could always be handed a second Network, and nothing about doing so worked: it stops and starts the libevent poll ticker off the loop thread, and ~Network fails the requests its router and transport are holding, which runs Core's poll continuation against a router that has just been destroyed. Throw instead, and record on the declaration what a real replacement has to do first. Also note why the snode bootstrap fetcher bypasses the router, and that session-router mode need not.
A QUIC handshake through a Session Router tunnel was budgeted with the figure chosen for a direct connect to a node's own address: 3s to cross a multi-hop path, after which the transport concludes the storage node is unreachable and strikes it in the SnodePool -- for the latency of a connection nominally made to ::1. Split the two. Direct handshakes go to 5s; a request that the router rewrote to the local end of a tunnel is marked as such and gets its own 10s. The transport cannot infer this from the address, which is loopback either way, so the request carries it. Also read the request's category before it is moved into the pending queue rather than after, and correct two option doc comments that named defaults the code had long since changed.
Nothing could reach us except as the response to something we sent: the transport's only inbound path was a request's own callback, and the stream it sends on had no handler registered for anything arriving the other way. A swarm subscription is delivered exactly that way, as a request of the storage server's own making, so it had nowhere to land. Register a generic handler on the connection's stream and pass what arrives up through Network, naming the node by the ed25519 key the connection is addressed by -- the same key whether it reached the node directly or through a tunnel. Generic rather than per-endpoint because what the names mean belongs to the storage server, not here. Report an established connection alongside it. The far end keys a subscription on the connection, so a reconnect silently drops it, and until now nothing said a connection had come back -- only that one had failed, via a listener that never fired.
Whether a subscription is worth making is a property of the routing mode, which Core has no way to see: it holds a Network and the router type lives in that Network's config.
Whatever the far end holds for a connection dies with it, and the only thing that said so was the per-node, one-shot failure listener the onion router uses to retire a path. A subscription needs the general form: every connection this transport loses, reported for as long as anyone is listening.
Everything from the `public:` above it was already public: the only specifiers in between belong to the nested AccountSeedAccess, which come after and do not change the enclosing class's access.
A client that has drained a node's namespaces has an established connection and current cursors, which is the only state a subscription can safely start from: subscribe before that and the gap between the last retrieve and the subscription taking effect is lost. So the existing poll is what both chooses the node and prepares it, and the subscription starts where the drain finishes. From there the node pushes each new message and the poll ticker stops. Renewal runs every 30s, far inside the server's 65 minute expiry, because renewing is not all the timer is for: a subscribed client sends nothing else, so the tick's retrieve is also the only thing that can notice the node has stopped holding our swarm. Losing the connection gives the subscription up -- the far end keys it to the connection and says nothing when it lapses -- and polling resumes, which is also what picks the next node. Subscribes with d=1 so a notification carries the message rather than just its metadata: the same bytes a retrieve would have returned, so no round trip and nothing to fetch. Nothing runs at all under onion requests, where the server would key the subscription to the last relay rather than to us.
A subscription that has stopped applying is silent. The storage server runs no swarm check when subscribing and none when a swarm moves underneath one: get_notifiers simply stops matching, so a client that has given up polling cannot tell "nothing has been sent to me" from "I am subscribed to a node that no longer holds my messages". So ask it something, every 30s, purely for the error. The cheapest question that still produces a 421 is a retrieve of a namespace that needs no signature -- the server decides wrong-swarm from the pubkey on the first two lines of the handler, before the auth check -- and of one nothing is ever stored in, so there is no cursor either: 97 bytes out, 57 back, against 3.3kB for the full poll this replaces. The request carries no swarm_pubkey, which is what stops the network layer helpfully retrying the 421 on a different member and reporting success. Renewal is a separate 15 minute timer now that it no longer has to carry the probe: the server's expiry is 65 minutes, and it only applies to a connection that has stayed up that long, since losing the connection loses the subscription outright. The probe is temporary. The storage server is gaining a notification that says outright when a subscription has stopped applying and carries the replacement swarm with it; this has to outlive the last node without it.
…lback _drop_subscription is reachable from inside the tickers it destroys -- _subscription_probe calls it directly -- and Loop::call_every hands out a shared_ptr whose deleter is a call_get of the delete, which runs inline once we are already on the loop. Dropping the last reference there would free the std::function being executed and return into it. Stopping the event is safe from within it; freeing the object is not, so hand it to Loop::reset_soon, which exists for this.
Session Router never reported its paths at all -- get_active_paths() was a stub returning nothing -- so the mode that is the default showed the user nothing about where their traffic went. Filling that in meant not reusing the type the routers keep their own paths in. A hop was a service_node, which a Session Router relay is not: it has no ports, no storage server version and no swarm, so reporting one meant inventing five fields, including a swarm id of 0 that means something else. And a path's destination sat beside the hop list rather than at the end of it, as two strings in one variant's metadata and absent from the other's -- from which the destination's country, the thing being asked for, could not be looked up at all. So the user-visible shape is now its own: hops of an identity and an address, in order, and nothing that only means something inside a router. What the last hop is depends on the route and the comment says so, rather than the type promising a destination that is sometimes a guess. The question changes with it. Enumerating paths suited the onion router's pools and nothing else: Session Router holds a session to every swarm member we have spoken to, the file server and every group's swarm, so listing them buries the one route anybody wants in dozens nobody can act on. Asking about a destination is answerable by all three routers -- direct returns the node itself, one hop -- and lets each resolve internally what it used to hand over and ask the caller to filter. The C wrapper for the old call is deleted rather than followed across: it exists for Session versions that predate the Client API and no client built on Client uses it.
get_path_to picked _paths[standard].front(), which is not the path a request goes down: selection skips struck paths, skips any path containing the destination, and then orders by how busy each is. The answer looked plausible and was usually wrong -- and the destination, which I had ignored as irrelevant to an onion path, is exactly what the conflict check turns on. Split the destination and category out of _find_valid_path's Request so it can be asked without one, and ask it.
Session Router is onion routing too -- it is the more capable of the two, and the mode push notifications exist for -- so "an onion path" as shorthand for an onion-request path is not loose, it is wrong. The push hook's comment read as though onion routing could not receive pushes, when it is specifically onion_requests that cannot.
Network re-aimed a request by itself: a 421 picked a different swarm member, an unreachable node walked to the next one, and either way the caller's callback fired for a node it was never told about. So Core recorded retrieve cursors against the node it asked rather than the one that answered, and would subscribe to a node that had just said it does not hold our account. Nothing below Core can fix that, because nothing below Core knows the substitution matters. That division made sense when session-ios was the main consumer and the logic had to live under the C API to be shared at all. With Client there is a better place for it. So Network stops deciding. Both retries are gone, along with Request::retry_421_count, Request::failed_nodes and the redirect_retry_count option that bounded one of them; a 421 and an unreachable node are now reported to the caller, distinguished by status code, and what to do about either is the caller's. Two things had to change to make that possible: Network now reports the collapsed batch status rather than the raw transport one. A batch whose subrequests all failed identically arrives as a transport-level 200, so a caller could not previously tell a misdirected poll from any other failure -- which is precisely the distinction it now has to make. And Network still adopts the swarm a 421 carries, because that is its own cache and the answer is authoritative; it simply does not act on it. Whoever retries then resolves against corrected membership instead of the stale set that misdirected them, which is the swarm correction that has never happened until now. Core gains _swarm_request to make those decisions in one place, and it rebuilds the request body per attempt: a retrieve carries the chosen node's cursor, and the old path re-sent one node's cursor to another.
_poll resolved the swarm and picked a member itself, then handed that member to _send_poll and to everything downstream -- including the hash cursors and the subscription -- regardless of which member the network layer had actually reached. It now goes through _swarm_request, which reports that. Building the batch moves into _build_poll_body so the helper can rebuild it per attempt. That matters here more than anywhere else: the batch carries one cursor per namespace, and those cursors belong to a particular member, so a re-aimed poll built from the old member's body would ask the new one to resume from a position it never issued. A continuation round pins the member it is continuing against, for the same reason, and falls back to choosing normally if that member has become unusable.
The PFS retrieve, delete, store and config push each resolved a swarm, picked its first member and sent, and each would now simply fail on a 421 that Network no longer recovers from. They go through _swarm_request instead, which re-aims for them. None of them needs to know which member answered -- only the poll records anything per-node -- but they all need the retrying, and having one implementation of it is the point. The subscribe is deliberately left sending directly: it is aimed at the member whose namespaces were just drained, and re-aiming it elsewhere would subscribe to a node other than the one Core is tracking. A failure there drops the subscription and returns to polling, which re-picks anyway. Configs needed naming as a friend: friendship does not reach a component through detail::CoreComponent.
test_swarm_retry drove Network's retry against a scripted router. That retry is Core's now, so the file tests Core instead -- through the poll, which is a real caller rather than a harness, so what is asserted is what a caller actually gets. MockNetwork grows two things to make that possible: a multi-member swarm, since it only ever returned one node, and an optional auto_reply so a test can script answers per member instead of firing every stored callback by hand. Writing them found a real defect. A member answering 421 was not recorded as spent, so re-resolving could pick the very same member and be rejected again, three times over, before the redirect limit stopped it. Network's version excluded the failed node explicitly and mine had dropped that: relying on the corrected swarm to no longer contain it only works when the rejection carried one, which an older storage server does not. The list now holds both kinds of spent member and is named for that rather than for one of them. The teardown case moves to its own file, since it is about Network rather than about swarms. It reached the loop thread via the retry; with that gone it uses get_swarm, which answers from the loop for the same reason.
Draining runs before the subscription exists, so a message stored between the last retrieve's snapshot and the subscription taking effect falls between the two: too late to be returned, too early to be pushed. Nothing else covers it -- the renewal sends no retrieve, and the probe asks about a namespace that is empty by design -- so it would sit unseen until the next reconnect drained again. Aimed at the member just subscribed with, rather than choosing afresh.
The probe and the renewal are the only traffic a subscribed client makes, so whether they are still happening is the first thing worth checking when pushes stop -- and the probe said nothing at all unless it failed. The renewal was already visible through _send_subscribe.
Reported from the CLI: a reliable SIGSEGV at exit, five in ten runs, in _drop_subscription called from the connection-lost hook. Core had no destructor, so members went in reverse declaration order -- and every ticker is declared after `_network` while `_loop` is declared first. So at teardown the tickers were released, then ~Network failed the requests its transport was holding, which fired our connection-lost hook, which marshalled onto the loop that was still alive, and stopped Tickers that had already been freed. Not a narrow race: a fixed ordering, which is why it reproduced. So detach the hooks and stop the timers before anything goes. ~Network already does the same for its own router and transport, and says why. Doing it in a destructor rather than by moving the member declarations around leaves the requirement written down instead of resting on where a field happens to sit. Unverified against the crash itself: reproducing it needs a live subscription, and Session Router cannot build paths in the environment I have -- no RC found for the pivot, zero path-builds, so nothing polls and nothing subscribes.
Every storage server endpoint answers in JSON except `monitor`, which is handled outside the RPC dispatch and replies with bt. So each subscribe, and each renewal after it, logged a parse warning while trying to read a clock offset and fork versions that a bt reply does not carry anyway. Recognised and skipped rather than parsed and complained about: a subscription renews on a timer, so this was a warning every fifteen minutes for the life of the process.
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.
This PR switches Session Router networking mode to subscribe to a swarm rather than periodically polling it, allowing faster delivery of messages -- in some cases arriving even before the sender is done sending it.
Live test using this mode in the (unreleased, experimental) session CLI testbed recorded at https://jagerman.com/2026-09-10%2019-58-25.mkv