sszylowxqlqtmerged#5
fix(db): bound every pool liveness ping so a dead connection cannot strand its slot
Mcrates/df-db/src/lib.rs+59−6
| @@ −6,7 +6,7 @@ | |||
| 6 | 6 | use std::time::Duration; | |
| 7 | 7 | ||
| 8 | 8 | use sqlx::postgres::{PgConnectOptions, PgPoolOptions}; | |
| 9 | − | use sqlx::ConnectOptions; | |
| 9 | + | use sqlx::{ConnectOptions, Connection}; | |
| 10 | 10 | ||
| 11 | 11 | pub mod ids; | |
| 12 | 12 | pub mod models; | |
| @@ −23,6 +23,13 @@ | |||
| 23 | 23 | /// concurrently. | |
| 24 | 24 | const MIGRATION_LOCK_KEY: i64 = 0x0D06_F00D_0000_0001u64 as i64; | |
| 25 | 25 | ||
| 26 | + | /// How long a liveness `ping()` may take before we treat the connection as dead. | |
| 27 | + | /// | |
| 28 | + | /// Comfortably longer than a healthy round trip to the pooler and well inside | |
| 29 | + | /// `acquire_timeout`, so discarding a dead connection still leaves most of the | |
| 30 | + | /// acquire budget to open a replacement. | |
| 31 | + | const PING_TIMEOUT: Duration = Duration::from_secs(2); | |
| 32 | + | ||
| 26 | 33 | /// Open the pool. | |
| 27 | 34 | /// | |
| 28 | 35 | /// Statement logging is disabled below WARN: at INFO sqlx logs every statement, | |
| @@ −37,19 +44,65 @@ | |||
| 37 | 44 | ||
| 38 | 45 | let pool = PgPoolOptions::new() | |
| 39 | 46 | .max_connections(max_connections) | |
| 40 | − | // The provided instance reports max_connections = 100 and that budget is | |
| 41 | − | // shared, so we stay well clear of it and recycle idle connections. | |
| 42 | 47 | .min_connections(1) | |
| 43 | 48 | .acquire_timeout(Duration::from_secs(10)) | |
| 44 | − | .idle_timeout(Duration::from_secs(300)) | |
| 45 | − | .max_lifetime(Duration::from_secs(1800)) | |
| 46 | − | .test_before_acquire(true) | |
| 49 | + | // No `idle_timeout` or `max_lifetime`, deliberately. | |
| 50 | + | // | |
| 51 | + | // Both are implemented by closing the connection from | |
| 52 | + | // `PoolConnection::return_to_pool`, which awaits `PgConnection::close()` | |
| 53 | + | // with no timeout of its own — unlike the `close_on_drop` path, which | |
| 54 | + | // bounds it. `close()` sends Terminate and then awaits the stream | |
| 55 | + | // shutdown. Against the pooler on :6543 the Terminate lands but the | |
| 56 | + | // shutdown never completes, so the spawned task hangs forever still | |
| 57 | + | // holding its pool slot, and the socket is never closed. | |
| 58 | + | // | |
| 59 | + | // With `max_lifetime` set to 30 minutes that leaked one slot every 30 | |
| 60 | + | // minutes, so every process died a fixed `max_connections * 30min` | |
| 61 | + | // after boot: on 2026-08-03 the worker booted at 01:01 and started | |
| 62 | + | // failing every acquire at 06:02. sqlx 0.8 has no keepalive option and | |
| 63 | + | // `test_before_acquire` cannot rescue it either, because `ping()` is | |
| 64 | + | // likewise unbounded and just burns the whole `acquire_timeout`. | |
| 65 | + | // | |
| 66 | + | // Recycling was never load-bearing: the instance allows 100 | |
| 67 | + | // connections and all three services together hold ~23. | |
| 68 | + | // | |
| 69 | + | // The built-in `test_before_acquire` is off for the same reason: its | |
| 70 | + | // `ping()` is unbounded, so a connection the pooler dropped silently | |
| 71 | + | // hangs the acquire instead of failing it. The hooks below do the same | |
| 72 | + | // liveness check under `PING_TIMEOUT`. | |
| 73 | + | // | |
| 74 | + | // They must report a dead connection as `Err`, never `Ok(false)`: | |
| 75 | + | // sqlx answers `Err` with `close_hard()`, which only awaits | |
| 76 | + | // `stream.shutdown()`, but answers `Ok(false)` with the same unbounded | |
| 77 | + | // `close()` that stranded slots above. | |
| 78 | + | .test_before_acquire(false) | |
| 79 | + | .before_acquire(|conn, _meta| Box::pin(async move { ping_bounded(conn).await.map(|()| true) })) | |
| 80 | + | // `return_to_pool` pings unconditionally on release, and that ping is | |
| 81 | + | // unbounded too — a connection that died while checked out would hang | |
| 82 | + | // the spawned release task and strand the slot for good. Pinging here | |
| 83 | + | // first means the unbounded one runs only against a connection that | |
| 84 | + | // answered milliseconds ago. | |
| 85 | + | .after_release(|conn, _meta| Box::pin(async move { ping_bounded(conn).await.map(|()| true) })) | |
| 47 | 86 | .connect_with(opts) | |
| 48 | 87 | .await?; | |
| 49 | 88 | ||
| 50 | 89 | Ok(pool) | |
| 51 | 90 | } | |
| 52 | 91 | ||
| 92 | + | /// `ping()` under a timeout, reporting a timeout as an error. | |
| 93 | + | /// | |
| 94 | + | /// Returning `Err` matters: it is what routes the connection to sqlx's bounded | |
| 95 | + | /// `close_hard()` rather than the `close()` that hangs against the pooler. | |
| 96 | + | async fn ping_bounded(conn: &mut sqlx::PgConnection) -> Result<(), sqlx::Error> { | |
| 97 | + | match tokio::time::timeout(PING_TIMEOUT, conn.ping()).await { | |
| 98 | + | Ok(result) => result, | |
| 99 | + | Err(_elapsed) => Err(sqlx::Error::Io(std::io::Error::new( | |
| 100 | + | std::io::ErrorKind::TimedOut, | |
| 101 | + | "connection ping timed out", | |
| 102 | + | ))), | |
| 103 | + | } | |
| 104 | + | } | |
| 105 | + | ||
| 53 | 106 | /// Apply pending migrations under an advisory lock. | |
| 54 | 107 | /// | |
| 55 | 108 | /// sqlx has its own locking, but it is per-migration; taking one session-level | |