Fix resolve concurrency issues with letters status being polled#2055
Conversation
Summary of ChangesHello @Shayan105, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical concurrency issues that arose from the integration of letter draft functionality, progress bar updates, and page limit enforcement. By introducing atomic database transactions for updating letter preview and generation statuses, the changes ensure data integrity and reliability, preventing inconsistent states when multiple operations might be attempting to modify the same letter record simultaneously. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors parts of the S2B letter generation to use separate database cursors for status updates, which is a good approach to resolve concurrency issues with UI polling. However, the refactoring has introduced a few issues. In both the preview and generate_letters_job methods, the logic to update the generation_status and clear generation_error_message on success was accidentally removed. There is also a duplicated line in the preview method. I've provided suggestions to fix these omissions and the redundant code. Additionally, there's a minor inconsistency in the return value of generate_letters_job which I've also pointed out.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
I'm not sure this change is needed. The new cursor you are creating is at the end of the method. At the end of the method when it returns, the main transaction will be committed so I don't see why this will effectively change something. Both polling the status and updating it should not cause deadlocks. It's doing simultaneous writes that can cause deadlocks. |
Fix sytax error. Refactor and ensure atomiity of transaction Fix concurrency problem Refactor and add comments Remove typos Co-Authored-By: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ith-letters-status-being-polled' into FIX-Resolve-concurrency-issues-with-letters-status-being-polled
|
@ecino After discussion the following change has been applied : replace the update_generation_status() to a mode general isolated_write() do isolated trasactions to the DB. |
| "generation_status": "done", | ||
| "generation_error_message": False, |
There was a problem hiding this comment.
Shouldn't we keep this in case the preview is not called from the frontend controller but from the backend ?
| { | ||
| "state": "done", | ||
| "date": fields.Datetime.now(), | ||
| "generation_status": "done", |
There was a problem hiding this comment.
Shouldn't we keep this for the backend calls?
NoeBerdoz
deleted the
FIX-Resolve-concurrency-issues-with-letters-status-being-polled
branch
3 participants
This PR is linked to this PR : CompassionCH/compassion-website#181
Summary
This PR fixes some issues resulting from the merging of the three following tasks :
🐛 Problem Description
After the recent merges, several regressions were identified:
generation_statusand the frontend polling for that status was causing a deadlock. The UI would get stuck in a loading state because it was reading an inconsistent status from the database before the transaction committed.✅ Proposed Solutions
To resolve these issues, the following changes have been implemented:
preview()method, which contains the page count validation, is now called during the final "send" action. This ensures the page limit is checked just before submission.generation_statusis now forced to commit immediately in its own transaction. This ensures atomicity and prevents the frontend from reading a stale state.statusandgeneration_statusfields are reverted to their initial values, allowing the corrected letter to be processed as a new attempt.