Repository navigation
fix(pool): bound return_to_pool so a dead connection can't leak its permit - #4350
rslowinski wants to merge 1 commit into
Conversation
|
Sorry about the noise, agent went a bit wild, I reigned it in and rewrote / went over this comment by hand. We hit the same failure in production: a server restart during a query execution causes the query to hang forever. We noticed this because we have a ~ hot loop that is constantly hitting postgres, so we have a high chance of having an in-flight query where the timing of an upstream restart causes it to hang. A gap found working on #4354 (now closed in favor of this PR):
The The fix is mechanical: give each the same bound and fall back to async fn close_bounded(self) {
let _ = crate::rt::timeout(RETURN_TO_POOL_PING_TIMEOUT, self.close()).await;
} |
Follow-up to the previous commit (transact-rs#4350), which bounds the on-release `ping()`. `return_to_pool` holds the pool permit across two more unbounded awaits on the same connection: - the `after_release` hook, which typically runs a query on the socket that may be dead; - `close()` on the pool-closed and beyond-`max_lifetime` paths. `close()` writes `Terminate` and waits for the peer to drop the connection, which a silent peer never does. We set `max_lifetime`, so the second path runs for every aged-out connection and leaks its permit exactly like the ping case. Offered upstream in transact-rs#4350 (comment).
Follow-up to the previous commit (transact-rs#4350), which bounds the on-release `ping()`. `return_to_pool` holds the pool permit across two more unbounded awaits on the same connection: - the `after_release` hook, which typically runs a query on the socket that may be dead; - `close()` on the pool-closed and beyond-`max_lifetime` paths. `close()` writes `Terminate` and waits for the peer to drop the connection, which a silent peer never does. We set `max_lifetime`, so the second path runs for every aged-out connection and would leak its permit exactly like the ping case. Offered upstream in transact-rs#4350 (comment).
abonander
left a comment
There was a problem hiding this comment.
It would be better to wrap the entire return_to_pool call in the timeout just to insure against stuff like close_hard deadlocking (though it really shouldn't).
I already have this fix in #3582 but that won't be merged until 0.10 at the earliest.
…ermit When a PoolConnection is dropped, the spawned return-to-pool task holds the connection's permit while it runs the after_release hook, pings, or closes the connection. Against a peer that vanished silently (no RST/FIN) these awaits never complete, so the permit is never released; after max_connections such drops every acquire() fails with PoolTimedOut. Bound the whole return_to_pool call. On timeout the Floating is dropped, which releases the permit and closes the socket locally. Fixes transact-rs#4349.
449f729 to
60772eb
Compare
|
Done: the timeout now wraps the whole |
…G switchover (#14706) Windmill job execution went down instance-wide ~55 min on 2026-10-03 after a CNPG postgres-windmill primary switchover: the client-side sqlx connection pools in windmill-app / workers-default / workers-native wedged into permanent PoolTimedOut and never self-healed. DB was healthy; manual rollout restart of all three deployments fixed it. Recurs on EVERY future switchover. Research finding: there is NO config knob to prevent the wedge. Windmill hardcodes the pool's max_lifetime(30m) / idle_timeout / acquire_timeout in backend/src/db_connect.rs; only DATABASE_CONNECTIONS (max pool size) is env-configurable, and DATABASE_URL query params cannot set sqlx pool options. The chart's app liveness probe is hardcoded to /api/version (no DB touch) with no values override, and worker pods have no liveness probe at all, so the in-chart self-healing path is unavailable. Root cause is an sqlx permit leak (transact-rs/sqlx#4350), not a Windmill misconfiguration. So: detect fast and remediate by restart. Adds a blackbox http_2xx Probe against windmill-app /api/health/status?force=true — an unauthenticated endpoint that runs a DB query THROUGH THE SAME POOL and returns 503 on a wedge (backend/windmill-api/src/health.rs), so probe_success goes to 0 within ~1 min. Pairs it with a WindmillPoolWedged critical alert (for:3m) whose runbook is the exact three-deployment rollout restart. This is the same wedge-detector pattern already used for ollama / tei / vllm. Blast radius: zero cluster mutation. Adds one Probe CR and one alert rule; no CNP change needed (blackbox egress is toEntities:cluster, windmill ingress is default-allow). No windmill pod restart to apply. Alert surfaces the wedge in ~3 min vs the ~15-25 min of the existing job-presence watchdog. Refs: reference_windmill_sqlx_pool_wedge_after_cnpg_switchover Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #4349.
Problem
When a
PoolConnectionis dropped, the spawned return-to-pool task holds the connection's permit while it runs theafter_releasehook,pings, orcloses the connection. Against a peer that vanished silently (no RST/FIN: NAT/firewall flow expiry, server restart mid-query, abandonedtokio::time::timeout) these awaits never complete. The permit is never released, and aftermax_connectionssuch drops everyacquire()fails withPoolTimedOutuntil the process restarts. Nothing logs, because nothing errors.Fix
Per review, the whole
Floating::return_to_poolcall is wrapped in a 5s timeout (sibling ofCLOSE_ON_DROP_TIMEOUT), which also covers theafter_releaseandmax_lifetime/pool-closedclose()paths raised in the thread. On timeout theFloatingis dropped: itsDecrementSizeGuardreleases the permit and the socket closes locally, so nothing inside (includingclose_hard) can pin the permit.Test
tests/postgres/pool.rsruns a realPgPoolagainst an in-process fake server that finishes the startup handshake and then goes silent. It drops a checked-out connection and asserts the nextacquire()opens a fresh connection instead of hittingPoolTimedOut. Needs noDATABASE_URL; gated toruntime-tokio. Fails onmain(PoolTimedOut at 20s), passes with the fix (~5s).