mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
refactor(db): make PgTimeout the only way to build a Postgres timeout
The rule that a timeout must be one Postgres accepts lived in a doc comment on PG_TIMEOUT_MAX_MILLIS and was enforced by a parser two crates away. Three separate escapes were found in review: an unbounded magnitude, a u128 overflow in the microsecond rounding, and uncanonicalized uppercase units. All three passed a String-typed DbConfig unchallenged. Parse the value into a PgTimeout newtype instead, next to the constant and the applier. DbConfig and apply_runtime_connection_timeouts now take PgTimeout, so a value Postgres would reject is unrepresentable rather than merely tested for, and PG_TIMEOUT_MAX_MILLIS is private. buzz-admin picked up the relay's 30s/5s defaults through DbConfig::default() even though it is a one-shot operator CLI with no shared pool to starve, and it never read the env overrides -- so a bulk repair outliving 30s was cancelled with no way to widen it. It now defaults to no caps and honours the same env vars via PgTimeout::from_env_or. Signed-off-by: Eli Foster <efoster@squareup.com>
This commit is contained in:
@@ -25,7 +25,7 @@ use std::sync::Arc;
|
||||
use anyhow::Result;
|
||||
use buzz_core::kind::KIND_NIP43_MEMBERSHIP_LIST;
|
||||
use buzz_core::tenant::{relay_url_authority, TenantContext};
|
||||
use buzz_db::{Db, DbConfig};
|
||||
use buzz_db::{Db, DbConfig, PgTimeout};
|
||||
use buzz_pubsub::{EventTopic, PubSubManager};
|
||||
use clap::{Parser, Subcommand};
|
||||
use nostr::{EventBuilder, Keys, Kind, Tag};
|
||||
@@ -420,8 +420,17 @@ async fn connect_member_services() -> Result<(Db, Arc<PubSubManager>, Keys)> {
|
||||
async fn connect_db() -> Result<Db> {
|
||||
let db_url = std::env::var("DATABASE_URL")
|
||||
.unwrap_or_else(|_| "postgres://buzz:buzz_dev@localhost:5432/buzz".to_string());
|
||||
// No runtime caps by default. `DbConfig`'s 30s/5s are sized for the relay's
|
||||
// request-serving traffic on a shared pool — an operator CLI is neither, and
|
||||
// a bulk repair that outlives 30s should finish, not get cancelled. The same
|
||||
// env vars still apply for an operator who wants a bound here.
|
||||
let db = Db::new(&DbConfig {
|
||||
database_url: db_url,
|
||||
statement_timeout: PgTimeout::from_env_or(
|
||||
"BUZZ_DB_STATEMENT_TIMEOUT",
|
||||
PgTimeout::disabled(),
|
||||
),
|
||||
lock_timeout: PgTimeout::from_env_or("BUZZ_DB_LOCK_TIMEOUT", PgTimeout::disabled()),
|
||||
..DbConfig::default()
|
||||
})
|
||||
.await?;
|
||||
|
||||
+303
-32
@@ -73,26 +73,168 @@ pub const RUNTIME_LOCK_TIMEOUT: &str = "5s";
|
||||
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.
|
||||
/// is spelled with. Private on purpose: [`PgTimeout`] is the only way to build a
|
||||
/// timeout, so no caller needs to range-check by hand.
|
||||
/// `pg_timeout_max_millis_matches_postgres` pins it to the live server.
|
||||
pub const PG_TIMEOUT_MAX_MILLIS: u128 = i32::MAX as u128;
|
||||
const PG_TIMEOUT_MAX_MILLIS: u128 = i32::MAX as u128;
|
||||
|
||||
/// Why a string is not a timeout Postgres would accept.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum PgTimeoutError {
|
||||
/// Not an integer with an optional `us`/`ms`/`s`/`min`/`h`/`d` unit.
|
||||
Malformed,
|
||||
/// Well-formed, but larger than Postgres can store in an `int` GUC.
|
||||
OutOfRange {
|
||||
/// The value in milliseconds, for the operator-facing message.
|
||||
millis: u128,
|
||||
},
|
||||
}
|
||||
|
||||
impl std::fmt::Display for PgTimeoutError {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
match self {
|
||||
Self::Malformed => write!(
|
||||
f,
|
||||
"expected an integer with an optional us/ms/s/min/h/d unit"
|
||||
),
|
||||
Self::OutOfRange { millis } => write!(
|
||||
f,
|
||||
"{millis}ms exceeds the {PG_TIMEOUT_MAX_MILLIS}ms Postgres stores in an int GUC"
|
||||
),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl std::error::Error for PgTimeoutError {}
|
||||
|
||||
/// A Postgres timeout that the server is known to accept.
|
||||
///
|
||||
/// Parsing is the only way to build one from operator input, so a [`DbConfig`]
|
||||
/// cannot carry a value that would break
|
||||
/// [`apply_runtime_connection_timeouts`] — which runs on every pooled
|
||||
/// connection, and would therefore fail all database access. The stored
|
||||
/// spelling is canonical: the magnitude followed by the lowercased unit, since
|
||||
/// Postgres' unit names are case-sensitive.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct PgTimeout(String);
|
||||
|
||||
impl PgTimeout {
|
||||
/// The default runtime statement timeout ([`RUNTIME_STATEMENT_TIMEOUT`]).
|
||||
pub fn statement_default() -> Self {
|
||||
Self(RUNTIME_STATEMENT_TIMEOUT.to_string())
|
||||
}
|
||||
|
||||
/// The default runtime lock timeout ([`RUNTIME_LOCK_TIMEOUT`]).
|
||||
pub fn lock_default() -> Self {
|
||||
Self(RUNTIME_LOCK_TIMEOUT.to_string())
|
||||
}
|
||||
|
||||
/// No limit at all — what schema migrations and one-shot operator tools
|
||||
/// run with.
|
||||
pub fn disabled() -> Self {
|
||||
Self(TIMEOUT_DISABLED.to_string())
|
||||
}
|
||||
|
||||
/// The canonical spelling, ready to hand to Postgres.
|
||||
pub fn as_str(&self) -> &str {
|
||||
&self.0
|
||||
}
|
||||
|
||||
/// Parse operator input, falling back to `default` with a warning.
|
||||
///
|
||||
/// Refusing to start on a bad timeout would be the same outage the limits
|
||||
/// prevent, so a malformed or out-of-range value keeps the documented
|
||||
/// default instead. `None` and blank mean "unset".
|
||||
pub fn parse_or(raw: Option<&str>, default: Self) -> Self {
|
||||
let Some(candidate) = raw.map(str::trim).filter(|value| !value.is_empty()) else {
|
||||
return default;
|
||||
};
|
||||
match candidate.parse::<Self>() {
|
||||
Ok(timeout) => timeout,
|
||||
Err(error) => {
|
||||
tracing::warn!(
|
||||
value = candidate,
|
||||
default = default.as_str(),
|
||||
%error,
|
||||
"ignoring unusable Postgres timeout"
|
||||
);
|
||||
default
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// [`PgTimeout::parse_or`] against an environment variable.
|
||||
pub fn from_env_or(var: &str, default: Self) -> Self {
|
||||
Self::parse_or(std::env::var(var).ok().as_deref(), default)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
impl PgTimeout {
|
||||
/// Build without validation, so a test can hand Postgres a value
|
||||
/// [`FromStr`](std::str::FromStr) refuses to produce and prove the server
|
||||
/// rejects it.
|
||||
pub(crate) fn unchecked(raw: impl Into<String>) -> Self {
|
||||
Self(raw.into())
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for PgTimeout {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
f.write_str(&self.0)
|
||||
}
|
||||
}
|
||||
|
||||
impl std::str::FromStr for PgTimeout {
|
||||
type Err = PgTimeoutError;
|
||||
|
||||
fn from_str(raw: &str) -> std::result::Result<Self, Self::Err> {
|
||||
let candidate = raw.trim();
|
||||
let digits = candidate.chars().take_while(char::is_ascii_digit).count();
|
||||
let (magnitude, unit) = candidate.split_at(digits);
|
||||
let unit = unit.trim().to_ascii_lowercase();
|
||||
|
||||
match pg_timeout_millis(magnitude, &unit) {
|
||||
Some(millis) if millis <= PG_TIMEOUT_MAX_MILLIS => {
|
||||
Ok(Self(format!("{magnitude}{unit}")))
|
||||
}
|
||||
Some(millis) => Err(PgTimeoutError::OutOfRange { millis }),
|
||||
None => Err(PgTimeoutError::Malformed),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// 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<u128> {
|
||||
let value = magnitude.parse::<u128>().ok()?;
|
||||
match unit {
|
||||
// Round half up without the `value + 500` intermediate, which overflows
|
||||
// for the top 500 representable microsecond values.
|
||||
"us" => Some(value / 1_000 + u128::from(value % 1_000 >= 500)),
|
||||
"" | "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,
|
||||
}
|
||||
}
|
||||
|
||||
/// Apply the runtime safety limits shared by writer, reader, audit, and search
|
||||
/// pools. Values are Postgres interval strings (`"30s"`, `"500ms"`), with
|
||||
/// [`TIMEOUT_DISABLED`] lifting a limit entirely.
|
||||
/// pools. [`PgTimeout::disabled`] lifts a limit entirely.
|
||||
pub async fn apply_runtime_connection_timeouts(
|
||||
connection: &mut PgConnection,
|
||||
statement_timeout: &str,
|
||||
lock_timeout: &str,
|
||||
statement_timeout: &PgTimeout,
|
||||
lock_timeout: &PgTimeout,
|
||||
) -> std::result::Result<(), sqlx::Error> {
|
||||
sqlx::query(
|
||||
"SELECT set_config('statement_timeout', $1, false), \
|
||||
set_config('lock_timeout', $2, false)",
|
||||
)
|
||||
.bind(statement_timeout)
|
||||
.bind(lock_timeout)
|
||||
.bind(statement_timeout.as_str())
|
||||
.bind(lock_timeout.as_str())
|
||||
.execute(connection)
|
||||
.await?;
|
||||
Ok(())
|
||||
@@ -562,12 +704,12 @@ pub struct DbConfig {
|
||||
pub replica_read_max_age_ms: u64,
|
||||
/// Postgres `statement_timeout` applied to every runtime connection. An
|
||||
/// operator running a backfill or working an incident can widen this without
|
||||
/// a code change; [`TIMEOUT_DISABLED`] removes the cap.
|
||||
pub statement_timeout: String,
|
||||
/// a code change; [`PgTimeout::disabled`] removes the cap.
|
||||
pub statement_timeout: PgTimeout,
|
||||
/// Postgres `lock_timeout` applied to every runtime connection. Bounds
|
||||
/// heavyweight and row lock waits only — advisory-lock waits are bounded by
|
||||
/// [`Self::statement_timeout`] instead.
|
||||
pub lock_timeout: String,
|
||||
pub lock_timeout: PgTimeout,
|
||||
}
|
||||
|
||||
impl Default for DbConfig {
|
||||
@@ -585,8 +727,8 @@ impl Default for DbConfig {
|
||||
max_lifetime_secs: 1800,
|
||||
idle_timeout_secs: 600,
|
||||
replica_read_max_age_ms: 0,
|
||||
statement_timeout: RUNTIME_STATEMENT_TIMEOUT.to_string(),
|
||||
lock_timeout: RUNTIME_LOCK_TIMEOUT.to_string(),
|
||||
statement_timeout: PgTimeout::statement_default(),
|
||||
lock_timeout: PgTimeout::lock_default(),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -6595,8 +6737,8 @@ mod tests {
|
||||
database_url: scratch_url,
|
||||
max_connections: 2,
|
||||
min_connections: 2,
|
||||
statement_timeout: TIGHT.to_string(),
|
||||
lock_timeout: TIGHT.to_string(),
|
||||
statement_timeout: TIGHT.parse().expect("test timeout literal"),
|
||||
lock_timeout: TIGHT.parse().expect("test timeout literal"),
|
||||
..DbConfig::default()
|
||||
})
|
||||
.await
|
||||
@@ -8529,10 +8671,138 @@ 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.
|
||||
/// The built-in defaults bypass parsing, so prove they would survive it —
|
||||
/// otherwise a typo in a literal ships a value Postgres refuses.
|
||||
#[test]
|
||||
fn builtin_timeout_defaults_are_parseable() {
|
||||
for default in [
|
||||
PgTimeout::statement_default(),
|
||||
PgTimeout::lock_default(),
|
||||
PgTimeout::disabled(),
|
||||
] {
|
||||
assert_eq!(
|
||||
default.as_str().parse::<PgTimeout>().as_ref(),
|
||||
Ok(&default),
|
||||
"{default} must round-trip through the parser"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pg_timeout_accepts_postgres_spellings_and_refuses_the_rest() {
|
||||
let default = PgTimeout::statement_default();
|
||||
for (accepted, canonical) in [
|
||||
("30s", "30s"),
|
||||
("500ms", "500ms"),
|
||||
("0", "0"),
|
||||
("45S", "45s"),
|
||||
("2MIN", "2min"),
|
||||
(" 10 H ", "10h"),
|
||||
] {
|
||||
assert_eq!(
|
||||
PgTimeout::parse_or(Some(accepted), default.clone()).as_str(),
|
||||
canonical,
|
||||
"{accepted} is a valid Postgres timeout"
|
||||
);
|
||||
}
|
||||
|
||||
// A rejected value must not reach Postgres: `after_connect` would fail
|
||||
// for every connection, which is worse than the documented default.
|
||||
for rejected in ["", " ", "soon", "30 seconds", "s30", "-5s", "30s;DROP"] {
|
||||
assert_eq!(
|
||||
PgTimeout::parse_or(Some(rejected), default.clone()),
|
||||
default,
|
||||
"{rejected:?} must fall back to the default"
|
||||
);
|
||||
}
|
||||
|
||||
assert_eq!(
|
||||
PgTimeout::parse_or(None, PgTimeout::lock_default()),
|
||||
PgTimeout::lock_default()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pg_timeout_refuses_magnitudes_postgres_cannot_store() {
|
||||
let default = PgTimeout::statement_default();
|
||||
// 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",
|
||||
// Parses as u128, so unlike the two above it reaches the unit
|
||||
// conversion — where rounding must not overflow on the way to the
|
||||
// range check.
|
||||
&format!("{}us", u128::MAX),
|
||||
&format!("{}us", u128::MAX - 499),
|
||||
] {
|
||||
assert_eq!(
|
||||
PgTimeout::parse_or(Some(rejected), default.clone()),
|
||||
default,
|
||||
"{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!(
|
||||
PgTimeout::parse_or(Some(accepted), default.clone()).as_str(),
|
||||
accepted,
|
||||
"{accepted} is inside the Postgres range"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Every spelling the parser emits must be usable by Postgres. A
|
||||
/// parser-only assertion missed that Postgres rejects uppercase units even
|
||||
/// though validation lowercases them.
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn pg_timeout_canonical_spellings_are_accepted_by_postgres() {
|
||||
let mut connection = PgConnection::connect(&admin_url().await)
|
||||
.await
|
||||
.expect("connect test Postgres");
|
||||
|
||||
for (raw, canonical) in [
|
||||
("500US", "500us"),
|
||||
("500MS", "500ms"),
|
||||
("2S", "2s"),
|
||||
("2MIN", "2min"),
|
||||
("2H", "2h"),
|
||||
("2D", "2d"),
|
||||
("0", "0"),
|
||||
(" 10 S ", "10s"),
|
||||
] {
|
||||
let parsed = PgTimeout::parse_or(Some(raw), PgTimeout::statement_default());
|
||||
assert_eq!(parsed.as_str(), canonical);
|
||||
apply_runtime_connection_timeouts(&mut connection, &parsed, &parsed)
|
||||
.await
|
||||
.unwrap_or_else(|error| panic!("{raw:?} normalized to {parsed}: {error}"));
|
||||
}
|
||||
|
||||
connection.close().await.expect("close test Postgres");
|
||||
}
|
||||
|
||||
/// `PG_TIMEOUT_MAX_MILLIS` is the bound [`PgTimeout`] range-checks 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() {
|
||||
@@ -8540,7 +8810,7 @@ mod tests {
|
||||
.await
|
||||
.expect("connect");
|
||||
|
||||
let boundary = PG_TIMEOUT_MAX_MILLIS.to_string();
|
||||
let boundary = PgTimeout::unchecked(PG_TIMEOUT_MAX_MILLIS.to_string());
|
||||
apply_runtime_connection_timeouts(&mut conn, &boundary, &boundary)
|
||||
.await
|
||||
.expect("PG_TIMEOUT_MAX_MILLIS must be settable");
|
||||
@@ -8548,13 +8818,10 @@ mod tests {
|
||||
// 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");
|
||||
let boundary_seconds = PgTimeout::unchecked(format!("{in_seconds}s"));
|
||||
apply_runtime_connection_timeouts(&mut conn, &boundary_seconds, &boundary_seconds)
|
||||
.await
|
||||
.expect("the boundary in seconds must be settable");
|
||||
|
||||
for over in [
|
||||
(PG_TIMEOUT_MAX_MILLIS + 1).to_string(),
|
||||
@@ -8562,9 +8829,13 @@ mod tests {
|
||||
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 err = apply_runtime_connection_timeouts(
|
||||
&mut conn,
|
||||
&PgTimeout::unchecked(over.clone()),
|
||||
&PgTimeout::lock_default(),
|
||||
)
|
||||
.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:?}"),
|
||||
|
||||
@@ -73,8 +73,8 @@ async fn migrate_on_exempt_connection(connection: &mut sqlx::PgConnection) -> Re
|
||||
async fn lift_runtime_timeouts(connection: &mut sqlx::PgConnection) -> Result<()> {
|
||||
crate::apply_runtime_connection_timeouts(
|
||||
connection,
|
||||
crate::TIMEOUT_DISABLED,
|
||||
crate::TIMEOUT_DISABLED,
|
||||
&crate::PgTimeout::disabled(),
|
||||
&crate::PgTimeout::disabled(),
|
||||
)
|
||||
.await?;
|
||||
Ok(())
|
||||
@@ -1174,6 +1174,13 @@ mod tests {
|
||||
.expect("create public schema");
|
||||
}
|
||||
|
||||
/// Parse a literal the tests control, so a typo in one fails loudly here
|
||||
/// rather than silently falling back to the runtime default.
|
||||
fn tight_timeout(raw: &str) -> crate::PgTimeout {
|
||||
raw.parse::<crate::PgTimeout>()
|
||||
.expect("test timeout literal must be valid")
|
||||
}
|
||||
|
||||
async fn applied_versions(pool: &PgPool) -> Vec<i64> {
|
||||
sqlx::query_scalar::<_, i64>(
|
||||
"SELECT version FROM _sqlx_migrations WHERE success ORDER BY version",
|
||||
@@ -1208,9 +1215,10 @@ mod tests {
|
||||
let pool = sqlx::postgres::PgPoolOptions::new()
|
||||
.max_connections(1)
|
||||
.after_connect(|connection, _meta| {
|
||||
Box::pin(crate::apply_runtime_connection_timeouts(
|
||||
connection, TIGHT, TIGHT,
|
||||
))
|
||||
let tight = tight_timeout(TIGHT);
|
||||
Box::pin(async move {
|
||||
crate::apply_runtime_connection_timeouts(connection, &tight, &tight).await
|
||||
})
|
||||
})
|
||||
.connect(&database_url)
|
||||
.await
|
||||
@@ -1336,9 +1344,10 @@ mod tests {
|
||||
sqlx::postgres::PgPoolOptions::new()
|
||||
.max_connections(2)
|
||||
.after_connect(move |connection, _meta| {
|
||||
Box::pin(crate::apply_runtime_connection_timeouts(
|
||||
connection, timeout, timeout,
|
||||
))
|
||||
let timeout = tight_timeout(timeout);
|
||||
Box::pin(async move {
|
||||
crate::apply_runtime_connection_timeouts(connection, &timeout, &timeout).await
|
||||
})
|
||||
})
|
||||
.connect(&database_url)
|
||||
.await
|
||||
|
||||
+22
-195
@@ -3,6 +3,7 @@
|
||||
use std::net::SocketAddr;
|
||||
use std::time::Duration;
|
||||
|
||||
use buzz_db::PgTimeout;
|
||||
use sha2::{Digest, Sha256};
|
||||
use thiserror::Error;
|
||||
use tracing::warn;
|
||||
@@ -102,11 +103,11 @@ pub struct Config {
|
||||
/// Postgres `statement_timeout` for every runtime connection
|
||||
/// (`BUZZ_DB_STATEMENT_TIMEOUT`, e.g. `45s`, `500ms`, `0` to disable).
|
||||
/// Tunable so a backfill or an incident does not need a code change.
|
||||
pub db_statement_timeout: String,
|
||||
pub db_statement_timeout: PgTimeout,
|
||||
/// Postgres `lock_timeout` for every runtime connection
|
||||
/// (`BUZZ_DB_LOCK_TIMEOUT`). Schema migrations always run with both limits
|
||||
/// lifted — see `buzz_db::migration::run_migrations`.
|
||||
pub db_lock_timeout: String,
|
||||
pub db_lock_timeout: PgTimeout,
|
||||
/// Public WebSocket URL of this relay, advertised in NIP-11.
|
||||
pub relay_url: String,
|
||||
/// Public WebSocket URL of the dedicated device-pairing relay, when configured.
|
||||
@@ -306,70 +307,6 @@ fn parse_bind_addr(raw: &str) -> Result<SocketAddr, ConfigError> {
|
||||
.map_err(|e| ConfigError::InvalidBindAddr(e.to_string()))
|
||||
}
|
||||
|
||||
/// 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<u128> {
|
||||
let value = magnitude.parse::<u128>().ok()?;
|
||||
match unit {
|
||||
// Round half up without the `value + 500` intermediate, which overflows
|
||||
// for the top 500 representable microsecond values.
|
||||
"us" => Some(value / 1_000 + u128::from(value % 1_000 >= 500)),
|
||||
"" | "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();
|
||||
};
|
||||
|
||||
let digits = candidate.chars().take_while(char::is_ascii_digit).count();
|
||||
let (magnitude, unit) = candidate.split_at(digits);
|
||||
let unit = unit.trim().to_ascii_lowercase();
|
||||
|
||||
match pg_timeout_millis(magnitude, &unit) {
|
||||
// Return the spelling we validated. PostgreSQL's timeout units are
|
||||
// case-sensitive, so returning `candidate` here would let `45S` pass
|
||||
// validation and then fail every pool's `after_connect`.
|
||||
Some(millis) if millis <= buzz_db::PG_TIMEOUT_MAX_MILLIS => {
|
||||
format!("{magnitude}{unit}")
|
||||
}
|
||||
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()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn positive_u64_from_env(name: &str, default: u64) -> Result<u64, ConfigError> {
|
||||
match std::env::var(name) {
|
||||
Ok(raw) => raw
|
||||
@@ -603,14 +540,10 @@ impl Config {
|
||||
.and_then(|v| v.parse::<u32>().ok())
|
||||
.filter(|&v| v > 0);
|
||||
|
||||
let db_statement_timeout = pg_timeout_or_default(
|
||||
std::env::var("BUZZ_DB_STATEMENT_TIMEOUT").ok().as_deref(),
|
||||
buzz_db::RUNTIME_STATEMENT_TIMEOUT,
|
||||
);
|
||||
let db_lock_timeout = pg_timeout_or_default(
|
||||
std::env::var("BUZZ_DB_LOCK_TIMEOUT").ok().as_deref(),
|
||||
buzz_db::RUNTIME_LOCK_TIMEOUT,
|
||||
);
|
||||
let db_statement_timeout =
|
||||
PgTimeout::from_env_or("BUZZ_DB_STATEMENT_TIMEOUT", PgTimeout::statement_default());
|
||||
let db_lock_timeout =
|
||||
PgTimeout::from_env_or("BUZZ_DB_LOCK_TIMEOUT", PgTimeout::lock_default());
|
||||
|
||||
let relay_url =
|
||||
std::env::var("RELAY_URL").unwrap_or_else(|_| "ws://localhost:3000".to_string());
|
||||
@@ -1131,7 +1064,6 @@ impl Config {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use sqlx::Connection;
|
||||
|
||||
// Mutex to serialize tests that mutate environment variables.
|
||||
// Parallel env-var mutation causes `defaults_are_valid` to see the invalid
|
||||
@@ -1348,117 +1280,6 @@ mod tests {
|
||||
assert_eq!(junk, 50, "unparsable value must fall back to the default");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pg_timeout_accepts_postgres_spellings_and_refuses_the_rest() {
|
||||
for (accepted, canonical) in [
|
||||
("30s", "30s"),
|
||||
("500ms", "500ms"),
|
||||
("0", "0"),
|
||||
("45S", "45s"),
|
||||
("2MIN", "2min"),
|
||||
(" 10 H ", "10h"),
|
||||
] {
|
||||
assert_eq!(
|
||||
pg_timeout_or_default(Some(accepted), "30s"),
|
||||
canonical,
|
||||
"{accepted} is a valid Postgres timeout"
|
||||
);
|
||||
}
|
||||
|
||||
// A rejected value must not reach Postgres: `after_connect` would fail
|
||||
// for every connection, which is worse than the documented default.
|
||||
for rejected in ["", " ", "soon", "30 seconds", "s30", "-5s", "30s;DROP"] {
|
||||
assert_eq!(
|
||||
pg_timeout_or_default(Some(rejected), "30s"),
|
||||
"30s",
|
||||
"{rejected:?} must fall back to the default"
|
||||
);
|
||||
}
|
||||
|
||||
assert_eq!(pg_timeout_or_default(None, "5s"), "5s");
|
||||
}
|
||||
|
||||
/// Every supported unit spelling emitted by the parser must be usable by
|
||||
/// PostgreSQL. A parser-only assertion missed that PostgreSQL rejects
|
||||
/// uppercase units even though validation lowercases them.
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn pg_timeout_canonical_spellings_are_accepted_by_postgres() {
|
||||
let database_url = std::env::var("BUZZ_TEST_DATABASE_URL")
|
||||
.or_else(|_| std::env::var("DATABASE_URL"))
|
||||
.unwrap_or_else(|_| {
|
||||
"postgres://buzz:buzz_dev@localhost:5432/buzz".to_string() // sadscan:disable np.postgres.1
|
||||
});
|
||||
let mut connection = sqlx::PgConnection::connect(&database_url)
|
||||
.await
|
||||
.expect("connect test Postgres");
|
||||
|
||||
for (raw, canonical) in [
|
||||
("500US", "500us"),
|
||||
("500MS", "500ms"),
|
||||
("2S", "2s"),
|
||||
("2MIN", "2min"),
|
||||
("2H", "2h"),
|
||||
("2D", "2d"),
|
||||
("0", "0"),
|
||||
(" 10 S ", "10s"),
|
||||
] {
|
||||
let parsed = pg_timeout_or_default(Some(raw), "30s");
|
||||
assert_eq!(parsed, canonical);
|
||||
buzz_db::apply_runtime_connection_timeouts(&mut connection, &parsed, &parsed)
|
||||
.await
|
||||
.unwrap_or_else(|error| panic!("{raw:?} normalized to {parsed:?}: {error}"));
|
||||
}
|
||||
|
||||
connection.close().await.expect("close test Postgres");
|
||||
}
|
||||
|
||||
#[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",
|
||||
// Parses as u128, so unlike the two above it reaches the unit
|
||||
// conversion — where rounding must not overflow on the way to the
|
||||
// range check.
|
||||
&format!("{}us", u128::MAX),
|
||||
&format!("{}us", u128::MAX - 499),
|
||||
] {
|
||||
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();
|
||||
@@ -1469,27 +1290,30 @@ mod tests {
|
||||
std::env::remove_var("BUZZ_DB_LOCK_TIMEOUT");
|
||||
let defaults = Config::from_env().expect("config");
|
||||
assert_eq!(
|
||||
defaults.db_statement_timeout,
|
||||
defaults.db_statement_timeout.as_str(),
|
||||
buzz_db::RUNTIME_STATEMENT_TIMEOUT
|
||||
);
|
||||
assert_eq!(defaults.db_lock_timeout, buzz_db::RUNTIME_LOCK_TIMEOUT);
|
||||
assert_eq!(
|
||||
defaults.db_lock_timeout.as_str(),
|
||||
buzz_db::RUNTIME_LOCK_TIMEOUT
|
||||
);
|
||||
|
||||
std::env::set_var("BUZZ_DB_STATEMENT_TIMEOUT", "90s");
|
||||
std::env::set_var("BUZZ_DB_LOCK_TIMEOUT", "250ms");
|
||||
let overridden = Config::from_env().expect("config");
|
||||
assert_eq!(overridden.db_statement_timeout, "90s");
|
||||
assert_eq!(overridden.db_lock_timeout, "250ms");
|
||||
assert_eq!(overridden.db_statement_timeout.as_str(), "90s");
|
||||
assert_eq!(overridden.db_lock_timeout.as_str(), "250ms");
|
||||
|
||||
std::env::set_var("BUZZ_DB_STATEMENT_TIMEOUT", "45S");
|
||||
std::env::set_var("BUZZ_DB_LOCK_TIMEOUT", " 2 MIN ");
|
||||
let canonicalized = Config::from_env().expect("config");
|
||||
assert_eq!(canonicalized.db_statement_timeout, "45s");
|
||||
assert_eq!(canonicalized.db_lock_timeout, "2min");
|
||||
assert_eq!(canonicalized.db_statement_timeout.as_str(), "45s");
|
||||
assert_eq!(canonicalized.db_lock_timeout.as_str(), "2min");
|
||||
|
||||
std::env::set_var("BUZZ_DB_STATEMENT_TIMEOUT", "half a minute");
|
||||
let junk = Config::from_env().expect("config");
|
||||
assert_eq!(
|
||||
junk.db_statement_timeout,
|
||||
junk.db_statement_timeout.as_str(),
|
||||
buzz_db::RUNTIME_STATEMENT_TIMEOUT,
|
||||
"a malformed value must not be handed to Postgres"
|
||||
);
|
||||
@@ -1503,11 +1327,14 @@ mod tests {
|
||||
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,
|
||||
out_of_range.db_statement_timeout.as_str(),
|
||||
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);
|
||||
assert_eq!(
|
||||
out_of_range.db_lock_timeout.as_str(),
|
||||
buzz_db::RUNTIME_LOCK_TIMEOUT
|
||||
);
|
||||
|
||||
match previous_statement {
|
||||
Some(value) => std::env::set_var("BUZZ_DB_STATEMENT_TIMEOUT", value),
|
||||
|
||||
Reference in New Issue
Block a user