fix(gateway): reject MCP task cancellation when the worker is stopped (#4963)

The cancel endpoint resolved McpTaskService whenever SQL persistence was
configured, even with mcp_tasks.enabled=false, and acknowledged the
request by recording cancel_requested_at. The background loop that owns
the remote cancel call only runs when enabled, so the fence was never
claimed and the remote task kept running indefinitely.

Gate the endpoint on app.state.mcp_tasks_available (set only after the
service is started) and return 503 before writing the fence, consistent
with the background-loop ownership contract. Read-only list/detail
endpoints remain available while the worker is stopped.
This commit is contained in:
Zeren Wang 2026-08-24 00:57:55 +02:00 committed by GitHub
parent 917fe595fc
commit b36504edf2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 49 additions and 1 deletions

View File

@ -20,7 +20,7 @@ DeerFlow is a LangGraph-based AI super agent system with a full-stack architectu
- Scheduled-task executions must reuse that same Gateway run lifecycle. The scheduler may decide *when* work runs, but it must dispatch through the existing run path rather than introducing a parallel execution stack.
- The background scheduler is single-instance by default. `scheduler.multi_instance=true` opts into lease-aware recovery across Gateway instances and requires shared Postgres, `run_ownership.heartbeat_enabled=true`, and `run_events.backend=db`; otherwise startup rejects the configuration. Live scheduled runs are preserved when a peer starts; expired leases are atomically taken over, stale post-launch writes are fenced by the dispatch lease owner, and the Postgres advisory-locked budget makes `max_concurrent_runs` a shared global cap (including pre-launch reservations).
- Long-running MCP work uses a separate durable task runtime rather than keeping remote task IDs or status polling inside the Agent loop. Explicit `task_toolsets` bind raw submit/status/cancel names; only submit remains Agent-visible, and its wrapper persists the remote handle before returning a local ID. `McpTaskService` claims due rows with leases, resolves a protocol-specific `McpTaskDriver`, and writes normalized snapshots back to `mcp_tasks`; expired leases are the restart-recovery mechanism, and a result returned after expiry or after a cancel request must be discarded even when the owner token still matches. The first cancel request fences an in-flight poll lease, while repeats preserve an active cancellation lease so they cannot issue concurrent remote cancels; cancellation backoff starts when the remote attempt finishes, so a slow timeout cannot consume the retry delay. Cancellation, polling, and notification batches isolate per-task exceptions; an unexpected cancellation/poll failure leaves that record's lease to expire, while notification failures release only the affected lease for retry. Input-required and terminal event snapshots are delivered by idempotent Agent runs and marked delivered only after run success; the trusted notification instruction stays outside the input boundary while the serialized remote event is framed as untrusted data. A busy-thread conflict is normalized back to the service boundary so the queued snapshot coalesces to the latest task event. A missing dispatched run becomes a failed delivery attempt, while transient run-store hydration errors stay distinguishable and retry the same lookup. The database is the source of truth; `ThreadState` receives only a bounded current-thread projection, and display names are neutralized at that model-state boundary. The installed process-local submitter is the source of truth for management-tool exposure; hot `mcp_tasks` edits take effect only after restart, and active skills must explicitly declare the list/cancel business tools.
- MCP notification failures use a consecutive counter separate from the idempotency-key `dispatch_attempt`, capped exponential backoff, latest-event rebuilding before a run launches, and a five-attempt budget before `dead_letter`. A permanently missing/mismatched target thread is dead-lettered immediately instead of being recreated or reclaimed. HTTP and Agent cancellation requests return after the durable cancel fence; the background loop alone owns the potentially slow remote call and retry schedule. The bounded notification error/count/status join poll and cancellation diagnostics in the task detail API and expanded card.
- MCP notification failures use a consecutive counter separate from the idempotency-key `dispatch_attempt`, capped exponential backoff, latest-event rebuilding before a run launches, and a five-attempt budget before `dead_letter`. A permanently missing/mismatched target thread is dead-lettered immediately instead of being recreated or reclaimed. HTTP and Agent cancellation requests return after the durable cancel fence; the background loop alone owns the potentially slow remote call and retry schedule. The HTTP cancel endpoint rejects requests with 503 when the loop is not running (`mcp_tasks_available` false, e.g. `mcp_tasks.enabled=false` with SQL persistence), so a cancellation is never acknowledged without a worker to perform it. The bounded notification error/count/status join poll and cancellation diagnostics in the task detail API and expanded card.
- `extensions_config.json` is written at runtime by the Gateway (`PUT`/`PATCH /api/mcp/config`, the MCP enable switch, skill updates), so the production compose mounts it read-write while `config.yaml` stays `:ro`; Helm copies its ConfigMap seed into a writable home-volume directory before Gateway starts. Every read-modify-write holds both `extensions_config_write_lock` and the sidecar advisory `extensions_config_file_lock`, because the process-local lock alone loses updates across workers. Docker mounts the compose file as its own mount point, and Linux refuses `rename()` over a mount point with `EBUSY` even when the mount is writable — so `atomic_write_extensions_config` keeps the temp-file-plus-rename path and falls back to an in-place overwrite only on `EBUSY`. That fallback is deliberately non-atomic (a crash mid-write truncates the file); it exists because the alternative is a write that can never succeed, and only its first occurrence per target is logged at warning level. Any other `errno` still propagates. Pinned by `tests/test_compose_extensions_config_writable.py`, `tests/test_extensions_config_atomic_write.py`, and `tests/test_helm_extensions_config_writable.py`.
- Scheduled-task dispatch enforces "at most one active run per task when `overlap_policy=skip`" at the DB layer via the partial unique index `uq_scheduled_task_run_active` (`scheduled_task_runs.task_id WHERE status IN ('queued','running')`). `ScheduledTaskService.dispatch_task`'s `has_active_runs` check is a non-atomic fast path (its own session, separated from the `create()` insert by `await` points), so two concurrent dispatches — a manual `POST /scheduled-tasks/{id}/trigger` racing the poller, a double-click, or a client retry — can both pass it; the index is the atomic arbiter, and the losing `create` surfaces as `ActiveScheduledRunConflict` (translated from `IntegrityError` in the repository) and collapses to the same outcome as the fast path (manual → 409 conflict, scheduled → a `"skipped"` tombstone). The scheduled-skip tombstone is created directly as terminal `"skipped"` (not a transient `"queued"`) so it never occupies the active slot the pre-existing run still holds. Sibling of the `runs` table's `uq_runs_thread_active` (PR #4003), which keys on `thread_id` and so does not cover the default `fresh_thread_per_run` context where every dispatch gets a new thread. Index is status-only, not `overlap_policy`-conditional (the policy is fixed to `"skip"` in the MVP).

View File

@ -110,6 +110,12 @@ async def cancel_mcp_task(
) -> dict[str, Any]:
service = get_mcp_task_service(request)
user_id = await _current_user_id(request)
if not getattr(request.app.state, "mcp_tasks_available", False):
# The service exists whenever SQL persistence is configured, but the
# background loop that owns the remote cancel call only runs when
# mcp_tasks.enabled=true. Recording cancel_requested_at without a
# worker would acknowledge a cancellation nobody will ever perform.
raise HTTPException(status_code=503, detail="MCP task cancellation worker is not running")
record = await service.cancel_task(
task_id=task_id,
thread_id=thread_id,

View File

@ -161,6 +161,7 @@ async def test_cancel_uses_service_with_exact_user_and_thread_scope(monkeypatch)
service.cancel_task.return_value = _record(status="working", cancel_requested_at="2026-08-05T00:00:06+00:00")
request = _request(repo)
request.app.state.mcp_task_service = service
request.app.state.mcp_tasks_available = True
monkeypatch.setattr(mcp_tasks, "get_current_user", AsyncMock(return_value="user-1"))
response = await mcp_tasks.cancel_mcp_task.__wrapped__(
@ -176,3 +177,44 @@ async def test_cancel_uses_service_with_exact_user_and_thread_scope(monkeypatch)
)
assert response["status"] == "working"
assert response["cancel_requested"] is True
@pytest.mark.asyncio
async def test_cancel_rejected_when_worker_not_running(monkeypatch) -> None:
repo = FakeRepository([_record()])
service = AsyncMock()
service.tracking_degraded_after_errors = 3
request = _request(repo)
request.app.state.mcp_task_service = service
request.app.state.mcp_tasks_available = False
monkeypatch.setattr(mcp_tasks, "get_current_user", AsyncMock(return_value="user-1"))
with pytest.raises(HTTPException) as excinfo:
await mcp_tasks.cancel_mcp_task.__wrapped__(
thread_id="thread-1",
task_id="mcp-task-1",
request=request,
)
assert excinfo.value.status_code == 503
service.cancel_task.assert_not_awaited()
@pytest.mark.asyncio
async def test_cancel_rejected_when_availability_flag_missing(monkeypatch) -> None:
repo = FakeRepository([_record()])
service = AsyncMock()
service.tracking_degraded_after_errors = 3
request = _request(repo)
request.app.state.mcp_task_service = service
monkeypatch.setattr(mcp_tasks, "get_current_user", AsyncMock(return_value="user-1"))
with pytest.raises(HTTPException) as excinfo:
await mcp_tasks.cancel_mcp_task.__wrapped__(
thread_id="thread-1",
task_id="mcp-task-1",
request=request,
)
assert excinfo.value.status_code == 503
service.cancel_task.assert_not_awaited()