Only track pending client resets done by the same core version

If the previous attempt at performing a client reset was done with a different core version then we should retry the client reset as the new version may have fixed a bug that made the previous attempt fail (or may be a downgrade to a version before when the bug was introduced). This also simplifies the tracking as it means that we don't need to be able to read trackers created by different versions. This also means that we can freely change the schema of the table, which this takes advantage of to drop the unused primary key and make the error required, as we never actually stored null and the code reading it would have crashed if it encountered a null error.
realm · Aug 2, 2024 · 1815605 · 1815605
1 parent b10122e
commit 1815605
Show file tree

Hide file tree

Showing 15 changed files with 210 additions and 334 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,7 +2,7 @@
 
 ### Enhancements
 * <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
-* None.
+* Client reset cycle detection now checks if the previous recovery attempt was made by the same core version, and if not attempts recovery again ([PR #7944](https://github.com/realm/realm-core/pull/7944)).
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)

diff --git a/src/realm/sync/CMakeLists.txt b/src/realm/sync/CMakeLists.txt
@@ -76,6 +76,7 @@ set(NOINST_HEADERS
     noinst/integer_codec.hpp
     noinst/migration_store.hpp
     noinst/pending_bootstrap_store.hpp
+    noinst/pending_reset_store.hpp
     noinst/protocol_codec.hpp
     noinst/root_certs.hpp
     noinst/sync_metadata_schema.hpp

diff --git a/src/realm/sync/client.cpp b/src/realm/sync/client.cpp
@@ -1655,7 +1655,7 @@ void SessionWrapper::handle_pending_client_reset_acknowledgement()
 {
     REALM_ASSERT(!m_finalized);
 
-    auto has_pending_reset = PendingResetStore::has_pending_reset(m_db->start_frozen());
+    auto has_pending_reset = PendingResetStore::has_pending_reset(*m_db->start_frozen());
     if (!has_pending_reset) {
         return; // nothing to do
     }
@@ -1678,7 +1678,7 @@ void SessionWrapper::handle_pending_client_reset_acknowledgement()
             logger.debug(util::LogCategory::reset, "Server has acknowledged %1", pending_reset);
 
             auto tr = self->m_db->start_write();
-            auto cur_pending_reset = PendingResetStore::has_pending_reset(tr);
+            auto cur_pending_reset = PendingResetStore::has_pending_reset(*tr);
             if (!cur_pending_reset) {
                 logger.debug(util::LogCategory::reset, "Client reset cycle detection tracker already removed.");
                 return;
@@ -1689,7 +1689,7 @@ void SessionWrapper::handle_pending_client_reset_acknowledgement()
             else {
                 logger.info(util::LogCategory::reset, "Found new %1", cur_pending_reset);
             }
-            PendingResetStore::clear_pending_reset(tr);
+            PendingResetStore::clear_pending_reset(*tr);
             tr->commit();
         });
 }

diff --git a/src/realm/sync/noinst/client_reset.cpp b/src/realm/sync/noinst/client_reset.cpp
@@ -410,18 +410,17 @@ void transfer_group(const Transaction& group_src, Transaction& group_dst, util::
     }
 }
 
-ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResyncMode mode,
-                                      PendingReset::Action action, const std::optional<Status>& error,
-                                      util::Logger& logger)
+static ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResyncMode mode,
+                                             PendingReset::Action action, const Status& error, util::Logger& logger)
 {
-    if (auto previous_reset = sync::PendingResetStore::has_pending_reset(wt_local)) {
+    if (auto previous_reset = sync::PendingResetStore::has_pending_reset(*wt_local)) {
         logger.info(util::LogCategory::reset, "Found a previous %1", *previous_reset);
         if (action != previous_reset->action) {
             // IF a different client reset is being performed, cler the pending client reset and start over.
             logger.info(util::LogCategory::reset,
                         "New '%1' client reset of type: '%2' is incompatible - clearing previous reset", action,
                         mode);
-            sync::PendingResetStore::clear_pending_reset(wt_local);
+            sync::PendingResetStore::clear_pending_reset(*wt_local);
         }
         else {
             switch (previous_reset->mode) {
@@ -444,10 +443,10 @@ ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResy
                                 util::LogCategory::reset,
                                 "A previous '%1' mode reset from %2 downgrades this mode ('%3') to DiscardLocal",
                                 previous_reset->mode, previous_reset->time, mode);
-                            sync::PendingResetStore::clear_pending_reset(wt_local);
+                            sync::PendingResetStore::clear_pending_reset(*wt_local);
                             break;
                         case ClientResyncMode::DiscardLocal:
-                            sync::PendingResetStore::clear_pending_reset(wt_local);
+                            sync::PendingResetStore::clear_pending_reset(*wt_local);
                             // previous mode Recover and this mode is Discard, this is not a cycle yet
                             break;
                         case ClientResyncMode::Manual:
@@ -473,7 +472,7 @@ ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResy
             mode = ClientResyncMode::DiscardLocal;
         }
     }
-    sync::PendingResetStore::track_reset(wt_local, mode, action, error);
+    sync::PendingResetStore::track_reset(*wt_local, mode, action, error);
     // Ensure we save the tracker object even if we encounter an error and roll
     // back the client reset later
     wt_local->commit_and_continue_writing();

diff --git a/src/realm/sync/noinst/client_reset.hpp b/src/realm/sync/noinst/client_reset.hpp
@@ -62,10 +62,6 @@ namespace _impl::client_reset {
 void transfer_group(const Transaction& tr_src, Transaction& tr_dst, util::Logger& logger,
                     bool allow_schema_additions);
 
-ClientResyncMode reset_precheck_guard(const TransactionRef& wt_local, ClientResyncMode mode,
-                                      sync::ProtocolErrorInfo::Action action, const std::optional<Status>& error,
-                                      util::Logger& logger);
-
 // preform_client_reset_diff() takes the Realm performs a client reset on
 // the Realm in 'path_local' given the Realm 'path_fresh' as the source of truth.
 // If the fresh path is not provided, discard mode is assumed and all data in the local

diff --git a/src/realm/sync/noinst/migration_store.cpp b/src/realm/sync/noinst/migration_store.cpp
@@ -60,7 +60,7 @@ bool MigrationStore::load_data(bool read_only)
             throw RuntimeError(ErrorCodes::UnsupportedFileFormatVersion,
                                "Invalid schema version for flexible sync migration store metadata");
         }
-        load_sync_metadata_schema(tr, &internal_tables);
+        load_sync_metadata_schema(*tr, &internal_tables);
     }
     else {
         if (read_only) {
@@ -72,7 +72,7 @@ bool MigrationStore::load_data(bool read_only)
         SyncMetadataSchemaVersions schema_versions(tr);
         // Create the metadata schema and set the version (in the same commit)
         schema_versions.set_version_for(tr, internal_schema_groups::c_flx_migration_store, c_schema_version);
-        create_sync_metadata_schema(tr, &internal_tables);
+        create_sync_metadata_schema(*tr, &internal_tables);
         tr->commit_and_continue_as_read();
     }
     REALM_ASSERT(m_migration_table);

diff --git a/src/realm/sync/noinst/pending_bootstrap_store.cpp b/src/realm/sync/noinst/pending_bootstrap_store.cpp
@@ -109,15 +109,15 @@ PendingBootstrapStore::PendingBootstrapStore(DBRef db, util::Logger& logger,
             throw RuntimeError(ErrorCodes::SchemaVersionMismatch,
                                "Invalid schema version for FLX sync pending bootstrap table group");
         }
-        load_sync_metadata_schema(tr, &internal_tables);
+        load_sync_metadata_schema(*tr, &internal_tables);
     }
     else {
         tr->promote_to_write();
         // Ensure the schema versions table is initialized (may add its own commit)
         SyncMetadataSchemaVersions schema_versions(tr);
         // Create the metadata schema and set the version (in the same commit)
         schema_versions.set_version_for(tr, internal_schema_groups::c_pending_bootstraps, c_schema_version);
-        create_sync_metadata_schema(tr, &internal_tables);
+        create_sync_metadata_schema(*tr, &internal_tables);
         tr->commit_and_continue_as_read();
     }
     REALM_ASSERT(m_table);