Never pad a file that shrank under a mount with zeros

crates/sandfs-cli/tests/mount.rs

@@ -1098,6 +1098,43 @@ fn rereading_a_changed_file_never_returns_cached_bytes() {
     sandfs.unmount(&ws);
 }
 
+#[test]
+fn a_file_that_shrank_in_the_folder_reads_back_without_zeros() {
+    if !fuse_available() {
+        return;
+    }
+    let folder = tempfile::tempdir().unwrap();
+    std::fs::write(folder.path().join("notes.md"), "twenty-four bytes long..").unwrap();
+    let mut sandfs = Sandfs::new();
+    let dirs = tempfile::tempdir().unwrap();
+    let ws = dirs.path().join("ws");
+    sandfs.mount(&["mount", folder.path().to_str().unwrap(), ws.to_str().unwrap()], &ws);
+
+    // Listed with its size but never read, as `ls -l` leaves it; then the folder's
+    // file shrinks, as a copy onto it truncates it first. The mount used to answer
+    // with the 3 bytes left padded by zeros to the 24 it had listed.
+    let file = ws.join("notes.md");
+    assert_eq!(std::fs::metadata(&file).unwrap().len(), 24);
+    std::fs::write(folder.path().join("notes.md"), "new").unwrap();
+    // The macOS client won't take a smaller size from a read, so there the first read
+    // fails, and the next asks for the size again. A read never gives zeros.
+    let mut reads = 0;
+    let read = loop {
+        reads += 1;
+        match std::fs::read(&file) {
+            Ok(read) => break read,
+            Err(err) if cfg!(target_os = "macos") && reads < 10 => {
+                eprintln!("read {reads}: {err}");
+                std::thread::sleep(std::time::Duration::from_millis(500));
+            }
+            Err(err) => panic!("read {reads}: {err}"),
+        }
+    };
+    assert_eq!(read, b"new");
+    assert_eq!(std::fs::metadata(&file).unwrap().len(), 3);
+    sandfs.unmount(&ws);
+}
+
 #[test]
 fn open_files_work_without_open_or_close_reaching_sandfs() {
     use std::io::{Read, Seek, SeekFrom, Write};

crates/sandfs-core/src/source.rs

@@ -71,7 +71,8 @@ pub type Listing = BTreeMap<String, SourceEntry>;
 /// Implementations must return the same answer for the same key every time; the
 /// workspace caches results and shares them between forks. The one exception is a
 /// listing's sizes: after a [`Source::prefetch`], a listing may report sizes it left
-/// out before.
+/// out before. A source whose files can change underneath it (a folder on disk) can
+/// also say what a file is now, through [`Source::entry_now`].
 pub trait Source: Send + Sync + fmt::Debug {
     /// Key of the root directory.
     fn root(&self) -> Result<SourceKey>;
@@ -128,6 +129,17 @@ pub trait Source: Send + Sync + fmt::Debug {
         let _ = key;
         false
     }
+
+    /// The file at `key` as the source has it now, for a source whose files can change
+    /// while a workspace is over it (a folder on disk). The workspace asks only once a
+    /// read shows a file is no longer what its listing said (see
+    /// [`crate::Workspace::recheck`]). `None`, the default, keeps the answer the
+    /// workspace has, as a source whose answers never change should; so does a source
+    /// that no longer has the file.
+    fn entry_now(&self, key: &str) -> Result<Option<SourceEntry>> {
+        let _ = key;
+        Ok(None)
+    }
 }
 
 /// The part of `bytes` a ranged read asks for, clamped to its end.
@@ -235,12 +247,12 @@ impl DirSource {
         if !root.is_dir() {
             return Err(Error::Source(format!("{} is not a directory", root.display())));
         }
-        Ok(DirSource { root: root.canonicalize().map_err(io_error)? })
+        Ok(DirSource { root: root.canonicalize().map_err(|err| io_error(root, err))? })
     }
 
     /// Fail unless `dir`, with every symlink resolved, is still `root` or inside it.
     fn check_inside(&self, dir: &Path) -> Result<()> {
-        if dir.canonicalize().map_err(io_error)?.starts_with(&self.root) {
+        if dir.canonicalize().map_err(|err| io_error(dir, err))?.starts_with(&self.root) {
             Ok(())
         } else {
             Err(Error::Source(format!("{} is outside the mounted directory", dir.display())))
@@ -258,8 +270,11 @@ impl DirSource {
             use std::os::unix::fs::OpenOptionsExt;
             // A directory-only open fails at once on anything else, such as a fifo
             // swapped in since the listing, which a plain open would block on.
-            let opened =
-                std::fs::OpenOptions::new().read(true).custom_flags(libc::O_DIRECTORY).open(dir).map_err(io_error)?;
+            let opened = std::fs::OpenOptions::new()
+                .read(true)
+                .custom_flags(libc::O_DIRECTORY)
+                .open(dir)
+                .map_err(|err| io_error(dir, err))?;
             if let Ok(path) = std::fs::read_link(format!("/proc/self/fd/{}", opened.as_raw_fd())) {
                 return if path == dir {
                     Ok(())
@@ -283,7 +298,7 @@ impl DirSource {
     /// reaches on the host; an absolute one outside is kept as written.
     fn link_target(&self, key: &str) -> Result<Bytes> {
         let path = self.host_path(key);
-        let target = std::fs::read_link(&path).map_err(io_error)?;
+        let target = std::fs::read_link(&path).map_err(|err| io_error(&path, err))?;
         let parent = Path::new(key).parent().unwrap_or(Path::new(""));
         let rewritten = match self.named_inside(parent, &target) {
             Some(_) if !target.has_root() => target.clone(),
@@ -364,6 +379,33 @@ impl DirSource {
         Some(inside.into_iter().collect())
     }
 
+    /// The entry for the file at `key`, of type `file_type`, with `meta` its metadata
+    /// (its own for a link, not its target's). `None` for what a workspace doesn't
+    /// hold: special files, and links whose target no workspace path could hold.
+    fn entry_of(
+        &self,
+        key: String,
+        file_type: std::fs::FileType,
+        meta: impl FnOnce() -> std::io::Result<std::fs::Metadata>,
+    ) -> Result<Option<SourceEntry>> {
+        let (kind, size, modified) = if file_type.is_dir() {
+            (Kind::Dir, None, None)
+        } else if file_type.is_file() {
+            let meta = meta().map_err(|err| io_error(&self.host_path(&key), err))?;
+            (Kind::File { executable: is_executable(&meta) }, Some(meta.len()), meta.modified().ok())
+        } else if file_type.is_symlink() {
+            let modified = meta().ok().and_then(|meta| meta.modified().ok());
+            match self.link_target(&key) {
+                Ok(target) => (Kind::Symlink, Some(target.len() as u64), modified),
+                Err(Error::NotText(_)) => return Ok(None),
+                Err(err) => return Err(err),
+            }
+        } else {
+            return Ok(None);
+        };
+        Ok(Some(SourceEntry { kind, key: key.as_str().into(), size, modified }))
+    }
+
     fn host_path(&self, key: &str) -> PathBuf {
         if key.is_empty() { self.root.clone() } else { self.root.join(key) }
     }
@@ -378,7 +420,7 @@ impl DirSource {
         let path = self.host_path(key);
         // Checked before opening, so a special file (a fifo) is never opened, and a
         // link is never followed: its target is read from the folder holding it.
-        let file_type = path.symlink_metadata().map_err(io_error)?.file_type();
+        let file_type = path.symlink_metadata().map_err(|err| io_error(&path, err))?.file_type();
         if file_type.is_symlink() {
             let parent = Path::new(key).parent().and_then(Path::to_str).unwrap_or_default();
             self.check_dir(parent, path.parent().unwrap_or(&self.root))?;
@@ -387,7 +429,7 @@ impl DirSource {
         if !file_type.is_file() {
             return Err(Error::Source(format!("{} is not a regular file", path.display())));
         }
-        let file = std::fs::File::open(&path).map_err(io_error)?;
+        let file = std::fs::File::open(&path).map_err(|err| io_error(&path, err))?;
         // On Linux the kernel names the file that was actually opened. It must be this
         // very path: a symlink anywhere on the way would resolve to another one. That
         // is one call, where resolving the parent costs one per component.
@@ -424,46 +466,54 @@ impl Source for DirSource {
     fn read_dir(&self, key: &str) -> Result<Listing> {
         let dir = self.host_path(key);
         self.check_dir(key, &dir)?;
-        let entries = std::fs::read_dir(dir).map_err(io_error)?;
+        let entries = std::fs::read_dir(&dir).map_err(|err| io_error(&dir, err))?;
         let mut listing = Listing::new();
         for entry in entries {
-            let entry = entry.map_err(io_error)?;
+            let entry = entry.map_err(|err| io_error(&dir, err))?;
             let Ok(name) = entry.file_name().into_string() else {
                 continue; // non-UTF-8 names cannot be addressed by a workspace path
             };
             // The entry's type usually comes with the listing, so only files, which
             // need a size and a mode, cost a stat (relative to the open directory).
-            let file_type = entry.file_type().map_err(io_error)?;
+            let file_type = entry.file_type().map_err(|err| io_error(&entry.path(), err))?;
             let child_key = if key.is_empty() { name.clone() } else { format!("{key}/{name}") };
-            let (kind, size, modified) = if file_type.is_dir() {
-                (Kind::Dir, None, None)
-            } else if file_type.is_file() {
-                let meta = entry.metadata().map_err(io_error)?;
-                (Kind::File { executable: is_executable(&meta) }, Some(meta.len()), meta.modified().ok())
-            } else if file_type.is_symlink() {
-                // The listing's metadata is the link's own, not its target's.
-                let modified = entry.metadata().ok().and_then(|meta| meta.modified().ok());
-                match self.link_target(&child_key) {
-                    Ok(target) => (Kind::Symlink, Some(target.len() as u64), modified),
-                    Err(Error::NotText(_)) => continue, // a target no workspace path could hold
-                    Err(err) => return Err(err),
-                }
-            } else {
-                continue; // special files are not part of a workspace
-            };
-            listing.insert(name, SourceEntry { kind, key: child_key.as_str().into(), size, modified });
+            if let Some(found) = self.entry_of(child_key, file_type, || entry.metadata())? {
+                listing.insert(name, found);
+            }
         }
         Ok(listing)
     }
 
+    fn entry_now(&self, key: &str) -> Result<Option<SourceEntry>> {
+        if key.is_empty() || !Path::new(key).components().all(|part| matches!(part, Component::Normal(_))) {
+            return Ok(None);
+        }
+        // The folder holding it is checked as a listing checks it: one swapped for a
+        // link since must not lead outside.
+        let parent = Path::new(key).parent().and_then(Path::to_str).unwrap_or_default();
+        let path = self.host_path(key);
+        let dir = path.parent().unwrap_or(&self.root);
+        if !dir.is_dir() {
+            return Ok(None);
+        }
+        self.check_dir(parent, dir)?;
+        let meta = match path.symlink_metadata() {
+            Ok(meta) => meta,
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),
+            Err(err) => return Err(io_error(&path, err)),
+        };
+        self.entry_of(key.to_owned(), meta.file_type(), || Ok(meta.clone()))
+    }
+
     fn read_file(&self, key: &str) -> Result<Bytes> {
         let mut file = match self.open_file(key)? {
             Opened::File(file) => file,
             Opened::Link(target) => return Ok(target),
         };
-        let size = file.metadata().map_err(io_error)?.len();
+        let error = |err| io_error(&self.host_path(key), err);
+        let size = file.metadata().map_err(error)?.len();
         let mut data = Vec::with_capacity(usize::try_from(size).unwrap_or(0));
-        file.read_to_end(&mut data).map_err(io_error)?;
+        file.read_to_end(&mut data).map_err(error)?;
         Ok(Bytes::from(data))
     }
 
@@ -472,10 +522,11 @@ impl Source for DirSource {
             Opened::File(file) => file,
             Opened::Link(target) => return Ok(slice(&target, offset, len)),
         };
-        let available = file.metadata().map_err(io_error)?.len().saturating_sub(offset);
-        file.seek(SeekFrom::Start(offset)).map_err(io_error)?;
+        let error = |err| io_error(&self.host_path(key), err);
+        let available = file.metadata().map_err(error)?.len().saturating_sub(offset);
+        file.seek(SeekFrom::Start(offset)).map_err(error)?;
         let mut data = Vec::with_capacity(usize::try_from(len.min(available)).unwrap_or(0));
-        file.take(len).read_to_end(&mut data).map_err(io_error)?;
+        file.take(len).read_to_end(&mut data).map_err(error)?;
         Ok(Bytes::from(data))
     }
 }
@@ -506,8 +557,9 @@ fn relative_link(from: &Path, to: &Path) -> PathBuf {
     if link.as_os_str().is_empty() { PathBuf::from(".") } else { link }
 }
 
-fn io_error(err: std::io::Error) -> Error {
-    Error::Source(err.to_string())
+/// An error from the disk at `path`, naming it, so a log of failures says where.
+fn io_error(path: &Path, err: std::io::Error) -> Error {
+    Error::Source(format!("{}: {err}", path.display()))
 }
 
 #[cfg(unix)]

crates/sandfs-core/src/workspace.rs

@@ -101,6 +101,30 @@ impl Cache {
         self.dirs.lock().expect("cache lock").insert(key, Arc::new(Mutex::new(Some(listing))));
     }
 
+    /// Drop the cached blob for `key`, whose file changed in its source.
+    fn forget_blob(&self, key: &CacheKey) {
+        self.blobs.lock().expect("cache lock").remove(key);
+    }
+
+    /// Put `entry` in the cached listing `key` under `name`, in place of the entry with
+    /// the same source key, and say whether that changed it. The listing's own lock is
+    /// held throughout, so two files of one directory changed at once both stay changed.
+    /// `then` runs under that lock too, so what it caches from `entry` changes in the
+    /// same order as the listing.
+    fn patch_listing(&self, key: &CacheKey, name: &str, entry: SourceEntry, then: impl FnOnce(SourceEntry)) -> bool {
+        let Some(slot) = self.dirs.lock().expect("cache lock").get(key).cloned() else { return false };
+        let mut listing = slot.lock().expect("cache slot lock");
+        let Some(current) = listing.as_ref() else { return false };
+        if current.get(name).is_none_or(|listed| listed.key != entry.key || *listed == entry) {
+            return false;
+        }
+        let mut fresh = (**current).clone();
+        fresh.insert(name.to_owned(), entry.clone());
+        *listing = Some(Arc::new(fresh));
+        then(entry);
+        true
+    }
+
     /// The cached blob for `key`, if it is loaded and not being loaded right now.
     fn peek_blob(&self, key: &CacheKey) -> Option<Bytes> {
         let slot = self.blobs.lock().expect("cache lock").get(key)?.clone();
@@ -409,6 +433,58 @@ impl Workspace {
         })
     }
 
+    /// Ask the source again about the file at `path`, whose bytes this workspace hasn't
+    /// changed, after a read showed it is no longer what its listing said: a read that
+    /// came back shorter than the listed size, from a folder written to meanwhile. Its
+    /// size, time and cached bytes become what the source says now (see
+    /// [`Source::entry_now`]), for this file alone; the rest of its directory stays as
+    /// first listed. A file whose mode changed here keeps its mode. Returns whether
+    /// anything changed. Forks share the listing, as they share every listing.
+    pub fn recheck(&mut self, path: &str) -> Result<bool> {
+        let path = VPath::parse(path)?;
+        let Ok(Resolved::File { content: Content::Base { mount, key, .. }, .. }) = self.resolve(&path) else {
+            return Ok(false);
+        };
+        let Some(now) = mount.source.entry_now(&key)? else { return Ok(false) };
+        let mut changed = false;
+        // A file whose mode changed here, or that has several names, holds the
+        // source's size and time itself.
+        let held = match self.overlay.get(&path) {
+            Some(Overlay::Linked { id }) => {
+                let id = *id;
+                self.links.get_mut(&id).map(|linked| &mut linked.content)
+            }
+            Some(Overlay::File { .. }) => match self.overlay.get_mut(&path) {
+                Some(Overlay::File { content, .. }) => Some(content),
+                _ => None,
+            },
+            _ => None,
+        };
+        if let Some(Content::Base { key: held, size, modified, .. }) = held
+            && *held == key
+            && matches!(now.kind, Kind::File { .. })
+            && (*size, *modified) != (now.size, now.modified)
+        {
+            (*size, *modified) = (now.size, now.modified);
+            changed = true;
+        }
+        // A file moved here from another directory isn't in this listing.
+        if let (Some(parent), Some(name)) = (path.parent(), path.name())
+            && let Some(dir) = self.base_dir(&parent, &mount)?
+            && self.cached_listing(&mount, &dir)?.get(name).is_some_and(|entry| entry.key == key)
+            && let Some((mount_path, _)) = self.mounts.iter().find(|(_, owner)| owner.id == mount.id)
+            && self.cache.patch_listing(&(mount.id, dir), name, now, |now| {
+                self.cache.remember_entry(mount.id, &path.as_str()[mount_path.as_str().len() + 1..], now);
+            })
+        {
+            changed = true;
+        }
+        if changed {
+            self.cache.forget_blob(&(mount.id, key));
+        }
+        Ok(changed)
+    }
+
     /// [`Metadata::modified`] alone: the source's time for a file nobody wrote, found
     /// without reading the file even when the source left its size out.
     pub fn modified(&self, path: &str) -> Result<Option<SystemTime>> {