fix: reduce container timeout to 30 min and fix orphaned containers on timeout

docs/developer-guide.md

@@ -175,7 +175,7 @@ REDIS_URL=redis://localhost:6380/0   # matches docker-compose port mapping
 
 ```bash
 CONTAINER_IMAGE=localhost/forge-dev:latest   # built with podman above
-CONTAINER_TIMEOUT=7200                        # 2 hours max
+CONTAINER_TIMEOUT=1800                        # 30 minutes max
 CONTAINER_MEMORY=4g
 CONTAINER_CPUS=2
 ```

src/forge/config.py

@@ -300,8 +300,8 @@ def ignored_ci_checks(self) -> list[str]:
         description="Container image for task execution (local or registry URL)",
     )
     container_timeout: int = Field(
-        default=7200,
-        description="Container execution timeout in seconds (default: 2 hours)",
+        default=1800,
+        description="Container execution timeout in seconds (default: 30 minutes)",
     )
     container_memory: str = Field(
         default="4g",

src/forge/sandbox/runner.py

@@ -59,7 +59,7 @@ class ContainerConfig:
     """Configuration for container execution."""
 
     image: str = DEFAULT_IMAGE
-    timeout_seconds: int = 7200  # 2 hours default
+    timeout_seconds: int = 1800  # 30 minutes default
     memory_limit: str = "4g"
     cpu_limit: str = "2"
     network_mode: str = "slirp4netns"  # Rootless networking
@@ -314,6 +314,55 @@ def _build_podman_command(
 
         return cmd
 
+    async def _stop_timed_out_container(
+        self,
+        container_name: str,
+        process: asyncio.subprocess.Process,
+    ) -> None:
+        """Stop a running container and ensure the podman run process exits."""
+        stop_process = await asyncio.create_subprocess_exec(
+            "podman",
+            "stop",
+            "-t",
+            "10",
+            container_name,
+            stdout=asyncio.subprocess.DEVNULL,
+            stderr=asyncio.subprocess.DEVNULL,
+        )
+
+        should_kill = False
+        try:
+            await asyncio.wait_for(stop_process.wait(), timeout=15.0)
+            if stop_process.returncode != 0:
+                logger.warning(
+                    f"podman stop failed for {container_name} "
+                    f"(exit {stop_process.returncode}), killing"
+                )
+                should_kill = True
+        except TimeoutError:
+            logger.warning(f"Container {container_name} didn't stop, killing")
+            should_kill = True
+
+        if should_kill:
+            kill_process = await asyncio.create_subprocess_exec(
+                "podman",
+                "kill",
+                container_name,
+                stdout=asyncio.subprocess.DEVNULL,
+                stderr=asyncio.subprocess.DEVNULL,
+            )
+            try:
+                await asyncio.wait_for(kill_process.wait(), timeout=15.0)
+            except TimeoutError:
+                logger.warning(f"podman kill for {container_name} did not finish")
+
+        try:
+            await asyncio.wait_for(process.wait(), timeout=15.0)
+        except TimeoutError:
+            logger.warning(f"podman run process for {container_name} did not exit, killing")
+            process.kill()
+            await process.wait()
+
     async def run(
         self,
         workspace_path: Path,
@@ -377,9 +426,8 @@ async def run(
                     timeout=config.timeout_seconds + 60,  # Extra buffer
                 )
             except TimeoutError:
-                logger.error("Container execution timed out")
-                process.kill()
-                await process.wait()
+                logger.error(f"Container execution timed out, stopping {container_name}")
+                await self._stop_timed_out_container(container_name, process)
                 return ContainerResult(
                     success=False,
                     exit_code=-1,
@@ -389,30 +437,7 @@ async def run(
                 )
             except asyncio.CancelledError:
                 logger.warning(f"Container execution cancelled, stopping {container_name}")
-                # Stop the container via podman (more reliable than killing process)
-                stop_process = await asyncio.create_subprocess_exec(
-                    "podman",
-                    "stop",
-                    "-t",
-                    "10",
-                    container_name,
-                    stdout=asyncio.subprocess.DEVNULL,
-                    stderr=asyncio.subprocess.DEVNULL,
-                )
-                try:
-                    await asyncio.wait_for(stop_process.wait(), timeout=15.0)
-                except TimeoutError:
-                    logger.warning(f"Container {container_name} didn't stop, killing")
-                    kill_process = await asyncio.create_subprocess_exec(
-                        "podman",
-                        "kill",
-                        container_name,
-                        stdout=asyncio.subprocess.DEVNULL,
-                        stderr=asyncio.subprocess.DEVNULL,
-                    )
-                    await kill_process.wait()
-                # Wait for the original process to finish
-                await process.wait()
+                await self._stop_timed_out_container(container_name, process)
                 raise  # Re-raise CancelledError
 
             exit_code = process.returncode or 0

tests/test_sandbox_runner.py

@@ -63,8 +63,8 @@ async def test_simple_container_run(self):
     async def test_container_config_defaults(self):
         """Test ContainerConfig has sensible defaults."""
         config = ContainerConfig()
-        assert config.image == "forge-dev:latest"
-        assert config.timeout_seconds == 7200  # 2 hours
+        assert config.image == "localhost/forge-dev:latest"
+        assert config.timeout_seconds == 1800  # 30 minutes
         assert config.memory_limit == "4g"
         assert config.cpu_limit == "2"
         assert config.skip_tests is False

tests/unit/sandbox/test_runner_timeout_cleanup.py

@@ -0,0 +1,70 @@
+"""Tests for container runner timeout cleanup."""
+
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+from forge.sandbox.runner import ContainerRunner
+
+
+def _runner_without_init() -> ContainerRunner:
+    return object.__new__(ContainerRunner)
+
+
+@pytest.mark.asyncio
+async def test_stop_failure_kills_container_and_waits_for_run_process() -> None:
+    runner = _runner_without_init()
+    stop_process = MagicMock()
+    stop_process.returncode = 1
+    stop_process.wait = AsyncMock()
+    kill_process = MagicMock()
+    kill_process.wait = AsyncMock()
+    run_process = MagicMock()
+    run_process.wait = AsyncMock()
+    run_process.kill = MagicMock()
+
+    with patch(
+        "forge.sandbox.runner.asyncio.create_subprocess_exec",
+        new=AsyncMock(side_effect=[stop_process, kill_process]),
+    ) as mock_exec:
+        await runner._stop_timed_out_container("forge-ticket-abc123", run_process)
+
+    assert mock_exec.call_count == 2
+    assert mock_exec.call_args_list[0].args[:3] == ("podman", "stop", "-t")
+    assert mock_exec.call_args_list[1].args[:2] == ("podman", "kill")
+    run_process.wait.assert_awaited()
+    run_process.kill.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_run_process_wait_timeout_kills_run_process() -> None:
+    runner = _runner_without_init()
+    stop_process = MagicMock()
+    stop_process.returncode = 0
+    stop_process.wait = AsyncMock()
+    run_process = MagicMock()
+    run_process.wait = AsyncMock()
+    run_process.kill = MagicMock()
+    calls = 0
+
+    async def fake_wait_for(awaitable, timeout):  # noqa: ANN001, ARG001
+        nonlocal calls
+        calls += 1
+        if calls == 2:
+            awaitable.close()
+            raise TimeoutError
+        return await awaitable
+
+    with (
+        patch(
+            "forge.sandbox.runner.asyncio.create_subprocess_exec",
+            new=AsyncMock(return_value=stop_process),
+        ) as mock_exec,
+        patch("forge.sandbox.runner.asyncio.wait_for", side_effect=fake_wait_for),
+    ):
+        await runner._stop_timed_out_container("forge-ticket-abc123", run_process)
+
+    mock_exec.assert_awaited_once()
+    run_process.kill.assert_called_once()
+    assert run_process.wait.call_count == 2
+    assert run_process.wait.await_count == 1