From abb2586f8ca67414b5c6e796d2c468755fe26ee3 Mon Sep 17 00:00:00 2001 From: Luke Tornquist Date: Fri, 6 Nov 2026 13:07:04 -0500 Subject: [PATCH] Pin sqlx to the upstream fix for a cancelled BEGIN (#9192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem sqlx 0.9.0 counts a transaction only after Postgres acknowledges its `BEGIN`. If the future is dropped while `BEGIN` is still in flight (a cancelled REQ history read, a client that disconnects, a timeout), sqlx doesn't queue a `ROLLBACK`. The pool's release ping then reads the late `BEGIN` reply, and the writer pool hands the connection to the next borrower while it is still inside that transaction. The reader pool already closes such connections in its release check (`Db::read_pool_options`). The writer pool has no release check. Load tests showed what happens next: - `there is already a transaction in progress` (25001) on the next event read's `BEGIN`. - Sometimes a burst of `current transaction is aborted` (25P02). The leftover from the event read is `READ ONLY`, so the next write fails and aborts it, and every later statement on that connection fails until the connection is closed. During a burst, ban checks failed closed with 503 and window reads returned 500. - A read-write leftover would also leave autocommit writes uncommitted, and the next read's `ROLLBACK` would discard them. ## Change - Pin every sqlx crate to transact-rs/sqlx#4394 (merged, not yet released). That commit takes the transaction count before the `BEGIN` round trip, so a cancelled begin queues its `ROLLBACK`. The crates share types, so they have to move together. The pin also picks up the other 23 commits between v0.9.0 and that fix, including the libsqlite3-sys fix `sqlx-sqlite` was already pinned for. - Add a regression test, `writer_pool_postgres_tests::begin_cancelled_in_flight_leaves_no_writer_transaction_open`. It holds `BEGIN` in flight behind a paused proxy and times it out, for both the event read's `BEGIN ... READ ONLY` and a plain `begin()`. It then asserts that the next borrower's autocommit write commits and that its backend is `idle`, not inside a transaction. Other upstream changes in the pin that reach the Postgres path: `TCP_NODELAY` on sockets (transact-rs/sqlx#4336), reuse of the TLS connector across connection attempts (transact-rs/sqlx#4242), a fix for a memory leak when the statement cache is disabled (transact-rs/sqlx#4337), a describe-time generic-plan change used only by the macros (transact-rs/sqlx#4286), and the `whoami` default-user fix (transact-rs/sqlx#4303). The rest are SQLite, MySQL, CLI, or docs changes. Remove the pin once sqlx 0.9.1 ships. ## What got simpler The lockfile loses its second `sqlx-core` copy, which the sqlx-sqlite-only pin used to pull in. No runtime code changes. A pool-level release check would have fixed the same bug, but it costs a round trip on every writer release. Comments that described the 0.9.0 behavior as current are refreshed: the reader pool's release check (`Db::read_pool_options`) and the lease heartbeat (`EntityDeletionStore::heartbeat`). The reader test `proof_timeout_mid_begin_leaves_no_reader_transaction_open` asserted that the release check closed the connection left mid-`BEGIN`. With the pin, sqlx rolls that transaction back itself, so the test now asserts the check finds nothing to close. That also catches a sqlx regression the check would otherwise hide. Identified debt (not in this PR): the release check's open-transaction arm, and the reader-session `default_transaction_isolation` pin it depends on, are now only a backstop against a sqlx regression. They could be removed in a follow-up. ## Testing - Against sqlx 0.9.0 (main's `Cargo.toml`/`Cargo.lock` at 962b3fdd), the new writer test fails for each `BEGIN` form on its own. The event read fails with `cannot execute INSERT in a read-only transaction`, and after a plain `begin()` the next borrower's backend is not `idle`. With the pin it passes 5/5. - Single-machine load-test A/B against main (962b3fdd), with three main runs and two pin runs. Main logged 25001 and 25P02 in every run (41/80, 10/9, 12/18); the pin logged none. Client error rate, latency, and throughput matched within run-to-run noise. - Full suite on the pin: every `just ci` recipe, the PostgreSQL lane, and `just test`. The only failure from the pin was the reader test above. Reds that don't involve sqlx: the desktop Tauri recipes (`alsa-sys` needs ALSA headers on the build host), `mobile-check`/`mobile-test` (a Flutter SDK `ColorScheme` type clash), and two `buzz-acp` `git_bootstrap` tests (no git credentials on the host). Two bridge search-timeout tests failed only under the full parallel lane; run alone they pass on both the pin and 0.9.0. - Two later commits change only tests, comments, and `Cargo.lock` (the lock now moves only the sqlx crates). At those heads: the PostgreSQL lane, the unit lane, and workspace clippy pass. Three tests failed once under a contended full lane, and each passed 3/3 alone on both the pin and 0.9.0. The two transaction tests fail on 0.9.0 and pass on the pin. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Luke Tornquist Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 --- Cargo.lock | 69 +++----------- Cargo.toml | 22 ++++- crates/buzz-db/src/runtime/mod.rs | 8 +- crates/buzz-db/src/runtime/tests.rs | 3 + .../tests/reader_pool_postgres_tests.rs | 19 ++-- .../tests/writer_pool_postgres_tests.rs | 94 +++++++++++++++++++ .../src/store/entity_deletion/leases.rs | 5 +- 7 files changed, 145 insertions(+), 75 deletions(-) create mode 100644 crates/buzz-db/src/runtime/tests/writer_pool_postgres_tests.rs diff --git a/Cargo.lock b/Cargo.lock index d19342367f..1b9802dd82 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9915,10 +9915,9 @@ dependencies = [ [[package]] name = "sqlx" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "378620ccc25c62c89d8be1c819e76a88d59bdcc3304733330788948e619bfd71" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ - "sqlx-core 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "sqlx-core", "sqlx-macros", "sqlx-mysql", "sqlx-postgres", @@ -9928,8 +9927,7 @@ dependencies = [ [[package]] name = "sqlx-core" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "05b44e85bf579a8eeb4ceaa77a3a523baf2bf0e9bac7e40f405d537b5d2d5ccb" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ "base64 0.22.1", "bytes", @@ -9952,7 +9950,7 @@ dependencies = [ "rustls", "serde", "serde_json", - "sha2 0.10.9", + "sha2 0.11.0", "smallvec", "thiserror 2.0.18", "tokio", @@ -9963,48 +9961,14 @@ dependencies = [ "webpki-roots 1.0.10", ] -[[package]] -name = "sqlx-core" -version = "0.9.0" -source = "git+https://github.com/launchbadge/sqlx?rev=94aafe3a68884d923b0798a767c8d7f6cfda89d2#94aafe3a68884d923b0798a767c8d7f6cfda89d2" -dependencies = [ - "base64 0.22.1", - "bytes", - "cfg-if 1.0.4", - "chrono", - "crc", - "crossbeam-queue", - "either", - "event-listener", - "futures-core", - "futures-intrusive", - "futures-io", - "futures-util", - "hashbrown 0.16.1", - "hashlink", - "indexmap 2.14.0", - "log", - "memchr", - "percent-encoding", - "serde", - "serde_json", - "sha2 0.11.0", - "smallvec", - "thiserror 2.0.18", - "tracing", - "url", - "uuid", -] - [[package]] name = "sqlx-macros" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bd2b84f2bc39a5705ef27ec785a11c934a41bbd4a24941e257927cddc26b60bf" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ "proc-macro2", "quote", - "sqlx-core 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "sqlx-core", "sqlx-macros-core", "syn 2.0.117", ] @@ -10012,8 +9976,7 @@ dependencies = [ [[package]] name = "sqlx-macros-core" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fb8d96de5fdc85a5c4ec813432b523ec637e80ba98f046555f75f7908ddac7c3" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ "cfg-if 1.0.4", "dotenvy", @@ -10024,8 +9987,8 @@ dependencies = [ "quote", "serde", "serde_json", - "sha2 0.10.9", - "sqlx-core 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "sha2 0.11.0", + "sqlx-core", "sqlx-mysql", "sqlx-postgres", "sqlx-sqlite", @@ -10038,8 +10001,7 @@ dependencies = [ [[package]] name = "sqlx-mysql" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "90b8020fe17c5f2c245bfa2505d7ef59c5604839527c740266ad2214acebea27" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ "bitflags 2.13.0", "byteorder", @@ -10057,7 +10019,7 @@ dependencies = [ "serde", "sha1 0.11.0", "sha2 0.11.0", - "sqlx-core 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "sqlx-core", "thiserror 2.0.18", "tracing", "uuid", @@ -10066,8 +10028,7 @@ dependencies = [ [[package]] name = "sqlx-postgres" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "87a2bdd6e83f6b3ea525ca9fee568030508b58355a43d0b2c1674d5f79dcd65e" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ "atoi", "base64 0.22.1", @@ -10092,7 +10053,7 @@ dependencies = [ "serde_json", "sha2 0.11.0", "smallvec", - "sqlx-core 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "sqlx-core", "stringprep", "thiserror 2.0.18", "tracing", @@ -10103,7 +10064,7 @@ dependencies = [ [[package]] name = "sqlx-sqlite" version = "0.9.0" -source = "git+https://github.com/launchbadge/sqlx?rev=94aafe3a68884d923b0798a767c8d7f6cfda89d2#94aafe3a68884d923b0798a767c8d7f6cfda89d2" +source = "git+https://github.com/launchbadge/sqlx?rev=f1e94ec34dd5f11ad6488a3a8ac7988439a3f757#f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" dependencies = [ "atoi", "chrono", @@ -10118,7 +10079,7 @@ dependencies = [ "log", "percent-encoding", "serde", - "sqlx-core 0.9.0 (git+https://github.com/launchbadge/sqlx?rev=94aafe3a68884d923b0798a767c8d7f6cfda89d2)", + "sqlx-core", "thiserror 2.0.18", "tracing", "url", diff --git a/Cargo.toml b/Cargo.toml index e0925d9489..235efe04dc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -193,8 +193,20 @@ strip = true # allowlist for the auth token). Revert to crates.io once #449 lands upstream. [patch.crates-io] aws-creds = { git = "https://github.com/tlongwell-block/rust-s3", rev = "c9fce3620dd434c1f810101d672cf384268dbb0f" } -# MeshLLM 0.76 uses rusqlite 0.40 / libsqlite3-sys 0.38. SQLx 0.9.0's -# optional SQLite driver still caps libsqlite3-sys below 0.38, which Cargo -# resolves even though Buzz only enables PostgreSQL. Pin the merged upstream -# compatibility fix until it is released: https://github.com/transact-rs/sqlx/pull/4402 -sqlx-sqlite = { git = "https://github.com/launchbadge/sqlx", rev = "94aafe3a68884d923b0798a767c8d7f6cfda89d2" } +# SQLx: pin every crate to one unreleased upstream commit until 0.9.1 ships. +# The crates share types, so they must move together. It carries two fixes: +# - https://github.com/transact-rs/sqlx/pull/4394: a `BEGIN` whose future is +# dropped before Postgres answers no longer returns its pooled connection +# still inside the transaction. 0.9.0 queues no `ROLLBACK` for it, so the +# next borrower runs inside the leftover transaction (`25001`, then `25P02` +# once a statement in it fails). +# - https://github.com/transact-rs/sqlx/pull/4402: MeshLLM 0.76 uses +# libsqlite3-sys 0.38, which 0.9.0's optional SQLite driver rejects; Cargo +# resolves it even though Buzz only enables PostgreSQL. +sqlx = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } +sqlx-core = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } +sqlx-macros = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } +sqlx-macros-core = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } +sqlx-mysql = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } +sqlx-postgres = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } +sqlx-sqlite = { git = "https://github.com/launchbadge/sqlx", rev = "f1e94ec34dd5f11ad6488a3a8ac7988439a3f757" } diff --git a/crates/buzz-db/src/runtime/mod.rs b/crates/buzz-db/src/runtime/mod.rs index 2c0d045263..a8b177a22f 100644 --- a/crates/buzz-db/src/runtime/mod.rs +++ b/crates/buzz-db/src/runtime/mod.rs @@ -1074,10 +1074,10 @@ impl Db { /// maintenance dials a replacement. /// /// The check also closes, rather than returns, a connection still inside - /// a transaction. A checkout dropped while its `BEGIN` was in flight (a - /// timed-out proof, a cancelled caller) has no `ROLLBACK` queued: sqlx - /// counts the transaction only once `BEGIN` is acknowledged. Once the - /// reader answers, that connection would otherwise go back to the pool + /// a transaction. sqlx rolls back a checkout dropped while its `BEGIN` + /// was in flight (a timed-out proof, a cancelled caller) since + /// transact-rs/sqlx#4394, so this is a backstop: a connection that still + /// came back inside a transaction would otherwise go back to the pool /// `idle in transaction`. Closing it ends the session, and the server /// rolls the transaction back. Routed reads only begin `REPEATABLE READ` /// (see [`Db::proved_reader`]) and reader sessions default to `READ diff --git a/crates/buzz-db/src/runtime/tests.rs b/crates/buzz-db/src/runtime/tests.rs index 204efaa2ef..4311f22e4b 100644 --- a/crates/buzz-db/src/runtime/tests.rs +++ b/crates/buzz-db/src/runtime/tests.rs @@ -4218,6 +4218,9 @@ mod listed_channel_name_postgres_tests; #[path = "tests/reader_pool_postgres_tests.rs"] mod reader_pool_postgres_tests; +#[path = "tests/writer_pool_postgres_tests.rs"] +mod writer_pool_postgres_tests; + #[path = "tests/event_hydration_postgres_tests.rs"] mod event_hydration_postgres_tests; diff --git a/crates/buzz-db/src/runtime/tests/reader_pool_postgres_tests.rs b/crates/buzz-db/src/runtime/tests/reader_pool_postgres_tests.rs index d3ac5877ff..5e17e61308 100644 --- a/crates/buzz-db/src/runtime/tests/reader_pool_postgres_tests.rs +++ b/crates/buzz-db/src/runtime/tests/reader_pool_postgres_tests.rs @@ -311,13 +311,14 @@ async fn terminated_warm_reader_connection_fails_closed_to_writer() { /// A proof that times out while its `BEGIN` is still in flight must not /// return the connection to the pool with a server-side transaction open. -/// sqlx counts the transaction only once `BEGIN` is acknowledged, so a -/// cancelled begin queues no `ROLLBACK`, and the release ping consumes the -/// late `BEGIN` reply: without the reader pool's release check, the -/// connection comes back `idle in transaction` once the reader resumes. -/// The database defaults to `REPEATABLE READ`, the isolation routed reads -/// begin with, so the check only sees the open transaction because reader -/// sessions pin their own default. +/// sqlx queues a `ROLLBACK` for a begin cancelled in flight +/// (transact-rs/sqlx#4394), so the connection comes back clean and the +/// reader pool's release check has nothing to close. Before that fix the +/// release ping consumed the late `BEGIN` reply and only the release check +/// kept the connection from returning `idle in transaction`. The database +/// defaults to `REPEATABLE READ`, the isolation routed reads begin with, so +/// the check can only see an open transaction because reader sessions pin +/// their own default. #[tokio::test(flavor = "current_thread")] #[ignore = "requires Postgres"] async fn proof_timeout_mid_begin_leaves_no_reader_transaction_open() { @@ -393,8 +394,8 @@ async fn proof_timeout_mid_begin_leaves_no_reader_transaction_open() { let closes = release_closes(&snapshotter); assert_eq!( closes.get("open_transaction"), - Some(&1), - "the release check must close the connection left inside BEGIN; got {closes:?}" + None, + "sqlx must roll back the cancelled BEGIN before the release check runs; got {closes:?}" ); read_pool.close().await; diff --git a/crates/buzz-db/src/runtime/tests/writer_pool_postgres_tests.rs b/crates/buzz-db/src/runtime/tests/writer_pool_postgres_tests.rs new file mode 100644 index 0000000000..4e42455854 --- /dev/null +++ b/crates/buzz-db/src/runtime/tests/writer_pool_postgres_tests.rs @@ -0,0 +1,94 @@ +//! Writer-pool connection hygiene against a real Postgres. + +use super::*; + +/// A `BEGIN` cancelled before Postgres answers must not hand its pooled +/// connection to the next borrower still inside that transaction. +/// +/// sqlx 0.9.0 counted a transaction only once `BEGIN` was acknowledged, so a +/// begin dropped in flight queued no `ROLLBACK` and the release ping consumed +/// the late reply (fixed upstream in transact-rs/sqlx#4394). The writer pool +/// has no release check, so the next borrower's autocommit statements ran +/// inside the leftover transaction: `25001` on its next `BEGIN`, `25P02` on +/// every statement once one of them failed, and its writes were left +/// uncommitted. Covers the event read's custom `BEGIN ... READ ONLY` +/// ([`crate::event::fetch_with_read_deadline`]) and a plain `begin()`. +#[tokio::test(flavor = "current_thread")] +#[ignore = "requires Postgres"] +async fn begin_cancelled_in_flight_leaves_no_writer_transaction_open() { + let (seed, _scratch) = ScratchDb::migrated(&admin_url().await, "begin_cxl").await; + let options = seed.connect_options(); + let proxy = + crate::test_support::PausingProxy::spawn(options.get_host().to_owned(), options.get_port()) + .await; + // One connection, so every checkout is the one the cancelled begin used; + // like the writer pool, no release check. + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .connect_lazy_with((*options).clone().host("127.0.0.1").port(proxy.port)); + sqlx::query("CREATE TABLE begin_cancel_probe (n int NOT NULL)") + .execute(&pool) + .await + .expect("create probe table"); + + for (n, form) in [(1, "event read BEGIN ... READ ONLY"), (2, "plain begin()")] { + let mut conn = pool.acquire().await.expect("checkout before cancel"); + proxy.set_paused(true); + let budget = Duration::from_millis(200); + let cancelled = if n == 1 { + tokio::time::timeout( + budget, + crate::event::fetch_with_read_deadline( + &mut conn, + sqlx::query("SELECT 1"), + crate::event::ReadDeadline::PerRead, + ), + ) + .await + .is_err() + } else { + tokio::time::timeout(budget, conn.begin()).await.is_err() + }; + assert!( + cancelled, + "{form}: the paused proxy must hold BEGIN in flight" + ); + drop(conn); + proxy.set_paused(false); + + let mut next = pool.acquire().await.expect("checkout after cancel"); + let pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&mut *next) + .await + .expect("next borrower's backend pid"); + // A leftover `READ ONLY` transaction fails this write; a read-write one + // leaves it uncommitted (checked below). + if let Err(e) = sqlx::query("INSERT INTO begin_cancel_probe (n) VALUES ($1)") + .bind(n) + .execute(&mut *next) + .await + { + panic!("{form}: the next borrower's autocommit write failed: {e}"); + } + let state: Option = + sqlx::query_scalar("SELECT state FROM pg_stat_activity WHERE pid = $1") + .bind(pid) + .fetch_one(&seed) + .await + .expect("read the next borrower's backend state"); + assert_eq!( + state.as_deref(), + Some("idle"), + "{form}: the next borrower runs inside the cancelled BEGIN's transaction" + ); + } + + // Read from a connection outside the proxied pool: an autocommit write + // that ran inside a leftover transaction is not committed. + let committed: Vec = sqlx::query_scalar("SELECT n FROM begin_cancel_probe ORDER BY n") + .fetch_all(&seed) + .await + .expect("read probe rows"); + assert_eq!(committed, vec![1, 2], "every autocommit write must commit"); + pool.close().await; +} diff --git a/crates/buzz-db/src/store/entity_deletion/leases.rs b/crates/buzz-db/src/store/entity_deletion/leases.rs index 42ceeb5599..add97324d9 100644 --- a/crates/buzz-db/src/store/entity_deletion/leases.rs +++ b/crates/buzz-db/src/store/entity_deletion/leases.rs @@ -105,9 +105,8 @@ impl EntityDeletionStore { /// can be renewed only while nobody has reclaimed it. /// /// One autocommit statement: the worker drops this future whenever its - /// stage handler finishes first, and sqlx 0.9 queues no `ROLLBACK` for a - /// `begin()` dropped while its `BEGIN` is in flight, so the pooled - /// connection would carry an open transaction into the next write. + /// stage handler finishes first, so the renewal leaves no transaction + /// to roll back. pub async fn heartbeat(&self, lease: &EntityLease, duration: Duration) -> Result<()> { let millis = duration_millis(duration)?; // `lock_owned`'s predicate. After a row-lock wait, Postgres re-checks it