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-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>
SonarQube flagged three float equality checks in the classifier tests
(python:S1244, "do not perform equality checks with floating point values"):
the _text_quality empty case and the ocr_page_fraction 0.0/1.0 assertions now
use pytest.approx.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #855 round 3 (non-blocking test completeness):
- Add a test that a mostly-digital doc with one full-page image carries the
image_heavy flag yet still routes fast (ocr_frac < OCR_PAGE_FRACTION) -- the
flag-vs-routing asymmetry operators read in the metrics, now guarded against
silent regression.
- test_full_page_image_routes_ocr also asserts the scanned flag (no text layer).
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>
Address PR #855 review (all non-blocking):
- classifier: _sample_indices now always includes the first AND last page (the
old evenly-spaced sample missed the tail, e.g. last sampled index 95 on a
100-page doc -- a scanned tail could be missed).
- classifier + metrics: document that flags are diagnostic and fire
independently of routing (image_heavy on ANY page vs the ocr route needing a
page FRACTION), so flag{image_heavy} is expected to exceed classified{ocr}.
- classifier: clarify the text-quality whitespace comment (caps at 12%) and note
the image double-count approximation (min() caps coverage).
- tests: add the scanned (no text layer) and bad_text_layer (junk text over an
image) flag paths, and a test pinning first/last-page sampling.
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>
The OOM hotfix (#852) set graphics_limit=5000, which caught the 955k-drawing
bomb but let a second pathology through: form/table PDFs (e.g. student records)
have ~1.5k grid-line vector drawings per page -- under 5000, so uncapped. With
those pages uncapped, pymupdf4llm's O(n^2) find_tables grinds ~17s/page, so a
7-page form hits the 120s timeout. All 6 current backfill parse failures in
tenant-blackbox-demo are this exact timeout (zero OOM, zero error).
Measured on a 7-page sample: graphics_limit=2000 -> 119s (timeout), 1000 -> 2.9s
-- with identical extracted text and ZERO recovered tables either way (the
expensive analysis produces nothing useful on these dense forms). Lowering the
default to 1000 makes them index in ~3s; the bomb file (955k >> 1000) stays
capped, and pages with genuine simple tables (<1000 drawings) still get table
detection.
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>
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>
- 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>
Extend the documents-vs-chunks split to the remaining status surfaces so all
three report consistently (Deck #195):
- nc_get_vector_sync_status MCP tool + VectorSyncStatusResponse: add
indexed_documents (distinct) and indexed_chunks; keep indexed_count as a
deprecated alias of indexed_chunks. Reuses count_indexed.
- userinfo HTML page (/app/vector-sync/status): show Indexed Documents AND
Indexed Chunks rows; switch its count to count_indexed (which also excludes
placeholder points — the old raw count included them).
- /api/v1/vector-sync/status: restore indexed_count as a deprecated alias of
indexed_chunks so existing consumers (integration tests, pre-#115 UI) keep
working; the change is now purely additive for indexed_count.
Tests: VectorSyncStatusResponse documents/chunks/alias + zeroed defaults.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add IngestTransport.active_consumer_count (0 by default; LocalTransport stores
the started count) so app.py logs the worker count without re-checking
INGEST_QUEUE — the last backend-knowledge leak in the lifespan is gone.
- Document that DistributedTransport is postgres/procrastinate-specific by design
(aclose() calls ProcrastinateTaskProducer.drain()); other distributed backends
would be separate IngestTransport subclasses.
- Clarify the _wire_vector_sync_state log line (writes app.state + singleton, not
only the singleton).
- Strengthen the LocalTransport test: assert active_consumer_count transitions
0→N and that each worker receives a distinct cloned receive stream.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Note at both metrics-task call sites that receive_stream is None in postgres
mode (get_ingest_pending falls back to procrastinate counts).
- Add test_default_is_exact_true covering the status-endpoint count path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Finish the hexagonal ports-&-adapters split started in #183. The producer side
already had a TaskProducer port + adapters, but the consumer side was
unabstracted and the INGEST_QUEUE selection leaked into a duplicated
`if use_postgres:` branch across both app.py lifespan paths.
Introduce an IngestTransport ABC (vector/queue/transport.py) that bundles the
producer with running (or not running) the in-process consumer pool, built by a
single build_transport() factory:
- LocalTransport (INGEST_QUEUE=memory): in-process anyio stream drained by an
N-worker pool that run_consumers starts.
- DistributedTransport (INGEST_QUEUE=postgres): wraps ProcrastinateTaskProducer;
run_consumers is a no-op because the consumer is the external `worker` role.
Both lifespan paths now call build_transport + _wire_vector_sync_state (new
helper that centralizes the app.state / module-singleton / browser-app writes) +
transport.run_consumers + transport.aclose(), with no INGEST_QUEUE branching and
no getattr drain probe. Adding a future backend (Redis/NATS/SQS) is one new
adapter + one build_transport arm, with no app.py or scanner change.
Preserves the single-tenant parallelism invariant (one shared multiplexed queue
+ N-worker pool, per-document not per-user dispatch) and documents it in
ADR-028. The worker CLI is unchanged (it is the external consumer).
Refs: Deck #196 (Deck #197 tracks the explicit parallelism regression test)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add Validator("VECTOR_SYNC_METRICS_REFRESH_INTERVAL", gte=1) so a 0/negative
value can't turn the publish loop into a busy-spin.
- Annotate count_indexed's qdrant_client param as AsyncQdrantClient.
- Add tests: exact kwarg is forwarded to qdrant count, and the placeholder
filter matches False (excludes placeholders) with chunk_index pinned to 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The only queue metric, mcp_vector_sync_queue_size, was updated inline by the
single-user consumer (processor_task) but never by the multi-user consumer
(oauth_processor_task). On multi-user tenants (e.g. blackbox-demo, 5 users) the
gauge read 0 for 24h while the live anyio buffer held ~2214 pending documents
(shown by /api/v1/vector-sync/status). The "indexed" figure was also a chunk
count (16039 points ≈ 480 docs) mislabelled as documents.
Publish a consumer-independent snapshot from a periodic task
(vector/metrics_publisher.vector_sync_metrics_task), spawned in BOTH lifespan
task groups (single-user and multi-user) and every queue backend:
- mcp_vector_sync_pending_documents — outstanding work via
ingest_status.get_ingest_pending() (anyio buffer depth or procrastinate
todo+doing); also keeps the legacy queue_size gauge meaningful on all paths.
- mcp_vector_sync_indexed_documents — distinct documents, counted exactly and
cheaply via the one chunk_index=0 point per document (no facet).
- mcp_vector_sync_indexed_chunks — total non-placeholder points.
The /api/v1/vector-sync/status endpoint now returns indexed_documents (distinct
docs) AND indexed_chunks separately, so documents and chunks are no longer
conflated. The publisher uses approximate Qdrant counts (every-N-seconds gauge);
the on-demand endpoint counts exactly. New knob:
VECTOR_SYNC_METRICS_REFRESH_INTERVAL (default 20s). Fail-safe: a metrics refresh
never disturbs ingest.
BREAKING CHANGE: /api/v1/vector-sync/status field `indexed_documents` now holds
the distinct-document count (was the chunk count); the chunk count moved to the
new `indexed_chunks` field. The Astrolabe UI + the nc_get_vector_sync_status MCP
tool / userinfo page are harmonized in a follow-up (Deck #195).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Optional review hardening on #849 (non-blocking nits from the approve):
- `_build_search_xml`: emit `<d:firstresult>` on `offset is not None` rather
than truthiness, so a future explicit offset=0 isn't silently dropped.
- `_key`: key on `file_id is not None` so a (hypothetical) file_id of 0 isn't
treated as absent and mis-keyed onto path.
- `_type_search_args`: XML-escape the MIME type before interpolating it into
the SEARCH literal (defense-in-depth for any future user-supplied value),
with a unit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review on #849:
- Critical: add missing `await` on the exception-path fallback in
search_files_all -- it returned a coroutine instead of the result list.
Add unit tests for both the offset-page-raises (fallback) and
offset-zero-raises (propagate) paths, which previously had no coverage.
- Guard `_key` dedup against items missing both file_id and path (fall back
to id(item)) so they can't collapse under a shared None key and drop rows.
- Document the offset-ignored discard-and-refetch decision.
- Split the offset paging into `_search_offset_paged` (returns None to signal
fallback) and share the truncation warning via `_warn_if_truncated`,
cutting cognitive complexity below the threshold (SonarCloud S3776).
- Make the test side_effect helpers synchronous (SonarCloud S7503).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector-sync scanner expanded a tagged folder into its PDF descendants via
`WebdavClient.find_by_type(scope=dir)` with no result limit. A WebDAV SEARCH
with no `<d:nresults>` returns only Nextcloud's default page (~100 on the
affected instance), so large tagged folders were silently truncated and most
documents were never queued for indexing (e.g. a 220-file folder yielded 100).
Add `search_files_all`, which pages the SEARCH to completion. It uses
`<d:firstresult>` offset paging where supported and, because Nextcloud 31
ignores offset (verified against a live instance), detects the repeated page
and falls back to a single bounded fetch with an explicit large `<d:nresults>`.
`find_all_by_type` wraps this and is now used for tagged-folder expansion;
`find_by_type` is unchanged for the interactive MCP tools.
Crossing `WEBDAV_SEARCH_MAX_RESULTS` logs a warning and increments the new
`astrolabe_document_scan_truncated_total` metric, so a coverage cap can never
again hide files silently.
Scope: this fixes discovery only. Cross-user double-processing of identical
shared files (point-ID collisions) is tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A file shared across many users — directly, or via a group folder shared
to a group — was parsed and embedded once per user. Chunk point IDs are
user-agnostic (uuid5(tenant_id, doc_id=fileid, chunk_index)), but the
per-user freshness gate filtered Qdrant by user_id, so two readers
ping-ponged: each overwrote the other's points and each kept seeing "not
indexed for me", reprocessing every scan. Production telemetry (note
386945, finding #5) measured identical docs re-processed every few hours
at 7-13s each, with PDF parse ~62% of per-doc cost.
Layer 1 — tenant-wide dedup:
- Thread the scanner's tag-REPORT etag into the file DocumentTask and the
chunk payload; index `etag` as a KEYWORD field.
- vector/sharing_state.find_indexed_content scrolls tenant-wide (no
user_id filter) for a non-placeholder point matching
(doc_id, doc_type, etag), gated on embedding_identity in Python so a
model switch correctly forces a re-embed.
- Scanner skips enqueue and the processor skips fetch/parse/embed when a
match exists (cross-worker race-guard before WebDAV read). Dedup is
fail-safe: a Qdrant error degrades to "process normally".
Layer 2 — observed-access ACL (no admin / GroupFolders API needed):
- Each point carries `acl_principals` = the set of user:<uid> whose
scanner has observed (hence can read) the file. The per-user tag REPORT
is the access oracle; group membership/GroupFolders enumeration is
admin-only and unavailable in multi-user modes.
- build_ownership_filter ORs MatchAny(acl_principals, ["user:<me>"]) so a
deduplicated shared/group-folder point surfaces to every reader;
verify-on-read (_verify_files) remains the precise ACL gate.
- Deletion/eviction become "release one user": drop the principal and
delete the points only when the set empties, so one user untagging a
shared file doesn't evict it for the others. Legacy points without the
field keep the original per-user delete.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An unset INGEST_QUEUE auto-derived "postgres" whenever DATABASE_URL was
PostgreSQL, silently starting the procrastinate ingest worker (schema
migration, reclaim cron, deferred jobs) on every Postgres-backed tenant —
even though none had opted into the api/worker split. Observed on
tenant-blackbox-demo (:0.98.0): ~600 "Deferred 1 job" log lines / 24h.
Resolve an unset INGEST_QUEUE to "memory" (the in-process anyio queue)
regardless of the database backend. procrastinate is now strictly opt-in
via an explicit INGEST_QUEUE=postgres; the existing guard still rejects
postgres against a SQLite DATABASE_URL. Docs + unit test updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 Document the _doc_queueing_lock ":" delimiter invariant (user_id and the
controlled doc_type enum are colon-free, so the key is collision-safe; a
future doc_type with ":" must not be added).
🟡 API pod no longer opens the procrastinate connector twice on startup: add
ProcrastinateTaskProducer.ensure_schema() (applies the schema on the
already-open pool) and have both lifespan branches build the producer then
ensure_schema — one open/close cycle, matching the worker. build_producer now
returns the concrete producer type.
🟢 Document in ports.py that a long-lived-connection producer may optionally
provide drain() (lifespan probes via getattr).
🟢 Add a unit test that a non-credential pipeline error propagates (for
procrastinate's RetryStrategy) and still closes the client via finally.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 Document why ProcrastinateTaskProducer.connect() uses `await app.open_async()`
(AwaitableContext: await opens a long-lived pool, closed by drain()) and add a
connect()/drain() lifecycle unit test (InMemoryConnector) asserting the pool is
opened by connect and closed by drain — previously untested.
🟡 get_procrastinate_conninfo: forward connect_timeout from DATABASE_URL or
default 10s so an unreachable DB can't hang worker/API startup indefinitely;
warn only on other dropped query params. + tests.
🟢 INGEST_DELETE_SUCCEEDED_JOBS (default true) makes the worker's succeeded-job
deletion configurable for audit retention.
🟢 Worker startup logs via logger.info (structured/OTel) instead of click.echo.
🟢 INGEST_STALLED_JOB_SECONDS (default 300) makes the crash-reclaim threshold
tunable for slow embedding backends; reclaim reads it per-run.
The broad `except` in _apply_ingest_queue_schema_open is kept deliberately:
procrastinate wraps psycopg errors, so narrowing to psycopg.errors.* would miss
the wrapped DDL-conflict and turn a benign concurrent-apply race into a failure;
the presence re-check re-raises genuine errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 3 review follow-ups:
- Enforce the folder cap (MAX_PATH_PREFIXES=20) inside normalize_path_prefixes
so the REST/viz endpoints are bounded too, not just the MCP tool's Field
and the PHP client. Single server-side enforcement point; the MCP tool's
Field(max_length=...) now references the same constant.
- Widen the SearchAlgorithm ABC and both concrete implementations'
path_prefixes param to Iterable[str] | None, matching the widening of
build_base_filter_conditions from the prior round.
- Add a normalize_path_prefixes cap test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 2 review follow-ups:
- Add Field(max_length=20) to the nc_semantic_search path_prefixes param so
an LLM client can't build an unbounded OR-filter (mirrors the cap the
Astrolabe PHP controller applies on the UI path).
- Note in normalize_path_prefixes that the two-pass collect-then-strip is
deliberate (the `if path_prefix:` guard is truthy for whitespace-only
input; the strip pass is what drops it).
- Tests: exercise build_base_filter_conditions with 3 folders (guards the
list comprehension) and parametrize the no-path case over None, empty
list, and blank-only inputs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend the ADR-027 Phase 2 path filter from a single path_prefix to a
list of folders. The new normalize_path_prefixes() helper is the single
source of truth for trimming, dropping blanks, and de-duplicating, and
folds the legacy single path_prefix into the list for backward
compatibility.
build_base_filter_conditions() adds one MatchText to the must clause for
a single folder (unchanged shape) and OR-s multiple folders via a nested
Filter(should=[...]) so a file under any selected folder matches while
still AND-ing against the ACL/doc_type/date conditions.
path_prefixes is threaded through every search surface: the
nc_semantic_search MCP tool, the visualization API (JSON body), and the
viz route (CSV query param). The Astrolabe frontend folder picker that
produces these lists ships in a companion astrolabe PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔴 nc_get_vector_sync_status reported pending=0 for INGEST_QUEUE=postgres: the
AppContext/OAuthAppContext per-session yields snapshotted the stream fields but
never forwarded task_producer, so lifespan_ctx.task_producer was always None.
Convert task_producer to a @property that reads _vector_sync_state live (like
eviction_task_group), removing the snapshot field so the yields can't drop it.
Add a regression test pinning the contract on both contexts.
🟡 Remove the unused _RECLAIM_TASK_NAME constant.
🟡 get_procrastinate_conninfo: warn + document that DATABASE_URL query params
(application_name, connect_timeout, …) are dropped.
🟡 worker: open the procrastinate App once — apply_ingest_queue_schema gains
manage_connection=False so the worker reuses its own open connector instead
of a redundant open/close before run_worker_async.
🟢 Clarify the apply-schema broad-except comment (non-race errors re-raise) and
document the deliberate Any typing in ingest_status.get_ingest_pending.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stop excluding tests/ from the ty-check pre-commit hook so touched test files
are type-checked. Fix the new ingest tests under the now-active check:
- cast duck-typed JobContext / App test doubles to their declared types;
- narrow the gated Postgres fixture's str | None URL (pytest.skip isn't modelled
as NoReturn by ty).
Pre-existing type issues in untouched test modules are unaffected (the hook
checks only changed files); they'll be cleaned as those files are next touched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-architect document ingest from the shared NATS-glued document-processor to a
per-tenant, in-process model owned by nextcloud-mcp-server (Deck #183). The MCP
server now owns both sides of ingest:
- Producer (api role): the scanner defers one job per changed document into the
app's Postgres via procrastinate (queueing_lock dedup; no execution lock, so a
crashed worker can't deadlock a doc — Qdrant upserts are idempotent).
- Consumer (worker role): `nextcloud-mcp-server worker` drains the queue and runs
the existing process_document pipeline; a periodic task reclaims jobs orphaned
in `doing` by a crash.
INGEST_QUEUE selects the transport (auto: postgres when DATABASE_URL is Postgres,
else the in-process anyio queue for SQLite/dev). procrastinate manages its own
tables (applied on a fresh DB at startup and by `db upgrade`). The vector-sync
status surface reads job counts from Postgres in postgres mode. procrastinate +
psycopg3 ship in the [postgres] extra; the app's own engine still uses asyncpg
(driver unification is a follow-up handled in the rendered Helm chart).
NATS JetStream, the Postgres-queue stub, the bus status subscriber, and nats-py
are removed.
BREAKING CHANGE: the external-NATS-ingest env vars are removed
(INGEST_MODE, STATUS_BACKEND, INGEST_BUS_URL, INGEST_BUS_NUM_REPLICAS,
FACT_EVENT_EMITTER). Use INGEST_QUEUE (memory|postgres) and the `worker`
command instead. TENANT_ID is retained (no longer NATS-subject-charset-validated).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a path_prefix filter to semantic search, honoured on both the MCP tool and
the dense-only visualization/API paths through the shared filter contract.
- build_base_filter_conditions: append FieldCondition(file_path,
MatchText(path_prefix)) when set. file_path is only on doc_type == "file"
points, so a non-empty path_prefix implicitly restricts to files.
- Promote path_prefix to an explicit keyword param on the SearchAlgorithm ABC
and both algorithms; thread it through nc_semantic_search (blank ⇒ no filter),
the /api/v1 search endpoints, and the viz route.
- Add a file_path TEXT payload index to _PAYLOAD_INDEX_FIELDS (no content
re-index; idempotent startup migration). MatchText tokenizes on server Qdrant
and matches by substring on local/embedded qdrant-client — both serve folder
scoping.
- Update ADR-027 (Phase 2 implemented; readiness table; semantics note). Tests.
Refs ADR-027 Phase 2. Deck #177.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a modified_after/modified_before date-range filter to semantic search,
honoured on both the MCP tool path (BM25HybridSearchAlgorithm) and the
dense-only visualization/API path (SemanticSearchAlgorithm) through one shared
contract.
- Promote modified_after/modified_before to explicit keyword params on the
SearchAlgorithm ABC and both concrete algorithms; factor the shared
placeholder+ownership+doc_type+date filter into
access_filter.build_base_filter_conditions so new filters land in one place.
- nc_semantic_search: accept RFC 3339 / ISO 8601 (or Unix seconds) bounds via
utils.validation.parse_modified_timestamp; Annotated/Field constraints on the
numeric args; explicit McpError guard for after > before. Thread the parsed
bounds through the cross-app and per-doc_type dispatch.
- /api/v1 search endpoints + viz route parse the same formats and 400 on bad or
inverted ranges.
- Add a modified_at INTEGER payload index to _PAYLOAD_INDEX_FIELDS; the
idempotent _ensure_payload_indexes() startup path migrates existing
collections with no content re-index.
- Update ADR-027 to resolve the review feedback (validation placement, shared
algorithm contract, deferral of nc_semantic_search_answer, payload index,
RFC-3339-at-the-boundary rationale). Add unit tests.
Refs ADR-027. Deck #177.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the two important findings from the claude bot's latest re-review:
- _verify_files: skip get_excluded_file_paths entirely when the tag REPORT
returns no files. An empty `tagged` yields an empty `tagged_ids` regardless
of exclusions, so the lookup's 2xlen(EXCLUDED_TAGS) WebDAV fan-out is wasted
work in the common "this tag matched nothing" case. The per-result loop still
runs, so malformed doc_ids are still kept (fail-open) — pinned by a new test
(test_verify_files_empty_tag_set_skips_exclusion_lookup), which also asserts
the exclusion lookup is never awaited.
- Rewrite the semaphore comment: it claimed "the slot bounds them", but the slot
only caps concurrent *searches* — get_excluded_file_paths internally spawns a
task group issuing 2xlen(EXCLUDED_TAGS) concurrent WebDAV calls, so live
Nextcloud connections can exceed VERIFICATION_CONCURRENCY. Comment now says so
and points at configuration.md.
The third 🟡 (sequential dir expansion in find_files_by_tag) is pre-existing and
flagged by the reviewer as a follow-up, not part of this PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the blocking + important findings from the claude bot's re-review:
- test (blocking): pin the file verifier's fail-open contract for definitive
403/404 on the tag REPORT, not just transient 503/429. A disabled systemtags
endpoint commonly 403s; unlike the per-access verifiers (where 403/404 = drop),
the batch file verifier must keep all results since the whole set hinges on one
REPORT. Adds _http_error(403)/_http_error(404) to
test_verify_files_tag_fetch_failure_keeps_all and documents the asymmetry.
- docs (important): migration caveat — if vector-index was created as
user_visible=False (manual occ tag:add, or pre-release), an owner's tag won't
surface in a recipient's REPORT and shared-file results are silently dropped
after upgrade. Note that the MCP server's get_or_create_tag defaults to
user_visible=True, and how to verify/fix an existing tag.
- docs (important): note the file verifier's latency scales with both the
Depth:infinity folder expansion and the EXCLUDED_TAGS lookup (~2 WebDAV calls
per excluded tag, fanned out under one slot); suggest lowering
VERIFICATION_CONCURRENCY for large excluded-tag lists / deeply tagged trees.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verify-on-read only checked file *accessibility* (file_accessible_by_id),
never tag membership, so a file removed from the `vector-index` tag (but
still readable) kept surfacing in semantic search, and stale points only
got evicted when they happened to rank in a search's top-K.
Rework `_verify_files` to gate on current `vector-index` tag membership via
a single batch `find_files_by_tag(tag, mime_type_filter="application/pdf")`
REPORT per search (plus a one-shot EXCLUDED_TAGS lookup for exclusion-wins
parity) — exactly what the scanner indexes. A file is kept iff it is in that
set, so untagged / deleted / excluded files drop out immediately and the
existing eviction wiring reclaims their Qdrant points. The gate is strict
for all file results, own and shared. Mirrors the batch-fetch-and-intersect
shape of `_verify_news_items` (one semaphore slot, fail-open on fetch error,
malformed-id keep).
- Promote the tag name to a `vector_sync_pdf_tag` Settings field (dynaconf
env mapping VECTOR_SYNC_PDF_TAG) used by both scanner and verifier;
drop the scanner's direct os.getenv.
- Expose `find_files_by_tag` on NextcloudClientProtocol.
- Rewrite the file-verifier unit tests (tagged/untagged/deleted/excluded/
fail-open/non-numeric); update the ACL + verify-on-read integration tests
to seed tagged PDFs.
- Amend ADR-019 and the configuration.md verify-on-read latency budget.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Failed deletes no longer bump astrolabe_documents_indexed_total: the outer
except in process_document now gates doc_type on operation != "delete", so a
delete error is counted as processed-error but not as an indexing event.
Added test_failed_delete_is_processed_but_not_indexed.
- registry parse span: pass record_exception=True explicitly (matches
instrument_tool) and add a structured logger.warning on the parse-error path
(processor/tier/byte_size/duration_ms) for a Loki-aggregatable failed-parse
signal.
- test_error_does_not_increment_throughput: snapshot-before/delta pattern
instead of absolute 0.0 (counters are global singletons).
- config: document the deliberate gateway asymmetry between
get_embedding_model_name() (no gateway branch) and
get_embedding_provider_family() (short-circuits on gateway).
- Cleanup in touched scope: narrow `except (HTTPStatusError, Exception)` to
`except Exception` (drop now-unused import); convert registry signatures from
Optional[...] to `... | None`.
Refs Deck #175, PR #831.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer findings:
- Fix double-count of exhausted-retry failures: the inner final-retry branch
and the outer except both recorded a processing error. Consolidate to the
outer handler (single call site); inner branch keeps only the Qdrant-upsert
error metric. Regression test added.
- Deletes are no longer counted as indexing events: the delete success path
drops doc_type so astrolabe_documents_indexed_total is not inflated.
Regression test added.
- Reuse the already-resolved `settings` in _index_document instead of a second
get_settings() call.
- Use explicit `> 0` guards in record_document_parse / record_embedding instead
of truthiness checks.
SonarCloud:
- S1244 (BUG): replace float `==` equality in metric tests with pytest.approx.
- S5332 (hotspot): use https in the gateway-URL test fixture.
- S1192: extract the repeated "vector_sync.chunk_count" span-attribute literal
into a module constant.
Review nit: move the duplicated `_sample` test helper into a shared
`metric_sample` fixture in tests/unit/conftest.py.
Refs Deck #175, PR #831.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make per-tier bottlenecks in the document-processing pipeline
(scan -> fetch -> parse -> chunk -> embed -> Qdrant upsert) visible via
metrics, traces, and structured logs. Today the document_processors layer
emits only a logger.info line: no metric, no span, and page counts live only
inside a log string. The single processing-duration histogram is unlabeled and
whole-document, so it cannot isolate parse vs embed vs upsert.
New astrolabe_* metric family (distinct from the mcp_* protocol metrics):
- astrolabe_document_parse_{duration_seconds,total} + pages/chars/bytes counters
recorded at the ProcessorRegistry.process() boundary (covers all current and
future processors uniformly)
- astrolabe_document_escalation_total (dormant; tiered-pipeline readiness)
- astrolabe_embedding_{duration_seconds,requests_total,chunks_total,chars_total}
- astrolabe_document_chunks_total, astrolabe_documents_indexed_total{source,status}
Tracing: new document_processor.parse child span + enriched embed/chunk span
attributes (provider/model/batch_size/chunk_count). Structured logs gain a
consistent field vocabulary (doc_id, doc_type, processor, tier, pages, chars,
byte_size, chunks, duration_ms, status) so Loki can aggregate without regex.
Tier-readiness: processor/tier are labels from day one and a tier property is
added to DocumentProcessor, so adding docling/OCR/LLM tiers later is additive
(new label values, never new metrics). Tenant comes from the kube namespace
label; mime_type/model are span attributes only (cardinality). Existing
mcp_vector_sync_*/mcp_qdrant_* are left untouched.
Refs Deck #175 (superset of #173 Phase 2). Dashboard/recording-rules follow-up
tracked on #175 for homelab-argocd.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Modernize the new models (DeckCardSummary, DeckCommentSummary,
StackOverview, BoardOverviewResponse + the loosened unions) to PEP 604
syntax (list[...] / X | None), per CLAUDE.md.
- Make status="done" exclude archived cards so open/done/archived partition
the board with no overlap (a done+archived card is reported only as
"archived"); document the semantics in docstrings and docs/deck.md, add a
partition unit test.
- deck_get_archived_stacks: pass through label/assigned_to filters (status
stays archived-only by definition); note the limitation in the docstring.
- Rename _validate_description_max_length → _validate_positive_length (now a
generic positive-length guard).
- Soften deck_get_board_overview docstring: it views board state and omits
the ACL/user/label-management fields deck_get_board exposes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EMBEDDING_GATEWAY_URL is configured as a bare origin (scheme://host:port) —
the deployment's Service URL. GatewayProvider now appends the gateway's /v1
base path before handing the URL to the OpenAI SDK, so both embed posts
({base}/embeddings) and dimension discovery ({base}/models) land under /v1.
Idempotent: a URL already ending in /v1 is left unchanged.
This lets EMBEDDING_GATEWAY_URL stay a bare domain (matching the gitops
Service URLs) instead of requiring a hand-appended /v1.
Also align the `embedding_gateway_model` field default with _DEFAULTS
("mistral/mistral-embed"). The gateway catalog is provider-namespaced, and
_detect_dimension matches `entry.id == embedding_model`; the stale
un-namespaced default would silently miss the catalog entry and leave the
dimension unresolved (re-triggering the external-mode startup crash).
Tests: bare / trailing-slash / idempotent normalization + a bare-origin
discovery test asserting /v1/models. 16 gateway-provider tests pass;
providers + vector suites green (129 total); ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deck read tools returned too many tokens to be usable as boards grow — even
deck_get_stacks(description_max_length=1) exceeded the MCP token limit because
every card was fully serialized in list views.
- Add compact projection models (DeckCardSummary, DeckCommentSummary,
StackOverview, BoardOverviewResponse) and a uniform detail="summary"|"full"
knob (summary default) on deck_get_cards / get_stacks / get_stack /
get_archived_stacks.
- Add pre-serialization filtering: status (open/done/archived/all), label,
assigned_to.
- Add deck_get_board_overview: board title + label legend + stacks with
compact card rows + counts in a single call.
- Compact comments: detail / message_max_length / newest-first order on
deck_get_card_comments.
- Docs + unit/integration tests.
BREAKING CHANGE: deck list tools now default to detail="summary" and
status="open". The include_archived_cards parameter is replaced by status
(use status="all" to include archived cards); pass detail="full" to restore
the previous per-card shape.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
