Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion tests/hermes_cli/test_atomic_json_write.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import pytest

from utils import atomic_json_write
from utils import atomic_json_write, atomic_replace


class TestAtomicJsonWrite:
Expand Down Expand Up @@ -165,6 +165,28 @@ def test_mode_applied_when_supported(self, tmp_path):
actual = stat_mod.S_IMODE(target.stat().st_mode)
assert actual == 0o600

def test_atomic_replace_retries_transient_permission_error(self, tmp_path):
import utils

target = tmp_path / "data.json"
tmp_file = tmp_path / "data.tmp"
tmp_file.write_text('{"ok": true}', encoding="utf-8")
calls = 0
real_replace = os.replace

def flaky_replace(src, dst):
nonlocal calls
calls += 1
if calls < 3:
raise PermissionError("temporarily locked")
real_replace(src, dst)

with patch.object(utils.os, "replace", side_effect=flaky_replace):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only proves eventual success. When the implementation is scoped to Windows, explicitly simulate that branch here and add a separate persistent-failure assertion so the bounded retry still surfaces a real denial.

assert atomic_replace(tmp_file, target) == str(target)

assert calls == 3
assert json.loads(target.read_text(encoding="utf-8")) == {"ok": True}

def test_concurrent_writes_dont_corrupt(self, tmp_path):
"""Multiple rapid writes should each produce valid JSON."""
import threading
Expand Down
12 changes: 11 additions & 1 deletion utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import os
import stat
import tempfile
import time
from pathlib import Path
from typing import Any, Union
from urllib.parse import urlparse
Expand All @@ -15,6 +16,8 @@


TRUTHY_STRINGS = frozenset({"1", "true", "yes", "on"})
_ATOMIC_REPLACE_MAX_ATTEMPTS = 8
_ATOMIC_REPLACE_RETRY_DELAY = 0.01


def is_truthy_value(value: Any, default: bool = False) -> bool:
Expand Down Expand Up @@ -78,7 +81,14 @@ 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
os.replace(str(tmp_path), real_path)
for attempt in range(_ATOMIC_REPLACE_MAX_ATTEMPTS):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR rationale is Windows mandatory locking, but this loop retries every POSIX PermissionError too. Please scope the retry to the Windows path so a permanent POSIX permission denial still propagates immediately.

try:
os.replace(str(tmp_path), real_path)
break
except PermissionError:
if attempt == _ATOMIC_REPLACE_MAX_ATTEMPTS - 1:
raise
time.sleep(_ATOMIC_REPLACE_RETRY_DELAY * (attempt + 1))
return real_path


Expand Down
Loading