Skip to content

fix(agent-cli): Windows temp-cwd cleanup must not fail a successful batch - #317

Open
ppcvote wants to merge 1 commit into
NVIDIA:mainfrom
ppcvote:fix/windows-temp-cwd-cleanup
Open

fix(agent-cli): Windows temp-cwd cleanup must not fail a successful batch#317
ppcvote wants to merge 1 commit into
NVIDIA:mainfrom
ppcvote:fix/windows-temp-cwd-cleanup

Conversation

@ppcvote

@ppcvote ppcvote commented Jul 30, 2026

Copy link
Copy Markdown

Closes #315.

Root cause — one correction to the issue's hypothesis

It isn't a cross-worker collision on a shared temp file: every invocation already gets a unique directory (tempfile.TemporaryDirectory(prefix="skillspector_cli_")mkdtemp under the hood). The race is delete-at-__exit__ vs the CLI's process-tree teardown. On Windows, a directory cannot be removed while any live process has it as its working directory or holds any handle inside it — I verified both hold modes raise exactly WinError 32 in controlled experiments. The claude binary on Windows is a .cmd shim that spawns node children; when any of that tree outlives the direct child (or a killed-but-unreaped child lingers on the timeout path), rmtree at __exit__ throws after the model has already answered, and llm_analyzer_base records the batch as llm_batch_failed. Concurrency widens the teardown window, which is why the issue's batch runs fail while one-file-at-a-time runs pass, and why the failing set varies between runs.

Fix

mkdtemp + explicit best-effort cleanup in a finally: _cleanup_temp_dir retries briefly (10 × 0.2 s — enough for the common case where the holder exits within a beat), then falls back to ignore_errors and leaks the directory with a warning. A cleanup failure never outranks a successful response. Detection/scan behavior is untouched; concurrency is preserved (no serialization).

Verification on Windows 10

Mechanism-faithful repro: a fake claude.cmd that answers normally on stdout but detaches a grandchild whose CWD is the temp dir (via Start-Process, so no pipe-handle inheritance — matching how the real CLI's children behave), 8 concurrent batches through run_agent_cli:

build result
main 8/8 batches fail, PermissionError: [WinError 32] ... 'C:\...\skillspector_cli_<rand>' — the exact signature from #315
this branch 8/8 batches succeed; dirs reclaimed by retry when the holder exits quickly, else leaked with one warning

Controlled experiments (independent of the shim) confirm each mechanism: another process's CWD blocks rmtree (WinError 32 naming the directory — matching the issue's error, which names the directory rather than a file); an open file handle inside blocks it too (naming the file).

Tests

5 new cases in tests/unit/test_agent_cli.py: happy-path removal, transient-failure retry, never-raises-when-stuck (deliberate leak), a Windows-only real open-handle hold, and a regression test asserting run_agent_cli returns the model's response even when rmtree keeps failing. 87/87 pass on Windows — which also exercises the suite on the platform this provider was breaking on (4 pre-existing, unrelated Windows failures elsewhere in the full suite reproduce identically on a clean main; detailed in #316).

🤖 Generated with Claude Code

…atch

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
(NVIDIA#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 NVIDIA#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 NVIDIA#315

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ppcvote <risky9763@gmail.com>

@rng1995 rng1995 left a comment

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.

[Automated SkillSpector Review]

Approved. Explicit best-effort cleanup prevents a Windows temp-directory handle race from converting an already-successful CLI response into a failed batch. Cleanup still retries transient failures, then warns and leaks rather than raising; the directory remains unique per invocation and cleanup runs in finally. The agent-CLI suite passes (86 passed, 1 Windows-only skipped).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

claude_cli provider: concurrent temp-file collision on Windows drops most files from LLM analysis ([WinError 32])

2 participants