Repository navigation
fix(cache): fail the run when a cache hit's outputs can't be restored - #770
Merged
Merged
Conversation
wan9chi
force-pushed
the
fix-cache-hit-restore-failure
branch
from
September 27, 2026 16:40
8282c74 to
7a0993c
Compare
fspy benchmarklinuxmacoswindows |
A cache hit whose output archive can't be extracted now fails the run with a non-zero exit code and is reported as failed, with its error, in the run summary and `--last-details`. The entry and its archive are removed from the local cache, so the next run misses instead of failing the same way. Remote hits are recorded locally before they're restored, so this covers them too; the next run fetches the remote entry again. Closes #767 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
A remote hit is now recorded locally only after its output archive is extracted, so a failed restore leaves no entry to evict and only the downloaded archive is removed. A local hit whose outputs can't be restored is still evicted, but only if the entry still refers to the archive that failed, so an entry another process recorded in the meantime is kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
wan9chi
force-pushed
the
fix-cache-hit-restore-failure
branch
from
October 2, 2026 02:37
7a0993c to
2e24d9e
Compare
A local hit whose outputs can't be restored is no longer removed from the cache. Its error suggests running `vp cache clean` instead, as on main, while the cause chain stays `failed to extract the output archive: <error>`. Remote hits are unchanged: they're recorded locally only once their outputs are restored, so a failed restore only removes the downloaded archive. The fix is now listed under the remote caching changelog entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
wan9chi
marked this pull request as ready for review
October 2, 2026 03:36
wan9chi
added a commit
that referenced
this pull request
Oct 4, 2026
… HTTP/2 (#787) ## Motivation In `read-write` mode, a task waited for its upload to the remote cache before it counted as finished, so every task that depended on it waited too. A slow remote cache slowed down the whole run, even though nothing in the run needed the upload. Once uploads run side by side, HTTP/1.1 also opens a connection for each one; HTTP/2 lets them share one. ## Changes - **Background uploads.** Once a task's result is cached locally, its upload starts in the background and the task finishes right away. When the graph is done, `vp run` prints `Waiting for N remote cache uploads to finish (Ctrl-C to cancel)...` and waits for them before the summary. An upload's error is set later, through the `Arc<OnceLock<UploadError>>` in `CacheUpdateStatus::Updated`, and the summary reads it after the wait. - **Cancellation.** A new interrupt token, which only Ctrl-C cancels, cancels the uploads. Their entries stay in the local cache, and the summary says they weren't uploaded because they were interrupted. Fast-fail still stops lookups but no longer stops uploads, so tasks that succeeded before another one failed are still uploaded. `docs/cancellation.md` covers both. - **HTTP/2.** reqwest's `http2` feature is on. HTTPS endpoints use HTTP/2 if the server accepts it in the TLS handshake and HTTP/1.1 otherwise. `http://` endpoints stay on HTTP/1.1. - **Tests.** Whether an upload is still running when the graph is done depends on how fast the remote cache responds. So e2e steps that upload set `VP_RUN_INTERNAL_HIDE_PENDING_UPLOADS`, which keeps the waiting line out of their output, including the first step of `restore_failure` from #770. The cases that show the waiting line or cancel uploads put `vtt stalled-remote-cache --stall /store` from #795 in front of the backend. Their fetches reach the backend and miss, and their uploads never finish. Like the other backend cases, they need Node.js and are skipped on Windows. New e2e cases: `pending_uploads`, `hide_pending_uploads`, and `fast_fail_during_upload`. `ctrl_c_during_upload` from #795 now shows the waiting line. A unit test checks that the client offers `h2` over TLS. ## Notes for reviewers - **Uploads aren't capped.** Each upload in progress holds its encoded entry in memory and its output archive open, plus its own connection on HTTP/1.1, and none of that counts against `--concurrency-limit`. A large graph with a slow endpoint can pile them up. A cap is left for a follow-up. - **No HTTP/3.** reqwest's `http3` feature needs `--cfg reqwest_unstable` in every build, vite-plus included, adds aws-lc-rs next to ring, bypasses proxies, and is used only when the client forces HTTP/3 for every request. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
Motivation
When a cache hit's output archive can't be restored, for example because it was deleted or is corrupt,
vp runprints an error but exits 0, and both the summary and--last-detailsreport the task as a cache hit. CI passes without the task's outputs. Remote hits are also recorded locally before they're restored, so a remote hit that can't be restored leaves a local entry that fails every later run the same way, even though the remote entry is fine.Changes
TaskResult::RestoreFailed. It counts as failed rather than as a hit, sets a non-zero exit code, and--last-detailsshows it as→ Cache hit, but the outputs couldn't be restored(orRemote cache hit) followed by the error and its causes.Cache restore failed. Run `vp cache clean` to clear the cache: failed to extract the output archive: …instead ofCache lookup failed: failed to restore cached outputs from <path>; …. The entry stays in the cache, so the hint is still needed. For a remote hit, it'sCache restore failed: failed to extract the output archive: …, without the hint, because nothing was saved locally.output_cache_testdeletes the archive of a cached task and checks the failed hit,--last-details, and that the task executes again aftervp cache clean. This runs on all platforms.remote_cache::restore_failure(ignored and not on Windows, like the other remote backend cases) makes a remote hit's restore fail and checks that the downloaded archive is removed and the next run fetches and restores the entry again.vtt rm --ext <suffix> <dir>removes the archive without hardcoding its name or the cache schema directory.Closes #767