fix(acp): address review findings in stale kill implementation

- Mirror DISABLE_STALE_ACP_AGENT_KILLS to os.environ in server_applicator
- Add top-level exception handler in _run() to prevent silent task failures
- Return True (proceed with kill) when psutil is unavailable but PID matches
- Document psutil as required dependency with defensive ImportError fallback

Author: Mateusz

docs/user_guide/backends/overview.md

@@ -61,7 +61,7 @@ To change the idle delay:
 - Environment: `STALE_ACP_AGENT_KILL_IDLE_SECONDS=<seconds>`
 - Configuration file: `stale_acp_agent_kill_idle_seconds: <seconds>`
 
-When **psutil** is installed (runtime dependency), before terminating a child the proxy checks that the OS process is still the same one it spawned (creation time and, when available, executable path), so an unrelated process that reused the PID is not killed. If psutil is missing, idle-kill falls back to the subprocess handle only (weaker).
+**psutil** is a required runtime dependency (declared in `pyproject.toml`). Before terminating a child, the proxy uses it to verify the OS process is still the same one it spawned (creation time and, when available, executable path), so an unrelated process that reused the PID is not killed. The code also has a defensive import fallback: if `psutil` cannot be imported at runtime, idle-kill falls back to the subprocess handle only (weaker).
 
 Precedence: **CLI** overrides **environment** overrides **configuration file**. INFO-level logs describe when a kill is scheduled, cancelled, or executed.
 

src/connectors/acp_core/acp_subprocess_identity.py

@@ -91,7 +91,7 @@ def stale_kill_still_same_os_process(
         return False
 
     if _psutil is None:
-        return False
+        return True
 
     try:
         current = _psutil.Process(identity.pid)

src/connectors/acp_core/base_connector.py

@@ -246,52 +246,67 @@ async def _schedule_stale_kill_after_turn(self, runtime: ACPProcessRuntime) -> N
 
         async def _run() -> None:
             try:
-                await asyncio.sleep(delay)
-            except asyncio.CancelledError:
-                return
-            if not connector._stale_acp_kill_enabled():
-                return
-            proc = runtime_ref.process
-            if proc is None or proc.poll() is not None:
-                return
-            ident = runtime_ref.acp_subprocess_identity
-            if ident is not None and not stale_kill_still_same_os_process(proc, ident):
-                if logger.isEnabledFor(logging.INFO):
-                    logger.info(
-                        "Stale ACP kill skipped: OS process identity mismatch "
-                        "(backend=%s pid=%s project=%s)",
+                try:
+                    await asyncio.sleep(delay)
+                except asyncio.CancelledError:
+                    return
+                if not connector._stale_acp_kill_enabled():
+                    return
+                proc = runtime_ref.process
+                if proc is None or proc.poll() is not None:
+                    return
+                ident = runtime_ref.acp_subprocess_identity
+                if ident is not None and not stale_kill_still_same_os_process(
+                    proc, ident
+                ):
+                    if logger.isEnabledFor(logging.INFO):
+                        logger.info(
+                            "Stale ACP kill skipped: OS process identity mismatch "
+                            "(backend=%s pid=%s project=%s)",
+                            connector.backend_type,
+                            getattr(proc, "pid", None),
+                            runtime_ref.project_dir,
+                        )
+                    return
+                if ident is None and logger.isEnabledFor(logging.DEBUG):
+                    logger.debug(
+                        "ACP stale kill has no subprocess identity fingerprint "
+                        "(backend=%s pid=%s); relying on subprocess handle state only",
                         connector.backend_type,
                         getattr(proc, "pid", None),
-                        runtime_ref.project_dir,
                     )
-                return
-            if ident is None and logger.isEnabledFor(logging.DEBUG):
-                logger.debug(
-                    "ACP stale kill has no subprocess identity fingerprint "
-                    "(backend=%s pid=%s); relying on subprocess handle state only",
-                    connector.backend_type,
-                    getattr(proc, "pid", None),
-                )
-            req_lock = runtime_ref.request_lock
-            if req_lock is not None and req_lock.locked():
+                req_lock = runtime_ref.request_lock
+                if req_lock is not None and req_lock.locked():
+                    if logger.isEnabledFor(logging.INFO):
+                        logger.info(
+                            "Stale ACP kill skipped: request in progress "
+                            "(backend=%s pid=%s)",
+                            connector.backend_type,
+                            getattr(proc, "pid", None),
+                        )
+                    return
                 if logger.isEnabledFor(logging.INFO):
                     logger.info(
-                        "Stale ACP kill skipped: request in progress (backend=%s pid=%s)",
+                        "Stale ACP agent idle timeout: terminating process (backend=%s "
+                        "pid=%s project=%s model=%s client_session=%s)",
                         connector.backend_type,
                         getattr(proc, "pid", None),
+                        runtime_ref.project_dir,
+                        runtime_ref.model,
+                        runtime_ref.client_session_id,
                     )
-                return
-            if logger.isEnabledFor(logging.INFO):
-                logger.info(
-                    "Stale ACP agent idle timeout: terminating process (backend=%s "
-                    "pid=%s project=%s model=%s client_session=%s)",
+                await connector._kill_runtime(runtime_ref)
+            except asyncio.CancelledError:
+                raise
+            except Exception:
+                logger.exception(
+                    "Stale ACP agent kill task failed (backend=%s project=%s model=%s "
+                    "client_session=%s)",
                     connector.backend_type,
-                    getattr(proc, "pid", None),
                     runtime_ref.project_dir,
                     runtime_ref.model,
                     runtime_ref.client_session_id,
                 )
-            await connector._kill_runtime(runtime_ref)
 
         runtime.stale_kill_task = asyncio.create_task(_run())
 

src/core/cli_support/applicators/server_applicator.py

@@ -258,6 +258,7 @@ def _apply_disable_stale_acp_agent_kills(
         if raw is not True:
             return
         overrides["disable_stale_acp_agent_kills"] = True
+        os.environ["DISABLE_STALE_ACP_AGENT_KILLS"] = "true"
         resolution.record(
             "disable_stale_acp_agent_kills",
             True,

tests/unit/connectors/acp_core/test_acp_stale_kill_timer.py

@@ -109,6 +109,35 @@ async def test_stale_kill_task_calls_kill_runtime_after_delay(
     mock_kill.assert_awaited_once_with(runtime)
 
 
+@pytest.mark.asyncio
+async def test_stale_kill_task_logs_exception_when_kill_runtime_raises(
+    connector: DummyAcpConnector,
+    runtime: ACPProcessRuntime,
+) -> None:
+    mock_proc = MagicMock()
+    mock_proc.poll.return_value = None
+    mock_proc.pid = 4242
+    runtime.process = mock_proc
+    with (
+        patch.object(
+            connector,
+            "_stale_acp_kill_delay_seconds",
+            return_value=0.01,
+        ),
+        patch.object(
+            connector,
+            "_kill_runtime",
+            new_callable=AsyncMock,
+            side_effect=RuntimeError("boom"),
+        ),
+        patch("src.connectors.acp_core.base_connector.logger") as mock_logger,
+    ):
+        await connector._schedule_stale_kill_after_turn(runtime)
+        assert runtime.stale_kill_task is not None
+        await asyncio.wait_for(runtime.stale_kill_task, timeout=2.0)
+    mock_logger.exception.assert_called_once()
+
+
 @pytest.mark.asyncio
 async def test_stale_kill_skipped_when_identity_mismatch(
     connector: DummyAcpConnector,

tests/unit/connectors/acp_core/test_acp_subprocess_identity.py

@@ -92,3 +92,11 @@ def test_stale_kill_rejects_exe_mismatch(mock_popen: MagicMock) -> None:
 
 def test_stale_kill_false_when_identity_none(mock_popen: MagicMock) -> None:
     assert stale_kill_still_same_os_process(mock_popen, None) is False
+
+
+def test_stale_kill_true_when_psutil_unavailable_but_identity_present(
+    mock_popen: MagicMock,
+) -> None:
+    ident = AcpSubprocessIdentity(pid=9001, create_time=1.0, exe_key="a.exe")
+    with patch.object(sid_mod, "_psutil", None):
+        assert stale_kill_still_same_os_process(mock_popen, ident) is True

tests/unit/core/cli_support/applicators/test_server_applicator.py

@@ -249,12 +249,14 @@ def test_apply_disable_stale_acp_agent_kills(
     ) -> None:
         """Test that --disable-stale-acp-agent-kills is applied."""
         empty_args.disable_stale_acp_agent_kills = True
-        applicator.apply(empty_args, overrides, resolution)
+        with mock.patch.dict(os.environ, {}, clear=True):
+            applicator.apply(empty_args, overrides, resolution)
 
-        assert overrides.get("disable_stale_acp_agent_kills") is True
-        assert resolution.is_set("disable_stale_acp_agent_kills")
-        cli_params = resolution.latest_by_source(ParameterSource.CLI)
-        assert "disable_stale_acp_agent_kills" in cli_params
+            assert overrides.get("disable_stale_acp_agent_kills") is True
+            assert os.environ.get("DISABLE_STALE_ACP_AGENT_KILLS") == "true"
+            assert resolution.is_set("disable_stale_acp_agent_kills")
+            cli_params = resolution.latest_by_source(ParameterSource.CLI)
+            assert "disable_stale_acp_agent_kills" in cli_params
 
     def test_apply_stale_acp_agent_kill_idle_seconds(
         self,