-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RCORE-2232 Actually check for unuploaded changes in no_pending_local_changes() #7967
Conversation
77c9cbd
to
9e43725
Compare
Pull Request Test Coverage Report for Build thomas.goyne_491Details
💛 - Coveralls |
size_t base_version = 0; | ||
auto upload_client_version = | ||
version_type(m_arrays->root.get_as_ref_or_tagged(s_progress_upload_client_version_iip).get_as_int()); | ||
if (upload_client_version > m_sync_history_base_version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: size_t base_version = std::max(size_t(upload_client_version - m_sync_history_base_version), 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version_type is unsigned so upload_client_version - m_sync_history_base_version will never be smaller than 0.
I think you mean upload_client_version > last_integrated_client_version
I wrote a test that I think can be used as a base. You can use
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
We can have local changesets stored which have already been uploaded and acknoledged by the server, so checking all of the changesets is incorrect. We need to instead only check changesets for versions after the current position of the upload cursor.
The problem is getting the server to produce the DOWNLOAD message that causes problems in the first place. It requires concurrent uploads and downloads so that the client receives a non-empty download which acknowledges a write but has a changeset based on the version before the write. This seems like it shouldn't be hard to do but every time I try to reduce a complex scenario that causes it to something understandable it stops happening. |
9e43725
to
9252ccc
Compare
We can have local changesets stored which have already been uploaded and acknowledged by the server, so checking all of the changesets is incorrect. We need to instead only check changesets for versions after the current position of the upload cursor.
I wrote a basic test since we didn't have any tests at all which hit the case where we threw this exception, but I couldn't figure out how to write a standalone test for this scenario (one of the tests I wrote for #7921 hit it very indirectly). For things to break we need to have the server send us a DOWNLOAD with upload_client_version > download_client_version (i.e. it's acknowledged the upload of some version, but needs us to store some older version still for merging purposes) and then try to write a copy before we receive a DOWNLOAD message which bumps download_client_version. In practice this requires that we be uploading while receiving non-bootstrap sync downloads, and even then there's a pretty small window where things are broken.