Skip to content

fix: auto-start workers in TaskHandler context manager#391

Open
nthmost-orkes wants to merge 6 commits into
mainfrom
fix/critical-sdk-behaviors
Open

fix: auto-start workers in TaskHandler context manager#391
nthmost-orkes wants to merge 6 commits into
mainfrom
fix/critical-sdk-behaviors

Conversation

@nthmost-orkes

@nthmost-orkes nthmost-orkes commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Summary

TaskHandler.__enter__ now calls start_processes() automatically, so with TaskHandler(...) as th: works correctly without a separate th.start_processes() call inside the block.

Changes:

  • __enter__ calls start_processes() on entry; if it raises, calls stop_processes() before propagating (since Python skips __exit__ when __enter__ raises)
  • start_processes() gains an idempotency guard (_processes_started flag) so existing code that calls it explicitly inside the with block is unaffected
  • stop_processes() resets the flag so stop → restart cycles work correctly
  • Flag is set after a successful start so a failed start remains retryable

Fixes conductor-oss/getting-started#42.

User impact

First-time users following the quickstart who used with TaskHandler(...) as th: without calling th.start_processes() would see their workflow hang in RUNNING indefinitely — 0 workers were ever started. Now the context manager behaves like standard Python: setup happens in __enter__, teardown in __exit__.

Test plan

  • test_context_manager_enter — verifies with handler as h: h is handler
  • test_context_manager_exit — verifies stop_processes() is called on exit
  • Full unit suite: python3 -m pytest tests/unit/ — 440 passed

🤖 Generated with Claude Code

Comment thread src/conductor/client/automator/task_handler.py Outdated
@nthmost-orkes

Copy link
Copy Markdown
Contributor Author

Changes prompted by this review:

Thinking through the implications surfaced two edge cases worth hardening, both now pushed as follow-up commits:

  • Partial-failure cleanup in __enter__ — Python skips __exit__ when __enter__ raises, so if start_processes() bailed mid-spawn, any already-started workers would leak. __enter__ now calls stop_processes() before re-raising.
  • Flag-set ordering_processes_started = True now runs after a successful start rather than before, so a failed start remains retryable instead of silently no-op'ing on the next attempt.

For the main behavior change itself:

  • Backward compat is preserved — the idempotency guard means existing code that calls start_processes() explicitly inside a with block is unaffected.
  • stop_processes() resets the flag, so stop → restart cycles still work.

@nthmost-orkes nthmost-orkes changed the title fix: auto-start workers in context manager; warn on RUNNING+failed tasks fix: auto-start workers in TaskHandler context manager Apr 23, 2026
@nthmost-orkes
nthmost-orkes requested a review from v1r3n June 16, 2026 23:02
nthmost-orkes and others added 5 commits July 17, 2026 13:35
… tasks

- TaskHandler.__enter__ now calls start_processes() so `with TaskHandler(...)
  as h:` works out of the box without a separate h.start_processes() call.
  start_processes() is idempotent via _processes_started guard, so existing
  code that calls it explicitly inside the with-block is unaffected.
  Fixes conductor-oss/getting-started#42.

- WorkflowExecutor.execute() and execute_workflow() now log a WARNING when
  the workflow returns RUNNING after the wait timeout but a task is already
  in FAILED/FAILED_WITH_TERMINAL_ERROR state, surfacing the failure reason
  that would otherwise be invisible to the caller.
  Fixes conductor-oss/getting-started#41.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The conductor_server.sh one-liner downloaded a 606 MB JAR to the
current directory with no progress indicator and ran in the foreground.
Replace it with the `conductor server start` CLI approach, which handles
download location, caching, and UX correctly.

Fixes conductor-oss/getting-started#47

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If start_processes() raised partway through spawning workers, the flag
was already True, causing a subsequent retry call to be a silent no-op.
Moving the assignment to the end means a failed start remains retryable.
Python skips __exit__ when __enter__ raises, so an exception mid-startup
would leak any workers already spawned. Wrap the call so stop_processes()
runs before the exception propagates.
The extra get_workflow() call on every RUNNING return adds an
unnecessary network round-trip. The underlying behavior (workflow stays
RUNNING while a failed task awaits retries) is correct Conductor
behavior, not a bug — tracked as a docs gap in
conductor-oss/getting-started#53.
@nthmost-orkes
nthmost-orkes force-pushed the fix/critical-sdk-behaviors branch from dff0baf to c99dd69 Compare July 17, 2026 20:36
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/conductor/client/automator/task_handler.py 83.42% <100.00%> (+4.73%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…rent_init

__enter__ now calls start_processes(), which pickles metrics_settings to
pass it to child processes. The mock on clean_metrics_directory is not
picklable, so the test must assert before any processes are started and
clean up via stop_processes() directly.
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.

Python SDK: TaskHandler context manager silently starts 0 workers if start_processes() is not called explicitly

2 participants