fix(captions): upgrade Whisper and recover GPU startup - #345
fix(captions): upgrade Whisper and recover GPU startup#345vitaligusatinsky wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 18 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 (6)
📝 WalkthroughWalkthroughThe STT model changes to large-v3-turbo q5_0. Whisper startup supports GPU initialization with CPU fallback. STT shutdown now cancels active work, stops the helper, releases the singleton, and runs before Electron quits. ChangesSTT model and runtime configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/stt/index.ts (1)
253-257: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck cancellation after a successful final chunk.
If shutdown starts while
server.transcribe()is resolving,transcribeChunk()can return a successful result. The final chunk then returns a completed transcript because no cancellation check follows this await.Check
this.cancelEpochorthis.shuttingDownaftertranscribeChunk()resolves and before accepting its result. Add a test where the fake helper callsshutdownStt()and then resolves successfully.Proposed fix
const result = await this.transcribeChunk( req.samples.subarray(chunk.startSample, chunk.endSample), language, ).catch((error) => { if (error instanceof Error && error.name === "AbortError") throw error; // ... }); + if (this.shuttingDown || this.cancelEpoch !== epoch) throw cancelledError();As per coding guidelines, “Add a test for every new behavior in the same package as the code under test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/stt/index.ts` around lines 253 - 257, Update the final-chunk flow around transcribeChunk to check this.cancelEpoch or this.shuttingDown immediately after a successful resolution and before accepting or returning its result, propagating cancellation instead. Add a same-package test whose fake transcription helper calls shutdownStt() and then resolves successfully, verifying the request is cancelled rather than producing a completed transcript.Source: Coding guidelines
electron/stt/whisperServer.ts (1)
131-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the readiness probe request; the loop can hang past the deadline.
fetch(baseUrl)has no timeout signal. Node's global fetch does not apply a default request deadline. If the helper accepts the TCP connection but never sends a response, theawaitnever settles. TheDate.now() < deadlinetest only runs at the top of the loop, so the 60 s budget never fires.start()then hangs forever, because the outerPromise.raceonly covers process exit, and a wedged-but-alive helper never emitsexit.Attach a per-request signal so each probe fails fast and the loop re-checks the deadline.
🛡️ Proposed fix to bound each readiness probe
try { - const res = await fetch(baseUrl, { method: "GET" }); + const res = await fetch(baseUrl, { + method: "GET", + signal: AbortSignal.timeout(2_000), + }); if (res.ok) return; } catch { // not up yet }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/stt/whisperServer.ts` around lines 131 - 149, Update pollUntilReady so each fetch probe is bounded by the remaining time until deadline, using a per-request abort signal and ensuring the signal is cleaned up after the request settles. Preserve the existing readiness, shouldContinue, retry-delay, and timeout error behavior while allowing the loop to re-check the deadline when a probe hangs.
🧹 Nitpick comments (1)
scripts/build-whisper-stt.sh (1)
240-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale
-aexplanation above this block.The comment at lines 220-222 still states that
-apreserves the symlink farm. The code now usescp -P. The two comments contradict each other within the same function. Change the earlier comment to name-P.The
rm -fpluscp -Psequence itself is correct.-Pdoes not resolve the link target, so the farm is recreated in glob order without requiring the target to exist first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-whisper-stt.sh` around lines 240 - 246, Update the earlier symlink-preservation comment in the same function to refer to cp -P instead of -a, keeping the rm -f and cp -P implementation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/stt/index.ts`:
- Around line 167-171: Update WhisperServerManager.start() handling in prepare()
to catch startup rejections and, when this.shuttingDown is true, replace them
with cancelledError(); otherwise preserve and rethrow the original error. Add a
regression test in the same package covering start() triggering shutdown before
rejecting, and verify the in-flight transcribe() reports an AbortError.
In `@electron/stt/whisperServer.test.ts`:
- Around line 287-342: Pin process.platform to "linux" for this test and restore
its original value in the finally cleanup so WhisperServerManager.start
consistently exercises the non-Windows executable-check path. Rename the child()
helper’s local const process to avoid shadowing the global process object, while
preserving the existing mocked child behavior.
In `@electron/stt/whisperServer.ts`:
- Around line 285-297: Update the gpuStartupFailed detection in the launch retry
block to evaluate stderr diagnostics line by line, allowing backend and failure
indicators on separate lines while preserving the existing backend filtering and
failure-token precision. Keep the current CPU fallback behavior unchanged once a
matching line is found.
- Around line 219-254: Serialize concurrent WhisperServer start() calls with a
stored in-flight start promise on the server instance. Have later start() calls
await and reuse the existing promise, while clearing it after completion or
failure; preserve the existing process/port checks and launch behavior so only
one child process can be spawned.
In `@technical-documentation/architecture/transcription-and-captions.md`:
- Around line 227-236: Update the document’s stale DTW preset reference to
WHISPER_AHEADS_LARGE_V3_TURBO and change the whisper-server first-run readiness
budget from 30 seconds to 60 seconds. Revise the preceding long-form chunking
paragraph so it accurately reflects sequential bounded chunking and timestamp
restoration rather than claiming no manual windowing.
---
Outside diff comments:
In `@electron/stt/index.ts`:
- Around line 253-257: Update the final-chunk flow around transcribeChunk to
check this.cancelEpoch or this.shuttingDown immediately after a successful
resolution and before accepting or returning its result, propagating
cancellation instead. Add a same-package test whose fake transcription helper
calls shutdownStt() and then resolves successfully, verifying the request is
cancelled rather than producing a completed transcript.
In `@electron/stt/whisperServer.ts`:
- Around line 131-149: Update pollUntilReady so each fetch probe is bounded by
the remaining time until deadline, using a per-request abort signal and ensuring
the signal is cleaned up after the request settles. Preserve the existing
readiness, shouldContinue, retry-delay, and timeout error behavior while
allowing the loop to re-check the deadline when a probe hangs.
---
Nitpick comments:
In `@scripts/build-whisper-stt.sh`:
- Around line 240-246: Update the earlier symlink-preservation comment in the
same function to refer to cp -P instead of -a, keeping the rm -f and cp -P
implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28620b75-eb5a-4a30-86c7-e1356bef04b3
📒 Files selected for processing (10)
electron/main.tselectron/native/whisper-stt/src/main.cppelectron/stt/index.test.tselectron/stt/index.tselectron/stt/modelManager.test.tselectron/stt/modelManager.tselectron/stt/whisperServer.test.tselectron/stt/whisperServer.tsscripts/build-whisper-stt.shtechnical-documentation/architecture/transcription-and-captions.md
|
All CodeRabbit findings are addressed in a437fdd, including the two outside-diff findings (post-chunk cancellation and bounded readiness probes) and the stale build-comment nitpick. Verification: full Vitest suite 1706 passed / 1 skipped, focused STT tests passed 31/31, app and test TypeScript checks passed, lint passed with only pre-existing warnings, Biome and shell syntax passed. An independent verifier audited all 8 findings with no blocker. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== HEAD and merge base ==='
git status --short
git rev-parse --short HEAD
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
echo "BASE=$base"
echo '=== Changed files ==='
git diff --name-status "$base"...HEAD
echo '=== Recent commits ==='
git log --oneline --decorate -12 "$base"..HEAD
echo '=== TypeScript outlines ==='
ast-grep outline electron/stt/index.ts --items all --view expanded
ast-grep outline electron/stt/whisperServer.ts --items all --view expanded
ast-grep outline electron/main.ts --items all --view expanded
echo '=== Shell script changed-region context ==='
git diff --unified=80 "$base"...HEAD -- scripts/build-whisper-stt.shLength of output: 18345 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Follow-up commit ==='
git show --format=fuller --stat --find-renames a437fdd7
git show --format= --unified=100 a437fdd7 -- \
electron/stt/index.ts \
electron/stt/index.test.ts \
electron/stt/whisperServer.ts \
electron/stt/whisperServer.test.ts \
scripts/build-whisper-stt.sh
echo '=== SttManager lifecycle methods ==='
sed -n '50,370p' electron/stt/index.ts
echo '=== WhisperServerManager lifecycle methods ==='
sed -n '90,385p' electron/stt/whisperServer.ts
echo '=== STT lifecycle tests ==='
sed -n '1,460p' electron/stt/index.test.ts
echo '=== Whisper server lifecycle tests ==='
sed -n '1,520p' electron/stt/whisperServer.test.ts
echo '=== Native startup and backend selection ==='
sed -n '1,260p' electron/native/whisper-stt/src/main.cppLength of output: 120844 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
echo '=== Complete current PR diff ==='
git diff --check "$base"...HEAD
git diff --unified=35 "$base"...HEAD -- \
electron/main.ts \
electron/stt/modelManager.ts \
electron/stt/modelManager.test.ts \
electron/native/whisper-stt/src/main.cpp \
technical-documentation/architecture/transcription-and-captions.md
echo '=== Electron quit handling ==='
sed -n '490,550p' electron/main.ts
rg -n -C 5 '\bshutdownStt\s*\(|\bbefore-quit\b|\bapp\.quit\s*\(' electron --glob '*.ts'
echo '=== Model validation configuration ==='
sed -n '1,300p' electron/stt/modelManager.ts
echo '=== Inference resource cleanup and native initialization ==='
sed -n '360,560p' electron/stt/whisperServer.ts
sed -n '250,520p' electron/native/whisper-stt/src/main.cppLength of output: 82783
I found no new findings. The follow-up covers the post-chunk cancellation check, bounded readiness probes, concurrent helper startup, and the corrected build-script comment. The Electron quit path and pinned model validation are also consistent with the PR objectives.
|
1 participant
What changed
Why
On macOS the helper could exit before becoming ready when Metal failed to allocate a buffer. Electron then waited for a timeout and transcription/captions appeared broken. The existing small model also traded too much transcription quality for size.
Impact
Transcription remains fully local, uses the faster/more accurate large-v3-turbo family, keeps Metal when it works, and transparently falls back to CPU when GPU initialization fails.
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation