Address bot reviews

Author: VyasGuru

sdk/runanywhere-commons/CMakeLists.txt

@@ -198,6 +198,11 @@ set(_SAVED_BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS})
 set(BUILD_SHARED_LIBS OFF)
 FetchContent_MakeAvailable(libarchive)
 set(BUILD_SHARED_LIBS ${_SAVED_BUILD_SHARED_LIBS})
+
+if(TARGET bz2_bundled AND TARGET archive_static)
+    target_link_libraries(archive_static bz2_bundled)
+endif()
+
 message(STATUS "libarchive ready (v${LIBARCHIVE_VERSION})")
 
 # =============================================================================

sdk/runanywhere-commons/include/rac/core/capabilities/rac_lifecycle.h

@@ -234,6 +234,25 @@ RAC_API rac_handle_t rac_lifecycle_get_service(rac_handle_t handle);
  */
 RAC_API rac_result_t rac_lifecycle_require_service(rac_handle_t handle, rac_handle_t* out_service);
 
+/**
+ * @brief Acquire (pin) the current service, preventing unload while held.
+ *
+ * Increments an internal refcount. The caller MUST call rac_lifecycle_release_service()
+ * when done. Unload/destroy will block until all acquired references are released.
+ *
+ * @param handle Lifecycle manager handle
+ * @param out_service Output: Service handle (pinned)
+ * @return RAC_SUCCESS or RAC_ERROR_NOT_INITIALIZED if not loaded
+ */
+RAC_API rac_result_t rac_lifecycle_acquire_service(rac_handle_t handle, rac_handle_t* out_service);
+
+/**
+ * @brief Release a previously acquired service reference.
+ *
+ * @param handle Lifecycle manager handle
+ */
+RAC_API void rac_lifecycle_release_service(rac_handle_t handle);
+
 /**
  * @brief Track an operation error
  *

sdk/runanywhere-commons/include/rac/infrastructure/extraction/rac_extraction.h

@@ -48,11 +48,19 @@ typedef struct rac_extraction_options {
 /**
  * @brief Default extraction options.
  */
+#ifdef __cplusplus
+inline constexpr rac_extraction_options_t RAC_EXTRACTION_OPTIONS_DEFAULT = {
+    RAC_TRUE,             /* skip_macos_resources */
+    RAC_FALSE,            /* skip_symlinks */
+    RAC_ARCHIVE_TYPE_NONE /* archive_type_hint */
+};
+#else
 static const rac_extraction_options_t RAC_EXTRACTION_OPTIONS_DEFAULT = {
     RAC_TRUE,             /* skip_macos_resources */
     RAC_FALSE,            /* skip_symlinks */
     RAC_ARCHIVE_TYPE_NONE /* archive_type_hint */
 };
+#endif
 
 // =============================================================================
 // EXTRACTION RESULT

sdk/runanywhere-commons/src/backends/onnx/onnx_backend.cpp

@@ -24,11 +24,8 @@
 #include "rac/core/rac_logger.h"
 
 #if SHERPA_ONNX_AVAILABLE
-extern "C" {
-    int espeak_Initialize(int output, int buflength, const char *path, int options);
-    int espeak_SetVoiceByName(const char *name);
-}
-#define ESPEAK_AUDIO_OUTPUT_SYNCHRONOUS 0x0003
+// espeak-ng reinitialization is handled by destroying and recreating
+// the SherpaOnnxOfflineTts instance via the sherpa-onnx C API.
 #endif
 
 namespace runanywhere {
@@ -966,26 +963,15 @@ bool ONNXTTS::load_model(const std::string& model_path, TTSModelType model_type,
         return false;
     }
 
-    sherpa_tts_ = new_tts;
-
-    // Force espeak-ng to use THIS model's data_dir.
-    // Sherpa-ONNX uses std::once_flag for espeak_Initialize, so only the first
-    // model loaded gets its data_dir registered. Re-calling espeak_Initialize
-    // directly resets the internal path_home to the current model's directory.
-    if (!espeak_data_dir_.empty()) {
-        int reinit = espeak_Initialize(ESPEAK_AUDIO_OUTPUT_SYNCHRONOUS, 0, espeak_data_dir_.c_str(), 0);
-        RAC_LOG_INFO("ONNX.TTS", "espeak_Initialize override: result=%d (expected 22050), data_dir=%s",
-            reinit, espeak_data_dir_.c_str());
-
-        if (reinit == 22050) {
-            int voice_test = espeak_SetVoiceByName("en-us");
-            RAC_LOG_INFO("ONNX.TTS", "espeak_SetVoiceByName('en-us') test: result=%d (0=success)", voice_test);
-            int voice_test_gb = espeak_SetVoiceByName("en-gb");
-            RAC_LOG_INFO("ONNX.TTS", "espeak_SetVoiceByName('en-gb') test: result=%d (0=success)", voice_test_gb);
-        } else {
-            RAC_LOG_ERROR("ONNX.TTS", "espeak_Initialize override FAILED with code %d", reinit);
-        }
+    // Workaround for sherpa-onnx std::once_flag issue: espeak_Initialize is
+    // only called internally on the very first SherpaOnnxCreateOfflineTts call.
+    // When switching TTS models with different data_dir, destroy and recreate
+    // the instance so the config (including data_dir) is applied correctly.
+    if (sherpa_tts_ && sherpa_tts_ != new_tts) {
+        SherpaOnnxDestroyOfflineTts(sherpa_tts_);
+        sherpa_tts_ = nullptr;
     }
+    sherpa_tts_ = new_tts;
 
     sample_rate_ = SherpaOnnxOfflineTtsSampleRate(sherpa_tts_);
     int num_speakers = SherpaOnnxOfflineTtsNumSpeakers(sherpa_tts_);

sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp

@@ -60,6 +60,10 @@ struct LifecycleManager {
     // Thread safety
     std::mutex mutex{};
 
+    // Service pinning (acquire/release) — prevents unload while service is in use
+    std::atomic<int> service_refcount{0};
+    std::condition_variable service_cv{};
+
     LifecycleManager() {
         // Set start time (mirrors Swift's startTime = Date())
         auto now = std::chrono::system_clock::now();
@@ -225,13 +229,45 @@ rac_result_t rac_lifecycle_load(rac_handle_t handle, const char* model_path, con
     return result;
 }
 
+rac_result_t rac_lifecycle_acquire_service(rac_handle_t handle, rac_handle_t* out_service) {
+    if (handle == nullptr || out_service == nullptr) {
+        return RAC_ERROR_NULL_POINTER;
+    }
+
+    auto* mgr = static_cast<LifecycleManager*>(handle);
+    std::lock_guard<std::mutex> lock(mgr->mutex);
+
+    if (mgr->state.load() != RAC_LIFECYCLE_STATE_LOADED || mgr->current_service.load() == nullptr) {
+        return RAC_ERROR_NOT_INITIALIZED;
+    }
+
+    mgr->service_refcount.fetch_add(1);
+    *out_service = mgr->current_service.load();
+    return RAC_SUCCESS;
+}
+
+void rac_lifecycle_release_service(rac_handle_t handle) {
+    if (handle == nullptr) {
+        return;
+    }
+
+    auto* mgr = static_cast<LifecycleManager*>(handle);
+    int prev = mgr->service_refcount.fetch_sub(1);
+    if (prev <= 1) {
+        mgr->service_cv.notify_all();
+    }
+}
+
 rac_result_t rac_lifecycle_unload(rac_handle_t handle) {
     if (handle == nullptr) {
         return RAC_ERROR_NULL_POINTER;
     }
 
     auto* mgr = static_cast<LifecycleManager*>(handle);
-    std::lock_guard<std::mutex> lock(mgr->mutex);
+    std::unique_lock<std::mutex> lock(mgr->mutex);
+
+    // Wait for all acquired services to be released
+    mgr->service_cv.wait(lock, [mgr] { return mgr->service_refcount.load() == 0; });
 
     // Mirrors Swift: if let modelId = await lifecycle.currentResourceId
     if (!mgr->current_model_id.empty()) {
@@ -269,7 +305,10 @@ rac_result_t rac_lifecycle_reset(rac_handle_t handle) {
     }
 
     auto* mgr = static_cast<LifecycleManager*>(handle);
-    std::lock_guard<std::mutex> lock(mgr->mutex);
+    std::unique_lock<std::mutex> lock(mgr->mutex);
+
+    // Wait for all acquired services to be released
+    mgr->service_cv.wait(lock, [mgr] { return mgr->service_refcount.load() == 0; });
 
     // Track unload if currently loaded (mirrors Swift reset())
     if (!mgr->current_model_id.empty()) {

sdk/runanywhere-commons/src/features/diffusion/diffusion_component.cpp

@@ -396,8 +396,8 @@ extern "C" rac_result_t rac_diffusion_component_generate(rac_handle_t handle,
         // Reset cancellation flag (also atomic, but set under lock for consistency)
         component->cancel_requested = false;
 
-        // Get service from lifecycle manager
-        rac_result_t result = rac_lifecycle_require_service(component->lifecycle, &service);
+        // Pin service via acquire to prevent unload during generation
+        rac_result_t result = rac_lifecycle_acquire_service(component->lifecycle, &service);
         if (result != RAC_SUCCESS) {
             RAC_LOG_ERROR("Diffusion.Component", "No model loaded - cannot generate");
             return result;
@@ -418,6 +418,9 @@ extern "C" rac_result_t rac_diffusion_component_generate(rac_handle_t handle,
     // Perform generation outside lock
     rac_result_t result = rac_diffusion_generate(service, &effective_options, out_result);
 
+    // Release pinned service in all exit paths
+    rac_lifecycle_release_service(component->lifecycle);
+
     if (result != RAC_SUCCESS) {
         RAC_LOG_ERROR("Diffusion.Component", "Generation failed: %d", result);
         rac_lifecycle_track_error(component->lifecycle, result, "generate");
@@ -491,8 +494,8 @@ extern "C" rac_result_t rac_diffusion_component_generate_with_callbacks(
         // Reset cancellation flag
         component->cancel_requested = false;
 
-        // Get service from lifecycle manager
-        rac_result_t result = rac_lifecycle_require_service(component->lifecycle, &service);
+        // Pin service via acquire to prevent unload during generation
+        rac_result_t result = rac_lifecycle_acquire_service(component->lifecycle, &service);
         if (result != RAC_SUCCESS) {
             RAC_LOG_ERROR("Diffusion.Component", "No model loaded - cannot generate");
             if (error_callback) {
@@ -526,6 +529,9 @@ extern "C" rac_result_t rac_diffusion_component_generate_with_callbacks(
     rac_result_t result = rac_diffusion_generate_with_progress(service, &effective_options,
                                                                diffusion_progress_wrapper, &ctx, &gen_result);
 
+    // Release pinned service in all exit paths
+    rac_lifecycle_release_service(component->lifecycle);
+
     if (result != RAC_SUCCESS) {
         RAC_LOG_ERROR("Diffusion.Component", "Generation failed: %d", result);
         rac_lifecycle_track_error(component->lifecycle, result, "generateWithCallbacks");

sdk/runanywhere-commons/src/features/llm/llm_component.cpp

@@ -223,9 +223,14 @@ extern "C" void rac_llm_component_destroy(rac_handle_t handle) {
 
     auto* component = reinterpret_cast<rac_llm_component*>(handle);
 
-    // Destroy lifecycle manager (will cleanup service if loaded)
-    if (component->lifecycle) {
-        rac_lifecycle_destroy(component->lifecycle);
+    // Acquire component mutex to serialize against in-flight operations.
+    // lifecycle_destroy -> unload will block until any acquired services are released.
+    {
+        std::lock_guard<std::mutex> lock(component->mtx);
+        if (component->lifecycle) {
+            rac_lifecycle_destroy(component->lifecycle);
+            component->lifecycle = nullptr;
+        }
     }
 
     log_info("LLM.Component", "LLM component destroyed");
@@ -771,13 +776,14 @@ extern "C" rac_result_t rac_llm_component_cancel(rac_handle_t handle) {
 
     auto* component = reinterpret_cast<rac_llm_component*>(handle);
 
-    // Do NOT acquire component->mtx here. generate_stream() holds the mutex
-    // for the entire streaming duration, so locking here would deadlock until
-    // generation finishes — defeating the purpose of cancel.
-    // rac_llm_cancel() ultimately sets an atomic bool, which is safe without the lock.
-    rac_handle_t service = rac_lifecycle_get_service(component->lifecycle);
-    if (service) {
+    // Use acquire/release to pin the service for the duration of the cancel call,
+    // preventing use-after-free if destroy races with cancel.
+    // Do NOT acquire component->mtx — generate_stream() holds it during streaming.
+    rac_handle_t service = nullptr;
+    rac_result_t acq = rac_lifecycle_acquire_service(component->lifecycle, &service);
+    if (acq == RAC_SUCCESS && service) {
         rac_llm_cancel(service);
+        rac_lifecycle_release_service(component->lifecycle);
     }
 
     log_info("LLM.Component", "Generation cancellation requested");

sdk/runanywhere-commons/src/features/vad/vad_analytics.cpp

@@ -71,7 +71,11 @@ rac_result_t rac_vad_analytics_create(rac_vad_analytics_handle_t* out_handle) {
         return RAC_ERROR_INVALID_PARAMETER;
     }
 
-    *out_handle = new (std::nothrow) rac_vad_analytics_s();
+    try {
+        *out_handle = new (std::nothrow) rac_vad_analytics_s();
+    } catch (...) {
+        *out_handle = nullptr;
+    }
     if (!*out_handle) {
         return RAC_ERROR_OUT_OF_MEMORY;
     }

sdk/runanywhere-commons/src/features/vad/vad_component.cpp

@@ -9,6 +9,7 @@
  * Do NOT add features not present in the Swift code.
  */
 
+#include <atomic>
 #include <chrono>
 #include <cstdlib>
 #include <cstring>
@@ -42,8 +43,8 @@ struct rac_vad_component {
     rac_vad_audio_callback_fn audio_callback;
     void* audio_user_data;
 
-    /** Initialization state */
-    bool is_initialized;
+    /** Initialization state (atomic for lock-free query from callbacks) */
+    std::atomic<bool> is_initialized;
 
     /** Mutex for thread safety */
     std::mutex mtx;
@@ -181,8 +182,7 @@ extern "C" rac_bool_t rac_vad_component_is_initialized(rac_handle_t handle) {
         return RAC_FALSE;
 
     auto* component = reinterpret_cast<rac_vad_component*>(handle);
-    std::lock_guard<std::mutex> lock(component->mtx);
-    return component->is_initialized ? RAC_TRUE : RAC_FALSE;
+    return component->is_initialized.load(std::memory_order_acquire) ? RAC_TRUE : RAC_FALSE;
 }
 
 extern "C" rac_result_t rac_vad_component_initialize(rac_handle_t handle) {
@@ -479,13 +479,8 @@ extern "C" rac_lifecycle_state_t rac_vad_component_get_state(rac_handle_t handle
         return RAC_LIFECYCLE_STATE_IDLE;
 
     auto* component = reinterpret_cast<rac_vad_component*>(handle);
-    std::lock_guard<std::mutex> lock(component->mtx);
-
-    if (component->is_initialized) {
-        return RAC_LIFECYCLE_STATE_LOADED;
-    }
-
-    return RAC_LIFECYCLE_STATE_IDLE;
+    return component->is_initialized.load(std::memory_order_acquire) ? RAC_LIFECYCLE_STATE_LOADED
+                                                                      : RAC_LIFECYCLE_STATE_IDLE;
 }
 
 extern "C" rac_result_t rac_vad_component_get_metrics(rac_handle_t handle,

sdk/runanywhere-commons/src/features/voice_agent/voice_agent.cpp

@@ -13,6 +13,7 @@
 #include <cstring>
 #include <mutex>
 #include <new>
+#include <thread>
 
 #include "rac/core/rac_analytics_events.h"
 #include "rac/core/rac_audio_utils.h"
@@ -48,6 +49,10 @@ struct rac_voice_agent {
     // State — atomic so is_ready() checks don't need the mutex
     std::atomic<bool> is_configured{false};
 
+    // Shutdown barrier — prevents use-after-free on destroy
+    std::atomic<bool> is_shutting_down{false};
+    std::atomic<int> in_flight{0};
+
     // Whether we own the component handles (and should destroy them)
     bool owns_components;
 
@@ -237,12 +242,17 @@ void rac_voice_agent_destroy(rac_voice_agent_handle_t handle) {
         return;
     }
 
+    // Signal shutdown and wait for all in-flight operations (including lock-free ones)
+    handle->is_shutting_down.store(true, std::memory_order_release);
+    handle->is_configured.store(false, std::memory_order_release);
+
+    // Spin-wait until all in-flight operations complete
+    while (handle->in_flight.load(std::memory_order_acquire) > 0) {
+        std::this_thread::yield();
+    }
+
     {
-        // Acquire mutex so any in-flight process_voice_turn / process_stream
-        // finishes before we tear down components. Setting is_configured=false
-        // ensures new callers return immediately after we release.
         std::lock_guard<std::mutex> lock(handle->mutex);
-        handle->is_configured.store(false, std::memory_order_release);
 
         if (handle->owns_components) {
             RAC_LOG_DEBUG("VoiceAgent", "Destroying owned component handles");
@@ -909,10 +919,23 @@ rac_result_t rac_voice_agent_detect_speech(rac_voice_agent_handle_t handle, cons
         return RAC_ERROR_INVALID_ARGUMENT;
     }
 
+    // Check shutdown barrier (this is a lock-free path)
+    if (handle->is_shutting_down.load(std::memory_order_acquire)) {
+        return RAC_ERROR_INVALID_STATE;
+    }
+    handle->in_flight.fetch_add(1, std::memory_order_acq_rel);
+
+    // Re-check after incrementing to avoid TOCTOU with destroy
+    if (handle->is_shutting_down.load(std::memory_order_acquire)) {
+        handle->in_flight.fetch_sub(1, std::memory_order_acq_rel);
+        return RAC_ERROR_INVALID_STATE;
+    }
+
     // VAD doesn't require is_configured (mirrors Swift)
     rac_result_t result =
         rac_vad_component_process(handle->vad_handle, samples, sample_count, out_speech_detected);
 
+    handle->in_flight.fetch_sub(1, std::memory_order_acq_rel);
     return result;
 }