Repository navigation
Fix very slow remote temp root registration in copyPaths() - #538
Merged
Merged
Conversation
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.
Since 734a205, 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 #533. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTemporary GC root registration now uses batched ChangesBatch addTempRoots API
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Jul 7, 2026
Use the new batched addTempRoots for both convenience and better performance for LocalStore.
Use the new batched addTempRoots for both convenience and better performance for LocalStore.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libstore/include/nix/store/store-api.hh (1)
726-742: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep
addTempRootvirtual for downstream stores.
addTempRoot(const StorePath&)is now a non-virtual wrapper, so any out-of-treeStoresubclass that still overrides the old signature will compile but stop participating in GC when called throughStore&/Store*. In-tree implementations already useaddTempRoots, but this is still a breaking API change for public consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libstore/include/nix/store/store-api.hh` around lines 726 - 742, Keep addTempRoot(const StorePath&) virtual in Store so downstream Store subclasses overriding the single-path API still participate in garbage collection when called through Store& or Store*. Update the Store interface around addTempRoot and addTempRoots so addTempRoot remains a virtual entry point (or otherwise forwards in a way that preserves overrides), and keep the existing addTempRoots behavior for bulk roots.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/libstore/include/nix/store/store-api.hh`:
- Around line 726-742: Keep addTempRoot(const StorePath&) virtual in Store so
downstream Store subclasses overriding the single-path API still participate in
garbage collection when called through Store& or Store*. Update the Store
interface around addTempRoot and addTempRoots so addTempRoot remains a virtual
entry point (or otherwise forwards in a way that preserves overrides), and keep
the existing addTempRoots behavior for bulk roots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 863ba6be-aaa4-41e1-8777-f23e47e2a3b9
📒 Files selected for processing (11)
src/libstore/daemon.ccsrc/libstore/gc.ccsrc/libstore/include/nix/store/gc-store.hhsrc/libstore/include/nix/store/local-store.hhsrc/libstore/include/nix/store/remote-store.hhsrc/libstore/include/nix/store/store-api.hhsrc/libstore/include/nix/store/worker-protocol.hhsrc/libstore/remote-store.ccsrc/libstore/restricted-store.ccsrc/libstore/store-api.ccsrc/libstore/worker-protocol.cc
cole-h
approved these changes
Jul 7, 2026
This branch was previously deployed
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
3 participants
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.
Motivation
Since 734a205,
copyPaths()callsaddTempRoot()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 operationAddTempRootsto do all paths in a single call.Fixes #533. This borrows the
addTempRoots()interface added in NixOS#15719.Context
Summary by CodeRabbit
New Features
Bug Fixes