From 3f9669f7e5eeb6bb4a8a9d4828ea45256319bcc8 Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 29 Aug 2026 22:40:19 +0100 Subject: [PATCH] fix: readiness 503s, maintenance gating, cache invalidation, and webhook/engine hardening - /health/ready returns 503 (not 200) when not ready; body no longer leaks raw backend error text (api + web tiers) - SYSTEM_MAINTENANCE now gates the web /api/* proxy (503 MAINTENANCE) instead of only hiding the SPA UI - channel mutations invalidate messages/packets/packet_groups/dashboard caches so visibility/key changes apply immediately (was stale up to 1h) - async DB engine: pool_size/max_overflow/pre_ping matching the sync engine (survives Postgres restarts); adispose() awaited from the API lifespan (async connections were leaked by the sync dispose) - webhooks: permanent 4xx (except 408/429) fail fast instead of retrying; dispatch queue bounded at 1000 with drop-oldest + warning; shutdown logs dropped events; matches_event evaluated once - SPA: apiGet/apiPostForm redirect to OIDC login on 401; chart date labels and route timestamps rendered in UTC/configured timezone - e2e compose collector healthcheck uses 'meshcore-hub health collector' (image has no pgrep) - docs/upgrading.md: fold unreleased notes into v0.18.0 and document the above --- docs/upgrading.md | 25 +++++- e2e/docker-compose.test.yml | 3 +- src/meshcore_hub/api/app.py | 18 +++-- src/meshcore_hub/api/cache_invalidation.py | 18 ++++- src/meshcore_hub/collector/subscriber.py | 30 +++++++- src/meshcore_hub/collector/webhook.py | 47 ++++++----- src/meshcore_hub/common/database.py | 55 ++++++++++++- src/meshcore_hub/web/app.py | 25 ++++-- .../web/static/js/spa-react/pages/Routes.tsx | 8 +- .../web/static/js/spa-react/utils/api.test.ts | 56 ++++++++++++++ .../web/static/js/spa-react/utils/api.ts | 2 + .../static/js/spa-react/utils/charts.test.ts | 3 +- .../web/static/js/spa-react/utils/charts.ts | 7 +- tests/test_api/test_cache.py | 61 +++++++++++++-- tests/test_collector/test_subscriber.py | 11 +++ tests/test_collector/test_webhook.py | 45 +++++++++++ tests/test_common/test_database.py | 77 ++++++++++++++++++- tests/test_web/conftest.py | 12 ++- tests/test_web/test_app.py | 19 +++++ tests/test_web/test_health.py | 21 ++++- 20 files changed, 489 insertions(+), 54 deletions(-) create mode 100644 src/meshcore_hub/web/static/js/spa-react/utils/api.test.ts diff --git a/docs/upgrading.md b/docs/upgrading.md index a762d99..4dff2bd 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -2,7 +2,9 @@ This guide covers upgrading from a previous MeshCore Hub release to the current version. Check the relevant version section below before upgrading. -## Unreleased — Security hardening (fail-closed defaults) +## v0.18.0 + +### Security hardening (fail-closed defaults) Four behaviour changes tighten default security. **Two are potentially breaking for existing deployments — read before upgrading.** @@ -18,7 +20,7 @@ Four behaviour changes tighten default security. **Two are potentially breaking **CORS credentials tightened.** An unset `CORS_ORIGINS` still allows all origins for compatibility with external API consumers, but no longer advertises `allow-credentials` (wildcard origins are never valid with credentials in browsers anyway). Explicit origin lists are unchanged. -## Unreleased — Route evaluator performance remediation +### Route evaluator performance remediation The background route evaluator was triggering a multi-second `packet_path_hops` scan per route per sweep, flooding the slow-query log and blocking route saves on the same scan. This release reduces load across four orthogonal levers. **Two are destructive / behaviour-changing — read before upgrading.** @@ -34,6 +36,25 @@ The background route evaluator was triggering a multi-second `packet_path_hops` **Performance rationale:** EXPLAIN analysis showed both candidate query plans (Merge Join full scan and forced Nested Loop) cost ~10–15 s at 6.4 M `packet_path_hops` rows; no SQL rewrite, stats change, or CTE trick could avoid the scan. The fix is purely data-volume pressure: smaller table (retention), smaller candidate set (window cap), index-only scan (covering index), and less frequent sweeps. See `docs/routes.md` for the updated evaluator contract. +### Operational hardening (health probes, maintenance mode, webhooks) + +A set of correctness fixes around readiness, maintenance mode, and webhook delivery. **One change affects monitoring setups — read before upgrading.** + +**⚠️ Monitoring change: `/health/ready` now returns 503 when not ready.** Both the API and web tiers previously answered HTTP 200 with a `{"status": "not_ready", ...}` body while the database/API backend was down — probes that judge by status code (Docker healthchecks, Kubernetes readiness, most load balancers) kept the instance in rotation. The endpoint now returns **503** when not ready and 200 when ready. **Action:** only needed if you run a custom probe that parsed the response body while relying on the 200 — switch it to status-code semantics. The body also no longer echoes raw backend error text (just the exception type). + +**Maintenance mode now actually blocks API calls.** `SYSTEM_MAINTENANCE=true` was documented to "prevent all API calls", but only the React UI was gated — the web tier's `/api/*` proxy kept forwarding to the backend. The proxy now returns `503 {"detail": "Site is in maintenance mode", "code": "MAINTENANCE"}` for every API request while maintenance mode is on. **Action:** scripts or automation that call the API through the web tier during maintenance windows will now see 503s — which is the intended behaviour; schedule them outside the window. + +**Webhook delivery: fail-fast on permanent errors, bounded queue.** + +- Permanent client errors (any 4xx except `408`/`429` — e.g. a mistyped URL, expired auth, or rejected payload) are **no longer retried**. Previously every non-2xx response burned the full retry cycle (up to 4 attempts with backoff), so one misconfigured endpoint stalled the dispatch pipeline for ~14–45 s per event. Server errors (5xx), timeouts, and transport failures are still retried as before. +- The internal webhook queue is now **capped at 1000 events**. When endpoints are down or too slow, the oldest queued events are dropped with a `WARNING` log (including the dropped count) instead of growing memory without bound. Nothing changes on healthy meshes. + +**Channel changes invalidate dependent caches.** Creating, updating, or deleting a channel now also drops the cached `messages`, `packets`, `packet-groups`, and dashboard responses (previously only the channels list itself). A channel demoted from community to member visibility — or deleted — no longer serves its messages to lower-privileged roles for up to the remaining cache TTL (up to 1 h for the dashboard). No action required. + +**API survives Postgres restarts (async engine hardening).** The API's async connection pool now matches the sync engine's sizing (`pool_size=20`, `max_overflow=30`) and uses `pool_pre_ping`, so a Postgres restart or failover no longer leaves stale pooled connections that error as 500s until they churn. Shutdown also correctly closes async-pooled connections (previously leaked). No action required; SQLite deployments are unchanged. + +**Web UI fixes.** Dashboard chart date labels and route-match timestamps now render in the configured timezone — chart labels were off by one day for users in negative UTC offsets, and route-match times were off by the user's UTC offset. Expired OIDC sessions now redirect to the login flow on any API GET (previously only writes did; reads spun forever). No action required. + ## v0.17.0 ### React Web UI diff --git a/e2e/docker-compose.test.yml b/e2e/docker-compose.test.yml index 35fe72c..fe0e13f 100644 --- a/e2e/docker-compose.test.yml +++ b/e2e/docker-compose.test.yml @@ -135,7 +135,8 @@ services: - DATABASE_PASSWORD=e2epassword command: ["collector"] healthcheck: - test: ["CMD", "pgrep", "-f", "meshcore-hub"] + # Same probe as the main compose file — the image has no pgrep. + test: ["CMD", "meshcore-hub", "health", "collector"] interval: 5s timeout: 5s retries: 3 diff --git a/src/meshcore_hub/api/app.py b/src/meshcore_hub/api/app.py index 6fcb160..4b3d1be 100644 --- a/src/meshcore_hub/api/app.py +++ b/src/meshcore_hub/api/app.py @@ -6,6 +6,7 @@ from typing import Any, AsyncGenerator from fastapi import FastAPI, Request from fastapi.middleware.cors import CORSMiddleware +from fastapi.responses import JSONResponse from sqlalchemy import text from meshcore_hub import __version__ @@ -63,7 +64,9 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: if _cache is not None and hasattr(_cache, "close"): _cache.close() if _db_manager: - _db_manager.dispose() + # adispose: the async engine's pooled connections must be closed + # from this (running) event loop. + await _db_manager.adispose() _db_manager = None logger.info("Database connection closed") @@ -288,15 +291,19 @@ def create_app( return {"status": "healthy", "version": __version__} @app.get("/health/ready", tags=["Health"]) - def health_ready() -> dict: - """Readiness check including database and optional Redis.""" + def health_ready() -> JSONResponse: + """Readiness check including database and optional Redis. + + Returns 503 when not ready so Docker/Kubernetes readiness probes + (which judge by HTTP status code) pull the instance from rotation. + """ try: db = get_db_manager() with db.session_scope() as session: session.execute(text("SELECT 1")) result: dict[str, str] = {"status": "ready", "database": "connected"} except Exception as e: - result = {"status": "not_ready", "database": str(e)} + result = {"status": "not_ready", "database": type(e).__name__} redis_cache = getattr(app.state, "redis_cache", None) redis_enabled = getattr(app.state, "redis_enabled", False) @@ -306,7 +313,8 @@ def create_app( else: result["redis"] = "unreachable" - return result + status_code = 200 if result.get("status") == "ready" else 503 + return JSONResponse(content=result, status_code=status_code) return app diff --git a/src/meshcore_hub/api/cache_invalidation.py b/src/meshcore_hub/api/cache_invalidation.py index 62d0ac7..48a770e 100644 --- a/src/meshcore_hub/api/cache_invalidation.py +++ b/src/meshcore_hub/api/cache_invalidation.py @@ -79,8 +79,24 @@ def _drop(request: Request, prefix: str) -> None: def invalidate_channels(request: Request) -> None: - """Drop cached ``GET /channels`` responses (role-aware, URL-path keys).""" + """Drop cached ``GET /channels`` responses (role-aware, URL-path keys). + + Channel visibility also drives role-based redaction and per-channel + breakdowns in several other cached endpoints, so they must be dropped + alongside the channel list or they serve stale data until their own TTL + expires (up to one hour for the dashboard): + + - ``GET /messages`` — visible-channel filtering (URL-path keys) + - ``GET /packets`` and ``GET /packet-groups`` — channel/event metadata + derived from channel visibility (endpoint-name-style prefixes) + - ``GET /dashboard/*`` — ``channel_message_counts`` in ``stats`` and the + per-channel series in ``message-activity`` + """ _drop(request, "/api/v1/channels") + _drop(request, "/api/v1/messages") + _drop(request, "packets") + _drop(request, "packet_groups") + invalidate_dashboard(request) def invalidate_routes(request: Request) -> None: diff --git a/src/meshcore_hub/collector/subscriber.py b/src/meshcore_hub/collector/subscriber.py index 0151658..282ae4f 100644 --- a/src/meshcore_hub/collector/subscriber.py +++ b/src/meshcore_hub/collector/subscriber.py @@ -15,6 +15,7 @@ import signal import threading import time import uuid +from collections import deque from datetime import datetime, timezone from typing import Any, Callable, Optional, TYPE_CHECKING @@ -80,8 +81,11 @@ class Subscriber(LetsMeshNormalizer): self._handlers: dict[str, EventHandler] = {} self._db_connected = False self._health_reporter: Optional[HealthReporter] = None - # Webhook processing - self._webhook_queue: list[tuple[str, dict[str, Any], str]] = [] + # Webhook processing. The queue is bounded: a dead endpoint slows + # the dispatch thread, and an unbounded queue would grow without + # limit on a busy mesh — drop the oldest events instead. + self._webhook_queue: deque[tuple[str, dict[str, Any], str]] = deque() + self._webhook_queue_max: int = 1000 self._webhook_lock = threading.Lock() self._webhook_thread: Optional[threading.Thread] = None # Data cleanup @@ -377,6 +381,18 @@ class Subscriber(LetsMeshNormalizer): public_key: Source node public key """ with self._webhook_lock: + if len(self._webhook_queue) >= self._webhook_queue_max: + dropped = len(self._webhook_queue) - self._webhook_queue_max + 1 + # Drop oldest: the slowest path to recovery is a backlog of + # stale events; newest events are the most useful. + while len(self._webhook_queue) >= self._webhook_queue_max: + self._webhook_queue.popleft() + logger.warning( + "Webhook queue full (%d events); dropped %d oldest " + "events (webhook endpoint too slow or down?)", + self._webhook_queue_max, + dropped, + ) self._webhook_queue.append((event_type, payload, public_key)) def _start_webhook_processor(self) -> None: @@ -398,7 +414,7 @@ class Subscriber(LetsMeshNormalizer): while self._running: # Get queued events - events_to_process: list[tuple[str, dict[str, Any], str]] = [] + events_to_process: deque[tuple[str, dict[str, Any], str]] = deque() with self._webhook_lock: if self._webhook_queue: events_to_process = self._webhook_queue.copy() @@ -419,6 +435,14 @@ class Subscriber(LetsMeshNormalizer): finally: loop.run_until_complete(dispatcher.stop()) loop.close() + with self._webhook_lock: + if self._webhook_queue: + logger.warning( + "Webhook processor stopped with %d events still " + "queued (dropped)", + len(self._webhook_queue), + ) + self._webhook_queue.clear() logger.info("Webhook processor stopped") self._webhook_thread = threading.Thread( diff --git a/src/meshcore_hub/collector/webhook.py b/src/meshcore_hub/collector/webhook.py index 3b7bb32..0471b35 100644 --- a/src/meshcore_hub/collector/webhook.py +++ b/src/meshcore_hub/collector/webhook.py @@ -243,24 +243,19 @@ class WebhookDispatcher: results: dict[str, bool] = {} - # Dispatch to all matching webhooks concurrently - tasks = [] - for webhook in self.webhooks: - if not webhook.enabled: - continue - if webhook.matches_event(event_type, payload): - tasks.append(self._send_webhook(webhook, event_data)) + # Dispatch to all matching webhooks concurrently. Evaluate + # matches_event once — re-running it for the zip would double the + # filter work and risk desynchronizing results from tasks. + matched = [ + webhook + for webhook in self.webhooks + if webhook.enabled and webhook.matches_event(event_type, payload) + ] + tasks = [self._send_webhook(webhook, event_data) for webhook in matched] if tasks: task_results = await asyncio.gather(*tasks, return_exceptions=True) - for webhook, result in zip( - [ - w - for w in self.webhooks - if w.enabled and w.matches_event(event_type, payload) - ], - task_results, - ): + for webhook, result in zip(matched, task_results): if isinstance(result, Exception): results[webhook.name] = False logger.error(f"Webhook {webhook.name} failed: {result}") @@ -312,11 +307,25 @@ class WebhookDispatcher: f"(status={response.status_code})" ) return True - else: - logger.warning( - f"Webhook {webhook.name} returned status {response.status_code}" + + last_error = Exception(f"HTTP {response.status_code}") + + # Permanent client errors (bad URL, auth, payload rejected) + # will never succeed on retry — fail fast instead of + # multiplying the outage window. 408/429 are retryable. + if 400 <= response.status_code < 500 and response.status_code not in ( + 408, + 429, + ): + logger.error( + f"Webhook {webhook.name} returned status " + f"{response.status_code}; not retryable" ) - last_error = Exception(f"HTTP {response.status_code}") + return False + + logger.warning( + f"Webhook {webhook.name} returned status " f"{response.status_code}" + ) except httpx.TimeoutException as e: logger.warning(f"Webhook {webhook.name} timed out: {e}") diff --git a/src/meshcore_hub/common/database.py b/src/meshcore_hub/common/database.py index 02ebfd3..f875daf 100644 --- a/src/meshcore_hub/common/database.py +++ b/src/meshcore_hub/common/database.py @@ -1,5 +1,7 @@ """Database connection and session management.""" +import asyncio +import logging import os from contextlib import asynccontextmanager, contextmanager from typing import Any, AsyncGenerator, Generator @@ -11,6 +13,8 @@ from sqlalchemy.orm import Session, sessionmaker from meshcore_hub.common.models.base import Base +logger = logging.getLogger(__name__) + def _resolve_pg_schema(database_url: str, schema: str | None) -> str | None: """Resolve the Postgres schema to scope a connection to (search_path). @@ -212,8 +216,25 @@ class DatabaseManager: if self._schema: server_settings["search_path"] = self._schema async_connect_args["server_settings"] = server_settings + + # Mirror the sync engine's pool configuration (see + # create_database_engine): without pool_pre_ping a Postgres restart + # leaves stale asyncpg connections in the pool that error on next + # use. In-memory SQLite uses a non-overflow pool, so skip there. + is_memory_sqlite = self.database_url in ( + "sqlite+aiosqlite://", + "sqlite+aiosqlite:///:memory:", + ) + async_engine_kwargs: dict[str, Any] = {"pool_pre_ping": True} + if not is_memory_sqlite: + async_engine_kwargs["pool_size"] = 20 + async_engine_kwargs["max_overflow"] = 30 + self._async_engine = create_async_engine( - async_url, echo=self._echo, connect_args=async_connect_args + async_url, + echo=self._echo, + connect_args=async_connect_args, + **async_engine_kwargs, ) # Apply the same SQLite pragmas as the sync engine (see @@ -293,10 +314,36 @@ class DatabaseManager: yield session def dispose(self) -> None: - """Dispose of the database engine and connection pool.""" - self.engine.dispose() + """Dispose of the database engines and connection pools. + + The async engine's pooled connections can only be closed from a + running event loop. When this method is called outside any loop + (e.g. a CLI that used ``async_session`` via ``asyncio.run``), the + async engine is disposed on a temporary loop. Inside a running + loop, use :meth:`adispose` instead — the async engine is then left + for that call and a warning is logged. + """ if self._async_engine is not None: - self._async_engine.sync_engine.dispose() + try: + asyncio.get_running_loop() + except RuntimeError: + asyncio.run(self._async_engine.dispose()) + self._async_engine = None + self._async_session_factory = None + else: + logger.warning( + "dispose() called inside a running event loop; async " + "engine not closed — use adispose()" + ) + self.engine.dispose() + + async def adispose(self) -> None: + """Dispose both engines from an async context (e.g. app lifespan).""" + if self._async_engine is not None: + await self._async_engine.dispose() + self._async_engine = None + self._async_session_factory = None + self.engine.dispose() # Global database manager instance (initialized at runtime) diff --git a/src/meshcore_hub/web/app.py b/src/meshcore_hub/web/app.py index d834a03..6777298 100644 --- a/src/meshcore_hub/web/app.py +++ b/src/meshcore_hub/web/app.py @@ -828,6 +828,11 @@ def create_app( ) async def api_proxy(request: Request, path: str) -> Response: """Proxy API requests to the backend API server.""" + if getattr(request.app.state, "system_maintenance", False): + return JSONResponse( + {"detail": "Site is in maintenance mode", "code": "MAINTENANCE"}, + status_code=503, + ) oidc_enabled = getattr(request.app.state, "oidc_enabled", False) user_roles: frozenset[str] = frozenset() user_id: str | None = None @@ -1129,15 +1134,25 @@ def create_app( return {"status": "healthy", "version": __version__} @app.get("/health/ready", tags=["Health"]) - async def health_ready(request: Request) -> dict: - """Readiness check including API connectivity.""" + async def health_ready(request: Request) -> JSONResponse: + """Readiness check including API connectivity. + + Returns 503 when not ready so Docker/Kubernetes readiness probes + (which judge by HTTP status code) pull the instance from rotation. + """ try: response = await request.app.state.http_client.get("/health") if response.status_code == 200: - return {"status": "ready", "api": "connected"} - return {"status": "not_ready", "api": f"status {response.status_code}"} + return JSONResponse({"status": "ready", "api": "connected"}) + return JSONResponse( + {"status": "not_ready", "api": f"status {response.status_code}"}, + status_code=503, + ) except Exception as e: - return {"status": "not_ready", "api": str(e)} + return JSONResponse( + {"status": "not_ready", "api": type(e).__name__}, + status_code=503, + ) # --- SEO Endpoints --- def _get_https_base_url(request: Request) -> str: diff --git a/src/meshcore_hub/web/static/js/spa-react/pages/Routes.tsx b/src/meshcore_hub/web/static/js/spa-react/pages/Routes.tsx index 6728886..12f8541 100644 --- a/src/meshcore_hub/web/static/js/spa-react/pages/Routes.tsx +++ b/src/meshcore_hub/web/static/js/spa-react/pages/Routes.tsx @@ -11,6 +11,7 @@ import { Link, useNavigate, useSearchParams } from "react-router"; import { useAppConfig, hasRole } from "@/context/AppConfigContext"; import { apiGet, apiPost, apiPut, apiDelete } from "@/utils/api"; +import { useFormatDateTime } from "@/utils/format"; import { qk, invalidate } from "@/utils/queryKeys"; import { usePageTitle } from "@/hooks/usePageTitle"; import { Loading, ErrorAlert } from "@/components/Alerts"; @@ -372,6 +373,7 @@ function MatchRow({ onNavigate: (url: string) => void; }) { const { t } = useTranslation(); + const { formatDateTime } = useFormatDateTime(); const prefixLen = 2 * (route.match_width || 1); const pathLookup = new Map( (route.route_nodes || []).map((rn) => [ @@ -439,7 +441,7 @@ function MatchRow({ })} {match.received_at && ( - {new Date(match.received_at).toLocaleString()} + {formatDateTime(match.received_at)} )} @@ -474,9 +476,9 @@ function DetailContent({ {i === historyData.length - 1 ? t("common.now") - : new Date(`${d.date}T00:00:00`).toLocaleDateString( + : new Date(`${d.date}T00:00:00Z`).toLocaleDateString( undefined, - { day: "2-digit", month: "2-digit" }, + { day: "2-digit", month: "2-digit", timeZone: "UTC" }, )} ))} diff --git a/src/meshcore_hub/web/static/js/spa-react/utils/api.test.ts b/src/meshcore_hub/web/static/js/spa-react/utils/api.test.ts new file mode 100644 index 0000000..7650d27 --- /dev/null +++ b/src/meshcore_hub/web/static/js/spa-react/utils/api.test.ts @@ -0,0 +1,56 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { apiGet } from "./api"; + +function setOidcEnabled(enabled: boolean) { + window.__APP_CONFIG__ = { + ...(window.__APP_CONFIG__ ?? {}), + oidc_enabled: enabled, + } as typeof window.__APP_CONFIG__; +} + +describe("apiGet auth redirect", () => { + beforeEach(() => { + // A real URL object's href setter rejects relative URLs, so stub a + // minimal window.location instead. + const locationStub = { + href: "http://localhost/nodes?page=2", + pathname: "/nodes", + search: "?page=2", + origin: "http://localhost", + }; + vi.stubGlobal("location", locationStub); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("redirects to the login flow on 401 when OIDC is enabled", async () => { + setOidcEnabled(true); + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue( + new Response("unauthorized", { status: 401 }), + ), + ); + + await expect(apiGet("/api/v1/nodes")).rejects.toThrow("API error: 401"); + expect(window.location.href).toBe( + "/auth/login?next=%2Fnodes%3Fpage%3D2", + ); + }); + + it("does not redirect on 401 when OIDC is disabled", async () => { + setOidcEnabled(false); + const originalHref = window.location.href; + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue( + new Response("unauthorized", { status: 401 }), + ), + ); + + await expect(apiGet("/api/v1/nodes")).rejects.toThrow("API error: 401"); + expect(window.location.href).toBe(originalHref); + }); +}); diff --git a/src/meshcore_hub/web/static/js/spa-react/utils/api.ts b/src/meshcore_hub/web/static/js/spa-react/utils/api.ts index 1b89017..4180185 100644 --- a/src/meshcore_hub/web/static/js/spa-react/utils/api.ts +++ b/src/meshcore_hub/web/static/js/spa-react/utils/api.ts @@ -30,6 +30,7 @@ export async function apiGet( } } const response = await fetch(url, { signal }); + checkAuthResponse(response); if (!response.ok) { throw new Error(`API error: ${response.status} ${response.statusText}`); } @@ -91,6 +92,7 @@ export async function apiPostForm( headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: body.toString(), }); + checkAuthResponse(response); if (!response.ok) { const text = await response.text(); throw new Error(`API error: ${response.status} - ${text}`); diff --git a/src/meshcore_hub/web/static/js/spa-react/utils/charts.test.ts b/src/meshcore_hub/web/static/js/spa-react/utils/charts.test.ts index 08e3e37..81540bf 100644 --- a/src/meshcore_hub/web/static/js/spa-react/utils/charts.test.ts +++ b/src/meshcore_hub/web/static/js/spa-react/utils/charts.test.ts @@ -18,9 +18,10 @@ import { const t = ((key: string) => key) as unknown as TFunction; const dayLabel = (date: string) => - new Date(date).toLocaleDateString("en-GB", { + new Date(`${date}T00:00:00Z`).toLocaleDateString("en-GB", { day: "numeric", month: "short", + timeZone: "UTC", }); const series = (counts: number[]): ActivitySeries => ({ diff --git a/src/meshcore_hub/web/static/js/spa-react/utils/charts.ts b/src/meshcore_hub/web/static/js/spa-react/utils/charts.ts index 0627da6..5a72447 100644 --- a/src/meshcore_hub/web/static/js/spa-react/utils/charts.ts +++ b/src/meshcore_hub/web/static/js/spa-react/utils/charts.ts @@ -183,11 +183,16 @@ function createChartOptions(showLegend: boolean): ChartOptions<"line"> { } function formatDateLabels(data: { date: string }[]): string[] { + // Backend buckets are UTC date-only strings ("%Y-%m-%d"). Parse them as + // UTC and format with an explicit timeZone so users in negative UTC + // offsets don't see every label shifted to the previous day. return data.map((d) => { - const date = new Date(d.date); + const date = new Date(`${d.date}T00:00:00Z`); + if (Number.isNaN(date.getTime())) return d.date; return date.toLocaleDateString("en-GB", { day: "numeric", month: "short", + timeZone: "UTC", }); }); } diff --git a/tests/test_api/test_cache.py b/tests/test_api/test_cache.py index 77c0d94..d3d9d99 100644 --- a/tests/test_api/test_cache.py +++ b/tests/test_api/test_cache.py @@ -1,7 +1,7 @@ """Tests for API cache layer.""" import json -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest from fastapi import FastAPI, Request @@ -1012,7 +1012,9 @@ class TestLifespanRedis: app.state.redis_enabled = False app_module._db_manager = None - with patch.object(app_module, "DatabaseManager", return_value=MagicMock()): + mock_db = MagicMock() + mock_db.adispose = AsyncMock() + with patch.object(app_module, "DatabaseManager", return_value=mock_db): async with lifespan(app): assert isinstance(app.state.redis_cache, NullCache) @@ -1030,7 +1032,9 @@ class TestLifespanRedis: app.state.redis_key_prefix = "myprefix" app_module._db_manager = None - with patch.object(app_module, "DatabaseManager", return_value=MagicMock()): + mock_db = MagicMock() + mock_db.adispose = AsyncMock() + with patch.object(app_module, "DatabaseManager", return_value=mock_db): with patch("meshcore_hub.common.redis.RedisCacheBackend") as mock_cls: mock_instance = MagicMock() mock_cls.return_value = mock_instance @@ -1054,6 +1058,7 @@ class TestLifespanRedis: mock_cache = MagicMock() mock_db_manager = MagicMock() + mock_db_manager.adispose = AsyncMock() app_module._db_manager = None with patch.object(app_module, "DatabaseManager", return_value=mock_db_manager): @@ -1064,7 +1069,7 @@ class TestLifespanRedis: async with lifespan(app): pass mock_cache.close.assert_called_once() - mock_db_manager.dispose.assert_called_once() + mock_db_manager.adispose.assert_awaited_once() class TestXCacheMiddleware: @@ -1121,6 +1126,25 @@ class TestHealthReadyRedis: assert response.status_code == 200 assert response.json()["redis"] == "unreachable" + def test_health_ready_returns_503_when_db_down(self, client_no_auth): + """Readiness probes judge by status code: DB down must be a 503.""" + import meshcore_hub.api.app as app_module + + def _raise(): + raise RuntimeError("db down") + + original = app_module.get_db_manager + app_module.get_db_manager = _raise + try: + response = client_no_auth.get("/health/ready") + finally: + app_module.get_db_manager = original + assert response.status_code == 503 + data = response.json() + assert data["status"] == "not_ready" + # Error detail is reduced to the exception type, not the raw message. + assert data["database"] == "RuntimeError" + class TestCliRedis: def test_redis_enabled_shows_banner(self): @@ -1414,7 +1438,19 @@ class TestCacheInvalidationHelpers: cache = MagicMock() invalidate_channels(_make_request_with_cache(cache)) - cache.delete.assert_called_once_with("/api/v1/channels") + # Channel list itself (URL-path keys)... + cache.delete.assert_any_call("/api/v1/channels") + # ...plus every dependent namespace: messages/packets redaction is + # visibility-driven and dashboard counts embed per-channel series. + for prefix in ( + "/api/v1/messages", + "packets", + "packet_groups", + "dashboard", + "/api/v1/dashboard", + ): + cache.delete.assert_any_call(prefix) + assert cache.delete.call_count == 6 def test_invalidate_routes_drops_url_path_prefix(self): from meshcore_hub.api.cache_invalidation import invalidate_routes @@ -1504,6 +1540,18 @@ class TestMutationInvalidationIntegration: # --- Channels --------------------------------------------------------- + def _assert_channel_dependent_drops(self, mock_cache: MagicMock) -> None: + """Channel visibility drives redaction/counts in these cached reads.""" + for prefix in ( + "/api/v1/channels", + "/api/v1/messages", + "packets", + "packet_groups", + "dashboard", + "/api/v1/dashboard", + ): + mock_cache.delete.assert_any_call(prefix) + def test_create_channel_invalidates_channels(self, client_no_auth, api_db_session): mock_cache = self._install_mock_cache(client_no_auth) resp = client_no_auth.post( @@ -1517,6 +1565,7 @@ class TestMutationInvalidationIntegration: ) assert resp.status_code == 201 mock_cache.delete.assert_any_call("/api/v1/channels") + self._assert_channel_dependent_drops(mock_cache) def test_update_channel_invalidates_channels(self, client_no_auth, sample_channel): mock_cache = self._install_mock_cache(client_no_auth) @@ -1526,12 +1575,14 @@ class TestMutationInvalidationIntegration: ) assert resp.status_code == 200 mock_cache.delete.assert_any_call("/api/v1/channels") + self._assert_channel_dependent_drops(mock_cache) def test_delete_channel_invalidates_channels(self, client_no_auth, sample_channel): mock_cache = self._install_mock_cache(client_no_auth) resp = client_no_auth.delete(f"/api/v1/channels/{sample_channel.id}") assert resp.status_code == 204 mock_cache.delete.assert_any_call("/api/v1/channels") + self._assert_channel_dependent_drops(mock_cache) # --- Routes ----------------------------------------------------------- diff --git a/tests/test_collector/test_subscriber.py b/tests/test_collector/test_subscriber.py index b080a98..01ec733 100644 --- a/tests/test_collector/test_subscriber.py +++ b/tests/test_collector/test_subscriber.py @@ -1288,6 +1288,17 @@ class TestSubscriberDispatch: assert len(subscriber._webhook_queue) == 0 + def test_webhook_queue_is_bounded_and_drops_oldest(self, subscriber): + """A full queue drops the oldest events instead of growing forever.""" + subscriber._webhook_queue_max = 3 + for i in range(5): + subscriber._queue_webhook_event("test_event", {"seq": i}, "a" * 64) + + assert len(subscriber._webhook_queue) == 3 + # Oldest two events were dropped; the newest remain in order. + remaining = [payload["seq"] for _, payload, _ in subscriber._webhook_queue] + assert remaining == [2, 3, 4] + def test_start_with_mqtt_retry(self, mock_mqtt_client, db_manager): """MQTT connection is retried on failure.""" mock_mqtt_client.connect.side_effect = [ diff --git a/tests/test_collector/test_webhook.py b/tests/test_collector/test_webhook.py index 11a87c2..7fb18e0 100644 --- a/tests/test_collector/test_webhook.py +++ b/tests/test_collector/test_webhook.py @@ -318,6 +318,51 @@ class TestWebhookDispatcher: await dispatcher.stop() + @pytest.mark.asyncio + async def test_dispatch_4xx_not_retried(self, dispatcher): + """Permanent client errors must fail fast instead of retrying.""" + webhook = WebhookConfig( + url="https://example.com/webhook", + name="notfound-webhook", + max_retries=3, + retry_backoff=0.01, + ) + dispatcher.add_webhook(webhook) + await dispatcher.start() + + mock_response = AsyncMock() + mock_response.status_code = 404 + + with patch.object(dispatcher._client, "post", return_value=mock_response) as m: + result = await dispatcher.dispatch("event", {"data": "test"}) + assert result == {"notfound-webhook": False} + # Exactly one request despite max_retries=3. + assert m.call_count == 1 + + await dispatcher.stop() + + @pytest.mark.asyncio + async def test_dispatch_5xx_retried(self, dispatcher): + """Server errors are still retried.""" + webhook = WebhookConfig( + url="https://example.com/webhook", + name="server-error-webhook", + max_retries=2, + retry_backoff=0.01, + ) + dispatcher.add_webhook(webhook) + await dispatcher.start() + + mock_response = AsyncMock() + mock_response.status_code = 503 + + with patch.object(dispatcher._client, "post", return_value=mock_response) as m: + result = await dispatcher.dispatch("event", {"data": "test"}) + assert result == {"server-error-webhook": False} + assert m.call_count == 3 # initial + 2 retries + + await dispatcher.stop() + @pytest.mark.asyncio async def test_dispatch_timeout(self, dispatcher): """Test dispatch handles timeouts.""" diff --git a/tests/test_common/test_database.py b/tests/test_common/test_database.py index 05afee4..38b8ae2 100644 --- a/tests/test_common/test_database.py +++ b/tests/test_common/test_database.py @@ -1,7 +1,7 @@ """Tests for database engine configuration.""" from pathlib import Path -from unittest.mock import patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest from sqlalchemy import text @@ -149,3 +149,78 @@ class TestPostgresSessionTimezone: server_settings = kwargs["connect_args"]["server_settings"] assert server_settings["timezone"] == "UTC" assert server_settings["search_path"] == "meshcorehub" + + def test_async_engine_gets_pool_pre_ping_and_sizing(self) -> None: + """Async engine mirrors the sync engine's pool config.""" + manager = DatabaseManager.__new__(DatabaseManager) + manager.database_url = "postgresql://u:p@h/db" + manager._echo = False + manager._schema = None + manager._async_engine = None + manager._async_session_factory = None + + with patch("meshcore_hub.common.database.create_async_engine") as mock_async: + manager._ensure_async_engine() + _, kwargs = mock_async.call_args + assert kwargs["pool_pre_ping"] is True + assert kwargs["pool_size"] == 20 + assert kwargs["max_overflow"] == 30 + + def test_async_engine_skips_pool_args_for_memory_sqlite(self) -> None: + """In-memory SQLite uses a non-overflow pool; sizing args must be omitted.""" + manager = DatabaseManager.__new__(DatabaseManager) + manager.database_url = "sqlite+aiosqlite:///:memory:" + manager._echo = False + manager._schema = None + manager._async_engine = None + manager._async_session_factory = None + + with ( + patch("meshcore_hub.common.database.create_async_engine") as mock_async, + patch("meshcore_hub.common.database.event.listens_for"), + ): + manager._ensure_async_engine() + _, kwargs = mock_async.call_args + assert "pool_size" not in kwargs + assert "max_overflow" not in kwargs + + async def test_adispose_closes_async_engine(self) -> None: + """adispose awaits the async engine dispose and clears it.""" + manager = DatabaseManager.__new__(DatabaseManager) + manager.database_url = "sqlite+aiosqlite:///:memory:" + manager._echo = False + manager._schema = None + mock_async_engine = AsyncMock() + manager._async_engine = mock_async_engine + manager._async_session_factory = object() + manager.engine = MagicMock() + + await manager.adispose() + + mock_async_engine.dispose.assert_awaited_once() + manager.engine.dispose.assert_called_once() + assert manager._async_engine is None + assert manager._async_session_factory is None + + def test_dispose_sync_context_closes_async_engine_via_temp_loop(self) -> None: + """Outside a running loop, dispose() drains the async engine too.""" + manager = DatabaseManager.__new__(DatabaseManager) + manager.database_url = "sqlite+aiosqlite:///:memory:" + manager._echo = False + manager._schema = None + disposed = [] + + async def _fake_async_dispose() -> None: + disposed.append(True) + + mock_async_engine = MagicMock() + mock_async_engine.dispose = _fake_async_dispose + manager._async_engine = mock_async_engine + manager._async_session_factory = object() + manager.engine = MagicMock() + + manager.dispose() + + assert disposed == [True] + manager.engine.dispose.assert_called_once() + assert manager._async_engine is None diff --git a/tests/test_web/conftest.py b/tests/test_web/conftest.py index a43b3c7..688f1b8 100644 --- a/tests/test_web/conftest.py +++ b/tests/test_web/conftest.py @@ -247,7 +247,12 @@ class MockHttpClient: } def set_response( - self, method: str, path: str, status_code: int = 200, json_data: Any = None + self, + method: str, + path: str, + status_code: int = 200, + json_data: Any = None, + exc: Exception | None = None, ) -> None: """Set a custom response for a specific endpoint. @@ -256,11 +261,14 @@ class MockHttpClient: path: URL path status_code: Response status code json_data: JSON response body + exc: When set, the client raises this instead of responding + (simulates an unreachable backend) """ key = f"{method}:{path}" self._responses[key] = { "status_code": status_code, "json": json_data, + "exc": exc, } def set_paged_response(self, method: str, path: str, handler: Any) -> None: @@ -292,6 +300,8 @@ class MockHttpClient: return self._response_from_data( {"status_code": 404, "json": {"detail": "Not found"}} ) + if response_data.get("exc") is not None: + raise response_data["exc"] return self._response_from_data(response_data) async def request( diff --git a/tests/test_web/test_app.py b/tests/test_web/test_app.py index fdcaed0..95094fd 100644 --- a/tests/test_web/test_app.py +++ b/tests/test_web/test_app.py @@ -552,6 +552,25 @@ class TestSystemMaintenance: assert '"system_maintenance": true' in client.get("/").text + def test_maintenance_blocks_api_proxy( + self, mock_http_client: MockHttpClient + ) -> None: + """Maintenance mode must gate the API proxy, not just the SPA UI.""" + app = create_app( + api_url="http://localhost:8000", + api_key="test-api-key", + system_maintenance=True, + features=ALL_FEATURES_ENABLED, + ) + app.state.http_client = mock_http_client + client = TestClient(app, raise_server_exceptions=True) + + response = client.get("/api/v1/nodes") + assert response.status_code == 503 + assert response.json()["code"] == "MAINTENANCE" + # The proxy must not have forwarded anything to the backend. + assert mock_http_client.last_request_params is None + def test_maintenance_off_by_default(self, client: TestClient) -> None: """Without maintenance, nav links render normally (regression).""" html = client.get("/").text diff --git a/tests/test_web/test_health.py b/tests/test_web/test_health.py index bcd31f9..a6e9816 100644 --- a/tests/test_web/test_health.py +++ b/tests/test_web/test_health.py @@ -64,7 +64,7 @@ class TestHealthReadyEndpoint: client = TestClient(web_app, raise_server_exceptions=True) response = client.get("/health/ready") - assert response.status_code == 200 + assert response.status_code == 503 data = response.json() assert data["status"] == "not_ready" assert "status 500" in data["api"] @@ -81,7 +81,24 @@ class TestHealthReadyEndpoint: client = TestClient(web_app, raise_server_exceptions=True) response = client.get("/health/ready") - assert response.status_code == 200 + assert response.status_code == 503 data = response.json() assert data["status"] == "not_ready" assert "status 404" in data["api"] + + def test_health_ready_with_api_unreachable( + self, web_app: Any, mock_http_client: MockHttpClient + ) -> None: + """Test that health/ready returns 503 when the API call raises.""" + web_app.state.http_client = mock_http_client + mock_http_client.set_response( + "GET", "/health", exc=RuntimeError("connection refused") + ) + + client = TestClient(web_app, raise_server_exceptions=True) + response = client.get("/health/ready") + + assert response.status_code == 503 + data = response.json() + assert data["status"] == "not_ready" + assert data["api"] == "RuntimeError"