From 3790cf6d6021bd514bd5aac72a6faa221fde2bd9 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 16 Jun 2026 19:13:05 +0200 Subject: [PATCH 1/2] fix(vector): self-heal stale app passwords on auth failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleted/disabled Nextcloud users left their app_passwords row in storage, so user_manager_task re-spawned their scanner every poll interval only to 401 again — an endless re-spawn/auth-failure loop (observed on tenant-blackbox-demo: ~534 respawns/3h, matching the 60s poll interval). - Delete the stored app password on a hard 401/403 in user_scanner_task (both the pre-validation and in-scan-loop paths), breaking the re-spawn loop at the source so the user-manager stops recreating the scanner. - Add a periodic credential_cleanup_task backstop (hourly) that sweeps cleanup_invalid_app_passwords for anything the per-scanner path misses. - Run the startup cleanup for all deployment modes: drop the stale `not oauth_enabled` guard so login_flow tenants (the cloud default) are covered. NOTE: login_flow startup now makes one concurrent OCS validation call per stored user before readiness. Refs Deck #198. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/app.py | 42 ++++-- nextcloud_mcp_server/vector/oauth_sync.py | 93 +++++++++++- .../unit/vector/test_credential_self_heal.py | 139 ++++++++++++++++++ 3 files changed, 259 insertions(+), 15 deletions(-) create mode 100644 tests/unit/vector/test_credential_self_heal.py diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 39c383b8..fea4960d 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -130,6 +130,7 @@ from nextcloud_mcp_server.server.oauth_tools import register_oauth_tools from nextcloud_mcp_server.vector.metrics_publisher import vector_sync_metrics_task from nextcloud_mcp_server.vector.oauth_sync import ( ProvisionSignal, + credential_cleanup_task, oauth_processor_task, user_manager_task, ) @@ -2067,20 +2068,25 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = # how many per-user scanners the user-manager later starts. await _sweep_orphan_placeholders_if_enabled() - # Clean up stale app passwords at startup (BasicAuth mode only) - if not oauth_enabled: - try: - removed = await token_storage.cleanup_invalid_app_passwords( - nextcloud_host=nextcloud_host_for_sync + # Clean up stale app passwords at startup. All deployment modes + # now authenticate background sync via locally-stored app + # passwords (the OAuth refresh-token path was removed), so this + # must run regardless of oauth_enabled — login_flow tenants were + # previously skipped, letting deleted-user credentials linger and + # drive an endless scanner re-spawn/401 loop (Deck #198). The + # credential_cleanup_task started below repeats it on a cadence. + try: + removed = await token_storage.cleanup_invalid_app_passwords( + nextcloud_host=nextcloud_host_for_sync + ) + if removed: + logger.info( + "Cleaned up %s stale app password(s): %s", + len(removed), + removed, ) - if removed: - logger.info( - "Cleaned up %s stale app password(s): %s", - len(removed), - removed, - ) - except Exception as e: - logger.warning("App password cleanup failed (non-fatal): %s", e) + except Exception as e: + logger.warning("App password cleanup failed (non-fatal): %s", e) # Initialize the ingest transport. INGEST_QUEUE selects the # backend (Deck #183, ADR-028): ``memory`` builds an in-process @@ -2134,6 +2140,16 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = provision_signal, ) + # Periodic backstop sweep removing app passwords that no + # longer authenticate (deleted/disabled users), complementing + # the per-scanner self-heal in user_scanner_task (Deck #198). + await tg.start( + credential_cleanup_task, + token_storage, + shutdown_event, + nextcloud_host_for_sync, + ) + # In-process consumer pool. ``run_consumers`` is a no-op for # the distributed (postgres) backend — the out-of-process # ``worker`` role consumes there. The closure binds this diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index bbbcaa44..8860f89a 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -125,6 +125,34 @@ class UserSyncState: started_at: float = field(default_factory=time.time) +async def _remove_stale_credential(user_id: str, status_code: int) -> None: + """Delete a user's stored app password after a hard auth failure. + + A 401/403 from Nextcloud means the stored app password is no longer usable — + the user was deleted, disabled, or revoked the password. Without removing it + the credential lingers in storage and ``user_manager_task`` re-spawns this + scanner on its next poll (the user is still "provisioned"), only to fail auth + again — an endless re-spawn/401 loop that keeps hammering Nextcloud (Deck + #198). Deleting the row drops the user out of + ``get_all_app_password_user_ids()`` so the scanner is not recreated. + + Best-effort: a storage failure here is logged, not raised — the periodic + ``credential_cleanup_task`` sweep (and the next startup sweep) are backstops. + """ + try: + storage = await _get_initialized_basic_auth_storage() + if await storage.delete_app_password(user_id): + logger.info( + "[BasicAuth] Removed stale app password for %s after 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 + ) + + async def get_user_client_basic_auth( user_id: str, nextcloud_host: str, @@ -215,10 +243,12 @@ async def user_scanner_task( except HTTPStatusError as e: if e.response.status_code in (401, 403): logger.warning( - "[BasicAuth] Credential validation failed for %s (HTTP %s), not starting scan loop", + "[BasicAuth] Credential validation failed for %s (HTTP %s), " + "removing stale credential and not starting scan loop", user_id, e.response.status_code, ) + await _remove_stale_credential(user_id, e.response.status_code) return raise finally: @@ -262,10 +292,12 @@ async def user_scanner_task( status_code = e.response.status_code if status_code in (401, 403): logger.warning( - "[BasicAuth] Scanner auth failed for %s (HTTP %s), stopping scanner. User may need to re-provision credentials.", + "[BasicAuth] Scanner auth failed for %s (HTTP %s), removing stale " + "credential and stopping scanner. User must re-provision to resume sync.", user_id, status_code, ) + await _remove_stale_credential(user_id, status_code) break elif status_code == 429: retry_after = min(int(e.response.headers.get("Retry-After", "60")), 300) @@ -410,6 +442,63 @@ async def multi_user_processor_task( oauth_processor_task = multi_user_processor_task +# Backstop sweep cadence for cleanup_invalid_app_passwords (seconds). The +# per-scanner 401 deletion (_remove_stale_credential) is the primary self-heal; +# this periodic sweep is defense-in-depth for credentials whose scanner never +# ran or whose in-scanner deletion failed. One hour keeps the per-user OCS +# validation load negligible. +CREDENTIAL_CLEANUP_INTERVAL = 3600 + + +async def credential_cleanup_task( + storage: "RefreshTokenStorage", + shutdown_event: anyio.Event, + nextcloud_host: str, + *, + task_status: TaskStatus = anyio.TASK_STATUS_IGNORED, +) -> None: + """Periodically remove app passwords that no longer authenticate. + + Backstop for the per-scanner self-heal (``_remove_stale_credential``): + validates every stored app password against Nextcloud and deletes the ones + that return 401/403, so credentials for deleted/disabled users cannot + accumulate even if their scanner never ran (Deck #198). Runs on a fixed + cadence until ``shutdown_event`` is set. A fresh sweep already runs once at + startup (app lifespan), so this sleeps first. + """ + logger.info( + "[BasicAuth] Credential cleanup task started (interval: %ss)", + CREDENTIAL_CLEANUP_INTERVAL, + ) + 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 + if shutdown_event.is_set(): + break + + try: + removed = await storage.cleanup_invalid_app_passwords(nextcloud_host) + if removed: + logger.info( + "[BasicAuth] Periodic cleanup removed %s stale app password(s): %s", + len(removed), + removed, + ) + except Exception as e: + logger.warning( + "[BasicAuth] Periodic credential cleanup failed (non-fatal): %s", + format_exception_group(e), + ) + + logger.info("[BasicAuth] Credential cleanup task stopped") + + async def _run_user_scanner_with_scope( user_id: str, cancel_scope: anyio.CancelScope, diff --git a/tests/unit/vector/test_credential_self_heal.py b/tests/unit/vector/test_credential_self_heal.py new file mode 100644 index 00000000..55063e5e --- /dev/null +++ b/tests/unit/vector/test_credential_self_heal.py @@ -0,0 +1,139 @@ +"""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 +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 anyio +import httpx +import pytest + +from nextcloud_mcp_server.vector import oauth_sync + +pytestmark = pytest.mark.unit + + +def _http_401() -> httpx.HTTPStatusError: + req = httpx.Request("GET", "https://cloud.example.org/ocs") + return httpx.HTTPStatusError( + "unauth", request=req, response=httpx.Response(401, request=req) + ) + + +async def test_remove_stale_credential_deletes(mocker): + """The helper deletes the user's app password via storage.""" + storage = mocker.MagicMock() + storage.delete_app_password = mocker.AsyncMock(return_value=True) + mocker.patch.object( + oauth_sync, + "_get_initialized_basic_auth_storage", + mocker.AsyncMock(return_value=storage), + ) + + await oauth_sync._remove_stale_credential("ghost-user", 401) + + storage.delete_app_password.assert_awaited_once_with("ghost-user") + + +async def test_remove_stale_credential_swallows_storage_error(mocker): + """Best-effort: a storage failure is logged, never raised (backstops cover it).""" + storage = mocker.MagicMock() + storage.delete_app_password = mocker.AsyncMock(side_effect=RuntimeError("db down")) + mocker.patch.object( + oauth_sync, + "_get_initialized_basic_auth_storage", + mocker.AsyncMock(return_value=storage), + ) + + # Must not raise. + 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 + 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.close = mocker.AsyncMock() + mocker.patch.object( + oauth_sync, + "get_user_client_basic_auth", + mocker.AsyncMock(return_value=fake_client), + ) + storage = mocker.MagicMock() + storage.delete_app_password = mocker.AsyncMock(return_value=True) + mocker.patch.object( + oauth_sync, + "_get_initialized_basic_auth_storage", + mocker.AsyncMock(return_value=storage), + ) + + await oauth_sync.user_scanner_task( + "ghost-user", + mocker.MagicMock(), # send_stream — unused on the pre-validation path + anyio.Event(), # shutdown_event + anyio.Event(), # wake_event + "https://cloud.example.org", + ) + + 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 + credential before the scanner stops.""" + fake_client = mocker.AsyncMock() + fake_client.capabilities = mocker.AsyncMock(return_value={}) # pre-validation ok + fake_client.close = mocker.AsyncMock() + mocker.patch.object( + oauth_sync, + "get_user_client_basic_auth", + mocker.AsyncMock(return_value=fake_client), + ) + mocker.patch.object( + oauth_sync, + "scan_user_documents", + mocker.AsyncMock(side_effect=_http_401()), + ) + storage = mocker.MagicMock() + storage.delete_app_password = mocker.AsyncMock(return_value=True) + mocker.patch.object( + oauth_sync, + "_get_initialized_basic_auth_storage", + mocker.AsyncMock(return_value=storage), + ) + + await oauth_sync.user_scanner_task( + "ghost-user", + mocker.MagicMock(), + anyio.Event(), + anyio.Event(), + "https://cloud.example.org", + ) + + storage.delete_app_password.assert_awaited_once_with("ghost-user") + + +async def test_credential_cleanup_task_sweeps_then_stops(mocker): + """The periodic backstop validates stored passwords via + ``cleanup_invalid_app_passwords`` and exits on shutdown.""" + mocker.patch.object(oauth_sync, "CREDENTIAL_CLEANUP_INTERVAL", 0) + shutdown = anyio.Event() + storage = mocker.MagicMock() + + async def _cleanup(host): + shutdown.set() # stop the loop after the first sweep + return ["ghost-user"] + + storage.cleanup_invalid_app_passwords = mocker.AsyncMock(side_effect=_cleanup) + + await oauth_sync.credential_cleanup_task( + storage, shutdown, "https://cloud.example.org" + ) + + storage.cleanup_invalid_app_passwords.assert_awaited_once_with( + "https://cloud.example.org" + ) From 7a9e4a8681abaa45764902e81a273d76c3f17ccb Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 16 Jun 2026 19:23:46 +0200 Subject: [PATCH 2/2] fix(vector): propagate cancel in cleanup task; cover 403 + sweep-failure 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) --- nextcloud_mcp_server/app.py | 9 ++++ nextcloud_mcp_server/vector/oauth_sync.py | 26 +++++++--- .../unit/vector/test_credential_self_heal.py | 47 +++++++++++++++---- 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index fea4960d..b2f69649 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -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 ) diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index 8860f89a..0a1eeed6 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -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 diff --git a/tests/unit/vector/test_credential_self_heal.py b/tests/unit/vector/test_credential_self_heal.py index 55063e5e..7644fd5d 100644 --- a/tests/unit/vector/test_credential_self_heal.py +++ b/tests/unit/vector/test_credential_self_heal.py @@ -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