ThordekkCore/PHASE2_OPTIONS_EXTENSIVE_EVALUATION.md

PHASE 2 OPTIONS - EXTENSIVE EVALUATION

Generated: October 27, 2025 22:55 Goal: Remove synchronization barrier blocking main thread at line 761 Constraint: Must maintain session cleanup and error handling

---

CURRENT BLOCKING CODE ANALYSIS

The Problem (Lines 722-783 in BotWorldSessionMgr.cpp)

// Main thread submits tasks and collects futures
std::vector<std::future<void>> futures;
std::vector<ObjectGuid> futureGuids;

for (auto& [guid, botSession] : sessionsToUpdate)
{
    auto future = Performance::GetThreadPool().Submit(taskPriority, updateLogic);
    futures.push_back(std::move(future));
    futureGuids.push_back(guid);
}

// SYNCHRONIZATION BARRIER - Main thread BLOCKS here
for (size_t i = 0; i < futures.size(); ++i)
{
    auto& future = futures[i];
    bool completed = false;
    uint32 retries = 0;

    while (!completed && retries < MAX_RETRIES)
    {
        auto status = future.wait_for(TIMEOUT);

        if (status == std::future_status::ready)
        {
            completed = true;
            future.get();
        }
        else
        {
            ++retries;
            // STUCK HERE AT LINE 761 when future never completes
            TC_LOG_WARN("Future {} not ready after {}ms", i, retries * 100);
            Performance::GetThreadPool().WakeAllWorkers();
        }
    }

    if (!completed)
    {
        TC_LOG_FATAL("DEADLOCK DETECTED: Future {} did not complete!", i);
    }
}

// Collect disconnected sessions from queue
{
    std::lock_guard<std::recursive_mutex> lock(disconnectionMutex);
    while (!disconnectionQueue.empty())
    {
        disconnectedSessions.push_back(disconnectionQueue.front());
        disconnectionQueue.pop();
    }
}

// Cleanup disconnected sessions
if (!disconnectedSessions.empty())
{
    std::lock_guard<std::recursive_mutex> lock(_sessionsMutex);
    for (ObjectGuid const& guid : disconnectedSessions)
    {
        _botSessions.erase(guid);
        sBotPriorityMgr->RemoveBot(guid);
    }
}

Why This Blocks

1. Synchronization Barrier: Main thread MUST wait for ALL futures
2. Single Failure Point: If ONE future hangs, entire system hangs
3. ObjectAccessor Deadlock: Worker thread acquires lock main thread needs
4. Infinite Retry: Retry loop can run forever if future never completes

Critical Requirements

Any solution MUST handle:

1. Session cleanup (disconnected bots must be removed)
2. Error propagation (exceptions in worker threads)
3. Resource management (shared_ptr lifecycle)
4. Performance monitoring (botsUpdated counter)

---

OPTION 1: FIRE-AND-FORGET (FULL ASYNC)

Description

Remove futures entirely - submit tasks without waiting for completion.

Implementation

// Remove future storage (delete lines 506-507)
// std::vector<std::future<void>> futures;
// std::vector<ObjectGuid> futureGuids;

// Submit tasks without saving futures (modify lines 684-691)
for (auto& [guid, botSession] : sessionsToUpdate)
{
    if (!botSession || !botSession->IsActive())
    {
        disconnectedSessions.push_back(guid);
        continue;
    }

    BotPriority botPriority = sBotPriorityMgr->GetPriority(guid);
    Performance::TaskPriority taskPriority = MapBotPriorityToTaskPriority(botPriority);

    // Submit and immediately discard future
    Performance::GetThreadPool().Submit(taskPriority, updateLogic);
    // NO futures.push_back() - task runs in background
}

// Delete entire synchronization barrier (delete lines 707-783)
// No waiting - main thread continues immediately!

// Modify updateLogic to handle cleanup internally
auto updateLogic = [guid, botSession, diff, this, &botsUpdated]()
{
    try
    {
        botSession->Update(diff, filter);
        botsUpdated.fetch_add(1, std::memory_order_relaxed);
    }
    catch (...)
    {
        // Queue for cleanup on NEXT tick instead of THIS tick
        std::lock_guard<std::recursive_mutex> lock(_sessionsMutex);
        _pendingRemovals.push_back(guid);  // NEW: Cleanup queue
    }
};

