Merge PR #219: fix: guard POSIX-only process functions for Windows compatibility
Authored by Farukest. Fixes #218.
This commit is contained in:
commit
2ba87a10b0
5 changed files with 129 additions and 16 deletions
|
|
@ -19,7 +19,10 @@ import asyncio
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
import platform
|
||||||
import subprocess
|
import subprocess
|
||||||
|
|
||||||
|
_IS_WINDOWS = platform.system() == "Windows"
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Dict, List, Optional, Any
|
from typing import Dict, List, Optional, Any
|
||||||
|
|
||||||
|
|
@ -166,7 +169,7 @@ class WhatsAppAdapter(BasePlatformAdapter):
|
||||||
],
|
],
|
||||||
stdout=subprocess.DEVNULL,
|
stdout=subprocess.DEVNULL,
|
||||||
stderr=subprocess.DEVNULL,
|
stderr=subprocess.DEVNULL,
|
||||||
preexec_fn=os.setsid,
|
preexec_fn=None if _IS_WINDOWS else os.setsid,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Wait for bridge to be ready via HTTP health check
|
# Wait for bridge to be ready via HTTP health check
|
||||||
|
|
@ -211,13 +214,19 @@ class WhatsAppAdapter(BasePlatformAdapter):
|
||||||
# Kill the entire process group so child node processes die too
|
# Kill the entire process group so child node processes die too
|
||||||
import signal
|
import signal
|
||||||
try:
|
try:
|
||||||
os.killpg(os.getpgid(self._bridge_process.pid), signal.SIGTERM)
|
if _IS_WINDOWS:
|
||||||
|
self._bridge_process.terminate()
|
||||||
|
else:
|
||||||
|
os.killpg(os.getpgid(self._bridge_process.pid), signal.SIGTERM)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
self._bridge_process.terminate()
|
self._bridge_process.terminate()
|
||||||
await asyncio.sleep(1)
|
await asyncio.sleep(1)
|
||||||
if self._bridge_process.poll() is None:
|
if self._bridge_process.poll() is None:
|
||||||
try:
|
try:
|
||||||
os.killpg(os.getpgid(self._bridge_process.pid), signal.SIGKILL)
|
if _IS_WINDOWS:
|
||||||
|
self._bridge_process.kill()
|
||||||
|
else:
|
||||||
|
os.killpg(os.getpgid(self._bridge_process.pid), signal.SIGKILL)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
self._bridge_process.kill()
|
self._bridge_process.kill()
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
|
|
||||||
80
tests/tools/test_windows_compat.py
Normal file
80
tests/tools/test_windows_compat.py
Normal file
|
|
@ -0,0 +1,80 @@
|
||||||
|
"""Tests for Windows compatibility of process management code.
|
||||||
|
|
||||||
|
Verifies that os.setsid and os.killpg are never called unconditionally,
|
||||||
|
and that each module uses a platform guard before invoking POSIX-only functions.
|
||||||
|
"""
|
||||||
|
|
||||||
|
import ast
|
||||||
|
import pytest
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
# Files that must have Windows-safe process management
|
||||||
|
GUARDED_FILES = [
|
||||||
|
"tools/environments/local.py",
|
||||||
|
"tools/process_registry.py",
|
||||||
|
"tools/code_execution_tool.py",
|
||||||
|
"gateway/platforms/whatsapp.py",
|
||||||
|
]
|
||||||
|
|
||||||
|
PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent
|
||||||
|
|
||||||
|
|
||||||
|
def _get_preexec_fn_values(filepath: Path) -> list:
|
||||||
|
"""Find all preexec_fn= keyword arguments in Popen calls."""
|
||||||
|
source = filepath.read_text(encoding="utf-8")
|
||||||
|
tree = ast.parse(source, filename=str(filepath))
|
||||||
|
values = []
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if isinstance(node, ast.keyword) and node.arg == "preexec_fn":
|
||||||
|
values.append(ast.dump(node.value))
|
||||||
|
return values
|
||||||
|
|
||||||
|
|
||||||
|
class TestNoUnconditionalSetsid:
|
||||||
|
"""preexec_fn must never be a bare os.setsid reference."""
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("relpath", GUARDED_FILES)
|
||||||
|
def test_preexec_fn_is_guarded(self, relpath):
|
||||||
|
filepath = PROJECT_ROOT / relpath
|
||||||
|
if not filepath.exists():
|
||||||
|
pytest.skip(f"{relpath} not found")
|
||||||
|
values = _get_preexec_fn_values(filepath)
|
||||||
|
for val in values:
|
||||||
|
# A bare os.setsid would be: Attribute(value=Name(id='os'), attr='setsid')
|
||||||
|
assert "attr='setsid'" not in val or "IfExp" in val or "None" in val, (
|
||||||
|
f"{relpath} has unconditional preexec_fn=os.setsid"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestIsWindowsConstant:
|
||||||
|
"""Each guarded file must define _IS_WINDOWS."""
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("relpath", GUARDED_FILES)
|
||||||
|
def test_has_is_windows(self, relpath):
|
||||||
|
filepath = PROJECT_ROOT / relpath
|
||||||
|
if not filepath.exists():
|
||||||
|
pytest.skip(f"{relpath} not found")
|
||||||
|
source = filepath.read_text(encoding="utf-8")
|
||||||
|
assert "_IS_WINDOWS" in source, (
|
||||||
|
f"{relpath} missing _IS_WINDOWS platform guard"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestKillpgGuarded:
|
||||||
|
"""os.killpg must always be behind a platform check."""
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("relpath", GUARDED_FILES)
|
||||||
|
def test_no_unguarded_killpg(self, relpath):
|
||||||
|
filepath = PROJECT_ROOT / relpath
|
||||||
|
if not filepath.exists():
|
||||||
|
pytest.skip(f"{relpath} not found")
|
||||||
|
source = filepath.read_text(encoding="utf-8")
|
||||||
|
lines = source.splitlines()
|
||||||
|
for i, line in enumerate(lines):
|
||||||
|
stripped = line.strip()
|
||||||
|
if "os.killpg" in stripped or "os.getpgid" in stripped:
|
||||||
|
# Check that there's an _IS_WINDOWS guard in the surrounding context
|
||||||
|
context = "\n".join(lines[max(0, i - 15):i + 1])
|
||||||
|
assert "_IS_WINDOWS" in context or "else:" in context, (
|
||||||
|
f"{relpath}:{i + 1} has unguarded os.killpg/os.getpgid call"
|
||||||
|
)
|
||||||
|
|
@ -20,6 +20,7 @@ Platform: Linux / macOS only (Unix domain sockets). Disabled on Windows.
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
import platform
|
||||||
import signal
|
import signal
|
||||||
import socket
|
import socket
|
||||||
import subprocess
|
import subprocess
|
||||||
|
|
@ -28,6 +29,8 @@ import tempfile
|
||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
import uuid
|
import uuid
|
||||||
|
|
||||||
|
_IS_WINDOWS = platform.system() == "Windows"
|
||||||
from typing import Any, Dict, List, Optional
|
from typing import Any, Dict, List, Optional
|
||||||
|
|
||||||
# Availability gate: UDS requires a POSIX OS
|
# Availability gate: UDS requires a POSIX OS
|
||||||
|
|
@ -405,7 +408,7 @@ def execute_code(
|
||||||
stdout=subprocess.PIPE,
|
stdout=subprocess.PIPE,
|
||||||
stderr=subprocess.PIPE,
|
stderr=subprocess.PIPE,
|
||||||
stdin=subprocess.DEVNULL,
|
stdin=subprocess.DEVNULL,
|
||||||
preexec_fn=os.setsid,
|
preexec_fn=None if _IS_WINDOWS else os.setsid,
|
||||||
)
|
)
|
||||||
|
|
||||||
# --- Poll loop: watch for exit, timeout, and interrupt ---
|
# --- Poll loop: watch for exit, timeout, and interrupt ---
|
||||||
|
|
@ -514,7 +517,10 @@ def execute_code(
|
||||||
def _kill_process_group(proc, escalate: bool = False):
|
def _kill_process_group(proc, escalate: bool = False):
|
||||||
"""Kill the child and its entire process group."""
|
"""Kill the child and its entire process group."""
|
||||||
try:
|
try:
|
||||||
os.killpg(os.getpgid(proc.pid), signal.SIGTERM)
|
if _IS_WINDOWS:
|
||||||
|
proc.terminate()
|
||||||
|
else:
|
||||||
|
os.killpg(os.getpgid(proc.pid), signal.SIGTERM)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
try:
|
try:
|
||||||
proc.kill()
|
proc.kill()
|
||||||
|
|
@ -527,7 +533,10 @@ def _kill_process_group(proc, escalate: bool = False):
|
||||||
proc.wait(timeout=5)
|
proc.wait(timeout=5)
|
||||||
except subprocess.TimeoutExpired:
|
except subprocess.TimeoutExpired:
|
||||||
try:
|
try:
|
||||||
os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
|
if _IS_WINDOWS:
|
||||||
|
proc.kill()
|
||||||
|
else:
|
||||||
|
os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
try:
|
try:
|
||||||
proc.kill()
|
proc.kill()
|
||||||
|
|
|
||||||
|
|
@ -1,12 +1,15 @@
|
||||||
"""Local execution environment with interrupt support and non-blocking I/O."""
|
"""Local execution environment with interrupt support and non-blocking I/O."""
|
||||||
|
|
||||||
import os
|
import os
|
||||||
|
import platform
|
||||||
import shutil
|
import shutil
|
||||||
import signal
|
import signal
|
||||||
import subprocess
|
import subprocess
|
||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
|
|
||||||
|
_IS_WINDOWS = platform.system() == "Windows"
|
||||||
|
|
||||||
from tools.environments.base import BaseEnvironment
|
from tools.environments.base import BaseEnvironment
|
||||||
|
|
||||||
# Noise lines emitted by interactive shells when stdin is not a terminal.
|
# Noise lines emitted by interactive shells when stdin is not a terminal.
|
||||||
|
|
@ -74,7 +77,7 @@ class LocalEnvironment(BaseEnvironment):
|
||||||
stdout=subprocess.PIPE,
|
stdout=subprocess.PIPE,
|
||||||
stderr=subprocess.STDOUT,
|
stderr=subprocess.STDOUT,
|
||||||
stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL,
|
stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL,
|
||||||
preexec_fn=os.setsid,
|
preexec_fn=None if _IS_WINDOWS else os.setsid,
|
||||||
)
|
)
|
||||||
|
|
||||||
if stdin_data is not None:
|
if stdin_data is not None:
|
||||||
|
|
@ -107,12 +110,15 @@ class LocalEnvironment(BaseEnvironment):
|
||||||
while proc.poll() is None:
|
while proc.poll() is None:
|
||||||
if _interrupt_event.is_set():
|
if _interrupt_event.is_set():
|
||||||
try:
|
try:
|
||||||
pgid = os.getpgid(proc.pid)
|
if _IS_WINDOWS:
|
||||||
os.killpg(pgid, signal.SIGTERM)
|
proc.terminate()
|
||||||
try:
|
else:
|
||||||
proc.wait(timeout=1.0)
|
pgid = os.getpgid(proc.pid)
|
||||||
except subprocess.TimeoutExpired:
|
os.killpg(pgid, signal.SIGTERM)
|
||||||
os.killpg(pgid, signal.SIGKILL)
|
try:
|
||||||
|
proc.wait(timeout=1.0)
|
||||||
|
except subprocess.TimeoutExpired:
|
||||||
|
os.killpg(pgid, signal.SIGKILL)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
proc.kill()
|
proc.kill()
|
||||||
reader.join(timeout=2)
|
reader.join(timeout=2)
|
||||||
|
|
@ -122,7 +128,10 @@ class LocalEnvironment(BaseEnvironment):
|
||||||
}
|
}
|
||||||
if time.monotonic() > deadline:
|
if time.monotonic() > deadline:
|
||||||
try:
|
try:
|
||||||
os.killpg(os.getpgid(proc.pid), signal.SIGTERM)
|
if _IS_WINDOWS:
|
||||||
|
proc.terminate()
|
||||||
|
else:
|
||||||
|
os.killpg(os.getpgid(proc.pid), signal.SIGTERM)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
proc.kill()
|
proc.kill()
|
||||||
reader.join(timeout=2)
|
reader.join(timeout=2)
|
||||||
|
|
|
||||||
|
|
@ -32,6 +32,7 @@ Usage:
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
import platform
|
||||||
import shlex
|
import shlex
|
||||||
import shutil
|
import shutil
|
||||||
import signal
|
import signal
|
||||||
|
|
@ -39,6 +40,8 @@ import subprocess
|
||||||
import threading
|
import threading
|
||||||
import time
|
import time
|
||||||
import uuid
|
import uuid
|
||||||
|
|
||||||
|
_IS_WINDOWS = platform.system() == "Windows"
|
||||||
from dataclasses import dataclass, field
|
from dataclasses import dataclass, field
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any, Dict, List, Optional
|
from typing import Any, Dict, List, Optional
|
||||||
|
|
@ -199,7 +202,7 @@ class ProcessRegistry:
|
||||||
stdout=subprocess.PIPE,
|
stdout=subprocess.PIPE,
|
||||||
stderr=subprocess.STDOUT,
|
stderr=subprocess.STDOUT,
|
||||||
stdin=subprocess.PIPE,
|
stdin=subprocess.PIPE,
|
||||||
preexec_fn=os.setsid,
|
preexec_fn=None if _IS_WINDOWS else os.setsid,
|
||||||
)
|
)
|
||||||
|
|
||||||
session.process = proc
|
session.process = proc
|
||||||
|
|
@ -551,7 +554,10 @@ class ProcessRegistry:
|
||||||
elif session.process:
|
elif session.process:
|
||||||
# Local process -- kill the process group
|
# Local process -- kill the process group
|
||||||
try:
|
try:
|
||||||
os.killpg(os.getpgid(session.process.pid), signal.SIGTERM)
|
if _IS_WINDOWS:
|
||||||
|
session.process.terminate()
|
||||||
|
else:
|
||||||
|
os.killpg(os.getpgid(session.process.pid), signal.SIGTERM)
|
||||||
except (ProcessLookupError, PermissionError):
|
except (ProcessLookupError, PermissionError):
|
||||||
session.process.kill()
|
session.process.kill()
|
||||||
elif session.env_ref and session.pid:
|
elif session.env_ref and session.pid:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue