Skip to content

Retry transient Windows atomic write failures - #2603

Open
medisean wants to merge 1 commit into
vllm-project:mainfrom
medisean:codex/retry-windows-atomic-writes
Open

Retry transient Windows atomic write failures#2603
medisean wants to merge 1 commit into
vllm-project:mainfrom
medisean:codex/retry-windows-atomic-writes

Conversation

@medisean

Copy link
Copy Markdown

Closes #2573

Summary:

  • Retry transient PermissionError failures during Windows atomic replacement.
  • Add regression coverage for a replacement that succeeds after retries.

Tests:

  • Python syntax compilation passed.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces retry logic in LocalStorage._write_file to handle transient PermissionError exceptions during os.replace operations, which typically occur on Windows, and adds a corresponding unit test. The review feedback suggests restricting this retry behavior to Windows platforms (os.name == 'nt') to avoid unnecessary delays on non-Windows systems where such errors are usually permanent, and recommends mocking os.name in the unit test to ensure it passes across all platforms.

Comment on lines +181 to +184
except PermissionError:
if attempt == 4:
raise
time.sleep(0.01 * (attempt + 1))

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))

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)

@varungup90

Copy link
Copy Markdown
Collaborator

Can you sign the PR

Copilot AI left a comment

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.

Pull request overview

Adds retries for transient Windows permission failures during atomic local-storage writes.

Changes:

  • Retries os.replace up to five times with incremental delays.
  • Adds regression coverage for a transient replacement failure.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
python/aibrix/aibrix/storage/local.py Adds retry logic for payload replacement.
python/aibrix/tests/storage/test_local_storage.py Tests successful replacement after one retry.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +177 to +184
for attempt in range(5):
try:
os.replace(tmp_path, path)
break
except PermissionError:
if attempt == 4:
raise
time.sleep(0.01 * (attempt + 1))
@Jeffwan
Jeffwan requested a review from zhangjyr September 4, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] LocalStorage atomic write fails intermittently on Windows when a reader holds the destination

3 participants