fix(storage): stop a cancelled begin() from poisoning the pool - #20
Merged
Conversation
`sqlx`'s `pool.begin()` is not cancellation-safe. If the future is dropped
after BEGIN has reached Postgres but before `begin()` returns, sqlx never
receives a `Transaction` to roll back and keeps no record that one is open.
The connection goes back to the pool looking clean, and every later query
handed that connection silently joins the orphaned transaction.
Nothing ever commits it. It accumulates locks across whatever tables the
reused connection touches, pins a transaction id against vacuum, and stalls
any TRUNCATE or DDL on those tables until the process exits. axum drops a
handler future exactly this way when a client disconnects mid-request, which
is ordinary browser behaviour and constant under Playwright.
Caught by turning on `log_statement=all` and reading one backend's history:
12 x statement: BEGIN
11 x statement: COMMIT
0 x statement: ROLLBACK
The statements after the unmatched BEGIN were unrelated work for a dozen
different documents — session touches, workspace_members role checks,
document lookups, `UpdatesStore::since` for many doc_ids — all inside one
transaction that had been open for minutes. `pg_locks` for that backend held
AccessShare on doc_updates, board_updates and comments plus RowExclusive on
sessions: four subsystems no single operation touches.
The fix is an `after_release` hook that issues ROLLBACK on the way back into
the pool. It has to be here rather than at a call site: the cancelled task
never receives a `Transaction`, so no caller can clean up after itself.
ROLLBACK is a no-op when no transaction is open, which is the common case.
Two supporting changes:
- `idle_in_transaction_session_timeout=30s` and `application_name`. The
timeout is a backstop, explicitly NOT the fix — the connection in this bug
is in constant use and never idles, so it would never have fired. The
application_name is what identified the connection as ours in the first
place.
- `e2e/support/reset.ts` retries for ~46s instead of ~25s. Its old window
expired just before the 30s backstop would have released a lock, which
made the backstop useless to the suite.
`tests/cancel_safety.rs` reproduces the leak against the real pool builder —
it fails in under a second without the `after_release` guard and survives
~3000 cancelled `begin()` calls with it. The other four tests in that file
are hypotheses this investigation ruled out: a cancelled fetch, a dropped
`Transaction`, a transaction dropped by cancellation, and a query cancelled
mid-execution. sqlx handles all four correctly. They are kept because a
future sqlx upgrade that regresses any of them would reintroduce this class
of leak silently.
Verified: fmt, clippy, 339 nextest tests, and the full Playwright suite with
no stranded transaction left behind — where the three runs before this change
each left one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the Postgres transaction leak flagged in #19. Independent of that PR —
different crates, no overlap — so the two can merge in either order.
Root cause
sqlx'spool.begin()is not cancellation-safe. If the future is droppedafter
BEGINhas reached Postgres but beforebegin()returns, sqlx neverreceives a
Transactionto roll back and keeps no record that one is open.The connection goes back to the pool looking clean, and every later query
handed that connection silently joins the orphaned transaction.
Nothing ever commits it. It accumulates locks across whatever tables the
reused connection happens to touch, pins a transaction id against vacuum, and
stalls any TRUNCATE or DDL on those tables until the process exits.
axum drops a handler future exactly this way when a client disconnects
mid-request — ordinary browser behaviour, and constant under Playwright.
The evidence
Turning on
log_statement=alland reading one backend's whole history:The statements after the unmatched
BEGINwere unrelated work for a dozendifferent documents — session touches,
workspace_membersrole checks,document lookups,
UpdatesStore::sincefor many doc_ids — all inside onetransaction that had been open for minutes.
pg_locksfor that backend:doc_updatesboard_updatescommentssessionsFour subsystems that no single operation touches.
The fix
An
after_releasehook that issuesROLLBACKon the way back into the pool.This has to live at the pool rather than at a call site: the cancelled task
never receives a
Transaction, so no caller can clean up after itself, andany future
begin()call site would reintroduce the bug.ROLLBACKis ano-op when no transaction is open, which is the overwhelmingly common case.
Supporting changes
idle_in_transaction_session_timeout=30sandapplication_name. Thetimeout is a backstop and explicitly not the fix — the connection in this
bug is in constant use and never idles, so it would never have fired. It is
kept for anything that still escapes.
application_nameis what identifiedthe connection as ours in the first place, and is why the next occurrence
will be obvious.
e2e/support/reset.tsnow retries for ~46s instead of ~25s. Its old windowexpired just before the 30s backstop would have released a lock, which
made that backstop useless to this suite.
Tests
tests/cancel_safety.rsreproduces the leak against the real pool builder.It fails in 0.31s without the
after_releaseguard and survives ~3000cancelled
begin()calls with it — verified both ways by temporarily removingthe guard.
The other four tests in that file are hypotheses this investigation ruled out:
a cancelled fetch, a dropped
Transaction, a transaction dropped bycancellation, and a query cancelled mid-execution. sqlx handles all four
correctly. They are kept because they are cheap and because a future sqlx
upgrade that regresses any of them would reintroduce this class of leak with
nothing else in the suite noticing.
tests/pool_hygiene.rspins the backstop: that the setting is applied, andthat Postgres really does reclaim a session left idle in a transaction.
Verification
cargo fmt,clippy -D warnings, 339 nextest tests, and the fullPlaywright suite:
The two remaining failures are the known environmental pair:
tree-reorder"nest" (macOS-only dnd-kit, passes in CI) and
ws-reconnect(toxiproxy'sadmin API is unreachable on this machine).
The
comment-anchors,two-users-convergeandupload-imageflakes thatrecurred across the earlier runs are gone, and the suite is ~1-3 minutes
faster — both consistent with removing the lock contention. That is one run,
so: strongly suggestive rather than proven.
🤖 Generated with Claude Code