diff --git a/src/main/speech/model-manager-download-error.test.ts b/src/main/speech/model-manager-download-error.test.ts index 14c33908e..b75bdc592 100644 --- a/src/main/speech/model-manager-download-error.test.ts +++ b/src/main/speech/model-manager-download-error.test.ts @@ -5,24 +5,22 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { SPEECH_MODEL_CATALOG } from './model-catalog' import { ModelManager } from './model-manager' -const { httpsGetMock } = vi.hoisted(() => ({ - httpsGetMock: vi.fn() +const { netRequestMock } = vi.hoisted(() => ({ + netRequestMock: vi.fn() })) vi.mock('electron', () => ({ app: { getPath: () => '/tmp/orca-speech-models-test' + }, + net: { + request: netRequestMock } })) -vi.mock('https', async () => { - const actual = await vi.importActual('https') - return { ...(actual as Record), get: httpsGetMock } -}) - describe('ModelManager download failures', () => { beforeEach(() => { - httpsGetMock.mockReset() + netRequestMock.mockReset() }) it('rejects failed model downloads so the caller can surface the error', async () => { @@ -31,8 +29,15 @@ describe('ModelManager download failures', () => { const manifest = SPEECH_MODEL_CATALOG[0] const errorHandlers: ((err: Error) => void)[] = [] const request = { - destroy: vi.fn(() => request), - setTimeout: vi.fn(() => request), + abort: vi.fn(() => request), + end: vi.fn(() => { + queueMicrotask(() => { + for (const handler of errorHandlers) { + handler(new Error('network down')) + } + }) + return request + }), on: vi.fn((event: string, cb: (err: Error) => void) => { if (event === 'error') { errorHandlers.push(cb) @@ -49,14 +54,7 @@ describe('ModelManager download failures', () => { return request }) } - httpsGetMock.mockImplementation(() => { - queueMicrotask(() => { - for (const handler of errorHandlers) { - handler(new Error('network down')) - } - }) - return request - }) + netRequestMock.mockReturnValue(request) const manager = new ModelManager(dir) await expect(manager.downloadModel(manifest.id)).rejects.toThrow('network down') diff --git a/src/main/speech/model-manager-stream-cleanup.test.ts b/src/main/speech/model-manager-stream-cleanup.test.ts index 6f665b292..246ccfe10 100644 --- a/src/main/speech/model-manager-stream-cleanup.test.ts +++ b/src/main/speech/model-manager-stream-cleanup.test.ts @@ -5,21 +5,19 @@ import { PassThrough } from 'stream' import { beforeEach, describe, expect, it, vi } from 'vitest' import { ModelManager } from './model-manager' -const { httpsGetMock } = vi.hoisted(() => ({ - httpsGetMock: vi.fn() +const { netRequestMock } = vi.hoisted(() => ({ + netRequestMock: vi.fn() })) vi.mock('electron', () => ({ app: { getPath: () => '/tmp/orca-speech-models-test' + }, + net: { + request: netRequestMock } })) -vi.mock('https', async () => { - const actual = await vi.importActual('https') - return { ...(actual as Record), get: httpsGetMock } -}) - type ModelManagerInternals = { downloadFile: ( url: string, @@ -33,7 +31,7 @@ type ModelManagerInternals = { describe('ModelManager stream cleanup', () => { beforeEach(() => { - httpsGetMock.mockReset() + netRequestMock.mockReset() }) it('removes response progress listeners after a model download finishes', async () => { @@ -45,16 +43,24 @@ describe('ModelManager stream cleanup', () => { } response.statusCode = 200 response.headers = { 'content-length': '4' } + const responseHandlers: ((response: unknown) => void)[] = [] const request = { - destroy: vi.fn(() => request), - setTimeout: vi.fn(() => request), - on: vi.fn(() => request), + abort: vi.fn(() => request), + end: vi.fn(() => { + for (const handler of responseHandlers) { + handler(response) + } + return request + }), + on: vi.fn((event: string, cb: (response: unknown) => void) => { + if (event === 'response') { + responseHandlers.push(cb) + } + return request + }), off: vi.fn(() => request) } - httpsGetMock.mockImplementation((_url: URL, cb: (response: unknown) => void) => { - cb(response) - return request - }) + netRequestMock.mockReturnValue(request) const manager = new ModelManager(dir) as unknown as ModelManagerInternals const download = manager.downloadFile( diff --git a/src/main/speech/model-manager.test.ts b/src/main/speech/model-manager.test.ts index 7879d86d8..dc5815845 100644 --- a/src/main/speech/model-manager.test.ts +++ b/src/main/speech/model-manager.test.ts @@ -6,15 +6,18 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { SPEECH_MODEL_CATALOG } from './model-catalog' import { ModelManager } from './model-manager' -const { hasOpenAiSpeechApiKeyMock, httpsGetMock, spawnMock } = vi.hoisted(() => ({ +const { hasOpenAiSpeechApiKeyMock, netRequestMock, spawnMock } = vi.hoisted(() => ({ hasOpenAiSpeechApiKeyMock: vi.fn(), - httpsGetMock: vi.fn(), + netRequestMock: vi.fn(), spawnMock: vi.fn() })) vi.mock('electron', () => ({ app: { getPath: () => '/tmp/orca-speech-models-test' + }, + net: { + request: netRequestMock } })) @@ -23,11 +26,6 @@ vi.mock('child_process', async () => { return { ...(actual as Record), spawn: spawnMock } }) -vi.mock('https', async () => { - const actual = await vi.importActual('https') - return { ...(actual as Record), get: httpsGetMock } -}) - vi.mock('./openai-api-key-store', () => ({ hasOpenAiSpeechApiKey: hasOpenAiSpeechApiKeyMock })) @@ -52,7 +50,7 @@ type ModelManagerInternals = { describe('ModelManager', () => { beforeEach(() => { - httpsGetMock.mockReset() + netRequestMock.mockReset() hasOpenAiSpeechApiKeyMock.mockReset() hasOpenAiSpeechApiKeyMock.mockReturnValue(false) spawnMock.mockReset() @@ -129,23 +127,30 @@ describe('ModelManager', () => { try { const manifest = SPEECH_MODEL_CATALOG[0] const errorHandlers: ((err: Error) => void)[] = [] - const timeoutHandlers: (() => void)[] = [] + const responseHandlers: ((response: unknown) => void)[] = [] + const redirectHandlers: (( + statusCode: number, + method: string, + redirectUrl: string + ) => void)[] = [] const request = { - destroy: vi.fn((err?: Error) => { + abort: vi.fn(() => { queueMicrotask(() => { for (const handler of errorHandlers) { - handler(err ?? new Error('destroyed')) + handler(new Error('Aborted')) } }) return request }), - setTimeout: vi.fn((_ms: number, cb: () => void) => { - timeoutHandlers.push(cb) - return request - }), on: vi.fn((event: string, cb: (err: Error) => void) => { if (event === 'error') { errorHandlers.push(cb) + } else if (event === 'response') { + responseHandlers.push(cb as unknown as (response: unknown) => void) + } else if (event === 'redirect') { + redirectHandlers.push( + cb as unknown as (statusCode: number, method: string, redirectUrl: string) => void + ) } return request }), @@ -156,75 +161,154 @@ describe('ModelManager', () => { errorHandlers.splice(index, 1) } } - if (event === 'timeout') { - const index = timeoutHandlers.indexOf(cb as () => void) + if (event === 'response') { + const index = responseHandlers.indexOf(cb as (response: unknown) => void) if (index !== -1) { - timeoutHandlers.splice(index, 1) + responseHandlers.splice(index, 1) + } + } + if (event === 'redirect') { + const index = redirectHandlers.indexOf( + cb as (statusCode: number, method: string, redirectUrl: string) => void + ) + if (index !== -1) { + redirectHandlers.splice(index, 1) } } return request - }) + }), + end: vi.fn(() => request) } - httpsGetMock.mockImplementation( - ( - _url: URL, - options: { signal?: AbortSignal } | ((response: unknown) => void), - _cb?: (response: unknown) => void - ) => { - if (typeof options !== 'function') { - options.signal?.addEventListener('abort', () => request.destroy(new Error('Aborted')), { - once: true - }) - } - return request - } - ) + netRequestMock.mockReturnValue(request) const manager = new ModelManager(dir) const download = manager.downloadModel(manifest.id) manager.cancelDownload(manifest.id) await expect(download).resolves.toBeUndefined() - expect(request.destroy).toHaveBeenCalledWith(expect.any(Error)) + expect(netRequestMock).toHaveBeenCalledWith({ + method: 'GET', + url: expect.stringMatching(/^https:\/\//) + }) + expect(request.end).toHaveBeenCalled() + expect(request.abort).toHaveBeenCalled() expect(request.off).toHaveBeenCalledWith('error', expect.any(Function)) - expect(request.off).toHaveBeenCalledWith('timeout', expect.any(Function)) + expect(request.off).toHaveBeenCalledWith('response', expect.any(Function)) + expect(request.off).toHaveBeenCalledWith('redirect', expect.any(Function)) expect(errorHandlers).toHaveLength(0) - expect(timeoutHandlers).toHaveLength(0) + expect(responseHandlers).toHaveLength(0) + expect(redirectHandlers).toHaveLength(0) expect((await manager.getModelState(manifest.id)).status).toBe('not-downloaded') } finally { rmSync(dir, { recursive: true, force: true }) } }) + it('settles immediately when the abort signal fires before a response', async () => { + vi.useFakeTimers() + const dir = mkdtempSync(join(tmpdir(), 'orca-model-manager-')) + try { + const errorHandlers: ((err: Error) => void)[] = [] + const responseHandlers: ((response: unknown) => void)[] = [] + const redirectHandlers: (( + statusCode: number, + method: string, + redirectUrl: string + ) => void)[] = [] + const request = { + abort: vi.fn(() => request), + on: vi.fn((event: string, cb: (err: Error) => void) => { + if (event === 'error') { + errorHandlers.push(cb) + } else if (event === 'response') { + responseHandlers.push(cb as unknown as (response: unknown) => void) + } else if (event === 'redirect') { + redirectHandlers.push( + cb as unknown as (statusCode: number, method: string, redirectUrl: string) => void + ) + } + return request + }), + off: vi.fn((event: string, cb: ((err: Error) => void) | (() => void)) => { + if (event === 'error') { + const index = errorHandlers.indexOf(cb as (err: Error) => void) + if (index !== -1) { + errorHandlers.splice(index, 1) + } + } + if (event === 'response') { + const index = responseHandlers.indexOf(cb as (response: unknown) => void) + if (index !== -1) { + responseHandlers.splice(index, 1) + } + } + if (event === 'redirect') { + const index = redirectHandlers.indexOf( + cb as (statusCode: number, method: string, redirectUrl: string) => void + ) + if (index !== -1) { + redirectHandlers.splice(index, 1) + } + } + return request + }), + end: vi.fn(() => request) + } + netRequestMock.mockReturnValue(request) + const controller = new AbortController() + const manager = new ModelManager(dir) as unknown as ModelManagerInternals + + const download = manager.downloadFile( + 'https://example.com/model.tar.bz2', + join(dir, 'model.tar.bz2'), + 1, + 'm', + () => true, + controller.signal + ) + const outcomePromise = download.then( + () => 'resolved', + (error) => (error instanceof Error ? error.message : String(error)) + ) + controller.abort() + await vi.advanceTimersByTimeAsync(0) + + await expect(outcomePromise).resolves.toBe('Aborted') + expect(request.abort).toHaveBeenCalled() + expect(request.off).toHaveBeenCalledWith('error', expect.any(Function)) + expect(request.off).toHaveBeenCalledWith('response', expect.any(Function)) + expect(request.off).toHaveBeenCalledWith('redirect', expect.any(Function)) + expect(errorHandlers).toHaveLength(0) + expect(responseHandlers).toHaveLength(0) + expect(redirectHandlers).toHaveLength(0) + } finally { + vi.useRealTimers() + rmSync(dir, { recursive: true, force: true }) + } + }) + it('times out a model download request that never responds', async () => { vi.useFakeTimers() const dir = mkdtempSync(join(tmpdir(), 'orca-model-manager-')) try { const errorHandlers: ((err: Error) => void)[] = [] - const timeoutHandlers: (() => void)[] = [] + const responseHandlers: ((response: unknown) => void)[] = [] + const redirectHandlers: (( + statusCode: number, + method: string, + redirectUrl: string + ) => void)[] = [] const request = { - destroy: vi.fn((err?: Error) => { - if (err) { - queueMicrotask(() => { - for (const handler of errorHandlers) { - handler(err) - } - }) - } - return request - }), - setTimeout: vi.fn((ms: number, cb: () => void) => { - timeoutHandlers.push(cb) - setTimeout(() => { - for (const handler of timeoutHandlers) { - handler() - } - }, ms) - return request - }), + abort: vi.fn(() => request), on: vi.fn((event: string, cb: (err: Error) => void) => { if (event === 'error') { errorHandlers.push(cb) + } else if (event === 'response') { + responseHandlers.push(cb as unknown as (response: unknown) => void) + } else if (event === 'redirect') { + redirectHandlers.push( + cb as unknown as (statusCode: number, method: string, redirectUrl: string) => void + ) } return request }), @@ -235,16 +319,25 @@ describe('ModelManager', () => { errorHandlers.splice(index, 1) } } - if (event === 'timeout') { - const index = timeoutHandlers.indexOf(cb as () => void) + if (event === 'response') { + const index = responseHandlers.indexOf(cb as (response: unknown) => void) if (index !== -1) { - timeoutHandlers.splice(index, 1) + responseHandlers.splice(index, 1) + } + } + if (event === 'redirect') { + const index = redirectHandlers.indexOf( + cb as (statusCode: number, method: string, redirectUrl: string) => void + ) + if (index !== -1) { + redirectHandlers.splice(index, 1) } } return request - }) + }), + end: vi.fn(() => request) } - httpsGetMock.mockReturnValue(request) + netRequestMock.mockReturnValue(request) const manager = new ModelManager(dir) as unknown as ModelManagerInternals const download = manager.downloadFile( @@ -263,11 +356,13 @@ describe('ModelManager', () => { const outcome = await Promise.race([outcomePromise, Promise.resolve('pending')]) expect(outcome).toBe('Model download timed out after 120 seconds without network activity') - expect(request.destroy).toHaveBeenCalledWith() + expect(request.abort).toHaveBeenCalledWith() expect(request.off).toHaveBeenCalledWith('error', expect.any(Function)) - expect(request.off).toHaveBeenCalledWith('timeout', expect.any(Function)) + expect(request.off).toHaveBeenCalledWith('response', expect.any(Function)) + expect(request.off).toHaveBeenCalledWith('redirect', expect.any(Function)) expect(errorHandlers).toHaveLength(0) - expect(timeoutHandlers).toHaveLength(0) + expect(responseHandlers).toHaveLength(0) + expect(redirectHandlers).toHaveLength(0) } finally { vi.useRealTimers() rmSync(dir, { recursive: true, force: true }) diff --git a/src/main/speech/model-manager.ts b/src/main/speech/model-manager.ts index 7f4f5b480..30c31468b 100644 --- a/src/main/speech/model-manager.ts +++ b/src/main/speech/model-manager.ts @@ -1,11 +1,9 @@ /* eslint-disable max-lines -- Why: model download, checksum, extraction, and cleanup share one state machine so progress/error transitions stay coupled. */ -import { app } from 'electron' +import { app, net } from 'electron' import { join, resolve, relative } from 'path' import { existsSync, mkdirSync, createWriteStream, createReadStream, rmSync } from 'fs' import { readdir, rm } from 'fs/promises' import { createHash } from 'crypto' -import { get as httpsGet } from 'https' -import type { IncomingMessage } from 'http' import { pipeline } from 'stream/promises' import { spawn } from 'child_process' import type { @@ -22,6 +20,12 @@ type DownloadHandle = { } type ProgressCallback = (modelId: string, progress: number) => void +type DownloadIncomingMessage = Electron.IncomingMessage & + NodeJS.ReadableStream & { + headers: Record + resume: () => void + destroy?: () => void + } const DOWNLOAD_IDLE_TIMEOUT_MS = 120_000 @@ -287,16 +291,35 @@ export class ModelManager { } let settled = false - let request: ReturnType | null = null + let request: Electron.ClientRequest | null = null + let idleTimeout: ReturnType | null = null + const onSignalAbort = (): void => { + const activeRequest = request + rejectOnce(new Error('Aborted')) + activeRequest?.abort() + } + const clearIdleTimeout = (): void => { + if (idleTimeout) { + clearTimeout(idleTimeout) + idleTimeout = null + } + } const cleanupRequestListeners = (): void => { const activeRequest = request + clearIdleTimeout() if (!activeRequest) { return } activeRequest.off('error', onRequestError) - activeRequest.off('timeout', onRequestTimeout) + activeRequest.off('response', onResponse) + activeRequest.off('redirect', onRedirect) + signal?.removeEventListener('abort', onSignalAbort) request = null } + const resetIdleTimeout = (): void => { + clearIdleTimeout() + idleTimeout = setTimeout(onRequestTimeout, DOWNLOAD_IDLE_TIMEOUT_MS) + } const resolveOnce = (): void => { if (settled) { return @@ -321,62 +344,57 @@ export class ModelManager { `Model download timed out after ${DOWNLOAD_IDLE_TIMEOUT_MS / 1000} seconds without network activity` ) ) - activeRequest?.destroy() + activeRequest?.abort() } - const onResponse = (response: IncomingMessage): void => { - if ( - response.statusCode === 301 || - response.statusCode === 302 || - response.statusCode === 303 || - response.statusCode === 307 || - response.statusCode === 308 - ) { - const redirectUrl = response.headers.location - if (!redirectUrl) { - response.resume() - rejectOnce(new Error('Redirect without location')) - return - } - if (redirectCount >= 5) { - response.resume() - rejectOnce(new Error('Too many redirects')) - return - } - let resolvedRedirect: URL - try { - resolvedRedirect = new URL(redirectUrl, parsedUrl) - } catch { - response.resume() - rejectOnce(new Error('Invalid redirect URL')) - return - } - if (resolvedRedirect.protocol !== 'https:') { - response.resume() - rejectOnce(new Error('Model download redirect must use HTTPS')) - return - } - response.resume() - this.downloadFile( - resolvedRedirect.toString(), - dest, - expectedSize, - modelId, - isAborted, - signal, - redirectCount + 1 - ) - .then(resolveOnce) - .catch(rejectOnce) + const onRedirect = (_statusCode: number, _method: string, redirectUrl: string): void => { + if (redirectCount >= 5) { + const activeRequest = request + rejectOnce(new Error('Too many redirects')) + activeRequest?.abort() return } - + let resolvedRedirect: URL + try { + resolvedRedirect = new URL(redirectUrl, parsedUrl) + } catch { + const activeRequest = request + rejectOnce(new Error('Invalid redirect URL')) + activeRequest?.abort() + return + } + if (resolvedRedirect.protocol !== 'https:') { + const activeRequest = request + rejectOnce(new Error('Model download redirect must use HTTPS')) + activeRequest?.abort() + return + } + const activeRequest = request + cleanupRequestListeners() + activeRequest?.abort() + this.downloadFile( + resolvedRedirect.toString(), + dest, + expectedSize, + modelId, + isAborted, + signal, + redirectCount + 1 + ) + .then(resolveOnce) + .catch(rejectOnce) + } + const onResponse = (incoming: Electron.IncomingMessage): void => { + const response = incoming as DownloadIncomingMessage if (response.statusCode !== 200) { response.resume() rejectOnce(new Error(`HTTP ${response.statusCode}`)) return } - const totalSize = parseInt(response.headers['content-length'] || '0', 10) || expectedSize + const contentLength = response.headers['content-length'] + const totalSize = + parseInt(Array.isArray(contentLength) ? contentLength[0] : contentLength || '0', 10) || + expectedSize let downloaded = 0 const fileStream = createWriteStream(dest) @@ -385,9 +403,10 @@ export class ModelManager { response.off('data', onResponseData) } const onResponseData = (chunk: Buffer): void => { + resetIdleTimeout() if (isAborted()) { - request?.destroy(new Error('Aborted')) - response.destroy() + request?.abort() + response.destroy?.() fileStream.destroy() return } @@ -412,15 +431,18 @@ export class ModelManager { }) } - request = signal - ? httpsGet(parsedUrl, { signal }, onResponse) - : httpsGet(parsedUrl, onResponse) + request = net.request({ method: 'GET', url: parsedUrl.toString() }) - // Why: cancellation only helps after the user presses cancel; a peer - // that accepts the socket and goes silent must not leave the model stuck - // in "downloading" forever. - request.setTimeout(DOWNLOAD_IDLE_TIMEOUT_MS, onRequestTimeout) + // Why: Electron's net stack honors app proxy settings, unlike Node's + // https client, but it does not expose request.setTimeout(). + resetIdleTimeout() request.on('error', onRequestError) + request.on('response', onResponse) + request.on('redirect', onRedirect) + if (signal) { + signal.addEventListener('abort', onSignalAbort, { once: true }) + } + request.end() }) }