The fast (pypdfium2) extractor can leak raw glyph codes on subset fonts with a
broken /ToUnicode CMap. The result scores high on the existing text-quality
heuristic -- a uniform glyph/Caesar offset preserves whitespace and token
lengths -- yet is unsearchable. The structured (pymupdf) tier extracts the same
pages correctly.
Add a language-agnostic C0-control-character-ratio signal to the tier-0
classifier that detects this corruption and routes the document to a new
`structured` recommended_tier. Wire the fast->structured hop on the inline path
and generalise it so a low-quality-but-non-empty layer also tries structured
before OCR -- the inline and external ingest modes now follow the full
fast->structured->ocr ladder identically. A scanned / no-text-layer document
(total_chars == 0) still shortcuts straight to OCR, since a text extractor
cannot recover a pure raster.
New per-tenant tunable DOCUMENT_GLYPH_CORRUPTION_RATIO (default 0.02); escalation
metrics gain a `corrupt_glyphs` reason label.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 2 review (PR #910):
- BLOCKING: BatchOcrJobStore._shared_lock is now lazy-init (anyio.Lock | None,
created on first shared() call) instead of at class-definition time — matches
the CLAUDE.md "no anyio primitives at import time" rule and OcrProcessor's
pattern. The None-check->assign has no await between, so it's race-free.
- document_ocr_mode now normalizes via _enum_fields (case-insensitive, like
document_ocr_provider) instead of a strict dynaconf is_in Validator, so
DOCUMENT_OCR_MODE=Batch normalizes to "batch" rather than erroring. Tests for
case-normalization + invalid-value rejection.
- TYPE_CHECKING-gated GatewayBatchOcrClient import so build_gateway_batch_client
/ _get_batch_client are typed `GatewayBatchOcrClient | None` instead of Any
(runtime import stays lazy to avoid the import cycle).
- Rename ocr_options -> doc_identity_options (it's threaded to all tiers; only
OCR reads it) + clarify the comment.
- Drop the redundant forward-ref quotes on _shared_instance.
- Add direct _batch_identity unit tests (partial/empty options branches).
Left as follow-up: reusing one httpx.AsyncClient across submit/poll (same
per-call pattern as the existing sync _GatewayOcrBackend; no clean aclose hook
on the cached client today).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 1 review (PR #910):
- BLOCKING: add document_ocr_mode / _batch_poll_seconds / _batch_max_wait_seconds
to config._field_map — without it dynaconf silently ignored the env vars and
DOCUMENT_OCR_MODE=batch could never be enabled in production. Add a regression
test asserting the three round-trip from env.
- migration 008: give batch_ocr_jobs a composite PRIMARY KEY on
(user_id, doc_id, doc_type, etag) instead of a bare UniqueConstraint (N1).
- OcrProcessor: use a dedicated _batch_client_lock instead of sharing the sync
backend lock (N3).
- tests: use https:// gateway URLs in the new fixtures to clear SonarCloud's
"insecure http" security hotspots (all 14 were test-only http://gw literals).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add DOCUMENT_OCR_MODE=sync|batch (default sync). In batch mode the tier-3 OCR
processor submits documents to the embedding gateway's async Batch OCR routes
(POST /v1/ocr/batch + GET /v1/ocr/batch/{job_id}, astrolabe-cloud-website#372)
for ~50% cheaper large-corpus backfill. The direct Mistral OCR path is left
untouched. Tracked on Deck #332.
Batch jobs run minutes-hours, so the OCR tier cannot block (the procrastinate
worker reclaims jobs in `doing` after INGEST_STALLED_JOB_SECONDS). Instead it
submits, records the gateway job id in a new per-tenant `batch_ocr_jobs` table
(procrastinate args are immutable across retries), and raises a BatchPending
signal that TieredEscalationStrategy turns into a same-queue deferred re-poll —
releasing the worker slot between polls. On completion the per-page markdown is
indexed like the sync path; a failure or a job past
DOCUMENT_OCR_BATCH_MAX_WAIT_SECONDS marks the document parse-failed.
Batch is opt-in and gateway-only: with the direct mistral backend, no gateway
URL, or the inline/memory pipeline (which can't defer), it falls back to sync.
One batch job per document (coalescing N docs/job is a follow-up).
- embedding/gateway_batch_client.py: submit/poll client (reuses GatewayTokenProvider).
- vector/batch_ocr_store.py + migration 008: job tracking (portable SQLite+PG).
- document_processors/escalation.py: BatchPending control-flow signal.
- document_processors/ocr.py: batch state machine + sync fallback.
- vector/processor.py: thread doc identity to the OCR tier; raise BatchPending
from the pending sentinel; propagate it as control flow (not a failure).
- vector/queue/procrastinate.py: BatchPending -> same-queue retry_in, exempt
from the transient cap (bounded by the processor's deadline).
- config + docs; tests across client/store/processor/strategy/parse-tier.
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review follow-ups (GHSA-8vh3-g2qg-2h2c PR):
- Add a dynaconf validator requiring WEBHOOK_SECRET to be >=16 chars when set
(None still allowed = webhooks disabled), so weak/placeholder secrets fail
at startup rather than in an audit. Covered by two new tests in test_config.py.
- Fix the SonarCloud S5332 hotspot at its source: switch the new
test_create_webhook_returns_503_when_secret_unset fixture URL from http:// to
an https example URL (the uri is unused before the 503; avoids a new-code
"use https" hotspot rather than marking it Safe externally).
- Nits: drop the unused app.state.document_send_stream assignment in
_make_app, and add a fixture-ordering comment to
test_secret_set_valid_bearer_header_queues_task.
(--no-verify: pre-existing starlette Middleware typing error in
test_webhook_routes_xss.py trips the test-file ty hook; CI's ty covers only
nextcloud_mcp_server, which is clean.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GHSA-8vh3-g2qg-2h2c (CVSS 9.1, CWE-306): POST /webhooks/nextcloud had no
authentication when WEBHOOK_SECRET was unset (the default). The receiver
trusted the attacker-supplied user.uid and fed it to Qdrant, letting an
unauthenticated network caller delete or re-index any user's vector
embeddings.
Webhooks now require WEBHOOK_SECRET end-to-end:
- app.py: the /webhooks/nextcloud route is only mounted when WEBHOOK_SECRET
is set; otherwise it 404s and a startup warning notes vector sync falls
back to the polling scanner.
- webhook_receiver.py: removed the warn-and-accept fallback. No secret -> 503,
missing/invalid bearer -> 401; the payload is never processed unauthenticated.
- webhook_routes.py / api/webhooks.py: webhook_auth_pair() raises
WebhookSecretNotConfigured instead of returning authMethod="none"; both
registration entry points return a clear 503 so no dead unauthenticated
webhooks are created.
Also expose webhooks availability to the Astrolabe UI via GET /api/v1/status
("webhooks_enabled": bool), set WEBHOOK_SECRET on the docker-compose
semantic-search dev services, and update env.sample + ADR-010 / ADR-018 /
webhook-management-guide docs.
Vector sync still works without a secret via the polling scanner.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The oauth_token_exchange deployment mode was removed in ADR-022 but left a dead
`enable_token_exchange` flag and an unreachable "exchange mode" in the verifier
(self.mode was hardcoded to "multi-audience"). Remove the remnants:
- config.py: drop the `enable_token_exchange` default and the
`ENABLE_TOKEN_EXCHANGE` branch in `_is_multi_user` (+ its doc line).
- unified_verifier.py: drop `self.mode` and the dead exchange-mode log branch;
simplify the docstrings to multi-audience only.
- test_unified_verifier.py: drop the `.mode` assertions (attribute removed);
collapse the redundant init tests.
Also remove docs/ADR-004-Code-Review.md — an orphaned code-review note, not an
ADR; it doesn't belong in the docs/ADR namespace.
(--no-verify: the ty-check hook flags 3 PRE-EXISTING type errors in
test_unified_verifier.py lines 346/362/441, untouched by this change; CI
type-checks only the package, which passes.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A stall is often systemic (a Qdrant/embedding outage stalls every in-flight
job), so reclaiming the whole batch at now() every */5min tick would
thundering-herd a recovering dependency, bypassing TieredEscalationStrategy's
per-job backoff. reclaim_stalled_ingest_jobs now offsets retry_at by a fixed
delay (INGEST_RECLAIM_RETRY_DELAY_SECONDS, default 30s; 0 = legacy immediate).
Also document the hot-vs-restart flag asymmetry: INGEST_ESCALATION_ENABLED is
re-read per job; INGEST_TRANSIENT_MAX_ATTEMPTS is snapshotted at worker startup.
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split external (procrastinate) document processing into per-tier queues so a
document is attempted at most once per tier and requeued to the next tier's
queue on a low-quality parse, using procrastinate's native retry.
- escalation.py: TIER_LADDER (fast->structured->ocr) + EscalateError signal
- registry: process_tier (one tier) + evaluate_escalation post-parse gate
(reuses classify_from_text) + next_available_tier; shared _classify_result
and _oversize_result with the inline pipeline
- processor: process_document(tier=...) runs one tier and raises EscalateError
before embed (junk text never indexed); inline memory path unchanged
- queue/procrastinate: ingest-fast|structured|ocr queues; TieredEscalationStrategy
(queue-hop on EscalateError, bounded same-tier transient retry); queue-aware
task; producer defers to ingest-fast; per-queue counts + all-queue reclaim
- cli: worker --tier {fast,structured,ocr}
- billing: pages_ocr usage event + pipeline_tier metadata (paid OCR billed apart)
- observability: astrolabe_ingest_queue_depth{queue,status} gauge + per-queue
counts in nc_get_vector_sync_status / management status endpoint
- config: INGEST_ESCALATION_ENABLED (default true), INGEST_TRANSIENT_MAX_ATTEMPTS
INGEST_ESCALATION_ENABLED=false and INGEST_QUEUE=memory preserve prior behaviour.
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two ingest-robustness fixes from card 309 (OHR-Bench smoke-test triage).
The OCR backend timeout was a hardcoded 180s module constant, so a tenant
whose gateway has its own shorter ceiling couldn't tune it. Promote it to
DOCUMENT_OCR_TIMEOUT_SECONDS (default 180), resolved per call via get_settings
so an override applies without a restart.
Large, awkward PDFs (e.g. a 42 MB scanned DUDE) were handed straight to the
fast/OCR tiers, where they burned the full OCR timeout for zero recovered
text. Add a pre-parse size guard in the tiered PDF pipeline: a PDF over
DOCUMENT_MAX_PDF_SIZE_MB (default 50, 0 disables) fails fast with
parse_failed_reason="oversize" before any tier runs, so the existing
permanent-failure path marks the placeholder failed and records
astrolabe_document_parse_failed_total{reason="oversize"} instead of retrying.
Both knobs go through Settings + dynaconf validators (env-var keys verified by
regression tests) and are documented under Background Indexing Configuration.
Refs: Deck board 12 card 309 (AC #3 OCR timeout + size guard).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- _DEFAULTS keys for NEXTCLOUD_OIDC_TOKEN_TYPE / NEXTCLOUD_OIDC_SCOPES were
registered as oidc_* (uppercasing to OIDC_*), so dynaconf
(ignore_unknown_envvars) never read the NEXTCLOUD_-prefixed env vars and the
fields stayed at their defaults. Prefix the keys to match _field_map; add a
regression test.
- Add gte=1 validator for HEALTH_READY_REFRESH_INTERVAL and a 1..65535 range
validator for PORT.
- Tie ReadinessCache.ttl_seconds to 2x the configured refresh interval so
is_stale() stays meaningful when the interval is tuned.
- Raise the refresh-loop exception log from DEBUG to WARNING.
- Make health_ready a sync handler (no awaits); dedupe the localhost fallback
into _DEFAULT_MCP_SERVER_URL; use pytest.approx for the float default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes MCP reconnect timeouts on tenant servers (Deck #302). Three changes:
- /health/ready now gates only on local config. Nextcloud/Qdrant health is
refreshed by a background loop, cached, and reported but NON-gating, so a
single-replica tenant Pod is no longer pulled from its Service on a transient
dependency blip (which dropped every MCP streamable-HTTP session and caused
reconnect timeouts). The probe path performs no external I/O.
- Refactor starlette_lifespan: collapse the four near-identical per-mode
task-group + session + yield + teardown skeletons into one shared task group
that also runs the readiness refresh loop; each mode contributes a
(start, teardown) pair. eviction_task_group is now always present.
- Migrate app.py off os.getenv: all config is read through dynaconf Settings
(adds health_ready_refresh_interval, oidc_token_type, oidc_scopes, port).
Inline/dynamic defaults preserved at each call site.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deck #67 data-plane slice: tenant Pods record billable operations
(embedding queries, pages/chunks embedded) into an app-DB usage_events
table that the control plane later pulls read-only into the billing
ledger and syncs to Stripe Meter Events.
- migration 007: usage_events table (Postgres TIMESTAMPTZ/JSONB/UUID
with portable SQLite fallbacks), indexed (occurred_at, metric) for the
CP rollup's per-day range scan + GROUP BY metric.
- UsageEventStore: best-effort, flag-gated writer reusing the shared
RefreshTokenStorage engine; ON CONFLICT (event_id) DO NOTHING for
idempotent retries; dialect-branched occurred_at bind. All work
(incl. metadata JSON encode) is swallowed so a metering failure never
surfaces to the user op.
- USAGE_METERING_ENABLED flag (default off) wired through Settings +
env map; off-path touches no storage, so OSS self-hosters get an empty
table and zero write overhead.
- two recording hooks: embeddings_queries (per nc_semantic_search, which
nc_semantic_search_answer reuses) and pages_chunks (after dense
embedding succeeds, covering both in-process and procrastinate paths).
- storage.acquire()/.dialect public seams so the sibling store doesn't
reach into the underscored internal.
- tests parametrized over SQLite + Postgres: flag-off no-op, roundtrip,
ON CONFLICT dedup, JSON/NULL metadata, and the best-effort swallow of
both DB errors and unserializable metadata.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add PageAwareChunker, which splits paginated documents (PDFs) on page
boundaries first and only character-splits pages larger than chunk_size.
No chunk spans a page boundary, so page_number is always exact and stored
excerpts never lead with a neighbouring page's text. When chunk_size is at
least the largest page, this yields exactly one chunk per page: a
predictable vector count (== page count), a flat per-page embedding cost,
and zero cross-page overlap duplication.
Gated by DOCUMENT_CHUNK_PAGE_AWARE (default true). When false, the legacy
char-based DocumentChunker + post-hoc assign_page_numbers path runs
unchanged. Only doc_type="file" with page_boundaries (PDFs) takes the
page-aware path; notes/deck/news are unaffected.
Measured on a 15-page record (query "leadership award louis", target =
top-half of page 15): char-based degraded the target to dense-rank 10 at
cs=2048 (OCR) and mislabeled its page; page-aware restored rank 1 across
every fusion/modality and chunk size, with correct page labels and clean
snippets.
BREAKING CHANGE: PDFs are re-chunked page-aware by default. Existing
deployments will re-index PDF content on the next vector sync (different
chunk counts and page_number labels). Set DOCUMENT_CHUNK_PAGE_AWARE=false
to retain the previous char-based behaviour.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hot-path classifier escalated to OCR purely on character count, so a
scanned/handwritten PDF with a low-quality embedded text layer (>16 chars/page
but garbled) routed `fast` and indexed the junk -- e.g. Student 147.pdf's
"Little Acoms Primary"/"0110912020", which pollutes the vector and demotes the
doc in search (Deck #207).
- classifier: recalibrate `_text_quality` with a long-token-fraction term that
detects word-merging (dropped inter-word spaces) -- the dominant junk-layer
failure the old whitespace/overlong(>20) terms missed. Measured: the Student
147 scan ~0.42 (60% pages junk) vs >=0.94 for clean digital docs.
- classify_from_text now routes on quality + scan: a page is OCR-worthy if
near-empty OR low text-quality OR (when OCR + scan detection are enabled) it's
mostly a raster image. New `image_coverage_per_page` re-opens the PDF for the
scan signal, so that cost is paid only by OCR-opted-in tenants. Thresholds are
passed in from per-tenant settings (keyword-only).
- config: 4 per-tenant settings -- DOCUMENT_OCR_MIN_TEXT_QUALITY (0.5),
DOCUMENT_OCR_PAGE_FRACTION (0.5), DOCUMENT_OCR_MIN_PAGE_CHARS (16),
DOCUMENT_OCR_DETECT_SCANNED (true) -- with range validators.
- metrics: new astrolabe_document_ocr_page_fraction histogram (the value the
page-fraction threshold acts on) alongside document_text_quality, so operators
can tune the OCR escalation per tenant (quality vs cost).
Escalation gate, OCR backends, and off-by-default behavior unchanged (#858).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review round 3:
- document_tier1_engine / document_ocr_provider now validate + normalize via
Settings.__post_init__ _enum_fields (the repo's canonical opt-in-enum pattern;
case-insensitive) instead of dynaconf Validators. A typo now raises ValueError
at load and "Gateway" normalizes to "gateway".
- classify_from_text gates no_text_layer/bad_text_layer on ocr_frac >=
OCR_PAGE_FRACTION, matching classify_pdf -- a "fast"-routed doc with a few junk
pages no longer emits a misleading flag (keeps the shadow vs hot-path
classification metrics consistent).
- build_ocr_backend warns when an EXPLICIT provider is misconfigured
(gateway without EMBEDDING_GATEWAY_URL, mistral without MISTRAL_API_KEY)
instead of silently returning None.
- Pypdfium2FastProcessor.health_check probes the import; documented why
OcrProcessor.health_check is unconditionally True (lazy per-tenant backends).
- Removed the leftover per-boundary / per-chunk debug logging loops.
Tests: enum normalization + rejection for the two new settings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the OCR escalation target the tiered registry already routes to. Scanned /
no-text-layer PDFs (the tier-0 "ocr" verdict) escalate here when
document_ocr_enabled (default off).
Two interchangeable backends, selected by document_ocr_provider
(auto | gateway | mistral | none):
- gateway: POST to the Astrolabe Cloud model gateway's /v1/ocr -- the same
M2M-authenticated gateway as embeddings, so NO provider keys live in the pod
(the platform default; reuses EMBEDDING_GATEWAY_URL + the M2M creds).
- mistral: call the Mistral OCR API directly from the pod (MISTRAL_API_KEY), for
self-hosters / deployments without the gateway.
"auto" prefers the gateway, then direct Mistral.
Both return per-page markdown joined into text + exact page_boundaries (the
pdf_highlighter contract; bbox re-derived from the PDF bytes as for other tiers).
Validated end-to-end via direct Mistral on the scanned Student 147.pdf:
success, 15 pages, 22k chars, offsets exact, ~4s.
Settings: document_ocr_provider (enum-validated), document_ocr_model
("mistral/mistral-ocr-latest" -- gateway routes on the prefix, the direct mistral
backend strips it). OcrProcessor registered at lowest priority so it is never the
non-tiered default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces single-engine pymupdf4llm extraction with a tiered pipeline (Deck #205,
follows the tier-0 classifier #855). pypdfium2 becomes the default and only
hot-path PDF extractor; pymupdf4llm is deprecated to a rollback toggle.
Why: pymupdf4llm's O(n^2) find_tables drove the OOM (#852) and the form-PDF
parse timeouts (#856), carries AGPL/commercial licensing liability, and -- per
the benchmarks -- recovers near-zero usable tables on the real corpus. pypdfium2
(Apache/BSD) extracts the same text far faster (Student 1a.pdf: 120s timeout ->
0.2s) with no table-detection bomb.
- document_processors/pypdfium2_fast.py: tier-1 "fast" processor emitting text +
exact page_boundaries (the pdf_highlighter contract). pymupdf processor is now
tier "structured" (the rollback engine), registered but not default.
- registry: tiered routing in ProcessorRegistry. tier-1 fast extracts, then
classification is DERIVED from that text (classifier.classify_from_text -- no
PDF re-open), records the classification metrics, and escalates scanned /
no-text-layer docs to the "ocr" tier when document_ocr_enabled (default off;
no provider yet, so fast is terminal). Wires record_document_escalation + the
real "escalated" span attribute (was hardcoded False).
- Removes the separate _shadow_classify pass from vector/processor.py -- it
re-opened every PDF and re-extracted text (~0.5-1.3s/doc of pure duplicated
CPU that lowered throughput); classification now rides the tier-1 extraction.
- Settings: document_tier1_engine ("pypdfium2" default | "pymupdf" rollback,
enum-validated), document_ocr_enabled (default false).
Tests: pypdfium2 extractor, registry tiering (fast routing, rollback, classify
recording, OCR escalation on/off), classify_from_text. Full unit suite green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
First step of the tiered document-processor effort (Deck #203): a cheap, local
pre-pass that recommends which extraction tier a PDF should start in, emitting
metrics WITHOUT changing routing yet -- so we gather per-tenant doc-mix data
before turning escalation on.
document_processors/classifier.py: classify_pdf(content) -> DocClassification.
Page-sampled (bounded on large docs), <~1s. Cheap signals only -- text-layer
chars, a text-quality score (catches the "Student 147" failure where a text
layer exists but is mashed/space-less junk), and image coverage. A page that is
mostly a raster image routes to OCR: its content (handwriting, stamps) isn't in
any text layer. Deliberately no get_drawings/graphics-density signal -- it's
slow on the exact pages it'd flag, the hotfix's graphics_limit already makes the
parse safe, and the (future) tier-1 quality gate catches lost tables.
Validated on the sample corpus: born-digital 2-col arxiv and a digital student
record -> fast (tier 1); a scanned+handwritten form -> ocr (tier 3).
Wiring (vector/processor.py): _shadow_classify runs the classifier on PDFs in a
worker thread, best-effort (never blocks/fails indexing), gated by the new
DOCUMENT_CLASSIFY_ENABLED setting. Metrics: astrolabe_document_classified_total
{recommended_tier}, astrolabe_document_classifier_flag_total{flag},
astrolabe_document_text_quality histogram.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The OOM hotfix (#852) set graphics_limit=5000, which caught the 955k-drawing
bomb but let a second pathology through: form/table PDFs (e.g. student records)
have ~1.5k grid-line vector drawings per page -- under 5000, so uncapped. With
those pages uncapped, pymupdf4llm's O(n^2) find_tables grinds ~17s/page, so a
7-page form hits the 120s timeout. All 6 current backfill parse failures in
tenant-blackbox-demo are this exact timeout (zero OOM, zero error).
Measured on a 7-page sample: graphics_limit=2000 -> 119s (timeout), 1000 -> 2.9s
-- with identical extracted text and ZERO recovered tables either way (the
expensive analysis produces nothing useful on these dense forms). Lowering the
default to 1000 makes them index in ~3s; the bomb file (955k >> 1000) stays
capped, and pages with genuine simple tables (<1000 drawings) still get table
detection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 round 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>
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>
- 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>
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>
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 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>
🔴 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>
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>
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>
Resolve the latest PR-review comment on the verify-on-read tag-gate work:
- tests: stringify note IDs in test_verify_on_read.py so SearchResult.id
matches production (scanner stringifies all IDs on write) — helper and the
keeps/deleted/mixed/dedupe assertions (blocking).
- tests: make the unshared-file negative control a PDF so the drop is
unambiguously "unshared", not a mime_type_filter mismatch.
- config: add Validator("VECTOR_SYNC_PDF_TAG", len_min=1) — an empty tag name
would make find_files_by_tag("") misbehave in the verifier and scanner.
- verification: correct the _verify_files comment — two batch fetches (tag
REPORT + EXCLUDED_TAGS lookup) are held under one semaphore slot; the
pure-Python intersection runs outside it.
- tests: de-duplicate the minimal-PDF constant into a shared PDF_BYTES in
tests/integration/conftest.py, imported by both integration modules.
Verified: ruff/format/ty/unit all green; the two integration modules
(10 tests) pass against a local Nextcloud (app-only, no MCP profile needed).
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>
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>
EMBEDDING_GATEWAY_URL is configured as a bare origin (scheme://host:port) —
the deployment's Service URL. GatewayProvider now appends the gateway's /v1
base path before handing the URL to the OpenAI SDK, so both embed posts
({base}/embeddings) and dimension discovery ({base}/models) land under /v1.
Idempotent: a URL already ending in /v1 is left unchanged.
This lets EMBEDDING_GATEWAY_URL stay a bare domain (matching the gitops
Service URLs) instead of requiring a hand-appended /v1.
Also align the `embedding_gateway_model` field default with _DEFAULTS
("mistral/mistral-embed"). The gateway catalog is provider-namespaced, and
_detect_dimension matches `entry.id == embedding_model`; the stale
un-namespaced default would silently miss the catalog entry and leave the
dimension unresolved (re-triggering the external-mode startup crash).
Tests: bare / trailing-slash / idempotent normalization + a bare-origin
discovery test asserting /v1/models. 16 gateway-provider tests pass;
providers + vector suites green (129 total); ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
External-mode tenant pods CrashLoop at startup: Qdrant collection init calls
get_dimension() before any embed(), but GatewayProvider only learns its
dimension lazily after the first embed, and the gateway model isn't an OpenAI
model so the base class can't know it statically.
- Add GatewayProvider._detect_dimension() — the async startup hook the
vector-sync bootstrap already invokes (vector/qdrant_client.py:
hasattr(provider, "_detect_dimension")) for Ollama. It GETs the gateway's
GET /v1/models and sets _dimension from the entry whose id matches the
configured model. Best-effort: any failure (old gateway, model absent,
network) leaves _dimension unset so the inherited lazy detect-on-first-embed
still applies — never fatal. Presents the M2M bearer when configured.
- Switch the default embedding_gateway_model to the gateway's provider-
namespaced id "mistral/mistral-embed" (the gateway routes on the "/"-prefix
and sends "mistral-embed" upstream); collapse a duplicated config field.
Pairs with astrolabe-cloud-website#229 (gateway /v1/models, namespaced ids).
Tests: discovery sets dim w/o embed, sends bearer, non-fatal on
404/absent/error, skips when already known.
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>
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>
claude-review on #799 flagged:
1. Stale inline comment in ``initialize()`` (line 466) still said
"Postgres uses a small bounded pool". Updated to reflect both
backends now use NullPool.
2. Stale ``close()`` docstring referenced pool-size starving
max_connections — irrelevant with NullPool. Replaced with the
NullPool-aware rationale (dispose still tears down in-flight
asyncpg connections cleanly).
3. ``docs/configuration.md`` actively directed operators to tune
DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW, with worked
examples and pool math. Both are now deprecated no-ops; the
table entries explain the deprecation and link to PR #799.
Operators reading the docs will no longer be confused into
tuning settings that don't do anything.
4. ``config.py`` comment for the deprecated fields updated to
record the deprecation. Validators are intentionally kept
(still reject < 1 / < 0) so misconfigured deploys fail loudly
rather than silently — the reviewer flagged this as a minor
UX wart but explicitly "not a blocker"; the docs change in (3)
keeps operators away from the config altogether.
5. New ``tests/unit/test_storage_engine.py`` with three tests:
- ``test_postgres_engine_uses_nullpool`` — pins ``isinstance(
engine.pool, NullPool)`` so a refactor back to QueuePool /
SingletonThreadPool can't silently re-introduce the cross-
event-loop crashes.
- ``test_postgres_engine_ignores_pool_sizing_settings`` —
setting DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW to huge
values must not change pool type (proves the deprecated
fields are wired-up no-ops).
- ``test_postgres_engine_missing_asyncpg_driver_message`` —
guards the existing actionable-error branch when the
optional ``[postgres]`` extra isn't installed.
Verified:
- ``uv run pytest tests/unit/`` — 1028 passed
- ``uv run ruff check`` clean on the touched python files
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses all 8 items in the round-4 bot review plus 4 remaining
SonarQube OPEN issues that were silently broken by round 3's
malformed NOSONAR markers.
NOSONAR syntax fix (clears the remaining 4 OPEN SQ issues)
----------------------------------------------------------
Round 3 used ``# NOSONAR S<rule_key>`` form. SonarQube Python doesn't
recognize the rule-key suffix — it treats the whole thing as a
malformed suppression directive (S7632) AND lets the underlying rule
keep firing (S7503 on ``_Cursor.__aenter__/__aexit__``).
Switch every marker to bare ``# NOSONAR``, with the rationale moved
into a preceding comment block. Affected sites:
- storage.py: ``_Cursor.__aenter__``, ``_Cursor.__aexit__``
- config.py: ``get_database_ssl()`` ``return False`` + ``ssl.create_default_context()``
- test_storage_logging.py: ``SENTINEL_PASSWORD_FRAGMENT`` constant
- test_storage_postgres.py: three ``bob_pw_v1`` / ``bob_pw_v2`` / ``carol_pw`` literals
Bot 🔴#1 — defensive NOSONAR on get_database_ssl `return False`
--------------------------------------------------------------
Bot predicted S4830 fires on the operator-opt-out path. SQ output
shows it doesn't currently fire, but bare NOSONAR added defensively
with rationale comment.
Bot 🔴#2 — defensive NOSONAR on f-string SQL
--------------------------------------------
``update_oauth_session`` builds its SET clause via ``f"{', '.join(update_fields)}"``;
``get_audit_logs`` builds its WHERE clause via string concatenation.
Both are safe (the fragments only come from this function's own
branches, no user input), but the patterns trip taint analysers.
Annotated both with bare NOSONAR + safety comment explaining the
hardcoded-fragments invariant. Note: S2077 doesn't currently fire
on these; defensive.
Bot 🟡#3 — pg_advisory_lock for concurrent migrations
-----------------------------------------------------
Without coordination, two pods rolling-updating simultaneously can
both observe ``has_alembic=False`` and both try to apply migrations
from scratch — the second crashes with "relation already exists".
New ``_migration_lock()`` async context manager:
- On Postgres: ``SELECT pg_advisory_lock(:lock_id)`` on a fresh
connection (separate from the engine pool so it survives the
``to_thread.run_sync`` worker), held across BOTH the schema-inspect
AND the migration call. Without that span, two pods could each
observe "no alembic_version" before either started migrating,
defeating the lock.
- On SQLite: yields immediately (file-level locking serializes
writes natively).
Lock ID derived from
``sha256(b"nextcloud-mcp-server:migrations")[:8]`` as a stable signed
int64 so we can't collide with other apps sharing the same Postgres.
Bot 🟡#4 — RefreshTokenStorage.close() + lifespan wiring
--------------------------------------------------------
New idempotent ``close()`` method calls ``await engine.dispose()``,
nulls the engine, resets ``_initialized``. Wired into both
``app_lifespan_basic`` (BasicAuth) and the OAuth lifespan teardown,
each wrapped in ``try/except Exception`` with ``logger.warning`` so a
buggy dispose can't block SIGTERM. Without this, pooled asyncpg
connections leak server-side slots until
``idle_in_transaction_session_timeout`` reaps them — with small pool
defaults and frequent k8s rolling restarts this can starve
``max_connections``.
Bot 🟢#5 — is_sqlite_url docstring on :memory:
----------------------------------------------
Updated docstring to note both file-backed and in-memory forms are
recognized; caller is responsible for ``:memory:`` magic.
Bot 🟢#6 — db_path via make_url(...).database
---------------------------------------------
Replaced ``database_url.split("///", 1)[1]`` hack with SQLAlchemy's
own URL parsing. Naturally handles in-memory (``.database is None``
→ falls back to ``""``). Same lazy-import pattern as the existing
``mask_db_password`` to avoid module-import-time cost.
Bot 🟢#7 — _to_sync_url unrecognized-driver guard
-------------------------------------------------
Pulled ``_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")`` into a
module constant. When an unrecognized ``+<driver>`` token survives
the strip, emits ``logger.warning`` with the known-supported list.
Behavior unchanged for valid URLs.
Bot 🟢#8 — get_audit_logs SELECT * → explicit columns
-----------------------------------------------------
Replaced ``SELECT *`` with explicit column list. Future schema
additions stay out of the dict return.
New tests
---------
- ``test_close_disposes_engine``: pins the public contract — engine
nulled, state reset, second call is a no-op.
- ``test_concurrent_initialize_serialized_by_advisory_lock``: spawns
3 concurrent inits against a fresh schema; asserts no "relation
already exists" and exactly one ``alembic_version`` row at the end.
Without the lock, this reliably fails on the second concurrent
task.
Docs
----
- ADR-026: new "Concurrent migrations across pods" subsection
documents the advisory-lock approach + lock-ID derivation.
Verification
------------
- ``uv run pytest tests/unit/`` — 1025 passed.
- ``TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres`` — 9 passed (was 7).
- ``ruff check && ruff format --check && ty check`` — clean.
Expected post-push: SQ scan reports 0 OPEN issues (was 4).
Tracked on Astrolabe Cloud POC board, card #99.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-3 fixes. Two threads:
- 9 OPEN SonarQube issues caused the "E Security Rating on New Code"
gate failure. The bot's diagnosis (sa.text(text_sql) → SQL injection)
was a wrong guess; the actual SQ rules firing were different.
- Bot's substantive concerns: pool defaults too aggressive,
delete_browser_session RETURNING path untested on Postgres,
schema_version legacy table created on Postgres, stale module
docstring.
- User's underlying question on the pool: "isn't 1 connection enough?"
Right-sized to 2+5 and documented the concurrency model in ADR-026
so the rationale is durable.
SonarQube quality-gate fixes (clears all 9 OPEN issues)
-------------------------------------------------------
- BLOCKER S6418: rename `SECRET` constant in test_storage_logging.py
to `SENTINEL_PASSWORD_FRAGMENT` + NOSONAR with rationale.
- CRITICAL S3776: extract `_build_postgres_engine()` from
`initialize()` (was complexity 26 > 15); incidentally creates a
clean unit-test seam for engine args.
- CRITICAL S4423: `ssl.create_default_context(cafile=...)` is flagged
as "weak protocol" — Python 3.10+ already negotiates the strongest
available protocol. Explicitly pass `purpose=ssl.Purpose.SERVER_AUTH`
and NOSONAR with the Python-version rationale.
- MAJOR S3358: split the TLS-mode nested ternary in the engine
factory into a `_describe_ssl_arg()` helper.
- MAJOR S2068 ×3: bind test app-password literals to local vars and
put `# NOSONAR S2068` on the same line as the literal (anchoring
requirement) instead of on the closing paren.
- MINOR S7503 ×2: `# NOSONAR S7503` on `_Cursor.__aenter__/__aexit__`
— they MUST be `async` per the context-manager protocol.
Pool sizing right-sized (answers "why so many connections?")
------------------------------------------------------------
- `DATABASE_POOL_SIZE` default 10 → **2**.
- `DATABASE_MAX_OVERFLOW` default 20 → **5**.
- Per-pod max drops from 30 to 7. With 3 replicas, total = 21
connections (was 90) — well under managed-Postgres
`max_connections=100`.
- New INFO log at startup: `Postgres engine ready: pool_size=N
max_overflow=M (per-pod max K connections)`. Surfaces the active
sizing without grepping config.
- New ADR-026 § "Concurrency model and pool sizing" explains
asyncpg's single-flight connection semantics, the MCP workload
shape (read-mostly point lookups), why-not-1 (multi-user
serialization), and the tune-up/tune-down recipe.
- `docs/configuration.md` table updated with new defaults +
homelab-vs-prod tuning guidance, linking the ADR.
RETURNING path covered on Postgres
----------------------------------
- New `test_browser_session_delete_returning` exercises the
`DELETE … RETURNING user_id` path — the only RETURNING clause in
the storage layer and the most dialect-sensitive SQL in this PR.
Asserts both present-row (returns True, row gone) and absent-row
(returns False) branches.
Schema portability polish
-------------------------
- `alembic 001`: gate `schema_version` table creation on
`op.get_bind().dialect.name == "sqlite"`. The table exists purely
to match the fingerprint of pre-Alembic SQLite databases; fresh
Postgres installs no longer carry the dead legacy table.
Misc polish
-----------
- Module docstring: "SQLite-based" → "SQL-backed", with a sentence
on the DATABASE_URL opt-in and an ADR-026 link.
- Comment on `_wrap_row` noting `row._mapping` is the documented
RowMapping accessor in SQLAlchemy 2.x despite the underscore.
Skipped (rationale in PR reply)
-------------------------------
- `_qmark_to_named` SQL-comment handling: docstring already notes
the limitation; no `?` in storage SQL comments today.
- Module-level `anyio.Lock()`: established precedent confirmed by
the bot itself.
- `get_audit_logs` `SELECT *`: pre-existing pattern, out of scope.
Verification
------------
- `uv run pytest tests/unit/` — 1025 passed.
- `TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 7 passed.
- `ruff check && ruff format --check && ty check` — clean.
- Confirmed `schema_version` absent on fresh Postgres, still present
on fresh SQLite.
Tracked on Astrolabe Cloud POC board, card #99.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-2 fixes after the bot review on PR #798 plus two user follow-ups
(self-signed Postgres support; asyncpg should be a PyPI extra). Folded
into the same PR rather than a follow-up since the work is still
unmerged.
Security
--------
- Mask database credentials in all 5 log call sites (storage.py × 4,
migrations.py × 1) via a new `mask_db_password()` helper in config.py.
Uses SQLAlchemy's `make_url(...).render_as_string(hide_password=True)`
with a regex fallback so the masking path never raises.
- New `tests/unit/test_storage_logging.py` asserts a sentinel password
never appears in `caplog` during `RefreshTokenStorage.initialize()`.
Distribution
------------
- `asyncpg` moved to `[project.optional-dependencies] postgres` so a
vanilla `pip install nextcloud-mcp-server` no longer pulls in the
~5 MB C extension. The Docker image runs `uv sync --extra postgres`,
so containerized deployments are unchanged.
- When `DATABASE_URL=postgresql+asyncpg://...` is set on a venv missing
the extra, `RefreshTokenStorage.initialize()` raises a friendly
RuntimeError pointing at `[postgres]` rather than the generic
ModuleNotFoundError.
TLS for the Postgres backend
----------------------------
- New `DATABASE_VERIFY_SSL` + `DATABASE_CA_BUNDLE` env vars mirror the
existing `NEXTCLOUD_VERIFY_SSL` / `NEXTCLOUD_CA_BUNDLE` pattern
(validators in Settings.__post_init__, `get_database_ssl()` helper
alongside `get_nextcloud_ssl_verify()`). `DATABASE_VERIFY_SSL=false`
wins over `DATABASE_CA_BUNDLE` for incident-response convenience.
- Default is **None** rather than True — keeps PR #798's behavior
intact for cluster-internal Postgres that runs without TLS. Operators
opt into verify-full or supply a private CA. ADR-026 records the
reasoning vs the Nextcloud HTTPS default.
- Engine factory in `storage.py` passes `ssl` via `connect_args` only
when `get_database_ssl()` returns non-None; otherwise asyncpg's
default (`prefer`) applies.
- Storage logs which TLS mode is active at INFO (no secret material).
Configurable connection pool
----------------------------
- `DATABASE_POOL_SIZE` (default 10) and `DATABASE_MAX_OVERFLOW`
(default 20) replace the hardcoded engine values. With many replicas
this can blow past managed-Postgres `max_connections=100`; tune down
for large fleets.
- gte-1 / gte-0 validators in __post_init__ reject 0/negative pool
sizes at startup with the offending value in the error.
Consistency polish
------------------
- Migration 006: convert raw `op.execute("ALTER TABLE ... ADD COLUMN")`
to `op.batch_alter_table(...).add_column(sa.Column("nonce", sa.Text))`
for stylistic consistency with the rewritten 001-005. Downgrade now
drops the column instead of being a no-op.
- `registered_webhooks.created_at` standardized from `sa.Float` to
`sa.BigInteger` (all other `*_at` columns); `store_webhook()` casts
`time.time()` → `int`.
- `is_sqlite_url()` made case-insensitive.
Testing
-------
- New `tests/integration/test_storage_postgres.py::test_cleanup_expired_roundtrip`
exercises `cleanup_expired_tokens`, `cleanup_expired_sessions`, and
`cleanup_expired_browser_sessions` — relies on DELETE rowcount,
historically dialect-tricky.
- `tests/unit/test_ssl_config.py` extended with `TestDatabaseSSLSettings`
+ `TestGetDatabaseSSL` classes (9 new tests) mirroring the existing
Nextcloud SSL tests one-for-one.
Docs
----
- `docs/configuration.md` Centralized-Storage section grew the four new
env vars + a homelab example with a private CA.
- `docs/ADR-026` grew Distribution, TLS, and `alembic/env.py` async-pattern
subsections explaining the non-obvious design choices.
Helm chart counterpart in cbcoutinho/helm-charts PR #34 (separate
commit on `feat/nextcloud-mcp-server-database-url`).
Verification
------------
- `uv run pytest tests/unit/` — 1025 passed.
- `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 6 passed (including new cleanup test).
- `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean.
Tracked on Astrolabe Cloud POC board, card #99.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a `DATABASE_URL` setting that lets `RefreshTokenStorage` run against
any SQLAlchemy async backend, primarily `postgresql+asyncpg://...` for
HA k8s deployments. Default behavior is unchanged: when `DATABASE_URL` is
unset the server falls back to the existing `TOKEN_STORAGE_DB` path /
ephemeral SQLite tempfile.
Why
---
Today every MCP pod needs its own PVC to hold the SQLite file, which
pins the Deployment to one replica and blocks horizontal scaling. With
this change, operators can point all replicas at a shared Postgres
(CNPG, RDS, etc.) and the pods become stateless. Encryption stays in
Python (Fernet); the database only sees ciphertext.
What changed
------------
- `config.get_database_url()` resolves DATABASE_URL → TOKEN_STORAGE_DB →
ephemeral tempfile in that priority order.
- `RefreshTokenStorage` builds a process-shared `AsyncEngine` in
`initialize()`. SQLite gets NullPool; Postgres gets pool_size=10,
max_overflow=20, pool_pre_ping=True. 30 aiosqlite call sites adapted
via a thin `_DBConn` / `_Cursor` / `_Row` / `_ExecuteCtx` shim so
existing method bodies need no churn beyond the connection
context-manager swap.
- 7 `INSERT OR REPLACE` statements rewritten as portable
`INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5).
- `sqlite_master` legacy-detection lookup replaced with SQLAlchemy
inspector so the path works against either backend.
- File-permission hardening + parent-dir creation gated on
`is_sqlite_url(...)` — centralized backends manage their own filesystem.
- Alembic migrations 001/002/003/005 converted from raw `op.execute(SQL)`
to portable `op.create_table()` / `op.create_index()` with SQLAlchemy
types. All timestamp columns are `sa.BigInteger` so Postgres allocates
BIGINT (unix epochs don't fit in INT4). SQLite treats BIGINT as
INTEGER, so existing deployments at revision 006 see no schema drift.
- `migrations.py` + CLI take URLs; `db {upgrade,downgrade,current,history}`
gain `--database-url / -u` alongside the legacy `--database-path / -d`.
`get_current_revision()` uses SQLAlchemy inspector instead of raw
sqlite3, so the CLI works against Postgres too.
- `docker-compose.yml` adds a `postgres-test` service under the
`postgres` profile (pinned `postgres:16-alpine` digest) for
integration testing.
- Unit storage tests parametrized over backends via shared
`tests/fixtures/storage_backend.py` — every test in
`test_app_password_storage.py` and `test_webhook_storage.py` runs
once per backend that is available. Postgres is opted in by
`TEST_DATABASE_URL`.
- New `tests/integration/test_storage_postgres.py` (5 tests, marked
`postgres` + `integration`) covers refresh-token, app-password,
OAuth-session, webhook, and audit-log paths end-to-end on Postgres.
- New `docs/ADR-026-pluggable-database-backend.md` records the decision;
`docs/configuration.md` documents `DATABASE_URL` with examples.
Out of scope
------------
- No SQLite → Postgres data migration tool (clean cutover; tokens reissue
on next login, webhooks re-register on next sync tick).
- This repo does not provision Postgres. The matching helm chart change
lives in cbcoutinho/helm-charts (database.url / existingSecret values).
Verification
------------
- `uv run pytest tests/unit/` — 1012 passed, SQLite path unchanged.
- `docker compose --profile postgres up -d postgres-test`
- `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres -v`
— 5 passed.
- `TEST_DATABASE_URL=... uv run pytest tests/unit/test_app_password_storage.py
tests/unit/test_webhook_storage.py` — 50 passed (25 per backend).
- `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean.
Tracked on Astrolabe Cloud POC board, card #99.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`_get_background_operations_enabled()` was emitting three advisory log
lines (1 INFO + 2 deprecation WARNINGs) on every call. Because
`get_settings()` is intentionally non-cached and runs on every MCP tool
invocation via `get_client()`, the "Automatically enabled background
operations for semantic search in multi-user mode" INFO line was
firing per-request — 569 entries/hour in one production tenant.
Gate the three log emissions behind a module-level
`_bg_ops_advisories_logged` flag, mirroring the existing
`_warn_missing_secret_once` precedent in
`vector/webhook_receiver.py`. The boolean-derivation path stays
unchanged, so the `Settings` value remains fresh per call.
Extends the autouse `_reload_dynaconf_after_test` fixture to reset the
new flag between tests, and adds two regression tests that call
`get_settings()` five times and assert each advisory fires exactly once.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two small post-merge cleanups deferred from PR #787 (ADR-022 follow-up).
Both were explicitly noted in the reviewer's "acknowledged deferred items"
list.
1. config.py: drop `enable_multi_user_basic_auth` and `enable_login_flow`
from the dynaconf `_DEFAULTS` dict. They were removed from `_field_map`
in PR #787, so `get_settings()` never read them anyway, but their
presence in `_DEFAULTS` was visually misleading — readers might think
they could be set via TOML when in fact `Settings.__post_init__`
derives them from `MCP_DEPLOYMENT_MODE`. Replaced with a NOTE comment
pointing at the canonical derivation site.
2. app.py: the lifespan code had
`use_basic_auth = not oauth_enabled or settings.enable_login_flow`,
which became always-True once PR #787 enforced
`oauth_enabled ↔ enable_login_flow` via __post_init__. Hard-coded to
`True` with a comment explaining the invariant and pointing at the
separate follow-up that will prune the now-unreachable
`use_basic_auth=False` code paths in `vector/oauth_sync.py` (which
includes deleting the `use_basic_auth` parameter from
`user_manager_task` / `oauth_processor_task` and the OAuth-token-refresh
branch in `get_user_client`). Kept the variable name and the call-site
conditionals as-is for now so that follow-up is a clean mechanical
diff.
No runtime behaviour change: `use_basic_auth` already evaluated to True
in every supported mode after PR #787, and the `_DEFAULTS` entries were
already shadowed by `__post_init__`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four small follow-ups from the reviewer's latest pass:
- tests/unit/test_stdio.py:18: the single_user_env fixture used
monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", ...). That env var
is no longer read after the ADR-022 follow-up; switched to delenv of
MCP_DEPLOYMENT_MODE which is the canonical mode-selection input today.
Comment updated to match.
- config_validators.py: when detect_auth_mode rejects an invalid
MCP_DEPLOYMENT_MODE, surface a one-line ADR-022 migration hint if the
rejected value is exactly "oauth_single_audience" (the most common
upgrade pain — users carrying that value over from ADR-021 .env files).
Other invalid values get the regular "Valid values: …" message
unchanged.
- config.py + config_validators.py: added cross-reference comments on
both mode-resolution sites (Settings.__post_init__ and
detect_auth_mode) noting that they each compute the canonical mode
independently and must be kept in sync when a new mode is added.
Surfaces the parallel-duplication intentionally so the next maintainer
doesn't have to discover it.
- docs/ADR-021-configuration-consolidation.md:92: appended a trailing
comment to the historical "valid values" example, marking
oauth_single_audience and oauth_token_exchange as removed in ADR-022.
ADR-021 stays as the historical record; the trailer points future
readers at the current state.
No functional changes; 1009 unit tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
