fix: clear model extraction abort pollers (#2881)

src/main/speech/model-manager.test.ts

@@ -2,16 +2,25 @@ import { createHash } from 'crypto'
 import { mkdtempSync, rmSync, writeFileSync } from 'fs'
 import { tmpdir } from 'os'
 import { join } from 'path'
-import { describe, expect, it, vi } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
 import { SPEECH_MODEL_CATALOG } from './model-catalog'
 import { ModelManager } from './model-manager'
 
+const { spawnMock } = vi.hoisted(() => ({
+  spawnMock: vi.fn()
+}))
+
 vi.mock('electron', () => ({
   app: {
     getPath: () => '/tmp/orca-speech-models-test'
   }
 }))
 
+vi.mock('child_process', async () => {
+  const actual = await vi.importActual('child_process')
+  return { ...(actual as Record<string, unknown>), spawn: spawnMock }
+})
+
 type ModelManagerInternals = {
   verifyArchiveSha256: (archivePath: string, expectedSha256: string) => Promise<void>
   downloadFile: (
@@ -21,9 +30,19 @@ type ModelManagerInternals = {
     modelId: string,
     isAborted: () => boolean
   ) => Promise<void>
+  extractArchive: (
+    archivePath: string,
+    destDir: string,
+    modelId: string,
+    isAborted: () => boolean
+  ) => Promise<void>
 }
 
 describe('ModelManager', () => {
+  beforeEach(() => {
+    spawnMock.mockReset()
+  })
+
   it('requires pinned SHA-256 hashes for every catalog archive', () => {
     for (const manifest of SPEECH_MODEL_CATALOG) {
       expect(manifest.archiveSha256).toMatch(/^[a-f0-9]{64}$/)
@@ -65,4 +84,59 @@ describe('ModelManager', () => {
       rmSync(dir, { recursive: true, force: true })
     }
   })
+
+  it('clears extraction abort polling when the child does not close', async () => {
+    vi.useFakeTimers()
+    const dir = mkdtempSync(join(tmpdir(), 'orca-model-manager-'))
+    try {
+      const handlers: Record<string, ((arg?: unknown) => void)[]> = {
+        close: [],
+        error: []
+      }
+      const stderrHandlers: ((chunk: Buffer) => void)[] = []
+      const child = {
+        stderr: {
+          on: vi.fn((_event: string, cb: (chunk: Buffer) => void) => {
+            stderrHandlers.push(cb)
+            return child.stderr
+          }),
+          off: vi.fn((_event: string, cb: (chunk: Buffer) => void) => {
+            const index = stderrHandlers.indexOf(cb)
+            if (index !== -1) {
+              stderrHandlers.splice(index, 1)
+            }
+            return child.stderr
+          })
+        },
+        kill: vi.fn(),
+        on: vi.fn((event: string, cb: (arg?: unknown) => void) => {
+          handlers[event]?.push(cb)
+          return child
+        }),
+        off: vi.fn((event: string, cb: (arg?: unknown) => void) => {
+          handlers[event] = handlers[event]?.filter((handler) => handler !== cb) ?? []
+          return child
+        })
+      }
+      spawnMock.mockReturnValue(child)
+      const manager = new ModelManager(dir) as unknown as ModelManagerInternals
+
+      const extraction = manager.extractArchive(join(dir, 'model.tar.bz2'), dir, 'm', () => true)
+      const rejection = expect(extraction).rejects.toThrow('Aborted')
+      await vi.advanceTimersByTimeAsync(250)
+      await rejection
+
+      expect(child.kill).toHaveBeenCalledWith('SIGKILL')
+      expect(child.kill).toHaveBeenCalledTimes(1)
+      expect(handlers.close).toHaveLength(0)
+      expect(handlers.error).toHaveLength(0)
+      expect(stderrHandlers).toHaveLength(0)
+
+      vi.advanceTimersByTime(1000)
+      expect(child.kill).toHaveBeenCalledTimes(1)
+    } finally {
+      vi.useRealTimers()
+      rmSync(dir, { recursive: true, force: true })
+    }
+  })
 })

src/main/speech/model-manager.ts

@@ -367,36 +367,66 @@ export class ModelManager {
       )
 
       let stderr = ''
-      child.stderr?.on('data', (chunk: Buffer) => {
-        stderr += chunk.toString()
-      })
-
-      const timeout = setTimeout(() => {
-        child.kill('SIGKILL')
-        reject(new Error('Extraction timed out after 10 minutes'))
-      }, 600_000)
-      const abortPoll = setInterval(() => {
-        if (isAborted()) {
-          child.kill('SIGKILL')
-          reject(new Error('Aborted'))
+      let settled = false
+      let timeout: ReturnType<typeof setTimeout> | null = null
+      let abortPoll: ReturnType<typeof setInterval> | null = null
+      const cleanup = (): void => {
+        if (timeout) {
+          clearTimeout(timeout)
+          timeout = null
         }
-      }, 250)
-
-      child.on('close', (code) => {
-        clearTimeout(timeout)
-        clearInterval(abortPoll)
+        if (abortPoll) {
+          clearInterval(abortPoll)
+          abortPoll = null
+        }
+        child.stderr?.off('data', onStderrData)
+        child.off('close', onClose)
+        child.off('error', onError)
+      }
+      const fail = (error: Error, killChild = false): void => {
+        if (settled) {
+          return
+        }
+        settled = true
+        cleanup()
+        if (killChild) {
+          child.kill('SIGKILL')
+        }
+        reject(error)
+      }
+      const onStderrData = (chunk: Buffer): void => {
+        stderr += chunk.toString()
+      }
+      const onClose = (code: number | null): void => {
+        if (settled) {
+          return
+        }
+        settled = true
+        cleanup()
         if (code === 0) {
           resolve()
         } else {
           reject(new Error(`tar exited with code ${code}: ${stderr.slice(0, 500)}`))
         }
-      })
+      }
+      const onError = (err: Error): void => {
+        fail(err)
+      }
 
-      child.on('error', (err) => {
-        clearTimeout(timeout)
-        clearInterval(abortPoll)
-        reject(err)
-      })
+      child.stderr?.on('data', onStderrData)
+      timeout = setTimeout(() => {
+        fail(new Error('Extraction timed out after 10 minutes'), true)
+      }, 600_000)
+      abortPoll = setInterval(() => {
+        if (isAborted()) {
+          // Why: if the extraction child wedges and never emits close/error,
+          // the abort poller must still clear itself when we reject.
+          fail(new Error('Aborted'), true)
+        }
+      }, 250)
+
+      child.on('close', onClose)
+      child.on('error', onError)
     })
   }