Fix: Delete old sessions in cleanup cron

[benjaminfrueh]

📝 Summary

The oc_text_steps table was growing significantly. This adds a step to the cleanup cron which deletes sessions with their steps older than 90 days. The sessions are deleted in batches of 1000 and the execution is time-limited for 30 seconds.

🏁 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 (README or documentation) has been updated or is not required

[mejo-]

Thanks for tackling this! @max-nextcloud and me discussed this PR and think it's good to merge (except for the nitpicking comment 😆).

Deleting sessions is not very intrusive as the frontend will merely suggest a reload to the user if the session got removed server-side.

The thread of data loss is very minor as that would be only editing steps that exist in the frontend and were not synchronized to the server yet. Since steps get send immediately and we currently set the editor to read-only once sending steps failed for some seconds, it can only be seconds of editing changes to the document.

[benjaminfrueh]

@mejo- and @max-nextcloud I did make some changes shortly before your comment and pushed them. I changed the variable names to be consistent and also for safety added logic so sessions containing step ids >= last_saved_version are never deleted.

Could you review those changes and let me know if there are further todos. I also adjusted the tests accordingly.

[mejo-]

Thanks @benjaminfrueh, much appreciated!

for safety added logic so sessions containing step ids >= last_saved_version are never deleted.

I don't think that's necessary and would be in favour of reverting this change. As @max-nextcloud explained in his comment to your other PR, sessions are bound to a specific client. They're more or less merely the authentication token created when a client joins an editing session.

Any client with an active session will autosave the document every thirty seconds, what incorporates all steps into the saved document and bumps last_saved_version. It's not a problem if steps from deleted sessions exist. Any client applies all steps, regardless from which session they come. So I don't see extra benefit in binding session cleanup to the fact that all steps from this session were applied and autosaved already. But maybe let's wait for the thoughts from @max-nextcloud on this 😉

[benjaminfrueh]

HI @mejo-, thanks again for the detailed feedback. That makes total sense, in this case it should be save to delete any session which is older than the defined threshhold of currently 90 days. I did revert the recent change and added a seperate commit for the variable name consistency.

[max-nextcloud]

All good to go now from my side (besides the missing signoff on the revert that shows in the CI checks). Thanks a lot for tackling this.

[benjaminfrueh]

@max-nextcloud missing sign-off for revert commit is added now

[mejo-]

Sorry, I just spotted that the PR still has logic to clean up steps, which should be reverted as well.

[mejo-]

This delete query should be removed. As explained earlier, we don't want to delete the steps from sessions that got deleted. This task should really merely remove sessions older than 90 days as they will be recreated easily without data loss. Removing steps more or less unconditionally on the other hand might result in data loss.

[benjaminfrueh]

Okay, in the recent commit I removed the step deletion part from the deleteOldSessions function.

[mejo-]

Very nice, now all good to go 🎉

[mejo-]

@benjaminfrueh except for the now failing test 🙈 😆

[benjaminfrueh]

@benjaminfrueh except for the now failing test 🙈 😆

Test is updated now, thanks again for the review!

[mejo-]

/backport to stable32

[mejo-]

/backport to stable31

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[mejo-]

/backport to stable32

[backportbot]

The backport to stable32 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable32
git pull origin stable32

# Create the new backport branch
git checkout -b backport/7665/stable32

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 6687601b 10ab5b56 d866c001 54fb2ef4 1dc1cba7 ebaf60f7 594bf8b4 b1afd239 3665ce75

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/7665/stable32

Error: Failed to check for changes with origin/stable32: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.