Skip to content

fix(tools): close cached transports when async worker threads exit - #135482

Draft
iainlane wants to merge 3 commits into
NousResearch:mainfrom
iainlane:codex/worker-loop-lifetime-20261009
Draft

iainlane wants to merge 3 commits into
NousResearch:mainfrom
iainlane:codex/worker-loop-lifetime-20261009

Conversation

@iainlane

@iainlane iainlane commented Oct 9, 2026 •

Copy link
Copy Markdown

This core PR is independent and can merge at any time.

Executor worker threads reuse persistent event loops between tool calls. When those threads exit, the previous loop cache leaves the loop, selector sockets and cached async clients open. This follow-up gives each worker an asyncio.Runner owner whose lifetime follows the thread-local cache. Both persistent executor workers and temporary workers used inside an already-running loop use the same teardown. Teardown cancels remaining tasks, finishes async generators, closes cached clients on their owning loop, then closes the runner. Timeout cancellation stays inside the worker loop, and the caller retains nonblocking executor shutdown. Clients on other live loops remain open.

The first commit extracts the existing cached-client teardown functions into a topical sibling because the auxiliary-client module already exceeds the file-growth limit. The second commit adds worker lifetime ownership and the regression. The third commit preserves Riccardo Vecchi’s contributed teardown test and adds coverage for cancellation of background tasks that await real default-executor threads. The original worker-loop implementation predates the adopted feature contributions, so this is a separate core repair.

Validation used actual executor threads and an SDK HTTP client connected to a local HTTP server. Two assertions fail on unchanged persistent-worker production. The parameterised running-loop mode separately reproduced a closed loop with an open cached client and an unfinished generator before the sibling correction. The final three complete files pass all 256 tests with resource, runtime, unraisable and thread warnings treated as errors and automatic retries disabled. The regression checks transport reuse, generator cleanup before client closure, worker resource closure and an unaffected main-loop client in both worker modes. The existing fake timeout executor also closes its deliberately unsubmitted test coroutine.

The blocking code-health check passes. Scoped Ruff passes; the full type comparison retains the same 95 inherited findings with no additions or removals. Three existing files retain their formatter debt; the new sibling and changed scopes are formatted. This evidence covers the focused worker and auxiliary-client lifecycle, without a Matrix service or global validation claim.

Contributed task and executor teardown coverage

I adopted Riccardo Vecchi’s test-only contribution, preserving its author, author date and original commit message. The parameterised test exercises both persistent workers and the bridge inside an already-running loop. It checks that teardown cancels a background task, runs the task’s cleanup, completes its real default-executor work and closes the worker loop. Event synchronisation and cleanup release the test’s own executor thread even on the unfixed implementation.

The test uses the default-executor shutdown API supported by the project’s minimum Python 3.11 version and follows the project formatter. On the frozen unfixed base, the exact adapted test gives one failure and one pass. On the reviewed implementation, both cases pass through the canonical runner with zero retries and no complete-child shutdown resource warnings. Its affected-file types, Ruff and formatting checks are all clear. Production code and the two previously reviewed signed commits are unchanged.

The earlier 256 passing assertions in three complete files remain authenticated evidence for unchanged source. The new complete file adds two passing cases, giving 258 source-qualified assertions across four files; this is not a new combined four-file invocation. These local macOS/Python 3.14 results do not establish hosted CI, Windows, Linux, native Matrix service behaviour, RSS or non-cooperative thread cancellation.

Cached auxiliary-client teardown is mixed into the large routing module.
The worker-loop lifetime correction needs to await only the transports
owned by a terminating worker, without expanding that routing module.

Move the three transport-close functions into a lifecycle sibling and
import the close function at its existing callers. Their full function
ASTs and the remaining routing-module statements are unchanged.
Tool dispatch keeps one event loop per executor thread so cached async
clients can reuse their transports between calls. The thread-local
cache stored only the loop, so ending an executor left the loop and its
selector sockets open. Auxiliary clients also remained cached after
their owner thread exited. Temporary workers used inside an already
running event loop also closed their loops before finishing generators
and cached transports.

