analysis to md file

This commit re-adds the ThreadPool functionality that was reverted, incorporating
changes from PRs 4340, 4342, and 4343:

* ThreadPool with TaskResult wrapper for deadline support (PR 4342)
* Simplified FIFO queue instead of priority system (PR 4343)
* Deadline parameter threading through AI functions (PR 4340)
* Comprehensive metrics and session tracking capabilities
* Proper error handling for deadline exceeded scenarios

The implementation maintains compatibility with the transposition table
caching system and provides improved performance monitoring.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

RACE_CONDITION_ANALYSIS.md

@@ -0,0 +1,150 @@
+# Race Condition Analysis for Eagle0
+
+## Summary
+This document provides an analysis of potential race conditions in the Eagle0 codebase after merging the `fix-race` branch and re-adding ThreadPool functionality from PRs 4340, 4342, and 4343.
+
+## Fixed Issues
+
+### ✅ ShardokEngine Shared State Issue (FIXED by fix-race branch)
+The `fix-race` branch successfully addressed a major race condition by changing `ShardokEngine` from being passed as `shared_ptr<ShardokEngine>` to being passed by value (copy). 
+
+**Key changes:**
+- `BasicLookaheadCalculator` now takes `const ShardokEngine innerEngine` (by value)
+- `CalcOne` creates a local copy: `auto innerEngine = ShardokEngine(guessedEngine, false)`
+- All engine method calls changed from `innerEngine->` to `innerEngine.`
+
+This ensures each thread works with its own independent copy of the engine state, eliminating concurrent access to shared mutable state.
+
+## Remaining Potential Race Conditions
+
+### 1. Transposition Table Global Access (HIGH RISK)
+**Location:** `TranspositionTable.cpp`, global instance `g_transpositionTable`
+
+**Issues:**
+- Global shared state accessed by multiple threads simultaneously
+- Hash collisions possible under high concurrency
+- Depth-based storage logic when threads work at different depths
+- Potential ABA problems in compare-and-swap operations
+
+**Impact:** Most likely culprit for remaining crashes given it was added recently
+
+### 2. Global Random Generator (MEDIUM RISK)
+**Location:** `AIScoreCalculator.cpp`
+```cpp
+static const auto _averageGenerator = std::make_shared<SequenceRandomGenerator>(_averageSequence);
+```
+
+**Issues:**
+- Shared across all threads
+- If `SequenceRandomGenerator` isn't thread-safe internally, concurrent access could corrupt state
+- No synchronization around access to this shared generator
+
+### 3. ActionPointDistances Cache (MEDIUM RISK)
+**Location:** `ActionPointDistancesCache.cpp`
+
+**Issues:**
+- Thread-local caching mechanism may have synchronization issues
+- Cache invalidation across threads could be problematic
+- Global cache updates might race with thread-local cache access
+
+### 4. Performance Logging Atomics (LOW RISK)
+**Location:** `AIScoreCalculator.cpp`, `AttackerScorePerformanceLogger`
+```cpp
+intervalTime.fetch_add(duration);  // duration is a double
+```
+
+**Issues:**
+- Atomic operations on doubles aren't guaranteed lock-free on all platforms
+- Could cause performance degradation or incorrect metrics
+
+### 5. ThreadPool Task Ordering (MEDIUM RISK)
+**Location:** `ThreadPool.hpp`
+
+**Issues:**
+- FIFO queue (deque) could have ordering dependencies
+- Deadline handling might create races if tasks timeout while processing
+- Session-based metrics collection adds new shared state
+- Task cancellation and cleanup could race with execution
+
+### 6. Future Aggregation (LOW RISK)
+**Location:** `AIScoreCalculator.cpp`, `BestCommandIndex` function
+
+**Issues:**
+- Futures collected and results aggregated
+- Unexpected completion order could cause issues
+- Error states might not be handled consistently
+
+## Recommendations
+
+### Immediate Actions
+
+1. **Make Random Generators Thread-Local**
+   ```cpp
+   thread_local auto t_randomGenerator = 
+       std::make_shared<SequenceRandomGenerator>(_averageSequence);
+   ```
+
+2. **Add Defensive Checks**
+   - Validate game state after engine operations
+   - Check for NaN/infinity before storing scores
+   - Assert transposition table entry consistency
+
+3. **Improve Transposition Table Locking**
+   - Consider sharding with separate locks per shard
+   - Investigate lock-free data structures
+   - Add more granular locking around critical sections
+
+### Investigation Steps
+
+1. **Run Thread Sanitizer**
+   ```bash
+   bazel test --config=tsan //src/test/cpp/net/eagle0/shardok/ai:all
+   ```
+
+2. **Add Detailed Logging**
+   - Log all transposition table store/probe operations with thread IDs
+   - Track random generator access patterns
+   - Monitor ThreadPool task lifecycle
+
+3. **Verify Thread Safety**
+   - Audit `SequenceRandomGenerator` for thread safety
+   - Review `ShardokEngine` copy constructor for deep copy completeness
+   - Check all global/static variables for proper synchronization
+
+### Long-term Improvements
+
+1. **Redesign Transposition Table**
+   - Implement thread-local transposition tables with periodic merging
+   - Use concurrent hash map implementation (e.g., Intel TBB concurrent_hash_map)
+   - Add versioning to prevent ABA problems
+
+2. **Eliminate Global State**
+   - Pass random generators explicitly rather than using globals
+   - Consider dependency injection for caches and tables
+   - Make performance loggers thread-local
+
+3. **Improve ThreadPool Robustness**
+   - Add task dependency tracking
+   - Implement proper cancellation tokens
+   - Add timeout recovery mechanisms
+
+## Testing Recommendations
+
+1. **Stress Testing**
+   - Run AI calculations with many threads simultaneously
+   - Use different random seeds to expose timing-dependent bugs
+   - Test with various game states and board configurations
+
+2. **Reproducibility**
+   - Add deterministic mode with fixed seeds
+   - Log thread scheduling information
+   - Create minimal test cases that reproduce crashes
+
+3. **Monitoring**
+   - Add metrics for lock contention
+   - Track task completion times and timeout rates
+   - Monitor memory usage patterns
+
+## Conclusion
+
+While the `fix-race` branch addressed the critical ShardokEngine shared state issue, several race condition risks remain. The transposition table and global random generator are the most likely culprits for any remaining crashes. Implementing the recommended immediate actions should significantly improve stability, while the long-term improvements will make the system more robust and maintainable.

