Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix LnkLst propagation of context_flag when replacing the root during erase #7414

Merged
merged 2 commits into from Mar 6, 2024

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Mar 5, 2024

Jonathan reported a failure in a new evergreen runner that was running the client reset tests under a REALM_MAX_BPNODE_SIZE=4 configuration. It turns out that we have never run any sync tests with this enabled on CI. The test happened to hit a case of removing an element from a LnkLst that also had an invalidated object in it and this was causing strange failures.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[2024/03/01 06:23:06.284] 3: realm-object-store-tests is a Catch2 v3.5.2 host application.
[2024/03/01 06:23:06.284] 3: Run with -? for options
[2024/03/01 06:23:06.284] 3:
[2024/03/01 06:23:06.284] 3: -------------------------------------------------------------------------------
[2024/03/01 06:23:06.284] 3: client reset collections of links - cf::ListOfObjects
[2024/03/01 06:23:06.284] 3:   local has unresolved links
[2024/03/01 06:23:06.284] 3:   local adds two new links and remote deletes a different dest object
[2024/03/01 06:23:06.284] 3: -------------------------------------------------------------------------------
[2024/03/01 06:23:06.284] 3: /data/mci/789a545b0869856ff4579cad2dba5e38/realm-core/test/object-store/sync/client_reset.cpp:2987
[2024/03/01 06:23:06.284] 3: ...............................................................................
[2024/03/01 06:23:06.284] 3:
[2024/03/01 06:23:06.284] 3: /data/mci/789a545b0869856ff4579cad2dba5e38/realm-core/test/object-store/sync/client_reset.cpp:2756: FAILED:
[2024/03/01 06:23:06.284] 3:   {Unknown expression after the reported line}
[2024/03/01 06:23:06.284] 3: due to unexpected exception with messages:
[2024/03/01 06:23:06.284] 3:   test_mode := DiscardLocal
[2024/03/01 06:23:06.284] 3:   No object with key '-3' in 'class_dest'
[2024/03/01 06:23:06.284] 3:
[2024/03/01 06:23:06.284] 3: Assertion failure: /data/mci/789a545b0869856ff4579cad2dba5e38/realm-core/test/object-store/sync/client_reset.cpp:2756
[2024/03/01 06:23:06.284] 3: 	 from expresion: '{Unknown expression after the reported line}'
[2024/03/01 06:23:06.284] 3: 	 with expansion: '{Unknown expression after the reported line}'
[2024/03/01 06:23:06.284] 3: 	 message: test_mode := DiscardLocal
[2024/03/01 06:23:06.284] 3: 	 message: No object with key '-3' in 'class_dest'

We were using a ref lookup to get the context flag from an array that had just been deleted, so when doing an erase on a LnkLst that triggered a root_replace, the context flag was always false.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@ironage ironage self-assigned this Mar 5, 2024
@cla-bot cla-bot bot added the cla: yes label Mar 5, 2024
Copy link

coveralls-official bot commented Mar 5, 2024

Pull Request Test Coverage Report for Build james.stone_497

Details

  • 37 of 38 (97.37%) changed or added relevant lines in 2 files are covered.
  • 142 unchanged lines in 14 files lost coverage.
  • Overall coverage decreased (-0.01%) to 90.925%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/bplustree.cpp 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
src/realm/index_string.hpp 1 93.68%
src/realm/sync/instruction_replication.cpp 1 90.49%
src/realm/list.cpp 2 88.62%
src/realm/sync/noinst/server/server_history.cpp 2 67.94%
src/realm/uuid.cpp 2 97.01%
src/realm/sync/noinst/client_reset.cpp 4 93.07%
src/realm/alloc_slab.cpp 5 92.93%
src/realm/sync/noinst/client_impl_base.cpp 5 85.59%
src/realm/table.cpp 6 90.7%
src/realm/alloc.cpp 8 76.32%
Totals Coverage Status
Change from base Build 2092: -0.01%
Covered Lines: 238415
Relevant Lines: 262210

💛 - Coveralls

Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!!!

The changes to handle the context flag propagation in this commit
are not actually fixes because the context flag is not used for
any other type than BPTree<ObjKey> (LnkLst), and these
other cases are for split_root() which is only ever called for
BPTree<Int> in the case of backlinks storage which doesn't care
about the context flag. So these are just defensive in case future
code does rely on this.
@ironage
Copy link
Contributor Author

ironage commented Mar 5, 2024

potentially fixes #5405

@ironage ironage merged commit 919a4b1 into master Mar 6, 2024
33 of 37 checks passed
@ironage ironage deleted the js/bp4-client-reset branch March 6, 2024 19:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants