diff --git a/CHANGELOG.md b/CHANGELOG.md index b711d728..39fb0006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,8 @@ breaking changes may land in a minor release. ### Changed +- Document the live-session removal guard's measured ceiling (#732): `delete`, `archive` and `clean` still remove a run directory when a listing omits a live session. Behavior unchanged; the psmux half is reported upstream (psmux/psmux#622), its retirement tracked in #754. + - **psmux sessions now live in a per-project registry** (#537). bmad-loop points `PSMUX_DATA_DIR` at `//_mux`, so a prune in one project cannot address another's servers at all. A bare `psmux ls` no longer shows them — `bmad-loop mux` prints the @@ -180,6 +182,7 @@ breaking changes may land in a minor release. ### Fixed +- Tell a failed window listing apart from an empty session: liveness raises when the failure is unproven, metadata keeps its sentinel and warns (#525). - Anchor the TUI's paused-spec read and its `Request replan` write on the tree the run owns. Under isolation both resolved against the main checkout, so the review modals showed that copy of the spec and the replan reset it — reporting success while the run's diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 31735b18..e9d79595 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -288,7 +288,7 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w - `bmad-loop stop ` — stop a live run. The default is a **hard stop**: stop now, abandoning the in-flight item and killing the agent session. `--graceful` instead requests a **graceful stop** — the engine finishes the in-flight item (a story through commit, a sweep bundle through commit, or an in-progress sweep triage — after which no bundles start), then finalizes cleanly and stops as a resumable `stopped` run, suppressing any pending auto-sweeps; `--cancel-graceful` withdraws a pending request. Both modes ride the same `stop-request.json` control file, which carries the mode: a hard stop lodges `mode: "hard"` **before** it signals, and that atomic, project-confined write is also what supersedes a pending graceful request. The engine honors a hard request at the next item boundary and mid-session, where each adapter's wait loop polls it twice per iteration — before and after the loop's own up-to-5s wait — so a quiet session normally lands the stop well inside the 10s grace window. That is the common case rather than a bound: an iteration blocked on a transport call, or waiting out `RESULT_GRACE_S` for an artifact, can exceed the window on either adapter before the next poll — an in-flight socket read or tmux call cannot be interrupted from the polling thread, so no placement of the check makes the interval unconditionally short. What the file does guarantee is reach: a hard stop lands on every platform and multiplexer backend, including one where an inter-process signal is never delivered at all (#319). A nested auto-sweep runs inside its parent but mints its own run dir, so it also polls the _owning_ run's channel: stopping the parent stops the child mid-session rather than leaving it to the force-kill backstop. The child's read of the parent channel is hard-only — a graceful stop already keeps a child sweep from starting, and lets one already in flight finish. Teardown is unbounded on top of that — the opencode HTTP adapter then asks the server to abort and to report usage, and a server that will not answer those leaves the stop to the force-kill backstop, exactly as it would have before #319. SIGTERM still goes out alongside it as the POSIX fast path, but it is no longer the mechanism; the engine stays the single writer of `stopped`, and the external force-kill + `run-stop fallback=True` past the grace window now marks a stop this tool had to finish from outside — a teardown that outran the window reaches it as readily as an engine that never read the request — where before #319 it marked every native-Windows stop. The TUI surfaces the same pair: `x` hard-stops, `S` requests a graceful stop. - `bmad-loop delete ` — delete a run directory and its out-of-tree control-plane dir (`--force` stops it first if live). - `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it, control-plane dir included (`--force` stops it first if live). The tarball holds the run dir, so it carries no `events/`. It is staged through an exclusively created temp under a fresh unpredictable name per attempt, so a planted name is never followed or reused, the failure cleanup is provably its own, and a temp stranded by a kill cannot deny later attempts; the tarball is `fsync`ed before the publish — the run dir is removed immediately after, so it is the only remaining copy. A published archive lands at mode `0600` rather than a umask-derived one (#591). -- Removal refuses while a matching agent session is live that the project cannot prove is another one's, even when the engine is dead: for an untagged session the run dir is the last ownership proof `cleanup` can read, so removing it would leak the session ([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)). A session tagged to another project carries its own proof and never blocks. Run `cleanup` first, having confirmed the session is this project's (`attach`): for an _untagged_ session `cleanup` proves ownership by that same run dir, so two projects sharing a run id can prune each other's. Or pass `--force`, which removes anyway and kills nothing. `clean` leaves such a run untouched and reports it as protected. +- Removal refuses while a matching agent session is live that the project cannot prove is another one's, even when the engine is dead: for an untagged session the run dir is the last ownership proof `cleanup` can read, so removing it would leak the session ([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)). A session tagged to another project carries its own proof and never blocks. Run `cleanup` first, having confirmed the session is this project's (`attach`): for an _untagged_ session `cleanup` proves ownership by that same run dir, so two projects sharing a run id can prune each other's. Or pass `--force`, which removes anyway and kills nothing. `clean` leaves such a run untouched and reports it as protected. The check is an observation with a measured ceiling ([#732](https://github.com/bmad-code-org/bmad-loop/issues/732)): a listing that omits a live session reads as "no session", so removal proceeds. Two ways to meet it, both measured on psmux 3.3.8 — a `has-session` whose 500 ms connect does not land reaps a live session's registry entry, and until the server's registry maintenance re-writes it `ls` omits that session while exiting 0 with nothing on stderr (that maintenance runs on a nominal 5 s check in the server's own loop, so the window has no hard bound; 1.7 s was one measured sample); and a process whose PATH lacks the multiplexer binary reads every session as absent for as long as that PATH lasts, however long the server keeps running. The first is a successful query with an incomplete answer and the second is a missing binary, and they do not even look alike — a reap takes one entry, so its listing may still name every other session, while a missing binary yields nothing at all — but both reach the guard as the same verdict, this run's name is not in the listing, which is what a genuinely dead session produces too. So it accepts both rather than wedging every removal path behind `--force`. If a removal does strand a session, clear it with `kill-session` on `bmad-loop-` — but neither failure leaves it reachable straight away: restore the multiplexer to PATH first, and after a reap wait for the server's next registry tick to republish the entry, or the target will not resolve. On psmux the sessions live in the per-project registry, so run it with the export `bmad-loop mux` prints. Confirm the session is this project's before killing, since the name carries no project — and note that `attach` is no longer the way to do it: it resolves the run directory first, which the removal just deleted. Attach in the multiplexer directly instead, and for an _untagged_ session accept that the run dir it lost was its only ownership proof, so what is left is the session's own contents and what you know of the run. - `bmad-loop cleanup` — remove leftover tmux artifacts for finished/stopped runs. `--json` emits the sessions and ctl windows removed (or, with `--dry-run`, that would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json). - `bmad-loop clean` — reclaim disk from concluded runs per `[cleanup]`: tear down worktrees a mid-flight stop orphaned, trim heavy `worktrees/` from runs kept for history, archive/delete past the retention window, and sweep orphaned run control-plane dirs from the out-of-tree state root (`--dry-run`, `--keep`, `--retain N`, `--hard`). `--json` emits what was reclaimed (or would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json), with `freed_bytes` a raw integer. - `bmad-loop tui` — the interactive dashboard (`--low-frame-rate` for slow/SSH links). diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index 9aa60036..1325343b 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -216,17 +216,30 @@ def list_window_ids(self, session: str) -> list[str]: that diverges remains usable, but falls back to the ambiguous by-name lookup whenever several kinds share a run id. - Raises :class:`MultiplexerError` if the transport itself fails (timeout / - missing binary): an empty list means "no windows" and must not be - conflated with "couldn't ask" — this op backs the engine's liveness - probe (:meth:`window_alive`).""" + Raises :class:`MultiplexerError` whenever the listing could not be TAKEN + — a transport failure (timeout / missing binary) or a query that failed + without proving the session gone: an empty list means "no windows" and + must not be conflated with "couldn't ask", because this op backs the + engine's liveness probe (:meth:`window_alive`). + + So ``[]`` carries a positive claim, not a shrug: the backend either + listed the session's windows and found none, or established that the + session no longer exists. A backend answering over a server therefore + owes callers a discrimination — a server that errors while its windows + are alive must not answer ``[]`` (#525). Which conditions PROVE absence + is the backend's own question: the exit code alone does not decide it, + and neither does a confirming :meth:`has_session`, whose False is + weaker than it looks (see its note).""" @abstractmethod def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]]: """One tuple per window in ``session``, each holding the requested backend fields in order. Best-effort: returns ``[]`` on a transport - failure (unlike :meth:`list_window_ids`, this is metadata, not a liveness - probe, so a sentinel is safe). + failure OR a failed query (unlike :meth:`list_window_ids`, this is + metadata, not a liveness probe, so a sentinel is safe — the answer + degrades toward doing nothing, never toward claiming a death or a + kill). A backend SHOULD still say on stderr when the failure did not + prove the session gone, so an every-call failure is not silent (#525). A ``window_id`` column carries the same id form :meth:`current_window_id` AND :meth:`list_window_ids` return; core compares all three directly. The @@ -241,10 +254,13 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] def window_alive(self, session: str, window_id: str) -> bool: """True iff ``window_id`` is still a window of ``session``. - May raise :class:`MultiplexerError` when liveness is unknowable (a - transport timeout / missing binary) — callers must treat that as "don't - know", not "dead", and must not tear down a possibly-working session on - it.""" + May raise :class:`MultiplexerError` when liveness is unknowable — a + transport timeout / missing binary, or any other failure to take the + listing this membership test reads (see :meth:`list_window_ids`). + Callers must treat that as "don't know", not "dead", and must not tear + down a possibly-working session on it. Reachable in ordinary operation, + not only under a hung binary: a live server that refuses or drops the + query answers here (#525).""" @abstractmethod def kill_window(self, target: str) -> None: diff --git a/src/bmad_loop/adapters/psmux_backend.py b/src/bmad_loop/adapters/psmux_backend.py index 4b741867..bf336de2 100644 --- a/src/bmad_loop/adapters/psmux_backend.py +++ b/src/bmad_loop/adapters/psmux_backend.py @@ -555,6 +555,17 @@ def list_window_ids(self, session: str) -> list[str]: # psmux's list-windows emits bare `@N` lines; qualify them identically # to new_window or window_alive's membership check (native_id in # list_window_ids) would read every window as dead. + # + # No `_SESSION_GONE_STDERR` override, and that is a measurement rather + # than an omission (#525). psmux 3.3.8 words a vanished session as + # `psmux: no server running on session ''` — the base's fragment + # matches it — and its client-side variant carries `can't find session` + # too. The failures that must NOT read as gone are worded well clear of + # both: a live session whose key was rejected answers `psmux: Invalid + # session key`, and one whose server is unreachable answers `psmux: + # connection timed out`. Both were rc 1 with the windows demonstrably + # alive — this backend is where the bug was actually reachable, because + # a per-session TCP server has failure modes tmux's socket does not. return [ self._qualified_window_id(session, window_id) for window_id in super().list_window_ids(session) @@ -594,6 +605,16 @@ def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]] id_columns = {i for i, field in enumerate(fields) if field == "window_id"} if not opt_columns and not id_columns: return rows + if not rows: + # No rows to fill, so the option listing below has nothing to fill + # them WITH — a pure short-circuit, byte-identical output. It is + # load-bearing anyway (#525): the base answers [] both for a session + # it proved gone and for a spawn that never landed, silently in each + # case, and pressing on would spend a second probe and then warn that + # the option listing failed — about a session that is legitimately + # gone, or on a box with no multiplexer at all. The base's honest + # silences must not be re-broken by the wrapper that reads them. + return rows # The #221 degrade: an empty or `:`-bearing session cannot be routed # with `-t`, and an unrouted read would answer from whichever server # the fallback picks — fill "" without issuing reads at all. @@ -901,7 +922,12 @@ def _scoped_options(self, session: str) -> dict[str, str] | None: surprising {} is possible and is not proof that no keys are set.""" try: proc = self._run(["show-options", "-q", "-t", session], check=False) - except (subprocess.SubprocessError, OSError): + except (subprocess.SubprocessError, OSError, UnicodeError): + # UnicodeError as in the base's listings (#525): this is the SECOND + # probe of a two-probe read, so a strict-codec leaf that decoded the + # window listing cleanly can still fault here — and the caller above + # is a best-effort metadata op that must degrade to "unset", never + # raise a decode error out of it. return None if proc.returncode != 0: return None @@ -951,10 +977,12 @@ def _sweep_orphan_keys(self, session: str) -> None: # A session being swept just minted a window, so an empty live # list is a failed probe, not an empty session — treating it as # truth would sweep every key, live windows included. Warned for - # the same reason the listing failure above is, and this is the - # branch that actually fires: list_window_ids RAISES on a - # transport fault (caught below) and answers [] only on rc != 0, - # so silence here is a server failing every launch with no signal. + # the same reason the listing failure above is. Since #525 the + # only way to reach here is a listing that PROVED the session + # gone — every other failure raises and lands in the arm below — + # which under a just-minted window means the server died between + # the mint and the sweep. Its keys died with it, so the warning + # is the whole remaining duty. print( f"warning: orphan-key sweep on {session} could not list live " "windows; orphaned keys unswept until the next launch", @@ -978,10 +1006,11 @@ def kill_window(self, target: str) -> None: # (the project tag scopes the prune retry; the return key keeps both # return legs armed, see _parked_trailer). Scope resolves before the kill because a name # token cannot be resolved once the window is dead. An empty liveness - # listing is ambiguous — a failed probe, or a session that died with - # its last window — so it degrades toward retaining the keys; the - # launch-time orphan sweep reclaims them once the window is provably - # gone. Discovery is generic by the seam's marker — the backend must + # listing means the session is gone (#525 narrowed it to that; anything + # unproven raises into the arm below), and its keys went with the + # server — but this path cannot tell that from the pre-#525 reading, so + # it still degrades toward retaining them; the launch-time orphan sweep + # reclaims whatever survived once the window is provably gone. Discovery is generic by the seam's marker — the backend must # not know which option names callers use. Best-effort throughout: # cleanup failure warns (the sweep precedent) but never blocks or # fails the kill. diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index c52500f0..7ce6e7d0 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -6,7 +6,9 @@ the native-Windows :mod:`.psmux_backend` leaf — can subclass :class:`BaseTmuxBackend` and swap only class attributes (:attr:`BaseTmuxBackend._BINARY` for the spawned binary, :attr:`BaseTmuxBackend._ENCODING` / :attr:`BaseTmuxBackend._ERRORS` -for output decoding — a scrubbed +for output decoding, :attr:`BaseTmuxBackend._SESSION_GONE_STDERR` for the stderr +wordings that prove a session is gone rather than a listing merely failed, #525 +— a scrubbed per-call ``env`` is a ``_run`` parameter, and an :meth:`BaseTmuxBackend._run` override is left for timeout tweaks) plus the shell-dialect hooks (``_shell_wrap``, ``_join_argv``, ``_parked_trailer``, ``_source_prefix``, ``_window_launch`` and the @@ -65,6 +67,14 @@ class BaseTmuxBackend(TerminalMultiplexer): #: mid-capture. Honored even where :attr:`_ENCODING` is None, so POSIX keeps #: the locale codec and only stops being strict. A leaf may still override. _ERRORS: str | None = "backslashreplace" + #: stderr fragments (matched case-insensitively, as substrings) that PROVE + #: the target session is gone, as opposed to a listing that merely failed. + #: This is the whole discrimination rule behind :meth:`_session_proved_gone` + #: — see it for why the answer has to be stderr and why the tuple is narrow. + #: A tmux-family leaf whose multiplexer words this differently overrides the + #: tuple; psmux deliberately does not (its own wordings are covered — see + #: the note in :mod:`.psmux_backend`). + _SESSION_GONE_STDERR: tuple[str, ...] = ("no server running", "can't find session") #: Diagnostic from the last :meth:`version` probe (see #: :meth:`TerminalMultiplexer.version_error`). A class-level default so an #: instance that never probed answers None instead of AttributeError. @@ -109,6 +119,99 @@ def _run( raise TmuxError(f"{self._BINARY} {' '.join(argv[:2])} failed: {proc.stderr.strip()}") return proc + def _session_proved_gone(self, proc: subprocess.CompletedProcess[str]) -> bool: + """Whether a non-zero listing exit PROVES the session no longer exists. + + The discrimination the seam's ``[]`` rests on (#525). A non-zero exit + covers two unrelated answers: the session is gone (an ordinary, + knowable fact) and the listing could not be taken at all (a server that + errored while its windows are alive, a rejected auth, an unreachable + port). Folding both to ``[]`` reports the second as "this session has + no windows" — a death the engine acts on and a kill the prune reports + as verified. + + Only stderr can tell them apart: the exit CODE cannot (both multiplexers + exit 1 for everything), and a confirming ``has_session`` round trip + cannot either — that predicate folds every non-zero exit to False by its + own documented contract, so it answers "gone" for exactly the auth and + timeout faults this discrimination exists to catch. + + The tuple is an ALLOWLIST, deliberately: an unrecognized wording is + unknowable, never proof. The failure mode of a too-narrow tuple (a + vanished session read as unverifiable) is loud and self-clearing; a + too-wide one silently restores the bug. Measured rather than assumed, + on tmux 3.4 and psmux 3.3.8 — see the ``_SESSION_GONE_STDERR`` rows in + tests/test_multiplexer.py, which carry the transcript. Neither binary + localizes these strings, so a case-insensitive substring match is enough. + + Both operands are folded, not just the captured text: the case-folding is + a promise to the OVERRIDING leaf, and folding one side only would honor it + for the base's own (already lower-case) tuple while silently breaking a + leaf that spelled its fragment the way its binary prints it. A BLANK + fragment is dropped rather than matched: ``"" in err`` is true for every + error there is, and ``" " in err`` for very nearly as many — any message + with a space in it. Either is #525 restored in full and in silence, from + an authoring slip (a trailing comma, a fragment built from config) that + no reviewer would see, so the two slips that could reintroduce the bug + cannot. The fragment is dropped, not stripped: a leaf that meant a + leading space meant it, and silently re-anchoring its fragment would + substitute a different rule for the one it declared. + """ + err = proc.stderr.lower() + return any(f.lower() in err for f in self._SESSION_GONE_STDERR if f.strip()) + + def _warn_unproven_listing( + self, verb: str, proc: subprocess.CompletedProcess[str] | BaseException + ) -> None: + """Say out loud that a METADATA listing failed for a reason other than the + session being gone (#525). + + The same lens as :meth:`list_window_ids`, the opposite conclusion: these + callers read tags and window rows, not liveness, and their sentinel + degrades toward doing NOTHING — an empty candidate list prunes nothing, + an unread tag reads as untagged and is left alone — so the documented + ``[]`` / ``{}`` stays. What was missing is the signal: a server erroring + on every call made the tool behave as if the sessions it manages had + simply stopped existing, with nothing on stderr to say why. Silent for a + genuinely gone session, which is an answer, not a fault. + + Takes the completed process OR the transport exception that replaced one: + a timeout and a spawn that died prove no more about the session than an + unrecognized non-zero exit does, and the caller's sentinel is the same, + so the signal must be too. + + The one failure left deliberately silent is a multiplexer that is not + installed at all: a box without one has no sessions to report on, so the + absence is an answer rather than a fault (the standing reading, see + :meth:`list_sessions`). That check lives HERE, gating only the + diagnostic, and must not be hoisted into the callers as an early return. + A ``shutil.which`` short-circuit ahead of :meth:`_run` decides the + RETURN VALUE from the ambient PATH, which makes the seam unreachable + through the one spawn primitive — every caller and every test that + injects a transport gets the sentinel no matter what it injected, and + the divergence hides on whichever platform happens to have the binary. + Gating the warning costs one failed exec on a binary-less box and keeps + the answer where the contract says it comes from. + + Warn-only by construction, like every other diagnostic in this module: + under the TUI stderr is captured for the app's whole run (see + ``tui/app.py``), so this is a CLI-visible signal. + """ + if not shutil.which(self._BINARY): + return + if isinstance(proc, BaseException): + outcome, detail = "failed", f"{type(proc).__name__}: {proc}" + else: + if self._session_proved_gone(proc): + return + outcome = f"exited {proc.returncode}" + detail = proc.stderr.strip() or "(no stderr)" + print( + f"warning: {self._BINARY} {verb} {outcome} without proving " + f"the session gone; reading it as empty: {detail}", + file=sys.stderr, + ) + def _tmux(self, *args: str) -> str: # The strict form: a non-zero exit already raises TmuxError inside _run. # A timeout / missing binary escapes _run raw, so trap it here once and @@ -190,9 +293,15 @@ def session_options(self, option: str) -> dict[str, str]: proc = self._run( ["list-sessions", "-F", f"#{{session_name}}\t#{{{option}}}"], check=False ) - except (subprocess.SubprocessError, OSError): + except (subprocess.SubprocessError, OSError, UnicodeError) as exc: + # UnicodeError for the reason list_window_ids names it: a leaf that + # overrides _ERRORS back to a strict handler raises a ValueError-family + # decode error neither other arm covers, and this method's contract is + # warn-and-sentinel, never a raise. + self._warn_unproven_listing("list-sessions", exc) return {} if proc.returncode != 0: # no server / no sessions + self._warn_unproven_listing("list-sessions", proc) return {} options: dict[str, str] = {} for line in proc.stdout.splitlines(): @@ -318,11 +427,15 @@ def list_window_ids(self, session: str) -> list[str]: # display-message -t exits 0 with empty output, so list the # session's window ids and check membership instead. # - # A transport failure (timeout / missing binary) must RAISE, not return []. - # window_alive() is the engine's liveness probe; a sentinel [] would falsely - # read as "window dead -> session crashed" on a mere tmux hang. The honest - # answer to "is it alive?" is "unknowable" -> MultiplexerError. A real dead - # window still returns [] via the returncode != 0 path below (no exception). + # A failed listing must RAISE, not return []. window_alive() is the engine's + # liveness probe; a sentinel [] would falsely read as "window dead -> session + # crashed". The honest answer to "is it alive?" is "unknowable" -> + # MultiplexerError. That covers the transport failures below (timeout / + # missing binary / decode) AND a non-zero exit that does not prove the + # session is gone (#525) — see _session_proved_gone for the discrimination + # and why an exit code alone cannot make it. A session that really vanished + # still returns [] (no exception): the prune must be able to report its + # kills as removed rather than invent a phantom survivor. # # UnicodeError is a transport failure too. _run no longer decodes strictly # on any platform (_ERRORS is backslashreplace, #380), so this arm is now @@ -339,7 +452,12 @@ def list_window_ids(self, session: str) -> list[str]: except (subprocess.TimeoutExpired, OSError, UnicodeError) as exc: raise TmuxError(f"{self._BINARY} list-windows failed: {exc}") from exc if probe.returncode != 0: - return [] + if self._session_proved_gone(probe): + return [] + raise TmuxError( + f"{self._BINARY} list-windows on {session} exited {probe.returncode} " + f"without proving the session gone: {probe.stderr.strip() or '(no stderr)'}" + ) return probe.stdout.split() def pipe_pane(self, window_id: str, log_file: Path) -> None: @@ -447,12 +565,26 @@ def window_pane_pids(self, target: str) -> list[int]: return [] def list_windows(self, session: str, fields: list[str]) -> list[tuple[str, ...]]: + # No missing-binary pre-gate here, deliberately, unlike list_sessions / + # session_options: those two answer "what exists" and may decide that from + # the ambient PATH, but this one is reached with a session already in hand + # and its answer must come from _run — the one spawn primitive the seam + # documents. A `shutil.which` short-circuit ahead of it returns the + # sentinel without consulting the transport at all, so an injected _run is + # never asked, and the behavior silently diverges by whether the binary + # happens to be on THIS box's PATH. The no-multiplexer silence this was + # reaching for is gated inside _warn_unproven_listing instead, where it + # costs a failed exec and nothing else. fmt = "\t".join(f"#{{{field}}}" for field in fields) try: probe = self._run(["list-windows", "-t", f"={session}", "-F", fmt], check=False) - except (subprocess.SubprocessError, OSError): + except (subprocess.SubprocessError, OSError, UnicodeError) as exc: + # UnicodeError as in session_options: a strict-codec leaf must get the + # documented sentinel, not a raw decode error out of a best-effort op. + self._warn_unproven_listing("list-windows", exc) return [] if probe.returncode != 0: + self._warn_unproven_listing("list-windows", probe) return [] rows: list[tuple[str, ...]] = [] for line in probe.stdout.splitlines(): diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 0404ba41..4f9ffe57 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -2350,10 +2350,72 @@ def live_session_may_be_ours(project: Path, run_id: str) -> bool: reaped port file on a 5 s tick, source-read at v3.3.8), a binary's PATH presence is per-process while the server is not, and the listing is load-sensitive; so a "proof of absence" either wedges every removal behind - `--force` or quietly accepts a refutable proof. The degrade above is the - guard's owner's documented trade, kept deliberately; the measured cost of - the unobservable-multiplexer window is filed for that owner to revisit - rather than overturned here. + `--force` or quietly accepts a refutable proof. + + Revisited with the cost measured (#732), and the degrade is KEPT — not for + want of a stronger read, but because the more dangerous of the two costs + reaches this function as an ordinary answer. On psmux 3.3.8 a live session's + registry entry is reaped by any `has-session` whose 500 ms connect does not + land, and until the server's registry maintenance re-writes it the listing + does not name that session while reporting no fault at all: exit **0**, no + stderr (measured; the listing empty in that run because the session was the + only one, the server pid alive throughout, the entry back 1.7 s later). + That maintenance runs when the server's own loop notices 5 s have elapsed — + a nominal interval, not a scheduling guarantee, so 1.7 s is a sample and 5 s + is not a bound. The listing therefore does not fail, it answers wrongly — + and the read this guard makes is the one with no discrimination in it at + all. ``mux_sessions()`` lands on ``BaseTmuxBackend.list_sessions``, which + returns ``[]`` under the three conditions its own comment names — the binary + is missing, no server is running, or the query itself fails — and says + nothing about which. None of the three raises: the bundled backend folds + ``SubprocessError`` and ``OSError`` into that same sentinel, and only an + out-of-tree backend raises ``MultiplexerError`` here. Nor does the + diagnostic #525 added: ``_warn_unproven_listing`` sits on + ``session_options`` and the window path, not on this one. And the reap + reaches none of those branches anyway, because its exit is 0. So there is + neither a signal to condition on nor a word on stderr about it. + + The second cost lasts as long as its cause: a process whose PATH lacks the + binary reads every session as absent for as long as that PATH does (measured + through the seam — the sessions reappear when PATH is restored), because the + binary is per-process while the server is not. That one is no mystery to the + transport: ``list_sessions`` decides it from ``shutil.which`` on purpose, + being one of the two methods that report what exists, and folds it into the + same ``[]`` deliberately (its own comment: the absence of sessions and the + absence of the multiplexer are indistinguishable here). The fold is the + seam's decision; what reaches this function is its result. + + What the two costs share is not a shape but a verdict. A reap takes one + session's entry, so its listing may still name every other session on the + box; a missing binary yields nothing at all. Both arrive as the same + *negative membership* — this run's name is not in the listing — which is + exactly what a genuinely dead session produces, and there is no fourth + answer to distinguish them by. + + The named alternatives were weighed and refused. Retry-with-delay, in the + shape this guard could hold, pays a delay on every removal that finds no + session — the common case — once per run inside `clean`; a retry hoisted to + command scope would pay it once, but neither can bound a window whose close + is a nominal tick rather than a deadline. A tag-first read draws on the same + listing and is blind in exactly the same window. Warning whenever the + multiplexer is unusable fires on every box that simply has no multiplexer, + which is the noise ``BaseTmuxBackend._warn_unproven_listing`` already stays + silent about. What is left is the accepted ceiling, written down here and in + docs/FEATURES.md so an operator meets it as a documented limit rather than a + surprise. + + The psmux half is a *defect*, not a design: four sibling reap sites in that + binary gate on `ConnectionRefused` precisely because "a timeout means + busy-but-alive and must not be deleted" (its own comment), the `has-session` + site does not, and tmux 3.4 blocks rather than reaps under the same + treatment (measured). Reported as psmux/psmux#622, which is the thing to + watch. Retiring it takes two parties, though, and an upstream fix alone is + not enough: the reap window closes for this project once a psmux release + repairs that site AND the supported floor is raised to that release or + later, since an installed 3.3.8 keeps reaping whatever master does. And it + retires only itself — the PATH case is ours, and so are the two arms above + it, a backend that cannot be selected and a listing that raises. What that + fix removes is one measured case, not the ceiling. Two registry-root-era additions on that unchanged contract: diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index b9ae3efc..a1bcdfb9 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -749,19 +749,19 @@ def prune_ctl_windows(project: Path) -> tuple[list[str], list[str], list[str]]: set membership either way, so the verdict costs one extra round trip instead of N. A transport fault raises and nothing can be claimed there. - Two ceilings, both deliberate: - - - The membership test pairs list_windows' `window_id` column with - list_window_ids. The seam states its symmetry rules pairwise and this pair - is stated because of THIS caller — a backend qualifying one side and not - the other reads every candidate as removed, which is #435 restored on the - optimistic side, with no error anywhere. - - `[]` is read as "the session went with its last window". The seam's `[]` - is wider than that: BaseTmuxBackend folds EVERY nonzero exit to `[]`, so a - server that errors while its windows live would report them removed. The - common cause by far is the session really being gone, and pessimism there - would invent a phantom survivor on every future sweep. Narrowing the - sentinel is a change to the engine's liveness probe, not to this function. + One ceiling, deliberate: the membership test pairs list_windows' `window_id` + column with list_window_ids. The seam states its symmetry rules pairwise and + this pair is stated because of THIS caller — a backend qualifying one side + and not the other reads every candidate as removed, which is #435 restored + on the optimistic side, with no error anywhere. + + `[]` is read as "the session went with its last window", and since #525 the + seam means exactly that: a listing that merely FAILED raises instead of + folding to `[]`, so it lands in `unverifiable` below rather than reporting + every candidate removed. The narrow reading is what keeps the pessimism + honest in the other direction too — a genuinely vanished session still + answers `[]`, so its kills are reported as removed instead of leaving a + phantom survivor for every future sweep to re-report. """ mux = get_multiplexer() candidates = _ctl_window_candidates(project) diff --git a/tests/test_multiplexer.py b/tests/test_multiplexer.py index 500cc71b..81a8ee94 100644 --- a/tests/test_multiplexer.py +++ b/tests/test_multiplexer.py @@ -321,6 +321,265 @@ def test_seam_methods_never_leak_raw_subprocess_error(boom_run, tmp_path): assert mux.current_pane_id() is None +# --------------------------------------------- proved-gone vs unknowable (#525) +# +# The seam's `[]` is a positive claim: the backend listed the session and found +# no windows, OR established that the session is gone. Every OTHER non-zero exit +# is a listing that could not be taken, and answering `[]` there reports a live +# session as empty — a death the engine acts on, a kill the prune calls verified. +# +# The rows below are a TRANSCRIPT, not invented fixtures. Measured 2026-08-31 +# against the real binaries, tmux 3.4 (WSL) and psmux 3.3.8 (66cf613): +# +# tmux list-windows -t =ghost rc 1 "can't find session: ghost" +# tmux list-windows, no server rc 1 "no server running on /tmp/tmux-1000/default" +# psmux list-windows -t =ghost rc 1 "psmux: no server running on session 'ghost'" +# psmux, tampered .key, WINDOWS ALIVE rc 1 "psmux: Invalid session key" +# psmux, wrong .port, WINDOWS ALIVE rc 1 "psmux: connection timed out" +# +# The last two are why an exit code cannot make this call: identical rc, live +# windows. A rejected `-F` format is NOT on the list because neither binary +# fails on one — both exit 0 (tmux prints nothing, psmux echoes the literal), so +# there is no "rejected format" arm to discriminate. + +_PROVED_GONE_STDERR = [ + "can't find session: ghost", + "no server running on /tmp/tmux-1000/default", + "psmux: no server running on session 'ghost'", +] +_UNPROVEN_STDERR = [ + "psmux: Invalid session key", + "psmux: connection timed out", + "psmux: no response from server (timed out)", + "", # a non-zero exit that said nothing at all proves nothing at all +] + + +def _failing_listing(monkeypatch, stderr: str) -> None: + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: "/usr/bin/tmux") + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **_k: subprocess.CompletedProcess(argv, 1, stdout="", stderr=stderr), + ) + + +@pytest.mark.parametrize("stderr", _PROVED_GONE_STDERR) +def test_list_window_ids_answers_empty_when_the_session_is_proved_gone(monkeypatch, stderr): + """A vanished session is an ANSWER, not a fault: `[]`, no exception. + + Raising here instead would be the same dishonest report from the other + side — prune_ctl_windows would invent a phantom survivor that every + subsequent cleanup re-reports and nothing ever clears.""" + _failing_listing(monkeypatch, stderr) + assert TmuxMultiplexer().list_window_ids("s") == [] + assert TmuxMultiplexer().window_alive("s", "@1") is False + + +@pytest.mark.parametrize("stderr", _UNPROVEN_STDERR) +def test_list_window_ids_raises_when_a_nonzero_exit_proves_nothing(monkeypatch, stderr): + """The bug (#525): a listing that failed while the windows are alive must not + read as an empty session. + + Both live-window rows come from a psmux server that was still serving its + windows — one refused the auth, one was unreachable — and folding either to + `[]` tells the engine's liveness probe the window died and the prune that + its kills are verified. + + Ablation: replace the `_session_proved_gone` guard with a bare `return []` + and every row here fails, while the proved-gone sibling above still passes. + """ + _failing_listing(monkeypatch, stderr) + with pytest.raises(MultiplexerError): + TmuxMultiplexer().list_window_ids("s") + with pytest.raises(MultiplexerError): + TmuxMultiplexer().window_alive("s", "@1") + + +def test_gone_stderr_match_is_case_insensitive_and_substring(monkeypatch): + """The fragments are matched inside a longer line and without regard to case: + every measured wording embeds them in a sentence (`psmux: no server running + on session 'x'`), and a backend that capitalizes its first word must not + thereby turn a vanished session into an unverifiable one.""" + _failing_listing(monkeypatch, "PSMUX: No Server Running on session 'ghost'\n") + assert TmuxMultiplexer().list_window_ids("s") == [] + + +def test_a_leaf_may_override_the_proved_gone_wordings(monkeypatch): + """The rule is a class attribute so a tmux-family leaf whose multiplexer words + absence differently can replace it without touching a method body — the same + swap-a-class-attr contract as `_BINARY` / `_ENCODING`. + + psmux deliberately does NOT override (its measured wordings are covered by + the base tuple); this locks the seam open for the leaf that isn't.""" + + class Dialect(TmuxMultiplexer): + # Mixed case ON THE FRAGMENT, deliberately: the case-folding is a promise + # to the leaf, and folding only the captured text keeps the base's own + # (lower-case) tuple working while silently breaking every override that + # spelled its fragment the way its binary prints it — a vanished session + # read as unverifiable, i.e. a phantom survivor forever. + _SESSION_GONE_STDERR = ("Session Vanished",) + + _failing_listing(monkeypatch, "the session vanished, sorry") + assert Dialect().list_window_ids("s") == [] + # ...and the base's own wordings stop counting once replaced + _failing_listing(monkeypatch, "can't find session: ghost") + with pytest.raises(MultiplexerError): + Dialect().list_window_ids("s") + + +@pytest.mark.parametrize("blank", ["", " ", "\t", "\n"]) +def test_a_blank_gone_fragment_never_matches(monkeypatch, blank): + """A blank fragment in the tuple must be dropped, not matched. + + `"" in err` is true for every error there is, and `" " in err` for very + nearly as many — any message with a space in it, which is all of them. So + one stray element (a trailing comma, a leaf building its tuple from config) + folds EVERY failed listing back to `[]`: #525 restored in full, in the + silent direction, from a slip no reviewer would see. + + The whitespace rows are the point — an emptiness check that only rejects + `""` leaves the more likely slip live. Ablation: weaken `if f.strip()` to + `if f` and the space and newline rows fail. The tab row passes either way, + because no message these binaries emit contains one; it is here to pin the + rule, not to drive the ablation. The stderr below keeps its trailing newline + for the same reason the wordings are verbatim — that is how a real binary + hands it over, and it is what makes the `"\\n"` row a live threat rather than + a hypothetical one.""" + + class Sloppy(TmuxMultiplexer): + _SESSION_GONE_STDERR = ("no server running", blank) + + _failing_listing(monkeypatch, "psmux: Invalid session key\n") + with pytest.raises(MultiplexerError): + Sloppy().list_window_ids("s") + # the real sibling still counts — the filter drops the element, not the rule + _failing_listing(monkeypatch, "no server running on /tmp/tmux-1000/default") + assert Sloppy().list_window_ids("s") == [] + + +@pytest.mark.parametrize("stderr", _UNPROVEN_STDERR) +def test_metadata_listings_keep_their_sentinel_but_say_so(monkeypatch, capsys, stderr): + """Same lens, opposite conclusion (#525). `list_windows` and `session_options` + read metadata, not liveness, and their sentinel degrades toward doing + NOTHING — an empty candidate list prunes nothing, an unread tag reads as + untagged and is left alone — so the documented `[]` / `{}` stays. + + What was missing is the signal: a server erroring on every call made the + tool behave as if the sessions it manages had stopped existing, silently.""" + _failing_listing(monkeypatch, stderr) + mux = TmuxMultiplexer() + assert mux.list_windows("s", ["window_id"]) == [] + assert mux.session_options("@opt") == {} + err = capsys.readouterr().err + assert err.count("without proving the session gone") == 2 + # A non-zero exit that said NOTHING proves nothing either, and the warning has + # to survive having no detail to quote — the row this parametrization used to + # drop, under which a silent-on-blank-stderr regression passed. + assert (stderr.strip() or "(no stderr)") in err + + +def test_metadata_listings_warn_when_the_transport_itself_failed(boom_run, capsys): + """A timeout / a spawn that died proves no more about the session than an + unrecognized non-zero exit, and the sentinel is identical — so the signal has + to be too, or the seam's "say when the failure proved nothing" is only half + true and the quietest failures are the ones that stay quiet. + + The `shutil.which` pre-gate is the deliberate exception and is covered by + test_seam_methods_never_leak_raw_subprocess_error's no-binary case: a box + with no multiplexer has no sessions to report on.""" + mux = TmuxMultiplexer() + assert mux.list_windows("s", ["window_id"]) == [] + assert mux.session_options("@opt") == {} + err = capsys.readouterr().err + assert err.count("without proving the session gone") == 2 + assert type(boom_run).__name__ in err + + +def test_metadata_listings_contain_a_decode_fault(monkeypatch, capsys): + """A strict-codec leaf must get the documented sentinel here too, not a raw + UnicodeDecodeError out of a method the seam calls best-effort. + + `list_window_ids` has named this arm since #380 — a leaf overriding `_ERRORS` + back to a strict handler raises a ValueError-family decode error that neither + `SubprocessError` nor `OSError` covers. The metadata siblings promise `[]` / + `{}` on a transport failure and a decode fault is one, so the same arm belongs + here; without it the promise holds for every failure except this one.""" + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: "/usr/bin/tmux") + + def boom(*_a, **_k): + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte") + + monkeypatch.setattr(tmux_base.subprocess, "run", boom) + mux = TmuxMultiplexer() + assert mux.list_windows("s", ["window_id"]) == [] + assert mux.session_options("@opt") == {} + assert capsys.readouterr().err.count("without proving the session gone") == 2 + + +def test_metadata_listings_are_silent_without_a_binary(monkeypatch, capsys): + """No multiplexer installed is an ANSWER, not a fault: both metadata listings + answer their sentinel and say nothing. + + Uniformity is the assertion. A sibling that warned here would fire on every + call on such a box while the other stayed quiet, and a diagnostic that fires + for one method and not its twin teaches the reader to ignore it. + + Silent, NOT spawnless, and the difference is the whole point. The silence is + gated inside `_warn_unproven_listing`, after `_run` has been asked; gating it + by short-circuiting ahead of `_run` would decide the RETURN VALUE from the + ambient PATH, which makes the seam unreachable through its own spawn + primitive — the injected transport below would never be consulted, and the + resulting behavior would differ by platform rather than by contract. That + was a real regression: it left `list_windows` answering `[]` for every + psmux test on Linux, where the binary does not exist, while staying green on + Windows, where it does. + + Ablation: drop the `shutil.which` guard in `_warn_unproven_listing` and this + fails on the two warnings it must not print.""" + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: None) + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **_k: (_ for _ in ()).throw(FileNotFoundError(argv[0])), + ) + mux = TmuxMultiplexer() + assert mux.list_windows("s", ["window_id"]) == [] + assert mux.session_options("@opt") == {} + assert capsys.readouterr().err == "" + + +def test_list_windows_answer_comes_from_run_not_from_path(monkeypatch): + """`list_windows` must consult `_run` even when the binary is absent from PATH. + + The seam documents `_run` as the ONE place a spawn happens and the source of + every answer. A `shutil.which` short-circuit ahead of it silently substitutes + the ambient PATH for the transport, so a caller (or a test) that injects a + transport is never asked — which is exactly how the same suite passed on + Windows and failed 15 ways on Linux. + + Ablation: reinstate an early `if not shutil.which(...): return []` in + `list_windows` and this fails on the injected rows never arriving.""" + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: None) + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **_k: subprocess.CompletedProcess(argv, 0, stdout="@1\tshell\n", stderr=""), + ) + assert TmuxMultiplexer().list_windows("s", ["window_id", "window_name"]) == [("@1", "shell")] + + +def test_metadata_listings_stay_silent_for_a_gone_session(monkeypatch, capsys): + """A vanished session is an answer, not a fault — warning about it would put + noise on the ordinary path (`cleanup` after the last session ends).""" + _failing_listing(monkeypatch, "no server running on /tmp/tmux-1000/default") + mux = TmuxMultiplexer() + assert mux.list_windows("s", ["window_id"]) == [] + assert mux.session_options("@opt") == {} + assert capsys.readouterr().err == "" + + def test_list_window_ids_decode_fault_raises_the_seam_type(monkeypatch): """A byte the codec cannot decode is a transport failure like a timeout: the liveness probe must answer MultiplexerError ("unknowable"), not leak the raw diff --git a/tests/test_psmux_backend.py b/tests/test_psmux_backend.py index a9b17f9c..4008d493 100644 --- a/tests/test_psmux_backend.py +++ b/tests/test_psmux_backend.py @@ -203,6 +203,81 @@ def test_list_windows_id_column_is_findable_in_list_window_ids(monkeypatch): assert all(row[0] in live for row in rows) +def test_list_windows_does_not_probe_options_for_an_empty_listing(monkeypatch, capsys): + """An empty window listing ends the read here — no option probe, no warning. + + The base answers `[]` for a missing binary — after one failed spawn, silently — + and for a session it PROVED gone (#525). Both are honest silences, and this + wrapper is what would re-break them: it fetches the id-keyed options + whenever an `@` column is asked for, so pressing on past an empty listing + spends a second probe and then warns that the option listing failed — on a + box with no multiplexer at all, or about a session that is legitimately + gone. Since there are no rows to fill, the fetch could only ever be waste. + + Ablation: drop the `if not rows` short-circuit and both halves fail — the + gone case gains a show-options spawn plus a warning, the unspawnable case + gains a second doomed spawn and the same stray warning.""" + spawned: list[list[str]] = [] + + def gone(argv, **_kwargs): + spawned.append(argv) + return subprocess.CompletedProcess( + argv, 1, stdout="", stderr="psmux: no server running on session 's'" + ) + + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: "C:/bin/psmux.exe") + monkeypatch.setattr(tmux_base.subprocess, "run", gone) + mux = PsmuxMultiplexer() + assert mux.list_windows("s", ["window_id", "@bmad_project"]) == [] + assert [a[1] for a in spawned] == ["list-windows"] # never show-options + assert capsys.readouterr().err == "" + + # ...and when the binary cannot be spawned at all, one doomed attempt, not + # two: the base returns [] from the failed spawn and this wrapper stops + # there rather than sending a second one after options that cannot exist. + # The transport says so, never PATH — `which` decides only whether the + # failure is worth a warning, so this half holds identically on a box that + # has psmux and one that does not. + spawned.clear() + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: None) + monkeypatch.setattr( + tmux_base.subprocess, + "run", + lambda argv, **_k: spawned.append(argv) or _raise(FileNotFoundError(argv[0])), + ) + assert mux.list_windows("s", ["window_id", "@bmad_project"]) == [] + assert [a[1] for a in spawned] == ["list-windows"] + assert capsys.readouterr().err == "" + + +def _raise(exc: BaseException): + raise exc + + +def test_scoped_options_contains_a_decode_fault(monkeypatch, capsys): + """The SECOND probe of the two-probe read must degrade, not raise. + + A strict-codec leaf can decode the window listing cleanly and still fault on + the option listing, so `_scoped_options` needs the same `UnicodeError` arm + the base's listings carry (#525). Without it a raw `UnicodeDecodeError` + escapes `list_windows` — a method the seam calls best-effort, whose callers + catch nothing. + + Ablation: drop `UnicodeError` from the catch tuple and this fails on the + raw decode error escaping.""" + + def fake(argv, **_kwargs): + if argv[1] == "show-options": + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte") + return subprocess.CompletedProcess(argv, 0, stdout="@1\n", stderr="") + + monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: "C:/bin/psmux.exe") + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + rows = PsmuxMultiplexer().list_windows("s", ["window_id", "@bmad_project"]) + assert rows == [("s:@1", "")] # the option column degrades to "unset" + assert "show-options listing failed" in capsys.readouterr().err + + def test_qualification_degrades_to_bare_on_colon_session(monkeypatch, tmp_path): # A `:` in the session name would split the target at the wrong colon on # replay — both methods degrade to the bare id identically (the #221 rule). @@ -1597,16 +1672,23 @@ def fake(argv, **kwargs): def test_sweep_treats_empty_live_list_as_failed_probe(monkeypatch, tmp_path, capsys): # We just minted a window in this session, so an empty live listing is a # failed probe, not an empty session — believing it would sweep every key, - # live windows included. It is also said out loud: list_window_ids raises on - # a transport fault and answers [] only on rc != 0, so this branch is a - # server failing every launch, and silence would leak keys with no signal. + # live windows included. It is also said out loud: since #525 list_window_ids + # answers [] ONLY for a listing that proved the session gone, so reaching this + # branch right after a mint means the server died under us — and silence would + # leak keys with no signal. + # + # The stderr below is load-bearing, not decoration: it is psmux 3.3.8's real + # vanished-session wording. Any other wording now RAISES (the sweep's outer + # arm warns instead), which is a different branch than this test pins. listing = '@bmad_project__blw@2 "live"\n@bmad_project__blw@7 "gone"\n' calls = [] def fake(argv, **kwargs): calls.append(argv) if argv[1] == "list-windows": - return subprocess.CompletedProcess(argv, 1, stdout="", stderr="no server") + return subprocess.CompletedProcess( + argv, 1, stdout="", stderr="psmux: no server running on session 'ctl'" + ) out = {"new-window": "@2\n", "show-options": listing}.get(argv[1], "") return subprocess.CompletedProcess(argv, 0, stdout=out, stderr="") diff --git a/tests/test_runs.py b/tests/test_runs.py index fb0af4ce..7213871e 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -4770,6 +4770,41 @@ def unselectable(): assert not runs.live_session_may_be_ours(tmp_path, "ctl") +def test_live_session_may_be_ours_reads_a_successful_listing_as_the_whole_truth( + tmp_path, monkeypatch +): + """The accepted ceiling of #732, pinned so it cannot be closed by accident. + + The two tests above cover a listing that *fails*. This one covers the + listing that succeeds and is wrong: psmux reaps a live session's registry + entry on any `has-session` whose 500 ms connect does not land, and until the + server's registry maintenance re-writes it `ls` exits 0 with that session + simply missing — no error, no stderr. (That maintenance runs on a nominal + 5 s check in the server's own loop, so the window has no hard bound; 1.7 s + was one measured sample.) The guard therefore cannot tell "absent" from + "omitted", and removal proceeds. That is the documented trade, not an + oversight; a change that makes an empty or short listing block instead has + to delete this test and read why. + + The third case is the positive control that makes the first two mean + something: the same guard, same fixtures, answers True the moment the + listing does name the session, so the two Falses are answers rather than a + harness that cannot say anything else.""" + name = runs.session_name("20260826-000000-run1") + monkeypatch.setattr(runs, "ctl_session_for", lambda project, mux=None: runs.CTL_SESSION) + + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([])) + assert not runs.live_session_may_be_ours(tmp_path, "20260826-000000-run1") + + # Non-empty and still omitting it: a reap takes one session's entry, not the + # registry, so "the listing came back with rows in it" is no reassurance either. + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([runs.session_name("other")])) + assert not runs.live_session_may_be_ours(tmp_path, "20260826-000000-run1") + + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([name])) + assert runs.live_session_may_be_ours(tmp_path, "20260826-000000-run1") + + def test_prune_sessions_claims_a_historical_ctl_prefixed_session(tmp_path, monkeypatch): """End to end through the sweep: a `bmad-loop-ctl-foo` session minted by an older release (`--run-id ctl-foo`), untagged, with this project's dead run diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index 920a4adc..bba68aa7 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -1387,12 +1387,26 @@ def fake(argv, **kwargs): raise OSError("server gone") if kill == "undecodable": raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte") + if kill == "unproven-nonzero": + # A server that errored while its windows are alive: rc 1, + # and a stderr that proves nothing about the session. psmux + # 3.3.8's verbatim answer to a listing whose auth the live + # server rejected — the shape #525 is about. + return subprocess.CompletedProcess( + argv, 1, stdout="", stderr="psmux: Invalid session key" + ) if kill == "session-gone": # rc 1, not rc 0 with empty stdout: real tmux answers a # vanished session with a nonzero exit and list_window_ids # folds it to [] — same verdict, and the path the transport - # actually takes. - return subprocess.CompletedProcess(argv, 1, stdout="", stderr="") + # actually takes. The stderr is tmux 3.4's verbatim wording + # and is load-bearing since #525: it is what makes this a + # PROVED vanish rather than a listing that merely failed, + # and an rc 1 without it now lands in `unverifiable` + # instead (test_prune_ctl_windows_..._unproven_nonzero). + return subprocess.CompletedProcess( + argv, 1, stdout="", stderr=f"can't find session: {launch.CTL_SESSION}" + ) gone = {a[-1] for a in killed} if kill == "lands" else set() return subprocess.CompletedProcess( argv, 0, stdout="\n".join(r[0] for r in rows if r[0] not in gone), stderr="" @@ -1490,9 +1504,15 @@ def test_prune_ctl_windows_undecodable_liveness_is_a_transport_fault(monkeypatch def test_prune_ctl_windows_reads_an_empty_listing_as_the_session_going_with_it( monkeypatch, tmp_path: Path ): - """`[]` is the seam's "no windows", not a failed probe (only a transport fault - raises) — a ctl session that died with its last window really did take the - candidate, so pessimism here would report a phantom survivor forever.""" + """`[]` is the seam's "no windows", not a failed probe — a ctl session that + died with its last window really did take the candidate, so pessimism here + would report a phantom survivor forever. + + The other half of #525's discrimination, and the reason narrowing the + sentinel could not simply be "raise on rc != 0": a vanished session exits + non-zero too, and turning THAT into `unverifiable` is the same dishonest + report from the other side — one that every subsequent cleanup re-reports + and nothing ever clears.""" _ctl_prune_fake(monkeypatch, tmp_path, kill="session-gone") assert launch.prune_ctl_windows(tmp_path) == ( @@ -1502,6 +1522,30 @@ def test_prune_ctl_windows_reads_an_empty_listing_as_the_session_going_with_it( ) +def test_prune_ctl_windows_unproven_nonzero_listing_claims_nothing(monkeypatch, tmp_path: Path): + """A listing that exits non-zero WITHOUT proving the session gone is a failed + probe, and the kills it was meant to verify stay unverifiable (#525). + + This is the exact over-optimistic report #435 exists to eliminate, reached + by the one route it left open: the backend folded every non-zero exit to + `[]`, so a server erroring while its windows are alive answered "this + session has no windows" and every candidate was classified verifiably + removed — with no error anywhere and nothing to re-try. + + Ablation: drop the `_session_proved_gone` guard in + `BaseTmuxBackend.list_window_ids` (return `[]` on any non-zero exit) and + this fails on `removed` carrying both windows, while its `session-gone` + sibling above still passes — the two together pin the discrimination + rather than either direction alone.""" + _ctl_prune_fake(monkeypatch, tmp_path, kill="unproven-nonzero") + + assert launch.prune_ctl_windows(tmp_path) == ( + [], + [], + ["sweep-20260101-000000-dead", "run-20260101-000000-dead2"], + ) + + def test_prune_ctl_windows_with_no_candidates_never_probes(monkeypatch, tmp_path: Path): """The listing is a real round trip; a prune with nothing to kill must not pay for it (and must not read an empty ctl session as anything at all)."""