Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions backend/director/core/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,28 +220,31 @@ class OutputMessage(BaseMessage):
status: MsgStatus = MsgStatus.progress

def update_status(self, status: MsgStatus):
"""Update the status of the message and publish the message to the socket. for loading state."""
"""Update the status and broadcast to session room."""
self.status = status
self._publish()

def push_update(self):
"""Publish the message to the socket."""
"""Push real-time update to session room."""
try:
self._publish()
except Exception as e:
print(f"Error in emitting message: {str(e)}")
print(f"Error in emitting update to session {self.session_id}: {str(e)}")

Comment on lines +235 to 240
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick

Don’t swallow exceptions; replace print with proper logging and explicit conversion flag

Use logger.exception (or at least logger.warning with exc_info). Also prefer {e!s} over str(e) to satisfy RUF010. Avoid broad Exception if possible.

Apply this diff:

-    def push_update(self):
-        """Push real-time update to session room."""
-        try:
-            self._publish()
-        except Exception as e:
-            print(f"Error in emitting update to session {self.session_id}: {str(e)}")
+    def push_update(self):
+        """Push real-time update to session room."""
+        try:
+            self._publish()
+        except RuntimeError as e:
+            logger.warning("Emit failed outside request context for session %s: %s", self.session_id, e, exc_info=True)

Add the following once at the top of the file to support logging (outside the changed hunk):

import logging
logger = logging.getLogger(__name__)
🧰 Tools
🪛 Ruff (0.13.1)

231-231: Do not catch blind exception: Exception

(BLE001)


232-232: Use explicit conversion flag

Replace with conversion flag

(RUF010)

🤖 Prompt for AI Agents
In backend/director/core/session.py around lines 228 to 233, the except block
currently prints errors and swallows exceptions; add a module-level logger by
importing logging and creating logger = logging.getLogger(__name__) at the top
of the file, then replace the print with a logger.exception call (or
logger.warning(..., exc_info=True)) and format the message using {e!s} instead
of str(e) to satisfy RUF010; also avoid catching blanket Exception if you
can—catch the specific exceptions that _publish may raise (or re-raise after
logging) so errors aren’t silently swallowed.

def publish(self):
"""Store the message in the database. for conversation history and publish the message to the socket."""
"""Store in database and broadcast final result to session room."""
self._publish()

def _publish(self):
try:
emit("chat", self.model_dump(), namespace="/chat")
emit("chat", self.model_dump(),
room=self.session_id,
namespace="/chat")
print(f"Emitted message to session room: {self.session_id}")
except Exception as e:
print(f"Error in emitting message: {str(e)}")
self.db.add_or_update_msg_to_conv(**self.model_dump())
print(f"Error emitting to session {self.session_id}: {str(e)}")

self.db.add_or_update_msg_to_conv(**self.model_dump())
Comment on lines +247 to +254
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick

Room-scoped emit is correct; switch prints to logging and avoid blind except

Good: emit(..., room=self.session_id, namespace="/chat"). Replace print with logger and avoid catching broad Exception. Also use explicit f-string conversion flags per RUF010.

Apply this diff:

-        try:
-            emit("chat", self.model_dump(), 
-                 room=self.session_id, 
-                 namespace="/chat")
-            print(f"Emitted message to session room: {self.session_id}")
-        except Exception as e:
-            print(f"Error emitting to session {self.session_id}: {str(e)}")
+        try:
+            emit(
+                "chat",
+                self.model_dump(),
+                room=self.session_id,
+                namespace="/chat",
+            )
+            logger.info("Emitted message to session room: %s", self.session_id)
+        except RuntimeError as e:
+            logger.warning(
+                "Error emitting to session %s: %s",
+                self.session_id,
+                e,
+                exc_info=True,
+            )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
emit("chat", self.model_dump(),
room=self.session_id,
namespace="/chat")
print(f"Emitted message to session room: {self.session_id}")
except Exception as e:
print(f"Error in emitting message: {str(e)}")
self.db.add_or_update_msg_to_conv(**self.model_dump())
print(f"Error emitting to session {self.session_id}: {str(e)}")
self.db.add_or_update_msg_to_conv(**self.model_dump())
try:
emit(
"chat",
self.model_dump(),
room=self.session_id,
namespace="/chat",
)
logger.info("Emitted message to session room: %s", self.session_id)
except RuntimeError as e:
logger.warning(
"Error emitting to session %s: %s",
self.session_id,
e,
exc_info=True,
)
self.db.add_or_update_msg_to_conv(**self.model_dump())
🧰 Tools
🪛 Ruff (0.13.1)

244-244: Do not catch blind exception: Exception

(BLE001)


245-245: Use explicit conversion flag

Replace with conversion flag

(RUF010)

🤖 Prompt for AI Agents
In backend/director/core/session.py around lines 240-247, replace the two
print() calls with the module logger (e.g., logger.info or logger.debug for the
success message, and logger.error or logger.exception for failures), and stop
using a blind "except Exception"; instead catch the specific
socketio/emit-related exceptions you expect (or at minimum re-raise after
logging). When logging the exception include the exception object with an
explicit conversion flag (e.g., {e!r}) or use logger.exception(...,
exc_info=True) to record the traceback, and ensure
db.add_or_update_msg_to_conv(**self.model_dump()) still runs in the appropriate
place (either after successful emit or in a finally if it must always run).


def format_user_message(message: dict) -> dict:
message_content = message.get("content")
Expand Down Expand Up @@ -393,6 +396,10 @@ def delete(self):
"""Delete the session from the database."""
return self.db.delete_session(self.session_id)

