Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
_SETUP_DELAY_SECONDS = 0.5
_SETUP_POLL_INTERVAL_SECONDS = 0.05
_MAX_SETUP_WAIT_SECONDS = 2.0
_INTERRUPT_GRACE_SECONDS = 0.5

_WINDOWS_SPECIALS: dict[str, str] = {
"ENTER": "\n",
Expand Down Expand Up @@ -172,6 +173,7 @@ def close(self) -> None:
return

self._stop_reader.set()
self._terminate_child_processes()

if self.process is not None:
try:
Expand Down Expand Up @@ -327,6 +329,62 @@ def clear_screen(self) -> None:
time.sleep(_SCREEN_CLEAR_DELAY_SECONDS)
self._command_running_event.clear()

def _terminate_child_processes(self) -> bool:
"""Terminate descendants of the persistent PowerShell process."""
if (
platform.system() != "Windows"
or self.process is None
or self.process.poll() is not None
):
return False

script = f"""
$root = {self.process.pid}
$childrenByParent = @{{}}
Get-CimInstance Win32_Process | ForEach-Object {{
$parentId = [int]$_.ParentProcessId
if (-not $childrenByParent.ContainsKey($parentId)) {{
$childrenByParent[$parentId] = New-Object System.Collections.Generic.List[int]
}}
$childrenByParent[$parentId].Add([int]$_.ProcessId)
}}
$toStop = New-Object System.Collections.Generic.List[int]
function Add-Descendants([int]$processId) {{
if (-not $childrenByParent.ContainsKey($processId)) {{ return }}
foreach ($childId in $childrenByParent[$processId]) {{
if ($childId -eq $PID) {{ continue }}
$toStop.Add($childId)
Add-Descendants $childId
}}
}}
Add-Descendants $root
for ($i = $toStop.Count - 1; $i -ge 0; $i--) {{
Stop-Process -Id $toStop[$i] -Force -ErrorAction SilentlyContinue
}}
if ($toStop.Count -gt 0) {{ exit 0 }} else {{ exit 1 }}
"""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Get-CimInstance Win32_Process enumerates all processes on the system. On machines with thousands of processes this could take 1-2+ seconds. Consider documenting expected latency in the method docstring, or adding a timeout check if this becomes a production issue.

Not blocking - this only runs during interrupt/close, so the overhead is acceptable for fixing the cleanup bug.

startupinfo = None
startupinfo_cls = getattr(subprocess, "STARTUPINFO", None)
if startupinfo_cls is not None:
startupinfo = startupinfo_cls()
startupinfo.dwFlags |= getattr(subprocess, "STARTF_USESHOWWINDOW", 0)
creationflags = getattr(subprocess, "CREATE_NO_WINDOW", 0)

try:
result = subprocess.run(
[self.shell_path, "-NoLogo", "-NoProfile", "-Command", script],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=False,
timeout=5.0,
startupinfo=startupinfo,
creationflags=creationflags,
)
return result.returncode == 0
except (subprocess.TimeoutExpired, OSError) as exc:
logger.debug("Failed to terminate PowerShell child processes: %s", exc)
return False

def interrupt(self) -> bool:
"""Interrupt the active command if the process is still alive."""
if self.process is None or self.process.poll() is not None:
Expand All @@ -341,15 +399,21 @@ def interrupt(self) -> bool:
except Exception as exc:
logger.debug("Failed to send CTRL_BREAK_EVENT: %s", exc)

if not sent_ctrl_break:
if sent_ctrl_break:
time.sleep(_INTERRUPT_GRACE_SECONDS)

terminated_children = self._terminate_child_processes()
sent_ctrl_c_input = False
if not sent_ctrl_break and not terminated_children:
try:
self._write_to_stdin(_WINDOWS_SPECIALS["C-C"])
sent_ctrl_c_input = True
except RuntimeError as exc:
logger.debug("Failed to write Ctrl+C to PowerShell stdin: %s", exc)
return False

self._command_running_event.clear()
return True
return sent_ctrl_break or terminated_children or sent_ctrl_c_input

def is_running(self) -> bool:
"""Return whether a command is still running in the PowerShell session."""
Expand Down
3 changes: 2 additions & 1 deletion tests/tools/browser_use/test_browser_toolset.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ def _mock_browser_executor_init():
def fake_init(self, **_kwargs):
self.full_output_save_dir = None
self._initialized = False
self._cleanup_initiated = False
# Toolset tests never allocate browser resources; keep close() a no-op.
self._cleanup_initiated = True
self._action_timeout_seconds = 30.0
self._async_executor = MagicMock()
self._async_executor.close = MagicMock()
Expand Down
106 changes: 106 additions & 0 deletions tests/tools/terminal/test_windows_ctrl_c.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
"""Windows-specific terminal interrupt behavior tests."""

import platform
import subprocess

import pytest

from openhands.tools.terminal.definition import TerminalAction
from openhands.tools.terminal.terminal import create_terminal_session
from openhands.tools.terminal.terminal.terminal_session import TerminalCommandStatus


pytestmark = pytest.mark.skipif(
platform.system() != "Windows",
reason="Windows CTRL_BREAK/PowerShell process behavior only applies on Windows",
)


def _powershell_process_exists(pid: int) -> bool:
result = subprocess.run(
[
"powershell.exe",
"-NoLogo",
"-NoProfile",
"-Command",
(
f"if (Get-Process -Id {pid} -ErrorAction SilentlyContinue) "
"{ exit 0 } else { exit 1 }"
),
],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=False,
)
return result.returncode == 0


def _stop_powershell_process(pid: int) -> None:
subprocess.run(
[
"powershell.exe",
"-NoLogo",
"-NoProfile",
"-Command",
f"Stop-Process -Id {pid} -Force -ErrorAction SilentlyContinue",
],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=False,
)


@pytest.mark.timeout(20)
def test_windows_ctrl_c_interrupt_kills_child_process_tree(tmp_path) -> None:
"""Ctrl-C after a timeout should stop the process that kept the command alive.

This captures the behavior promised by the timeout prompt. The current
PowerShell backend sends CTRL_BREAK to the persistent PowerShell process, but
does not ensure child processes launched by the command are terminated.
"""
pid_path = tmp_path / "child.pid"
script_path = tmp_path / "wait_on_child.ps1"
script_path.write_text(
"\n".join(
[
f"$pidPath = '{pid_path.as_posix()}'",
"$child = Start-Process -FilePath powershell.exe "
"-ArgumentList '-NoLogo','-NoProfile','-Command',"
"'Start-Sleep -Seconds 120' -PassThru",
"Set-Content -LiteralPath $pidPath -Value $child.Id",
"Wait-Process -Id $child.Id",
]
),
encoding="utf-8",
)

session = create_terminal_session(
work_dir=str(tmp_path),
terminal_type="powershell",
no_change_timeout_seconds=1,
)
child_pid: int | None = None
child_was_still_running = False
try:
session.initialize()

obs = session.execute(TerminalAction(command=f"& '{script_path.as_posix()}'"))

assert obs.metadata.exit_code == -1
assert session.prev_status == TerminalCommandStatus.NO_CHANGE_TIMEOUT
assert pid_path.exists()
child_pid = int(pid_path.read_text(encoding="utf-8").strip())
assert _powershell_process_exists(child_pid)

session.execute(TerminalAction(command="C-c", is_input=True, timeout=3))

child_was_still_running = _powershell_process_exists(child_pid)
finally:
if child_pid is not None:
_stop_powershell_process(child_pid)
session.close()

assert not child_was_still_running, (
"Windows Ctrl-C reported through the terminal did not terminate the "
"child process that kept the timed-out command alive."
)
Loading