fix(utils): handle Windows sharing violations in atomic_replace

[lEWFkRAD]

What does this PR do?

Stops atomic_replace() from silently dropping writes on Windows when another process holds a read handle on the target file.

CPython opens files without FILE_SHARE_DELETE on Windows, so os.replace onto a file with any concurrent reader fails with ERROR_SHARING_VIOLATION → PermissionError (EACCES). The helper's copy fallback only caught EXDEV/EBUSY, so the exception propagated (and was swallowed by most callers), the update was lost, and the freshly-written .tmp file was orphaned. This is hit routinely on an active Windows gateway: gateway/run.py::_persist_active_agents rewrites gateway_state.json at every turn boundary while gateway/status.py::read_runtime_status readers poll the same file — and it is latent in every other atomic_json_write/atomic_yaml_write call site (config, auth, cron state, …).

The fix treats the Windows sharing-violation class as fallback-eligible, in two stages:

1. Bounded retry of the atomic rename (5 × 20 ms) — sharing violations from transient readers usually clear within milliseconds, and a successful retry keeps the write fully atomic.
2. Copy/fsync/unlink fallback (the existing EXDEV/EBUSY path) if the handle is still held, so the write lands instead of vanishing. The docstring now notes the fallback's non-atomicity, which is why the rename is retried first.

EXDEV/EBUSY skip the retry loop (they never clear on retry) and go straight to the copy fallback as before, so cross-device deployments pay no new latency. POSIX behavior is unchanged: EACCES there means directory permissions, the copy fallback would fail identically, and the error still propagates.

Related Issue

Fixes #57775

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• utils.py: added _is_windows_sharing_error() / _replace_error_allows_fallback() predicates and a bounded retry loop (_SHARING_RETRY_ATTEMPTS × _SHARING_RETRY_DELAY_S) in atomic_replace(); Windows PermissionError/EACCES now retries the rename, then uses the existing copy fallback. Docstring updated.
• tests/test_atomic_replace_symlinks.py:
  • test_atomic_replace_windows_held_read_handle_falls_back_to_copy — real handle held for the whole call (Windows): copy fallback lands the content, no orphaned .tmp.
  • test_atomic_replace_windows_transient_reader_succeeds_via_retry — real handle released from the retry loop's sleep hook (Windows, deterministic): the atomic rename wins and shutil.copyfile is asserted unreached.
  • test_atomic_json_write_windows_concurrent_reader — end-to-end gateway_state.json scenario through atomic_json_write (Windows).
  • test_atomic_replace_sharing_violation_simulated_retry_then_copy — cross-platform pin of the retry count (1 + _SHARING_RETRY_ATTEMPTS) before the copy runs, via monkeypatched os.replace.
  • test_atomic_replace_eacces_propagates_on_posix — pins that POSIX EACCES still propagates unchanged.
  • test_atomic_replace_other_oserror_propagates — switched its sentinel errno from EACCES (now fallback-eligible on Windows) to ENOSPC, which is non-fallback on every platform.

How to Test

1. pytest tests/test_atomic_replace_symlinks.py -q on Linux/macOS — the simulated retry/fallback and POSIX-propagation tests run; Windows-specific ones skip.
2. On Windows, same command — the three real-sharing-violation tests run against actual ERROR_SHARING_VIOLATIONs.
3. Manual repro of the original bug (Windows, pre-fix it raises PermissionError and orphans a .tmp):

   import utils
   utils.atomic_json_write("gateway_state.json", {"active_agents": 1})
   with open("gateway_state.json") as reader:
       utils.atomic_json_write("gateway_state.json", {"active_agents": 2})  # now succeeds

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass — targeted files pass on Windows except pre-existing native-Windows environment failures unrelated to this change (symlink-privilege WinError 1314 without Developer Mode, and a POSIX chmod 0o600 assertion); full details in the test run notes above
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Windows 11, Python 3.11

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — docstring updated
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — the change is Windows-gated; POSIX semantics are pinned by a new test
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

🤖 Generated with Claude Code

[teknium1]

Thanks for tracing this to the shared write helper. The current-main premise remains valid: utils.py:114-118 only falls back for EXDEV/EBUSY, while gateway status reads use path.read_text() at gateway/status.py:439-448 and turn-boundary persistence is best-effort at gateway/run.py:4575-4579.

Problems

