Stage 2b review round 6: close the publication window, and stop trusting retirement

Checking whether the terminal is still ours and publishing the prepared frame
were two steps. A writer abandoning the reservation between them had its
retirement overwritten by the publication that followed, leaving a frame in
flight that nothing had invalidated. They are one transaction now.

Commit asks about the terminal's state again rather than inferring it from
retirement. Removing that guard in the first review round was wrong: a
publication racing a retirement produces a frame that matches on identity and
on every generation, so identity carries the argument only while nothing else
can publish. It can.

Author: tleonhardt

cmd2/prompt_toolkit_bridge.py

@@ -325,18 +325,22 @@ def prepare(self, app: "Application[Any]") -> PreparedRender | None:
             self._renderer.output = original
             self._preparing = False
 
-        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, 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
-
-        prepared = PreparedRender(batch=recorder.batch(), generations=generations)
-        self._in_flight = prepared
-        return prepared
+        with self._lock.transaction("publish"):
+            # Checking and publishing are one step. Apart, they are two, and a writer that
+            # abandons the reservation between them has its retirement overwritten by the
+            # publication that follows -- leaving a frame in flight that nothing invalidated
+            # and everything downstream believes is current.
+            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, or a failure that
+                # abandoned the reservation outright. Either way the operations are recorded
+                # against a terminal that has moved on.
+                self._retire()
+                return None
+
+            prepared = PreparedRender(batch=recorder.batch(), generations=generations)
+            self._in_flight = prepared
+            return prepared
 
     def commit(self, prepared: PreparedRender) -> bool:
         """Revalidate a prepared frame and, if it is still current, emit it.
@@ -348,13 +352,19 @@ def commit(self, prepared: PreparedRender) -> bool:
         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.
+                # nothing has validated, and possibly emit it twice.
+                #
+                # Read here, after the terminal has been acquired: read before the wait, it
+                # answers a question about a terminal somebody else still held, since a writer
+                # can retire the batch while this call queues, changing neither the
+                # generations nor the size.
+                return False
+            if self._needs_resynchronization or self._reserved_emission_stopped:
+                # Not covered by the identity test above. Retirement clears the frame in
+                # flight, but a preparation completing concurrently can publish a new one over
+                # that retirement, and the frame it publishes matches on identity and on every
+                # generation. What makes it uncommittable is the state of the terminal, so
+                # that is what is asked.
                 return False
             if self.generations() != prepared.generations:
                 self.require_resynchronization("the terminal changed between preparing and committing")

tests/test_prompt_toolkit_bridge.py

@@ -851,3 +851,41 @@ def test_an_unknown_origin_request_that_finds_emission_stopped_writes_nothing(se
         assert harness.written() == ""
         # Nothing was queued either: a reply now would be answering a request never made.
         assert harness.bridge.report_cursor_row(4) is False
+
+    def test_a_frame_is_not_published_over_an_abandonment(self) -> None:
+        """Review finding: checking and publishing must be one step, or one overwrites the other."""
+        handover = RetiringLock()
+        harness = Harness(lock=TerminalLock(lock=handover))
+        # Skip the preflight acquisition; abandon emission as the publication takes the lock.
+        handover.schedule(None, lambda: harness.bridge.stop_reserved_emission(OSError("terminal went away")))
+
+        assert harness.prepare() is None
+        assert harness.bridge.in_flight is None
+
+    def test_a_frame_in_flight_while_emission_is_abandoned_is_not_committed(self) -> None:
+        """Retirement alone cannot carry this: a publication can overwrite a retirement.
+
+        The state is built directly because that is the point -- commit must reject a frame
+        that is in flight while emission is abandoned, whatever sequence produced that pair,
+        rather than trusting that nothing can produce it.
+        """
+        harness = Harness()
+        prepared = harness.prepare()
+        assert prepared is not None
+        harness.bridge.stop_reserved_emission(OSError("terminal went away"))
+        harness.bridge._in_flight = prepared
+        harness.clear()
+
+        assert harness.bridge.commit(prepared) is False
+        assert harness.written() == ""
+
+    def test_a_frame_in_flight_while_recovery_is_owed_is_not_committed(self) -> None:
+        harness = Harness()
+        prepared = harness.prepare()
+        assert prepared is not None
+        harness.bridge.require_resynchronization("another writer")
+        harness.bridge._in_flight = prepared
+        harness.clear()
+
+        assert harness.bridge.commit(prepared) is False
+        assert harness.written() == ""