RCORE-2185 Sync client should steal file ident of fresh realm when performing client reset #7850
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
added 7 commits
June 27, 2024 19:09
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1207Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1213Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1214Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1215Details
💛 - Coveralls |
1 task
added 2 commits
July 1, 2024 10:10
…resh realm download session
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1222Details
💛 - Coveralls |
tgoyne
reviewed
Jul 1, 2024
Comment on lines
1891
to
1895
| // Send QUERY messages when the upload progress client version reaches the snapshot version | ||
| // of a pending subscription, or if this is a fresh realm download session, since UPLOAD | ||
| // messages are not allowed and the upload progress will not be updated. | ||
| return m_upload_progress.client_version >= m_pending_flx_sub_set->snapshot_version || | ||
| REALM_UNLIKELY(is_fresh_realm_download()); |
There was a problem hiding this comment.
I'm pretty sure this logic is invalid? We can't reorder things like this because the pending subscription may depend on the writes before it which are waiting to be uploaded. We don't want to send Pending subscriptions to the server when downloading a fresh Realm; we want to send the currently active one.
There was a problem hiding this comment.
this is only for the fresh realm, so there are no meaningful writes. The fresh realm bootstraps using the active subscriptions of the local realm (but they are marked pending when committed to the fresh realm) so I think it's safe to assume that any changes before that were uploaded because otherwise we would have had the same problem when bootstrapping the local realm.
There was a problem hiding this comment.
The fresh realm sync session has its own (new) subscription store with only one pending subscription (other than query version 0), which is the active subscription from the local realm. Since we're not sending upload messages, we can't rely on the m_upload_progress.client_version value - I guess it could wait until query version 0 is complete.
tgoyne
reviewed
Jul 1, 2024
src/realm/sync/client_base.hpp
Outdated
| Sync = 0, | ||
| // Download a fresh realm | ||
| ClientReset, | ||
| // Download a fresh realm => BIND message sessionReason = 1 |
There was a problem hiding this comment.
This seems to be changing this type to conflate two completely unrelated bits of state. This enum is naming the values passed to the server for the session reason, and adding other states doesn't make much sense.
There was a problem hiding this comment.
Updated - now this enum is back to the original 2 values and a separate bool member variable (m_fresh_realm_download) was added to ClientImpl::Session that is set to true for client reset fresh download sessions in Activate()
The original SessionReason couldn't be used as-is since it is set to ClientReset for both the fresh download and client reset diff sessions...
Comment on lines
2297
to
2298
| // Storage for the file ident from the fresh realm that was migrated to the local realm | ||
| SaltedFileIdent client_file_ident_out; |
There was a problem hiding this comment.
I think it would be better to reread the ident from the file rather than using an out parameter. This would make it harder to accidentally have inconsistent states where we fail to actually update the ident in the file and don't notice.
There was a problem hiding this comment.
Updated - the file_ident_out parameter has been removed and the local realm file ident is being extracted after the client reset diff is complete.
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1231Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1236Details
💛 - Coveralls |
added 2 commits
July 2, 2024 16:57
test/test_client_reset.cpp
Outdated
| version_type current_client_version; | ||
| SaltedFileIdent file_ident; | ||
| SyncProgress sync_progress; | ||
| auto wt = target->start_read(); |
src/realm/sync/client.cpp
Outdated
| @@ -169,6 +169,10 @@ class SessionWrapper final : public util::AtomicRefCountBase, DB::CommitListener | |||
|
|
|||
| const SessionReason m_session_reason; | |||
|
|
|||
| // If false, QUERY and MARK messages are allowed but UPLOAD messages will not | |||
| // be sent to the server. | |||
| const bool m_allow_upload_messages = true; | |||
There was a problem hiding this comment.
no need to assign a default value since you must do it in the constructor
| @@ -1871,13 +1870,17 @@ void Session::send_message() | |||
| return false; | |||
| } | |||
|
|
|||
| // Send QUERY messages when the upload progress client version reaches the snapshot version | |||
| // of a pending subscription, or if this is a fresh realm download session, since UPLOAD | |||
| // messages are not allowed and the upload progress will not be updated. | |||
There was a problem hiding this comment.
the local upload progress is actually updated, but no upload messages are sent
| return m_upload_progress.client_version >= m_pending_flx_sub_set->snapshot_version; | ||
| }; | ||
|
|
||
| if (check_pending_flx_version()) { | ||
| return send_query_change_message(); // throws | ||
| } | ||
|
|
||
| // Don't allow UPLOAD messages for client reset fresh realm download sessions |
There was a problem hiding this comment.
not really doing anything in that sense to justify the comment
| @@ -847,6 +847,10 @@ class ClientImpl::Session { | |||
| /// Returns the schema version the synchronization session connects with to the server. | |||
| uint64_t get_schema_version() noexcept; | |||
|
|
|||
| // Returns false if this session is not allowed to send UPLOAD messages to the server to | |||
| // update the cursor info, such as during a client reset fresh realm download | |||
| bool are_uploads_allowed() noexcept; | |||
There was a problem hiding this comment.
a bit odd to have this method while there is also m_allow_upload
There was a problem hiding this comment.
I renamed the function to upload_messages_allowed() and m_allow_uploads to m_delay_uploads to hopefully better clarify their purpose.
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1237Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1238Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1243Details
💛 - Coveralls |
…et and no file ident
| @@ -952,7 +956,7 @@ class ClientImpl::Session { | |||
| // Set to true when download completion is reached. Set to false after a | |||
There was a problem hiding this comment.
you need to update the comment
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1245Details
💛 - Coveralls |
jbreams
approved these changes
Jul 3, 2024
danieltabacaru
approved these changes
Jul 3, 2024
|
@tgoyne , can you take another look when you get a chance? |
tgoyne
approved these changes
Jul 3, 2024
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What, How & Why?
When a client reset occurs, the local realm should assume the file ident from the fresh realm when the client reset diff takes place at the end of the client reset. Currently, it is requesting a new file ident and using that value for the local realm, which is interfering with the client being able to receive role changes while the client reset diff is in progress.
Fixes #7846
☑️ ToDos
[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed