Skip to content

fix(db): keep the reader pool warm and drop its pre-acquire ping - #8495

Open
TheSentinel454 wants to merge 6 commits into
mainfrom
elrond/reader-pool-warm
Open

TheSentinel454 wants to merge 6 commits into
mainfrom
elrond/reader-pool-warm

Conversation

@TheSentinel454

@TheSentinel454 TheSentinel454 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The reader pool's 150ms acquire_timeout covers 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:

  • abandons in-flight dials mid-handshake (Postgres logs could not accept SSL connection: EOF detected),
  • closes healthy idle connections whose ping overran instead of returning them to the pool, and
  • falls every miss back to the writer, which then exhausts its own pool.

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)

  • Warm floor: reader min_connections is now max / 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's max_connections.
  • Floor fills at boot: with max_lifetime/idle_timeout set, a lazy sqlx 0.9 pool does not dial min_connections at 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's BEGIN … READ ONLY already 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.
  • Bounded fence proof: with no pre-acquire ping, the proof (Aurora capability probe, BEGIN, heartbeat observation) is the first round trip on a checkout. It now runs under READER_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 reason reader_proof_timeout instead 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 its BEGIN is acknowledged and the transaction rolls back normally. Dropping it mid-BEGIN would return the connection to the pool idle in transaction (sqlx counts the transaction only after BEGIN is acknowledged, so a cancelled begin queues no ROLLBACK). 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_connections maintenance rather than re-implementing it. No new config knob. The reader test proxy now lives once in test_support instead of inside store/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 connection idle 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

  • Postgres tests (runtime/tests/reader_pool_postgres_tests.rs):
    • the boot ping leaves the reader pool at exactly its floor, all idle (base leaves one connection);
    • a warm reader whose heartbeat read blocks (exclusive lock) falls back to the writer as reader_proof_timeout within one proof budget;
    • a warm reader connection terminated server-side falls back as reader_validation_error with no client error;
    • a proof that times out mid-BEGIN on a paused reader (test proxy that holds bytes, like docker pause) falls back as reader_proof_timeout and, once the reader resumes, leaves no idle in transaction reader backend. This test fails with the runtime fix reverted.
  • Unit tests: min-connections mapping, reader pool options, construction still dials nothing.
  • Full gate (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.

Elrond added 2 commits October 20, 2026 16:17
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>
@TheSentinel454
TheSentinel454 force-pushed the elrond/reader-pool-warm branch from b26bc2c to fc1763e Compare October 20, 2026 20:17
Elrond added 2 commits October 20, 2026 16:34
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>
@TheSentinel454
TheSentinel454 marked this pull request as ready for review October 20, 2026 20:51
@TheSentinel454
TheSentinel454 requested a review from a team as a code owner October 20, 2026 20:51
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: a93f3282cc017e8decc8ea803d92967c4c3e36df...b4bd073abf5c9ee685850b556ac9435f630f8959
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: MEDIUM

Timed-out serving proofs can strand reader-pool connections and prevent recovery after replica failover.

Findings

[MEDIUM] Bound connection cleanup for detached serving proofs

  • Category: Reliability
  • Location: crates/buzz-db/src/runtime/mod.rs:1320 (source)
  • Description: Serving reads use SubscriptionHistory, for which bounded_release() is false. After the one-second timeout, this detached proof therefore retains its checked-out connection without an application deadline. With the acquisition ping now disabled, established connections that become blackholed during replica failover reach this branch instead of being discarded during acquisition. If these proofs occupy every reader slot, the pool cannot connect to a recovered endpoint until the original sockets error; the warm floor cannot replace connections that remain checked out.
  • Impact: Replica offloading can remain unavailable after the reader endpoint recovers, forcing serving traffic onto the writer and risking writer-pool exhaustion.
  • Recommendation: Ensure timed-out serving proofs close their connections and release pool slots within a bounded deadline while preserving protection against cancellation during BEGIN. Cover recovery where existing sessions remain blocked but new connections can succeed.

Notes

  • Review used read-only static inspection. Builds and tests were not executed.

Generated by Codex Security Review |
Requested by: @TheSentinel454 |
Workflow run

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>
@github-actions github-actions Bot added codex-security-review-current The posted Codex security review matches its recorded range. and removed codex-security-review-current The posted Codex security review matches its recorded range. labels Oct 20, 2026
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>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 20, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 [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.

@wpfleger96 wpfleger96 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 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_release spawns the proof (crates/buzz-db/src/runtime/mod.rs:1319-1321). That covers SubscriptionHistory and Authorization (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 until BEGIN is answered or the socket errors, as the comment at :1315-1316 says. Nothing on the application side bounds that. connect_read_pool sets no keepalive or socket timeout (:983-992), and sqlx's max_lifetime/idle_timeout reaper and min_connections maintenance 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 BEGIN reply, and don't drop the future either, since dropping it brings back the idle in transaction leak. 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 through close_on_drop.
    • Test: none of the current tests cover this. proof_timeout_mid_begin_leaves_no_reader_transaction_open resumes the proxy before it checks the pool (runtime/tests/reader_pool_postgres_tests.rs:263-280). The maintenance test in store/usage.rs uses close_on_drop. PausingProxy also 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.

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_pool doc 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.

This branch was successfully deployed

1 active deployment
codex-review — b4bd073a Deployed Oct 20, 2026 by TheSentinel454 via Run Codex Security Review #9275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants