fix(storage): address review on PR #799 (stale comments, docs deprecation, unit test)

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) <noreply@anthropic.com>

docs/configuration.md

@@ -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

@@ -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

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
 

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()