Fix: Add cleanup for orphaned text steps

[benjaminfrueh]

📝 Summary

The oc_text_steps table was growing significantly. One reason for that were orphaned steps accumulating in the database. Steps without existing sessions were never cleaned up by the existing cleanup job.

This addresses orphaned steps only. A separate PR will add garbage collection for very old text sessions to prevent long-term accumulation.

The steps are deleted in batches of 1000 and the execution is time-limited for 30 seconds.

Fixes: #3740
Fixes: #3915

🏁 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

[max-nextcloud]

Thanks a lot for tackling this @benjaminfrueh !

The wording here is confusing and we've been wanting to fix this for a while - but never got to it.
A session is a single users editing connection. It can be ephemeral. If the network is flaky the and the user disconnects for more then 30 seconds the client will reconnect and create a new session for example. If multiple users edit the same document there are multiple sessions at the same time.
The steps are basically capturing the changes a user made with a given session. Even if the session does not exist anymore they continue to be relevant as even if i am not editing anymore and my session has been cleaned up - the changes that I made should persist.

However every 30 seconds we autosave the document including the full yjs editing history (docuementState). Someone who starts to edit will only fetch the latest documentState and all steps that have been created afterwards. So for people joining steps with an id lower than the lastSavedVersion in the corresponding document are not needed anymore. However there may be people who are already connected who have not yet received all steps up to the lastSavedVersion so we cannot clean it up right away.

I'm sorry this is such a mess to begin with. It evolved over time and we did not manage to clean it up. I'm happy to talk it through with you to figure out which steps we can remove and how to clean this up for the future.

[mejo-]

Thanks for tackling this. Code changes look good to me. I only have a minor nitpicking comment.

Also, these issues seem related and it might be worth to mention them in the commit or PR message:

• #3740
• #3915

[benjaminfrueh]

Hi @max-nextcloud and @mejo- , thank you for the feedback. I've pushed some changes.

• Only orphaned steps with a version (id) < lastSavedVersion are deleted, with a 24h safety buffer
• Steps with no existing document are deleted
• Steps with an existing session or version >= lastSavedVersion are not deleted
• Added Qb suffixes to the query builder variables

If you have any more suggestions or proposed changes for the cleanup I would be happy to discuss them with you in detail.

[mejo-]

Points from discussion with @benjaminfrueh:

• Condition to keep steps of active sessions doesn't help for scenario when client comes back after long time. Steps from sessions that got closed in the meantime will be missing then as well.
• What happens if client comes back after a long time and asks for steps older than last one available on server? Code seems like I get an incomplete subset of steps (if I ask for steps > 10 and server has only steps starting from 15 onwards, I get steps 15+).
• What's the consequence client-side? Does it reconnect because applying the steps results in the yjs state that indicates missing steps?
• Shall we return an error server-side instead if asking for steps older than those that exist?

[max-nextcloud]

@mejo- You are right. I was confusing what the query does. I was thinking it only removes steps for documents where no sessions exist anymore at all. But it would remove those from a session that stopped existing even if further sessions exist.

Right now the client would fetch the steps that it can get. It may not notice that steps are missing if there are no steps arriving that depend upon them. If there are steps missing that later steps depend upon the client will notice and attempt to recover the state by loading the saved documentState and all steps since then. However we have observed that that can lead to small parts of the content being duplicated. So I would not want to rely on it.

Detecting the missing step server side does not seem possible right now as we are just using the auto-incrementing id to retrieve newer steps. We used to have a version count that was per document and increased in steps of one - but I thought it was a clever idea to remove that 😶‍🌫️ - which I've been regretting ever since 😉 . But now you cannot tell if some step is missing in between the version provided by the client and the latest step.

[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.)

[max-nextcloud]

Thanks a lot for tackling this!

@mejo- and me observed an out of sync state that was persisted to the documentState and could be recovered by replaying all the steps.

We need to make sure we can always load the editing session from the documentState and the steps that follow it.

Right now this does not seem to be the case. Let's discuss together what can be done about this and then get this merged once we are sure the client does not write incomplete documentState anymore. See also #7704 and #7692 for the bigger picture.

[max-nextcloud]

There have been no reconnect attempts in the last week and we also observed none. So seems fine to clean up the orphaned steps as planned.

[max-nextcloud]

@mejo- I think your change request has been addressed, right?

[mejo-]

@max-nextcloud agreed, let's get this merged. I guess it should also be backported, right @benjaminfrueh?

[mejo-]

/backport to stable32