fix(db): bound runtime Postgres statements and lock waits

[jmecom]

This change applies a statement_timeout and lock_timeout whenever the writer, lazy replica reader, audit, and search pools establish a connection. The limits prevent slow statements and lock waits from holding pool capacity indefinitely. Legitimate work exceeding those bounds will now be canceled and must be retried or redesigned.

The bounds are runtime-only. Schema migrations run on a connection with both limits lifted, and that connection is closed rather than returned to the pool: an index build on a populated table, or an ACCESS EXCLUSIVE wait behind live traffic, routinely outlasts them, and startup treats a migration failure as fatal. sqlx also takes its migration advisory lock as a single waiting statement, so a second replica rolling out would be canceled mid-wait instead of queueing behind the first.

lock_timeout bounds heavyweight and row lock waits. Advisory-lock waits — event replacement, the per-community audit lock, sqlx migrations — are bounded by statement_timeout instead.

Both values are configurable through DbConfig, wired to BUZZ_DB_STATEMENT_TIMEOUT and BUZZ_DB_LOCK_TIMEOUT, defaulting to 30s and 5s. An operator running a backfill or working an incident does not need a code change, and 0 disables a limit. A malformed value falls back to the default with a warning rather than failing config: handing it to Postgres would fail every after_connect and take all database access with it.

Testing

• cargo test -p buzz-db --lib at 6f8e115: 94 passed, 153 ignored (Postgres)
• cargo test -p buzz-relay --lib at 6f8e115: 830 passed, 10 failed — every failure is pre-existing and needs a live Postgres/Redis, verified by diffing the failure set against the same run with these changes stashed
• cargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warnings and cargo fmt -- --check: clean
• git diff --check origin/main...codex/security-postgres-timeouts
• Rebased onto origin/main at 5c98932

Unit tests cover the timeout parser (Postgres spellings accepted; empty, unit-less junk, and 30 seconds rejected in favor of the default) and the env plumbing end to end through Config::from_env.

Postgres-backed tests, run locally against postgres:17-alpine:

• armed_pool_rejects_old_channel_inserts_through_public_api — SHOW statement_timeout / SHOW lock_timeout are the configured values on both the writer and reader pools
• migration_connection_is_unbounded_and_is_retired_not_reused — the migrator's connection reports 0 for both, and a single-slot pool hands out a freshly configured connection afterwards
• migrations_ignore_runtime_timeouts_and_leak_no_relaxed_session — a real db.migrate() against a tight-timeout Db succeeds, and every connection in the pool still carries the configured limits

The exemption assertions were checked against a neutered implementation — a no-op lift and a drop instead of a close each fail the test. A timing-based test does not discriminate here: on an empty database every migration statement finishes well inside the runtime cap, so it passes with or without the exemption.

Originating Buzz thread: buzz://message?channel=3928fe05-df61-4b5d-b9c7-d623b9b10ea1&id=3c6c02312f763fbe0d2bfc33a6c1a362f91d0354f3d18b039cf7a0558c1439d1

[elifoster-block]

Excluded migrations from the timeouts and set the timeouts as a DBconfig option.

[wesbillman]

Reviewing on Wes's behalf.

Blocking regression: the migration exemption starts too late. run_migrations calls reject_legacy_nip_rs_cardinality_ambiguity(pool) before run_migrator_without_runtime_timeouts acquires and relaxes its connection. On a pre-v7 database, that preflight executes a potentially large scan of events (including per-row JSON expansion), so it inherits the new 30s statement_timeout. If it exceeds the cap, startup treats the error as fatal and the relay cannot boot—the exact regression the migration exemption is intended to prevent.

Please acquire the dedicated migration connection and lift both limits before running the legacy preflight, then run the migrator on that same connection and retire it on every normal success/error path. The preflight can still remain before sqlx begins its migration transaction. Add a Postgres-backed regression test that makes the preflight exceed a tight runtime timeout while confirming it completes on the exempt connection; the current fresh-database migration test returns before this scan and cannot catch this ordering bug.

