Skip to content
Open
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
10 changes: 9 additions & 1 deletion python/aibrix/aibrix/storage/local.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import os
import shutil
import tempfile
import time
from datetime import datetime, timezone
from pathlib import Path
from typing import AsyncIterator, BinaryIO, Optional, TextIO, Union
Expand Down Expand Up @@ -173,7 +174,14 @@ def _write_file(self, path: Path, reader: Reader) -> None:
f.write(str(reader))
f.flush()
os.fsync(f.fileno())
os.replace(tmp_path, path)
for attempt in range(5):
try:
os.replace(tmp_path, path)
break
except PermissionError:
if attempt == 4:
raise
time.sleep(0.01 * (attempt + 1))
Comment on lines +181 to +184

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.

medium

On non-Windows platforms (like Linux/macOS), a PermissionError during os.replace is typically a permanent permission issue (e.g., write permission denied) rather than a transient sharing violation. Retrying in these cases introduces unnecessary delays (up to 100ms) before raising the exception.

We should restrict this retry behavior to Windows (os.name == 'nt').

Note: _write_json_file (line 553) also uses os.replace and could suffer from the same transient Windows failures. Consider extracting this retry logic into a shared helper function (e.g., _safe_replace) so both _write_file and _write_json_file can reuse it.

Suggested change
except PermissionError:
if attempt == 4:
raise
time.sleep(0.01 * (attempt + 1))
except PermissionError:
if os.name != "nt" or attempt == 4:
raise
time.sleep(0.01 * (attempt + 1))

Comment on lines +177 to +184
except Exception:
try:
os.close(fd)
Expand Down
19 changes: 19 additions & 0 deletions python/aibrix/tests/storage/test_local_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,25 @@ async def test_local_storage_initialization(self):
assert storage.base_path == Path(tmp_dir)
assert storage.base_path.exists()

def test_write_retries_when_destination_is_temporarily_locked(self, monkeypatch, tmp_path):
storage = LocalStorage(base_path=str(tmp_path))
target = tmp_path / "object"
attempts = 0
real_replace = os.replace

def replace_with_transient_lock(source, destination):
nonlocal attempts
attempts += 1
if attempts == 1:
raise PermissionError("destination is temporarily locked")
return real_replace(source, destination)

monkeypatch.setattr(os, "replace", replace_with_transient_lock)

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.

medium

To ensure this test passes on non-Windows platforms (where os.name is not 'nt'), we should mock os.name to 'nt' using monkeypatch so that the retry logic is triggered and tested correctly.

Suggested change
monkeypatch.setattr(os, "replace", replace_with_transient_lock)
monkeypatch.setattr(os, "name", "nt")
monkeypatch.setattr(os, "replace", replace_with_transient_lock)

storage._write_file(target, storage._wrap_data("content"))

assert target.read_text() == "content"
assert attempts == 2

@pytest.mark.asyncio
async def test_environment_variable_override(self):
"""Test that STORAGE_LOCAL_PATH environment variable is respected."""
Expand Down
Loading