fix: add mutex to ConnectionHandles to prevent race condition crash (#378)

Add internal mutex protection to ConnectionHandles class to prevent
double-free crash when ReleaseWorker (statement cleanup) and CloseWorker
(connection close) race to free the same statement handle.

- Add _handlesMutex to protect statement handle map operations
- Protect clear(), checkin(), checkout(), size(), exists() methods
- Add enhanced logging with thread IDs for debugging
- Add stress test to verify fix (examples/stress-test-race-condition.js)

Fixes #378

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

Co-Authored-By: Claude <[email protected]>

Author: TimelordUK

cpp/include/odbc/connection_handles.h

@@ -5,6 +5,7 @@
 #include <functional>
 #include <map>
 #include <memory>
+#include <mutex>
 #include <set>
 #include <vector>
 
@@ -15,10 +16,11 @@
 namespace mssql {
 /**
  * @brief Manages ODBC statement handles for a connection
- * 
- * This class is NOT thread-safe by itself. Thread safety is provided
- * by the containing OdbcConnection class through external synchronization.
- * 
+ *
+ * This class is thread-safe. All public methods are protected by an internal
+ * mutex to prevent race conditions between concurrent operations like
+ * statement release (checkin) and connection close (clear).
+ *
  * The class provides a checkout/checkin pattern for statement handles
  * where each statement is identified by a unique ID.
  */
@@ -59,9 +61,13 @@ class ConnectionHandles {
  private:
   shared_ptr<IOdbcStatementHandle> store(const long statement_id,
                                          shared_ptr<IOdbcStatementHandle> handle);
-  shared_ptr<IOdbcStatementHandle> find(const long statement_id);
+  shared_ptr<IOdbcStatementHandle> find_unlocked(const long statement_id);
   std::map<long, std::shared_ptr<SafeHandle<IOdbcStatementHandle>>> _statementHandles;
   std::shared_ptr<IOdbcEnvironmentHandle> rawEnvHandle_;  // Raw handle - not wrapped in SafeHandle
   std::shared_ptr<SafeHandle<IOdbcConnectionHandle>> connectionHandle_;
+
+  // Mutex for thread-safe access to statement handles
+  // This prevents race conditions between checkin() and clear()
+  mutable std::mutex _handlesMutex;
 };
 }  // namespace mssql

cpp/src/odbc/connection_handles.cpp

@@ -45,11 +45,15 @@ ConnectionHandles::~ConnectionHandles() {
 }
 
 void ConnectionHandles::clear() {
-  SQL_LOG_DEBUG_STREAM("ConnectionHandles::clear - size = " << _statementHandles.size());
+  std::lock_guard<std::mutex> lock(_handlesMutex);
+
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::clear - ENTER - thread="
+                       << std::this_thread::get_id() << " size=" << _statementHandles.size());
 
   // Direct iteration - no need for intermediate vector
   for (const auto& [id, safeHandle] : _statementHandles) {
-    SQL_LOG_DEBUG_STREAM("destruct OdbcStatementCache - free statement " << id);
+    SQL_LOG_DEBUG_STREAM("ConnectionHandles::clear - processing statement " << id
+                         << " thread=" << std::this_thread::get_id());
     if (safeHandle) {
       auto handle = safeHandle->get();
       if (handle) {
@@ -73,40 +77,55 @@ void ConnectionHandles::clear() {
         // Also try to explicitly close the cursor/statement
         SQLCloseCursor(handle->get_handle());
         SQLFreeStmt(handle->get_handle(), SQL_CLOSE);
+      } else {
+        SQL_LOG_WARNING_STREAM("ConnectionHandles::clear - statement " << id
+                               << " handle already invalid/freed");
       }
       // During connection cleanup, we need to force-free handles
       // Reset references first to avoid warning about freeing referenced handles
+      SQL_LOG_DEBUG_STREAM("ConnectionHandles::clear - freeing statement " << id
+                           << " thread=" << std::this_thread::get_id());
       safeHandle->resetReferences();
       safeHandle->free();
+    } else {
+      SQL_LOG_WARNING_STREAM("ConnectionHandles::clear - statement " << id
+                             << " safeHandle is null");
     }
   }
 
   _statementHandles.clear();
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::clear - EXIT - thread=" << std::this_thread::get_id());
 }
 
-shared_ptr<IOdbcStatementHandle> ConnectionHandles::find(const long statement_id) {
+shared_ptr<IOdbcStatementHandle> ConnectionHandles::find_unlocked(const long statement_id) {
+  // NOTE: This is an internal method - caller must hold _handlesMutex
   const auto itr = _statementHandles.find(statement_id);
   if (itr != _statementHandles.end()) {
     auto ref = itr->second->get();
     if (ref) {
       return ref;
     }
-    SQL_LOG_WARNING_STREAM("Found invalid statement handle for id: " << statement_id);
+    SQL_LOG_WARNING_STREAM("find_unlocked - statement " << statement_id
+                           << " found but handle is invalid");
   }
   return nullptr;
 }
 
 size_t ConnectionHandles::size() const {
+  std::lock_guard<std::mutex> lock(_handlesMutex);
   return _statementHandles.size();
 }
 
 bool ConnectionHandles::exists(long statement_id) const {
+  std::lock_guard<std::mutex> lock(_handlesMutex);
   return _statementHandles.find(statement_id) != _statementHandles.end();
 }
 
 shared_ptr<IOdbcStatementHandle> ConnectionHandles::store(const long statement_id,
                                                           shared_ptr<IOdbcStatementHandle> handle) {
-  SQL_LOG_DEBUG_STREAM("ConnectionHandles::store - statementId = " << statement_id);
+  // NOTE: This is an internal method - caller must hold _handlesMutex
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::store - statementId=" << statement_id
+                       << " thread=" << std::this_thread::get_id());
 
   auto safeHandle = std::make_shared<SafeHandle<IOdbcStatementHandle>>(
       "Statement_" + std::to_string(statement_id), handle);
@@ -115,23 +134,33 @@ shared_ptr<IOdbcStatementHandle> ConnectionHandles::store(const long statement_i
   const auto [it, inserted] = _statementHandles.try_emplace(statement_id, safeHandle);
 
   if (!inserted) {
-    SQL_LOG_ERROR_STREAM(
-        "ConnectionHandles::store - statementId already exists = " << statement_id);
+    SQL_LOG_ERROR_STREAM("ConnectionHandles::store - statementId already exists = " << statement_id
+                         << " thread=" << std::this_thread::get_id());
     return nullptr;
   }
 
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::store - statementId=" << statement_id
+                       << " stored successfully, total handles=" << _statementHandles.size());
   return handle;
 }
 
 shared_ptr<IOdbcStatementHandle> ConnectionHandles::checkout(long statement_id) {
   if (statement_id < 0) {
-    SQL_LOG_ERROR_STREAM("ConnectionHandles::checkout - invalid statementId = " << statement_id);
+    SQL_LOG_ERROR_STREAM("ConnectionHandles::checkout - invalid statementId=" << statement_id
+                         << " thread=" << std::this_thread::get_id());
     return nullptr;
   }
 
-  auto statement = find(statement_id);
+  std::lock_guard<std::mutex> lock(_handlesMutex);
+
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkout - ENTER statementId=" << statement_id
+                       << " thread=" << std::this_thread::get_id()
+                       << " current_handles=" << _statementHandles.size());
+
+  auto statement = find_unlocked(statement_id);
   if (statement) {
-    SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkout ok on statementId = " << statement_id);
+    SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkout - found existing statementId=" << statement_id
+                         << " thread=" << std::this_thread::get_id());
     return statement;
   }
 
