Skip to content

chore: add try-catch to db inserts so that they don't break the stream#89

Open
akiva10b wants to merge 1 commit into
mainfrom
sc-42805
Open

chore: add try-catch to db inserts so that they don't break the stream#89
akiva10b wants to merge 1 commit into
mainfrom
sc-42805

Conversation

@akiva10b

Copy link
Copy Markdown
Contributor

This pull request improves the robustness of the chat streaming API by ensuring that failures during post-agent operations—such as updating the conversation summary or persisting the turn—do not prevent the successful delivery of the final message event to the client. It introduces best-effort error handling and logging for these scenarios and adds comprehensive tests to verify this behavior.

Error Handling and Robustness Improvements:

  • Introduced the _finalize_stream_success helper in server/chat/V2/views.py to encapsulate summary updating and turn persistence after agent response, ensuring that exceptions in these steps are logged and do not interrupt message delivery. Fallback values are used if errors occur. [1] [2]
  • Added the _capture_stream_success_exception function to log and capture exceptions during post-agent operations, improving observability without affecting the client experience.
  • Updated the finalize_success method in server/chat/V2/logging/turn_logging_service.py to use a database transaction for atomicity and adjusted the placement of statistics calculation for consistency. [1] [2] [3] [4]

Testing Enhancements:

  • Added two integration tests in server/chat/tests/test_streaming_integration.py to verify that failures in summary updating and turn persistence do not prevent the final SSE message from being sent, and that such exceptions are properly captured and logged.

Code Structure and Typing:

  • Introduced the StreamSuccessResult dataclass in server/chat/V2/views.py to standardize the metadata returned for the final SSE message event, improving code clarity and maintainability.

These changes collectively make the chat streaming endpoint more resilient to backend errors, ensuring a better user experience and more reliable error reporting.

@coolify-sefaria-github

coolify-sefaria-github Bot commented Mar 17, 2026

Copy link
Copy Markdown

The preview deployment for sefaria/ai-chatbot:server is ready. 🟢

Open app | Open Build Logs | Open Application Logs

Last updated at: 2026-03-17 20:17:29 CET

Copilot AI 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.

Pull request overview

This PR improves resiliency of the V2 chat streaming endpoint by ensuring that failures in post-agent “success path” operations (summary update and DB persistence/logging) are handled best-effort and do not prevent the final event: message SSE from reaching the client.

Changes:

  • Introduces _finalize_stream_success + _capture_stream_success_exception in server/chat/V2/views.py to isolate and harden post-agent work behind try/except with logging + Sentry capture.
  • Wraps TurnLoggingService.finalize_success in a DB transaction for atomicity and moves stats computation to a consistent spot.
  • Adds integration tests proving that summary/persistence failures do not produce an SSE error event and still deliver the final message event.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
server/chat/V2/views.py Adds best-effort finalization helper to prevent post-agent failures from breaking SSE message delivery; captures/logs exceptions with context.
server/chat/V2/logging/turn_logging_service.py Makes assistant-message persistence + session updates atomic via transaction.atomic() and adjusts stats computation placement.
server/chat/tests/test_streaming_integration.py Adds integration tests for summary-update and persistence failures ensuring final SSE message still arrives and exceptions are captured.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/chat/V2/views.py
return braintrust.TracedThreadPoolExecutor(max_workers=1)


def _capture_stream_success_exception(

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.

I think this might not be best practice in python, since you're creating a specific function to capture a single case of exceptions.
Please implement using a context manager, so it's cleaner and resuable.

Comment thread server/chat/V2/views.py
)


def _finalize_stream_success(

@yitzhakc yitzhakc Mar 19, 2026

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.

This is a lot of logic that is really unrelated to the View at all. Let's try and separate the concerns of responding to the user, and managing the various database updates.
I understand that it might be overkill at this point to extract this logic to an external queue/broker (although maybe we should make a story for it) but at the very least let's take it out of views.py.

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.

3 participants