fix(delegate): process-based subagent isolation to eliminate GIL contention - #60053
fix(delegate): process-based subagent isolation to eliminate GIL contention#60053bennybuoy wants to merge 8 commits into
Conversation
…ention Batch subagents in delegate_task run on ThreadPoolExecutor workers inside the parent process, sharing one GIL with the TUI/desktop WebSocket event loop. When 3+ subagents do CPU-heavy work (SSE parsing, regex, JSON) the event loop is starved — the WebSocket stalls for tens of seconds and the desktop app drops the connection (issues NousResearch#58576, NousResearch#57903, NousResearch#32079). This PR adds an opt-in process-isolation path (delegation.process_isolation: true in config.yaml). When enabled, each batch child runs in its own OS process via multiprocessing(spawn), giving each subagent its own GIL — mirroring how OpenClaw spawns subagents as separate Node.js child processes. Architecture: - tools/delegate_process.py (new): ChildProcessSpec (picklable parent-side handle), _child_process_main (spawn entry point — reconstructs AIAgent inside the child from plain params, runs conversation, streams progress events over IPC queue), run_children_in_processes (parent-side poll loop — relays IPC events into the same progress callbacks the thread path uses, heartbeats parent activity, propagates interrupts via shared multiprocessing.Event, enforces timeout with hard process termination, reaps all children so no zombies survive). - tools/delegate_tool.py: _build_child_process_spec extracts picklable constructor params from the parent agent. Batch path branches: process isolation runs run_children_in_processes; thread path stays unchanged. Single-task mode always runs in-process (no pool overhead). - hermes_cli/config.py: delegation.process_isolation (bool, default False). Key design decisions: - spawn context (not fork): parent holds live httpx/SSL clients, SQLite connections, and running threads — fork would inherit them corrupt. - IPC bridge: child writes plain-dict events to multiprocessing.Queue; parent reads and calls the original progress callbacks. The TUI/gateway cannot tell the difference — event format is identical. - Interrupt: shared multiprocessing.Event; child watcher thread polls it and calls agent.interrupt(). Parent can hard-terminate after grace. - Session DB: child opens its own SQLite connection to the same DB path. - File-state: child checks writes against parent's read snapshot, passes modified paths back via result entry; parent folds them into its registry. - Credential pool: leased in parent before spawn, released after completion. - Config-gated (default false): safe opt-in, existing tests unchanged. Addresses: NousResearch#58576 NousResearch#57903 NousResearch#32079 Supersedes mitigation approach in: NousResearch#57933 (closed)
…assthrough 12 tests covering: - Children spawn as separate OS processes (PID verification) - IPC event relay from child to parent - Interrupt propagation across process boundaries - Timeout with responsive child (interrupt path) - Timeout with unresponsive child (hard-kill path) - Config gate selects process vs thread path - Results sorted by task_index regardless of completion order - No zombie processes survive the batch - GIL freedom: parent stays responsive during child CPU work - Child process exceptions caught and reported as error entries - Params survive pickling (spawn requirement) Fix: _child_process_main now passes extra params (test_config, goal, etc.) to the agent factory via **_extra, so stub factories in spawned children can receive test configuration across the process boundary.
The real AIAgent constructor TypeErrors on unknown kwargs (task_index, task_count, etc.), so **_extra must only include stub-factory keys (goal, role, _test_config) when agent_factory is explicitly set. When agent_factory is None (production), _extra is empty and the constructor gets only its declared kwargs. E2E verified: 3 subagents ran in separate OS processes via LlamaHerd, each returned correct results (4, 6, 8) in 8.4s total.
…ction spawn re-imports the entire Python module tree per child (~1-2s startup, ~150MB/child). forkserver forks a clean helper process early (before the parent opens httpx clients / SQLite connections), then forks children from it — startup drops to ~10ms, memory to ~20-30MB/child via copy-on-write. The forkserver process is clean (no clients, no threads, no SSL), so forking from it is always safe. This brings the overhead for 3 children from ~450MB (spawn) down to ~75-100MB (forkserver). No fd cleanup needed in _child_process_main: the forkserver helper starts clean, and the IPC Queue/Event objects are managed by the forkserver context itself — blindly closing fds would break them. All 163 tests pass (151 existing + 12 process-isolation tests).
There was a problem hiding this comment.
Pull request overview
Adds an opt-in process-isolated execution mode for delegate_task batch subagents to avoid GIL contention that can stall the TUI/desktop WebSocket event loop. This introduces a new multiprocessing-based runner plus refactors in the existing delegation code so thread- and process-modes emit compatible progress/result shapes.
Changes:
- Add
tools/delegate_process.pyto run batch subagents in isolated OS processes with IPC event/result relaying. - Refactor
tools/delegate_tool.pyto share child settings resolution between thread and process modes and to optionally select process isolation for batch runs. - Add
delegation.process_isolationconfig default (false) and new tests for the process-isolation path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/delegate_tool.py | Adds the config gate, child-settings refactor, and process-isolation batch execution path. |
| tools/delegate_process.py | Implements forkserver/spawn-based child process lifecycle, IPC relay, interrupts, timeouts, and cleanup. |
| tests/tools/test_delegate_process.py | Adds unit tests for process spawning, IPC relay, interrupts/timeouts, ordering, and cleanup. |
| hermes_cli/config.py | Adds delegation.process_isolation: false default config entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ctx = mp.get_context("forkserver") | ||
| event_queue = ctx.Queue() | ||
| result_queue = ctx.Queue() |
There was a problem hiding this comment.
Stale — the forkserver change has been reverted (commit d101daa). The branch now uses spawn, which is available on all platforms including Windows. I also benchmarked forkserver vs spawn and found no meaningful difference in startup time (1.73s vs 1.75s) or memory (87MB vs 88MB per child). The forkserver path still goes through spawn._main → prepare, so it doesn't benefit from copy-on-write module imports as I expected.
| _files_written_map = file_state.writes_since( | ||
| "", wall_start, [] | ||
| ) # all writes since wall_start |
There was a problem hiding this comment.
Good catch — fixed in commit c6401e0. Changed writes_since("", wall_start, []) to writes_since("", wall_start, _files_read) so writes to files the subagent read during its run are surfaced in the subagent.complete payload.
Related to the subagent GIL-contention / WebSocket-stall family: #58576 (event loop stalls under agent work), #32079 (regex held the GIL), #57903 (RCA of the busy-poll GIL contention), and #57933 (closed — identified subprocess isolation as the real fix but didn't implement it). This PR is the first to actually implement process-based isolation via |
|
Closing to rework — the forkserver change doesn't deliver the expected startup performance improvement over spawn. Will re-push once the approach is validated. |
…ory reduction" This reverts commit 405fec2.
Update: forkserver experiment revertedI tested switching from
The forkserver path still goes through The branch now uses |
…urfaced file_state.writes_since filters by the paths iterable — passing [] means no paths match, so _files_written was always empty even when the subagent wrote files. Pass _files_read instead so writes to files the subagent read during its run are reported in the subagent.complete payload. Found by Copilot review on PR NousResearch#60053.
| proc = ctx.Process( | ||
| target=_child_process_main, | ||
| args=(spec.params, event_queue, result_queue, spec.interrupt_event), | ||
| daemon=True, |
There was a problem hiding this comment.
Fixed in commit a2f9b69. Removed daemon=True — the parent-side poll loop already joins/terminates all children before returning, so daemon isn't needed for cleanup. This allows orchestrator-role subagents to spawn their own children via multiprocessing.
| # At least 2 of the 3 children should have different PIDs from the parent | ||
| # (spawn guarantees separate processes; the PID is the proof) | ||
| child_pids.discard(parent_pid) | ||
| self.assertGreaterEqual( | ||
| len(child_pids), 1, | ||
| f"Expected child processes with different PIDs from parent ({parent_pid}), " | ||
| f"got child PIDs: {child_pids}" | ||
| ) |
There was a problem hiding this comment.
Fixed in commit a2f9b69. Strengthened the assertion from assertGreaterEqual(len(child_pids), 1) to assertEqual(len(child_pids), 3) — all 3 children must have distinct PIDs from each other and from the parent.
daemon=True prevents orchestrator-role subagents from spawning their own children via multiprocessing — Python disallows daemon processes from creating child processes. The parent-side poll loop already joins/kill all children before returning, so daemon isn't needed for cleanup. Also strengthen the PID test to assert all 3 children have distinct PIDs (not just at least 1 different from parent). Found by Copilot review on PR NousResearch#60053.
| if child_timeout: | ||
| for i in list(pending): | ||
| elapsed = now - started_mono | ||
| if i not in timeout_signaled and elapsed > child_timeout: | ||
| timeout_signaled[i] = now |
There was a problem hiding this comment.
Fixed in commit e8f7fd7. Added child_start_mono[i] tracked at proc.start() and changed the timeout calculation from now - started_mono to now - child_start_mono.get(i, started_mono), so each child's timeout is measured from its own start time. The fallback to started_mono handles the edge case where a child failed to start.
…t batch start child_timeout was using started_mono (batch-level start) for all children, so staggered process startup ate into later children's timeout budget. Track child_start_mono[i] at proc.start() and compute elapsed per child from its own start time. Found by Copilot review on PR NousResearch#60053.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for pursuing a real isolation path for batch delegation. Current main still runs batch children in a DaemonThreadPoolExecutor (tools/delegate_tool.py:2566), so the underlying structural concern remains.
Problems
tools/delegate_process.py:288-320rebuilds anAIAgentwithoutprovider_require_parameters,provider_data_collection, orrequest_overrides. Current main forwards these attools/delegate_tool.py:1348-1354; process mode would silently change provider-routing behavior.tools/delegate_tool.py:1541-1544explicitly accepts that process children cannot rotate credential-pool entries. Current main leases and swaps pooled credentials before execution (tools/delegate_tool.py:1770-1780), so this weakens existing rate-limit recovery.- The new public
delegation.process_isolationoption has no docs update. Delegation configuration is documented atwebsite/docs/user-guide/configuration.md:1985-2011.
Suggested changes
- Build the process spec from the current child-construction contract and add process/thread parity tests for routing overrides and credential-pool behavior.
- Preserve pool rotation through a parent-mediated protocol, or fail closed for pooled credentials instead of silently degrading recovery.
- Document the opt-in and its operational/platform trade-offs.
Automated hermes-sweeper review.
| providers_ignored=params.get("providers_ignored"), | ||
| providers_order=params.get("providers_order"), | ||
| provider_sort=params.get("provider_sort"), | ||
| openrouter_min_coding_score=params.get("openrouter_min_coding_score"), |
There was a problem hiding this comment.
Blocking: this constructor omits current child-routing inputs (provider_require_parameters, provider_data_collection, and request_overrides; see current tools/delegate_tool.py:1348-1354). Carry those values through the process spec and add a process/thread parity test, otherwise enabling isolation changes provider request behavior.
| # onto every relayed event before the child process even boots. | ||
| session_id = f"{_time.strftime('%Y%m%d_%H%M%S')}_{_uuid.uuid4().hex[:6]}" | ||
|
|
||
| # Lease a credential in the parent so the child starts with a live key |
There was a problem hiding this comment.
Blocking: process mode intentionally loses credential-pool rotation here. Current children receive _credential_pool and _run_single_child leases/swaps a live credential before running (tools/delegate_tool.py:1770-1780). Preserve that recovery contract through parent-mediated IPC, or reject process isolation when a pool is active rather than silently degrading it.
|
Closing as superseded for the original desktop/dashboard responsiveness objective by the merged compute-host isolation work in #65895. The current branch is also substantially stale against Thanks for the review and specific feedback. |
Problem
delegate_taskruns batch subagents onThreadPoolExecutorworkers inside the parent process. All subagent threads share one GIL with the TUI/desktop WebSocket event loop. When 3+ subagents do CPU-heavy work (SSE parsing, regex, JSON), the event loop is starved — the WebSocket stalls for tens of seconds and the desktop app drops the connection.Tracked issues:
Nobody has proposed the process-based fix before. All existing mitigations (locks, semaphores, poll tweaks, turn caps, background modes) work within the threading model. PR #57933 (closed/unmerged) explicitly identified "subprocess isolation for LLM calls" as the real fix but didn't implement it.
Solution
Opt-in process isolation (
delegation.process_isolation: truein config.yaml, defaultfalse). When enabled, each batch child runs in its own OS process viamultiprocessing(forkserver), giving each subagent its own GIL — mirroring how OpenClaw spawns subagents as separate Node.js child processes.Architecture
tools/delegate_process.py(new, 810 lines):ChildProcessSpec(picklable parent-side handle),_child_process_main(forkserver entry point — reconstructs the AIAgent inside the child from picklable params, runs the conversation, streams progress over IPC),run_children_in_processes(parent poll loop — relays IPC events to identical progress callbacks, heartbeats parent activity, propagates interrupts, enforces timeouts, reaps all children)tools/delegate_tool.py:_build_child_process_specextracts constructor args as a plain dict and validates picklability before spawning; batch path branches to process mode when enabledhermes_cli/config.py:delegation.process_isolation: false(opt-in config gate)Why forkserver (not spawn or fork)
Key design decisions
multiprocessing.Queue, parent relays to identical progress callbacks — the TUI/gateway cannot tell the differencemultiprocessing.Event— child watcher thread polls and callsagent.interrupt()delegation.child_timeout_secondsdefaults toNone(no timeout — subagents doing big jobs won't get cancelled). Stuck-child protection is via heartbeat staleness monitor. Hard process termination with grace period (key advantage over threads — you CAN kill a stuck process).Testing
tests/tools/test_delegate_process.pycovering: PID verification (children are separate OS processes), IPC event relay, interrupt propagation, timeout/hard-termination, config gate selection, result ordering by task_index, zombie cleanup, GIL freedom (parent stays responsive during child CPU work), child exception handling, params picklabilityConfig
When
false(default), the existingThreadPoolExecutorpath is used — no behavior change. Whentrue, batch subagents run in isolated OS processes. Single-task mode always uses threads (no pool overhead for one child).Scope
This PR does NOT touch MOA (
agent/moa_loop.py). MOA uses a separateThreadPoolExecutorfor reference model fan-out — it's I/O-bound API calls (not CPU-bound agent loops), so GIL pressure is much lower. That needs a different fix (async SDK migration), not process isolation.Future improvements (not for this PR)