Repository navigation
Fix/stream disconnect thread hang - #2207
Closed
gesila1073 wants to merge 2 commits into
Closed
gesila1073 wants to merge 2 commits into
gesila1073 wants to merge 2 commits into
Conversation
|
Contributor
WalkthroughIntroduces a ChangesHTTP Disconnect Cancellation for Streaming Chat
Sequence DiagramsequenceDiagram
participant HTTPClient
participant api_chat as /api/chat handler
participant DisconnectWatcher as _schedule_disconnect_watcher
participant CancelEvent as stream_cancel_event
participant call_stream as ToolCallingLLM.call_stream
participant Formatter as stream_chat_formatter
participant Cleanup as _stream_with_trace_cleanup
api_chat->>CancelEvent: create threading.Event()
api_chat->>DisconnectWatcher: schedule with CancelEvent
api_chat->>Cleanup: wrap SSE stream with cancel_event
loop polling
DisconnectWatcher->>HTTPClient: request.is_disconnected()
end
HTTPClient-->>DisconnectWatcher: disconnected=True
DisconnectWatcher->>CancelEvent: set()
CancelEvent-->>call_stream: cancel_event.is_set() → raise LLMInterruptedError
Formatter->>Formatter: catch LLMInterruptedError → return (no SSE error)
Cleanup->>Cleanup: finally → end OTel span, set CancelEvent
CancelEvent-->>DisconnectWatcher: watcher exits loop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_server_endpoints.py (1)
597-621: 💤 Low valueConsider moving
import threadingto the top of the file.The
import threadingon lines 604 and 632 is repeated inside each test function. Moving it to the file's import section would be more consistent with the coding guidelines.Also applies to: 624-649
🤖 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 `@tests/test_server_endpoints.py` around lines 597 - 621, The import statement for threading is repeated inside multiple test functions including test_stream_with_trace_cleanup_sets_cancel_event_on_normal_completion. Remove the import threading statement from inside all test function bodies and add it once at the top of the file with the other import statements to follow standard Python conventions and avoid code duplication.Source: Coding guidelines
holmes/core/conversations_worker/worker.py (1)
862-863: 💤 Low valueCancel event plumbed but currently unused in worker context.
The
cancel_eventis created and passed tocall_stream, but unlike the server path (which has_schedule_disconnect_watcher), nothing sets this event in the worker context. This is fine — it provides infrastructure for future stop-conversation support — but currently cancellation won't trigger for worker-driven chats.Also applies to: 947-947
🤖 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 `@holmes/core/conversations_worker/worker.py` around lines 862 - 863, The cancel_event created in the worker context is prepared and passed to call_stream but there is no mechanism to actually trigger it, unlike the server path which uses _schedule_disconnect_watcher. To address this, add a clarifying comment near where cancel_event is instantiated documenting that while the event provides infrastructure for future stop-conversation support, it is currently intentionally not being set in the worker context and should be implemented when that feature is added in the future.
🤖 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.
Nitpick comments:
In `@holmes/core/conversations_worker/worker.py`:
- Around line 862-863: The cancel_event created in the worker context is
prepared and passed to call_stream but there is no mechanism to actually trigger
it, unlike the server path which uses _schedule_disconnect_watcher. To address
this, add a clarifying comment near where cancel_event is instantiated
documenting that while the event provides infrastructure for future
stop-conversation support, it is currently intentionally not being set in the
worker context and should be implemented when that feature is added in the
future.
In `@tests/test_server_endpoints.py`:
- Around line 597-621: The import statement for threading is repeated inside
multiple test functions including
test_stream_with_trace_cleanup_sets_cancel_event_on_normal_completion. Remove
the import threading statement from inside all test function bodies and add it
once at the top of the file with the other import statements to follow standard
Python conventions and avoid code duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c088394d-07c9-4ef4-bb5b-17d0157c299d
📒 Files selected for processing (7)
holmes/core/conversations_worker/worker.pyholmes/core/exceptions.pyholmes/core/models.pyholmes/core/tool_calling_llm.pyholmes/utils/stream.pyserver.pytests/test_server_endpoints.py
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
2 participants
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 by CodeRabbit
Release Notes