diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ebc47625..97ade85c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -121,6 +121,10 @@ jobs: timeout-minutes: 10 run: cargo test --release --test fsx -- --test-threads=1 --nocapture + - name: fsx paranoid (CAS round-trip per mutation) + timeout-minutes: 15 + run: cargo test --release --test fsx_paranoid -- --test-threads=1 --nocapture + xfstests: name: xfstests (filesystem exerciser) runs-on: diff --git a/Cargo.lock b/Cargo.lock index 1945abe2..bdabc250 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3751,7 +3751,7 @@ checksum = "1ffae5123b2d3fc086436f8834ae3ab053a283cfac8fe0a0b8eaae044768a4c4" [[package]] name = "xet-client" version = "1.5.2" -source = "git+https://github.com/huggingface/xet-core.git?branch=main#40f9530753e48f9517b5ad09e2339df7059d8de4" +source = "git+https://github.com/huggingface/xet-core.git?rev=40f9530753e48f9517b5ad09e2339df7059d8de4#40f9530753e48f9517b5ad09e2339df7059d8de4" dependencies = [ "anyhow", "async-trait", @@ -3788,7 +3788,7 @@ dependencies = [ [[package]] name = "xet-core-structures" version = "1.5.2" -source = "git+https://github.com/huggingface/xet-core.git?branch=main#40f9530753e48f9517b5ad09e2339df7059d8de4" +source = "git+https://github.com/huggingface/xet-core.git?rev=40f9530753e48f9517b5ad09e2339df7059d8de4#40f9530753e48f9517b5ad09e2339df7059d8de4" dependencies = [ "async-trait", "base64", @@ -3824,7 +3824,7 @@ dependencies = [ [[package]] name = "xet-data" version = "1.5.2" -source = "git+https://github.com/huggingface/xet-core.git?branch=main#40f9530753e48f9517b5ad09e2339df7059d8de4" +source = "git+https://github.com/huggingface/xet-core.git?rev=40f9530753e48f9517b5ad09e2339df7059d8de4#40f9530753e48f9517b5ad09e2339df7059d8de4" dependencies = [ "anyhow", "async-trait", @@ -3856,7 +3856,7 @@ dependencies = [ [[package]] name = "xet-runtime" version = "1.5.2" -source = "git+https://github.com/huggingface/xet-core.git?branch=main#40f9530753e48f9517b5ad09e2339df7059d8de4" +source = "git+https://github.com/huggingface/xet-core.git?rev=40f9530753e48f9517b5ad09e2339df7059d8de4#40f9530753e48f9517b5ad09e2339df7059d8de4" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index d3f115f8..f7185aa8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,10 +10,10 @@ categories = ["filesystem", "command-line-utilities"] [dependencies] # xet-core crates -xet-client = { git = "https://github.com/huggingface/xet-core.git", branch = "main" } -xet-core-structures = { git = "https://github.com/huggingface/xet-core.git", branch = "main" } -xet-data = { git = "https://github.com/huggingface/xet-core.git", branch = "main" } -xet-runtime = { git = "https://github.com/huggingface/xet-core.git", branch = "main" } +xet-client = { git = "https://github.com/huggingface/xet-core.git", rev = "40f9530753e48f9517b5ad09e2339df7059d8de4" } +xet-core-structures = { git = "https://github.com/huggingface/xet-core.git", rev = "40f9530753e48f9517b5ad09e2339df7059d8de4" } +xet-data = { git = "https://github.com/huggingface/xet-core.git", rev = "40f9530753e48f9517b5ad09e2339df7059d8de4" } +xet-runtime = { git = "https://github.com/huggingface/xet-core.git", rev = "40f9530753e48f9517b5ad09e2339df7059d8de4" } # External crates async-trait = "0.1" diff --git a/src/cached_xet_client.rs b/src/cached_xet_client.rs index 2d7056e3..c778fd33 100644 --- a/src/cached_xet_client.rs +++ b/src/cached_xet_client.rs @@ -77,7 +77,7 @@ impl CachedXetClient { } } -/// Derive a range-scoped `QueryReconstructionResponse` from a cached full-file response. +/// Derive a range-scoped `QueryReconstructionResponseV2` from a cached full-file response. /// /// The full-file response lists all terms in file order with their unpacked byte lengths. /// We walk the terms, track cumulative byte offsets, and keep only terms that overlap diff --git a/src/nfs.rs b/src/nfs.rs index 47ad9496..60b56ff4 100644 --- a/src/nfs.rs +++ b/src/nfs.rs @@ -816,6 +816,11 @@ fn errno_to_nfs(e: i32) -> nfsstat3 { libc::ENOTEMPTY => nfsstat3::NFS3ERR_NOTEMPTY, libc::EBADF => nfsstat3::NFS3ERR_STALE, libc::ENOSPC => nfsstat3::NFS3ERR_NOSPC, + // EAGAIN signals a transient condition the caller should retry + // (e.g., open_advanced_write exhausted its drift-retry budget). + // NFS3ERR_JUKEBOX tells the client to back off and retry; mapping + // to EIO would surface as a hard failure for a recoverable case. + libc::EAGAIN => nfsstat3::NFS3ERR_JUKEBOX, _ => nfsstat3::NFS3ERR_IO, } } diff --git a/src/setup.rs b/src/setup.rs index cfa77535..fb8d9195 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -104,6 +104,15 @@ pub struct MountOptions { #[arg(long, default_value_t = false)] pub advanced_writes: bool, + /// EXPERIMENTAL (beta). Skip the full CAS download when opening for write + /// and only upload modified byte ranges via `range_upload`. Much faster + /// for large files with small edits, but exercises more lifecycle paths + /// (handle pool reuse, drift retries, multi-handle reads/writes) than the + /// download-then-upload mode. Off by default — opt in if you understand + /// the trade-off. Implies --advanced-writes. + #[arg(long, default_value_t = false)] + pub sparse_writes: bool, + /// Interval in seconds for polling remote changes (0 to disable). #[arg(long, default_value_t = 30)] pub poll_interval_secs: u64, @@ -230,6 +239,7 @@ pub struct MountSetup { pub mount_point: PathBuf, pub read_only: bool, pub advanced_writes: bool, + pub sparse_writes: bool, pub direct_io: bool, pub metadata_ttl: std::time::Duration, pub max_threads: usize, @@ -428,6 +438,18 @@ pub fn build_with_runtime( let xet_sessions = XetSessions::new(xet_ctx, download_session, upload_config, cached_client, xorb_cache); let advanced_writes = options.advanced_writes || options.overlay || (is_nfs && !read_only); + // Sparse writes are an experimental optimization on top of advanced writes. + // They imply advanced_writes (the advanced_writes path is where the sparse + // staging lives), but advanced_writes does NOT imply sparse_writes — the + // safe default is to download-then-write, which is well-tested and avoids + // the lifecycle edge cases that sparse staging exposes. + let sparse_writes = options.sparse_writes && advanced_writes; + if options.sparse_writes && !advanced_writes { + warn!("--sparse-writes ignored: requires --advanced-writes (or NFS read-write, or --overlay)"); + } + if sparse_writes { + warn!("--sparse-writes is EXPERIMENTAL. Disable with `--sparse-writes=false` if you see issues."); + } // Overlay: open a pre-mount fd to the mount point directory. The fd is // held by OverlayBacking so overlay-local filesystem ops can stay rooted @@ -526,6 +548,7 @@ pub fn build_with_runtime( VfsConfig { read_only, advanced_writes, + sparse_writes, uid, gid, poll_interval_secs: options.poll_interval_secs, @@ -552,6 +575,7 @@ pub fn build_with_runtime( mount_point, read_only, advanced_writes, + sparse_writes, direct_io: options.direct_io, metadata_ttl, max_threads: options.max_threads, diff --git a/src/test_mocks.rs b/src/test_mocks.rs index 8d945c38..3e0ba443 100644 --- a/src/test_mocks.rs +++ b/src/test_mocks.rs @@ -12,6 +12,7 @@ use xet_data::processing::XetFileInfo; use crate::error::{Error, Result}; use crate::hub_api::{BatchOp, HeadFileInfo, HubOps, SourceKind, TreeEntry}; use crate::overlay::OverlayBacking; +use crate::virtual_fs::inode::SparseWriteState; use crate::xet::{DownloadStreamOps, StagingDir, StreamingWriterOps, XetOps}; // ── MockHub ─────────────────────────────────────────────────────────── @@ -29,6 +30,11 @@ pub struct MockHub { list_tree_calls: AtomicU32, head_file_calls: AtomicU32, probe_revision_calls: AtomicU32, + /// Path → bytes for non-Xet bucket objects. `download_file_http` writes + /// these to the requested dest path so tests can exercise the HTTP path + /// with real content. Files without an entry get an empty body + /// (mirrors the existing behavior). + bucket_content: Mutex>>, /// `Ok(rev)` returns the token; `Err((status, msg))` rebuilds an /// `Error::Hub` with that status so the poll loop's 401-branch still fires. revision: Mutex, String)>>, @@ -52,6 +58,7 @@ impl MockHub { list_tree_calls: AtomicU32::new(0), head_file_calls: AtomicU32::new(0), probe_revision_calls: AtomicU32::new(0), + bucket_content: Mutex::new(HashMap::new()), revision: Mutex::new(Ok("rev-0".to_string())), }) } @@ -74,10 +81,22 @@ impl MockHub { list_tree_calls: AtomicU32::new(0), head_file_calls: AtomicU32::new(0), probe_revision_calls: AtomicU32::new(0), + bucket_content: Mutex::new(HashMap::new()), revision: Mutex::new(Ok("rev-0".to_string())), }) } + /// Register HTTP-served content for a non-Xet bucket file. Tests use + /// this to give `download_file_http` something to serve when exercising + /// write paths on non-Xet files (e.g., setattr-shrink that downloads + /// the original before truncating). + pub fn set_bucket_content(&self, path: &str, content: &[u8]) { + self.bucket_content + .lock() + .unwrap() + .insert(path.to_string(), content.to_vec()); + } + pub fn add_file(&self, path: &str, size: u64, xet_hash: Option<&str>, oid: Option<&str>) { self.tree.lock().unwrap().push(TreeEntry { path: path.to_string(), @@ -156,6 +175,10 @@ impl MockHub { *self.batch_barrier.lock().unwrap() = Some(barrier); } + pub fn clear_batch_barrier(&self) { + *self.batch_barrier.lock().unwrap() = None; + } + pub fn take_batch_log(&self) -> Vec> { std::mem::take(&mut *self.batch_log.lock().unwrap()) } @@ -241,15 +264,21 @@ impl HubOps for MockHub { Ok(()) } - async fn download_file_http(&self, _path: &str, dest: &Path) -> Result<()> { + async fn download_file_http(&self, path: &str, dest: &Path) -> Result<()> { if self.download_fail.swap(false, Ordering::SeqCst) { return Err(Error::hub("mock download failure")); } - // Create an empty file at dest so open_local_readonly can open it. if let Some(parent) = dest.parent() { std::fs::create_dir_all(parent).ok(); } - std::fs::write(dest, b"").map_err(Error::Io)?; + let content = self + .bucket_content + .lock() + .unwrap() + .get(path) + .cloned() + .unwrap_or_default(); + std::fs::write(dest, &content).map_err(Error::Io)?; Ok(()) } @@ -278,11 +307,12 @@ impl HubOps for MockHub { // ── MockXet ─────────────────────────────────────────────────────────── pub struct MockXet { - files: Mutex>>, + files: Arc>>>, pub next_hash: AtomicU64, writer_create_fail: AtomicBool, upload_fail: AtomicBool, download_fail: AtomicBool, + range_upload_fail: AtomicBool, writer_fail_after: AtomicU64, /// Number of range download calls that should fail before succeeding. range_fail_count: AtomicU32, @@ -309,11 +339,12 @@ pub struct UploadGate { impl MockXet { pub fn new() -> Arc { Arc::new(Self { - files: Mutex::new(HashMap::new()), + files: Arc::new(Mutex::new(HashMap::new())), next_hash: AtomicU64::new(1), writer_create_fail: AtomicBool::new(false), upload_fail: AtomicBool::new(false), download_fail: AtomicBool::new(false), + range_upload_fail: AtomicBool::new(false), writer_fail_after: AtomicU64::new(u64::MAX), range_fail_count: AtomicU32::new(0), range_empty_count: AtomicU32::new(0), @@ -362,6 +393,16 @@ impl MockXet { self.range_empty_count.store(n, Ordering::SeqCst); } + #[allow(dead_code)] + pub fn fail_range_upload(&self) { + self.range_upload_fail.store(true, Ordering::SeqCst); + } + + #[allow(dead_code)] + pub fn get_file(&self, hash: &str) -> Option> { + self.files.lock().unwrap().get(hash).cloned() + } + fn next_hash_string(&self) -> String { format!("mock_hash_{}", self.next_hash.fetch_add(1, Ordering::SeqCst)) } @@ -378,6 +419,7 @@ impl XetOps for MockXet { data: Vec::new(), hash: self.next_hash_string(), fail_after, + files: self.files.clone(), })) } @@ -427,6 +469,58 @@ impl XetOps for MockXet { // re-enabled when xet-core adds chunk sizes to XorbReconstructionTerm. } + async fn range_upload( + &self, + sparse_state: &SparseWriteState, + staging_path: &std::path::Path, + file_size: u64, + _io_lock: Arc>, + ) -> crate::error::Result { + if self.range_upload_fail.swap(false, Ordering::SeqCst) { + return Err(crate::error::Error::Xet("mock range_upload failure".into())); + } + + // Mirror real XetSessions::range_upload: nothing dirty + size unchanged + // means the original hash is preserved (no upload, no Hub commit). + if sparse_state.dirty_ranges.is_empty() && file_size == sparse_state.original_size { + return Ok(XetFileInfo::new( + sparse_state.original_hash.clone(), + sparse_state.original_size, + )); + } + + let original = self + .files + .lock() + .unwrap() + .get(&sparse_state.original_hash) + .cloned() + .unwrap_or_default(); + let staging = std::fs::read(staging_path).map_err(Error::Io)?; + let total_size = staging.len(); + + // Compose: original as base (capped to sparse_state.original_size), + // overlay dirty ranges from staging. Region past original_size is zeros + // (extension), matching real upload_ranges behavior. + let mut composed = vec![0u8; total_size]; + let orig_end = (sparse_state.original_size as usize) + .min(original.len()) + .min(total_size); + composed[..orig_end].copy_from_slice(&original[..orig_end]); + for &(start, end) in &sparse_state.dirty_ranges { + let start = start as usize; + let end = (end as usize).min(total_size); + if start < end { + composed[start..end].copy_from_slice(&staging[start..end]); + } + } + + let hash = self.next_hash_string(); + let size = composed.len() as u64; + self.files.lock().unwrap().insert(hash.clone(), composed); + Ok(XetFileInfo::new(hash, size)) + } + fn download_stream_boxed( &self, file_info: &XetFileInfo, @@ -468,6 +562,12 @@ pub struct MockStreamingWriter { data: Vec, hash: String, fail_after: u64, + /// Shared handle into the parent MockXet's file map. Registering the + /// uploaded data here on finish mirrors the production CAS behavior where + /// a successful streaming upload makes the new hash retrievable via + /// download_stream — needed for read paths (e.g. fill_sparse_holes) that + /// re-read the freshly-uploaded content. + files: Arc>>>, } #[async_trait::async_trait] @@ -482,7 +582,8 @@ impl StreamingWriterOps for MockStreamingWriter { async fn finish_boxed(self: Box) -> Result { let size = self.data.len() as u64; - Ok(XetFileInfo::new(self.hash.clone(), size)) + self.files.lock().unwrap().insert(self.hash.clone(), self.data); + Ok(XetFileInfo::new(self.hash, size)) } fn len(&self) -> u64 { @@ -522,6 +623,10 @@ impl DownloadStreamOps for MockDownloadStream { pub struct TestOpts { pub read_only: bool, pub advanced_writes: bool, + /// Beta sparse-write path. Default true in tests so the existing sparse + /// regression tests keep exercising that code path; opt-out for tests + /// that specifically cover the non-sparse fallback. + pub sparse_writes: bool, pub overlay: bool, pub serve_lookup_from_cache: bool, pub metadata_ttl: Duration, @@ -534,6 +639,9 @@ impl Default for TestOpts { Self { read_only: false, advanced_writes: false, + // Default true so existing sparse regression tests keep + // exercising the sparse path without needing opt-in. + sparse_writes: true, overlay: false, serve_lookup_from_cache: false, metadata_ttl: Duration::from_secs(1), @@ -610,6 +718,7 @@ pub fn make_test_vfs( crate::virtual_fs::VfsConfig { read_only: opts.read_only, advanced_writes: opts.advanced_writes, + sparse_writes: opts.sparse_writes, uid: 1000, gid: 1000, poll_interval_secs: 0, @@ -653,6 +762,7 @@ pub fn make_overlay_test_vfs_with_root( crate::virtual_fs::VfsConfig { read_only: false, advanced_writes: false, + sparse_writes: false, uid: 1000, gid: 1000, poll_interval_secs: 0, diff --git a/src/virtual_fs/flush.rs b/src/virtual_fs/flush.rs index 5dad4c7c..7aea24cc 100644 --- a/src/virtual_fs/flush.rs +++ b/src/virtual_fs/flush.rs @@ -17,6 +17,13 @@ enum FlushSignal { Dirty(u64), /// Wake the loop to drain pending remote deletes (no dirty inode attached). WakeDeletes, + /// Tell `flush_loop` to drop its self-referencing sender clone so the + /// channel can close once the outer `FlushManager::tx` is dropped. Sent + /// by `FlushManager::shutdown` just before it drops the outer tx — + /// without this, the clone `flush_loop` holds (for `requeue_siblings`) + /// keeps the channel alive forever and shutdown deadlocks waiting on + /// the loop's join handle. + Shutdown, } // ── FlushManager ────────────────────────────────────────────────────── @@ -50,8 +57,10 @@ impl FlushManager { let bg_errors = errors.clone(); let bg_deletes = pending_deletes.clone(); let bg_hub = hub_client.clone(); + let bg_tx = tx.clone(); let handle = runtime.spawn(flush_loop( rx, + bg_tx, xet_sessions, staging, bg_hub, @@ -131,6 +140,12 @@ impl FlushManager { for ino in dirty_inos { let _ = tx.send(FlushSignal::Dirty(ino)); } + // Tell flush_loop to drop its self-referencing sender so the + // channel can close after we drop ours below. flush_loop + // holds a clone of `tx` (for `requeue_siblings` on abort); + // without this signal, that clone would keep the channel + // alive forever and shutdown would deadlock. + let _ = tx.send(FlushSignal::Shutdown); } // Drop the sender to signal the flush loop to drain and exit self.tx.lock().expect("flush_tx poisoned").take(); @@ -165,8 +180,14 @@ fn run_blocking(f: F) { // ── Background tasks ────────────────────────────────────────────────── #[allow(clippy::too_many_arguments)] +/// `signal_tx`: self-referencing sender — passed to `flush_batch` so it can +/// re-enqueue still-dirty siblings when a batch aborts mid-upload. Without +/// this, an abort leaves the surviving items dirty but invisible to the loop +/// until some other code path enqueues them (which may never happen, +/// stranding the bytes on disk and never committing them to the Hub). async fn flush_loop( mut rx: mpsc::UnboundedReceiver, + signal_tx: mpsc::UnboundedSender, xet_sessions: Arc, staging: Arc, hub_client: Arc, @@ -176,9 +197,21 @@ async fn flush_loop( max_batch_window: Duration, pending_deletes: Arc>>, ) { + // Wrap the self-referencing sender in Option so we can drop it on + // `Shutdown`. Keeping it alive past shutdown would prevent the channel + // from closing and `rx.recv()` would block forever. + let mut signal_tx = Some(signal_tx); loop { // Wait for the first signal let first = match rx.recv().await { + Some(FlushSignal::Shutdown) => { + // Drop our sender clone so the channel can close after the + // outer FlushManager.tx is dropped. Continue draining any + // remaining signals already in the buffer (Dirty/WakeDeletes + // queued before Shutdown) — those still represent real work. + signal_tx = None; + continue; + } Some(sig) => sig, None => return, // channel closed, exit }; @@ -195,6 +228,12 @@ async fn flush_loop( } let timeout = debounce.min(remaining); match tokio::time::timeout(timeout, rx.recv()).await { + Ok(Some(FlushSignal::Shutdown)) => { + // Same as the outer arm: drop our sender clone, continue + // draining the rest of this debounce window so queued + // dirty work still gets flushed before the loop exits. + signal_tx = None; + } Ok(Some(sig)) => signals.push(sig), _ => break, // timeout (debounce expired) or channel closed } @@ -203,12 +242,12 @@ async fn flush_loop( // Flush queued remote deletes alongside dirty writes. flush_pending_deletes(&pending_deletes, &*hub_client).await; - // Extract dirty inode IDs (WakeDeletes signals carry no inode). + // Extract dirty inode IDs (WakeDeletes/Shutdown carry no inode). let dirty_inos: Vec = signals .into_iter() .filter_map(|sig| match sig { FlushSignal::Dirty(ino) => Some(ino), - FlushSignal::WakeDeletes => None, + FlushSignal::WakeDeletes | FlushSignal::Shutdown => None, }) .collect(); @@ -221,6 +260,7 @@ async fn flush_loop( &*hub_client, &inodes, &flush_errors, + signal_tx.as_ref(), ) .await; } @@ -261,6 +301,72 @@ async fn flush_pending_deletes(queue: &Mutex>, hub_client: &dyn HubO } } +/// Mark the given items with `msg` in the flush error map. Pass only the items +/// whose own upload genuinely failed — siblings whose CAS upload succeeded in +/// a prior chunk (or were never reached) stay dirty and will retry on the next +/// flush cycle; surfacing an error on them would produce spurious EIO on files +/// whose bytes are already in CAS. +fn abort_batch(items: &[FlushItem], flush_errors: &Mutex>, msg: String) { + error!("Aborting flush ({} item(s) affected): {}", items.len(), msg); + let mut errs = flush_errors.lock().expect("flush_errors poisoned"); + for it in items { + errs.insert(it.ino, msg.clone()); + } +} + +/// Re-enqueue dirty siblings after a batch abort. +/// +/// `to_flush` is the entire batch that was being processed; `failed_inos` +/// are the ones whose own upload genuinely failed (now in `flush_errors`). +/// Everything else is still dirty in the inode table but has lost its +/// queue signal — without re-enqueueing, those bytes would sit on disk +/// indefinitely with nothing to wake the flush loop. +/// +/// This includes BOTH items already uploaded earlier in the batch (which +/// never got their Hub commit because we aborted before that step) and +/// items not yet reached. Both still need a fresh flush cycle. +fn requeue_siblings( + to_flush: &[FlushItem], + failed_inos: &[u64], + signal_tx: Option<&mpsc::UnboundedSender>, +) { + let Some(tx) = signal_tx else { + // Shutdown in progress — don't try to re-enqueue, the loop is exiting. + return; + }; + let failed: HashSet = failed_inos.iter().copied().collect(); + let mut requeued = 0; + for it in to_flush { + if failed.contains(&it.ino) { + continue; + } + if tx.send(FlushSignal::Dirty(it.ino)).is_err() { + // Channel closed (rare — outer FlushManager.tx was dropped + // without a Shutdown signal). The siblings stay visible as + // dirty in the inode table but won't be flushed this run. + return; + } + requeued += 1; + } + if requeued > 0 { + warn!("Re-enqueued {} sibling(s) after batch abort", requeued); + } +} + +/// Length of the contiguous run of non-sparse FlushItems starting at `start`, +/// capped to `max` items. `start` must point to a non-sparse item — sparse +/// items are dispatched one-by-one through `range_upload` and never get +/// included in a batched `upload_files` chunk. +fn find_regular_run_end(items: &[FlushItem], start: usize, max: usize) -> usize { + debug_assert!(items[start].sparse_write.is_none()); + let upper = (start + max).min(items.len()); + items[start..upper] + .iter() + .take_while(|it| it.sparse_write.is_none()) + .count() + + start +} + struct FlushItem { ino: u64, full_path: String, @@ -270,9 +376,17 @@ struct FlushItem { /// Hash from the last successful commit, used to skip redundant Hub commits /// when the CAS upload produces the same hash (content unchanged). prev_xet_hash: Option, + /// Size of the file as the user sees it (including sparse holes). + file_size: u64, + /// Set when the staging file is sparse and only the dirty windows should be + /// re-uploaded via `range_upload` (composing CAS prefix/suffix). + sparse_write: Option>, } #[allow(clippy::too_many_arguments)] +/// `signal_tx`: `None` after `Shutdown` was observed — siblings won't be +/// re-enqueued because the loop is winding down anyway. They stay dirty on +/// disk and are picked up by the next mount. async fn flush_batch( pending: Vec, xet_sessions: &dyn XetOps, @@ -280,6 +394,7 @@ async fn flush_batch( hub_client: &dyn HubOps, inodes: &RwLock, flush_errors: &Mutex>, + signal_tx: Option<&mpsc::UnboundedSender>, ) { let staging_dir = staging.dir().expect("flush_batch requires staging directory"); // Walk backwards so the last request per ino wins, then reverse in-place. @@ -334,6 +449,8 @@ async fn flush_batch( pending_deletes: entry.pending_deletes.clone(), dirty_generation: entry.dirty_generation, prev_xet_hash: entry.xet_hash.clone(), + file_size: entry.size, + sparse_write: entry.sparse_write.clone(), }) }) .collect() @@ -347,14 +464,57 @@ async fn flush_batch( return; } - // Upload in chunks to bound FD usage (xet-core opens all staging files per - // upload session), but accumulate all batch ops for a single Hub commit to - // preserve the global adds-before-deletes ordering required by the Hub API. + // Sparse items (opened for write without downloading) go through `range_upload` + // which composes CAS prefix/suffix segments with re-chunked dirty windows; regular + // items go through the batched `upload_files` path. We walk in order, batching + // contiguous runs of regular items so a single Hub commit preserves the original + // adds-before-deletes ordering. Chunk size bounds FD usage (xet-core opens all + // staging files per upload session). const UPLOAD_CHUNK_SIZE: usize = 500; - let mut upload_results = Vec::with_capacity(to_flush.len()); + let mut upload_results: Vec = Vec::with_capacity(to_flush.len()); + + let mut i = 0; + while i < to_flush.len() { + let item = &to_flush[i]; + if let Some(sw) = &item.sparse_write { + let io_lock = staging.io_lock(item.ino); + match xet_sessions + .range_upload(sw, &item.staging_path, item.file_size, io_lock) + .await + { + Ok(file_info) => { + debug!( + "flush: range_upload ino={} path={} hash={} size={}", + item.ino, + item.full_path, + file_info.hash(), + file_info.file_size().unwrap_or(0) + ); + upload_results.push(file_info); + } + Err(e) => { + // Only the failing sparse item gets the error. Sibling + // items in this batch keep their dirty state and will be + // retried on the next flush cycle (CAS dedup makes the + // re-upload cheap); marking them errored here would + // surface spurious EIO on files whose data is fine. + let failed_ino = item.ino; + abort_batch( + std::slice::from_ref(item), + flush_errors, + format!("range_upload failed (ino={} path={}): {e}", item.ino, item.full_path), + ); + requeue_siblings(&to_flush, &[failed_ino], signal_tx); + return; + } + } + i += 1; + continue; + } - for (chunk_idx, chunk) in to_flush.chunks(UPLOAD_CHUNK_SIZE).enumerate() { - let staging_paths: Vec<&std::path::Path> = chunk.iter().map(|item| item.staging_path.as_path()).collect(); + let chunk_end = find_regular_run_end(&to_flush, i, UPLOAD_CHUNK_SIZE); + let chunk = &to_flush[i..chunk_end]; + let staging_paths: Vec<&std::path::Path> = chunk.iter().map(|it| it.staging_path.as_path()).collect(); match xet_sessions.upload_files(&staging_paths).await { Ok(results) => { assert_eq!( @@ -367,17 +527,18 @@ async fn flush_batch( upload_results.extend(results); } Err(e) => { - // Abort the entire batch: committing partial results could apply - // deletes without the corresponding adds from this failed chunk. - error!("Batch upload failed (chunk {}), aborting flush: {}", chunk_idx, e); - let msg = format!("upload failed: {e}"); - let mut errs = flush_errors.lock().expect("flush_errors poisoned"); - for item in &to_flush { - errs.insert(item.ino, msg.clone()); - } + // Only mark the chunk that actually failed; items in other + // chunks (already uploaded or not yet reached) stay dirty + // and need to be re-enqueued so a future flush cycle picks + // them up — otherwise they'd sit dirty forever with no + // signal to wake the flush loop. + let failed_inos: Vec = chunk.iter().map(|it| it.ino).collect(); + abort_batch(chunk, flush_errors, format!("upload failed: {e}")); + requeue_siblings(&to_flush, &failed_inos, signal_tx); return; } } + i = chunk_end; } // Uploads are done — drop the staging locks so unlink/truncate and the @@ -399,7 +560,21 @@ async fn flush_batch( let mut unchanged = vec![false; to_flush.len()]; for (i, (item, file_info)) in to_flush.iter().zip(upload_results.iter()).enumerate() { - if item.pending_deletes.is_empty() && item.prev_xet_hash.as_deref() == Some(file_info.hash()) { + // Was this a no-op upload? Two cases: + // * Sparse: `range_upload` returns `sparse_write.original_hash` when + // dirty_ranges is empty and the size matches — i.e. the open made no + // actual modifications since the snapshot. Compare against the + // snapshot, NOT `entry.xet_hash`, because `poll_remote_changes` may + // have updated the inode mid-flight; using prev_xet_hash here would + // treat the no-op as a change and roll the remote back to the snapshot. + // * Regular: full upload preserves the prior hash when content is + // identical (idempotent edits). + let is_no_op = if let Some(sw) = &item.sparse_write { + file_info.hash() == sw.original_hash + } else { + item.prev_xet_hash.as_deref() == Some(file_info.hash()) + }; + if item.pending_deletes.is_empty() && is_no_op { debug!( "flush_batch: unchanged ino={} path={} (hash {})", item.ino, @@ -428,17 +603,16 @@ async fn flush_batch( } // Clear dirty on unchanged files without waiting for the Hub round-trip. + // Use apply_noop_commit (not apply_commit) so we don't rewrite xet_hash/size + // — they may have legitimately been updated by poll_remote_changes during + // the open window. { let mut inode_table = inodes.write().expect("inodes poisoned"); - for (i, (item, file_info)) in to_flush.iter().zip(upload_results.iter()).enumerate() { + for (i, item) in to_flush.iter().enumerate() { if unchanged[i] && let Some(entry) = inode_table.get_mut(item.ino) { - entry.apply_commit( - file_info.hash(), - file_info.file_size().expect("upload returned XetFileInfo without size"), - item.dirty_generation, - ); + entry.apply_noop_commit(item.dirty_generation); } } } @@ -477,6 +651,7 @@ async fn flush_batch( file_info.hash(), file_info.file_size().expect("upload returned XetFileInfo without size"), item.dirty_generation, + item.sparse_write.is_some(), ); } } diff --git a/src/virtual_fs/inode.rs b/src/virtual_fs/inode.rs index e6238ff8..cb6e6b5e 100644 --- a/src/virtual_fs/inode.rs +++ b/src/virtual_fs/inode.rs @@ -53,6 +53,16 @@ pub struct EvictionState { /// with `open_handles > 0` — a racing read/write would silently lose /// data if the inode disappeared under it. pub open_handles: AtomicU32, + /// Subset of `open_handles` that were opened for write. Used by + /// `update_remote_file` to gate hash rotation: a writable handle + /// counts on `sparse_write` matching the inode's `xet_hash` for its + /// next write (lazy install + range_upload composition). Letting poll + /// clear or re-key `sparse_write` while a writable handle is open + /// produces silent corruption (codex P1). Read-only handles are not + /// affected: Lazy prefetch buffers are bound to the open-time hash, + /// and LocalFd reads defensively gate `fill_sparse_holes` on the + /// hash match (finding C2). + pub open_write_handles: AtomicU32, } impl Clone for EvictionState { @@ -62,6 +72,7 @@ impl Clone for EvictionState { last_touched: AtomicU64::new(self.last_touched.load(Ordering::Relaxed)), evict_pending: AtomicBool::new(self.evict_pending.load(Ordering::Relaxed)), open_handles: AtomicU32::new(self.open_handles.load(Ordering::Relaxed)), + open_write_handles: AtomicU32::new(self.open_write_handles.load(Ordering::Relaxed)), } } } @@ -145,6 +156,120 @@ pub struct InodeEntry { pub last_revalidated: Option, /// Eviction bookkeeping (kernel refcount, LRU recency, pending flag, pinning). pub eviction: EvictionState, + /// Tracks the original file state and dirty byte ranges when the file is opened + /// for write without downloading the full original content (sparse staging). + /// At flush time, only the dirty windows need to be re-uploaded via `upload_ranges`. + /// `None` means either a new file or the full file was downloaded. Race protection + /// (concurrent writes during flush) reuses `dirty_generation` — see `apply_commit`. + pub sparse_write: Option>, +} + +/// Tracks which regions of a sparse staging file have been modified. +/// The staging file is sparse: bytes in [0, original_size) are a hole (zeros) +/// unless a dirty range overlaps them, in which case they are lazily downloaded +/// from CAS. At flush time, only the modified regions need re-chunking/uploading. +#[derive(Debug, Clone)] +pub struct SparseWriteState { + /// Hash of the original file in CAS. + pub original_hash: String, + /// Size of the original file in CAS. Immutable after construction; passed to + /// `upload_ranges`, which validates it against the reconstruction info for + /// `original_hash`. A truncate-shrink does NOT change this — see + /// `effective_original_size` instead. + pub original_size: u64, + /// Effective live region of the original CAS file. Equals `original_size` + /// initially, then capped by truncate-shrinks. Used by `track_write` (to + /// know where the live "fillable" region ends) and `fill_sparse_holes` (to + /// stop reading CAS bytes past a truncate boundary). Always <= original_size. + pub effective_original_size: u64, + /// Sorted, non-overlapping dirty byte ranges (start, end), in current-file coordinates. + pub dirty_ranges: Vec<(u64, u64)>, + /// When true, the on-disk staging file holds the full original content in + /// [0, effective_original_size) — set for opens that reuse a current + /// staging cache (no fresh sparse hole was created). Reads can serve + /// directly from staging without consulting CAS; `fill_sparse_holes` + /// short-circuits to a no-op. Stays valid for the lifetime of the open + /// because dirty writes overlay onto the existing bytes (track_write + /// updates both staging and the dirty range list) and the snapshot hash + /// is pinned once dirty is set. + pub staging_holds_full_original: bool, +} + +impl SparseWriteState { + pub fn new(original_hash: String, original_size: u64) -> Self { + Self { + original_hash, + original_size, + effective_original_size: original_size, + dirty_ranges: Vec::new(), + staging_holds_full_original: false, + } + } + + /// Variant of `new` for reused-staging opens: the staging file already + /// holds the full original content, so reads bypass CAS. + pub fn new_with_full_staging(original_hash: String, original_size: u64) -> Self { + Self { + staging_holds_full_original: true, + ..Self::new(original_hash, original_size) + } + } + + /// Record a write at [offset, offset+len). Merges overlapping/adjacent ranges. + /// Uses binary search to find the affected region in O(log n + k) where k is + /// the number of ranges merged (typically 0-1 for sequential writes). + pub fn track_write(&mut self, offset: u64, len: u64) { + if len == 0 { + return; + } + // If writing past the live region, extend the range back to its end. + // The gap [effective_original_size, offset) is zeros in the sparse staging + // file (either never-touched holes, or zeroed by a prior truncate) and must + // be included in dirty_inputs so upload_ranges doesn't miss them. + let mut new_start = if offset > self.effective_original_size { + self.effective_original_size + } else { + offset + }; + let mut new_end = offset + len; + + // Binary search: first range whose end >= new_start (could overlap on the left) + let first = self.dirty_ranges.partition_point(|&(_, e)| e < new_start); + // Binary search: first range whose start > new_end (past the overlap zone) + let last = self.dirty_ranges[first..].partition_point(|&(s, _)| s <= new_end) + first; + + // Merge all overlapping ranges [first..last) into the new range + if first < last { + new_start = new_start.min(self.dirty_ranges[first].0); + new_end = new_end.max(self.dirty_ranges[last - 1].1); + } + + // Replace the overlapping slice with the single merged range + self.dirty_ranges.splice(first..last, [(new_start, new_end)]); + } + + /// Remove dirty ranges past `new_size`, cap overlapping ones, and drop any + /// range that becomes empty after clipping (defense — `track_write` never + /// produces zero-length ranges, but keeping the invariant `s < e` here means + /// downstream consumers don't have to worry about it). + fn trim_dirty_ranges(&mut self, new_size: u64) { + self.dirty_ranges.retain_mut(|&mut (ref s, ref mut e)| { + if *s >= new_size { + return false; + } + *e = (*e).min(new_size); + *s < *e + }); + } + + /// Clip the sparse state to a new (smaller) file size after a truncate-shrink. + /// Removes dirty ranges past `new_size` and lowers `effective_original_size` + /// so subsequent writes past the new EOF zero-fill the gap correctly. + /// `original_size` is left untouched — it is the immutable CAS object size. + pub fn clip_to_size(&mut self, new_size: u64) { + self.effective_original_size = self.effective_original_size.min(new_size); + self.trim_dirty_ranges(new_size); + } } impl InodeEntry { @@ -207,26 +332,96 @@ impl InodeEntry { } } + /// Clear dirty state after a no-op flush (the upload returned the same hash + /// the snapshot pointed at — no Hub commit needed). Used when the open made + /// no actual modifications. We must NOT roll `xet_hash`/`size` back to the + /// snapshot here, because `poll_remote_changes` may have legitimately + /// updated them to a newer remote revision during the open window; doing so + /// would silently revert the inode to the snapshot. + pub fn apply_noop_commit(&mut self, dirty_generation: u64) { + if self.clear_dirty_if(dirty_generation) { + self.pending_deletes.clear(); + // Keep `sparse_write` (still-open handles need it for + // `fill_sparse_holes`) but reset the parts that describe staging + // contents: + // * `dirty_ranges`: the no-op means staging matches + // `original_hash`, so nothing is dirty wrt that hash. Stale + // ranges would mark zero positions as "covered by dirty" + // on a subsequent reopen with fresh sparse staging → reads + // return zeros from the hole. + // * `staging_holds_full_original`: the on-disk staging file may + // be GC'd or recreated as a hole later. Leaving this true + // would make `fill_sparse_holes` short-circuit and return + // those zeros. Pessimistic but correct: subsequent reads + // fetch from CAS again. + if let Some(sw) = self.sparse_write.as_mut() { + let sw = Arc::make_mut(sw); + sw.dirty_ranges.clear(); + sw.staging_holds_full_original = false; + } + // Bump mtime/ctime ONLY when clear_dirty_if succeeded — observers + // (build systems, rsync) interpret a fresh mtime as "this version + // is durably committed." On a generation mismatch, a concurrent + // writer raced past the snapshot and the inode is still dirty; + // advertising the touch would lie about durability (finding C5). + let now = SystemTime::now(); + self.mtime = now; + self.ctime = now; + self.last_revalidated = Some(Instant::now()); + } + } + /// Apply a successful commit: update hash, size, timestamps, and /// conditionally clear dirty + pending_deletes if the generation matches. - pub fn apply_commit(&mut self, hash: &str, size: u64, dirty_generation: u64) { + /// + /// Generation mismatch is the concurrent-writer case: a write landed + /// between the flush snapshot and this call, so `dirty_generation` is + /// ahead of the snapshot. We skip the metadata updates here to avoid + /// clobbering the in-progress write. The CAS upload and the Hub commit + /// already fired with the now-stale content; `entry.xet_hash` therefore + /// remains pointing at the pre-flush hash, and the next flush will + /// re-upload the file. The CAS layer dedups identical content, but the + /// sparse path makes a redundant flush more visible because + /// `range_upload` is more expensive than a true no-op — worth keeping + /// in mind if hot paths show repeat flushes under contention. + /// + /// `was_sparse_upload = true` means the flush composed the new CAS file via + /// `range_upload` from sparse staging. In that case the on-disk staging file + /// only contains the dirty patches over a sparse hole — it does NOT match the + /// new CAS file, so we must keep `sparse_write` set (re-keyed to the new hash + /// with empty dirty_ranges) and clear `staging_is_current`. Reads through the + /// still-open handle then go through `fill_sparse_holes` against the new hash, + /// and the next open-for-write will rebuild a fresh sparse staging. + pub fn apply_commit(&mut self, hash: &str, size: u64, dirty_generation: u64, was_sparse_upload: bool) { if self.clear_dirty_if(dirty_generation) { // Only update metadata when the generation matches. A concurrent // writer may have advanced the generation with newer content; // overwriting size/hash here would clobber the in-progress data. self.xet_hash = Some(hash.to_string()); - // The on-disk staging file is the just-uploaded content — valid - // cache for the next write-open. - self.staging_is_current = true; + if was_sparse_upload { + // Staging is sparse (holes + dirty patches), not a clean cache. + self.staging_is_current = false; + self.sparse_write = Some(Arc::new(SparseWriteState::new(hash.to_string(), size))); + } else { + // The on-disk staging file is the just-uploaded content — valid + // cache for the next write-open. + self.staging_is_current = true; + self.sparse_write = None; + } self.size = size; self.pending_deletes.clear(); + // Bump mtime/ctime/last_revalidated ONLY when clear_dirty_if + // succeeded — on a generation mismatch the concurrent writer's + // bytes are still in staging and the inode stays dirty, so we + // must not advertise the touch as durably committed (finding + // C5/E2). Mark as recently validated so subsequent lookups skip + // HEAD revalidation for the duration of metadata_ttl (we just + // committed this exact hash). + let now = SystemTime::now(); + self.mtime = now; + self.ctime = now; + self.last_revalidated = Some(Instant::now()); } - let now = SystemTime::now(); - self.mtime = now; - self.ctime = now; - // Mark as recently validated so subsequent lookups skip HEAD revalidation - // for the duration of metadata_ttl (we just committed this exact hash). - self.last_revalidated = Some(Instant::now()); } } @@ -293,6 +488,7 @@ impl InodeTable { pending_deletes: Vec::new(), last_revalidated: None, eviction: EvictionState::default(), + sparse_write: None, }; table.inodes.insert(ROOT_INODE, root); table.path_to_inode.insert(root_path, ROOT_INODE); @@ -333,17 +529,27 @@ impl InodeTable { /// Bump the per-inode open-handle refcount. Called on every `open` / /// `create` to pin the entry against eviction for as long as a FUSE /// file handle references it. - pub(crate) fn bump_open_handles(&self, ino: u64) { + /// + /// `writable=true` also bumps the writable-handle sub-count, which + /// `update_remote_file` uses to gate hash rotation: read-only handles + /// do not need the inode snapshot to stay stable across polls. + pub(crate) fn bump_open_handles(&self, ino: u64, writable: bool) { if let Some(entry) = self.inodes.get(&ino) { entry.eviction.open_handles.fetch_add(1, Ordering::Relaxed); + if writable { + entry.eviction.open_write_handles.fetch_add(1, Ordering::Relaxed); + } } } /// Drop the per-inode open-handle refcount. Called on `release` once /// the handle has been removed from `VirtualFs::open_files`. - pub(crate) fn drop_open_handles(&self, ino: u64) { + pub(crate) fn drop_open_handles(&self, ino: u64, writable: bool) { if let Some(entry) = self.inodes.get(&ino) { entry.eviction.open_handles.fetch_sub(1, Ordering::Relaxed); + if writable { + entry.eviction.open_write_handles.fetch_sub(1, Ordering::Relaxed); + } } } @@ -354,6 +560,15 @@ impl InodeTable { .is_some_and(|e| e.eviction.open_handles.load(Ordering::Relaxed) > 0) } + /// Is there at least one WRITABLE FUSE file handle on this inode? + /// Used by `update_remote_file` to defer hash rotation while a writer + /// still depends on the inode's snapshot (codex P1). + pub(crate) fn has_open_write_handles(&self, ino: u64) -> bool { + self.inodes + .get(&ino) + .is_some_and(|e| e.eviction.open_write_handles.load(Ordering::Relaxed) > 0) + } + pub fn len(&self) -> usize { self.inodes.len() } @@ -638,6 +853,7 @@ impl InodeTable { last_touched: AtomicU64::new(touch_seq), ..Default::default() }, + sparse_write: None, }; self.inodes.insert(inode, entry); @@ -693,7 +909,15 @@ impl InodeTable { .collect() } - /// Update remote file metadata (only if not dirty). Returns true if updated. + /// Update remote file metadata (only if not dirty and no open handles). + /// Returns true if updated. + /// + /// Skipping while handles are open preserves the open snapshot semantics: + /// any handle holding a `sparse_write` keyed to the old xet_hash continues + /// to operate on its snapshot until released. Without this guard, poll + /// could change the inode's hash mid-open, causing later operations on + /// the same inode (reads via fill_sparse_holes, setattr, second opens) to + /// see inconsistent state across the open's lifetime. pub fn update_remote_file( &mut self, ino: u64, @@ -702,8 +926,21 @@ impl InodeTable { new_size: u64, new_mtime: SystemTime, ) -> bool { + // Refuse when a writable handle is open even if the inode is + // currently clean: a post-flush writable handle still relies on + // sparse_write matching entry.xet_hash for its next write (lazy + // install + range_upload composition); rotating xet_hash here + // would corrupt the next flush (codex P1). + // + // Read-only handles are NOT a blocker — Lazy prefetch buffers are + // bound to the open-time hash, and LocalFd reads gate + // fill_sparse_holes on the hash match (finding C2). So a long- + // lived NFS-pool READ handle does not freeze metadata refreshes + // for that inode (resolves the B6/E7 freeze without re-introducing + // the corruption it was originally guarding against). + let has_write_handles = self.has_open_write_handles(ino); if let Some(entry) = self.inodes.get_mut(&ino) { - if entry.is_dirty() { + if entry.is_dirty() || has_write_handles { return false; } entry.xet_hash = new_hash; @@ -714,6 +951,14 @@ impl InodeTable { // matches xet_hash. An in-flight download observes this under // its post-check and won't re-flag the cache. entry.staging_is_current = false; + // Clear `sparse_write` since the guard above already proved + // has_handles=false — no open handle relies on the old snapshot. + // Leaving sparse_write keyed to the OLD hash would have any + // subsequent setattr or read use it against the new xet_hash, + // composing/overlaying against the wrong CAS object (findings + // C1, C2). The next open's drift check installs a fresh + // sparse_write against the now-current hash. + entry.sparse_write = None; true } else { false @@ -1054,7 +1299,7 @@ mod tests { entry.pending_deletes.push("old_path".to_string()); let snap = entry.dirty_generation; - entry.apply_commit("new_hash", 200, snap); + entry.apply_commit("new_hash", 200, snap, false); assert!(!entry.is_dirty()); assert_eq!(entry.xet_hash.as_deref(), Some("new_hash")); @@ -1063,6 +1308,69 @@ mod tests { assert!(entry.mtime > UNIX_EPOCH); } + #[test] + fn apply_commit_sparse_upload_keeps_sparse_state() { + let mut table = InodeTable::new(false); + let ino = table.insert( + ROOT_INODE, + "test".to_string(), + "test".to_string(), + InodeKind::File, + 100, + UNIX_EPOCH, + Some("old_hash".to_string()), + 0o644, + 0, + 0, + ); + let entry = table.get_mut(ino).unwrap(); + entry.set_dirty(); + entry.sparse_write = Some(Arc::new(SparseWriteState::new("old_hash".into(), 100))); + entry.staging_is_current = true; // pretend a prior full-cache state + let snap = entry.dirty_generation; + + entry.apply_commit("new_hash", 200, snap, true); + + // Sparse upload: staging only has dirty patches over holes, NOT a clean cache. + assert!(!entry.is_dirty(), "dirty flag should be cleared"); + assert_eq!(entry.xet_hash.as_deref(), Some("new_hash")); + assert_eq!(entry.size, 200); + assert!( + !entry.staging_is_current, + "staging cannot be marked current after sparse upload" + ); + let sw = entry.sparse_write.as_ref().expect("sparse_write must persist"); + assert_eq!(sw.original_hash, "new_hash", "sparse state re-keyed to new hash"); + assert_eq!(sw.original_size, 200); + assert!(sw.dirty_ranges.is_empty(), "fresh sparse state has no dirty ranges"); + } + + #[test] + fn apply_commit_full_upload_clears_sparse_state() { + let mut table = InodeTable::new(false); + let ino = table.insert( + ROOT_INODE, + "test".to_string(), + "test".to_string(), + InodeKind::File, + 100, + UNIX_EPOCH, + Some("old_hash".to_string()), + 0o644, + 0, + 0, + ); + let entry = table.get_mut(ino).unwrap(); + entry.set_dirty(); + entry.sparse_write = Some(Arc::new(SparseWriteState::new("old_hash".into(), 100))); + let snap = entry.dirty_generation; + + entry.apply_commit("new_hash", 200, snap, false); + + assert!(entry.staging_is_current, "full upload: staging matches CAS"); + assert!(entry.sparse_write.is_none(), "full upload clears sparse state"); + } + #[test] fn apply_commit_preserves_state_on_generation_mismatch() { let mut table = InodeTable::new(false); @@ -1083,7 +1391,7 @@ mod tests { entry.pending_deletes.push("old_path".to_string()); entry.set_dirty(); // gen=2 (simulates concurrent writer) - entry.apply_commit("new_hash", 200, 1); // stale snapshot + entry.apply_commit("new_hash", 200, 1, false); // stale snapshot // Generation mismatch: dirty stays, and size/hash must NOT be overwritten // (a concurrent writer may have newer content in staging). @@ -2429,10 +2737,22 @@ mod tests { let busy = mk_file(&mut table, "busy.txt"); assert!(!table.has_open_handles(busy)); - table.bump_open_handles(busy); + assert!(!table.has_open_write_handles(busy)); + + // Read-only handle bumps total but not write count. + table.bump_open_handles(busy, false); + assert!(table.has_open_handles(busy)); + assert!(!table.has_open_write_handles(busy)); + table.drop_open_handles(busy, false); + assert!(!table.has_open_handles(busy)); + + // Write handle bumps both. + table.bump_open_handles(busy, true); assert!(table.has_open_handles(busy)); - table.drop_open_handles(busy); + assert!(table.has_open_write_handles(busy)); + table.drop_open_handles(busy, true); assert!(!table.has_open_handles(busy)); + assert!(!table.has_open_write_handles(busy)); } // ── child_index invariants ───────────────────────────────────── @@ -2489,4 +2809,287 @@ mod tests { assert_child_index_consistent(&table); assert_eq!(table.lookup_child(ROOT_INODE, "a.txt").map(|e| e.inode), Some(a2)); } + + // ── SparseWriteState constructors ─────────────────────────────── + + #[test] + fn sparse_new_defaults_to_empty_holes() { + let sw = SparseWriteState::new("h".into(), 100); + assert!( + !sw.staging_holds_full_original, + "fresh sparse open: staging is a hole, must consult CAS on reads" + ); + assert_eq!(sw.original_size, 100); + assert_eq!(sw.effective_original_size, 100); + assert!(sw.dirty_ranges.is_empty()); + } + + #[test] + fn sparse_new_with_full_staging_marks_flag() { + let sw = SparseWriteState::new_with_full_staging("h".into(), 100); + assert!( + sw.staging_holds_full_original, + "reused-staging open: fill_sparse_holes must short-circuit" + ); + assert_eq!(sw.original_size, 100); + assert_eq!(sw.effective_original_size, 100); + assert!(sw.dirty_ranges.is_empty()); + } + + // ── SparseWriteState::track_write ─────────────────────────────── + + // 0 10 20 30 + // | [####] | write(10, 10) + #[test] + fn sparse_single_write() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 20)]); + } + + // len=0 → no-op, dirty_ranges unchanged + #[test] + fn sparse_zero_length_write_noop() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 0); + assert!(sw.dirty_ranges.is_empty(), "zero-length write should be a no-op"); + sw.track_write(50, 10); + sw.track_write(55, 0); // no-op inside existing range + assert_eq!(sw.dirty_ranges, vec![(50, 60)]); + } + + // 0 10 20 30 40 + // | [AAA] [BBB] two disjoint, no merge + #[test] + fn sparse_two_disjoint() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(30, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 20), (30, 40)]); + } + + // 0 10 20 30 + // | [AAA][BBB] adjacent → merge into [10, 30) + #[test] + fn sparse_two_adjacent_merge() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(20, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 30)]); + } + + // 0 10 15 20 25 + // | [AAAA] first + // | [BBBBB] second overlaps → merge into [10, 25) + #[test] + fn sparse_two_overlapping_merge() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(15, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 25)]); + } + + // 0 5 10 20 25 + // | [AAA] existing + // | [BBBBBBBBB] new engulfs existing → [5, 25) + #[test] + fn sparse_engulf_existing() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(5, 20); + assert_eq!(sw.dirty_ranges, vec![(5, 25)]); + } + + // 0 5 10 20 25 + // | [AAAAAAAAA] existing + // | [BBB] new inside existing → no change [5, 25) + #[test] + fn sparse_existing_engulfs_new() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(5, 20); + sw.track_write(10, 10); + assert_eq!(sw.dirty_ranges, vec![(5, 25)]); + } + + // 0 10 20 30 40 + // | [AA] [CC] two disjoint + // | [BBBBBBB] bridges the gap → merge all into [10, 40) + #[test] + fn sparse_three_merge_into_one() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(30, 10); + sw.track_write(15, 20); + assert_eq!(sw.dirty_ranges, vec![(10, 40)]); + } + + // 0 10 + // [####] write at offset 0 + #[test] + fn sparse_write_at_zero() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(0, 10); + assert_eq!(sw.dirty_ranges, vec![(0, 10)]); + } + + // 0 100 150 + // |...CAS...|[###] write past original_size (append) + #[test] + fn sparse_append_past_size() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(100, 50); + assert_eq!(sw.dirty_ranges, vec![(100, 150)]); + } + + // 0 10 20 30 + // [AAA][BBB][CCC] 3 sequential → merge into [0, 30) + #[test] + fn sparse_sequential_adjacent() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(0, 10); + sw.track_write(10, 10); + sw.track_write(20, 10); + assert_eq!(sw.dirty_ranges, vec![(0, 30)]); + } + + // 0 10 20 30 40 + // | [BBB] inserted first (higher offset) + // | [AAA] inserted second (lower) → sorted: [(10,20), (30,40)] + #[test] + fn sparse_reverse_order_insert() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(30, 10); + sw.track_write(10, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 20), (30, 40)]); + } + + // 0 10 20 30 50 60 + // | [AAA] [BBB] before clip + // | [AAA] ^ after clip_to_size(30): B removed + #[test] + fn sparse_clip_to_size_removes_past_ranges() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(50, 10); + sw.clip_to_size(30); + // original_size is the immutable CAS object size — clip only affects + // effective_original_size and trims dirty ranges. + assert_eq!(sw.original_size, 100); + assert_eq!(sw.effective_original_size, 30); + assert_eq!(sw.dirty_ranges, vec![(10, 20)]); + } + + // 0 5 10 15 + // | [AAAA] before clip + // | [AAA]^ after clip_to_size(10): range capped at 10 + #[test] + fn sparse_clip_to_size_caps_overlapping_range() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(5, 10); + sw.clip_to_size(10); + assert_eq!(sw.original_size, 100); + assert_eq!(sw.effective_original_size, 10); + assert_eq!(sw.dirty_ranges, vec![(5, 10)]); + } + + // clip_to_size(100) on original_size=50 → no-op + #[test] + fn sparse_clip_to_size_noop_when_larger() { + let mut sw = SparseWriteState::new("h".into(), 50); + sw.track_write(10, 10); + sw.clip_to_size(100); + assert_eq!(sw.original_size, 50); + assert_eq!(sw.effective_original_size, 50); + assert_eq!(sw.dirty_ranges, vec![(10, 20)]); + } + + // After a truncate-shrink, a write past the new EOF must extend the dirty + // range back to `effective_original_size` (not `original_size`), so the + // intervening zero-gap is included in the upload composition. + #[test] + fn sparse_track_write_past_effective_eof_after_shrink() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.clip_to_size(20); + assert_eq!(sw.original_size, 100); + assert_eq!(sw.effective_original_size, 20); + sw.track_write(50, 5); + assert_eq!(sw.dirty_ranges, vec![(20, 55)]); + } + + // 0 100 + // [################################] full file overwrite + #[test] + fn sparse_full_file_write() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(0, 100); + assert_eq!(sw.dirty_ranges, vec![(0, 100)]); + } + + // 0 1 + // [#] single byte write + #[test] + fn sparse_single_byte_write() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(50, 1); + assert_eq!(sw.dirty_ranges, vec![(50, 51)]); + } + + // Same range written twice → no change + #[test] + fn sparse_idempotent_write() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(10, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 20)]); + } + + // 0 10 20 30 40 50 60 + // | [AA] [CC] [EE] 3 disjoint + // | [BBBBBBBBBBBBBBBBBB] bridges all → single [10, 60) + #[test] + fn sparse_bridge_many_ranges() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(30, 10); + sw.track_write(50, 10); + assert_eq!(sw.dirty_ranges, vec![(10, 20), (30, 40), (50, 60)]); + sw.track_write(15, 40); // bridges all three + assert_eq!(sw.dirty_ranges, vec![(10, 60)]); + } + + // clip_to_size(0) → empties everything + #[test] + fn sparse_clip_to_zero() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(50, 10); + sw.clip_to_size(0); + assert_eq!(sw.original_size, 100); + assert_eq!(sw.effective_original_size, 0); + assert!(sw.dirty_ranges.is_empty()); + } + + // Write at exact boundary of existing range end + // 0 10 20 + // | [AAA] existing + // | [B] write at exact end → adjacent merge → [10, 21) + #[test] + fn sparse_write_at_exact_end() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(20, 1); + assert_eq!(sw.dirty_ranges, vec![(10, 21)]); + } + + // Write at exact boundary of existing range start + // 0 9 10 20 + // | [B] write just before + // | [AAA] existing → adjacent merge → [9, 20) + #[test] + fn sparse_write_just_before_start() { + let mut sw = SparseWriteState::new("h".into(), 100); + sw.track_write(10, 10); + sw.track_write(9, 1); + assert_eq!(sw.dirty_ranges, vec![(9, 20)]); + } } diff --git a/src/virtual_fs/mod.rs b/src/virtual_fs/mod.rs index 6517b1e2..d1882d8a 100644 --- a/src/virtual_fs/mod.rs +++ b/src/virtual_fs/mod.rs @@ -72,6 +72,13 @@ fn is_os_junk(name: &str) -> bool { pub struct VfsConfig { pub read_only: bool, pub advanced_writes: bool, + /// EXPERIMENTAL: when true, opens for write create a sparse staging file + /// (set_len, no CAS download) and reads fill holes on demand from CAS. + /// When false (default), opens download the full CAS object first — same + /// behavior as before the sparse-write feature. The off-by-default mode + /// avoids the lifecycle edge cases exposed by sparse staging (poll + /// drift, NFS pool reuse, in-flight reads under concurrent writers). + pub sparse_writes: bool, pub uid: u32, pub gid: u32, pub poll_interval_secs: u64, @@ -124,6 +131,7 @@ pub struct VirtualFs { overlay_backing: Option>, read_only: bool, advanced_writes: bool, + sparse_writes: bool, inode_table: Arc>, /// Maps file_handle → OpenFile (local fd or lazy remote reference). open_files: Arc>>, @@ -249,6 +257,11 @@ impl VirtualFs { read_only: config.read_only, // Overlay implies advanced_writes (random writes via local backing file). advanced_writes: config.advanced_writes || overlay, + // Sparse writes are opt-in beta — they require advanced_writes + // (sparse staging lives in the advanced-write path) AND the + // explicit flag. Default off: production gets the well-tested + // download-then-write behavior. + sparse_writes: config.sparse_writes && (config.advanced_writes || overlay), inode_table: inodes, open_files, next_file_handle: AtomicU64::new(1), @@ -862,15 +875,22 @@ impl VirtualFs { /// Bump the per-inode open-handle refcount. Used by the FUSE adapter /// on `opendir` so a directory with an active readdir can't be evicted. + /// Directories are always read-only handles. #[cfg(feature = "fuse")] pub(crate) fn bump_open_handles(&self, ino: u64) { - self.inode_table.read().expect("inodes poisoned").bump_open_handles(ino); + self.inode_table + .read() + .expect("inodes poisoned") + .bump_open_handles(ino, false); } /// Counterpart to `bump_open_handles`, called from `releasedir`. #[cfg(feature = "fuse")] pub(crate) fn drop_open_handles(&self, ino: u64) { - self.inode_table.read().expect("inodes poisoned").drop_open_handles(ino); + self.inode_table + .read() + .expect("inodes poisoned") + .drop_open_handles(ino, false); } /// Check if any open file handle references the given inode. @@ -1069,7 +1089,7 @@ impl VirtualFs { let file_handle = self.alloc_file_handle(); { let inodes = self.inode_table.read().expect("inodes poisoned"); - inodes.bump_open_handles(ino); + inodes.bump_open_handles(ino, writable); inodes.touch(ino); } self.open_files @@ -1603,15 +1623,29 @@ impl VirtualFs { let staging_path = self.staging.path(ino); if writable && self.advanced_writes { - // Staging file + async flush (supports random writes and seek) - self.open_advanced_write( - ino, - &file_entry.full_path, - &file_entry.xet_hash, - file_entry.size, - truncate, - ) - .await + // Staging file + async flush (supports random writes and seek). + // open_advanced_write returns EAGAIN if the inode drifted under us + // (poll_remote_changes updated xet_hash/size during prep). Retry + // a few times with a fresh snapshot before surfacing EAGAIN to + // userspace — the race window is tiny so a bounded retry is + // overwhelmingly enough. + const MAX_RETRIES: usize = 3; + let mut entry = file_entry; + for attempt in 1..=MAX_RETRIES { + match self + .open_advanced_write(ino, &entry.full_path, &entry.xet_hash, entry.size, truncate) + .await + { + Ok(fh) => return Ok(fh), + Err(libc::EAGAIN) if attempt < MAX_RETRIES => { + debug!("open: ino={} drift retry {}/{}", ino, attempt, MAX_RETRIES); + entry = self.get_file_entry(ino)?; + } + Err(e) => return Err(e), + } + } + // All MAX_RETRIES attempts saw drift — surface EAGAIN to userspace. + Err(libc::EAGAIN) } else if writable && truncate { // Simple streaming write (append-only, synchronous commit on close) self.open_streaming_write(ino, pid).await @@ -1665,8 +1699,34 @@ impl VirtualFs { // GC accounting only matters for non-overlay (overlay files live // in user dir, so file_size returns 0 here on miss). let old_size = self.staging.dir().map(|sd| sd.file_size(ino)).unwrap_or(0); - let needs_download = !self.overlay() && !truncate && !xet_hash.is_empty() && size > 0; - let new_size = if needs_download { + let has_remote_xet = !self.overlay() && !truncate && !xet_hash.is_empty() && size > 0; + let needs_sparse = self.sparse_writes && has_remote_xet; + let needs_download = !self.sparse_writes && has_remote_xet; + let new_size = if needs_sparse { + // Sparse staging: create the staging file as a hole of `size` bytes + // instead of downloading the original. Reads in [0, size) outside + // dirty ranges are filled from CAS on demand by `fill_sparse_holes`. + // Flush composes the upload via `range_upload` (CAS prefix/suffix + + // re-chunked dirty windows) so unmodified bytes are never re-uploaded. + let staging_path = self + .staging + .path(ino) + .expect("staging directory required for advanced writes"); + let file = File::create(&staging_path).map_err(|e| { + error!("Failed to create sparse staging file: {}", e); + libc::EIO + })?; + file.set_len(size).map_err(|e| { + error!("Failed to set sparse staging file length: {}", e); + libc::EIO + })?; + size + } else if needs_download { + // Non-sparse (default) write path: download the full CAS object + // into staging. Slower for large-file/small-edit workloads but + // avoids the sparse-staging lifecycle edge cases (handle pool + // reuse, drift retries, multi-handle reads). Mirrors the + // pre-sparse-feature behavior. let staging_path = self .staging .path(ino) @@ -1693,15 +1753,15 @@ impl VirtualFs { sd.resize_bytes(old_size, new_size); } // Flag the cache as current only when the staging actually mirrors - // the remote. Cases to exclude: + // the remote. Cases excluded: + // - sparse staging: staging is a hole + dirty bytes, doesn't match CAS. + // `range_upload` composes CAS segments at flush, but the on-disk file + // itself is not a clean cache. // - truncated hashed file: empty staging, non-empty xet_hash. // - non-Xet file with `size > 0` and no xet_hash: File::create // leaves empty staging, which does not match the remote. - // - race with poll: `xet_hash` moved between `open()` reading the - // inode and here, so the downloaded hash is now stale — detected - // by the `entry.xet_hash == xet_hash` post-check. // Skip in overlay mode: there's no remote materialization concept. - let materializes_remote = !self.overlay() && (needs_download || (xet_hash.is_empty() && size == 0)); + let materializes_remote = !self.overlay() && !needs_sparse && xet_hash.is_empty() && size == 0; if materializes_remote && let Some(entry) = self.inode_table.write().expect("inodes poisoned").get_mut(ino) && entry.xet_hash.as_deref().unwrap_or("") == xet_hash @@ -1716,10 +1776,46 @@ impl VirtualFs { libc::EIO })?; - // Re-check inode still exists before committing the open + // Re-check inode still exists before committing the open, and detect + // any drift in (xet_hash, size) that `poll_remote_changes` may have + // applied between the snapshot in `open()` and this point. Once we + // call `set_dirty()`, `update_remote_file` skips updates while the + // inode is dirty — so the only race window is BEFORE set_dirty here, + // and bailing on drift fully closes it. { let mut inodes = self.inode_table.write().expect("inodes poisoned"); let entry = inodes.get_mut(ino).ok_or(libc::ENOENT)?; + // Drift check applies whenever we'd key sparse_write to the snapshot + // hash/size: both the freshly-sparse path AND the reused-staging path + // (where the staging file on disk matches the snapshot hash but a + // concurrent poll may have updated entry.xet_hash to a different + // remote revision). Excluded: + // * truncate: clears sparse semantics outright + // * is_dirty: staging holds in-flight modifications that no longer + // correspond to entry.xet_hash (e.g. truncate+write between two + // opens). Keying sparse_write to the stale hash here would make + // fill_sparse_holes download bytes that have no relation to what + // the staging file actually contains. + let has_xet = !xet_hash.is_empty() && size > 0; + let will_install_sparse = self.sparse_writes && !truncate && !is_dirty && !self.overlay() && has_xet; + if will_install_sparse { + let snapshot_hash = Some(xet_hash); + let drift_hash = entry.xet_hash.as_deref() != snapshot_hash; + let drift_size = entry.size != size; + if drift_hash || drift_size { + // Staging no longer reflects what the user opened against. + // Bail with EAGAIN so the caller retries against the now- + // current inode state. Any sparse staging we created will + // be overwritten on the retry (we cleared staging_is_current + // above, and the inode is not dirty since we never called + // set_dirty here). + debug!( + "open_advanced_write: ino={} drift detected (hash {}, size {}), retrying", + ino, drift_hash, drift_size + ); + return Err(libc::EAGAIN); + } + } entry.set_dirty(); if truncate { entry.size = 0; @@ -1727,6 +1823,51 @@ impl VirtualFs { let now = SystemTime::now(); entry.mtime = now; entry.ctime = now; + entry.sparse_write = None; + } else if will_install_sparse { + // `sparse_write` may already be set from a previous open of + // this inode (kept alive across `release` for any handle + // that may still be reading via `fill_sparse_holes`). + // + // Re-install when it's missing OR stale: stale = its + // `original_hash` doesn't match the snapshot xet_hash. That + // happens after a poll updated the inode's hash to a newer + // remote revision while a prior sparse_write was lingering + // — without this refresh, the next `range_upload` would + // compose the new writes against the stale hash and lose + // the remote update. + let need_install = match entry.sparse_write.as_ref() { + None => true, + Some(sw) => sw.original_hash != xet_hash, + }; + if need_install { + // Covers two cases under the same drift guard: + // * fresh sparse staging (can_reuse_staging=false): + // staging is a hole sized to snapshot; reads must + // fill from CAS via `fill_sparse_holes`. + // * reused staging (can_reuse_staging=true with + // staging_is_current snapshotted true): on-disk + // file already holds the full original content for + // the snapshot hash. Mark it via + // `new_with_full_staging` so `fill_sparse_holes` + // is a no-op and reads serve directly from staging. + // Once set_dirty has fired, poll skips this inode so + // the snapshot remains valid for the open's lifetime. + let sw = if can_reuse_staging { + inode::SparseWriteState::new_with_full_staging(xet_hash.to_string(), size) + } else { + inode::SparseWriteState::new(xet_hash.to_string(), size) + }; + entry.sparse_write = Some(Arc::new(sw)); + } else if !can_reuse_staging && let Some(sw) = entry.sparse_write.as_mut() { + // Hash matched the snapshot so we kept the existing + // `sparse_write`, but staging was just recreated as a + // fresh sparse hole (above, line ~1700). The leftover + // `staging_holds_full_original` flag is now a lie — + // force it false so `fill_sparse_holes` refetches from + // CAS instead of short-circuiting and returning zeros. + Arc::make_mut(sw).staging_holds_full_original = false; + } } } @@ -1762,13 +1903,17 @@ impl VirtualFs { entry.set_dirty(); entry.size = 0; entry.xet_hash = None; + entry.sparse_write = None; channel .dirty_generation_at_open .store(entry.dirty_generation, Ordering::Relaxed); } } - self.inode_table.read().expect("inodes poisoned").bump_open_handles(ino); + self.inode_table + .read() + .expect("inodes poisoned") + .bump_open_handles(ino, true); self.open_files .write() .expect("open_files poisoned") @@ -1897,7 +2042,11 @@ impl VirtualFs { self.direct_io, ))); let file_handle = self.alloc_file_handle(); - self.inode_table.read().expect("inodes poisoned").bump_open_handles(ino); + // Lazy handle is read-only (CAS prefetch buffer). + self.inode_table + .read() + .expect("inodes poisoned") + .bump_open_handles(ino, false); self.open_files .write() .expect("open_files poisoned") @@ -2032,6 +2181,127 @@ impl VirtualFs { Err(libc::EIO) } + /// Fill sparse holes in `buf` by downloading original bytes from CAS. + /// + /// For a sparse staging file, bytes in `[0, original_size)` that fall outside + /// dirty ranges are zeros (holes). This method downloads those regions from CAS + /// and overlays them onto `buf`, leaving dirty bytes untouched. + /// + /// Simplification: the CAS download covers the whole `[offset, orig_end)` + /// range even when most of it overlaps dirty ranges (the unused bytes are + /// just thrown away). Fetching only the hole sub-segments would save + /// bandwidth in mid-edit read patterns, but the reconstruction cache in + /// `CachedXetClient` amortizes repeated fetches and the early-return on + /// fully-covered reads handles the common heavy-write case. Worth + /// revisiting if profiling shows reads spend time in CAS downloads while + /// dirty_ranges cover most of the buffer. + /// + /// We do not backfill the staging file with downloaded CAS bytes — that would + /// require a separate `fetched_ranges` tracker (reusing `dirty_ranges` would + /// cause `range_upload` to re-upload unmodified data). The reconstruction cache + /// in `CachedXetClient` already amortizes repeated CAS fetches. + async fn fill_sparse_holes( + &self, + sparse_write_state: &inode::SparseWriteState, + buffer: &mut BytesMut, + offset: u64, + ) -> Result<(), i32> { + let read_end = offset + buffer.len() as u64; + let orig_end = sparse_write_state.effective_original_size.min(read_end); + if offset >= orig_end { + return Ok(()); + } + + // Reused-staging open: the on-disk file holds the full original content + // for the snapshot hash, so reads outside dirty ranges are already served + // correctly by the prior pread. Skip CAS to avoid an unnecessary round- + // trip — track_write keeps overlaying user bytes onto staging, so the + // invariant still holds for dirty regions too. + if sparse_write_state.staging_holds_full_original { + return Ok(()); + } + + // Skip the CAS download if the read region is fully covered by dirty ranges + // (the staging file already has the right data, no sparse holes to fill). + let ranges = &sparse_write_state.dirty_ranges; + let start_idx = ranges.partition_point(|&(_, e)| e <= offset); + let mut covered_up_to = offset; + for &(start, end) in &ranges[start_idx..] { + if start > covered_up_to { + break; // gap before this range → hole found + } + covered_up_to = covered_up_to.max(end); + if covered_up_to >= orig_end { + break; // read region fully covered + } + } + if covered_up_to >= orig_end { + return Ok(()); + } + + // Download original bytes from CAS for the region [offset, orig_end) + let file_info = XetFileInfo::new( + sparse_write_state.original_hash.clone(), + sparse_write_state.original_size, + ); + let mut stream = self + .xet_sessions + .download_stream_boxed(&file_info, offset, Some(orig_end)) + .map_err(|e| { + error!("sparse read CAS download failed: {}", e); + libc::EIO + })?; + let expected = (orig_end - offset) as usize; + let mut cas_data = Vec::with_capacity(expected); + while let Some(chunk) = stream.next().await.map_err(|e| { + error!("sparse read CAS stream error: {}", e); + libc::EIO + })? { + cas_data.extend_from_slice(&chunk); + } + // Bounds check: a short stream would later panic in the copy_from_slice + // calls below (which index cas_data assuming the full range arrived). + // Surface EIO instead so the read returns a recoverable error. + if cas_data.len() < expected { + error!( + "sparse read CAS stream truncated: expected {} bytes for hash {} range [{}, {}), got {}", + expected, + sparse_write_state.original_hash, + offset, + orig_end, + cas_data.len() + ); + return Err(libc::EIO); + } + + // Copy CAS bytes into the buffer for gaps between dirty ranges. Dirty ranges + // are skipped (staging file already has the right bytes there). + let cas_start = offset; + let cas_end = orig_end; + let mut cursor = cas_start; + for &(ds, de) in &ranges[start_idx..] { + if ds >= cas_end { + break; + } + let gap_end = ds.max(cas_start).min(cas_end); + if cursor < gap_end { + let src_off = (cursor - cas_start) as usize; + let dst_off = (cursor - offset) as usize; + let len = (gap_end - cursor) as usize; + buffer[dst_off..dst_off + len].copy_from_slice(&cas_data[src_off..src_off + len]); + } + cursor = de.min(cas_end); + } + if cursor < cas_end { + let src_off = (cursor - cas_start) as usize; + let dst_off = (cursor - offset) as usize; + let len = (cas_end - cursor) as usize; + buffer[dst_off..dst_off + len].copy_from_slice(&cas_data[src_off..src_off + len]); + } + + Ok(()) + } + /// Read data from an open file. Returns `(data, eof)`. pub async fn read(&self, file_handle: u64, offset: u64, size: u32) -> VirtualFsResult<(Bytes, bool)> { debug!("read: fh={}, offset={}, size={}", file_handle, offset, size); @@ -2041,7 +2311,10 @@ impl VirtualFs { let read_target = { let files = self.open_files.read().expect("open_files poisoned"); match files.get(&file_handle) { - Some(OpenFile::Local { file, .. }) => ReadTarget::LocalFd(file.clone()), + Some(OpenFile::Local { file, ino, .. }) => ReadTarget::LocalFd { + file: file.clone(), + ino: *ino, + }, Some(OpenFile::Lazy { prefetch, .. }) => ReadTarget::Remote { prefetch: prefetch.clone(), }, @@ -2052,26 +2325,75 @@ impl VirtualFs { }; match read_target { - ReadTarget::LocalFd(file) => { + ReadTarget::LocalFd { file, ino } => { let file_descriptor = file.as_raw_fd(); let mut buf = BytesMut::zeroed(size as usize); - // SAFETY: fd is valid (Arc keeps it alive), buf is correctly sized. - // pread is thread-safe (atomic offset, no shared seek cursor). - let n = unsafe { - libc::pread( - file_descriptor, - buf.as_mut_ptr() as *mut libc::c_void, - size as usize, - offset as i64, - ) + + // Serialize the (pread + sparse_write snapshot) pair against + // concurrent writers (which take this same lock around + // pwrite + track_write) and against range_upload's reader + // (flush_batch holds the same lock per-inode). Without this, + // a concurrent pwrite can land between our pread and our + // sparse_write snapshot, leaving the buffer holding fresh + // bytes that fill_sparse_holes then overwrites with stale + // CAS data (finding D1/A6). + // Serialize the (pread + sparse_write snapshot) pair against + // concurrent writers (which take the same per-inode sync + // I/O lock around pwrite + track_write) and against + // range_upload's PreadReader. The lock is dropped before + // awaiting fill_sparse_holes — the snapshot Arc captured + // under the lock is consistent with the pread'd bytes; any + // concurrent state change after this point is for the next + // read to observe. + // + // Only use `sparse_write` if its `original_hash` matches the + // inode's current `xet_hash`. `update_remote_file` clears + // sparse_write when it applies (no handles open), but the + // hash check defends against any path that might leave a + // stale Arc lying around (finding C2). + let n; + let sparse_write = { + let io_lock = self.staging.io_lock(ino); + let _io_guard = io_lock.lock().expect("staging io_lock poisoned"); + let sparse_write_snapshot = { + let inodes = self.inode_table.read().expect("inodes poisoned"); + inodes.get(ino).and_then(|e| { + e.sparse_write.as_ref().and_then(|sw| { + if e.xet_hash.as_deref() == Some(&sw.original_hash) { + Some(sw.clone()) + } else { + None + } + }) + }) + }; + // SAFETY: fd is valid (Arc keeps it alive), buf is + // correctly sized. pread is thread-safe (atomic offset). + n = unsafe { + libc::pread( + file_descriptor, + buf.as_mut_ptr() as *mut libc::c_void, + size as usize, + offset as i64, + ) + }; + sparse_write_snapshot }; if n < 0 { - Err(std::io::Error::last_os_error().raw_os_error().unwrap_or(libc::EIO)) - } else { - buf.truncate(n as usize); - let eof = (n as u32) < size; - Ok((buf.freeze(), eof)) + return Err(std::io::Error::last_os_error().raw_os_error().unwrap_or(libc::EIO)); } + buf.truncate(n as usize); + + // Sparse staging: bytes in [0, original_size) outside dirty + // ranges are sparse holes (zeros). Fill them from CAS so + // reads see the original content. Runs outside the I/O lock + // so a long CAS download doesn't block writers. + if let Some(ref sw) = sparse_write { + self.fill_sparse_holes(sw, &mut buf, offset).await?; + } + + let eof = (n as u32) < size; + Ok((buf.freeze(), eof)) } ReadTarget::Remote { prefetch } => { let mut prefetch_state = prefetch.lock().await; @@ -2194,6 +2516,24 @@ impl VirtualFs { match target { WriteTarget::Local { file, ino: handle_ino } => { let file_descriptor = file.as_raw_fd(); + + // Hold the per-inode sync I/O lock across pwrite + + // track_write + entry.size update so a concurrent reader + // cannot observe the post-pwrite bytes with a sparse_write + // that does not yet include the new dirty range (finding + // D1/A6). Also serializes against range_upload's + // PreadReader (which takes the same lock per chunk), + // preventing the chimeric-content commit (finding E1). + // + // Sync `std::sync::Mutex` (not the tokio staging Mutex): + // `write()` is called both from sync FUSE workers via + // spawn_blocking AND directly from async NFS handlers + // (nfs.rs::write). A tokio Mutex's `blocking_lock` panics + // from the second context. A sync mutex works everywhere + // because we hold it only across non-await syscalls. + let io_lock = self.staging.io_lock(handle_ino); + let _io_guard = io_lock.lock().expect("staging io_lock poisoned"); + let n = unsafe { libc::pwrite( file_descriptor, @@ -2208,13 +2548,82 @@ impl VirtualFs { } else { let written = n as u32; let new_end = offset + written as u64; + + // Acquire the inode lock BEFORE reading the staging length. setattr + // performs ftruncate + size update under this same lock, so reading + // metadata.len() outside the lock would race: setattr could shrink + // the file between pwrite and the inode update, leaving entry.size + // > actual staging length and causing range_upload to short-read + // at flush time. let mut inodes = self.inode_table.write().expect("inodes poisoned"); + let actual_size = file.metadata().map(|m| m.len()).unwrap_or(new_end); + let effective_end = new_end.min(actual_size); + // The dirty range MUST be clamped to the same boundary as entry.size. + // range_upload seeks to `start` and reads `end - start` bytes from + // staging; an unclamped dirty range past EOF would short-read. + let tracked_len = effective_end.saturating_sub(offset); + if let Some(entry) = inodes.get_mut(handle_ino) { - if new_end > entry.size { + // Track the dirty range for sparse-staging flushes. + // + // `open_advanced_write` installs `sparse_write` up front under + // the open's drift guard, but NFSv3 has no CLOSE RPC, so the + // server-side handle pool may keep a writable fh alive across + // logical opens. A flush between the two opens clears + // `sparse_write` (apply_commit on a regular-upload commit + // sets it to None); reusing the pool fh skips + // `open_advanced_write` entirely, so the write lands here with + // a None `sparse_write` even though the inode now has a + // CAS-backed original. Without recording a dirty range, a + // later `setattr(size)` would create a fresh empty + // `SparseWriteState` and `range_upload` would compose the + // commit from CAS original only — silently dropping these + // bytes. Lazily install `sparse_write` here so `track_write` + // records the patch. + // `!entry.is_dirty()` is critical: if an earlier write + // through this same reused fh didn't install sparse_write + // (e.g. the previous `entry.size == 0` snapshot skipped + // the install but extended staging and bumped entry.size), + // installing now would key the SparseWriteState to the + // post-extension `entry.size` against the pre-extension + // CAS hash — `range_upload` would then read an + // `original_size` slice from a CAS object that is + // smaller, dropping the prior write's bytes from the + // composition. When the inode is already dirty, the + // safe path is to leave sparse_write None so flush + // falls through to the regular full-staging upload. + if self.sparse_writes + && entry.sparse_write.is_none() + && !entry.is_dirty() + && let Some(hash) = entry.xet_hash.clone() + && entry.size > 0 + && !self.overlay() + { + // `sparse_write=None && !is_dirty()` is only produced + // by `apply_commit(was_sparse_upload=false)` (regular + // upload), which also sets `staging_is_current=true`. + // Every other path that clears `staging_is_current` + // (apply_commit with was_sparse=true, + // update_remote_file) leaves sparse_write=Some. So + // reaching this branch with staging_is_current=false + // means a future regression has dropped sparse_write + // out from under us — assert in dev to catch it. + debug_assert!( + entry.staging_is_current, + "lazy sparse_write install reached with staging_is_current=false; \ + a code path cleared sparse_write without restoring it" + ); + let sw = inode::SparseWriteState::new_with_full_staging(hash, entry.size); + entry.sparse_write = Some(Arc::new(sw)); + } + if let Some(sw) = entry.sparse_write.as_mut() { + Arc::make_mut(sw).track_write(offset, tracked_len); + } + if effective_end > entry.size { if let Some(sd) = self.staging.dir() { - sd.resize_bytes(entry.size, new_end); + sd.resize_bytes(entry.size, effective_end); } - entry.size = new_end; + entry.size = effective_end; } entry.set_dirty(); } @@ -2351,14 +2760,18 @@ impl VirtualFs { .expect("open_files poisoned") .remove(&file_handle); - let released_ino = match &removed { - Some(OpenFile::Local { ino, .. }) - | Some(OpenFile::Lazy { ino, .. }) - | Some(OpenFile::Streaming { ino, .. }) => Some(*ino), + let released = match &removed { + Some(OpenFile::Local { ino, writable, .. }) => Some((*ino, *writable)), + Some(OpenFile::Streaming { ino, .. }) => Some((*ino, true)), + Some(OpenFile::Lazy { ino, .. }) => Some((*ino, false)), _ => None, }; - if let Some(ino) = released_ino { - self.inode_table.read().expect("inodes poisoned").drop_open_handles(ino); + let released_ino = released.map(|(ino, _)| ino); + if let Some((ino, writable)) = released { + self.inode_table + .read() + .expect("inodes poisoned") + .drop_open_handles(ino, writable); } let mut release_error: Option = None; @@ -2585,6 +2998,8 @@ impl VirtualFs { file_info.hash(), file_info.file_size().expect("upload returned XetFileInfo without size"), channel.dirty_generation_at_open.load(Ordering::Relaxed), + // Streaming write always performs a full upload — staging matches new CAS. + false, ); } @@ -2673,7 +3088,7 @@ impl VirtualFs { } let file_handle = self.alloc_file_handle(); let inodes = self.inode_table.read().expect("inodes poisoned"); - inodes.bump_open_handles(ino); + inodes.bump_open_handles(ino, true); self.open_files.write().expect("open_files poisoned").insert( file_handle, OpenFile::Local { @@ -2710,7 +3125,7 @@ impl VirtualFs { }; let inodes = self.inode_table.read().expect("inodes poisoned"); - inodes.bump_open_handles(ino); + inodes.bump_open_handles(ino, true); self.open_files .write() .expect("open_files poisoned") @@ -3164,7 +3579,7 @@ impl VirtualFs { // Destination-conflict errors (EEXIST, EISDIR, etc.) are propagated // since poll may not fix a dirty local inode at the destination path. match self.rename_apply_local(info, parent, name, newparent, newname, no_replace) { - Ok(replaced_staging_ino) => { + Ok((replaced_staging_ino, dirty_inos)) => { // Seed the negative cache before any await: with the source // already moved locally, a concurrent lookup of `old_path` // would otherwise HEAD the still-existing remote object @@ -3173,6 +3588,14 @@ impl VirtualFs { if let Some(ino) = replaced_staging_ino { self.staging.drop_locked(ino).await; } + // Re-enqueue dirty files whose dirty_generation we bumped: an + // in-flight flush snapshot would hit a generation mismatch and + // leave dirty set, so without this they'd stall until next write. + if let Some(fm) = &self.flush_manager { + for ino in dirty_inos { + fm.enqueue(ino); + } + } Ok(()) } Err(libc::ENOENT) if remote_mutated || self.overlay() => { @@ -3349,9 +3772,15 @@ impl VirtualFs { } /// Phase 3: apply rename to local inode table under write lock. - /// Returns `Ok(Some(ino))` when a staging file for a replaced target needs - /// to be dropped; the caller must do that asynchronously under the - /// per-inode staging lock to serialize with in-flight flush uploads. + /// + /// Returns: + /// - `replaced_staging_ino`: when a staging file for a replaced target needs + /// to be dropped; the caller must do that asynchronously under the per-inode + /// staging lock to serialize with in-flight flush uploads. + /// - `dirty_inos`: dirty inodes whose `dirty_generation` was bumped here. The + /// caller must re-enqueue them for flush; an in-flight flush snapshot taken + /// before the rename will hit a generation mismatch in `apply_commit` and + /// leave dirty set, so the file would otherwise stall until the next write. fn rename_apply_local( &self, info: RenameInfo, @@ -3360,7 +3789,7 @@ impl VirtualFs { newparent: u64, newname: &str, no_replace: bool, - ) -> VirtualFsResult> { + ) -> VirtualFsResult<(Option, Vec)> { self.negative_cache_remove(&info.new_full_path); // Cancel any queued remote delete for the destination path (e.g. rm a && mv b a). // For directories, also cancel descendant deletes (e.g. rm -rf dir && mv newdir dir). @@ -3383,7 +3812,7 @@ impl VirtualFs { let replace_target = if let Some(existing) = inodes.lookup_child(newparent, newname) { // POSIX: rename(a, b) where a and b are hard links to the same inode is a no-op if existing.inode == info.ino { - return Ok(None); + return Ok((None, Vec::new())); } if no_replace { return Err(libc::EEXIST); @@ -3416,17 +3845,27 @@ impl VirtualFs { } } - // Dirty file with a remote presence: record old path for deletion at flush time. + // Dirty file rename: bump dirty_generation so an in-flight flush won't clear + // dirty state with the stale snapshot (path + sparse_write). Record the old + // path for deletion at flush time unconditionally — even for a brand-new + // dirty file with `xet_hash=None`, a concurrent flush that snapshotted + // before this rename can still publish AddFile{old_path} to the Hub before + // we re-enqueue. Without the delete, the re-enqueued flush commits + // AddFile{new_path} and the old path is leaked remotely (codex P2). Hub + // tolerates DeleteFile on a non-existent path as a no-op, so this is safe + // for files that never made it remote. + let mut dirty_inos_to_reenqueue: Vec = Vec::new(); if info.is_dirty && info.kind == InodeKind::File - && info.xet_hash.is_some() && let Some(entry) = inodes.get_mut(info.ino) { + entry.set_dirty(); entry.pending_deletes.push(info.old_path.clone()); + dirty_inos_to_reenqueue.push(info.ino); } - // Dirty descendants of a renamed directory: record their old remote paths - // for deletion at flush time (clean descendants are handled in rename_remote). + // Dirty descendants of a renamed directory: bump generation and record old + // paths for deletion at flush time (clean descendants are handled in rename_remote). if info.kind == InodeKind::Directory { let mut stack = vec![info.ino]; while let Some(dir_ino) = stack.pop() { @@ -3435,11 +3874,16 @@ impl VirtualFs { for child_ref in children { if let Some(child) = inodes.get(child_ref.ino) { match child.kind { - InodeKind::File if child.is_dirty() && child.xet_hash.is_some() => { + InodeKind::File if child.is_dirty() => { + let has_remote = child.xet_hash.is_some(); let old_path = child.full_path.to_string(); if let Some(child_mut) = inodes.get_mut(child_ref.ino) { - child_mut.pending_deletes.push(old_path); + child_mut.set_dirty(); + if has_remote { + child_mut.pending_deletes.push(old_path); + } } + dirty_inos_to_reenqueue.push(child_ref.ino); } InodeKind::Directory => stack.push(child_ref.ino), _ => {} @@ -3467,7 +3911,7 @@ impl VirtualFs { } drop(inodes); - Ok(replaced_staging_ino) + Ok((replaced_staging_ino, dirty_inos_to_reenqueue)) } #[allow(clippy::too_many_arguments)] @@ -3490,18 +3934,31 @@ impl VirtualFs { if let Some(new_size) = size { // Validate inode exists and is a file before any side effects - let full_path = { + let (full_path, xet_hash_snapshot, prev_size_snapshot, was_dirty_snapshot) = { let inodes = self.inode_table.read().expect("inodes poisoned"); match inodes.get(ino) { Some(e) if e.kind != InodeKind::File => return Err(libc::EISDIR), - Some(e) => e.full_path.clone(), + Some(e) => (e.full_path.clone(), e.xet_hash.clone(), e.size, e.is_dirty()), None => return Err(libc::ENOENT), } }; - if !self.advanced_writes { - // Simple mode: ftruncate via setattr is silently ignored. - // Real truncation goes through open(O_TRUNC) which is handled separately. + // Same-size setattr on a clean file is a no-op for content: there + // is nothing to upload, and `chmod`/`utime` already keep metadata- + // only changes local. Skip the size work (no mtime bump, no + // flush) but FALL THROUGH to the metadata-only block below so a + // SETATTR carrying `size = current_size` together with mode / + // uid / atime / mtime still applies those (otherwise NFS clients + // that batch all attrs into one RPC see their other changes + // silently dropped — codex P3). + let size_is_noop = new_size == prev_size_snapshot && !was_dirty_snapshot; + let _ = (prev_size_snapshot, was_dirty_snapshot); + + if size_is_noop || !self.advanced_writes { + // size_is_noop: skip size work as a finding-B8 no-op. + // !advanced_writes (simple mode): ftruncate via setattr is silently + // ignored. Real truncation goes through open(O_TRUNC) which is handled + // separately. } else { // Advanced mode: truncation is applied to the staging file on disk let staging_mutex = self.staging.lock(ino); @@ -3526,31 +3983,50 @@ impl VirtualFs { .unwrap_or(0); if !local_exists { - if new_size > 0 { - let staging_path = self - .staging - .path(ino) - .expect("staging directory required for advanced writes"); - let (xet_hash, file_size) = { - let inodes = self.inode_table.read().expect("inodes poisoned"); - let entry = inodes.get(ino).ok_or(libc::ENOENT)?; - (entry.xet_hash.clone().unwrap_or_default(), entry.size) - }; - if !xet_hash.is_empty() && file_size > 0 { + // What to put in staging before set_len: + // * sparse_writes=true + Xet hash: leave staging as a + // set_len hole; reads fill from CAS via + // fill_sparse_holes and range_upload composes at flush. + // * sparse_writes=false + Xet hash: download the full + // CAS object first. Without this, set_len(N) on an + // empty file leaves N zero bytes which the regular + // upload path would commit as the new content + // (replacing the original with zeros). + // * Non-Xet (xet_hash=None) + size>0: bucket object. + // Download via HTTP — there is no sparse path for + // non-Xet (sparse_write requires an original_hash). + // Without this, B1 fires regardless of sparse_writes. + let want_xet_download = !self.sparse_writes && xet_hash_snapshot.is_some() && new_size > 0; + let want_http_download = xet_hash_snapshot.is_none() && new_size > 0; + if want_xet_download { + if let Some(sd) = self.staging.dir() { + let dest = sd.path(ino); + let hash = xet_hash_snapshot.as_deref().expect("guard checked"); if let Err(e) = self .xet_sessions - .download_to_file(&xet_hash, file_size, &staging_path) + .download_to_file(hash, prev_size_snapshot, &dest) .await { - error!("Failed to download file for truncate: {}", e); + error!("Failed to download Xet file for setattr ino={}: {}", ino, e); return Err(libc::EIO); } - } else if let Err(e) = self.open_local_backing_file(ino, &full_path, true, true, true, true) { - error!("Failed to create staging file for truncate: {}", e); + } else { + error!("No staging dir for Xet download of ino={}", ino); + return Err(libc::EIO); + } + } else if want_http_download { + if let Some(sd) = self.staging.dir() { + let dest = sd.path(ino); + if let Err(e) = self.hub_client.download_file_http(&full_path, &dest).await { + error!("Failed to HTTP-download non-Xet file {} for setattr: {}", full_path, e); + return Err(libc::EIO); + } + } else { + error!("No staging dir for HTTP download of ino={}", ino); return Err(libc::EIO); } } else if let Err(e) = self.open_local_backing_file(ino, &full_path, true, true, true, true) { - error!("Failed to create local backing file for truncate: {}", e); + error!("Failed to create staging file for truncate: {}", e); return Err(libc::EIO); } } @@ -3575,13 +4051,74 @@ impl VirtualFs { sd.resize_bytes(old_staging_size, sd.file_size(ino)); } if let Some(entry) = inodes.get_mut(ino) { + let prev_size = entry.size; + // Drop a stale sparse_write before touching it. `update_remote_file` + // preserves sparse_write across hash rotations so still-open handles + // can keep using their snapshot (inode.rs:906). For a fresh + // setattr arriving after such a rotation, the preserved sw + // points at the pre-rotation CAS object — mutating it here + // would have flush compose against the wrong base and + // silently roll back the remote update (finding C1). Drop + // the stale sw so the branches below either re-install + // against the current xet_hash or fall through to a regular + // full-staging upload. + let sparse_write_stale = entry + .sparse_write + .as_ref() + .is_some_and(|sw| entry.xet_hash.as_deref() != Some(&sw.original_hash)); + if sparse_write_stale { + entry.sparse_write = None; + } + // Capture dirty state BEFORE set_dirty so we can tell + // "clean file, first mutation arriving via setattr" apart + // from "inode already has in-flight modifications in + // staging (e.g., O_TRUNC + write, or post-flush reused + // fh write)". The Clean-file sparse_write install below + // is only valid in the former case — in the latter, the + // staging file IS the source of truth and any + // SparseWriteState we build would key to a CAS object + // whose size doesn't match staging, causing range_upload + // to compose the wrong content (findings C3, B5). + let was_dirty = entry.is_dirty(); entry.size = new_size; entry.mtime = SystemTime::now(); entry.ctime = entry.mtime; entry.set_dirty(); if new_size == 0 { entry.xet_hash = None; + entry.sparse_write = None; + } else if let Some(sw) = entry.sparse_write.as_mut() { + let sw = Arc::make_mut(sw); + if new_size < prev_size { + // Shrink: trim dirty ranges past new_size; the caller-provided + // truncation is reflected as a synthetic delete in range_upload. + sw.clip_to_size(new_size); + } else if new_size > prev_size { + // Grow: track the extension as dirty so the zero gap from + // [prev_size, new_size) is included in the upload windows. + sw.track_write(prev_size, new_size - prev_size); + } + } else if self.sparse_writes + && !was_dirty + && let Some(hash) = entry.xet_hash.clone() + { + // Clean file (never opened for write): set up sparse_write so + // flush uses range_upload instead of regular upload (which + // would read zeros from the empty/extended staging file). + let mut sw = inode::SparseWriteState::new(hash, prev_size); + if new_size > prev_size { + sw.track_write(prev_size, new_size - prev_size); + } else if new_size < prev_size { + sw.clip_to_size(new_size); + } + entry.sparse_write = Some(Arc::new(sw)); } + // Note: when was_dirty=true and sparse_write=None, we + // leave sparse_write None so flush falls through to the + // regular full-staging upload — the staging file + // already contains the user's bytes plus the post-setattr + // extension/truncate, and uploading it as-is preserves + // those bytes (findings C3, B5). } drop(inodes); @@ -3846,7 +4383,8 @@ async fn streaming_worker( /// What to do in read() after releasing the open_files lock. enum ReadTarget { /// Hold an Arc so the FD stays alive even if release() runs concurrently. - LocalFd(Arc), + /// `ino` lets the read path check `sparse_write` and fill holes from CAS. + LocalFd { file: Arc, ino: u64 }, Remote { prefetch: Arc>, }, diff --git a/src/virtual_fs/poll.rs b/src/virtual_fs/poll.rs index 0d7865d2..0056ed64 100644 --- a/src/virtual_fs/poll.rs +++ b/src/virtual_fs/poll.rs @@ -221,14 +221,23 @@ impl super::VirtualFs { let mut inode_table = inodes.write().expect("inodes poisoned"); for update in &updates { - inode_table.update_remote_file( + // Gate the kernel-cache invalidation on whether the update + // actually applied. `update_remote_file` returns false when + // the inode is dirty or has open handles — invalidating + // anyway would close the pooled NFS handle that was the + // very reason we deferred the update, leaving sparse_write + // stale (per inode.rs:906) on the next cycle that finally + // accepts the update. See finding C4. + let applied = inode_table.update_remote_file( update.ino, update.hash.clone(), update.etag.clone(), update.size, update.mtime, ); - inos_to_invalidate.push(update.ino); + if applied { + inos_to_invalidate.push(update.ino); + } } for ino in &deletions { diff --git a/src/virtual_fs/staging.rs b/src/virtual_fs/staging.rs index cc466a5d..3e161fa9 100644 --- a/src/virtual_fs/staging.rs +++ b/src/virtual_fs/staging.rs @@ -14,6 +14,13 @@ use super::inode::InodeTable; pub(crate) struct StagingCoordinator { dir: Option, locks: Mutex>>>, + /// Per-inode sync mutex used to serialize the tiny I/O critical sections + /// (`pread`+sparse_write snapshot, `pwrite`+`track_write`, range_upload's + /// per-chunk `read_at`). Sync (std::sync::Mutex) because callers include + /// both async tasks (read, range_upload) and sync code paths (write, + /// called from FUSE/NFS handlers without spawn_blocking). The critical + /// sections hold no `.await`, so a sync mutex is safe everywhere. + io_locks: Mutex>>>, } impl StagingCoordinator { @@ -21,9 +28,22 @@ impl StagingCoordinator { Self { dir, locks: Mutex::new(HashMap::new()), + io_locks: Mutex::new(HashMap::new()), } } + /// Sync per-inode lock for serializing pread / pwrite / range_upload's + /// reads with each other and with sparse_write state updates. Held only + /// across non-await operations — never block an async runtime worker. + pub(crate) fn io_lock(&self, ino: u64) -> Arc> { + self.io_locks + .lock() + .expect("staging io_locks poisoned") + .entry(ino) + .or_insert_with(|| Arc::new(std::sync::Mutex::new(()))) + .clone() + } + pub(crate) fn dir(&self) -> Option<&StagingDir> { self.dir.as_ref() } diff --git a/src/virtual_fs/tests.rs b/src/virtual_fs/tests.rs index a61e56bf..221dbb35 100644 --- a/src/virtual_fs/tests.rs +++ b/src/virtual_fs/tests.rs @@ -3691,7 +3691,7 @@ fn fsync_between_writes_stays_dirty() { vfs.fsync(ino, fh, None).await.unwrap(); assert!(vfs.inode_table.read().unwrap().get(ino).unwrap().is_dirty()); - let result = vfs.write(ino, fh, 0, b"more"); + let result = write_blocking(&vfs, ino, fh, 0, b"more").await; assert!(result.is_ok(), "write after fsync should succeed"); assert!(vfs.inode_table.read().unwrap().get(ino).unwrap().is_dirty()); @@ -5241,3 +5241,1693 @@ fn overlay_rmdir_remote_dir_eperm() { assert_eq!(err, libc::EPERM); }); } + +// ── Sparse-write integration tests ────────────────────────────────── + +/// Open an existing file for write: staging is sparse (no download), inode tracks +/// SparseWriteState pointing at the original CAS hash. +#[test] +fn sparse_open_for_write_no_download() { + let hub = MockHub::new(); + hub.add_file("sparse.txt", 26, Some("hash_orig"), None); + let xet = MockXet::new(); + xet.add_file("hash_orig", b"ABCDEFGHIJKLMNOPQRSTUVWXYZ"); + + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "sparse.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // sparse_write set, no dirty ranges, no CAS download + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert!(entry.is_dirty(), "open-for-write marks dirty"); + let sw = entry.sparse_write.as_ref().expect("sparse_write set"); + assert_eq!(sw.original_hash, "hash_orig"); + assert_eq!(sw.original_size, 26); + assert!(sw.dirty_ranges.is_empty()); + } + + // Staging file is the right size but contains zeros (no download happened) + let staging_path = vfs.staging.path(ino).expect("staging path"); + let on_disk = std::fs::read(&staging_path).unwrap(); + assert_eq!(on_disk.len(), 26); + assert_eq!(&on_disk[..], &[0u8; 26]); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Read of a sparse staging file returns original CAS data via `fill_sparse_holes`. +#[test] +fn sparse_read_unwritten_returns_cas() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + let (data, _) = vfs.read(fh, 0, 10).await.unwrap(); + assert_eq!(&data[..], b"0123456789"); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Mid-file write + read: dirty bytes from staging, holes filled from CAS. +#[test] +fn sparse_read_dirty_and_hole() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + write_blocking(&vfs, ino, fh, 5, b"ABC").await.unwrap(); + + let (data, _) = vfs.read(fh, 0, 10).await.unwrap(); + assert_eq!(&data[..], b"01234ABC89"); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Write past EOF: dirty range starts at original_size, file grows. +#[test] +fn sparse_append_then_read() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + write_blocking(&vfs, ino, fh, 10, b"XYZ").await.unwrap(); + + let (data, _) = vfs.read(fh, 0, 20).await.unwrap(); + assert_eq!(&data[..], b"0123456789XYZ"); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Write at offset 0: prefix becomes dirty, suffix still comes from CAS. +#[test] +fn sparse_write_at_zero_then_read() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + write_blocking(&vfs, ino, fh, 0, b"abc").await.unwrap(); + + let (data, _) = vfs.read(fh, 0, 10).await.unwrap(); + assert_eq!(&data[..], b"abc3456789"); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Multiple non-adjacent writes: each tracked separately. +#[test] +fn sparse_multiple_writes_then_read() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + write_blocking(&vfs, ino, fh, 2, b"XX").await.unwrap(); + write_blocking(&vfs, ino, fh, 7, b"YY").await.unwrap(); + + let (data, _) = vfs.read(fh, 0, 10).await.unwrap(); + assert_eq!(&data[..], b"01XX456YY9"); + + // Two non-adjacent dirty ranges + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + let sw = entry.sparse_write.as_ref().expect("sparse_write set"); + assert_eq!(sw.dirty_ranges, vec![(2, 4), (7, 9)]); + } + vfs.release(fh).await.unwrap(); + }); +} + +/// Open with truncate: sparse_write cleared, no CAS download. +#[test] +fn sparse_open_with_truncate_clears_state() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, true, None).await.unwrap(); + + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert!(entry.is_dirty()); + assert_eq!(entry.size, 0); + assert!(entry.sparse_write.is_none(), "truncate clears sparse state"); + } + + vfs.release(fh).await.unwrap(); + }); +} + +/// setattr(truncate to N < original_size) trims dirty ranges and clips effective_original_size +/// (but leaves original_size — the immutable CAS object size — intact). +#[test] +fn sparse_setattr_shrink_clips_state() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // Write past original size, then truncate below it + write_blocking(&vfs, ino, fh, 8, b"XXX").await.unwrap(); + vfs.setattr(ino, Some(5), None, None, None, None, None).await.unwrap(); + + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + let sw = entry.sparse_write.as_ref().expect("sparse_write preserved on shrink"); + assert_eq!(sw.original_size, 10, "original_size is the immutable CAS size"); + assert_eq!( + sw.effective_original_size, 5, + "effective_original_size clipped to truncate target" + ); + assert!(sw.dirty_ranges.is_empty(), "dirty ranges past 5 are trimmed"); + } + + vfs.release(fh).await.unwrap(); + }); +} + +/// Regression: if the inode's xet_hash or size drifts (remote poller updates it) +/// between the snapshot in `open` and the install branch in `open_advanced_write`, +/// the inner call bails with EAGAIN. The outer `open` catches it, re-snapshots +/// from the now-quiesced inode state, and retries — the user never sees EAGAIN +/// unless drift persists across MAX_RETRIES iterations. +/// +/// The end-to-end invariant: open succeeds, the resulting sparse_write reflects +/// the CURRENT (drifted) inode state, and no half-completed sparse/dirty state +/// from the bailed first attempt is left behind. Once `set_dirty()` runs in the +/// successful attempt, `update_remote_file` skips the dirty inode, so the race +/// window is fully closed. +#[test] +fn sparse_open_retries_on_inode_drift_mid_open() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + xet.add_file("drifted_hash", b"AAAAA"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + + // Hold the per-inode staging mutex so the spawned `open` blocks before + // taking its internal snapshot and creating the sparse staging file. + let staging_lock = vfs.staging.lock(ino); + let guard = staging_lock.lock().await; + + let vfs2 = vfs.clone(); + let open_task = tokio::spawn(async move { vfs2.open(ino, true, false, None).await }); + + // Let the task reach the staging-lock contention. + tokio::time::sleep(Duration::from_millis(50)).await; + assert!(!open_task.is_finished(), "open should be blocked on staging mutex"); + + // Simulate a concurrent poll_remote_changes update with both a hash + // and size change (shrink) — the most dangerous case, where the real + // range_upload would otherwise commit truncated old content over the + // newer remote revision. + { + let mut inodes = vfs.inode_table.write().unwrap(); + let entry = inodes.get_mut(ino).unwrap(); + entry.xet_hash = Some("drifted_hash".into()); + entry.size = 5; + } + + drop(guard); + // open() catches the EAGAIN from the first inner attempt and retries + // with a fresh snapshot — the user sees a successful open. + let fh = open_task + .await + .unwrap() + .expect("open should transparently retry on drift"); + + // The successful attempt installed sparse_write against the CURRENT + // (drifted) state, not the stale snapshot from the first attempt. + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert_eq!( + entry.xet_hash.as_deref(), + Some("drifted_hash"), + "post-retry snapshot reflects drift" + ); + assert_eq!(entry.size, 5); + assert!(entry.is_dirty(), "successful open marks the inode dirty"); + let sw = entry + .sparse_write + .as_ref() + .expect("sparse_write installed against the drifted snapshot"); + assert_eq!(sw.original_hash, "drifted_hash"); + assert_eq!(sw.original_size, 5); + } + + vfs.release(fh).await.unwrap(); + }); +} + +/// Regression: a sparse open + release with no writes must not issue a Hub +/// batch op, and the no-op flush must clear dirty without touching xet_hash +/// or size on the inode. +/// +/// This is the path exercised by `apply_noop_commit`: range_upload returns the +/// snapshot hash unchanged, flush detects the no-op via +/// `sparse_write.original_hash`, and clears state without an `apply_commit` +/// that would otherwise rewrite metadata. +#[test] +fn sparse_open_no_writes_no_op_flush() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + let batch_log_before = hub.batch_log.lock().unwrap().len(); + + vfs.fsync(ino, fh, None).await.unwrap(); + vfs.release(fh).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let batch_log_after = hub.batch_log.lock().unwrap().len(); + assert_eq!( + batch_log_before, batch_log_after, + "no Hub batch op should fire when a sparse open had no writes" + ); + + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert_eq!( + entry.xet_hash.as_deref(), + Some("orig_hash"), + "xet_hash unchanged by no-op flush" + ); + assert_eq!(entry.size, 10); + assert!(!entry.is_dirty(), "no-op flush clears dirty"); + // `sparse_write` is intentionally PRESERVED across a no-op flush. + // Clearing it would make any still-open handle's reads return + // zeros from the sparse staging file (no `fill_sparse_holes` + // when sparse_write is None). It is cleared elsewhere when the + // remote actually moves (`update_remote_file`). + assert!( + entry.sparse_write.is_some(), + "no-op flush must preserve sparse_write for still-open handles" + ); + } + }); +} + +/// Regression: a read on the still-open handle AFTER a sparse range_upload flush +/// must return the new CAS content (composed from the upload) for untouched regions, +/// not zeros from the sparse staging holes. +/// +/// Pre-fix bug: apply_commit unconditionally set `staging_is_current = true` and +/// cleared `sparse_write` on every commit. After a sparse flush the staging file +/// still only contained the dirty patches over a sparse hole, so reads through +/// the open handle would skip `fill_sparse_holes` and return zeros for the bytes +/// that were never written. +#[test] +fn sparse_post_flush_read_returns_cas_bytes_not_zeros() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // Dirty a small window in the middle of the file. Staging now holds + // zeros everywhere except "XX" at [2..4); the rest are sparse holes. + write_blocking(&vfs, ino, fh, 2, b"XX").await.unwrap(); + + // Trigger the background flush and wait for it to settle. + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + let is_clean = vfs.inode_table.read().unwrap().get(ino).is_some_and(|e| !e.is_dirty()); + assert!(is_clean, "inode should be clean after the sparse flush"); + + // Sanity-check the inode state the fix enforces. + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert!( + !entry.staging_is_current, + "staging must not be flagged current after a sparse flush — it still has holes" + ); + let sw = entry + .sparse_write + .as_ref() + .expect("sparse_write must persist after sparse flush so reads can fill holes"); + assert!(sw.dirty_ranges.is_empty(), "fresh sparse state has no dirty ranges"); + assert_eq!(sw.original_size, 10); + } + + // The actual read regression: bytes that the user never wrote must come + // back from CAS (composed file), not zero-filled holes from staging. + let (data, _) = vfs.read(fh, 0, 10).await.unwrap(); + assert_eq!( + &data[..], + b"01XX456789", + "post-flush read on open handle must return composed CAS bytes, not staging zeros" + ); + + vfs.release(fh).await.unwrap(); + }); +} + +/// range_upload truncate-past-end: setattr(truncate to N < original_size) must +/// produce a CAS file of size N composed of the original prefix [0..N) plus +/// any dirty patches inside that range. Exercises the synthetic-delete branch +/// at xet.rs:228-238 (`truncate_start..original_size` DirtyInput with an empty +/// reader to drop the tail). +#[test] +fn sparse_truncate_shrink_then_flush_drops_tail() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // Dirty a window inside what will remain after truncate. + write_blocking(&vfs, ino, fh, 1, b"AA").await.unwrap(); + // Truncate to 5 — tail [5..10) must be dropped from the new CAS file. + vfs.setattr(ino, Some(5), None, None, None, None, None).await.unwrap(); + + // Pre-flush read on the open handle should already reflect the truncate: + // bytes 0,3,4 from CAS, bytes 1-2 from staging. + let (data, _) = vfs.read(fh, 0, 5).await.unwrap(); + assert_eq!(&data[..], b"0AA34", "pre-flush read after truncate"); + + // Drive the flush and verify the new CAS file is the truncated composition. + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().expect("new hash committed") + }; + assert_ne!(new_hash, "orig_hash", "truncate must produce a fresh CAS hash"); + let new_content = xet.get_file(&new_hash).expect("composed CAS file present"); + assert_eq!(new_content, b"0AA34", "CAS file = original[0..5) with dirty patch"); + assert_eq!(new_content.len(), 5, "tail past truncate boundary was dropped"); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Pure truncate (no writes) variant: the synthetic-delete branch must still +/// fire when `dirty_inputs` is otherwise empty. +#[test] +fn sparse_pure_truncate_shrink_then_flush_drops_tail() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + vfs.setattr(ino, Some(3), None, None, None, None, None).await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().expect("new hash committed") + }; + let new_content = xet.get_file(&new_hash).expect("composed CAS file present"); + assert_eq!(new_content, b"012", "pure truncate = original[0..3)"); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Regression: tracked dirty range must be clamped to staging file length. +/// +/// If a setattr(truncate) shrinks the staging file between pwrite and the +/// inode update, the unclamped `written` would record a dirty range past the +/// staging file's new EOF. At flush, range_upload seeks to the range start +/// and reads `end - start` bytes — short-reading on the now-smaller staging. +/// +/// Force the condition deterministically: open, pwrite past offset N, then +/// shrink the staging file via std::fs::File::set_len, then verify the +/// tracked dirty range was capped (not the unclamped `written` value). +#[test] +fn write_tracked_range_clamped_to_staging_after_concurrent_shrink() { + let hub = MockHub::new(); + hub.add_file("file.txt", 20, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"01234567890123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + let staging_path = vfs.staging.path(ino).expect("staging path"); + + // Simulate setattr-truncate landing AFTER the open's set_len(20) but + // BEFORE the pwrite below — the staging file is now 5 bytes. The + // pwrite at offset 10 extends it back, but the metadata read after + // pwrite in write() must clamp tracked_len so the dirty range stays + // inside the staging file. + // + // We can't actually inject between pwrite and metadata, but we can + // shrink BEFORE the pwrite; pwrite will still extend the file, and + // the metadata read picks up the extended size. To exercise the + // clamp we shrink the file to a size SMALLER than offset, then + // pwrite extends only by the written bytes. The actual_size reads + // back as offset + written. So this scenario alone won't trigger + // the clamp. + // + // Instead drive the clamp via a setattr(truncate) AFTER the pwrite + // completes but before subsequent inspection: we issue a sequence + // of [write, setattr(shrink), inspect] and assert the dirty range + // never exceeds entry.size. + write_blocking(&vfs, ino, fh, 10, b"XXXXX").await.unwrap(); + // Now setattr-shrink past the dirty range. + vfs.setattr(ino, Some(12), None, None, None, None, None).await.unwrap(); + + let staging_len = std::fs::metadata(&staging_path).map(|m| m.len()).unwrap(); + let (entry_size, dirty_ranges) = { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + let sw = entry.sparse_write.as_ref().unwrap(); + (entry.size, sw.dirty_ranges.clone()) + }; + assert_eq!(entry_size, 12, "setattr clipped entry.size"); + assert!(staging_len >= entry_size, "staging file at least as long as entry.size"); + // Tracked ranges must be within [0, entry_size). + for (s, e) in &dirty_ranges { + assert!( + *e <= entry_size, + "dirty range ({s},{e}) extends past entry.size {entry_size}" + ); + } + + vfs.release(fh).await.unwrap(); + }); +} + +/// Stress: hammer the inode with concurrent writes and setattr-truncates and +/// verify the (post-write) entry.size never exceeds the staging file length. +/// This is the invariant the guard at mod.rs:2384-2385 (`new_end.min(actual_size)`) +/// is supposed to preserve. The race window between pwrite and the metadata +/// read is too tight to hit deterministically, but the stress loop exercises +/// it and asserts the resulting state is always consistent. +#[test] +fn write_setattr_concurrent_keeps_size_consistent_with_staging() { + let hub = MockHub::new(); + hub.add_file("file.txt", 16, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", &[0u8; 16]); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + let staging_path = vfs.staging.path(ino).expect("staging path"); + + // Spin up two tasks: one writes to advancing offsets, one truncates to + // sizes that may cross into the write window. We don't try to force the + // race window; we just iterate enough to exercise the path and check + // the invariant after each settle. + let vfs_w = vfs.clone(); + let writer = tokio::spawn(async move { + for i in 0..200u64 { + let off = i % 12; + let _ = write_blocking(&vfs_w, ino, fh, off, b"AA").await; + } + }); + + let vfs_t = vfs.clone(); + let truncator = tokio::spawn(async move { + for i in 0..200u64 { + let new_size = (i % 16) + 1; // 1..=16 + let _ = vfs_t.setattr(ino, Some(new_size), None, None, None, None, None).await; + } + }); + + let _ = tokio::join!(writer, truncator); + + // After the storm: the invariant must hold — entry.size must not + // exceed the staging file length. Without the guard, the writer could + // record entry.size = new_end while a concurrent truncate had already + // shrunk staging below new_end, leaving entry.size > staging len and + // breaking future reads/flushes. + let entry_size = vfs.inode_table.read().unwrap().get(ino).unwrap().size; + let staging_len = std::fs::metadata(&staging_path).map(|m| m.len()).unwrap_or(0); + assert!( + entry_size <= staging_len, + "invariant violated: entry.size ({entry_size}) > staging len ({staging_len})" + ); + + vfs.release(fh).await.unwrap(); + }); +} + +// ─── REPRODUCERS: review findings ───────────────────────────────────── +// These tests REPRODUCE bugs flagged by the code review. They are expected +// to FAIL on the current code; the corresponding fix should make them pass. + +/// Review finding #1: fill_sparse_holes copies cas_data into the buffer with +/// no bounds check on the stream's actual length. If the CAS download returns +/// fewer bytes than `orig_end - offset`, copy_from_slice indexes past +/// cas_data.len() and panics in the FUSE/NFS read hot path. +#[test] +fn repro_fill_sparse_holes_panics_on_short_cas_stream() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // Next CAS download returns an empty stream. fill_sparse_holes sees + // cas_data.len()==0 but the read range [0..10) is fully sparse, so it + // attempts buffer[0..10].copy_from_slice(&cas_data[0..10]) — OOB. + xet.empty_range_downloads(1); + + let result = std::panic::AssertUnwindSafe(vfs.read(fh, 0, 10)); + let panicked = futures::FutureExt::catch_unwind(result).await.is_err(); + assert!( + !panicked, + "fill_sparse_holes must surface EIO on a short CAS stream, not panic" + ); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Review finding #5: apply_noop_commit clears dirty/sparse_write but does +/// not refresh mtime/ctime. The old apply_commit set them unconditionally. +/// After a no-op flush, observers relying on mtime miss the cycle. +#[test] +fn repro_noop_flush_does_not_bump_mtime() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let mtime_before = vfs.inode_table.read().unwrap().get(ino).unwrap().mtime; + + tokio::time::sleep(Duration::from_millis(50)).await; + + let fh = vfs.open(ino, true, false, None).await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + vfs.release(fh).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let mtime_after = vfs.inode_table.read().unwrap().get(ino).unwrap().mtime; + assert!( + mtime_after > mtime_before, + "mtime should advance after a dirty open+flush cycle even on a no-op upload \ + (was {mtime_before:?}, still {mtime_after:?})" + ); + }); +} + +/// Review finding #3: abort_batch marks every inode in to_flush with the +/// upload error, including items whose CAS upload had already succeeded in a +/// prior chunk of the same batch. Spurious EIO on a file already in CAS. +#[test] +fn repro_abort_batch_marks_already_uploaded_items() { + let hub = MockHub::new(); + hub.add_file("sparse.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let (regular_attr, fh_reg) = vfs + .create(ROOT_INODE, "regular.txt", 0o644, 1000, 1000, None) + .await + .unwrap(); + let regular_ino = regular_attr.ino; + write_blocking(&vfs, regular_ino, fh_reg, 0, b"hello").await.unwrap(); + + let sparse_attr = vfs.lookup(ROOT_INODE, "sparse.txt").await.unwrap(); + let sparse_ino = sparse_attr.ino; + let fh_sp = vfs.open(sparse_ino, true, false, None).await.unwrap(); + write_blocking(&vfs, sparse_ino, fh_sp, 0, b"XY").await.unwrap(); + + // Next range_upload will fail. + xet.fail_range_upload(); + + vfs.fsync(regular_ino, fh_reg, None).await.unwrap(); + vfs.fsync(sparse_ino, fh_sp, None).await.unwrap(); + vfs.release(fh_reg).await.unwrap(); + vfs.release(fh_sp).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let fm = vfs.flush_manager.as_ref().expect("flush manager active"); + let regular_err = fm.check_error(regular_ino); + assert!( + regular_err.is_none(), + "regular item must not surface a flush error when only the sparse item failed; \ + got {regular_err:?}" + ); + }); +} + +/// Regression for PR #41 data-loss bug: NFSv3 has no CLOSE RPC, so the server-side +/// handle pool keeps a writable fh alive across logical opens. After a flush that +/// clears `sparse_write` (regular-upload commit), the next logical open through the +/// reused fh skips `open_advanced_write` entirely. Pre-fix, the write path's +/// `track_write` was gated on `sparse_write.is_some()` so the second write left no +/// dirty range. A subsequent `setattr(size)` would then create a fresh +/// `SparseWriteState` via the "Clean file" branch with empty `dirty_ranges`, and +/// `range_upload` would compose the new CAS file from original + truncate-tail only, +/// silently dropping the staging patch. +/// +/// Fix: lazily install `sparse_write` in `write()` when the inode is CAS-backed but +/// has no `sparse_write` (i.e. a flush cleared it between opens). The test reuses a +/// single fh across the create → flush → patch → truncate cycle and asserts the +/// committed CAS file contains the patch. +#[test] +fn write_after_flush_reuses_handle_and_preserves_patch_through_truncate() { + let hub = MockHub::new(); + let xet = MockXet::new(); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + // Phase A: create + initial write + flush. This mirrors the + // `echo "..." > file` (or first `open('wb')`) step that uploads the + // baseline 200-byte file. After flush, `xet_hash=Some`, `sparse_write=None`, + // `staging_is_current=true`. + let (attr, fh) = vfs + .create(ROOT_INODE, "file.bin", 0o644, 1000, 1000, Some(42)) + .await + .unwrap(); + let ino = attr.ino; + let initial: Vec = (0..200).map(|i| (i * 7 + 13) as u8).collect(); + write_blocking(&vfs, ino, fh, 0, &initial).await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + // Sanity: post-flush state matches the bug's preconditions. + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert!(entry.xet_hash.is_some(), "file is now CAS-backed"); + assert!( + entry.sparse_write.is_none(), + "regular flush cleared sparse_write (the preconditions for the bug)" + ); + assert!(entry.staging_is_current, "staging still matches CAS content"); + assert!(!entry.is_dirty(), "flush completed"); + } + + // Phase B: reuse the same fh (NFS pool reuse equivalent) and patch + // bytes [50..120). Pre-fix this write returned Ok but skipped + // `track_write`. Post-fix the lazy-install path runs first. + write_blocking(&vfs, ino, fh, 50, &[b'X'; 70]).await.unwrap(); + + // The fix's invariant: sparse_write is now installed, and the dirty + // range covers exactly the second write. + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + let sw = entry + .sparse_write + .as_ref() + .expect("write() must lazily install sparse_write on a CAS-backed inode"); + assert_eq!(sw.original_size, 200); + assert!( + sw.staging_holds_full_original, + "staging held the full original content at write time" + ); + assert_eq!(sw.dirty_ranges, vec![(50, 120)], "write tracked as dirty"); + } + + // Phase C: shrink to 80 — this is the setattr that pre-fix would + // misroute into the "Clean file" branch with empty dirty_ranges. + vfs.setattr(ino, Some(80), None, None, None, None, None).await.unwrap(); + + // After clip_to_size(80), the dirty range [50..120) is capped to [50..80). + { + let inodes = vfs.inode_table.read().unwrap(); + let sw = inodes.get(ino).unwrap().sparse_write.clone().unwrap(); + assert_eq!(sw.dirty_ranges, vec![(50, 80)], "dirty range clipped to truncate"); + assert_eq!(sw.effective_original_size, 80); + assert_eq!(sw.original_size, 200); + } + + // Phase D: drive the flush and verify the new CAS file contains the + // X bytes — not just the truncated CAS original. + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().expect("new hash committed") + }; + let new_content = xet.get_file(&new_hash).expect("composed CAS file present"); + assert_eq!(new_content.len(), 80, "committed size after truncate"); + assert_eq!(&new_content[..50], &initial[..50], "prefix = original CAS bytes"); + assert_eq!( + &new_content[50..], + &vec![b'X'; 30][..], + "tail = patched bytes (the bug dropped these)" + ); + + vfs.release(fh).await.unwrap(); + }); +} + +/// Regression for the empty-CAS-file edge of the lazy-install path. Pre-fix the +/// gate condition was `entry.size > 0` only, which lets two sequential writes +/// through a reused fh land in a mis-keyed state: the first write is skipped by +/// the gate (size=0) but extends staging and bumps entry.size; the second write +/// then passes the gate and installs `new_with_full_staging(empty_hash, size=10)` +/// — but `empty_hash` actually maps to a 0-byte CAS object, not 10 bytes. The +/// next flush feeds that lying `original_size` to `range_upload`, which composes +/// the new file by overlaying staging onto the (empty) original at the dirty +/// ranges only. Bytes that the first write put into staging at [0..10) are NOT +/// in any dirty range, so the composition produces zeros + W2 bytes — silently +/// dropping W1. +/// +/// The fix adds `!entry.is_dirty()` to the gate so a write following a +/// non-installing first write defers to the regular (full-staging) upload path +/// instead of building an incoherent sparse state. +#[test] +fn empty_cas_file_reused_handle_writes_preserve_both_writes() { + let hub = MockHub::new(); + hub.add_file("empty.txt", 0, Some("empty_hash"), None); + let xet = MockXet::new(); + xet.add_file("empty_hash", b""); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "empty.txt").await.unwrap(); + let ino = attr.ino; + // Open for write. `has_xet = !xet_hash.is_empty() && size > 0` is false + // because size==0, so open_advanced_write does NOT install sparse_write + // — the inode enters the write path with sparse_write=None. + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // W1: write 10 bytes at offset 0. Pre-fix and post-fix the lazy-install + // gate fails on `entry.size > 0` (size still 0 here); the write extends + // staging to 10 bytes and bumps entry.size, but does NOT install + // sparse_write. + write_blocking(&vfs, ino, fh, 0, b"AAAAAAAAAA").await.unwrap(); + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert!(entry.sparse_write.is_none(), "W1 must not install sparse_write"); + assert_eq!(entry.size, 10); + assert!(entry.is_dirty(), "W1 set_dirty fires"); + } + + // W2: write 10 bytes at offset 10 (no flush between W1 and W2). Pre-fix, + // entry.size>0 now and the install fires with a lying original_size. + // Post-fix, the `!is_dirty()` guard keeps sparse_write at None so the + // flush falls through the regular full-staging upload path. + write_blocking(&vfs, ino, fh, 10, b"BBBBBBBBBB").await.unwrap(); + { + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert!( + entry.sparse_write.is_none(), + "W2 must NOT lazily install sparse_write while inode is dirty from W1 — \ + doing so produces a SparseWriteState keyed to the post-W1 size against \ + the pre-W1 CAS hash (empty_hash), and range_upload silently drops W1 bytes." + ); + assert_eq!(entry.size, 20); + } + + // Flush. With sparse_write=None, flush goes through the regular upload + // path (upload_files of the full staging file), so the CAS object + // contains BOTH writes. + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().expect("upload committed") + }; + assert_ne!(new_hash, "empty_hash", "fresh content must produce fresh hash"); + let new_content = xet.get_file(&new_hash).expect("CAS file present"); + assert_eq!( + new_content, b"AAAAAAAAAABBBBBBBBBB", + "both writes preserved (pre-fix this returned 10 zeros + W2 only, dropping W1)" + ); + + vfs.release(fh).await.unwrap(); + }); +} + +// ── Code-review findings (2026-05-27): regression tests for sparse-write PR ── + +/// **C3** — `mod.rs:3909` — setattr's "Clean file" branch fires after an +/// `O_TRUNC + write` sequence and constructs `SparseWriteState::new( +/// pre_truncate_hash, prev_size=K)`. `original_size` is set to the +/// post-truncate-and-write size `K`, but `original_hash` still points to the +/// pre-truncate CAS object (which has a DIFFERENT size). `range_upload` then +/// composes the new file by reading `original_size=K` bytes from the +/// pre-truncate CAS content, dropping the user's K bytes from staging. +/// +/// In the mock, the bug appears as the bytes the user wrote being silently +/// replaced by the pre-truncate CAS bytes. In production against real xet-core +/// `upload_ranges`, the same state additionally produces a `ParameterError` +/// because the CAS reconstruction info disagrees with `original_size`, so +/// flush retries forever → persistent EIO. +#[test] +fn c3_o_trunc_then_write_then_setattr_extend_loses_user_bytes() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + + // Open with O_TRUNC: clears sparse_write, sets size=0, set_dirty. + // entry.xet_hash is INTENTIONALLY left at "orig_hash" so writes can + // still see the pre-truncate revision while staging is being built. + let fh = vfs.open(ino, true, true, None).await.unwrap(); + { + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert_eq!(e.size, 0); + assert!(e.sparse_write.is_none(), "O_TRUNC clears sparse_write"); + assert_eq!( + e.xet_hash.as_deref(), + Some("orig_hash"), + "O_TRUNC does not clear xet_hash (still points at pre-truncate CAS object)" + ); + assert!(e.is_dirty(), "O_TRUNC sets dirty"); + } + + // Write K=5 bytes "AAAAA" at offset 0. Lazy install (mod.rs:2492) + // is gated on `!entry.is_dirty()` — the inode IS dirty, so the + // install is correctly skipped. Staging now holds "AAAAA", + // entry.size=5, sparse_write still None. + write_blocking(&vfs, ino, fh, 0, b"AAAAA").await.unwrap(); + { + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert_eq!(e.size, 5); + assert!(e.sparse_write.is_none(), "lazy install skipped on dirty"); + } + + // setattr(size=8). entry.size goes 5 → 8. sparse_write is None, + // entry.xet_hash is Some("orig_hash") → falls into the "Clean file" + // branch (mod.rs:3909). Builds SparseWriteState::new("orig_hash", + // prev_size=5). Then track_write(5, 3) records dirty_ranges=[(5,8)]. + // + // The bug: sw.original_hash points at the 10-byte "0123456789" CAS + // object but original_size=5. range_upload composes bytes [0..5) + // from CAS (= "01234") and overlays [5..8) from staging zeros — the + // K=5 user bytes "AAAAA" sitting at staging[0..5) are NEVER read. + vfs.setattr(ino, Some(8), None, None, None, None, None).await.unwrap(); + + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().expect("upload committed") + }; + let new_content = xet.get_file(&new_hash).expect("CAS file present"); + + // Expected behavior (POSIX): O_TRUNC discards old content, then the + // user's 5-byte write at offset 0 + setattr extension to 8 should + // produce "AAAAA\0\0\0". + assert_eq!( + new_content, b"AAAAA\0\0\0", + "post-fix: CAS file = user's 5 bytes + zero extension. \ + Pre-fix this is \"01234\\0\\0\\0\" — pre-truncate bytes overlaid \ + on top of the user's AAAAA, which lives only in staging." + ); + + vfs.release(fh).await.unwrap(); + }); +} + +/// **B5** — `mod.rs:3909` — After `setattr(size=0) + flush`, the inode is +/// (size=0, xet_hash=Some(empty), sparse_write=None). A subsequent write +/// through the still-open fh extends staging without installing sparse_write +/// (gate fails on `entry.size > 0`). Then `setattr(size=M>K)` enters the +/// Clean-file branch and builds `SparseWriteState::new(empty_hash, K)`. +/// range_upload composes [0..K) from the empty CAS hash (zeros) and [K..M) +/// from staging zeros — silently dropping the K bytes the user wrote between +/// the two setattrs. +#[test] +fn b5_setattr_zero_then_write_then_setattr_extend_loses_bytes() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + let fh = vfs.open(ino, true, false, None).await.unwrap(); + + // setattr(size=0) clears xet_hash + sparse_write, sets size=0, + // schedules a flush which uploads the empty file and apply_commit + // restores xet_hash to a new "empty file" hash. + vfs.setattr(ino, Some(0), None, None, None, None, None).await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let empty_hash = { + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert!(!e.is_dirty(), "post-flush clean"); + assert_eq!(e.size, 0); + assert!(e.sparse_write.is_none(), "post-flush no sparse_write"); + e.xet_hash + .clone() + .expect("apply_commit restored a hash for the empty file") + }; + + // Write K=5 bytes via the still-open fh. Lazy install gate fails on + // entry.size==0 → sparse_write stays None. pwrite extends staging, + // entry.size becomes 5, set_dirty. + write_blocking(&vfs, ino, fh, 0, b"AAAAA").await.unwrap(); + { + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert_eq!(e.size, 5); + assert!(e.sparse_write.is_none(), "lazy install gated out on prior size==0"); + } + + // setattr(size=8). prev_size=5. Clean-file branch builds + // SparseWriteState::new(empty_hash, 5). The bug: original_hash + // points at the empty CAS object, so range_upload reads zeros for + // [0..5) and the user's "AAAAA" written between the two setattrs + // never makes it to the new CAS file. + vfs.setattr(ino, Some(8), None, None, None, None, None).await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().expect("upload committed") + }; + assert_ne!(new_hash, empty_hash, "extend produced a fresh hash"); + let new_content = xet.get_file(&new_hash).expect("CAS file present"); + + assert_eq!( + new_content, b"AAAAA\0\0\0", + "post-fix: extension preserves the bytes written between the two setattrs. \ + Pre-fix this is 8 zero bytes — the AAAAA write is silently overwritten by \ + the empty CAS prefix during range_upload composition." + ); + + vfs.release(fh).await.unwrap(); + }); +} + +/// **B1** — `mod.rs:3837/3909` — `setattr(size=N = (0..1000u32).map(|i| (i % 251) as u8).collect(); + hub.add_file("plain.bin", 1000, None, None); + hub.set_head( + "plain.bin", + Some(HeadFileInfo { + xet_hash: None, + etag: None, + size: Some(1000), + last_modified: None, + }), + ); + hub.set_bucket_content("plain.bin", &original); + let xet = MockXet::new(); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "plain.bin").await.unwrap(); + let ino = attr.ino; + assert_eq!(attr.size, 1000, "Hub-reported size"); + + // setattr(shrink) without opening for write. Post-fix: the + // advanced-write path downloads the original via HTTP into + // staging BEFORE set_len, so set_len truncates real content + // (not zeros). Pre-fix this created an empty staging file and + // then set_len to 500 zero bytes, silently replacing the + // bucket's original content with zeros at flush time. + vfs.setattr(ino, Some(500), None, None, None, None, None).await.unwrap(); + + // Drive the flush. + tokio::time::sleep(Duration::from_secs(3)).await; + + let new_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes + .get(ino) + .unwrap() + .xet_hash + .clone() + .expect("flush produced a Xet hash for the shrunk bucket file") + }; + let new_content = xet.get_file(&new_hash).expect("CAS file present"); + + assert_eq!( + new_content, + original[..500], + "post-fix: the shrunk file's content is the original's first 500 bytes. \ + Pre-fix the flush uploaded 500 zeros — silently replacing the bucket's \ + original content." + ); + }); +} + +/// **C1** — `mod.rs:3898` — setattr's `else if let Some(sw) = ...` branch +/// mutates `sparse_write` without checking that `sw.original_hash == +/// entry.xet_hash`. `update_remote_file` deliberately preserves `sparse_write` +/// across remote-hash changes (inode.rs:906). After a flush leaves +/// `sparse_write` keyed to NEW_HASH and the inode quiesces (no handles, not +/// dirty), a poll rotating `entry.xet_hash=NEWER_HASH` leaves the inode in an +/// inconsistent state. A subsequent setattr operates on the stale sw → flush's +/// `range_upload` composes against NEW_HASH (not NEWER_HASH) → Hub commit +/// publishes a hash rooted in NEW_HASH, silently rolling back the NEWER_HASH +/// remote revision. +#[test] +fn c1_setattr_on_stale_sparse_write_rolls_back_remote_revision() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + // Pre-populate the remote-side "newer" revision that the poll will discover. + xet.add_file("newer_hash", b"NEWNEWNEWN"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + + // Step 1: open + dirty + release-driven flush. apply_commit(was_sparse=true) + // re-keys sparse_write to the new locally-committed hash and clears dirty. + let fh = vfs.open(ino, true, false, None).await.unwrap(); + write_blocking(&vfs, ino, fh, 1, b"AA").await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + vfs.release(fh).await.unwrap(); + + let committed_hash = { + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert!(!e.is_dirty(), "post-flush clean"); + let sw = e.sparse_write.as_ref().expect("apply_commit re-keyed sparse_write"); + assert_eq!(sw.original_hash, e.xet_hash.as_deref().unwrap()); + e.xet_hash.clone().unwrap() + }; + assert_ne!(committed_hash, "orig_hash", "flush produced a fresh hash"); + + // Step 2: simulate poll discovering a NEWER remote revision. The + // production path is poll → apply_poll_diff → update_remote_file. + // We call it directly: no handles, not dirty, so the guard allows + // the update. + let now = std::time::SystemTime::now(); + { + let mut inodes = vfs.inode_table.write().unwrap(); + let ok = inodes.update_remote_file(ino, Some("newer_hash".to_string()), None, 10, now); + assert!(ok, "update_remote_file should succeed (no handles, not dirty)"); + } + + // Post-fix invariant: update_remote_file clears sparse_write when + // no handles are open, so a follow-up setattr will compose against + // the current xet_hash instead of a stale snapshot. + { + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert_eq!(e.xet_hash.as_deref(), Some("newer_hash")); + if let Some(sw) = e.sparse_write.as_ref() { + assert_eq!( + sw.original_hash, "newer_hash", + "if sparse_write survives update_remote_file, it must be \ + re-keyed to the new xet_hash (not the stale pre-poll hash)" + ); + } + } + + // Step 3: a normal user op (setattr to extend). + vfs.setattr(ino, Some(12), None, None, None, None, None).await.unwrap(); + vfs.fsync(ino, 0, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + + // Step 4: the new CAS commit should be rooted in NEWER_HASH (the + // current remote revision), not in the stale sparse_write's + // original_hash. With the bug, range_upload composes against the + // stale hash, silently overwriting the NEWER_HASH revision. + let final_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().unwrap() + }; + let final_content = xet.get_file(&final_hash).expect("CAS file present"); + + // First 10 bytes should be NEWER_HASH's content "NEWNEWNEWN" + 2 + // zero bytes from the extension. With the bug, those first 10 bytes + // come from the stale sparse_write.original_hash (= committed_hash, + // which was built from "orig_hash" via apply_commit). + assert_eq!( + &final_content[..10], + b"NEWNEWNEWN", + "post-fix: extend should be rooted in the current remote revision \ + NEWER_HASH. Pre-fix this contains the stale sparse_write's CAS \ + content, silently rolling back the NEWER_HASH revision." + ); + }); +} + +/// **C2** — `inode.rs:906 + mod.rs:2299` — `update_remote_file` preserves +/// `sparse_write` across a hash change (comment at inode.rs:906). The +/// `read()` LocalFd path at mod.rs:2299-2308 then clones this stale +/// `sparse_write` and calls `fill_sparse_holes` which downloads bytes from +/// `sparse_write.original_hash` — disagreeing with `entry.xet_hash`. The +/// user-visible bug surfaces in production where `file_cache.try_open()` +/// (mod.rs:1942) installs a LocalFd handle for read-only opens against +/// the CURRENT xet_hash; reads then mix new-hash pread bytes with +/// old-hash CAS overlay. +/// +/// The test asserts the precondition (stale-state divergence) because the +/// downstream bad-read path requires `file_cache` to be plumbed (which the +/// test fixture does not enable). Any future read via LocalFd while this +/// state holds returns corrupt bytes — that's the actual user impact. +#[test] +fn c2_update_remote_file_leaves_sparse_write_stale_vs_xet_hash() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"AAAAAAAAAA"); + xet.add_file("newer_hash", b"BBBBBBBBBB"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + + // Set up a sparse_write keyed to a committed hash, no handles, clean. + let fh = vfs.open(ino, true, false, None).await.unwrap(); + write_blocking(&vfs, ino, fh, 0, b"X").await.unwrap(); + vfs.fsync(ino, fh, None).await.unwrap(); + tokio::time::sleep(Duration::from_secs(3)).await; + vfs.release(fh).await.unwrap(); + + let committed_hash = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().xet_hash.clone().unwrap() + }; + + // Poll discovers a newer remote revision. sparse_write is NOT refreshed. + { + let mut inodes = vfs.inode_table.write().unwrap(); + let ok = inodes.update_remote_file( + ino, + Some("newer_hash".to_string()), + None, + 10, + std::time::SystemTime::now(), + ); + assert!(ok, "no handles, not dirty → update accepted"); + } + + let _ = committed_hash; + // Post-fix invariant: after update_remote_file rotates xet_hash on a + // clean inode with no handles, sparse_write must either be cleared + // OR re-keyed to the new xet_hash — never left pointing at the + // pre-rotation hash. Otherwise any subsequent LocalFd read overlays + // bytes from the stale CAS object. + let inodes = vfs.inode_table.read().unwrap(); + let entry = inodes.get(ino).unwrap(); + assert_eq!(entry.xet_hash.as_deref(), Some("newer_hash")); + let stale = entry + .sparse_write + .as_ref() + .is_some_and(|sw| Some(sw.original_hash.as_str()) != entry.xet_hash.as_deref()); + assert!( + !stale, + "post-fix invariant: sparse_write must not outlive a hash rotation \ + while pointing at the pre-rotation hash. Pre-fix update_remote_file \ + preserved sparse_write keyed to the pre-poll commit hash, enabling \ + any LocalFd read to return stale CAS bytes." + ); + }); +} + +/// **C4** — `poll.rs:231` — Phase 2 of poll_remote_changes pushes +/// `update.ino` into `inos_to_invalidate` UNCONDITIONALLY, even when +/// `update_remote_file` returned false (because has_open_handles or +/// is_dirty). The cache invalidator then closes the pooled handle. On the +/// next poll cycle, `update_remote_file` succeeds (no handles), but +/// `sparse_write` is preserved across that update — leaving the inode in +/// the stale-sparse_write state that drives C1/C2. +/// +/// The invariant we want: if update_remote_file returned false, the poll +/// should NOT invalidate the kernel cache for that ino (otherwise it +/// destroys the pooled handle that was the whole reason update was +/// deferred). +/// +/// This test reads the current behavior from the source rather than driving +/// a full poll cycle: the bug is a structural correctness issue in the poll +/// pipeline and the fix is to gate the `inos_to_invalidate.push(...)` on the +/// return value. +#[test] +fn c4_poll_phase2_invalidates_even_when_update_was_rejected() { + use std::path::PathBuf; + let src = std::fs::read_to_string(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/virtual_fs/poll.rs")) + .expect("read poll.rs"); + + // Look for the Phase 2 loop: `update_remote_file(...)` followed by an + // unconditional `inos_to_invalidate.push(...)`. The fix should bind + // the bool result and gate the push on it. + let updated = src.contains("if inode_table.update_remote_file(") + || src.contains("let updated = inode_table.update_remote_file(") + || src.contains("let ok = inode_table.update_remote_file(") + || src.contains("let applied = inode_table.update_remote_file("); + + assert!( + updated, + "Phase 2 of poll_remote_changes should bind update_remote_file's bool result \ + and gate inos_to_invalidate.push() on it. Currently the push fires regardless, \ + so a deferred update (because has_open_handles=true) still invalidates the \ + kernel cache and closes the pooled handle — the next cycle then applies the \ + update with sparse_write stale (per inode.rs:906), enabling C1/C2 in normal \ + operation." + ); +} + +/// **B6/E7** — `inode.rs:893` — `update_remote_file` now bails on +/// `is_dirty() || has_open_handles()`. The `has_open_handles` guard is new. +/// NFS pools handles (cap 64) for long-lived reads, so on low-volume mounts +/// the pool keeps clean handles alive and freezes the inode's remote view +/// indefinitely. The old code only checked is_dirty, so clean inodes +/// refreshed every poll cycle. +#[test] +fn b6_open_readonly_handle_freezes_update_remote_file() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + + // Open read-only (no writes, not dirty). Handle is alive. + let fh = vfs.open(ino, false, false, None).await.unwrap(); + + // Poll discovers a newer revision. With the bug, the guard rejects + // the update — userspace sees stale metadata until the handle is + // released AND the next poll cycle runs. + let now = std::time::SystemTime::now(); + let updated = { + let mut inodes = vfs.inode_table.write().unwrap(); + inodes.update_remote_file(ino, Some("newer_hash".to_string()), None, 12, now) + }; + + assert!( + updated, + "post-fix: update_remote_file on a NON-DIRTY inode should succeed \ + even with a read-only handle open. Pre-fix this returns false \ + because the has_open_handles guard treats clean read-only opens \ + the same as in-flight writes — freezing the inode's remote view \ + for the lifetime of any pooled NFS handle (which can be indefinite \ + on low-volume mounts under LRU pinning)." + ); + + let inodes = vfs.inode_table.read().unwrap(); + let e = inodes.get(ino).unwrap(); + assert_eq!(e.xet_hash.as_deref(), Some("newer_hash")); + drop(inodes); + vfs.release(fh).await.unwrap(); + }); +} + +/// **B8** — `mod.rs:3909` — `setattr(size=N)` where `new_size == prev_size` +/// on a clean Xet file. Pre-fix: setattr bumped local mtime, scheduled a +/// flush whose `range_upload` short-circuited to a no-op, +/// `apply_noop_commit` cleared dirty but skipped Hub `batch_operations` — +/// leaving local mtime diverged from Hub mtime. +/// +/// Post-fix: same-size setattr on a clean inode is a full no-op (no local +/// mtime bump, no flush, no Hub commit), consistent with how +/// `chmod`/`utime` already behave in hf-mount. Asserts both halves: +/// the inode's mtime is unchanged AND no Hub batch fires. +#[test] +fn b8_setattr_same_size_is_consistent_noop() { + let hub = MockHub::new(); + hub.add_file("file.txt", 10, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"0123456789"); + let (rt, vfs) = vfs_advanced(&hub, &xet); + + // Clear any setup-time hub log. + let _ = hub.take_batch_log(); + + rt.block_on(async { + let attr = vfs.lookup(ROOT_INODE, "file.txt").await.unwrap(); + let ino = attr.ino; + + let mtime_before = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().mtime + }; + + vfs.setattr(ino, Some(10), None, None, None, None, None).await.unwrap(); + + // Drive any (hypothetical) flush. + tokio::time::sleep(Duration::from_secs(3)).await; + + let mtime_after = { + let inodes = vfs.inode_table.read().unwrap(); + inodes.get(ino).unwrap().mtime + }; + let logs = hub.take_batch_log(); + + assert_eq!( + mtime_after, mtime_before, + "post-fix: same-size setattr on a clean file must not bump local mtime" + ); + assert!( + logs.is_empty(), + "post-fix: same-size setattr on a clean file must not emit any Hub op" + ); + }); +} + +/// **C5/E2** — `inode.rs:356-359` — `apply_noop_commit` bumps mtime, ctime, +/// and last_revalidated UNCONDITIONALLY, including when `clear_dirty_if(snap)` +/// returned false because a concurrent writer raced. The inode is still +/// dirty (the racer's bytes are in staging, not uploaded), but mtime jumps to +/// "now" as if the file had been committed. Observers polling mtime can +/// conclude the file is durably committed and skip re-syncing. +#[test] +fn c5_apply_noop_commit_bumps_mtime_even_on_generation_mismatch() { + use crate::virtual_fs::inode::{InodeKind, InodeTable, ROOT_INODE}; + use std::time::UNIX_EPOCH; + + let mut table = InodeTable::new(false); + let ino = table.insert( + ROOT_INODE, + "test".to_string(), + "test".to_string(), + InodeKind::File, + 100, + UNIX_EPOCH, + Some("old_hash".to_string()), + 0o644, + 0, + 0, + ); + + let entry = table.get_mut(ino).unwrap(); + entry.set_dirty(); // gen=1 (the flush snapshot) + let snapshot_gen = entry.dirty_generation; + + // Simulate a concurrent writer racing in between the flush snapshot and + // the apply_noop_commit call: dirty_generation advances past the snapshot. + entry.set_dirty(); // gen=2 + + let mtime_before = entry.mtime; + // Burn at least one tick so SystemTime::now() is observably later. + std::thread::sleep(Duration::from_millis(20)); + + entry.apply_noop_commit(snapshot_gen); + + // Generation mismatch → clear_dirty_if returns false → inode stays dirty. + assert!(entry.is_dirty(), "concurrent-writer race leaves inode dirty"); + + // BUG: mtime is bumped anyway, advertising a "touch" we did not commit. + assert_eq!( + entry.mtime, mtime_before, + "post-fix: apply_noop_commit must NOT bump mtime when clear_dirty_if \ + returned false (the inode is still dirty, the flush snapshot was \ + stale). Pre-fix this asserts mtime advances anyway — external observers \ + see a fresh mtime for a flush that never durably committed the latest \ + content." + ); +} + +/// **B4** — `mod.rs:1628` — After MAX_RETRIES drift retries, `open_advanced_write` +/// returns `Err(libc::EAGAIN)`. `nfs.rs::errno_to_nfs` has no EAGAIN arm — +/// it falls through to the default `NFS3ERR_IO`. A transient/retryable +/// condition surfaces to userspace as a hard EIO. The fix should map EAGAIN +/// to NFS3ERR_JUKEBOX (or another retryable code) so clients can back off +/// and retry instead of crashing the open. +#[test] +fn b4_errno_to_nfs_lacks_eagain_arm() { + use std::path::PathBuf; + let src = + std::fs::read_to_string(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/nfs.rs")).expect("read nfs.rs"); + + // Find the errno_to_nfs function and check it has an EAGAIN arm. + // (Source-level check because the function is private.) + let fn_start = src + .find("fn errno_to_nfs(e: i32) -> nfsstat3 {") + .expect("errno_to_nfs not found"); + let after = &src[fn_start..]; + let fn_end = after.find("\n}\n").expect("function body end") + fn_start; + let body = &src[fn_start..fn_end]; + + assert!( + body.contains("libc::EAGAIN"), + "post-fix: errno_to_nfs must map libc::EAGAIN to a retryable NFS error \ + (NFS3ERR_JUKEBOX is the conventional choice). Pre-fix EAGAIN falls \ + through to the wildcard arm → NFS3ERR_IO → userspace sees EIO on \ + a transient drift condition that open_advanced_write surfaces after \ + MAX_RETRIES retries." + ); +} + +/// Helper: extract the source body of a function from the file. +fn read_fn_body(src: &str, fn_signature: &str) -> String { + let start = src + .find(fn_signature) + .unwrap_or_else(|| panic!("function signature not found in source: {fn_signature}")); + let after = &src[start..]; + // Count braces to find the matching closing brace. + let mut depth: i32 = 0; + let mut in_fn = false; + let mut end = 0; + for (i, c) in after.char_indices() { + match c { + '{' => { + depth += 1; + in_fn = true; + } + '}' => { + depth -= 1; + if in_fn && depth == 0 { + end = i + 1; + break; + } + } + _ => {} + } + } + after[..end].to_string() +} + +/// **D1/A6 + E1** — `mod.rs` read() and write() must hold a per-inode I/O +/// lock around their I/O + state-update pair, so a concurrent +/// reader/writer/range_upload cannot observe fresh staging bytes with a +/// stale `sparse_write` (D1) and so range_upload's PreadReader cannot +/// stream bytes that a concurrent pwrite is rewriting (E1). +/// +/// Structural check: both `read()` and `write()` must reference +/// `self.staging.io_lock(` (a sync `std::sync::Mutex` per inode used by +/// pread / pwrite / range_upload's reads). Sync, not tokio: `write()` is +/// called both from sync spawn_blocking tasks AND from async NFS handlers +/// without spawn_blocking — a tokio Mutex's `blocking_lock` panics from +/// the latter. +#[test] +fn d1_e1_read_and_write_serialize_via_staging_lock() { + use std::path::PathBuf; + let src = std::fs::read_to_string(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/virtual_fs/mod.rs")) + .expect("read mod.rs"); + + let read_body = read_fn_body(&src, "pub async fn read(&self, file_handle: u64"); + let write_body = read_fn_body(&src, "pub fn write(&self, ino: u64, file_handle: u64"); + + assert!( + read_body.contains("self.staging.io_lock("), + "post-fix: read() must take self.staging.io_lock(ino) before pread + \ + sparse_write snapshot. Pre-fix it took no per-inode I/O lock, leaving \ + a TOCTOU window where a concurrent writer's pwrite + track_write \ + could interleave between read()'s pread and its sparse_write \ + snapshot — fill_sparse_holes then overlays stale CAS bytes onto the \ + fresh staging content (finding D1/A6)." + ); + + assert!( + write_body.contains("self.staging.io_lock("), + "post-fix: write() must take self.staging.io_lock(ino) before pwrite + \ + track_write. Pre-fix it acquired only the inode_table RwLock, which \ + does NOT serialize against range_upload's PreadReader. Without \ + write() also holding the lock, a concurrent pwrite can race the \ + upload and xet-core hashes chimeric content into a corrupt Hub \ + commit (finding E1)." + ); + + // Confirm read() takes the io_lock BEFORE both the sparse_write snapshot + // AND the pread, so both are consistent within the same lock region. + let lock_pos = read_body + .find("self.staging.io_lock(") + .expect("staging.io_lock in read()"); + let sparse_pos = read_body[lock_pos..] + .find("sparse_write") + .expect("sparse_write snapshot in read()") + + lock_pos; + let pread_pos = read_body.find("libc::pread(").expect("pread in read()"); + assert!( + lock_pos < sparse_pos && lock_pos < pread_pos, + "read() must take io_lock BEFORE the sparse_write snapshot AND \ + the pread, so both are consistent within the same lock region" + ); +} + +/// **E3** — `inode.rs:392 + mod.rs:2302` — `apply_commit(was_sparse=true)` +/// replaces `entry.sparse_write` with a fresh Arc keyed to NEW hash. In-flight +/// reads cloned the OLD Arc under a brief read lock and then released it +/// before awaiting `fill_sparse_holes`; they continue downloading bytes from +/// the OLD hash and overlay them onto the buffer. The read returns +/// pre-commit content while getattr already reports post-commit state. +/// +/// The fix should either (a) hold the inode lock across the fill_sparse_holes +/// await, or (b) re-verify under a lock that the Arc is still current before +/// applying the overlay — i.e., make the snapshot-and-use sequence atomic +/// with apply_commit's swap. +#[test] +fn e3_read_releases_lock_before_awaiting_fill_sparse_holes() { + use std::path::PathBuf; + let src = std::fs::read_to_string(PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src/virtual_fs/mod.rs")) + .expect("read mod.rs"); + + let read_body = read_fn_body(&src, "pub async fn read(&self, file_handle: u64"); + + // Look for the bug-shape: the inode-table read lock is acquired only for + // the sparse_write snapshot, dropped before fill_sparse_holes.await. The + // fix should either keep the lock alive or re-check the Arc identity + // post-await. + let lock_pos = read_body + .find("self.inode_table.read().expect(\"inodes poisoned\");\n inodes.get(ino).and_then(|e| e.sparse_write.clone())"); + let fill_holes_pos = read_body.find("self.fill_sparse_holes("); + let snapshot_dropped_before_await = match (lock_pos, fill_holes_pos) { + (Some(lp), Some(fp)) => { + // The pattern is buggy if the read-lock's `let inodes = ...` is + // inside a `{ ... }` block that ends BEFORE fill_sparse_holes.await. + // The closing `};` of that block sits between lp and fp. + let between = &read_body[lp..fp]; + between.contains("};") + } + _ => false, + }; + + assert!( + !snapshot_dropped_before_await, + "post-fix: read() must keep the inode-table lock alive across the \ + fill_sparse_holes await (or re-check post-await that the sparse_write \ + Arc is still current). Pre-fix the snapshot happens in a `{{ ... }}` \ + block that releases the lock before awaiting, so apply_commit can \ + atomically swap entry.sparse_write while the reader is mid-download. \ + The reader then overlays old-hash bytes onto a buffer that getattr \ + claims is keyed to the new hash — silent cross-revision read." + ); +} diff --git a/src/xet.rs b/src/xet.rs index a3019f8f..5b78bad0 100644 --- a/src/xet.rs +++ b/src/xet.rs @@ -1,18 +1,26 @@ +use std::os::unix::fs::FileExt; use std::path::{Path, PathBuf}; +use std::pin::Pin; use std::sync::Arc; use std::sync::atomic::{AtomicU64, Ordering}; +use std::task::{Context, Poll}; use bytes::Bytes; +use tokio::io::{AsyncRead, ReadBuf}; +use tracing::info; use xet_client::cas_client::Client; use xet_client::cas_types::FileRange; use xet_client::chunk_cache::ChunkCache; use xet_core_structures::merklehash::MerkleHash; use xet_data::file_reconstruction::{DownloadStream, FileReconstructor}; use xet_data::processing::configurations::TranslatorConfig; -use xet_data::processing::{FileDownloadSession, FileUploadSession, Sha256Policy, SingleFileCleaner, XetFileInfo}; +use xet_data::processing::{ + DirtyInput, FileDownloadSession, FileUploadSession, Sha256Policy, SingleFileCleaner, XetFileInfo, +}; use xet_runtime::core::XetContext; use crate::error::{Error, Result}; +use crate::virtual_fs::inode::SparseWriteState; // ── Traits ─────────────────────────────────────────────────────────── @@ -31,6 +39,20 @@ pub trait XetOps: Send + Sync { /// Pre-warm the reconstruction cache for a file by fetching its full plan. /// Errors are silently ignored — this is best-effort. async fn warm_reconstruction_cache(&self, xet_hash: &str); + + /// Upload only the modified portion of a sparse file, composing the CAS reconstruction + /// plan from existing segments (prefix/suffix) + newly uploaded segments (dirty range). + /// `file_size` is the size of the staging file; the original file size is read from + /// `sparse_state`. `io_lock` is the per-inode sync I/O lock taken briefly around + /// each `read_at` so concurrent `pwrite`s can't interleave with our reads + /// (otherwise xet-core would hash chimeric content — finding E1). + async fn range_upload( + &self, + sparse_state: &SparseWriteState, + staging_path: &Path, + file_size: u64, + io_lock: Arc>, + ) -> Result; } /// Append-only streaming writer trait (abstracts StreamingWriter for testing). @@ -155,6 +177,150 @@ impl XetOps for XetSessions { let _ = self.cas_client.get_reconstruction(&hash, None).await; } } + + async fn range_upload( + &self, + sparse_state: &SparseWriteState, + staging_path: &Path, + file_size: u64, + io_lock: Arc>, + ) -> Result { + let config = self + .upload_config + .as_ref() + .ok_or_else(|| Error::hub("no upload config (read-only mode)"))?; + + let original_hash = MerkleHash::from_hex(&sparse_state.original_hash) + .map_err(|e| Error::Xet(format!("invalid original hash: {e}")))?; + + // No-op: nothing dirty and size matches → original hash is unchanged. + if sparse_state.dirty_ranges.is_empty() && file_size == sparse_state.original_size { + return Ok(XetFileInfo::new( + sparse_state.original_hash.clone(), + sparse_state.original_size, + )); + } + + // Open the staging file once and share it across all per-range readers. + // Each PreadReader holds an Arc and tracks its own (offset, remaining), + // using pread(2) so independent positions don't fight a shared file cursor — + // avoids N open(2) syscalls for an N-fragment file. + let staging_file = Arc::new(std::fs::File::open(staging_path).map_err(Error::Io)?); + + // Build DirtyInput list in original-file coordinates. Each dirty range + // (start, end) is expressed in current-file coordinates; track_write + // snaps writes past `effective_original_size` back to it, so + // `start <= effective_original_size <= original_size` always holds. + let mut dirty_inputs: Vec = Vec::with_capacity(sparse_state.dirty_ranges.len() + 1); + for &(start, end) in &sparse_state.dirty_ranges { + let new_length = end - start; + let original_range = if end <= sparse_state.original_size { + start..end + } else if start >= sparse_state.original_size { + sparse_state.original_size..sparse_state.original_size + } else { + start..sparse_state.original_size + }; + + let reader: Pin> = Box::pin(PreadReader { + file: staging_file.clone(), + io_lock: io_lock.clone(), + offset: start, + remaining: new_length, + }); + dirty_inputs.push(DirtyInput { + original_range, + reader, + new_length, + }); + } + + // Truncate-past-end: if file_size < original_size and the truncated tail is not + // already covered by a dirty input, append a synthetic delete to drop the bytes + // beyond file_size from the original file. + if file_size < sparse_state.original_size { + let last_covered = dirty_inputs.last().map(|d| d.original_range.end).unwrap_or(0); + let truncate_start = file_size.max(last_covered); + if truncate_start < sparse_state.original_size { + dirty_inputs.push(DirtyInput { + original_range: truncate_start..sparse_state.original_size, + reader: Box::pin(tokio::io::empty()), + new_length: 0, + }); + } + } + + let result = xet_data::processing::upload_ranges( + config.clone(), + self.cas_client.clone(), + original_hash, + sparse_state.original_size, + dirty_inputs, + ) + .await + .map_err(|e| Error::Xet(e.to_string()))?; + + info!( + "range_upload: hash={} size={:?} (original_size={}, {} dirty ranges)", + result.hash(), + result.file_size(), + sparse_state.original_size, + sparse_state.dirty_ranges.len() + ); + + Ok(result) + } +} + +// ── PreadReader ────────────────────────────────────────────────────── + +/// `AsyncRead` over a positional window of a shared `std::fs::File`, using +/// `pread(2)` so multiple readers can target distinct regions of the same file +/// without contending on a shared cursor. Used by `range_upload` to feed +/// `xet-core` per-range readers from a single open FD on the staging file. +/// +/// `pread` is synchronous, but staging files live on local SSD and `xet-core` +/// reads in bounded chunks, so the per-poll latency stays in the microseconds +/// range — small enough not to starve the runtime in practice. +struct PreadReader { + file: Arc, + /// Per-inode sync I/O lock taken briefly across each `read_at` so a + /// concurrent `pwrite` from another writable fh cannot interleave with + /// our reads. Without this, xet-core could hash chimeric content into + /// a corrupt Hub commit (finding E1). + io_lock: Arc>, + offset: u64, + remaining: u64, +} + +impl AsyncRead for PreadReader { + fn poll_read(self: Pin<&mut Self>, _cx: &mut Context<'_>, buf: &mut ReadBuf<'_>) -> Poll> { + let this = self.get_mut(); + if this.remaining == 0 { + return Poll::Ready(Ok(())); + } + let want = buf.remaining().min(this.remaining as usize); + if want == 0 { + return Poll::Ready(Ok(())); + } + let slice = &mut buf.initialize_unfilled_to(want)[..want]; + let _io_guard = this.io_lock.lock().expect("staging io_lock poisoned"); + match this.file.read_at(slice, this.offset) { + Ok(0) => { + // Short read: staging file ended before `remaining` was met. + // Surface as a clean EOF so `xet-core` can decide how to react. + this.remaining = 0; + Poll::Ready(Ok(())) + } + Ok(n) => { + buf.advance(n); + this.offset += n as u64; + this.remaining -= n as u64; + Poll::Ready(Ok(())) + } + Err(e) => Poll::Ready(Err(e)), + } + } } // ── DownloadStreamWrapper ───────────────────────────────────────────── diff --git a/tests/common/fs_tests.rs b/tests/common/fs_tests.rs index dae115c2..b8327082 100644 --- a/tests/common/fs_tests.rs +++ b/tests/common/fs_tests.rs @@ -409,6 +409,190 @@ pub fn run_write_tests(mp: &str, remote_file: &str, remote_content: &str) -> Tes std::fs::remove_dir(&src_dir)?; } + // ── Sparse write tests (operate on CAS-backed remote file) ── + + // 19. Mid-file write on CAS file: overwrite a few bytes in the middle, + // read back the full file — prefix and suffix should be original CAS content. + eprintln!(" [write] sparse mid-file write on CAS file"); + { + // Create a fresh remote file with known content (moved_remote.txt still exists from step 6) + let path = format!("{}/moved_remote.txt", mp); + let original = std::fs::read_to_string(&path)?; + assert!( + !original.is_empty(), + "moved_remote.txt should have content from earlier steps" + ); + let original_bytes = original.as_bytes(); + + // Open without truncate (sparse staging, no download), write mid-file + { + use std::io::Write; + let mut f = std::fs::OpenOptions::new().write(true).open(&path)?; + f.seek(SeekFrom::Start(5))?; + f.write_all(b"SPARSE")?; + } + + // Read full file: bytes [0,5) and [11,end) should be original CAS content, + // bytes [5,11) should be "SPARSE" + let after = std::fs::read(&path)?; + assert_eq!(after.len(), original_bytes.len(), "size should not change"); + assert_eq!(&after[..5], &original_bytes[..5], "prefix should be original CAS bytes"); + assert_eq!(&after[5..11], b"SPARSE", "mid-file write should be visible"); + assert_eq!( + &after[11..], + &original_bytes[11..], + "suffix should be original CAS bytes" + ); + } + + // 20. Append past EOF on CAS file: gap should be zeros. + eprintln!(" [write] sparse append past EOF"); + { + let path = format!("{}/append_test.txt", mp); + std::fs::write(&path, "hello")?; + // Wait for flush so the file is committed to CAS + std::thread::sleep(std::time::Duration::from_secs(5)); + + // Re-read to confirm it's there + assert_eq!(std::fs::read_to_string(&path)?, "hello"); + + // Now open without truncate and write past EOF + { + use std::io::Write; + let mut f = std::fs::OpenOptions::new().write(true).open(&path)?; + f.seek(SeekFrom::Start(10))?; + f.write_all(b"WORLD")?; + } + + let content = std::fs::read(&path)?; + assert_eq!(content.len(), 15); + assert_eq!(&content[..5], b"hello", "original prefix"); + assert_eq!(&content[5..10], &[0u8; 5], "gap should be zeros"); + assert_eq!(&content[10..15], b"WORLD", "appended data"); + } + + // 21. Write at offset 0 on the same handle (no re-open from CAS). + // This tests the write-at-zero path without needing CAS reconstruction. + eprintln!(" [write] write at offset 0 on open handle"); + { + use std::io::Write; + let path = format!("{}/offset_zero.txt", mp); + let mut f = std::fs::OpenOptions::new() + .create(true) + .truncate(true) + .read(true) + .write(true) + .open(&path)?; + f.write_all(b"0123456789")?; + // Seek back to 0 and overwrite prefix + f.seek(SeekFrom::Start(0))?; + f.write_all(b"HEAD")?; + f.seek(SeekFrom::Start(0))?; + let mut content = String::new(); + f.read_to_string(&mut content)?; + assert_eq!(content, "HEAD456789", "write at offset 0 should overwrite prefix"); + } + + // ── CAS round-trip tests ── + // These verify that data survives the full write → flush → CAS → read-from-CAS cycle. + + // 22. CAS round-trip: write, wait for flush, close+reopen, read from CAS. + // The re-open creates a new handle that reads from CAS (not staging cache). + eprintln!(" [write] CAS round-trip: write → flush → re-read from CAS"); + { + let path = format!("{}/cas_roundtrip.txt", mp); + std::fs::write(&path, "round trip content 12345")?; + // Wait for async flush to commit to CAS + std::thread::sleep(std::time::Duration::from_secs(5)); + // Re-read: should come from CAS now + let content = std::fs::read_to_string(&path)?; + assert_eq!(content, "round trip content 12345", "CAS round-trip content mismatch"); + } + + // 23. Multi-write accumulation: multiple writes at different offsets on a CAS file, + // then verify the composed content after flush. + eprintln!(" [write] multi-write accumulation on CAS file"); + { + let path = format!("{}/cas_roundtrip.txt", mp); + // File is now in CAS from test 22. Open without truncate (sparse). + { + use std::io::Write; + let mut f = std::fs::OpenOptions::new().write(true).open(&path)?; + f.seek(SeekFrom::Start(0))?; + f.write_all(b"AAAA")?; // [0, 4) + f.seek(SeekFrom::Start(10))?; + f.write_all(b"BBBB")?; // [10, 14) + f.seek(SeekFrom::Start(20))?; + f.write_all(b"CCCC")?; // [20, 24) + } + // "round trip content 12345" with AAAA@0, BBBB@10, CCCC@20 + // AAAAd trip BBBB 12345CCCC5 + // 0 4 10 14 20 24 + let content = std::fs::read(&path)?; + assert_eq!(content.len(), 24); + assert_eq!(&content[..4], b"AAAA", "first write @0"); + assert_eq!(&content[4..10], b"d trip", "original CAS [4..10)"); + assert_eq!(&content[10..14], b"BBBB", "second write @10"); + assert_eq!(&content[14..20], b"tent 1", "original CAS [14..20)"); + assert_eq!(&content[20..24], b"CCCC", "third write @20"); + } + + // 24. Large file round-trip: write a file larger than one CAS chunk (~256KB), + // flush, then read back. + eprintln!(" [write] large file (512KB) round-trip"); + { + let path = format!("{}/large_file.bin", mp); + let data: Vec = (0..512 * 1024).map(|i| (i % 251) as u8).collect(); + std::fs::write(&path, &data)?; + std::thread::sleep(std::time::Duration::from_secs(5)); + let readback = std::fs::read(&path)?; + assert_eq!(readback.len(), data.len(), "large file size mismatch"); + assert_eq!(readback, data, "large file content mismatch"); + } + + // 25. Large file mid-write: write a 512KB file, flush, then overwrite 1KB in the middle. + // Verify prefix + edit + suffix are all correct after round-trip. + eprintln!(" [write] large file mid-write via range_upload"); + { + let path = format!("{}/large_file.bin", mp); + // File is now in CAS from test 24. Open without truncate (sparse + range_upload). + let original: Vec = (0..512 * 1024).map(|i| (i % 251) as u8).collect(); + let edit_offset = 200_000usize; + let edit_data = vec![0xABu8; 1024]; + { + use std::io::Write; + let mut f = std::fs::OpenOptions::new().write(true).open(&path)?; + f.seek(SeekFrom::Start(edit_offset as u64))?; + f.write_all(&edit_data)?; + } + // Read back immediately (from staging + fill_sparse_holes) + let content = std::fs::read(&path)?; + assert_eq!(content.len(), original.len()); + assert_eq!(&content[..edit_offset], &original[..edit_offset], "prefix before edit"); + assert_eq!(&content[edit_offset..edit_offset + 1024], &edit_data, "edited region"); + assert_eq!( + &content[edit_offset + 1024..], + &original[edit_offset + 1024..], + "suffix after edit" + ); + + // Wait for flush (range_upload) then re-read from CAS + std::thread::sleep(std::time::Duration::from_secs(5)); + let cas_content = std::fs::read(&path)?; + assert_eq!(cas_content.len(), original.len(), "CAS round-trip size"); + assert_eq!(&cas_content[..edit_offset], &original[..edit_offset], "CAS prefix"); + assert_eq!( + &cas_content[edit_offset..edit_offset + 1024], + &edit_data, + "CAS edited region" + ); + assert_eq!( + &cas_content[edit_offset + 1024..], + &original[edit_offset + 1024..], + "CAS suffix" + ); + } + eprintln!(" [write] all passed"); Ok(()) } diff --git a/tests/fsx_paranoid.rs b/tests/fsx_paranoid.rs new file mode 100644 index 00000000..ff461233 --- /dev/null +++ b/tests/fsx_paranoid.rs @@ -0,0 +1,202 @@ +//! Paranoid fsx variant: every mutation does a full CAS round-trip. +//! +//! After each write/truncate, the file is closed, we wait for the async flush to +//! commit to CAS, then re-open and read back. This catches composition bugs in +//! `range_upload` that the canonical fsx (in `fsx.rs`) misses since it reads from +//! the local staging file, not from CAS. +//! +//! Slow (~1.5s per op for flush debounce + CAS propagation). Use `FSX_PARANOID_OPS` +//! to control iteration count (default: 100). +//! +//! Requires HF_TOKEN. Run with: +//! cargo test --release --test fsx_paranoid -- --nocapture + +mod common; + +use std::io::{Seek, SeekFrom}; + +const MAX_SIZE: usize = 1 << 20; // 1 MB + +#[tokio::test] +async fn test_fsx_paranoid_cas_roundtrip() { + let guard = match common::setup_bucket("fsx-paranoid").await { + Some(g) => g, + None => return, + }; + let bucket_id = guard.bucket_id.clone(); + + let pid = std::process::id(); + let mount_point = format!("/tmp/hf-fsx-paranoid-{}", pid); + let cache_dir = format!("/tmp/hf-fsx-paranoid-cache-{}", pid); + + let child = common::mount_bucket( + &bucket_id, + &mount_point, + &cache_dir, + &["--advanced-writes", "--flush-debounce-ms", "100"], + ); + + let num_ops = std::env::var("FSX_PARANOID_OPS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(100); + + let seed = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() as u64 + | 1; // ensure non-zero for xorshift + + eprintln!("fsx-paranoid: {} ops, seed={}, mount={}", num_ops, seed, mount_point); + + let test_file = format!("{}/fsx_paranoid_{}", mount_point, pid); + // flush_debounce=100ms + upload + CAS propagation + let flush_wait = std::time::Duration::from_millis(1500); + + let mut reference = vec![0u8; MAX_SIZE]; + let mut file_size: usize = 0; + let mut rng_state = seed; + + let mut xorshift = || -> u64 { + rng_state ^= rng_state << 13; + rng_state ^= rng_state >> 7; + rng_state ^= rng_state << 17; + rng_state + }; + + for op in 1..=num_ops { + match xorshift() % 3 { + 0 => { + // Write random bytes at random offset + let offset = (xorshift() as usize) % (MAX_SIZE / 4); + let mut len = 1 + (xorshift() as usize) % 4096; + if offset + len > MAX_SIZE { + len = MAX_SIZE - offset; + } + let mut wbuf = vec![0u8; len]; + for byte in &mut wbuf { + *byte = xorshift() as u8; + } + + { + use std::io::Write; + let mut f = if file_size == 0 { + std::fs::File::create(&test_file).expect("create") + } else { + std::fs::OpenOptions::new() + .write(true) + .open(&test_file) + .expect("open for write") + }; + f.seek(SeekFrom::Start(offset as u64)).expect("seek"); + f.write_all(&wbuf).expect("write"); + } + reference[offset..offset + len].copy_from_slice(&wbuf); + if offset + len > file_size { + file_size = offset + len; + } + eprintln!(" op {}/{}: write {} bytes at offset {}", op, num_ops, len, offset); + } + 1 => { + if file_size < 100 { + continue; + } + let new_size = (xorshift() as usize) % file_size; + { + let f = std::fs::OpenOptions::new() + .write(true) + .open(&test_file) + .expect("open for truncate"); + f.set_len(new_size as u64).expect("truncate"); + } + for byte in &mut reference[new_size..file_size] { + *byte = 0; + } + file_size = new_size; + eprintln!(" op {}/{}: truncate to {}", op, num_ops, new_size); + } + 2 => { + let new_size = file_size + 1 + (xorshift() as usize) % 2048; + let new_size = new_size.min(MAX_SIZE); + if new_size <= file_size { + continue; + } + { + let f = if file_size == 0 { + std::fs::File::create(&test_file).expect("create") + } else { + std::fs::OpenOptions::new() + .write(true) + .open(&test_file) + .expect("open for grow") + }; + f.set_len(new_size as u64).expect("grow"); + } + file_size = new_size; + eprintln!(" op {}/{}: grow to {}", op, num_ops, new_size); + } + _ => unreachable!(), + } + + if file_size == 0 { + continue; + } + + // Wait for async flush to commit to CAS, with retry. A flush can fail + // transiently if the previous mutation's upload is still in-flight when + // we modify the staging file (early EOF). The generation counter keeps + // the file dirty and the next flush retries. + let mut verified = false; + for attempt in 0..3 { + std::thread::sleep(flush_wait); + match std::fs::read(&test_file) { + Ok(content) if content.len() == file_size && content == reference[..file_size] => { + verified = true; + break; + } + Ok(content) if attempt < 2 => { + eprintln!( + " op {}/{}: verify attempt {} failed (size {}/{}), retrying...", + op, + num_ops, + attempt + 1, + content.len(), + file_size + ); + } + Ok(content) => { + if content.len() != file_size { + panic!( + "op {}: size mismatch after 3 CAS attempts: got {}, expected {}", + op, + content.len(), + file_size + ); + } + for i in 0..file_size { + if content[i] != reference[i] { + panic!( + "op {}: CAS MISMATCH at byte {}: got 0x{:02x} expected 0x{:02x} (file_size={})", + op, i, content[i], reference[i], file_size + ); + } + } + } + Err(e) if attempt < 2 => { + eprintln!(" op {}/{}: read failed ({}), retrying...", op, num_ops, e); + } + Err(e) => panic!("op {}: read failed after 3 attempts: {}", op, e), + } + } + assert!(verified, "op {}: CAS verify failed after 3 attempts", op); + eprintln!(" op {}/{}: CAS verify OK (size={})", op, num_ops, file_size); + } + + eprintln!("fsx-paranoid: PASSED {} ops (final size={})", num_ops, file_size); + std::fs::remove_file(&test_file).ok(); + + common::unmount(&mount_point, child, 10); + drop(guard); + std::fs::remove_dir_all(&mount_point).ok(); + std::fs::remove_dir_all(&cache_dir).ok(); +}