refactor(vector-sync): address round-1 review nits

- ProvisionSignal.wait() re-arms in a finally so a cancelled wait (shutdown
  racing the doorbell) leaves a fresh unset event, not a stale set-but-consumed
  one; preserves the no-await-before-swap lost-wakeup guarantee.
- Hoist user_manager_task's _wake_on helper out of the while loop (one object,
  not one per iteration).
- Test: assert ProvisionSignal via its public wait() contract instead of the
  private _event attribute.
- Add test_provision_app_password_wakes_user_manager covering the
  api/passwords.py wake path (previously only LFv2 web + MCP tool were tested).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-06-12 09:56:27 +02:00
co-authored by Claude Opus 4.8
parent d8e3e9bc33
commit 3f09453926
3 changed files with 64 additions and 13 deletions
+18 -11
View File
@@ -58,9 +58,11 @@ class ProvisionSignal:
event back to shared state — avoiding any ``app`` ↔ ``vector`` import cycle.
Concurrency: ``anyio.Event`` is sticky, so a ``ring()`` that lands before
``wait()`` is still observed. ``wait()`` re-arms with no ``await`` between
observing the set and swapping the event, so under cooperative scheduling a
concurrent ``ring()`` cannot slip into that window and be lost.
``wait()`` is still observed. ``wait()`` re-arms in a ``finally`` with no
``await`` before the swap, so under cooperative scheduling a concurrent
``ring()`` cannot slip into that window and be lost — and the re-arm also
runs if ``wait()`` is cancelled (e.g. shutdown racing the doorbell), leaving
a fresh unset event rather than a stale set-but-consumed one.
"""
def __init__(self) -> None:
@@ -72,10 +74,13 @@ class ProvisionSignal:
async def wait(self) -> None:
"""Block until the next ring, then re-arm for the following cycle."""
await self._event.wait()
# No await before the swap: a concurrent ring() cannot interleave here,
# so it lands on the fresh event and the next wait() observes it.
self._event = anyio.Event()
try:
await self._event.wait()
finally:
# Re-arm even on cancellation. The assignment is not a checkpoint,
# so no concurrent ring() can interleave before the swap; a ring
# that already arrived lands on the fresh event for the next wait().
self._event = anyio.Event()
# Process-wide app-password storage for the BasicAuth client path.
@@ -475,6 +480,12 @@ async def user_manager_task(
logger.info("[BasicAuth] User manager started (poll interval: %ss)", poll_interval)
task_status.started()
# 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:
await wait_fn()
scope.cancel()
while not shutdown_event.is_set():
try:
# Query the app_passwords table — background sync always
@@ -537,10 +548,6 @@ async def user_manager_task(
# 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.
async def _wake_on(wait_fn, scope: anyio.CancelScope) -> None:
await wait_fn()
scope.cancel()
try:
with anyio.move_on_after(poll_interval):
async with anyio.create_task_group() as wake_tg:
@@ -254,6 +254,48 @@ async def test_provision_app_password_success(temp_storage, mocker):
assert get_kwargs["auth"] == ("testuser", "aaaaa-bbbbb-ccccc-ddddd-eeeee")
async def test_provision_app_password_wakes_user_manager(temp_storage, mocker):
"""A successful provision rings the background-sync doorbell so the user
manager re-polls immediately (the api/passwords.py wake path)."""
mocker.patch(
"nextcloud_mcp_server.api.passwords.get_settings",
return_value=MagicMock(
nextcloud_host="http://localhost:8080",
nextcloud_verify_ssl=True,
nextcloud_ca_bundle=None,
),
)
mock_response = MagicMock()
mock_response.status_code = 200
mock_response.json.return_value = {"ocs": {"data": {"id": "testuser"}}}
mock_client = AsyncMock()
mock_client.get = AsyncMock(return_value=mock_response)
mock_client.__aenter__ = AsyncMock(return_value=mock_client)
mock_client.__aexit__ = AsyncMock()
mocker.patch(
"nextcloud_mcp_server.api.passwords.nextcloud_httpx_client",
return_value=mock_client,
)
# Spy on the doorbell helper (imported locally from app at call time).
notify = mocker.patch("nextcloud_mcp_server.app.notify_user_provisioned")
client = TestClient(create_test_app(temp_storage))
response = client.post(
"/api/v1/users/testuser/app-password",
headers={
"Authorization": create_basic_auth_header(
"testuser", "aaaaa-bbbbb-ccccc-ddddd-eeeee"
)
},
)
assert response.status_code == 200
assert response.json()["success"] is True
notify.assert_called_once()
async def test_provision_app_password_uses_loginname_not_uid(temp_storage, mocker):
"""Regression: when the Nextcloud UID differs from the loginName (e.g.
OIDC-provisioned users whose UID is their display name — UID
@@ -203,11 +203,13 @@ def test_notify_user_provisioned_noop_without_manager(mocker):
app_module.notify_user_provisioned()
def test_notify_user_provisioned_rings_when_present(mocker):
async def test_notify_user_provisioned_rings_when_present(mocker):
"""When a manager is running, the helper rings its signal."""
import nextcloud_mcp_server.app as app_module
signal = ProvisionSignal()
mocker.patch.object(app_module._vector_sync_state, "provision_signal", signal)
app_module.notify_user_provisioned()
assert signal._event.is_set()
# Public contract: after a ring, the next wait() returns without blocking.
with anyio.fail_after(1):
await signal.wait()