fix: stale caches for sync state when peer is added #732
Conversation
This file contains 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
This fixes the following bug: 1. Device A creates a project. 2. Device A creates an observation. 3. Device A reads the sync state. 4. Device A invites Device B. 5. Device A reads the sync state again. It's wrong! It's the same state as in Step 3. This happened because of two stale state caches, both of which I fixed. - `NamespaceSyncState`'s cache needed to be invalidated when a peer gets added. I did this by adding an update in `CoreSyncState#addPeer`, which `NamespaceSyncState` "listens to". - `SyncState`'s cache also needed to be invalidated. I investigated doing this before realizing that its cache only saves a single method call to `NamespaceSyncState.prototype.getState`, which is itself cached. Rather than properly fix the cache (which would add some additional complexity), I simply removed it. Fixes [#724]. [#724]: #724
gmaclennan
approved these changes
Jul 30, 2024
There was a problem hiding this comment.
Thanks for all the research into this, and for finding both a fix and tests for the failing case. Apologies that this code is a bit complicated - I found it hard to simplify! The solution makes sense, and it does seem like we were "over-caching" and I think it's fine to remove that unnecessary cache. At some point I would like to gather some metrics via Sentry about how long the state calculations are taking on low-end devices, when there are lots of cores and lots of data.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes the following bug:
This happened because of two stale state caches, both of which I fixed.
NamespaceSyncState's cache needed to be invalidated when a peer gets added. I did this by adding an update inCoreSyncState#addPeer, whichNamespaceSyncState"listens to".SyncState's cache also needed to be invalidated. I investigated doing this before realizing that its cache only saves a single method call toNamespaceSyncState.prototype.getState, which is itself cached. Rather than properly fix the cache (which would add some additional complexity), I simply removed it.Fixes #724.