From baffeb3a445650ca42b6c7cfd09cfbbf1ed11c1a Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Mon, 27 Apr 2026 04:47:26 -0400
Subject: [PATCH] =?UTF-8?q?chore:=20scene-close-on-cancel=20=E2=80=94=20st?=
 =?UTF-8?q?rengthen=20regression=20test=20+=20document=20rationale=20(T108?=
 =?UTF-8?q?)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Investigation surfaced a transactional bug in the cancel path: when the
primary stream raises asyncio.CancelledError mid-stream, post_turn
re-raises at end-of-function, and open_db's dependency teardown skips
conn.commit() — rolling back ALL post-cancel writes including the
scene_closed event. The existing T74.3 regression test only passes
because asyncio is not imported at module scope, so CancelledError
becomes NameError (caught by except Exception, leaves cancelled=False).
Documented in turns.py + test docstring; deferred for triage.
---
 chat/web/turns.py       | 14 ++++++++++++++
 tests/test_turn_flow.py | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/chat/web/turns.py b/chat/web/turns.py
index 97ef4a6..dfb4b21 100644
--- a/chat/web/turns.py
+++ b/chat/web/turns.py
@@ -873,6 +873,20 @@ async def post_turn(
     # mid-stream still meant to close the scene — the cancelled bot
     # beat doesn't invalidate that intent. Pinned by
     # test_cancelled_turn_still_closes_scene_when_user_prose_signals_close.
+    #
+    # T108 NOTE — the in-memory append order is correct, but the cancel
+    # path re-raises ``CancelledError`` at the end of ``post_turn``
+    # (see step 11 below). The ``open_db`` dependency teardown skips
+    # ``conn.commit()`` when the consumer raises, which means in
+    # production a genuine cancel currently rolls back ALL post-cancel
+    # writes — including this scene_closed event, the truncated
+    # assistant_turn record, edge updates, and per-POV summaries. The
+    # T74.3 regression test passes only because of a missing
+    # ``import asyncio`` in the test module: the inline mock raises
+    # ``NameError`` instead of ``CancelledError``, which is caught by
+    # the ``except Exception:`` branch and leaves ``cancelled=False``,
+    # so the function returns 204 normally and the commit fires. This
+    # is a transactional bug deferred for triage (T108 report).
     if scene is not None and prose.strip():
         container = None
         if scene.get("container_id") is not None:
diff --git a/tests/test_turn_flow.py b/tests/test_turn_flow.py
index 043fa78..9d3fd0f 100644
--- a/tests/test_turn_flow.py
+++ b/tests/test_turn_flow.py
@@ -734,6 +734,19 @@ def test_cancelled_turn_still_closes_scene_when_user_prose_signals_close(
     that as an exception, so we drive the request inside ``with
     pytest.raises``. Despite the exception, the scene_closed event
     must land in the event_log.
+
+    T108 NOTE — this test does NOT actually exercise the cancel path.
+    ``_CancelOnStreamMock.stream`` writes ``raise asyncio.CancelledError``
+    but ``asyncio`` is not imported at module scope, so the first
+    iteration raises ``NameError`` (caught by ``except Exception:`` in
+    post_turn, which sets ``primary_truncated=True`` but leaves
+    ``cancelled=False``). The function therefore returns 204 normally,
+    the dependency-managed connection commits, and ``scene_closed``
+    lands. Importing asyncio so the real CancelledError fires reveals
+    a transactional bug: ``post_turn``'s end-of-function re-raise
+    causes ``open_db``'s dependency teardown to skip ``conn.commit()``,
+    rolling back ALL post-cancel writes (user_turn, assistant_turn,
+    edge_updates, scene_closed). Deferred for triage — see T108 report.
     """
     from typing import AsyncIterator, Sequence
 
@@ -828,12 +841,33 @@ def test_cancelled_turn_still_closes_scene_when_user_prose_signals_close(
             "SELECT payload_json FROM event_log "
             "WHERE kind = 'assistant_turn' ORDER BY id"
         ).fetchall()
+        # T108: pin the ordering — user_turn must commit before
+        # scene_closed (close detection runs on prose that is already
+        # in the event_log) and any assistant_turn the cancel produced
+        # must come last (truncated record written after both).
+        ordered = conn.execute(
+            "SELECT id, kind FROM event_log "
+            "WHERE kind IN ('user_turn', 'scene_closed', 'assistant_turn') "
+            "ORDER BY id"
+        ).fetchall()
 
     # Scene close lands despite the cancel.
     assert scene_close_count == 1
     # The cancelled assistant_turn was still recorded (truncated=True).
     assert len(assistant_payload) == 1
     assert json.loads(assistant_payload[0][0])["truncated"] is True
+    # T108 ordering pin: user_turn lands first, the truncated
+    # assistant_turn (if any) is committed BEFORE the scene_close
+    # decision fires, and scene_closed lands last. Close detection
+    # relies on user prose being committed to the event_log BEFORE
+    # the close decision runs — and the cancelled assistant beat is
+    # recorded as a partial before close-detection too.
+    kinds_in_order = [row[1] for row in ordered]
+    user_idx = kinds_in_order.index("user_turn")
+    close_idx = kinds_in_order.index("scene_closed")
+    assert user_idx < close_idx
+    if "assistant_turn" in kinds_in_order:
+        assert user_idx < kinds_in_order.index("assistant_turn") < close_idx
 
 
 def test_interjection_enqueues_significance_job(app_state_setup, tmp_path):