diff --git a/main/src/main.ts b/main/src/main.ts index 5276dcd41..a68779569 100644 --- a/main/src/main.ts +++ b/main/src/main.ts @@ -271,6 +271,17 @@ app.whenReady().then(async () => { mainWindow.webContents.once('did-finish-load', () => { log.info('Main window did-finish-load event triggered') }) + + // Windows-specific: Handle system shutdown/restart/logout + if (process.platform === 'win32') { + mainWindow.on('session-end', (event) => { + log.info( + `[session-end] Windows session ending (reasons: ${event.reasons.join(', ')}), forcing cleanup...` + ) + stopToolhive() + safeTrayDestroy() + }) + } } } catch (error) { log.error('Failed to create main window:', error) @@ -346,9 +357,19 @@ app.on('before-quit', async (e) => { }) app.on('will-quit', (e) => blockQuit('will-quit', e)) +app.on('quit', () => { + log.info('[quit event] Ensuring ToolHive cleanup...') + // Only cleanup if not already tearing down to avoid double cleanup + if (!getTearingDownState()) { + stopToolhive() + safeTrayDestroy() + } +}) + // Docker / Ctrl-C etc. ;['SIGTERM', 'SIGINT'].forEach((sig) => - process.on(sig as NodeJS.Signals, async () => { + process.on(sig, async () => { + log.info(`[${sig}] start...`) if (getTearingDownState()) return setTearingDownState(true) setQuittingState(true) @@ -366,6 +387,12 @@ app.on('will-quit', (e) => blockQuit('will-quit', e)) }) ) +process.on('exit', (code) => { + log.info(`[process exit] code=${code}, ensuring ToolHive is stopped...`) + // Note: Only synchronous operations work here, so we force immediate SIGKILL + stopToolhive({ force: true }) +}) + ipcMain.handle('dark-mode:toggle', () => { nativeTheme.themeSource = nativeTheme.shouldUseDarkColors ? 'light' : 'dark' return nativeTheme.shouldUseDarkColors diff --git a/main/src/tests/toolhive-manager.test.ts b/main/src/tests/toolhive-manager.test.ts index 31dc530a2..16493a01e 100644 --- a/main/src/tests/toolhive-manager.test.ts +++ b/main/src/tests/toolhive-manager.test.ts @@ -70,10 +70,10 @@ const mockGetQuittingState = vi.mocked(getQuittingState) class MockProcess extends EventEmitter { pid = 12345 killed = false - kill() { - this.killed = true - this.emit('exit', 0) + // Don't automatically set killed - let tests control this + // This allows testing delayed SIGKILL scenarios + return true } } @@ -179,6 +179,7 @@ describe('toolhive-manager', () => { { stdio: ['ignore', 'ignore', 'pipe'], detached: false, + windowsHide: true, } ) @@ -236,6 +237,7 @@ describe('toolhive-manager', () => { await vi.advanceTimersByTimeAsync(50) await startPromise + mockProcess.killed = true mockProcess.emit('exit', 1) expect(mockLog.warn).toHaveBeenCalledWith( @@ -256,6 +258,7 @@ describe('toolhive-manager', () => { mockCaptureMessage.mockClear() + mockProcess.killed = true mockProcess.emit('exit', 1) expect(mockCaptureMessage).toHaveBeenCalledWith( @@ -274,6 +277,7 @@ describe('toolhive-manager', () => { mockCaptureMessage.mockClear() + mockProcess.killed = true mockProcess.emit('exit', 0) expect(mockCaptureMessage).not.toHaveBeenCalled() @@ -299,6 +303,7 @@ describe('toolhive-manager', () => { const restartPromise = restartToolhive() // Let the original process exit during restart + mockProcess.killed = true mockProcess.emit('exit', 0) await vi.advanceTimersByTimeAsync(50) @@ -368,6 +373,7 @@ describe('toolhive-manager', () => { { stdio: ['ignore', 'ignore', 'pipe'], detached: false, + windowsHide: true, } ) }) @@ -416,4 +422,243 @@ describe('toolhive-manager', () => { expect(isToolhiveRunning()).toBe(true) }) }) + + describe('stopToolhive', () => { + beforeEach(async () => { + // Start a process before each stop test + const startPromise = startToolhive() + await vi.advanceTimersByTimeAsync(50) + await startPromise + vi.clearAllMocks() + }) + + it('does nothing if no process is running', () => { + stopToolhive() // Stop once + vi.clearAllMocks() + + stopToolhive() // Try to stop again + + expect(mockLog.info).toHaveBeenCalledWith( + expect.stringContaining('No process to stop') + ) + }) + + it('sends SIGTERM by default for graceful shutdown', () => { + const killSpy = vi.spyOn(mockProcess, 'kill') + + stopToolhive() + + expect(killSpy).toHaveBeenCalledWith('SIGTERM') + expect(mockLog.info).toHaveBeenCalledWith( + expect.stringContaining('SIGTERM sent, result:') + ) + }) + + it('schedules SIGKILL after 2 seconds if process does not exit gracefully', async () => { + // Mock the process to not be killed immediately + const killSpy = vi.spyOn(mockProcess, 'kill') + mockProcess.killed = false + + stopToolhive() + + // SIGTERM should be sent immediately + expect(killSpy).toHaveBeenCalledWith('SIGTERM') + expect(killSpy).toHaveBeenCalledTimes(1) + + // Advance time by less than 2 seconds - SIGKILL should not be sent yet + await vi.advanceTimersByTimeAsync(1000) + expect(killSpy).toHaveBeenCalledTimes(1) + + // Advance to 2 seconds - SIGKILL should be sent + await vi.advanceTimersByTimeAsync(1000) + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(killSpy).toHaveBeenCalledTimes(2) + expect(mockLog.warn).toHaveBeenCalledWith( + expect.stringContaining( + 'Process 12345 did not exit gracefully, forcing SIGKILL' + ) + ) + }) + + it('does not send SIGKILL if process exits gracefully within 2 seconds', async () => { + const killSpy = vi.spyOn(mockProcess, 'kill') + + stopToolhive() + + // SIGTERM sent + expect(killSpy).toHaveBeenCalledWith('SIGTERM') + + // Simulate process exiting gracefully + mockProcess.killed = true + + // Advance time to when SIGKILL would have been sent + await vi.advanceTimersByTimeAsync(2000) + + // SIGKILL should NOT be sent since process already exited + expect(killSpy).toHaveBeenCalledTimes(1) + expect(killSpy).not.toHaveBeenCalledWith('SIGKILL') + }) + + it('sends immediate SIGKILL when force option is true', () => { + const killSpy = vi.spyOn(mockProcess, 'kill') + + stopToolhive({ force: true }) + + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(killSpy).toHaveBeenCalledTimes(1) + expect(mockLog.info).toHaveBeenCalledWith( + expect.stringContaining('[stopToolhive] SIGKILL sent, result:') + ) + }) + + it('does not schedule delayed SIGKILL when force option is true', async () => { + const killSpy = vi.spyOn(mockProcess, 'kill') + mockProcess.killed = false + + stopToolhive({ force: true }) + + // Immediate SIGKILL sent + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(killSpy).toHaveBeenCalledTimes(1) + + // Advance time - no additional kill should occur + await vi.advanceTimersByTimeAsync(2000) + expect(killSpy).toHaveBeenCalledTimes(1) + }) + + it('clears pending kill timer when called multiple times', async () => { + mockProcess.killed = false + + // Start first process + const startPromise1 = startToolhive() + await vi.advanceTimersByTimeAsync(50) + await startPromise1 + + const firstProcess = mockProcess + const firstKillSpy = vi.spyOn(firstProcess, 'kill') + + stopToolhive() // First stop - schedules SIGKILL + expect(firstKillSpy).toHaveBeenCalledWith('SIGTERM') + + // Before timer fires, start and stop again + await vi.advanceTimersByTimeAsync(500) + + const newMockProcess = new MockProcess() + mockSpawn.mockReturnValue( + newMockProcess as unknown as ReturnType + ) + + const startPromise2 = startToolhive() + await vi.advanceTimersByTimeAsync(50) + await startPromise2 + + const secondKillSpy = vi.spyOn(newMockProcess, 'kill') + + stopToolhive() // Second stop - should clear first timer + + // Advance past when first timer would have fired + await vi.advanceTimersByTimeAsync(2000) + + // Only the second process should get SIGKILL, not the first + expect(secondKillSpy).toHaveBeenCalledWith('SIGKILL') + expect(firstKillSpy).toHaveBeenCalledTimes(1) // Only SIGTERM, no SIGKILL + }) + + it('handles kill errors and attempts force kill as fallback', () => { + const killSpy = vi.spyOn(mockProcess, 'kill') + killSpy.mockImplementationOnce(() => { + throw new Error('Kill failed') + }) + killSpy.mockImplementationOnce(() => true) + + stopToolhive() + + expect(mockLog.error).toHaveBeenCalledWith( + '[stopToolhive] Failed to send SIGTERM:', + expect.any(Error) + ) + expect(killSpy).toHaveBeenCalledWith('SIGTERM') + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + }) + + it('clears toolhiveProcess reference after stopping', () => { + stopToolhive() + + expect(isToolhiveRunning()).toBe(false) + expect(mockLog.info).toHaveBeenCalledWith( + '[stopToolhive] Process cleanup completed' + ) + }) + + it('uses captured process reference in timer callback', async () => { + const killSpy = vi.spyOn(mockProcess, 'kill') + mockProcess.killed = false + + const capturedProcess = mockProcess + + stopToolhive() + + // Process reference is cleared immediately + expect(isToolhiveRunning()).toBe(false) + + // But timer should still have access to the process + await vi.advanceTimersByTimeAsync(2000) + + expect(killSpy).toHaveBeenCalledWith('SIGKILL') + expect(capturedProcess.killed).toBe(false) // Our mock doesn't auto-set this + }) + }) + + describe('restartToolhive', () => { + it('stops existing process and starts a new one', async () => { + // Start initial process + const startPromise = startToolhive() + await vi.advanceTimersByTimeAsync(50) + await startPromise + + const firstProcess = mockProcess + const firstKillSpy = vi.spyOn(firstProcess, 'kill') + + vi.clearAllMocks() + + // Create new mock process for restart + const newMockProcess = new MockProcess() + mockSpawn.mockReturnValue( + newMockProcess as unknown as ReturnType + ) + + const restartPromise = restartToolhive() + await vi.advanceTimersByTimeAsync(50) + await restartPromise + + expect(firstKillSpy).toHaveBeenCalled() + expect(mockSpawn).toHaveBeenCalled() + expect(mockLog.info).toHaveBeenCalledWith( + 'ToolHive restarted successfully' + ) + }) + + it('prevents concurrent restarts', async () => { + const startPromise = startToolhive() + await vi.advanceTimersByTimeAsync(50) + await startPromise + + vi.clearAllMocks() + + const newMockProcess = new MockProcess() + mockSpawn.mockReturnValue( + newMockProcess as unknown as ReturnType + ) + + const restart1 = restartToolhive() + const restart2 = restartToolhive() + + await vi.advanceTimersByTimeAsync(50) + await Promise.all([restart1, restart2]) + + expect(mockLog.info).toHaveBeenCalledWith( + 'Restart already in progress, skipping...' + ) + }) + }) }) diff --git a/main/src/toolhive-manager.ts b/main/src/toolhive-manager.ts index 58f0217de..5a9afd7c2 100644 --- a/main/src/toolhive-manager.ts +++ b/main/src/toolhive-manager.ts @@ -29,6 +29,7 @@ let toolhiveProcess: ReturnType | undefined let toolhivePort: number | undefined let toolhiveMcpPort: number | undefined let isRestarting = false +let killTimer: NodeJS.Timeout | undefined export function getToolhivePort(): number | undefined { return toolhivePort @@ -127,9 +128,11 @@ export async function startToolhive(): Promise { { stdio: ['ignore', 'ignore', 'pipe'], detached: false, + // Ensure child process is killed when parent exits + // On Windows, this creates a job object to enforce cleanup + windowsHide: true, } ) - log.info(`[startToolhive] Process spawned with PID: ${toolhiveProcess.pid}`) scope.addBreadcrumb({ @@ -213,15 +216,80 @@ export async function restartToolhive(): Promise { } } -export function stopToolhive(): void { - if (toolhiveProcess && !toolhiveProcess.killed) { - log.info('Stopping ToolHive process...') - toolhiveProcess.kill() - toolhiveProcess = undefined - log.info(`[stopToolhive] Process stopped and reset`) - } else { - log.info(`[stopToolhive] No process to stop`) +/** Attempt to kill a process, returning true on success */ +function tryKillProcess( + process: ReturnType, + signal: NodeJS.Signals, + logPrefix: string +): boolean { + try { + const result = process.kill(signal) + log.info(`${logPrefix} ${signal} sent, result: ${result}`) + return result + } catch (err) { + log.error(`${logPrefix} Failed to send ${signal}:`, err) + return false } } +/** Schedule delayed SIGKILL if process doesn't exit gracefully */ +function scheduleForceKill( + process: ReturnType, + pid: number +): void { + killTimer = setTimeout(() => { + killTimer = undefined + + if (!process.killed) { + log.warn( + `[stopToolhive] Process ${pid} did not exit gracefully, forcing SIGKILL...` + ) + tryKillProcess(process, 'SIGKILL', '[stopToolhive]') + } + }, 2000) +} + +export function stopToolhive(options?: { force?: boolean }): void { + const force = options?.force ?? false + + // Clear any pending kill timer + if (killTimer) { + clearTimeout(killTimer) + killTimer = undefined + } + + // Early return if no process to stop + if (!toolhiveProcess || toolhiveProcess.killed) { + log.info( + `[stopToolhive] No process to stop (process=${!!toolhiveProcess}, killed=${toolhiveProcess?.killed})` + ) + return + } + + const pidToKill = toolhiveProcess.pid + log.info(`Stopping ToolHive process (PID: ${pidToKill})...`) + + // Capture process reference before clearing global + const processToKill = toolhiveProcess + toolhiveProcess = undefined + + // Attempt to kill the process + const signal: NodeJS.Signals = force ? 'SIGKILL' : 'SIGTERM' + const killed = tryKillProcess(processToKill, signal, '[stopToolhive]') + + // If graceful shutdown failed, try force kill immediately + if (!killed) { + tryKillProcess(processToKill, 'SIGKILL', '[stopToolhive]') + log.info(`[stopToolhive] Process cleanup completed`) + return + } + + // For graceful shutdown, schedule delayed force kill + if (!force && pidToKill !== undefined) { + scheduleForceKill(processToKill, pidToKill) + } + + log.info(`[stopToolhive] Process cleanup completed`) +} + export { binPath }