Repository navigation
Cache hits replay the original log and exit status - #160
Open
calvin-archastro wants to merge 2 commits into
Open
calvin-archastro wants to merge 2 commits into
calvin-archastro wants to merge 2 commits into
Conversation
Contributor
Author
|
ArchDev is watching this pull request. State: watching (cycle 3 of 3 for head 911ff4f).
Paused by the |
calvin-archastro
force-pushed
the
factory/tsk_2V8v8TvDrPgG2r6dh4dDiL-d44f3154-8358-40eb-ab49-
branch
from
October 19, 2026 02:26
f045896 to
911ff4f
Compare
This branch has not been deployed
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: Cache hits replay the original log and exit status
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
A cache hit restored outputs but did not replay the stored target log or the recorded duration. The hit result used an empty
output/log,duration_ms: 0, and a hardcoded exit code of 0. JSON then treated cached results like skips and droppedexit_codetoNone, so CI logs for cached targets stayed silent and status fields did not match the manifest.Intent: on a hit, print
Manifest.logthrough the same captured-output path live targets use, mark the status line(cached), and report the stored exit code and duration.--no-replaysuppresses only the printed log.What changed
Before,
check_cachereturned(cache_hit, hash). A hit built anExecutionResultwith empty output, zero duration, andSome(0), then calledmark_cachedwith only the address.build_execution_outputsetexit_codetoNonewhencachedwas true.After, lookup is
CacheLookup::Hit { manifest, log }orMiss(hash). The log blob is loaded before restore. A missing or unreadable blob, or a restore failure, warns and is a miss so the target runs instead of replaying nothing. A manifest with no log digest is still a hit with an empty log.cached_resultfillsoutputandlogfrom the blob (from_utf8_lossy),duration_msfrommanifest.duration_ms, andexit_codefrommanifest.exit_code. Unless the mode is JSON or--no-replayis set, that log is printed on the same channel asaster logs/ExecutionResult.output.ProgressDisplay::mark_cachedthen prints a(cached)suffix (CI and verbose include PASS and duration; normal mode prints the address and suffix). JSON keeps the log on the result and does not print it. Skips still useexit_code: None; cache hits no longer do. Failures are still not cached, so the stored code is expected to be 0.Global
--no-replay(default off) is threaded through the run, affected, and heterogeneous executors viaExecutor::with_no_replay. It suppresses printing only;cached, exit code, and duration stay on the result.Scope
Seven paths, 24 hunks, +305/−42:
.github/workflows/ci.yml,src/cli/commands.rs,src/cli/output.rs,src/executor/runner.rs,src/main.rs,src/ui/progress.rs,tests/cache_artifacts.rs. Those patch hunks are fully included (omitted_hunks: 0,omitted_paths: 0). No hasher or remote-cache backend changes are in this revision.Risk assessment
Author grade: low. Most of the change is output and status reporting. Hit eligibility is slightly stricter: a matching manifest whose log blob cannot be read is no longer a hit. Non-UTF-8 logs can be altered by lossy UTF-8 conversion when printed or stored on
output.User impact
A cache hit on captured
aster run/ target execution reprints the stored log and shows(cached), with stored duration in CI and verbose progress.aster run --jsonresults includecached: true, the storedexit_code,duration_ms, and the log inoutput, without printing the log to stderr.--no-replayhides the replayed log only. Callers that treated a cachedexit_codeas absent will now see the stored code.Testing
Canonical test added in source:
tests/cache_artifacts.rscache_hit_replays_stored_log_and_exit_status. It runs//app:build(echo hello, cache enabled), checksaster logsstoredhello, then asserts a second run replayshelloand(cached);--jsonreportscached: true,exit_code0, aduration_ms, andoutputcontaininghellowithouthelloon stderr;--no-replayomitshellobut still marks(cached);--json --no-replaystill reports cached, exit code, and duration. The workflow diff adds--test cache_artifacts. The task states this is the macOS job, and that Ubuntu already runscargo test --locked --workspace --all-targets --all-features.Advisory pipeline outcomes:
format,lint, andtesteach reportedsucceededwith exit code 0. Check excerpts are truncated and do not namecache_hit_replays_stored_log_and_exit_status, so this review cannot confirm that function from the CI log. Patch text is not proof the test passed.diagnostics.truncatedis true; the pipeline log is not complete evidence.Follow-ups and known issues
Streaming targets (
execute_streaming) do not consult the cache. Replay covers captured run/target execution only.aster watchuses a quiet executor and does not print target logs. Cached results still settests: None.