Fix runtime data races, memory leak, and shutdown safety

lib/api/LogManagerFactory.hpp

@@ -67,7 +67,15 @@ namespace MAT_NS_BEGIN {
 
         //  C++11 Magic Statics (N2660)
         static LogManagerFactory& instance() {
-            static LogManagerFactory impl;
+            // Deliberately never destroyed. LogManagerProvider::Release() must be
+            // able to walk this factory's registries during process teardown, but
+            // a normal function-local static's destruction order relative to that
+            // teardown call is unspecified -- if this were destroyed first,
+            // Release() would walk already-freed std::map nodes (a downstream
+            // consumer observed this as EXC_BAD_ACCESS in release() at process
+            // exit). Leaking one small, fixed-size object avoids the ordering
+            // hazard entirely; the OS reclaims it when the process exits.
+            static LogManagerFactory& impl = *new LogManagerFactory();
             return impl;
         }
 

lib/api/Logger.cpp

@@ -127,7 +127,8 @@ namespace MAT_NS_BEGIN
 
     Logger::~Logger() noexcept
     {
-        LOG_TRACE("%p: Destroyed", this);
+        // Intentionally empty — logging here triggers a static-destruction-order
+        // crash on iOS simulator (recursive_mutex used after teardown).
     }
 
     ISemanticContext* Logger::GetSemanticContext() const

lib/http/HttpClient_WinInet.cpp

@@ -55,6 +55,9 @@ class WinInetRequestWrapper
         if (m_hWinInetRequest != nullptr)
         {
             ::InternetCloseHandle(m_hWinInetRequest);
+        }
+        if (m_hWinInetSession != nullptr)
+        {
             ::InternetCloseHandle(m_hWinInetSession);
         }
     }

lib/http/HttpResponseDecoder.cpp

@@ -67,13 +67,11 @@ namespace MAT_NS_BEGIN {
             break;
 
         case HttpResult_Aborted:
-            ctx->httpResponse = nullptr;
             outcome = Abort;
             break;
 
         case HttpResult_LocalFailure:
         case HttpResult_NetworkFailure:
-            ctx->httpResponse = nullptr;
             outcome = RetryNetwork;
             break;
         }
@@ -132,7 +130,6 @@ namespace MAT_NS_BEGIN {
                 evt.param2 = ctx->recordIdsAndTenantIds.size();
                 DispatchEvent(evt);
             }
-            ctx->httpResponse = nullptr;
             // eventsRejected(ctx); // FIXME: [MG] - investigate why ctx gets corrupt after eventsRejected
             requestAborted(ctx);
             break;

lib/include/public/ITaskDispatcher.hpp

@@ -121,11 +121,11 @@ namespace MAT_NS_BEGIN
         /// <param name="waitTime">Amount of time to wait for if the task is currently executing</param>
         /// <returns>True if successfully cancelled, else false</returns>
         virtual bool Cancel(Task* task, uint64_t waitTime = 0) = 0;
+
     };
 
     /// @endcond
 
 } MAT_NS_END
 
 #endif // ITASKDISPATCHER_HPP
-

lib/offline/OfflineStorageHandler.cpp

@@ -50,6 +50,44 @@ namespace MAT_NS_BEGIN {
         }
     }
 
