Merge pull request #55 from Valorith/codex/enforce-suppressed-buff-persistence

Fix buff persistence regression when suppressed schema is missing

common/database/database_update_manifest.h

@@ -7187,6 +7187,18 @@ ALTER TABLE `character_parcels`
 
 ALTER TABLE `character_parcels_containers`
 	ADD COLUMN `evolve_amount` INT UNSIGNED NOT NULL DEFAULT '0' AFTER `quantity`;
+)",
+		.content_schema_update = false
+	},
+	ManifestEntry{
+		.version = 9329,
+		.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
 	},

zone/cli/tests/cli_zone_state.cpp

@@ -1,7 +1,9 @@
 #include "zone/zone_cli.h"
+#include "zone/client.h"
 #include "zone/corpse.h"
 
 #include "common/cli/eqemu_command_handler.h"
+#include "common/repositories/character_buffs_repository.h"
 #include "common/repositories/npc_types_repository.h"
 #include "common/repositories/respawn_times_repository.h"
 
@@ -824,6 +826,116 @@ inline void TestHpManaEnd()
 	);
 }
 
+inline void TestClientBuffPersistence()
+{
+	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(
+		"Client Buff Persistence > `character_buffs.suppressed` column exists",
+		true,
+		schema_check.Success() && schema_check.RowCount() == 1
+	);
+
+	CharacterBuffsRepository::DeleteWhere(
+		database,
+		fmt::format("`character_id` = {}", test_character_id)
+	);
+
+	Client saver;
+	saver.SetCharacterId(test_character_id);
+	saver.SetName("buff-persistence-save");
+
+	auto saver_buffs = saver.GetBuffs();
+	for (int slot = 0; slot < saver.GetMaxBuffSlots(); ++slot) {
+		saver_buffs[slot]                         = Buffs_Struct{};
+		saver_buffs[slot].spellid                 = SPELL_UNKNOWN;
+		saver_buffs[slot].suppressedid            = 0;
+		saver_buffs[slot].suppressedticsremaining = -1;
+	}
+
+	saver_buffs[0].spellid        = normal_spell_id;
+	saver_buffs[0].casterlevel    = 50;
+	saver_buffs[0].ticsremaining  = 22;
+	saver_buffs[0].instrument_mod = 10;
+
+	saver_buffs[1].spellid                 = SPELL_SUPPRESSED;
+	saver_buffs[1].suppressedid            = suppressed_spell_id;
+	saver_buffs[1].suppressedticsremaining = 18;
+	saver_buffs[1].ticsremaining           = 4;
+	saver_buffs[1].casterlevel             = 55;
+	saver_buffs[1].instrument_mod          = 10;
+
+	database.SaveBuffs(&saver);
+
+	const auto persisted_rows = CharacterBuffsRepository::GetWhere(
+		database,
+		fmt::format("`character_id` = {} ORDER BY `slot_id`", test_character_id)
+	);
+
+	RunTest("Client Buff Persistence > Two buff rows were saved", 2, (int) persisted_rows.size());
+	RunTest(
+		"Client Buff Persistence > Suppressed row persisted with suppressed flag",
+		true,
+		persisted_rows.size() > 1 && persisted_rows[1].suppressed == 1
+	);
+	RunTest(
+		"Client Buff Persistence > Suppressed row persisted underlying spell ID",
+		(int) suppressed_spell_id,
+		persisted_rows.size() > 1 ? (int) persisted_rows[1].spell_id : -1
+	);
+	RunTest(
+		"Client Buff Persistence > Suppressed row persisted underlying duration",
+		18,
+		persisted_rows.size() > 1 ? persisted_rows[1].ticsremaining : -1
+	);
+
+	Client loader;
+	loader.SetCharacterId(test_character_id);
+	loader.SetName("buff-persistence-load");
+	loader.SetClientVersion(EQ::versions::ClientVersion::RoF2);
+	database.LoadBuffs(&loader);
+
+	auto loaded_buffs = loader.GetBuffs();
+	RunTest(
+		"Client Buff Persistence > Normal buff restored as active spell",
+		(int) normal_spell_id,
+		(int) loaded_buffs[0].spellid
+	);
+	RunTest(
+		"Client Buff Persistence > Normal buff duration restored",
+		22,
+		loaded_buffs[0].ticsremaining
+	);
+	RunTest(
+		"Client Buff Persistence > Suppressed buff restored as suppressed placeholder",
+		(int) SPELL_SUPPRESSED,
+		(int) loaded_buffs[1].spellid
+	);
+	RunTest(
+		"Client Buff Persistence > Suppressed buff restored underlying spell ID",
+		(int) suppressed_spell_id,
+		(int) loaded_buffs[1].suppressedid
+	);
+	RunTest(
+		"Client Buff Persistence > Suppressed buff restored underlying duration",
+		18,
+		(int) loaded_buffs[1].suppressedticsremaining
+	);
+	RunTest(
+		"Client Buff Persistence > Suppressed placeholder timer reset on load",
+		0,
+		loaded_buffs[1].ticsremaining
+	);
+
+	CharacterBuffsRepository::DeleteWhere(
+		database,
+		fmt::format("`character_id` = {}", test_character_id)
+	);
+}
+
 inline void TestBuffs()
 {
 	for (auto &e: entity_list.GetNPCList()) {
@@ -1130,6 +1242,7 @@ void ZoneCLI::TestZoneState(int argc, char **argv, argh::parser &cmd, std::strin
 
 	TestZoneVariables();
 	TestHpManaEnd();
+	TestClientBuffPersistence();
 	TestBuffs();
 	TestLocationChange();
 	TestEntityVariables();

zone/zonedb.cpp

@@ -2877,14 +2877,6 @@ void ZoneDatabase::UpdateAltCurrencyValue(uint32 char_id, uint32 currency_id, ui
 
 void ZoneDatabase::SaveBuffs(Client *client)
 {
-	CharacterBuffsRepository::DeleteWhere(
-		database,
-		fmt::format(
-			"`character_id` = {}",
-			client->CharacterID()
-		)
-	);
-
 	auto      buffs          = client->GetBuffs();
 	const int max_buff_slots = client->GetMaxBuffSlots();
 
@@ -2933,8 +2925,52 @@ void ZoneDatabase::SaveBuffs(Client *client)
 		v.emplace_back(e);
 	}
 
+	database.TransactionBegin();
+
+	const auto delete_result = database.QueryDatabase(
+		fmt::format(
+			"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()) {
-		CharacterBuffsRepository::ReplaceMany(database, v);
+		// Use < rather than != because REPLACE INTO can report 2× affected rows when it replaces
+		// an existing row (delete + insert). Since we DELETE first in the same transaction, these
+		// are always pure inserts, but < is more defensive and avoids false-positive failures.
+		const auto saved_count = CharacterBuffsRepository::ReplaceMany(database, v);
+		if (saved_count < static_cast<int>(v.size())) {
+			database.TransactionRollback();
+			LogError(
+				"Failed to save all buffs for character [{}] [{}]. Expected at least [{}] rows saved, got [{}]. Verify the `character_buffs` schema is up to date.",
+				client->GetCleanName(),
+				client->CharacterID(),
+				v.size(),
+				saved_count
+			);
+			return;
+		}
+	}
+
+	const auto commit_result = database.TransactionCommit();
+	if (!commit_result.Success()) {
+		database.TransactionRollback();
+		LogError(
+			"Failed to commit buff save transaction for character [{}] [{}]: {}",
+			client->GetCleanName(),
+			client->CharacterID(),
+			commit_result.ErrorMessage()
+		);
 	}
 }