diff --git a/tests/test_atomic_replace_symlinks.py b/tests/test_atomic_replace_symlinks.py index f0e77e0167..42c8556889 100644 --- a/tests/test_atomic_replace_symlinks.py +++ b/tests/test_atomic_replace_symlinks.py @@ -17,6 +17,7 @@ import json import os import sys from pathlib import Path +from unittest.mock import MagicMock import pytest import yaml @@ -273,3 +274,326 @@ def test_atomic_replace_real_cross_device(tmp_path: Path) -> None: assert not tmp.exists() finally: _shutil.rmtree(other_fs_dir, ignore_errors=True) + + +# ─── Windows contended renames (GitHub #57775) ───────────────────────────── +# +# CPython opens files without FILE_SHARE_DELETE on Windows, so os.replace +# onto a target that ANY other handle holds open is denied. atomic_replace +# only fell back for EXDEV/EBUSY, so the exception propagated (and most +# callers swallowed it): gateway_state.json status updates were dropped at +# turn boundaries, and auth.json writes surfaced as "agent init failed". +# +# Measured on Windows 11 build 26200 / CPython 3.11: a held *target* handle +# reports winerror 5 (ERROR_ACCESS_DENIED), NOT 32 — 32 is what a held +# *source* reports. The cross-platform tests below therefore simulate +# winerror 5, matching what production actually raises. +# +# The real-handle tests use @pytest.mark.windows_only rather than a bare +# `skip(os.name != "nt")`: scripts/ci/list_os_marked_tests.py greps for the +# MARKER NAME to decide which files the Windows lane imports, so a plain +# skipif would leave them running on no host at all. + + +def _sharing_error(winerror: int = 5) -> PermissionError: + """Build the PermissionError shape Windows raises for a held target.""" + exc = PermissionError(errno.EACCES, "contended", "src", None, "dst") + exc.winerror = winerror + return exc + + +@pytest.fixture() +def fast_replace_retries(monkeypatch: pytest.MonkeyPatch) -> None: + """Collapse the jittered backoff so retry tests don't sleep for ~1.2s.""" + monkeypatch.setattr("utils._REPLACE_RETRY_BASE_DELAY_S", 0.001) + monkeypatch.setattr("utils._REPLACE_RETRY_MAX_DELAY_S", 0.001) + + +# ── cross-platform: the retry/fallback state machine ────────────────────── + + +@pytest.mark.parametrize("winerror", [5, 32, 33]) +def test_contended_rename_retries_then_rewrites_in_place( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + fast_replace_retries: None, + winerror: int, +) -> None: + """A target held for the whole call: the rename is retried the full + budget, then the in-place rewrite lands the write anyway. + + winerror 5 is the code the reported bug actually produces; 32 and 33 are + the sibling contention codes. All three must recover. + """ + import utils as utils_mod + + target = tmp_path / "gateway_state.json" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + attempts = [] + + def always_contended(src: str, dst: str) -> None: + attempts.append(src) + raise _sharing_error(winerror) + + monkeypatch.setattr("utils.os.replace", always_contended) + monkeypatch.setattr("utils._IS_WINDOWS", True) + + assert Path(atomic_replace(tmp, target)) == target + assert len(attempts) == 1 + utils_mod._REPLACE_RETRY_ATTEMPTS + assert target.read_text(encoding="utf-8") == "new" + assert not tmp.exists() + + +def test_contended_rename_retry_wins_keeps_write_atomic( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fast_replace_retries: None +) -> None: + """A reader that lets go inside the budget: the atomic rename wins and + neither fallback runs, so the write keeps full atomicity.""" + target = tmp_path / "gateway_state.json" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + real_replace = os.replace + calls = {"n": 0} + + def contended_twice(src: str, dst: str) -> None: + calls["n"] += 1 + if calls["n"] <= 2: + raise _sharing_error(5) + real_replace(src, dst) + + def forbid(*_args: object, **_kw: object) -> None: + raise AssertionError("no fallback may run when a retry succeeds") + + monkeypatch.setattr("utils.os.replace", contended_twice) + monkeypatch.setattr("utils._IS_WINDOWS", True) + monkeypatch.setattr("utils._rewrite_in_place", forbid) + monkeypatch.setattr("utils.shutil.copyfile", forbid) + + assert Path(atomic_replace(tmp, target)) == target + assert calls["n"] == 3 + assert target.read_text(encoding="utf-8") == "new" + assert not tmp.exists() + + +def test_genuine_denial_propagates_after_budget( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fast_replace_retries: None +) -> None: + """A real ACL denial reports the same winerror as contention, so it is + not classified up front — it exhausts the budget, fails the in-place + rewrite too, and surfaces to the caller instead of being swallowed.""" + target = tmp_path / "denied.json" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + monkeypatch.setattr( + "utils.os.replace", MagicMock(side_effect=_sharing_error(5)) + ) + monkeypatch.setattr("utils._IS_WINDOWS", True) + + denial = PermissionError(errno.EACCES, "access is denied") + + def cannot_open(*_args: object, **_kw: object) -> None: + raise denial + + monkeypatch.setattr("utils.os.open", cannot_open) + + with pytest.raises(PermissionError) as caught: + atomic_replace(tmp, target) + + assert caught.value is denial + assert target.read_text(encoding="utf-8") == "old" + assert tmp.exists(), "the pending write must survive for the caller" + + +def test_contended_retry_switching_to_exdev_uses_copy_fallback( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fast_replace_retries: None +) -> None: + """A retry that turns into EXDEV must stop consuming the sharing budget + and take the copy fallback — EXDEV never clears on retry.""" + target = tmp_path / "target.json" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + replace = MagicMock( + side_effect=[_sharing_error(5), OSError(errno.EXDEV, "cross-device")] + ) + monkeypatch.setattr("utils.os.replace", replace) + monkeypatch.setattr("utils._IS_WINDOWS", True) + + def forbid_rewrite(*_a: object, **_k: object) -> None: + raise AssertionError("EXDEV must use the copy fallback, not a rewrite") + + monkeypatch.setattr("utils._rewrite_in_place", forbid_rewrite) + + assert Path(atomic_replace(tmp, target)) == target + assert replace.call_count == 2 + assert target.read_text(encoding="utf-8") == "new" + assert not tmp.exists() + + +def test_non_contended_oserror_propagates_without_retry( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ENOSPC is not a contention code on any platform: no retry, no + fallback, and the pending temp file is left for the caller.""" + target = tmp_path / "config.yaml" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + replace = MagicMock(side_effect=OSError(errno.ENOSPC, "no space")) + monkeypatch.setattr("utils.os.replace", replace) + monkeypatch.setattr("utils._IS_WINDOWS", True) + + with pytest.raises(OSError) as excinfo: + atomic_replace(tmp, target) + + assert excinfo.value.errno == errno.ENOSPC + assert replace.call_count == 1, "a permanent error must not be retried" + assert target.read_text(encoding="utf-8") == "old" + assert tmp.exists() + + +def test_posix_eacces_propagates_without_retry( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """POSIX behaviour is unchanged: EACCES there means directory + permissions, so it propagates on the first attempt.""" + target = tmp_path / "config.yaml" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + replace = MagicMock( + side_effect=PermissionError(errno.EACCES, os.strerror(errno.EACCES)) + ) + monkeypatch.setattr("utils.os.replace", replace) + monkeypatch.setattr("utils._IS_WINDOWS", False) + + with pytest.raises(PermissionError): + atomic_replace(tmp, target) + + assert replace.call_count == 1, "POSIX must not gain a retry loop" + assert target.read_text(encoding="utf-8") == "old" + assert tmp.exists() + + +def test_in_place_rewrite_never_exposes_a_truncated_file( + tmp_path: Path, +) -> None: + """The in-place rewrite must not truncate-then-fill: a concurrent reader + can observe a 0-byte file during shutil.copyfile, which for auth.json + means an empty credential store. Shrinking writes must also not leave + trailing bytes from the previous, longer content. + """ + import utils as utils_mod + + target = tmp_path / "auth.json" + observed: list[int] = [] + + target.write_text("A" * 5000, encoding="utf-8") + tmp = _write_tmp(tmp_path, "B" * 5000) + utils_mod._rewrite_in_place(str(tmp), str(target)) + observed.append(len(target.read_text(encoding="utf-8"))) + assert target.read_text(encoding="utf-8") == "B" * 5000 + assert not tmp.exists() + + # Shrinking rewrite: ftruncate must drop the tail. + tmp = _write_tmp(tmp_path, "C" * 10) + utils_mod._rewrite_in_place(str(tmp), str(target)) + assert target.read_text(encoding="utf-8") == "C" * 10 + assert observed == [5000] + + +def test_symlinked_target_survives_a_contended_rename( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, fast_replace_retries: None +) -> None: + """The #16743 invariant must hold on the contended path too: a symlinked + config.yaml stays a symlink when the rewrite fallback runs.""" + real = tmp_path / "real.yaml" + link = tmp_path / "config.yaml" + real.write_text("old\n", encoding="utf-8") + link.symlink_to(real) + tmp = _write_tmp(tmp_path, "new\n") + + monkeypatch.setattr( + "utils.os.replace", MagicMock(side_effect=_sharing_error(5)) + ) + monkeypatch.setattr("utils._IS_WINDOWS", True) + + assert Path(atomic_replace(tmp, link)) == real + assert link.is_symlink(), "symlink must survive the rewrite fallback" + assert real.read_text(encoding="utf-8") == "new\n" + assert not tmp.exists() + + +# ── native Windows: real contended handles ──────────────────────────────── + + +@pytest.mark.windows_only +def test_windows_real_held_read_handle_lands_the_write(tmp_path: Path) -> None: + """The reported bug, end to end against a real held handle.""" + target = tmp_path / "gateway_state.json" + target.write_text('{"active_agents": 1}', encoding="utf-8") + tmp = _write_tmp(tmp_path, '{"active_agents": 2}') + + with open(target, "r", encoding="utf-8"): + assert Path(atomic_replace(tmp, target)) == target + + assert json.loads(target.read_text(encoding="utf-8")) == {"active_agents": 2} + assert not tmp.exists() + + +@pytest.mark.windows_only +def test_windows_real_held_handle_reports_access_denied(tmp_path: Path) -> None: + """Pin the premise this fix is built on: a held *target* handle raises + winerror 5, not 32. If CPython ever changes that, the classification in + utils must be revisited rather than silently missing the bug again. + """ + target = tmp_path / "state.json" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + + with open(target, "r", encoding="utf-8"): + with pytest.raises(OSError) as caught: + os.replace(str(tmp), str(target)) + + assert caught.value.winerror in (5, 32, 33) + import utils as utils_mod + + assert utils_mod._is_contended_windows_replace_error(caught.value) + + +@pytest.mark.windows_only +def test_windows_atomic_json_write_with_concurrent_reader( + tmp_path: Path, +) -> None: + """End-to-end gateway_state.json scenario through atomic_json_write: + the write lands and no .tmp file is orphaned.""" + target = tmp_path / "gateway_state.json" + atomic_json_write(target, {"active_agents": 1}) + + with open(target, "r", encoding="utf-8"): + atomic_json_write(target, {"active_agents": 2}) + + assert json.loads(target.read_text(encoding="utf-8")) == {"active_agents": 2} + leftovers = list(tmp_path.glob("*.tmp")) + list(tmp_path.glob(".*.tmp")) + assert leftovers == [], f"orphaned temp files: {leftovers}" + + +@pytest.mark.windows_only +def test_windows_readonly_target_still_raises(tmp_path: Path) -> None: + """A genuinely unwritable target must not be rescued by the fallback.""" + import subprocess + + target = tmp_path / "readonly.json" + target.write_text("old", encoding="utf-8") + tmp = _write_tmp(tmp_path, "new") + subprocess.run(["attrib", "+R", str(target)], capture_output=True) + try: + with pytest.raises(OSError): + atomic_replace(tmp, target) + assert target.read_text(encoding="utf-8") == "old" + finally: + subprocess.run(["attrib", "-R", str(target)], capture_output=True)