Skip to content

Conversation

@max-nextcloud
Copy link
Collaborator

Backport of #3890

📝 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:

  • 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.

Scenarios

Actual remaining 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. 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.

Leftover steps from race condition

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.

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation is not required

@cypress
Copy link

cypress bot commented Mar 5, 2023

Passing run #8906 ↗︎

0 140 0 0 Flakiness 0

Details:

[stable26] fix: load fresh session if none are remaining
Project: Text Commit: 7e47b82168
Status: Passed Duration: 03:24 💡
Started: Mar 6, 2023 8:10 AM Ended: Mar 6, 2023 8:14 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@max-nextcloud max-nextcloud changed the title fix: load fresh session if none are remaining [stable26] fix: load fresh session if none are remaining Mar 5, 2023
@max-nextcloud max-nextcloud added bug Something isn't working 4. to release feature: sync backported successfully backported 26 feedback labels Mar 6, 2023
@max-nextcloud max-nextcloud added this to the Nextcloud 26 milestone Mar 6, 2023
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 max-nextcloud force-pushed the backport/stable26/3890-load-fresh-session branch from 130233a to 7e47b82 Compare March 6, 2023 08:02
@juliusknorr juliusknorr merged commit d79cdaf into stable26 Mar 6, 2023
@delete-merged-branch delete-merged-branch bot deleted the backport/stable26/3890-load-fresh-session branch March 6, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release 26 feedback backported successfully backported bug Something isn't working feature: sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants