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>
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>
Follow-up to PR #814 review.
NatsStatusSubscriber.run() called task_status.started() *after* the fallible
pull_subscribe, so a NATS broker that wasn't ready when the MCP server started
would crash the lifespan instead of retrying. Bus status is a non-critical
observability path, so:
- signal started() before the first subscribe (semantics: "loop is running",
not "subscription succeeded");
- retry a failed subscribe with backoff instead of propagating;
- on a real fetch error (not an idle timeout) drop the subscription and
re-subscribe rather than fetching against a possibly-dead handle.
Also anchor the _content_hash etag-threading TODO to the PR #814 review thread
so it is discoverable outside git blame.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarCloud's python:S7632 parses the literal ``# NOSONAR`` token wherever it
appears — including inside explanatory comments that *quote* the directive —
and treats the following text as a malformed suppression. The actual bare
``# NOSONAR`` suppression lines are fine; the flagged lines were the prose
comments describing them. Reword those comments to drop the inner ``#`` so the
analyzer no longer sees a directive.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Nextcloud authenticates app passwords against the *loginName*, which differs
from the UID for OIDC-provisioned users (e.g. user_oidc makes the UID the
display name: UID "Ada Lovelace", loginName "ada@example.com"). The runtime
consumers of stored app passwords bound the UID as the BasicAuth username, so
every Notes/Files/Shares/CalDAV call returned HTTP 401.
PR #818 fixed only the provisioning endpoint; the consuming paths were missed.
Observed on a login_flow tenant (NC's own OIDC app as IdP): the background-sync
scan loop never started ("Credential validation failed ... HTTP 401") and
semantic search returned 0 results because the ACL shared_with_me lookup 401'd
and degraded to a self-only owner filter.
Root cause: NextcloudClient / CalendarClient conflated two identities — the
DAV/URL path identity (the user_id the whole system keys on = NC UID) and the
auth-credential username (the loginName). Decouple them:
- Thread a keyword-only auth_username through NextcloudClient -> CalendarClient
(defaults to username, so single-user / OAuth where UID == loginName is
unchanged).
- get_user_client_basic_auth (background sync + the /api/v1/vector-viz/search
endpoint) authenticates as the stored loginName, UID for paths.
- _get_client_from_login_flow (the get_client(ctx) MCP-tool path) does the same.
- cleanup_invalid_app_passwords validates with the loginName, so it no longer
401s and wrongly deletes a valid OIDC user's password.
The loginName is already persisted in app_passwords.username and returned by
get_app_password_with_scopes. Adds unit tests covering the UID != loginName
split for both client builders, the calendar credential/path split, and the
cleanup validation. Also genericises the example user in the #818 comment/test
(real name/email -> Ada Lovelace / ada@example.com).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new decomposition modules used `# NOSONAR: reason` (colon form), which
SonarCloud flags as a malformed suppression comment (python:S7632) and which
fails to suppress the intended issue. Switch to the repo's bare `# NOSONAR`
convention with the rationale in a comment above, matching config.py and
auth/storage.py. This also lets the suppression silence python:S7503 (async
method without await) on the protocol-required no-op aclose stubs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- gateway_client: guard token cache with a lazy anyio.Lock so concurrent
embed calls share one M2M token request instead of racing
- status subscriber: distinguish idle fetch timeouts from real broker
errors (log + 5s backoff) instead of swallowing all and spinning
- nats: warn when the bus URL uses unencrypted transport (non-tls://)
- collection_metadata: accept an optional shared httpx client, make TLS
verify explicit, document the unauthenticated control-plane contract
- replace python -O-stripped asserts with explicit ValueError in the bus
status builder and the api metadata source
- document why the nil-UUID sentinel point can't collide with content ids
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Notes scan in scan_user_documents ran inline without a try/except, while
files/news/deck each had their own guard. On instances without the Notes app
installed, notes.get_all_notes() raises HTTPStatusError 404, which propagated
out of scan_user_documents and aborted the entire per-user vector sync before
files/news/deck were ever reached -- yielding "0 documents indexed" and, after
5 consecutive errors, stopping the scanner.
Extract the Notes scan into scan_notes() (mirroring scan_news_items /
scan_deck_cards) and wrap the call in a per-app try/except. A 404 (app not
installed/disabled) is now logged at info and skipped; other apps still scan.
Deletion-tracking runs only after a successful Notes fetch, so a failed fetch
can never mass-delete a user's indexed notes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarCloud:
- Resolve 6 S5332 hotspots (http→https in test fixture URLs).
- S6418: hoist the unauthenticated AsyncOpenAI placeholder to a named constant
+ NOSONAR (genuine non-secret; gateway ignores it when unauthenticated).
- Fix two reliability bugs: None-index guard in the gateway token-cache test
(S2259) and float `> 0.0` instead of `!= 0.0` in the sentinel test (S1244).
- status.py idle path sleeps 0.1s instead of sleep(0) (S7491); NOSONAR on the
protocol-required async no-await aclose() stubs (S7503).
Claude review:
- Remove three leftover debug print() calls in app.py (logger.info already
covers them).
- payload_backfill: drop parsed_at from the backfilled-keys docstring (it is
per-document state, not a deployment scalar); add a clean 404 precondition
for BasicAuth deployments without an OAuth token verifier.
- status.py: task_status typed TaskStatus | None (drop type: ignore).
- nats.py: TODO to thread etags for file/deck/news; note etag default → None.
- factory: warn on unknown INGEST_BUS_URL scheme; raise ValueError instead of
assert for the external-mode preconditions.
- docs/configuration.md: document the decomposition hook-point env vars + that
nats-py ships core (lazy-imported) and external+bus uses two NATS connections.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
get_user_client_basic_auth built a fresh RefreshTokenStorage and ran
storage.initialize() — a full Alembic `upgrade` in a worker thread — on every
call. Once the /api/v1 search endpoints (unified_search, vector_search) and the
chunk-context endpoint were wired to use it, concurrent requests ran concurrent
Alembic upgrades, which race on Alembic's non-thread-safe module-global
EnvironmentContext proxy and intermittently raise `KeyError: 'script'` →
HTTP 500 (seen on multi-user-basic/nc31; nc32 got lucky).
Cache one process-wide, already-initialized storage instance behind a lazily
created anyio.Lock so the one-time migration runs exactly once and never races.
Callers passing an explicit `storage` are unaffected. Also removes a redundant
per-request migration from the search hot path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- vector/qdrant_client.py: add owner_id to _PAYLOAD_INDEX_FIELDS (BLOCKING).
Every search applies MatchAny(key="owner_id", ...); without a keyword index
Qdrant full-scans the collection and may 400 on Qdrant Cloud strict mode.
_ensure_payload_indexes is idempotent so existing collections migrate at
startup.
- search/access_filter.py: bound the process-global _owners_cache with an LRU
cap (was one unbounded entry per active user, never evicted); document the
owner-level over-fetch limitation (a prolific sharer floods the recall
buffer with ghost candidates that verify-on-read drops, with no second
Qdrant pass) as a TODO toward per-file filtering.
- search/algorithms.py + semantic.py + bm25_hybrid.py: promote
accessible_owners from **kwargs to an explicit keyword-only parameter on the
SearchAlgorithm ABC and both implementations, so a misspelled keyword is a
type error rather than a silent fall back to self-only scope.
- search/verification.py: document that _verify_files now verifies by global
file id (WebDAV SEARCH), not by path.
- tests/unit/search/test_access_filter.py: add cache-hit, TTL-expiry,
failure-not-cached, and LRU-bound tests.
Bumps the astrolabe submodule with the matching #89 review fixes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the seven §10.2 hook-point modules + five env vars so Astrolabe Cloud can
offload document processing to the external document-processor / embedding
gateway. Purely additive: with every setting unset the server behaves exactly
as today, so self-hosters are unaffected (Deck #92).
Hook points (all default to current monolith behavior):
- config: EMBEDDING_PROVIDER, INGEST_MODE, STATUS_BACKEND,
COLLECTION_METADATA_SOURCE, FACT_EVENT_EMITTER (+ supporting settings),
validated in Settings.__post_init__ (fail-fast STATUS_BACKEND=local with
INGEST_MODE=external); shared canonical.py.
- vector/payload_keys.py + acl_hash.py: cross-impl NAMESPACE/point_id (§2.2)
and BLAKE2b-128 ACL hash (§11), pinned by fixtures shared with the
document-processor repo.
- embedding/gateway_client.py: OpenAI-compatible GatewayProvider authenticating
via M2M OIDC client-credentials (separate realm); manual-only registry entry.
- vector/collection_metadata.py: sentinel-point / API metadata source with env
fallback.
- vector/queue/: hexagonal ingest producer ports + memory/NATS adapters
(Postgres seam); INGEST_MODE=external publishes mcp.ingest.requested.{tenant}
instead of the in-memory stream and skips the in-process processor pool. The
lifespan becomes a composition root across both deployment branches.
- vector/queue/status.py: STATUS_BACKEND=bus subscriber feeding a StatusStore
the vector-sync status endpoint reads.
- admin/payload_backfill.py: POST /api/v1/admin/payload-backfill (admin scope);
processor writes the new payload keys; query-side ACL pre-filter gated behind
ACL_PREFILTER_ENABLED (default off).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector index has always been strictly per-user: every Qdrant payload
carries a `user_id` and the search filter is `user_id == querying_user`.
A file Alice indexed cannot be discovered by Bob even if she has shared
it with him — Bob would have to re-index it under his own user_id to
make it searchable, which means duplicate index entries for every share
recipient.
Switch to ownership-with-ACL-expansion:
- New `nextcloud_mcp_server.search.access_filter` module:
- `list_accessible_owners(sharing_client, user_id)` calls the OCS
Sharing API (`shared_with_me=true`) and returns
`{user_id} ∪ {uid_owner of each share}`. Fails open to `[user_id]`
so a misbehaving Sharing API doesn't black-hole search.
- `build_ownership_filter(user_id, accessible_owners)` returns a
Qdrant `Filter` whose `should` branch matches either the new
`owner_id IN accessible_owners` field or the legacy `user_id` field.
The legacy branch keeps points indexed before this change reachable
without a migration backfill.
- Indexer payload (`vector/processor.py`) now writes `owner_id` alongside
`user_id`. `DocumentTask` gains an optional `owner_id` field; today the
scanner always runs as the owner so the processor falls back to
`user_id`, but the field is plumbed so a future shared-with-me crawler
can set the true owner without reshaping the payload contract.
- `SemanticSearchAlgorithm.search` and `BM25HybridSearchAlgorithm.search`
accept `accessible_owners` via kwargs and use the new ownership filter.
Default behaviour with no kwarg is unchanged (self-only).
- Both user-facing callers — the MCP tool path (`server/semantic.py`) and
the visualization Starlette route (`auth/viz_routes.py`) — compute
`accessible_owners` from the authenticated Nextcloud client before
invoking the search algorithm. Eviction, scanner deletion, placeholder,
and chunk-context paths intentionally keep the legacy `user_id`
semantics (those are "operations on a specific user's records", not
cross-user reads).
- 10 new unit tests in `tests/unit/search/test_access_filter.py` cover
self-only default, owner expansion, dedup, fallback fields, OCS
failure, and the legacy `should`-branch shape.
Pairs with cbcoutinho/astrolabe#89 — together they let an Astrolabe user
find content owners have shared with them without going through any
re-authorization flow or re-indexing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The startup sweep was reading settings.qdrant_collection (the raw config
value, default "nextcloud_content") instead of settings.get_collection_name(),
which is what every other vector-sync operation uses. When QDRANT_COLLECTION
is not overridden, get_collection_name() auto-generates a
{deployment-id}-{model-name} name; the sweep was targeting a non-existent
collection and silently returning (0, 0).
Also adds the AsyncQdrantClient type annotation that was missing on
sweep_orphan_placeholders, and renames its parameter from collection_name
to collection to make it clear the value must be the resolved name.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the per-tenant nextcloud-mcp-server Pod OOMKills mid-batch, the
in-memory anyio processor queue is lost but the placeholder Qdrant
points (is_placeholder=true, status=pending) survive. The next Pod's
scanner re-runs, sees the existing placeholders, applies the
5 × VECTOR_SYNC_SCAN_INTERVAL staleness gate (~5h with the deployed
1h scan interval), and skips them. Result: 0 documents indexed for
the duration of the gate after every restart.
Stamps a process-level instance_id (UUID per Pod-process) onto every
placeholder write. A new sweep_orphan_placeholders helper, called
once from starlette_lifespan after the Qdrant client is initialised
and before the scanner / user-manager spawns, scrolls the collection
and deletes any placeholder whose instance_id doesn't match the
current Pod's (including placeholders with no instance_id field —
back-compat for pre-fix Pod versions). The scanner's next cycle
naturally re-creates fresh placeholders and queues work normally;
no DocumentTask reconstruction needed.
Sweep is one-shot at startup, not periodic — the existing staleness
gate still covers same-Pod recovery, and the cross-Pod-restart gap
was the only failure mode. Failure is non-fatal (logged via
vector_sync.orphan_sweep_failed) so a transient Qdrant hiccup at
boot doesn't prevent the scanner from running.
Both lifespan branches (single-user BasicAuth, OAuth / multi-user
BasicAuth) call the sweep via a module-local helper. A new
VECTOR_SYNC_ORPHAN_SWEEP_ENABLED setting (default True) provides
an escape hatch.
Closes Deck #101.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nextcloud Deck PR #7910 added IWebhookCompatibleEvent to CardCreated/
Updated/DeletedEvent and BoardUpdatedEvent, so Deck can finally emit
real-time webhooks via core's webhook_listeners app. Wire this into
the existing preset → parser → DocumentTask pipeline that already
backs Notes / Calendar / Tables / Forms / Files sync.
- Add deck_sync preset (app=deck, 4 events) and drop the stale
"Deck does not support webhooks" comment.
- Teach webhook_parser to convert Deck card events into
DocumentTask(doc_type=deck_card, operation=index|delete) with
stack_id metadata. BoardUpdatedEvent logs delivery at INFO and
returns None — the polling scanner reconciles affected cards.
- Cover three new unit tests for the deck create/delete/board-update
paths plus symmetric fail-open tests for missing card.id /
node.id in _parse_deck_event and _parse_file_event.
The astrolabe admin UI auto-discovers the new preset via
filter_presets_by_installed_apps(); no astrolabe-side wiring is
required for it to appear in the Webhook Management card grid.
Note: requires Deck ≥1.18.x (where PR #7910 lands); the preset is
hidden when the Deck app isn't installed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ``BM25SparseEmbeddingProvider.__init__`` calls
``fastembed.SparseTextEmbedding(model_name="Qdrant/bm25")`` which
downloads ~50 MB of model weights from HuggingFace and loads them
into memory — observed >5 s wall-clock in production. The inference
methods (``encode_async``, ``encode_batch_async``) already wrap work
in ``anyio.to_thread.run_sync``, so the design intent is clearly to
keep FastEmbed off the event loop. That protection just didn't
cover the constructor.
Symptom in the Astrolabe Cloud per-tenant deploy (deck #102 smoke):
~30–90 s after a user enables semantic search, the pod tips into a
SIGKILL-restart cycle. Loki shows a single log line
Initializing BM25 sparse embedding provider: Qdrant/bm25
followed by nothing else from the event loop until exitCode 137.
Kubernetes ``/health/live`` httpGet probe timeout=5s fires 6 times
in a row, kubelet kills the container, restart, repeat.
Fix: switch ``get_bm25_service()`` to an async accessor that wraps
the first-time construction in ``anyio.to_thread.run_sync``. Two
existing call sites (``vector/processor.py:603``,
``search/bm25_hybrid.py:123``) update to ``await``. Both are
already inside async functions so the await is free.
New unit test pins the invariant by monkey-patching
``BM25SparseEmbeddingProvider.__init__`` with ``time.sleep(1)`` and
asserting a concurrent ``anyio.sleep(0.05)`` finishes promptly —
the test fails if the constructor ever runs back on the event loop.
Same pattern exists in ``OllamaEmbeddingProvider.__init__`` (sync
``httpx.get`` health-check). Ollama isn't enabled in any current
deploy; filed as a follow-up.
Refs:
- Astrolabe Cloud deck card #102 (smoke discovery)
- Sibling fix#799 (NullPool for cross-loop-asyncpg, same class
of "anyio bites you in production" bug)
Verified:
- ``uv run pytest tests/unit/`` — 1027 passed
- ``uv run ruff check`` clean on touched files
- ``uv run ty check`` clean on touched files
- New tests pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to #787/#789 (ADR-022 cleanup). After
`oauth_enabled ↔ enable_login_flow` became an invariant, the
`use_basic_auth=False` branch in `vector/oauth_sync.py` — and the
parameter wiring that fed it — was no longer reachable from any
supported deployment mode. This commit removes the dead code.
- nextcloud_mcp_server/vector/oauth_sync.py:
- Deleted `get_user_client_oauth` (the OAuth-token refresh helper) and
its `VECTOR_SYNC_SCOPES` constant.
- Deleted the `get_user_client` dispatcher. Internal callers now call
`get_user_client_basic_auth` directly.
- Dropped the `use_basic_auth: bool` parameter from `user_scanner_task`,
`multi_user_processor_task`, `_run_user_scanner_with_scope`, and
`user_manager_task`.
- Dropped the `token_broker` parameter from the same four functions —
they no longer need it now that the OAuth-refresh path is gone. The
`TokenBrokerService` constructed in `app.py` is still used by the
management API revoke endpoint, just not by background sync.
- Simplified the user-list query in `user_manager_task` to always read
from the `app_passwords` table.
- Replaced all `mode_label = "BasicAuth" if use_basic_auth else "OAuth"`
with a literal `[BasicAuth]` log prefix (keeps existing log filters
working).
- Updated the module docstring to describe the post-cleanup shape.
- Dropped the now-unused `TYPE_CHECKING` import of `TokenBrokerService`.
- nextcloud_mcp_server/app.py: dropped the `use_basic_auth = True` block
and the now-stale `token_broker if not use_basic_auth else None` /
`use_basic_auth` positional args from the two `tg.start(...)` calls in
the multi-user vector-sync lifespan. Token broker construction stays —
still consumed by the management API revoke endpoint via
`app.state.oauth_context["token_broker"]`.
- tests/integration/test_app_password_provisioning.py: deleted four tests
that exercised the now-removed OAuth-refresh path
(`test_oauth_mode_uses_refresh_token_only`,
`test_oauth_mode_raises_error_without_token`,
`test_get_user_client_oauth_function`,
`test_oauth_mode_requires_token_broker`) plus the
`test_get_user_client_dispatches_to_basic_auth` test for the deleted
dispatcher. Updated the module docstring + imports accordingly. The
BasicAuth-mode tests (`test_basic_auth_mode_uses_local_storage`,
`test_multiple_users_basic_auth_mode`, etc.) all remain.
No runtime-behaviour change in any supported deployment mode — the deleted
branches were already unreachable post-PR #787. 3 files changed,
+59 / -301; 1010 unit tests pass; integration jobs for
`mcp-login-flow` and `mcp-multi-user-basic` are the critical regression
gates before merge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same pattern as the ENABLE_LOGIN_FLOW removal in the previous commit:
the deployment mode (MCP_DEPLOYMENT_MODE) is the single source of truth
for selecting an auth flow. The ENABLE_MULTI_USER_BASIC_AUTH env-var
alias is redundant with `MCP_DEPLOYMENT_MODE=multi_user_basic`.
Unlike the ENABLE_LOGIN_FLOW removal — where silent removal was safe
because Login Flow v2 is the auto-detection default — silent removal
here would be a surprise: a user with only ENABLE_MULTI_USER_BASIC_AUTH=true
in their .env would auto-detect into LOGIN_FLOW after upgrade (wrong
runtime mode). Mitigation: detect_auth_mode now reads os.environ
directly for both legacy aliases and raises ValueError with a one-line
migration message if either is set. Applied retroactively to
ENABLE_LOGIN_FLOW as well — loud is better than silent.
- nextcloud_mcp_server/config.py:
- Drop the dynaconf env-var alias entry for ENABLE_MULTI_USER_BASIC_AUTH.
- Update the `enable_multi_user_basic_auth` field docstring to mark it
as derived / not user-settable.
- `_is_multi_user_mode()` (early-config helper, runs before Settings
is built) switched to checking MCP_DEPLOYMENT_MODE directly. Now
consistent with the canonical detection in detect_auth_mode.
- nextcloud_mcp_server/config_validators.py:
- Drop the auto-detection branch (`if settings.enable_multi_user_basic_auth`).
Selection of MULTI_USER_BASIC is now exclusively via the explicit
MCP_DEPLOYMENT_MODE branch.
- Add `enable_multi_user_basic_auth` to `_sync_derived_flags` alongside
`enable_login_flow` — both flags are now derived from the resolved mode.
- Drop `enable_multi_user_basic_auth` from
`MODE_REQUIREMENTS[MULTI_USER_BASIC].required` and from the
`forbidden` lists of SINGLE_USER_BASIC and LOGIN_FLOW (no longer
user input → no meaningful forbidden check).
- Add loud-deprecation `ValueError` block at the top of detect_auth_mode
that errors with a clear migration message when ENABLE_MULTI_USER_BASIC_AUTH
or ENABLE_LOGIN_FLOW is found in os.environ.
- tests/unit/test_config_validators.py:
- Switch ~10 fixtures from `enable_multi_user_basic_auth=True` to
`deployment_mode="multi_user_basic"` (mirrors `enable_login_flow`
treatment from the previous commit).
- Switch two `patch.dict(os.environ, {"ENABLE_MULTI_USER_BASIC_AUTH": "true"})`
blocks to use MCP_DEPLOYMENT_MODE.
- Rename `test_forbidden_multi_user_basic_auth` to
`test_forbidden_multi_user_basic_when_credentials_present` — the
scenario is now an explicit-mode + credentials conflict, not an
env-var-flag conflict.
- Add `test_legacy_enable_multi_user_basic_auth_env_var_errors` and
`test_legacy_enable_login_flow_env_var_errors` to exercise the new
loud-deprecation ValueError path.
- docker-compose.yml: mcp-multi-user-basic profile switched to
`MCP_DEPLOYMENT_MODE=multi_user_basic`.
- env.sample: replaced `#ENABLE_MULTI_USER_BASIC_AUTH=true` example with
`#MCP_DEPLOYMENT_MODE=multi_user_basic`.
- docs/authentication.md, configuration.md, troubleshooting.md,
auth-flows.md, webhook-management-guide.md,
configuration-migration-v2.md, ADR-025: replaced env-var examples
with the canonical MCP_DEPLOYMENT_MODE form.
- docs/ADR-020: marked partly superseded by ADR-022.
- CLAUDE.md: Multi-User BasicAuth section updated to set
MCP_DEPLOYMENT_MODE.
- nextcloud_mcp_server/vector/oauth_sync.py: module docstring updated.
BREAKING CHANGE: ENABLE_MULTI_USER_BASIC_AUTH is no longer read from
the environment, and setting it now raises a startup ValueError with
a migration message. Replace `ENABLE_MULTI_USER_BASIC_AUTH=true` with
`MCP_DEPLOYMENT_MODE=multi_user_basic`. The same loud-deprecation
check is also applied to the recently-removed ENABLE_LOGIN_FLOW —
replace with `MCP_DEPLOYMENT_MODE=login_flow` (or drop both;
`login_flow` is the auto-detect default when no other auth env vars
are set).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 17 reviewer (🟡 Important):
1. docs/configuration.md degraded-migration runbook said `doc_id backfill
failed on …` but the actual log line in qdrant_client.py:415 is
`doc_id backfill scroll failed on …`. Operators grepping the runbook
string would have missed it. Insert the `scroll` qualifier.
2. _create_one_payload_index returned True on the 400 schema-conflict
path, so a wrong-type index discovered at create time skipped the
consolidated `Payload index creation incomplete` summary — but a
wrong-type index discovered via the existing-schema check at line
195-206 did fire it. Tenants whose payload_schema is hidden from
their JWT (Qdrant Cloud collection-scoped tokens) only ever observe
the create-time path, so they never saw the operator-level summary.
Return False so the summary fires in both cases.
3. docs/configuration.md said the upgrade-time delay was `proportional to
point count while writes are issued` — overstating the cost. Writes
are proportional to int-typed points only; the scroll itself is
proportional to total point count. Reword.
Local-mode collection-creation regression (root-cause of failing
single-user / login-flow / multi-user-basic CI jobs):
PR #779 changed the existence probe in get_qdrant_client from
collection_exists() (returned bool in both modes) to get_collection()
+ except UnexpectedResponse(status_code=404). The HTTP-mode client
raises UnexpectedResponse with a 404 body, but the local/in-memory
client raises ValueError(f"Collection {name} not found") — see
qdrant_client/local/async_qdrant_local.py. The narrow except clause
let the ValueError propagate, app.py's lifespan re-raised as
RuntimeError, and the mcp container crashed on first start. Catch
ValueError too, with a `not found` substring guard so genuine
programming bugs (bad collection_name, etc.) still surface.
Tests: extend the existing 400-path test to assert the new
failed_fields contract; add two get_qdrant_client unit tests pinning
the local-mode VE catch (positive case + propagation case).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Detect pre-existing payload indexes with the wrong schema type in
`_ensure_payload_indexes`. The previous "field already in
existing_schema → skip" branch silently survived a collection migrated
from the int-doc_id era where `doc_id` is indexed as INTEGER, letting
`MatchValue(value="123")` searches keep failing with HTTP 400 on Qdrant
Cloud strict mode — exactly the production failure this PR was meant to
fix. New behaviour: compare `existing_schema[field].data_type` against
the declared type; on mismatch log a WARNING and append to
`failed_fields` so the consolidated end-of-function summary picks it up.
No auto-repair (operator intervention only — see docs/configuration.md
recovery procedure). New test exercises the doc_id-INTEGER scenario
end-to-end and asserts both the per-field WARNING and the summary line.
Clarify the `_verify_news_items` malformed-doc_id rationale: the news
API has no per-item endpoint, so a malformed doc_id genuinely cannot be
verified against the source of truth. We err toward false-positive
(keep) over false-negative (drop) — same conservative posture as
`_verify_notes` and `_verify_deck_cards`. The producer-side validation
is the real security boundary; the verifier is defence-in-depth. Both
the inline comment and the WARNING message now spell this out.
Add a TODO in `get_last_indexed_timestamp` flagging the O(N) cost on
every incremental sync tick. The previous single-page `limit=10_000`
silently bounded the scroll; paginating fixed correctness but made the
unbounded cost visible. The follow-up tracker (canonical TODO at
`api/visualization.py`) covers migrating the max-`indexed_at` to a
sentinel point or collection metadata for O(1) lookup.
Consolidate the duplicate non-numeric-doc_type TODOs at
`api/visualization.py:508` and `auth/viz_routes.py:570` into a single
canonical comment in `visualization.py`; `viz_routes.py` is reduced to
a back-reference. Removes the rot risk of "fixed in one place,
forgotten in the other." The canonical comment also references the
O(1) timestamp follow-up in `scanner.py`.
Document the `batch_size = 256` (qdrant_client.py) vs
`_DELETION_TRACKING_PAGE_SIZE = 1024` (scanner.py) split with
cross-referencing comments at each site: the smaller batch is for the
read-write backfill upsert path (Qdrant accepts ~256-point chunks
comfortably); the larger page is for read-only deletion-tracking
scrolls where no per-page write round-trip applies.
Replace `assert qdrant_client is not None` in `scan_user_documents`
with `cast(AsyncQdrantClient, qdrant_client)` plus an explanatory
comment. `assert` is silently elided under `-O`; `cast` is the
conventional zero-cost narrower for branches the type checker can't
infer from the surrounding `if not initial_sync` ternary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to PR #778 — `collection_exists()` is also denied by Qdrant
Cloud on a collection-scoped JWT, so the multi-tenant fix needs to go
one step further: use `get_collection(name)` (the underlying GET
`/collections/{name}` call) and treat a 404 `UnexpectedResponse` as
the "doesn't exist" signal. That endpoint is the only existence-probe
Qdrant permits on a collection-scoped JWT — listing or probing
collection metadata cluster-wide is a tenant-isolation boundary by
design.
Hit during Astrolabe Cloud smoke17 with the post-#778 image:
qdrant_client.http.exceptions.UnexpectedResponse: 403 (Forbidden)
raw response: {"error":"forbidden"}
File "qdrant_client.py", line 84, in get_qdrant_client
collection_present = await _qdrant_client.collection_exists(...)
Folds the existence check into the same `get_collection()` call that
already runs immediately afterward for dimension validation, so the
new path is also one fewer round-trip on the happy path.
Cold-start (collection genuinely missing) behavior is unchanged: 404
→ `collection_info` is None → fall through to `create_collection()`.
Whether `create_collection` succeeds is an orthogonal concern (managed
multi-tenant setups pre-provision collections externally; admin-key
single-tenant setups can create on the fly).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The startup path in `get_qdrant_client()` calls `get_collections()` to
check whether the configured collection already exists. That's a
cluster-wide list operation; in managed multi-tenant Qdrant Cloud
deployments where each tenant's JWT is scoped to a single collection
(by design — `access: [{"collection": "tenant_<id>", "access": "rw"}]`),
the call returns `403 Forbidden` and the FastAPI lifespan crashes:
qdrant_client.http.exceptions.UnexpectedResponse: 403 (Forbidden)
raw response: {"error":"forbidden"}
RuntimeError: Cannot start vector sync - Qdrant initialization failed
Switching to `collection_exists(collection_name)` (per-collection
HEAD-style probe) only requires access to the named collection, which
the tenant JWT has. Single-tenant deployments using an admin/master
key are unaffected — they had access to both forms; this picks the
narrower one.
Doesn't change creation semantics: when the collection isn't present
the code path still calls `create_collection`. In a managed setup
where the collection is pre-provisioned by an external admin (e.g.,
the Astrolabe Cloud control plane's create-tenant workflow), that
branch never fires for an existing tenant; cold-start tenants get
their collection created by the workflow before the Pod boots.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defer publication of `_qdrant_client` until after the in-lock backfill +
payload-index migration awaits complete. The fast-path check at the top of
`get_qdrant_client` reads the singleton without holding the init lock, so
publishing the constructed-but-unmigrated client let concurrent fast-path
callers fire filtered searches before `_ensure_payload_indexes` ran —
producing HTTP 400 ("Index required but not found") on Qdrant Cloud strict
mode. Local `provisional` is now used for every await inside the lock; the
global is assigned exactly once, last.
Replace the five hand-rolled `scroll(..., limit=10000)` calls in
`vector/scanner.py` (notes / files / news / deck-cards deletion tracking,
plus the timestamp scroll) with a single paginated `_scroll_all_points`
helper. The previous single-page cap silently dropped deletion-tracking
points beyond the first 10 k for any user past that threshold. Pagination
follows Qdrant's documented contract (loop until `next_page_offset is
None`) with a fixed per-page `_DELETION_TRACKING_PAGE_SIZE = 1024`.
Extract `_create_one_payload_index` from `_ensure_payload_indexes` to drop
its cognitive complexity below the SonarQube limit (17 → ≤ 15) without
losing the per-field error-containment rationale; every comment is
preserved verbatim on the helper.
Drop the stale `SearchResult.id` `int | str` comment and the redundant
`str(d)` coercion in `_verify_news_items` — the contract has been
str-only since the producer-side stringification landed earlier in this
PR.
Fix eight `doc_id=<int>` test calls in `test_chunk_context_offset_gate.py`
that violated the `doc_id: str` signature of `get_chunk_with_context`,
plus align `_make_result` in `test_verification.py` to coerce `id=str(...)`
matching the production contract — and update 30+ assertions from int
sets (`{1, 2, 3}`) to str sets (`{"1", "2", "3"}`) so the tests now model
the post-PR `SearchResult.id: str` reality end-to-end. Previously these
were masked by the `str(d)` coercion now removed from production.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add chunk_start_offset / chunk_end_offset to _PAYLOAD_INDEX_FIELDS so
the legacy offset-based fallback in search/context.py works on Qdrant
Cloud strict mode (pre-#75 clients have no chunk_index payload).
- Cover chunk_index / chunk_start_offset / chunk_end_offset in the
payload-index summary test; refresh the stale field-list comment.
- Flag the is_valid_nextcloud_doc_id gate at both chunk-context handler
sites with a TODO for future non-numeric doc_types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _group_int_doc_ids: use type(value) is not int instead of isinstance,
since bool is an int subclass and would otherwise stringify to
"True"/"False" and corrupt legacy payloads on backfill.
- Replace doc_id.isdigit() guards in 5 boundary sites
(api/visualization, auth/viz_routes, search/context note/news_item/
deck_card branches) with a shared is_valid_nextcloud_doc_id helper
that rejects "0", leading zeros, and Unicode digit classes
(superscripts, Arabic-Indic, Devanagari) which pass isdigit() but
cannot be valid MySQL AUTO_INCREMENT IDs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- search/context.py: drop the doc_type=='file' guard on skip_offset_lookup
so notes / deck cards / news items also bypass the unindexed offset
fallback when chunk_index is available. Legacy chunk_index=None data
still uses the offset path.
- vector/qdrant_client.py: clarify the backfill/_ensure_payload_indexes
ordering invariant (backfill rewrites payload values only, never schema
or indexes). Acknowledge OSS-vs-Cloud uncertainty in the 400-branch
comment and the new-collection call-site comment.
- vector/scanner.py: hoist qdrant_client to function scope so the
file-scroll block doesn't depend on a name bound inside the
notes-scroll block.
- tests/unit/test_chunk_context_offset_gate.py: flip the note-with-
chunk_index test to assert the offset fallback is skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three coordinated fixes flagged as Important in the round-10 review of
PR #773:
1. Index chunk_index. The chunk-context fast path in
_get_chunk_by_index_from_qdrant and get_chunk_bbox_and_page_from_qdrant
filters on chunk_index, but the field was absent from
_PAYLOAD_INDEX_FIELDS. On Qdrant Cloud strict mode every chunk-context
lookup via chunk_index would 400 and silently fall back to the
document re-fetch path — the exact failure mode the chunk_index
shortcut exists to avoid. Added as INTEGER schema.
2. Catch raw network errors in _ensure_payload_indexes. The
create_payload_index loop only caught UnexpectedResponse, so an
httpx.ConnectError or asyncio.TimeoutError mid-loop would propagate
uncaught — leaving _qdrant_client assigned and silently skipping all
remaining fields. Added a broad Exception catch with the same
per-field containment as the 5xx path: log at ERROR with exc_info,
append to failed_fields, continue. New test covers the path.
3. Lazy-initialise _qdrant_init_lock. Constructing anyio.Lock() at
module import time works for the asyncio backend but anyio's docs
advise instantiating synchronization primitives within an async
context, and pyproject.toml's anyio_mode = "auto" means tests can
run under trio. Moved the construction into get_qdrant_client; safe
under cooperative multitasking because there is no await between the
None-check and the assignment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the four 🟡 important findings from claude-bot review on PR #773:
str (non-Optional) and the guard would silently skip the Qdrant lookup
for an empty string. Removing the guard matches the type signature.
(`all([…, doc_id, …])` rejects None and empty string, plus
`assert doc_id is not None`). No code change needed.
`get_qdrant_client()` with a module-level `anyio.Lock`. Double-checked
locking keeps the steady-state hot path lock-free. Without this,
parallel cold-start callers could all enter the init block and run
`_backfill_doc_id_to_string` + `_ensure_payload_indexes` redundantly
(idempotent, but noisy). Pattern matches `auth/storage.py:2071`.
behavior with three tests covering the float-warning path (the gap
called out in the review), the str/None silent-skip paths, and the
int-grouping happy path.
Verification:
- ruff check / format: clean
- ty check -- nextcloud_mcp_server: clean
- uv run pytest tests/unit/: 969 passed
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Skip and warn instead of stringifying floats / unexpected types in the
backfill helper. A stray doc_id=3.0 would otherwise be rewritten to
"3.0", which producers (str(int)) and the keyword index would never
match, and which int() on the verification side would reject. Also add
a doc_id=0 case to the backfill test to guard against a future
falsy-skip regression.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer findings (1 blocking + 2 important):
- 🔴 Replace `import asyncio` / `await asyncio.sleep(0)` with
`import anyio` / `await anyio.sleep(0)` in the four async side-effect
helpers (_scroll_raises, _upsert_raises, _get_collection_raises,
_create_index). CLAUDE.md mandates anyio for all async operations;
conftest pins the backend to asyncio so the asyncio.sleep call worked
today, but the inconsistency would surface the moment that pin moves.
- 🟡 Replace the sentinel's zero dense vector with a single non-zero
element (`[1e-9] + [0.0] * (dimension - 1)`). Cosine distance is
mathematically undefined for the zero vector and Qdrant Cloud strict
mode rejects zero-vector upserts. The exact value doesn't matter
(sentinel never participates in a search — no user_id/doc_id/doc_type
payload) but the upsert itself must be valid.
- 🟡 Avoid the duplicate `get_collection` round-trip on every restart.
`_ensure_payload_indexes` now accepts an optional
`existing_schema: dict | None` parameter; when None it fetches
collection_info itself (and the get_collection-failure swallow still
applies), but `get_qdrant_client` already fetches collection_info
for dimension validation in the existing-collection branch — pass
`collection_info.payload_schema or {}` through to skip the second
call. The new-collection branch passes `existing_schema={}`
explicitly since a freshly created collection has no payload schema.
The 🟡 deck_card iteration-fallback finding doesn't apply: the
`isdigit()` guard at context.py:612 returns early before either the
fast-path or the iteration fallback runs, so non-numeric doc_ids
cannot reach the inner `c.id == int(doc_id)` comparison.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback (2 items):
- Add a BOOL payload index for `is_placeholder` alongside the three
KEYWORD fields. Strict-mode index-required filtering on Qdrant Cloud
enforces a payload index on any field used in a `FieldCondition`
regardless of value type, so `get_placeholder_filter` and
`delete_placeholder_point` would have produced HTTP 400 on Cloud
instances even after this PR's KEYWORD fix.
Implementation: replace `_KEYWORD_PAYLOAD_FIELDS: tuple` with
`_PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType]` so each
field carries its own schema type. Rename
`_ensure_keyword_payload_indexes` to `_ensure_payload_indexes` since
the function now creates more than just KEYWORD indexes. The
per-field log line now includes the schema type
("Created KEYWORD payload index on 'doc_id'", "Created BOOL payload
index on 'is_placeholder'") so operators can tell which type was
created without checking the source.
- Correct the misleading `wait=True` docstring in
`_apply_backfill_writes`. The previous wording said
`_ensure_payload_indexes` runs "immediately after this function",
but `_apply_backfill_writes` is called in a loop inside
`_backfill_doc_id_to_string` — the index creation runs after the
backfill function *returns*, not after each write. Rewrote the
docstring to capture both load-bearing reasons:
(1) per-batch commit ordering for crash-recovery safety, and
(2) ensuring the keyword index built later covers committed
payloads only.
Adds `test_ensure_payload_indexes_includes_is_placeholder_as_bool`
asserting the schema type is BOOL specifically. Existing tests
updated to use the new dict-based registry (side_effect lists now
extend to all four entries; field-set assertions derive from the
registry instead of hardcoding 3 KEYWORD names).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback (3 important + 3 nits):
- Wrap _ensure_keyword_payload_indexes' get_collection() call in
try/except. The qdrant_client singleton is already assigned by the
time this function runs, so a transient timeout/DNS failure
propagating out left the process holding a usable client with the
migration silently skipped on every subsequent call. Now logs ERROR
with exc_info and returns; next process restart retries.
- Add `and "doc_id" in point.payload` guard to the four set
comprehensions in scanner.py (indexed_doc_ids, indexed_file_ids,
indexed_item_ids, indexed_card_ids). Previously a payload missing
the doc_id key would raise KeyError and crash the entire scan.
- Tighten test_ensure_keyword_payload_indexes_logs_400_as_warning to
match the per-field warning prefix exactly (`startswith("Schema
conflict on payload index")`), so a future change adding 400s to
the partial-failure summary surfaces here as a count mismatch.
- Add new-collection vs existing-collection context to the
_backfill_doc_id_to_string docstring's `dimension` parameter.
- Replace the misleading "rewrote 0/N from int to str" wording when
no rewriting was needed with "N points scanned, none required
rewriting (collection already in str form)".
- Add test_ensure_keyword_payload_indexes_logs_and_returns_when_
get_collection_raises mirroring the scroll-failure test.
SonarCloud (1 CRITICAL + 1 MINOR):
- Refactor _backfill_doc_id_to_string to bring cognitive complexity
under 15 (was 19). Extracted two pure helpers: _group_int_doc_ids
(group point IDs by stringified doc_id) and _apply_backfill_writes
(apply set_payload calls and return rewritten count). The main
function's scroll/loop/sentinel structure is unchanged.
- Add `await asyncio.sleep(0)` to the three async test side_effect
helpers (_scroll_raises, _upsert_raises, _create_index) so they use
an actual async feature (S7503). The async-callable shape is still
required to avoid the AsyncMock unawaited-coroutine warning when
side_effect raises.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses three important findings from the latest reviewer comment:
- Add progress INFO log every 20 scroll batches (≈5120 points at
batch_size=256) in _backfill_doc_id_to_string so a long-running
migration on a large collection (50k+ points) doesn't look like a
startup hang. The line carries collection name, scanned count, and
rewritten count so it doubles as a heartbeat.
- Track non-400 failures in _ensure_keyword_payload_indexes and emit
a WARNING summary line listing every field that failed to get an
index. Per-field ERROR lines are easy to miss in startup noise; the
summary makes the partial-failure state visible at a glance.
- Split the sentinel upsert out of the data-scroll try/except in
_backfill_doc_id_to_string. A scroll-time failure still logs ERROR
with the new "scroll failed" wording (data is incomplete). A
sentinel-write failure now logs WARNING with "data succeeded but
sentinel write failed" wording — data is correct, only the
short-circuit marker is missing, and the next restart re-scrolls
an already-clean collection (idempotent zero-write) before retrying
the upsert.
Also fix the RuntimeWarning emitted by
test_backfill_logs_and_returns_when_scroll_raises: replace the bare
`RuntimeError` side_effect with an async-callable side_effect so
AsyncMock awaits the coroutine before the exception propagates.
Three new unit tests cover the new branches:
test_backfill_emits_progress_log_every_20_batches,
test_backfill_logs_warning_when_sentinel_upsert_fails,
test_ensure_keyword_payload_indexes_summarises_failed_fields.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove three stale `# Use numeric file ID` / `# Pass file path` comments
in scanner.py. file_id is already normalized to str() above each call
site, so the inline comments mislead readers.
- Wrap `_backfill_doc_id_to_string` scroll loop + sentinel upsert in
try/except Exception. The qdrant_client singleton is assigned before
this migration runs, so a transient scroll failure was leaving the
process holding a usable client with int payloads permanently
unbackfilled until the next restart. Catch broadly, log ERROR with
exc_info, and return without writing the sentinel — next process
restart retries from scratch.
- Note `:memory:` mode behavior near the sentinel constants so future
readers don't read the every-start scroll as a bug.
- Document the two degraded-migration ERROR log signals in
docs/configuration.md so operators know when a clean restart is
required to recover indexing.
- Add unit test asserting scroll-time exceptions are logged and swallowed
without writing the sentinel.
Closes round-4 review feedback on PR #773.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
