Skip to content

fix(v1): drain EnvClient cancel tasks by removal so close() cannot spin - #2518

Merged
xeophon merged 1 commit into
mainfrom
fix/env-client-close-spin
Sep 4, 2026
Merged

xeophon merged 1 commit into
mainfrom
fix/env-client-close-spin

Conversation

@willccbb

@willccbb willccbb commented Sep 3, 2026 •

Copy link
Copy Markdown
Member

Symptom

In served mode (the default), Ctrl-C with rollouts in flight leaves the eval leader alive at 100% CPU after it prints interrupted — cleaning up, please wait...; only kill -9 ends it. A supervisor's SIGTERM to the whole process group does the same. Faulthandler shows the leader inside EnvClient.close(), called from _server's teardown under asyncio.run's _cancel_all_tasks.

Cause

_cancel_all_tasks cancels the in-flight client.run() calls; each one schedules a fire-and-forget _send_cancel task and parks it in _cancel_tasks with set.discard as its done callback. By the time close() runs, those sends have finished, and their discard callbacks are queued on the loop but have not run.

close() drained the set with while self._cancel_tasks: await asyncio.gather(*self._cancel_tasks, return_exceptions=True). Since CPython 3.12, gather() over futures that are all already done completes eagerly inside gather() itself, so the await returns without yielding to the loop. The queued discard callbacks never run, the set never empties, and the loop spins.

A stdlib-only version of the loop (three finished tasks, discard done-callbacks, the same while / gather drain) drains on 3.11.10 and spins on 3.12.13, 3.13.14 and 3.14.7.

Fix

close() pops each task from the set before awaiting it, so the set shrinks by removal rather than by callback. Cancels scheduled while a pending one is awaited are still picked up by the while. The discard callback stays for the normal path, where it releases a finished cancel's reference during the run.

Measurement

A served-mode eval of the echo-v1 fixture (null harness, subprocess runtime) against a local OpenAI-compatible stub that never answers, so both rollouts sit in EnvClient._request, then one signal. Python 3.13.14, macOS.

signal before after
SIGINT to the leader alive 60 s later at 98-100% CPU, until the driver aborted it exit 130 in 0.8 s, nothing left in the process group
SIGTERM to the process group alive 60 s later at 100% CPU, until the driver aborted it exit 130 in 0.8 s, nothing left in the process group

Test

test_env_client_close_drains_finished_cancels seeds an EnvClient with finished _send_cancel tasks in the state above and calls close(). It runs the scenario in a child interpreter with a subprocess timeout: the old close() never yields to the loop, so no in-loop timeout (asyncio.wait_for) can bound it. Fails on main under 3.12+ (killed at 30 s), passes with this change in about a second.

Related issues

#1991, #1992 and #1994 describe the pool's own shutdown of its workers: a hard-killed worker orphaning its subprocess, the broker keeping a dead worker's request pending, remote resources a killed worker cannot release. This is a different mechanism. The pool and its workers already exit when the leader dies (the death pipe in _arm_teardown); here it is the leader itself that cannot exit.

🤖 Generated with Claude Code


Note

Medium Risk
Touches eval teardown and ZMQ client shutdown on interrupt; behavior change is narrow but on a critical exit path used by served-mode eval.

Overview
Fixes served-mode shutdown where the eval leader could sit at 100% CPU inside EnvClient.close() after SIGINT/SIGTERM (Python 3.12+), because draining fire-and-forget _send_cancel tasks with asyncio.gather() on already-finished tasks completes without yielding, so discard done-callbacks never run and _cancel_tasks never empties.

EnvClient.close() now pops each task from _cancel_tasks and awaits it individually, shrinking the set by removal instead of relying on callbacks. The existing discard callback remains for the normal in-run path.

Adds test_env_client_close_drains_finished_cancels, which reproduces finished cancel tasks with queued callbacks in a child process with a 30s subprocess timeout (in-loop timeouts cannot bound a non-yielding spin).

Reviewed by Cursor Bugbot for commit 14abddd. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix EnvClient.close spinning when cancel tasks are already finished

  • Replaces the gather-based drain in EnvClient.close with a loop that removes each tracked task from the set before awaiting it, so finished cancel tasks don't keep the set non-empty
  • Adds a subprocess-based regression test in test_e2e.py that registers finished cancel tasks and asserts close() completes within 30 seconds
  • Risk: non-CancelledError exceptions from an individual cancel task now propagate from close, whereas the previous gather call suppressed them

Macroscope summarized 14abddd.

Since CPython 3.12, asyncio.gather() over tasks that have all already
finished completes eagerly, so awaiting it never yields to the loop.
EnvClient.close() drained `_cancel_tasks` by re-gathering the set until
the tasks' `discard` done-callbacks emptied it; those callbacks sit in the
ready queue behind the coroutine and never run, so the loop spins at 100%
CPU inside asyncio.run's _cancel_all_tasks. Ctrl-C with rollouts in flight
in served mode hit this, as did a supervisor's SIGTERM to the process
group; 3.11 drained.

close() now pops each task before awaiting it, so the set shrinks whether
or not the loop ever runs the callbacks. The regression test runs the
finished-tasks state in a child interpreter: a spin that never yields
never lets an in-loop timeout fire either.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@macroscopeapp

macroscopeapp Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 14abddd

Macroscope's review found this PR approvable — This is a focused shutdown bug fix that replaces a callback-dependent task drain with explicit removal and awaiting, plus a regression test for the event-loop spin. Its impact is confined to EnvClient teardown and introduces no schema, security, deployment, or user-facing capability changes.

You can add or adjust custom eligibility rules. Learn more.

@xeophon
xeophon merged commit 15a73cb into main Sep 4, 2026
13 checks passed
@xeophon
xeophon deleted the fix/env-client-close-spin branch September 4, 2026 10:25
kcoopermiller added a commit that referenced this pull request Sep 4, 2026
Resolves `verifiers/v1/serve/client.py` in main's favour: #2518 fixes the
same eager-gather spin in `EnvClient.close()` that this branch had patched
(pop each cancel task before awaiting it), so the branch's hunk is dropped.
`uv lock` re-run on the merged tree; prime-runs stays 0.1.1, prime-traces
0.0.3, and main's Python 3.14 bump carries through.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EYdQmKAVnGteT1tkimJb3g
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants