From 702f66e6b168ef5e7fe1581effad3d29df4299d8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 7 Jun 2026 15:20:41 +0200 Subject: [PATCH] refactor(usage): address round-1 review on PR #871 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - store: add optional `enabled` param to record_usage_event so hot-path callers (nc_semantic_search) pass the already-resolved flag instead of forcing a second uncached Settings build (ADR-024); falls back to get_settings() when None so the store stays self-gating for standalone use. - hooks: thread enabled= through both call sites; bump the outer shared()/construction failure log from debug → warning so "metering enabled but no billing data" is visible at the default INFO level. - migration: instantiate postgresql.JSONB() to match the sibling TIMESTAMP(timezone=True) column. - tests: fix the misleading "asyncpg returns JSONB as a JSON string" comment; add occurred_at dialect round-trip test and an enabled-param short-circuit test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/scheduled_tasks.lock | 1 + .../20260610_1200_007_add_usage_events.py | 2 +- nextcloud_mcp_server/server/semantic.py | 9 ++- nextcloud_mcp_server/usage/store.py | 11 +++- nextcloud_mcp_server/vector/processor.py | 9 ++- tests/unit/test_usage_store.py | 57 ++++++++++++++++++- 6 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 .claude/scheduled_tasks.lock diff --git a/.claude/scheduled_tasks.lock b/.claude/scheduled_tasks.lock new file mode 100644 index 00000000..4e015a00 --- /dev/null +++ b/.claude/scheduled_tasks.lock @@ -0,0 +1 @@ +{"sessionId":"fdfa73f5-8734-459a-8f23-67e1eeaa804c","pid":13329,"procStart":"23713","acquiredAt":1780838157352} \ No newline at end of file diff --git a/nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py b/nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py index 53205772..17c38d40 100644 --- a/nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py +++ b/nextcloud_mcp_server/alembic/versions/20260610_1200_007_add_usage_events.py @@ -60,7 +60,7 @@ def upgrade() -> None: # (json.dumps) on SQLite. sa.Column( "metadata", - postgresql.JSONB if is_pg else sa.Text, + postgresql.JSONB() if is_pg else sa.Text, nullable=True, ), ) diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 5ce62d69..9640573d 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -534,9 +534,16 @@ def configure_semantic_tools(mcp: FastMCP): "fusion": fusion, "doc_types": doc_types, }, + # Pass the already-resolved flag so the store doesn't + # rebuild Settings on this hot query path (ADR-024). + enabled=settings.usage_metering_enabled, ) except Exception: - logger.debug( + # Reached only when shared()/store construction itself + # raises (record_usage_event swallows its own write + # failures). Metering is on, so warn — a silent DEBUG line + # would hide "operator enabled metering but gets no data". + logger.warning( "usage metering hook (embeddings_queries) skipped", exc_info=True, ) diff --git a/nextcloud_mcp_server/usage/store.py b/nextcloud_mcp_server/usage/store.py index 3f1b7298..c7551b61 100644 --- a/nextcloud_mcp_server/usage/store.py +++ b/nextcloud_mcp_server/usage/store.py @@ -72,6 +72,7 @@ class UsageEventStore: occurred_at: datetime | None = None, metadata: dict[str, Any] | None = None, event_id: str | None = None, + enabled: bool | None = None, ) -> None: """Record one billable usage event (best-effort, flag-gated). @@ -86,8 +87,16 @@ class UsageEventStore: metadata: Optional rawest-unit context (provider, model, tokens, doc_type, ...). Stored as JSONB (Postgres) / JSON text (SQLite). event_id: Optional idempotency key; defaults to a fresh UUID4. + enabled: The resolved ``USAGE_METERING_ENABLED`` value. ``None`` + (default) re-reads it via ``get_settings()`` so the store stays + self-gating for standalone/test use. Hot-path callers that + already hold the flag should pass it to avoid a second uncached + ``Settings`` build (``get_settings()`` is non-cached per + ADR-024 and ``nc_semantic_search`` is on the query path). """ - if not get_settings().usage_metering_enabled: + if enabled is None: + enabled = get_settings().usage_metering_enabled + if not enabled: return start = time.time() diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 863ae9e9..0565cd75 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -839,9 +839,16 @@ async def _index_document( "user_id": doc_task.user_id, "total_chars": total_chars, }, + # Pass the already-resolved flag so the store doesn't + # rebuild Settings here (ADR-024). + enabled=settings.usage_metering_enabled, ) except Exception: - logger.debug( + # Reached only when shared()/store construction itself + # raises (record_usage_event swallows its own write + # failures). Metering is on, so warn rather than hide the + # "enabled but no billing data" case in DEBUG logs. + logger.warning( "usage metering hook (pages_chunks) skipped", exc_info=True ) diff --git a/tests/unit/test_usage_store.py b/tests/unit/test_usage_store.py index f14aca6d..11ab5083 100644 --- a/tests/unit/test_usage_store.py +++ b/tests/unit/test_usage_store.py @@ -14,6 +14,7 @@ that a DB failure is swallowed instead of surfacing to the caller. import json import tempfile import uuid +from datetime import datetime, timezone from pathlib import Path import pytest @@ -84,6 +85,32 @@ async def test_flag_off_is_noop(storage, monkeypatch): assert await _count(storage) == 0 +async def test_enabled_param_short_circuits_without_reading_settings( + storage, monkeypatch +): + """An explicit ``enabled`` flag is honored without touching get_settings(). + + Hot-path callers pass the already-resolved flag; the store must not rebuild + Settings when given one. ``enabled=False`` is a no-op; ``enabled=True`` + writes even though the (boobytrapped) settings lookup would raise. + """ + + def _boom(): + raise AssertionError("get_settings() must not be called when enabled is passed") + + monkeypatch.setattr(store_module, "get_settings", _boom) + store = UsageEventStore(storage) + + await store.record_usage_event(metric="pages_chunks", value=1, enabled=False) + assert await _count(storage) == 0 + + eid = str(uuid.uuid4()) + await store.record_usage_event( + metric="pages_chunks", value=1, event_id=eid, enabled=True + ) + assert await _count(storage) == 1 + + async def test_insert_roundtrip(storage, monkeypatch): """A recorded event lands and reads back with the right fields.""" _set_metering(monkeypatch, True) @@ -127,11 +154,39 @@ async def test_metadata_json_roundtrip(storage, monkeypatch): ) row = await _fetch(storage, eid) raw = row[4] - # asyncpg returns JSONB as a JSON string (no codec); SQLite stores TEXT. + # Depending on the asyncpg/SQLAlchemy JSONB codec in play, Postgres may + # return JSONB as a Python dict or as a JSON str; SQLite stores TEXT. + # Handle both so the test is robust across driver/codec versions. loaded = raw if isinstance(raw, dict) else json.loads(raw) assert loaded == meta +async def test_occurred_at_roundtrip(storage, monkeypatch): + """occurred_at round-trips to the same instant on both backends. + + The store binds a datetime on Postgres and an ISO string on SQLite (the + only dialect-specific branch in the store); this pins that both read back + to the same instant regardless of the stored representation. + """ + _set_metering(monkeypatch, True) + store = UsageEventStore(storage) + eid = str(uuid.uuid4()) + when = datetime(2026, 6, 10, 12, 0, 0, tzinfo=timezone.utc) + await store.record_usage_event( + metric="pages_chunks", value=1, event_id=eid, occurred_at=when + ) + row = await _fetch(storage, eid) + stored = row[1] + # SQLite returns the ISO string we bound; Postgres returns a datetime. + parsed = stored if isinstance(stored, datetime) else datetime.fromisoformat(stored) + # Aware-datetime equality compares the instant, so a UTC value coming back + # in another session tz still matches; a naive value (none expected) is + # treated as UTC. + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + assert parsed == when + + async def test_metadata_none_is_null(storage, monkeypatch): """Omitting metadata stores SQL NULL, not the string 'null'.""" _set_metering(monkeypatch, True)