def update(self, **kwargs) -> bool:
"""Update the session in the database."""
return self.db.update_session(self.session_id, **kwargs)

def emit_event(self, event: BaseEvent, namespace="/chat"):
"""Emits a structured WebSocket event to notify all clients about updates."""

Expand Down
34 changes: 28 additions & 6 deletions backend/director/entrypoint/api/socket_io.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,40 @@
import os

from flask import current_app as app
from flask_socketio import Namespace

from flask_socketio import Namespace, join_room, emit
from director.db import load_db
from director.handler import ChatHandler


class ChatNamespace(Namespace):
"""Chat namespace for socket.io"""

"""Chat namespace for socket.io with session-based rooms"""

def on_connect(self):
"""Handle client connection"""
print(f"Client connected to chat namespace")

def on_chat(self, message):
"""Handle chat messages"""
"""Handle chat messages and auto-join session room"""
session_id = message.get('session_id')
if not session_id:
emit('error', {'message': 'session_id is required'})
return

join_room(session_id)
print(f"Client joined session room {session_id}")

Comment on lines +16 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

🧩 Analysis chain

Auto-join is correct; prefer logging over print and consider event name for errors

  • Good: requiring session_id and joining the room.
  • Swap print for app.logger.
  • Verify that emitting 'error' as an event name matches client handlers and isn’t conflicting with any reserved handling; consider a namespaced event like 'chat_error' if not already standardized.

Apply this diff for logging:

-        if not session_id:
-            emit('error', {'message': 'session_id is required'})
+        if not session_id:
+            emit('error', {'message': 'session_id is required'})
             return
             
         join_room(session_id)
-        print(f"Client joined session room {session_id}")
+        app.logger.info("Client joined session room %s", session_id)

Also confirm client expectations:


🌐 Web query:

Is 'error' a reserved or special event name in Socket.IO client handlers that could interfere with custom error messaging? Should we prefer a custom event name like 'chat_error'?

💡 Result:

Short answer: Yes — "error" is a built‑in Socket.IO event (fired on connection/transport errors) and is treated as a special/reserved event name, so using it for your own application-level error messages can conflict or be confusing. Use a custom name such as "chat_error" (or any app‑specific name) for application error payloads. [1][2][3]

Sources:

  • Official Socket.IO docs — client/server event list (shows "error" as an event). [1][2]
  • Community guidance (StackOverflow) recommending app‑specific event names instead of reusing "error". [3]

Replace print with logger and stop emitting reserved 'error' event — use an app-specific event (e.g. 'chat_error')

  • In backend/director/entrypoint/api/socket_io.py replace print(...) with app.logger.info("Client joined session room %s", session_id).
  • Do not emit('error', ...); "error" is a built-in Socket.IO event. Emit a custom event (e.g. emit('chat_error', {'message': 'session_id is required'})) and update client handlers to listen for that name.
  • Verify no other server/client code expects application-level messages on the generic "error" event and update those callers/handlers accordingly.
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/socket_io.py around lines 16 to 24, replace
the print statement with app.logger.info("Client joined session room %s",
session_id) and stop emitting the Socket.IO reserved "error" event; instead emit
a custom application event such as emit('chat_error', {'message': 'session_id is
required'}) when session_id is missing. Update any client/server handlers that
expect application-level errors on "error" to listen for the new event name, and
scan the codebase to ensure no other components rely on application messages
using the generic "error" event before deploying.

chat_handler = ChatHandler(
db=load_db(os.getenv("SERVER_DB_TYPE", app.config["DB_TYPE"]))
)
chat_handler.chat(message)

def on_join_session(self, data):
"""Explicitly join a session room (for reconnection)"""
session_id = data.get('session_id')
if session_id:
join_room(session_id)
emit('session_joined', {'session_id': session_id})
print(f"Client explicitly joined session room {session_id}")

Comment on lines +30 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick

Add else-branch for missing session_id and replace print with logging

If session_id is missing, emit an error back to the client. Use app.logger instead of print.

Apply this diff:

     def on_join_session(self, data):
         """Explicitly join a session room (for reconnection)"""
         session_id = data.get('session_id')
-        if session_id:
-            join_room(session_id)
-            emit('session_joined', {'session_id': session_id})
-            print(f"Client explicitly joined session room {session_id}")
+        if session_id:
+            join_room(session_id)
+            emit('session_joined', {'session_id': session_id})
+            app.logger.info("Client explicitly joined session room %s", session_id)
+        else:
+            emit('error', {'message': 'session_id is required to join a session'})
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def on_join_session(self, data):
"""Explicitly join a session room (for reconnection)"""
session_id = data.get('session_id')
if session_id:
join_room(session_id)
emit('session_joined', {'session_id': session_id})
print(f"Client explicitly joined session room {session_id}")
def on_join_session(self, data):
"""Explicitly join a session room (for reconnection)"""
session_id = data.get('session_id')
if session_id:
join_room(session_id)
emit('session_joined', {'session_id': session_id})
app.logger.info("Client explicitly joined session room %s", session_id)
else:
emit('error', {'message': 'session_id is required to join a session'})
🤖 Prompt for AI Agents
In backend/director/entrypoint/api/socket_io.py around lines 30 to 37, the
on_join_session handler lacks an else-branch to handle a missing session_id and
uses print for logging; modify the function to emit an error event back to the
client (e.g., emit('error', {'message': 'session_id required'})) when session_id
is falsy and replace the print statement with app.logger (use app.logger.info or
app.logger.error as appropriate) to log the join action and error; ensure you
import or reference the Flask app context correctly if needed so app.logger is
available.

def on_disconnect(self):
"""Handle client disconnection"""
print(f"Client disconnected from chat namespace")