-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(chat): newly created chats being marked as failed #7310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR fixes an issue where newly created chat sessions with no messages were incorrectly marked as failed. It adds a 5-minute grace period by ORing the existing "has non-system message" check with a "created within last 5 minutes" condition using timezone-aware datetime comparison. The implementation properly uses datetime.now(timezone.utc) for timezone-aware comparison with the database's timezone-aware time_created field.
Confidence Score: 4/5
- Safe to merge with minimal risk; logic is sound and addresses the stated issue
- The PR correctly implements a 5-minute grace period for newly created chats using proper timezone-aware datetime handling that matches the database column's
DateTime(timezone=True)type. The logic is straightforward and uses SQLAlchemy'sor_()operator appropriately. One minor style suggestion was made about consistency elsewhere in the file, but it doesn't affect this PR's functionality. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| backend/onyx/db/chat.py | 4/5 | Adds 5-minute grace period for newly created chats using timezone-aware comparison; logic is sound |
Sequence Diagram
sequenceDiagram
participant Client
participant API as get_chat_sessions_by_user
participant DB as Database
Client->>API: "Request chat sessions (include_failed_chats=False)"
API->>DB: "Query ChatSession with filters"
alt Has non-system messages
DB-->>API: "Return session"
else No messages but created < 5 min ago
DB-->>API: "Return session (grace period)"
else No messages and created > 5 min ago
DB-->>API: "Exclude session (failed)"
end
API-->>Client: "Return filtered chat sessions"
Additional Comments (1)
This would align with the pattern used throughout the codebase and ensure consistent timezone handling. Prompt To Fix With AIThis is a comment left during a code review.
Path: backend/onyx/db/chat.py
Line: 400:400
Comment:
While reviewing this PR, I noticed this line uses the deprecated `datetime.utcnow()` instead of the timezone-aware `datetime.now(timezone.utc)` that's used in the new code on line 186. Consider updating this for consistency:
```suggestion
cutoff_time = datetime.now(timezone.utc) - timedelta(days=days_old)
```
This would align with the pattern used throughout the codebase and ensure consistent timezone handling.
How can I resolve this? If you propose a fix, please make it concise. |
Co-authored-by: Dane Urban <[email protected]>
Description
There was an earlier PR that described chats with no messages in them as failed. Currently creating a chat session and adding the first message are done in two separate api routes. This leads to there being a period where the new chat is marked as failed. This PR adds leeway for newly created chats so they are no longer instantly marked as failed.
How Has This Been Tested?
Maual verification
Additional Options
Summary by cubic
Added a 5-minute grace period for newly created chats so sessions without user messages aren’t flagged as failed. The backend query now ORs the “has non-system message” check with “created within last 5 minutes” using a timezone-aware timestamp.
Written for commit b35df5a. Summary will update on new commits.