Only try to substitute input if fetching from its original location fails #202
Conversation
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
WalkthroughRefactors Input::getAccessorUnchecked to separate store-path reuse and substitution from fetch logic, adding makeStoreAccessor and staged fetch/substitute flow; adds a new flake substitution functional test and registers it; adds a clarifying comment in disableNet() about local substituters. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Input
participant Store as "Nix Store"
participant Scheme as "Fetch Scheme"
Caller->>Input: getAccessorUnchecked()
alt precomputed storePath + isFinal + narHash
Input->>Store: ensurePath(storePath)
Store-->>Input: ok / error
alt ok
Input->>Input: makeStoreAccessor(storePath) (set fingerprint, display)
Input-->>Caller: SubstitutedSourceAccessor
else error
Input->>Input: proceed to fetch
end
else
Input->>Input: proceed to fetch
end
rect rgba(200,230,255,0.25)
Note right of Input: Attempt fetch via scheme
Input->>Scheme: getAccessor(input)
Scheme-->>Input: accessor or throw
end
alt fetch succeeded
Input-->>Caller: accessor (fingerprint propagated)
else fetch failed
alt storePath exists
Input->>Store: ensurePath(storePath)
alt substitution succeeded
Input->>Input: makeStoreAccessor(storePath)
Input-->>Caller: SubstitutedSourceAccessor (warning logged)
else substitution failed
Input-->>Caller: rethrow original fetch error (substitution error swallowed)
end
else
Input-->>Caller: rethrow original fetch error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/libfetchers/fetchers.cc (1)
355-383: Fallback substitution flow looks good; minor polish
- Consider also caching the fingerprint on the returned
Inputwhen you synthesize it for the accessor to keep symmetry with the else-branch. Low risk, but keeps invariants tighter.- The success log on substitution might be less noisy as info/notice rather than warn, given this is an expected fallback.
- if (!accessor->fingerprint) - accessor->fingerprint = result.getFingerprint(store); + if (!accessor->fingerprint) { + accessor->fingerprint = result.getFingerprint(store); + result.cachedFingerprint = accessor->fingerprint; + } else result.cachedFingerprint = accessor->fingerprint;tests/functional/flakes/substitution.sh (2)
7-9: Make lazy-trees skip check robustThe current string compare may be brittle. Query JSON and test explicitly.
-if [[ $(nix config show lazy-trees) = true ]]; then +if nix config show --json | jq -e '.["lazy-trees"] == true' >/dev/null; then exit 0 fi
25-28: Pre-create local cache dir to avoid tool/version differencesSome versions expect the directory to exist. Harmless if it already does.
cache="file://$TEST_ROOT/binary-cache" -nix copy --to "$cache" "$path1" +mkdir -p "$TEST_ROOT/binary-cache" +nix copy --to "$cache" "$path1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libfetchers/fetchers.cc(1 hunks)tests/functional/flakes/meson.build(1 hunks)tests/functional/flakes/substitution.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libfetchers/fetchers.cc (4)
src/libfetchers/path.cc (2)
store(120-159)store(120-120)src/libflake/flake.cc (2)
getFingerprint(968-991)getFingerprint(968-968)src/libfetchers/git.cc (2)
getFingerprint(885-914)getFingerprint(885-885)src/libfetchers/filtering-source-accessor.cc (2)
getFingerprint(62-67)getFingerprint(62-62)
tests/functional/flakes/substitution.sh (1)
tests/functional/flakes/common.sh (2)
createFlake1(51-56)createFlake2(58-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (2)
src/libfetchers/fetchers.cc (1)
346-353: Nice early return when a valid store path existsReusing the already-valid store path for final inputs keeps control flow simple and avoids redundant fetches.
tests/functional/flakes/meson.build (1)
38-39: Test registration LGTMIncluding
substitution.shin the flakes suite looks correct.
…ails Previously we always tried to substitute first (by calling `ensurePath()`). This wasn't a problem before lazy trees, because we always end up copying to the store anyway. But that's not the case with lazy trees. So frequently we ended up substituting an input that we had already fetched. This showed up as fetching source from https://cache.nixos.org for inputs that you could swear Nix had already fetched just before. This was especially a problem for Nixpkgs inputs, since many Nixpkgs revisions are in cache.nixos.org. Note that this could also be a problem without lazy trees, e.g. with a local input (like a Nixpkgs clone) that happens to be in the binary cache. So we now only try substitution as a last resort, if we cannot fetch the input normally.
edolstra
force-pushed
the
substitute-inputs-on-failure-only
branch
from
565c79c to
3fddd14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/libfetchers/fetchers.cc(1 hunks)src/nix/main.cc(1 hunks)tests/functional/flakes/meson.build(1 hunks)tests/functional/flakes/substitution.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/nix/main.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/functional/flakes/substitution.sh
- src/libfetchers/fetchers.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_x86_64-linux / test
- GitHub Check: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (1)
tests/functional/flakes/meson.build (1)
38-38: New flakes test registered — LGTM.Including
substitution.shin the flakes suite aligns with the PR objective. Commas and structure look correct.
cole-h
reviewed
Sep 12, 2025
| // Fall back to substitution. | ||
| try { | ||
| store->ensurePath(*storePath); | ||
| warn( |
Member
There was a problem hiding this comment.
Should this really be warn level? IMO, probably would be fine as debug level instead.
Collaborator
Author
There was a problem hiding this comment.
Yeah, I don't think we want to quietly swallow a fetcher error since the user may want to know about it so they can fix it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Previously we always tried to substitute first (by calling
ensurePath()). This wasn't a problem before lazy trees, because we always end up copying to the store anyway. But that's not the case with lazy trees. So frequently we ended up substituting an input that we had already fetched.This showed up as
for inputs that you could swear Nix had already fetched just before. This was especially a problem for Nixpkgs inputs, since many Nixpkgs revisions are in cache.nixos.org.
Note that this could also be a problem without lazy trees, e.g. with a local input (like a Nixpkgs clone) that happens to be in the binary cache.
So we now only try substitution as a last resort, if we cannot fetch the input normally.
Context
Summary by CodeRabbit
Bug Fixes
Tests