Skip to content

fix(desktop-backend): give a stalled chat stream a terminal state#9720

Merged
kodjima33 merged 1 commit into
mainfrom
watchdog/chat-stream-terminal-state
Jul 15, 2026
Merged

fix(desktop-backend): give a stalled chat stream a terminal state#9720
kodjima33 merged 1 commit into
mainfrom
watchdog/chat-stream-terminal-state

Conversation

@kodjima33

@kodjima33 kodjima33 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

The bug

A desktop chat turn whose upstream stalls or dies mid-answer never terminates for the client. Two gaps in handle_streaming (Backend-Rust/src/routes/chat_completions.rs):

  1. No idle bound. The reqwest client sets connect_timeout only, and the total-response timeout is deliberately applied to non-streaming calls alone (a long answer is not a stuck one) — so nothing bounded byte_stream.next().await. An upstream that accepts the request and then stops emitting bytes (Anthropic overload/stall, a half-open connection behind a LB) parks the turn until Cloud Run's ~300s request timeout kills the socket.
  2. No terminal event. On a transport error, a stall, or an upstream EOF before message_stop, the read loop just breaks. The SSE body then ends with no finish_reason, no error chunk, and no data: [DONE] — but [DONE] is only ever yielded from the MessageStop and Error arms. The OpenAI SSE contract this endpoint speaks ends at finish_reason/[DONE], so the client's assistant bubble spins with no error to show.

User-visible: send a message, get 200 text/event-stream, watch a spinner for up to five minutes, get nothing — no answer, no error.

Root cause

The turn had no terminal state on the failure paths. Only the two happy-ish paths (message_stop, Anthropic error event) closed the stream; every other way a stream can end was an implicit, silent one. The Gemini proxy already bounds its handler correctly (GEMINI_TOTAL_TIMEOUT / within_gemini_deadline in proxy.rs) — chat had no equivalent.

The fix

  • STREAM_IDLE_TIMEOUT (60s) wraps byte_stream.next(). Anthropic emits ping events every few seconds throughout a generation, so a 60s gap with zero bytes means the stream is dead, not slow. This bounds only the idle gap — a long answer that keeps streaming is untouched.
  • stream_termination_chunks(sent_done, sent_finish) terminates the body on every exit path. It emits the error chunk only when no finish_reason was sent, so a turn whose answer already completed is never contradicted after the fact; and nothing at all when [DONE] already went out.

Per the fallback-telemetry decision table this is a hard failure (no continue), so it takes the tracing::error! path, not record_fallback.

Verification

  • cargo test363 passed, 0 failed (3 new).
  • cargo clippy -- -D warnings clean (the exact CI command), cargo fmt --check clean.
  • New regression tests cover the terminal decision: dies before message_stop → error + [DONE]; answer already finished → [DONE] only, no error; already sent [DONE] → nothing.

Not exercised against a live stalled Anthropic upstream — reproducing that needs a stalling upstream the const ANTHROPIC_API_URL can't be pointed at. The terminal-chunk decision is behaviorally tested through its seam; the idle-timeout wiring itself is verified by inspection and compile only. Flagging that explicitly rather than implying end-to-end proof.

🤖 automated by hourly watchdog; opened for review, not merged.

Review in cubic

handle_streaming had no bound on the gap between upstream bytes and no
terminal event when the stream died mid-flight:

- The client is built with connect_timeout only, and the total-response
  timeout is deliberately applied to non-streaming calls alone (a long
  answer is not a stuck one), so nothing bounded `byte_stream.next()`.
  An upstream that accepted the request and then stopped emitting bytes
  parked the turn until Cloud Run's ~300s request timeout killed it.
- On a transport error, a stall, or an EOF before `message_stop`, the
  read loop just broke: the SSE body ended with no finish_reason, no
  error chunk, and no `data: [DONE]`. The desktop client's contract ends
  at finish_reason/[DONE], so the assistant bubble spun with no error.

Bound the idle gap (STREAM_IDLE_TIMEOUT, 60s — Anthropic pings every few
seconds throughout a generation, so a gap that long means dead, not slow)
and terminate the body ourselves in stream_termination_chunks(). The error
chunk is emitted only when no finish_reason was sent, so a turn whose
answer already completed is never contradicted after the fact.

The Gemini proxy already bounds its handler this way (within_gemini_deadline);
chat had no equivalent.

Verification: cargo test (363 passed), cargo clippy -- -D warnings clean,
cargo fmt --check clean. The three new tests cover the terminal decision
(dies early -> error + [DONE]; already finished -> [DONE] only; already
sent [DONE] -> nothing). NOT exercised against a live stalled Anthropic
upstream — the idle-timeout wiring itself is verified by inspection only.

@tianmind-studio tianmind-studio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed a236ca10 and found no blocking issue. The state model is coherent: the idle timeout is reset by any upstream bytes (including Anthropic ping events), failure/EOF paths get an error plus [DONE], a prior finish_reason gets only [DONE], and existing terminal events are not duplicated.

Windows validation with the repository-pinned Rust 1.88.0 MSVC toolchain:

  • The three new stream_termination_chunks tests passed.
  • cargo fmt --check passed.
  • cargo clippy -- -D warnings passed.
  • Full cargo test reached 359/363; the four failures are existing routes::proxy local-socket deadline tests. I reran that group on the PR base 298290d4b and got the identical four failures (83/87 passed), so they are not introduced here.

I did not exercise a live stalled Anthropic connection; the remaining risk is the integration seam already called out in the PR description. The timeout wiring itself is straightforward and compile-checked, and the terminal decision is covered behaviorally.

@kodjima33
kodjima33 merged commit 2ef07f7 into main Jul 15, 2026
22 of 25 checks passed
@kodjima33
kodjima33 deleted the watchdog/chat-stream-terminal-state branch July 15, 2026 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants