diff --git a/docs/75-example-coding-agent.md b/docs/75-example-coding-agent.md index 510416ec6..d43e049af 100644 --- a/docs/75-example-coding-agent.md +++ b/docs/75-example-coding-agent.md @@ -66,6 +66,12 @@ This command installs the OpenAI client into each required SWE-smith base image ## Training +Shell commands run inside the Linux repository containers. `SMITH_CMD_TIMEOUT` +(default: 120 seconds) limits each agent command; `SMITH_EVAL_TIMEOUT` (default: +600 seconds) limits the final pytest run. On timeout, the harness terminates the +command's process group, including child workers, before continuing and reports +exit code 124. + The distributed launcher has three roles and must be started in this order: ```text diff --git a/examples/swe_smith/agents/smith_agent.py b/examples/swe_smith/agents/smith_agent.py index aff033634..6c99c680e 100755 --- a/examples/swe_smith/agents/smith_agent.py +++ b/examples/swe_smith/agents/smith_agent.py @@ -4,6 +4,7 @@ from __future__ import annotations +import contextlib import http.client import json import logging @@ -11,6 +12,7 @@ import random import re import shutil +import signal import subprocess import sys import time @@ -340,17 +342,27 @@ def _agent_env() -> dict[str, str]: def _run(command: str, timeout: int) -> tuple[str, int]: try: - proc = subprocess.run( + with subprocess.Popen( ["bash", "-c", command], cwd=TESTBED, - capture_output=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, - timeout=timeout, env=_agent_env(), - ) - return proc.stdout + proc.stderr, proc.returncode - except subprocess.TimeoutExpired: - return f"[timed out after {timeout}s]", 124 + start_new_session=True, + ) as proc: + try: + stdout, stderr = proc.communicate(timeout=timeout) + except subprocess.TimeoutExpired: + # Kill shell descendants before another action or evaluation + # starts, including workers still holding the output pipes. + with contextlib.suppress(ProcessLookupError): + os.killpg(proc.pid, signal.SIGKILL) + # Timeout observations discard partial output. Reap the shell + # without decoding it; the context manager closes the pipes. + proc.wait() + return f"[timed out after {timeout}s]", 124 + return stdout + stderr, proc.returncode except Exception as exc: return f"[failed to start: {exc}]", 1 diff --git a/tests/examples/test_swe_smith_command_timeout.py b/tests/examples/test_swe_smith_command_timeout.py new file mode 100644 index 000000000..4a3e74999 --- /dev/null +++ b/tests/examples/test_swe_smith_command_timeout.py @@ -0,0 +1,95 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Timed-out shell actions must not keep running during later agent turns.""" + +from __future__ import annotations + +import os +import shlex +import signal +import sys +import time + +import pytest + +from examples.swe_smith.agents import smith_agent + +pytestmark = pytest.mark.skipif(os.name != "posix", reason="The SWE-smith harness runs in POSIX containers") + + +@pytest.mark.parametrize("shell_exits_first", [False, True], ids=["foreground-command", "inherited-output-pipes"]) +def test_timeout_stops_child_and_grandchild(monkeypatch, tmp_path, shell_exits_first): + script = tmp_path / "command.py" + script.write_text( + "import os, pathlib, subprocess, sys, time\n" + "root = pathlib.Path(__file__).parent\n" + "role = sys.argv[1]\n" + "(root / (role + '.ready')).write_text(str(os.getpid()))\n" + "child = subprocess.Popen([sys.executable, __file__, 'grandchild']) if role == 'child' else None\n" + "while not (root / 'release').exists():\n" + " time.sleep(0.01)\n" + "(root / (role + '.late')).write_text('changed after timeout')\n" + "if child is not None:\n" + " child.wait()\n" + ) + monkeypatch.setattr(smith_agent, "TESTBED", str(tmp_path)) + command = f"{shlex.quote(sys.executable)} {shlex.quote(str(script))} child" + # Keep a foreground shell alive, or let it exit while descendants hold the + # captured pipes open. Both cases must terminate the entire command group. + command += " &" if shell_exits_first else " && echo done" + + try: + start = time.monotonic() + output, returncode = smith_agent._run(command, timeout=1) + elapsed = time.monotonic() - start + assert (output, returncode) == ("[timed out after 1s]", 124) + assert elapsed < 4 + assert (tmp_path / "child.ready").exists() + assert (tmp_path / "grandchild.ready").exists() + + # A surviving descendant can now mutate the worktree in the next turn. + (tmp_path / "release").touch() + deadline = time.monotonic() + 1 + while time.monotonic() < deadline and not list(tmp_path.glob("*.late")): + time.sleep(0.01) + assert list(tmp_path.glob("*.late")) == [] + finally: + # Let the original implementation's surviving processes exit as well, + # so running the regression before the fix leaves no waiting children. + (tmp_path / "release").touch() + + +def test_timeout_tolerates_process_group_exit_race(monkeypatch, tmp_path): + monkeypatch.setattr(smith_agent, "TESTBED", str(tmp_path)) + killpg = os.killpg + + def already_exited(process_group: int, sig: int) -> None: + assert sig == signal.SIGKILL + killpg(process_group, sig) + raise ProcessLookupError("group exited before signal delivery") + + monkeypatch.setattr(os, "killpg", already_exited) + command = f"{shlex.quote(sys.executable)} -c 'import time; time.sleep(30)'" + + assert smith_agent._run(command, timeout=1) == ("[timed out after 1s]", 124) + + +def test_normal_command_preserves_output_and_exit_status(monkeypatch, tmp_path): + monkeypatch.setattr(smith_agent, "TESTBED", str(tmp_path)) + + assert smith_agent._run("printf stdout; printf stderr >&2; exit 7", timeout=5) == ("stdoutstderr", 7) + + +def test_timeout_discards_partial_output_without_decoding_it(monkeypatch, tmp_path): + monkeypatch.setattr(smith_agent, "TESTBED", str(tmp_path)) + + assert smith_agent._run("printf '\\377'; sleep 30", timeout=1) == ("[timed out after 1s]", 124) + + +def test_command_start_failure_is_reported(monkeypatch, tmp_path): + monkeypatch.setattr(smith_agent, "TESTBED", str(tmp_path / "missing")) + + output, returncode = smith_agent._run("true", timeout=5) + + assert returncode == 1 + assert output.startswith("[failed to start:")