From 6ca1c05b039395e27b47926c689bd9b757919a8c Mon Sep 17 00:00:00 2001 From: wan9chi Date: Mon, 28 Sep 2026 00:39:59 +0800 Subject: [PATCH 1/3] fix(cache): fail the run when a cache hit's outputs can't be restored 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 --- CHANGELOG.md | 1 + crates/vt/src/session/cache/mod.rs | 30 +++++- crates/vt/src/session/event.rs | 3 + crates/vt/src/session/execute/mod.rs | 56 ++++++----- crates/vt/src/session/execute/scheduler.rs | 5 - crates/vt/src/session/mod.rs | 1 - crates/vt/src/session/reporter/summary.rs | 40 ++++++-- crates/vt_bin/src/vtt/rm.rs | 30 +++++- .../fixtures/output_cache_test/snapshots.toml | 35 +++++++ ...globs___missing_archive_fails_cache_hit.md | 62 +++++++++++++ .../fixtures/remote_cache/snapshots.toml | 65 +++++++++++++ .../remote_cache/snapshots/restore_failure.md | 93 +++++++++++++++++++ 12 files changed, 371 insertions(+), 50 deletions(-) create mode 100644 crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md create mode 100644 crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 5aa6da994..bf952a5d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +- **Fixed** A cache hit whose output files can't be restored, for example because the cached archive was deleted or is corrupt, now fails the run with a non-zero exit code, and the run summary reports the task as failed with its error instead of as a cache hit. The entry is removed from the cache, so the next run executes the task, or restores it from the remote cache again, instead of failing the same way ([#770](https://github.com/voidzero-dev/vite-task/pull/770)). - **Changed** When a task isn't cached because it wrote a file it also read, `vp run --last-details` now says the task read and wrote the file, and shows the `cache: { input, output }` exclusions that let the task be cached ([#784](https://github.com/voidzero-dev/vite-task/pull/784)). - **Changed** The run summary now says a task that wrote a file it also read was `not cached because it modified its inputs`, and the statistics in `vp run --verbose` and `vp run --last-details` use the singular for a count of one, e.g. `1 task • 1 cache miss` ([#783](https://github.com/voidzero-dev/vite-task/pull/783)). - **Fixed** An invalid glob in `--filter` no longer shows its error message twice ([#763](https://github.com/voidzero-dev/vite-task/pull/763)). diff --git a/crates/vt/src/session/cache/mod.rs b/crates/vt/src/session/cache/mod.rs index fe3fcfdd4..d90276e5e 100644 --- a/crates/vt/src/session/cache/mod.rs +++ b/crates/vt/src/session/cache/mod.rs @@ -394,12 +394,12 @@ impl ExecutionCache { } // No cache found with the current cache entry key, - // check if execution key maps to a different cache entry key + // check if execution key maps to a different cache entry key. It + // still maps to the current key if that entry was removed. if let Some(old_cache_key) = self.get_cache_key_by_execution_key(execution_cache_key).await? + && old_cache_key != *cache_key { - // `get_by_cache_key` above returned None for the *current* cache key, - // so the associated key must differ. let mismatch = old_cache_key.into_mismatch(cache_key); return Ok(Err(CacheMiss::FingerprintMismatch(mismatch))); } @@ -519,6 +519,18 @@ impl ExecutionCache { } Ok(upload) } + + /// Remove the local entry for `cache_metadata` and its output archive at + /// `archive_path`, so later runs miss instead of hitting it. + pub async fn remove( + &self, + cache_metadata: &CacheMetadata, + archive_path: &AbsolutePath, + ) -> anyhow::Result<()> { + // Best-effort: the archive may already be missing. + let _ = std::fs::remove_file(archive_path.as_path()); + self.delete_cache_entry(&CacheEntryKey::from_metadata(cache_metadata)).await + } } // Basic database operations @@ -601,6 +613,18 @@ impl ExecutionCache { self.upsert("cache_entries", cache_key, cache_value).await } + #[expect( + clippy::significant_drop_tightening, + reason = "lock guard must be held while executing the prepared statement" + )] + async fn delete_cache_entry(&self, cache_key: &CacheEntryKey) -> anyhow::Result<()> { + let key_blob = serialize_cache(cache_key)?; + let conn = self.conn.lock().await; + let mut delete_stmt = conn.prepare_cached("DELETE FROM cache_entries WHERE key=?")?; + delete_stmt.execute([key_blob])?; + Ok(()) + } + async fn upsert_task_fingerprint( &self, execution_cache_key: &ExecutionCacheKey, diff --git a/crates/vt/src/session/event.rs b/crates/vt/src/session/event.rs index 7459e90b4..449da8620 100644 --- a/crates/vt/src/session/event.rs +++ b/crates/vt/src/session/event.rs @@ -10,6 +10,8 @@ use super::cache::{CacheHitSource, CacheMiss, remote::UploadError}; pub enum CacheErrorKind { /// Cache lookup (`try_hit`) failed. Lookup, + /// Restoring the output files of a cache hit failed. + Restore, /// Writing the cache entry failed after successful execution. Update, } @@ -18,6 +20,7 @@ impl std::fmt::Display for CacheErrorKind { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::Lookup => f.write_str("lookup"), + Self::Restore => f.write_str("restore"), Self::Update => f.write_str("update"), } } diff --git a/crates/vt/src/session/execute/mod.rs b/crates/vt/src/session/execute/mod.rs index bb8bad7f8..30fa4b1e4 100644 --- a/crates/vt/src/session/execute/mod.rs +++ b/crates/vt/src/session/execute/mod.rs @@ -345,17 +345,12 @@ impl Report { /// lookup failure, spawn failure, cache update failure) do not abort the /// caller. #[tracing::instrument(level = "debug", skip_all)] -#[expect( - clippy::too_many_arguments, - reason = "these are the unavoidable inputs for a free-function cache-aware spawn" -)] pub async fn execute_spawn( mut leaf_reporter: Box, spawn_execution: &SpawnExecution, cache: &ExecutionCache, workspace_root: &Arc, cache_dir: &AbsolutePath, - program_name: &str, fast_fail_token: CancellationToken, cancel_token: CancellationToken, ) -> SpawnOutcome { @@ -365,7 +360,6 @@ pub async fn execute_spawn( cache, workspace_root, cache_dir, - program_name, fast_fail_token, cancel_token, ); @@ -382,14 +376,12 @@ pub async fn execute_spawn( /// report, `Ok` is the report of a pipeline that ran to the end. The caller /// unwraps both into the same single `finish()`, so the distinction is pure /// control flow and a value on either side is equally valid. -#[expect(clippy::too_many_arguments, reason = "forwarded verbatim from `execute_spawn`")] async fn run( reporter: &mut dyn LeafExecutionReporter, spawn_execution: &SpawnExecution, cache: &ExecutionCache, workspace_root: &Arc, cache_dir: &AbsolutePath, - program_name: &str, fast_fail_token: CancellationToken, cancel_token: CancellationToken, ) -> Result { @@ -410,16 +402,18 @@ async fn run( // runs exactly once on every arm) and either replay the hit — no need // to execute the command — or carry the globbed inputs into the run. let (stdio_config, globbed_inputs) = match lookup { - CacheLookup::Hit(CacheHit { value: cached, source }) => { + CacheLookup::Hit { hit: CacheHit { value: cached, source }, metadata } => { let mut stdio_config = reporter.start(CacheStatus::Hit { replayed_duration: cached.duration, source }); return Ok(replay_cache_hit( &mut stdio_config, &cached, + cache, + metadata, workspace_root, cache_dir, - program_name, - )); + ) + .await); } CacheLookup::Miss { miss, globbed_inputs } => { (reporter.start(CacheStatus::Miss(miss)), globbed_inputs) @@ -525,9 +519,10 @@ async fn run( /// outcome provides: a hit owns the cached entry to replay, a miss keeps the /// reason plus the globbed inputs (reused by the cache-update phase after the /// run), and disabled has neither. -enum CacheLookup { - /// Cache hit — the cached entry to replay, and where it came from. - Hit(CacheHit), +enum CacheLookup<'a> { + /// Cache hit — the cached entry to replay, where it came from, and the + /// metadata that looked it up. + Hit { hit: CacheHit, metadata: &'a CacheMetadata }, /// Cache miss — the detailed reason (`NotFound` or `FingerprintMismatch`). Miss { miss: CacheMiss, globbed_inputs: BTreeMap }, /// Caching is disabled for this task (no cache metadata). @@ -537,13 +532,13 @@ enum CacheLookup { /// Phase 1: compute the globbed inputs and try to hit the cache. A remote hit /// downloads its output archive into `cache_dir`. Remote requests stop when /// `cancel_token` is cancelled. -async fn lookup_cache( - cache_metadata: Option<&CacheMetadata>, +async fn lookup_cache<'a>( + cache_metadata: Option<&'a CacheMetadata>, cache: &ExecutionCache, workspace_root: &Arc, cache_dir: &AbsolutePath, cancel_token: &CancellationToken, -) -> Result { +) -> Result, Report> { let Some(cache_metadata) = cache_metadata else { return Ok(CacheLookup::Disabled); }; @@ -563,7 +558,7 @@ async fn lookup_cache( .try_hit(cache_metadata, &globbed_inputs, workspace_root, cache_dir, cancel_token) .await { - Ok(Ok(cached)) => Ok(CacheLookup::Hit(cached)), + Ok(Ok(hit)) => Ok(CacheLookup::Hit { hit, metadata: cache_metadata }), Ok(Err(miss)) => Ok(CacheLookup::Miss { miss, globbed_inputs }), Err(err) => { Err(Report::failed(ExecutionError::Cache { kind: CacheErrorKind::Lookup, source: err })) @@ -573,12 +568,13 @@ async fn lookup_cache( /// Phase 3 (cache hit): replay the captured stdout/stderr and restore the /// output archive. -fn replay_cache_hit( +async fn replay_cache_hit( stdio_config: &mut StdioConfig, cached: &CacheEntryValue, + cache: &ExecutionCache, + cache_metadata: &CacheMetadata, workspace_root: &Arc, cache_dir: &AbsolutePath, - program_name: &str, ) -> Report { for output in cached.std_outputs.iter() { let writer: &mut dyn std::io::Write = match output.kind { @@ -590,21 +586,21 @@ fn replay_cache_hit( } // Restore output files from the cached archive. Failure here means the - // archive file is missing, truncated, or otherwise unreadable — the - // task can't proceed because the cache promised the outputs would be - // restored. Surface a recovery instruction rather than just the raw - // I/O error so users know to clear the cache. + // archive is missing or unreadable, or its files can't be written. The + // task fails because the cache promised the outputs would be restored, + // and the entry is removed so later runs miss instead of failing again. if let Some(ref archive_name) = cached.output_archive { let archive_path = cache_dir.join(archive_name.as_str()); if let Err(err) = archive::extract_output_archive(workspace_root, &archive_path) { - let err = err.context(vt_str::format!( - "failed to restore cached outputs from {}; the archive may have been deleted \ - or corrupted. Run `{program_name} cache clean` to clear the cache.", - archive_path.as_path().display() - )); + if let Err(err) = cache.remove(cache_metadata, &archive_path).await { + tracing::warn!(?err, "failed to remove a cache entry that couldn't be restored"); + } return Report::Failed { cache_update: CacheUpdateStatus::NotUpdated(CacheNotUpdatedReason::CacheHit), - error: ExecutionError::Cache { kind: CacheErrorKind::Lookup, source: err }, + error: ExecutionError::Cache { + kind: CacheErrorKind::Restore, + source: err.context("failed to extract the output archive"), + }, }; } } diff --git a/crates/vt/src/session/execute/scheduler.rs b/crates/vt/src/session/execute/scheduler.rs index 34180d62b..74ee4f35e 100644 --- a/crates/vt/src/session/execute/scheduler.rs +++ b/crates/vt/src/session/execute/scheduler.rs @@ -44,9 +44,6 @@ struct ExecutionContext<'a> { workspace_root: &'a Arc, /// Directory where cache files (db, archives) are stored. cache_dir: &'a AbsolutePath, - /// Public-facing program name (e.g. `vp`), used in user-facing error - /// messages that suggest a CLI command (e.g. `cache clean`). - program_name: &'a str, /// Token cancelled when a task fails. Kills in-flight child processes /// (via `start_kill` in spawn.rs). fast_fail_token: CancellationToken, @@ -206,7 +203,6 @@ impl ExecutionContext<'_> { self.cache, self.workspace_root, self.cache_dir, - self.program_name, self.fast_fail_token.clone(), self.cancel_token.clone(), ) @@ -262,7 +258,6 @@ impl Session<'_> { cache, workspace_root: &self.workspace_path, cache_dir: &self.cache_path, - program_name: self.program_name.as_str(), fast_fail_token, cancel_token, }; diff --git a/crates/vt/src/session/mod.rs b/crates/vt/src/session/mod.rs index 6495080c7..4cf0ad200 100644 --- a/crates/vt/src/session/mod.rs +++ b/crates/vt/src/session/mod.rs @@ -730,7 +730,6 @@ impl<'a> Session<'a> { cache, &self.workspace_path, &self.cache_path, - self.program_name.as_str(), fast_fail_token, cancel_token, ) diff --git a/crates/vt/src/session/reporter/summary.rs b/crates/vt/src/session/reporter/summary.rs index 36e89a05c..14936fc4f 100644 --- a/crates/vt/src/session/reporter/summary.rs +++ b/crates/vt/src/session/reporter/summary.rs @@ -73,6 +73,9 @@ pub enum TaskResult { source: CacheHitSource, }, + /// Cache hit whose output files couldn't be restored. Always a failure. + RestoreFailed { source: CacheHitSource, error: SavedError }, + /// In-process execution (built-in command like echo). Always successful. InProcess, @@ -90,8 +93,8 @@ pub enum TaskResult { /// - `Miss`: cache lookup found no match or a mismatch. /// - `Disabled`: no cache configuration for this task. /// -/// `Hit` and `InProcessExecution` are handled by [`TaskResult::CacheHit`] -/// and [`TaskResult::InProcess`] respectively. +/// `Hit` is handled by [`TaskResult::CacheHit`] or [`TaskResult::RestoreFailed`], +/// and `InProcessExecution` by [`TaskResult::InProcess`]. #[derive(Serialize, Deserialize)] pub enum SpawnedCacheStatus { Miss(SavedCacheMissReason), @@ -227,6 +230,7 @@ impl SummaryStats { } stats.total_saved += Duration::from_millis(*saved_duration_ms); } + TaskResult::RestoreFailed { .. } => stats.failed += 1, TaskResult::InProcess => { stats.cache_disabled += 1; } @@ -351,10 +355,14 @@ impl TaskResult { }; match cache_status { - CacheStatus::Hit { replayed_duration, source } => Self::CacheHit { - saved_duration_ms: duration_to_ms(*replayed_duration), - source: *source, - }, + // The only error a cache hit can have is a failed restore. + CacheStatus::Hit { replayed_duration, source } => saved_error.map_or_else( + || Self::CacheHit { + saved_duration_ms: duration_to_ms(*replayed_duration), + source: *source, + }, + |error| Self::RestoreFailed { source: *source, error: error.clone() }, + ), CacheStatus::Disabled(CacheDisabledReason::InProcessExecution) => Self::InProcess, CacheStatus::Disabled(CacheDisabledReason::NoCacheMetadata) => Self::Spawned { cache_status: SpawnedCacheStatus::Disabled, @@ -537,6 +545,7 @@ impl TaskResult { const fn is_success(&self) -> bool { match self { Self::CacheHit { .. } | Self::InProcess => true, + Self::RestoreFailed { .. } => false, Self::Spawned { outcome, .. } => matches!(outcome, SpawnOutcome::Success { .. }), } } @@ -548,6 +557,7 @@ impl TaskResult { /// Examples: /// - "→ Cache hit - output replayed - 102.96ms saved" /// - "→ Remote cache hit - output replayed - 102.96ms saved" + /// - "→ Cache hit, but the outputs couldn't be restored" /// - "→ Cache miss: no previous cache entry found" /// - "→ Cache disabled in task configuration" fn format_cache_detail(&self) -> (Str, &[Str]) { @@ -596,12 +606,12 @@ impl TaskResult { Self::CacheHit { saved_duration_ms, source } => { let d = Duration::from_millis(*saved_duration_ms); let formatted_duration = format_summary_duration(d); - let hit = match source { - CacheHitSource::Local => "Cache hit", - CacheHitSource::Remote => "Remote cache hit", - }; + let hit = format_hit(*source); vt_str::format!("→ {hit} - output replayed - {formatted_duration} saved") } + Self::RestoreFailed { source, .. } => { + vt_str::format!("→ {}, but the outputs couldn't be restored", format_hit(*source)) + } Self::InProcess => Str::from("→ Cache disabled for built-in command"), Self::Spawned { cache_status, .. } => match cache_status { SpawnedCacheStatus::Disabled => Str::from("→ Cache disabled in task configuration"), @@ -643,6 +653,7 @@ impl TaskResult { const fn cache_detail_style(&self) -> Style { match self { Self::CacheHit { .. } => Style::new().green(), + Self::RestoreFailed { .. } => Style::new().red(), Self::InProcess => Style::new().bright_black(), Self::Spawned { cache_status: SpawnedCacheStatus::Disabled, .. } => { Style::new().bright_black() @@ -685,6 +696,7 @@ impl TaskResult { pub const fn error(&self) -> Option<&SavedError> { match self { Self::CacheHit { .. } | Self::InProcess => None, + Self::RestoreFailed { error, .. } => Some(error), Self::Spawned { outcome, .. } => match outcome { SpawnOutcome::Success { infra_error, .. } => infra_error.as_ref(), SpawnOutcome::Failed { .. } => None, @@ -694,6 +706,14 @@ impl TaskResult { } } +/// "Cache hit" or "Remote cache hit", for the full summary's detail line. +const fn format_hit(source: CacheHitSource) -> &'static str { + match source { + CacheHitSource::Local => "Cache hit", + CacheHitSource::Remote => "Remote cache hit", + } +} + // ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ // Full summary rendering (--verbose and --last-details) // ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ diff --git a/crates/vt_bin/src/vtt/rm.rs b/crates/vt_bin/src/vtt/rm.rs index a8f98d6c0..52a623ccb 100644 --- a/crates/vt_bin/src/vtt/rm.rs +++ b/crates/vt_bin/src/vtt/rm.rs @@ -1,4 +1,17 @@ +/// Remove files and directories. +/// +/// Usage: `vtt rm [-rf] ...` or `vtt rm --ext ` +/// +/// With `--ext`, every file under ``, recursively, whose name ends with +/// `` is removed instead. Like `vtt list-dir --recursive`, this lets +/// tests remove cache archives without hardcoding their names or the +/// per-schema-version subdirectory they live under. pub fn run(args: &[String]) -> Result<(), Box> { + if let [flag, suffix, dir] = args + && flag == "--ext" + { + return remove_with_suffix(std::path::Path::new(dir), suffix); + } let mut recursive = false; let mut paths = Vec::new(); for arg in args { @@ -8,7 +21,7 @@ pub fn run(args: &[String]) -> Result<(), Box> { } } if paths.is_empty() { - return Err("Usage: vtt rm [-rf] ...".into()); + return Err("Usage: vtt rm [-rf] ... | vtt rm --ext ".into()); } for path in paths { let p = std::path::Path::new(path); @@ -20,3 +33,18 @@ pub fn run(args: &[String]) -> Result<(), Box> { } Ok(()) } + +fn remove_with_suffix( + dir: &std::path::Path, + suffix: &str, +) -> Result<(), Box> { + for entry in std::fs::read_dir(dir)? { + let entry = entry?; + if entry.file_type()?.is_dir() { + remove_with_suffix(&entry.path(), suffix)?; + } else if entry.file_name().to_string_lossy().ends_with(suffix) { + std::fs::remove_file(entry.path())?; + } + } + Ok(()) +} diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml index c72cc53f3..36c328869 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml @@ -32,6 +32,41 @@ steps = [ ], comment = "file restored from archive" }, ] +[[e2e]] +name = "output_globs___missing_archive_fails_cache_hit" +comment = """ +When the archive of a cache hit is missing, its output files can't be restored. The run fails, and the entry is removed from the cache, so the next run executes the task instead of hitting the entry again. +""" +steps = [ + { argv = [ + "vt", + "run", + "build", + ], comment = "first run — cache miss, writes the archive" }, + { argv = [ + "vtt", + "rm", + "--ext", + ".tar.zst", + "node_modules/.vite/task-cache", + ], comment = "delete the archive" }, + { argv = [ + "vt", + "run", + "build", + ], comment = "second run — cache hit, but restoring fails, so the run fails" }, + { argv = [ + "vt", + "run", + "--last-details", + ], comment = "the task is reported as failed, with its error" }, + { argv = [ + "vt", + "run", + "build", + ], comment = "third run — the entry was removed, so the task executes" }, +] + [[e2e]] name = "output_globs___old_archive_removed_on_rewrite" comment = """ diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md new file mode 100644 index 000000000..d02aee4ca --- /dev/null +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md @@ -0,0 +1,62 @@ +# output_globs___missing_archive_fails_cache_hit + +When the archive of a cache hit is missing, its output files can't be restored. The run fails, and the entry is removed from the cache, so the next run executes the task instead of hitting the entry again. + +## `vt run build` + +first run — cache miss, writes the archive + +``` +$ vtt write-file dist/output.txt built +``` + +## `vtt rm --ext .tar.zst node_modules/.vite/task-cache` + +delete the archive + +``` +``` + +## `vt run build` + +second run — cache hit, but restoring fails, so the run fails + +**Exit code:** 1 + +``` +$ vtt write-file dist/output.txt built ◉ cache hit, replaying +✗ Cache restore failed: failed to extract the output archive: +``` + +## `vt run --last-details` + +the task is reported as failed, with its error + +**Exit code:** 1 + +``` + +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + Vite+ Task Runner • Execution Summary +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + +Statistics: 1 task • 0 cache hits • 0 cache misses • 1 failed +Performance: 0% cache hit rate + +Task Details: +──────────────────────────────────────────────── + [1] output-cache-test#build: $ vtt write-file dist/output.txt built + → Cache hit, but the outputs couldn't be restored + ✗ Error: Cache restore failed + ↳ failed to extract the output archive + ↳ +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +``` + +## `vt run build` + +third run — the entry was removed, so the task executes + +``` +$ vtt write-file dist/output.txt built +``` diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml index ba61d0b67..9c2248369 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml @@ -251,6 +251,71 @@ steps = [ ], comment = "The corrupt download was removed." }, ] +[[e2e]] +name = "restore_failure" +cfg = "not(windows)" +ignore = true +steps = [ + { argv = [ + "remote-cache-server", + "vt", + "run", + "build", + ], envs = [ + [ + "VP_REMOTE_CACHE", + "read-write", + ], + ] }, + [ + "vt", + "cache", + "clean", + ], + [ + "vtt", + "rm", + "-rf", + "dist", + ], + { argv = [ + "vtt", + "write-file", + "dist", + "file", + ], comment = "A file where the output directory goes makes restoring fail." }, + { argv = [ + "remote-cache-server", + "vt", + "run", + "build", + ], comment = "The remote hit can't be restored, so the task fails." }, + { argv = [ + "vt", + "run", + "--last-details", + ], comment = "The details include the underlying error." }, + { argv = [ + "vtt", + "list-dir", + "node_modules/.vite/task-cache", + "--ext", + ".tar.zst", + "--recursive", + ], comment = "The downloaded archive was removed along with the entry." }, + [ + "vtt", + "rm", + "dist", + ], + { argv = [ + "remote-cache-server", + "vt", + "run", + "build", + ], comment = "The local entry was removed, so the remote entry is fetched and restored again." }, +] + [[e2e]] name = "invalid_endpoint" steps = [ diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md new file mode 100644 index 000000000..1f6576bb9 --- /dev/null +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md @@ -0,0 +1,93 @@ +# restore_failure + +## `VP_REMOTE_CACHE=read-write remote-cache-server vt run build` + +``` +$ vtt write-file dist/output.txt built + +[remote-cache] POST /fetch 404 +[remote-cache] POST /store 200 +``` + +## `vt cache clean` + +``` +``` + +## `vtt rm -rf dist` + +``` +``` + +## `vtt write-file dist file` + +A file where the output directory goes makes restoring fail. + +``` +``` + +## `remote-cache-server vt run build` + +The remote hit can't be restored, so the task fails. + +**Exit code:** 1 + +``` +$ vtt write-file dist/output.txt built ◉ remote cache hit, replaying +✗ Cache restore failed: failed to extract the output archive: failed to unpack `/dist/output.txt`: failed to unpack `dist/output.txt` into `/dist/output.txt`: + +[remote-cache] POST /fetch 200 exact +[remote-cache] GET /blob/1 200 +``` + +## `vt run --last-details` + +The details include the underlying error. + +**Exit code:** 1 + +``` + +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + Vite+ Task Runner • Execution Summary +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + +Statistics: 1 task • 0 cache hits • 0 cache misses • 1 failed +Performance: 0% cache hit rate + +Task Details: +──────────────────────────────────────────────── + [1] remote-cache#build: $ vtt write-file dist/output.txt built + → Remote cache hit, but the outputs couldn't be restored + ✗ Error: Cache restore failed + ↳ failed to extract the output archive + ↳ failed to unpack `/dist/output.txt` + ↳ failed to unpack `dist/output.txt` into `/dist/output.txt` + ↳ +━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +``` + +## `vtt list-dir node_modules/.vite/task-cache --ext .tar.zst --recursive` + +The downloaded archive was removed along with the entry. + +``` +``` + +## `vtt rm dist` + +``` +``` + +## `remote-cache-server vt run build` + +The local entry was removed, so the remote entry is fetched and restored again. + +``` +$ vtt write-file dist/output.txt built ◉ remote cache hit, replaying + +--- +vt run: remote cache hit. +[remote-cache] POST /fetch 200 exact +[remote-cache] GET /blob/1 200 +``` From 2e24d9e6cf528995eb6e382bd9360ed1b8290ba4 Mon Sep 17 00:00:00 2001 From: wan9chi Date: Fri, 2 Oct 2026 10:35:11 +0800 Subject: [PATCH 2/3] fix(cache): record remote hits only once their outputs are restored 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 --- CHANGELOG.md | 2 +- crates/vt/src/session/cache/mod.rs | 172 +++++++++++++++--- crates/vt/src/session/cache/remote.rs | 6 +- crates/vt/src/session/execute/mod.rs | 44 ++--- .../fixtures/remote_cache/snapshots.toml | 4 +- .../remote_cache/snapshots/restore_failure.md | 4 +- 6 files changed, 171 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf952a5d5..13d64cbbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -- **Fixed** A cache hit whose output files can't be restored, for example because the cached archive was deleted or is corrupt, now fails the run with a non-zero exit code, and the run summary reports the task as failed with its error instead of as a cache hit. The entry is removed from the cache, so the next run executes the task, or restores it from the remote cache again, instead of failing the same way ([#770](https://github.com/voidzero-dev/vite-task/pull/770)). +- **Fixed** A cache hit whose output files can't be restored, for example because the cached archive was deleted or is corrupt, now fails the run with a non-zero exit code, and the run summary reports the task as failed with its error instead of as a cache hit. The entry isn't kept in the local cache, so the next run executes the task, or restores it from the remote cache again, instead of failing the same way ([#770](https://github.com/voidzero-dev/vite-task/pull/770)). - **Changed** When a task isn't cached because it wrote a file it also read, `vp run --last-details` now says the task read and wrote the file, and shows the `cache: { input, output }` exclusions that let the task be cached ([#784](https://github.com/voidzero-dev/vite-task/pull/784)). - **Changed** The run summary now says a task that wrote a file it also read was `not cached because it modified its inputs`, and the statistics in `vp run --verbose` and `vp run --last-details` use the singular for a count of one, e.g. `1 task • 1 cache miss` ([#783](https://github.com/voidzero-dev/vite-task/pull/783)). - **Fixed** An invalid glob in `--filter` no longer shows its error message twice ([#763](https://github.com/voidzero-dev/vite-task/pull/763)). diff --git a/crates/vt/src/session/cache/mod.rs b/crates/vt/src/session/cache/mod.rs index d90276e5e..7c77d7e2c 100644 --- a/crates/vt/src/session/cache/mod.rs +++ b/crates/vt/src/session/cache/mod.rs @@ -13,7 +13,7 @@ pub use display::{ SpawnFingerprintChange, detect_spawn_fingerprint_changes, format_input_change_str, format_spawn_change, }; -use rusqlite::{Connection, OptionalExtension as _}; +use rusqlite::{Connection, OptionalExtension as _, TransactionBehavior}; use serde::{Deserialize, Serialize}; use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; @@ -127,7 +127,8 @@ pub enum CacheHitSource { /// The local cache. #[default] Local, - /// The remote cache. The entry has since been recorded locally. + /// The remote cache. The entry is recorded locally once its outputs are + /// restored. Remote, } @@ -315,10 +316,11 @@ impl ExecutionCache { /// Returns `Ok(Ok(cache_hit))` on cache hit, `Ok(Err(cache_miss))` on miss. /// /// After a local miss, the remote cache is queried if the task has one. A - /// remote hit is recorded locally, with its output archive downloaded into - /// `cache_dir`, and is never uploaded. If the local cache has an entry for - /// the task, its miss reason is kept. Otherwise the reason comes from the - /// remote cache. Remote requests stop when `cancel_token` is cancelled. + /// remote hit has its output archive downloaded into `cache_dir`, is + /// recorded locally by [`Self::restore`], and is never uploaded. If the + /// local cache has an entry for the task, its miss reason is kept. + /// Otherwise the reason comes from the remote cache. Remote requests stop + /// when `cancel_token` is cancelled. #[tracing::instrument(level = "debug", skip_all)] pub async fn try_hit( &self, @@ -408,10 +410,10 @@ impl ExecutionCache { } /// Fetch the entry from the remote cache at `endpoint`. An exact entry - /// that passes validation is a hit once its output archive is downloaded - /// and the entry is recorded locally. A fallback entry, a failed - /// validation, or a failed read is a miss. An error while validating - /// counts as a failed read, so the remote entry never fails the task. + /// that passes validation is a hit once its output archive is downloaded. + /// A fallback entry, a failed validation, or a failed read is a miss. An + /// error while validating counts as a failed read, so the remote entry + /// never fails the task. #[expect(clippy::too_many_arguments, reason = "forwarded from `try_hit`")] async fn try_hit_remote( &self, @@ -449,10 +451,7 @@ impl ExecutionCache { } None => None, }; - let cache_value = CacheEntryValue { output_archive, ..cache_value }; - self.record(cache_key, &cache_metadata.execution_cache_key, &cache_value, cache_dir) - .await?; - Ok(Ok(cache_value)) + Ok(Ok(CacheEntryValue { output_archive, ..cache_value })) } /// Record an entry locally. @@ -520,16 +519,68 @@ impl ExecutionCache { Ok(upload) } - /// Remove the local entry for `cache_metadata` and its output archive at - /// `archive_path`, so later runs miss instead of hitting it. - pub async fn remove( + /// Restore the output files of `hit` into `workspace_root`. + /// + /// A remote hit is recorded locally once its outputs are restored. If + /// restoring fails, the archive is removed, along with the local entry of + /// a local hit, so later runs miss instead of failing again. + pub async fn restore( &self, cache_metadata: &CacheMetadata, - archive_path: &AbsolutePath, + hit: &CacheHit, + workspace_root: &AbsolutePath, + cache_dir: &AbsolutePath, ) -> anyhow::Result<()> { - // Best-effort: the archive may already be missing. - let _ = std::fs::remove_file(archive_path.as_path()); - self.delete_cache_entry(&CacheEntryKey::from_metadata(cache_metadata)).await + let cache_key = CacheEntryKey::from_metadata(cache_metadata); + if let Some(archive_name) = &hit.value.output_archive { + let archive_path = cache_dir.join(archive_name.as_str()); + if let Err(err) = archive::extract_output_archive(workspace_root, &archive_path) { + match hit.source { + CacheHitSource::Local => { + if let Err(err) = self.evict(&cache_key, archive_name, cache_dir).await { + tracing::warn!( + ?err, + "failed to remove a cache entry that couldn't be restored" + ); + } + } + // Not recorded yet, so only the downloaded archive refers + // to it. Best-effort: the file may already be missing. + CacheHitSource::Remote => { + let _ = std::fs::remove_file(archive_path.as_path()); + } + } + return Err(err.context("failed to extract the output archive")); + } + } + + if hit.source == CacheHitSource::Remote + && let Err(err) = self + .record(&cache_key, &cache_metadata.execution_cache_key, &hit.value, cache_dir) + .await + { + // The outputs are restored, so the task still succeeds. The next + // run fetches the entry from the remote cache again. + tracing::warn!(?err, "failed to record a remote cache hit locally"); + } + Ok(()) + } + + /// Remove the entry for `cache_key` and its output archive + /// `archive_name`, unless the entry no longer refers to that archive. + /// Archive names are unique per recorded entry, so an entry that another + /// process recorded in the meantime is kept. + async fn evict( + &self, + cache_key: &CacheEntryKey, + archive_name: &str, + cache_dir: &AbsolutePath, + ) -> anyhow::Result<()> { + if self.delete_cache_entry_with_archive(cache_key, archive_name).await? { + // Best-effort: the archive may already be missing. + let _ = std::fs::remove_file(cache_dir.join(archive_name).as_path()); + } + Ok(()) } } @@ -613,16 +664,36 @@ impl ExecutionCache { self.upsert("cache_entries", cache_key, cache_value).await } + /// Delete the entry for `cache_key` if it still refers to the output + /// archive `archive_name`. Returns whether the entry was deleted. #[expect( clippy::significant_drop_tightening, - reason = "lock guard must be held while executing the prepared statement" + reason = "lock guard must be held for the whole transaction" )] - async fn delete_cache_entry(&self, cache_key: &CacheEntryKey) -> anyhow::Result<()> { + async fn delete_cache_entry_with_archive( + &self, + cache_key: &CacheEntryKey, + archive_name: &str, + ) -> anyhow::Result { let key_blob = serialize_cache(cache_key)?; - let conn = self.conn.lock().await; - let mut delete_stmt = conn.prepare_cached("DELETE FROM cache_entries WHERE key=?")?; - delete_stmt.execute([key_blob])?; - Ok(()) + let mut conn = self.conn.lock().await; + // Taking the write lock up front keeps other processes from replacing + // the entry between the read and the delete. + let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; + let value_blob: Option> = tx + .prepare_cached("SELECT value FROM cache_entries WHERE key=?")? + .query_row([&key_blob], |row| row.get(0)) + .optional()?; + let Some(value_blob) = value_blob else { + return Ok(false); + }; + let value: CacheEntryValue = deserialize_cache(&value_blob)?; + if value.output_archive.as_deref() != Some(archive_name) { + return Ok(false); + } + tx.prepare_cached("DELETE FROM cache_entries WHERE key=?")?.execute([&key_blob])?; + tx.commit()?; + Ok(true) } async fn upsert_task_fingerprint( @@ -744,4 +815,51 @@ mod tests { .unwrap(); assert_eq!(count_b, 0); } + + fn write_archive(dir: &AbsolutePath, name: &str) -> AbsolutePathBuf { + let path = dir.join(name); + std::fs::write(path.as_path(), b"archive").unwrap(); + path + } + + fn entry_with_archive(name: &str) -> CacheEntryValue { + CacheEntryValue { output_archive: Some(Str::from(name)), ..remote::tests::cache_value() } + } + + #[tokio::test] + async fn evict_removes_the_entry_and_its_archive() { + let (_tmp, dir) = temp_dir(); + let cache = ExecutionCache::load_from_path(&dir).unwrap(); + let key = remote::tests::cache_key(ResolvedGlobConfig::default_auto()); + let archive = write_archive(&dir, "stale.tar.zst"); + cache.upsert_cache_entry(&key, &entry_with_archive("stale.tar.zst")).await.unwrap(); + + cache.evict(&key, "stale.tar.zst", &dir).await.unwrap(); + + assert!(cache.get_by_cache_key(&key).await.unwrap().is_none()); + assert!(!archive.as_path().exists()); + } + + /// Another process can replace the entry between a failed restore and its + /// eviction. The replacement is kept, along with its archive. + #[tokio::test] + async fn evict_keeps_an_entry_replaced_by_another_process() { + let (_tmp, dir) = temp_dir(); + let cache = ExecutionCache::load_from_path(&dir).unwrap(); + let key = remote::tests::cache_key(ResolvedGlobConfig::default_auto()); + cache.upsert_cache_entry(&key, &entry_with_archive("stale.tar.zst")).await.unwrap(); + + let other_process = ExecutionCache::load_from_path(&dir).unwrap(); + let replacement = write_archive(&dir, "replacement.tar.zst"); + other_process + .upsert_cache_entry(&key, &entry_with_archive("replacement.tar.zst")) + .await + .unwrap(); + + cache.evict(&key, "stale.tar.zst", &dir).await.unwrap(); + + let kept = cache.get_by_cache_key(&key).await.unwrap().unwrap(); + assert_eq!(kept.output_archive.as_deref(), Some("replacement.tar.zst")); + assert!(replacement.as_path().exists()); + } } diff --git a/crates/vt/src/session/cache/remote.rs b/crates/vt/src/session/cache/remote.rs index 18008f28e..76477ea2a 100644 --- a/crates/vt/src/session/cache/remote.rs +++ b/crates/vt/src/session/cache/remote.rs @@ -335,7 +335,7 @@ fn decode_key(bytes: &[u8]) -> Result { } #[cfg(test)] -mod tests { +pub(super) mod tests { use std::{collections::BTreeMap, io::Read as _, net::TcpListener, time::Duration}; use tokio::sync::oneshot; @@ -355,7 +355,7 @@ mod tests { /// A key whose spawn fingerprint runs `vtt build`. `SpawnFingerprint`'s /// fields are private to `vt_plan`, so it's decoded from types with the /// same encoding. Update them if `SpawnFingerprint` changes. - fn cache_key(input_config: ResolvedGlobConfig) -> CacheEntryKey { + pub(in crate::session::cache) fn cache_key(input_config: ResolvedGlobConfig) -> CacheEntryKey { #[derive(SchemaWrite)] enum ProgramFingerprintLayout { OutsideWorkspace { program_name: Str }, @@ -387,7 +387,7 @@ mod tests { } } - fn cache_value() -> CacheEntryValue { + pub(in crate::session::cache) fn cache_value() -> CacheEntryValue { CacheEntryValue { post_run_fingerprint: PostRunFingerprint::default(), std_outputs: Arc::new([StdOutput { diff --git a/crates/vt/src/session/execute/mod.rs b/crates/vt/src/session/execute/mod.rs index 30fa4b1e4..1a42756e1 100644 --- a/crates/vt/src/session/execute/mod.rs +++ b/crates/vt/src/session/execute/mod.rs @@ -31,7 +31,7 @@ use self::{ spawn::{ChildHandle, ChildOutcome, SpawnStdio, spawn}, }; use super::{ - cache::{CacheEntryValue, CacheHit, CacheMiss, ExecutionCache, archive}, + cache::{CacheHit, CacheMiss, ExecutionCache}, event::{ CacheDisabledReason, CacheErrorKind, CacheNotUpdatedReason, CacheStatus, CacheUpdateStatus, ExecutionError, @@ -402,12 +402,14 @@ async fn run( // runs exactly once on every arm) and either replay the hit — no need // to execute the command — or carry the globbed inputs into the run. let (stdio_config, globbed_inputs) = match lookup { - CacheLookup::Hit { hit: CacheHit { value: cached, source }, metadata } => { - let mut stdio_config = - reporter.start(CacheStatus::Hit { replayed_duration: cached.duration, source }); + CacheLookup::Hit { hit, metadata } => { + let mut stdio_config = reporter.start(CacheStatus::Hit { + replayed_duration: hit.value.duration, + source: hit.source, + }); return Ok(replay_cache_hit( &mut stdio_config, - &cached, + &hit, cache, metadata, workspace_root, @@ -567,16 +569,16 @@ async fn lookup_cache<'a>( } /// Phase 3 (cache hit): replay the captured stdout/stderr and restore the -/// output archive. +/// output files. async fn replay_cache_hit( stdio_config: &mut StdioConfig, - cached: &CacheEntryValue, + hit: &CacheHit, cache: &ExecutionCache, cache_metadata: &CacheMetadata, workspace_root: &Arc, cache_dir: &AbsolutePath, ) -> Report { - for output in cached.std_outputs.iter() { + for output in hit.value.std_outputs.iter() { let writer: &mut dyn std::io::Write = match output.kind { pipe::OutputKind::StdOut => &mut stdio_config.writers.stdout_writer, pipe::OutputKind::StdErr => &mut stdio_config.writers.stderr_writer, @@ -585,24 +587,14 @@ async fn replay_cache_hit( let _ = writer.flush(); } - // Restore output files from the cached archive. Failure here means the - // archive is missing or unreadable, or its files can't be written. The - // task fails because the cache promised the outputs would be restored, - // and the entry is removed so later runs miss instead of failing again. - if let Some(ref archive_name) = cached.output_archive { - let archive_path = cache_dir.join(archive_name.as_str()); - if let Err(err) = archive::extract_output_archive(workspace_root, &archive_path) { - if let Err(err) = cache.remove(cache_metadata, &archive_path).await { - tracing::warn!(?err, "failed to remove a cache entry that couldn't be restored"); - } - return Report::Failed { - cache_update: CacheUpdateStatus::NotUpdated(CacheNotUpdatedReason::CacheHit), - error: ExecutionError::Cache { - kind: CacheErrorKind::Restore, - source: err.context("failed to extract the output archive"), - }, - }; - } + // Failure here means the archive is missing or unreadable, or its files + // can't be written. The task fails because the cache promised the + // outputs would be restored. + if let Err(err) = cache.restore(cache_metadata, hit, workspace_root, cache_dir).await { + return Report::Failed { + cache_update: CacheUpdateStatus::NotUpdated(CacheNotUpdatedReason::CacheHit), + error: ExecutionError::Cache { kind: CacheErrorKind::Restore, source: err }, + }; } Report::CacheHit diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml index 9c2248369..16045e01c 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots.toml @@ -302,7 +302,7 @@ steps = [ "--ext", ".tar.zst", "--recursive", - ], comment = "The downloaded archive was removed along with the entry." }, + ], comment = "The downloaded archive was removed, and the entry wasn't cached locally." }, [ "vtt", "rm", @@ -313,7 +313,7 @@ steps = [ "vt", "run", "build", - ], comment = "The local entry was removed, so the remote entry is fetched and restored again." }, + ], comment = "Nothing was cached locally, so the remote entry is fetched and restored again." }, ] [[e2e]] diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md index 1f6576bb9..3012d5dcf 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/restore_failure.md @@ -69,7 +69,7 @@ Task Details: ## `vtt list-dir node_modules/.vite/task-cache --ext .tar.zst --recursive` -The downloaded archive was removed along with the entry. +The downloaded archive was removed, and the entry wasn't cached locally. ``` ``` @@ -81,7 +81,7 @@ The downloaded archive was removed along with the entry. ## `remote-cache-server vt run build` -The local entry was removed, so the remote entry is fetched and restored again. +Nothing was cached locally, so the remote entry is fetched and restored again. ``` $ vtt write-file dist/output.txt built ◉ remote cache hit, replaying From ef581aebdf55468ea76cbfd868d22df3f588d838 Mon Sep 17 00:00:00 2001 From: wan9chi Date: Fri, 2 Oct 2026 11:32:37 +0800 Subject: [PATCH 3/3] fix(cache): suggest `vp cache clean` instead of evicting a local hit 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: `. 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 --- CHANGELOG.md | 3 +- crates/vt/src/session/cache/mod.rs | 141 +++--------------- crates/vt/src/session/cache/remote.rs | 6 +- crates/vt/src/session/event.rs | 14 +- crates/vt/src/session/execute/mod.rs | 23 ++- crates/vt/src/session/execute/scheduler.rs | 5 + crates/vt/src/session/mod.rs | 1 + .../fixtures/output_cache_test/snapshots.toml | 9 +- ...globs___missing_archive_fails_cache_hit.md | 15 +- 9 files changed, 80 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13d64cbbe..5a035665e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,10 @@ # Changelog -- **Fixed** A cache hit whose output files can't be restored, for example because the cached archive was deleted or is corrupt, now fails the run with a non-zero exit code, and the run summary reports the task as failed with its error instead of as a cache hit. The entry isn't kept in the local cache, so the next run executes the task, or restores it from the remote cache again, instead of failing the same way ([#770](https://github.com/voidzero-dev/vite-task/pull/770)). - **Changed** When a task isn't cached because it wrote a file it also read, `vp run --last-details` now says the task read and wrote the file, and shows the `cache: { input, output }` exclusions that let the task be cached ([#784](https://github.com/voidzero-dev/vite-task/pull/784)). - **Changed** The run summary now says a task that wrote a file it also read was `not cached because it modified its inputs`, and the statistics in `vp run --verbose` and `vp run --last-details` use the singular for a count of one, e.g. `1 task • 1 cache miss` ([#783](https://github.com/voidzero-dev/vite-task/pull/783)). - **Fixed** An invalid glob in `--filter` no longer shows its error message twice ([#763](https://github.com/voidzero-dev/vite-task/pull/763)). - **Changed** The detailed summary from `vp run --verbose` and `vp run --last-details` now shows each underlying cause of an error on its own line ([#761](https://github.com/voidzero-dev/vite-task/pull/761)). -- **Added** Remote caching. Configure an endpoint with the workspace's `cache: { remote: { url } }` or `VP_REMOTE_CACHE_URL`, and choose access with `--remote-cache=off|read|read-write` or `VP_REMOTE_CACHE`. The default is `read` with an endpoint and `off` without one. After a local cache miss, `vp run` looks the task up in the remote cache and, on a hit, restores its outputs and caches it locally. The task output and the run summary show which hits came from the remote cache. A failed read is just a cache miss, with the failure as its reason. In `read-write` mode, `vp run` also uploads the results of successful, cacheable tasks after caching them locally. A failed upload doesn't fail the task; the run summary shows a warning instead. Ctrl-C, or a failing task, stops remote cache requests right away, and a task still being looked up doesn't start. Tasks can opt out with `cache: { remote: false }`. Requests use the proxy environment variables or, on macOS and Windows, the system proxy settings ([#727](https://github.com/voidzero-dev/vite-task/pull/727), [#755](https://github.com/voidzero-dev/vite-task/pull/755), [#756](https://github.com/voidzero-dev/vite-task/pull/756), [#757](https://github.com/voidzero-dev/vite-task/pull/757), [#764](https://github.com/voidzero-dev/vite-task/pull/764), [#771](https://github.com/voidzero-dev/vite-task/pull/771), [#772](https://github.com/voidzero-dev/vite-task/pull/772), [#786](https://github.com/voidzero-dev/vite-task/pull/786)). +- **Added** Remote caching. Configure an endpoint with the workspace's `cache: { remote: { url } }` or `VP_REMOTE_CACHE_URL`, and choose access with `--remote-cache=off|read|read-write` or `VP_REMOTE_CACHE`. The default is `read` with an endpoint and `off` without one. After a local cache miss, `vp run` looks the task up in the remote cache and, on a hit, restores its outputs and caches it locally. The task output and the run summary show which hits came from the remote cache. A failed read is just a cache miss, with the failure as its reason. In `read-write` mode, `vp run` also uploads the results of successful, cacheable tasks after caching them locally. A failed upload doesn't fail the task; the run summary shows a warning instead. Ctrl-C, or a failing task, stops remote cache requests right away, and a task still being looked up doesn't start. Tasks can opt out with `cache: { remote: false }`. Requests use the proxy environment variables or, on macOS and Windows, the system proxy settings ([#727](https://github.com/voidzero-dev/vite-task/pull/727), [#755](https://github.com/voidzero-dev/vite-task/pull/755), [#756](https://github.com/voidzero-dev/vite-task/pull/756), [#757](https://github.com/voidzero-dev/vite-task/pull/757), [#764](https://github.com/voidzero-dev/vite-task/pull/764), [#770](https://github.com/voidzero-dev/vite-task/pull/770), [#771](https://github.com/voidzero-dev/vite-task/pull/771), [#772](https://github.com/voidzero-dev/vite-task/pull/772), [#786](https://github.com/voidzero-dev/vite-task/pull/786)). - **Fixed** On Windows, environment variable names used by `vp run` now match regardless of ASCII letter case. Assignments in task commands override earlier assignments and inherited variables spelled differently, and `FORCE_COLOR`, `VP_RUN_CONCURRENCY_LIMIT`, and variables requested through `@voidzero-dev/vite-task-client` are found under any spelling ([#747](https://github.com/voidzero-dev/vite-task/pull/747)). - **Changed** A task's cache settings now go inside `cache`, e.g. `cache: { env: ["NODE_ENV"], input: ["src/**"] }`; `cache: true` is the same as `cache: {}`. `env`, `untrackedEnv`, `input`, and `output` are no longer supported at the top level of a task ([#749](https://github.com/voidzero-dev/vite-task/pull/749)). - **Fixed** Cached tasks on macOS no longer intermittently fail with exit 2 and `oils I/O error (main): No such process` when a fast command finishes before the shell gets scheduled. The bundled shell that runs task commands is updated to Oils 0.38.0, which fixes this race ([#702](https://github.com/voidzero-dev/vite-task/issues/702), [#703](https://github.com/voidzero-dev/vite-task/pull/703)). diff --git a/crates/vt/src/session/cache/mod.rs b/crates/vt/src/session/cache/mod.rs index 7c77d7e2c..c93d22235 100644 --- a/crates/vt/src/session/cache/mod.rs +++ b/crates/vt/src/session/cache/mod.rs @@ -13,7 +13,7 @@ pub use display::{ SpawnFingerprintChange, detect_spawn_fingerprint_changes, format_input_change_str, format_spawn_change, }; -use rusqlite::{Connection, OptionalExtension as _, TransactionBehavior}; +use rusqlite::{Connection, OptionalExtension as _}; use serde::{Deserialize, Serialize}; use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; @@ -396,12 +396,12 @@ impl ExecutionCache { } // No cache found with the current cache entry key, - // check if execution key maps to a different cache entry key. It - // still maps to the current key if that entry was removed. + // check if execution key maps to a different cache entry key if let Some(old_cache_key) = self.get_cache_key_by_execution_key(execution_cache_key).await? - && old_cache_key != *cache_key { + // `get_by_cache_key` above returned None for the *current* cache key, + // so the associated key must differ. let mismatch = old_cache_key.into_mismatch(cache_key); return Ok(Err(CacheMiss::FingerprintMismatch(mismatch))); } @@ -522,8 +522,9 @@ impl ExecutionCache { /// Restore the output files of `hit` into `workspace_root`. /// /// A remote hit is recorded locally once its outputs are restored. If - /// restoring fails, the archive is removed, along with the local entry of - /// a local hit, so later runs miss instead of failing again. + /// they can't be, its downloaded archive is removed instead, so the next + /// run fetches it again. Returns an error if the outputs can't be + /// restored. pub async fn restore( &self, cache_metadata: &CacheMetadata, @@ -531,54 +532,27 @@ impl ExecutionCache { workspace_root: &AbsolutePath, cache_dir: &AbsolutePath, ) -> anyhow::Result<()> { - let cache_key = CacheEntryKey::from_metadata(cache_metadata); if let Some(archive_name) = &hit.value.output_archive { let archive_path = cache_dir.join(archive_name.as_str()); if let Err(err) = archive::extract_output_archive(workspace_root, &archive_path) { - match hit.source { - CacheHitSource::Local => { - if let Err(err) = self.evict(&cache_key, archive_name, cache_dir).await { - tracing::warn!( - ?err, - "failed to remove a cache entry that couldn't be restored" - ); - } - } - // Not recorded yet, so only the downloaded archive refers - // to it. Best-effort: the file may already be missing. - CacheHitSource::Remote => { - let _ = std::fs::remove_file(archive_path.as_path()); - } + if hit.source == CacheHitSource::Remote { + // Best-effort: the file may already be missing. + let _ = std::fs::remove_file(archive_path.as_path()); } return Err(err.context("failed to extract the output archive")); } } - if hit.source == CacheHitSource::Remote - && let Err(err) = self + if hit.source == CacheHitSource::Remote { + let cache_key = CacheEntryKey::from_metadata(cache_metadata); + if let Err(err) = self .record(&cache_key, &cache_metadata.execution_cache_key, &hit.value, cache_dir) .await - { - // The outputs are restored, so the task still succeeds. The next - // run fetches the entry from the remote cache again. - tracing::warn!(?err, "failed to record a remote cache hit locally"); - } - Ok(()) - } - - /// Remove the entry for `cache_key` and its output archive - /// `archive_name`, unless the entry no longer refers to that archive. - /// Archive names are unique per recorded entry, so an entry that another - /// process recorded in the meantime is kept. - async fn evict( - &self, - cache_key: &CacheEntryKey, - archive_name: &str, - cache_dir: &AbsolutePath, - ) -> anyhow::Result<()> { - if self.delete_cache_entry_with_archive(cache_key, archive_name).await? { - // Best-effort: the archive may already be missing. - let _ = std::fs::remove_file(cache_dir.join(archive_name).as_path()); + { + // The outputs are restored, so the task still succeeds. The + // next run fetches the entry from the remote cache again. + tracing::warn!(?err, "failed to record a remote cache hit locally"); + } } Ok(()) } @@ -664,38 +638,6 @@ impl ExecutionCache { self.upsert("cache_entries", cache_key, cache_value).await } - /// Delete the entry for `cache_key` if it still refers to the output - /// archive `archive_name`. Returns whether the entry was deleted. - #[expect( - clippy::significant_drop_tightening, - reason = "lock guard must be held for the whole transaction" - )] - async fn delete_cache_entry_with_archive( - &self, - cache_key: &CacheEntryKey, - archive_name: &str, - ) -> anyhow::Result { - let key_blob = serialize_cache(cache_key)?; - let mut conn = self.conn.lock().await; - // Taking the write lock up front keeps other processes from replacing - // the entry between the read and the delete. - let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; - let value_blob: Option> = tx - .prepare_cached("SELECT value FROM cache_entries WHERE key=?")? - .query_row([&key_blob], |row| row.get(0)) - .optional()?; - let Some(value_blob) = value_blob else { - return Ok(false); - }; - let value: CacheEntryValue = deserialize_cache(&value_blob)?; - if value.output_archive.as_deref() != Some(archive_name) { - return Ok(false); - } - tx.prepare_cached("DELETE FROM cache_entries WHERE key=?")?.execute([&key_blob])?; - tx.commit()?; - Ok(true) - } - async fn upsert_task_fingerprint( &self, execution_cache_key: &ExecutionCacheKey, @@ -815,51 +757,4 @@ mod tests { .unwrap(); assert_eq!(count_b, 0); } - - fn write_archive(dir: &AbsolutePath, name: &str) -> AbsolutePathBuf { - let path = dir.join(name); - std::fs::write(path.as_path(), b"archive").unwrap(); - path - } - - fn entry_with_archive(name: &str) -> CacheEntryValue { - CacheEntryValue { output_archive: Some(Str::from(name)), ..remote::tests::cache_value() } - } - - #[tokio::test] - async fn evict_removes_the_entry_and_its_archive() { - let (_tmp, dir) = temp_dir(); - let cache = ExecutionCache::load_from_path(&dir).unwrap(); - let key = remote::tests::cache_key(ResolvedGlobConfig::default_auto()); - let archive = write_archive(&dir, "stale.tar.zst"); - cache.upsert_cache_entry(&key, &entry_with_archive("stale.tar.zst")).await.unwrap(); - - cache.evict(&key, "stale.tar.zst", &dir).await.unwrap(); - - assert!(cache.get_by_cache_key(&key).await.unwrap().is_none()); - assert!(!archive.as_path().exists()); - } - - /// Another process can replace the entry between a failed restore and its - /// eviction. The replacement is kept, along with its archive. - #[tokio::test] - async fn evict_keeps_an_entry_replaced_by_another_process() { - let (_tmp, dir) = temp_dir(); - let cache = ExecutionCache::load_from_path(&dir).unwrap(); - let key = remote::tests::cache_key(ResolvedGlobConfig::default_auto()); - cache.upsert_cache_entry(&key, &entry_with_archive("stale.tar.zst")).await.unwrap(); - - let other_process = ExecutionCache::load_from_path(&dir).unwrap(); - let replacement = write_archive(&dir, "replacement.tar.zst"); - other_process - .upsert_cache_entry(&key, &entry_with_archive("replacement.tar.zst")) - .await - .unwrap(); - - cache.evict(&key, "stale.tar.zst", &dir).await.unwrap(); - - let kept = cache.get_by_cache_key(&key).await.unwrap().unwrap(); - assert_eq!(kept.output_archive.as_deref(), Some("replacement.tar.zst")); - assert!(replacement.as_path().exists()); - } } diff --git a/crates/vt/src/session/cache/remote.rs b/crates/vt/src/session/cache/remote.rs index 76477ea2a..18008f28e 100644 --- a/crates/vt/src/session/cache/remote.rs +++ b/crates/vt/src/session/cache/remote.rs @@ -335,7 +335,7 @@ fn decode_key(bytes: &[u8]) -> Result { } #[cfg(test)] -pub(super) mod tests { +mod tests { use std::{collections::BTreeMap, io::Read as _, net::TcpListener, time::Duration}; use tokio::sync::oneshot; @@ -355,7 +355,7 @@ pub(super) mod tests { /// A key whose spawn fingerprint runs `vtt build`. `SpawnFingerprint`'s /// fields are private to `vt_plan`, so it's decoded from types with the /// same encoding. Update them if `SpawnFingerprint` changes. - pub(in crate::session::cache) fn cache_key(input_config: ResolvedGlobConfig) -> CacheEntryKey { + fn cache_key(input_config: ResolvedGlobConfig) -> CacheEntryKey { #[derive(SchemaWrite)] enum ProgramFingerprintLayout { OutsideWorkspace { program_name: Str }, @@ -387,7 +387,7 @@ pub(super) mod tests { } } - pub(in crate::session::cache) fn cache_value() -> CacheEntryValue { + fn cache_value() -> CacheEntryValue { CacheEntryValue { post_run_fingerprint: PostRunFingerprint::default(), std_outputs: Arc::new([StdOutput { diff --git a/crates/vt/src/session/event.rs b/crates/vt/src/session/event.rs index 449da8620..c52300569 100644 --- a/crates/vt/src/session/event.rs +++ b/crates/vt/src/session/event.rs @@ -2,6 +2,7 @@ use std::{process::ExitStatus, time::Duration}; use vt_path::RelativePathBuf; use vt_server::Error as IpcServerError; +use vt_str::Str; use super::cache::{CacheHitSource, CacheMiss, remote::UploadError}; @@ -10,7 +11,8 @@ use super::cache::{CacheHitSource, CacheMiss, remote::UploadError}; pub enum CacheErrorKind { /// Cache lookup (`try_hit`) failed. Lookup, - /// Restoring the output files of a cache hit failed. + /// Restoring the output files of a remote cache hit failed. A local hit + /// fails with [`ExecutionError::LocalCacheRestore`] instead. Restore, /// Writing the cache entry failed after successful execution. Update, @@ -40,6 +42,16 @@ pub enum ExecutionError { source: anyhow::Error, }, + /// Restoring the output files of a local cache hit failed. The entry + /// stays in the cache, so later runs fail the same way until the cache is + /// cleared. + #[error("Cache restore failed. Run `{program_name} cache clean` to clear the cache")] + LocalCacheRestore { + program_name: Str, + #[source] + source: anyhow::Error, + }, + /// The OS failed to spawn the child process (e.g., command not found). #[error("Failed to spawn process")] Spawn(#[source] anyhow::Error), diff --git a/crates/vt/src/session/execute/mod.rs b/crates/vt/src/session/execute/mod.rs index 1a42756e1..0426f060f 100644 --- a/crates/vt/src/session/execute/mod.rs +++ b/crates/vt/src/session/execute/mod.rs @@ -31,7 +31,7 @@ use self::{ spawn::{ChildHandle, ChildOutcome, SpawnStdio, spawn}, }; use super::{ - cache::{CacheHit, CacheMiss, ExecutionCache}, + cache::{CacheHit, CacheHitSource, CacheMiss, ExecutionCache}, event::{ CacheDisabledReason, CacheErrorKind, CacheNotUpdatedReason, CacheStatus, CacheUpdateStatus, ExecutionError, @@ -345,12 +345,17 @@ impl Report { /// lookup failure, spawn failure, cache update failure) do not abort the /// caller. #[tracing::instrument(level = "debug", skip_all)] +#[expect( + clippy::too_many_arguments, + reason = "these are the unavoidable inputs for a free-function cache-aware spawn" +)] pub async fn execute_spawn( mut leaf_reporter: Box, spawn_execution: &SpawnExecution, cache: &ExecutionCache, workspace_root: &Arc, cache_dir: &AbsolutePath, + program_name: &str, fast_fail_token: CancellationToken, cancel_token: CancellationToken, ) -> SpawnOutcome { @@ -360,6 +365,7 @@ pub async fn execute_spawn( cache, workspace_root, cache_dir, + program_name, fast_fail_token, cancel_token, ); @@ -376,12 +382,14 @@ pub async fn execute_spawn( /// report, `Ok` is the report of a pipeline that ran to the end. The caller /// unwraps both into the same single `finish()`, so the distinction is pure /// control flow and a value on either side is equally valid. +#[expect(clippy::too_many_arguments, reason = "forwarded verbatim from `execute_spawn`")] async fn run( reporter: &mut dyn LeafExecutionReporter, spawn_execution: &SpawnExecution, cache: &ExecutionCache, workspace_root: &Arc, cache_dir: &AbsolutePath, + program_name: &str, fast_fail_token: CancellationToken, cancel_token: CancellationToken, ) -> Result { @@ -414,6 +422,7 @@ async fn run( metadata, workspace_root, cache_dir, + program_name, ) .await); } @@ -577,6 +586,7 @@ async fn replay_cache_hit( cache_metadata: &CacheMetadata, workspace_root: &Arc, cache_dir: &AbsolutePath, + program_name: &str, ) -> Report { for output in hit.value.std_outputs.iter() { let writer: &mut dyn std::io::Write = match output.kind { @@ -591,9 +601,18 @@ async fn replay_cache_hit( // can't be written. The task fails because the cache promised the // outputs would be restored. if let Err(err) = cache.restore(cache_metadata, hit, workspace_root, cache_dir).await { + let error = match hit.source { + CacheHitSource::Local => { + ExecutionError::LocalCacheRestore { program_name: program_name.into(), source: err } + } + // Not recorded locally, so there's no entry to clear. + CacheHitSource::Remote => { + ExecutionError::Cache { kind: CacheErrorKind::Restore, source: err } + } + }; return Report::Failed { cache_update: CacheUpdateStatus::NotUpdated(CacheNotUpdatedReason::CacheHit), - error: ExecutionError::Cache { kind: CacheErrorKind::Restore, source: err }, + error, }; } diff --git a/crates/vt/src/session/execute/scheduler.rs b/crates/vt/src/session/execute/scheduler.rs index 74ee4f35e..34180d62b 100644 --- a/crates/vt/src/session/execute/scheduler.rs +++ b/crates/vt/src/session/execute/scheduler.rs @@ -44,6 +44,9 @@ struct ExecutionContext<'a> { workspace_root: &'a Arc, /// Directory where cache files (db, archives) are stored. cache_dir: &'a AbsolutePath, + /// Public-facing program name (e.g. `vp`), used in user-facing error + /// messages that suggest a CLI command (e.g. `cache clean`). + program_name: &'a str, /// Token cancelled when a task fails. Kills in-flight child processes /// (via `start_kill` in spawn.rs). fast_fail_token: CancellationToken, @@ -203,6 +206,7 @@ impl ExecutionContext<'_> { self.cache, self.workspace_root, self.cache_dir, + self.program_name, self.fast_fail_token.clone(), self.cancel_token.clone(), ) @@ -258,6 +262,7 @@ impl Session<'_> { cache, workspace_root: &self.workspace_path, cache_dir: &self.cache_path, + program_name: self.program_name.as_str(), fast_fail_token, cancel_token, }; diff --git a/crates/vt/src/session/mod.rs b/crates/vt/src/session/mod.rs index 4cf0ad200..6495080c7 100644 --- a/crates/vt/src/session/mod.rs +++ b/crates/vt/src/session/mod.rs @@ -730,6 +730,7 @@ impl<'a> Session<'a> { cache, &self.workspace_path, &self.cache_path, + self.program_name.as_str(), fast_fail_token, cancel_token, ) diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml index 36c328869..8ba3fa56a 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots.toml @@ -35,7 +35,7 @@ steps = [ [[e2e]] name = "output_globs___missing_archive_fails_cache_hit" comment = """ -When the archive of a cache hit is missing, its output files can't be restored. The run fails, and the entry is removed from the cache, so the next run executes the task instead of hitting the entry again. +When the archive of a cache hit is missing, its output files can't be restored. The run fails and suggests clearing the cache, after which the task executes again. """ steps = [ { argv = [ @@ -60,11 +60,16 @@ steps = [ "run", "--last-details", ], comment = "the task is reported as failed, with its error" }, + { argv = [ + "vt", + "cache", + "clean", + ], comment = "clear the cache, as the error suggests" }, { argv = [ "vt", "run", "build", - ], comment = "third run — the entry was removed, so the task executes" }, + ], comment = "third run — cache miss, so the task executes" }, ] [[e2e]] diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md index d02aee4ca..dc13f8766 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md +++ b/crates/vt_bin/tests/e2e_snapshots/fixtures/output_cache_test/snapshots/output_globs___missing_archive_fails_cache_hit.md @@ -1,6 +1,6 @@ # output_globs___missing_archive_fails_cache_hit -When the archive of a cache hit is missing, its output files can't be restored. The run fails, and the entry is removed from the cache, so the next run executes the task instead of hitting the entry again. +When the archive of a cache hit is missing, its output files can't be restored. The run fails and suggests clearing the cache, after which the task executes again. ## `vt run build` @@ -25,7 +25,7 @@ second run — cache hit, but restoring fails, so the run fails ``` $ vtt write-file dist/output.txt built ◉ cache hit, replaying -✗ Cache restore failed: failed to extract the output archive: +✗ Cache restore failed. Run `vt cache clean` to clear the cache: failed to extract the output archive: ``` ## `vt run --last-details` @@ -47,15 +47,22 @@ Task Details: ──────────────────────────────────────────────── [1] output-cache-test#build: $ vtt write-file dist/output.txt built → Cache hit, but the outputs couldn't be restored - ✗ Error: Cache restore failed + ✗ Error: Cache restore failed. Run `vt cache clean` to clear the cache ↳ failed to extract the output archive ↳ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ``` +## `vt cache clean` + +clear the cache, as the error suggests + +``` +``` + ## `vt run build` -third run — the entry was removed, so the task executes +third run — cache miss, so the task executes ``` $ vtt write-file dist/output.txt built