Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,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 @@ -290,7 +293,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

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
69 changes: 59 additions & 10 deletions apps/frontend/src/main/agent/agent-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import type { AppSettings } from '../../shared/types/settings';
import { getOAuthModeClearVars } from './env-utils';
import { getAugmentedEnv } from '../env-utils';
import { getToolInfo } from '../cli-tool-manager';
import { isWindows } from '../platform';


function deriveGitBashPath(gitExePath: string): string | null {
Expand Down Expand Up @@ -696,15 +697,39 @@ export class AgentProcessManager {
// Mark this specific spawn as killed so its exit handler knows to ignore
this.state.markSpawnAsKilled(agentProcess.spawnId);

// Send SIGTERM first for graceful shutdown
agentProcess.process.kill('SIGTERM');

// Force kill after timeout
setTimeout(() => {
if (!agentProcess.process.killed) {
agentProcess.process.kill('SIGKILL');
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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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);
          }

}, 5000);
} else {
// Unix: SIGTERM then SIGKILL
agentProcess.process.kill('SIGTERM');
setTimeout(() => {
try {
agentProcess.process.kill('SIGKILL');
} catch {
// Process may already be dead
}
}, 5000);
}

this.state.deleteProcess(taskId);
return true;
Expand All @@ -716,15 +741,39 @@ export class AgentProcessManager {
}

/**
* 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
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
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
31 changes: 31 additions & 0 deletions apps/frontend/src/main/terminal/pty-daemon-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import * as path from 'path';
import { fileURLToPath } from 'url';
import { spawn, ChildProcess } from 'child_process';
import { app } from 'electron';
import { isWindows } from '../platform';

// ESM-compatible __dirname
const __filename = fileURLToPath(import.meta.url);
Expand Down Expand Up @@ -402,6 +403,36 @@ class PtyDaemonClient {
*/
shutdown(): void {
this.isShuttingDown = true;

// Kill the daemon process if we spawned it
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
}
}, 2000);
}
} catch {
// Process may already be dead
}
this.daemonProcess = null;
}

this.disconnect();
this.pendingRequests.clear();
this.dataHandlers.clear();
Expand Down
Loading