Skip to content

Fix async ORM connection ownership race - #214

Merged
tmgbedu merged 4 commits into
mainfrom
worktree-agent-1364
Sep 6, 2026
Merged

Fix async ORM connection ownership race#214
tmgbedu merged 4 commits into
mainfrom
worktree-agent-1364

Conversation

@tmgbedu

@tmgbedu tmgbedu commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • isolate SQLAlchemy connection and transaction state by asyncio task
  • use scoped auto-commit connections for ordinary reads and writes and remove duplicate commits
  • keep SQLite multi-statement schema alterations atomic on one explicit transaction
  • add concurrent pagination, concurrent transaction ownership, and nested transaction regression coverage

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)
  • PostgreSQL port was reachable, but the host PostgreSQL service did not expose the repository test role/database, so PostgreSQL model integration tests could not authenticate.

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

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 tmgbedu left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-missing2175 passed, 7 skipped, total coverage 84.33% (fail_under=80 ✅). Per-file: connection.py 99% (only the two pass stubs at lines 104/107 uncovered), postgres_connection.py 100%, testing/transaction.py 61% — matches the reported numbers.
  • API stability: grepped all production callers — nothing assigns to Connection.connection or mutates Connection.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 own AsyncConnection while child tasks correctly inherit the enclosing transaction (guarded by test_transaction_context_propagates_and_clean_context_is_isolated).
  • Transaction.__aenter__ failure path and __aexit__ both release owned connections in finally — verified by the enter-failure and cancellation tests, which assert engine.pool.checkedout() == 0 on a pool_size=1, max_overflow=0 engine. No leaks.
  • The DatabaseTransaction harness rework is safe: IsolatedAsyncioTestCase (Python ≥3.12, per requires-python) runs asyncSetUp/test/asyncTearDown in one shared contextvars context, so the token created in asyncStartTestRun is reset in the same context in asyncStopTestRun. Explicit rollback() followed by __aexit__(None, None, None) is the SQLAlchemy-supported pattern (inactive transaction close is a no-op).
  • Removing the duplicate commits in select_one and PostgresConnection.insert_get_id is correct: the engine.begin() scope in _execute already commits, and async results are pre-buffered, so fetchone()/mappings().all() after connection release are safe.
  • commit_transaction/rollback via SQLAlchemy's get_nested_transaction()/get_transaction() handle arbitrary nesting depth correctly (SQLAlchemy restores _previous_nested on savepoint close).

New tests genuinely guard the fix

  • test_concurrent_tasks_own_their_connections_and_transactions and test_concurrent_paginate_calls fail pre-fix (a single shared self.connection yields identical id()s / concurrent-op errors on one AsyncConnection). Event-synchronized → deterministic.
  • test_postgres_connection.py is 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)

  1. connection.py:100get_connection() outside a transaction now returns a new unmanaged connection per call that the caller must close (old version cached it and close() reclaimed it). No production callers remain, but it's a public leak footgun; consider a docstring warning or making it private.
  2. connection.py:84-90 — the transactions property 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.
  3. connection.py:63 — per-instance ContextVar creation is discouraged by the stdlib docs (contexts hold refs); acceptable here because DatabaseManager caches Connection instances, but worth a comment.
  4. Writes issued after an explicit await transaction.commit() inside a Transaction block will autobegin and be rolled back when the owned connection closes — consistent with SQLAlchemy's own engine.begin() semantics, but a doc note would help API users.

@tmgbedu
tmgbedu merged commit c4ea303 into main Sep 6, 2026
6 checks passed
@tmgbedu
tmgbedu deleted the worktree-agent-1364 branch September 6, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant