Repository navigation
fix(windows): coalesce the deletion reported before a case-only rename - #1204
Conversation
Renaming a file to a different case, `file` to `FILE`, makes ReadDirectoryChangesW report FILE_ACTION_REMOVED for the old name on top of the usual RENAMED_OLD_NAME/RENAMED_NEW_NAME pair. Watchdog already turned that pair into a move event, so a case-only rename surfaced as a FileDeletedEvent followed by a FileMovedEvent, and a directory renamed the same way as a FileDeletedEvent followed by a DirMovedEvent. Consumers had no way to tell those deletions from real ones. DirectoryChangeReader.get_events() now drops a deletion when the very next record renames that same path. A rename that also changes the name reports the pair alone, and nothing can rename a path that was genuinely deleted, so the condition cannot swallow a real deletion; a deletion ending the batch is left untouched. Cross-directory moves are deliberately unchanged: Windows reports those as a plain delete/create carrying no rename records at all, whether or not the case differs, so they cannot be recovered here. tests/test_emitter.py gains test_case_only_rename_on_windows, which fails on the spurious FileDeletedEvent before the fix and passes after it. tests/test_observers_winapi.py pins the negative cases directly. Closes gorakhargosh#752
|
CI note, so the red X isn't misread: the 2 failing jobs are 🐧 Linux 3.13t / 3.14t, both on The 24 other jobs pass, including every Windows one. On 🪟 Windows 3.14 the new tests all ran green: Happy to rebase if you land a fix for the debounce test and want a clean run. |
| ec.add(DirModifiedEvent, "dir2") | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Actually, there is no need for a new test, but only to adapt test_case_change(). For instance, the check for Windows in the events list should disappear with your patch, correct?
watchdog/tests/test_emitter.py
Lines 191 to 198 in a0bb46c
Since the test still passes, I guess there is something missing?
In that same test, maybe could you add your tests ("file" -> "FILE", and "dir" -> "DIR"), add a subfolder renaming too, so that all cases are checked?
There was a problem hiding this comment.
Good question, and the answer is that nothing is missing — but test_case_change() is misleadingly named, which is what makes it look that way.
It does mv(p("dir1", "file"), p("dir2", "FILE")), which is a move between two directories that happens to also change the case. Windows reports a cross-directory move with no rename records at all, so there is nothing for the patch to coalesce and the Windows branch has to stay. Here are the raw ReadDirectoryChangesW actions, with the filter bypassed so this is what the OS actually hands us:
[1] dir1/file -> dir2/FILE (what the test does) DELETED, CREATED, MODIFIED
[2] dir1/a -> dir2/b (no case change) DELETED, CREATED, MODIFIED
[3] other -> OTHER (in place) DELETED, RENAMED_OLD_NAME, RENAMED_NEW_NAME
[4] subdir -> SUBDIR (in place) DELETED, RENAMED_OLD_NAME, RENAMED_NEW_NAME
[5] dir1/sub -> dir1/SUB (in place) MODIFIED, DELETED, RENAMED_OLD_NAME, RENAMED_NEW_NAME, MODIFIED
[1] and [2] are byte-for-byte the same shape, which is the proof: the existing test's behaviour has nothing to do with case. The extra DELETED only appears on the in-place renames, [3] to [5], and that is the one this PR drops.
So the test passing before and after is correct, not a symptom.
I have folded everything into test_case_change() as you asked and dropped the separate test. It now runs the original cross-directory move unchanged, then three in-place renames: a file, a directory, and a subfolder one level down. Checking it has teeth — with winapi.py reverted to master:
$ git checkout upstream/master -- src/watchdog/observers/winapi.py
$ python -m pytest tests/test_emitter.py::test_case_change
E AssertionError: wrong number of events
found_events = [FileDeletedEvent(src_path='...\test_case_change0\other'),
FileMovedEvent(src_path='...\other', dest_path='...\OTHER')]
1 failed
and restored, 1 passed. Full suite here: 2 failed, 162 passed, 13 skipped on Windows 11 / Python 3.13.13 — the two failures are test_renaming_top_level_directory and test_move_nested_subdirectories_on_windows, which fail the same way on a clean master checkout on this machine. ruff format --check and ruff check are clean.
One thing I could not verify myself: the Linux and macOS expectations for the three new in-place renames. I mirrored the existing patterns — the file case follows test_file_lifecyle and the directory case follows test_renaming_top_level_directory — but I have no macOS machine, so if FSEvents reports a case-only rename differently that is where it will show up. Happy to adjust once CI has run.
Review feedback: rather than a separate Windows-only test, cover every case-change variant in test_case_change() and add a subfolder rename. The cross-directory move that test_case_change() already performed is kept exactly as it was, including its Windows branch, because Windows reports that operation with no rename records at all. Three in-place renames are added after it: a file, a directory, and a subfolder one level down. Reverting winapi.py to master now fails this test on the in-place file rename with a FileDeletedEvent alongside the FileMovedEvent, so the coalescing is what the test pins.
|
Thanks @MohammedAlkindi 🥂 |
Closes #752
Symptom
Renaming a file to a different case —
filetoFILE— emits two events on Windows instead of one:A directory renamed the same way emits
FileDeletedEventfollowed byDirMovedEvent. Since the deletion is indistinguishable from a real one, consumers see a file vanish and then move.Cause
The move event was never the problem — watchdog already builds it correctly from the rename pair. The extra deletion comes straight from the OS. Dumping the raw
ReadDirectoryChangesWrecords on Windows 11 (3 runs each, identical every time):file→FILEREMOVED(file),RENAMED_OLD_NAME(file),RENAMED_NEW_NAME(FILE)a→bRENAMED_OLD_NAME(a),RENAMED_NEW_NAME(b)sub→SUB(dir)REMOVED(sub),RENAMED_OLD_NAME(sub),RENAMED_NEW_NAME(SUB)Windows prepends a
FILE_ACTION_REMOVEDfor the old name only when the rename changes nothing but case.queue_events()translates each record independently, so that record became a deletion event.Fix
DirectoryChangeReader.get_events()drops a deletion when the very next record renames that same path. Two properties make that condition safe rather than heuristic:CREATEDrecord sits between them and the deletion is kept.A deletion that ends the batch is also kept, so a rename split across two
ReadDirectoryChangesWbuffers degrades to today's behaviour rather than losing an event.Deliberately unchanged
Cross-directory moves.
tests/test_emitter.py::test_case_change— the test linked from the issue — movesdir1/filetodir2/FILE, and Windows reports that asREMOVED+CREATED+MODIFIEDwith no rename records at all. It reportsdir1/a→dir2/bidentically, so this is not case-related: the information needed to emit a move is simply absent, and recovering it would mean correlating unrelated deletions with later creations. That test is left exactly as it is, and this PR does not change cross-directory behaviour.Testing
Run on Windows 11, Python 3.13.13.
tests/test_emitter.py::test_case_only_rename_on_windows— new. Renames a file and a directory by case only and asserts exactly one move event each. Fails before the fix withAssertionError: wrong number of events, having foundFileDeletedEvent+FileMovedEvent; passes after.tests/test_observers_winapi.py— new unit tests pinning the condition, including the three negative cases that must keep their deletion: deleted-recreated-then-renamed, a deletion followed by a rename of a different path, and a deletion ending the batch.mastercheckout, 163 passed / 2 failed with this branch. The same two —test_renaming_top_level_directoryandtest_move_nested_subdirectories_on_windows— fail identically before and after (verified by stashing the change and re-running); they are pre-existing on Windows and untouched here.ruff format --check,ruff check, andsphinx-buildare clean.mypy src docs/source/examplesreports 11 errors both before and after — the same set, unusedtype: ignorecomments plus POSIX-only attributes flagged when run on Windows. No new errors introduced.I could not run the Linux or macOS jobs locally, so I left the new emitter test Windows-gated rather than assert behaviour I had not observed — the bug is a Windows-API artefact and the fix is confined to
winapi.py. CI is the authority for the other two platforms.