From 0fa53b0adef1c1036ffba92de75732d25bac6adb Mon Sep 17 00:00:00 2001 From: ppcvote Date: Thu, 30 Jul 2026 15:48:49 +0800 Subject: [PATCH] fix(agent-cli): Windows temp-cwd cleanup must not fail a successful batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, run_agent_cli's TemporaryDirectory raised WinError 32 at __exit__ whenever any live process still held the per-invocation temp cwd — and the agent CLI's process tree routinely does: the binary is a .cmd shim whose node children can outlive the direct child, and a killed-but-unreaped child on the timeout path holds its cwd too. A directory that is any live process's working directory (or contains any open handle) cannot be removed on Windows, so cleanup raised *after* the model had already answered, and llm_analyzer_base recorded the batch as llm_batch_failed. Under batch concurrency the teardown window widens, which is why multi-file scans failed while single-file scans passed (#315). Each invocation already has a unique mkdtemp directory, so this was never a cross-worker path collision; it is delete-at-exit racing the process tree's teardown. Fix: mkdtemp + explicit best-effort cleanup. _cleanup_temp_dir retries briefly (10 x 0.2s) to reclaim the directory once the holder exits, then leaks it with a warning instead of raising. A cleanup failure never outranks a successful response. Verified on Windows 10 with a mechanism-faithful fake CLI (a .cmd shim that detaches a grandchild holding the temp cwd): 8 concurrent batches fail 8/8 with the exact WinError 32 signature from #315 on main, and pass 8/8 with this change. Controlled experiments confirm both hold modes (another process's cwd; an open file handle inside the dir) block rmtree with WinError 32. Tests: 5 new cases including a Windows-only real-handle hold and a regression test asserting run_agent_cli returns the response when rmtree keeps failing. tests/unit/test_agent_cli.py 87/87 on Windows. Closes #315 Co-Authored-By: Claude Fable 5 Signed-off-by: ppcvote --- src/skillspector/providers/_agent_cli.py | 41 ++++++++++- tests/unit/test_agent_cli.py | 92 ++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/src/skillspector/providers/_agent_cli.py b/src/skillspector/providers/_agent_cli.py index 1ee1ab22..26ad4e97 100644 --- a/src/skillspector/providers/_agent_cli.py +++ b/src/skillspector/providers/_agent_cli.py @@ -53,6 +53,7 @@ import subprocess import tempfile import threading +import time from collections.abc import Callable from dataclasses import dataclass from typing import Any @@ -632,6 +633,37 @@ def _drain_stream(stream: Any, buf: bytearray, cap: int, on_overflow: Any) -> No pass +_CLEANUP_RETRIES = 10 +_CLEANUP_RETRY_DELAY_S = 0.2 + + +def _cleanup_temp_dir(path: str) -> None: + """Best-effort removal of the per-invocation temp cwd. + + On Windows a directory cannot be deleted while any process has it as its + working directory or holds a handle inside it. The agent CLI's process + tree can outlive the direct child by a beat (a ``.cmd`` shim's node child, + or a killed-but-not-reaped child on the timeout path), so the first + attempts may fail transiently with ``WinError 32``. Retry briefly, then + leak the directory with a warning rather than raise: by the time cleanup + runs the model's response is already in hand, and a temp-dir leak must + never fail the batch (#315). + """ + for _ in range(_CLEANUP_RETRIES): + try: + shutil.rmtree(path) + return + except OSError: + time.sleep(_CLEANUP_RETRY_DELAY_S) + shutil.rmtree(path, ignore_errors=True) + if os.path.isdir(path): + logger.warning( + "Could not remove temp dir %s (handle still held by the CLI " + "process tree?); leaking it rather than failing the batch", + path, + ) + + def _run_bounded( proc: subprocess.Popen, prompt_bytes: bytes, timeout: float ) -> tuple[int | None, bytes, bytes, bool]: @@ -767,7 +799,12 @@ def run_agent_cli( child_env = _scrub_env() # -- Run in a temporary directory (no CWD access) ------------------------- - with tempfile.TemporaryDirectory(prefix="skillspector_cli_") as tmp_cwd: + # mkdtemp + explicit best-effort cleanup instead of TemporaryDirectory: + # the context manager's rmtree-at-__exit__ raises on Windows while the + # CLI's process tree still holds the directory as its cwd, turning an + # already-successful call into a batch failure (#315). + tmp_cwd = tempfile.mkdtemp(prefix="skillspector_cli_") + try: logger.debug( "Running %s argv=%r cwd=%s timeout=%ss", binary_name, @@ -792,6 +829,8 @@ def run_agent_cli( # CLI cannot exhaust memory before the cap is enforced (a chatty child # could otherwise buffer unbounded output until the timeout). returncode, stdout_raw, stderr_raw, overflow = _run_bounded(proc, prompt_bytes, timeout) + finally: + _cleanup_temp_dir(tmp_cwd) # -- Fail-closed checks --------------------------------------------------- if overflow: diff --git a/tests/unit/test_agent_cli.py b/tests/unit/test_agent_cli.py index c47cffea..27244378 100644 --- a/tests/unit/test_agent_cli.py +++ b/tests/unit/test_agent_cli.py @@ -733,3 +733,95 @@ def test_is_available_false_even_when_binary_present( ok, reason = _agent_cli.is_available("agy") assert ok is False assert "disabled" in (reason or "") + + +# --------------------------------------------------------------------------- +# _cleanup_temp_dir — Windows-safe temp cwd removal (#315) +# --------------------------------------------------------------------------- + + +class TestCleanupTempDir: + """Cleanup failure must never outrank a successful CLI response (#315). + + On Windows the CLI's process tree can hold the temp cwd (a ``.cmd`` + shim's node child, or a killed child on the timeout path), so rmtree can + fail transiently — or persistently — after the model already answered. + """ + + def test_removes_directory(self, tmp_path) -> None: + target = tmp_path / "cli_cwd" + target.mkdir() + (target / "scratch.txt").write_text("x") + _agent_cli._cleanup_temp_dir(str(target)) + assert not target.exists() + + def test_retries_transient_failure_then_succeeds( + self, tmp_path, monkeypatch: pytest.MonkeyPatch + ) -> None: + target = tmp_path / "cli_cwd" + target.mkdir() + real_rmtree = _agent_cli.shutil.rmtree + calls = {"n": 0} + + def flaky_rmtree(path, ignore_errors=False): + calls["n"] += 1 + if calls["n"] < 3: + raise OSError(32, "held by another process") + return real_rmtree(path, ignore_errors=ignore_errors) + + monkeypatch.setattr(_agent_cli.shutil, "rmtree", flaky_rmtree) + monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None) + _agent_cli._cleanup_temp_dir(str(target)) + assert not target.exists() + assert calls["n"] == 3 + + def test_never_raises_when_removal_keeps_failing( + self, tmp_path, monkeypatch: pytest.MonkeyPatch + ) -> None: + target = tmp_path / "cli_cwd" + target.mkdir() + + def stuck_rmtree(path, ignore_errors=False): + if not ignore_errors: + raise OSError(32, "held by another process") + + monkeypatch.setattr(_agent_cli.shutil, "rmtree", stuck_rmtree) + monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None) + # Must not raise; the directory is leaked deliberately. + _agent_cli._cleanup_temp_dir(str(target)) + assert target.exists() + + @pytest.mark.skipif(sys.platform != "win32", reason="Windows handle semantics") + def test_open_handle_does_not_raise_on_windows( + self, tmp_path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A real open handle inside the dir — the exact WinError 32 case.""" + target = tmp_path / "cli_cwd" + target.mkdir() + monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None) + held = open(target / "held.txt", "w") # noqa: SIM115 — handle held on purpose + try: + _agent_cli._cleanup_temp_dir(str(target)) # must not raise + finally: + held.close() + _agent_cli._cleanup_temp_dir(str(target)) + assert not target.exists() + + +@patch("skillspector.providers._agent_cli.find_binary", return_value=CLAUDE_BINARY) +@patch("skillspector.providers._agent_cli.subprocess.Popen") +class TestRunAgentCLISurvivesCleanupFailure: + def test_response_returned_when_temp_dir_cannot_be_removed( + self, mock_popen: MagicMock, _mock_binary: MagicMock, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Regression for #315: rmtree failure must not fail the batch.""" + mock_popen.return_value = _make_ok_process(_GOOD_CLAUDE_OUTPUT.encode()) + + def stuck_rmtree(path, ignore_errors=False): + if not ignore_errors: + raise OSError(32, "held by another process") + + monkeypatch.setattr(_agent_cli.shutil, "rmtree", stuck_rmtree) + monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None) + result = run_agent_cli("claude", PROMPT, model=MODEL) + assert result # the model's answer survives the cleanup failure