From cf58a7e0483e3c72d546d99578dc0cd232f0ffaf Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Thu, 25 May 2023 10:05:14 +0200 Subject: [PATCH 1/5] Fix crash when opening FLX realm after client reset failure --- src/realm/object-store/sync/sync_session.cpp | 6 ++-- src/realm/sync/subscriptions.cpp | 2 ++ test/object-store/sync/flx_sync.cpp | 31 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp index 1a339154065..78254495143 100644 --- a/src/realm/object-store/sync/sync_session.cpp +++ b/src/realm/object-store/sync/sync_session.cpp @@ -579,12 +579,12 @@ void SyncSession::handle_fresh_realm_downloaded(DBRef db, Status status, lock.unlock(); if (auto local_subs_store = get_flx_subscription_store()) { // In DiscardLocal mode, only the active subscription set is preserved - // this means that we have to remove all other subscriptions including later + // This means that we have to remove all other subscriptions including later // versioned ones. auto mut_sub = local_subs_store->get_active().make_mutable_copy(); + // Mark the new subscription set Complete because there must always exist an active subscription set. + mut_sub.update_state(sync::SubscriptionSet::State::Complete); local_subs_store->supercede_all_except(mut_sub); - mut_sub.update_state(sync::SubscriptionSet::State::Error, - util::make_optional(status.reason())); std::move(mut_sub).commit(); } const bool try_again = false; diff --git a/src/realm/sync/subscriptions.cpp b/src/realm/sync/subscriptions.cpp index a412810dd73..1d598634f3a 100644 --- a/src/realm/sync/subscriptions.cpp +++ b/src/realm/sync/subscriptions.cpp @@ -903,6 +903,8 @@ void SubscriptionStore::supercede_prior_to(TransactionRef tr, int64_t version_id void SubscriptionStore::supercede_all_except(MutableSubscriptionSet& mut_sub) const { + // 'mut_sub' can only supersede the other subscription sets if it is in Complete state. + REALM_ASSERT_EX(mut_sub.state() == SubscriptionSet::State::Complete, mut_sub.state()); auto version_to_keep = mut_sub.version(); supercede_prior_to(mut_sub.m_tr, version_to_keep); diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp index bd6afe7b0ed..e19782f1c45 100644 --- a/test/object-store/sync/flx_sync.cpp +++ b/test/object-store/sync/flx_sync.cpp @@ -40,6 +40,7 @@ #include "realm/sync/client_base.hpp" #include "realm/sync/config.hpp" #include "realm/sync/noinst/client_history_impl.hpp" +#include #include "realm/sync/noinst/pending_bootstrap_store.hpp" #include "realm/sync/noinst/server/access_token.hpp" #include "realm/sync/protocol.hpp" @@ -811,6 +812,36 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { }) ->run(); } + + SECTION("DiscardLocal: open realm after client reset failure") { + config_local.sync_config->client_resync_mode = ClientResyncMode::DiscardLocal; + auto&& [error_future, error_handler] = make_error_handler(); + config_local.sync_config->error_handler = error_handler; + + std::string fresh_path = realm::_impl::ClientResetOperation::get_fresh_path_for(config_local.path); + // create a non-empty directory that we'll fail to delete + util::make_dir(fresh_path); + util::File(util::File::resolve("file", fresh_path), util::File::mode_Write); + + auto test_reset = reset_utils::make_baas_flx_client_reset(config_local, config_remote, harness.session()); + test_reset->run(); + + // Client reset fails due to sync client not being able to create the fresh realm. + auto sync_error = wait_for_future(std::move(error_future)).get(); + CHECK(sync_error.get_system_error() == sync::make_error_code(sync::ClientError::auto_client_reset_failure)); + + // Open the realm again. This should not crash. + { + auto config_copy = config_local; + auto&& [err_future, err_handler] = make_error_handler(); + config_local.sync_config->error_handler = err_handler; + + auto realm_post_reset = Realm::get_shared_realm(config_copy); + auto sync_error = wait_for_future(std::move(err_future)).get(); + CHECK(sync_error.get_system_error() == + sync::make_error_code(sync::ClientError::auto_client_reset_failure)); + } + } } TEST_CASE("flx: creating an object on a class with no subscription throws", "[sync][flx][app]") { From 3bce4a0dd7769a375b7cb0f4c19575f042dd3e5c Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Thu, 25 May 2023 10:30:13 +0200 Subject: [PATCH 2/5] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c561c37a95b..26169bc3076 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * Access token refresh for websockets was not updating the location metadata ([#6630](https://github.com/realm/realm-core/issues/6630), since v13.9.3) * Fix several UBSan failures which did not appear to result in functional bugs ([#6649](https://github.com/realm/realm-core/pull/6649)). * Fix an out-of-bounds read in sectioned results when sectioned are removed by modifying all objects in that section to no longer appear in that section ([#6649](https://github.com/realm/realm-core/pull/6649), since v13.12.0) +* Fixed a potential crash when opening the realm after failing to download a fresh FLX realm during an automatic client reset ([#6494](https://github.com/realm/realm-core/issues/6494), since v12.3.0) ### Breaking changes * None. From dfc3b65e96e5e3122a8d2612714fd6837543e46a Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Mon, 29 May 2023 12:13:37 +0200 Subject: [PATCH 3/5] Don't superceed pending subscriptions in case of a client reset failure --- src/realm/object-store/sync/sync_session.cpp | 11 +------ src/realm/sync/subscriptions.cpp | 31 -------------------- src/realm/sync/subscriptions.hpp | 4 --- 3 files changed, 1 insertion(+), 45 deletions(-) diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp index 78254495143..6efeb2449f1 100644 --- a/src/realm/object-store/sync/sync_session.cpp +++ b/src/realm/object-store/sync/sync_session.cpp @@ -577,16 +577,7 @@ void SyncSession::handle_fresh_realm_downloaded(DBRef db, Status status, return; } lock.unlock(); - if (auto local_subs_store = get_flx_subscription_store()) { - // In DiscardLocal mode, only the active subscription set is preserved - // This means that we have to remove all other subscriptions including later - // versioned ones. - auto mut_sub = local_subs_store->get_active().make_mutable_copy(); - // Mark the new subscription set Complete because there must always exist an active subscription set. - mut_sub.update_state(sync::SubscriptionSet::State::Complete); - local_subs_store->supercede_all_except(mut_sub); - std::move(mut_sub).commit(); - } + const bool try_again = false; sync::SessionErrorInfo synthetic( make_error_code(sync::Client::Error::auto_client_reset_failure), diff --git a/src/realm/sync/subscriptions.cpp b/src/realm/sync/subscriptions.cpp index 1d598634f3a..f8b045ad6bc 100644 --- a/src/realm/sync/subscriptions.cpp +++ b/src/realm/sync/subscriptions.cpp @@ -901,37 +901,6 @@ void SubscriptionStore::supercede_prior_to(TransactionRef tr, int64_t version_id remove_query.remove(); } -void SubscriptionStore::supercede_all_except(MutableSubscriptionSet& mut_sub) const -{ - // 'mut_sub' can only supersede the other subscription sets if it is in Complete state. - REALM_ASSERT_EX(mut_sub.state() == SubscriptionSet::State::Complete, mut_sub.state()); - auto version_to_keep = mut_sub.version(); - supercede_prior_to(mut_sub.m_tr, version_to_keep); - - std::list to_finish; - std::unique_lock lk(m_pending_notifications_mutex); - m_pending_notifications_cv.wait(lk, [&] { - return m_outstanding_requests == 0; - }); - for (auto it = m_pending_notifications.begin(); it != m_pending_notifications.end();) { - if (it->version != version_to_keep) { - to_finish.splice(to_finish.end(), m_pending_notifications, it++); - } - else { - ++it; - } - } - - REALM_ASSERT_EX(version_to_keep >= m_min_outstanding_version, version_to_keep, m_min_outstanding_version); - m_min_outstanding_version = version_to_keep; - - lk.unlock(); - - for (auto& req : to_finish) { - req.promise.emplace_value(SubscriptionSet::State::Superseded); - } -} - MutableSubscriptionSet SubscriptionStore::make_mutable_copy(const SubscriptionSet& set) const { auto new_tr = m_db->start_write(); diff --git a/src/realm/sync/subscriptions.hpp b/src/realm/sync/subscriptions.hpp index a2d4247056d..44f35ab99d0 100644 --- a/src/realm/sync/subscriptions.hpp +++ b/src/realm/sync/subscriptions.hpp @@ -330,10 +330,6 @@ class SubscriptionStore : public std::enable_shared_from_this // version ID. If there is no SubscriptionSet with that version ID, this throws KeyNotFound. SubscriptionSet get_by_version(int64_t version_id) const; - // Fulfill all previous subscriptions by superceding them. This does not - // affect the mutable subscription identified by the parameter. - void supercede_all_except(MutableSubscriptionSet& mut_sub) const; - // Returns true if there have been commits to the DB since the given version bool would_refresh(DB::version_type version) const noexcept; From 31e4a812e11635d4014f78c249483bbb2b1fe81b Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Mon, 29 May 2023 12:14:01 +0200 Subject: [PATCH 4/5] Add test --- test/object-store/sync/flx_sync.cpp | 55 +++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp index e19782f1c45..b39a2eaf165 100644 --- a/test/object-store/sync/flx_sync.cpp +++ b/test/object-store/sync/flx_sync.cpp @@ -283,6 +283,11 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { {"list_of_ints_field", PropertyType::Int | PropertyType::Array}, {"sum_of_list_field", PropertyType::Int}, }}, + {"TopLevel2", + { + {"_id", PropertyType::ObjectId, Property::IsPrimary{true}}, + {"queryable_str_field", PropertyType::String | PropertyType::Nullable}, + }}, }; // some of these tests make additive schema changes which is only allowed in dev mode @@ -433,6 +438,56 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { ->run(); } + SECTION("Recover: subscription and offline writes after client reset failure") { + config_local.sync_config->client_resync_mode = ClientResyncMode::Recover; + auto&& [error_future, error_handler] = make_error_handler(); + config_local.sync_config->error_handler = error_handler; + + std::string fresh_path = realm::_impl::ClientResetOperation::get_fresh_path_for(config_local.path); + // create a non-empty directory that we'll fail to delete + util::make_dir(fresh_path); + util::File(util::File::resolve("file", fresh_path), util::File::mode_Write); + + auto test_reset = reset_utils::make_baas_flx_client_reset(config_local, config_remote, harness.session()); + test_reset + ->make_local_changes([&](SharedRealm local_realm) { + auto mut_sub = local_realm->get_latest_subscription_set().make_mutable_copy(); + auto table = local_realm->read_group().get_table("class_TopLevel2"); + mut_sub.insert_or_assign(Query(table)); + mut_sub.commit(); + + CppContext c(local_realm); + local_realm->begin_transaction(); + Object::create(c, local_realm, "TopLevel2", + std::any(AnyDict{{"_id"s, ObjectId::gen()}, {"queryable_str_field"s, "foo"s}})); + local_realm->commit_transaction(); + }) + ->on_post_reset([](SharedRealm local_realm) { + // Verify offline subscription was not removed. + auto subs = local_realm->get_latest_subscription_set(); + auto table = local_realm->read_group().get_table("class_TopLevel2"); + REQUIRE(subs.find(Query(table))); + }) + ->run(); + + // Remove the folder preventing the completion of a client reset. + util::try_remove_dir_recursive(fresh_path); + + auto config_copy = config_local; + config_local.sync_config->error_handler = nullptr; + + // Attempt to open the realm again. + // This time the client reset succeesds and the offline subscription and writes are recovered. + auto realm = Realm::get_shared_realm(config_copy); + wait_for_download(*realm); + wait_for_upload(*realm); + + auto table = realm->read_group().get_table("class_TopLevel2"); + auto str_col = table->get_column_key("queryable_str_field"); + REQUIRE(table->size() == 1); + REQUIRE(table->get_object(0).get(str_col) == "foo"); + } + SECTION("Recover: offline writes and subscriptions (multiple subscriptions)") { config_local.sync_config->client_resync_mode = ClientResyncMode::Recover; auto&& [reset_future, reset_handler] = make_client_reset_handler(); From 538fdab717f45905077fbed0e91dbf9334a0c7d8 Mon Sep 17 00:00:00 2001 From: Daniel Tabacaru Date: Mon, 29 May 2023 19:58:43 +0200 Subject: [PATCH 5/5] Changes after code review --- test/object-store/sync/flx_sync.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp index b39a2eaf165..ef2afea00ec 100644 --- a/test/object-store/sync/flx_sync.cpp +++ b/test/object-store/sync/flx_sync.cpp @@ -475,12 +475,14 @@ TEST_CASE("flx: client reset", "[sync][flx][app][client reset]") { auto config_copy = config_local; config_local.sync_config->error_handler = nullptr; + auto&& [reset_future, reset_handler] = make_client_reset_handler(); + config_copy.sync_config->notify_after_client_reset = reset_handler; // Attempt to open the realm again. - // This time the client reset succeesds and the offline subscription and writes are recovered. + // This time the client reset succeeds and the offline subscription and writes are recovered. auto realm = Realm::get_shared_realm(config_copy); - wait_for_download(*realm); - wait_for_upload(*realm); + ClientResyncMode mode = reset_future.get(); + REQUIRE(mode == ClientResyncMode::Recover); auto table = realm->read_group().get_table("class_TopLevel2"); auto str_col = table->get_column_key("queryable_str_field");