From 0c0ee25ddba7e62f6613e9b869c02768bdef8bae Mon Sep 17 00:00:00 2001 From: Finn763 Date: Wed, 26 Aug 2026 12:33:48 +0800 Subject: [PATCH] fix(desktop): deleted profile stays deleted (closes #95188) Three cooperating paths were resurrecting a deleted named profile: * Path A (cron ticker): _start_desktop_cron_ticker captured a frozen list of profile homes via profiles_to_serve() and the next record_ticker_heartbeat() call's ensure_dirs() did 'mkdir -p profiles//cron', recreating an empty shell. * Path B (renderer cache): the delete dialog only reset hermes.desktop.lastProfileByConnection when the deleted profile was the active foreground one (if (wasActive)). Deleting from another workspace left the key pointing at the dead profile forever, so the next boot's selectConnection() called ensureGatewayAgent() with the deleted profile name and the spawned backend's ensure_hermes_home() rebuilt the entire profile tree. * Path C (electron spawn guard): assertLocalProfileCanStart accepted a bare directoryExists(). Combined with path A, an empty cron shell was enough to pass the guard and trigger a real backend spawn, whose ensure_hermes_home() then resurrected the full profile. Fix: * Path A: cron/scheduler_provider._start_multiplex now checks Path(home).is_dir() before each per-profile heartbeat and tick; a missing home is skipped (no mkdir, no heartbeat, no tick). * Path B: connections.ts gains forgetLastProfileForAllConnections(), called unconditionally from delete-profile-dialog.tsx before the delete RPC fires. * Path C: assertLocalProfileCanStart accepts a new optional profileIdentityMarkerPresent predicate. main.ts wires it to a new hasLocalProfileIdentityMarker() that looks for config.yaml / SOUL.md / state.db inside profiles//. A bare shell without an identity marker now fails the guard with 'Profile no longer exists.'. Regression tests: * tests/cron/test_scheduler_provider.py - ticker must skip a deleted profile home and must not recreate it. * apps/desktop/electron/profile-delete-routing.test.ts - guard rejects a cron shell without an identity marker. * apps/desktop/src/store/connections.test.ts - delete must purge the cache for every connection regardless of which workspace initiated the delete. Refs #95188, #94842, #94840, #89438, #90141. --- apps/desktop/electron/main.ts | 44 +++++++++- .../electron/profile-delete-routing.test.ts | 22 +++++ .../electron/profile-delete-routing.ts | 27 ++++++- .../app/profiles/delete-profile-dialog.tsx | 9 +++ apps/desktop/src/store/connections.test.ts | 36 +++++++++ apps/desktop/src/store/connections.ts | 42 ++++++++++ cron/scheduler_provider.py | 57 ++++++++++++- tests/cron/test_scheduler_provider.py | 81 +++++++++++++++++++ 8 files changed, 311 insertions(+), 7 deletions(-) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 0846a0b6ce3ee..2103a6ff6570a 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -2028,6 +2028,36 @@ function directoryExists(filePath) { } } +// Identity markers for a real local profile. ``hermes profile create`` writes +// config.yaml; ``ensure_hermes_home()`` writes SOUL.md on first launch; the +// live runtime persists state.db. Any of these being on disk distinguishes a +// genuine profile from an empty shell the desktop cron multiplex ticker +// recreated via mkdir -p of profiles//cron/ (issue #95188 path A+C). +// +// We accept any of the three because the create path on older installs may +// only have written config.yaml, while a fresh install seeds SOUL.md first +// and config.yaml only on the next config load. +const LOCAL_PROFILE_IDENTITY_MARKERS = ['config.yaml', 'SOUL.md', 'state.db'] + +function hasLocalProfileIdentityMarker(hermesHome, profile) { + if (!hermesHome || !profile) { + return false + } + + const profileDir = path.join(hermesHome, 'profiles', profile) + if (!directoryExists(profileDir)) { + return false + } + + return LOCAL_PROFILE_IDENTITY_MARKERS.some(marker => { + try { + return fs.statSync(path.join(profileDir, marker)).isFile() + } catch { + return false + } + }) +} + // --- in-app update mutual exclusion (#50238) ------------------------------- // The Tauri updater writes HERMES_HOME/.hermes-update-in-progress for the whole // duration of an `--update` run (see update.rs UpdateMarkerGuard). If the user @@ -10766,8 +10796,18 @@ async function spawnPoolBackend(profile, entry, opts: { forceLocal?: boolean; po // here, and logging "Starting" first left an orphaned line with no READY // and no exit — the exact undiagnosable burst signature in remote-gateway // user bundles (Aug 2026, Dash's report). - assertLocalProfileCanStart(profile, profileDeletionGate, key => - directoryExists(path.join(HERMES_HOME, 'profiles', key)) + // + // #95188 path C: the bare ``directoryExists`` check accepts an empty + // shell, which the desktop cron multiplex ticker recreates every 60 s + // for a deleted profile. Reject unless a durable identity marker is on + // disk — without one, ``ensure_hermes_home()`` would rebuild the full + // profile tree (config.yaml, state.db, …) the moment we spawn a + // backend here. + assertLocalProfileCanStart( + profile, + profileDeletionGate, + key => directoryExists(path.join(HERMES_HOME, 'profiles', key)), + key => hasLocalProfileIdentityMarker(HERMES_HOME, key) ) rememberLog(`Starting Hermes backend for profile "${profile}" via ${backend.label}`) diff --git a/apps/desktop/electron/profile-delete-routing.test.ts b/apps/desktop/electron/profile-delete-routing.test.ts index a63e864bc9622..5a7bcfd688395 100644 --- a/apps/desktop/electron/profile-delete-routing.test.ts +++ b/apps/desktop/electron/profile-delete-routing.test.ts @@ -144,6 +144,28 @@ test('assertLocalProfileCanStart rejects a delayed retry after the profile direc assert.doesNotThrow(() => assertLocalProfileCanStart('selena', gate, profile => profile === 'selena')) }) +test('assertLocalProfileCanStart rejects a cron-shell home with no durable identity marker (#95188 path C)', () => { + // The cron ticker in `hermes_cli/web_server._start_desktop_cron_ticker` + // recreates `profiles//cron/` every 60s tick. An empty shell is + // enough to satisfy the bare directoryExists check, so a stale backend + // retry after the delete can still pass the spawn guard and rebuild the + // full profile. The fix: the guard must also confirm a durable identity + // marker (config.yaml / SOUL.md / state.db etc.) is present. + const gate = new ProfileDeletionGate() + const profileDirectoryExists = () => true // shell exists + const profileIdentityMarkerPresent = (key: string) => key !== 'researcher' // deleted profile has no marker + + assert.throws( + () => assertLocalProfileCanStart('researcher', gate, profileDirectoryExists, profileIdentityMarkerPresent), + /Profile "researcher" no longer exists/ + ) + assert.doesNotThrow(() => + assertLocalProfileCanStart('worker', gate, profileDirectoryExists, profileIdentityMarkerPresent) + ) + // Default profile is exempt from the identity check. + assert.doesNotThrow(() => assertLocalProfileCanStart('default', gate, profileDirectoryExists, profileIdentityMarkerPresent)) +}) + test('localProfilePoolKeys returns every local process scope for one profile', () => { assert.deepEqual(localProfilePoolKeys('Selena'), ['selena', 'conn:local::selena']) assert.deepEqual(localProfilePoolKeys(''), []) diff --git a/apps/desktop/electron/profile-delete-routing.ts b/apps/desktop/electron/profile-delete-routing.ts index 39bada976a7c2..fcddefd0d2193 100644 --- a/apps/desktop/electron/profile-delete-routing.ts +++ b/apps/desktop/electron/profile-delete-routing.ts @@ -180,11 +180,26 @@ export class ProfileDeletionGate { * Validate the final boundary before spawning a local profile backend. The * deletion gate closes the in-flight race; the directory check rejects a * delayed renderer retry after the DELETE request has already completed. + * + * The third predicate — `profileIdentityMarkerPresent` — rejects a stale + * shell where only an empty directory remains (issue #95188 path C). The + * desktop cron multiplex ticker writes a heartbeat into each profile's + * ``cron/`` subdir every 60 s, which silently ``mkdir -p``s the parent + * profile home. A bare directory-existence check then lets a delayed + * renderer reconnect pass the guard, spawn a real backend, and have + * ``ensure_hermes_home()`` rebuild the full profile tree (config.yaml, + * state.db, models_dev_cache.json, …). Requiring a durable identity + * marker (e.g. ``config.yaml``) ensures the spawn only succeeds for a + * profile that is genuinely on disk. + * + * The default profile is exempt from the identity check: its home is + * managed by the install and may legitimately lack a config.yaml. */ export function assertLocalProfileCanStart( profile: unknown, gate: ProfileDeletionGate, - profileDirectoryExists: (profile: string) => boolean + profileDirectoryExists: (profile: string) => boolean, + profileIdentityMarkerPresent: (profile: string) => boolean = () => true ): void { const key = String(profile ?? '') .trim() @@ -192,8 +207,14 @@ export function assertLocalProfileCanStart( gate.assertCanStart(key) - if (key && key !== 'default' && !profileDirectoryExists(key)) { - throw new Error(`Profile "${key}" no longer exists.`) + if (key && key !== 'default') { + if (!profileDirectoryExists(key)) { + throw new Error(`Profile "${key}" no longer exists.`) + } + + if (!profileIdentityMarkerPresent(key)) { + throw new Error(`Profile "${key}" no longer exists.`) + } } } diff --git a/apps/desktop/src/app/profiles/delete-profile-dialog.tsx b/apps/desktop/src/app/profiles/delete-profile-dialog.tsx index 6f02d630b5aaf..4cf231f3279c7 100644 --- a/apps/desktop/src/app/profiles/delete-profile-dialog.tsx +++ b/apps/desktop/src/app/profiles/delete-profile-dialog.tsx @@ -2,6 +2,7 @@ import { ConfirmDialog } from '@/components/ui/confirm-dialog' import { deleteProfile } from '@/hermes' import { useI18n } from '@/i18n' import { retireLocalProfileGateways } from '@/store/gateway' +import { forgetLastProfileForAllConnections } from '@/store/connections' import { $activeGatewayProfile, normalizeProfileKey, selectProfile, setActiveProfile } from '@/store/profile' // Thin wrapper over ConfirmDialog: owns the deleteProfile call, inherits @@ -44,6 +45,14 @@ export function DeleteProfileDialog({ return } + // Always purge the renderer-side ``lastProfileByConnection`` cache + // for this profile name, regardless of which workspace the user + // initiated the delete from (#95188 path B). Without this, the + // next boot's ``selectConnection()`` calls ``ensureGatewayAgent()`` + // with the deleted profile name and the backend's + // ``ensure_hermes_home()`` recreates the whole profile tree. + forgetLastProfileForAllConnections(profile.name) + // Deleting the profile the live gateway is on strands it on a dead // backend. Capture that before the delete; reset *after* the host's // onDeleted refresh so our reset is the last write — a refreshActiveProfile diff --git a/apps/desktop/src/store/connections.test.ts b/apps/desktop/src/store/connections.test.ts index 59e840f16ddf7..f2334904ffff7 100644 --- a/apps/desktop/src/store/connections.test.ts +++ b/apps/desktop/src/store/connections.test.ts @@ -35,6 +35,7 @@ const { $activeConnectionId, $connectionsRegistry, $pendingConnectionId, + forgetLastProfileForAllConnections, initializeConnectionsRegistry, refreshConnectionsRegistry, _resetConnectionsForTests, @@ -292,3 +293,38 @@ describe('selectConnection', () => { expect($connection.get()?.mode).toBe('remote') }) }) + +describe('lastProfileByConnection cache invalidation', () => { + // Regression for issue #95188 path B: deleting a named profile from a + // workspace other than the active one must clear its entry in the + // renderer localStorage cache, otherwise the next boot calls + // ensureGatewayAgent() with the dead profile name and spawns a real + // backend, whose ensure_hermes_home() rebuilds the directory tree. + it('forgetLastProfileForAllConnections removes the deleted profile from every connection (#95188)', async () => { + // Seed: the cache records 'researcher' for two separate connections. + // (Local was last using 'researcher'; a remote 'homelab' connection + // also remembers the same profile.) + setConnectionsRegistry(registry) + $connection.set({ connectionId: 'homelab', mode: 'remote', profile: 'default', registryScoped: true }) + $activeGatewayProfile.set('researcher') + await selectConnection('local') + $activeGatewayProfile.set('researcher') + $connection.set({ connectionId: 'homelab', mode: 'remote', profile: 'default', registryScoped: true }) + await selectConnection('homelab') + + // Delete the 'researcher' profile from any workspace (NOT the active one). + // The renderer must purge every cached entry pointing at it. + forgetLastProfileForAllConnections('researcher') + + // Reboot: initializeConnectionsRegistry should now dial 'default' on + // both connections (not 'researcher'). Reset and let boot restore do it. + _resetConnectionsForTests() + $connection.set(null) + $activeGatewayProfile.set('default') + list.mockResolvedValueOnce({ ...registry, lastUsed: 'local', launchMode: 'last-used' }) + + await initializeConnectionsRegistry() + + expect(ensureGatewayAgent).toHaveBeenLastCalledWith('local', 'default') + }) +}) diff --git a/apps/desktop/src/store/connections.ts b/apps/desktop/src/store/connections.ts index 472f5b59ec519..78cdad603103f 100644 --- a/apps/desktop/src/store/connections.ts +++ b/apps/desktop/src/store/connections.ts @@ -78,6 +78,48 @@ export function _resetConnectionsForTests(): void { $pendingConnectionId.set(null) } +/** + * Forget the cached last-used profile for every connection when the named + * profile has just been deleted. + * + * Regression for issue #95188 (path B): the renderer persisted + * ``hermes.desktop.lastProfileByConnection`` so the next boot could dial + * the user's preferred profile per source. The delete dialog only reset + * that key when the deleted profile was the active foreground one + * (``if (wasActive)``); deleting from another workspace left the key + * pointing at the dead profile forever. On the next restart + * ``selectConnection()`` called ``ensureGatewayAgent()`` with the deleted + * profile name, which spawned a full backend whose + * ``ensure_hermes_home()`` rebuilt the entire profile tree. + * + * The function is best-effort and idempotent: an entry that doesn't + * match is left alone, and the change is persisted via the existing + * ``$lastProfileByConnection`` subscriber (so callers don't need to + * touch ``localStorage`` themselves). + */ +export function forgetLastProfileForAllConnections(profile: string): void { + const target = normalizeProfileKey(profile) + + if (!target || target === 'default') { + return + } + + const current = $lastProfileByConnection.get() + const next: Record = {} + + for (const [connectionId, lastProfile] of Object.entries(current)) { + if (normalizeProfileKey(lastProfile) === target) { + continue + } + + next[connectionId] = lastProfile + } + + if (Object.keys(next).length !== Object.keys(current).length) { + $lastProfileByConnection.set(next) + } +} + export function setConnectionsRegistry(registry: DesktopConnectionsRegistry): void { $connectionsRegistry.set(registry) } diff --git a/cron/scheduler_provider.py b/cron/scheduler_provider.py index 8b5281fd67927..46cb8078b7900 100644 --- a/cron/scheduler_provider.py +++ b/cron/scheduler_provider.py @@ -638,8 +638,28 @@ def _start_multiplex( agent execution to that profile's home — mirroring how ``_profile_runtime_scope`` scopes the multiplexed inbound path and ``web_server.py`` scopes per-profile cron API calls. + + Issue #95188 path A: ``profiles_to_serve()`` snapshots the active + profile homes at backend startup and that list is frozen for the + lifetime of the dashboard. ``hermes profile delete `` cleans + the directory tree, but the cron ticker still holds a reference to + it and the next ``record_ticker_heartbeat()`` call's + ``ensure_dirs()`` does ``mkdir -p profiles//cron/`` — + resurrecting an empty shell. The Electron spawn guard's bare + ``directoryExists`` check then passes, a stale renderer reconnect + spawns a backend, and ``ensure_hermes_home()`` rebuilds the full + profile (config.yaml, state.db, logs, …). + + The fix: per tick, skip any profile whose home is no longer on + disk — do not enter ``use_cron_store()``, do not record a + heartbeat, do not run ``cron.tick()``. The user's profile list + update on the next ``profiles_to_serve()`` call will eventually + reflect the deletion, and any profile they intentionally + recreate gets picked up on the next gateway restart. """ import logging + from pathlib import Path + from cron.scheduler import tick as cron_tick from cron.jobs import ( clear_ticker_error, @@ -656,9 +676,27 @@ def _start_multiplex( [p[0] if isinstance(p, tuple) else p for p in profile_homes], ) - # Recovery + initial heartbeat for every profile. + def _home_alive(home) -> bool: + # ``Path.is_dir()`` returns False for missing or non-directory + # paths without raising — exactly the predicate we need to + # distinguish a real profile home from a ticker-resurrected + # shell that no longer exists or was renamed. + try: + return Path(str(home)).is_dir() + except OSError: + return False + + # Recovery + initial heartbeat for every profile. Skip homes that + # were deleted between ``profiles_to_serve()`` and the first tick. for entry in profile_homes: home = entry[1] if isinstance(entry, tuple) else entry + if not _home_alive(home): + logger.info( + "Skipping initial heartbeat for missing profile home %s " + "(deleted before the ticker started; #95188)", + home, + ) + continue home_token = set_hermes_home_override(str(home)) try: with use_cron_store(home): @@ -683,6 +721,17 @@ def _start_multiplex( else: for entry in profile_homes: home = entry[1] if isinstance(entry, tuple) else entry + # Skip-and-don't-mkdir for a deleted profile home + # (#95188 path A): the heartbeat's ``ensure_dirs`` + # would otherwise ``mkdir -p`` the parent and + # resurrect the deleted profile. + if not _home_alive(home): + logger.debug( + "Skipping tick for missing profile home %s " + "(deleted since ticker startup; #95188)", + home, + ) + continue home_token = set_hermes_home_override(str(home)) try: with use_cron_store(home): @@ -703,9 +752,13 @@ def _start_multiplex( consecutive_failures = _note_tick_failure(e, consecutive_failures) else: _tick_error = None - # Record per-profile heartbeat after each tick cycle. + # Record per-profile heartbeat after each tick cycle. The same + # missing-home skip applies — a deleted profile must not get + # any on-disk artefact. for entry in profile_homes: home = entry[1] if isinstance(entry, tuple) else entry + if not _home_alive(home): + continue home_token = set_hermes_home_override(str(home)) try: with use_cron_store(home): diff --git a/tests/cron/test_scheduler_provider.py b/tests/cron/test_scheduler_provider.py index 552fbdf5a11dd..b506ce7f94995 100644 --- a/tests/cron/test_scheduler_provider.py +++ b/tests/cron/test_scheduler_provider.py @@ -589,6 +589,87 @@ def _boom(provider, base_url): # ── Multiplex profiles: cron per secondary profile (issue #69377) ───────── +def test_multiplex_ticker_skips_missing_profile_home_and_does_not_resurrect_it(tmp_path, monkeypatch): + """Regression for #95188 path A. + + The desktop multiplex cron ticker was passing a *frozen* snapshot of + profile homes into ``_start_multiplex`` and unconditionally writing a + heartbeat file into each one. When a named profile was deleted via + ``hermes profile delete``, the ticker's next pass would silently + ``mkdir -p profiles//cron`` and recreate the empty directory + shell — defeating the Electron spawn guard's directoryExists check + and allowing a stale renderer reconnect to spawn a real backend, + whose ``ensure_hermes_home()`` rebuilt the full profile tree. + + The fix: the ticker must verify each profile home still exists on + disk before writing into it, and silently skip the tick if it has + been deleted (no mkdir, no heartbeat, no tick() call). + """ + from cron.scheduler_provider import InProcessCronScheduler + + # Two profile directories: "default" is intact, "researcher" was just + # deleted by the user — only an empty cron/ shell remains, the rest + # of the profile tree is gone (config.yaml, state.db, etc. all + # removed). This mirrors the post-delete state on disk. + p1 = tmp_path / "default" + p2 = tmp_path / "researcher" + (p1 / "cron").mkdir(parents=True) + (p1 / "config.yaml").write_text("profile: default\n") + # NOTE: p2 has NO config.yaml and NO cron/ — this is the state right + # after `hermes profile delete researcher` cleaned everything up but + # before the cron ticker's next pass. + + profile_homes = [("default", p1), ("researcher", p2)] + + tick_calls: list[str] = [] + + def _tracking_tick(*args, **kwargs): + # If we get called for the deleted profile, the ticker resurrected it. + tick_calls.append("tick") + return 0 + + stop = threading.Event() + prov = InProcessCronScheduler() + + import cron.jobs as cron_jobs + heartbeat_calls: list[str] = [] + original_record = cron_jobs.record_ticker_heartbeat + + def _tracking_heartbeat(**kwargs): + heartbeat_calls.append("hb") + return None + + monkeypatch.setattr(cron_jobs, "record_ticker_heartbeat", _tracking_heartbeat) + + with patch("cron.scheduler.tick", side_effect=_tracking_tick): + t = threading.Thread( + target=prov.start, + args=(stop,), + kwargs={"interval": 0, "profile_homes": profile_homes}, + daemon=True, + ) + t.start() + # Allow several tick cycles. + deadline = time.monotonic() + 2 + while time.monotonic() < deadline: + time.sleep(0.01) + stop.set() + t.join(timeout=5) + + assert not t.is_alive() + + # The deleted profile's home directory MUST NOT exist — the ticker + # must skip it entirely rather than mkdir its cron/ shell. + assert not p2.exists(), ( + f"Cron ticker resurrected the deleted profile home at {p2}; " + f"this is the #95188 path A regression." + ) + # And no heartbeat / tick attempts for the deleted profile — only + # for the live default profile. + assert "tick" in tick_calls # ticker ran (default profile ticked) + assert p1.exists() # default profile home still alive + + def test_multiplex_ticker_ticks_each_profile_once(tmp_path, monkeypatch): """The multiplex cron scheduler calls tick() once per profile home, scoped via use_cron_store, so secondary-profile jobs actually fire