From 6af13ff33f61d94b6ec1f4d897d67afbd1ce29a3 Mon Sep 17 00:00:00 2001 From: Leo Zhadanovsky Date: Thu, 3 Sep 2026 08:55:53 +0000 Subject: [PATCH] fix(session-pid): stop a recycled pid from preserving a stale session mapping `cleanup_orphaned_sessions()` prunes `session_pid_.txt` only when `platform_compat.pid_exists()` is false, and on POSIX that is literally `os.kill(pid, 0)`. Linux draws thread ids from the pid space, exposes `/proc/`, and lets you signal a tid -- so once a dead session's pid is recycled as a THREAD of an unrelated live process, the mapping passes the check and survives indefinitely. What that costs depends on the mapping. One that records a start token is already safe to resolve: `session_pid_sig._pid_recycled()` compares the live start token and refuses on a mismatch on both the strict and the lenient path, and a tid's live token cannot match the dead process's. A LEGACY token-less mapping has no recorded token for that guard to compare, so it keeps resolving for as long as the file lingers -- same-uid only, so robustness rather than a privilege boundary. Independently of either, nothing retires the files until the next gateway restart. Adds `platform_compat.live_thread_group_leaders()`: a single `os.listdir("/proc")` for the whole sweep. That listing enumerates ONLY thread-group leaders -- a non-leader tid is absent from it even though `/proc/` stays directly openable, which is exactly why the cheaper per-pid probes cannot tell the two apart. One directory read replaces a synchronous `/proc//status` read per mapping, so the sweep performs no per-entry file I/O at all; measured on Linux that is 1.5 ms once against 7.8 ms across 233 mappings. It is also cheaper than `process_matches()`, which shells out to `ps` on macOS and so cannot be used per entry. The helper returns `None` -- never an empty set -- on non-Linux, on an unreadable `/proc`, and on a listing carrying no pids, and the caller treats `None` as retain-everything. An inconclusive answer can therefore never be the thing that decides a pid is stale; it only ever narrows an existing liveness check. The mapping files are materialized before the snapshot is taken, and that ordering is load-bearing rather than incidental: a mapping file is written at spawn, so every path in the list belongs to a process that already existed when the snapshot was read, and is therefore present in it and retained. Iterating the glob lazily instead would let an entry yielded AFTER the snapshot belong to a pid absent from it -- pruning a live session's mapping. The list is bounded by the mapping count the sweep was already reading per-entry (233 on the host measured above). The narrowing is asked for on ONE call site, and the sweep runs only where it already ran. The stale-mapping pass is now `_prune_stale_session_pid_files(narrow_with_leaders=)` -- private, since it has one production consumer. The gateway's boot path and its force-exit handler both pass `False`, so each does exactly the work it did before this branch: `no-new-work-on-gateway-boot-path` names orphan sweeps specifically and the leaders snapshot is a `/proc` read that path may not carry, and a signal handler that reaches for extra work before its `os._exit` is a handler that may not get there. The graceful-shutdown sweep asks for the narrowing. That placement is the point rather than a compromise. Nothing is spawning a session by then, so the pass is not racing a mapping publisher, and it holds exactly the position the sweep already held -- so this change adds no concurrency that `main` did not already have, and needs no new lock, no new lock file in an agent-writable directory, and no work on a path that must stay short. The cost is that a recycled-pid mapping is retired at shutdown rather than mid-run; bounding accumulation across restarts is what the sweep is for, and resolution of a token-bearing mapping is already guarded by the recorded start token independently of it. Owned plainly: a gateway that is hard-killed never reaches the graceful path, so on that host its recycled-pid mappings wait for a later clean exit. Snapshot staleness is still handled, because the guarantee should not rest on the call site alone. The leaders set is read ONCE for the pass, so a pid recycled after that read would be absent from it while naming a live process. Absence from the snapshot therefore selects a CANDIDATE only, and the decision takes a reading for that one pid: new `platform_compat.is_thread_group_leader()` reads `Tgid` from `/proc//status`, and a mapping is unlinked only on a definite "not a process" -- retained on True and on every inconclusive answer. That read is paid only for candidates, so the common path still does no per-entry file I/O and the host-wide listing keeps doing the cheap filtering it was added for. The sweep is synchronous filesystem work end to end -- a glob to drive it, a `pid_exists` per entry, one `/proc` listing, and an unlink per pruned entry -- so the two call sites that run inside the orchestrator coroutine (startup and post-shutdown) now `await asyncio.to_thread(...)`. That also takes the pre-existing glob, probe and unlink work off the loop, so those paths are strictly less blocking than before this branch. The force-exit signal handler keeps the synchronous call: a handler cannot await, and the process calls `os._exit` immediately afterwards, so loop latency is not meaningful there. The pruning predicate is Linux-only: both new helpers answer `None` off Linux and `None` never licenses a removal, so macOS and Windows keep the pre-existing `pid_exists`-only outcome. The `asyncio.to_thread` move is NOT so scoped -- both orchestrator sweeps now run on a worker thread on every platform. That is what takes the pre-existing glob, probe and unlink work off the loop, and it is safe on all three: `cleanup_orphaned_sessions` takes the pid-file lock and signals tracked pids, and calls nothing that is bound to the loop or the main thread (no `signal.signal`, no loop access, no `asyncio` use anywhere in it). Tests: `live_thread_group_leaders()` unit coverage including every fail-open branch, and a behaviour test that recycles a real thread tid and asserts the mapping is pruned. Five cover the per-pid re-read: `is_thread_group_leader()` on this process, on a real live thread's tid (which `pid_exists` reports alive -- the trap), on non-Linux, on a vanished pid and on a malformed `status`. Three pin the contracts the call sites rest on: the `False` setting takes no `/proc` read at all, the pass leaves the shared `kiro_session_pids.txt` byte-equal while pruning a mapping, and a snapshot taken before this process existed does not cost the live mapping. The byte-equal test pins its `os.kill` probe rather than assuming pid 99999 is dead on the host, as its sibling sweeps already do -- `pid_max` is 4194304, so an unpinned probe is a load-dependent runner flake. Each of those was checked by INVERSION rather than only by passing -- removing the revalidation fails the snapshot test, and flipping the setting to the narrowed form fails the no-`/proc` test. 461 passed / 23 skipped across the two touched test files, against 447 / 23 on `main`; 500 passed across the publish-path, gateway, spawn-offload and sweep-helper suites; `mypy src/kiro_crew/` clean over 1378 source files. Deliberately deferred: `_skip_tagged` (session_pid.py) still asks only `pid_exists` of the owning gateway pid, so a gateway pid recycled as a tid keeps its orphans from being reaped. It is the same class this change fixes one pass later and the leaders set is already in hand, but narrowing a kill-safety predicate changes what gets SIGKILLed rather than what gets unlinked, and that belongs in its own change with its own tests. The comment-history gate added in #9328 judges every file a diff touches, and this branch clears it without editing one pre-existing comment: the comments it adds carry their reason without the historical framing docs/system-specs/common/code-style.md forbids, and `slack/gateway.py` shows only functional edits, its `#7518` provenance reference intact. --- docs/system-specs/common/platform-compat.md | 1 + docs/system-specs/modules/session.md | 25 +++- src/kiro_crew/platform_compat.py | 73 ++++++++++ src/kiro_crew/session_pid.py | 111 ++++++++++++---- src/kiro_crew/slack/gateway.py | 34 ++++- test/test_pid_lifecycle.py | 140 ++++++++++++++++++++ test/test_platform_compat.py | 119 +++++++++++++++++ 7 files changed, 475 insertions(+), 28 deletions(-) diff --git a/docs/system-specs/common/platform-compat.md b/docs/system-specs/common/platform-compat.md index 071c2a87822..98aa01c1bbe 100644 --- a/docs/system-specs/common/platform-compat.md +++ b/docs/system-specs/common/platform-compat.md @@ -31,6 +31,7 @@ produces exactly those silent failures, which is why the helper is named per cal | Parent PID | `get_ppid(pid)` | `/proc` read / libproc | | Match process cmdline | `process_matches(pid, needles)` | `/proc//cmdline` / `ps` | | Process start time (PID-reuse guard) | `process_start_time(pid)` | `/proc//stat` / `ps -o lstart=` (both answer `None` on Windows, so the guard silently never confirms) | +| Is this pid a PROCESS rather than a thread | `is_thread_group_leader(pid)` for one pid; `live_thread_group_leaders()` once for a whole sweep | `pid_exists(pid)` alone (Linux numbers threads from the pid space and POSIX permits signalling a tid, so a pid recycled as a THREAD of an unrelated process reads as alive forever). Both answer `None`, never `False`, when unknowable — treat `None` as "retain", never as licence to act | | Signals | `platform_compat.SIGKILL` / `SIGTERM` | `signal.SIGKILL` (undefined on Windows) | | Spawn isolation | `start_new_session=IS_POSIX` + `creationflags=CREATE_NEW_PROCESS_GROUP` | bare `start_new_session=True` | | Re-exec the current Python module | `reexec_python_module(module, args)` | `os.execv(sys.executable, [sys.executable, ...])` (breaks when the Windows interpreter path contains spaces) | diff --git a/docs/system-specs/modules/session.md b/docs/system-specs/modules/session.md index 751cf09ed6c..0b9d6811ea4 100644 --- a/docs/system-specs/modules/session.md +++ b/docs/system-specs/modules/session.md @@ -1467,7 +1467,30 @@ a trust root on its own; publication therefore also writes a sidecar; strict resolvers fail closed until the next turn's publish re-signs the mapping. Benign and self-healing — no migration step. - **Stale cleanup**: the orphan sweep removes `session_pid_.sig` - alongside its `.txt` for dead pids (`session_pid.py`). + alongside its `.txt` for dead pids (`session_pid.py`). "Dead" is not + `pid_exists` alone: Linux numbers threads from the pid space, so a dead + session's pid recycled as a THREAD of an unrelated live process still + satisfies that probe and the mapping would survive forever (observed on a + host whose pid counter had wrapped: 233 mappings, one naming a 6-day-dead + session through a thread). `_prune_stale_session_pid_files` therefore + removes a mapping when the pid is unsignalable, OR when it is absent from + one `platform_compat.live_thread_group_leaders()` snapshot **and** a + per-pid `platform_compat.is_thread_group_leader(pid)` re-read returns + `False`. Both helpers answer `None` when the question is unknowable + (non-Linux, unreadable `/proc`), and `None` never licenses a removal — so + macOS and Windows keep the pre-existing `pid_exists`-only behaviour. Two + orderings are load-bearing: the snapshot is taken AFTER the glob (a pid + that starts in that window lands IN the set and is retained), and absence + from it selects a *candidate* rather than the outcome, so a pid recycled + since the snapshot — whose new owner has already republished the mapping at + that same path — is retained by the re-read instead of losing a live + session's identity. The snapshot costs one `/proc` directory read for the + whole pass, which is still work the gateway boot path does not carry: + `narrow_with_leaders=False` there per `no-new-work-on-gateway-boot-path` + (and on the force-exit handler, which must reach its `os._exit`), while the + graceful-shutdown sweep asks for the narrowing. This pass touches only the + `session_pid_` family, never the shared `kiro_session_pids.txt` that + pass 1 rewrites. - **Threat model** (full version in the `session_pid_sig.py` module docstring): file forgery, cross-pid replay, tampering, and symlink planting are blocked; deliberate same-uid impersonation via diff --git a/src/kiro_crew/platform_compat.py b/src/kiro_crew/platform_compat.py index bf4905a96df..d5cb9c1c75a 100644 --- a/src/kiro_crew/platform_compat.py +++ b/src/kiro_crew/platform_compat.py @@ -2814,6 +2814,79 @@ def process_owner_uid(pid: int) -> int | None: PID_UNSIGNALABLE = "unsignalable" # exists but we cannot signal it (POSIX EPERM) +def live_thread_group_leaders() -> frozenset[int] | None: + """Every pid on the host that is a PROCESS, or ``None`` when unknowable. + + Linux numbers threads from the same space as processes and exposes + ``/proc/`` for them, and POSIX permits signalling a tid — so a tid + satisfies both :func:`pid_exists` and :func:`pid_liveness` while naming no + process at all. A caller holding a recorded pid therefore cannot tell "my + process is still alive" from "that number now belongs to some unrelated + process's thread", which matters once the pid counter wraps + (``/proc/sys/kernel/pid_max`` is commonly 4194304 and a busy host cycles it + in hours). + + The discriminator is ``/proc`` itself: its top-level listing enumerates + ONLY thread-group leaders. A non-leader tid is absent from that listing + even though ``/proc/`` stays directly openable — which is exactly why + the cheaper per-pid probes cannot see the difference. + + Deliberately ONE directory read for the whole host rather than a read per + pid. A sweep over N recorded mappings costs a single ``os.listdir`` instead + of N opens of ``/proc//status`` (measured on Linux: 1.5 ms once versus + 7.8 ms across 233 mappings), so no per-entry synchronous file read happens + on the caller's thread at all. Also cheaper than ``process_matches``, which + shells out to ``ps`` on macOS and so cannot be used per entry in a sweep. + + Returns ``None`` — never an empty set — whenever the answer is not knowable + (non-Linux, unreadable ``/proc``, or a listing with no numeric entries). + Callers use this to *narrow* a liveness check, so an inconclusive result + must never be the thing that decides a pid is stale: treat ``None`` as + "retain everything". + """ + if not IS_LINUX: + return None + try: + entries = os.listdir("/proc") + except OSError: + return None + leaders = {int(name) for name in entries if name.isdigit()} + if not leaders: + return None + return frozenset(leaders) + + +def is_thread_group_leader(pid: int) -> bool | None: + """Whether ``pid`` names a PROCESS right now, or ``None`` when unknowable. + + The per-pid counterpart to :func:`live_thread_group_leaders`, for the one + question a host-wide snapshot cannot answer. A snapshot is a reading taken at + an instant, so a pid recycled AFTER it was taken is absent from it while + naming a live process. A caller about to act destructively on "absent from + the snapshot" therefore needs a reading taken now, for that pid alone. + + ``/proc//status`` carries ``Tgid``, the pid of the thread group's + leader, so ``Tgid == pid`` is a process while a non-leader tid reports its + leader's pid instead. One file read is the cheap way to ask about one pid, + where the top-level ``/proc`` listing is the cheap way to ask about all of + them -- which is why this narrows the snapshot rather than replacing it. + + Returns ``None`` -- never ``False`` -- whenever the answer is not knowable + (non-Linux, the pid is gone, an unreadable or malformed ``status``), so an + inconclusive read can never be the thing that licenses a destructive action. + """ + if not IS_LINUX: + return None + try: + with open(f"/proc/{pid}/status", encoding="utf-8", errors="replace") as handle: + for line in handle: + if line.startswith("Tgid:"): + return int(line.split()[1]) == pid + except (OSError, ValueError, IndexError): + return None + return None + + def pid_liveness(pid: int) -> str: """Three-way liveness probe: PID_DEAD / PID_ALIVE / PID_UNSIGNALABLE. diff --git a/src/kiro_crew/session_pid.py b/src/kiro_crew/session_pid.py index 02dc069868e..da2a56dd215 100644 --- a/src/kiro_crew/session_pid.py +++ b/src/kiro_crew/session_pid.py @@ -709,7 +709,7 @@ def _cleanup_orphaned_mcp_servers() -> int: return killed -def cleanup_orphaned_sessions() -> None: +def cleanup_orphaned_sessions(*, narrow_with_leaders: bool = True) -> None: """Kill leftover kiro-cli processes from a previous gateway run. Reads ``kiro_session_pids.txt`` (written at spawn time), validates each @@ -722,6 +722,13 @@ def cleanup_orphaned_sessions() -> None: Also sweeps orphaned MCP server processes via ``_cleanup_orphaned_mcp_servers`` which uses the separate ``kiro_pids.txt`` (child:parent format). + ``narrow_with_leaders`` is forwarded to + :func:`_prune_stale_session_pid_files`. The gateway passes ``False`` on its + boot path and in its force-exit handler, so both do exactly the work they + did before the recycled-pid change; the narrowing applies on the graceful + shutdown path, which is not spawning sessions. + + Additionally cleans up: - Stale ``session_pid_*.txt`` files for processes that no longer exist. - Empty directories under ``sessions/`` left by subagents that produced @@ -759,8 +766,50 @@ def _skip_tagged(gw_pid: int, _pid: int) -> bool: logger.info("Cleaned up %d orphaned MCP server processes", mcp_killed) # Third pass: remove stale session_pid_*.txt files for dead processes + _prune_stale_session_pid_files(narrow_with_leaders=narrow_with_leaders) + + # Fourth pass: remove empty session workspace dirs (orphaned subagent dirs) + sessions_dir = config_dir() / "sessions" + empty_dirs = 0 + if sessions_dir.exists(): + for d in sessions_dir.iterdir(): + if d.is_dir() and not any(d.iterdir()): + try: + d.rmdir() + empty_dirs += 1 + except OSError: + pass # directory became non-empty or was already removed + if empty_dirs: + logger.info("Cleaned up %d empty session workspace dirs", empty_dirs) + + +def _prune_stale_session_pid_files(*, narrow_with_leaders: bool = True) -> int: + """Remove ``session_pid_.txt`` mappings whose pid is not that session. + + ``narrow_with_leaders`` decides whether the thread-group-leaders snapshot is + taken. It costs one ``/proc`` directory read for the whole pass and is what + catches a pid recycled as a THREAD of a live process, but it is work the + gateway boot path may not carry: ``no-new-work-on-gateway-boot-path`` names + orphan sweeps specifically, so the boot caller passes ``False``. The + narrowing is asked for on the graceful shutdown path instead. + + A live session's pid is both signalable and a thread-group leader, so it is + retained under either setting, and this pass never touches the shared + ``kiro_session_pids.txt`` that pass 1 rewrites. + + Returns the number of mapping files removed. + """ stale_pid_files = 0 - for pid_file in config_dir().glob("session_pid_*.txt"): + pid_files = list(config_dir().glob("session_pid_*.txt")) + # Snapshot the host's thread-group leaders ONCE for the whole sweep — one + # directory read instead of a synchronous /proc read per mapping. + # + # Ordering matters: snapshot AFTER globbing. A pid that starts in the window + # between the two lands IN the set and is retained; one that exits in that + # window is absent and is pruned, which is correct. Snapshotting first would + # invert both. + leaders = platform_compat.live_thread_group_leaders() if narrow_with_leaders else None + for pid_file in pid_files: try: pid = int(pid_file.stem.removeprefix("session_pid_")) except ValueError: @@ -773,29 +822,45 @@ def _skip_tagged(gw_pid: int, _pid: int) -> bool: logger.debug("Could not remove malformed pid file: %s", pid_file.name) continue # os.kill(pid, 0) would terminate the process on Windows — probe instead. - if not platform_compat.pid_exists(pid): - pid_file.unlink(missing_ok=True) - # Remove the HMAC sidecar (session_pid_.sig) alongside its - # .txt — a dangling sidecar is harmless (verification requires - # both) but would accumulate forever. - pid_file.with_suffix(".sig").unlink(missing_ok=True) - stale_pid_files += 1 + # + # The leaders set narrows the liveness test: a dead session's pid can be + # recycled as a THREAD of an unrelated live process, and a tid satisfies + # ``pid_exists``, so that probe alone would keep the mapping forever. + # + # Resolution of a TOKEN-BEARING mapping is already safe without this: + # ``session_pid_sig._pid_recycled`` compares the live start token and + # refuses on a mismatch on both the strict and the lenient path, and a + # tid's live start token cannot match the dead process's. What this + # sweep adds is (a) pruning LEGACY token-less mappings, where that + # guard has no recorded token to compare and callers keep resolving, + # and (b) bounding accumulation — observed on a host whose pid counter + # had wrapped: 233 mappings, 1 still naming a 6-day-dead session via a + # thread of an unrelated process. + # + # ``leaders is None`` means the question was unanswerable (non-Linux, + # unreadable /proc), so it never contributes to a prune. + if platform_compat.pid_exists(pid): + if leaders is None or pid in leaders: + continue + # Absence from the snapshot selects a CANDIDATE, never the outcome. + # The snapshot was read before this loop, so a pid recycled since -- + # whose mapping the new owner has already republished at this same + # path -- is missing from it while naming a LIVE session. Unlinking + # that mapping would lose a live session's identity, so the decision + # needs a reading for this pid taken now. Retain on anything but a + # definite "not a process", and pay the per-pid read only for the + # few candidates rather than for every mapping. + if platform_compat.is_thread_group_leader(pid) is not False: + continue + pid_file.unlink(missing_ok=True) + # Remove the HMAC sidecar (session_pid_.sig) alongside its + # .txt — a dangling sidecar is harmless (verification requires + # both) but would accumulate forever. + pid_file.with_suffix(".sig").unlink(missing_ok=True) + stale_pid_files += 1 if stale_pid_files: logger.info("Cleaned up %d stale session PID files", stale_pid_files) - - # Fourth pass: remove empty session workspace dirs (orphaned subagent dirs) - sessions_dir = config_dir() / "sessions" - empty_dirs = 0 - if sessions_dir.exists(): - for d in sessions_dir.iterdir(): - if d.is_dir() and not any(d.iterdir()): - try: - d.rmdir() - empty_dirs += 1 - except OSError: - pass # directory became non-empty or was already removed - if empty_dirs: - logger.info("Cleaned up %d empty session workspace dirs", empty_dirs) + return stale_pid_files def cleanup_orphaned_session_roots() -> int: diff --git a/src/kiro_crew/slack/gateway.py b/src/kiro_crew/slack/gateway.py index dd206cb9604..cc97875736e 100644 --- a/src/kiro_crew/slack/gateway.py +++ b/src/kiro_crew/slack/gateway.py @@ -11215,7 +11215,19 @@ def _probe_persistence() -> str | None: # Clean up orphaned kiro-cli processes from previous runs from kiro_crew.session import cleanup_orphaned_sessions - cleanup_orphaned_sessions() + # Off-loop: the sweep is synchronous filesystem work and this runs + # inside the orchestrator coroutine. + # + # ``narrow_with_leaders=False`` keeps boot doing exactly what it did + # before the recycled-pid work: the leaders snapshot is a /proc read this + # path may not carry, since no-new-work-on-gateway-boot-path names orphan + # sweeps. + # + # The narrowing is asked for on the graceful-shutdown sweep only, so a + # gateway that is hard-killed never runs it and its recycled-pid mappings + # wait for a later clean exit. That is the accepted cost of keeping this + # path, and the force-exit handler, doing exactly their pre-existing work. + await asyncio.to_thread(cleanup_orphaned_sessions, narrow_with_leaders=False) # Same "previous run left residue" concern as the orphan sweep above, for # telemetry rather than processes: any open-session crumb on disk belongs @@ -11469,7 +11481,16 @@ def _on_signal(*_args: object) -> None: nonlocal _shutting_down if _shutting_down: print("\n👻 Force exit!") - cleanup_orphaned_sessions() + # Synchronous by necessity: a signal handler cannot await. + # The process calls os._exit immediately below, so loop latency + # does not matter on this path. + # + # ``narrow_with_leaders=False`` so this handler does exactly the + # work it did before the recycled-pid change: killing leftover + # processes is what this path is for, and a handler that reaches + # for extra work before its os._exit is a handler that may not + # get there. + cleanup_orphaned_sessions(narrow_with_leaders=False) # os._exit skips atexit, so the log queue's drain hook never # runs — flush the queued gateway.log tail here, bounded so a # wedged disk cannot hang the force exit. @@ -11700,8 +11721,13 @@ async def _start_bg_session() -> None: logger.warning("Graceful shutdown timed out — force exiting") print("👻 Goodbye!") - # Kill any kiro-cli processes that survived graceful shutdown - cleanup_orphaned_sessions() + # Kill any kiro-cli processes that survived graceful shutdown. + # Off-loop: still inside the orchestrator coroutine here. + # + # The one call site that asks for the leaders narrowing. Nothing spawns a + # session by this point, so the sweep is not racing a mapping publisher -- + # the same position the sweep already held here before this change. + await asyncio.to_thread(cleanup_orphaned_sessions) # This is a hard exit too: os._exit skips atexit, so the log queue's # drain hook never runs here either. Without this the whole shutdown # tail is lost -- including the "Graceful shutdown timed out" warning diff --git a/test/test_pid_lifecycle.py b/test/test_pid_lifecycle.py index 1f107bd82aa..3ccafcf85e0 100644 --- a/test/test_pid_lifecycle.py +++ b/test/test_pid_lifecycle.py @@ -7,6 +7,7 @@ import signal import subprocess import sys +import threading from collections import deque from collections.abc import Iterator from pathlib import Path @@ -366,6 +367,145 @@ def unlink_that_fails_on_bad(path_self, *a, **kw): assert (tmp_path / "session_pid_bad!name.txt").exists() assert not (tmp_path / "session_pid_99999.txt").exists() + @pytest.mark.skipif(sys.platform != "linux", reason="tids share the pid space on Linux only") + def test_pid_file_recycled_as_a_thread_is_deleted( + self, tmp_path: Path, session_pid_file: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A mapping whose pid now names a THREAD of a live process is stale. + + Linux draws tids from the pid space and lets you signal one, so such a + pid passes ``pid_exists`` and such a mapping would survive forever. A + token-bearing mapping is already safe to resolve — ``_pid_recycled`` + refuses on a start-token mismatch, and a tid's token cannot match — so + what is pruned here is the legacy token-less form, which has no recorded + token for that guard to compare, plus the accumulation itself. + + Uses a real live thread's native tid rather than a fake ``/proc``, so + the test exercises the same kernel behaviour that produced the bug. + """ + from kiro_crew.session_pid import cleanup_orphaned_sessions + + monkeypatch.setattr("kiro_crew.session_pid.config_dir", lambda: tmp_path) + session_pid_file.write_text("") + + tid_box: dict[str, int] = {} + release = threading.Event() + captured = threading.Event() + + def _hold() -> None: + tid_box["tid"] = threading.get_native_id() + captured.set() + release.wait(timeout=30) + + holder = threading.Thread(target=_hold, daemon=True) + holder.start() + assert captured.wait(timeout=30), "helper thread never reported its tid" + tid = tid_box["tid"] + assert tid != os.getpid(), "native_id must differ from the group leader" + + try: + thread_map = tmp_path / f"session_pid_{tid}.txt" + leader_map = tmp_path / f"session_pid_{os.getpid()}.txt" + thread_map.write_text("sess-recycled-as-thread") + leader_map.write_text("sess-live-leader") + + # NOT patching os.kill: both pids are genuinely signalable here, + # which is exactly the condition the old predicate could not split. + with patch("kiro_crew.session_pid._cleanup_orphaned_mcp_servers", return_value=0): + cleanup_orphaned_sessions() + + assert not thread_map.exists(), "a pid that is only a thread must be pruned" + assert leader_map.exists(), "a live thread-group leader must be retained" + finally: + release.set() + holder.join(timeout=30) + + def test_boot_setting_reads_no_proc( + self, tmp_path: Path, session_pid_file: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """``narrow_with_leaders=False`` must not take the leaders snapshot. + + The gateway boot path passes it so the sweep costs exactly what it cost + before this branch: ``no-new-work-on-gateway-boot-path`` names orphan + sweeps, so a regression that read the leaders set anyway would put a + ``/proc`` scan back on the boot path, where the readiness cost of it is + not visible to anyone reading the sweep. + """ + from kiro_crew.session_pid import cleanup_orphaned_sessions + + monkeypatch.setattr("kiro_crew.session_pid.config_dir", lambda: tmp_path) + session_pid_file.write_text("") + + def _refuse() -> set[int] | None: + raise AssertionError("the boot setting must not read /proc for leaders") + + monkeypatch.setattr( + "kiro_crew.session_pid.platform_compat.live_thread_group_leaders", _refuse + ) + with patch("kiro_crew.session_pid._cleanup_orphaned_mcp_servers", return_value=0): + cleanup_orphaned_sessions(narrow_with_leaders=False) + + def test_prune_pass_leaves_the_shared_pid_file_alone( + self, tmp_path: Path, session_pid_file: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The deferred pass must not rewrite the file the boot sweep owns. + + Deferring the prune past readiness is only sound because this pass touches + ``session_pid_.txt`` mappings and nothing else. If it also rewrote + ``kiro_session_pids.txt`` it would race the spawns that append to it once + the gateway is serving, and a lost entry is an unkillable orphan. + """ + from kiro_crew.session_pid import _prune_stale_session_pid_files + + monkeypatch.setattr("kiro_crew.session_pid.config_dir", lambda: tmp_path) + session_pid_file.write_text("111:222\n") + (tmp_path / "session_pid_99999.txt").write_text("sess-dead") + + # The probe is pinned, not assumed: ``pid_max`` is 4194304 here, so 99999 + # is an ordinary live pid on a host whose counter has passed it, and a live + # pid is retained -- which would fail the removal assertion below on a + # long-running runner rather than in review. The sibling sweeps above pin + # it the same way; ``os.kill`` is what ``platform_compat.pid_exists`` + # reaches for on POSIX. + with patch("os.kill", side_effect=ProcessLookupError): + removed = _prune_stale_session_pid_files() + + assert removed == 1 + assert not (tmp_path / "session_pid_99999.txt").exists() + assert session_pid_file.read_text() == "111:222\n" + + def test_stale_snapshot_does_not_delete_a_live_mapping( + self, tmp_path: Path, session_pid_file: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A pid recycled after the snapshot keeps the mapping its new owner wrote. + + The leaders snapshot is read once for the whole pass, so a pid that became + a live process after it was taken is absent from it while naming a LIVE + session whose mapping already sits at that path. Deciding on the snapshot + alone unlinks that live mapping, which is a lost session identity, not a + tidy-up; the per-pid re-read is what refuses. The shipped call sites do not + run this pass beside live sessions, so this is defence in depth rather than + load-bearing -- it keeps the guarantee a property of the function instead of + of where it happens to be called from. + """ + from kiro_crew.session_pid import _prune_stale_session_pid_files + + monkeypatch.setattr("kiro_crew.session_pid.config_dir", lambda: tmp_path) + live = tmp_path / f"session_pid_{os.getpid()}.txt" + live.write_text("sess-published-after-the-snapshot") + + # A snapshot from before this process existed: the pid is signalable and + # is a real thread-group leader, yet absent from the set. + monkeypatch.setattr( + "kiro_crew.session_pid.platform_compat.live_thread_group_leaders", + lambda: frozenset({1}), + ) + + removed = _prune_stale_session_pid_files() + + assert removed == 0 + assert live.exists(), "a live leader absent from a stale snapshot must be retained" + class TestResetStateUntracksParentPid: def test_reset_state_untracks_parent_pid(self) -> None: diff --git a/test/test_platform_compat.py b/test/test_platform_compat.py index 7e4409a4af2..23763a09373 100644 --- a/test/test_platform_compat.py +++ b/test/test_platform_compat.py @@ -4565,3 +4565,122 @@ def test_no_lock_site_opens_truncating(self): "lock files opened truncating before the acquire (GH-9248); " "use platform_compat.open_lock_file: " + ", ".join(offenders) ) + + +class TestLiveThreadGroupLeaders: + """``live_thread_group_leaders`` narrows liveness; it must fail OPEN.""" + + @pytest.mark.skipif(sys.platform != "linux", reason="/proc lists leaders on Linux only") + def test_own_process_is_a_leader(self): + leaders = pc.live_thread_group_leaders() + assert leaders is not None + assert os.getpid() in leaders + + @pytest.mark.skipif(sys.platform != "linux", reason="tids share the pid space on Linux only") + def test_a_live_thread_is_not_a_leader(self): + """The whole point: a tid is signalable and has /proc, but is not a process. + + Uses a real thread's native id rather than a synthetic ``/proc`` so the + assertion rests on kernel behaviour, not on a fixture's idea of it. + """ + box: dict[str, int] = {} + captured = threading.Event() + release = threading.Event() + + def _hold() -> None: + box["tid"] = threading.get_native_id() + captured.set() + release.wait(timeout=30) + + holder = threading.Thread(target=_hold, daemon=True) + holder.start() + try: + assert captured.wait(timeout=30) + tid = box["tid"] + assert tid != os.getpid() + # Signalable and openable under /proc — the two things a naive check reads. + assert pc.pid_exists(tid) is True + leaders = pc.live_thread_group_leaders() + assert leaders is not None + assert tid not in leaders, "a non-leader tid must not appear in the /proc listing" + assert os.getpid() in leaders, "its group leader must still appear" + finally: + release.set() + holder.join(timeout=30) + + def test_non_linux_fails_open(self, monkeypatch): + """Off Linux the question is unanswerable, so never claim 'thread'.""" + monkeypatch.setattr(pc, "IS_LINUX", False) + assert pc.live_thread_group_leaders() is None + + def test_unreadable_proc_fails_open(self, monkeypatch): + """An OSError reading /proc yields None (retain), never an empty set.""" + monkeypatch.setattr(pc, "IS_LINUX", True) + + def _boom(*_args, **_kwargs): + raise OSError("permission denied") + + monkeypatch.setattr(pc.os, "listdir", _boom) + assert pc.live_thread_group_leaders() is None + + def test_numberless_proc_fails_open(self, monkeypatch): + """A listing with no pids is nonsense, not 'every recorded pid is a thread'.""" + monkeypatch.setattr(pc, "IS_LINUX", True) + monkeypatch.setattr(pc.os, "listdir", lambda *_a, **_k: ["cpuinfo", "meminfo", "self"]) + assert pc.live_thread_group_leaders() is None + + +class TestIsThreadGroupLeader: + """The per-pid re-read, for a pid a host-wide snapshot answers wrongly.""" + + @pytest.mark.skipif(sys.platform != "linux", reason="/proc carries Tgid on Linux only") + def test_own_process_is_a_leader(self): + assert pc.is_thread_group_leader(os.getpid()) is True + + @pytest.mark.skipif(sys.platform != "linux", reason="tids share the pid space on Linux only") + def test_a_live_thread_is_not_a_leader(self): + """The whole point: a signalable tid must answer False, not None.""" + box: dict[str, int] = {} + captured = threading.Event() + release = threading.Event() + + def _hold() -> None: + box["tid"] = threading.get_native_id() + captured.set() + release.wait(timeout=30) + + holder = threading.Thread(target=_hold, daemon=True) + holder.start() + try: + assert captured.wait(timeout=30), "helper thread never reported its tid" + tid = box["tid"] + assert tid != os.getpid() + assert pc.pid_exists(tid) is True, "a tid is signalable -- that is the trap" + assert pc.is_thread_group_leader(tid) is False + finally: + release.set() + holder.join(timeout=30) + + def test_non_linux_is_unknowable(self, monkeypatch): + monkeypatch.setattr(pc, "IS_LINUX", False) + assert pc.is_thread_group_leader(os.getpid()) is None + + def test_missing_status_is_unknowable(self, monkeypatch): + """A pid that has gone must not read as 'not a process'.""" + monkeypatch.setattr(pc, "IS_LINUX", True) + + def _boom(*_a, **_k): + raise FileNotFoundError("no such pid") + + monkeypatch.setattr("builtins.open", _boom) + assert pc.is_thread_group_leader(4242) is None + + def test_malformed_status_is_unknowable(self, monkeypatch): + """A status file with no parsable Tgid answers None, never False.""" + import io + + monkeypatch.setattr(pc, "IS_LINUX", True) + monkeypatch.setattr( + "builtins.open", lambda *_a, **_k: io.StringIO("Name:\tx\nTgid:\tnotanumber\n") + ) + assert pc.is_thread_group_leader(4242) is None