{{ message }}
Repository navigation
fix(desktop): retry a save whose rename an SMB share refused (#5322) - #5520
Merged
Merged
Conversation
On a Windows SMB share, MoveFileEx(REPLACE_EXISTING) — what fs.rename compiles to, and the last step of the atomic save — is refused with ACCESS_DENIED, surfaced as EPERM, while any handle is open on the target. MarkText's own file watcher stats the open document, so a save races its own watcher: the temp file is written, the rename over the document is rejected, and the edit is lost. Measured on a real Windows SMB share (loopback share, windows-latest): one open read handle held across every save 50/50 renames refused watcher stat storm at 1ms (~12000 stats) 102/300 refused watcher polling as the app configures it 1/200 refused no handle open at all 0/50 refused The same probes against Samba from a Linux client never fail, so this is the Windows SMB redirector's share-mode check, not a server-side one. This is a 0.19 -> 0.20 regression, and not from #4778 as the issue assumed. 0.19.x wrote the document in place with fs-extra's outputFile and never renamed, so it could not hit this. #4852 introduced the temp+rename atomic save (for #3786/#3828), which is what races; #4778 then forced usePolling for UNC paths, which raised the stat rate enough to turn an occasional lost save into a reliable one. Reverting #4778 would only push the failure back below the reporter's notice — it would not fix it, and it would leave mapped network drives (Z:\, not a UNC path, so never force-polled) failing occasionally either way. The handle is released within milliseconds, so back off and try the save again instead of dropping the edit. Retrying re-runs the whole atomic write, which is safe: write-file-atomic cleans up its temp file on failure. Three retries at 50/150/300ms take the 1ms-stat-storm failure rate from 102/300 to 3/300; at the app's real stat rate the remaining window is far smaller still. A process that holds a handle open continuously (an antivirus scanner, a sync client) is out of reach of any retry — that save still reports mt::tab-save-failure and the tab stays dirty, rather than silently losing the edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Build artifacts for PR #5520: Run: https://github.com/marktext/marktext/actions/runs/35808544485 |
A follow-up probe showed the earlier explanation was wrong on two counts: MoveFileEx(REPLACE_EXISTING) is refused for an open target on local NTFS just as on a share (50/50 either way), and asking for FILE_SHARE_DELETE makes no difference (err=5 with and without it). What separates the two is how long the handle stays open - microseconds for a local stat, a network round trip for one over SMB - so only shares lose the race in practice. The retry itself is unchanged; only the reasoning it records. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The watcher stats on a fixed period, so a retry only helps if its cumulative offset lands on a phase the stat is not holding. 50/150/300 accumulates to 50/200/500 - the last two are multiples of chokidar's 100ms interval, i.e. exactly the phase that just failed, so only the first retry was ever doing anything. Simulated against both periods the app uses (100ms watchFile, 150ms awaitWriteFinish), that loses 33/57/75% of first failures once a stat holds the file for 60/70/80ms. 50/75/50 accumulates to 50/125/175 - three distinct phases - and clears every first failure up to a ~70ms stat, while finishing in 175ms instead of 500ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #5322
Summary
Saving a document opened from a Windows network share fails at the atomic save's
renamewithEPERM: the temp file is written, the rename over the document is refused, and the edit is lost.fs.renamecompiles toMoveFileEx(REPLACE_EXISTING), which Windows refuses withACCESS_DENIED(libuv surfaces it asEPERM) whenever any handle is open on the target. MarkText's own file watcherstats the open document, so a save races its own watcher.This backs off and retries the save three times (50/150/300 ms) when the failure is one a retry can clear (
EPERM/EACCES/EBUSY). Retrying re-runs the whole atomic write, which is safe —write-file-atomicremoves its temp file on failure.What actually decides whether it fails
Measured on
windows-latest, against one folder reached two ways — by its local path and through a loopback SMB share:usePolling: true)usePolling: false— i.e. rc.1's behaviourAnd with
MoveFileExcalled directly, controlling the share mode the holder asks for:So the refusal is not specific to SMB, and does not depend on
FILE_SHARE_DELETE: an open handle blocks the replace on local NTFS just the same. What separates the two is how long the handle stays open — astatcosts microseconds locally but a network round trip over SMB, so only on a share does the watcher's polling keep landing inside the rename. That also explains the reporter's "every save fails" against my 1/200 on a loopback share: a real LAN round trip is far slower again, so the window is proportionally wider.The same probes run against Samba from a Linux client never fail, because POSIX rename semantics allow replacing an open file at all.
The root cause is not #4778
The issue assumed the regression came from #4778 forcing
usePollingfor UNC paths.usePolling: falsefails too, at the same rate — polling only raises thestatrate, and the failure rate tracks that rate. It amplifies the race; it does not cause it.The actual regression is 0.19 → 0.20:
fs-extra'soutputFile) and never renamed, so it could not hit this at all.usePollingfor UNC paths, raising the stat rate enough to turn an occasional lost save into a reliable one, which is when it got noticed.Two consequences worth stating: reverting #4778 would only push the failure back below the reporter's threshold rather than fix it, and the blast radius is wider than UNC — a network drive mapped to a letter (
Z:\) is not a UNC path, so it is never force-polled, yet it fails the same way.Verification
Three retries take the 1 ms-stat-storm failure rate from 102/300 to 3/300; at the rate the app actually stats, the remaining window is far smaller again. A process that holds a handle open continuously (an antivirus scanner, a sync client) is out of reach of any retry — and, per the local-path row above, that applies on a local disk too. Those saves still report
mt::tab-save-failureand the tab stays dirty, so the edit is never silently lost.The measurements came from a throwaway harness on a temporary branch (loopback
New-SmbShareonwindows-latest, Samba onubuntu-latest, and aMoveFileExP/Invoke probe); it is not part of this PR.Type of change
Test plan
packages/desktop/test/unit/specs/write-file-smb-retry.spec.ts— retries a refused rename and saves; gives up bounded; does not retry a failure retrying cannot clear (ENOSPC).write-file-atomic.spec.ts/write-file-missing-dir.spec.tsstill pass.🤖 Generated with Claude Code