fix(utils): retry transient Windows lock failures in atomic_replace - #79793
Closed
ruochu88s wants to merge 1 commit into
Closed
fix(utils): retry transient Windows lock failures in atomic_replace#79793ruochu88s wants to merge 1 commit into
ruochu88s wants to merge 1 commit into
Conversation
On Windows, `os.replace()` intermittently fails when another process holds a
short-lived handle on the destination without FILE_SHARE_DELETE. Antivirus
scanners, search indexers, cloud sync clients, and a just-closed editor all do
this. The call fails with one of:
WinError 5 (access denied)
WinError 32 (sharing violation)
WinError 33 (lock violation)
None of these are covered by the existing EXDEV/EBUSY copy fallback, so the
exception propagates and the write is lost even though a retry microseconds
later would succeed. Because `atomic_replace()` backs config writes, session
state, and credential updates, a background scanner touching the file at the
wrong moment surfaces as an unexplained save failure.
Fix: retry those three winerror codes with a bounded backoff
(50ms/100ms/200ms/400ms/800ms/1s, ~2.5s total). Retries are deliberately
bounded so a genuine ACL denial still propagates instead of hanging; the
predicate is also gated on os.name == "nt" so POSIX behavior is untouched.
EXDEV/EBUSY continue to take the existing copy fallback path, and symlink
resolution is unchanged.
Tests: 10 tests covering each retried code, exhaustion still raising,
non-transient errors propagating immediately, POSIX being unaffected, and
symlink targets still resolved. Verified the suite fails when the retry
predicate is reverted.
Collaborator
19 tasks
13 tasks
Collaborator
|
Superseded by #84852. Your winerror set The one gap: retry alone doesn't rescue a reader that outlives the budget (a desktop auth-init holds |
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
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.
What does this PR do?
On Windows,
os.replace()intermittently fails when another process holds ashort-lived handle on the destination without
FILE_SHARE_DELETE. Antivirusscanners, search indexers, cloud sync clients (OneDrive/Dropbox), and a
just-closed editor all do this. The call fails with one of:
None of these are covered by the existing
EXDEV/EBUSYcopy fallback, so theexception propagates and the write is lost — even though a retry a few
milliseconds later would succeed.
atomic_replace()backs config writes, session state, and credential updates,so a background scanner touching the file at the wrong moment surfaces to the
user as an unexplained save failure with a raw
WinErrorin the traceback.Why this approach
Retry only the three winerror codes that are genuinely transient, with a bounded
backoff (50ms/100ms/200ms/400ms/800ms/1s, ~2.5s total). Bounded is the important
part: a real ACL denial still propagates instead of hanging, which keeps a
permissions bug diagnosable rather than turning it into a stall.
The predicate is gated on
os.name == "nt"so POSIX behaviour is untouched, andEXDEV/EBUSYcontinue to take the existing copy-fallback path. Symlinkresolution is unchanged.
An alternative would be to widen the existing copy fallback to cover these
codes, but that changes replace semantics (the copy path is not atomic) for a
failure that is usually gone within milliseconds. Retrying preserves atomicity.
Related Issue
No existing issue — I searched open issues/PRs and found no report of this. Happy
to open a tracking issue first if maintainers prefer that order.
Type of Change
Changes Made
utils.py_WINDOWS_ATOMIC_REPLACE_RETRY_DELAYS— bounded backoff schedule._WINDOWS_TRANSIENT_REPLACE_ERRORS = frozenset({5, 32, 33})._is_transient_windows_replace_error()— gated onos.name == "nt"andthe
winerrorattribute, so it can never fire on POSIX.atomic_replace(): wrap theos.replace()call in a bounded retry loop,logging each retry at DEBUG. Existing
EXDEV/EBUSYhandling and symlinkresolution are unchanged.
tests/test_atomic_replace_symlinks.py— extend with the retry cases.Checklist
Code
fix(utils):)Coverage: each retried winerror code, retry exhaustion still raising, a
non-transient error propagating immediately without retries, POSIX being
unaffected, and symlink targets still resolved.
I verified the tests actually fail when the fix is reverted — emptying
_WINDOWS_TRANSIENT_REPLACE_ERRORSturns the suite red, confirming theassertions are not vacuous.
Documentation & Housekeeping
rationale is documented in an inline comment)
cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.md— N/Aos.name == "nt", so POSIX takes exactly the previous code pathScreenshots / Logs
With the fix, a transient lock is retried instead of surfacing: