From 2c5c8a380faa315363f50e4b2c54cd0ede9ed6c3 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Tue, 11 Aug 2026 20:12:58 +0000 Subject: [PATCH 1/2] fix(lists): cursor-scope in get_entry, rollback on failed reorder (tsk-u23vjy) Re-scoped fix-forward of closed duplicate PR #2183. Of the card's four defects, two are already resolved or impossible on dev: position-0 collision was fixed by #2265's atomic in-INSERT allocation (existing concurrency test guards it), and NULL list_id rows cannot exist (schema NOT NULL). The two real ones land here: - get_entry read cur.description outside the cursor context; moved inside. - reorder_entries left already-issued UPDATEs pending when one raised, so the next unrelated commit() flushed a half-applied reorder; now rolls back and re-raises, with a test proving the pending write neither survives immediately nor resurfaces via a later unrelated commit (proven red against the unguarded store). --- .../2361-lists-store-cursor-rollback.md | 6 ++ tests/projects/test_lists_store.py | 68 +++++++++++++++++++ tinyagentos/projects/lists_store.py | 30 +++++--- 3 files changed, 93 insertions(+), 11 deletions(-) create mode 100644 changelog.d/2361-lists-store-cursor-rollback.md diff --git a/changelog.d/2361-lists-store-cursor-rollback.md b/changelog.d/2361-lists-store-cursor-rollback.md new file mode 100644 index 000000000..080159d0d --- /dev/null +++ b/changelog.d/2361-lists-store-cursor-rollback.md @@ -0,0 +1,6 @@ +### Fixed + +- Project list entries: `get_entry` no longer reads cursor metadata after the + cursor closes, and a failed reorder now rolls back its partial updates so a + later unrelated write cannot commit a half-applied ordering (tsk-u23vjy, + fix-forward of #2183). diff --git a/tests/projects/test_lists_store.py b/tests/projects/test_lists_store.py index d42a56569..0c186063c 100644 --- a/tests/projects/test_lists_store.py +++ b/tests/projects/test_lists_store.py @@ -327,3 +327,71 @@ async def slow_next_position(project_id, list_id): positions = {e1["position"], e2["position"]} assert len(positions) == 2, f"expected distinct positions, got {positions}" + + +# --------------------------------------------------------------------------- +# tsk-u23vjy (fix-forward of the closed duplicate PR #2183, re-scoped to what +# is real on dev): of the card's four defects, position-0 collision is already +# fixed by the atomic INSERT (#2265, concurrency test above) and NULL list_id +# rows are impossible (schema: list_id TEXT NOT NULL). The two below remain. +# --------------------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_get_entry_reads_description_before_cursor_closes(entries_store): + e = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="Test entry", + original_text="Test entry", author_kind="agent", author_id="agent-1", + ) + result = await entries_store.get_entry(e["id"]) + assert result is not None + assert result["id"] == e["id"] + assert result["text"] == "Test entry" + + +@pytest.mark.asyncio +async def test_reorder_entries_rolls_back_on_exception(entries_store, monkeypatch): + """If an UPDATE raises partway, the earlier UPDATEs must be rolled back -- + otherwise they sit pending on the shared connection and the next unrelated + commit() flushes a half-applied reorder.""" + a = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="A", original_text="A", + author_kind="agent", author_id="agent-1", + ) + b = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="B", original_text="B", + author_kind="agent", author_id="agent-1", + ) + + real_execute = entries_store._db.execute + update_calls = 0 + + async def failing_execute(sql, params=()): + nonlocal update_calls + if sql.startswith("UPDATE project_list_entries SET position"): + update_calls += 1 + if update_calls == 2: + raise RuntimeError("boom") + return await real_execute(sql, params) + + monkeypatch.setattr(entries_store._db, "execute", failing_execute) + + with pytest.raises(RuntimeError): + await entries_store.reorder_entries( + "prj-1", "lst-1", + [{"id": a["id"], "position": 1}, {"id": b["id"], "position": 0}], + ) + monkeypatch.undo() + + # The half-applied first UPDATE must be gone immediately... + a_after = await entries_store.get_entry(a["id"]) + b_after = await entries_store.get_entry(b["id"]) + assert a_after["position"] == 0 + assert b_after["position"] == 1 + + # ...and must NOT resurface when an unrelated write commits later. + await entries_store.add_entry( + list_id="lst-2", project_id="prj-1", text="Unrelated", + original_text="Unrelated", author_kind="agent", author_id="agent-1", + ) + a_final = await entries_store.get_entry(a["id"]) + assert a_final["position"] == 0 diff --git a/tinyagentos/projects/lists_store.py b/tinyagentos/projects/lists_store.py index be3695531..ac2face06 100644 --- a/tinyagentos/projects/lists_store.py +++ b/tinyagentos/projects/lists_store.py @@ -174,8 +174,9 @@ async def get_entry(self, entry_id: str) -> dict | None: row = await cur.fetchone() if row is None: return None - keys = [d[0] for d in cur.description] - return dict(zip(keys, row)) + # cur.description is only guaranteed while the cursor is open. + keys = [d[0] for d in cur.description] + return dict(zip(keys, row)) async def list_entries( self, @@ -259,15 +260,22 @@ async def delete_entry(self, entry_id: str) -> bool: return cursor.rowcount == 1 async def reorder_entries(self, project_id: str, list_id: str, entries: list[dict]) -> bool: - for entry in entries: - cursor = await self._db.execute( - "UPDATE project_list_entries SET position = ?, updated_at = ? " - "WHERE id = ? AND project_id = ? AND list_id = ?", - (entry["position"], time.time(), entry["id"], project_id, list_id), - ) - if cursor.rowcount == 0: - await self._db.rollback() - return False + try: + for entry in entries: + cursor = await self._db.execute( + "UPDATE project_list_entries SET position = ?, updated_at = ? " + "WHERE id = ? AND project_id = ? AND list_id = ?", + (entry["position"], time.time(), entry["id"], project_id, list_id), + ) + if cursor.rowcount == 0: + await self._db.rollback() + return False + except Exception: + # Without this, the UPDATEs already issued stay pending on the + # shared connection and the next unrelated commit() flushes a + # half-applied reorder. Roll back, then re-raise. + await self._db.rollback() + raise await self._db.commit() return True From 2fee9379ad29c2bd2ba2ede5360c7d97f931dcb7 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Tue, 11 Aug 2026 20:31:48 +0000 Subject: [PATCH 2/2] fix(lists): widen reorder rollback guard to BaseException, cover commit() CodeRabbit's Major on #2361, folded: asyncio.CancelledError does not inherit Exception, so task cancellation mid-reorder left the issued UPDATEs pending exactly like the original hazard, and commit() sat outside the guard. The commit moves inside the try and the handler catches BaseException, rolling back before re-raising. Regression tests for both paths; the cancellation test proven red against the except-Exception guard. --- tests/projects/test_lists_store.py | 71 +++++++++++++++++++++++++++++ tinyagentos/projects/lists_store.py | 8 ++-- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/tests/projects/test_lists_store.py b/tests/projects/test_lists_store.py index 0c186063c..5252bdc0b 100644 --- a/tests/projects/test_lists_store.py +++ b/tests/projects/test_lists_store.py @@ -395,3 +395,74 @@ async def failing_execute(sql, params=()): ) a_final = await entries_store.get_entry(a["id"]) assert a_final["position"] == 0 + + +@pytest.mark.asyncio +async def test_reorder_entries_rolls_back_on_cancellation(entries_store, monkeypatch): + """CancelledError is not an Exception; the rollback guard must still fire.""" + a = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="A", original_text="A", + author_kind="agent", author_id="agent-1", + ) + b = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="B", original_text="B", + author_kind="agent", author_id="agent-1", + ) + + real_execute = entries_store._db.execute + update_calls = 0 + + async def cancelling_execute(sql, params=()): + nonlocal update_calls + if sql.startswith("UPDATE project_list_entries SET position"): + update_calls += 1 + if update_calls == 2: + raise asyncio.CancelledError() + return await real_execute(sql, params) + + monkeypatch.setattr(entries_store._db, "execute", cancelling_execute) + + with pytest.raises(asyncio.CancelledError): + await entries_store.reorder_entries( + "prj-1", "lst-1", + [{"id": a["id"], "position": 1}, {"id": b["id"], "position": 0}], + ) + monkeypatch.undo() + + await entries_store.add_entry( + list_id="lst-2", project_id="prj-1", text="Unrelated", + original_text="Unrelated", author_kind="agent", author_id="agent-1", + ) + assert (await entries_store.get_entry(a["id"]))["position"] == 0 + assert (await entries_store.get_entry(b["id"]))["position"] == 1 + + +@pytest.mark.asyncio +async def test_reorder_entries_rolls_back_on_commit_failure(entries_store, monkeypatch): + a = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="A", original_text="A", + author_kind="agent", author_id="agent-1", + ) + b = await entries_store.add_entry( + list_id="lst-1", project_id="prj-1", text="B", original_text="B", + author_kind="agent", author_id="agent-1", + ) + + async def failing_commit(): + raise RuntimeError("commit boom") + + monkeypatch.setattr(entries_store._db, "commit", failing_commit) + + with pytest.raises(RuntimeError): + await entries_store.reorder_entries( + "prj-1", "lst-1", + [{"id": a["id"], "position": 1}, {"id": b["id"], "position": 0}], + ) + monkeypatch.undo() + + await entries_store.add_entry( + list_id="lst-2", project_id="prj-1", text="Unrelated", + original_text="Unrelated", author_kind="agent", author_id="agent-1", + ) + assert (await entries_store.get_entry(a["id"]))["position"] == 0 + assert (await entries_store.get_entry(b["id"]))["position"] == 1 diff --git a/tinyagentos/projects/lists_store.py b/tinyagentos/projects/lists_store.py index ac2face06..81aa60f4a 100644 --- a/tinyagentos/projects/lists_store.py +++ b/tinyagentos/projects/lists_store.py @@ -270,13 +270,15 @@ async def reorder_entries(self, project_id: str, list_id: str, entries: list[dic if cursor.rowcount == 0: await self._db.rollback() return False - except Exception: + await self._db.commit() + except BaseException: # Without this, the UPDATEs already issued stay pending on the # shared connection and the next unrelated commit() flushes a - # half-applied reorder. Roll back, then re-raise. + # half-applied reorder. BaseException, not Exception: task + # cancellation (CancelledError) must also roll back, and commit() + # itself is inside the guard for the same reason. await self._db.rollback() raise - await self._db.commit() return True async def _get_next_position(self, project_id: str, list_id: str) -> int: