Tuesday, August 13, 2024
2 changes · saas-17.4
Resolved issues and error corrections
This fix prevents users from missing real-time notifications during busy or concurrent activity, improving reliability for collaboration features such as Discuss, live chat, and shared editing. It also batches notification sending more efficiently to reduce unnecessary backend work.
Original PR description
Before this PR, some notifications could be missed due to the way notifications are fetched. We tracked the last fetched ID and only fetched notifications with greater IDs. However, this approach is…
Before this PR, some notifications could be missed due to the way notifications are fetched. We tracked the last fetched ID and only fetched notifications with greater IDs. However, this approach is subject to concurrency issues because primary keys are assigned before commit. As a result, we might fetch a notification with a greater ID and miss one with a lower ID that hasn't been committed yet. Problematic scenario: - A bus_bus record is inserted with ID 1. - A bus_bus record is inserted and committed with ID 2. - A bus_bus record with ID 2 is fetched, but ID 1 is missing because it was not yet committed. - The bus_bus record with ID 1 was missed. To solve this issue, the WebSocket class now keeps track of notifications received within a 10-second window. Fetches will target unknown notifications with an ID greater than the smallest one in this window. This effectively prevents missing notifications, as the 10-second window is much larger than the commit concurrency. This PR also includes a back-port of https://github.com/odoo/odoo/pull/174874 to ensure the create is done as close as possible to the commit. Enterprise: https://github.com/odoo/enterprise/pull/68302 Note 1: the bug exists since forever (v8.0), so it should be fixed in all versions ideally. It potentially explains a lot of strange issues we had with many collaborative features from the past and until now (discuss and longpolling back then, editor and RTC more recently, to only name a few). Starting to fix in 17.4 now to fix our prod in priority. Note 2: even with this fix, the order of notifications is still not guaranteed (but at least we don't miss them anymore). If 2 notifications from 2 different commits are in the same batch of dispatch, notifications will be ordered by acquisition time of id, and not by commit time (which would be preferable most of the time, as data in db will be based on commit order). If the same two notifications and 2 commits are in a separate dispatch, the notifications will be ordered by commit time, as the notification from the first commit will be sent immediately, even if is has a higher id, and the one from the 2nd commit will be sent afterwards. Very order-sensitive business code should therefore enforce order either with locks or through business data. If there is an actual concurrency issue the auto-retry will probably resolve the order by itself most of the time though.
This fix stops Odoo from saving a company selection that the user is not allowed to access when opening mail links. Users may still see an access error for restricted records, but their session will no longer become unusable afterward.
Original PR description
As of #157399, the /mail/view route attempts to expand the `cids` cookie with the "suggested_company", which should be the company of the target record, if the current companies for the users aren't…
As of #157399, the /mail/view route attempts to expand the `cids` cookie with the "suggested_company", which should be the company of the target record, if the current companies for the users aren't sufficient to access it. Unfortunately, the target record may belong to a company the user does not have access to, so expanding `cids` would lead to an error page, due to accessing a forbidden company. Previously though, the expanded `cids` would be passed via query params to the redirected URL. It would lead to an error page, but wouldn't permanently impact the user session. After #157399, the updated `cids` cookie is permanent for the session, and the user will be permanently locked out, each attempt at navigating the backend leading to an error page. For example, going to `/web` will lead to an access error, redirecting the user instantly to `/web/login?error=access`. To fix this we should first do the access check with the updated `allowed_company_ids`, and only update the cookie if that worked. In case of failure, the user will have an error page, but there will be no impact to their session.