From ddde75e85034feb4561316143df24b50ab663c7c Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sat, 17 Oct 2026 23:29:23 +0200 Subject: [PATCH 1/7] mason: handle storage folders on separate volumes (EXDEV fallback, mount-point retire, per-volume preflight) Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/backup.rs | 57 +++++- crates/aft/src/bash_background/mod.rs | 61 +++++- crates/aft/src/callgraph_store/mod.rs | 55 ++++-- crates/aft/src/commands/state.rs | 50 ++++- crates/aft/src/compress/mod.rs | 35 +++- crates/aft/src/cross_volume.rs | 269 ++++++++++++++++++++++++++ crates/aft/src/lib.rs | 1 + crates/aft/src/migrate_storage.rs | 191 ++++++++++++++++-- crates/aft/src/storage_retention.rs | 103 +++++++++- crates/aft/src/walk_boundary.rs | 85 +++++++- 10 files changed, 855 insertions(+), 52 deletions(-) create mode 100644 crates/aft/src/cross_volume.rs diff --git a/crates/aft/src/backup.rs b/crates/aft/src/backup.rs index 214ecf35a..f4e54e8ec 100644 --- a/crates/aft/src/backup.rs +++ b/crates/aft/src/backup.rs @@ -2980,8 +2980,10 @@ impl BackupStore { if harness_backups.exists() { let _ = std::fs::remove_dir(&harness_backups); } - match std::fs::rename(&root_backups, &harness_backups) { - Ok(()) => { + // The harness folder and the root normally share a volume, but a move + // that crosses volumes falls back to a copy instead of failing. + match crate::cross_volume::move_path(&root_backups, &harness_backups) { + Ok(_) => { crate::slog_info!( "moved legacy root backups into harness namespace: {}", harness_backups.display() @@ -2993,18 +2995,35 @@ impl BackupStore { harness_backups.display(), error ); - if create_private_durable_dir(&harness_backups).is_err() { + if let Err(error) = create_private_durable_dir(&harness_backups) { + crate::slog_warn!( + "legacy root backups kept in {}: cannot create {}: {}", + root_backups.display(), + harness_backups.display(), + error + ); return; } if let Ok(entries) = std::fs::read_dir(&root_backups) { for entry in entries.flatten() { let source = entry.path(); let target = harness_backups.join(entry.file_name()); - if !target.exists() { - let _ = std::fs::rename(source, target); + if target.exists() { + continue; + } + if let Err(error) = crate::cross_volume::move_path(&source, &target) { + // The backup stays at its legacy path; say so + // instead of dropping the failure. + crate::slog_warn!( + "legacy backup {} kept in place: cannot move it to {}: {}", + source.display(), + target.display(), + error + ); } } } + // Removes the legacy folder only when every backup left it. let _ = std::fs::remove_dir(&root_backups); } } @@ -7064,6 +7083,34 @@ mod tests { let _ = fs::remove_dir_all(&dir_b); } + #[test] + fn legacy_root_backups_move_into_a_harness_folder_on_another_volume() { + let storage = tempfile::tempdir().unwrap(); + let root_backups = storage.path().join("backups"); + fs::create_dir_all(root_backups.join("session-a")).unwrap(); + fs::write(root_backups.join("session-a/meta.json"), b"{}").unwrap(); + let harness = crate::harness::Harness::Opencode; + let harness_dir = storage.path().join(harness.storage_segment()); + fs::create_dir_all(&harness_dir).unwrap(); + // Every rename from the root into the harness folder now fails with EXDEV. + let _volume = + crate::walk_boundary::simulated_volumes::simulate(&harness_dir, u64::MAX - 21); + + let mut store = BackupStore::new(); + store.set_storage_dir_for_harness(storage.path().to_path_buf(), harness, 72); + store.repair_root_backups_if_needed(); + + assert_eq!( + fs::read(harness_dir.join("backups/session-a/meta.json")).unwrap(), + b"{}", + "the backup history must reach the harness folder" + ); + assert!( + !root_backups.exists(), + "the legacy folder is removed after the copy" + ); + } + #[test] fn restore_last_operation_restores_all_top_entries_for_same_op() { let path_a = temp_file("op_restore_a.txt", "a1"); diff --git a/crates/aft/src/bash_background/mod.rs b/crates/aft/src/bash_background/mod.rs index 19673cee2..c6b073d4f 100644 --- a/crates/aft/src/bash_background/mod.rs +++ b/crates/aft/src/bash_background/mod.rs @@ -1350,8 +1350,10 @@ pub fn repair_legacy_root_tasks(storage_root: &std::path::Path, harness: crate:: let _ = std::fs::remove_dir(&harness_tasks); } - match std::fs::rename(&root_tasks, &harness_tasks) { - Ok(()) => crate::slog_info!( + // The harness folder and the root normally share a volume, but a move + // that crosses volumes falls back to a copy instead of failing. + match crate::cross_volume::move_path(&root_tasks, &harness_tasks) { + Ok(_) => crate::slog_info!( "moved legacy root bash tasks into harness namespace: {}", harness_tasks.display() ), @@ -1361,18 +1363,35 @@ pub fn repair_legacy_root_tasks(storage_root: &std::path::Path, harness: crate:: harness_tasks.display(), error ); - if crate::private_storage::create_dir_all(&harness_tasks).is_err() { + if let Err(error) = crate::private_storage::create_dir_all(&harness_tasks) { + crate::slog_warn!( + "legacy root bash tasks kept in {}: cannot create {}: {}", + root_tasks.display(), + harness_tasks.display(), + error + ); return; } if let Ok(entries) = std::fs::read_dir(&root_tasks) { for entry in entries.flatten() { let source = entry.path(); let target = harness_tasks.join(entry.file_name()); - if !target.exists() { - let _ = std::fs::rename(source, target); + if target.exists() { + continue; + } + if let Err(error) = crate::cross_volume::move_path(&source, &target) { + // The task stays readable at its legacy path; say so + // instead of dropping the failure. + crate::slog_warn!( + "legacy bash task {} kept in place: cannot move it to {}: {}", + source.display(), + target.display(), + error + ); } } } + // Removes the legacy folder only when every task left it. let _ = std::fs::remove_dir(&root_tasks); } } @@ -1624,3 +1643,35 @@ mod shell_resolution_tests { ); } } + +#[cfg(test)] +mod legacy_root_task_repair_tests { + use std::fs; + + use crate::harness::Harness; + use crate::walk_boundary::simulated_volumes::simulate; + + #[test] + fn legacy_root_tasks_move_into_a_harness_folder_on_another_volume() { + let storage = tempfile::tempdir().unwrap(); + let root_tasks = storage.path().join("bash-tasks"); + fs::create_dir_all(root_tasks.join("task-1")).unwrap(); + fs::write(root_tasks.join("task-1/meta.json"), b"{\"id\":1}").unwrap(); + let harness_dir = storage.path().join(Harness::Opencode.storage_segment()); + fs::create_dir_all(&harness_dir).unwrap(); + // Every rename from the root into the harness folder now fails with EXDEV. + let _volume = simulate(&harness_dir, u64::MAX - 20); + + super::repair_legacy_root_tasks(storage.path(), Harness::Opencode); + + assert_eq!( + fs::read(harness_dir.join("bash-tasks/task-1/meta.json")).unwrap(), + b"{\"id\":1}", + "the task must reach the harness folder" + ); + assert!( + !root_tasks.exists(), + "the legacy folder is removed after the copy" + ); + } +} diff --git a/crates/aft/src/callgraph_store/mod.rs b/crates/aft/src/callgraph_store/mod.rs index a70e3a1cd..1cf2af1fc 100644 --- a/crates/aft/src/callgraph_store/mod.rs +++ b/crates/aft/src/callgraph_store/mod.rs @@ -12485,24 +12485,29 @@ fn sweep_orphaned_build_temps_store_wide(callgraph_dir: &Path) { }; let mut skipped_foreign_mounts = 0usize; - // Root-keyed layout: every `/callgraph/` directory. + // Root-keyed layout: every `/callgraph/` directory. The + // callgraph folder may be a separate volume mounted under the storage + // root, so its children are compared against the callgraph folder's own + // device. Skipping that whole folder as "foreign" would leave every other + // key's orphaned build temporaries on it forever. let root_keyed_dir = storage_root.join(domain); if root_keyed_dir.is_dir() { - if boundary.should_descend(&root_keyed_dir).unwrap_or(false) { - if let Ok(entries) = std::fs::read_dir(&root_keyed_dir) { - for entry in entries.flatten() { - let path = entry.path(); - if path.is_dir() { - if boundary.should_descend(&path).unwrap_or(false) { - sweep_orphaned_build_temps(&path); - } else { - skipped_foreign_mounts += 1; + match crate::walk_boundary::DeviceBoundary::for_root(&root_keyed_dir) { + Ok(domain_boundary) => { + if let Ok(entries) = std::fs::read_dir(&root_keyed_dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + if domain_boundary.should_descend(&path).unwrap_or(false) { + sweep_orphaned_build_temps(&path); + } else { + skipped_foreign_mounts += 1; + } } } } } - } else { - skipped_foreign_mounts += 1; + Err(_) => skipped_foreign_mounts += 1, } } @@ -24954,6 +24959,32 @@ mod cold_build_insert_tests { ); } + #[test] + fn orphan_temp_sweep_reaches_a_callgraph_folder_mounted_from_another_volume() { + let storage = tempdir().unwrap(); + let storage_root = storage.path(); + let callgraph = storage_root.join("callgraph"); + let dead_dir = callgraph.join("deadkey"); + fs::create_dir_all(&dead_dir).unwrap(); + let orphan = "deadkey.g100.1.sqlite.tmp.1.200"; + write_build_temp_with_age( + &dead_dir, + orphan, + ORPHANED_BUILD_TEMP_MIN_AGE + Duration::from_secs(60), + ); + let live_dir = callgraph.join("livekey"); + fs::create_dir_all(&live_dir).unwrap(); + // The callgraph folder is a separate volume from the storage root. + let _volume = crate::walk_boundary::simulated_volumes::simulate(&callgraph, u64::MAX - 40); + + sweep_orphaned_build_temps_store_wide(&live_dir); + + assert!( + !dead_dir.join(orphan).exists(), + "another key's orphan on the mounted callgraph folder must be reclaimed" + ); + } + #[test] fn orphan_temp_sweep_negative_control_age_predicate_is_what_spares_fresh() { // NEGATIVE CONTROL, mutation-proved: forcing the age predicate to accept diff --git a/crates/aft/src/commands/state.rs b/crates/aft/src/commands/state.rs index 0dfb96b27..42c4c6fb9 100644 --- a/crates/aft/src/commands/state.rs +++ b/crates/aft/src/commands/state.rs @@ -320,12 +320,32 @@ fn repair_root_scoped_harness_file( } if let Some(parent) = harness_path.parent() { - if crate::private_storage::create_dir_all(parent).is_err() { - return harness_path; + if let Err(error) = crate::private_storage::create_dir_all(parent) { + crate::slog_warn!( + "keeping {} at the storage root: cannot create {}: {}", + root_path.display(), + parent.display(), + error + ); + return root_path; + } + } + // The harness folder and the root normally share a volume, but a move + // that crosses volumes falls back to a copy instead of failing. If the + // move still fails, keep using the root file rather than pointing at a + // harness path that holds nothing. + match crate::cross_volume::move_path(&root_path, &harness_path) { + Ok(_) => harness_path, + Err(error) => { + crate::slog_warn!( + "keeping {} at the storage root: cannot move it to {}: {}", + root_path.display(), + harness_path.display(), + error + ); + root_path } } - let _ = fs::rename(root_path, &harness_path); - harness_path } fn legacy_host_path(ctx: &AppContext, key: &str) -> Option { @@ -451,4 +471,26 @@ mod tests { let path = temp.path().join("trusted-filter-projects.json"); assert_eq!(std::fs::read_to_string(&path).unwrap(), "[]"); } + + #[test] + fn root_state_file_moves_into_a_harness_folder_on_another_volume() { + let temp = tempfile::tempdir().unwrap(); + let ctx = test_context(); + ctx.update_config(|config| { + config.storage_dir = Some(temp.path().to_path_buf()); + }); + std::fs::write(temp.path().join("last_announced_version"), "0.40.0").unwrap(); + let harness_dir = temp.path().join("opencode"); + std::fs::create_dir_all(&harness_dir).unwrap(); + // Every rename from the root into the harness folder now fails with EXDEV. + let _volume = + crate::walk_boundary::simulated_volumes::simulate(&harness_dir, u64::MAX - 22); + + let path = legacy_harness_path(&ctx, Harness::Opencode, "last_announced_version") + .expect("known key"); + + assert_eq!(path, harness_dir.join("last_announced_version")); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "0.40.0"); + assert!(!temp.path().join("last_announced_version").exists()); + } } diff --git a/crates/aft/src/compress/mod.rs b/crates/aft/src/compress/mod.rs index 979a67e38..061740473 100644 --- a/crates/aft/src/compress/mod.rs +++ b/crates/aft/src/compress/mod.rs @@ -1544,7 +1544,18 @@ pub(crate) fn repair_legacy_user_filter_dir(storage_dir: &Path, harness: Harness if target.exists() { continue; } - let _ = fs::rename(entry.path(), target); + // The harness folder and the root normally share a volume, but a move + // that crosses volumes falls back to a copy instead of failing. + if let Err(error) = crate::cross_volume::move_path(&entry.path(), &target) { + // The filter stays at its legacy path; say so instead of dropping + // the failure. + crate::slog_warn!( + "legacy filter {} kept in place: cannot move it to {}: {}", + entry.path().display(), + target.display(), + error + ); + } } if fs::read_dir(&legacy_dir) @@ -1661,6 +1672,28 @@ mod tests { ); assert!(!storage.join("filters/root-only.toml").exists()); } + + #[test] + fn repair_legacy_user_filter_dir_moves_filters_into_a_harness_folder_on_another_volume() { + let temp = tempfile::tempdir().unwrap(); + let storage = temp.path(); + fs::create_dir_all(storage.join("filters")).unwrap(); + fs::create_dir_all(storage.join("opencode/filters")).unwrap(); + fs::write(storage.join("filters/root-only.toml"), "root").unwrap(); + // Every rename from the root into the harness folder now fails with EXDEV. + let _volume = crate::walk_boundary::simulated_volumes::simulate( + &storage.join("opencode"), + u64::MAX - 23, + ); + + repair_legacy_user_filter_dir(storage, Harness::Opencode); + + assert_eq!( + fs::read_to_string(storage.join("opencode/filters/root-only.toml")).unwrap(), + "root" + ); + assert!(!storage.join("filters").exists()); + } } #[cfg(test)] diff --git a/crates/aft/src/cross_volume.rs b/crates/aft/src/cross_volume.rs new file mode 100644 index 000000000..82fe09b76 --- /dev/null +++ b/crates/aft/src/cross_volume.rs @@ -0,0 +1,269 @@ +//! Moves between storage folders that may sit on different volumes. +//! +//! The rebuildable cache folders under the storage root (views, blobs, +//! callgraph, semantic, index, symbols, inspect) can be placed on a separate +//! volume, as a mount point or through a symlink. A `rename(2)` from one +//! volume to another fails with `EXDEV` (`ERROR_NOT_SAME_DEVICE` on Windows), +//! so a move whose source and destination folders can differ in volume goes +//! through [`move_path`], which falls back to a copy that is published by a +//! same-folder rename in the destination and only then removes the source. +//! +//! [`rename`] is the plain rename every such move starts with. Tests make it +//! report the cross-device error for folders registered with +//! `crate::walk_boundary::simulated_volumes`, because CI has no second volume. + +use std::fs; +use std::io; +use std::path::{Path, PathBuf}; + +/// True when `error` is the operating system's "cannot move across volumes" +/// error. +pub(crate) fn is_cross_device(error: &io::Error) -> bool { + #[cfg(unix)] + if error.raw_os_error() == Some(libc::EXDEV) { + return true; + } + // ERROR_NOT_SAME_DEVICE + #[cfg(windows)] + if error.raw_os_error() == Some(17) { + return true; + } + error.kind() == io::ErrorKind::CrossesDevices +} + +#[cfg(test)] +fn cross_device_error() -> io::Error { + #[cfg(unix)] + return io::Error::from_raw_os_error(libc::EXDEV); + #[cfg(windows)] + return io::Error::from_raw_os_error(17); + #[cfg(not(any(unix, windows)))] + return io::Error::from(io::ErrorKind::CrossesDevices); +} + +/// `fs::rename`, except that under test a move between two folders on +/// different simulated volumes fails exactly as a real cross-volume rename. +pub(crate) fn rename(from: &Path, to: &Path) -> io::Result<()> { + #[cfg(test)] + { + use crate::walk_boundary::simulated_volumes::device_of; + let source = device_of(from); + let destination = to.parent().and_then(device_of); + if (source.is_some() || destination.is_some()) && source != destination { + return Err(cross_device_error()); + } + } + fs::rename(from, to) +} + +/// True when `path` is on a different volume from its parent folder, which is +/// what a mount point looks like. False when either device cannot be read +/// (Windows standard metadata has no volume identifier). +pub(crate) fn is_mount_point(path: &Path) -> io::Result { + let Some(parent) = path.parent() else { + return Ok(false); + }; + let own = crate::walk_boundary::filesystem_device_id(path)?; + let above = crate::walk_boundary::filesystem_device_id(parent)?; + Ok(matches!((own, above), (Some(own), Some(above)) if own != above)) +} + +/// How [`move_path`] moved its source. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum Moved { + /// One rename: source and destination are on the same volume. + Renamed, + /// The volumes differ: the source was copied, published under the + /// destination name, and then removed. + Copied, +} + +/// Move the file or directory `from` to `to`, which must not exist. +/// +/// A same-volume move is one rename. A cross-volume move copies `from` into a +/// staging name inside `to`'s own folder, syncs every copied file and folder, +/// renames the staging copy to `to`, and removes `from` last. If the copy +/// fails, the staging copy is removed and `from` is left untouched, so an +/// error never loses data. If only the final removal of `from` fails, the +/// move has still happened: the leftover source is logged and `Ok` returned. +pub(crate) fn move_path(from: &Path, to: &Path) -> io::Result { + let error = match rename(from, to) { + Ok(()) => return Ok(Moved::Renamed), + Err(error) => error, + }; + if !is_cross_device(&error) { + return Err(error); + } + if fs::symlink_metadata(to).is_ok() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "cannot move {} across volumes: {} already exists", + from.display(), + to.display() + ), + )); + } + let parent = to + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + let staging = staging_path(parent, to)?; + if let Err(error) = copy_tree(from, &staging).and_then(|()| { + // Same folder, so this rename stays on the destination volume. + fs::rename(&staging, to)?; + crate::durability::sync_dir(parent) + }) { + remove_path(&staging).ok(); + return Err(io::Error::new( + error.kind(), + format!( + "cannot move {} across volumes to {}: {error}; the source was left in place", + from.display(), + to.display() + ), + )); + } + if let Err(error) = remove_path(from) { + crate::slog_warn!( + "moved {} across volumes to {} but could not remove the source: {error}", + from.display(), + to.display() + ); + } + Ok(Moved::Copied) +} + +fn staging_path(parent: &Path, to: &Path) -> io::Result { + let mut nonce = [0_u8; 8]; + getrandom::fill(&mut nonce).map_err(io::Error::other)?; + Ok(parent.join(format!( + ".{}.aft-move-{}-{:016x}", + to.file_name().unwrap_or_default().to_string_lossy(), + std::process::id(), + u64::from_le_bytes(nonce) + ))) +} + +/// Copy `from` to the new path `to`, keeping permissions, and sync it. +/// Symlinks are recreated as links (never followed); any other special file +/// is refused rather than silently skipped. +fn copy_tree(from: &Path, to: &Path) -> io::Result<()> { + let metadata = fs::symlink_metadata(from)?; + let kind = metadata.file_type(); + if kind.is_symlink() { + #[cfg(unix)] + return std::os::unix::fs::symlink(fs::read_link(from)?, to); + #[cfg(not(unix))] + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!("cannot copy symlink {} across volumes", from.display()), + )); + } + if kind.is_file() { + fs::copy(from, to)?; + fs::File::open(to)?.sync_all()?; + return Ok(()); + } + if !kind.is_dir() { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!("cannot copy special file {} across volumes", from.display()), + )); + } + fs::create_dir(to)?; + for entry in fs::read_dir(from)? { + let entry = entry?; + copy_tree(&entry.path(), &to.join(entry.file_name()))?; + } + fs::set_permissions(to, metadata.permissions())?; + crate::durability::sync_dir(to) +} + +fn remove_path(path: &Path) -> io::Result<()> { + match fs::symlink_metadata(path) { + Ok(metadata) if metadata.is_dir() => fs::remove_dir_all(path), + Ok(_) => fs::remove_file(path), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::walk_boundary::simulated_volumes::simulate; + + #[test] + fn cross_volume_move_copies_publishes_and_removes_the_source() { + let temp = tempfile::tempdir().unwrap(); + let source = temp.path().join("source"); + fs::create_dir_all(source.join("nested")).unwrap(); + fs::write(source.join("a.json"), b"first").unwrap(); + fs::write(source.join("nested/b.bin"), b"second").unwrap(); + let other = temp.path().join("other-volume"); + fs::create_dir(&other).unwrap(); + let _volume = simulate(&other, u64::MAX - 1); + let destination = other.join("moved"); + + assert!( + is_cross_device(&rename(&source, &destination).unwrap_err()), + "the seam must reproduce the kernel's cross-volume refusal" + ); + assert_eq!(move_path(&source, &destination).unwrap(), Moved::Copied); + + assert!(!source.exists(), "the source is removed after the copy"); + assert_eq!(fs::read(destination.join("a.json")).unwrap(), b"first"); + assert_eq!( + fs::read(destination.join("nested/b.bin")).unwrap(), + b"second" + ); + let leftovers = fs::read_dir(&other) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect::>(); + assert_eq!(leftovers, vec![std::ffi::OsString::from("moved")]); + } + + #[test] + fn same_volume_move_is_one_rename() { + let temp = tempfile::tempdir().unwrap(); + let source = temp.path().join("file"); + fs::write(&source, b"x").unwrap(); + let destination = temp.path().join("renamed"); + assert_eq!(move_path(&source, &destination).unwrap(), Moved::Renamed); + assert_eq!(fs::read(destination).unwrap(), b"x"); + } + + #[test] + fn cross_volume_move_refuses_an_existing_destination_and_keeps_the_source() { + let temp = tempfile::tempdir().unwrap(); + let source = temp.path().join("state.json"); + fs::write(&source, b"keep").unwrap(); + let other = temp.path().join("other-volume"); + fs::create_dir(&other).unwrap(); + let _volume = simulate(&other, u64::MAX - 2); + let destination = other.join("state.json"); + fs::write(&destination, b"existing").unwrap(); + + let error = move_path(&source, &destination).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::AlreadyExists); + assert_eq!(fs::read(&source).unwrap(), b"keep"); + assert_eq!(fs::read(&destination).unwrap(), b"existing"); + } + + #[test] + fn a_folder_on_another_device_than_its_parent_is_a_mount_point() { + let temp = tempfile::tempdir().unwrap(); + let mounted = temp.path().join("views"); + fs::create_dir(&mounted).unwrap(); + let plain = temp.path().join("plain"); + fs::create_dir(&plain).unwrap(); + // Register the parent too so the comparison also works where the + // platform reports no real device number. + let _parent = simulate(temp.path(), u64::MAX - 3); + let _volume = simulate(&mounted, u64::MAX - 4); + assert!(is_mount_point(&mounted).unwrap()); + assert!(!is_mount_point(&plain).unwrap()); + } +} diff --git a/crates/aft/src/lib.rs b/crates/aft/src/lib.rs index d5d9d89a0..5bd75f395 100644 --- a/crates/aft/src/lib.rs +++ b/crates/aft/src/lib.rs @@ -79,6 +79,7 @@ pub mod config_fix; pub mod config_live; pub mod config_resolve; pub mod context; +pub(crate) mod cross_volume; pub(crate) mod database_open; pub mod db; pub mod developer_tools; diff --git a/crates/aft/src/migrate_storage.rs b/crates/aft/src/migrate_storage.rs index c02928e3e..703507a2d 100644 --- a/crates/aft/src/migrate_storage.rs +++ b/crates/aft/src/migrate_storage.rs @@ -156,17 +156,17 @@ pub fn run_with_options(args: Args, options: Options) -> ExitStatus { } }; - let free_bytes = match options.disk_free_override { - Some(bytes) => Ok(bytes), - None => free_bytes_at(&args.to), - }; - let free_bytes = match free_bytes { - Ok(bytes) => bytes, - Err(error) => { + // Each destination volume must hold twice the bytes headed for it. The + // cache folders (index, semantic, symbols) may be on a different volume + // from the target root, so the root's free space alone says nothing about + // them. + let volumes = match destination_volumes(&args, source_bytes, &options) { + Ok(volumes) => volumes, + Err((path, error)) => { log.write(serde_json::json!({ "level": "error", "step": "preflight", - "path": args.to, + "path": path, "status": "disk_check_failed", "bytes_source": source_bytes, "error": error.to_string(), @@ -174,16 +174,21 @@ pub fn run_with_options(args: Args, options: Options) -> ExitStatus { return ExitStatus::MigrationFailed; } }; - let required = source_bytes.saturating_mul(2); - let has_space = free_bytes >= required; - log.write(serde_json::json!({ - "level": if has_space { "info" } else { "error" }, - "step": "preflight", - "bytes_source": source_bytes, - "bytes_free": free_bytes, - "bytes_required": required, - "ok": has_space, - })); + let mut has_space = true; + for volume in &volumes { + let required = volume.source_bytes.saturating_mul(2); + let ok = volume.free_bytes >= required; + has_space &= ok; + log.write(serde_json::json!({ + "level": if ok { "info" } else { "error" }, + "step": "preflight", + "path": volume.path, + "bytes_source": volume.source_bytes, + "bytes_free": volume.free_bytes, + "bytes_required": required, + "ok": ok, + })); + } if !has_space { return ExitStatus::InsufficientDisk; } @@ -765,11 +770,97 @@ fn source_size(path: &Path) -> io::Result { Ok(total) } +/// One destination volume of a migration and the source bytes headed for it. +struct DestinationVolume { + /// A folder on that volume: the target root, or the first destination + /// found on another volume. + path: PathBuf, + source_bytes: u64, + free_bytes: u64, +} + +/// Group the bytes to migrate by the volume each destination is on, and read +/// each volume's free space. The target root's volume is charged with every +/// byte not headed to another volume; when all destinations share it, that is +/// the whole source tree, as the check has always counted. An error names the +/// path whose volume could not be read. +fn destination_volumes( + args: &MigrationArgs, + source_bytes: u64, + options: &Options, +) -> Result, (PathBuf, io::Error)> { + let device = |path: &Path| { + crate::walk_boundary::filesystem_device_id(existing_ancestor(path)) + .map_err(|error| (path.to_path_buf(), error)) + }; + let free_at = |path: &Path| { + options + .disk_free_override + .map_or_else(|| free_bytes_at(path), Ok) + .map_err(|error| (path.to_path_buf(), error)) + }; + let root_device = device(&args.to)?; + let mut elsewhere: Vec<(Option, PathBuf, u64)> = Vec::new(); + for &item in migration_items() { + let destination = target_path(args, item); + let destination_device = device(&destination)?; + if destination_device == root_device { + continue; + } + let source = args.from.join(item.source_name); + let bytes = source_size(&source).map_err(|error| (source, error))?; + match elsewhere + .iter_mut() + .find(|(device, _, _)| *device == destination_device) + { + Some(volume) => volume.2 = volume.2.saturating_add(bytes), + None => elsewhere.push(( + destination_device, + existing_ancestor(&destination).to_path_buf(), + bytes, + )), + } + } + let moved_elsewhere = elsewhere + .iter() + .fold(0_u64, |total, (_, _, bytes)| total.saturating_add(*bytes)); + let mut volumes = vec![DestinationVolume { + path: args.to.clone(), + source_bytes: source_bytes.saturating_sub(moved_elsewhere), + free_bytes: free_at(&args.to)?, + }]; + for (_, path, bytes) in elsewhere { + volumes.push(DestinationVolume { + free_bytes: free_at(&path)?, + path, + source_bytes: bytes, + }); + } + Ok(volumes) +} + +#[cfg(test)] +thread_local! { + /// Free bytes reported for folders at or under each path, so a test can + /// give a simulated second volume less room than the target root. + static FREE_BYTES_FOR_TEST: std::cell::RefCell> = + const { std::cell::RefCell::new(Vec::new()) }; +} + #[cfg(unix)] fn free_bytes_at(path: &Path) -> io::Result { use std::ffi::CString; use std::os::unix::ffi::OsStrExt; + #[cfg(test)] + if let Some(bytes) = FREE_BYTES_FOR_TEST.with(|slot| { + slot.borrow() + .iter() + .find(|(root, _)| path.starts_with(root)) + .map(|(_, bytes)| *bytes) + }) { + return Ok(bytes); + } let probe = existing_ancestor(path); let c_path = CString::new(probe.as_os_str().as_bytes()) .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "path contains NUL byte"))?; @@ -786,10 +877,18 @@ fn free_bytes_at(path: &Path) -> io::Result { fn free_bytes_at(_path: &Path) -> io::Result { // v0.27 is Unix-prioritized. Windows disk preflight should be wired to // GetDiskFreeSpaceExW through windows-sys in a follow-up. + #[cfg(test)] + if let Some(bytes) = FREE_BYTES_FOR_TEST.with(|slot| { + slot.borrow() + .iter() + .find(|(root, _)| _path.starts_with(root)) + .map(|(_, bytes)| *bytes) + }) { + return Ok(bytes); + } Ok(u64::MAX) } -#[cfg(unix)] fn existing_ancestor(path: &Path) -> &Path { let mut current = path; while !current.exists() { @@ -950,6 +1049,60 @@ fn after_publish(source: &Path, target: &Path) { mod tests { use super::*; + #[test] + fn preflight_checks_free_space_on_the_volume_of_each_destination() { + let dir = tempfile::tempdir().unwrap(); + let from = dir.path().join("legacy"); + fs::create_dir_all(from.join("index/key")).unwrap(); + fs::write(from.join("index/key/cache.bin"), vec![0_u8; 1000]).unwrap(); + fs::create_dir_all(from.join("backups")).unwrap(); + fs::write(from.join("backups/meta.json"), b"{}").unwrap(); + let to = dir.path().join("target"); + let index = to.join("index"); + fs::create_dir_all(&index).unwrap(); + // The index folder is a separate volume with room for less than twice + // the index bytes; the target root itself has plenty of room. + let _volume = crate::walk_boundary::simulated_volumes::simulate(&index, u64::MAX - 30); + FREE_BYTES_FOR_TEST + .with(|slot| *slot.borrow_mut() = vec![(index.clone(), 1500), (to.clone(), u64::MAX)]); + let log = dir.path().join("migration.jsonl"); + + let status = run_with_options( + Args { + from: Some(from), + to: to.clone(), + harness: Harness::Opencode, + log: Some(log.clone()), + status: false, + }, + Options::default(), + ); + FREE_BYTES_FOR_TEST.with(|slot| slot.borrow_mut().clear()); + + assert_eq!(status, ExitStatus::InsufficientDisk); + let preflight = fs::read_to_string(&log) + .unwrap() + .lines() + .map(|line| serde_json::from_str::(line).unwrap()) + .filter(|record| record["step"] == "preflight") + .collect::>(); + let index_line = preflight + .iter() + .find(|record| record["path"] == serde_json::json!(index)) + .expect("the index volume is checked on its own"); + assert_eq!(index_line["bytes_source"], 1000); + assert_eq!(index_line["ok"], false); + let root_line = preflight + .iter() + .find(|record| record["path"] == serde_json::json!(to)) + .expect("the target root is checked"); + assert_eq!( + root_line["bytes_source"], 2, + "bytes headed to the index volume are not charged to the root" + ); + assert_eq!(root_line["ok"], true); + } + #[test] fn migration_marker_does_not_truncate_a_colliding_temporary() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/aft/src/storage_retention.rs b/crates/aft/src/storage_retention.rs index a692ae70d..73b6cc4e7 100644 --- a/crates/aft/src/storage_retention.rs +++ b/crates/aft/src/storage_retention.rs @@ -1101,10 +1101,18 @@ fn validate_tree( /// Keep trash on the cache's filesystem so retiring a root is a single rename. /// A failed unlink or daemon exit leaves a recognizable directory for retry. +/// +/// A cache folder that is itself a mount point (its own volume, separate from +/// the domain folder above it) cannot be renamed into the domain's trash: the +/// rename would cross volumes, and a mount point cannot be moved anyway. Its +/// contents are retired instead, see `retire_mount_point_contents`. fn retire_directory(cache: &Path) -> io::Result<()> { let parent = cache .parent() .ok_or_else(|| io::Error::other("missing cache parent"))?; + if crate::cross_volume::is_mount_point(cache)? { + return retire_mount_point_contents(cache); + } let trash = parent.join(".retention-trash"); fs::create_dir_all(&trash)?; if !fs::symlink_metadata(&trash)?.is_dir() { @@ -1114,12 +1122,53 @@ fn retire_directory(cache: &Path) -> io::Result<()> { test_hook(cache, "rename-root"); let mut nonce = [0_u8; 16]; getrandom::fill(&mut nonce).map_err(io::Error::other)?; - fs::rename( + crate::cross_volume::rename( cache, - trash.join(format!("retired-{:032x}", u128::from_le_bytes(nonce))), + &trash.join(format!("retired-{:032x}", u128::from_le_bytes(nonce))), ) } +/// Retire a mount-point cache folder by emptying it. Its entries are first +/// renamed into a trash folder inside the mount, so every rename stays on that +/// volume and a crash leaves one recognizable `retired-*` folder. The trash is +/// then deleted right away: the background reaper only looks in the domain +/// folder's trash, which is on the other volume. The caller has already +/// validated the tree, which bounds it to `TREE_ENTRIES` entries. The empty +/// mount point stays, and `emptied_mount_point` makes later passes treat it +/// as absent. +fn retire_mount_point_contents(cache: &Path) -> io::Result<()> { + let trash = cache.join(".retention-trash"); + fs::create_dir_all(&trash)?; + if !fs::symlink_metadata(&trash)?.is_dir() { + return Err(io::Error::other("non-directory retention trash retained")); + } + #[cfg(test)] + test_hook(cache, "rename-root"); + let mut nonce = [0_u8; 16]; + getrandom::fill(&mut nonce).map_err(io::Error::other)?; + let retired = trash.join(format!("retired-{:032x}", u128::from_le_bytes(nonce))); + fs::create_dir(&retired)?; + for entry in fs::read_dir(cache)? { + let entry = entry?; + if entry.file_name() == ".retention-trash" { + continue; + } + crate::cross_volume::rename(&entry.path(), &retired.join(entry.file_name()))?; + } + fs::remove_dir_all(&trash) +} + +/// True for a mount-point cache folder that `retire_mount_point_contents` +/// already emptied (or that never held anything). The folder itself cannot be +/// removed, so without this check every later pass would find the key again +/// and retire it again. +fn emptied_mount_point(cache: &Path) -> bool { + crate::cross_volume::is_mount_point(cache).unwrap_or(false) + && fs::read_dir(cache).is_ok_and(|mut entries| { + entries.all(|entry| entry.is_ok_and(|entry| entry.file_name() == ".retention-trash")) + }) +} + fn reap_retired(storage: &Path) -> io::Result<()> { // Different daemon processes may discover the same tombstones. This lock // is separate from admission: slow unlinks must not block binds or pins. @@ -1514,6 +1563,10 @@ fn candidate_inner( } else { directory.join(key) }; + if !flat && emptied_mount_point(&cache) { + // Already retired: only the mount point itself is left. + return Ok(()); + } if !flat && history .get(key) @@ -1771,7 +1824,9 @@ fn due_first( if *flat { continue; } - if fs::symlink_metadata(directory.join(&key)).is_ok_and(|metadata| metadata.is_dir()) { + if fs::symlink_metadata(directory.join(&key)).is_ok_and(|metadata| metadata.is_dir()) + && !emptied_mount_point(&directory.join(&key)) + { present = true; if directory == &storage.join("views") { report.due_views_remaining += 1; @@ -3234,6 +3289,45 @@ mod storage_retention_tests { assert!(path.exists()); } + #[test] + fn retention_retires_a_mount_point_cache_by_emptying_it_on_its_own_volume() { + let (temp, _, _, path) = due_cache_fixture(); + fs::create_dir_all(path.join("nested")).unwrap(); + fs::write(path.join("nested/derived.bin"), b"rebuildable").unwrap(); + // The domain folder and the cache folder below it report different + // devices, as when a separate volume is mounted at the cache folder. + // Renaming the cache into `views/.retention-trash` would then fail + // with EXDEV. + let _views = crate::walk_boundary::simulated_volumes::simulate( + &temp.path().join("views"), + u64::MAX - 10, + ); + let _mount = crate::walk_boundary::simulated_volumes::simulate(&path, u64::MAX - 11); + + let report = run_pass(temp.path(), &|| false); + assert_eq!(report.removed_roots, 1, "{report:?}"); + assert!(report.errors.is_empty(), "{report:?}"); + assert!(path.is_dir(), "the mount point itself must stay"); + assert_eq!( + fs::read_dir(&path).unwrap().count(), + 0, + "every entry of the mounted cache is retired" + ); + assert!( + !temp.path().join("views/.retention-trash").exists() + || fs::read_dir(temp.path().join("views/.retention-trash")) + .unwrap() + .count() + == 0, + "nothing crossed into the domain folder's trash" + ); + + // The emptied mount point counts as gone, so the next pass does not + // retire it again. + let report = run_pass(temp.path(), &|| false); + assert_eq!(report.removed_roots, 0, "{report:?}"); + } + #[test] fn retention_reaper_resumes_retired_roots_without_a_pass() { let (temp, _, _, path) = due_cache_fixture(); @@ -4161,7 +4255,8 @@ fn clear_unknown_observation(storage: &Path, key: &str) -> io::Result<()> { fn cache_key_present(storage: &Path, key: &str, deadline: Instant) -> io::Result { for (directory, flat) in domain_paths(storage)? { if !flat { - if directory.join(key).try_exists()? { + let cache = directory.join(key); + if cache.try_exists()? && !emptied_mount_point(&cache) { return Ok(true); } } else if directory.try_exists()? { diff --git a/crates/aft/src/walk_boundary.rs b/crates/aft/src/walk_boundary.rs index 57b300f5f..085f79059 100644 --- a/crates/aft/src/walk_boundary.rs +++ b/crates/aft/src/walk_boundary.rs @@ -133,18 +133,99 @@ pub(crate) fn same_filesystem_device(root_device: u64, child_device: u64) -> boo pub(crate) fn filesystem_device_id(path: &Path) -> io::Result> { use std::os::unix::fs::MetadataExt; - Ok(Some(std::fs::metadata(path)?.dev())) + let metadata = std::fs::metadata(path)?; + #[cfg(test)] + if let Some(device) = simulated_volumes::device_of(path) { + return Ok(Some(device)); + } + Ok(Some(metadata.dev())) } #[cfg(not(unix))] -pub(crate) fn filesystem_device_id(_path: &Path) -> io::Result> { +pub(crate) fn filesystem_device_id(path: &Path) -> io::Result> { // Standard metadata on non-Unix platforms has file attributes but no // reliable volume identifier, so this raw-directory-walk check is a no-op. // Recursive walks using `ignore::WalkBuilder` still enforce their own // cross-filesystem restriction. + #[cfg(test)] + { + std::fs::metadata(path)?; + if let Some(device) = simulated_volumes::device_of(path) { + return Ok(Some(device)); + } + } + let _ = path; Ok(None) } +/// Test seam that makes chosen folders look like separate volumes. +/// +/// CI has no second filesystem to mount, so tests register a folder with a +/// made-up device number. `filesystem_device_id` then reports that number for +/// the folder and everything under it, and `crate::cross_volume::rename` +/// refuses a move between two different simulated devices with the same +/// cross-device error the kernel returns. Registrations are keyed by path, so +/// tests running in parallel in their own temporary folders do not see each +/// other's volumes. +#[cfg(test)] +pub(crate) mod simulated_volumes { + use std::path::{Path, PathBuf}; + use std::sync::{Mutex, OnceLock}; + + fn registry() -> &'static Mutex> { + static VOLUMES: OnceLock>> = OnceLock::new(); + VOLUMES.get_or_init(Default::default) + } + + /// Removes its registration when dropped. + #[must_use] + pub(crate) struct SimulatedVolume { + paths: Vec, + } + + impl Drop for SimulatedVolume { + fn drop(&mut self) { + registry() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .retain(|(path, _)| !self.paths.contains(path)); + } + } + + /// Report `path` and everything below it as device `device`. + pub(crate) fn simulate(path: &Path, device: u64) -> SimulatedVolume { + let mut paths = vec![path.to_path_buf()]; + // Register the resolved spelling too (macOS /var is /private/var), so + // a caller that canonicalized the path still matches. + if let Ok(canonical) = std::fs::canonicalize(path) { + if canonical != path { + paths.push(canonical); + } + } + registry() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .extend(paths.iter().map(|path| (path.clone(), device))); + SimulatedVolume { paths } + } + + /// The simulated device of the innermost registered folder containing + /// `path`, or `None` when no registered folder contains it. + pub(crate) fn device_of(path: &Path) -> Option { + let volumes = registry() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if volumes.is_empty() { + return None; + } + volumes + .iter() + .filter(|(root, _)| path.starts_with(root)) + .max_by_key(|(root, _)| root.components().count()) + .map(|(_, device)| *device) + } +} + #[cfg(test)] mod tests { use std::io; From dff71eb6a439a536af75440f0b00a8a1c8838120 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 00:40:56 +0200 Subject: [PATCH 2/7] mason: clarify comments on legacy-folder move failures Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/backup.rs | 5 +++-- crates/aft/src/bash_background/mod.rs | 5 +++-- crates/aft/src/compress/mod.rs | 4 ++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/aft/src/backup.rs b/crates/aft/src/backup.rs index f4e54e8ec..9ec4969af 100644 --- a/crates/aft/src/backup.rs +++ b/crates/aft/src/backup.rs @@ -3012,8 +3012,9 @@ impl BackupStore { continue; } if let Err(error) = crate::cross_volume::move_path(&source, &target) { - // The backup stays at its legacy path; say so - // instead of dropping the failure. + // A backup that cannot be moved stays in the root + // `backups/` folder; log the move failure instead + // of dropping it. crate::slog_warn!( "legacy backup {} kept in place: cannot move it to {}: {}", source.display(), diff --git a/crates/aft/src/bash_background/mod.rs b/crates/aft/src/bash_background/mod.rs index c6b073d4f..aadc059df 100644 --- a/crates/aft/src/bash_background/mod.rs +++ b/crates/aft/src/bash_background/mod.rs @@ -1380,8 +1380,9 @@ pub fn repair_legacy_root_tasks(storage_root: &std::path::Path, harness: crate:: continue; } if let Err(error) = crate::cross_volume::move_path(&source, &target) { - // The task stays readable at its legacy path; say so - // instead of dropping the failure. + // A task that cannot be moved stays readable in the + // root `bash-tasks/` folder; log the move failure + // instead of dropping it. crate::slog_warn!( "legacy bash task {} kept in place: cannot move it to {}: {}", source.display(), diff --git a/crates/aft/src/compress/mod.rs b/crates/aft/src/compress/mod.rs index 061740473..7e226d8f6 100644 --- a/crates/aft/src/compress/mod.rs +++ b/crates/aft/src/compress/mod.rs @@ -1547,8 +1547,8 @@ pub(crate) fn repair_legacy_user_filter_dir(storage_dir: &Path, harness: Harness // The harness folder and the root normally share a volume, but a move // that crosses volumes falls back to a copy instead of failing. if let Err(error) = crate::cross_volume::move_path(&entry.path(), &target) { - // The filter stays at its legacy path; say so instead of dropping - // the failure. + // A filter that cannot be moved stays in the root `filters/` + // folder; log the move failure instead of dropping it. crate::slog_warn!( "legacy filter {} kept in place: cannot move it to {}: {}", entry.path().display(), From fd8bd279a7794fa6ee468ef256686bc2bd13b575 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 00:42:52 +0200 Subject: [PATCH 3/7] release notes and search-quality descriptor for storage-cross-volume Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .alfonso/release-notes/next-notes.md | 1 + .../slice-descriptors/land-storage-cross-volume.json | 12 ++++++++++++ 2 files changed, 13 insertions(+) create mode 100644 benchmarks/aft-search/slice-descriptors/land-storage-cross-volume.json diff --git a/.alfonso/release-notes/next-notes.md b/.alfonso/release-notes/next-notes.md index 74cb240bc..c7dbbe51c 100644 --- a/.alfonso/release-notes/next-notes.md +++ b/.alfonso/release-notes/next-notes.md @@ -22,6 +22,7 @@ - gh shim (manifest 19): `gh workflow disable` and `gh workflow enable` run under the operator bypass, like `run cancel`, and are refused as undeclared on older manifests. - Backups: the daemon writes its ENGRAM backup catalog (`engram-catalog.json` in the storage root) at startup, so a fresh machine is covered without a hand-written file. It declares only what exists right now: `aft.db` (captured page by page) and the trusted-filter project list as device-local, and undo history, checkpoints, background task output, logs, diagnostics, caches and generated shims as excluded from backup. Undo history keeps its earlier entry ids, so ENGRAM records the switch to excluded on the same entries, and an id stays with its folder across restarts. The catalog is written only when the storage folder is named `aft`, since ENGRAM looks nowhere else. - Background commands: a command that finishes while AFT restarts is now reported with its real exit code, not as an unknown outcome; replay reads the exit marker again after it finds the command's process gone. +- Storage on several volumes: legacy folder moves, state-file repair and filter-folder repair fall back to copying (or keep the original and say so) when a storage folder sits on another volume, retention retires a cache folder that is itself a mount point by emptying it on its own volume, and the storage migration checks free space on each destination volume. ## Sandbox profiles diff --git a/benchmarks/aft-search/slice-descriptors/land-storage-cross-volume.json b/benchmarks/aft-search/slice-descriptors/land-storage-cross-volume.json new file mode 100644 index 000000000..03e1d2c0b --- /dev/null +++ b/benchmarks/aft-search/slice-descriptors/land-storage-cross-volume.json @@ -0,0 +1,12 @@ +{ + "targeted_mechanism": "none", + "kind": "harness", + "slice_class": "engine_unwired", + "fixtures": [ + "crates/aft/tests/integration/tool_call_parity_test.rs" + ], + "fenced_files": [ + "crates/aft/src/lib.rs" + ], + "note": "The only fenced change is a module declaration in lib.rs for the cross-volume rename helper. Ranking and routing are unchanged; benchmark rows must remain byte-identical." +} From 8fef6b41808be5b18ca3c797bfc9d14fd992a611 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:22:26 +0200 Subject: [PATCH 4/7] mason: reap mount-local retention trash outside admission (#504) Discover mount-point tombstones without binding history, keep pending trash distinct from an emptied mount, and defer recursive unlinks to the reaper. Snapshot mount-local retirements under admission, then validate and unlink outside both admission and SQLite's filesystem guard. Retry container creation if a previous reaper removes it concurrently. Add interrupted-retirement and lock-separation regressions. Update the existing mount-point test to await background reaping rather than assuming synchronous unlink; its reclamation assertions are unchanged. Both new tests fail before the fixes and each fails independently with its corresponding fix removed. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/storage_retention.rs | 277 +++++++++++++++++++++++----- 1 file changed, 230 insertions(+), 47 deletions(-) diff --git a/crates/aft/src/storage_retention.rs b/crates/aft/src/storage_retention.rs index 73b6cc4e7..3161db7c9 100644 --- a/crates/aft/src/storage_retention.rs +++ b/crates/aft/src/storage_retention.rs @@ -1130,24 +1130,31 @@ fn retire_directory(cache: &Path) -> io::Result<()> { /// Retire a mount-point cache folder by emptying it. Its entries are first /// renamed into a trash folder inside the mount, so every rename stays on that -/// volume and a crash leaves one recognizable `retired-*` folder. The trash is -/// then deleted right away: the background reaper only looks in the domain -/// folder's trash, which is on the other volume. The caller has already -/// validated the tree, which bounds it to `TREE_ENTRIES` entries. The empty -/// mount point stays, and `emptied_mount_point` makes later passes treat it -/// as absent. +/// volume and a crash leaves recognizable `retired-*` folders. Only move entries +/// while the caller holds admission and SQLite's filesystem guard; the reaper +/// discovers mount-local trash independently and does the slow unlink later. +/// The mount point itself stays even after its retired contents are reclaimed. fn retire_mount_point_contents(cache: &Path) -> io::Result<()> { let trash = cache.join(".retention-trash"); - fs::create_dir_all(&trash)?; - if !fs::symlink_metadata(&trash)?.is_dir() { - return Err(io::Error::other("non-directory retention trash retained")); - } #[cfg(test)] test_hook(cache, "rename-root"); let mut nonce = [0_u8; 16]; getrandom::fill(&mut nonce).map_err(io::Error::other)?; let retired = trash.join(format!("retired-{:032x}", u128::from_le_bytes(nonce))); - fs::create_dir(&retired)?; + loop { + fs::create_dir_all(&trash)?; + if !fs::symlink_metadata(&trash)?.is_dir() { + return Err(io::Error::other("non-directory retention trash retained")); + } + match fs::create_dir(&retired) { + Ok(()) => break, + // A previous reaper can remove the empty container after taking + // its snapshot, but cannot discover this new tombstone until the + // caller releases admission. Recreate the container if it vanished. + Err(error) if error.kind() == io::ErrorKind::NotFound => continue, + Err(error) => return Err(error), + } + } for entry in fs::read_dir(cache)? { let entry = entry?; if entry.file_name() == ".retention-trash" { @@ -1155,20 +1162,92 @@ fn retire_mount_point_contents(cache: &Path) -> io::Result<()> { } crate::cross_volume::rename(&entry.path(), &retired.join(entry.file_name()))?; } - fs::remove_dir_all(&trash) + Ok(()) } -/// True for a mount-point cache folder that `retire_mount_point_contents` -/// already emptied (or that never held anything). The folder itself cannot be -/// removed, so without this check every later pass would find the key again -/// and retire it again. +/// Retired data still counts as present until the reaper has reclaimed it. fn emptied_mount_point(cache: &Path) -> bool { + crate::cross_volume::is_mount_point(cache).unwrap_or(false) + && fs::read_dir(cache).is_ok_and(|mut entries| entries.next().is_none()) +} + +/// Do not retire an empty mount again, or create more tombstones while its +/// previous retirement awaits reaping. The reaper scans these mounts even if +/// no binding history or live cache contents remain. +fn retired_mount_point(cache: &Path) -> bool { crate::cross_volume::is_mount_point(cache).unwrap_or(false) && fs::read_dir(cache).is_ok_and(|mut entries| { entries.all(|entry| entry.is_ok_and(|entry| entry.file_name() == ".retention-trash")) }) } +fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> { + let entries = { + // Snapshot mount-local tombstones under admission so a concurrent + // retirement finishes moving entries before any of them are unlinked. + // Domain-local retirement publishes a whole tree with one rename. + let _admission = if mount_local { + match crate::fs_lock::try_acquire(&storage.join("retention/sweep.lock"), Duration::ZERO) + { + Ok(lease) => Some(lease), + Err(_) => return Ok(()), + } + } else { + None + }; + match fs::read_dir(trash) { + Ok(entries) if fs::symlink_metadata(trash)?.is_dir() => { + if mount_local && crate::cross_volume::is_mount_point(trash)? { + return Err(io::Error::other( + "foreign retention trash filesystem retained", + )); + } + entries.collect::>>()? + } + Ok(_) => return Err(io::Error::other("non-directory retention trash retained")), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error), + } + }; + for entry in entries { + let name = entry.file_name(); + let name = name.to_string_lossy(); + if !name.strip_prefix("retired-").is_some_and(|nonce| { + nonce.len() == 32 && nonce.bytes().all(|byte| byte.is_ascii_hexdigit()) + }) { + continue; + } + let path = entry.path(); + if mount_local && crate::cross_volume::is_mount_point(&path)? { + continue; + } + // Check entry types and filesystem boundaries again after a restart; + // an unexpected symlink or mount must keep the trash, not be unlinked. + if let Err(error) = validate_tree(&path, Instant::now() + Duration::from_secs(60), false) { + crate::slog_warn!( + "storage retention trash={} retained: {error}", + path.display() + ); + continue; + } + #[cfg(test)] + test_hook(storage, "reap-root"); + if let Err(error) = fs::remove_dir_all(&path) { + crate::slog_warn!( + "storage retention trash={} unlink deferred: {error}", + path.display() + ); + } + } + if mount_local { + // Never recursively delete the container: a concurrent retirement may + // have populated it since the snapshot. An empty-directory unlink is + // safe outside admission and leaves any new tombstones for a later pass. + let _ = fs::remove_dir(trash); + } + Ok(()) +} + fn reap_retired(storage: &Path) -> io::Result<()> { // Different daemon processes may discover the same tombstones. This lock // is separate from admission: slow unlinks must not block binds or pins. @@ -1178,41 +1257,26 @@ fn reap_retired(storage: &Path) -> io::Result<()> { if flat { continue; } - let trash = domain.join(".retention-trash"); - let entries = match fs::read_dir(&trash) { - Ok(entries) if fs::symlink_metadata(&trash)?.is_dir() => entries, - Ok(_) => return Err(io::Error::other("non-directory retention trash retained")), + reap_trash(storage, &domain.join(".retention-trash"), false)?; + let entries = match fs::read_dir(&domain) { + Ok(entries) => entries, Err(error) if error.kind() == io::ErrorKind::NotFound => continue, Err(error) => return Err(error), }; for entry in entries { let entry = entry?; - let name = entry.file_name(); - let name = name.to_string_lossy(); - if !name.strip_prefix("retired-").is_some_and(|nonce| { - nonce.len() == 32 && nonce.bytes().all(|byte| byte.is_ascii_hexdigit()) - }) { + let cache = entry.path(); + if !key_valid(&entry.file_name().to_string_lossy()) || !entry.file_type()?.is_dir() { continue; } - let path = entry.path(); - // Check entry types and filesystem boundaries again after a restart; - // an unexpected symlink or mount must keep the trash, not be unlinked. - if let Err(error) = - validate_tree(&path, Instant::now() + Duration::from_secs(60), false) - { - crate::slog_warn!( - "storage retention trash={} retained: {error}", - path.display() - ); - continue; - } - #[cfg(test)] - test_hook(storage, "reap-root"); - if let Err(error) = fs::remove_dir_all(&path) { - crate::slog_warn!( - "storage retention trash={} unlink deferred: {error}", - path.display() - ); + let mounted = match crate::cross_volume::is_mount_point(&cache) { + Ok(mounted) => mounted, + // A normal cache can be retired while the reaper lists mounts. + Err(error) if error.kind() == io::ErrorKind::NotFound => continue, + Err(error) => return Err(error), + }; + if mounted { + reap_trash(storage, &cache.join(".retention-trash"), true)?; } } } @@ -1563,8 +1627,8 @@ fn candidate_inner( } else { directory.join(key) }; - if !flat && emptied_mount_point(&cache) { - // Already retired: only the mount point itself is left. + if !flat && retired_mount_point(&cache) { + // Only the mount point and possibly its queued trash are left. return Ok(()); } if !flat @@ -1825,7 +1889,7 @@ fn due_first( continue; } if fs::symlink_metadata(directory.join(&key)).is_ok_and(|metadata| metadata.is_dir()) - && !emptied_mount_point(&directory.join(&key)) + && !retired_mount_point(&directory.join(&key)) { present = true; if directory == &storage.join("views") { @@ -3292,6 +3356,15 @@ mod storage_retention_tests { #[test] fn retention_retires_a_mount_point_cache_by_emptying_it_on_its_own_volume() { let (temp, _, _, path) = due_cache_fixture(); + let (reap_tx, reap_rx) = std::sync::mpsc::channel(); + let _hook = observe_test_hook( + temp.path(), + Arc::new(move |step| { + if step == "reap-root" { + let _ = reap_tx.send(()); + } + }), + ); fs::create_dir_all(path.join("nested")).unwrap(); fs::write(path.join("nested/derived.bin"), b"rebuildable").unwrap(); // The domain folder and the cache folder below it report different @@ -3307,6 +3380,14 @@ mod storage_retention_tests { let report = run_pass(temp.path(), &|| false); assert_eq!(report.removed_roots, 1, "{report:?}"); assert!(report.errors.is_empty(), "{report:?}"); + reap_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + // The hook runs before unlink; wait for the worker to release its lock + // before checking that all mount-local trash has gone. + let reaped = crate::fs_lock::try_acquire( + &temp.path().join("retention/reap.lock"), + Duration::from_secs(10), + ) + .unwrap(); assert!(path.is_dir(), "the mount point itself must stay"); assert_eq!( fs::read_dir(&path).unwrap().count(), @@ -3321,6 +3402,7 @@ mod storage_retention_tests { == 0, "nothing crossed into the domain folder's trash" ); + drop(reaped); // The emptied mount point counts as gone, so the next pass does not // retire it again. @@ -3328,6 +3410,107 @@ mod storage_retention_tests { assert_eq!(report.removed_roots, 0, "{report:?}"); } + #[test] + fn retention_reaper_resumes_interrupted_mount_point_retirement() { + let (temp, _, _, path) = due_cache_fixture(); + let _views = crate::walk_boundary::simulated_volumes::simulate( + &temp.path().join("views"), + u64::MAX - 10, + ); + let _mount = crate::walk_boundary::simulated_volumes::simulate(&path, u64::MAX - 11); + let trash = path.join(".retention-trash"); + let retired = trash.join(format!("retired-{:032x}", 1)); + fs::create_dir_all(&retired).unwrap(); + // Model a daemon exit after the last rename, before any unlink. + fs::rename(path.join("payload.bin"), retired.join("payload.bin")).unwrap(); + assert!(!emptied_mount_point(&path), "retired data is still on disk"); + + reap_retired(temp.path()).unwrap(); + assert!( + !trash.exists(), + "a later reaper pass must reclaim local trash" + ); + assert!(path.is_dir(), "the mount point itself must stay"); + assert!(emptied_mount_point(&path)); + + let unknown = trash.join("unrecognized"); + let foreign = trash.join(format!("retired-{:032x}", 2)); + fs::create_dir_all(&unknown).unwrap(); + fs::create_dir_all(&foreign).unwrap(); + fs::write(unknown.join("important"), b"not retired").unwrap(); + fs::write(foreign.join("important"), b"another volume").unwrap(); + let _foreign = crate::walk_boundary::simulated_volumes::simulate(&foreign, u64::MAX - 12); + reap_retired(temp.path()).unwrap(); + assert!(unknown.join("important").exists()); + assert!(foreign.join("important").exists()); + assert!(!emptied_mount_point(&path)); + } + + #[test] + fn retention_mount_point_unlink_runs_outside_sweep_lock() { + let (temp, _, scope, path) = due_cache_fixture(); + let _views = crate::walk_boundary::simulated_volumes::simulate( + &temp.path().join("views"), + u64::MAX - 10, + ); + let _mount = crate::walk_boundary::simulated_volumes::simulate(&path, u64::MAX - 11); + let sweep_lock = temp.path().join("retention/sweep.lock"); + let rename_lock = sweep_lock.clone(); + let renamed = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let renamed_hook = renamed.clone(); + let _rename_hook = observe_test_hook( + &path, + Arc::new(move |step| { + if step == "rename-root" { + assert!(crate::fs_lock::try_acquire(&rename_lock, Duration::ZERO).is_err()); + renamed_hook.store(true, std::sync::atomic::Ordering::SeqCst); + } + }), + ); + let reaped = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let reaped_hook = reaped.clone(); + let _reap_hook = observe_test_hook( + temp.path(), + Arc::new(move |step| { + if step == "reap-root" { + let _admission = + crate::fs_lock::try_acquire(&sweep_lock, Duration::ZERO).unwrap(); + reaped_hook.store(true, std::sync::atomic::Ordering::SeqCst); + } + }), + ); + let mut report = SweepReport::default(); + retire_due_cache( + temp.path(), + &temp.path().join("views"), + &scope, + &bindings(temp.path()).unwrap(), + crate::pins::now_ms(), + Instant::now() + PASS_BUDGET, + &|| false, + &mut report, + ) + .unwrap(); + assert_eq!(report.removed_roots, 1, "{report:?}"); + assert!(renamed.load(std::sync::atomic::Ordering::SeqCst)); + let trash = path.join(".retention-trash"); + assert!( + trash.is_dir(), + "mount-local trash was deleted during locked retirement" + ); + let retired = fs::read_dir(&trash) + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + assert!(retired.join("payload.bin").exists()); + + reap_retired(temp.path()).unwrap(); + assert!(reaped.load(std::sync::atomic::Ordering::SeqCst)); + assert!(!trash.exists()); + } + #[test] fn retention_reaper_resumes_retired_roots_without_a_pass() { let (temp, _, _, path) = due_cache_fixture(); From 0cd436198196470c3e9a811a2bfafad7176a94c8 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:51:42 +0200 Subject: [PATCH 5/7] mason: gate mount-trash scans with a durable pending marker (#504) Publish a fresh retention/mount-trash-pending generation under admission before moving mounted cache entries. Reapers skip cache listings and mount probes without the hint; retain it for unreaped tombstones or busy snapshots, and clear it only after a clean scan and an admission-protected generation comparison. Add zero-probe and concurrent-retirement regressions. The interrupted-retirement test now uses the actual retirement path, checking marker publication before moves while retaining its reclamation and boundary assertions. Removing the marker gate or neutralizing the generation comparison independently reddens the corresponding new test. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/storage_retention.rs | 218 +++++++++++++++++++++++++--- 1 file changed, 199 insertions(+), 19 deletions(-) diff --git a/crates/aft/src/storage_retention.rs b/crates/aft/src/storage_retention.rs index 3161db7c9..c96d4e57b 100644 --- a/crates/aft/src/storage_retention.rs +++ b/crates/aft/src/storage_retention.rs @@ -1106,12 +1106,12 @@ fn validate_tree( /// the domain folder above it) cannot be renamed into the domain's trash: the /// rename would cross volumes, and a mount point cannot be moved anyway. Its /// contents are retired instead, see `retire_mount_point_contents`. -fn retire_directory(cache: &Path) -> io::Result<()> { +fn retire_directory(storage: &Path, cache: &Path) -> io::Result<()> { let parent = cache .parent() .ok_or_else(|| io::Error::other("missing cache parent"))?; if crate::cross_volume::is_mount_point(cache)? { - return retire_mount_point_contents(cache); + return retire_mount_point_contents(storage, cache); } let trash = parent.join(".retention-trash"); fs::create_dir_all(&trash)?; @@ -1134,13 +1134,19 @@ fn retire_directory(cache: &Path) -> io::Result<()> { /// while the caller holds admission and SQLite's filesystem guard; the reaper /// discovers mount-local trash independently and does the slow unlink later. /// The mount point itself stays even after its retired contents are reclaimed. -fn retire_mount_point_contents(cache: &Path) -> io::Result<()> { +fn retire_mount_point_contents(storage: &Path, cache: &Path) -> io::Result<()> { let trash = cache.join(".retention-trash"); - #[cfg(test)] - test_hook(cache, "rename-root"); let mut nonce = [0_u8; 16]; getrandom::fill(&mut nonce).map_err(io::Error::other)?; - let retired = trash.join(format!("retired-{:032x}", u128::from_le_bytes(nonce))); + let nonce = format!("{:032x}", u128::from_le_bytes(nonce)); + // Cache-level mounts are rare. Publish a durable hint before moving data + // so ordinary reaper passes need not probe every cache's filesystem. Each + // retirement replaces the generation under admission, letting the reaper + // detect a newer retirement before it clears the hint. + atomic_json(&storage.join("retention/mount-trash-pending"), &nonce)?; + #[cfg(test)] + test_hook(cache, "rename-root"); + let retired = trash.join(format!("retired-{nonce}")); loop { fs::create_dir_all(&trash)?; if !fs::symlink_metadata(&trash)?.is_dir() { @@ -1181,7 +1187,9 @@ fn retired_mount_point(cache: &Path) -> bool { }) } -fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> { +/// Returns whether any recognizable tombstones could not be reclaimed. A busy +/// mount-local snapshot also keeps the pending hint for a later reaper pass. +fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result { let entries = { // Snapshot mount-local tombstones under admission so a concurrent // retirement finishes moving entries before any of them are unlinked. @@ -1190,7 +1198,7 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> match crate::fs_lock::try_acquire(&storage.join("retention/sweep.lock"), Duration::ZERO) { Ok(lease) => Some(lease), - Err(_) => return Ok(()), + Err(_) => return Ok(true), } } else { None @@ -1205,10 +1213,11 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> entries.collect::>>()? } Ok(_) => return Err(io::Error::other("non-directory retention trash retained")), - Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(false), Err(error) => return Err(error), } }; + let mut pending = false; for entry in entries { let name = entry.file_name(); let name = name.to_string_lossy(); @@ -1219,11 +1228,13 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> } let path = entry.path(); if mount_local && crate::cross_volume::is_mount_point(&path)? { + pending = true; continue; } // Check entry types and filesystem boundaries again after a restart; // an unexpected symlink or mount must keep the trash, not be unlinked. if let Err(error) = validate_tree(&path, Instant::now() + Duration::from_secs(60), false) { + pending = true; crate::slog_warn!( "storage retention trash={} retained: {error}", path.display() @@ -1233,6 +1244,7 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> #[cfg(test)] test_hook(storage, "reap-root"); if let Err(error) = fs::remove_dir_all(&path) { + pending = true; crate::slog_warn!( "storage retention trash={} unlink deferred: {error}", path.display() @@ -1245,7 +1257,7 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result<()> // safe outside admission and leaves any new tombstones for a later pass. let _ = fs::remove_dir(trash); } - Ok(()) + Ok(pending) } fn reap_retired(storage: &Path) -> io::Result<()> { @@ -1253,11 +1265,21 @@ fn reap_retired(storage: &Path) -> io::Result<()> { // is separate from admission: slow unlinks must not block binds or pins. let _lease = crate::fs_lock::try_acquire(&storage.join("retention/reap.lock"), Duration::ZERO) .map_err(|error| io::Error::other(error.to_string()))?; + let marker = storage.join("retention/mount-trash-pending"); + let mount_scan = match fs::read(&marker) { + Ok(generation) => Some(generation), + Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Err(error) => return Err(error), + }; + let mut mount_trash_pending = false; for (domain, flat) in domain_paths(storage)? { if flat { continue; } reap_trash(storage, &domain.join(".retention-trash"), false)?; + if mount_scan.is_none() { + continue; + } let entries = match fs::read_dir(&domain) { Ok(entries) => entries, Err(error) if error.kind() == io::ErrorKind::NotFound => continue, @@ -1269,6 +1291,8 @@ fn reap_retired(storage: &Path) -> io::Result<()> { if !key_valid(&entry.file_name().to_string_lossy()) || !entry.file_type()?.is_dir() { continue; } + #[cfg(test)] + test_hook(storage, "mount-trash-probe"); let mounted = match crate::cross_volume::is_mount_point(&cache) { Ok(mounted) => mounted, // A normal cache can be retired while the reaper lists mounts. @@ -1276,7 +1300,30 @@ fn reap_retired(storage: &Path) -> io::Result<()> { Err(error) => return Err(error), }; if mounted { - reap_trash(storage, &cache.join(".retention-trash"), true)?; + mount_trash_pending |= reap_trash(storage, &cache.join(".retention-trash"), true)?; + } + } + } + if !mount_trash_pending { + if let Some(scanned) = mount_scan { + #[cfg(test)] + test_hook(storage, "mount-trash-scanned"); + let Ok(_admission) = + crate::fs_lock::try_acquire(&storage.join("retention/sweep.lock"), Duration::ZERO) + else { + return Ok(()); + }; + // Retirements publish under the same sweep.lock held by _admission. + // Keep a newer generation: it may describe trash created after this + // reaper scanned that domain. + match fs::read(&marker) { + Ok(current) if current == scanned => { + fs::remove_file(&marker)?; + crate::durability::sync_dir(&storage.join("retention"))?; + } + Ok(_) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), } } } @@ -1447,7 +1494,7 @@ fn retire_due_cache( return Ok(()); } let phase = Instant::now(); - retire_directory(&cache)?; + retire_directory(storage, &cache)?; *report.timings_ms.entry("due_rename".into()).or_default() += phase.elapsed().as_millis() as u64; report.removed_roots += 1; @@ -1826,7 +1873,7 @@ fn candidate_inner( } } } else { - retire_directory(&cache)?; + retire_directory(storage, &cache)?; report.removed_bytes += after_views .iter() .map(|(_, bytes)| bytes) @@ -3410,6 +3457,45 @@ mod storage_retention_tests { assert_eq!(report.removed_roots, 0, "{report:?}"); } + #[test] + fn retention_reaper_skips_mount_probes_without_pending_marker() { + let (temp, _, scope, _) = due_cache_fixture(); + cache(temp.path(), "callgraph", &scope); + let ordinary = temp + .path() + .join(format!("views/.retention-trash/retired-{:032x}", 1)); + fs::create_dir_all(&ordinary).unwrap(); + fs::write(ordinary.join("payload.bin"), b"retired").unwrap(); + let probes = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let observed = probes.clone(); + let _hook = observe_test_hook( + temp.path(), + Arc::new(move |step| { + if step == "mount-trash-probe" { + observed.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + } + }), + ); + reap_retired(temp.path()).unwrap(); + assert_eq!( + probes.load(std::sync::atomic::Ordering::SeqCst), + 0, + "no marker means no cache-folder mount probes" + ); + assert!(!ordinary.exists(), "domain-local trash still gets reaped"); + + let marker = temp.path().join("retention/mount-trash-pending"); + { + let _admission = barrier(temp.path()).unwrap(); + atomic_json(&marker, &"scan").unwrap(); + } + reap_retired(temp.path()).unwrap(); + assert_eq!(probes.load(std::sync::atomic::Ordering::SeqCst), 2); + assert!(!marker.exists(), "a clean scan clears the marker"); + reap_retired(temp.path()).unwrap(); + assert_eq!(probes.load(std::sync::atomic::Ordering::SeqCst), 2); + } + #[test] fn retention_reaper_resumes_interrupted_mount_point_retirement() { let (temp, _, _, path) = due_cache_fixture(); @@ -3419,10 +3505,34 @@ mod storage_retention_tests { ); let _mount = crate::walk_boundary::simulated_volumes::simulate(&path, u64::MAX - 11); let trash = path.join(".retention-trash"); - let retired = trash.join(format!("retired-{:032x}", 1)); - fs::create_dir_all(&retired).unwrap(); - // Model a daemon exit after the last rename, before any unlink. - fs::rename(path.join("payload.bin"), retired.join("payload.bin")).unwrap(); + let marker = temp.path().join("retention/mount-trash-pending"); + let before_moves = path.clone(); + let pending = marker.clone(); + let _hook = observe_test_hook( + &path, + Arc::new(move |step| { + if step == "rename-root" { + assert!( + pending.is_file(), + "publish the marker before moving entries" + ); + assert!(before_moves.join("payload.bin").exists()); + } + }), + ); + { + let _admission = barrier(temp.path()).unwrap(); + // Model a daemon exit after all moves but before the reaper runs. + retire_directory(temp.path(), &path).unwrap(); + } + let retired = fs::read_dir(&trash) + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + assert!(retired.join("payload.bin").exists()); + assert!(marker.exists()); assert!(!emptied_mount_point(&path), "retired data is still on disk"); reap_retired(temp.path()).unwrap(); @@ -3430,6 +3540,7 @@ mod storage_retention_tests { !trash.exists(), "a later reaper pass must reclaim local trash" ); + assert!(!marker.exists(), "a clean reap clears the marker"); assert!(path.is_dir(), "the mount point itself must stay"); assert!(emptied_mount_point(&path)); @@ -3440,10 +3551,79 @@ mod storage_retention_tests { fs::write(unknown.join("important"), b"not retired").unwrap(); fs::write(foreign.join("important"), b"another volume").unwrap(); let _foreign = crate::walk_boundary::simulated_volumes::simulate(&foreign, u64::MAX - 12); + { + let _admission = barrier(temp.path()).unwrap(); + atomic_json(&marker, &"protected").unwrap(); + } reap_retired(temp.path()).unwrap(); assert!(unknown.join("important").exists()); assert!(foreign.join("important").exists()); assert!(!emptied_mount_point(&path)); + assert!(marker.exists(), "unreaped tombstones keep the marker"); + } + + #[test] + fn retention_mount_trash_marker_survives_concurrent_retirement() { + let (temp, _, _, path) = due_cache_fixture(); + let _views = crate::walk_boundary::simulated_volumes::simulate( + &temp.path().join("views"), + u64::MAX - 10, + ); + let _mount = crate::walk_boundary::simulated_volumes::simulate(&path, u64::MAX - 11); + { + let _admission = barrier(temp.path()).unwrap(); + retire_directory(temp.path(), &path).unwrap(); + } + let marker = temp.path().join("retention/mount-trash-pending"); + let previous = fs::read(&marker).unwrap(); + let (scanned_tx, scanned_rx) = std::sync::mpsc::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let release_rx = Mutex::new(release_rx); + let hook = observe_test_hook( + temp.path(), + Arc::new(move |step| { + if step == "mount-trash-scanned" { + scanned_tx.send(()).unwrap(); + release_rx + .lock() + .unwrap() + .recv_timeout(Duration::from_secs(20)) + .unwrap(); + } + }), + ); + let storage = temp.path().to_path_buf(); + let worker = std::thread::spawn(move || reap_retired(&storage)); + scanned_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + { + let _admission = barrier(temp.path()).unwrap(); + fs::write(path.join("new-payload.bin"), b"retired after the scan").unwrap(); + retire_directory(temp.path(), &path).unwrap(); + } + let current = fs::read(&marker).unwrap(); + release_tx.send(()).unwrap(); + worker.join().unwrap().unwrap(); + drop(hook); + assert_ne!( + previous, current, + "each retirement publishes a new generation" + ); + assert_eq!( + fs::read(&marker).expect("a concurrent retirement's marker must survive"), + current, + "a concurrent retirement's marker must survive" + ); + let trash = path.join(".retention-trash"); + let retired = fs::read_dir(&trash) + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + assert!(retired.join("new-payload.bin").exists()); + reap_retired(temp.path()).unwrap(); + assert!(!trash.exists()); + assert!(!marker.exists()); } #[test] @@ -3514,7 +3694,7 @@ mod storage_retention_tests { #[test] fn retention_reaper_resumes_retired_roots_without_a_pass() { let (temp, _, _, path) = due_cache_fixture(); - retire_directory(&path).unwrap(); + retire_directory(temp.path(), &path).unwrap(); let trash = temp.path().join("views/.retention-trash"); assert_eq!(fs::read_dir(&trash).unwrap().count(), 1); assert!(!path.exists()); @@ -3571,7 +3751,7 @@ mod storage_retention_tests { let report = run_pass(temp.path(), &|| false); assert_eq!(report.removed_roots, 0, "{report:?}"); assert!(path.exists()); - retire_directory(&path).unwrap(); + retire_directory(temp.path(), &path).unwrap(); reap_retired(temp.path()).unwrap(); assert_eq!( fs::read_dir(temp.path().join("views/.retention-trash")) From 5ad9eaf7f76ee7286e981773e7924353a8ae9dc0 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 03:40:59 +0200 Subject: [PATCH 6/7] mason: size migration preflight from children still to copy (#504) A retry after a partial migration used to charge each destination volume with the full source subtree, including children an earlier run already published, so a volume that needed no more data could be refused with InsufficientDisk. The preflight now sizes each item with the same already_published rule migrate_whole and migrate_child_union use to skip destinations, and charges the target root only for its own pending bytes. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/migrate_storage.rs | 200 +++++++++++++++++++++++++++--- 1 file changed, 186 insertions(+), 14 deletions(-) diff --git a/crates/aft/src/migrate_storage.rs b/crates/aft/src/migrate_storage.rs index 703507a2d..87bd20a7c 100644 --- a/crates/aft/src/migrate_storage.rs +++ b/crates/aft/src/migrate_storage.rs @@ -565,7 +565,7 @@ fn migrate_whole( log: &mut JsonLogger, ) -> io::Result<()> { let final_path = target_path(args, item); - if final_path.exists() { + if already_published(&final_path) { log.write(serde_json::json!({ "level": "warn", "step": "copy", @@ -614,7 +614,7 @@ fn migrate_child_union( let name = entry.file_name(); let child_source = entry.path(); let child_final = final_dir.join(&name); - if child_final.exists() { + if already_published(&child_final) { log.write(serde_json::json!({ "level": "warn", "step": "copy", @@ -770,7 +770,67 @@ fn source_size(path: &Path) -> io::Result { Ok(total) } -/// One destination volume of a migration and the source bytes headed for it. +/// Whether the copy step skips this destination because an earlier run already +/// published it there. `migrate_whole`, `migrate_child_union` and the +/// free-space preflight all use this one rule, so a retried migration is +/// charged only for the data it will actually copy. +fn already_published(final_path: &Path) -> bool { + final_path.exists() +} + +/// Source bytes of one migration item: all of them, and the part the copy +/// step will still copy (what `already_published` does not skip). +struct ItemBytes { + total: u64, + pending: u64, +} + +fn item_bytes(args: &MigrationArgs, item: MigrationItem) -> io::Result { + let source = args.from.join(item.source_name); + if !source.exists() { + return Ok(ItemBytes { + total: 0, + pending: 0, + }); + } + let final_path = target_path(args, item); + match item.merge { + MergeKind::ChildUnion if source.is_dir() => { + let mut bytes = ItemBytes { + total: 0, + pending: 0, + }; + for entry in fs::read_dir(&source)? { + let entry = entry?; + let child = source_size(&entry.path())?; + bytes.total = bytes.total.saturating_add(child); + if !already_published(&final_path.join(entry.file_name())) { + bytes.pending = bytes.pending.saturating_add(child); + } + } + Ok(bytes) + } + MergeKind::Whole if already_published(&final_path) => Ok(ItemBytes { + total: source_size(&source)?, + pending: 0, + }), + // The remaining cases count the whole source: a `Whole` item whose + // destination does not exist yet, the `TrustJson` list (merged into + // the target file on every run), and a `ChildUnion` whose source is a + // plain file rather than a folder (the copy step reports that as an + // error; the preflight only sizes it). + MergeKind::Whole | MergeKind::ChildUnion | MergeKind::TrustJson => { + let total = source_size(&source)?; + Ok(ItemBytes { + total, + pending: total, + }) + } + } +} + +/// One destination volume of a migration and the source bytes this run will +/// copy onto it. struct DestinationVolume { /// A folder on that volume: the target root, or the first destination /// found on another volume. @@ -779,11 +839,14 @@ struct DestinationVolume { free_bytes: u64, } -/// Group the bytes to migrate by the volume each destination is on, and read -/// each volume's free space. The target root's volume is charged with every -/// byte not headed to another volume; when all destinations share it, that is -/// the whole source tree, as the check has always counted. An error names the -/// path whose volume could not be read. +/// Group the bytes still to copy by the volume each destination is on, and +/// read each volume's free space. Entries an earlier run already published +/// (see `already_published`) are not counted, so a retry after a partial run +/// needs room only for what is left. The target root's volume is charged with +/// every source byte that is neither headed to another volume nor already +/// published, including files outside the migrated items, as the check has +/// always counted. An error names the path whose volume or size could not be +/// read. fn destination_volumes( args: &MigrationArgs, source_bytes: u64, @@ -800,15 +863,22 @@ fn destination_volumes( .map_err(|error| (path.to_path_buf(), error)) }; let root_device = device(&args.to)?; + // Source bytes the root's volume does not need room for: everything headed + // to another volume, and whatever the copy step will skip on the root's. + let mut not_for_root = 0_u64; let mut elsewhere: Vec<(Option, PathBuf, u64)> = Vec::new(); for &item in migration_items() { let destination = target_path(args, item); let destination_device = device(&destination)?; + let item_bytes = + item_bytes(args, item).map_err(|error| (args.from.join(item.source_name), error))?; if destination_device == root_device { + not_for_root = + not_for_root.saturating_add(item_bytes.total.saturating_sub(item_bytes.pending)); continue; } - let source = args.from.join(item.source_name); - let bytes = source_size(&source).map_err(|error| (source, error))?; + not_for_root = not_for_root.saturating_add(item_bytes.total); + let bytes = item_bytes.pending; match elsewhere .iter_mut() .find(|(device, _, _)| *device == destination_device) @@ -821,12 +891,9 @@ fn destination_volumes( )), } } - let moved_elsewhere = elsewhere - .iter() - .fold(0_u64, |total, (_, _, bytes)| total.saturating_add(*bytes)); let mut volumes = vec![DestinationVolume { path: args.to.clone(), - source_bytes: source_bytes.saturating_sub(moved_elsewhere), + source_bytes: source_bytes.saturating_sub(not_for_root), free_bytes: free_at(&args.to)?, }]; for (_, path, bytes) in elsewhere { @@ -1103,6 +1170,111 @@ mod tests { assert_eq!(root_line["ok"], true); } + /// A legacy tree whose `index` has two cache folders, `a` (1000 bytes) and + /// `b` (100 bytes), migrating to a target whose `index` folder is its own + /// volume. An earlier run already published `index/a` there and stopped + /// before writing either completion marker. Returns the temporary folder, + /// the arguments for the retry, and the destination `index` folder. + fn partially_migrated_index() -> (tempfile::TempDir, Args, PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let from = dir.path().join("legacy"); + for (child, size) in [("a", 1000), ("b", 100)] { + fs::create_dir_all(from.join("index").join(child)).unwrap(); + fs::write( + from.join("index").join(child).join("cache.bin"), + vec![0_u8; size], + ) + .unwrap(); + } + fs::create_dir_all(from.join("backups")).unwrap(); + fs::write(from.join("backups/meta.json"), b"{}").unwrap(); + let to = dir.path().join("target"); + let index = to.join("index"); + fs::create_dir_all(index.join("a")).unwrap(); + fs::write(index.join("a/cache.bin"), vec![0_u8; 1000]).unwrap(); + let args = Args { + from: Some(from), + to, + harness: Harness::Opencode, + log: Some(dir.path().join("migration.jsonl")), + status: false, + }; + (dir, args, index) + } + + fn preflight_records(log: &Path) -> Vec { + fs::read_to_string(log) + .unwrap() + .lines() + .map(|line| serde_json::from_str::(line).unwrap()) + .filter(|record| record["step"] == "preflight") + .collect() + } + + #[test] + fn preflight_retry_counts_only_children_still_to_copy() { + let (_dir, args, index) = partially_migrated_index(); + let to = args.to.clone(); + let log = args.log.clone().unwrap(); + let _volume = crate::walk_boundary::simulated_volumes::simulate(&index, u64::MAX - 31); + // Twice the whole index (2200) does not fit, but twice the 100 bytes + // of `b` that are still to copy does. + FREE_BYTES_FOR_TEST + .with(|slot| *slot.borrow_mut() = vec![(index.clone(), 1500), (to.clone(), u64::MAX)]); + + let status = run_with_options(args, Options::default()); + FREE_BYTES_FOR_TEST.with(|slot| slot.borrow_mut().clear()); + + let preflight = preflight_records(&log); + assert_eq!(status, ExitStatus::Success, "preflight: {preflight:?}"); + let index_line = preflight + .iter() + .find(|record| record["path"] == serde_json::json!(index)) + .expect("the index volume is checked on its own"); + assert_eq!(index_line["bytes_source"], 100); + assert_eq!(index_line["bytes_required"], 200); + assert_eq!(index_line["ok"], true); + let root_line = preflight + .iter() + .find(|record| record["path"] == serde_json::json!(to)) + .expect("the target root is checked"); + assert_eq!( + root_line["bytes_source"], 2, + "index bytes, copied or not, are not charged to the root" + ); + assert_eq!(fs::read(index.join("b/cache.bin")).unwrap().len(), 100); + assert_eq!(fs::read(index.join("a/cache.bin")).unwrap().len(), 1000); + assert!(target_marker_path_from(&to, &Harness::Opencode).exists()); + } + + #[test] + fn preflight_retry_refuses_when_remaining_children_do_not_fit() { + let (_dir, args, index) = partially_migrated_index(); + let to = args.to.clone(); + let log = args.log.clone().unwrap(); + let _volume = crate::walk_boundary::simulated_volumes::simulate(&index, u64::MAX - 32); + // Twice the 100 bytes still to copy is more than the volume's 150. + FREE_BYTES_FOR_TEST + .with(|slot| *slot.borrow_mut() = vec![(index.clone(), 150), (to.clone(), u64::MAX)]); + + let status = run_with_options(args, Options::default()); + FREE_BYTES_FOR_TEST.with(|slot| slot.borrow_mut().clear()); + + assert_eq!(status, ExitStatus::InsufficientDisk); + let preflight = preflight_records(&log); + let index_line = preflight + .iter() + .find(|record| record["path"] == serde_json::json!(index)) + .expect("the refusal names the index volume"); + assert_eq!(index_line["bytes_source"], 100); + assert_eq!(index_line["bytes_free"], 150); + assert_eq!(index_line["ok"], false); + assert!( + !index.join("b").exists(), + "a refused preflight copies nothing" + ); + } + #[test] fn migration_marker_does_not_truncate_a_colliding_temporary() { let dir = tempfile::tempdir().unwrap(); From 63f40596762a0d13264dfe0b938cdcb803622eac Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:39:00 +0200 Subject: [PATCH 7/7] mason: preserve live legacy writers during cross-volume repair (#504) Adopt live legacy bash sessions in place and defer migration until writers are quiescent and no registry pins their paths. Coordinate backup session migration with existing admission gates and inactive stack leases. Sync mount-local trash deletions before clearing the durable pending marker, including parent-sync retries after container removal. Replace the malformed task migration fixture with a real terminal record: unknown metadata cannot prove quiescence. Backup migration assertions now require the session, not the whole parent, to disappear because stable admission-gate inodes must stay. New live-writer and injected sync-failure regressions fail before the fixes and under four named mutation controls; restored gates pass. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/backup.rs | 143 +++++++++------- crates/aft/src/bash_background/mod.rs | 159 +++++++++++++++++- crates/aft/src/bash_background/persistence.rs | 13 ++ crates/aft/src/bash_background/registry.rs | 15 ++ crates/aft/src/commands/configure.rs | 3 +- crates/aft/src/durability.rs | 15 ++ crates/aft/src/storage_retention.rs | 70 +++++++- .../integration/backups_dual_write_test.rs | 4 +- 8 files changed, 353 insertions(+), 69 deletions(-) diff --git a/crates/aft/src/backup.rs b/crates/aft/src/backup.rs index 9ec4969af..9843c40df 100644 --- a/crates/aft/src/backup.rs +++ b/crates/aft/src/backup.rs @@ -2909,8 +2909,18 @@ impl BackupStore { } fn session_dir(&self, session: &str) -> Option { - self.backups_dir() - .map(|d| d.join(Self::session_hash(session))) + let direct = self.backups_dir()?.join(Self::session_hash(session)); + if !direct.exists() && self.effective_storage_harness().is_some() { + let legacy = self + .storage_dir + .as_ref()? + .join("backups") + .join(Self::session_hash(session)); + if legacy.exists() { + return Some(legacy); + } + } + Some(direct) } fn session_hash(session: &str) -> String { @@ -2964,68 +2974,39 @@ impl BackupStore { return; } let harness_backups = storage_dir.join(harness).join("backups"); - if dir_has_entries(&harness_backups) { + if let Err(error) = create_private_durable_dir(&harness_backups) { + crate::slog_warn!( + "cannot create harness backup dir {}: {error}", + harness_backups.display() + ); return; } - if let Some(parent) = harness_backups.parent() { - if let Err(error) = create_private_durable_dir(parent) { - crate::slog_warn!( - "failed to create harness backup dir {}: {}", - parent.display(), - error - ); - return; - } - } - if harness_backups.exists() { - let _ = std::fs::remove_dir(&harness_backups); - } - // The harness folder and the root normally share a volume, but a move - // that crosses volumes falls back to a copy instead of failing. - match crate::cross_volume::move_path(&root_backups, &harness_backups) { - Ok(_) => { - crate::slog_info!( - "moved legacy root backups into harness namespace: {}", - harness_backups.display() - ); + let Ok(entries) = std::fs::read_dir(&root_backups) else { + return; + }; + for entry in entries.flatten() { + let source = entry.path(); + let target = harness_backups.join(entry.file_name()); + if !entry.file_type().is_ok_and(|kind| kind.is_dir()) || target.exists() { + continue; } - Err(error) => { + // Use the same admission gate and stack leases as stale-session GC: + // a paused writer must finish before copy+unlink can move its files. + let Ok(Some(_gate)) = session_gc_gate(&source, false) else { + continue; + }; + let Ok(Some(leases)) = acquire_inactive_session_leases(&source) else { + continue; + }; + // The gate prevents new leases. Close probe handles before moving + // the session, and keep stable gate files in their original parent. + drop(leases); + if let Err(error) = crate::cross_volume::move_path(&source, &target) { crate::slog_warn!( - "failed to move legacy root backups into {}: {}; trying child merge", - harness_backups.display(), - error + "legacy backup {} kept in place: cannot move it to {}: {error}", + source.display(), + target.display() ); - if let Err(error) = create_private_durable_dir(&harness_backups) { - crate::slog_warn!( - "legacy root backups kept in {}: cannot create {}: {}", - root_backups.display(), - harness_backups.display(), - error - ); - return; - } - if let Ok(entries) = std::fs::read_dir(&root_backups) { - for entry in entries.flatten() { - let source = entry.path(); - let target = harness_backups.join(entry.file_name()); - if target.exists() { - continue; - } - if let Err(error) = crate::cross_volume::move_path(&source, &target) { - // A backup that cannot be moved stays in the root - // `backups/` folder; log the move failure instead - // of dropping it. - crate::slog_warn!( - "legacy backup {} kept in place: cannot move it to {}: {}", - source.display(), - target.display(), - error - ); - } - } - } - // Removes the legacy folder only when every backup left it. - let _ = std::fs::remove_dir(&root_backups); } } } @@ -3503,6 +3484,12 @@ impl BackupStore { path: gate_path.display().to_string(), message: error.to_string(), })?; + if self.session_dir(session).as_ref() != Some(&session_dir) { + // Repair may have migrated an inactive session while we waited for + // its gate. Resolve again instead of recreating its legacy folder. + drop(_gate); + return self.acquire_stack_disk_lock(session, key); + } create_private_durable_dir(&session_dir).map_err(|error| AftError::IoError { path: session_dir.display().to_string(), message: error.to_string(), @@ -5626,6 +5613,40 @@ fn backup_sequence(backup_id: &str) -> Option { #[cfg(test)] mod tests { + #[test] + fn cross_volume_legacy_backup_repair_keeps_active_stack_writer() { + use std::io::Write; + let storage = tempfile::tempdir().unwrap(); + let mut writer = BackupStore::new(); + writer.set_storage_dir(storage.path().to_path_buf(), 72); + let session = "legacy-writer"; + let key = storage.path().join("source.txt"); + let lock = writer.acquire_stack_disk_lock(session, &key).unwrap(); + let legacy = writer.session_dir(session).unwrap(); + let mut output = std::fs::File::create(legacy.join("payload")).unwrap(); + output.write_all(b"before").unwrap(); + let mut repair = BackupStore::new(); + repair.set_storage_dir_for_harness( + storage.path().to_path_buf(), + crate::harness::Harness::Opencode, + 72, + ); + let harness = storage.path().join("opencode"); + let _volume = crate::walk_boundary::simulated_volumes::simulate(&harness, u64::MAX - 20); + repair.repair_root_backups_if_needed(); + output.write_all(b"after").unwrap(); + drop(output); + assert_eq!(fs::read(legacy.join("payload")).unwrap(), b"beforeafter"); + assert_eq!(repair.session_dir(session), Some(legacy.clone())); + drop(lock); + repair.repair_root_backups_if_needed(); + assert!(!legacy.exists()); + assert_eq!( + fs::read(repair.session_dir(session).unwrap().join("payload")).unwrap(), + b"beforeafter" + ); + } + #[test] fn stale_session_gc_preserves_active_stack_lease() { let storage = tempfile::tempdir().unwrap(); @@ -7107,8 +7128,8 @@ mod tests { "the backup history must reach the harness folder" ); assert!( - !root_backups.exists(), - "the legacy folder is removed after the copy" + !root_backups.join("session-a").exists(), + "the session is removed after the copy; stable admission gates stay" ); } diff --git a/crates/aft/src/bash_background/mod.rs b/crates/aft/src/bash_background/mod.rs index aadc059df..e0e8f2a1b 100644 --- a/crates/aft/src/bash_background/mod.rs +++ b/crates/aft/src/bash_background/mod.rs @@ -1329,6 +1329,12 @@ pub fn repair_legacy_root_tasks(storage_root: &std::path::Path, harness: crate:: if !dir_has_entries(&root_tasks) { return; } + // Detached writers retain both open output handles and absolute marker paths. + // Even a same-volume rename can strand their later path-based writes. Keep + // the entire legacy tree readable in place until every task is quiescent. + if !legacy_root_tasks_quiescent(&root_tasks).unwrap_or(false) { + return; + } let harness_tasks = storage_root .join(harness.storage_segment()) @@ -1398,6 +1404,45 @@ pub fn repair_legacy_root_tasks(storage_root: &std::path::Path, harness: crate:: } } +fn legacy_root_tasks_quiescent(root: &std::path::Path) -> std::io::Result { + use persistence::{discover_task_ids, read_exit_marker, read_task_at, resolve_task_layout}; + for session in std::fs::read_dir(root)? { + let session = session?; + if !session.file_type()?.is_dir() { + return Ok(false); + } + let (ids, invalid) = discover_task_ids(&session.path())?; + if !invalid.is_empty() { + return Ok(false); + } + for id in ids { + let task = resolve_task_layout(&session.path(), &id)?; + let metadata = read_task_at(&task)?; + if read_exit_marker(&task.paths)?.is_some() { + continue; + } + if !metadata.is_terminal() { + return Ok(false); + } + if metadata.child_pid.is_none() && metadata.pgid.is_none() { + continue; + } + #[cfg(unix)] + if let Some(pgid) = metadata.pgid.filter(|pgid| *pgid > 0) { + // Only ESRCH proves that no group member can still write. A + // permission error or an unsupported probe keeps the task put. + if unsafe { libc::kill(-pgid, 0) } == -1 + && std::io::Error::last_os_error().raw_os_error() == Some(libc::ESRCH) + { + continue; + } + } + return Ok(false); + } + } + Ok(true) +} + fn dir_has_entries(path: &std::path::Path) -> bool { std::fs::read_dir(path) .map(|mut entries| entries.next().is_some()) @@ -1652,12 +1697,116 @@ mod legacy_root_task_repair_tests { use crate::harness::Harness; use crate::walk_boundary::simulated_volumes::simulate; + #[cfg(unix)] + #[test] + fn bash_background_legacy_live_writer_survives_cross_volume_repair_and_replay() { + use super::persistence::*; + use super::registry::BgTaskRegistry; + use super::BgTaskStatus; + use std::process::{Command, Stdio}; + + let storage = tempfile::tempdir().unwrap(); + let session = "legacy-live-writer"; + let id = "bash-0123456789abcdef"; + let task = create_task_layout(storage.path(), session, id).unwrap(); + let handles = TaskIoHandles::create(&task, BgMode::Pipes, false).unwrap(); + let release = storage.path().join("release"); + let mut command = Command::new("/bin/sh"); + command.args([ + "-c", + "printf before; while ! test -f \"$1\"; do sleep 0.02; done; printf after; printf error-after >&2; printf 37 > \"$2\"; exit 37", + "legacy-writer", + ]); + command.arg(&release).arg(&task.paths.exit); + command.stdout(Stdio::from( + handles.clone_file(TaskArtifact::Stdout).unwrap(), + )); + command.stderr(Stdio::from( + handles.clone_file(TaskArtifact::Stderr).unwrap(), + )); + super::process::start_new_session(&mut command); + let mut child = command.spawn().unwrap(); + let mut metadata = PersistedTask::starting( + id.into(), + session.into(), + "legacy writer".into(), + storage.path().into(), + None, + None, + true, + false, + ); + metadata.mark_running(child.id(), child.id() as i32); + write_task_at(&task, &metadata).unwrap(); + let harness = storage.path().join(Harness::Opencode.storage_segment()); + fs::create_dir_all(&harness).unwrap(); + let _volume = simulate(&harness, u64::MAX - 20); + + super::repair_legacy_root_tasks(storage.path(), Harness::Opencode); + let registry = BgTaskRegistry::default(); + registry.replay_session(&harness, session).unwrap(); + let running = registry.status(id, session, None, None, 4096).unwrap(); + let replay_exit = registry.task_exit_path(id, session); + // Release even on the broken implementation so no child is left waiting. + fs::write(&release, b"finish").unwrap(); + assert_eq!(child.wait().unwrap().code(), Some(37)); + assert_eq!(running.info.status, BgTaskStatus::Running); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + let completed = loop { + let snapshot = registry.status(id, session, None, None, 4096).unwrap(); + if snapshot.info.status.is_terminal() { + break snapshot; + } + assert!(std::time::Instant::now() < deadline, "task did not finish"); + std::thread::sleep(std::time::Duration::from_millis(20)); + }; + assert_eq!(completed.exit_code, Some(37)); + assert_eq!(completed.info.status, BgTaskStatus::Failed); + assert_eq!(replay_exit, Some(task.paths.exit.clone())); + assert_eq!(fs::read(&task.paths.stdout).unwrap(), b"beforeafter"); + assert_eq!(fs::read(&task.paths.stderr).unwrap(), b"error-after"); + assert!(task.paths.exit.exists()); + registry.repair_legacy_root_tasks(storage.path(), Harness::Opencode); + assert!( + task.paths.exit.exists(), + "repeated configure must retain pinned paths" + ); + registry.detach(); + drop(registry); + drop(handles); + drop(task); + + super::repair_legacy_root_tasks(storage.path(), Harness::Opencode); + assert!(!storage.path().join("bash-tasks").exists()); + let moved = resolve_task(&harness, session, id).unwrap(); + assert_eq!(read_task_at(&moved).unwrap().exit_code, Some(37)); + assert_eq!(fs::read(&moved.paths.stdout).unwrap(), b"beforeafter"); + assert_eq!( + read_exit_marker(&moved.paths).unwrap(), + Some(ExitMarker::Code(37)) + ); + } + #[test] fn legacy_root_tasks_move_into_a_harness_folder_on_another_volume() { + use super::persistence::*; let storage = tempfile::tempdir().unwrap(); let root_tasks = storage.path().join("bash-tasks"); - fs::create_dir_all(root_tasks.join("task-1")).unwrap(); - fs::write(root_tasks.join("task-1/meta.json"), b"{\"id\":1}").unwrap(); + let id = "bash-0123456789abcdef"; + let task = create_task_layout(storage.path(), "terminal", id).unwrap(); + let mut metadata = PersistedTask::starting( + id.into(), + "terminal".into(), + "done".into(), + storage.path().into(), + None, + None, + true, + false, + ); + metadata.mark_terminal(super::BgTaskStatus::Completed, Some(0), None); + write_task_at(&task, &metadata).unwrap(); + drop(task); let harness_dir = storage.path().join(Harness::Opencode.storage_segment()); fs::create_dir_all(&harness_dir).unwrap(); // Every rename from the root into the harness folder now fails with EXDEV. @@ -1666,8 +1815,10 @@ mod legacy_root_task_repair_tests { super::repair_legacy_root_tasks(storage.path(), Harness::Opencode); assert_eq!( - fs::read(harness_dir.join("bash-tasks/task-1/meta.json")).unwrap(), - b"{\"id\":1}", + read_task_at(&resolve_task(&harness_dir, "terminal", id).unwrap()) + .unwrap() + .exit_code, + Some(0), "the task must reach the harness folder" ); assert!( diff --git a/crates/aft/src/bash_background/persistence.rs b/crates/aft/src/bash_background/persistence.rs index 27d547b32..b192c63cc 100644 --- a/crates/aft/src/bash_background/persistence.rs +++ b/crates/aft/src/bash_background/persistence.rs @@ -875,6 +875,19 @@ pub fn session_tasks_dir(storage_dir: &Path, session_id: &str) -> PathBuf { if direct.exists() { return direct; } + // Configure replays through a harness root. Live pre-namespace tasks must + // retain their original paths until their detached writers have stopped. + if storage_dir + .file_name() + .is_some_and(|name| name == "opencode" || name == "pi") + { + if let Some(root) = storage_dir.parent() { + let legacy = root.join("bash-tasks").join(&session_hash); + if legacy.exists() { + return legacy; + } + } + } let mut harness_matches = ["opencode", "pi"] .into_iter() .map(|harness| { diff --git a/crates/aft/src/bash_background/registry.rs b/crates/aft/src/bash_background/registry.rs index f9811c9d4..49365c684 100644 --- a/crates/aft/src/bash_background/registry.rs +++ b/crates/aft/src/bash_background/registry.rs @@ -4273,6 +4273,21 @@ impl BgTaskRegistry { self.replay_session_inner(storage_dir, session_id, Some(project_root)) } + pub(crate) fn repair_legacy_root_tasks(&self, storage_root: &Path, harness: Harness) { + let root_tasks = storage_root.join("bash-tasks"); + // Replayed tasks pin their artifact directories, including after exit. + // Defer their migration to a later daemon rather than invalidate those + // handles during a repeated configure in this daemon. + if self + .task_arcs() + .iter() + .any(|task| task.paths.dir.starts_with(&root_tasks)) + { + return; + } + super::repair_legacy_root_tasks(storage_root, harness); + } + fn retire_orphaned_watch_tombstones(&self, binding_session_id: &str) -> Result<(), String> { // Every harness seen on this root: an orphan written by one harness's // route must be retired even after another harness configured it. diff --git a/crates/aft/src/commands/configure.rs b/crates/aft/src/commands/configure.rs index 506574cda..1d84e6198 100644 --- a/crates/aft/src/commands/configure.rs +++ b/crates/aft/src/commands/configure.rs @@ -6495,7 +6495,8 @@ fn replay_configure_session_parts( session_id: &str, root_path: &Path, ) { - crate::bash_background::repair_legacy_root_tasks(storage_root, harness); + ctx.bash_background() + .repair_legacy_root_tasks(storage_root, harness); // A replayed task a worker was waiting on is granted one worker wait // window from now, so the registry needs the configured window first. ctx.bash_background() diff --git a/crates/aft/src/durability.rs b/crates/aft/src/durability.rs index 828708f60..8664c06dd 100644 --- a/crates/aft/src/durability.rs +++ b/crates/aft/src/durability.rs @@ -95,6 +95,18 @@ pub(crate) fn sync_file(file: &File, path: &Path) -> io::Result<()> { } pub(crate) fn sync_dir(path: &Path) -> io::Result<()> { + #[cfg(test)] + if FAIL_DIRECTORY_SYNC.with(|slot| { + let mut slot = slot.borrow_mut(); + if slot.as_deref() == Some(path) { + slot.take(); + true + } else { + false + } + }) { + return Err(io::Error::other("injected directory sync failure")); + } #[cfg(unix)] { File::open(path)?.sync_all()?; @@ -119,6 +131,9 @@ pub(crate) enum EventKind { #[cfg(test)] thread_local! { + pub(crate) static FAIL_DIRECTORY_SYNC: std::cell::RefCell> = const { + std::cell::RefCell::new(None) + }; static EVENTS: std::cell::RefCell> = const { std::cell::RefCell::new(Vec::new()) }; diff --git a/crates/aft/src/storage_retention.rs b/crates/aft/src/storage_retention.rs index c96d4e57b..e9fb2f081 100644 --- a/crates/aft/src/storage_retention.rs +++ b/crates/aft/src/storage_retention.rs @@ -1213,7 +1213,14 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result>>()? } Ok(_) => return Err(io::Error::other("non-directory retention trash retained")), - Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(false), + Err(error) if error.kind() == io::ErrorKind::NotFound => { + if mount_local { + // A previous pass may have unlinked the container but failed + // to make that deletion durable. Retry the parent barrier. + crate::durability::sync_dir(trash.parent().unwrap())?; + } + return Ok(false); + } Err(error) => return Err(error), } }; @@ -1256,6 +1263,14 @@ fn reap_trash(storage: &Path, trash: &Path, mount_local: bool) -> io::Result {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + crate::durability::sync_dir(trash.parent().unwrap())?; } Ok(pending) } @@ -3562,6 +3577,59 @@ mod storage_retention_tests { assert!(marker.exists(), "unreaped tombstones keep the marker"); } + fn mount_trash_sync_failure_keeps_marker(keep_container: bool) { + let (temp, _, _, path) = due_cache_fixture(); + let _views = crate::walk_boundary::simulated_volumes::simulate( + &temp.path().join("views"), + u64::MAX - 10, + ); + let _mount = crate::walk_boundary::simulated_volumes::simulate(&path, u64::MAX - 11); + { + let _admission = barrier(temp.path()).unwrap(); + retire_directory(temp.path(), &path).unwrap(); + } + let trash = path.join(".retention-trash"); + if keep_container { + fs::write(trash.join("unknown"), b"keep unrecognized entries").unwrap(); + } + let sync_path = if keep_container { &trash } else { &path }; + let marker = temp.path().join("retention/mount-trash-pending"); + let generation = fs::read(&marker).unwrap(); + crate::durability::FAIL_DIRECTORY_SYNC.with(|slot| { + *slot.borrow_mut() = Some(sync_path.to_path_buf()); + }); + let result = reap_retired(temp.path()); + crate::durability::FAIL_DIRECTORY_SYNC.with(|slot| slot.borrow_mut().take()); + assert!( + result.is_err(), + "mount-local sync failure must defer reaping" + ); + assert_eq!(fs::read(&marker).unwrap(), generation); + assert_eq!(trash.exists(), keep_container); + + crate::durability::take(); + reap_retired(temp.path()).unwrap(); + assert!(!marker.exists(), "a durable reap clears the pending marker"); + #[cfg(unix)] + { + let events = crate::durability::take(); + assert!(events.contains(&(crate::durability::EventKind::DirectorySync, path))); + if keep_container { + assert!(events.contains(&(crate::durability::EventKind::DirectorySync, trash))); + } + } + } + + #[test] + fn retention_mount_trash_parent_sync_failure_keeps_marker() { + mount_trash_sync_failure_keeps_marker(false); + } + + #[test] + fn retention_mount_trash_container_sync_failure_keeps_marker() { + mount_trash_sync_failure_keeps_marker(true); + } + #[test] fn retention_mount_trash_marker_survives_concurrent_retirement() { let (temp, _, _, path) = due_cache_fixture(); diff --git a/crates/aft/tests/integration/backups_dual_write_test.rs b/crates/aft/tests/integration/backups_dual_write_test.rs index d3a38abe7..839530c2f 100644 --- a/crates/aft/tests/integration/backups_dual_write_test.rs +++ b/crates/aft/tests/integration/backups_dual_write_test.rs @@ -460,8 +460,8 @@ fn backup_process_maintenance_repairs_legacy_root_backups() { "legacy root backups should be moved under the configured harness" ); assert!( - !storage.path().join("backups").exists(), - "legacy root backups directory should be removed after repair" + !legacy.exists(), + "legacy session should be removed after repair; stable admission gates stay" ); }