Skip to content

fix(windows): prevent zombie process accumulation on app close #1259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AndyMik90 merged 5 commits into from
Jan 17, 2026
Merged

fix(windows): prevent zombie process accumulation on app close #1259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db3e041
fix(windows): prevent zombie process accumulation on app close
VDT-91 Jan 17, 2026
8bd59ea
Merge branch 'develop' into fix/windows-process-cleanup-clean
AndyMik90 Jan 17, 2026
93cdb98
fix(windows): extract killProcessGracefully utility with timer cleanup
VDT-91 Jan 17, 2026
d916c9d
Merge branch 'develop' into fix/windows-process-cleanup-clean
VDT-91 Jan 17, 2026
d4805af
Merge branch 'develop' into fix/windows-process-cleanup-clean
VDT-91 Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ const mockProcess = Object.assign(new EventEmitter(), {
killed: false,
kill: vi.fn(() => {
mockProcess.killed = true;
// Emit exit event synchronously to simulate process termination
// (needed for killAllProcesses wait - using nextTick for more predictable timing)
process.nextTick(() => mockProcess.emit('exit', 0, null));
return true;
})
});
Expand Down Expand Up @@ -329,7 +332,12 @@ describe('Subprocess Spawn Integration', () => {
const result = manager.killTask('task-1');

expect(result).toBe(true);
expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM');
// 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');
}
Comment on lines +335 to +340
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 -n

Repository: 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.ts

Repository: 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 -100

Repository: 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.

expect(manager.isRunning('task-1')).toBe(false);
});

Expand Down
58 changes: 37 additions & 21 deletions apps/frontend/src/main/agent/agent-process.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
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

Unused import isWindows.

/**
* Type for supported CLI tools
Expand Down Expand Up @@ -719,40 +720,55 @@
*/
killProcess(taskId: string): boolean {
const agentProcess = this.state.getProcess(taskId);
if (agentProcess) {
try {
// Mark this specific spawn as killed so its exit handler knows to ignore
this.state.markSpawnAsKilled(agentProcess.spawnId);
if (!agentProcess) return false;

// Send SIGTERM first for graceful shutdown
agentProcess.process.kill('SIGTERM');
// Mark this specific spawn as killed so its exit handler knows to ignore
this.state.markSpawnAsKilled(agentProcess.spawnId);

// Force kill after timeout
setTimeout(() => {
if (!agentProcess.process.killed) {
agentProcess.process.kill('SIGKILL');
}
}, 5000);
// Use shared platform-aware kill utility
killProcessGracefully(agentProcess.process, {
debugPrefix: '[AgentProcess]',
debug: process.env.DEBUG === 'true' || process.env.NODE_ENV === 'development'
});

this.state.deleteProcess(taskId);
return true;
} catch {
return false;
}
}
return false;
this.state.deleteProcess(taskId);
return true;
}

/**
* Kill all running processes
* Kill all running processes and wait for them to exit
*/
async killAllProcesses(): Promise<void> {
const KILL_TIMEOUT_MS = 10000; // 10 seconds max wait

const killPromises = this.state.getRunningTaskIds().map((taskId) => {
return new Promise<void>((resolve) => {
const agentProcess = this.state.getProcess(taskId);

if (!agentProcess) {
resolve();
return;
}

// Set up timeout to not block forever
const timeoutId = setTimeout(() => {
resolve();
}, KILL_TIMEOUT_MS);

// Listen for exit event if the process supports it
// (process.once is available on real ChildProcess objects, but may not be in test mocks)
if (typeof agentProcess.process.once === 'function') {
agentProcess.process.once('exit', () => {
clearTimeout(timeoutId);
resolve();
});
}

// Kill the process
this.killProcess(taskId);
resolve();
});
});

await Promise.all(killPromises);
}

Expand Down
16 changes: 15 additions & 1 deletion apps/frontend/src/main/app-updater.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ autoUpdater.autoInstallOnAppQuit = true; // Automatically install on app quit
// Update channels: 'latest' for stable, 'beta' for pre-release
type UpdateChannel = 'latest' | 'beta';

// Store interval ID for cleanup during shutdown
let periodicCheckIntervalId: ReturnType<typeof setInterval> | null = null;