@@ -143,38 +172,54 @@ shared_ptr<IOdbcStatementHandle> ConnectionHandles::checkout(long statement_id)
   // Get connection handle for allocation
   auto connRef = connectionHandle_->get();
   if (!connRef) {
-    SQL_LOG_ERROR_STREAM("ConnectionHandles::checkout - connection handle invalid");
+    SQL_LOG_ERROR_STREAM("ConnectionHandles::checkout - connection handle invalid, statementId="
+                         << statement_id << " thread=" << std::this_thread::get_id());
     return nullptr;
   }
 
   if (!safeHandle->alloc(connRef->get_handle())) {
-    SQL_LOG_ERROR_STREAM("ConnectionHandles::checkout - failed to allocate statement handle");
+    SQL_LOG_ERROR_STREAM("ConnectionHandles::checkout - failed to allocate statement handle for statementId="
+                         << statement_id << " thread=" << std::this_thread::get_id());
     return nullptr;
   }
 
   // Store the SafeHandle wrapper
   _statementHandles[statement_id] = safeHandle;
 
-  SQL_LOG_DEBUG_STREAM(
-      "ConnectionHandles::checkout - created new handle for statementId = " << statement_id);
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkout - created new handle for statementId="
+                       << statement_id << " thread=" << std::this_thread::get_id()
+                       << " total_handles=" << _statementHandles.size());
 
   // Return the handle itself, not the SafeHandle
   return safeHandle->get();
 }
 
 void ConnectionHandles::checkin(long statementId) {
-  SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkin - statementId = " << statementId);
+  std::lock_guard<std::mutex> lock(_handlesMutex);
+
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkin - ENTER statementId=" << statementId
+                       << " thread=" << std::this_thread::get_id()
+                       << " current_handles=" << _statementHandles.size());
 
   const auto itr = _statementHandles.find(statementId);
   if (itr == _statementHandles.end()) {
-    SQL_LOG_ERROR_STREAM(
-        "ConnectionHandles::checkin - no handle found for statementId = " << statementId);
+    // This can happen legitimately if clear() already freed this statement
+    // during connection close - not necessarily an error
+    SQL_LOG_WARNING_STREAM("ConnectionHandles::checkin - statementId=" << statementId
+                           << " not found (may have been freed by clear()) thread="
+                           << std::this_thread::get_id());
     return;
   }
 
   // Free the handle through SafeHandle
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkin - freeing statementId=" << statementId
+                       << " thread=" << std::this_thread::get_id());
   itr->second->free();
   _statementHandles.erase(statementId);
+
+  SQL_LOG_DEBUG_STREAM("ConnectionHandles::checkin - EXIT statementId=" << statementId
+                       << " thread=" << std::this_thread::get_id()
+                       << " remaining_handles=" << _statementHandles.size());
 }
 
 // Return the interface pointer