diff --git a/CHANGELOG.md b/CHANGELOG.md index 30bba8be6..1032a2a96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - tmux listing parse failures are retried once and reported as a distinct condition instead of surfacing as a bare `ValueError` that reads like "session not found" one layer up. libtmux 0.53.1+ zips `parse_output`'s fields with `strict=True`, so any short row (a pane or session vanishing mid-listing, or trailing fields tmux omits) raised `ValueError: zip() argument 2 is shorter than argument 1` — which propagated through `server.sessions`/`window.panes`, blocked launches outright, and left the pipe-liveness watchdog unable to tell a genuinely-gone session from a transient parse failure. Adds `TmuxLookupError` and routes the listing reads in `clients/tmux.py` through a single retry-and-classify wrapper; a failed `create_session` no longer leaves an orphaned tmux session that blocks relaunching the same name. Also caps `libtmux<0.53.1`, the last release that zips non-strict (caom-anv) - Codex handoff extraction now skips native TUI activity cells without relying on an English verb allowlist, including when the model's reply starts with prose (#545) - `list_sessions` ownership metadata now persists the effective canonical launch directory, stays stable after pane `cd`, and purges stale terminal rows before same-name session relaunches so reused sessions report the new directory/profile (#497) +- make session teardown atomic so tmux and the terminal registry can no longer diverge (#498). `delete_session` previously trusted a pre-loop liveness reading and an unverified `kill_session` result, so a session could survive while its registry rows were deleted (orphaned tmux session) or vice versa (ghost rows that later misattributed a reused session name). Now: session creation and session teardown are mutually exclusive per session *name* (a concurrent launch can no longer interleave with a teardown of the same name), `kill_session` returns True only once the session is confirmed gone, and registry rows are deleted only *after* that confirmation. **Error-contract change:** a teardown whose tmux kill cannot be confirmed now raises (surfaced as HTTP 500 on `DELETE /sessions/{name}`) instead of reporting success — the registry rows are left intact and the operation is safe to re-run, which reconciles the survivor. Backend authors: `TerminalBackend.kill_session` must not return True for a merely-dispatched kill ## [2.4.1] - 2026-08-04 diff --git a/docs/terminal-lifecycle.md b/docs/terminal-lifecycle.md index c93e492e6..f215177da 100644 --- a/docs/terminal-lifecycle.md +++ b/docs/terminal-lifecycle.md @@ -13,14 +13,20 @@ exhaust system resources. CAO provides automatic and manual cleanup paths. | Handoff completes successfully (auto-delete) | Yes | | `delete_terminal` MCP tool | Yes | | `DELETE /terminals/{id}` API | Yes | -| `cao shutdown --session ` | No | -| `cao shutdown --all` | No | +| `cao shutdown --session ` | Yes | +| `cao shutdown --all` | Yes | | Process crash | No | -Snapshots are only saved when a terminal is deleted individually via -`terminal_service.delete_terminal`. Session-level shutdown (`delete_session`) -kills windows directly and does not snapshot. If you want scrollback preserved, -delete terminals individually before shutting down the session. +Individual deletion snapshots via `terminal_service.delete_terminal`. +Session-level shutdown (`delete_session`, which both `cao shutdown` modes reach +over `DELETE /sessions/{name}`) snapshots too: capturing each terminal's +scrollback is an explicit step of the teardown, and it deliberately runs +*before* the session kill, since scrollback only exists while the pane does. A +crash bypasses both paths, so nothing is captured. + +Capture is best-effort everywhere, not just at session level: a snapshot whose +write fails is logged and teardown continues regardless of which path took it. +So "Yes" above means the path attempts a snapshot, not that one is guaranteed. ## Snapshot files diff --git a/src/cli_agent_orchestrator/backends/base.py b/src/cli_agent_orchestrator/backends/base.py index 3d6903b44..9e4016064 100644 --- a/src/cli_agent_orchestrator/backends/base.py +++ b/src/cli_agent_orchestrator/backends/base.py @@ -72,6 +72,32 @@ def session_exists(self, session_name: str) -> bool: """ ... + def session_exists_strict(self, session_name: str) -> bool: + """Check if a session exists, RAISING when the lookup cannot be answered. + + Semantics differ from ``session_exists`` only in the error case: + ``session_exists`` collapses a lookup error into False ("assume + absent"), whereas this must distinguish a confirmed absence (return + False) from an inability to tell (raise). Teardown confirmation MUST + use this so a transient backend error is never misread as "session + gone" (#498). + + Implementations must query in a way that keeps the two apart. That is a + real constraint, not a formality: a client library that swallows its own + transport errors and reports an empty result set makes a lookup failure + LOOK like an absence, and a strict check layered over it fails OPEN no + matter what it does with its own exceptions. ``TmuxBackend`` therefore + issues its own ``list-sessions`` and classifies the exit status + (``clients/tmux.py``) rather than using libtmux's session collection. + + The default implementation delegates to ``session_exists`` for backends + that cannot yet make the distinction; those backends therefore retain the + old lenient, fail-OPEN behavior, and the teardown guarantee is only as + strong as this method. HerdrBackend is in that position — tracked as a + follow-up. + """ + return self.session_exists(session_name) + @abstractmethod def list_sessions(self) -> List[Dict[str, str]]: """List all sessions managed by this backend. @@ -85,11 +111,20 @@ def list_sessions(self) -> List[Dict[str, str]]: def kill_session(self, session_name: str) -> bool: """Kill/destroy a session. + Implementations MUST NOT return True on a merely-dispatched kill: session + teardown treats True as proof the session is gone and only then drops the + matching registry rows, so an optimistic True is what lets tmux and the + registry diverge (#498). Confirm the session is actually gone — poll if + the kill is asynchronous — before returning True. + Args: session_name: Session to kill Returns: - True if session was killed, False if not found + True once the session is confirmed gone; False if it was not found + OR the kill could not be confirmed within the backend's bound. A + caller that must tell those two apart re-checks existence itself via + ``session_exists_strict``. """ ... diff --git a/src/cli_agent_orchestrator/backends/tmux_backend.py b/src/cli_agent_orchestrator/backends/tmux_backend.py index b3760a2eb..bc2ae9d51 100644 --- a/src/cli_agent_orchestrator/backends/tmux_backend.py +++ b/src/cli_agent_orchestrator/backends/tmux_backend.py @@ -45,6 +45,9 @@ def create_session( def session_exists(self, session_name: str) -> bool: return self._client.session_exists(session_name) + def session_exists_strict(self, session_name: str) -> bool: + return self._client.session_exists_strict(session_name) + def list_sessions(self) -> List[Dict[str, str]]: return self._client.list_sessions() diff --git a/src/cli_agent_orchestrator/clients/database.py b/src/cli_agent_orchestrator/clients/database.py index ddec3adb8..72f263898 100644 --- a/src/cli_agent_orchestrator/clients/database.py +++ b/src/cli_agent_orchestrator/clients/database.py @@ -1384,6 +1384,27 @@ def delete_terminals_by_session(tmux_session: str) -> int: return deleted +def delete_terminals_by_ids(terminal_ids: List[str]) -> int: + """Delete specific terminal rows by id. Returns the number deleted. + + Unlike ``delete_terminals_by_session`` (which deletes EVERY row for a + session name), this deletes only the given ids. Session teardown uses it to + scope its reconciliation sweep to the incarnation it started tearing down, + so a concurrent same-name recreate — whose rows carry freshly generated ids + — is never swept (#498). + """ + if not terminal_ids: + return 0 + with SessionLocal() as db: + deleted = ( + db.query(TerminalModel) + .filter(TerminalModel.id.in_(terminal_ids)) + .delete(synchronize_session=False) + ) + db.commit() + return deleted + + def create_inbox_message(sender_id: str, receiver_id: str, message: str) -> InboxMessage: """Create inbox message with status=MessageStatus.PENDING. diff --git a/src/cli_agent_orchestrator/clients/tmux.py b/src/cli_agent_orchestrator/clients/tmux.py index 8afffe06c..ca7e8476c 100644 --- a/src/cli_agent_orchestrator/clients/tmux.py +++ b/src/cli_agent_orchestrator/clients/tmux.py @@ -5,9 +5,10 @@ import re import shlex import subprocess +import sys import time import uuid -from typing import Callable, Dict, List, Optional, TypeVar +from typing import Callable, Dict, FrozenSet, List, Optional, Tuple, TypeVar import libtmux from libtmux.pane import Pane @@ -49,14 +50,317 @@ class TmuxLookupError(RuntimeError): deliberately NOT a subclass of ``ValueError``: an ``except ValueError`` written for the not-found case must not swallow it. + ``session_exists_strict`` raises it for a SECOND, unrelated cause: a + ``list-sessions`` that tmux itself could not answer (an unreachable socket + whose server may still be alive, a permission error, an unrecognised + failure). Both causes mean the same thing to a caller — the answer is + UNKNOWN — which is why they share one type. Session teardown turns it into a + failed, re-runnable teardown with the registry left intact (#498), the + fail-CLOSED direction. + Treat it as transient and retryable — ``TmuxClient`` already retries the read once before raising. """ +# --------------------------------------------------------------------------- +# stderr classification for ``session_exists_strict``. +# +# WARNING: THE WORDING BELOW IS TMUX-VERSION-DEPENDENT. The same injected +# failure does NOT produce the same message on every tmux: a socket path that is +# a regular file yields "Socket operation on non-socket" (ENOTSOCK) on tmux 3.7b +# but "no server running on " (ECONNREFUSED) on tmux 3.2a. So do not grow +# these tables from one version's observed output, and do not write tests around +# a vector whose message tmux words ITSELF. The "error connecting to +# ()" messages are worded by the kernel's errno and are the portable +# ones. Wording we do not recognise raises, which is the safe direction. +# --------------------------------------------------------------------------- + +# tmux got a definitive answer from the kernel ABOUT THE SOCKET PATH: the path +# resolved and nothing is listening on it (ECONNREFUSED). No server on the socket +# means no sessions on it, so this is a CONFIRMED absence. Treating it as an +# error instead would strand registry rows left by a long-dead server forever — +# ``delete_session`` would raise on every retry and never reconcile them. +_NO_SERVER_STDERR_MARKERS = ("no server running",) + +# The socket path does not exist (ENOENT). On its own this is NOT an absence: a +# tmux server keeps serving its socket after the PATH has been unlinked — the +# classic cause is a tmp-cleaner / systemd-tmpfiles sweeping /tmp on a long-lived +# host, and the recovery is ``kill -USR1 ``, which makes the server +# recreate the path. A never-created socket and an unlinked socket under a LIVE +# server produce byte-identical stderr, so no string matching can separate them; +# this marker is a confirmed absence only once ``_tmux_server_liveness`` says no +# tmux server is bound to the path. +_SOCKET_MISSING_STDERR_MARKERS = ("no such file or directory",) + +# "error connecting to /tmp/tmux-1000/default (No such file or directory)". +# Greedy, anchored on the trailing "()", so socket paths that +# themselves contain " (" still parse. +_CONNECT_ERROR_PATH_PATTERN = re.compile(r"error connecting to (?P.+) \([^()]*\)\s*$") + +# Verdicts of ``_tmux_server_liveness``. UNKNOWN must be treated exactly like +# ALIVE by anything deciding whether a session can be declared gone. +_SERVER_ALIVE = "alive" +_SERVER_ABSENT = "absent" +_SERVER_UNKNOWN = "unknown" + +# Which socket-table detector to prefer is a PLATFORM question, so it is read +# from a named module constant rather than from ``sys.platform`` at the call site: +# a test can then exercise another platform's ladder without mutating the real +# ``sys`` for everything else running in the process. +_HOST_PLATFORM = sys.platform + +# The kernel's AF_UNIX socket table. Linux-only; absence of it is "cannot tell". +_PROC_NET_UNIX = "/proc/net/unix" + +# macOS/BSD equivalent. ``-U`` selects AF_UNIX sockets only; ``-F n`` prints one +# field per line ("n") so socket paths containing spaces survive, which +# column splitting would not guarantee. Deliberately NO other flags: ``-b`` +# (avoid blocking kernel calls) suppresses the socket rows entirely on Linux lsof +# 4.94, and ``-n``/``-P`` only affect internet sockets, which ``-U`` excludes. +_LSOF_COMMAND = "lsof" +_LSOF_ARGUMENTS = ("-U", "-F", "n") +# lsof walks every process's descriptors, so on a busy host it is slow, and it +# can block on a hung mount. Teardown must not hang behind it: this bound is the +# whole reason the call is safe to make in that path. A timeout is "cannot tell" +# from THIS detector, so the ladder simply moves on. +_LSOF_TIMEOUT_SECONDS = 3.0 +_PROCESS_LIST_TIMEOUT_SECONDS = 5.0 +# Linux lsof decorates a unix socket's name with " type=STREAM" / " type=DGRAM"; +# macOS lsof does not. Stripped so both platforms yield the bare bound path. +_LSOF_NAME_SUFFIX_PATTERN = re.compile(r"\s+type=\S+\s*$") + + +def _connect_error_socket_path(stderr_lines: List[str]) -> Optional[str]: + """The socket path out of tmux's own "error connecting to ..." message. + + Taken from tmux rather than re-derived from libtmux/``TMUX_TMPDIR``, so the + path we probe is exactly the one tmux failed to reach. ``None`` when no line + has that shape — the caller must then fail CLOSED, since a "no such file or + directory" from somewhere other than the connect (a missing config file, say) + says nothing about the server. + """ + for line in stderr_lines: + match = _CONNECT_ERROR_PATH_PATTERN.search(line.strip()) + if match: + return match.group("path") + return None + + +def _bound_unix_socket_paths_via_proc() -> Optional[FrozenSet[str]]: + """Bound AF_UNIX socket paths from the kernel's own table (Linux). + + Returns ``None`` when the table cannot be read — which is the NORMAL case off + Linux, where ``/proc/net/unix`` simply does not exist. ``None`` means "this + detector cannot tell", never "nothing is bound"; see + ``_bound_unix_socket_paths`` for how the ladder handles that. + """ + try: + with open(_PROC_NET_UNIX, "r", encoding="utf-8", errors="replace") as handle: + lines = handle.readlines() + except OSError: + return None + + paths = set() + # Columns: Num RefCount Protocol Flags Type St Inode Path. The path is the + # last column and is optional (unnamed sockets have none); maxsplit keeps + # paths containing spaces intact. Line 0 is the header. + for line in lines[1:]: + columns = line.rstrip("\n").split(None, 7) + if len(columns) == 8 and columns[7]: + paths.add(columns[7]) + return frozenset(paths) + + +def _bound_unix_socket_paths_via_lsof() -> Optional[FrozenSet[str]]: + """Bound AF_UNIX socket paths from ``lsof -U`` (macOS/BSD, and Linux backup). + + ``None`` when lsof is missing, times out, or produces nothing parsable — all + "cannot tell", never "nothing is bound". In particular an EMPTY parse is + reported as ``None`` rather than as an empty set: a host with no named unix + socket anywhere is implausible, so an empty listing much more likely means + lsof failed, and reading it as "nothing is bound" would fail OPEN. + + Like ``/proc/net/unix``, lsof keeps reporting the path after it has been + unlinked, because the report comes from the process's open descriptor rather + than from the filesystem. Verified on macOS by the reviewer of fixup 5 (lsof + still named an unlinked private tmux socket) and re-verified here on Linux + lsof 4.94. + + Nonzero exit is NOT treated as failure on its own: lsof routinely exits 1 + after warning about processes it could not fully inspect while still printing + a complete-enough listing. The parse result is what decides. + """ + try: + result = subprocess.run( + [_LSOF_COMMAND, *_LSOF_ARGUMENTS], + capture_output=True, + text=True, + timeout=_LSOF_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.SubprocessError): + # FileNotFoundError (no lsof) and TimeoutExpired both land here. + return None + + paths = set() + for line in result.stdout.splitlines(): + # "-F n" emits one field per line, tagged by its first character: "p" + # for the pid set, "n" for a name. Anything else is not a name. + if not line.startswith("n"): + continue + name = line[1:] + # Both forms, because the " type=STREAM" decoration is indistinguishable + # from a socket path that genuinely ends that way. Keeping the undecorated + # name as well means such a path still matches; the extra string can only + # ever produce a spurious ALIVE, which is the fail-CLOSED direction. + for candidate in (name, _LSOF_NAME_SUFFIX_PATTERN.sub("", name)): + # Unnamed sockets render as "type=STREAM" alone, and lsof uses + # "->0x..." / "socket" placeholders; only absolute paths are usable. + if candidate.startswith("/"): + paths.add(candidate) + return frozenset(paths) or None + + +# The socket-table detectors, in the order they should be tried, per platform. +# Split out as a named seam so the non-Linux ladder is exercisable from a Linux +# test (and vice versa) instead of only on the platform that selects it. +def _socket_table_detectors( + platform_name: Optional[str] = None, +) -> Tuple[Callable[[], Optional[FrozenSet[str]]], ...]: + """Which socket-table detectors to try, most authoritative first. + + Linux prefers ``/proc/net/unix``: exact, pid-free and free of a subprocess. + Everywhere else that file does not exist, so lsof leads — the alternative + (fixup 5's behaviour) was that the whole tier reported "cannot tell" on every + non-Linux host, which combined with fail-closed made ``session_exists_strict`` + raise unconditionally there and blocked teardown permanently. + + lsof is kept as a Linux backup too, for the case where ``/proc`` is not + mounted in the namespace CAO runs in. + """ + if platform_name is None: + platform_name = _HOST_PLATFORM + if platform_name.startswith("linux"): + return (_bound_unix_socket_paths_via_proc, _bound_unix_socket_paths_via_lsof) + return (_bound_unix_socket_paths_via_lsof,) + + +def _bound_unix_socket_paths() -> Optional[FrozenSet[str]]: + """Every filesystem path an AF_UNIX socket is currently bound to. + + Returns ``None`` only when NO detector on this platform could answer. + Callers MUST treat ``None`` as "cannot tell", never as "nothing is bound". + + This is the one piece of evidence that survives the failure + ``session_exists_strict`` has to detect: the bound path stays visible after + that path has been UNLINKED, and disappears when the owning process dies. + tmux itself cannot tell us — its only channel to the server is the path that + is gone. + """ + for detector in _socket_table_detectors(): + paths = detector() + if paths is not None: + return paths + return None + + +def _tmux_process_command_lines() -> Optional[List[str]]: + """Command lines of running processes that mention tmux, via ``ps``. + + Last resort, for hosts where no socket table can be read at all (no + ``/proc/net/unix`` AND no usable ``lsof``). Returns ``None`` if the + process table cannot be listed at all. Deliberately coarse: it can prove + "there is no tmux process anywhere" and it can prove "a tmux process names + this socket path", but on a server started without an explicit ``-S``/``-L`` + the socket path does not appear in the argv at all, so a match failure is + ambiguous rather than negative — see ``_tmux_server_liveness``. + """ + try: + result = subprocess.run( + ["ps", "-A", "-o", "command="], + capture_output=True, + text=True, + timeout=_PROCESS_LIST_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.SubprocessError): + return None + if result.returncode != 0: + return None + return [line for line in result.stdout.splitlines() if "tmux" in line] + + +def _socket_path_aliases(socket_path: str) -> FrozenSet[str]: + """``socket_path`` plus its fully resolved form. + + tmux reports the path it tried to connect to; a socket table may report the + same socket under a resolved path instead (on macOS ``/tmp`` is a symlink to + ``/private/tmp``). Comparing both forms can only ever turn a missed match into + a match, i.e. ABSENT into ALIVE, which is the fail-CLOSED direction. + """ + aliases = {socket_path} + try: + aliases.add(os.path.realpath(socket_path)) + except OSError: + pass + return frozenset(aliases) + + +def _tmux_server_liveness(socket_path: str) -> str: + """Is a tmux server serving ``socket_path``, even if the path is unlinked? + + Returns ``_SERVER_ALIVE``, ``_SERVER_ABSENT`` (demonstrably none) or + ``_SERVER_UNKNOWN``. Only ``_SERVER_ABSENT`` licenses concluding that a + session is gone; the other two must fail CLOSED. + + The ladder, in order: + + 1. **The socket table**, via ``_bound_unix_socket_paths`` — per platform, + ``/proc/net/unix`` on Linux and ``lsof -U`` on macOS/BSD (see + ``_socket_table_detectors``). Authoritative in BOTH directions: a live tmux + server necessarily holds a bound AF_UNIX socket at its path, so + path-not-listed is real evidence of absence, and the entry survives the + path being unlinked. Verified on Linux 6.1 / tmux 3.2a for both detectors: + listed while alive, still listed after the socket file is unlinked, + unlisted the moment the server dies (also after a normal ``kill-server``, + which leaves the socket FILE behind). + 2. **The process table** (``ps``), only if no socket-table detector could + answer at all. Coarse by nature, so it answers ABSENT only when there is no + tmux process anywhere, ALIVE when some tmux process names this exact path, + and UNKNOWN for a tmux process it cannot attribute to the path — a server + started without an explicit ``-S``/``-L`` (i.e. every real CAO server) does + not carry its socket path in argv, so a match failure here is ambiguous, + never negative. + + UNKNOWN when nothing in the ladder can be consulted. That case fails closed + and must stay RARE: fixup 5 made it universal off Linux, which blocked + teardown permanently there. + """ + candidates = _socket_path_aliases(socket_path) + + bound_paths = _bound_unix_socket_paths() + if bound_paths is not None: + return _SERVER_ALIVE if candidates & bound_paths else _SERVER_ABSENT + + command_lines = _tmux_process_command_lines() + if command_lines is None: + return _SERVER_UNKNOWN + if not command_lines: + return _SERVER_ABSENT + if any(candidate in command_line for command_line in command_lines for candidate in candidates): + return _SERVER_ALIVE + return _SERVER_UNKNOWN + + class TmuxClient: """Simplified tmux client for basic operations.""" + # How long ``kill_session`` waits for tmux to actually reap the session + # before giving up and reporting the kill unconfirmed. tmux's own reaping is + # asynchronous with respect to ``kill-session`` returning, so a bound is + # needed; 2s covers the observed lag with margin without stalling a teardown. + _KILL_SESSION_VERIFY_TIMEOUT_SECONDS = 2.0 + _KILL_SESSION_VERIFY_INTERVAL_SECONDS = 0.2 + def __init__(self) -> None: self.server = libtmux.Server() @@ -909,34 +1213,80 @@ def read() -> List[Dict[str, str]]: return [] def kill_session(self, session_name: str) -> bool: - """Kill tmux session. + """Kill tmux session, returning True only once it is CONFIRMED gone. - Returns: - True if a session was killed, False if there was nothing to kill - (or the kill itself failed). + Dispatching the kill is not the same as the session being gone: tmux + reaps asynchronously, so ``session.kill()`` can return while the session + is still listed. Session teardown treats True as proof the session is + gone and only then drops the matching registry rows, so an optimistic + True is exactly what lets tmux and the registry diverge (#498). Hence the + bounded verification poll below. + + Returns False for BOTH "no such session" and "kill dispatched but not + confirmed gone within the verify bound"; callers that need to tell those + apart re-check existence themselves (see ``services/session_service.py``). A listing parse failure does NOT return False here: "we could not look" is not "nothing to kill", and swallowing it is exactly how a failed launch leaves a live session that blocks the retry. The kill is retried - through the parse-free tmux CLI instead. + through the parse-free tmux CLI instead — and then verified like any + other, so the fallback cannot report an unconfirmed kill as success. """ try: session = self._find_session(session_name) if session is None: return False self._read_listing(f"kill-session '{session_name}'", session.kill) - logger.info(f"Killed tmux session: {session_name}") - return True except TmuxLookupError as e: logger.warning( f"tmux listing failed while killing session {session_name}: {e} — " "falling back to the tmux CLI" ) - return self._kill_via_cli(session_name) + if not self._kill_via_cli(session_name): + return False except Exception as e: logger.error(f"Failed to kill session {session_name}: {e}") return False + return self._confirm_session_gone(session_name) + + def _confirm_session_gone(self, session_name: str) -> bool: + """Poll, bounded, until tmux confirms ``session_name`` is gone. + + Confirmation uses ``session_exists_strict``, not ``session_exists``: a + transient tmux/socket error during the poll must NOT be misread as + "session gone". The strict check raises ``TmuxLookupError`` on an + unanswerable lookup, which returns False here — so a kill is never + reported as confirmed while the session may still be alive. + + Returns True only on a CONFIRMED absence. + """ + start = time.monotonic() + deadline = start + self._KILL_SESSION_VERIFY_TIMEOUT_SECONDS + while True: + try: + still_here = self.session_exists_strict(session_name) + except TmuxLookupError as e: + logger.error( + f"Could not confirm tmux session {session_name} is gone after " + f"kill_session: {e}" + ) + return False + if not still_here: + logger.info(f"Killed tmux session: {session_name}") + return True + if time.monotonic() >= deadline: + # Elapsed time helps diagnose a false negative: on a heavily + # loaded host tmux can take longer than the bound to reap a + # session that kill() did dispatch, so a caller re-run will + # typically then see it gone. + elapsed = time.monotonic() - start + logger.error( + f"Tmux session {session_name} still exists {elapsed:.2f}s after kill_session" + ) + return False + time.sleep(self._KILL_SESSION_VERIFY_INTERVAL_SECONDS) + def kill_window(self, session_name: str, window_name: str) -> bool: """Kill a specific tmux window within a session. @@ -971,6 +1321,12 @@ def session_exists(self, session_name: str) -> bool: a wrong False is the worst possible answer. On a parse failure we ask tmux directly (``has-session``), which needs no listing parse. + Still LENIENT about everything else: any other lookup error collapses to + False ("assume absent"). Fine for best-effort callers (status/UI, the + duplicate-name guard), but NOT for teardown confirmation, where a + transient socket error must not read as "gone" — use + ``session_exists_strict`` there (#498). + Raises: TmuxLookupError: The listing could not be parsed AND the direct probe could not answer either — genuinely unknown. @@ -989,6 +1345,138 @@ def session_exists(self, session_name: str) -> bool: except Exception: return False + def session_exists_strict(self, session_name: str) -> bool: + """Check if a session exists, RAISING when the lookup cannot be answered. + + Unlike ``session_exists``, this never collapses a query error into + "absent": a live session returns True, a confirmed absence returns False, + and an inability to tell raises ``TmuxLookupError`` — the only exception + type this method lets out. Teardown depends on that third case existing — + it deletes registry rows only once tmux is provably gone, so "couldn't + tell" reaching it as False is precisely how rows get dropped from under a + live session (#498). + + Why this does NOT go through ``self.server.sessions``: libtmux's + ``Server.sessions`` property wraps its ``list-sessions`` fetch in a bare + ``try/except Exception: pass`` and returns whatever it collected. A + transient socket or tmux failure therefore yields an EMPTY QueryList, + indistinguishable from a server with no sessions, and the lookup on it + reports a clean absence. Any strict check built on that property fails + OPEN by construction, no matter how its own exceptions are handled. So we + issue the query ourselves through ``Server.cmd`` (which runs the tmux + subprocess and exposes ``returncode``/``stderr`` without swallowing + anything) and classify the outcome. + + **False is only ever returned on positive evidence of absence:** + + * exit 0 and the name is not in the list — the list is authoritative; + membership is an exact string match against ``#{session_name}``, with no + tmux target-syntax parsing involved, so no prefix or ``=``/``:`` quoting + subtleties. Matches the old ``sessions.get(session_name=...)``. + * the kernel refused the connection to the socket path (ECONNREFUSED, + ``_NO_SERVER_STDERR_MARKERS``) — the path resolved and nothing is + listening, so there is no server and therefore no session. + * the socket path does NOT exist (ENOENT, ``_SOCKET_MISSING_STDERR_MARKERS``) + **and** ``_tmux_server_liveness`` finds no tmux server bound to it. The + extra check is load-bearing: a server keeps serving its socket after the + path is unlinked (tmp-cleaners do this; ``kill -USR1 `` recovers), + and that produces stderr byte-identical to a socket that never existed. + Concluding absence from the message alone declared live sessions dead — + the fail-open this check exists to prevent. + + Everything else raises: permission denied, a path that cannot be resolved + at all (ENOTDIR, ENAMETOOLONG), an unrecognised message, a nonzero exit + with no message, a missing socket whose server liveness cannot be + established, or any unexpected exception from the tmux call itself. + + **What this does NOT guarantee.** The check reaches the server only + through its socket PATH, so it cannot see a server the path no longer + leads to: + + * If the path is unlinked and then re-created — by a second tmux server, + or by an unrelated file or directory — ``list-sessions`` answers about + the NEW object and reports the original server's sessions absent. (On + tmux 3.2a a plain file at the path even yields ECONNREFUSED, i.e. the + confirmed-absence branch above.) No check built on the socket path can + detect that; it needs a server identity CAO does not record. + * ``/proc/net/unix`` is per network namespace, so a server holding the + unlinked socket from another netns reads as absent (it is also + unreachable from here). The lsof detector has the analogous blind spot: + unprivileged lsof cannot inspect other users' processes, so a socket + bound by ANOTHER user's tmux server can read as absent. CAO's own + server always runs as the same user. + * Where no socket table can be read at all the liveness probe falls back + to the process table and answers UNKNOWN for any tmux process it cannot + attribute to the path. That fails CLOSED, but it means an unrelated + tmux server can block reconciliation of a dead server's rows on such a + host. This is now the rare case (no ``/proc/net/unix`` AND no usable + ``lsof``); in fixup 5 it was every non-Linux host, which blocked + teardown there permanently. + * The stderr tables are tmux-version-dependent (see the comment on them). + Wording from a build we do not recognise lands in the raise branch: a + loud, non-destructive, re-runnable teardown FAILURE rather than a silent + false success. + + Behaviour above measured on private sockets against tmux 3.2a / libtmux + 0.51.0 on Linux 6.1 (this repo's dev environment) with BOTH socket-table + detectors, and, for the stderr wording, tmux 3.7b in fixup 4. The macOS + lsof behaviour is the reviewer's measurement on macOS; no other BSD has + been exercised. + """ + try: + return self._classify_session_lookup(session_name) + except TmuxLookupError: + raise + except Exception as exc: + # The tmux call itself can fail in ways that are not about the + # session (no tmux binary, OSError spawning it). Callers handle + # exactly one type, so translate rather than leak: both call sites + # already turn TmuxLookupError into the fail-CLOSED outcome. + raise TmuxLookupError( + f"could not determine whether tmux session '{session_name}' exists: " + f"the lookup itself failed with {type(exc).__name__}: {exc}" + ) from exc + + def _classify_session_lookup(self, session_name: str) -> bool: + """``session_exists_strict`` without the exception normalisation.""" + result = self.server.cmd("list-sessions", "-F", "#{session_name}") + + if result.returncode == 0: + return session_name in result.stdout + + stderr_lines = [str(line) for line in result.stderr] + stderr = " ".join(stderr_lines) + lowered = stderr.lower() + + if any(marker in lowered for marker in _NO_SERVER_STDERR_MARKERS): + return False + + if any(marker in lowered for marker in _SOCKET_MISSING_STDERR_MARKERS): + socket_path = _connect_error_socket_path(stderr_lines) + if socket_path is not None: + liveness = _tmux_server_liveness(socket_path) + if liveness == _SERVER_ABSENT: + return False + verdict = ( + "a tmux server is still bound to it" + if liveness == _SERVER_ALIVE + else "whether a tmux server is still bound to it cannot be " + "determined on this host" + ) + raise TmuxLookupError( + f"could not determine whether tmux session '{session_name}' " + f"exists: the tmux socket {socket_path} is gone from the " + f"filesystem and {verdict}, so the session may be alive. A " + "tmp-cleaner unlinking the socket does this; " + "`kill -USR1 ` makes the server recreate it." + ) + + raise TmuxLookupError( + f"could not determine whether tmux session '{session_name}' exists: " + f"list-sessions exited {result.returncode} " + f"({stderr or 'no error output'})" + ) + def get_pane_working_directory(self, session_name: str, window_name: str) -> Optional[str]: """Get the current working directory of a pane. diff --git a/src/cli_agent_orchestrator/services/session_lock.py b/src/cli_agent_orchestrator/services/session_lock.py new file mode 100644 index 000000000..061089204 --- /dev/null +++ b/src/cli_agent_orchestrator/services/session_lock.py @@ -0,0 +1,91 @@ +"""Per-session-name lifecycle lock serializing session CREATE against TEARDOWN. + +Why this exists (#498): tmux and the terminal registry (SQLite) are two separate +stores, and a session's lifecycle transitions write BOTH. Without mutual +exclusion, a create landing inside a teardown's critical section interleaves +arbitrarily and orphans state in one store or the other: + +* teardown snapshots the rows for ``name``, confirms tmux gone, and sweeps — + while a concurrent create has meanwhile put a NEW tmux session under ``name`` + plus a fresh row. Scoping the sweep by row id keeps the new row alive, but + nothing stops the reverse interleaving where the create's tmux session is + killed by the teardown that already decided ``name`` was dead. +* two concurrent teardowns of ``name`` both dispatch a kill and both sweep. + +Making each transition atomic per session NAME removes every such interleaving: +a create either completes entirely before a teardown starts (the teardown then +sees the new rows and tears them down properly) or starts entirely after it (the +name is free and the create builds a clean incarnation). Both orders leave the +two stores agreeing, which is the only invariant that matters. + +Design notes +------------ +``threading.Lock``, not ``asyncio.Lock``. The two call paths do not share an +execution model: teardown is a fully synchronous function that the API runs via +``asyncio.to_thread`` (``api/main.py``), and the CLI reaches it over HTTP, so it +executes on a worker thread with no running loop — an ``asyncio.Lock`` is simply +not acquirable there. Creation runs on the event loop. A threading primitive is +the only one reachable from both, and there is no single-event-loop assumption to +rely on (the MCP server is a separate process talking HTTP). + +To keep that safe, the lock is held only across each path's SHORT state-transition +critical section — never across a long ``await``. In particular the create path +releases before ``provider.initialize()`` (tens of seconds), so a teardown of the +same name is never blocked behind an agent launch. See the call sites for exactly +what each critical section spans. + +Locks are refcounted and dropped at zero, so the registry cannot grow without +bound as sessions come and go. +""" + +import threading +from contextlib import contextmanager +from typing import Dict, Iterator, Tuple + +# Guards the registry below. Only ever held for the few dict operations in +# ``session_lifecycle_lock``'s acquire/release bookkeeping — never across the +# caller's critical section, so it can't serialize different session names. +_registry_guard = threading.Lock() + +# session name -> (lock, number of holders+waiters currently interested in it). +# The count is what makes eviction safe: the entry is removed only once nobody +# is using it, so two threads contending for the same name always end up on the +# SAME lock object (a plain "pop on release" would let a waiter be handed a +# fresh, uncontended lock and defeat the mutual exclusion entirely). +_session_locks: Dict[str, Tuple[threading.Lock, int]] = {} + + +@contextmanager +def session_lifecycle_lock(session_name: str) -> Iterator[None]: + """Hold the lifecycle lock for ``session_name`` across the with-block. + + Different session names never contend (the whole point — a global lock would + serialize every session operation on the server); the same name is strictly + serialized, for create-vs-teardown and teardown-vs-teardown alike. + + Release is guaranteed on every exit path, exceptions included. + + NOT reentrant: a call path already holding the lock for ``name`` must not + re-enter it for the same ``name``, or it self-deadlocks. The two critical + sections are deliberately narrow and call nothing that re-acquires — plugin + dispatch and provider initialization both run outside them. + """ + with _registry_guard: + lock, holders = _session_locks.get(session_name, (threading.Lock(), 0)) + _session_locks[session_name] = (lock, holders + 1) + + lock.acquire() + try: + yield + finally: + lock.release() + with _registry_guard: + # Re-read rather than trusting the count captured above: other + # threads have adjusted it in the meantime. + entry = _session_locks.get(session_name) + if entry is not None: + held_lock, holders = entry + if holders <= 1: + del _session_locks[session_name] + else: + _session_locks[session_name] = (held_lock, holders - 1) diff --git a/src/cli_agent_orchestrator/services/session_service.py b/src/cli_agent_orchestrator/services/session_service.py index c938c5b9e..3809446aa 100644 --- a/src/cli_agent_orchestrator/services/session_service.py +++ b/src/cli_agent_orchestrator/services/session_service.py @@ -17,14 +17,20 @@ 1. create_terminal() with new_session=True creates a new tmux session 2. Additional terminals are added via create_terminal() with new_session=False 3. delete_session() removes the entire session and all contained terminals + +Transitions 1/2 and 3 are mutually exclusive per session NAME, via the lifecycle +lock in ``services/session_lock.py`` — see ``delete_session`` for why. """ import logging -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple from cli_agent_orchestrator.backends.base import TerminalBackend from cli_agent_orchestrator.backends.registry import get_backend -from cli_agent_orchestrator.clients.database import list_terminals_by_session +from cli_agent_orchestrator.clients.database import ( + delete_terminals_by_ids, + list_terminals_by_session, +) from cli_agent_orchestrator.constants import SESSION_PREFIX from cli_agent_orchestrator.models.inbox import OrchestrationType from cli_agent_orchestrator.models.kiro_engine import KiroEngine @@ -33,9 +39,11 @@ PluginRegistry, PostCreateSessionEvent, PostKillSessionEvent, + PostKillTerminalEvent, ) from cli_agent_orchestrator.services.plugin_dispatch import dispatch_plugin_event from cli_agent_orchestrator.services.session_env import clear_session_env +from cli_agent_orchestrator.services.session_lock import session_lifecycle_lock from cli_agent_orchestrator.services.terminal_service import create_terminal from cli_agent_orchestrator.utils.agent_profiles import resolve_provider @@ -214,50 +222,274 @@ def get_session(session_name: str) -> Dict: def delete_session(session_name: str, registry: PluginRegistry | None = None) -> Dict: - """Delete session and cleanup. + """Delete session and cleanup, reconciling tmux and the registry atomically. + + Two properties make the two stores impossible to diverge (#498): + + **Mutual exclusion.** The whole critical section — enumerate rows, capture + scrollback, confirm tmux gone, dismantle runtimes, delete rows — runs under + the per-session-name lifecycle lock (``services/session_lock.py``) — the SAME + lock ``create_terminal`` holds while it creates a session/window and writes + the matching row. Without it no ordering discipline helps: a create landing + mid-teardown can put a live tmux session under this name after we have + already decided the name is dead, and a second concurrent teardown can + double-kill and double-sweep. Serializing per NAME leaves teardowns of + DIFFERENT sessions fully concurrent. + + **Nothing is dismantled until the kill is confirmed.** There is therefore + nothing to roll back, and no snapshot/restore machinery: an earlier revision + deleted rows first and restored them from a snapshot on failure, which both + reconstructed them lossily (``last_active`` was dropped) and restored ONLY + the rows — the FIFO reader, status-monitor buffers and provider registration + stayed torn down, so the "restored" terminal was a zombie, a row that looked + live with no pipeline behind it. Ordering: + + 1. Enumerate the incarnation's rows under the lock. Because the lock also + covers creation, this list cannot grow behind us — a concurrent create is + either fully included or has not started. + 2. Snapshot each terminal's scrollback/metadata + (``capture_terminal_snapshot``). This must precede the kill (scrollback + only exists while the pane does), and it is safe there because it is + READ-ONLY with respect to terminal state — it reads tmux and writes two + files under the log directory. If the kill then fails to confirm, no + terminal has been touched. + 3. Check liveness STRICTLY: a lookup error must not be misread as "gone". + ``session_exists`` collapses any error to False, which would drop the + registry while the session may be alive. + 4. If the session is alive, ``kill_session`` — which per the backend + contract returns True only once the session is CONFIRMED gone, and False + for both a failed kill and an already-absent target. A False is + disambiguated by a strict follow-up check: confirmed gone (it vanished + between the check and the kill lookup) is SUCCESS; still provably alive + is a real failure. Killing the SESSION kills every window in it, so no + per-window kill is needed first — and doing it this way is what lets the + destructive work all sit after the confirmation. + 5. Only now that tmux is provably gone: dismantle each terminal's runtime + (``dismantle_terminal_runtime`` with ``kill_window=False`` — the windows + died with the session), then delete the rows (scoped by id via + ``delete_terminals_by_ids``, so a same-name session created after this + teardown finishes keeps its own rows), and drop the forwarded-env mapping. + Every step here is individually guarded: past the confirmation point the + teardown has succeeded, so a failing step is reported in ``errors`` rather + than raised — raising would both misreport a completed teardown and, since + dispatch is deferred to 6, lose every event for it. + 6. Release the lock, THEN emit ``post_kill_terminal`` per torn-down terminal + followed by ``post_kill_session``. No plugin code runs inside the critical + section — see the dispatch loop for why that matters on the API path. + + A terminal whose runtime teardown DEFERS (``dismantle_terminal_runtime`` + returns False — Grok has not yet released its private home, #596) keeps its + row: that row is the only retry handle for the deferred cleanup, so it is + neither deleted in step 5 nor swept, the session is reported in ``errors`` + instead of ``deleted``, and a re-run finishes the job. The tmux session is + still gone by then — the deferral is about on-disk provider state, not about + the session — so this is a partially-complete teardown, which is exactly what + the caller is told. + + If tmux cannot be confirmed dead we raise having changed nothing but two + snapshot files: the surviving session keeps its rows, its FIFO readers, its + status-monitor state and its providers, so it is still a fully working + session rather than a half-dismantled one, and a re-run reconciles it. + + NOTE — how far the guarantee actually reaches. It rests on two backend + properties, and is exactly as strong as the weaker one: + + * ``kill_session`` returning True only once the session is confirmed gone. + TmuxBackend polls; HerdrBackend does not yet (it returns the close + subprocess's exit code without a liveness check). + * ``session_exists_strict`` distinguishing absence from an unanswerable + lookup. TmuxBackend does, by classifying a ``list-sessions`` exit status + itself (``clients/tmux.py``); the ABC default just delegates to the lenient + ``session_exists``, which collapses any error to "absent" and so fails OPEN. + + On a backend with the default strict check, therefore, this teardown is no + safer than the pre-fix behavior in the lookup-error case — a transient error + still reads as "gone" and the rows still get dropped. The tmux path answers + False only on positive evidence of absence (its docstring lists the residual + holes it cannot close); herdr's does not, and it is not made worse here. + Tracked as a follow-up. Returns: Dict with 'deleted' (list of deleted session names) and 'errors' (list of error dicts). """ result: Dict = {"deleted": [], "errors": []} + # Terminals whose row was actually dropped, with the metadata their + # post_kill_terminal payload needs. Collected under the lock, dispatched + # after it is released. + torn_down: List[Tuple[str, Dict]] = [] try: - session_alive = get_backend().session_exists(session_name) - from cli_agent_orchestrator.services import terminal_service - terminals = list_terminals_by_session(session_name) - - # Clean up each terminal (snapshot, kill window, FIFO reader, - # status buffer, provider, DB) via the event-driven teardown path. - cleanup_complete = True - for terminal in terminals: + # Hold the lifecycle lock across the ENTIRE critical section. Nothing + # inside re-acquires it: terminal_service's teardown halves and + # plugin dispatch never touch session lifecycle, so there is no + # self-deadlock path. Guaranteed released on every exit, exceptions + # included (context manager). + with session_lifecycle_lock(session_name): + terminals = list_terminals_by_session(session_name) + incarnation_ids = [t["id"] for t in terminals] + + # Step 2: read-only scrollback/metadata capture, which has to happen + # while the panes still exist. ``metadata`` is kept because both + # steps in phase 5 need it (the row-deletion half builds the + # post_kill_terminal payload from it, after the row is gone). + captured: List[Tuple[str, Dict | None]] = [] + for terminal in terminals: + try: + metadata = terminal_service.capture_terminal_snapshot(terminal["id"]) + except Exception as e: + logger.warning(f"Failed to snapshot terminal {terminal['id']}: {e}") + metadata = None + captured.append((terminal["id"], metadata)) + + # Step 3/4: confirm the tmux session is gone BEFORE dismantling + # anything. Any inability to confirm raises with the session fully + # intact — never half-dismantled, never registry-less. + backend = get_backend() try: - if terminal_service.delete_terminal(terminal["id"], registry=registry) is False: + session_still_alive = backend.session_exists_strict(session_name) + except Exception as e: + raise RuntimeError( + f"could not verify tmux session '{session_name}' liveness during " + f"teardown ({e}); registry left intact for reconciliation on re-run" + ) from e + + if session_still_alive: + killed = backend.kill_session(session_name) + if not killed: + # False means "not found" OR "kill unconfirmed" (backend + # contract, backends/base.py). Only a still-alive session is + # a real failure. + try: + still_here = backend.session_exists_strict(session_name) + except Exception as e: + raise RuntimeError( + f"could not verify tmux session '{session_name}' liveness " + f"after kill_session ({e}); registry left intact for " + "reconciliation on re-run" + ) from e + if still_here: + raise RuntimeError( + f"tmux session '{session_name}' still exists after " + "kill_session; registry left intact for reconciliation " + "on re-run" + ) + + # Step 5: tmux is provably gone — now, and only now, dismantle the + # per-terminal runtime and drop the rows. kill_window=False: the + # windows died with the session, so the tmux-facing steps would only + # log spurious warnings. + # ``registry=None``: the per-terminal events are dispatched together + # with post_kill_session AFTER the lock is released — see below. + cleanup_complete = True + deferred_ids: List[str] = [] + for terminal_id, metadata in captured: + try: + runtime_released = terminal_service.dismantle_terminal_runtime( + terminal_id, metadata, kill_window=False + ) + except Exception as e: + logger.warning(f"Failed to cleanup terminal {terminal_id}: {e}") + runtime_released = True + if runtime_released is False: + # Grok has not yet released its private home (#596). The row + # is the only handle a retry has, so keep it: skip both the + # row delete and the sweep below for this id, and report the + # session as not fully deleted. cleanup_complete = False + deferred_ids.append(terminal_id) result["errors"].append( { - "terminal_id": terminal["id"], + "terminal_id": terminal_id, "error": "cleanup deferred; retry delete_session", } ) + continue + try: + if terminal_service.delete_terminal_row(terminal_id, metadata, registry=None): + if metadata: + torn_down.append((terminal_id, metadata)) + except Exception as e: + logger.warning(f"Failed to delete registry row for {terminal_id}: {e}") + + # Both remaining steps are guarded exactly like the two above, and + # for a sharper reason: by here the kill is CONFIRMED and every + # runtime and row is already gone, so the teardown has SUCCEEDED and + # is durable. Letting a raise out of this tail would report that + # completed teardown as a total failure AND — because all plugin + # dispatch is deferred past the lock — drop every event for it. The + # per-terminal events would be unrecoverable, since a re-run rebuilds + # ``torn_down`` from rows that no longer exist and can only re-emit + # post_kill_session. Reachable, not theoretical: the sweep is a DB + # write and the engine sets neither busy_timeout nor WAL, so + # ``database is locked`` is an ordinary outcome under CAO's concurrent + # writers. Nothing is swallowed — the failure is surfaced in + # ``result["errors"]`` and the log, just not as an exception that also + # destroys the event record. + + # Sweep any row the loop missed (e.g. a terminal whose row deletion + # raised above), scoped to this incarnation's ids so a later + # same-name session is never touched. Idempotent — a no-op when the + # loop already cleared them. Deferred ids are excluded: their rows + # are the retry handle for cleanup that has NOT happened yet. + try: + delete_terminals_by_ids([i for i in incarnation_ids if i not in deferred_ids]) + except Exception as e: + logger.warning(f"Failed to sweep registry rows for {session_name}: {e}") + result["errors"].append( + { + "session": session_name, + "step": "delete_terminals_by_ids", + "error": str(e), + } + ) + + # Drop the per-session forwarded-env mapping (issue #248). Safe + # even when no vars were forwarded — the helper is a no-op then. + try: + clear_session_env(session_name) + except Exception as e: + logger.warning(f"Failed to clear forwarded env for {session_name}: {e}") + result["errors"].append( + {"session": session_name, "step": "clear_session_env", "error": str(e)} + ) + + if cleanup_complete: + result["deleted"].append(session_name) + logger.info(f"Deleted session: {session_name}") + else: + logger.warning( + "Session %s backend was removed but terminal cleanup is deferred", session_name + ) + + # ALL plugin dispatch runs OUTSIDE the lock — per-terminal events + # included. Plugin code is third-party and unbounded, and on the API path + # it does not merely get scheduled: ``delete_session`` runs under + # ``asyncio.to_thread`` (api/main.py), so there is no running loop and + # ``dispatch_plugin_event`` falls back to ``asyncio.run``, executing the + # hook to completion inline. Dispatching from inside the critical section + # would therefore let one slow or hanging plugin hold the lifecycle lock + # and stall every subsequent create/teardown of this session name. Both + # events describe work that is already complete and durable by here, so + # emitting them after the release loses nothing. + # Per terminal, isolated: the teardown is finished and durable by here, so + # one unusable metadata dict (a missing ``tmux_session``, a validation + # error) must not turn a completed delete into a failure, nor cost the + # remaining terminals their event. ``dispatch_plugin_event`` already + # isolates the hook itself; this covers building the event around it. + for terminal_id, metadata in torn_down: + try: + dispatch_plugin_event( + registry, + "post_kill_terminal", + PostKillTerminalEvent( + session_id=metadata["tmux_session"], + terminal_id=terminal_id, + agent_name=metadata.get("agent_profile"), + ), + ) except Exception as e: - logger.warning(f"Failed to cleanup terminal {terminal['id']}: {e}") - - # Kill backend session only if it still exists - if session_alive: - get_backend().kill_session(session_name) - - # Drop the per-session forwarded-env mapping (issue #248). Safe - # even when no vars were forwarded — the helper is a no-op then. - clear_session_env(session_name) - - if cleanup_complete: - result["deleted"].append(session_name) - logger.info(f"Deleted session: {session_name}") - else: - logger.warning( - "Session %s backend was removed but terminal cleanup is deferred", session_name - ) + logger.warning(f"Failed to emit post_kill_terminal for {terminal_id}: {e}") dispatch_plugin_event( registry, "post_kill_session", diff --git a/src/cli_agent_orchestrator/services/terminal_service.py b/src/cli_agent_orchestrator/services/terminal_service.py index 5748c5c08..9de8a2b37 100644 --- a/src/cli_agent_orchestrator/services/terminal_service.py +++ b/src/cli_agent_orchestrator/services/terminal_service.py @@ -25,7 +25,7 @@ import time from datetime import datetime from enum import Enum -from typing import Any, Callable, Dict, List, Optional +from typing import Any, Callable, Dict, List, Optional, Tuple from cli_agent_orchestrator.backends.registry import get_backend from cli_agent_orchestrator.clients.database import ( @@ -80,6 +80,7 @@ get_session_env, set_session_env, ) +from cli_agent_orchestrator.services.session_lock import session_lifecycle_lock from cli_agent_orchestrator.services.status_monitor import status_monitor from cli_agent_orchestrator.services.step_output_store import _validate_key_part from cli_agent_orchestrator.utils.agent_profiles import load_agent_profile @@ -184,6 +185,76 @@ def _resolve_working_directory(working_directory: Optional[str]) -> str: ) +def _roll_back_backend_create_locked( + session_name: str, + window_name: str, + *, + created_session: bool, +) -> None: + """Undo the backend resource a create just made. CALLER MUST HOLD the + lifecycle lock for ``session_name``. + + Used by ``create_terminal``'s locked critical section so a failure between + the backend create and the registry write cannot leave a live tmux + session/window with no row. Both branches matter and they are NOT the same + teardown: + + * ``created_session=True`` -- this call created the whole session, so kill the + session and drop any forwarded env stashed for the name, so secrets don't + linger in memory or bleed into a future reuse of the name. + * ``created_session=False`` -- this call only added a WINDOW to a session that + already existed (``new_session=False``: every MCP spawn/assign-into-an- + existing-session call). Kill ONLY that window, so the pre-existing session + and its other terminals are left alone. Note this is not a guarantee that + the session survives: tmux drops a session when its last window dies, and + the peer window that made the session non-empty at the `session_exists` + check can be reaped by its own process exiting before this rollback runs -- + the lifecycle lock serializes CAO's transitions, not a pane's exit. In that + race the session collapses and the peer's registry row is left pointing at + a dead session. Killing the whole session instead would be strictly worse + (it would destroy peers that ARE alive, which is the common case), so this + stays window-scoped; the residual race is the same one the outer `except` + path already carries and is tracked separately. + + Best-effort and never raises: it runs while an exception is already in + flight, and that original failure is the one the caller must see. + """ + if created_session: + # `finally`, not a following statement: the env mapping must be dropped + # however the kill turns out -- including when it raises a BaseException + # (KeyboardInterrupt/SystemExit), which `except Exception` does not catch. + # Sequencing these as two independent try blocks skipped the clear on + # exactly that path, leaving a forwarded secret in the process-global map + # keyed to a session name that is gone and may later be reused. + # `finally` still lets a BaseException propagate, which is what we want: + # a Ctrl-C must not be swallowed here. + try: + if not get_backend().kill_session(session_name): + # Falsy means the backend could not confirm the kill (or found + # nothing to kill). Either way the name may still be live, so say + # so -- a silent branch here is how an orphan goes unnoticed. + logger.warning( + f"Rollback: kill_session({session_name}) did not confirm the kill; " + "the session may still be live" + ) + except Exception: + logger.exception(f"Rollback: failed to kill session {session_name}") + finally: + try: + clear_session_env(session_name) + except Exception: + logger.exception(f"Rollback: failed to clear session env for {session_name}") + else: + try: + if not get_backend().kill_window(session_name, window_name): + logger.warning( + f"Rollback: kill_window({session_name}:{window_name}) did not confirm " + "the kill; the window may still be live" + ) + except Exception: + logger.exception(f"Rollback: failed to kill window {session_name}:{window_name}") + + async def create_terminal( provider: str, agent_profile: str, @@ -369,52 +440,13 @@ async def create_terminal( # terminal's working_directory. This is the effective launch cwd either way. resolved_working_directory = _resolve_working_directory(working_directory) - # Step 2: Create tmux session or window - if new_session: + # Normalize the session name BEFORE anything keys off it: the lifecycle + # lock below is per session NAME, so it must be taken on the SAME string + # the tmux create and the registry row use, or a create and a teardown of + # what is really one session would take two different locks. + if new_session and not session_name.startswith(SESSION_PREFIX): # Ensure session name has the CAO prefix for identification - if not session_name.startswith(SESSION_PREFIX): - session_name = f"{SESSION_PREFIX}{session_name}" - - # Prevent duplicate sessions - if get_backend().session_exists(session_name): - raise ValueError(f"Session '{session_name}' already exists") - - # Wipe any stale mapping a prior aborted lifecycle for this name - # may have left behind, so a no-env relaunch can't inherit them. - clear_session_env(session_name) - - # Create new tmux session with initial window - get_backend().create_session( - session_name, - window_name, - terminal_id, - resolved_working_directory, - extra_env=env_vars, - ) - session_created = True # only set after successful creation - delete_terminals_by_session(session_name) - - # Persist forwarded env only after the tmux session actually - # exists; the failure path below clears it if a later step - # tears the session back down. - if env_vars: - set_session_env(session_name, env_vars) - else: - # Add window to existing session - if not get_backend().session_exists(session_name): - raise ValueError(f"Session '{session_name}' not found") - # Merge explicit per-step env_vars over the persisted session env - # (per-step wins on conflict): workflow routing ids like - # CAO_WORKFLOW_RUN_ID must reach the window even when it joins an - # existing session (issue #408). - window_name = get_backend().create_window( - session_name, - window_name, - terminal_id, - resolved_working_directory, - extra_env={**get_session_env(session_name), **(env_vars or {})}, - ) - window_created = True # only set after successful creation + session_name = f"{SESSION_PREFIX}{session_name}" # Step 3: Build a runtime skill catalog only for providers that consume # it at launch time (see RUNTIME_SKILL_PROMPT_PROVIDERS). @@ -438,20 +470,171 @@ async def create_terminal( f"copilot_cli." ) - # Step 3c: Persist terminal metadata to database after restrictions - # are resolved so API reads and snapshots report the actual launch policy. - db_create_terminal( - terminal_id, - session_name, - window_name, - provider, - agent_profile, - allowed_tools, - caller_id=caller_id, - engine=resolved_engine.value if resolved_engine is not None else None, - group=group, - metadata=metadata, - working_directory=resolved_working_directory, + # Step 3c: Create the tmux session/window and its registry row as ONE + # atomic step, under the per-session-name lifecycle lock (#498). This + # merges what used to be two separate steps -- the tmux create and the + # metadata persist -- precisely because they must become visible together. + # + # Note that everything above is already outside the lock by + # construction: profile load, Kiro engine resolution, tool-policy + # resolution and worktree provisioning are either pure reads or concern + # no session state, so the critical section stays down to the tmux + + # registry writes that actually have to be atomic against a concurrent + # teardown. That also lets the registry row be written exactly once, with + # its final allowed_tools and engine, inside the section. + # + # Why locked: without mutual exclusion a concurrent delete_session for + # the same name interleaves arbitrarily -- the teardown can decide the + # name is dead and then kill the session this call just created, or + # sweep between the tmux create and the row write, leaving one store + # holding state the other doesn't know about. Serializing per NAME (not + # globally) leaves creates of DIFFERENT sessions fully concurrent. + # + # Why on a worker thread: the lock is a threading primitive (the only + # kind reachable from both this coroutine and the synchronous teardown + # the API runs via to_thread -- see services/session_lock.py). Acquiring + # it directly here would block the EVENT LOOP for as long as a + # concurrent teardown of this name holds it (its tmux kill-verify poll + # and per-terminal FIFO joins are each seconds), freezing every other + # request. Off-loop, only this worker thread waits. + # + # Why the section ends here: provider.initialize() below can take tens + # of seconds, and a teardown of this name must never queue behind an + # agent launch. Everything inside is short, synchronous state mutation. + def _create_session_or_window_locked() -> Tuple[str, bool, bool]: + """Runs under the lifecycle lock on a worker thread. + + Returns (window_name, session_created, window_created) -- the caller + needs all three: create_window may rename the window, and the + failure path keys its cleanup off which one this call created. + + A failure after the backend create RETURNS rolls that resource back + HERE, still holding the lock, before re-raising: on return the tmux + session/window and its registry row both exist, and on such a raise + neither does. Without that the outer flags below would still be False + (they are only assigned from a successful RETURN), the `except` + cleanup would tear down nothing, and the failure would leave a live + tmux session with no registry row -- the exact divergence #498 exists + to eliminate. A "database is locked" OperationalError out of + db_create_terminal is an ordinary outcome under CAO's concurrent + writers, so this is a routine path, not a pathological one. + + NOT covered (pre-existing, and deliberately not claimed): a failure + INSIDE the backend create itself, after it has already made the tmux + resource but before it returns. `TmuxClient.create_session` lands the + session at `server.new_session(...)` and only then reads + `session.windows[0].name` -- a fresh list-windows fetch that can raise + (IndexError, or its own `ValueError` when the name is None), with + `create_window` shaped the same way. That leaks a session/window this + closure never learns about, so the rollback below cannot fire. Same + gap existed pre-#498, which set its flag only after the create + returned. Closing it needs the guard to extend INTO the backend + create; tracked separately. + + Why the rollback is INSIDE the lock rather than reported out to the + outer cleanup path: the lock's entire purpose is that, for one + session NAME, create and teardown are serialized so the name is + never observable half-built. Rolling back after release would reopen + that window -- between the release and the kill, another thread can + acquire the name and legitimately succeed (a new_session=False + create adding a window to what it sees as a live session, or a + teardown plus a fresh new_session=True create rebuilding the name) -- + and the late kill would then destroy an incarnation this call does + not own, leaving ITS row pointing at nothing. Under the lock the + name goes free -> free with no observable intermediate state. + """ + assert session_name is not None # narrowed by the caller + with session_lifecycle_lock(session_name): + if new_session: + # Prevent duplicate sessions + if get_backend().session_exists(session_name): + raise ValueError(f"Session '{session_name}' already exists") + + # Wipe any stale mapping a prior aborted lifecycle for this + # name may have left behind, so a no-env relaunch can't + # inherit them. + clear_session_env(session_name) + + # Create new tmux session with initial window + get_backend().create_session( + session_name, + window_name, + terminal_id, + resolved_working_directory, + extra_env=env_vars, + ) + created_window_name = window_name + created_session, created_window = True, False + else: + # Add window to existing session. Same lock, same reason: a + # window added mid-teardown would otherwise survive the + # session kill (or its row would be swept while the window + # lives on). + if not get_backend().session_exists(session_name): + raise ValueError(f"Session '{session_name}' not found") + # Merge explicit per-step env_vars over the persisted session + # env (per-step wins on conflict): workflow routing ids like + # CAO_WORKFLOW_RUN_ID must reach the window even when it + # joins an existing session (issue #408). + created_window_name = get_backend().create_window( + session_name, + window_name, + terminal_id, + resolved_working_directory, + extra_env={**get_session_env(session_name), **(env_vars or {})}, + ) + created_session, created_window = False, True + + # From here the backend resource EXISTS, so every remaining step + # is guarded: on failure the resource is rolled back under this + # same lock before the exception leaves the closure. See the + # docstring for why the rollback belongs here and not in the + # caller's `except`. + try: + if created_session: + # Drop rows a previous incarnation of this session name + # left behind. Inside the lock, so it can never race the + # row write of a concurrent create for the same name. + delete_terminals_by_session(session_name) + + if env_vars: + # Persist forwarded env only after the tmux session + # actually exists; rolled back below if a later step + # tears the session down again. + set_session_env(session_name, env_vars) + + # Persist the registry row INSIDE the critical section so the + # tmux session/window and its row become visible together. A + # teardown that observes the new tmux state is then guaranteed + # to also observe the row, instead of killing a session whose + # row it cannot see and leaving it orphaned. The row carries + # the launch policy already resolved above (allowed_tools, + # engine), so API reads and snapshots report what was actually + # launched. + db_create_terminal( + terminal_id, + session_name, + created_window_name, + provider, + agent_profile, + allowed_tools, + caller_id=caller_id, + engine=resolved_engine.value if resolved_engine is not None else None, + group=group, + metadata=metadata, + working_directory=resolved_working_directory, + ) + except BaseException: + _roll_back_backend_create_locked( + session_name, + created_window_name, + created_session=created_session, + ) + raise + return created_window_name, created_session, created_window + + window_name, session_created, window_created = await asyncio.to_thread( + _create_session_or_window_locked ) # Step 4/5: Set up the FIFO event-driven output pipeline for pipe-pane @@ -1623,145 +1806,242 @@ def read_output_range(terminal_id: str, offset: int, length: int) -> str: return data.decode("utf-8", errors="replace") -def delete_terminal(terminal_id: str, registry: PluginRegistry | None = None) -> bool: - """Delete terminal and kill its tmux window.""" +def capture_terminal_snapshot(terminal_id: str) -> Optional[Dict]: + """Persist a terminal's scrollback + metadata snapshot. NON-DESTRUCTIVE. + + The read-only first third of terminal teardown, split out so session + teardown can run it BEFORE the session kill while leaving every destructive + step until AFTER the kill is confirmed (#498). It has to precede the kill -- + scrollback only exists while the pane does -- and because it only reads tmux + and writes two files under ``TERMINAL_LOG_DIR``, running it ahead of a kill + that then fails to confirm changes no terminal state at all. + + Returns the terminal's metadata (both later thirds need it), or None when no + registry row exists -- i.e. there is nothing to tear down. The returned dict + carries one key that is NOT a registry column: ``live_working_directory``, + the pane's cwd read here while the pane still exists. + ``dismantle_terminal_runtime`` needs it for issue #100's worktree cleanup and + cannot read it itself -- on the session-teardown path the pane is already + gone by the time it runs -- so the single read is captured here and passed + along rather than repeated. + """ + metadata = get_terminal_metadata(terminal_id) + if not metadata: + return None + + # Read the pane's live working directory BEFORE anything destroys the pane. + # Single read, reused for two purposes: the scrollback snapshot below, and + # issue #100 Phase 1's worktree cleanup (recognizing a worktree-backed + # terminal from its live cwd alone -- there is no separate CAO-side record + # of which terminals are worktree-backed). Best-effort: a read failure + # means the snapshot's working_directory field is None and no worktree + # cleanup runs later. + live_working_directory = None try: - # Unregister from herdr inbox service - svc = get_herdr_inbox_service() - if svc: - try: - svc.unregister_terminal(terminal_id) - except Exception as e: - logger.warning(f"Failed to unregister terminal {terminal_id} from herdr inbox: {e}") + live_working_directory = get_backend().get_pane_working_directory( + metadata["tmux_session"], metadata["tmux_window"] + ) + except Exception as e: + logger.warning(f"Failed to read working directory for {terminal_id}: {e}") + metadata["live_working_directory"] = live_working_directory - # Get metadata before deletion - metadata = get_terminal_metadata(terminal_id) + # Snapshot scrollback + metadata before killing (for debugging/restore) + try: + # Capture plain text full scrollback (no -e, no line cap) + scrollback = get_backend().get_history( + metadata["tmux_session"], + metadata["tmux_window"], + strip_escapes=True, + full_history=True, + ) + scrollback_path = TERMINAL_LOG_DIR / f"{terminal_id}.scrollback" + scrollback_path.write_text(scrollback, encoding="utf-8") - if metadata: - # Read the pane's live working directory BEFORE kill_window below - # destroys the pane. Single read, reused for two purposes: the - # scrollback snapshot below, and issue #100 Phase 1's worktree - # cleanup (recognizing a worktree-backed terminal from its live - # cwd alone -- there is no separate CAO-side record of which - # terminals are worktree-backed). Best-effort: a read failure - # means the snapshot's working_directory field is None and no - # worktree cleanup runs below. - live_working_directory = None - try: - live_working_directory = get_backend().get_pane_working_directory( - metadata["tmux_session"], metadata["tmux_window"] - ) - except Exception as e: - logger.warning(f"Failed to read working directory for {terminal_id}: {e}") + import json as _json - # Snapshot scrollback + metadata before killing (for debugging/restore) - try: - # Capture plain text full scrollback (no -e, no line cap) - scrollback = get_backend().get_history( - metadata["tmux_session"], - metadata["tmux_window"], - strip_escapes=True, - full_history=True, - ) - scrollback_path = TERMINAL_LOG_DIR / f"{terminal_id}.scrollback" - scrollback_path.write_text(scrollback, encoding="utf-8") - - import json as _json - - snapshot = { - "terminal_id": terminal_id, - "session_name": metadata["tmux_session"], - "window_name": metadata["tmux_window"], - "agent_profile": metadata.get("agent_profile"), - "provider": metadata["provider"], - "working_directory": live_working_directory, - "allowed_tools": metadata.get("allowed_tools"), - "caller_id": metadata.get("caller_id"), - } - snapshot_path = TERMINAL_LOG_DIR / f"{terminal_id}.snapshot.json" - snapshot_path.write_text(_json.dumps(snapshot, indent=2), encoding="utf-8") - except Exception as e: - logger.warning(f"Failed to snapshot terminal {terminal_id}: {e}") + snapshot = { + "terminal_id": terminal_id, + "session_name": metadata["tmux_session"], + "window_name": metadata["tmux_window"], + "agent_profile": metadata.get("agent_profile"), + "provider": metadata["provider"], + "working_directory": live_working_directory, + "allowed_tools": metadata.get("allowed_tools"), + "caller_id": metadata.get("caller_id"), + } + snapshot_path = TERMINAL_LOG_DIR / f"{terminal_id}.snapshot.json" + snapshot_path.write_text(_json.dumps(snapshot, indent=2), encoding="utf-8") + except Exception as e: + logger.warning(f"Failed to snapshot terminal {terminal_id}: {e}") - # Stop pipe-pane logging - try: - get_backend().stop_pipe_pane(metadata["tmux_session"], metadata["tmux_window"]) - except Exception as e: - logger.warning(f"Failed to stop pipe-pane for {terminal_id}: {e}") + return metadata - # Stop FIFO reader and cleanup FIFO file. Must run BEFORE kill_window - # so the reader thread (which reopens the FIFO on EOF) unblocks and - # joins before the pane disappears. - try: - fifo_manager.stop_reader(terminal_id) - except Exception as e: - logger.warning(f"Failed to stop FIFO reader for {terminal_id}: {e}") - # Clear state detector buffers for this terminal - try: - status_monitor.clear_terminal(terminal_id) - except Exception as e: - logger.warning(f"Failed to clear state detector for {terminal_id}: {e}") +def dismantle_terminal_runtime( + terminal_id: str, + metadata: Optional[Dict], + kill_window: bool = True, +) -> bool: + """Tear down a terminal's runtime state, but NOT its registry row. + + The destructive middle third: herdr inbox deregistration, pipe-pane stop, + FIFO reader stop, status-monitor clear, the tmux window kill, worktree + cleanup, provider cleanup, and the per-terminal bookkeeping registries. Every + step is individually guarded and idempotent, so re-running it on an + already-dismantled terminal is a no-op -- which is what makes a re-run after + a failed session teardown safe. + + ``kill_window=False`` skips the two tmux-facing steps (pipe-pane stop and the + window kill). Session teardown passes False because it has already confirmed + the whole tmux SESSION gone, so the window no longer exists and both calls + would only produce spurious warnings. + + Returns False when provider cleanup was DEFERRED (Grok's private-home owner + could not yet be inspected/stopped), meaning the caller must keep the + registry row so a later DELETE can retry; True when the runtime is fully + dismantled. Reporting True on a deferral would turn a temporary process race + into a permanent private-home leak. + + Ordering note: stopping the FIFO reader before killing the window is + preferred but not load-bearing -- since issue #382 the reader loop uses a + non-blocking fd plus a ``select`` timeout and holds its own keepalive write + end, so it can never park waiting on the pane and always observes the stop + flag within one poll interval. + """ + # Unregister from herdr inbox service + svc = get_herdr_inbox_service() + if svc: + try: + svc.unregister_terminal(terminal_id) + except Exception as e: + logger.warning(f"Failed to unregister terminal {terminal_id} from herdr inbox: {e}") + if metadata and kill_window: + # Stop pipe-pane logging. Before the FIFO steps below, so the pane stops + # writing to the FIFO before its reader (and the FIFO file) go away. + try: + get_backend().stop_pipe_pane(metadata["tmux_session"], metadata["tmux_window"]) + except Exception as e: + logger.warning(f"Failed to stop pipe-pane for {terminal_id}: {e}") + + # Deliberately OUTSIDE the `if metadata:` block below: both of these need + # only terminal_id. Gating them on metadata meant a failed snapshot (a + # `get_terminal_metadata` that raised, or "database is locked") skipped them, + # orphaning the FIFO reader thread and the status-detector buffers for a + # terminal whose row the by-id sweep then deleted anyway -- a reader with + # nothing left to read from and no record it exists. + try: + fifo_manager.stop_reader(terminal_id) + except Exception as e: + logger.warning(f"Failed to stop FIFO reader for {terminal_id}: {e}") + + # Clear state detector buffers for this terminal + try: + status_monitor.clear_terminal(terminal_id) + except Exception as e: + logger.warning(f"Failed to clear state detector for {terminal_id}: {e}") + + if metadata: + if kill_window: # Kill the tmux window (this terminates the agent process) try: get_backend().kill_window(metadata["tmux_session"], metadata["tmux_window"]) except Exception as e: logger.warning(f"Failed to kill tmux window for {terminal_id}: {e}") - # issue #100 Phase 1: if this terminal was worktree-backed (its live - # cwd matched the CAO-managed worktree path shape), remove the - # worktree + branch now that the process using it is gone. - # `remove_worktree` is itself best-effort/never-raises, matching - # every other step in this teardown. - # - # The parsed terminal_id MUST match the terminal actually being - # deleted here, not just "some" CAO worktree path. Without this - # guard: a worktree-backed terminal A (cwd - # .../.cao/worktrees/A) can spawn a non-worktree terminal B with - # working_directory explicitly set to A's cwd (handoff/assign - # both accept an explicit working_directory, and "here" -- the - # caller's own directory -- is a common choice). Deleting B -- - # including handoff's automatic success teardown -- would then - # read B's pane cwd (== A's worktree path), parse terminal_id - # "A" out of it, and force-remove A's still-running worktree. - # Mismatched parses now fall through as a no-op leak (Phase 3 - # territory) instead of destroying another terminal's checkout. - parsed = worktree_service.parse_worktree_path(live_working_directory) - if parsed is not None: - worktree_repo_root, worktree_terminal_id = parsed - if worktree_terminal_id == terminal_id: - worktree_service.remove_worktree(worktree_repo_root, worktree_terminal_id) - - # Grok cleanup can be deferred when a private-home owner cannot yet be - # inspected/stopped. Keep both the provider mapping and DB metadata so - # a subsequent DELETE can retry; reporting success here would turn a - # temporary process race into a permanent private-home leak. - if provider_manager.cleanup_provider(terminal_id) is False: + # issue #100 Phase 1: if this terminal was worktree-backed (its live + # cwd matched the CAO-managed worktree path shape), remove the + # worktree + branch now that the process using it is gone. + # `remove_worktree` is itself best-effort/never-raises, matching + # every other step in this teardown. + # + # The parsed terminal_id MUST match the terminal actually being + # deleted here, not just "some" CAO worktree path. Without this + # guard: a worktree-backed terminal A (cwd + # .../.cao/worktrees/A) can spawn a non-worktree terminal B with + # working_directory explicitly set to A's cwd (handoff/assign + # both accept an explicit working_directory, and "here" -- the + # caller's own directory -- is a common choice). Deleting B -- + # including handoff's automatic success teardown -- would then + # read B's pane cwd (== A's worktree path), parse terminal_id + # "A" out of it, and force-remove A's still-running worktree. + # Mismatched parses now fall through as a no-op leak (Phase 3 + # territory) instead of destroying another terminal's checkout. + parsed = worktree_service.parse_worktree_path(metadata.get("live_working_directory")) + if parsed is not None: + worktree_repo_root, worktree_terminal_id = parsed + if worktree_terminal_id == terminal_id: + worktree_service.remove_worktree(worktree_repo_root, worktree_terminal_id) + + # Grok cleanup can be deferred when a private-home owner cannot yet be + # inspected/stopped. Keep both the provider mapping and DB metadata so + # a subsequent DELETE can retry; reporting success here would turn a + # temporary process race into a permanent private-home leak. + if provider_manager.cleanup_provider(terminal_id) is False: + return False + with _memory_injected_lock: + _memory_injected_terminals.discard(terminal_id) + # Drop any per-curator dispatch lock so the registry doesn't grow + # forever as memory_manager terminals come and go. + from cli_agent_orchestrator.services.memory_service import _curator_locks + + _curator_locks.pop(terminal_id, None) + return True + + +def delete_terminal_row( + terminal_id: str, + metadata: Optional[Dict], + registry: PluginRegistry | None = None, +) -> bool: + """Drop a terminal's registry row and emit ``post_kill_terminal``. + + The final third of terminal teardown, split out so session teardown can + defer it past its kill-confirmation point (#498) -- deleting a row for a + session that turns out to still be alive is exactly how the registry and + tmux diverge. ``metadata`` is what ``capture_terminal_snapshot`` returned; + it is needed for the event payload because the row is gone by the time the + event is built. + + ``registry=None`` drops the row WITHOUT emitting. Session teardown passes + None and emits the events itself once it has released the lifecycle lock, so + that no third-party plugin ever runs inside its critical section; the + single-terminal ``delete_terminal`` path holds no such lock and passes its + registry straight through. + """ + deleted = db_delete_terminal(terminal_id) + logger.info(f"Deleted terminal: {terminal_id}") + if deleted and metadata: + dispatch_plugin_event( + registry, + "post_kill_terminal", + PostKillTerminalEvent( + session_id=metadata["tmux_session"], + terminal_id=terminal_id, + agent_name=metadata.get("agent_profile"), + ), + ) + return deleted + + +def delete_terminal(terminal_id: str, registry: PluginRegistry | None = None) -> bool: + """Delete terminal and kill its tmux window. + + Single-terminal teardown: all three thirds back to back, in the order they + have always run. Session teardown does NOT use this -- it interleaves its own + tmux kill-confirmation between them (see ``services/session_service.py``). + + Returns False when the teardown was deferred (see + ``dismantle_terminal_runtime``), leaving the row in place for a retry. + """ + try: + metadata = capture_terminal_snapshot(terminal_id) + if not dismantle_terminal_runtime(terminal_id, metadata): logger.warning( "Terminal %s cleanup deferred; retaining metadata for a retry", terminal_id ) return False - with _memory_injected_lock: - _memory_injected_terminals.discard(terminal_id) - # Drop any per-curator dispatch lock so the registry doesn't grow - # forever as memory_manager terminals come and go. - from cli_agent_orchestrator.services.memory_service import _curator_locks - - _curator_locks.pop(terminal_id, None) - deleted = db_delete_terminal(terminal_id) - logger.info(f"Deleted terminal: {terminal_id}") - if deleted and metadata: - dispatch_plugin_event( - registry, - "post_kill_terminal", - PostKillTerminalEvent( - session_id=metadata["tmux_session"], - terminal_id=terminal_id, - agent_name=metadata.get("agent_profile"), - ), - ) - return deleted + return delete_terminal_row(terminal_id, metadata, registry=registry) except Exception as e: logger.error(f"Failed to delete terminal {terminal_id}: {e}") diff --git a/test/clients/test_tmux_client.py b/test/clients/test_tmux_client.py index 28e816941..0be79812b 100644 --- a/test/clients/test_tmux_client.py +++ b/test/clients/test_tmux_client.py @@ -517,16 +517,83 @@ def test_get_session_windows_error(self, tmux): # ── kill_session ───────────────────────────────────────────────────── +def _cmd_result(returncode, stdout=(), stderr=()): + """Build a stand-in for libtmux's ``tmux_cmd`` result object. + + ``session_exists_strict`` reads exactly three attributes off it, so this is + the whole surface. Used to drive the verify poll, which now runs its own + ``list-sessions`` instead of touching ``server.sessions`` (#498). + """ + result = MagicMock() + result.returncode = returncode + result.stdout = list(stdout) + result.stderr = list(stderr) + return result + + class TestKillSession: def test_kill_session_success(self, tmux): mock_session = MagicMock() tmux.server.sessions.get.return_value = mock_session + # The strict verify runs list-sessions: exit 0 with "ses" absent from the + # name list is an authoritative "gone" (#498). + tmux.server.cmd.return_value = _cmd_result(0, stdout=["other"]) result = tmux.kill_session("ses") assert result is True mock_session.kill.assert_called_once() + def test_kill_session_polls_until_session_confirmed_gone(self, tmux, monkeypatch): + """The BOUNDED RETRY loop is what makes True mean "confirmed gone". + + tmux does not always reap a session synchronously with ``session.kill()``, + so the primitive polls. Here the session is still listed on the first + verify and only absent on the second: kill_session must keep polling and + return True, having slept between attempts. Only immediate-success and + the timeout=0 path were covered before, leaving the retry loop — the + whole point of the confirmation contract — unexercised (#498). + """ + mock_session = MagicMock() + tmux.server.sessions.get.return_value = mock_session + # 1st verify: still listed -> must sleep and retry. 2nd: gone -> True. + tmux.server.cmd.side_effect = [ + _cmd_result(0, stdout=["ses"]), + _cmd_result(0, stdout=[]), + ] + sleeps: list[float] = [] + monkeypatch.setattr( + "cli_agent_orchestrator.clients.tmux.time.sleep", lambda s: sleeps.append(s) + ) + + result = tmux.kill_session("ses") + + assert result is True + mock_session.kill.assert_called_once() + # Exactly one retry: it slept once, between the alive verify and the + # one that confirmed absence. + assert sleeps == [tmux._KILL_SESSION_VERIFY_INTERVAL_SECONDS] + assert tmux.server.cmd.call_count == 2 + + def test_kill_session_lookup_error_during_verify_is_not_gone(self, tmux, monkeypatch): + """A transient lookup error during the verification poll must NOT be + read as "session gone": kill_session returns False, never a false True + (#498).""" + mock_session = MagicMock() + tmux.server.sessions.get.return_value = mock_session + # Found on the initial lookup; the verify's list-sessions then fails in a + # way that is NOT an absence (permission denied), so the strict check + # raises TmuxLookupError, which must be caught as a failed kill. + tmux.server.cmd.return_value = _cmd_result( + 1, stderr=["error connecting to /tmp/x.sock (Permission denied)"] + ) + monkeypatch.setattr(tmux, "_KILL_SESSION_VERIFY_TIMEOUT_SECONDS", 0) + + result = tmux.kill_session("ses") + + assert result is False + mock_session.kill.assert_called_once() + def test_kill_session_not_found(self, tmux): tmux.server.sessions.get.return_value = None @@ -541,6 +608,19 @@ def test_kill_session_error(self, tmux): assert result is False + def test_kill_session_returns_false_when_session_survives(self, tmux, monkeypatch): + mock_session = MagicMock() + tmux.server.sessions.get.return_value = mock_session + # Every verify authoritatively still lists the session, so the bounded + # poll expires without confirmation. + tmux.server.cmd.return_value = _cmd_result(0, stdout=["ses"]) + monkeypatch.setattr(tmux, "_KILL_SESSION_VERIFY_TIMEOUT_SECONDS", 0) + + result = tmux.kill_session("ses") + + assert result is False + mock_session.kill.assert_called_once() + # ── kill_window ────────────────────────────────────────────────────── diff --git a/test/clients/test_tmux_lookup_error.py b/test/clients/test_tmux_lookup_error.py index e07c69175..d2a6000af 100644 --- a/test/clients/test_tmux_lookup_error.py +++ b/test/clients/test_tmux_lookup_error.py @@ -37,6 +37,21 @@ def parse_failure(*, times: int, then=None): return [ValueError(ZIP_ERROR)] * times + ([then] if then is not None else []) +def list_sessions_result(*names, returncode=0): + """Stand-in for libtmux's ``tmux_cmd`` result for a ``list-sessions`` call. + + ``kill_session``'s verification poll asks ``session_exists_strict``, which + runs its own ``list-sessions`` through ``server.cmd`` rather than reading + ``server.sessions`` — so the parse failures these tests inject never reach + it and it needs an answer of its own (#498). + """ + result = MagicMock() + result.returncode = returncode + result.stdout = list(names) + result.stderr = [] + return result + + def make_window(pane=None): window = MagicMock() if pane is not None: @@ -305,6 +320,10 @@ def test_other_post_creation_failure_rolls_the_session_back(self, tmux, tmp_path class TestKillParseFailure: def test_kill_session_falls_back_to_the_cli(self, tmux): tmux.server.sessions.get.side_effect = parse_failure(times=2) + # The CLI kill exiting 0 is not on its own a True: the verification poll + # still has to see the session gone. Exit 0 with "ses" absent from the + # name list is that authoritative "gone" (#498). + tmux.server.cmd.return_value = list_sessions_result() with patch("cli_agent_orchestrator.clients.tmux.subprocess") as mock_subprocess: mock_subprocess.run.return_value = MagicMock(returncode=0, stderr="") @@ -317,6 +336,24 @@ def test_kill_session_falls_back_to_the_cli(self, tmux): "=ses", ] + def test_cli_fallback_still_has_to_confirm_the_session_is_gone(self, tmux, monkeypatch): + """A dispatched CLI kill is not a confirmed kill. + + The fallback is exempt from the parse-prone listing, not from the + confirmation contract: with the session still listed after the CLI kill + exits 0, kill_session must report False so teardown leaves the registry + intact instead of dropping rows for a live session (#498). + """ + tmux.server.sessions.get.side_effect = parse_failure(times=2) + tmux.server.cmd.return_value = list_sessions_result("ses") + monkeypatch.setattr(tmux, "_KILL_SESSION_VERIFY_TIMEOUT_SECONDS", 0) + + with patch("cli_agent_orchestrator.clients.tmux.subprocess") as mock_subprocess: + mock_subprocess.run.return_value = MagicMock(returncode=0, stderr="") + assert tmux.kill_session("ses") is False + + mock_subprocess.run.assert_called_once() + def test_kill_session_reports_absence_without_calling_the_cli(self, tmux): tmux.server.sessions.get.return_value = None diff --git a/test/clients/test_tmux_session_exists_strict.py b/test/clients/test_tmux_session_exists_strict.py new file mode 100644 index 000000000..24e619fa0 --- /dev/null +++ b/test/clients/test_tmux_session_exists_strict.py @@ -0,0 +1,670 @@ +"""``TmuxClient.session_exists_strict`` against a REAL tmux server (#498). + +This is the primitive the whole confirm-then-dismantle teardown rests on: it must +keep "the session is gone" apart from "the lookup failed", because +``delete_session`` deletes registry rows only once tmux is provably gone, and a +lookup error reaching it as False is exactly how rows get dropped from under a +live session. + +Every test here runs a real ``tmux`` on a private socket, so the classification is +pinned against the tool's actual exit statuses and messages rather than against a +mock of the layer under test. That distinction is load-bearing: the bug this file +was written for was that the previous implementation went through libtmux's +``Server.sessions`` property, which wraps its ``list-sessions`` fetch in +``try/except Exception: pass``. Under a MagicMock that looks fine; against a real +socket failure it yields an EMPTY session list, so an unanswerable lookup reports +a clean absence and the check fails OPEN. No amount of mocking finds that — only +a real transport failure does. + +Transient failures are injected AT THE TMUX BOUNDARY (pointing the client at a +socket path that cannot be spoken to, unlinking the socket, or killing the server +underneath it), never by stubbing ``session_exists_strict`` or the ``cmd`` call +itself. + +One constraint the injection vectors have to respect: **tmux's stderr wording is +version-dependent.** Pointing tmux at a socket path that is a regular file yields +``Socket operation on non-socket`` (ENOTSOCK) on tmux 3.7b but ``no server running +on `` (ECONNREFUSED) on tmux 3.2a — the earlier version's message reads as a +confirmed absence, so tests built on that vector are red on tmux < 3.3. The +messages tmux renders as ``error connecting to ()`` are worded by +the kernel's errno instead, so they are stable across versions; ``_unreachable`` +below uses one of those (ENOTDIR), and it is also privilege-independent, unlike +the permission-denied vector. +""" + +import contextlib +import os +import shutil +import signal +import subprocess +import tempfile +import time +import uuid +from pathlib import Path +from unittest import mock + +import pytest + +from cli_agent_orchestrator.clients import tmux as tmux_module +from cli_agent_orchestrator.clients.tmux import TmuxClient, TmuxLookupError + +pytestmark = pytest.mark.skipif(shutil.which("tmux") is None, reason="tmux not installed") + +_LSOF_AVAILABLE = shutil.which("lsof") is not None +requires_lsof = pytest.mark.skipif(not _LSOF_AVAILABLE, reason="lsof not installed") + +# A binary name that cannot exist, for making a detector tier unavailable. +_MISSING_BINARY = "cao-no-such-binary-abc123" + + +def _hide_kernel_socket_table(monkeypatch): + """Make the ladder behave as it does on a host without ``/proc/net/unix``.""" + monkeypatch.setattr(tmux_module, "_PROC_NET_UNIX", "/nonexistent/proc/net/unix") + + +def _pretend_to_be_macos(monkeypatch): + """Drive the detector ladder down its macOS path from this Linux host. + + Both halves of what a Mac is, through the module's two seams: + + * ``_HOST_PLATFORM`` selects the LADDER (``_socket_table_detectors`` puts + ``/proc/net/unix`` first on Linux and leaves it out entirely elsewhere), and + * ``_PROC_NET_UNIX`` points somewhere nonexistent, because on a Mac it is. + + Setting only the platform would prove nothing here — this box's real + ``/proc/net/unix`` would still answer if the ladder wrongly consulted it. With + both set, a ladder that still leads with the kernel table gets no answer at + all and falls through to the coarse process tier, which is precisely the + fixup-5 regression. + + It is NOT a substitute for running the suite on a Mac: only the real platform + proves that macOS lsof prints what this parser reads. + """ + monkeypatch.setattr(tmux_module, "_HOST_PLATFORM", "darwin") + _hide_kernel_socket_table(monkeypatch) + + +def _hide_lsof(monkeypatch): + monkeypatch.setattr(tmux_module, "_LSOF_COMMAND", _MISSING_BINARY) + + +def _client_on(socket_path): + """A TmuxClient talking to ``socket_path`` instead of the default server.""" + import libtmux + + client = TmuxClient() + client.server = libtmux.Server(socket_path=str(socket_path)) + return client + + +@pytest.fixture +def short_tmpdir(): + """A scratch directory with a SHORT absolute path, removed afterwards. + + Not ``tmp_path``: pytest derives it from the test name under + ``$TMPDIR/pytest-of-/``, which on macOS already blows past the ~104-byte + ``sockaddr_un`` limit — tmux then fails every command with "File name too + long" and the tests measure that instead of what they mean to. + """ + directory = Path(tempfile.mkdtemp(prefix="cao-t4-", dir="/tmp")) + try: + yield directory + finally: + shutil.rmtree(directory, ignore_errors=True) + + +@pytest.fixture +def tmux_socket(short_tmpdir): + """A private tmux socket path, with the server killed on the way out. + + Private socket: these tests must never see — or reap — the developer's own + tmux sessions, and CAO's real sessions live on the default socket. + """ + socket_path = short_tmpdir / f"s-{uuid.uuid4().hex[:6]}.sock" + try: + yield socket_path + finally: + subprocess.run( + ["tmux", "-S", str(socket_path), "kill-server"], + capture_output=True, + check=False, + ) + + +@pytest.fixture +def unrelated_tmux_server(short_tmpdir): + """A live tmux server on a DIFFERENT private socket, killed on the way out. + + Present so the coarse process-table tier cannot answer ABSENT by accident: + with some tmux process running that does not name the socket under test, that + tier says UNKNOWN (→ fail closed → raise). Every developer machine looks like + this, which is why the macOS regression showed up as "teardown always + raises". Tests that must prove the SOCKET TABLE reached a verdict take this + fixture so passing via "no tmux anywhere" is impossible. + """ + socket_path = short_tmpdir / f"other-{uuid.uuid4().hex[:6]}.sock" + subprocess.run( + ["tmux", "-S", str(socket_path), "new-session", "-d", "-s", "cao-unrelated"], + capture_output=True, + check=True, + ) + try: + yield socket_path + finally: + subprocess.run( + ["tmux", "-S", str(socket_path), "kill-server"], capture_output=True, check=False + ) + + +def _start_session(socket_path, session_name): + subprocess.run( + ["tmux", "-S", str(socket_path), "new-session", "-d", "-s", session_name], + capture_output=True, + check=True, + ) + + +def _server_pid(socket_path): + """PID of the tmux server on ``socket_path`` (asked while it is reachable).""" + result = subprocess.run( + ["tmux", "-S", str(socket_path), "display-message", "-p", "#{pid}"], + capture_output=True, + text=True, + check=True, + ) + return int(result.stdout.strip()) + + +def _unreachable(directory): + """A socket path tmux cannot speak to, with VERSION-STABLE stderr. + + The path traverses a regular file, so path resolution fails with ENOTDIR and + tmux reports ``error connecting to (Not a directory)``. The wording + comes from ``strerror`` rather than from tmux, so unlike the non-socket vector + it does not change between tmux versions (measured identical on 3.2a), and + unlike the permission-denied vector it behaves the same for root. + + What matters is that it is genuinely UNANSWERABLE: a server could be running + for all we know, we simply cannot ask. + """ + blocker = directory / "not-a-directory" + if not blocker.exists(): + blocker.write_text("blocks path resolution") + return blocker / "s.sock" + + +def _wait_until(predicate, timeout=5.0): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(0.05) + return predicate() + + +def _process_is_alive(pid): + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + return True + + +class TestConfirmedAnswers: + """The two cases the check is allowed to answer.""" + + def test_live_session_is_true(self, tmux_socket): + _start_session(tmux_socket, "cao-alive") + + assert _client_on(tmux_socket).session_exists_strict("cao-alive") is True + + def test_absent_session_on_a_running_server_is_false(self, tmux_socket): + _start_session(tmux_socket, "cao-alive") + + assert _client_on(tmux_socket).session_exists_strict("cao-absent") is False + + def test_match_is_exact_not_a_prefix(self, tmux_socket): + """``cao-demo`` must not answer for ``cao-demo-2`` or vice versa. + + tmux's own target syntax prefix-matches by default (``-t foo`` finds + ``foobar``), so a check built on it could report a DIFFERENT session's + liveness — and a teardown would then refuse to dismantle a genuinely dead + session, or worse, act on the wrong one. + """ + _start_session(tmux_socket, "cao-demo-2") + client = _client_on(tmux_socket) + + assert client.session_exists_strict("cao-demo-2") is True + assert client.session_exists_strict("cao-demo") is False + assert client.session_exists_strict("cao-demo-22") is False + + def test_no_server_at_all_is_a_confirmed_absence(self, tmux_socket): + """A socket nothing is listening on: False, NOT an error. + + No server holds no sessions, so there is nothing a teardown could orphan. + Classifying this as unanswerable would strand registry rows left over from + a server that has since died — ``delete_session`` would raise forever and + never reconcile them. + """ + assert not os.path.exists(tmux_socket) + + assert _client_on(tmux_socket).session_exists_strict("cao-anything") is False + + def test_server_shut_down_under_us_is_a_confirmed_absence(self, tmux_socket): + """Same, via the other message tmux uses ("no server running on ..."). + + Reached when the socket FILE outlives its server, which is what a + ``kill-server`` leaves behind — the ordinary end state of a CAO session, + so this must not be an error either. + """ + _start_session(tmux_socket, "cao-alive") + client = _client_on(tmux_socket) + assert client.session_exists_strict("cao-alive") is True + + subprocess.run( + ["tmux", "-S", str(tmux_socket), "kill-server"], capture_output=True, check=True + ) + + assert client.session_exists_strict("cao-alive") is False + + +class TestUnanswerableLookupsFailClosed: + """The case that must NOT be answered — the point of the whole method. + + Each injects a real transport failure and asserts ``TmuxLookupError``. Before + #498's fix these all returned False ("session gone"), which is what let a + teardown drop the registry rows of a session that was still running. + """ + + def test_socket_path_cannot_be_resolved(self, short_tmpdir): + """tmux: "error connecting to ... (Not a directory)" — cannot tell, raise.""" + with pytest.raises(TmuxLookupError, match="could not determine whether"): + _client_on(_unreachable(short_tmpdir)).session_exists_strict("cao-demo") + + @pytest.mark.skipif( + os.geteuid() == 0, reason="root is not stopped by mode 000, so this vector cannot inject" + ) + def test_socket_directory_is_unreadable(self, short_tmpdir): + """tmux: "Permission denied" — cannot tell, must raise. + + The closest reachable analogue of the transient socket failure a loaded or + sandboxed host produces: the path could name a live server, we simply + cannot ask. + """ + locked = short_tmpdir / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + try: + with pytest.raises(TmuxLookupError, match="could not determine whether"): + _client_on(locked / "s.sock").session_exists_strict("cao-demo") + finally: + os.chmod(locked, 0o755) + + def test_error_message_names_the_session_and_the_cause(self, short_tmpdir): + """The raise has to be diagnosable — it surfaces as an HTTP 500 body.""" + with pytest.raises(TmuxLookupError) as excinfo: + _client_on(_unreachable(short_tmpdir)).session_exists_strict("cao-demo") + + message = str(excinfo.value) + assert "cao-demo" in message + assert "not a directory" in message.lower() + + def test_a_live_session_is_never_reported_gone_when_the_lookup_fails(self, tmux_socket): + """The end-to-end shape of the bug, on one real session. + + The session is REALLY running the whole time. Asked over a working socket + the check says True; asked over a broken one it must RAISE rather than say + False — because a False here is what authorises dismantling it. + """ + _start_session(tmux_socket, "cao-still-running") + assert _client_on(tmux_socket).session_exists_strict("cao-still-running") is True + + # Same session, unreachable socket path. + with pytest.raises(TmuxLookupError): + _client_on(_unreachable(tmux_socket.parent)).session_exists_strict("cao-still-running") + + # Still there — nothing about the failed lookup touched it. + assert _client_on(tmux_socket).session_exists_strict("cao-still-running") is True + + +class TestUnlinkedSocketUnderALiveServer: + """A missing socket path is NOT by itself proof that the session is gone. + + A tmux server keeps serving its socket after the PATH has been unlinked — the + everyday cause is a tmp-cleaner or systemd-tmpfiles sweeping /tmp on a + long-lived host, and the server recovers on ``kill -USR1 ``. tmux then + fails with ``error connecting to (No such file or directory)``, which is + BYTE-IDENTICAL to the message for a socket that never existed. Reading that + message as a confirmed absence let teardown delete the registry rows and + dismantle the FIFO/status state of a session whose agents were still running + (#498, review finding F1). + + The two tests below are the discriminating pair: same stderr, opposite + verdicts, decided by whether a server is actually bound to the path. + """ + + def test_unlinked_socket_with_a_live_server_raises(self, tmux_socket): + _start_session(tmux_socket, "cao-unlinked") + client = _client_on(tmux_socket) + assert client.session_exists_strict("cao-unlinked") is True + server_pid = _server_pid(tmux_socket) + + os.unlink(tmux_socket) # exactly what a tmp-cleaner does + try: + assert not os.path.exists(tmux_socket) + assert _process_is_alive(server_pid), "server died — test would prove nothing" + + with pytest.raises(TmuxLookupError, match="could not determine whether"): + client.session_exists_strict("cao-unlinked") + + # And the session really was alive throughout: SIGUSR1 makes the + # server recreate the socket, and it is simply there again. + os.kill(server_pid, signal.SIGUSR1) + assert _wait_until(lambda: os.path.exists(tmux_socket)) + assert client.session_exists_strict("cao-unlinked") is True + finally: + # Never leave a server behind: the fixture's kill-server cannot reach + # one whose socket path is gone. + with contextlib.suppress(ProcessLookupError): + os.kill(server_pid, signal.SIGKILL) + + def test_unlinked_socket_with_a_dead_server_is_a_confirmed_absence(self, tmux_socket): + """The other half: rows for a dead server must stay teardownable. + + Same ENOENT stderr as the test above. If this returned an error instead of + False, ``delete_session`` would raise on every retry and registry rows + left by a server that is long gone could never be reconciled — which is + why the fix disambiguates rather than just failing closed on the message. + """ + _start_session(tmux_socket, "cao-doomed") + client = _client_on(tmux_socket) + server_pid = _server_pid(tmux_socket) + + os.unlink(tmux_socket) + os.kill(server_pid, signal.SIGKILL) + assert _wait_until(lambda: not _process_is_alive(server_pid)) + + assert client.session_exists_strict("cao-doomed") is False + + def test_liveness_falls_back_to_the_process_table(self, tmux_socket, monkeypatch): + """Last resort: no socket table at all, so ``ps`` must still say ALIVE. + + Pins the bottom tier for real (not with a stubbed probe): this server was + started with an explicit ``-S``, so the socket path is in its argv. That + is exactly the tier's blind spot too — a server started WITHOUT ``-S`` + (every real CAO server) does not name its socket, which is why this tier + can only ever answer ALIVE or UNKNOWN here, never ABSENT. + """ + _hide_kernel_socket_table(monkeypatch) + _hide_lsof(monkeypatch) + _start_session(tmux_socket, "cao-fallback") + client = _client_on(tmux_socket) + server_pid = _server_pid(tmux_socket) + + os.unlink(tmux_socket) + try: + with pytest.raises(TmuxLookupError, match="still bound to it"): + client.session_exists_strict("cao-fallback") + finally: + with contextlib.suppress(ProcessLookupError): + os.kill(server_pid, signal.SIGKILL) + + def test_undeterminable_liveness_fails_closed( + self, tmux_socket, unrelated_tmux_server, monkeypatch + ): + """No socket table AND no usable process table: raise, even for a socket + that never existed. + + Detection being unavailable is not evidence of absence. This is the one + place the fix deliberately gives up reconciling a possibly-dead server in + exchange for never dismantling a possibly-live one — and after fixup 6 it + takes THREE mechanisms to be unavailable at once, where fixup 5 reached it + on every non-Linux host. + """ + _hide_kernel_socket_table(monkeypatch) + _hide_lsof(monkeypatch) + monkeypatch.setattr(tmux_module, "_tmux_process_command_lines", lambda: None) + assert not os.path.exists(tmux_socket) + + with pytest.raises(TmuxLookupError, match="cannot be determined on this host"): + _client_on(tmux_socket).session_exists_strict("cao-anything") + + +class TestPortableServerDetection: + """Server detection must reach a VERDICT on macOS/BSD too, not just Linux. + + Fixup 5 detected "is a tmux server still bound to this unlinked path?" only + through ``/proc/net/unix``. That file does not exist off Linux, so the tier + answered "cannot tell" for every socket, and the (correct) fail-closed rule + then made ``session_exists_strict`` raise UNCONDITIONALLY on macOS — + permanently blocking teardown and shipping two red tests there, while CI + (ubuntu-latest on every job) stayed green. + + The fix distinguishes "detection was INCONCLUSIVE" from "this detector is + UNAVAILABLE on this platform" and gives the second case another detector: + ``lsof -U``, which keeps reporting a socket's path after the path has been + unlinked because it reads the process's open descriptor, not the filesystem. + + These tests run on Linux through two seams. Hiding ``_PROC_NET_UNIX`` + exercises the lsof MECHANISM (it is what a Mac has instead of a kernel table); + ``_pretend_to_be_macos`` additionally sets ``_HOST_PLATFORM`` so the ladder + SELECTION is exercised too. Neither is a substitute for running the suite on a + Mac, since only the real platform proves macOS lsof prints what we parse. + """ + + def test_the_detector_ladder_is_platform_selected(self): + """Linux prefers the kernel table; everything else must NOT be left + with only a detector that cannot exist there.""" + linux = tmux_module._socket_table_detectors("linux") + assert linux[0] is tmux_module._bound_unix_socket_paths_via_proc + assert tmux_module._bound_unix_socket_paths_via_lsof in linux + + for platform_name in ("darwin", "freebsd13", "openbsd7"): + ladder = tmux_module._socket_table_detectors(platform_name) + assert tmux_module._bound_unix_socket_paths_via_proc not in ladder + assert ladder == (tmux_module._bound_unix_socket_paths_via_lsof,) + + @requires_lsof + def test_lsof_reports_the_bound_path_even_after_it_is_unlinked(self, tmux_socket): + """The mechanism the mac path rests on, measured against real tmux. + + Same property ``/proc/net/unix`` has: bound while alive, STILL bound once + the path is unlinked, gone once the server dies. + """ + _start_session(tmux_socket, "cao-lsof") + server_pid = _server_pid(tmux_socket) + try: + assert str(tmux_socket) in tmux_module._bound_unix_socket_paths_via_lsof() + + os.unlink(tmux_socket) + assert str(tmux_socket) in tmux_module._bound_unix_socket_paths_via_lsof() + + os.kill(server_pid, signal.SIGKILL) + assert _wait_until(lambda: not _process_is_alive(server_pid)) + assert _wait_until( + lambda: str(tmux_socket) not in tmux_module._bound_unix_socket_paths_via_lsof() + ) + finally: + with contextlib.suppress(ProcessLookupError): + os.kill(server_pid, signal.SIGKILL) + + @requires_lsof + def test_no_server_at_all_is_confirmed_without_the_kernel_table( + self, tmux_socket, unrelated_tmux_server, monkeypatch + ): + """STATE 1 on the mac path: nothing ever listened → False, teardownable. + + This is the regression, reproduced on Linux. With the kernel table hidden, + fixup 5 fell through to the process table, saw an unrelated tmux server it + could not attribute to this path, answered UNKNOWN and RAISED — so on + macOS no absence could ever be confirmed and teardown could never proceed. + """ + _hide_kernel_socket_table(monkeypatch) + assert not os.path.exists(tmux_socket) + + assert _client_on(tmux_socket).session_exists_strict("cao-anything") is False + + @requires_lsof + def test_dead_server_behind_an_unlinked_socket_is_confirmed_without_the_kernel_table( + self, tmux_socket, unrelated_tmux_server, monkeypatch + ): + """STATE 2 on the mac path: server dead, socket unlinked → False. + + The other test fixup 5 ships red on macOS. Rows left by a server that is + genuinely gone have to stay reconcilable, or ``delete_session`` raises on + every retry forever. + """ + _hide_kernel_socket_table(monkeypatch) + _start_session(tmux_socket, "cao-doomed") + client = _client_on(tmux_socket) + server_pid = _server_pid(tmux_socket) + + os.unlink(tmux_socket) + os.kill(server_pid, signal.SIGKILL) + assert _wait_until(lambda: not _process_is_alive(server_pid)) + + assert client.session_exists_strict("cao-doomed") is False + + @requires_lsof + def test_the_macos_ladder_reaches_a_verdict( + self, tmux_socket, unrelated_tmux_server, monkeypatch + ): + """The same absence, decided by the ladder the DARWIN branch selects. + + The two tests above hide the kernel table, which proves the lsof detector + works but still reaches it as Linux's tier 2. This one also flips + ``_HOST_PLATFORM``, so the verdict has to come from the non-Linux ladder — + the one fixup 5 left with a single detector that cannot exist there. + """ + _pretend_to_be_macos(monkeypatch) + assert not os.path.exists(tmux_socket) + + assert _client_on(tmux_socket).session_exists_strict("cao-anything") is False + + @requires_lsof + def test_live_server_behind_an_unlinked_socket_still_raises_without_the_kernel_table( + self, tmux_socket, monkeypatch + ): + """STATE 3 on the mac path: the property fixup 5 won, kept. + + Making states 1 and 2 answerable off Linux must not re-open the fail-open: + a server still serving an unlinked socket has to raise, not return False. + """ + _hide_kernel_socket_table(monkeypatch) + _start_session(tmux_socket, "cao-unlinked-mac") + client = _client_on(tmux_socket) + server_pid = _server_pid(tmux_socket) + + os.unlink(tmux_socket) + try: + assert _process_is_alive(server_pid), "server died — test would prove nothing" + + with pytest.raises(TmuxLookupError, match="still bound to it"): + client.session_exists_strict("cao-unlinked-mac") + finally: + with contextlib.suppress(ProcessLookupError): + os.kill(server_pid, signal.SIGKILL) + + def test_lsof_is_bounded_in_time(self, monkeypatch): + """Teardown must not hang behind lsof on a busy host or a hung mount.""" + recorded = {} + + def fake_run(argv, **kwargs): + recorded.update(argv=argv, kwargs=kwargs) + raise subprocess.TimeoutExpired(argv, kwargs["timeout"]) + + monkeypatch.setattr(tmux_module.subprocess, "run", fake_run) + + # A timeout is "this detector cannot tell", never "nothing is bound". + assert tmux_module._bound_unix_socket_paths_via_lsof() is None + assert recorded["kwargs"]["timeout"] == tmux_module._LSOF_TIMEOUT_SECONDS + assert 0 < tmux_module._LSOF_TIMEOUT_SECONDS <= 10 + # "-b" must not creep in: it suppresses the socket rows on Linux lsof. + assert "-b" not in recorded["argv"] + + def test_a_missing_lsof_is_not_an_absence(self, monkeypatch): + _hide_lsof(monkeypatch) + + assert tmux_module._bound_unix_socket_paths_via_lsof() is None + + def test_an_empty_lsof_listing_is_not_an_absence(self, monkeypatch): + """An empty parse means lsof failed, not that no socket is bound. + + Returning an empty set here would make every missing socket path read as + a confirmed absence — the exact fail-open shape of the original bug. + """ + monkeypatch.setattr( + tmux_module.subprocess, + "run", + lambda argv, **kwargs: subprocess.CompletedProcess(argv, 0, "", ""), + ) + + assert tmux_module._bound_unix_socket_paths_via_lsof() is None + + def test_lsof_name_decoration_and_non_paths_are_handled(self, monkeypatch): + """Linux lsof appends " type=STREAM"; macOS lsof does not. Both parse. + + Unnamed sockets render as a bare "type=STREAM" name and must not become + a bound path. Field-per-line output is also what keeps a socket path + containing a SPACE intact, which column splitting would not. + """ + stdout = "p123\nn/tmp/with space.sock type=STREAM\nntype=STREAM\nn/tmp/plain.sock\nf7\n" + monkeypatch.setattr( + tmux_module.subprocess, + "run", + lambda argv, **kwargs: subprocess.CompletedProcess(argv, 1, stdout, "warning"), + ) + + paths = tmux_module._bound_unix_socket_paths_via_lsof() + + # Nonzero exit with a usable listing still counts: lsof exits 1 merely + # for warning about processes it could not fully inspect. + assert "/tmp/with space.sock" in paths + assert "/tmp/plain.sock" in paths + assert "type=STREAM" not in paths + + +class TestOnlyTmuxLookupErrorEscapes: + """The docstring advertises exactly one exception type; keep it true. + + Both call sites (``kill_session``'s verify poll and ``delete_session``'s + confirm step) are written against ``TmuxLookupError``, so anything else + leaking out relies on their broader ``except Exception`` guards to stay + fail-closed by accident (review finding F7). + """ + + def test_a_failing_tmux_invocation_is_translated(self, tmux_socket): + client = _client_on(tmux_socket) + + with mock.patch.object(client.server, "cmd", side_effect=OSError("tmux binary gone")): + with pytest.raises(TmuxLookupError, match="could not determine whether"): + client.session_exists_strict("cao-demo") + + def test_the_original_cause_is_preserved(self, tmux_socket): + client = _client_on(tmux_socket) + cause = OSError("tmux binary gone") + + with mock.patch.object(client.server, "cmd", side_effect=cause): + with pytest.raises(TmuxLookupError) as excinfo: + client.session_exists_strict("cao-demo") + + assert excinfo.value.__cause__ is cause + assert "tmux binary gone" in str(excinfo.value) + + +class TestLenientCheckIsUnchanged: + """``session_exists`` must keep its fail-OPEN behavior. + + Dozens of best-effort callers (status, UI, duplicate-name guards) rely on it + collapsing errors to False, so #498 deliberately hardened only the strict + check. Pinned here so a later "consistency" cleanup has to be a deliberate + choice rather than an accident. + """ + + def test_lenient_check_swallows_an_unanswerable_lookup(self, short_tmpdir): + assert _client_on(_unreachable(short_tmpdir)).session_exists("cao-demo") is False + + def test_lenient_check_still_finds_a_live_session(self, tmux_socket): + _start_session(tmux_socket, "cao-alive") + + assert _client_on(tmux_socket).session_exists("cao-alive") is True diff --git a/test/services/test_plugin_event_emission.py b/test/services/test_plugin_event_emission.py index 6ac8f0631..fa3ad59cd 100644 --- a/test/services/test_plugin_event_emission.py +++ b/test/services/test_plugin_event_emission.py @@ -88,49 +88,212 @@ async def test_create_session_does_not_dispatch_on_failure(self, mock_create_ter registry.dispatch.assert_not_awaited() - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.db_delete_terminal") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_dispatches_post_kill_session_event_after_cleanup( - self, mock_tmux, mock_list_terminals, mock_delete_terminal + self, + mock_tmux, + mock_list_terminals, + mock_capture, + mock_dismantle, + mock_db_delete_terminal, + mock_sweep, ): - """Session kill should emit after per-terminal cleanup and the tmux kill succeed.""" + """Session kill should emit after per-terminal cleanup and the tmux kill succeed. + + Ordering is what this pins, against the real three-phase teardown (#498): + the read-only snapshot, then the confirmed tmux kill, then the destructive + per-terminal phases, and only then — after the lifecycle lock is released — + both events. ``delete_terminal_row`` is deliberately NOT mocked, since + session teardown now calls it with ``registry=None`` and emits the + per-terminal event itself; stubbing it would hide that handoff. Its DB + write is stubbed at ``db_delete_terminal`` instead. + """ registry = _registry_mock() call_order: list[str] = [] - async def record_dispatch(*_args): - call_order.append("dispatch") + async def record_dispatch(event_type, _event): + call_order.append(f"dispatch:{event_type}") - mock_tmux.return_value.session_exists.return_value = True - mock_tmux.return_value.kill_session.side_effect = lambda *_: call_order.append( - "kill_session" + mock_tmux.return_value.session_exists_strict.return_value = True + mock_tmux.return_value.kill_session.side_effect = lambda *_: ( + call_order.append("kill_session") or True ) - # One contained terminal so we can assert it is torn down before the - # session is killed and the event is emitted. + # One contained terminal so we can assert its teardown phases straddle + # the session kill in the right order. mock_list_terminals.return_value = [{"id": "abcd1234"}] - mock_delete_terminal.side_effect = lambda *_args, **_kwargs: call_order.append( - "delete_terminal" + mock_capture.side_effect = lambda tid: ( + call_order.append("capture_snapshot") + or {"tmux_session": "cao-demo", "tmux_window": "developer-abcd", "id": tid} ) + mock_dismantle.side_effect = lambda *_args, **_kwargs: call_order.append("dismantle") + mock_db_delete_terminal.side_effect = lambda *_: call_order.append("db_delete") or True registry.dispatch.side_effect = record_dispatch result = delete_session("cao-demo", registry=registry) assert result == {"deleted": ["cao-demo"], "errors": []} - assert call_order == ["delete_terminal", "kill_session", "dispatch"] - # Each contained terminal is cleaned up via the event-driven teardown path. - mock_delete_terminal.assert_called_once_with("abcd1234", registry=registry) + # Nothing destructive happens before the kill is confirmed, and both + # events fire only after the rows are gone. + assert call_order == [ + "capture_snapshot", + "kill_session", + "dismantle", + "db_delete", + "dispatch:post_kill_terminal", + "dispatch:post_kill_session", + ] + # The windows died with the session, so the per-terminal teardown must not + # try to kill them again. + mock_dismantle.assert_called_once() + assert mock_dismantle.call_args.kwargs["kill_window"] is False event_type, event = registry.dispatch.await_args.args assert event_type == "post_kill_session" assert isinstance(event, PostKillSessionEvent) assert event.session_id == "cao-demo" assert event.session_name == "cao-demo" + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.db_delete_terminal") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") + @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") + @patch("cli_agent_orchestrator.services.session_service.get_backend") + def test_delete_session_emits_post_kill_terminal_for_each_contained_terminal( + self, mock_tmux, mock_list_terminals, mock_capture, _dismantle, _db_delete, _sweep + ): + """Session teardown must emit one post_kill_terminal per terminal. + + Session teardown no longer calls ``delete_terminal``; it drives the three + phases itself. That made it possible to drop the per-terminal event on this + path while the single-terminal path kept it, so pin the payloads here too + (#498). + """ + registry = _registry_mock() + dispatched: list[tuple] = [] + + async def record_dispatch(event_type, event): + dispatched.append((event_type, event)) + + mock_tmux.return_value.session_exists_strict.return_value = True + mock_tmux.return_value.kill_session.return_value = True + mock_list_terminals.return_value = [{"id": "aaaa1111"}, {"id": "bbbb2222"}] + mock_capture.side_effect = lambda tid: { + "tmux_session": "cao-demo", + "tmux_window": f"developer-{tid[:4]}", + "agent_profile": "developer", + } + registry.dispatch.side_effect = record_dispatch + + delete_session("cao-demo", registry=registry) + + terminal_events = [e for kind, e in dispatched if kind == "post_kill_terminal"] + assert [e.terminal_id for e in terminal_events] == ["aaaa1111", "bbbb2222"] + assert all(isinstance(e, PostKillTerminalEvent) for e in terminal_events) + assert all(e.session_id == "cao-demo" for e in terminal_events) + assert all(e.agent_name == "developer" for e in terminal_events) + assert [kind for kind, _ in dispatched][-1] == "post_kill_session" + + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.db_delete_terminal") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") + @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") + @patch("cli_agent_orchestrator.services.session_service.get_backend") + def test_one_unusable_terminal_does_not_abort_the_completed_teardown( + self, mock_tmux, mock_list_terminals, mock_capture, _dismantle, _db_delete, _sweep + ): + """A terminal whose event cannot be BUILT must not fail the delete. + + By the time these events are emitted the kill is confirmed, the rows are + gone and the runtime is dismantled — all of it durable. Letting one + terminal's payload construction propagate would turn that finished + teardown into an API 500 and cost the *other* terminals their event, + reporting failure for work that entirely succeeded. + + The failure is injected by patching the event class in + ``session_service``'s namespace only, because it cannot be provoked + through metadata: ``delete_terminal_row`` builds the identical payload + first (under its own ``except``), so any metadata that breaks the + after-lock construction breaks that one and the terminal never reaches + this loop. That makes the hazard unreachable today — this pins the + isolation so it stays that way if either payload changes. + """ + registry = _registry_mock() + dispatched: list[tuple] = [] + + async def record_dispatch(event_type, event): + dispatched.append((event_type, event)) + + mock_tmux.return_value.session_exists_strict.return_value = True + mock_tmux.return_value.kill_session.return_value = True + mock_list_terminals.return_value = [{"id": "aaaa1111"}, {"id": "bbbb2222"}] + mock_capture.side_effect = lambda tid: { + "tmux_session": "cao-demo", + "agent_profile": "developer", + "id": tid, + } + registry.dispatch.side_effect = record_dispatch + + def explode_for_the_first_terminal(*, terminal_id, **kwargs): + if terminal_id == "aaaa1111": + raise ValueError("unbuildable payload") + return PostKillTerminalEvent(terminal_id=terminal_id, **kwargs) + + with patch( + "cli_agent_orchestrator.services.session_service.PostKillTerminalEvent", + side_effect=explode_for_the_first_terminal, + ): + result = delete_session("cao-demo", registry=registry) + + assert result == {"deleted": ["cao-demo"], "errors": []} + assert [kind for kind, _ in dispatched] == [ + "post_kill_terminal", + "post_kill_session", + ] + assert dispatched[0][1].terminal_id == "bbbb2222" + + @patch("cli_agent_orchestrator.services.terminal_service.db_delete_terminal") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") + @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") + @patch("cli_agent_orchestrator.services.session_service.get_backend") + def test_delete_session_does_not_dispatch_when_the_kill_is_unconfirmed( + self, mock_tmux, mock_list_terminals, mock_capture, mock_dismantle, mock_db_delete_terminal + ): + """A session that survives its kill must emit NOTHING and dismantle nothing. + + The confirm-then-dismantle guarantee, seen from the plugin side: a plugin + told ``post_kill_session`` treats the session as gone, so emitting it for a + session still running would propagate the divergence outward (#498). + """ + registry = _registry_mock() + mock_tmux.return_value.session_exists_strict.return_value = True + # kill_session reports failure and the session is still there afterwards. + mock_tmux.return_value.kill_session.return_value = False + mock_list_terminals.return_value = [{"id": "abcd1234"}] + mock_capture.return_value = { + "tmux_session": "cao-demo", + "tmux_window": "developer-abcd", + } + + with pytest.raises(RuntimeError, match="still exists after kill_session"): + delete_session("cao-demo", registry=registry) + + registry.dispatch.assert_not_awaited() + mock_dismantle.assert_not_called() + mock_db_delete_terminal.assert_not_called() + @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_does_not_dispatch_on_failure(self, mock_tmux, mock_list_terminals): """Session deletion failures must not emit events.""" registry = _registry_mock() - mock_tmux.return_value.session_exists.return_value = True + mock_tmux.return_value.session_exists_strict.return_value = True mock_list_terminals.side_effect = RuntimeError("db error") with pytest.raises(RuntimeError, match="db error"): diff --git a/test/services/test_session_service.py b/test/services/test_session_service.py index 90327f44b..c9fcf48b2 100644 --- a/test/services/test_session_service.py +++ b/test/services/test_session_service.py @@ -643,24 +643,42 @@ def test_get_session_error(self, mock_get_backend): class TestDeleteSession: - """Tests for delete_session function.""" - - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + """Tests for delete_session function. + + delete_session (#498) runs its whole critical section under the + per-session-name lifecycle lock, captures each terminal's scrollback + (read-only) first, checks session liveness with a STRICT existence check + (a lookup error is not "gone"), disambiguates a False kill via a strict + follow-up (a session gone-before-kill is success, not failure), and only + THEN dismantles the per-terminal runtime and deletes registry rows — scoped + BY ID to the incarnation it started tearing down. Faithful-fake, real-DB + reconciliation and concurrency tests live in test_session_teardown_atomic.py. + """ + + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal_row") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_success( self, mock_get_backend, mock_list_terminals, - mock_delete_terminal, + mock_capture, + mock_dismantle, + mock_delete_row, + mock_delete_terminals_by_ids, ): """Test deleting session successfully. - delete_session delegates per-terminal teardown (FIFO reader, status - buffer, provider, DB) to terminal_service.delete_terminal, then kills - the backend session and returns the Dict result shape. + delete_session captures each terminal's snapshot, kills the backend + session through the verified backend primitive, and only after that + confirmation dismantles the runtime (FIFO reader, status buffer, + provider) and deletes the rows + sweeps by id. """ - mock_get_backend.return_value.session_exists.return_value = True + mock_get_backend.return_value.session_exists_strict.return_value = True + mock_get_backend.return_value.kill_session.return_value = True mock_list_terminals.return_value = [ {"id": "terminal1"}, {"id": "terminal2"}, @@ -670,71 +688,121 @@ def test_delete_session_success( assert result == {"deleted": ["cao-test"], "errors": []} mock_get_backend.return_value.kill_session.assert_called_once_with("cao-test") - # Each terminal is torn down via the event-driven delete_terminal path. - assert mock_delete_terminal.call_count == 2 - mock_delete_terminal.assert_any_call("terminal1", registry=ANY) - mock_delete_terminal.assert_any_call("terminal2", registry=ANY) - - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + # Registry rows are reconciled after kill_session confirms the session + # is gone — scoped to the incarnation's ids, not the whole session name. + mock_delete_terminals_by_ids.assert_called_once_with(["terminal1", "terminal2"]) + # Snapshots are captured while the panes still exist ... + assert mock_capture.call_count == 2 + mock_capture.assert_any_call("terminal1") + mock_capture.assert_any_call("terminal2") + # ... and the runtime + row are only touched after the kill was confirmed. + assert mock_dismantle.call_count == 2 + mock_dismantle.assert_any_call("terminal1", ANY, kill_window=False) + mock_dismantle.assert_any_call("terminal2", ANY, kill_window=False) + assert mock_delete_row.call_count == 2 + mock_delete_row.assert_any_call("terminal1", ANY, registry=ANY) + mock_delete_row.assert_any_call("terminal2", ANY, registry=ANY) + + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal_row") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_when_backend_session_already_gone( - self, mock_get_backend, mock_list_terminals, mock_delete_terminal + self, + mock_get_backend, + mock_list_terminals, + mock_capture, + mock_dismantle, + mock_delete_row, + mock_delete_terminals_by_ids, ): """Backend session already gone — delete_session should not raise and not - call kill_session, but still tear down each terminal via delete_terminal.""" - mock_get_backend.return_value.session_exists.return_value = False + call kill_session, but still tear down each terminal and reconcile the + registry.""" + mock_get_backend.return_value.session_exists_strict.return_value = False mock_list_terminals.return_value = [{"id": "terminal1"}] result = delete_session("cao-test") assert result == {"deleted": ["cao-test"], "errors": []} mock_get_backend.return_value.kill_session.assert_not_called() - mock_delete_terminal.assert_called_once_with("terminal1", registry=ANY) - - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + mock_capture.assert_called_once_with("terminal1") + mock_dismantle.assert_called_once_with("terminal1", ANY, kill_window=False) + mock_delete_row.assert_called_once_with("terminal1", ANY, registry=ANY) + mock_delete_terminals_by_ids.assert_called_once_with(["terminal1"]) + + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal_row") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_no_terminals( - self, mock_get_backend, mock_list_terminals, mock_delete_terminal + self, + mock_get_backend, + mock_list_terminals, + mock_capture, + mock_dismantle, + mock_delete_row, + mock_delete_terminals_by_ids, ): """Test deleting session with no terminals.""" - mock_get_backend.return_value.session_exists.return_value = True + mock_get_backend.return_value.session_exists_strict.return_value = True + mock_get_backend.return_value.kill_session.return_value = True mock_list_terminals.return_value = [] result = delete_session("cao-test") assert result == {"deleted": ["cao-test"], "errors": []} mock_get_backend.return_value.kill_session.assert_called_once_with("cao-test") - mock_delete_terminal.assert_not_called() + mock_capture.assert_not_called() + mock_dismantle.assert_not_called() + mock_delete_row.assert_not_called() + mock_delete_terminals_by_ids.assert_called_once_with([]) @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_error(self, mock_get_backend, mock_list_terminals): """Test deleting session with error.""" - mock_get_backend.return_value.session_exists.return_value = True + mock_get_backend.return_value.session_exists_strict.return_value = True mock_list_terminals.side_effect = Exception("Database error") with pytest.raises(Exception, match="Database error"): delete_session("cao-test") - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal_row") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_continues_when_terminal_cleanup_fails( - self, mock_get_backend, mock_list_terminals, mock_delete_terminal + self, + mock_get_backend, + mock_list_terminals, + mock_capture, + mock_dismantle, + mock_delete_row, + mock_delete_terminals_by_ids, ): - """Test that delete_session continues even when terminal teardown fails for some terminals.""" - mock_get_backend.return_value.session_exists.return_value = True + """delete_session continues when one terminal's snapshot capture fails. + + A failed capture yields no metadata but must not abort the teardown, drop + the terminal from the incarnation, or skip the session kill. + """ + mock_get_backend.return_value.session_exists_strict.return_value = True + mock_get_backend.return_value.kill_session.return_value = True mock_list_terminals.return_value = [ {"id": "terminal1"}, {"id": "terminal2"}, {"id": "terminal3"}, ] - # First terminal teardown fails, others succeed - mock_delete_terminal.side_effect = [ - Exception("Terminal teardown error for terminal1"), + # First terminal's snapshot capture fails, others succeed + mock_capture.side_effect = [ + Exception("Snapshot error for terminal1"), None, # terminal2 succeeds None, # terminal3 succeeds ] @@ -744,19 +812,47 @@ def test_delete_session_continues_when_terminal_cleanup_fails( # Session should still be deleted despite per-terminal teardown failure assert result == {"deleted": ["cao-test"], "errors": []} mock_get_backend.return_value.kill_session.assert_called_once_with("cao-test") - # All three terminal teardowns were attempted - assert mock_delete_terminal.call_count == 3 + # All three captures were attempted ... + assert mock_capture.call_count == 3 + # ... and every terminal is still dismantled and row-deleted: a failed + # capture only costs its metadata (passed as None), never its teardown. + assert mock_dismantle.call_count == 3 + assert mock_delete_row.call_count == 3 + mock_dismantle.assert_any_call("terminal1", None, kill_window=False) + mock_delete_row.assert_any_call("terminal1", None, registry=ANY) + # The by-id sweep still backstops any row a failed delete left behind. + mock_delete_terminals_by_ids.assert_called_once_with( + ["terminal1", "terminal2", "terminal3"] + ) - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal_row") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_reports_deferred_terminal_cleanup( - self, mock_get_backend, mock_list_terminals, mock_delete_terminal + self, + mock_get_backend, + mock_list_terminals, + mock_capture, + mock_dismantle, + mock_delete_row, + mock_delete_terminals_by_ids, ): - """An explicit retryable teardown result must not be reported deleted.""" - mock_get_backend.return_value.session_exists.return_value = True + """An explicit retryable teardown result must not be reported deleted. + + A deferred runtime teardown (Grok has not released its private home yet, + #596) keeps the terminal's registry row: the row is the only retry handle, + so neither the per-terminal delete nor the by-id sweep may drop it, and + the session is reported in ``errors`` rather than ``deleted``. The tmux + session itself is still killed — the deferral is about on-disk provider + state, not the session. + """ + mock_get_backend.return_value.session_exists_strict.return_value = True + mock_get_backend.return_value.kill_session.return_value = True mock_list_terminals.return_value = [{"id": "grok-terminal"}] - mock_delete_terminal.return_value = False + mock_dismantle.return_value = False result = delete_session("cao-grok") @@ -765,15 +861,28 @@ def test_delete_session_reports_deferred_terminal_cleanup( {"terminal_id": "grok-terminal", "error": "cleanup deferred; retry delete_session"} ] mock_get_backend.return_value.kill_session.assert_called_once_with("cao-grok") - - @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal") + # The retry handle survives both row-deletion paths. + mock_delete_row.assert_not_called() + mock_delete_terminals_by_ids.assert_called_once_with([]) + + @patch("cli_agent_orchestrator.services.session_service.delete_terminals_by_ids") + @patch("cli_agent_orchestrator.services.terminal_service.delete_terminal_row") + @patch("cli_agent_orchestrator.services.terminal_service.dismantle_terminal_runtime") + @patch("cli_agent_orchestrator.services.terminal_service.capture_terminal_snapshot") @patch("cli_agent_orchestrator.services.session_service.list_terminals_by_session") @patch("cli_agent_orchestrator.services.session_service.get_backend") def test_delete_session_cleans_up_each_terminal( - self, mock_get_backend, mock_list_terminals, mock_delete_terminal + self, + mock_get_backend, + mock_list_terminals, + mock_capture, + mock_dismantle, + mock_delete_row, + mock_delete_terminals_by_ids, ): - """Test that delete_session tears down every terminal in the session via delete_terminal.""" - mock_get_backend.return_value.session_exists.return_value = True + """Test that delete_session tears down every terminal in the session.""" + mock_get_backend.return_value.session_exists_strict.return_value = True + mock_get_backend.return_value.kill_session.return_value = True mock_list_terminals.return_value = [ {"id": "term-aaa"}, {"id": "term-bbb"}, @@ -784,9 +893,11 @@ def test_delete_session_cleans_up_each_terminal( result = delete_session("cao-multi-terminal") assert result == {"deleted": ["cao-multi-terminal"], "errors": []} - # Verify delete_terminal was called for each terminal with the correct ID - assert mock_delete_terminal.call_count == 4 - mock_delete_terminal.assert_any_call("term-aaa", registry=ANY) - mock_delete_terminal.assert_any_call("term-bbb", registry=ANY) - mock_delete_terminal.assert_any_call("term-ccc", registry=ANY) - mock_delete_terminal.assert_any_call("term-ddd", registry=ANY) + # Verify all three teardown phases ran for each terminal id + assert mock_capture.call_count == 4 + assert mock_dismantle.call_count == 4 + assert mock_delete_row.call_count == 4 + for tid in ("term-aaa", "term-bbb", "term-ccc", "term-ddd"): + mock_capture.assert_any_call(tid) + mock_dismantle.assert_any_call(tid, ANY, kill_window=False) + mock_delete_row.assert_any_call(tid, ANY, registry=ANY) diff --git a/test/services/test_session_teardown_atomic.py b/test/services/test_session_teardown_atomic.py new file mode 100644 index 000000000..c22730c5f --- /dev/null +++ b/test/services/test_session_teardown_atomic.py @@ -0,0 +1,1522 @@ +"""Atomic session-teardown tests (#498). + +These exercise the REAL ``delete_session`` reconciliation logic against: + +* a faithful in-memory tmux backend that models the side effects that actually + cause the two stores to drift — ``kill_window`` dropping the last window (and + thus the whole session), ``kill_session`` racing tmux's own reaping, and + ``kill_session`` failing outright, +* a REAL SQLite registry (``clients.database`` with a per-test engine), so + ``list_terminals_by_session`` / ``delete_terminals_by_ids`` / + ``db_delete_terminal`` run their production SQL, and +* a ``FakeRuntime`` recording the non-row side effects (FIFO reader, status + monitor, provider registration), so a terminal that keeps its row while its + pipeline is gone — a zombie — is observable rather than invisible. + +Mocking ``delete_session`` itself would prove nothing for an ordering bug, so we +drive the true function and assert the invariant the fix requires: after a +SUCCESSFUL return the tmux session is provably gone AND no registry rows survive +for it; a kill that never takes is surfaced as an error, not a false success, +with the session left WHOLE (rows AND runtime); and a re-run reconciles a +half-torn-down session. + +The concurrency tests drive the real functions from real threads and force the +interleaving with barriers/events injected at the exact race windows, rather than +mocking the race away. +""" + +import threading +import time +from typing import Dict, List, Set +from unittest.mock import MagicMock + +import pytest +from sqlalchemy import create_engine +from sqlalchemy.exc import OperationalError +from sqlalchemy.orm import sessionmaker + +from cli_agent_orchestrator.backends.registry import set_backend +from cli_agent_orchestrator.clients import database +from cli_agent_orchestrator.services import ( + session_env, + session_lock, + session_service, + terminal_service, +) + +# Any thread join / lock acquire in these tests must be bounded: a self-deadlock +# or a lock never released has to FAIL the test, not hang the whole run. +DEADLOCK_TIMEOUT = 10.0 + + +def _wait_until_lock_contended(session_name, waiters=2, timeout=DEADLOCK_TIMEOUT): + """Block until ``waiters`` threads are registered on ``session_name``'s lock. + + This is the deterministic way to establish that a second caller is blocked, + and it replaces the only alternative available to a test that cannot see + inside ``lock.acquire()``: "assert it is still blocked because it had not + finished after N seconds". That formulation is a guess about scheduling, and + a busy machine invalidates it — which is exactly how these tests flaked in + the full suite while passing in isolation. + + ``session_lifecycle_lock`` bumps its refcount BEFORE calling + ``lock.acquire()``, so a count of 2 while another thread demonstrably holds + the lock means the second caller has committed to acquiring it and cannot + reach its critical section until the holder releases. Blocked-ness is then a + property of the construction rather than of the clock: a slow machine only + makes this wait longer, it cannot make the observation wrong. + + ``timeout`` is an outer safety bound only, so a thread that never arrives + fails the test instead of hanging the run. + """ + deadline = time.monotonic() + timeout + while True: + with session_lock._registry_guard: + entry = session_lock._session_locks.get(session_name) + registered = 0 if entry is None else entry[1] + if registered >= waiters: + return + if time.monotonic() >= deadline: + raise AssertionError( + f"only {registered} thread(s) registered on the lifecycle lock for " + f"{session_name!r} after {timeout}s, expected {waiters}: the " + "operation under test never reached the lock" + ) + time.sleep(0.005) + + +class FakeTmuxBackend: + """In-memory stand-in for the tmux backend modelling teardown side effects. + + State is ``session_name -> {window_name, ...}``. The behaviours that matter + for the atomicity bug are modelled faithfully: + + * ``kill_window`` removes a window; removing a session's LAST window drops + the whole session — exactly like tmux, so a "was it alive" snapshot taken + before the terminal loop is stale by the time the kill would run. + * ``kill_session`` removes the session AND all its windows (as tmux does), + and mirrors the verified tmux primitive's two production failure shapes: + - ``kill_lag``: ``session.kill()`` returns but tmux has not finished + reaping, so ``kill_session`` polls before returning True. + - ``kill_fails``: the kill is swallowed entirely and the session + survives indefinitely (observation A). + """ + + def __init__(self, kill_lag: int = 0, kill_fails: bool = False) -> None: + self._sessions: Dict[str, Set[str]] = {} + self._kill_lag = kill_lag + self._kill_fails = kill_fails + self._pending_reap: Dict[str, int] = {} + self.kill_session_calls = 0 + self.kill_window_calls = 0 + self._lock = threading.Lock() + + # --- test helpers --- + def add_session(self, session_name: str, windows: Set[str]) -> None: + self._sessions[session_name] = set(windows) + + def windows(self, session_name: str) -> Set[str]: + """The live windows of ``session_name`` (empty set if it is gone). + + Lets a test assert WHICH windows survived a partial teardown, not merely + that the session still exists. + """ + return set(self._sessions.get(session_name, set())) + + # --- backend surface used by delete_session / terminal teardown --- + def session_exists(self, session_name: str) -> bool: + # Resolve a lagged kill: report alive until the lag counter drains. + if session_name in self._pending_reap: + remaining = self._pending_reap[session_name] + if remaining <= 0: + self._pending_reap.pop(session_name, None) + self._sessions.pop(session_name, None) + return False + self._pending_reap[session_name] = remaining - 1 + return True + return session_name in self._sessions + + def session_exists_strict(self, session_name: str) -> bool: + # No transport layer in the in-memory fake, so a strict check can never + # fail to answer: it is identical to the lenient one here. Tests that + # need to model a lookup error subclass this and override the strict + # check to raise (see PostLoopLookupErrorBackend). + return self.session_exists(session_name) + + def kill_session(self, session_name: str) -> bool: + with self._lock: + self.kill_session_calls += 1 + if session_name not in self._sessions: + return False + if self._kill_fails: + # Swallowed failure: session survives, caller (old code) never knew. + return False + if self._kill_lag > 0: + # The tmux primitive now owns verification: it does not report + # success until the lagged reap has actually completed. + self._pending_reap[session_name] = self._kill_lag + for _ in range(self._kill_lag + 1): + if not self.session_exists(session_name): + return True + return False + self._sessions.pop(session_name, None) + return True + + def kill_window(self, session_name: str, window_name: str) -> bool: + with self._lock: + self.kill_window_calls += 1 + windows = self._sessions.get(session_name) + if not windows or window_name not in windows: + return False + windows.discard(window_name) + # tmux drops a session once its last window is killed. + if not windows: + self._sessions.pop(session_name, None) + return True + + # --- surface touched by capture_terminal_snapshot (read-only) --- + def get_history(self, session_name, window_name, **kwargs) -> str: + return f"scrollback for {session_name}:{window_name}" + + def get_pane_working_directory(self, session_name, window_name) -> str: + return "/tmp" + + def stop_pipe_pane(self, session_name, window_name) -> None: + return None + + def supports_event_inbox(self) -> bool: + return False + + # --- surface touched by create_terminal --- + def create_session(self, session_name, window_name, *args, **kwargs) -> None: + with self._lock: + if session_name in self._sessions: + raise RuntimeError(f"duplicate session {session_name}") + self._sessions[session_name] = {window_name} + + def create_window(self, session_name, window_name, *args, **kwargs) -> str: + with self._lock: + self._sessions.setdefault(session_name, set()).add(window_name) + return window_name + + def pipe_pane(self, session_name, window_name, target) -> None: + return None + + def send_special_key(self, session_name, window_name, key) -> None: + return None + + def get_pane_id(self, terminal_id, session_name, window_name) -> str: + return f"%{terminal_id}" + + +class FakeRuntime: + """Records the NON-ROW per-terminal state a teardown dismantles. + + A terminal is "live" when its row, its FIFO reader, its status-monitor + buffers and its provider registration all exist. The zombie bug the fix + removes was a row restored WITHOUT any of the rest, so the tests need to see + those three independently of the DB. + """ + + def __init__(self) -> None: + self.fifo_readers: Set[str] = set() + self.status_buffers: Set[str] = set() + self.providers: Set[str] = set() + self.lock = threading.Lock() + + def register(self, terminal_id: str) -> None: + with self.lock: + self.fifo_readers.add(terminal_id) + self.status_buffers.add(terminal_id) + self.providers.add(terminal_id) + + def is_fully_live(self, terminal_id: str) -> bool: + with self.lock: + return ( + terminal_id in self.fifo_readers + and terminal_id in self.status_buffers + and terminal_id in self.providers + ) + + def is_fully_gone(self, terminal_id: str) -> bool: + with self.lock: + return not ( + terminal_id in self.fifo_readers + or terminal_id in self.status_buffers + or terminal_id in self.providers + ) + + +@pytest.fixture +def runtime(monkeypatch): + """Patch the runtime singletons terminal create/teardown touch onto a recorder. + + Both directions are recorded so a create's registrations and a teardown's + dismantling are visible to the same assertions. + """ + rt = FakeRuntime() + monkeypatch.setattr( + terminal_service.fifo_manager, + "stop_reader", + lambda tid: rt.fifo_readers.discard(tid), + ) + monkeypatch.setattr( + terminal_service.status_monitor, + "clear_terminal", + lambda tid: rt.status_buffers.discard(tid), + ) + monkeypatch.setattr( + terminal_service.provider_manager, + "cleanup_provider", + lambda tid: rt.providers.discard(tid), + ) + # Create-path stubs: real providers would launch a CLI agent. + monkeypatch.setattr( + terminal_service.fifo_manager, + "create_reader", + lambda tid, **kw: rt.fifo_readers.add(tid), + ) + + def _create_provider(provider, terminal_id, *args, **kwargs): + rt.status_buffers.add(terminal_id) + rt.providers.add(terminal_id) + stub = MagicMock() + stub.shell_baseline = None + + async def _init(): + return None + + stub.initialize = _init + return stub + + monkeypatch.setattr(terminal_service.provider_manager, "create_provider", _create_provider) + return rt + + +@pytest.fixture +def real_db(tmp_path, monkeypatch): + """Point ``clients.database`` at a fresh per-test SQLite registry.""" + engine = create_engine( + f"sqlite:///{tmp_path / 'cao.db'}", + connect_args={"check_same_thread": False}, + ) + database.Base.metadata.create_all(bind=engine) + monkeypatch.setattr( + database, + "SessionLocal", + sessionmaker(autocommit=False, autoflush=False, bind=engine), + ) + # terminal_service imports TERMINAL_LOG_DIR at module scope; redirect the + # snapshot writes into tmp_path so tests don't touch the real log dir. + log_dir = tmp_path / "logs" + log_dir.mkdir() + monkeypatch.setattr(terminal_service, "TERMINAL_LOG_DIR", log_dir) + try: + yield engine + finally: + engine.dispose() + + +@pytest.fixture(autouse=True) +def _reset_backend(): + """Ensure the registry backend singleton is restored after each test.""" + yield + set_backend(None) # type: ignore[arg-type] + + +def _seed(backend, session_name, terminals, runtime=None): + """Create session windows + matching DB rows. ``terminals`` is a list of + (terminal_id, window_name).""" + backend.add_session(session_name, {w for _, w in terminals}) + for terminal_id, window_name in terminals: + database.create_terminal( + terminal_id=terminal_id, + tmux_session=session_name, + tmux_window=window_name, + provider="claude_code", + agent_profile="developer", + ) + if runtime is not None: + runtime.register(terminal_id) + + +def test_success_leaves_no_orphan_in_either_store(real_db, runtime): + """Happy path: after delete_session, tmux session gone AND no DB rows.""" + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-happy", [("t1", "w1"), ("t2", "w2")], runtime) + + result = session_service.delete_session("cao-happy") + + assert result == {"deleted": ["cao-happy"], "errors": []} + assert backend.session_exists("cao-happy") is False + assert database.list_terminals_by_session("cao-happy") == [] + # The runtime is dismantled too — no half-state left behind. + assert runtime.is_fully_gone("t1") + assert runtime.is_fully_gone("t2") + + +def test_kill_session_lag_is_confirmed_before_returning_success(real_db, runtime): + """kill_session returns before tmux reaps the session (a real race). + + The tmux backend primitive polls until the session is provably gone, and + delete_session trusts that verified result instead of polling a second time. + The invariant still holds THE MOMENT delete_session returns. + """ + backend = FakeTmuxBackend(kill_lag=3) + set_backend(backend) + _seed(backend, "cao-lag", [("t1", "w1")], runtime) + + result = session_service.delete_session("cao-lag") + + assert result == {"deleted": ["cao-lag"], "errors": []} + # Provably gone at return time — not "eventually". + assert backend.session_exists("cao-lag") is False + assert database.list_terminals_by_session("cao-lag") == [] + assert backend.kill_session_calls == 1 + + +def test_silent_kill_session_failure_is_surfaced_not_swallowed(real_db, runtime): + """kill_session fails silently (observation A) — must raise, not report success. + + Pre-fix code ignored kill_session's return and reported success while the + tmux session lived on, orphaned. The reconciling code trusts the verified + kill result and raises when the session survives. + """ + backend = FakeTmuxBackend(kill_fails=True) + set_backend(backend) + _seed(backend, "cao-broken", [("t1", "w1")], runtime) + + with pytest.raises(RuntimeError, match="still exists after kill_session"): + session_service.delete_session("cao-broken") + + # The tmux session survives (the failure was real) ... + assert backend.session_exists("cao-broken") is True + + +def test_failed_kill_leaves_session_whole_not_a_zombie(real_db, runtime): + """An unconfirmable kill must leave the session FULLY intact (#498). + + This is the zombie bug. The prior revision dismantled every terminal's + runtime (FIFO reader, status buffers, provider) and deleted its row, then on + a failed kill restored ONLY the row from a snapshot — leaving a registry + entry for a terminal with no output pipeline, no status tracking and no + provider: a row that looks live and is not. The fix defers ALL destructive + work past the confirmation point, so a failed kill changes nothing. + """ + backend = FakeTmuxBackend(kill_fails=True) + set_backend(backend) + _seed(backend, "cao-zombie", [("t1", "w1"), ("t2", "w2")], runtime) + + with pytest.raises(RuntimeError, match="still exists after kill_session"): + session_service.delete_session("cao-zombie") + + # Session alive => rows present ... + assert backend.session_exists("cao-zombie") is True + assert {r["id"] for r in database.list_terminals_by_session("cao-zombie")} == {"t1", "t2"} + # ... AND the runtime behind those rows is still there. This is what the + # snapshot/restore approach could not deliver. + assert runtime.is_fully_live("t1") + assert runtime.is_fully_live("t2") + # The windows are untouched too, so the surviving session is still usable. + assert backend.kill_window_calls == 0 + + +def test_failed_kill_preserves_last_active(real_db, runtime): + """The restore path was lossy: ``last_active`` did not survive it (#498, P2). + + Not deleting the row in the first place preserves every column, including the + ones a hand-written reconstruction forgot. + """ + backend = FakeTmuxBackend(kill_fails=True) + set_backend(backend) + _seed(backend, "cao-lossy", [("t1", "w1")], runtime) + before = database.get_terminal_metadata("t1") + assert before is not None and before["last_active"] is not None + + with pytest.raises(RuntimeError): + session_service.delete_session("cao-lossy") + + after = database.get_terminal_metadata("t1") + assert after is not None + assert after["last_active"] == before["last_active"] + assert after["agent_profile"] == before["agent_profile"] + assert after["provider"] == before["provider"] + + +def test_rerun_reconciles_half_torn_down_session(real_db, runtime): + """delete_session is idempotent and re-runnable after a failed teardown. + + First run: kill_session is broken → raises, tmux session survives with its + rows. Second run (kill now works): the liveness check finds the surviving + session, kills it via the verified primitive, and the teardown completes. + Reconciled — no orphan in either store. + """ + backend = FakeTmuxBackend(kill_fails=True) + set_backend(backend) + _seed(backend, "cao-recover", [("t1", "w1")], runtime) + + with pytest.raises(RuntimeError): + session_service.delete_session("cao-recover") + assert backend.session_exists("cao-recover") is True + + # Repair the backend (kill now succeeds) and re-run — must reconcile. + backend._kill_fails = False + + result = session_service.delete_session("cao-recover") + + assert result == {"deleted": ["cao-recover"], "errors": []} + assert backend.session_exists("cao-recover") is False + assert database.list_terminals_by_session("cao-recover") == [] + assert runtime.is_fully_gone("t1") + + +def test_rerun_after_partial_runtime_teardown_is_idempotent(real_db, runtime): + """A re-run over an already-dismantled runtime must still complete. + + Models the residue of a crash midway through phase 5: t1's runtime is + already gone and its row already deleted, t2 is untouched, the tmux session + still stands. Every dismantle step is idempotent, so the re-run reconciles + the remainder rather than raising on the parts already done. + """ + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-partial", [("t1", "w1"), ("t2", "w2")], runtime) + # Simulate the half-done state. + terminal_service.dismantle_terminal_runtime( + "t1", database.get_terminal_metadata("t1"), kill_window=False + ) + database.delete_terminal("t1") + assert runtime.is_fully_gone("t1") + + result = session_service.delete_session("cao-partial") + + assert result == {"deleted": ["cao-partial"], "errors": []} + assert backend.session_exists("cao-partial") is False + assert database.list_terminals_by_session("cao-partial") == [] + assert runtime.is_fully_gone("t2") + + +def test_leftover_row_from_failed_row_delete_is_reconciled(real_db, runtime, monkeypatch): + """A terminal whose row deletion raises is still swept by the by-id sweep. + + ``delete_terminal_row`` raising must not (a) abort the whole teardown, nor + (b) leave a registry row pointing at a session that is now dead. The + post-kill ``delete_terminals_by_ids`` sweep reconciles it. + """ + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-leak", [("t1", "w1"), ("t2", "w2")], runtime) + + real_delete_row = terminal_service.delete_terminal_row + + def _flaky(terminal_id, metadata, registry=None): + if terminal_id == "t1": + raise RuntimeError("boom during t1 row delete") + return real_delete_row(terminal_id, metadata, registry=registry) + + monkeypatch.setattr(terminal_service, "delete_terminal_row", _flaky) + + result = session_service.delete_session("cao-leak") + + assert result == {"deleted": ["cao-leak"], "errors": []} + assert backend.session_exists("cao-leak") is False + # t1's row survived its failed delete; the reconciliation sweep guarantees + # no registry row outlives the dead session. + assert database.list_terminals_by_session("cao-leak") == [] + + +def test_already_dead_session_is_safe_noop(real_db, runtime): + """Deleting an already-dead session (no tmux session, stray rows) is a safe + no-op that still reconciles the registry — no kill, no error.""" + backend = FakeTmuxBackend() + set_backend(backend) + # DB row exists but the tmux session does NOT (died externally). + database.create_terminal( + terminal_id="t1", + tmux_session="cao-ghost", + tmux_window="w1", + provider="claude_code", + agent_profile="developer", + ) + runtime.register("t1") + + result = session_service.delete_session("cao-ghost") + + assert result == {"deleted": ["cao-ghost"], "errors": []} + assert backend.kill_session_calls == 0 + assert database.list_terminals_by_session("cao-ghost") == [] + assert runtime.is_fully_gone("t1") + + +# ── Finding 1: a lookup error during verification is not "gone" ────────────── + + +class PostLoopLookupErrorBackend(FakeTmuxBackend): + """Strict existence check raises on the liveness check. + + Models a transient libtmux/socket error at exactly the moment + ``delete_session`` checks liveness. The lenient ``session_exists`` collapses + to False ("assume gone"); the STRICT check must surface the error so the + teardown does not delete rows for a session it could not confirm dead (#498 + finding 1). + """ + + def session_exists_strict(self, session_name: str) -> bool: + raise OSError("tmux socket error during liveness check") + + +def test_lookup_error_in_liveness_check_does_not_report_false_success(real_db, runtime): + """A lookup error on the liveness check must raise and preserve BOTH the + rows and the runtime — never dismantle a session that may be alive. + + On the pre-fix code the check used the lenient ``session_exists``, which + swallows the error and returns False, so kill_session was skipped and the + rows swept — a false success while the session may live on. + """ + backend = PostLoopLookupErrorBackend() + set_backend(backend) + _seed(backend, "cao-flaky", [("t1", "w1")], runtime) + + with pytest.raises(RuntimeError, match="could not verify tmux session"): + session_service.delete_session("cao-flaky") + + # The session may still be alive, so it must be left entirely alone. + assert {r["id"] for r in database.list_terminals_by_session("cao-flaky")} == {"t1"} + assert runtime.is_fully_live("t1") + + +class VerifyPollLookupErrorClient: + """A TmuxClient-like object whose verification poll hits a lookup error. + + Exercises ``TmuxClient.kill_session``'s REAL verify loop (via + ``session_exists_strict``) against a transient error: the initial lookup + finds the session, ``session.kill()`` is dispatched, and the strict verify + then raises a non-absence error — which must make kill_session return False, + not a false True (#498 finding 1). + """ + + def __init__(self) -> None: + from cli_agent_orchestrator.clients.tmux import TmuxClient + + self._client = TmuxClient.__new__(TmuxClient) # bypass libtmux.Server() + self._client.server = self # we stand in for .server.sessions.get + self.sessions = self + self._kill_dispatched = False + self.killed = MagicMock() + + # server.sessions.get(...) surface + def get(self, session_name=None, **kwargs): + if not self._kill_dispatched: + self._kill_dispatched = True + session = MagicMock() + session.kill = self.killed + return session + # verification poll: transient transport error (NOT absence) + raise OSError("tmux socket error during verify poll") + + def kill_session(self, session_name): + return self._client.kill_session(session_name) + + +def test_kill_session_verify_poll_lookup_error_returns_false(): + """TmuxClient.kill_session must return False when the verify poll can't tell + whether the session is gone — the error must not read as confirmed absence + (#498 finding 1).""" + stub = VerifyPollLookupErrorClient() + + assert stub.kill_session("cao-x") is False + stub.killed.assert_called_once() + + +# ── Finding 4: disappearance between check and kill is success, not 500 ────── + + +class DisappearBeforeKillBackend(FakeTmuxBackend): + """Session is present at the strict liveness check but gone by kill_session. + + Models tmux dropping the session in the window between ``delete_session``'s + liveness check and ``kill_session``'s own lookup. ``kill_session`` then + returns False PER CONTRACT (base.py: False also means "not found"), which + must be treated as success — the target is already gone (#498 finding 4). + """ + + def __init__(self) -> None: + super().__init__() + self._checked_once = False + + def _liveness_check(self, session_name: str) -> bool: + # First liveness call (whichever method the code under test uses) + # reports alive, but drops the session as a side effect — it vanishes in + # the race window. kill_session then sees it already absent and returns + # False per contract; the follow-up strict check confirms the absence, so + # the fix treats it as success. Overriding BOTH check methods keeps the + # test honest against the pre-fix code (which used lenient + # session_exists). + if not self._checked_once: + self._checked_once = True + self._sessions.pop(session_name, None) + self._pending_reap.pop(session_name, None) + return True + return session_name in self._sessions + + def session_exists(self, session_name: str) -> bool: + return self._liveness_check(session_name) + + def session_exists_strict(self, session_name: str) -> bool: + return self._liveness_check(session_name) + + +def test_disappearance_between_check_and_kill_is_success_and_cleans_up( + real_db, runtime, monkeypatch +): + """If the session vanishes between the liveness check and the kill lookup, + delete_session SUCCEEDS and still runs residual-row cleanup (#498 finding 4). + + Pre-fix, any False from kill_session was treated as "still exists" and the + service raised RuntimeError (surfaced as HTTP 500), skipping cleanup — even + though teardown had actually succeeded. The fix's strict follow-up check + sees the confirmed absence and proceeds. + """ + backend = DisappearBeforeKillBackend() + set_backend(backend) + # Two terminals; the second's row delete raises, leaving a residual row that + # only the post-kill sweep can clear — proving cleanup ran and was NOT + # skipped by a spurious failure. + _seed(backend, "cao-vanish", [("t1", "w1"), ("t-leak", "w2")], runtime) + + real_delete_row = terminal_service.delete_terminal_row + + def _flaky(terminal_id, metadata, registry=None): + if terminal_id == "t-leak": + raise RuntimeError("boom during t-leak row delete") + return real_delete_row(terminal_id, metadata, registry=registry) + + monkeypatch.setattr(terminal_service, "delete_terminal_row", _flaky) + + result = session_service.delete_session("cao-vanish") + + assert result == {"deleted": ["cao-vanish"], "errors": []} + # Residual incarnation rows are cleaned up (cleanup not skipped by a + # spurious "still exists" failure on the already-gone kill). + assert database.list_terminals_by_session("cao-vanish") == [] + + +# ── Finding 3: real concurrency — the lifecycle lock, driven by real threads ── +# +# These do NOT mock the race away. Each drives the production functions from +# threads and forces the interleaving with an event tripped inside a backend +# method that only the critical section calls, so the "other" operation is +# guaranteed to be in flight at the exact moment the race window is open. + + +def _run_threads(targets, timeout=DEADLOCK_TIMEOUT): + """Run callables concurrently; return their results/exceptions in order. + + Joins with a timeout so a self-deadlock or a lock never released FAILS the + test instead of hanging the run forever. + """ + results: List = [None] * len(targets) + + def _wrap(i, fn): + def _inner(): + try: + results[i] = ("ok", fn()) + except BaseException as e: # noqa: BLE001 — reported to the test + results[i] = ("raised", e) + + return _inner + + threads = [ + threading.Thread(target=_wrap(i, fn), name=f"race-{i}", daemon=True) + for i, fn in enumerate(targets) + ] + for t in threads: + t.start() + for t in threads: + t.join(timeout) + alive = [t.name for t in threads if t.is_alive()] + assert not alive, f"threads did not finish within {timeout}s (deadlock?): {alive}" + return results + + +class TeardownEntryGateBackend(FakeTmuxBackend): + """Signals when a teardown has entered its critical section, then waits. + + ``session_exists_strict`` is called by ``delete_session`` only AFTER it has + taken the lifecycle lock and enumerated rows. Tripping ``entered`` there and + then blocking on ``release`` parks a teardown with the lock held and its race + window wide open, so the other thread's attempt is guaranteed to overlap. + + ``phases`` records, in order, the backend mutations that a caller can only + reach from INSIDE a lifecycle critical section. That turns "did the create + interleave?" into a question about ordering, answerable exactly, instead of a + question about how much wall clock elapsed without the create finishing. + """ + + def __init__(self) -> None: + super().__init__() + self.entered = threading.Event() + self.release = threading.Event() + self.phases: List[str] = [] + self._phases_lock = threading.Lock() + self._gated = False + + def _record(self, phase: str) -> None: + with self._phases_lock: + self.phases.append(phase) + + def session_exists_strict(self, session_name: str) -> bool: + if not self._gated: + self._gated = True + self._record("teardown-enter") + self.entered.set() + assert self.release.wait(DEADLOCK_TIMEOUT), "gate never released" + return self.session_exists(session_name) + + def kill_session(self, session_name: str) -> bool: + killed = super().kill_session(session_name) + self._record("teardown-kill") + return killed + + def create_session(self, session_name, window_name, *args, **kwargs) -> None: + super().create_session(session_name, window_name, *args, **kwargs) + self._record("create-enter") + + +def _create_in_thread(session_name): + """Run the async create_terminal from a worker thread (its own loop).""" + import asyncio + + return asyncio.run( + terminal_service.create_terminal( + provider="claude_code", + agent_profile="developer", + session_name=session_name, + new_session=True, + ) + ) + + +def test_create_blocked_by_in_flight_teardown_same_name(real_db, runtime): + """Order A — teardown holds the name, a create for it must WAIT (#498 F3). + + The teardown is parked inside its critical section with the lock held and its + race window open; a create for the same name is launched and must not + interleave. When the teardown is released, it completes; only then does the + create build the new incarnation. Both stores end up describing the SAME + thing — the new session, live, with exactly its own row — and the old + incarnation is gone from both. + + Without mutual exclusion this is the interleaving that orphans: the create + puts a live tmux session under the name while the teardown, which already + decided the name was dead, kills it and sweeps. + """ + backend = TeardownEntryGateBackend() + set_backend(backend) + _seed(backend, "cao-race-a", [("t-old", "w-old")], runtime) + + create_started = threading.Event() + create_finished = threading.Event() + + def _teardown(): + return session_service.delete_session("cao-race-a") + + def _create(): + assert backend.entered.wait(DEADLOCK_TIMEOUT), "teardown never entered its section" + create_started.set() + try: + return _create_in_thread("cao-race-a") + finally: + create_finished.set() + + def _referee(): + # Wait for POSITIVE evidence that the create is parked on the lifecycle + # lock the teardown is holding, then release the teardown. The create is + # committed to ``lock.acquire()`` at that point, so it provably cannot + # reach its critical section until the teardown's ``with`` block exits. + try: + assert create_started.wait(DEADLOCK_TIMEOUT), "create thread never started" + _wait_until_lock_contended("cao-race-a") + assert ( + not create_finished.is_set() + ), "create completed while the teardown still held the lifecycle lock" + finally: + # Unconditional: a referee that died holding the gate would strand + # the teardown and report a bogus deadlock instead of the real failure. + backend.release.set() + + results = _run_threads([_teardown, _create, _referee]) + teardown_result, create_result, referee_result = results + + assert teardown_result[0] == "ok", f"teardown failed: {teardown_result[1]!r}" + assert ( + referee_result[0] == "ok" + ), f"create was not blocked by the in-flight teardown: {referee_result[1]!r}" + assert create_result[0] == "ok", f"create failed: {create_result[1]!r}" + + # No interleaving, proven by ORDER: the create's critical section began only + # after the teardown had entered its own and killed the old incarnation. + # Without the lock, the create's ``create_session`` would land between + # ``teardown-enter`` and ``teardown-kill`` — the interleaving that orphans. + assert backend.phases == [ + "teardown-enter", + "teardown-kill", + "create-enter", + ], f"create/teardown critical sections interleaved: {backend.phases}" + + new_id = create_result[1].id + # Stores agree: the new incarnation is live in tmux and is the ONLY row. + assert backend.session_exists("cao-race-a") is True + assert {r["id"] for r in database.list_terminals_by_session("cao-race-a")} == {new_id} + assert runtime.is_fully_live(new_id) + # The old incarnation is gone from both stores. + assert database.get_terminal_metadata("t-old") is None + assert runtime.is_fully_gone("t-old") + + +class CreateEntryGateBackend(FakeTmuxBackend): + """Signals when a CREATE has entered its critical section, then waits. + + ``create_session`` is only reached with the lifecycle lock held, so tripping + ``entered`` there parks a create mid-transition: tmux session made, row not + yet written. That is precisely the window in which a teardown must not be + able to observe the name. + """ + + def __init__(self) -> None: + super().__init__() + self.entered = threading.Event() + self.release = threading.Event() + + def create_session(self, session_name, window_name, *args, **kwargs) -> None: + super().create_session(session_name, window_name, *args, **kwargs) + self.entered.set() + assert self.release.wait(DEADLOCK_TIMEOUT), "gate never released" + + +def test_teardown_blocked_by_in_flight_create_same_name(real_db, runtime): + """Order B — a create holds the name, a teardown for it must WAIT (#498 F3). + + This is the interleaving id-scoping alone cannot fix. The create is parked + with the tmux session already made but its row NOT yet written. If a teardown + could run now it would enumerate ZERO rows, find a live tmux session, kill + it, and return success — destroying the session the create is still building + while the create's row (written afterwards) survives, pointing at nothing. + + With the lock, the teardown blocks until the create is complete, then + enumerates the NEW row and tears the new incarnation down properly: both + stores end empty. + """ + backend = CreateEntryGateBackend() + set_backend(backend) + + teardown_started = threading.Event() + teardown_finished = threading.Event() + + def _create(): + return _create_in_thread("cao-race-b") + + def _teardown(): + assert backend.entered.wait(DEADLOCK_TIMEOUT), "create never entered its section" + teardown_started.set() + try: + return session_service.delete_session("cao-race-b") + finally: + teardown_finished.set() + + def _referee(): + # Same deterministic probe as order A, mirrored: release the create only + # once the teardown is provably parked on the lock the create holds. + try: + assert teardown_started.wait(DEADLOCK_TIMEOUT), "teardown thread never started" + _wait_until_lock_contended("cao-race-b") + assert ( + not teardown_finished.is_set() + ), "teardown completed while the create still held the lifecycle lock" + finally: + # Unconditional, for the same reason as order A. + backend.release.set() + + results = _run_threads([_create, _teardown, _referee]) + create_result, teardown_result, referee_result = results + + assert create_result[0] == "ok", f"create failed: {create_result[1]!r}" + assert ( + referee_result[0] == "ok" + ), f"teardown was not blocked by the in-flight create: {referee_result[1]!r}" + assert teardown_result[0] == "ok", f"teardown failed: {teardown_result[1]!r}" + + new_id = create_result[1].id + # The teardown saw the new row (it could not have run before the row was + # written) and tore the incarnation down completely — neither store retains it. + assert backend.session_exists("cao-race-b") is False + assert database.list_terminals_by_session("cao-race-b") == [] + assert database.get_terminal_metadata(new_id) is None + assert runtime.is_fully_gone(new_id) + + +class OverlapDetectingBackend(FakeTmuxBackend): + """Detects two teardowns of the same name inside their critical sections. + + Every teardown passes through ``session_exists_strict`` while holding the + lifecycle lock. Marking the name busy there and unmarking it on the way out + records whether any two critical sections for the SAME name were ever + concurrent. Under mutual exclusion this must never happen. + + The FIRST entrant holds its section open until the other teardown is provably + registered on the lifecycle lock — a positive signal that it has arrived and + is committed to acquiring, rather than a guess that it probably has by now — + and then dwells briefly so that an implementation WITHOUT mutual exclusion, + which would keep walking straight into this method, is actually seen. + + That dwell is a sensitivity aid ONLY, and is deliberately not load-bearing: + making it too short can merely cause the detector to MISS a broken + implementation, never to fail a correct one. The proof that the two teardowns + serialized does not rest on it — see ``enumerated`` in the test. + """ + + SENSITIVITY_DWELL = 0.05 + + def __init__(self) -> None: + super().__init__() + self._busy: Set[str] = set() + self._busy_lock = threading.Lock() + self.overlaps: List[str] = [] + self._held_open = False + + def session_exists_strict(self, session_name: str) -> bool: + with self._busy_lock: + if session_name in self._busy: + self.overlaps.append(session_name) + self._busy.add(session_name) + hold_open, self._held_open = not self._held_open, True + try: + if hold_open: + _wait_until_lock_contended(session_name) + time.sleep(self.SENSITIVITY_DWELL) + return self.session_exists(session_name) + finally: + with self._busy_lock: + self._busy.discard(session_name) + + +def test_two_concurrent_teardowns_same_name_do_not_overlap(real_db, runtime, monkeypatch): + """Two teardowns of the same name must SERIALIZE, not interleave (#498 F3). + + Without the lock both enumerate the same rows, both act on the same live + session, and both sweep — the second racing the first's kill and row + deletion. The overlap detector asserts the critical sections never coexist: + the second waits, then finds the session already gone and the rows already + swept and completes as a no-op. + + Both calls must succeed — tearing down an already-dead session is not an + error — and neither store may retain anything. + """ + backend = OverlapDetectingBackend() + set_backend(backend) + _seed(backend, "cao-double", [("t1", "w1"), ("t2", "w2")], runtime) + + # What each teardown enumerated at the TOP of its critical section. This is + # the timing-free proof that the two serialized: row enumeration and row + # deletion both live inside the critical section, so if the sections cannot + # coexist then whichever teardown runs second is guaranteed to find the + # registry already swept. Seeing the same rows twice means they overlapped. + enumerated: List[int] = [] + enumerated_lock = threading.Lock() + real_list_terminals = session_service.list_terminals_by_session + + def _recording_list_terminals(session_name): + rows = real_list_terminals(session_name) + if session_name == "cao-double": + with enumerated_lock: + enumerated.append(len(rows)) + return rows + + monkeypatch.setattr(session_service, "list_terminals_by_session", _recording_list_terminals) + + start = threading.Barrier(2, timeout=DEADLOCK_TIMEOUT) + + def _teardown(): + start.wait() + return session_service.delete_session("cao-double") + + results = _run_threads([_teardown, _teardown]) + + for status, value in results: + assert status == "ok", f"a teardown raised: {value!r}" + assert value == {"deleted": ["cao-double"], "errors": []} + assert backend.overlaps == [], ( + "two teardowns of the same session name were inside their critical " + f"sections simultaneously: {backend.overlaps}" + ) + # The winner saw both rows, the loser saw a registry already swept. Only + # strict serialization produces this; an interleaving has the loser + # enumerating rows the winner has not deleted yet. + assert sorted(enumerated) == [0, 2], ( + "the two teardowns did not serialize — rows each enumerated inside its " + f"critical section: {enumerated}" + ) + # Exactly one kill reached the backend: the loser saw the session already gone. + assert backend.kill_session_calls == 1 + assert backend.session_exists("cao-double") is False + assert database.list_terminals_by_session("cao-double") == [] + assert runtime.is_fully_gone("t1") + assert runtime.is_fully_gone("t2") + + +def test_teardowns_of_different_names_run_concurrently(real_db, runtime): + """The lock is per NAME, not global: different names must OVERLAP (#498). + + Each teardown signals on entry to its critical section and then waits for + the OTHER to signal before proceeding. That is only satisfiable if both hold + their locks at the same time. A global lock would deadlock here — which the + join timeout turns into a failure rather than a hang. + """ + + class RendezvousBackend(FakeTmuxBackend): + def __init__(self) -> None: + super().__init__() + self.arrived = threading.Barrier(2, timeout=DEADLOCK_TIMEOUT) + self._seen: Set[str] = set() + + def session_exists_strict(self, session_name: str) -> bool: + if session_name not in self._seen: + self._seen.add(session_name) + # Both teardowns must be inside their critical sections at once. + self.arrived.wait() + return self.session_exists(session_name) + + backend = RendezvousBackend() + set_backend(backend) + _seed(backend, "cao-a", [("ta", "wa")], runtime) + _seed(backend, "cao-b", [("tb", "wb")], runtime) + + results = _run_threads( + [ + lambda: session_service.delete_session("cao-a"), + lambda: session_service.delete_session("cao-b"), + ] + ) + + for status, value in results: + assert status == "ok", f"a teardown raised: {value!r}" + assert backend.session_exists("cao-a") is False + assert backend.session_exists("cao-b") is False + assert database.list_terminals_by_session("cao-a") == [] + assert database.list_terminals_by_session("cao-b") == [] + + +class RecreateOnKillBackend(FakeTmuxBackend): + """Reuses the session name the instant the old incarnation's kill lands. + + ``kill_session`` reaps the old session and then immediately stands a NEW one + up under the same name with its own fresh-id row — modelling a create that + claims the freed name between the kill and the teardown's sweep. Hooking + ``kill_session`` (rather than the sweep helper) keeps the injection point + identical across implementations, so the test measures the sweep's SCOPING + rather than which helper it happens to call. + """ + + def kill_session(self, session_name: str) -> bool: + killed = super().kill_session(session_name) + if killed and not database.get_terminal_metadata("t-new"): + self.add_session(session_name, {"w-new"}) + database.create_terminal( + terminal_id="t-new", + tmux_session=session_name, + tmux_window="w-new", + provider="claude_code", + agent_profile="developer", + ) + return killed + + +def test_sweep_is_scoped_to_the_enumerated_incarnation(real_db, runtime): + """The reconciliation sweep deletes ONLY the ids it enumerated (#498). + + Defence in depth behind the lifecycle lock. The lock stops an in-process + create from interleaving, but the sweep must still be scoped by ID rather + than by session NAME, because "every row carrying this name" is not the same + set as "the rows this teardown decided to remove" — anything that appears + under the name after enumeration (an out-of-band writer, a future + multi-process topology, a retry that re-registers) belongs to a live + incarnation this teardown knows nothing about. + + A new incarnation claims the name after enumeration (see + ``RecreateOnKillBackend``). It must survive. Under the old + ``delete_terminals_by_session(name)`` sweep it is destroyed — an unconditional + delete of EVERY row for the name — while its tmux session lives on. + """ + backend = RecreateOnKillBackend() + set_backend(backend) + _seed(backend, "cao-reuse", [("t-old", "w-old")], runtime) + + result = session_service.delete_session("cao-reuse") + + assert result == {"deleted": ["cao-reuse"], "errors": []} + # The newcomer's row and tmux session both survive; only the enumerated + # incarnation was removed. + assert backend.session_exists("cao-reuse") is True + assert {r["id"] for r in database.list_terminals_by_session("cao-reuse")} == {"t-new"} + + +def test_teardown_does_not_self_deadlock_on_the_lifecycle_lock(real_db, runtime): + """The lock is non-reentrant, so nothing inside the critical section may + re-acquire it. A regression that did (e.g. a nested delete_session, or a + plugin dispatched from inside the lock that tears down the same name) would + hang forever; the timeout makes it a FAILURE instead. + + Also proves the lock registry does not leak: after N teardowns of distinct + names, nothing remains registered. + """ + backend = FakeTmuxBackend() + set_backend(backend) + for i in range(5): + _seed(backend, f"cao-nd{i}", [(f"t{i}", f"w{i}")], runtime) + + results = _run_threads( + [(lambda n=f"cao-nd{i}": session_service.delete_session(n)) for i in range(5)] + ) + for status, value in results: + assert status == "ok", f"teardown raised: {value!r}" + + # Refcounted registry drains to empty — no unbounded accumulation. + assert session_lock._session_locks == {} + + # And the lock is genuinely re-acquirable after every release (a lock left + # held would block here until the timeout). + acquired = [] + for i in range(5): + with session_lock.session_lifecycle_lock(f"cao-nd{i}"): + acquired.append(i) + assert acquired == [0, 1, 2, 3, 4] + assert session_lock._session_locks == {} + + +def test_no_plugin_code_runs_inside_the_lifecycle_lock(real_db, runtime): + """Plugin hooks must be dispatched only AFTER the lock is released (#498). + + Plugin code is third-party and unbounded, and on the API path it does not + merely get scheduled: ``delete_session`` runs under ``asyncio.to_thread``, so + ``dispatch_plugin_event`` finds no running loop and falls back to + ``asyncio.run`` — the hook executes to completion INLINE. A hook dispatched + from inside the critical section therefore holds the per-name lifecycle lock + for as long as it runs, and a slow or hanging plugin stalls every subsequent + create/teardown of that session name. + + Rather than assert on ordering (which cannot tell "after the last step" from + "after the release"), each hook here probes the lock directly: it tries to + acquire the very lock ``delete_session`` uses, non-blocking. Succeeding proves + the lock was already free when the hook ran. Both event types are checked, + because ``post_kill_terminal`` is emitted per contained terminal from what + used to be the middle of the critical section. + """ + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-plug", [("t1", "w1"), ("t2", "w2")], runtime) + + observed: List[tuple] = [] + + class ProbingRegistry: + """A plugin registry whose hook tests whether the lock is still held.""" + + async def dispatch(self, event_type, event): + # The registry guard is only ever held for dict bookkeeping, so + # reading the entry here is safe and non-blocking. + entry = session_lock._session_locks.get("cao-plug") + if entry is None: + # Refcount drained to zero and the entry was evicted, which only + # happens after the holder released: definitively free. + observed.append((event_type, True)) + return + lock, _holders = entry + free = lock.acquire(blocking=False) + observed.append((event_type, free)) + if free: + lock.release() + + session_service.delete_session("cao-plug", registry=ProbingRegistry()) + + kinds = [kind for kind, _ in observed] + # Every terminal's event fired, plus the session one, and nothing was + # swallowed by moving the dispatch out of the loop. + assert kinds == ["post_kill_terminal", "post_kill_terminal", "post_kill_session"] + still_locked = [kind for kind, was_free in observed if not was_free] + assert still_locked == [], ( + "these plugin events were dispatched while the lifecycle lock for " + f"'cao-plug' was still held: {still_locked}" + ) + # The teardown itself still did its job. + assert backend.session_exists("cao-plug") is False + assert database.list_terminals_by_session("cao-plug") == [] + + +# ── A raise in the TAIL of the critical section must not destroy the events ─── + + +class RecordingRegistry: + """Records every dispatched event type, in order.""" + + def __init__(self) -> None: + self.events: List[str] = [] + + async def dispatch(self, event_type, event): + self.events.append(event_type) + + +def _db_locked(*_args, **_kwargs): + """Raise the SQLite error the tail sweep realistically hits. + + ``clients/database.py`` builds its engine with neither ``busy_timeout`` nor + WAL, so with several concurrent writers (status monitor, inbox, other + terminals) a write can exhaust sqlite3's default 5s timeout and raise. + """ + raise OperationalError("DELETE FROM terminals", {}, Exception("database is locked")) + + +def test_tail_sweep_failure_does_not_lose_the_plugin_events(real_db, runtime, monkeypatch): + """A raise in the tail must not turn a COMPLETED teardown into zero events. + + By the time the by-id sweep runs, tmux is provably gone, every runtime is + dismantled and every row is deleted — the teardown is complete and durable. + Before the guard, an ``OperationalError`` from that sweep propagated out of + ``delete_session``, so the dispatch loop past the critical section was never + reached and ALL THREE events were dropped. The per-terminal ones are then + unrecoverable: a re-run rebuilds ``torn_down`` from rows that no longer + exist, so it can only re-emit ``post_kill_session``. + + Pre-PR this could not happen — ``delete_terminal`` emitted + ``post_kill_terminal`` inline per terminal, so a tail failure cost at most + the session event. Deferring dispatch past the lock is right; its failure + mode needed handling. + """ + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-tail", [("t1", "w1"), ("t2", "w2")], runtime) + monkeypatch.setattr(session_service, "delete_terminals_by_ids", _db_locked) + + registry = RecordingRegistry() + result = session_service.delete_session("cao-tail", registry=registry) + + # Every event the completed work is entitled to, in order. + assert registry.events == ["post_kill_terminal", "post_kill_terminal", "post_kill_session"] + # The teardown DID complete, so it must not be reported as a total failure. + assert result["deleted"] == ["cao-tail"] + assert backend.session_exists("cao-tail") is False + assert database.list_terminals_by_session("cao-tail") == [] + assert runtime.is_fully_gone("t1") + assert runtime.is_fully_gone("t2") + # ... but the tail error is not swallowed silently either. + assert [e["step"] for e in result["errors"]] == ["delete_terminals_by_ids"] + assert "database is locked" in result["errors"][0]["error"] + + +def test_clear_session_env_failure_does_not_lose_the_plugin_events(real_db, runtime, monkeypatch): + """Same for the other unguarded tail step, the forwarded-env drop. + + It is the last statement in the critical section, so a raise here loses the + events for work that is entirely finished — including the row sweep. + """ + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-envfail", [("t1", "w1")], runtime) + + def _boom(session_name): + raise RuntimeError("session env store unreadable") + + monkeypatch.setattr(session_service, "clear_session_env", _boom) + + registry = RecordingRegistry() + result = session_service.delete_session("cao-envfail", registry=registry) + + assert registry.events == ["post_kill_terminal", "post_kill_session"] + assert result["deleted"] == ["cao-envfail"] + assert [e["step"] for e in result["errors"]] == ["clear_session_env"] + assert database.list_terminals_by_session("cao-envfail") == [] + + +def test_tail_guard_does_not_mask_an_unconfirmed_kill(real_db, runtime, monkeypatch): + """The guard covers ONLY the post-confirmation tail. + + An unconfirmed kill must still raise with nothing dismantled and no event + emitted — the tail steps are never even reached, so guarding them cannot + convert a real failure into a reported success. + """ + backend = FakeTmuxBackend(kill_fails=True) + set_backend(backend) + _seed(backend, "cao-tailsafe", [("t1", "w1")], runtime) + swept: List[List[str]] = [] + monkeypatch.setattr( + session_service, "delete_terminals_by_ids", lambda ids: swept.append(list(ids)) + ) + + registry = RecordingRegistry() + with pytest.raises(RuntimeError, match="still exists after kill_session"): + session_service.delete_session("cao-tailsafe", registry=registry) + + assert registry.events == [] + assert swept == [] + assert backend.session_exists("cao-tailsafe") is True + assert {r["id"] for r in database.list_terminals_by_session("cao-tailsafe")} == {"t1"} + assert runtime.is_fully_live("t1") + + +# --- Atomicity of the locked CREATE closure itself ------------------------- +# +# The tests above pin the teardown side and the create-vs-teardown ordering. The +# two below pin the create's OWN all-or-nothing property: the closure holding the +# lifecycle lock makes the backend resource AND its registry row, and a failure +# between them must not leave the backend resource behind. + + +def _create_in_thread_kw(**kwargs): + """Run the async create_terminal from a worker thread with explicit kwargs. + + Same as ``_create_in_thread`` but for the cases that need to vary + ``new_session`` / ``env_vars``. + """ + import asyncio + + return asyncio.run( + terminal_service.create_terminal( + provider="claude_code", + agent_profile="developer", + **kwargs, + ) + ) + + +def test_row_write_failure_kills_the_session_it_just_created(real_db, runtime, monkeypatch): + """new_session=True: a mid-closure failure must not orphan the tmux session. + + The registry write is the LAST step of the locked critical section, and it is + the one that realistically fails: ``clients/database.py`` builds its engine + with neither ``busy_timeout`` nor WAL, so a concurrent writer makes + "database is locked" an ordinary outcome rather than a pathological one. + + The regression this catches: the closure returns its + ``(window_name, session_created, window_created)`` tuple only on FULL + success, so when it raises after ``create_session`` landed, the outer + ``session_created`` flag the ``except`` block keys its teardown off is still + False — nothing is killed, and a live tmux session is left with no registry + row. That is precisely the divergence #498 exists to eliminate, produced by + the create path instead of the teardown path. Pre-#498 code set the flag + immediately after ``create_session``, so any later failure killed it. + """ + backend = FakeTmuxBackend() + set_backend(backend) + monkeypatch.setattr(terminal_service, "db_create_terminal", _db_locked) + + with pytest.raises(OperationalError, match="database is locked"): + _create_in_thread_kw( + session_name="cao-rollback", + new_session=True, + env_vars={"SECRET": "s3cret"}, + ) + + # The session this call created is GONE — not left running behind a failed + # create, and not waiting on an out-of-band reconciliation to notice it. + assert backend.session_exists("cao-rollback") is False + assert backend.kill_session_calls == 1 + # Neither store holds anything for the name: no orphan in EITHER direction. + assert database.list_terminals_by_session("cao-rollback") == [] + # Forwarded env is dropped with the session, so the secret cannot linger in + # memory or bleed into a future reuse of the name. + assert session_env.get_session_env("cao-rollback") == {} + + +def test_row_write_failure_kills_only_the_window_it_added(real_db, runtime, monkeypatch): + """new_session=False: kill the added WINDOW, leave the session and its peers. + + This is the branch every MCP spawn/assign-into-an-existing-session call + takes, and its rollback is NOT the session-level one: the session pre-existed + this call, so tearing it down would destroy terminals the failed create never + owned. Only the one window added under the lock may go. + + Same regression as the session branch — ``window_created`` is also assigned + only from a successful return, so a mid-closure failure left the pane alive + with no row: invisible to every list/tree view (the row is gone), never + reconciled, sitting there indefinitely. + """ + backend = FakeTmuxBackend() + set_backend(backend) + _seed(backend, "cao-existing", [("t-peer", "w-peer")], runtime) + monkeypatch.setattr(terminal_service, "db_create_terminal", _db_locked) + + with pytest.raises(OperationalError, match="database is locked"): + _create_in_thread_kw(session_name="cao-existing", new_session=False) + + # The added window is gone... + assert backend.kill_window_calls == 1 + # ...and the pre-existing session survived with its own window untouched. + assert backend.session_exists("cao-existing") is True + assert backend.windows("cao-existing") == {"w-peer"} + assert backend.kill_session_calls == 0 + # The peer terminal is untouched in both stores. + assert {r["id"] for r in database.list_terminals_by_session("cao-existing")} == {"t-peer"} + assert runtime.is_fully_live("t-peer") + + +class KeyboardInterruptOnKillBackend(FakeTmuxBackend): + """``kill_session`` raises a BaseException instead of returning. + + Models a Ctrl-C (or a SystemExit from a shutdown signal) landing while the + rollback is mid-kill — the one window where ``except Exception`` around the + kill is not enough. + """ + + def kill_session(self, session_name: str) -> bool: + self.kill_session_calls += 1 + raise KeyboardInterrupt() + + +def test_rollback_clears_forwarded_env_even_when_the_kill_raises_base_exception( + real_db, runtime, monkeypatch +): + """A BaseException out of the rollback kill must not strand the secret. + + ``_roll_back_backend_create_locked`` clears the forwarded-env mapping in a + ``finally``, not as a following statement, for exactly this case: with two + sequential ``try/except Exception`` blocks a ``KeyboardInterrupt``/ + ``SystemExit`` from the kill skips the clear entirely, leaving an operator's + forwarded secret (``cao launch --env``) in the process-global map keyed to a + session name that no longer has a live session — and which a later launch can + reuse and silently inherit. ``finally`` still lets the BaseException + propagate, which is required: a Ctrl-C must not be swallowed here. + """ + backend = KeyboardInterruptOnKillBackend() + set_backend(backend) + monkeypatch.setattr(terminal_service, "db_create_terminal", _db_locked) + + # The KeyboardInterrupt from the rollback replaces the OperationalError that + # triggered it, and propagates all the way out: create_terminal's own cleanup + # `except Exception` cannot catch a BaseException either. + with pytest.raises(KeyboardInterrupt): + _create_in_thread_kw( + session_name="cao-ctrl-c", + new_session=True, + env_vars={"SECRET": "s3cret"}, + ) + + # The kill WAS attempted (so this is the interrupted-rollback path, not a + # rollback that never ran) ... + assert backend.kill_session_calls == 1 + # ... and the secret is gone regardless of how that attempt ended. + assert session_env.get_session_env("cao-ctrl-c") == {} + # The row write is what failed, so neither store should hold a row. + assert database.list_terminals_by_session("cao-ctrl-c") == [] + # The lifecycle lock is released on the BaseException path too — a leaked + # lock would deadlock every later create/teardown of this name, so prove a + # subsequent acquire of the SAME name still succeeds promptly. + assert session_lock._session_locks == {} + acquired = threading.Event() + + def _reacquire(): + with session_lock.session_lifecycle_lock("cao-ctrl-c"): + acquired.set() + + t = threading.Thread(target=_reacquire, daemon=True) + t.start() + t.join(timeout=DEADLOCK_TIMEOUT) + assert acquired.is_set(), "lifecycle lock was not released when the rollback was interrupted"