RCORE-2063 Fix some client resets potentially failing with AutoClientResetFailed if a new client reset condition occurred before the first one completed #7542

michael-wb · 2024-04-03T04:34:07Z

What, How & Why?

If a new client reset occurs with a different action (e.g. rolled back to PBS), the original client reset tracking info (e.g. for migrated to FLX) will be removed and the new client reset will be allowed to continue. (Note: the action and error were originally included in this PR, but moved to a new PR #7649.

Added extra sync client hook events to capture different steps along the way during a client reset - this allows the "Test client migration and rollback with recovery" test to pause the client reset and roll back to PBS while the FLX migration client reset is in progress, effective reproducing the condition that was intermittently failing in the past. This test was previously taking 90+ secs to complete and about a third of this time was waiting for the reconnect timer to expire for the sync session that was active during migration and rollback. Added handle_reconnect() call after migration/rollback to cancel this timer, shaving around 30 secs off this test.

Fixes #7539

☑️ ToDos

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

coveralls-official · 2024-04-03T08:23:02Z

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1147

Details

61 of 66 (92.42%) changed or added relevant lines in 5 files are covered.
84 unchanged lines in 12 files lost coverage.
Overall coverage decreased (-0.008%) to 90.835%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/realm/sync/noinst/client_reset.cpp	34	39	87.18%

Files with Coverage Reduction	New Missed Lines	%
src/realm/array_blobs_big.cpp	2	98.58%
src/realm/array_key.cpp	2	96.55%
src/realm/cluster.cpp	2	75.6%
src/realm/sync/client.cpp	3	91.78%
src/realm/sync/instruction_replication.cpp	3	91.48%
src/realm/sync/noinst/server/server_history.cpp	4	63.51%
test/sync_fixtures.hpp	7	75.56%
src/realm/index_string.cpp	8	84.63%
src/realm/sync/noinst/client_impl_base.cpp	8	82.47%
src/realm/sync/instruction_applier.cpp	12	68.51%

Totals
Change from base Build 2376:	-0.008%
Covered Lines:	214628
Relevant Lines:	236283

💛 - Coveralls

danieltabacaru · 2024-04-03T13:20:37Z

Don't we want to fix the real issue too (clearing tracking the client reset in case of a rollback)?

michael-wb · 2024-04-03T13:23:07Z

oh, yes - the test was originally rolling back during client reset on purpose to see what the issues are and the client reset tracking entry was being left over, causing the original failure.
I'll fix this, too.

michael-wb · 2024-04-19T14:16:58Z

Added client reset error and action tracking to the client reset metadata storage (producing v2) and if the new client reset action is different from the current client reset action, then the older client reset tracking will be removed in favor of the new client reset. Also updated the "Test client migration and rollback with recovery" test to reflect this operation:

Realm.Sync.Client.Reset - Connection[2]: Session[12]: Found a previous recoverable client reset of type: 'Recover' for 'MigrateToFLX' at: 2024-04-19 14:04:15.848902000
Realm.Sync.Client.Reset - Connection[2]: Session[12]: Originating client reset error: WrongSyncType: Client connected using partition-based sync when app has been migrated to flexible sync
Realm.Sync.Client.Reset - Connection[2]: Session[12]: New recoverable client reset of type: 'Recover' for 'RevertToPBS' is incompatible - clearing previous reset
Realm.Sync.Client.Reset - Connection[2]: Session[12]: New client reset error: WrongSyncType: Client connected using flexible sync when app has been reverted back to partition-based sync

jbreams · 2024-04-29T20:49:07Z

Can we break this PR up into the three separate PRs? 1) adding the extra info to the client reset tracker to show what the original error was 2) changing it so that if a new client reset of a different type starts it will be allowed to continue 3) any changes to fix up the test ?

michael-wb · 2024-04-29T20:51:53Z

Can we break this PR up into the three separate PRs? 1) adding the extra info to the client reset tracker to show what the original error was 2) changing it so that if a new client reset of a different type starts it will be allowed to continue 3) any changes to fix up the test ?

Sure - I can do that, although it may be better to create two PRs - one for the additions and another that allows a different type to continue + updates to migration test

