Repository navigation
1.0.6: connection-lifecycle hardening - #25
Merged
Merged
Conversation
read_buf returns Ok(0) on peer EOF, which is not an error. The startup, upstream-auth (cleartext/MD5/SCRAM), post-auth, pooled reset (DISCARD ALL), context-injection, pool-connection-create, and checkin-reset loops all treated Ok(0) as "no data yet" and looped again, so a client or upstream that closed mid-handshake spun the connection task at ~100% CPU until the handshake timeout (30s) or reset timeout (5s) fired, logging a spurious per-connection "handshake timeout". Route every handshake-phase read loop through a single stream::read_or_eof helper that maps Ok(0) to UnexpectedEof, mirroring the steady-state pipe, which already handled EOF since #11. Verified red-then-green (tests/eof-storm.mjs, Suite 11): 15 connect-and-close events with a 1s handshake timeout burned ~8 CPU-seconds and logged 16 handshake timeouts on 1.0.5; 0 and 0 after the fix. Needs no Postgres. Adds docs/architecture-review-lifecycle.md mapping the register to one class.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The admin API bound 0.0.0.0 unconditionally, exposing the unauthenticated /status and /metrics endpoints — which reveal pool topology (every database, role, and bucket count) — on every interface. On a multi-homed host this leaked the tenant/role namespace to anything that could route to the port. Add an admin_host config (CLI --admin-host, file admin_host, env PGVPD_ADMIN_HOST) defaulting to 127.0.0.1, and bind it instead of 0.0.0.0. Wider exposure is now deliberate opt-in. Verified red-then-green (tests/admin-bind.mjs, Suite 12): on main the admin port is reachable over a non-loopback address (10.x here); fixed, it is reachable only on 127.0.0.1, and PGVPD_ADMIN_HOST=0.0.0.0 re-opens it. Needs no Postgres. Updates threat-model.md and adversarial-test-plan.md to mark #13 fixed. Pre-existing since the admin API (0.5).
solidcitizen
changed the title
added 5 commits
September 18, 2026 20:13
A pooled connection whose upstream had gone away (Postgres restart, failover, or
an admin terminating the backend) failed the next client to check it out: the
DISCARD ALL reset hit a dead socket or a FATAL from the terminating backend and
surfaced as a connection error. After an upstream bounce, one client was
victimized per stale pooled connection until the bucket cleared.
Checkout now resets the connection through reset_pooled and, if that fails for
any reason (dead socket, EOF, or an ErrorResponse — a reset never legitimately
fails on a reusable connection), discards it and retries with a fresh one,
bounded to 3 attempts, transparent to the client. If the upstream is genuinely
down, create_connection fails and that error is surfaced instead of looping.
Verified red-then-green (tests/drizzle/upstream-restart.mjs, Suite 13): warm the
pool, kill pgvpd's upstream backends via pg_terminate_backend, reconnect. Without
the fix the first reused checkout fails ("DISCARD ALL failed: terminating
connection"); with it all post-kill clients recover with correct tenant context.
Pre-existing since pooling (0.3).
…14) Pool mode previously dropped every CancelRequest and handed all clients on a bucket the same cached upstream BackendKeyData, so cancellation was a no-op and a naive forward would have hit an arbitrary tenant's query. Each client is now handed pgvpd's OWN minted (pid, secret). A CancelRegistry maps that key to the client's current upstream connection's real backend key for the life of the pipe (removed via an RAII guard on end/cancel). A client CancelRequest is looked up and forwarded to that upstream connection alone; an unknown or stale key is a safe no-op. Each PooledConn now preserves its real backend key across checkin/checkout so the route survives connection reuse. #14: when a client drops an in-flight query, checkin cancels the orphaned query upstream (via the connection's real key) before draining, so the slot is reclaimed promptly instead of waiting out the drain timeout. Verified red-then-green: - Suite 14 (cancel-isolation.mjs): two tenants, cancel one -> only its query aborts (57014), the other completes. Minted keys are unique per client. - Suite 15 (orphan-cancel.mjs): client drops mid-query -> orphan cancelled upstream (active before=1, after=0); no-op without the fix. Plus unit tests in cancel.rs and protocol.rs. Full suite 58/58. Ships in 1.0.6 with #24, #13, #15.
Cleanup items from NexusPlus's hostile round on the 1.0.3 RC:
- pgvpd_pool_creates_total counted attempts, not successes: the counter was bumped
before create_connection, so a failed connect inflated it. Increment on success.
- The checkin reset's timeout path was folded into a generic "reset failed or
timed out" log. Distinguish a reset that timed out ("reset timed out") from one
that failed ("reset failed") so a drain/reset timeout is clearly visible.
- CI clippy did not lint tests or benches. Switch CI to
`cargo clippy --all-targets -- -D warnings` and clear the pre-existing backlog
(needless range loop in the bench; byte-str literals, field-reassign-default,
and get().is_none() in test code). All fixes are test/bench-only and
behavior-preserving.
cargo fmt, cargo clippy --all-targets -- -D warnings, and 100 unit tests all pass.
Ships in 1.0.6.
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.
1.0.6 — connection-lifecycle hardening. Root-cause fixes for the bug class mapped in
docs/architecture-review-lifecycle.md, each verified red-then-green with a regression suite. Full integration suite: 58/58; clippy--all-targets(rust 1.98), fmt, 100 unit tests all green.Fixed / Added
read_bufOk(0)). One sharedstream::read_or_eofthrough all nine loops. 8.05 CPU-s / 16 timeouts → 0 / 0 (Suite 11).0.0.0.0unauthenticated (leaked pool topology). Now binds127.0.0.1by default;PGVPD_ADMIN_HOSTopts in (Suite 12).pool_creates_totalcounts successes not attempts; reset timeout vs. failure logged distinctly; CI lint gate now runscargo clippy --all-targetswith the backlog cleared.The whole open register is addressed. No publish until merged + tagged.