Conversation
b0dd2c0 to
ccb72e4
Compare
Renaming a profile moved profiles/<old>/ to profiles/<new>/, so the row DATA travelled with the directory, but the profile name is also baked into keys/values the move left untouched: session keys (agent:<old>:* namespace), sessions.profile_name (fail-closed owner ladder / Desktop sidebar scope / @session: deep links), sessions.origin_json.profile, gateway_heartbeats.profile, delivery_obligations (session_key + adapter_profile), telegram_dm_topic_* profile_name bindings, and the gateway_routing index. Left stale, every inbound event on a chat keyed to the old name resolved to a profile that no longer exists — flooding errors.log with "Profile <old> does not exist ... falling back to global HERMES_HOME" every few seconds — and renamed sessions dropped out of the sidebar / broke their deep links. The routing index is held in memory by a live multiplexer and written back periodically, so a CLI-side DB rewrite alone is clobbered. Fix in layers: - SessionDB.rekey_profile_state: atomic durable rewrite of the state.db tables, matching the agent:<name>: namespace by exact prefix (substr, not LIKE — '_' is a legal profile-name character and a LIKE wildcard), rewriting the profile inside routing/origin JSON, and REFUSING on a target collision (routing rows or telegram bindings) instead of silently merging. - SessionStore.rekey_profile_routing: rekey the in-memory routing index (keys + origin.profile) then persist — the half a DB write cannot reach. Raises on a target-key collision before mutating. - Control verb migrate-profile-identity (params-carrying; the socket passes params only to handlers that declare them, bare handlers unchanged) so a live gateway rekeys its in-memory copy AND both durable stores (routing home + the renamed profile's own state.db). - rename_profile calls the verb when a multiplexer is live and, if it fails, does NOT fall back to a racing CLI-side write: it prints a warning telling the operator to restart the gateway and retry. With no live gateway it performs the durable rewrite itself (safe: nothing else holds the store open). Checkpoints keyed by the profile's workdir path are a known related gap, tracked separately, not addressed here. Tests: rekey_profile_state (all tables, routing/origin JSON, collisions, idempotent, no-op), rekey_profile_routing (namespace + origin, no-op, no overwrite), control verb param passing, and rename end-to-end for both the live-gateway (delegates, refuses unsafe fallback) and no-gateway (durable rewrite) paths.
ccb72e4 to
9143428
Compare
Review: fix(profiles): migrate session/routing identity on profile renameSummary What changed
Strengths
Findings
Verdict Reviewed using Hermes-Agent |
A rename under a live multiplexer that could not reach the control verb warned and stopped there, leaving the operator with no way to finish: the rename cannot be repeated (profiles/<old> is gone) and the CLI deliberately never rewrites the routing DB a live gateway holds in memory. - `hermes profile migrate-identity <old> <new>`: retries the migration — delegates to the gateway control verb while a multiplexer is live, performs the durable rewrite of both state DBs when none is. Idempotent, and exits non-zero naming the offending database on a collision, a lock, or a partial failure. Only the name format and the existence of the new profile are checked; the old profile directory is expected to be gone. - An older gateway that does not implement the verb is reported as such (`identify` answers while the migrate verb does not), not as "no gateway". - `_migrate_profile_identity` returns an explicit success/failure result so the command can set its exit code; the rename warning now names the exact invocation. - A failed control answer keeps the raw payload when it carries no reason field. - The offline failure branch called `click.echo` in a module that never imports `click`: a failed second database raised NameError instead of printing its warning.
|
Both findings addressed — thanks, the failed-live-migration path was a real gap. Which option, and why not fall-through We kept the ownership rule rather than falling through to the durable rewrite under a live gateway: the
The rename warning now ends with the exact invocation: Raw answer (second finding) The reason is now extracted as Same pass, one adjacent bug: the offline failure branch called Tests — two new invariants, both proven red on this branch's base (implementation stashed) and green with it:
|
|
Salvaged into #112653 with your commit cherry-picked (authorship preserved). That PR lands #111927 (rename identity migration) together with the #112592 atomic-writer cluster (#112594 first-in, #112601 caller sites, #112596 sweep) on current |
Bug Description
Renaming a multiplexed profile leaves the old profile name baked into persisted session/routing
state, so the gateway keeps routing to a profile that no longer exists. With a bot still connected
to the renamed profile's chats,
errors.logfills withProfile 'foo' does not exist ... falling back to global HERMES_HOMEevery few seconds, and renamed sessions drop out of the Desktop sidebar/ break their
@session:deep links (sessions.profile_namestill names the old profile).Fixes #111926
Root Cause
rename_profilemovesprofiles/<old>/toprofiles/<new>/(row data travels with the directory)and already migrates the alias, Honcho blocks,
active_profile, and the live multiplexer's adapters.But the profile name is also baked into keys/values the move does not touch, none of which were
migrated:
agent:<old>:*— the routing index (gateway_routingin the rootstate.db+sessions.jsonmirror) and the renamed profile's ownsessions.session_keyrows.sessions.profile_name— read by the fail-closed owner ladder, Desktop sidebar scope, and@session:<profile>/<id>deep links.gateway_heartbeats.profileanddelivery_obligations(session_key + adapter_profile).The routing index is load-bearing: a live multiplexer holds it in memory (
SessionStore._entries)and writes it back periodically, so a direct DB rewrite alone is clobbered on the next save — the
old namespace resurfaces until the gateway restarts.
This is the session-store sibling of the earlier ghost-profile rename fix (that covered the
filesystem/adapter layer; this covers the persisted identity layer).
Fix
Migrate profile-name-keyed state, split by who owns the store:
SessionDB.rekey_profile_state(old, new)— durable rewrite of all fourstate.dbtables(session-key namespaces,
profile_name, heartbeats, delivery rows, and the routing index key +the profile embedded in its JSON payload). Idempotent; skips
delivery_obligationswhen theledger has not created it yet.
SessionStore.rekey_profile_routing(old, new)— rekey the in-memory routing index (keys +origin.profile) and persist, the half a DB write cannot reach. Leaves a pre-existing new-namekey untouched rather than merging.
migrate-profile-identity(carrying{old,new}) so a live gateway does both.The socket protocol now passes
paramsonly to handlers that declare aparamsargument; barehandlers (identify/status/rescan/pause) are called with no args exactly as before.
rename_profilecalls the verb when a multiplexer is live; otherwise it performs the durable DBrewrite itself (safe — nothing else holds the store open). Never fatal to the rename.
hermes profile migrate-identity <old> <new>— standalone retry for the case where the live verbcould not migrate: the rename cannot simply be repeated (
profiles/<old>is gone) and the CLI mustnot rewrite a routing DB a live gateway holds in memory. Delegates to the verb while a multiplexer
is live, performs the durable rewrite itself when none is, is idempotent, and exits non-zero naming
the database on a collision, a lock, or a partial failure. A gateway that is running but does not
implement the verb (
identifyanswers, the migrate verb does not) is reported as such._migrate_profile_identityreturns that success/failure result so the command can set its exit code,and the rename warning now names the exact invocation.
Checkpoints keyed by the profile's workdir path are a known related gap, called out in the issue and
left for a separate change.
How to Verify
gateway.multiplex_profiles: true) serving a secondaryfoowith a chat.hermes profile rename foo bar.errors.logno longer logsProfile 'foo' does not exist;state.db/sessions.jsoncontainagent:bar:*keys andprofile_name = 'bar', none underfoo. No gateway restart required.hermes profile migrate-identity foo bar. That command exits non-zero while the gateway stilldeclines, exits 0 once it has been restarted (which reloads the routing index, so the live rekey
lands) or stopped (the durable rewrite is then safe), and running it twice in a row rekeys nothing
the second time.
Test Plan
tests/hermes_state/test_rekey_profile_state.py— all four tables, routing JSON key +embedded profile, idempotent, no-op on equal/empty names.
tests/gateway/test_rekey_profile_routing.py— in-memory namespace +origin.profilerewrite, no-op, no-overwrite of an existing target key.
tests/gateway/test_control_socket.py::test_verb_handler_receives_params— params reach adeclaring handler; bare handlers unaffected.
tests/hermes_cli/test_profiles.py— rename end-to-end for both the live-gateway (delegatesto the verb, no direct DB write) and no-gateway (durable rewrite lands) paths.
tests/hermes_cli/test_profiles.py— review follow-up: the retry command repairs thefailed-live-migration end state in both stores and is idempotent; a live gateway answering with an
unusable payload exits non-zero, quotes the raw answer, names the retry command, and still performs
no direct DB rewrite.
tests/hermes_cli/andtests/gateway/test_control_socket.pyshow no new failures — the 13failures in the
test_update_*files are pre-existing on this branch's base (identical with theimplementation stashed).
ruffclean.Risk Assessment
Low–Medium. New code paths only run during
hermes profile rename; the control verb is additive andthe protocol change is backward-compatible (bare handlers keep their zero-arg signature). The
in-memory rekey reuses
SessionStore's own lock and persist path. Blast radius is confined to therename flow and the new
rekey_*methods.