fix(http): give POST /api/chat/stream a PrivacyProxy fallback and err… - #819
fix(http): give POST /api/chat/stream a PrivacyProxy fallback and err…#819dale053 wants to merge 4 commits into
Conversation
…or event (GeniePod#818) On any LLM backend failure, handle_chat_stream propagated the error straight up through its Result<()> return: the caller just logged it and returned Ok(()), so the client got HTTP 200 + a "start" event and then the connection silently closed. No {"type":"error",...} event, no {"type":"done",...} event, and (unlike POST /api/chat's process_chat_turn) no PrivacyProxy escalation attempt at all - the function didn't even take a privacy_proxy parameter. Threads privacy_proxy: Option<&PrivacyProxyConfig> into handle_chat_stream and mirrors process_chat_turn's escalation logic: - Context overflow escalates to PrivacyProxy before ever attempting the local model, same as the blocking path. - A producer failure (once nothing has been shown to the client yet) escalates to PrivacyProxy for a LocalDecline trigger; a PrivacyProxy response is stashed into the existing StreamState's `pending` field so the unchanged tool-detection/finalization code downstream treats it exactly like buffered-but-unflushed stream output, correctly turning it into a "replace" or "token" event. - If escalation isn't configured, isn't allowed, or itself fails, the client now gets a {"type":"error","message":...} event before the connection closes, instead of nothing. A disconnect-triggered cancellation (the client closing the connection) is unaffected: `state_r?` already propagates that case immediately, before any of this new logic runs, exactly as before. Two new tests reproduce the original silent-failure bug and prove the escalation path actually delivers a PrivacyProxy response to the client (via a real mock proxy server, not just a passing assertion).
matedev01
left a comment
matedev01
left a comment
There was a problem hiding this comment.
LGTM — POST /api/chat/stream now gets a PrivacyProxy fallback and a real error event on backend failure, instead of a silent HTTP 200 + dead connection. Escalation logic carefully mirrors handle_chat's existing blocking-path behavior, and the PR description traces through why the client-disconnect path is unaffected. Verified: clippy -D warnings clean, server tests (46) incl. 2 new loopback-TCP tests pass, fmt clean.
📝 WalkthroughWalkthroughThe streaming chat endpoint now supports PrivacyProxy escalation for context overflow and local backend failures. It emits NDJSON error events when failures cannot be escalated, with integration tests covering both behaviors. ChangesStreaming chat error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 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: 1
🧹 Nitpick comments (3)
crates/genie-core/src/server.rs (3)
1296-1311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication in the error-event write + return.
Both branches write the same
{"type":"error","message": local_err.to_string()}event andreturn Err(local_err). Consider factoring this into a small local closure/helper to avoid the repeated pattern.🤖 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/server.rs` around lines 1296 - 1311, Factor the duplicated error-event write and Err return in the surrounding server request-handling logic into a small local helper or closure. Reuse it from both branches while preserving the existing local_err message, awaited write_stream_event call, and error propagation behavior.
1132-1193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the overflow-estimation heuristic into a shared helper.
The comment at Line 1134 states this logic "mirrors
process_chat_turn(POST /api/chat)". Duplicating the token-estimate heuristic and theRESPONSE_RESERVE_TOKENS/JETSON_BASELINE_CONTEXT_TOKENScomparison inline here risks the two code paths silently diverging if one is tuned without the other.♻️ Suggested extraction
fn estimate_context_overflow(messages: &[Message]) -> bool { let estimated_tokens: usize = messages.iter().map(|m| m.content.len() / 4).sum(); estimated_tokens + crate::agent_harness::RESPONSE_RESERVE_TOKENS > crate::runtime_boundary::JETSON_BASELINE_CONTEXT_TOKENS as usize }Then call
estimate_context_overflow(&messages)from bothhandle_chatandhandle_chat_stream.🤖 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/server.rs` around lines 1132 - 1193, Extract the duplicated overflow heuristic into a shared estimate_context_overflow helper accepting the messages slice and returning the existing boolean comparison using RESPONSE_RESERVE_TOKENS and JETSON_BASELINE_CONTEXT_TOKENS. Replace the inline calculation in the shown streaming handler and the corresponding logic in process_chat_turn/handle_chat with calls to this helper, preserving the current escalation behavior.
3903-4111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage for the LocalDecline paths; consider adding a ContextOverflow test.
Both new tests exercise backend-failure escalation (
LocalDecline), matching issue#818's two required scenarios well. The new pre-flight overflow-escalation branch (Lines 1132-1193) has no dedicated integration test in this file 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 `@crates/genie-core/src/server.rs` around lines 3903 - 4111, Add a dedicated integration test for the pre-flight context-overflow escalation branch near the existing stream backend-failure tests. Exercise an oversized conversation/request that triggers the ContextOverflow path, configure PrivacyProxy, and assert the proxy response reaches the client; use the existing ChatServer setup and stream response helper while keeping backend-failure coverage 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 `@crates/genie-core/src/server.rs`:
- Around line 1287-1290: In the LocalDecline escalation success branch, update
state.mode together with state.pending after receiving the proxy response,
resetting it to StreamMode::Undecided before returning r. Match the state reset
behavior used by the overflow-escalation branch so finalization can emit
non-tool escalated text incrementally.
---
Nitpick comments:
In `@crates/genie-core/src/server.rs`:
- Around line 1296-1311: Factor the duplicated error-event write and Err return
in the surrounding server request-handling logic into a small local helper or
closure. Reuse it from both branches while preserving the existing local_err
message, awaited write_stream_event call, and error propagation behavior.
- Around line 1132-1193: Extract the duplicated overflow heuristic into a shared
estimate_context_overflow helper accepting the messages slice and returning the
existing boolean comparison using RESPONSE_RESERVE_TOKENS and
JETSON_BASELINE_CONTEXT_TOKENS. Replace the inline calculation in the shown
streaming handler and the corresponding logic in process_chat_turn/handle_chat
with calls to this helper, preserving the current escalation behavior.
- Around line 3903-4111: Add a dedicated integration test for the pre-flight
context-overflow escalation branch near the existing stream backend-failure
tests. Exercise an oversized conversation/request that triggers the
ContextOverflow path, configure PrivacyProxy, and assert the proxy response
reaches the client; use the existing ChatServer setup and stream response helper
while keeping backend-failure coverage unchanged.
🪄 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: 94a9ca2a-1842-41ae-a50c-bf739614751f
📒 Files selected for processing (1)
crates/genie-core/src/server.rs
| Ok(r) => { | ||
| state.pending = r.clone(); | ||
| r | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset state.mode alongside state.pending on LocalDecline escalation success.
Unlike the overflow-escalation branch (Line 1182), this path overwrites state.pending with the proxy response but leaves state.mode untouched. If the interrupted local stream had already buffered JSON-looking content (state.mode == StreamMode::Tool) before failing, and the escalated response doesn't itself parse as a tool call, the finalization code's state.mode == StreamMode::Undecided check (Line 1345) will skip flushing the escalated text as a "token" event — it only reaches the client via the final "done" event, not incrementally.
🐛 Proposed fix
Ok(r) => {
state.pending = r.clone();
+ state.mode = StreamMode::Undecided;
r
}📝 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.
| Ok(r) => { | |
| state.pending = r.clone(); | |
| r | |
| } | |
| Ok(r) => { | |
| state.pending = r.clone(); | |
| state.mode = StreamMode::Undecided; | |
| r | |
| } |
🤖 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/server.rs` around lines 1287 - 1290, In the
LocalDecline escalation success branch, update state.mode together with
state.pending after receiving the proxy response, resetting it to
StreamMode::Undecided before returning r. Match the state reset behavior used by
the overflow-escalation branch so finalization can emit non-tool escalated text
incrementally.
matedev01
left a comment
matedev01
left a comment
There was a problem hiding this comment.
One more from CodeRabbit worth fixing before merge, even though it's Minor: the LocalDecline escalation branch sets state.pending = r.clone() but not state.mode, unlike the overflow-escalation branch just above it which resets both. If the interrupted local stream had already classified as StreamMode::Tool before the LocalDecline trigger fired, and the escalated response isn't itself a tool call, finalization's mode == Undecided check skips the incremental "token"" flush — the text still reaches the client, just only in the final "done"event instead of incrementally. One-line fix mirroring the sibling branch: addstate.mode = StreamMode::Undecided;alongsidestate.pending = r.clone();`.
Everything else here is careful and well-traced — nice work on the disconnect-path analysis.
2 participants
Summary
Gives
POST /api/chat/streamaPrivacyProxyfallback and a real error event on backend failure.Closes #818 — on any LLM backend failure, the client used to get
HTTP 200+ a"start"event, then the connection silently closed with no signal at all.Changes
crates/genie-core/src/server.rs:handle_chat_streamnow takesprivacy_proxy: Option<&PrivacyProxyConfig>, matchinghandle_chat's existing signature.Mirrors
process_chat_turn's escalation logic for the streaming path:PrivacyProxybefore ever attempting the local model, same as the blocking path.PrivacyProxyfor aLocalDeclinetrigger, but only once nothing has been shown to the client yet (!state.emitted_text).Once partial text has streamed, mixing in an unrelated proxy response would be more confusing than a clean error.
PrivacyProxyresponse is stashed into the existingStreamState.pendingfield.The unchanged tool-detection/finalization code downstream treats it exactly like buffered-but-unflushed stream output, so it correctly becomes a
"replace"or"token"event with zero further special-casing.{"type":"error","message":...}event before the connection closes.RequestRoute::ChatStreamdispatch arm) to pass the already-in-scopeprivacy_proxyvariable through.ChatServerover a loopback TCP connection rather than asserting against internal state directly.Why the disconnect path is unaffected
A client-initiated disconnect and a backend failure surface through the same
tokio::select!block, but they're already distinguishable before my change: a disconnect only ever manifests as the consumer'swrite_stream_eventhitting a broken pipe, andstate_r?propagates that immediately, before any of the new escalation logic runs.My changes only ever see
llm_resultwhen the consumer completed cleanly (no disconnect) and the producer either succeeded or hit a genuine backend error.I traced this carefully rather than assuming it, since it's easy to get backwards.
Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux dev machine:
What I observed
stream_backend_failure_without_privacy_proxy_writes_error_event: passes.Drives a real
ChatServerwired to aMockLlmBackendwith an empty reply queue (so the first call errors), opens a raw TCP connection, sendsPOST /api/chat/stream, and asserts the raw response bytes contain a{"type":"error",...}event.This is the same test I used to first empirically reproduce the bug before writing any fix — before my change it failed (no error event present); after, it passes.
stream_backend_failure_escalates_to_privacy_proxy: passes.Same setup, but with a
[privacy_proxy]config pointing at a real mock HTTP server on a second loopback port.Asserts the client receives the proxy's response text and no error event.
While writing this test I hit a real bug in my own mock server's response body (missing the
rolefield the OpenAI-compatible parser requires) — caught by actually running the test and reading the failure, not by inspection.cargo test --workspace --locked --all-targets: every crate green.One unrelated test (
genie-common'stegrastats::tests::mem_available_mb_async_matches_sync_version, which reads live system memory) failed once and passed on rerun in isolation — confirmed viagit diffthatgenie-commonhas zero changes from this PR, so this is a pre-existing flake, not a regression.cargo clippy --workspace -- -D warnings: clean across all crates.--no-default-featuresclippy + test (genie-core, genie-ctl): clean and green, including both new tests.cargo fmt --all -- --check: no diff.Both new tests exercise the real code path over a real TCP socket, not a hand-constructed unit-level shortcut.
Test plan
cargo test -p genie-core --lib stream_backend_failure— both new tests.[services.llm].urlat a dead port), sendPOST /api/chat/stream, confirm the response now includes a{"type":"error",...}event instead of silently closing.[privacy_proxy]pointing at a real PrivacyProxy instance, repeat the same request, confirm the client receives an escalated response instead of an error.Notes for reviewers
!state.emitted_text.If the model has already streamed some real text and then the connection to it drops mid-stream, this PR does not attempt to recover — it's still a silent-ish failure in that narrow case (no error event, since the code path that would write one only exists for the
llm_resultproducer-error branch, not a mid-stream drop after some tokens already flowed).I judged that a client that's already seen partial text has enough context to not be totally confused by a stalled stream, and layering escalation on top of partial output risked worse UX than the status quo.
Flagging in case a maintainer wants that case covered too.
The fix's structure (check
local_err, decide escalate-vs-error before returning) covers those the same way it covers immediate backend failure, but I only empirically verified the immediate-failure case since that's what [bug] POST /api/chat/stream sends "start" then silently closes on any LLM backend failure — no error event, no PrivacyProxy fallback #818 reported.Summary by CodeRabbit
New Features
Bug Fixes