fix: load fresh session if none are remaining #3890
Conversation
|
/backport 4ffa4eb to stable26 |
1 flaky tests on run #8866 ↗︎
Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Sync > recovers from a lost connection |
Output
Screenshots
|
|
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
|
No need to compile as this is server side fix only & cypress tests. |
| $freshSession = true; | ||
| try { | ||
| $this->documentService->resetDocument($file->getId(), $forceRecreate); | ||
| $freshSession = true; |
There was a problem hiding this comment.
If I understand correctly, this means that we always start a fresh session here, also if resetDocument() throws a DocumentHasUnsavedChangesException, right? Could this lead to data loss of the unsaved changes? Probably we cannot do anything about it anway, as there is no session to apply the changes to anymore, right?
There was a problem hiding this comment.
You're right - that's exactly what this change is aiming for. Let me explain the reasoning in more detail.
We would not need the session to apply the steps - they are self contained - as long as all of them are present.
However if the session does not exist anymore it must have been removed. If steps are still present this only happens during a close call. So close was called on the session.
If other sessions are still around we won't run into this block anyway. If there were other sessions around during the close they would have been closed as well and would have cleaned up the steps. Therefore if we end up here the session must have been the last active session when it was closed. In that case it will have cleaned up it's steps and the remaining unsaved changes are just leftovers from the race condition and yes... we cannot apply them anymore because we lack the history leading up to them.
| $remainingSessions = $this->sessionService->getAllSessions($file->getId()); | ||
| $freshSession = false; | ||
| if ($forceRecreate || count($activeSessions) === 0) { | ||
| if ($forceRecreate || count($remainingSessions) === 0) { |
There was a problem hiding this comment.
Not sure I understand the impact of this change: we only create a fresh session now if "no session was found", instead of "no active session was found" before, right? So we will always try to restore sessions, even if they're not active anymore?
There was a problem hiding this comment.
Yes. Sessions become inactive after 5 minutes. But there may still be steps that were created during the session but never persisted. All the cleanup functions will leave those steps and their session in place (unless forceReload is set). This change makes it so we still recover these changes after more than 5 minutes.
When creating a new session check for all existing sessions to take those into account that are only preserved because of unsaved changes. It there still aren't any sessions make sure to load a fresh editing session even if there are unsaved steps. These steps most likely originate from a race condition when closing the last editing session and pushing steps at the same time. We were seeing empty editing sessions in production while the markdown files had some content. This was happening because there were some steps still around even though all sessions had been closed. When the last session was closed steps were pushed at the same time. The push passed the session check before the session was cleared. The session data and the steps were cleared and only then the new step was added. This left the database in an inconsistent state: * just a few late steps instead of a full history * no sessions these steps would belong to Instead of trying to prevent this race condition detect this state and recover from it. Sessions are sometimes aborted without close request in the middle of editing. In this case all steps are present in the database and the session also remains. The session will not be cleaned up during `resetDocument`. Instead `resetDocument` throws `DocumentHasUnsavedChagesException`. We can detect this scenario because even `removeInactiveSessions` also takes remaining steps into account and leaves sessions with steps alone. Therefore sessions will remain in `apiService->create(...)` and `freshSession` won't be set. Steps are only cleaned up when the last active session is closed. Even if a few steps manage to sneak in after the clean up the session will be removed and so `remainingSessions` will be empty during `create`. Therefore `freshSession` will be set and the content will be send out. The leftovers won't be cleaned up until the file has been saved again. But that should be fine. They will not impact the editing. Signed-off-by: Max <[email protected]>
max-nextcloud
force-pushed
the
debug/yjs-failing-initial-steps
branch
from
4ffa4eb to
d307be8
Compare
mejo-
left a comment
mejo-
left a comment
There was a problem hiding this comment.
Thanks a lot for the extra explanations. Sounds sensible to me. And awesome that you were able to track down the race condition ❤️
|
The backport to stable26 failed. Please do this backport manually. |
|
Manually backported with #3891 |
📝 Summary
When creating a new session
check for all existing sessions
to take those into account
that are only preserved because of unsaved changes.
It there still aren't any sessions
make sure to load a fresh editing session
even if there are unsaved steps.
These steps most likely originate from a race condition when closing the last editing session and pushing steps at the same time.
Background
We were seeing empty editing sessions in production while the markdown files had some content.
This was happening because there were some steps still around even though all sessions had been closed.
When the last session was closed
steps were pushed at the same time.
The push passed the session check before the session was cleared. The session data and the steps were cleared
and only then the new step was added.
This left the database in an inconsistent state:
Fix
Instead of trying to prevent this race condition
detect this state and recover from it.
Scenarios
Unsaved changes
Sessions are sometimes aborted without close request in the middle of editing. In this case all steps are present in the database and the session also remains.
The session will not be cleaned up during
resetDocument. InsteadresetDocumentthrowsDocumentHasUnsavedChagesException.We can detect this scenario because even
removeInactiveSessionsalso takes remaining steps into account and leaves sessions with steps alone. Therefore sessions will remain inapiService->create(...)andfreshSessionwon't be set.Leftover steps
Steps are only cleaned up when the last active session is closed. Even if a few steps manage to sneak in after the clean up the session will be removed
and so
remainingSessionswill be empty duringcreate. ThereforefreshSessionwill be set and the content will be send out. The leftovers won't be cleaned up until the file has been saved again. But that should be fine. They will not impact the editing.🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)