Skip to content

fix(utils): retry transient atomic replace permission errors - #36921

Closed
zapabob wants to merge 1 commit into
NousResearch:mainfrom
zapabob:codex/fix-windows-atomic-replace-retry
Closed

fix(utils): retry transient atomic replace permission errors#36921
zapabob wants to merge 1 commit into
NousResearch:mainfrom
zapabob:codex/fix-windows-atomic-replace-retry

Conversation

@zapabob

@zapabob zapabob commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • retry transient PermissionError failures around os.replace in atomic_replace
  • keep the retry bounded so real permission failures still surface
  • add a regression test for transient replace failures

Why

On Windows, antivirus/indexers or another rapid writer can briefly hold the target path while atomic_json_write is replacing it. That can make otherwise-safe concurrent writes fail with PermissionError even though retrying moments later succeeds.

Validation

  • python -m pytest --timeout-method=thread tests/hermes_cli/test_atomic_json_write.py -q
  • python -m ruff check utils.py tests/hermes_cli/test_atomic_json_write.py

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 1, 2026

@teknium1 teknium1 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.

Thanks for addressing a real Windows atomic-write failure mode. Current main still performs a single os.replace at utils.py:115, while its handler only recognizes EXDEV/EBUSY at utils.py:116-135, so PermissionError remains unhandled.

Problems

  • The proposed loop retries every platform at utils.py:84-91, despite the Windows-specific rationale. That delays permanent POSIX permission failures.
  • Current main added the EXDEV/EBUSY copy fallback in utils.py:114-135 via bf8effad0 (#43852). This branch is currently unmergeable, so salvage needs to integrate the retry without bypassing that fallback.
  • The new test at tests/hermes_cli/test_atomic_json_write.py:168-188 covers only success after transient failures; it does not cover retry exhaustion or POSIX non-retry behavior. The duplicate discussion on #45022 identified the same coverage gap.

Suggested changes

  • Scope bounded PermissionError retries to Windows within the current fallback structure.
  • Add Windows-success, Windows-exhaustion, and POSIX-immediate-propagation tests with sleep mocked out.

Automated hermes-sweeper review.

Comment thread utils.py
target_str = str(target)
real_path = os.path.realpath(target_str) if os.path.islink(target_str) else target_str
os.replace(str(tmp_path), real_path)
for attempt in range(_ATOMIC_REPLACE_MAX_ATTEMPTS):

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.

The PR rationale is Windows mandatory locking, but this loop retries every POSIX PermissionError too. Please scope the retry to the Windows path so a permanent POSIX permission denial still propagates immediately.

raise PermissionError("temporarily locked")
real_replace(src, dst)

with patch.object(utils.os, "replace", side_effect=flaky_replace):

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.

This only proves eventual success. When the implementation is scoped to Windows, explicitly simulate that branch here and add a separate persistent-failure assertion so the bounded retry still surfaces a real denial.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #84852.

Your bounded-retry instinct was right and is preserved in #84852 — a retry that wins keeps the write fully atomic, which no pure-fallback approach achieves. Credited as a co-author.

Two extensions: the retry is keyed on the specific winerror codes ({5, 32, 33}) rather than PermissionError broadly, so a genuine ACL denial doesn't silently burn the budget; and a fallback handles the reader that outlives the retries, which retry alone drops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants