From 7e86a73bb69644011992255396d9f6309cb53368 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 23 Oct 2025 18:26:23 +0200 Subject: [PATCH 1/2] feat(main): trying to always kill thv process --- main/src/main.ts | 29 ++++++++++++++++++++++++++- main/src/toolhive-manager.ts | 39 +++++++++++++++++++++++++++++++----- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/main/src/main.ts b/main/src/main.ts index 5276dcd41..2cddb7ddb 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, no async + stopToolhive() +}) + ipcMain.handle('dark-mode:toggle', () => { nativeTheme.themeSource = nativeTheme.shouldUseDarkColors ? 'light' : 'dark' return nativeTheme.shouldUseDarkColors diff --git a/main/src/toolhive-manager.ts b/main/src/toolhive-manager.ts index 58f0217de..3dcab50d2 100644 --- a/main/src/toolhive-manager.ts +++ b/main/src/toolhive-manager.ts @@ -127,9 +127,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({ @@ -215,12 +217,39 @@ export async function restartToolhive(): Promise { export function stopToolhive(): void { if (toolhiveProcess && !toolhiveProcess.killed) { - log.info('Stopping ToolHive process...') - toolhiveProcess.kill() + log.info(`Stopping ToolHive process (PID: ${toolhiveProcess.pid})...`) + + try { + const killed = toolhiveProcess.kill('SIGTERM') + log.info(`[stopToolhive] SIGTERM sent, result: ${killed}`) + + setTimeout(() => { + if (toolhiveProcess && !toolhiveProcess.killed) { + log.warn( + '[stopToolhive] Process did not exit gracefully, forcing SIGKILL...' + ) + try { + toolhiveProcess.kill('SIGKILL') + } catch (err) { + log.error('[stopToolhive] Failed to force kill:', err) + } + } + }, 2000) + } catch (err) { + log.error('[stopToolhive] Error killing process:', err) + try { + toolhiveProcess.kill('SIGKILL') + } catch (forceErr) { + log.error('[stopToolhive] Failed to force kill:', forceErr) + } + } + toolhiveProcess = undefined - log.info(`[stopToolhive] Process stopped and reset`) + log.info(`[stopToolhive] Process cleanup completed`) } else { - log.info(`[stopToolhive] No process to stop`) + log.info( + `[stopToolhive] No process to stop (process=${!!toolhiveProcess}, killed=${toolhiveProcess?.killed})` + ) } } From 884a2948ce4c036860c56fe5539fc9ebed3054e5 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 23 Oct 2025 18:48:47 +0200 Subject: [PATCH 2/2] refactor: more readable code and adjust unit tests --- main/src/main.ts | 4 +- main/src/tests/toolhive-manager.test.ts | 251 +++++++++++++++++++++++- main/src/toolhive-manager.ts | 99 +++++++--- 3 files changed, 319 insertions(+), 35 deletions(-) diff --git a/main/src/main.ts b/main/src/main.ts index 2cddb7ddb..a68779569 100644 --- a/main/src/main.ts +++ b/main/src/main.ts @@ -389,8 +389,8 @@ app.on('quit', () => { process.on('exit', (code) => { log.info(`[process exit] code=${code}, ensuring ToolHive is stopped...`) - // Note: Only synchronous operations work here, no async - stopToolhive() + // Note: Only synchronous operations work here, so we force immediate SIGKILL + stopToolhive({ force: true }) }) ipcMain.handle('dark-mode:toggle', () => { 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 3dcab50d2..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 @@ -215,42 +216,80 @@ export async function restartToolhive(): Promise { } } -export function stopToolhive(): void { - if (toolhiveProcess && !toolhiveProcess.killed) { - log.info(`Stopping ToolHive process (PID: ${toolhiveProcess.pid})...`) - - try { - const killed = toolhiveProcess.kill('SIGTERM') - log.info(`[stopToolhive] SIGTERM sent, result: ${killed}`) - - setTimeout(() => { - if (toolhiveProcess && !toolhiveProcess.killed) { - log.warn( - '[stopToolhive] Process did not exit gracefully, forcing SIGKILL...' - ) - try { - toolhiveProcess.kill('SIGKILL') - } catch (err) { - log.error('[stopToolhive] Failed to force kill:', err) - } - } - }, 2000) - } catch (err) { - log.error('[stopToolhive] Error killing process:', err) - try { - toolhiveProcess.kill('SIGKILL') - } catch (forceErr) { - log.error('[stopToolhive] Failed to force kill:', forceErr) - } +/** 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) +} - toolhiveProcess = undefined - log.info(`[stopToolhive] Process cleanup completed`) - } else { +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 }