Skip to content

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

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

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
@shoom1

shoom1 commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

Superseded by #110.

The framework review series includes this work in fix(workflow): serialize turns, make init transactional, own resources (2c89da4), which builds on it: the derived WorkflowState, atomic orchestrator swap and shutdown cleanup here, plus turn/lifecycle lock ordering, transactional service construction, cancellation-safe shutdown owned by the controller, and a failed in-place reinitialization that keeps the manager so its preserved sessions survive.

Closing unmerged — the content is a strict subset of #110, not a dropped change.

@shoom1 shoom1 closed this Aug 3, 2026
@shoom1
shoom1 deleted the fix/workflow-controller-lifecycle branch August 3, 2026 05:24
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