From e98903c5027f660c3ff3109269db03a868147f1f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 17 May 2026 18:45:14 +0200 Subject: [PATCH] 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()