fix(lists): get_entry cursor scope + reorder rollback on failure (tsk-u23vjy)

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).

tests/projects/test_lists_store.py

@@ -327,3 +327,142 @@ 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
+
+
+@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

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,16 +260,25 @@ 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
-        await self._db.commit()
+        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
+            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. 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
         return True
 
     async def _get_next_position(self, project_id: str, list_id: str) -> int: