mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-25 16:02:11 +00:00
fix: patch sqlx so a cancelled BEGIN cannot poison a pooled connection (#10823)
* fix: patch sqlx so a cancelled BEGIN cannot poison a pooled connection Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzqmh52NU8fB9RBQNNkJGt * test: drop the migration run and fixed sleep from the sqlx patch guard Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzqmh52NU8fB9RBQNNkJGt * test: ignore the sqlx patch guard by default and point at it from where sqlx is changed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzqmh52NU8fB9RBQNNkJGt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
GitHub
Generated
+7
-14
@@ -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",
|
||||
|
||||
@@ -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" }
|
||||
|
||||
@@ -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<Postgres>) {
|
||||
// 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");
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user