[MohammadHijjawi97]

What

_delta() in the OpenAI-compatible chat provider now raises StreamedError, carrying the endpoint's message, when a stream chunk carries error. Before, it read that chunk as an empty delta. Chat then takes its existing mid-reply failure path: an error frame, with the partial text kept. One line in docs/architecture/chat.md states this.

Why

Once the first token has gone out, the status is already 200, so a later failure arrives inside the stream:

• llama.cpp sends data: {"error": {"code": 500, "message": …, "type": …}} (format_oai_sse in server-context.cpp)
• OpenRouter sends a chunk with a top-level error and finish_reason: "error", and its streaming docs say to treat it as a failure

The provider skipped that chunk, so the stream closed as if the model had finished:

• chat stored the cut-off text as a completed reply, with no error and no Retry
• Studio passed the truncated JSON to parse_json, which reported "model did not return valid JSON"

Repro against a mocked endpoint (partial text, then OpenRouter's documented error chunk):

Before:

stream ended normally; reply = 'Revenue climbed after'

After:

reply failed: StreamedError Provider disconnected unexpectedly | partial = 'Revenue climbed after'

The error reaches the chat as unknown, which offers Retry, because classify_chat_error reads an error's text only from an HTTP error response (a 400's error.type, and since #2179 llama.cpp's 500 message, for runtime_busy), and a StreamedError is not one.

Fixes # (no issue)

How to test

cd surfsense_local/backend
uv run pytest tests/unit/llm/providers/openai_compatible/test_streamed_error.py
uv run pytest -m unit
uv run ruff check .
python scripts/check_docs.py   # from the repo root

The new tests cover llama.cpp's and OpenRouter's error shapes and a bare string error. They fail without the change ("DID NOT RAISE StreamedError"). Locally pytest -m unit passed (3285); the chat streaming integration tests skipped without the real embedding model.

High-level PR Summary

This PR fixes a bug where the OpenAI-compatible chat provider incorrectly treated mid-stream errors from endpoints like llama.cpp and OpenRouter as successful completions. The fix introduces a StreamedError exception that's raised when an error chunk is detected, allowing the chat system to properly handle partial replies with an error state and offer retry options instead of treating truncated responses as complete.

⏱️ Estimated Review Time: 5-15 minutes

💡 Review Order Suggestion

Order	File Path
1	surfsense_local/backend/tests/unit/llm/providers/openai_compatible/test_streamed_error.py
2	surfsense_local/backend/modules/llm/providers/openai_compatible/chat.py
3	docs/architecture/chat.md

Summary by CodeRabbit

• Bug Fixes
  • Errors reported during a model response stream now mark the turn as failed instead of treating it as complete.
  • Chat tests now return an error response with the provider’s message when a stream fails, rather than showing text produced before the error.
• Documentation
  • Clarified how errors received during streaming affect turn status and chat-test results.

[vercel]

@MohammadHijjawi97 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: MODSetter/SurfSense/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: e5a31379-f397-437a-96fa-f692734fd9c5

📥 Commits

Reviewing files that changed from the base of the PR and between ad631c9 and eec650e.

📒 Files selected for processing: 6

• docs/architecture/chat.md
• docs/architecture/connections.md
• surfsense_local/backend/modules/llm/connections/router.py
• surfsense_local/backend/modules/llm/providers/openai_compatible/chat.py
• surfsense_local/backend/tests/integration/llm/conftest.py
• surfsense_local/backend/tests/integration/llm/test_connections.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The OpenAI-compatible provider now raises StreamedError when a stream chunk contains an error. The chat-test route maps that exception to HTTP 502 with an error detail. Unit and integration tests cover streamed errors, and architecture documentation describes the behavior.

Changes: Stream error handling

Layer / File(s)	Summary
Detect and report streamed errors surfsense_local/backend/modules/llm/providers/openai_compatible/chat.py, surfsense_local/backend/tests/unit/llm/providers/openai_compatible/test_streamed_error.py, docs/architecture/chat.md	The provider extracts an error message from dictionary or scalar payloads, trims and caps it, and raises StreamedError for error chunks. Unit tests cover both payload forms and preceding text output. The documentation states that an in-stream error after the first token fails the turn.
Map stream errors to chat-test responses surfsense_local/backend/modules/llm/connections/router.py, surfsense_local/backend/tests/integration/llm/conftest.py, surfsense_local/backend/tests/integration/llm/test_connections.py, docs/architecture/connections.md	The chat-test route returns HTTP 502 with a chat request failed: detail for StreamedError. Integration test support can emit a configured error after content deltas, and the integration test checks the response. The documentation describes the HTTP 502 response.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ChatTest as Chat-test route
  participant Provider as OpenAI-compatible provider
  ChatTest->>Provider: Start chat request
  Provider-->>ChatTest: Content delta, then StreamedError
  ChatTest-->>ChatTest: Return HTTP 502 with error detail

Loading

Merge Risk: ⚪ Minimal · up to eec65

Stream failures are mapped to failed chat responses, and normal chat preserves retryable failure state after partial output. No supported merge-blocking issue remains.

Security architecture risk: 🔵 Low · up to eec65

The change makes reported stream failures explicit while retaining partial replies and existing recovery behavior. No expanded access or privilege was identified. Verification was static, with limited end-to-end coverage of the exact failure path.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — An endpoint controlling a consumed stream can now explicitly fail that generation and supply bounded diagnostic text to its chat-test caller. The inspected path continues to use the stored connection and its existing egress gate; it does not introduce a new destination-selection or privilege-granting path.

Trust Boundaries and Controls

• observed — Endpoint error text does not determine ordinary chat's error classification. StreamedError falls through the existing type-and-status classifier to a generic unknown-error message, whereas connection testing deliberately returns the bounded provider diagnostic in its HTTP error detail.

Resilience and Maintainability Implications

• inferred — The new producer failure reuses existing settlement and retry ownership rules rather than creating a second state transition mechanism. Inspection found no introduced failure-state inconsistency: partial content retains its error marker, finalization is shielded, and retry validates the failed latest turn before transactional replacement.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly describes the main change: fail replies when an endpoint sends an in-stream error that cuts off the response.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[MODSetter]

Reviewed this. The fix is needed and the detection is right:

• llama.cpp (at the app's pinned b11050) sends an error after streaming has started as data: {"error": {...}};
• OpenRouter documents the same top-level error;
• ordinary chunks never carry one;
• "error": null is ignored.

Your three tests fail against dev with "DID NOT RAISE". Two things before it can merge.

1. It conflicts with dev; please rebase

• openai_compatible/chat.py: [Local|Cache] Let the model reuse the start of every prompt #2178 added _reuse() right after _delta(), plus reuse logging in _stream(). The conflict is positional, so keep both functions.
• docs/architecture/chat.md: feat(chat): keep chat and agent replies running in the background #2179 rewrote the section. The bullet you edit ("A turn that ends without any answer text is deleted…") no longer exists, and endings now live under "How a turn ends": the Failed row and "A failed turn is kept, with or without text". Put your sentence there, and don't bring back the deleted bullets.

2. The Model setup chat test answers 500 on an in-stream error

modules/llm/connections/router.py:316-330 catches SignInRequiredError, PlanLimitError, httpx.HTTPError and (ValueError, KeyError), but not the new StreamedError, which is a RuntimeError. With an OpenRouter-style error chunk after "Hel", POST /llm/connections/{id}/chat-test answers:

• on dev: 200 {"reply":"Hel"};
• with this PR: 500 Internal Server Error.

OpenRouter says the error chunk can be the first event, so a failing model in that dialog is the common case.

What to change

• Catch StreamedError there and answer the same 502 chat request failed: ... that httpx errors get.
• Add a route test for it.

Not blocking:

• Local in-stream errors are all reported as "unknown". feat(chat): keep chat and agent replies running in the background #2179 added runtime_busy, but only for an HTTP 500 (chat/errors.py:123-128). At b11050, a cache overflow during decoding goes to every running slot in-stream, and running several replies at once makes that more likely. Worth deciding while you rebase whether classify_chat_error should see these too.
• The description is out of date. It says classify_chat_error never reads message text, but dev now reads llama.cpp's 500 message (chat/errors.py:158-166).

Push the fixes to this branch and it will be reviewed again.

[MODSetter]

Merged into dev, thanks. The chat test now answers an in-stream error with a 502 and the endpoint's message, with a route test for it. The chat.md line sits under "How a turn ends", as asked.

One thing from our earlier note is still open, for a next PR if you'd like it: an in-stream error from the local runtime is classified unknown. runtime_busy is decided only for an HTTP 500 (chat/errors.py:123-128), and at b11050 a cache overflow while decoding reaches every running slot in-stream. A StreamedError falls through to unknown at :137.

[MohammadHijjawi97]

Thanks for the review. Merged dev into the branch. _reuse() and the reuse logging from #2178 are kept alongside the in-stream error check, and the docs sentence now sits under "How a turn ends", next to "A failed turn is kept"; none of the removed bullets are back. The Model setup chat test now catches StreamedError and answers the same 502 "chat request failed: ..." as httpx errors, with a route test where the stream sends "Hel" and then an OpenRouter-style error chunk (it was a 500 before the change). docs/architecture/connections.md notes the 502 too. I also corrected the description: classify_chat_error does read llama.cpp's 500 message now. I left the in-stream runtime_busy classification out of this PR to keep it focused.