Client reset with recovery fixes

CHANGELOG.md

@@ -9,6 +9,9 @@
 * `Set::assign_intersection()` on `Set<StringData>`, `Set<BinaryData>`, and `Set<Mixed>` containing string or binary would cause a use-after-free if a set was intersected with itself ([PR #7144](https://github.com/realm/realm-core/pull/7144), since v10.0.0).
 * Set algebra on `Set<StringData>` and `Set<BinaryData>` gave incorrect results when used on platforms where `char` is signed ([#7135](https://github.com/realm/realm-core/issues/7135), since v13.23.3).
 * Errors encountered while reapplying local changes for client reset recovery on partition-based sync Realms would result in the client reset attempt not being recorded, possibly resulting in an endless loop of attempting and failing to automatically recover the client reset. Flexible sync and errors from the server after completing the local recovery were handled correctly ([PR #7149](https://github.com/realm/realm-core/pull/7149), since v10.2.0).
+* During a client reset with recovery when recovering a move or set operation on a `LnkLst` or `Lst<Mixed>` that operated on indices that were not also added in the recovery, links to an object which had been deleted by another client while offline would be recreated by the recovering client. But the objects of these links would only have the primary key populated and all other fields would be default values. Now, instead of creating these zombie objects, the lists being recovered skip such deleted links. ([#7112](https://github.com/realm/realm-core/issues/7112) since the beginning of client reset with recovery in v11.16.0)
+* During a client reset recovery a Set of links could be missing items, or an exception could be thrown that prevents recovery ex: "Requested index 1 calling get() on set 'source.collection' when max is 0" ([#7112](https://github.com/realm/realm-core/issues/7112), since the beginning of client reset with recovery in v11.16.0)
+* Calling `sort()` or `distinct()` on a `LnkSet` that had unresolved links in it would produce duplicate indices.
 
 ### Breaking changes
 * None.

src/realm/collection.cpp

@@ -44,6 +44,8 @@ size_t real2virtual(const std::vector<size_t>& vec, size_t ndx) noexcept
 {
     // Subtract the number of tombstones below ndx.
     auto it = std::lower_bound(vec.begin(), vec.end(), ndx);
+    // A tombstone index has no virtual mapping. This is an error.
+    REALM_ASSERT_DEBUG_EX(it == vec.end() || *it != ndx, ndx, vec.size());
     auto n = it - vec.begin();
     return ndx - n;
 }

src/realm/collection.hpp

@@ -601,6 +601,11 @@ class ObjCollectionBase : public Interface, public _impl::ObjListProxy {
         return _impl::real2virtual(m_unresolved, ndx);
     }
 
+    bool real_is_unresolved(size_t ndx) const noexcept
+    {
+        return std::find(m_unresolved.begin(), m_unresolved.end(), ndx) != m_unresolved.end();
+    }
+
     /// Rebuild the list of tombstones if there is a possibility that it has
     /// changed.
     ///

src/realm/object_converter.cpp

@@ -41,9 +41,9 @@ void InterRealmValueConverter::copy_list(const Obj& src_obj, Obj& dst_obj, bool*
     LstBasePtr dst = dst_obj.get_listbase_ptr(m_dst_col);
 
     bool updated = false;
-    size_t len_src = src->size();
-    size_t len_dst = dst->size();
-    size_t len_min = std::min(len_src, len_dst);
+    const size_t len_src = src->size();
+    const size_t len_dst_orig = dst->size();
+    size_t len_min = std::min(len_src, len_dst_orig);
 
     size_t ndx = 0;
     size_t suffix_len = 0;
@@ -52,25 +52,47 @@ void InterRealmValueConverter::copy_list(const Obj& src_obj, Obj& dst_obj, bool*
         ndx++;
     }
 
+    if (ndx == len_src && len_src == len_dst_orig) {
+        // all are equal, early out
+        if (update_out) {
+            *update_out = false;
+        }
+        return;
+    }
+
     size_t suffix_len_max = len_min - ndx;
 
     while (suffix_len < suffix_len_max &&
-           cmp_src_to_dst(src->get_any(len_src - 1 - suffix_len), dst->get_any(len_dst - 1 - suffix_len), nullptr,
-                          update_out) == 0) {
+           cmp_src_to_dst(src->get_any(len_src - 1 - suffix_len), dst->get_any(len_dst_orig - 1 - suffix_len),
+                          nullptr, update_out) == 0) {
         suffix_len++;
     }
 
     len_min -= (ndx + suffix_len);
 
+    auto dst_as_link_list = dynamic_cast<LnkLst*>(dst.get());
+    auto dst_as_lst_mixed = dynamic_cast<Lst<Mixed>*>(dst.get());
+    auto is_link_to_deleted_object = [&](const Mixed& src_value, const Mixed& converted_value) -> bool {
+        return (dst_as_link_list && converted_value.is_null()) ||
+               (dst_as_lst_mixed && converted_value.is_null() && src_value.is_type(type_TypedLink));
+    };
+
+    std::vector<size_t> dst_to_erase;
     for (size_t i = 0; i < len_min; i++) {
         InterRealmValueConverter::ConversionResult converted_src;
-        if (cmp_src_to_dst(src->get_any(ndx), dst->get_any(ndx), &converted_src, update_out)) {
+        const Mixed src_value = src->get_any(ndx);
+        if (cmp_src_to_dst(src_value, dst->get_any(ndx), &converted_src, update_out)) {
             if (converted_src.requires_new_embedded_object) {
-                auto lnklist = dynamic_cast<LnkLst*>(dst.get());
-                REALM_ASSERT(lnklist); // this is the only type of list that supports embedded objects
-                Obj embedded = lnklist->create_and_set_linked_object(ndx);
+                REALM_ASSERT(dst_as_link_list); // this is the only type of list that supports embedded objects
+                Obj embedded = dst_as_link_list->create_and_set_linked_object(ndx);
                 track_new_embedded(converted_src.src_embedded_to_check, embedded);
             }
+            else if (is_link_to_deleted_object(src_value, converted_src.converted_value)) {
+                // this can happen when the source linked list points to an object
+                // which has been deleted in the dest Realm. Lists do not support
+                // setting an element to null, so it must be deleted later.
+                dst_to_erase.push_back(ndx);
+            }
             else {
                 dst->set_any(ndx, converted_src.converted_value);
             }
@@ -80,29 +102,36 @@ void InterRealmValueConverter::copy_list(const Obj& src_obj, Obj& dst_obj, bool*
     }
 
     // New elements must be inserted in dst.
-    while (len_dst < len_src) {
+    while (ndx < len_src - suffix_len) {
         InterRealmValueConverter::ConversionResult converted_src;
-        cmp_src_to_dst(src->get_any(ndx), Mixed{}, &converted_src, update_out);
+        const Mixed src_value = src->get_any(ndx);
+        cmp_src_to_dst(src_value, Mixed{}, &converted_src, update_out);
+        size_t dst_ndx_to_insert = dst->size() - suffix_len;
         if (converted_src.requires_new_embedded_object) {
-            auto lnklist = dynamic_cast<LnkLst*>(dst.get());
-            REALM_ASSERT(lnklist); // this is the only type of list that supports embedded objects
-            Obj embedded = lnklist->create_and_insert_linked_object(ndx);
+            REALM_ASSERT(dst_as_link_list); // this is the only type of list that supports embedded objects
+            Obj embedded = dst_as_link_list->create_and_insert_linked_object(dst_ndx_to_insert);
             track_new_embedded(converted_src.src_embedded_to_check, embedded);
         }
+        else if (is_link_to_deleted_object(src_value, converted_src.converted_value)) {
+            // ignore trying to insert a link to a object which no longer exists
+        }
         else {
-            dst->insert_any(ndx, converted_src.converted_value);
+            dst->insert_any(dst_ndx_to_insert, converted_src.converted_value);
         }
-        len_dst++;
         ndx++;
         updated = true;
     }
     // Excess elements must be removed from ll_dst.
-    if (len_dst > len_src) {
-        dst->remove(len_src - suffix_len, len_dst - suffix_len);
+    if (dst->size() > len_src) {
+        dst->remove(len_src - suffix_len, dst->size() - suffix_len);
         updated = true;
     }
 
-    REALM_ASSERT(dst->size() == len_src);
+    while (dst_to_erase.size()) {
+        size_t ndx_to_remove = dst_to_erase.back();
+        dst_as_link_list ? dst_as_link_list->remove(ndx_to_remove) : dst_as_lst_mixed->remove(ndx_to_remove);
+        dst_to_erase.pop_back();
+    }
     if (updated && update_out) {
         *update_out = updated;
     }
@@ -373,7 +402,8 @@ int InterRealmValueConverter::cmp_src_to_dst(Mixed src, Mixed dst, ConversionRes
                 // in different Realms we create a new object
                 if (m_opposite_of_src->get_primary_key_column()) {
                     Mixed src_link_pk = m_opposite_of_src->get_primary_key(src_link_key);
-                    dst_link = m_opposite_of_dst->create_object_with_primary_key(src_link_pk, did_update_out);
+                    dst_link =
+                        m_opposite_of_dst->get_object_with_primary_key(src_link_pk); // returns ObjKey{} if not found
                 }
                 else {
                     dst_link = m_opposite_of_dst->create_object();
@@ -404,8 +434,9 @@ int InterRealmValueConverter::cmp_src_to_dst(Mixed src, Mixed dst, ConversionRes
             // regular table, convert by pk
             if (src_link_table->get_primary_key_column()) {
                 Mixed src_pk = src_link_table->get_primary_key(src_link.get_obj_key());
-                Obj dst_link = dst_link_table->create_object_with_primary_key(src_pk, did_update_out);
-                converted_src = ObjLink{dst_link_table->get_key(), dst_link.get_key()};
+                if (Obj dst_link = dst_link_table->get_object_with_primary_key(src_pk)) {
+                    converted_src = ObjLink{dst_link_table->get_key(), dst_link.get_key()};
+                }
             }
             else if (src_link_table == dst_link_table) {
                 // no pk, but this is the same Realm, so convert by ObjKey

src/realm/set.hpp

@@ -932,6 +932,14 @@ inline void LnkSet::sort(std::vector<size_t>& indices, bool ascending) const
 
     m_set.sort(indices, ascending);
 
+    if (has_unresolved()) {
+        indices.erase(std::remove_if(indices.begin(), indices.end(),
+                                     [&](size_t ndx) {
+                                         return real_is_unresolved(ndx);
+                                     }),
+                      indices.end());
+    }
+
     // Map the output indices to virtual indices.
     std::transform(indices.begin(), indices.end(), indices.begin(), [this](size_t ndx) {
         return real2virtual(ndx);
@@ -949,6 +957,14 @@ inline void LnkSet::distinct(std::vector<size_t>& indices, util::Optional<bool>
 
     m_set.distinct(indices, sort_order);
 
+    if (has_unresolved()) {
+        indices.erase(std::remove_if(indices.begin(), indices.end(),
+                                     [&](size_t ndx) {
+                                         return real_is_unresolved(ndx);
+                                     }),
+                      indices.end());
+    }
+
     // Map the output indices to virtual indices.
     std::transform(indices.begin(), indices.end(), indices.begin(), [this](size_t ndx) {
         return real2virtual(ndx);

src/realm/sync/noinst/client_reset.cpp

@@ -366,9 +366,9 @@ void transfer_group(const Transaction& group_src, Transaction& group_dst, util::
     }
 
     converters::EmbeddedObjectConverter embedded_tracker;
-    // Now src and dst have identical schemas and no extraneous objects from dst.
-    // There may be missing object from src and the values of existing objects may
-    // still differ. Diff all the values and create missing objects on the fly.
+    // Now src and dst have identical schemas and all the top level objects are created.
+    // What is left to do is to diff all properties of the existing objects.
+    // Embedded objects are created on the fly.
     for (auto table_key : group_src.get_table_keys()) {
         if (should_skip_table(group_src, table_key))
             continue;

test/object-store/collection_fixtures.hpp

@@ -454,6 +454,7 @@ struct LinkedCollectionBase {
     virtual void clear_collection(Obj obj) = 0;
     virtual std::vector<Obj> get_links(Obj obj) = 0;
     virtual void move(Obj, size_t, size_t) {}
+    virtual void insert(Obj, size_t, ObjLink) {}
     bool remove_linked_object(Obj obj, ObjLink to)
     {
         auto links = get_links(obj);
@@ -518,6 +519,12 @@ struct ListOfObjects : public LinkedCollectionBase {
         auto coll = source.get_linklist(col);
         coll.move(from, to);
     }
+    void insert(Obj source, size_t ndx, ObjLink to) override
+    {
+        ColKey col = get_link_col_key(source.get_table());
+        auto coll = source.get_linklist(col);
+        coll.insert(ndx, to.get_obj_key());
+    }
     size_t size_of_collection(Obj obj)
     {
         ColKey col = get_link_col_key(obj.get_table());
@@ -587,6 +594,16 @@ struct ListOfMixedLinks : public LinkedCollectionBase {
         ColKey col = get_link_col_key(obj.get_table());
         obj.get_list<Mixed>(col).move(from, to);
     }
+    void insert(Obj from, size_t ndx, ObjLink to) override
+    {
+        ColKey col = get_link_col_key(from.get_table());
+        from.get_list<Mixed>(col).insert(ndx, to);
+        // When adding dynamic links through a mixed value, the relationship map needs to be dynamically updated.
+        // In practice, this is triggered by the addition of backlink columns to any table.
+        if (m_relation_updater) {
+            m_relation_updater();
+        }
+    }
 
     size_t count_unresolved_links(Obj obj)
     {