// At START of next UpdateSessions(), process pending removals
void BotWorldSessionMgr::UpdateSessions(uint32 diff)
{
    // Process cleanup from previous tick
    {
        std::lock_guard<std::recursive_mutex> lock(_sessionsMutex);
        for (ObjectGuid const& guid : _pendingRemovals)
        {
            _botSessions.erase(guid);
            sBotPriorityMgr->RemoveBot(guid);
        }
        _pendingRemovals.clear();
    }

    // Continue with normal update...
}

Pros

✅ Simplest implementation - delete 77 lines, add ~10 lines ✅ Zero main thread blocking - continues immediately ✅ No deadlock possible - main thread never waits ✅ Minimal code changes - low risk ✅ Performance - no synchronization overhead

Cons

❌ No error visibility - can't detect if bot updates fail THIS tick ❌ Delayed cleanup - disconnected bots removed NEXT tick (50ms delay) ❌ No progress tracking - don't know when updates complete ❌ Lost botsUpdated counter - can't track how many bots updated THIS tick ❌ Race condition potential - bot could be queued for removal while still updating

Risk Assessment

• Stability Risk: LOW - tasks complete async, errors logged
• Correctness Risk: MEDIUM - cleanup delayed by 1 tick
• Performance Risk: NONE - actually faster (no waiting)
• Debugging Risk: MEDIUM - harder to track which bots are failing

Mitigations

1. Track cleanup in health check system:

   sBotHealthCheck->RecordDisconnection(guid, currentTime);
   

2. Add completion callback:

   Performance::GetThreadPool().Submit(priority, [guid, botSession]() {
       botSession->Update(diff, filter);
       sBotPerformanceMon->RecordCompletion(guid);  // Track async
   });
   

3. Periodic validation:

   if (_tickCounter % 100 == 0)  // Every 5 seconds
   {
       ValidateAllSessions();  // Check for orphaned sessions
   }
   

Recommendation

⭐ BEST FOR: Immediate stability (fixes crash in 15 minutes) ❌ NOT GOOD FOR: Precise error tracking and session lifecycle management

---

OPTION 2: NON-BLOCKING POLL (CHECK READY ONLY)

Description

Keep futures but only check if they're ALREADY complete - don't wait.

Implementation

// Keep future storage (lines 506-507 unchanged)
std::vector<std::future<void>> futures;
std::vector<ObjectGuid> futureGuids;

// Submit tasks and save futures (lines 684-691 unchanged)
for (auto& [guid, botSession] : sessionsToUpdate)
{
    auto future = Performance::GetThreadPool().Submit(taskPriority, updateLogic);
    futures.push_back(std::move(future));
    futureGuids.push_back(guid);
}

// MODIFIED: Non-blocking poll (replace lines 707-783)
for (size_t i = 0; i < futures.size(); ++i)
{
    auto& future = futures[i];

    // Check if ready WITHOUT WAITING (0ms timeout)
    if (future.wait_for(std::chrono::milliseconds(0)) == std::future_status::ready)
    {
        try
        {
            future.get();  // Retrieve result + exceptions
        }
        catch (std::exception const& e)
        {
            TC_LOG_ERROR("module.playerbot.session",
                "Bot {} update failed: {}", futureGuids[i].ToString(), e.what());
            disconnectedSessions.push_back(futureGuids[i]);
        }
    }
    else
    {
        // Future not ready yet - let it complete in background
        // Store for next tick
        _pendingFutures.emplace_back(futureGuids[i], std::move(future));
    }
}

// At START of next tick, check pending futures from previous tick
void BotWorldSessionMgr::UpdateSessions(uint32 diff)
{
    // Check futures from previous tick(s)
    auto it = _pendingFutures.begin();
    while (it != _pendingFutures.end())
    {
        auto& [guid, future] = *it;

        if (future.wait_for(std::chrono::milliseconds(0)) == std::future_status::ready)
        {
            try
            {
                future.get();
            }
            catch (...)
            {
                TC_LOG_ERROR("module.playerbot.session", "Delayed bot {} failed", guid.ToString());
                _pendingRemovals.push_back(guid);
            }
            it = _pendingFutures.erase(it);
        }
        else
        {
            // Still not ready - check if timeout
            auto age = currentTime - it->submittedTime;
            if (age > 500)  // 500ms = 10 ticks
            {
                TC_LOG_ERROR("module.playerbot.session",
                    "Bot {} update timeout ({}ms) - removing", guid.ToString(), age);
                _pendingRemovals.push_back(guid);
                it = _pendingFutures.erase(it);
            }
            else
            {
                ++it;
            }
        }
    }

    // Continue with normal update...
}

