fix(desktop): work profile deletion silently reverted after app restart - #72152
fix(desktop): work profile deletion silently reverted after app restart#72152adurham wants to merge 3 commits into
Conversation
Two independent bugs let a deleted profile reappear / leave orphaned resources on next launch: 1. hermes_cli/profiles.py's backend-process scanner required argv[0] to resolve to an executable literally named "hermes". Electron's pool-backend spawn resolves the hermes console-script shim's path and execs it via the interpreter directly (python3 /path/to/hermes ...), so argv[0] reports as "python3" and the scanner never matched the running backend -- delete removed the profile's files but left its live backend process running (still bound to a port via uvicorn), which accumulates across repeated delete/recreate cycles. 2. The desktop sidebar's ProfileRail only refreshed its cached profile list once, on mount, so a delete/create/rename from another surface (another window, or the CLI) left a stale ghost entry until something unrelated triggered a refetch. Note: a delete via this window's own Manage-Profiles view already refreshes the shared $profiles atom ProfileRail subscribes to (confirmed by reading refreshProfiles() and handleConfirmDelete()) -- this fix only covers the cross-window/cross- process staleness gap, not a duplicate of the already-merged NousResearch#57329's Manage-Profiles rail-refresh work. Fix 1: recognize a python-interpreter argv[0] exec'ing a hermes-named console-script shim via argv[1]. Fix 2: refresh the profile list on window focus/visibilitychange, matching the existing pattern used elsewhere in the sidebar (sidebar/index.tsx, use-background-sync.ts, star-map.tsx, use-gateway-boot.ts all use the same focus+visibilitychange pattern). ## Related work already on main PR NousResearch#57329 (merged) fixed the *headline* symptom from issue NousResearch#52279 (deleted profile respawns) via a different, non-overlapping mechanism: routing profile-delete through the primary backend instead of spawning a fresh pool backend, plus a separate recreation guard in ensure_hermes_home() (NousResearch#49435, merged) that makes a backend spawned into a deleted profile's directory raise FileNotFoundError instead of silently recreating it. This PR is NOT a duplicate of that fix. Verified: even with both of those merged, a backend process that survives because of gap #1 above still holds a bound port via uvicorn -- it just can no longer resurrect the profile directory. That's real resource-hygiene, not a symptom already covered. Gap #2 touches a different file/component (ProfileRail / profile-switcher.tsx) than NousResearch#57329's rail-refresh half (which touched the Manage-Profiles view's own $profiles.ts / index.tsx) and covers a distinct staleness path (cross-window/cross-process, not same-window delete-then-refresh). Tests: tests/hermes_cli/test_profiles.py -- 156 passed (existing + regression coverage for the argv[0] python-interpreter detection case). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tems Documents outcomes for the 6 desktop-category Bucket A items: Submitted (4): - PR NousResearch#72151 -- RAF-throttle blank transcript on session switch - PR NousResearch#72152 -- profile deletion zombie backend + cross-window rail staleness - PR NousResearch#72153 -- Nerd Font terminal fallback - PR NousResearch#72155 -- desktop model picker hiding Anthropic Deprioritized, needs hand-reconciliation (3): drag-to-reorder, workspace tab close button, queued composer wrong-session delivery. All three rejected 1+ files on git apply --check with real semantic drift (not line-offset noise) against the fast-churning desktop session/composer code. Flagged for revisit rather than force-applying a stale patch. The profile-deletion and model-picker items both got real scrutiny beyond "does it apply cleanly": profile-deletion required two rounds of external consult after search-first turned up 2 already-merged PRs on the same headline symptom -- verified directly (not assumed) that this fix's two pieces are genuinely non-overlapping gaps, not stale duplicates. Model picker's fix was confirmed to extend an already-established upstream credential-detection pattern rather than introduce new CC-mimicry plumbing, addressing the specific caution flagged in the original audit. Saved verified patches to .upstream-candidates/ for reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alt-glitch
added
type/bug
… scripts
External review (Fable) caught a real false-positive widening in the
original commit: the new argv[1] script-name check reused the loose
`script_name == "hermes" or script_name.startswith("hermes")` pattern
(copy-pasted from the exe_name check above it), but argv[1] can be ANY
user-invoked python script path when argv[0] is a bare interpreter --
unlike a directly-resolved executable name, where a false match on the
substring is rare. A user's own script named e.g. "hermes-notes.py" or
"hermes-unrelated-tool" run via `python3 <script>` would be misidentified
as the console-script shim and become killable by profile delete.
Match against the actual known console-script entry points instead
(pyproject.toml [project.scripts]: hermes, hermes-agent, hermes-acp),
stripping the script's extension before comparing.
Added 2 regression tests: one confirms the false-positive case is now
rejected (fails against the pre-fix loose-match code, confirmed via a
scripted revert), the other confirms the other two real entry points
(hermes-agent, hermes-acp) still match via the shebang-exec path.
Tests: tests/hermes_cli/test_profiles.py -- 158 passed (156 previous + 2
new).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ran all 7 upstream PRs filed this session back through external review with the real diffs. 2 came back with genuine, actionable findings, both fixed and pushed as follow-up commits to the existing PRs: - NousResearch#72054 (MCP orphan reap): silent exception swallow in the cleanup path now logs; added a 4th test exercising the real shutdown()/park machinery end-to-end rather than only faked versions. - NousResearch#72152 (profile deletion): tightened a false-positive-prone script-name match to the actual known console-script entry points. 2 more findings were checked against the real code and resolved as non-issues (not accepted at face value, not dismissed either) -- one on NousResearch#72087 (content/blocks divergence risk -- verified architecturally safe since blocks are what's actually replayed regardless of content's state; image-strip gap -- verified blocks can never contain images given how they're populated) and one on NousResearch#72151 (ref-update ordering -- verified correct by reading the real flush function). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this beyond the prior deletion-respawn fix. Current main still gates _profile_bound_backend_pids() on markers or an argv[0] Hermes basename at hermes_cli/profiles.py:1342-1352; the described interpreter-plus-shim argv therefore exits before the backend/profile checks. Desktop still resolves a PATH hermes command and spawns its pool backend directly with --profile <profile> serve at apps/desktop/electron/main.ts:3876-3919 and apps/desktop/electron/main.ts:8081-8113. The mount-only rail refresh also remains at apps/desktop/src/app/chat/sidebar/profile-switcher.tsx:205-209.
Problems
- The new focus/visibility listener behavior lacks a runtime UI regression test. The desktop jsdom test project is configured in
apps/desktop/vitest.config.ts:4-16; current search found noProfileRailtest.
Suggested changes
- Add a focused test for focus, visible/hidden
visibilitychange, and cleanup on unmount.
Automated hermes-sweeper review.
| // used elsewhere in the sidebar (see refreshProjects/refreshProjectTree). | ||
| useEffect(() => { | ||
| void refreshActiveProfile() | ||
|
|
There was a problem hiding this comment.
Please add a jsdom regression test for this listener pair: focus and visible visibilitychange should refresh, hidden visibility changes should not, and unmount should remove both listeners. The desktop UI test project already supports this (apps/desktop/vitest.config.ts:4-16).
teknium1
added
sweeper:risk-compatibility
… a tested hook Addresses review feedback from the hermes-sweeper (salvageability=high, keep_open): "The new focus/visibility listener behavior lacks a runtime UI regression test... no ProfileRail test." Rendering the full ProfileRail component for this would drag in drag-and-drop, dialogs, hotkeys, and i18n unrelated to what needs testing. Instead, extracted the focus/visibilitychange wiring into its own use-profile-rail-refresh-on-active hook, matching this exact directory's own established convention (use-profile-prewarm.ts is the same shape: a small side-effect hook pulled out of ProfileRail specifically so it's unit-testable in isolation). Added 6 tests covering exactly what the review asked for: refresh on mount, refresh on window focus, refresh on visibilitychange while visible, NO refresh on visibilitychange while hidden, listener cleanup on unmount, and no listener accumulation across repeated mount/unmount cycles. Verified the tests have real teeth: simulated the exact bug this PR originally fixed (dropped the cleanup return, leaving listeners attached after unmount) and confirmed 4 of 6 tests correctly fail against it -- including "no accumulate listeners" showing 7 calls instead of 1, the exact leaked-listener signature. Restored the real fix and all 6 pass. ProfileRail itself is otherwise unchanged in behavior -- this is a pure extraction (same effect, same dependencies, same cleanup), not a behavior change. Full sidebar test suite: 93 passed across 12 files (up from 87 across 11), 0 regressions. Python side unaffected: 158 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Added per the sweeper's review -- extracted the focus/visibilitychange wiring into its own Verified the tests have real teeth: simulated the exact original bug (dropped cleanup, listeners left attached after unmount) and confirmed 4 of 6 fail against it -- including "no accumulate listeners" showing 7 calls instead of 1. Restored the fix and all 6 pass.
|
…s on all 7 PRs Documents the real external engagement on the 7 upstream PRs filed 2026-07-26, and the 3 substantive follow-up fixes pushed in response: - NousResearch#72054 closed as superseded, but merged anyway via NousResearch#74139 (contributor CrowLoki's reconciliation with NousResearch#62026, credited via Co-authored-by). - NousResearch#72087, NousResearch#72151, NousResearch#72152, NousResearch#72153, NousResearch#72155, NousResearch#72164 all reviewed by the repo's automated sweeper -- keep_open/high on all 6. - Fixed NousResearch#72087 (payload-proportional test assertions, catching a future allowlist-regression risk flagged by both the sweeper and an independent contributor who measured it precisely on their own fork). - Fixed NousResearch#72152 (extracted ProfileRail's focus/visibilitychange wiring into a tested hook, matching the directory's own established use-profile-prewarm.ts pattern). - Rebased NousResearch#72155 past a real merge conflict (an unrelated upstream test-pruning pass removed 3 tests my diff's context touched). All fixes verified by simulating the exact regression each review was warning about and confirming the new tests catch it, then restoring the real fix. Also noted a real environment issue found this session: the `upstream` remote's SSH URL intermittently fails to connect from this network; a one-off HTTPS fetch into a separate ref works around it without touching the configured remote. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SummaryThree PRs address distinct layers of the ghost-profile complex: #52301 prevents Desktop from respawning a backend during deletion, #59554 prevents stale startup state from recreating a missing profile, and #72152 detects interpreter-launched profile backends while refreshing stale Desktop profile rails. Related pull requests
Duplicates#52301 and #72152 both relate to #52279 but are not duplicates: #52301 fixes delete-request routing and was superseded by merged #57329, whereas #72152 targets residual backend-process detection and stale rail state. #59554 addresses a separate stale-startup recreation path. Suggested consolidationKeep #59554 and #72152 open with salvage paths: preserve #59554's guarded handling of stale HERMES_HOME and active_profile state, and preserve #72152's narrowly validated interpreter-launched backend detection plus its now-tested rail refresh. Treat closed #52301 as the superseded reference implementation already carried by #57329; no remaining listed PR has evidence supporting closure as a duplicate of another listed PR. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I47368(["issue #47368 (closed)"])
I50175(["issue #50175 (closed)"])
I52279(["issue #52279 (closed)"])
I69934(["issue #69934 (closed)"])
P72152["PR #72152 (open)"]
P72152 -.->|partial| I47368
P72152 -->|best fix| I50175
P72152 -.->|partial| I52279
P72152 -.->|partial| I69934
class I47368 closed
class I50175 closed
class I52279 closed
class I69934 closed
class P72152 open
class P72152 best
class P72152 target
click I47368 "https://github.com/NousResearch/hermes-agent/issues/47368"
click I50175 "https://github.com/NousResearch/hermes-agent/issues/50175"
click I52279 "https://github.com/NousResearch/hermes-agent/issues/52279"
click I69934 "https://github.com/NousResearch/hermes-agent/issues/69934"
click P72152 "https://github.com/NousResearch/hermes-agent/pull/72152"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 4 issues in this complex. Each diff was read against this issue; Assessment working set: 26 kB of PR diffs, 29 kB of issue/PR text, 18 kB of discussion (14 comments), 19 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Merged via #88228 (rebase-merge, your 3 commits with authorship preserved — commit 1ed94d2 and parents on main). Thanks @adurham for tracing this past the earlier respawn fixes and for adding the hook tests per the sweeper review — the interpreter-exec'd shim gap was verified live before merge (a real Closing this PR since the branch was stale against current main; all substantive work landed via the salvage PR with your commits intact. |
4 participants
What does this PR do?
Fixes two independent gaps that leave zombie backend processes / stale UI
after a work profile is deleted.
Root Cause
hermes_cli/profiles.py's backend-process scanner (used byprofile deleteto find and terminate the profile's running backend) requiredargv[0]to resolve to an executable literally namedhermes.Electron's pool-backend spawn resolves the hermes console-script shim's
path and execs it via the interpreter directly (
python3 /path/to/hermes ...), soargv[0]reports aspython3and thescanner never matches — delete removes the profile's files but leaves
its live backend process running, still bound to a port via uvicorn.
This accumulates across repeated delete/recreate cycles (the original
report on Deleted profile respawns indefinitely, accumulating zombie backend processes #52279 observed 8+ zombie processes).
ProfileRailonly refreshed its cached profilelist once, on mount, so a delete/create/rename from a different
window or the CLI left a stale ghost entry in this window until
something unrelated triggered a refetch.
Related work already on
main(read before reviewing this)profile respawns indefinitely) — a different, non-overlapping
mechanism: routing profile-delete through the primary backend instead
of spawning a fresh pool backend.
ensure_hermes_home()— abackend spawned into a deleted profile's directory now raises
FileNotFoundErrorinstead of silently recreating the skeleton.This PR is not a duplicate of either. Verified directly: even with
both of those merged, a backend process that survives because of gap #1
above still holds a bound port — it just can no longer resurrect the
profile directory. That's real, live resource hygiene, not an
already-covered symptom. Gap #2 touches a different file (
ProfileRail/profile-switcher.tsx) than #57329's rail-refresh half (which touched theManage-Profiles view's own
profile.ts/index.tsx) and covers a distinctstaleness path — cross-window/cross-process, not same-window
delete-then-refresh (which #57329 already handles via the shared
$profilesatomProfileRailalready subscribes to).Changes Made
hermes_cli/profiles.py:_profile_bound_backend_pids()now alsorecognizes a python-interpreter
argv[0](python/python3/python3.12/pythonw(.exe)) exec'ing ahermes-named console-script shim viaargv[1].apps/desktop/src/app/chat/sidebar/profile-switcher.tsx:ProfileRailnow refreshes on window
focus/visibilitychange, matching theexisting pattern used elsewhere in the sidebar (
sidebar/index.tsx,use-background-sync.ts,star-map.tsx,use-gateway-boot.tsall usethe same focus + visibilitychange pattern).
Related Issue
Related to (but does not duplicate) the already-closed #52279.
Type of Change
How to Test
hermes profile delete <name> --yesfrom a terminal while the profile's Desktopbackend is running.
ps aux | grep hermes), still holding its bound port.of deletion.
pytest tests/hermes_cli/test_profiles.py -q— 156 passed.Checklist
documented above why this isn't a duplicate)
existing pattern with no dedicated test file to extend)