Merge pull request #59 from Valorith/copilot/sub-pr-55

Fix buff persistence regression when suppressed schema is missing

common/database/database_update_manifest.h

@@ -7192,14 +7192,15 @@ ALTER TABLE `character_parcels_containers`
 	},
 	ManifestEntry{
 		.version = 9329,
-		.description = "2025_06_27_add_suppressed_to_character_buffs.sql",
+		.description = "2026_03_08_add_suppressed_to_character_buffs.sql",
 		.check = "SHOW COLUMNS FROM `character_buffs` LIKE 'suppressed'",
 		.condition = "empty",
 		.match = "",
 		.sql = R"(
 ALTER TABLE `character_buffs`
 	ADD COLUMN `suppressed` tinyint(1) unsigned NOT NULL DEFAULT 0 AFTER `instrument_mod`;
-)"
+)",
+		.content_schema_update = false
 	},
 // -- template; copy/paste this when you need to create a new entry
 //	ManifestEntry{

zone/cli/tests/cli_zone_state.cpp

@@ -828,9 +828,9 @@ inline void TestHpManaEnd()
 
 inline void TestClientBuffPersistence()
 {
-	constexpr uint32 test_character_id = 99999991;
-	constexpr uint16 normal_spell_id   = 6824;
-	constexpr uint16 suppressed_spell_id = 6824;
+	constexpr uint32 test_character_id  = 99999991;
+	constexpr uint16 normal_spell_id    = 6824;
+	constexpr uint16 suppressed_spell_id = 2550;
 
 	auto schema_check = database.QueryDatabase("SHOW COLUMNS FROM `character_buffs` LIKE 'suppressed'");
 	RunTest(
@@ -936,6 +936,107 @@ inline void TestClientBuffPersistence()
 	);
 }
 
