From a188e9fced9daf31055468032f411900b5406eec Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 05:31:56 +0200 Subject: [PATCH] fix(vector): guard unbound doc_task + address review nits (#891) Round-1 review on PR #891: - Guard processor_task's broad except handler against an unbound doc_task (mirrors multi_user_processor_task): initialise doc_task=None before the loop and branch the error log. Fixes a latent NameError if receive() raises a non-TimeoutError/EndOfStream before the first document binds. Regression test added. - Drop the unnecessary `from __future__ import annotations` in vector/_errors.py and express format_exception_group's non-group fast path as an explicit isinstance check. - Add a copy_resource Destination-header encoding test (analogue to MOVE); strengthen the ExceptionGroup test to assert the full leaf repr survives. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/_errors.py | 6 +-- nextcloud_mcp_server/vector/processor.py | 29 ++++++++++---- tests/unit/client/test_webdav.py | 20 ++++++++++ tests/unit/vector/test_errors.py | 3 ++ tests/unit/vector/test_processor_task.py | 50 ++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 tests/unit/vector/test_processor_task.py diff --git a/nextcloud_mcp_server/vector/_errors.py b/nextcloud_mcp_server/vector/_errors.py index 903be308..6e526317 100644 --- a/nextcloud_mcp_server/vector/_errors.py +++ b/nextcloud_mcp_server/vector/_errors.py @@ -9,8 +9,6 @@ exceptions so log lines name the actual cause; pair it with ``exc_info=True`` to keep the full traceback. """ -from __future__ import annotations - def format_exception_group(exc: BaseException) -> str: """Return a concise, leaf-naming string for ``exc``. @@ -20,9 +18,9 @@ def format_exception_group(exc: BaseException) -> str: result is meant for the human-readable portion of a log message, not for parsing. """ - leaves = _flatten(exc) - if len(leaves) == 1 and leaves[0] is exc: + if not isinstance(exc, BaseExceptionGroup): return repr(exc) + leaves = _flatten(exc) return f"{len(leaves)} sub-exception(s): " + "; ".join(repr(e) for e in leaves) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 3d48be6f..72172146 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -236,6 +236,11 @@ async def processor_task( # Signal that the task has started and is ready task_status.started() + # Initialised before the loop so the broad except handler below can't hit an + # unbound name if receive() itself raises a non-TimeoutError/EndOfStream + # exception on the very first iteration (mirrors multi_user_processor_task). + doc_task: DocumentTask | None = None + while not shutdown_event.is_set(): try: # Get document with timeout (allows checking shutdown) @@ -265,14 +270,22 @@ async def processor_task( break except Exception as e: - logger.error( - "Processor %s error processing %s_%s: %s", - worker_id, - doc_task.doc_type, - doc_task.doc_id, - format_exception_group(e), - exc_info=True, - ) + if doc_task is not None: + logger.error( + "Processor %s error processing %s_%s: %s", + worker_id, + doc_task.doc_type, + doc_task.doc_id, + format_exception_group(e), + exc_info=True, + ) + else: + logger.error( + "Processor %s error: %s", + worker_id, + format_exception_group(e), + exc_info=True, + ) # Continue to next document (no task_done() needed with streams) logger.info("Processor %s stopped", worker_id) diff --git a/tests/unit/client/test_webdav.py b/tests/unit/client/test_webdav.py index 396ced79..b1268f2f 100644 --- a/tests/unit/client/test_webdav.py +++ b/tests/unit/client/test_webdav.py @@ -588,3 +588,23 @@ async def test_move_resource_encodes_destination_header(mocker): destination = call.kwargs["headers"]["Destination"] assert "%23" in destination assert "#" not in destination + + +@pytest.mark.unit +async def test_copy_resource_encodes_destination_header(mocker): + """The COPY Destination header must be percent-encoded too (card 309).""" + mock_http_client = AsyncMock() + client = WebDAVClient(mock_http_client, "testuser") + + mock_response = AsyncMock() + mock_response.status_code = 201 + mock_response.raise_for_status = mocker.Mock() + mock_http_client.request = AsyncMock(return_value=mock_response) + + await client.copy_resource("a/old.pdf", "b/new #1.pdf") + + call = mock_http_client.request.call_args + assert call[0][1] == "/remote.php/dav/files/testuser/a/old.pdf" + destination = call.kwargs["headers"]["Destination"] + assert "%23" in destination + assert "#" not in destination diff --git a/tests/unit/vector/test_errors.py b/tests/unit/vector/test_errors.py index 3336e03a..f3f7f0e4 100644 --- a/tests/unit/vector/test_errors.py +++ b/tests/unit/vector/test_errors.py @@ -22,6 +22,9 @@ def test_format_exception_group_names_leaf_cause(): formatted = format_exception_group(group) assert "ConnectError" in formatted + # Assert the full leaf repr survives, not just the type name -- guards a + # future format change that kept the type but dropped the message. + assert repr(leaf) in formatted assert "unhandled errors in a TaskGroup" not in formatted assert "1 sub-exception" in formatted diff --git a/tests/unit/vector/test_processor_task.py b/tests/unit/vector/test_processor_task.py new file mode 100644 index 00000000..8276a073 --- /dev/null +++ b/tests/unit/vector/test_processor_task.py @@ -0,0 +1,50 @@ +"""Regression test for processor_task's exception handler (card 309 / PR #891). + +If ``receive_stream.receive()`` raises something other than +``TimeoutError``/``EndOfStream`` before any document is bound, the broad +``except`` handler must not crash on an unbound ``doc_task`` name. +""" + +from unittest.mock import MagicMock + +import anyio +import pytest + +from nextcloud_mcp_server.vector.processor import processor_task + + +class _ReceiveBoomThenEnd: + """First receive() raises a non-Timeout error (no doc_task bound yet); the + second ends the stream so the loop exits.""" + + def __init__(self, shutdown: anyio.Event): + self._calls = 0 + self._shutdown = shutdown + + async def receive(self): + self._calls += 1 + if self._calls == 1: + raise RuntimeError("transport blew up before any document") + self._shutdown.set() + raise anyio.EndOfStream + + def statistics(self): # pragma: no cover - not reached on the error path + return MagicMock(current_buffer_used=0) + + +@pytest.mark.unit +async def test_processor_task_receive_error_does_not_raise_unbound(caplog): + shutdown = anyio.Event() + stream = _ReceiveBoomThenEnd(shutdown) + + # Must complete without a NameError leaking out of the except handler. + with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.processor"): + await processor_task( + worker_id=0, + receive_stream=stream, # type: ignore[arg-type] + shutdown_event=shutdown, + nc_client=MagicMock(), + user_id="alice", + ) + + assert any("RuntimeError" in rec.message for rec in caplog.records)