feat(unified): pin the backend at the first statement, not at atomic() entry

Summary

A competing take on !25 (closed)'s pinning strategy. Everything about unified sessions stays as it is; the one thing that changes is when a transaction takes a connection out of the pool.

!25 (closed) pins at atomic() entry. That was load-bearing for two separate jobs, and only one of them actually needs a connection:

  1. Ownership — the block that opens the transaction is the block that ends it.
  2. The fork/join oraclesession.tx is not None was how a child task learned it was running inside a transaction.

Job 2 is a question, not a resource. Entry now stamps the session's transaction epoch into a contextvar and pins nothing; the first statement that needs a backend pins one, where Django on psycopg issues its BEGIN. Job 1 still holds because __exit__ is what commits, and a block that never queried has nothing to commit.

Why bother

Eager pinning taxes the interval between atomic() entry and the block's first SQL statement. That is zero for a block that queries immediately — and the whole of a template render under ATOMIC_REQUESTS, or a payment API call made inside the block. By Little's Law that interval is connections.

benchmarks/synthetic/pin_hold.py (added here) measures both arms from one checkout: BENCH_EAGER=1 force-pins at entry, reproducing what !25 (closed) does. 120 concurrent tasks against a pool of 8, medians of 21 runs:

non-DB work in the block async eager / lazy sync eager / lazy control eager / lazy
10 ms 0.316 / 0.124 0.231 / 0.114 0.132 / 0.128
50 ms 0.931 / 0.159 0.889 / 0.155 0.134 / 0.124
200 ms 3.307 / 0.327 3.275 / 0.321 0.134 / 0.126

Read the ratio as a shape, not a headline. The eager floor is ceil(tasks / pool) × hold and the lazy floor is hold, so the ratio is tasks / pool in the limit by construction and grows without bound as you turn either knob. What the benchmark shows is that hold time moves from entry→exit to first-statement→exit; how much that is worth is a property of the workload, not of the driver.

The control is the column that matters. A block that queries immediately ties at every hold time — there is no hold to remove, so there is no workload where this change costs throughput.

Three secondary effects go away with it: idle in transaction and the vacuum horizon stop running for blocks that have not touched the database yet, and a pool timeout raises from the first statement again rather than from atomic()'s __enter__, where a try/except around the block body cannot see it.

What changed

  • mark_transaction_open() is idempotent per transaction and fires from the autocommit setters as well as the tx setter. The second caller must not draw a fresh epoch — that invalidates the marks children forked after entry are already carrying and drops every one of them out of the transaction they are running in. clear_transaction_mark() resets the epoch so a block that never queried is still unmarked.
  • may_use() tests wants_transaction() rather than tx is not None. A child forked into a block that has not queried yet is inside it.
  • The async statement path gates on the resolved shared session (_wants_tx) instead of on its own connection's autocommit flag, which is per-connection and blind to a transaction the sync world opened.
  • The first BEGIN is serialised on SharedSession.begin_lock rather than on either connection's own lock — see "Found in review" below.
  • Entering a transaction releases this Connection's autocommit session pin.

The third piece is not optional. Without it, removing the eager pin reopens two holes immediately: a sync atomic() that has not queried stops capturing the first async write, and a child's async_atomic() emits SAVEPOINT with no transaction open (25P01). Both surfaced as test failures on the first run.

Found in review — two critical bugs, both fixed here

Fresh-context adversarial review after the branch was first pushed. Neither bug was caught by the 414 tests that were green at the time.

Two units of work could both open the transaction — silent data loss

tx lives on the SharedSession, but the lock guarding the check-open-assign that fills it lived on the connection. Units that fork before anything materialises async_connections each build their own wrapper, and so their own lock. Both read tx is None, both BEGIN, and the second assignment overwrites the first — taking everything already dispatched on the loser with it.

with transaction.atomic():
    async_to_sync(lambda: asyncio.gather(a(), b()))()   # committed 1 row of 2

Confirmed by A/B: feat/unified-ownership keeps both rows, this branch kept one. The eager pin filled tx before any child could run, so this branch is what exposed it — a lost write, not a leak.

Fixed by moving the lock beside the thing it guards. SharedSession.begin_lock is a threading.Lock, because the contenders span the sync world, the loop thread and sync_to_async workers. Async callers take it inside the executor the blocking BEGIN already runs in, then stamp the transaction mark back on the calling context — the worker thread has none of its contextvars, and a joiner that lost the race never assigns at all.

One Django connection held two pool slots

_begin_tx gives back the autocommit session pin when that Connection opens the transaction. When the async wrapper opened it instead, the Connection sat on a second slot for the rest of the block; a middleware query followed by an async view is enough. On a tight pool that is a hang, not a capacity regression — and it made this MR's headline claim false on the entire sync path, including the ATOMIC_REQUESTS example. Now:

after autocommit SELECT      : pin held = True
inside atomic(), no statement: pin held = False, tx = False
after first statement        : pin held = False, tx = True
after the block              : pin held = False, tx = False

AUTOCOMMIT=False never stamped the mark

connect() runs detached, so the flag was already False by the time the wrapper set it again and the setter short-circuited. The session then claimed a transaction with epoch 0, and every task forked before the first statement forked its own session out of a connection permanently in one.

Also fixed: the escape hatch in the docs has never worked

Pre-existing on both MRs. The docs told users to reach for async_connections._independent_connection() when a child genuinely wants its own transaction. It swaps a fresh wrapper in under the same alias, this registry is keyed by alias, so the child finds the same session and joins the transaction it was trying to escape — its writes then roll back with the parent, silently. Its replacement async_new_connection(), released in 6.1.2, behaves the same way — and since that is now also what upstream recommends for parallel queries, the note matters more than it did. A second DATABASES alias does work; the docs, the savepoint error message and a test now say so.

django-async-backend 6.1.2 — released, and this branch is specified against it

What was PR #78 shipped as 6.1.2 while this MR was open. It stamps every async connection with the task that created it and checks the stamp on every statement (_prepare_cursor, commit, rollback, savepoint), removes _independent_connection(), and adds async_new_connection() as the sanctioned way to get a connection of your own. The async extra floor moves to 6.1.2 here, because that is the ownership contract these tests specify.

The branch assumes that floor rather than negotiating with it: no fallbacks for older releases, .gitlab-ci.yml states the same floor as pyproject.toml so a resolver cannot quietly test an older one, and the tests and docs describe the ownership contract in the present tense rather than as a release delta. Version history stays in the changelog.

Re-run against the real release rather than the shadowed PR: 411 passed (py3.14, live PG 17). The three failures the shadow predicted were the three tests that asserted the old contract; they are re-aimed, not deleted:

test old contract now
..._forked_before_the_first_statement_still_joins a child forked in the lazy window joins the transaction renamed ..._forked_inside_the_block_is_refused_cleanly — upstream refuses it; what is pinned is that the refusal cannot leave the block able to commit part of its work
test_gather_inside_a_transaction_joins_it upstream documented the shape as supported split into ..._bare_gather_..._is_refused and test_async_new_connection_children_join_the_parents_transaction
test_upstreams_nested_task_guard_still_fires matched the old error wording matches the new one; the guard fires the same

The joining direction is still live, so SharedSession.begin_lock is not dead code. The guard compares the wrapper's owner, so it does not fire when the child built its own wrapper — which is exactly what happens when the transaction was opened from the sync world and nothing had materialised async_connections at fork time. test_two_children_racing_the_first_statement_both_land_in_the_tx still passes on 6.1.2, and still fails without the session-scoped lock. That was the critical bug this MR exposed, and 6.1.2 does not retire it.