Data Structure Addition

// In BotWorldSessionMgr.h
struct PendingFuture
{
    ObjectGuid guid;
    std::future<void> future;
    uint32 submittedTime;
};
std::deque<PendingFuture> _pendingFutures;
std::vector<ObjectGuid> _pendingRemovals;

Pros

✅ Main thread never blocks - only checks ready futures ✅ Error tracking - exceptions propagated on completion ✅ Graceful timeout - slow bots detected after 500ms ✅ Session cleanup - handled properly with error detection ✅ Progress visibility - know which bots completed vs pending

Cons

❌ Memory overhead - futures stored across multiple ticks ❌ Complexity - more code (deque management, timeout tracking) ❌ Potential memory leak - if futures NEVER complete ❌ Still vulnerable - if ThreadPool workers all blocked

Risk Assessment

• Stability Risk: LOW - main thread never waits
• Correctness Risk: LOW - proper error handling and cleanup
• Performance Risk: LOW - minimal overhead for deque management
• Debugging Risk: LOW - full error tracking and timeout detection

Mitigations

1. Memory leak prevention:

   if (_pendingFutures.size() > 1000)  // Sanity limit
   {
       TC_LOG_FATAL("module.playerbot.session",
           "Too many pending futures ({}) - ThreadPool may be deadlocked!",
           _pendingFutures.size());
       _pendingFutures.clear();  // Emergency cleanup
   }
   

2. Diagnostics:

   if (_tickCounter % 1200 == 0)  // Every 60 seconds
   {
       TC_LOG_INFO("module.playerbot.session",
           "Pending futures: {}, oldest: {}ms",
           _pendingFutures.size(),
           currentTime - _pendingFutures.front().submittedTime);
   }
   

Recommendation

⭐ BEST FOR: Production-quality fix with full error tracking ✅ GOOD FOR: Long-term stability and debugging ⚠️ REQUIRES: Careful memory management and timeout tuning

---

OPTION 3: SHORT TIMEOUT WITH CONTINUE

Description

Keep synchronization barrier but use SHORT timeout and continue on timeout instead of retry.

Implementation

// Keep future storage (lines 506-507 unchanged)
std::vector<std::future<void>> futures;
std::vector<ObjectGuid> futureGuids;

// Submit tasks (lines 684-691 unchanged)
for (auto& [guid, botSession] : sessionsToUpdate)
{
    auto future = Performance::GetThreadPool().Submit(taskPriority, updateLogic);
    futures.push_back(std::move(future));
    futureGuids.push_back(guid);
}

// MODIFIED: Single short timeout, no retry (replace lines 707-783)
constexpr auto TIMEOUT = std::chrono::milliseconds(50);  // One tick tolerance

for (size_t i = 0; i < futures.size(); ++i)
{
    auto& future = futures[i];
    auto status = future.wait_for(TIMEOUT);

    if (status == std::future_status::ready)
    {
        try
        {
            future.get();
        }
        catch (std::exception const& e)
        {
            TC_LOG_ERROR("module.playerbot.session",
                "Bot {} failed: {}", futureGuids[i].ToString(), e.what());
            disconnectedSessions.push_back(futureGuids[i]);
        }
    }
    else
    {
        // Timeout - log and CONTINUE (don't wait)
        TC_LOG_WARN("module.playerbot.session",
            "Bot {} update timeout (>50ms) - skipping", futureGuids[i].ToString());

        // Mark as slow for health check
        sBotHealthCheck->RecordTimeout(futureGuids[i]);

        // Don't add to disconnectedSessions - let it complete in background
        // Future will be destroyed, task continues executing
    }
}

Pros

✅ Minimal code changes - just modify timeout logic ✅ Some error tracking - completed tasks report errors ✅ Progress visibility - know which bots completed in time ✅ Bounded wait time - max 50ms * futures.size()

Cons

❌ Still blocks main thread - waits 50ms per bot ❌ Abandoned futures - tasks that timeout keep running but results ignored ❌ Resource leak potential - futures destroyed while tasks executing ❌ Scaling issue - 100 bots * 50ms = 5 seconds wait time ❌ Unfair to slow bots - legitimate slow updates get abandoned

Risk Assessment

• Stability Risk: MEDIUM - main thread still blocks (up to 5s with 100 bots)
• Correctness Risk: HIGH - abandoned futures may cause undefined behavior
• Performance Risk: HIGH - sequential 50ms waits don't scale
• Debugging Risk: MEDIUM - some errors tracked, timeouts logged

