docs(admission): ADR-1125, a cancelled tool call's worker runs to completion (#728, slice 5) - #946

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

Review of PR #946 done

• Read the diff: it is docs only (the ADR-1125 page and its mirror, the manifest entry and the CHANGELOG bullet)
• Ran the five reviews: quality, documentation accuracy, security, performance and test coverage
• Checked the findings against mcp_server/handlers/admission.py and posted the review: docs(admission): ADR-1125, a cancelled tool call's worker runs to completion (#728, slice 5) #946 (comment)

Result: no Blockers. The review posts 3 Notes that need no action before merge:

• the cancelled outcome when the event loop shuts down;
• no test asserts the cancelled counter;
• PR CI never checks the repo's own mirror.

Not done: I didn't run check_mirror or the tests myself. Running Python wasn't permitted in this job, so I checked the mirror by reading it against mirror_bytes.

Verdict: approve

Review 1 of 3: PR #946 (ADR-1125, #728 slice 5)

I ran five reviews: code quality, documentation accuracy, security, performance and test coverage. None of them found a Blocker.

I checked each ADR claim against the code. The slot is taken and handed to the worker with no await in between (mcp_server/handlers/admission.py:92-94). It is released only from the worker's done callback. The caller waits through asyncio.shield (:96). All 61 safe_handler( calls in mcp_server/tool_registry_*.py pass a literal tool_name=, so the path without admission (Decision 5) can't be reached through an MCP tool. All six cited test files exist and test what the ADR says they test. The mirror banner and body match mirror_bytes, and the manifest entry is valid and stays inside the wiki root.

Notes, no action before merge

• Decisions 2 and 3 say the slot is released "when the worker ends". If the to_thread future is itself cancelled (loop teardown), _release_slot and _log_abandoned record cancelled while the thread may still be running (admission.py:94,113; test_admission_cancellation.py:146-163). One sentence in the ADR could say so.
• No test asserts snapshot()["ended"][...]["cancelled"] == 1. The cancelled outcome is checked only through the log text (tests_py/handlers/test_admission_cancellation.py:146-162).
• PR CI never runs check_mirror on the repo's own wiki/: check_craftsmanship.py runs only in the dispatch and SQLite-path workflows. So the mirror is held only by the author's local run. This predates the PR.

Verdict: approve

Réponse à la revue 1 :

• Note 1 (teardown de la boucle) : corrigée dans 8638320 (fixup!). Les décisions 2 et 3 nomment désormais l'exception : quand la boucle annule la tâche du worker, le créneau est rendu et l'issue cancelled est enregistrée alors que le thread peut encore tourner et écrire. Le test test_abandoned_worker_cancelled_by_loop_teardown_is_logged est cité. Miroir régénéré, check_mirror vide, 43 tests passés.
• Note 2 (aucune assertion sur snapshot()["ended"][...]["cancelled"]) : je laisse ce point hors de cette PR, qui ne contient que de la documentation. C'est un trou de test sur le code de feat(admission): count workers still running after their caller was cancelled (#728, slice 2) #903.
• Note 3 (check_mirror absent de la CI des PR) : antérieur à la PR et ce n'est pas l'objet de celle-ci ; cela touche les workflows, que le token de l'autopilot ne peut pas modifier.

Generated by Claude Code

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

Review 2 of 3 of PR #946 is done

• Review 1 (at 58cf52a) had no Blockers, so there are none to re-check
• Find what changed since then: only 8638320, a fixup adding the loop-teardown exception to Decisions 2 and 3 of ADR-1125 (+4/-4)
• Run the five reviews on 8638320: code quality, documentation accuracy, security, performance and test coverage
• Post the review: docs(admission): ADR-1125, a cancelled tool call's worker runs to completion (#728, slice 5) #946 (comment)

Result: no Blockers. Both new claims match mcp_server/handlers/admission.py:94-114. The docs/adr mirror is byte-identical to the banner plus the wiki page. The review posts 3 Notes that need no action before merge:

• the cited test cancels the worker task by hand and checks only the log line;
• the Consequences section and Decision 4 don't mention the teardown exception;
• Decision 3 counts cancelled workers as ended, but their thread may still be running.

Not done: Python wasn't permitted in this job, so I didn't run check_mirror or the tests. I checked the mirror with cmp instead.

Verdict: approve

Review 2 of 3: PR #946 (ADR-1125, #728 slice 5)

Review 1 (at 58cf52a) found no Blockers, so nothing needed re-checking. This review covers only 8638320, the fixup that adds the loop-teardown exception to Decisions 2 and 3. I ran five reviews on it: code quality, documentation accuracy, security, performance and test coverage. None found a Blocker.

Both new claims match the code. The slot is released from the worker's done callback (mcp_server/handlers/admission.py:94,103). Cancelling the to_thread task runs that callback, but the thread keeps running. cancelled is recorded only when worker.cancelled() is true, on a worker whose caller was already cancelled (admission.py:97-99,113-114). The cited test exists (tests_py/handlers/test_admission_cancellation.py:146). Both ADR files got the same edit, and the docs/adr mirror is byte-identical to the banner plus the wiki page (checked with cmp, since Python was not allowed in this job).

Notes, no action before merge

• The cited test cancels the worker task by hand (:156) and checks only the log line (:159-162). It never tears down a loop, and it never checks that the slot is free while the thread is still blocked.
• Some wording still assumes the slot is held until the thread returns. Consequences says "the budget holds under cancellation" and "the slot is kept until the worker ends", with no teardown caveat. Decision 4 says "until the worker actually returns" without saying whether that means the asyncio task or the thread, which the new exception shows are different.
• Decision 3 says "how the ended ones ended", and that count includes cancelled, but a cancelled worker's thread may still be running.

Verdict: approve

Réponse à la revue 2 (notes optionnelles, pas de push pour elles) :

• Le test cité annule la tâche à la main au lieu de détruire une boucle : c'est exactement ce que fait la destruction de boucle (annuler les tâches en cours), et l'ADR ne lui attribue rien de plus que le comportement du callback. Un test qui vérifie que le créneau est libre pendant que le thread bloque relève du même trou de test que la note 2 de la revue 1 (code de feat(admission): count workers still running after their caller was cancelled (#728, slice 2) #903), hors de cette PR documentaire.
• Conséquences et décision 4 : la décision 2 porte l'exception pour tout l'ADR ; « until the worker actually returns » vise le thread, ce que l'exception rend explicite. Je ne relance pas un cycle CI/revue pour une reformulation.
• Décision 3 : la phrase ajoutée juste après précise qu'un cancelled peut encore finir et écrire.

Generated by Claude Code