Lowering VECTOR_SYNC_SCAN_INTERVAL to 5s (previous commit) fixed user
discovery on nc31 but exposed re-scan churn on the slower nc32 runner: each
scan re-queues the user's entire corpus, so a 5s cadence floods the single
processor worker faster than it drains (pending climbed to 20-30+ while
indexed stayed 0, status "syncing").
Discovery latency and re-scan churn are separate knobs. Keep
USER_POLL_INTERVAL short (5s) for prompt discovery — the per-user scanner
runs its initial scan immediately on start, so the corpus is queued once
right away — but restore a moderate SCAN_INTERVAL (30s) so re-scans don't
re-flood the queue. Indexing of the one-time initial scan completes well
inside the test's 90s budget; a note created just after that scan is still
picked up by the 30s re-scan.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After restoring the background-sync UI ids, the two indexing-dependent
multi-user-basic tests (chunk_context_uses_app_password, plotly_with_basic_auth)
surfaced a second, previously-masked failure: they provision a user, create a
note, then wait ~90s for it to be indexed.
The mcp-multi-user-basic service ran with the production cadence — scan
interval 60s and the default user-poll interval 60s. A freshly-provisioned
user isn't even *discovered* by the background-sync user manager for up to
60s, leaving too little of the 90s budget for the scan + single-worker
indexing to finish (observed: pending docs still "syncing" at timeout, or the
scanner not yet started → "idle" with 0 indexed).
Match the single-user service's short cadence (5s) and add a matching 5s
user-poll interval so discovery + scan + index complete well within the test
budget. Test-only config; production deployments set their own intervals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Astrolabe PHP→Vue settings refactor dropped three stable element ids
(#mcp-enable-background-button, #mcp-revoke-background-button,
#mcp-revoke-background-form) that the multi-user-basic integration suite
drives the background-sync enable/disable/revoke flows through. Their
absence timed out the 5s Playwright locators and failed four tests:
- test_astrolabe_multi_user_background_sync::test_multi_user_astrolabe_background_sync_enablement
- test_astrolabe_multi_user_background_sync::test_revoke_background_sync_access
- test_astrolabe_chunk_context::test_chunk_context_endpoint_uses_app_password
- test_astrolabe_plotly_visualization::test_astrolabe_plotly_visualization_with_basic_auth
(the latter two enable background sync via complete_astrolabe_authorization
before exercising the app-password / indexed-search paths).
Two-part fix:
1. Bump the astrolabe submodule to v0.20.1 (cbcoutinho/astrolabe#116),
which restores the three element ids on the refactored NcButtons.
2. Defense-in-depth in the test helpers: resolve the enable/revoke buttons
by their stable id first, falling back to the button's accessible name
so a future id rename degrades to a slower-but-working lookup instead of
a hard timeout. Avoids a combined `.or_()` locator, which would
strict-mode-violate (the id button also matches by text).
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>
LocalTransport.aclose() (added in round 3) closes its owned stream ends; the ADR
still described aclose() as a no-op for the memory stream. Update the prose to
match the shipped behaviour. Doc-only.
Refs: Deck #196
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>
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>
- 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>
- 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>
Add a comment at the deletion-tracking scroll noting that a user who
gained access to a shared file via the tenant-wide dedup path (without
indexing it) is absent from the user_id-filtered indexed_file_ids, so the
grace-period sweep never enqueues a delete for them — their stale
acl_principals entry is reclaimed lazily by verify-on-read eviction.
Addresses review nit #3.
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>
existing_principals() ran for every doc type when seeding acl_principals.
note/news_item/deck_card IDs are per-user (not globally unique) and chunk
point IDs are user-agnostic, so on an ID collision the merge would pull in
another user's principal and cross-surface their content via the
acl_principals search branch. It was also N wasted tenant-wide scrolls on
initial sync for those types. Gate the prior-principal merge on
doc_type == "file" (the only type with cross-user dedup + globally-unique
fileid); other types seed with the indexer only.
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>