Two behaviours 6.1.2 introduces, both measured here and newly pinned:

  • async_new_connection() does not part the session. It satisfies the ownership stamp — the wrapper is created inside the child — but the session registry is keyed by alias, so upstream's sanctioned parallel fan-out lands every child on the caller's session, inside the caller's transaction, taking turns on its one backend. Measured: three children, one transaction, all three rows visible to the parent and all three rolled back with it. Correct, and safer than one unowned transaction per child, but it is not the parallelism upstream's docs promise; a second DATABASES alias is what buys that. Documented in docs/unified-sessions.md.
  • A straggler that outlives the block is refused rather than committed. The gather-doesn't-cancel-siblings hole (#77) closes for any child that inherited the wrapper: its post-block write used to land on the autocommit pin and survive the rollback, and now raises. It stays open for a child that built its own wrapper, so that caveat is narrowed rather than deleted.

One thing to know either way: catching the RuntimeError yourself does not save the block. The refused write went through Django's mark_for_rollback_on_error, so a single stray gather inside a transaction costs the whole block. That is the safe outcome, and it is now asserted.

It also settles the argument with !25 (closed): eager pinning's one remaining structural advantage was closing the first-BEGIN race in gather(sql, sql), and that shape is now refused upstream — while the race that does survive is the sync-world one, which eager pinning never closed either.

Testing

  • django-vpg: see the Testing section below for current counts.
  • Three new regression tests, each confirmed to fail without its fix. The sync/async race needs 100 repetitions to be reliable — at 25 it only caught the bug two runs in three, which is worse than no test.
  • glitchtip-backend, full suite against this branch: 1534 tests, no hang, FAILED (failures=4, errors=38, skipped=2). The 38 errors are the Rust ingest tests failing on Valkey DNS; all four failures reproduce identically when the same run is pointed at feat/unified-ownership, so nothing here fails that eager pinning does not also fail. This is the run that matters for the new cross-world lock — the historical failure mode for this feature is a suite that hangs rather than one that fails.
  • Three of !25 (closed)'s own tests asserted eager pinning and were rewritten, not deleted: pin-at-entry became pin-at-first-statement, TestEagerPinFailure became TestFailedPin (the failure moved to the statement path), and the kill-switch test lost its premise so it now discriminates on the shared session itself.

Risks

This re-enables the predicate that 14075814 removed, and that commit's message records it deadlocking glitchtip's suite: a bare async INSERT ... ON CONFLICT under a TestCase's outer atomic opened a transaction nobody owned, and the next TransactionTestCase's TRUNCATE blocked on it forever. The reason it is safe now is that 14075814 predates the ownership rework — a task forked outside the block resolves to a fresh SharedSession whose defaults make wants_transaction False, so it opens nothing, while a joining task resolves to the session the block owns. Both reviewers probed this independently and agreed the predicate itself is sound; the glitchtip run is the empirical half.

A lock now spans the sync and async worlds. SharedSession.begin_lock is held across a blocking driver.begin(). Async callers take it inside an executor so the event loop never blocks on it, but a sync caller waiting for a pool slot does hold it, and an async caller then occupies an executor thread waiting. That is bounded by pool contention rather than unbounded, and the glitchtip run exercises it, but it is the part of this change most worth a second pair of eyes.

Round two: no global switch, and a second review pass

VPG_UNIFIED_SESSIONS is gone (breaking). Unified sessions are how the driver works. The switch offered a mode that is a data-integrity bug — two connections per alias, where a TestCase's rollback does not cover the async side — and calling that "restores the previous behaviour" invited people to trade a hang for silent divergence. OPTIONS={"pool": False} remains the per-alias opt-out, at the cost of pooling for that alias.

Deleting it removed ~40 lines and no downstream branch: session is None is still reached through pool: False, a bare django_vpg.dbapi user, and unified.detached(). The control test that demonstrated the feature is load-bearing did not need the flag either — it now runs against a pool: False alias as TestPoolFalseRestoresTheOldBreakage. Only the flag-parsing unit tests are actually gone.

