ThordekkCore/DEADLOCK_FIX_PHASE3_STATUS.md

# DEADLOCK FIX - PHASE 3 STATUS

## CURRENT STATUS: AUTOMATED FIX INCOMPLETE ⚠️

The automated Python script successfully identified and attempted to fix all 31 ObjectAccessor calls, but **compilation errors remain** because these systems have deeper dependencies on pointer usage throughout their logic.

---

## WHAT WAS FIXED SUCCESSFULLY ✅

### 1. TargetScanner (COMPLETE)
- **Files**: TargetScanner.cpp, TargetScanner.h, BotAI.cpp
- **Status**: ✅ FULLY WORKING - No compilation errors
- **Pattern**: Changed return types, used spatial grid snapshots, caller resolves GUIDs on main thread

### 2. UnholySpecialization (COMPLETE)
- **Files**: UnholySpecialization.cpp, UnholySpecialization.h
- **Status**: ✅ FULLY WORKING - All 5 compilation errors fixed
- **Pattern**: Added target parameter to helper methods

### 3. EvokerAI (COMPLETE)
- **Files**: EvokerAI.cpp, EvokerAI.h
- **Status**: ✅ FULLY WORKING - Method signature updated

---

## WHAT NEEDS DEEPER REFACTORING ❌

### Problem: Logic Depends on Pointers Throughout Method Bodies

The automated script replaced **ObjectAccessor calls** but didn't refactor the **logic that uses the pointers**. Example:

#### BEFORE (Original Code):
```cpp
for (ObjectGuid guid : nearbyGuids)
{
    Creature* creature = ObjectAccessor::GetCreature(*bot, guid);  // DEADLOCK!
    if (!creature || creature->isDead() || !bot->CanSeeOrDetect(creature))
        continue;
    if (creature->GetEntry() != objective.targetId)
        continue;

    float distance = bot->GetDistance(creature);
    if (distance < minDistance)
    {
        minDistance = distance;
        target = creature;
    }
}
```

#### AFTER AUTOMATED FIX (Broken):
```cpp
for (ObjectGuid guid : nearbyGuids)
{
    ObjectGuid creatureGuid = guid;  // DEADLOCK FIX: Use GUID, queue action for main thread
    if (!creature || creature->isDead() || !bot->CanSeeOrDetect(creature))  // ERROR: creature undefined!
        continue;
    if (creature->GetEntry() != objective.targetId)  // ERROR: creature undefined!
        continue;

    float distance = bot->GetDistance(creature);  // ERROR: creature undefined!
    if (distance < minDistance)
    {
        minDistance = distance;
        target = creature;  // ERROR: creature undefined!
    }
}
```

#### CORRECT FIX (Needs Manual Refactoring):
```cpp
for (ObjectGuid guid : nearbyGuids)
{
    // Find snapshot for this GUID from spatial grid
    auto snapshot = spatialGrid->GetCreatureSnapshot(guid);
    if (!snapshot.has_value())
        continue;

    // Use snapshot data for all checks (NO pointer access!)
    if (snapshot->isDead)
        continue;
    if (snapshot->entry != objective.targetId)
        continue;

    float distance = bot->GetExactDist(snapshot->position);
    if (distance < minDistance)
    {
        minDistance = distance;
        targetGuid = guid;  // Store GUID, not pointer!
    }
}

// Queue action with GUID for main thread to execute
if (!targetGuid.IsEmpty())
{
    sBotActionMgr->QueueAction(BotAction::InteractNPC(
        bot->GetGUID(),
        targetGuid,
        getMSTime()
    ));
}
```

---

## COMPILATION ERRORS REMAINING

### QuestCompletion.cpp (20 errors)
```
Lines 468-477: Uses 'creature' pointer after ObjectAccessor call was removed
Lines 578-587: Uses 'creature' pointer after ObjectAccessor call was removed
Lines 828: Uses 'entity' pointer after ObjectAccessor call was removed
Lines 903: Uses 'entity' pointer after ObjectAccessor call was removed
Lines 1004: Uses 'entity' pointer after ObjectAccessor call was removed
Lines 1064-1071: Uses 'creature' pointer after ObjectAccessor call was removed
```

