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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
0388735593
commit
a188e9fced
@@ -9,8 +9,6 @@ exceptions so log lines name the actual cause; pair it with ``exc_info=True`` to
|
|||||||
keep the full traceback.
|
keep the full traceback.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
|
||||||
|
|
||||||
|
|
||||||
def format_exception_group(exc: BaseException) -> str:
|
def format_exception_group(exc: BaseException) -> str:
|
||||||
"""Return a concise, leaf-naming string for ``exc``.
|
"""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
|
result is meant for the human-readable portion of a log message, not for
|
||||||
parsing.
|
parsing.
|
||||||
"""
|
"""
|
||||||
leaves = _flatten(exc)
|
if not isinstance(exc, BaseExceptionGroup):
|
||||||
if len(leaves) == 1 and leaves[0] is exc:
|
|
||||||
return repr(exc)
|
return repr(exc)
|
||||||
|
leaves = _flatten(exc)
|
||||||
return f"{len(leaves)} sub-exception(s): " + "; ".join(repr(e) for e in leaves)
|
return f"{len(leaves)} sub-exception(s): " + "; ".join(repr(e) for e in leaves)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -236,6 +236,11 @@ async def processor_task(
|
|||||||
# Signal that the task has started and is ready
|
# Signal that the task has started and is ready
|
||||||
task_status.started()
|
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():
|
while not shutdown_event.is_set():
|
||||||
try:
|
try:
|
||||||
# Get document with timeout (allows checking shutdown)
|
# Get document with timeout (allows checking shutdown)
|
||||||
@@ -265,14 +270,22 @@ async def processor_task(
|
|||||||
break
|
break
|
||||||
|
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.error(
|
if doc_task is not None:
|
||||||
"Processor %s error processing %s_%s: %s",
|
logger.error(
|
||||||
worker_id,
|
"Processor %s error processing %s_%s: %s",
|
||||||
doc_task.doc_type,
|
worker_id,
|
||||||
doc_task.doc_id,
|
doc_task.doc_type,
|
||||||
format_exception_group(e),
|
doc_task.doc_id,
|
||||||
exc_info=True,
|
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)
|
# Continue to next document (no task_done() needed with streams)
|
||||||
|
|
||||||
logger.info("Processor %s stopped", worker_id)
|
logger.info("Processor %s stopped", worker_id)
|
||||||
|
|||||||
@@ -588,3 +588,23 @@ async def test_move_resource_encodes_destination_header(mocker):
|
|||||||
destination = call.kwargs["headers"]["Destination"]
|
destination = call.kwargs["headers"]["Destination"]
|
||||||
assert "%23" in destination
|
assert "%23" in destination
|
||||||
assert "#" not 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
|
||||||
|
|||||||
@@ -22,6 +22,9 @@ def test_format_exception_group_names_leaf_cause():
|
|||||||
formatted = format_exception_group(group)
|
formatted = format_exception_group(group)
|
||||||
|
|
||||||
assert "ConnectError" in formatted
|
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 "unhandled errors in a TaskGroup" not in formatted
|
||||||
assert "1 sub-exception" in formatted
|
assert "1 sub-exception" in formatted
|
||||||
|
|
||||||
|
|||||||
@@ -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)
|
||||||
Reference in New Issue
Block a user