From bb84f8dfa8accea8d8554186ae1b77a7e4711c54 Mon Sep 17 00:00:00 2001 From: dramforever Date: Tue, 7 Apr 2026 15:56:00 +0800 Subject: [PATCH 1/4] libstore: Introduce batching LocalStore::addTempRoots Introduce LocalStore::addTempRoots, which adds all temproots in a batch for LocalStore. Make this available from the base class Store by making a virtual method Store::addTempRoots, overriding it in LocalStore and RemoteStore (it is just a loop for each path because the benefit of having this be batched is unclear), and making the single-path Store::addTempRoot a wrapper around that. --- src/libstore/gc.cc | 34 +++++++++++++------ src/libstore/include/nix/store/gc-store.hh | 2 +- src/libstore/include/nix/store/local-store.hh | 2 +- .../include/nix/store/remote-store.hh | 2 +- src/libstore/include/nix/store/store-api.hh | 13 +++++-- src/libstore/remote-store.cc | 7 ++-- src/libstore/restricted-store.cc | 2 +- 7 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 4f5541c47b1b..a4814779f34d 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -75,7 +75,7 @@ void LocalStore::createTempRootsFile() } } -void LocalStore::addTempRoot(const StorePath & path) +void LocalStore::addTempRoots(const StorePathSet & paths) { if (config->readOnly) { debug( @@ -124,12 +124,14 @@ void LocalStore::addTempRoot(const StorePath & path) } try { - debug("sending GC root '%s'", printStorePath(path)); - writeFull(fdRootsSocket->get(), printStorePath(path) + "\n", false); - char c; - readFull(fdRootsSocket->get(), &c, 1); - assert(c == '1'); - debug("got ack for GC root '%s'", printStorePath(path)); + for (auto & path : paths) { + debug("sending GC root '%s'", printStorePath(path)); + writeFull(fdRootsSocket->get(), printStorePath(path) + "\n", false); + char c; + readFull(fdRootsSocket->get(), &c, 1); + assert(c == '1'); + debug("got ack for GC root '%s'", printStorePath(path)); + } } catch (SystemError & e) { /* The garbage collector may have exited, so we need to restart. */ @@ -146,10 +148,22 @@ void LocalStore::addTempRoot(const StorePath & path) } } - /* Record the store path in the temporary roots file so it will be + /* Record the store paths in the temporary roots file so they will be seen by a future run of the garbage collector. */ - auto s = printStorePath(path) + '\0'; - writeFull(_fdTempRoots.lock()->get(), s); + + std::string s; + + for (auto & path : paths) + s += printStorePath(path) + '\0'; + + { + auto fdTempRoots(_fdTempRoots.lock()); + + /* This might not be atomic, but that's fine. Writes go in-order, and if + we partially write a store path, findTempRoots() will just ignore it, + and we'll send it the new temproots below if it's still running. */ + writeFull(fdTempRoots->get(), s); + } } static std::string censored = "{censored}"; diff --git a/src/libstore/include/nix/store/gc-store.hh b/src/libstore/include/nix/store/gc-store.hh index d3d42470e2c9..661d0f28b03a 100644 --- a/src/libstore/include/nix/store/gc-store.hh +++ b/src/libstore/include/nix/store/gc-store.hh @@ -93,7 +93,7 @@ struct GCResults * * The notion of GC roots actually not part of this class. * - * - The base `Store` class has `Store::addTempRoot()` because for a store + * - The base `Store` class has `Store::addTempRoots()` because for a store * that doesn't support garbage collection at all, a temporary GC root is * safely implementable as no-op. * diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh index 1add5d2b62c1..a94914d7a72e 100644 --- a/src/libstore/include/nix/store/local-store.hh +++ b/src/libstore/include/nix/store/local-store.hh @@ -299,7 +299,7 @@ public: RepairFlag repair, std::shared_ptr provenance) override; - void addTempRoot(const StorePath & path) override; + void addTempRoots(const StorePathSet & paths) override; private: diff --git a/src/libstore/include/nix/store/remote-store.hh b/src/libstore/include/nix/store/remote-store.hh index 289d694aedd3..3333c7130779 100644 --- a/src/libstore/include/nix/store/remote-store.hh +++ b/src/libstore/include/nix/store/remote-store.hh @@ -121,7 +121,7 @@ struct RemoteStore : public virtual Store, void ensurePath(const StorePath & path) override; - void addTempRoot(const StorePath & path) override; + void addTempRoots(const StorePathSet & paths) override; Roots findRoots(bool censor) override; diff --git a/src/libstore/include/nix/store/store-api.hh b/src/libstore/include/nix/store/store-api.hh index e16e2c77d36d..87a0d0f37530 100644 --- a/src/libstore/include/nix/store/store-api.hh +++ b/src/libstore/include/nix/store/store-api.hh @@ -727,9 +727,18 @@ public: * Add a store path as a temporary root of the garbage collector. * The root disappears as soon as we exit. */ - virtual void addTempRoot(const StorePath & path) + void addTempRoot(const StorePath & path) { - debug("not creating temporary root, store doesn't support GC"); + addTempRoots({path}); + } + + /** + * Add multiple store paths as temporary roots of the garbage collector. + * The roots disappears as soon as we exit. + */ + virtual void addTempRoots(const StorePathSet & paths) + { + debug("not creating temporary roots, store doesn't support GC"); } /** diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 14665716c345..d438050a1655 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -693,10 +693,13 @@ void RemoteStore::ensurePath(const StorePath & path) readInt(conn->from); } -void RemoteStore::addTempRoot(const StorePath & path) +void RemoteStore::addTempRoots(const StorePathSet & paths) { auto conn(getConnection()); - conn->addTempRoot(*this, &conn.daemonException, path); + + /* Unclear if adding a new operation would be worthwhile */ + for (auto & path : paths) + conn->addTempRoot(*this, &conn.daemonException, path); } Roots RemoteStore::findRoots(bool censor) diff --git a/src/libstore/restricted-store.cc b/src/libstore/restricted-store.cc index 06bf5746d7fb..09fea5e4514a 100644 --- a/src/libstore/restricted-store.cc +++ b/src/libstore/restricted-store.cc @@ -124,7 +124,7 @@ struct RestrictedStore : public virtual IndirectRootStore, public virtual GcStor unsupported("buildDerivation"); } - void addTempRoot(const StorePath & path) override {} + void addTempRoots(const StorePathSet & paths) override {} void addIndirectRoot(const std::filesystem::path & path) override {} From 7beda55f66a4cef7780a46a93ca975313fcbbc74 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Jul 2026 11:15:56 +0200 Subject: [PATCH 2/4] libstore: Add an AddTempRoots daemon protocol operation Since 734a205d58a559a622ebc98256f4af4d968426b7, copyPaths() calls addTempRoot() on the remote store for every path in the closure. However, this is extremely show for large closures, especially over high-latency SSH connections. So now, we use a new daemon operation AddTempRoots to do all paths in a single call. Fixes https://github.com/DeterminateSystems/nix-src/issues/533. Assisted-by: Claude Fable 5 --- src/libstore/daemon.cc | 9 +++++++++ .../include/nix/store/worker-protocol.hh | 2 ++ src/libstore/remote-store.cc | 19 ++++++++++++++++--- src/libstore/store-api.cc | 3 +-- src/libstore/worker-protocol.cc | 1 + 5 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 50368a53cd67..31cc2c512719 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -695,6 +695,15 @@ static void performOp( break; } + case WorkerProto::Op::AddTempRoots: { + auto paths = WorkerProto::Serialise::read(*store, rconn); + logger->startWork(); + store->addTempRoots(paths); + logger->stopWork(); + conn.to << 1; + break; + } + case WorkerProto::Op::AddPermRoot: { if (!trusted) throw Error( diff --git a/src/libstore/include/nix/store/worker-protocol.hh b/src/libstore/include/nix/store/worker-protocol.hh index dd7b3a603750..05b6ecc76499 100644 --- a/src/libstore/include/nix/store/worker-protocol.hh +++ b/src/libstore/include/nix/store/worker-protocol.hh @@ -113,6 +113,7 @@ struct WorkerProto static constexpr std::string_view featureQueryActiveBuilds = "queryActiveBuilds"; static constexpr std::string_view featureProvenance = "provenance"; static constexpr std::string_view featureVersionedAddToStoreMultiple = "versionedAddToStoreMultiple"; + static constexpr std::string_view featureAddTempRoots = "addTempRoots"; /** * A unidirectional read connection, to be used by the read half of the @@ -238,6 +239,7 @@ enum struct WorkerProto::Op : uint64_t { BuildPathsWithResults = 46, AddPermRoot = 47, QueryActiveBuilds = 48, + AddTempRoots = 49, }; struct WorkerProto::ClientHandshakeInfo diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index d438050a1655..0532029f872c 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -695,11 +695,24 @@ void RemoteStore::ensurePath(const StorePath & path) void RemoteStore::addTempRoots(const StorePathSet & paths) { + if (paths.empty()) + return; + auto conn(getConnection()); - /* Unclear if adding a new operation would be worthwhile */ - for (auto & path : paths) - conn->addTempRoot(*this, &conn.daemonException, path); + if (conn->protoVersion.features.contains(WorkerProto::featureAddTempRoots)) { + conn->to << WorkerProto::Op::AddTempRoots; + WorkerProto::write(*this, *conn, paths); + conn.processStderr(); + readInt(conn->from); + } else { + /* Fallback for daemons that don't support the batched + operation. Note that this is very slow for large sets of + paths on high-latency links, due to a network round-trip per + path. */ + for (auto & path : paths) + conn->addTempRoot(*this, &conn.daemonException, path); + } } Roots RemoteStore::findRoots(bool censor) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index e53e741acb4b..d2effb6353cd 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -974,8 +974,7 @@ std::map copyPaths( CheckSigsFlag checkSigs, SubstituteFlag substitute) { - for (auto & path : storePaths) - dstStore.addTempRoot(path); + dstStore.addTempRoots(storePaths); auto valid = dstStore.queryValidPaths(storePaths, substitute); diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc index 3c7acbfb4784..8baab6442317 100644 --- a/src/libstore/worker-protocol.cc +++ b/src/libstore/worker-protocol.cc @@ -26,6 +26,7 @@ const WorkerProto::Version WorkerProto::latest = { std::string{WorkerProto::featureQueryActiveBuilds}, std::string{WorkerProto::featureProvenance}, std::string{WorkerProto::featureVersionedAddToStoreMultiple}, + std::string{WorkerProto::featureAddTempRoots}, }, }; From 2e0dd7c2eb517dfd11107143d9c4e5414484fa0b Mon Sep 17 00:00:00 2001 From: dramforever Date: Tue, 7 Apr 2026 13:46:25 +0800 Subject: [PATCH 3/4] nix-store: Use addTempRoots for --serve QueryValidPaths Use the new batched addTempRoots for both convenience and better performance for LocalStore. --- src/nix/nix-store/nix-store.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nix/nix-store/nix-store.cc b/src/nix/nix-store/nix-store.cc index 1cc4d7aa9c07..0c36d7d39c9c 100644 --- a/src/nix/nix-store/nix-store.cc +++ b/src/nix/nix-store/nix-store.cc @@ -953,8 +953,7 @@ static void opServe(Strings opFlags, Strings opArgs) bool substitute = readInt(in); auto paths = ServeProto::Serialise::read(*store, rconn); if (lock && writeAllowed) - for (auto & path : paths) - store->addTempRoot(path); + store->addTempRoots(paths); if (substitute && writeAllowed) { store->substitutePaths(paths); From 334f160af34bdae2fa9439a3c2ddfd44a93ab9f6 Mon Sep 17 00:00:00 2001 From: dramforever Date: Tue, 7 Apr 2026 13:44:39 +0800 Subject: [PATCH 4/4] libstore: Use addTempRoots in DerivationGoal::haveDerivation Use the new batched addTempRoots for both convenience and better performance for LocalStore. --- src/libstore/build/derivation-goal.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index f0e113eb0959..ba78a88343c9 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -78,9 +78,12 @@ Goal::Co DerivationGoal::haveDerivation(bool storeDerivation) if (!drv->type().hasKnownOutputPaths()) experimentalFeatureSettings.require(Xp::CaDerivations); + StorePathSet outputPaths; for (auto & i : drv->outputsAndOptPaths(worker.store)) if (i.second.second) - worker.store.addTempRoot(*i.second.second); + outputPaths.insert(*i.second.second); + + worker.store.addTempRoots(outputPaths); /* We don't yet have any safe way to cache an impure derivation at this step. */