From f4ba915e09d5b3fa741f87178046ba3db00dd236 Mon Sep 17 00:00:00 2001 From: Adrien Date: Fri, 22 May 2026 18:17:14 +0200 Subject: [PATCH 1/3] fix(nfs): upgrade read-only handle on WRITE instead of returning STALE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If macOS (or any other NFSv3 client) issues a READ before a WRITE on the same file — which is the common case via stat/`ls` populating the attribute cache — `read()` opens a Lazy/read-only handle and pools it. The subsequent WRITE handler peeks the pool, finds the read-only handle, calls `virtual_fs.write()` which returns EBADF, and `errno_to_nfs` maps that to NFS3ERR_STALE. macOS NFS treats STALE on WRITE as a hard failure and silently discards the pending writes from its buffer — `dd` reports success but the bytes never reach the server. `fsync(2)` on the file later returns ESTALE. Fix: on EBADF, evict the read-only handle and open writable, then retry the write. Mirrors the existing EBADF retry already in `read()`. --- src/nfs.rs | 53 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/src/nfs.rs b/src/nfs.rs index 336b2e35..428554b2 100644 --- a/src/nfs.rs +++ b/src/nfs.rs @@ -274,19 +274,48 @@ impl NFSFileSystem for NFSAdapter { } async fn write(&self, id: fileid3, offset: u64, data: &[u8]) -> Result { - // virtual_fs.write is synchronous pwrite — no yield point where the - // pool could evict, so peek without pinning. - let file_handle = self - .handle_pool - .lock() - .expect("handle_pool poisoned") - .peek(id) - .ok_or(nfsstat3::NFS3ERR_STALE)?; - // NFS always uses advanced_writes (staging files), so write() is a - // synchronous pwrite() — safe to call from async context. - self.virtual_fs - .write(id, file_handle, offset, data) + // Fast path: try the existing pool handle. `virtual_fs.write` is a + // synchronous `pwrite` with no yield point, so peeking without + // pinning is safe — the pool can't evict mid-call. + // + // The pool may hold a *read-only* handle for this inode from a prior + // READ RPC. macOS NFS readily issues READs during stat / `ls` to + // populate its attribute cache, well before any write. Writing to a + // Lazy/read-only handle returns EBADF, which `errno_to_nfs` maps to + // `NFS3ERR_STALE`. macOS treats STALE on WRITE as a hard failure and + // silently drops the pending writes from its buffer — `dd` reports + // success but the bytes never reach the server. Symptom seen in the + // wild: `fsync(2)` on the file returns ESTALE. + // + // Fix: on EBADF, evict the read-only handle and reopen writable + // (which materializes the staging file), then retry the write. + let existing = self.handle_pool.lock().expect("handle_pool poisoned").peek(id); + if let Some(fh) = existing { + match self.virtual_fs.write(id, fh, offset, data) { + Ok(_) => { + self.virtual_fs.schedule_flush(id); + return self + .virtual_fs + .getattr(id) + .map(|a| vfs_attr_to_nfs(&a)) + .map_err(errno_to_nfs); + } + Err(libc::EBADF) => { + // Handle was opened read-only. Upgrade to writable. + self.evict_handle(id, fh).await; + } + Err(e) => return Err(errno_to_nfs(e)), + } + } + + // Slow path: open a writable handle and retry the write. + let fh = self + .virtual_fs + .open(id, true, false, None) + .await .map_err(errno_to_nfs)?; + self.insert_handle(id, fh).await; + self.virtual_fs.write(id, fh, offset, data).map_err(errno_to_nfs)?; // NFS has no close/flush RPC, so schedule a debounced flush after // each write to ensure data eventually gets committed to the Hub. self.virtual_fs.schedule_flush(id); From 9f6943b9345b3622cfa45fd54ba18bc97743701f Mon Sep 17 00:00:00 2001 From: Adrien Date: Fri, 22 May 2026 18:23:29 +0200 Subject: [PATCH 2/3] test(nfs): cover EBADF upgrade path in write handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tests that exercise the bug fix: * `write_after_read_upgrades_handle_instead_of_returning_stale` — main regression test: prior READ pools a Lazy handle, then WRITE must upgrade rather than surface NFS3ERR_STALE. * `second_write_reuses_writable_handle` — fast path remains the fast path after the upgrade (no churn on subsequent writes). * `write_without_prior_read_opens_writable_directly` — slow path works standalone when nothing is pooled. Verified by reverting `nfs.rs::write()` to the pre-fix version: tests 1 and 3 fail with NFS3ERR_STALE (test 2 panics before it can run). --- src/nfs.rs | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/src/nfs.rs b/src/nfs.rs index 428554b2..8c359fb5 100644 --- a/src/nfs.rs +++ b/src/nfs.rs @@ -902,6 +902,174 @@ fn nfstime_to_system_time(t: nfstime3) -> SystemTime { #[cfg(test)] mod tests { use super::*; + use crate::test_mocks::{MockHub, MockXet, TestOpts, make_test_vfs}; + + /// Regression: a WRITE that arrives after a prior READ has populated the + /// pool with a Lazy/read-only handle must NOT surface as `NFS3ERR_STALE`. + /// Pre-fix, `nfs.rs::write()` peeked the read-only handle, called + /// `virtual_fs.write()` which returned EBADF, and `errno_to_nfs` mapped + /// that to STALE — at which point macOS NFS silently discards the write. + /// + /// The fix: on EBADF, evict the read-only handle and re-open writable, + /// then retry. This test exercises that exact sequence. + #[test] + fn write_after_read_upgrades_handle_instead_of_returning_stale() { + let rt = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .unwrap(); + + let hub = MockHub::new(); + hub.add_file("file.txt", 11, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"hello world"); + + let vfs = make_test_vfs( + hub.clone(), + xet.clone(), + TestOpts { + advanced_writes: true, + ..Default::default() + }, + &rt, + ); + + // NFS adapter under test (read-write, like a real bucket mount). + let adapter = NFSAdapter::new(vfs.clone(), false); + + rt.block_on(async { + // Resolve the ino so we don't hard-code it. + let name = nfsstring(b"file.txt".to_vec()); + let ino = adapter.lookup(1, &name).await.expect("lookup"); + + // Step 1: a READ populates the pool with a read-only (Lazy) handle. + let (_buf, _eof) = adapter.read(ino, 0, 11).await.expect("read"); + let pooled_fh_after_read = adapter + .handle_pool + .lock() + .expect("poisoned") + .peek(ino) + .expect("pool entry"); + + // Step 2: the critical operation — WRITE on the same inode. + // Pre-fix: this returned Err(NFS3ERR_STALE). Post-fix: it must + // upgrade the handle and succeed. + let attr = adapter + .write(ino, 6, b"RUST!") + .await + .expect("write must not return STALE"); + + // The new attributes reflect the write. + assert_eq!(attr.size, 11, "file size should be unchanged (in-place edit)"); + + // The pool's handle must have been swapped for a writable one + // (the slow path inserts a fresh handle after the upgrade). + let pooled_fh_after_write = adapter + .handle_pool + .lock() + .expect("poisoned") + .peek(ino) + .expect("pool entry"); + assert_ne!( + pooled_fh_after_read, pooled_fh_after_write, + "pool handle must be different after the upgrade (old Lazy → new writable)" + ); + }); + } + + /// A second WRITE on the same ino reuses the now-writable pool handle + /// (fast path: no further open/release dance). + #[test] + fn second_write_reuses_writable_handle() { + let rt = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .unwrap(); + + let hub = MockHub::new(); + hub.add_file("file.txt", 11, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"hello world"); + + let vfs = make_test_vfs( + hub.clone(), + xet.clone(), + TestOpts { + advanced_writes: true, + ..Default::default() + }, + &rt, + ); + let adapter = NFSAdapter::new(vfs.clone(), false); + + rt.block_on(async { + let name = nfsstring(b"file.txt".to_vec()); + let ino = adapter.lookup(1, &name).await.expect("lookup"); + + // First read + write triggers the upgrade. + adapter.read(ino, 0, 11).await.expect("read"); + adapter.write(ino, 0, b"A").await.expect("first write"); + let fh_after_first_write = adapter + .handle_pool + .lock() + .expect("poisoned") + .peek(ino) + .expect("pool entry"); + + // Second write should take the fast path (no new open). + adapter.write(ino, 1, b"B").await.expect("second write"); + let fh_after_second_write = adapter + .handle_pool + .lock() + .expect("poisoned") + .peek(ino) + .expect("pool entry"); + + assert_eq!( + fh_after_first_write, fh_after_second_write, + "fast path must reuse the writable handle" + ); + }); + } + + /// A WRITE on a file that was never opened goes through the slow path + /// directly (no fast-path EBADF). Verifies the slow path stands alone. + #[test] + fn write_without_prior_read_opens_writable_directly() { + let rt = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build() + .unwrap(); + + let hub = MockHub::new(); + hub.add_file("file.txt", 11, Some("orig_hash"), None); + let xet = MockXet::new(); + xet.add_file("orig_hash", b"hello world"); + + let vfs = make_test_vfs( + hub.clone(), + xet.clone(), + TestOpts { + advanced_writes: true, + ..Default::default() + }, + &rt, + ); + let adapter = NFSAdapter::new(vfs.clone(), false); + + rt.block_on(async { + let name = nfsstring(b"file.txt".to_vec()); + let ino = adapter.lookup(1, &name).await.expect("lookup"); + + // Pool is empty for this ino; write goes straight to slow path. + assert!(adapter.handle_pool.lock().unwrap().peek(ino).is_none()); + adapter.write(ino, 0, b"X").await.expect("write"); + assert!( + adapter.handle_pool.lock().unwrap().peek(ino).is_some(), + "slow path must have inserted a writable handle" + ); + }); + } #[test] fn handle_pool_basic() { From 25d22e60b292115cbb0014c9cdea352e9bba0fc7 Mon Sep 17 00:00:00 2001 From: Adrien Date: Fri, 22 May 2026 19:51:18 +0200 Subject: [PATCH 3/3] fix(nfs): write through fresh fh before publishing to pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review pointed out a concrete race in the slow path of the EBADF upgrade: two concurrent NFS WRITE RPCs on the same ino can both peek a read-only handle, both get EBADF, and both fall through to the slow path. They open distinct writable handles fh_A and fh_B (serialized by virtual_fs's per-ino staging lock). Writer A inserts fh_A into the pool; writer B's `insert_handle` then releases fh_A as `replaced`. Writer A's subsequent pwrite hits EBADF on the now-released fh_A and maps back to NFS3ERR_STALE — the very silent-data-loss this code path exists to prevent. Reorder the slow path so the fresh fh is used PRIVATELY for the write, then published to the pool only on success. Nothing else can see the fh until its pwrite has committed to the staging file, so no other task can release it out from under us. Removes the race without introducing a per-inode mutex (the simpler-than-locking alternative suggested by Codex). Also: in the fast-path EBADF branch, remove the stale pool entry before re-entering, mirroring the analogous EBADF retry in `read()`. Pre-fix the entry survived `evict_handle` (which only releases the VFS handle), so a concurrent caller could peek and use a freshly-released fh. Guarded by a `peek == Some(fh)` check to avoid removing a different fh installed by a concurrent successful upgrader. --- src/nfs.rs | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/src/nfs.rs b/src/nfs.rs index 8c359fb5..47ad9496 100644 --- a/src/nfs.rs +++ b/src/nfs.rs @@ -301,21 +301,50 @@ impl NFSFileSystem for NFSAdapter { .map_err(errno_to_nfs); } Err(libc::EBADF) => { - // Handle was opened read-only. Upgrade to writable. + // Handle was opened read-only. Evict + remove from pool so + // a concurrent caller doesn't peek a freshly-released fh. + // Mirrors the analogous EBADF retry in `read()` above. + // Guard against removing a different fh: a successful + // concurrent upgrader may have already replaced our entry. + { + let mut pool = self.handle_pool.lock().expect("handle_pool poisoned"); + if pool.peek(id) == Some(fh) { + pool.remove(id); + } + } self.evict_handle(id, fh).await; } Err(e) => return Err(errno_to_nfs(e)), } } - // Slow path: open a writable handle and retry the write. + // Slow path: open a writable handle, run the pwrite, THEN publish the + // handle to the pool. Two concurrent writers can both reach this + // branch and open distinct writable handles (open is serialized by + // VirtualFs's per-inode staging lock, so the calls don't overlap, but + // they DO produce two distinct fh). If we inserted before the pwrite, + // the second writer's `insert_handle` would release the first + // writer's freshly-opened fh as `replaced` — and the first writer's + // subsequent pwrite would hit EBADF on a closed fh, mapping back to + // NFS3ERR_STALE (silent data loss, the very symptom this code path + // exists to avoid). By writing before publishing, the fh stays + // private to this task until its pwrite completes; nothing else can + // see it, nothing else can release it. let fh = self .virtual_fs .open(id, true, false, None) .await .map_err(errno_to_nfs)?; + if let Err(e) = self.virtual_fs.write(id, fh, offset, data) { + // Write failed — release the fh we opened so we don't leak it. + let _ = self.virtual_fs.release(fh).await; + return Err(errno_to_nfs(e)); + } + // Publish only on success. Any concurrent writer that reaches this + // point will release its own fh via the same Err path or replace + // ours via insert_handle's eviction, but by then our pwrite has + // committed to the staging file. self.insert_handle(id, fh).await; - self.virtual_fs.write(id, fh, offset, data).map_err(errno_to_nfs)?; // NFS has no close/flush RPC, so schedule a debounced flush after // each write to ensure data eventually gets committed to the Hub. self.virtual_fs.schedule_flush(id);