Lower readiness connect_timeout so wait_for_ready fails fast on a hung backend#54
Open
allansimon-mistral wants to merge 1 commit into
Open
Conversation
…g backend wait_for_ready retries 15 times, each using the engine's connect_args connect_timeout=60, so a backend that accepts TCP but never completes the connection handshake stalls readiness for ~15 minutes before failing. connect_timeout only bounds connection establishment (not query/DDL time, despite the old comment), and a local PGlite socket connects in milliseconds, so 60s only makes the failure case pathological. Lower it to 10s in both SQLAlchemy managers; healthy connects are unaffected and the 15 retries still tolerate a slow cold start.
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.
Small fix (two lines per file): lower the readiness
connect_timeoutfrom 60s to 10s in the two SQLAlchemy managers, so a hung backend failswait_for_readyin a couple of minutes instead of about 15.Problem
When a PGlite instance's socket accepts a TCP connection but its Postgres backend never completes the connection handshake,
SQLAlchemyAsyncPGliteManager.wait_for_ready()retries the connectionmax_retries=15times, and each attempt uses the engine'sconnect_args={"connect_timeout": 60, ...}. So a single unreachable backend blockswait_for_readyfor up to 15 x 60s, about 15 minutes, before it finally fails withpsycopg.errors.ConnectionTimeout.The 60s value is also mislabeled. The comment reads
# Much longer timeout for table creation, but psycopg'sconnect_timeoutbounds only connection establishment, not query or DDL execution. A local PGlite socket connects in milliseconds, so 60s does nothing for healthy connects and only makes the failure case pathological.This is reachable in normal use:
PGliteManager.start()reports the server ready as soon as the TCP port accepts a socket, so a backend that is listening but not yet serving (which happens under concurrent cold-starts, when several instances boot at once and starve the single-threaded node/WASM backend) gets past startup and intowait_for_ready, where the 15 x 60s stall then happens.Fix
Lower the default
connect_timeoutfrom 60 to 10 in both the sync and async SQLAlchemy managers, and correct the comment. Withmax_retries=15, worst-case readiness is now bounded at roughly 15 x 10s instead of 15 x 60s, so a stuck or unreachable backend fails in a reasonable time. Healthy connects are unaffected (they complete in well under 10s), and the 15 retries still tolerate a slow-but-healthy cold start.The change is two lines (the
connect_timeoutvalue and its comment) in each ofsrc/py_pglite/sqlalchemy/manager.pyandsrc/py_pglite/sqlalchemy/manager_async.py.Why not tighten start() instead
Making
start()'s readiness stricter is the tempting fix, since it is what reports a hung backend as ready. A driver-free probe there (for example sending a Postgres StartupMessage over the raw socket) does detect the hang, but PGlite's socket server handles one connection at a time, and a probe that begins a handshake and then disconnects leaves that single connection in a bad state and breaks the next real connection. So the safe place to bound this iswait_for_ready, which already opens a real, complete driver connection.Scope
This does not fix the underlying hang (socket accepts, backend never answers under concurrent cold-starts); it makes py-pglite fail fast and clearly instead of stalling for about 15 minutes when that, or any unreachable backend, occurs.
Related
#55 tracks the underlying hang this fails fast on, which appears fixed by #52.