diff --git a/.env.example b/.env.example index 1df633b5e..f46d2f389 100644 --- a/.env.example +++ b/.env.example @@ -40,7 +40,9 @@ REDIS_URL=redis://localhost:6379 # Postgres statement_timeout and lock_timeout applied to every runtime # connection. Accepts an integer (milliseconds) with an optional us/ms/s/min/h/d -# unit; `0` disables the limit. Schema migrations always run with both lifted. +# unit; `0` disables the limit. Postgres stores both as int milliseconds, so the +# ceiling is 2147483647ms (~24 days); anything above it, or otherwise malformed, +# falls back to the default. Schema migrations always run with both lifted. # BUZZ_DB_STATEMENT_TIMEOUT=30s # BUZZ_DB_LOCK_TIMEOUT=5s diff --git a/crates/buzz-db/src/lib.rs b/crates/buzz-db/src/lib.rs index 37977a454..4019d97d3 100644 --- a/crates/buzz-db/src/lib.rs +++ b/crates/buzz-db/src/lib.rs @@ -71,6 +71,13 @@ pub const RUNTIME_STATEMENT_TIMEOUT: &str = "30s"; pub const RUNTIME_LOCK_TIMEOUT: &str = "5s"; /// Postgres spelling of "no limit", used for schema migrations. pub const TIMEOUT_DISABLED: &str = "0"; +/// `statement_timeout` and `lock_timeout` are `int` GUCs measured in +/// milliseconds, so Postgres refuses anything larger regardless of the unit it +/// is spelled with. Callers building a [`DbConfig`] from operator input must +/// range-check against this: [`apply_runtime_connection_timeouts`] runs on every +/// pooled connection, so an unusable value fails all database access. +/// `pg_timeout_max_millis_matches_postgres` pins it to the live server. +pub const PG_TIMEOUT_MAX_MILLIS: u128 = i32::MAX as u128; /// Apply the runtime safety limits shared by writer, reader, audit, and search /// pools. Values are Postgres interval strings (`"30s"`, `"500ms"`), with @@ -8522,6 +8529,67 @@ mod tests { db.pool.close().await; } + /// [`PG_TIMEOUT_MAX_MILLIS`] is the bound callers range-check operator input + /// against, so it must be the server's real bound: too high and an accepted + /// value still fails every `after_connect`; too low and we reject settings + /// Postgres would have taken. + #[tokio::test] + #[ignore = "requires Postgres"] + async fn pg_timeout_max_millis_matches_postgres() { + let mut conn = PgConnection::connect(&admin_url().await) + .await + .expect("connect"); + + let boundary = PG_TIMEOUT_MAX_MILLIS.to_string(); + apply_runtime_connection_timeouts(&mut conn, &boundary, &boundary) + .await + .expect("PG_TIMEOUT_MAX_MILLIS must be settable"); + + // Same instant spelled in a coarser unit — the conversion the caller's + // range check performs must land inside the range too. + let in_seconds = (PG_TIMEOUT_MAX_MILLIS / 1_000).to_string(); + apply_runtime_connection_timeouts( + &mut conn, + &format!("{in_seconds}s"), + &format!("{in_seconds}s"), + ) + .await + .expect("the boundary in seconds must be settable"); + + for over in [ + (PG_TIMEOUT_MAX_MILLIS + 1).to_string(), + format!("{}ms", PG_TIMEOUT_MAX_MILLIS + 1), + format!("{}s", PG_TIMEOUT_MAX_MILLIS / 1_000 + 1), + "999999999999999999999999999999999999999999d".to_string(), + ] { + let err = apply_runtime_connection_timeouts(&mut conn, &over, RUNTIME_LOCK_TIMEOUT) + .await + .expect_err(&format!("{over} must be rejected by Postgres")); + let code = match &err { + sqlx::Error::Database(db) => db.code().map(|c| c.to_string()), + other => panic!("expected a database error, got {other:?}"), + }; + assert_eq!( + code.as_deref(), + Some("22023"), + "{over}: expected invalid_parameter_value" + ); + } + + // A rejected `set_config` leaves the session usable, so the failure mode + // is a poisoned pool of unbounded sessions only if the caller ignores it. + let statement_timeout: String = sqlx::query_scalar("SHOW statement_timeout") + .fetch_one(&mut conn) + .await + .expect("SHOW statement_timeout"); + assert_ne!( + statement_timeout, "0", + "a rejected value must not silently disable the limit" + ); + + conn.close().await.expect("close"); + } + /// `spawn_fence_probe` must verify the floor guard before letting the /// probe run — catalog shape AND observed behavior — and refuse on /// sabotage. This is the production gate for a relay running with diff --git a/crates/buzz-relay/src/config.rs b/crates/buzz-relay/src/config.rs index ee88c11ab..67fae1260 100644 --- a/crates/buzz-relay/src/config.rs +++ b/crates/buzz-relay/src/config.rs @@ -306,14 +306,30 @@ fn parse_bind_addr(raw: &str) -> Result { .map_err(|e| ConfigError::InvalidBindAddr(e.to_string())) } -/// Postgres accepts a timeout as an integer (milliseconds) with an optional -/// unit. Anything else is refused in favor of the default rather than failing -/// the config: a malformed value would break every `after_connect`, taking all -/// Postgres access with it, and a relay that keeps its documented default is a -/// better outcome than one that will not start. -fn pg_timeout_or_default(raw: Option<&str>, default: &str) -> String { - const UNITS: [&str; 6] = ["us", "ms", "s", "min", "h", "d"]; +/// Convert a Postgres timeout magnitude and unit to milliseconds, mirroring the +/// rounding Postgres applies to sub-millisecond `us` values. `None` means the +/// spelling is not something Postgres would accept. +fn pg_timeout_millis(magnitude: &str, unit: &str) -> Option { + let value = magnitude.parse::().ok()?; + match unit { + "us" => Some((value + 500) / 1_000), + "" | "ms" => Some(value), + "s" => value.checked_mul(1_000), + "min" => value.checked_mul(60_000), + "h" => value.checked_mul(3_600_000), + "d" => value.checked_mul(86_400_000), + _ => None, + } +} +/// Postgres accepts a timeout as an integer (milliseconds) with an optional +/// unit, bounded by `i32::MAX` ms once converted. Anything else is refused in +/// favor of the default rather than failing the config: an unusable value would +/// break every `after_connect`, taking all Postgres access with it, and a relay +/// that keeps its documented default is a better outcome than one that will not +/// start. Magnitude is range-checked, not just shape-checked — `999...9d` is a +/// well-formed spelling that Postgres still rejects. +fn pg_timeout_or_default(raw: Option<&str>, default: &str) -> String { let candidate = raw.map(str::trim).filter(|value| !value.is_empty()); let Some(candidate) = candidate else { return default.to_string(); @@ -321,19 +337,29 @@ fn pg_timeout_or_default(raw: Option<&str>, default: &str) -> String { let digits = candidate.chars().take_while(char::is_ascii_digit).count(); let (magnitude, unit) = candidate.split_at(digits); - let unit = unit.trim(); - let valid = !magnitude.is_empty() - && (unit.is_empty() || UNITS.iter().any(|known| unit.eq_ignore_ascii_case(known))); - if valid { - candidate.to_string() - } else { - tracing::warn!( - value = candidate, - default, - "ignoring malformed Postgres timeout — expected an integer with an optional \ - us/ms/s/min/h/d unit" - ); - default.to_string() + let unit = unit.trim().to_ascii_lowercase(); + + match pg_timeout_millis(magnitude, &unit) { + Some(millis) if millis <= buzz_db::PG_TIMEOUT_MAX_MILLIS => candidate.to_string(), + Some(millis) => { + tracing::warn!( + value = candidate, + millis = %millis, + max_millis = %buzz_db::PG_TIMEOUT_MAX_MILLIS, + default, + "ignoring out-of-range Postgres timeout — Postgres stores it as int milliseconds" + ); + default.to_string() + } + None => { + tracing::warn!( + value = candidate, + default, + "ignoring malformed Postgres timeout — expected an integer with an optional \ + us/ms/s/min/h/d unit" + ); + default.to_string() + } } } @@ -1337,6 +1363,47 @@ mod tests { assert_eq!(pg_timeout_or_default(None, "5s"), "5s"); } + #[test] + fn pg_timeout_refuses_magnitudes_postgres_cannot_store() { + // Well-formed spellings whose millisecond value exceeds the int GUC + // range. Postgres rejects these in `set_config`, which would fail every + // pool's `after_connect`. + for rejected in [ + "2147483648", + "2147483648ms", + "2147484s", + "35792min", + "597h", + "25d", + // Wider than any integer type — must fall back, not overflow. + "999999999999999999999999999999999999999999d", + "99999999999999999999999999999999999999999999999999", + ] { + assert_eq!( + pg_timeout_or_default(Some(rejected), "30s"), + "30s", + "{rejected:?} is out of range for Postgres and must fall back" + ); + } + + // The boundary itself, and the same instant in every unit, stay valid. + for accepted in [ + "2147483647", + "2147483647ms", + "2147483647000us", + "2147483s", + "35791min", + "596h", + "24d", + ] { + assert_eq!( + pg_timeout_or_default(Some(accepted), "30s"), + accepted, + "{accepted} is inside the Postgres range" + ); + } + } + #[test] fn db_timeout_env_overrides_and_invalid_fallback() { let _guard = ENV_MUTEX.lock().unwrap(); @@ -1366,6 +1433,21 @@ mod tests { "a malformed value must not be handed to Postgres" ); + // Shape-valid but far outside the int millisecond GUC range: Postgres + // would reject it in every pool's `after_connect`. + std::env::set_var( + "BUZZ_DB_STATEMENT_TIMEOUT", + "999999999999999999999999999999999999999999d", + ); + std::env::set_var("BUZZ_DB_LOCK_TIMEOUT", "25d"); + let out_of_range = Config::from_env().expect("config"); + assert_eq!( + out_of_range.db_statement_timeout, + buzz_db::RUNTIME_STATEMENT_TIMEOUT, + "an out-of-range value must not be handed to Postgres" + ); + assert_eq!(out_of_range.db_lock_timeout, buzz_db::RUNTIME_LOCK_TIMEOUT); + match previous_statement { Some(value) => std::env::set_var("BUZZ_DB_STATEMENT_TIMEOUT", value), None => std::env::remove_var("BUZZ_DB_STATEMENT_TIMEOUT"),