• utils.py:109 classifies every Windows PermissionError/EACCES as a sharing violation. This cannot distinguish ordinary access-denied failures from ERROR_SHARING_VIOLATION; please narrow the retry/copy recovery to the specific sharing-violation WinError.
• utils.py:162-164 keeps retrying if a retry returns EXDEV or EBUSY, although the adjacent comment says those errors never clear on retry. Break to the existing copy fallback for those errors instead.
• tests/test_atomic_replace_symlinks.py:483-491 only models generic EACCES; add coverage that access denied still propagates and that a retry transitioning to EXDEV/EBUSY does not consume the remaining sharing retries.

The PR base and current main have identical blobs for both changed files, so this should remain a small salvage after those corrections. This is an automated hermes-sweeper review.

[kevin182]

Independent confirmation of this failure mode on auth.json, including a variant that I think strengthens the case for the copy fallback in this PR: the collision also happens within a single process, between threads, not only across processes.

Environment: Windows 11 (10.0.26200), Python 3.11, hermes-agent 0.19.0.

User-visible failure

This surfaces as a hard error toast in the desktop app, not a silently dropped write:

Hermes error
agent init failed: [WinError 5] Access is denied:
'D:\Hermes\home\auth.json.tmp.28472.e1e20ec5ca6d4b33bd43fcadc9c0df95' -> 'D:\Hermes\home\auth.json'

Hermes error
agent init failed: [WinError 5] Access is denied:
'D:\Hermes\home\auth.json.tmp.28472.4d6afc8f2da54d17a3c5103003b42fd5' -> 'D:\Hermes\home\auth.json'

Note WinError 5 (ERROR_ACCESS_DENIED), not 32. os.replace onto a target another handle holds open returns 5 rather than 32; both map to EACCES, so both reach the same atomic_replace branch and re-raise identically.

The same-process detail

Both failures carry the same writer PID (28472) with different temp UUIDs, so they are two distinct write attempts from one process. That process is the desktop app's single Python backend:

28472 [python.exe, 95 threads] <- 33056 [python.exe] <- 2696 [Hermes.exe] <- explorer.exe

No second Hermes process was involved. The gateway was a separate PID and idle. So a reader thread and a writer thread inside that one 95-thread process are colliding on auth.json.

That matters for the fix: _auth_store_lock takes a cross-process advisory file lock, and _auth_lock_holder_for() tracks reentrancy in a threading.local. Whatever the intended semantics, it demonstrably does not serialize two threads within one process here. If the kernel lock is process-owned, a second thread would see it as already held rather than blocking. Worth a maintainer's eye, since it means tightening only the cross-process path will not close this.

Trigger

Reliable on this install: open several sessions in quick succession in the desktop app (clicking down the session history list). Each click runs an agent init, each init touches the auth store on its own thread, and inits that overlap collide. The result is a session that fails to initialize and is unusable until the click is retried.

Minimal repro, no Hermes involved

import os, tempfile, errno

d = tempfile.mkdtemp()
target = os.path.join(d, "auth.json")
tmp1, tmp2 = os.path.join(d, "t.a"), os.path.join(d, "t.b")
for p in (target, tmp1, tmp2):
    open(p, "w").write("{}\n")

os.replace(tmp1, target)          # CONTROL: no reader open
print("control: ok")

reader = open(target, "r")        # mirrors _load_auth_store -> read_text()
reader.read()
try:
    os.replace(tmp2, target)
except OSError as e:
    print(f"test: errno={e.errno} ({errno.errorcode.get(e.errno)}) winerror={e.winerror}")
    print(f"in (EXDEV, EBUSY)? {e.errno in (errno.EXDEV, errno.EBUSY)}")

control: ok
test: errno=13 (EACCES) winerror=5
in (EXDEV, EBUSY)? False

One ordinary reader is sufficient. No antivirus, no second process. Adding a Defender exclusion for the Hermes directory changed nothing, consistent with the cause being in-process readers.

Two differences from the gateway_state.json report

1. Not silently swallowed. #57775 notes most callers swallow the exception and the write is quietly dropped. _save_auth_store does not: the PermissionError propagates and is shown to the user as a failed agent init. The user-facing impact is broader than dropped writes alone.

2. No orphaned temp files on this path. _save_auth_store unlinks the temp in a finally, so auth.json does not accumulate .tmp files the way gateway_state.json does. The write is simply lost.

Also worth noting, hermes_cli/auth.py wraps most _load_auth_store() calls in _auth_store_lock(), but these call sites appear not to: lines 701, 987, 1430, 1609, 1651, 1657, 1678, 1992, 3454. Since _load_auth_store does auth_file.read_text(), any of those is a candidate reader.

On #57777 vs #45022

The same-process case argues specifically for this PR over retry-only. A competing reader in a different process tends to clear within milliseconds, so bounded retry usually rescues it. A reader thread inside the same process, holding the file across an init sequence, may still be open when the retry budget expires. The copy/fsync/unlink fallback is what makes that case land instead of failing. +1 for merging this one.

