Repository navigation
LocalReconciler cleanup uses os.killpg on Windows #578
Description
Summary
LocalReconciler unconditionally calls the Unix-only os.killpg() API when a local rollout times out or the controller shuts down. On native Windows, os.killpg is absent, so cleanup raises AttributeError before the worker is marked killed or the rollout is patched to the intended failed state.
This report is intentionally limited to the cleanup path. On Windows/Python 3.13, start_new_session=True is accepted by subprocess.Popen; the reproducible portability failure is the later os.killpg() call.
Current code
At main@88528bf4b7360d852f5bc5d023943697953ec332, agentlightning/controller/local_reconciler.py uses:
with contextlib.suppress(ProcessLookupError):
os.killpg(item.proc.pid, signal.SIGKILL)
The same helper is called from timeout reconciliation and controller shutdown.
Reproduction evidence
On native Windows:
import os
import subprocess
import sys
assert not hasattr(os, "killpg")
proc = subprocess.Popen(
[sys.executable, "-c", "pass"],
start_new_session=True,
)
assert proc.wait(timeout=10) == 0
Calling _kill_process_group() with a live fake process then reaches the missing os.killpg attribute. contextlib.suppress(ProcessLookupError) does not catch that error.
Expected behavior / contract question
Is the local controller intended to be POSIX-only?
- If yes, it should fail early with a clear unsupported-platform error and document the constraint.
- If native Windows is intended to work, the subprocess cleanup path needs a Windows-compatible bounded termination strategy while retaining process-group cleanup on POSIX.
I would be happy to prepare a focused PR once the intended platform contract is confirmed.
Activity
hzy46 commentedon Sep 2, 2026
Yes, we only tested on Ubuntu machines, which should a reasonable choice for training.
YusefSyed commentedon Sep 2, 2026
I audited the current cleanup path against the Python subprocess contract and Windows process-tree semantics. I do not think a small
os.namebranch toproc.kill(), orCREATE_NEW_PROCESS_GROUP+CTRL_BREAK, is an equivalent fix: the former terminates only the direct child, while the latter is a cooperative, console-dependent signal. Either can turn the visibleAttributeErrorinto a false-success state where descendant agent processes remain alive.The narrow initial contract I would propose is therefore provisional fail-fast behavior for the local subprocess controller only on native Windows, before
create_subprocess_execand before anyProcstate is created. Package import and other controller modes would remain available, and the existing POSIX process-group cleanup would remain unchanged.If native-Windows local execution is an intended product requirement, Windows Job Objects appear to be the appropriate follow-up ownership primitive: they can manage descendants as a unit, but would require explicit handle/assignment/closure semantics, behavior when the controller is already inside a job, proof that agent code cannot spawn before assignment, and real Windows CI. I would not substitute
taskkill, apsutiltree walk, or direct-child termination while claiming equivalent cleanup.Could a maintainer confirm this decision gate?
OS Independentclassifier is not intended to cover the local runner, may I submit the narrow pre-spawn unsupported-platform check plus a focused regression and feature-level documentation?I will wait for that platform-contract decision before changing source.
hzy46 commentedon Sep 2, 2026
Yusef Syed (@YusefSyed) Considering the significant effort of support job running in Windows, we do not have plan to maintain it officially. But we welcome community PR for supporting this.
rioyu123 commentedon Sep 2, 2026
Thanks for clarifying. Since native Windows job execution is not planned as an officially maintained platform, I don't think a partial
proc.kill()fallback would be a good fit.Would you be open to a narrow PR that makes
LocalReconcilerfail fast with a clear unsupported-platform error on native Windows before spawning a worker, and documents that the local runner is not supported there?Package imports and other controller modes would remain unchanged, and the PR would not claim Windows support.
hzy46 commentedon Sep 2, 2026
Rio Yu (@rioyu123)
I am ok with this change.