Skip to content

Commit bc4b1f2

Browse files
committed
Make @worker_task workers spawn-safe and default all platforms to spawn
Fixes the macOS failure mode where every @worker_task worker subprocess died with SIGSEGV (exitcode=-11) under the forced 'fork' start method and was silently restarted by the TaskHandler monitor every ~5s, so no task ever completed (issues #264, #271). Worker pickling (required by spawn/forkserver, which pickle Process args): - Worker.api_client is now a lazy property: no eager httpx.Client/TLS/auth in the parent process, and no '_thread.lock' in pickled state - Worker.__getstate__/__setstate__ exclude process-local runtime state (pending-tasks lock, background event loop, pending async futures) and rebuild it in the child - @worker_task-decorated functions pickle as importable references (module, qualname, __wrapped__ depth) resolved in the child, fixing "PicklingError: it's not the same object as module.name" caused by the decorator rebinding the module-level name to its wrapper Start method: - Default is now 'spawn' on all platforms (was fork everywhere except Windows). fork is unsafe with native state: Apple frameworks SIGSEGV on macOS (CPython's own default is spawn there since 3.8, bpo-33725) and fork-with-held-lock deadlocks on POSIX. CONDUCTOR_MP_START_METHOD=fork remains as an escape hatch Diagnostics and robustness: - TaskHandler.start_processes() cleans up already-started subprocesses and raises an actionable error when worker state cannot be pickled (was: the non-daemon logger process kept the interpreter alive forever -> hang) - Worker processes killed by a signal now log the signal number and PYTHONFAULTHANDLER=1 guidance instead of restarting silently Tests: - tests/unit/worker/test_worker_spawn_safety.py: pickle round-trip, identity preservation, async detection, lazy api_client, runtime-state isolation, fail-fast for nested functions, and execution of a Worker in a real spawn child process - tests/unit/worker/test_worker_config_integration.py no longer replaces sys.modules['...task_handler'] with a Mock at import time; that leaked into the whole pytest session and broke pickle-by-reference for every later test that started a real process - examples/spawn_safety_probe.py: standalone before/after probe script
1 parent fb10a2a commit bc4b1f2

8 files changed

Lines changed: 592 additions & 24 deletions

File tree

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1515

1616
### Changed
1717

18+
- **Multiprocessing start method now defaults to `spawn` on all platforms** (was `fork` everywhere except Windows). `fork` caused silently-restarting `SIGSEGV` worker subprocesses on macOS (exitcode `-11`) and fork-with-held-lock deadlocks on POSIX. Opt back in with `CONDUCTOR_MP_START_METHOD=fork`. Entrypoint scripts must use the standard `if __name__ == "__main__":` guard, and workers must be defined at module level
1819
- Legacy metrics emit unchanged by default; no env var required
1920
- `metrics_collector.py` is now a compatibility shim; `from conductor.client.telemetry.metrics_collector import MetricsCollector` continues to work
21+
22+
### Fixed
23+
24+
- `@worker_task` workers are now picklable, making the decorator path work with the `spawn`/`forkserver` start methods (fixes `TypeError: cannot pickle '_thread.lock' object` and `PicklingError: it's not the same object as ...`; issues #264, #271): `Worker.api_client` is created lazily in the worker process, runtime state (locks, pending async tasks, background loop) is excluded from pickling and rebuilt in the child, and decorated functions are pickled as importable references resolved in the child
25+
- `TaskHandler.start_processes()` no longer hangs the interpreter when a worker fails to start (e.g., unpicklable state under `spawn`); it now cleans up already-started subprocesses and raises with actionable guidance
26+
- Worker processes killed by a signal now log a diagnostic hint (signal number, `PYTHONFAULTHANDLER=1` guidance) instead of restarting silently

docs/WORKER.md

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -229,11 +229,6 @@ from conductor.client.configuration.configuration import Configuration
229229
from conductor.client.automator.task_handler import TaskHandler
230230
from conductor.client.worker.worker import Worker
231231

232-
#### Add these lines if running on a mac####
233-
from multiprocessing import set_start_method
234-
set_start_method('fork')
235-
############################################
236-
237232
SERVER_API_URL = 'http://localhost:8080/api'
238233
KEY_ID = '<KEY_ID>'
239234
KEY_SECRET = '<KEY_SECRET>'
@@ -261,10 +256,39 @@ workers = [
261256

262257
# TaskHandler scans for @worker_task decorated workers by default.
263258
# Set scan_for_annotated_workers=False if you want to disable auto-discovery.
264-
with TaskHandler(workers, configuration, scan_for_annotated_workers=True) as task_handler:
265-
task_handler.start_processes()
259+
# The __main__ guard is required: worker processes use the 'spawn'
260+
# multiprocessing start method by default, which re-imports this script.
261+
if __name__ == '__main__':
262+
with TaskHandler(workers, configuration, scan_for_annotated_workers=True) as task_handler:
263+
task_handler.start_processes()
266264
```
267265

266+
### Multiprocessing start method
267+
268+
`TaskHandler` runs each worker in its own process using Python's `multiprocessing`
269+
with the **`spawn`** start method by default on all platforms. `spawn` starts each
270+
worker as a fresh interpreter, which avoids the crashes and deadlocks `fork`
271+
causes in processes holding native state — most visibly on macOS, where forked
272+
workers can die with `SIGSEGV` (exitcode `-11`) inside Apple system frameworks
273+
and get silently restarted by the supervisor.
274+
275+
Requirements under `spawn` (standard Python multiprocessing rules):
276+
277+
- Guard your entrypoint with `if __name__ == '__main__':`
278+
- Define workers at module level (not nested inside functions or lambdas)
279+
- `configuration`, `metrics_settings`, and `event_listeners` passed to
280+
`TaskHandler` must be picklable
281+
282+
To opt back into the previous behavior (e.g., a Linux deployment relying on
283+
fork's copy-on-write memory sharing):
284+
285+
```shell
286+
export CONDUCTOR_MP_START_METHOD=fork # spawn (default) | fork | forkserver
287+
```
288+
289+
If a worker process dies from a signal repeatedly at startup, set
290+
`PYTHONFAULTHANDLER=1` to capture the crashing stack trace from the child.
291+
268292
### Resilience: auto-restart and health checks
269293

270294
If you run workers as a long-lived service (e.g., alongside FastAPI/Uvicorn), you can optionally enable process

examples/spawn_safety_probe.py

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
"""
2+
spawn_safety_probe.py - @worker_task spawn-safety probe (GitHub issues #264/#271).
3+
4+
Run from the repo root:
5+
PYTHONPATH=src python3 examples/spawn_safety_probe.py
6+
7+
Forces the 'spawn' multiprocessing start method (the SDK default; also the
8+
only safe method on macOS) and then checks, in order:
9+
10+
1. Which Worker attributes survive pickling (attribute-level diagnostic)
11+
2. Whole-Worker pickle round-trip + task execution on the restored worker
12+
3. Worker transferred into a REAL 'spawn' child process and executed there
13+
(exactly what TaskHandler.start_processes() does)
14+
15+
Broken SDK output: FAIL lines for api_client / _pending_tasks_lock /
16+
_execute_function; round-trip and spawn-child checks fail.
17+
Fixed SDK output: everything PASS.
18+
"""
19+
import os
20+
import sys
21+
22+
# Force spawn BEFORE importing any conductor module: task_handler pins the
23+
# start method at import time from this env var.
24+
os.environ["CONDUCTOR_MP_START_METHOD"] = "spawn"
25+
26+
import multiprocessing
27+
import pickle
28+
29+
from conductor.client.worker.worker_task import worker_task
30+
from conductor.client.http.models.task import Task
31+
32+
33+
@worker_task(task_definition_name="pickle_probe_task")
34+
def pickle_probe_task(x: int) -> dict:
35+
return {"doubled": x * 2}
36+
37+
38+
def _make_task() -> Task:
39+
return Task(
40+
task_id="probe-task-id",
41+
workflow_instance_id="probe-wf-id",
42+
task_def_name="pickle_probe_task",
43+
input_data={"x": 21},
44+
status="IN_PROGRESS",
45+
)
46+
47+
48+
def run_in_child(worker, result_queue) -> None:
49+
"""Spawn-child entry point: unpickling `worker` here IS the test."""
50+
result = worker.execute(_make_task())
51+
result_queue.put((str(result.status), result.output_data))
52+
53+
54+
def get_probe_worker():
55+
from conductor.client.automator.task_handler import get_registered_workers
56+
return next(w for w in get_registered_workers()
57+
if w.task_definition_name == "pickle_probe_task")
58+
59+
60+
def main() -> int:
61+
failures = 0
62+
print(f"python={sys.version.split()[0]} platform={sys.platform}")
63+
64+
worker = get_probe_worker() # importing task_handler pins the start method
65+
print(f"multiprocessing start method: {multiprocessing.get_start_method(allow_none=True)}\n")
66+
67+
# -- 1. attribute-level diagnostic --------------------------------------
68+
# Pickle what pickling actually serializes: the __getstate__ output.
69+
# (Raw __dict__ values legitimately contain process-local objects - a
70+
# threading.Lock, the original decorated function - which __getstate__
71+
# excludes or substitutes. If a future change adds unpicklable state and
72+
# forgets to handle it in __getstate__, this section catches it.)
73+
print("[1] Worker pickled-state (__getstate__) picklability:")
74+
state = worker.__getstate__() if hasattr(worker, "__getstate__") else dict(worker.__dict__)
75+
for k, v in state.items():
76+
try:
77+
pickle.dumps(v)
78+
print(f" PASS {k}")
79+
except Exception as e:
80+
failures += 1
81+
print(f" FAIL {k}: {type(e).__name__}: {str(e)[:70]}")
82+
83+
# -- 2. whole-worker round-trip + execute --------------------------------
84+
print("\n[2] Whole-Worker pickle round-trip:")
85+
try:
86+
restored = pickle.loads(pickle.dumps(worker))
87+
same_fn = restored.execute_function is worker.execute_function
88+
result = restored.execute(_make_task())
89+
if str(result.status) == "COMPLETED" and result.output_data == {"doubled": 42}:
90+
print(f" PASS round-trip (same function object: {same_fn}, "
91+
f"result: {result.output_data})")
92+
else:
93+
failures += 1
94+
print(f" FAIL unexpected result: {result.status} {result.output_data}")
95+
except Exception as e:
96+
failures += 1
97+
print(f" FAIL {type(e).__name__}: {str(e)[:100]}")
98+
99+
# -- 3. real spawn child --------------------------------------------------
100+
print("\n[3] Execute in a real 'spawn' child process:")
101+
ctx = multiprocessing.get_context("spawn")
102+
q = ctx.Queue()
103+
p = ctx.Process(target=run_in_child, args=(get_probe_worker(), q))
104+
try:
105+
p.start()
106+
status, output = q.get(timeout=60)
107+
p.join(timeout=10)
108+
if status == "COMPLETED" and output == {"doubled": 42}:
109+
print(f" PASS child exitcode={p.exitcode}, result={output}")
110+
else:
111+
failures += 1
112+
print(f" FAIL status={status} output={output}")
113+
except Exception as e:
114+
failures += 1
115+
print(f" FAIL {type(e).__name__}: {str(e)[:100]}")
116+
if p.is_alive():
117+
p.terminate()
118+
p.join(timeout=5)
119+
120+
print(f"\n{'ALL CHECKS PASSED' if failures == 0 else f'{failures} CHECK(S) FAILED'}")
121+
return 0 if failures else 1
122+
123+
124+
if __name__ == "__main__":
125+
sys.exit(main())

src/conductor/client/automator/task_handler.py

Lines changed: 56 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,11 @@
44
import inspect
55
import logging
66
import os
7+
import pickle
78
import signal
89
import threading
910
import time
10-
from multiprocessing import Process, freeze_support, Queue, set_start_method
11+
from multiprocessing import Process, freeze_support, Queue, set_start_method, get_start_method
1112
from sys import platform
1213
from typing import List, Optional, Any, Dict
1314

@@ -37,12 +38,25 @@
3738
_mp_fork_set = False
3839
if not _mp_fork_set:
3940
try:
40-
# The prometheus_client library holds a module-level threading lock;
41-
# forking while that lock is held causes a deadlock in child processes.
42-
# Set CONDUCTOR_MP_START_METHOD=spawn to avoid this if you hit the
43-
# deadlock. Default is fork for backward compatibility (spawn requires
44-
# all Process arguments to be picklable).
45-
_default_method = "spawn" if platform == "win32" else "fork"
41+
# Default start method: "spawn" on every platform.
42+
#
43+
# fork() is fundamentally unsafe in a process that holds native
44+
# framework state or non-main threads:
45+
# - macOS: Apple frameworks (CFNetwork, Security, libdispatch) may
46+
# SIGSEGV in forked children, producing silently-restarting worker
47+
# processes with exitcode=-11. CPython switched its own macOS
48+
# default to spawn in 3.8 for the same reason (bpo-33725).
49+
# - all POSIX: forking while another thread holds a lock (e.g. the
50+
# prometheus_client module-level lock, or the TaskHandler monitor
51+
# thread restarting a worker) leaves that lock permanently held in
52+
# the child -> silent deadlock.
53+
# Workers (including the @worker_task decorator path) are pickle-safe,
54+
# so spawn works everywhere. Deployments that rely on fork's
55+
# copy-on-write inheritance can opt back in with
56+
# CONDUCTOR_MP_START_METHOD=fork (re-exposing the hazards above).
57+
# NOTE: spawn requires the standard `if __name__ == "__main__":` guard
58+
# in the entrypoint script.
59+
_default_method = "spawn"
4660
_method = os.environ.get("CONDUCTOR_MP_START_METHOD", "").strip().lower() or _default_method
4761
if _method not in _VALID_MP_START_METHODS:
4862
logger.warning(
@@ -341,8 +355,26 @@ def start_processes(self) -> None:
341355
logger.info("Starting worker processes...")
342356
freeze_support()
343357
self._monitor_stop_event.clear()
344-
self.__start_task_runner_processes()
345-
self.__start_metrics_provider_process()
358+
try:
359+
self.__start_task_runner_processes()
360+
self.__start_metrics_provider_process()
361+
except (TypeError, pickle.PicklingError, AttributeError) as e:
362+
# Under the 'spawn'/'forkserver' start methods, Process arguments
363+
# are pickled at start(); unpicklable state fails here. Clean up
364+
# everything already started - otherwise the non-daemon logger
365+
# subprocess keeps the interpreter alive forever after the
366+
# exception (observed as a hang after "cannot pickle
367+
# '_thread.lock' object").
368+
logger.error(
369+
"Failed to start worker processes with start method %r: %s. "
370+
"Workers must be defined at module level (not nested inside "
371+
"functions), and configuration, metrics_settings and "
372+
"event_listeners must be picklable. "
373+
"See https://github.com/conductor-oss/conductor-python/issues/264",
374+
get_start_method(allow_none=True), e,
375+
)
376+
self.stop_processes()
377+
raise
346378
self.__start_monitor_thread()
347379
logger.info("Started all processes")
348380

@@ -445,6 +477,21 @@ def __check_and_restart_processes(self) -> None:
445477
worker = self.workers[i] if i < len(self.workers) else None
446478
worker_name = worker.get_task_definition_name() if worker is not None else f"worker[{i}]"
447479
logger.warning("Worker process exited (worker=%s, pid=%s, exitcode=%s)", worker_name, process.pid, exitcode)
480+
if exitcode < 0:
481+
# Negative exitcode == killed by signal (e.g. -11 is
482+
# SIGSEGV). Under the 'fork' start method this is the
483+
# classic symptom of fork-unsafe native libraries in the
484+
# parent (Apple frameworks on macOS, numpy/Accelerate,
485+
# grpc, ...).
486+
logger.warning(
487+
"Worker process %s was killed by signal %s. If this "
488+
"repeats on every restart, the process is likely "
489+
"crashing at startup: use the 'spawn' start method "
490+
"(CONDUCTOR_MP_START_METHOD=spawn, the default) and "
491+
"set PYTHONFAULTHANDLER=1 to capture the crashing "
492+
"stack trace.",
493+
worker_name, -exitcode,
494+
)
448495
if not self.restart_on_failure:
449496
continue
450497
self.__restart_worker_process(i)

0 commit comments

Comments
 (0)