From 9c89a58a07fa43fa72ed093b895235228e98e4ad Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 8 Jun 2026 16:58:31 +0200 Subject: [PATCH 1/3] feat(metering): pages_embedded = real parsed page count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pages_embedded` carried an interim chunk count (`len(chunk_texts)`, TODO #282). Reframe it as a charge for *parsing* (PDF page extraction / OCR) rather than a normalized content size: - Parsed files (PDFs) record `pages_embedded` = real `page_count` from the document processor metadata. - Text content (notes, deck cards, news items) is never parsed, carries no `page_count`, and records no `pages_embedded` row — only `tokens_embedded`. There is deliberately no chars/tokens-per-page constant; pages map 1:1 to parsed document pages. `record_indexing_usage` now takes `page_count` and records the two dimensions independently, gating `pages_embedded` on a truthy page count (not the doc_type) so a future non-PDF parsed type stays correct. Stays flag-gated + best-effort. Tests cover parsed-file, text-only, and zero-page cases. Deck #282 (board 8). Billing-model ADR corrected in astrolabe-cloud-website docs/control-plane/usage-metering.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/processor.py | 72 +++++++++++++++--------- tests/unit/test_processor_metering.py | 63 ++++++++++++++++++--- 2 files changed, 102 insertions(+), 33 deletions(-) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 5e92037c..8b7d6851 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -122,23 +122,30 @@ async def record_indexing_usage( chunk_count: int, token_count: int, total_chars: int, + page_count: int | None, ) -> None: - """Record the two billable usage events for one embedded document. + """Record the billable usage events for one embedded document. - ``pages_embedded`` is the buyer-facing "pages indexed" dimension; - ``tokens_embedded`` is the embedding request's token count — the same metric - search records, so the meter bills embedding tokens whether they were - incurred indexing a document or embedding a query (Deck #67). + Two metered dimensions (Deck #67), recorded independently: - TODO(#282): ``pages_embedded`` currently carries the raw chunk count - (``len(chunk_texts)``) as an interim value. The real normalized "pages - indexed" count — real pages for paginated types (PDF/DOCX/PPT), a fixed - chars/tokens-per-page constant otherwise — is deferred to instrumentation - card #282; this code (card #284) only lands the metric name/contract. + - ``tokens_embedded`` — the embedding request's token count, recorded for + *every* embedded document. The same metric search records, so the meter + bills embedding tokens whether they were incurred indexing a document or + embedding a query. + - ``pages_embedded`` — a charge for **parsing** (PDF page extraction / OCR), + not a normalized content size. ``page_count`` is the real number of pages + the document processor parsed. Text content (notes, deck cards, news + items) is never parsed, carries no ``page_count``, and accrues **no** + ``pages_embedded`` row — only ``tokens_embedded``. There is deliberately + no chars/tokens-per-page constant: pages map 1:1 to parsed document pages + (card #282). Best-effort and flag-gated: a metering failure is logged and never breaks indexing. No-op when metering is disabled or the document produced no chunks (an empty batch embeds nothing and would only write zero-value rows). + ``pages_embedded`` is additionally skipped when ``page_count`` is absent or + zero — gating on the page count itself (not the ``doc_type``) keeps this + correct if a future non-PDF parsed type starts reporting pages. Privacy note: ``user_id`` stays tenant-local — the CP rollup aggregates GROUP BY (day, metric) into ``usage_daily`` (no metadata column), so nothing @@ -159,24 +166,27 @@ async def record_indexing_usage( store = await UsageEventStore.shared() # enabled=True: the guard above already confirmed the flag, so the store # skips a second uncached Settings build per record (ADR-024). - # record_usage_event swallows its own write failures, so the two records - # are independent; if pages_embedded somehow raised mid-way, - # tokens_embedded would be skipped, leaving an unmatched pages_embedded - # row — acceptable under the (day, metric) SUM-aggregation billing model. - await store.record_usage_event( - # TODO(#282): value is the interim chunk count; switch to normalized - # real-page count when the per-page constant lands. - metric="pages_embedded", - value=chunk_count, - metadata=metadata, - enabled=True, - ) + # record_usage_event swallows its own write failures, so the records are + # independent; one raising never blocks the other — acceptable under the + # (day, metric) SUM-aggregation billing model. + + # tokens_embedded: recorded for every embedded document. await store.record_usage_event( metric="tokens_embedded", value=token_count, metadata=metadata, enabled=True, ) + # pages_embedded: parsed pages only. Text content has no page_count, so + # skip it rather than write a zero-value row that would misrepresent a + # no-parse document as billable parsing work. + if page_count: + await store.record_usage_event( + metric="pages_embedded", + value=page_count, + metadata=metadata, + enabled=True, + ) except Exception: # Reached only when shared()/store construction itself raises # (record_usage_event swallows its own write failures). Metering is on, @@ -912,11 +922,18 @@ async def _index_document( # Export token consumption to Prometheus (always-on, independent of # the billing flag) so Grafana sees indexing token cost. record_embedding_tokens(provider, "index", embed_tokens) - # Usage metering (Deck #67): record the chunk volume + - # embedding-token count for this document. Best-effort and - # flag-gated; placed after the embedding succeeds so it can never - # affect the indexing path. See record_indexing_usage for the + # Usage metering (Deck #67): record the embedding-token count (all + # docs) and, for parsed files, the real parsed-page count. Best- + # effort and flag-gated; placed after the embedding succeeds so it + # can never affect the indexing path. ``page_count`` is set by the + # document processors for PDFs and absent for text types, so text + # content meters tokens only. See record_indexing_usage for the # metric/privacy details. + # + # Narrow defensively: file_metadata values are loosely typed, so a + # malformed page_count meters as "no pages" rather than erroring on + # the indexing path. + raw_page_count = file_metadata.get("page_count") await record_indexing_usage( enabled=settings.usage_metering_enabled, provider=provider, @@ -926,6 +943,9 @@ async def _index_document( chunk_count=len(chunk_texts), token_count=embed_tokens, total_chars=total_chars, + page_count=( + raw_page_count if isinstance(raw_page_count, int) else None + ), ) async def generate_sparse_embeddings(): diff --git a/tests/unit/test_processor_metering.py b/tests/unit/test_processor_metering.py index 26838c29..a5449e4b 100644 --- a/tests/unit/test_processor_metering.py +++ b/tests/unit/test_processor_metering.py @@ -1,9 +1,12 @@ """Unit tests for the indexing-path usage-metering helper (Deck #67). -``record_indexing_usage`` records the two billable events (``pages_embedded`` + -``tokens_embedded``) after a document's chunks are embedded. These cover the -value mapping, the flag/zero-chunk no-ops, and the best-effort failure path -without standing up the full document pipeline. +``record_indexing_usage`` records the billable events after a document's chunks +are embedded: ``tokens_embedded`` for every document, and ``pages_embedded`` +only for parsed files (real ``page_count``). Text content (no ``page_count``) +meters tokens only — ``pages_embedded`` is a charge for parsing, not content +size (card #282). These cover the value mapping, the flag/zero-chunk no-ops, the +text-only path, and the best-effort failure path without standing up the full +document pipeline. """ from unittest.mock import AsyncMock, MagicMock @@ -25,8 +28,8 @@ def store_spy(monkeypatch): @pytest.mark.unit -async def test_records_pages_embedded_and_token_count(store_spy): - """Both events fire: pages_embedded = chunk count, tokens_embedded = tokens.""" +async def test_parsed_file_records_pages_and_tokens(store_spy): + """A parsed PDF fires both events: pages_embedded = real page count.""" await processor.record_indexing_usage( enabled=True, provider="mistral", @@ -36,11 +39,13 @@ async def test_records_pages_embedded_and_token_count(store_spy): chunk_count=110, token_count=4242, total_chars=170826, + page_count=12, ) calls = store_spy.record_usage_event.await_args_list by_metric = {c.kwargs["metric"]: c.kwargs["value"] for c in calls} - assert by_metric == {"pages_embedded": 110, "tokens_embedded": 4242} + # pages_embedded is the real parsed-page count, NOT the chunk count. + assert by_metric == {"pages_embedded": 12, "tokens_embedded": 4242} for c in calls: # Hot-path fast-gate + tenant-local attribution metadata. assert c.kwargs["enabled"] is True @@ -50,6 +55,47 @@ async def test_records_pages_embedded_and_token_count(store_spy): assert c.kwargs["metadata"]["doc_type"] == "file" +@pytest.mark.unit +async def test_text_doc_records_tokens_only(store_spy): + """Unparsed text content (no page_count) meters tokens, never pages.""" + await processor.record_indexing_usage( + enabled=True, + provider="mistral", + model="mistral-embed", + doc_type="note", + user_id="alice", + chunk_count=4, + token_count=512, + total_chars=7000, + page_count=None, + ) + + calls = store_spy.record_usage_event.await_args_list + by_metric = {c.kwargs["metric"]: c.kwargs["value"] for c in calls} + assert by_metric == {"tokens_embedded": 512} + assert "pages_embedded" not in by_metric + + +@pytest.mark.unit +async def test_zero_pages_skips_pages(store_spy): + """page_count=0 (e.g. an empty/corrupt PDF) records tokens but no pages.""" + await processor.record_indexing_usage( + enabled=True, + provider="mistral", + model="mistral-embed", + doc_type="file", + user_id="alice", + chunk_count=4, + token_count=99, + total_chars=1000, + page_count=0, + ) + + calls = store_spy.record_usage_event.await_args_list + by_metric = {c.kwargs["metric"]: c.kwargs["value"] for c in calls} + assert by_metric == {"tokens_embedded": 99} + + @pytest.mark.unit async def test_disabled_is_noop(store_spy): """Flag off → no store access, no events.""" @@ -62,6 +108,7 @@ async def test_disabled_is_noop(store_spy): chunk_count=10, token_count=20, total_chars=5, + page_count=3, ) store_spy.record_usage_event.assert_not_awaited() @@ -78,6 +125,7 @@ async def test_zero_chunks_is_noop(store_spy): chunk_count=0, token_count=0, total_chars=0, + page_count=3, ) store_spy.record_usage_event.assert_not_awaited() @@ -101,4 +149,5 @@ async def test_store_failure_is_swallowed(monkeypatch): chunk_count=3, token_count=7, total_chars=9, + page_count=2, ) From a3178cf1fa3b18782e369aa8dbe1d8d5d673d321 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 8 Jun 2026 17:03:46 +0200 Subject: [PATCH 2/3] refactor(metering): harden page_count guard per review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review follow-ups (PR #879): - Gate pages_embedded on `page_count and page_count > 0` so a malformed negative count meters as "no pages" rather than emitting a negative billing row (matches the documented call-site intent). - Exclude bool at the call-site narrowing (`isinstance(int) and not isinstance(bool)`) — bool is an int subclass, so a stray page_count=True would otherwise record pages=1. - Document chunk_count's role (empty-batch no-op guard) and the intentional tokens-before-pages ordering in the docstring/comments. - Add test_negative_pages_skips_pages. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/processor.py | 32 ++++++++++++++++-------- tests/unit/test_processor_metering.py | 20 +++++++++++++++ 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 8b7d6851..38c04b33 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -141,11 +141,13 @@ async def record_indexing_usage( (card #282). Best-effort and flag-gated: a metering failure is logged and never breaks - indexing. No-op when metering is disabled or the document produced no chunks - (an empty batch embeds nothing and would only write zero-value rows). - ``pages_embedded`` is additionally skipped when ``page_count`` is absent or - zero — gating on the page count itself (not the ``doc_type``) keeps this - correct if a future non-PDF parsed type starts reporting pages. + indexing. ``chunk_count`` is the empty-batch no-op guard — a document that + produced no chunks embedded nothing, so both events are skipped rather than + writing zero-value rows. ``pages_embedded`` is additionally skipped when + ``page_count`` is absent or not strictly positive — gating on the page count + itself (not the ``doc_type``) keeps this correct if a future non-PDF parsed + type starts reporting pages, and a malformed non-positive count meters as + "no pages" rather than emitting a zero/negative billing row. Privacy note: ``user_id`` stays tenant-local — the CP rollup aggregates GROUP BY (day, metric) into ``usage_daily`` (no metadata column), so nothing @@ -170,17 +172,20 @@ async def record_indexing_usage( # independent; one raising never blocks the other — acceptable under the # (day, metric) SUM-aggregation billing model. - # tokens_embedded: recorded for every embedded document. + # tokens_embedded first (intentional ordering): it is recorded for every + # embedded document, so the embedding cost is always captured before the + # conditional parsing cost — don't reverse this in a refactor. await store.record_usage_event( metric="tokens_embedded", value=token_count, metadata=metadata, enabled=True, ) - # pages_embedded: parsed pages only. Text content has no page_count, so - # skip it rather than write a zero-value row that would misrepresent a - # no-parse document as billable parsing work. - if page_count: + # pages_embedded: parsed pages only, and only a strictly positive count. + # Text content has no page_count; a zero/negative count is skipped rather + # than writing a row that would misrepresent a no-parse document as + # billable parsing work. + if page_count and page_count > 0: await store.record_usage_event( metric="pages_embedded", value=page_count, @@ -933,6 +938,8 @@ async def _index_document( # Narrow defensively: file_metadata values are loosely typed, so a # malformed page_count meters as "no pages" rather than erroring on # the indexing path. + # bool is an int subclass, so exclude it explicitly — a stray + # page_count=True in metadata must not slip through as pages=1. raw_page_count = file_metadata.get("page_count") await record_indexing_usage( enabled=settings.usage_metering_enabled, @@ -944,7 +951,10 @@ async def _index_document( token_count=embed_tokens, total_chars=total_chars, page_count=( - raw_page_count if isinstance(raw_page_count, int) else None + raw_page_count + if isinstance(raw_page_count, int) + and not isinstance(raw_page_count, bool) + else None ), ) diff --git a/tests/unit/test_processor_metering.py b/tests/unit/test_processor_metering.py index a5449e4b..7a46f7f5 100644 --- a/tests/unit/test_processor_metering.py +++ b/tests/unit/test_processor_metering.py @@ -96,6 +96,26 @@ async def test_zero_pages_skips_pages(store_spy): assert by_metric == {"tokens_embedded": 99} +@pytest.mark.unit +async def test_negative_pages_skips_pages(store_spy): + """A malformed negative page_count meters as 'no pages' (tokens only).""" + await processor.record_indexing_usage( + enabled=True, + provider="mistral", + model="mistral-embed", + doc_type="file", + user_id="alice", + chunk_count=4, + token_count=99, + total_chars=1000, + page_count=-1, + ) + + calls = store_spy.record_usage_event.await_args_list + by_metric = {c.kwargs["metric"]: c.kwargs["value"] for c in calls} + assert by_metric == {"tokens_embedded": 99} + + @pytest.mark.unit async def test_disabled_is_noop(store_spy): """Flag off → no store access, no events.""" From 9d6592860b2af5aab7f94098b463dde07516ca38 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 8 Jun 2026 17:08:29 +0200 Subject: [PATCH 3/3] test(metering): assert tokens-before-pages ordering invariant Round-2 review nit (PR #879): lock the intentional record ordering (tokens_embedded before the conditional pages_embedded) with an assertion in test_parsed_file_records_pages_and_tokens, so a refactor that reverses it fails a test rather than only contradicting a comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_processor_metering.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/unit/test_processor_metering.py b/tests/unit/test_processor_metering.py index 7a46f7f5..1b9dedc7 100644 --- a/tests/unit/test_processor_metering.py +++ b/tests/unit/test_processor_metering.py @@ -46,6 +46,11 @@ async def test_parsed_file_records_pages_and_tokens(store_spy): by_metric = {c.kwargs["metric"]: c.kwargs["value"] for c in calls} # pages_embedded is the real parsed-page count, NOT the chunk count. assert by_metric == {"pages_embedded": 12, "tokens_embedded": 4242} + # Intentional ordering: tokens (recorded for every doc) before pages (the + # conditional parsing cost). Asserted so a refactor can't silently reverse + # it — a comment alone is easier to delete than a failing test. + assert calls[0].kwargs["metric"] == "tokens_embedded" + assert calls[1].kwargs["metric"] == "pages_embedded" for c in calls: # Hot-path fast-gate + tenant-local attribution metadata. assert c.kwargs["enabled"] is True