Stage 2b review round 2: three more gaps between a frame and the terminal

A managed write with no supplied origin now forgets the remembered one instead
of keeping it. The write moved the cursor and may have scrolled the screen, so
the remembered row is precisely what is no longer true; recovery asks the
terminal rather than jumping to a row the prompt has left.

Preparation refuses to publish a frame when reserved emission was abandoned
during the render, not only when a recovery is owed. A layout callback that
stops emission left the caller holding a frame that was already retired.

The retirement check moved inside the commit transaction. Reading it before the
lock answers a question about a terminal somebody else still held: another
writer can retire the batch while the commit queues, changing neither the
generations nor the size. That check standing in for an explicit recovery guard
was the argument for removing the guard, and the argument only holds here.

Author: tleonhardt

cmd2/prompt_toolkit_bridge.py

@@ -216,11 +216,13 @@ def note_managed_write(self, prompt_anchor: int | None = None) -> None:
         against a screen the terminal no longer shows, from an origin it no longer has.
 
         :param prompt_anchor: the physical row the prompt now starts on, where the layer that
-            emitted the output knows it; recovery asks the terminal otherwise
+            emitted the output knows it. Passing nothing *forgets* the origin rather than
+            keeping the old one: the write moved the cursor and may have scrolled the screen,
+            so the remembered row is exactly what is no longer true, and recovery asks the
+            terminal instead.
         """
         self._terminal_generation += 1
-        if prompt_anchor is not None:
-            self._prompt_anchor = prompt_anchor
+        self._prompt_anchor = prompt_anchor
         self.require_resynchronization("managed output reached the terminal")
         self._request_redraw()
 
@@ -323,10 +325,12 @@ def prepare(self, app: "Application[Any]") -> PreparedRender | None:
             self._renderer.output = original
             self._preparing = False
 
-        if self.needs_resynchronization:
+        if self.needs_resynchronization or self.reserved_emission_stopped:
             # Something invalidated the terminal while the frame was being prepared -- a
-            # managed write from inside a layout callback, say. The operations are already
-            # recorded against a terminal that has moved on.
+            # managed write from inside a layout callback, say, or a failure that abandoned
+            # the reservation outright. Either way the operations are recorded against a
+            # terminal that has moved on, and publishing them would hand the caller a frame
+            # that is already retired.
             self._retire()
             return None
 
@@ -341,14 +345,17 @@ def commit(self, prepared: PreparedRender) -> bool:
         :return: whether the frame was emitted in full
         """
         assert_no_terminal_transaction("committing a prepared frame")
-        if prepared is not self._in_flight:
-            # Already retired, or from a previous attempt. Replaying it would emit a frame
-            # nothing has validated, and possibly emit it twice. Everything that invalidates
-            # the terminal retires the frame in flight, so this one test covers an owed
-            # recovery and an abandoned reservation as well as a superseded batch.
-            return False
-
         with self._lock.transaction("commit", generation=prepared.generations.geometry):
+            if prepared is not self._in_flight:
+                # Already retired, or from a previous attempt. Replaying it would emit a frame
+                # nothing has validated, and possibly emit it twice. Everything that
+                # invalidates the terminal retires the frame in flight, so this one test
+                # covers an owed recovery and an abandoned reservation as well as a superseded
+                # batch -- but only when it is read here, after the terminal has been
+                # acquired. Read before the wait, it answers a question about a terminal
+                # somebody else still held: a writer can retire the batch while this call
+                # queues for the lock, changing neither the generations nor the size.
+                return False
             if self.generations() != prepared.generations:
                 self.require_resynchronization("the terminal changed between preparing and committing")
                 return False

tests/test_prompt_toolkit_bridge.py

