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

RCORE-2150 Include originating client reset error message when reporting auto client reset failures #7761

Merged
merged 4 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

### Enhancements
* <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
* None.
* Include the originating client reset error in AutoClientResetFailure errors. ([#7761](https://github.com/realm/realm-core/pull/7761))

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
Expand Down
28 changes: 15 additions & 13 deletions src/realm/object-store/sync/sync_session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,10 @@ void SyncSession::download_fresh_realm(const sync::SessionErrorInfo& error_info)
auto mode = config(&SyncConfig::client_resync_mode);
if (mode == ClientResyncMode::Recover) {
handle_fresh_realm_downloaded(
nullptr, {ErrorCodes::RuntimeError,
"A client reset is required but the server does not permit recovery for this client"});
nullptr,
{ErrorCodes::RuntimeError,
"A client reset is required but the server does not permit recovery for this client"},
error_info);
return;
}
}
Expand Down Expand Up @@ -467,7 +469,7 @@ void SyncSession::download_fresh_realm(const sync::SessionErrorInfo& error_info)
catch (...) {
// Failed to open the fresh path after attempting to delete it, so we
// just can't do automatic recovery.
handle_fresh_realm_downloaded(nullptr, exception_to_status());
handle_fresh_realm_downloaded(nullptr, exception_to_status(), error_info);
return;
}

Expand Down Expand Up @@ -535,10 +537,10 @@ void SyncSession::download_fresh_realm(const sync::SessionErrorInfo& error_info)
// it immediately
fresh_sync_session->force_close();
if (subs.is_ok()) {
self->handle_fresh_realm_downloaded(db, std::move(error_info), std::move(subs.get_value()));
self->handle_fresh_realm_downloaded(db, Status::OK(), error_info, std::move(subs.get_value()));
}
else {
self->handle_fresh_realm_downloaded(nullptr, std::move(subs.get_status()));
self->handle_fresh_realm_downloaded(nullptr, std::move(subs.get_status()), error_info);
}
});
}
Expand All @@ -549,18 +551,18 @@ void SyncSession::download_fresh_realm(const sync::SessionErrorInfo& error_info)
fresh_sync_session->force_close();
if (auto strong_self = weak_self.lock()) {
if (status.is_ok()) {
strong_self->handle_fresh_realm_downloaded(db, std::move(error_info));
strong_self->handle_fresh_realm_downloaded(db, Status::OK(), error_info);
}
else {
strong_self->handle_fresh_realm_downloaded(nullptr, std::move(status));
strong_self->handle_fresh_realm_downloaded(nullptr, std::move(status), error_info);
}
}
});
}
fresh_sync_session->revive_if_needed();
}

