diff --git a/backend/app/api/routes/auth.py b/backend/app/api/routes/auth.py index 1e613524d..de987af18 100644 --- a/backend/app/api/routes/auth.py +++ b/backend/app/api/routes/auth.py @@ -189,15 +189,9 @@ async def set_advanced_auth_enabled(db: AsyncSession, enabled: bool) -> None: async def set_auth_enabled(db: AsyncSession, enabled: bool) -> None: """Set authentication enabled status.""" - from backend.app.core.auth import invalidate_auth_enabled_cache from backend.app.core.db_dialect import upsert_setting await upsert_setting(db, Settings, "auth_enabled", "true" if enabled else "false") - # Drop the cached auth-enabled flag so the change takes effect immediately - # instead of after the TTL (issue #2572). Safe pre-commit: only enabled=True - # is ever cached, and the newly-enabled True isn't visible to other sessions - # until this transaction commits, so no stale value can be re-cached here. - invalidate_auth_enabled_cache() # Note: Don't commit here - let get_db handle it or commit explicitly in the route diff --git a/backend/app/api/routes/system.py b/backend/app/api/routes/system.py index dd0c47aae..670ebe4f2 100644 --- a/backend/app/api/routes/system.py +++ b/backend/app/api/routes/system.py @@ -606,24 +606,6 @@ async def get_system_health( return await asyncio.to_thread(scan_logs, sensitive_strings=sensitive_strings) -@router.get("/db-pool") -async def get_db_pool( - _: User | None = RequirePermissionIfAuthEnabled(Permission.SYSTEM_READ), -): - """Live database connection-pool gauges for large-farm diagnostics (#2572). - - Reports the resolved pool configuration plus current checked-out / - checked-in / overflow counts. Deliberately takes no DB session — reading - the pool's own counters must not itself consume a connection, so this stays - truthful even when the pool is saturated. On a healthy install ``checked_out`` - sits well below ``config.pool_size + config.max_overflow``; sustained - saturation points at connections held across slow I/O (see #2572). - """ - from backend.app.core.database import get_pool_status - - return get_pool_status() - - @router.get("/appliance") async def get_appliance_defaults(): """Expose appliance-set state for the SPA's bootstrap surface. diff --git a/backend/app/core/auth.py b/backend/app/core/auth.py index a733d68b9..820e4ef0a 100644 --- a/backend/app/core/auth.py +++ b/backend/app/core/auth.py @@ -3,7 +3,6 @@ from __future__ import annotations import logging import os import secrets -import time from datetime import datetime, timedelta, timezone from typing import Annotated @@ -863,33 +862,6 @@ async def authenticate_user_by_email(db: AsyncSession, email: str, password: str return user -# Short-lived cache for the auth-enabled flag (issue #2572). The middleware -# and every ownership/permission dependency probe this once (or more) per -# request; on a large farm that DB round-trip is pure overhead because the -# value changes only when an admin toggles auth. -# -# SECURITY: only a ``True`` (auth-enabled) result is EVER cached. A disabled / -# unconfigured result is never cached, so a stale cache can only ever cause a -# request to REQUIRE auth that a moment ago wasn't required — it can never skip -# an auth check that is now required. Staleness fails CLOSED, never open (cf. -# GHSA-6mf4-q26m-47pv). ``set_auth_enabled`` invalidates explicitly on any -# toggle; the TTL is only a backstop for out-of-band changes (a direct DB edit, -# or another worker process in a multi-worker deployment). -_AUTH_ENABLED_CACHE_TTL_SECONDS = 30.0 -_auth_enabled_cached_value: bool = False -_auth_enabled_cached_until: float = 0.0 - - -def invalidate_auth_enabled_cache() -> None: - """Drop the cached auth-enabled flag so the next probe re-reads the DB. - - Call after any write that toggles the ``auth_enabled`` setting. - """ - global _auth_enabled_cached_value, _auth_enabled_cached_until - _auth_enabled_cached_value = False - _auth_enabled_cached_until = 0.0 - - async def is_auth_enabled(db: AsyncSession) -> bool: """Check if authentication is enabled. @@ -906,25 +878,12 @@ async def is_auth_enabled(db: AsyncSession) -> bool: no exception. Any OTHER failure (connection error, fd exhaustion, schema mismatch, …) propagates so the caller can deny the request (503 / 500). Fail-closed is the only safe default for an auth probe. - - Result is cached briefly to cut per-request DB load on large farms; only - the enabled=True result is cached, so a stale read can only fail closed. - See the module-level cache comment above. """ - global _auth_enabled_cached_value, _auth_enabled_cached_until - if _auth_enabled_cached_value and time.monotonic() < _auth_enabled_cached_until: - return True - result = await db.execute(select(Settings).where(Settings.key == "auth_enabled")) setting = result.scalar_one_or_none() - enabled = setting is not None and setting.value.lower() == "true" - if enabled: - _auth_enabled_cached_value = True - _auth_enabled_cached_until = time.monotonic() + _AUTH_ENABLED_CACHE_TTL_SECONDS - else: - # Never cache "disabled" — keep failing closed on any future staleness. - _auth_enabled_cached_value = False - return enabled + if setting is None: + return False + return setting.value.lower() == "true" async def _user_from_api_key(db: AsyncSession, api_key: APIKey) -> User | None: diff --git a/backend/app/core/config.py b/backend/app/core/config.py index 1a50a7b3d..eb29f6f83 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -74,17 +74,6 @@ class Settings(BaseSettings): log_dir: Path = _log_dir database_url: str = _external_db_url or f"sqlite+aiosqlite:///{_db_path}" - # Database connection pool sizing. ``None`` = use the built-in, dialect-aware - # default (PostgreSQL: pool_size 20 + max_overflow 80; SQLite: 20 + 200). - # Large PostgreSQL printer farms can raise these via the DB_POOL_SIZE / - # DB_MAX_OVERFLOW / DB_POOL_TIMEOUT / DB_POOL_RECYCLE env vars (issue #2572). - # Make sure PostgreSQL ``max_connections`` comfortably exceeds - # (pool_size + max_overflow) x number of app worker processes. - db_pool_size: int | None = Field(default=None, gt=0) - db_max_overflow: int | None = Field(default=None, ge=0) - db_pool_timeout: int | None = Field(default=None, gt=0) - db_pool_recycle: int | None = Field(default=None, gt=0) - # Logging log_level: str = "INFO" # Override with LOG_LEVEL env var or DEBUG=true log_to_file: bool = True # Set to false to disable file logging diff --git a/backend/app/core/database.py b/backend/app/core/database.py index 591e966f8..8e8b36c67 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -23,54 +23,12 @@ def _set_sqlite_pragmas(dbapi_conn, connection_record): cursor.close() -# Resolved connection-pool configuration, captured at engine creation so -# /system/db-pool can report it without re-deriving the dialect defaults. -_pool_config: dict = {} - - -def _resolve_pool_kwargs() -> dict: - """Build the pool kwargs for ``create_async_engine`` (issue #2572). - - Dialect-aware defaults, each overridable via env (``DB_POOL_SIZE`` etc.): - - PostgreSQL: pool_size 20 + max_overflow 80, ``pool_pre_ping`` (recover - server-dropped connections instead of erroring the request) and - ``pool_recycle`` 1800s. The old hard-coded 10 + 20 exhausted on large - farms while printer callbacks held connections. - - SQLite: pool_size 20 + max_overflow 200 (unchanged); no pre-ping / - recycle — the connection is a local file, not a server socket. - """ - if is_sqlite(): - pool_size = settings.db_pool_size if settings.db_pool_size is not None else 20 - max_overflow = settings.db_max_overflow if settings.db_max_overflow is not None else 200 - kwargs = {"pool_size": pool_size, "max_overflow": max_overflow} - else: - pool_size = settings.db_pool_size if settings.db_pool_size is not None else 20 - max_overflow = settings.db_max_overflow if settings.db_max_overflow is not None else 80 - kwargs = { - "pool_size": pool_size, - "max_overflow": max_overflow, - "pool_pre_ping": True, - "pool_recycle": settings.db_pool_recycle if settings.db_pool_recycle is not None else 1800, - } - if settings.db_pool_timeout is not None: - kwargs["pool_timeout"] = settings.db_pool_timeout - return kwargs - - def _create_engine(): """Create the async engine with dialect-appropriate settings.""" - kwargs = _resolve_pool_kwargs() - - global _pool_config - _pool_config = { - "pool_size": kwargs["pool_size"], - "max_overflow": kwargs["max_overflow"], - # SQLAlchemy's own defaults when we don't pass the kwarg. - "pool_timeout": kwargs.get("pool_timeout", 30), - "pool_recycle": kwargs.get("pool_recycle", -1), - "pool_pre_ping": kwargs.get("pool_pre_ping", False), - } - + if is_sqlite(): + kwargs = {"pool_size": 20, "max_overflow": 200} + else: + kwargs = {"pool_size": 10, "max_overflow": 20} eng = create_async_engine( settings.database_url, echo=settings.debug, @@ -121,36 +79,6 @@ async_session = async_sessionmaker( ) -def get_pool_status() -> dict: - """Snapshot the DB connection pool for diagnostics (issue #2572). - - Returns the resolved configuration plus live gauges (checked-out / - checked-in / overflow). Reads the pool's own counters — it does NOT - check out a connection, so it stays truthful even when the pool is - exhausted. Gauges a given pool implementation doesn't expose come back - as ``None`` rather than raising. - """ - pool = engine.sync_engine.pool - gauges: dict = {} - for key, method_name in ( - ("current_size", "size"), - ("checked_out", "checkedout"), - ("checked_in", "checkedin"), - ("overflow", "overflow"), - ): - method = getattr(pool, method_name, None) - try: - gauges[key] = method() if callable(method) else None - except Exception: - # A gauge should never take down the diagnostics endpoint. - gauges[key] = None - return { - "dialect": "sqlite" if is_sqlite() else "postgresql", - "config": dict(_pool_config), - **gauges, - } - - async def run_with_retry(fn, *, max_attempts: int = 3, label: str = ""): """Run an async DB operation with retry for SQLite 'database is locked' errors. diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index fdbe90562..8068894ff 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -95,22 +95,6 @@ def reset_spoolman_location_sync_cache(): _spoolman_location_sync_cache_clear() -@pytest.fixture(autouse=True) -def reset_auth_enabled_cache(): - """Drop the module-level auth-enabled cache between tests (issue #2572). - - ``is_auth_enabled`` caches an enabled=True result for a TTL. Without this - reset a test that enables auth would leave ``True`` cached, so a later test - running in auth-disabled mode (without going through ``set_auth_enabled``) - would wrongly see auth as enabled until the TTL expired — order-dependent - flakiness.""" - from backend.app.core.auth import invalidate_auth_enabled_cache - - invalidate_auth_enabled_cache() - yield - invalidate_auth_enabled_cache() - - @pytest.fixture(scope="session") def event_loop(): """Create an instance of the default event loop for each test session.""" diff --git a/backend/tests/unit/test_db_pool_and_auth_cache.py b/backend/tests/unit/test_db_pool_and_auth_cache.py deleted file mode 100644 index f55b41701..000000000 --- a/backend/tests/unit/test_db_pool_and_auth_cache.py +++ /dev/null @@ -1,191 +0,0 @@ -"""Tests for the DB connection-pool sizing/diagnostics and the auth-enabled -cache added for large printer farms (issue #2572).""" - -import asyncio - -import pytest -from sqlalchemy import text -from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine - - -class TestPoolConfiguration: - """P0: env-configurable, dialect-aware pool sizing.""" - - def test_sqlite_defaults_when_unset(self, monkeypatch): - """SQLite keeps 20 + 200 when no env override is set.""" - from backend.app.core import database - - for attr in ("db_pool_size", "db_max_overflow", "db_pool_timeout", "db_pool_recycle"): - monkeypatch.setattr(database.settings, attr, None, raising=False) - monkeypatch.setattr(database, "is_sqlite", lambda: True) - - kwargs = database._resolve_pool_kwargs() - assert kwargs["pool_size"] == 20 - assert kwargs["max_overflow"] == 200 - # No server-socket recycle/pre-ping for a local file. - assert "pool_pre_ping" not in kwargs - assert "pool_recycle" not in kwargs - - def test_postgres_defaults_raise_the_old_limits(self, monkeypatch): - """Postgres default is now 20 + 80 (was 10 + 20) with pre-ping + recycle.""" - from backend.app.core import database - - for attr in ("db_pool_size", "db_max_overflow", "db_pool_timeout", "db_pool_recycle"): - monkeypatch.setattr(database.settings, attr, None, raising=False) - monkeypatch.setattr(database, "is_sqlite", lambda: False) - - kwargs = database._resolve_pool_kwargs() - assert kwargs["pool_size"] == 20 - assert kwargs["max_overflow"] == 80 - assert kwargs["pool_pre_ping"] is True - assert kwargs["pool_recycle"] == 1800 - - def test_env_overrides_win_on_postgres(self, monkeypatch): - """DB_POOL_* overrides replace the dialect defaults.""" - from backend.app.core import database - - monkeypatch.setattr(database.settings, "db_pool_size", 100, raising=False) - monkeypatch.setattr(database.settings, "db_max_overflow", 200, raising=False) - monkeypatch.setattr(database.settings, "db_pool_timeout", 45, raising=False) - monkeypatch.setattr(database.settings, "db_pool_recycle", 600, raising=False) - monkeypatch.setattr(database, "is_sqlite", lambda: False) - - kwargs = database._resolve_pool_kwargs() - assert kwargs["pool_size"] == 100 - assert kwargs["max_overflow"] == 200 - assert kwargs["pool_timeout"] == 45 - assert kwargs["pool_recycle"] == 600 - - @pytest.mark.asyncio - async def test_concurrent_checkouts_exceed_base_pool_size(self, tmp_path): - """Regression (#2572): more concurrent sessions than pool_size must all - complete by drawing from max_overflow — not deadlock or time out. - - This is the failure the farm hit: printer callbacks held every base - connection, so unrelated requests waited on the pool. With headroom in - max_overflow, concurrent checkouts beyond pool_size still succeed. - - Uses a file-based SQLite URL so it gets a real queue pool — the - in-memory URL forces a single-connection StaticPool that ignores - pool_size/max_overflow entirely. - """ - db_file = tmp_path / "pool_regression.db" - eng = create_async_engine(f"sqlite+aiosqlite:///{db_file}", pool_size=2, max_overflow=10) - sm = async_sessionmaker(eng) - - async def _one(): - async with sm() as s: - await s.execute(text("SELECT 1")) - # Hold the checkout briefly so the calls genuinely overlap and - # force the pool past its base size of 2. - await asyncio.sleep(0.05) - return (await s.execute(text("SELECT 1"))).scalar() - - try: - results = await asyncio.gather(*[_one() for _ in range(12)]) - finally: - await eng.dispose() - - assert results == [1] * 12 - - -class TestPoolStatus: - """P3: diagnostics snapshot.""" - - def test_get_pool_status_shape(self): - from backend.app.core.database import get_pool_status - - status = get_pool_status() - assert status["dialect"] in ("sqlite", "postgresql") - for key in ("pool_size", "max_overflow", "pool_timeout", "pool_recycle", "pool_pre_ping"): - assert key in status["config"] - # Live gauges are present (values are ints on a QueuePool). - for key in ("current_size", "checked_out", "checked_in", "overflow"): - assert key in status - - -class TestAuthEnabledCache: - """P1: cache the auth-enabled probe, but only ever cache True.""" - - class _Setting: - def __init__(self, value): - self.value = value - - class _Result: - def __init__(self, setting): - self._setting = setting - - def scalar_one_or_none(self): - return self._setting - - class _CountingDB: - def __init__(self, value): - self._value = value - self.calls = 0 - - async def execute(self, *args, **kwargs): - self.calls += 1 - setting = None if self._value is None else TestAuthEnabledCache._Setting(self._value) - return TestAuthEnabledCache._Result(setting) - - @pytest.mark.asyncio - async def test_enabled_true_is_cached(self): - from backend.app.core import auth as auth_mod - - auth_mod.invalidate_auth_enabled_cache() - db = self._CountingDB("true") - - assert await auth_mod.is_auth_enabled(db) is True - assert db.calls == 1 - # Second probe served from cache — no new query. - assert await auth_mod.is_auth_enabled(db) is True - assert db.calls == 1 - - # Invalidation forces a re-read (e.g. after set_auth_enabled). - auth_mod.invalidate_auth_enabled_cache() - assert await auth_mod.is_auth_enabled(db) is True - assert db.calls == 2 - auth_mod.invalidate_auth_enabled_cache() - - @pytest.mark.asyncio - async def test_disabled_is_never_cached(self): - """SECURITY: a disabled result must never be cached, so staleness can - only ever fail closed (require auth), never open.""" - from backend.app.core import auth as auth_mod - - auth_mod.invalidate_auth_enabled_cache() - db = self._CountingDB("false") - - assert await auth_mod.is_auth_enabled(db) is False - assert db.calls == 1 - # Every probe re-reads while disabled. - assert await auth_mod.is_auth_enabled(db) is False - assert db.calls == 2 - auth_mod.invalidate_auth_enabled_cache() - - @pytest.mark.asyncio - async def test_unconfigured_returns_false_and_is_not_cached(self): - from backend.app.core import auth as auth_mod - - auth_mod.invalidate_auth_enabled_cache() - db = self._CountingDB(None) - - assert await auth_mod.is_auth_enabled(db) is False - assert await auth_mod.is_auth_enabled(db) is False - assert db.calls == 2 - auth_mod.invalidate_auth_enabled_cache() - - @pytest.mark.asyncio - async def test_db_error_propagates_fail_closed(self): - """A probe error must propagate (fail closed), not be swallowed.""" - from backend.app.core import auth as auth_mod - - auth_mod.invalidate_auth_enabled_cache() - - class _RaisingDB: - async def execute(self, *args, **kwargs): - raise RuntimeError("connection lost") - - with pytest.raises(RuntimeError): - await auth_mod.is_auth_enabled(_RaisingDB()) - auth_mod.invalidate_auth_enabled_cache()