fix: attach top-level session_id to chatbot websocket error sends - #596
kishore280 wants to merge 3 commits into
Conversation
The frontend filters every incoming websocket message by a top-level session_id before any type-based handling runs (client/src/hooks/ useMessageHandler.ts). Thirteen error sends in main_chatbot.py only put session_id inside the nested data object, so once a session is active those messages get silently dropped by the filter and never reach the error handler that clears the "waiting for reply" state. This is exactly what Arvo-AI#563 reports: input over the 20k token limit is correctly rejected server-side with a clear error message, but the message never arrives client-side, so the chat UI hangs indefinitely. Add session_id at the top level, matching every other send site in the file that already does this correctly (e.g. the streaming message path and the HOOK_BLOCKED error). Closes Arvo-AI#563
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughWebSocket processing now tracks ChangesWebSocket error context
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![arvo-ai-staging[bot]](https://avatars.githubusercontent.com/in/4088260?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/main_chatbot.py`:
- Around line 1474-1475: Update the exception handler around the token-counting
logic to use logger.exception("Error counting input tokens") instead of
formatting the exception into logger.error, preserving traceback context; retain
the broad catch only if the existing safety fallback depends on it.
- Line 1640: Reset session_id at the start of each message-loop iteration,
assign it from data.get("session_id") immediately after successful JSON parsing,
and remove the later duplicate assignment. Ensure the outer error handler cannot
reuse a previous iteration’s session identifier when parsing or earlier
processing fails.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d482032f-2b16-4ef6-8a6e-a58dc96d73ca
📒 Files selected for processing (1)
server/main_chatbot.py
session_id was only reassigned deep inside the message loop, so a failure before that point (e.g. invalid JSON) sent the outer error handler the previous message's session_id instead of the current one. Extract it right after parsing instead, and reset it at the top of each iteration so a pre-parse failure has no session_id to misattribute. Also switch the token-counting error log to logger.exception() so the traceback isn't dropped, and drop the now-unused exception variable.
1d135ba to
f056320
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/main_chatbot.py (1)
988-1001: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExtract
session_idbefore the rate-limit rejection.The rate-limit branch at Lines 990-997 sends an error before JSON parsing and Line 1001. That response has no top-level
session_id. The frontend can discard it and keep the message pending. Parse the message and capturesession_idbeforerate_limiter.is_allowed(...), while keeping the reset at Line 988 for malformed or failed messages.Proposed fix
async for message in websocket: session_id = None + data = json.loads(message) + session_id = data.get("session_id") # Rate limit check if not rate_limiter.is_allowed(client_id): ... continue logger.debug(f"Received message from client {client_id}: {message}") - data = json.loads(message) - session_id = data.get('session_id')🤖 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 `@server/main_chatbot.py` around lines 988 - 1001, Parse the incoming message and capture session_id before calling rate_limiter.is_allowed in the websocket message flow. Include that session_id in the rate-limit error response so the frontend can match and discard it, while retaining the existing session_id reset for malformed or failed messages.
🤖 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.
Outside diff comments:
In `@server/main_chatbot.py`:
- Around line 988-1001: Parse the incoming message and capture session_id before
calling rate_limiter.is_allowed in the websocket message flow. Include that
session_id in the rate-limit error response so the frontend can match and
discard it, while retaining the existing session_id reset for malformed or
failed messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eab64699-1e7f-4774-9cd1-0f7617375206
📒 Files selected for processing (1)
server/main_chatbot.py
|
This branch has not been deployed
1 participant
Closes #563. Chat input over 20k tokens gets correctly rejected server-side (added in f40eca5, months before this issue), but the UI hangs waiting for a reply that never visibly arrives.
Root cause: the frontend filters every incoming websocket message by a top-level
session_idbefore any type dispatch (useMessageHandler.ts:49-61). 13websocket.send()calls inmain_chatbot.pyonly setsession_idinside the nesteddataobject, so those messages get silently dropped once a session is active, including the 20k-token error itself.Fix: add
session_idat the top level on all 13 sites, matching the pattern already used correctly elsewhere in the file (e.g. the streaming path, theHOOK_BLOCKEDerror).Summary by CodeRabbit