fix(mcp): stop swallowing worker task cancellation

[hktitof]

Summary

Cancelling the parallel mode worker task did not stop it. _ServerWorker._run wraps command execution in except BaseException and asyncio.CancelledError is a BaseException, so an external cancel got recorded on the command future and the loop went back to await self._queue.get() with the task stuck in the cancelling state. is_done stayed false and anything awaiting the task, including interpreter shutdown, hung

The fix snapshots Task.cancelling() before each command and re-raises CancelledError only when that count grew while the command ran, so a cancel delivered from outside ends the task while a server raising CancelledError on its own stays recoverable through the command future. That keeps the contract the existing CancelledServer tests pin, and on Python 3.10, which has no Task.cancelling(), behavior stays as before

One interaction to flag: #5011 forwards caller cancels into the worker task for connect commands and needs the worker to survive them. If that PR merges first, its cancelled_by_caller flag can skip the new re-raise branch. This diff is written against main without that change and passes the existing cancellation tests as is

Test plan

• new test_worker_task_cancellation_stops_worker_and_fails_caller cancels the worker task while connect runs and asserts the caller sees CancelledError and the task ends cancelled. on main it fails with AssertionError: the worker task survived cancellation and is still looping, with the fix it passes
• new test_worker_survives_server_raised_cancelled_error_and_serves_next_command pins the other side, a CancelledError the server raises on its own reaches the caller through the future and the worker serves the next command and cleanup
• tests/mcp/ 475 passed, 24 skipped, the skips are live server integration. the test_server_errors.py file needs httpx, an optional dep missing in my sandbox until installed, unrelated to this change
• ruff check and ruff format --check clean, mypy src/agents/mcp/manager.py clean under strict mode
• make format, make lint and make typecheck pass, and make tests splits as 9606 passed in the parallel run with 14 pre-existing failures plus 77 passed in the serial run. the 14 are unrelated to this change: running the same commands on a pristine checkout of main gives the identical failures, they live in the repo's own code change verification runner tests and this diff never touches that file

Issue number

Closes #5054

Checks

• i've added new tests, if relevant
• i've run .agents/skills/code-change-verification/scripts/run.sh
• i've confirmed all verification steps pass

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2df0af5e0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[anishmehta24]

One gap: if the worker task is cancelled while a command is in flight, anything still sitting in the queue is never settled, so a caller awaiting a queued cleanup() hangs forever. Draining the queue and cancelling those futures on the way out (a finally around the loop) covers it.

Repro: start a connect that blocks, queue a cleanup(), cancel worker._task, then await the cleanup future with a timeout.

[Rehansanjay]

Checked this against the two cases from #5054, at 025294e2ce.

The split between the two kinds of CancelledError holds. A server that raises CancelledError on its own gets it back through the command future, and the worker keeps serving the next command. That is the case a plain except asyncio.CancelledError: ...; raise gets wrong: with that version the same test ends the worker.

The gap @anishmehta24 raised reproduces. With a connect blocking and a cleanup() queued behind it, cancelling the worker task ends the worker, and the queued cleanup() future is never settled (a 2s wait_for times out and server.cleanup is never called). On main the same sequence completes, only because the worker survives the cancel. Nothing in manager.py cancels a worker task itself, so this needs the cancel to come from outside, which is the case #5054 is about. Settling whatever is still queued before re-raising would close it, for example draining self._queue and cancelling each pending command.future.

Repro

import asyncio
from typing import Any, cast

import pytest

from agents.mcp import manager as manager_module


class BlockingConnectServer:
    def __init__(self) -> None:
        self.connect_started = asyncio.Event()
        self.cleanup_calls = 0

    async def connect(self) -> None:
        self.connect_started.set()
        await asyncio.sleep(3600)

    async def cleanup(self) -> None:
        self.cleanup_calls += 1


@pytest.mark.asyncio
async def test_queued_cleanup_is_settled_when_worker_task_is_cancelled() -> None:
    server = BlockingConnectServer()
    worker = manager_module._ServerWorker(cast(Any, server))

    connect_caller = asyncio.create_task(worker.connect(timeout_seconds=None))
    await asyncio.wait_for(server.connect_started.wait(), timeout=2)
    cleanup_caller = asyncio.create_task(worker.cleanup(timeout_seconds=None))
    await asyncio.sleep(0.05)

    worker._task.cancel()

    # On 025294e2ce this times out: the queued cleanup future is never settled.
    await asyncio.wait_for(asyncio.shield(cleanup_caller), timeout=2)

[hktitof]

thanks for checking this and for the repro steps @anishmehta24 @Rehansanjay, the worker loop now drains the queue when it exits so queued callers get CancelledError instead of hanging, adopted in 00f613b

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 00f613b888

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[jbeckwith-oai]

Requesting changes for the two inline findings: cancellation can abandon cleanup of a partially acquired server connection, and direct worker-task construction bypasses the configured asyncio task factory.

Reviewed the complete diff against main at 5f9899d, including independent reviews and controlled baseline-versus-PR lifecycle probes. The latest queue-draining change fixes the queued-waiter hang, but it does not address the remaining resource-cleanup issue.

Validation: all 83 manager tests passed on both Python 3.10 and Python 3.12 using the exact PR module and test snapshots. The broader MCP suite was interrupted and remains unverified.

[hktitof]

thanks for the review @jbeckwith-oai, both findings fixed in 87938c6, the worker now runs the server cleanup in the same task before re-raising the cancellation so a partially acquired connection gets released, and worker creation goes through the loop's task factory with the cancel counting kept on the worker side so 3.10 still works

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87938c6429

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6590009367

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[jbeckwith-oai]

Reviewed a8c0467238d6b618ec1f5f119ea5a05bb4659adb in its pinned review worktree.

The external cancellation hang is real. The current head separates task cancellation from server-raised CancelledError, performs emergency cleanup in the owning task with the manager cleanup timeout, settles queued callers, and preserves the configured task factory. The earlier cleanup and task-factory findings are addressed. Integration with #5011 will need to preserve this cancellation distinction.

Validation: complete-diff and supported-path desk review, two independent fresh-context review rounds, and git diff --check. Recorded hosted checks are green. No local runtime tests were executed.

[seratch]

Thanks for addressing the earlier cleanup, task-factory, and timeout findings. One lifecycle issue remains: when emergency cleanup raises or times out, its failure is swallowed without recording a cleanup outcome. The done callback then removes the worker, and a later reconnect() can start another connection without successful cleanup of the previous one.

Please retain the emergency-cleanup failure through the existing cleanup-result mechanism while still delivering the original cancellation to the connect caller. Add a manager-level regression covering cancellation during partial startup, failed or timed-out emergency cleanup, and subsequent reconnect, asserting that the failure remains observable and no second connection starts. This preserves the existing behavior that reconnect does not proceed after cleanup failure.