Repository navigation
feat(workers): stop and start the worker of a single library - #5846
Merged
cjkindel merged 5 commits intoOct 13, 2026
Merged
Conversation
The only way to stop workers was reset_workers, which stops all of them and only runs when every library reloads. Anything that changes one library's execution environment has to get that library's worker off it first: the worker holds .venv-exec on its import path, and on Windows its open files cannot be deleted. WorkerManager.stop_worker_for_library terminates one library's worker, fails anything awaiting it with the reason, unsubscribes its response topic, and drops its spawn claim so a later spawn is admitted. Every other worker keeps running. LibraryWorkers.start_worker_for_library starts that library's worker again through the same _start_workers rules (session required, unmet requirements and failed execution builds refused), limited to that one library.
Contributor
|
✅ GriptapeOps finished @cjkindel's request in 4m 12s
Review posted at c773e60. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![griptapeops[bot]](https://avatars.githubusercontent.com/in/3186107?s=60&v=4){kind=small}
stop_worker_for_library dropped the spawn claim of a spawn suspended between its claim and the registry write, so the spawn resumed afterwards and registered a live worker, and a start in that window could fork a second one. The claim is now an asyncio.Event the spawn sets when its attempt ends, and the stop waits for it before terminating whatever the spawn produced. reset_workers and stop_worker_for_library now share _stop_workers, so the two cannot drift. reset_workers still clears every claim without waiting.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| assert not stop.done() | ||
|
|
||
| released.set() | ||
| await spawn |
|
|
||
| released.set() | ||
| await spawn | ||
| await stop |
Contributor
Author
|
@griptapeops re-review |
Contributor
Contributor
There was a problem hiding this comment.
Both findings resolved, and the commits since c773e60 are clean against the rubric.
cjkindel
marked this pull request as ready for review
October 9, 2026 21:57
cjkindel
requested review from
a team and
mcaloud
and removed request for
a team
October 9, 2026 21:57
mcaloud
reviewed
Oct 13, 2026
mcaloud
left a comment
mcaloud
left a comment
Contributor
There was a problem hiding this comment.
Nothing blocks the merge, but finding 1 should be fixed before the next PR in the stack starts calling this. The worker ids are now read before the processes are terminated, so a worker that registers during the stop stays in the registry, and that also regresses reset_workers. Finding 2: the stop leaves the library's readiness gate waiting.
- MEDIUM
worker_manager.py:576: readworker_idsafter termination, not before - MEDIUM
worker_manager.py:563: record why the library has no worker, so the readiness gate waiters are released - LOW
test_library_worker_config.py:508: no test covers the guard onstart_worker_for_library
…s waiters _stop_workers read the worker ids before awaiting termination, so a worker that registered during that await stayed in the registry: routable to a dead process, skipped by start_worker_for_library as already registered, and its response topic left subscribed. reset_workers regressed the same way. The ids are now read after the termination. stop_worker_for_library now records why the library has no worker, so a node waiting on its readiness gate is released with the reason instead of waiting out the load timeout. expect_worker clears it on the restart. Adds a test for start_worker_for_library's no-session guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #5845 (base branch
feat/library-metadata-load-outcome). Part of #5102.This PR doesn't depend on #5845's code. It sits on top of it only to keep the stack linear.
Problem
The only way to stop workers is
reset_workers, which stops all of them and runs only when every library reloads. Anything that changes one library's execution environment has to get that library's worker off it first: the worker holds.venv-execon its import path, and on Windows its open files can't be deleted. The reset in the next PR is the first caller.Change
WorkerManager.stop_worker_for_library(library_name): stops one library's worker and leaves every other worker running. If that library's spawn is still in progress (claimed, but not yet in the registry), it waits for the spawn to finish, then terminates whatever process it produced. So when it returns, no worker for the library is running or about to start. It fails anything awaiting the worker withWorkerGoneError("... shut down to reset library ...")and unsubscribes its response topic. Worker registrations are read after the termination, so one that registers while its process is stopping is forgotten too. It also records why the library has no worker (note_worker_unavailable), so a node waiting on the readiness gate is released with that reason instead of waiting out the load timeout;expect_workerclears it on the restart. The spawn claim is now anasyncio.Eventthat the spawn sets when its attempt ends.reset_workersandstop_worker_for_libraryshare one_stop_workershelper.reset_workersstill clears every claim without waiting.LibraryWorkers.start_worker_for_library(library_name): starts that library's worker again through_start_workers, which gains anonly_library_namefilter. The usual rules still apply: a session is required, and unmet requirements and failed execution builds are refused.Testing
make checkand the unit suite pass. New tests:TestStopWorkerForLibraryintest_app_worker.py: only the target process and registration go, an in-progress spawn is waited out and the process it produced is stopped, in-flight requests settle with the reason, and a library with no worker is a no-op, a worker that registers mid-stop is forgotten, and the readiness gate is released with a reason.start_worker_for_libraryintest_library_worker_config.py.