Repository: vercel/next.js
Pull request: fix(turbo-persistence): Atomically mark old versioned directories for deletion before deleting them

This uses a trick similar to what I did in https://app.graphite.dev/github/pr/vercel/next.js/79425 to mark the directory for deletion using an atomic operation (a directory rename in this case) before deleting it, so that we can resume the deletion upon restart if we're interrupted while deleting the directory.

This may be important as deleting large directories can take some time.

I think this is the source of these user reports: https://vercel.slack.com/archives/C046HAU4H7F/p1748616184765179

Closes PACK-4765

Changed files:
filename: Cargo.lock
patch: @@ -2308,12 +2308,12 @@ dependencies = [
 
 [[package]]
 name = "errno"
-version = "0.3.8"
+version = "0.3.12"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a258e46cdc063eb8519c00b9fc845fc47bcfca4130e2f08e88665ceda8474245"
+checksum = "cea14ef9355e3beab063703aa9dab15afd25f0667c341310c1e5274bb1d0da18"
 dependencies = [
  "libc",
- "windows-sys 0.52.0",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -3033,7 +3033,7 @@ dependencies = [
  "httpdate",
  "itoa",
  "pin-project-lite",
- "socket2 0.4.9",
+ "socket2 0.5.8",
  "tokio",
  "tower-service",
  "tracing",
@@ -4039,6 +4039,12 @@ version = "0.4.14"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "78b3ae25bc7c8c38cec158d1f2757ee79e9b3740fbc7ccf0e59e4b08d793fa89"
 
+[[package]]
+name = "linux-raw-sys"
+version = "0.9.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12"
+
 [[package]]
 name = "litemap"
 version = "0.7.4"
@@ -6478,6 +6484,19 @@ dependencies = [
  "windows-sys 0.52.0",
 ]
 
+[[package]]
+name = "rustix"
+version = "1.0.7"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c71e83d6afe7ff64890ec6b71d6a69bb8a610ab78ce364b3352876bb4c801266"
+dependencies = [
+ "bitflags 2.9.0",
+ "errno",
+ "libc",
+ "linux-raw-sys 0.9.4",
+ "windows-sys 0.59.0",
+]
+
 [[package]]
 name = "rustls"
 version = "0.20.9"
@@ -8820,14 +8839,14 @@ checksum = "1ac9aa371f599d22256307c24a9d748c041e548cbf599f35d890f9d365361790"
 
 [[package]]
 name = "tempfile"
-version = "3.14.0"
+version = "3.20.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "28cce251fcbc87fac86a866eeb0d6c2d536fc16d06f184bb61aeae11aa4cee0c"
+checksum = "e8a64e3985349f2441a1a9ef0b853f869006c3855f2cda6862a94d26ebb9d6a1"
 dependencies = [
- "cfg-if",
  "fastrand 2.2.0",
+ "getrandom 0.3.2",
  "once_cell",
- "rustix 0.38.41",
+ "rustix 1.0.7",
  "windows-sys 0.59.0",
 ]
 
@@ -9611,11 +9630,13 @@ dependencies = [
  "rand 0.9.0",
  "rayon",
  "regex",
+ "rstest",
  "rustc-hash 2.1.1",
  "serde",
  "serde_json",
  "serde_path_to_error",
  "smallvec",
+ "tempfile",
  "thread_local",
  "tokio",
  "tracing",

filename: Cargo.toml
patch: @@ -406,7 +406,7 @@ sourcemap = "9.2.2"
 strsim = "0.11.1"
 shrink-to-fit = "0.2.10"
 syn = "2.0.100"
-tempfile = "3.3.0"
+tempfile = "3.20.0"
 thread_local = "1.1.8"
 thiserror = "1.0.48"
 tokio = "1.43.0"

filename: turbopack/crates/turbo-persistence/Cargo.toml
patch: @@ -31,7 +31,7 @@ zstd = { version = "0.13.2", features = ["zdict_builder"] }
 
 [dev-dependencies]
 rand = { workspace = true, features = ["small_rng"] }
-tempfile = "3.14.0"
+tempfile = { workspace = true }
 
 [lints]
 workspace = true

filename: turbopack/crates/turbo-tasks-backend/Cargo.toml
patch: @@ -55,6 +55,8 @@ turbo-tasks-testing = { workspace = true }
 criterion = { workspace = true, features = ["async_tokio"] }
 regex = { workspace = true }
 serde_json = { workspace = true }
+tempfile = { workspace = true }
+rstest = { workspace = true }
 
 [build-dependencies]
 turbo-tasks-build = { workspace = true }

filename: turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs
patch: @@ -1,6 +1,7 @@
 use std::{
     env,
-    fs::{metadata, read_dir, remove_dir_all},
+    ffi::{OsStr, OsString},
+    fs::{DirEntry, read_dir, remove_dir_all, rename},
     path::{Path, PathBuf},
     time::Duration,
 };
@@ -21,10 +22,23 @@ pub struct GitVersionInfo<'a> {
 }
 
 /// Specifies many databases that have a different version than the current one are retained.
-/// For example if MAX_OTHER_DB_VERSIONS is 2, there can be at most 3 databases in the directory,
-/// the current one and two older/newer ones. On CI it never keeps any other versions.
-const MAX_OTHER_DB_VERSIONS: usize = 2;
+/// For example if `DEFAULT_MAX_OTHER_DB_VERSIONS` is 2, there can be at most 3 databases in the
+/// directory, the current one and two older/newer ones. On CI it never keeps any other versions.
+const DEFAULT_MAX_OTHER_DB_VERSIONS: usize = 2;
 
+/// Directories are prefixed with this before being deleted, so that if we fail to fully delete the
+/// directory, we can pick up where we left off last time.
+const DELETION_PREFIX: &str = "__stale_";
+
+/// Given a base path, creates a version directory for the given `version_info`. Automatically
+/// cleans up old/stale databases.
+///
+/// **Envrionment Variables**
+/// - `TURBO_ENGINE_VERSION`: Forces use of a specific database version.
+/// - `TURBO_ENGINE_IGNORE_DIRTY`: Enable persistent caching in a dirty git repository. Otherwise a
+///   temporary directory is created.
+/// - `TURBO_ENGINE_DISABLE_VERSIONING`: Ignores versioning and always uses the same "unversioned"
+///   database when set.
 pub fn handle_db_versioning(
     base_path: &Path,
     version_info: &GitVersionInfo,
@@ -33,9 +47,6 @@ pub fn handle_db_versioning(
     if let Ok(version) = env::var("TURBO_ENGINE_VERSION") {
         return Ok(base_path.join(version));
     }
-    // Database versioning. Pass `TURBO_ENGINE_IGNORE_DIRTY` at runtime to ignore a
-    // dirty git repository. Pass `TURBO_ENGINE_DISABLE_VERSIONING` at runtime to disable
-    // versioning and always use the same database.
     let ignore_dirty = env::var("TURBO_ENGINE_IGNORE_DIRTY").ok().is_some();
     let disabled_versioning = env::var("TURBO_ENGINE_DISABLE_VERSIONING").ok().is_some();
     let version = if disabled_versioning {
@@ -63,48 +74,157 @@ pub fn handle_db_versioning(
     if let Some(version) = version {
         path = base_path.join(version);
 
-        let max_other_db_versions = if is_ci { 0 } else { MAX_OTHER_DB_VERSIONS };
+        let max_other_db_versions = if is_ci {
+            0
+        } else {
+            DEFAULT_MAX_OTHER_DB_VERSIONS
+        };
 
-        // Remove old databases if needed
         if let Ok(read_dir) = read_dir(base_path) {
-            let old_dbs = read_dir
-                .filter_map(|entry| {
-                    let entry = entry.ok()?;
-                    if !entry.file_type().ok()?.is_dir() {
-                        return None;
+            let mut old_dbs = Vec::new();
+            for entry in read_dir {
+                let Ok(entry) = entry else { continue };
+
+                // skip our target version (if it exists)
+                let name = entry.file_name();
+                if name == version {
+                    continue;
+                }
+
+                // skip non-directories
+                let Ok(file_type) = entry.file_type() else {
+                    continue;
+                };
+                if !file_type.is_dir() {
+                    continue;
+                }
+
+                // Find and try to finish removing any partially deleted directories
+                if name
+                    .as_encoded_bytes()
+                    .starts_with(AsRef::<OsStr>::as_ref(DELETION_PREFIX).as_encoded_bytes())
+                {
+                    // failures during cleanup of a cache directory are not fatal
+                    let _ = remove_dir_all(entry.path());
+                    continue;
+                }
+
+                old_dbs.push(entry);
+            }
+
+            if old_dbs.len() > max_other_db_versions {
+                old_dbs.sort_by_cached_key(|entry| {
+                    fn get_age(e: &DirEntry) -> Result<Duration> {
+                        let m = e.metadata()?;
+                        // Maybe change this: We care more about the atime/mtime of the files inside
+                        // the directory than the directory itself. atime is also fragile because it
+                        // can be impacted by recursive scanning tools (e.g. ripgrep). It might be
+                        // better for us to always explicitly touch a specific file inside the
+                        // versioned directory when reading the cache, and then use that file's
+                        // mtime.
+                        Ok(m.accessed().or_else(|_| m.modified())?.elapsed()?)
                     }
-                    let name = entry.file_name();
-                    let name = name.to_string_lossy();
-                    if name == version {
-                        return None;
+                    get_age(entry).unwrap_or(Duration::MAX)
+                });
+                for entry in old_dbs.into_iter().skip(max_other_db_versions) {
+                    let mut new_name = OsString::from(DELETION_PREFIX);
+                    new_name.push(entry.file_name());
+                    let new_path = base_path.join(new_name);
+                    // rename first, it's an atomic operation
+                    let rename_result = rename(entry.path(), &new_path);
+                    // Only try to delete the files if the rename succeeded, it's not safe to delete
+                    // contents if we didn't manage to first poison the directory by renaming it.
+                    if rename_result.is_ok() {
+                        // It's okay if this fails, as we've already poisoned the directory.
+                        let _ = remove_dir_all(&new_path);
                     }
-                    Some(entry.path())
-                })
-                .collect::<Vec<_>>();
-            if old_dbs.len() > max_other_db_versions {
-                let mut old_dbs = old_dbs
-                    .iter()
-                    .map(|p| {
-                        fn get_age(p: &Path) -> Result<Duration> {
-                            let m = metadata(p)?;
-                            Ok(m.accessed().or_else(|_| m.modified())?.elapsed()?)
-                        }
-                        (
-                            p,
-                            get_age(p).unwrap_or(Duration::from_secs(10 * 356 * 24 * 60 * 60)),
-                        )
-                    })
-                    .collect::<Vec<_>>();
-                old_dbs.sort_by_key(|(_, age)| *age);
-                for (p, _) in old_dbs.into_iter().skip(max_other_db_versions) {
-                    let _ = remove_dir_all(p);
                 }
             }
         }
     } else {
         path = base_path.join("temp");
-        let _ = remove_dir_all(&path);
+        // propagate errors: if this fails we may have stale files left over in the temp directory
+        remove_dir_all(&path)?;
     }
 
     Ok(path)
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{fs, thread::sleep};
+
+    use rstest::rstest;
+    use tempfile::TempDir;
+
+    use super::*;
+
+    fn count_entries(base_path: &Path) -> usize {
+        fs::read_dir(base_path)
+            .unwrap()
+            .collect::<Result<Vec<_>, _>>()
+            .unwrap()
+            .len()
+    }
+
+    #[rstest]
+    #[case::not_ci(false, DEFAULT_MAX_OTHER_DB_VERSIONS)]
+    #[case::ci(true, 0)]
+    fn test_max_versions(#[case] is_ci: bool, #[case] max_other_db_versions: usize) {
+        let tmp_dir = TempDir::new().unwrap();
+        let base_path = tmp_dir.path();
+        let current_version_name = "mock-version";
+
+        let version_info = GitVersionInfo {
+            describe: current_version_name,
+            dirty: false,
+        };
+
+        fs::create_dir(base_path.join(current_version_name)).unwrap();
+
+        // sleep to ensure `current_version_name` has the oldest atime/mtime
+        // it should be preserved regardless of atime/mtime
+        sleep(Duration::from_millis(100));
+
+        let num_other_dirs = max_other_db_versions + 3;
+        for i in 0..num_other_dirs {
+            fs::create_dir(base_path.join(format!("other-dir-{i}"))).unwrap();
+        }
+
+        assert_eq!(
+            count_entries(base_path),
+            num_other_dirs + 1, // +1 for current version
+        );
+
+        let versioned_path = handle_db_versioning(base_path, &version_info, is_ci).unwrap();
+
+        assert_eq!(versioned_path, base_path.join(current_version_name));
+        assert!(base_path.join(current_version_name).exists());
+        assert_eq!(
+            count_entries(base_path),
+            max_other_db_versions + 1, // +1 for current version
+        );
+    }
+
+    #[test]
+    fn test_cleanup_of_prefixed_items() {
+        let tmp_dir = TempDir::new().unwrap();
+        let base_path = tmp_dir.path();
+        let current_version_name = "mock-version";
+
+        let version_info = GitVersionInfo {
+            describe: current_version_name,
+            dirty: false,
+        };
+
+        for i in 0..5 {
+            fs::create_dir(base_path.join(format!("{DELETION_PREFIX}other-dir-{i}"))).unwrap();
+        }
+
+        assert_eq!(count_entries(base_path), 5);
+
+        handle_db_versioning(base_path, &version_info, /* is_ci */ false).unwrap();
+
+        assert_eq!(count_entries(base_path), 0);
+    }
+}