From 4201dd99bf21f72ffdbfd45e8f9106c5f33501ea Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:46:18 -0700 Subject: [PATCH] fix(speech): don't abandon a download that keeps making progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry loop gave up after 8 total failures regardless of progress, so a large model (e.g. the ~1GB Paraformer) over a network that resets every few hundred MB was abandoned mid-download even though every attempt was resuming and advancing — the exact 'stuck then fails' symptom, just later. Replace the all-time failure budget and the separate advancing-segment cap with a single rule: give up only on a genuine stall (repeated attempts with no forward progress) or an absolute request ceiling that bounds a pathological tiny-segment server. Any download that keeps advancing now runs to completion. Verified live under Electron 43: a transfer reset mid-stream on every attempt but advancing each time previously gave up at 67% after 8 attempts; it now completes. All prior resume/cancel/timeout scenarios still pass. --- .../model-manager-download-resume.test.ts | 55 +++++++++++++++++-- src/main/speech/model-manager.ts | 46 +++++++++------- 2 files changed, 77 insertions(+), 24 deletions(-) diff --git a/src/main/speech/model-manager-download-resume.test.ts b/src/main/speech/model-manager-download-resume.test.ts index 4f2d16470b3..bbfc88e7487 100644 --- a/src/main/speech/model-manager-download-resume.test.ts +++ b/src/main/speech/model-manager-download-resume.test.ts @@ -282,12 +282,14 @@ describe('ModelManager download resume', () => { } }) - it('bounds pathological advancing range segment requests', async () => { + it('bounds a server that advances by pathologically tiny segments forever', async () => { const dir = mkdtempSync(join(tmpdir(), 'orca-model-resume-')) try { const manager = new ModelManager(dir) as unknown as ModelManagerInternals const archivePath = join(dir, 'model.tar.bz2') let bytesWritten = 0 + // Advances one byte per request against a total larger than the request + // ceiling, so it makes forward progress forever without ever completing. const downloadFileMock = vi.spyOn(manager, 'downloadFile').mockImplementation(() => { bytesWritten += 1 writeFileSync(archivePath, Buffer.alloc(bytesWritten)) @@ -298,19 +300,64 @@ describe('ModelManager download resume', () => { manager.downloadArchiveWithRetry( 'https://example.com/model.tar.bz2', archivePath, - 300, + 1_000_000, 'm', () => false, new AbortController().signal ) - ).rejects.toThrow(/after 256 attempts/) + ).rejects.toThrow(/too many download requests/) - expect(downloadFileMock).toHaveBeenCalledTimes(256) + // The absolute request ceiling (MAX_TOTAL_DOWNLOAD_REQUESTS) bounds it. + expect(downloadFileMock).toHaveBeenCalledTimes(4_096) } finally { rmSync(dir, { recursive: true, force: true }) } }) + it('keeps resuming a download that advances across many mid-stream drops', async () => { + vi.useFakeTimers() + const dir = mkdtempSync(join(tmpdir(), 'orca-model-resume-')) + try { + // Every attempt delivers a small slice then drops mid-stream — the + // classic "dies partway, resumes" pattern. Because each attempt makes + // forward progress, the download must complete no matter how many drops + // it takes (regression guard: a fixed failure budget used to abandon a + // still-advancing large download around attempt 8). + const manager = new ModelManager(dir) as unknown as ModelManagerInternals + const archivePath = join(dir, 'model.tar.bz2') + const SLICE = 2 + let delivered = 0 + const downloadFileMock = vi.spyOn(manager, 'downloadFile').mockImplementation(() => { + delivered = Math.min(delivered + SLICE, PAYLOAD.length) + writeFileSync(archivePath, PAYLOAD.subarray(0, delivered)) + return Promise.reject(new Error('net::ERR_CONNECTION_RESET')) + }) + + const download = manager.downloadArchiveWithRetry( + 'https://example.com/model.tar.bz2', + archivePath, + PAYLOAD.length, + 'm', + () => false, + new AbortController().signal + ) + const outcome = download.then( + () => 'resolved', + (error: unknown) => (error instanceof Error ? error.message : String(error)) + ) + // Drive the per-attempt backoff (1s each, since progress resets the + // stall counter) well past the ten attempts this needs. + await vi.advanceTimersByTimeAsync(30_000) + + await expect(outcome).resolves.toBe('resolved') + expect(downloadFileMock).toHaveBeenCalledTimes(PAYLOAD.length / SLICE) + expect(readFileSync(archivePath)).toEqual(PAYLOAD) + } finally { + vi.useRealTimers() + rmSync(dir, { recursive: true, force: true }) + } + }) + it('keeps the known archive total when Content-Range omits it', async () => { const dir = mkdtempSync(join(tmpdir(), 'orca-model-resume-')) try { diff --git a/src/main/speech/model-manager.ts b/src/main/speech/model-manager.ts index fc83539bd34..c8f918f1526 100644 --- a/src/main/speech/model-manager.ts +++ b/src/main/speech/model-manager.ts @@ -51,10 +51,17 @@ const DOWNLOAD_IDLE_TIMEOUT_MS = 120_000 // request lets those connections finish instead of failing the same way on // every full restart. const DOWNLOAD_RETRY_DELAYS_MS = [1_000, 2_000, 4_000] -const MAX_DOWNLOAD_FAILURES = 8 -// Why: successful bounded ranges may require more than the failure budget, but -// a server returning pathological tiny segments must not create an endless loop. -const MAX_ADVANCING_RANGE_SEGMENTS = 256 +// Why: abandon a download only after this many CONSECUTIVE attempts make no +// forward progress. A resume that keeps advancing (even across many mid-stream +// drops) is never given up on — that is the whole point of resumable downloads +// for large models over flaky networks, and a fixed all-time failure budget +// would wrongly kill a steadily-progressing large download. +const MAX_NO_PROGRESS_ATTEMPTS = DOWNLOAD_RETRY_DELAYS_MS.length + 1 +// Why: an absolute backstop so a server handing back pathologically tiny (or +// zero-net) segments cannot loop forever; sized to cover the largest (~1GB) +// model even if a proxy caps each range response near ~256KB, far below any +// real CDN chunk. +const MAX_TOTAL_DOWNLOAD_REQUESTS = 4_096 // Why: a longer server window should be surfaced for a later manual retry, // rather than leaving the settings download looking stuck for minutes. const MAX_RETRY_AFTER_MS = 120_000 @@ -454,8 +461,6 @@ export class ModelManager { signal: AbortSignal ): Promise { let requestCount = 0 - let retryFailures = 0 - let advancingRangeSegments = 0 let noProgressStreak = 0 const totals: DownloadTotals = { totalBytes: expectedSize } for (;;) { @@ -466,6 +471,16 @@ export class ModelManager { if (offset === totals.totalBytes) { return } + // Why: absolute backstop against a server that never lets the download + // finish; the stall and abort paths handle the common cases first. + if (requestCount > MAX_TOTAL_DOWNLOAD_REQUESTS) { + throw describeInterruptedDownload( + new Error('too many download requests'), + offset, + totals.totalBytes, + requestCount - 1 + ) + } try { // Why: each attempt restarts from the canonical URL rather than the // last redirect target, because CDN redirect URLs are signed and expire. @@ -495,16 +510,9 @@ export class ModelManager { if (receivedBytes > offset) { // Why: some proxies return a valid but bounded range segment; request // the next segment immediately instead of failing checksum or waiting. + // Forward progress resets the stall counter; the total-request ceiling + // at the loop top bounds a pathological tiny-segment server. noProgressStreak = 0 - advancingRangeSegments += 1 - if (advancingRangeSegments >= MAX_ADVANCING_RANGE_SEGMENTS) { - throw describeInterruptedDownload( - incompleteResponse, - receivedBytes, - totals.totalBytes, - requestCount - ) - } continue } const retryableIncompleteResponse = incompleteResponse as HttpStatusError @@ -522,11 +530,9 @@ export class ModelManager { if (!isRetryableDownloadError(err)) { throw err } - retryFailures += 1 - if ( - retryFailures >= MAX_DOWNLOAD_FAILURES || - noProgressStreak > DOWNLOAD_RETRY_DELAYS_MS.length - ) { + // Why: give up only on a genuine stall (repeated no forward progress); + // a download that keeps advancing across drops is allowed to continue. + if (noProgressStreak >= MAX_NO_PROGRESS_ATTEMPTS) { throw describeInterruptedDownload(err, receivedBytes, totals.totalBytes, requestCount) } const retryAfterMs = (err as HttpStatusError).retryAfterMs