fix(db): bound runtime Postgres statements and lock waits - #4613
Open
jmecom wants to merge 4 commits into
Open
Conversation
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
marked this pull request as ready for review
August 3, 2026 21:00
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
approved these changes
Aug 4, 2026
elifoster-block
left a comment
There was a problem hiding this comment.
Excluded migrations from the timeouts and set the timeouts as a DBconfig option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change applies a
statement_timeoutandlock_timeoutwhenever 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 EXCLUSIVEwait 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_timeoutbounds heavyweight and row lock waits. Advisory-lock waits — event replacement, the per-community audit lock, sqlx migrations — are bounded bystatement_timeoutinstead.Both values are configurable through
DbConfig, wired toBUZZ_DB_STATEMENT_TIMEOUTandBUZZ_DB_LOCK_TIMEOUT, defaulting to 30s and 5s. An operator running a backfill or working an incident does not need a code change, and0disables a limit. A malformed value falls back to the default with a warning rather than failing config: handing it to Postgres would fail everyafter_connectand take all database access with it.Testing
cargo test -p buzz-db --libat6f8e115: 94 passed, 153 ignored (Postgres)cargo test -p buzz-relay --libat6f8e115: 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 stashedcargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warningsandcargo fmt -- --check: cleangit diff --check origin/main...codex/security-postgres-timeoutsorigin/mainat5c98932Unit tests cover the timeout parser (Postgres spellings accepted; empty, unit-less junk, and
30 secondsrejected in favor of the default) and the env plumbing end to end throughConfig::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_timeoutare the configured values on both the writer and reader poolsmigration_connection_is_unbounded_and_is_retired_not_reused— the migrator's connection reports0for both, and a single-slot pool hands out a freshly configured connection afterwardsmigrations_ignore_runtime_timeouts_and_leak_no_relaxed_session— a realdb.migrate()against a tight-timeoutDbsucceeds, and every connection in the pool still carries the configured limitsThe exemption assertions were checked against a neutered implementation — a no-op lift and a
dropinstead 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--ignoredbuzz-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