Repository navigation
Python: retry atomic session snapshot replacement on transient PermissionError - #10200
Simin Yuan (simin-yuan) wants to merge 3 commits into
Conversation
…sionError os.replace() raises PermissionError on Windows when the destination file is briefly held open, even though FileSessionStore already serializes writes per snapshot. Concurrent saves therefore fail instead of following the documented last-writer-wins behaviour. Retry the replacement a bounded number of times with a short delay, on Windows only, and keep propagating the original error once the retries are exhausted. Fixes microsoft#10137
There was a problem hiding this comment.
🟡 Changes recommended
Persistent failure with Windows retries enabled lacks test coverage for three replacement attempts, PermissionError propagation, snapshot preservation, temporary-file cleanup, and 10/20 ms waits.
1 open issue
What changed in this PR
This PR addresses #10137 by adding bounded retries for transient Windows snapshot replacement failures in FileSessionStore, while preserving the existing per-file lock and POSIX behavior.
Changes:
- Retry Windows
PermissionErrorfailures for up to three total attempts, with 10 ms and 20 ms waits. - Add tests for transient recovery and immediate failure when retries are disabled.
| File | Description |
|---|---|
python/packages/core/tests/core/test_sessions.py |
Tests retry recovery, snapshot preservation, and platform-gated behavior. |
python/packages/core/agent_framework/_sessions.py |
Adds the Windows-only retry helper and uses it during atomic snapshot writes. |
Review effort: Balanced
|
Eduard van Valkenburg (@eavanvalkenburg) thanks for the approval. On my side GitHub still reports |
Three attempts, PermissionError propagation, prior snapshot preserved, temp file cleaned, and the 10/20 ms waits, as the remaining review thread asked. Not executed in this environment; CI will run it.
|
Added the test the remaining thread asked for in One thing worth writing down, because it explains the earlier BLOCKED state: the The test is not executed on my machine (no full checkout here); CI will run it. |
Motivation & Context
Fixes #10137. On Windows,
FileSessionStore.set()raisesPermissionError: [WinError 5]when the destination snapshot file is briefly unavailable to
os.replace()— for exampleright after a previous replacement. The store already serializes writes per snapshot with
a process-local lock, so this is not a locking gap: the replacement itself fails, and a
racing save raises instead of following the last-writer-wins behaviour documented on the
class.
Measured on Windows 11 23H2 (OS build 22631.6199, Python 3.11.16,
mainat 13956ad),32 concurrent
set()calls for one session id, six runs:100 sequential
set()calls fail zero times either way. The failure needs concurrentsaves; a second process saving every 50 ms while another reads did not reproduce it on
this machine, so the table reports the condition I could measure rather than a mechanism.
Description & Review Guide
set()now replaces the snapshot through_replace_session_file(), which retriesos.replace()up to three times (10 ms, then20 ms) when it raises
PermissionError, and re-raises the original error after the lastattempt. The retry stays inside the existing per-file lock, so ordering guarantees are
unchanged. The retry runs on Windows only (
os.name == "nt"): on POSIX the sameexception means the caller genuinely may not replace the file.
longer reaches the caller; a persistent permission problem still fails, after at most
~30 ms of retries. POSIX behaviour is unchanged.
versus something like
ReplaceFileW, whether the retry delays suit the store's perfprofile, and whether the Windows-only gate is worth keeping — the alternative is to
retry everywhere and accept a 30 ms worst case on a call that is about to fail anyway.
The retry test pins the platform flag with
monkeypatch, so both branches are coveredon every CI OS.
New tests, both in
python/packages/core/tests/core/test_sessions.py:test_set_retries_transient_permission_error_during_atomic_replace— the name andlocation the automated triage suggested; it fails with the original
PermissionErrorwhen the retry is removed.
test_set_reports_permission_error_without_retrying_off_windows— the error is raisedafter a single
os.replace()attempt when the platform gate is off, and the previoussnapshot is left intact.
Related Issue
Fixes #10137
AI Assistance
AI assistance details: implementation, tests, and this PR description were drafted with an AI assistant. I ran every verification listed below myself on the Windows machine described above.
Contribution Checklist
python/packages/core/tests/core/test_sessions.py→ 192 passed, 1 skippedPermissionError) and passes with itruff format --checkandruff checkare clean on both touched filespyrightwithpyrightconfig.tests.json→ 0 errors, 0 warnings for both filescoresuite: not run locally for this revision — the workspace pinsuv==0.13.5and this machine has 0.11.7, souv syncrefuses without a global tool upgrade. Leaving it to CI.One failure on this host is unrelated to this change:
test_harness_file_access.py::test_filesystem_store_matches_in_memory_complexity_boundbuilds a path past MAX_PATH on a Windows host with
LongPathsEnabled=0and fails with orwithout the change; it covers the file store rather than the session store, and the
long-path behaviour is tracked separately in #10141.