diff --git a/dDatabase/GameDatabase/ITables/ICharXml.h b/dDatabase/GameDatabase/ITables/ICharXml.h index 22d006e5b..6fa8fb60e 100644 --- a/dDatabase/GameDatabase/ITables/ICharXml.h +++ b/dDatabase/GameDatabase/ITables/ICharXml.h @@ -2,6 +2,7 @@ #define __ICHARXML__H__ #include +#include #include #include @@ -10,9 +11,32 @@ public: // Get the character xml for the given character id. virtual std::string GetCharacterXml(const LWOOBJID charId) = 0; - // Update the character xml for the given character id. + // Overwrite the character xml for the given character id (dashboard edits, restores, maintenance). Bumps the save + // generation, so a world still holding the older version can't save over this one. virtual void UpdateCharacterXml(const LWOOBJID charId, const std::string_view lxfml) = 0; + /** + * Stale save guard. Every write to a character's xml bumps its save generation: a world loading the character to + * play it, a world saving it and the dashboard writing it. A world keeps the generation it loaded (or last saved) + * and only saves over that one, so a world that lost the character to another world or to the dashboard can't + * overwrite the newer data. + */ + struct CharacterXml { + std::string xml; + uint64_t generation{}; + }; + + // Take over the character for playing: bump its save generation and return the xml with the new generation. + // nullopt when the character has no xml. + virtual std::optional ClaimCharacterXml(const LWOOBJID charId) = 0; + + // Save if the stored generation is still `generation`: the xml is written, the generation becomes generation + 1 + // and true is returned. Otherwise nothing is written and false is returned (someone newer saved it). + virtual bool SaveCharacterXml(const LWOOBJID charId, const std::string_view lxfml, const uint64_t generation) = 0; + + // The stored save generation (0 when the character has no xml) + virtual uint64_t GetCharacterSaveGeneration(const LWOOBJID charId) = 0; + // Insert the character xml for the given character id. virtual void InsertCharacterXml(const LWOOBJID characterId, const std::string_view lxfml) = 0; diff --git a/dDatabase/GameDatabase/MySQL/MySQLDatabase.h b/dDatabase/GameDatabase/MySQL/MySQLDatabase.h index cbe5f3ed3..6a20c2a6a 100644 --- a/dDatabase/GameDatabase/MySQL/MySQLDatabase.h +++ b/dDatabase/GameDatabase/MySQL/MySQLDatabase.h @@ -75,6 +75,9 @@ public: std::optional GetAccountInfo(const std::string_view username) override; void InsertNewCharacter(const ICharInfo::Info info) override; void InsertCharacterXml(const LWOOBJID accountId, const std::string_view lxfml) override; + std::optional ClaimCharacterXml(const LWOOBJID charId) override; + bool SaveCharacterXml(const LWOOBJID charId, const std::string_view lxfml, const uint64_t generation) override; + uint64_t GetCharacterSaveGeneration(const LWOOBJID charId) override; std::string GetCharactersTable(uint32_t start, uint32_t length, const std::string_view search = "", uint32_t orderColumn = 0, bool orderAsc = true) override; std::vector GetAccountCharacterIds(LWOOBJID accountId) override; void DeleteCharacter(const LWOOBJID characterId) override; diff --git a/dDatabase/GameDatabase/MySQL/Tables/CharXml.cpp b/dDatabase/GameDatabase/MySQL/Tables/CharXml.cpp index 3d41f9d74..2ca2c1a57 100644 --- a/dDatabase/GameDatabase/MySQL/Tables/CharXml.cpp +++ b/dDatabase/GameDatabase/MySQL/Tables/CharXml.cpp @@ -11,7 +11,26 @@ std::string MySQLDatabase::GetCharacterXml(const LWOOBJID charId) { } void MySQLDatabase::UpdateCharacterXml(const LWOOBJID charId, const std::string_view lxfml) { - ExecuteUpdate("UPDATE charxml SET xml_data = ? WHERE id = ?;", lxfml, charId); + ExecuteUpdate("UPDATE charxml SET xml_data = ?, save_generation = save_generation + 1 WHERE id = ?;", lxfml, charId); +} + +std::optional MySQLDatabase::ClaimCharacterXml(const LWOOBJID charId) { + ExecuteUpdate("UPDATE charxml SET save_generation = save_generation + 1 WHERE id = ?;", charId); + // Read both from one row, so a write in between still leaves a matching pair + auto result = ExecuteSelect("SELECT xml_data, save_generation FROM charxml WHERE id = ? LIMIT 1;", charId); + if (!result->next()) return std::nullopt; + return CharacterXml{ result->getString("xml_data").c_str(), static_cast(result->getInt64("save_generation")) }; +} + +bool MySQLDatabase::SaveCharacterXml(const LWOOBJID charId, const std::string_view lxfml, const uint64_t generation) { + // The generation always changes, so a matching row is always counted as affected + return ExecuteUpdate("UPDATE charxml SET xml_data = ?, save_generation = ? WHERE id = ? AND save_generation = ?;", + lxfml, static_cast(generation + 1), charId, static_cast(generation)) > 0; +} + +uint64_t MySQLDatabase::GetCharacterSaveGeneration(const LWOOBJID charId) { + auto result = ExecuteSelect("SELECT save_generation FROM charxml WHERE id = ? LIMIT 1;", charId); + return result->next() ? static_cast(result->getInt64("save_generation")) : 0; } void MySQLDatabase::InsertCharacterXml(const LWOOBJID characterId, const std::string_view lxfml) { diff --git a/dDatabase/GameDatabase/SQLite/SQLiteDatabase.h b/dDatabase/GameDatabase/SQLite/SQLiteDatabase.h index 5af62bc1f..543c4cfe1 100644 --- a/dDatabase/GameDatabase/SQLite/SQLiteDatabase.h +++ b/dDatabase/GameDatabase/SQLite/SQLiteDatabase.h @@ -59,6 +59,9 @@ public: std::optional GetAccountInfo(const std::string_view username) override; void InsertNewCharacter(const ICharInfo::Info info) override; void InsertCharacterXml(const LWOOBJID accountId, const std::string_view lxfml) override; + std::optional ClaimCharacterXml(const LWOOBJID charId) override; + bool SaveCharacterXml(const LWOOBJID charId, const std::string_view lxfml, const uint64_t generation) override; + uint64_t GetCharacterSaveGeneration(const LWOOBJID charId) override; std::string GetCharactersTable(uint32_t start, uint32_t length, const std::string_view search = "", uint32_t orderColumn = 0, bool orderAsc = true) override; std::vector GetAccountCharacterIds(LWOOBJID accountId) override; void DeleteCharacter(const LWOOBJID characterId) override; diff --git a/dDatabase/GameDatabase/SQLite/Tables/CharXml.cpp b/dDatabase/GameDatabase/SQLite/Tables/CharXml.cpp index 2c50cb603..68f17fcec 100644 --- a/dDatabase/GameDatabase/SQLite/Tables/CharXml.cpp +++ b/dDatabase/GameDatabase/SQLite/Tables/CharXml.cpp @@ -11,7 +11,25 @@ std::string SQLiteDatabase::GetCharacterXml(const LWOOBJID charId) { } void SQLiteDatabase::UpdateCharacterXml(const LWOOBJID charId, const std::string_view lxfml) { - ExecuteUpdate("UPDATE charxml SET xml_data = ? WHERE id = ?;", lxfml, charId); + ExecuteUpdate("UPDATE charxml SET xml_data = ?, save_generation = save_generation + 1 WHERE id = ?;", lxfml, charId); +} + +std::optional SQLiteDatabase::ClaimCharacterXml(const LWOOBJID charId) { + ExecuteUpdate("UPDATE charxml SET save_generation = save_generation + 1 WHERE id = ?;", charId); + // Read both from one row, so a write in between still leaves a matching pair + auto [_, result] = ExecuteSelect("SELECT xml_data, save_generation FROM charxml WHERE id = ? LIMIT 1;", charId); + if (result.eof()) return std::nullopt; + return CharacterXml{ result.getStringField("xml_data"), static_cast(result.getInt64Field("save_generation")) }; +} + +bool SQLiteDatabase::SaveCharacterXml(const LWOOBJID charId, const std::string_view lxfml, const uint64_t generation) { + return ExecuteUpdate("UPDATE charxml SET xml_data = ?, save_generation = ? WHERE id = ? AND save_generation = ?;", + lxfml, static_cast(generation + 1), charId, static_cast(generation)) > 0; +} + +uint64_t SQLiteDatabase::GetCharacterSaveGeneration(const LWOOBJID charId) { + auto [_, result] = ExecuteSelect("SELECT save_generation FROM charxml WHERE id = ? LIMIT 1;", charId); + return result.eof() ? 0 : static_cast(result.getInt64Field("save_generation")); } void SQLiteDatabase::InsertCharacterXml(const LWOOBJID characterId, const std::string_view lxfml) { diff --git a/dDatabase/GameDatabase/TestSQL/TestSQLDatabase.h b/dDatabase/GameDatabase/TestSQL/TestSQLDatabase.h index 32adcbba8..c158962f9 100644 --- a/dDatabase/GameDatabase/TestSQL/TestSQLDatabase.h +++ b/dDatabase/GameDatabase/TestSQL/TestSQLDatabase.h @@ -38,6 +38,9 @@ class TestSQLDatabase : public GameDatabase { std::optional GetAccountInfo(const std::string_view username) override; void InsertNewCharacter(const ICharInfo::Info info) override; void InsertCharacterXml(const LWOOBJID accountId, const std::string_view lxfml) override; + std::optional ClaimCharacterXml(const LWOOBJID charId) override { return std::nullopt; }; + bool SaveCharacterXml(const LWOOBJID charId, const std::string_view lxfml, const uint64_t generation) override { return true; }; + uint64_t GetCharacterSaveGeneration(const LWOOBJID charId) override { return 0; }; std::vector GetAccountCharacterIds(LWOOBJID accountId) override; void DeleteCharacter(const LWOOBJID characterId) override; void SetCharacterName(const LWOOBJID characterId, const std::string_view name) override; diff --git a/dGame/Character.cpp b/dGame/Character.cpp index 46f061081..7d321ecba 100644 --- a/dGame/Character.cpp +++ b/dGame/Character.cpp @@ -26,6 +26,7 @@ #include "ePlayerFlag.h" #include "CDPlayerFlagsTable.h" #include "EconomyLedger.h" +#include "eServerDisconnectIdentifiers.h" Character::Character(LWOOBJID id, User* parentUser) { //First load the name, etc: @@ -52,8 +53,11 @@ void Character::UpdateInfoFromDatabase() { m_PermissionMap = charInfo->permissionMap; } - //Load the xmlData now: - m_XMLData = Database::Get()->GetCharacterXml(m_ID); + // Load the xmlData now. Loading takes the character over: saves from any server that loaded it before are refused. + auto saved = Database::Get()->ClaimCharacterXml(m_ID); + m_XMLData = saved ? std::move(saved->xml) : ""; + m_SaveGeneration = saved ? saved->generation : 0; + m_SaveRefused = false; if (m_XMLData.empty()) { LOG("Character %s (%llu) has no xml data!", m_Name.c_str(), m_ID); return; @@ -365,10 +369,36 @@ void Character::WriteToDatabase() { m_XMLData = printer.CStr(); //Finally, save to db: - Database::Get()->UpdateCharacterXml(m_ID, m_XMLData); + if (m_SaveRefused) { + LOG("Not saving character %llu:%s: newer data was saved elsewhere since it was loaded here", m_ID, m_Name.c_str()); + return; + } + if (!Database::Get()->SaveCharacterXml(m_ID, m_XMLData, m_SaveGeneration)) { + OnStaleSave(); + return; + } + m_SaveGeneration++; DashboardNotify::Changed("characters", m_ID); } +void Character::OnStaleSave() { + m_SaveRefused = true; + const auto stored = Database::Get()->GetCharacterSaveGeneration(m_ID); + LOG("Refused a stale save of character %llu:%s: this server has save generation %llu, the database %llu (another world " + "or the dashboard saved it since). The newer data is kept.", m_ID, m_Name.c_str(), m_SaveGeneration, stored); + const auto accountId = m_ParentUser ? m_ParentUser->GetAccountID() : 0; + const auto zone = Game::server ? Game::server->GetZoneID() : 0; + const auto instance = Game::server ? Game::server->GetInstanceID() : 0; + Database::Get()->InsertAuditLog(0, "World server", "stale_save_refused", + m_Name + ": a save from zone " + std::to_string(zone) + " instance " + std::to_string(instance) + " (generation " + std::to_string(m_SaveGeneration) + + ") was refused because a newer one (generation " + std::to_string(stored) + ") is stored; the newer data was kept", accountId, m_ID); + // If the player is still connected here, what they do next would be lost too: send them out so they load the + // newer data. The disconnect is handled later, like any other. + if (m_ParentUser && Game::server && Game::server->IsConnected(m_ParentUser->GetSystemAddress())) { + Game::server->Disconnect(m_ParentUser->GetSystemAddress(), eServerDisconnectIdentifiers::SAVE_FAILURE); + } +} + void Character::SetPlayerFlag(const uint32_t flagId, const bool value) { // If the flag is already set, we don't have to recalculate it if (GetPlayerFlag(flagId) == value) return; diff --git a/dGame/Character.h b/dGame/Character.h index 31e5eb7fd..e7ddfb37e 100644 --- a/dGame/Character.h +++ b/dGame/Character.h @@ -31,12 +31,23 @@ public: */ void WriteToDatabase(); void SaveXMLToDatabase(); + + /** + * A save was refused by the stale save guard: log and audit it, save nothing more from here and send the player + * out if they are still connected here, so they load the newer data. + */ + void OnStaleSave(); void UpdateFromDatabase(); void SaveXmlRespawnCheckpoints(); void LoadXmlRespawnCheckpoints(); const std::string& GetXMLData() const { return m_XMLData; } + + // The stale save guard's generation (see ICharXml::CharacterXml) and whether a save was refused because of it + uint64_t GetSaveGeneration() const { return m_SaveGeneration; } + void SetSaveGeneration(uint64_t generation) { m_SaveGeneration = generation; m_SaveRefused = false; } + bool IsSaveRefused() const { return m_SaveRefused; } const tinyxml2::XMLDocument& GetXMLDoc() const { return m_Doc; } void _setXmlDoc(tinyxml2::XMLDocument& doc) { doc.DeepCopy(&m_Doc); } @@ -615,6 +626,18 @@ private: */ std::string m_XMLData; + /** + * The save generation this server loaded or last saved (see ICharXml::CharacterXml). Saves only go through while + * the database still has it. + */ + uint64_t m_SaveGeneration{}; + + /** + * A save was refused because someone newer (another world or the dashboard) saved this character since it was + * loaded here: no more saves from this server. + */ + bool m_SaveRefused{}; + /** * The last zone visited by the character that was not an instance zone */ diff --git a/docs/Dashboard.md b/docs/Dashboard.md index 785ce7cb3..25a2fed6c 100644 --- a/docs/Dashboard.md +++ b/docs/Dashboard.md @@ -659,6 +659,15 @@ Everything that changes a character (the editor, restoring an old version, uploa player if they're online, saves a snapshot of the character as it was, then writes the change and notes it in the audit log. Nothing is ever changed under a player who is still in game. +Every character also has a save generation (`charxml.save_generation`), so an older copy of it can never be saved over +a newer one. It goes up when a world loads the character for play, on every save from that world and on every change +made here. A world only saves over the generation it loaded or last saved itself; anything else means someone newer +(another world, or the dashboard) saved the character since, so the save is refused and the newer data kept. That +covers a world that noticed a disconnect late after the player moved on, and a world still holding a character that +was edited here. A refused save is logged by the world and written to the audit log as `stale_save_refused` (so it +shows under the account's related data); that world saves nothing more for the character, and a player still +connected to it is disconnected (the client's save failure message) so they load the newer data. + - **Edit** (GM 8+, `characters_edit`): coins, U-score, level, and adding or removing items. - **History** (GM 3+, `characters_history`): earlier versions of the character. Each one shows what differs from now, can be downloaded as XML, and (with `characters_edit`) restored. The `character_snapshots` task saves every diff --git a/migrations/dlu/mysql/76_character_save_generation.sql b/migrations/dlu/mysql/76_character_save_generation.sql new file mode 100644 index 000000000..0e64349a6 --- /dev/null +++ b/migrations/dlu/mysql/76_character_save_generation.sql @@ -0,0 +1,8 @@ +/* Stale save guard: bumped whenever a world loads a character to play it, a world saves it or the dashboard writes it. + A world only saves over the generation it loaded or last saved; an older world's save is refused. Only added when + it is not there yet. */ +SET @dlu_column = (SELECT IF(COUNT(*) = 0, 'ALTER TABLE charxml ADD COLUMN save_generation BIGINT NOT NULL DEFAULT 0', 'DO 0') FROM information_schema.columns + WHERE table_schema = DATABASE() AND table_name = 'charxml' AND column_name = 'save_generation'); +PREPARE dlu_column_stmt FROM @dlu_column; +EXECUTE dlu_column_stmt; +DEALLOCATE PREPARE dlu_column_stmt; diff --git a/migrations/dlu/sqlite/59_character_save_generation.sql b/migrations/dlu/sqlite/59_character_save_generation.sql new file mode 100644 index 000000000..6137b6e9b --- /dev/null +++ b/migrations/dlu/sqlite/59_character_save_generation.sql @@ -0,0 +1,2 @@ +/* Stale save guard: see the MySQL migration. */ +ALTER TABLE charxml ADD COLUMN save_generation INTEGER NOT NULL DEFAULT 0; diff --git a/tests/dDatabaseTests/DatabaseParityTests.cpp b/tests/dDatabaseTests/DatabaseParityTests.cpp index e9c248a88..1d55cabd8 100644 --- a/tests/dDatabaseTests/DatabaseParityTests.cpp +++ b/tests/dDatabaseTests/DatabaseParityTests.cpp @@ -548,6 +548,29 @@ TEST_F(ParitySeeded, Characters) { } Both("GetCharacterXml", [](GameDatabase& db) { return db.GetCharacterXml(CHAR_BOB); }); Both("UpdateCharacterXml", [](GameDatabase& db) { db.UpdateCharacterXml(CHAR_GM, ""); return db.GetCharacterXml(CHAR_GM); }); + // Stale save guard: a world loads (claims), saves, then loses the character to another world or the dashboard + const auto claim = Both("ClaimCharacterXml", [](GameDatabase& db) { + const auto before = db.GetCharacterSaveGeneration(CHAR_GM); + const auto claimed = db.ClaimCharacterXml(CHAR_GM); + return json{ claimed.has_value(), claimed ? claimed->xml : "", claimed ? claimed->generation - before : 0, db.ClaimCharacterXml(42).has_value() }; + }); + EXPECT_EQ(claim, json({ true, "", 1, false })); + const auto saves = Both("SaveCharacterXml", [](GameDatabase& db) { + const auto first = db.ClaimCharacterXml(CHAR_GM)->generation; + const bool saved = db.SaveCharacterXml(CHAR_GM, "", first); + // Same content again from the new generation: still counted as saved (the generation always changes) + const bool savedAgain = db.SaveCharacterXml(CHAR_GM, "", first + 1); + // Another world takes the character over; the first world's next save is stale + const auto second = db.ClaimCharacterXml(CHAR_GM)->generation; + const bool stale = db.SaveCharacterXml(CHAR_GM, "", first + 2); + const bool fresh = db.SaveCharacterXml(CHAR_GM, "", second); + // A dashboard edit bumps it too + db.UpdateCharacterXml(CHAR_GM, ""); + const bool afterEdit = db.SaveCharacterXml(CHAR_GM, "", second + 1); + return json{ saved, savedAgain, stale, fresh, afterEdit, second - first, db.GetCharacterSaveGeneration(CHAR_GM) - first, db.GetCharacterXml(CHAR_GM), + db.SaveCharacterXml(42, "", 0), db.GetCharacterSaveGeneration(42) }; + }); + EXPECT_EQ(saves, json({ true, true, false, true, false, 3, 5, "", false, 0 })); Both("GetDashboardSnapshot", [](GameDatabase& db) { return db.GetDashboardSnapshot(); }); } diff --git a/tests/dGameTests/CMakeLists.txt b/tests/dGameTests/CMakeLists.txt index 983697571..04ba7611a 100644 --- a/tests/dGameTests/CMakeLists.txt +++ b/tests/dGameTests/CMakeLists.txt @@ -5,6 +5,7 @@ set(DGAMETEST_SOURCES "EconomyLedgerTests.cpp" "PlayerReportsLimitTests.cpp" "SlashCommandPermissionTests.cpp" + "StaleSaveGuardTests.cpp" ) add_subdirectory(dComponentsTests) diff --git a/tests/dGameTests/StaleSaveGuardTests.cpp b/tests/dGameTests/StaleSaveGuardTests.cpp new file mode 100644 index 000000000..3c48a1d2d --- /dev/null +++ b/tests/dGameTests/StaleSaveGuardTests.cpp @@ -0,0 +1,108 @@ +#include "GameDependencies.h" + +#include "Character.h" +#include "Database.h" + +namespace { + // One character's xml and save generation, like the charxml row + class SaveDatabase : public TestSQLDatabase { + public: + std::string xml = ""; + uint64_t generation = 7; + uint32_t saves = 0; + std::vector audits; + + std::optional ClaimCharacterXml(const LWOOBJID) override { return CharacterXml{ xml, ++generation }; } + bool SaveCharacterXml(const LWOOBJID, const std::string_view lxfml, const uint64_t expected) override { + if (expected != generation) return false; + xml = lxfml; + generation = expected + 1; + saves++; + return true; + } + uint64_t GetCharacterSaveGeneration(const LWOOBJID) override { return generation; } + // A dashboard edit + void UpdateCharacterXml(const LWOOBJID, const std::string_view lxfml) override { xml = lxfml; generation++; } + void InsertAuditLog(uint32_t, const std::string_view, const std::string_view action, const std::string_view, uint32_t, LWOOBJID) override { + audits.push_back(std::string(action)); + } + }; +} + +class StaleSaveGuardTest : public GameDependenciesTest { +protected: + SaveDatabase* db{}; + + void SetUp() override { + SetUpDependencies(); + db = new SaveDatabase(); + Database::_setDatabase(db); + } + + void TearDown() override { + TearDownDependencies(); + } +}; + +TEST_F(StaleSaveGuardTest, LoadingClaimsTheCharacter) { + Character character(1, nullptr); + character.UpdateFromDatabase(); + EXPECT_EQ(character.GetSaveGeneration(), 8u); + EXPECT_EQ(db->generation, 8u); + EXPECT_EQ(character.GetXMLData(), db->xml); +} + +TEST_F(StaleSaveGuardTest, SavesMoveTheGenerationAlong) { + Character character(1, nullptr); + character.UpdateFromDatabase(); + character.WriteToDatabase(); + character.WriteToDatabase(); + EXPECT_EQ(db->saves, 2u); + EXPECT_EQ(character.GetSaveGeneration(), db->generation); + EXPECT_FALSE(character.IsSaveRefused()); + EXPECT_TRUE(db->audits.empty()); +} + +TEST_F(StaleSaveGuardTest, AnotherWorldLoadingMakesTheOldOneStale) { + Character oldWorld(1, nullptr); + oldWorld.UpdateFromDatabase(); + oldWorld.WriteToDatabase(); + + Character newWorld(1, nullptr); + newWorld.UpdateFromDatabase(); + newWorld.WriteToDatabase(); + const auto newXml = db->xml; + + // The old world still has the player (a disconnect it noticed late) and saves: refused, the new data stays + oldWorld.WriteToDatabase(); + EXPECT_TRUE(oldWorld.IsSaveRefused()); + EXPECT_EQ(db->xml, newXml); + EXPECT_EQ(db->saves, 2u); + ASSERT_EQ(db->audits.size(), 1u); + EXPECT_EQ(db->audits[0], "stale_save_refused"); + + // It doesn't try again (or audit again) + oldWorld.WriteToDatabase(); + EXPECT_EQ(db->audits.size(), 1u); + + // The world that has the character keeps saving + newWorld.WriteToDatabase(); + EXPECT_FALSE(newWorld.IsSaveRefused()); + EXPECT_EQ(db->saves, 3u); +} + +TEST_F(StaleSaveGuardTest, DashboardEditWins) { + Character character(1, nullptr); + character.UpdateFromDatabase(); + Database::Get()->UpdateCharacterXml(1, ""); + character.WriteToDatabase(); + EXPECT_TRUE(character.IsSaveRefused()); + EXPECT_EQ(db->xml, ""); + + // Loading again (the player logs back in) picks the edit up and saves normally + character.UpdateFromDatabase(); + EXPECT_FALSE(character.IsSaveRefused()); + EXPECT_EQ(character.GetXMLData(), ""); + character.WriteToDatabase(); + EXPECT_FALSE(character.IsSaveRefused()); +}