fix(vector): gate scanner app polls on per-user enabled apps
The vector-sync scanner polled every indexed app (Notes, Files, News, Deck) for every provisioned user on each scan cycle. When a user lacks an app, its REST API returns 404; these were caught (indexing continued) but flooded tenant logs with repeated 404s, scaling with users x disabled-apps x scan-frequency and masking real failures. Add NextcloudClient.get_enabled_apps(), which reads the per-user /ocs/v2.php/core/navigation/apps endpoint (respects group restrictions). Chosen over /cloud/capabilities because the News app advertises no capability and never appears there. scan_user_documents now resolves the enabled-app set once per cycle and skips the Notes/News/Deck scans for apps the user lacks. Files stays unconditional (core Tags API, not a 404 source). Detection failures fall back to scanning every app (prior behaviour), so a transient nav-endpoint blip never silently halts indexing; the per-app 404 guards remain as the safety net. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
90d2192347
commit
2e609cbea7
@@ -199,6 +199,33 @@ class NextcloudClient:
|
||||
|
||||
return response.json()
|
||||
|
||||
async def get_enabled_apps(self) -> set[str]:
|
||||
"""Return the set of app ids enabled for the authenticated user.
|
||||
|
||||
Uses the per-user core navigation endpoint, which lists only apps the
|
||||
current user can access (respecting group restrictions). The vector
|
||||
scanner uses this to skip polling apps the user lacks, which would 404
|
||||
and flood tenant logs. Preferred over ``/cloud/capabilities`` because
|
||||
the News app advertises no capability and so never appears there.
|
||||
"""
|
||||
response = await self._client.get(
|
||||
"/ocs/v2.php/core/navigation/apps",
|
||||
headers={"OCS-APIRequest": "true", "Accept": "application/json"},
|
||||
)
|
||||
response.raise_for_status()
|
||||
data = response.json()
|
||||
entries = data.get("ocs", {}).get("data", []) or []
|
||||
enabled: set[str] = set()
|
||||
for entry in entries:
|
||||
# ``app`` is the canonical app id; ``id`` matches it for the apps we
|
||||
# gate. Union both so an unexpected nav-entry shape never hides an
|
||||
# enabled app — a false "disabled" would skip real indexing.
|
||||
for key in ("app", "id"):
|
||||
value = entry.get(key)
|
||||
if value:
|
||||
enabled.add(value)
|
||||
return enabled
|
||||
|
||||
async def notes_search_notes(self, *, query: str):
|
||||
"""Search notes using token-based matching with relevance ranking."""
|
||||
all_notes = self.notes.get_all_notes()
|
||||
|
||||
@@ -247,6 +247,28 @@ async def scanner_task(
|
||||
logger.info("Scanner task stopped - stream closed")
|
||||
|
||||
|
||||
async def _get_enabled_apps_or_none(
|
||||
nc_client: NextcloudClient, user_id: str, scan_id: int
|
||||
) -> set[str] | None:
|
||||
"""Enabled-app id set for gating, or ``None`` when detection fails.
|
||||
|
||||
``None`` signals "couldn't determine" — callers must then scan every app
|
||||
(the prior behaviour), so a transient navigation-endpoint failure never
|
||||
silently halts indexing. The per-app 404 guards in ``scan_user_documents``
|
||||
remain the safety net for that fallback path.
|
||||
"""
|
||||
try:
|
||||
return await nc_client.get_enabled_apps()
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"[SCAN-%s] Could not determine enabled apps for %s (%s); scanning all apps",
|
||||
scan_id,
|
||||
user_id,
|
||||
e,
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
async def scan_user_documents(
|
||||
user_id: str,
|
||||
send_stream: TaskProducer,
|
||||
@@ -328,6 +350,14 @@ async def scan_user_documents(
|
||||
|
||||
logger.debug("Found %s indexed documents in Qdrant", len(indexed_doc_ids))
|
||||
|
||||
# Determine which apps are enabled for this user so we skip polling
|
||||
# apps they lack — those polls 404 and flood tenant logs. ``None`` means
|
||||
# detection failed: fall back to scanning every app (prior behaviour).
|
||||
enabled_apps = await _get_enabled_apps_or_none(nc_client, user_id, scan_id)
|
||||
|
||||
def _app_enabled(app_id: str) -> bool:
|
||||
return enabled_apps is None or app_id in enabled_apps
|
||||
|
||||
# Notes (isolated so an uninstalled or disabled Notes app — whose API
|
||||
# returns 404 — cannot abort scanning of the other apps; this mirrors the
|
||||
# per-app try/except guards already wrapping files/news/deck below).
|
||||
@@ -336,29 +366,36 @@ async def scan_user_documents(
|
||||
current_time = time.time()
|
||||
queued = 0
|
||||
|
||||
try:
|
||||
queued += await scan_notes(
|
||||
user_id=user_id,
|
||||
send_stream=send_stream,
|
||||
nc_client=nc_client,
|
||||
initial_sync=initial_sync,
|
||||
scan_id=scan_id,
|
||||
prune_before=prune_before,
|
||||
indexed_doc_ids=indexed_doc_ids,
|
||||
grace_period=grace_period,
|
||||
current_time=current_time,
|
||||
)
|
||||
except HTTPStatusError as e:
|
||||
if e.response.status_code == 404:
|
||||
logger.info(
|
||||
"[SCAN-%s] Notes app unavailable for %s (HTTP 404); skipping notes",
|
||||
scan_id,
|
||||
user_id,
|
||||
if _app_enabled("notes"):
|
||||
try:
|
||||
queued += await scan_notes(
|
||||
user_id=user_id,
|
||||
send_stream=send_stream,
|
||||
nc_client=nc_client,
|
||||
initial_sync=initial_sync,
|
||||
scan_id=scan_id,
|
||||
prune_before=prune_before,
|
||||
indexed_doc_ids=indexed_doc_ids,
|
||||
grace_period=grace_period,
|
||||
current_time=current_time,
|
||||
)
|
||||
else:
|
||||
except HTTPStatusError as e:
|
||||
if e.response.status_code == 404:
|
||||
logger.info(
|
||||
"[SCAN-%s] Notes app unavailable for %s (HTTP 404); skipping notes",
|
||||
scan_id,
|
||||
user_id,
|
||||
)
|
||||
else:
|
||||
logger.warning("Failed to scan notes for %s: %s", user_id, e)
|
||||
except Exception as e:
|
||||
logger.warning("Failed to scan notes for %s: %s", user_id, e)
|
||||
except Exception as e:
|
||||
logger.warning("Failed to scan notes for %s: %s", user_id, e)
|
||||
else:
|
||||
logger.debug(
|
||||
"[SCAN-%s] Notes app not enabled for %s; skipping notes",
|
||||
scan_id,
|
||||
user_id,
|
||||
)
|
||||
|
||||
if initial_sync:
|
||||
logger.info("Sent %s documents for initial sync: %s", queued, user_id)
|
||||
@@ -666,31 +703,45 @@ async def scan_user_documents(
|
||||
|
||||
# Scan News items (starred + unread)
|
||||
news_queued = 0
|
||||
try:
|
||||
news_queued = await scan_news_items(
|
||||
user_id=user_id,
|
||||
send_stream=send_stream,
|
||||
nc_client=nc_client,
|
||||
initial_sync=initial_sync,
|
||||
scan_id=scan_id,
|
||||
if _app_enabled("news"):
|
||||
try:
|
||||
news_queued = await scan_news_items(
|
||||
user_id=user_id,
|
||||
send_stream=send_stream,
|
||||
nc_client=nc_client,
|
||||
initial_sync=initial_sync,
|
||||
scan_id=scan_id,
|
||||
)
|
||||
queued += news_queued
|
||||
except Exception as e:
|
||||
logger.warning("Failed to scan news items for %s: %s", user_id, e)
|
||||
else:
|
||||
logger.debug(
|
||||
"[SCAN-%s] News app not enabled for %s; skipping news items",
|
||||
scan_id,
|
||||
user_id,
|
||||
)
|
||||
queued += news_queued
|
||||
except Exception as e:
|
||||
logger.warning("Failed to scan news items for %s: %s", user_id, e)
|
||||
|
||||
# Scan Deck cards
|
||||
deck_queued = 0
|
||||
try:
|
||||
deck_queued = await scan_deck_cards(
|
||||
user_id=user_id,
|
||||
send_stream=send_stream,
|
||||
nc_client=nc_client,
|
||||
initial_sync=initial_sync,
|
||||
scan_id=scan_id,
|
||||
if _app_enabled("deck"):
|
||||
try:
|
||||
deck_queued = await scan_deck_cards(
|
||||
user_id=user_id,
|
||||
send_stream=send_stream,
|
||||
nc_client=nc_client,
|
||||
initial_sync=initial_sync,
|
||||
scan_id=scan_id,
|
||||
)
|
||||
queued += deck_queued
|
||||
except Exception as e:
|
||||
logger.warning("Failed to scan deck cards for %s: %s", user_id, e)
|
||||
else:
|
||||
logger.debug(
|
||||
"[SCAN-%s] Deck app not enabled for %s; skipping deck cards",
|
||||
scan_id,
|
||||
user_id,
|
||||
)
|
||||
queued += deck_queued
|
||||
except Exception as e:
|
||||
logger.warning("Failed to scan deck cards for %s: %s", user_id, e)
|
||||
|
||||
if queued > 0:
|
||||
logger.info(
|
||||
|
||||
@@ -7,7 +7,7 @@ folders) into a flat list of files.
|
||||
"""
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
@@ -31,6 +31,70 @@ def _make_client() -> Any:
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
def _navigation_response(entries: list[dict]) -> MagicMock:
|
||||
"""Build a mocked OCS v2 ``core/navigation/apps`` response."""
|
||||
response = MagicMock()
|
||||
response.status_code = 200
|
||||
response.raise_for_status = MagicMock()
|
||||
response.json.return_value = {"ocs": {"meta": {}, "data": entries}}
|
||||
return response
|
||||
|
||||
|
||||
class TestGetEnabledApps:
|
||||
async def test_returns_app_ids_from_navigation(self):
|
||||
client = _make_client()
|
||||
client._client = AsyncMock()
|
||||
client._client.get = AsyncMock(
|
||||
return_value=_navigation_response(
|
||||
[
|
||||
{"id": "files", "app": "files"},
|
||||
{"id": "notes", "app": "notes"},
|
||||
{"id": "deck", "app": "deck"},
|
||||
{"id": "news", "app": "news"},
|
||||
]
|
||||
)
|
||||
)
|
||||
|
||||
apps = await client.get_enabled_apps()
|
||||
|
||||
assert apps == {"files", "notes", "deck", "news"}
|
||||
# Hits the per-user navigation endpoint, not capabilities.
|
||||
assert (
|
||||
client._client.get.await_args.args[0] == "/ocs/v2.php/core/navigation/apps"
|
||||
)
|
||||
|
||||
async def test_unions_id_and_app_keys(self):
|
||||
"""When ``id`` and ``app`` differ, both are collected so an enabled
|
||||
app is never hidden by an unexpected nav-entry id."""
|
||||
client = _make_client()
|
||||
client._client = AsyncMock()
|
||||
client._client.get = AsyncMock(
|
||||
return_value=_navigation_response([{"id": "files_sharing", "app": "files"}])
|
||||
)
|
||||
|
||||
apps = await client.get_enabled_apps()
|
||||
|
||||
assert apps == {"files", "files_sharing"}
|
||||
|
||||
async def test_empty_navigation_returns_empty_set(self):
|
||||
client = _make_client()
|
||||
client._client = AsyncMock()
|
||||
client._client.get = AsyncMock(return_value=_navigation_response([]))
|
||||
|
||||
assert await client.get_enabled_apps() == set()
|
||||
|
||||
async def test_skips_entries_missing_both_keys(self):
|
||||
client = _make_client()
|
||||
client._client = AsyncMock()
|
||||
client._client.get = AsyncMock(
|
||||
return_value=_navigation_response(
|
||||
[{"name": "Logout", "href": "/logout"}, {"app": "notes"}]
|
||||
)
|
||||
)
|
||||
|
||||
assert await client.get_enabled_apps() == {"notes"}
|
||||
|
||||
|
||||
class TestNormaliseSearchResult:
|
||||
def test_adds_leading_slash_to_path(self):
|
||||
result = _normalise_search_result(
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
"""Unit tests for the vector scanner's enabled-app gating helper.
|
||||
|
||||
``scan_user_documents`` skips polling apps the user doesn't have enabled (those
|
||||
polls 404 and flood tenant logs). ``_get_enabled_apps_or_none`` resolves the
|
||||
enabled-app set, returning ``None`` on any failure so the caller falls back to
|
||||
scanning every app (the prior behaviour) rather than silently halting indexing.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
from httpx import HTTPStatusError, Request, Response
|
||||
|
||||
from nextcloud_mcp_server.vector.scanner import _get_enabled_apps_or_none
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
async def test_returns_enabled_set_on_success():
|
||||
nc_client = AsyncMock()
|
||||
nc_client.get_enabled_apps = AsyncMock(return_value={"files", "notes"})
|
||||
|
||||
result = await _get_enabled_apps_or_none(nc_client, "alice", scan_id=1234)
|
||||
|
||||
assert result == {"files", "notes"}
|
||||
|
||||
|
||||
async def test_returns_none_when_detection_raises(caplog):
|
||||
nc_client = AsyncMock()
|
||||
request = Request("GET", "http://nc.test/ocs/v2.php/core/navigation/apps")
|
||||
nc_client.get_enabled_apps = AsyncMock(
|
||||
side_effect=HTTPStatusError(
|
||||
"boom", request=request, response=Response(503, request=request)
|
||||
)
|
||||
)
|
||||
|
||||
import logging
|
||||
|
||||
caplog.set_level(logging.WARNING, logger="nextcloud_mcp_server.vector.scanner")
|
||||
result = await _get_enabled_apps_or_none(nc_client, "alice", scan_id=1234)
|
||||
|
||||
# None signals scan-all fallback; the inline gate treats `None` as
|
||||
# "every app enabled" so indexing never silently stops.
|
||||
assert result is None
|
||||
assert "scanning all apps" in caplog.text
|
||||
|
||||
|
||||
def test_none_set_enables_every_app():
|
||||
"""The gate predicate used in scan_user_documents: a None set means
|
||||
detection failed, so every app must be scanned (back-compat)."""
|
||||
|
||||
def app_enabled(app_id: str, enabled: set[str] | None) -> bool:
|
||||
return enabled is None or app_id in enabled
|
||||
|
||||
assert app_enabled("news", None) is True
|
||||
assert app_enabled("deck", None) is True
|
||||
# And a concrete set gates precisely.
|
||||
assert app_enabled("news", {"notes"}) is False
|
||||
assert app_enabled("notes", {"notes"}) is True
|
||||
Reference in New Issue
Block a user