From e14ae7123f665c975bd2f2ec2c5fdf68ec546f4b Mon Sep 17 00:00:00 2001 From: wan9chi Date: Mon, 9 Nov 2026 15:21:55 +0800 Subject: [PATCH] fix(fspy): don't treat deleting a file as reading it on Windows Deleting a file or folder on Windows opens it with DELETE access and no access to its data. DeleteFileW, RemoveDirectoryW, std's remove_dir_all, libuv's unlink and the MSVC STL's remove, which Node's rmSync uses, all do this, and MoveFileExW opens a rename's source the same way. fspy recorded every open without write access as a read, so the deleted paths became inferred inputs of the task. When the task then wrote such a path again, as a build does after cleaning its output folder, the input was fingerprinted as the task left it. A later step that changed the file, such as one stamping a version into the output, made the next run miss with "'' modified", though the task never read the file. On Unix, unlink, rmdir and a rename's source aren't recorded, so the same task hit there. An open that asks for DELETE and no data access is now not recorded, as on Unix. Recording it as a write instead would make deleted paths outputs and the folders above them written in, which Unix doesn't do. FILE_DELETE_ON_CLOSE requires DELETE, so it needs no check of its own: a temporary file deleted on close is still a read or a write by its data access. Opens that only query attributes, such as a stat, are still reads. CreateSymbolicLinkW creates a file link with such an open, which is no longer recorded either; the reparse point it then sets is still recorded as a write. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + crates/fspy/tests/rust_std.rs | 18 +++--- .../src/windows/convert.rs | 9 +-- .../src/windows/detours/nt.rs | 19 ++++--- .../src/windows/winapi_utils.rs | 56 +++++++++++++++++-- .../snapshots.toml | 35 ++++++++++++ .../output_changed_after_clean_and_rebuild.md | 38 +++++++++++++ 7 files changed, 148 insertions(+), 28 deletions(-) create mode 100644 crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots/output_changed_after_clean_and_rebuild.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 601ba9eb9..eb0954b90 100644 --- a/CHANGELOG.md +++ b/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)). diff --git a/crates/fspy/tests/rust_std.rs b/crates/fspy/tests/rust_std.rs index f229a6849..9bbbc37b4 100644 --- a/crates/fspy/tests/rust_std.rs +++ b/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(()) } diff --git a/crates/fspy_preload_windows/src/windows/convert.rs b/crates/fspy_preload_windows/src/windows/convert.rs index 72a095786..6f27d69cd 100644 --- a/crates/fspy_preload_windows/src/windows/convert.rs +++ b/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; } impl ToAccessMode for AccessMode { - unsafe fn to_access_mode(self) -> AccessMode { - self + unsafe fn to_access_mode(self) -> Option { + Some(self) } } impl ToAccessMode for ACCESS_MASK { - unsafe fn to_access_mode(self) -> AccessMode { + unsafe fn to_access_mode(self) -> Option { access_mask_to_mode(self) } } diff --git a/crates/fspy_preload_windows/src/windows/detours/nt.rs b/crates/fspy_preload_windows/src/windows/detours/nt.rs index dd1d6d576..00b76e95b 100644 --- a/crates/fspy_preload_windows/src/windows/detours/nt.rs +++ b/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 diff --git a/crates/fspy_preload_windows/src/windows/winapi_utils.rs b/crates/fspy_preload_windows/src/windows/winapi_utils.rs index 9acd8126c..da2ca13e2 100644 --- a/crates/fspy_preload_windows/src/windows/winapi_utils.rs +++ b/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 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 { + 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; diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots.toml b/crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots.toml index f19d52418..c3c7e376b 100644 --- a/crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots.toml +++ b/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", + ], +] diff --git a/crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots/output_changed_after_clean_and_rebuild.md b/crates/vt_bin/tests/e2e_snapshots/fixtures/clean_and_rebuild_output_folder/snapshots/output_changed_after_clean_and_rebuild.md new file mode 100644 index 000000000..3dea7edf5 --- /dev/null +++ b/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 +```