Give each worker a Runner owner with a finalizer attached to its
thread-local lifetime. Cancel remaining tasks and finish generators
before closing cached clients on that loop. Wait for their close tasks,
then let Runner shut down the executor and close the loop. Clients that
belong to other live loops remain cached and open.

The regression uses actual executor threads and SDK HTTP connections to
a local server. It checks reuse between calls, generator cleanup before
client closure, closed worker resources, and an unaffected main-thread
client in both worker modes. Timeout cancellation still runs inside the
worker loop, and executor shutdown remains nonblocking for the caller.
The existing fake timeout executor now also closes the test
coroutine which it deliberately never submits.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets labels Oct 9, 2026
@Riccardo-Vecchi

Copy link
Copy Markdown

I prepared complementary behavioral coverage for this PR's task/executor teardown, on top of head 90defd1af1:

1bd0b87718 — cover worker task and executor teardown

One new test, parametrized for persistent workers and the already-running-loop bridge, starts a real background asyncio task awaiting a real asyncio.to_thread operation. Its finally releases that native thread. After worker exit it asserts that the loop is closed, the task is completed/cancelled, both cleanup paths ran, and the default-executor thread is no longer alive. Synchronization uses events and executor shutdown, not a short sleep-based elapsed-time assertion. The test also reclaims its resources when run against unfixed code.

On current main at 14ec243c17, the exact test is 1 failed / 1 passed: persistent-worker teardown leaves the loop/task/cleanup unfinished and the executor thread alive; the running-loop case is a control that already passes. On this PR plus the unchanged test, both cases pass. This complements your existing cached-HTTP-client and async-generator checks; the fix remains yours, with no competing implementation or PR.

On the exact test commit, Linux / Python 3.14.7, using the isolated PM interpreter via HERMES_PYTHON:

scripts/run_tests.sh tests/test_model_tools_worker_shutdown.py tests/test_model_tools_async_bridge.py tests/agent/test_auxiliary_client.py tests/agent/test_async_httpx_del_neuter.py tests/agent/test_auxiliary_runtime_cache_key.py tests/agent/test_auxiliary_client_cache_eviction.py -j 2 --file-retries 0 -- -q -W error::ResourceWarning -W error::RuntimeWarning -W error::pytest.PytestUnraisableExceptionWarning -W error::pytest.PytestUnhandledThreadExceptionWarning
python scripts/check --commit HEAD

267 passed, 0 failed, with resource/runtime/unraisable/thread warnings treated as errors, retries disabled. 11 checks passed, zero code-health findings across the five changed files against the PR's merge-base 1744a19e0d. The later main commits through 827d3c5fdb only change the plugin catalog/contributor record, not these paths; this is not a claim of a tested merge/rebase.

Limits: no hosted CI, native Windows/macOS, full repository suite, production gateway, Matrix service, RSS measurement, or non-cooperative stuck-thread shutdown tested here. This test releases the executor operation in response to task cancellation; it does not assert that Python can forcibly stop arbitrary blocking work.

Prepared with OpenAI Codex assistance; no human review, maintainer approval, or merge claimed. The one test-only commit can be cherry-picked without changing your implementation or authorship.

iainlane pushed a commit to iainlane/hermes-agent that referenced this pull request Oct 9, 2026
Exercise the real async bridge with a background task and a native default-executor thread in persistent and running-loop modes. Complement PR NousResearch#135482 transport and async-generator coverage without replacing its implementation.

Prepared with OpenAI Codex assistance; no human review claimed.

Use the default-executor shutdown API supported by Python 3.11, which
is the minimum version for this project. The test releases its own
executor work before shutdown and verifies that the worker thread exits.
Exercise the real async bridge with a background task and a native default-executor thread in persistent and running-loop modes. Complement PR NousResearch#135482 transport and async-generator coverage without replacing its implementation.

Prepared with OpenAI Codex assistance; no human review claimed.

