Tell callers when the snode pool could not be refreshed

include/session/network/session_network.hpp

@@ -125,7 +125,7 @@ class Network : public std::enable_shared_from_this<Network> {
             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);
 };

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<SnodePool> {
   public:
     using network_fetcher_t = std::function<void(Request, network_response_callback_t)>;
+
+    // `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<void(bool refreshed)>;
     using fetcher_connectivity_check_t = std::function<bool()>;
 
     SnodePool(
@@ -74,7 +80,7 @@ class SnodePool : public std::enable_shared_from_this<SnodePool> {
     // Checks if the pool is empty or stale and triggers a refresh if needed
     virtual void refresh_if_needed(
             const std::vector<service_node>& in_use_nodes,
-            std::function<void()> on_refresh_complete = nullptr);
+            refresh_callback_t on_refresh_complete = nullptr);
 
     virtual void get_swarm(
             session::network::x25519_pubkey swarm_pubkey,
@@ -113,7 +119,7 @@ class SnodePool : public std::enable_shared_from_this<SnodePool> {
     int _snode_cache_refresh_failure_count = 0;
     std::vector<service_node> _refresh_candidate_nodes;
     std::vector<std::vector<std::byte>> _snode_refresh_results;
-    std::vector<std::function<void()>> _after_snode_cache_refresh;
+    std::vector<refresh_callback_t> _after_snode_cache_refresh;
 
     // Disk I/O functions
     void _load_from_disk();
@@ -145,6 +151,7 @@ class SnodePool : public std::enable_shared_from_this<SnodePool> {
             const bool use_direct_fetcher,
             const uint8_t total_requests);
     void _update_cache(std::string refresh_id, std::vector<service_node> nodes);
+    void _run_pending_refresh_callbacks(bool refreshed);
 };
 
 }  // namespace session::network

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())
@@ -1160,15 +1165,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.",

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(

src/network/session_network.cpp

@@ -409,8 +409,19 @@ void Network::get_random_nodes(
             std::vector<service_node> 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);
     });
@@ -836,7 +847,17 @@ void Network::_handle_421_retry(
             [this,
              req_to_retry = std::move(original_request),
              cb = std::move(final_callback),
-             failed_node = failed_node_copy] {
+             failed_node = failed_node_copy](bool refreshed) {
+                // Without a refresh 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 (!refreshed)
+                    return cb(
+                            false,
+                            false,
+                            ERROR_MISDIRECTED_REQUEST,
+                            {content_type_plain_text},
+                            "421 Misdirected Request, and the snode pool could not be refreshed");
+
                 auto swarm_pubkey = *req_to_retry.swarm_pubkey;
 
                 _snode_pool->get_swarm(
@@ -933,7 +954,20 @@ void Network::_resync_clock(
 
     // Refresh the snode pool if needed to ensure we have the most up-to-date cache
     std::vector<service_node> 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 =
@@ -1040,11 +1074,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);

src/network/snode_pool.cpp

@@ -339,6 +339,11 @@ void SnodePool::_refresh_snode_cache(std::optional<std::string> 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;
         }
 
@@ -416,6 +421,7 @@ void SnodePool::_refresh_snode_cache(std::optional<std::string> 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;
         }
 
@@ -570,6 +576,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;
         }
 
@@ -889,28 +896,35 @@ void SnodePool::_update_cache(std::string refresh_id, std::vector<service_node>
             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
@@ -1040,10 +1054,13 @@ void SnodePool::clear_node_strikes() {
 }
 
 void SnodePool::refresh_if_needed(
-        const std::vector<service_node>& in_use_nodes, std::function<void()> on_refresh_complete) {
+        const std::vector<service_node>& 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;
         }
 
@@ -1109,7 +1126,9 @@ 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);
     });
 }
 
@@ -1240,7 +1259,12 @@ void SnodePool::get_swarm(
 
             // Queue this entire function call to be re-run after the refresh.
             _after_snode_cache_refresh.push_back(
-                    [this, swarm_pubkey, ignore_strike_count, cb = std::move(cb)]() {
+                    [this, swarm_pubkey, ignore_strike_count, cb = std::move(cb)](bool refreshed) {
+                        // Re-deferring when the refresh never happened would just queue against the
+                        // next one that doesn't either; there is no swarm to give back
+                        if (!refreshed)
+                            return cb(swarm::INVALID_SWARM_ID, {});
+
                         this->get_swarm(swarm_pubkey, ignore_strike_count, std::move(cb));
                     });
 

tests/test_onion_request_router.cpp

@@ -154,7 +154,7 @@ namespace {
 
         void refresh_if_needed(
                 const std::vector<service_node>& /*in_use_nodes*/,
-                std::function<void()> /*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)
         }