Repository navigation
fix(postgres-mcp-server): avoid binding PG-wire connection to a closed event loop - #4846
Conversation
|
Fixes #4826 Why: PG-wire startup validation under What: Move PG-wire ( Test: Regression |
|
CI is green. Avoids binding the PG-wire connection to a closed event loop. Please review and merge when you can. |
84163f4 to
62ea937
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4846 +/- ##
==========================================
+ Coverage 93.80% 93.82% +0.01%
==========================================
Files 1080 1080
Lines 95085 95152 +67
Branches 15506 15514 +8
==========================================
+ Hits 89197 89277 +80
+ Misses 3511 3493 -18
- Partials 2377 2382 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kennthhz
left a comment
kennthhz
left a comment
There was a problem hiding this comment.
Diagnosis looks right to me, and the lifespan is the correct mechanism. I traced the root cause and it holds: every pool operation in PsycopgPoolConnection goes through self.rw_lock, and aiorwlock binds its loop lazily on first acquire, then raises on cross-loop use. asyncio.run() bound it to a loop that then closed.
Two things I'd want addressed before this merges, plus some smaller notes inline.
The one I care most about: nothing appears to test that the lifespan is actually registered. The new test calls server_lifespan(...) directly and patches mcp.run, so lifespan=server_lifespan in the constructor is unasserted. That matters more than it looks — see the inline comment.
I verified the framework behavior against mcp 2.0.0 and aiorwlock 1.5.0 locally. One claim I could not check: whether a test elsewhere in the suite already covers the lifespan registration. If so, ignore that comment.
Close cached pools together under a per-connection timeout and drop a connection from the map only after close() succeeds. Log nested-loop and cross-loop RuntimeErrors separately, pin the MCPServer lifespan argument, and opt loop-affine connectors into serving-loop startup validation.
Signed-off-by: Mangesh Raut <[email protected]>
awslabs#4846 moved loop-affine startup validation from main() into the MCP lifespan, so the username-only IAM startup test now checks the connection main() defers instead of an awaited validate_connection. Signed-off-by: Mangesh Raut <[email protected]>
Fixes #4826
Summary
Changes
When PG-wire startup validation ran under
asyncio.run(), the pool andaiorwlockwere bound to a temporary loop that then closed.mcp.run()serves tools on a different loop, so the firstrun_queryfailed with a different-event-loopRuntimeError.Move PG-wire (
PsycopgPoolConnection) startup validation into the MCP server lifespan so the pool initializes on the serving loop, and clean up loop-local connection maps on shutdown. Non-PG-wire validation stays on the existing sync path.User experience
Before: the first
run_queryafter PG-wire startup could fail with a closed / different event loop error. After: validation runs on the serving loop, so the first query uses a live pool.Checklist
Tests: regression covering
main()→ lifespan validation → firstrun_querywith a fake wire pool.uv run pytest— 2501 passed in the server package context. Ruff and pyright clean on touched files.Is this a breaking change? (Y/N) N
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.