fix(utils): handle EACCES in atomic_replace on Windows - #73807
Conversation
Native Win11 verification (monerostar)Host: Windows 11 build 26200, CPython 3.11.15, current Bug is realHolding the target open the way a running Hermes process often does makes
This PR’s
|
| Hold | copyfile fallback |
|---|---|
Python open("r") / open("r+") |
SUCCESS — target content becomes the new bytes |
| Other-process read hold | SUCCESS |
CreateFile share R+W / R+W+D |
SUCCESS |
Snippet shape used for the patched path (mirrors the PR):
if exc.errno not in (errno.EXDEV, errno.EBUSY, errno.EACCES):
raise
shutil.copyfile(tmp_str, real_path)
# …copystat/fsync/unlink as today…Honest limit (not a reason to reject)
If the target is opened with no write share (share_mode=0 or read-only share without FILE_SHARE_WRITE), both os.replace and copyfile fail with EACCES — the fallback cannot write the destination either. That is expected Windows sharing behavior; the PR still fixes the common “another handle has the file open with normal share flags” case described in the PR body (config migrate / update).
Review take
- Direction: approve-quality for a 4-line errno widen.
- Live evidence matches the claimed failure mode (
PermissionError: [WinError 5]during replace while target is open). - Optional follow-up (not blocking): a tiny unit test that monkeypatches
os.replaceto raiseOSError(errno.EACCES, ...)and asserts the copy fallback runs — keeps this regression covered on Linux CI too.
No code changes from me — verification only.
Sibling signal (cross-link)After the live Win11 verify above: #57777 (
Not a reject of this PR — just routing note so the two don’t thrash each other. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the shared helper; the current-main premise remains live: utils.py:115-125 re-raises EACCES, and config persistence reaches this helper (for example hermes_cli/config.py:3757).
Problems
utils.py:120addsEACCESto the fallback set on every platform, while the PR rationale is specific to Windows sharing violations. Current helper documentation limits fallback semantics toEXDEV/EBUSYatutils.py:104-106; preserve POSIXEACCESpropagation rather than broadening it.- The PR changes no tests. Existing copy-fallback coverage at
tests/test_atomic_replace_symlinks.py:198-215coversEXDEV, notEACCES.
Suggested changes
- Scope the new eligibility to Windows sharing violations.
- Add a simulated Windows-eligible
EACCESfallback test and a POSIXEACCESpropagation test.
The linked open PR #57777 demonstrates this distinction and related coverage. This is an automated hermes-sweeper review.
| # Windows: os.replace fails with EACCES when the target file is open | ||
| # in another handle (e.g. the running process has config.yaml | ||
| # mmap'd or loaded). Fall back to copy+unlink like EXDEV/EBUSY. | ||
| if exc.errno not in (errno.EXDEV, errno.EBUSY, errno.EACCES): |
There was a problem hiding this comment.
This admits EACCES on POSIX too, although the reported condition is Windows-specific. Please gate this fallback to the Windows sharing-violation case so ordinary POSIX permission denials continue to propagate directly.
os.replace() fails with PermissionError (EACCES) on Windows when the target config file is open in another handle (e.g. during config migration after update). Only EXDEV and EBUSY were handled, causing 'hermes config migrate' to crash with 'PermissionError: [WinError 5]'. Add EACCES to the fallback set so atomic_replace falls through to the copy+fsync+unlink path on Windows instead of raising.
Monkeypatches os.replace to raise OSError(errno.EACCES) and asserts the copy+unlink fallback path runs, keeping the Windows open-handle case covered on Linux CI too.
76a0793 to
d3939a8
Compare
|
Thanks for the detailed Win11 verification, @monerostar — really appreciate the live evidence! @teknium1: I've addressed both review points: Windows-gated fallback — EACCES now falls back to copy+unlink only on Windows (os.name == "nt"); on POSIX it keeps propagating as a genuine permission denial. |
EACCES on POSIX is a genuine permission denial and must keep propagating. Gate the new fallback to os.name == 'nt' (Windows sharing violations) per review feedback, and add a POSIX propagation test so both branches stay covered on Linux CI.
|
Superseded by #84852. You identified the right helper and the right fallback strategy. #84852 credits you as a co-author. The blanket Worth noting the narrower fix isn't |
os.replace() fails with PermissionError (EACCES) on Windows when the target config file is open in another handle (e.g. during config migration after update). Only EXDEV and EBUSY were handled, causing 'hermes config migrate' to crash with 'PermissionError: [WinError 5]'.
Add EACCES to the fallback set so atomic_replace falls through to the copy+fsync+unlink path on Windows instead of raising.
What does this PR do?
Related Issue
Fixes #
Type of Change
Changes Made
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"Screenshots / Logs