test(admission): MCP client cancel and disconnect keep the worker's slot and land its write (#728, slice 3) - #936

Claude finished @cdeust's task in 3m 16s —— View job

PR #936 review (review 1 of 3)

• Read the diff
• Ran five reviewer agents (code quality, docs accuracy, performance, security, test coverage)
• Checked their findings against mcp_server/handlers/admission.py and mcp_server/tool_error_handler.py
• Posted the verdict

I traced the Blocker below in the code but could not reproduce it. This runner can't install the MCP SDK or run Python, and the gh commands, including gh pr comment, need approval. So this review is posted in this job comment instead of a separate gh pr comment.

Security found nothing. The test-coverage reviewer confirmed two things. First, both tests really do cancel the call on the server side: record_abandoned is only reached from run_admitted's except CancelledError. Second, the mutant you described, releasing the slot at the cancel, makes _assert_abandoned_and_held fail.

Blocker

tests_py/server/test_mcp_protocol_cancellation.py:104-111: _wait_slot_returned waits a fixed number of loop turns, not until the 5 s guard, so the tests can fail at random on CI.

• writer.ended is set in the handler's own finally (:83), before the worker thread has finished.
• After that, the thread still has to:
  • finish run_until_complete
  • exit handler_transaction_scope
  • run loop.close() and metrics.Timer.__exit__ (tool_error_handler.py:87-110)
  • hand its result back to the main loop.
• Only then does the to_thread future complete and _release_slot run (admission.py:93-94,103).
• _wait_slot_returned gives this 100 turns of asyncio.sleep(0), about a millisecond. A thread waiting for the GIL can be held off for up to the 5 ms switch interval, so on a loaded runner the worker can miss that window. The test then fails with slot not returned even though the code under test is correct.
• This also contradicts the PR's own rule that "the only bound is a 5 s deadlock guard".
• Fix: bound this wait by time, the same way _until_abandoned is:

async def _until_slot_returned() -> None:
    sem = _get_semaphore(_TOOL)
    while sem._value != current_budget(_TOOL):  # noqa: SLF001
        await asyncio.sleep(0)

# in _finish_worker:
await asyncio.wait_for(_until_slot_returned(), _GUARD)

The completed == 1 check right after it stays sound: _release_slot and _log_abandoned run in the same callback batch when the worker finishes.

Fix this →

Notes, no action before merge

• Only the disconnect test checks that "teardown neither waits on nor kills the worker". In the cancel test the worker has already ended (:143) before the client exits (:150), so the docstring (:15) and CHANGELOG should say this for one path, not "both".
• "a started write is not rolled back by a cancel" (:13-14, CHANGELOG): at the cancel the handler is blocked before it writes (rows() == [] at :141). What the test actually shows is that a running handler still makes its write after the cancel.
• # noqa: SLF001 (:108, :117) has no effect, because ruff does not select SLF. The private Semaphore._value read is the real coupling.

Verdict: block

Claude finished @cdeust's task in 1m 56s —— View job

PR #936 review (review 2 of 3)

• Checked the review 1 Blocker against b1120ca
• Ran five reviewer agents (code quality, docs accuracy, performance, security, test coverage) on the 354ce42..b1120ca delta
• Checked their findings against mcp_server/handlers/admission.py, abandoned_work.py and tool_error_handler.py
• Posted the verdict

This is posted in the job comment, the same way as review 1, rather than with gh pr comment. The MCP SDK is not installed on this runner, so I could not run the tests or check Client(mode="legacy") against SDK 2.3.0 myself. CI (Pipeline) was still running when I posted this.

Review 1 Blocker: fixed

The fixed 100-turn wait in _wait_slot_returned is gone. Its replacement, _until(condition) (test_mcp_protocol_cancellation.py:111-123), polls with asyncio.wait_for(..., _GUARD). _finish_worker now waits for the slot to come back and then for in_flight to empty, before it asserts completed == 1.

That order matches the code: _release_slot is registered before _log_abandoned (admission.py:94 vs :99). The new _POLL constant has a # source: comment.

