Add basic WinFSP support on Windows (#3) - #327
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe pull request adds Windows support through a WinFSP filesystem adapter, platform-specific encrypted filename and synchronization behavior, Windows build dependencies, runtime wiring, CI coverage, and updated usage documentation. Windows WinFSP support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RencfsCLI
participant MountPointImpl
participant WinFSP
participant EncryptedFs
User->>RencfsCLI: Start rencfs with Windows drive letter
RencfsCLI->>MountPointImpl: Provide mount configuration
MountPointImpl->>WinFSP: Configure and mount volume
WinFSP->>EncryptedFs: Forward create, read, rename, and delete operations
EncryptedFs-->>WinFSP: Return filesystem results
WinFSP-->>User: Expose mounted drive operations
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Opire claim status: the reward record still points to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/build_and_tests.yaml (1)
27-27: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSwitching to the in-repo reusable workflow is the right call; consider scoping
secretsexplicitly.Now that the callee is local and reviewable,
secrets: inheritis far less risky than the previous remote@mainreference, but listing only the secrets the reusable workflow actually needs would silence the zizmorsecrets-inheritwarning and keep the blast radius small.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build_and_tests.yaml at line 27, Update the workflow invocation that uses ./.github/workflows/build_and_tests_reusable.yaml to replace broad secrets inheritance with an explicit mapping of only the secrets consumed by the reusable workflow. Inspect the callee’s declared secrets and pass those specific entries while preserving the existing workflow inputs.Source: Linters/SAST tools
.github/workflows/build_and_tests_reusable.yaml (1)
144-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSmoke test never exercises graceful unmount.
Stop-Process -ForceskipsMountHandle::umount, so the test proves mounting works but never validates clean shutdown — the very behavior this PR adds. Sending a graceful stop first (and assertingR:disappears) would cover it, falling back to-Forceon timeout. Cleaning up$dataDirafterwards would also keep the runner tidy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build_and_tests_reusable.yaml around lines 144 - 149, Update the smoke-test cleanup in the PowerShell finally block around $rencfsProcess to request graceful termination first, wait for the process to exit, and verify that the R: drive is unmounted; only fall back to Stop-Process -Force if graceful shutdown times out. Remove the temporary $dataDir after process cleanup.src/mount/windows.rs (1)
505-530: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffDirectory enumeration should be cached; dot entries are already translated
./..are normalized to./..bycreate_directory_entry_plus, so that part does not expose.$/$..to WinFSP. However,read_directorystill re-reads and re-sorts the whole directory for every sorted-marker chunk, which makes large directory enumeration O(n² log n); cache the sorted snapshot on the directoryFileContextfor the enumeration lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mount/windows.rs` around lines 505 - 530, Update read_directory around the self.fs.read_dir_plus call to cache the sorted directory-entry snapshot on FileContext for the lifetime of one enumeration, reusing it for subsequent marker-based chunks instead of re-reading and re-sorting. Ensure the cache is initialized once, preserves the existing name ordering, marker filtering, UTF-16 length check, and add_dir_info behavior, and is cleared when the enumeration completes or is otherwise reset.build.rs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
winfsp_wrsbuild step target-conditional.
build.rsis compiled for the host, so#[cfg(target_os = "windows")]makes the WinFSP linker setup run on any Windows host and skip cross-compiles to Windows from Linux. Gate this onCARGO_CFG_TARGET_OS; keep the inner#[cfg(target_os = "windows")]becausewinfsp_wrs_buildis only available for Windows target builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build.rs` around lines 1 - 4, Update the `main` function in `build.rs` to run `winfsp_wrs_build::build()` only when `CARGO_CFG_TARGET_OS` identifies a Windows target, while retaining the inner `#[cfg(target_os = "windows")]` guard so the Windows-only dependency is compiled only for Windows targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crypto.rs`:
- Around line 269-274: Update hash_file_name and the encryptedfs lookup paths
around directory listing, resolution, rename, and removal to recognize both
native and non-native dot-entry spellings, rather than relying on one
platform-specific result. Preserve normal hashing for other names, and add a
fixture/test covering a filesystem containing the non-native spelling so
cross-platform lookup succeeds.
In `@src/mount/windows.rs`:
- Around line 56-61: Update the security descriptor construction in the Windows
mount flow to replace the Everyone (`WD`) full-control ACE with the current
mounting user’s SID (`OW`), while retaining SYSTEM and Administrators access.
Resolve the current user SID at mount time and build the descriptor dynamically;
update `test_security_descriptor()` to use the same production descriptor format
and behavior.
- Around line 786-800: Keep setup_runtime alive for the entire test instead of
dropping it immediately after EncryptedFs::new; remove the explicit
drop(setup_runtime) and retain the runtime until all WindowsFs work completes,
preserving the existing setup flow.
---
Nitpick comments:
In @.github/workflows/build_and_tests_reusable.yaml:
- Around line 144-149: Update the smoke-test cleanup in the PowerShell finally
block around $rencfsProcess to request graceful termination first, wait for the
process to exit, and verify that the R: drive is unmounted; only fall back to
Stop-Process -Force if graceful shutdown times out. Remove the temporary
$dataDir after process cleanup.
In @.github/workflows/build_and_tests.yaml:
- Line 27: Update the workflow invocation that uses
./.github/workflows/build_and_tests_reusable.yaml to replace broad secrets
inheritance with an explicit mapping of only the secrets consumed by the
reusable workflow. Inspect the callee’s declared secrets and pass those specific
entries while preserving the existing workflow inputs.
In `@build.rs`:
- Around line 1-4: Update the `main` function in `build.rs` to run
`winfsp_wrs_build::build()` only when `CARGO_CFG_TARGET_OS` identifies a Windows
target, while retaining the inner `#[cfg(target_os = "windows")]` guard so the
Windows-only dependency is compiled only for Windows targets.
In `@src/mount/windows.rs`:
- Around line 505-530: Update read_directory around the self.fs.read_dir_plus
call to cache the sorted directory-entry snapshot on FileContext for the
lifetime of one enumeration, reusing it for subsequent marker-based chunks
instead of re-reading and re-sorting. Ensure the cache is initialized once,
preserves the existing name ordering, marker filtering, UTF-16 length check, and
add_dir_info behavior, and is cleared when the enumeration completes or is
otherwise reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7781076a-17ff-4f54-8db4-4434caaf3b51
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/build_and_tests.yaml.github/workflows/build_and_tests_reusable.yamlCargo.tomlbuild.rsdocs/readme/Usage.mdexamples/internal_ring_speed.rssrc/crypto.rssrc/crypto/read.rssrc/encryptedfs.rssrc/fs_util.rssrc/main.rssrc/mount.rssrc/mount/windows.rs
💤 Files with no reviewable changes (1)
- src/crypto/read.rs
|
Pushed
Validation: the earlier Windows run passed all 111 library tests, the repository Clippy command, rustfmt, and package creation. For The latest Actions run is awaiting first-time-contributor approval: https://github.com/xoriors/rencfs/actions/runs/30595747096. Could a maintainer approve it so the real WinFSP mount smoke test can run on |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mount/windows.rs (1)
487-514: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake replacing rename atomic with rollback protection.
In
EncryptedFsFuse3::rename,replace_if_existsdeletes the existing destination before callingEncryptedFs::rename. That leaves a multi-step path where, iffind_by_name,remove_*,remove_directory_entry,insert_directory_entry, or storage movement fails mid-way, the original destination entry/content can be removed without the rename completing. Prefer an atomic “rename replacing destination” path inEncryptedFs, or keep the destination under a coherent lock and rollback/detect failure before data loss.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mount/windows.rs` around lines 487 - 514, Update rename in EncryptedFsFuse3 so replace_if_exists does not remove the destination before the source rename can complete. Add or reuse an atomic replacement operation in EncryptedFs::rename that preserves the existing destination until all directory-entry and storage updates succeed, or provides rollback under one coherent lock; keep collision behavior unchanged when replacement is disabled.
🧹 Nitpick comments (1)
src/mount/windows.rs (1)
524-580: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCloning the entire cached directory listing on every paginated call.
cached_entries.as_ref().unwrap().clone()at Line 552 deep-copies every(String, FileInfo)in the directory on eachread_directoryinvocation, even though only entries aftermarkerare needed. For large directories enumerated across many WinFSP buffer-sized pages, this is O(n) work (allocations + string copies) per page, i.e. effectively O(n²) for a full listing.Since
directory_entriesis astd::sync::Mutex(not held across an.await), you can iterate directly under the lock instead of cloning out:♻️ Avoid the full-vector clone
- let entries = cached_entries.as_ref().unwrap().clone(); - drop(cached_entries); - - let mut exhausted = true; - for (name, info) in entries { + let entries_ref = cached_entries.as_ref().unwrap(); + let mut exhausted = true; + for (name, info) in entries_ref { if marker .as_deref() - .is_some_and(|marker| name.as_str() <= marker) + .is_some_and(|marker| name.as_str() <= marker.as_str()) { continue; } if name.encode_utf16().count() >= 255 { continue; } - if !add_dir_info(DirInfo::from_str(info, &name)) { + if !add_dir_info(DirInfo::from_str(info.clone(), name)) { exhausted = false; break; } } + drop(cached_entries);Separately, entries with names encoding to ≥255 UTF-16 units are silently skipped (Line 563-565) rather than surfaced as an error — worth a comment noting this is an intentional limit of the
DirInfobuffer rather than a bug, since it means such files/directories would simply never appear in directory listings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mount/windows.rs` around lines 524 - 580, Update read_directory to avoid cloning the entire cached_entries vector on each paginated call: keep the directory_entries mutex guard while iterating cached entries and invoking add_dir_info, releasing it only before any await (the existing read_dir_plus path). Preserve marker filtering, exhaustion cleanup, and pagination behavior. Add a brief comment at the UTF-16 name-length check documenting that skipping names of 255 or more units is an intentional DirInfo buffer limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.rs`:
- Around line 1-6: In the build script’s main function, remove the compile-time
#[cfg(target_os = "windows")] guard around winfsp_wrs_build::build(). Keep the
existing CARGO_CFG_TARGET_OS runtime check as the sole condition so
cross-compiling to Windows invokes the build dependency.
In `@src/encryptedfs.rs`:
- Around line 50-59: Update storage_entry_path for Windows trailing-dot names so
canonicalize() failures are surfaced instead of silently falling back to
directory.join(name). Prefer propagating the canonicalization error through
callers; if the function must remain infallible, emit a warning containing the
failure details before the fallback, ensuring trailing-dot entries do not
silently use a potentially altered path.
---
Outside diff comments:
In `@src/mount/windows.rs`:
- Around line 487-514: Update rename in EncryptedFsFuse3 so replace_if_exists
does not remove the destination before the source rename can complete. Add or
reuse an atomic replacement operation in EncryptedFs::rename that preserves the
existing destination until all directory-entry and storage updates succeed, or
provides rollback under one coherent lock; keep collision behavior unchanged
when replacement is disabled.
---
Nitpick comments:
In `@src/mount/windows.rs`:
- Around line 524-580: Update read_directory to avoid cloning the entire
cached_entries vector on each paginated call: keep the directory_entries mutex
guard while iterating cached entries and invoking add_dir_info, releasing it
only before any await (the existing read_dir_plus path). Preserve marker
filtering, exhaustion cleanup, and pagination behavior. Add a brief comment at
the UTF-16 name-length check documenting that skipping names of 255 or more
units is an intentional DirInfo buffer limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09bfdb98-73b6-47e1-be78-b17f9c96649f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/build_and_tests.yamlCargo.tomlbuild.rssrc/crypto.rssrc/encryptedfs.rssrc/encryptedfs/test.rssrc/mount/windows.rs
💤 Files with no reviewable changes (1)
- .github/workflows/build_and_tests.yaml
|
Follow-up
Validation: all 112 Windows library tests pass, including the new rollback regression and WinFSP adapter tests; rustfmt and the repository release Clippy command pass. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/encryptedfs.rs (2)
720-720: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider per-parent locking instead of one global mutation lock.
directory_entry_mutation_lockis a single filesystem-wide mutex, and increateit is held across inode serialization, contents-file creation withsync_all, directory syncs andopen()— per the comments at Lines 760-763 that path is deliberately slow. Every concurrent create/remove/rename in any directory is now serialized behind it (same pattern at Lines 927 and 1005), so multi-threaded workloads lose the parallelism theJoinSetfan-out was built for.A keyed lock (e.g.
ArcHashMap<u64, Mutex<()>>per parent ino, taking both parents in a stable order for rename) would preserve the atomicity you need for the check-then-insert races while keeping unrelated directories independent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/encryptedfs.rs` at line 720, Replace the filesystem-wide directory_entry_mutation_lock usage in create, remove, and rename with keyed mutexes scoped to each parent inode, retaining the lock across the existing mutation sequence. For rename, acquire both source and destination parent locks in a deterministic inode order to avoid deadlocks, while preserving atomic check-then-insert behavior and allowing unrelated directories to proceed concurrently.
2646-2678: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHash/ls entry pair has no compensation when only one half succeeds. Both the write and the remove path treat the hash file and the listing file as two independent operations, so a failure of the second leaves a half-entry: an orphan
lsfile appears inread_dirbut not infind_by_name/exists_by_name, or an orphan hash file blocks name reuse while staying invisible in listings.renamecompensates on its own paths, butcreate,remove_fileandremove_dirdo not.
src/encryptedfs.rs#L2646-L2678: after awaiting both handles, best-effort remove the side that succeeded before returning the error (see the sketch in the inline diff).src/encryptedfs.rs#L2758-L2766: ifremove_directory_entry_ls_filefails after the hash file was already deleted, restore the hash entry (or remove the ls file first and delete the hash entry last) so the entry stays consistently visible or consistently gone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/encryptedfs.rs` around lines 2646 - 2678, In src/encryptedfs.rs lines 2646-2678, update the create-entry flow around the hash and ls task handles to best-effort remove whichever side succeeded before returning an error from either operation, preserving consistent pair creation. In src/encryptedfs.rs lines 2758-2766, update the remove flow so a failure removing the ls entry does not leave the hash entry deleted: remove the ls side first and delete the hash side last, or restore the hash entry on failure, keeping both files consistently present or absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/encryptedfs.rs`:
- Line 720: Replace the filesystem-wide directory_entry_mutation_lock usage in
create, remove, and rename with keyed mutexes scoped to each parent inode,
retaining the lock across the existing mutation sequence. For rename, acquire
both source and destination parent locks in a deterministic inode order to avoid
deadlocks, while preserving atomic check-then-insert behavior and allowing
unrelated directories to proceed concurrently.
- Around line 2646-2678: In src/encryptedfs.rs lines 2646-2678, update the
create-entry flow around the hash and ls task handles to best-effort remove
whichever side succeeded before returning an error from either operation,
preserving consistent pair creation. In src/encryptedfs.rs lines 2758-2766,
update the remove flow so a failure removing the ls entry does not leave the
hash entry deleted: remove the ls side first and delete the hash side last, or
restore the hash entry on failure, keeping both files consistently present or
absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a219de1-ec4e-4eca-948a-4e644f7a80e0
📒 Files selected for processing (3)
src/encryptedfs.rssrc/encryptedfs/test.rssrc/mount/windows.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Description
Adds a basic native Windows mount implementation backed by WinFSP and the existing
EncryptedFs.Key changes:
MountHandle|, trailing-dot directory entries, directory/file flush behavior)R:mount smoke testThe Rust binding is
winfsp_wrs, which is MIT-licensed. WinFSP remains a separately installed runtime/driver and publishes its GPLv3 license with the WinFSP Free/Libre and Open Source Software exception.This also exposed a Windows
VirtualUnlockpanic inshush-rs; the dependency is temporarily pinned to the tested fix from Eyob94/shush-rs#22.Fixes #3
Type of change
Checklist
Validation
cargo test --release --lib mount::windows::tests: 3 passedcargo build --release --all-targets --all-features: passedcargo fmt --all -- --check: passedcargo package --allow-dirty --no-verify: passedThe GitHub Windows job additionally installs WinFSP and performs a real mounted-drive create/read/rename/delete smoke test.
Summary by CodeRabbit
New Features
Documentation
Tests