Repository navigation
Handle multiple messages for one taskId - #712
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly describes the main change: handling duplicate messages associated with one task ID. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description directly explains the duplicate task processing issue, the resulting MultipleObjectsReturnedException, and the listener and mapper changes that address it.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
af0fcf7c-0da8-4e9e-8dad-a6058741499f
📒 Files selected for processing: 4
lib/Controller/ChattyLLMController.phplib/Db/ChattyLLM/MessageMapper.phplib/Listener/ChattyLLMTaskListener.phptests/unit/Listener/ChattyLLMTaskListenerTest.php
💤 Files with no reviewable changes: 1
- lib/Controller/ChattyLLMController.php
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| $this->messageMapper->getMessageByTaskId($sessionId, $task->getId()); | ||
| return; | ||
| } catch (DoesNotExistException $e) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the duplicate check atomic with insertion.
When two executors handle the same task concurrently, both can receive DoesNotExistException before either reaches insert() at Line 158. Both then insert a message. The new check does not prevent the duplicate rows described in the PR. Use a database-enforced unique claim for the session and task, or a distributed lock that covers both the lookup and insertion. Account for existing duplicate rows before adding a unique constraint.
Concurrent task processing executors could run the same chat generation task twice, storing the assistant message twice with the same ocp_task_id. Polling check_generation then threw MultipleObjectsReturnedException, leaving the user without a response until regenerating. Make the message insertion idempotent in ChattyLLMTaskListener and make getMessageByTaskId tolerant of duplicate rows. Assisted-by: OpenCode:GLM-5.3-Flash Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Cover the listener storing the assistant message only once when a task was dispatched twice, and that title tasks are untouched. Assisted-by: OpenCode:GLM-5.3-Flash Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
4b7b555 to
7d1e16a
Compare
Fixes the retrieval of a chat message for one task ID so check_generation does not fail when the chat task ran twice.
This PR should be backported to assistant's stable34 (supports 33->34).
This should not happen in Nextcloud >= 34 thanks to nextcloud/server#62243 which could be backported to stable33.
Problem
When asking something in a new chat, the Assistant sometimes fails to show the answer. The generation indicator spins and never resolves the response. Pressing the "generate" button again works.
Reported for Nextcloud 33.0.8 in nextcloud-enterprise-aio.
The server log shows the following error on the
check_generationpolling endpoint:MultipleObjectsReturnedException: Did not expect more than one result when executing query "... FROM PREFIXassistant_chat_msgs WHERE session_id = ? AND ocp_task_id = ?"
Root cause
The chat message for a completed task is inserted once per
TaskSuccessfulEventinChattyLLMTaskListener. On Nextcloud 33, task processing executors fetch scheduled tasks non-atomically (SynchronousBackgroundJoband the OCS API used by task processing workers), so in a setup with several executors (e.g. the AIO cron container plus the taskprocessingworker container) the same task can be processed twice. Each execution dispatchesTaskSuccessfulEvent, so the same assistant message ends up twice inoc_assistant_chat_msgswith the sameocp_task_id.The frontend then polls
check_generation, whoseMessageMapper::getMessageByTaskId()usesfindEntity()and throwsMultipleObjectsReturnedExceptionas soon as more than one row matches. The exception was not caught, resulting in a 500 for every poll of the affected task. Regenerating schedules a new task, which is why the second attempt succeeds.Note: the atomic task claim (
fix(TaskProcessing): Claim tasks atomically in both SynchronousBackgroundJob and TaskProcessingApiController, #62243) only landed in server 35 and is not backported to stable33, so the underlying race remains on NC 33.Fix
ChattyLLMTaskListeneridempotent: skip the insert if a message with the sameocp_task_idis already stored for the session.MessageMapper::getMessageByTaskId()tolerant of duplicate rows (ORDER BY id LIMIT 1) so existing duplicates keep being pollable while the affected instance's data is cleaned up.Testing
tests/unit/Listener/ChattyLLMTaskListenerTest.phpcover the single insert, the idempotency when the event is dispatched twice and that title generation tasks are untouched.🤖 AI (if applicable)