From fc0520461a92962c06b002abd721955b5b025c86 Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 18 Jul 2026 13:04:06 +0100 Subject: [PATCH 1/4] feat: invalidate read caches on user/admin mutations Mutation handlers (POST/PUT/DELETE) on channels, routes, user profiles, node tags, and adoptions now drop the corresponding Redis cache entries after commit so the UI reflects changes on the next page load instead of waiting for the 30s/300s TTL. Adds meshcore_hub.api.cache_invalidation with seven helpers that encapsulate the two cache-key formats (endpoint-name keys like 'nodes:' vs URL-path keys like '/api/v1/channels:') and swallow backend errors so a cache outage never breaks a successful write. The helper is a no-op when Redis is disabled. Cross-entity embeddings are covered: node-tag writes invalidate nodes, messages, advertisements, and dashboard; adoption writes invalidate nodes, profiles, advertisements, and dashboard. CLI/collector mutations remain TTL-bound (infrequent, operator-driven). --- AGENTS.md | 29 ++ src/meshcore_hub/api/cache_invalidation.py | 105 ++++++ src/meshcore_hub/api/routes/adoptions.py | 22 ++ src/meshcore_hub/api/routes/channels.py | 7 + src/meshcore_hub/api/routes/node_tags.py | 22 ++ src/meshcore_hub/api/routes/routes.py | 7 + src/meshcore_hub/api/routes/user_profiles.py | 6 + tests/test_api/test_cache.py | 317 +++++++++++++++++++ 8 files changed, 515 insertions(+) create mode 100644 src/meshcore_hub/api/cache_invalidation.py diff --git a/AGENTS.md b/AGENTS.md index d7f24a5..0b24b54 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -91,6 +91,35 @@ docker compose -f docker-compose.yml -f docker-compose.dev.yml --profile core ex ## Conventions +### Cache invalidation on writes + +Every mutation handler (POST/PUT/DELETE) on a user/admin-mutable entity MUST call the matching `invalidate_*` helper from `meshcore_hub.api.cache_invalidation` after `session.commit()` succeeds, so the UI reflects the change on the next page load instead of waiting for the Redis TTL. The helper is a no-op when Redis is disabled and swallows backend errors, so it's always safe to call. + +```python +from meshcore_hub.api.cache_invalidation import invalidate_channels + +@router.put("/{channel_id}") +def update_channel(__: RequireAdmin, session: DbSession, channel_id: str, + body: ChannelUpdate, request: Request) -> ChannelRead: + # ... mutate ... + session.commit() + session.refresh(channel) + invalidate_channels(request) # after commit, before return + return _channel_to_read(channel) +``` + +Mapping (see `api/cache_invalidation.py` for the canonical prefix knowledge): + +| Mutation | Helper(s) | +|---|---| +| `POST/PUT/DELETE /channels` | `invalidate_channels` | +| `POST/PUT/DELETE /routes` | `invalidate_routes` (covers list, detail, history) | +| `PUT /user/profile/{id}` | `invalidate_profiles` + `invalidate_dashboard` | +| `POST/PUT/DELETE /nodes/{pk}/tags` | `invalidate_nodes` + `invalidate_messages` + `invalidate_advertisements` + `invalidate_dashboard` (tags drive names/filters across these) | +| `POST/DELETE /adoptions` | `invalidate_nodes` + `invalidate_profiles` + `invalidate_advertisements` + `invalidate_dashboard` (`adopted_by` embedded across these) | + +When adding a new `@cached` read endpoint, decide whether its key namespace belongs in an existing invalidate helper, and add a test in `tests/test_api/test_cache.py::TestMutationInvalidationIntegration`. Cache keys split across two formats (endpoint-name keys like `nodes:` vs URL-path keys like `/api/v1/channels:`) — the helper module encapsulates that, don't hand-roll prefixes. + ```python # Imports: stdlib, third-party, local import os diff --git a/src/meshcore_hub/api/cache_invalidation.py b/src/meshcore_hub/api/cache_invalidation.py new file mode 100644 index 0000000..ac8e320 --- /dev/null +++ b/src/meshcore_hub/api/cache_invalidation.py @@ -0,0 +1,105 @@ +"""Cache invalidation helpers for entity mutations. + +After any successful write (POST/PUT/DELETE) on a user/admin-mutable entity, +the corresponding read caches must be dropped so the UI reflects the change on +the next page load instead of waiting for TTL expiry. + +Cache key layout +---------------- +There are two key formats in ``api/cache.py``, and this module must know about +both because they coexist: + +1. ``@cached("endpoint_name")`` with no ``key_builder`` stores keys as + ``{endpoint_name}:{sorted_query_string}`` — examples: ``nodes:``, + ``profiles:``, ``advertisements:``, ``dashboard/activity:``, + ``dashboard/packet-breakdown:``. +2. ``@cached(..., key_builder=fn)`` ignores ``endpoint_name`` and uses whatever + the builder returns. The shared builder pattern across role-aware endpoints + is ``{request.url.path}:role={role}:{sorted_query_string}``, so the actual + keys start with the literal URL path — examples: ``/api/v1/channels:``, + ``/api/v1/routes:``, ``/api/v1/routes/{id}:``, + ``/api/v1/routes/{id}/history:``, ``/api/v1/dashboard/stats:``. + +``CacheBackend.delete(prefix)`` SCANs by ``{prefix}*``, so a single call drops +every key under a namespace (including all role variants and all sub-paths). +Helpers here delete every namespace an entity touches — including cross-entity +embeddings (e.g. adoptions surface inside ``nodes``, ``profiles``, and +``advertisements`` listings). + +All helpers are no-ops when ``app.state.redis_cache`` is missing (cache +disabled) and swallow any backend error so a cache outage never breaks a +successful write — matching the resilience pattern in ``RedisCacheBackend``. +""" + +import logging +from typing import Optional + +from fastapi import Request + +from meshcore_hub.common.redis import CacheBackend + +logger = logging.getLogger(__name__) + + +def _cache(request: Request) -> Optional[CacheBackend]: + """Return the cache backend for this app, or None if caching is disabled.""" + return getattr(request.app.state, "redis_cache", None) + + +def _drop(request: Request, prefix: str) -> None: + """Best-effort ``delete(prefix)``; never raises.""" + cache = _cache(request) + if cache is None: + return + try: + cache.delete(prefix) + except Exception as e: + logger.warning("Cache invalidation error for prefix %s: %s", prefix, e) + + +def invalidate_channels(request: Request) -> None: + """Drop cached ``GET /channels`` responses (role-aware, URL-path keys).""" + _drop(request, "/api/v1/channels") + + +def invalidate_routes(request: Request) -> None: + """Drop cached ``GET /routes``, ``/routes/{id}`` and ``/routes/{id}/history``. + + All three endpoints share the ``/api/v1/routes`` URL-path prefix in their + cache keys (the ``{id}`` and ``{id}/history`` sub-paths glob-match the + same SCAN), so a single ``delete`` covers them. + """ + _drop(request, "/api/v1/routes") + + +def invalidate_nodes(request: Request) -> None: + """Drop cached ``GET /nodes`` responses (endpoint-name keys, no key_builder).""" + _drop(request, "nodes") + + +def invalidate_profiles(request: Request) -> None: + """Drop cached ``GET /user/profiles`` responses (endpoint-name keys).""" + _drop(request, "profiles") + + +def invalidate_messages(request: Request) -> None: + """Drop cached ``GET /messages`` responses (role-aware, URL-path keys).""" + _drop(request, "/api/v1/messages") + + +def invalidate_advertisements(request: Request) -> None: + """Drop cached ``GET /advertisements`` responses (endpoint-name keys).""" + _drop(request, "advertisements") + + +def invalidate_dashboard(request: Request) -> None: + """Drop every cached ``GET /dashboard/*`` response. + + Dashboard endpoints split across both key formats: ``stats`` and + ``message-activity`` use a ``key_builder`` (URL-path keys under + ``/api/v1/dashboard``) while ``activity``, ``packet-activity``, + ``packet-breakdown`` and ``node-count`` use endpoint-name keys under + ``dashboard``. Delete both prefixes to cover all of them. + """ + _drop(request, "dashboard") + _drop(request, "/api/v1/dashboard") diff --git a/src/meshcore_hub/api/routes/adoptions.py b/src/meshcore_hub/api/routes/adoptions.py index aa7393f..000f939 100644 --- a/src/meshcore_hub/api/routes/adoptions.py +++ b/src/meshcore_hub/api/routes/adoptions.py @@ -7,6 +7,12 @@ from sqlalchemy import select from sqlalchemy.orm import selectinload from meshcore_hub.api.auth import RequireOperatorOrAdmin +from meshcore_hub.api.cache_invalidation import ( + invalidate_advertisements, + invalidate_dashboard, + invalidate_nodes, + invalidate_profiles, +) from meshcore_hub.api.dependencies import DbSession from meshcore_hub.api.profile_utils import get_or_create_profile from meshcore_hub.common.models import Node, UserProfileNode @@ -17,6 +23,19 @@ logger = logging.getLogger(__name__) router = APIRouter() +def _invalidate_adoption_caches(request: Request) -> None: + """Drop caches that embed adoption info. + + ``adopted_by`` and the ``?adopted_by=`` filter surface inside nodes, + profiles, and advertisements listings; dashboard operator/node counts + also depend on adoptions. + """ + invalidate_nodes(request) + invalidate_profiles(request) + invalidate_advertisements(request) + invalidate_dashboard(request) + + @router.post("", response_model=AdoptedNodeRead, status_code=201) def adopt_node( adopt_request: NodeAdoptRequest, @@ -59,6 +78,8 @@ def adopt_node( session.commit() session.refresh(association) + _invalidate_adoption_caches(request) + logger.info( "User %s adopted node %s", caller_id, @@ -119,6 +140,7 @@ def release_node( session.delete(association) session.commit() + _invalidate_adoption_caches(request) logger.info( "User %s released node %s", diff --git a/src/meshcore_hub/api/routes/channels.py b/src/meshcore_hub/api/routes/channels.py index e10265d..a9d40d6 100644 --- a/src/meshcore_hub/api/routes/channels.py +++ b/src/meshcore_hub/api/routes/channels.py @@ -5,6 +5,7 @@ from sqlalchemy import select from meshcore_hub.api.auth import RequireAdmin, RequireRead from meshcore_hub.api.cache import cached, sorted_query_string +from meshcore_hub.api.cache_invalidation import invalidate_channels from meshcore_hub.api.channel_visibility import ( VISIBILITY_LEVELS, get_max_visibility_level, @@ -75,6 +76,7 @@ def create_channel( __: RequireAdmin, session: DbSession, body: ChannelCreate, + request: Request, ) -> ChannelRead: """Create a new channel (admin only).""" existing = session.execute( @@ -106,6 +108,7 @@ def create_channel( session.commit() session.refresh(channel) + invalidate_channels(request) return _channel_to_read(channel, include_key=True) @@ -115,6 +118,7 @@ def update_channel( session: DbSession, channel_id: str, body: ChannelUpdate, + request: Request, ) -> ChannelRead: """Update a channel (admin only, name is immutable).""" channel = session.execute( @@ -145,6 +149,7 @@ def update_channel( session.commit() session.refresh(channel) + invalidate_channels(request) return _channel_to_read(channel, include_key=True) @@ -153,6 +158,7 @@ def delete_channel( __: RequireAdmin, session: DbSession, channel_id: str, + request: Request, ) -> None: """Delete a channel (admin only).""" channel = session.execute( @@ -163,3 +169,4 @@ def delete_channel( session.delete(channel) session.commit() + invalidate_channels(request) diff --git a/src/meshcore_hub/api/routes/node_tags.py b/src/meshcore_hub/api/routes/node_tags.py index d80f406..f3dfcd1 100644 --- a/src/meshcore_hub/api/routes/node_tags.py +++ b/src/meshcore_hub/api/routes/node_tags.py @@ -4,6 +4,12 @@ from fastapi import APIRouter, HTTPException, Request, status from sqlalchemy import select from meshcore_hub.api.auth import RequireOperatorOrAdmin, RequireRead +from meshcore_hub.api.cache_invalidation import ( + invalidate_advertisements, + invalidate_dashboard, + invalidate_messages, + invalidate_nodes, +) from meshcore_hub.api.dependencies import DbSession from meshcore_hub.common.models import Node, NodeTag, UserProfile, UserProfileNode from meshcore_hub.common.schemas.nodes import ( @@ -16,6 +22,19 @@ from meshcore_hub.common.schemas.nodes import ( router = APIRouter() +def _invalidate_node_tag_caches(request: Request) -> None: + """Drop caches that embed or depend on node tags. + + Tag values drive node display names and search/sort, message sender names, + advertisement tag labels, and dashboard counts/friendly names — so a tag + write invalidates all of them. + """ + invalidate_nodes(request) + invalidate_messages(request) + invalidate_advertisements(request) + invalidate_dashboard(request) + + def _check_tag_access( session: DbSession, caller_info: tuple[str, list[str]], @@ -111,6 +130,7 @@ def create_node_tag( session.commit() session.refresh(node_tag) + _invalidate_node_tag_caches(request) return NodeTagRead.model_validate(node_tag) @@ -167,6 +187,7 @@ def update_node_tag( session.commit() session.refresh(node_tag) + _invalidate_node_tag_caches(request) return NodeTagRead.model_validate(node_tag) @@ -197,3 +218,4 @@ def delete_node_tag( session.delete(node_tag) session.commit() + _invalidate_node_tag_caches(request) diff --git a/src/meshcore_hub/api/routes/routes.py b/src/meshcore_hub/api/routes/routes.py index 923a4c4..d82aafe 100644 --- a/src/meshcore_hub/api/routes/routes.py +++ b/src/meshcore_hub/api/routes/routes.py @@ -7,6 +7,7 @@ from sqlalchemy import select from meshcore_hub.api.auth import RequireAdmin, RequireRead from meshcore_hub.api.cache import cached, sorted_query_string +from meshcore_hub.api.cache_invalidation import invalidate_routes from meshcore_hub.api.channel_visibility import ( VISIBILITY_LEVELS, get_max_visibility_level, @@ -167,6 +168,7 @@ def create_route( __: RequireAdmin, session: DbSession, body: RouteCreate, + request: Request, ) -> RouteRead: """Create a new route (admin only).""" existing = session.execute( @@ -210,6 +212,7 @@ def create_route( _sync_observers(session, route, observer_nodes) session.commit() session.refresh(route) + invalidate_routes(request) return _route_to_read(route) @@ -320,6 +323,7 @@ def update_route( session: DbSession, route_id: str, body: RouteUpdate, + request: Request, ) -> RouteRead: """Update a route (admin only).""" route = session.execute( @@ -379,6 +383,7 @@ def update_route( session.commit() session.refresh(route) + invalidate_routes(request) return _route_to_read(route) @@ -387,6 +392,7 @@ def delete_route( __: RequireAdmin, session: DbSession, route_id: str, + request: Request, ) -> None: """Delete a route (admin only).""" route = session.execute( @@ -396,6 +402,7 @@ def delete_route( raise HTTPException(status_code=404, detail="Route not found") session.delete(route) session.commit() + invalidate_routes(request) @router.post("/preview", response_model=RoutePreviewResponse) diff --git a/src/meshcore_hub/api/routes/user_profiles.py b/src/meshcore_hub/api/routes/user_profiles.py index 4464de4..7be0c31 100644 --- a/src/meshcore_hub/api/routes/user_profiles.py +++ b/src/meshcore_hub/api/routes/user_profiles.py @@ -9,6 +9,10 @@ from sqlalchemy.orm import selectinload from meshcore_hub.api.auth import RequireRead, RequireUserOwner, X_USER_ID_HEADER from meshcore_hub.api.cache import cached +from meshcore_hub.api.cache_invalidation import ( + invalidate_dashboard, + invalidate_profiles, +) from meshcore_hub.api.dependencies import DbSession from meshcore_hub.api.profile_utils import get_or_create_profile from meshcore_hub.common.config import get_web_settings @@ -236,4 +240,6 @@ def update_profile( session.commit() session.refresh(profile) + invalidate_profiles(request) + invalidate_dashboard(request) return UserProfileRead.model_validate(profile) diff --git a/tests/test_api/test_cache.py b/tests/test_api/test_cache.py index 80baaf1..7bc7efa 100644 --- a/tests/test_api/test_cache.py +++ b/tests/test_api/test_cache.py @@ -1330,3 +1330,320 @@ class TestKeyBuilders: key = _messages_key_builder(request) assert "role=admin" in key assert "limit=10" in key + + +def _make_request_with_cache(cache): + """Build a Request whose ``app.state.redis_cache`` is *cache* (or absent).""" + app = FastAPI() + if cache is not None: + app.state.redis_cache = cache + return Request( + scope={"type": "http", "query_string": b"", "headers": [], "app": app} + ) + + +class TestCacheInvalidationHelpers: + """Unit tests for ``meshcore_hub.api.cache_invalidation`` helpers.""" + + def test_invalidate_channels_drops_url_path_prefix(self): + from meshcore_hub.api.cache_invalidation import invalidate_channels + + cache = MagicMock() + invalidate_channels(_make_request_with_cache(cache)) + cache.delete.assert_called_once_with("/api/v1/channels") + + def test_invalidate_routes_drops_url_path_prefix(self): + from meshcore_hub.api.cache_invalidation import invalidate_routes + + cache = MagicMock() + invalidate_routes(_make_request_with_cache(cache)) + # Single prefix covers /routes, /routes/{id}, /routes/{id}/history + cache.delete.assert_called_once_with("/api/v1/routes") + + def test_invalidate_nodes_drops_endpoint_name_prefix(self): + from meshcore_hub.api.cache_invalidation import invalidate_nodes + + cache = MagicMock() + invalidate_nodes(_make_request_with_cache(cache)) + cache.delete.assert_called_once_with("nodes") + + def test_invalidate_profiles_drops_endpoint_name_prefix(self): + from meshcore_hub.api.cache_invalidation import invalidate_profiles + + cache = MagicMock() + invalidate_profiles(_make_request_with_cache(cache)) + cache.delete.assert_called_once_with("profiles") + + def test_invalidate_messages_drops_url_path_prefix(self): + from meshcore_hub.api.cache_invalidation import invalidate_messages + + cache = MagicMock() + invalidate_messages(_make_request_with_cache(cache)) + cache.delete.assert_called_once_with("/api/v1/messages") + + def test_invalidate_advertisements_drops_endpoint_name_prefix(self): + from meshcore_hub.api.cache_invalidation import invalidate_advertisements + + cache = MagicMock() + invalidate_advertisements(_make_request_with_cache(cache)) + cache.delete.assert_called_once_with("advertisements") + + def test_invalidate_dashboard_drops_both_prefix_formats(self): + from meshcore_hub.api.cache_invalidation import invalidate_dashboard + + cache = MagicMock() + invalidate_dashboard(_make_request_with_cache(cache)) + # Dashboard endpoints split between endpoint-name keys and URL-path keys + cache.delete.assert_any_call("dashboard") + cache.delete.assert_any_call("/api/v1/dashboard") + assert cache.delete.call_count == 2 + + def test_helpers_are_noop_when_cache_missing(self): + # No redis_cache attribute on state — must not raise. + from meshcore_hub.api import cache_invalidation as inv + + request = _make_request_with_cache(cache=None) + inv.invalidate_channels(request) + inv.invalidate_routes(request) + inv.invalidate_nodes(request) + inv.invalidate_profiles(request) + inv.invalidate_messages(request) + inv.invalidate_advertisements(request) + inv.invalidate_dashboard(request) + + def test_helpers_swallow_backend_errors(self): + from meshcore_hub.api import cache_invalidation as inv + + cache = MagicMock() + cache.delete.side_effect = Exception("redis down") + request = _make_request_with_cache(cache) + # Must not raise. + inv.invalidate_channels(request) + inv.invalidate_dashboard(request) + + +class TestMutationInvalidationIntegration: + """End-to-end: mutation handlers must drop the expected cache prefixes.""" + + def _install_mock_cache(self, client) -> MagicMock: + """Attach a mock cache that always misses; return it for assertions.""" + mock_cache = MagicMock() + mock_cache.get.return_value = None + client.app.state.redis_cache = mock_cache + client.app.state.redis_cache_ttl = 30 + client.app.state.redis_cache_ttl_dashboard = 300 + client.app.state.redis_cache_ttl_route_detail = 300 + return mock_cache + + # --- Channels --------------------------------------------------------- + + 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( + "/api/v1/channels", + json={ + "name": "InvalidationTest", + "key_hex": "AABBCCDDEEFF00112233445566778899", + "visibility": "community", + "enabled": True, + }, + ) + assert resp.status_code == 201 + mock_cache.delete.assert_any_call("/api/v1/channels") + + def test_update_channel_invalidates_channels(self, client_no_auth, sample_channel): + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.put( + f"/api/v1/channels/{sample_channel.id}", + json={"enabled": False}, + ) + assert resp.status_code == 200 + mock_cache.delete.assert_any_call("/api/v1/channels") + + 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") + + # --- Routes ----------------------------------------------------------- + + def test_create_route_invalidates_routes(self, client_no_auth, api_db_session): + from meshcore_hub.common.models import Node + + n1 = Node(public_key="aa" * 16, name="A") + n2 = Node(public_key="bb" * 16, name="B") + api_db_session.add_all([n1, n2]) + api_db_session.commit() + + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.post( + "/api/v1/routes", + json={ + "from_label": "A", + "to_label": "B", + "node_public_keys": [n1.public_key, n2.public_key], + "match_width": 2, + }, + headers={"X-User-Roles": "admin"}, + ) + assert resp.status_code == 201 + mock_cache.delete.assert_any_call("/api/v1/routes") + + def test_update_route_invalidates_routes(self, client_no_auth, api_db_session): + from meshcore_hub.common.models import Node, Route, RouteNode + + nodes = [Node(public_key=f"{c:02x}" * 16, name=str(c)) for c in (1, 2)] + api_db_session.add_all(nodes) + api_db_session.flush() + route = Route(from_label="X", to_label="Y") + api_db_session.add(route) + api_db_session.flush() + for pos, n in enumerate(nodes): + api_db_session.add( + RouteNode( + route_id=route.id, + node_id=n.id, + position=pos, + expected_hash=n.public_key[:2].upper(), + ) + ) + api_db_session.commit() + + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.put( + f"/api/v1/routes/{route.id}", + json={"from_label": "NewFrom", "to_label": "NewTo"}, + headers={"X-User-Roles": "admin"}, + ) + assert resp.status_code == 200 + mock_cache.delete.assert_any_call("/api/v1/routes") + + def test_delete_route_invalidates_routes(self, client_no_auth, api_db_session): + from meshcore_hub.common.models import Node, Route, RouteNode + + nodes = [Node(public_key=f"{c:02x}" * 16, name=str(c)) for c in (3, 4)] + api_db_session.add_all(nodes) + api_db_session.flush() + route = Route(from_label="P", to_label="Q") + api_db_session.add(route) + api_db_session.flush() + for pos, n in enumerate(nodes): + api_db_session.add( + RouteNode( + route_id=route.id, + node_id=n.id, + position=pos, + expected_hash=n.public_key[:2].upper(), + ) + ) + api_db_session.commit() + + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.delete( + f"/api/v1/routes/{route.id}", + headers={"X-User-Roles": "admin"}, + ) + assert resp.status_code == 204 + mock_cache.delete.assert_any_call("/api/v1/routes") + + # --- User profiles ---------------------------------------------------- + + def test_update_profile_invalidates_profiles_and_dashboard( + self, client_no_auth, sample_user_profile + ): + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.put( + f"/api/v1/user/profile/{sample_user_profile.id}", + json={"name": "Renamed"}, + headers={ + "X-User-Id": sample_user_profile.user_id, + "X-User-Roles": "operator", + }, + ) + assert resp.status_code == 200 + mock_cache.delete.assert_any_call("profiles") + mock_cache.delete.assert_any_call("dashboard") + mock_cache.delete.assert_any_call("/api/v1/dashboard") + + # --- Node tags -------------------------------------------------------- + + def test_create_node_tag_invalidates_cross_entity_caches( + self, client_no_auth, sample_node, sample_operator_adoption + ): + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.post( + f"/api/v1/nodes/{sample_node.public_key}/tags", + json={"key": "name", "value": "Friendly"}, + headers={ + "X-User-Id": "operator-123", + "X-User-Roles": "operator", + }, + ) + assert resp.status_code == 201 + for prefix in ("nodes", "/api/v1/messages", "advertisements"): + mock_cache.delete.assert_any_call(prefix) + # Dashboard covers both key formats + mock_cache.delete.assert_any_call("dashboard") + mock_cache.delete.assert_any_call("/api/v1/dashboard") + + def test_delete_node_tag_invalidates_cross_entity_caches( + self, client_no_auth, sample_node, sample_node_tag, sample_operator_adoption + ): + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.delete( + f"/api/v1/nodes/{sample_node.public_key}/tags/{sample_node_tag.key}", + headers={ + "X-User-Id": "operator-123", + "X-User-Roles": "operator", + }, + ) + assert resp.status_code == 204 + for prefix in ("nodes", "/api/v1/messages", "advertisements", "dashboard"): + mock_cache.delete.assert_any_call(prefix) + + # --- Adoptions -------------------------------------------------------- + + def test_adopt_node_invalidates_cross_entity_caches( + self, client_no_auth, sample_node + ): + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.post( + "/api/v1/adoptions", + json={"public_key": sample_node.public_key}, + headers={"X-User-Id": "adopter-1", "X-User-Roles": "operator"}, + ) + assert resp.status_code == 201 + for prefix in ("nodes", "profiles", "advertisements", "dashboard"): + mock_cache.delete.assert_any_call(prefix) + mock_cache.delete.assert_any_call("/api/v1/dashboard") + + def test_release_node_invalidates_cross_entity_caches( + self, client_no_auth, sample_node, sample_adopted_node + ): + mock_cache = self._install_mock_cache(client_no_auth) + resp = client_no_auth.delete( + f"/api/v1/adoptions/{sample_node.public_key}", + headers={"X-User-Id": "oidc-user-123", "X-User-Roles": "operator"}, + ) + assert resp.status_code == 204 + for prefix in ("nodes", "profiles", "advertisements", "dashboard"): + mock_cache.delete.assert_any_call(prefix) + + # --- Resilience ------------------------------------------------------- + + def test_cache_delete_error_does_not_break_mutation( + self, client_no_auth, sample_channel + ): + """If Redis is down, the mutation must still succeed.""" + mock_cache = MagicMock() + mock_cache.get.return_value = None + mock_cache.delete.side_effect = Exception("redis down") + client_no_auth.app.state.redis_cache = mock_cache + client_no_auth.app.state.redis_cache_ttl = 30 + + resp = client_no_auth.put( + f"/api/v1/channels/{sample_channel.id}", + json={"enabled": False}, + ) + assert resp.status_code == 200 From 71f65714ee5f24932989c31793dacb39da06649f Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 18 Jul 2026 13:17:04 +0100 Subject: [PATCH 2/4] fix: force browser revalidation so cache invalidation reaches the UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After PR #311 wired server-side invalidation, a user reported that editing a Route still showed old values on the routes list for ~30s. Root cause: the api_cache_middleware emitted 'Cache-Control: private, max-age=30' on @cached GETs, which let the browser serve its local HTTP cache copy without revalidating. The Redis invalidation fired correctly but never mattered — the browser never asked the server. Switch all /api/* GET responses to 'private, no-cache' (synonymous with 'max-age=0, must-revalidate'). The browser now sends If-None-Match on every navigation; the server answers 304 when Redis is warm and unchanged (cheap — no body) or 200 after an invalidation. The per-endpoint Redis TTL (30s default, 300s dashboard/route-detail) still bounds cache.set lifetime; only the HTTP-layer max-age disappears. Most navigations are still 304s, so the cost is one tiny round-trip per page load while guaranteeing freshness after any mutation. Adds an end-to-end regression test (test_routes_list_refresh_after_mutation) modelling the exact reported scenario: cache-fill, conditional 304, PUT mutation, conditional 200 with fresh body. --- .env.example | 5 +- AGENTS.md | 2 + docs/configuration.md | 6 +- src/meshcore_hub/api/app.py | 29 ++++--- tests/test_api/test_cache.py | 163 +++++++++++++++++++++++++++++++---- 5 files changed, 175 insertions(+), 30 deletions(-) diff --git a/.env.example b/.env.example index 4972949..0148eed 100644 --- a/.env.example +++ b/.env.example @@ -418,7 +418,10 @@ PROMETHEUS_PORT=9090 # REDIS_CACHE_TTL_ROUTE_DETAIL=300 # Emit HTTP Cache-Control on /api/v1/* responses + ETag/If-None-Match on -# cached endpoints. Disable to suppress all client-side caching directives. +# cached endpoints. The policy is `private, no-cache` on GETs (forces +# browser revalidation so server-side cache invalidation reaches the UI +# on the next page load) and `no-store` on mutations. Disable to suppress +# all client-side caching directives. # API_CACHE_CONTROL_ENABLED=true # External Alertmanager port (when using --profile metrics) diff --git a/AGENTS.md b/AGENTS.md index 0b24b54..acb911b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,6 +95,8 @@ docker compose -f docker-compose.yml -f docker-compose.dev.yml --profile core ex Every mutation handler (POST/PUT/DELETE) on a user/admin-mutable entity MUST call the matching `invalidate_*` helper from `meshcore_hub.api.cache_invalidation` after `session.commit()` succeeds, so the UI reflects the change on the next page load instead of waiting for the Redis TTL. The helper is a no-op when Redis is disabled and swallows backend errors, so it's always safe to call. +The HTTP-layer cache policy on `/api/v1/*` GETs is `private, no-cache` (i.e. must-revalidate) precisely so this works: the browser always sends `If-None-Match` on navigation, the server answers 304 when Redis is warm and unchanged (cheap — no body) or 200 after an invalidation. Do NOT change this back to `max-age>0` — server-side cache invalidation cannot reach the browser's HTTP cache, so any freshness window would let stale responses survive a mutation until expiry. + ```python from meshcore_hub.api.cache_invalidation import invalidate_channels diff --git a/docs/configuration.md b/docs/configuration.md index 57fb494..fec99c6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -77,13 +77,13 @@ Responses fall into three buckets: | Bucket | Endpoints | `Cache-Control` | `ETag` | | --- | --- | --- | --- | -| `@cached` GETs | All Redis-cached endpoints (`/nodes`, `/routes`, `/routes/{id}`, `/dashboard/*`, `/packets`, `/packet-groups`, `/messages`, `/advertisements`, `/channels`, `/user/profiles`) | `private, max-age=` | Strong SHA-256 hash of the body; `If-None-Match` returns `304 Not Modified` | -| Other GETs | Per-id detail endpoints (`/nodes/{key}`, `/packets/{id}`, `/messages/{id}`, `/user/profile/{id}`, `/trace-paths`, `/telemetry`, etc.) | `private, max-age=0, must-revalidate` | _(none)_ | +| `@cached` GETs | All Redis-cached endpoints (`/nodes`, `/routes`, `/routes/{id}`, `/dashboard/*`, `/packets`, `/packet-groups`, `/messages`, `/advertisements`, `/channels`, `/user/profiles`) | `private, no-cache` | Strong SHA-256 hash of the body; `If-None-Match` returns `304 Not Modified` | +| Other GETs | Per-id detail endpoints (`/nodes/{key}`, `/packets/{id}`, `/messages/{id}`, `/user/profile/{id}`, `/trace-paths`, `/telemetry`, etc.) | `private, no-cache` | _(none)_ | | Mutating + health | `POST`/`PUT`/`DELETE` + `/health*` | `no-store` | _(none)_ | All API responses use `private` because several `@cached` endpoints are role-aware — their response shape/redaction varies by trusted-proxy `X-User-Id` / `X-User-Roles` headers, so shared/CDN caches must never store them. Browser caches key by URL + request headers and so remain correct. -Client `max-age` matches the configured Redis TTL for the endpoint (e.g. 300 s on `/routes/{id}`, 30 s on `/dashboard/*`). The `X-Cache: HIT|MISS` observability header continues to be emitted regardless of this setting. +The `no-cache` policy (synonymous with `max-age=0, must-revalidate`) forces the browser to revalidate via `If-None-Match` on every navigation. This is required because the server-side cache invalidation fired by mutation handlers cannot reach the browser's HTTP cache — any `max-age>0` window would let stale responses survive a mutation until expiry. The Redis cache layer (TTL-bounded per endpoint — 30 s default, 300 s on `/routes/{id}` and `/dashboard/*`) still shields the database; only the browser's local reuse window goes away. Most revalidations answer `304 Not Modified` (no body), so the cost is one cheap round-trip per navigation. The `X-Cache: HIT|MISS` observability header continues to be emitted regardless of this setting. ## Collector diff --git a/src/meshcore_hub/api/app.py b/src/meshcore_hub/api/app.py index c4c2fba..4b358ff 100644 --- a/src/meshcore_hub/api/app.py +++ b/src/meshcore_hub/api/app.py @@ -182,9 +182,8 @@ def create_app( Buckets (only when ``app.state.api_cache_control_enabled`` is True): * ``@cached`` endpoints (``request.state.cache_control_ttl`` set by - the decorator): ``private, max-age=`` + ``ETag`` echoed back. - * Uncached GETs under ``/api/v1``: ``private, max-age=0, - must-revalidate`` so browsers revalidate but may store. + the decorator): ``private, no-cache`` + ``ETag`` echoed back. + * Other GETs under ``/api/v1``: ``private, no-cache`` as well. * Mutating methods (POST/PUT/DELETE/PATCH): ``no-store``. * ``/health*`` endpoints: ``no-store``. @@ -192,6 +191,18 @@ def create_app( role-aware — their response shape/redaction varies by trusted-proxy ``X-User-Id`` / ``X-User-Roles`` headers, so shared/CDN caches must never store them. + + ``no-cache`` (i.e. ``max-age=0, must-revalidate``) is used uniformly + for GETs under ``/api/`` rather than ``max-age=`` because the + server-side cache invalidation in ``api.cache_invalidation`` cannot + reach the browser's HTTP cache. Any ``max-age>0`` would let the + browser reuse a stale response after a mutation until the freshness + window expires. ``no-cache`` forces a conditional request + (``If-None-Match``) on every navigation, so the server can answer + 304 when nothing changed (cheap — no body) or 200 after a mutation. + The Redis cache layer (TTL-bounded by ``cache_control_ttl``) still + shields the database; only the browser's local reuse window goes + away. """ response = await call_next(request) @@ -224,13 +235,11 @@ def create_app( elif path.startswith("/health"): response.headers["Cache-Control"] = "no-store" elif path.startswith("/api/"): - ttl = getattr(request.state, "cache_control_ttl", 0) - if isinstance(ttl, int) and ttl > 0: - response.headers["Cache-Control"] = f"private, max-age={ttl}" - else: - response.headers["Cache-Control"] = ( - "private, max-age=0, must-revalidate" - ) + # Force revalidation: server-side cache invalidation can't reach + # the browser's HTTP cache, so any max-age>0 would serve stale + # data after a mutation. ETag/If-None-Match still gives us cheap + # 304s on the hot path; the Redis cache layer still protects DB. + response.headers["Cache-Control"] = "private, no-cache" return response diff --git a/tests/test_api/test_cache.py b/tests/test_api/test_cache.py index 7bc7efa..7253a23 100644 --- a/tests/test_api/test_cache.py +++ b/tests/test_api/test_cache.py @@ -878,24 +878,32 @@ class TestCacheControlMiddleware: client_no_auth.app.state.redis_cache_ttl = 30 response = client_no_auth.get("/api/v1/nodes") assert response.status_code == 200 - assert response.headers["cache-control"] == "private, max-age=30" + # no-cache (must-revalidate) — see api/app.py middleware docstring. + # The Redis TTL still flows to cache.set(...); only HTTP max-age is + # dropped so server-side invalidation can reach the browser. + assert response.headers["cache-control"] == "private, no-cache" assert "etag" in response.headers - def test_cached_get_cache_control_uses_overridden_ttl(self, client_no_auth): - """Route detail endpoint should use the route-detail TTL (300s).""" + def test_cached_get_ttl_flows_to_redis_not_http(self, client_no_auth): + """Route detail endpoint's TTL must drive cache.set, not Cache-Control. + + Regression: previously the per-endpoint TTL (e.g. 300s for + ``/routes/{id}``) was emitted as ``Cache-Control: max-age=300``, + which let the browser serve stale data for 5 min after a mutation. + The TTL now only bounds the Redis cache lifetime; HTTP-layer is + always ``private, no-cache`` so the browser revalidates and the + server-side invalidation wins. + """ mock_cache = MagicMock() mock_cache.get.return_value = None client_no_auth.app.state.redis_cache = mock_cache client_no_auth.app.state.redis_cache_ttl = 30 client_no_auth.app.state.redis_cache_ttl_route_detail = 300 - # Need a route to exist; this is a happy-path check of the header value - # so we stub the cache and just hit the endpoint with a fake id. The - # endpoint will return 404 but still flow through the @cached decorator - # and the middleware. + # Hit the route detail endpoint with a fake id. The endpoint returns + # 404 but still flows through the @cached decorator; we only care + # that the TTL is NOT surfaced as an HTTP header. response = client_no_auth.get("/api/v1/routes/nonexistent-id") - # 404 is fine; we only care about the header applied by the decorator - # via request.state.cache_control_ttl. - assert response.headers.get("cache-control") == "private, max-age=300" + assert response.headers.get("cache-control") == "private, no-cache" def test_cached_get_304_on_matching_if_none_match(self, client_no_auth): mock_cache = MagicMock() @@ -913,22 +921,20 @@ class TestCacheControlMiddleware: response = client_no_auth.get("/api/v1/nodes", headers={"If-None-Match": etag}) assert response.status_code == 304 assert response.headers["ETag"] == etag - assert response.headers["cache-control"] == "private, max-age=30" + assert response.headers["cache-control"] == "private, no-cache" assert response.headers["x-cache"] == "HIT" # 304 must not carry a body. assert response.content in (b"", b"null") - def test_uncached_get_emits_must_revalidate(self, client_no_auth, sample_node): - """Uncached GET detail endpoints get max-age=0, must-revalidate.""" + def test_uncached_get_emits_no_cache(self, client_no_auth, sample_node): + """Uncached GET detail endpoints get the same no-cache policy.""" # Force the @cached list endpoint to NOT be the target by hitting the # per-id endpoint, which is not cached. if hasattr(client_no_auth.app.state, "redis_cache"): del client_no_auth.app.state.redis_cache response = client_no_auth.get(f"/api/v1/nodes/{sample_node.public_key}") assert response.status_code == 200 - assert ( - response.headers["cache-control"] == "private, max-age=0, must-revalidate" - ) + assert response.headers["cache-control"] == "private, no-cache" def test_post_emits_no_store(self, client_no_auth, api_db_session): """POST endpoints always get Cache-Control: no-store. @@ -1647,3 +1653,128 @@ class TestMutationInvalidationIntegration: json={"enabled": False}, ) assert resp.status_code == 200 + + +class TestMutationVisibilityThroughHttpCache: + """Regression: stale browser HTTP cache after a mutation. + + Scenario reported in the wild: user edits a Route, the routes list page + keeps showing old values for ~30s. Root cause was the API emitting + ``Cache-Control: private, max-age=30`` on ``@cached`` GETs, which lets + the browser serve its local copy without revalidating — so the + server-side invalidation never had a chance to fire. + + The policy is now ``private, no-cache`` for all ``@cached`` GETs, which + forces the browser to send ``If-None-Match`` on every navigation. This + test models the full round trip: cache-fill, conditional 304, mutation + (invalidating Redis), then conditional 200 with the fresh body. + """ + + def test_routes_list_refresh_after_mutation(self, client_no_auth, api_db_session): + from meshcore_hub.common.models import Node, Route, RouteNode + + # Seed a route the user will later edit. + nodes = [Node(public_key=f"{c:02x}" * 16, name=str(c)) for c in (1, 2)] + api_db_session.add_all(nodes) + api_db_session.flush() + route = Route(from_label="Origin", to_label="Dest") + api_db_session.add(route) + api_db_session.flush() + for pos, n in enumerate(nodes): + api_db_session.add( + RouteNode( + route_id=route.id, + node_id=n.id, + position=pos, + expected_hash=n.public_key[:2].upper(), + ) + ) + api_db_session.commit() + + # Real in-memory cache so set/get/delete behave end-to-end. Keys + # store the envelope the @cached decorator writes. + store: dict[str, str] = {} + + class _FakeCache: + def get(self, key): + return store.get(key) + + def set(self, key, value, ttl): + store[key] = value + + def delete(self, prefix): + # SCAN-style prefix glob, matching RedisCacheBackend.delete. + for k in list(store.keys()): + if k.startswith(prefix): + del store[k] + + def ping(self): + return True + + client_no_auth.app.state.redis_cache = _FakeCache() + client_no_auth.app.state.redis_cache_ttl = 30 + + # 1) Initial GET — populates cache and returns an ETag. + first = client_no_auth.get("/api/v1/routes", headers={"X-User-Roles": "admin"}) + assert first.status_code == 200 + assert first.headers["x-cache"] == "MISS" + assert first.headers["cache-control"] == "private, no-cache" + first_etag = first.headers["etag"] + first_body = first.json() + assert first_body["items"][0]["from_label"] == "Origin" + + # 2) Immediate re-fetch with If-None-Match must 304 (cache HIT, + # ETag matches). This is the cheap fast path the policy + # preserves: browser revalidates, server answers 304, no body. + cond = client_no_auth.get( + "/api/v1/routes", + headers={"X-User-Roles": "admin", "If-None-Match": first_etag}, + ) + assert cond.status_code == 304 + assert cond.headers["x-cache"] == "HIT" + assert cond.headers["cache-control"] == "private, no-cache" + assert cond.content in (b"", b"null") + + # 3) Mutate the route. The handler calls invalidate_routes(request), + # which must drop the cached entry so the next GET is a MISS. + mut = client_no_auth.put( + f"/api/v1/routes/{route.id}", + json={"from_label": "NewOrigin", "to_label": "NewDest"}, + headers={"X-User-Roles": "admin"}, + ) + assert mut.status_code == 200 + # Mutations are always no-store. + assert mut.headers["cache-control"] == "no-store" + + # 4) Browser navigates again, sending the stale If-None-Match from + # step 1. The server MUST NOT 304 here: Redis was invalidated, + # so the handler re-runs, produces a new ETag, and returns 200 + # with the fresh body. This is exactly the bug the user hit — + # under the old max-age=30 policy the browser never sent this + # request at all. + after = client_no_auth.get( + "/api/v1/routes", + headers={"X-User-Roles": "admin", "If-None-Match": first_etag}, + ) + assert after.status_code == 200 + assert after.headers["x-cache"] == "MISS" + assert after.headers["etag"] != first_etag + assert after.json()["items"][0]["from_label"] == "NewOrigin" + + def test_cached_gets_always_emit_no_cache_regardless_of_ttl(self, client_no_auth): + """Even a 300s dashboard TTL must not surface as max-age=300. + + The dashboard endpoints have ``redis_cache_ttl_dashboard=300``, but + that bound applies only to the Redis cache. The HTTP policy is + always ``private, no-cache`` so server-side invalidation can reach + the browser after any mutation. + """ + mock_cache = MagicMock() + mock_cache.get.return_value = None + client_no_auth.app.state.redis_cache = mock_cache + client_no_auth.app.state.redis_cache_ttl = 30 + client_no_auth.app.state.redis_cache_ttl_dashboard = 300 + + resp = client_no_auth.get("/api/v1/dashboard/activity") + assert resp.status_code == 200 + assert resp.headers["cache-control"] == "private, no-cache" From b6740068d31cccb26df2ee37faec6f20a3a486ff Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 18 Jul 2026 13:41:52 +0100 Subject: [PATCH 3/4] log: cache invalidation observability for production diagnosis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After PR #311's invalidation wiring and HTTP policy fix, a user reported the routes list still showed stale data for ~30s after a PUT. The HAR they sent actually showed the fix working (x-cache: MISS after PUT, fresh data returned) but had been captured with DevTools' 'Disable cache' enabled — so it didn't represent normal browsing. We had four competing hypotheses and no way to pick between them. Add structured INFO logging at the two points that matter: * meshcore_hub.api.cache_invalidation._drop now emits 'Cache invalidate start: prefix=... backend=...' and 'Cache invalidate ok: prefix=...'. The backend= field distinguishes RedisCacheBackend from NullCache in one glance, catching 'REDIS_ENABLED is actually false' cases. * meshcore_hub.common.redis.RedisCacheBackend.delete now emits 'Redis cache delete: prefix=... full_prefix=... keys_deleted=N scan_iterations=N'. keys_deleted=0 after a mutation that should have invalidated entries is the smoking gun for a cache-key mismatch between the store path (key_builder) and the delete path (prefix glob). * NullCache.delete emits a DEBUG line for the same reason. The error paths are also enriched with full_prefix for greppability. No behavioral changes — invalidation still swallows errors so cache outages never break a write. After deploy, a single route-edit repro will produce log output whose shape (start/ok/warning/missing, keys_deleted count) unambiguously identifies which of the four hypotheses is correct, so we can write a targeted fix instead of guessing. --- src/meshcore_hub/api/cache_invalidation.py | 25 ++- src/meshcore_hub/common/redis.py | 29 +++- tests/test_api/test_cache.py | 174 +++++++++++++++++++++ 3 files changed, 223 insertions(+), 5 deletions(-) diff --git a/src/meshcore_hub/api/cache_invalidation.py b/src/meshcore_hub/api/cache_invalidation.py index ac8e320..d198587 100644 --- a/src/meshcore_hub/api/cache_invalidation.py +++ b/src/meshcore_hub/api/cache_invalidation.py @@ -47,14 +47,35 @@ def _cache(request: Request) -> Optional[CacheBackend]: def _drop(request: Request, prefix: str) -> None: - """Best-effort ``delete(prefix)``; never raises.""" + """Best-effort ``delete(prefix)``; never raises. + + Emits structured log lines so production traces can confirm a mutation + handler actually fired invalidation and see how many Redis keys were + deleted. The ``backend=`` field distinguishes ``RedisCacheBackend`` + (real Redis) from ``NullCache`` (Redis disabled) in one glance — useful + when ``REDIS_ENABLED`` is misconfigured. + """ cache = _cache(request) if cache is None: + logger.debug( + "Cache invalidate skipped (no backend on app.state): prefix=%s", + prefix, + ) return + logger.info( + "Cache invalidate start: prefix=%s backend=%s", + prefix, + type(cache).__name__, + ) try: cache.delete(prefix) + logger.info("Cache invalidate ok: prefix=%s", prefix) except Exception as e: - logger.warning("Cache invalidation error for prefix %s: %s", prefix, e) + logger.warning( + "Cache invalidate error: prefix=%s error=%s", + prefix, + e, + ) def invalidate_channels(request: Request) -> None: diff --git a/src/meshcore_hub/common/redis.py b/src/meshcore_hub/common/redis.py index 8f4dbc5..8024442 100644 --- a/src/meshcore_hub/common/redis.py +++ b/src/meshcore_hub/common/redis.py @@ -32,7 +32,9 @@ class NullCache(CacheBackend): pass def delete(self, prefix: str) -> None: - pass + # Logged so production traces can distinguish "Redis disabled" from + # "Redis enabled but matched no keys" when diagnosing invalidation. + logger.debug("NullCache delete: prefix=%s", prefix) def ping(self) -> bool: return False @@ -84,19 +86,40 @@ class RedisCacheBackend(CacheBackend): logger.warning("Redis SET error for %s: %s", key, e) def delete(self, prefix: str) -> None: + # Diagnostic logging: emit one INFO line per call with the prefix, + # full prefix (incl. key_prefix), total keys deleted, and SCAN + # iterations. ``keys_deleted=0`` after a mutation that should have + # invalidated entries is the smoking gun for a cache-key mismatch + # between the store path (key_builder) and the delete path (prefix). + full_prefix = self._full_key(prefix) + total_deleted = 0 + iterations = 0 try: - full_prefix = self._full_key(prefix) cursor = 0 while True: cursor, keys = self._client.scan( cursor, match=f"{full_prefix}*", count=100 ) + iterations += 1 if keys: self._client.delete(*keys) + total_deleted += len(keys) if cursor == 0: break + logger.info( + "Redis cache delete: prefix=%s full_prefix=%s keys_deleted=%d scan_iterations=%d", + prefix, + full_prefix, + total_deleted, + iterations, + ) except Exception as e: - logger.warning("Redis DELETE error for prefix %s: %s", prefix, e) + logger.warning( + "Redis DELETE error: prefix=%s full_prefix=%s error=%s", + prefix, + full_prefix, + e, + ) def ping(self) -> bool: try: diff --git a/tests/test_api/test_cache.py b/tests/test_api/test_cache.py index 7253a23..99a94d9 100644 --- a/tests/test_api/test_cache.py +++ b/tests/test_api/test_cache.py @@ -1778,3 +1778,177 @@ class TestMutationVisibilityThroughHttpCache: resp = client_no_auth.get("/api/v1/dashboard/activity") assert resp.status_code == 200 assert resp.headers["cache-control"] == "private, no-cache" + + +class TestInvalidationLogging: + """Diagnostic logging for cache invalidation. + + These tests pin down the log lines that operators grep for when + diagnosing whether mutation handlers actually fire invalidation and + whether Redis SCAN matches stored keys. The shape of the log output + is part of the contract — changing it breaks log dashboards and the + diagnostic runbook. + """ + + def test_drop_logs_start_and_ok_on_success(self, caplog): + from meshcore_hub.api.cache_invalidation import invalidate_routes + + cache = MagicMock() + cache.__class__.__name__ = "RedisCacheBackend" + request = _make_request_with_cache(cache) + + with caplog.at_level("INFO", logger="meshcore_hub.api.cache_invalidation"): + invalidate_routes(request) + + messages = [r.message for r in caplog.records] + assert any( + "Cache invalidate start" in m + and "prefix=/api/v1/routes" in m + and "backend=RedisCacheBackend" in m + for m in messages + ), f"start line missing or malformed: {messages}" + assert any( + "Cache invalidate ok" in m and "prefix=/api/v1/routes" in m + for m in messages + ), f"ok line missing or malformed: {messages}" + + def test_drop_logs_warning_on_backend_error(self, caplog): + from meshcore_hub.api.cache_invalidation import invalidate_channels + + cache = MagicMock() + cache.delete.side_effect = Exception("redis down") + request = _make_request_with_cache(cache) + + with caplog.at_level("WARNING", logger="meshcore_hub.api.cache_invalidation"): + invalidate_channels(request) + + # Must not raise; warning must carry prefix + error text. + assert any( + "Cache invalidate error" in r.message + and "prefix=/api/v1/channels" in r.message + and "redis down" in r.message + for r in caplog.records + ), [r.message for r in caplog.records] + + def test_drop_logs_skipped_when_no_backend(self, caplog): + from meshcore_hub.api.cache_invalidation import invalidate_nodes + + # No redis_cache attribute on app.state. + request = _make_request_with_cache(cache=None) + + with caplog.at_level("DEBUG", logger="meshcore_hub.api.cache_invalidation"): + invalidate_nodes(request) + + assert any( + "Cache invalidate skipped" in r.message and "prefix=nodes" in r.message + for r in caplog.records + ), [r.message for r in caplog.records] + + def test_drop_logs_backend_name_distinguishes_nullcache(self, caplog): + """If NullCache is wired in, the start log must say so. + + Catches the 'REDIS_ENABLED is actually false in production' case + in one log line. + """ + from meshcore_hub.api.cache_invalidation import invalidate_routes + + null_cache = NullCache() + request = _make_request_with_cache(null_cache) + + with caplog.at_level("INFO", logger="meshcore_hub.api.cache_invalidation"): + invalidate_routes(request) + + messages = [r.message for r in caplog.records] + assert any( + "backend=NullCache" in m and "prefix=/api/v1/routes" in m for m in messages + ), f"expected backend=NullCache in start log, got: {messages}" + + def test_redis_delete_logs_keys_deleted_count(self, caplog): + with patch("redis.Redis") as mock_redis_cls: + mock_client = MagicMock() + mock_redis_cls.return_value = mock_client + mock_client.scan.return_value = (0, [b"hub:nodes:1", b"hub:nodes:2"]) + + backend = RedisCacheBackend(key_prefix="hub") + with caplog.at_level("INFO", logger="meshcore_hub.common.redis"): + backend.delete("nodes") + + # One INFO line with prefix, full_prefix, and keys_deleted=2. + info_records = [r for r in caplog.records if r.levelname == "INFO"] + assert len(info_records) == 1, [r.message for r in caplog.records] + msg = info_records[0].message + assert "Redis cache delete" in msg + assert "prefix=nodes" in msg + assert "full_prefix=hub:nodes" in msg + assert "keys_deleted=2" in msg + assert "scan_iterations=1" in msg + + def test_redis_delete_logs_zero_keys_on_empty_scan(self, caplog): + """The smoking-gun signal for the production bug. + + If invalidation fires but SCAN matches nothing, ``keys_deleted=0`` + appears in the log. That points directly at a cache-key mismatch + between the store path (key_builder) and the delete path (prefix). + """ + with patch("redis.Redis") as mock_redis_cls: + mock_client = MagicMock() + mock_redis_cls.return_value = mock_client + mock_client.scan.return_value = (0, []) + + backend = RedisCacheBackend(key_prefix="hub") + with caplog.at_level("INFO", logger="meshcore_hub.common.redis"): + backend.delete("/api/v1/routes") + + info_records = [r for r in caplog.records if r.levelname == "INFO"] + assert len(info_records) == 1 + msg = info_records[0].message + assert "keys_deleted=0" in msg + assert "prefix=/api/v1/routes" in msg + assert "full_prefix=hub:/api/v1/routes" in msg + + def test_redis_delete_warning_includes_full_prefix(self, caplog): + with patch("redis.Redis") as mock_redis_cls: + mock_client = MagicMock() + mock_redis_cls.return_value = mock_client + mock_client.scan.side_effect = Exception("scan timeout") + + backend = RedisCacheBackend(key_prefix="hub") + with caplog.at_level("WARNING", logger="meshcore_hub.common.redis"): + backend.delete("nodes") + + warning_records = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warning_records) == 1 + msg = warning_records[0].message + assert "Redis DELETE error" in msg + assert "prefix=nodes" in msg + assert "full_prefix=hub:nodes" in msg + assert "scan timeout" in msg + + def test_redis_delete_multi_page_scan_logs_total_keys(self, caplog): + """Multi-page SCAN must accumulate keys_deleted across iterations.""" + with patch("redis.Redis") as mock_redis_cls: + mock_client = MagicMock() + mock_redis_cls.return_value = mock_client + mock_client.scan.side_effect = [ + (42, [b"hub:nodes:1", b"hub:nodes:2"]), + (0, [b"hub:nodes:3"]), + ] + + backend = RedisCacheBackend(key_prefix="hub") + with caplog.at_level("INFO", logger="meshcore_hub.common.redis"): + backend.delete("nodes") + + info_records = [r for r in caplog.records if r.levelname == "INFO"] + assert len(info_records) == 1 + msg = info_records[0].message + assert "keys_deleted=3" in msg + assert "scan_iterations=2" in msg + + def test_nullcache_delete_emits_debug_log(self, caplog): + cache = NullCache() + with caplog.at_level("DEBUG", logger="meshcore_hub.common.redis"): + cache.delete("nodes") + assert any( + "NullCache delete" in r.message and "prefix=nodes" in r.message + for r in caplog.records + ), [r.message for r in caplog.records] From e93c7fd9d605263170f92ab41708ca8f81687d0f Mon Sep 17 00:00:00 2001 From: Louis King Date: Sat, 18 Jul 2026 13:55:02 +0100 Subject: [PATCH 4/4] fix: re-evaluate route_result synchronously after route create/update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported symptom: user edits a Route (e.g. lowers packet_count_threshold from 6 to 3), and the routes list card still shows the old threshold for ~30s. The cache stack was exonerated — x-cache: MISS on the stale GET, response body had the new packet_count_threshold=3, but route_result. threshold remained at 6. Root cause was neither HTTP cache nor Redis cache. The list card's stats row (renderStatsRow in routes.js:82) displays route_result.threshold / effective_clear / matched_count, which are persisted by the background route_evaluator on a 30-60s schedule — separate DB row from the route's direct fields. The PUT handler updated the route row immediately but never triggered a re-evaluation, so route_result carried the stale snapshot from the prior evaluator cycle until the next sweep. Fix: add _reevaluate_route(session, route) helper that runs evaluate_route + upsert_route_result synchronously after the route commit. Called from create_route (initial evaluation) and update_route (refresh on every config change). Disabled routes short-circuit (no point evaluating a route that's turned off). One bounded DB scan per write — same cost as a single-route evaluator tick. After this fix, the PUT response itself carries a fresh route_result reflecting the new packet_count_threshold / clear_threshold, and the next GET /api/v1/routes returns it. The list card updates on the very next render cycle. Tests: - test_update_threshold_immediately_reflects_in_route_result: seeds a stale RouteResult with the OLD threshold, sends a PUT, asserts the response's route_result.threshold/effective_clear now match the new route config. - test_disabled_route_does_not_trigger_evaluation: monkeypatches evaluate_route to a spy, asserts it's never called for disabled routes. --- src/meshcore_hub/api/routes/routes.py | 26 ++++++ tests/test_api/test_routes.py | 118 ++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) diff --git a/src/meshcore_hub/api/routes/routes.py b/src/meshcore_hub/api/routes/routes.py index d82aafe..0df70c0 100644 --- a/src/meshcore_hub/api/routes/routes.py +++ b/src/meshcore_hub/api/routes/routes.py @@ -16,9 +16,11 @@ from meshcore_hub.api.channel_visibility import ( from meshcore_hub.api.dependencies import DbSession from meshcore_hub.collector.routes import ( derive_expected_hash, + evaluate_route, evaluate_route_history, preview_route, recent_matches, + upsert_route_result, ) from meshcore_hub.common.config import get_collector_settings from meshcore_hub.common.models.node import Node @@ -143,6 +145,28 @@ def _sync_observers( session.add(RouteObserver(route_id=route.id, node_id=node.id)) +def _reevaluate_route(session: DbSession, route: Route) -> None: + """Synchronously evaluate *route* and persist the fresh ``RouteResult``. + + The background evaluator (collector.route_evaluator) writes + ``RouteResult`` on a schedule (default 60s). Without this synchronous + re-eval, the route's ``packet_count_threshold`` / ``clear_threshold`` + changes take up to that interval to surface in the UI — the list card + displays ``route_result.threshold`` / ``effective_clear``, not the + route's just-updated direct fields, so it shows the stale snapshot + until the next evaluator cycle. Running the eval inline on every + create/update keeps the post-mutation GET consistent with the new + config at the cost of one bounded DB scan per write. + """ + if not route.enabled: + return + since = datetime.now(timezone.utc) - timedelta(hours=route.window_hours) + state, quality, matched_count = evaluate_route(session, route, since) + upsert_route_result(session, route, state, quality, matched_count) + session.commit() + session.refresh(route) + + @router.get("", response_model=RouteList) @cached("routes", key_builder=_routes_key_builder) def list_routes( @@ -212,6 +236,7 @@ def create_route( _sync_observers(session, route, observer_nodes) session.commit() session.refresh(route) + _reevaluate_route(session, route) invalidate_routes(request) return _route_to_read(route) @@ -383,6 +408,7 @@ def update_route( session.commit() session.refresh(route) + _reevaluate_route(session, route) invalidate_routes(request) return _route_to_read(route) diff --git a/tests/test_api/test_routes.py b/tests/test_api/test_routes.py index 9d191b4..209e4bf 100644 --- a/tests/test_api/test_routes.py +++ b/tests/test_api/test_routes.py @@ -384,6 +384,124 @@ class TestUpdateRoute: public_keys = [rn["public_key"] for rn in data["route_nodes"]] assert new_node.public_key in public_keys + def test_update_threshold_immediately_reflects_in_route_result( + self, client_no_auth, api_db_session + ): + """Regression: PUT-changed threshold must surface in route_result now. + + Before this fix, ``route_result`` (written by a background + evaluator on a 30-60s schedule) kept the OLD threshold until the + next evaluator cycle. The routes list card displays + ``route_result.threshold`` / ``effective_clear``, so the UI showed + stale values for ~30s after a PUT even though the server returned + ``x-cache: MISS`` with the route's direct fields updated. The + PUT handler now runs ``_reevaluate_route`` synchronously after + commit so the very next GET sees a fresh ``route_result``. + """ + from meshcore_hub.common.models.route_result import RouteResult + + nodes = _sample_nodes(api_db_session, 2) + route = Route( + from_label="Sync", + to_label="Eval", + packet_count_threshold=6, + clear_threshold=12, + enabled=True, + ) + api_db_session.add(route) + api_db_session.flush() + for pos, n in enumerate(nodes): + api_db_session.add( + RouteNode( + route_id=route.id, + node_id=n.id, + position=pos, + expected_hash=n.public_key[:2].upper(), + ) + ) + # Seed a stale RouteResult snapshot from a hypothetical prior + # evaluator run using the OLD config (threshold=6, clear=12). + # Without synchronous re-eval, this is what the PUT response + # would continue to return until the next background sweep. + api_db_session.add( + RouteResult( + route_id=route.id, + state="healthy", + quality="clear", + matched_count=24, + threshold=6, + effective_clear=12, + evaluated_at=datetime.now(timezone.utc), + ) + ) + api_db_session.commit() + + resp = client_no_auth.put( + f"/api/v1/routes/{route.id}", + json={"packet_count_threshold": 3, "clear_threshold": 6}, + headers={"X-User-Roles": "admin"}, + ) + assert resp.status_code == 200 + data = resp.json() + # The route's direct fields reflect the new config... + assert data["packet_count_threshold"] == 3 + assert data["clear_threshold"] == 6 + # ...AND route_result must reflect them too, not the stale + # snapshot from the seeded prior evaluation. + assert data["route_result"] is not None + assert ( + data["route_result"]["threshold"] == 3 + ), "route_result.threshold should reflect the new packet_count_threshold" + assert ( + data["route_result"]["effective_clear"] == 6 + ), "route_result.effective_clear should reflect the new clear_threshold" + + def test_disabled_route_does_not_trigger_evaluation( + self, client_no_auth, api_db_session, monkeypatch + ): + """Disabled routes short-circuit ``_reevaluate_route`` (no point + evaluating a route that won't be displayed as active). Guards + against unnecessary DB scans on bulk config changes.""" + from meshcore_hub.api.routes import routes as routes_module + + called = {"count": 0} + + def _spy_evaluate(*args, **kwargs): + called["count"] += 1 + return ("healthy", "clear", 0) + + monkeypatch.setattr(routes_module, "evaluate_route", _spy_evaluate) + + nodes = _sample_nodes(api_db_session, 2) + route = Route( + from_label="Off", + to_label="Line", + packet_count_threshold=3, + enabled=False, + ) + api_db_session.add(route) + api_db_session.flush() + for pos, n in enumerate(nodes): + api_db_session.add( + RouteNode( + route_id=route.id, + node_id=n.id, + position=pos, + expected_hash=n.public_key[:2].upper(), + ) + ) + api_db_session.commit() + + resp = client_no_auth.put( + f"/api/v1/routes/{route.id}", + json={"description": "still off"}, + headers={"X-User-Roles": "admin"}, + ) + assert resp.status_code == 200 + assert ( + called["count"] == 0 + ), "evaluate_route must not be called for disabled routes" + class TestDeleteRoute: def test_delete_success(self, client_no_auth, api_db_session):