fix(windows): prevent zombie process accumulation on app close #1258
Conversation
Python 3.8+ changed DLL search behavior - os.add_dll_directory() is now required for DLL resolution. PYTHONPATH alone doesn't work because: 1. .pth files are NOT processed when PYTHONPATH is set 2. pywin32_bootstrap.py (which calls os.add_dll_directory) never runs 3. pywintypes312.dll cannot be found without explicit DLL path setup This fix adds a PYTHONSTARTUP bootstrap script that: - Calls os.add_dll_directory() for pywin32_system32 before any imports - Uses site.addsitedir() to properly process .pth files - Adds pywin32_system32 to PATH as fallback Changes: - python-env-manager.ts: Add PYTHONSTARTUP and PATH configuration - download-python.cjs: Generate bootstrap script during build - Added comprehensive tests for the fix Fixes AndyMik90#810, AndyMik90#861 May also fix AndyMik90#943, AndyMik90#656, AndyMik90#630, AndyMik90#853 Co-Authored-By: Claude Opus 4.5 <[email protected]>
Changes based on PR AndyMik90#1005 review comments: 1. Add .pyd extension check to sysloader filter (coderabbitai) - More precise filtering to avoid matching unrelated files 2. Add comments documenting DLL duplication trade-off (coderabbitai) - Explains why DLLs are copied to 3 locations (~2MB extra) - Documents the reliability vs bundle size trade-off 3. Add sync comments between bootstrap script locations (gemini, coderabbitai) - download-python.cjs and python-env-manager.ts now reference each other - Ensures future maintainers know to keep them synchronized 4. Add warning log when PYTHONSTARTUP creation fails (coderabbitai) - Surfaces the failure so users know pywin32 fix may not work - Mentions PATH fallback limitation on Python 3.8+ 5. Improve tests with Vitest best practices (coderabbitai) - Use vi.stubEnv instead of manual process.env mutation - Better PYTHONSTARTUP assertion logic - Integration test now uses actual generated content instead of duplicate Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace existsSync + writeFileSync pattern with atomic 'wx' flag approach
as recommended by Node.js official documentation to prevent file system
race conditions.
Changes:
- python-env-manager.ts: Use writeFileSync with { flag: 'wx' } and handle
EEXIST error silently (file already exists is expected)
- download-python.cjs: Same pattern for __init__.py creation
- Updated tests to verify new atomic write behavior
The 'wx' flag atomically fails if the file exists (EEXIST), eliminating
the window between existsSync check and writeFileSync where another
process could create/modify the file.
Reference: https://nodejs.org/api/fs.html#file-system-flags
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Use taskkill /f /t on Windows to properly kill process trees (SIGTERM/SIGKILL are ignored on Windows) - Make killAllProcesses() wait for process exit events with timeout - Kill PTY daemon process on shutdown - Clear periodic update check interval on app quit Fixes process accumulation in Task Manager after closing Auto-Claude. Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughIntroduces comprehensive Windows-specific improvements: bundled Python pywin32 DLL handling with file copying and startup script bootstrap logic, platform-aware process termination with Windows taskkill fallback and Unix signal sequencing, and periodic update check cleanup during app shutdown. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @VDT-91, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the application's shutdown behavior on Windows by preventing the accumulation of orphaned processes. It ensures that all spawned child processes, including agent processes and the PTY daemon, are properly terminated when the application closes. Additionally, it resolves specific Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // This avoids TOCTOU race condition where existsSync + writeFileSync could | ||
| // allow another process to create/modify the file between check and write. | ||
| // See: https://nodejs.org/api/fs.html#file-system-flags | ||
| writeFileSync(startupScript, scriptContent, { encoding: 'utf-8', flag: 'wx' }); |
Check failure
Code scanning / CodeQL
Insecure temporary file High
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes to prevent zombie processes on Windows when closing the application. The changes include platform-aware process killing logic in agent-process.ts and pty-daemon-client.ts, ensuring taskkill is used on Windows. The killAllProcesses function is now asynchronous and correctly waits for processes to exit, which is a key part of the fix. Additionally, the app updater's periodic check is now properly cleaned up on application quit.
A significant portion of the changes involves complex workarounds for pywin32 installation and DLL loading issues on Windows, both in the build script (download-python.cjs) and at runtime (python-env-manager.ts). While these fixes appear robust and are well-tested, I've identified a couple of areas where maintainability could be improved by reducing code duplication. My review includes suggestions to address these points.
| const scriptContent = `# Auto-Claude pywin32 bootstrap script | ||
| # This script runs via PYTHONSTARTUP before the main script. | ||
| # It ensures pywin32 DLLs can be found on Python 3.8+ where | ||
| # os.add_dll_directory() is required for DLL search paths. | ||
| # | ||
| # See: https://github.com/AndyMik90/Auto-Claude/issues/810 | ||
| # See: https://github.com/mhammond/pywin32/blob/main/win32/Lib/pywin32_bootstrap.py | ||
| import os | ||
| import sys | ||
| def _bootstrap_pywin32(): | ||
| """Bootstrap pywin32 DLL loading for Python 3.8+""" | ||
| # Get the site-packages directory (where this script is located) | ||
| site_packages = os.path.dirname(os.path.abspath(__file__)) | ||
| # 1. Add pywin32_system32 to DLL search path (Python 3.8+ requirement) | ||
| # This is the critical fix - without this, pywintypes DLL cannot be loaded | ||
| pywin32_system32 = os.path.join(site_packages, 'pywin32_system32') | ||
| if os.path.isdir(pywin32_system32): | ||
| if hasattr(os, 'add_dll_directory'): | ||
| try: | ||
| os.add_dll_directory(pywin32_system32) | ||
| except OSError: | ||
| pass # Directory already added or doesn't exist | ||
| # Also add to PATH as fallback for edge cases | ||
| current_path = os.environ.get('PATH', '') | ||
| if pywin32_system32 not in current_path: | ||
| os.environ['PATH'] = pywin32_system32 + os.pathsep + current_path | ||
| # 2. Use site.addsitedir() to process .pth files | ||
| # This triggers pywin32.pth which imports pywin32_bootstrap | ||
| # The bootstrap adds win32, win32/lib to sys.path and calls add_dll_directory | ||
| try: | ||
| import site | ||
| if site_packages not in sys.path: | ||
| site.addsitedir(site_packages) | ||
| except Exception: | ||
| pass # site module issues shouldn't break the app | ||
| # Run bootstrap immediately when this script is loaded | ||
| _bootstrap_pywin32() | ||
| `; |
There was a problem hiding this comment.
This Python bootstrap script content is duplicated from apps/frontend/scripts/download-python.cjs. The comment on line 779 even highlights that they must be kept in sync. This creates a significant maintainability risk, as changes in one place might be forgotten in the other, leading to hard-to-debug inconsistencies between the build-time and runtime environments.
To avoid this, I recommend moving the script content to a single, separate file (e.g., pywin32-bootstrap.py.template) and having both download-python.cjs and python-env-manager.ts read from that file. This would create a single source of truth for the script.
| const dllFiles = fs.readdirSync(pywin32System32).filter(f => f.endsWith('.dll')); | ||
| for (const dll of dllFiles) { | ||
| const srcPath = path.join(pywin32System32, dll); | ||
| const destPath = path.join(win32Dir, dll); | ||
|
|
||
| try { | ||
| fs.copyFileSync(srcPath, destPath); | ||
| console.log(`[download-python] Copied ${dll} to win32/`); | ||
| } catch (err) { | ||
| console.warn(`[download-python] Failed to copy ${dll} to win32/: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| // 5. Also copy DLLs to site-packages root for maximum compatibility | ||
| for (const dll of dllFiles) { | ||
| const srcPath = path.join(pywin32System32, dll); | ||
| const destPath = path.join(sitePackagesDir, dll); | ||
|
|
||
| try { | ||
| fs.copyFileSync(srcPath, destPath); | ||
| console.log(`[download-python] Copied ${dll} to site-packages root`); | ||
| } catch (err) { | ||
| console.warn(`[download-python] Failed to copy ${dll}: ${err.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
These two loops for copying DLLs to different destinations contain duplicated logic. To improve maintainability and reduce redundancy, you can refactor this into a single loop that iterates over a list of target destinations.
const dllFiles = fs.readdirSync(pywin32System32).filter(f => f.endsWith('.dll'));
const copyTargets = [
{ dir: win32Dir, name: 'win32/' },
{ dir: sitePackagesDir, name: 'site-packages root' },
];
for (const dll of dllFiles) {
const srcPath = path.join(pywin32System32, dll);
for (const target of copyTargets) {
const destPath = path.join(target.dir, dll);
try {
fs.copyFileSync(srcPath, destPath);
console.log(`[download-python] Copied ${dll} to ${target.name}`);
} catch (err) {
console.warn(`[download-python] Failed to copy ${dll} to ${target.name}: ${err.message}`);
}
}
}
|
Closing in favor of #1259 which contains only the Windows process cleanup fix (without unrelated pywin32 commits that were accidentally included). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts`:
- Around line 296-301: Replace direct reads of process.platform in the test with
parameterized cases that mock platform detection via the platform abstraction
module (the same pattern as in platform.test.ts); for the test around
mockProcess.kill in subprocess-spawn tests, iterate the platforms
['win32','darwin','linux'], stub the platform detection to return each value,
run the shutdown logic that calls mockProcess.kill, and assert for 'win32' that
mockProcess.kill was called with no args and for 'darwin' and 'linux' that
mockProcess.kill was called with 'SIGTERM' (use the existing mockProcess.kill
reference and the subprocess spawn/shutdown function under test to locate where
to run the assertions).
In `@apps/frontend/src/main/__tests__/python-env-manager.test.ts`:
- Around line 186-201: Update the test that checks non-Windows behavior to run
for both 'darwin' and 'linux' platforms: iterate or parameterize over the
platforms array (e.g., ['darwin','linux']), set process.platform to each value
before calling manager.getPythonEnv(), and assert that (manager as
any).sitePackagesPath is used as PYTHONPATH and PYTHONSTARTUP is undefined;
reference the existing test’s use of process.platform, (manager as
any).sitePackagesPath, and manager.getPythonEnv to locate and modify the test.
Ensure you restore any mocked platform state between iterations.
- Around line 107-109: Replace the repeated "(manager as any)" casts by
introducing a typed test helper reference for the manager (e.g., a local const
testManager typed to the test helper / mock manager interface) and use that
helper to set sitePackagesPath and other properties instead of "as any"; update
each occurrence referenced (around the areas noted) to use the typed helper.
Also add a Linux platform unit test alongside the existing darwin test so
non-Windows coverage includes both macOS and Linux (i.e., duplicate the darwin
platform test case for linux and ensure platform-specific branches exercise both
darwin and linux paths).
In `@apps/frontend/src/main/agent/agent-process.ts`:
- Around line 700-732: The Windows fallback can run after the child already
exited and its PID reused; modify the isWindows() branch around
agentProcess.process.kill()/spawn(...) to store the setTimeout handle (e.g.,
const taskkillTimeout = setTimeout(...)), attach an 'exit' listener on
agentProcess.process that clears that timeout, and inside the timeout callback
check that the process has not already exited by verifying the exitCode is null
(agentProcess.process.exitCode === null) before calling spawn('taskkill', ...);
also ensure you remove the 'exit' listener after it fires to avoid leaks and
keep existing try/catch behavior.
In `@apps/frontend/src/main/app-updater.ts`:
- Around line 41-43: The periodic interval variable periodicCheckIntervalId may
be re-used if initializeAppUpdater runs multiple times; before creating a new
setInterval in initializeAppUpdater, check if periodicCheckIntervalId is
non-null and call clearInterval(periodicCheckIntervalId) and set it to null,
then create and assign the new interval; also ensure any shutdown/cleanup
function (where periodicCheckIntervalId is used, e.g., app shutdown logic)
similarly clears and nulls periodicCheckIntervalId to avoid duplicate timers.
In `@apps/frontend/src/main/python-env-manager.ts`:
- Around line 704-723: The code currently uses direct platform checks and
hardcoded delimiters when building pythonPath and configuring windowsEnv
(references: this.sitePackagesPath, pythonPath, windowsEnv, pywin32System32);
replace process.platform === 'win32' and the hardcoded ';' with the platform
helpers isWindows() and getPathDelimiter() respectively in both Windows-specific
blocks so the logic uses isWindows() to gate the Windows-only branches and
getPathDelimiter() when joining paths (ensure you update both the PYTHONPATH
construction and pywin32_system32 handling to use these helpers).
| // On Windows, kill() is called without arguments; on Unix, kill('SIGTERM') is used | ||
| if (process.platform === 'win32') { | ||
| expect(mockProcess.kill).toHaveBeenCalled(); | ||
| } else { | ||
| expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Find platform-related utility files
fd -t f -i platform . --extension ts --extension tsx --extension js | head -20Repository: AndyMik90/Auto-Claude
Length of output: 164
🏁 Script executed:
# Check the test file to understand its structure and imports
head -50 apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1804
🏁 Script executed:
# Search for platform detection functions in the codebase
rg -t ts -t tsx "isWindows|isMacOS|isLinux" --max-count 10Repository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
# Check if process.platform is used elsewhere in tests
rg -t ts "process\.platform" apps/frontend/src/__tests__/ -B 2 -A 2Repository: AndyMik90/Auto-Claude
Length of output: 622
🏁 Script executed:
# Check the platform abstraction module
cat -n apps/frontend/src/shared/platform.tsRepository: AndyMik90/Auto-Claude
Length of output: 2080
🏁 Script executed:
# Check the platform tests to see how they handle multi-platform testing
cat -n apps/frontend/src/main/platform/__tests__/platform.test.ts | head -100Repository: AndyMik90/Auto-Claude
Length of output: 3234
🏁 Script executed:
# Look at the full test file context around the problematic code
sed -n '280,310p' apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1170
🏁 Script executed:
# Check imports in the test file
grep -n "^import\|^from" apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 363
Parameterize and mock platform detection to test all three platforms.
This test uses process.platform directly instead of importing platform detection functions from the platform abstraction module. More importantly, it only validates the behavior for the test runner's OS. Per coding guidelines and the established pattern in apps/frontend/src/main/platform/__tests__/platform.test.ts, use parameterized tests or separate test cases that mock process.platform to verify all three behaviors (Windows, macOS, Linux) regardless of the runner's platform.
🤖 Prompt for AI Agents
In `@apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts` around
lines 296 - 301, Replace direct reads of process.platform in the test with
parameterized cases that mock platform detection via the platform abstraction
module (the same pattern as in platform.test.ts); for the test around
mockProcess.kill in subprocess-spawn tests, iterate the platforms
['win32','darwin','linux'], stub the platform detection to return each value,
run the shutdown logic that calls mockProcess.kill, and assert for 'win32' that
mockProcess.kill was called with no args and for 'darwin' and 'linux' that
mockProcess.kill was called with 'SIGTERM' (use the existing mockProcess.kill
reference and the subprocess spawn/shutdown function under test to locate where
to run the assertions).
| (manager as any).sitePackagesPath = sitePackagesPath; | ||
|
|
||
| // Mock existsSync to return true for the startup script |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its line count
wc -l apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 128
🏁 Script executed:
# Read the relevant sections of the test file to verify the any-casts
sed -n '100,115p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 697
🏁 Script executed:
# Check the additional line ranges mentioned
sed -n '120,125p' apps/frontend/src/main/__tests__/python-env-manager.test.ts
sed -n '137,145p' apps/frontend/src/main/__tests__/python-env-manager.test.ts
sed -n '156,165p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1017
🏁 Script executed:
# Look for non-Windows test patterns to verify darwin-only concern
rg -A 5 -B 5 "describe.*non.?Windows|describe.*darwin" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Check if there's a platform abstraction module referenced in guidelines
fd -t f "platform" apps/frontend/src/main/ --type f | head -20Repository: AndyMik90/Auto-Claude
Length of output: 123
🏁 Script executed:
# Search for platform module imports in the test file
rg "import.*platform" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Read the test file structure to understand describe blocks and test organization
head -60 apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1673
🏁 Script executed:
# Look for all describe and it blocks to understand test structure
rg "^\s*(describe|it)\(" apps/frontend/src/main/__tests__/python-env-manager.test.ts | head -30Repository: AndyMik90/Auto-Claude
Length of output: 1137
🏁 Script executed:
# Search for platform-related mocking in the entire test file
rg "process.platform|mockReturnValue|darwin|linux|win32" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 2456
🏁 Script executed:
# Check the platform abstraction module to understand available functions
cat apps/frontend/src/main/platform/*.ts | head -80Repository: AndyMik90/Auto-Claude
Length of output: 2024
🏁 Script executed:
# Look for how other tests in the codebase handle cross-platform testing
rg -B 2 -A 5 "darwin.*linux|linux.*win32" apps/frontend/src/main/__tests__/ | head -40Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Search specifically for linux platform testing
rg "linux" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Get the non-Windows platform test in detail
sed -n '170,185p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 580
🏁 Script executed:
# Check how many non-Windows tests exist and what platforms they test
rg -B 3 "Object.defineProperty.*platform" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1149
🏁 Script executed:
# Check if the test file imports from the platform abstraction module
head -30 apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 850
🏁 Script executed:
# Get exact line numbers for all any-casts in the test file
rg -n "as any" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 617
🏁 Script executed:
# Verify the specific line ranges mentioned: 107-109, 123-124, 140-141, 159-160, 193-194, 221-223, 246-248, 291-292
for range in "107,109" "123,124" "140,141" "159,160" "193,194" "221,223" "246,248" "291,292"; do
echo "=== Lines $range ==="
sed -n "${range}p" apps/frontend/src/main/__tests__/python-env-manager.test.ts
doneRepository: AndyMik90/Auto-Claude
Length of output: 880
🏁 Script executed:
# Check the full non-Windows platform test section to see if linux should be tested
sed -n '165,175p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 546
🏁 Script executed:
# Verify how the platform module should be used in tests
grep -r "isWindows\|isMacOS\|isLinux" apps/frontend/src/main/__tests__/ | head -10Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Check if there are examples of platform abstraction usage in other test files
rg "from.*platform" apps/frontend/src/main/__tests__/ -A 2Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Verify the exact structure of the non-Windows test to confirm it only tests darwin
sed -n '165,180p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 718
🏁 Script executed:
# Check if PythonEnvManager uses platform abstraction functions internally
grep -n "isWindows\|isMacOS\|isLinux\|process.platform" apps/frontend/src/main/python-env-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 452
🏁 Script executed:
# Get a broader view of the non-Windows platform test block
sed -n '161,175p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 746
🏁 Script executed:
# Check if there's guidance on how to mock platform functions for testing
cat apps/frontend/src/main/platform/types.ts 2>/dev/null | head -20Repository: AndyMik90/Auto-Claude
Length of output: 425
🏁 Script executed:
# Find the exact non-Windows platform test block
sed -n '155,180p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1153
🏁 Script executed:
# Check what the test validates to understand if linux coverage is needed
sed -n '163,176p' apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 702
🏁 Script executed:
# Look at the implementation to see if behavior differs between darwin and linux
sed -n '700,730p' apps/frontend/src/main/python-env-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 1736
🏁 Script executed:
# Find the complete "should not add Windows-specific env vars on non-Windows platforms" test
rg -B 5 -A 15 "should not add Windows-specific env vars on non-Windows platforms" apps/frontend/src/main/__tests__/python-env-manager.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 801
🏁 Script executed:
# Check if there are separate test cases for linux and darwin, or just one non-Windows test
rg -n "it\(" apps/frontend/src/main/__tests__/python-env-manager.test.ts | grep -i "windows\|darwin\|linux\|platform"Repository: AndyMik90/Auto-Claude
Length of output: 488
Replace as any casts with a typed test helper and add linux platform test coverage.
ESLint flags Unexpected any; a typed helper avoids repeated casts and keeps intent clear. The non-Windows platform test currently covers only darwin; per coding guidelines, platform-specific tests must cover all three platforms (Windows, macOS, Linux).
🧩 Example refactor
+type TestablePythonEnvManager = PythonEnvManager & {
+ sitePackagesPath: string | null;
+ ensurePywin32StartupScript(sitePackagesPath: string): void;
+};
+
- let manager: PythonEnvManager;
+ let manager: TestablePythonEnvManager;
beforeEach(() => {
- manager = new PythonEnvManager();
+ manager = new PythonEnvManager() as TestablePythonEnvManager;
vi.clearAllMocks();
});
@@
- (manager as any).sitePackagesPath = sitePackagesPath;
+ manager.sitePackagesPath = sitePackagesPath;
@@
- const ensureScript = (manager as any).ensurePywin32StartupScript.bind(manager);
+ const ensureScript = manager.ensurePywin32StartupScript.bind(manager);Add linux test alongside the darwin test to cover all non-Windows platforms.
Also applies to: 123-124, 140-141, 159-160, 193-194, 221-223, 246-248, 291-292
🧰 Tools
🪛 GitHub Check: TypeScript (ESLint)
[warning] 107-107:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/__tests__/python-env-manager.test.ts` around lines 107
- 109, Replace the repeated "(manager as any)" casts by introducing a typed test
helper reference for the manager (e.g., a local const testManager typed to the
test helper / mock manager interface) and use that helper to set
sitePackagesPath and other properties instead of "as any"; update each
occurrence referenced (around the areas noted) to use the typed helper. Also add
a Linux platform unit test alongside the existing darwin test so non-Windows
coverage includes both macOS and Linux (i.e., duplicate the darwin platform test
case for linux and ensure platform-specific branches exercise both darwin and
linux paths).
| it('should not add Windows-specific env vars on non-Windows platforms', () => { | ||
| // Restore non-Windows platform | ||
| Object.defineProperty(process, 'platform', { value: 'darwin' }); | ||
|
|
||
| const sitePackagesPath = '/test/site-packages'; | ||
|
|
||
| // Access private property for testing | ||
| (manager as any).sitePackagesPath = sitePackagesPath; | ||
|
|
||
| const env = manager.getPythonEnv(); | ||
|
|
||
| // Should not have PYTHONSTARTUP set | ||
| expect(env.PYTHONSTARTUP).toBeUndefined(); | ||
|
|
||
| // PYTHONPATH should just be the site-packages (no win32 additions) | ||
| expect(env.PYTHONPATH).toBe(sitePackagesPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the test file exists and examine the specific section
wc -l "apps/frontend/src/main/__tests__/python-env-manager.test.ts"Repository: AndyMik90/Auto-Claude
Length of output: 186
🏁 Script executed:
# Read the test file around the lines mentioned (186-201) with context
sed -n '170,220p' "apps/frontend/src/main/__tests__/python-env-manager.test.ts"Repository: AndyMik90/Auto-Claude
Length of output: 1730
🏁 Script executed:
# Search for other platform-related tests in the file to understand full test coverage
rg -n "should not add Windows|darwin|linux|win32" "apps/frontend/src/main/__tests__/python-env-manager.test.ts"Repository: AndyMik90/Auto-Claude
Length of output: 1826
Test non-Windows behavior on both macOS and Linux.
The test only covers the darwin platform; add linux to the parameterization to match the guideline requirement of testing all three platforms (Windows, macOS, Linux).
Example parameterization
- it('should not add Windows-specific env vars on non-Windows platforms', () => {
- Object.defineProperty(process, 'platform', { value: 'darwin' });
+ it.each(['darwin', 'linux'])(
+ 'should not add Windows-specific env vars on %s',
+ (platform) => {
+ Object.defineProperty(process, 'platform', { value: platform });
@@
- });
+ }
+ );🧰 Tools
🪛 GitHub Check: TypeScript (ESLint)
[warning] 193-193:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/__tests__/python-env-manager.test.ts` around lines 186
- 201, Update the test that checks non-Windows behavior to run for both 'darwin'
and 'linux' platforms: iterate or parameterize over the platforms array (e.g.,
['darwin','linux']), set process.platform to each value before calling
manager.getPythonEnv(), and assert that (manager as any).sitePackagesPath is
used as PYTHONPATH and PYTHONSTARTUP is undefined; reference the existing test’s
use of process.platform, (manager as any).sitePackagesPath, and
manager.getPythonEnv to locate and modify the test. Ensure you restore any
mocked platform state between iterations.
| if (isWindows()) { | ||
| // Windows: .kill() then taskkill fallback | ||
| // SIGTERM/SIGKILL are ignored on Windows, so we use taskkill /f /t | ||
| try { | ||
| agentProcess.process.kill(); | ||
| if (agentProcess.process.pid) { | ||
| const pid = agentProcess.process.pid; | ||
| setTimeout(() => { | ||
| try { | ||
| // taskkill /f = force kill, /t = kill child processes tree | ||
| spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], { | ||
| stdio: 'ignore', | ||
| detached: true | ||
| }).unref(); | ||
| } catch { | ||
| // Process may already be dead | ||
| } | ||
| }, 5000); | ||
| } | ||
| } catch { | ||
| // Process may already be dead | ||
| } | ||
| }, 5000); | ||
| } else { | ||
| // Unix: SIGTERM then SIGKILL | ||
| agentProcess.process.kill('SIGTERM'); | ||
| setTimeout(() => { | ||
| try { | ||
| agentProcess.process.kill('SIGKILL'); | ||
| } catch { | ||
| // Process may already be dead | ||
| } | ||
| }, 5000); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the relevant file
find apps/frontend -name "agent-process.ts" -type f | head -5Repository: AndyMik90/Auto-Claude
Length of output: 186
🏁 Script executed:
# Read the full context around lines 700-732
cat -n apps/frontend/src/main/agent/agent-process.ts | sed -n '680,780p'Repository: AndyMik90/Auto-Claude
Length of output: 3677
🏁 Script executed:
# Check for isWindows import and platform module usage
grep -n "isWindows\|isMacOS\|isLinux\|platform" apps/frontend/src/main/agent/agent-process.ts | head -20Repository: AndyMik90/Auto-Claude
Length of output: 266
🏁 Script executed:
# Search for existing exit event handlers and process lifecycle handling
grep -n "\.once\|\.on\|exit\|exitCode" apps/frontend/src/main/agent/agent-process.ts | head -20Repository: AndyMik90/Auto-Claude
Length of output: 922
🏁 Script executed:
# Check the broader killProcess and killAllProcesses context
cat -n apps/frontend/src/main/agent/agent-process.ts | sed -n '690,800p'Repository: AndyMik90/Auto-Claude
Length of output: 4558
🌐 Web query:
Node.js ChildProcess exitCode property null timing when process exits
💡 Result:
Short answer: child.exitCode is null while the child is still running and is only set once the child has actually exited. Check it (or use the exit/close event callbacks) after the 'exit' or 'close' events — 'exit' gives the numeric code/signal when the process ends, and 'close' is emitted after stdio streams are closed (and always after 'exit'). [1][2]
References:
- Node.js ChildProcess docs (exitCode / exit / close). [1] [2]
🌐 Web query:
Windows PID reuse timeout how quickly can PID be reused
💡 Result:
Short answer: Windows has no fixed "timeout" — a PID can be reused as soon as the terminated process's kernel process object is destroyed (i.e., the process has exited and the last handles/references to its object are closed), so reuse can be effectively immediate under load. Use an open process handle or compare creation times to detect/reject reuse. [1][2][3]
Sources:
- Microsoft Learn: GetProcessId / GetProcessIdOfThread (remarks about PID uniqueness only while process exists). [1]
- OSR/NTDEV discussion on PID reuse and delaying reuse by holding a handle/object reference. [2]
- Practical guidance (ToolHelp/Handle, check creation time) about PID reuse when building process trees. [3]
Guard Windows taskkill against stale PID reuse.
The 5s fallback taskkill can fire after the process exits; Windows reuses PIDs immediately, risking termination of an unrelated process. Listen to the exit event to clear the timeout and check exitCode === null before executing taskkill to prevent stale invocations.
🛠️ Suggested fix
- if (isWindows()) {
- // Windows: .kill() then taskkill fallback
- // SIGTERM/SIGKILL are ignored on Windows, so we use taskkill /f /t
- try {
- agentProcess.process.kill();
- if (agentProcess.process.pid) {
- const pid = agentProcess.process.pid;
- setTimeout(() => {
- try {
- // taskkill /f = force kill, /t = kill child processes tree
- spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], {
- stdio: 'ignore',
- detached: true
- }).unref();
- } catch {
- // Process may already be dead
- }
- }, 5000);
- }
- } catch {
- // Process may already be dead
- }
- } else {
+ if (isWindows()) {
+ // Windows: .kill() then taskkill fallback (cancel if the process exits)
+ const pid = agentProcess.process.pid;
+ let fallbackTimer: NodeJS.Timeout | undefined;
+ const clearFallback = () => {
+ if (fallbackTimer) clearTimeout(fallbackTimer);
+ };
+ if (typeof agentProcess.process.once === 'function') {
+ agentProcess.process.once('exit', clearFallback);
+ }
+ try {
+ agentProcess.process.kill();
+ } catch {
+ // Process may already be dead
+ }
+ if (pid) {
+ fallbackTimer = setTimeout(() => {
+ if (agentProcess.process.exitCode === null) {
+ try {
+ // taskkill /f = force kill, /t = kill child processes tree
+ spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], {
+ stdio: 'ignore',
+ detached: true
+ }).unref();
+ } catch {
+ // Process may already be dead
+ }
+ }
+ }, 5000);
+ }
+ } else {🤖 Prompt for AI Agents
In `@apps/frontend/src/main/agent/agent-process.ts` around lines 700 - 732, The
Windows fallback can run after the child already exited and its PID reused;
modify the isWindows() branch around agentProcess.process.kill()/spawn(...) to
store the setTimeout handle (e.g., const taskkillTimeout = setTimeout(...)),
attach an 'exit' listener on agentProcess.process that clears that timeout, and
inside the timeout callback check that the process has not already exited by
verifying the exitCode is null (agentProcess.process.exitCode === null) before
calling spawn('taskkill', ...); also ensure you remove the 'exit' listener after
it fires to avoid leaks and keep existing try/catch behavior.
| // Store interval ID for cleanup during shutdown | ||
| let periodicCheckIntervalId: ReturnType<typeof setInterval> | null = null; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard against duplicate periodic intervals.
If initializeAppUpdater can run more than once (e.g., re-init after window recreation), a previous interval can keep running. Consider clearing any existing interval before creating a new one.
♻️ Proposed fix
- periodicCheckIntervalId = setInterval(() => {
+ if (periodicCheckIntervalId) {
+ clearInterval(periodicCheckIntervalId);
+ periodicCheckIntervalId = null;
+ }
+ periodicCheckIntervalId = setInterval(() => {
console.warn('[app-updater] Performing periodic update check');
autoUpdater.checkForUpdates().catch((error) => {
console.error('[app-updater] ❌ Periodic update check failed:', error.message);
if (DEBUG_UPDATER) {
console.error('[app-updater:debug] Full error:', error);
}
});
}, FOUR_HOURS);Also applies to: 195-195
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/app-updater.ts` around lines 41 - 43, The periodic
interval variable periodicCheckIntervalId may be re-used if initializeAppUpdater
runs multiple times; before creating a new setInterval in initializeAppUpdater,
check if periodicCheckIntervalId is non-null and call
clearInterval(periodicCheckIntervalId) and set it to null, then create and
assign the new interval; also ensure any shutdown/cleanup function (where
periodicCheckIntervalId is used, e.g., app shutdown logic) similarly clears and
nulls periodicCheckIntervalId to avoid duplicate timers.
| // Build PYTHONPATH - for Windows with pywin32, we need to include win32 and win32/lib | ||
| // since the .pth file that normally adds these isn't processed when using PYTHONPATH | ||
| let pythonPath = this.sitePackagesPath || ''; | ||
| if (this.sitePackagesPath && process.platform === 'win32') { | ||
| const pathSep = ';'; // Windows path separator | ||
| const win32Path = path.join(this.sitePackagesPath, 'win32'); | ||
| const win32LibPath = path.join(this.sitePackagesPath, 'win32', 'lib'); | ||
| pythonPath = [this.sitePackagesPath, win32Path, win32LibPath].join(pathSep); | ||
| } | ||
|
|
||
| // Windows-specific pywin32 DLL loading fix | ||
| // On Windows with bundled packages, we need to ensure pywin32 DLLs can be found. | ||
| // Python 3.8+ requires os.add_dll_directory() for DLL search paths. | ||
| // We achieve this by: | ||
| // 1. Adding pywin32_system32 to PATH (fallback for some edge cases) | ||
| // 2. Setting PYTHONSTARTUP to bootstrap script that calls os.add_dll_directory() | ||
| // and uses site.addsitedir() to process .pth files (which imports pywin32_bootstrap) | ||
| let windowsEnv: Record<string, string> = {}; | ||
| if (this.sitePackagesPath && process.platform === 'win32') { | ||
| const pywin32System32 = path.join(this.sitePackagesPath, 'pywin32_system32'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file mentioned in the review
find . -name "python-env-manager.ts" -type fRepository: AndyMik90/Auto-Claude
Length of output: 111
🏁 Script executed:
# List platform-related files to find the platform abstraction module
find . -path "*/main/platform*" -o -path "*/src/platform*" | head -20Repository: AndyMik90/Auto-Claude
Length of output: 332
🏁 Script executed:
# Look for platform module exports
fd -e ts -e js | xargs grep -l "export.*isWindows\|export.*getPathDelimiter" | head -5Repository: AndyMik90/Auto-Claude
Length of output: 142
🏁 Script executed:
# Check the platform module exports
cat apps/frontend/src/main/platform/index.tsRepository: AndyMik90/Auto-Claude
Length of output: 10296
🏁 Script executed:
# Check python-env-manager.ts at the specified lines and around
sed -n '700,760p' apps/frontend/src/main/python-env-manager.ts | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 3500
🏁 Script executed:
# Check current imports in python-env-manager.ts
head -50 apps/frontend/src/main/python-env-manager.ts | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 2319
🏁 Script executed:
# Check lines 721-760 to see the full scope of "Also applies to: 721-753"
sed -n '721,760p' apps/frontend/src/main/python-env-manager.ts | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 2238
Replace direct process.platform checks and hardcoded delimiters with platform abstraction helpers.
The code uses process.platform === 'win32' checks and hardcoded ; path separators directly instead of the platform abstraction module. Replace these with isWindows() and getPathDelimiter() throughout both Windows-specific blocks (lines 704–723 and 721–754).
♻️ Suggested patch
-import { isLinux, isWindows } from './platform';
+import { isLinux, isWindows, getPathDelimiter } from './platform';
@@
- if (this.sitePackagesPath && process.platform === 'win32') {
- const pathSep = ';'; // Windows path separator
+ if (this.sitePackagesPath && isWindows()) {
+ const pathSep = getPathDelimiter();
const win32Path = path.join(this.sitePackagesPath, 'win32');
const win32LibPath = path.join(this.sitePackagesPath, 'win32', 'lib');
pythonPath = [this.sitePackagesPath, win32Path, win32LibPath].join(pathSep);
}
let windowsEnv: Record<string, string> = {};
- if (this.sitePackagesPath && process.platform === 'win32') {
+ if (this.sitePackagesPath && isWindows()) {
const pywin32System32 = path.join(this.sitePackagesPath, 'pywin32_system32');
const currentPath = baseEnv['PATH'] || baseEnv['Path'] || '';
if (currentPath && !currentPath.includes(pywin32System32)) {
- windowsEnv['PATH'] = `${pywin32System32};${currentPath}`;
+ windowsEnv['PATH'] = `${pywin32System32}${getPathDelimiter()}${currentPath}`;
} else if (!currentPath) {
windowsEnv['PATH'] = pywin32System32;
}🤖 Prompt for AI Agents
In `@apps/frontend/src/main/python-env-manager.ts` around lines 704 - 723, The
code currently uses direct platform checks and hardcoded delimiters when
building pythonPath and configuring windowsEnv (references:
this.sitePackagesPath, pythonPath, windowsEnv, pywin32System32); replace
process.platform === 'win32' and the hardcoded ';' with the platform helpers
isWindows() and getPathDelimiter() respectively in both Windows-specific blocks
so the logic uses isWindows() to gate the Windows-only branches and
getPathDelimiter() when joining paths (ensure you update both the PYTHONPATH
construction and pywin32_system32 handling to use these helpers).
1 participant
Summary
Fixes Windows-specific issue where processes accumulate in Task Manager after closing Auto-Claude. This addresses issues #1252, #641, #607, and relates to the previously closed but unmerged PR #753.
Root Causes Fixed
agent-process.tstaskkill /f /tto kill process treeskillAllProcesses()resolves immediatelyagent-process.tspty-daemon-client.tsshutdown()app-updater.tsChanges
agent-process.ts: Windows-awarekillProcess()usingtaskkill /f /tfallback after.kill(), andkillAllProcesses()that properly waits for process exit eventspty-daemon-client.ts: Kill PTY daemon process on shutdown using platform-appropriate methodapp-updater.ts: Store periodic check interval ID and exportstopPeriodicUpdates()functionindex.ts: CallstopPeriodicUpdates()inbefore-quithandlersubprocess-spawn.test.ts: Update tests to handle platform-specific kill behaviorImplementation Details
The fix follows the existing pattern from
worktree-handlers.ts(lines 3033-3061) and uses the centralized platform abstraction (isWindows()fromplatform/index.ts) as documented in CLAUDE.md.Windows behavior:
Unix behavior:
Test plan
--no-file-parallelism)npm run build)npm run package:winbuilds successfullyManual Testing Steps Performed
npm run package:winnode.exe,python.exe,cmd.exe,pwsh.exe, orAuto-Claude.exeprocesses remain🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.