From 361443f317d6926c48c9d81483b54c7894ca7d9f Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Fri, 7 Aug 2026 09:49:57 -0700 Subject: [PATCH 1/7] Retain no more than 1 stale db version, and then only if it isn't too old. --- Cargo.lock | 3 + packages/next/src/lib/turbopack-cache-seed.ts | 28 +- turbopack/crates/turbo-persistence/Cargo.toml | 4 +- turbopack/crates/turbo-persistence/README.md | 2 +- .../turbo-persistence/src/bin/sst_inspect.rs | 7 +- turbopack/crates/turbo-persistence/src/db.rs | 106 ++++- turbopack/crates/turbo-persistence/src/lib.rs | 5 +- .../crates/turbo-persistence/src/tests.rs | 123 +++++- .../crates/turbo-tasks-backend/Cargo.toml | 1 + .../src/database/db_versioning.rs | 380 ++++++++++++++---- 10 files changed, 550 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a4f7a717b3f1..f330e2fe0f6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9945,6 +9945,8 @@ dependencies = [ "rand 0.10.1", "rayon", "rustc-hash 2.1.1", + "serde", + "serde_json", "smallvec", "tempfile", "thread_local", @@ -10049,6 +10051,7 @@ dependencies = [ "hashbrown 0.14.5", "indexmap 2.13.0", "indoc", + "jiff", "lzzzz", "parking_lot", "rand 0.10.1", diff --git a/packages/next/src/lib/turbopack-cache-seed.ts b/packages/next/src/lib/turbopack-cache-seed.ts index 3e1e6a87859b..ada783b490cf 100644 --- a/packages/next/src/lib/turbopack-cache-seed.ts +++ b/packages/next/src/lib/turbopack-cache-seed.ts @@ -76,10 +76,10 @@ function findSeedSource( const currentWorktree = path.resolve(worktreeInfo.worktreeRoot) // We are going to find the best candidate worktree - // based on the newest mtime of the CURRENT file in the cache directory. + // based on the most recently used cache directory. // We only look at our version - let best: { versionDir: string; mtimeMs: number } | undefined + let best: { versionDir: string; lastUsedMs: number } | undefined for (const root of [ worktreeInfo.mainRepoRoot, ...listLinkedWorktreeRoots(worktreeInfo.mainRepoRoot), @@ -93,21 +93,33 @@ function findSeedSource( 'turbopack', version ) - const mtimeMs = currentMtimeMs(versionDir) - if (mtimeMs === undefined) continue - if (!best || mtimeMs > best.mtimeMs) { - best = { versionDir, mtimeMs } + const lastUsedMs = currentLastUsedMs(versionDir) + if (lastUsedMs === undefined) continue + if (!best || lastUsedMs > best.lastUsedMs) { + best = { versionDir, lastUsedMs } } } return best?.versionDir } -function currentMtimeMs(versionDir: string): number | undefined { +// When the cache in `versionDir` was last used, in epoch milliseconds, or undefined if there is +// no usable cache there. Read from the `last_used_time` the persistence layer records in CURRENT. +// +// No fallback for the pre-JSON CURRENT format: callers only look inside the directory named for +// the running binary's own cache version, which a binary that old could not have written. +function currentLastUsedMs(versionDir: string): number | undefined { + let lastUsed try { - return fs.statSync(path.join(versionDir, 'CURRENT')).mtimeMs + lastUsed = JSON.parse( + fs.readFileSync(path.join(versionDir, 'CURRENT'), 'utf8') + ).last_used_time } catch { + // missing, unreadable, or not valid JSON - treat it as not a seed candidate return undefined } + if (typeof lastUsed !== 'string') return undefined + const parsed = Date.parse(lastUsed) + return Number.isNaN(parsed) ? undefined : parsed } function dirHasEntries(dir: string): boolean { diff --git a/turbopack/crates/turbo-persistence/Cargo.toml b/turbopack/crates/turbo-persistence/Cargo.toml index de892885a6fe..b70c73ccf1fb 100644 --- a/turbopack/crates/turbo-persistence/Cargo.toml +++ b/turbopack/crates/turbo-persistence/Cargo.toml @@ -20,7 +20,7 @@ crc32fast = { workspace = true } dashmap = { workspace = true} either = { workspace = true } fs-err = { workspace = true } -jiff = "0.2.10" +jiff = { version = "0.2.10", features = ["serde"] } lzzzz = { workspace = true } memmap2 = "0.9.5" nohash-hasher = { workspace = true } @@ -31,6 +31,8 @@ postcard = { workspace = true, features = ["alloc", "use-std"] } zerocopy = { version = "0.8", features = ["derive"] } quick_cache = { workspace = true } rustc-hash = { workspace = true } +serde = { workspace = true } +serde_json = { workspace = true } smallvec = { workspace = true } thread_local = { workspace = true } tracing = { workspace = true } diff --git a/turbopack/crates/turbo-persistence/README.md b/turbopack/crates/turbo-persistence/README.md index 75172b7e4af4..e8457b173b57 100644 --- a/turbopack/crates/turbo-persistence/README.md +++ b/turbopack/crates/turbo-persistence/README.md @@ -12,7 +12,7 @@ It supports having multiple key families, which are stored in separate files, bu ## On disk format -There is a single `CURRENT` file which stores the latest committed sequence number. +There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when the database was last opened or committed to (`last_used_time`). The last-used time lives in the file rather than being taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. All other files have a sequence number as file name, e. g. `0000123.sst`. All files are immutable once their sequence number is <= the committed sequence number. But they might be deleted when they are superseded by other committed files. diff --git a/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs b/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs index 4bda8d36ab34..f359cb743528 100644 --- a/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs +++ b/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs @@ -24,6 +24,7 @@ use turbo_persistence::{ BLOCK_HEADER_SIZE, checksum_block, meta_file::MetaFile, mmap_helper::advise_mmap_for_persistence, + read_current_version, sst_filter::SstFilter, static_sorted_file::{ BLOCK_TYPE_FIXED_KEY_NO_HASH, BLOCK_TYPE_FIXED_KEY_WITH_HASH, BLOCK_TYPE_KEY_NO_HASH, @@ -211,9 +212,9 @@ fn format_bytes(bytes: u64) -> String { /// and apply SstFilter to skip superseded entries. fn collect_sst_info(db_path: &Path) -> Result>> { // Read the CURRENT sequence number — only files with seq <= current are valid. - let current: u32 = File::open(db_path.join("CURRENT"))? - .read_u32::() - .context("Failed to read CURRENT file")?; + let current = read_current_version(db_path)? + .context("CURRENT file is missing")? + .max_sequence_number; // Read .del files to find sequences that were deleted but not yet cleaned up. let mut deleted_seqs: HashSet = HashSet::new(); diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index 014a37a9ea76..2ac675d4cf6c 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -20,6 +20,7 @@ use jiff::Timestamp; use memmap2::Mmap; use nohash_hasher::BuildNoHashHasher; use parking_lot::{Mutex, RwLock}; +use serde::{Deserialize, Serialize}; use smallvec::SmallVec; use tracing::span::EnteredSpan; @@ -159,19 +160,79 @@ impl WriteOperationGuard<'_> { } } +/// The contents of the `CURRENT` file: which sequence number is committed, and when the database +/// was last used. +/// +/// Serialized as a small JSON object. `last_used_time` lives in the file rather than being taken +/// from its mtime because mtimes don't survive the directory being copied or restored (CI cache +/// restore, `cp -r`, container image builds), which would corrupt version eviction's idea of age. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct CurrentDbVersion { + /// The highest sequence number that is part of the committed database. Files with a greater + /// sequence number are orphans from an interrupted write and get deleted on open. + pub max_sequence_number: u32, + /// When this database was last opened or committed to. + pub last_used_time: Timestamp, +} + +/// Reads the `CURRENT` file in the database directory `path`. +/// +/// Returns `Ok(None)` if the file doesn't exist, which for a writable database means "not +/// initialized yet". +/// +/// A `CURRENT` that exists but doesn't parse is an error rather than something to recover from +/// here. Version directories are named after the build that wrote them, so a change to this format +/// comes with a new directory name and this function never sees an older one; anything unparsable +/// is corruption. Callers that scan directories they didn't write (see the cache-version eviction +/// in `turbo-tasks-backend`) are the ones that have to tolerate it. +pub fn read_current_version(path: &Path) -> Result> { + let current_path = path.join("CURRENT"); + let content = match fs::read(¤t_path) { + Ok(content) => content, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) => return Err(e).context("Failed to read CURRENT file"), + }; + + serde_json::from_slice::(&content) + .with_context(|| { + format!( + "CURRENT file at {} is corrupt ({} bytes)", + current_path.display(), + content.len() + ) + }) + .map(Some) +} + /// Durably and atomically updates the `CURRENT` file in the database directory `path` to point at -/// `seq`. +/// `seq`, stamping it as used now. /// -/// The write is made atomic by writing `seq` to a temporary `CURRENT.next` file, flushing it, and -/// then `rename`ing it over `CURRENT`. A `rename` within a directory is atomic on POSIX and -/// replaces the destination on Windows, so a concurrent or crashing writer can never observe a -/// torn `CURRENT` (in-place overwrites, by contrast, can leave a partially-written value on a -/// crash mid-write). After the rename we fsync the directory so the new `CURRENT` → inode mapping -/// survives a crash. +/// The write is made atomic by writing to a temporary `CURRENT.next` file, flushing it, and then +/// `rename`ing it over `CURRENT`. A `rename` within a directory is atomic on POSIX and replaces the +/// destination on Windows, so a concurrent or crashing writer can never observe a torn `CURRENT` +/// (in-place overwrites, by contrast, can leave a partially-written value on a crash mid-write). +/// After the rename we fsync the directory so the new `CURRENT` → inode mapping survives a crash. fn commit_current(path: &Path, seq: u32) -> Result<()> { + commit_current_version( + path, + &CurrentDbVersion { + max_sequence_number: seq, + last_used_time: Timestamp::now(), + }, + ) +} + +/// As [`commit_current`], but writes an explicit [`CurrentDbVersion`]. +fn commit_current_version(path: &Path, version: &CurrentDbVersion) -> Result<()> { + // Serialize up front and write once: `serde_json::to_writer` into an unbuffered `File` would + // issue a syscall per JSON token, and this runs on every commit and every open. + let mut contents = + serde_json::to_string(version).context("Failed to serialize the CURRENT file")?; + contents.push('\n'); + let next_path = path.join("CURRENT.next"); let mut next_file = File::create(&next_path)?; - next_file.write_u32::(seq)?; + next_file.write_all(contents.as_bytes())?; next_file.sync_data()?; drop(next_file); @@ -438,7 +499,7 @@ impl TurboPersistence parallel_scheduler, config, }); - db.open_directory(false)?; + db.open_directory(true)?; Ok(db) } @@ -479,18 +540,11 @@ impl TurboPersistence /// Loads an existing database directory and performs cleanup if necessary. fn load_directory(&mut self, entries: ReadDir, read_only: bool) -> Result { let mut meta_files = Vec::new(); - let mut current_file = match File::open(self.path.join("CURRENT")) { - Ok(file) => file, - Err(e) => { - if !read_only && e.kind() == std::io::ErrorKind::NotFound { - return Ok(false); - } else { - return Err(e).context("Failed to open CURRENT file"); - } - } + let current = match read_current_version(&self.path)? { + Some(version) => version.max_sequence_number, + None if !read_only => return Ok(false), + None => bail!("Failed to open database: CURRENT file is missing"), }; - let current = current_file.read_u32::()?; - drop(current_file); let mut deleted_files = HashSet::new(); for entry in entries { @@ -599,6 +653,18 @@ impl TurboPersistence .store(meta_files.is_empty(), Ordering::Relaxed); inner.meta_files = meta_files; inner.current_sequence_number = current; + + // Refresh the last-used stamp. This happens even for a read-only open: opening to read is + // still a use, and eviction shouldn't treat a database as abandoned just because nothing + // wrote to it. Rewriting `CURRENT` is the only mutation a read-only open makes. + // + // Best-effort. On failure the stamp keeps its old value, so the database looks less + // recently used than it is and may be evicted early; that's recoverable, and an open that + // can otherwise succeed shouldn't fail over a cache-eviction hint. A failure part-way + // through can leave a stale `CURRENT.next` behind, which is harmless: the next + // `commit_current` truncates it. + let _ = commit_current(&self.path, current); + Ok(true) } diff --git a/turbopack/crates/turbo-persistence/src/lib.rs b/turbopack/crates/turbo-persistence/src/lib.rs index 90f6e4fef06c..ebc5e52f1daa 100644 --- a/turbopack/crates/turbo-persistence/src/lib.rs +++ b/turbopack/crates/turbo-persistence/src/lib.rs @@ -30,7 +30,10 @@ mod tests; pub use arc_bytes::ArcBytes; pub use compression::checksum_block; -pub use db::{CommitStats, CompactConfig, MetaFileEntryInfo, MetaFileInfo, TurboPersistence}; +pub use db::{ + CommitStats, CompactConfig, CurrentDbVersion, MetaFileEntryInfo, MetaFileInfo, + TurboPersistence, read_current_version, +}; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum FamilyKind { diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index 81dd762563e7..f4d896087ecd 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -6,7 +6,7 @@ use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::{ DbConfig, FamilyConfig, FamilyKind, constants::{MAX_MEDIUM_VALUE_SIZE, MAX_SMALL_VALUE_SIZE}, - db::{CompactConfig, TurboPersistence}, + db::{CompactConfig, CurrentDbVersion, TurboPersistence, read_current_version}, parallel_scheduler::ParallelScheduler, write_batch::WriteBatch, }; @@ -2229,3 +2229,124 @@ fn stale_current_next_is_recovered() -> Result<()> { Ok(()) } + +/// `CURRENT` round-trips through JSON, recording both the sequence number and a last-used time. +#[test] +fn current_file_is_json_with_last_used_time() -> Result<()> { + use crate::parallel_scheduler::SerialScheduler; + + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + let before = jiff::Timestamp::now(); + { + let db = TurboPersistence::::open(path.to_path_buf())?; + let batch = db.write_batch()?; + batch.put(0, vec![1u8], vec![42u8].into())?; + db.commit_write_batch(batch)?; + db.shutdown()?; + } + let after = jiff::Timestamp::now(); + + // `CURRENT` is a stable on-disk format that external tools parse without going through this + // crate, so the JSON shape and these field names are a public contract, not an internal detail. + let raw = fs::read_to_string(path.join("CURRENT"))?; + assert!(raw.contains("max_sequence_number"), "got: {raw}"); + assert!(raw.contains("last_used_time"), "got: {raw}"); + + let version = read_current_version(path)?.expect("CURRENT should exist"); + assert!(version.max_sequence_number > 0); + assert!( + version.last_used_time >= before && version.last_used_time <= after, + "last_used_time {} outside [{before}, {after}]", + version.last_used_time + ); + + Ok(()) +} + +/// A `CURRENT` that exists but doesn't parse is corruption, and opening must fail loudly rather +/// than silently treating the database as empty — that would orphan and delete every SST. +#[test] +fn corrupt_current_file_fails_to_open() -> Result<()> { + use crate::parallel_scheduler::SerialScheduler; + + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + { + let db = TurboPersistence::::open(path.to_path_buf())?; + let batch = db.write_batch()?; + batch.put(0, vec![1u8], vec![42u8].into())?; + db.commit_write_batch(batch)?; + db.shutdown()?; + } + + // A truncated `CURRENT`, e.g. from an interrupted copy. Four bytes specifically: that used to + // be the whole file, and a length-based format guess would read it as a sequence number. + fs::write(path.join("CURRENT"), [0u8; 4])?; + + assert!( + read_current_version(path).is_err(), + "a truncated CURRENT must be reported as corrupt, not parsed" + ); + assert!( + TurboPersistence::::open(path.to_path_buf()).is_err(), + "opening a database with a corrupt CURRENT must fail" + ); + + // The data must still be on disk: a failed open must not have deleted anything. + let ssts = fs::read_dir(path)? + .filter_map(|e| e.ok()) + .filter(|e| e.path().extension().is_some_and(|ext| ext == "sst")) + .count(); + assert!(ssts > 0, "a failed open must not delete SST files"); + + Ok(()) +} + +/// Opening a database refreshes its last-used time even when nothing is written, so that a +/// read-only session still counts as a use for cache-version eviction. +#[test] +fn opening_a_database_refreshes_last_used_time() -> Result<()> { + use crate::parallel_scheduler::SerialScheduler; + + let tempdir = tempfile::tempdir()?; + let path = tempdir.path(); + + { + let db = TurboPersistence::::open(path.to_path_buf())?; + let batch = db.write_batch()?; + batch.put(0, vec![1u8], vec![42u8].into())?; + db.commit_write_batch(batch)?; + db.shutdown()?; + } + + // Backdate the recorded timestamp, leaving the sequence number intact. + let stale = read_current_version(path)?.unwrap(); + let backdated = jiff::Timestamp::now() - jiff::SignedDuration::from_hours(72); + fs::write( + path.join("CURRENT"), + serde_json::to_vec(&CurrentDbVersion { + max_sequence_number: stale.max_sequence_number, + last_used_time: backdated, + })?, + )?; + + { + let db = TurboPersistence::::open(path.to_path_buf())?; + db.shutdown()?; + } + + let refreshed = read_current_version(path)?.unwrap(); + assert_eq!( + refreshed.max_sequence_number, stale.max_sequence_number, + "sequence number must be preserved" + ); + assert!( + refreshed.last_used_time > backdated, + "last_used_time should be refreshed on open" + ); + + Ok(()) +} diff --git a/turbopack/crates/turbo-tasks-backend/Cargo.toml b/turbopack/crates/turbo-tasks-backend/Cargo.toml index 8919e3e5a109..57ec522fad8a 100644 --- a/turbopack/crates/turbo-tasks-backend/Cargo.toml +++ b/turbopack/crates/turbo-tasks-backend/Cargo.toml @@ -45,6 +45,7 @@ dashmap = { workspace = true, features = ["raw-api"]} fs-err = { workspace = true } hashbrown = { workspace = true, features = ["raw"] } indexmap = { workspace = true } +jiff = "0.2.10" lzzzz = { workspace = true, optional = true } parking_lot = { workspace = true } rand = { workspace = true } diff --git a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs index 306d6b46d920..d2702e004a6d 100644 --- a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs +++ b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs @@ -7,6 +7,8 @@ use std::{ use anyhow::Result; use fs_err::{DirEntry, read_dir, remove_dir_all, rename}; +use jiff::Timestamp; +use turbo_persistence::read_current_version; /// Information gathered by `vergen_gitcl` in the top-level binary crate and passed down. This /// information must be computed in the top-level crate for cargo incremental compilation to work @@ -21,10 +23,9 @@ pub struct GitVersionInfo<'a> { pub dirty: bool, } -/// Specifies many databases that have a different version than the current one are retained. -/// 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; +/// How many days a database with a version other than the current one is retained since it was +/// last used. Overridable via the `TURBO_ENGINE_VERSION_TTL_DAYS` environment variable. +const DEFAULT_OTHER_DB_VERSION_TTL_DAYS: u64 = 3; /// 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. @@ -33,12 +34,19 @@ const DELETION_PREFIX: &str = "__stale_"; /// Given a base path, creates a version directory for the given `version_info`. Automatically /// cleans up old/stale databases. /// +/// Exactly one database whose version isn't the current one is retained — the most recently used, +/// and only if it was used within [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`] — so that switching back to +/// a branch you recently left still finds its cache intact. On CI none are retained. The current +/// version is always retained. +/// /// **Environment Variables** /// - `TURBO_ENGINE_VERSION`: Forces use of a specific database version. /// - `TURBO_ENGINE_IGNORE_DIRTY`: Enable filesystem cache 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. +/// - `TURBO_ENGINE_VERSION_TTL_DAYS`: How many days to retain a database whose version isn't the +/// current one, as a whole number. Overrides [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`]. pub fn handle_db_versioning( base_path: &Path, version_info: &GitVersionInfo, @@ -74,14 +82,33 @@ pub fn handle_db_versioning( if let Some(version) = version { path = base_path.join(version); - let max_other_db_versions = if is_ci { - 0 + // On CI nothing is ever switched back to, so no other version is worth its disk. + let ttl = if is_ci { + None } else { - DEFAULT_MAX_OTHER_DB_VERSIONS + Some(other_db_version_ttl()) }; if let Ok(read_dir) = read_dir(base_path) { - let mut old_dbs = Vec::new(); + let evict = |entry: DirEntry| { + 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); + } + }; + + // Of the other versions we keep only the most recently used one, and only if it's + // within the TTL. Anything past the TTL is evicted as soon as we see it; the best + // candidate so far is held back until something more recent displaces it, and whatever + // is still held at the end is the one we keep. + let mut newest: Option<(Duration, DirEntry)> = None; for entry in read_dir { let Ok(entry) = entry else { continue }; @@ -109,38 +136,32 @@ pub fn handle_db_versioning( continue; } - old_dbs.push(entry); - } + // With no TTL nothing is retained, so don't spend a read working out an age that + // can't change the outcome. + let Some(ttl) = ttl else { + evict(entry); + continue; + }; - if old_dbs.len() > max_other_db_versions { - old_dbs.sort_by_cached_key(|entry| { - fn get_age(e: &DirEntry) -> Result { - 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()?) - } - 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); + let age = time_since_last_used(&entry)?; + if age > ttl { + evict(entry); + continue; + } + match &newest { + Some((newest_age, _)) if *newest_age <= age => evict(entry), + // This entry is more recent, so drop the one we were holding + _ => { + if let Some((_, previous)) = newest.replace((age, entry)) { + evict(previous); + } } } } } + + // The selected version is stamped as used by the persistence layer when it opens the + // database, so there's nothing to record here. } else { path = base_path.join("temp"); if path.exists() { @@ -153,81 +174,292 @@ pub fn handle_db_versioning( Ok(path) } +/// How long to retain a database whose version isn't the current one, honoring the +/// `TURBO_ENGINE_VERSION_TTL_DAYS` override. Falls back to [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`] if +/// the variable is unset or unparsable. +fn other_db_version_ttl() -> Duration { + let Ok(raw) = env::var("TURBO_ENGINE_VERSION_TTL_DAYS") else { + return ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS); + }; + // `u64::from_str` accepts a leading `+`; require plain digits so the accepted syntax is + // exactly what the name promises. + let trimmed = raw.trim(); + let days = (!trimmed.is_empty() && trimmed.bytes().all(|b| b.is_ascii_digit())) + .then(|| trimmed.parse::().ok()) + .flatten(); + match days { + Some(days) => ttl_from_days(days), + None => { + eprintln!( + "WARNING: Ignoring TURBO_ENGINE_VERSION_TTL_DAYS={raw:?}, expected a whole number \ + of days." + ); + ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS) + } + } +} + +fn ttl_from_days(days: u64) -> Duration { + Duration::from_secs(days.saturating_mul(24 * 60 * 60)) +} + +/// How long ago the version directory `entry` was last used, read from the `last_used_time` its +/// `CURRENT` file records. +/// +/// A directory with no `CURRENT` at all isn't a database we finished writing — access to the cache +/// root is serialized, so this can't be one that's mid-initialization — and gets [`Duration::MAX`] +/// so it's evicted ahead of any real cache. A `CURRENT` that exists but can't be read is a +/// different matter: that's an unexpected IO or corruption problem, and the error propagates rather +/// than being turned into a deletion. +/// +/// A stamp in the future (backwards clock jump, or a copy from a machine with a fast clock) reads +/// as age zero, so a version is never evicted for looking too new. +fn time_since_last_used(entry: &DirEntry) -> Result { + let Some(version) = read_current_version(&entry.path())? else { + return Ok(Duration::MAX); + }; + Ok(Timestamp::now() + .duration_since(version.last_used_time) + .try_into() + .unwrap_or_default()) +} + #[cfg(test)] mod tests { - use std::{fs, thread::sleep}; + use std::fs; use rstest::rstest; use tempfile::TempDir; + use turbo_persistence::CurrentDbVersion; use super::*; - fn count_entries(base_path: &Path) -> usize { - fs::read_dir(base_path) - .unwrap() - .collect::, _>>() + const CURRENT_VERSION: &str = "mock-version"; + + fn version_info() -> GitVersionInfo<'static> { + GitVersionInfo { + describe: CURRENT_VERSION, + dirty: false, + } + } + + /// Creates a version directory that looks like a real database (i.e. has a `CURRENT` file), + /// last used `used_ago` in the past. + fn create_version_dir(base_path: &Path, name: &str, used_ago: Duration) { + let path = base_path.join(name); + fs::create_dir(&path).unwrap(); + let last_used_time = Timestamp::now() - jiff::SignedDuration::try_from(used_ago).unwrap(); + fs::write( + path.join("CURRENT"), + serde_json::to_vec(&CurrentDbVersion { + max_sequence_number: 0, + last_used_time, + }) + .unwrap(), + ) + .unwrap(); + } + + fn entry_names(base_path: &Path) -> Vec { + let mut names = fs::read_dir(base_path) .unwrap() - .len() + .map(|e| e.unwrap().file_name().into_string().unwrap()) + .collect::>(); + names.sort(); + names } + /// Only the most recently used other version survives, and the current version survives + /// regardless of how stale it is. On CI no other version survives at all. #[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) { + #[case::not_ci(false, &["mock-version", "other-dir-0"])] + #[case::ci(true, &["mock-version"])] + fn test_max_versions(#[case] is_ci: bool, #[case] expected: &[&str]) { 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, - }; + // the least recently used of all, and preserved anyway + create_version_dir(base_path, CURRENT_VERSION, Duration::from_secs(60 * 60)); - fs::create_dir(base_path.join(current_version_name)).unwrap(); + for i in 0..4 { + // `other-dir-0` is the most recently used, so it's the one retained + create_version_dir( + base_path, + &format!("other-dir-{i}"), + Duration::from_secs(i + 1), + ); + } - // 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 versioned_path = handle_db_versioning(base_path, &version_info(), is_ci).unwrap(); + assert_eq!(versioned_path, base_path.join(CURRENT_VERSION)); + assert_eq!(entry_names(base_path), expected); + } - 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(); - } + /// A version that hasn't been used within the TTL is evicted even though it's within the count + /// limit. + #[test] + fn test_ttl_evicts_unused_version() { + let tmp_dir = TempDir::new().unwrap(); + let base_path = tmp_dir.path(); + + create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); + create_version_dir( + base_path, + "stale-version", + ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS) + Duration::from_secs(60), + ); + + handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); + + assert_eq!(entry_names(base_path), vec![CURRENT_VERSION]); + } + + /// A version used within the TTL is retained. + #[test] + fn test_ttl_retains_recently_used_version() { + let tmp_dir = TempDir::new().unwrap(); + let base_path = tmp_dir.path(); + + create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); + create_version_dir( + base_path, + "recent-version", + ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS) - Duration::from_secs(60), + ); + + handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); assert_eq!( - count_entries(base_path), - num_other_dirs + 1, // +1 for current version + entry_names(base_path), + vec!["mock-version", "recent-version"] ); + } - let versioned_path = handle_db_versioning(base_path, &version_info, is_ci).unwrap(); + /// A directory with no `CURRENT` file isn't one of ours, so it's evicted rather than occupying + /// the single retention slot — even when it holds recently written files. + #[rstest] + #[case::empty(false)] + #[case::with_recent_data_file(true)] + fn test_version_without_stamp_is_evicted(#[case] with_data_file: bool) { + let tmp_dir = TempDir::new().unwrap(); + let base_path = tmp_dir.path(); + + create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); - assert_eq!(versioned_path, base_path.join(current_version_name)); - assert!(base_path.join(current_version_name).exists()); + let unstamped = base_path.join("unstamped-version"); + fs::create_dir(&unstamped).unwrap(); + if with_data_file { + fs::write(unstamped.join("00000001.sst"), b"data").unwrap(); + } + + handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); + + assert_eq!(entry_names(base_path), vec![CURRENT_VERSION]); + } + + /// A `CURRENT` that exists but can't be parsed is an unexpected failure, not something to + /// silently act on: the error propagates instead of the directory being deleted. + #[test] + fn test_corrupt_current_propagates_error() { + let tmp_dir = TempDir::new().unwrap(); + let base_path = tmp_dir.path(); + + create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); + let corrupt = base_path.join("corrupt-version"); + fs::create_dir(&corrupt).unwrap(); + fs::write(corrupt.join("CURRENT"), b"not json").unwrap(); + + assert!( + handle_db_versioning(base_path, &version_info(), /* is_ci */ false).is_err(), + "an unreadable CURRENT should surface as an error" + ); + assert!( + entry_names(base_path).contains(&"corrupt-version".to_string()), + "the directory should not have been deleted" + ); + } + + /// An unparsable `TURBO_ENGINE_VERSION_TTL_DAYS` falls back to the default rather than + /// retaining nothing. + /// + /// Not `#[rstest]`-parameterized over several inputs: this mutates process-wide environment + /// state, so the cases can't run concurrently with each other or with any other test that + /// reads the same variable. + #[test] + fn test_ttl_days_override() { + // SAFETY: single-threaded test, and no other test reads this variable. + unsafe { + env::set_var("TURBO_ENGINE_VERSION_TTL_DAYS", "7"); + } + assert_eq!(other_db_version_ttl(), ttl_from_days(7)); + + unsafe { + env::set_var("TURBO_ENGINE_VERSION_TTL_DAYS", "not-a-number"); + } assert_eq!( - count_entries(base_path), - max_other_db_versions + 1, // +1 for current version + other_db_version_ttl(), + ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS), + "an unparsable value should fall back to the default" ); + + unsafe { + env::remove_var("TURBO_ENGINE_VERSION_TTL_DAYS"); + } + assert_eq!( + other_db_version_ttl(), + ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS) + ); + } + + /// The survivor is the most recently used regardless of the order `read_dir` yields entries, + /// including when the eventual winner is seen last and displaces an earlier candidate. + #[rstest] + #[case::ascending(&[1u64, 2, 3, 4])] + #[case::descending(&[4u64, 3, 2, 1])] + #[case::winner_last(&[3u64, 2, 4, 1])] + fn test_survivor_independent_of_scan_order(#[case] ages: &[u64]) { + let tmp_dir = TempDir::new().unwrap(); + let base_path = tmp_dir.path(); + + create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); + for age in ages { + create_version_dir(base_path, &format!("age-{age}"), Duration::from_secs(*age)); + } + + handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); + + // age-1 is the most recently used, so it's the one that survives. + assert_eq!(entry_names(base_path), vec!["age-1", CURRENT_VERSION]); } + /// On CI every other version is evicted regardless of age, so an unreadable `CURRENT` in one of + /// them is never consulted and can't fail the run. #[test] - fn test_cleanup_of_prefixed_items() { + fn test_ci_ignores_unreadable_current() { 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, - }; + create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); + let corrupt = base_path.join("corrupt-version"); + fs::create_dir(&corrupt).unwrap(); + fs::write(corrupt.join("CURRENT"), b"not json").unwrap(); + + handle_db_versioning(base_path, &version_info(), /* is_ci */ true).unwrap(); + + assert_eq!(entry_names(base_path), vec![CURRENT_VERSION]); + } + + #[test] + fn test_cleanup_of_prefixed_items() { + let tmp_dir = TempDir::new().unwrap(); + let base_path = tmp_dir.path(); 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(); + handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); - assert_eq!(count_entries(base_path), 0); + assert!(entry_names(base_path).is_empty()); } } From 85f082d1eb72052100198feb384715626c182aa9 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 8 Aug 2026 12:32:04 -0700 Subject: [PATCH 2/7] Tighten comments on db version retention Remove development narrative and restated code from the comments added in this change, and fix two stale references to the old count-based retention design (a doc comment mentioning a 'count limit' that no longer exists, and the test_max_versions name). --- packages/next/src/lib/turbopack-cache-seed.ts | 2 +- turbopack/crates/turbo-persistence/README.md | 2 +- turbopack/crates/turbo-persistence/src/db.rs | 24 ++++---- .../crates/turbo-persistence/src/tests.rs | 8 +-- .../src/database/db_versioning.rs | 60 ++++++++----------- 5 files changed, 42 insertions(+), 54 deletions(-) diff --git a/packages/next/src/lib/turbopack-cache-seed.ts b/packages/next/src/lib/turbopack-cache-seed.ts index ada783b490cf..b02125e5f382 100644 --- a/packages/next/src/lib/turbopack-cache-seed.ts +++ b/packages/next/src/lib/turbopack-cache-seed.ts @@ -106,7 +106,7 @@ function findSeedSource( // no usable cache there. Read from the `last_used_time` the persistence layer records in CURRENT. // // No fallback for the pre-JSON CURRENT format: callers only look inside the directory named for -// the running binary's own cache version, which a binary that old could not have written. +// the running binary's own cache version, which no binary that old could have written. function currentLastUsedMs(versionDir: string): number | undefined { let lastUsed try { diff --git a/turbopack/crates/turbo-persistence/README.md b/turbopack/crates/turbo-persistence/README.md index e8457b173b57..289b943ea310 100644 --- a/turbopack/crates/turbo-persistence/README.md +++ b/turbopack/crates/turbo-persistence/README.md @@ -12,7 +12,7 @@ It supports having multiple key families, which are stored in separate files, bu ## On disk format -There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when the database was last opened or committed to (`last_used_time`). The last-used time lives in the file rather than being taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. +There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when the database was last opened or committed to (`last_used_time`). The last-used time is stored in the file rather than taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. All other files have a sequence number as file name, e. g. `0000123.sst`. All files are immutable once their sequence number is <= the committed sequence number. But they might be deleted when they are superseded by other committed files. diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index 2ac675d4cf6c..726d3a174ca1 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -165,7 +165,7 @@ impl WriteOperationGuard<'_> { /// /// Serialized as a small JSON object. `last_used_time` lives in the file rather than being taken /// from its mtime because mtimes don't survive the directory being copied or restored (CI cache -/// restore, `cp -r`, container image builds), which would corrupt version eviction's idea of age. +/// restore, `cp -r`, container image builds). #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CurrentDbVersion { /// The highest sequence number that is part of the committed database. Files with a greater @@ -180,11 +180,9 @@ pub struct CurrentDbVersion { /// Returns `Ok(None)` if the file doesn't exist, which for a writable database means "not /// initialized yet". /// -/// A `CURRENT` that exists but doesn't parse is an error rather than something to recover from -/// here. Version directories are named after the build that wrote them, so a change to this format -/// comes with a new directory name and this function never sees an older one; anything unparsable -/// is corruption. Callers that scan directories they didn't write (see the cache-version eviction -/// in `turbo-tasks-backend`) are the ones that have to tolerate it. +/// A `CURRENT` that exists but doesn't parse is an error, not an older format to fall back on: +/// version directories are named after the build that wrote them, so a format change comes with a +/// new directory name and this function never sees an older one. pub fn read_current_version(path: &Path) -> Result> { let current_path = path.join("CURRENT"); let content = match fs::read(¤t_path) { @@ -654,15 +652,13 @@ impl TurboPersistence inner.meta_files = meta_files; inner.current_sequence_number = current; - // Refresh the last-used stamp. This happens even for a read-only open: opening to read is - // still a use, and eviction shouldn't treat a database as abandoned just because nothing - // wrote to it. Rewriting `CURRENT` is the only mutation a read-only open makes. + // Refresh the last-used stamp. This happens even for a read-only open — opening to read is + // still a use — and is the only mutation such an open makes. // - // Best-effort. On failure the stamp keeps its old value, so the database looks less - // recently used than it is and may be evicted early; that's recoverable, and an open that - // can otherwise succeed shouldn't fail over a cache-eviction hint. A failure part-way - // through can leave a stale `CURRENT.next` behind, which is harmless: the next - // `commit_current` truncates it. + // Best-effort: on failure the stamp keeps its old value, so the database looks less + // recently used than it is and may be evicted early. An open that can otherwise succeed + // shouldn't fail over a cache-eviction hint. A failure part-way through can leave a stale + // `CURRENT.next` behind, which the next `commit_current` truncates. let _ = commit_current(&self.path, current); Ok(true) diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index f4d896087ecd..1cb00d4f3770 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -2248,8 +2248,8 @@ fn current_file_is_json_with_last_used_time() -> Result<()> { } let after = jiff::Timestamp::now(); - // `CURRENT` is a stable on-disk format that external tools parse without going through this - // crate, so the JSON shape and these field names are a public contract, not an internal detail. + // External tools parse `CURRENT` without going through this crate, so these field names are a + // public contract. let raw = fs::read_to_string(path.join("CURRENT"))?; assert!(raw.contains("max_sequence_number"), "got: {raw}"); assert!(raw.contains("last_used_time"), "got: {raw}"); @@ -2282,8 +2282,8 @@ fn corrupt_current_file_fails_to_open() -> Result<()> { db.shutdown()?; } - // A truncated `CURRENT`, e.g. from an interrupted copy. Four bytes specifically: that used to - // be the whole file, and a length-based format guess would read it as a sequence number. + // A truncated `CURRENT`, e.g. from an interrupted copy. Four bytes specifically, so that a + // length-based format guess would misread it as a raw sequence number. fs::write(path.join("CURRENT"), [0u8; 4])?; assert!( diff --git a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs index d2702e004a6d..5ca0c8df5986 100644 --- a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs +++ b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs @@ -34,10 +34,10 @@ const DELETION_PREFIX: &str = "__stale_"; /// Given a base path, creates a version directory for the given `version_info`. Automatically /// cleans up old/stale databases. /// -/// Exactly one database whose version isn't the current one is retained — the most recently used, -/// and only if it was used within [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`] — so that switching back to -/// a branch you recently left still finds its cache intact. On CI none are retained. The current -/// version is always retained. +/// The current version is always retained. Alongside it, exactly one database whose version isn't +/// the current one is kept — the most recently used, and only if it was used within +/// [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`] — so that switching back to a branch you recently left +/// still finds its cache intact. On CI none are retained. /// /// **Environment Variables** /// - `TURBO_ENGINE_VERSION`: Forces use of a specific database version. @@ -105,9 +105,7 @@ pub fn handle_db_versioning( }; // Of the other versions we keep only the most recently used one, and only if it's - // within the TTL. Anything past the TTL is evicted as soon as we see it; the best - // candidate so far is held back until something more recent displaces it, and whatever - // is still held at the end is the one we keep. + // within the TTL. let mut newest: Option<(Duration, DirEntry)> = None; for entry in read_dir { let Ok(entry) = entry else { continue }; @@ -136,8 +134,8 @@ pub fn handle_db_versioning( continue; } - // With no TTL nothing is retained, so don't spend a read working out an age that - // can't change the outcome. + // With no TTL nothing is retained, so don't read an age that can't change the + // outcome — which also means a corrupt `CURRENT` can't fail the run. let Some(ttl) = ttl else { evict(entry); continue; @@ -150,7 +148,6 @@ pub fn handle_db_versioning( } match &newest { Some((newest_age, _)) if *newest_age <= age => evict(entry), - // This entry is more recent, so drop the one we were holding _ => { if let Some((_, previous)) = newest.replace((age, entry)) { evict(previous); @@ -159,9 +156,6 @@ pub fn handle_db_versioning( } } } - - // The selected version is stamped as used by the persistence layer when it opens the - // database, so there's nothing to record here. } else { path = base_path.join("temp"); if path.exists() { @@ -174,15 +168,13 @@ pub fn handle_db_versioning( Ok(path) } -/// How long to retain a database whose version isn't the current one, honoring the -/// `TURBO_ENGINE_VERSION_TTL_DAYS` override. Falls back to [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`] if -/// the variable is unset or unparsable. +/// How long to retain a database whose version isn't the current one. Falls back to +/// [`DEFAULT_OTHER_DB_VERSION_TTL_DAYS`] if `TURBO_ENGINE_VERSION_TTL_DAYS` is unset or unparsable. fn other_db_version_ttl() -> Duration { let Ok(raw) = env::var("TURBO_ENGINE_VERSION_TTL_DAYS") else { return ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS); }; - // `u64::from_str` accepts a leading `+`; require plain digits so the accepted syntax is - // exactly what the name promises. + // `u64::from_str` accepts a leading `+`; require plain digits instead. let trimmed = raw.trim(); let days = (!trimmed.is_empty() && trimmed.bytes().all(|b| b.is_ascii_digit())) .then(|| trimmed.parse::().ok()) @@ -206,14 +198,13 @@ fn ttl_from_days(days: u64) -> Duration { /// How long ago the version directory `entry` was last used, read from the `last_used_time` its /// `CURRENT` file records. /// -/// A directory with no `CURRENT` at all isn't a database we finished writing — access to the cache -/// root is serialized, so this can't be one that's mid-initialization — and gets [`Duration::MAX`] -/// so it's evicted ahead of any real cache. A `CURRENT` that exists but can't be read is a -/// different matter: that's an unexpected IO or corruption problem, and the error propagates rather -/// than being turned into a deletion. +/// A directory with no `CURRENT` isn't a database we finished writing — access to the cache root is +/// serialized, so it can't be one that's mid-initialization — and gets [`Duration::MAX`] so it's +/// evicted ahead of any real cache. A `CURRENT` that exists but can't be read is corruption or IO +/// failure: the error propagates rather than turning into a deletion. /// -/// A stamp in the future (backwards clock jump, or a copy from a machine with a fast clock) reads -/// as age zero, so a version is never evicted for looking too new. +/// A stamp in the future (clock skew) reads as age zero, so a version is never evicted for looking +/// too new. fn time_since_last_used(entry: &DirEntry) -> Result { let Some(version) = read_current_version(&entry.path())? else { return Ok(Duration::MAX); @@ -274,7 +265,10 @@ mod tests { #[rstest] #[case::not_ci(false, &["mock-version", "other-dir-0"])] #[case::ci(true, &["mock-version"])] - fn test_max_versions(#[case] is_ci: bool, #[case] expected: &[&str]) { + fn test_only_most_recently_used_other_version_is_retained( + #[case] is_ci: bool, + #[case] expected: &[&str], + ) { let tmp_dir = TempDir::new().unwrap(); let base_path = tmp_dir.path(); @@ -295,8 +289,8 @@ mod tests { assert_eq!(entry_names(base_path), expected); } - /// A version that hasn't been used within the TTL is evicted even though it's within the count - /// limit. + /// A version that hasn't been used within the TTL is evicted, even with the retention slot + /// free. #[test] fn test_ttl_evicts_unused_version() { let tmp_dir = TempDir::new().unwrap(); @@ -379,12 +373,11 @@ mod tests { ); } - /// An unparsable `TURBO_ENGINE_VERSION_TTL_DAYS` falls back to the default rather than - /// retaining nothing. + /// `TURBO_ENGINE_VERSION_TTL_DAYS` overrides the TTL, and an unparsable value falls back to the + /// default rather than retaining nothing. /// - /// Not `#[rstest]`-parameterized over several inputs: this mutates process-wide environment - /// state, so the cases can't run concurrently with each other or with any other test that - /// reads the same variable. + /// The cases share one test rather than being `#[rstest]`-parameterized because they mutate + /// process-wide environment state and so can't run concurrently. #[test] fn test_ttl_days_override() { // SAFETY: single-threaded test, and no other test reads this variable. @@ -428,7 +421,6 @@ mod tests { handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); - // age-1 is the most recently used, so it's the one that survives. assert_eq!(entry_names(base_path), vec!["age-1", CURRENT_VERSION]); } From 4c89392c3022a52f3e895bfb26e69629593a4d53 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 8 Aug 2026 16:32:17 -0700 Subject: [PATCH 3/7] Don't rewrite CURRENT on open Stamping last_used_time on every open cost two fsyncs (the CURRENT.next sync_data plus the directory sync) to refresh a hint compared against a 3-day TTL. Commits already stamp it, so any session that writes anything is covered; the open-time write only mattered for read-only or no-write sessions, which are rare and would shift the stamp by minutes against a threshold of days. Also restores open_read_only_with_parallel_scheduler to pass read_only: false to open_directory as it did before - that flag was only flipped to keep the open-time stamp write from running cleanup. --- turbopack/crates/turbo-persistence/README.md | 2 +- turbopack/crates/turbo-persistence/src/db.rs | 59 ++++--------------- .../crates/turbo-persistence/src/tests.rs | 27 ++++++--- 3 files changed, 31 insertions(+), 57 deletions(-) diff --git a/turbopack/crates/turbo-persistence/README.md b/turbopack/crates/turbo-persistence/README.md index 289b943ea310..582d63335ea6 100644 --- a/turbopack/crates/turbo-persistence/README.md +++ b/turbopack/crates/turbo-persistence/README.md @@ -12,7 +12,7 @@ It supports having multiple key families, which are stored in separate files, bu ## On disk format -There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when the database was last opened or committed to (`last_used_time`). The last-used time is stored in the file rather than taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. +There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when the database was last committed to (`last_used_time`). The last-used time is stored in the file rather than taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. All other files have a sequence number as file name, e. g. `0000123.sst`. All files are immutable once their sequence number is <= the committed sequence number. But they might be deleted when they are superseded by other committed files. diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index 726d3a174ca1..9d0329a729cf 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -2,7 +2,7 @@ use std::{ borrow::Cow, collections::HashSet, fmt::Display, - io::{BufWriter, Write}, + io::{BufWriter, ErrorKind, Write}, mem::take, ops::RangeInclusive, path::{Path, PathBuf}, @@ -162,16 +162,13 @@ impl WriteOperationGuard<'_> { /// The contents of the `CURRENT` file: which sequence number is committed, and when the database /// was last used. -/// -/// Serialized as a small JSON object. `last_used_time` lives in the file rather than being taken -/// from its mtime because mtimes don't survive the directory being copied or restored (CI cache -/// restore, `cp -r`, container image builds). #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CurrentDbVersion { /// The highest sequence number that is part of the committed database. Files with a greater /// sequence number are orphans from an interrupted write and get deleted on open. pub max_sequence_number: u32, - /// When this database was last opened or committed to. + /// When this database was last written. Opens that commit nothing don't update it, so this + /// tracks last write rather than last use. pub last_used_time: Timestamp, } @@ -179,15 +176,11 @@ pub struct CurrentDbVersion { /// /// Returns `Ok(None)` if the file doesn't exist, which for a writable database means "not /// initialized yet". -/// -/// A `CURRENT` that exists but doesn't parse is an error, not an older format to fall back on: -/// version directories are named after the build that wrote them, so a format change comes with a -/// new directory name and this function never sees an older one. pub fn read_current_version(path: &Path) -> Result> { let current_path = path.join("CURRENT"); let content = match fs::read(¤t_path) { Ok(content) => content, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(None), Err(e) => return Err(e).context("Failed to read CURRENT file"), }; @@ -211,42 +204,19 @@ pub fn read_current_version(path: &Path) -> Result> { /// (in-place overwrites, by contrast, can leave a partially-written value on a crash mid-write). /// After the rename we fsync the directory so the new `CURRENT` → inode mapping survives a crash. fn commit_current(path: &Path, seq: u32) -> Result<()> { - commit_current_version( - path, - &CurrentDbVersion { - max_sequence_number: seq, - last_used_time: Timestamp::now(), - }, - ) -} - -/// As [`commit_current`], but writes an explicit [`CurrentDbVersion`]. -fn commit_current_version(path: &Path, version: &CurrentDbVersion) -> Result<()> { - // Serialize up front and write once: `serde_json::to_writer` into an unbuffered `File` would - // issue a syscall per JSON token, and this runs on every commit and every open. + let version: &CurrentDbVersion = &CurrentDbVersion { + max_sequence_number: seq, + last_used_time: Timestamp::now(), + }; let mut contents = serde_json::to_string(version).context("Failed to serialize the CURRENT file")?; contents.push('\n'); - let next_path = path.join("CURRENT.next"); let mut next_file = File::create(&next_path)?; next_file.write_all(contents.as_bytes())?; next_file.sync_data()?; drop(next_file); - fs::rename(&next_path, path.join("CURRENT"))?; - - // Fsync the directory. This is the single durability barrier for a commit: by the time we get - // here every file created earlier in the commit (SST/meta/blob and any `.del` file) already - // exists, so this one fsync flushes *all* of their directory entries together with the CURRENT - // rename. Because the file *contents* were already `sync_data`'d before this call and the - // rename is the last directory mutation, a crash can never leave a durable CURRENT pointing at - // files whose directory entries were lost. Callers therefore do not need a separate directory - // fsync before invoking this. - // - // Skipped on Windows: `sync_data` on a directory handle fails with ERROR_ACCESS_DENIED (the - // handle `File::open` returns for a directory has no write access).Apparently metadata changes - // are always atomic on windows so this is simply unneeded. #[cfg(not(windows))] File::open(path) .and_then(|dir| dir.sync_data()) @@ -497,7 +467,7 @@ impl TurboPersistence parallel_scheduler, config, }); - db.open_directory(true)?; + db.open_directory(false)?; Ok(db) } @@ -518,7 +488,7 @@ impl TurboPersistence Ok(()) } Err(e) => { - if !read_only && e.kind() == std::io::ErrorKind::NotFound { + if !read_only && e.kind() == ErrorKind::NotFound { self.create_and_init_directory() .context("Creating and initializing persistence directory failed")?; Ok(()) @@ -652,15 +622,6 @@ impl TurboPersistence inner.meta_files = meta_files; inner.current_sequence_number = current; - // Refresh the last-used stamp. This happens even for a read-only open — opening to read is - // still a use — and is the only mutation such an open makes. - // - // Best-effort: on failure the stamp keeps its old value, so the database looks less - // recently used than it is and may be evicted early. An open that can otherwise succeed - // shouldn't fail over a cache-eviction hint. A failure part-way through can leave a stale - // `CURRENT.next` behind, which the next `commit_current` truncates. - let _ = commit_current(&self.path, current); - Ok(true) } diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index 1cb00d4f3770..d3ed3d652f36 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -2305,10 +2305,10 @@ fn corrupt_current_file_fails_to_open() -> Result<()> { Ok(()) } -/// Opening a database refreshes its last-used time even when nothing is written, so that a -/// read-only session still counts as a use for cache-version eviction. +/// The last-used time is stamped by commits, not by opens: an open that writes nothing leaves +/// `CURRENT` untouched, so it costs no fsync. #[test] -fn opening_a_database_refreshes_last_used_time() -> Result<()> { +fn opening_without_writing_leaves_last_used_time_alone() -> Result<()> { use crate::parallel_scheduler::SerialScheduler; let tempdir = tempfile::tempdir()?; @@ -2338,14 +2338,27 @@ fn opening_a_database_refreshes_last_used_time() -> Result<()> { db.shutdown()?; } - let refreshed = read_current_version(path)?.unwrap(); + let after_open = read_current_version(path)?.unwrap(); assert_eq!( - refreshed.max_sequence_number, stale.max_sequence_number, + after_open.max_sequence_number, stale.max_sequence_number, "sequence number must be preserved" ); + assert_eq!( + after_open.last_used_time, backdated, + "an open with no writes must not rewrite CURRENT" + ); + + // A commit does stamp it. + { + let db = TurboPersistence::::open(path.to_path_buf())?; + let batch = db.write_batch()?; + batch.put(0, vec![2u8], vec![43u8].into())?; + db.commit_write_batch(batch)?; + db.shutdown()?; + } assert!( - refreshed.last_used_time > backdated, - "last_used_time should be refreshed on open" + read_current_version(path)?.unwrap().last_used_time > backdated, + "committing must refresh last_used_time" ); Ok(()) From e983533448dfabc0c6d9970dd6bc1e8f305c55d8 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 8 Aug 2026 16:37:45 -0700 Subject: [PATCH 4/7] Rename CURRENT's last_used_time field to commit_time The field is only ever stamped by commits, so the old name overpromised. Renames the Rust field, the JSON key, and the TS reader, plus the db_versioning helper that consumes it (time_since_last_used -> time_since_last_commit). The retention concept stays 'last used' since a cache that's in use gets written to. --- packages/next/src/lib/turbopack-cache-seed.ts | 29 ++++++++++--------- turbopack/crates/turbo-persistence/README.md | 2 +- turbopack/crates/turbo-persistence/src/db.rs | 11 ++++--- .../crates/turbo-persistence/src/tests.rs | 24 +++++++-------- .../src/database/db_versioning.rs | 19 ++++++------ 5 files changed, 43 insertions(+), 42 deletions(-) diff --git a/packages/next/src/lib/turbopack-cache-seed.ts b/packages/next/src/lib/turbopack-cache-seed.ts index b02125e5f382..57d8f5b3c5d0 100644 --- a/packages/next/src/lib/turbopack-cache-seed.ts +++ b/packages/next/src/lib/turbopack-cache-seed.ts @@ -76,10 +76,10 @@ function findSeedSource( const currentWorktree = path.resolve(worktreeInfo.worktreeRoot) // We are going to find the best candidate worktree - // based on the most recently used cache directory. + // based on the most recently written cache directory. // We only look at our version - let best: { versionDir: string; lastUsedMs: number } | undefined + let best: { versionDir: string; commitTimeMs: number } | undefined for (const root of [ worktreeInfo.mainRepoRoot, ...listLinkedWorktreeRoots(worktreeInfo.mainRepoRoot), @@ -93,32 +93,33 @@ function findSeedSource( 'turbopack', version ) - const lastUsedMs = currentLastUsedMs(versionDir) - if (lastUsedMs === undefined) continue - if (!best || lastUsedMs > best.lastUsedMs) { - best = { versionDir, lastUsedMs } + const commitTimeMs = currentCommitTimeMs(versionDir) + if (commitTimeMs === undefined) continue + if (!best || commitTimeMs > best.commitTimeMs) { + best = { versionDir, commitTimeMs } } } return best?.versionDir } -// When the cache in `versionDir` was last used, in epoch milliseconds, or undefined if there is -// no usable cache there. Read from the `last_used_time` the persistence layer records in CURRENT. +// When the cache in `versionDir` was last committed to, in epoch milliseconds, or undefined if +// there is no usable cache there. Read from the `commit_time` the persistence layer records in +// CURRENT. // // No fallback for the pre-JSON CURRENT format: callers only look inside the directory named for // the running binary's own cache version, which no binary that old could have written. -function currentLastUsedMs(versionDir: string): number | undefined { - let lastUsed +function currentCommitTimeMs(versionDir: string): number | undefined { + let commitTime try { - lastUsed = JSON.parse( + commitTime = JSON.parse( fs.readFileSync(path.join(versionDir, 'CURRENT'), 'utf8') - ).last_used_time + ).commit_time } catch { // missing, unreadable, or not valid JSON - treat it as not a seed candidate return undefined } - if (typeof lastUsed !== 'string') return undefined - const parsed = Date.parse(lastUsed) + if (typeof commitTime !== 'string') return undefined + const parsed = Date.parse(commitTime) return Number.isNaN(parsed) ? undefined : parsed } diff --git a/turbopack/crates/turbo-persistence/README.md b/turbopack/crates/turbo-persistence/README.md index 582d63335ea6..7934eac48162 100644 --- a/turbopack/crates/turbo-persistence/README.md +++ b/turbopack/crates/turbo-persistence/README.md @@ -12,7 +12,7 @@ It supports having multiple key families, which are stored in separate files, bu ## On disk format -There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when the database was last committed to (`last_used_time`). The last-used time is stored in the file rather than taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. +There is a single `CURRENT` file, a small JSON object holding the latest committed sequence number (`max_sequence_number`) and when that commit happened (`commit_time`). The commit time is stored in the file rather than taken from its mtime so that it survives the directory being copied or restored. External tools read this file, so its field names are a stable contract. All other files have a sequence number as file name, e. g. `0000123.sst`. All files are immutable once their sequence number is <= the committed sequence number. But they might be deleted when they are superseded by other committed files. diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index 9d0329a729cf..a1a7dba67f0d 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -160,16 +160,15 @@ impl WriteOperationGuard<'_> { } } -/// The contents of the `CURRENT` file: which sequence number is committed, and when the database -/// was last used. +/// The contents of the `CURRENT` file: which sequence number is committed, and when that commit +/// happened. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CurrentDbVersion { /// The highest sequence number that is part of the committed database. Files with a greater /// sequence number are orphans from an interrupted write and get deleted on open. pub max_sequence_number: u32, - /// When this database was last written. Opens that commit nothing don't update it, so this - /// tracks last write rather than last use. - pub last_used_time: Timestamp, + /// When this database was last committed to. + pub commit_time: Timestamp, } /// Reads the `CURRENT` file in the database directory `path`. @@ -206,7 +205,7 @@ pub fn read_current_version(path: &Path) -> Result> { fn commit_current(path: &Path, seq: u32) -> Result<()> { let version: &CurrentDbVersion = &CurrentDbVersion { max_sequence_number: seq, - last_used_time: Timestamp::now(), + commit_time: Timestamp::now(), }; let mut contents = serde_json::to_string(version).context("Failed to serialize the CURRENT file")?; diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index d3ed3d652f36..3fba45773764 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -2230,9 +2230,9 @@ fn stale_current_next_is_recovered() -> Result<()> { Ok(()) } -/// `CURRENT` round-trips through JSON, recording both the sequence number and a last-used time. +/// `CURRENT` round-trips through JSON, recording both the sequence number and the commit time. #[test] -fn current_file_is_json_with_last_used_time() -> Result<()> { +fn current_file_is_json_with_commit_time() -> Result<()> { use crate::parallel_scheduler::SerialScheduler; let tempdir = tempfile::tempdir()?; @@ -2252,14 +2252,14 @@ fn current_file_is_json_with_last_used_time() -> Result<()> { // public contract. let raw = fs::read_to_string(path.join("CURRENT"))?; assert!(raw.contains("max_sequence_number"), "got: {raw}"); - assert!(raw.contains("last_used_time"), "got: {raw}"); + assert!(raw.contains("commit_time"), "got: {raw}"); let version = read_current_version(path)?.expect("CURRENT should exist"); assert!(version.max_sequence_number > 0); assert!( - version.last_used_time >= before && version.last_used_time <= after, - "last_used_time {} outside [{before}, {after}]", - version.last_used_time + version.commit_time >= before && version.commit_time <= after, + "commit_time {} outside [{before}, {after}]", + version.commit_time ); Ok(()) @@ -2305,10 +2305,10 @@ fn corrupt_current_file_fails_to_open() -> Result<()> { Ok(()) } -/// The last-used time is stamped by commits, not by opens: an open that writes nothing leaves +/// The commit time is stamped by commits, not by opens: an open that writes nothing leaves /// `CURRENT` untouched, so it costs no fsync. #[test] -fn opening_without_writing_leaves_last_used_time_alone() -> Result<()> { +fn opening_without_writing_leaves_commit_time_alone() -> Result<()> { use crate::parallel_scheduler::SerialScheduler; let tempdir = tempfile::tempdir()?; @@ -2329,7 +2329,7 @@ fn opening_without_writing_leaves_last_used_time_alone() -> Result<()> { path.join("CURRENT"), serde_json::to_vec(&CurrentDbVersion { max_sequence_number: stale.max_sequence_number, - last_used_time: backdated, + commit_time: backdated, })?, )?; @@ -2344,7 +2344,7 @@ fn opening_without_writing_leaves_last_used_time_alone() -> Result<()> { "sequence number must be preserved" ); assert_eq!( - after_open.last_used_time, backdated, + after_open.commit_time, backdated, "an open with no writes must not rewrite CURRENT" ); @@ -2357,8 +2357,8 @@ fn opening_without_writing_leaves_last_used_time_alone() -> Result<()> { db.shutdown()?; } assert!( - read_current_version(path)?.unwrap().last_used_time > backdated, - "committing must refresh last_used_time" + read_current_version(path)?.unwrap().commit_time > backdated, + "committing must refresh commit_time" ); Ok(()) diff --git a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs index 5ca0c8df5986..9e69fbd6c56a 100644 --- a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs +++ b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs @@ -141,7 +141,7 @@ pub fn handle_db_versioning( continue; }; - let age = time_since_last_used(&entry)?; + let age = time_since_last_commit(&entry)?; if age > ttl { evict(entry); continue; @@ -195,8 +195,9 @@ fn ttl_from_days(days: u64) -> Duration { Duration::from_secs(days.saturating_mul(24 * 60 * 60)) } -/// How long ago the version directory `entry` was last used, read from the `last_used_time` its -/// `CURRENT` file records. +/// How long ago the version directory `entry` was last committed to, read from the `commit_time` +/// its `CURRENT` file records. A cache that's in use gets written to, so this stands in for how +/// recently the version was used. /// /// A directory with no `CURRENT` isn't a database we finished writing — access to the cache root is /// serialized, so it can't be one that's mid-initialization — and gets [`Duration::MAX`] so it's @@ -205,12 +206,12 @@ fn ttl_from_days(days: u64) -> Duration { /// /// A stamp in the future (clock skew) reads as age zero, so a version is never evicted for looking /// too new. -fn time_since_last_used(entry: &DirEntry) -> Result { +fn time_since_last_commit(entry: &DirEntry) -> Result { let Some(version) = read_current_version(&entry.path())? else { return Ok(Duration::MAX); }; Ok(Timestamp::now() - .duration_since(version.last_used_time) + .duration_since(version.commit_time) .try_into() .unwrap_or_default()) } @@ -235,16 +236,16 @@ mod tests { } /// Creates a version directory that looks like a real database (i.e. has a `CURRENT` file), - /// last used `used_ago` in the past. - fn create_version_dir(base_path: &Path, name: &str, used_ago: Duration) { + /// last committed to `committed_ago` in the past. + fn create_version_dir(base_path: &Path, name: &str, committed_ago: Duration) { let path = base_path.join(name); fs::create_dir(&path).unwrap(); - let last_used_time = Timestamp::now() - jiff::SignedDuration::try_from(used_ago).unwrap(); + let commit_time = Timestamp::now() - jiff::SignedDuration::try_from(committed_ago).unwrap(); fs::write( path.join("CURRENT"), serde_json::to_vec(&CurrentDbVersion { max_sequence_number: 0, - last_used_time, + commit_time, }) .unwrap(), ) From 54aee4c156355d63e87a314b48ee5ad638829eb7 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 8 Aug 2026 16:50:08 -0700 Subject: [PATCH 5/7] Drop low-value db versioning tests Removes three tests that weren't earning their keep: - test_ttl_days_override: exercised a trivial parse routine at the cost of unsafe process-wide env mutation and a SAFETY obligation nothing enforced. - test_ttl_retains_recently_used_version: the not_ci case of test_only_most_recently_used_other_version_is_retained already covers retaining a within-TTL version. - test_survivor_independent_of_scan_order: named for a property it couldn't establish, since read_dir order isn't controllable from the test. The min-tracking it actually covered is trivial. With the parse no longer under test, also drops the hand-rolled digits-only guard in favor of plain u64 parsing, which now accepts a leading '+'. --- .../src/database/db_versioning.rs | 83 +------------------ 1 file changed, 3 insertions(+), 80 deletions(-) diff --git a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs index 9e69fbd6c56a..ee5a2f1cf6fa 100644 --- a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs +++ b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs @@ -174,14 +174,9 @@ fn other_db_version_ttl() -> Duration { let Ok(raw) = env::var("TURBO_ENGINE_VERSION_TTL_DAYS") else { return ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS); }; - // `u64::from_str` accepts a leading `+`; require plain digits instead. - let trimmed = raw.trim(); - let days = (!trimmed.is_empty() && trimmed.bytes().all(|b| b.is_ascii_digit())) - .then(|| trimmed.parse::().ok()) - .flatten(); - match days { - Some(days) => ttl_from_days(days), - None => { + match raw.trim().parse::() { + Ok(days) => ttl_from_days(days), + Err(_) => { eprintln!( "WARNING: Ignoring TURBO_ENGINE_VERSION_TTL_DAYS={raw:?}, expected a whole number \ of days." @@ -309,27 +304,6 @@ mod tests { assert_eq!(entry_names(base_path), vec![CURRENT_VERSION]); } - /// A version used within the TTL is retained. - #[test] - fn test_ttl_retains_recently_used_version() { - let tmp_dir = TempDir::new().unwrap(); - let base_path = tmp_dir.path(); - - create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); - create_version_dir( - base_path, - "recent-version", - ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS) - Duration::from_secs(60), - ); - - handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); - - assert_eq!( - entry_names(base_path), - vec!["mock-version", "recent-version"] - ); - } - /// A directory with no `CURRENT` file isn't one of ours, so it's evicted rather than occupying /// the single retention slot — even when it holds recently written files. #[rstest] @@ -374,57 +348,6 @@ mod tests { ); } - /// `TURBO_ENGINE_VERSION_TTL_DAYS` overrides the TTL, and an unparsable value falls back to the - /// default rather than retaining nothing. - /// - /// The cases share one test rather than being `#[rstest]`-parameterized because they mutate - /// process-wide environment state and so can't run concurrently. - #[test] - fn test_ttl_days_override() { - // SAFETY: single-threaded test, and no other test reads this variable. - unsafe { - env::set_var("TURBO_ENGINE_VERSION_TTL_DAYS", "7"); - } - assert_eq!(other_db_version_ttl(), ttl_from_days(7)); - - unsafe { - env::set_var("TURBO_ENGINE_VERSION_TTL_DAYS", "not-a-number"); - } - assert_eq!( - other_db_version_ttl(), - ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS), - "an unparsable value should fall back to the default" - ); - - unsafe { - env::remove_var("TURBO_ENGINE_VERSION_TTL_DAYS"); - } - assert_eq!( - other_db_version_ttl(), - ttl_from_days(DEFAULT_OTHER_DB_VERSION_TTL_DAYS) - ); - } - - /// The survivor is the most recently used regardless of the order `read_dir` yields entries, - /// including when the eventual winner is seen last and displaces an earlier candidate. - #[rstest] - #[case::ascending(&[1u64, 2, 3, 4])] - #[case::descending(&[4u64, 3, 2, 1])] - #[case::winner_last(&[3u64, 2, 4, 1])] - fn test_survivor_independent_of_scan_order(#[case] ages: &[u64]) { - let tmp_dir = TempDir::new().unwrap(); - let base_path = tmp_dir.path(); - - create_version_dir(base_path, CURRENT_VERSION, Duration::ZERO); - for age in ages { - create_version_dir(base_path, &format!("age-{age}"), Duration::from_secs(*age)); - } - - handle_db_versioning(base_path, &version_info(), /* is_ci */ false).unwrap(); - - assert_eq!(entry_names(base_path), vec!["age-1", CURRENT_VERSION]); - } - /// On CI every other version is evicted regardless of age, so an unreadable `CURRENT` in one of /// them is never consulted and can't fail the run. #[test] From 6c36a28fcf9efb8b8cc956f9ead1bfe05b5a4ce3 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 8 Aug 2026 19:17:30 -0700 Subject: [PATCH 6/7] remove more tests simplify comments --- packages/next/src/lib/turbopack-cache-seed.ts | 3 - turbopack/crates/turbo-persistence/src/db.rs | 15 ++- .../crates/turbo-persistence/src/tests.rs | 101 +----------------- .../src/database/db_versioning.rs | 9 +- 4 files changed, 15 insertions(+), 113 deletions(-) diff --git a/packages/next/src/lib/turbopack-cache-seed.ts b/packages/next/src/lib/turbopack-cache-seed.ts index 57d8f5b3c5d0..876d1aef4477 100644 --- a/packages/next/src/lib/turbopack-cache-seed.ts +++ b/packages/next/src/lib/turbopack-cache-seed.ts @@ -105,9 +105,6 @@ function findSeedSource( // When the cache in `versionDir` was last committed to, in epoch milliseconds, or undefined if // there is no usable cache there. Read from the `commit_time` the persistence layer records in // CURRENT. -// -// No fallback for the pre-JSON CURRENT format: callers only look inside the directory named for -// the running binary's own cache version, which no binary that old could have written. function currentCommitTimeMs(versionDir: string): number | undefined { let commitTime try { diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index a1a7dba67f0d..9f56c44379dd 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -164,8 +164,7 @@ impl WriteOperationGuard<'_> { /// happened. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CurrentDbVersion { - /// The highest sequence number that is part of the committed database. Files with a greater - /// sequence number are orphans from an interrupted write and get deleted on open. + /// The highest sequence number that is part of the committed database. pub max_sequence_number: u32, /// When this database was last committed to. pub commit_time: Timestamp, @@ -216,6 +215,17 @@ fn commit_current(path: &Path, seq: u32) -> Result<()> { next_file.sync_data()?; drop(next_file); fs::rename(&next_path, path.join("CURRENT"))?; + // Fsync the directory. This is the single durability barrier for a commit: by the time we get + // here every file created earlier in the commit (SST/meta/blob and any `.del` file) already + // exists, so this one fsync flushes *all* of their directory entries together with the CURRENT + // rename. Because the file *contents* were already `sync_data`'d before this call and the + // rename is the last directory mutation, a crash can never leave a durable CURRENT pointing at + // files whose directory entries were lost. Callers therefore do not need a separate directory + // fsync before invoking this. + // + // Skipped on Windows: `sync_data` on a directory handle fails with ERROR_ACCESS_DENIED (the + // handle `File::open` returns for a directory has no write access).Apparently metadata changes + // are always atomic on windows so this is simply unneeded. #[cfg(not(windows))] File::open(path) .and_then(|dir| dir.sync_data()) @@ -620,7 +630,6 @@ impl TurboPersistence .store(meta_files.is_empty(), Ordering::Relaxed); inner.meta_files = meta_files; inner.current_sequence_number = current; - Ok(true) } diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index 3fba45773764..be3dac9f824f 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -2248,7 +2248,7 @@ fn current_file_is_json_with_commit_time() -> Result<()> { } let after = jiff::Timestamp::now(); - // External tools parse `CURRENT` without going through this crate, so these field names are a + // next.js parses `CURRENT` without going through this crate, so these field names are a // public contract. let raw = fs::read_to_string(path.join("CURRENT"))?; assert!(raw.contains("max_sequence_number"), "got: {raw}"); @@ -2264,102 +2264,3 @@ fn current_file_is_json_with_commit_time() -> Result<()> { Ok(()) } - -/// A `CURRENT` that exists but doesn't parse is corruption, and opening must fail loudly rather -/// than silently treating the database as empty — that would orphan and delete every SST. -#[test] -fn corrupt_current_file_fails_to_open() -> Result<()> { - use crate::parallel_scheduler::SerialScheduler; - - let tempdir = tempfile::tempdir()?; - let path = tempdir.path(); - - { - let db = TurboPersistence::::open(path.to_path_buf())?; - let batch = db.write_batch()?; - batch.put(0, vec![1u8], vec![42u8].into())?; - db.commit_write_batch(batch)?; - db.shutdown()?; - } - - // A truncated `CURRENT`, e.g. from an interrupted copy. Four bytes specifically, so that a - // length-based format guess would misread it as a raw sequence number. - fs::write(path.join("CURRENT"), [0u8; 4])?; - - assert!( - read_current_version(path).is_err(), - "a truncated CURRENT must be reported as corrupt, not parsed" - ); - assert!( - TurboPersistence::::open(path.to_path_buf()).is_err(), - "opening a database with a corrupt CURRENT must fail" - ); - - // The data must still be on disk: a failed open must not have deleted anything. - let ssts = fs::read_dir(path)? - .filter_map(|e| e.ok()) - .filter(|e| e.path().extension().is_some_and(|ext| ext == "sst")) - .count(); - assert!(ssts > 0, "a failed open must not delete SST files"); - - Ok(()) -} - -/// The commit time is stamped by commits, not by opens: an open that writes nothing leaves -/// `CURRENT` untouched, so it costs no fsync. -#[test] -fn opening_without_writing_leaves_commit_time_alone() -> Result<()> { - use crate::parallel_scheduler::SerialScheduler; - - let tempdir = tempfile::tempdir()?; - let path = tempdir.path(); - - { - let db = TurboPersistence::::open(path.to_path_buf())?; - let batch = db.write_batch()?; - batch.put(0, vec![1u8], vec![42u8].into())?; - db.commit_write_batch(batch)?; - db.shutdown()?; - } - - // Backdate the recorded timestamp, leaving the sequence number intact. - let stale = read_current_version(path)?.unwrap(); - let backdated = jiff::Timestamp::now() - jiff::SignedDuration::from_hours(72); - fs::write( - path.join("CURRENT"), - serde_json::to_vec(&CurrentDbVersion { - max_sequence_number: stale.max_sequence_number, - commit_time: backdated, - })?, - )?; - - { - let db = TurboPersistence::::open(path.to_path_buf())?; - db.shutdown()?; - } - - let after_open = read_current_version(path)?.unwrap(); - assert_eq!( - after_open.max_sequence_number, stale.max_sequence_number, - "sequence number must be preserved" - ); - assert_eq!( - after_open.commit_time, backdated, - "an open with no writes must not rewrite CURRENT" - ); - - // A commit does stamp it. - { - let db = TurboPersistence::::open(path.to_path_buf())?; - let batch = db.write_batch()?; - batch.put(0, vec![2u8], vec![43u8].into())?; - db.commit_write_batch(batch)?; - db.shutdown()?; - } - assert!( - read_current_version(path)?.unwrap().commit_time > backdated, - "committing must refresh commit_time" - ); - - Ok(()) -} diff --git a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs index ee5a2f1cf6fa..5431c5ed8808 100644 --- a/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs +++ b/turbopack/crates/turbo-tasks-backend/src/database/db_versioning.rs @@ -191,16 +191,11 @@ fn ttl_from_days(days: u64) -> Duration { } /// How long ago the version directory `entry` was last committed to, read from the `commit_time` -/// its `CURRENT` file records. A cache that's in use gets written to, so this stands in for how -/// recently the version was used. +/// its `CURRENT` file records /// /// A directory with no `CURRENT` isn't a database we finished writing — access to the cache root is /// serialized, so it can't be one that's mid-initialization — and gets [`Duration::MAX`] so it's -/// evicted ahead of any real cache. A `CURRENT` that exists but can't be read is corruption or IO -/// failure: the error propagates rather than turning into a deletion. -/// -/// A stamp in the future (clock skew) reads as age zero, so a version is never evicted for looking -/// too new. +/// evicted ahead of any real cache fn time_since_last_commit(entry: &DirEntry) -> Result { let Some(version) = read_current_version(&entry.path())? else { return Ok(Duration::MAX); From 824e0576b99c4289c2d7c69f8e3e541345bf792e Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Sat, 8 Aug 2026 19:26:44 -0700 Subject: [PATCH 7/7] fix clip --- turbopack/crates/turbo-persistence/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index be3dac9f824f..da007139c160 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -6,7 +6,7 @@ use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::{ DbConfig, FamilyConfig, FamilyKind, constants::{MAX_MEDIUM_VALUE_SIZE, MAX_SMALL_VALUE_SIZE}, - db::{CompactConfig, CurrentDbVersion, TurboPersistence, read_current_version}, + db::{CompactConfig, TurboPersistence, read_current_version}, parallel_scheduler::ParallelScheduler, write_batch::WriteBatch, };