admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(vector): propagate cancel in cleanup task; cover 403 + sweep-failure

Browse Source 
Address round-1 review on #913 and the SonarCloud new_reliability_rating gate:

- credential_cleanup_task no longer catches the cancellation exception
  (Sonar python:S7497). A task-group cancel must propagate for structured-
  concurrency teardown; graceful shutdown still flows through shutdown_event,
  so the sleep no longer needs a cancel/break.
- Parametrize the scanner self-heal tests over 401 AND 403 (handled
  identically at both call sites) and add a test that a failing periodic
  sweep is logged non-fatally and does not crash the task.
- Log the stored-user count before the startup sweep (operability signal),
  add a debug line when the credential row was already gone, and document
  the at-most-one extra-401 convergence in _remove_stale_credential.

Refs Deck #198.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-06-16 19:23:46 +02:00
co-authored by Claude Opus 4.8
parent 3790cf6d60
commit 7a9e4a8681
3 changed files with 67 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/app.py
+9
View File
@@ -2076,6 +2076,15 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None =
# drive an endless scanner re-spawn/401 loop (Deck #198). The
# credential_cleanup_task started below repeats it on a cadence.
try:
# Log the cohort first: the sweep makes one OCS validation
# call per stored user before readiness, so the count is the
# operability signal if startup latency ever climbs.
stored = await token_storage.get_all_app_password_user_ids()
if stored:
logger.info(
"Running startup credential sweep for %s stored user(s)",
len(stored),
)
removed = await token_storage.cleanup_invalid_app_passwords(
nextcloud_host=nextcloud_host_for_sync
)
nextcloud_mcp_server/vector/oauth_sync.py
+20 -6
View File
@@ -136,6 +136,11 @@ async def _remove_stale_credential(user_id: str, status_code: int) -> None:
#198). Deleting the row drops the user out of
``get_all_app_password_user_ids()`` so the scanner is not recreated.
Convergence: if ``user_manager_task`` already snapshotted the user IDs for
the current poll before this deletion, it re-spawns the scanner once more on
the next cycle, which fails auth and deletes again — at most one extra 401
per manager poll interval, versus the unbounded loop before this fix.
Best-effort: a storage failure here is logged, not raised — the periodic
``credential_cleanup_task`` sweep (and the next startup sweep) are backstops.
"""
@@ -147,6 +152,14 @@ async def _remove_stale_credential(user_id: str, status_code: int) -> None:
user_id,
status_code,
)
else:
# Row already gone — raced with the periodic sweep or another
# scanner exit. Harmless; logged for diagnostics.
logger.debug(
"[BasicAuth] No stale app password to remove for %s (HTTP %s)",
user_id,
status_code,
)
except Exception as e:
logger.warning(
"[BasicAuth] Failed to remove stale app password for %s: %s", user_id, e
@@ -473,12 +486,13 @@ async def credential_cleanup_task(
task_status.started()
while not shutdown_event.is_set():
# Sleep first — startup already swept; wake early on shutdown.
try:
with anyio.move_on_after(CREDENTIAL_CLEANUP_INTERVAL):
await shutdown_event.wait()
except anyio.get_cancelled_exc_class():
break
# Sleep first — startup already swept; wake early on shutdown. The
# graceful path goes through shutdown_event (move_on_after returns once
# teardown sets it), so we deliberately do NOT catch the cancellation
# exception: a task-group cancel must propagate for structured-
# concurrency teardown (re-swallowing it would breach anyio's contract).
with anyio.move_on_after(CREDENTIAL_CLEANUP_INTERVAL):
await shutdown_event.wait()
if shutdown_event.is_set():
break
tests/unit/vector/test_credential_self_heal.py
+38 -9
View File
@@ -1,12 +1,14 @@
"""Unit tests: self-healing removal of stale app passwords (Deck #198).
When a Nextcloud user is deleted/disabled, their stored app password keeps
returning 401. Previously the scanner just stopped, leaving the credential in
returning 401/403. Previously the scanner just stopped, leaving the credential in
storage so ``user_manager_task`` re-spawned the scanner every poll interval — an
endless re-spawn/401 loop. The scanner now deletes the credential on a hard auth
failure, and a periodic ``credential_cleanup_task`` sweeps any that slip through.
"""
import logging
import anyio
import httpx
import pytest
@@ -16,10 +18,10 @@ from nextcloud_mcp_server.vector import oauth_sync
pytestmark = pytest.mark.unit
def _http_401() -> httpx.HTTPStatusError:
def _http_error(status: int) -> httpx.HTTPStatusError:
req = httpx.Request("GET", "https://cloud.example.org/ocs")
return httpx.HTTPStatusError(
"unauth", request=req, response=httpx.Response(401, request=req)
"auth failure", request=req, response=httpx.Response(status, request=req)
)
@@ -52,11 +54,12 @@ async def test_remove_stale_credential_swallows_storage_error(mocker):
await oauth_sync._remove_stale_credential("ghost-user", 401)
async def test_scanner_removes_credential_on_prevalidation_401(mocker):
"""A 401 validating creds deletes the credential and never enters the scan
@pytest.mark.parametrize("status", [401, 403])
async def test_scanner_removes_credential_on_prevalidation_auth_failure(mocker, status):
"""A 401/403 validating creds deletes the credential and never enters the scan
loop — so ``user_manager_task`` won't see the user as provisioned again."""
fake_client = mocker.AsyncMock()
fake_client.capabilities = mocker.AsyncMock(side_effect=_http_401())
fake_client.capabilities = mocker.AsyncMock(side_effect=_http_error(status))
fake_client.close = mocker.AsyncMock()
mocker.patch.object(
oauth_sync,
@@ -82,8 +85,9 @@ async def test_scanner_removes_credential_on_prevalidation_401(mocker):
storage.delete_app_password.assert_awaited_once_with("ghost-user")
async def test_scanner_removes_credential_on_scan_loop_401(mocker):
"""A 401 raised while scanning (not pre-validation) also deletes the
@pytest.mark.parametrize("status", [401, 403])
async def test_scanner_removes_credential_on_scan_loop_auth_failure(mocker, status):
"""A 401/403 raised while scanning (not pre-validation) also deletes the
credential before the scanner stops."""
fake_client = mocker.AsyncMock()
fake_client.capabilities = mocker.AsyncMock(return_value={}) # pre-validation ok
@@ -96,7 +100,7 @@ async def test_scanner_removes_credential_on_scan_loop_401(mocker):
mocker.patch.object(
oauth_sync,
"scan_user_documents",
mocker.AsyncMock(side_effect=_http_401()),
mocker.AsyncMock(side_effect=_http_error(status)),
)
storage = mocker.MagicMock()
storage.delete_app_password = mocker.AsyncMock(return_value=True)
@@ -137,3 +141,28 @@ async def test_credential_cleanup_task_sweeps_then_stops(mocker):
storage.cleanup_invalid_app_passwords.assert_awaited_once_with(
"https://cloud.example.org"
)
async def test_credential_cleanup_task_swallows_sweep_exception(mocker, caplog):
"""A failing sweep is logged non-fatally and does not crash the task — the
loop survives to retry on the next cadence."""
mocker.patch.object(oauth_sync, "CREDENTIAL_CLEANUP_INTERVAL", 0)
shutdown = anyio.Event()
storage = mocker.MagicMock()
async def _boom(host):
shutdown.set() # exit after this (failed) iteration
raise RuntimeError("sweep failed")
storage.cleanup_invalid_app_passwords = mocker.AsyncMock(side_effect=_boom)
with caplog.at_level(
logging.WARNING, logger="nextcloud_mcp_server.vector.oauth_sync"
):
# Must return normally — the exception is swallowed, not propagated.
await oauth_sync.credential_cleanup_task(
storage, shutdown, "https://cloud.example.org"
)
storage.cleanup_invalid_app_passwords.assert_awaited_once()
assert "non-fatal" in caplog.text