feat(main): trying to always kill thv process

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

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<typeof spawn>
+      )
+
+      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<typeof spawn>
+      )
+
+      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<typeof spawn>
+      )
+
+      const restart1 = restartToolhive()
+      const restart2 = restartToolhive()
+
+      await vi.advanceTimersByTimeAsync(50)
+      await Promise.all([restart1, restart2])
+
+      expect(mockLog.info).toHaveBeenCalledWith(
+        'Restart already in progress, skipping...'
+      )
+    })
+  })
 })