Repository navigation
fix(voice): make model downloads atomic and route stream write errors - #1574
Merged
Merged
Conversation
Two defects from vastsa#1571: the WriteStream's error listener attached only after the copy loop, so an ENOSPC/EIO mid-download was an unhandled 'error' event in the Electron main process — an app crash; and bytes went straight to the final path, so an interrupted download left a truncated model that existsSync then reported as downloaded forever, with re-download a silent no-op. Write to <target>.part and rename into place after the stream closes; route stream errors into the copy loop through a racing rejection; and await the stream's close (handle release) before renaming or unlinking, both of which fail with EBUSY/EPERM on Windows against an open handle. An interrupted download now cleans up its .part file and re-downloading just works.
When disk errors win the stream race, the generator now releases the open response reader before cleaning up the partial file. This prevents a failed download from continuing to consume the response in the background.
Owner
|
Thanks for the fix. The writer-error crash and partial final-file risks were real. I added cancellation of the response reader when a write fails, so the remote stream stops instead of continuing in the background. The updated candidate passed the targeted regression tests, typecheck, PR-base check, and Electron E2E, then merged as 647548d. |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1571
Summary
<target>.partandrenameinto place only after the stream closes, so an interrupted download can no longer leave a truncated file forModelManager.initializeStates'sexistsSyncto report asdownloaded— re-downloading after a mid-download crash/force-quit just works.WriteStream'serrorlistener is attached before the copy loop starts, and its rejection races the source reads: an ENOSPC/EIO mid-download now surfaces as a normal download failure (stateerror, retryable from the UI) instead of an unhandlederrorevent in the Electron main process.close(handle release) is awaited before the rename and before unlinking a failed.part— both fail with EBUSY/EPERM on Windows against an open handle.Rationale
Described in #1571: the crash path and the unrecoverable truncated-file state share one root cause — the final path was written directly with errors unobserved mid-stream. The temp-file + rename split removes the truncated-state failure mode entirely, and the error routing turns the crash into a retryable failure.
Affected specs / ADRs / E2E scenarios
test:e2e:live-voiceetc.: NOT RUN — those cover the realtime call path, and download E2E would need real hub traffic. Validation below is a new unit suite over the real downloader with a stubbed hub.Validation (on latest
origin/maineec988c)apps/desktop/test/voice-model-download.test.mjs(3 tests, over the realmodel-downloader.tswith@huggingface/hubstubbed through module hooks): completed download lands at the final path with no.partresidue; mid-stream source failure rejects and leaves neither file; an unwritable.partpath rejects as a download failure instead of an uncaught stream error. 5× repeat runs stable.tsc --noEmitinpackages/voice-runtimeandapps/desktop: pass.voice-service-idle-unload(7),voice-settings-surface(2).Compatibility / remaining risk
.partsuffix lives inside the model cache directory; nothing else reads that directory, and stale.partfiles from crashes are inert (never match the final path).download()short-circuits only on the final path now, so pre-existing truncated files from older builds — if any user has one — become re-downloadable, which is the intended repair.ModelManageris untouched.