Sync access token refreshes shouldn't extend SyncSession lifetime

[jbreams]

What, How & Why?

This fixes an issue discovered in a HELP ticket. If you open a synchronized realm and it needs to refresh its access token, and that refresh operation doesn't complete before the realm is closed, the lifetime of the sync session will be extended until the refresh is complete. If you open the realm again before that refresh is complete, then you'll have two sync sessions both try to start synchronizing the realm when the refreshes do complete, and that will crash the process with a MultipleSyncAgents exception.

☑️ ToDos

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

[tgoyne]

I think this is pointing at a bigger lifecycle problem because the session being kept alive should result in it being reused by the new Realm, but I don't see any reason why this workaround would cause problems.

[danieltabacaru]

I think this is pointing at a bigger lifecycle problem because the session being kept alive should result in it being reused by the new Realm, but I don't see any reason why this workaround would cause problems.

Yeah, I agree. The fix looks good though.

[jbreams]

I think this fix is probably correct even if we also fixed the lifecycle issue. I guess if we wanted to become inactive and we were waiting for an access token we could become Dying instead of Inactive and then when we came back to re-open the sync session we could revive the session then?

[jbreams]

Maybe the root lifecycle issue is that this line is just wrong

realm-core/src/realm/object-store/sync/sync_session.cpp

Line 1175 in f3303e5

become_inactive(std::move(lock));

we should check what the stop policy is like we do in the Active state and act appropriately.

[jbreams]

I think we should merge this fix before trying larger scale changes to the lifecycle of SyncSession. @danieltabacaru does this lgtm? the test failures are from macos machines failing while invoking cmake 😞 .

[coveralls-official]

Pull Request Test Coverage Report for Build jonathan.reams_3577

Details

• 73 of 73 (100.0%) changed or added relevant lines in 3 files are covered.
• 109 unchanged lines in 18 files lost coverage.
• Overall coverage decreased (-0.02%) to 91.105%

---

Files with Coverage Reduction	New Missed Lines	%
src/realm/sync/network/websocket.cpp	1	72.2%
src/realm/sync/noinst/server/server_history.cpp	1	62.92%
src/realm/util/serializer.cpp	1	90.43%
test/fuzz_tester.hpp	1	57.73%
src/realm/object-store/shared_realm.cpp	2	91.9%
src/realm/query_expression.hpp	3	93.81%
src/realm/sync/client.cpp	3	90.96%
src/realm/sync/noinst/protocol_codec.hpp	3	76.27%
src/realm/unicode.cpp	3	83.83%
src/realm/sync/instruction_applier.cpp	5	67.7%

Totals
Change from base Build 2642:	-0.02%
Covered Lines:	217338
Relevant Lines:	238558

---

💛 - Coveralls

[jbreams]

Only test failure is from an existing build issue.