Fix async ORM connection ownership race - #214
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Covers the still-untested paths from the connection ownership rework: Transaction enter-failure cleanup, explicit commit/rollback, nested savepoint contexts, the transactions property, commit/rollback guard errors, reconnect release, PostgresConnection.insert_get_id, and the DatabaseTransaction test harness.
tmgbedu
left a comment
There was a problem hiding this comment.
Code Review verdict: ✅ APPROVE
(Posted as a comment because GitHub blocks formal self-approval — the reviewer agent shares this account. Treat this as the approval of record for task #1663.)
Reviewed the full diff (3 commits, head b78701f), traced all callers of the changed APIs, and ran the validation suite.
Verification performed
- Full suite:
uv run pytest --ignore=tests/masoniteorm/postgres --cov --cov-report=term-missing→ 2175 passed, 7 skipped, total coverage 84.33% (fail_under=80 ✅). Per-file:connection.py99% (only the twopassstubs at lines 104/107 uncovered),postgres_connection.py100%,testing/transaction.py61% — matches the reported numbers. - API stability: grepped all production callers — nothing assigns to
Connection.connectionor mutatesConnection.transactions, so converting them to read-only properties is safe.Transaction/Connection.transaction()are additive. No unnecessary changes to core abstractions.
Correctness of the fix
ContextVar-based ownership gives sibling asyncio tasks their ownAsyncConnectionwhile child tasks correctly inherit the enclosing transaction (guarded bytest_transaction_context_propagates_and_clean_context_is_isolated).Transaction.__aenter__failure path and__aexit__both release owned connections infinally— verified by the enter-failure and cancellation tests, which assertengine.pool.checkedout() == 0on apool_size=1, max_overflow=0engine. No leaks.- The
DatabaseTransactionharness rework is safe:IsolatedAsyncioTestCase(Python ≥3.12, perrequires-python) runsasyncSetUp/test/asyncTearDownin one sharedcontextvarscontext, so the token created inasyncStartTestRunis reset in the same context inasyncStopTestRun. Explicitrollback()followed by__aexit__(None, None, None)is the SQLAlchemy-supported pattern (inactive transaction close is a no-op). - Removing the duplicate commits in
select_oneandPostgresConnection.insert_get_idis correct: theengine.begin()scope in_executealready commits, and async results are pre-buffered, sofetchone()/mappings().all()after connection release are safe. commit_transaction/rollbackvia SQLAlchemy'sget_nested_transaction()/get_transaction()handle arbitrary nesting depth correctly (SQLAlchemy restores_previous_nestedon savepoint close).
New tests genuinely guard the fix
test_concurrent_tasks_own_their_connections_and_transactionsandtest_concurrent_paginate_callsfail pre-fix (a single sharedself.connectionyields identicalid()s / concurrent-op errors on oneAsyncConnection). Event-synchronized → deterministic.test_postgres_connection.pyis fully mocked — no live Postgres required.
sqlite nested-savepoint quirk (coverage engineer's note)
Does not block this PR — safe to defer to #1664. The old code issued the identical begin() → begin_nested() sequence on the same connection, so the pysqlite deferred-BEGIN behavior (savepoint before any DML commits on RELEASE) is pre-existing, sqlite-only, and not introduced or worsened here. The workaround comment in test_nested_commit_is_discarded_by_outer_rollback documents it well.
Non-blocking observations (fine to address in follow-ups)
connection.py:100—get_connection()outside a transaction now returns a new unmanaged connection per call that the caller must close (old version cached it andclose()reclaimed it). No production callers remain, but it's a public leak footgun; consider a docstring warning or making it private.connection.py:84-90— thetransactionsproperty reports at most 2 entries (root + current nested) even at deeper nesting, whereas the old stack reported true depth. Nothing branches on depth beyond emptiness (which is correct), but worth a docstring note.connection.py:63— per-instanceContextVarcreation is discouraged by the stdlib docs (contexts hold refs); acceptable here becauseDatabaseManagercachesConnectioninstances, but worth a comment.- Writes issued after an explicit
await transaction.commit()inside aTransactionblock will autobegin and be rolled back when the owned connection closes — consistent with SQLAlchemy's ownengine.begin()semantics, but a doc note would help API users.
Summary
Root cause
DatabaseManager caches framework Connection instances, but Connection stored one AsyncConnection and transaction stack globally. Concurrent request tasks therefore executed and committed the same SQLAlchemy transaction. select_one() and PostgreSQL insert_get_id() also issued a duplicate commit after run() had already committed.
Validation
uv run pytest tests/masoniteorm/sqlite/builder/test_sqlite_transaction.py tests/masoniteorm/sqlite/builder/test_sqlite_builder_pagination.py tests/masoniteorm/commands/test_migrate_commands.py -q(16 passed)uv run pytest tests/masoniteorm --ignore=tests/masoniteorm/postgres -q(728 passed, 7 skipped)uv run ruff check ...(passed)