src/main/cpp/net/eagle0/common/TaskResult.hpp

@@ -0,0 +1,39 @@
+//
+// TaskResult.hpp - Result wrapper for task execution with status information
+//
+
+#ifndef EAGLE0_TASK_RESULT_HPP
+#define EAGLE0_TASK_RESULT_HPP
+
+namespace eagle0::common {
+
+enum class TaskStatus { SUCCESS = 0, DEADLINE_EXCEEDED = 1, CANCELLED = 2 };
+
+template<typename T>
+struct TaskResult {
+    T value;
+    TaskStatus status;
+
+    TaskResult() : value{}, status(TaskStatus::SUCCESS) {}
+    TaskResult(T val) : value(std::move(val)), status(TaskStatus::SUCCESS) {}
+    TaskResult(T val, TaskStatus stat) : value(std::move(val)), status(stat) {}
+
+    // Convenience methods for checking status
+    T get() const { return value; }
+    bool succeeded() const { return status == TaskStatus::SUCCESS; }
+    bool deadlineExceeded() const { return status == TaskStatus::DEADLINE_EXCEEDED; }
+    bool cancelled() const { return status == TaskStatus::CANCELLED; }
+
+    // Factory methods for cleaner construction
+    static TaskResult Success(T val) { return TaskResult(std::move(val), TaskStatus::SUCCESS); }
+    static TaskResult DeadlineExceeded(T val = T{}) {
+        return TaskResult(std::move(val), TaskStatus::DEADLINE_EXCEEDED);
+    }
+    static TaskResult Cancelled(T val = T{}) {
+        return TaskResult(std::move(val), TaskStatus::CANCELLED);
+    }
+};
+
+}  // namespace eagle0::common
+
+#endif  // EAGLE0_TASK_RESULT_HPP

src/main/cpp/net/eagle0/common/ThreadPool.hpp

@@ -8,11 +8,11 @@
 #include <atomic>
 #include <chrono>
 #include <condition_variable>
+#include <deque>
 #include <functional>
 #include <future>
 #include <memory>
 #include <mutex>
-#include <queue>
 #include <thread>
 #include <vector>
 
@@ -45,38 +45,18 @@ public:
 private:
     struct Task {
         std::function<void()> function;
-        int priority;
         TimePoint deadline;
         bool has_deadline;
 
-        Task(std::function<void()> f, int p, TimePoint d, bool has_d)
+        Task(std::function<void()> f, TimePoint d, bool has_d)
             : function(std::move(f)),
-              priority(p),
               deadline(d),
               has_deadline(has_d) {}
-
-        // Higher priority values and earlier deadlines have higher priority
-        bool operator<(const Task& other) const {
-            if (priority != other.priority) {
-                return priority < other.priority;  // Lower priority values have lower priority in
-                                                   // priority_queue
-            }
-            if (has_deadline && other.has_deadline) {
-                return deadline > other.deadline;  // Later deadlines have lower priority
-            }
-            if (has_deadline && !other.has_deadline) {
-                return false;  // Tasks with deadlines have higher priority
-            }
-            if (!has_deadline && other.has_deadline) {
-                return true;  // Tasks without deadlines have lower priority
-            }
-            return false;  // Equal priority, no preference
-        }
     };
 
     std::vector<std::thread> workers;
-    std::priority_queue<Task> tasks;
-    std::mutex queue_mutex;
+    std::deque<Task> tasks;          // Simple FIFO queue instead of priority queue
+    mutable std::mutex queue_mutex;  // mutable for const methods like queue_size()
     std::condition_variable condition;
     std::atomic<bool> stop{false};
 
@@ -85,7 +65,7 @@ public:
         for (size_t i = 0; i < num_threads; ++i) {
             workers.emplace_back([this] {
                 while (true) {
-                    Task task{nullptr, 0, TimePoint{}, false};
+                    Task task{nullptr, TimePoint{}, false};
                     {
                         std::unique_lock<std::mutex> lock(queue_mutex);
                         condition.wait(lock, [this] { return stop.load() || !tasks.empty(); });
@@ -93,8 +73,8 @@ public:
                         if (stop.load() && tasks.empty()) { return; }
 
                         if (!tasks.empty()) {
-                            task = std::move(const_cast<Task&>(tasks.top()));
-                            tasks.pop();
+                            task = std::move(tasks.front());
+                            tasks.pop_front();
                         } else {
                             continue;
                         }
@@ -107,9 +87,9 @@ public:
         }
     }
 
-    // Enqueue a task with priority only
+    // Enqueue a task without deadline
     template<class F, class... Args>
-    auto enqueue(F&& f, Args&&... args, int priority = 0)
+    auto enqueue(F&& f, Args&&... args)
             -> std::future<TaskResult<std::invoke_result_t<F, Args...>>> {
         using return_type = std::invoke_result_t<F, Args...>;
         using result_type = TaskResult<return_type>;
@@ -126,21 +106,21 @@ public:
         {
             std::unique_lock<std::mutex> lock(queue_mutex);
             if (stop.load()) { throw std::runtime_error("enqueue on stopped ThreadPool"); }
-            tasks.emplace([task]() { (*task)(); }, priority, TimePoint{}, false);
+            tasks.emplace_back([task]() { (*task)(); }, TimePoint{}, false);
         }
 
         condition.notify_one();
         return result;
     }
 
-    // Enqueue a task with priority and deadline
-    template<class F, class... Args>
-    auto enqueue_with_deadline(F&& f, Args&&... args, int priority, TimePoint deadline)
-            -> std::future<TaskResult<std::invoke_result_t<F, Args...>>> {
-        using return_type = std::invoke_result_t<F, Args...>;
+    // Enqueue a task with deadline
+    template<class F>
+    auto enqueue_with_deadline(F&& f, TimePoint deadline)
+            -> std::future<TaskResult<std::invoke_result_t<F>>> {
+        using return_type = std::invoke_result_t<F>;
         using result_type = TaskResult<return_type>;
 
-        auto actualTask = std::bind(std::forward<F>(f), std::forward<Args>(args)...);
+        auto actualTask = std::forward<F>(f);
 
         auto task = std::make_shared<std::packaged_task<result_type()>>(
                 [actualTask = std::move(actualTask), deadline]() mutable -> result_type {
@@ -155,7 +135,7 @@ public:
         {
             std::unique_lock<std::mutex> lock(queue_mutex);
             if (stop.load()) { throw std::runtime_error("enqueue on stopped ThreadPool"); }
-            tasks.emplace([task]() { (*task)(); }, priority, deadline, true);
+            tasks.emplace_back([task]() { (*task)(); }, deadline, true);
         }
 
         condition.notify_one();
@@ -164,25 +144,27 @@ public:
 
     // Get current queue size (approximate, for monitoring)
     size_t queue_size() const {
-        std::unique_lock<std::mutex> lock(const_cast<std::mutex&>(queue_mutex));
+        std::unique_lock<std::mutex> lock(queue_mutex);
         return tasks.size();
     }
 
     // Get detailed queue information for debugging
     void debug_queue_state() const {
-        std::unique_lock<std::mutex> lock(const_cast<std::mutex&>(queue_mutex));
+        std::unique_lock<std::mutex> lock(queue_mutex);
         printf("ThreadPool: Queue size: %zu\n", tasks.size());
         if (!tasks.empty()) {
-            // Create a copy to inspect priorities without modifying queue
-            auto queue_copy = tasks;
-            std::vector<int> priorities;
-            while (!queue_copy.empty()) {
-                priorities.push_back(queue_copy.top().priority);
-                queue_copy.pop();
+            int with_deadline = 0;
+            int without_deadline = 0;
+            for (const auto& task : tasks) {
+                if (task.has_deadline) {
+                    with_deadline++;
+                } else {
+                    without_deadline++;
+                }
             }
-            printf("ThreadPool: Priorities in queue: ");
-            for (int p : priorities) { printf("%d ", p); }
-            printf("\n");
+            printf("ThreadPool: Tasks with deadline: %d, without deadline: %d\n",
+                   with_deadline,
+                   without_deadline);
         }
     }
 

src/main/cpp/net/eagle0/shardok/ai/AIScoreCalculator.cpp

@@ -45,7 +45,7 @@ struct ImmediateAndLookaheadScore {
         bool isDefender,
         int remainingLookahead,
         int maxRepeatCount,
-        const shared_ptr<ShardokEngine> &innerEngine,
+        const ShardokEngine innerEngine,
         ScoreValue currentUtility,
         const AIStrategy &attackerStrategy,
         const SettingsGetter &settingsGetter,
@@ -913,7 +913,7 @@ auto BasicLookaheadCalculator(
         const bool isDefender,
         const int remainingLookahead,
         const int maxRepeatCount,
-        const shared_ptr<ShardokEngine> &innerEngine,
+        const ShardokEngine innerEngine,
         const ScoreValue currentUtility,
         const AIStrategy &attackerStrategy,
         const SettingsGetter &settingsGetter,
@@ -923,7 +923,7 @@ auto BasicLookaheadCalculator(
         std::chrono::steady_clock::time_point deadline) -> std::future<ScoreValue> {
     // Check transposition table before expensive computation
     auto cachedScore =
-            g_transpositionTable.probe(innerEngine->GetCurrentGameState(), remainingLookahead, pid);
+            g_transpositionTable.probe(innerEngine.GetCurrentGameState(), remainingLookahead, pid);
 
     if (cachedScore.has_value()) {
         // Return cached result immediately
@@ -938,14 +938,14 @@ auto BasicLookaheadCalculator(
         // Store the current utility in the transposition table and return it
         // Note: Store with depth 1 since depth 0 indicates an empty entry in the transposition
         // table
-        g_transpositionTable.store(innerEngine->GetCurrentGameState(), 1, pid, nextUtility);
+        g_transpositionTable.store(innerEngine.GetCurrentGameState(), 1, pid, nextUtility);
 
         std::promise<ScoreValue> p;
         p.set_value(nextUtility);
         return p.get_future();
     }
 
-    if (const CommandListSPtr nextCommands = innerEngine->GetAvailableCommandsForAIPlayer(pid);
+    if (const CommandListSPtr nextCommands = innerEngine.GetAvailableCommandsForAIPlayer(pid);
         nextCommands && !nextCommands->empty()) {
         // Get the future from BestCommandIndex without calling .get()
         auto bestCommandFuture = BestCommandIndex(
@@ -953,7 +953,7 @@ auto BasicLookaheadCalculator(
                 isDefender,
                 remainingLookahead - 1,
                 maxRepeatCount,
-                *innerEngine,
+                innerEngine,
                 attackerStrategy,
                 nextUtility,
                 settingsGetter,
@@ -975,7 +975,7 @@ auto BasicLookaheadCalculator(
 
                     ScoreValue resultScore;
                     if (auto &nextCommand =
-                                innerEngine->GetAvailableCommandsForAIPlayer(pid)->at(index);
+                                innerEngine.GetAvailableCommandsForAIPlayer(pid)->at(index);
                         nextCommand->GetCommandType() !=
                         net::eagle0::shardok::common::END_TURN_COMMAND) {
                         resultScore = immediateScore;
@@ -985,7 +985,7 @@ auto BasicLookaheadCalculator(
 
                     // Store in transposition table before returning
                     g_transpositionTable.store(
-                            innerEngine->GetCurrentGameState(),
+                            innerEngine.GetCurrentGameState(),
                             remainingLookahead,
                             pid,
                             resultScore);
@@ -996,7 +996,7 @@ auto BasicLookaheadCalculator(
 
     // No commands available, store and return the current utility as a future
     g_transpositionTable
-            .store(innerEngine->GetCurrentGameState(), remainingLookahead, pid, nextUtility);
+            .store(innerEngine.GetCurrentGameState(), remainingLookahead, pid, nextUtility);
 
     std::promise<ScoreValue> p;
     p.set_value(nextUtility);
@@ -1029,12 +1029,12 @@ auto CalcOne(
         return returnValue;
     }
 
-    auto innerEngine = std::make_shared<ShardokEngine>(guessedEngine, false);
-    innerEngine->PostCommand(pid, commandIndex, randomGenerator);
+    auto innerEngine = ShardokEngine(guessedEngine, false);
+    innerEngine.PostCommand(pid, commandIndex, randomGenerator);
 
     auto innerUtility = AIScoreCalculator::GuessedStateScore(
             isDefender,
-            innerEngine->GetCurrentGameState(),
+            innerEngine.GetCurrentGameState(),
             attackerStrategy,
             allCastleCoords,
             settingsGetter,