Skip to content

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

Open
jmecom wants to merge 4 commits into
mainfrom
codex/security-postgres-timeouts
Open

fix(db): bound runtime Postgres statements and lock waits#4613
jmecom wants to merge 4 commits into
mainfrom
codex/security-postgres-timeouts

Conversation

@jmecom

@jmecom jmecom commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

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_apiSHOW 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.

Still unverified: these three are #[ignore = "requires Postgres"] and CI's ignored-test jobs select by explicit filter, so they run only when someone runs them by hand. The full --ignored buzz-db suite was not run to completion locally (it exceeds a 10-minute budget); the three above were run individually.

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

jmecom added 2 commits August 3, 2026 13:44
Set a 30-second statement timeout and 5-second lock timeout on writer, reader, audit, and search pool connections. Verify the effective session settings alongside the existing writer guard.

Co-authored-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
@jmecom
jmecom marked this pull request as ready for review August 3, 2026 21:00
@jmecom
jmecom requested a review from a team as a code owner August 3, 2026 21:00
elifoster-block and others added 2 commits August 4, 2026 12:07
Schema migrations now run on a connection with both limits lifted. An
index build on a populated table, or an ACCESS EXCLUSIVE wait behind live
traffic, routinely outlasts the runtime caps, and startup treats a
migration failure as fatal — so inheriting them turned a slow migration
into a relay that cannot boot. sqlx also takes its migration advisory
lock as one waiting statement, so a second replica rolling out would be
canceled mid-wait instead of queueing. The connection is closed rather
than returned to the pool, since its session still carries no limits.

The two values move to DbConfig, wired to BUZZ_DB_STATEMENT_TIMEOUT and
BUZZ_DB_LOCK_TIMEOUT with the previous constants as defaults, so a
backfill or an incident does not need a code change. A malformed value
falls back to the default with a warning: handing it to Postgres would
fail every after_connect and take all database access with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Eli Foster <efoster@squareup.com>
Split the exemption into `lift_runtime_timeouts` and `retire_connection`
so both halves are observable, and cover them: the migrator's connection
reports both limits as `0`, and a single-slot pool hands out a freshly
configured connection afterwards rather than the relaxed session.

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

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Eli Foster <efoster@squareup.com>

@elifoster-block elifoster-block left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants