admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #799 from cbcoutinho/fix/storage-nullpool-async-asyncpg

Browse Source 
fix(storage): use NullPool for Postgres engine (cross-loop crashes under anyio)
This commit is contained in:
Chris Coutinho
2026-05-17 18:47:26 +02:00
committed by GitHub
parent 39d70cfdf5 e98903c502
commit e6fa4e14dd
4 changed files with 141 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/configuration.md
+8 -8
View File
@@ -138,15 +138,15 @@ TOKEN_ENCRYPTION_KEY=<fernet-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):
nextcloud_mcp_server/auth/storage.py
+37 -27
View File
@@ -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,
@@ -464,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,
@@ -534,11 +534,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,35 +584,26 @@ 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
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
nextcloud_mcp_server/config.py
+8 -6
View File
@@ -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
tests/unit/test_storage_engine.py
+88
View File
@@ -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()