/**
* Set the update channel for electron-updater.
* - 'latest': Only receive stable releases (default)
Expand Down Expand Up @@ -189,7 +192,7 @@ export function initializeAppUpdater(window: BrowserWindow, betaUpdates = false)
const FOUR_HOURS = 4 * 60 * 60 * 1000;
console.warn(`[app-updater] Periodic checks scheduled every ${FOUR_HOURS / 1000 / 60 / 60} hours`);

setInterval(() => {
periodicCheckIntervalId = setInterval(() => {
console.warn('[app-updater] Performing periodic update check');
autoUpdater.checkForUpdates().catch((error) => {
console.error('[app-updater] ❌ Periodic update check failed:', error.message);
Expand Down Expand Up @@ -492,3 +495,14 @@ export async function downloadStableVersion(): Promise<void> {
autoUpdater.allowDowngrade = false;
}
}

/**
* Stop periodic update checks - called during app shutdown
*/
export function stopPeriodicUpdates(): void {
if (periodicCheckIntervalId) {
clearInterval(periodicCheckIntervalId);
periodicCheckIntervalId = null;
console.warn('[app-updater] Periodic update checks stopped');
}
}
5 changes: 4 additions & 1 deletion apps/frontend/src/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { TerminalManager } from './terminal-manager';
import { pythonEnvManager } from './python-env-manager';
import { getUsageMonitor } from './claude-profile/usage-monitor';
import { initializeUsageMonitorForwarding } from './ipc-handlers/terminal-handlers';
import { initializeAppUpdater } from './app-updater';
import { initializeAppUpdater, stopPeriodicUpdates } from './app-updater';
import { DEFAULT_APP_SETTINGS } from '../shared/constants';
import { readSettingsFile } from './settings-utils';
import { setupErrorLogging } from './app-logger';
Expand Down Expand Up @@ -440,6 +440,9 @@ app.on('window-all-closed', () => {

// Cleanup before quit
app.on('before-quit', async () => {
// Stop periodic update checks
stopPeriodicUpdates();

// Stop usage monitor
const usageMonitor = getUsageMonitor();
usageMonitor.stop();
Expand Down
60 changes: 10 additions & 50 deletions apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
findTaskWorktree,
} from '../../worktree-paths';
import { persistPlanStatus, updateTaskMetadataPrUrl } from './plan-file-utils';
import { killProcessGracefully } from '../../platform';

// Regex pattern for validating git branch names
const GIT_BRANCH_REGEX = /^[a-zA-Z0-9][a-zA-Z0-9._/-]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$/;
Expand Down Expand Up @@ -2006,27 +2007,11 @@ export function registerWorktreeHandlers(
debug('TIMEOUT: Merge process exceeded', MERGE_TIMEOUT_MS, 'ms, killing...');
resolved = true;

// Platform-specific process termination
if (process.platform === 'win32') {
// On Windows, .kill() without signal terminates the process tree
// SIGTERM/SIGKILL are not supported the same way on Windows
try {
mergeProcess.kill();
} catch {
// Process may already be dead
}
} else {
// On Unix-like systems, use SIGTERM first, then SIGKILL as fallback
mergeProcess.kill('SIGTERM');
// Give it a moment to clean up, then force kill
setTimeout(() => {
try {
mergeProcess.kill('SIGKILL');
} catch {
// Process may already be dead
}
}, 5000);
}
// Platform-specific process termination with fallback
killProcessGracefully(mergeProcess, {
debugPrefix: '[MERGE]',
debug: isDebugMode
});

// Check if merge might have succeeded before the hang
// Look for success indicators in the output
Expand Down Expand Up @@ -3030,35 +3015,10 @@ export function registerWorktreeHandlers(
resolved = true;

// Platform-specific process termination with fallback
if (process.platform === 'win32') {
try {
createPRProcess.kill();
// Fallback: forcefully kill with taskkill if process ignores initial kill
if (createPRProcess.pid) {
setTimeout(() => {
try {
spawn('taskkill', ['/pid', createPRProcess.pid!.toString(), '/f', '/t'], {
stdio: 'ignore',
detached: true
}).unref();
} catch {
// Process may already be dead
}
}, 5000);
}
} catch {
// Process may already be dead
}
} else {
createPRProcess.kill('SIGTERM');
setTimeout(() => {
try {
createPRProcess.kill('SIGKILL');
} catch {
// Process may already be dead
}
}, 5000);
}
killProcessGracefully(createPRProcess, {
debugPrefix: '[PR_CREATION]',
debug: isDebugMode
});

resolve({
success: false,
Expand Down
Loading
Loading