From 017364025b83f6525a24774eb351ba8b37bc5442 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 30 Jun 2026 05:18:42 +0000 Subject: [PATCH] Address issue #478: harden file watching against transient paths and 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 --- CHANGELOG.md | 1 + MacDown/Code/Document/MPDocument.m | 7 ++++-- MacDown/Code/Utility/MPFileWatcher.h | 6 ++++- MacDown/Code/Utility/MPFileWatcher.m | 19 +++++++++++++-- MacDown/Code/Utility/MPResourceWatcherSet.m | 6 +++++ MacDownTests/MPFileWatcherTests.m | 26 +++++++++++++++++++++ MacDownTests/MPResourceWatcherSetTests.m | 9 +++++++ 7 files changed, 69 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79cf01a9..e5cd6d28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ### Fixed - Fix the Insert Table toolbar button doing nothing when the editor pane had focus, and corrupting the document when clicked repeatedly (a second table was inserted inside the first table's cell); every click now inserts a clean, well-separated table regardless of which pane has focus (#278) -- thanks @rcuisnier for the report! +- Fix auto-reload silently breaking after an external editor's atomic save by making the local-volume watcher check fall back to the parent directory when the file is transiently missing; also guard resource-file watchers against remote volumes and tear down any prior watcher before re-arming (#478) - Fix the Preview pane still auto-scrolling toward the editor when typing after Sync Panes was turned off mid-session; toggling Sync Panes now takes effect immediately (settling the panes on disable, re-syncing on enable) without needing to reopen the document (#441) -- thanks @gregwillits! - Fix blank preview when opening saved or externally-originated documents: the real document file is no longer used as the preview's base resource, which WebKit on macOS 26 can silently refuse to load (e.g. files with the execute bit set by sync clients like OneDrive, or stale TCC/provenance state) (#431, #405) -- thanks @maskedspitz, @songjianbupt, and @craigrodger for the diagnosis! - Improve render-path responsiveness: single-pass word/character counting, cached body-extraction regex, and bounded renderer polling with cancellation (#388) diff --git a/MacDown/Code/Document/MPDocument.m b/MacDown/Code/Document/MPDocument.m index 930e1565..92511181 100644 --- a/MacDown/Code/Document/MPDocument.m +++ b/MacDown/Code/Document/MPDocument.m @@ -3856,11 +3856,14 @@ + (NSString *)toggleCheckboxAtIndex:(NSUInteger)index inMarkdown:(NSString *)mar - (void)startFileWatching { + // Tear down any previously-armed watcher first, so an early return below + // (nil URL, or a path that cannot be watched) can never leak a stale + // watcher for an old session. Related to #478. + [self stopFileWatching]; + if (!self.fileURL || !self.fileURL.isFileURL) return; - [self stopFileWatching]; - if (![MPFileWatcher canWatchPath:self.fileURL.path]) return; diff --git a/MacDown/Code/Utility/MPFileWatcher.h b/MacDown/Code/Utility/MPFileWatcher.h index f51022e2..304ea4be 100644 --- a/MacDown/Code/Utility/MPFileWatcher.h +++ b/MacDown/Code/Utility/MPFileWatcher.h @@ -16,7 +16,11 @@ /// YES if currently watching. @property (nonatomic, readonly, getter=isWatching) BOOL watching; -/// YES if the path is on a local volume that should receive vnode watchers. +/// YES if the path can receive vnode watchers: it is non-nil, non-empty, and +/// resides on a local volume. When the file itself does not exist (e.g. during +/// an external editor's atomic rename-replace save) the locality check falls +/// back to the parent directory. Returns NO for nil, empty, non-local, or +/// otherwise unresolvable paths. + (BOOL)canWatchPath:(NSString *)path; /// Create a watcher for the given path. Calls handler on the main queue diff --git a/MacDown/Code/Utility/MPFileWatcher.m b/MacDown/Code/Utility/MPFileWatcher.m index dd3b1170..026304df 100644 --- a/MacDown/Code/Utility/MPFileWatcher.m +++ b/MacDown/Code/Utility/MPFileWatcher.m @@ -21,11 +21,26 @@ + (BOOL)canWatchPath:(NSString *)path if (!path.length) return NO; - NSURL *url = [NSURL fileURLWithPath:path]; + // NSURLVolumeIsLocalKey can only be read from a URL that resolves to an + // existing item. During an external editor's atomic rename-replace save the + // file transiently disappears, so probing the file itself would wrongly + // report the path as non-local and disable auto-reload. Fall back to the + // parent directory's volume in that case. Related to #478. + NSFileManager *fileManager = [NSFileManager defaultManager]; + NSString *probePath = path; + if (![fileManager fileExistsAtPath:probePath]) + probePath = [path stringByDeletingLastPathComponent]; + if (!probePath.length || ![fileManager fileExistsAtPath:probePath]) + return NO; + + NSURL *url = [NSURL fileURLWithPath:probePath]; NSNumber *isLocal = nil; + NSError *error = nil; if (![url getResourceValue:&isLocal - forKey:NSURLVolumeIsLocalKey error:NULL]) + forKey:NSURLVolumeIsLocalKey error:&error]) { + NSLog(@"MPFileWatcher: cannot determine volume locality for %@: %@", + probePath, error.localizedDescription); return NO; } diff --git a/MacDown/Code/Utility/MPResourceWatcherSet.m b/MacDown/Code/Utility/MPResourceWatcherSet.m index de98e55c..f38b99d2 100644 --- a/MacDown/Code/Utility/MPResourceWatcherSet.m +++ b/MacDown/Code/Utility/MPResourceWatcherSet.m @@ -57,6 +57,12 @@ - (void)updateWatchedPaths:(NSSet *)paths - (void)addWatcherForPath:(NSString *)path { + // Skip paths that cannot receive vnode watchers (nil/empty or on a remote + // volume) before constructing a watcher, mirroring the guard in + // -[MPDocument startFileWatching]. Related to #478. + if (![MPFileWatcher canWatchPath:path]) + return; + __weak MPResourceWatcherSet *weakSelf = self; NSString *watchedPath = [path copy]; diff --git a/MacDownTests/MPFileWatcherTests.m b/MacDownTests/MPFileWatcherTests.m index bd0e916d..ad922f7e 100644 --- a/MacDownTests/MPFileWatcherTests.m +++ b/MacDownTests/MPFileWatcherTests.m @@ -87,6 +87,32 @@ - (void)testCannotWatchNilPath XCTAssertFalse([MPFileWatcher canWatchPath:nil]); } +- (void)testCannotWatchEmptyPath +{ + XCTAssertFalse([MPFileWatcher canWatchPath:@""]); +} + +- (void)testCanWatchNonexistentLocalPath +{ + // A file that does not yet exist (e.g. mid atomic rename-replace save) on a + // local volume must still be considered watchable: the locality check falls + // back to the parent directory. Otherwise auto-reload silently dies during + // an external editor's save. Related to #478. + NSString *path = [self.testDirectory stringByAppendingPathComponent:@"gone.txt"]; + XCTAssertFalse([self.fileManager fileExistsAtPath:path]); + XCTAssertTrue([MPFileWatcher canWatchPath:path]); +} + +- (void)testCannotWatchPathWithNonexistentParent +{ + // Neither the file nor its parent directory exists -> nothing to probe for + // volume locality, so watching is not possible. + NSString *path = [[self.testDirectory + stringByAppendingPathComponent:@"missing-dir"] + stringByAppendingPathComponent:@"file.txt"]; + XCTAssertFalse([MPFileWatcher canWatchPath:path]); +} + #pragma mark - Event Handling - (void)testHandlerCalledOnFileWrite diff --git a/MacDownTests/MPResourceWatcherSetTests.m b/MacDownTests/MPResourceWatcherSetTests.m index 439fd317..b71ed618 100644 --- a/MacDownTests/MPResourceWatcherSetTests.m +++ b/MacDownTests/MPResourceWatcherSetTests.m @@ -122,6 +122,15 @@ - (void)testSkipsNonexistentFiles XCTAssertEqual(self.watcherSet.watchedPaths.count, 0u); } +- (void)testSkipsUnwatchablePath +{ + // An unwatchable path (here an empty string, but in practice a remote + // volume) must be rejected before a watcher is constructed and never + // stored. Related to #478. + [self.watcherSet updateWatchedPaths:[NSSet setWithObject:@""]]; + XCTAssertEqual(self.watcherSet.watchedPaths.count, 0u); +} + - (void)testDelegateCalledOnChange { NSString *path = [self createTestFileWithName:@"watch.png"];