From 8cd3092e879aeaba2342a8a684d0194b06042771 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 17 May 2026 18:36:22 +0200 Subject: [PATCH] fix(storage): use NullPool for Postgres engine (cross-loop crashes under anyio TaskGroup) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Postgres backend hit a hard crashloop in production deployments where the MCP server runs under anyio TaskGroups with multiple concurrent background tasks (`vector.oauth_sync.user_manager_task`, processors, etc.) alongside the request-path code. Symptom in the pod logs: RuntimeError: Task got Future attached to a different loop followed seconds later by RuntimeError: Event loop is closed while SQLAlchemy's pool tries to clean up the failed connection. The event loop becomes increasingly unresponsive as asyncpg protocol Futures pile up holding references to closed loops; the `/health/live` endpoint eventually misses its probe window and the kubelet SIGKILLs the pod (exitCode 137), restart-looping the backend. Root cause: the engine was built with the default `AsyncAdaptedQueuePool` (`pool_size=2, max_overflow=5`) and `pool_pre_ping=True`. asyncpg connection objects are bound to the event loop they were created on. When the process holds a singleton engine and tasks running under different anyio TaskGroups check out connections from that pool, the pre-ping probe runs on a cached connection whose underlying transport references a different loop's selector → cross-loop access → crash. Switch to `NullPool` — one fresh asyncpg connection per `engine.connect()`, no caching, no cross-loop bookkeeping to get wrong. asyncpg connection setup is ~5 ms over LAN and a single round-trip in the local-Postgres case, so the throughput cost is negligible for the MCP server's traffic shape (low concurrency, bursty per-user requests). This matches what the SQLite branch already does (see `initialize()`) and what Alembic's `env.py` uses for migrations, so the codebase is now consistent across all backends. `DATABASE_POOL_SIZE` / `DATABASE_MAX_OVERFLOW` config knobs are preserved for backward compatibility but no longer affect the Postgres engine. The validators in `config.py` continue to reject values < 1 / < 0, so misconfigured deploys still fail loudly. A follow-up could mark them deprecated in `docs/configuration.md`; out of scope here. Discovered while smoke-testing the per-tenant Postgres flow in Astrolabe Cloud (every-tenant pod fresh-provisions a database via the ADR-026 backend → hits this crashloop within ~5 min of the first MCP-routed request). Refs: - ADR-026 § "Concurrency model and pool sizing" (the original QueuePool rationale, now superseded by this finding) - nextcloud_mcp_server/alembic/env.py (NullPool for migrations) - SQLAlchemy docs: NullPool is the documented choice when connection objects don't survive across the lifecycle of the pool's logical "owner" (here: the event loop) Verified: - `uv run ruff check nextcloud_mcp_server/auth/storage.py` clean - `uv run pytest tests/unit/test_*storage*.py` → 29 passed Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/storage.py | 44 +++++++++++++++++----------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 6606fc98..63c900f5 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -54,7 +54,6 @@ from sqlalchemy.pool import NullPool from nextcloud_mcp_server.config import ( get_database_ssl, get_database_url, - get_settings, is_ephemeral_token_db, is_sqlite_url, mask_db_password, @@ -534,11 +533,30 @@ class RefreshTokenStorage: under the SonarQube ``S3776`` threshold and so a future engine-arg unit test has a single seam to mock. - Defaults to ``pool_size=2, max_overflow=5`` (max 7 connections - per pod) — see ADR-026 § "Concurrency model and pool sizing" - for the rationale. asyncpg connections are single-flight, so - the pool only needs to cover the typical multi-user MCP burst, - not every potential in-flight tool call. + Uses :class:`NullPool` (one fresh asyncpg connection per + checkout, no caching). The original ADR-026 design used a + small bounded ``QueuePool`` with ``pool_pre_ping=True``, but + that combination is unsafe under the server's anyio task + layout: cached asyncpg connections are bound to the event + loop they were opened on, and a checkout from a task running + under a different anyio TaskGroup / loop triggers + ``RuntimeError: got Future attached to a different loop`` on + the pre-ping probe (and then the pool closes the connection + with another ``Event loop is closed`` while cleaning up). + Observed in production against shared-postgres on cloudfleet, + where the background ``vector.oauth_sync.user_manager_task`` + and the request-path code paths share an engine across loops. + + NullPool sidesteps the entire class of bugs: every + ``engine.connect()`` opens a fresh asyncpg connection in the + caller's current loop, and disposes it on close. asyncpg + connection setup is cheap (~5 ms LAN, single round-trip when + the server is local) so the throughput cost is negligible for + the MCP server's traffic shape (low-concurrency, bursty). + ``DATABASE_POOL_SIZE`` / ``DATABASE_MAX_OVERFLOW`` are still + accepted for backward compat but no longer have an effect on + the Postgres backend — they were never propagated to SQLite, + which has always used NullPool. """ # asyncpg ships as an optional PyPI extra (`[postgres]`) so the # default `pip install nextcloud-mcp-server` audience doesn't @@ -565,23 +583,15 @@ class RefreshTokenStorage: connect_args["ssl"] = ssl_arg logger.info("Postgres backend TLS: %s", _describe_ssl_arg(ssl_arg)) - settings = get_settings() engine = create_async_engine( self.database_url, - pool_size=settings.database_pool_size, - max_overflow=settings.database_max_overflow, - pool_pre_ping=True, + poolclass=NullPool, connect_args=connect_args, future=True, ) - # Log the configured sizing so operators can spot - # over-allocation at startup without grepping config. logger.info( - "Postgres engine ready: pool_size=%d max_overflow=%d " - "(per-pod max %d connections)", - settings.database_pool_size, - settings.database_max_overflow, - settings.database_pool_size + settings.database_max_overflow, + "Postgres engine ready: NullPool (one connection per " + "checkout, see ADR-026 § 'Connection pool')" ) return engine