fix/autosave timer - #8917
Conversation
max-nextcloud
commented
Jul 27, 2026
max-nextcloud
commented
- fix(autosave): every time the typing stops
- chore(refactor): document tracking into provideSyncService
- fix(autosave): only save when server is ready
- fix(autosave): better dirty tracking with versions
- chore(refactor): Save service with its own bus
- chore(refactor): separate SaveService from SyncService and ydoc
- fix(autosave): exponential delay for retries on error
- fix(autosave): retry if server throttled save request
- fix(autosave): handle out of sync clocks
8be1182 to
74180f3
Compare
ffd4f3b to
f89b634
Compare
mejo-
left a comment
mejo-
left a comment
There was a problem hiding this comment.
I have quite some comments, but most of them are probably indicators that I'm uncertain about the impact of the code changes. To be honest I would prefer pairing review in a call 😆
| force, | ||
| manualSave, | ||
| }) | ||
| this.emit('stateChange', { dirty: false }) |
There was a problem hiding this comment.
Was this removal intentional? It seems unrelated to the commit and I didn't check whether it's save to be removed.
There was a problem hiding this comment.
You're right. It's in the wrong commit. Should have been part of 3355f46 .
It's intentional. Before we toggled the dirty state by emitting these events. But that does not work for some cases. Now we compare versions which covers these events:
- Server response happily to save but does not actually save.
The server will only save new versions every 10 seconds.
If the latest save just happened it will still respond with 200
but list the outdated version in the response.
Comparing the versions shows that our changes have not been saved yet. - Other user already saved the file with our changes.
So far dirty would stay true until WE save our changes.
Comparing the versions also shows the file was saved
when it was saved by someone else.
| if (now < lastSave + SERVER_AUTOSAVE_INTERVAL) { | ||
| logger.debug('Not autosaving as last save is recent', { lastSave, now }) | ||
| const nextSave = lastSave + SERVER_AUTOSAVE_INTERVAL + Math.random() * MAX_RANDOM_AUTOSAVE_DELAY | ||
| setTimeout(() => this.autosave(), (nextSave - now) * 1000) |
There was a problem hiding this comment.
It took me some time to understand the logic here. If I got it right, it means that the next save will happen sometime between 0 and 13 seconds from now but not before lastSave + 10s.
What's the purpose of randomising it in a range of 3 seconds? To make autosaves more useful because other clients save at a different point in time? Or to lower the load on the server?
There was a problem hiding this comment.
Without the random range all clients would attempt to save at the same time. That seemed to be asking for trouble. I had race conditions in mind - but also what you are saying.
I later removed both the attempt at calculating the time the server is available again and the randomness for the sake of simplicity and deterministic behavior of the client for easier testing.
| * @param event that triggered the update | ||
| * @param event.version with changes pushed to the server | ||
| */ | ||
| function updateVersionWithChanges(event: { version: number }) { |
There was a problem hiding this comment.
I always forget about the logic, but can we be sure that a remote higher version always includes our changes? Or can a remote client push a higher version than our local version because they added changes and our changes get lost that way?
There was a problem hiding this comment.
There are two different mechanisms:
- For the sync everyone pushes their changes, the server assigns incremental ids to them and yjs builds a consistent document from them. Our local changes are first aggregated in the editor ydoc and then send as an update (step) to the server. The update is independent from
document.version. It is a diff against a ydoc that is build based on all steps we received from the server. The server then responds with a version which is the version used to store these steps. Any later version means the corresponding steps have been saved later. They do not necessarily build upon our own changes - but yjs handles that. - For the save everyone pushes a serialization of their current editor doc alongside the last version they received and processed. The serialized content will always include all changes up to the version specified and may include local changes that have not been pushed yet.
| // double the delay on every failed attempt | ||
| const delay = Math.pow(2, this.autosaveErrorCount) * RETRY_TIMEOUT | ||
| setTimeout(this.autosave, delay * 1000) | ||
| this.autosaveErrorCount++ |
There was a problem hiding this comment.
This can grow infinitely, no? Wouldn't it be better to clamp this to a maximum value?
Also, Math.pow(2, this.autosaveErrorCount) * RETRY_TIMEOUT is a bit hard to parse in the first moment, even though I must admit that it's a clever way to achieve "doubly on each increment" 😉
There was a problem hiding this comment.
Good points. We do not use the counter anywhere else so we could as well just start with an instance variable and then double that up to a maximum. Also... if the user continues typing autosave will be triggered again anyway. If the server is in maintenance mode and does not respond we will keep trying as the lastVersionSavedTime does not increment and this error handling does not block further requests. So we will need something else anyway.
| */ | ||
| function updateDocument(event: { document: Document }) { | ||
| document.value = event.document | ||
| // Limit lastSavedVersionTime to now. No saving from the future. |
There was a problem hiding this comment.
Given this change I now wonder whether it's clever to calculate delays by comparing server and client timestamps at all. It's probably pretty common that the clocks differ by a few seconds, no?
There was a problem hiding this comment.
I did remove some of the computations later on. But maybe we should fully rely on the clients clock.
| if (saved === false) { | ||
| // Make sure to not hammer the server if clocks are out of sync. | ||
| setTimeout(this.autosave, SERVER_AUTOSAVE_INTERVAL * 1000) | ||
| setTimeout(this.autosave, (SERVER_AUTOSAVE_INTERVAL - AUTOSAVE_DEBOUNCE) * 1000) |
There was a problem hiding this comment.
What's the purpose of subtracting one second here?
There was a problem hiding this comment.
this.autosave will trigger a debounce and only save afterwards. So if we want to save after SERVER_AUTOSAVE_INTERVAL we need to take that into account.
Debounce will delay the execution of the function every time it is called. So autosave was waiting until no updates occured for 30 seconds. That's a long time and does not feel responsive. Try to autosave every time the user stops typing for at least one second. Signed-off-by: Max <max@nextcloud.com>
Also removed the document attribute from the `sync` event load. It is already included in the `change` event load. Signed-off-by: Max <max@nextcloud.com>
The server will only accept autosaves every 10 seconds. `document` contains the last saved timestamp. Compute the time to autosave next. Add a small random delay (up to 3 seconds) to avoid all connected clients from saving at the same time. Signed-off-by: Max <max@nextcloud.com>
Keep track of the version that has our changes and compare it to the last saved version on the server. This allows fixing two scenarios: * Server response happily to save but does not actually save. The server will only save new versions every 10 seconds. If the latest save just happened it will still respond with 200 but list the outdated version in the response. Comparing the versions shows that our changes have not been saved yet. * Other user already saved the file. our changes. So far dirty would stay true until WE save our changes. Comparing the versions also shows the file was saved when it was saved by someone else. Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
* `getSaveData` now prepares the data to send to the server. * `provideSaveService` now handles all the sync service events calling functions on `saveService` where needed. Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
The server will only perform one actual save every 10 seconds. Trigger another autosave if the save attempt was throttled. `document` is udpated by throttled save attempts. Rely on its `lastSavedVersionTime` for the retry. Signed-off-by: Max <max@nextcloud.com>
* If the server claims to have saved the doc in the future use the current timestamp instead. Autosave happens at least ten seconds after `lastSavedVersionTime`. So if that was far into the future it would never happen. * If the server is living in the past delay the autosave retries without relying on `lastSavedVersionTime`. Signed-off-by: Max <max@nextcloud.com>
Adding a random offset makes determenistic testing harder. Now we also do not depend on in sync clocks. Signed-off-by: Max <max@nextcloud.com>
Autosave is triggered by the push of the steps. Saving before that delays the autosave because of server throttling. Signed-off-by: Max <max@nextcloud.com>
Some of our runners are slow. Give them more time to finish the runs. Also separate local and CI config in the config file. Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Only consider the local clock. Avoid problems if clocks are out of sync or if the server is unresponsive and `lastSavedVersionTime` does not get updated Signed-off-by: Max <max@nextcloud.com>
f89b634 to
a743d03
Compare
2 participants
