refactor(usage): address round-3 review on PR #871
- 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
2bbf4ed967
commit
3a8ea893c6
+1
-1
@@ -17,7 +17,7 @@ fallbacks (``TEXT``/``TIMESTAMP``); the control plane never queries SQLite.
|
|||||||
|
|
||||||
Revision ID: 007
|
Revision ID: 007
|
||||||
Revises: 006
|
Revises: 006
|
||||||
Create Date: 2026-06-10 12:00:00.000000
|
Create Date: 2026-06-07 12:00:00.000000
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import sqlalchemy as sa
|
import sqlalchemy as sa
|
||||||
@@ -26,6 +26,8 @@ import uuid
|
|||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
|
import anyio
|
||||||
|
|
||||||
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage, get_shared_storage
|
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage, get_shared_storage
|
||||||
from nextcloud_mcp_server.config import get_settings
|
from nextcloud_mcp_server.config import get_settings
|
||||||
from nextcloud_mcp_server.observability.metrics import record_db_operation
|
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
|
# Process-wide cached instance returned by ``shared()`` so the hot search
|
||||||
# path doesn't allocate a fresh wrapper per metered query. The store is
|
# path doesn't allocate a fresh wrapper per metered query. The store is
|
||||||
# stateless beyond its storage handle, so one instance is reusable.
|
# 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_instance: "UsageEventStore | None" = None
|
||||||
|
_shared_lock: anyio.Lock = anyio.Lock()
|
||||||
|
|
||||||
def __init__(self, storage: RefreshTokenStorage) -> None:
|
def __init__(self, storage: RefreshTokenStorage) -> None:
|
||||||
self._storage = storage
|
self._storage = storage
|
||||||
@@ -67,10 +73,17 @@ class UsageEventStore:
|
|||||||
cached :class:`RefreshTokenStorage` (running ``initialize()`` / Alembic
|
cached :class:`RefreshTokenStorage` (running ``initialize()`` / Alembic
|
||||||
on first access, so ``usage_events`` exists), and the wrapper itself is
|
on first access, so ``usage_events`` exists), and the wrapper itself is
|
||||||
stateless, so reusing one instance avoids a per-call allocation on the
|
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:
|
async with cls._shared_lock:
|
||||||
cls._shared_instance = cls(await get_shared_storage())
|
if cls._shared_instance is None:
|
||||||
|
cls._shared_instance = cls(await get_shared_storage())
|
||||||
return cls._shared_instance
|
return cls._shared_instance
|
||||||
|
|
||||||
async def record_usage_event(
|
async def record_usage_event(
|
||||||
|
|||||||
Reference in New Issue
Block a user