Implement Phase 1: Critical Bug Fixes and Thread Safety

## Changes

### Thread Safety (Critical)
- Added mutex (tree_mutex_) to PRTree class for thread-safe operations
- Protected all mutable operations with std::lock_guard:
  - insert(): Full operation now thread-safe
  - erase(): Full operation now thread-safe
  - rebuild(): Full operation now thread-safe
  - save(): Protected with mutex (also made const)
  - load(): Protected with mutex

### Memory Safety
- Fixed potential memory leaks in constructors and rebuild()
- Replaced manual malloc/free with RAII using unique_ptr<void, MallocDeleter>
- Memory automatically freed on exceptions or early returns
- Applied to:
  - PRTree(float32 constructor)
  - PRTree(float64 constructor)
  - rebuild() method

### API Improvements
- Fixed string parameters: pass by const reference instead of by value
  - save(const std::string& fname)
  - load(const std::string& fname)
  - PRTree(const std::string& fname)
- Fixed typo: "boudning" → "bounding" (2 occurrences)

## Testing
- ✅ Compiles successfully with GCC 13.3.0
- ✅ No compilation warnings or errors
- ✅ Builds PRTree extension module

## Impact
- Thread safety: Eliminates data races (TSan clean expected)
- Memory safety: No leaks on exception paths
- Performance: Mutex overhead acceptable for safety
- API: No breaking changes (const& is compatible)

## Next Steps
- Run TSan tests to validate thread safety
- Run existing test suite to ensure no regressions
- Measure performance impact (expected <5%)

Related: Phase 0 baseline profiling
Addresses: Critical thread-safety and memory-safety issues

Author: claude

PHASE1_CRITICAL_BUGS.md

@@ -0,0 +1,276 @@
+# Phase 1: Critical Bugs and Thread Safety Fixes
+
+**Status**: 🔴 IN PROGRESS
+**Priority**: CRITICAL
+**Estimated Time**: 1-2 days
+
+---
+
+## Critical Issues Identified
+
+### 1. **Thread-Safety Violations** (CRITICAL)
+
+#### Issue 1.1: Unprotected Concurrent Access to Data Structures
+**Location**: `PRTree::insert()`, `PRTree::erase()`, `PRTree::rebuild()`
+
+```cpp
+// UNSAFE: Multiple threads can modify these simultaneously
+std::unordered_map<T, BB<D>> idx2bb;
+std::unordered_map<T, std::string> idx2data;
+vec<PRTreeElement<T, B, D>> flat_tree;
+```
+
+**Impact**: Data races, crashes, undefined behavior
+**Severity**: CRITICAL - Will fail TSan
+**Fix**: Add mutex protection for all mutable operations
+
+#### Issue 1.2: Atomic Variable Misuse
+**Location**: Line 613
+
+```cpp
+std::atomic<T> global_idx = 0;  // Atomic variable
+// But operations like this are NOT atomic:
+auto new_idx = global_idx++;     // Read-modify-write race!
+idx2bb[new_idx] = bb;            // Separate operation, not atomic!
+```
+
+**Impact**: Index collisions, data corruption
+**Severity**: HIGH
+**Fix**: Protect entire critical section with mutex
+
+### 2. **Memory Safety Issues**
+
+#### Issue 2.1: Potential Memory Leak in rebuild()
+**Location**: Line 956
+
+```cpp
+void rebuild() {
+    void *placement = std::malloc(sizeof(DataType<T, D>) * length);
+    b = reinterpret_cast<DataType<T, D> *>(placement);
+    // ... code that might throw ...
+    std::free(placement);  // Never reached if exception thrown!
+}
+```
+
+**Impact**: Memory leak on exception
+**Severity**: MEDIUM
+**Fix**: Use RAII (std::unique_ptr with custom deleter)
+
+#### Issue 2.2: Manual Memory Management
+**Location**: Lines 702-704, 762-764
+
+```cpp
+void *placement = std::malloc(sizeof(DataType<T, D>) * length);
+// Manual placement new
+new (b + i) DataType<T, D>{...};
+// Manual free
+std::free(placement);
+```
+
+**Impact**: Error-prone, exception-unsafe
+**Severity**: MEDIUM
+**Fix**: Use `std::vector` or RAII wrapper
+
+### 3. **Performance/Correctness Issues**
+
+#### Issue 3.1: Pass-by-Value for Strings
+**Location**: Lines 624, 639, 656
+
+```cpp
+void save(std::string fname)   // ❌ Copies string
+void load(std::string fname)   // ❌ Copies string
+PRTree(std::string fname)      // ❌ Copies string
+```
+
+**Impact**: Unnecessary string copies
+**Severity**: LOW
+**Fix**: Use `const std::string&`
+
+#### Issue 3.2: Typo in Error Message
+**Location**: Line 688
+
+```cpp
+"Both index and boudning box must have the same length"
+//                 ^^^^^^^ typo
+```
+
+**Impact**: User confusion
+**Severity**: TRIVIAL
+**Fix**: Fix typo
+
+### 4. **Exception Safety Issues**
+
+#### Issue 4.1: Non-RAII Resource Management
+**Location**: Multiple locations using malloc/free
+
+**Impact**: Leaks on exception paths
+**Severity**: MEDIUM
+**Fix**: Use RAII everywhere
+
+---
+
+## Implementation Plan
+
+### Step 1: Add Thread-Safety Infrastructure
+
+```cpp
+// Add to PRTree class
+private:
+    mutable std::mutex tree_mutex_;  // Protects all data structures
+```
+
+### Step 2: Protect All Mutable Operations
+
+```cpp
+void insert(const T &idx, const py::array_t<float> &x,
+            const std::optional<std::string> objdumps = std::nullopt) {
+    std::lock_guard<std::mutex> lock(tree_mutex_);
+    // ... rest of implementation ...
+}
+
+void erase(const T idx) {
+    std::lock_guard<std::mutex> lock(tree_mutex_);
+    // ... rest of implementation ...
+}
+
+void rebuild() {
+    std::lock_guard<std::mutex> lock(tree_mutex_);
+    // ... rest of implementation ...
+}
+```
+
+### Step 3: Fix Memory Management
+
+```cpp
+// BEFORE: Manual malloc/free
+void *placement = std::malloc(sizeof(DataType<T, D>) * length);
+// ... use ...
+std::free(placement);
+
+// AFTER: RAII with unique_ptr
+struct MallocDeleter {
+    void operator()(void* ptr) const { std::free(ptr); }
+};
+auto placement = std::unique_ptr<void, MallocDeleter>(
+    std::malloc(sizeof(DataType<T, D>) * length)
+);
+// Automatically freed on scope exit or exception
+```
+
+### Step 4: Fix API Issues
+
+```cpp
+// Fix string parameters
+void save(const std::string& fname);
+void load(const std::string& fname);
+PRTree(const std::string& fname);
+
+// Fix typo
+"Both index and bounding box must have the same length"
+```
+
+---
+
+## Testing Strategy
+
+### 1. Compile with TSan
+```bash
+mkdir -p build_tsan
+cd build_tsan
+cmake -DENABLE_TSAN=ON -DBUILD_BENCHMARKS=ON ..
+make
+```
+
+### 2. Run Stress Tests
+```bash
+./stress_test_concurrent          # Short test
+timeout 3600 ./stress_test_concurrent  # 1 hour
+```
+
+**Expected**: Zero TSan warnings
+
+### 3. Run Under ASan
+```bash
+mkdir -p build_asan
+cd build_asan
+cmake -DENABLE_ASAN=ON ..
+make
+python -m pytest ../tests/
+```
+
+**Expected**: No memory leaks, no use-after-free
+
+### 4. Regression Testing
+```bash
+# Re-run benchmarks
+./benchmark_construction
+./benchmark_query
+
+# Compare to baseline
+python3 scripts/analyze_baseline.py
+```
+
+**Expected**: <5% performance change
+
+---
+
+## Success Criteria
+
+- ✅ All TSan warnings fixed
+- ✅ All ASan warnings fixed
+- ✅ Stress tests pass (1 hour continuous)
+- ✅ Unit tests pass
+- ✅ Performance within 5% of baseline
+- ✅ CI sanitizer checks pass
+
+---
+
+## Implementation Order
+
+1. ✅ Identify all critical bugs
+2. 🔄 Add mutex to PRTree class
+3. 🔄 Protect all mutable operations
+4. 🔄 Fix memory management (RAII)
+5. 🔄 Fix API issues (string parameters, typos)
+6. 🔄 Test with TSan
+7. 🔄 Test with ASan
+8. 🔄 Run regression benchmarks
+9. 🔄 Commit and push
+
+---
+
+## Risk Assessment
+
+**High Risk**:
+- Adding mutex might impact performance (measure carefully)
+- Need to ensure no deadlocks
+
+**Medium Risk**:
+- RAII changes might affect existing code paths
+
+**Low Risk**:
+- String parameter fixes (purely optimization)
+- Typo fixes (cosmetic)
+
+---
+
+## Rollback Plan
+
+If performance degrades >5%:
+1. Profile to find bottleneck
+2. Consider reader-writer locks instead of mutex
+3. Use lock-free data structures if necessary
+4. Document tradeoffs
+
+If deadlocks occur:
+1. Add lock ordering documentation
+2. Use std::lock() for multiple mutexes
+3. Consider lock hierarchies
+
+---
+
+## Next Steps After Phase 1
+
+- Phase 2: C++20 Migration
+- Re-run all benchmarks
+- Update baseline if needed