[kevin182]

Could the P3 on this PR be revisited? The issue it fixes (#57775) is P2, and from a user's seat this is not cosmetic: it makes sessions impossible to open.

I want to be precise about the impact, because I think the original framing may be what led to the lower priority.

#57775 describes the failure as writes being silently dropped by callers that swallow the exception. On the auth.json path that is not what happens. _save_auth_store lets the PermissionError propagate, and the desktop app surfaces it as:

Hermes error
agent init failed: [WinError 5] Access is denied:
'D:\Hermes\home\auth.json.tmp.28472.e1e20ec5ca6d4b33bd43fcadc9c0df95' -> 'D:\Hermes\home\auth.json'

agent init failed means the session does not open. The conversation is intact on disk, but it cannot be used until the init happens to win the race on a later attempt.

Why I think this warrants more than P3:

1. It blocks the primary workflow, not a background task. Opening a session is the main thing the desktop app does. When init loses the race, that session is unusable.

2. Ordinary use triggers it. The reliable reproduction here is clicking down the session history list. Each click starts an agent init, inits that overlap race each other, and some fail. Nothing unusual in the configuration is required.

3. It is not limited to multi-process setups. Both failures above came from the same PID with different temp UUIDs, inside one 95-thread desktop backend, with the gateway idle in a separate process. So the exposed path is the default single-app case, not an exotic deployment.

4. No setting mitigates it. I went through this at length: stopping the gateway, reducing concurrent sessions, and a Windows Defender exclusion all made no difference, because the competing reader is a thread inside the writing process. The only workaround available to a user is to click again and hope, which is a coin flip.

5. It recurs across restarts and across days. I hit it repeatedly in one evening across several sessions, on multiple distinct backend PIDs, surviving both app restarts and a full update to 0.19.0.

I am not arguing the diagnosis in #57775 is wrong. It is exactly right, and this PR looks like the correct fix. My point is narrower: the user-facing severity on the auth path is higher than "dropped write" suggests, and the fix being P3 while the defect is P2 leaves a blocking bug open with a tested patch already written.

For what it is worth, I have applied the equivalent change locally and it resolves the failure. A standalone test of the logic, unpatched versus patched, on Windows 11 / Python 3.11:

ORIGINAL:
  no reader                           OK
  transient reader (clears in 50ms)   RAISED  WinError 5
  persistent reader (never closes)    RAISED  WinError 5

PATCHED (bounded retry, then copy fallback):
  no reader                           OK    (0 ms)
  transient reader (clears in 50ms)   OK    (62 ms, retry won, atomicity preserved)
  persistent reader (never closes)    OK    (106 ms, copy fallback, write landed)

The persistent-reader row is the one that matters for this PR specifically: retry alone does not rescue it, which is what distinguishes this approach from #45022.

Happy to provide anything else useful. Thanks for the work on this.

[OutThisLife]

Superseded by #84852.

Your retry-then-fallback architecture is the right shape and #84852 keeps it — it is the only approach in this cluster that handles both a transient reader and one that outlives the retry budget. Credited as a co-author.

Two things surfaced while verifying on real Windows (11 build 26200, CPython 3.11) that needed a reshape rather than a patch:

The error code is 5, not 32. A held target handle reports ERROR_ACCESS_DENIED; ERROR_SHARING_VIOLATION (32) is what a held source reports:

TARGET open for read (in-process)     winerror=5
TARGET open by ANOTHER PROCESS        winerror=5
SOURCE (tmp) open for read            winerror=32

So the primary predicate never fired for the bug, and every real case fell through to the os.access branch. @kevin182 flagged WinError 5 in this thread and that turned out to be the key detail.

The os.access narrowing cannot discriminate denial. os.replace needs delete-child rights on the parent directory, so a directory-level denial reports the target as writable:

parent dir ACL: deny delete-child+write   os.access(tgt,W)=True   -> classified transient

#84852 stops guessing: contention and genuine denial take the same bounded path, and a real denial is re-raised unchanged with its temp file intact.

One more thing worth flagging, since it affected the review: the three Windows tests here gated with pytest.skip(os.name != "nt") and no marker. scripts/ci/list_os_marked_tests.py greps for the marker name to pick which files the Windows lane imports, so they ran on no host at all — the green suite was covering zero of the real path. #84852 uses @pytest.mark.windows_only and the tests are confirmed failing on main with WinError 5, passing with the fix.

Thanks for filing #57775 with a clean reproduction — the diagnosis in that issue was correct throughout.