Fix race in send_upload_message

[danieltabacaru]

What, How & Why?

A race in send_upload_message() causes subscriptions not being sent to the server (and as a side effect a notification on that subscription may never be fulfilled). This can happen when send_upload_message() is called as a result of a commit previous to the one for subscriptions. send_upload_message() then forwards the upload cursor past the version associated with the subscription and so the subscription is never uploaded to the server.

A second bug was uncovered while investigating this issue.

SessionWrapper reads the state of the SubscriptionStore when it is created, but this state can change after a client reset (when then state of existing subscription may change, in addition to new subscriptions being created and old subscriptions being removed). This can cause a crash with realm::KeyNotFound when trying to update a subscription which does not exist anymore.

Fixes #7076, #7090.

☑️ ToDos

• 📝 Changelog update
• [ ] 🚦 Tests (or not relevant)
• [ ] C-API, if public C++ API changed.

[coveralls-official]

Pull Request Test Coverage Report for Build daniel.tabacaru_600

• 20 of 20 (100.0%) changed or added relevant lines in 3 files are covered.
• 96 unchanged lines in 13 files lost coverage.
• Overall coverage decreased (-0.009%) to 91.573%

---

Files with Coverage Reduction	New Missed Lines	%
src/realm/sync/instruction_applier.cpp	2	70.42%
src/realm/sync/network/http.hpp	2	80.87%
src/realm/sync/noinst/server/server_history.cpp	2	67.69%
src/realm/sync/network/network.cpp	3	89.87%
src/realm/util/serializer.cpp	3	90.03%
src/realm/query_expression.hpp	4	93.43%
src/realm/sync/noinst/changeset_index.cpp	4	80.97%
src/realm/sync/noinst/client_reset_operation.cpp	4	87.23%
src/realm/unicode.cpp	4	90.15%
src/realm/util/assert.hpp	4	87.5%

Totals
Change from base Build 1792:	-0.009%
Covered Lines:	230694
Relevant Lines:	251923

---

💛 - Coveralls

[ironage]

I tested this against BAAS version 27f42f55a7944ed7d8ba9fad1854a4b22714cb8d where I had been able to repro the hang in the test "app: flx-sync basic tests" and it looks like it is fixed. Great job tracking down the issue here.
I'd suggest adding a changelog note as it may be affecting customers or SDK tests.

[danieltabacaru]

I'd suggest adding a changelog note as it may be affecting customers or SDK tests.

I'll definitely add changelog entries for both bugs.

[michael-wb]

Could the failure above be related to these changes?

4: Assertion failure: /System/Volumes/Data/data/mci/cf69e96e7aa9ce3fd7f27dd3fa585338/realm-core/test/object-store/sync/flx_migration.cpp:796
4: 	 from expresion: 'active_subs.size() == 2'
4: 	 with expansion: '1 == 2'

https://spruce.mongodb.com/task/realm_core_stable_macos_1100_x64_asan_baas_integration_tests_patch_56a048d9f450445f2d52c7405f3019a533bdaa3f_65402af732f4172339970ae6_23_10_30_22_15_20/tests?execution=0&sortBy=STATUS&sortDir=ASC

[danieltabacaru]

Could the failure above be related to these changes?

Everything looks fine. I looked at the logs and we do send the query, but it seems we don't wait for it to be complete:

[2023/10/30 22:49:07.675] 4: Connection[3]: Session[7]: Sending: QUERY(query_version=3, query_size=88, query="{"Object":"(realm_id = \"migration-test\")","Object2":"(realm_id = \"migration-test\")"}", snapshot_version=18)

My theory is that there is a race and wait_for_download fires before the query is bootstrapped.

If my theory is correct, one way to fix it is:

timed_sleeping_wait_for(
          [&]() -> bool {
              return realm->get_latest_subscription_set().version() == 2;
          },
          std::chrono::seconds(60));
realm->get_latest_subscription_set()
            .get_state_change_notification(sync::SubscriptionSet::State::Complete)
            .get();

There will be no wait in most cases.