fix(stt): bound whisper-server HTTP reads and fail closed on errors - #939
Open
kurosawareiji7007-hub wants to merge 3 commits into
Open
Conversation
The STT server path still used unbounded read_line, had no connect/request deadline or status check, and fail-opened non-JSON bodies into fake transcripts — the same class of bug fixed for LLM/HA in GeniePod#655/GeniePod#656.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe whisper-server STT path now uses bounded HTTP reads, connection and request deadlines, mandatory 2xx responses, and fail-closed JSON decoding. Tests cover valid, invalid, error, and oversized responses. ChangesWhisper-server STT reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SttEngine
participant WhisperServer
participant HttpResponseParser
SttEngine->>WhisperServer: Send STT request with deadlines
WhisperServer-->>HttpResponseParser: Return bounded HTTP response
HttpResponseParser-->>SttEngine: Validate status and decode JSON transcript
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/genie-core/src/voice/stt.rs (1)
360-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the address to the connect error.
The connect failure branch returns the raw
io::Errorwithout the target address. The timeout branch also omits it. The nearby client incrates/genie-core/src/llm/openai_compat.rsincludes the address in both branches, which makes port misconfiguration easier to diagnose.♻️ Proposed change
- Ok(Ok(stream)) => stream, - Ok(Err(e)) => return Err(e.into()), - Err(_) => anyhow::bail!( - "whisper-server connect timed out after {}s", - WHISPER_SERVER_CONNECT_TIMEOUT.as_secs() - ), + Ok(Ok(stream)) => stream, + Ok(Err(e)) => anyhow::bail!("whisper-server connect failed ({addr}): {e}"), + Err(_) => anyhow::bail!( + "whisper-server connect timed out after {}s ({addr})", + WHISPER_SERVER_CONNECT_TIMEOUT.as_secs() + ),🤖 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 `@crates/genie-core/src/voice/stt.rs` around lines 360 - 366, Update the connection error handling around the stream connection match to include the target address in both the Err(e) failure and timeout branches, matching the diagnostic format used by the nearby OpenAI-compatible client while preserving the existing error propagation and timeout duration.
🤖 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 `@crates/genie-core/src/voice/stt.rs`:
- Around line 613-618: Update the language resolution chain in the surrounding
STT response parsing to reject server values that are blank or equal to "auto"
before applying fallbacks. Preserve valid parsed language values, then fall back
to language_hint and detected_language so unusable server tags do not win.
- Around line 586-596: Update the non-2xx response handling around the status
check to truncate the trimmed body excerpt before interpolating it into the
anyhow error, capping it to a reasonable bounded length rather than the full
WHISPER_SERVER_MAX_BODY_BYTES payload. Preserve the existing whitespace
normalization, empty-body formatting, and HTTP status in the error message.
- Around line 1228-1235: Update the test’s reader construction to use
std::io::Cursor around the existing Vec<u8> response data instead of
tokio::io::Cursor, preserving the BufReader setup and overflow payload. Ensure
the assertion uses HttpReadError::HeaderLineTooLong.
---
Nitpick comments:
In `@crates/genie-core/src/voice/stt.rs`:
- Around line 360-366: Update the connection error handling around the stream
connection match to include the target address in both the Err(e) failure and
timeout branches, matching the diagnostic format used by the nearby
OpenAI-compatible client while preserving the existing error propagation and
timeout duration.
🪄 Autofix (Beta)
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: a7eabdab-13e2-42be-97e2-b423e879191c
📒 Files selected for processing (2)
CHANGELOG.mdcrates/genie-core/src/voice/stt.rs
Comment on lines
+586
to
+596
| if !(200..300).contains(&status) { | ||
| let trimmed = body.trim().replace('\n', " "); | ||
| anyhow::bail!( | ||
| "whisper-server HTTP {status}{}", | ||
| if trimmed.is_empty() { | ||
| String::new() | ||
| } else { | ||
| format!(": {trimmed}") | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Truncate the body before it enters the error message.
The body can be up to WHISPER_SERVER_MAX_BODY_BYTES (256 KiB). A non-2xx response therefore produces an error string of the same size, which is then logged upstream. Cap the excerpt.
🐛 Proposed fix
if !(200..300).contains(&status) {
- let trimmed = body.trim().replace('\n', " ");
+ let mut trimmed = body.trim().replace('\n', " ");
+ const MAX_ERR_EXCERPT: usize = 256;
+ if trimmed.len() > MAX_ERR_EXCERPT {
+ let cut = trimmed
+ .char_indices()
+ .take_while(|(i, _)| *i <= MAX_ERR_EXCERPT)
+ .last()
+ .map(|(i, _)| i)
+ .unwrap_or(0);
+ trimmed.truncate(cut);
+ trimmed.push('…');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !(200..300).contains(&status) { | |
| let trimmed = body.trim().replace('\n', " "); | |
| anyhow::bail!( | |
| "whisper-server HTTP {status}{}", | |
| if trimmed.is_empty() { | |
| String::new() | |
| } else { | |
| format!(": {trimmed}") | |
| } | |
| ); | |
| } | |
| if !(200..300).contains(&status) { | |
| let mut trimmed = body.trim().replace('\n', " "); | |
| const MAX_ERR_EXCERPT: usize = 256; | |
| if trimmed.len() > MAX_ERR_EXCERPT { | |
| let cut = trimmed | |
| .char_indices() | |
| .take_while(|(i, _)| *i <= MAX_ERR_EXCERPT) | |
| .last() | |
| .map(|(i, _)| i) | |
| .unwrap_or(0); | |
| trimmed.truncate(cut); | |
| trimmed.push('…'); | |
| } | |
| anyhow::bail!( | |
| "whisper-server HTTP {status}{}", | |
| if trimmed.is_empty() { | |
| String::new() | |
| } else { | |
| format!(": {trimmed}") | |
| } | |
| ); | |
| } |
🤖 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 `@crates/genie-core/src/voice/stt.rs` around lines 586 - 596, Update the
non-2xx response handling around the status check to truncate the trimmed body
excerpt before interpolating it into the anyhow error, capping it to a
reasonable bounded length rather than the full WHISPER_SERVER_MAX_BODY_BYTES
payload. Preserve the existing whitespace normalization, empty-body formatting,
and HTTP status in the error message.
Comment on lines
+613
to
+618
| language: parsed | ||
| .get("language") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from) | ||
| .or_else(|| language_hint.map(str::to_string)) | ||
| .or(detected_language), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject blank or auto language values from the server.
parsed["language"].as_str() returns Some("") when whisper-server sends an empty language field, and Some("auto") when detection is disabled. Both values then win over language_hint and over detect_language_from_text, so the transcript carries a useless language tag. Filter the value before the fallback chain.
🐛 Proposed fix
language: parsed
.get("language")
.and_then(|v| v.as_str())
+ .map(str::trim)
+ .filter(|v| !v.is_empty() && !v.eq_ignore_ascii_case("auto"))
.map(String::from)
.or_else(|| language_hint.map(str::to_string))
.or(detected_language),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| language: parsed | |
| .get("language") | |
| .and_then(|v| v.as_str()) | |
| .map(String::from) | |
| .or_else(|| language_hint.map(str::to_string)) | |
| .or(detected_language), | |
| language: parsed | |
| .get("language") | |
| .and_then(|v| v.as_str()) | |
| .map(str::trim) | |
| .filter(|v| !v.is_empty() && !v.eq_ignore_ascii_case("auto")) | |
| .map(String::from) | |
| .or_else(|| language_hint.map(str::to_string)) | |
| .or(detected_language), |
🤖 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 `@crates/genie-core/src/voice/stt.rs` around lines 613 - 618, Update the
language resolution chain in the surrounding STT response parsing to reject
server values that are blank or equal to "auto" before applying fallbacks.
Preserve valid parsed language values, then fall back to language_hint and
detected_language so unusable server tags do not win.
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
1 participant
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.
Summary
transcribe_via_serverstill used a hand-rolledread_lineloop with no line/body cap, no connect/request deadlines, no HTTP status check, and fail-opened non-JSON bodies into a spoken transcript. That is the same outbound-HTTP class already closed for LLM/HA clients (#655/#656).Problem
A hung, slow, or broken whisper-server could OOM the voice loop on unbounded header/body growth, hang forever on connect/read, or have HTML/plain error pages spoken as user speech.
Root cause
The whisper-server path never moved onto the shared
read_response/HttpResponseLimitsreader, and treated any parse failure as transcript text.Solution
read_responsewith a 256 KiB body captextfield); fail closed otherwiseScope boundaries
transcribe_via_serverHTTP path incrates/genie-core/src/voice/stt.rsTests
whisper_server_http_accepts_json_transcriptwhisper_server_http_rejects_non_2xxwhisper_server_http_rejects_non_json_bodywhisper_server_read_rejects_oversized_header_lineBefore/after results
HeaderLineTooLong; connect/request deadlines applyFull
cargo test -p genie-coreruns in Linux CI (this Windows host cannot link fullgenie-coredue to Unix-gated APIs).Regression protection
Unit tests fail closed on the previous fail-open and unbounded-read behaviors.
Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
mainincrates/genie-core/src/voice/stt.rs+CHANGELOG.mdtranscript_from_whisper_http_responseand sharedread_responseoversize-header rejectioncargo testcannot link on this Windows host (UnixStream/ Unix libc gates); Linux CI runs the real lib testsWhat I observed
{text:...}parses to a transcriptHeaderLineTooLongunderHttpResponseLimitsJetson gap: no Jetson/whisper-server instance on this host. Behavior is deterministic HTTP client hardening; CI Linux unit tests cover the fail-closed paths. Hardware validation would be a live
/inferenceround-trip against whisper-server on device.Fixes
Fixes #936
Summary by CodeRabbit