Codex's second point is also addressed: both clients now use mode="legacy", so the call goes through the in-memory transport and JSON-RPC instead of DirectDispatcher.

The new code

• Mutant still caught. A mutant that releases the slot at the cancel still fails _assert_abandoned_and_held. The held-slot check (:159, :187) runs before release is set (:138). A double release leaves _value at budget + 1, so _slot_returned times out.
• No regression hidden. The extra in_flight wait does not hide a lost record (it times out), a double record_ended (a -1 count stays in the snapshot), or a wrong outcome (completed == 1 fails).
• Security and performance: nothing found. The SQL uses parameters, the store is under tmp_path, and every wait has a time limit.

Notes, no action before merge

• :18 and the CHANGELOG still say the teardown check holds for "both" paths, but only the disconnect test checks it.
• :20 "never by elapsed time" is now only partly true, because orderings also depend on the condition polls in _until.
• :143 assert snap["in_flight"] == {} can no longer fail, because :141 already waited for that exact condition.

Verdict: approve

Revue Claude 1/3 (verdict block) : le Blocker (attente bornée par 100 tours de boucle au lieu de _GUARD) est celui que Codex a aussi relevé ; il est corrigé dans b1120ca (_until(condition) borné par asyncio.wait_for(..., _GUARD), 20/20 exécutions vertes).

Les trois notes sont justes et corrigées dans 0205b08 :

• « teardown ne bloque pas » n'est prouvé que par le test de déconnexion ; la docstring et le CHANGELOG le disent maintenant pour ce seul test (le test d'annulation prouve, lui, que la session sert un nouvel appel).
• « une écriture commencée n'est pas annulée » devient ce que le test montre : un handler encore en cours à l'annulation, bloqué avant d'écrire, fait son écriture après que le client a cessé d'écouter.
• les # noqa: SLF001 sans effet (ruff ne sélectionne pas SLF) sont retirés.

Generated by Claude Code

Claude finished @cdeust's task in 2m 56s —— View job

PR #936 review (review 2 of 3)

• Check the review 1 Blocker against b1120ca / 0205b08
• Run five reviewer agents on the 354ce42..0205b08 delta (code quality, docs accuracy, performance, security, test coverage)
• Check their findings against admission.py, abandoned_work.py and tool_error_handler.py
• Post the verdict

I couldn't run anything on this runner: Python and gh both need approval here, and the MCP SDK isn't installed. So the checks below come from reading the code, and the verdict is posted in this job comment instead of through gh pr comment.

Review 1 Blocker: fixed

_wait_slot_returned, which counted 100 loop turns, is replaced by _until(condition) (tests_py/server/test_mcp_protocol_cancellation.py:115-127). It polls every 1 ms and gives up after _GUARD via asyncio.wait_for. All three loop-side waits now use it: the slot return, the abandoned outcome and the abandoned registration.

_finish_worker (:144-148) has no race. record_ended lowers in_flight and records the completed outcome under the same lock (abandoned_work.py:41-45), so once in_flight is empty, completed is already recorded.

The Codex P2 finding (the default "auto" mode never crosses JSON-RPC) is addressed with mode="legacy" in both tests. I couldn't check this against the SDK source, but it can't make a test pass when it should fail: if no cancel reached the server, _until(in_flight) at :162 and :190 would time out. The slot-release mutant still fails _assert_abandoned_and_held in both tests.

The docs notes from review 1 are fixed too. The docstring and CHANGELOG now say which test covers the session-survives check and which covers teardown, and they describe the blocked-before-write case accurately. The # noqa: SLF001 comments, which had no effect, are gone. The new _POLL constant has a # source: comment. The file is 198 lines.

New Blockers

None.

Notes, no action before merge

• :170-171: writer.started.clear() / writer.ended.clear() do nothing, since nothing waits on those events afterwards.
• :147: assert snap["in_flight"] == {} can't fail, because the _until at :145 already waited for exactly that.
• :186-187: the comment says the server cancels on read EOF. The test doesn't show that: the cancel could also come from the client task group being torn down. Either way the handler is cancelled, which is what the test checks.

Verdict: approve