fix(aura): let a duration rule edit an aura its own trigger creates

Turtle's Bestial Wrath gives the hunter's pet Scent of Blood for 18s, but
C_UnitAuras reported 8s and then dropped the timer entirely while the buff
was still up.

Server-side (tortoise-wow spell_hunter_bestial_wrath) Bestial Wrath neither
applies the aura nor carries its duration on the wire: effect 0 is
TRIGGER_SPELL(Scent of Blood, 52995), and the script then stretches the
holder that trigger just created via SetAuraMaxDuration/SetAuraDuration.
Both are plain field writes, and 1.12's only duration packet is scoped to an
aura-bearer that is a PLAYER, which a pet is not -- so the client never hears
the 18s and keeps the 8s base it computed when the aura landed. Exactly the
class of edit Aura::Source's duration rules exist to mirror; two things had
to change before one could express it.

The affected-aura selector demanded a SpellFamilyFlags overlap, and every
Turtle custom spell carries flags of 0 (308 in the hunter family alone), so
no mask could ever name this aura. It now takes a family-flag overlap and/or
a SpellIconID, at least one of the two -- an icon is shared across a spell's
whole rank ladder, so it stays as rank-proof as a flag, and icon 2245 picks
out exactly the four Scent of Blood records and nothing else. Registrars
reject the mask-0 + icon-0 rule that would match a whole class family, and
RefreshDurationByFamily takes the same invariant so the two entry points
cannot disagree about what they match.

A trigger can also CREATE the aura it edits, which the immediate-apply model
could not see. The trigger's SMSG_SPELL_GO arrives before the triggered
cast's, so editing at trigger time either matches nothing or is overwritten
moments later when the triggered cast stores its own base duration -- the
edit is lost either way. Idempotent ops now ARM the target as well, and the
next matching store re-applies the edit, from the cast path or the
application path, whichever the aura arrives on. Only refresh and set arm:
re-running those lands on the same value, while reduce would shave twice and
remove would kill an aura that was legitimately re-applied, so Conflagrate
behaves exactly as before.

The rule reads Bestial Wrath's own duration from the client's spell data
instead of hardcoding the server's 18s, so a rebalance that patches the DBC
carries over on its own.

Fixes brues-code/pfUI#50

Author: brues-code

docs/API.md

@@ -15629,20 +15629,24 @@ trigger lands:
 | `triggerFamily` | `SpellFamilyName` of the triggering cast (e.g. `6` priest). |
 | `triggerSchool` | School index the trigger must be (`0` physical … `2` fire, `5` shadow, `6` arcane), or `< 0` for any. Matching by family + school covers a whole class of spells at once — every rank, plus server-added ones — rather than a named ability. |
 | `affectedFamily` | `SpellFamilyName` of the affected aura (e.g. `5` warlock, `11` shaman, `6` priest). |
-| `affectedFamilyFlags` | A `SpellFamilyFlags` bitmask; the affected aura matches if it overlaps. Family + flag is rank-proof (covers every rank at once). |
-| `affectedIcon` | `SpellIconID` the affected aura must have, or `0` to match any. |
+| `affectedFamilyFlags` | A `SpellFamilyFlags` bitmask; the affected aura matches if it overlaps. Family plus flag is rank-proof, so it covers every rank at once. Pass `0` to select on the icon alone. |
+| `affectedIcon` | `SpellIconID` the affected aura must have, or `0` to match any. Give this when the aura has no `SpellFamilyFlags`, which is common for a server's custom spells. An icon is shared by a spell's whole rank ladder, so it stays rank-proof. |
 | `op` | `"refresh"` (reset to full duration), `"reduce"` (subtract `valueSeconds`, removing the aura if it would go non-positive), `"set"` (to `valueSeconds`), `"remove"`. |
 | `valueSeconds` | Amount for `reduce`/`set`; ignored otherwise. |
 
