Repository navigation
Cached outputs are restored onto disk on a cache hit - #150
Merged
calvin-archastro merged 2 commits intoOct 15, 2026
Merged
calvin-archastro merged 2 commits into
calvin-archastro merged 2 commits into
Conversation
Contributor
Author
|
ArchDev is watching this pull request. State: settled (cycle 2 of 2 for head d68104d).
Paused by the |
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
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.
Epic: Aster restores real build artifacts from a shared cache, including the GitHub Actions cache
Task: Cached outputs are restored onto disk on a cache hit
Local git hooks skipped: pre-push on the submission push and the publish push. CI is the gate for what those hooks check.
Problem and author intent
Aster’s cache could pack declared target outputs into the local content-addressed store, but a hit could not put those artifacts back on disk. The store only recorded hash, timestamp, and success, so stale workspace files were never replaced from blobs.
Pack also rewrote a relative symlink that stayed inside the project into a collapsed project-relative path. A link at
dist/linkswhose text was../app.jswas stored asdist/app.js. Manifest decode rejected any target that was not a strictly safe relative path, so a..component failed even when resolving it from the link’s parent never left the project. Restoring that collapsed text as the link would point at the wrong place relative to the link parent.The intent of this change is to materialize a packed entry list under the project root: validate every path before the first delete, replace each declared output (file, directory, or symlink) from the local store, keep original in-project relative link text, and on a missing or corrupt blob remove the declared paths again so the caller can treat the hit as a miss.
What changed
Before,
packcollapsed in-project relative symlink targets, andManifestvalidation refused those..targets. There was norestore.After:
src/cache/outputs.rsaddsrestore(project_root, entries, store). It checks every entry before deleting anything. Absolute paths, empty paths, backslashes, drive-style:, a leading/, or any..in a declared output path are refused and nothing is removed. Duplicate paths are refused. Entries are then ordered parents-before-children.symlink_metadata. A symlink or non-directory isremove_file(so a directory symlink is not followed); a real directory isremove_dir_all. Removal walks deepest-first.0o777are applied. File bytes are read from the store into a same-directory temp (.{name}.aster-restore.{pid}.{seq},create_new),sync_all’d, mode-set, then renamed over the destination. Symlinks are recreated with the stored target text (symlinkon Unix,symlink_fileon Windows).restorereturnsErr. A failed write also deletes its temp file.../app.jsstays../app.js). An absolute link that names a path inside the project is still stored as that project-relative path. Empty targets and targets containing\or:are rejected.src/cache/artifact.rsaccepts a symlink target with..only when lexical resolution from the link parent does not climb out of the project. Absolute targets, prefixes, and escapes are still rejected.src/cache/mod.rsre-exportsrestorenext topack.Scope
Three files:
src/cache/artifact.rs,src/cache/mod.rs, andsrc/cache/outputs.rs(16 hunks, +516 / −13). No omitted hunks, paths, or patch bytes in the supplied diff. Executor cache-hit wiring and remote backends are not in this revision. Advisory pipeline results list three checks (format,lint,test), all succeeded with exit code 0; those log excerpts are truncated, so they are not treated as complete evidence of what ran.Risk assessment
High. Restore deletes and rewrites paths in the workspace. A bad path or a followed directory symlink could remove the wrong tree.
Mitigations in this change: all entries are validated before the first delete; declared paths cannot contain
..or leaveproject_root; symlink targets are accepted only when resolution from the link parent stays inside the project; removal uses symlink metadata and does not follow links; file writes are temp-plus-rename; failure deletes the declared roots again. Undeclared siblings are not in the removal list.Residual risk: path checks are lexical (component walk), not a canonicalized filesystem walk, so unusual link layouts outside the declared entries are not re-resolved at delete time. Cleanup errors on the failure path are ignored. Mode bits are applied only on Unix. Windows restore always uses
symlink_file.User impact
restoreis available to cache callers, but this revision does not wire it into the executor. A cache hit still does not restore artifacts until that caller lands.When
restoreis invoked, each declared output root is replaced from the local store, including Unix modes and symlink text. Files outside the declared set are left in place. A path that escapes the project fails closed with no deletes. A missing or corrupt blob fails and leaves none of the declared outputs behind, rather than a partial tree.Testing
New tests are present in the diff; the patch is not evidence that they passed.
In
src/cache/outputs.rs:restore_round_trip_matches_packed_tree_including_modesrestore_aborts_on_corrupt_blob_and_leaves_no_partial_outputrestore_refuses_path_that_escapes_project_rootrestore_same_directory_symlink_stays_inside_the_projectrestore_removes_a_directory_symlink_without_following_itIn
src/cache/artifact.rs:decode_accepts_parent_relative_symlink_that_stays_insideAn existing pack assertion now expects
../app.jsinstead of the collapseddist/app.js.The advisory
testcheck is reported succeeded (exit code 0), as areformatandlint. Excerpts are truncated and do not name these functions, so this review cannot confirm from the logs that these tests ran or that the cited CI command covered them. The workflow file is not in this diff. Author notes say theTestjob runscargo test --locked --workspace --all-targets --all-featuresand that the five restore tests passed; that is a claim, not log evidence here. Windows symlink creation is cfg-gated and is not shown as executed.Follow-ups and known issues
CacheBackendsupport are out of scope; restore is unused by the runner in this revision.artifact.rsandoutputs.rs, and they differ (to_strvsto_string_lossy).Err.testoutcome as proof these new cases ran.