Skip to content

LocalReconciler cleanup uses os.killpg on Windows #578

Closed
#583
Closed
LocalReconciler cleanup uses os.killpg on Windows#578
#583

Description

@rioyu123
Rio Yu (rioyu123)
opened on Sep 1, 2026

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

  1. hzy46 commented on Sep 2, 2026

    @hzy46
    Zhiyuan He (hzy46)
    on Sep 2, 2026
    Contributor

    Yes, we only tested on Ubuntu machines, which should a reasonable choice for training.

  2. YusefSyed commented on Sep 2, 2026

    @YusefSyed
    Yusef Syed (YusefSyed)
    on Sep 2, 2026
    Contributor

    I audited the current cleanup path against the Python subprocess contract and Windows process-tree semantics. I do not think a small os.name branch to proc.kill(), or CREATE_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 visible AttributeError into 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_exec and before any Proc state 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, a psutil tree walk, or direct-child termination while claiming equivalent cleanup.

    Could a maintainer confirm this decision gate?

    • If the OS Independent classifier 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?
    • If native Windows is intended, would a separate Job Object design with a Windows end-to-end process-tree test and continuing Windows CI be acceptable?

    I will wait for that platform-contract decision before changing source.

  3. hzy46 commented on Sep 2, 2026

    @hzy46
    Zhiyuan He (hzy46)
    on Sep 2, 2026
    Contributor

    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.

  4. rioyu123 commented on Sep 2, 2026

    @rioyu123
    Rio Yu (rioyu123)
    on Sep 2, 2026
    ContributorAuthor

    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 LocalReconciler fail 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.

  5. hzy46 commented on Sep 2, 2026

    @hzy46
    Zhiyuan He (hzy46)
    on Sep 2, 2026
    Contributor

    Rio Yu (@rioyu123)

    Would you be open to a narrow PR that makes LocalReconciler fail fast with a clear unsupported-platform error on native Windows before spawning a worker, and documents that the local runner is not supported there?

    I am ok with this change.

  6. Zhiyuan He (hzy46)
    closed this as completedin #583on Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions