Quality-gate fixes (new-code conditions on PR #902):
- new_security_hotspots_reviewed: drop the fake "http://nextcloud" host in the
manager tests to https:// (python:S5332 ×2).
- new_security_rating: generate the integration test's fake app password with
secrets.token_urlsafe instead of a hardcoded literal (python:S2068).
- new_reliability_rating: restructure the user_manager sleep so an explicit
await checkpoint lives inside the cancellation scope — await one waiter
directly while watching shutdown via start_soon (python:S7490). Behaviour is
unchanged: timeout, shutdown, or a provisioning ring all end the sleep.
Review nits:
- Move the shutdown test's fail_after(2) to wrap the whole task group so it
actually bounds the task-group exit (was guarding a no-op sleep); drop the
sleep(0) stub (python:S7491).
- Type _wake_on's wait_fn as Callable[[], Awaitable[object]].
- Note in _wire_vector_sync_state why provision_signal is set on the singleton
only, not fanned out to app.state.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ProvisionSignal.wait() re-arms in a finally so a cancelled wait (shutdown
racing the doorbell) leaves a fresh unset event, not a stale set-but-consumed
one; preserves the no-await-before-swap lost-wakeup guarantee.
- Hoist user_manager_task's _wake_on helper out of the while loop (one object,
not one per iteration).
- Test: assert ProvisionSignal via its public wait() contract instead of the
private _event attribute.
- Add test_provision_app_password_wakes_user_manager covering the
api/passwords.py wake path (previously only LFv2 web + MCP tool were tested).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Background vector sync discovered newly provisioned users only on the
periodic user-manager poll (VECTOR_SYNC_USER_POLL_INTERVAL, default 60s),
delaying first indexing by up to a minute. Add a ProvisionSignal doorbell
that provisioning paths ring after storing a user's app password, waking
user_manager_task to re-poll and spawn the user's scanner at once. The
periodic poll remains the backstop (covers cross-replica provisioning).
- ProvisionSignal (stable reference, wait-and-re-arm) held on
VectorSyncState; closes the lost-wakeup window (no await between observing
the ring and re-arming; anyio.Event stickiness covers a mid-poll ring)
- user_manager_task races its poll timeout against the doorbell + shutdown
- notify_user_provisioned() rung from the three app-password provisioning
sites: Login Flow v2 web, MCP provisioning tool, management/BasicAuth API
Note: the pre-existing scanner_wake_event was never .set() and only wakes
existing scanners; a brand-new user has none, so the manager is what must
be nudged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review on PR #891 (no blockers):
- Add test_encode_dav_path_encodes_exactly_once pinning the documented
decoded-input precondition ("already%20encoded.pdf" -> "already%2520...").
- format_exception_group: proper singular/plural ("1 sub-exception" vs
"N sub-exceptions") instead of "(s)".
- oauth_sync: use `if doc_task is not None:` to match processor_task's guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review on PR #893:
- record_qdrant_operation("upsert","error") now fires only when the exhausted
retry was actually a Qdrant failure (reason=="qdrant"); an embed/connection
failure exhausts retries before Qdrant is called, so attributing it to
mcp_qdrant_operations_total{error} inflated that signal. The cause is still
captured by record_ingest_dropped.
- Add test_mistral_embed_retries_on_5xx: exercises the full Mistral retry path
(5xx SDKError then success), not just the predicate.
- Add test_generate_does_not_retry_on_bad_request: generate() fast-fails on a
permanent 4xx.
- Move astrolabe_vector_ingest_dropped_total's definition into the astrolabe_
pipeline-metrics block (was in the mcp_ section).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review on PR #893:
- Add test_generate_retries_on_connection_error (generate() shares the transient
retry; guards the decorator against accidental removal).
- Add test_process_document_records_drop_on_exhausted_retries: drives
process_document to retry-exhaustion and asserts record_ingest_dropped is
called once with the classified reason (processor-level coverage, not just the
_drop_reason unit).
- Note in _drop_reason that a multi-failure group is labelled by its first leaf
(best-effort, no "mixed" bucket).
SonarCloud: the quality gate was failing on new_security_hotspots_reviewed
(S5332 "use https") from http:// URLs in the test _req() helpers — switched to
https:// (mirrors commit 98c9d58e).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review on PR #893:
- _drop_reason now descends through nested ExceptionGroups to the first leaf
(was single-level), so a doubly-wrapped cause isn't mislabelled "other";
added a nested-group test. Commented why both the httpx and openai isinstance
branches exist (raw Nextcloud-API errors vs SDK-wrapped variants).
- Documented that generate() intentionally shares the broadened transient retry
(RAG sampling path), with the worst-case latency note.
- Added a docstring note to process_document on how the provider-level retry
(5x) layers over the outer loop (3x in-process / 1x procrastinate).
- Added test_embed_batch_retries_on_connection_error for the batch path.
- Renamed test_retry_reraises_non_rate_limit_immediately ->
test_retry_reraises_when_predicate_returns_false (it tests the predicate, not
a specific status).
SonarCloud:
- S5708 (BLOCKER) on the helper's dynamic `except exception_type`: the type is
constrained to BaseException/tuple by the signature; suppressed with a
justified NOSONAR.
- S7503 (async without await) in the embed-retry test: use AsyncMock side_effect
instead of a hand-rolled async function.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review on PR #891:
- Guard processor_task's broad except handler against an unbound doc_task
(mirrors multi_user_processor_task): initialise doc_task=None before the loop
and branch the error log. Fixes a latent NameError if receive() raises a
non-TimeoutError/EndOfStream before the first document binds. Regression test
added.
- Drop the unnecessary `from __future__ import annotations` in vector/_errors.py
and express format_exception_group's non-group fast path as an explicit
isinstance check.
- Add a copy_resource Destination-header encoding test (analogue to MOVE);
strengthen the ExceptionGroup test to assert the full leaf repr survives.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From card 309 (OHR-Bench smoke-test triage): during a backend-pod rollover the
embedding endpoint was briefly unreachable, and openai.APIConnectionError /
ConnectError propagated unretried (the provider only retried 429). Documents
exhausted the 3 in-process retries and were dropped for that scan cycle.
Broaden the provider-level retry to the transient set -- APIConnectionError,
APITimeoutError, 429, and 5xx -- on the existing exponential backoff (2s->60s,
5 attempts), so a few seconds of retry rides through the rollover. Permanent
4xx (auth, bad request) still re-raise immediately. Generalize the shared
_retry helper (retry_on_rate_limit -> retry_on_transient, predicate renamed to
should_retry, accurate log label) with a back-compat alias; Mistral gets 429+5xx
for parity. The production gateway path inherits this via GatewayProvider, which
delegates to the decorated OpenAIProvider methods.
Add astrolabe_vector_ingest_dropped_total{reason}, incremented when a document
exhausts retries, classified (connection|timeout|rate_limit|server|qdrant|other)
by _drop_reason so the embed-drop rate is alertable per cause. Dropped docs are
NOT marked failed, so the next full scan re-picks them (re-queue via scan loop).
Refs: Deck board 12 card 309 (AC #1 no permanently-dropped docs; embed-drop
metric for AC #5).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two ingest-robustness fixes from card 309 (OHR-Bench smoke-test triage).
WebDAV paths flowed through the client already URL-decoded (unquote on the
PROPFIND/REPORT <d:href>, or raw MCP-tool input), so a '#' reached httpx as a
URL fragment and silently truncated the request -> spurious 404 on otherwise
valid files (e.g. law filenames with '#', commas, double/trailing spaces).
Route every caller-path builder through a new _webdav_path helper that
percent-encodes the path once (preserving separators); the MOVE/COPY
Destination header is encoded too.
Vector-sync runs inside anyio task groups, so a child-task failure surfaced as
a BaseExceptionGroup whose str() is the useless "unhandled errors in a
TaskGroup (N sub-exception)" -- hiding the real ConnectError operators need.
Add format_exception_group to flatten the group to its leaf exceptions and use
it at the broad catch/log sites in processor.py and oauth_sync.py.
Refs: Deck board 12 card 309 (AC #4 filename handling, AC #2 observability).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Addresses round-1 review on #878:
- Move the eager `document_processors` imports out of the API startup graph:
`app.py` (get_registry now imported inside initialize_document_processors,
after the disabled early-return) and `vector/processor.py` (get_registry now
imported at its single use site). Importing `app` + `cli` no longer loads
`document_processors` / `_isolation` at all -- the #877 stack is fully out of
startup (pymupdf still loads via search/pdf_highlighter, a Windows-compatible
and separately-tracked concern).
- Make `tests/unit/test_pdf_parse_isolation.py` importable on Windows: guard the
top-level `import resource` with try/except and skip the three rlimit
computation tests via a `requires_resource` marker when the module is absent.
The Windows no-op / import-guard tests don't use the real module and still run.
- Fix the `# pragma: no cover` comment on the win32 branch to be accurate.
- Add `enable-cache: true` to the package-smoke setup-uv step.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Billing product model finalized (Deck #281): bill pages externally, record
tokens internally. Rename the data-plane metric literals to match the now-
canonical contract (Deck #284) — the control plane's METRIC_EVENT_NAMES is
already renamed, so the old names would be unmapped and never sync to Stripe.
Rename (values unchanged):
- embeddings_queries → tokens_embedded (value = real token count, already
emitted by this PR; the unit upstream providers bill on).
- pages_chunks → pages_embedded (value kept as len(chunk_texts) interim;
TODO(#282): real normalized "pages indexed" count — real pages for paginated
types, chars/tokens-per-page constant otherwise — is deferred to the
instrumentation card, this only lands the name/contract).
- All literals, log strings, docstrings, comments, the migration comment, and
tests renamed; grep confirms zero old strings remain.
Observability (new): export embedding token cost to Prometheus as
astrolabe_embedding_tokens_total{provider,operation} (operation = index|query)
so the billed cost unit is visible in Grafana, not just the per-tenant billing
DB. Dedicated counter (doesn't inflate the existing chunk/request metrics) and
always-on (independent of USAGE_METERING_ENABLED, so OSS/self-host gets it).
Wired on both the indexing batch embed and the search query embed (query inside
the per-request cache-miss branch, so reused embeddings aren't double-counted).
Note: the rename orphans any pre-existing embeddings_queries/pages_chunks rows
in tenant app DBs (CP no longer maps them) — acceptable; pipeline is inert with
throwaway dev/sandbox data.
Deck #284 (folded into PR #875).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-5 claude-review (merge-ready; all nits):
- 🟡 Added test_empty_doc_types_normalizes_to_null pinning doc_types=[] → None
in record_search_usage metadata (matches the None case).
- 🟡 record_search_usage docstring now notes nc_semantic_search_answer always
meters with doc_types=None (it exposes no doc_types parameter).
- 🟢 BM25HybridSearchAlgorithm.__init__ now sets query_embedding /
query_token_count alongside _embedded_query, so all three cache fields are
instance attributes from construction (was relying on the class-level
SearchAlgorithm defaults).
- 🟢 Ollama embed_batch_with_usage caches _dimension inline (mirrors
OpenAI/Mistral), so the dimension is set via any embed path.
- 🟢 record_indexing_usage documents the independent-record / partial-failure
semantics under SUM aggregation.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 claude-review findings (no blockers):
- 🟡 Untested server-layer metering hook (raised across rounds): extracted the
nc_semantic_search embeddings_queries recording into a module-level
record_search_usage() helper (mirroring record_indexing_usage) and added
tests/unit/server/test_semantic_metering.py — value = query token count,
flag-off no-op, None token → 0, doc_types metadata bounding, best-effort
failure swallowed.
- 🟡 Dedup-hit skipped metering invisibly: the existing dedup info log now
states "no embedding/usage recorded" so a "fewer embeddings_queries rows than
expected" audit lands on the dedup path directly.
Deferred 🟢 nits (stated on the PR): search 0-token rows are recorded
deliberately (the query embedding ran; zero is a sum no-op) — documented in the
helper; embed_tokens closure locality and the OpenAI embed() dual path are
unchanged (correct as-is / separate refactor).
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 claude-review findings:
- 🟡 Base-class recursion invariant: documented on embed_with_usage /
embed_batch_with_usage that a provider overriding embed()/embed_batch() to
delegate to the *_with_usage variant MUST also override that variant, or the
two recurse. (No recursion today; the shipped providers pair the overrides.)
- 🟡 Processor metering had no unit test: extracted the two-event recording
into a module-level record_indexing_usage() helper and added
tests/unit/test_processor_metering.py (value mapping, flag/zero-chunk no-ops,
best-effort failure swallowed).
- 🟡 SonarQube hotspots (python:S5332) were 3 http:// URLs in the new test
fixtures (mock hosts, never contacted) blocking the quality gate
(new_security_hotspots_reviewed). Switched them to https:// so no hotspot is
raised.
- 🟢 Zero-chunk guard: record_indexing_usage() no-ops when chunk_count == 0, so
an empty document no longer writes zero-value billing rows.
Deferred (stated on the PR): Mistral x.index-or-0 sort key (pre-existing,
equivalent), CHANGELOG note for the Ollama /api/embed switch (CHANGELOG is
commitizen-generated from commit bodies, which document it), class-var
query_token_count (safe under the per-request instance pattern).
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
embeddings_queries now records the embedding request's token count (the unit
upstream providers bill on) instead of an operation count, and fires on the
indexing path too. Previously only semantic search recorded it (value=1), so a
re-indexing run produced no embeddings_queries events at all — only pages_chunks.
- Provider layer: additive embed_with_usage / embed_batch_with_usage surface the
per-request token count (Mistral/OpenAI usage.total_tokens, Bedrock Titan
inputTextTokenCount, Ollama prompt_eval_count); a char-based estimate is the
fallback (Simple, and any provider/response without a token field). Gateway and
EmbeddingService forward through. The count travels as a return value / a
per-request SearchAlgorithm attribute — never on the singleton — so concurrent
indexing + search can't mis-attribute bills.
- Indexing (vector/processor.py): records embeddings_queries (value=batch tokens)
alongside the existing pages_chunks event.
- Search (server/semantic.py): value is now the query embedding's token count,
relayed from BM25HybridSearchAlgorithm via query_token_count.
The astrolabe_embeddings_queries Stripe meter (sum aggregation) now sums tokens
with no CP/Terraform change. The meter "queries"->tokens naming/unit
clarification (homelab-terraform #254) + CP rollup/portal copy is a follow-up.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Extract `_app_enabled` to a module-level helper so the gate predicate is
unit-tested directly instead of via an inline copy that could drift.
- Move `import logging` to module scope in test_scanner_app_gating.py.
- Harden `get_enabled_apps` OCS-envelope parsing (`X or {}` / `or []`) so a
present-but-null `ocs`/`data` coerces to empty instead of raising on
`None.get`; add parametrized malformed-envelope tests.
- Use https:// in the test request URL (SonarCloud S5332 hotspot).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector-sync scanner polled every indexed app (Notes, Files, News,
Deck) for every provisioned user on each scan cycle. When a user lacks
an app, its REST API returns 404; these were caught (indexing
continued) but flooded tenant logs with repeated 404s, scaling with
users x disabled-apps x scan-frequency and masking real failures.
Add NextcloudClient.get_enabled_apps(), which reads the per-user
/ocs/v2.php/core/navigation/apps endpoint (respects group
restrictions). Chosen over /cloud/capabilities because the News app
advertises no capability and never appears there.
scan_user_documents now resolves the enabled-app set once per cycle and
skips the Notes/News/Deck scans for apps the user lacks. Files stays
unconditional (core Tags API, not a 404 source). Detection failures
fall back to scanning every app (prior behaviour), so a transient
nav-endpoint blip never silently halts indexing; the per-app 404 guards
remain as the safety net.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- 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>
- remove accidentally-committed .claude/scheduled_tasks.lock (Claude Code
runtime artifact swept in by `git add -A`) and gitignore it; the rest
of .claude/ stays tracked.
- store: cache UsageEventStore.shared() as a process-wide instance so the
hot search path doesn't allocate a fresh wrapper per metered query (the
wrapper is stateless beyond its storage handle).
- hooks: pass enabled=True directly (the outer guard already confirmed
the flag) instead of re-reading settings.usage_metering_enabled.
- migration: document the no-TTL retention design (control-plane rollup
owns the lifecycle; the data plane only appends).
- tests: assert the best-effort error path logs at WARNING (observability
contract).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
Deck #67 data-plane slice: tenant Pods record billable operations
(embedding queries, pages/chunks embedded) into an app-DB usage_events
table that the control plane later pulls read-only into the billing
ledger and syncs to Stripe Meter Events.
- migration 007: usage_events table (Postgres TIMESTAMPTZ/JSONB/UUID
with portable SQLite fallbacks), indexed (occurred_at, metric) for the
CP rollup's per-day range scan + GROUP BY metric.
- UsageEventStore: best-effort, flag-gated writer reusing the shared
RefreshTokenStorage engine; ON CONFLICT (event_id) DO NOTHING for
idempotent retries; dialect-branched occurred_at bind. All work
(incl. metadata JSON encode) is swallowed so a metering failure never
surfaces to the user op.
- USAGE_METERING_ENABLED flag (default off) wired through Settings +
env map; off-path touches no storage, so OSS self-hosters get an empty
table and zero write overhead.
- two recording hooks: embeddings_queries (per nc_semantic_search, which
nc_semantic_search_answer reuses) and pages_chunks (after dense
embedding succeeds, covering both in-process and procrastinate paths).
- storage.acquire()/.dialect public seams so the sibling store doesn't
reach into the underscored internal.
- tests parametrized over SQLite + Postgres: flag-off no-op, roundtrip,
ON CONFLICT dedup, JSON/NULL metadata, and the best-effort swallow of
both DB errors and unserializable metadata.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 4 on PR #868: tighten the assign_page_numbers
guard from `page_boundaries is not None` to a truthy check, so a PDF with an
empty boundary list no longer enters the trace span and fires the alarming
"NO page numbers assigned" warning for a harmless no-op.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 2 on PR #868:
- Extract the use_page_aware branching into a pure `should_use_page_aware`
helper and cover the (doc_type, page_boundaries, page_aware_setting) matrix
in tests/unit/test_processor_routing.py (file+boundaries+enabled, empty
list, None, non-file doc types, disabled setting).
- Clarify the PageAwareChunker.chunk_text no-boundaries comment: the processor
pre-filters via should_use_page_aware, so that branch is a direct-call safety
net, not a production indexing path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 1 on PR #868:
- use_page_aware now gates on `bool(page_boundaries)` instead of
`is not None`, so a PDF that yields an empty boundary list takes the
char-based path explicitly (assign_page_numbers no-ops on []) rather than
the page-aware chunker's no-boundaries fallback. Same result, clearer intent.
- add test_oversized_page_with_leading_whitespace_offsets, exercising the
start+start_index offset path for an oversized page whose sub-chunks have
leading whitespace.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add PageAwareChunker, which splits paginated documents (PDFs) on page
boundaries first and only character-splits pages larger than chunk_size.
No chunk spans a page boundary, so page_number is always exact and stored
excerpts never lead with a neighbouring page's text. When chunk_size is at
least the largest page, this yields exactly one chunk per page: a
predictable vector count (== page count), a flat per-page embedding cost,
and zero cross-page overlap duplication.
Gated by DOCUMENT_CHUNK_PAGE_AWARE (default true). When false, the legacy
char-based DocumentChunker + post-hoc assign_page_numbers path runs
unchanged. Only doc_type="file" with page_boundaries (PDFs) takes the
page-aware path; notes/deck/news are unaffected.
Measured on a 15-page record (query "leadership award louis", target =
top-half of page 15): char-based degraded the target to dense-rank 10 at
cs=2048 (OCR) and mislabeled its page; page-aware restored rank 1 across
every fusion/modality and chunk size, with correct page labels and clean
snippets.
BREAKING CHANGE: PDFs are re-chunked page-aware by default. Existing
deployments will re-index PDF content on the next vector sync (different
chunk counts and page_number labels). Set DOCUMENT_CHUNK_PAGE_AWARE=false
to retain the previous char-based behaviour.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tagging an existing file/folder emits only OCP\SystemTag\MapperEvent — never a
Node*Event — so tagged PDFs were previously only picked up by the hourly
scanner. Subscribe to the tag event and reconcile membership so adding/removing
the `vector-index` tag (re)indexes in near-real time.
- webhook_presets: add OCP\SystemTag\MapperEvent to the files_sync preset
(NC 32+, where MapperEvent gained getWebhookSerializable(); harmless on older
servers — it just never fires).
- webhook_parser: parse MapperEvent (objectType=files) into a path-less file
"reconcile" task. The payload carries only a fileid + tagIds (no name/path),
so assign and unassign both collapse to a reconcile.
- processor._reconcile_tag_event: resolve the fileid against the user's current
vector-index PDFs (find_files_by_tag). Present -> index with the resolved
path/etag; absent -> flip to delete. Naturally handles "an unrelated tag
changed" and a tagged folder's own fileid (no-op; the scanner still expands
folders to descendants).
- Unit tests for the parser branch and the reconcile.
The matching admin-UI preset change ships separately in the astrolabe app repo.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review round 3:
- document_tier1_engine / document_ocr_provider now validate + normalize via
Settings.__post_init__ _enum_fields (the repo's canonical opt-in-enum pattern;
case-insensitive) instead of dynaconf Validators. A typo now raises ValueError
at load and "Gateway" normalizes to "gateway".
- classify_from_text gates no_text_layer/bad_text_layer on ocr_frac >=
OCR_PAGE_FRACTION, matching classify_pdf -- a "fast"-routed doc with a few junk
pages no longer emits a misleading flag (keeps the shadow vs hot-path
classification metrics consistent).
- build_ocr_backend warns when an EXPLICIT provider is misconfigured
(gateway without EMBEDDING_GATEWAY_URL, mistral without MISTRAL_API_KEY)
instead of silently returning None.
- Pypdfium2FastProcessor.health_check probes the import; documented why
OcrProcessor.health_check is unconditionally True (lazy per-tenant backends).
- Removed the leftover per-boundary / per-chunk debug logging loops.
Tests: enum normalization + rejection for the two new settings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review:
- 🔴 OcrProcessor now resolves its backend once and reuses it. Rebuilding per
call created a fresh GatewayTokenProvider each time -- discarding its M2M-token
cache, so every OCR'd document fetched a new token -- and a new Mistral client.
- 🔴 build_ocr_backend uses explicit ValueError (not assert, which is stripped
under `python -O`) for the gateway M2M triple.
- PIPELINE_TIER in the Qdrant payload now reflects the tier that actually
produced the doc: the registry stamps result.metadata["pipeline_tier"] and the
processor reads it (was hardcoded "fast", wrong for OCR/structured).
- Escalation now requires classification.page_count > 0, so a zero-page
(empty/corrupt) PDF isn't pointlessly sent to OCR; documented that a fast
FAILURE (encrypted/unopenable) is a hard failure and is not OCR-escalated.
- Documented the OCR page_boundaries separator-attribution choice.
- Downgraded the per-document page-boundary / page-assignment INFO logs to debug.
New tests: zero-page no-escalation, pipeline_tier stamping.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces single-engine pymupdf4llm extraction with a tiered pipeline (Deck #205,
follows the tier-0 classifier #855). pypdfium2 becomes the default and only
hot-path PDF extractor; pymupdf4llm is deprecated to a rollback toggle.
Why: pymupdf4llm's O(n^2) find_tables drove the OOM (#852) and the form-PDF
parse timeouts (#856), carries AGPL/commercial licensing liability, and -- per
the benchmarks -- recovers near-zero usable tables on the real corpus. pypdfium2
(Apache/BSD) extracts the same text far faster (Student 1a.pdf: 120s timeout ->
0.2s) with no table-detection bomb.
- document_processors/pypdfium2_fast.py: tier-1 "fast" processor emitting text +
exact page_boundaries (the pdf_highlighter contract). pymupdf processor is now
tier "structured" (the rollback engine), registered but not default.
- registry: tiered routing in ProcessorRegistry. tier-1 fast extracts, then
classification is DERIVED from that text (classifier.classify_from_text -- no
PDF re-open), records the classification metrics, and escalates scanned /
no-text-layer docs to the "ocr" tier when document_ocr_enabled (default off;
no provider yet, so fast is terminal). Wires record_document_escalation + the
real "escalated" span attribute (was hardcoded False).
- Removes the separate _shadow_classify pass from vector/processor.py -- it
re-opened every PDF and re-extracted text (~0.5-1.3s/doc of pure duplicated
CPU that lowered throughput); classification now rides the tier-1 extraction.
- Settings: document_tier1_engine ("pypdfium2" default | "pymupdf" rollback,
enum-validated), document_ocr_enabled (default false).
Tests: pypdfium2 extractor, registry tiering (fast routing, rollback, classify
recording, OCR escalation on/off), classify_from_text. Full unit suite green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review follow-ups (PR #857), both documentation-only:
- sharing_state.py: reconcile_document_path docstring no longer claims it
returns False when no real points exist — it returns True and the set_payload
is a Qdrant-side no-op (callers discard the return value).
- scanner.py: reword the rename-reconcile comment to state the precise reason
(modified_at stable so not re-queued; path may be stale from a rename) rather
than the loose "dedup miss / etag changed" phrasing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review follow-ups (PR #857):
- scanner.py: skip the rename-reconcile when the existing metadata point is a
placeholder. reconcile_document_path only touches real chunks, so a not-yet-
indexed file would just incur a 0-point set_payload; the real index writes the
current path anyway.
- test_sharing_state.py: add a dedup-hit case where the file was renamed AND the
user is new to the ACL, asserting both set_payload writes fire (file_path/title
and acl_principals).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector-sync pipeline derived an indexed file's display title from the
document's embedded metadata (e.g. a PDF's /Title), falling back to the
filename only when absent. That embedded title frequently disagrees with how
the user named the file in Nextcloud and is confusing in the astrolabe
vector-viz UI (a passive consumer of the `title` payload field).
For files, always derive the title from the Nextcloud filename via a shared
`file_title_from_path` helper. Notes/deck/news keep their metadata titles.
A rename/move in Nextcloud keeps the fileid (doc_id) and content (etag/mtime)
but changes the path, so both the dedup claim and the scanner freshness gate
skip re-embedding and the stored file_path/title go stale. Add
`reconcile_document_path`: a metadata-only set_payload that refreshes
file_path + title on the existing real chunks without re-fetch/re-embed.
Wire it into both skip paths:
- dedup hit (etag unchanged on rename) via claim_existing_index(current_path=...)
- scanner incremental skip (etag changed, mtime stable)
Both reuse already-fetched payloads, so steady-state scans add no extra
round-trip (reconcile is a no-op when the path is unchanged).
Refs: Deck #204
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #855 round 2:
- 🔴 _shadow_classify swallowed all exceptions at DEBUG, so a systematic
failure (pymupdf bug, memory pressure) is invisible at LOG_LEVEL=INFO and
trips SonarQube S2221/S5754. Log at WARNING instead (still best-effort --
indexing is unaffected).
- classifier: use `with pymupdf.open(...) as doc` instead of manual try/finally.
- tests: release the Pixmap's native memory (del pix) in the image fixtures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
First step of the tiered document-processor effort (Deck #203): a cheap, local
pre-pass that recommends which extraction tier a PDF should start in, emitting
metrics WITHOUT changing routing yet -- so we gather per-tenant doc-mix data
before turning escalation on.
document_processors/classifier.py: classify_pdf(content) -> DocClassification.
Page-sampled (bounded on large docs), <~1s. Cheap signals only -- text-layer
chars, a text-quality score (catches the "Student 147" failure where a text
layer exists but is mashed/space-less junk), and image coverage. A page that is
mostly a raster image routes to OCR: its content (handwriting, stamps) isn't in
any text layer. Deliberately no get_drawings/graphics-density signal -- it's
slow on the exact pages it'd flag, the hotfix's graphics_limit already makes the
parse safe, and the (future) tier-1 quality gate catches lost tables.
Validated on the sample corpus: born-digital 2-col arxiv and a digital student
record -> fast (tier 1); a scanned+handwritten form -> ocr (tier 3).
Wiring (vector/processor.py): _shadow_classify runs the classifier on PDFs in a
worker thread, best-effort (never blocks/fails indexing), gated by the new
DOCUMENT_CLASSIFY_ENABLED setting. Metrics: astrolabe_document_classified_total
{recommended_tier}, astrolabe_document_classifier_flag_total{flag},
astrolabe_document_text_quality histogram.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 round 3 (all 🟡, no blockers):
- config: DOCUMENT_PARSE_TIMEOUT_SECONDS is now float (default 120.0) so a
fractional value is honoured rather than silently stored in an int field;
matches anyio.move_on_after's float seconds.
- _isolation: comment that a clean rlimit MemoryError leaves the worker alive
in anyio's pool (vs the SIGKILL/BrokenWorkerProcess path that respawns) --
acceptable since RLIMIT_AS caps virtual address space, not RSS.
- processor: note the `if indexed is False` is a deliberate identity check --
a successful index (incl. dedup hit) returns None and must not be mistaken
for a parse failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 round 2:
- config: DOCUMENT_PDF_GRAPHICS_LIMIT validator is now gte=1 (pymupdf4llm treats
0 as "no cap", which would re-expose the OOM); documented the zero semantics
and that the per-worker mem rlimit needs a pod restart to change.
- processor: annotate `_index_document -> bool | None` and document the contract
so the `if indexed is False` check is explicit/type-checkable.
- tests: add the RLIM_INFINITY-hard branch assertion for _apply_mem_limit
(soft==target, hard stays unbounded).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 review:
- pymupdf.py: the metadata `doc` was only closed on the PdfParseFailed and
success paths, so a failure in `_extract_metadata`/`mkdir`/`get_settings`
leaked it. `doc` is only needed for metadata + page_count (the heavy parse
works from `content` bytes in the worker), so open it, read metadata, and
close it immediately under try/finally; drop the two later doc.close() calls.
- processor.py: a permanent parse failure early-returned from `_index_document`,
after which `process_document` still recorded record_qdrant_operation("upsert",
"success") + record_vector_sync_processing(success) -- counting an OOM/timeout
bomb as astrolabe_documents_indexed_total{status="success"}. `_index_document`
now returns False on that path and the caller skips the success metrics (the
failure is already recorded via document_parse_failed_total + the registry's
document_parse_total{error}).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The document processor crash-looped on one pathological PDF: pymupdf4llm's
table/graphics detection over a page with ~1M vector path items ballooned past
the 2 GiB pod limit. The parse ran in a thread, so nothing could interrupt or
memory-bound it -- a single bad file OOM-killed the whole pod.
Run the parse in an isolated worker subprocess (anyio.to_process, cancellable)
with an RLIMIT_AS memory cap and a wall-clock timeout, so a pathological file
fails THAT document instead of the pod (new document_processors/_isolation.py).
Also pass graphics_limit (default 5000) to to_markdown -- validated to cut the
known trigger page from 112 s to 23 s with bounded memory.
On a permanent parse failure the processor returns success=False (instead of
raising, which would retry 3x); vector/processor.py marks the placeholder
"failed" and skips indexing, and the scanner stops re-queuing failed placeholders
until the file changes -- so a doomed file no longer churns.
New per-tenant (per-pod env) settings: DOCUMENT_PDF_GRAPHICS_LIMIT,
DOCUMENT_PARSE_TIMEOUT_SECONDS, DOCUMENT_PARSE_MEM_LIMIT_MB. New metric
astrolabe_document_parse_failed_total{reason=timeout|oom|error} surfaces hard
failures that previously killed the process before any except ran.
First PR of the tiered document-processor effort (Deck #199); tier 0/1/3
pipeline tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Starlette lifespan started `vector_sync_metrics_task` with undefined
names `task_producer` and `receive_stream`. Those locals only exist inside
the `_wire_vector_sync_state` helper; in the lifespan the transport is bound
as `ingest_transport`. The undefined reference raised `NameError`, which
aborted the background-sync task group and crashed startup in every
deployment mode ("Application startup failed. Exiting.").
Introduced by fbe70ecd ("feat: backend-agnostic vector-sync gauges").
Pass `ingest_transport.producer` / `ingest_transport.receive_stream` at both
call sites (single-user app.py:1791, OAuth/login-flow app.py:2012).
Also fix 10 pre-existing `ty` possibly-missing-attribute diagnostics: the
deck indexing code in scanner.py, processor.py and search/context.py reads
full-DeckCard-only fields (description, type, owner, etag, lastModified) off
`stack.cards`, typed `list[DeckCard | DeckCardSummary]`. Freshly-fetched
stacks from `get_stacks()` always hold full DeckCards (the summary
projection only happens in the tool layer), so narrow with
`cast(list[DeckCard], ...)` — matching the existing pattern in
server/deck.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- _clear_vector_sync_state also nulls shutdown_event / scanner_wake_event on
shutdown, symmetric with the stream/producer fields (the next startup rebinds
them via _wire_vector_sync_state).
- Comment that the "DocumentTask" string subscript in LocalTransport is
intentional (TYPE_CHECKING-only class; anyio ignores the runtime type arg).
- Move app.py's annotation-only IngestTransport / TaskProducer imports under
TYPE_CHECKING (the module uses `from __future__ import annotations`), keeping
only build_transport at runtime.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Clear the module-singleton ingest references (task_producer,
document_send_stream, document_receive_stream) on lifespan shutdown via a new
_clear_vector_sync_state() helper, mirroring the eviction_task_group cleanup.
Defense-in-depth so a late webhook (or a module-singleton integration test)
can't touch a producer/stream backed by an already-closed resource.
- Add IngestTransport.backend_name ("memory"/"postgres") and use it in both
lifespan log lines, removing the last settings.ingest_queue read from the
background-sync setup — the lifespan no longer inspects the backend at all.
- Cover backend_name in the build_transport adapter-selection tests.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- LocalTransport.run_consumers increments active_consumer_count per worker
(instead of once after the loop) so the count is accurate if a later
tg.start() raises mid-pool.
- Add LocalTransport.aclose() to explicitly close its owned send/receive stream
ends (belt-and-suspenders against unclosed-resource warnings; anyio aclose is
idempotent, and by shutdown the scanner is already winding down). Reworded the
base IngestTransport.aclose() docstring to point at the overrides.
- Inline ingest_transport.producer at the scanner/user_manager call sites,
dropping the single-use task_producer alias in both lifespan paths.
- Annotate DistributedTransport._producer explicitly as ProcrastinateTaskProducer
so the drain() coupling is visible and ty catches drift.
- Add a unit test for LocalTransport.aclose() (closes the owned streams,
idempotent).
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename the lifespan-local `transport` to `ingest_transport` in both paths so it
no longer shadows the get_app(transport=...) HTTP-transport parameter.
- Log the memory backend selection in build_transport, symmetric with the
postgres branch, so startup logs name the chosen ingest backend either way.
- Note in _wire_vector_sync_state why eviction_task_group is intentionally not
set there (it only exists once the lifespan's task group is running).
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
