refactor(vector-sync): clear SonarCloud gate + round-2 nits
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
3f09453926
commit
79d9d62e6a
@@ -397,6 +397,10 @@ def _wire_vector_sync_state(
|
|||||||
``eviction_task_group`` is deliberately not set here: it only exists once the
|
``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
|
lifespan has entered its ``anyio.create_task_group()`` (after this call), so
|
||||||
the lifespan assigns it on the singleton directly at that point.
|
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
|
send_stream = transport.send_stream
|
||||||
receive_stream = transport.receive_stream
|
receive_stream = transport.receive_stream
|
||||||
|
|||||||
@@ -20,6 +20,7 @@ background sync.
|
|||||||
|
|
||||||
import logging
|
import logging
|
||||||
import time
|
import time
|
||||||
|
from collections.abc import Awaitable, Callable
|
||||||
from dataclasses import dataclass, field
|
from dataclasses import dataclass, field
|
||||||
|
|
||||||
import anyio
|
import anyio
|
||||||
@@ -482,7 +483,9 @@ async def user_manager_task(
|
|||||||
|
|
||||||
# Sleep helper: await one of the wakeup events, then end the sleep by
|
# Sleep helper: await one of the wakeup events, then end the sleep by
|
||||||
# cancelling the shared scope. Defined once (not per loop iteration).
|
# 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()
|
await wait_fn()
|
||||||
scope.cancel()
|
scope.cancel()
|
||||||
|
|
||||||
@@ -546,17 +549,17 @@ async def user_manager_task(
|
|||||||
|
|
||||||
# Sleep until the next poll tick, but wake early on shutdown or a
|
# Sleep until the next poll tick, but wake early on shutdown or a
|
||||||
# provisioning signal so a just-provisioned user is discovered at once.
|
# provisioning signal so a just-provisioned user is discovered at once.
|
||||||
# Race both waits in a child task group; whichever fires first cancels
|
# Watch shutdown concurrently and block here on a provisioning ring;
|
||||||
# the scope, ending the sleep. move_on_after caps it at poll_interval.
|
# 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:
|
try:
|
||||||
with anyio.move_on_after(poll_interval):
|
with anyio.move_on_after(poll_interval):
|
||||||
async with anyio.create_task_group() as wake_tg:
|
async with anyio.create_task_group() as wake_tg:
|
||||||
wake_tg.start_soon(
|
wake_tg.start_soon(
|
||||||
_wake_on, shutdown_event.wait, wake_tg.cancel_scope
|
_wake_on, shutdown_event.wait, wake_tg.cancel_scope
|
||||||
)
|
)
|
||||||
wake_tg.start_soon(
|
await _wake_on(provision_signal.wait, wake_tg.cancel_scope)
|
||||||
_wake_on, provision_signal.wait, wake_tg.cancel_scope
|
|
||||||
)
|
|
||||||
except anyio.get_cancelled_exc_class():
|
except anyio.get_cancelled_exc_class():
|
||||||
break
|
break
|
||||||
|
|
||||||
|
|||||||
@@ -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``.
|
BasicAuth coverage in ``test_app_password_provisioning.py``.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import secrets
|
||||||
import tempfile
|
import tempfile
|
||||||
import time
|
import time
|
||||||
from pathlib import Path
|
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 ───────────────────────────
|
# ── 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(
|
completed = LoginFlowPollResult(
|
||||||
status="completed",
|
status="completed",
|
||||||
server="https://cloud.example.com",
|
server="https://cloud.example.com",
|
||||||
login_name="alice",
|
login_name="alice",
|
||||||
app_password="aaaaa-bbbbb-ccccc-ddddd-eeeee",
|
app_password=fake_app_password,
|
||||||
)
|
)
|
||||||
flow_client = AsyncMock()
|
flow_client = AsyncMock()
|
||||||
flow_client.poll.return_value = completed
|
flow_client.poll.return_value = completed
|
||||||
|
|||||||
@@ -134,7 +134,7 @@ async def test_user_manager_wakes_on_provision_signal(mocker):
|
|||||||
shutdown_event,
|
shutdown_event,
|
||||||
scanner_wake_event,
|
scanner_wake_event,
|
||||||
storage,
|
storage,
|
||||||
"http://nextcloud",
|
"https://nextcloud",
|
||||||
user_states,
|
user_states,
|
||||||
tg,
|
tg,
|
||||||
provision_signal,
|
provision_signal,
|
||||||
@@ -163,15 +163,22 @@ async def test_user_manager_shutdown_still_breaks_sleep(mocker):
|
|||||||
mocker.patch(
|
mocker.patch(
|
||||||
"nextcloud_mcp_server.vector.oauth_sync.get_settings", return_value=settings
|
"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(
|
mocker.patch(
|
||||||
"nextcloud_mcp_server.vector.oauth_sync._run_user_scanner_with_scope",
|
"nextcloud_mcp_server.vector.oauth_sync._run_user_scanner_with_scope",
|
||||||
# No users provisioned, so this is never called; keep a harmless stub.
|
_unused_scanner,
|
||||||
lambda *a, **k: anyio.sleep(0),
|
|
||||||
)
|
)
|
||||||
|
|
||||||
storage = _FakeStorage(set())
|
storage = _FakeStorage(set())
|
||||||
shutdown_event = anyio.Event()
|
shutdown_event = anyio.Event()
|
||||||
|
|
||||||
|
# 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:
|
async with anyio.create_task_group() as tg:
|
||||||
await tg.start(
|
await tg.start(
|
||||||
user_manager_task,
|
user_manager_task,
|
||||||
@@ -179,16 +186,13 @@ async def test_user_manager_shutdown_still_breaks_sleep(mocker):
|
|||||||
shutdown_event,
|
shutdown_event,
|
||||||
anyio.Event(),
|
anyio.Event(),
|
||||||
storage,
|
storage,
|
||||||
"http://nextcloud",
|
"https://nextcloud",
|
||||||
{},
|
{},
|
||||||
tg,
|
tg,
|
||||||
ProvisionSignal(),
|
ProvisionSignal(),
|
||||||
)
|
)
|
||||||
await anyio.sleep(0.05) # let it enter the sleep
|
await anyio.sleep(0.05) # let it enter the sleep
|
||||||
shutdown_event.set()
|
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
|
|
||||||
|
|
||||||
|
|
||||||
# ── notify_user_provisioned no-op guard ──────────────────────────────────────
|
# ── notify_user_provisioned no-op guard ──────────────────────────────────────
|
||||||
|
|||||||
Reference in New Issue
Block a user