Skip to content

test(git): close test repositories instead of rmtree so teardown works on Windows - #4880

Closed
tsurutanmen wants to merge 1 commit into
modelcontextprotocol:mainfrom
tsurutanmen:fix/git-tests-windows-teardown
Closed

tsurutanmen wants to merge 1 commit into
modelcontextprotocol:mainfrom
tsurutanmen:fix/git-tests-windows-teardown

Conversation

@tsurutanmen

Copy link
Copy Markdown

Description

On Windows, 40 of the 87 tests in src/git error during teardown of the test_repository fixture. The assertions pass, but pytest reports PermissionError from shutil.rmtree(repo_path):

  • [WinError 32] (file in use): GitPython still holds handles, such as its persistent git cat-file processes, when the fixture removes the directory.
  • [WinError 5] (access denied): git writes loose object files as read-only, and shutil.rmtree cannot delete read-only files on Windows. This shows up in the two git_add tests, where the git CLI writes the objects.

This PR calls test_repo.close() to release the handles, and drops the explicit rmtree. The repository lives under pytest's tmp_path, and pytest removes that directory itself, including read-only files.

Server Details

  • Server: git
  • Changes to: tests only

Motivation and Context

Contributors on Windows currently see 40 errors from a clean checkout. That makes it hard to tell whether a change broke anything.

How Has This Been Tested?

On Windows 11, Python 3.11, git for Windows:

  • before: 47 passed, 40 errors
  • after: 47 passed

ruff check and pyright are clean. There are no behavior changes on Linux or macOS: close() is a no-op there apart from releasing resources, and tmp_path cleanup already applies.

Breaking Changes

None. Only tests change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

I found this while working on #4879, which also touches this server.

🤖 Generated with Claude Code

…s on Windows

On Windows the test_repository fixture teardown raised PermissionError
for 40 of 87 tests: shutil.rmtree ran while GitPython still held
handles (WinError 32), and git's loose object files are read-only,
which rmtree cannot delete (WinError 5). Close the repo to release its
handles and leave tmp_path removal to pytest, which handles read-only
files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@STiFLeR7 STiFLeR7 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary. src/git's test_repository fixture manually shutil.rmtree(repo_path)s after each test. On Windows the PR reports PermissionError during teardown on 40 of 87 tests: GitPython's persistent git cat-file process still holds file handles (WinError 32), and git writes loose objects read-only, which shutil.rmtree can't delete (WinError 5). The fix calls test_repo.close() (releasing GitPython's handles) and drops the explicit rmtree, relying on pytest's own tmp_path cleanup instead.

Verified. (Windows 11, git 2.51.0, GitPython 3.2.1, pytest 8.3.3 — real run, not a mock)

  • Ran tests/test_server.py on a fresh sparse checkout of current main, unpatched: 67 passed, 0 errors — I could not reproduce the claimed baseline (40 errors). Ran the full tests/ directory too: 164 passed, 1 unrelated failure (test_console_script_nonexistent_repository_exits_with_one_line_error, an environment/PATH issue from an editable install, not a teardown error).
  • The reasoning behind the fix is still sound even though I couldn't reproduce the symptom: pytest's tmp_path cleanup (_pytest.pathlib.rm_rf) already chmods and retries on PermissionError, so the described hand-off (fixture closes handles, pytest removes read-only files) is correct in principle, and close() is a safe no-op-ish call on all platforms.
  • Didn't find a regression risk: the PR only removes an explicit rmtree call and adds a close(); no behavior change on Linux/macOS, matching the PR's own claim.

Findings. None that block this — see "Not covered" for why I'm flagging the mismatch rather than treating it as settled.

Not covered / unverified. My environment (git 2.51.0, GitPython 3.2.1) didn't reproduce the 40-error baseline the PR describes, which is a meaningfully different git/GitPython version combination than whatever CI or the author's machine used — worth double-checking the PR still reflects current behavior before merging on the strength of the described repro alone, since the underlying handle-holding behavior may be version-dependent. I didn't install an older GitPython to chase the exact combination that reproduces it.

Verdict. Comment. The fix itself is safe and reasonable regardless, but I'd flag to the maintainer that the specific "40 errors" baseline didn't reproduce for me on a current Windows/git/GitPython stack, so the underlying bug may already be narrower (or different) than described.

@cliffhall cliffhall added the closed-superseded-v2 Closed: fixed or made obsolete by the v2 / 2026-07-28 refactor (#4875) label Oct 20, 2026
@cliffhall

Copy link
Copy Markdown
Member

Thank you for this pull request, and apologies for the long wait for a response.

Since it was opened, the reference servers have been rebuilt for the 2026-07-28 MCP specification (#4857) and released as v2 (2.0.0 on npm for the TypeScript servers, 2026.10.19 on PyPI for the Python servers). The code this PR changes has been replaced, and the behavior it addresses has either been fixed in v2 or no longer exists in the same form.

This is not a judgment on the change or on you; it's a consequence of the code it targets having been replaced. If the problem still happens on v2, please open a fresh issue with the version you're running and steps to reproduce. Note that this repository now accepts contributions as issues rather than pull requests from anyone but its maintainers; see CONTRIBUTING.md.

This is part of the backlog triage in #4875.

@cliffhall cliffhall closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closed-superseded-v2 Closed: fixed or made obsolete by the v2 / 2026-07-28 refactor (#4875)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants