[1/2] Unique the filesystem package cache by build inputs - #1306
[1/2] Unique the filesystem package cache by build inputs#1306greenhat wants to merge 13 commits into
Conversation
The filesystem package cache at target/miden/packages persisted across builds and keyed files by package name alone. When build inputs changed (dependency set, versions, pins, or the compiler itself), leftover .masp files from older builds could satisfy the SDK proc-macro lookups through MIDENC_PACKAGE_CACHE and bake stale FPI procedure roots into generated code, failing only at transaction execution (#1302, surfaced by #1300). Derive the cache path as target/miden/packages/<fingerprint>, where the fingerprint hashes the compiler version and revision, the build-relevant options, and the project's recursive manifest closure (including resolved dependency schemes and preassembled package contents). Session memoizes the fingerprint, so the package registry and the MIDENC_PACKAGE_CACHE variable handed to nested cargo builds keep agreeing on one derivation. Registry construction now also prunes stale midenc-owned cache entries (fingerprint directories and legacy flat .masp files) next to the current directory. The pruning is load-bearing: the macros track their package reads with include_bytes! dummies, so a surviving stale directory would keep cargo reusing a stale macro expansion. Deleting it forces re-expansion against the fresh cache. The fingerprint intentionally excludes Rust sources and lockfiles: every run rewrites each resolved package into the cache before its consumers expand, and content changes at a stable path already invalidate consumers through the include_bytes! tracking. The cargo-miden integration tests that asserted the flat packages/<name>.masp layout now locate the build's single fingerprint directory instead.
…erprint gaps A pre-submit review of the fingerprinted package cache found one race and several hardening gaps. The prune deleted every sibling fingerprint directory unconditionally, so two concurrent builds of one project with different inputs (debug and release builds of the same checked-in example, as the test suite itself arranges) could delete each other's live cache between a package write and the consuming macro's read. Each build now holds an exclusive advisory lock on a .build-lock file inside its fingerprint directory for the registry's lifetime, and the prune deletes a sibling only when its lock is free or absent. A live directory is skipped; a lock that cannot be verified is left in place with a warning, since deleting an unverifiable cache risks reviving the stale-expansion bug the prune exists to prevent. Pruning is also refused entirely when the target path is not fingerprint-named, so external callers of the public constructor cannot sweep an arbitrary parent directory, and failed removals of owned entries are logged at warn with their consequence. The fingerprint gains two inputs that escaped it: the inherited RUSTFLAGS environment (composed into every nested cargo build) and the containing workspace's root manifests (member manifests do not change when workspace-level fields do). Moved git branches remain outside the fingerprint by design, now documented. The fingerprint format is defined once and shared by the producer, the prune recognizer, and their tests; record_options destructures Options exhaustively so a future field must be explicitly classified as fingerprinted or ignored; link libraries contribute their declared identity instead of a redundant package load; and the design rationale that previously lived outside the tree is captured in module and function docs. The FPI macro diagnostic for the cache-lookup branch now names the searched MIDENC_PACKAGE_CACHE directory and the expected package file names instead of an empty candidate list and a profile-directory hint that branch never consults. New tests pin the liveness behavior (a locked sibling survives, an unlocked one is pruned), the misuse guard, the fingerprint walk's cycle guard and degradation markers, and — end to end — the invalidation contract itself: rebuilding after a dependency source change keeps the same cache path but replaces the FPI procedure root baked into the consumer's assembly.
The stale-expansion story rested entirely on pruning: a consumer's cached macro expansion was re-expanded only because its include_bytes! target vanished. Pruning is best-effort, so every failure path — a live locked directory that deliberately survives, a failed removal on a restrictive filesystem, a directory recreated by a still-running old-input build — left the original stale-roots bug reachable.
Emit const _: Option<&str> = option_env!("MIDENC_PACKAGE_CACHE") into every FPI expansion, next to the existing include_bytes! constants. The variable's value carries the fingerprinted cache path, so rustc records it in the consumer's dep-info and Cargo re-expands the macro whenever the fingerprint rotates, even when a stale directory survives on disk. The same mechanism already invalidates cached expansions for MIDENC_EMIT_WIT. include_bytes! keeps covering content changes at an unchanged path.
Pruning and locking thereby demote from correctness-critical to defense in depth: they remove legacy flat files whose expansions predate this tracking, keep the cache parent bounded, and prevent transient mid-build file loss. The prepare_filesystem_cache doc is updated to say so.
The pre-submit review found four defects around cache preparation. A build that failed to create its own cache directory still swept every sibling; an identical-fingerprint contender ran unprotected after observing WouldBlock, so its directory could be deleted mid-build once the first holder exited; the lock file lived inside the directory it protected, leaving create-before-lock and unlock-before-delete windows; and the public constructor pruned the parent of any 16-hex-named path, so an arbitrary caller-supplied location could have unrelated siblings deleted. Locks now live outside the deletable directory as packages/<fingerprint>.lock and are acquired before the directory is created. Builders hold the lock shared, so any number of identical-input builds stay protected at once; pruning demands the exclusive lock and holds it while remove_dir_all runs, then removes the orphaned lock file after verifying no builder acquired it. The residual close-to-unlink race is documented as accepted: option_env!(MIDENC_PACKAGE_CACHE) in FPI expansions is the correctness boundary, and pruning is defense in depth. Preparation now stops before any deletion when the cache or its parent cannot be created, and the destructive sweep runs only for paths in the owned miden/packages/<fingerprint> layout. Registry insertion also detects a same-name, same-version, different-digest conflict before touching disk, so a rejected package no longer overwrites the cache file that the accepted in-memory package no longer matches. Accepting paths still rewrite their file on every run, which the content self-heal relies on. Legacy flat cache entries are matched case-insensitively via Package::EXTENSION, so a Legacy.MASP leftover no longer survives the sweep on case-insensitive filesystems. Prune failures keep their consequence in the message and now name the actual parent directory; the log-only nature of cleanup reporting is documented on the constructor.
Three inputs escaped the fingerprint or degraded it silently. A bare relative --manifest-path (e.g. plain Cargo.toml) produced an empty project directory, so the manifest walk recorded a load failure and never visited path dependencies; the derivation now absolutizes the locator against the session's configured working directory. A workspace member dependency was classified by file extension instead of being resolved through the loaded workspace; it now resolves via get_member_by_relative_path like the canonical resolver. RUSTUP_TOOLCHAIN influences the nested build's toolchain selection the same way inherited RUSTFLAGS influences its flags, so it is fingerprinted the same way, as a parameter the session reads from the environment. The walk now uses a private source manager, so computing the cache path no longer interns every closure manifest into the compilation session's source manager as a side effect. The remaining walk-versus-resolver deviations are consolidated into one comment pointing at the closest in-tree sibling (frontend/masm's collect_dependency_metadata_for_scheme): path dependencies are extension-classified before canonicalization, and git declarations are recorded but never recursed. The module docs now cover the degraded cases and their recovery: Cargo-only projects without a miden-project.toml (root manifests hashed, no dependency recursion, reported at debug level), moved unpinned git revisions and transitive git dependencies being outside the closure, dropped-but-cached names lingering until the fingerprint next rotates, and the per-member-session assumption workspace builds rely on, noted where the root session is created.
The cargo-miden cache tests had become tautological: the lookup helper only returned directories that already contained the expected package, and the pre-build cleanup that attributed the artifact to the build under test was removed with the fingerprint layout. The tests now snapshot the time before the build and assert the located package was written at or after it, the helper accepts only fingerprint-shaped directory names so a regression to a flat layout fails, and the masm test derives the expected file name from the dependency name instead of repeating a literal. The end-to-end FPI test previously covered only the same-fingerprint half of the design: a dependency source change rewrites the package in place and include_bytes! re-expands the consumer. A third build phase now covers the rotation half that #1302 is actually about: bumping the dependency's version in its manifests moves the cache to a new fingerprint directory, removes the old one, and the consumer's assembly carries only the new procedure root. The digest-recognition helper documents its coupling to the current u64-immediate lowering shape so a codegen change there is not misread as a stale-root regression. Also guards the swapp-note fixture mutation like the existing one, and corrects the persist_cargo_miden_dependency docs: that directory is a legacy fallback consulted only when MIDENC_PACKAGE_CACHE is unset.
… publication atomic The third review round found the lock protocol violating its own contract on two legs. A builder observing WouldBlock — meaning a pruner was deleting its directory at that moment — continued unlocked for the whole build, and the constructor doc claimed a lock it did not hold. And because flock binds to the inode, unlinking lock files after pruning let a builder hold a lock on an unlinked inode while the next pruner locked a fresh file at the same path and deleted the live directory. Builders now take the blocking lock_shared on their fingerprint lock: pruners only ever try exclusive locks and never wait while holding one, and a builder waits only for its own lock while holding none, so the wait is deadlock-free and bounded by one in-progress removal. Lock files become permanent rendezvous objects — empty, bounded by the number of distinct fingerprints ever seen — which removes the inode ABA together with the three orphan-lock helpers. Preparation failures keep the cache configured so the first publication reports the concrete filesystem error, and the docs now describe that degraded mode instead of contradicting it, including that ownership is checked lexically by design and symlinked cache layouts are outside the contract. Package publication writes to a process-unique temp file and renames over the target, so a concurrent identical-fingerprint build can no longer expose a truncated package to a reader; the remaining read-versus-include_bytes window is documented as part of the same-fingerprint boundary. The fingerprint gains CARGO_ENCODED_RUSTFLAGS, which takes precedence over RUSTFLAGS when Cargo invokes rustc. The cache-layout machinery moves out of registry.rs into package_cache.rs, split into guard, create, lock, and sweep helpers, and the module docs now record what the memoized derivation assumes (root-session-only use, clones keeping their fingerprint, the deliberate target-dir exclusion), why the walk cannot reuse miden-project's resolver (it needs the registry whose cache path is being derived, and graph building performs git checkouts), which ambient inputs stay unfingerprinted and why, and the intended content-addressed end-state that belongs with the #1290 package redesign.
The compiler-side cache writers and pruner match the .masp extension case-insensitively via Package::EXTENSION, but the macro-side reader filtered with a case-sensitive literal — so on default-case-insensitive filesystems the pruner would delete a Foo.MASP the reader could never have resolved. Align the reader on the same rule.
…sserts A new fixture pins the load-bearing invalidation mechanism by itself: two prepopulated cache directories whose basic-wallet packages embed different receive-asset roots, and two plain cargo builds of an unchanged consumer sharing one target directory, differing only in the MIDENC_PACKAGE_CACHE value. The embedded roots must follow the environment value in both directions — proof that the option_env! recorded by FPI expansions re-expands consumers on cache rotation without any help from manifest changes or midenc's own driver. The build-attribution asserts in the cargo-miden tests tolerate whole-second mtime truncation, and the fingerprint-directory helper documents its newest-mtime tie-break. The integration-network compile_rust_package helper no longer persists packages to target/miden/<profile>: nothing under that suite reads the path, and compiler-driven builds resolve dependencies exclusively through the fingerprinted cache.
…cargo builds Cargo prefers CARGO_ENCODED_RUSTFLAGS over RUSTFLAGS, and the nested cargo inherited the caller's environment — so a CI image or build-script context exporting the encoded variable silently replaced every mandatory Miden flag: no --cfg miden, no wasm target features, no immediate-abort panic strategy, with nothing attributing the resulting breakage to the inherited variable. cargo_env now emits the composed flags in both spellings; the explicit encoded value makes any inherited one inert. The encoding splits the composed string on whitespace, which is exactly how cargo interprets the plain variable, so the flags cannot change meaning between the two forms.
Two preparation legs degraded harder than the documented contract. A lock-open failure aborted preparation entirely while its log claimed the build was continuing — the cache directory then materialized anyway through the first publication's create_dir_all, unlocked and unswept. And a cache-create failure returned None after the shared lock was already acquired, releasing the one guard that would protect the directory a later publication recreates. The first leg now creates the directory before returning, and the second keeps and returns the held lock, skipping only the sweep; the function doc states what Some and None actually mean. With the encoded rustflags now set authoritatively for nested builds, the inherited CARGO_ENCODED_RUSTFLAGS value has no effect on what gets built, so it leaves the fingerprint (a comment records why, so it is not re-added). The cache-path producer and the owned-layout validator were two unlinked lexical derivations; the parent-path construction moves next to the validator and the session derivation test asserts the produced path satisfies the ownership check, so a future layout change breaks a test instead of silently disabling locking and pruning.
…uites Compiler-driven builds always set MIDENC_PACKAGE_CACHE and the FPI macro has no fallback once it is set, so persisting compiled dependency packages to target/miden/release never influenced any test — the five calls only wrote artifacts into the checked-in example and fixture trees. An editor-driven consumer expansion without the variable is served by the dependency's own cargo miden build output, not by test-suite side effects. The counter-note test also loses its dependency pre-build, which existed only to feed the persistence; the consumer build compiles the contract itself.
The option_env!(MIDENC_PACKAGE_CACHE) recording is the most user-visible behavior change on this branch — consumer crates now recompile whenever the fingerprinted cache path rotates — and it was missing from the changelog entries for #1302.
e94968a to
04abb26
Compare
bitwalker
left a comment
There was a problem hiding this comment.
Looks pretty good, but Codex found a few issues that I think are worth addressing before we merge
| } | ||
| }; | ||
|
|
||
| if let Err(err) = lock.lock_shared() { |
There was a problem hiding this comment.
[P1] Serialize same-fingerprint writers. This shared lock prevents pruning but permits multiple builders to publish different bytes to the same package filename. Because package-selecting inputs such as search paths are excluded from the fingerprint, one build can read another build’s FPI roots while linking its own in-memory package. Use immutable generations, include every selecting input, or take an exclusive writer lock.
| project_dir.join("target").join("miden").join("packages") | ||
| } | ||
|
|
||
| pub(crate) fn is_owned_filesystem_cache_path(filesystem_cache: &Path) -> bool { |
There was a problem hiding this comment.
[P1] A suffix does not prove cache ownership. The existing public custom-cache constructor reaches this check, but any caller path ending in miden/packages/<16-hex> passes and enables recursive deletion of sibling fingerprint directories and flat .masp files. Require an ownership marker/capability or restrict pruning to internally derived Session cache paths.
| filesystem_cache_dir: Option<&Path>, | ||
| extra_rust_flags: String, | ||
| ) -> Vec<(&'static str, String)> { | ||
| let encoded_rust_flags = |
There was a problem hiding this comment.
[P1] Preserve encoded Cargo flags. This replaces inherited CARGO_ENCODED_RUSTFLAGS with an encoding derived only from plain RUSTFLAGS, silently losing legitimate caller flags. Preserve Cargo’s encoded value using its 0x1f argument boundaries, merge the mandatory and explicit flags, and emit the combined value.
| } | ||
| } | ||
| DependencyVersionScheme::Workspace { member, .. } => { | ||
| if let Some(manifest_path) = workspace |
There was a problem hiding this comment.
[P2] Normalize workspace manifest paths. miden-project accepts workspace members referenced as dep/miden-project.toml, but get_member_by_relative_path compares against the member directory. That valid spelling therefore records unresolved-workspace and skips the member’s manifest closure. Normalize manifest-file paths to their parent before lookup.
| // build-input fingerprint, so Cargo re-expands this macro whenever the fingerprint | ||
| // rotates — even when a stale cache directory survives on disk. The `include_bytes!` | ||
| // constants above cover content changes at an unchanged path. | ||
| const _: Option<&str> = option_env!("MIDENC_PACKAGE_CACHE"); |
There was a problem hiding this comment.
[P2] Qualify generated tracking names. This code is emitted into consumer scope, where a user-defined Option or option_env! can shadow these names. Qualify the type and macro through core so cache tracking remains hygienic, and add a consumer-scope shadowing test.
Close #1302
The filesystem package cache at
target/miden/packagespersisted across builds and keyed files by package name alone. The SDK proc macros resolve FPI dependency packages from it throughMIDENC_PACKAGE_CACHEat expansion time, so when build inputs changed (dependency set, versions, pins, or the compiler itself), a leftover.maspfrom an older build could satisfy the lookup and bake stale procedure roots into generated code — failing only at transaction execution, far from the cause.The cache path is now
target/miden/packages/<fingerprint>/, where the fingerprint hashes the compiler version and revision, the build-relevant options (including the inheritedRUSTFLAGSenvironment), and the project's recursive manifest closure: root and workspace-root manifests, each path dependency's manifests, resolved dependency schemes (so workspace-inherited pins count), and the content of preassembled.maspdependencies. The existing single derivation point inSessionstill feeds both consumers — the registry writes and the env var handed to nested cargo builds — so nothing else changes shape.Rust sources and lockfiles are deliberately not fingerprinted: the fingerprint covers what changes the set and identity of packages, while content-level changes self-heal — every run rewrites each resolved package into the cache before its consumers expand, and the
include_bytes!reference the macros emit makes cargo re-expand when package contents change at a stable path. The rationale lives in the module docs.The FPI expansion also records
option_env!("MIDENC_PACKAGE_CACHE"), so the fingerprinted cache path itself is a Cargo-tracked input of every consumer: when the fingerprint rotates, cargo re-expands the macro against the new cache even if a stale directory survives on disk. Registry creation additionally prunes stale midenc-owned cache state (sibling fingerprint directories and legacy flat.maspfiles whose expansions predate the env tracking) as defense in depth, and each build holds an advisory lock on a.build-lockfile inside its fingerprint directory so concurrent builds of one project with different inputs cannot delete each other's live cache mid-build; a cache path that is not fingerprint-named is never used to sweep its parent, and failed removals of owned entries warn with their consequence.The FPI macro diagnostic for the cache-lookup branch now names the searched cache directory and the expected package file names instead of an empty candidate list and a profile-directory hint that branch never consults (SDK changelog entry included).