fix(approval): stop a pending approval dying on the session idle TTL (#13478)

autobot-backend/services/agent_terminal/approval_ttl_13478_test.py

@@ -0,0 +1,173 @@
+# Copyright 2025-2026 mrveiss
+# SPDX-License-Identifier: Apache-2.0
+"""A pending approval must not die on the session's idle TTL (#13478).
+
+#13216 reported that approvals "expire after 30 minutes". Reading the code, the
+30-minute poll budget only bounds the *turn* — it never cleared the pending
+approval, and `_approve_command_internal` executes on approve with no coroutine
+waiting. The real expiry was one layer down and an hour later:
+
+    session_manager.py:388   await self.redis_client.setex(key, TTL_1_HOUR, ...)
+
+`set_pending_approval` re-persists immediately (`service.py:213`), so the clock
+started when the prompt was raised and nothing refreshed it while the human
+thought about it. An hour on, `get_session` missed and the approve endpoint
+returned `Session not found` — the approval was unrecoverable.
+
+The second half is subtler. `_restore_pending_approval` rebuilds the state from
+chat history and is documented as surviving restarts, but it ran only inside
+`create_session`, which mints a **new** session_id. The approve endpoint is
+addressed by the **old** id — what the persisted approval message and the GUI
+button both carry — so the restore existed and could not be reached from the one
+path that needed it.
+"""
+
+from __future__ import annotations
+
+import json
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+
+pytestmark = pytest.mark.asyncio
+
+
+def _manager():
+    from services.agent_terminal.session_manager import SessionManager
+
+    mgr = SessionManager.__new__(SessionManager)
+    mgr.sessions = {}
+    mgr.redis_client = AsyncMock()
+    mgr.chat_history_manager = None
+    import asyncio
+
+    mgr._sessions_lock = asyncio.Lock()
+    return mgr
+
+
+def _session(session_id="sess-1", conversation_id="conv-1", pending=None):
+    from services.agent_terminal.models import AgentSessionState, AgentTerminalSession
+    from services.command_approval_manager import AgentRole
+
+    s = AgentTerminalSession(
+        session_id=session_id,
+        agent_id="agent-1",
+        agent_role=AgentRole.CHAT_AGENT,
+        conversation_id=conversation_id,
+    )
+    if pending:
+        s.pending_approval = pending
+        s.state = AgentSessionState.AWAITING_APPROVAL
+    return s
+
+
+def _setex_calls(mgr):
+    return [c.args for c in mgr.redis_client.setex.call_args_list]
+
+
+# --- D1: the TTL ------------------------------------------------------------
+
+
+async def test_an_idle_session_still_expires_in_an_hour():
+    """The GC this TTL exists for must keep working."""
+    from constants.ttl_constants import TTL_1_HOUR
+
+    mgr = _manager()
+    await mgr._persist_session(_session())
+
+    key, ttl, _ = _setex_calls(mgr)[0]
+    assert key == "agent_terminal:session:sess-1"
+    assert ttl == TTL_1_HOUR, "an abandoned session must still be collected"
+
+
+async def test_a_session_awaiting_approval_gets_a_long_ttl():
+    """The bug: an hour after the prompt, approving returned 'Session not found'."""
+    from constants.ttl_constants import TTL_1_HOUR
+    from services.agent_terminal.session_manager import APPROVAL_PENDING_SESSION_TTL
+
+    mgr = _manager()
+    await mgr._persist_session(_session(pending={"command": "ls -la"}))
+
+    _, ttl, _ = _setex_calls(mgr)[0]
+    assert ttl == APPROVAL_PENDING_SESSION_TTL
+    assert ttl > TTL_1_HOUR, (
+        "a session waiting on a human is not idle — the thing it waits for is a "
+        "person, and an hour is well inside normal"
+    )
+
+
+async def test_the_persisted_payload_still_carries_the_approval():
+    """A longer TTL is useless if the approval itself is not in the record."""
+    mgr = _manager()
+    await mgr._persist_session(_session(pending={"command": "ls -la", "risk": "HIGH"}))
+
+    _, _, payload = _setex_calls(mgr)[0]
+    assert json.loads(payload)["pending_approval"]["command"] == "ls -la"
+
+
+# --- D2: reaching the restore from the approve path -------------------------
+
+
+async def test_a_pending_approval_records_which_conversation_it_belongs_to():
+    """`get_session` takes a session_id; the restore needs a conversation_id."""
+    mgr = _manager()
+    await mgr._persist_session(_session(pending={"command": "ls -la"}))
+
+    pointers = [c for c in _setex_calls(mgr) if "approval_conversation" in c[0]]
+    assert pointers, "no pointer from session_id to its conversation was written"
+    key, ttl, value = pointers[0]
+    assert key == "agent_terminal:approval_conversation:sess-1"
+    assert value == "conv-1"
+    assert ttl > 3600
+
+
+async def test_no_pointer_is_written_for_a_session_without_an_approval():
+    """It exists only to rescue an approval; anything else is litter."""
+    mgr = _manager()
+    await mgr._persist_session(_session())
+
+    assert not [c for c in _setex_calls(mgr) if "approval_conversation" in c[0]]
+
+
+async def test_an_unknown_session_id_still_returns_none():
+    """The ordinary bad-id case must stay cheap and quiet."""
+    mgr = _manager()
+    mgr.redis_client.get = AsyncMock(return_value=None)
+
+    assert await mgr.get_session("no-such-session") is None
+
+
+async def test_a_vanished_session_is_rebuilt_from_its_unanswered_approval():
+    """The path that was unreachable: approve by the OLD session id.
+
+    Before this, `_restore_pending_approval` only ran in `create_session` and
+    produced a new id, so a GUI button carrying the old one could never resolve.
+    """
+    mgr = _manager()
+    mgr.redis_client.get = AsyncMock(side_effect=[None, b"conv-1"])
+    mgr.chat_history_manager = MagicMock()
+
+    async def _restore(session, conversation_id):
+        session.pending_approval = {"command": "ls -la", "conversation_id": conversation_id}
+
+    mgr._restore_pending_approval = _restore
+
+    session = await mgr.get_session("sess-1")
+
+    assert session is not None, "the approval was recoverable and was not recovered"
+    assert session.session_id == "sess-1", "must keep the id the approve request used"
+    assert session.get_pending_command() == "ls -la"
+
+
+async def test_a_vanished_session_with_no_outstanding_approval_is_not_resurrected():
+    """Only an unanswered approval justifies rebuilding a dead session."""
+    mgr = _manager()
+    mgr.redis_client.get = AsyncMock(side_effect=[None, b"conv-1"])
+    mgr.chat_history_manager = MagicMock()
+
+    async def _restore(session, conversation_id):
+        return None  # already answered — nothing to restore
+
+    mgr._restore_pending_approval = _restore
+
+    assert await mgr.get_session("sess-1") is None

autobot-backend/services/agent_terminal/session_manager.py

@@ -13,8 +13,15 @@
 import uuid
 from typing import Dict, List
 
+from autobot_shared.env_utils import env_int
 from autobot_shared.logging_manager import get_logger
 from constants.ttl_constants import TTL_1_HOUR
+
+# #13478: how long a session holding a pending approval survives in Redis.
+# Deliberately long: the thing it is waiting for is a person, and #13481
+# established that an approval does not expire on a timer. This bounds the
+# stored session, not the approval's validity.
+APPROVAL_PENDING_SESSION_TTL: int = env_int("AUTOBOT_APPROVAL_PENDING_SESSION_TTL_SECONDS", default=7 * 24 * 60 * 60)
 from services.command_approval_manager import AgentRole
 from type_defs.common import Metadata
 
@@ -320,8 +327,52 @@ async def get_session(self, session_id: str) -> AgentTerminalSession | None:
             except Exception as e:
                 logger.error("Failed to load session from Redis: %s", e)
 
-        # Session not found in memory or Redis
-        return None
+        # #13478: last resort — the session record is gone, but it may have been
+        # holding a pending approval. `_restore_pending_approval` can rebuild
+        # that from chat history and is documented as surviving restarts, yet it
+        # only ran inside `create_session`, which mints a NEW session_id. The
+        # approve endpoint is addressed by the OLD id (it is what the persisted
+        # approval message and the GUI button carry), so the restore was
+        # unreachable from the one path that needed it.
+        return await self._rebuild_session_from_pending_approval(session_id)
+
+    async def _rebuild_session_from_pending_approval(self, session_id: str) -> AgentTerminalSession | None:
+        """Reconstruct a vanished session that still has an unanswered approval.
+
+        Returns None when the session simply does not exist — the ordinary case
+        for a bad id, and it must stay cheap and quiet.
+        """
+        conversation_id = await self._conversation_for_expired_session(session_id)
+        if not conversation_id or not self.chat_history_manager:
+            return None
+
+        session = AgentTerminalSession(
+            session_id=session_id,
+            agent_id="restored",
+            # Lowest privilege on purpose. The role the original session ran
+            # under is not recorded in the restored approval, and a rebuilt
+            # session exists only to let a human resolve a decision that was
+            # already gated — it must never be the thing that widens what the
+            # command is allowed to do.
+            agent_role=AgentRole.CHAT_AGENT,
+            conversation_id=conversation_id,
+        )
+        await self._restore_pending_approval(session, conversation_id)
+
+        if not session.has_pending_approval():
+            # Nothing outstanding: the approval was already answered, so there is
+            # no session worth resurrecting.
+            return None
+
+        async with self._sessions_lock:
+            self.sessions[session_id] = session
+
+        logger.info(
+            "Rebuilt session %s from its pending approval in conversation %s (#13478)",
+            session_id,
+            conversation_id,
+        )
+        return session
 
     async def list_sessions(
         self,
@@ -378,19 +429,66 @@ async def close_session(self, session_id: str) -> bool:
         return True
 
     async def _persist_session(self, session: AgentTerminalSession) -> None:
-        """Persist session to Redis"""
+        """Persist session to Redis.
+
+        #13478: the TTL depends on whether the session is *idle* or *waiting on a
+        human*. TTL_1_HOUR garbage-collects abandoned sessions, which is right —
+        but it was applied to a session holding a pending approval too, and
+        nothing refreshed it. `set_pending_approval` re-persists immediately
+        (service.py:213), so the clock started the moment the prompt was raised
+        and ran out an hour later. `_approve_command_internal` then hit
+        `if not session: return {"error": "Session not found"}` and the approval
+        was unrecoverable.
+
+        That was the real expiry behind #13216 — not the 30-minute poll budget,
+        which only bounds the turn, and not a restart, which the session
+        survives. A person taking longer than an hour to answer a prompt is the
+        normal case this feature exists for, so the wait must not be what kills
+        it (#13481: approval does not expire on a timer).
+        """
         try:
             key = f"agent_terminal:session:{session.session_id}"
             # Issue #372: Use model method to reduce feature envy
             session_data = session.to_persist_dict()
 
+            ttl = APPROVAL_PENDING_SESSION_TTL if session.has_pending_approval() else TTL_1_HOUR
             json_data = json.dumps(session_data)
-            await self.redis_client.setex(key, TTL_1_HOUR, json_data)  # 1 hour TTL
-            logger.debug(f"Persisted session {session.session_id} to Redis with key {key}")
+            await self.redis_client.setex(key, ttl, json_data)
+
+            # #13478: a companion pointer so the approve path can still find the
+            # conversation after the session itself is gone. `get_session` takes
+            # a session_id and `_restore_pending_approval` needs a
+            # conversation_id, so without this the chat-history restore — which
+            # exists and works — is unreachable from the approve endpoint.
+            if session.has_pending_approval() and session.conversation_id:
+                await self.redis_client.setex(
+                    f"agent_terminal:approval_conversation:{session.session_id}",
+                    APPROVAL_PENDING_SESSION_TTL,
+                    session.conversation_id,
+                )
+
+            logger.debug("Persisted session %s to Redis with key %s (ttl=%ds)", session.session_id, key, ttl)
 
         except Exception as e:
             logger.error("Failed to persist session to Redis: %s", e)
 
+    async def _conversation_for_expired_session(self, session_id: str) -> str | None:
+        """The conversation a vanished session was serving, if it had an approval."""
+        if not self.redis_client:
+            return None
+        try:
+            raw = await asyncio.wait_for(
+                self.redis_client.get(f"agent_terminal:approval_conversation:{session_id}"),
+                timeout=2.0,
+            )
+        except (asyncio.TimeoutError, Exception) as exc:  # noqa: BLE001
+            logger.warning("Could not read approval-conversation pointer for %s: %s", session_id, exc)
+            return None
+
+        if raw is None:
+            return None
+        return raw.decode("utf-8") if isinstance(raw, (bytes, bytearray)) else str(raw)
+
     async def _restore_pending_approval(self, session: AgentTerminalSession, conversation_id: str) -> None:
         """
         Restore pending approval from chat history (survives backend restarts).