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>
🟡 The `worker` command never called initialize_document_processors(), so a
worker pod with ENABLE_UNSTRUCTURED/TESSERACT/CUSTOM configured silently ran
PyMuPDF-only (only the import-time-registered processor). The always-on API pod
registers them in its lifespan; the worker has its own startup path, so call
initialize_document_processors() there too (before run_worker_async).
🟢 Drop the unused get_database_url monkeypatch in the Postgres integration
fixture (build_app_for_url passes the URL explicitly; only the ssl lookup needs
pinning).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 review (non-blocking) items:
- get_procrastinate_conninfo: warn on an empty connect_timeout= value (it falls
back to the 10s default); preserve an explicit connect_timeout=0.
- Document the _doc_queueing_lock user_id invariant (NC rejects ':' in usernames).
- docs/configuration.md: note that `db downgrade` leaves procrastinate's tables
in place and how to drop them on a full teardown.
- reclaim_stalled_ingest_jobs: debug heartbeat log when nothing is stalled.
- Drop the redundant list() wrap in the integration stalled-jobs assertion.
Logging pattern: define a module-level `logger = logging.getLogger(__name__)`
and use it instead of function-local or inline getLogger(__name__) calls
(config.py, config_validators.py, tests/.../test_scope_authorization.py). The
test file's dev-only `scripts.*` import gets a ty: ignore since it resolves via
sys.path at runtime, not as an installed package.
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>
- visualization.py: drop the CSV string-split branch. The Astrolabe PHP
client sends path_prefixes as a JSON array, so only a list is accepted;
any other shape is ignored rather than comma-split (which would corrupt
folder names containing commas).
- viz_routes.py: split the path_prefixes query param on newline (a comma
is a valid POSIX path char; a newline is not) and pass None instead of
[""] when the param is absent.
- access_filter.py: widen build_base_filter_conditions' path_prefixes to
Iterable[str] for consistency with normalize_path_prefixes.
- ADR-027: document the newline delimiter (frontend/viz route) and JSON
array (PHP->MCP body), and the PHP-side cap on list width.
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>
CI runs `uv run --frozen ty check -- nextcloud_mcp_server` and `uv run pytest -m
unit`, which install the default + dev groups but not the `[postgres]` optional
extra. vector/queue/procrastinate.py imports procrastinate at module scope (the
task registration needs App/Blueprint), so without it installed ty fails on
unresolved imports and the procrastinate unit tests fail to collect.
Add procrastinate + psycopg to the dev group (kept in the [postgres] extra for
production opt-in) so dev/CI always type-check and test against them, while
SQLite/personal installs stay free of the Postgres deps. Matches the repo's
optional-DB-driver philosophy.
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>
Prevent concurrent version bumps and spurious releases in the release
pipeline:
- Add a workflow-level concurrency group (cancel-in-progress: false) so
only one bump-version run executes at a time. Concurrent runs have
previously raced to bump the version and push tags, causing release
failures. Subsequent pushes now queue instead of cancelling an
in-flight bump/release.
- Make commitizen the single source of truth for whether a release is
warranted. The previous grep heuristic counted commits matching
feat|fix|docs|refactor|perf|test|build|ci|chore, but commitizen only
bumps for feat/fix/breaking changes. A CI- or docs-only push therefore
set bumped=true and fired release+docker against the old, already
released tag. Now we compare the latest tag before/after running
bump-mcp.sh and only set bumped=true (and emit the new tag) when it
actually changes, so release/docker exit early on non-release pushes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remaining items from the PR #831 Claude review:
- processor span symmetry: add "vector_sync.total_chars" to the sparse
embedding span (already on the dense span) and drop the redundant
"embedding.batch_size" attribute from both spans — it always equalled
vector_sync.chunk_count and would mislead once batching is split.
- metrics: document the deliberate "throughput counts only on full success"
contract in record_document_parse (partial extractions flagged
success=False are counted as a parse-error but never inflate
pages/chars/bytes throughput).
- config: extract _detect_base_provider() -> (family, model) as the single
source of truth for the provider-detection priority chain, shared by
get_embedding_model_name() and get_embedding_provider_family(). Preserves
the intentional gateway asymmetry (only the family method short-circuits).
- base.py: Optional[...] -> PEP 604 `... | None`; drop now-unused import.
Behavior unchanged (get_embedding_* outputs covered by test_config.py).
Refs Deck #175, PR #831.
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>