Fatal Flaw

With 100 bots:

100 bots * 50ms timeout = 5000ms (5 seconds) per tick

This violates the 50ms per tick requirement! Main thread would STILL freeze.

Recommendation

❌ NOT RECOMMENDED - doesn't solve the scaling problem ❌ AVOID - abandoned futures are dangerous

---

OPTION 4: BATCH PROCESSING WITH TIMEOUT

Description

Process futures in batches with timeout per batch, not per future.

Implementation

// Keep future storage
std::vector<std::future<void>> futures;
std::vector<ObjectGuid> futureGuids;

// Submit all tasks
for (auto& [guid, botSession] : sessionsToUpdate)
{
    auto future = Performance::GetThreadPool().Submit(taskPriority, updateLogic);
    futures.push_back(std::move(future));
    futureGuids.push_back(guid);
}

// BATCH PROCESSING: Wait for batch with TOTAL timeout
constexpr auto BATCH_TIMEOUT = std::chrono::milliseconds(25);  // Half a tick
auto batchStart = std::chrono::steady_clock::now();

for (size_t i = 0; i < futures.size(); ++i)
{
    // Check time budget
    auto elapsed = std::chrono::steady_clock::now() - batchStart;
    if (elapsed >= BATCH_TIMEOUT)
    {
        TC_LOG_WARN("module.playerbot.session",
            "Batch timeout at future {}/{} - processed {} bots in {}ms",
            i, futures.size(), i,
            std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count());
        break;  // Stop processing, continue with next tick
    }

    // Calculate remaining time
    auto remaining = BATCH_TIMEOUT - elapsed;

    auto& future = futures[i];
    auto status = future.wait_for(remaining);

    if (status == std::future_status::ready)
    {
        try
        {
            future.get();
        }
        catch (...)
        {
            disconnectedSessions.push_back(futureGuids[i]);
        }
    }
    else
    {
        // Timeout - skip rest
        TC_LOG_WARN("module.playerbot.session",
            "Future {} timeout - stopping batch processing", i);
        break;
    }
}

Pros

✅ Bounded total time - max 25ms regardless of bot count ✅ Partial progress - processes as many as possible in budget ✅ Graceful degradation - slow bots don't block fast bots

Cons

❌ Still blocks main thread - up to 25ms per tick ❌ Unfair distribution - later bots in list starved ❌ Incomplete processing - some bots never get updated ❌ Abandoned futures - unchecked futures destroyed

Risk Assessment

• Stability Risk: MEDIUM - 25ms blocking is acceptable but not ideal
• Correctness Risk: HIGH - some bots may never update
• Performance Risk: MEDIUM - 25ms is half a tick budget
• Debugging Risk: LOW - clear timeout logging

Recommendation

⚠️ USE ONLY IF: Fire-and-forget is too risky AND you accept partial updates ❌ NOT IDEAL: Better to use Option 2 (non-blocking poll)

---

OPTION 5: ASYNC CLEANUP QUEUE (HYBRID)

Description

Combine fire-and-forget submission with async cleanup tracking using concurrent queue.

Implementation

// Thread-safe cleanup queue (add to BotWorldSessionMgr.h)
#include <concurrent_queue.h>  // MSVC or Boost lockfree queue
concurrency::concurrent_queue<ObjectGuid> _asyncDisconnections;
std::atomic<uint32> _asyncBotsUpdated{0};

// Submit with async cleanup callback
for (auto& [guid, botSession] : sessionsToUpdate)
{
    BotPriority botPriority = sBotPriorityMgr->GetPriority(guid);
    Performance::TaskPriority taskPriority = MapBotPriorityToTaskPriority(botPriority);

    // Capture weak_ptr to detect session destruction
    std::weak_ptr<BotSession> weakSession = botSession;

    auto updateLogic = [guid, weakSession, diff, this]()
    {
        // Check if session still exists
        auto session = weakSession.lock();
        if (!session)
        {
            _asyncDisconnections.push(guid);  // Thread-safe queue
            return;
        }

        try
        {
            BotPacketFilter filter(session.get());
            bool result = session->Update(diff, filter);

            if (!result)
            {
                _asyncDisconnections.push(guid);
            }
            else
            {
                _asyncBotsUpdated.fetch_add(1, std::memory_order_relaxed);
            }
        }
        catch (...)
        {
            TC_LOG_ERROR("module.playerbot.session",
                "Exception in bot {} update", guid.ToString());
            _asyncDisconnections.push(guid);
        }
    };

    // Fire and forget
    Performance::GetThreadPool().Submit(taskPriority, updateLogic);
}

