fix(vfs): clamp tracked dirty range to staging length + cleanup nits

Code review follow-up addressing 5 nits:

1. write() recorded the dirty range with the unclamped `written` even
   when the post-pwrite metadata read showed the staging file had been
   shrunk by a concurrent setattr(truncate). At flush, range_upload
   would seek to the range start and read end - start bytes, short-
   reading on the smaller staging. Now use `effective_end.saturating_sub
   (offset)` so the tracked range matches what's actually on disk.

2. trim_dirty_ranges now drops ranges where the clipped end equals
   start. track_write doesn't produce zero-length ranges so this is
   defense, but keeping the (s < e) invariant downstream is cheap and
   avoids consumers having to think about it.

3. Comment on fill_sparse_holes: the CAS download grabs the whole
   [offset, orig_end) even when most of it overlaps dirty bytes. Worth
   a follow-up only if profiling shows it.

4. Comment on apply_commit's generation-mismatch path: the CAS upload
   and Hub commit fire even when local metadata can't be updated, and
   the next flush will re-upload. CAS dedup handles duplicate content;
   noting the sparse path makes redundant flushes more visible.

5. Replace the unreachable!() at the end of the retry loop in open()
   with an explicit `Err(libc::EAGAIN)`. Same observable behavior,
   cleaner control flow that the compiler can prove total.

Plus a regression test (write_tracked_range_clamped_to_staging_after_
concurrent_shrink) that pwrites then setattr-shrinks past the dirty
range and asserts the tracked dirty_ranges stay within entry.size.

Author: XciD

src/virtual_fs/inode.rs

@@ -218,14 +218,17 @@ impl SparseWriteState {
         self.dirty_ranges.splice(first..last, [(new_start, new_end)]);
     }
 
-    /// Remove dirty ranges past `new_size` and cap overlapping ones.
+    /// 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);
-            true
+            *s < *e
         });
     }
 
@@ -316,6 +319,17 @@ impl InodeEntry {
     /// Apply a successful commit: update hash, size, timestamps, and
     /// conditionally clear dirty + pending_deletes if the generation matches.
     ///
+    /// 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

src/virtual_fs/mod.rs

@@ -1610,28 +1610,22 @@ impl VirtualFs {
             // userspace — the race window is tiny so a bounded retry is
             // overwhelmingly enough.
             const MAX_RETRIES: usize = 3;
-            let mut last_entry = file_entry;
-            for attempt in 0..MAX_RETRIES {
+            let mut entry = file_entry;
+            for attempt in 1..=MAX_RETRIES {
                 match self
-                    .open_advanced_write(
-                        ino,
-                        &last_entry.full_path,
-                        &last_entry.xet_hash,
-                        last_entry.size,
-                        truncate,
-                    )
+                    .open_advanced_write(ino, &entry.full_path, &entry.xet_hash, entry.size, truncate)
                     .await
                 {
                     Ok(fh) => return Ok(fh),
-                    Err(libc::EAGAIN) if attempt + 1 < MAX_RETRIES => {
-                        debug!("open: ino={} drift retry {}/{}", ino, attempt + 1, MAX_RETRIES);
-                        last_entry = self.get_file_entry(ino)?;
-                        continue;
+                    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),
                 }
             }
-            unreachable!("MAX_RETRIES loop exited without a result")
+            // 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
@@ -2104,6 +2098,15 @@ impl VirtualFs {
     /// 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
@@ -2383,19 +2386,25 @@ impl VirtualFs {
                     // range_upload would hit EOF on the now-smaller file.
                     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 also be clamped to the same boundary.
+                    // range_upload seeks to `offset` and reads `end - offset` bytes
+                    // from staging; if we recorded the unclamped `written` here the
+                    // shrink would leave a dirty range past EOF and the upload
+                    // would short-read at flush time.
+                    let tracked_len = effective_end.saturating_sub(offset);
 
                     let mut inodes = self.inode_table.write().expect("inodes poisoned");
                     if let Some(entry) = inodes.get_mut(handle_ino) {
                         // Track the dirty range for sparse-staging flushes.
                         if let Some(sw) = entry.sparse_write.as_mut() {
-                            Arc::make_mut(sw).track_write(offset, written as u64);
+                            Arc::make_mut(sw).track_write(offset, tracked_len);
                         } else if !entry.is_dirty()
                             && let Some(hash) = entry.xet_hash.clone()
                         {
                             // Clean → dirty transition (e.g. NFS handle upgrade): set
                             // up sparse tracking so flush can use range_upload.
                             let mut sw = inode::SparseWriteState::new(hash, entry.size);
-                            sw.track_write(offset, written as u64);
+                            sw.track_write(offset, tracked_len);
                             entry.sparse_write = Some(Arc::new(sw));
                         }
                         if effective_end > entry.size {

src/virtual_fs/tests.rs

@@ -5776,6 +5776,77 @@ fn write_lazy_installs_sparse_write_on_clean_inode_transition() {
     });
 }
 
+/// 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)`)