From 3a8ea893c6f0425467a6d6c5f43ab0fbb31719b4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 7 Jun 2026 15:35:21 +0200 Subject: [PATCH] refactor(usage): address round-3 review on PR #871 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - store: guard UsageEventStore.shared() with a class-level anyio.Lock so two concurrent cold-start callers don't both build (and one silently overwrite) the cached instance — mirrors get_shared_storage(). Document that tests should construct the store directly to avoid singleton leak. - migration: rename 20260610 -> 20260607 and fix Create Date to today so `alembic history` isn't future-dated (revision id 007 / down_revision 006 unchanged; single head verified). Co-Authored-By: Claude Opus 4.8 (1M context) --- ... => 20260607_1200_007_add_usage_events.py} | 2 +- nextcloud_mcp_server/usage/store.py | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) rename nextcloud_mcp_server/alembic/versions/{20260610_1200_007_add_usage_events.py => 20260607_1200_007_add_usage_events.py} (98%) diff --git a/nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py b/nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py similarity index 98% rename from nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py rename to nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py index fcfefa45..e2a41937 100644 --- a/nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py +++ b/nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py @@ -17,7 +17,7 @@ fallbacks (``TEXT``/``TIMESTAMP``); the control plane never queries SQLite. Revision ID: 007 Revises: 006 -Create Date: 2026-06-10 12:00:00.000000 +Create Date: 2026-06-07 12:00:00.000000 """ import sqlalchemy as sa diff --git a/nextcloud_mcp_server/usage/store.py b/nextcloud_mcp_server/usage/store.py index b2298e78..092b0c40 100644 --- a/nextcloud_mcp_server/usage/store.py +++ b/nextcloud_mcp_server/usage/store.py @@ -26,6 +26,8 @@ import uuid from datetime import datetime, timezone from typing import Any +import anyio + from nextcloud_mcp_server.auth.storage import RefreshTokenStorage, get_shared_storage from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.observability.metrics import record_db_operation @@ -54,7 +56,11 @@ class UsageEventStore: # Process-wide cached instance returned by ``shared()`` so the hot search # path doesn't allocate a fresh wrapper per metered query. The store is # stateless beyond its storage handle, so one instance is reusable. + # ``anyio.Lock()`` doesn't bind to an event loop at construction, so a + # class-level instance is safe to define here (mirrors + # ``get_shared_storage``'s ``_shared_lock``). _shared_instance: "UsageEventStore | None" = None + _shared_lock: anyio.Lock = anyio.Lock() def __init__(self, storage: RefreshTokenStorage) -> None: self._storage = storage @@ -67,10 +73,17 @@ class UsageEventStore: cached :class:`RefreshTokenStorage` (running ``initialize()`` / Alembic on first access, so ``usage_events`` exists), and the wrapper itself is stateless, so reusing one instance avoids a per-call allocation on the - ``nc_semantic_search`` hot path. + ``nc_semantic_search`` hot path. The lock mirrors ``get_shared_storage`` + so two concurrent cold-start callers don't both build (and one silently + overwrite) the instance. + + Tests should construct ``UsageEventStore(storage)`` directly rather than + via ``shared()``: the cache is a process global with no teardown hook, + so a test that called ``shared()`` would leak its storage into the next. """ - if cls._shared_instance is None: - cls._shared_instance = cls(await get_shared_storage()) + async with cls._shared_lock: + if cls._shared_instance is None: + cls._shared_instance = cls(await get_shared_storage()) return cls._shared_instance async def record_usage_event(