Address issue #371: bypass atomic safe-save on remote volumes#502
Merged
Conversation
NSDocument's default "safe save" (write to a temp file, then swap it in, plus an mtime-based conflict check) is unreliable on FUSE/network volumes such as SSHFS: the temp-file swap can fail outright, and mtime inconsistencies cause a spurious "changed by another application" conflict dialog even when nothing else touched the file. Choosing "Save Anyway" then hard-fails with "couldn't be saved in folder tmp". For destinations on non-local volumes, write directly instead of going through NSDocument's atomic safe-save path. This also stops the "volume does not support permanent version storage" prompt, since it's raised from within the same path being bypassed. Local-volume saves are unaffected. Extracted the existing NSURLVolumeIsLocalKey probing logic out of MPFileWatcher's canWatchPath: into a new pathIsOnLocalVolume: class method so MPDocument can reuse it for the save-path decision. Related to #371
Add a volumeLocalityChecker injection seam on MPDocument so tests can simulate a non-local save destination without a real network mount, letting the actual bypass branch (not just the local/no-op branches) be exercised in CI. Soften the versioning-prompt-suppression comment and changelog wording to note it's inferred from the call chain rather than directly observed, and note the deliberate trade-off of skipping NSFileCoordinator-mediated write coordination for non-local volumes. Related to #371
Contributor
Code Coverage ReportCurrent Coverage: 63.25% Coverage Details (Summary) |
schuyler
added a commit
that referenced
this pull request
Jul 1, 2026
## Summary Follow-up fix for issue #371. The original "spinning out of control" bug (mount collapse during vnode watching on remote/FUSE volumes) was already fixed in a prior PR by having `MPFileWatcher` skip watching non-local volumes. After validating that fix in an RC build, the reporter found a remaining problem: saving a markdown file on a remote/FUSE-mounted volume (e.g. SSHFS) triggers a spurious "The document could not be saved. The file has been changed by another application." conflict dialog, and choosing "Save Anyway" then hard-fails with "The file couldn't be saved in the folder 'tmp'." "Save As" also shows the "volume does not support permanent version storage" prompt. Root cause: `NSDocument`'s default "safe save" (`writeSafelyToURL:ofType:forSaveOperation:error:`) writes to a temp file and swaps it into place via `NSFileCoordinator`, plus checks the destination's on-disk modification date for conflicts. Both are unreliable on FUSE/network volumes — mtime semantics are inconsistent (false conflict positives), and the temp-file swap can fail outright. This PR: - Extracts the existing `NSURLVolumeIsLocalKey`-based locality probe out of `MPFileWatcher`'s `+canWatchPath:` into a new, reusable `+ (BOOL)pathIsOnLocalVolume:(NSString *)path` class method. - Overrides `MPDocument`'s `writeSafelyToURL:ofType:forSaveOperation:error:` to bypass NSDocument's atomic safe-save for non-local destinations (checked against the save destination, not the document's current URL, so cross-volume Save As is classified correctly) and write directly instead via the existing `writeToURL:ofType:forSaveOperation:originalContentsURL:error:` → `writeToURL:ofType:error:` path. This should also stop the "volume does not support permanent version storage" prompt, since it's raised from the same call chain being bypassed. - Adds a `volumeLocalityChecker` injection seam on `MPDocument` so the actual bypass-decision logic can be exercised in CI without a real network mount. - Local-volume saves are completely unaffected (fall through to `[super writeSafelyToURL:...]` unchanged). ## Files Changed - `MacDown/Code/Utility/MPFileWatcher.h` / `.m` — new `+pathIsOnLocalVolume:` class method. - `MacDown/Code/Document/MPDocument.m` — new `writeSafelyToURL:ofType:forSaveOperation:error:` override, `shouldBypassSafeSaveForURL:` helper, `volumeLocalityChecker` injection seam. - `MacDownTests/MPFileWatcherTests.m` — tests for `pathIsOnLocalVolume:`. - `MacDownTests/MPDocumentIOTests.m` — tests for the save-path bypass decision (including the true bypass branch, via the injection seam). - `CHANGELOG.md` — Unreleased/Fixed entry. ## Related Issue Related to #371 ## Manual Testing Plan A real FUSE/network mount isn't available in CI, so the following should be verified manually before/while validating this fix: 1. **Setup:** Mount a network/FUSE volume to reproduce the original bug conditions — SSHFS via macFUSE (`brew install --cask macfuse && brew install sshfs`), or an SMB/AFP share if macFUSE's system-extension approval is impractical (the fix uses a generic "is local volume" check, not SSHFS-specific detection, so SMB/AFP should behave the same way). 2. **Remote-volume fix:** - Open and edit a `.md` file on the mounted volume, save repeatedly (Cmd+S). Expected: no "changed by another application" dialog, no "couldn't be saved in folder tmp" error. - "Save As" a new document to the mounted volume. Expected: no "volume does not support permanent version storage" prompt. 3. **Local-volume regression:** - Normal open/edit/save on a local file: behavior unchanged, Versions/autosave still work (check via Finder "Browse All Versions"). - Genuine external-modification conflict on a **local** file (edit externally via Terminal while open in MacDown, then save): the "changed by another application" dialog should still appear — confirms the bypass is correctly scoped to non-local volumes only. 4. **Save As across volumes:** local → remote and remote → local, confirming each takes the correct path (direct write vs. atomic safe-save) based on the destination. 5. **Autosave-in-place:** confirm the fix holds when autosave triggers the save, not just manual Cmd+S. ## Review Notes - Reviewed by an architectural consult before implementation (confirmed `writeSafelyToURL:ofType:forSaveOperation:error:` is the correct override point, and that a locality check on the save destination — not `self.fileURL` — is required for correct Save As handling). - Reviewed by a code-review pass after initial implementation; feedback addressed by adding the `volumeLocalityChecker` injection seam (so the actual bypass branch has real test coverage, not just the local/no-op branches) and softening comments/changelog wording around the versioning-prompt suppression, which is inferred from the code path rather than directly observed (no real network mount available in CI). - A documentation pass found nothing in `plans/` that needed updating as a result of this change. --- _Generated by [Claude Code](https://claude.ai/code/session_01PxTBTWM3vQyy5bW8zu8YaG)_ --------- Co-authored-by: Claude <[email protected]>
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
Follow-up fix for issue #371. The original "spinning out of control" bug (mount collapse during vnode watching on remote/FUSE volumes) was already fixed in a prior PR by having
MPFileWatcherskip watching non-local volumes. After validating that fix in an RC build, the reporter found a remaining problem: saving a markdown file on a remote/FUSE-mounted volume (e.g. SSHFS) triggers a spurious "The document could not be saved. The file has been changed by another application." conflict dialog, and choosing "Save Anyway" then hard-fails with "The file couldn't be saved in the folder 'tmp'." "Save As" also shows the "volume does not support permanent version storage" prompt.Root cause:
NSDocument's default "safe save" (writeSafelyToURL:ofType:forSaveOperation:error:) writes to a temp file and swaps it into place viaNSFileCoordinator, plus checks the destination's on-disk modification date for conflicts. Both are unreliable on FUSE/network volumes — mtime semantics are inconsistent (false conflict positives), and the temp-file swap can fail outright.This PR:
NSURLVolumeIsLocalKey-based locality probe out ofMPFileWatcher's+canWatchPath:into a new, reusable+ (BOOL)pathIsOnLocalVolume:(NSString *)pathclass method.MPDocument'swriteSafelyToURL:ofType:forSaveOperation:error:to bypass NSDocument's atomic safe-save for non-local destinations (checked against the save destination, not the document's current URL, so cross-volume Save As is classified correctly) and write directly instead via the existingwriteToURL:ofType:forSaveOperation:originalContentsURL:error:→writeToURL:ofType:error:path. This should also stop the "volume does not support permanent version storage" prompt, since it's raised from the same call chain being bypassed.volumeLocalityCheckerinjection seam onMPDocumentso the actual bypass-decision logic can be exercised in CI without a real network mount.[super writeSafelyToURL:...]unchanged).Files Changed
MacDown/Code/Utility/MPFileWatcher.h/.m— new+pathIsOnLocalVolume:class method.MacDown/Code/Document/MPDocument.m— newwriteSafelyToURL:ofType:forSaveOperation:error:override,shouldBypassSafeSaveForURL:helper,volumeLocalityCheckerinjection seam.MacDownTests/MPFileWatcherTests.m— tests forpathIsOnLocalVolume:.MacDownTests/MPDocumentIOTests.m— tests for the save-path bypass decision (including the true bypass branch, via the injection seam).CHANGELOG.md— Unreleased/Fixed entry.Related Issue
Related to #371
Manual Testing Plan
A real FUSE/network mount isn't available in CI, so the following should be verified manually before/while validating this fix:
brew install --cask macfuse && brew install sshfs), or an SMB/AFP share if macFUSE's system-extension approval is impractical (the fix uses a generic "is local volume" check, not SSHFS-specific detection, so SMB/AFP should behave the same way)..mdfile on the mounted volume, save repeatedly (Cmd+S). Expected: no "changed by another application" dialog, no "couldn't be saved in folder tmp" error.Review Notes
writeSafelyToURL:ofType:forSaveOperation:error:is the correct override point, and that a locality check on the save destination — notself.fileURL— is required for correct Save As handling).volumeLocalityCheckerinjection seam (so the actual bypass branch has real test coverage, not just the local/no-op branches) and softening comments/changelog wording around the versioning-prompt suppression, which is inferred from the code path rather than directly observed (no real network mount available in CI).plans/that needed updating as a result of this change.Generated by Claude Code