From 978ca224a12608ed8cabde2ed82c74f564813509 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Wed, 9 Oct 2024 10:26:57 -0700 Subject: [PATCH] Skip backup files when scanning for audit Realms to upload (#8038) Backup realms from file format upgrades are placed next to the original file, so the audit realm pool will find the backup and attempt to upload it, triggering another upgrade and backup until the filename becomes too long and it crashes instead. It needs to instead skip the backup files and delete any lingering backups of Realms which have already been uploaded. --- CHANGELOG.md | 2 +- src/realm/backup_restore.cpp | 5 +++++ src/realm/backup_restore.hpp | 2 +- src/realm/object-store/audit.mm | 34 +++++++++++++++++++++++++++++++++ test/object-store/audit.cpp | 15 ++++++++++++--- 5 files changed, 53 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 933f9009d0f..c6d545ea4b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* The events library would attempt to upload backup files created as part of file format upgrades, causing backup copies of those backups to be made, looping until the maximum file name size was reached ([#8040](https://github.com/realm/realm-core/issues/8040), since v11.17.0). ### Breaking changes * None. diff --git a/src/realm/backup_restore.cpp b/src/realm/backup_restore.cpp index 8070642af0d..9b6d9e33f6a 100644 --- a/src/realm/backup_restore.cpp +++ b/src/realm/backup_restore.cpp @@ -80,6 +80,11 @@ BackupHandler::BackupHandler(const std::string& path, const VersionList& accepte m_delete_versions = to_be_deleted; } +std::string BackupHandler::get_prefix() const +{ + return m_prefix; +} + bool BackupHandler::must_restore_from_backup(int current_file_format_version) const { if (current_file_format_version == 0) diff --git a/src/realm/backup_restore.hpp b/src/realm/backup_restore.hpp index e0a258546ac..3222b1c1280 100644 --- a/src/realm/backup_restore.hpp +++ b/src/realm/backup_restore.hpp @@ -34,7 +34,7 @@ class BackupHandler { void restore_from_backup(); void cleanup_backups(); void backup_realm_if_needed(int current_file_format_version, int target_file_format_version); - std::string get_prefix(); + std::string get_prefix() const; static std::string get_prefix_from_path(const std::string& path); // default lists of accepted versions and backups to delete when they get old enough diff --git a/src/realm/object-store/audit.mm b/src/realm/object-store/audit.mm index ceae5c38943..85313cf96cd 100644 --- a/src/realm/object-store/audit.mm +++ b/src/realm/object-store/audit.mm @@ -655,6 +655,7 @@ explicit AuditRealmPool(Private, std::shared_ptr user, const AuditConf void open_new_realm() REQUIRES(!m_mutex); void wait_for_upload(std::shared_ptr) REQUIRES(m_mutex); void scan_for_realms_to_upload() REQUIRES(!m_mutex); + bool clean_up_backup(const std::string& file_name); std::string prefixed_partition(std::string const& partition); }; @@ -814,6 +815,8 @@ explicit AuditRealmPool(Private, std::shared_ptr user, const AuditConf while (dir.next(file_name)) { if (!StringData(file_name).ends_with(".realm")) continue; + if (clean_up_backup(file_name)) + continue; std::string path = m_path_root + file_name; if (m_open_paths.count(path)) { @@ -846,6 +849,37 @@ explicit AuditRealmPool(Private, std::shared_ptr user, const AuditConf m_cv.notify_all(); } +bool AuditRealmPool::clean_up_backup(const std::string& file_name) +{ + auto pos = file_name.find(".backup."); + if (pos == std::string::npos) + return false; + + auto base_name = StringData(file_name.c_str(), file_name.find('.')); + auto base_realm_path = util::format("%1%2.realm", m_path_root, base_name); + + // If the file which this was a backup of no longer exists we no longer need + // the backup, and the backup won't be cleaned up by the normal code path as + // that happens when the base file is opened. + if (!util::File::exists(base_realm_path)) { + auto path = m_path_root + file_name; + m_logger->info("Events: Removing stale backup of uploaded file at '%1'", path); + util::File::remove(path); + return true; + } + + pos = file_name.find(".backup.", pos + 1); + if (pos == std::string::npos) + return true; + + // .backup appears multiple times, so this was a backup of a backup and + // should be removed even though the base Realm still exists + auto path = m_path_root + file_name; + m_logger->info("Events: Removing backup of a backup at '%1'", path); + util::File::remove(path); + return true; +} + void AuditRealmPool::open_new_realm() { ObjectSchema schema = {"AuditEvent", diff --git a/test/object-store/audit.cpp b/test/object-store/audit.cpp index c4fad02fb36..3e3432f0150 100644 --- a/test/object-store/audit.cpp +++ b/test/object-store/audit.cpp @@ -1542,7 +1542,7 @@ TEST_CASE("audit realm sharding", "[sync][pbs][audit]") { std::string file_name; util::DirScanner dir(root); size_t file_count = 0; - size_t unlocked_file_count = 0; + std::vector unlocked_files; while (dir.next(file_name)) { if (!StringData(file_name).ends_with(".realm")) continue; @@ -1551,7 +1551,7 @@ TEST_CASE("audit realm sharding", "[sync][pbs][audit]") { // than it. 1 MB errs on the side of never getting spurious failures. REQUIRE(util::File::get_size_static(root + "/" + file_name) < 1024 * 1024); if (DB::call_with_lock(root + "/" + file_name, [](auto&) {})) { - ++unlocked_file_count; + unlocked_files.push_back(file_name); } } // The exact number of shards is fuzzy due to the combination of the @@ -1561,7 +1561,16 @@ TEST_CASE("audit realm sharding", "[sync][pbs][audit]") { // There should be exactly two files open still: the one we're currently // writing to, and the first one which we wrote and are waiting for the // upload to complete. - REQUIRE(unlocked_file_count == file_count - 2); + REQUIRE(unlocked_files.size() == file_count - 2); + + // Create a backup copy of each of the unlocked files which should be cleaned up + for (auto& file : unlocked_files) { + BackupHandler handler(root + "/" + file, {}, {}); + handler.backup_realm_if_needed(23, 24); + // Set the version field in the backup file to 23 so that opening it + // won't accidentally work + util::File(handler.get_prefix() + "v23.backup.realm", util::File::mode_Update).write(12, "\x17"); + } auto get_sorted_events = [&] { auto events = get_audit_events(test_session, false);