From 8cd3092e879aeaba2342a8a684d0194b06042771 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 17 May 2026 18:36:22 +0200 Subject: [PATCH 1/2] 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 From e98903c5027f660c3ff3109269db03a868147f1f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 17 May 2026 18:45:14 +0200 Subject: [PATCH 2/2] fix(storage): address review on PR #799 (stale comments, docs deprecation, unit test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-review on #799 flagged: 1. Stale inline comment in ``initialize()`` (line 466) still said "Postgres uses a small bounded pool". Updated to reflect both backends now use NullPool. 2. Stale ``close()`` docstring referenced pool-size starving max_connections — irrelevant with NullPool. Replaced with the NullPool-aware rationale (dispose still tears down in-flight asyncpg connections cleanly). 3. ``docs/configuration.md`` actively directed operators to tune DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW, with worked examples and pool math. Both are now deprecated no-ops; the table entries explain the deprecation and link to PR #799. Operators reading the docs will no longer be confused into tuning settings that don't do anything. 4. ``config.py`` comment for the deprecated fields updated to record the deprecation. Validators are intentionally kept (still reject < 1 / < 0) so misconfigured deploys fail loudly rather than silently — the reviewer flagged this as a minor UX wart but explicitly "not a blocker"; the docs change in (3) keeps operators away from the config altogether. 5. New ``tests/unit/test_storage_engine.py`` with three tests: - ``test_postgres_engine_uses_nullpool`` — pins ``isinstance( engine.pool, NullPool)`` so a refactor back to QueuePool / SingletonThreadPool can't silently re-introduce the cross- event-loop crashes. - ``test_postgres_engine_ignores_pool_sizing_settings`` — setting DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW to huge values must not change pool type (proves the deprecated fields are wired-up no-ops). - ``test_postgres_engine_missing_asyncpg_driver_message`` — guards the existing actionable-error branch when the optional ``[postgres]`` extra isn't installed. Verified: - ``uv run pytest tests/unit/`` — 1028 passed - ``uv run ruff check`` clean on the touched python files Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration.md | 16 ++--- nextcloud_mcp_server/auth/storage.py | 20 +++---- nextcloud_mcp_server/config.py | 14 +++-- tests/unit/test_storage_engine.py | 88 ++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 24 deletions(-) create mode 100644 tests/unit/test_storage_engine.py diff --git a/docs/configuration.md b/docs/configuration.md index bb037d84..a29aa626 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -138,15 +138,15 @@ TOKEN_ENCRYPTION_KEY= | `TOKEN_STORAGE_DB` | Optional | Legacy SQLite-only path. Used when `DATABASE_URL` is unset. Falls back to a per-process ephemeral tempfile when both are unset. | | `DATABASE_VERIFY_SSL` | Optional | TLS verification toggle for the Postgres backend. Unset (default) → asyncpg's `prefer` mode (TLS if offered, no verification — keeps cluster-internal Postgres working). `true` → full cert verification. `false` → silence cert errors (homelab / self-signed). | | `DATABASE_CA_BUNDLE` | Optional | Path to a PEM file containing a private CA. Implies `DATABASE_VERIFY_SSL=true`. Use this for self-hosted Postgres signed by your homelab CA instead of disabling verification. | -| `DATABASE_POOL_SIZE` | Optional (default `2`) | Per-pod SQLAlchemy connection pool size for the Postgres backend. asyncpg connections are single-flight, so this only needs to cover concurrent storage ops (not concurrent tool calls). See [ADR-026 § Concurrency model and pool sizing](ADR-026-pluggable-database-backend.md). | -| `DATABASE_MAX_OVERFLOW` | Optional (default `5`) | Per-pod burst connections beyond `DATABASE_POOL_SIZE`. Max per-pod = `pool_size + max_overflow` (default 7). Set to `0` for a hard cap. With 3 replicas the default totals 21 connections — well under managed-Postgres `max_connections=100`. | +| `DATABASE_POOL_SIZE` | Deprecated, no-op | Was per-pod SQLAlchemy pool size for the Postgres backend. The engine now uses `NullPool` (one fresh asyncpg connection per checkout) to avoid cross-event-loop crashes under anyio TaskGroups — see [ADR-026 § Connection pool](ADR-026-pluggable-database-backend.md) and [#799](https://github.com/cbcoutinho/nextcloud-mcp-server/pull/799). Still accepted for backward compatibility; setting it has no effect. | +| `DATABASE_MAX_OVERFLOW` | Deprecated, no-op | Was per-pod burst connection cap on top of `DATABASE_POOL_SIZE`. Now ignored (see above). | -Operators with very high concurrency (many MCP clients per pod, or -expensive Nextcloud round-trips holding storage locks) should tune these -up; single-user / homelab deployments can drop to `DATABASE_POOL_SIZE=1 -DATABASE_MAX_OVERFLOW=2` for the smallest possible footprint. The -server logs the configured sizes at startup so over-allocation is -visible without grepping config. +The asyncpg engine is `NullPool`-only: each `engine.connect()` opens +and tears down a fresh asyncpg connection in the caller's current +event loop. On LAN-local Postgres the per-connection overhead is a +single round-trip (~5 ms), so the throughput cost is negligible for +the MCP server's traffic shape (low concurrency, bursty per-user +requests). Homelab example (self-signed Postgres with a private CA): diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 63c900f5..4d58069d 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -463,10 +463,11 @@ class RefreshTokenStorage: if Path(self.db_path).exists(): os.chmod(self.db_path, 0o600) - # Create the shared async engine for the chosen backend. SQLite uses - # NullPool (per-call connections, matches the prior aiosqlite-direct - # behavior); Postgres uses a small bounded pool — see - # ``_build_postgres_engine`` for sizing rationale. + # Create the shared async engine for the chosen backend. Both + # SQLite and Postgres use NullPool (per-call connections, no + # cross-loop bookkeeping). SQLite mirrors the prior + # aiosqlite-direct behavior; see ``_build_postgres_engine`` for + # the Postgres rationale. if is_sqlite: self.engine = create_async_engine( self.database_url, @@ -598,12 +599,11 @@ class RefreshTokenStorage: async def close(self) -> None: """Dispose the underlying AsyncEngine on shutdown. - Without an explicit dispose, asyncpg's pooled connections leak - server-side slots until the Postgres - ``idle_in_transaction_session_timeout`` reaps them — with the - small pool defaults and frequent k8s rolling restarts this can - starve ``max_connections``. Idempotent: safe to call from any - number of shutdown hooks. + With ``NullPool`` the dispose call has no idle pool to drain, + but it still cleanly tears down any in-flight asyncpg + connections held by active checkouts so shutdown hooks don't + leave dangling transports behind. Idempotent: safe to call + from any number of shutdown hooks. """ if self.engine is None: return diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 0a1730dc..ca841636 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -520,12 +520,14 @@ class Settings: # or supply a private-CA bundle. database_verify_ssl: bool | None = None database_ca_bundle: str | None = None - # Postgres connection pool sizing (ADR-026). The asyncpg engine maps - # these to its underlying QueuePool. Per-pod max = pool_size + - # max_overflow. Defaults are intentionally small (2 + 5 = 7) because - # asyncpg connections are single-flight and the typical MCP workload - # is light read-mostly point lookups. Validate >= 1 / >= 0 in - # __post_init__. + # Postgres connection pool sizing — DEPRECATED, retained for + # backward compatibility. The asyncpg engine switched to NullPool + # in #799 (cross-event-loop crashes under anyio TaskGroups made + # the original QueuePool + pool_pre_ping setup unsafe). These + # fields no longer affect the Postgres engine; the validators + # below still reject invalid values so misconfigured deploys + # fail loudly rather than silently. See ADR-026 § Connection + # pool and docs/configuration.md. database_pool_size: int = 2 database_max_overflow: int = 5 diff --git a/tests/unit/test_storage_engine.py b/tests/unit/test_storage_engine.py new file mode 100644 index 00000000..63d69279 --- /dev/null +++ b/tests/unit/test_storage_engine.py @@ -0,0 +1,88 @@ +"""Unit tests for ``RefreshTokenStorage._build_postgres_engine``. + +PR #799 switched the Postgres engine from ``AsyncAdaptedQueuePool`` +to ``NullPool`` to eliminate cross-event-loop crashes under anyio +TaskGroups. The method is factored out explicitly so a future +engine-arg unit test has a single seam to mock — these tests pin +the pool class and the connect-args plumbing so a refactor can't +silently regress to a sharing pool. +""" + +from __future__ import annotations + +import pytest +from sqlalchemy.ext.asyncio import AsyncEngine +from sqlalchemy.pool import NullPool + +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = pytest.mark.unit + + +def _storage(url: str) -> RefreshTokenStorage: + # ``encryption_key=None`` is fine for engine-shape tests; the + # cipher is only constructed lazily for cipher-protected ops, + # and these tests never call those. + return RefreshTokenStorage(database_url=url, encryption_key=None) + + +# The unit-test environment may not have the optional ``[postgres]`` +# extra installed (asyncpg is the C-extension dep). Skip the +# engine-construction tests when asyncpg isn't importable rather +# than hitting the "DATABASE_URL points at Postgres via asyncpg but +# the 'asyncpg' driver is not installed" guard — that branch is +# exercised explicitly by ``test_postgres_engine_missing_asyncpg_driver_message`` +# below. +asyncpg_required = pytest.importorskip("asyncpg") + + +def test_postgres_engine_uses_nullpool(): + """The Postgres engine must use ``NullPool`` to avoid cross-loop + crashes under anyio TaskGroups (see PR #799).""" + storage = _storage("postgresql+asyncpg://mcp:placeholder@db.example.com:5432/mcp") + engine = storage._build_postgres_engine() + + assert isinstance(engine, AsyncEngine) + # ``engine.pool`` is the sync proxy pool; the underlying pool + # class is what we care about for the loop-binding behaviour. + assert isinstance(engine.pool, NullPool), ( + f"expected NullPool, got {type(engine.pool).__name__} — a regression " + "to QueuePool/SingletonThreadPool will re-introduce the cross-event-" + "loop crashes from PR #799" + ) + + +def test_postgres_engine_ignores_pool_sizing_settings(monkeypatch: pytest.MonkeyPatch): + """``DATABASE_POOL_SIZE`` / ``DATABASE_MAX_OVERFLOW`` are kept as + deprecated no-ops for backward compat. NullPool has no concept of + these, so changing them must not raise or change pool type.""" + # Stash arbitrarily large values into the settings the engine + # consults; NullPool is parameterless so the engine should ignore + # them entirely. + from nextcloud_mcp_server import config as cfg + + monkeypatch.setattr(cfg.get_settings(), "database_pool_size", 99, raising=False) + monkeypatch.setattr(cfg.get_settings(), "database_max_overflow", 99, raising=False) + + storage = _storage("postgresql+asyncpg://mcp:placeholder@db.example.com:5432/mcp") + engine = storage._build_postgres_engine() + assert isinstance(engine.pool, NullPool) + + +def test_postgres_engine_missing_asyncpg_driver_message( + monkeypatch: pytest.MonkeyPatch, +): + """When the ``+asyncpg`` dialect is requested but the asyncpg + optional dep isn't installed, the engine builder must surface an + actionable error before SQLAlchemy emits its generic + ``ModuleNotFoundError``.""" + import importlib.util + + def _fake_find_spec(name: str): + return None if name == "asyncpg" else importlib.util.find_spec(name) + + monkeypatch.setattr(importlib.util, "find_spec", _fake_find_spec) + + storage = _storage("postgresql+asyncpg://mcp:placeholder@db.example.com:5432/mcp") + with pytest.raises(RuntimeError, match="asyncpg.*not installed"): + storage._build_postgres_engine()