feat: Catch renames during watcher - #767
Conversation
📝 WalkthroughWalkthroughThe watcher now detects filesystem renames, emits destination and source paths, transfers frecency history, and exposes rename data through C, Node, Bun, and Python APIs. Tests cover rename pairing, subscriptions, frecency behavior, FFI access, and language integrations. ChangesRename-aware file watching
Tooling cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/fff-core/src/watcher/background_watcher.rs (1)
688-721: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftOne LMDB write transaction per renamed file.
copy_historyopens and commits its own write transaction for every entry inrenames. A directory move produces one explicit rename pair per file, so a large move turns into thousands of sequential commits on the watcher thread. This blocks event processing for the whole batch.Consider a batched API, for example
copy_history_many(&[(from, to)]), that writes all pairs in one transaction.🤖 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 `@crates/fff-core/src/watcher/background_watcher.rs` around lines 688 - 721, Update the frecency rename-history handling around shared_frecency and copy_history to use a batched operation such as copy_history_many for all renames in one write transaction. Preserve the existing copied-history semantics, collect only successfully carried destination paths, and retain the current error and picker-update behavior.
🤖 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 `@crates/fff-core/src/watcher/rescan_tests.rs`:
- Around line 585-598: Update the file-opening step in the rescan test before
set_modified to use OpenOptions with write access for src/keep.rs instead of
File::open. Preserve the existing timestamp assignment and watcher feed
behavior.
In `@crates/fff-core/src/watcher/watch.rs`:
- Around line 511-519: Ensure rename matching in the subscription loop preserves
destination-path ignore rules: after computing the destination result from
sub.filter_mask(&path_refs, &mut scratch), only merge source-path matches that
remain allowed by that destination result, so an ignored destination produces no
event. Update the matched-mask logic around filter_mask and retain normal source
matching for non-ignored destinations.
---
Nitpick comments:
In `@crates/fff-core/src/watcher/background_watcher.rs`:
- Around line 688-721: Update the frecency rename-history handling around
shared_frecency and copy_history to use a batched operation such as
copy_history_many for all renames in one write transaction. Preserve the
existing copied-history semantics, collect only successfully carried destination
paths, and retain the current error and picker-update behavior.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f344e43-c7d4-4f93-bb43-7157bc362662
📒 Files selected for processing (27)
Makefilecrates/fff-c/include/fff.hcrates/fff-c/src/watch.rscrates/fff-c/tests/smoke.ccrates/fff-core/src/dbs/frecency.rscrates/fff-core/src/shared.rscrates/fff-core/src/watcher/background_watcher.rscrates/fff-core/src/watcher/rescan_tests.rscrates/fff-core/src/watcher/watch.rscrates/fff-core/tests/rename_frecency_test.rscrates/fff-core/tests/watch_subscription_test.rscrates/fff-python/src/finder.rscrates/fff-python/src/types.rspackages/fff-bun/README.mdpackages/fff-bun/src/fff-api.tspackages/fff-bun/src/ffi.tspackages/fff-bun/test/watch.test.tspackages/fff-node/README.mdpackages/fff-node/src/fff-api.tspackages/fff-node/src/ffi.tspackages/fff-node/test/watch.mjspackages/fff-python/README.mdpackages/fff-python/src/fff/__init__.pyipackages/fff-python/tests/test_watch.pypackages/pi-fff/src/index.tspackages/shared/fff-api.tstests/test_cd_during_post_scan.lua
| let mtime = std::fs::metadata(f.path("src/gone.rs")) | ||
| .unwrap() | ||
| .modified() | ||
| .unwrap(); | ||
| std::fs::remove_file(f.path("src/gone.rs")).unwrap(); | ||
| std::fs::File::open(f.path("src/keep.rs")) | ||
| .unwrap() | ||
| .set_modified(mtime) | ||
| .unwrap(); | ||
|
|
||
| f.feed([ | ||
| remove_file(f.path("src/gone.rs")), | ||
| modify(f.path("src/keep.rs")), | ||
| ]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether this test module is gated to unix and whether Windows CI runs it.
rg -nP --type=rust -B5 'mod rescan_tests' crates/fff-core/src
fd -e yml -e yaml . .github/workflows --exec rg -nl 'windows'Repository: dmtrKovalenko/fff
Length of output: 653
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test module and nearby test setup ---'
cat -n crates/fff-core/src/watcher/mod.rs | sed -n '1,30p'
cat -n crates/fff-core/src/watcher/rescan_tests.rs | sed -n '560,610p'
printf '%s\n' '--- Windows and rescan_stats workflow entries ---'
for f in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -f "$f" ] || continue
printf '\n### %s\n' "$f"
rg -n -C 5 'windows|rescan_stats|cargo test|cargo nextest|target' "$f" || true
done
printf '%s\n' '--- all rescan_stats references ---'
rg -n -C 3 'rescan_stats|rescan_tests' .Repository: dmtrKovalenko/fff
Length of output: 39893
🌐 Web query:
Rust std fs File set_modified Windows FILE_WRITE_ATTRIBUTES File::open read access source
💡 Result:
To set the modification time of a file on Windows using Rust, you must open the file with the appropriate access rights. Using standard read-only access (via File::open) is insufficient because changing file timestamps requires the FILE_WRITE_ATTRIBUTES access right [1]. When using Rust's std::fs::File::set_times (or the nightly set_modified method), the underlying Windows API function is SetFileTime, which requires the file handle to have been opened with at least FILE_WRITE_ATTRIBUTES access [2][3][1]. To achieve this in Rust, you can use OpenOptions combined with the OpenOptionsExt trait available on Windows [4][5]. This allows you to manually specify the dwDesiredAccess flag for the underlying CreateFileW call [4][6]. Example implementation: use std::fs::OpenOptions; use std::os::windows::fs::OpenOptionsExt; use std::fs::FileTimes; use std::time::SystemTime; fn main -> std::io::Result<> { // Define the required access: FILE_WRITE_ATTRIBUTES (0x100) const FILE_WRITE_ATTRIBUTES: u32 = 0x100; let file = OpenOptions::new.access_mode(FILE_WRITE_ATTRIBUTES).open("your_file.txt")?; let new_time = SystemTime::now; let times = FileTimes::new.set_modified(new_time); // This will now succeed because the file handle has FILE_WRITE_ATTRIBUTES file.set_times(times)?; Ok() } In summary: 1. File::open typically uses read access (GENERIC_READ), which does not include FILE_WRITE_ATTRIBUTES [7][1]. 2. You must use OpenOptions::new.access_mode(FILE_WRITE_ATTRIBUTES).open(...) to obtain the necessary handle permissions [4][1]. 3. Once opened with this permission, you can successfully invoke set_times or set_modified [2][3].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants
- 2: https://dev-doc.rust-lang.org/beta/std/fs/struct.File.html
- 3: https://doc.rust-lang.org/stable/std/fs/fn.set_times.html
- 4: https://dev-doc.rust-lang.org/stable/std/os/windows/fs/trait.OpenOptionsExt.html
- 5: https://github.com/rust-lang/rust/blob/4a9d5368/library/std/src/os/windows/fs.rs
- 6: https://github.com/rust-lang/rust/blob/3802025f400af7817ba4874587e6a2df95abd65d/library/std/src/sys/windows/fs.rs
- 7: https://doc.rust-lang.org/std/fs/struct.File.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Cargo feature and test configuration ---'
rg -n -C 4 'rescan-stats|rescan_stats|default|features' Cargo.toml crates/fff-core/Cargo.toml
printf '%s\n' '--- Fixture file creation and permissions ---'
rg -n -C 5 'struct Fixture|impl Fixture|fn write\(|OpenOptions|set_permissions|readonly' crates/fff-core/src/watcher/rescan_tests.rs crates/fff-core/srcRepository: dmtrKovalenko/fff
Length of output: 15714
Open keep.rs with write access before calling set_modified. Windows CI runs this test, and File::open lacks the access required to change timestamps. Use OpenOptions::new().write(true).open(...).
🤖 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 `@crates/fff-core/src/watcher/rescan_tests.rs` around lines 585 - 598, Update
the file-opening step in the rescan test before set_modified to use OpenOptions
with write access for src/keep.rs instead of File::open. Preserve the existing
timestamp assignment and watcher feed behavior.
| let path_refs: Vec<&str> = paths.iter().map(String::as_str).collect(); | ||
| let from_refs: Vec<&str> = from_paths.iter().map(String::as_str).collect(); | ||
| let mut scratch = Vec::new(); | ||
| let mut deliveries = Vec::with_capacity(state.subs.len()); | ||
| for sub in &state.subs { | ||
| let matched = sub.filter_mask(&path_refs, &mut scratch); | ||
| let mut matched = sub.filter_mask(&path_refs, &mut scratch); | ||
| if has_renames { | ||
| matched |= sub.filter_mask(&from_refs, &mut scratch); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Rename matching bypasses the per-subscription ignore list.
filter_mask applies the ignore rules inside each call. When you OR the two masks, a bit cleared by the destination ignore can be set again by the source path. Example: subscription pattern **, ignore *.log, rename a.txt → b.log. The first mask is 0, the second mask sets the bit, and the subscriber receives an event whose path is the ignored b.log. The documented contract says ignored paths produce no events.
Decide the intended rule and enforce it. If the destination must stay ignorable, mask the rename hits with the destination result.
🔧 One possible fix
for sub in &state.subs {
let mut matched = sub.filter_mask(&path_refs, &mut scratch);
if has_renames {
- matched |= sub.filter_mask(&from_refs, &mut scratch);
+ // A rename is announced only if the destination itself is
+ // not excluded by this subscription.
+ let not_ignored = sub.filter_mask(&path_refs, &mut scratch)
+ | sub.ignore_free_mask(&path_refs, &mut scratch);
+ matched |= sub.filter_mask(&from_refs, &mut scratch) & not_ignored;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let path_refs: Vec<&str> = paths.iter().map(String::as_str).collect(); | |
| let from_refs: Vec<&str> = from_paths.iter().map(String::as_str).collect(); | |
| let mut scratch = Vec::new(); | |
| let mut deliveries = Vec::with_capacity(state.subs.len()); | |
| for sub in &state.subs { | |
| let matched = sub.filter_mask(&path_refs, &mut scratch); | |
| let mut matched = sub.filter_mask(&path_refs, &mut scratch); | |
| if has_renames { | |
| matched |= sub.filter_mask(&from_refs, &mut scratch); | |
| } | |
| for sub in &state.subs { | |
| let mut matched = sub.filter_mask(&path_refs, &mut scratch); | |
| if has_renames { | |
| // A rename is announced only if the destination itself is | |
| // not excluded by this subscription. | |
| let not_ignored = sub.filter_mask(&path_refs, &mut scratch) | |
| | sub.ignore_free_mask(&path_refs, &mut scratch); | |
| matched |= sub.filter_mask(&from_refs, &mut scratch) & not_ignored; | |
| } |
🤖 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 `@crates/fff-core/src/watcher/watch.rs` around lines 511 - 519, Ensure rename
matching in the subscription loop preserves destination-path ignore rules: after
computing the destination result from sub.filter_mask(&path_refs, &mut scratch),
only merge source-path matches that remain allowed by that destination result,
so an ignored destination produces no event. Update the matched-mask logic
around filter_mask and retain normal source matching for non-ignored
destinations.
1 participant
Summary by CodeRabbit
New Features
renamedevent with both destination and original paths.Documentation
Bug Fixes