Skip to content

fix(workflow): controller readiness, atomic orchestrator swap, and shutdown cleanup#107

Open
shoom1 wants to merge 1 commit into
developfrom
fix/workflow-controller-lifecycle
Open

fix(workflow): controller readiness, atomic orchestrator swap, and shutdown cleanup#107
shoom1 wants to merge 1 commit into
developfrom
fix/workflow-controller-lifecycle

Conversation

@shoom1

@shoom1 shoom1 commented Jul 18, 2026

Copy link
Copy Markdown
Owner

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_ready meant "manager object exists": _background_init published self._workflow before initialize_services() ran, so a failed or still-initializing manager was reported ready, and ensure_initialized() returned True before ever consulting _init_error.
  • Unnamed extra: 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).
  • Orchestrator swap assigned the new manager before initializing it: a failed swap left a broken manager with the working one lost; a successful swap orphaned the old manager without cleanup() (which tears down real resources — sessions, sandbox manager).
  • App shutdown (background_init CM exit) only cancelled the init task; manager.cleanup() was never called anywhere in the app.

Fix

  • Publish-after-success in _background_init; failure/cancel paths clean up the partially-built manager best-effort. is_ready/ensure_initialized become truthful with no new state to drift (states remain derivable: uninitialized / initializing / ready / failed).
  • asyncio.Lock around initialize_services() in BaseWorkflowManager — concurrency-safe and idempotent for both orchestrators and any direct library consumer.
  • Swap: initialize replacement → swap atomically → clean up old; on init failure the working manager stays in place and the broken replacement is cleaned up.
  • WorkflowController.close(): idempotent (cancel init → detach → cleanup), wired into the background_init CM finally, 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_initialized blocking until services finish, swap-failure-keeps-old / swap-success-cleans-old, idempotent close(), CM-exit cleanup, and concurrent + sequential initialize_services single-run. Full offline suite: 1990 passed. (The traitlets RuntimeWarning visible 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

…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
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.

1 participant