fix(stt): verify cached model checksum - #239
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
89e07a8 to
545043d
Compare
594c29f to
597f664
Compare
597f664 to
ac1bce7
Compare
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Both holes are real. The second is the better catch: if (await areModelsPresent(opts.baseDir)) continue; gated the entire loop on the whisper file existing, so a second entry in STT_MODELS would silently never download. And verifying before the final rename rather than after is plainly right — main's version parks a corrupt file at the live model path on its way to throwing.
What I can't take is the quarantine. rename(tmp, filePath) is already atomic, so .bad closes no window; it only destroys the user's only copy before we know a replacement is obtainable. Offline, HF 5xx, disk full — main left a model behind, this leaves none, plus a stranded 264 MB file nothing in this tree ever deletes.
The failure mode I'd want covered before merge is upstream digest drift; details inline. Short version: the model URL goes through resolve/main, so the pin is a bet, and this PR converts "breaks new installs" into "invalidates every existing cache at once."
Rebase note: main has moved 30 commits since your base, including an STT status rework, but it only added completedSec/totalSec to SttStatusEvent and modelManager.ts never touches that type — you're clean there. One UX consequence: prepare() emits phase: "model" before ensureModels, which the renderer maps to mediaStage.downloadingModel, so the new cache hash renders as "Downloading speech model" for a second. Worth a separate beat or a reworded string.
| const badPath = `${filePath}.bad`; | ||
| await rm(badPath, { force: true }); | ||
| await rename(filePath, badPath); |
There was a problem hiding this comment.
This is the part that needs to go. rename(tmp, filePath) on line 203 is already atomic, so quarantining first closes no window — it just removes the only copy on disk before we know a replacement is obtainable. fetchWithRetry throws when the machine is offline and on a non-retryable HTTP status; pipeline throws on ENOSPC. In each of those, main still had a model at the live path and this leaves the user with none, plus a 264 MB .bad that nothing in this repo ever cleans up. It also doubles peak disk (.bad + .partial) in exactly the path meant to recover from trouble. Drop 171-173: leave the stale file where it is, write .partial, verify that, and let the final rename replace it.
| if (!expectedSha256) return; | ||
| const actual = await sha256OfFile(filePath); |
There was a problem hiding this comment.
This changes who a stale pin hurts, and I think that deserves an answer in the PR. STT_MODELS.whisper.files[0].url resolves through resolve/main — a mutable branch pointer — so expectedSha256 describes whatever HuggingFace served the day it was recorded, not an immutable revision. Today a re-upload breaks first-time downloads only; everyone with a cache skips the check and keeps working. After this, every cached install is invalidated on the same launch, and since the fresh download mismatches too it throws, so they land on no model rather than a stale one. Pinning the URL to a commit SHA in this same PR would make the digest an actual invariant instead of a bet on main.
| const badPath = `${filePath}.bad`; | ||
| await rm(badPath, { force: true }); | ||
| await rename(tmp, badPath); |
There was a problem hiding this comment.
main had .catch(() => undefined) on this rename and it was load-bearing. On Windows, renaming a file we just finished writing hits EPERM/EBUSY often enough to matter — AV scanners hold a handle for a beat after the stream closes. Unguarded, that errno propagates out of ensureFile and the user sees a bare EPERM: operation not permitted, rename instead of the SHA-256 mismatch message that tells them what actually went wrong. Restore the .catch so the informative error is the one that escapes. Same applies to the rename on line 173 if any form of it survives.
2 participants
Summary
Related issue
No linked issue; found by corrupting a non-empty cached model and exercising startup.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; model-cache integrity behavior.
Testing