From 92f74b3429046de8a0f268eed6426f585fab7441 Mon Sep 17 00:00:00 2001 From: ruochu88s <248221513+ruochu88s@users.noreply.github.com> Date: Thu, 6 Aug 2026 07:47:12 +0800 Subject: [PATCH] fix(utils): retry transient Windows lock failures in atomic_replace 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. --- tests/test_atomic_replace_symlinks.py | 73 +++++++++++++++++++++++++ utils.py | 79 ++++++++++++++++++++------- 2 files changed, 131 insertions(+), 21 deletions(-) diff --git a/tests/test_atomic_replace_symlinks.py b/tests/test_atomic_replace_symlinks.py index 84f60bab31cc..17b21cfef54a 100644 --- a/tests/test_atomic_replace_symlinks.py +++ b/tests/test_atomic_replace_symlinks.py @@ -21,6 +21,8 @@ import pytest import yaml +import utils + # Ensure the repo root is importable when running via `pytest tests/...`. _REPO_ROOT = Path(__file__).resolve().parent.parent if str(_REPO_ROOT) not in sys.path: @@ -88,6 +90,77 @@ def test_atomic_replace_accepts_pathlike_and_str(tmp_path: Path) -> None: assert target.read_text(encoding="utf-8") == "2" +def _windows_lock_error(winerror: int) -> PermissionError: + exc = PermissionError(errno.EACCES, "destination is temporarily locked") + exc.winerror = winerror + return exc + + +def test_windows_transient_replace_error_classification(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(utils.os, "name", "nt") + + for winerror in (5, 32, 33): + assert utils._is_transient_windows_replace_error(_windows_lock_error(winerror)) + + assert not utils._is_transient_windows_replace_error(_windows_lock_error(2)) + + +def test_atomic_replace_retries_transient_windows_lock( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "target.yaml" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + real_replace = os.replace + attempts = 0 + sleeps: list[float] = [] + + def flaky_replace(src: str, dst: str) -> None: + nonlocal attempts + attempts += 1 + if attempts < 3: + raise _windows_lock_error(5) + real_replace(src, dst) + + monkeypatch.setattr(utils, "_is_transient_windows_replace_error", lambda _exc: True) + monkeypatch.setattr(utils.os, "replace", flaky_replace) + monkeypatch.setattr(utils.time, "sleep", sleeps.append) + + atomic_replace(tmp, target) + + assert attempts == 3 + assert sleeps == [0.05, 0.1] + assert target.read_text(encoding="utf-8") == "new" + + +def test_atomic_replace_reraises_after_windows_retry_budget( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "target.yaml" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + attempts = 0 + + def always_locked(_src: str, _dst: str) -> None: + nonlocal attempts + attempts += 1 + raise _windows_lock_error(32) + + monkeypatch.setattr(utils, "_WINDOWS_ATOMIC_REPLACE_RETRY_DELAYS", (0.0, 0.0)) + monkeypatch.setattr(utils, "_is_transient_windows_replace_error", lambda _exc: True) + monkeypatch.setattr(utils.os, "replace", always_locked) + monkeypatch.setattr(utils.time, "sleep", lambda _delay: None) + + with pytest.raises(PermissionError): + atomic_replace(tmp, target) + + assert attempts == 3 + assert target.read_text(encoding="utf-8") == "old" + assert tmp.exists(), "failed replacement must leave the temp file for caller cleanup" + + # ─── atomic_json_write / atomic_yaml_write wiring ────────────────────────── diff --git a/utils.py b/utils.py index 66d2f82be56e..a44d6d445878 100644 --- a/utils.py +++ b/utils.py @@ -7,6 +7,7 @@ import shutil import stat import tempfile +import time from pathlib import Path from typing import Any, Union from urllib.parse import urlparse @@ -16,6 +17,20 @@ logger = logging.getLogger(__name__) +# Windows can briefly deny replace/rename while antivirus, indexers, sync clients, +# or a just-closed editor still holds the destination without FILE_SHARE_DELETE. +# Keep retries bounded so a real ACL denial still surfaces to the caller. +_WINDOWS_ATOMIC_REPLACE_RETRY_DELAYS = (0.05, 0.1, 0.2, 0.4, 0.8, 1.0) +_WINDOWS_TRANSIENT_REPLACE_ERRORS = frozenset({5, 32, 33}) + + +def _is_transient_windows_replace_error(exc: OSError) -> bool: + return ( + os.name == "nt" + and getattr(exc, "winerror", None) in _WINDOWS_TRANSIENT_REPLACE_ERRORS + ) + + TRUTHY_STRINGS = frozenset({"1", "true", "yes", "on"}) @@ -111,28 +126,50 @@ def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: target_str = str(target) real_path = os.path.realpath(target_str) if os.path.islink(target_str) else target_str tmp_str = str(tmp_path) - try: - os.replace(tmp_str, real_path) - except OSError as exc: - if exc.errno not in (errno.EXDEV, errno.EBUSY): - raise - logger.debug( - "atomic_replace: %s -> %s failed with %s; falling back to copy", - tmp_str, - real_path, - errno.errorcode.get(exc.errno, exc.errno), - ) - shutil.copyfile(tmp_str, real_path) - try: - shutil.copystat(tmp_str, real_path) - except OSError: - pass + retry_index = 0 + while True: try: - with open(real_path, "rb") as f: - os.fsync(f.fileno()) - except OSError: - pass - os.unlink(tmp_str) + os.replace(tmp_str, real_path) + break + except OSError as exc: + if ( + _is_transient_windows_replace_error(exc) + and retry_index < len(_WINDOWS_ATOMIC_REPLACE_RETRY_DELAYS) + ): + delay = _WINDOWS_ATOMIC_REPLACE_RETRY_DELAYS[retry_index] + retry_index += 1 + logger.debug( + "atomic_replace: transient Windows lock %s for %s -> %s; " + "retrying in %.2fs (%d/%d)", + getattr(exc, "winerror", None), + tmp_str, + real_path, + delay, + retry_index, + len(_WINDOWS_ATOMIC_REPLACE_RETRY_DELAYS), + ) + time.sleep(delay) + continue + if exc.errno not in (errno.EXDEV, errno.EBUSY): + raise + logger.debug( + "atomic_replace: %s -> %s failed with %s; falling back to copy", + tmp_str, + real_path, + errno.errorcode.get(exc.errno, exc.errno), + ) + shutil.copyfile(tmp_str, real_path) + try: + shutil.copystat(tmp_str, real_path) + except OSError: + pass + try: + with open(real_path, "rb") as f: + os.fsync(f.fileno()) + except OSError: + pass + os.unlink(tmp_str) + break return real_path