Stage 3 review: a pause that timed out relinquished nothing, and now says so

Bounding the join stopped the hang but left the caller believing the display
had stopped. It had not: the thread was still inside the application, still
rendering and still reading input. Suspension went on to release the margins
and run the guest, so two programs shared one terminal; the borrowed layout and
key bindings went back to the main prompt underneath a thread still using them;
and the display object was dropped at the end of the command, taking the only
record of the problem with it.

A timed-out pause now raises. Nothing that follows a successful pause happens:
no handoff, no restoration, no typeahead transfer. The caller is the only one
who knows what it was about to do with the terminal, so the failure goes to it
rather than being reported and swallowed.

The refusal is recorded on the application rather than on the display object,
which the next command replaces. The thread outlives it, and one terminal
cannot have two input readers.

Startup keeps propagating the failure that brought it there rather than a
secondary cleanup failure -- the session is disabled either way, and "did not
start" is the useful half. A stop that fails during ordinary teardown still
propagates, as it did before.

Guarding _resume() against a live thread became unreachable once _pause()
refuses instead of returning, so it is gone rather than left as a branch no
test can reach.

Author: tleonhardt

cmd2/command_toolbar.py

@@ -92,10 +92,10 @@ def pipe_target(stream: Any) -> Any:
 
 
 class _DisplayStillRunningError(RuntimeError):
-    """Raised when a previous display's thread has not finished.
+    """Raised when the display's thread did not finish within its timeout.
 
