fix(storage): fix the begin() leak at the source, not on release - #21
Merged
Conversation
#20 fixed the orphaned-transaction leak with an `after_release` hook that issued an unconditional ROLLBACK. That works, and it is far too noisy to ship. Postgres answers a ROLLBACK with no open transaction with `WARNING: there is no transaction in progress`, and the common case is a clean connection, so nearly every release produced one. It was logged twice over: sqlx maps the notice to a tracing WARN on `sqlx::postgres::notice`, which knot's default `info` filter passes, and Postgres writes it server-side because log_min_messages defaults to `warning` — which an operator cannot turn off from inside knot. Measured on an idle dev server: knot log: 44053 of 44102 lines (99.9%) postgres log: 23000 of 23031 lines (99.9%, ~767/min) That buries every real error and fills disks, so it is a release blocker. Making the ROLLBACK conditional needs to know whether the connection is in a transaction, and there is no reliable way to ask. sqlx guards its own rollback with the transaction status from ReadyForQuery, but that accessor is pub(crate). The timestamp proxies do not work either: under the extended protocol the implicit transaction starts at Parse, so on a CLEAN connection `now()` and `statement_timestamp()` differ by ~60us and pg_stat_activity's `xact_start` and `query_start` by ~685us. Both were measured, and both would have made the check fire on every release — the same flood. So this fixes the defect where it happens instead. `sqlx::Pool::begin` is not cancellation-safe: drop the future after BEGIN reaches Postgres but before `begin()` returns, and sqlx never receives a `Transaction` to roll back and holds no record one is open. `knot_storage::begin` runs the BEGIN on a detached task, so the caller can be cancelled but the task still completes — the `Transaction` is always constructed and always dropped, and sqlx's own `Drop` issues the ROLLBACK. All nine `.begin()` call sites move to it. The `after_release` hook is gone. `idle_in_transaction_session_timeout` and `application_name` stay: the first is a backstop, the second is what made the original diagnosis possible. Tests split to match. `the_raw_pool_begin_is_not_cancellation_safe` pins the upstream behaviour and fails if sqlx ever fixes it, which is when the wrapper can be retired. `knot_begin_survives_cancellation` is the guard, checking after every cancellation because a later successful begin()/rollback() on the same pooled connection would roll the orphaned transaction back and hide it. The `after_release`-specific test in pool_hygiene.rs is deleted with the hook. Verified: fmt, clippy, 340 nextest tests, targeted e2e, and a 60s sample of a running server — 0 warnings in either log, down from ~767/min, with no stranded transaction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two timing bets in the cancellation tests, both of which passed on a Docker Desktop VM and failed on CI, where Postgres is a host service. a_cancelled_fetch_does_not_strand_its_connection used a fixed 3ms deadline against an 8MB fetch. On CI the whole fetch landed first, so nothing was ever cancelled — caught only because the test asserts that it WAS cancelled rather than silently proving nothing. The deadline is now swept from microseconds upward over a smaller payload, so some attempt lands mid-flight on any hardware, and the stranding check runs after every cancellation instead of once at the end. knot_begin_survives_cancellation slept a fixed 2ms before checking. The wrapper guarantees the transaction is always rolled back, not that it is rolled back by any particular instant: the detached task still has to finish its BEGIN and let Drop send the ROLLBACK. It now polls for the connection to come clean with a 5s ceiling, which is still decisive — the bug it guards against never resolves, at any deadline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
the_raw_pool_begin_is_not_cancellation_safe reproduces by landing a cancellation in the gap between BEGIN reaching Postgres and begin() returning, and the width of that gap is connection latency. Against Postgres in Docker Desktop it hits on the first cancellation. On CI, where Postgres is a host service over loopback, 2997 cancelled calls stranded nothing. That makes it useless as a gate: it cannot distinguish "sqlx fixed it upstream" from "the window was never reachable here", which is the only thing it exists to tell you. So it is #[ignore]d with instructions to run it deliberately on a setup with real latency. knot_begin_survives_cancellation stays as the CI gate, with a comment that is honest about the same sensitivity: it is green everywhere and decisive where the window exists, which is the setup that found the bug. 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
1 participant
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.
Release blocker for 0.5.0, and self-inflicted — this fixes a defect in #20.
#20 fixed the orphaned-transaction leak with an
after_releasehook issuing anunconditional
ROLLBACK. It works, and it is far too noisy to ship.Postgres answers a
ROLLBACKwith no open transaction withWARNING: there is no transaction in progress. The common case is a clean connection, so nearlyevery release produced one — logged twice over: sqlx maps the notice to a
tracing WARN on
sqlx::postgres::notice, which knot's defaultinfofilterpasses, and Postgres writes it server-side because
log_min_messagesdefaultsto
warning, which an operator cannot turn off from inside knot.Measured on an idle dev server:
That buries every real error and fills disks.
Why not just make the ROLLBACK conditional
Because there is no reliable way to ask whether a connection is in a
transaction. sqlx guards its own rollback with the transaction status from
ReadyForQuery, but that accessor ispub(crate).I tried the timestamp proxies and measured them rather than trusting the
reasoning. Both fail, in the direction that matters — on a clean connection:
now()vsstatement_timestamp()pg_stat_activity.xact_startvsquery_startUnder the extended protocol the implicit transaction starts at Parse, slightly
before the statement timestamp. Either check would have fired on every release
— the same flood, just with extra steps. I had written the first one and a
confident comment explaining why it was exact before the test disproved it.
What this does instead
Fixes the defect where it happens.
sqlx::Pool::beginis not cancellation-safe:drop the future after
BEGINreaches Postgres but beforebegin()returns andsqlx never receives a
Transactionto roll back, and holds no record one isopen.
knot_storage::beginruns theBEGINon a detached task. The caller can becancelled, but the task still runs to completion — so the
Transactionisalways constructed and always dropped, and sqlx's own
Dropissues theROLLBACK. All nine
.begin()call sites move to it; theafter_releasehookis deleted.
idle_in_transaction_session_timeoutandapplication_namestay. The first isa backstop; the second is what made the original diagnosis possible.
Tests
the_raw_pool_begin_is_not_cancellation_safe— characterises the upstreambehaviour and fails if sqlx ever fixes it, which is the signal that the
wrapper can be retired.
knot_begin_survives_cancellation— the guard. Checks after everycancellation, because a later successful
begin()/rollback()on the samepooled connection would roll the orphaned transaction back and hide the bug.
after_release-specific test inpool_hygiene.rsis deleted with thehook it pinned.
Verification
cargo fmt,clippy -D warnings, 340 nextest tests, targeted e2e (4/4),and a 60-second sample against a running server: 0 warnings in either log,
0 stranded transactions.
🤖 Generated with Claude Code