fix(cache): harden remote cache requests and restores

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 - **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. Tasks can opt out with `cache: { remote: false }` ([#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)).
+- **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. 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)).
 - **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)).

crates/vt/src/session/cache/remote.rs

@@ -58,6 +58,10 @@ pub enum ReadError {
     CorruptValue(#[source] wincode::error::ReadError),
     #[error("remote cache key is corrupt")]
     CorruptKey(#[source] Option<wincode::error::ReadError>),
+    /// The value has an output archive but the entry has no blob, or the
+    /// other way around.
+    #[error("remote cache entry's blob doesn't match its value")]
+    MismatchedBlob,
     #[error("downloaded archive is corrupt")]
     CorruptArchive(#[source] io::Error),
     #[error("failed to write the downloaded archive")]
@@ -85,7 +89,8 @@ pub(super) struct Restore {
 /// Turn the result of a fetch into an entry to restore or a miss. `validate`
 /// checks an exact entry against the current execution. A fallback's miss
 /// reason compares its key with `cache_key`. A failed fetch, an entry that
-/// doesn't decode, or a validation error is a read failure.
+/// doesn't decode or whose blob doesn't match its value, or a validation error
+/// is a read failure.
 #[expect(
     clippy::result_large_err,
     reason = "`CacheMiss` is intentionally large, and a lookup returns it once"
@@ -97,8 +102,11 @@ pub(super) fn resolve(
 ) -> Result<Restore, CacheMiss> {
     let (value, blob_id) = match fetched.map_err(ReadError::into_miss)? {
         Fetched::Exact { value, blob_id } => {
-            let value = deserialize_cache(&value)
+            let value: CacheEntryValue = deserialize_cache(&value)
                 .map_err(|err| ReadError::CorruptValue(err).into_miss())?;
+            if value.output_archive.is_some() != blob_id.is_some() {
+                return Err(ReadError::MismatchedBlob.into_miss());
+            }
             (value, blob_id)
         }
         Fetched::Fallback { key } => {
@@ -222,13 +230,18 @@ async fn download_checked(
 }
 
 /// Send the blob's chunks through `sender` until the blob ends or the receiver
-/// is dropped.
+/// is dropped. The check drops the receiver when it fails, which stops the
+/// download without waiting for the next chunk.
 async fn send_chunks(
     mut download: Download,
     sender: mpsc::Sender<Bytes>,
 ) -> Result<(), vt_remote_cache::Error> {
-    while let Some(chunk) = download.chunk().await? {
-        // The check drops the receiver when it fails.
+    loop {
+        let chunk = tokio::select! {
+            chunk = download.chunk() => chunk?,
+            () = sender.closed() => break,
+        };
+        let Some(chunk) = chunk else { break };
         if sender.send(chunk).await.is_err() {
             break;
         }
@@ -298,10 +311,10 @@ fn decode_key(bytes: &[u8]) -> Result<CacheEntryKey, ReadError> {
 
 #[cfg(test)]
 mod tests {
-    use std::{collections::BTreeMap, time::Duration};
+    use std::{collections::BTreeMap, io::Read as _, net::TcpListener, time::Duration};
 
     use vt_graph::config::ResolvedGlobConfig;
-    use vt_path::RelativePathBuf;
+    use vt_path::{AbsolutePathBuf, RelativePathBuf};
     use vt_plan::cache_metadata::{EnvValueHash, SpawnFingerprint};
 
     use super::*;
@@ -377,7 +390,7 @@ mod tests {
     }
 
     fn not_validated(_: &CacheEntryValue) -> anyhow::Result<Option<FingerprintMismatch>> {
-        panic!("only exact entries that decode are validated")
+        panic!("only exact entries that decode and match their blob are validated")
     }
 
     fn read_failure(miss: CacheMiss) -> Str {
@@ -479,6 +492,17 @@ mod tests {
         }
     }
 
+    #[test]
+    fn blob_that_does_not_match_the_value_is_a_read_failure() {
+        let key = cache_key(ResolvedGlobConfig::default_auto());
+        let without_archive = CacheEntryValue { output_archive: None, ..cache_value() };
+        for (value, blob_id) in [(cache_value(), None), (without_archive, Some(Str::from("1")))] {
+            let fetched = Ok(Fetched::Exact { value: serialize_cache(&value).unwrap(), blob_id });
+            let miss = resolve(fetched, &key, not_validated).unwrap_err();
+            assert_eq!(read_failure(miss), "remote cache entry's blob doesn't match its value");
+        }
+    }
+
     #[test]
     fn fallback_key_that_does_not_decode_is_a_corrupt_entry() {
         let key = cache_key(ResolvedGlobConfig::default_auto());
@@ -497,4 +521,39 @@ mod tests {
             assert_eq!(read_failure(miss), "remote cache key is corrupt");
         }
     }
+
+    #[tokio::test]
+    async fn failed_archive_check_stops_the_download() {
+        let listener = TcpListener::bind("127.0.0.1:0").unwrap();
+        let endpoint: Arc<str> =
+            Arc::from(vt_str::format!("http://{}/projects/test", listener.local_addr().unwrap()));
+        let (done_sender, done_receiver) = std::sync::mpsc::channel::<()>();
+        // The response announces more than it sends and stays open until the
+        // test is done, so only the failed check can end the download.
+        let server = std::thread::spawn(move || {
+            let (mut stream, _) = listener.accept().unwrap();
+            let mut request = Vec::new();
+            while !request.ends_with(b"\r\n\r\n") {
+                let mut buf = [0; 1024];
+                let n = stream.read(&mut buf).unwrap();
+                assert_ne!(n, 0, "connection closed before the request ended");
+                request.extend_from_slice(&buf[..n]);
+            }
+            stream
+                .write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 1000\r\n\r\nnot an archive")
+                .unwrap();
+            let _ = done_receiver.recv();
+        });
+        let dir = tempfile::tempdir().unwrap();
+        let cache_dir = AbsolutePathBuf::new(dir.path().to_path_buf()).unwrap();
+
+        let error = RemoteClients::default()
+            .download_archive(&endpoint, "1", &cache_dir)
+            .await
+            .unwrap_err();
+        assert!(matches!(error, ReadError::CorruptArchive(_)), "{error:?}");
+        assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 0);
+        drop(done_sender);
+        server.join().unwrap();
+    }
 }

crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/read_unreachable_endpoint.md

@@ -26,7 +26,7 @@ Task Details:
   [1] remote-cache#build: $ vtt write-file dist/output.txt built ✓
       → Cache miss: remote cache fetch failed
         ↳ network error
-        ↳ error sending request for url (http://127.0.0.1:0/projects/test/fetch)
+        ↳ error sending request
         ↳ client error (Connect)
         ↳ tcp connect error
         ↳ <os error>

crates/vt_bin/tests/e2e_snapshots/fixtures/remote_cache/snapshots/unreachable_endpoint.md

@@ -29,12 +29,12 @@ Task Details:
   [1] remote-cache#build: $ vtt write-file dist/output.txt built ✓
       → Cache miss: remote cache fetch failed
         ↳ network error
-        ↳ error sending request for url (http://127.0.0.1:0/projects/test/fetch)
+        ↳ error sending request
         ↳ client error (Connect)
         ↳ tcp connect error
         ↳ <os error>
       ⚠ Not uploaded to the remote cache: network error
-        ↳ error sending request for url (http://127.0.0.1:0/projects/test/store)
+        ↳ error sending request
         ↳ client error (Connect)
         ↳ tcp connect error
         ↳ <os error>

crates/vt_remote_cache/Cargo.toml

@@ -10,7 +10,12 @@ rust-version.workspace = true
 [dependencies]
 bytes = { workspace = true }
 ciborium = { workspace = true }
-reqwest = { workspace = true, features = ["multipart", "rustls-no-provider", "stream"] }
+reqwest = { workspace = true, features = [
+  "multipart",
+  "rustls-no-provider",
+  "stream",
+  "system-proxy",
+] }
 rustls = { workspace = true, features = ["ring", "std"] }
 serde = { workspace = true, features = ["derive"] }
 serde_bytes = { workspace = true }

crates/vt_remote_cache/README.md

@@ -10,8 +10,8 @@ Client for the [remote cache server API](https://github.com/voidzero-dev/vite-ta
 
 `Client::store` sends a multipart request: a CBOR `metadata` part with the key, secondary key, and value as byte strings, and an optional `blob` part streamed from a file. The response body isn't decoded.
 
-Only HTTP 200 counts as success for every operation.
+Only HTTP 200 counts as success for every operation. Redirects aren't followed, so a redirect fails like any other status.
 
-reqwest configures TLS. It uses the process's default rustls crypto provider, which the client installs as ring unless one is already installed, and verifies certificates with the operating system's verifier. Connections time out after 10 seconds. Reads time out after 60 seconds, and until the response headers arrive, that limit also covers sending the request.
+reqwest configures TLS. It uses the process's default rustls crypto provider, which the client installs as ring unless one is already installed, and verifies certificates with the operating system's verifier. Requests go through the proxy set in `HTTPS_PROXY`, `HTTP_PROXY`, or `ALL_PROXY`, except for hosts in `NO_PROXY`. Without those variables, the system proxy settings are used on macOS and Windows. Connections time out after 10 seconds. Reads time out after 60 seconds, and until the response headers arrive, that limit also covers sending the request.
 
-`Error` names the kind of failure: an invalid endpoint, a client that couldn't be created, a blob file that couldn't be read, a network error (including timeouts and responses that end early), a status other than 200, or a malformed fetch response. Its messages contain no OS-specific details, so they can be shown to users as is. The details are in the source: the underlying error, the parse error for an endpoint that isn't a URL, or the message in an error response's body.
+`Error` names the kind of failure: an invalid endpoint, a client that couldn't be created, a blob file that couldn't be read, a network error (including timeouts and responses that end early), a status other than 200, or a malformed fetch response. Its messages contain no OS-specific details, so they can be shown to users as is. The details are in the source: the underlying error, the parse error for an endpoint that isn't a URL, or the message in an error response's body. Network errors leave out the request URL, since the endpoint may contain credentials, such as a token in its query.