RCORE-2185 Sync client should steal file ident of fresh realm when performing client reset

CHANGELOG.md

@@ -22,6 +22,7 @@
 ### Internals
 * Fixed `Table::remove_object_recursive` which wouldn't recursively follow links through a single `Mixed` property. This feature is exposed publicly on `Table` but no SDK currently uses it, so this is considered internal. ([#7829](https://github.com/realm/realm-core/issues/7829), likely since the introduction of Mixed)
 * Upload completion is now tracked in a multiprocess-compatible manner ([PR #7796](https://github.com/realm/realm-core/pull/7796)).
+* The local realm will assume the the client file ident of the fresh realm during a client reset. ([PR #7850](https://github.com/realm/realm-core/pull/7850))
 
 ----------------------------------------------
 

src/realm/sync/config.hpp

@@ -134,6 +134,7 @@ enum class SyncClientHookEvent {
     BindMessageSent,
     ClientResetMergeComplete,
     BootstrapBatchAboutToProcess,
+    UploadMessageSent,
 };
 
 enum class SyncClientHookAction {

src/realm/sync/noinst/client_impl_base.cpp

@@ -1688,8 +1688,10 @@ void Session::activate()
     if (REALM_LIKELY(!get_client().is_dry_run())) {
         bool file_exists = util::File::exists(get_realm_path());
         m_performing_client_reset = get_client_reset_config().has_value();
+        m_fresh_realm_download = client_reset::is_fresh_path(get_realm_path());
 
-        logger.info("client_reset_config = %1, Realm exists = %2 ", m_performing_client_reset, file_exists);
+        logger.info("client_reset_config = %1, Realm exists = %2, fresh realm download = %3",
+                    m_performing_client_reset, file_exists, m_fresh_realm_download ? "yes" : "no");
         if (!m_performing_client_reset) {
             get_history().get_status(m_last_version_available, m_client_file_ident, m_progress); // Throws
         }
@@ -1701,7 +1703,7 @@ void Session::activate()
     REALM_ASSERT_3(m_last_version_available, >=, m_progress.upload.client_version);
     init_progress_handler();
 
-    logger.debug("last_version_available  = %1", m_last_version_available);                    // Throws
+    logger.debug("last_version_available = %1", m_last_version_available);                     // Throws
     logger.debug("progress_download_server_version = %1", m_progress.download.server_version); // Throws
     logger.debug("progress_download_client_version = %1",
                  m_progress.download.last_integrated_client_version);                                      // Throws
@@ -1871,14 +1873,20 @@ void Session::send_message()
             return false;
         }
 
-        return m_upload_progress.client_version >= m_pending_flx_sub_set->snapshot_version;
+        // 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(m_fresh_realm_download);
     };
 
     if (check_pending_flx_version()) {
         return send_query_change_message(); // throws
     }
 
-    if (m_allow_upload && (m_last_version_available > m_upload_progress.client_version)) {
+    // Don't allow UPLOAD messages for client reset fresh realm download sessions
+    if (REALM_LIKELY(!m_fresh_realm_download) && m_allow_upload &&
+        (m_last_version_available > m_upload_progress.client_version)) {
         return send_upload_message(); // Throws
     }
 }
@@ -1889,7 +1897,7 @@ void Session::send_bind_message()
     REALM_ASSERT_EX(m_state == Active, m_state);
 
     session_ident_type session_ident = m_ident;
-    bool need_client_file_ident = !have_client_file_ident();
+    bool need_client_file_ident = !have_client_file_ident() && !m_performing_client_reset;
     const bool is_subserver = false;
 
 
@@ -2135,6 +2143,8 @@ void Session::send_upload_message()
                                                locked_server_version); // Throws
     m_conn.initiate_write_message(out, this);                          // Throws
 
+    call_debug_hook(SyncClientHookEvent::UploadMessageSent);
+
     // Other messages may be waiting to be sent
     enlist_to_send(); // Throws
 }
@@ -2245,29 +2255,50 @@ bool Session::client_reset_if_needed()
         return false;
     }
 
+    // Save a copy of the status and action in case an error/exception occurs
+    Status cr_status = client_reset_config->error;
+    ProtocolErrorInfo::Action cr_action = client_reset_config->action;
+
     auto on_flx_version_complete = [this](int64_t version) {
         this->on_flx_sync_version_complete(version);
     };
-    bool did_reset =
-        client_reset::perform_client_reset(logger, *get_db(), std::move(*client_reset_config), m_client_file_ident,
-                                           get_flx_subscription_store(), on_flx_version_complete);
+    try {
+        // The file ident from the fresh realm will be copied over to the local realm
+        bool did_reset = client_reset::perform_client_reset(logger, *get_db(), std::move(*client_reset_config),
+                                                            get_flx_subscription_store(), on_flx_version_complete);
 
-    call_debug_hook(SyncClientHookEvent::ClientResetMergeComplete);
-    if (!did_reset) {
+        call_debug_hook(SyncClientHookEvent::ClientResetMergeComplete);
+        if (!did_reset) {
+            return false;
+        }
+    }
+    catch (const std::exception& e) {
+        auto err_msg = util::format("A fatal error occurred during '%1' client reset diff for %2: '%3'", cr_action,
+                                    cr_status, e.what());
+        logger.error(err_msg.c_str());
+        SessionErrorInfo err_info(Status{ErrorCodes::AutoClientResetFailed, err_msg}, IsFatal{true});
+        suspend(err_info);
         return false;
     }
 
     // The fresh Realm has been used to reset the state
-    logger.debug("Client reset is completed, path=%1", get_realm_path()); // Throws
+    logger.debug("Client reset is completed, path = %1", get_realm_path()); // Throws
+
+    get_history().get_status(m_last_version_available, m_client_file_ident, m_progress); // Throws
+    // Print the version/progress information before performing the asserts
+    logger.debug("client_file_ident = %1, client_file_ident_salt = %2", m_client_file_ident.ident,
+                 m_client_file_ident.salt);                                // Throws
+    logger.debug("last_version_available = %1", m_last_version_available); // Throws
+    logger.debug("upload_progress_client_version = %1, upload_progress_server_version = %2",
+                 m_progress.upload.client_version,
+                 m_progress.upload.last_integrated_server_version); // Throws
+    logger.debug("download_progress_client_version = %1, download_progress_server_version = %2",
+                 m_progress.download.last_integrated_client_version,
+                 m_progress.download.server_version); // Throws
 
-    SaltedFileIdent client_file_ident;
-    get_history().get_status(m_last_version_available, client_file_ident, m_progress); // Throws
-    REALM_ASSERT_3(m_client_file_ident.ident, ==, client_file_ident.ident);
-    REALM_ASSERT_3(m_client_file_ident.salt, ==, client_file_ident.salt);
     REALM_ASSERT_EX(m_progress.download.last_integrated_client_version == 0,
                     m_progress.download.last_integrated_client_version);
     REALM_ASSERT_EX(m_progress.upload.client_version == 0, m_progress.upload.client_version);
-    logger.trace("last_version_available  = %1", m_last_version_available); // Throws
 
     m_upload_progress = m_progress.upload;
     m_download_progress = m_progress.download;
@@ -2321,35 +2352,10 @@ Status Session::receive_ident_message(SaltedFileIdent client_file_ident)
         return Status::OK();       // Success
     }
 
-    // if a client reset happens, it will take care of setting the file ident
-    // and if not, we do it here
-    bool did_client_reset = false;
-
-    // Save some of the client reset info for reporting to the client if an error occurs.
-    Status cr_status(Status::OK()); // Start with no client reset
-    ProtocolErrorInfo::Action cr_action = ProtocolErrorInfo::Action::NoAction;
-    if (auto& cr_config = get_client_reset_config()) {
-        cr_status = cr_config->error;
-        cr_action = cr_config->action;
-    }
-
-    try {
-        did_client_reset = client_reset_if_needed();
-    }
-    catch (const std::exception& e) {
-        auto err_msg = util::format("A fatal error occurred during '%1' client reset for %2: '%3'", cr_action,
-                                    cr_status, e.what());
-        logger.error(err_msg.c_str());
-        SessionErrorInfo err_info(Status{ErrorCodes::AutoClientResetFailed, err_msg}, IsFatal{true});
-        suspend(err_info);
-        return Status::OK();
-    }
-    if (!did_client_reset) {
-        get_history().set_client_file_ident(client_file_ident,
-                                            m_fix_up_object_ids); // Throws
-        m_progress.download.last_integrated_client_version = 0;
-        m_progress.upload.client_version = 0;
-    }
+    get_history().set_client_file_ident(client_file_ident,
+                                        m_fix_up_object_ids); // Throws
+    m_progress.download.last_integrated_client_version = 0;
+    m_progress.upload.client_version = 0;
 
     // Ready to send the IDENT message
     ensure_enlisted_to_send(); // Throws

src/realm/sync/noinst/client_impl_base.hpp

@@ -983,6 +983,9 @@ class ClientImpl::Session {
     // True while this session is in the process of performing a client reset.
     bool m_performing_client_reset = false;
 
+    // True if this session is used to download a fresh realm during a client reset.
+    bool m_fresh_realm_download = false;
+
     // The latest sync progress reported by the server via a DOWNLOAD
     // message. See struct SyncProgress for a description. The values stored in
     // `m_progress` either are persisted, or are about to be.
@@ -1427,6 +1430,12 @@ inline void ClientImpl::Session::message_sent()
     // No message will be sent after the UNBIND message
     REALM_ASSERT(!m_unbind_message_send_complete);
 
+    // Is there a pending client reset diff to try to perform once the bind
+    // message has been sent?
+    if (m_bind_message_sent && m_performing_client_reset) {
+        client_reset_if_needed(); // resets m_performing_client_reset to false
+    }
+
     if (m_unbind_message_sent) {
         REALM_ASSERT(!m_enlisted_to_send);
 

src/realm/sync/noinst/client_reset.cpp

@@ -480,8 +480,8 @@ ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResy
     return mode;
 }
 
-bool perform_client_reset_diff(DB& db_local, sync::ClientReset& reset_config, sync::SaltedFileIdent client_file_ident,
-                               util::Logger& logger, sync::SubscriptionStore* sub_store,
+bool perform_client_reset_diff(DB& db_local, sync::ClientReset& reset_config, util::Logger& logger,
+                               sync::SubscriptionStore* sub_store,
                                util::FunctionRef<void(int64_t)> on_flx_version_complete)
 {
     DB& db_remote = *reset_config.fresh_copy;
@@ -491,13 +491,6 @@ bool perform_client_reset_diff(DB& db_local, sync::ClientReset& reset_config, sy
     bool recover_local_changes =
         actual_mode == ClientResyncMode::Recover || actual_mode == ClientResyncMode::RecoverOrDiscard;
 
-    logger.info(util::LogCategory::reset,
-                "Client reset: path_local = %1, client_file_ident = (ident: %2, salt: %3), "
-                "remote_path = %4, requested_mode = %5, action = %6, actual_mode = %7, will_recover = %8, "
-                "originating_error = %9",
-                db_local.get_path(), client_file_ident.ident, client_file_ident.salt, db_remote.get_path(),
-                reset_config.mode, reset_config.action, actual_mode, recover_local_changes, reset_config.error);
-
     auto& repl_local = dynamic_cast<ClientReplication&>(*db_local.get_replication());
     auto& history_local = repl_local.get_history();
     history_local.ensure_updated(wt_local->get_version());
@@ -507,14 +500,22 @@ bool perform_client_reset_diff(DB& db_local, sync::ClientReset& reset_config, sy
     auto& history_remote = repl_remote.get_history();
 
     sync::SaltedVersion fresh_server_version = {0, 0};
+    sync::SaltedFileIdent fresh_file_ident = {0, 0};
     {
         SyncProgress remote_progress;
         sync::version_type remote_version_unused;
-        SaltedFileIdent remote_ident_unused;
-        history_remote.get_status(remote_version_unused, remote_ident_unused, remote_progress);
+        history_remote.get_status(remote_version_unused, fresh_file_ident, remote_progress);
         fresh_server_version = remote_progress.latest_server_version;
     }
 
+    logger.info(util::LogCategory::reset,
+                "Client reset: path_local = %1, fresh_file_ident = (ident: %2, salt: %3), "
+                "fresh_server_version = (ident: %4, salt: %5), remote_path = %6, requested_mode = %7, action = %8, "
+                "actual_mode = %9, will_recover = %10, originating_error = %11",
+                db_local.get_path(), fresh_file_ident.ident, fresh_file_ident.salt, fresh_server_version.version,
+                fresh_server_version.salt, db_remote.get_path(), reset_config.mode, reset_config.action, actual_mode,
+                recover_local_changes, reset_config.error);
+
     TransactionRef tr_remote;
     std::vector<client_reset::RecoveredChange> recovered;
     if (recover_local_changes) {
@@ -534,7 +535,7 @@ bool perform_client_reset_diff(DB& db_local, sync::ClientReset& reset_config, sy
 
     // now that the state of the fresh and local Realms are identical,
     // reset the local sync history and steal the fresh Realm's ident
-    history_local.set_history_adjustments(logger, wt_local->get_version(), client_file_ident, fresh_server_version,
+    history_local.set_history_adjustments(logger, wt_local->get_version(), fresh_file_ident, fresh_server_version,
                                           recovered);
 
     int64_t subscription_version = 0;

src/realm/sync/noinst/client_reset.hpp

@@ -72,9 +72,9 @@ ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResy
 // Realm is removed.
 // If the fresh path is provided, the local Realm is changed such that its state is equal
 // to the fresh Realm. Then the local Realm will have its client file ident set to
-// 'client_file_ident'
-bool perform_client_reset_diff(DB& db, sync::ClientReset& reset_config, sync::SaltedFileIdent client_file_ident,
-                               util::Logger& logger, sync::SubscriptionStore* sub_store,
+// the file ident from the fresh realm
+bool perform_client_reset_diff(DB& db, sync::ClientReset& reset_config, util::Logger& logger,
+                               sync::SubscriptionStore* sub_store,
                                util::FunctionRef<void(int64_t)> on_flx_version_complete);
 
 } // namespace _impl::client_reset

src/realm/sync/noinst/client_reset_operation.cpp

@@ -52,8 +52,7 @@ bool is_fresh_path(const std::string& path)
 }
 
 bool perform_client_reset(util::Logger& logger, DB& db, sync::ClientReset&& reset_config,
-                          sync::SaltedFileIdent new_file_ident, sync::SubscriptionStore* sub_store,
-                          util::FunctionRef<void(int64_t)> on_flx_version)
+                          sync::SubscriptionStore* sub_store, util::FunctionRef<void(int64_t)> on_flx_version)
 {
     REALM_ASSERT(reset_config.mode != ClientResyncMode::Manual);
     REALM_ASSERT(reset_config.fresh_copy);
@@ -98,8 +97,8 @@ bool perform_client_reset(util::Logger& logger, DB& db, sync::ClientReset&& rese
     if (notify_after) {
         previous_state = db.start_frozen(frozen_before_state_version);
     }
-    bool did_recover = client_reset::perform_client_reset_diff(db, reset_config, new_file_ident, logger, sub_store,
-                                                               on_flx_version); // throws
+    bool did_recover =
+        client_reset::perform_client_reset_diff(db, reset_config, logger, sub_store, on_flx_version); // throws
 
     if (notify_after) {
         notify_after(previous_state->get_version_of_current_transaction(), did_recover);

src/realm/sync/noinst/client_reset_operation.hpp

@@ -39,8 +39,7 @@ std::string get_fresh_path_for(const std::string& realm_path);
 bool is_fresh_path(const std::string& realm_path);
 
 bool perform_client_reset(util::Logger& logger, DB& db, sync::ClientReset&& reset_config,
-                          sync::SaltedFileIdent new_file_ident, sync::SubscriptionStore* sub_store,
-                          util::FunctionRef<void(int64_t)> on_flx_version);
+                          sync::SubscriptionStore* sub_store, util::FunctionRef<void(int64_t)> on_flx_version);
 
 } // namespace realm::_impl::client_reset
 

test/object-store/sync/flx_migration.cpp

@@ -537,7 +537,9 @@ TEST_CASE("An interrupted migration or rollback can recover on the next session"
         return [&config, &state, error](std::weak_ptr<SyncSession> weak_session,
                                         const SyncClientHookData& data) mutable {
             auto session = weak_session.lock();
-            REQUIRE(session);
+            if (!session) {
+                return SyncClientHookAction::NoAction;
+            }
 
             if (data.event == SyncClientHookEvent::BindMessageSent &&
                 session->path() == _impl::client_reset::get_fresh_path_for(config.path)) {

test/object-store/sync/flx_sync.cpp

@@ -596,6 +596,24 @@ TEST_CASE("flx: client reset", "[sync][flx][client reset][baas]") {
         ++after_reset_count;
     };
 
+    config_local.sync_config->on_sync_client_event_hook = [](std::weak_ptr<SyncSession> weak_session,
+                                                             const SyncClientHookData& data) {
+        // To prevent the upload cursors from becoming out of sync when the local realm assumes
+        // the client file ident of the fresh realm, UPLOAD messages are not allowed during the
+        // fresh realm download so the server's upload cursor versions start at 0 when the
+        // local realm resumes after the client reset.
+        if (data.event == SyncClientHookEvent::UploadMessageSent) {
+            // If this is an UPLOAD message event, check to see if the fresh realm is being downloaded
+            if (auto session = weak_session.lock()) {
+                // Check for a "fresh" path to determine if this is a client reset fresh download session
+                if (_impl::client_reset::is_fresh_path(session->path())) {
+                    FAIL("UPLOAD messages are not allowed during client reset fresh realm download");
+                }
+            }
+        }
+        return SyncClientHookAction::NoAction;
+    };
+
     SECTION("Recover: offline writes and subscription (single subscription)") {
         config_local.sync_config->client_resync_mode = ClientResyncMode::Recover;
         auto&& [reset_future, reset_handler] = make_client_reset_handler();

test/object-store/util/sync/sync_test_utils.cpp

@@ -434,15 +434,14 @@ struct FakeLocalClientReset : public TestClientReset {
             }
             remote_realm->commit_transaction();
 
-            sync::SaltedFileIdent fake_ident{1, 123456789};
             auto local_db = TestHelper::get_db(local_realm);
             auto logger = util::Logger::get_default_logger();
             sync::ClientReset reset_config{m_mode,
                                            TestHelper::get_db(remote_realm),
                                            {ErrorCodes::SyncClientResetRequired, "Bad client file ident"}};
 
             using _impl::client_reset::perform_client_reset_diff;
-            perform_client_reset_diff(*local_db, reset_config, fake_ident, *logger, nullptr, [](int64_t) {});
+            perform_client_reset_diff(*local_db, reset_config, *logger, nullptr, [](int64_t) {});
 
             remote_realm->close();
             if (m_on_post_reset) {