Skip to content

feat(admission): count workers still running after their caller was cancelled (#728, slice 2) - #903

Merged
github-actions[bot] merged 1 commit into
mainfrom
fix/728-abandoned-work-status
Nov 6, 2026
Merged

feat(admission): count workers still running after their caller was cancelled (#728, slice 2)#903
github-actions[bot] merged 1 commit into
mainfrom
fix/728-abandoned-work-status

Conversation

@cdeust

@cdeust cdeust commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Summary

Tranche 2 de #728 : rendre lisible l'état des workers qui continuent après l'annulation de leur appelant.

asyncio.to_thread ne peut pas arrêter un thread en cours. La tranche 1 (#885) garde donc le créneau d'admission jusqu'à la fin du worker et journalise son issue. Mais rien ne disait combien de ces workers tournaient encore, ni comment les autres avaient fini. Le nouveau module mcp_server/handlers/abandoned_work.py compte, par outil, les workers abandonnés encore en vol (in_flight) et l'issue de chacun de ceux qui ont fini (ended : completed / raised / cancelled). run_admitted enregistre l'abandon, et _log_abandoned enregistre l'issue et indique dans sa ligne de log combien de workers abandonnés de l'outil restent en cours.

Refs #728 (pas Closes : critère « Define and report in-flight mutation status » couvert seulement pour la définition et le log, voir plus bas)

Type of change

  • New feature (non-breaking change which adds functionality)

Test plan

  • tests_py/handlers/test_abandoned_work_status.py (nouveau, 4 tests, ordonnancement par threading.Event, aucun verdict ne dépend du temps écoulé) : un worker abandonné compte dans in_flight jusqu'à sa fin ; chaque issue est comptée à part ; un appelant qui attend n'est jamais compté ; deux issues identiques s'additionnent, et le log décompte (1 puis 0 encore en cours). Rouges sans le changement de admission.py (3 sur 4 échouent ; le 4e vérifie qu'un appelant qui attend n'est pas compté).
  • tests_py/handlers/test_admission_cancellation.py : les messages de log attendus portent le décompte.
  • pytest tests_py/handlers tests_py/server tests_py/invariants/test_I10_pool_capacity.py : 1848 passés, 82 ignorés (PostgreSQL absent ici).
  • Gate de mutation ADR-1122 lancé en local (GITHUB_BASE_REF=main python scripts/ci_mutation_diff.py, mutmut 3.8.1) : 2 fichiers, 0 échec, 0 non testé. Un premier essai avait trouvé un survivant dans record_ended (deux issues identiques jamais testées) ; le test same_outcome_adds_up l'a tué.
  • ruff check et ruff format --check sur les fichiers modifiés : OK. python scripts/check_craftsmanship.py : OK.

Audit notes

  • Pas de modification de memory_stats dans cette PR. Le premier essai y ajoutait abandoned_work à la réponse, mais modifier handler met toute la fonction dans le périmètre du gate de mutation, et 58 mutants préexistants y survivent (champs de la réponse non vérifiés par test_memory_stats.py). Cette exposition client fait l'objet d'une tranche à part.
  • Non vérifié ici : l'annulation MCP réelle (déconnexion du client) avec des stores isolés, critère 4 de l'issue.

Breaking changes

Aucun. Seul changement visible : la ligne de log WARNING d'un worker abandonné se termine désormais par (N abandoned still running).

Generated by Claude Code

@claude
feat(admission): count workers still running after their caller was c…
277390b 
…ancelled (#728, slice 2)

A cancelled tool call leaves its asyncio.to_thread worker running; slice 1
(#885) kept its admission slot and logged its outcome, but nothing said how
many such workers were still running or how the ended ones did.

handlers/abandoned_work counts, per tool, the abandoned workers in flight and
the outcome of each one that ended (completed / raised / cancelled).
run_admitted records the abandonment, and _log_abandoned records the outcome
and names in its log line how many abandoned workers of the tool remain.

Refs #728
@cdeust cdeust mentioned this pull request Nov 6, 2026
Closed
5 tasks
@cdeust
cdeust marked this pull request as ready for review November 6, 2026 15:11
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-06T15:12:08.897602Z 277390b Draft marked ready
🔒 Security Review ✅ Completed 2026-11-06T15:13:53.159212Z 277390b Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Review of PR #903: approve, no Blockers

  • Gather the diff and context
  • Run five foreground reviews (quality, security, performance, tests, docs)
  • Check the findings and post the summary with gh pr comment

None of the five reviews found a Blocker. The summary, with 3 Notes and no action needed before merge, is here.

The previous Pipeline run on this PR passed, and a new one is in progress. I couldn't run tests, ruff or the craftsmanship gate in this job because those commands need approval.

Verdict: approve

@claude

claude Bot commented Nov 6, 2026

Copy link
Copy Markdown

Review 1/3 (ADR-1119): PR #903

Five foreground reviews ran: quality, security, performance, tests and docs. None found a Blocker.

  • Security: tool_name is always a hard-coded registry string, so the counter dicts are capped at the registered tools. The log only takes fixed arguments, so log injection isn't possible.
  • Layers: the only new import is handlers → handlers.
  • Size limits: files and methods are within 300/40 lines.
  • Constants: _OUTCOMES and _LOCK aren't numeric literals, so the gate doesn't check them for # source:.
  • Counting: record_abandoned and record_ended stay balanced on every path traced, including a worker already done at cancel time and loop teardown.
  • Test ordering: worker discovery through all_tasks() and the callback order asyncio.wait depends on are deterministic.
  • CI: the previous Pipeline run on this PR passed. A new run is in progress.
  • Not run: I couldn't run tests or linters locally in this job.

Notes, no action before merge

  • admission.py:96-99,113: any CancelledError from shield counts as abandoned, including when the worker task itself is cancelled (loop teardown). The cancelled outcome then takes in_flight down while the thread may still run, so the "still running" count is low at teardown.
  • test_admission_cancellation.py and tests_py/server/test_safe_handler_cancellation.py: their fixtures never call abandoned_work.reset(), and the latter can leave detect_gaps workers pending. The process-wide registry carries over between tests, so a later snapshot() assertion could flake.
  • test_abandoned_work_status.py:75,90,118: worker bodies use release.wait(_GUARD) without an assert. A broken ordering would time out after 5 s and still count as completed, so assert release.wait(_GUARD) would make that fail loudly.

Verdict: approve

@github-actions
github-actions Bot merged commit 540df96 into main Nov 6, 2026
32 checks passed
@cdeust cdeust mentioned this pull request Nov 6, 2026
Merged
4 of 5 tasks
cdeust pushed a commit that referenced this pull request Nov 6, 2026
@claude
Merge origin/main into fix/728-abandoned-worker-report
ed0c4c1 
#903 (540df96) brought abandoned_work, the same registry as worker_ledger.
Keep main's admission.py, drop worker_ledger and its tests, and have
memory_stats return abandoned_work.snapshot() as abandoned_workers.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_018kyZYtgfAGUYWDSLAfQPUp
This was referenced Nov 7, 2026
Merged
Merged
github-actions Bot pushed a commit that referenced this pull request Nov 7, 2026
@cdeust @claude
docs(admission): ADR-1125, a cancelled tool call's worker runs to com…
574a3fb 
…pletion (#728, slice 5) (#946)

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

Issue #728 asks to keep work counted "until completion or a documented
cooperative cancellation boundary". Slices 1-4 (#885, #903, #902, #936,
#943) implement and prove the first: the slot is released by the worker's
done callback, the caller awaits through asyncio.shield, abandoned workers
are reported in memory_stats.abandoned_workers. Nothing forwards the
cancellation into the worker thread, so no cooperative boundary exists.

ADR-1125 records those semantics, amends ADR-0329 (the slot belongs to the
worker), states what a handler that wants to stop early must specify, and
lists the tests that hold each point. Mirror regenerated with
wiki_decision_mirror.generate_mirror.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_018kyZYs8nydviS39BjwT88E

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

---------

Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cdeust @claude