vllm-project / vllm-project/aibrix

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

Open
#2,573 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
5.1k
Forks
694
Avg merge
1d 19h
Merged PRs (30d)
98

Description

## Summary

`LocalStorage`'s atomic write fails intermittently on Windows with `WinError 5` when another coroutine has the destination open for reading. The write path itself is correct; the incompatibility is that a POSIX `rename` over an open destination always succeeds, and the Windows equivalent does not.

This surfaces as flaky `local_metastore_job` failures in `tests/batch/test_e2e_abnormal_job_behavior.py`, with a different test failing on each run.

## Environment

- `main` at `b36d2a4`
- Windows 11, Python 3.12
- Component: `python/aibrix`, `aibrix/storage/local.py`

## What happens

```
[WinError 5] Access is denied:
'...\pytest-588\test_job_restore_after_mds_cra1\.metastore\.batchjob_meta%3Aoldest_unfinished_created_at.ggr_8a32.tmp'
-> '.metastore\batchjob_meta%3Aoldest_unfinished_created_at'
```

Three runs of `pytest tests/batch`, same checkout:

```
run 1 test_job_cancellation_in_progress_before_preparation
test_job_restore_after_mds_crash_during_finalizing_after_runtime_shutdown
run 2 test_job_expiration_in_finalizing
test_job_restore_after_mds_crash_during_in_progress
test_job_restore_after_mds_crash_during_finalizing_after_runtime_shutdown
run 3 test_job_restore_after_mds_crash_during_in_progress (file run alone: 1 failed, 14 passed)
```

Different tests each time, which is what pointed at a race rather than a broken assertion.

## Why

`_write_file` is careful and is not the problem. The temp file is closed before the swap, because the `os.fdopen` context manager exits first:

```python
# aibrix/storage/local.py:160
fd, tmp_path = tempfile.mkstemp(dir=str(path.parent), prefix=f".{path.name}.", suffix=".tmp")
try:
with os.fdopen(fd, "wb") as f:
f.write(bytes(reader)); f.flush(); os.fsync(f.fileno())
os.replace(tmp_path, path)
```

The destination is the issue. Reads use a plain `open`:

```python
# local.py:269
with open(path, "rb") as f:
# local.py:561
with open(path, "r") as f:
```

CPython's `open` does not pass `FILE_SHARE_DELETE` on Windows, so while any reader holds the file, `MoveFileEx(..., MOVEFILE_REPLACE_EXISTING)` under `os.replace` returns `ERROR_ACCESS_DENIED`. On Linux the same `rename(2)` unlinks the old inode and readers keep their open handle, so the swap is invisible and always succeeds.

`batchjob_meta:oldest_unfinished_created_at` is a good place to hit it: it is a shared aggregate that several jobs read and rewrite, so read and replace overlap readily.

To be clear about confidence: the platform behaviour, the failing call, and the flakiness are measured. The specific overlapping reader is inferred from the access pattern rather than caught in the act, so treat that part as the likely mechanism rather than a proven trace.

## Expected behavior

`LocalStorage` is the default substrate for local development and for the `local_metastore_job` test path, so it should behave the same on both platforms.

## Possible directions

I have no preference, and each has a real cost:

Retry the `os.replace` briefly on `PermissionError`. Smallest change, and it matches how most Python projects handle this, but it turns a correctness gap into a timing one.

Open readers with sharing that permits deletion, via `os.open` with `O_BINARY` plus a `msvcrt`/`ctypes` share mode, or a small platform shim. Correct, but it introduces Windows-specific file opening into a currently portable module.

Declare Windows unsupported for `LocalStorage` and skip these tests there. Honest and cheap, if that is the intent. There is no marker today, so a Windows contributor currently reads this as a broken checkout.

Happy to send a PR for whichever you prefer.

## Related

`#2572` fixes a separate Windows-only failure in the same test tree, a `mkstemp` descriptor left open in `test_driver.py`. That one is a test bug; this one is not.

Contributor guide

Open the contributing guide

Research direction

Start with _write_file and the reader call sites in python/aibrix/storage/local.py, then reproduce the race with the affected tests in tests/batch/test_e2e_abnormal_job_behavior.py on Windows. Review the proposed retry, sharing, and unsupported-platform directions with maintainers before choosing an approach. Done means the local_metastore_job tests no longer fail intermittently when a reader overlaps an atomic write.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.