fix(mcp): reject non-finite poll_after_seconds on TaskSnapshot (#4750)

Closes #4749

Co-authored-by: icn5381 <255778606+icn5381@users.noreply.github.com>
This commit is contained in:
icn5381 2026-08-14 23:48:44 +08:00 committed by GitHub
parent bd01ba9bf9
commit 79761908a4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 26 additions and 3 deletions

View File

@ -1,7 +1,7 @@
### MCP System (`packages/harness/deerflow/mcp/`) ### MCP System (`packages/harness/deerflow/mcp/`)
- Uses `langchain-mcp-adapters` `MultiServerMCPClient` for multi-server management - Uses `langchain-mcp-adapters` `MultiServerMCPClient` for multi-server management
- **Long-running task foundation**: `mcp/tasks/` defines the protocol-neutral `McpTaskDriver` contract and normalized `TaskSnapshot` states (`submitted`, `working`, `input_required`, `completed`, `failed`, `cancelled`). `persistence/mcp_tasks/` owns the durable remote-handle mapping, poll schedule, notification state, lease owner, and a consecutive poll-error counter (incremented on failed polls, reset on any applied snapshot — the total `poll_attempt_count` grows on every claim and cannot distinguish failure streaks) for a later driver-layer backoff/terminal-failure policy; `app/mcp_tasks/McpTaskService` performs status calls outside the Agent/LLM loop. A status result is applied only when the worker still owns an unexpired lease, so a stale result cannot be written after expiry even before another worker reclaims the row. Poll timestamps and retry schedules are based on the remote call's completion time rather than the scan start. If submission succeeds but persistence fails, the service best-effort cancels the remote task and preserves the original persistence error if that compensation also fails. The exact `uq_mcp_tasks_user_server_remote` conflict is different: an existing durable row already owns the remote handle, so the conflict surfaces without cancelling that tracked task. Unexpected per-task poll failures are isolated from sibling claims and remain recoverable through lease expiry; Gateway shutdown cancels the poller so a hung external status call cannot block process exit. `input_required` and terminal states stop polling and become `notification_status=pending` for later Agent/UI delivery. Durable recovery requires a SQL database backend (`sqlite` or `postgres`); the in-memory backend leaves the repository/service unavailable. The runtime is startup-configured by `mcp_tasks` and disabled by default until a concrete driver is registered; this foundation does not alter ordinary MCP tool behavior on its own. - **Long-running task foundation**: `mcp/tasks/` defines the protocol-neutral `McpTaskDriver` contract and normalized `TaskSnapshot` states (`submitted`, `working`, `input_required`, `completed`, `failed`, `cancelled`). A driver-supplied `poll_after_seconds` must be a finite positive number, validated at the `TaskSnapshot` boundary so every driver is held to the same invariant rather than each one guarding the consumer that turns the interval into a `timedelta`. `persistence/mcp_tasks/` owns the durable remote-handle mapping, poll schedule, notification state, lease owner, and a consecutive poll-error counter (incremented on failed polls, reset on any applied snapshot — the total `poll_attempt_count` grows on every claim and cannot distinguish failure streaks) for a later driver-layer backoff/terminal-failure policy; `app/mcp_tasks/McpTaskService` performs status calls outside the Agent/LLM loop. A status result is applied only when the worker still owns an unexpired lease, so a stale result cannot be written after expiry even before another worker reclaims the row. Poll timestamps and retry schedules are based on the remote call's completion time rather than the scan start. If submission succeeds but persistence fails, the service best-effort cancels the remote task and preserves the original persistence error if that compensation also fails. The exact `uq_mcp_tasks_user_server_remote` conflict is different: an existing durable row already owns the remote handle, so the conflict surfaces without cancelling that tracked task. Unexpected per-task poll failures are isolated from sibling claims and remain recoverable through lease expiry; Gateway shutdown cancels the poller so a hung external status call cannot block process exit. `input_required` and terminal states stop polling and become `notification_status=pending` for later Agent/UI delivery. Durable recovery requires a SQL database backend (`sqlite` or `postgres`); the in-memory backend leaves the repository/service unavailable. The runtime is startup-configured by `mcp_tasks` and disabled by default until a concrete driver is registered; this foundation does not alter ordinary MCP tool behavior on its own.
- **Lazy initialization**: Tools loaded on first use via `get_cached_mcp_tools()` - **Lazy initialization**: Tools loaded on first use via `get_cached_mcp_tools()`
- **Cache invalidation**: Detects extensions-config changes by comparing the resolved config path and a `(mtime, size, sha256)` content signature against the values recorded at initialization, not a strict mtime `>` comparison. This catches same-second edits, mtime that stays put or moves backward (`git checkout`, `cp -p` / backup restore, `tar` / `rsync`, object-store / network mounts), and a switch to a different config file with an equal-or-older mtime. The signature helper (`config/file_signature.py::get_config_signature`) is shared with `config/app_config.py::get_app_config()` for the sibling runtime-editable config file, rather than each maintaining its own copy. `ExtensionsConfig.resolve_config_path()` raises `FileNotFoundError` for an explicit `config_path`/`DEER_FLOW_EXTENSIONS_CONFIG_PATH` that points at a missing file — an operator-asserted path going missing is a real misconfiguration, so this is intentionally loud for callers that load the config for actual use (e.g. `from_file()` via `get_mcp_tools()`); only the fallback search mode returns `None`. The MCP cache's own path resolution (`mcp/cache.py::_resolve_config_path`) is narrower: it catches that specific `FileNotFoundError` locally and treats it the same as "unconfigured", so this staleness check degrades to "not stale" instead of propagating an exception when a previously-valid explicit/env-var config disappears mid-run - **Cache invalidation**: Detects extensions-config changes by comparing the resolved config path and a `(mtime, size, sha256)` content signature against the values recorded at initialization, not a strict mtime `>` comparison. This catches same-second edits, mtime that stays put or moves backward (`git checkout`, `cp -p` / backup restore, `tar` / `rsync`, object-store / network mounts), and a switch to a different config file with an equal-or-older mtime. The signature helper (`config/file_signature.py::get_config_signature`) is shared with `config/app_config.py::get_app_config()` for the sibling runtime-editable config file, rather than each maintaining its own copy. `ExtensionsConfig.resolve_config_path()` raises `FileNotFoundError` for an explicit `config_path`/`DEER_FLOW_EXTENSIONS_CONFIG_PATH` that points at a missing file — an operator-asserted path going missing is a real misconfiguration, so this is intentionally loud for callers that load the config for actual use (e.g. `from_file()` via `get_mcp_tools()`); only the fallback search mode returns `None`. The MCP cache's own path resolution (`mcp/cache.py::_resolve_config_path`) is narrower: it catches that specific `FileNotFoundError` locally and treats it the same as "unconfigured", so this staleness check degrades to "not stale" instead of propagating an exception when a previously-valid explicit/env-var config disappears mid-run
- **Transports**: stdio (command-based), SSE, HTTP - **Transports**: stdio (command-based), SSE, HTTP

View File

@ -1,5 +1,6 @@
from __future__ import annotations from __future__ import annotations
import math
from dataclasses import dataclass, field from dataclasses import dataclass, field
from enum import StrEnum from enum import StrEnum
from typing import Any from typing import Any
@ -50,8 +51,10 @@ class TaskSnapshot:
def __post_init__(self) -> None: def __post_init__(self) -> None:
if not isinstance(self.status, TaskStatus): if not isinstance(self.status, TaskStatus):
object.__setattr__(self, "status", TaskStatus(self.status)) object.__setattr__(self, "status", TaskStatus(self.status))
if self.poll_after_seconds is not None and self.poll_after_seconds <= 0: if self.poll_after_seconds is not None and (not math.isfinite(self.poll_after_seconds) or self.poll_after_seconds <= 0):
raise ValueError("poll_after_seconds must be positive") # NaN and infinity survive a bare `<= 0` check but break the consumer,
# which turns this interval into a `timedelta` for the next poll.
raise ValueError("poll_after_seconds must be a finite positive number")
if self.status == TaskStatus.INPUT_REQUIRED and self.input_required is None: if self.status == TaskStatus.INPUT_REQUIRED and self.input_required is None:
raise ValueError("input_required status requires an input_required payload") raise ValueError("input_required status requires an input_required payload")

View File

@ -1,3 +1,5 @@
from datetime import timedelta
import pytest import pytest
from deerflow.mcp.tasks import McpTaskDriverRegistry, TaskSnapshot, TaskStatus, TaskSubmission from deerflow.mcp.tasks import McpTaskDriverRegistry, TaskSnapshot, TaskStatus, TaskSubmission
@ -14,6 +16,24 @@ def test_input_required_snapshot_requires_payload():
TaskSnapshot(status=TaskStatus.INPUT_REQUIRED) TaskSnapshot(status=TaskStatus.INPUT_REQUIRED)
@pytest.mark.parametrize("interval", [float("nan"), float("inf")])
def test_snapshot_rejects_non_finite_poll_interval(interval):
with pytest.raises(ValueError, match="poll_after_seconds must be a finite positive number"):
TaskSnapshot(status=TaskStatus.WORKING, poll_after_seconds=interval)
@pytest.mark.parametrize("interval", [0, -1, float("-inf")])
def test_snapshot_rejects_non_positive_poll_interval(interval):
with pytest.raises(ValueError, match="poll_after_seconds"):
TaskSnapshot(status=TaskStatus.WORKING, poll_after_seconds=interval)
def test_snapshot_keeps_valid_poll_interval_schedulable():
snapshot = TaskSnapshot(status=TaskStatus.WORKING, poll_after_seconds=12.5)
assert snapshot.poll_after_seconds == 12.5
assert timedelta(seconds=snapshot.poll_after_seconds) == timedelta(seconds=12.5)
def test_submission_rejects_empty_remote_id(): def test_submission_rejects_empty_remote_id():
with pytest.raises(ValueError, match="remote_task_id must not be empty"): with pytest.raises(ValueError, match="remote_task_id must not be empty"):
TaskSubmission(remote_task_id=" ", snapshot=TaskSnapshot(status=TaskStatus.SUBMITTED)) TaskSubmission(remote_task_id=" ", snapshot=TaskSnapshot(status=TaskStatus.SUBMITTED))