fix(Core/Spells): revert CAST proc ordering and add ApplySpellMod recursion guard (#24990)

Co-authored-by: blinkysc <blinkysc@users.noreply.github.com>
2026-03-08 10:10:28 +00:00 · 2026-03-03 16:45:06 -06:00
parent a013968436
commit cccd52dab8
8 changed files with 36 additions and 390 deletions
--- a/data/sql/updates/db_world/2026_02_28_09.sql
+++ b/data/sql/updates/db_world/2026_02_28_09.sql
@@ -1,5 +0,0 @@
-- DB update 2026_02_28_08 -> 2026_02_28_09
-- Fingers of Frost buff: change SpellPhaseMask from 3 (CAST|HIT) to 1 (CAST only).
-- With !IsTriggered() removed from CAST proc blocks, triggered spells now fire
-- CAST procs, so HIT phase is no longer needed for triggered spell consumption.
-UPDATE `spell_proc` SET `SpellPhaseMask` = 1 WHERE `SpellId` = 74396;
--- a/data/sql/updates/db_world/2026_03_01_03.sql
+++ b/data/sql/updates/db_world/2026_03_01_03.sql
@@ -1,3 +0,0 @@
-- DB update 2026_03_01_02 -> 2026_03_01_03
-- Arcane Blast debuff: spell_proc override to consume at CAST phase via family masks (AM/AE/ABarr only)
-UPDATE `spell_proc` SET `ProcFlags`=69632, `SpellFamilyMask0`=6144, `SpellFamilyMask1`=32768, `SpellFamilyMask2`=0, `SpellPhaseMask`=1, `Charges`=1 WHERE `SpellId`=36032;
--- a/src/server/game/Spells/Auras/SpellAuraEffects.cpp
+++ b/src/server/game/Spells/Auras/SpellAuraEffects.cpp
@@ -820,6 +820,17 @@ void AuraEffect::ApplySpellMod(Unit* target, bool apply)
    // Auras with charges do not mod amount of passive auras
    if (GetBase()->IsUsingCharges())
        return;
+
+    // Guard against infinite recursion: a spell mod recalculating an aura that
+    // triggers ApplySpellMod again (self-referencing or mutual spell mods).
+    if (m_isRecalculatingPassiveAuras)
+    {
+        LOG_DEBUG("spells.aura", "AuraEffect::ApplySpellMod: Recursion detected for spell {} effect {}, skipping passive aura recalculation",
+            GetId(), GetEffIndex());
+        return;
+    }
+    m_isRecalculatingPassiveAuras = true;
+
    // reapply some passive spells after add/remove related spellmods
    // Warning: it is a dead loop if 2 auras each other amount-shouldn't happen
    switch (GetMiscValue())
@@ -906,6 +917,8 @@ void AuraEffect::ApplySpellMod(Unit* target, bool apply)
        default:
            break;
    }
+
+    m_isRecalculatingPassiveAuras = false;
 }

 void AuraEffect::Update(uint32 diff, Unit* caster)
--- a/src/server/game/Spells/Auras/SpellAuraEffects.h
+++ b/src/server/game/Spells/Auras/SpellAuraEffects.h
@@ -144,6 +144,7 @@ private:
    uint8 const m_effIndex;
    bool m_canBeRecalculated;
    bool m_isPeriodic;
+    bool m_isRecalculatingPassiveAuras = false;
 private:
    float CalcPeriodicCritChance(Unit const* caster, Unit const* target) const;

--- a/src/server/game/Spells/Auras/SpellAuras.cpp
+++ b/src/server/game/Spells/Auras/SpellAuras.cpp
@@ -2354,20 +2354,7 @@ void Aura::ConsumeProcCharges(SpellProcEntry const* procEntry)
    else if (IsUsingCharges())
    {
        if (!GetCharges())
-        {
-            // Defer removal while spell mods are being consumed,
-            // cleaned up in Spell::_cast() after handle_immediate()
-            if (GetType() == UNIT_AURA_TYPE
-                && (HasEffectType(SPELL_AURA_ADD_FLAT_MODIFIER)
-                    || HasEffectType(SPELL_AURA_ADD_PCT_MODIFIER)))
-            {
-                if (Player* player = GetUnitOwner()->ToPlayer())
-                    if (player->m_spellModTakingSpell)
-                        return;
-            }
-
            Remove();
-        }
    }
 }