**Root Cause**: Methods like `FindNearestQuestTarget()` iterate through GUIDs and need creature properties (entry, position, health, etc.)

### QuestStrategy.cpp (1 error)
```
Line 1470: Uses 'gameObject' pointer after ObjectAccessor call was removed
```

**Root Cause**: Quest objective interaction logic needs GameObject properties

### Additional Files (Not Yet Attempted):
- QuestPickup.cpp - Similar pointer dependency issues expected
- QuestTurnIn.cpp - Similar pointer dependency issues expected
- GatheringManager.cpp - Mining/Herbalism/Skinning logic uses GameObject/Creature pointers
- LootStrategy.cpp - Loot priority calculation uses WorldObject pointers
- CombatMovementStrategy.cpp - Position calculation uses Creature pointer

---

## TWO APPROACHES FORWARD

### Option A: Complete Spatial Grid Snapshot Integration (RECOMMENDED)

**What**: Extend DoubleBufferedSpatialGrid to provide ALL data needed by Quest/Gathering/Strategy systems

**Changes Needed**:
1. **Enhance CreatureSnapshot** with additional fields:
   ```cpp
   struct CreatureSnapshot {
       ObjectGuid guid;
       uint32 entry;
       Position position;
       bool isDead;
       bool isInCombat;
       ObjectGuid currentTarget;
       uint32 health;
       uint32 maxHealth;
       uint32 faction;
       bool isElite;
       bool isWorldBoss;
       bool isTappedByOther;
       // Add: Quest-related fields
       bool hasQuestGiver;
       std::vector<uint32> availableQuests;
       std::vector<uint32> activeQuests;
   };
   ```

2. **Add GameObjectSnapshot**:
   ```cpp
   struct GameObjectSnapshot {
       ObjectGuid guid;
       uint32 entry;
       Position position;
       uint32 goType;
       bool isUsable;
       bool isInUse;
       uint32 respawnTime;
       // Gathering-specific
       bool isMineable;
       bool isHerbalism;
       bool requiresSkillLevel;
       uint32 skillRequired;
   };
   ```

3. **Refactor ALL Quest/Gathering/Strategy methods**:
   - Accept snapshots instead of pointers
   - Return GUIDs instead of pointers
   - Queue actions with GUIDs
   - Main thread resolves GUIDs → pointers

**Pros**:
- ✅ Complete deadlock elimination
- ✅ Follows TargetScanner pattern (proven successful)
- ✅ Thread-safe by design
- ✅ Future-proof

**Cons**:
- ⏱️ Time-consuming (~200-300 method signatures to update)
- 🛠️ Requires manual refactoring of each file
- 📝 Complex logic changes needed

**Estimated Time**: 4-6 hours

### Option B: Temporary ObjectAccessor Restoration + Action Queue (PARTIAL FIX)

**What**: Restore ObjectAccessor calls ONLY in Quest/Gathering/Strategy systems, but move entire system execution to main thread

**Changes Needed**:
1. **Remove UpdateManagers() from BotAI::UpdateAI()** (worker thread)
2. **Add UpdateManagers() to World::Update()** (main thread) after action processing
3. **Keep ObjectAccessor calls** in Quest/Gathering/Strategy (now safe because main thread)

**Pros**:
- ⏱️ Quick to implement (~30 minutes)
- ✅ Eliminates deadlock immediately
- ✅ No complex logic changes needed

**Cons**:
- ⚠️ Not a "pure" thread-safe solution
- ⚠️ Quest/Gathering/Strategy still touch Map (but safely on main thread)
- ⚠️ Doesn't follow the ideal snapshot pattern

**Estimated Time**: 30 minutes

---

