From 9c2f9fac46dd8b0a5e3729944455362c816021bc Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 7 Jun 2026 15:42:12 +0200 Subject: [PATCH] refactor(usage): address round-4 review on PR #871 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - hooks: document why user_id in metadata is safe — it stays tenant-local (the CP rollup aggregates GROUP BY (day, metric) into usage_daily, which has no metadata column, so it never reaches Stripe) and is retained to keep Deck #67's future per-user attribution derivable from the app DB. - migration: instantiate the SQLite-side column types (sa.Text() etc.) for visual parity with the instantiated Postgres types. - tests: assert the WARNING contract in the unserializable-metadata test too; add an autouse fixture that resets UsageEventStore._shared_instance so a stray shared() call can't leak across tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../20260607_1200_007_add_usage_events.py | 10 +++---- nextcloud_mcp_server/server/semantic.py | 6 ++++ nextcloud_mcp_server/vector/processor.py | 6 ++++ tests/unit/test_usage_store.py | 30 ++++++++++++++++--- 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py b/nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py index e2a41937..dc61317a 100644 --- a/nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py +++ b/nextcloud_mcp_server/alembic/versions/20260607_1200_007_add_usage_events.py @@ -44,7 +44,7 @@ def upgrade() -> None: # both backends (see usage/store.py). sa.Column( "event_id", - postgresql.UUID(as_uuid=False) if is_pg else sa.Text, + postgresql.UUID(as_uuid=False) if is_pg else sa.Text(), primary_key=True, ), # Operation completion time (UTC). Real TIMESTAMPTZ on Postgres so the @@ -52,18 +52,18 @@ def upgrade() -> None: # TIMESTAMP on SQLite (stored as ISO text, queryable in tests). sa.Column( "occurred_at", - postgresql.TIMESTAMP(timezone=True) if is_pg else sa.TIMESTAMP, + postgresql.TIMESTAMP(timezone=True) if is_pg else sa.TIMESTAMP(), nullable=False, ), # Catalog metric: 'embeddings_queries' or 'pages_chunks'. - sa.Column("metric", sa.Text, nullable=False), - sa.Column("value", sa.BigInteger, nullable=False), + sa.Column("metric", sa.Text(), nullable=False), + sa.Column("value", sa.BigInteger(), nullable=False), # Rawest unit per request (provider, model, tokens, doc_type, ...). # JSONB on Postgres so the CP can slice on dimensions later; TEXT # (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 4b3574f3..f8d36221 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -523,6 +523,12 @@ def configure_semantic_tools(mcp: FastMCP): # cost). Best-effort and gated on the flag so the off-path touches # no storage. nc_semantic_search_answer reuses this tool, so it # records here too — do not add a second hook there. + # + # Privacy note: user_id stays tenant-local. The CP rollup + # aggregates GROUP BY (day, metric) into usage_daily, which has no + # metadata column, so nothing here propagates to Stripe; the value + # is retained only so Deck #67's "per-user attribution derivable + # from app-DB metadata later" stays possible without a re-migration. if settings.usage_metering_enabled: try: store = await UsageEventStore.shared() diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 87e94a6d..b9bd2472 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -826,6 +826,12 @@ async def _index_document( # 'pages_chunks' event. Best-effort and gated on the flag so the # off-path (OSS default) touches no storage; placed after the # embedding succeeds so it can never affect the indexing path. + # + # Privacy note: user_id stays tenant-local — the CP rollup + # aggregates GROUP BY (day, metric) into usage_daily (no metadata + # column), so nothing here reaches Stripe; it is retained only to + # keep Deck #67's future per-user attribution derivable from the + # app DB without a re-migration. if settings.usage_metering_enabled: try: store = await UsageEventStore.shared() diff --git a/tests/unit/test_usage_store.py b/tests/unit/test_usage_store.py index e15222a8..475254c8 100644 --- a/tests/unit/test_usage_store.py +++ b/tests/unit/test_usage_store.py @@ -28,6 +28,19 @@ from nextcloud_mcp_server.usage.store import UsageEventStore pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +def _reset_shared_usage_store(): + """Keep the process-wide ``shared()`` cache from leaking across tests. + + These tests construct ``UsageEventStore(storage)`` directly, but a stray + ``shared()`` call (here or in a smoke test sharing the process) would + otherwise poison later tests with a stale storage handle. + """ + UsageEventStore._shared_instance = None + yield + UsageEventStore._shared_instance = None + + @pytest.fixture async def storage(storage_backend): """Initialized RefreshTokenStorage backed by SQLite or Postgres.""" @@ -231,7 +244,9 @@ async def test_best_effort_swallows_db_errors(storage, monkeypatch, caplog): ) -async def test_best_effort_swallows_unserializable_metadata(storage, monkeypatch): +async def test_best_effort_swallows_unserializable_metadata( + storage, monkeypatch, caplog +): """Non-serializable metadata is swallowed, not raised into the caller. json.dumps runs inside the best-effort try, so a metadata value the JSON @@ -245,9 +260,16 @@ async def test_best_effort_swallows_unserializable_metadata(storage, monkeypatch bad_metadata = {"obj": object()} # Must not raise. - await store.record_usage_event( - metric="pages_chunks", value=1, metadata=bad_metadata - ) + with caplog.at_level(logging.WARNING, logger="nextcloud_mcp_server.usage.store"): + await store.record_usage_event( + metric="pages_chunks", value=1, metadata=bad_metadata + ) # Nothing was written — the encode failed before the insert. assert await _count(storage) == 0 + # Silent data loss would be a footgun once metering is on: same WARNING + # contract as the DB-error path. + assert any( + r.levelno == logging.WARNING and "usage metering write dropped" in r.message + for r in caplog.records + )