fix: Don't persist cache files that error while writing (#1579)

This adds an early return in memory.rs so that if an InternalError is returned by a cache computation¹, we don't attempt to write the error and don't persist the temp file. This kills two birds with one stone:

1. It fixes the "Internal errors should never be written out" issue we see in Sentry.
2. More importantly, if a computation fails with an InternalError because of lack of disk space, we discard the resulting file immediately instead of preserving its (likely useless) contents.

Moreover, I also added some extra validation after symcache/ppdbcache/sourcemapcache conversion. Now, if somehow the write goes through but the file is invalid, we also return an InternalError and it will be deleted. This costs us nothing because parsing these files is essentially free.

crates/symbolicator-js/src/sourcemap_cache.rs

@@ -104,6 +104,13 @@ fn write_sourcemap_cache(file: &mut File, source: &str, sourcemap: &str) -> Cach
     let file = writer.into_inner().map_err(io::Error::from)?;
     file.sync_all()?;
 
+    // Parse the sourcemapcache file to verify integrity
+    let bv = ByteView::map_file_ref(file)?;
+    if SourceMapCache::parse(&bv).is_err() {
+        tracing::error!("Failed to verify integrity of freshly written SourceMapCache");
+        return Err(CacheError::InternalError);
+    }
+
     Ok(())
 }
 

crates/symbolicator-native/src/caches/ppdb_caches.rs

@@ -149,5 +149,12 @@ fn write_ppdb_cache(file: &mut File, object_handle: &ObjectHandle) -> CacheEntry
     let file = writer.into_inner().map_err(io::Error::from)?;
     file.sync_all()?;
 
+    // Parse the ppdbcache file to verify integrity
+    let bv = ByteView::map_file_ref(file)?;
+    if PortablePdbCache::parse(&bv).is_err() {
+        tracing::error!("Failed to verify integrity of freshly written PortablePDB Cache");
+        return Err(CacheError::InternalError);
+    }
+
     Ok(())
 }

crates/symbolicator-native/src/caches/symcaches.rs

@@ -271,6 +271,13 @@ fn write_symcache(
     let file = writer.into_inner().map_err(io::Error::from)?;
     file.sync_all()?;
 
+    // Parse the symcache file to verify integrity
+    let bv = ByteView::map_file_ref(file)?;
+    if SymCache::parse(&bv).is_err() {
+        tracing::error!("Failed to verify integrity of freshly written SymCache");
+        return Err(CacheError::InternalError);
+    }
+
     Ok(())
 }
 

crates/symbolicator-service/src/caching/memory.rs

@@ -212,6 +212,12 @@ impl<T: CacheItemRequest> Cacher<T> {
                     let byte_view = ByteView::map_file_ref(temp_file.as_file())?;
                     entry = Ok(byte_view);
                 }
+                Err(CacheError::InternalError) => {
+                    // If there was an `InternalError` during computation (for instance because
+                    // of an io error), we return immediately without writing the error
+                    // or persisting the temp file.
+                    return Err(CacheError::InternalError);
+                }
                 Err(err) => {
                     let mut temp_fd = tokio::fs::File::from_std(temp_file.reopen()?);
                     err.write(&mut temp_fd).await?;

crates/symbolicator-service/src/caching/tests.rs

@@ -1,5 +1,5 @@
 use std::fs::{self, File};
-use std::io::Write;
+use std::io::{self, Write};
 use std::path::Path;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::thread::sleep;
@@ -917,3 +917,79 @@ async fn test_lazy_computation_limit() {
 
     assert_eq!(num_outdated, 2);
 }
+
+/// A request to compute a cache item that always fails.
+#[derive(Clone)]
+struct FailingTestCacheItem(CacheError);
+
+impl CacheItemRequest for FailingTestCacheItem {
+    type Item = String;
+
+    const VERSIONS: CacheVersions = CacheVersions {
+        current: 1,
+        fallbacks: &[],
+    };
+
+    fn compute<'a>(&'a self, temp_file: &'a mut NamedTempFile) -> BoxFuture<'a, CacheEntry> {
+        Box::pin(async move {
+            fs::write(temp_file.path(), "garbage data")?;
+            Err(self.0.clone())
+        })
+    }
+
+    fn load(&self, data: ByteView<'static>) -> CacheEntry<Self::Item> {
+        Ok(std::str::from_utf8(data.as_slice()).unwrap().to_owned())
+    }
+}
+
+/// Verifies that an internal error during computation results in the temporary
+/// file being discarded instead of persisted.
+#[tokio::test]
+async fn test_failing_cache_write() {
+    test::setup();
+    let cache_dir = test::tempdir();
+
+    let config = Config {
+        cache_dir: Some(cache_dir.path().to_path_buf()),
+        ..Default::default()
+    };
+    let cache = Cache::from_config(
+        CacheName::Objects,
+        &config,
+        CacheConfig::from(CacheConfigs::default().derived),
+        Arc::new(AtomicIsize::new(1)),
+        1024,
+    )
+    .unwrap();
+    let cacher = Cacher::new(cache, Default::default());
+
+    // Case 1: internal error
+    let request = FailingTestCacheItem(CacheError::InternalError);
+    let key = CacheKey::for_testing("global/internal_error");
+
+    let entry = cacher
+        .compute_memoized(request, key.clone())
+        .await
+        .unwrap_err();
+    assert_eq!(entry, CacheError::InternalError);
+
+    // The computation returned `InternalError`, so the file should not have been
+    // persisted
+    let cache_file_path = cache_dir.path().join("objects").join(key.cache_path(1));
+    assert!(!fs::exists(cache_file_path).unwrap());
+
+    // Case 2: malformed error
+    let request = FailingTestCacheItem(CacheError::Malformed("this is garbage".to_owned()));
+    let key = CacheKey::for_testing("global/malformed");
+
+    let entry = cacher
+        .compute_memoized(request, key.clone())
+        .await
+        .unwrap_err();
+    assert_eq!(entry, CacheError::Malformed("this is garbage".to_owned()));
+
+    // The computation returned `Malformed`, so the file should have been
+    // persisted
+    let cache_file_path = cache_dir.path().join("objects").join(key.cache_path(1));
+    assert!(fs::exists(cache_file_path).unwrap());
+}