@@ -10,6 +10,7 @@
 """
 
 import io
+import threading
 from concurrent.futures import Future
 from typing import Any
 
@@ -41,13 +42,20 @@ def isatty(self) -> bool:
 class Harness:
     """A real application over a reserved terminal, with the stream it writes to."""
 
-    def __init__(self, rows: int = 24, columns: int = 40, reserved_rows: int = 1, content: Any = "hello") -> None:
+    def __init__(
+        self,
+        rows: int = 24,
+        columns: int = 40,
+        reserved_rows: int = 1,
+        content: Any = "hello",
+        lock: TerminalLock | None = None,
+    ) -> None:
         self.stream = TtyStringIO()
         self.size = Size(rows=rows, columns=columns)
         self.backend = Vt100_Output(self.stream, lambda: self.size)
         self.display = TerminalDisplay(self.backend, reserved_rows=reserved_rows)
         assert self.display.acquire() is True
-        self.lock = TerminalLock()
+        self.lock = lock or TerminalLock()
         self.app: Application[Any] = Application(
             layout=Layout(Window(FormattedTextControl(content))),
             output=self.display.output,
@@ -208,7 +216,7 @@ def test_uncommitted_frame_metadata_is_not_dispatched(self) -> None:
         harness = Harness()
         prepared = harness.prepare()
         assert prepared is not None
-        harness.bridge.note_managed_write()
+        harness.bridge.note_managed_write(prompt_anchor=1)
         assert harness.bridge.can_dispatch_input is False
         assert harness.bridge.commit(prepared) is False
         assert harness.bridge.can_dispatch_input is False
@@ -274,7 +282,7 @@ def test_discarded_frame_resynchronizes_terminal_modes(self) -> None:
         assert prepared is not None
         # Preparation advanced the flag even though the terminal saw nothing.
         assert harness.renderer._bracketed_paste_enabled is True
-        harness.bridge.note_managed_write()
+        harness.bridge.note_managed_write(prompt_anchor=1)
         harness.bridge.commit(prepared)
         harness.clear()
 
@@ -291,7 +299,7 @@ def test_recovery_restores_the_baseline_only_after_a_full_frame(self) -> None:
         harness = Harness()
         prepared = harness.prepare()
         assert prepared is not None
-        harness.bridge.note_managed_write()
+        harness.bridge.note_managed_write(prompt_anchor=1)
         harness.bridge.commit(prepared)
         harness.resynchronize()
         harness.clear()
@@ -374,7 +382,7 @@ def test_repeated_invalidation_yields_to_managed_output(self) -> None:
         harness.bridge.set_redraw_scheduler(lambda: scheduled.append(1))
 
         for _ in range(5):
-            harness.bridge.note_managed_write()
+            harness.bridge.note_managed_write(prompt_anchor=1)
 
         assert len(scheduled) == 1
         assert harness.bridge.redraw_pending is True
@@ -387,10 +395,10 @@ def test_a_redraw_is_requested_again_after_it_is_served(self) -> None:
         harness = Harness()
         scheduled: list[int] = []
         harness.bridge.set_redraw_scheduler(lambda: scheduled.append(1))
-        harness.bridge.note_managed_write()
+        harness.bridge.note_managed_write(prompt_anchor=1)
         harness.resynchronize()
         harness.render()
-        harness.bridge.note_managed_write()
+        harness.bridge.note_managed_write(prompt_anchor=1)
         assert len(scheduled) == 2
 
 
@@ -660,3 +668,69 @@ def test_a_cursor_report_is_validated_against_the_screen_when_released(self) ->
         harness.bridge.request_cursor_position()
         assert harness.bridge.report_cursor_row(24) is True
         assert harness.renderer._min_available_height == 24 - 24 + 1
+
+
+class RetiringLock:
+    """A lock that runs a callback at the moment it is handed over.
+
+    This stands in for another writer retiring the batch while a commit waits for the
+    terminal. Driving that with two real threads cannot say *where* the second thread got to
+    before the lock was released -- the interleaving the test is about is the one where the
+    commit is already past its own checks -- so the handover itself is the seam to inject at.
+    """
+
+    def __init__(self) -> None:
+        self._lock = threading.RLock()
+        self.on_acquire: Any = None
+
+    def acquire(self, *args: Any, **kwargs: Any) -> bool:
+        acquired = self._lock.acquire(*args, **kwargs)
+        if self.on_acquire is not None:
+            callback, self.on_acquire = self.on_acquire, None
+            callback()
+        return acquired
+
+    def release(self) -> None:
+        self._lock.release()
+
+
+class TestReviewRegressionsRoundTwo:
+    def test_a_managed_write_without_an_origin_forgets_the_old_one(self) -> None:
+        """Review finding 1: the output moved the cursor, so the remembered row is stale."""
+        harness = Harness()
+        harness.render()
+        assert harness.bridge.prompt_anchor == 1
+        harness.bridge.note_managed_write()
+        assert harness.bridge.prompt_anchor is None
+
+        harness.clear()
+        harness.resynchronize()
+        written = harness.written()
+        assert "\x1b[1;1H" not in written
+        assert "\x1b[6n" in written
+        assert harness.bridge.needs_resynchronization is True
+
+    def test_a_frame_prepared_after_emission_stopped_is_not_published(self) -> None:
+        """Review finding 2: stopping is not the same state as owing a recovery."""
+
+        def content() -> str:
+            harness.bridge.stop_reserved_emission(OSError("terminal went away"))
+            return "hello"
+
+        harness = Harness(content=content)
+        assert harness.prepare() is None
+        assert harness.bridge.in_flight is None
+        assert harness.bridge.reserved_emission_stopped is True
+
+    def test_a_frame_retired_while_the_commit_waits_is_not_emitted(self) -> None:
+        """Review finding 3: retirement only replaces an explicit guard if read under the lock."""
+        handover = RetiringLock()
+        harness = Harness(lock=TerminalLock(lock=handover))
+        prepared = harness.prepare()
+        assert prepared is not None
+        harness.clear()
+
+        handover.on_acquire = lambda: harness.bridge.require_resynchronization("another writer")
+        assert harness.bridge.commit(prepared) is False
+        assert harness.written() == ""
+        assert harness.bridge.needs_resynchronization is True