Address issue #478: harden file watching against transient paths and remote volumes#492
Merged
Merged
Conversation
…remote volumes - canWatchPath: falls back to the parent directory for the volume-locality check when the file is transiently missing (e.g. an external editor's atomic rename-replace save), so auto-reload is no longer silently disabled. It now also captures and logs the NSError from getResourceValue:. - Guard MPResourceWatcherSet -addWatcherForPath: with canWatchPath: up front, mirroring the document watcher. - Reorder MPDocument -startFileWatching so stopFileWatching always runs first, preventing a stale watcher leak on the nil/non-file-URL early return. - Add regression tests for the empty-string, non-existent-local, and missing-parent canWatchPath: branches and an unwatchable resource path. - Update the canWatchPath: header doc comment to describe all cases. Related to #478
schuyler
force-pushed
the
claude/release-candidate-blockers-7cn2q8
branch
from
6860df1 to
0173640
Compare
Contributor
Code Coverage ReportCurrent Coverage: 62.93% Coverage Details (Summary) |
schuyler
added
the
rc-pending
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the three file-watcher gaps identified in review of the merged PR #424 (
canWatchPath:local-volume gating). The headline fix is an auto-reload regression: after an external editor's atomic rename-replace save, the locality check could disable watching and silently stop live preview updates.Changes
+[MPFileWatcher canWatchPath:]—NSURLVolumeIsLocalKeycan only be read from a URL that resolves to an existing item. When the file is transiently missing (mid atomic save), probe the parent directory's volume instead, so auto-reload isn't wrongly disabled. ReturnNOonly if neither the path nor its parent resolves, or the volume is non-local. Also captures theNSErrorfromgetResourceValue:(wasNULL) andNSLogs it to help diagnose "Device not configured" failures.-[MPResourceWatcherSet addWatcherForPath:]— added an explicitcanWatchPath:guard up front, mirroring the document watcher. (Resource watchers were already filtered indirectly viaMPFileWatcher's internal check + theisWatchingstore gate, so this is clarity/symmetry/testability, not a behavior fix.)-[MPDocument startFileWatching]— moved[self stopFileWatching]to the top, above the nil/non-file-URL early return, so no early return can leak a previously-armed watcher.canWatchPath:header comment to describe all return-NOcases (nil/empty/non-local/unresolvable) and the parent-directory fallback.Notes on the issue framing
Two of the three "gaps" turned out to be milder than the issue body suggested, and I've documented that on the issue:
stopFileWatchingran before thecanWatchPath:return; only the nil-URL guard preceded it. The reorder closes that remaining edge.The substantive fix is Gap 2 (the atomic-save auto-reload regression).
Tests
Added to existing suites (no new files, so no Xcode target wiring needed):
MPFileWatcherTests:testCannotWatchEmptyPath,testCanWatchNonexistentLocalPath(the regression lock — fails against the old code),testCannotWatchPathWithNonexistentParent.MPResourceWatcherSetTests:testSkipsUnwatchablePath.Existing tests (
testInitWithNonexistentPath,testSkipsNonexistentFiles) still hold:canWatchPath:now returnsYESfor a non-existent-but-local path, butinitWithPath:'sopen(O_EVTONLY)still fails for a truly absent file, so nothing is falsely watched.Tests run on macOS CI only (per
CLAUDE.md); changes verified by code reading and a fresh-eyes review in this Linux environment.Manual Testing Plan
printf ... > tmp && mv tmp file.md)..mdfile on a remote/network volume (SMB/AFP share) and open it — the app should behave normally with live-watching simply skipped (no errors/hangs).Related Issue
Related to #478
Generated by Claude Code