A second adversarial pass then found three more code bugs, all fixed:

  • A cancelled task stranded a live BEGIN. await run_in_executor(...) is a cancellation point and the worker thread is not cancellable, so cancelling mid-BEGIN let the block unwind first, roll back nothing, and the worker then publish its transaction to a session nobody owned. asyncio.timeout, wait_for and ASGI client-disconnect all reach it. Shielded and drained now, so __aexit__ finds it.
  • BEGIN was running on the loop's default executor, where a slow one also stalls loop.getaddrinfo, asyncio.to_thread and every other run_in_executor(None, ...). It has its own pool now.
  • The race loser kept its autocommit pin — and with a session-wide lock, losing is the normal outcome for every contender after the first.

Plus: AUTOCOMMIT=False had no test (reverting its fix left the suite green — now covered), and the sync/async race test was a 50% coin flip when the module ran in order. My "100 reps, 5/5" measurement for it was taken with -k, in isolation, which is not how CI runs it. Replaced with a deterministic assertion of the invariant.

Four documentation claims were wrong

All measured, all corrected in 36f2278:

  • "Concurrent threads are detected, not silently allowed" — false in the worst case. Two concurrent thread_sensitive=False workers that each open their own outermost atomic() merge into one transaction; the second's block exits successfully and its rows are then discarded by the first's rollback. Nothing raises. 5/5 deterministic, and the same commit sticks on an opted-out alias, so this feature causes it. It is documented as a limitation now and pinned by a test.
  • "A pool timeout or a connection error still raises from the first statement" — connection errors still raise from __enter__, on this branch and the parent alike, because set_autocommit() calls ensure_connection().
  • "Each world keeps its own autocommit pin" — only the sync world has one. The async side takes a pooled backend per statement, so TEMP tables and GUCs do not survive between two consecutive async statements.
  • The detection section described a mechanism the code explicitly rejects and quoted an error string that does not exist in the tree.

The benchmark was independently re-verified as faithful, including the decisive control: with BENCH_POOL above BENCH_TASKS the two arms tie, and with a tight pool eager is ~6x slower. So the gap is pool-slot hold time and nothing else. The docstring no longer claims the ratio is TASKS / POOL "by construction" — measured is about half that.

Known and not fixed

One Django connection can still hold two pool slots when the async side opens the transaction. The fix went in for the sync direction only: atomic() entry and _begin_tx hand the pin back, but nothing releases a sync Connection's pin when async_atomic() is what opens the transaction. Middleware query then an async write needs two slots, and hangs on a pool of one.

The fix needs the session to know its pin holders and release them from whichever thread opens the transaction. That is safe in every supported configuration — the holder is always quiescent, because concurrent units are exactly the unsupported case above — but it is a cross-thread release of a pooled resource next to a lock this MR has already had to fix twice, so it wants its own review rather than being tacked on at the end of this one. Filed rather than rushed; the MR is Draft anyway.

Testing

411 tests pass (py3.14, live PG 17, django-async-backend 6.1.2); scripts/check.sh clean. Every regression test added in this MR is confirmed to fail without its fix.

glitchtip-backend is not exposed to the 6.1.2 change either way — it pins django-async-backend>=6.0,<6.1, so the ownership guard arrives there with the Django 6.1 upgrade, not with this.

Relationship to !25 (closed)

This branch is feat/unified-ownership plus the commits listed in the compare view. It is targeted at main so the two are genuinely alternative merge candidates.

Note that origin/feat/unified-ownership is currently one commit behind local (98fa893), so the comparison view will be slightly off until that is pushed.

AI disclosure: Claude Opus 5 wrote the implementation, the tests, the benchmark and this description, ran the measurements, the adversarial reviews, and the 6.1.2 compatibility work.

Edited by David Burke

Merge request reports

Loading
Loading