fix: resolve WinError 32 during sparse-checkout fallback on Windows

[JanDeDobbeleer]

Description

Problem

On Windows, apm install consistently failed with:

❌ Failed to install github.com/.../skills/conventional-commit: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\Users...\AppData\Local\Temp\tmpXXXXX\repo'

All packages installed via sparse checkout (subdirectory packages like skills) were affected.

Root cause

Two distinct failure paths, both caused by git file handles still being open when cleanup ran:

Path A — 20% progress (sparse checkout fails, falls back to full clone) When _try_sparse_checkout failed, the code tried to shutil.rmtree the partial sparse-checkout directory before cloning into the same path. On Windows, git fetch spawns git-remote-https.exe as a child process whose handles linger asynchronously after subprocess.run returns. The rmtree would fail silently (locked files remain), then Repo.clone_from into the non-empty directory would fail. TemporaryDirectory.__exit__ then called plain shutil.rmtree again — raising WinError 32 as the visible error.

Path B — 70% progress (sparse checkout succeeds) After extracting the subdirectory, a Repo(temp_clone_path) object was created to read the commit SHA. This GitPython object held .git/index handles open. When the with tempfile.TemporaryDirectory() block exited, its __exit__ called shutil.rmtree without any retry logic, hitting WinError 32 on those open handles.

Fix

Three targeted changes in download_subdirectory_package:

1. TemporaryDirectory → mkdtemp + try/finally + _rmtree — gives explicit control over cleanup timing and uses the retry-capable
   _rmtree helper instead of the plain shutil.rmtree inside TemporaryDirectory.__exit__.

2. Full-clone fallback uses a fresh repo_clone/ subdirectory instead of re-using the (possibly locked) repo/ sparse-checkout path. The
   locked directory is left untouched until _rmtree(temp_dir) in the finally block, by which point all git subprocesses have long exited.

3. Added _close_repo() helper that calls repo.git.clear_cache() + repo.close() to explicitly release GitPython file handles. Applied to
   the commit-SHA capture Repo object so handles are freed before _rmtree runs.

Tests

Added TestWindowsCleanupHelpers (5 tests) and TestDownloadSubdirectoryPackageWindowsCleanup (2 tests) covering:

• _rmtree removes normal and read-only directories
• _close_repo handles None, releases handles, and swallows exceptions
• SHA-capture Repo is closed before _rmtree in the success path
• Full-clone fallback always targets repo_clone/, never the sparse path

Pre-Post screenshot

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

[Copilot]

Pull request overview

Fixes a Windows-specific installation failure (WinError 32) when downloading subdirectory packages via sparse-checkout and its fallback paths by ensuring Git/GitPython resources are closed before cleanup and by making cleanup more robust.

Changes:

• Add Windows-safe cleanup helpers (_close_repo, _rmtree) and switch subdirectory download temp handling from TemporaryDirectory to mkdtemp + try/finally.
• Change full-clone fallback to use a fresh repo_clone/ directory instead of reusing the sparse-checkout path.
• Add tests covering the new cleanup helpers and the updated sparse-checkout/fallback behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/apm_cli/deps/github_downloader.py	Introduces _close_repo/_rmtree and updates download_subdirectory_package temp clone/cleanup flow to avoid Windows file-handle races.
tests/test_github_downloader.py	Adds unit tests for cleanup helpers and for sparse-checkout success/failure cleanup behavior.

[Copilot]

test_close_repo_swallows_exceptions() only asserts that _close_repo() doesn’t raise when repo.git.clear_cache() fails, but it doesn’t verify that repo.close() is still invoked. Since the main goal is to release handles, the test should assert that close() is called even if clear_cache() throws (this will also catch regressions where _close_repo stops short of closing on errors).

Suggested change

	_close_repo(repo)
	_close_repo(repo)
	# Even if clear_cache fails, we must still attempt it and close the repo
	repo.git.clear_cache.assert_called_once()
	repo.close.assert_called_once()

[danielmeppiel]

Hey @JanDeDobbeleer — welcome to APM and thank you for this fantastic first contribution! 🎉

The root cause analysis in the PR description is top-notch — clearly identifying the two distinct failure paths (sparse-checkout fallback vs. success) and the GitPython handle lifecycle issue. The pre/post screenshot is a great touch too. This is exactly the kind of thorough, well-documented bug fix we love to see.

The approach is clean and well-scoped: mkdtemp + try/finally for explicit cleanup control, fresh repo_clone/ directory to sidestep locked paths, and _close_repo() to release handles eagerly. Tests cover all the right scenarios. CI is all green. ✅

I have just two small suggestions on Copilot's review above to tighten the robustness of the new helpers — both are quick fixes. Once those are in, this is ready to merge. Really appreciate you taking the time to track this down and fix it properly!

[JanDeDobbeleer]

@danielmeppiel done