+Give at least one of `affectedFamilyFlags` and `affectedIcon`. A rule with
+neither would match every aura of the class, so it is rejected.
+
 The rule only fires for the aura **cast by the same unit** as the trigger
 (these mechanics act on the caster's own DoT), and misses are excluded for
 free (a missed trigger isn't in the packet's hit list).
 
 Rules keyed to an **exact trigger spellID** (rather than a family + school
 category) are registered inside the DLL instead. `!!!ClassicAPI`'s built-in
 Turtle mods — Conflagrate shaving 3s off the caster's Immolate, Molten Blast
-refreshing the caster's Flame Shock — live in `src/turtle/DurationMods.cpp`,
-and Carnage's roll-gated Rip/Rake refresh in `src/turtle/Carnage.cpp`.
+refreshing the caster's Flame Shock, and Bestial Wrath stretching the pet's
+Scent of Blood to its own length — live in `src/turtle/DurationMods.cpp`, and
+Carnage's roll-gated Rip/Rake refresh in `src/turtle/Carnage.cpp`.
 
 The one rule registered from Lua
 ([Util/AuraDurationModifiers.lua](../AddOns/!!!ClassicAPI/Util/AuraDurationModifiers.lua))

src/aura/Source.cpp

@@ -360,6 +360,12 @@ Entry *Claim(uint32_t now) {
 // refresh path too.
 void SignalAuraChanged(uint64_t guid);
 
+// Defined with the duration-modifier machinery below. A trigger can CREATE the
+// aura it then edits, in which case the edit is armed at trigger time and
+// applied here, when the aura it was waiting for actually lands. See PendingMod.
+struct Entry;
+void ConsumePendingMods(Entry &e, uint32_t now);
+
 // The SpellGo hook: authoritative caster + caster-modified (talented) timing.
 // Identity is `(target, spell, caster)`, so a second caster of the same spell
 // opens its own entry instead of overwriting the first's, and a recast by the
@@ -387,6 +393,7 @@ void StoreFromCast(uint64_t targetGuid, uint32_t spellId, uint64_t casterGuid,
         e = Claim(now);
         *e = {targetGuid, casterGuid, spellId, expirationMs, durationMs,
               now,        SLOT_UNBOUND, KIND_UNKNOWN, true};
+        ConsumePendingMods(*e, now);
         return;
     }
     e->stampMs = now; // refresh EvictAbsent grace on any touch
@@ -399,6 +406,7 @@ void StoreFromCast(uint64_t targetGuid, uint32_t spellId, uint64_t casterGuid,
     // ourselves (deferred to the world tick, coalesced). A NEW application
     // doesn't need this: its descriptor write fires the engine's own event.
     SignalAuraChanged(targetGuid);
+    ConsumePendingMods(*e, now);
 }
 
 // The OnAuraAdded / OnAuraStacksChanged application hooks: a descriptor slot,
@@ -429,6 +437,7 @@ void StoreFromApplication(uint64_t targetGuid, uint32_t spellId,
         e = Claim(now);
         *e = {targetGuid, casterGuid, spellId, expirationMs, durationMs, now,
               static_cast<int16_t>(slot), kind, true};
+        ConsumePendingMods(*e, now);
         return;
     }
 
@@ -440,6 +449,11 @@ void StoreFromApplication(uint64_t targetGuid, uint32_t spellId,
     // back to unknown.
     if (kind != KIND_UNKNOWN)
         e->kind = kind;
+    // An armed server edit applies whoever owns the timing — the server made it
+    // AFTER the cast this entry came from, so it is the newer truth. It also
+    // adopts the entry, which makes the ownership guard below keep the edited
+    // value instead of the application's base-duration guess.
+    ConsumePendingMods(*e, now);
     if (e->casterGuid != 0)
         return; // SpellGo owns this entry; keep its caster + talented timing
     if (casterGuid != 0)
@@ -523,8 +537,10 @@ void Evict(uint64_t targetGuid, uint32_t spellId, int slot) {
 //   Molten Blast -> Flame Shock:  refresh    (RefreshHolder → reset to max)
 // A trigger is matched either by exact spellID (from the server's script
 // binding, stable across ranks) or by SpellFamilyName + school; the affected
-// aura by SpellFamilyName + a family-flag overlap (+ optional icon) — rank-proof,
-// exactly how the server's scripts find it. Conflagrate's *full*-consume path
+// aura by SpellFamilyName plus a family-flag overlap and/or a SpellIconID —
+// rank-proof, exactly how the server's scripts find it, and the icon half is
+// what reaches a custom spell with no family flags at all (see
+// AffectedMatchesRaw). Conflagrate's *full*-consume path
 // removes Immolate, which clears
 // the descriptor slot → OnAuraRemoved already handles it; the reduce rule
 // covers the keep-ticking case. Probabilistic refreshes (Carnage's roll) are
@@ -549,9 +565,23 @@ int g_modCount = 0;
 
 // The affected-aura selector alone, shared with the Lua-facing by-family
 // refresh so the two cannot disagree about what they match.
+//
+// SpellFamilyName always has to match. Past that a rule discriminates by a
+// SpellFamilyFlags overlap, by SpellIconID, or by both; a zero mask or a zero
+// icon drops that half of the test. The icon-only form is what names an aura
+// whose SpellFamilyFlags are ZERO — every one of Turtle's custom spells is
+// (308 in the hunter family alone), so the mask cannot select them while the
+// icon still can, and an icon is shared by a spell's whole rank ladder, which
+// keeps it rank-proof exactly like a family flag. Registrars reject the
+// degenerate mask-0 + icon-0 rule, which would match a whole class family.
 bool AffectedMatchesRaw(const uint8_t *rec, uint32_t family, uint64_t mask,
                         uint32_t icon) {
-    if (!Spell::Lookup::IsFitToFamily(rec, family, mask))
+    if (rec == nullptr)
+        return false;
+    if (*reinterpret_cast<const uint32_t *>(
+            rec + Offsets::OFF_SPELL_RECORD_FAMILY_NAME) != family)
+        return false;
+    if (mask != 0 && !Spell::Lookup::IsFitToFamily(rec, family, mask))
         return false;
     if (icon != 0 &&
         *reinterpret_cast<const uint32_t *>(
@@ -616,23 +646,23 @@ void FlushAuraSignals() {
     g_pendingSignalCount = 0;
 }
 
-void ApplyMod(Entry &e, const DurationMod &m, uint32_t now) {
-    switch (m.op) {
+void ApplyModOp(Entry &e, int32_t op, int32_t valueMs, uint32_t now) {
+    switch (op) {
     case MOD_REFRESH:
         if (e.durationMs > 0) {
             e.expirationMs = now + e.durationMs; // RefreshHolder → reset to max
             SignalAuraChanged(e.targetGuid);
         }
         break;
     case MOD_SET:
-        e.durationMs = static_cast<uint32_t>(m.valueMs);
-        e.expirationMs = now + static_cast<uint32_t>(m.valueMs);
+        e.durationMs = static_cast<uint32_t>(valueMs);
+        e.expirationMs = now + static_cast<uint32_t>(valueMs);
         SignalAuraChanged(e.targetGuid);
         break;
     case MOD_REDUCE:
         if (e.expirationMs != 0) {
-            if (e.expirationMs > now + static_cast<uint32_t>(m.valueMs)) {
-                e.expirationMs -= static_cast<uint32_t>(m.valueMs);
+            if (e.expirationMs > now + static_cast<uint32_t>(valueMs)) {
+                e.expirationMs -= static_cast<uint32_t>(valueMs);
                 SignalAuraChanged(e.targetGuid);
             } else {
                 e.used = false; // shaved to/past now → server removes it
@@ -645,6 +675,88 @@ void ApplyMod(Entry &e, const DurationMod &m, uint32_t now) {
     }
 }
 
+void ApplyMod(Entry &e, const DurationMod &m, uint32_t now) {
+    ApplyModOp(e, m.op, m.valueMs, now);
+}
+
+// ---- Deferred duration edits (the trigger creates the aura it edits) ------
+//
+// A trigger does not always act on an aura that is ALREADY there. Bestial
+// Wrath's effect 0 is TRIGGER_SPELL(Scent of Blood), and the server's script
+// then stretches the very holder that trigger just created (tortoise-wow
+// spell_hunter_bestial_wrath). The trigger's own SMSG_SPELL_GO reaches us
+// first, so at trigger time either nothing matches yet, or something matches
+// and is overwritten moments later when the triggered cast stores its own base
+// duration. Either way the edit is lost.
+//
+// So a trigger also ARMS its targets, and the next matching store inside the
+// window re-applies the edit — from the cast path or the application path,
+// whichever the triggered aura arrives on.
+//
+// Only the IDEMPOTENT ops arm. Re-running refresh or set lands on the same
+// value, while reduce would shave a second time and remove would kill an aura
+// that was legitimately re-applied. That keeps Conflagrate (reduce) behaving
+// exactly as before.
+bool OpIsIdempotent(int32_t op) {
+    return op == MOD_REFRESH || op == MOD_SET;
+}
+
+struct PendingMod {
+    uint64_t target;
+    uint64_t caster;
+    uint32_t affectedFamily;
+    uint64_t affectedMask;
+    uint32_t affectedIcon;
+    int32_t op;
+    int32_t valueMs;
+    uint32_t untilMs;
+    bool used;
+};
+// The triggered cast follows its trigger within one packet burst, so the window
+// only has to cover network jitter. Sized like the other arm tables.
+constexpr int kPendingModMax = 8;
+constexpr uint32_t kPendingModTtlMs = 2000;
+PendingMod g_pendingMods[kPendingModMax];
+
+void ArmPendingMod(uint64_t target, uint64_t caster, const DurationMod &m,
+                   uint32_t now) {
+    PendingMod *slot = nullptr;
+    for (auto &p : g_pendingMods) {
+        // Re-arming the same rule on the same target refreshes it in place.
+        if (p.used && p.target == target && p.affectedFamily == m.affectedFamily &&
+            p.affectedMask == m.affectedMask && p.affectedIcon == m.affectedIcon) {
+            slot = &p;
+            break;
+        }
+        if (slot == nullptr && (!p.used || Time::Clock::Reached(now, p.untilMs)))
+            slot = &p;
+    }
+    if (slot == nullptr)
+        slot = &g_pendingMods[0]; // all live — steal the first
+    *slot = {target,   caster,  m.affectedFamily,     m.affectedMask,
+             m.affectedIcon, m.op, m.valueMs, now + kPendingModTtlMs, true};
+}
+
+void ConsumePendingMods(Entry &e, uint32_t now) {
+    for (auto &p : g_pendingMods) {
+        if (!p.used || p.target != e.targetGuid ||
+            Time::Clock::Reached(now, p.untilMs))
+            continue;
+        // Caster-scoped exactly like the immediate path, adoption included.
+        if (e.casterGuid != 0 && e.casterGuid != p.caster)
+            continue;
+        if (!AffectedMatchesRaw(
+                Spell::Lookup::RecordForID(static_cast<int>(e.spellId)),
+                p.affectedFamily, p.affectedMask, p.affectedIcon))
+            continue;
+        if (e.casterGuid == 0)
+            e.casterGuid = p.caster;
+        ApplyModOp(e, p.op, p.valueMs, now);
+        p.used = false; // one-shot, like the server's single-holder edit
+        return;
+    }
+}
+
 // On a trigger cast landing on its hit targets, mirror the server's duration
 // edit on the caster's own matching cached aura. Called from SpellGo_h before
 // the aura gate — triggers (Conflagrate, Molten Blast) apply no aura of their
@@ -681,6 +793,11 @@ void ApplyDurationModifiers(uint32_t triggerSpellId, uint64_t caster,
                 ApplyMod(e, m, now);
                 break; // one matching aura per (rule, target), like the server
             }
+            // The aura may not be here yet, or may be about to be overwritten by
+            // the cast this trigger sets off. Arm the idempotent ops so the edit
+            // lands when it arrives — see PendingMod.
+            if (OpIsIdempotent(m.op))
+                ArmPendingMod(targets[t], caster, m, now);
         }
     }
 }
@@ -689,8 +806,11 @@ bool RegisterDurationMod(uint32_t triggerSpellId, uint32_t triggerFamily,
                          int32_t triggerSchool, uint32_t affectedFamily,
                          uint64_t affectedMask, uint32_t affectedIcon, int op,
                          int32_t valueMs) {
-    // Trigger must be identified one way or the other.
-    if ((triggerSpellId == 0 && triggerFamily == 0) || affectedMask == 0 ||
+    // Trigger must be identified one way or the other, and the affected aura
+    // needs at least one discriminator past its family — a flag overlap, an
+    // icon, or both. Neither would match every aura of a class.
+    if ((triggerSpellId == 0 && triggerFamily == 0) ||
+        (affectedMask == 0 && affectedIcon == 0) ||
         op < MOD_REFRESH || op > MOD_REMOVE)
         return false;
     for (int i = 0; i < g_modCount; ++i) { // replace an identical rule
@@ -1641,7 +1761,9 @@ int RefreshJudgements(uint64_t unitGuid, uint64_t attackerGuid) {
 uint32_t RefreshDurationByFamily(uint64_t unitGuid, uint32_t family,
                                  uint64_t mask, uint32_t icon,
                                  uint64_t casterGuid) {
-    if (unitGuid == 0 || mask == 0 || casterGuid == 0)
+    // Same selector invariant the rule registrar enforces: family plus at
+    // least one of flag-overlap / icon.
+    if (unitGuid == 0 || (mask == 0 && icon == 0) || casterGuid == 0)
         return 0;
     const uint32_t now = NowMs();
     for (int i = 0; i < g_usedHigh; ++i) {

src/aura/Source.h

@@ -61,10 +61,11 @@ bool Get(uint64_t unitGuid, uint32_t spellId, int slot, uint64_t *outCaster,
          uint32_t *outExpirationMs, uint32_t *outDurationMs);
 
 // Refreshes `casterGuid`'s aura on `unitGuid` matching the same selector the
-// duration rules use — SpellFamilyName + family-flag overlap + optional icon
-// (0 = any) — to a full duration from now. Returns the spellID refreshed, or 0
-// if nothing matched. For a mechanic whose duration edit reaches the client
-// too late to be attributed to a cast packet, so no rule can express it.
+// duration rules use — SpellFamilyName plus a family-flag overlap and/or a
+// SpellIconID, at least one of the two — to a full duration from now. Returns
+// the spellID refreshed, or 0 if nothing matched. For a mechanic whose
+// duration edit reaches the client too late to be attributed to a cast packet,
+// so no rule can express it.
 uint32_t RefreshDurationByFamily(uint64_t unitGuid, uint32_t family,
                                  uint64_t mask, uint32_t icon,
                                  uint64_t casterGuid);
@@ -108,8 +109,10 @@ enum DurationModOp {
 };
 
 // Register a server duration-modifier rule from C++. The trigger is matched by
-// exact `triggerSpellId`; the affected aura by SpellFamilyName + a family-flag
-// overlap (+ optional `affectedIcon`, 0 = any). `valueMs` is the reduce/set
+// exact `triggerSpellId`; the affected aura by SpellFamilyName plus an
+// `affectedMask` family-flag overlap and/or an `affectedIcon`, at least one of
+// the two (0 drops that half of the test; a custom spell with no family flags
+// is named by its icon alone). `valueMs` is the reduce/set
 // amount in milliseconds (ignored by refresh/remove). Used by src/turtle
 // modules for the server's built-in mods (the family/school-matched variant is
 // still Lua-registerable via C_UnitAuras.RegisterAuraDurationModifierByTrigger).