From 1462c0f9674055a2995c37ab9fd080aa6f3f891a Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 11:03:36 -0400 Subject: [PATCH 01/44] Integrate reserved toolbar ownership across Stage 4 handoffs Keep nested prompts and selection on the shared reservation while transferring input ownership. Release physical rows before job-control suspension and before measuring the first pager frame, then reacquire using current terminal geometry. Restore bindings and terminal state after interrupted prompts, failed pager teardown, and reacquisition errors. Add transition regressions for completion, secrets, typeahead, resize, fallback, and signals. Validation: 2647 passed, 6 skipped; make check and make docs-test pass. Acceptance and dynamic harness gates pass at 12, 24, and 40 rows. Real-terminal and Windows qualification remain open. --- cmd2/cmd2.py | 39 +++-- cmd2/command_toolbar.py | 22 ++- cmd2/reserved_output.py | 9 +- cmd2/reserved_toolbar.py | 139 ++++++++++++++++-- cmd2/terminal_display.py | 5 + tests/test_reserved_lifecycle.py | 6 +- tests/test_reserved_terminal.py | 236 +++++++++++++++++++++++++++++++ tests/test_reserved_toolbar.py | 28 ++++ 8 files changed, 456 insertions(+), 28 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index ab9a8b269..92724a8a6 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -86,6 +86,7 @@ from prompt_toolkit.output import DummyOutput, create_output from prompt_toolkit.patch_stdout import patch_stdout from prompt_toolkit.shortcuts import CompleteStyle, PromptSession, choice, set_title +from prompt_toolkit.shortcuts.choice_input import ChoiceInput from prompt_toolkit.styles import DynamicStyle from rich.console import ( Group, @@ -2175,7 +2176,8 @@ def suspend_bottom_toolbar(self) -> Iterator[None]: Use this context manager around application-specific calls to ``input()``, other terminal UIs, or subprocesses that inherit the terminal. cmd2 automatically suspends - its toolbar for its own input prompts, external pagers, and shell commands. + its toolbar for external pagers and shell commands. Managed input prompts borrow + the reservation without releasing the rows. In reserved mode this also gives the reserved rows back, because a program that inherits the terminal knows nothing about a scroll region and would find its output @@ -3790,9 +3792,9 @@ def _read_raw_input( The command display is stopped either way, but only some prompts give the terminal away with it. The main prompt is the one the reservation exists for: it renders through the reserved output, and the toolbar has to still be there while the user is - typing -- that is what "stable across ordinary commands" means. Any other session is - an application prompt cmd2 has not bound to the reservation, so it gets the terminal - to itself, rows included. + typing. Managed nested prompts on the same terminal borrow that reservation with + their own bridge after the command input reader stops. Other terminals and prompts + inside an external handoff retain the exclusive-terminal path. :param prompt: the prompt text or a callable that returns the prompt. :param session: the PromptSession instance to use for reading. @@ -3800,11 +3802,13 @@ def _read_raw_input( :return: the stripped input string. :raises EOFError: if the input stream is closed or the user signals EOF (e.g., Ctrl+D) """ - owns_the_reservation = session is self.main_session + reserved = self._reserved_toolbar + owns_the_reservation = session is self.main_session or (reserved is not None and reserved.can_manage(session)) with self._quiesce_bottom_toolbar() if owns_the_reservation else self.suspend_bottom_toolbar(): - reserved = self._reserved_toolbar if owns_the_reservation and reserved is not None and reserved.bridge is not None: reserved.bridge.finish_command_output() + with reserved.prompt_session(session): + return self._read_raw_input_now(prompt, session, **prompt_kwargs) return self._read_raw_input_now(prompt, session, **prompt_kwargs) def _read_raw_input_now( @@ -3932,7 +3936,7 @@ def read_input( temp_session: PromptSession[str] = PromptSession( auto_suggest=self.main_session.auto_suggest, - bottom_toolbar=self.get_bottom_toolbar if self.main_session.bottom_toolbar is not None else None, + bottom_toolbar=self.main_session.bottom_toolbar, color_depth=self.main_session.color_depth, complete_style=self.main_session.complete_style, complete_in_thread=self.main_session.complete_in_thread, @@ -3961,7 +3965,7 @@ def read_secret( :raises Exception: any other exceptions raised by prompt() """ temp_session: PromptSession[str] = PromptSession( - bottom_toolbar=self.get_bottom_toolbar if self.main_session.bottom_toolbar is not None else None, + bottom_toolbar=self.main_session.bottom_toolbar, color_depth=self.main_session.color_depth, enable_suspend=self.main_session.enable_suspend, input=self.main_session.input, @@ -4937,7 +4941,7 @@ def do_quit(self, _: argparse.Namespace) -> bool | None: self.last_result = True return True - @command_toolbar.suspend_toolbar + @command_toolbar.quiesce_toolbar def select(self, opts: str | Iterable[str] | Iterable[tuple[Any, str | None]], prompt: str = "Your choice? ") -> Any: """Present a menu to the user. @@ -4972,7 +4976,22 @@ def select(self, opts: str | Iterable[str] | Iterable[tuple[Any, str | None]], p try: while True: with create_app_session(input=self.main_session.input, output=self.main_session.output): - result = choice(message=prompt, options=fulloptions) + reserved = self._reserved_toolbar + if reserved is not None and reserved.can_manage(self.main_session): + if reserved.bridge is not None: + reserved.bridge.finish_command_output() + # ChoiceInput.prompt() constructs and runs in one step. Binding + # its application first gives the initial frame reserved geometry. + selection = ChoiceInput( + message=prompt, + options=fulloptions, + style=self.main_session.style, + enable_suspend=self.main_session.enable_suspend, + )._create_application() + with reserved.prompt_application(selection): + result = selection.run() + else: + result = choice(message=prompt, options=fulloptions) if result is not None: return result except KeyboardInterrupt: diff --git a/cmd2/command_toolbar.py b/cmd2/command_toolbar.py index fc68ec43b..018e67de4 100644 --- a/cmd2/command_toolbar.py +++ b/cmd2/command_toolbar.py @@ -721,7 +721,9 @@ def page(self, text: str, *, chop: bool) -> None: # Measuring the toolbar can invoke its callback; keep that work on the # UI thread along with rendering and layout changes. toolbar_height = self._call_in_ui(lambda: self.toolbar.preferred_height(size.columns, size.rows).preferred) - if output_fits(text, size.columns, max(0, size.rows - toolbar_height), chop=chop): + reserved = self.cmd.reserved_toolbar + available_rows = size.rows if reserved is not None and reserved.is_active else max(0, size.rows - toolbar_height) + if output_fits(text, size.columns, available_rows, chop=chop): self.cmd.stdout.write(text) self.cmd.stdout.flush() return @@ -736,6 +738,7 @@ def page(self, text: str, *, chop: bool) -> None: entered = False restored = False close_error: BaseException | None = None + handoff = contextlib.ExitStack() def restore() -> None: """Give the application back to the display, whichever thread is doing it. @@ -763,6 +766,10 @@ def enter() -> None: # duration -- otherwise the pager swaps in its layout and nothing is ever painted. self._set_render_suppressed(False) self.app.renderer.erase() + if reserved is not None: + # The very first pager layout must see the physical size. Releasing from + # enter_alternate_screen during replay is too late: that frame was measured. + handoff.enter_context(reserved.suspended()) self.app.layout = layout self.app.key_bindings = pager.bindings self.app.editing_mode = EditingMode.EMACS @@ -776,8 +783,20 @@ def leave() -> None: entered = False try: self.app.renderer.erase() + except BaseException: + # An erase can fail before quitting the alternate screen. Complete upstream's + # mode/buffer cleanup before returning the main-screen reservation; never + # retry the erase itself, which may already have changed visible output. + with contextlib.suppress(Exception): + transaction = ( + reserved.lock.transaction("pager cleanup") if reserved is not None else contextlib.nullcontext() + ) + with transaction: + self.app.renderer.reset() + raise finally: restore() + handoff.__exit__(*sys.exc_info()) self.app.renderer.request_absolute_cursor_position() self.app.invalidate() @@ -813,6 +832,7 @@ def close() -> None: # the full-screen flag and editing mode would carry into the next prompt. if not restored: restore() + handoff.__exit__(*sys.exc_info()) # A reservation abandoned while the pager was open deferred its legacy fallback until # the pager closed; now that it has, on the main screen, finish it. if self._legacy_fallback_pending and self.thread_is_alive: diff --git a/cmd2/reserved_output.py b/cmd2/reserved_output.py index 58bc742d3..1f28831bc 100644 --- a/cmd2/reserved_output.py +++ b/cmd2/reserved_output.py @@ -55,6 +55,7 @@ def __init__(self, wrapped: Output, display: "TerminalDisplay") -> None: """ self._wrapped = wrapped self._display = display + self._alternate_handoff_owned = False # A plain attribute rather than a property: Output declares stdout as writable, and # code that reaches for the real stream must find the backend's, not a copy of it. self.stdout = getattr(wrapped, "stdout", None) @@ -158,13 +159,17 @@ def enter_alternate_screen(self) -> None: nothing about a reservation. Margins are restored before the switch so the main buffer is left in the state the shell expects if the switch is never undone. """ - self._display.release_region_for_handoff() + self._alternate_handoff_owned = not self._display.handoff_active + if self._alternate_handoff_owned: + self._display.release_region_for_handoff() self._wrapped.enter_alternate_screen() def quit_alternate_screen(self) -> None: """Return to the main buffer and re-establish the reservation for its geometry.""" self._wrapped.quit_alternate_screen() - self._display.reacquire_region_after_handoff() + if self._alternate_handoff_owned: + self._alternate_handoff_owned = False + self._display.reacquire_region_after_handoff() def scroll_buffer_to_prompt(self) -> None: """Scroll the Windows viewport to the prompt, then re-check the viewport origin. diff --git a/cmd2/reserved_toolbar.py b/cmd2/reserved_toolbar.py index 31cb79db8..c8acad42c 100644 --- a/cmd2/reserved_toolbar.py +++ b/cmd2/reserved_toolbar.py @@ -17,14 +17,18 @@ new ``bottom_toolbar`` to the session still reaches the band. """ -from contextlib import contextmanager, suppress +import os +import signal +from contextlib import ExitStack, contextmanager, suppress from types import TracebackType from typing import TYPE_CHECKING, Any, Self -from prompt_toolkit.filters import Condition +from prompt_toolkit.application import run_in_terminal +from prompt_toolkit.filters import Condition, Never from prompt_toolkit.layout import HSplit, Window from prompt_toolkit.layout.containers import ConditionalContainer from prompt_toolkit.styles import DynamicStyle +from prompt_toolkit.utils import suspend_to_background_supported from .prompt_toolkit_bridge import PromptToolkitBridge from .reserved_output import ReservedOutput @@ -107,6 +111,9 @@ def __init__( self._original_filter: Any = None self._installed_filter: Any = None self.stopped_handler: Callable[[], None] | None = None + self._job_control_stack = ExitStack() + self._nested_stacks: list[ExitStack] = [] + self._nested_bridges: list[PromptToolkitBridge] = [] @property def is_active(self) -> bool: @@ -132,7 +139,7 @@ def display(self) -> TerminalDisplay: @property def bridge(self) -> PromptToolkitBridge | None: """The renderer bridge while active, else ``None``.""" - return self._bridge + return self._nested_bridges[-1] if self._nested_bridges else self._bridge @property def painter(self) -> ToolbarPainter | None: @@ -195,6 +202,7 @@ def start(self) -> bool: # From here the application's own renders go through prepare and commit, which is # what puts them in the same queue as command output and toolbar paints. self._bridge.bind(app) + self._job_control_stack.enter_context(self._job_control(app)) # When the bridge abandons reserved rendering it cannot resume anything itself: # the rows are still withheld and the renderer is still routed through it. Giving # them back is this object's job, and it is what lets compatibility rendering @@ -239,6 +247,94 @@ def take_pending_error(self) -> BaseException | None: error = self._painter.take_pending_error() return error + @contextmanager + def _job_control(self, app: Any) -> "Iterator[None]": + """Release the physical reservation inside upstream's cooked-mode handoff.""" + original = app.suspend_to_background + + def suspend_to_background(suspend_group: bool = True) -> None: + if not suspend_to_background_supported(): + return + + def suspend_process() -> None: + # run_in_terminal has stopped rendering and detached input before this runs. + # A signal callback itself must never acquire the terminal transaction. + with self.suspended(): + os.kill(0 if suspend_group else os.getpid(), signal.SIGTSTP) + + run_in_terminal(suspend_process) + + app.suspend_to_background = suspend_to_background + try: + yield + finally: + if app.suspend_to_background is suspend_to_background: + app.suspend_to_background = original + + def can_manage(self, session: "PromptSession[Any]") -> bool: + """Whether a temporary prompt uses this terminal and its managed input reader.""" + return ( + self._display is not None + and not self._display.handoff_active + and session.input is self._session.input + and session.app.output in (self._bound_output, self._display.terminal.output) + and native_toolbar_container(session) is not None + ) + + @contextmanager + def prompt_session(self, session: "PromptSession[Any]") -> "Iterator[None]": + """Lend the reservation to a temporary prompt after the command reader has stopped.""" + if session is self._session: + yield + return + native = native_toolbar_container(session) + if native is None: + raise RuntimeError("cannot locate the nested session's bottom toolbar window") + with self.prompt_application(session.app, native): + yield + + @contextmanager + def prompt_application(self, app: Any, native: ConditionalContainer | None = None) -> "Iterator[None]": + """Bind a managed input application while retaining the physical reservation.""" + previous_output, previous_renderer_output = app.output, app.renderer.output + previous_filter = native.filter if native is not None else Never() + output = self._bound_output + installed_filter = previous_filter & Condition(lambda: not self.is_active) + bridge = PromptToolkitBridge(renderer=app.renderer, display=self.display, lock=self._lock) + previous_bridge = self.bridge + if previous_bridge is not None: + previous_bridge.forget_prompt_anchor() + previous_bridge.note_owner_change() + + def restore() -> None: + bridge.unbind() + if bridge in self._nested_bridges: + self._nested_bridges.remove(bridge) + if app.output is output: + app.output = previous_output + if app.renderer.output is output: + app.renderer.output = previous_renderer_output + if native is not None and native.filter is installed_filter: + native.filter = previous_filter + if previous_bridge is not None: + previous_bridge.forget_prompt_anchor() + previous_bridge.require_resynchronization("a nested prompt returned the terminal") + + with ExitStack() as stack: + stack.callback(restore) + # Abandonment restores the guest immediately, before native rendering resumes. + self._nested_stacks.append(stack) + stack.callback(self._nested_stacks.remove, stack) + app.output = app.renderer.output = output + if native is not None: + native.filter = installed_filter + self._nested_bridges.append(bridge) + bridge.bind(app) + bridge.set_frame_committed_handler(self.refresh) + bridge.set_emission_stopped_handler(self._emission_stopped) + stack.enter_context(self._job_control(app)) + yield + @contextmanager def suspended(self) -> "Iterator[None]": """Give the rows back for the duration of the block, and take them again after. @@ -267,8 +363,9 @@ def suspended(self) -> "Iterator[None]": yield return - outermost = self._suspend_depth == 0 + outermost = self._suspend_depth == 0 and not display.handoff_active self._suspend_depth += 1 + body_failed = False try: if outermost: with self._lock.transaction("suspend"): @@ -279,13 +376,25 @@ def suspended(self) -> "Iterator[None]": # paint over their output. self._invalidate_ownership("the terminal was handed to another program") yield + except BaseException: + body_failed = True + raise finally: self._suspend_depth -= 1 if outermost: - with self._lock.transaction("resume"): - display.reacquire_region_after_handoff() - self._invalidate_ownership("the terminal came back from another program") - self.refresh() + try: + with self._lock.transaction("resume"): + display.reacquire_region_after_handoff() + self._invalidate_ownership("the terminal came back from another program") + self.refresh() + except Exception as error: + self._pending_error = error + with suppress(Exception): + self.stop() + # A failed guest (including Ctrl-C/termination) remains the reason for + # unwinding; cleanup failure is available through take_pending_error(). + if not body_failed: + raise def _invalidate_ownership(self, reason: str) -> None: """Discard everything that described the screen before ownership changed. @@ -298,6 +407,9 @@ def _invalidate_ownership(self, reason: str) -> None: self._bridge.forget_prompt_anchor() self._bridge.forget_unfinished_command_output() self._bridge.require_resynchronization(reason) + for bridge in self._nested_bridges: + bridge.forget_prompt_anchor() + bridge.require_resynchronization(reason) def refresh(self) -> bool: """Evaluate the toolbar's content and paint whatever changed. @@ -335,8 +447,8 @@ def _emission_stopped(self) -> None: The error the bridge is holding is taken here rather than left with it: the bridge is dropped a moment later, and an error the user never sees is the same as none. """ - if self._bridge is not None and self._pending_error is None: - self._pending_error = self._bridge.take_pending_error() + if self.bridge is not None and self._pending_error is None: + self._pending_error = self.bridge.take_pending_error() with suppress(Exception): self.stop() @@ -357,8 +469,8 @@ def _paint_failed(self, error: BaseException) -> None: """ self._pending_error = error self._consecutive_paint_failures += 1 - if self._bridge is not None: - self._bridge.require_resynchronization("a toolbar paint failed; the cursor's position is unknown") + if self.bridge is not None: + self.bridge.require_resynchronization("a toolbar paint failed; the cursor's position is unknown") if self._consecutive_paint_failures >= _MAX_CONSECUTIVE_PAINT_FAILURES: with suppress(Exception): self.stop() @@ -371,6 +483,9 @@ def stop(self) -> None: other. """ display, self._display = self._display, None + for stack in reversed(tuple(self._nested_stacks)): + stack.close() + self._job_control_stack.close() if self._bridge is not None: self._bridge.unbind() self._bridge = None diff --git a/cmd2/terminal_display.py b/cmd2/terminal_display.py index 50184b192..6de70f629 100644 --- a/cmd2/terminal_display.py +++ b/cmd2/terminal_display.py @@ -346,6 +346,11 @@ def is_reserved(self) -> bool: """Whether a reservation is currently installed.""" return self._geometry is not None + @property + def handoff_active(self) -> bool: + """Whether another terminal owner holds the screen, even below the height floor.""" + return self._handoff_active + @property def output(self) -> "Output": """The output callers should render through: the adapter while reserved, else the backend.""" diff --git a/tests/test_reserved_lifecycle.py b/tests/test_reserved_lifecycle.py index 3bf52fe46..cc19dec5a 100644 --- a/tests/test_reserved_lifecycle.py +++ b/tests/test_reserved_lifecycle.py @@ -603,8 +603,8 @@ def test_the_main_prompt_leaves_the_native_toolbar_hidden(self) -> None: finally: harness.close() - def test_another_session_still_gets_the_terminal_to_itself(self) -> None: - """A prompt cmd2 has not bound to the reservation renders outside it, for now.""" + def test_another_session_on_the_same_terminal_borrows_the_reservation(self) -> None: + """Managed nested input retains the main-screen band.""" harness = Harness(mode="reserved") try: with harness.app._reserved_toolbar_context(): @@ -616,7 +616,7 @@ def test_another_session_still_gets_the_terminal_to_itself(self) -> None: harness.app._read_raw_input("> ", other) - assert seen == [False] + assert seen == [True] assert toolbar.display.is_reserved is True finally: harness.close() diff --git a/tests/test_reserved_terminal.py b/tests/test_reserved_terminal.py index 611d5c97c..84d40eba7 100644 --- a/tests/test_reserved_terminal.py +++ b/tests/test_reserved_terminal.py @@ -2,6 +2,7 @@ import asyncio import io +import signal import sys import threading import time @@ -14,6 +15,8 @@ import pytest from prompt_toolkit.application import run_in_terminal from prompt_toolkit.data_structures import Size +from prompt_toolkit.shortcuts import PromptSession +from prompt_toolkit.shortcuts.choice_input import ChoiceInput from cmd2 import command_toolbar from cmd2.reserved_toolbar import ReservedToolbar @@ -609,7 +612,239 @@ def drive() -> None: return shown.is_set() +class TestNestedPrompts: + def test_abandoning_reservation_during_nested_input_restores_both_owners(self, terminal_harness) -> None: + harness, terminal = terminal_harness + session = PromptSession(input=harness.pipe, output=harness.backend, bottom_toolbar="STATUS") + original_render = session.app.renderer.render + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + reserved = harness.app.reserved_toolbar + + def abandon(): + reserved.stop() + harness.pipe.send_text("answer\n") + + assert harness.app._read_raw_input("Nested: ", session, pre_run=abandon) == "answer" + assert reserved.bridge is None + assert session.app.output is harness.backend + assert session.app.renderer.render == original_render + assert harness.app._command_toolbar._proxy is not None + assert terminal.screen.margins is None + + def test_nested_resize_and_failed_guest_restore_the_main_owner(self, terminal_harness) -> None: + harness, terminal = terminal_harness + session = PromptSession(input=harness.pipe, output=harness.backend, bottom_toolbar="STATUS") + original = session.app.renderer.render + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + main_bridge = harness.app.reserved_toolbar.bridge + + def fail(): + resize(harness, terminal, 12, 40) + session.app._on_resize() + raise ValueError("nested failure") + + with pytest.raises(ValueError, match="nested failure"): + harness.app._read_raw_input("Nested: ", session, pre_run=fail) + assert harness.app.reserved_toolbar.bridge is main_bridge + assert session.app.renderer.render == original + assert terminal.screen.margins == pyte.screens.Margins(0, 10) + assert terminal.screen.display[-1].startswith("STATUS") + read_prompt(harness, terminal) + + @pytest.mark.parametrize( + ("kind", "keys", "expected"), + [("input", "ans\t", "answer"), ("secret", "hidden-value\n", "hidden-value"), ("select", "\x1b[B\r", "two")], + ) + def test_managed_input_keeps_the_band_and_one_reader(self, terminal_harness, kind, keys, expected) -> None: + harness, terminal = terminal_harness + created = [] + seen = [] + accepted = False + selected = False + + def ready(app): + nonlocal accepted, selected + if not seen and app.renderer._min_available_height > 0: + seen.append((terminal.screen.margins, terminal.screen.display[-1])) + assert not harness.app._command_toolbar.thread_is_alive + harness.pipe.send_text(keys + ("" if kind == "input" else "trailing\n")) + elif kind == "input" and not selected and app.current_buffer.complete_state is not None: + selected = True + harness.pipe.send_text("\t") + elif kind == "input" and not accepted and app.current_buffer.text.rstrip() == "answer": + accepted = True + harness.pipe.send_text("\rtrailing\n") + + def make_session(*args, **kwargs): + session = PromptSession(*args, **kwargs) + created.append(session) + + session.app.after_render += ready + return session + + original_create = ChoiceInput._create_application + + def make_choice(choice): + app = original_create(choice) + app.after_render += ready + return app + + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + before = len(terminal.getvalue()) + watchdog = threading.Timer(5, harness.pipe.close) + watchdog.start() + try: + with ( + mock.patch("cmd2.cmd2.PromptSession", make_session), + mock.patch.object(ChoiceInput, "_create_application", make_choice), + ): + if kind == "input": + result = harness.app.read_input("Value: ", choices=["answer"]) + elif kind == "secret": + result = harness.app.read_secret("Secret: ") + else: + result = harness.app.select(["one", "two"]) + assert result.rstrip() == expected + assert seen[0][0] == pyte.screens.Margins(0, 22) + assert seen[0][1].startswith("STATUS") + assert "\x1b[r" not in terminal.getvalue()[before:] + assert harness.app._command_toolbar.thread_is_alive + assert harness.app._read_raw_input("Next: ", harness.app.main_session) == "trailing" + assert not harness.app.reserved_toolbar._nested_bridges + if kind == "secret": + assert "hidden-value" not in terminal.getvalue() + finally: + watchdog.cancel() + watchdog.join() + + @pytest.mark.parametrize("keys", ["\x03", "\x04"]) + def test_nested_input_interrupt_restores_bindings(self, terminal_harness, keys) -> None: + harness, terminal = terminal_harness + session = PromptSession(input=harness.pipe, output=harness.backend, bottom_toolbar="STATUS") + original_output = session.app.output + original_render = session.app.renderer.render + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + with pytest.raises((KeyboardInterrupt, EOFError)): + harness.app._read_raw_input("Nested: ", session, pre_run=lambda: harness.pipe.send_text(keys)) + assert session.app.output is original_output + assert session.app.renderer.render == original_render + assert harness.app._command_toolbar.thread_is_alive + assert terminal.screen.margins == pyte.screens.Margins(0, 22) + assert terminal.screen.display[-1].startswith("STATUS") + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") +@pytest.mark.parametrize("owner", ["command", "prompt", "pager"]) +def test_ctrl_z_releases_before_signaling_and_reacquires_after_resume(terminal_harness, monkeypatch, owner) -> None: + harness, terminal = terminal_harness + harness.app.main_session.enable_suspend = True + stopped = threading.Event() + seen = [] + + def stop_process(pid, sig): + # This is the actual key-binding -> run_in_terminal -> signal path. The signal is + # replaced so the test runner never suspends itself or its parent process group. + seen.append((pid, sig, terminal.screen.margins, harness.app.main_session.app.output.get_size())) + assert harness.app.main_session.app._running_in_terminal + resize(harness, terminal, 12, 80) + stopped.set() + + monkeypatch.setattr("cmd2.reserved_toolbar.os.kill", stop_process) + with harness.app._reserved_toolbar_context(): + original_suspend = harness.app.main_session.app.suspend_to_background + if owner in ("command", "pager"): + with harness.app._command_toolbar_context(): + display = harness.app._command_toolbar + + def suspend() -> None: + harness.pipe.send_text("\x1a") + assert stopped.wait(5) + display._call_in_ui(lambda: None) + + if owner == "pager": + assert run_pager(harness, terminal, while_open=suspend) + else: + suspend() + assert terminal.screen.margins == pyte.screens.Margins(0, 10) + assert terminal.screen.display[-1].startswith("STATUS") + else: + ui = harness.app.main_session.app + accepted = False + + def ready(app): + nonlocal accepted + if stopped.is_set() and not accepted: + accepted = True + harness.pipe.send_text("\n") + + ui.after_render += ready + watchdog = threading.Timer(5, harness.pipe.close) + watchdog.start() + try: + assert ( + harness.app._read_raw_input( + "TEST> ", harness.app.main_session, pre_run=lambda: harness.pipe.send_text("\x1a") + ) + == "" + ) + assert stopped.is_set() + assert terminal.screen.margins == pyte.screens.Margins(0, 10) + finally: + watchdog.cancel() + watchdog.join() + ui.after_render -= ready + assert seen == [(0, signal.SIGTSTP, None, Size(24, 80))] + assert terminal.screen.margins is None + assert harness.app.main_session.app.suspend_to_background != original_suspend + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX termination handler") +def test_termination_unwinds_reserved_command_ownership(terminal_harness) -> None: + harness, terminal = terminal_harness + original_output = harness.app.main_session.app.output + displays = [] + + def terminate(): + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + displays.append(harness.app._command_toolbar) + harness.app.termination_signal_handler(signal.SIGTERM, None) + + with pytest.raises(SystemExit) as exit_info: + terminate() + assert exit_info.value.code == 128 + signal.SIGTERM + assert not displays[0].thread_is_alive + assert harness.app.main_session.app.output is original_output + assert terminal.screen.margins is None + + class TestPager: + def test_first_pager_frame_uses_full_geometry(self, terminal_harness) -> None: + harness, terminal = terminal_harness + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + display = harness.app._command_toolbar + sizes = [] + + def before_render(app): + if display._paging: + sizes.append(app.output.get_size()) + + display.app.before_render += before_render + try: + assert run_pager(harness, terminal) + assert sizes + assert all(size == Size(24, 80) for size in sizes) + finally: + display.app.before_render -= before_render + + def test_exact_usable_height_does_not_open_a_pager(self, terminal_harness) -> None: + harness, terminal = terminal_harness + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + text = "\n".join(f"line {i}" for i in range(23)) + with mock.patch.object(command_toolbar, "Pager", side_effect=AssertionError("unnecessary pager")): + harness.app._command_toolbar.page(text, chop=False) + assert any("line 22" in row for row in terminal.screen.display) + assert terminal.screen.display[-1].startswith("STATUS") + """The built-in pager renders a full screen of its own, so its frames must not be suppressed the way an ordinary command's empty frames are.""" @@ -771,6 +1006,7 @@ def drive() -> None: assert not forced_close.is_set(), "the quit callback failed to release page()" assert display.app.full_screen is False assert display.app.renderer.full_screen is False + assert display.app.renderer._in_alternate_screen is False assert display.app.layout is display._layout assert display.app.key_bindings is display._bindings or display.app.key_bindings is bindings assert display.app.editing_mode is editing_mode diff --git a/tests/test_reserved_toolbar.py b/tests/test_reserved_toolbar.py index 7f1cc7d03..3abf2014a 100644 --- a/tests/test_reserved_toolbar.py +++ b/tests/test_reserved_toolbar.py @@ -61,6 +61,34 @@ def app(self) -> Any: return self.session.app +@pytest.mark.parametrize("guest_fails", [False, True]) +def test_failed_reacquisition_releases_bindings_without_masking_the_guest(monkeypatch, guest_fails) -> None: + harness = Harness() + try: + with harness.toolbar: + display = harness.toolbar.display + + def fail(): + raise OSError("resume failed") + + monkeypatch.setattr(display, "reacquire_region_after_handoff", fail) + expected = ValueError if guest_fails else OSError + + def run_guest(): + with harness.toolbar.suspended(): + if guest_fails: + raise ValueError("guest failed") + + with pytest.raises(expected, match="guest failed" if guest_fails else "resume failed"): + run_guest() + assert harness.toolbar.bridge is None + assert harness.app.output is harness.backend + assert display.lease_depth == 0 + assert isinstance(harness.toolbar.take_pending_error(), OSError) + finally: + harness.close() + + class TestBinding: def test_starting_binds_the_application_and_its_renderer(self) -> None: harness = Harness() From c322cdec6c8304e93b150a60ab829c3ff6f932d7 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 11:14:25 -0400 Subject: [PATCH 02/44] Keep the toolbar visible while the pager prepares its first frame Defer clearing the main-screen band until the first prepared pager frame is ready to commit, while exposing full terminal geometry during layout. Clear retained cells before managed output, external handoffs, recovery, or shutdown so they cannot leak into history. Add regressions for startup visibility and deferred cleanup. Validation: 2652 passed, 6 skipped; make check and make docs-test pass. --- cmd2/command_toolbar.py | 2 +- cmd2/managed_output.py | 3 +++ cmd2/prompt_toolkit_bridge.py | 10 ++++++++ cmd2/reserved_toolbar.py | 10 ++++++-- cmd2/terminal_display.py | 22 ++++++++++++++-- tests/test_reserved_terminal.py | 45 +++++++++++++++++++++++++++++++++ tests/test_terminal_display.py | 11 ++++++++ 7 files changed, 98 insertions(+), 5 deletions(-) diff --git a/cmd2/command_toolbar.py b/cmd2/command_toolbar.py index 018e67de4..ff5a5b633 100644 --- a/cmd2/command_toolbar.py +++ b/cmd2/command_toolbar.py @@ -769,7 +769,7 @@ def enter() -> None: if reserved is not None: # The very first pager layout must see the physical size. Releasing from # enter_alternate_screen during replay is too late: that frame was measured. - handoff.enter_context(reserved.suspended()) + handoff.enter_context(reserved.suspended(defer_band_clear=True)) self.app.layout = layout self.app.key_bindings = pager.bindings self.app.editing_mode = EditingMode.EMACS diff --git a/cmd2/managed_output.py b/cmd2/managed_output.py index b0425f598..812b2aa38 100644 --- a/cmd2/managed_output.py +++ b/cmd2/managed_output.py @@ -63,6 +63,9 @@ def write(self, data: str) -> int: """ with self._lock.transaction("managed write"): try: + before_write = getattr(self.bridge, "before_managed_write", None) + if before_write is not None: + before_write() if self._output is None: written = self._stream.write(data) else: diff --git a/cmd2/prompt_toolkit_bridge.py b/cmd2/prompt_toolkit_bridge.py index c4ebccae9..8590695b6 100644 --- a/cmd2/prompt_toolkit_bridge.py +++ b/cmd2/prompt_toolkit_bridge.py @@ -300,6 +300,13 @@ def forget_unfinished_command_output(self) -> None: """ self._unfinished_command_output = False + def before_managed_write(self) -> None: + """Remove retained pager-startup cells before output can scroll them into history. + + Called inside the writer's transaction, before emitting any command text. + """ + self._display.clear_deferred_band() + def finish_command_output(self) -> None: """Start the next prompt on a fresh line if command output left one unfinished. @@ -831,6 +838,9 @@ def commit(self, prepared: PreparedRender) -> bool: self.require_resynchronization("the frame was laid out for a different size") return False try: + # Preparation may be expensive for a large pager. Its main-screen bar is + # retained until this validated batch is ready to replace the screen. + self._display.clear_deferred_band() prepared.batch.replay(self._display.output) self._display.output.flush() except Exception as error: # noqa: BLE001 - the terminal's state is now unknown diff --git a/cmd2/reserved_toolbar.py b/cmd2/reserved_toolbar.py index c8acad42c..6312faac5 100644 --- a/cmd2/reserved_toolbar.py +++ b/cmd2/reserved_toolbar.py @@ -336,7 +336,7 @@ def restore() -> None: yield @contextmanager - def suspended(self) -> "Iterator[None]": + def suspended(self, *, defer_band_clear: bool = False) -> "Iterator[None]": """Give the rows back for the duration of the block, and take them again after. A program that inherits the terminal -- a shell command, an editor, an external pager @@ -357,6 +357,9 @@ def suspended(self) -> "Iterator[None]": says nothing about whose terminal it is: the guest the outer block handed it to still has it, and reinstalling margins or painting a band over their screen would be the same mistake as never releasing at all. + + :param defer_band_clear: keep the main-screen bar visible during managed pager + preparation; external terminal users must retain the default immediate clear. """ display = self._display if display is None: @@ -367,9 +370,12 @@ def suspended(self) -> "Iterator[None]": self._suspend_depth += 1 body_failed = False try: + if not defer_band_clear: + with self._lock.transaction("clear retained toolbar"): + display.clear_deferred_band() if outermost: with self._lock.transaction("suspend"): - display.release_region_for_handoff() + display.release_region_for_handoff(defer_band_clear=defer_band_clear) # Forgotten as the terminal changes hands, not after the guest has # finished with it. From this moment the remembered row describes a screen # someone else is writing on, and anything that rendered against it would diff --git a/cmd2/terminal_display.py b/cmd2/terminal_display.py index 6de70f629..b9d8e885e 100644 --- a/cmd2/terminal_display.py +++ b/cmd2/terminal_display.py @@ -330,6 +330,7 @@ def __init__(self, output: "Output", reserved_rows: int = 1) -> None: self._depth = 0 self._adapter: Any = None self._handoff_active = False + self._deferred_band: Geometry | None = None @property def terminal(self) -> PhysicalTerminal: @@ -433,6 +434,7 @@ def release(self) -> None: def _teardown(self) -> None: """Restore full-screen margins and drop the adapter.""" + self.clear_deferred_band() self._adapter = None self._handoff_active = False if self._geometry is None: @@ -493,7 +495,7 @@ def reconfigure(self) -> bool: self._adapter = self._make_adapter() return True - def release_region_for_handoff(self) -> None: + def release_region_for_handoff(self, *, defer_band_clear: bool = False) -> None: """Restore full-screen margins for a program taking the terminal over. The reservation is remembered rather than dropped: the lease is still held, and @@ -505,6 +507,9 @@ def release_region_for_handoff(self) -> None: that had shrunk below the floor is released but still ours, and a guest can take it from that state just as readily; tying the record to the geometry snapshot would let a later resize reinstall margins over the guest's screen. + + :param defer_band_clear: retain the bar while a managed pager prepares its first + frame. The bridge clears it before commit; recovery and teardown clear it too. """ if self._depth == 0: return @@ -512,7 +517,19 @@ def release_region_for_handoff(self) -> None: if self._geometry is None: return geometry, self._geometry = self._geometry, None - self._terminal.release_region(geometry) + if defer_band_clear: + # A managed pager can prepare its full-screen frame while the old bar remains + # visible. Clear it only when that frame is ready to replace the main screen. + self._deferred_band = geometry + self._terminal.release_region() + else: + self._terminal.release_region(geometry) + + def clear_deferred_band(self) -> None: + """Clear a pager's retained main-screen bar before emission, recovery, or shutdown.""" + geometry, self._deferred_band = self._deferred_band, None + if geometry is not None: + self._terminal.release_region(geometry) def reacquire_region_after_handoff(self) -> None: """Re-establish the reservation after a program hands the terminal back. @@ -523,6 +540,7 @@ def reacquire_region_after_handoff(self) -> None: """ if not self._handoff_active or self._depth == 0: return + self.clear_deferred_band() self._handoff_active = False # Coming back is an ordinary reconfiguration, so it goes through the one path that # does the whole job: re-check backend capability, measure, install only if the diff --git a/tests/test_reserved_terminal.py b/tests/test_reserved_terminal.py index 84d40eba7..264e2807c 100644 --- a/tests/test_reserved_terminal.py +++ b/tests/test_reserved_terminal.py @@ -818,6 +818,51 @@ def terminate(): class TestPager: + @pytest.mark.parametrize("action", ["resume", "output", "external"]) + def test_retained_startup_bar_is_cleared_before_output_or_external_handoff(self, terminal_harness, action) -> None: + harness, terminal = terminal_harness + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + reserved = harness.app.reserved_toolbar + with reserved.suspended(defer_band_clear=True): + assert terminal.screen.display[-1].startswith("STATUS") + if action == "output": + # Blank lines can scroll the retained bar without overwriting its text. + harness.app.stdout.write("\n" * 50) + harness.app.stdout.flush() + elif action == "external": + with reserved.suspended(): + assert terminal.screen.display[-1].strip() == "" + else: + assert reserved.display._deferred_band is not None + assert reserved.display._deferred_band is None + assert terminal.screen.display[-1].startswith("STATUS") + history = ["".join(line[x].data for x in sorted(line)) for line in terminal.screen.history.top] + assert not any("STATUS" in row for row in history) + + def test_toolbar_remains_visible_until_the_first_pager_frame_is_prepared(self, terminal_harness, monkeypatch) -> None: + harness, terminal = terminal_harness + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + reserved = harness.app.reserved_toolbar + display = harness.app._command_toolbar + original_prepare = reserved.bridge.prepare + seen = [] + + def prepare(*args, **kwargs): + frame = original_prepare(*args, **kwargs) + if display._paging and frame is not None and not seen: + # Observe after the expensive layout work, before commit. Checking only + # the completed pager would miss the blank interval while a file loads. + seen.append(terminal.screen.display[-1]) + assert display.app.output.get_size() == Size(24, 80) + return frame + + monkeypatch.setattr(reserved.bridge, "prepare", prepare) + assert run_pager(harness, terminal) + assert seen + assert seen[0].startswith("STATUS") + assert reserved.display._deferred_band is None + assert terminal.screen.display[-1].startswith("STATUS") + def test_first_pager_frame_uses_full_geometry(self, terminal_harness) -> None: harness, terminal = terminal_harness with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): diff --git a/tests/test_terminal_display.py b/tests/test_terminal_display.py index 0508c002c..c2ed5eb7c 100644 --- a/tests/test_terminal_display.py +++ b/tests/test_terminal_display.py @@ -52,6 +52,17 @@ def make_resizable_output(rows: int = 24, columns: int = 80) -> tuple[Vt100_Outp class TestGeometry: + def test_shutdown_clears_a_bar_retained_for_an_unfinished_pager(self) -> None: + output, stream = make_output() + display = TerminalDisplay(output) + display.acquire() + display.release_region_for_handoff(defer_band_clear=True) + stream.seek(0) + stream.truncate() + display.release() + assert "\x1b[24;1H\x1b[0m\x1b[J" in stream.getvalue() + assert display._deferred_band is None + def test_usable_rows_subtracts_the_reservation_exactly_once(self) -> None: """The double-subtraction mutation: at 24 rows with 1 reserved, 22 is the wrong answer.""" geometry = Geometry(generation=1, physical_rows=24, columns=80, reserved_rows=1) From 5b4acab6a5c571b36bd02aa0a44d9278680247c1 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 11:17:24 -0400 Subject: [PATCH 03/44] Guard POSIX suspend signal lookup on Windows Resolve SIGTSTP conditionally before scheduling job-control suspension, preserving POSIX behavior without referencing a signal absent from Windows. Verified with mypy --platform win32 and the full test suite. --- cmd2/reserved_toolbar.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd2/reserved_toolbar.py b/cmd2/reserved_toolbar.py index 6312faac5..f8c2651fc 100644 --- a/cmd2/reserved_toolbar.py +++ b/cmd2/reserved_toolbar.py @@ -253,14 +253,15 @@ def _job_control(self, app: Any) -> "Iterator[None]": original = app.suspend_to_background def suspend_to_background(suspend_group: bool = True) -> None: - if not suspend_to_background_supported(): + suspend_signal = getattr(signal, "SIGTSTP", None) + if suspend_signal is None or not suspend_to_background_supported(): return def suspend_process() -> None: # run_in_terminal has stopped rendering and detached input before this runs. # A signal callback itself must never acquire the terminal transaction. with self.suspended(): - os.kill(0 if suspend_group else os.getpid(), signal.SIGTSTP) + os.kill(0 if suspend_group else os.getpid(), suspend_signal) run_in_terminal(suspend_process) From 3354add75c847a5579cd27aacc5a529955e9763f Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 11:51:52 -0400 Subject: [PATCH 04/44] Add include_py to getting_started.py for ease of testing --- examples/getting_started.py | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/getting_started.py b/examples/getting_started.py index 97dc05747..430137f20 100755 --- a/examples/getting_started.py +++ b/examples/getting_started.py @@ -60,6 +60,7 @@ def __init__(self) -> None: bottom_toolbar_mode=cmd2.ToolbarMode.AUTO, enable_rprompt=True, include_ipy=True, + include_py=True, persistent_history_file="cmd2_history.dat", refresh_interval=0.5, # refresh the UI twice a second to keep the bottom toolbar timestamp current shortcuts=shortcuts, From 1795cd9cd9ce558470e714be02dfe0b4f14b8e66 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 12:08:05 -0400 Subject: [PATCH 05/44] Added raise_exception.py example pyscript for just testing a script that raises an exception --- examples/scripts/raise_exception.py | 1 + 1 file changed, 1 insertion(+) create mode 100644 examples/scripts/raise_exception.py diff --git a/examples/scripts/raise_exception.py b/examples/scripts/raise_exception.py new file mode 100644 index 000000000..857b3678d --- /dev/null +++ b/examples/scripts/raise_exception.py @@ -0,0 +1 @@ +raise ValueError("An example exception") From 9649d4d858ba9882b65c629f563e47b9470066ac Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 12:27:40 -0400 Subject: [PATCH 06/44] Keep terminal pipelines in the foreground job during suspend and resume Let terminal-attached POSIX pipelines share cmd2's process group so Ctrl-Z and fg transfer input ownership together. Avoid forwarding SIGINT back to that group, while retaining isolated groups for nonterminal pipelines and Windows. Add real PTY coverage for repeated suspend/resume, shell input, resizing, and command recovery, plus process-group and SIGINT regression tests. --- cmd2/cmd2.py | 13 +-- cmd2/utils.py | 6 +- tests/test_command_toolbar.py | 17 ++++ tests/test_pipeline_job_control.py | 138 +++++++++++++++++++++++++++++ tests/test_utils.py | 9 ++ 5 files changed, 177 insertions(+), 6 deletions(-) create mode 100644 tests/test_pipeline_job_control.py diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 92724a8a6..95702a619 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3531,15 +3531,13 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: subproc_stdin = open(read_fd, encoding="utf-8") # noqa: SIM115 new_stdout: TextIO = cast(TextIO, open(write_fd, "w", encoding="utf-8")) # noqa: SIM115 - # Create pipe process in a separate group to isolate our signals from it. If a Ctrl-C event occurs, - # our sigint handler will forward it only to the most recent pipe process. This makes sure pipe - # processes close in the right order (most recent first). + # Captured pipelines use isolated groups for ordered Ctrl-C forwarding. + # Terminal-attached POSIX pipelines must instead belong to our foreground + # job, so Ctrl-Z and the shell's fg stop/resume every terminal reader together. kwargs: dict[str, Any] = {} if sys.platform == "win32": kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP else: - kwargs["start_new_session"] = True - # Attempt to run the pipe process in the user's preferred shell instead of the default behavior of using sh. shell = os.environ.get("SHELL") if shell: @@ -3555,6 +3553,11 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: ) pipe_stderr = None if isinstance(sys.stderr, utils.StdSim) else command_toolbar.pipe_target(sys.stderr) + if sys.platform != "win32": + kwargs["start_new_session"] = not any( + stream is not None and stream.isatty() for stream in (pipe_stdout, pipe_stderr) + ) + with contextlib.ExitStack() as terminal_stack: # The toolbar can neither draw nor hold the keyboard while a pipe process owns # the terminal, so step aside until that process has finished. diff --git a/cmd2/utils.py b/cmd2/utils.py index f9c449346..101d626bb 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -573,7 +573,11 @@ def send_sigint(self) -> None: # the whole process group to make sure it propagates further than the shell try: group_id = os.getpgid(self._proc.pid) - os.killpg(group_id, signal.SIGINT) + # Terminal-attached pipelines share our foreground group and already + # received the keyboard signal. Forwarding to it would invoke cmd2's + # handler recursively. Captured pipelines still need forwarding. + if group_id != os.getpgrp(): + os.killpg(group_id, signal.SIGINT) except ProcessLookupError: return diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index a691d391c..8555aacaf 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -1,6 +1,7 @@ """Command toolbar lifecycle and terminal integration tests.""" import contextlib +import subprocess import sys import threading import time @@ -119,6 +120,22 @@ def __getattr__(self, name): return getattr(self.file, name) +@pytest.mark.parametrize(("stdout_tty", "stderr_tty"), [(False, False), (True, False), (False, True), (True, True)]) +def test_pipeline_process_group_selection(toolbar_app, tmp_path, monkeypatch, running_pipe_process, stdout_tty, stderr_tty): + app, _, _ = toolbar_app + with (tmp_path / "stdout").open("w+") as out, (tmp_path / "stderr").open("w+") as err: + app.stdout = FileTerminal(out) if stdout_tty else out + monkeypatch.setattr(sys, "stderr", FileTerminal(err) if stderr_tty else err) + with mock.patch("subprocess.Popen", wraps=subprocess.Popen) as popen: + app.onecmd_plus_hooks(f'help | "{sys.executable}" -S -c "import sys; print(sys.stdin.read())"') + options = popen.call_args.kwargs + if sys.platform == "win32": + assert options["creationflags"] == subprocess.CREATE_NEW_PROCESS_GROUP + assert "start_new_session" not in options + else: + assert options["start_new_session"] == (not (stdout_tty or stderr_tty)) + + @pytest.mark.parametrize("builtin_pager", [False, True]) def test_command_toolbar_pipe_process_inherits_terminal(toolbar_app, tmp_path, builtin_pager, running_pipe_process) -> None: app, _, _ = toolbar_app diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py new file mode 100644 index 000000000..a925048db --- /dev/null +++ b/tests/test_pipeline_job_control.py @@ -0,0 +1,138 @@ +"""Exercise pipeline job control through a real controlling terminal and outer shell.""" + +import codecs +import contextlib +import os +import select +import shlex +import shutil +import signal +import subprocess +import sys +import time +from pathlib import Path + +import pyte +import pytest + +pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") + + +@pytest.mark.parametrize("finish", ["q", "\x03"]) +def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None: + import fcntl + import pty + import struct + import termios + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + pager = tmp_path / "pager.py" + pager_pid = tmp_path / "pager.pid" + pager.write_text( + "import os, pathlib, signal, sys, termios, tty\n" + f"pathlib.Path({str(pager_pid)!r}).write_text(str(os.getpid()))\n" + "sys.stdin.read()\n" + # Like less, use an inherited terminal descriptor for keyboard input when + # stdin is a pipe. This also works in the broken detached-session case. + "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" + " saved = termios.tcgetattr(terminal)\n" + " def resume(*args):\n" + " tty.setcbreak(terminal)\n" + " print('PAGER_RESUMED', flush=True)\n" + " signal.signal(signal.SIGCONT, resume)\n" + " try:\n" + " tty.setcbreak(terminal)\n" + " print('PAGER_READY', flush=True)\n" + " while os.read(terminal.fileno(), 1) != b'q':\n" + " pass\n" + " finally:\n" + " termios.tcsetattr(terminal, termios.TCSANOW, saved)\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd, ToolbarMode\n" + "app = Cmd(bottom_toolbar_mode=ToolbarMode.RESERVED)\n" + "app.prompt = 'TEST> '\n" + "app.main_session.bottom_toolbar = 'STATUS'\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 24, 80, 0, 0)) + # Establish a controlling terminal in a fresh interpreter, avoiding preexec_fn + # (unsafe when pytest or its plugins have started threads). + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + screen = pyte.Screen(80, 24) + screen.write_process_input = lambda data: os.write(master, data.encode()) + stream = pyte.Stream(screen) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def send(data): + os.write(master, data.encode()) + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + stream.feed(data) + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}") + + job_group = None + try: + wait_until(lambda: "OUTER> " in transcript) + send(f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n") + wait_until(lambda: screen.display[-1].startswith("STATUS")) + job_group = os.tcgetpgrp(master) + send(f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n") + wait_until(lambda: "PAGER_READY" in transcript) + for rows in (12, 24): + send("\x1a") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + start = len(transcript) + # A child left running can steal these keystrokes from the shell. + send("printf 'SHELL_%s\\n' OWNS_INPUT\n") + wait_until(lambda start=start: "SHELL_OWNS_INPUT" in transcript[start:]) + fcntl.ioctl(master, termios.TIOCSWINSZ, struct.pack("HHHH", rows, 80, 0, 0)) + screen.resize(lines=rows, columns=80) + start = len(transcript) + send("stty size\n") + wait_until(lambda start=start, rows=rows: f"\r\n{rows} 80\r\n" in transcript[start:]) + start = len(transcript) + send("fg\n") + wait_until(lambda start=start: "PAGER_RESUMED" in transcript[start:]) + assert os.tcgetpgrp(master) == job_group + send(finish) + wait_until(lambda: screen.display[-1].startswith("STATUS") and "TEST>" in "\n".join(screen.display)) + start = len(transcript) + send("help quit\n") + wait_until(lambda: "Exit this application" in transcript[start:]) + send("quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + # Kill only this test's job, including stopped descendants, on assertion failure. + if pager_pid.exists(): + with contextlib.suppress(ProcessLookupError): + os.kill(int(pager_pid.read_text()), signal.SIGKILL) + if job_group is not None: + with contextlib.suppress(ProcessLookupError): + os.killpg(job_group, signal.SIGKILL) + process.kill() + process.wait(timeout=5) + os.close(master) diff --git a/tests/test_utils.py b/tests/test_utils.py index 9c737d800..d63b4faa7 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -219,6 +219,15 @@ def test_proc_reader_send_sigint(pr_none) -> None: assert ret_code == -signal.SIGINT +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: + with mock.patch("os.getpgid", return_value=os.getpgrp()), mock.patch("os.killpg") as killpg: + pr_none.send_sigint() + killpg.assert_not_called() + pr_none.terminate() + pr_none.wait() + + def test_proc_reader_terminate(pr_none) -> None: assert pr_none._proc.poll() is None pr_none.terminate() From 37645b59755a8795f7d4412fe85f40515ff6a263 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 13:25:04 -0400 Subject: [PATCH 07/44] Repaint reserved toolbar after minimum-height reacquisition --- cmd2/toolbar_painter.py | 13 +++- tests/test_reserved_terminal.py | 109 ++++++++++++++++++++++++++++++++ tests/test_toolbar_painter.py | 17 +++++ 3 files changed, 137 insertions(+), 2 deletions(-) diff --git a/cmd2/toolbar_painter.py b/cmd2/toolbar_painter.py index 79310360c..063b7d416 100644 --- a/cmd2/toolbar_painter.py +++ b/cmd2/toolbar_painter.py @@ -355,7 +355,7 @@ def __init__( self._autowrap_after_paint = autowrap_after_paint self._last_frame: ToolbarFrame | None = None self._last_attrs: Mapping[str, Attrs] | None = None - self._last_band: tuple[int, int, int, object] | None = None + self._last_band: tuple[int, int, int, object, int] | None = None self._pending_error: BaseException | None = None @property @@ -438,7 +438,16 @@ def paint(self, prepared: PreparedFrame) -> bool: return False frame = prepared.frame - band = (geometry.physical_rows, geometry.columns, geometry.reserved_rows, geometry.buffer_id) + # Identical coordinates do not imply retained cells. A shrink below the + # reservation floor can erase the band, then growth can reacquire exactly + # the same dimensions and viewport. The new generation needs a full paint. + band = ( + geometry.physical_rows, + geometry.columns, + geometry.reserved_rows, + geometry.buffer_id, + geometry.generation, + ) previous = self._last_frame if band == self._last_band else None previous_attrs = self._last_attrs if previous is not None else None runs = _changed_runs(previous, previous_attrs, frame, prepared.attrs) diff --git a/tests/test_reserved_terminal.py b/tests/test_reserved_terminal.py index 264e2807c..67702d6f9 100644 --- a/tests/test_reserved_terminal.py +++ b/tests/test_reserved_terminal.py @@ -300,6 +300,76 @@ class TestResize: attached, so the poll on ``output.get_size()`` is the only way a resize arrives there. """ + @pytest.mark.parametrize("terminal_harness", [2, 3], indirect=True) + @pytest.mark.parametrize("text", ["", "typed"]) + def test_minimum_height_reacquisition_through_idle_polling(self, terminal_harness, monkeypatch, text) -> None: + """Fallback can erase a cached band before growth returns to exactly the same size.""" + harness, terminal = terminal_harness + ui = harness.app.main_session.app + ui.terminal_size_polling_interval = 0.01 + task = None + polls = 0 + errors = [] + + async def until(predicate): + async with asyncio.timeout(5): + while not predicate(): # noqa: ASYNC110 - observe upstream state without driving a redraw + await asyncio.sleep(0.01) + + async def transitions(): + try: + if text: + harness.pipe.send_text(text) + await until(lambda: ui.current_buffer.text == text) + for rows, columns in ((3, 80), (2, 80), (3, 80), (4, 40), (24, 100), (3, 80), (2, 80), (3, 80)): + baseline = polls + # Let upstream establish/settle its size baseline before changing it. + await until(lambda baseline=baseline: polls >= baseline + 2) + if harness.size != Size(rows=rows, columns=columns): + resize(harness, terminal, rows, columns) + toolbar = harness.app.reserved_toolbar + await until(lambda toolbar=toolbar, rows=rows: toolbar.is_active == (rows >= 3)) + if rows >= 3: + await until(lambda toolbar=toolbar, rows=rows: toolbar.display.geometry.physical_rows == rows) + await until(lambda: terminal.screen.display[-1].startswith("STATUS")) + await until(lambda: any(row.startswith("TEST> " + text) for row in terminal.screen.display)) + assert ui.current_buffer.text == text + assert ui.current_buffer.cursor_position == len(text) + except Exception as error: # noqa: BLE001 - report outside prompt-toolkit's event loop + errors.append(error) + finally: + harness.pipe.send_text("\n") + + def ready(app): + nonlocal task + if task is None: + task = ui.create_background_task(transitions()) + + ui.after_render += ready + try: + with harness.app._reserved_toolbar_context(): + get_size = ui.output.get_size + + def observe_size(): + nonlocal polls + try: + current = asyncio.current_task() + except RuntimeError: + current = None + if current is not None and current.get_coro().__name__ == "_poll_output_size": + polls += 1 + return get_size() + + monkeypatch.setattr(ui.output, "get_size", observe_size) + assert harness.app._read_raw_input("TEST> ", harness.app.main_session) == text + assert task is not None + task.result() + if errors: + raise errors[0] + finally: + ui.after_render -= ready + assert terminal.screen.margins is None + @pytest.mark.parametrize("terminal_harness", [2], indirect=True) def test_initially_short_prompt_grows_and_accepts_visible_input(self, terminal_harness) -> None: harness, terminal = terminal_harness @@ -330,6 +400,45 @@ def ready(app): watchdog.join() ui.after_render -= ready + @pytest.mark.parametrize("terminal_harness", [3], indirect=True) + @pytest.mark.parametrize("partial", ["", "PARTIAL"]) + def test_quiet_command_reacquires_the_same_band(self, terminal_harness, monkeypatch, partial) -> None: + """The command poll restores the whole band without disturbing unfinished output.""" + harness, terminal = terminal_harness + harness.app.main_session.app.terminal_size_polling_interval = 0.01 + with harness.app._reserved_toolbar_context(), harness.app._command_toolbar_context(): + toolbar = harness.app.reserved_toolbar + output = harness.app._command_toolbar.app.output + get_size = output.get_size + polls = 0 + + def observe_size(): + nonlocal polls + try: + current = asyncio.current_task() + except RuntimeError: + current = None + if current is not None and current.get_coro().__name__ == "_poll_output_size": + polls += 1 + return get_size() + + monkeypatch.setattr(output, "get_size", observe_size) + harness.app.stdout.write(partial) + harness.app.stdout.flush() + for rows in (2, 3, 4, 3, 2, 3): + baseline = polls + assert wait_for(lambda baseline=baseline: polls >= baseline + 2) + resize(harness, terminal, rows, 80) + assert wait_for(lambda rows=rows: toolbar.is_active == (rows >= 3)) + if rows >= 3: + assert wait_for(lambda rows=rows: toolbar.display.geometry.physical_rows == rows) + assert wait_for(lambda: terminal.screen.display[-1].startswith("STATUS")) + if partial: + assert any(row.startswith(partial) for row in terminal.screen.display) + harness.app.poutput(" COMPLETE") + assert any(row.startswith(partial + " COMPLETE") for row in terminal.screen.display) + assert terminal.screen.margins is None + @pytest.mark.parametrize("terminal_harness", [2], indirect=True) def test_initially_short_terminal_acquires_during_a_quiet_command(self, terminal_harness, monkeypatch) -> None: harness, terminal = terminal_harness diff --git a/tests/test_toolbar_painter.py b/tests/test_toolbar_painter.py index 603327c81..c1d9095f7 100644 --- a/tests/test_toolbar_painter.py +++ b/tests/test_toolbar_painter.py @@ -470,6 +470,23 @@ def test_a_geometry_change_forces_a_full_repaint(self) -> None: assert harness.paint("hi") is True assert "\x1b[12;1Hhi " in harness.visible() + @pytest.mark.parametrize("prepare_while_inactive", [False, True]) + def test_reacquiring_the_same_band_repaints_all_cells(self, prepare_while_inactive: bool) -> None: + """Returning from fallback must not reuse cells painted before the reservation ended.""" + harness = Harness(rows=3) + assert harness.paint("hi") is True + harness.display.resize(2) + assert not harness.display.is_reserved + if prepare_while_inactive: + assert harness.painter.prepare(lambda: "hi") is None + harness.display.resize(3) + harness.clear() + assert harness.paint("hi") is True + assert "\x1b[3;1Hhi " in harness.visible() + harness.clear() + assert harness.paint("hi") is False + assert harness.written() == "" + def test_empty_content_is_painted_rather_than_skipped(self) -> None: """An empty toolbar is an intentional visibility change and must reach the band.""" harness = Harness() From 2b78110f921fe4fbb04c21bd901a75af36fb42e5 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 13:51:07 -0400 Subject: [PATCH 08/44] Accept bash 5.1+ bracketed-paste output in the job-control test Bash 5.1 and later turn bracketed paste off with "\x1b[?2004l\r" before running a command, so the reply to "stty size" follows a bare carriage return rather than "\r\n". The test's predicate required the latter and timed out on Linux CI while passing on macOS, whose bash 3.2 has no bracketed paste. Match the reply at any line boundary instead. --- tests/test_pipeline_job_control.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index a925048db..17636ba9f 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -3,6 +3,7 @@ import codecs import contextlib import os +import re import select import shlex import shutil @@ -113,7 +114,9 @@ def wait_until(predicate): screen.resize(lines=rows, columns=80) start = len(transcript) send("stty size\n") - wait_until(lambda start=start, rows=rows: f"\r\n{rows} 80\r\n" in transcript[start:]) + # Bash 5.1+ turns bracketed paste off with "\x1b[?2004l\r" before running the + # command, so the reply may follow a bare "\r" rather than "\r\n". + wait_until(lambda start=start, rows=rows: re.search(rf"[\r\n]{rows} 80\r\n", transcript[start:]) is not None) start = len(transcript) send("fg\n") wait_until(lambda start=start: "PAGER_RESUMED" in transcript[start:]) From c05240170816ad08f00d4884610135a933671e25 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 14:31:53 -0400 Subject: [PATCH 09/44] Report from the job-control test's pager with os.write, not print The helper pager announced readiness and resumption with print. Its SIGCONT handler could therefore run while the main thread was still inside the PAGER_READY print, since the test stops the job the instant that marker appears, and Python raised "reentrant call inside <_io.BufferedWriter>". The pager died with a traceback whose source line contained PAGER_RESUMED, which satisfied the first wait by accident, and the second timed out. Write the markers with os.write, which is safe from a signal handler, and match them as whole lines so a traceback can never satisfy the wait. --- tests/test_pipeline_job_control.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 17636ba9f..aa9c520a8 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -39,13 +39,16 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None # stdin is a pipe. This also works in the broken detached-session case. "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" " saved = termios.tcgetattr(terminal)\n" + # os.write rather than print: a signal handler that uses buffered stdout raises + # "reentrant call inside <_io.BufferedWriter>" when the signal lands mid-write, + # which happens when the job is stopped while still reporting readiness. " def resume(*args):\n" " tty.setcbreak(terminal)\n" - " print('PAGER_RESUMED', flush=True)\n" + " os.write(1, b'PAGER_RESUMED\\n')\n" " signal.signal(signal.SIGCONT, resume)\n" " try:\n" " tty.setcbreak(terminal)\n" - " print('PAGER_READY', flush=True)\n" + " os.write(1, b'PAGER_READY\\n')\n" " while os.read(terminal.fileno(), 1) != b'q':\n" " pass\n" " finally:\n" @@ -102,7 +105,8 @@ def wait_until(predicate): wait_until(lambda: screen.display[-1].startswith("STATUS")) job_group = os.tcgetpgrp(master) send(f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n") - wait_until(lambda: "PAGER_READY" in transcript) + # Whole lines only: a traceback naming the marker must not satisfy the wait. + wait_until(lambda: "PAGER_READY\r\n" in transcript) for rows in (12, 24): send("\x1a") wait_until(lambda: os.tcgetpgrp(master) == process.pid) @@ -119,7 +123,7 @@ def wait_until(predicate): wait_until(lambda start=start, rows=rows: re.search(rf"[\r\n]{rows} 80\r\n", transcript[start:]) is not None) start = len(transcript) send("fg\n") - wait_until(lambda start=start: "PAGER_RESUMED" in transcript[start:]) + wait_until(lambda start=start: "PAGER_RESUMED\r\n" in transcript[start:]) assert os.tcgetpgrp(master) == job_group send(finish) wait_until(lambda: screen.display[-1].startswith("STATUS") and "TEST>" in "\n".join(screen.display)) From b1f80a6d8d1ede4784790e6820f1afa9c127ca34 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 14:38:28 -0400 Subject: [PATCH 10/44] Cover the reserved toolbar's remaining job-control and nested-prompt paths Three paths had no test: declining to suspend where the platform or the stop signal is missing, refusing a nested session whose layout has no toolbar window, and invalidating a nested prompt's bridge when the terminal is handed to a guest program. Each new test fails if its line is removed. --- tests/test_reserved_toolbar.py | 61 ++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/test_reserved_toolbar.py b/tests/test_reserved_toolbar.py index 3abf2014a..bb7cea6c2 100644 --- a/tests/test_reserved_toolbar.py +++ b/tests/test_reserved_toolbar.py @@ -7,6 +7,7 @@ """ import io +import signal from typing import Any import pytest @@ -834,3 +835,63 @@ def test_suspending_a_toolbar_that_never_started_does_nothing(self) -> None: assert harness.written() == "" finally: harness.close() + + +class TestNestedPrompts: + def test_a_nested_session_without_a_toolbar_window_is_refused(self) -> None: + """Lending the reservation to a layout with no toolbar window would leave two toolbars.""" + harness = Harness() + try: + harness.toolbar.start() + main_bridge = harness.toolbar.bridge + nested: PromptSession[str] = PromptSession(input=harness.pipe, output=harness.backend) + nested.app.layout = Layout(Window()) + with pytest.raises(RuntimeError, match="nested session"), harness.toolbar.prompt_session(nested): + pass + assert nested.app.output is harness.backend + assert harness.toolbar.bridge is main_bridge + finally: + harness.close() + + def test_suspending_invalidates_a_nested_prompt_bridge_too(self) -> None: + """The guest program writes over the nested prompt's screen as much as the main one's.""" + harness = Harness() + try: + harness.toolbar.start() + nested: PromptSession[str] = PromptSession(input=harness.pipe, output=harness.backend, bottom_toolbar="NESTED") + with harness.toolbar.prompt_session(nested): + bridge = harness.toolbar.bridge + assert bridge is not None + assert bridge is not harness.toolbar._bridge + bridge.set_prompt_anchor(5) + with harness.toolbar.suspended(): + assert bridge.prompt_anchor is None + assert bridge.needs_resynchronization is True + assert "handed to another program" in (bridge.resynchronization_reason or "") + assert "came back from another program" in (bridge.resynchronization_reason or "") + finally: + harness.close() + + +class TestJobControl: + @pytest.mark.parametrize("missing", ["support", "signal"]) + def test_suspending_to_background_does_nothing_where_it_is_unsupported( + self, monkeypatch: pytest.MonkeyPatch, missing: str + ) -> None: + """Without a stop signal or a supporting platform there is no process to stop.""" + harness = Harness() + try: + harness.toolbar.start() + calls: list[Any] = [] + monkeypatch.setattr("cmd2.reserved_toolbar.run_in_terminal", calls.append) + if missing == "support": + monkeypatch.setattr("cmd2.reserved_toolbar.suspend_to_background_supported", lambda: False) + else: + monkeypatch.delattr(signal, "SIGTSTP", raising=False) + harness.clear() + harness.app.suspend_to_background() + assert calls == [] + assert harness.written() == "" + assert harness.toolbar.is_active is True + finally: + harness.close() From a785b9699742b00638e9f886a172aae834fd40a1 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 15:04:40 -0400 Subject: [PATCH 11/44] Wait for the whole job to stop before typing at the shell in the job-control test The shell takes the terminal back as soon as its direct child stops, but the helper pager, a grandchild, may not have processed its stop signal yet. It sits in a one-byte terminal read, and the tty read loop hands a woken reader any byte already queued before it checks for the pending stop, so the pager consumed the first keystroke of the probe command and the shell ran "rintf". Free-threaded builds under load widened the window enough to hit on CI. Wait until both cmd2 and the pager report the stopped state before typing. --- tests/test_pipeline_job_control.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index aa9c520a8..9dfc3648b 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -98,6 +98,19 @@ def wait_until(predicate): return pytest.fail(f"terminal condition timed out:\n{transcript}") + def stopped(*pids: int) -> bool: + """Whether every process is stopped, not merely deprived of the terminal. + + The shell takes the terminal back as soon as its child stops, but a grandchild still + blocked in a one-byte terminal read is woken by the stop signal and, if a keystroke + has arrived by then, consumes it before it stops. Typing has to wait for the whole job. + """ + listing = subprocess.run( + ["ps", "-o", "stat=", "-p", ",".join(map(str, pids))], capture_output=True, text=True, check=False + ) + states = listing.stdout.split() + return len(states) == len(pids) and all(state.startswith("T") for state in states) + job_group = None try: wait_until(lambda: "OUTER> " in transcript) @@ -107,9 +120,11 @@ def wait_until(predicate): send(f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n") # Whole lines only: a traceback naming the marker must not satisfy the wait. wait_until(lambda: "PAGER_READY\r\n" in transcript) + pager_process = int(pager_pid.read_text()) for rows in (12, 24): send("\x1a") wait_until(lambda: os.tcgetpgrp(master) == process.pid) + wait_until(lambda: stopped(job_group, pager_process)) start = len(transcript) # A child left running can steal these keystrokes from the shell. send("printf 'SHELL_%s\\n' OWNS_INPUT\n") From c16e943a066cdf5d964418e7c696a52de18ee9a8 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 15:42:49 -0400 Subject: [PATCH 12/44] Fix forwarding of process-directed SIGINT to pipelines --- cmd2/utils.py | 14 ++++++++++---- tests/test_pipeline_job_control.py | 9 +++++++-- tests/test_utils.py | 25 +++++++++++++++++++------ 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index 101d626bb..9422a9f40 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -573,10 +573,16 @@ def send_sigint(self) -> None: # the whole process group to make sure it propagates further than the shell try: group_id = os.getpgid(self._proc.pid) - # Terminal-attached pipelines share our foreground group and already - # received the keyboard signal. Forwarding to it would invoke cmd2's - # handler recursively. Captured pipelines still need forwarding. - if group_id != os.getpgrp(): + if group_id == os.getpgrp(): + # A process-directed SIGINT may not have reached the pipeline. + # Ignore our own forwarded signal to avoid recursive handling, + # then restore the handler even if the group has exited. + original_handler = signal.signal(signal.SIGINT, signal.SIG_IGN) + try: + os.killpg(group_id, signal.SIGINT) + finally: + signal.signal(signal.SIGINT, original_handler) + else: os.killpg(group_id, signal.SIGINT) except ProcessLookupError: return diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 9dfc3648b..747318928 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -19,7 +19,7 @@ pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") -@pytest.mark.parametrize("finish", ["q", "\x03"]) +@pytest.mark.parametrize("finish", ["q", "\x03", "process_sigint"]) def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None: import fcntl import pty @@ -140,7 +140,12 @@ def stopped(*pids: int) -> bool: send("fg\n") wait_until(lambda start=start: "PAGER_RESUMED\r\n" in transcript[start:]) assert os.tcgetpgrp(master) == job_group - send(finish) + if finish == "process_sigint": + # Signal cmd2 alone, as with `kill -INT `. Its pipeline + # shares the terminal group but must receive a forwarded SIGINT. + os.kill(job_group, signal.SIGINT) + else: + send(finish) wait_until(lambda: screen.display[-1].startswith("STATUS") and "TEST>" in "\n".join(screen.display)) start = len(transcript) send("help quit\n") diff --git a/tests/test_utils.py b/tests/test_utils.py index d63b4faa7..ea73082ee 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -220,12 +220,25 @@ def test_proc_reader_send_sigint(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") -def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: - with mock.patch("os.getpgid", return_value=os.getpgrp()), mock.patch("os.killpg") as killpg: - pr_none.send_sigint() - killpg.assert_not_called() - pr_none.terminate() - pr_none.wait() +@pytest.mark.parametrize("group_exited", [False, True]) +def test_proc_reader_forwards_to_its_own_group(pr_none, group_exited) -> None: + original_handler = signal.getsignal(signal.SIGINT) + + def forward(group_id, signum): + assert group_id == os.getpgrp() + assert signum == signal.SIGINT + assert signal.getsignal(signal.SIGINT) == signal.SIG_IGN + if group_exited: + raise ProcessLookupError + + try: + with mock.patch("os.getpgid", return_value=os.getpgrp()), mock.patch("os.killpg", side_effect=forward) as killpg: + pr_none.send_sigint() + killpg.assert_called_once_with(os.getpgrp(), signal.SIGINT) + assert signal.getsignal(signal.SIGINT) == original_handler + finally: + pr_none.terminate() + pr_none.wait() def test_proc_reader_terminate(pr_none) -> None: From ece4307cb59b4590a2119ed6f53ee400591281ed Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 16:09:12 -0400 Subject: [PATCH 13/44] Avoid duplicate SIGINT delivery to terminal pipelines --- cmd2/cmd2.py | 50 ++++++++--- cmd2/utils.py | 132 ++++++++++++++++++++++++++--- tests/test_command_toolbar.py | 5 +- tests/test_pipeline_job_control.py | 81 +++++++++++++++--- tests/test_utils.py | 17 +--- 5 files changed, 234 insertions(+), 51 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 95702a619..9fbf15c53 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -2183,7 +2183,11 @@ def suspend_bottom_toolbar(self) -> Iterator[None]: inherits the terminal knows nothing about a scroll region and would find its output confined to rows it never asked for. The rows are taken again afterwards. """ - with self._quiesce_bottom_toolbar(): + reader = self._cur_pipe_proc_reader + with ( + reader.borrow_terminal() if reader is not None else contextlib.nullcontext(), + self._quiesce_bottom_toolbar(), + ): reserved = self._reserved_toolbar if reserved is None: @@ -3531,9 +3535,8 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: subproc_stdin = open(read_fd, encoding="utf-8") # noqa: SIM115 new_stdout: TextIO = cast(TextIO, open(write_fd, "w", encoding="utf-8")) # noqa: SIM115 - # Captured pipelines use isolated groups for ordered Ctrl-C forwarding. - # Terminal-attached POSIX pipelines must instead belong to our foreground - # job, so Ctrl-Z and the shell's fg stop/resume every terminal reader together. + # Isolate pipeline signals from cmd2. Terminal pipelines receive the + # foreground terminal; ProcReader relays their job-control stops. kwargs: dict[str, Any] = {} if sys.platform == "win32": kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP @@ -3553,10 +3556,18 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: ) pipe_stderr = None if isinstance(sys.stderr, utils.StdSim) else command_toolbar.pipe_target(sys.stderr) + terminal_fd = None if sys.platform != "win32": - kwargs["start_new_session"] = not any( - stream is not None and stream.isatty() for stream in (pipe_stdout, pipe_stderr) - ) + for stream in (pipe_stdout, pipe_stderr): + if stream is not None and stream.isatty(): + with contextlib.suppress(OSError, ValueError): + if os.tcgetpgrp(stream.fileno()) == os.getpgrp(): + terminal_fd = stream.fileno() + break + if terminal_fd is None: + kwargs["start_new_session"] = True + else: + kwargs["process_group"] = 0 with contextlib.ExitStack() as terminal_stack: # The toolbar can neither draw nor hold the keyboard while a pipe process owns @@ -3572,21 +3583,36 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: shell=True, **kwargs, ) + if terminal_fd is not None: + import signal + + # The producer may still write diagnostics while the consumer owns + # the terminal. Block SIGTTOU after Popen so the child retains normal + # job-control behavior, and restore our mask with the handoff. + previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) + terminal_stack.callback(signal.pthread_sigmask, signal.SIG_SETMASK, previous_mask) + cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, terminal_fd=terminal_fd) # Popen was called with shell=True so the user can chain pipe commands and redirect their output # like: !ls -l | grep user | wc -l > out.txt. But this makes it difficult to know if the pipe process # started OK, since the shell itself always starts. Therefore, we will wait a short time and check # if the pipe process is still running. with contextlib.suppress(subprocess.TimeoutExpired): - proc.wait(0.2) + if cmd_pipe_proc_reader is None: + proc.wait(0.2) + else: + cmd_pipe_proc_reader.wait_for_exit(0.2) # Check if the pipe process already exited if proc.returncode is not None: + if cmd_pipe_proc_reader is not None: + cmd_pipe_proc_reader.wait() subproc_stdin.close() new_stdout.close() raise RedirectionError(f"Pipe process exited with code {proc.returncode} before command could run") redir_saved_state.redirecting = True - cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) + if cmd_pipe_proc_reader is None: + cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) self.stdout = new_stdout @@ -3807,7 +3833,11 @@ def _read_raw_input( """ reserved = self._reserved_toolbar owns_the_reservation = session is self.main_session or (reserved is not None and reserved.can_manage(session)) - with self._quiesce_bottom_toolbar() if owns_the_reservation else self.suspend_bottom_toolbar(): + reader = self._cur_pipe_proc_reader + with ( + reader.borrow_terminal() if reader is not None else contextlib.nullcontext(), + self._quiesce_bottom_toolbar() if owns_the_reservation else self.suspend_bottom_toolbar(), + ): if owns_the_reservation and reserved is not None and reserved.bridge is not None: reserved.bridge.finish_command_output() with reserved.prompt_session(session): diff --git a/cmd2/utils.py b/cmd2/utils.py index 9422a9f40..38e20aed7 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -13,6 +13,7 @@ from collections.abc import ( Callable, Iterable, + Iterator, MutableSequence, ) from difflib import SequenceMatcher @@ -539,16 +540,33 @@ class ProcReader: If neither are pipes, then the process will run normally and no output will be captured. """ - def __init__(self, proc: PopenTextIO, stdout: StdSim | TextIO, stderr: StdSim | TextIO) -> None: + def __init__( + self, proc: PopenTextIO, stdout: StdSim | TextIO, stderr: StdSim | TextIO, *, terminal_fd: int | None = None + ) -> None: """ProcReader initializer. :param proc: the Popen process being read from :param stdout: the stream to write captured stdout :param stderr: the stream to write captured stderr. + :param terminal_fd: controlling terminal to lend to a POSIX process in its own group """ self._proc = proc self._stdout = stdout self._stderr = stderr + self._terminal_fd = terminal_fd + self._process_done = threading.Event() + self._sigint_forwarded = False + self._terminal_available = threading.Event() + self._terminal_available.set() + if terminal_fd is not None: + import signal + + self._original_group = os.tcgetpgrp(terminal_fd) + self._set_foreground_group(terminal_fd, proc.pid) + # The child may have tried to read before we could give it the terminal. + with contextlib.suppress(ProcessLookupError): + os.killpg(proc.pid, signal.SIGCONT) + threading.Thread(name="pipe_job", target=self._wait_for_job, args=(terminal_fd,), daemon=True).start() self._out_thread = threading.Thread(name="out_thread", target=self._reader_thread_func, kwargs={"read_stdout": True}) @@ -573,26 +591,114 @@ def send_sigint(self) -> None: # the whole process group to make sure it propagates further than the shell try: group_id = os.getpgid(self._proc.pid) - if group_id == os.getpgrp(): - # A process-directed SIGINT may not have reached the pipeline. - # Ignore our own forwarded signal to avoid recursive handling, - # then restore the handler even if the group has exited. - original_handler = signal.signal(signal.SIGINT, signal.SIG_IGN) - try: - os.killpg(group_id, signal.SIGINT) - finally: - signal.signal(signal.SIGINT, original_handler) - else: + # Pipelines have their own group. Never re-signal our own group: + # other ProcReader callers may share it and already received Ctrl-C. + if group_id != os.getpgrp(): + self._sigint_forwarded = True os.killpg(group_id, signal.SIGINT) except ProcessLookupError: return def terminate(self) -> None: """Terminate the process.""" - self._proc.terminate() + if self._terminal_fd is None: + self._proc.terminate() + else: + import signal + + # Popen.terminate() polls first, which would compete with our waitpid thread. + with contextlib.suppress(ProcessLookupError): + os.kill(self._proc.pid, signal.SIGTERM) + + @staticmethod + def _set_foreground_group(terminal_fd: int, group_id: int) -> None: + """Transfer the terminal without stopping this background thread with SIGTTOU.""" + import signal + + previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) + try: + os.tcsetpgrp(terminal_fd, group_id) + finally: + signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) + + @contextlib.contextmanager + def borrow_terminal(self) -> Iterator[None]: + """Let an input prompt or shell command in cmd2 temporarily use the pipeline's terminal.""" + terminal_fd = self._terminal_fd + if terminal_fd is None or os.tcgetpgrp(terminal_fd) != self._proc.pid: + yield + return + self._terminal_available.clear() + try: + self._set_foreground_group(terminal_fd, self._original_group) + yield + finally: + try: + if self._proc.returncode is None: + self._set_foreground_group(terminal_fd, self._proc.pid) + finally: + self._terminal_available.set() + + def _wait_for_job(self, terminal_fd: int) -> None: + """Reap a foreground pipeline and relay its stops to the outer shell's job. + + This is the only waitpid caller for a terminal pipeline. Watching on a separate + thread also catches Ctrl-Z while the command is blocked writing to its pipe. + """ + import signal + + try: + while True: + _, status = os.waitpid(self._proc.pid, os.WUNTRACED) + if not os.WIFSTOPPED(status): + self._proc.returncode = os.waitstatus_to_exitcode(status) + return + if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): + # A cmd2 prompt can borrow the terminal while the producer runs. + # Defer the pipeline's terminal reads until that prompt returns. + self._terminal_available.wait() + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + # This also handles a pending stop from the startup handoff. + os.killpg(self._proc.pid, signal.SIGCONT) + continue + + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + # Stop every terminal reader before returning control to the outer shell. + os.killpg(self._proc.pid, signal.SIGSTOP) + os.killpg(os.getpgrp(), signal.SIGSTOP) + # Execution resumes here when the outer shell continues cmd2. A `bg` + # must not steal the terminal from that shell. + if os.tcgetpgrp(terminal_fd) == self._original_group: + self._set_foreground_group(terminal_fd, self._proc.pid) + os.killpg(self._proc.pid, signal.SIGCONT) + finally: + try: + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + if self._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT) and not self._sigint_forwarded: + # Ctrl-C killed the consumer. Interrupt a command still producing + # output (or sleeping) too. The consumer has been reaped, so the + # main thread's handler cannot forward this signal back to it. + os.kill(os.getpid(), signal.SIGINT) + finally: + self._process_done.set() + + def wait_for_exit(self, timeout: float | None = None) -> None: + """Wait for process exit without competing with the terminal job's waitpid thread. + + :param timeout: maximum seconds to wait, or None to wait indefinitely + :raises subprocess.TimeoutExpired: if the process is still running after timeout + """ + if self._terminal_fd is None: + self._proc.wait(timeout) + elif not self._process_done.wait(timeout) and timeout is not None: + raise subprocess.TimeoutExpired(self._proc.args, timeout) def wait(self) -> None: """Wait for the process to finish.""" + if self._terminal_fd is not None: + self.wait_for_exit() if self._out_thread.is_alive(): self._out_thread.join() if self._err_thread.is_alive(): @@ -624,7 +730,7 @@ def _reader_thread_func(self, read_stdout: bool) -> None: raise ValueError("read_stream is None") # Run until process completes - while self._proc.poll() is None: + while (self._proc.poll() if self._terminal_fd is None else self._proc.returncode) is None: available = read_stream.peek() # type: ignore[attr-defined, ty:unresolved-attribute] if available: read_stream.read(len(available)) diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 8555aacaf..6b9c8feb5 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -133,7 +133,10 @@ def test_pipeline_process_group_selection(toolbar_app, tmp_path, monkeypatch, ru assert options["creationflags"] == subprocess.CREATE_NEW_PROCESS_GROUP assert "start_new_session" not in options else: - assert options["start_new_session"] == (not (stdout_tty or stderr_tty)) + # A stream claiming isatty() is insufficient: these files do not + # refer to our controlling terminal, so no foreground handoff is safe. + assert options["start_new_session"] + assert "process_group" not in options @pytest.mark.parametrize("builtin_pager", [False, True]) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 747318928..bd06ded09 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -19,8 +19,10 @@ pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") -@pytest.mark.parametrize("finish", ["q", "\x03", "process_sigint"]) -def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None: +@pytest.mark.parametrize("finish", ["q", "\x03", "process_sigint", "group_sigint", "exit_sigint", "read_input", "shell_input"]) +@pytest.mark.parametrize("stop_job", [False, True]) +@pytest.mark.parametrize("shell_child", [False, True]) +def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child) -> None: import fcntl import pty import struct @@ -31,10 +33,11 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None pytest.skip("requires an interactive bash shell") pager = tmp_path / "pager.py" pager_pid = tmp_path / "pager.pid" + interrupts = tmp_path / "interrupts" pager.write_text( "import os, pathlib, signal, sys, termios, tty\n" f"pathlib.Path({str(pager_pid)!r}).write_text(str(os.getpid()))\n" - "sys.stdin.read()\n" + f"if {finish != 'exit_sigint'!r}: sys.stdin.read()\n" # Like less, use an inherited terminal descriptor for keyboard input when # stdin is a pipe. This also works in the broken detached-session case. "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" @@ -46,6 +49,12 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None " tty.setcbreak(terminal)\n" " os.write(1, b'PAGER_RESUMED\\n')\n" " signal.signal(signal.SIGCONT, resume)\n" + " def interrupt(*args):\n" + f" fd = os.open({str(interrupts)!r}, os.O_WRONLY | os.O_CREAT | os.O_APPEND, 0o600)\n" + " os.write(fd, b'I')\n" + " os.close(fd)\n" + " os.write(1, b'PAGER_INTERRUPT\\n')\n" + f" signal.signal(signal.SIGINT, {'signal.SIG_DFL' if finish == 'exit_sigint' else 'interrupt'})\n" " try:\n" " tty.setcbreak(terminal)\n" " os.write(1, b'PAGER_READY\\n')\n" @@ -58,7 +67,14 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None application = tmp_path / "application.py" application.write_text( "from cmd2 import Cmd, ToolbarMode\n" - "app = Cmd(bottom_toolbar_mode=ToolbarMode.RESERVED)\n" + "import os, time\n" + "class App(Cmd):\n" + " def do_busy(self, statement):\n" + " os.write(2, b'BUSY_READY\\n')\n" + " time.sleep(30)\n" + " def do_ask(self, statement):\n" + " self.poutput(self.read_input('INPUT> '))\n" + "app = App(bottom_toolbar_mode=ToolbarMode.RESERVED)\n" "app.prompt = 'TEST> '\n" "app.main_session.bottom_toolbar = 'STATUS'\n" "app.cmdloop()\n", @@ -66,6 +82,10 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish) -> None ) master, slave = pty.openpty() fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 24, 80, 0, 0)) + if finish == "exit_sigint": + settings = termios.tcgetattr(slave) + settings[3] |= termios.TOSTOP + termios.tcsetattr(slave, termios.TCSANOW, settings) # Establish a controlling terminal in a fresh interpreter, avoiding preexec_fn # (unsafe when pytest or its plugins have started threads). bootstrap = ( @@ -112,16 +132,35 @@ def stopped(*pids: int) -> bool: return len(states) == len(pids) and all(state.startswith("T") for state in states) job_group = None + pipeline_group = None try: wait_until(lambda: "OUTER> " in transcript) send(f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n") wait_until(lambda: screen.display[-1].startswith("STATUS")) job_group = os.tcgetpgrp(master) - send(f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n") + command = "busy" if finish == "exit_sigint" else "help -v" + if finish == "read_input": + command = "ask" + elif finish == "shell_input": + input_script = tmp_path / "input.py" + input_script.write_text("import os\nos.write(2, b'INPUT> ')\ninput()\n", encoding="utf-8") + command = f"shell {shlex.quote(sys.executable)} {shlex.quote(str(input_script))}" + pipe_command = f"{shlex.quote(sys.executable)} {shlex.quote(str(pager))}" + if shell_child: + # Keep a shell between Popen and the terminal reader, rather than allowing + # the final command to replace it with exec. + pipe_command = f"{shlex.quote(shell)} -c {shlex.quote(pipe_command + '; :')}" + send(f"{command} | {pipe_command}\n") + if finish in ("read_input", "shell_input"): + wait_until(lambda: any(line.startswith("INPUT>") for line in screen.display)) + send("answer\n") # Whole lines only: a traceback naming the marker must not satisfy the wait. wait_until(lambda: "PAGER_READY\r\n" in transcript) + if finish == "exit_sigint": + wait_until(lambda: "BUSY_READY\r\n" in transcript) pager_process = int(pager_pid.read_text()) - for rows in (12, 24): + pipeline_group = os.tcgetpgrp(master) + for rows in (12, 24) if stop_job else (): send("\x1a") wait_until(lambda: os.tcgetpgrp(master) == process.pid) wait_until(lambda: stopped(job_group, pager_process)) @@ -139,13 +178,26 @@ def stopped(*pids: int) -> bool: start = len(transcript) send("fg\n") wait_until(lambda start=start: "PAGER_RESUMED\r\n" in transcript[start:]) - assert os.tcgetpgrp(master) == job_group - if finish == "process_sigint": - # Signal cmd2 alone, as with `kill -INT `. Its pipeline - # shares the terminal group but must receive a forwarded SIGINT. - os.kill(job_group, signal.SIGINT) - else: - send(finish) + assert os.tcgetpgrp(master) == pipeline_group + if finish == "exit_sigint": + send("\x03") + elif finish in ("\x03", "process_sigint", "group_sigint"): + for expected_count in (1, 2): + if finish == "process_sigint": + # Signal cmd2 alone, as with `kill -INT `. + os.kill(job_group, signal.SIGINT) + elif finish == "group_sigint": + os.killpg(pipeline_group, signal.SIGINT) + else: + send(finish) + wait_until(lambda count=expected_count: transcript.count("PAGER_INTERRUPT\r\n") >= count) + # Keep the handler alive long enough to observe a duplicate delivery, + # then also check that a second real interrupt is not suppressed. + deadline = time.monotonic() + 0.1 + wait_until(lambda deadline=deadline: time.monotonic() >= deadline) + assert interrupts.read_text() == "I" * expected_count + if finish != "exit_sigint": + send("q") wait_until(lambda: screen.display[-1].startswith("STATUS") and "TEST>" in "\n".join(screen.display)) start = len(transcript) send("help quit\n") @@ -160,6 +212,9 @@ def stopped(*pids: int) -> bool: if job_group is not None: with contextlib.suppress(ProcessLookupError): os.killpg(job_group, signal.SIGKILL) + if pipeline_group is not None: + with contextlib.suppress(ProcessLookupError): + os.killpg(pipeline_group, signal.SIGKILL) process.kill() process.wait(timeout=5) os.close(master) diff --git a/tests/test_utils.py b/tests/test_utils.py index ea73082ee..9c2ce01c6 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -220,22 +220,11 @@ def test_proc_reader_send_sigint(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") -@pytest.mark.parametrize("group_exited", [False, True]) -def test_proc_reader_forwards_to_its_own_group(pr_none, group_exited) -> None: - original_handler = signal.getsignal(signal.SIGINT) - - def forward(group_id, signum): - assert group_id == os.getpgrp() - assert signum == signal.SIGINT - assert signal.getsignal(signal.SIGINT) == signal.SIG_IGN - if group_exited: - raise ProcessLookupError - +def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: try: - with mock.patch("os.getpgid", return_value=os.getpgrp()), mock.patch("os.killpg", side_effect=forward) as killpg: + with mock.patch("os.getpgid", return_value=os.getpgrp()), mock.patch("os.killpg") as killpg: pr_none.send_sigint() - killpg.assert_called_once_with(os.getpgrp(), signal.SIGINT) - assert signal.getsignal(signal.SIGINT) == original_handler + killpg.assert_not_called() finally: pr_none.terminate() pr_none.wait() From 4a79cb4f43e353289d68df326cf5e59e4843519a Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 16:33:28 -0400 Subject: [PATCH 14/44] Fix POSIX pipeline signal races and speed up terminal tests --- cmd2/utils.py | 13 ++++++- pyproject.toml | 2 + tests/test_pipeline_job_control.py | 61 ++++++++++++++++++++++++------ 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index 38e20aed7..203d564aa 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -666,7 +666,10 @@ def _wait_for_job(self, terminal_fd: int) -> None: self._set_foreground_group(terminal_fd, self._original_group) # Stop every terminal reader before returning control to the outer shell. os.killpg(self._proc.pid, signal.SIGSTOP) - os.killpg(os.getpgrp(), signal.SIGSTOP) + # Target this thread so the whole process stops before this call returns. + # killpg can deliver SIGSTOP to another thread on Linux, allowing this + # worker to run ahead and resume the pipeline before cmd2 has stopped. + signal.raise_signal(signal.SIGSTOP) # Execution resumes here when the outer shell continues cmd2. A `bg` # must not steal the terminal from that shell. if os.tcgetpgrp(terminal_fd) == self._original_group: @@ -692,7 +695,13 @@ def wait_for_exit(self, timeout: float | None = None) -> None: """ if self._terminal_fd is None: self._proc.wait(timeout) - elif not self._process_done.wait(timeout) and timeout is not None: + elif timeout is None: + # A process-directed signal may reach a worker thread. Python still runs + # its handler on the main thread, so periodically return from the wait + # to dispatch it even when the main thread's system call was not interrupted. + while not self._process_done.wait(0.1): + pass + elif not self._process_done.wait(timeout): raise subprocess.TimeoutExpired(self._proc.args, timeout) def wait(self) -> None: diff --git a/pyproject.toml b/pyproject.toml index e3ce52543..59a42147e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -106,6 +106,8 @@ warn_unused_ignores = false testpaths = ["tests"] addopts = [ "-n=auto", + # Spread slow terminal cases across workers instead of queuing them in one batch. + "--maxschedchunk=1", "--cov=cmd2", "--cov-config=pyproject.toml", "--cov-report=xml", diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index bd06ded09..3d7faa559 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -19,9 +19,17 @@ pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") -@pytest.mark.parametrize("finish", ["q", "\x03", "process_sigint", "group_sigint", "exit_sigint", "read_input", "shell_input"]) -@pytest.mark.parametrize("stop_job", [False, True]) -@pytest.mark.parametrize("shell_child", [False, True]) +@pytest.mark.parametrize( + ("finish", "stop_job", "shell_child"), + [ + pytest.param("interrupts", True, False, id="direct-signals-and-job-control"), + pytest.param("interrupts", True, True, id="shell-signals-and-job-control"), + pytest.param("exit_sigint", False, False, id="interrupt-busy-producer"), + pytest.param("exit_sigint", True, True, id="stop-and-interrupt-busy-producer"), + pytest.param("read_input", False, False, id="nested-prompt"), + pytest.param("shell_input", False, True, id="shell-input"), + ], +) def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child) -> None: import fcntl import pty @@ -65,9 +73,12 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo encoding="utf-8", ) application = tmp_path / "application.py" + application_pid = tmp_path / "application.pid" + interrupt_request = tmp_path / "interrupt.request" application.write_text( "from cmd2 import Cmd, ToolbarMode\n" - "import os, time\n" + "import os, pathlib, signal, threading, time\n" + f"pathlib.Path({str(application_pid)!r}).write_text(str(os.getpid()))\n" "class App(Cmd):\n" " def do_busy(self, statement):\n" " os.write(2, b'BUSY_READY\\n')\n" @@ -77,6 +88,17 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo "app = App(bottom_toolbar_mode=ToolbarMode.RESERVED)\n" "app.prompt = 'TEST> '\n" "app.main_session.bottom_toolbar = 'STATUS'\n" + f"if {finish == 'interrupts'!r}:\n" + " def interrupt_from_worker():\n" + f" request = pathlib.Path({str(interrupt_request)!r})\n" + " for _ in range(2):\n" + " while not request.exists():\n" + " time.sleep(0.01)\n" + " request.unlink()\n" + # A process-directed signal can be delivered to any unblocked thread. + # Force that case so an indefinite main-thread wait cannot pass by luck. + " signal.pthread_kill(threading.get_ident(), signal.SIGINT)\n" + " threading.Thread(target=interrupt_from_worker, daemon=True).start()\n" "app.cmdloop()\n", encoding="utf-8", ) @@ -137,7 +159,13 @@ def stopped(*pids: int) -> bool: wait_until(lambda: "OUTER> " in transcript) send(f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n") wait_until(lambda: screen.display[-1].startswith("STATUS")) - job_group = os.tcgetpgrp(master) + # A foreground-group query is an observation, not the child's identity. + # It can change during startup and handoffs. Never use an unverified + # foreground query as a kill()/killpg() destination. + job_group = int(application_pid.read_text()) + assert job_group > 1 + assert os.getpgid(job_group) == job_group + wait_until(lambda: os.tcgetpgrp(master) == job_group) command = "busy" if finish == "exit_sigint" else "help -v" if finish == "read_input": command = "ask" @@ -159,7 +187,9 @@ def stopped(*pids: int) -> bool: if finish == "exit_sigint": wait_until(lambda: "BUSY_READY\r\n" in transcript) pager_process = int(pager_pid.read_text()) - pipeline_group = os.tcgetpgrp(master) + pipeline_group = os.getpgid(pager_process) + assert pipeline_group > 1 + assert pipeline_group != job_group for rows in (12, 24) if stop_job else (): send("\x1a") wait_until(lambda: os.tcgetpgrp(master) == process.pid) @@ -181,15 +211,20 @@ def stopped(*pids: int) -> bool: assert os.tcgetpgrp(master) == pipeline_group if finish == "exit_sigint": send("\x03") - elif finish in ("\x03", "process_sigint", "group_sigint"): - for expected_count in (1, 2): - if finish == "process_sigint": + elif finish == "interrupts": + # Exercise every signal route on this live pipeline, avoiding a fresh + # interpreter and terminal setup for each overlapping matrix combination. + sources = ("terminal", "terminal", "process", "process", "thread", "thread", "group", "group") + for expected_count, source in enumerate(sources, start=1): + if source == "process": # Signal cmd2 alone, as with `kill -INT `. os.kill(job_group, signal.SIGINT) - elif finish == "group_sigint": + elif source == "thread": + interrupt_request.touch() + elif source == "group": os.killpg(pipeline_group, signal.SIGINT) else: - send(finish) + send("\x03") wait_until(lambda count=expected_count: transcript.count("PAGER_INTERRUPT\r\n") >= count) # Keep the handler alive long enough to observe a duplicate delivery, # then also check that a second real interrupt is not suppressed. @@ -215,6 +250,8 @@ def stopped(*pids: int) -> bool: if pipeline_group is not None: with contextlib.suppress(ProcessLookupError): os.killpg(pipeline_group, signal.SIGKILL) + # Release the PTY before reaping its session leader. On macOS, waiting + # while the master is still open can leave terminal teardown blocked. + os.close(master) process.kill() process.wait(timeout=5) - os.close(master) From 66365f5851f2882b16bf0d6c42eb8140f899b6bc Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 16:50:42 -0400 Subject: [PATCH 15/44] Collect coverage from terminal test subprocesses --- pyproject.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 59a42147e..cd68a4b1f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -116,6 +116,8 @@ addopts = [ ] [tool.coverage.run] +# Include the cmd2 applications launched by the terminal integration tests. +patch = ["subprocess"] # Use sys.monitoring on Python 3.12+; coverage falls back with a warning on 3.11. core = "sysmon" source = ["cmd2"] From 004f8355a253729718c545ae2ff5c1268c760e24 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 12 Sep 2026 16:57:47 -0400 Subject: [PATCH 16/44] Cover terminal pipeline cleanup and resume paths --- tests/test_cmd2.py | 35 +++++++++++++++++++++++++++++---- tests/test_utils.py | 47 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index ec75d35f2..961963842 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -865,7 +865,11 @@ def test_pipe_to_shell_and_redirect(redirection_app, running_pipe_process) -> No os.remove(filename) -def test_pipe_to_shell_error(redirection_app, mocker, capsys) -> None: +@pytest.mark.parametrize( + "terminal", + [False, pytest.param(True, marks=pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control"))], +) +def test_pipe_to_shell_error(redirection_app, mocker, capsys, terminal) -> None: """An already-exited pipe process must be reported before the command runs. A real nonexistent command may take longer than the startup probe under load. @@ -876,12 +880,35 @@ def test_pipe_to_shell_error(redirection_app, mocker, capsys) -> None: process = popen.return_value process.returncode = 127 process.wait.return_value = 127 - - out, err = run_cmd(redirection_app, "print_output | foobarbaz.this_does_not_exist") + if terminal: + target = mocker.Mock() + target.isatty.return_value = True + target.fileno.return_value = 10 + mocker.patch("cmd2.command_toolbar.pipe_target", return_value=target) + mocker.patch("os.tcgetpgrp", return_value=os.getpgrp()) + sigmask = mocker.patch("signal.pthread_sigmask", return_value=set()) + reader = mocker.patch("cmd2.utils.ProcReader").return_value + + if terminal: + # run_cmd captures stderr in a StdSim, which deliberately disables terminal handoff. + redirection_app.onecmd_plus_hooks("print_output | foobarbaz.this_does_not_exist") + out, error_text = capsys.readouterr() + err = error_text.splitlines() + else: + out, err = run_cmd(redirection_app, "print_output | foobarbaz.this_does_not_exist") assert not out assert "Pipe process exited with code 127 before command could run" in " ".join(err) assert capsys.readouterr().out == "" - process.wait.assert_called_once() + if terminal: + reader.wait_for_exit.assert_called_once_with(0.2) + reader.wait.assert_called_once_with() + process.wait.assert_not_called() + assert sigmask.call_args_list == [ + mock.call(signal.SIG_BLOCK, {signal.SIGTTOU}), + mock.call(signal.SIG_SETMASK, set()), + ] + else: + process.wait.assert_called_once() assert popen.call_args.kwargs["stdin"].closed diff --git a/tests/test_utils.py b/tests/test_utils.py index 9c2ce01c6..9590bc8fc 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -249,6 +249,53 @@ def test_proc_reader_terminate(pr_none) -> None: assert ret_code == -signal.SIGTERM +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("already_exited", [False, True]) +def test_proc_reader_terminate_terminal_job(already_exited) -> None: + proc = mock.Mock(stdout=None, stderr=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + with mock.patch("os.kill", side_effect=ProcessLookupError if already_exited else None) as kill: + reader.terminate() + kill.assert_called_once_with(proc.pid, signal.SIGTERM) + # Only the job watcher may reap this process; Popen.terminate() would poll it. + proc.terminate.assert_not_called() + proc.poll.assert_not_called() + proc.wait.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("stop_signal", ["SIGTTIN", "SIGTTOU"]) +def test_proc_reader_resumes_terminal_access_after_borrow(stop_signal) -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + stopped_status = (getattr(signal, stop_signal) << 8) | 0x7F + with ( + mock.patch("os.waitpid", side_effect=[(proc.pid, stopped_status), (proc.pid, 0)]), + mock.patch("os.tcgetpgrp", return_value=proc.pid), + mock.patch.object(reader, "_set_foreground_group") as foreground, + mock.patch.object(reader._terminal_available, "wait", return_value=True) as available, + mock.patch("os.killpg") as killpg, + mock.patch("signal.raise_signal") as stop, + ): + reader._wait_for_job(10) + available.assert_called_once_with() + killpg.assert_called_once_with(proc.pid, signal.SIGCONT) + stop.assert_not_called() + foreground.assert_called_once_with(10, reader._original_group) + assert proc.returncode == 0 + assert reader._process_done.is_set() + + +def test_proc_reader_wait_for_exit_without_terminal() -> None: + proc = mock.Mock(stdout=None, stderr=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader.wait_for_exit(timeout=0.2) + proc.wait.assert_called_once_with(0.2) + + @pytest.fixture def context_flag(): return cu.ContextFlag() From e327df616cb572df09bb1f1ffc732a1db14bd422 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 11:18:57 -0400 Subject: [PATCH 17/44] Fix terminal input and wrapper job control in pipelines --- cmd2/cmd2.py | 27 +++-- cmd2/utils.py | 161 ++++++++++++++++++++++------- tests/test_pipeline_job_control.py | 91 ++++++++++++---- tests/test_utils.py | 158 +++++++++++++++++++++++++++- 4 files changed, 362 insertions(+), 75 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 9fbf15c53..dcbda0965 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -2183,11 +2183,7 @@ def suspend_bottom_toolbar(self) -> Iterator[None]: inherits the terminal knows nothing about a scroll region and would find its output confined to rows it never asked for. The rows are taken again afterwards. """ - reader = self._cur_pipe_proc_reader - with ( - reader.borrow_terminal() if reader is not None else contextlib.nullcontext(), - self._quiesce_bottom_toolbar(), - ): + with self._quiesce_bottom_toolbar(): reserved = self._reserved_toolbar if reserved is None: @@ -3583,6 +3579,9 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: shell=True, **kwargs, ) + # Only the child should own a read end. In particular, a consumer + # exit must unblock a producer writing to a full pipe immediately. + subproc_stdin.close() if terminal_fd is not None: import signal @@ -3592,6 +3591,7 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) terminal_stack.callback(signal.pthread_sigmask, signal.SIG_SETMASK, previous_mask) cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, terminal_fd=terminal_fd) + terminal_stack.enter_context(cmd_pipe_proc_reader.manage_terminal()) # Popen was called with shell=True so the user can chain pipe commands and redirect their output # like: !ls -l | grep user | wc -l > out.txt. But this makes it difficult to know if the pipe process @@ -3614,6 +3614,15 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: if cmd_pipe_proc_reader is None: cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) + if terminal_fd is not None: + import io + + pipe_fd = os.dup(new_stdout.fileno()) + new_stdout.close() + new_stdout = io.TextIOWrapper( + io.BufferedWriter(utils.PipelineWriter(pipe_fd, cmd_pipe_proc_reader)), encoding="utf-8" + ) + self.stdout = new_stdout # Hold the suspension open until _restore_output() reaps the pipe process. @@ -3693,6 +3702,8 @@ def _restore_output(self, statement: Statement, saved_redir_state: utils.Redirec with contextlib.suppress(BrokenPipeError): # Close the file or pipe that stdout was redirected to + if self._cur_pipe_proc_reader is not None: + self._cur_pipe_proc_reader.finish_producer() self.stdout.close() # Restore self.stdout @@ -3833,11 +3844,7 @@ def _read_raw_input( """ reserved = self._reserved_toolbar owns_the_reservation = session is self.main_session or (reserved is not None and reserved.can_manage(session)) - reader = self._cur_pipe_proc_reader - with ( - reader.borrow_terminal() if reader is not None else contextlib.nullcontext(), - self._quiesce_bottom_toolbar() if owns_the_reservation else self.suspend_bottom_toolbar(), - ): + with self._quiesce_bottom_toolbar() if owns_the_reservation else self.suspend_bottom_toolbar(): if owns_the_reservation and reserved is not None and reserved.bridge is not None: reserved.bridge.finish_command_output() with reserved.prompt_session(session): diff --git a/cmd2/utils.py b/cmd2/utils.py index 203d564aa..2e6d47c06 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -1,9 +1,11 @@ """Shared utility functions.""" import contextlib +import errno import functools import glob import inspect +import io import itertools import os import re @@ -555,18 +557,12 @@ def __init__( self._stderr = stderr self._terminal_fd = terminal_fd self._process_done = threading.Event() - self._sigint_forwarded = False + self._producer_finished = False self._terminal_available = threading.Event() - self._terminal_available.set() + self._terminal_lock = threading.RLock() + self._job_resumed = threading.Event() if terminal_fd is not None: - import signal - self._original_group = os.tcgetpgrp(terminal_fd) - self._set_foreground_group(terminal_fd, proc.pid) - # The child may have tried to read before we could give it the terminal. - with contextlib.suppress(ProcessLookupError): - os.killpg(proc.pid, signal.SIGCONT) - threading.Thread(name="pipe_job", target=self._wait_for_job, args=(terminal_fd,), daemon=True).start() self._out_thread = threading.Thread(name="out_thread", target=self._reader_thread_func, kwargs={"read_stdout": True}) @@ -594,7 +590,6 @@ def send_sigint(self) -> None: # Pipelines have their own group. Never re-signal our own group: # other ProcReader callers may share it and already received Ctrl-C. if group_id != os.getpgrp(): - self._sigint_forwarded = True os.killpg(group_id, signal.SIGINT) except ProcessLookupError: return @@ -622,22 +617,75 @@ def _set_foreground_group(terminal_fd: int, group_id: int) -> None: signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) @contextlib.contextmanager - def borrow_terminal(self) -> Iterator[None]: - """Let an input prompt or shell command in cmd2 temporarily use the pipeline's terminal.""" + def manage_terminal(self) -> Iterator[None]: + """Watch the pipeline and suspend the shell's whole job on the main thread.""" + import signal + terminal_fd = self._terminal_fd - if terminal_fd is None or os.tcgetpgrp(terminal_fd) != self._proc.pid: + if terminal_fd is None: yield return - self._terminal_available.clear() + previous_handler = signal.getsignal(signal.SIGTSTP) + + def suspend_job(signum: int, frame: Any) -> None: + try: + if previous_handler != signal.SIG_DFL: + if callable(previous_handler): + previous_handler(signum, frame) + return + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + # Ignore our group-directed copy, then stop this thread synchronously. + # Wrappers in our job must stop too. Unlike SIGSTOP, SIGTSTP is + # discarded for orphaned groups, which have no shell to resume them. + signal.signal(signal.SIGTSTP, signal.SIG_IGN) + try: + os.killpg(self._original_group, signal.SIGTSTP) + signal.signal(signal.SIGTSTP, signal.SIG_DFL) + signal.raise_signal(signal.SIGTSTP) + finally: + signal.signal(signal.SIGTSTP, suspend_job) + finally: + try: + if self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._original_group: + self._set_foreground_group(terminal_fd, self._proc.pid) + finally: + self._job_resumed.set() + + signal.signal(signal.SIGTSTP, suspend_job) try: - self._set_foreground_group(terminal_fd, self._original_group) + threading.Thread(name="pipe_job", target=self._wait_for_job, args=(terminal_fd,), daemon=True).start() yield finally: + signal.signal(signal.SIGTSTP, previous_handler) + + @contextlib.contextmanager + def lend_terminal(self) -> Iterator[None]: + """Lend the terminal only while writing to or waiting for the consumer. + + Command code retains foreground access between writes, including arbitrary + reads through input(), getpass(), or third-party libraries. Lending during + writes lets an interactive consumer drain a full pipe without deadlocking. + """ + terminal_fd = self._terminal_fd + if terminal_fd is None or self._proc.returncode is not None: + yield + return + with self._terminal_lock: try: - if self._proc.returncode is None: - self._set_foreground_group(terminal_fd, self._proc.pid) - finally: - self._terminal_available.set() + self._set_foreground_group(terminal_fd, self._proc.pid) + except OSError as error: + # The group can disappear before the watcher has reaped its leader. + if error.errno not in (errno.ESRCH, errno.EINVAL): + raise + self._terminal_available.set() + try: + yield + finally: + with self._terminal_lock: + self._terminal_available.clear() + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) def _wait_for_job(self, terminal_fd: int) -> None: """Reap a foreground pipeline and relay its stops to the outer shell's job. @@ -654,39 +702,47 @@ def _wait_for_job(self, terminal_fd: int) -> None: self._proc.returncode = os.waitstatus_to_exitcode(status) return if os.WSTOPSIG(status) in (signal.SIGTTIN, signal.SIGTTOU): - # A cmd2 prompt can borrow the terminal while the producer runs. - # Defer the pipeline's terminal reads until that prompt returns. - self._terminal_available.wait() - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - # This also handles a pending stop from the startup handoff. - os.killpg(self._proc.pid, signal.SIGCONT) + # Command code owns the terminal between pipe writes. Defer + # consumer terminal access until the next write or final wait. + while True: + self._terminal_available.wait(0.1) + with self._terminal_lock: + # A stopped consumer can be killed before another write. + # Keep reaping even while command code owns the terminal. + pid, pending_status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + if pid and not os.WIFSTOPPED(pending_status): + self._proc.returncode = os.waitstatus_to_exitcode(pending_status) + return + # A short write may already have returned the terminal. + # Do not turn that ordinary handoff into a job suspension. + if not self._terminal_available.is_set(): + continue + foreground = os.tcgetpgrp(terminal_fd) + if foreground == self._proc.pid: + os.killpg(self._proc.pid, signal.SIGCONT) + break + if foreground == self._proc.pid: continue if os.tcgetpgrp(terminal_fd) == self._proc.pid: self._set_foreground_group(terminal_fd, self._original_group) # Stop every terminal reader before returning control to the outer shell. os.killpg(self._proc.pid, signal.SIGSTOP) - # Target this thread so the whole process stops before this call returns. - # killpg can deliver SIGSTOP to another thread on Linux, allowing this - # worker to run ahead and resume the pipeline before cmd2 has stopped. - signal.raise_signal(signal.SIGSTOP) - # Execution resumes here when the outer shell continues cmd2. A `bg` - # must not steal the terminal from that shell. - if os.tcgetpgrp(terminal_fd) == self._original_group: - self._set_foreground_group(terminal_fd, self._proc.pid) + self._job_resumed.clear() + os.kill(os.getpid(), signal.SIGTSTP) + self._job_resumed.wait() os.killpg(self._proc.pid, signal.SIGCONT) finally: try: if os.tcgetpgrp(terminal_fd) == self._proc.pid: self._set_foreground_group(terminal_fd, self._original_group) - if self._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT) and not self._sigint_forwarded: - # Ctrl-C killed the consumer. Interrupt a command still producing - # output (or sleeping) too. The consumer has been reaped, so the - # main thread's handler cannot forward this signal back to it. - os.kill(os.getpid(), signal.SIGINT) finally: self._process_done.set() + def finish_producer(self) -> None: + """Disable producer cancellation before flushing and closing its pipe.""" + self._producer_finished = True + def wait_for_exit(self, timeout: float | None = None) -> None: """Wait for process exit without competing with the terminal job's waitpid thread. @@ -707,7 +763,8 @@ def wait_for_exit(self, timeout: float | None = None) -> None: def wait(self) -> None: """Wait for the process to finish.""" if self._terminal_fd is not None: - self.wait_for_exit() + with self.lend_terminal(): + self.wait_for_exit() if self._out_thread.is_alive(): self._out_thread.join() if self._err_thread.is_alive(): @@ -760,6 +817,32 @@ def _write_bytes(stream: StdSim | TextIO, to_write: bytes | str) -> None: stream.buffer.write(to_write) +class PipelineWriter(io.FileIO): + """A pipe whose blocking writes temporarily give the consumer terminal access.""" + + def __init__(self, fd: int, reader: ProcReader) -> None: + """Take ownership of a pipe descriptor managed by reader.""" + super().__init__(fd, "w") + self._reader = reader + + def write(self, b: Any) -> int: + """Write bytes while the consumer can interact with the terminal.""" + import signal + + try: + with self._reader.lend_terminal(): + return cast(int, super().write(b)) + except BrokenPipeError: + # Ctrl-C during a blocking write must cancel the command, even if it + # normally catches BrokenPipeError. Raise here rather than signaling + # asynchronously: a late signal could interrupt redirection cleanup. + with contextlib.suppress(subprocess.TimeoutExpired): + self._reader.wait_for_exit(0.2) + if not self._reader._producer_finished and self._reader._proc.returncode in (-signal.SIGINT, 128 + signal.SIGINT): + raise KeyboardInterrupt from None + raise + + class ContextFlag: """A context manager which is also used as a boolean flag value within the default sigint handler. diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 3d7faa559..cf21be86a 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -20,17 +20,20 @@ @pytest.mark.parametrize( - ("finish", "stop_job", "shell_child"), + ("finish", "stop_job", "shell_child", "launcher"), [ - pytest.param("interrupts", True, False, id="direct-signals-and-job-control"), - pytest.param("interrupts", True, True, id="shell-signals-and-job-control"), - pytest.param("exit_sigint", False, False, id="interrupt-busy-producer"), - pytest.param("exit_sigint", True, True, id="stop-and-interrupt-busy-producer"), - pytest.param("read_input", False, False, id="nested-prompt"), - pytest.param("shell_input", False, True, id="shell-input"), + pytest.param("interrupts", True, False, "direct", id="direct-signals-and-job-control"), + pytest.param("interrupts", True, True, "sh", id="wrapper-signals-and-job-control"), + pytest.param("exit_sigint", False, False, "direct", id="interrupt-busy-producer"), + pytest.param("exit_sigint", True, True, "uv", id="uv-stop-and-interrupt-busy-producer"), + pytest.param("read_input", False, False, "direct", id="nested-prompt"), + pytest.param("shell_input", False, True, "direct", id="shell-input"), + pytest.param("direct_input", False, False, "sh", id="direct-input-with-toolbar-off"), + pytest.param("direct_input", False, False, "exec", id="direct-input-in-orphaned-session"), + pytest.param("interrupts", False, False, "exec", id="orphaned-job-control"), ], ) -def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child) -> None: +def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child, launcher) -> None: import fcntl import pty import struct @@ -43,18 +46,25 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo pager_pid = tmp_path / "pager.pid" interrupts = tmp_path / "interrupts" pager.write_text( - "import os, pathlib, signal, sys, termios, tty\n" + "import errno, os, pathlib, signal, sys, termios, tty\n" f"pathlib.Path({str(pager_pid)!r}).write_text(str(os.getpid()))\n" f"if {finish != 'exit_sigint'!r}: sys.stdin.read()\n" # Like less, use an inherited terminal descriptor for keyboard input when # stdin is a pipe. This also works in the broken detached-session case. "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" " saved = termios.tcgetattr(terminal)\n" + " def setcbreak():\n" + " while True:\n" + " try:\n" + " tty.setcbreak(terminal)\n" + " return\n" + " except termios.error as error:\n" + " if error.args[0] != errno.EINTR: raise\n" # os.write rather than print: a signal handler that uses buffered stdout raises # "reentrant call inside <_io.BufferedWriter>" when the signal lands mid-write, # which happens when the job is stopped while still reporting readiness. " def resume(*args):\n" - " tty.setcbreak(terminal)\n" + " setcbreak()\n" " os.write(1, b'PAGER_RESUMED\\n')\n" " signal.signal(signal.SIGCONT, resume)\n" " def interrupt(*args):\n" @@ -64,7 +74,7 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo " os.write(1, b'PAGER_INTERRUPT\\n')\n" f" signal.signal(signal.SIGINT, {'signal.SIG_DFL' if finish == 'exit_sigint' else 'interrupt'})\n" " try:\n" - " tty.setcbreak(terminal)\n" + " setcbreak()\n" " os.write(1, b'PAGER_READY\\n')\n" " while os.read(terminal.fileno(), 1) != b'q':\n" " pass\n" @@ -77,16 +87,29 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo interrupt_request = tmp_path / "interrupt.request" application.write_text( "from cmd2 import Cmd, ToolbarMode\n" - "import os, pathlib, signal, threading, time\n" + "import getpass, os, pathlib, signal, threading, time\n" + "signal.signal(signal.SIGTSTP, signal.SIG_DFL)\n" f"pathlib.Path({str(application_pid)!r}).write_text(str(os.getpid()))\n" "class App(Cmd):\n" " def do_busy(self, statement):\n" " os.write(2, b'BUSY_READY\\n')\n" + " self.stdout.write('x' * 262144)\n" + " self.stdout.flush()\n" " time.sleep(30)\n" " def do_ask(self, statement):\n" " self.poutput(self.read_input('INPUT> '))\n" - "app = App(bottom_toolbar_mode=ToolbarMode.RESERVED)\n" + " def do_direct(self, statement):\n" + " self.stdout.buffer.write(b'x' * 262144)\n" + " self.stdout.flush()\n" + " assert self.select('first second', 'SELECT> ') == 'first'\n" + " assert input('PLAIN> ') == 'answer'\n" + " assert getpass.getpass('SECRET> ') == 'secret'\n" + " os.write(2, b'RAW> ')\n" + " assert os.read(0, 7) == b'direct\\n'\n" + " self.poutput('INPUT_COMPLETE')\n" + f"app = App(bottom_toolbar_mode=ToolbarMode.{'OFF' if finish == 'direct_input' else 'RESERVED'})\n" "app.prompt = 'TEST> '\n" + "app.debug = True\n" "app.main_session.bottom_toolbar = 'STATUS'\n" f"if {finish == 'interrupts'!r}:\n" " def interrupt_from_worker():\n" @@ -147,6 +170,7 @@ def stopped(*pids: int) -> bool: blocked in a one-byte terminal read is woken by the stop signal and, if a keystroke has arrived by then, consumes it before it stops. Typing has to wait for the whole job. """ + pids = tuple(set(pids)) listing = subprocess.run( ["ps", "-o", "stat=", "-p", ",".join(map(str, pids))], capture_output=True, text=True, check=False ) @@ -157,18 +181,30 @@ def stopped(*pids: int) -> bool: pipeline_group = None try: wait_until(lambda: "OUTER> " in transcript) - send(f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n") - wait_until(lambda: screen.display[-1].startswith("STATUS")) + launch = f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}" + if launcher == "sh": + launch = f"{shlex.quote(shell)} -c {shlex.quote(launch + '; :')}" + elif launcher == "uv": + uv = shutil.which("uv") + if uv is None: + pytest.skip("requires uv") + launch = f"{shlex.quote(uv)} run --no-project -- {launch}" + elif launcher == "exec": + launch = "exec " + launch + send(launch + "\n") + wait_until(lambda: "TEST>" in "\n".join(screen.display)) # A foreground-group query is an observation, not the child's identity. # It can change during startup and handoffs. Never use an unverified # foreground query as a kill()/killpg() destination. - job_group = int(application_pid.read_text()) + app_pid = int(application_pid.read_text()) + job_group = os.getpgid(app_pid) assert job_group > 1 - assert os.getpgid(job_group) == job_group wait_until(lambda: os.tcgetpgrp(master) == job_group) command = "busy" if finish == "exit_sigint" else "help -v" if finish == "read_input": command = "ask" + elif finish == "direct_input": + command = "direct" elif finish == "shell_input": input_script = tmp_path / "input.py" input_script.write_text("import os\nos.write(2, b'INPUT> ')\ninput()\n", encoding="utf-8") @@ -179,6 +215,11 @@ def stopped(*pids: int) -> bool: # the final command to replace it with exec. pipe_command = f"{shlex.quote(shell)} -c {shlex.quote(pipe_command + '; :')}" send(f"{command} | {pipe_command}\n") + if finish == "direct_input": + for prompt, response in (("SELECT>", "\r"), ("PLAIN>", "answer\n"), ("SECRET>", "secret\n"), ("RAW>", "direct\n")): + wait_until(lambda prompt=prompt: prompt in "\n".join(screen.display)) + assert os.tcgetpgrp(master) == job_group + send(response) if finish in ("read_input", "shell_input"): wait_until(lambda: any(line.startswith("INPUT>") for line in screen.display)) send("answer\n") @@ -190,10 +231,15 @@ def stopped(*pids: int) -> bool: pipeline_group = os.getpgid(pager_process) assert pipeline_group > 1 assert pipeline_group != job_group + if launcher == "exec": + start = len(transcript) + send("\x1a") + wait_until(lambda: "PAGER_RESUMED\r\n" in transcript[start:]) + assert os.tcgetpgrp(master) == pipeline_group for rows in (12, 24) if stop_job else (): send("\x1a") wait_until(lambda: os.tcgetpgrp(master) == process.pid) - wait_until(lambda: stopped(job_group, pager_process)) + wait_until(lambda: stopped(job_group, app_pid, pager_process)) start = len(transcript) # A child left running can steal these keystrokes from the shell. send("printf 'SHELL_%s\\n' OWNS_INPUT\n") @@ -218,7 +264,7 @@ def stopped(*pids: int) -> bool: for expected_count, source in enumerate(sources, start=1): if source == "process": # Signal cmd2 alone, as with `kill -INT `. - os.kill(job_group, signal.SIGINT) + os.kill(app_pid, signal.SIGINT) elif source == "thread": interrupt_request.touch() elif source == "group": @@ -233,18 +279,19 @@ def stopped(*pids: int) -> bool: assert interrupts.read_text() == "I" * expected_count if finish != "exit_sigint": send("q") - wait_until(lambda: screen.display[-1].startswith("STATUS") and "TEST>" in "\n".join(screen.display)) + wait_until(lambda: os.tcgetpgrp(master) == job_group and "TEST>" in "\n".join(screen.display)) start = len(transcript) send("help quit\n") wait_until(lambda: "Exit this application" in transcript[start:]) send("quit\n") - wait_until(lambda: os.tcgetpgrp(master) == process.pid) + if launcher != "exec": + wait_until(lambda: os.tcgetpgrp(master) == process.pid) finally: # Kill only this test's job, including stopped descendants, on assertion failure. if pager_pid.exists(): with contextlib.suppress(ProcessLookupError): os.kill(int(pager_pid.read_text()), signal.SIGKILL) - if job_group is not None: + if job_group is not None and job_group != process.pid: with contextlib.suppress(ProcessLookupError): os.killpg(job_group, signal.SIGKILL) if pipeline_group is not None: diff --git a/tests/test_utils.py b/tests/test_utils.py index 9590bc8fc..c097c8d6a 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,5 +1,6 @@ """Unit testing for cmd2/utils.py module.""" +import errno import math import os import signal @@ -230,6 +231,14 @@ def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: pr_none.wait() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_sigint_after_consumer_exit() -> None: + reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) + with mock.patch("os.getpgid", side_effect=ProcessLookupError), mock.patch("os.killpg") as killpg: + reader.send_sigint() + killpg.assert_not_called() + + def test_proc_reader_terminate(pr_none) -> None: assert pr_none._proc.poll() is None pr_none.terminate() @@ -266,22 +275,37 @@ def test_proc_reader_terminate_terminal_job(already_exited) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") @pytest.mark.parametrize("stop_signal", ["SIGTTIN", "SIGTTOU"]) -def test_proc_reader_resumes_terminal_access_after_borrow(stop_signal) -> None: +@pytest.mark.parametrize("expired_handoff", [False, True]) +def test_proc_reader_resumes_terminal_access_after_handoff(stop_signal, expired_handoff) -> None: proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) reader._terminal_fd = 10 reader._original_group = 456 + reader._terminal_available.set() stopped_status = (getattr(signal, stop_signal) << 8) | 0x7F + handoffs = iter([False, True] if expired_handoff else [True]) + + def handoff(timeout): + assert timeout == 0.1 + if next(handoffs): + reader._terminal_available.set() + else: + reader._terminal_available.clear() + return True + with ( - mock.patch("os.waitpid", side_effect=[(proc.pid, stopped_status), (proc.pid, 0)]), + mock.patch( + "os.waitpid", + side_effect=[(proc.pid, stopped_status), *([(0, 0)] * (2 if expired_handoff else 1)), (proc.pid, 0)], + ), mock.patch("os.tcgetpgrp", return_value=proc.pid), mock.patch.object(reader, "_set_foreground_group") as foreground, - mock.patch.object(reader._terminal_available, "wait", return_value=True) as available, + mock.patch.object(reader._terminal_available, "wait", side_effect=handoff) as available, mock.patch("os.killpg") as killpg, mock.patch("signal.raise_signal") as stop, ): reader._wait_for_job(10) - available.assert_called_once_with() + assert available.call_count == (2 if expired_handoff else 1) killpg.assert_called_once_with(proc.pid, signal.SIGCONT) stop.assert_not_called() foreground.assert_called_once_with(10, reader._original_group) @@ -296,6 +320,132 @@ def test_proc_reader_wait_for_exit_without_terminal() -> None: proc.wait.assert_called_once_with(0.2) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("handler_kind", ["default", "ignored", "custom"]) +def test_proc_reader_suspend_restores_signal_handler(handler_kind) -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + reader._terminal_available.set() + previous = {"default": signal.SIG_DFL, "ignored": signal.SIG_IGN, "custom": mock.Mock()}[handler_kind] + groups = [proc.pid, reader._original_group] if handler_kind == "default" else [reader._original_group] + with ( + mock.patch("signal.getsignal", return_value=previous), + mock.patch("signal.signal") as set_handler, + mock.patch("signal.raise_signal") as stop, + mock.patch("os.killpg") as killpg, + mock.patch("os.tcgetpgrp", side_effect=groups), + mock.patch("threading.Thread"), + mock.patch.object(reader, "_set_foreground_group") as foreground, + reader.manage_terminal(), + ): + handler = set_handler.call_args.args[1] + handler(signal.SIGTSTP, None) + assert reader._job_resumed.is_set() + set_handler.assert_called_with(signal.SIGTSTP, previous) + foreground.assert_called_with(10, proc.pid) + if handler_kind == "default": + killpg.assert_called_once_with(reader._original_group, signal.SIGTSTP) + stop.assert_called_once_with(signal.SIGTSTP) + assert set_handler.call_args_list == [ + mock.call(signal.SIGTSTP, handler), + mock.call(signal.SIGTSTP, signal.SIG_IGN), + mock.call(signal.SIGTSTP, signal.SIG_DFL), + mock.call(signal.SIGTSTP, handler), + mock.call(signal.SIGTSTP, previous), + ] + else: + killpg.assert_not_called() + stop.assert_not_called() + if handler_kind == "custom": + previous.assert_called_once_with(signal.SIGTSTP, None) + + +def test_proc_reader_captured_pipeline_needs_no_terminal() -> None: + reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) + with reader.manage_terminal(), reader.lend_terminal(): + assert not reader._terminal_available.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_lending_restores_terminal_on_write_error() -> None: + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def failing_write(): + with reader.lend_terminal(): + assert reader._terminal_available.is_set() + raise BrokenPipeError + + with ( + mock.patch("os.tcgetpgrp", return_value=123), + mock.patch.object(reader, "_set_foreground_group") as foreground, + pytest.raises(BrokenPipeError), + ): + failing_write() + assert not reader._terminal_available.is_set() + assert foreground.call_args_list == [mock.call(10, 123), mock.call(10, 456)] + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("error_number", [errno.ESRCH, errno.EINVAL, errno.EBADF]) +def test_proc_reader_handoff_to_disappearing_group(error_number) -> None: + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def write(): + with reader.lend_terminal(): + assert reader._terminal_available.is_set() + + with ( + mock.patch("os.tcgetpgrp", return_value=456), + mock.patch.object(reader, "_set_foreground_group", side_effect=OSError(error_number, "handoff failed")), + ): + if error_number == errno.EBADF: + with pytest.raises(OSError, match="handoff failed"): + write() + else: + write() + assert not reader._terminal_available.is_set() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_reaps_killed_consumer_without_another_handoff() -> None: + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + stopped_status = (signal.SIGTTIN << 8) | 0x7F + with ( + mock.patch("os.waitpid", side_effect=[(123, stopped_status), (123, signal.SIGKILL)]), + mock.patch("os.tcgetpgrp", return_value=456), + mock.patch.object(reader._terminal_available, "wait", return_value=False), + mock.patch("os.killpg") as killpg, + ): + reader._wait_for_job(10) + assert proc.returncode == -signal.SIGKILL + assert reader._process_done.is_set() + killpg.assert_not_called() + proc.wait.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal pipeline writer") +@pytest.mark.parametrize("returncode", [-signal.SIGINT, 128 + signal.SIGINT, 0]) +@pytest.mark.parametrize("finished", [False, True]) +def test_pipeline_writer_cancels_interrupted_producer_but_not_cleanup(returncode, finished) -> None: + reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None, returncode=returncode), sys.stdout, sys.stderr) + if finished: + reader.finish_producer() + read_fd, write_fd = os.pipe() + os.close(read_fd) + expected = KeyboardInterrupt if returncode != 0 and not finished else BrokenPipeError + with cu.PipelineWriter(write_fd, reader) as writer, pytest.raises(expected): + writer.write(b"output") + + @pytest.fixture def context_flag(): return cu.ContextFlag() From b75e18e3bccd3e6374083b10cfedd818054a4a1a Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 11:50:09 -0400 Subject: [PATCH 18/44] Preserve ignored Ctrl-Z for session-led pipelines --- cmd2/cmd2.py | 25 +++++++++++++++++-------- tests/test_cmd2.py | 11 +++++++++++ tests/test_pipeline_job_control.py | 17 ++++++++++++----- 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index dcbda0965..bf2fbb8e7 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3571,14 +3571,23 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: if pipe_stdout is not None or pipe_stderr is not None: terminal_stack.enter_context(self.suspend_bottom_toolbar()) - proc = subprocess.Popen( # noqa: S602 - statement.redirect_to, - stdin=subproc_stdin, - stdout=subprocess.PIPE if pipe_stdout is None else pipe_stdout, - stderr=subprocess.PIPE if pipe_stderr is None else pipe_stderr, - shell=True, - **kwargs, - ) + with contextlib.ExitStack() as spawn_stack: + if terminal_fd is not None and os.getpgrp() == os.getsid(0): + import signal + + # A session leader's job has no outer shell to resume it. + # Its pipeline must inherit the same Ctrl-Z behavior: the + # new group would otherwise make SIGTSTP actionable again. + previous_tstp = signal.signal(signal.SIGTSTP, signal.SIG_IGN) + spawn_stack.callback(signal.signal, signal.SIGTSTP, previous_tstp) + proc = subprocess.Popen( # noqa: S602 + statement.redirect_to, + stdin=subproc_stdin, + stdout=subprocess.PIPE if pipe_stdout is None else pipe_stdout, + stderr=subprocess.PIPE if pipe_stderr is None else pipe_stderr, + shell=True, + **kwargs, + ) # Only the child should own a read end. In particular, a consumer # exit must unblock a producer writing to a full pipe immediately. subproc_stdin.close() diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 961963842..acd97a62a 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -886,8 +886,18 @@ def test_pipe_to_shell_error(redirection_app, mocker, capsys, terminal) -> None: target.fileno.return_value = 10 mocker.patch("cmd2.command_toolbar.pipe_target", return_value=target) mocker.patch("os.tcgetpgrp", return_value=os.getpgrp()) + mocker.patch("os.getsid", return_value=os.getpgrp()) sigmask = mocker.patch("signal.pthread_sigmask", return_value=set()) reader = mocker.patch("cmd2.utils.ProcReader").return_value + previous_tstp = signal.getsignal(signal.SIGTSTP) + + def start_pipe(*args, **kwargs): + # Session-led pipelines inherit ignored Ctrl-Z, but the caller's + # handler must be restored even when startup reports an early exit. + assert signal.getsignal(signal.SIGTSTP) == signal.SIG_IGN + return process + + popen.side_effect = start_pipe if terminal: # run_cmd captures stderr in a StdSim, which deliberately disables terminal handoff. @@ -900,6 +910,7 @@ def test_pipe_to_shell_error(redirection_app, mocker, capsys, terminal) -> None: assert "Pipe process exited with code 127 before command could run" in " ".join(err) assert capsys.readouterr().out == "" if terminal: + assert signal.getsignal(signal.SIGTSTP) == previous_tstp reader.wait_for_exit.assert_called_once_with(0.2) reader.wait.assert_called_once_with() process.wait.assert_not_called() diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index cf21be86a..9a38cdecc 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -47,6 +47,7 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo interrupts = tmp_path / "interrupts" pager.write_text( "import errno, os, pathlib, signal, sys, termios, tty\n" + f"if {launcher == 'exec'!r}: assert signal.getsignal(signal.SIGTSTP) == signal.SIG_IGN\n" f"pathlib.Path({str(pager_pid)!r}).write_text(str(os.getpid()))\n" f"if {finish != 'exit_sigint'!r}: sys.stdin.read()\n" # Like less, use an inherited terminal descriptor for keyboard input when @@ -76,8 +77,10 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo " try:\n" " setcbreak()\n" " os.write(1, b'PAGER_READY\\n')\n" - " while os.read(terminal.fileno(), 1) != b'q':\n" - " pass\n" + " while True:\n" + " key = os.read(terminal.fileno(), 1)\n" + " if key == b'q': break\n" + " if key == b'p': os.write(1, b'PAGER_ALIVE\\n')\n" " finally:\n" " termios.tcsetattr(terminal, termios.TCSANOW, saved)\n", encoding="utf-8", @@ -138,7 +141,9 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" ) - env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell) + # Exercise the same pipeline shell on developer machines and in CI. An + # inherited zsh can exec the pager directly, hiding bash's stop/wait behavior. + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) os.close(slave) @@ -232,9 +237,11 @@ def stopped(*pids: int) -> bool: assert pipeline_group > 1 assert pipeline_group != job_group if launcher == "exec": + # There is no outer shell to run fg: Ctrl-Z must leave the pager + # usable. Require a fresh read acknowledgement, not a SIGCONT. start = len(transcript) - send("\x1a") - wait_until(lambda: "PAGER_RESUMED\r\n" in transcript[start:]) + send("\x1ap") + wait_until(lambda: "PAGER_ALIVE\r\n" in transcript[start:]) assert os.tcgetpgrp(master) == pipeline_group for rows in (12, 24) if stop_job else (): send("\x1a") From a7a733f6314e9285b91bbe6d8b69c36a7e71b66f Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 12:10:32 -0400 Subject: [PATCH 19/44] Run shell producers inside terminal pipelines and isolate worker-thread pipes A shell command piped to an interactive consumer, such as `shell git log | less`, hung: the child inherited the raw pipe descriptor, bypassing the writer that lends the terminal per write, so the consumer stopped on its first terminal access while the producer blocked on the full pipe. do_shell now spawns its child in the pipeline's process group and lends the terminal for the child's lifetime, so the consumer keeps the terminal and Ctrl-C and Ctrl-Z reach both processes as in a shell pipeline. If the pipeline exits before the child can join its group, the child runs in our own group as before. Because such a producer can outlive the consumer that led the group, ProcReader.send_sigint() falls back to the leader's pid as the group id once the leader has been reaped, rather than doing nothing. A pipe started off the main thread could not install job-control signal handlers and failed after Popen, leaving the child unreaped. The terminal pipeline path is now taken only on the main thread; elsewhere the pipeline keeps running in its own session. --- cmd2/cmd2.py | 57 ++++++++++----- cmd2/utils.py | 20 +++-- tests/test_pipeline_job_control.py | 113 ++++++++++++++++++++++++++--- tests/test_utils.py | 39 ++++++++-- 4 files changed, 191 insertions(+), 38 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index bf2fbb8e7..85c1033d5 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3554,12 +3554,15 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: terminal_fd = None if sys.platform != "win32": - for stream in (pipe_stdout, pipe_stderr): - if stream is not None and stream.isatty(): - with contextlib.suppress(OSError, ValueError): - if os.tcgetpgrp(stream.fileno()) == os.getpgrp(): - terminal_fd = stream.fileno() - break + # Job control installs signal handlers, which only the main thread may do. + # Elsewhere, keep the pipeline in its own session as before. + if threading.current_thread() is threading.main_thread(): + for stream in (pipe_stdout, pipe_stderr): + if stream is not None and stream.isatty(): + with contextlib.suppress(OSError, ValueError): + if os.tcgetpgrp(stream.fileno()) == os.getpgrp(): + terminal_fd = stream.fileno() + break if terminal_fd is None: kwargs["start_new_session"] = True else: @@ -5248,17 +5251,37 @@ def do_shell(self, args: argparse.Namespace) -> None: utils.expand_user_in_tokens(tokens) expanded_command = " ".join(tokens) - # Prevent KeyboardInterrupts while in the shell process. The shell process will - # still receive the SIGINT since it is in the same process group as us. - with self.sigint_protection: - # For any stream that is a StdSim, we will use a pipe so we can capture its output - proc = subprocess.Popen( # noqa: S602 - expanded_command, - stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] - stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, - shell=True, - **kwargs, - ) + # A terminal pipeline's consumer needs the terminal to drain the pipe, but a shell + # command writes into that pipe itself rather than through self.stdout, which lends + # the terminal per write. Run the command inside the pipeline's job instead, for as + # long as it runs: the consumer keeps the terminal, and Ctrl-C and Ctrl-Z reach both + # processes, as they would in a shell pipeline. + pipeline = self._cur_pipe_proc_reader + pipeline_group = None + if pipeline is not None and not isinstance(self.stdout, utils.StdSim): # type: ignore[unreachable] + pipeline_group = pipeline.terminal_group + + # Prevent KeyboardInterrupts while in the shell process. The shell process still + # receives the SIGINT: it is in our process group or in the foreground pipeline's. + with self.sigint_protection, contextlib.ExitStack() as terminal_stack: + if pipeline is not None and pipeline_group is not None: + kwargs["process_group"] = pipeline_group + terminal_stack.enter_context(pipeline.lend_terminal()) + while True: + try: + # For any stream that is a StdSim, we will use a pipe so we can capture its output + proc = subprocess.Popen( # noqa: S602 + expanded_command, + stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] + stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, + shell=True, + **kwargs, + ) + break + except PermissionError: + # The pipeline exited before the command could join its group. + if kwargs.pop("process_group", None) is None: + raise proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) proc_reader.wait() diff --git a/cmd2/utils.py b/cmd2/utils.py index 2e6d47c06..1cd31195b 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -587,12 +587,15 @@ def send_sigint(self) -> None: # the whole process group to make sure it propagates further than the shell try: group_id = os.getpgid(self._proc.pid) - # Pipelines have their own group. Never re-signal our own group: - # other ProcReader callers may share it and already received Ctrl-C. - if group_id != os.getpgrp(): - os.killpg(group_id, signal.SIGINT) except ProcessLookupError: - return + # Pipelines lead their own group. A shell command that joined it, such + # as `shell sleep 100 | head -1`, can outlive the reaped consumer. + group_id = self._proc.pid + # Never re-signal our own group: other ProcReader callers may share it + # and already received Ctrl-C. + if group_id != os.getpgrp(): + with contextlib.suppress(ProcessLookupError): + os.killpg(group_id, signal.SIGINT) def terminate(self) -> None: """Terminate the process.""" @@ -605,6 +608,13 @@ def terminate(self) -> None: with contextlib.suppress(ProcessLookupError): os.kill(self._proc.pid, signal.SIGTERM) + @property + def terminal_group(self) -> int | None: + """Process group of a running terminal pipeline, which a producer may join, or None.""" + if self._terminal_fd is None or self._proc.returncode is not None: + return None + return self._proc.pid + @staticmethod def _set_foreground_group(terminal_fd: int, group_id: int) -> None: """Transfer the terminal without stopping this background thread with SIGTTOU.""" diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 9a38cdecc..d6182788e 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -20,20 +20,22 @@ @pytest.mark.parametrize( - ("finish", "stop_job", "shell_child", "launcher"), + ("finish", "stop_job", "shell_child", "launcher", "producer"), [ - pytest.param("interrupts", True, False, "direct", id="direct-signals-and-job-control"), - pytest.param("interrupts", True, True, "sh", id="wrapper-signals-and-job-control"), - pytest.param("exit_sigint", False, False, "direct", id="interrupt-busy-producer"), - pytest.param("exit_sigint", True, True, "uv", id="uv-stop-and-interrupt-busy-producer"), - pytest.param("read_input", False, False, "direct", id="nested-prompt"), - pytest.param("shell_input", False, True, "direct", id="shell-input"), - pytest.param("direct_input", False, False, "sh", id="direct-input-with-toolbar-off"), - pytest.param("direct_input", False, False, "exec", id="direct-input-in-orphaned-session"), - pytest.param("interrupts", False, False, "exec", id="orphaned-job-control"), + pytest.param("interrupts", True, False, "direct", "command", id="direct-signals-and-job-control"), + pytest.param("interrupts", True, True, "sh", "command", id="wrapper-signals-and-job-control"), + pytest.param("exit_sigint", False, False, "direct", "command", id="interrupt-busy-producer"), + pytest.param("exit_sigint", True, True, "uv", "command", id="uv-stop-and-interrupt-busy-producer"), + pytest.param("exit_sigint", False, False, "direct", "shell", id="interrupt-busy-shell-producer"), + pytest.param("exit_sigint", True, True, "sh", "shell", id="wrapper-stop-and-interrupt-busy-shell-producer"), + pytest.param("read_input", False, False, "direct", "command", id="nested-prompt"), + pytest.param("shell_input", False, True, "direct", "command", id="shell-input"), + pytest.param("direct_input", False, False, "sh", "command", id="direct-input-with-toolbar-off"), + pytest.param("direct_input", False, False, "exec", "command", id="direct-input-in-orphaned-session"), + pytest.param("interrupts", False, False, "exec", "command", id="orphaned-job-control"), ], ) -def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child, launcher) -> None: +def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child, launcher, producer) -> None: import fcntl import pty import struct @@ -88,8 +90,10 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo application = tmp_path / "application.py" application_pid = tmp_path / "application.pid" interrupt_request = tmp_path / "interrupt.request" + last_result = tmp_path / "last_result" application.write_text( "from cmd2 import Cmd, ToolbarMode\n" + "from cmd2.plugin import CommandFinalizationData\n" "import getpass, os, pathlib, signal, threading, time\n" "signal.signal(signal.SIGTSTP, signal.SIG_DFL)\n" f"pathlib.Path({str(application_pid)!r}).write_text(str(os.getpid()))\n" @@ -110,7 +114,11 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo " os.write(2, b'RAW> ')\n" " assert os.read(0, 7) == b'direct\\n'\n" " self.poutput('INPUT_COMPLETE')\n" + " def record_result(self, data: CommandFinalizationData) -> CommandFinalizationData:\n" + f" pathlib.Path({str(last_result)!r}).write_text(repr(self.last_result))\n" + " return data\n" f"app = App(bottom_toolbar_mode=ToolbarMode.{'OFF' if finish == 'direct_input' else 'RESERVED'})\n" + "app.register_cmdfinalization_hook(app.record_result)\n" "app.prompt = 'TEST> '\n" "app.debug = True\n" "app.main_session.bottom_toolbar = 'STATUS'\n" @@ -206,6 +214,21 @@ def stopped(*pids: int) -> bool: assert job_group > 1 wait_until(lambda: os.tcgetpgrp(master) == job_group) command = "busy" if finish == "exit_sigint" else "help -v" + if producer == "shell": + # A shell command writes into the pipe itself rather than through cmd2's + # stdout, so cmd2 cannot lend the terminal write by write. Like seq or git + # log, this producer dies from SIGINT rather than handling it. + busy_script = tmp_path / "busy.py" + busy_script.write_text( + "import os, signal, sys, time\n" + "signal.signal(signal.SIGINT, signal.SIG_DFL)\n" + "os.write(2, b'BUSY_READY\\n')\n" + "sys.stdout.write('x' * 262144)\n" + "sys.stdout.flush()\n" + "time.sleep(30)\n", + encoding="utf-8", + ) + command = f"shell {shlex.quote(sys.executable)} {shlex.quote(str(busy_script))}" if finish == "read_input": command = "ask" elif finish == "direct_input": @@ -287,6 +310,11 @@ def stopped(*pids: int) -> bool: if finish != "exit_sigint": send("q") wait_until(lambda: os.tcgetpgrp(master) == job_group and "TEST>" in "\n".join(screen.display)) + if producer == "shell": + # Ctrl-C reached the producer directly, as in a shell pipeline. It did not + # merely die of a broken pipe once the pager was gone. + wait_until(last_result.exists) + assert last_result.read_text() == repr(-signal.SIGINT) start = len(transcript) send("help quit\n") wait_until(lambda: "Exit this application" in transcript[start:]) @@ -309,3 +337,66 @@ def stopped(*pids: int) -> bool: os.close(master) process.kill() process.wait(timeout=5) + + +def test_pipeline_from_worker_thread_stays_isolated(tmp_path) -> None: + """A pipe started off the main thread cannot install job-control handlers. + + It must fall back to running the pipeline in its own session, as before, rather + than failing after Popen and leaving the child unreaped. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd, ToolbarMode\n" + "import os, threading\n" + "app = Cmd(bottom_toolbar_mode=ToolbarMode.OFF)\n" + "outcome = []\n" + "worker = threading.Thread(target=lambda: outcome.append(app.onecmd_plus_hooks('help quit | cat')))\n" + "worker.start()\n" + "worker.join()\n" + "try:\n" + " reaped = os.waitpid(-1, os.WNOHANG)\n" + "except ChildProcessError:\n" + " reaped = None\n" + "os.write(1, f'WORKER_DONE {outcome} {reaped}\\n'.encode())\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + transcript += decoder.decode(os.read(master, 65536)) + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}") + + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + # The whole line: a partial read must not satisfy the wait before the reap result arrives. + wait_until(lambda: re.search(r"WORKER_DONE .*\r\n", transcript) is not None) + assert "Exit this application" in transcript + assert "WORKER_DONE [False] None" in transcript + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index c097c8d6a..ea2afa8ef 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -223,7 +223,7 @@ def test_proc_reader_send_sigint(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: try: - with mock.patch("os.getpgid", return_value=os.getpgrp()), mock.patch("os.killpg") as killpg: + with mock.patch("os.getpgrp", return_value=pr_none._proc.pid), mock.patch("os.killpg") as killpg: pr_none.send_sigint() killpg.assert_not_called() finally: @@ -232,11 +232,40 @@ def test_proc_reader_does_not_resignal_its_own_group(pr_none) -> None: @pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") -def test_proc_reader_sigint_after_consumer_exit() -> None: - reader = cu.ProcReader(mock.Mock(stdout=None, stderr=None), sys.stdout, sys.stderr) - with mock.patch("os.getpgid", side_effect=ProcessLookupError), mock.patch("os.killpg") as killpg: +def test_proc_reader_sigint_after_pipeline_exit() -> None: + reader = cu.ProcReader(mock.Mock(pid=os.getpid() + 1, stdout=None, stderr=None), sys.stdout, sys.stderr) + with ( + mock.patch("os.getpgid", side_effect=ProcessLookupError), + mock.patch("os.killpg", side_effect=ProcessLookupError) as killpg, + ): reader.send_sigint() - killpg.assert_not_called() + killpg.assert_called_once_with(reader._proc.pid, signal.SIGINT) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_sigint_reaches_group_after_leader_exit() -> None: + """A shell producer joins the pipeline's group and can outlive the consumer that led it.""" + import subprocess + + # A terminal pipeline leads its own group within our session, so a producer may join it. + leader = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"], process_group=0) + reader = cu.ProcReader(leader, sys.stdout, sys.stderr) + member_code = ( + "import signal, time; signal.signal(signal.SIGINT, signal.SIG_DFL); print('ready', flush=True); time.sleep(30)" + ) + member = subprocess.Popen([sys.executable, "-c", member_code], stdout=subprocess.PIPE, process_group=leader.pid) + try: + assert member.stdout is not None + assert member.stdout.readline().strip() == b"ready" + reader.terminate() + reader.wait() + assert leader.returncode == -signal.SIGTERM + + reader.send_sigint() + assert member.wait(timeout=5) == -signal.SIGINT + finally: + member.kill() + member.wait() def test_proc_reader_terminate(pr_none) -> None: From d4566a02cb386911a60fe5d1042a06031f5517c3 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 13:00:50 -0400 Subject: [PATCH 20/44] Cover the shell command's fallback paths in terminal pipelines Exercise ProcReader.terminal_group for readers without a terminal and for a finished pipeline, the shell command's retry in its own group when the pipeline's group is gone before it can join, and a PermissionError from Popen that has nothing to do with the join, which must still propagate. --- tests/test_cmd2.py | 29 +++++++++++++++++++++++++++++ tests/test_utils.py | 12 ++++++++++++ 2 files changed, 41 insertions(+) diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index acd97a62a..6c7f3dfab 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -428,6 +428,35 @@ def test_shell_manual_call(base_app) -> None: base_app.do_shell(cmd) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_shell_falls_back_to_own_group_when_pipeline_exited(base_app, tmp_path) -> None: + import contextlib + import subprocess + from unittest import mock + + # A group whose only member has exited cannot be joined. The consumer of a terminal + # pipeline can exit between the check and the spawn, like `shell sleep 1 | true`. + leader = subprocess.Popen([sys.executable, "-c", "pass"], process_group=0) + leader.wait() + base_app._cur_pipe_proc_reader = mock.Mock(terminal_group=leader.pid, lend_terminal=contextlib.nullcontext) + with (tmp_path / "output").open("w+") as output: + base_app.stdout = output + base_app.do_shell("echo joined") + output.seek(0) + assert output.read() == "joined\n" + assert base_app.last_result == 0 + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX shell executable") +def test_shell_permission_error_unrelated_to_pipeline(base_app, tmp_path, monkeypatch) -> None: + unusable_shell = tmp_path / "shell" + unusable_shell.write_text("#!/bin/sh\n") + unusable_shell.chmod(0o644) + monkeypatch.setenv("SHELL", str(unusable_shell)) + with pytest.raises(PermissionError): + base_app.do_shell("echo hi") + + def test_base_error(base_app) -> None: _out, err = run_cmd(base_app, "meow") assert "is not a recognized command" in err[0] diff --git a/tests/test_utils.py b/tests/test_utils.py index ea2afa8ef..f3d931af0 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -268,6 +268,18 @@ def test_proc_reader_sigint_reaches_group_after_leader_exit() -> None: member.wait() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups") +def test_proc_reader_terminal_group() -> None: + proc = mock.Mock(pid=4242, returncode=None, stdout=None, stderr=None) + assert cu.ProcReader(proc, sys.stdout, sys.stderr).terminal_group is None + + with mock.patch("os.tcgetpgrp", return_value=os.getpgrp()): + reader = cu.ProcReader(proc, sys.stdout, sys.stderr, terminal_fd=0) + assert reader.terminal_group == proc.pid + proc.returncode = 0 + assert reader.terminal_group is None + + def test_proc_reader_terminate(pr_none) -> None: assert pr_none._proc.poll() is None pr_none.terminate() From 0102c44d1ed622b858b53c27c01e6601b491c9d8 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 13:37:37 -0400 Subject: [PATCH 21/44] Keep the terminal lent across an interrupted pipeline write A signal that interrupts a blocking pipe write, such as the job-control stop ProcReader relays for Ctrl-Z, returned a partial count, so PipelineWriter ended its lend and the terminal went back to cmd2's group for the instant before the next write re-lent it. A consumer that had just resumed a terminal read could hit that window and be stopped again with SIGTTIN, racing with the next Ctrl-Z in the job-control test on a loaded runner. Finish the buffer under a single lend instead. When a terminal condition times out, the job-control tests now also report the pty's foreground group and the state and wait channel of every process under the outer shell, so a silent transcript still shows which process was waiting for whom. --- cmd2/utils.py | 15 ++++++++++++-- tests/test_pipeline_job_control.py | 33 ++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index 1cd31195b..c3a35714e 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -836,12 +836,23 @@ def __init__(self, fd: int, reader: ProcReader) -> None: self._reader = reader def write(self, b: Any) -> int: - """Write bytes while the consumer can interact with the terminal.""" + """Write all of b while the consumer can interact with the terminal. + + A signal that interrupts a blocking write, such as the job-control stop + ProcReader relays, returns a partial count. Finishing the buffer under the + same lend keeps the terminal with the consumer instead of returning it for + the instant between two writes, when a consumer that has just resumed a + terminal read would be stopped again with SIGTTIN. + """ import signal + view = memoryview(b).cast("B") try: with self._reader.lend_terminal(): - return cast(int, super().write(b)) + written = 0 + while written < len(view): + written += cast(int, super().write(view[written:])) + return written except BrokenPipeError: # Ctrl-C during a blocking write must cancel the command, even if it # normally catches BrokenPipeError. Raise here rather than signaling diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index d6182788e..1ad24ca23 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -19,6 +19,35 @@ pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") +def describe_processes(root: int, master: int) -> str: + """Report the terminal's foreground group and every descendant of root, for a timeout. + + A silent transcript says only that nothing happened. Process states (T for stopped) + and wait channels say which process was waiting for whom. + """ + try: + foreground: object = os.tcgetpgrp(master) + except OSError as error: + foreground = error + listing = subprocess.run( + ["ps", "-e", "-o", "pid,ppid,pgid,stat,wchan,command"], capture_output=True, text=True, check=False + ) + rows = listing.stdout.splitlines() + parents = {} + for row in rows[1:]: + fields = row.split(maxsplit=2) + if len(fields) >= 2 and fields[0].isdigit() and fields[1].isdigit(): + parents[int(fields[0])] = int(fields[1]) + family = {root} + while True: + grown = family | {pid for pid, parent in parents.items() if parent in family} + if grown == family: + break + family = grown + described = [row for row in rows[1:] if row.split(maxsplit=1)[0].isdigit() and int(row.split(maxsplit=1)[0]) in family] + return "\n".join([f"foreground process group: {foreground}", rows[0] if rows else "", *described]) + + @pytest.mark.parametrize( ("finish", "stop_job", "shell_child", "launcher", "producer"), [ @@ -174,7 +203,7 @@ def wait_until(predicate): stream.feed(data) if predicate(): return - pytest.fail(f"terminal condition timed out:\n{transcript}") + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") def stopped(*pids: int) -> bool: """Whether every process is stopped, not merely deprived of the terminal. @@ -387,7 +416,7 @@ def wait_until(predicate): transcript += decoder.decode(os.read(master, 65536)) if predicate(): return - pytest.fail(f"terminal condition timed out:\n{transcript}") + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") try: wait_until(lambda: "OUTER> " in transcript) From 7643b9a39232fadbf9cfaf9a75971226f782165b Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 13:49:37 -0400 Subject: [PATCH 22/44] Relay pipeline stops to the main thread with a thread-directed signal The watcher thread stopped cmd2's job by sending SIGTSTP to the whole process with os.kill(). A process-directed signal may be taken by any thread that does not block it, and only the main thread runs Python signal handlers, so when it went elsewhere while the main thread slept in a system call, the handler never ran: the pipeline stayed stopped, the terminal was already back with cmd2's group, and the outer shell never saw the job stop. CI caught this with a shell producer sleeping in waitpid, and the process listing showed the main thread still there with the pipeline stopped. Use pthread_kill() on the main thread, which queues the signal on that thread and interrupts its system call. --- cmd2/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index c3a35714e..c0ab01be1 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -739,7 +739,10 @@ def _wait_for_job(self, terminal_fd: int) -> None: # Stop every terminal reader before returning control to the outer shell. os.killpg(self._proc.pid, signal.SIGSTOP) self._job_resumed.clear() - os.kill(os.getpid(), signal.SIGTSTP) + # Signal the main thread itself. Only it runs Python signal handlers, and a + # process-directed signal may be taken by another thread while the main + # thread sleeps in a system call, which then never returns to run the handler. + signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) self._job_resumed.wait() os.killpg(self._proc.pid, signal.SIGCONT) finally: From d3b263082f9b08e13af4348004c3a26a3aa455f1 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 14:14:57 -0400 Subject: [PATCH 23/44] Let the main thread relay a pipeline stop from a blocking write or wait Only the main thread runs Python signal handlers, and the job-control stop the pipeline watcher relays can wake another thread, however it is addressed: CI on macOS showed the pipeline stopped, the terminal already returned, and cmd2's main thread still asleep in the shell command's waitpid, and the earlier Linux failures had it asleep in a pipe write. A thread that never returns from its system call never runs the handler, so the job is never suspended. Wait in short polls at both points instead, as wait_for_exit already does: PipelineWriter writes through a non-blocking pipe and polls for room, and the shell command waits in slices while its child belongs to a terminal pipeline. The job-control test now delivers the relay to the watcher thread itself in the two cases that failed, which reproduced the hang deterministically. The reserved-terminal pager test waits for the bar repainted after a handoff, as the command display renders on its own thread and can land it a frame later than the resume, which Windows CI hit. --- cmd2/cmd2.py | 10 +++++++++ cmd2/utils.py | 22 +++++++++++++------ tests/test_pipeline_job_control.py | 34 ++++++++++++++++++------------ tests/test_reserved_terminal.py | 4 +++- 4 files changed, 50 insertions(+), 20 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 85c1033d5..a1154bacb 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5284,6 +5284,16 @@ def do_shell(self, args: argparse.Namespace) -> None: raise proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) + if pipeline_group is not None: + # Only the main thread runs Python signal handlers, and the job-control + # stop the pipeline's watcher relays may wake another thread. Return from + # the wait regularly so the handler runs while the command is still going. + while True: + try: + proc_reader.wait_for_exit(0.1) + break + except subprocess.TimeoutExpired: + continue proc_reader.wait() # Save the return code of the application for use in a pyscript diff --git a/cmd2/utils.py b/cmd2/utils.py index c0ab01be1..a366f08b2 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -836,25 +836,35 @@ class PipelineWriter(io.FileIO): def __init__(self, fd: int, reader: ProcReader) -> None: """Take ownership of a pipe descriptor managed by reader.""" super().__init__(fd, "w") + os.set_blocking(fd, False) self._reader = reader def write(self, b: Any) -> int: """Write all of b while the consumer can interact with the terminal. - A signal that interrupts a blocking write, such as the job-control stop - ProcReader relays, returns a partial count. Finishing the buffer under the - same lend keeps the terminal with the consumer instead of returning it for - the instant between two writes, when a consumer that has just resumed a - terminal read would be stopped again with SIGTTIN. + The whole buffer goes out under one lend. Returning the terminal between two + writes, even for an instant, would stop a consumer that had just resumed a + terminal read with SIGTTIN. + + A full pipe is awaited in short polls rather than a blocking write. Only the + main thread runs Python signal handlers, and the job-control stop ProcReader + relays may wake another thread, so the main thread has to return to Python + code on its own for the handler to run. """ + import select import signal view = memoryview(b).cast("B") + poller = select.poll() + poller.register(self.fileno(), select.POLLOUT) try: with self._reader.lend_terminal(): written = 0 while written < len(view): - written += cast(int, super().write(view[written:])) + try: + written += os.write(self.fileno(), view[written:]) + except BlockingIOError: + poller.poll(100) return written except BrokenPipeError: # Ctrl-C during a blocking write must cancel the command, even if it diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 1ad24ca23..1ad303737 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -49,22 +49,24 @@ def describe_processes(root: int, master: int) -> str: @pytest.mark.parametrize( - ("finish", "stop_job", "shell_child", "launcher", "producer"), + ("finish", "stop_job", "shell_child", "launcher", "producer", "relay"), [ - pytest.param("interrupts", True, False, "direct", "command", id="direct-signals-and-job-control"), - pytest.param("interrupts", True, True, "sh", "command", id="wrapper-signals-and-job-control"), - pytest.param("exit_sigint", False, False, "direct", "command", id="interrupt-busy-producer"), - pytest.param("exit_sigint", True, True, "uv", "command", id="uv-stop-and-interrupt-busy-producer"), - pytest.param("exit_sigint", False, False, "direct", "shell", id="interrupt-busy-shell-producer"), - pytest.param("exit_sigint", True, True, "sh", "shell", id="wrapper-stop-and-interrupt-busy-shell-producer"), - pytest.param("read_input", False, False, "direct", "command", id="nested-prompt"), - pytest.param("shell_input", False, True, "direct", "command", id="shell-input"), - pytest.param("direct_input", False, False, "sh", "command", id="direct-input-with-toolbar-off"), - pytest.param("direct_input", False, False, "exec", "command", id="direct-input-in-orphaned-session"), - pytest.param("interrupts", False, False, "exec", "command", id="orphaned-job-control"), + pytest.param("interrupts", True, False, "direct", "command", "main", id="direct-signals-and-job-control"), + pytest.param("interrupts", True, True, "sh", "command", "main", id="wrapper-signals-and-job-control"), + pytest.param("exit_sigint", False, False, "direct", "command", "main", id="interrupt-busy-producer"), + pytest.param("exit_sigint", True, True, "uv", "command", "worker", id="uv-stop-and-interrupt-busy-producer"), + pytest.param("exit_sigint", False, False, "direct", "shell", "main", id="interrupt-busy-shell-producer"), + pytest.param("exit_sigint", True, True, "sh", "shell", "worker", id="wrapper-stop-and-interrupt-busy-shell-producer"), + pytest.param("read_input", False, False, "direct", "command", "main", id="nested-prompt"), + pytest.param("shell_input", False, True, "direct", "command", "main", id="shell-input"), + pytest.param("direct_input", False, False, "sh", "command", "main", id="direct-input-with-toolbar-off"), + pytest.param("direct_input", False, False, "exec", "command", "main", id="direct-input-in-orphaned-session"), + pytest.param("interrupts", False, False, "exec", "command", "main", id="orphaned-job-control"), ], ) -def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_job, shell_child, launcher, producer) -> None: +def test_pipeline_stops_with_cmd2_and_returns_terminal( + tmp_path, finish, stop_job, shell_child, launcher, producer, relay +) -> None: import fcntl import pty import struct @@ -125,6 +127,12 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal(tmp_path, finish, stop_jo "from cmd2.plugin import CommandFinalizationData\n" "import getpass, os, pathlib, signal, threading, time\n" "signal.signal(signal.SIGTSTP, signal.SIG_DFL)\n" + f"if {relay == 'worker'!r}:\n" + # The kernel may hand a signal to a thread other than the one it was aimed at. + # Deliver the job-control relay to the watcher that sends it, so the main thread, + # blocked in a pipe write or a wait, only learns of it if it returns on its own. + " _pthread_kill = signal.pthread_kill\n" + " signal.pthread_kill = lambda thread_id, signum: _pthread_kill(threading.get_ident(), signum)\n" f"pathlib.Path({str(application_pid)!r}).write_text(str(os.getpid()))\n" "class App(Cmd):\n" " def do_busy(self, statement):\n" diff --git a/tests/test_reserved_terminal.py b/tests/test_reserved_terminal.py index 67702d6f9..18c5bb57c 100644 --- a/tests/test_reserved_terminal.py +++ b/tests/test_reserved_terminal.py @@ -944,7 +944,9 @@ def test_retained_startup_bar_is_cleared_before_output_or_external_handoff(self, else: assert reserved.display._deferred_band is not None assert reserved.display._deferred_band is None - assert terminal.screen.display[-1].startswith("STATUS") + # The command display keeps rendering on its own thread, so the bar it repaints + # after the handoff can land a frame later than the resume itself. + assert wait_for(lambda: terminal.screen.display[-1].startswith("STATUS")) history = ["".join(line[x].data for x in sorted(line)) for line in terminal.screen.history.top] assert not any("STATUS" in row for row in history) From 589b46859761a2a9366a1ba7410fa801098482c5 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 14:38:30 -0400 Subject: [PATCH 24/44] Keep the pipeline descriptor blocking for shell producers Making the pipe non-blocking regressed shell commands piped to a terminal consumer: do_shell() hands the same open file description to the child, which writes to it directly and failed with EAGAIN once the pipe was full. Leave the descriptor blocking. PipelineWriter still returns to Python regularly, as the job-control relay requires, by polling for room and writing at most PIPE_BUF bytes at a time, which cannot block once the pipe reports it is writable. The job-control test's timeout diagnostics now cope with a sandbox that cannot run ps. --- cmd2/utils.py | 21 ++++++++------- tests/test_pipeline_job_control.py | 9 ++++--- tests/test_utils.py | 43 ++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 13 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index a366f08b2..5e4ee3b8f 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -836,7 +836,6 @@ class PipelineWriter(io.FileIO): def __init__(self, fd: int, reader: ProcReader) -> None: """Take ownership of a pipe descriptor managed by reader.""" super().__init__(fd, "w") - os.set_blocking(fd, False) self._reader = reader def write(self, b: Any) -> int: @@ -846,25 +845,27 @@ def write(self, b: Any) -> int: writes, even for an instant, would stop a consumer that had just resumed a terminal read with SIGTTIN. - A full pipe is awaited in short polls rather than a blocking write. Only the - main thread runs Python signal handlers, and the job-control stop ProcReader - relays may wake another thread, so the main thread has to return to Python - code on its own for the handler to run. + A full pipe is awaited in short polls rather than in one blocking write. Only + the main thread runs Python signal handlers, and the job-control stop ProcReader + relays may wake another thread, so the main thread has to return to Python code + on its own for the handler to run. The descriptor itself stays blocking: a shell + command inherits it, and a producer that found it non-blocking would fail with + EAGAIN once the pipe filled. """ import select import signal view = memoryview(b).cast("B") + fd = self.fileno() poller = select.poll() - poller.register(self.fileno(), select.POLLOUT) + poller.register(fd, select.POLLOUT) try: with self._reader.lend_terminal(): written = 0 while written < len(view): - try: - written += os.write(self.fileno(), view[written:]) - except BlockingIOError: - poller.poll(100) + # Once there is room, a write of at most PIPE_BUF bytes does not block. + if poller.poll(100): + written += os.write(fd, view[written : written + select.PIPE_BUF]) return written except BrokenPipeError: # Ctrl-C during a blocking write must cancel the command, even if it diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 1ad303737..c8d26a369 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -29,9 +29,12 @@ def describe_processes(root: int, master: int) -> str: foreground: object = os.tcgetpgrp(master) except OSError as error: foreground = error - listing = subprocess.run( - ["ps", "-e", "-o", "pid,ppid,pgid,stat,wchan,command"], capture_output=True, text=True, check=False - ) + try: + listing = subprocess.run( + ["ps", "-e", "-o", "pid,ppid,pgid,stat,wchan,command"], capture_output=True, text=True, check=False + ) + except OSError as error: + return f"foreground process group: {foreground}\nno process listing: {error}" rows = listing.stdout.splitlines() parents = {} for row in rows[1:]: diff --git a/tests/test_utils.py b/tests/test_utils.py index f3d931af0..5b2adb980 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,5 +1,6 @@ """Unit testing for cmd2/utils.py module.""" +import contextlib import errno import math import os @@ -280,6 +281,48 @@ def test_proc_reader_terminal_group() -> None: assert reader.terminal_group is None +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pipes") +@pytest.mark.parametrize("producer", ["writer", "child"]) +def test_pipeline_writer_delivers_more_than_the_pipe_holds(producer) -> None: + """Both cmd2's writes and a child inheriting the descriptor must wait for a slow consumer. + + A shell producer gets the descriptor itself, so it must stay blocking: a child that + inherits O_NONBLOCK fails with EAGAIN once the pipe is full. + """ + import subprocess + import threading + + payload = b"x" * 4 * 1024 * 1024 + read_fd, write_fd = os.pipe() + received = bytearray() + + def drain() -> None: + while chunk := os.read(read_fd, 65536): + received.extend(chunk) + time.sleep(0.001) + + reader = mock.Mock(lend_terminal=contextlib.nullcontext) + writer = cu.PipelineWriter(write_fd, reader) + consumer = threading.Thread(target=drain) + consumer.start() + try: + if producer == "writer": + assert writer.write(payload) == len(payload) + else: + child = subprocess.run( + [sys.executable, "-c", f"import sys; sys.stdout.buffer.write(b'x' * {len(payload)})"], + stdout=writer.fileno(), + stderr=subprocess.PIPE, + check=False, + ) + assert child.returncode == 0, child.stderr.decode() + finally: + writer.close() + consumer.join() + os.close(read_fd) + assert bytes(received) == payload + + def test_proc_reader_terminate(pr_none) -> None: assert pr_none._proc.poll() is None pr_none.terminate() From f34262cced336fc2b96509cb7a1600827559a8c5 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 19:34:11 -0400 Subject: [PATCH 25/44] Added stage4_manual.py example for ease of testing --- examples/stage4_manual.py | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 examples/stage4_manual.py diff --git a/examples/stage4_manual.py b/examples/stage4_manual.py new file mode 100644 index 000000000..a31b1ac14 --- /dev/null +++ b/examples/stage4_manual.py @@ -0,0 +1,40 @@ +## Example for testing nested input, secret input, and select input. Also demonstrates +## how to suspend the bottom toolbar and hand off the terminal to a guest. + +import time + +from getting_started import BasicApp + + +class ManualApp(BasicApp): + def do_nested(self, _arg): + value = self.read_input("Value> ", choices=["alpha", "beta"], history=["alpha", "beta"]) + self.poutput(repr(value)) + + def do_secret(self, _arg): + self.read_secret("Dummy secret> ") + self.poutput("Secret accepted; value not displayed") + + def do_choose(self, _arg): + self.poutput(self.select(["alpha", "beta"], "Choose> ")) + + def do_quiet(self, _arg): + time.sleep(10) + + def do_partial(self, _arg): + self.stdout.write("PARTIAL") + self.stdout.flush() + time.sleep(10) + self.stdout.write("END\n") + self.stdout.flush() + + def do_handoff(self, arg): + with self.suspend_bottom_toolbar(): + with self.suspend_bottom_toolbar(): + input("Guest owns terminal; resize, then Enter> ") + if arg.strip() == "fail": + raise RuntimeError("Intentional guest failure") + + +if __name__ == "__main__": + ManualApp().cmdloop() From 9f50afb87b978c4c231f23089cd9c10661943c94 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 19:50:25 -0400 Subject: [PATCH 26/44] Stabilize orphaned-session pipeline terminal test --- tests/test_pipeline_job_control.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index c8d26a369..9f4087193 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -84,6 +84,12 @@ def test_pipeline_stops_with_cmd2_and_returns_terminal( pager.write_text( "import errno, os, pathlib, signal, sys, termios, tty\n" f"if {launcher == 'exec'!r}: assert signal.getsignal(signal.SIGTSTP) == signal.SIG_IGN\n" + # Interactive bash leaves TTIN/TTOU ignored when exec replaces it. Give + # the simulated pager normal terminal-access stops: EOF can arrive before + # cmd2 lends it the terminal, and an ignored TTIN makes that read fail with + # EIO instead of waiting for the handoff. Preserve the inherited TSTP policy. + "signal.signal(signal.SIGTTIN, signal.SIG_DFL)\n" + "signal.signal(signal.SIGTTOU, signal.SIG_DFL)\n" f"pathlib.Path({str(pager_pid)!r}).write_text(str(os.getpid()))\n" f"if {finish != 'exit_sigint'!r}: sys.stdin.read()\n" # Like less, use an inherited terminal descriptor for keyboard input when @@ -299,6 +305,9 @@ def stopped(*pids: int) -> bool: pipeline_group = os.getpgid(pager_process) assert pipeline_group > 1 assert pipeline_group != job_group + # Readiness output can precede the foreground handoff. Send terminal + # signals and keystrokes only once the pipeline can receive them. + wait_until(lambda: os.tcgetpgrp(master) == pipeline_group) if launcher == "exec": # There is no outer shell to run fg: Ctrl-Z must leave the pager # usable. Require a fresh read acknowledgement, not a SIGCONT. From 66513f6c9f0da20f92352ac7c1dc91a8a7581ab3 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Mon, 14 Sep 2026 22:19:23 -0400 Subject: [PATCH 27/44] Hold the display thread until a Ctrl-Z stop has taken it Ctrl-Z in the built-in pager, or at the command toolbar, sends SIGTSTP from the display thread. A process-directed signal lands on the main thread, and on Linux the display thread carries on for a few milliseconds: it restores raw mode, reattaches its input and redraws before the job has stopped. When the shell resumes the job it puts back the terminal modes it saved beforehand, and nothing is left to undo that. The terminal stays cooked, keys are held until Enter, and with a refreshing toolbar every cursor-position query is echoed as ^[[row;colR, as reported after fg with the embedded pager on Debian. After sending the signal, wait for the stop to take the sending thread as well. A stop shows as time passing while asleep; a signal that was ignored, or discarded for an orphaned process group, shows as nothing and the wait ends on its own. A thread-directed duplicate would be simpler, but macOS does not discard it when the job is continued, and the job stops again after fg. The command toolbar installs this as the application's suspend_to_background() unless the reserved toolbar's job-control hook, which releases its rows first and then stops the process the same way, is already there. The regression test drives the pager under an interactive bash on a pty, suspends and resumes it, and requires that it still answers q. --- cmd2/command_toolbar.py | 41 ++++++++++++- cmd2/reserved_toolbar.py | 4 +- tests/test_pipeline_job_control.py | 98 ++++++++++++++++++++++++++++++ tests/test_reserved_terminal.py | 2 +- 4 files changed, 141 insertions(+), 4 deletions(-) diff --git a/cmd2/command_toolbar.py b/cmd2/command_toolbar.py index ff5a5b633..095eff7a4 100644 --- a/cmd2/command_toolbar.py +++ b/cmd2/command_toolbar.py @@ -8,12 +8,13 @@ import signal import sys import threading +import time from collections.abc import Callable, Iterator from concurrent.futures import Future from concurrent.futures import TimeoutError as FutureTimeoutError from typing import TYPE_CHECKING, Any, TextIO, TypeVar, cast -from prompt_toolkit.application import Application, create_app_session +from prompt_toolkit.application import Application, create_app_session, run_in_terminal from prompt_toolkit.application.current import _current_app_session, get_app_session from prompt_toolkit.enums import EditingMode from prompt_toolkit.filters import Condition, to_filter @@ -43,6 +44,33 @@ _R = TypeVar("_R") +def suspend_process_group(suspend_group: bool = True) -> None: + """Stop this process, or its whole process group, with SIGTSTP from any thread. + + A process-directed stop signal can be taken by a thread other than the sender. Sent + from the display thread, it lands on the main thread, and the display thread carries + on for a few milliseconds: it puts the terminal back into raw mode and redraws before + the job has stopped. When the shell resumes the job it restores the modes it saved + beforehand, and nothing is left to undo that -- the terminal stays cooked, keys are + held until Enter, and every cursor-position query is echoed as ``^[[row;colR``. + + So after sending the signal this thread waits for the stop to take it too. A stop + shows as time passing while asleep; a signal that was ignored, or discarded for an + orphaned process group, shows as nothing, and the wait ends on its own. + + :param suspend_group: stop the whole process group, as a shell's Ctrl-Z would, rather + than only this process + """ + if not hasattr(signal, "SIGTSTP"): # pragma: no cover - POSIX only + return + os.kill(0 if suspend_group else os.getpid(), signal.SIGTSTP) + deadline = time.monotonic() + 0.25 + while (before := time.monotonic()) < deadline: + time.sleep(0.01) + if time.monotonic() - before > 0.1: + return + + def suspend_toolbar(func: _F) -> _F: """Give a method exclusive access to the terminal, reserved rows included. @@ -217,6 +245,12 @@ def __init__(self, cmd: "Cmd") -> None: session = cmd.main_session self.app = session.app + # Ctrl-Z during a command is handled on the display's thread, where upstream's + # version would not hold the thread until the job has stopped. The reserved toolbar + # layers its row release over whatever is installed, so one it put there first is + # left in place; it releases the rows and then stops the process the same way. + if "suspend_to_background" not in vars(self.app): + cast("Any", self.app).suspend_to_background = self._suspend_to_background # PromptSession has no public hook for replacing just its input area. # Keep this small dependency on its layout shape in one place, and fail # explicitly if upstream changes it. Reuse the actual toolbar container, @@ -282,6 +316,11 @@ def suspend(event: KeyPressEvent) -> None: self._bindings = bindings self._suspend_binding = suspend + @staticmethod + def _suspend_to_background(suspend_group: bool = True) -> None: + """Suspend like upstream's ``Application.suspend_to_background()``, from any thread.""" + run_in_terminal(functools.partial(suspend_process_group, suspend_group)) + def _display_started(self, app: Application[str]) -> None: # noqa: ARG002 """Report that the display is up and has finished its first frame.""" self._ready.set() diff --git a/cmd2/reserved_toolbar.py b/cmd2/reserved_toolbar.py index f8c2651fc..af97abcfc 100644 --- a/cmd2/reserved_toolbar.py +++ b/cmd2/reserved_toolbar.py @@ -17,7 +17,6 @@ new ``bottom_toolbar`` to the session still reaches the band. """ -import os import signal from contextlib import ExitStack, contextmanager, suppress from types import TracebackType @@ -30,6 +29,7 @@ from prompt_toolkit.styles import DynamicStyle from prompt_toolkit.utils import suspend_to_background_supported +from .command_toolbar import suspend_process_group from .prompt_toolkit_bridge import PromptToolkitBridge from .reserved_output import ReservedOutput from .terminal_display import TerminalDisplay @@ -261,7 +261,7 @@ def suspend_process() -> None: # run_in_terminal has stopped rendering and detached input before this runs. # A signal callback itself must never acquire the terminal transaction. with self.suspended(): - os.kill(0 if suspend_group else os.getpid(), suspend_signal) + suspend_process_group(suspend_group) run_in_terminal(suspend_process) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 9f4087193..efdf6d142 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -449,3 +449,101 @@ def wait_until(predicate): os.close(master) process.kill() process.wait(timeout=5) + + +@pytest.mark.parametrize("mode", ["reserved", "legacy"]) +def test_pager_resumes_with_raw_terminal_after_suspend(tmp_path, mode) -> None: + """Ctrl-Z in the built-in pager, then fg, must bring the pager back, not echoed cursor reports. + + The display thread sends the stop signal, which the kernel may hand to the main + thread. Unless the sending thread stops synchronously too, it restores raw mode + before the job has stopped, the shell puts its own modes back on fg, and every + cursor-position query afterwards is echoed as ^[[row;colR. + """ + import fcntl + import pty + import struct + import termios + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd, ToolbarMode\n" + "class App(Cmd):\n" + " def do_page(self, statement):\n" + " self.ppaged('\\n'.join(f'line {n:04d}' for n in range(400)))\n" + # A refreshing toolbar, as in the examples: its periodic redraws are what turn a + # cooked terminal into a stream of echoed cursor reports. + f"app = App(bottom_toolbar_mode=ToolbarMode.{mode.upper()}, refresh_interval=0.5)\n" + "app.prompt = 'TEST> '\n" + "app.main_session.bottom_toolbar = 'STATUS'\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 24, 80, 0, 0)) + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + screen = pyte.Screen(80, 24) + screen.write_process_input = lambda data: os.write(master, data.encode()) + stream = pyte.Stream(screen) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def send(data): + os.write(master, data.encode()) + + def pump(seconds, predicate=lambda: False): + nonlocal transcript + deadline = time.monotonic() + seconds + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + stream.feed(data) + if predicate(): + return True + return predicate() + + def pager_shown(): + return any("q: quit" in line for line in screen.display) + + try: + assert pump(10, lambda: "OUTER> " in transcript) + send(f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n") + assert pump(10, lambda: "TEST>" in "\n".join(screen.display)), transcript + # Let the prompt finish its cursor-position handshake before typing at it. + pump(0.5) + send("page\n") + assert pump(10, pager_shown), transcript + start = len(transcript) + send("\x1a") + assert pump(10, lambda: "OUTER> " in transcript[start:]), transcript + start = len(transcript) + send("fg\n") + assert pump(10, pager_shown), transcript[start:] + # Give a broken resume time to start looping before judging the transcript. + pump(1.5) + resumed = transcript[start:] + echoed = re.findall(r"\^\[\[\d+;\d+R", resumed) + assert not echoed, f"cursor position reports were echoed after fg: {echoed[:5]}\n{resumed}" + assert pager_shown(), "\n".join(screen.display) + # A cooked terminal would hold the key back until Enter: the pager must quit on q alone. + start = len(transcript) + send("q") + assert pump(10, lambda: "TEST>" in transcript[start:]), f"pager did not quit on q after fg:\n{transcript[start:]}" + send("quit\n") + pump(5, lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_reserved_terminal.py b/tests/test_reserved_terminal.py index 18c5bb57c..9cc600729 100644 --- a/tests/test_reserved_terminal.py +++ b/tests/test_reserved_terminal.py @@ -858,7 +858,7 @@ def stop_process(pid, sig): resize(harness, terminal, 12, 80) stopped.set() - monkeypatch.setattr("cmd2.reserved_toolbar.os.kill", stop_process) + monkeypatch.setattr("cmd2.command_toolbar.os.kill", stop_process) with harness.app._reserved_toolbar_context(): original_suspend = harness.app.main_session.app.suspend_to_background if owner in ("command", "pager"): From b163a1ea5d66735ecfed0f14e91322b7cf957131 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Tue, 15 Sep 2026 07:20:26 -0400 Subject: [PATCH 28/44] Assert pipeline isolation without a platform branch in the toolbar test --- tests/test_command_toolbar.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 6b9c8feb5..2e8d0dcde 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -129,14 +129,13 @@ def test_pipeline_process_group_selection(toolbar_app, tmp_path, monkeypatch, ru with mock.patch("subprocess.Popen", wraps=subprocess.Popen) as popen: app.onecmd_plus_hooks(f'help | "{sys.executable}" -S -c "import sys; print(sys.stdin.read())"') options = popen.call_args.kwargs - if sys.platform == "win32": - assert options["creationflags"] == subprocess.CREATE_NEW_PROCESS_GROUP - assert "start_new_session" not in options - else: - # A stream claiming isatty() is insufficient: these files do not - # refer to our controlling terminal, so no foreground handoff is safe. - assert options["start_new_session"] - assert "process_group" not in options + # A stream claiming isatty() is insufficient: these files do not refer to our + # controlling terminal, so no foreground handoff is safe. The pipeline is isolated + # the ordinary way instead: a new process group on Windows, a new session on POSIX. + isolation = "creationflags" if sys.platform == "win32" else "start_new_session" + expected = subprocess.CREATE_NEW_PROCESS_GROUP if sys.platform == "win32" else True + assert options[isolation] == expected + assert options.keys().isdisjoint({"process_group", "start_new_session", "creationflags"} - {isolation}) @pytest.mark.parametrize("builtin_pager", [False, True]) From 416c6dae87214cd0e2866f5280b9da06c817e8dd Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Tue, 15 Sep 2026 17:45:17 -0400 Subject: [PATCH 29/44] Cover both outcomes of the wait after sending SIGTSTP --- tests/test_command_toolbar.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 2e8d0dcde..234d8d240 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -242,6 +242,37 @@ def test_command_toolbar_ctrl_z(toolbar_app, supported, enabled) -> None: assert [key.key for key in keys] == ([] if supported and enabled else [Keys.ControlZ]) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX job control") +@pytest.mark.parametrize("stopped", [True, False]) +def test_suspend_process_group_waits_for_the_stop_to_take_its_thread(stopped) -> None: + """The sender sleeps until the stop reaches it, or gives up once nothing has happened. + + A stop shows as time passing while asleep, so a clock that jumps across the first sleep + stands in for a job that was stopped and then resumed. A signal that was ignored, or + discarded for an orphaned group, lets the clock advance only by what was slept. + """ + import signal + + clock = [100.0] + sleeps = [] + + def sleep(seconds): + sleeps.append(seconds) + clock[0] += 5.0 if stopped and len(sleeps) == 1 else seconds + + with ( + mock.patch("cmd2.command_toolbar.time", SimpleNamespace(monotonic=lambda: clock[0], sleep=sleep)), + mock.patch("cmd2.command_toolbar.os.kill") as kill, + ): + command_toolbar.suspend_process_group() + kill.assert_called_once_with(0, signal.SIGTSTP) + if stopped: + assert sleeps == [0.01] + else: + assert len(sleeps) > 1 + assert clock[0] >= 100.25 - 1e-6 + + def test_command_toolbar_script_output_has_no_batching_delay(toolbar_app) -> None: app, _, output = toolbar_app sleep = mock.Mock() From 72ae6e59489a800cae64a2b7825f0009b5eb7d5f Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 20 Sep 2026 10:13:08 -0400 Subject: [PATCH 30/44] Lend the terminal to a pipeline while it starts up A pager such as less puts the terminal in raw mode as it starts, before cmd2 has written to the pipe and lent it the terminal. Its background tcsetattr() was stopped with SIGTTOU, and on macOS that call fails with EINTR once the process is continued instead of being restarted. less ignores the failure, so it ran on a cooked terminal: q needed Enter and arrow and paging keys were echoed as escape sequences. --- cmd2/cmd2.py | 8 ++- tests/test_pipeline_job_control.py | 90 ++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index a1154bacb..bc214a50a 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3613,7 +3613,13 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: if cmd_pipe_proc_reader is None: proc.wait(0.2) else: - cmd_pipe_proc_reader.wait_for_exit(0.2) + # A pager such as less sets its terminal modes as it starts, before it + # reads the pipe. It must own the terminal by then: a background + # tcsetattr() stops it with SIGTTOU, and on macOS that call fails with + # EINTR when the process is continued instead of being restarted. less + # ignores the failure and runs on a cooked terminal. + with cmd_pipe_proc_reader.lend_terminal(): + cmd_pipe_proc_reader.wait_for_exit(0.2) # Check if the pipe process already exited if proc.returncode is not None: diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index efdf6d142..bb19476a8 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -547,3 +547,93 @@ def pager_shown(): os.close(master) process.kill() process.wait(timeout=5) + + +@pytest.mark.parametrize("mode", ["off", "reserved"]) +def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path, mode) -> None: + """A pager such as less puts the terminal in raw mode as it starts, before reading its pipe. + + It has to own the terminal by then. A background tcsetattr() stops it with SIGTTOU, and + on macOS the call then fails with EINTR once it is continued rather than being restarted. + less ignores that failure, leaving a cooked terminal: q needs Enter and keys are echoed. + """ + import pty + import termios + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + pager = tmp_path / "pager.py" + outcome = tmp_path / "outcome" + pager.write_text( + "import os, pathlib, sys, termios, tty\n" + "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" + " saved = termios.tcgetattr(terminal)\n" + " try:\n" + # Like less, make a single attempt and carry on whatever comes of it. + " try:\n" + " tty.setcbreak(terminal)\n" + " result = 'ok'\n" + " except termios.error as error:\n" + " result = repr(error)\n" + f" pathlib.Path({str(outcome)!r}).write_text(result)\n" + " while os.read(terminal.fileno(), 1) != b'q': pass\n" + " finally:\n" + " termios.tcsetattr(terminal, termios.TCSANOW, saved)\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd, ToolbarMode\n" + f"app = Cmd(bottom_toolbar_mode=ToolbarMode.{mode.upper()})\n" + "app.prompt = 'TEST> '\n" + "app.main_session.bottom_toolbar = 'STATUS'\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + os.write(master, f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n".encode()) + wait_until(outcome.exists) + wait_until(lambda: outcome.read_text() != "") + assert outcome.read_text() == "ok" + assert not termios.tcgetattr(master)[3] & termios.ICANON + # A cooked terminal would hold the key back until Enter. + start = len(transcript) + os.write(master, b"q") + wait_until(lambda: "TEST>" in transcript[start:]) + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) From 7f574f5203bd7589b26a63490d7280c71b278a57 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 20 Sep 2026 10:34:33 -0400 Subject: [PATCH 31/44] Let a shell producer keep the terminal after its consumer exits When a pipeline's consumer exited, its watcher returned the terminal to cmd2 even while do_shell() was still lending it to a producer running in the same group. The producer's next terminal read stopped it with SIGTTIN, and nothing watches an ordinary shell command for stops, so do_shell() waited forever. The watcher now leaves the terminal alone while a lend is active; the lender returns it when it finishes. --- cmd2/utils.py | 7 ++- tests/test_pipeline_job_control.py | 82 ++++++++++++++++++++++++++++++ tests/test_utils.py | 26 +++++++++- 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index 5e4ee3b8f..961087a96 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -747,8 +747,11 @@ def _wait_for_job(self, terminal_fd: int) -> None: os.killpg(self._proc.pid, signal.SIGCONT) finally: try: - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) + with self._terminal_lock: + # A shell producer in this group may outlive the consumer and still read + # the terminal. While a lend is active, its holder returns the terminal. + if not self._terminal_available.is_set() and os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) finally: self._process_done.set() diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index bb19476a8..61bfbe581 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -637,3 +637,85 @@ def wait_until(predicate): os.close(master) process.kill() process.wait(timeout=5) + + +def test_shell_producer_keeps_the_terminal_after_its_consumer_exits(tmp_path) -> None: + """A shell producer that outlives its consumer still reads the terminal. + + do_shell() lends the terminal to the pipeline's group for as long as the producer runs. + The consumer's exit must not take it back early: the producer would stop with SIGTTIN on + its next terminal read, and nothing watches an ordinary shell command for stops. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + consumer = tmp_path / "consumer.py" + consumer.write_text("import os, time\ntime.sleep(0.5)\nos.write(2, b'CONSUMER_DONE\\n')\n", encoding="utf-8") + producer = tmp_path / "producer.py" + producer.write_text( + "import os, signal, time\n" + # Interactive bash leaves TTIN ignored in what it execs, which turns a background read into EIO. + "signal.signal(signal.SIGTTIN, signal.SIG_DFL)\n" + "time.sleep(1.5)\n" + "os.write(2, b'PRODUCER> ')\n" + "os.write(2, b'GOT ' + os.read(0, 7))\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "from cmd2 import Cmd, ToolbarMode\n" + "app = Cmd(bottom_toolbar_mode=ToolbarMode.OFF)\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + python = shlex.quote(sys.executable) + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{python} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + os.write(master, f"shell {python} {shlex.quote(str(producer))} | {python} {shlex.quote(str(consumer))}\n".encode()) + wait_until(lambda: "CONSUMER_DONE\r\n" in transcript) + wait_until(lambda: "PRODUCER> " in transcript) + os.write(master, b"answer\n") + wait_until(lambda: "GOT answer" in transcript) + # cmd2 owns the terminal again once the producer is done. + start = len(transcript) + wait_until(lambda: "TEST>" in transcript[start:]) + os.write(master, b"help quit\n") + wait_until(lambda: "Exit this application" in transcript[start:]) + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) diff --git a/tests/test_utils.py b/tests/test_utils.py index 5b2adb980..ba4307511 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -392,11 +392,35 @@ def handoff(timeout): assert available.call_count == (2 if expired_handoff else 1) killpg.assert_called_once_with(proc.pid, signal.SIGCONT) stop.assert_not_called() - foreground.assert_called_once_with(10, reader._original_group) + # The lend is still active: its holder returns the terminal, not the watcher. + foreground.assert_not_called() assert proc.returncode == 0 assert reader._process_done.is_set() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("lent", [False, True]) +def test_proc_reader_exit_returns_terminal_unless_lent(lent) -> None: + """A shell producer in a lent pipeline group may outlive the consumer and still need the terminal.""" + proc = mock.Mock(pid=123, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + if lent: + reader._terminal_available.set() + with ( + mock.patch("os.waitpid", return_value=(proc.pid, 0)), + mock.patch("os.tcgetpgrp", return_value=proc.pid), + mock.patch.object(reader, "_set_foreground_group") as foreground, + ): + reader._wait_for_job(10) + if lent: + foreground.assert_not_called() + else: + foreground.assert_called_once_with(10, reader._original_group) + assert reader._process_done.is_set() + + def test_proc_reader_wait_for_exit_without_terminal() -> None: proc = mock.Mock(stdout=None, stderr=None) reader = cu.ProcReader(proc, sys.stdout, sys.stderr) From dc1a9df573326b42b90001d0b902f01335b77b0a Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 20 Sep 2026 11:19:18 -0400 Subject: [PATCH 32/44] Relay a shell producer's stop once its consumer is gone After a pipeline's consumer exits, a surviving shell producer is all that is left of the foreground group, so Ctrl-Z reaches it alone. The watcher that relays stops to the whole job ended with the consumer, and do_shell() polled a stopped child forever while the outer shell never regained the terminal. A ProcReader for a command that joined a pipeline's job is now the only waitpid caller for it, sees its stops as well as its exit, and suspends the whole job through the pipeline's existing SIGTSTP handler. --- cmd2/cmd2.py | 19 ++++------ cmd2/utils.py | 60 ++++++++++++++++++++++++++++-- tests/test_pipeline_job_control.py | 13 ++++++- tests/test_utils.py | 56 ++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 15 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index bc214a50a..dadc83ebe 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5289,17 +5289,14 @@ def do_shell(self, args: argparse.Namespace) -> None: if kwargs.pop("process_group", None) is None: raise - proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr) - if pipeline_group is not None: - # Only the main thread runs Python signal handlers, and the job-control - # stop the pipeline's watcher relays may wake another thread. Return from - # the wait regularly so the handler runs while the command is still going. - while True: - try: - proc_reader.wait_for_exit(0.1) - break - except subprocess.TimeoutExpired: - continue + # A command that joined the pipeline's job is waited for in short polls. Only the + # main thread runs Python signal handlers, and the job-control stop the pipeline's + # watcher relays may wake another thread. Once the consumer and its watcher are + # gone, the same wait relays the command's own stops, such as Ctrl-Z. + joined_pipeline = pipeline if "process_group" in kwargs else None + proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, pipeline=joined_pipeline) + if joined_pipeline is not None: + proc_reader.wait_for_exit() proc_reader.wait() # Save the return code of the application for use in a pyscript diff --git a/cmd2/utils.py b/cmd2/utils.py index 961087a96..2a3887da0 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -543,7 +543,13 @@ class ProcReader: """ def __init__( - self, proc: PopenTextIO, stdout: StdSim | TextIO, stderr: StdSim | TextIO, *, terminal_fd: int | None = None + self, + proc: PopenTextIO, + stdout: StdSim | TextIO, + stderr: StdSim | TextIO, + *, + terminal_fd: int | None = None, + pipeline: "ProcReader | None" = None, ) -> None: """ProcReader initializer. @@ -551,11 +557,13 @@ def __init__( :param stdout: the stream to write captured stdout :param stderr: the stream to write captured stderr. :param terminal_fd: controlling terminal to lend to a POSIX process in its own group + :param pipeline: terminal pipeline whose process group proc joined as a producer """ self._proc = proc self._stdout = stdout self._stderr = stderr self._terminal_fd = terminal_fd + self._pipeline = pipeline self._process_done = threading.Event() self._producer_finished = False self._terminal_available = threading.Event() @@ -755,6 +763,49 @@ def _wait_for_job(self, terminal_fd: int) -> None: finally: self._process_done.set() + def _relay_producer_stop(self) -> None: + """Suspend the shell's whole job for a stopped producer that outlived the consumer. + + Ctrl-Z reaches only the foreground group, and the producer may be all that is + left of it. The watcher ended with the consumer, so nothing else relays the stop. + """ + import signal + + terminal_fd = self._terminal_fd + if terminal_fd is None or not self._process_done.is_set(): + # A live watcher relays the consumer's stop and continues the whole group. + return + with self._terminal_lock: + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + with contextlib.suppress(ProcessLookupError): + os.killpg(self._proc.pid, signal.SIGSTOP) + self._job_resumed.clear() + signal.pthread_kill(threading.main_thread().ident or 0, signal.SIGTSTP) + self._job_resumed.wait() + with contextlib.suppress(ProcessLookupError): + os.killpg(self._proc.pid, signal.SIGCONT) + + def _wait_for_producer(self, pipeline: "ProcReader", timeout: float | None) -> None: + """Wait for a producer in a terminal pipeline's job, relaying its job-control stops. + + This is the only waitpid caller for such a producer. It polls so that the main + thread keeps returning to Python code, where signal handlers run. + """ + import time + + deadline = None if timeout is None else time.monotonic() + timeout + while self._proc.returncode is None: + pid, status = os.waitpid(self._proc.pid, os.WNOHANG | os.WUNTRACED) + if not pid: + if deadline is not None and time.monotonic() >= deadline: + raise subprocess.TimeoutExpired(self._proc.args, timeout or 0) + time.sleep(0.05) + elif os.WIFSTOPPED(status): + pipeline._relay_producer_stop() + else: + self._proc.returncode = os.waitstatus_to_exitcode(status) + def finish_producer(self) -> None: """Disable producer cancellation before flushing and closing its pipe.""" self._producer_finished = True @@ -765,7 +816,9 @@ def wait_for_exit(self, timeout: float | None = None) -> None: :param timeout: maximum seconds to wait, or None to wait indefinitely :raises subprocess.TimeoutExpired: if the process is still running after timeout """ - if self._terminal_fd is None: + if self._pipeline is not None: + self._wait_for_producer(self._pipeline, timeout) + elif self._terminal_fd is None: self._proc.wait(timeout) elif timeout is None: # A process-directed signal may reach a worker thread. Python still runs @@ -812,7 +865,8 @@ def _reader_thread_func(self, read_stdout: bool) -> None: raise ValueError("read_stream is None") # Run until process completes - while (self._proc.poll() if self._terminal_fd is None else self._proc.returncode) is None: + polled = self._terminal_fd is None and self._pipeline is None + while (self._proc.poll() if polled else self._proc.returncode) is None: available = read_stream.peek() # type: ignore[attr-defined, ty:unresolved-attribute] if available: read_stream.read(len(available)) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 61bfbe581..dd1a6d134 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -639,12 +639,17 @@ def wait_until(predicate): process.wait(timeout=5) -def test_shell_producer_keeps_the_terminal_after_its_consumer_exits(tmp_path) -> None: +@pytest.mark.parametrize("suspend", [False, True]) +def test_shell_producer_keeps_the_terminal_after_its_consumer_exits(tmp_path, suspend) -> None: """A shell producer that outlives its consumer still reads the terminal. do_shell() lends the terminal to the pipeline's group for as long as the producer runs. The consumer's exit must not take it back early: the producer would stop with SIGTTIN on its next terminal read, and nothing watches an ordinary shell command for stops. + + Ctrl-Z then reaches the producer alone, since it is all that is left of the foreground + group. With the consumer's watcher gone, do_shell() has to relay that stop to the + whole job itself, or it waits forever on a stopped child. """ import pty @@ -706,6 +711,12 @@ def wait_until(predicate): os.write(master, f"shell {python} {shlex.quote(str(producer))} | {python} {shlex.quote(str(consumer))}\n".encode()) wait_until(lambda: "CONSUMER_DONE\r\n" in transcript) wait_until(lambda: "PRODUCER> " in transcript) + if suspend: + start = len(transcript) + os.write(master, b"\x1a") + wait_until(lambda: os.tcgetpgrp(master) == process.pid and "OUTER> " in transcript[start:]) + os.write(master, b"fg\n") + wait_until(lambda: os.tcgetpgrp(master) not in (process.pid, os.getpgid(process.pid))) os.write(master, b"answer\n") wait_until(lambda: "GOT answer" in transcript) # cmd2 owns the terminal again once the producer is done. diff --git a/tests/test_utils.py b/tests/test_utils.py index ba4307511..9e2736693 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -6,6 +6,7 @@ import os import signal import sys +import threading import time from unittest import ( mock, @@ -754,3 +755,58 @@ def bar_method(self) -> None: cu.categorize([func2, b.bar_method], category) assert getattr(func2, attr_name) == category assert getattr(Bar.bar_method, attr_name) == category + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +def test_proc_reader_producer_wait_times_out() -> None: + import subprocess + + pipeline = mock.Mock() + proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) + with mock.patch("os.waitpid", return_value=(0, 0)), pytest.raises(subprocess.TimeoutExpired): + reader.wait_for_exit(0) + pipeline._relay_producer_stop.assert_not_called() + proc.wait.assert_not_called() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("watcher_done", [False, True]) +@pytest.mark.parametrize("foreground_group", [123, 456]) +def test_proc_reader_relays_producer_stop_once_the_watcher_is_gone(watcher_done, foreground_group) -> None: + consumer = mock.Mock(pid=123, stdout=None, stderr=None, returncode=0) + pipeline = cu.ProcReader(consumer, sys.stdout, sys.stderr) + pipeline._terminal_fd = 10 + pipeline._original_group = 456 + if watcher_done: + pipeline._process_done.set() + proc = mock.Mock(pid=321, stdout=None, stderr=None, returncode=None) + reader = cu.ProcReader(proc, sys.stdout, sys.stderr, pipeline=pipeline) + stopped_status = (signal.SIGTSTP << 8) | 0x7F + + def resume(thread_id, signum): + assert thread_id == threading.main_thread().ident + assert signum == signal.SIGTSTP + pipeline._job_resumed.set() + + with ( + mock.patch("os.waitpid", side_effect=[(proc.pid, stopped_status), (proc.pid, 0)]), + mock.patch("os.tcgetpgrp", return_value=foreground_group), + mock.patch.object(pipeline, "_set_foreground_group") as foreground, + mock.patch("os.killpg") as killpg, + mock.patch("signal.pthread_kill", side_effect=resume) as relay, + ): + reader.wait_for_exit() + assert proc.returncode == 0 + if not watcher_done: + # The consumer's watcher sees the same Ctrl-Z and suspends the job itself. + relay.assert_not_called() + killpg.assert_not_called() + foreground.assert_not_called() + return + relay.assert_called_once() + assert killpg.call_args_list == [mock.call(consumer.pid, signal.SIGSTOP), mock.call(consumer.pid, signal.SIGCONT)] + if foreground_group == consumer.pid: + foreground.assert_called_once_with(10, pipeline._original_group) + else: + foreground.assert_not_called() From 11c36884b86c9958bb5262a9e64cfc70dd1a86d4 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sun, 20 Sep 2026 11:34:10 -0400 Subject: [PATCH 33/44] Send pager test keys from the application's event loop thread A Windows pipe input parses sent text in the caller's thread, while the running application flushes that same parser from its event loop once ttimeoutlen has passed. The parser is a generator, so the two overlapping raised "generator already executing" in test_pager_search_abort_keys on free-threaded Python 3.14t. Hand the send to the application's loop so parsing and flushing happen on one thread. --- tests/test_pager.py | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/tests/test_pager.py b/tests/test_pager.py index 8dfae5d08..0af1bbdd4 100644 --- a/tests/test_pager.py +++ b/tests/test_pager.py @@ -80,13 +80,13 @@ def observe(ui): def interact(): try: assert entered.wait(2) - pipe.send_text("\x1b[C" if chop else " ") + send_keys(app, pipe, "\x1b[C" if chop else " ") assert scrolled.wait(2) app.main_session.output.size = Size(rows=20, columns=60) app.main_session.app.invalidate() assert resized.wait(2) finally: - pipe.send_text("qnext\n") + send_keys(app, pipe, "qnext\n") app.main_session.app.after_render += observe with ThreadPoolExecutor() as executor: @@ -99,10 +99,31 @@ def interact(): assert app._read_raw_input("Next: ", app.main_session) == "next" +def send_keys(app, pipe, text) -> None: + """Send text to the pipe input from the thread that runs the prompt-toolkit application. + + A Windows pipe input parses the text in the caller's thread, while the application + flushes that same parser from its event loop once ttimeoutlen has passed. The parser + is a generator, so the two overlapping raise "generator already executing", which a + free-threaded interpreter makes likely. A POSIX pipe input is read by the loop itself. + """ + loop = app.main_session.app.loop + if loop is not None: + try: + loop.call_soon_threadsafe(pipe.send_text, text) + except RuntimeError: + # The application finished and closed its loop in the meantime. + pass + else: + return + pipe.send_text(text) + + class PagerKeys: """Send keys to the built-in pager and wait for the frame that reflects them.""" def __init__(self, app, pipe, pager) -> None: + self.app = app self.pipe = pipe self.pager = pager self.presses = 0 @@ -134,7 +155,7 @@ def press(self, keys, row, column=0) -> None: """Send keys and wait for a drawn frame that shows the expected position.""" with self.updated: handled = self.presses - self.pipe.send_text(keys) + send_keys(self.app, self.pipe, keys) deadline = time.monotonic() + 5 index = 0 with self.updated: @@ -168,7 +189,7 @@ def interact(): assert entered.wait(5) script(PagerKeys(app, pipe, created[0])) finally: - pipe.send_text("q") + send_keys(app, pipe, "q") app.main_session.app.after_render += observe with mock.patch("cmd2.command_toolbar.Pager", side_effect=make_pager), ThreadPoolExecutor() as executor: @@ -310,11 +331,11 @@ def observe(ui) -> None: def interact() -> None: assert entered.wait(5), "pager never opened" # Scroll first, so the pager is known to be reading keys before the close key. - pipe.send_text("j") - pipe.send_text(key) + send_keys(app, pipe, "j") + send_keys(app, pipe, key) if not closed.wait(5): # Rescue the blocked main thread so this fails as an assertion, not a hang. - pipe.send_text("q") + send_keys(app, pipe, "q") raise AssertionError(f"{key!r} did not close the pager") app.main_session.app.after_render += observe From add03684f59ef32c6a87e0e84b8bbcc01231da08 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 21:19:13 -0400 Subject: [PATCH 34/44] Start the startup-mode test's pager without site-packages cmd2 lends a new pipeline the terminal for its 0.2s startup check, and a pager must set its terminal modes within it. Coverage's subprocess hook, loaded from site-packages, started coverage in the test's Python pager and slowed its startup enough on a busy macOS runner to miss that window. Its tcsetattr() then ran in the background and failed with EINTR. Run the pager with -S so it starts as quickly as less would. --- tests/test_pipeline_job_control.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index dd1a6d134..955375a35 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -622,7 +622,10 @@ def wait_until(predicate): wait_until(lambda: "OUTER> " in transcript) os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) wait_until(lambda: "TEST>" in transcript) - os.write(master, f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n".encode()) + # cmd2 lends the terminal for its 0.2s startup check. Like less, the pager must set its modes + # within it. -S skips site-packages, where coverage's subprocess hook would otherwise start + # coverage in the pager and slow its startup enough on a busy CI runner to miss that window. + os.write(master, f"help -v | {shlex.quote(sys.executable)} -S {shlex.quote(str(pager))}\n".encode()) wait_until(outcome.exists) wait_until(lambda: outcome.read_text() != "") assert outcome.read_text() == "ok" From ea472954d34a17116422d20329579d1b2c9c8b85 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:00:35 -0400 Subject: [PATCH 35/44] Fall back from auto mode when prompt-toolkit's version is unknown A frozen or vendored application may ship without package metadata, and importlib.metadata.version() then raises PackageNotFoundError. It escaped cmdloop() instead of letting auto fall back to legacy rendering. Treat an unknown version as unqualified: auto falls back, and reserved reports why. --- cmd2/toolbar_mode.py | 11 ++++++++++- tests/test_toolbar_mode.py | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/cmd2/toolbar_mode.py b/cmd2/toolbar_mode.py index a83e896ec..3b39ce718 100644 --- a/cmd2/toolbar_mode.py +++ b/cmd2/toolbar_mode.py @@ -25,6 +25,7 @@ """ from enum import StrEnum +from importlib.metadata import PackageNotFoundError from importlib.metadata import version as _installed_version from typing import TYPE_CHECKING @@ -84,7 +85,15 @@ def _dependency_capability(version: str | None = None) -> tuple[bool, str]: :param version: the version to judge; the installed one by default :return: whether it is qualified, and a reason suitable for diagnostics """ - installed = version if version is not None else _installed_version("prompt_toolkit") + if version is not None: + installed = version + else: + try: + installed = _installed_version("prompt_toolkit") + except PackageNotFoundError: + # A frozen or vendored application may ship without package metadata. An unknown + # version is not a qualified one, and auto has to fall back rather than fail. + return False, "the installed prompt-toolkit version cannot be determined" if installed in QUALIFIED_PROMPT_TOOLKIT_VERSIONS: return True, "qualified prompt-toolkit" qualified = ", ".join(sorted(QUALIFIED_PROMPT_TOOLKIT_VERSIONS)) diff --git a/tests/test_toolbar_mode.py b/tests/test_toolbar_mode.py index 6911e7366..9726cc4bf 100644 --- a/tests/test_toolbar_mode.py +++ b/tests/test_toolbar_mode.py @@ -7,6 +7,7 @@ """ import io +from importlib.metadata import PackageNotFoundError import pytest from prompt_toolkit.data_structures import Size @@ -14,6 +15,7 @@ from prompt_toolkit.output.vt100 import Vt100_Output import cmd2 +from cmd2 import toolbar_mode from cmd2.toolbar_mode import ( QUALIFIED_PROMPT_TOOLKIT_VERSIONS, ToolbarMode, @@ -65,6 +67,22 @@ def test_an_unqualified_version_is_reported_with_its_number(self) -> None: assert supported is False assert "3.0.99" in reason + def test_missing_package_metadata_is_unqualified(self, monkeypatch) -> None: + """A frozen or vendored application may have no metadata to read the version from.""" + + def missing(name: str) -> str: + raise PackageNotFoundError(name) + + monkeypatch.setattr(toolbar_mode, "_installed_version", missing) + supported, reason = _dependency_capability() + assert supported is False + assert "cannot be determined" in reason + + # auto falls back rather than failing the command loop. + mode, reason = _select_toolbar_mode("auto", qualified_output(), toolbar_enabled=True, interactive=True) + assert mode is ToolbarMode.LEGACY + assert "cannot be determined" in reason + class TestAutomaticSelection: def test_a_qualified_terminal_selects_reserved(self) -> None: From 06608b0c0fed77ea8c0179be5a878cfebf38b753 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:00:35 -0400 Subject: [PATCH 36/44] Keep the command loop running when the toolbar display will not stop A toolbar callback that did not return within the shutdown timeout made toolbar.stop() raise, and the error escaped cmdloop(). Had it not, the next prompt would have refused to run on a terminal the display still held. Report the display that would not stop, as one that cannot start already is, and wait at the main prompt for it to release the terminal. Ctrl-C during that wait ends the loop. --- cmd2/cmd2.py | 29 +++++++++++++++++ tests/test_command_toolbar.py | 59 +++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index dadc83ebe..1864f48e0 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -2170,6 +2170,27 @@ def _require_terminal_ownership(self) -> None: return raise RuntimeError("the bottom toolbar's display has not released the terminal") + def _await_terminal_ownership(self) -> bool: + """Wait for a display that would not stop to release the terminal before the main prompt. + + Prompting while its thread is still inside the application would put two readers on one + terminal, so the command loop waits rather than failing. The thread usually finishes once + its toolbar callback returns. If it never does, Ctrl-C gives up on it and ends the loop. + + :return: whether the terminal is ours to prompt on + """ + display = self._display_holding_terminal + if display is None: + return True + self.perror("Waiting for the bottom toolbar to release the terminal. Press Ctrl-C to quit.") + try: + while display.thread_is_alive: + time.sleep(0.05) + except KeyboardInterrupt: + return False + self._require_terminal_ownership() + return True + @contextlib.contextmanager def suspend_bottom_toolbar(self) -> Iterator[None]: """Temporarily hide the command toolbar and give exclusive access to the terminal. @@ -2278,6 +2299,11 @@ def _command_toolbar_context(self) -> Iterator[None]: with self.sigint_protection: try: toolbar.stop() + except command_toolbar._DisplayStillRunningError as exc: + # The display has already disabled itself and kept hold of the terminal, + # which the next prompt waits for. Like a display that cannot start, it + # must not escape cmdloop() over a toolbar callback that will not return. + self.perror(f"Disabling the bottom toolbar during commands: {exc}") finally: # Always forget a toolbar that has been torn down. Keeping a failed # one would disable the toolbar for the rest of the session. @@ -4158,6 +4184,9 @@ def _cmdloop(self) -> None: self._startup_commands.clear() while not stop: + if not self._await_terminal_ownership(): + break + # Get commands from user try: line = self._read_command_line(self.prompt) diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 234d8d240..4bee349ce 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -1188,6 +1188,65 @@ def test_the_refusal_lifts_when_the_display_finally_exits(toolbar_app, expire_st assert app.main_session.app.erase_when_done == prompt_erase +def test_cmdloop_waits_for_a_display_that_would_not_stop(toolbar_app, monkeypatch, capsys) -> None: + """A toolbar callback that will not return must not end the session. + + The loop reports the display it gave up on, then waits for the terminal back before + prompting on it again rather than failing out of cmdloop(). + """ + app, _, _ = toolbar_app + blocked = threading.Event() + monkeypatch.setattr(command_toolbar, "_SHUTDOWN_TIMEOUT", 0.01) + lines = iter(["wedge", "quit"]) + holders = [] + + def read_command_line(_prompt): + holders.append(app._display_holding_terminal) + return next(lines) + + def release_once_abandoned(): + deadline = time.monotonic() + 5 + while app._display_holding_terminal is None and time.monotonic() < deadline: + time.sleep(0.01) + blocked.set() + + def command(line, **kwargs): + if line == "wedge": + _block_the_display(app, blocked) + threading.Thread(target=release_once_abandoned, daemon=True).start() + return line == "quit" + + monkeypatch.setattr(app, "_read_command_line", read_command_line) + monkeypatch.setattr(app, "onecmd_plus_hooks", command) + try: + app._cmdloop() + finally: + blocked.set() + + assert holders == [None, None] + assert app.main_session.app.layout is app.main_session.layout + err = capsys.readouterr().err + assert "did not stop" in err + assert "Waiting for the bottom toolbar" in err + + +def test_cmdloop_gives_up_on_a_stuck_display_at_ctrl_c(toolbar_app, monkeypatch) -> None: + """A callback that never returns leaves Ctrl-C as the way out, which ends the loop cleanly.""" + app, _, _ = toolbar_app + + class Interrupted: + @property + def thread_is_alive(self) -> bool: + raise KeyboardInterrupt + + app._display_holding_terminal = Interrupted() + read = mock.Mock() + monkeypatch.setattr(app, "_read_command_line", read) + + app._cmdloop() + read.assert_not_called() + + def test_a_surviving_display_stops_another_from_starting(toolbar_app, expire_startup) -> None: app, _, _ = toolbar_app blocked = expire_startup(app) From a6143168b49f435a7fe83d6aaf5048a144610ba5 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:00:35 -0400 Subject: [PATCH 37/44] Take the terminal back before retrying a shell command in cmd2's group When a terminal pipeline exits before a shell command can join its group, do_shell() retries in cmd2's own group. The terminal was still lent to the dead pipeline, so the command stopped with SIGTTIN on its first terminal read and the wait for it never returned. End the lend before retrying. --- cmd2/cmd2.py | 4 ++++ tests/test_cmd2.py | 24 ++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 1864f48e0..ac611c31e 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5317,6 +5317,10 @@ def do_shell(self, args: argparse.Namespace) -> None: # The pipeline exited before the command could join its group. if kwargs.pop("process_group", None) is None: raise + # The retry runs in our own group, so take the terminal back from the dead + # pipeline first. Its watcher left it lent, and the command would otherwise + # stop with SIGTTIN on its first terminal read, with nothing to resume it. + terminal_stack.close() # A command that joined the pipeline's job is waited for in short polls. Only the # main thread runs Python signal handlers, and the job-control stop the pipeline's diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index 6c7f3dfab..b178a25f0 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -438,13 +438,33 @@ def test_shell_falls_back_to_own_group_when_pipeline_exited(base_app, tmp_path) # pipeline can exit between the check and the spawn, like `shell sleep 1 | true`. leader = subprocess.Popen([sys.executable, "-c", "pass"], process_group=0) leader.wait() - base_app._cur_pipe_proc_reader = mock.Mock(terminal_group=leader.pid, lend_terminal=contextlib.nullcontext) - with (tmp_path / "output").open("w+") as output: + lent = [] + + @contextlib.contextmanager + def lend_terminal(): + lent.append(True) + try: + yield + finally: + lent.pop() + + # The retry runs in our own group, so the terminal has to come back from the dead + # pipeline first. Otherwise the command stops with SIGTTIN on its first terminal read. + spawned_while_lent = [] + real_popen = subprocess.Popen + + def popen(*args, **kwargs): + spawned_while_lent.append(bool(lent)) + return real_popen(*args, **kwargs) + + base_app._cur_pipe_proc_reader = mock.Mock(terminal_group=leader.pid, lend_terminal=lend_terminal) + with (tmp_path / "output").open("w+") as output, mock.patch("subprocess.Popen", popen): base_app.stdout = output base_app.do_shell("echo joined") output.seek(0) assert output.read() == "joined\n" assert base_app.last_result == 0 + assert spawned_while_lent == [True, False] @pytest.mark.skipif(sys.platform == "win32", reason="POSIX shell executable") From 9be7a4d6b67d6357309a9bbbeeceec0910ed7074 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:00:36 -0400 Subject: [PATCH 38/44] Block SIGTTOU only while lending the terminal to a pipeline SIGTTOU was blocked on the main thread for the whole life of a terminal pipeline. A signal mask survives fork and exec, so every child started meanwhile, whether a shell producer or a subprocess run by command code, kept SIGTTOU blocked for life and could change terminal modes from the background. The consumer now owns the terminal only during a lend, so block SIGTTOU for the lend alone, and spawn a shell producer, which starts inside one, with it unblocked. --- cmd2/cmd2.py | 25 ++++------ cmd2/utils.py | 52 +++++++++++++++----- tests/test_cmd2.py | 7 ++- tests/test_pipeline_job_control.py | 79 ++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 32 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index ac611c31e..c3a1d77c1 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3621,13 +3621,6 @@ def _redirect_output(self, statement: Statement) -> utils.RedirectionSavedState: # exit must unblock a producer writing to a full pipe immediately. subproc_stdin.close() if terminal_fd is not None: - import signal - - # The producer may still write diagnostics while the consumer owns - # the terminal. Block SIGTTOU after Popen so the child retains normal - # job-control behavior, and restore our mask with the handoff. - previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) - terminal_stack.callback(signal.pthread_sigmask, signal.SIG_SETMASK, previous_mask) cmd_pipe_proc_reader = utils.ProcReader(proc, self.stdout, sys.stderr, terminal_fd=terminal_fd) terminal_stack.enter_context(cmd_pipe_proc_reader.manage_terminal()) @@ -5304,14 +5297,16 @@ def do_shell(self, args: argparse.Namespace) -> None: terminal_stack.enter_context(pipeline.lend_terminal()) while True: try: - # For any stream that is a StdSim, we will use a pipe so we can capture its output - proc = subprocess.Popen( # noqa: S602 - expanded_command, - stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] - stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, - shell=True, - **kwargs, - ) + # For any stream that is a StdSim, we will use a pipe so we can capture its output. + # A command joining the pipeline is spawned inside the lend, which blocks SIGTTOU. + with utils.unblocked_sigttou() if "process_group" in kwargs else contextlib.nullcontext(): + proc = subprocess.Popen( # noqa: S602 + expanded_command, + stdout=subprocess.PIPE if isinstance(self.stdout, utils.StdSim) else self.stdout, # type: ignore[unreachable] + stderr=subprocess.PIPE if isinstance(sys.stderr, utils.StdSim) else sys.stderr, + shell=True, + **kwargs, + ) break except PermissionError: # The pipeline exited before the command could join its group. diff --git a/cmd2/utils.py b/cmd2/utils.py index 2a3887da0..fbdff31a4 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -536,6 +536,22 @@ def write(self, b: bytes) -> None: self.std_sim_instance.flush() +@contextlib.contextmanager +def unblocked_sigttou() -> Iterator[None]: + """Let a child started inside :meth:`ProcReader.lend_terminal` keep normal job control. + + The lend blocks SIGTTOU for its thread, and a child inherits that mask for life. Spawning + touches no terminal, so unblocking it for the spawn alone cannot stop this thread. + """ + import signal + + previous_mask = signal.pthread_sigmask(signal.SIG_UNBLOCK, {signal.SIGTTOU}) + try: + yield + finally: + signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) + + class ProcReader: """Used to capture stdout and stderr from a Popen process if any of those were set to subprocess.PIPE. @@ -685,25 +701,35 @@ def lend_terminal(self) -> Iterator[None]: reads through input(), getpass(), or third-party libraries. Lending during writes lets an interactive consumer drain a full pipe without deadlocking. """ + import signal + terminal_fd = self._terminal_fd if terminal_fd is None or self._proc.returncode is not None: yield return - with self._terminal_lock: - try: - self._set_foreground_group(terminal_fd, self._proc.pid) - except OSError as error: - # The group can disappear before the watcher has reaped its leader. - if error.errno not in (errno.ESRCH, errno.EINVAL): - raise - self._terminal_available.set() + # While the consumer owns the terminal, a signal handler run on this thread may still + # write diagnostics to it. Block SIGTTOU for the lend only: a signal mask survives fork + # and exec, so blocking it for the whole pipeline would leak into every child the + # command starts. A child started during a lend must unblock it; see unblocked_sigttou(). + previous_mask = signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGTTOU}) try: - yield - finally: with self._terminal_lock: - self._terminal_available.clear() - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) + try: + self._set_foreground_group(terminal_fd, self._proc.pid) + except OSError as error: + # The group can disappear before the watcher has reaped its leader. + if error.errno not in (errno.ESRCH, errno.EINVAL): + raise + self._terminal_available.set() + try: + yield + finally: + with self._terminal_lock: + self._terminal_available.clear() + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) + finally: + signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) def _wait_for_job(self, terminal_fd: int) -> None: """Reap a foreground pipeline and relay its stops to the outer shell's job. diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index b178a25f0..d4e6fe375 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -963,10 +963,9 @@ def start_pipe(*args, **kwargs): reader.wait_for_exit.assert_called_once_with(0.2) reader.wait.assert_called_once_with() process.wait.assert_not_called() - assert sigmask.call_args_list == [ - mock.call(signal.SIG_BLOCK, {signal.SIGTTOU}), - mock.call(signal.SIG_SETMASK, set()), - ] + # SIGTTOU is blocked only inside ProcReader's lends. Blocking it for the whole + # pipeline would leak the mask into every child the command starts. + sigmask.assert_not_called() else: process.wait.assert_called_once() assert popen.call_args.kwargs["stdin"].closed diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 955375a35..3410b9ee6 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -642,6 +642,85 @@ def wait_until(predicate): process.wait(timeout=5) +@pytest.mark.parametrize("producer", ["command", "shell"]) +def test_pipeline_children_inherit_an_ordinary_signal_mask(tmp_path, producer) -> None: + """Processes started during a terminal pipeline must not inherit a blocked SIGTTOU. + + cmd2 blocks SIGTTOU for itself while it lends the terminal. A signal mask survives fork + and exec, so a child spawned with it blocked -- a shell producer, or a subprocess run by + command code -- would keep it for life, and change terminal modes from the background + where it should be stopped. + """ + import pty + + shell = shutil.which("bash") + if shell is None: + pytest.skip("requires an interactive bash shell") + probe = tmp_path / "probe.py" + outcome = tmp_path / "outcome" + probe.write_text( + "import pathlib, signal\n" + "blocked = signal.SIGTTOU in signal.pthread_sigmask(signal.SIG_BLOCK, [])\n" + f"pathlib.Path({str(outcome)!r}).write_text(repr(blocked))\n", + encoding="utf-8", + ) + application = tmp_path / "application.py" + application.write_text( + "import subprocess, sys\n" + "from cmd2 import Cmd\n" + "class App(Cmd):\n" + " def do_probe(self, _):\n" + " self.poutput('probing')\n" + f" subprocess.run([sys.executable, {str(probe)!r}], check=True)\n" + "app = App()\n" + "app.prompt = 'TEST> '\n" + "app.cmdloop()\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(os.environ['TEST_SHELL'], ['bash', '--noprofile', '--norc', '-i'])" + ) + env = dict(os.environ, TERM="xterm-256color", PS1="OUTER> ", TEST_SHELL=shell, SHELL=shell) + env["PYTHONPATH"] = str(Path(__file__).resolve().parents[1]) + process = subprocess.Popen([sys.executable, "-c", bootstrap], stdin=slave, stdout=slave, stderr=slave, env=env) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + data = decoder.decode(os.read(master, 65536)) + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}\n{describe_processes(process.pid, master)}") + + command = "probe" if producer == "command" else f"shell {shlex.quote(sys.executable)} {shlex.quote(str(probe))}" + try: + wait_until(lambda: "OUTER> " in transcript) + os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) + wait_until(lambda: "TEST>" in transcript) + start = len(transcript) + os.write(master, f"{command} | cat\n".encode()) + wait_until(lambda: outcome.exists() and outcome.read_text() != "" and "TEST>" in transcript[start:]) + assert outcome.read_text() == "False" + os.write(master, b"quit\n") + wait_until(lambda: os.tcgetpgrp(master) == process.pid) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + @pytest.mark.parametrize("suspend", [False, True]) def test_shell_producer_keeps_the_terminal_after_its_consumer_exits(tmp_path, suspend) -> None: """A shell producer that outlives its consumer still reads the terminal. From 2bee1d365e2076117ba7f92b6eff82c5171a8eb3 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:14:04 -0400 Subject: [PATCH 39/44] Hold cmd2's startup check open in the pager startup-mode test The test raced cmd2's fixed 0.2s startup lend: a pager that set its terminal modes after it ended was stopped with SIGTTOU and, on macOS, failed with EINTR. Skipping coverage made the pager start faster, but a busy free-threaded macOS runner still missed the window. Have the test application hold the startup check open until the pager reports, so the test checks that the pager owns the terminal throughout the check rather than how fast it starts. Record whether the pager already held the terminal, so a failure shows which side of the window it hit. --- tests/test_pipeline_job_control.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/test_pipeline_job_control.py b/tests/test_pipeline_job_control.py index 3410b9ee6..428dd05c6 100644 --- a/tests/test_pipeline_job_control.py +++ b/tests/test_pipeline_job_control.py @@ -570,12 +570,14 @@ def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path, mode) -> Non "with os.fdopen(os.dup(sys.stderr.fileno()), 'rb', buffering=0) as terminal:\n" " saved = termios.tcgetattr(terminal)\n" " try:\n" + # Whether cmd2 had lent the terminal yet, should the attempt fail. + " foreground = os.tcgetpgrp(terminal.fileno()) == os.getpgrp()\n" # Like less, make a single attempt and carry on whatever comes of it. " try:\n" " tty.setcbreak(terminal)\n" " result = 'ok'\n" " except termios.error as error:\n" - " result = repr(error)\n" + " result = f'{error!r}, foreground before the attempt: {foreground}'\n" f" pathlib.Path({str(outcome)!r}).write_text(result)\n" " while os.read(terminal.fileno(), 1) != b'q': pass\n" " finally:\n" @@ -584,7 +586,20 @@ def test_pipeline_pager_can_set_terminal_modes_at_startup(tmp_path, mode) -> Non ) application = tmp_path / "application.py" application.write_text( - "from cmd2 import Cmd, ToolbarMode\n" + "import pathlib, time\n" + "from cmd2 import Cmd, ToolbarMode, utils\n" + # The pipeline's first wait is cmd2's 0.2s startup check. Hold it open until the pager + # has reported, so the test does not race that timer on a busy CI runner: what it checks + # is that the pager owns the terminal throughout the check, however slowly it starts. + "startup_wait = utils.ProcReader.wait_for_exit\n" + "def held_startup_wait(reader, timeout=None):\n" + " utils.ProcReader.wait_for_exit = startup_wait\n" + f" outcome = pathlib.Path({str(outcome)!r})\n" + " deadline = time.monotonic() + 5\n" + " while time.monotonic() < deadline and not (outcome.exists() and outcome.read_text()):\n" + " time.sleep(0.01)\n" + " return startup_wait(reader, timeout)\n" + "utils.ProcReader.wait_for_exit = held_startup_wait\n" f"app = Cmd(bottom_toolbar_mode=ToolbarMode.{mode.upper()})\n" "app.prompt = 'TEST> '\n" "app.main_session.bottom_toolbar = 'STATUS'\n" @@ -622,10 +637,7 @@ def wait_until(predicate): wait_until(lambda: "OUTER> " in transcript) os.write(master, f"{shlex.quote(sys.executable)} {shlex.quote(str(application))}\n".encode()) wait_until(lambda: "TEST>" in transcript) - # cmd2 lends the terminal for its 0.2s startup check. Like less, the pager must set its modes - # within it. -S skips site-packages, where coverage's subprocess hook would otherwise start - # coverage in the pager and slow its startup enough on a busy CI runner to miss that window. - os.write(master, f"help -v | {shlex.quote(sys.executable)} -S {shlex.quote(str(pager))}\n".encode()) + os.write(master, f"help -v | {shlex.quote(sys.executable)} {shlex.quote(str(pager))}\n".encode()) wait_until(outcome.exists) wait_until(lambda: outcome.read_text() != "") assert outcome.read_text() == "ok" From de239390b6181c4a78c201628f6ff5451ce7ffcb Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:34:59 -0400 Subject: [PATCH 40/44] Leave the application's Ctrl-Z handler as found when the reservation ends _job_control saved app.suspend_to_background, the class's bound method, and on exit wrote it back as an instance attribute. The command display installs its thread-safe Ctrl-Z handler only where the application has none of its own, so after a fallback from reserved rendering, or a later legacy loop, Ctrl-Z during a command went back to upstream's handler and could leave the terminal cooked after fg. Delete the attribute instead when nothing had replaced the method. --- cmd2/reserved_toolbar.py | 8 +++++++- tests/test_reserved_toolbar.py | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/cmd2/reserved_toolbar.py b/cmd2/reserved_toolbar.py index af97abcfc..5b2215d5c 100644 --- a/cmd2/reserved_toolbar.py +++ b/cmd2/reserved_toolbar.py @@ -251,6 +251,9 @@ def take_pending_error(self) -> BaseException | None: def _job_control(self, app: Any) -> "Iterator[None]": """Release the physical reservation inside upstream's cooked-mode handoff.""" original = app.suspend_to_background + # Whether anything had replaced upstream's method. The command display installs its own + # only where nothing has, so writing the bound method back would shut it out for good. + replaced = "suspend_to_background" in vars(app) def suspend_to_background(suspend_group: bool = True) -> None: suspend_signal = getattr(signal, "SIGTSTP", None) @@ -270,7 +273,10 @@ def suspend_process() -> None: yield finally: if app.suspend_to_background is suspend_to_background: - app.suspend_to_background = original + if replaced: + app.suspend_to_background = original + else: + del app.suspend_to_background def can_manage(self, session: "PromptSession[Any]") -> bool: """Whether a temporary prompt uses this terminal and its managed input reader.""" diff --git a/tests/test_reserved_toolbar.py b/tests/test_reserved_toolbar.py index bb7cea6c2..a44289042 100644 --- a/tests/test_reserved_toolbar.py +++ b/tests/test_reserved_toolbar.py @@ -874,6 +874,29 @@ def test_suspending_invalidates_a_nested_prompt_bridge_too(self) -> None: class TestJobControl: + def test_stopping_leaves_the_application_as_it_found_it(self) -> None: + """Upstream's method is not put back as an instance attribute when the toolbar stops. + + The command display installs its thread-safe Ctrl-Z handler only where the application + has none of its own, so a restored bound method would keep it out for the rest of the + session after a fallback from reserved rendering. + """ + harness = Harness() + try: + with harness.toolbar: + assert "suspend_to_background" in vars(harness.app) + assert "suspend_to_background" not in vars(harness.app) + + def installed() -> None: + pass + + harness.app.suspend_to_background = installed + with harness.toolbar: + assert harness.app.suspend_to_background is not installed + assert harness.app.suspend_to_background is installed + finally: + harness.close() + @pytest.mark.parametrize("missing", ["support", "signal"]) def test_suspending_to_background_does_nothing_where_it_is_unsupported( self, monkeypatch: pytest.MonkeyPatch, missing: str From 041785f351da5d6776b20ca5364fb8650afe03c2 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:35:00 -0400 Subject: [PATCH 41/44] Take the terminal back only when the last overlapping lend ends do_shell() lends the terminal for as long as a shell producer runs, and a pipe write from another thread lends and returns meanwhile. The first lend to end cleared availability and took the terminal back, so the producer stopped with SIGTTIN on its next terminal read and nothing resumed it. Count lends, and hand the terminal back after the last. --- cmd2/utils.py | 14 +++++++++++--- tests/test_utils.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/cmd2/utils.py b/cmd2/utils.py index fbdff31a4..fbb133db7 100644 --- a/cmd2/utils.py +++ b/cmd2/utils.py @@ -583,6 +583,8 @@ def __init__( self._process_done = threading.Event() self._producer_finished = False self._terminal_available = threading.Event() + # Lends in progress, guarded by _terminal_lock. The terminal is available while any is. + self._lends = 0 self._terminal_lock = threading.RLock() self._job_resumed = threading.Event() if terminal_fd is not None: @@ -720,14 +722,20 @@ def lend_terminal(self) -> Iterator[None]: # The group can disappear before the watcher has reaped its leader. if error.errno not in (errno.ESRCH, errno.EINVAL): raise + self._lends += 1 self._terminal_available.set() try: yield finally: with self._terminal_lock: - self._terminal_available.clear() - if os.tcgetpgrp(terminal_fd) == self._proc.pid: - self._set_foreground_group(terminal_fd, self._original_group) + self._lends -= 1 + # Lends overlap: do_shell() lends for as long as a shell producer runs, + # while a pipe write from another thread lends and returns. Only the last + # to end takes the terminal back, or the producer would stop with SIGTTIN. + if not self._lends: + self._terminal_available.clear() + if os.tcgetpgrp(terminal_fd) == self._proc.pid: + self._set_foreground_group(terminal_fd, self._original_group) finally: signal.pthread_sigmask(signal.SIG_SETMASK, previous_mask) diff --git a/tests/test_utils.py b/tests/test_utils.py index 9e2736693..d5aa5a806 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -498,6 +498,42 @@ def failing_write(): assert foreground.call_args_list == [mock.call(10, 123), mock.call(10, 456)] +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") +@pytest.mark.parametrize("inner", ["nested", "thread"]) +def test_proc_reader_keeps_the_terminal_lent_until_the_last_lend_ends(inner) -> None: + """A shorter lend inside a longer one must not take the terminal back early. + + do_shell() lends for as long as a shell producer runs. A pipe write from another thread + meanwhile lends and returns. If its return took the terminal back, the producer would stop + with SIGTTIN on its next terminal read, and nothing would resume it. + """ + import threading + + reader = cu.ProcReader(mock.Mock(pid=123, stdout=None, stderr=None, returncode=None), sys.stdout, sys.stderr) + reader._terminal_fd = 10 + reader._original_group = 456 + + def short_lend() -> None: + with reader.lend_terminal(): + pass + + with ( + mock.patch("os.tcgetpgrp", return_value=123), + mock.patch.object(reader, "_set_foreground_group") as foreground, + ): + with reader.lend_terminal(): + if inner == "nested": + short_lend() + else: + worker = threading.Thread(target=short_lend) + worker.start() + worker.join() + assert reader._terminal_available.is_set() + assert mock.call(10, 456) not in foreground.call_args_list + assert not reader._terminal_available.is_set() + assert foreground.call_args_list[-1] == mock.call(10, 456) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX terminal job control") @pytest.mark.parametrize("error_number", [errno.ESRCH, errno.EINVAL, errno.EBADF]) def test_proc_reader_handoff_to_disappearing_group(error_number) -> None: From 80ce6a2980f21cc5776fee84dfe5706b7fb6be50 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:35:00 -0400 Subject: [PATCH 42/44] Let Ctrl-C end the wait for a toolbar display that will not stop The stuck display left the terminal in raw mode, where Ctrl-C is only a key, and the binding that would raise SIGINT runs on the event loop the toolbar callback is blocking. So the wait before the next prompt could not be interrupted, despite saying it could. Cook the terminal before waiting, so its driver sends SIGINT again. Test it with a real Ctrl-C on a pseudo-terminal rather than a simulated KeyboardInterrupt. --- cmd2/cmd2.py | 5 +++ tests/test_command_toolbar.py | 80 +++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index c3a1d77c1..1a77eba2c 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -2182,6 +2182,11 @@ def _await_terminal_ownership(self) -> bool: display = self._display_holding_terminal if display is None: return True + # The display left the terminal in raw mode, where Ctrl-C is only a key, and the binding + # that would signal it runs on the event loop the stuck callback is blocking. Cook the + # terminal so its driver sends SIGINT again. This is not undone: a display that finishes + # restores the modes it found, and one given up on should leave the terminal cooked. + self.main_session.app.input.cooked_mode().__enter__() self.perror("Waiting for the bottom toolbar to release the terminal. Press Ctrl-C to quit.") try: while display.thread_is_alive: diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 4bee349ce..99a221b1e 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -1247,6 +1247,86 @@ def thread_is_alive(self) -> bool: read.assert_not_called() +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX pseudo-terminal") +def test_ctrl_c_reaches_the_wait_for_a_stuck_display(tmp_path) -> None: + """A real Ctrl-C, not a simulated KeyboardInterrupt, has to end the wait. + + The stuck display left the terminal in raw mode, where the driver does not turn Ctrl-C + into SIGINT. Only the display's own key binding does that, and its event loop is the one + blocked in the toolbar callback. + """ + import codecs + import os + import pty + import select + + application = tmp_path / "application.py" + application.write_text( + "import threading\n" + "from cmd2 import Cmd, ToolbarMode, command_toolbar\n" + "command_toolbar._SHUTDOWN_TIMEOUT = 0.2\n" + "wedged = threading.Event()\n" + "entered = threading.Event()\n" + "def toolbar():\n" + " if wedged.is_set():\n" + " entered.set()\n" + " threading.Event().wait()\n" + " return 'STATUS'\n" + "class App(Cmd):\n" + " def do_wedge(self, _):\n" + " wedged.set()\n" + " self._command_toolbar.app.invalidate()\n" + " assert entered.wait(5)\n" + "app = App(bottom_toolbar_mode=ToolbarMode.LEGACY)\n" + "app.prompt = 'TEST> '\n" + "app.main_session.bottom_toolbar = toolbar\n" + "app.cmdloop()\n" + "print('LOOP_ENDED', flush=True)\n", + encoding="utf-8", + ) + master, slave = pty.openpty() + bootstrap = ( + "import os, fcntl, sys, termios; os.setsid(); " + "fcntl.ioctl(0, termios.TIOCSCTTY, 0); " + "os.execv(sys.executable, [sys.executable, sys.argv[1]])" + ) + env = dict(os.environ, TERM="xterm-256color") + process = subprocess.Popen( + [sys.executable, "-c", bootstrap, str(application)], stdin=slave, stdout=slave, stderr=slave, env=env + ) + os.close(slave) + decoder = codecs.getincrementaldecoder("utf-8")("replace") + transcript = "" + + def wait_until(predicate): + nonlocal transcript + deadline = time.monotonic() + 10 + while time.monotonic() < deadline: + if select.select([master], [], [], 0.05)[0]: + try: + data = decoder.decode(os.read(master, 65536)) + except OSError: + data = "" + transcript += data + if "\x1b[6n" in data: + # Answer prompt-toolkit's cursor-position request as a terminal would. + os.write(master, b"\x1b[1;1R") + if predicate(): + return + pytest.fail(f"terminal condition timed out:\n{transcript}") + + try: + wait_until(lambda: "TEST>" in transcript) + os.write(master, b"wedge\r") + wait_until(lambda: "Waiting for the bottom toolbar" in transcript) + os.write(master, b"\x03") + wait_until(lambda: "LOOP_ENDED" in transcript) + finally: + os.close(master) + process.kill() + process.wait(timeout=5) + + def test_a_surviving_display_stops_another_from_starting(toolbar_app, expire_startup) -> None: app, _, _ = toolbar_app blocked = expire_startup(app) From d4b1fd377ad1c21f62defd1bab2619cdca3ebd3c Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Wed, 23 Sep 2026 22:35:00 -0400 Subject: [PATCH 43/44] Reset pipe state even when restoring redirected output fails Waiting for a terminal pipeline now hands the terminal back, which can fail, for example after a hangup. _restore_output then left the dead pipeline current and _redirecting set, so ppaged() stopped paging and Ctrl-C went to a process group that was gone. Restore both in a finally. --- cmd2/cmd2.py | 48 +++++++++++++++++++++++++--------------------- tests/test_cmd2.py | 25 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 1a77eba2c..b2452a5dd 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -3733,31 +3733,35 @@ def _restore_output(self, statement: Statement, saved_redir_state: utils.Redirec terminal_stack.callback(saved_redir_state.toolbar_suspension.close) saved_redir_state.toolbar_suspension = None - if saved_redir_state.redirecting: - # If we redirected output to the clipboard - if ( - statement.redirector in (constants.REDIRECTION_OVERWRITE, constants.REDIRECTION_APPEND) - and not statement.redirect_to - ): - self.stdout.seek(0) - write_to_paste_buffer(self.stdout.read()) - - with contextlib.suppress(BrokenPipeError): - # Close the file or pipe that stdout was redirected to - if self._cur_pipe_proc_reader is not None: - self._cur_pipe_proc_reader.finish_producer() - self.stdout.close() + try: + if saved_redir_state.redirecting: + # If we redirected output to the clipboard + if ( + statement.redirector in (constants.REDIRECTION_OVERWRITE, constants.REDIRECTION_APPEND) + and not statement.redirect_to + ): + self.stdout.seek(0) + write_to_paste_buffer(self.stdout.read()) - # Restore self.stdout - self.stdout = cast(TextIO, saved_redir_state.saved_self_stdout) + with contextlib.suppress(BrokenPipeError): + # Close the file or pipe that stdout was redirected to + if self._cur_pipe_proc_reader is not None: + self._cur_pipe_proc_reader.finish_producer() + self.stdout.close() - # Check if we need to wait for the process being piped to - if self._cur_pipe_proc_reader is not None: - self._cur_pipe_proc_reader.wait() + # Restore self.stdout + self.stdout = cast(TextIO, saved_redir_state.saved_self_stdout) - # These are restored regardless of whether the command redirected - self._cur_pipe_proc_reader = saved_redir_state.saved_pipe_proc_reader - self._redirecting = saved_redir_state.saved_redirecting + # Check if we need to wait for the process being piped to. Handing the + # terminal back as it finishes can fail, for example after a hangup. + if self._cur_pipe_proc_reader is not None: + self._cur_pipe_proc_reader.wait() + finally: + # These are restored regardless of whether the command redirected, or whether + # restoring it failed: a pipeline left current would keep ppaged() from paging + # and send Ctrl-C to a process group that is gone. + self._cur_pipe_proc_reader = saved_redir_state.saved_pipe_proc_reader + self._redirecting = saved_redir_state.saved_redirecting def get_command_func(self, command: str) -> BoundCommandFunc[...] | None: """Get the bound command function for a command. diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py index d4e6fe375..537db1a9d 100644 --- a/tests/test_cmd2.py +++ b/tests/test_cmd2.py @@ -971,6 +971,31 @@ def start_pipe(*args, **kwargs): assert popen.call_args.kwargs["stdin"].closed +def test_restore_output_resets_pipe_state_when_the_wait_fails(base_app) -> None: + """A failed handback while waiting for the pipe process must not leave it current. + + Otherwise ppaged() would never page again, and Ctrl-C would keep going to a dead group. + """ + import errno + + statement = base_app.statement_parser.parse("help | less") + saved_stdout = base_app.stdout + saved = cmd2.utils.RedirectionSavedState(saved_stdout, None, False) + saved.redirecting = True + reader = mock.Mock() + reader.wait.side_effect = OSError(errno.EIO, "terminal hung up") + base_app._cur_pipe_proc_reader = reader + base_app._redirecting = True + base_app.stdout = io.StringIO() + + with pytest.raises(OSError, match="terminal hung up"): + base_app._restore_output(statement, saved) + + assert base_app.stdout is saved_stdout + assert base_app._cur_pipe_proc_reader is None + assert base_app._redirecting is False + + def test_send_to_paste_buffer(redirection_app: RedirectionApp, capsys: pytest.CaptureFixture[str], mocker) -> None: # Exercise cmd2's real clipboard redirection against a private backend, not the # shared OS clipboard (which another test run or desktop application can alter). From 7f6b8dc8359e10f3a50bd90703f3e3b058f5a138 Mon Sep 17 00:00:00 2001 From: Todd Leonhardt Date: Sat, 26 Sep 2026 08:52:14 -0400 Subject: [PATCH 44/44] Catch Ctrl-C that lands while the stuck-toolbar notice is printing The wait for a toolbar display that will not stop cooked the terminal and printed its notice before entering the try that catches Ctrl-C. With the terminal cooked, Ctrl-C can arrive at once, and on Linux CI it landed while the notice was still being written, escaping cmdloop() with a traceback. Cook the terminal and print the notice inside the try. --- cmd2/cmd2.py | 13 +++++++------ tests/test_command_toolbar.py | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index b2452a5dd..e944e8b87 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -2182,13 +2182,14 @@ def _await_terminal_ownership(self) -> bool: display = self._display_holding_terminal if display is None: return True - # The display left the terminal in raw mode, where Ctrl-C is only a key, and the binding - # that would signal it runs on the event loop the stuck callback is blocking. Cook the - # terminal so its driver sends SIGINT again. This is not undone: a display that finishes - # restores the modes it found, and one given up on should leave the terminal cooked. - self.main_session.app.input.cooked_mode().__enter__() - self.perror("Waiting for the bottom toolbar to release the terminal. Press Ctrl-C to quit.") try: + # The display left the terminal in raw mode, where Ctrl-C is only a key, and the binding + # that would signal it runs on the event loop the stuck callback is blocking. Cook the + # terminal so its driver sends SIGINT again. This is not undone: a display that finishes + # restores the modes it found, and one given up on should leave the terminal cooked. + # From here on Ctrl-C can arrive at any point, including while the notice is printing. + self.main_session.app.input.cooked_mode().__enter__() + self.perror("Waiting for the bottom toolbar to release the terminal. Press Ctrl-C to quit.") while display.thread_is_alive: time.sleep(0.05) except KeyboardInterrupt: diff --git a/tests/test_command_toolbar.py b/tests/test_command_toolbar.py index 99a221b1e..29f96a784 100644 --- a/tests/test_command_toolbar.py +++ b/tests/test_command_toolbar.py @@ -1247,6 +1247,25 @@ def thread_is_alive(self) -> bool: read.assert_not_called() +def test_ctrl_c_while_announcing_the_wait_ends_the_loop(toolbar_app, monkeypatch) -> None: + """The terminal is cooked before the notice, so Ctrl-C may land while it is still printing.""" + app, _, _ = toolbar_app + + class Stuck: + thread_is_alive = True + + def interrupted(*args, **kwargs): + raise KeyboardInterrupt + + app._display_holding_terminal = Stuck() + monkeypatch.setattr(app, "perror", interrupted) + read = mock.Mock() + monkeypatch.setattr(app, "_read_command_line", read) + + app._cmdloop() + read.assert_not_called() + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX pseudo-terminal") def test_ctrl_c_reaches_the_wait_for_a_stuck_display(tmp_path) -> None: """A real Ctrl-C, not a simulated KeyboardInterrupt, has to end the wait.