Conversation
After lockFlake() creates flake.lock in a PosixSourceAccessor, it needs to be able to invalidate the lstat cache.
Even with lazy trees, this forced a NAR hash computation for path flakes that we don't want.
📝 WalkthroughWalkthroughThis PR introduces a cache invalidation mechanism across source accessor implementations and updates path fetching logic to avoid store copies. Test infrastructure consolidates git repository setup helpers, and flake locking behavior is refined with explicit cache invalidation after lock file writes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
edolstra
added
the
flake-regression-test
edolstra
force-pushed
the
lazy-path-accessor
branch
from
a383c0d to
7b3a6d9
Compare
Note: the path fetcher no longer computes lastModified, because that's not lazy.
edolstra
force-pushed
the
lazy-path-accessor
branch
from
7b3a6d9 to
9503ac5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libutil/posix-source-accessor.cc (1)
90-107: Verify Boost concurrent_flat_mapclear()blocking behavior and consider non-blocking eviction.The code's keying for invalidation (line 113) matches the insertion key (line 97), so invalidation is correct. However, the eviction strategy on lines 104–105 is problematic:
clear()is a global blocking operation that prevents all concurrent reads/writes while executing. Under concurrent load, this can cause severe contention. The check-and-clear is also non-atomic—another thread may add entries between thesize()check and theclear()call.Consider using a non-blocking eviction strategy, such as
erase_if, or document that this strategy is acceptable for the expected concurrency profile. Additionally, the magic number16384should be a named constant for clarity.
🧹 Nitpick comments (3)
tests/functional/fetchGitRefs.sh (1)
11-14: Repo setup refactor is OK; consider standardizing${repo}-tmpvs$repo.tmpcleanup.
Right now cleanup removes${repo}-tmp, whilecreateGitReporemoves$repoand$repo.tmp; if${repo}-tmpis no longer used, it’s just extra noise.tests/functional/fetchGit.sh (1)
15-18: Good helper adoption; optional: add a non-empty-path guard sincecreateGitRepois destructive (rm -rf).
Especially for variables likerepo, a quick[[ -n "${repo:-}" ]]before calling keeps failures loud and avoids accidentalrm -rf .tmp-style surprises.Also applies to: 214-215, 275-275, 287-287, 315-315
src/libfetchers/fetch-to-store.cc (1)
61-61: Optimize: avoid redundantto_string()calls.The condition calls
path.to_string()twice. Store the result in a variable to improve efficiency.♻️ Proposed optimization
+ auto pathStr = path.to_string(); - if (barf && !filter && !(path.to_string().starts_with("/") || path.to_string().starts_with("«path:/"))) + if (barf && !filter && !(pathStr.starts_with("/") || pathStr.starts_with("«path:/"))) throw Error("source path '%s' is uncacheable (filter=%d)", path, (bool) filter);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/libfetchers/fetch-to-store.ccsrc/libfetchers/filtering-source-accessor.ccsrc/libfetchers/include/nix/fetchers/filtering-source-accessor.hhsrc/libfetchers/path.ccsrc/libflake/flake.ccsrc/libutil/include/nix/util/posix-source-accessor.hhsrc/libutil/include/nix/util/source-accessor.hhsrc/libutil/include/nix/util/source-path.hhsrc/libutil/mounted-source-accessor.ccsrc/libutil/posix-source-accessor.ccsrc/libutil/union-source-accessor.cctests/functional/common/functions.shtests/functional/fetchGit.shtests/functional/fetchGitRefs.shtests/functional/fetchGitShallow.shtests/functional/fetchGitSubmodules.shtests/functional/fetchGitVerification.shtests/functional/fetchPath.shtests/functional/flakes/common.shtests/functional/flakes/flakes.shtests/functional/flakes/relative-paths.shtests/functional/git-hashing/simple-common.shtests/functional/git-hashing/simple-sha1.shtests/functional/git-hashing/simple-sha256.shtests/functional/nix-profile.sh
💤 Files with no reviewable changes (3)
- tests/functional/git-hashing/simple-common.sh
- tests/functional/flakes/flakes.sh
- tests/functional/flakes/common.sh
🧰 Additional context used
🧬 Code graph analysis (16)
tests/functional/git-hashing/simple-sha256.sh (1)
tests/functional/common/functions.sh (1)
createGitRepo(374-383)
tests/functional/nix-profile.sh (1)
tests/functional/common/functions.sh (2)
requireGit(166-168)createGitRepo(374-383)
tests/functional/fetchGitRefs.sh (1)
tests/functional/common/functions.sh (1)
createGitRepo(374-383)
src/libutil/include/nix/util/source-accessor.hh (1)
src/libutil/mounted-source-accessor.cc (16)
path(24-28)path(24-24)path(30-34)path(30-30)path(36-40)path(36-36)path(42-46)path(42-42)path(48-52)path(48-48)path(54-58)path(54-54)path(60-74)path(60-60)path(76-80)path(76-76)
tests/functional/fetchGitVerification.sh (1)
tests/functional/common/functions.sh (1)
createGitRepo(374-383)
src/libutil/posix-source-accessor.cc (2)
src/libfetchers/filtering-source-accessor.cc (4)
invalidateCache(71-74)invalidateCache(71-71)path(99-102)path(99-99)src/libutil/include/nix/util/posix-source-accessor.hh (11)
path(34-34)path(36-36)path(38-38)path(40-40)path(42-42)path(44-44)path(74-74)path(81-81)path(88-88)path(90-90)path(92-92)
tests/functional/fetchGit.sh (1)
tests/functional/common/functions.sh (2)
createGitRepo(374-383)initGitRepo(364-372)
src/libfetchers/path.cc (1)
src/libfetchers/fetch-to-store.cc (2)
makeSourcePathToHashCacheKey(8-13)makeSourcePathToHashCacheKey(9-9)
tests/functional/git-hashing/simple-sha1.sh (1)
tests/functional/common/functions.sh (1)
createGitRepo(374-383)
tests/functional/flakes/relative-paths.sh (1)
tests/functional/common/functions.sh (1)
initGitRepo(364-372)
tests/functional/fetchGitShallow.sh (1)
tests/functional/common/functions.sh (1)
createGitRepo(374-383)
tests/functional/fetchGitSubmodules.sh (1)
tests/functional/common/functions.sh (1)
createGitRepo(374-383)
src/libutil/union-source-accessor.cc (1)
src/libutil/include/nix/util/source-path.hh (1)
accessor(117-120)
src/libflake/flake.cc (1)
src/libflake/include/nix/flake/flake.hh (1)
getFlake(125-126)
src/libutil/include/nix/util/posix-source-accessor.hh (2)
src/libfetchers/filtering-source-accessor.cc (2)
path(99-102)path(99-99)src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh (14)
path(35-35)path(37-37)path(39-39)path(41-41)path(43-43)path(45-45)path(47-47)path(49-49)path(51-51)path(53-53)path(55-55)path(61-61)path(66-66)path(98-98)
src/libfetchers/filtering-source-accessor.cc (3)
src/libutil/posix-source-accessor.cc (2)
invalidateCache(111-114)invalidateCache(111-111)src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh (16)
path(35-35)path(37-37)path(39-39)path(41-41)path(43-43)path(45-45)path(47-47)path(49-49)path(51-51)path(53-53)path(55-55)path(61-61)path(66-66)path(98-98)path(100-100)prefix(78-78)src/libutil/include/nix/util/canon-path.hh (1)
prefix(280-280)
🔇 Additional comments (23)
tests/functional/common/functions.sh (1)
364-383: LGTM! Well-designed test helpers for Git repository setup.The
initGitRepoandcreateGitRepofunctions provide clean, reusable abstractions for test repository initialization. The intentional word splitting ofextraArgsis properly documented and appropriate for passing flags like--object-format=sha256.tests/functional/flakes/relative-paths.sh (2)
48-48: LGTM! Clean refactoring to use centralized helper.Replacing inline Git initialization with
initGitRepoimproves consistency across the test suite.
138-138: LGTM! Consistent use of the new helper.tests/functional/nix-profile.sh (3)
7-7: LGTM! Proper prerequisite check.Adding
requireGitensures the test environment has Git available before proceeding.
17-17: Verify the behavioral change from path-based to Git-based flakes.The test now initializes flake directories as Git repositories (using
createGitRepo) and performs commits, rather than using plain path-based flakes. This changes what's being tested.Given the PR's objective to make path inputs lazy, confirm this change is intentional—either to validate that Git-based flakes continue working correctly, or because path-based flake behavior has changed in a way that requires test adjustments.
Also applies to: 55-56
232-232: Verify the URL scheme change in conflict resolution messages.The expected conflict messages now reference
git+file://URLs instead ofpath:URLs. This should align with the switch to Git-based flakes in the test setup.Confirm this is the expected behavior: should the profile conflict messages recommend
git+file://URLs when the original installation used a Git repository?Also applies to: 236-236
tests/functional/git-hashing/simple-sha256.sh (1)
9-9: LGTM! Consistent refactoring with proper hash algorithm configuration.The switch to
createGitRepowith--object-format=$hashAlgoaligns with the standardized test helper usage across the suite.tests/functional/fetchGitVerification.sh (1)
24-24: LGTM! Clean consolidation of setup logic.Replacing inline Git initialization with
createGitRepoimproves consistency while preserving the test-specificgpg.formatconfiguration on Line 25.tests/functional/fetchGitShallow.sh (1)
9-9: Nice simplification viacreateGitRepo(keeps test intent, reduces duplication).tests/functional/fetchGitSubmodules.sh (1)
31-35: Consistent use ofcreateGitRepoimproves test readability and avoids repeated git-config boilerplate.Also applies to: 196-209
tests/functional/git-hashing/simple-sha1.sh (1)
7-7: HelpercreateGitRepois properly in scope for this test.
The sourcing chain is complete:simple-sha1.sh→simple-common.sh→common.sh(git-hashing) →../common.sh(functional) →common/functions.sh, which definescreateGitRepo()at line 374. No issues.src/libutil/include/nix/util/source-accessor.hh (1)
212-216: LGTM! Clean cache invalidation API.The new
invalidateCachemethod provides a clean hook for derived accessors to clear per-path caches. The virtual method with a no-op default is appropriate since not all accessors maintain caches.tests/functional/fetchPath.sh (1)
7-8: LGTM! Test correctly validates lazy path behavior.The updated test correctly verifies that the path fetcher no longer provides
lastModifiedby default, aligning with the PR's lazy-path objective. The subsequent test confirms that explicitlastModifiedoverrides still work as expected.src/libutil/include/nix/util/posix-source-accessor.hh (1)
81-81: LGTM! Correct override declaration.The
invalidateCacheoverride is properly declared and matches the base class signature.src/libfetchers/path.cc (1)
154-164: The cache key consistency is correct and requires no changes.The cache pre-creation logic in path.cc (lines 154-164) and the cache lookup in fetch-to-store.cc use matching keys:
- Both use the fingerprint format:
"path:{narHash}"- Both use the method:
ContentAddressMethod::Raw::NixArchive- Both use the path:
"/"(sinceSourcePath(accessor)defaults toCanonPath::root, which resolves to"/"viasubpath.abs())The accessor fingerprint is correctly set by path.cc before the accessor is returned, and it is properly retrieved during cache lookup in fetch-to-store.cc.
src/libflake/flake.cc (2)
439-439: LGTM: Implements lazy evaluation for the initial flake fetch.The change from the default
requireLockable = trueto explicitfalsedefers lockability requirements, avoiding eager NAR hash computation. The flake is properly re-fetched withlockFlags.requireLockableon line 930 after the lock file is written, ensuring correctness while optimizing the initial fetch.
922-923: LGTM: Correctly invalidates cache after modifying the lock file.After writing the lock file via
putFile, the accessor's cached lstat information becomes stale. This invalidation ensures subsequent reads reflect the updated file state.src/libutil/union-source-accessor.cc (1)
88-92: LGTM: Correctly propagates cache invalidation to all contained accessors.The implementation properly forwards the invalidation call to each accessor in the union, ensuring consistent cache state across all sources.
src/libutil/include/nix/util/source-path.hh (1)
117-120: LGTM: Clean delegation to accessor's cache invalidation.The method provides a convenient cache invalidation API through
SourcePath, correctly delegating to the underlying accessor.src/libfetchers/filtering-source-accessor.cc (1)
71-74: LGTM: Correctly forwards cache invalidation with path transformation.The implementation properly adjusts the path by prepending the prefix before delegating to the next accessor, maintaining consistency with other
FilteringSourceAccessormethods.src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh (1)
55-55: LGTM: Correct method declaration.The override declaration is properly formed and matches the implementation.
src/libutil/mounted-source-accessor.cc (1)
95-107: Good delegation; verifyresolve()invariants for invalidateCache callers (assert safety).Line 105 resolves then forwards to the underlying accessor, consistent with the other mounted operations. Just confirm no caller can pass a
CanonPaththat isn’t covered by mounts (otherwise Line 70 assert inresolve()can fire).src/libutil/posix-source-accessor.cc (1)
111-114: Implementation is correct; parent directory invalidation not needed in this workflow.The
invalidateCache()call at line 113 properly invalidates only the lock file's cached lstat entry after it's written. Parent directory invalidation is unnecessary because:
- The cache stores only individual path stat info (keyed by absolute path), not directory listings
- Parent directory metadata is unaffected by writing/modifying a file within it
- In the lockFlake() workflow, after writing flake.lock via
putFile(), the subsequentgetFlake()call re-reads the file contentNo changes needed.
2 participants
Motivation
This is a missing bit from the original lazy-trees (NixOS#6530). Path inputs were still copied to the Nix store, which is slow and wasteful.
Context
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.