From 051b23d09a50e9805f66af3a29009bcbf95233c1 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 12:04:30 +0200 Subject: [PATCH 1/3] fix(db): stop pysqlite lazy BEGIN so an outermost SAVEPOINT rolls back on SQLite (#350) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/session.py | 13 +++++ .../db/tests/test_db_sqlite_savepoint.py | 53 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 framework/db/tests/test_db_sqlite_savepoint.py diff --git a/framework/db/simple_module_db/session.py b/framework/db/simple_module_db/session.py index 6971732f..de764dd0 100644 --- a/framework/db/simple_module_db/session.py +++ b/framework/db/simple_module_db/session.py @@ -153,6 +153,12 @@ def _configure_sqlite( @event.listens_for(engine.sync_engine, "connect") def _set_sqlite_pragmas(dbapi_connection, _connection_record) -> None: # type: ignore[misc] + # Take transaction control away from pysqlite, which otherwise emits + # BEGIN lazily before the first DML only. An outermost SAVEPOINT would + # then start the transaction and its RELEASE would COMMIT it (GH #350). + # Done first: PRAGMAs below then run in autocommit, which WAL and + # ``foreign_keys`` both require. + dbapi_connection.isolation_level = None cursor = dbapi_connection.cursor() try: if foreign_keys: @@ -163,3 +169,10 @@ def _set_sqlite_pragmas(dbapi_connection, _connection_record) -> None: # type: cursor.execute("PRAGMA journal_mode=WAL") finally: cursor.close() + + @event.listens_for(engine.sync_engine, "begin") + def _explicit_begin(conn) -> None: # type: ignore[misc] + # Sessions sharing one DBAPI connection (a StaticPool in tests) would + # otherwise issue BEGIN inside the first one's open transaction. + if not getattr(conn.connection.driver_connection, "in_transaction", False): + conn.exec_driver_sql("BEGIN") diff --git a/framework/db/tests/test_db_sqlite_savepoint.py b/framework/db/tests/test_db_sqlite_savepoint.py new file mode 100644 index 00000000..0d5be29c --- /dev/null +++ b/framework/db/tests/test_db_sqlite_savepoint.py @@ -0,0 +1,53 @@ +"""GH #350: an outermost begin_nested() must not commit on RELEASE on SQLite.""" + +from __future__ import annotations + +import pytest +from simple_module_db.session import init_db +from sqlalchemy import text + + +@pytest.fixture(params=["memory", "file"]) +async def db_state(request, tmp_path): + url = ( + "sqlite+aiosqlite:///:memory:" + if request.param == "memory" + else f"sqlite+aiosqlite:///{tmp_path / 'sp.db'}" + ) + state = init_db(url) + async with state.engine.begin() as conn: + await conn.execute(text("create table t (id integer primary key)")) + try: + yield state + finally: + await state.engine.dispose() + + +async def _count(state, where: str = "1=1") -> int: + async with state.session_factory() as db: + return (await db.execute(text(f"select count(*) from t where {where}"))).scalar() + + +async def test_outermost_savepoint_is_rolled_back_with_the_outer_transaction(db_state): + async with db_state.session_factory() as db: + async with db.begin_nested(): # no DML before it + await db.execute(text("insert into t values (1)")) + await db.rollback() + assert await _count(db_state) == 0 + + +async def test_savepoint_after_write_still_rolls_back(db_state): + async with db_state.session_factory() as db: + await db.execute(text("update t set id = id")) + async with db.begin_nested(): + await db.execute(text("insert into t values (2)")) + await db.rollback() + assert await _count(db_state, "id = 2") == 0 + + +async def test_committed_work_with_savepoint_persists(db_state): + async with db_state.session_factory() as db: + async with db.begin_nested(): + await db.execute(text("insert into t values (3)")) + await db.commit() + assert await _count(db_state, "id = 3") == 1 From 9d5f0b8f289edf9d2e250b643781793beca4aadd Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 12:58:24 +0200 Subject: [PATCH 2/3] fix(db): begin explicitly only before an outermost SAVEPOINT on SQLite (#350) The blanket isolation_level=None + BEGIN recipe made every read a snapshot, so concurrent read-then-write transactions failed with SQLITE_BUSY_SNAPSHOT (tenants concurrency tests). Restrict the explicit BEGIN to the savepoint event, guarded by in_transaction. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/session.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/framework/db/simple_module_db/session.py b/framework/db/simple_module_db/session.py index de764dd0..f038d4ed 100644 --- a/framework/db/simple_module_db/session.py +++ b/framework/db/simple_module_db/session.py @@ -153,12 +153,6 @@ def _configure_sqlite( @event.listens_for(engine.sync_engine, "connect") def _set_sqlite_pragmas(dbapi_connection, _connection_record) -> None: # type: ignore[misc] - # Take transaction control away from pysqlite, which otherwise emits - # BEGIN lazily before the first DML only. An outermost SAVEPOINT would - # then start the transaction and its RELEASE would COMMIT it (GH #350). - # Done first: PRAGMAs below then run in autocommit, which WAL and - # ``foreign_keys`` both require. - dbapi_connection.isolation_level = None cursor = dbapi_connection.cursor() try: if foreign_keys: @@ -170,9 +164,14 @@ def _set_sqlite_pragmas(dbapi_connection, _connection_record) -> None: # type: finally: cursor.close() - @event.listens_for(engine.sync_engine, "begin") - def _explicit_begin(conn) -> None: # type: ignore[misc] - # Sessions sharing one DBAPI connection (a StaticPool in tests) would - # otherwise issue BEGIN inside the first one's open transaction. + @event.listens_for(engine.sync_engine, "savepoint") + def _begin_before_outermost_savepoint(conn, _name) -> None: # type: ignore[misc] + # pysqlite only emits BEGIN lazily before DML, so a SAVEPOINT that is + # the first statement would *start* the transaction and its RELEASE + # would COMMIT it, leaving the outer rollback nothing to undo (GH #350). + # Open the transaction ourselves, only in that case. SQLAlchemy's + # blanket recipe (isolation_level=None + BEGIN on every transaction) + # was rejected: it turns every read into a snapshot, so a later write + # fails with SQLITE_BUSY_SNAPSHOT, which ignores busy_timeout. if not getattr(conn.connection.driver_connection, "in_transaction", False): conn.exec_driver_sql("BEGIN") From 0d7314107a52a32e6f7327d894ecc50d006550cd Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 14:43:09 +0200 Subject: [PATCH 3/3] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/session.py | 3 +++ .../db/tests/test_db_sqlite_savepoint.py | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/framework/db/simple_module_db/session.py b/framework/db/simple_module_db/session.py index f038d4ed..3012a30c 100644 --- a/framework/db/simple_module_db/session.py +++ b/framework/db/simple_module_db/session.py @@ -148,6 +148,9 @@ def _configure_sqlite( facade, and ``connect`` fires on the DBAPI connection underneath it. All three PRAGMAs must be re-issued per connection except ``journal_mode``, which is a property of the file; re-issuing it is cheap and idempotent. + + Also registers a ``savepoint`` listener that opens the transaction before an + outermost SAVEPOINT (pysqlite would otherwise let its RELEASE commit; GH #350). """ apply_wal = wal and _is_file_database(database_url) diff --git a/framework/db/tests/test_db_sqlite_savepoint.py b/framework/db/tests/test_db_sqlite_savepoint.py index 0d5be29c..e4b5ffd3 100644 --- a/framework/db/tests/test_db_sqlite_savepoint.py +++ b/framework/db/tests/test_db_sqlite_savepoint.py @@ -51,3 +51,25 @@ async def test_committed_work_with_savepoint_persists(db_state): await db.execute(text("insert into t values (3)")) await db.commit() assert await _count(db_state, "id = 3") == 1 + + +async def test_failed_savepoint_rolls_back_only_itself(db_state): + from sqlalchemy.exc import IntegrityError + + async with db_state.session_factory() as db: + await db.execute(text("insert into t values (4)")) + with pytest.raises(IntegrityError): + async with db.begin_nested(): + await db.execute(text("insert into t values (5)")) + await db.execute(text("insert into t values (4)")) + await db.commit() + assert await _count(db_state, "id = 4") == 1 + assert await _count(db_state, "id = 5") == 0 + + +async def test_nested_savepoints_do_not_double_begin(db_state): + async with db_state.session_factory() as db: + async with db.begin_nested(), db.begin_nested(): + await db.execute(text("insert into t values (6)")) + await db.rollback() + assert await _count(db_state, "id = 6") == 0