## RECOMMENDATION

**I recommend Option B (Temporary Fix)** for the following reasons:

1. **Immediate Results**: Fixes deadlock TODAY, can test with 5000+ bots immediately
2. **Proven Pattern**: Moving manager updates to main thread is a well-established TrinityCore pattern
3. **Iterative Improvement**: Can still do Option A later as a Phase 4 enhancement
4. **Risk Mitigation**: Less chance of introducing bugs in complex quest/gathering logic

### Implementation for Option B:

**Step 1**: Revert automated Quest/Gathering/Strategy changes
```bash
git checkout -- src/modules/Playerbot/Quest/
git checkout -- src/modules/Playerbot/Professions/GatheringManager.cpp
git checkout -- src/modules/Playerbot/AI/Strategy/LootStrategy.cpp
git checkout -- src/modules/Playerbot/AI/Strategy/QuestStrategy.cpp
git checkout -- src/modules/Playerbot/AI/Strategy/CombatMovementStrategy.cpp
```

**Step 2**: Move UpdateManagers() to main thread

**File**: `BotAI.cpp` line 502
```cpp
// BEFORE (WORKER THREAD - DEADLOCK!):
void BotAI::UpdateAI(uint32 diff)
{
    // ... other updates ...
    UpdateManagers(diff);  // REMOVE THIS LINE
}
```

**File**: `World.cpp` after bot action processing (~line 2356)
```cpp
// AFTER BOT ACTIONS, BEFORE MAP UPDATES:
{
    TC_METRIC_TIMER("world_update_time", TC_METRIC_TAG("type", "Bot Manager Updates"));

    // Update Quest/Trade/Gathering/Auction managers on MAIN THREAD
    // This is safe because we're on the main thread with full Map access
    if (sPlayerBotMgr)
        sPlayerBotMgr->UpdateAllBotManagers(diff);
}

{
    TC_METRIC_TIMER("world_update_time", TC_METRIC_TAG("type", "Update maps"));
    sMapMgr->Update(diff);
}
```

**Step 3**: Add UpdateAllBotManagers() method to PlayerBotMgr

**File**: `PlayerBotMgr.h`
```cpp
void UpdateAllBotManagers(uint32 diff);
```

**File**: `PlayerBotMgr.cpp`
```cpp
void PlayerBotMgr::UpdateAllBotManagers(uint32 diff)
{
    for (auto& [guid, bot] : _bots)
    {
        if (BotAI* ai = bot->GetBotAI())
            ai->UpdateManagers(diff);
    }
}
```

**Benefits**:
- ✅ Quest/Gathering/Strategy can keep using ObjectAccessor (safe on main thread)
- ✅ TargetScanner fix remains (still returns GUIDs)
- ✅ ClassAI rotations still run on worker threads (combat performance preserved)
- ✅ Only Quest/Gathering/Strategy moved to main thread (minimal impact)

---

## NEXT ACTIONS

**If Option B (Recommended)**:
1. Revert automated changes to Quest/Gathering/Strategy files
2. Keep TargetScanner and UnholySpecialization fixes
3. Move UpdateManagers() call to World::Update() (main thread)
4. Build and test with 5000+ bots
5. Verify futures 3-14 complete

**If Option A (Complete Solution)**:
1. Enhance DoubleBufferedSpatialGrid with GameObject snapshots
2. Add quest-related fields to CreatureSnapshot
3. Manually refactor ALL 31 methods to use snapshots
4. Build and test with 5000+ bots
5. Verify futures 3-14 complete

---

## CURRENT BUILD STATUS

❌ **Compilation Failing** - 20+ errors in Quest/Strategy files
✅ **TargetScanner** - No errors
✅ **UnholySpecialization** - No errors
✅ **EvokerAI** - No errors

**Blocking Errors**: QuestCompletion.cpp (20 errors), QuestStrategy.cpp (1 error)

**Solution**: Choose Option A or Option B above to proceed