feat(cache): Write temporary files in a dedicated directory

src/actors/common/cache.rs

@@ -1,6 +1,5 @@
 use std::collections::BTreeMap;
 use std::fmt;
-use std::fs;
 use std::io;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
@@ -42,6 +41,12 @@ pub struct Cacher<T: CacheItemRequest> {
     current_computations: ComputationMap<T::Item, T::Error>,
 }
 
+impl<T: CacheItemRequest> Cacher<T> {
+    pub fn tempfile(&self) -> io::Result<NamedTempFile> {
+        self.config.tempfile()
+    }
+}
+
 impl<T: CacheItemRequest> Clone for Cacher<T> {
     fn clone(&self) -> Self {
         // https://github.com/rust-lang/rust/issues/26925
@@ -202,13 +207,7 @@ impl<T: CacheItemRequest> Cacher<T> {
         // just got pruned.
         metric!(counter(&format!("caches.{}.file.miss", name)) += 1);
 
-        let temp_file = if let Some(ref path) = cache_path {
-            let dir = path.parent().unwrap();
-            tryf!(fs::create_dir_all(dir));
-            tryf!(NamedTempFile::new_in(dir))
-        } else {
-            tryf!(NamedTempFile::new())
-        };
+        let temp_file = tryf!(self.tempfile());
 
         let future = request.compute(temp_file.path()).and_then(move |status| {
             if let Some(ref cache_path) = cache_path {

src/actors/objects/mod.rs

@@ -14,7 +14,7 @@ use futures::future::{FutureExt, TryFutureExt};
 use futures01::{future, Future};
 use symbolic::common::ByteView;
 use symbolic::debuginfo::{Archive, Object};
-use tempfile::{tempfile_in, NamedTempFile};
+use tempfile::tempfile_in;
 
 use crate::actors::common::cache::{CacheItemRequest, CachePath, Cacher};
 use crate::cache::{Cache, CacheKey, CacheStatus};
@@ -177,9 +177,12 @@ impl CacheItemRequest for FetchFileDataRequest {
             self.0.file_id.write_sentry_scope(scope);
         });
 
-        let download_dir = tryf!(path.parent().ok_or(ObjectErrorKind::NoTempDir)).to_owned();
-        let download_file =
-            tryf!(NamedTempFile::new_in(&download_dir).context(ObjectErrorKind::Io));
+        let download_file = tryf!(self.0.data_cache.tempfile().context(ObjectErrorKind::Io));
+        let download_dir = tryf!(download_file
+            .path()
+            .parent()
+            .ok_or(ObjectErrorKind::NoTempDir))
+        .to_owned();
         let request = self
             .0
             .download_svc

src/actors/symbolication.rs

@@ -1,6 +1,6 @@
 use std::collections::{BTreeMap, BTreeSet};
 use std::convert::TryInto;
-use std::io::Write;
+use std::io::{self, Write};
 use std::iter::FromIterator;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -1220,12 +1220,15 @@ impl SymbolicationActor {
     fn save_minidump(
         minidump: Bytes,
         failed_cache: crate::cache::Cache,
-    ) -> failure::Fallible<Option<PathBuf>> {
+    ) -> io::Result<Option<PathBuf>> {
         if let Some(dir) = failed_cache.cache_dir() {
             std::fs::create_dir_all(dir)?;
-            let tmp = tempfile::NamedTempFile::new_in(dir)?;
+            let tmp = tempfile::Builder::new()
+                .prefix("minidump")
+                .suffix(".dmp")
+                .tempfile_in(dir)?;
             tmp.as_file().write_all(&*minidump)?;
-            let (_file, path) = tmp.keep()?;
+            let (_file, path) = tmp.keep().map_err(|e| e.error)?;
             Ok(Some(path))
         } else {
             log::debug!("No diagnostics retention configured, not saving minidump");

src/cache.rs

@@ -96,8 +96,19 @@ pub struct Cache {
     name: &'static str,
 
     /// Directory to use for storing cache items. Will be created if it does not exist.
+    ///
+    /// Leaving this as None will disable this cache.
     cache_dir: Option<PathBuf>,
 
+    /// Directory to use for temporary files.
+    ///
+    /// When writing a new file into the cache it is best to write it to a temporary file in
+    /// a sibling directory, once fully written it can then be atomically moved to the
+    /// actual location withing the `cache_dir`.
+    ///
+    /// Just like for `cache_dir` when this cache is disabled this will be `None`.
+    tmp_dir: Option<PathBuf>,
+
     /// Time when this process started.
     start_time: SystemTime,
 
@@ -109,6 +120,7 @@ impl Cache {
     pub fn from_config(
         name: &'static str,
         cache_dir: Option<PathBuf>,
+        tmp_dir: Option<PathBuf>,
         cache_config: CacheConfig,
     ) -> io::Result<Self> {
         if let Some(ref dir) = cache_dir {
@@ -117,6 +129,7 @@ impl Cache {
         Ok(Cache {
             name,
             cache_dir,
+            tmp_dir,
             start_time: SystemTime::now(),
             cache_config,
         })
@@ -278,6 +291,17 @@ impl Cache {
             ByteView::open(path)
         })
     }
+
+    /// Create a new temporary file to use in the cache.
+    pub fn tempfile(&self) -> io::Result<NamedTempFile> {
+        match self.tmp_dir {
+            Some(ref path) => {
+                std::fs::create_dir_all(path)?;
+                Ok(tempfile::Builder::new().prefix("tmp").tempfile_in(path)?)
+            }
+            None => Ok(NamedTempFile::new()?),
+        }
+    }
 }
 
 #[derive(Debug, Clone, Eq, Ord, PartialEq, PartialOrd)]
@@ -323,26 +347,58 @@ pub struct Caches {
 
 impl Caches {
     pub fn from_config(config: &Config) -> io::Result<Self> {
+        let tmp_dir = config.cache_dir("tmp");
+        if let Some(ref tmp) = tmp_dir {
+            if tmp.exists() {
+                std::fs::remove_dir_all(tmp)?;
+            }
+            std::fs::create_dir_all(tmp)?;
+        }
         Ok(Self {
             objects: {
                 let path = config.cache_dir("objects");
-                Cache::from_config("objects", path, config.caches.downloaded.into())?
+                Cache::from_config(
+                    "objects",
+                    path,
+                    tmp_dir.clone(),
+                    config.caches.downloaded.into(),
+                )?
             },
             object_meta: {
                 let path = config.cache_dir("object_meta");
-                Cache::from_config("object_meta", path, config.caches.derived.into())?
+                Cache::from_config(
+                    "object_meta",
+                    path,
+                    tmp_dir.clone(),
+                    config.caches.derived.into(),
+                )?
             },
             symcaches: {
                 let path = config.cache_dir("symcaches");
-                Cache::from_config("symcaches", path, config.caches.derived.into())?
+                Cache::from_config(
+                    "symcaches",
+                    path,
+                    tmp_dir.clone(),
+                    config.caches.derived.into(),
+                )?
             },
             cficaches: {
                 let path = config.cache_dir("cficaches");
-                Cache::from_config("cficaches", path, config.caches.derived.into())?
+                Cache::from_config(
+                    "cficaches",
+                    path,
+                    tmp_dir.clone(),
+                    config.caches.derived.into(),
+                )?
             },
             diagnostics: {
                 let path = config.cache_dir("diagnostics");
-                Cache::from_config("diagnostics", path, config.caches.diagnostics.into())?
+                Cache::from_config(
+                    "diagnostics",
+                    path,
+                    tmp_dir,
+                    config.caches.diagnostics.into(),
+                )?
             },
         })
     }
@@ -384,12 +440,49 @@ mod tests {
         let _cache = Cache::from_config(
             "test",
             Some(cachedir.clone()),
+            None,
             CacheConfig::Downloaded(Default::default()),
         );
         let fsinfo = fs::metadata(cachedir).unwrap();
         assert!(fsinfo.is_dir());
     }
 
+    #[test]
+    fn test_caches_tmp_created() {
+        let basedir = tempdir().unwrap();
+        let cachedir = basedir.path().join("cache");
+        let tmpdir = cachedir.join("tmp");
+
+        let _caches = Caches::from_config(&Config {
+            cache_dir: Some(cachedir),
+            ..Default::default()
+        });
+
+        let fsinfo = fs::metadata(tmpdir).unwrap();
+        assert!(fsinfo.is_dir());
+    }
+
+    #[test]
+    fn test_caches_tmp_cleared() {
+        let basedir = tempdir().unwrap();
+        let cachedir = basedir.path().join("cache");
+        let tmpdir = cachedir.join("tmp");
+
+        create_dir_all(&tmpdir).unwrap();
+        let spam = tmpdir.join("spam");
+        File::create(&spam).unwrap();
+        let fsinfo = fs::metadata(&spam).unwrap();
+        assert!(fsinfo.is_file());
+
+        let _caches = Caches::from_config(&Config {
+            cache_dir: Some(cachedir),
+            ..Default::default()
+        });
+
+        let fsinfo = fs::metadata(spam);
+        assert!(fsinfo.is_err());
+    }
+
     #[test]
     fn test_max_unused_for() -> Result<(), CleanupError> {
         let tempdir = tempdir()?;
@@ -398,6 +491,7 @@ mod tests {
         let cache = Cache::from_config(
             "test",
             Some(tempdir.path().to_path_buf()),
+            None,
             CacheConfig::Derived(DerivedCacheConfig {
                 max_unused_for: Some(Duration::from_millis(10)),
                 ..Default::default()
@@ -434,6 +528,7 @@ mod tests {
         let cache = Cache::from_config(
             "test",
             Some(tempdir.path().to_path_buf()),
+            None,
             CacheConfig::Derived(DerivedCacheConfig {
                 retry_misses_after: Some(Duration::from_millis(20)),
                 ..Default::default()
@@ -479,6 +574,7 @@ mod tests {
         let cache = Cache::from_config(
             "test",
             Some(tempdir.path().to_path_buf()),
+            None,
             CacheConfig::Derived(Default::default()),
         )?;
 

src/config.rs

@@ -248,6 +248,10 @@ pub struct Config {
 }
 
 impl Config {
+    /// Return a cache directory `dir`, it is joined with the configured base cache directory.
+    ///
+    /// If there is no base cache directory configured this means no caching should happen
+    /// and this returns None.
     pub fn cache_dir<P>(&self, dir: P) -> Option<PathBuf>
     where
         P: AsRef<Path>,