From 79d9d62e6a39b8f871acf9b1ce17a93b7896d823 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 12 Jun 2026 10:08:18 +0200 Subject: [PATCH] refactor(vector-sync): clear SonarCloud gate + round-2 nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quality-gate fixes (new-code conditions on PR #902): - new_security_hotspots_reviewed: drop the fake "http://nextcloud" host in the manager tests to https:// (python:S5332 ×2). - new_security_rating: generate the integration test's fake app password with secrets.token_urlsafe instead of a hardcoded literal (python:S2068). - new_reliability_rating: restructure the user_manager sleep so an explicit await checkpoint lives inside the cancellation scope — await one waiter directly while watching shutdown via start_soon (python:S7490). Behaviour is unchanged: timeout, shutdown, or a provisioning ring all end the sleep. Review nits: - Move the shutdown test's fail_after(2) to wrap the whole task group so it actually bounds the task-group exit (was guarding a no-op sleep); drop the sleep(0) stub (python:S7491). - Type _wake_on's wait_fn as Callable[[], Awaitable[object]]. - Note in _wire_vector_sync_state why provision_signal is set on the singleton only, not fanned out to app.state. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/app.py | 4 ++ nextcloud_mcp_server/vector/oauth_sync.py | 15 ++++--- .../test_login_flow_provision_wake.py | 6 ++- .../test_user_manager_provision_wake.py | 44 ++++++++++--------- 4 files changed, 42 insertions(+), 27 deletions(-) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 98a37d08..510d80cf 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -397,6 +397,10 @@ def _wire_vector_sync_state( ``eviction_task_group`` is deliberately not set here: it only exists once the lifespan has entered its ``anyio.create_task_group()`` (after this call), so the lifespan assigns it on the singleton directly at that point. + ``provision_signal`` is likewise excluded on purpose — only ``user_manager_task`` + consumes it (request handlers reach it via ``notify_user_provisioned``), so the + multi-user lifespan sets it on the singleton directly rather than fanning it out + to ``app.state``/the browser sub-app. """ send_stream = transport.send_stream receive_stream = transport.receive_stream diff --git a/nextcloud_mcp_server/vector/oauth_sync.py b/nextcloud_mcp_server/vector/oauth_sync.py index e5e6a020..bbbcaa44 100644 --- a/nextcloud_mcp_server/vector/oauth_sync.py +++ b/nextcloud_mcp_server/vector/oauth_sync.py @@ -20,6 +20,7 @@ background sync. import logging import time +from collections.abc import Awaitable, Callable from dataclasses import dataclass, field import anyio @@ -482,7 +483,9 @@ async def user_manager_task( # Sleep helper: await one of the wakeup events, then end the sleep by # cancelling the shared scope. Defined once (not per loop iteration). - async def _wake_on(wait_fn, scope: anyio.CancelScope) -> None: + async def _wake_on( + wait_fn: Callable[[], Awaitable[object]], scope: anyio.CancelScope + ) -> None: await wait_fn() scope.cancel() @@ -546,17 +549,17 @@ async def user_manager_task( # Sleep until the next poll tick, but wake early on shutdown or a # provisioning signal so a just-provisioned user is discovered at once. - # Race both waits in a child task group; whichever fires first cancels - # the scope, ending the sleep. move_on_after caps it at poll_interval. + # Watch shutdown concurrently and block here on a provisioning ring; + # whichever fires first cancels the shared scope and ends the sleep, + # while move_on_after caps the wait at poll_interval. Awaiting one waiter + # directly keeps an explicit checkpoint inside the cancellation scope. try: with anyio.move_on_after(poll_interval): async with anyio.create_task_group() as wake_tg: wake_tg.start_soon( _wake_on, shutdown_event.wait, wake_tg.cancel_scope ) - wake_tg.start_soon( - _wake_on, provision_signal.wait, wake_tg.cancel_scope - ) + await _wake_on(provision_signal.wait, wake_tg.cancel_scope) except anyio.get_cancelled_exc_class(): break diff --git a/tests/integration/test_login_flow_provision_wake.py b/tests/integration/test_login_flow_provision_wake.py index f1f78ab4..d6b1cea6 100644 --- a/tests/integration/test_login_flow_provision_wake.py +++ b/tests/integration/test_login_flow_provision_wake.py @@ -18,6 +18,7 @@ This is the Login Flow v2 deployment-mode counterpart to the multi-user BasicAuth coverage in ``test_app_password_provisioning.py``. """ +import secrets import tempfile import time from pathlib import Path @@ -99,11 +100,14 @@ async def test_login_flow_provision_wakes_user_manager(temp_storage, mocker): ) # ── mock only the Nextcloud Login Flow v2 poll ─────────────────────────── + # Generated, not a hardcoded literal — keeps this a fake token, not a + # credential pattern (SonarQube python:S2068). + fake_app_password = secrets.token_urlsafe(24) completed = LoginFlowPollResult( status="completed", server="https://cloud.example.com", login_name="alice", - app_password="aaaaa-bbbbb-ccccc-ddddd-eeeee", + app_password=fake_app_password, ) flow_client = AsyncMock() flow_client.poll.return_value = completed diff --git a/tests/unit/vector/test_user_manager_provision_wake.py b/tests/unit/vector/test_user_manager_provision_wake.py index 2463f350..bd29731d 100644 --- a/tests/unit/vector/test_user_manager_provision_wake.py +++ b/tests/unit/vector/test_user_manager_provision_wake.py @@ -134,7 +134,7 @@ async def test_user_manager_wakes_on_provision_signal(mocker): shutdown_event, scanner_wake_event, storage, - "http://nextcloud", + "https://nextcloud", user_states, tg, provision_signal, @@ -163,32 +163,36 @@ async def test_user_manager_shutdown_still_breaks_sleep(mocker): mocker.patch( "nextcloud_mcp_server.vector.oauth_sync.get_settings", return_value=settings ) + + async def _unused_scanner(*args, **kwargs): + # No users provisioned, so this is never called; keep a harmless stub. + return None + mocker.patch( "nextcloud_mcp_server.vector.oauth_sync._run_user_scanner_with_scope", - # No users provisioned, so this is never called; keep a harmless stub. - lambda *a, **k: anyio.sleep(0), + _unused_scanner, ) storage = _FakeStorage(set()) shutdown_event = anyio.Event() - async with anyio.create_task_group() as tg: - await tg.start( - user_manager_task, - None, - shutdown_event, - anyio.Event(), - storage, - "http://nextcloud", - {}, - tg, - ProvisionSignal(), - ) - await anyio.sleep(0.05) # let it enter the sleep - shutdown_event.set() - # If shutdown didn't break the 1000s sleep, fail_after would trip. - with anyio.fail_after(2): - await anyio.sleep(0) # task group exit below is the real assertion + # fail_after wraps the whole task group: if shutdown_event doesn't break the + # 1000s sleep, the task group never exits and the 2s deadline trips. + with anyio.fail_after(2): + async with anyio.create_task_group() as tg: + await tg.start( + user_manager_task, + None, + shutdown_event, + anyio.Event(), + storage, + "https://nextcloud", + {}, + tg, + ProvisionSignal(), + ) + await anyio.sleep(0.05) # let it enter the sleep + shutdown_event.set() # ── notify_user_provisioned no-op guard ──────────────────────────────────────