Fix Windows RecursionError in temporary directory cleanup#21
Open
yeabwang wants to merge 8 commits intoanthropics:mainfrom
Open
Fix Windows RecursionError in temporary directory cleanup#21yeabwang wants to merge 8 commits intoanthropics:mainfrom
yeabwang wants to merge 8 commits intoanthropics:mainfrom
Conversation
- Add WindowsFileSystemCleaner class with multiple fallback strategies - Implement safe_rmtree with recursion protection (max depth 100) - Add retry logic for transient Windows filesystem errors - Support for locked files, permissions, and long path issues - Include safe temporary directory creation helpers Fixes RecursionError in Windows temporary directory cleanup operations.
- Add safe_temp_directory context manager with platform-aware cleanup - Implement SafeTemporaryDirectory as drop-in replacement for tempfile.TemporaryDirectory - Support custom prefixes, suffixes, and error handling options - Compatible interface with existing tempfile.TemporaryDirectory usage - Automatic cleanup using platform-specific safe removal methods Provides Windows-safe temporary directory management without code changes.
- Implement WindowsPatches class for automatic monkey patching - Replace tempfile.TemporaryDirectory with SafeTemporaryDirectory on Windows - Add context manager for temporary patch application - Auto-apply patches on module import for maximum compatibility - Provide manual patch control with apply/remove methods Enables transparent Windows filesystem fixes without modifying existing code.
- Add safe_rmtree method to PlatformAdapter class - Integrate Windows cleanup utilities for Windows platforms - Add create_safe_tempdir method with platform-specific handling - Maintain fallback to standard operations on non-Windows platforms - Extend platform adapter pattern to cover filesystem operations Completes cross-platform filesystem compatibility layer.
- Auto-apply Windows compatibility patches when claudecode module is imported - Graceful fallback if Windows patches are not available - Transparent activation without requiring code changes - Ensures maximum compatibility across all usage patterns Completes non-invasive Windows RecursionError fix implementation.
- Add 18 pytest tests covering all Windows cleanup functionality - Test platform detection, safe temp operations, context managers - Validate SafeTemporaryDirectory class behavior and error handling - Include Windows-specific functionality and cross-platform compatibility - Add performance and reliability tests with recursion protection - Provide integration test for complete workflow validation Ensures robust testing of Windows RecursionError fixes (191 total tests).
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical Windows filesystem issue where RecursionError was occurring in 5 workflow integration tests during temporary directory cleanup, caused by infinite recursion in shutil.rmtree().
The solution implements a comprehensive cross-platform adapter pattern with multiple fallback cleanup strategies:
- Windows-specific cleanup utilities with recursion protection
- Drop-in replacement for
tempfile.TemporaryDirectory - Automatic monkey patching on Windows platforms
- Additional test coverage for the fix
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
claudecode/windows_patches.py |
Monkey-patching framework for Windows compatibility with auto-application on import |
claudecode/windows_cleanup.py |
Windows filesystem cleanup utilities with recursion protection and multiple fallback strategies |
claudecode/test_windows_cleanup.py |
Comprehensive test suite for Windows cleanup functionality |
claudecode/test_utils.py |
Enhanced test utilities with Windows-specific testing capabilities |
claudecode/safe_temp.py |
Platform-aware temporary directory management as drop-in replacement |
claudecode/platform_utils.py |
Cross-platform adapter for handling OS-specific command execution and filesystem operations |
claudecode/github_action_audit.py |
Updated to use platform adapter for Claude CLI execution |
claudecode/__init__.py |
Auto-applies Windows patches on module import |
README.md |
Added Windows setup and troubleshooting documentation |
| """ | ||
| Manager for Windows-specific monkey patches. | ||
|
|
||
| Handles patching to fix Windows filesystem issues |
There was a problem hiding this comment.
Extra space in comment. Should be 'Handles patching' instead of 'Handles patching'.
Suggested change
| Handles patching to fix Windows filesystem issues | |
| Handles patching to fix Windows filesystem issues |
| Args: | ||
| max_retries: Retry attempts for locked files | ||
| retry_delay: Delay between retries (seconds) | ||
| max_depth: Max directory depth (prevents infinite recursion) |
There was a problem hiding this comment.
[nitpick] The magic number 100 for max_depth should be documented or made configurable. Consider adding a constant or explaining why 100 is the appropriate recursion limit.
Suggested change
| max_depth: Max directory depth (prevents infinite recursion) | |
| def __init__(self, max_retries: int = 3, retry_delay: float = 0.1, max_depth: int = DEFAULT_MAX_DEPTH): | |
| """ | |
| Args: | |
| max_retries: Retry attempts for locked files | |
| retry_delay: Delay between retries (seconds) | |
| max_depth: Max directory depth (prevents infinite recursion, default: DEFAULT_MAX_DEPTH) |
|
|
||
| 1. Install Node.js (v18 or later) and npm: | ||
| - Download from https://nodejs.org or use nvm-windows. | ||
| 2. Install the Claude CLI:npm install -g @anthropic-ai/claude-code |
There was a problem hiding this comment.
Missing space after colon. Should be '2. Install the Claude CLI: npm install -g @anthropic-ai/claude-code'.
Suggested change
| 2. Install the Claude CLI:npm install -g @anthropic-ai/claude-code | |
| 2. Install the Claude CLI: npm install -g @anthropic-ai/claude-code |
| [Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | ||
|
|
||
| 4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | ||
| 5. Verify installation:claude --version |
There was a problem hiding this comment.
Missing space after colon. Should be '3. Add npm global directory to PATH: $npmPath = npm config get prefix'.
Suggested change
| 5. Verify installation:claude --version | |
| 2. Install the Claude CLI: npm install -g @anthropic-ai/claude-code | |
| 3. Add npm global directory to PATH: $npmPath = npm config get prefix | |
| [Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | |
| 4. Set PowerShell execution policy: Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | |
| 5. Verify installation: claude --version |
| 3. Add npm global directory to PATH:$npmPath = npm config get prefix | ||
| [Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | ||
|
|
||
| 4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned |
There was a problem hiding this comment.
Missing space after colon. Should be '4. Set PowerShell execution policy: Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned'.
Suggested change
| 4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | |
| 4. Set PowerShell execution policy: Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned |
| [Environment]::SetEnvironmentVariable("Path", [Environment]::GetEnvironmentVariable("Path", "User") + ";$npmPath", "User") | ||
|
|
||
| 4. Set PowerShell execution policy:Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned | ||
| 5. Verify installation:claude --version |
There was a problem hiding this comment.
Missing space after colon. Should be '5. Verify installation: claude --version'.
Suggested change
| 5. Verify installation:claude --version | |
| 5. Verify installation: claude --version |
|
|
||
| Expected output: 1.0.71 (Claude Code) or similar. | ||
|
|
||
| Troubleshooting |
There was a problem hiding this comment.
The 'Troubleshooting' section should be formatted as a proper header with markdown syntax (## Troubleshooting).
Suggested change
| Troubleshooting | |
| ## Troubleshooting |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Windows filesystem operations were causing
RecursionErrorin 5 workflow integration tests due to infinite recursion inshutil.rmtree()during temporary directory cleanup.Solution
tempfile.TemporaryDirectory