Fix set_len, read, write, and sync_all bugs (#764)

criccomini · web-flow · commit 81e6d2334804 · 2025-06-02T07:00:01.000+08:00
* Fix set_len to keep position

* Clean up formatting

* Rename orig to cursor

* Fix reads and writes

* Fix sync bugs
diff --git a/crates/vfs/src/memory/file.rs b/crates/vfs/src/memory/file.rs
@@ -114,13 +114,19 @@ impl File {
     /// Attempts to sync all OS-internal metadata to disk.
     #[allow(dead_code)]
     pub async fn sync_all(&self) -> io::Result<()> {
+        // Lock the Inner state and drive it to Idle
+        let mut inner = self.inner.lock().await;
+        inner.complete_inflight().await;
         Ok(())
     }
 
     /// This function is similar to `sync_all`, except that it may not
     /// synchronize file metadata to the filesystem.
     #[allow(dead_code)]
     pub async fn sync_data(&self) -> io::Result<()> {
+        // Lock the Inner state and drive it to Idle
+        let mut inner = self.inner.lock().await;
+        inner.complete_inflight().await;
         Ok(())
     }
 
@@ -133,6 +139,8 @@ impl File {
     /// will be extended to size and have all of the
     /// intermediate data filled in with 0s.
     ///
+    /// Length changes to not affect the cursor position.
+    ///
     /// # Errors
     ///
     /// This function will return an error if the file
@@ -157,6 +165,7 @@ impl File {
         inner.state = Busy(Box::pin(async move {
             let mut std = std.lock();
             let len = std.get_ref().len() as u64;
+            let cursor_pos = std.position();
 
             let extension = if size <= len {
                 None
@@ -182,9 +191,9 @@ impl File {
                 }
                 Ok(())
             }
-            .map(|_| 0); // the value is discarded later
+            .map(|_| cursor_pos);
 
-            // Return the result as a seek
+            // Reset to the original position (even if it's > size now)
             (Operation::Seek(res), buf)
         }));
 
@@ -241,7 +250,8 @@ impl AsyncRead for File {
                     let mut buf = buf_cell.take().unwrap();
 
                     if !buf.is_empty() {
-                        buf.copy_to(dst);
+                        let n = buf.copy_to(dst);
+                        inner.pos += n as u64;
                         *buf_cell = Some(buf);
                         return Ready(Ok(()));
                     }
@@ -260,7 +270,8 @@ impl AsyncRead for File {
 
                     match op {
                         Operation::Read(Ok(_)) => {
-                            buf.copy_to(dst);
+                            let n = buf.copy_to(dst);
+                            inner.pos += n as u64;
                             inner.state = Idle(Some(buf));
                             return Ready(Ok(()));
                         }
@@ -388,6 +399,9 @@ impl AsyncWrite for File {
 
                     let n = buf.copy_from(src);
 
+                    // Update position tracking with the number of bytes written
+                    inner.pos += n as u64;
+
                     let std = me.std.clone();
 
                     let blocking_task_join_handle = Box::pin(async move {
diff --git a/crates/vfs/src/tests.rs b/crates/vfs/src/tests.rs
@@ -60,6 +60,131 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    async fn poll_complete_after_write() -> Result<()> {
+        use futures::future::poll_fn;
+        use std::pin::Pin;
+        use tokio::io::{AsyncSeek, AsyncWriteExt};
+
+        let path = "pos_after_write.txt";
+        let mut f = File::create(path).await?;
+        f.write_all(b"hello").await?; // 5 bytes
+        f.flush().await?; // wait for the write to finish
+
+        // Ask the file itself where it thinks the cursor is.
+        let mut pinned = Pin::new(&mut f);
+        let pos = poll_fn(|cx| pinned.as_mut().poll_complete(cx)).await?;
+
+        // **BUG**: returns 0 with the current implementation
+        assert_eq!(pos, 5, "cursor should sit just past the 5 written bytes");
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn poll_complete_after_set_len() -> Result<()> {
+        use futures::future::poll_fn;
+        use std::pin::Pin;
+        use tokio::io::{AsyncSeek, AsyncSeekExt, AsyncWriteExt};
+
+        let path = "pos_after_set_len.txt";
+        let mut f = File::create(path).await?;
+        f.write_all(b"abcdef").await?; // 6 bytes
+        f.flush().await?;
+        f.seek(std::io::SeekFrom::Start(2)).await?; // move to byte 2
+
+        f.set_len(100).await?; // extend – should *not* move the cursor
+
+        let mut pinned = Pin::new(&mut f);
+        let pos = poll_fn(|cx| pinned.as_mut().poll_complete(cx)).await?;
+
+        // **BUG**: current code reports 0 instead of 2
+        assert_eq!(pos, 2, "set_len must preserve the current offset");
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn poll_complete_after_read() -> Result<()> {
+        use futures::future::poll_fn;
+        use std::pin::Pin;
+        use tokio::io::{AsyncReadExt, AsyncSeek, AsyncWriteExt};
+
+        // Create a test file with some content
+        let path = "pos_after_read.txt";
+        let mut f = File::create(path).await?;
+        f.write_all(b"hello world").await?; // 11 bytes
+        f.flush().await?;
+
+        // Close and reopen the file for reading
+        drop(f);
+        let mut f = File::open(path).await?;
+
+        // Read part of the content
+        let mut buf = [0u8; 5]; // Only read first 5 bytes
+        f.read_exact(&mut buf).await?;
+        assert_eq!(&buf, b"hello");
+
+        // Ask the file where it thinks the cursor is
+        let mut pinned = Pin::new(&mut f);
+        let pos = poll_fn(|cx| pinned.as_mut().poll_complete(cx)).await?;
+
+        // Verify the cursor has advanced by the number of bytes read
+        assert_eq!(pos, 5, "cursor should sit just past the 5 read bytes");
+
+        // Read more bytes and check position again
+        f.read_exact(&mut buf).await?; // Read next 5 bytes
+        assert_eq!(&buf, b" worl");
+
+        let mut pinned = Pin::new(&mut f);
+        let pos = poll_fn(|cx| pinned.as_mut().poll_complete(cx)).await?;
+
+        // Verify cursor has advanced further
+        assert_eq!(pos, 10, "cursor should advance after additional reads");
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn seek_after_pending_write() -> Result<()> {
+        use tokio::io::{AsyncSeekExt, AsyncWriteExt};
+
+        let path = "seek_after_pending.txt";
+        let mut f = File::create(path).await?;
+        f.write_all(b"xyz").await?; // leaves an in-flight write
+
+        // Try to find out where we are without an explicit flush.
+        // Tokio’s real File lets this succeed; our version currently errors.
+        let pos = f.seek(std::io::SeekFrom::Current(0)).await;
+
+        // **BUG**: expect Ok(3), get Err(other operation is pending)
+        assert_eq!(
+            pos.unwrap(),
+            3,
+            "seek should wait for the write to finish"
+        );
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn test_sync_all() -> Result<()> {
+        let buf = [0u8; 1024];
+        let path = "test_sync_all.txt";
+        let tmp_path = "test_sync_all.txt.tmp";
+        let mut f = OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .open(&tmp_path)
+            .await?;
+        f.write_all(&buf).await?;
+        f.sync_all().await?;
+        assert!(vfs::try_exists(&tmp_path).await?);
+        assert_eq!(vfs::metadata(&tmp_path).await?.len(), buf.len() as u64);
+        vfs::rename(tmp_path, path).await?;
+        assert!(vfs::try_exists(path).await?);
+        assert_eq!(vfs::metadata(path).await?.len(), buf.len() as u64);
+        Ok(())
+    }
+
     async fn file_write_read() -> Result<()> {
         let path = "test.txt";
         let contents = "Mock content";