--- a/src/server/game/Spells/Spell.cpp
+++ b/src/server/game/Spells/Spell.cpp
@@ -3951,70 +3951,8 @@ void Spell::_cast(bool skipCheck)
    }
    else
    {
-        // CAST phase procs for immediate spells (including channeled)
-        if (m_originalCaster)
-        {
-            uint32 procAttacker = m_procAttacker;
-            if (!procAttacker)
-            {
-                bool IsPositive = m_spellInfo->IsPositive();
-                if (m_spellInfo->DmgClass == SPELL_DAMAGE_CLASS_MAGIC)
-                {
-                    procAttacker = IsPositive ? PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_POS : PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG;
-                }
-                else
-                {
-                    procAttacker = IsPositive ? PROC_FLAG_DONE_SPELL_NONE_DMG_CLASS_POS : PROC_FLAG_DONE_SPELL_NONE_DMG_CLASS_NEG;
-                }
-            }
-
-            uint32 hitMask = PROC_HIT_NORMAL;
-
-            for (std::list<TargetInfo>::iterator ihit = m_UniqueTargetInfo.begin(); ihit != m_UniqueTargetInfo.end(); ++ihit)
-            {
-                if (ihit->missCondition != SPELL_MISS_NONE)
-                    continue;
-
-                if (!ihit->crit)
-                    continue;
-
-                hitMask |= PROC_HIT_CRITICAL;
-                break;
-            }
-
-            Unit::ProcSkillsAndAuras(m_originalCaster, m_originalCaster, procAttacker, PROC_FLAG_NONE, hitMask, 1, BASE_ATTACK, m_spellInfo, m_triggeredByAuraSpell.spellInfo,
-                m_triggeredByAuraSpell.effectIndex, this, nullptr, nullptr, PROC_SPELL_PHASE_CAST);
-        }
-
        // Immediate spell, no big deal
        handle_immediate();
-
-        // Clean up deferred 0-charge spell modifier auras
-        // Copy to vector first — aura->Remove() can modify m_appliedMods
-        std::vector<Aura*> appliedModsCopy(m_appliedMods.begin(), m_appliedMods.end());
-        for (Aura* aura : appliedModsCopy)
-        {
-            if (!aura->IsRemoved() && aura->IsUsingCharges()
-                && !aura->GetCharges())
-                aura->Remove();
-        }
-
-        // Also clean up deferred modifier auras not in m_appliedMods
-        if (Unit* caster = m_caster)
-        {
-            std::vector<Aura*> deferred;
-            for (auto const& [id, aura] : caster->GetOwnedAuras())
-            {
-                if (!aura->IsRemoved() && aura->IsUsingCharges()
-                    && !aura->GetCharges()
-                    && (aura->HasEffectType(SPELL_AURA_ADD_FLAT_MODIFIER)
-                        || aura->HasEffectType(SPELL_AURA_ADD_PCT_MODIFIER)))
-                    deferred.push_back(aura);
-            }
-            for (Aura* aura : deferred)
-                if (!aura->IsRemoved())
-                    aura->Remove();
-        }
    }

    if (resetAttackTimers)
@@ -4042,9 +3980,13 @@ void Spell::_cast(bool skipCheck)
    if (modOwner)
        modOwner->SetSpellModTakingSpell(this, false);