void SyncSession::handle_fresh_realm_downloaded(DBRef db, StatusWith<sync::SessionErrorInfo> error_info,
void SyncSession::handle_fresh_realm_downloaded(DBRef db, Status result, const sync::SessionErrorInfo& cr_error_info,
std::optional<sync::SubscriptionSet> new_subs)
{
util::CheckedUniqueLock lock(m_state_mutex);
Expand All @@ -571,15 +573,16 @@ void SyncSession::handle_fresh_realm_downloaded(DBRef db, StatusWith<sync::Sessi
// - unable to write the fresh copy to the file system
// - during download of the fresh copy, the fresh copy itself is reset
// - in FLX mode there was a problem fulfilling the previously active subscription
if (!error_info.is_ok()) {
if (error_info.get_status() == ErrorCodes::OperationAborted) {
if (!result.is_ok()) {
if (result == ErrorCodes::OperationAborted) {
return;
}
lock.unlock();

sync::SessionErrorInfo synthetic(
Status{ErrorCodes::AutoClientResetFailed,
util::format("A fatal error occurred during client reset: '%1'", error_info.get_status())},
util::format("A fatal error occurred during '%1' client reset for %2: '%3'",
cr_error_info.server_requests_action, cr_error_info.status, result)},
sync::IsFatal{true});
handle_error(synthetic);
return;
Expand All @@ -598,8 +601,7 @@ void SyncSession::handle_fresh_realm_downloaded(DBRef db, StatusWith<sync::Sessi
m_client_reset_fresh_copy = db;
CompletionCallbacks callbacks;
// Save the client reset error for when the original sync session is revived
REALM_ASSERT(error_info.is_ok()); // required if we get here
m_client_reset_error = std::move(error_info.get_value());
m_client_reset_error = cr_error_info;

std::swap(m_completion_callbacks, callbacks);
// always swap back, even if advance_state throws
Expand Down
2 changes: 1 addition & 1 deletion src/realm/object-store/sync/sync_session.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ class SyncSession : public std::enable_shared_from_this<SyncSession> {

void download_fresh_realm(const sync::SessionErrorInfo& error_info)
REQUIRES(!m_config_mutex, !m_state_mutex, !m_connection_state_mutex);
void handle_fresh_realm_downloaded(DBRef db, StatusWith<sync::SessionErrorInfo> error_info,
void handle_fresh_realm_downloaded(DBRef db, Status result, const sync::SessionErrorInfo& cr_error_info,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I needed access to both the Status that just occurred as well as the SessionErrorInfo for the originating client reset error. When it was using the StatusWith, you would only have access to one or the other...

std::optional<sync::SubscriptionSet> new_subs = std::nullopt)
REQUIRES(!m_state_mutex, !m_config_mutex, !m_connection_state_mutex);
void handle_error(sync::SessionErrorInfo) REQUIRES(!m_state_mutex, !m_config_mutex, !m_connection_state_mutex);
Expand Down
12 changes: 11 additions & 1 deletion src/realm/sync/noinst/client_impl_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2345,11 +2345,21 @@ Status Session::receive_ident_message(SaltedFileIdent client_file_ident)
// if a client reset happens, it will take care of setting the file ident
// and if not, we do it here
bool did_client_reset = false;

// Save some of the client reset info for reporting to the client if an error occurs.
Status cr_status(Status::OK()); // Start with no client reset
ProtocolErrorInfo::Action cr_action = ProtocolErrorInfo::Action::NoAction;
if (auto& cr_config = get_client_reset_config()) {
cr_status = cr_config->error;
cr_action = cr_config->action;
}

try {
did_client_reset = client_reset_if_needed();
}
catch (const std::exception& e) {
auto err_msg = util::format("A fatal error occurred during client reset: '%1'", e.what());
auto err_msg = util::format("A fatal error occurred during '%1' client reset for %2: '%3'", cr_action,
cr_status, e.what());
logger.error(err_msg.c_str());
SessionErrorInfo err_info(Status{ErrorCodes::AutoClientResetFailed, err_msg}, IsFatal{true});
suspend(err_info);
Expand Down
10 changes: 6 additions & 4 deletions test/object-store/sync/flx_sync.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,8 @@ TEST_CASE("flx: client reset", "[sync][flx][client reset][baas]") {
REQUIRE(before_reset_count == 1);
REQUIRE(after_reset_count == 0);
REQUIRE(sync_error.status == ErrorCodes::AutoClientResetFailed);
REQUIRE(sync_error.status.reason().find(
"SyncClientResetRequired: Bad client file identifier (IDENT)") != std::string::npos);
REQUIRE(sync_error.is_client_reset_requested());
local_realm->refresh();
auto table = local_realm->read_group().get_table("class_TopLevel");
Expand Down Expand Up @@ -1133,6 +1135,8 @@ TEST_CASE("flx: client reset", "[sync][flx][client reset][baas]") {
auto sync_error = wait_for_future(std::move(err_future)).get();
INFO(sync_error.status);
CHECK(sync_error.status == ErrorCodes::AutoClientResetFailed);
REQUIRE(sync_error.status.reason().find(
"SyncClientResetRequired: Bad client file identifier (IDENT)") != std::string::npos);
})
->run();
}
Expand Down Expand Up @@ -1420,8 +1424,7 @@ TEST_CASE("flx: client reset", "[sync][flx][client reset][baas]") {
REQUIRE(!ref);
REQUIRE(error);
REQUIRE_THROWS_CONTAINING(std::rethrow_exception(error),
"A fatal error occurred during client reset: 'Client reset cannot recover when "
"classes have been removed: {AddedClass}'");
"'Client reset cannot recover when classes have been removed: {AddedClass}'");
});
error_future.get();
CHECK(before_reset_count == 1);
Expand All @@ -1441,8 +1444,7 @@ TEST_CASE("flx: client reset", "[sync][flx][client reset][baas]") {
REQUIRE(error);
REQUIRE_THROWS_CONTAINING(
std::rethrow_exception(error),
"A fatal error occurred during client reset: 'The following changes cannot be "
"made in additive-only schema mode:\n"
"'The following changes cannot be made in additive-only schema mode:\n"
"- Property 'TopLevel._id' has been changed from 'object id' to 'uuid'.\nIf your app is running in "
"development mode, you can delete the realm and restart the app to update your schema.'");
});
Expand Down
11 changes: 6 additions & 5 deletions test/test_client_reset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -730,12 +730,12 @@ TEST(ClientReset_DoNotRecoverSchema)
// schema change is not allowed and so fails with a client reset error.
{
Session::Config session_config;
std::string error_msg = "Some bad client file identifier (IDENT)";
{
Session::Config::ClientReset cr_config{
ClientResyncMode::DiscardLocal,
sg_fresh1,
{ErrorCodes::SyncClientResetRequired, "Bad client file identifier (IDENT)"},
sync::ProtocolErrorInfo::Action::ClientReset};
Session::Config::ClientReset cr_config{ClientResyncMode::DiscardLocal,
sg_fresh1,
{ErrorCodes::SyncClientResetRequired, error_msg},
sync::ProtocolErrorInfo::Action::ClientReset};
session_config.client_reset_config = std::move(cr_config);
}

Expand All @@ -746,6 +746,7 @@ TEST(ClientReset_DoNotRecoverSchema)
return;
REALM_ASSERT(error_info);
CHECK_EQUAL(error_info->status, ErrorCodes::AutoClientResetFailed);
CHECK(error_info->status.reason().find(error_msg) != std::string::npos);
bowl.add_stone();
};
Session session = fixture.make_session(path_1, server_path_2, std::move(session_config));
Expand Down
Loading