Skip to content

fix(cache): fail the run when a cache hit's outputs can't be restored - #770

Merged
wan9chi merged 3 commits into
mainfrom
fix-cache-hit-restore-failure
Oct 2, 2026
Merged

wan9chi merged 3 commits into
mainfrom
fix-cache-hit-restore-failure

Conversation

@wan9chi

@wan9chi wan9chi commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Motivation

When a cache hit's output archive can't be restored, for example because it was deleted or is corrupt, vp run prints an error but exits 0, and both the summary and --last-details report 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

  • A failed restore is saved in the summary as a new TaskResult::RestoreFailed. It counts as failed rather than as a hit, sets a non-zero exit code, and --last-details shows it as → Cache hit, but the outputs couldn't be restored (or Remote cache hit) followed by the error and its causes.
  • For a local hit, the error is now Cache restore failed. Run `vp cache clean` to clear the cache: failed to extract the output archive: … instead of Cache 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's Cache restore failed: failed to extract the output archive: …, without the hint, because nothing was saved locally.
  • A remote hit is saved locally only after its outputs are restored, so a failed restore just deletes the downloaded archive, and the next run fetches the remote entry again.
  • Tests: output_cache_test deletes the archive of a cached task and checks the failed hit, --last-details, and that the task executes again after vp 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

@wan9chi
wan9chi force-pushed the fix-cache-hit-restore-failure branch from 8282c74 to 7a0993c Compare September 27, 2026 16:40
@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

fspy benchmark

linux

dynamic/launch             change  +0.50%  [ -6.00% ..  +8.82%]  overhead  +269.29%
dynamic/access             change  -0.15%  [ -1.60% ..  +1.85%]  overhead   +13.91%
dynamic/access-relative    change  -0.22%  [ -1.74% ..  +2.21%]  overhead   +60.53%
dynamic/access-contended   change  -0.69%  [ -3.76% ..  +1.39%]  overhead   +14.75%
static/launch              change  +1.99%  [ -4.32% ..  +7.33%]  overhead  +727.87%
static/access              change  -0.64%  [ -1.90% ..  +0.90%]  overhead  +808.63%
static/access-relative     change  +0.17%  [ -0.89% ..  +1.16%]  overhead +1385.11%
static/access-contended    change  -0.21%  [ -1.13% ..  +0.73%]  overhead +3162.54%

macos

dynamic/launch             change  -0.01%  [-10.67% .. +11.09%]  overhead  +238.39%
dynamic/access             change  +2.01%  [-13.42% .. +89.12%]  overhead    +8.45%
dynamic/access-relative    change  +0.74%  [ -6.43% ..  +6.72%]  overhead  +241.79%
dynamic/access-contended   change  +0.64%  [ -6.09% .. +14.89%]  overhead    +1.05%

windows

dynamic/launch             change  +0.35%  [ -5.37% ..  +8.05%]  overhead   +25.21%
dynamic/access             change  -0.51%  [-18.23% ..  +8.92%]  overhead    +1.74%
dynamic/access-relative    change  -1.86%  [-35.22% ..  +0.51%]  overhead    +1.19%
dynamic/access-contended   change  -0.89%  [-11.45% ..  +6.09%]  overhead    +2.20%

wan9chi and others added 2 commits October 2, 2026 10:37
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
wan9chi force-pushed the fix-cache-hit-restore-failure branch from 7a0993c to 2e24d9e Compare October 2, 2026 02:37
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
wan9chi marked this pull request as ready for review October 2, 2026 03:36
@wan9chi
wan9chi merged commit 040e086 into main Oct 2, 2026
19 checks passed
@wan9chi
wan9chi deleted the fix-cache-hit-restore-failure branch 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A cache hit whose outputs can't be restored exits 0 and is reported as a hit

1 participant