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,65 +201,81 @@ 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
-        // after HTTP callback. Make sure it is atomic / thread-safe.
-        LOCKGUARD(m_flushLock);
+        try
+        {
+            // Flush could be executed from context of worker thread, as well as from TPM and
+            // after HTTP callback. Make sure it is atomic / thread-safe.
+            LOCKGUARD(m_flushLock);
 
-        // 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.
-        m_flushHandle.Cancel();
+            // 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 reporting nullptr once that task finishes.
+            m_flushHandle.Cancel();
 
-        size_t dbSizeBeforeFlush = (m_offlineStorageMemory != nullptr) ? m_offlineStorageMemory->GetSize() : 0;
-        if ((m_offlineStorageMemory) && (dbSizeBeforeFlush > 0) && (m_offlineStorageDisk))
-        {
-            // This will block on and then take a lock for the duration of this move, and
-            // StoreRecord() will then block until the move completes.
-            auto records = m_offlineStorageMemory->GetRecords(false, EventLatency_Unspecified);
-            std::vector<StorageRecordId> ids;
+            size_t dbSizeBeforeFlush = (m_offlineStorageMemory != nullptr) ? m_offlineStorageMemory->GetSize() : 0;
+            if ((m_offlineStorageMemory) && (dbSizeBeforeFlush > 0) && (m_offlineStorageDisk))
+            {
+                // This will block on and then take a lock for the duration of this move, and
+                // StoreRecord() will then block until the move completes.
+                auto records = m_offlineStorageMemory->GetRecords(false, EventLatency_Unspecified);
+                std::vector<StorageRecordId> ids;
+
+                // TODO: [MG] - consider running the batch in transaction
+                //            if (sqlite)
+                //                sqlite->Execute("BEGIN");
 
-            // TODO: [MG] - consider running the batch in transaction
-            //            if (sqlite)
-            //                sqlite->Execute("BEGIN");
+                size_t totalSaved = m_offlineStorageDisk->StoreRecords(records);
 
-            size_t totalSaved = m_offlineStorageDisk->StoreRecords(records);
+                // TODO: [MG] - consider running the batch in transaction
+                //            if (sqlite)
+                //                sqlite->Execute("END");
 
-            // TODO: [MG] - consider running the batch in transaction
-            //            if (sqlite)
-            //                sqlite->Execute("END");
+                // Delete records from reserved on flush
+                HttpHeaders dummy;
+                bool fromMemory = true;
+                m_offlineStorageMemory->DeleteRecords(ids, dummy, fromMemory);
 
-            // Delete records from reserved on flush
-            HttpHeaders dummy;
-            bool fromMemory = true;
-            m_offlineStorageMemory->DeleteRecords(ids, dummy, fromMemory);
+                // Notify event listener about the records cached
+                OnStorageRecordsSaved(totalSaved);
 
-            // Notify event listener about the records cached
-            OnStorageRecordsSaved(totalSaved);
+                if (m_offlineStorageMemory->GetSize() > dbSizeBeforeFlush)
+                {
+                    // We managed to accumulate as much data as we had before the flush,
+                    // means we cannot keep up flushing at the same speed as incoming
+                    // obviously because the disk is slower than ram.
+                    LOG_WARN("Data is arriving too fast!");
+                }
+            }
 
-            if (m_offlineStorageMemory->GetSize() > dbSizeBeforeFlush)
+            // Checkpoint DB
+            if (m_config.HasConfig(CFG_BOOL_CHECKPOINT_DB_ON_FLUSH) && m_config[CFG_BOOL_CHECKPOINT_DB_ON_FLUSH])
             {
-                // We managed to accumulate as much data as we had before the flush,
-                // means we cannot keep up flushing at the same speed as incoming
-                // obviously because the disk is slower than ram.
-                LOG_WARN("Data is arriving too fast!");
+                m_offlineStorageDisk->Flush();
             }
-        }
 
-        // Checkpoint DB
-        if (m_config.HasConfig(CFG_BOOL_CHECKPOINT_DB_ON_FLUSH) && m_config[CFG_BOOL_CHECKPOINT_DB_ON_FLUSH]) 
+            m_isStorageFullNotificationSend = false;
+            m_flushComplete.post();
+            m_flushPending = false;
+        }
+        catch (...)
         {
-            m_offlineStorageDisk->Flush();
+            LOCKGUARD(m_flushLock);
+            m_flushComplete.post();
+            m_flushPending = false;
+            throw;
         }
-
-        m_isStorageFullNotificationSend = false;
-
-        // Flush is done, notify the waiters
-        m_flushComplete.post();
-        m_flushPending = false;
-        m_logManager.EndActivity();
     }
 
     bool OfflineStorageHandler::StoreRecord(StorageRecord const& record)
@@ -260,7 +314,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;
     }