Repository navigation
fix(db): keep the reader pool warm and drop its pre-acquire ping - #8495
TheSentinel454 wants to merge 6 commits into
Conversation
The reader pool's 150ms acquire budget covers both the idle-connection ping and any new-connection dial. When readers slow down, the timeout abandons dials mid-TLS, closes healthy idle connections whose ping overran, and moves every read onto the writer. In production on 2026-10-19 and 2026-10-20 this showed up as thousands of "could not accept SSL connection: EOF detected" per second on both readers, followed by writer pool exhaustion. - Reader min_connections is now max/5 (at least 1). sqlx re-dials that floor in the background, under its own deadline rather than the request budget, whenever a connection is reaped or closed. - The boot ping fills the floor, retrying acquire timeouts for up to 5s, so new pods do not serve their first reads from an empty pool. - test_before_acquire(false) on the reader: the routed read's BEGIN already proves the connection and fails closed to the writer, and sqlx still pings connections on release. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
… maintenance Review follow-ups on the reader warm floor: - With no pre-acquire ping, the fence proof (Aurora probe, BEGIN, heartbeat observation) is the first round trip on a checkout. Bound it with READER_PROOF_TIMEOUT (1s) so a reader that stops answering without closing its sockets fails closed to the writer as reader_proof_timeout instead of hanging the request until TCP gives up. - The boot ping no longer holds a vector of checkouts under the 150ms acquire budget. It closes its probe connection, which makes sqlx dial the floor in background maintenance under its own deadline. Probe acquire timeouts retry with bounded backoff. - Move the reader-pool Postgres tests to their own module, poll for the asynchronous release instead of asserting before it lands, and cover the proof timeout and a server-terminated warm connection. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
b26bc2c to
fc1763e
Compare
abandoned_fleet_collection_releases_its_reader_slot abandons the collection with a 1s caller deadline, which now races the reader's 1s fence-proof timeout; the proof can win and fail closed first. Use a 500ms caller deadline so the test still pins close-on-drop on caller abandonment. Also apply rustfmt to the new reader-pool test module. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
🔐 Codex Security Review
Review SummaryOverall Risk: MEDIUM
Findings[MEDIUM]
|
Dropping the fence proof at READER_PROOF_TIMEOUT while its BEGIN was in flight leaked a server-side transaction: sqlx bumps the transaction depth only after BEGIN is acknowledged, so the cancelled begin queued no ROLLBACK, and the release ping consumed the late BEGIN reply and returned the connection to the pool idle in transaction. On expiry the proof is now spawned to completion off the request path, so BEGIN finishes and the transaction drops through sqlx's normal rollback. Bounded-release checkouts are still dropped, since close-on-drop leaves nothing to leak. The request path stays bounded at the proof timeout. The reader test proxy moves to test_support and now pauses (holds and later delivers bytes, like docker pause) so the new Postgres test can resume a reader mid-BEGIN. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
The pausing proxy resumes the stalled session, so asserting after resume let the ordinary release ping free the slot and a removed close_on_drop survived. Poll the pool size to zero while the reader is still paused, then resume and prove a replacement connection works. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/operator/docs/publication) reviews.
Reviewed head b4bd073, base cdc2b12; PR merge base fcdabcf.
The existing detached-proof recovery finding remains actionable at this head. With pre-acquire ping disabled, idle connections whose old endpoint stops answering enter the proof and remain checked out after its timeout. If all reader slots are retained, a healthy replacement endpoint cannot be reached until the old sockets fail. The request deadline is bounded, but cleanup is not. Bound connection disposal while preserving safe handling of an unacknowledged BEGIN; validate recovery with old sessions permanently stalled while new connections can succeed. This confirms the existing report rather than adding a duplicate inline finding. Exact recovery time depends on socket settings and was not measured.
One new P3 publication finding is inline. No additional distinct production defect established.
Optional follow-up: the boot warm-up still uses the 150ms serving acquire budget for every retry (runtime/mod.rs:1043–1058). If a cold handshake consistently takes longer, the probe exits before the close that triggers floor maintenance; an empty SQLx pool’s reaper has nothing to reap. This leaves part of the original slow-start problem unresolved, rather than introducing a new regression. Clarify that limitation in the tradeoffs, and consider a separately bounded background bootstrap budget. Also document the new floor/reader_proof_timeout for operators and replace the future-tense testing note with current evidence.
Source-only: inspected production paths, regression-test source, matching cached SQLx 0.9 source, and the public description (no attached images). No PR code, tests, builds or apps executed. One read-only CI snapshot showed successful executed checks and skipped jobs, including successful PostgreSQL and backend integration checks. Production failover, delayed handshakes, pool sizing and native client failure/retry behavior were not exercised. Minimalism 9/10; elegance 9/10; correctness 8/10 pending the linked cleanup defect. Non-blocking COMMENT review; humans decide merge readiness.
| /// the read falls back to the writer, and the next read dials again; under | ||
| /// a fleet-wide burst that loop turned brief reader slowness into a writer | ||
| /// pool exhaustion (production, 2026-10-19 and 2026-10-20: thousands of | ||
| /// `could not accept SSL connection: EOF detected` per second on both |
There was a problem hiding this comment.
🤖 [P3] Generalize deployment details in the public material
This source comment includes incident-specific deployment context, and the PR description’s Summary and “Warm floor” bullet add actual fleet sizing and replica topology. Keep the useful failure mechanism and sizing formula, but remove actual deployment configuration from these public surfaces. For example: “The warm floor is one fifth of the configured reader pool size (at least one) per relay instance; check aggregate standing connections against the reader’s max_connections.” Describe the abandoned-handshake failure generically in this comment. This is a public-material cleanup, not a credential finding.
There was a problem hiding this comment.
🤖 Automated review of b4bd073abf5c9ee685850b556ac9435f630f8959 (base cdc2b129c006755bd923e75462c610df2815b383): one blocking issue. The warm floor, the boot fill through sqlx maintenance, dropping the pre-acquire ping and the 1s proof bound on the request path all look right. Detaching the timed-out proof also fixes the idle in transaction leak from cancelling mid-BEGIN. The blocker is what happens to the reader slot after a serving proof times out.
Blocking
- A replica that stops answering can keep the reader pool full after a healthy replica is reachable. On timeout, every operation without
bounded_releasespawns the proof (crates/buzz-db/src/runtime/mod.rs:1319-1321). That coversSubscriptionHistoryandAuthorization(runtime/observability.rs:102-112), so every serving read. The detached task still owns the checked-out connection, so the slot counts toward pool size untilBEGINis answered or the socket errors, as the comment at:1315-1316says. Nothing on the application side bounds that.connect_read_poolsets no keepalive or socket timeout (:983-992), and sqlx'smax_lifetime/idle_timeoutreaper andmin_connectionsmaintenance only see idle connections and total size. Here's the failure: the replica endpoint fails over and the old sockets stop answering without closing. Each serving read checks out one of the warm connections, times out after 1s, and leaves it stuck in a detached task. Once every slot is held, acquires time out at 150ms and every read goes to the writer. The new endpoint would accept connections, but the pool has no free slot to dial it. That lasts until the kernel gives up on the old sockets, so minutes, not seconds, and it puts the writer under the same load this PR is meant to prevent. On base, the pre-acquire ping ran inside the acquire timeout, so a stalled idle connection was dropped and its slot freed at acquire time. Without that ping, the proof also has to bound disposal, not just request latency.- Fix: when a serving proof times out, close the connection and free its slot within a bounded time. Don't wait for the
BEGINreply, and don't drop the future either, since dropping it brings back theidle in transactionleak. Closing the socket instead of returning it gets both: the slot is freed and the server ends the transaction when the session goes away. One way is a bounded detach that closes the connection (close/close_on_drop) instead of letting it go back to the pool if the proof hasn't finished by a second deadline. Maintenance reads already avoid this throughclose_on_drop. - Test: none of the current tests cover this.
proof_timeout_mid_begin_leaves_no_reader_transaction_openresumes the proxy before it checks the pool (runtime/tests/reader_pool_postgres_tests.rs:263-280). The maintenance test instore/usage.rsusesclose_on_drop.PausingProxyalso stalls new connections. A regression test needs a proxy that keeps already-open sessions stalled but lets new ones through. Fill a small serving pool with stalled sessions, then assert a later routed read is served by the reader within a bound.
- Fix: when a serving proof times out, close the connection and free its slot within a bounded time. Don't wait for the
Optional
- Boot fill is conditional. Every boot retry still gets the 150ms acquire budget (
runtime/mod.rs:1043-1058). So if cold handshakes consistently take longer than that, all five attempts fail and the close that starts floor maintenance never runs. That leaves part of the slow-start problem in place. Either a separate, longer boot-only acquire budget or a note in the tradeoffs would cover it. - The
connect_read_pooldoc says sqlx re-dials the floor "under sqlx's own 300s maintenance deadline" whenever a connection is reaped, closed on drop or fails its release check (runtime/mod.rs:968-971). 300s is the deadline for the close-on-drop path. Reaper-triggered maintenance uses the reaper's next-run deadline instead, so the sentence is only true for some of those triggers.
The new reader-pool Postgres tests pass at this head in hosted CI and locally, as do PostgreSQL Domain, Backend Integration and Relay E2E. I also reproduced the blocker live against a disposable Postgres, using a throwaway test with a proxy that keeps already-open sessions stalled but lets new ones through. Reader pool floor 1, max 2. After the warm connections stalled, two count_events_routed calls fell back to the writer on reader_proof_timeout (1,023ms and 1,005ms). Then new connections were allowed, and a fresh connection through the same proxy ran SELECT 1 fine. Even so, all 12 serving reads over the next 10s went to the writer on reader_acquire_timeout, none went to the replica, and the pool stayed at size 2 / idle 0 the whole time. I didn't get a comparison run at the merge base. This matches the open Codex security comment and Wes's review.
Summary
The reader pool's 150ms
acquire_timeoutcovers the whole acquire: the pre-acquire idle-connection ping and any new-connection dial (TCP + TLS + SCRAM). When readers slow down for a moment, the timeout:could not accept SSL connection: EOF detected),In production on 2026-10-19 and 2026-10-20 this showed up as thousands of abandoned handshakes per second on both readers, starting in the same second on nearly every relay pod, followed ~13s later by writer pool-acquire timeouts and a connection spike. The database itself logged no statement/lock timeouts or connection-limit errors before the burst. The cascade was the client tearing down its own reader connections.
This PR removes that amplifier. It does not change the request budget, writer fallback, or the writer pool.
Change (
crates/buzz-db/src/runtime/mod.rs)min_connectionsis nowmax / 5, at least 1, derived from the existing reader pool size (no new config knob). sqlx re-dials the floor in background maintenance, under its own 300s deadline rather than the request budget, whenever a connection is reaped or closed. At the current reader pool size of 100 that is 20 standing connections per pod, so roughly 20 × pod count per reader endpoint (about 340 at 17 pods), to check against the instance'smax_connections.max_lifetime/idle_timeoutset, a lazy sqlx 0.9 pool does not dialmin_connectionsat construction. The existing boot ping now closes its probe connection instead of returning it, which triggers sqlx's floor maintenance. Probe acquires that overrun 150ms retry with bounded backoff (250ms, 500ms, 1s, 2s). The doc comment that said sqlx dials the minimum eagerly was wrong for this config and is corrected.test_before_acquire(false)on the reader: the routed read'sBEGIN … READ ONLYalready proves the connection and fails closed to the writer (reader_validation_error). sqlx still pings connections on release, so dead connections are still filtered. This removes one round trip per checkout and the warm-connection churn.BEGIN, heartbeat observation) is the first round trip on a checkout. It now runs underREADER_PROOF_TIMEOUT(1s), so a reader that stops answering without closing its sockets (partition, host loss) fails closed to the writer as the new route reasonreader_proof_timeoutinstead of hanging the request until TCP gives up. 1s is well above a normal round trip, so a brief reader stall waits on the reader rather than flooding the writer. A proof that expires is detached, not dropped: it runs to completion off the request path so itsBEGINis acknowledged and the transaction rolls back normally. Dropping it mid-BEGINwould return the connection to the poolidle in transaction(sqlx counts the transaction only afterBEGINis acknowledged, so a cancelled begin queues noROLLBACK). Bounded-release checkouts are still dropped, because close-on-drop leaves nothing to leak.What got simpler
The reader checkout path does one less round trip and no longer closes healthy connections on a slow ping. Boot warm-up reuses sqlx's own
min_connectionsmaintenance rather than re-implementing it. No new config knob. The reader test proxy now lives once intest_supportinstead of insidestore/usage.rs.Tradeoffs: the reader is now dialed at boot (previously "no reader dial at boot"); if the reader is down at boot this only produces log noise and reads still fail closed to the writer. On a proof timeout the detached proof keeps its connection until the reader answers (then it rolls back and returns to the pool) or the socket errors; against a fully unresponsive reader that holds a pool slot, after which acquires time out at 150ms and fall back as before. A caller that drops a routed read mid-
BEGIN(client disconnect) can still leave a connectionidle in transaction; that predates this PR and is rare, and is left as identified debt.Related issue
N/A (production incident follow-up; none found).
Testing
runtime/tests/reader_pool_postgres_tests.rs):reader_proof_timeoutwithin one proof budget;reader_validation_errorwith no client error;BEGINon a paused reader (test proxy that holds bytes, likedocker pause) falls back asreader_proof_timeoutand, once the reader resumes, leaves noidle in transactionreader backend. This test fails with the runtime fix reverted.just fmt-check,just clippy, Postgres lane,just test-unit,just test) on a Blox workstation at the head SHA; results are posted before this leaves draft.