// NO WAITING - main thread continues immediately

// Process async disconnections (non-blocking)
{
    std::lock_guard<std::recursive_mutex> lock(_sessionsMutex);
    ObjectGuid guid;
    while (_asyncDisconnections.try_pop(guid))
    {
        _botSessions.erase(guid);
        sBotPriorityMgr->RemoveBot(guid);
    }
}

// Report async stats
uint32 updated = _asyncBotsUpdated.exchange(0, std::memory_order_relaxed);
sBotPerformanceMon->EndTick(currentTime, updated, botsSkipped);

Pros

✅ Zero main thread blocking - fire and forget ✅ Proper cleanup - disconnections handled immediately ✅ Thread-safe - lock-free concurrent queue ✅ Error tracking - exceptions logged and handled ✅ Progress tracking - botsUpdated counter preserved ✅ Weak_ptr safety - detects destroyed sessions

Cons

❌ Requires concurrent_queue - external dependency (MSVC has it, may need Boost on Linux) ❌ Complexity - more moving parts ❌ Weak_ptr overhead - atomic reference counting

Risk Assessment

• Stability Risk: VERY LOW - proven lock-free pattern
• Correctness Risk: VERY LOW - proper error handling and cleanup
• Performance Risk: VERY LOW - lock-free is fast
• Debugging Risk: LOW - full error tracking

Dependency Check

// MSVC (Windows) - Built-in
#include <concurrent_queue.h>
concurrency::concurrent_queue<ObjectGuid> _asyncDisconnections;

// GCC/Clang (Linux/Mac) - Boost required
#include <boost/lockfree/queue.hpp>
boost::lockfree::queue<ObjectGuid> _asyncDisconnections{1000};  // Fixed size

Recommendation

⭐ BEST OVERALL - combines all benefits with minimal downsides ✅ USE IF: You have Boost or MSVC ❌ SKIP IF: Can't add dependency (use Option 2 instead)

---

COMPARISON MATRIX

Option	Main Thread Blocking	Error Tracking	Cleanup	Complexity	Dependencies	Stability
1. Fire-and-Forget	✅ None	⚠️ Limited	⚠️ Delayed	✅ Simple	✅ None	⭐⭐⭐⭐
2. Non-Blocking Poll	✅ None	✅ Full	✅ Proper	⚠️ Medium	✅ None	⭐⭐⭐⭐⭐
3. Short Timeout	❌ 5s (100 bots)	⚠️ Partial	⚠️ Abandoned	✅ Simple	✅ None	⭐
4. Batch Timeout	⚠️ 25ms	⚠️ Partial	❌ Incomplete	⚠️ Medium	✅ None	⭐⭐
5. Async Queue	✅ None	✅ Full	✅ Immediate	⚠️ Medium	❌ Boost/MSVC	⭐⭐⭐⭐⭐

---

FINAL RECOMMENDATIONS

For Immediate Stability (NOW)

OPTION 1: Fire-and-Forget

• ⏱️ Implementation: 15 minutes
• ✅ Gets server running immediately
• ⚠️ Cleanup delayed by 1 tick (acceptable)
• Use Case: Emergency fix to stop crashes

For Production Quality (WITHIN 1 HOUR)

OPTION 5: Async Cleanup Queue (if have MSVC or Boost)

• ⏱️ Implementation: 30 minutes
• ✅ Zero blocking, full error tracking, immediate cleanup
• ✅ Production-ready
• Use Case: Long-term solution

OPTION 2: Non-Blocking Poll (if no concurrent_queue)

• ⏱️ Implementation: 45 minutes
• ✅ Zero blocking, full error tracking
• ⚠️ Slightly more complex than Option 5
• Use Case: Long-term solution without dependencies

AVOID

❌ Option 3: Doesn't scale, still blocks 5s with 100 bots ❌ Option 4: Partial updates, unfair to later bots

---

IMPLEMENTATION PRIORITY

Step 1: Implement Option 1 (15 minutes)

Get server stable NOW

Step 2: Test Quest System (30 minutes)

Verify quests work with stable server

Step 3: Upgrade to Option 2 or 5 (30-45 minutes)

Production-quality fix

Total Time: 75-90 minutes to complete fix

---

Report End