diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 43ff602ec1..6b76f995f9 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -11849,8 +11849,7 @@ dependencies = [ [[package]] name = "sqlx" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1fefb893899429669dcdd979aff487bd78f4064e5e7907e4269081e0ef7d97dc" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "sqlx-core", "sqlx-macros", @@ -11862,8 +11861,7 @@ dependencies = [ [[package]] name = "sqlx-core" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee6798b1838b6a0f69c007c133b8df5866302197e404e8b6ee8ed3e3a5e68dc6" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "base64 0.22.1", "bigdecimal", @@ -11901,8 +11899,7 @@ dependencies = [ [[package]] name = "sqlx-macros" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2d452988ccaacfbf5e0bdbc348fb91d7c8af5bee192173ac3636b5fb6e6715d" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "proc-macro2", "quote", @@ -11914,8 +11911,7 @@ dependencies = [ [[package]] name = "sqlx-macros-core" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "19a9c1841124ac5a61741f96e1d9e2ec77424bf323962dd894bdb93f37d5219b" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "dotenvy", "either", @@ -11939,8 +11935,7 @@ dependencies = [ [[package]] name = "sqlx-mysql" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aa003f0038df784eb8fecbbac13affe3da23b45194bd57dba231c8f48199c526" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "atoi", "base64 0.22.1", @@ -11984,8 +11979,7 @@ dependencies = [ [[package]] name = "sqlx-postgres" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "db58fcd5a53cf07c184b154801ff91347e4c30d17a3562a635ff028ad5deda46" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "atoi", "base64 0.22.1", @@ -12025,8 +12019,7 @@ dependencies = [ [[package]] name = "sqlx-sqlite" version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c2d12fe70b2c1b4401038055f90f151b78208de1f9f89a7dbfd41587a10c3eea" +source = "git+https://github.com/windmill-labs/sqlx?rev=6bdaee94fa62a01561125646da3f99eb341f2457#6bdaee94fa62a01561125646da3f99eb341f2457" dependencies = [ "atoi", "chrono", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 0bfa52d3e6..0e08b0f060 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -214,6 +214,26 @@ all_sqlx_features = ["all_languages", "enterprise", "enterprise_saml", "embeddin "windmill-git-sync/all_sqlx_features"] [patch.crates-io] +# v0.8.6 plus one commit: `Pool::begin` is not cancel-safe on Postgres. sqlx raises the +# transaction depth its rollback-on-drop guard keys on only *after* the BEGIN round trip, so +# a cancelled caller (a disconnecting API client, a `timeout`, an aborted task) leaves the +# session in a transaction nothing will end, and the pool hands that connection out again — +# every later query on it fails with 25P02 until max_lifetime recycles it 30 minutes on. +# Reported upstream in 2022 (launchbadge/sqlx#2054), fixed for SQLite only, and still present +# in 0.9.0. Drop this the moment upstream carries the fix. +# The whole family has to move together: `sqlx-postgres` depends on `sqlx-core` by path +# inside the sqlx workspace, so patching it alone leaves two incompatible `sqlx-core`s and +# `Postgres` stops implementing the `Database` the macros expect. +# Changing any of this — a bump, a rebase of the fork, dropping these lines — still compiles +# clean, so run the guard that actually checks the behaviour is still there: +# cargo test -p windmill-common --test sqlx_begin_cancel_safe -- --ignored +sqlx = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } +sqlx-core = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } +sqlx-macros = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } +sqlx-macros-core = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } +sqlx-postgres = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } +sqlx-mysql = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } +sqlx-sqlite = { git = "https://github.com/windmill-labs/sqlx", rev = "6bdaee94fa62a01561125646da3f99eb341f2457" } object_store = { git = "https://github.com/apache/arrow-rs-object-store", rev = "36752c975d4f29e20b57c91f81a10872dcd48ae7" } # Use tiberius main branch for libgssapi 0.8.1 fix (https://github.com/prisma/tiberius/issues/343) tiberius = { git = "https://github.com/prisma/tiberius", rev = "59db57960a14b422fb3a1309aa4aa47880896ff8" } diff --git a/backend/windmill-common/tests/sqlx_begin_cancel_safe.rs b/backend/windmill-common/tests/sqlx_begin_cancel_safe.rs new file mode 100644 index 0000000000..1f387984b0 --- /dev/null +++ b/backend/windmill-common/tests/sqlx_begin_cancel_safe.rs @@ -0,0 +1,72 @@ +//! Guards the `sqlx` entries in `[patch.crates-io]` — `backend/Cargo.toml` carries the why. +//! Dropping the patch still compiles, so a test is what notices. +//! +//! Ignored by default: it only has something to say when the sqlx dependency moves, and it +//! spends a couple of seconds waiting on a deliberately slow round trip. Run it whenever you +//! touch sqlx — a version bump, a change to the patch entries, a fork rebase: +//! +//! ```text +//! cargo test -p windmill-common --test sqlx_begin_cancel_safe -- --ignored +//! ``` + +use sqlx::{Connection, PgConnection, Pool, Postgres}; +use std::time::{Duration, Instant}; + +#[sqlx::test] +#[ignore = "run with --ignored after any sqlx bump or change to [patch.crates-io]"] +async fn begin_cancelled_mid_round_trip_leaves_no_open_transaction(db: Pool) { + // One connection, so the session inspected below is the one the cancelled begin used. + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .min_connections(0) + .connect_with((*db.connect_options()).clone()) + .await + .expect("failed to build pool"); + let pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()") + .fetch_one(&pool) + .await + .unwrap(); + + // A plain `BEGIN` answers in well under a millisecond, which is too narrow to cancel + // reliably; appending a sleep widens the round trip and runs through the same + // `PgTransactionManager::begin` the patch fixes. + let cancelled = tokio::time::timeout( + Duration::from_millis(300), + pool.begin_with("BEGIN; SELECT pg_sleep(2);"), + ) + .await; + assert!(cancelled.is_err(), "the begin must not have completed"); + + let mut admin = PgConnection::connect_with(&(*db.connect_options()).clone()) + .await + .expect("failed to open an observing connection"); + + // sqlx only flushes the queued ROLLBACK once the abandoned statement has answered, so + // wait for the session to stop running rather than sleeping a fixed time a loaded runner + // could overshoot. + let deadline = Instant::now() + Duration::from_secs(30); + let state = loop { + let state: String = sqlx::query_scalar("SELECT state FROM pg_stat_activity WHERE pid = $1") + .bind(pid) + .fetch_optional(&mut admin) + .await + .unwrap() + .flatten() + .unwrap_or_default(); + if state != "active" || Instant::now() >= deadline { + break state; + } + tokio::time::sleep(Duration::from_millis(100)).await; + }; + + assert!( + !state.starts_with("idle in transaction"), + "connection returned to the pool still inside a transaction (state {state:?}) — is \ + the sqlx patch in backend/Cargo.toml still applied?" + ); + + sqlx::query_scalar::<_, i32>("SELECT 1") + .fetch_one(&pool) + .await + .expect("pool must still serve queries"); +} diff --git a/docs/validation.md b/docs/validation.md index 050eacd75b..74e9cf5677 100644 --- a/docs/validation.md +++ b/docs/validation.md @@ -16,6 +16,7 @@ After making changes, run the appropriate checks and fix all errors before consi | Multiple gated modules | `cargo check --features enterprise,parquet` | Combine only the flags you need | | API route changes | `cargo check` | Then update `openapi.yaml` and run `npm run generate-backend-client` | | Database migrations | `cargo check` | Test migration applies cleanly with `sqlx migrate run` | +| The `sqlx` dependency (version bump, `[patch.crates-io]` entries, fork rebase) | `cargo test -p windmill-common --test sqlx_begin_cancel_safe -- --ignored` | Windmill runs a patched `sqlx`: upstream's `Pool::begin` is not cancel-safe on Postgres, and a cancelled one poisons the pooled connection for 30 minutes. Losing the patch still compiles, so this ignored test is the only thing that notices. `backend/Cargo.toml` has the detail | **Never** use `--features all_sqlx_features` — it compiles everything and is very slow. Check `backend/Cargo.toml` `[features]` to find the right flags.