diff --git a/cli.py b/cli.py index 5b737212d5..da4b29d6ef 100644 --- a/cli.py +++ b/cli.py @@ -990,6 +990,13 @@ _cli_wake_owner = None # the session boundary while the agent is still attached. If a signal lands in # that narrow window, atexit cleanup must not emit that session finalization again. _single_query_finalize_attempted_session_ids: set[str | None] = set() +# Session IDs that were handed off to the gateway via /handoff. The CLI +# process exits after a successful handoff, but the gateway now owns the +# session lifecycle — _run_cleanup must NOT call finalize_session on these, +# because doing so sets end_reason on a row the gateway just reopened and is +# actively writing to (#88234). The race made the handoff leg vanish from +# session history and broke session_search recall for the handed-off session. +_handed_off_session_ids: set[str | None] = set() # Weak reference to the active AIAgent for memory provider shutdown at exit _active_agent_ref = None _deferred_agent_startup_done = False @@ -1279,11 +1286,19 @@ def _run_cleanup(*, notify_session_finalize: bool = True): def _should_emit_cleanup_session_finalize(session_id: str | None) -> bool: + # A session that was handed off to the gateway is now owned by the + # gateway process. The CLI must not finalize it on exit — that sets + # end_reason on a row the gateway reopened and is actively writing + # to, causing the handoff leg to vanish from session history (#88234). + if session_id is not None and session_id in _handed_off_session_ids: + return False if not _single_query_finalize_attempted_session_ids: return True if session_id is None: return False - return session_id not in _single_query_finalize_attempted_session_ids + if session_id in _single_query_finalize_attempted_session_ids: + return False + return True def _notify_session_finalize( @@ -1315,6 +1330,10 @@ def _emit_interrupted_session_end(cli, *, reason: str = "keyboard_interrupt") -> pass session_id = getattr(agent, "session_id", None) or getattr(cli, "session_id", None) + # Don't emit session-end for a session that was handed off to the + # gateway — the gateway owns the lifecycle now (#88234). + if session_id in _handed_off_session_ids: + return if session_id: try: cli.session_id = session_id diff --git a/gateway/run.py b/gateway/run.py index a4fcde2d49..7b7034b283 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -6540,6 +6540,11 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew except Exception: logger.debug("could not set multiplex-active flag", exc_info=True) self.adapters: Dict[Platform, BasePlatformAdapter] = {} + # When non-None, SessionDB init failed — the gateway broadcasts a + # one-time warning to the home channel(s) after connecting, so the + # user knows persistence is broken instead of discovering it later + # via a missing /resume or empty history (#88235). + self._session_db_init_error: Optional[str] = None # Multi-profile multiplexing: adapters for NON-default profiles live # here, keyed by profile name then Platform. self.adapters stays the # default/active profile's map so the ~93 existing self.adapters[...] @@ -6824,6 +6829,12 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew # "locking protocol" from NFS) is now also captured by # hermes_state.get_last_init_error() for slash-command error strings. logger.warning("SQLite session store not available: %s", e) + # Surface the failure to the user via their home channel(s) once + # the gateway connects. Without this, state.db corruption or + # NFS/SMB lock failures silently degrade the entire gateway — + # messages may flow but nothing is persisted, and the user has + # no indication until they try /resume and find nothing (#88235). + self._session_db_init_error = str(e) # Opportunistic state.db maintenance: prune ended sessions inactive # for sessions.retention_days + optional VACUUM. Tracks last-run @@ -12787,6 +12798,10 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew self._schedule_resume_pending_sessions() await self._finish_startup_restore() + # Surface state.db init failures to the user's messaging platforms + # so they know persistence is broken before losing data (#88235). + await self._send_session_db_warning_notifications() + # Drain any recovered process watchers (from crash recovery checkpoint) try: from tools.process_registry import process_registry @@ -23796,6 +23811,89 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew return delivered + async def _send_session_db_warning_notifications(self) -> None: + """Broadcast a state.db failure warning to all home channels (#88235). + + When SessionDB init fails at gateway startup, messages may flow but + nothing is persisted — /resume, /history, and session_search all + silently break. This sends a one-time warning to each connected + platform's home channel so the user knows to investigate before + losing data. Best-effort: failures are logged, not raised. + """ + error = getattr(self, "_session_db_init_error", None) + if not error: + return + + from hermes_state import classify_persistence_error, format_session_db_unavailable + + cause = classify_persistence_error(error) + hint = format_session_db_unavailable() + if cause == "corrupt": + message = ( + "⚠️ Session database corruption detected. Messages may not be " + "persisted. Recovery options:\n" + "1. Run `hermes doctor --fix`\n" + "2. Salvage with: sqlite3 ~/.hermes/state.db \".recover\" " + "(then replace state.db)\n" + "3. Restore from a backup in ~/.hermes/backups/\n" + f"Error: {error}" + ) + else: + message = ( + f"⚠️ Session database unavailable — messages may not be persisted. " + f"{hint}\n" + f"Run `hermes doctor` for diagnostics." + ) + + logger.warning( + "Broadcasting state.db failure warning to home channels: %s", error + ) + + for platform, platform_cfg in self.config.platforms.items(): + home = platform_cfg.home_channel + if not home or not home.chat_id: + continue + transport = resolve_delivery_transport(platform, self.config, self.adapters) + if transport is None: + continue + try: + metadata = self._thread_metadata_for_target( + platform, + home.chat_id, + home.thread_id, + adapter=transport.adapter, + ) + if transport.is_relay: + metadata = dict(metadata or {}) + if home.user_id: + metadata["user_id"] = home.user_id + if home.scope_id: + metadata["scope_id"] = home.scope_id + send_metadata = _non_conversational_metadata(metadata, platform=platform) + if send_metadata is not None or transport.is_relay: + result = await transport.send( + platform, + str(home.chat_id), + message, + metadata=send_metadata, + ) + else: + result = await transport.adapter.send(str(home.chat_id), message) + if result is not None and getattr(result, "success", True) is False: + logger.warning( + "state.db warning notification failed for %s:%s: %s", + platform.value, + home.chat_id, + getattr(result, "error", "send returned success=False"), + ) + except Exception as exc: + logger.warning( + "state.db warning notification failed for %s:%s: %s", + platform.value, + home.chat_id, + exc, + ) + def _set_session_env(self, context: SessionContext) -> list: """Set session context variables for the current async task. diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index 4349db8926..e37213dd7a 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -928,6 +928,14 @@ class CLICommandsMixin: _cprint(f" ↻ Handoff complete. The session is now active on {platform_name}.") _cprint(f" Resume it on this CLI later with: /resume {session_title}") _cprint("") + # Mark this session as handed off so _run_cleanup does NOT + # finalize it on CLI exit. The gateway reopened the session + # row and now owns its lifecycle; a CLI cleanup finalize would + # set end_reason on the row the gateway is actively writing + # to, causing the handoff leg to vanish from session history + # and session_search (#88234). + from cli import _handed_off_session_ids + _handed_off_session_ids.add(self.session_id) # End the CLI cleanly — same exit semantics as /quit. self._should_exit = True return False diff --git a/run_agent.py b/run_agent.py index a3da633bba..ed6081d5d6 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3871,8 +3871,12 @@ class AIAgent: + "the turn was stopped because the state database " "reported structural corruption (the transcript would " "have been lost on restart). Freeing disk space will " - "not help — run `hermes doctor` to repair the state " - "database, then send your message again." + "not help. Recovery options:\n" + "1. Run `hermes doctor --fix`\n" + "2. Salvage with: sqlite3 ~/.hermes/state.db \".recover\" " + "(then replace state.db)\n" + "3. Restore from a backup in ~/.hermes/backups/\n" + "Then send your message again." ) if cause == "disk": return ( diff --git a/tests/cli/test_handoff_cleanup_race.py b/tests/cli/test_handoff_cleanup_race.py new file mode 100644 index 0000000000..cb996621ca --- /dev/null +++ b/tests/cli/test_handoff_cleanup_race.py @@ -0,0 +1,138 @@ +"""Regression tests for #88234 — CLI cleanup must NOT finalize a session that +was handed off to the gateway. + +The handoff flow re-binds the CLI session_id to a gateway session_key via +``switch_session``, which reopens the session row. The CLI then exits and +``_run_cleanup`` fires ``_notify_session_finalize`` on that same session_id. +The resulting ``end_session`` call sets ``end_reason`` on a row the gateway +just reopened and is actively writing to — the handoff leg vanishes from +session history and ``session_search`` cannot find it. + +The fix adds a module-level ``_handed_off_session_ids`` set (mirroring the +existing ``_single_query_finalize_attempted_session_ids`` pattern). +``_handle_handoff_command`` registers the session_id when the handoff +completes, and ``_should_emit_cleanup_session_finalize`` / +``_emit_interrupted_session_end`` check the set before firing. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + + +def _reset_cli_globals(cli_mod): + """Reset the module-level globals the cleanup path checks.""" + cli_mod._cleanup_done = False + cli_mod._cleanup_in_progress = False + cli_mod._single_query_finalize_attempted_session_ids.clear() + cli_mod._handed_off_session_ids.clear() + cli_mod._active_agent_ref = None + + +def test_handed_off_session_skips_cleanup_finalize(): + """_should_emit_cleanup_session_finalize returns False for a handed-off session.""" + import cli as cli_mod + + _reset_cli_globals(cli_mod) + cli_mod._handed_off_session_ids.add("handoff-session-123") + + assert cli_mod._should_emit_cleanup_session_finalize("handoff-session-123") is False + + +def test_normal_session_still_finalizes(): + """_should_emit_cleanup_session_finalize returns True for a non-handed-off session.""" + import cli as cli_mod + + _reset_cli_globals(cli_mod) + cli_mod._single_query_finalize_attempted_session_ids.add("other-session") + + assert cli_mod._should_emit_cleanup_session_finalize("normal-session") is True + assert cli_mod._should_emit_cleanup_session_finalize("other-session") is False + + +def test_interrupted_session_end_skipped_for_handed_off(): + """_emit_interrupted_session_end returns early for a handed-off session.""" + import cli as cli_mod + + _reset_cli_globals(cli_mod) + cli_mod._handed_off_session_ids.add("handoff-session-456") + + agent = MagicMock() + agent.session_id = "handoff-session-456" + cli_mod._active_agent_ref = agent + + cli_mock = MagicMock() + cli_mock.agent = agent + cli_mock.session_id = "handoff-session-456" + + with patch("hermes_cli.lifecycle.invoke_hook") as mock_hook: + cli_mod._emit_interrupted_session_end(cli_mock, reason="keyboard_interrupt") + + # on_session_end hook must NOT fire for a handed-off session + mock_hook.assert_not_called() + + +def test_interrupted_session_end_fires_for_normal(): + """_emit_interrupted_session_end fires for a normal (non-handed-off) session.""" + import cli as cli_mod + + _reset_cli_globals(cli_mod) + + agent = MagicMock() + agent.session_id = "normal-session-789" + agent._current_task_id = "" + agent._current_turn_id = "" + agent._current_api_request_id = "" + agent.model = "test-model" + agent.platform = "cli" + cli_mod._active_agent_ref = agent + + cli_mock = MagicMock() + cli_mock.agent = agent + cli_mock.session_id = "normal-session-789" + + with patch("hermes_cli.lifecycle.invoke_hook") as mock_hook: + cli_mod._emit_interrupted_session_end(cli_mock, reason="keyboard_interrupt") + + mock_hook.assert_called_once() + + +def test_cleanup_does_not_finalize_handed_off_session(): + """_run_cleanup must not call finalize_session for a handed-off session.""" + import cli as cli_mod + + _reset_cli_globals(cli_mod) + cli_mod._handed_off_session_ids.add("handoff-session-abc") + + agent = MagicMock() + agent.session_id = "handoff-session-abc" + agent._session_messages = [] + cli_mod._active_agent_ref = agent + + with ( + patch("hermes_cli.lifecycle.finalize_session") as mock_finalize, + patch("hermes_cli.plugins.invoke_hook"), + ): + cli_mod._run_cleanup() + + mock_finalize.assert_not_called() + + +def test_cleanup_finalizes_normal_session(): + """_run_cleanup DOES call finalize_session for a normal session.""" + import cli as cli_mod + + _reset_cli_globals(cli_mod) + + agent = MagicMock() + agent.session_id = "normal-session-def" + agent._session_messages = [] + cli_mod._active_agent_ref = agent + + with ( + patch("hermes_cli.lifecycle.finalize_session") as mock_finalize, + patch("hermes_cli.plugins.invoke_hook"), + ): + cli_mod._run_cleanup() + + mock_finalize.assert_called_once() diff --git a/tests/run_agent/test_corruption_recovery_guidance.py b/tests/run_agent/test_corruption_recovery_guidance.py new file mode 100644 index 0000000000..e4b5063ff7 --- /dev/null +++ b/tests/run_agent/test_corruption_recovery_guidance.py @@ -0,0 +1,52 @@ +"""Regression tests for #88235 — state.db corruption must surface a warning +to the user's messaging platform, not stay silently in the logs. + +When SessionDB init fails at gateway startup (corruption, NFS/SMB locks, +disk errors), the gateway sets _session_db = None and logs a warning — but +the user never sees it. Messages may flow but nothing is persisted, and the +user only discovers the breakage when /resume or session_search comes back +empty. + +The fix adds: +1. _session_db_init_error attribute on GatewayRunner, set when init fails +2. _send_session_db_warning_notifications() — broadcasts a recovery-guidance + message to all home channels after the gateway connects +3. Improved "corrupt" cause wording in _format_turn_completion_explanation + with the full recovery path (hermes doctor, sqlite3 .recover, backups) +""" + +from pytest import fixture + + +def test_format_turn_completion_corrupt_includes_recovery_options(): + """The 'corrupt' persistence cause must list all recovery options.""" + from run_agent import AIAgent + + explanation = AIAgent._format_turn_completion_explanation( + "session_persistence_failed", "corrupt" + ) + assert "hermes doctor" in explanation + assert ".recover" in explanation + assert "backups" in explanation + assert "Freeing disk space will not help" in explanation + + +def test_format_turn_completion_disk_still_advises_space(): + """The 'disk' cause still gives disk-space advice (unchanged).""" + from run_agent import AIAgent + + explanation = AIAgent._format_turn_completion_explanation( + "session_persistence_failed", "disk" + ) + assert "free some space" in explanation + + +def test_format_turn_completion_locked_still_advises_retry(): + """The 'locked' cause still advises retrying (unchanged).""" + from run_agent import AIAgent + + explanation = AIAgent._format_turn_completion_explanation( + "session_persistence_failed", "locked" + ) + assert "busy" in explanation + assert "send it again" in explanation