diff --git a/.github/workflows/dashboard-ci.yml b/.github/workflows/dashboard-ci.yml index 263ea07a6..c1fa7151d 100644 --- a/.github/workflows/dashboard-ci.yml +++ b/.github/workflows/dashboard-ci.yml @@ -247,18 +247,36 @@ jobs: run: | ./acore.sh test core + # This job runs zero-conf, so the core falls back to console-only logging and + # writes no Errors.log, which is what the build workflows scrape. The grep is a + # second net in case a later change stops the updater from failing the run on a + # bad import. - name: Test authserver dry-run run: | source ./acore.sh config load cd env/dist/bin - timeout 5m ./authserver -dry-run + timeout 5m ./authserver -dry-run 2>&1 | tee "$RUNNER_TEMP/authserver-dry-run.log" + status=${PIPESTATUS[0]} + [ "$status" -eq 0 ] || exit "$status" + + if grep -qE "Applying of file .+ failed!" "$RUNNER_TEMP/authserver-dry-run.log"; then + echo "::error::A SQL update failed to apply during the authserver dry run" + exit 1 + fi continue-on-error: false - name: Test worldserver dry-run run: | source ./acore.sh config load cd env/dist/bin - timeout 5m ./worldserver -dry-run + timeout 5m ./worldserver -dry-run 2>&1 | tee "$RUNNER_TEMP/worldserver-dry-run.log" + status=${PIPESTATUS[0]} + [ "$status" -eq 0 ] || exit "$status" + + if grep -qE "Applying of file .+ failed!" "$RUNNER_TEMP/worldserver-dry-run.log"; then + echo "::error::A SQL update failed to apply during the worldserver dry run" + exit 1 + fi continue-on-error: false @@ -288,6 +306,46 @@ jobs: timeout-minutes: 30 continue-on-error: false + # Proves the dry-run steps above actually fail on a bad import. + - name: Verify a broken SQL update fails the dry run + run: | + # Absolute: the trap fires after the cd below. + probe="$PWD/data/sql/updates/pending_db_world/rev_9999999999999999999.sql" + trap 'rm -f "$probe"' EXIT + + cat > "$probe" <<'EOF' + DELETE FROM `ci_gate_probe_table_that_does_not_exist` WHERE `id` = 1; + EOF + + source ./acore.sh config load + cd env/dist/bin + + # Without this the step fails on a healthy tree as soon as it gains a + # `shell: bash`: that turns pipefail on, so errexit aborts on the exit 1 the + # assertions below are here to check for. + set +o pipefail + timeout 5m ./worldserver -dry-run 2>&1 | tee "$RUNNER_TEMP/broken-update-dry-run.log" + status=${PIPESTATUS[0]} + + # Exactly 1, so a hang (timeout returns 124) does not pass as a detection. + if [ "$status" -ne 1 ]; then + echo "::error::expected worldserver -dry-run to exit 1 on a broken SQL update, got $status" + exit 1 + fi + + # 1 is also the generic startup failure code, so require the updater to be the one + # that failed the run, on the probe file specifically. + if ! grep -qE "Applying of file .*rev_9999999999999999999\.sql.* failed!" "$RUNNER_TEMP/broken-update-dry-run.log"; then + echo "::error::worldserver -dry-run exited 1 without reporting the broken probe update" + exit 1 + fi + + if ! grep -q "Failed Database Update" "$RUNNER_TEMP/broken-update-dry-run.log"; then + echo "::error::the dry run did not terminate on the failed update count" + exit 1 + fi + timeout-minutes: 10 + # The old "only save on a cache miss" guard is dropped: the per-run key # only ever collides on a re-run, since github.run_id is stable across # attempts, and there the save is a no-op anyway. diff --git a/src/server/apps/authserver/Main.cpp b/src/server/apps/authserver/Main.cpp index f5c3b80bc..d19e2b83c 100644 --- a/src/server/apps/authserver/Main.cpp +++ b/src/server/apps/authserver/Main.cpp @@ -27,6 +27,7 @@ #include "AuthSocketMgr.h" #include "Banner.h" #include "Config.h" +#include "DBUpdater.h" #include "DatabaseEnv.h" #include "DatabaseLoader.h" #include "GitRevision.h" @@ -149,6 +150,12 @@ int main(int argc, char** argv) // Stop auth server if dry run if (sConfigMgr->isDryRun()) { + if (uint32 failed = DBUpdaterUtil::GetFailedUpdateCount()) + { + LOG_FATAL("server.authserver", "Dry run completed with {} failed database update(s), terminating.", failed); + return 1; + } + LOG_INFO("server.authserver", "Dry run completed, terminating."); return 0; } diff --git a/src/server/database/Updater/DBUpdater.cpp b/src/server/database/Updater/DBUpdater.cpp index a98cae3ff..554e568d8 100644 --- a/src/server/database/Updater/DBUpdater.cpp +++ b/src/server/database/Updater/DBUpdater.cpp @@ -64,6 +64,22 @@ std::string& DBUpdaterUtil::corrected_path() return path; } +uint32& DBUpdaterUtil::failed_updates() +{ + static uint32 count = 0; + return count; +} + +void DBUpdaterUtil::MarkUpdateFailed() +{ + ++failed_updates(); +} + +uint32 DBUpdaterUtil::GetFailedUpdateCount() +{ + return failed_updates(); +} + // Auth Database template<> std::string DBUpdater::GetConfigEntry() @@ -524,6 +540,10 @@ void DBUpdater::ApplyFile(DatabaseWorkerPool& pool, std::string const& hos "If you are a developer, please fix your sql query.", path.generic_string(), pool.GetConnectionInfo()->database); + // Recorded in both modes. A dry run does not throw below, so it keeps attempting the + // remaining files and this count is the only thing left to fail the run on. + DBUpdaterUtil::MarkUpdateFailed(); + if (!sConfigMgr->isDryRun()) { if (uint32 delay = sConfigMgr->GetOption("Updates.ExceptionShutdownDelay", 10000)) diff --git a/src/server/database/Updater/DBUpdater.h b/src/server/database/Updater/DBUpdater.h index 58ebf3320..506c549c1 100644 --- a/src/server/database/Updater/DBUpdater.h +++ b/src/server/database/Updater/DBUpdater.h @@ -60,8 +60,15 @@ public: static bool CheckExecutable(); + // Counts every update file that failed to apply, in any mode. A dry run does not throw + // on a bad file, so it keeps going and a single run reports all of them; whoever ends + // the run must check this and exit non-zero, otherwise CI goes green on a failed import. + static void MarkUpdateFailed(); + static uint32 GetFailedUpdateCount(); + private: static std::string& corrected_path(); + static uint32& failed_updates(); }; template diff --git a/src/server/game/World/World.cpp b/src/server/game/World/World.cpp index f68c524c5..ad54e8364 100644 --- a/src/server/game/World/World.cpp +++ b/src/server/game/World/World.cpp @@ -42,6 +42,7 @@ #include "CreatureGroups.h" #include "CreatureTextMgr.h" #include "DBCStores.h" +#include "DBUpdater.h" #include "DatabaseEnv.h" #include "DisableMgr.h" #include "DynamicVisibility.h" @@ -1058,6 +1059,13 @@ void World::SetInitialWorldSettings() if (sConfigMgr->isDryRun()) { sMapMgr->UnloadAll(); + + if (uint32 failed = DBUpdaterUtil::GetFailedUpdateCount()) + { + LOG_FATAL("server.loading", "AzerothCore Dry Run Completed With {} Failed Database Update(s), Terminating.", failed); + exit(1); + } + LOG_INFO("server.loading", "AzerothCore Dry Run Completed, Terminating."); exit(0); } diff --git a/src/tools/dbimport/Main.cpp b/src/tools/dbimport/Main.cpp index 06b3ce665..9daa32d17 100644 --- a/src/tools/dbimport/Main.cpp +++ b/src/tools/dbimport/Main.cpp @@ -17,6 +17,7 @@ #include "Banner.h" #include "Config.h" +#include "DBUpdater.h" #include "DatabaseEnv.h" #include "DatabaseLoader.h" #include "IoContext.h" @@ -95,6 +96,12 @@ int main(int argc, char** argv) LOG_INFO("dbimport", "Halting process..."); + if (uint32 failed = DBUpdaterUtil::GetFailedUpdateCount()) + { + LOG_FATAL("dbimport", "{} update file(s) failed to apply!", failed); + return 1; + } + return 0; }