-    One terminal cannot have two input readers, so a display that would not stop is a display
-    that cannot be started again.
+    Nothing was relinquished: the thread is still inside the application, so whatever was
+    about to be done with the terminal must not be.
     """
 
 
@@ -304,7 +304,11 @@ def start(self) -> None:
                     stack.callback(self._restore_stream, obj, name, wrapper)
             self._resume()
         except BaseException:
-            self.stop()
+            # A cleanup that cannot finish is reported by the state it leaves behind -- the
+            # display is disabled for the session either way -- and the failure that brought
+            # us here is the one worth propagating.
+            with contextlib.suppress(Exception):
+                self.stop()
             raise
 
     @staticmethod
@@ -313,10 +317,6 @@ def _restore_stream(obj: Any, name: str, stream: ToolbarStream) -> None:
             setattr(obj, name, stream.original)
 
     def _resume(self) -> None:
-        if self._thread is not None and self._thread.is_alive():
-            # A previous display never finished. One terminal cannot have two input readers,
-            # and the old one still holds the application.
-            raise _DisplayStillRunningError("the bottom toolbar's previous display is still running")
         self._ready.clear()
         self._error = None
         stack = self._display_stack = contextlib.ExitStack()
@@ -449,13 +449,11 @@ def _pause(self) -> None:
                 if self.app.is_running and self.app.loop is not None:
                     self.app.loop.call_soon_threadsafe(self._exit)
                 if self._thread is not None:
+                    # Bounded, so a render callback blocked inside the display cannot hold the
+                    # thread that is tearing it down.
                     self._thread.join(timeout=_SHUTDOWN_TIMEOUT)
                     if self._thread.is_alive():
-                        # Bounded, so a render callback blocked inside the display cannot hold
-                        # the thread that is tearing it down. The reference is kept rather
-                        # than cleared: the thread is still running the application, and
-                        # starting a second one would put two input readers on one terminal.
-                        self.cmd.perror(f"The bottom toolbar did not stop within {_SHUTDOWN_TIMEOUT} seconds")
+                        self._abandon_stuck_display()
                     else:
                         self._thread = None
                 # Return the borrowed application to the main prompt, including on
@@ -471,6 +469,29 @@ def _pause(self) -> None:
         finally:
             self._pausing = False
 
+    def _abandon_stuck_display(self) -> None:
+        """Report that the display did not stop, and refuse to pretend it did.
+
+        A pause that timed out relinquished nothing. The thread is still inside the
+        application: still rendering, still reading input. Everything that would normally
+        follow a pause assumes the opposite -- the caller hands the terminal to a guest, the
+        borrowed layout and key bindings go back to the main prompt, and the display object is
+        dropped at the end of the command. Each of those would be acting on an application
+        that is still running.
+
+        So nothing further happens here. The application's state is left as the running thread
+        expects to find it, and the failure is raised rather than reported and swallowed,
+        because only the caller knows what it was about to do with the terminal.
+
+        The refusal is recorded on the application rather than on this object, which the next
+        command replaces. The thread outlives it, and one terminal cannot have two input
+        readers.
+
+        :raises _DisplayStillRunningError: always
+        """
+        self.cmd._command_toolbar_disabled = True
+        raise _DisplayStillRunningError(f"the bottom toolbar did not stop within {_SHUTDOWN_TIMEOUT} seconds")
+
     def stop(self) -> None:
         """Flush output, stop rendering, and restore the terminal and its streams."""
         try:
@@ -601,10 +622,4 @@ def suspend(self) -> Iterator[None]:
             yield
         finally:
             with self.cmd.sigint_protection:
-                try:
-                    self._resume()
-                except _DisplayStillRunningError as exc:
-                    # The display that would not stop is still holding the application, so
-                    # there is nothing to come back to. Report it once and carry on without a
-                    # toolbar: the command that suspended it is not this failure's to end.
-                    self.cmd.perror(f"Bottom toolbar not restored: {exc}")
+                self._resume()

tests/test_command_toolbar.py

@@ -1,5 +1,6 @@
 """Command toolbar lifecycle and terminal integration tests."""
 
+import contextlib
 import sys
 import threading
 import time
@@ -861,26 +862,74 @@ def blocking_toolbar() -> str:
         blocked.set()
 
 
-def test_command_toolbar_does_not_start_a_second_display_over_a_stuck_one(toolbar_app, monkeypatch, capsys) -> None:
-    """One terminal, one input reader: a display that would not stop cannot be restarted."""
+def _block_the_display(app, blocked: threading.Event) -> None:
+    """Wedge the running display inside a render callback it cannot leave."""
+    app.main_session.bottom_toolbar = lambda: blocked.wait(timeout=10) or "STATUS"
+    app._command_toolbar.app.invalidate()
+
+
+def test_command_toolbar_suspension_does_not_hand_over_a_terminal_it_still_owns(toolbar_app, monkeypatch) -> None:
+    """A pause that timed out did not stop anything, and must not pretend otherwise."""
     app, _, _ = toolbar_app
     blocked = threading.Event()
     monkeypatch.setattr(command_toolbar, "_SHUTDOWN_TIMEOUT", 0.2)
+    entered = []
 
     try:
-        with app._command_toolbar_context():
+        # The command context's own teardown fails the same way, for the same reason: the
+        # display never stopped. That is the established contract for a stop that fails.
+        with contextlib.suppress(RuntimeError), app._command_toolbar_context():
             display = app._command_toolbar
             assert display is not None
-            first_thread = display._thread
+            _block_the_display(app, blocked)
 
-            # Block the display inside a render, so its thread cannot finish.
-            app.main_session.bottom_toolbar = lambda: blocked.wait(timeout=10) or "STATUS"
-            display.app.invalidate()
+            with pytest.raises(RuntimeError, match="did not stop"), app.suspend_bottom_toolbar():
+                entered.append(True)
 
-            with app.suspend_bottom_toolbar():
+            # The guest never ran: the display still owns the application and the terminal.
+            assert entered == []
+            assert display.app.is_running is True
+    finally:
+        blocked.set()
+
+
+def test_command_toolbar_that_would_not_stop_is_not_used_again(toolbar_app, monkeypatch) -> None:
+    """The surviving thread outlives this display object, so the refusal has to as well."""
+    app, _, _ = toolbar_app
+    blocked = threading.Event()
+    monkeypatch.setattr(command_toolbar, "_SHUTDOWN_TIMEOUT", 0.2)
+
+    try:
+        with contextlib.suppress(RuntimeError), app._command_toolbar_context():
+            _block_the_display(app, blocked)
+            with contextlib.suppress(RuntimeError), app.suspend_bottom_toolbar():
+                pass
+
+        assert app._command_toolbar_disabled is True
+
+        # A later command must not start a second display over the one still running.
+        with app._command_toolbar_context():
+            assert app._command_toolbar is None
+    finally:
+        blocked.set()
+
+
+def test_command_toolbar_that_would_not_stop_keeps_the_application(toolbar_app, monkeypatch) -> None:
+    """Its layout and bindings are still in use; restoring them would pull them out from under it."""
+    app, _, _ = toolbar_app
+    blocked = threading.Event()
+    monkeypatch.setattr(command_toolbar, "_SHUTDOWN_TIMEOUT", 0.2)
+
+    try:
+        with contextlib.suppress(RuntimeError), app._command_toolbar_context():
+            display = app._command_toolbar
+            assert display is not None
+            layout = display.app.layout
+            _block_the_display(app, blocked)
+
+            with contextlib.suppress(RuntimeError), app.suspend_bottom_toolbar():
                 pass
 
-            assert display._thread is first_thread
-            assert "not restored" in capsys.readouterr().err
+            assert display.app.layout is layout
     finally:
         blocked.set()