Use the default-executor shutdown API supported by Python 3.11, which
is the minimum version for this project. The test releases its own
executor work before shutdown and verifies that the worker thread exits.
@iainlane
iainlane force-pushed the codex/worker-loop-lifetime-20261009 branch from 35ebdc6 to a4027e0 Compare October 9, 2026 16:35
@Riccardo-Vecchi

Copy link
Copy Markdown

I independently checked the adopted test on Linux / PM-provisioned Python 3.14.7. The production delta from my offered 1bd0b8771834d64718999cb691cfc354a385470c is empty; only the contributed test's formatting and cleanup were adapted.

On 35ebdc6f1e4d4211150567232ed1f62bb61ecea4, one combined invocation of the same six-file set from my original comment gives 267 passed, 0 failed, 33.6 s, two workers and zero retries, with ResourceWarning, RuntimeWarning, unraisable and thread warnings treated as errors. scripts/check --commit HEAD passes all 11 checks, with zero blocking/advisory health findings against merge-base 1744a19e0df5.

While that run completed, the head changed to a4027e0661448650ff40cdd7cda0269c3127a9fc. I fetched it and verified both commits have the identical full tree a8dbcc8e3668ef21acbc661e1113d47645b14d43; git diff is empty. Thus these receipts cover the exact current source tree, not a claim that I reran the suite on the rewritten commit. Author/date and the AI-assistance disclosure remain preserved, with you as committer on the current head.

This adds an independent Linux receipt to your macOS validation. I did not rerun the adapted test's RED baseline today; its earlier RED on main and your adapted-baseline result remain separate evidence. No hosted CI, full repository suite, native Windows, live Matrix, RSS measurement or non-cooperative thread cancellation is implied. The task releases its own executor work cooperatively.

Disclosure: this follow-up and verification were prepared with OpenAI Codex assistance; no human review, maintainer approval or merge is claimed. Thanks for integrating the complementary coverage without duplicating the implementation.

@iainlane

iainlane commented Oct 9, 2026

Copy link
Copy Markdown
Author

I adopted your test as a separate commit, preserving your authorship, author date and original message. It adds coverage for pending-task cancellation and real default-executor teardown in both bridge modes. I adjusted the failure cleanup to use the API supported by the project’s minimum Python 3.11 version and applied the project formatter.

I reproduced one failure and one pass on our frozen unfixed base, then two passes on the reviewed fix with the same adapted test. The canonical runner used zero retries and the complete child output has no shutdown resource warnings. The new file also passes types, Ruff and formatting checks. The two reviewed production commits are unchanged. These are local macOS/Python 3.14 results; I am not claiming hosted CI or the other platform and service scopes from your comment.

Thank you for the additional behavioural coverage.

The adopted test is in commit a4027e06.

iainlane pushed a commit to iainlane/hermes-agent that referenced this pull request Oct 10, 2026
Exercise the real async bridge with a background task and a native default-executor thread in persistent and running-loop modes. Complement PR NousResearch#135482 transport and async-generator coverage without replacing its implementation.

Prepared with OpenAI Codex assistance; no human review claimed.

Use the default-executor shutdown API supported by Python 3.11, which
is the minimum version for this project. The test releases its own
executor work before shutdown and verifies that the worker thread exits.
iainlane pushed a commit to iainlane/hermes-agent that referenced this pull request Oct 10, 2026
Exercise the real async bridge with a background task and a native default-executor thread in persistent and running-loop modes. Complement PR NousResearch#135482 transport and async-generator coverage without replacing its implementation.

Prepared with OpenAI Codex assistance; no human review claimed.

Use the default-executor shutdown API supported by Python 3.11, which
is the minimum version for this project. The test releases its own
executor work before shutdown and verifies that the worker thread exits.
iainlane pushed a commit to iainlane/hermes-agent that referenced this pull request Oct 10, 2026
Exercise the real async bridge with a background task and a native default-executor thread in persistent and running-loop modes. Complement PR NousResearch#135482 transport and async-generator coverage without replacing its implementation.

Prepared with OpenAI Codex assistance; no human review claimed.

Use the default-executor shutdown API supported by Python 3.11, which
is the minimum version for this project. The test releases its own
executor work before shutdown and verifies that the worker thread exits.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants