Skip to content

fix(update): clean orphaned hermes shim entries from PendingFileRenameOperations - #85942

Open
Halldrix wants to merge 2 commits into
NousResearch:mainfrom
Halldrix:fix/cleanup-pending-file-rename-operations
Open

fix(update): clean orphaned hermes shim entries from PendingFileRenameOperations#85942
Halldrix wants to merge 2 commits into
NousResearch:mainfrom
Halldrix:fix/cleanup-pending-file-rename-operations

Conversation

@Halldrix

@Halldrix Halldrix commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

_schedule_replace_on_reboot queues MoveFileExW(MOVEFILE_DELAY_UNTIL_REBOOT) pairs into the Session Manager registry so a locked hermes.exe can be renamed aside on next boot. Neither success nor failure of the subsequent recovery install removes the queued entries, so across repeated failed boot-recoveries they accumulate (one per failed boot — the #85839 report counted 12). On the next reboot the Session Manager applies entry #1, renaming the current, healthy hermes.exe to a .old. backup — the shim vanishes after a reboot that was supposed to fix things.

Add _cleanup_pending_file_rename_operations() (called from _cleanup_quarantined_exes on every launch) that scans the PendingFileRenameOperations registry value and removes hermes-shim pairs whose source file no longer exists (the shim was rewritten by a later install — the pending rename is a booby trap) or whose .old. target backup no longer exists (stale pair from a failed cycle). Also disarms "armed" pairs where the source is the current healthy hermes*.exe (no .old.) and the target is a .old. backup — this is the booby trap that would destroy the healthy shim on next boot. Once we're running, the install is good; disarm it.

Improvements over original:

  • Read-first pattern: opens registry with KEY_READ, only re-opens with KEY_WRITE when changes exist — avoids Access Denied on every launch for non-elevated Windows users.
  • Test-safety: integration test mocks winreg so it never touches the real registry on Windows dev machines.
  • 11 tests (was 10), including explicit armed-pair coverage.

Related Issue

Fixes #85839

Type of Change

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

Changes Made

  • hermes_cli/main.py — new _cleanup_pending_file_rename_operations() function with read-first pattern + armed-pair disarm logic + call from _cleanup_quarantined_exes
  • tests/hermes_cli/test_update_pending_rename_cleanup.py — 11 new tests (armed pair, test-safety mock, read-first verified)

How to Test

Sabotage-run: with the fix stashed, all 11 tests fail (11/11 red); with the fix restored they all pass. Verified locally before opening.

Evidence

The #85839 report counted 12 accumulated PendingFileRenameOperations entries from repeated failed boot-recoveries. On the next reboot the Session Manager applied the first entry, renaming the healthy hermes.exehermes.exe.old.<ts>, making the shim vanish after a reboot that was supposed to fix things.

Checklist

  • I have read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I have searched for existing PRs and found no duplicates
  • I have run tests and they pass
  • I have added tests for my changes

🛠️ Dev: Halldrix
🤖 Sidekick: Hermes Agent v0.20.0

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage area/install-update Installer, updater, packaging, wheels, doctor 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 labels Aug 14, 2026
adamcap926 added a commit to adamcap926/hermes-agent that referenced this pull request Aug 14, 2026
On Windows `hermes update` can never complete. `hermes` is launched
through a distlib console-script launcher, which is not a thin redirect:
it spawns `venv\Scripts\python.exe` as a child, hands it the launcher's
own path as the script, and blocks. Every invocation is two processes:

    hermes.exe (pid A, holds Scripts\hermes.exe as its running image)
      \_ python.exe (pid B, runs hermes_cli.main)

The update ends in `uv pip install -e .`, which rewrites the console-
script shims -- including the one pid A has mapped. Windows refuses to
replace a running image, so uv fails with "The process cannot access the
file because it is being used by another process. (os error 32)". The
git path fails, the ZIP fallback fails identically, and no retry can
succeed: nothing pid B does releases the lock, because the holder is its
parent.

Remove the hazard rather than race it. Before the update touches
anything, re-launch it as a detached `python -m hermes_cli.main`
grandchild and let pids B and A exit. The grandchild waits for the
launcher to disappear, then updates against an ordinary unlocked file:

    hermes.exe (A) --> python.exe (B) --> python.exe (C, detached)
         exit             exit              waits for A, then updates

Hooked into cmd_update after --check (which installs nothing) and before
the update lock is acquired: a trampolining parent that had already
written the lock marker would release it moments later while the
grandchild ran unlocked, since the grandchild's acquire() sees its
still-live parent as an ancestor and would run under a claim about to
vanish.

stdin is DEVNULL so the detached updater cannot race the reclaimed shell
for keystrokes. That also selects the existing non-interactive path, so
local changes follow updates.non_interactive_local_changes (default
stash) rather than prompting where nobody can answer.

Off Windows maybe_trampoline() returns False immediately and behavior is
unchanged; POSIX replaces a running executable's inode atomically.

Verified on Windows 11 / Python 3.11.15 / uv, non-elevated: before, four
consecutive failed updates; after, the launcher exits, the grandchild
completes the update, and no os error 32 occurs on either path.

Scoped deliberately to the trampoline. The residual case where another
process holds a shim, and cleanup of the reboot-deferred rename entries
that case queues, are addressed by NousResearch#68821 and NousResearch#85942 respectively and
are not duplicated here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FGLv7LyNrpaPm4ciGqhyxD
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(update): clean orphaned hermes shim entries from PendingFileRenameOperations

Targeted, well-documented fix for a nasty Windows footgun. Observations:

  1. Test-safety hazard: test_integration_called_from_cleanup_quarantined_exes calls the real _cleanup_pending_file_rename_operations() (via original()) with _is_windows patched to True and no winreg mock. On a Windows dev machine this opens the real HKLM\SYSTEM\CurrentControlSet\Control\Session Manager with KEY_WRITE and, if hermes entries exist, rewrites the actual registry. On Linux CI it is a silent no-op (winreg import fails), so the hazard only surfaces on Windows hosts. The test should mock winreg (or the whole function) so it can never touch the real registry.

  2. Every hermes launch on Windows now opens HKLM\...\Session Manager with KEY_READ | KEY_WRITE — including for non-elevated users, who will hit Access Denied on every single invocation (silently swallowed, but it is an elevated-access attempt on the hot startup path). Consider a read-first pattern: open with KEY_READ, scan, and only re-open with KEY_WRITE when a removal is actually needed.

  3. Possible gap against the primary booby trap: a pair whose source (hermes.exe) and .old. target both exist is deliberately kept (test_keeps_pair_when_both_source_and_target_exist). But that is exactly the armed rename the Session Manager will apply at next boot — renaming the healthy shim away — per the docstring's own scenario Architecture planning #3. If _schedule_replace_on_reboot's rename-away shape (hermes*.exehermes*.exe.old.*) is identifiable, disarming it once the current install is known-good (regardless of whether the .old. file exists) would close the loop; otherwise the trap survives whenever a stale .old. backup coexists.

…eOperations

`_schedule_replace_on_reboot` queues `MoveFileExW(MOVEFILE_DELAY_UNTIL_REBOOT)`
pairs into the Session Manager registry so a locked `hermes.exe` can be renamed
aside on next boot.  Neither success nor failure of the subsequent recovery
install removes the queued entries, so across repeated failed boot-recoveries
they accumulate (one per failed boot — the NousResearch#85839 report counted 12).  On the
next reboot the Session Manager applies entry #1, renaming the current, healthy
`hermes.exe` to a `.old.` backup — the shim vanishes after a reboot that was
supposed to fix things.

Add `_cleanup_pending_file_rename_operations()` (called from
`_cleanup_quarantined_exes` on every launch) that scans the
`PendingFileRenameOperations` registry value and removes hermes-shim pairs
whose source file no longer exists (the shim was rewritten by a later install
— the pending rename is a booby trap) or whose `.old.` target backup no longer
exists (stale pair from a failed cycle).  Non-hermes entries are left
untouched.

Fixes NousResearch#85839

🛠️ Dev: Halldrix
🤖 Sidekick: Hermes Agent v0.20.0
@Halldrix
Halldrix force-pushed the fix/cleanup-pending-file-rename-operations branch from 0f3879c to 5fdd264 Compare August 16, 2026 18:16
- Read-first pattern: open registry with KEY_READ, only re-open with
  KEY_WRITE when changes exist (avoids Access Denied on non-elevated users)
- Disarm 'armed' pairs: healthy hermes*.exe (no .old.) + .old. target
  is the NousResearch#85839 booby trap — remove it once install is confirmed good
- Test-safety: mock winreg in integration test so it never touches real registry
- Add test for armed-pair category (both source and target exist)
@Halldrix

Copy link
Copy Markdown
Contributor Author

Thanks for the review — all three observations were valid and are addressed in 60a21ba44.

1. Test-safety hazard — Fixed. The integration test now injects a winreg mock into sys.modules before calling original(), so it never touches the real HKLM\SYSTEM\CurrentControlSet\Control\Session Manager key on any platform.

2. KEY_WRITE on the hot path — Fixed. The function now opens with KEY_READ first, scans, and only re-opens with KEY_WRITE when removed > 0. Non-elevated users no longer hit an Access Denied write-attempt on every launch.

3. Armed-pair gap — Fixed. The core issue: a pair where hermes.exe exists (source, no .old.) and hermes.exe.old.<ts> exists (target) is precisely the armed rename the Session Manager will apply next boot — renaming the healthy shim away. Once we're running, the install is known-good, so this category is now disarmed regardless of whether the .old. file still exists. The previous test_keeps_pair_when_both_source_and_target_exist asserted the wrong expectation and has been replaced with two tests that verify the armed pair is removed (healthy shim + .old. target with both files present, and a legacy variant).

11/11 tests pass on Linux CI and verified on a native Windows 11 host (Python 3.11.15). Sabotage-run (fix stashed) → 11/11 red, fix restored → 11/11 green.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage 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.

Windows: failed boot-recovery quarantines accumulate MoveFileEx pending renames — next reboot renames the healthy hermes.exe away

3 participants