-    // CAST phase procs for delayed spells
-    if (m_spellState == SPELL_STATE_DELAYED
-        && m_originalCaster)
+    // Handle procs on cast - only for non-triggered spells
+    // Triggered spells (from auras, items, etc.) should not fire CAST phase procs
+    // as they are not player-initiated casts. This prevents issues like Arcane Potency
+    // charges being consumed by periodic damage effects (e.g., Blizzard ticks).
+    // Must be called AFTER handle_immediate() so spell mods (like Missile Barrage's
+    // duration reduction) are applied before the aura is consumed by the proc.
+    if (m_originalCaster && !IsTriggered())
    {
        uint32 procAttacker = m_procAttacker;
        if (!procAttacker)
--- a/src/server/scripts/Spells/spell_mage.cpp
+++ b/src/server/scripts/Spells/spell_mage.cpp
@@ -959,11 +959,23 @@ class spell_mage_fingers_of_frost : public AuraScript

    void PrepareProc(ProcEventInfo& eventInfo)
    {
-        // Block channeled spells (e.g. Blizzard channel start) from consuming charges.
-        // All other filtering is handled by SpellPhaseMask=1 (CAST only) in spell_proc.
        if (Spell const* spell = eventInfo.GetProcSpell())
-            if (spell->GetSpellInfo()->IsChanneled())
+        {
+            bool isTriggered = spell->IsTriggered();
+            bool isCastPhase = (eventInfo.GetSpellPhaseMask() & PROC_SPELL_PHASE_CAST) != 0;
+            bool isChanneled = spell->GetSpellInfo()->IsChanneled();
+            bool prevent = false;
+
+            if (isTriggered)
+                prevent = false;
+            else if (isChanneled)
+                prevent = true;
+            else if (!isCastPhase)
+                prevent = true;
+
+            if (prevent)
                PreventDefaultAction();
+        }
    }

    void OnRemove(AuraEffect const* /*aurEff*/, AuraEffectHandleModes /*mode*/)
--- a/src/test/server/game/Spells/SpellProcPhaseOrderingTest.cpp
+++ b/src/test/server/game/Spells/SpellProcPhaseOrderingTest.cpp
@@ -1,301 +0,0 @@
-/*
- * This file is part of the AzerothCore Project. See AUTHORS file for Copyright information
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program. If not, see <http://www.gnu.org/licenses/>.
- */
-
-/**
- * @file SpellProcPhaseOrderingTest.cpp
- * @brief Tests for proc phase ordering: CAST -> HIT -> FINISH
- *
- * Validates that CAST-phase procs are isolated from HIT-phase events
- * and vice versa. This is critical for correct behavior of spells like
- * Arcane Potency (consumed on CAST) not being affected by HIT events
- * from the same spell cast.
- *
- * Related fix: Non-channeled immediate spells fire CAST procs before
- * handle_immediate() to ensure CAST -> HIT -> FINISH ordering.
- */
-
-#include "ProcChanceTestHelper.h"
-#include "ProcEventInfoHelper.h"
-#include "AuraStub.h"
-#include "SpellInfoTestHelper.h"
-#include "gtest/gtest.h"
-
-using namespace testing;
-
-class SpellProcPhaseOrderingTest : public ::testing::Test
-{
-protected:
-    void SetUp() override {}
-
-    void TearDown() override
-    {
-        for (auto* si : _spellInfos)
-            delete si;
-        _spellInfos.clear();
-    }
-
-    SpellInfo* CreateSpellInfo(uint32 id)
-    {
-        auto* si = SpellInfoBuilder().WithId(id).Build();
-        _spellInfos.push_back(si);
-        return si;
-    }
-
-    std::vector<SpellInfo*> _spellInfos;
-};
-
-// =============================================================================
-// Phase Isolation: CAST-only procs must not trigger on HIT events
-// =============================================================================
-
-TEST_F(SpellProcPhaseOrderingTest, CastPhaseProc_TriggersOnCastEvent)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .Build();
-
-    auto castEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
-}
-
-TEST_F(SpellProcPhaseOrderingTest, CastPhaseProc_DoesNotTriggerOnHitEvent)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .Build();
-
-    auto hitEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, hitEvent));
-}
-
-TEST_F(SpellProcPhaseOrderingTest, CastPhaseProc_DoesNotTriggerOnFinishEvent)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .Build();
-
-    auto finishEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_FINISH)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, finishEvent));
-}
-
-// =============================================================================
-// Phase Isolation: HIT-only procs must not trigger on CAST events
-// =============================================================================
-
-TEST_F(SpellProcPhaseOrderingTest, HitPhaseProc_DoesNotTriggerOnCastEvent)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
-        .Build();
-
-    auto castEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
-}
-
-// =============================================================================
-// Charge consumption respects phase isolation
-// Simulates the Arcane Potency scenario: charges consumed on CAST phase
-// should not be consumed again when HIT phase fires later.
-// =============================================================================
-
-TEST_F(SpellProcPhaseOrderingTest, ChargeConsumedOnCast_NotConsumedAgainOnHit)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    // Proc entry configured for CAST phase only (like Arcane Potency)
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithChance(100.0f)
-        .Build();
-
-    auto aura = AuraStubBuilder()
-        .WithId(12345)
-        .WithCharges(2)
-        .Build();
-
-    // CAST phase event fires first (correct ordering)
-    auto castEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    // CAST phase matches -> proc triggers, charge consumed
-    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
-    ProcChanceTestHelper::SimulateConsumeProcCharges(aura.get(), procEntry);
-    EXPECT_EQ(aura->GetCharges(), 1);
-
-    // HIT phase event fires second
-    auto hitEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    // HIT phase does NOT match CAST-only proc -> no trigger, no charge consumed
-    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, hitEvent));
-    EXPECT_EQ(aura->GetCharges(), 1); // Still 1, not consumed
-}
-
-TEST_F(SpellProcPhaseOrderingTest, ChargeConsumedOnCast_AvailableForNextSpell)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithChance(100.0f)
-        .Build();
-
-    auto aura = AuraStubBuilder()
-        .WithId(12345)
-        .WithCharges(2)
-        .Build();
-
-    auto castEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    // First spell cast consumes one charge
-    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
-    ProcChanceTestHelper::SimulateConsumeProcCharges(aura.get(), procEntry);
-    EXPECT_EQ(aura->GetCharges(), 1);
-    EXPECT_FALSE(aura->IsRemoved());
-
-    // Second spell cast consumes last charge
-    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
-    ProcChanceTestHelper::SimulateConsumeProcCharges(aura.get(), procEntry);
-    EXPECT_EQ(aura->GetCharges(), 0);
-    EXPECT_TRUE(aura->IsRemoved());
-}
-
-// =============================================================================
-// Multi-phase procs (CAST | HIT) trigger on both phases
-// =============================================================================
-
-TEST_F(SpellProcPhaseOrderingTest, MultiPhaseProc_TriggersOnBothCastAndHit)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    auto procEntry = SpellProcEntryBuilder()
-        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST | PROC_SPELL_PHASE_HIT)
-        .Build();
-
-    auto castEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    auto hitEvent = ProcEventInfoBuilder()
-        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
-        .WithHitMask(PROC_HIT_NORMAL)
-        .WithDamageInfo(&damageInfo)
-        .Build();
-
-    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
-    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, hitEvent));
-}
-
-// =============================================================================
-// All three phases are distinct
-// =============================================================================
-
-TEST_F(SpellProcPhaseOrderingTest, AllThreePhases_MutuallyExclusive)
-{
-    auto* spellInfo = CreateSpellInfo(1);
-    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
-
-    uint32 phases[] = { PROC_SPELL_PHASE_CAST, PROC_SPELL_PHASE_HIT, PROC_SPELL_PHASE_FINISH };
-
-    for (uint32 procPhase : phases)
-    {
-        auto procEntry = SpellProcEntryBuilder()
-            .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-            .WithSpellPhaseMask(procPhase)
-            .Build();
-
-        for (uint32 eventPhase : phases)
-        {
-            auto event = ProcEventInfoBuilder()
-                .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
-                .WithSpellPhaseMask(eventPhase)
-                .WithHitMask(PROC_HIT_NORMAL)
-                .WithDamageInfo(&damageInfo)
-                .Build();
-
-            if (procPhase == eventPhase)
-                EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, event))
-                    << "Phase " << procPhase << " should match itself";
-            else
-                EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, event))
-                    << "Phase " << procPhase << " should not match phase " << eventPhase;
-        }
-    }
-}