chore: scene-close-on-cancel — strengthen regression test + document rationale (T108)
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.
This commit is contained in:
@@ -873,6 +873,20 @@ async def post_turn(
|
|||||||
# mid-stream still meant to close the scene — the cancelled bot
|
# mid-stream still meant to close the scene — the cancelled bot
|
||||||
# beat doesn't invalidate that intent. Pinned by
|
# beat doesn't invalidate that intent. Pinned by
|
||||||
# test_cancelled_turn_still_closes_scene_when_user_prose_signals_close.
|
# 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():
|
if scene is not None and prose.strip():
|
||||||
container = None
|
container = None
|
||||||
if scene.get("container_id") is not None:
|
if scene.get("container_id") is not None:
|
||||||
|
|||||||
@@ -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
|
that as an exception, so we drive the request inside ``with
|
||||||
pytest.raises``. Despite the exception, the scene_closed event
|
pytest.raises``. Despite the exception, the scene_closed event
|
||||||
must land in the event_log.
|
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
|
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 "
|
"SELECT payload_json FROM event_log "
|
||||||
"WHERE kind = 'assistant_turn' ORDER BY id"
|
"WHERE kind = 'assistant_turn' ORDER BY id"
|
||||||
).fetchall()
|
).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.
|
# Scene close lands despite the cancel.
|
||||||
assert scene_close_count == 1
|
assert scene_close_count == 1
|
||||||
# The cancelled assistant_turn was still recorded (truncated=True).
|
# The cancelled assistant_turn was still recorded (truncated=True).
|
||||||
assert len(assistant_payload) == 1
|
assert len(assistant_payload) == 1
|
||||||
assert json.loads(assistant_payload[0][0])["truncated"] is True
|
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):
|
def test_interjection_enqueues_significance_job(app_state_setup, tmp_path):
|
||||||
|
|||||||
Reference in New Issue
Block a user