From 289fab6a680efaf3ae7410f94dfded3be64b0fd1 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Thu, 16 Jul 2026 21:40:43 +0200 Subject: [PATCH] libstore: remove ssh{,-ng}:// connection pooling it's used very rarely, on ssh it's pretty much broken, and on ssh-ng it's standing squarely in the way of the rpc transition. we have not found significant use of this feature in public configs (with all of *two* repositories using this store parameter), so why even keep it? Change-Id: Ifcafdf794d815001a1a55a771e5823010e5b174b --- doc/manual/rl-next/no-max-connections.md | 11 +++++ lix/libstore/legacy-ssh-store.cc | 50 +++++++++++------------ lix/libstore/machines.cc | 4 -- lix/libstore/remote-store-connection.hh | 37 +++++++++++------ lix/libstore/remote-store.cc | 52 ++++++++---------------- lix/libstore/remote-store.hh | 13 +----- 6 files changed, 76 insertions(+), 91 deletions(-) create mode 100644 doc/manual/rl-next/no-max-connections.md diff --git a/doc/manual/rl-next/no-max-connections.md b/doc/manual/rl-next/no-max-connections.md new file mode 100644 index 000000000..a33a68a5e --- /dev/null +++ b/doc/manual/rl-next/no-max-connections.md @@ -0,0 +1,11 @@ +--- +synopsis: "Remove `max-connections` store parameters for `ssh://` and `ssh-ng://` stores" +cls: [] +category: Miscellany +credits: [horrors] +--- + +The `max-connections` parameter was undocumented, untested, and (in the case of `ssh`) even ignored +entirely for remote builds. During a survey of public nixos configurations we have found *two* uses +of `max-connections` for `ssh-ng`, and none at all for `ssh`. Since it is so rarely used but brings +significant internal complexity that hinders improvements we have decided to remove these features. diff --git a/lix/libstore/legacy-ssh-store.cc b/lix/libstore/legacy-ssh-store.cc index 530e115cd..f0f4eb5b1 100644 --- a/lix/libstore/legacy-ssh-store.cc +++ b/lix/libstore/legacy-ssh-store.cc @@ -1,6 +1,7 @@ #include "lix/libstore/legacy-ssh-store.hh" #include "libutil/error.hh" #include "libutil/logging.hh" +#include "libutil/sync.hh" #include "lix/libutil/archive.hh" #include "lix/libutil/async-io.hh" #include "lix/libutil/async.hh" @@ -86,9 +87,6 @@ struct LegacySSHStoreConfig : CommonSSHStoreConfig const Setting remoteProgram{this, "nix-store", "remote-program", "Path to the `nix-store` executable on the remote machine."}; - const Setting maxConnections{this, 1, "max-connections", - "Maximum number of concurrent SSH connections."}; - const std::string name() override { return "SSH Store"; } std::string doc() override @@ -256,7 +254,7 @@ struct LegacySSHStore final : public Store std::string host; - ref> connections; + Sync connection; SSH ssh; @@ -266,20 +264,18 @@ struct LegacySSHStore final : public Store : Store(config) , config_(std::move(config)) , host(host) - , connections( - make_ref>( - std::max(1, (int) config_.maxConnections), - [this]() { return openConnection(); }, - [](const ref & r) { return r->good; } - ) - ) , ssh(host, config_.port, config_.sshKey, config_.sshPublicHostKey, config_.compress) { } - kj::Promise>> openConnection() + kj::Promise> getConnection() try { - auto conn = make_ref(); + auto conn = co_await connection.lock(); + if (conn->good && conn->sshConn) { + co_return conn; + } + + *conn = {}; conn->sshConn = ssh.startCommand( fmt("%s --serve --write", config_.remoteProgram) + (config_.remoteStore.get() == "" ? "" : " --store " + shellEscape(config_.remoteStore.get())) @@ -314,14 +310,14 @@ struct LegacySSHStore final : public Store conn->act.emplace_back(logger->startActivity(lvlDebug, actUnknown, "remote store " + getUri())); conn->logHandlerPromise = conn->logHandler(host); - return {conn}; + co_return conn; } catch (Error & e) { std::string msg = chomp(drainFD(conn->sshConn->stderrPipe.get(), false)); throw Error("cannot connect to %s: %s (%s)", getUri(), e.msg(), msg); } } catch (...) { - return {result::current_exception()}; - }; + co_return result::current_exception(); + } std::string getUri() override { @@ -331,7 +327,7 @@ struct LegacySSHStore final : public Store kj::Promise>> queryPathInfoUncached(const StorePath & path, const Activity * context) override try { - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); debug("querying remote host '%s' for info on '%s'", host, printStorePath(path)); @@ -359,7 +355,7 @@ struct LegacySSHStore final : public Store try { debug("adding path '%s' to remote host '%s'", printStorePath(info.path), host); - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); unsigned result; if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 5) { @@ -403,16 +399,16 @@ struct LegacySSHStore final : public Store kj::Promise>> narFromPath(const StorePath & path, const Activity * context) override try { - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); struct NarStream : AsyncInputStream { - Pool::Handle conn; + Sync::Lock conn; AsyncFdIoStream stream{AsyncFdIoStream::shared_fd{}, conn->sshConn->socket.get()}; AsyncBufferedInputStream buffered{stream, conn->fromBuf}; box_ptr copier{copyNAR(buffered)}; - NarStream(Pool::Handle conn) : conn(std::move(conn)) {} + NarStream(Sync::Lock conn) : conn(std::move(conn)) {} kj::Promise>> read(void * buffer, size_t size) override { @@ -476,7 +472,7 @@ public: const StorePath & drvPath, const BasicDerivation & drv, BuildMode buildMode ) override try { - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); // this is a duplicate of DerivationGoal::buildDescription because ugh. // getting that information into here where needed is nigh *impossible* @@ -509,7 +505,7 @@ public: if (evalStore && evalStore.get() != this) throw Error("building on an SSH store is incompatible with '--eval-store'"); - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); Strings ss; for (auto & p : drvPaths) { @@ -568,7 +564,7 @@ public: co_return result::success(); } - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); out.merge(TRY_AWAIT(conn->sendCommand( ServeProto::Command::QueryClosure, includeOutputs, ServeProto::write(*conn, paths) @@ -582,7 +578,7 @@ public: kj::Promise> queryValidPaths(const StorePathSet & paths, SubstituteFlag maybeSubstitute = NoSubstitute) override try { - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); co_return TRY_AWAIT(conn->sendCommand( ServeProto::Command::QueryValidPaths, @@ -596,7 +592,7 @@ public: kj::Promise> init() override try { - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); co_return result::success(); } catch (...) { co_return result::current_exception(); @@ -604,7 +600,7 @@ public: kj::Promise> getProtocol() override try { - auto conn(TRY_AWAIT(connections->get())); + auto conn(TRY_AWAIT(getConnection())); co_return conn->remoteVersion; } catch (...) { co_return result::current_exception(); diff --git a/lix/libstore/machines.cc b/lix/libstore/machines.cc index cb04b6c34..40db786b9 100644 --- a/lix/libstore/machines.cc +++ b/lix/libstore/machines.cc @@ -37,10 +37,6 @@ bool Machine::mandatoryMet(const std::set & features) const kj::Promise>> Machine::openStore() const try { StoreConfig::Params storeParams; - if (storeUri.starts_with("ssh://")) { - // Remote builds become flakey, when having more than one ssh connection - storeParams["max-connections"] = "1"; - } if (storeUri.starts_with("ssh://") || storeUri.starts_with("ssh-ng://")) { if (sshKey != "") diff --git a/lix/libstore/remote-store-connection.hh b/lix/libstore/remote-store-connection.hh index 6f9a4a218..2a18ae1d9 100644 --- a/lix/libstore/remote-store-connection.hh +++ b/lix/libstore/remote-store-connection.hh @@ -121,12 +121,21 @@ struct RemoteStore::Connection */ struct RemoteStore::ConnectionHandle { - Pool::Handle handle; + Sync, AsyncMutex>::Lock handle; - ConnectionHandle(Pool::Handle && handle) : handle(std::move(handle)) {} + ConnectionHandle(Sync, AsyncMutex>::Lock && handle) + : handle(std::move(handle)) + { + } - RemoteStore::Connection & operator * () { return *handle; } - RemoteStore::Connection * operator -> () { return &*handle; } + RemoteStore::Connection & operator*() + { + return **handle; + } + RemoteStore::Connection * operator->() + { + return &**handle; + } kj::Promise> processStderr(AsyncFdIoStream & stream); @@ -148,11 +157,11 @@ struct RemoteStore::ConnectionHandle // the stack, but that's sufficiently suspect to warrant being as careful. auto invalidateOnCancel = kj::defer([&] { if (std::uncaught_exceptions() == 0) { - handle.markBad(); + *handle = nullptr; } }); - AsyncFdIoStream stream{AsyncFdIoStream::shared_fd{}, handle->getFD()}; + AsyncFdIoStream stream{AsyncFdIoStream::shared_fd{}, (*handle)->getFD()}; // if the last argument can be serialized normally we will serialize *all* // arguments at once and hand off to the remote. if the last argument does @@ -165,7 +174,7 @@ struct RemoteStore::ConnectionHandle ((msg << std::forward(args)), ...); TRY_AWAIT(stream.writeFull(msg.s.data(), msg.s.size())); } catch (...) { - handle.markBad(); + *handle = nullptr; throw; } LIX_TRY_AWAIT(processStderr(stream)); @@ -183,7 +192,7 @@ struct RemoteStore::ConnectionHandle }(ImmediateArgsIdxs{}); TRY_AWAIT(stream.writeFull(msg.s.data(), msg.s.size())); } catch (...) { - handle.markBad(); + *handle = nullptr; throw; } @@ -195,14 +204,16 @@ struct RemoteStore::ConnectionHandle co_return result::success(); } else { try { - AsyncBufferedInputStream from{stream, handle->fromBuf}; - auto result = LIX_TRY_AWAIT(WorkerProto::readAsync( - from, *handle->store, handle->daemonVersion, WorkerProto::Serialise::read - )); + AsyncBufferedInputStream from{stream, (*handle)->fromBuf}; + auto result = LIX_TRY_AWAIT( + WorkerProto::readAsync( + from, *(*handle)->store, (*handle)->daemonVersion, WorkerProto::Serialise::read + ) + ); invalidateOnCancel.cancel(); co_return result; } catch (...) { - handle.markBad(); + *handle = nullptr; throw; } } diff --git a/lix/libstore/remote-store.cc b/lix/libstore/remote-store.cc index 101a895a0..20d5d6e95 100644 --- a/lix/libstore/remote-store.cc +++ b/lix/libstore/remote-store.cc @@ -1,4 +1,5 @@ #include "libutil/fmt.hh" +#include "libutil/ref.hh" #include "lix/libutil/async-collect.hh" #include "lix/libutil/async-io.hh" #include "lix/libutil/async.hh" @@ -39,32 +40,23 @@ namespace nix { /* TODO: Separate these store impls into different files, give them better names */ -RemoteStore::RemoteStore(const RemoteStoreConfig & config) - : Store(config) - , connections(make_ref>( - std::max(1, (int) config.maxConnections), - [this]() { return openAndInitConnection(); }, - [this](const ref & r) { - return std::chrono::duration_cast( - std::chrono::steady_clock::now() - r->startTime - ) - .count() - < this->config().maxConnectionAge; - } - )) -{ -} +RemoteStore::RemoteStore(const RemoteStoreConfig & config) : Store(config) {} kj::Promise>> RemoteStore::openConnectionForDaemonForwarding() { return openConnection(); } -kj::Promise>> RemoteStore::openAndInitConnection() +kj::Promise> RemoteStore::getConnection() try { - auto conn = TRY_AWAIT(openConnection()); - TRY_AWAIT(initConnection(*conn)); - co_return conn; + auto conn = co_await connection.lock(); + if (*conn) { + co_return {std::move(conn)}; + } + + *conn = TRY_AWAIT(openConnection()); + TRY_AWAIT(initConnection(**conn)); + co_return {std::move(conn)}; } catch (...) { co_return result::current_exception(); } @@ -184,26 +176,19 @@ try { kj::Promise> RemoteStore::ConnectionHandle::processStderr(AsyncFdIoStream & stream) try { - auto ex = TRY_AWAIT(handle->processStderr(stream)); + auto ex = TRY_AWAIT((*handle)->processStderr(stream)); if (ex.e) { co_return result::failure(ex.e); } co_return result::success(); } catch (...) { - handle.markBad(); - co_return result::current_exception(); -} - -kj::Promise> RemoteStore::getConnection() -try { - co_return ConnectionHandle(TRY_AWAIT(connections->get())); -} catch (...) { + *handle = nullptr; co_return result::current_exception(); } kj::Promise> RemoteStore::setOptions() try { - TRY_AWAIT(setOptions(*(TRY_AWAIT(getConnection()).handle))); + TRY_AWAIT(setOptions(*(TRY_AWAIT(getConnection())))); co_return result::success(); } catch (...) { co_return result::current_exception(); @@ -373,10 +358,6 @@ kj::Promise>> RemoteStore::addCAToStore( try { auto conn(TRY_AWAIT(getConnection())); - // The dump source may invoke the store, so we need to make some room. - connections->incCapacity(); - Finally cleanup([&]() { connections->decCapacity(); }); - co_return make_ref(TRY_AWAIT(conn.sendCommand( WorkerProto::Op::AddToStore, name, @@ -708,8 +689,7 @@ try { kj::Promise> RemoteStore::getProtocol() try { - auto conn(TRY_AWAIT(connections->get())); - co_return conn->daemonVersion; + co_return TRY_AWAIT(getConnection())->daemonVersion; } catch (...) { co_return result::current_exception(); } @@ -856,7 +836,7 @@ try { TRY_AWAIT(framed.finish()); co_return result::success(); } catch (...) { - handle.markBad(); + *handle = nullptr; co_return result::current_exception(); } }; diff --git a/lix/libstore/remote-store.hh b/lix/libstore/remote-store.hh index b66cf3138..ef5174746 100644 --- a/lix/libstore/remote-store.hh +++ b/lix/libstore/remote-store.hh @@ -5,6 +5,7 @@ #include #include +#include "lix/libutil/sync.hh" #include "lix/libstore/store-api.hh" #include "lix/libstore/gc-store.hh" #include "lix/libstore/log-store.hh" @@ -24,14 +25,6 @@ template class Pool; struct RemoteStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; - - const Setting maxConnections{this, 1, "max-connections", - "Maximum number of concurrent connections to the Nix daemon."}; - - const Setting maxConnectionAge{this, - std::numeric_limits::max(), - "max-connection-age", - "Maximum age of a connection before it is closed."}; }; /** @@ -184,11 +177,9 @@ protected: virtual kj::Promise>> openConnection() = 0; - kj::Promise>> openAndInitConnection(); - virtual kj::Promise> initConnection(Connection & conn); - ref> connections; + Sync, AsyncMutex> connection; virtual kj::Promise> setOptions(Connection & conn);