…et cycles (#7649) * Broke out the client reset error and action storage from PR #7542 * Removed client reset recovery_allowed flag and other updates from review * Updated pending_client_reset store to use the schema metadata tables * Fixed pausing a session does not hold the DB open test * Moved ownership of reset store to SessionWrapper * Fixed migration test crash - need to save client reset error in handle fresh realm downloaded * Updated PendingResetStore to be static functions instead of an initialized object; updates from review * Make ClientReset::error no longer optional; fixed subscriptions tests * updated changelog after release * updates from review

…ration-test

jbreams · 2024-06-03T19:17:58Z

CHANGELOG.md

@@ -17,7 +17,7 @@
 -----------

 ### Internals
-* None.
+* Fix client reset failure during sync migration due to previous incomplete client reset. ([PR #7542](https://github.com/realm/realm-core/pull/7542), since v13.11.0)


I think this actually belongs under the Fixed section rather than the Internals section.

Also would be helpful to describe how the client reset failure would manifest. Like what would the error message be for users?

…ration-test

danieltabacaru

Looks good. Should we rename the PR to mention the bug being fixed (and not the test)?

danieltabacaru · 2024-05-31T14:51:28Z

src/realm/sync/config.hpp

@@ -132,6 +132,11 @@ enum class SyncClientHookEvent {
    SessionActivating,
    SessionSuspended,
    BindMessageSent,
+    IdentMessageReceived,


we don't seem to use (yet) most of these events, so maybe we should add them when they're needed

Removed the unused events

michael-wb added the no-changelog label Apr 3, 2024

michael-wb self-assigned this Apr 3, 2024

cla-bot bot added the cla: yes label Apr 3, 2024

michael-wb requested review from danieltabacaru and jbreams April 3, 2024 13:17

michael-wb marked this pull request as draft April 3, 2024 13:23

danieltabacaru mentioned this pull request Apr 11, 2024

RCORE-2080: Use the correct target conditional #7579

Merged

michael-wb linked an issue Apr 19, 2024 that may be closed by this pull request

Track client reset reason in table that detects client reset cycles #6154

Closed

michael-wb removed the no-changelog label Apr 19, 2024

michael-wb marked this pull request as ready for review April 19, 2024 15:47

michael-wb pushed a commit that referenced this pull request Apr 30, 2024

Broke out the client reset error and action storage from PR #7542

805add6

michael-wb mentioned this pull request Apr 30, 2024

RCORE-1386 Track client reset reason in table that detects client reset cycles #7649

Merged

2 tasks

michael-wb marked this pull request as draft May 30, 2024 18:52

Updated PR 7542 with changes from master (and PR 7649)

807aa83

michael-wb force-pushed the mwb/fix-migration-test branch from 63bce84 to 807aa83 Compare May 31, 2024 03:02

michael-wb removed a link to an issue May 31, 2024

Track client reset reason in table that detects client reset cycles #6154

Closed

Ensure client reset failures get appropriate err info

ef92b32

michael-wb marked this pull request as ready for review May 31, 2024 04:08

Michael Wilkerson-Barker added 3 commits May 31, 2024 10:46

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-mig…

a157ad2

…ration-test

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-mig…

64fdebd

…ration-test

Updated changelog after release

a952a00

jbreams approved these changes Jun 3, 2024

View reviewed changes

jbreams reviewed Jun 3, 2024

View reviewed changes

Michael Wilkerson-Barker added 3 commits June 3, 2024 15:21

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-mig…

274e5b9

…ration-test

Updated changelog

a7e929a

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-mig…

518e499

…ration-test

danieltabacaru approved these changes Jun 4, 2024

View reviewed changes

michael-wb changed the title ~~RCORE-2063 Fix flakey 'Test client migration and rollback with recovery' test~~ RCORE-2063 Fix some client resets potentially failing with AutoClientResetFailed if a new client reset condition occurred before the first one completed Jun 4, 2024

Removed unused SyncClientHookEvent entries

5930822

michael-wb merged commit ab1361f into master Jun 4, 2024
38 checks passed

michael-wb deleted the mwb/fix-migration-test branch June 4, 2024 14:21

github-actions bot mentioned this pull request Jun 7, 2024

Prepare for 14.10.0 #7788

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCORE-2063 Fix some client resets potentially failing with AutoClientResetFailed if a new client reset condition occurred before the first one completed #7542

RCORE-2063 Fix some client resets potentially failing with AutoClientResetFailed if a new client reset condition occurred before the first one completed #7542

michael-wb commented Apr 3, 2024 •

edited

Loading

coveralls-official bot commented Apr 3, 2024 •

edited

Loading

danieltabacaru commented Apr 3, 2024

michael-wb commented Apr 3, 2024

michael-wb commented Apr 19, 2024 •

edited

Loading

jbreams commented Apr 29, 2024

michael-wb commented Apr 29, 2024 •

edited

Loading

jbreams Jun 3, 2024

jbreams Jun 3, 2024

michael-wb Jun 3, 2024

danieltabacaru left a comment

danieltabacaru May 31, 2024

michael-wb Jun 4, 2024

RCORE-2063 Fix some client resets potentially failing with AutoClientResetFailed if a new client reset condition occurred before the first one completed #7542

RCORE-2063 Fix some client resets potentially failing with AutoClientResetFailed if a new client reset condition occurred before the first one completed #7542

Conversation

michael-wb commented Apr 3, 2024 • edited Loading

What, How & Why?

☑️ ToDos

coveralls-official bot commented Apr 3, 2024 • edited Loading

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1147

Details

💛 - Coveralls

danieltabacaru commented Apr 3, 2024

michael-wb commented Apr 3, 2024

michael-wb commented Apr 19, 2024 • edited Loading

jbreams commented Apr 29, 2024

michael-wb commented Apr 29, 2024 • edited Loading

jbreams Jun 3, 2024

Choose a reason for hiding this comment

jbreams Jun 3, 2024

Choose a reason for hiding this comment

michael-wb Jun 3, 2024

Choose a reason for hiding this comment

danieltabacaru left a comment

Choose a reason for hiding this comment

danieltabacaru May 31, 2024

Choose a reason for hiding this comment

michael-wb Jun 4, 2024

Choose a reason for hiding this comment

michael-wb commented Apr 3, 2024 •

edited

Loading

coveralls-official bot commented Apr 3, 2024 •

edited

Loading

michael-wb commented Apr 19, 2024 •

edited

Loading

michael-wb commented Apr 29, 2024 •

edited

Loading