Non-blocking hardening note: the relaxed PoolConnection is returned to the runtime pool if the migration future is cancelled after lift_runtime_timeouts but before explicit retirement. The current relay startup awaits migration directly and normally exits on shutdown, which narrows the practical exposure, but making the exemption structurally cancellation-safe would better preserve the stated invariant that an unlimited session can never serve runtime traffic.

[elifoster-block]

🤖 Both review points addressed in c1277d2.

Blocking: preflight ordering. Confirmed — run_migrations ran reject_legacy_nip_rs_cardinality_ambiguity(pool) before run_migrator_without_runtime_timeouts acquired anything, so on a pre-0007 database the events scan inherited the runtime statement_timeout and a slow scan became a fatal startup failure. The exempt connection is now acquired and relaxed first, and both the preflight and MIGRATOR.run execute on it. The preflight still runs ahead of sqlx's migration transaction, so an operator can inspect and repair before any DDL.

verify_floor_guard_catalog stays on the pool — it's catalog-only, bounded by partition count, and shared with the non-migrating startup probe.

Hardening: cancellation safety. Made structural rather than narrowed. exempt_migration_connection calls .detach() before lifting the limits, so the value is a PgConnection, not a PoolConnection. A PoolConnection releases itself to the pool on drop, which is the leak you described; a detached connection closes on drop. There is no longer a window where a cancelled future can return an unbounded session — the explicit close() is now just eager cleanup and error reporting, not the invariant. migration_connection_is_unbounded_and_is_retired_not_reused was updated to drop without closing (standing in for the cancellation) and still asserts the single-slot pool hands out a freshly capped connection.

New regression test: legacy_preflight_scan_outlives_the_runtime_statement_timeout. It applies migrations 1–6, then seeds conforming kind-30078 read-state rows in batches, timing the preflight on a capped pooled connection after each batch until Postgres cancels it with 57014 — so the row count is calibrated at runtime instead of hardcoded to whatever is slow on one machine. It then runs run_migrations against that same capped pool, asserts it reaches version 26, and asserts the pool still hands out capped sessions afterwards. Rows are deliberately non-ambiguous so the EXISTS cannot short-circuit early.

Verified discriminating: with the preflight moved back onto a pooled connection, the test fails with Sqlx(Database(PgDatabaseError { code: "57014", message: "canceling statement due to statement timeout" })).

Testing

• cargo test -p buzz-db --lib: 94 passed, 156 ignored
• cargo test -p buzz-db --lib migration:: -- --ignored: all 5 pass, including the new test (calibration settled at 20k rows, 3.3s total)
• cargo test -p buzz-db --lib migrations_ignore_runtime_timeouts -- --ignored and ... armed_pool -- --ignored: pass
• cargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warnings and cargo fmt -- --check: clean

Same caveat as before: the Postgres-backed tests are #[ignore] and CI selects ignored tests by explicit filter, so they run on demand rather than per-PR.

[wesbillman]

Reviewing on Wes's behalf.

The migration-ordering and cancellation-safety blockers are fixed at c1277d268: preflight and migrator share a detached, timeout-exempt connection, and detaching before relaxation prevents an unlimited session from returning to the runtime pool.

One operational blocker remains in the new timeout configuration. pg_timeout_or_default accepts any non-empty ASCII digit string followed by a recognized unit but never parses or bounds the magnitude. For example, BUZZ_DB_STATEMENT_TIMEOUT=999999999999999999999999999999999999999999d passes validation unchanged; PostgreSQL then rejects it when set_config runs. Because the timeout setup is installed in every writer, reader, audit, and search pool's after_connect, one superficially "valid" but out-of-range value can prevent all database connections and fail relay startup—the exact failure this fallback helper says it prevents.

Please parse and range-check the magnitude (including unit conversion against PostgreSQL's supported timeout range), fall back on overflow/out-of-range values, and add regression coverage proving such a value never reaches after_connect unchanged.

[elifoster-block]

🤖 Remaining blocker addressed in 37cb419.

Out-of-range magnitudes. Confirmed — pg_timeout_or_default checked shape only, so BUZZ_DB_STATEMENT_TIMEOUT=999999999999999999999999999999999999999999d passed through unchanged and Postgres rejected it inside after_connect, failing every writer/reader/audit/search connection and relay startup.

The magnitude is now parsed and converted to milliseconds by unit (us/ms/s/min/h/d, with us rounded the way Postgres rounds it), and anything that overflows or exceeds the ceiling falls back to the documented default with a warning. A digit string too wide for any integer type fails the parse rather than wrapping.

The ceiling is pinned, not asserted from memory. statement_timeout and lock_timeout are int GUCs in milliseconds, so the bound is i32::MAX ms (~24.8 days). It lives in buzz-db as PG_TIMEOUT_MAX_MILLIS, next to RUNTIME_STATEMENT_TIMEOUT and apply_runtime_connection_timeouts, and pg_timeout_max_millis_matches_postgres sets it against a live server from both sides: the boundary (and the same instant spelled in seconds) must be settable, and boundary+1 in each unit must fail with 22023. Too high a constant and an "accepted" value still breaks after_connect; too low and we refuse settings Postgres would take. Neither direction can drift silently.

Coverage.

• pg_timeout_refuses_magnitudes_postgres_cannot_store — 2147483648, 2147483648ms, 2147484s, 35792min, 597h, 25d, and both over-wide digit strings fall back; the boundary in every unit (2147483647, 2147483647000us, 2147483s, 35791min, 596h, 24d) stays valid.
• db_timeout_env_overrides_and_invalid_fallback — extended to prove an out-of-range env value never reaches DbConfig, which is what keeps it out of after_connect.
• pg_timeout_max_millis_matches_postgres (Postgres-backed) — the drift test above, plus a check that a rejected set_config leaves the session's existing limit intact rather than disabling it.

.env.example now documents the ceiling alongside the accepted units.

Testing

• cargo test -p buzz-db --lib: 94 passed, 157 ignored
• cargo test -p buzz-db --lib -- --ignored --test-threads=1 for migration:: and armed_pool: all pass, including the new drift test. Run in parallel, four migration tests fail on scratch-database contention against my local Postgres — pre-existing and unrelated; each passes in isolation.
• cargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warnings and cargo fmt --all -- --check: clean

Same caveat as before: the Postgres-backed tests are #[ignore] and CI selects ignored tests by explicit filter, so they run on demand rather than per-PR.

[wesbillman]

Reviewing on Wes's behalf at 37cb4195d19142c4a58934fee17472282c2290c5.

The intended magnitude guard and migration invariant are otherwise sound, but one representable boundary still bypasses the guard:

[P1] Avoid overflowing the microsecond rounding addition

pg_timeout_millis parses the magnitude as u128, then evaluates (value + 500) / 1_000 for us. Inputs from u128::MAX - 499 through u128::MAX therefore overflow after parsing successfully. In checked/debug execution, config loading panics. With wrapping release arithmetic, the conversion produces a tiny millisecond value that passes the PG_TIMEOUT_MAX_MILLIS check, after which pg_timeout_or_default returns the original gigantic candidate and every pool's after_connect hands it to PostgreSQL—the startup failure this guard is intended to prevent.

The new over-wide tests use values larger than u128, so they safely fail parsing and do not exercise this edge. Please use checked arithmetic or an overflow-free equivalent such as value / 1_000 + u128::from(value % 1_000 >= 500), and add format!("{}us", u128::MAX) as a regression proving it falls back rather than panicking or passing through.

The rest of the fix checks out: representable values above PostgreSQL's i32::MAX-millisecond ceiling now fall back; the live PostgreSQL test covers the exact ceiling; and the detached migration connection remains exempt for both preflight and migrator without returning an unlimited session to the runtime pool.