refactor(usage): address round-4 review on PR #871

- 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) <noreply@anthropic.com>

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,
         ),
     )

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()

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()

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
+    )