fix: always trust worker storage over stale da-admin on DO rehydration#138
Closed
kptdobe wants to merge 1 commit into
Closed
fix: always trust worker storage over stale da-admin on DO rehydration#138kptdobe wants to merge 1 commit into
kptdobe wants to merge 1 commit into
Conversation
bosschaert
reviewed
May 8, 2026
Comment on lines
383
to
+396
| // Read the stored state from internal worker storage (errors are non-fatal) | ||
| try { | ||
| const timingBeforeReadState = Date.now(); | ||
| const stored = await readState(docName, storage); | ||
| timingReadStateDuration = Date.now() - timingBeforeReadState; | ||
|
|
||
| if (stored && stored.length > 0) { | ||
| Y.applyUpdate(ydoc, stored); | ||
|
|
||
| // Check if the state from the worker storage is the same as the current state in da-admin. | ||
| // So for example if da-admin doesn't have the doc any more, or if it has been altered in | ||
| // another way, we don't use the state of the worker storage. | ||
| const fromStorage = docType === 'json' ? doc2json(ydoc) : doc2aem(ydoc); | ||
| if (fromStorage === current) { | ||
| restored = true; | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log('[docroom] Restored from worker persistence', docName); | ||
| } | ||
| // Worker storage holds the most-recent Yjs state, including edits that have not yet | ||
| // been flushed to da-admin (e.g. because the debounced PUT was still in flight when the | ||
| // DO was evicted). Always trust storage over da-admin; the syncadmin/deleteadmin APIs | ||
| // are responsible for clearing stale storage when da-admin is modified externally. | ||
| restored = true; |
Contributor
There was a problem hiding this comment.
What happens if the document is updated directly in da-admin (eg via a PUT there) while someone has an editor open on the same document? It seems like this change will then basically ignore that change made in da-admin and overwrite it with the editor content...?
Contributor
Author
There was a problem hiding this comment.
Is this a real use case ? If you PUT a completely different doc while someone is editing, I think it is much better that the editor wins...
Contributor
There was a problem hiding this comment.
This is a real use case for situations like localizing documents. Someone may have a doc open in their editor when a new localization batch is started and pushed. In that case we would want the da-admin PUT to overwrite the content in an open editor.
The `fromStorage === current` string comparison failed whenever worker storage held edits newer than da-admin (e.g. debounced PUT still in flight when the DO was evicted). This set restored=false, fired the 1-second reload timer, and overwrote the ydoc with the stale da-admin snapshot — causing recently-inserted images to disappear. Worker storage is always the most-recent Yjs state for the collaborative session. The syncadmin/deleteadmin APIs are responsible for clearing storage when da-admin is modified externally, so removing the string comparison is safe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kptdobe
force-pushed
the
fix/trust-worker-storage
branch
from
a761e94 to
9c8e251
Compare
Contributor
Author
|
This one is too controversial, let's close it for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a Durable Object is evicted (e.g. during an image upload that briefly drops the WebSocket),
bindStaterefetches the document from da-admin and comparesfromStorage === current. This comparison fails whenever worker storage holds edits that have not yet been flushed to da-admin (debounced PUT still in flight). The mismatch setsrestored = false, fires the 1-second reload timer, and overwrites the Yjs document with the stale da-admin snapshot — causing recently-inserted images to disappear.Root cause
The
fromStorage === currentstring comparison was meant to detect "da-admin was externally modified while the DO was sleeping." In practice it fires in the normal case: any edit that hasn't been saved to da-admin yet will producefromStorage !== current, triggering the destructive reload.Solution
Remove the string comparison and always set
restored = truewhen valid storage exists.Why this is safe: Worker storage is updated synchronously on every Yjs transaction (via the
updatelistener) and therefore always holds the most-recent collaborative state — potentially newer than da-admin. Thesyncadminanddeleteadminadmin APIs are the correct mechanism for clearing stale storage when da-admin is modified externally.Tests
Added: Test bindstate trusts storage when storage is newer than da-admin — verifies that the reload timer is NOT registered when storage contains edits not yet in da-admin, and that the storage content survives without being overwritten.
🤖 Generated with Claude Code