From 7da9bd74eb251d2b0de0548f3a91de91884b17a9 Mon Sep 17 00:00:00 2001 From: Vayle Date: Sun, 8 Mar 2026 11:33:31 -0400 Subject: [PATCH 01/25] Enforce suppressed buff persistence schema --- common/database/database_update_manifest.h | 11 ++ zone/cli/tests/cli_zone_state.cpp | 112 +++++++++++++++++++++ zone/zonedb.cpp | 11 +- 3 files changed, 133 insertions(+), 1 deletion(-) diff --git a/common/database/database_update_manifest.h b/common/database/database_update_manifest.h index a32ec7db8..2671ad4ce 100644 --- a/common/database/database_update_manifest.h +++ b/common/database/database_update_manifest.h @@ -7190,6 +7190,17 @@ ALTER TABLE `character_parcels_containers` )", .content_schema_update = false }, + ManifestEntry{ + .version = 9329, + .description = "2025_06_27_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`; +)" + }, // -- template; copy/paste this when you need to create a new entry // ManifestEntry{ // .version = 9228, diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 881873e61..5749766d8 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/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,115 @@ inline void TestHpManaEnd() ); } +inline void TestClientBuffPersistence() +{ + constexpr uint32 test_character_id = 99999991; + constexpr uint16 normal_spell_id = 6824; + constexpr uint16 suppressed_spell_id = 6824; + + 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"); + 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 +1241,7 @@ void ZoneCLI::TestZoneState(int argc, char **argv, argh::parser &cmd, std::strin TestZoneVariables(); TestHpManaEnd(); + TestClientBuffPersistence(); TestBuffs(); TestLocationChange(); TestEntityVariables(); diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index ee932e9a1..91c466d20 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2934,7 +2934,16 @@ void ZoneDatabase::SaveBuffs(Client *client) } if (!v.empty()) { - CharacterBuffsRepository::ReplaceMany(database, v); + const auto saved_count = CharacterBuffsRepository::ReplaceMany(database, v); + if (saved_count != static_cast(v.size())) { + LogError( + "Failed to save all buffs for character [{}] [{}]. Expected [{}] rows, saved [{}]. Verify the `character_buffs` schema is up to date.", + client->GetCleanName(), + client->CharacterID(), + v.size(), + saved_count + ); + } } } From ee49bf5cd9328bd0c7f5712689da83c8a4cb08fb Mon Sep 17 00:00:00 2001 From: Vayle <76063792+Valorith@users.noreply.github.com> Date: Sun, 8 Mar 2026 11:43:58 -0400 Subject: [PATCH 03/25] Update zone/cli/tests/cli_zone_state.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- zone/cli/tests/cli_zone_state.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 5749766d8..f3c0833e6 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -895,6 +895,7 @@ inline void TestClientBuffPersistence() Client loader; loader.SetCharacterId(test_character_id); loader.SetName("buff-persistence-load"); + loader.SetClientVersion(ClientVersion::RoF2); database.LoadBuffs(&loader); auto loaded_buffs = loader.GetBuffs(); From 1c7a914083ca08f27e0ed9ccfd8f398f26e9ea4e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 15:44:09 +0000 Subject: [PATCH 04/25] Add vcpkg download cache and retry steps to linux CI job Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- .github/workflows/build.yaml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 7ad7003ef..bc5246131 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -52,6 +52,33 @@ jobs: - name: Mark workspace safe run: git config --global --add safe.directory "$GITHUB_WORKSPACE" + - name: Cache vcpkg downloads + uses: actions/cache@v4 + with: + path: submodules/vcpkg/downloads + key: ${{ runner.os }}-vcpkg-downloads-${{ hashFiles('submodules/vcpkg/vcpkg.json', 'submodules/vcpkg/ports/**', 'submodules/vcpkg/versions/**') }} + restore-keys: | + ${{ runner.os }}-vcpkg-downloads- + + - name: vcpkg install (retry) + working-directory: ${{ github.workspace }} + run: | + set -e + cd submodules/vcpkg + ./bootstrap-vcpkg.sh -disableMetrics + + for i in 1 2 3 4 5; do + echo "vcpkg install attempt $i..." + if ./vcpkg install --triplet x64-linux; then + exit 0 + fi + echo "vcpkg install failed; sleeping before retry..." + sleep $((i * 10)) + done + + echo "vcpkg install failed after retries" + exit 1 + - name: Build working-directory: ${{ github.workspace }} run: | From 15c15f0687f34fd7279d7a295b08b9786fd52693 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 15:46:48 +0000 Subject: [PATCH 07/25] Fix false error log: use < instead of != when checking ReplaceMany result in SaveBuffs Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/zonedb.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 91c466d20..6a1607086 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2935,9 +2935,9 @@ void ZoneDatabase::SaveBuffs(Client *client) if (!v.empty()) { const auto saved_count = CharacterBuffsRepository::ReplaceMany(database, v); - if (saved_count != static_cast(v.size())) { + if (saved_count < static_cast(v.size())) { LogError( - "Failed to save all buffs for character [{}] [{}]. Expected [{}] rows, saved [{}]. Verify the `character_buffs` schema is up to date.", + "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(), From e359852ccf7fa64b3d492c868536819d539529fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 15:47:13 +0000 Subject: [PATCH 08/25] Wrap SaveBuffs delete+replace in a transaction to prevent buff wipe on insert failure Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/zonedb.cpp | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 91c466d20..8ff5277b7 100644 --- a/zone/zonedb.cpp +++ b/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,9 +2925,20 @@ void ZoneDatabase::SaveBuffs(Client *client) v.emplace_back(e); } + database.TransactionBegin(); + + CharacterBuffsRepository::DeleteWhere( + database, + fmt::format( + "`character_id` = {}", + client->CharacterID() + ) + ); + if (!v.empty()) { const auto saved_count = CharacterBuffsRepository::ReplaceMany(database, v); if (saved_count != static_cast(v.size())) { + database.TransactionRollback(); LogError( "Failed to save all buffs for character [{}] [{}]. Expected [{}] rows, saved [{}]. Verify the `character_buffs` schema is up to date.", client->GetCleanName(), @@ -2943,8 +2946,11 @@ void ZoneDatabase::SaveBuffs(Client *client) v.size(), saved_count ); + return; } } + + database.TransactionCommit(); } void ZoneDatabase::LoadBuffs(Client *client) From 6290ef42c6d0ef39158fc5526fae1f74ca3d8663 Mon Sep 17 00:00:00 2001 From: Vayle <76063792+Valorith@users.noreply.github.com> Date: Sun, 8 Mar 2026 12:22:30 -0400 Subject: [PATCH 09/25] Update zone/zonedb.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- zone/zonedb.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 8ff5277b7..40baef1c7 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2950,7 +2950,15 @@ void ZoneDatabase::SaveBuffs(Client *client) } } - database.TransactionCommit(); + const auto commit_result = database.TransactionCommit(); + if (!commit_result) { + database.TransactionRollback(); + LogError( + "Failed to commit buff save transaction for character [{}] [{}].", + client->GetCleanName(), + client->CharacterID() + ); + } } void ZoneDatabase::LoadBuffs(Client *client) From bd92f19869312cc4134a3c7d5eb8043671c53571 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:25:35 +0000 Subject: [PATCH 10/25] Fix build error: use fully-qualified EQ::versions::ClientVersion::RoF2; use < for REPLACE row count check Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/cli/tests/cli_zone_state.cpp | 2 +- zone/zonedb.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index f3c0833e6..1eb391a81 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -895,7 +895,7 @@ inline void TestClientBuffPersistence() Client loader; loader.SetCharacterId(test_character_id); loader.SetName("buff-persistence-load"); - loader.SetClientVersion(ClientVersion::RoF2); + loader.SetClientVersion(EQ::versions::ClientVersion::RoF2); database.LoadBuffs(&loader); auto loaded_buffs = loader.GetBuffs(); diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 40baef1c7..5cac91b4c 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2937,7 +2937,7 @@ void ZoneDatabase::SaveBuffs(Client *client) if (!v.empty()) { const auto saved_count = CharacterBuffsRepository::ReplaceMany(database, v); - if (saved_count != static_cast(v.size())) { + if (saved_count < static_cast(v.size())) { database.TransactionRollback(); LogError( "Failed to save all buffs for character [{}] [{}]. Expected [{}] rows, saved [{}]. Verify the `character_buffs` schema is up to date.", From 2e62fb37bf4c7d489e7683372132424548a480f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:56:47 +0000 Subject: [PATCH 11/25] Fix zonedb.cpp: use commit_result.Success() instead of !commit_result for MySQLRequestResult Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/zonedb.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 5cac91b4c..9f0fa0033 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2951,12 +2951,13 @@ void ZoneDatabase::SaveBuffs(Client *client) } const auto commit_result = database.TransactionCommit(); - if (!commit_result) { + if (!commit_result.Success()) { database.TransactionRollback(); LogError( - "Failed to commit buff save transaction for character [{}] [{}].", + "Failed to commit buff save transaction for character [{}] [{}]: {}", client->GetCleanName(), - client->CharacterID() + client->CharacterID(), + commit_result.ErrorMessage() ); } } From 43d3a631a0860ecb64834ac88c0e5b627cbe3096 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 17:58:47 +0000 Subject: [PATCH 12/25] Fix build: use EQ::versions::ClientVersion::RoF2 in cli_zone_state test Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/cli/tests/cli_zone_state.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index f3c0833e6..1eb391a81 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -895,7 +895,7 @@ inline void TestClientBuffPersistence() Client loader; loader.SetCharacterId(test_character_id); loader.SetName("buff-persistence-load"); - loader.SetClientVersion(ClientVersion::RoF2); + loader.SetClientVersion(EQ::versions::ClientVersion::RoF2); database.LoadBuffs(&loader); auto loaded_buffs = loader.GetBuffs(); From fcd03df1f86ed5146c76737afdbc1d45cb5197a5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 18:50:43 +0000 Subject: [PATCH 13/25] Resolve conflicts: adopt base branch error message wording in SaveBuffs Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/zonedb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 9f0fa0033..61229916c 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2940,7 +2940,7 @@ void ZoneDatabase::SaveBuffs(Client *client) if (saved_count < static_cast(v.size())) { database.TransactionRollback(); LogError( - "Failed to save all buffs for character [{}] [{}]. Expected [{}] rows, saved [{}]. Verify the `character_buffs` schema is up to date.", + "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(), From ad52a2f73956de965fa4d5b13a651b28d1863b87 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 19:12:31 +0000 Subject: [PATCH 14/25] Merge base branch: resolve zone/zonedb.cpp conflict by keeping transaction rollback Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- .github/workflows/build.yaml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 7ad7003ef..bc5246131 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -52,6 +52,33 @@ jobs: - name: Mark workspace safe run: git config --global --add safe.directory "$GITHUB_WORKSPACE" + - name: Cache vcpkg downloads + uses: actions/cache@v4 + with: + path: submodules/vcpkg/downloads + key: ${{ runner.os }}-vcpkg-downloads-${{ hashFiles('submodules/vcpkg/vcpkg.json', 'submodules/vcpkg/ports/**', 'submodules/vcpkg/versions/**') }} + restore-keys: | + ${{ runner.os }}-vcpkg-downloads- + + - name: vcpkg install (retry) + working-directory: ${{ github.workspace }} + run: | + set -e + cd submodules/vcpkg + ./bootstrap-vcpkg.sh -disableMetrics + + for i in 1 2 3 4 5; do + echo "vcpkg install attempt $i..." + if ./vcpkg install --triplet x64-linux; then + exit 0 + fi + echo "vcpkg install failed; sleeping before retry..." + sleep $((i * 10)) + done + + echo "vcpkg install failed after retries" + exit 1 + - name: Build working-directory: ${{ github.workspace }} run: | From d93af715953c909953bd8f5a261efe6eb4903ac3 Mon Sep 17 00:00:00 2001 From: Vayle <76063792+Valorith@users.noreply.github.com> Date: Sun, 8 Mar 2026 19:47:17 -0400 Subject: [PATCH 15/25] Update .github/workflows/build.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index bc5246131..731c960ab 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -56,7 +56,7 @@ jobs: uses: actions/cache@v4 with: path: submodules/vcpkg/downloads - key: ${{ runner.os }}-vcpkg-downloads-${{ hashFiles('submodules/vcpkg/vcpkg.json', 'submodules/vcpkg/ports/**', 'submodules/vcpkg/versions/**') }} + key: ${{ runner.os }}-vcpkg-downloads-${{ hashFiles('vcpkg.json', 'submodules/vcpkg/ports/**', 'submodules/vcpkg/versions/**') }} restore-keys: | ${{ runner.os }}-vcpkg-downloads- From d0e1c027e928fe84af5647bec040a7c2e96d9b6d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Mar 2026 23:54:04 +0000 Subject: [PATCH 17/25] Address PR review: check transaction/delete results, fix manifest, use distinct spell IDs in test Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- common/database/database_update_manifest.h | 5 +++-- zone/cli/tests/cli_zone_state.cpp | 6 ++--- zone/zonedb.cpp | 26 ++++++++++++++++++---- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/common/database/database_update_manifest.h b/common/database/database_update_manifest.h index 2671ad4ce..1cce442f0 100644 --- a/common/database/database_update_manifest.h +++ b/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{ diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 1eb391a81..2a8591341 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/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( diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 61229916c..4dfbdec29 100644 --- a/zone/zonedb.cpp +++ b/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); From 44ab4f0a0fea1f01ec2026e0f0da8351c4a1389b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 00:06:07 +0000 Subject: [PATCH 18/25] Add rollback regression test: verify pre-existing buffs survive failed SaveBuffs() Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/cli/tests/cli_zone_state.cpp | 102 ++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 2a8591341..040cbd49e 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -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(); From 69dc2ba2460b0ec62ee343a400e201f194bd2ebf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 00:45:22 +0000 Subject: [PATCH 20/25] Fix vcpkg install step to run from repo root so vcpkg.json manifest is found Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- .github/workflows/build.yaml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 731c960ab..a460f702d 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -64,12 +64,11 @@ jobs: working-directory: ${{ github.workspace }} run: | set -e - cd submodules/vcpkg - ./bootstrap-vcpkg.sh -disableMetrics + ./submodules/vcpkg/bootstrap-vcpkg.sh -disableMetrics for i in 1 2 3 4 5; do echo "vcpkg install attempt $i..." - if ./vcpkg install --triplet x64-linux; then + if ./submodules/vcpkg/vcpkg install --triplet x64-linux; then exit 0 fi echo "vcpkg install failed; sleeping before retry..." From 2666d624eb71fb79ed91d9e341b4e8be5f4396a1 Mon Sep 17 00:00:00 2001 From: Vayle <76063792+Valorith@users.noreply.github.com> Date: Sun, 8 Mar 2026 20:51:07 -0400 Subject: [PATCH 22/25] Update zone/cli/tests/cli_zone_state.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- zone/cli/tests/cli_zone_state.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 040cbd49e..f7de11b38 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -1013,6 +1013,10 @@ inline void TestClientBuffPersistenceRollback() "CHANGE `suppressed_bkp` `suppressed` tinyint(1) unsigned NOT NULL DEFAULT 0" ); RunTest("Client Buff Persistence > Rollback: column restore succeeded", true, restore_result.Success()); + if (!restore_result.Success()) { + // Fail fast to avoid running further tests with a broken schema + return; + } // Pre-existing rows must still be present because the transaction was rolled back const auto rows_after = CharacterBuffsRepository::GetWhere( From cd23894b80bbea06dc5f64240ac1014ba1ef5aac Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 00:52:49 +0000 Subject: [PATCH 23/25] Gate DDL rollback test behind --run-ddl-tests flag to avoid table locks on production databases Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- zone/cli/tests/cli_zone_state.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 040cbd49e..fd8183e5a 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -936,8 +936,19 @@ inline void TestClientBuffPersistence() ); } -inline void TestClientBuffPersistenceRollback() +inline void TestClientBuffPersistenceRollback(bool run_ddl) { + // DDL on the live character_buffs table can lock it and disrupt running servers. + // Only run when the caller explicitly opts in via --run-ddl-tests. + if (!run_ddl) { + RunTest( + "Client Buff Persistence > Rollback: skipped (pass --run-ddl-tests to enable)", + true, + true + ); + return; + } + 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; @@ -1344,7 +1355,7 @@ void ZoneCLI::TestZoneState(int argc, char **argv, argh::parser &cmd, std::strin TestZoneVariables(); TestHpManaEnd(); TestClientBuffPersistence(); - TestClientBuffPersistenceRollback(); + TestClientBuffPersistenceRollback(cmd["--run-ddl-tests"]); TestBuffs(); TestLocationChange(); TestEntityVariables(); From d8edd960f9a65dc6ef9266e21479064263345e78 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Mar 2026 01:11:23 +0000 Subject: [PATCH 25/25] Address review: consistent TransactionBegin, remove DDL rollback test, revert build.yaml vcpkg steps Co-authored-by: Valorith <76063792+Valorith@users.noreply.github.com> --- .github/workflows/build.yaml | 26 ------- zone/cli/tests/cli_zone_state.cpp | 117 ------------------------------ zone/zonedb.cpp | 14 +--- 3 files changed, 4 insertions(+), 153 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a460f702d..7ad7003ef 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -52,32 +52,6 @@ jobs: - name: Mark workspace safe run: git config --global --add safe.directory "$GITHUB_WORKSPACE" - - name: Cache vcpkg downloads - uses: actions/cache@v4 - with: - path: submodules/vcpkg/downloads - key: ${{ runner.os }}-vcpkg-downloads-${{ hashFiles('vcpkg.json', 'submodules/vcpkg/ports/**', 'submodules/vcpkg/versions/**') }} - restore-keys: | - ${{ runner.os }}-vcpkg-downloads- - - - name: vcpkg install (retry) - working-directory: ${{ github.workspace }} - run: | - set -e - ./submodules/vcpkg/bootstrap-vcpkg.sh -disableMetrics - - for i in 1 2 3 4 5; do - echo "vcpkg install attempt $i..." - if ./submodules/vcpkg/vcpkg install --triplet x64-linux; then - exit 0 - fi - echo "vcpkg install failed; sleeping before retry..." - sleep $((i * 10)) - done - - echo "vcpkg install failed after retries" - exit 1 - - name: Build working-directory: ${{ github.workspace }} run: | diff --git a/zone/cli/tests/cli_zone_state.cpp b/zone/cli/tests/cli_zone_state.cpp index 3a4e3546b..2a8591341 100644 --- a/zone/cli/tests/cli_zone_state.cpp +++ b/zone/cli/tests/cli_zone_state.cpp @@ -936,122 +936,6 @@ inline void TestClientBuffPersistence() ); } -inline void TestClientBuffPersistenceRollback(bool run_ddl) -{ - // DDL on the live character_buffs table can lock it and disrupt running servers. - // Only run when the caller explicitly opts in via --run-ddl-tests. - if (!run_ddl) { - RunTest( - "Client Buff Persistence > Rollback: skipped (pass --run-ddl-tests to enable)", - true, - true - ); - return; - } - - 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()); - if (!restore_result.Success()) { - // Fail fast to avoid running further tests with a broken schema - return; - } - - // 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()) { @@ -1359,7 +1243,6 @@ void ZoneCLI::TestZoneState(int argc, char **argv, argh::parser &cmd, std::strin TestZoneVariables(); TestHpManaEnd(); TestClientBuffPersistence(); - TestClientBuffPersistenceRollback(cmd["--run-ddl-tests"]); TestBuffs(); TestLocationChange(); TestEntityVariables(); diff --git a/zone/zonedb.cpp b/zone/zonedb.cpp index 4dfbdec29..b39203ce2 100644 --- a/zone/zonedb.cpp +++ b/zone/zonedb.cpp @@ -2925,16 +2925,7 @@ void ZoneDatabase::SaveBuffs(Client *client) v.emplace_back(e); } - 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; - } + database.TransactionBegin(); const auto delete_result = database.QueryDatabase( fmt::format( @@ -2954,6 +2945,9 @@ void ZoneDatabase::SaveBuffs(Client *client) } if (!v.empty()) { + // 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(v.size())) { database.TransactionRollback();