fix(windows): prevent zombie process accumulation on app close #1259
Conversation
- 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]>
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 provides a critical fix for a Windows-specific issue where background processes would persist as "zombies" after the main application closed. By implementing robust, platform-aware process termination strategies and ensuring all background tasks are properly shut down, this change significantly improves resource management and application stability on Windows. 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! |
![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 effectively addresses process leaks on Windows by implementing platform-aware termination logic. The use of taskkill for Windows processes, the improved waiting mechanism in killAllProcesses, and the cleanup of the app updater and PTY daemon are all excellent changes. I have one suggestion to further improve the robustness of the process killing logic on Windows.
| 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 | ||
| } |
There was a problem hiding this comment.
The taskkill fallback logic is currently inside the try block that wraps agentProcess.process.kill(). If kill() throws an error (for instance, if the process has already exited), the catch block is entered, and the taskkill logic is skipped. On Windows, taskkill is the more reliable method for ensuring a process is terminated, so it should be attempted even if the initial kill() call fails. I recommend moving the taskkill logic outside of this try...catch block to ensure the fallback always runs.
try {
agentProcess.process.kill();
} catch {
// Process may already be dead, or kill failed.
// Proceed to taskkill as a fallback.
}
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);
}
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a platform-aware graceful-kill utility and integrates it into agent process management, PTY daemon shutdown, worktree handlers, and app-updater cleanup; tests for the new kill logic and adjusted subprocess tests were added/updated. Changes
Sequence DiagramsequenceDiagram
participant App as App (before-quit)
participant AgentMgr as Agent Manager
participant Kill as killProcessGracefully()
participant OS as Operating System
participant Timer as Timeout Handler
App->>AgentMgr: killAllProcesses()
AgentMgr->>Kill: killProcess(childProcess, options) [for each task]
Note over Kill: Attempt graceful termination
Kill->>OS: kill()/SIGTERM (platform-specific)
OS-->>Kill: may emit exit/error
alt no exit within timeout
Timer->>Kill: timeout (GRACEFUL_KILL_TIMEOUT_MS)
Kill->>OS: taskkill /F /T (Windows) or SIGKILL (Unix)
OS-->>Kill: exit/error
end
Kill->>AgentMgr: resolve when process exited or timed out
AgentMgr->>App: all tasks handled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 1 issue(s) must be addressed (0 required, 1 recommended). 6 non-blocking suggestion(s) to consider.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- Medium: 1 issue(s)
- Low: 6 issue(s)
Generated by Auto Claude PR Review
Findings (7 selected of 7 total)
🟡 [25133a06805f] [MEDIUM] Taskkill fallback skipped if .kill() throws
📁 apps/frontend/src/main/agent/agent-process.ts:700
The taskkill fallback (lines 706-717) is inside the try block that wraps .kill() at line 704. If .kill() throws an exception, execution jumps to the catch block on line 719, completely skipping the setTimeout that schedules taskkill. This defeats the purpose of having a fallback - the taskkill should run regardless of whether .kill() succeeds or throws. This could leave zombie processes on Windows when .kill() fails (e.g., permission denied, process in weird state).
Suggested fix:
Capture PID before the try block and schedule taskkill unconditionally:
```typescript
if (isWindows()) {
const pid = agentProcess.process.pid;
try {
agentProcess.process.kill();
} catch {
// Process may already be dead
}
// Schedule taskkill regardless of .kill() success
if (pid) {
setTimeout(() => {
try {
spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], {
stdio: 'ignore',
detached: true
}).unref();
} catch { /* ... */ }
}, 5000);
}
}
#### 🔵 [eb8a7e34fe56] [LOW] Windows taskkill runs unconditionally after 5 seconds
📁 `apps/frontend/src/main/agent/agent-process.ts:707`
On Windows, taskkill is scheduled to run after 5000ms unconditionally, even if the process has already exited. While taskkill failing silently is handled via try/catch, this is wasteful. In extremely rare cases of rapid PID reuse (process churn), the PID could belong to a different process.
**Suggested fix:**
Track the timeout ID and clear it when the process exits naturally, or check process status before running taskkill. Low priority since PID reuse in 5 seconds is very rare and try/catch handles failures.
#### 🔵 [689f261ba164] [LOW] Magic number: KILL_TIMEOUT_MS defined but 5000ms not
📁 `apps/frontend/src/main/agent/agent-process.ts:747`
The 10000ms timeout is properly defined as KILL_TIMEOUT_MS constant, but the 5000ms timeout used for graceful kill delays (lines 707, 731) is a magic number. The same 5000ms appears in pty-daemon-client.ts, but with an inconsistent 2000ms on line 428.
**Suggested fix:**
Extract to a named constant: const GRACEFUL_KILL_TIMEOUT_MS = 5000; at module level. Also consider making pty-daemon-client.ts consistent (uses 2000ms instead of 5000ms for SIGKILL timeout).
#### 🔵 [08d6996f1597] [LOW] Inconsistent SIGKILL timeout (2000ms vs 5000ms)
📁 `apps/frontend/src/main/terminal/pty-daemon-client.ts:428`
The SIGKILL timeout in pty-daemon-client.ts is 2000ms while agent-process.ts uses 5000ms for the same purpose. This inconsistency suggests either one timeout is wrong, or there's a valid reason that should be documented.
**Suggested fix:**
Use a consistent timeout value across both files (recommend 5000ms to match agent-process.ts) or document why daemon termination uses a shorter timeout.
#### 🔵 [0c6475f3a926] [LOW] Empty catch blocks swallow errors silently
📁 `apps/frontend/src/main/agent/agent-process.ts:714`
Multiple empty catch blocks (lines 714, 719, 729, 736) swallow errors with only a comment. While this is intentional during shutdown, the complete absence of logging makes debugging difficult when process termination fails for unexpected reasons. This follows the existing pattern from worktree-handlers.ts.
**Suggested fix:**
Consider adding debug-level logging for non-ESRCH errors: catch (err: unknown) { if ((err as NodeJS.ErrnoException).code !== 'ESRCH') console.debug('[AgentProcess] Unexpected error during kill:', err); }. This preserves silent behavior for expected 'no such process' errors while logging unexpected failures.
#### 🔵 [81631a83a7a3] [LOW] Missing warning when process.once is unavailable
📁 `apps/frontend/src/main/agent/agent-process.ts:765`
The check `if (typeof agentProcess.process.once === 'function')` handles test mocks, but in production ChildProcess always has 'once'. If this check fails in production, it indicates a serious bug but the code silently falls back to timeout without logging.
**Suggested fix:**
Add a warning log when the check fails: if (typeof agentProcess.process.once !== 'function') { console.warn('[AgentProcess] Process missing once method - using timeout fallback'); }
#### 🔵 [b00a02eb0d04] [LOW] Opportunity: Extract shared platform-aware kill utility
📁 `apps/frontend/src/main/agent/agent-process.ts:700`
The taskkill pattern is now duplicated in 3 locations: agent-process.ts:700-732, pty-daemon-client.ts:408-433, and worktree-handlers.ts:3033-3061. The pattern is identical across all three. A shared utility in the platform/ module would improve maintainability.
**Suggested fix:**
Consider extracting to apps/frontend/src/main/platform/process.ts: export function killProcessTree(pid: number, options?: { delay?: number }): void. This is optional - the PR correctly follows the existing pattern.
---
*This review was generated by Auto Claude.*
- Extract shared killProcessGracefully() to platform module - Fix Issue AndyMik90#1: Move taskkill outside try-catch scope - Fix Issue #2: Track exit state to skip unnecessary taskkill - Fix Issue AndyMik90#3: Add GRACEFUL_KILL_TIMEOUT_MS constant - Fix Issue AndyMik90#4: Use consistent 5000ms timeout everywhere - Fix Issue AndyMik90#5: Add debug logging for catch blocks - Fix Issue AndyMik90#6: Log warning when process.once unavailable - Fix Issue AndyMik90#7: Eliminate code duplication across 3 files - Fix timer leak: Clear timeout on process exit/error, unref timer Add comprehensive tests (19 test cases) covering: - Windows taskkill fallback behavior - Unix SIGTERM/SIGKILL sequence - Timer cleanup and memory leak prevention - Edge cases and error handling Co-Authored-By: Claude Opus 4.5 <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { getOAuthModeClearVars } from './env-utils'; | ||
| import { getAugmentedEnv } from '../env-utils'; | ||
| import { getToolInfo } from '../cli-tool-manager'; | ||
| import { isWindows, killProcessGracefully } from '../platform'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts (1)
9-13: Use platform abstraction and parameterize the test to cover all platforms.The test at lines 327-332 directly checks
process.platforminstead of using the platform module abstraction. Replace the direct platform check withisWindows(), and wrap the assertion indescribe.each(['win32', 'darwin', 'linux'])with amockPlatform()helper (following the pattern inclaude-integration-handler.test.ts) so all platform behaviors are tested deterministically rather than depending on the host OS.🔧 Suggested approach
Add a mockPlatform helper similar to existing tests, then refactor the test:
+function mockPlatform(platform: NodeJS.Platform) { + Object.defineProperty(process, 'platform', { + value: platform, + writable: true, + configurable: true + }); +} + - it('should kill task and remove from tracking', async () => { + describe.each(['win32', 'darwin', 'linux'] as const)('should kill task on %s', (platform) => { + beforeEach(() => { + mockPlatform(platform as NodeJS.Platform); + }); + + it('and remove from tracking', async () => { const { AgentManager } = await import('../../main/agent'); const manager = new AgentManager(); manager.configure(undefined, AUTO_CLAUDE_SOURCE); await manager.startSpecCreation('task-1', TEST_PROJECT_PATH, 'Test'); expect(manager.isRunning('task-1')).toBe(true); const result = manager.killTask('task-1'); expect(result).toBe(true); - if (process.platform === 'win32') { + if (platform === 'win32') { expect(mockProcess.kill).toHaveBeenCalled(); } else { expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM'); } expect(manager.isRunning('task-1')).toBe(false); - }); + }); + });
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/platform/index.ts`:
- Around line 485-494: The SIGKILL fallback is suppressed by checking
childProcess.killed even though that flag is set immediately after calling
childProcess.kill('SIGTERM'), so remove the redundant "!childProcess.killed"
guard in the branch that sends SIGKILL and instead rely solely on the hasExited
flag (set by the 'exit' listener) to decide whether to send SIGKILL; update the
logic in the block referencing childProcess.kill('SIGTERM'), hasExited, and the
SIGKILL branch so SIGKILL is attempted when hasExited is false, and also
update/remove the unit test named process-kill.test.ts that asserts "skips
SIGKILL if process.killed is true" to reflect the corrected behavior.
In `@apps/frontend/src/main/terminal/pty-daemon-client.ts`:
- Line 13: Replace the manual SIGKILL timeout logic in pty-daemon-client (where
a timer is created to SIGKILL a child after GRACEFUL_KILL_TIMEOUT_MS) with the
shared killProcessGracefully utility: import killProcessGracefully and call it
with the child process (or PID) and any existing timeout value instead of
creating an unreferenced setTimeout; remove the manual timer and any direct
process.kill calls tied to that timer so killProcessGracefully can manage
unref/cleanup and avoid keeping the event loop alive or killing reused PIDs.
| if (isWindows()) { | ||
| log('Running taskkill for PID:', pid); | ||
| spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], { | ||
| stdio: 'ignore', | ||
| detached: true | ||
| }).unref(); | ||
| } else if (!childProcess.killed) { | ||
| log('Sending SIGKILL to PID:', pid); | ||
| childProcess.kill('SIGKILL'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of the killProcessGracefully function
cat -n apps/frontend/src/main/platform/index.ts | head -550 | tail -100Repository: AndyMik90/Auto-Claude
Length of output: 3690
🏁 Script executed:
# Also check the test file mentioned
cat -n apps/frontend/src/main/platform/__tests__/process-kill.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 12791
🏁 Script executed:
# Search for hasExited usage in the file
rg "hasExited" apps/frontend/src/main/platform/index.ts -A 2 -B 2Repository: AndyMik90/Auto-Claude
Length of output: 462
SIGKILL fallback is effectively disabled on Unix due to childProcess.killed check.
In Node.js, childProcess.killed becomes true immediately after .kill() is called, not after the process actually exits. Since line 467 calls childProcess.kill('SIGTERM') first, the killed property is already true when the timeout fires. This means the else if (!childProcess.killed) guard at line 491 prevents SIGKILL from being sent even when the process ignores SIGTERM and remains alive.
The code already tracks actual process exit via the hasExited variable (set by the 'exit' event listener). Remove the redundant !childProcess.killed check to rely only on hasExited:
🐛 Suggested fix
- } else if (!childProcess.killed) {
- log('Sending SIGKILL to PID:', pid);
- childProcess.kill('SIGKILL');
- }
+ } else {
+ log('Sending SIGKILL to PID:', pid);
+ childProcess.kill('SIGKILL');
+ }This also requires updating the test at apps/frontend/src/main/platform/__tests__/process-kill.test.ts lines 193–202 ("skips SIGKILL if process.killed is true"), which currently tests the incorrect behavior where SIGKILL is skipped based on the killed flag. That test should be removed or refactored to reflect that SIGKILL should always attempt when the process hasn't actually exited.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isWindows()) { | |
| log('Running taskkill for PID:', pid); | |
| spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], { | |
| stdio: 'ignore', | |
| detached: true | |
| }).unref(); | |
| } else if (!childProcess.killed) { | |
| log('Sending SIGKILL to PID:', pid); | |
| childProcess.kill('SIGKILL'); | |
| } | |
| if (isWindows()) { | |
| log('Running taskkill for PID:', pid); | |
| spawn('taskkill', ['/pid', pid.toString(), '/f', '/t'], { | |
| stdio: 'ignore', | |
| detached: true | |
| }).unref(); | |
| } else { | |
| log('Sending SIGKILL to PID:', pid); | |
| childProcess.kill('SIGKILL'); | |
| } |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/platform/index.ts` around lines 485 - 494, The SIGKILL
fallback is suppressed by checking childProcess.killed even though that flag is
set immediately after calling childProcess.kill('SIGTERM'), so remove the
redundant "!childProcess.killed" guard in the branch that sends SIGKILL and
instead rely solely on the hasExited flag (set by the 'exit' listener) to decide
whether to send SIGKILL; update the logic in the block referencing
childProcess.kill('SIGTERM'), hasExited, and the SIGKILL branch so SIGKILL is
attempted when hasExited is false, and also update/remove the unit test named
process-kill.test.ts that asserts "skips SIGKILL if process.killed is true" to
reflect the corrected behavior.
| import { fileURLToPath } from 'url'; | ||
| import { spawn, ChildProcess } from 'child_process'; | ||
| import { app } from 'electron'; | ||
| import { isWindows, GRACEFUL_KILL_TIMEOUT_MS } from '../platform'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its current state
git ls-files | grep -i "pty-daemon"Repository: AndyMik90/Auto-Claude
Length of output: 163
🏁 Script executed:
# Look at the current implementation in the target file
cat apps/frontend/src/main/terminal/pty-daemon-client.tsRepository: AndyMik90/Auto-Claude
Length of output: 11446
🏁 Script executed:
# Find the platform module and check if killProcessGracefully exists
fd -t f "platform" apps/frontend/src/main --type fRepository: AndyMik90/Auto-Claude
Length of output: 123
🏁 Script executed:
# Search for killProcessGracefully definition
rg "killProcessGracefully" apps/frontend/src/main --type ts --type tsx -A 20Repository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
fd -t f "platform" apps/frontend/src/main -name "*.ts" -o -name "*.tsx" -o -name "*.js"Repository: AndyMik90/Auto-Claude
Length of output: 295
🏁 Script executed:
# Look at the platform module structure
ls -la apps/frontend/src/main/platform/Repository: AndyMik90/Auto-Claude
Length of output: 421
🏁 Script executed:
# Check the platform module index file
head -100 apps/frontend/src/main/platform.ts 2>/dev/null || head -100 apps/frontend/src/main/platform/index.ts 2>/dev/null || cat apps/frontend/src/main/platform.ts 2>/dev/null || cat apps/frontend/src/main/platform/index.ts 2>/dev/nullRepository: AndyMik90/Auto-Claude
Length of output: 2432
🏁 Script executed:
# Search for any file in platform directory
find apps/frontend/src/main/platform -type f -name "*.ts" -o -name "*.tsx" -o -name "*.js" | head -20Repository: AndyMik90/Auto-Claude
Length of output: 309
🏁 Script executed:
# Read the full platform/index.ts file
wc -l apps/frontend/src/main/platform/index.ts && cat apps/frontend/src/main/platform/index.tsRepository: AndyMik90/Auto-Claude
Length of output: 13325
🏁 Script executed:
# Check the process-kill test file to understand expected behavior
cat apps/frontend/src/main/platform/__tests__/process-kill.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 10285
Replace manual shutdown with killProcessGracefully to prevent stray timers and ensure clean app exit.
The current implementation leaves the SIGKILL timeout unreferenced and untracked, which can:
- Keep the event loop alive and delay app shutdown
- Risk killing a reused PID if the app doesn't fully exit
The shared killProcessGracefully() utility properly handles timer cleanup by clearing it on process exit and calling .unref() to avoid blocking Node.js shutdown.
🛠️ Suggested fix (use shared killProcessGracefully)
-import { isWindows, GRACEFUL_KILL_TIMEOUT_MS } from '../platform';
+import { killProcessGracefully } from '../platform';
@@
- if (this.daemonProcess && this.daemonProcess.pid) {
- try {
- if (isWindows()) {
- // Windows: use taskkill to force kill process tree
- spawn('taskkill', ['/pid', this.daemonProcess.pid.toString(), '/f', '/t'], {
- stdio: 'ignore',
- detached: true
- }).unref();
- } else {
- // Unix: SIGTERM then SIGKILL
- this.daemonProcess.kill('SIGTERM');
- const daemonProc = this.daemonProcess;
- setTimeout(() => {
- try {
- if (daemonProc) {
- daemonProc.kill('SIGKILL');
- }
- } catch {
- // Process may already be dead
- }
- }, GRACEFUL_KILL_TIMEOUT_MS);
- }
- } catch {
- // Process may already be dead
- }
- this.daemonProcess = null;
- }
+ if (this.daemonProcess) {
+ killProcessGracefully(this.daemonProcess, { debugPrefix: '[PTY_DAEMON]' });
+ this.daemonProcess = null;
+ }🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/pty-daemon-client.ts` at line 13, Replace the
manual SIGKILL timeout logic in pty-daemon-client (where a timer is created to
SIGKILL a child after GRACEFUL_KILL_TIMEOUT_MS) with the shared
killProcessGracefully utility: import killProcessGracefully and call it with the
child process (or PID) and any existing timeout value instead of creating an
unreferenced setTimeout; remove the manual timer and any direct process.kill
calls tied to that timer so killProcessGracefully can manage unref/cleanup and
avoid keeping the event loop alive or killing reused PIDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts`:
- Around line 335-340: Update the test to stop reading process.platform directly
and instead import the platform helper functions (isWindows, isMacOS, isLinux)
from apps/frontend/src/main/platform, then parameterize the test over the three
platforms ('win32', 'darwin', 'linux') by mocking the appropriate helper to
return true for each case; for each parameterized run, call the same code that
invokes killProcessGracefully() and assert on mockProcess.kill (for 'win32'
expect(mockProcess.kill).toHaveBeenCalled() with no args; for 'darwin' and
'linux' expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM')); ensure you
reset/restore mocks between iterations so the platform helper stubs don't leak
across cases.
| // 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.
🧩 Analysis chain
🏁 Script executed:
# First, find platform abstraction exports in the frontend platform module
rg -n "export\s+.*(platform|isWindows|isMac|isLinux|getPlatform)" apps/frontend/src/main/platform -g '*.ts'Repository: AndyMik90/Auto-Claude
Length of output: 405
🏁 Script executed:
# Read the test file to understand the context and structure
head -n 50 apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 2344
🏁 Script executed:
# Get imports and the specific test section
sed -n '1,20p' apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts && echo "..." && sed -n '320,350p' apps/frontend/src/__tests__/integration/subprocess-spawn.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 2058
🏁 Script executed:
# Find and read the AgentManager implementation to see killTask() method
rg -n "killTask" apps/frontend/src/main -A 15 -g '*.ts'Repository: AndyMik90/Auto-Claude
Length of output: 8823
🏁 Script executed:
# Find ProcessManager and its killProcess method
rg -n "class ProcessManager" apps/frontend/src/main -A 50 -g '*.ts' | head -100Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Also search for killProcess implementation
rg -n "killProcess" apps/frontend/src/main -B 2 -A 10 -g '*.ts'Repository: AndyMik90/Auto-Claude
Length of output: 42058
Use platform abstraction functions with tri-platform test parameterization.
This test uses process.platform directly and only validates the current OS. Per platform abstraction guidelines, import platform detection helpers from apps/frontend/src/main/platform/ (e.g., isWindows(), isMacOS(), isLinux()) and parameterize the test to cover all three platforms deterministically. Mock the platform function to test win32, darwin, and linux behavior without relying on the test runner's current OS. The underlying killProcessGracefully() function already supports all three platforms correctly.
🤖 Prompt for AI Agents
In `@apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts` around
lines 335 - 340, Update the test to stop reading process.platform directly and
instead import the platform helper functions (isWindows, isMacOS, isLinux) from
apps/frontend/src/main/platform, then parameterize the test over the three
platforms ('win32', 'darwin', 'linux') by mocking the appropriate helper to
return true for each case; for each parameterized run, call the same code that
invokes killProcessGracefully() and assert on mockProcess.kill (for 'win32'
expect(mockProcess.kill).toHaveBeenCalled() with no args; for 'darwin' and
'linux' expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM')); ensure you
reset/restore mocks between iterations so the platform helper stubs don't leak
across cases.
2 participants
Summary
Fixes Windows-specific issue where processes accumulate in Task Manager after closing Auto-Claude. This is a minimal, surgical fix (~80 lines) affecting 5 files, following existing codebase patterns.
Closes #1252 (Windows Process Cleanup Issues)
Root Causes Fixed
agent-process.tstaskkill /f /twith process tree killkillAllProcesses()resolves immediatelyagent-process.tspty-daemon-client.tsapp-updater.tsChanges
agent-process.tskillProcess(): Uses Windows-aware termination - calls.kill()thentaskkill /f /tfallback after 5s timeoutkillAllProcesses(): Now waits for exit events with 10s timeout before resolvingpty-daemon-client.tsshutdown(): Kills daemon process usingtaskkill /f /ton Windows, SIGTERM/SIGKILL on Unixapp-updater.tsstopPeriodicUpdates()export to clear intervalindex.tsstopPeriodicUpdates()inbefore-quithandlerTest Updates
killAllProcesses()testsPattern Used
Follows the exact
taskkillpattern already established inworktree-handlers.ts:3033-3061:Test Plan
npm run testin apps/frontend)npm run typecheck)npm run build)npm run package:win, installed, ran tasks, closed app - no orphaned processes in Task ManagerRisk Assessment
killProcess()Windows handlingkillAllProcesses()wait logic🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.5 [email protected]
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.