Fix spurious session corruption error #12280
Conversation
Move the server versions check to each time we reconnect to the server rather than the first time,although, as per comment it will still only trigger the first time, but it will avoid us awaiting and mean we know we're connected to the server when we try, and get automatic retries. Fixes element-hq/element-web#26967
dbkr
added
the
T-Defect
src/stores/LifecycleStore.ts
Outdated
| // Note that although we do this on a reconnect, we cache the server's versions in memory | ||
| // indefinitely, so it will only ever trigger the toast on the first connection after a fresh | ||
| // restart of the client. | ||
| if (await client.isVersionSupported(version)) { |
There was a problem hiding this comment.
It's rather odd that we (sometimes) make another /versions request, rather than using the result of the /versions request that the client has just successfully done, which caused the transition to the Syncing state.
I appreciate that changing that would mostly mean changes in the js-sdk (in the implementation of isVersionSupported, I guess), so I'm not asking you to change this now as part of a quick fix to this issue, but maybe you could record the situation in a comment?
There was a problem hiding this comment.
Yeah, the js-sdk impl also laments the lack of cache expiration which seems roughly in the same category of problems. I can put a comment there?
There was a problem hiding this comment.
Actually I think it does always re-use the result of the /versions request since it caches the response and of course, in this case, we know it's already done one so it never actually does the request. Noted in 8616cc2.
There was a problem hiding this comment.
of course, in this case, we know it's already done one
Per https://github.com/matrix-org/matrix-react-sdk/pull/12280/files/946636614e83b032859f8626e0bfde75ca41df89..8616cc2e1d55cb5bdbf1a5acb0d459e2ffcb994c#r1500777247, I don't think that's right.
src/stores/LifecycleStore.ts
Outdated
| // restart of the client (which has just happened since it's started syncing, so this should | ||
| // never actually cause a request).) |
There was a problem hiding this comment.
I don't think that's accurate. Or rather, it's accurate, but not for the reason given. The sync loop doesn't populate the serverVersionsPromise which is used by isVersionSupported.
There was a problem hiding this comment.
Ah, in my brief investigation it appeared to only do a single request, but not 100% sure so I've just removed that bit of the comment.
Co-authored-by: Richard van der Hoff <[email protected]>
dbkr
added
the
backport staging
* Fix spurious session corruption error Move the server versions check to each time we reconnect to the server rather than the first time,although, as per comment it will still only trigger the first time, but it will avoid us awaiting and mean we know we're connected to the server when we try, and get automatic retries. Fixes element-hq/element-web#26967 * Move test & add regression test * Write some more tests * More comments & catch exceptions in server versions check * Note caching behaviour * Typo Co-authored-by: Richard van der Hoff <[email protected]> * Remove the bit of the comment that might be wrong --------- Co-authored-by: Richard van der Hoff <[email protected]> (cherry picked from commit 1403cd8)
Move the server versions check to each time we reconnect to the server rather than the first time,although, as per comment it will still only trigger the first time, but it will avoid us awaiting and mean we know we're connected to the server when we try, and get automatic retries.
Fixes element-hq/element-web#26967
Checklist