+inline void TestClientBuffPersistenceRollback()
+{
+	constexpr uint32 test_character_id    = 99999992;
+	constexpr uint16 preexisting_spell_id = 6824;  // stable, widely-used spell present in all data sets
+	constexpr int    suppressed_tics      = 99;
+
+	// Skip if suppressed column is absent — nothing to rename
+	auto schema_check = database.QueryDatabase("SHOW COLUMNS FROM `character_buffs` LIKE 'suppressed'");
+	if (!schema_check.Success() || schema_check.RowCount() == 0) {
+		RunTest(
+			"Client Buff Persistence > Rollback: skipped (suppressed column absent)",
+			true,
+			true
+		);
+		return;
+	}
+
+	// Ensure a clean starting state
+	CharacterBuffsRepository::DeleteWhere(
+		database,
+		fmt::format("`character_id` = {}", test_character_id)
+	);
+
+	// Insert one pre-existing buff row that must survive a failed SaveBuffs()
+	auto pre          = CharacterBuffsRepository::NewEntity();
+	pre.character_id  = test_character_id;
+	pre.slot_id       = 0;
+	pre.spell_id      = preexisting_spell_id;
+	pre.caster_level  = 40;
+	pre.ticsremaining = 30;
+	const int inserted = CharacterBuffsRepository::InsertMany(database, { pre });
+	RunTest("Client Buff Persistence > Rollback: pre-existing row inserted", 1, inserted);
+	if (inserted != 1) {
+		return;
+	}
+
+	// Rename `suppressed` so that REPLACE INTO (which lists it by name) fails
+	const auto rename_result = database.QueryDatabase(
+		"ALTER TABLE `character_buffs` "
+		"CHANGE `suppressed` `suppressed_bkp` tinyint(1) unsigned NOT NULL DEFAULT 0"
+	);
+	RunTest("Client Buff Persistence > Rollback: column rename succeeded", true, rename_result.Success());
+	if (!rename_result.Success()) {
+		CharacterBuffsRepository::DeleteWhere(
+			database,
+			fmt::format("`character_id` = {}", test_character_id)
+		);
+		return;
+	}
+
+	// Try to save a different buff — the REPLACE INTO will fail and must roll back
+	Client saver;
+	saver.SetCharacterId(test_character_id);
+	saver.SetName("buff-rollback-save");
+	saver.SetClientVersion(EQ::versions::ClientVersion::RoF2);
+
+	auto saver_buffs = saver.GetBuffs();
+	for (int slot = 0; slot < saver.GetMaxBuffSlots(); ++slot) {
+		saver_buffs[slot]        = Buffs_Struct{};
+		saver_buffs[slot].spellid = SPELL_UNKNOWN;
+	}
+
+	// Use SPELL_SUPPRESSED — always passes IsValidOrSuppressedSpell, no spell-data dependency
+	saver_buffs[0].spellid                 = SPELL_SUPPRESSED;
+	saver_buffs[0].suppressedid            = preexisting_spell_id;
+	saver_buffs[0].suppressedticsremaining = suppressed_tics;
+	saver_buffs[0].ticsremaining           = 5;
+	saver_buffs[0].casterlevel             = 60;
+
+	database.SaveBuffs(&saver);
+
+	// Restore the column before any assertions so subsequent tests are unaffected
+	const auto restore_result = database.QueryDatabase(
+		"ALTER TABLE `character_buffs` "
+		"CHANGE `suppressed_bkp` `suppressed` tinyint(1) unsigned NOT NULL DEFAULT 0"
+	);
+	RunTest("Client Buff Persistence > Rollback: column restore succeeded", true, restore_result.Success());
+
+	// Pre-existing rows must still be present because the transaction was rolled back
+	const auto rows_after = CharacterBuffsRepository::GetWhere(
+		database,
+		fmt::format("`character_id` = {} ORDER BY `slot_id`", test_character_id)
+	);
+
+	RunTest(
+		"Client Buff Persistence > Rollback: pre-existing rows not wiped on insert failure",
+		1,
+		(int) rows_after.size()
+	);
+	RunTest(
+		"Client Buff Persistence > Rollback: pre-existing spell ID preserved after rollback",
+		(int) preexisting_spell_id,
+		rows_after.empty() ? -1 : (int) rows_after[0].spell_id
+	);
+
+	CharacterBuffsRepository::DeleteWhere(
+		database,
+		fmt::format("`character_id` = {}", test_character_id)
+	);
+}
+
 inline void TestBuffs()
 {
 	for (auto &e: entity_list.GetNPCList()) {
@@ -1243,6 +1344,7 @@ void ZoneCLI::TestZoneState(int argc, char **argv, argh::parser &cmd, std::strin
 	TestZoneVariables();
 	TestHpManaEnd();
 	TestClientBuffPersistence();
+	TestClientBuffPersistenceRollback();
 	TestBuffs();
 	TestLocationChange();
 	TestEntityVariables();

zone/zonedb.cpp

@@ -2925,15 +2925,33 @@ void ZoneDatabase::SaveBuffs(Client *client)
 		v.emplace_back(e);
 	}
 
-	database.TransactionBegin();
+	const auto begin_result = database.QueryDatabase("START TRANSACTION");
+	if (!begin_result.Success()) {
+		LogError(
+			"Failed to begin buff save transaction for character [{}] [{}]: {}",
+			client->GetCleanName(),
+			client->CharacterID(),
+			begin_result.ErrorMessage()
+		);
+		return;
+	}
 
-	CharacterBuffsRepository::DeleteWhere(
-		database,
+	const auto delete_result = database.QueryDatabase(
 		fmt::format(
-			"`character_id` = {}",
+			"DELETE FROM `character_buffs` WHERE `character_id` = {}",
 			client->CharacterID()
 		)
 	);
+	if (!delete_result.Success()) {
+		database.TransactionRollback();
+		LogError(
+			"Failed to delete existing buffs for character [{}] [{}]: {}",
+			client->GetCleanName(),
+			client->CharacterID(),
+			delete_result.ErrorMessage()
+		);
+		return;
+	}
 
 	if (!v.empty()) {
 		const auto saved_count = CharacterBuffsRepository::ReplaceMany(database, v);