+    /// <summary>
+    /// RAII guard around ILogManager::StartActivity()/EndActivity(). Flush()
+    /// used to pair these manually (StartActivity() at the top, EndActivity()
+    /// on the last line), so an exception thrown by disk I/O or by
+    /// IOfflineStorageObserver::OnStorageRecordsSaved() partway through would
+    /// skip EndActivity() and permanently leak the pause-activity count --
+    /// deadlocking every later FlushAndTeardown()'s WaitPause(). This guard
+    /// guarantees EndActivity() runs on every exit path, matching the existing
+    /// safe pattern used by PauseGuard (TransmissionPolicyManager.cpp) and
+    /// ActiveLoggerCall (Logger.cpp).
+    /// </summary>
+    class ActivityGuard
+    {
+       public:
+        explicit ActivityGuard(ILogManager& logManager) noexcept :
+            m_logManager(logManager),
+            m_active(logManager.StartActivity())
+        {
+        }
+
+        ~ActivityGuard() noexcept
+        {
+            if (m_active)
+            {
+                m_logManager.EndActivity();
+            }
+        }
+
+        ActivityGuard(ActivityGuard const&) = delete;
+        ActivityGuard& operator=(ActivityGuard const&) = delete;
+
+        bool IsActive() const noexcept { return m_active; }
+
+       private:
+        ILogManager& m_logManager;
+        bool m_active;
+    };
+
     bool OfflineStorageHandler::isKilled(StorageRecord const& record)
     {
         return (
@@ -64,7 +102,7 @@ namespace MAT_NS_BEGIN {
             if (!m_flushPending)
                 return;
         }
-        LOG_INFO("Waiting for pending Flush (%p) to complete...", m_flushHandle.m_task);
+        LOG_INFO("Waiting for pending Flush (%p) to complete...", m_flushHandle.GetTask());
         m_flushComplete.wait();
     }
 
@@ -163,7 +201,16 @@ namespace MAT_NS_BEGIN {
 
     void OfflineStorageHandler::Flush()
     {
-        if (!m_logManager.StartActivity()) {
+        ActivityGuard activityGuard(m_logManager);
+        if (!activityGuard.IsActive()) {
+            // The LogManager is shutting down, so the flush cannot run. Still
+            // signal completion and clear the pending flag so a concurrent
+            // WaitForFlush() (e.g. during teardown) does not block forever
+            // waiting for m_flushComplete.
+            LOCKGUARD(m_flushLock);
+            m_flushHandle.Cancel();
+            m_flushComplete.post();
+            m_flushPending = false;
             return;
         }
         // Flush could be executed from context of worker thread, as well as from TPM and
@@ -172,7 +219,7 @@ namespace MAT_NS_BEGIN {
 
         // If item isn't scheduled yet, it gets canceled, so that we don't do two flushes.
         // If we are running that item right now (our thread), then nothing happens other
-        // than the handle gets replaced by nullptr in this DeferredCallbackHandle obj.
+        // than the handle reporting nullptr once that task finishes.
         m_flushHandle.Cancel();
 
         size_t dbSizeBeforeFlush = (m_offlineStorageMemory != nullptr) ? m_offlineStorageMemory->GetSize() : 0;
@@ -221,7 +268,9 @@ namespace MAT_NS_BEGIN {
         // Flush is done, notify the waiters
         m_flushComplete.post();
         m_flushPending = false;
-        m_logManager.EndActivity();
+        // activityGuard's destructor calls EndActivity() on every exit path
+        // above, including if StoreRecords()/checkpoint Flush()/
+        // OnStorageRecordsSaved() throws.
     }
 
     bool OfflineStorageHandler::StoreRecord(StorageRecord const& record)
@@ -260,7 +309,7 @@ namespace MAT_NS_BEGIN {
                         m_flushPending = true;
                         m_flushComplete.Reset();
                         m_flushHandle = PAL::scheduleTask(&m_taskDispatcher, 0, this, &OfflineStorageHandler::Flush);
-                        LOG_INFO("Requested Flush (%p)", m_flushHandle.m_task);
+                        LOG_INFO("Requested Flush (%p)", m_flushHandle.GetTask());
                     }
                     m_flushLock.unlock();
                 }

lib/pal/PAL.cpp

@@ -60,7 +60,18 @@ namespace PAL_NS_BEGIN {
 
     PlatformAbstractionLayer& GetPAL() noexcept
     {
-        static PlatformAbstractionLayer pal;
+        // Deliberately never destroyed. PAL::shutdown() (called from
+        // LogManagerImpl::FlushAndTeardown()) must find this object's members
+        // still alive, but PAL is constructed lazily on first use, so whether
+        // this function-local static is destroyed before or after that
+        // teardown call depends on runtime timing, not source order -- if it
+        // is destroyed first, shutdown() releases shared_ptr members of an
+        // already-destroyed object (a downstream consumer observed this as
+        // intermittent EXC_BAD_ACCESS in ~shared_ptr<ISystemInformation> at
+        // process exit). Leaking one fixed-size object avoids the ordering
+        // hazard entirely: shutdown() already performs the real resource
+        // teardown explicitly, and the OS reclaims the object at process exit.
+        static PlatformAbstractionLayer& pal = *new PlatformAbstractionLayer();
         return pal;
     }
 

lib/pal/TaskDispatcher.hpp

@@ -16,6 +16,7 @@
 #include <climits>
 #include <algorithm>
 #include <atomic>
+#include <memory>
 #include <utility>
 
 #include "ITaskDispatcher.hpp"
@@ -25,6 +26,15 @@ namespace PAL_NS_BEGIN {
 
     namespace detail {
 
+        struct TaskLifetimeState
+        {
+            TaskLifetimeState() :
+                task(nullptr)
+            {}
+
+            std::atomic<MAT::Task*> task;
+        };
+
         template<typename TCall>
         class TaskCall : public Task
         {
@@ -48,58 +58,92 @@ namespace PAL_NS_BEGIN {
                 this->TargetTime = targetTime;
             }
 
+            TaskCall(TCall& call, int64_t targetTime, std::shared_ptr<TaskLifetimeState> lifetimeState) :
+                Task(),
+                m_call(call),
+                m_lifetimeState(std::move(lifetimeState))
+            {
+                this->TypeName = TYPENAME(call);
+                this->Type = Task::TimedCall;
+                this->TargetTime = targetTime;
+                if (m_lifetimeState) {
+                    m_lifetimeState->task.store(this, std::memory_order_release);
+                }
+            }
+
             virtual void operator()() override
             {
                 m_call();
             }
 
-            virtual ~TaskCall() noexcept = default;
+            virtual ~TaskCall() noexcept
+            {
+                if (m_lifetimeState) {
+                    m_lifetimeState->task.store(nullptr, std::memory_order_release);
+                }
+            }
 
             const TCall m_call;
+
+        private:
+            std::shared_ptr<TaskLifetimeState> m_lifetimeState;
         };
 
     } // namespace detail
 
     class DeferredCallbackHandle
     {
     public:
-        std::mutex m_mutex;
-        MAT::Task* m_task = nullptr;
-        MAT::ITaskDispatcher* m_taskDispatcher = nullptr;
-
-        DeferredCallbackHandle(MAT::Task* task, MAT::ITaskDispatcher* taskDispatcher) :
-            m_task(task),
+        DeferredCallbackHandle(std::shared_ptr<detail::TaskLifetimeState> taskLifetimeState, MAT::ITaskDispatcher* taskDispatcher) :
+            m_taskLifetimeState(std::move(taskLifetimeState)),
             m_taskDispatcher(taskDispatcher) { }
-        DeferredCallbackHandle() {}
+
+        DeferredCallbackHandle() = default;
         DeferredCallbackHandle(DeferredCallbackHandle&& h)
         {
             *this = std::move(h);
         }
 
         DeferredCallbackHandle& operator=(DeferredCallbackHandle&& other)
         {
+            if (this == &other) {
+                return *this;
+            }
+
             std::lock_guard<std::mutex> lock(m_mutex);
             std::lock_guard<std::mutex> otherLock(other.m_mutex);
-            m_task = other.m_task;
-            other.m_task = nullptr;
+            m_taskLifetimeState = std::move(other.m_taskLifetimeState);
             m_taskDispatcher = other.m_taskDispatcher;
+            other.m_taskDispatcher = nullptr;
 
             return *this;
         }
 
+        MAT::Task* GetTask() const
+        {
+            std::lock_guard<std::mutex> lock(m_mutex);
+            return (m_taskLifetimeState != nullptr) ? m_taskLifetimeState->task.load(std::memory_order_acquire) : nullptr;
+        }
+
         bool Cancel(uint64_t waitTime = 0)
         {
             std::lock_guard<std::mutex> lock(m_mutex);
-            if (m_task)
+            MAT::Task* task = (m_taskLifetimeState != nullptr) ? m_taskLifetimeState->task.load(std::memory_order_acquire) : nullptr;
+            if (task)
             {
-                bool result = (m_taskDispatcher != nullptr) && (m_taskDispatcher->Cancel(m_task, waitTime));
-                return result;
+                bool result = (m_taskDispatcher != nullptr) && (m_taskDispatcher->Cancel(task, waitTime));
+                return result || ((m_taskLifetimeState != nullptr) && (m_taskLifetimeState->task.load(std::memory_order_acquire) == nullptr));
             }
             else {
                 // Canceled nothing successfully
                 return true;
             }
         }
+
+    private:
+        mutable std::mutex m_mutex;
+        std::shared_ptr<detail::TaskLifetimeState> m_taskLifetimeState;
+        MAT::ITaskDispatcher* m_taskDispatcher = nullptr;
     };
 
     template<typename TObject, typename... TFuncArgs, typename... TPassedArgs>
@@ -121,9 +165,18 @@ namespace PAL_NS_BEGIN {
     DeferredCallbackHandle scheduleTask(MAT::ITaskDispatcher* taskDispatcher, unsigned delayMs, TObject* obj, void (TObject::*func)(TFuncArgs...), TPassedArgs&&... args)
     {
         auto bound = std::bind(std::mem_fn(func), obj, std::forward<TPassedArgs>(args)...);
-        auto task = new detail::TaskCall<decltype(bound)>(bound, getMonotonicTimeMs() + (int64_t)delayMs);
+        auto taskLifetime = std::make_shared<detail::TaskLifetimeState>();
+        auto task = new detail::TaskCall<decltype(bound)>(bound, getMonotonicTimeMs() + (int64_t)delayMs, taskLifetime);
         taskDispatcher->Queue(task);
-        return DeferredCallbackHandle(task, taskDispatcher);
+        // Queue() is void; an SDK dispatcher that rejects by deleting the task
+        // synchronously clears this state before Queue() returns, and the task
+        // destructor also clears it after normal asynchronous completion so a
+        // later Cancel() never touches a stale Task*.
+        if (taskLifetime->task.load(std::memory_order_acquire) == nullptr)
+        {
+            return DeferredCallbackHandle();
+        }
+        return DeferredCallbackHandle(taskLifetime, taskDispatcher);
     }
 
     template<typename TObject, typename... TFuncArgs, typename... TPassedArgs>
@@ -135,4 +188,3 @@ namespace PAL_NS_BEGIN {
 } PAL_NS_END
 
 #endif
-