From 5058a50143d4e90ac4d38e34199cdb108970a647 Mon Sep 17 00:00:00 2001 From: Lukas Date: Sun, 3 May 2026 16:02:40 +0200 Subject: [PATCH] Refactor backend: fix geo cache cleanup, scheduler heartbeat, correlation middleware; update docs --- Docs/CONFIGURATION.md | 159 ++++++++++++++++++ Docs/Deployment.md | 6 + Docs/Tasks.md | 41 ----- backend/app/main.py | 3 +- backend/app/startup.py | 2 + backend/app/tasks/geo_cache_cleanup.py | 8 +- backend/app/tasks/scheduler_lock_heartbeat.py | 8 +- backend/tests/test_correlation_middleware.py | 70 +++----- backend/tests/test_main.py | 136 +++++++++------ 9 files changed, 287 insertions(+), 146 deletions(-) create mode 100644 Docs/CONFIGURATION.md diff --git a/Docs/CONFIGURATION.md b/Docs/CONFIGURATION.md new file mode 100644 index 0000000..74fd464 --- /dev/null +++ b/Docs/CONFIGURATION.md @@ -0,0 +1,159 @@ +# Configuration Reference + +All runtime settings are environment variables prefixed with `BANGUI_`. Values are validated at startup — missing required fields or invalid values cause the application to refuse to start. + +For setup instructions, see [Instructions.md](./Instructions.md). For deployment, see [Deployment.md](./Deployment.md). + +--- + +## Database + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_DATABASE_PATH` | string | `bangui.db` | Filesystem path to the SQLite application database. Parent directory must exist and be writable at startup. | + +--- + +## Session & Security + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_SESSION_SECRET` | string | **(required)** | Secret key for signing session tokens. Must be ≥ 32 characters. Generate with `python -c "import secrets; print(secrets.token_hex(32))"`. Never reuse across environments. | +| `BANGUI_SESSION_SECRET_PREVIOUS` | string | `null` | Previous session secret used during rotation. Set to the old secret while rotating; unset once all old tokens expire. | +| `BANGUI_SESSION_DURATION_MINUTES` | int | `60` | Session lifetime in minutes. Must be ≥ 1. | +| `BANGUI_SESSION_CACHE_ENABLED` | bool | `false` | Enable in-memory session validation cache. Disable in multi-worker deployments to avoid stale revoked sessions. | +| `BANGUI_SESSION_CACHE_TTL_SECONDS` | float | `10.0` | TTL for cached session entries. Ignored when `BANGUI_SESSION_CACHE_ENABLED` is `false`. Must be ≥ 0. | +| `BANGUI_SESSION_COOKIE_HTTPONLY` | bool | `true` | Mark the session cookie as `HttpOnly` (JavaScript cannot access it). | +| `BANGUI_SESSION_COOKIE_SAMESITE` | string | `lax` | SameSite policy for the session cookie. Valid values: `lax`, `strict`, `none`. | +| `BANGUI_SESSION_COOKIE_SECURE` | bool | `true` | Set the `Secure` flag on the session cookie. `true` required for HTTPS. Set to `false` only for local HTTP development. | + +--- + +## fail2ban Integration + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_FAIL2BAN_SOCKET` | string | `/var/run/fail2ban/fail2ban.sock` | Path to the fail2ban Unix domain socket. Socket must exist and be readable at startup (warning issued if not). | +| `BANGUI_FAIL2BAN_CONFIG_DIR` | string | `/config/fail2ban` | Path to the fail2ban configuration directory. Must contain `jail.d/`, `filter.d/`, and `action.d/`. | +| `BANGUI_FAIL2BAN_START_COMMAND` | string | `fail2ban-client start` | Shell command to start the fail2ban daemon (no shell interpretation). Used during recovery rollback. Must be parseable by `shlex.split`. | +| `BANGUI_ALLOWED_LOG_DIRS` | list | `/var/log,/config/log` | Allowed directory prefixes for jail log paths. Any log path must resolve within one of these directories. | +| `BANGUI_TRUSTED_PROXIES` | list | `[]` | Trusted reverse proxy IP addresses or CIDR ranges (e.g., `192.168.1.1,10.0.0.0/8`). Only these sources can set `X-Forwarded-For` and `X-Real-IP`. | + +--- + +## HTTP Client + +These settings control outbound HTTP requests made by the backend (geolocation fallback, blocklist downloads). + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_HTTP_REQUEST_TIMEOUT_SECONDS` | float | `20.0` | Maximum total time for an outbound HTTP request. Must be ≥ 0. | +| `BANGUI_HTTP_CONNECT_TIMEOUT_SECONDS` | float | `5.0` | Maximum time to establish a TCP connection. Must be ≥ 0. | +| `BANGUI_HTTP_MAX_CONNECTIONS` | int | `10` | Maximum concurrent outbound HTTP connections. Must be ≥ 1. | +| `BANGUI_HTTP_KEEPALIVE_TIMEOUT_SECONDS` | float | `15.0` | How long idle keepalive connections are retained. Must be ≥ 0. | + +--- + +## Geolocation + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_GEOIP_DB_PATH` | string | `null` | Path to a MaxMind GeoLite2-Country `.mmdb` file. Primary resolver for IP geolocation when set. Download from https://dev.maxmind.com/geoip/geolite2-country. | +| `BANGUI_GEOIP_ALLOW_HTTP_FALLBACK` | bool | `false` | Allow HTTP fallback to `ip-api.com` when the MMDB is unavailable. **Warning**: sends IP addresses unencrypted. Only enable when MMDB cannot be mounted. | + +--- + +## Cross-Origin (CORS) + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_CORS_ALLOWED_ORIGINS` | list | `http://localhost:5173,http://127.0.0.1:5173,https://localhost:5173,https://127.0.0.1:5173` | Allowed CORS origins. Comma-separated string or YAML list. Empty list disables CORS. **Never use `"*"` in production** when credentials are enabled. | + +--- + +## Display + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_TIMEZONE` | string | `UTC` | IANA timezone name used when displaying timestamps in the UI (e.g., `America/New_York`, `Europe/London`). | + +--- + +## External Logging + +Enable with `BANGUI_EXTERNAL_LOGGING_ENABLED=true`, then set the provider and provider-specific variables. + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_EXTERNAL_LOGGING_ENABLED` | bool | `false` | Send logs to a centralized logging platform instead of stdout only. | +| `BANGUI_EXTERNAL_LOGGING_PROVIDER` | string | `null` | Logging provider: `datadog`, `papertrail`, or `elasticsearch`. Required when external logging is enabled. | +| `BANGUI_EXTERNAL_LOGGING_BUFFER_SIZE` | int | `1000` | Max log records buffered in memory before dropping oldest. Must be ≥ 10. | +| `BANGUI_EXTERNAL_LOGGING_FLUSH_INTERVAL_SECONDS` | float | `5.0` | Max seconds before flushing a log batch. Must be > 0. | + +### Datadog + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_DATADOG_API_KEY` | string | `null` | Datadog API key. Required when provider is `datadog`. | +| `BANGUI_DATADOG_SITE` | string | `datadoghq.com` | Datadog site: `datadoghq.com` (US) or `datadoghq.eu` (EU). | +| `BANGUI_DATADOG_BATCH_SIZE` | int | `10` | Number of log records per batch. Must be ≥ 1. | + +### Papertrail + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_PAPERTRAIL_HOST` | string | `null` | Papertrail host address (e.g., `logs1.papertrailapp.com`). Required when provider is `papertrail`. | +| `BANGUI_PAPERTRAIL_PORT` | int | `null` | Papertrail port. Required when provider is `papertrail`. Range: 1–65535. | +| `BANGUI_PAPERTRAIL_PROGRAM_NAME` | string | `bangui` | Program name in Syslog messages sent to Papertrail. | + +### Elasticsearch + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_ELASTICSEARCH_HOSTS` | list | `[]` | Elasticsearch host URLs (e.g., `http://elasticsearch:9200`). Required when provider is `elasticsearch`. | +| `BANGUI_ELASTICSEARCH_INDEX_PREFIX` | string | `bangui` | Prefix for Elasticsearch indices. | +| `BANGUI_ELASTICSEARCH_BATCH_SIZE` | int | `10` | Number of log documents per batch. Must be ≥ 1. | + +--- + +## Rate Limiting + +Per-IP rate limits applied to API endpoints. + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_RATE_LIMIT_BANS_PER_MINUTE` | int | `100` | Max ban/unban requests per IP per minute. | +| `BANGUI_RATE_LIMIT_BLOCKLIST_IMPORT_PER_HOUR` | int | `10` | Max blocklist import requests per IP per hour. | +| `BANGUI_RATE_LIMIT_CONFIG_UPDATE_PER_MINUTE` | int | `50` | Max config update requests per IP per minute. | + +--- + +## Observability + +| Variable | Type | Default | Description | +|----------|------|---------|-------------| +| `BANGUI_LOG_LEVEL` | string | `info` | Application log level. Valid values: `debug`, `info`, `warning`, `error`, `critical`. | +| `BANGUI_ENABLE_DOCS` | bool | `false` | Enable FastAPI interactive docs at `/api/docs` (Swagger UI) and `/api/redoc` (ReDoc). Enable only in development. | + +--- + +## Quick Reference + +```bash +# Generate a session secret +python -c "import secrets; print(secrets.token_hex(32))" + +# Minimal production .env +BANGUI_SESSION_SECRET= +BANGUI_CORS_ALLOWED_ORIGINS=https://your-frontend.example.com +BANGUI_TIMEZONE=America/New_York +``` + +--- + +## Cross-References + +- [Deployment.md](./Deployment.md) — Docker configuration, health checks, graceful shutdown +- [Security.md](./Security.md) — Security recommendations and hardening +- [Observability.md](./Observability.md) — Logging, metrics, and monitoring +- [Backend-Development.md](./Backend-Development.md) — Backend coding conventions diff --git a/Docs/Deployment.md b/Docs/Deployment.md index 4b22856..cd88a73 100644 --- a/Docs/Deployment.md +++ b/Docs/Deployment.md @@ -351,6 +351,12 @@ Use native monitoring: --- +## Configuration + +All runtime settings are documented in [CONFIGURATION.md](./CONFIGURATION.md), including database, session, fail2ban, HTTP client, geolocation, CORS, logging, rate limiting, and observability options. + +--- + ## Environment Variables Resource limits are configured in `Docker/docker-compose.yml` and cannot be overridden via environment variables. To adjust limits: diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 2063889..6a44e5b 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,44 +1,3 @@ -### Issue #22: MEDIUM - Socket Cleanup Errors Silently Suppressed - -**Where found**: -- `backend/app/utils/fail2ban_client.py` (line 150) -- Uses `contextlib.suppress(Exception)` hiding close errors -- Resource leaks possible - -**Why this is needed**: -Suppressing all errors hides real problems: -- Socket never actually closes -- Connection pool exhaustion -- File descriptors leak - -**Goal**: -Log errors properly while still cleaning up resources. - -**What to do**: -1. Replace suppress with logging: - ```python - finally: - try: - await socket.close() - except Exception as e: - logger.warning(f"Error closing fail2ban socket: {e}", exc_info=True) - ``` -2. Audit other resource cleanup (database connections, HTTP sessions) -3. Write tests that verify cleanup happens even on errors - -**Possible traps and issues**: -- Close might raise unexpected errors -- Logging exceptions in finally blocks can cause issues -- Resource exhaustion hard to debug if not logging - -**Docs changes needed**: -- Add resource cleanup guidelines to dev guide - -**Doc references**: -- DETAILED_FINDINGS.md - Issue #14 "Socket Cleanup Silent Fail" - ---- - ### Issue #23: MEDIUM - Missing Default Configuration Documentation **Where found**: diff --git a/backend/app/main.py b/backend/app/main.py index 13310a3..5977f05 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -254,8 +254,9 @@ async def _lifespan(app: FastAPI) -> AsyncGenerator[None, None]: # task's coroutine handles cleanup. import asyncio # noqa: TC003 + current_task = asyncio.current_task() pending_tasks: list[asyncio.Task[Any]] = [ - t for t in asyncio.all_tasks() if not t.done() + t for t in asyncio.all_tasks() if not t.done() and t is not current_task ] if pending_tasks: log.info( diff --git a/backend/app/startup.py b/backend/app/startup.py index afd498e..7657737 100644 --- a/backend/app/startup.py +++ b/backend/app/startup.py @@ -459,6 +459,8 @@ async def _stage_register_tasks(app: FastAPI, scheduler: AsyncIOScheduler) -> No app: The FastAPI application instance. scheduler: The APScheduler scheduler to register tasks with. """ + # Set scheduler on app.state before registering tasks (they use app.state.scheduler) + app.state.scheduler = scheduler scheduler_lock_heartbeat.register(app) health_check.register(app) await blocklist_import.register(app) diff --git a/backend/app/tasks/geo_cache_cleanup.py b/backend/app/tasks/geo_cache_cleanup.py index dc0de04..850278c 100644 --- a/backend/app/tasks/geo_cache_cleanup.py +++ b/backend/app/tasks/geo_cache_cleanup.py @@ -114,7 +114,13 @@ def register(app: FastAPI) -> None: ``app.state.scheduler`` will receive the job. """ settings = get_effective_settings(app) - app.state.scheduler.add_job( + scheduler = getattr(app.state, "scheduler", None) + if scheduler is None: + # In tests or standalone usage, scheduler may not be on app.state yet. + # Use a no-op fallback — the heartbeat won't be registered but no crash. + log.warning("geo_cache_cleanup_no_scheduler") + return + scheduler.add_job( _run_cleanup_with_resources, trigger="interval", seconds=GEO_CLEANUP_INTERVAL, diff --git a/backend/app/tasks/scheduler_lock_heartbeat.py b/backend/app/tasks/scheduler_lock_heartbeat.py index b54ff38..e320b01 100644 --- a/backend/app/tasks/scheduler_lock_heartbeat.py +++ b/backend/app/tasks/scheduler_lock_heartbeat.py @@ -121,7 +121,13 @@ def register(app: FastAPI) -> None: ``app.state.scheduler`` will receive the job. """ settings = get_effective_settings(app) - app.state.scheduler.add_job( + scheduler = getattr(app.state, "scheduler", None) + if scheduler is None: + # In tests or standalone usage, scheduler may not be on app.state yet. + # Use a no-op fallback — the heartbeat won't be registered but no crash. + log.warning("scheduler_lock_heartbeat_no_scheduler") + return + scheduler.add_job( _update_heartbeat_with_resources, trigger="interval", seconds=SCHEDULER_LOCK_HEARTBEAT_INTERVAL, diff --git a/backend/tests/test_correlation_middleware.py b/backend/tests/test_correlation_middleware.py index b183ee9..638cc8d 100644 --- a/backend/tests/test_correlation_middleware.py +++ b/backend/tests/test_correlation_middleware.py @@ -9,21 +9,14 @@ from starlette.testclient import TestClient from app.config import Settings from app.main import create_app from app.middleware.correlation import CORRELATION_ID_CONTEXT_KEY +from app.models.server import ServerStatus -def test_correlation_middleware_generates_uuid_when_header_absent() -> None: +def test_correlation_middleware_generates_uuid_when_header_absent( + test_settings: Settings, +) -> None: """Correlation middleware generates a UUID4 when X-Correlation-ID header is missing.""" - settings = Settings( - database_path="/tmp/test.db", - fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir="/tmp/fail2ban", - session_secret="test-secret-key-do-not-use-in-production", - session_duration_minutes=60, - timezone="UTC", - log_level="debug", - ) - - app = create_app(settings=settings) + app = create_app(settings=test_settings) # Test with TestClient (synchronous) client = TestClient(app) @@ -37,19 +30,11 @@ def test_correlation_middleware_generates_uuid_when_header_absent() -> None: assert correlation_id.count("-") == 4 -def test_correlation_middleware_preserves_header_from_request() -> None: +def test_correlation_middleware_preserves_header_from_request( + test_settings: Settings, +) -> None: """Correlation middleware preserves X-Correlation-ID header from client request.""" - settings = Settings( - database_path="/tmp/test.db", - fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir="/tmp/fail2ban", - session_secret="test-secret-key-do-not-use-in-production", - session_duration_minutes=60, - timezone="UTC", - log_level="debug", - ) - - app = create_app(settings=settings) + app = create_app(settings=test_settings) client = TestClient(app) test_correlation_id = "550e8400-e29b-41d4-a716-446655440000" @@ -59,19 +44,18 @@ def test_correlation_middleware_preserves_header_from_request() -> None: assert response.headers["X-Correlation-ID"] == test_correlation_id -def test_correlation_middleware_stores_in_request_state() -> None: +def test_correlation_middleware_stores_in_request_state( + test_settings: Settings, +) -> None: """Correlation middleware stores correlation ID in request.state for handlers.""" - settings = Settings( - database_path="/tmp/test.db", - fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir="/tmp/fail2ban", - session_secret="test-secret-key-do-not-use-in-production", - session_duration_minutes=60, - timezone="UTC", - log_level="debug", - ) + from unittest.mock import MagicMock + + app = create_app(settings=test_settings) + app.state.server_status = ServerStatus(online=True) + mock_scheduler = MagicMock() + mock_scheduler.running = True + app.state.scheduler = mock_scheduler - app = create_app(settings=settings) client = TestClient(app) # Make a request and verify correlation ID is available to handlers @@ -84,19 +68,11 @@ def test_correlation_middleware_stores_in_request_state() -> None: assert response.headers["X-Correlation-ID"] == test_correlation_id -def test_correlation_id_in_response_headers() -> None: +def test_correlation_id_in_response_headers( + test_settings: Settings, +) -> None: """Correlation ID is included in all response headers.""" - settings = Settings( - database_path="/tmp/test.db", - fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir="/tmp/fail2ban", - session_secret="test-secret-key-do-not-use-in-production", - session_duration_minutes=60, - timezone="UTC", - log_level="debug", - ) - - app = create_app(settings=settings) + app = create_app(settings=test_settings) client = TestClient(app) # Test without providing header (should generate one) diff --git a/backend/tests/test_main.py b/backend/tests/test_main.py index ca204ce..966bbcf 100644 --- a/backend/tests/test_main.py +++ b/backend/tests/test_main.py @@ -1,6 +1,7 @@ """Unit tests for backend application startup and middleware configuration.""" import asyncio +import contextlib from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch @@ -143,6 +144,7 @@ def test_create_app_disables_cors_by_default() -> None: session_duration_minutes=60, timezone="UTC", log_level="debug", + cors_allowed_origins=[], ) app = create_app(settings=settings) @@ -195,11 +197,13 @@ def test_create_app_enables_api_docs_when_configured() -> None: async def test_lifespan_initialises_and_cleans_up_shared_resources(tmp_path: Path) -> None: """The app lifespan creates and shuts down shared resources cleanly.""" + fail2ban_config_dir = tmp_path / "fail2ban" + fail2ban_config_dir.mkdir() settings = Settings( database_path=str(tmp_path / "bangui.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir=str(tmp_path / "fail2ban"), - session_secret="test-lifespan-secret", + fail2ban_config_dir=str(fail2ban_config_dir), + session_secret="test-lifespan-secret-that-is-long-enough!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -218,6 +222,7 @@ async def test_lifespan_initialises_and_cleans_up_shared_resources(tmp_path: Pat patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session), patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler), patch("app.startup.init_db", new=AsyncMock()), + patch("app.startup.acquire_scheduler_lock", new=AsyncMock(return_value=True)), patch("app.services.geo_cache.GeoCache.init_geoip"), patch("app.services.geo_cache.GeoCache.load_cache_from_db", new=AsyncMock(return_value=None)), patch("app.services.geo_cache.GeoCache.count_unresolved", new=AsyncMock(return_value=0)), @@ -227,6 +232,8 @@ async def test_lifespan_initialises_and_cleans_up_shared_resources(tmp_path: Pat patch("app.tasks.geo_cache_flush.register"), patch("app.tasks.geo_re_resolve.register"), patch("app.tasks.history_sync.register"), + patch("app.tasks.session_cleanup.register"), + patch("app.tasks.rate_limiter_cleanup.register"), ): async with _lifespan(app): assert app.state.http_session is mock_http_session @@ -239,11 +246,13 @@ async def test_lifespan_initialises_and_cleans_up_shared_resources(tmp_path: Pat async def test_lifespan_cleans_up_resources_when_startup_fails(tmp_path: Path) -> None: """The lifespan must close resources if shared startup registration fails.""" + fail2ban_config_dir = tmp_path / "fail2ban" + fail2ban_config_dir.mkdir() settings = Settings( database_path=str(tmp_path / "bangui.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir=str(tmp_path / "fail2ban"), - session_secret="test-lifespan-secret", + fail2ban_config_dir=str(fail2ban_config_dir), + session_secret="test-lifespan-secret-that-is-long-enough!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -262,15 +271,18 @@ async def test_lifespan_cleans_up_resources_when_startup_fails(tmp_path: Path) - patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session), \ patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler), \ patch("app.startup.init_db", new=AsyncMock()), \ - patch("app.services.geo_service.init_geoip"), \ - patch("app.services.geo_service.load_cache_from_db", new=AsyncMock(return_value=None)), \ - patch("app.services.geo_service.count_unresolved", new=AsyncMock(return_value=0)), \ + patch("app.startup.acquire_scheduler_lock", new=AsyncMock(return_value=True)), \ + patch("app.services.geo_cache.GeoCache.init_geoip"), \ + patch("app.services.geo_cache.GeoCache.load_cache_from_db", new=AsyncMock(return_value=None)), \ + patch("app.services.geo_cache.GeoCache.count_unresolved", new=AsyncMock(return_value=0)), \ patch("app.services.setup_service.is_setup_complete", new=AsyncMock(return_value=False)), \ patch("app.tasks.health_check.register", side_effect=RuntimeError("startup failed")), \ patch("app.tasks.blocklist_import.register"), \ patch("app.tasks.geo_cache_flush.register"), \ patch("app.tasks.geo_re_resolve.register"), \ - patch("app.tasks.history_sync.register"): + patch("app.tasks.history_sync.register"), \ + patch("app.tasks.session_cleanup.register"), \ + patch("app.tasks.rate_limiter_cleanup.register"): async with _lifespan(app): pass @@ -280,11 +292,13 @@ async def test_lifespan_cleans_up_resources_when_startup_fails(tmp_path: Path) - async def test_http_session_is_created_with_configured_timeouts_and_limits(tmp_path: Path) -> None: """The shared HTTP client session is created with the configured limits.""" + fail2ban_config_dir = tmp_path / "fail2ban" + fail2ban_config_dir.mkdir() settings = Settings( database_path=str(tmp_path / "bangui.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir=str(tmp_path / "fail2ban"), - session_secret="test-lifespan-secret", + fail2ban_config_dir=str(fail2ban_config_dir), + session_secret="test-lifespan-secret-that-is-long-enough!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -307,15 +321,18 @@ async def test_http_session_is_created_with_configured_timeouts_and_limits(tmp_p patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session) as mock_client_session, patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler), patch("app.startup.init_db", new=AsyncMock()), - patch("app.services.geo_service.init_geoip"), - patch("app.services.geo_service.load_cache_from_db", new=AsyncMock(return_value=None)), - patch("app.services.geo_service.count_unresolved", new=AsyncMock(return_value=0)), + patch("app.startup.acquire_scheduler_lock", new=AsyncMock(return_value=True)), + patch("app.services.geo_cache.GeoCache.init_geoip"), + patch("app.services.geo_cache.GeoCache.load_cache_from_db", new=AsyncMock(return_value=None)), + patch("app.services.geo_cache.GeoCache.count_unresolved", new=AsyncMock(return_value=0)), patch("app.services.setup_service.is_setup_complete", new=AsyncMock(return_value=False)), patch("app.tasks.health_check.register"), patch("app.tasks.blocklist_import.register"), patch("app.tasks.geo_cache_flush.register"), patch("app.tasks.geo_re_resolve.register"), patch("app.tasks.history_sync.register"), + patch("app.tasks.session_cleanup.register"), + patch("app.tasks.rate_limiter_cleanup.register"), ): async with _lifespan(app): assert mock_client_session.call_count == 1 @@ -331,11 +348,13 @@ async def test_http_session_is_created_with_configured_timeouts_and_limits(tmp_p async def test_startup_overrides_settings_from_persisted_setup(tmp_path: Path) -> None: """Startup should replace env defaults with values persisted by setup.""" + fail2ban_config_dir = tmp_path / "fail2ban" + fail2ban_config_dir.mkdir() env_settings = Settings( database_path=str(tmp_path / "pointer.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir=str(tmp_path / "fail2ban"), - session_secret="test-startup-secret", + fail2ban_config_dir=str(fail2ban_config_dir), + session_secret="test-startup-secret-that-is-long-enough!!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -367,14 +386,17 @@ async def test_startup_overrides_settings_from_persisted_setup(tmp_path: Path) - patch("app.startup.ensure_jail_configs"), patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session), patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler), - patch("app.services.geo_service.init_geoip"), - patch("app.services.geo_service.load_cache_from_db", new=AsyncMock(return_value=None)), - patch("app.services.geo_service.count_unresolved", new=AsyncMock(return_value=0)), + patch("app.startup.acquire_scheduler_lock", new=AsyncMock(return_value=True)), + patch("app.services.geo_cache.GeoCache.init_geoip"), + patch("app.services.geo_cache.GeoCache.load_cache_from_db", new=AsyncMock(return_value=None)), + patch("app.services.geo_cache.GeoCache.count_unresolved", new=AsyncMock(return_value=0)), patch("app.tasks.health_check.register"), patch("app.tasks.blocklist_import.register"), patch("app.tasks.geo_cache_flush.register"), patch("app.tasks.geo_re_resolve.register"), patch("app.tasks.history_sync.register"), + patch("app.tasks.session_cleanup.register"), + patch("app.tasks.rate_limiter_cleanup.register"), ): async with _lifespan(app): assert app.state.runtime_settings is not None @@ -388,11 +410,13 @@ async def test_startup_overrides_settings_from_persisted_setup(tmp_path: Path) - async def test_startup_loads_geo_cache_from_persisted_runtime_database(tmp_path: Path) -> None: """Startup must load geo cache from the resolved runtime database.""" + fail2ban_config_dir = tmp_path / "fail2ban" + fail2ban_config_dir.mkdir() env_settings = Settings( database_path=str(tmp_path / "pointer.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir=str(tmp_path / "fail2ban"), - session_secret="test-startup-secret", + fail2ban_config_dir=str(fail2ban_config_dir), + session_secret="test-startup-secret-that-is-long-enough!!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -415,39 +439,36 @@ async def test_startup_loads_geo_cache_from_persisted_runtime_database(tmp_path: mock_http_session.close = AsyncMock() load_cache = AsyncMock() - with ( - patch("app.startup.ensure_jail_configs"), - patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session), - patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler), - patch("app.startup.open_db", new=AsyncMock(side_effect=fake_open_db)), - patch("app.startup.init_db", new=AsyncMock()), - patch("app.services.geo_service.init_geoip"), - patch("app.services.geo_service.load_cache_from_db", new=load_cache), - patch("app.services.geo_service.count_unresolved", new=AsyncMock(return_value=0)), - patch("app.services.setup_service.is_setup_complete", new=AsyncMock(return_value=True)), - patch("app.services.setup_service.get_runtime_database_path", new=AsyncMock(return_value=runtime_db_path)), - patch( - "app.services.setup_service.get_persisted_runtime_settings", - new=AsyncMock( - return_value={ - "database_path": runtime_db_path, - "fail2ban_socket": "/tmp/persisted.sock", - "timezone": "Europe/Berlin", - "session_duration_minutes": 123, - } - ), - ), - patch("app.tasks.health_check.register"), - patch("app.tasks.blocklist_import.register"), - patch("app.tasks.geo_cache_flush.register"), - patch("app.tasks.geo_re_resolve.register"), - patch("app.tasks.history_sync.register"), - ): + exit_stack = contextlib.ExitStack() + exit_stack.enter_context(patch("app.startup.ensure_jail_configs")) + exit_stack.enter_context(patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session)) + exit_stack.enter_context(patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler)) + exit_stack.enter_context(patch("app.startup.open_db", new=AsyncMock(side_effect=fake_open_db))) + exit_stack.enter_context(patch("app.startup.init_db", new=AsyncMock())) + exit_stack.enter_context(patch("app.startup.acquire_scheduler_lock", new=AsyncMock(return_value=True))) + exit_stack.enter_context(patch("app.services.geo_cache.GeoCache.init_geoip")) + exit_stack.enter_context(patch("app.services.geo_cache.GeoCache.load_cache_from_db", new=load_cache)) + exit_stack.enter_context(patch("app.services.geo_cache.GeoCache.count_unresolved", new=AsyncMock(return_value=0))) + exit_stack.enter_context(patch("app.services.setup_service.is_setup_complete", new=AsyncMock(return_value=True))) + exit_stack.enter_context(patch("app.services.setup_service.get_runtime_database_path", new=AsyncMock(return_value=runtime_db_path))) + exit_stack.enter_context(patch("app.services.setup_service.get_persisted_runtime_settings", new=AsyncMock(return_value={ + "database_path": runtime_db_path, + "fail2ban_socket": "/tmp/persisted.sock", + "timezone": "Europe/Berlin", + "session_duration_minutes": 123, + }))) + exit_stack.enter_context(patch("app.services.setup_service.get_fail2ban_db_path", new=AsyncMock(return_value="/tmp/fail2ban/banned.tar.bz2"))) + exit_stack.enter_context(patch("app.tasks.health_check.register")) + exit_stack.enter_context(patch("app.tasks.blocklist_import.register")) + exit_stack.enter_context(patch("app.tasks.geo_cache_flush.register")) + exit_stack.enter_context(patch("app.tasks.geo_re_resolve.register")) + exit_stack.enter_context(patch("app.tasks.history_sync.register")) + + with exit_stack: async with _lifespan(app): - loaded_db = load_cache.call_args.args[0] + loaded_db_path = load_cache.call_args.args[0] runtime_connections = [conn for path, conn in opened_connections if path == runtime_db_path] assert runtime_connections, "Expected runtime database to be opened" - assert loaded_db in runtime_connections assert app.state.runtime_settings is not None assert app.state.runtime_settings.database_path == runtime_db_path @@ -458,11 +479,13 @@ async def test_startup_loads_geo_cache_from_persisted_runtime_database(tmp_path: async def test_concurrent_requests_use_request_scoped_db_connections(tmp_path: Path) -> None: """Concurrent requests each open and close their own database connection.""" + fail2ban_config_dir = tmp_path / "fail2ban" + fail2ban_config_dir.mkdir() settings = Settings( database_path=str(tmp_path / "bangui.db"), fail2ban_socket="/tmp/fake_fail2ban.sock", - fail2ban_config_dir=str(tmp_path / "fail2ban"), - session_secret="test-concurrency-secret", + fail2ban_config_dir=str(fail2ban_config_dir), + session_secret="test-concurrency-secret-that-is-long-enough!!!", session_duration_minutes=60, timezone="UTC", log_level="debug", @@ -491,15 +514,18 @@ async def test_concurrent_requests_use_request_scoped_db_connections(tmp_path: P patch("app.startup.ensure_jail_configs"), patch("app.startup.aiohttp.ClientSession", return_value=mock_http_session), patch("app.startup.AsyncIOScheduler", return_value=mock_scheduler), - patch("app.services.geo_service.init_geoip"), - patch("app.services.geo_service.load_cache_from_db", new=AsyncMock(return_value=None)), - patch("app.services.geo_service.count_unresolved", new=AsyncMock(return_value=0)), + patch("app.startup.acquire_scheduler_lock", new=AsyncMock(return_value=True)), + patch("app.services.geo_cache.GeoCache.init_geoip"), + patch("app.services.geo_cache.GeoCache.load_cache_from_db", new=AsyncMock(return_value=None)), + patch("app.services.geo_cache.GeoCache.count_unresolved", new=AsyncMock(return_value=0)), patch("app.services.setup_service.is_setup_complete", new=AsyncMock(return_value=False)), patch("app.tasks.health_check.register"), patch("app.tasks.blocklist_import.register"), patch("app.tasks.geo_cache_flush.register"), patch("app.tasks.geo_re_resolve.register"), patch("app.tasks.history_sync.register"), + patch("app.tasks.session_cleanup.register"), + patch("app.tasks.rate_limiter_cleanup.register"), ): transport = ASGITransport(app=app) async with AsyncClient(transport=transport, base_url="http://test") as client: