fix: reuse immutable Git build artifacts without checkout - #6887
Open
baszalmstra wants to merge 20 commits into
Open
fix: reuse immutable Git build artifacts without checkout#6887baszalmstra wants to merge 20 commits into
baszalmstra wants to merge 20 commits into
Conversation
baszalmstra
force-pushed
the
fix/immutable-git-artifact-cache
branch
from
August 25, 2026 10:12
71ff897 to
1468374
Compare
The checkout-free artifact key stands a synthetic backend identifier in for the real one, which is only knowable after a checkout. That identifier hashed the whole `SourceBuildSpec`, which carries `exclude_newer`. For a relative `exclude-newer` such as this workspace's own "7d", `ResolvedExcludeNewer::cutoff` is `Utc::now() - duration` and therefore differs on every invocation. The key was consequently unique per process. The checkout-free path could never hit across runs, and because the immutable key *replaces* the backend-keyed one for both lookup and store, every `pixi install` rebuilt the package and wrote another full `.conda` into the workspace-local artifact cache, which has no reclamation path. That is a regression against the previous backend identifier, which was stable across processes. List the inputs explicitly instead. `exclude_newer` only influences which backend gets solved, and that is already validated on lookup by re-solving the recorded backend and comparing its resolved identifier, so it does not belong in the key at all.
`immutable_variant_file_hashes` already hashed each variant file's contents, but it returned the path alongside the hash and the pair went into the key. Variant file paths are absolute (`workspace.root.join(..)`), so every cached artifact was tied to one workspace location: moving or renaming the workspace invalidated the whole immutable cache even though none of the build inputs had changed. What the build reads is the content, so hash only that, in declaration order -- order still matters because later variant files override earlier ones.
Only `channel_alias` was hashed. `root_dir` is the other half of channel resolution: `DiscoveredBackend` resolves the source manifest's `[package.build] channels` through the same `ChannelConfig`, so a relative channel entry resolves against `root_dir`. The stored `ImmutableBackend` records the channel URLs as they resolved at build time, and the checkout-free lookup re-solves from those, so a `root_dir` change would be invisible to the validation and a stale artifact would be handed back. This is not purely theoretical even though the artifact cache is usually workspace-local: `CacheBase::Workspace` falls back to the global cache root when no workspace is configured, so entries can be shared between invocations rooted in different directories.
`immutable_backend` and `lookup_immutable` mapped only `NotFound` onto a miss and propagated every other IO error, which `map_cache_err` turned into a hard `SourceBuildError`. The checkout-free probe runs before the checkout on every immutable build, so a cache directory that was merely unreadable -- a concurrent pixi, a virus scanner or backup agent holding the handle, a bad mount -- failed the build outright, and did so under the misleading `CreateWorkDirectory` variant. It was also internally inconsistent: a *corrupt* sidecar already degraded to a miss via `serde_json::from_slice(..).ok()`, while an *unreadable* one was fatal. Both mean the same thing. Rebuilding is always a safe fallback for a cache, so make that structural: both now return `Option` rather than `Result`, which removes the `?` at the call sites and makes it impossible to reintroduce. The lock open and lock acquisition degrade the same way. Storing is deliberately left alone -- a failure to write the cache after a successful build is still fatal, matching the existing `store`.
Both `lookup` and `lookup_immutable` resolved the cached `.conda` with `entry_dir.join(&sidecar.artifact_filename)`. `Path::join` normalises nothing, and joining an absolute path discards the base entirely, so a sidecar that had been tampered with or corrupted could name any file on the system. The cache then returned that file together with the sidecar's own `RepoDataRecord` -- url and sha256 included -- vouching for it, and the recorded sha256 propagates into dependents' cache keys. `store_inner` writes the field from the built artifact's own file name, so requiring a single ordinary path component rejects nothing legitimate. This matters more on the checkout-free path, which by design skips the mtime and glob validation that otherwise backstops `lookup`. Note this bounds the damage rather than authenticating the entry: the sidecar and the `.conda` share a directory and therefore a trust level, and the artifact is still not re-hashed on lookup (that would cost a full file hash per cache hit). What it removes is the ability to point *outside* that directory.
`ArtifactCacheKey` is used verbatim as the entry's directory name. url-safe base64 has a case-significant alphabet; NTFS and APFS do not fold-safe filenames. Two distinct keys differing only in case therefore resolve to one on-disk entry, and the wrong artifact comes back. The odds are small -- roughly 55 bits of effective entropy instead of 64 -- but the checkout-free path deliberately skips the mtime and glob validation that otherwise notices a wrong entry, so it has nothing to fall back on. Lowercase hex retires the class for five extra characters per entry directory. The same base64-of-xxh3 pattern appears in seven other places in this crate. Those are left alone: they name work directories and ephemeral environments rather than gating artifact reuse, and changing them all is a much wider blast radius than this PR should have. Existing artifact cache entries become unreachable, which is already true of this branch since it changes key derivation anyway.
`BackendOverride::is_empty` has exactly one caller -- the gate on the source build's checkout-free artifact cache path -- so nothing would notice if it started answering `false` for the default: the optimisation would just silently stop running everywhere. Worth pinning because the feature currently has no end-to-end coverage at all. Every one of the 33 dispatcher integration tests installs an in-memory backend instantiator, which correctly answers `false` here, so the whole `compute_inner` immutable branch is dead in that suite and a green CI says nothing about it. Reaching the other arm needs a real published backend and a network, which is why the earlier commits in this branch carry their regression coverage as unit tests against the key derivation and the cache layer instead. Also documents on `is_empty` itself what it gates, since the name gives no hint.
The checkout-free path reads `ImmutableBackend.channels` off disk and feeds it into an environment solve before anything has validated it, and nothing can: the identifier it is checked against is re-derived from the same descriptor. So a cache file chooses which channel is contacted and which package is downloaded and installed. Nothing in the sidecar drove a solve before this feature, so it is worth writing down. It is acceptable because the artifact cache lives inside the workspace -- whoever can write the sidecar can also write `pixi.toml`, which grants arbitrary channels and code execution at build time, so the descriptor adds no reach. The comment says so explicitly, and says what would change if the cache ever moved somewhere less trusted. It also records why the obvious guard is wrong: restricting the descriptor to the workspace's own channels disables the optimisation for the canonical setup, because `[package.build] channels` routinely names a backend channel (`https://prefix.dev/pixi-build-backends`) that the workspace does not list. Every minimal-backend test workspace in this repo is shaped that way. Also applies `cargo fmt` over the preceding commits and moves `immutable_identifier_tests` to the end of the file, since clippy's `items_after_test_module` fires otherwise.
The feature had no coverage at all. Every dispatcher integration test
installs an in-memory backend, and this suite's session-wide autouse
`setup_build_backend_override` fixture points its backends at
workspace-built binaries. Both are `BackendOverride`s, and the
checkout-free path is gated on there being none, so it was unreachable
in every existing test and a green CI said nothing about it.
The proof is destructive rather than a log assertion. After the first
build the test deletes the git checkout cache *and the origin repository
itself*, so any attempt to materialise the source has nothing to fetch
from. A second install that still succeeds can only have reused the
cached artifact.
The consuming workspace deliberately uses a relative `exclude-newer`,
which is what most workspaces (including pixi's own) do. That resolves
to `Utc::now() - 7d`, a different instant on every invocation, so the
test also covers the cache key staying stable across processes.
Verified by negative control rather than by assuming: reintroducing the
wall-clock input into the key makes the second install miss the cache,
attempt a checkout, and fail with
failed to fetch file:///.../git_package:
fatal: '...' does not appear to be a git repository
Marked `extra_slow`. It needs a real, channel-solved backend, since an
overridden one has no content-addressed identity to validate against --
which is exactly why no existing test could reach this code.
The checkout-free path was gated on `BackendOverride::is_empty()`, i.e. on *no* backend anywhere being overridden. Overriding a single backend therefore disabled the optimisation for every package in the workspace, including packages built by a completely different, channel-solved backend that the override never touched. The gate is also redundant. `resolve_immutable_backend_identifier_from_spec` resolves through `ResolvedBackendCommandKey`, which re-applies the override, and returns `None` for anything that is not an `EnvironmentSpec`. An overridden backend is a `CommandSpec::System`, so it already declines the checkout-free path on its own -- per backend, which is the right granularity. Drop the gate and `is_empty()` with it: this PR added that method for this single caller, so it now has none. The end-to-end test covers this: it leaves an unrelated backend overridden while the package under test uses a channel-solved one. Verified by negative control -- restoring the gate makes that test fail on the deleted repository, exactly as a cache miss would.
baszalmstra
marked this pull request as ready for review
August 27, 2026 14:00
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
1 participant
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.
A full Git commit identifies the source content, but source-build cache hits still fetch and check out the repository before checking for a cached artifact. This draft adds immutable artifact aliases for pinned Git sources while retaining backend-identifier validation and the existing mutable-source path.
Testing
git diff --check.cargoorrustfmt.AI Disclosure
Tools: Pi
Checklist