fix(fspy): don't treat deleting a file as reading it on Windows

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Changelog
 
+- **Fixed** On Windows, the files a task deletes or moves, such as a build cleaning its output folder before writing it again, are no longer among its inputs, as on Linux and macOS. Before, when a later step changed one of those files, for example by stamping a version into the rebuilt output, the next run missed the cache with a reason like `'dist/index.js' modified`, though the task never read the file ([#980](https://github.com/voidzero-dev/vite-task/pull/980)).
 - **Changed** In `@voidzero-dev/vite-task-client`, a `getEnvs` query that matches every env name, `{ prefix: '' }` or `*`, now returns nothing instead of every environment variable, and no longer adds them to the cache key ([#976](https://github.com/voidzero-dev/vite-task/pull/976)).
 - **Added** `vp run --json` prints every package and task in the workspace, with each task's `dependsOn` resolved to task IDs, as one JSON document for scripts, editors and coding agents. It runs nothing and, like `vp run` without a task, loads every package's config. It prints the same document from any directory in the workspace: indented in a terminal, on one line when captured. Its `$schema` field points to a JSON Schema that describes each field, which `vp run` writes to the system temp directory ([#948](https://github.com/voidzero-dev/vite-task/pull/948)).
 - **Fixed** On Linux, where every user shares the temp folder `/tmp`, the helper files `vp run` writes there, such as the library that tracks file accesses, now go in a folder only the current user can write to, like `/tmp/fspy-1000`. Before, they went in a folder such as `/tmp/fspy` that belonged to whoever ran `vp run` first: running a different version as another user failed, and the same version used files that user could replace ([#947](https://github.com/voidzero-dev/vite-task/pull/947)).

crates/fspy/tests/rust_std.rs

@@ -160,16 +160,14 @@ async fn symlink_destination() -> anyhow::Result<()> {
         }
     })
     .await?;
-    for link in ["file_link", "folder_link"] {
-        let link = tmp_path.join(link);
-        #[cfg(unix)]
-        assert_contains(&accesses, &link, PathAccess::Write);
-        // `CreateSymbolicLinkW` creates the link with an open that lacks write
-        // access, recorded as a read, or as a mkdir for a folder link, before
-        // it sets the reparse point.
-        #[cfg(windows)]
-        test_utils::assert_includes(&accesses, &link, PathAccess::Write);
-    }
+    assert_contains(&accesses, &tmp_path.join("file_link"), PathAccess::Write);
+    let folder_link = tmp_path.join("folder_link");
+    #[cfg(unix)]
+    assert_contains(&accesses, &folder_link, PathAccess::Write);
+    // `CreateSymbolicLinkW` makes a folder link as a folder, recorded as a
+    // mkdir, before it sets the reparse point.
+    #[cfg(windows)]
+    test_utils::assert_includes(&accesses, &folder_link, PathAccess::Write);
 
     Ok(())
 }

crates/fspy_preload_windows/src/windows/convert.rs

@@ -11,18 +11,19 @@ use crate::windows::winapi_utils::{
     access_mask_to_mode, combine_paths, get_path_name, get_u16_str,
 };
 
+/// The access to record, or `None` for one that isn't recorded.
 pub trait ToAccessMode: Debug {
-    unsafe fn to_access_mode(self) -> AccessMode;
+    unsafe fn to_access_mode(self) -> Option<AccessMode>;
 }
 
 impl ToAccessMode for AccessMode {
-    unsafe fn to_access_mode(self) -> AccessMode {
-        self
+    unsafe fn to_access_mode(self) -> Option<AccessMode> {
+        Some(self)
     }
 }
 
 impl ToAccessMode for ACCESS_MASK {
-    unsafe fn to_access_mode(self) -> AccessMode {
+    unsafe fn to_access_mode(self) -> Option<AccessMode> {
         access_mask_to_mode(self)
     }
 }

crates/fspy_preload_windows/src/windows/detours/nt.rs

@@ -370,11 +370,11 @@ unsafe fn handle_destination(
 
 // A reparse point set on a file or folder, which makes it a link, is recorded
 // as a write before the call, so a failed call is recorded too, as `symlink`
-// is on Unix. `CreateSymbolicLinkW` creates the link with an open that lacks
-// write access, recorded as a read (or as a mkdir for a folder link), and
-// sets its reparse point here. A junction, as libuv makes for Node's
-// `fs.symlink` with `'junction'`, is a folder, recorded as a mkdir, with a
-// mount point set here.
+// is on Unix. `CreateSymbolicLinkW` creates the link with an open that asks
+// for `DELETE` and no data access, which isn't recorded (or as a mkdir for a
+// folder link), and sets its reparse point here. A junction, as libuv makes
+// for Node's `fs.symlink` with `'junction'`, is a folder, recorded as a mkdir,
+// with a mount point set here.
 static DETOUR_NT_FS_CONTROL_FILE: Detour<
     unsafe extern "system" fn(
         file_handle: HANDLE,
@@ -495,6 +495,10 @@ static DETOUR_NT_QUERY_ATTRIBUTES_FILE: Detour<
     };
 
 unsafe fn handle_open(access_mode: impl ToAccessMode, path: impl ToAbsolutePath) {
+    // SAFETY: converting access mask to AccessMode via FFI-aware trait
+    let Some(access_mode) = (unsafe { access_mode.to_access_mode() }) else {
+        return;
+    };
     // SAFETY: accessing the global client which was initialized during DLL_PROCESS_ATTACH
     let client = unsafe { global_client() };
     // SAFETY: resolving path from Windows object attributes or handle for access tracking
@@ -505,10 +509,7 @@ unsafe fn handle_open(access_mode: impl ToAccessMode, path: impl ToAbsolutePath)
             };
             let path = path.as_slice();
             let path_access = path.iter().rposition(|c| *c == u16::from(b'*')).map_or_else(
-                || {
-                    // SAFETY: converting access mask to AccessMode via FFI-aware trait
-                    access_mode.to_access_mode().with_path(IpcPath::from_wide(path))
-                },
+                || access_mode.with_path(IpcPath::from_wide(path)),
                 |wildcard_pos| {
                     let path_before_wildcard = &path[..wildcard_pos];
                     let slash_pos = path_before_wildcard

crates/fspy_preload_windows/src/windows/winapi_utils.rs

@@ -13,8 +13,8 @@ use winapi::{
     um::{
         fileapi::GetFinalPathNameByHandleW,
         winnt::{
-            ACCESS_MASK, FILE_APPEND_DATA, FILE_READ_DATA, FILE_WRITE_DATA, GENERIC_READ,
-            GENERIC_WRITE,
+            ACCESS_MASK, DELETE, FILE_APPEND_DATA, FILE_EXECUTE, FILE_READ_DATA, FILE_WRITE_DATA,
+            GENERIC_ALL, GENERIC_EXECUTE, GENERIC_READ, GENERIC_WRITE, MAXIMUM_ALLOWED,
         },
     },
 };
@@ -93,14 +93,34 @@ pub unsafe fn get_path_name(handle: HANDLE) -> winsafe::SysResult<SmallVec<u16,
     Ok(path)
 }
 
-pub const fn access_mask_to_mode(desired_access: ACCESS_MASK) -> AccessMode {
+/// The access an open asking for `desired_access` is recorded as, or `None`
+/// if it isn't recorded. An open asking for `DELETE` and no data access, as
+/// deleting a file or folder or renaming it does, doesn't read the path, like
+/// `unlink`, `rmdir` and a rename's source, which aren't recorded on Unix.
+/// The attribute access such opens add doesn't make them reads, and
+/// `FILE_DELETE_ON_CLOSE` requires `DELETE`, so it needs no check of its own.
+/// Any other open is a write if it asks to write the data, and a read
+/// otherwise, attribute-only opens such as a stat included.
+pub const fn access_mask_to_mode(desired_access: ACCESS_MASK) -> Option<AccessMode> {
+    const DATA_ACCESS: ACCESS_MASK = FILE_READ_DATA
+        | FILE_WRITE_DATA
+        | FILE_APPEND_DATA
+        | FILE_EXECUTE
+        | GENERIC_READ
+        | GENERIC_WRITE
+        | GENERIC_EXECUTE
+        | GENERIC_ALL
+        | MAXIMUM_ALLOWED;
+    if desired_access & DELETE != 0 && desired_access & DATA_ACCESS == 0 {
+        return None;
+    }
     let has_write = (desired_access & (FILE_WRITE_DATA | FILE_APPEND_DATA | GENERIC_WRITE)) != 0;
     let has_read = (desired_access & (FILE_READ_DATA | GENERIC_READ)) != 0;
-    if has_write {
+    Some(if has_write {
         if has_read { AccessMode::ReadWrite } else { AccessMode::Write }
     } else {
         AccessMode::Read
-    }
+    })
 }
 
 pub struct HeapPath(PWSTR);
@@ -166,6 +186,32 @@ mod tests {
         test_get_path_name(str::repeat("a", 255).as_str());
     }
 
+    #[test]
+    fn delete_only_opens_are_not_recorded() {
+        use fspy_shared::ipc::AccessMode;
+        use winapi::um::winnt::{
+            DELETE, FILE_READ_ATTRIBUTES, FILE_READ_DATA, FILE_WRITE_ATTRIBUTES, GENERIC_READ,
+            GENERIC_WRITE, SYNCHRONIZE,
+        };
+
+        use super::access_mask_to_mode;
+
+        // std's `remove_dir_all`, `DeleteFileW`, and libuv's unlink.
+        for access in [
+            DELETE,
+            DELETE | FILE_READ_ATTRIBUTES | SYNCHRONIZE,
+            DELETE | FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES | SYNCHRONIZE,
+        ] {
+            assert_eq!(access_mask_to_mode(access), None);
+        }
+        assert_eq!(access_mask_to_mode(DELETE | FILE_READ_DATA), Some(AccessMode::Read));
+        assert_eq!(
+            access_mask_to_mode(DELETE | GENERIC_READ | GENERIC_WRITE),
+            Some(AccessMode::ReadWrite)
+        );
+        assert_eq!(access_mask_to_mode(FILE_READ_ATTRIBUTES | SYNCHRONIZE), Some(AccessMode::Read));
+    }
+
     #[test]
     fn test_combine_path() {
         use widestring::u16cstr;

crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots.toml

@@ -38,3 +38,38 @@ steps = [
     "out/a.txt",
   ],
 ]
+
+[[e2e]]
+name = "output_changed_after_clean_and_rebuild"
+comment = """
+The task removes `out` with `out/a.txt` in it and writes `out/a.txt` again, then a later step changes `out/a.txt`. Removing the file doesn't read it, so the change doesn't make the next run miss.
+"""
+steps = [
+  { argv = [
+    "vtt",
+    "write-file",
+    "out/a.txt",
+    "stale",
+  ], comment = "an earlier build left `out/a.txt`" },
+  [
+    "vt",
+    "run",
+    "build",
+  ],
+  { argv = [
+    "vtt",
+    "write-file",
+    "out/a.txt",
+    "stamped",
+  ], comment = "an uncached step changes the output, like a step stamping a version into it" },
+  [
+    "vt",
+    "run",
+    "build",
+  ],
+  [
+    "vtt",
+    "print-file",
+    "out/a.txt",
+  ],
+]

crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots/output_changed_after_clean_and_rebuild.md

@@ -0,0 +1,38 @@
+# output_changed_after_clean_and_rebuild
+
+The task removes `out` with `out/a.txt` in it and writes `out/a.txt` again, then a later step changes `out/a.txt`. Removing the file doesn't read it, so the change doesn't make the next run miss.
+
+## `vtt write-file out/a.txt stale`
+
+an earlier build left `out/a.txt`
+
+```
+```
+
+## `vt run build`
+
+```
+$ vtt fs read src/a.txt list out rm out mkdir out write out/a.txt hi stat out
+```
+
+## `vtt write-file out/a.txt stamped`
+
+an uncached step changes the output, like a step stamping a version into it
+
+```
+```
+
+## `vt run build`
+
+```
+$ vtt fs read src/a.txt list out rm out mkdir out write out/a.txt hi stat out ◉ cache hit, replaying
+
+---
+vt run: cache hit.
+```
+
+## `vtt print-file out/a.txt`
+
+```
+hi
+```