fix(workflow): controller readiness, atomic orchestrator swap, and shutdown cleanup#107
Open
shoom1 wants to merge 1 commit into
Open
fix(workflow): controller readiness, atomic orchestrator swap, and shutdown cleanup#107shoom1 wants to merge 1 commit into
shoom1 wants to merge 1 commit into
Conversation
…e-correct Review finding: is_ready meant "manager object exists" — background init published the manager before initialize_services() ran, so a failed or in-flight init was reported ready and ensure_initialized() returned early. Orchestrator swaps replaced the working manager before the replacement initialized (a failed swap left a broken manager), leaked the old manager without cleanup, and app shutdown never called manager cleanup at all. - _background_init publishes the manager only after initialize_services() succeeds; failure/cancel paths clean up the partially-built manager. is_ready/ensure_initialized are now truthful with no extra state. - Orchestrator swap initializes the replacement first, swaps atomically, then cleans up the old manager; on failure the working manager stays. - BaseWorkflowManager.initialize_services() gains an asyncio.Lock: the _initialized guard was check-then-act, so background init racing a first user message (process → _ensure_initialized) ran the body twice. - WorkflowController.close(): idempotent cancel + manager cleanup, wired into the background_init context manager exit (the app shutdown path). Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv
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
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.
Addresses review finding 4 (workflow readiness and lifecycle ownership). Verified all claims against the code first; every one held, plus one unnamed race.
Problems (confirmed)
is_readymeant "manager object exists":_background_initpublishedself._workflowbeforeinitialize_services()ran, so a failed or still-initializing manager was reported ready, andensure_initialized()returnedTruebefore ever consulting_init_error.initialize_services()guarded with check-then-act (if self._initialized: return) and no lock — background init racing a first user message (process()→_ensure_initialized()) ran the whole body twice (duplicate registry refresh, duplicate service creation).cleanup()(which tears down real resources — sessions, sandbox manager).background_initCM exit) only cancelled the init task;manager.cleanup()was never called anywhere in the app.Fix
_background_init; failure/cancel paths clean up the partially-built manager best-effort.is_ready/ensure_initializedbecome truthful with no new state to drift (states remain derivable: uninitialized / initializing / ready / failed).asyncio.Lockaroundinitialize_services()inBaseWorkflowManager— concurrency-safe and idempotent for both orchestrators and any direct library consumer.WorkflowController.close(): idempotent (cancel init → detach → cleanup), wired into thebackground_initCMfinally, which is the app's shutdown path (app.py:555).Deliberately not an explicit state enum: the reviewer suggested one, but with publish-after-success the four states are already derivable from
(_workflow, _init_task, _init_error)— an enum would duplicate state that can drift.Tests
TDD: 9 new tests written first and watched fail — readiness during in-flight init, failed-init cleanup + not-ready,
ensure_initializedblocking until services finish, swap-failure-keeps-old / swap-success-cleans-old, idempotentclose(), CM-exit cleanup, and concurrent + sequentialinitialize_servicessingle-run. Full offline suite: 1990 passed. (The traitletsRuntimeWarningvisible in full runs was verified pre-existing via stashed A/B run — order/GC-dependent, in untouched sandbox tests.)https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv