fix: resolve audit findings across cache, hooks, input, and logging

.github/workflows/coverage-pages.yml

@@ -69,10 +69,11 @@ jobs:
         shell: powershell
 
       - name: Run Tests
-        run: |
-          & ".\build\mingw-debug\tests\DetourModKit_tests.exe" --gtest_output=xml:test-results.xml
-          if ($LASTEXITCODE -ne 0) { throw "Tests failed with exit code $LASTEXITCODE" }
-        shell: powershell
+        # CTest runs each test case in its own process for state isolation between
+        # suites (see pr-check.yml). The per-test .gcda coverage data accumulates
+        # across those processes, so the gcovr step below still sees full coverage.
+        run: ctest --preset mingw-debug
+        shell: bash
 
       - name: Generate HTML Coverage Report
         run: |

.github/workflows/pr-check.yml

@@ -76,11 +76,16 @@ jobs:
         shell: powershell
 
       - name: Run Tests
-        timeout-minutes: 5
-        run: |
-          & ".\build\mingw-debug\tests\DetourModKit_tests.exe" --gtest_output=xml:test-results.xml
-          if ($LASTEXITCODE -ne 0) { throw "Tests failed with exit code $LASTEXITCODE" }
-        shell: powershell
+        # Run through CTest so each test case executes in its own process. The
+        # monolithic single-process run shares every singleton and file-scope static
+        # across suites, which lets one suite's teardown contaminate the next (for
+        # example a Bootstrap lifecycle worker still inside DMK_Shutdown when the next
+        # Bootstrap suite starts hooking). Per-process isolation matches the release
+        # and MSVC jobs and removes that class of flake. Coverage .gcda data still
+        # accumulates across the per-test processes for the gcovr step below.
+        timeout-minutes: 12
+        run: ctest --preset mingw-debug
+        shell: bash
 
       - name: Generate Coverage Report
         run: |

AGENTS.md

@@ -410,3 +410,7 @@ These are called at 60+ fps from game hook callbacks. Never add allocations, exc
 - **Do not use** `EventDispatcher::emit()` from hook callbacks -- use `emit_safe()` instead to prevent unhandled handler exceptions from crashing the host process.
 - **Do not return** from a memory-writing helper before its post-write cache maintenance (instruction-cache flush and cache-range invalidation) once bytes have been modified -- run the cleanup on every exit path, even when a later step such as restoring page protection fails.
 - **Do not let** a public doc comment describe behavior the implementation no longer has -- lifecycle and ordering claims must match the code, and are best pinned by a test.
+- **Do not take** a shared lock in a const query/accessor that can be called from inside a `with_*`/`try_with_*` callback without first making it reentrancy-guard-aware (skip the lock when the per-thread guard is non-zero, since the callback already holds it). Recursive shared acquisition of the non-recursive `shared_mutex`/SRWLOCK on one thread is undefined behavior and deadlocks if a writer is queued between the two acquisitions (see `HookManager::lock_hooks_shared_reentrant`).
+- **Do not key** a cache store and its invalidation/eviction by different addresses or shard-selection functions. Eviction must use the same canonical key and the same containment lookup as insertion and read, or entries silently survive invalidation (see `Memory::invalidate_range`, which scans every shard because storage is sharded by query address, not region base).
+- **Do not let** a queue or backlog fed at an external event rate grow without bound -- clamp the pending count to a documented ceiling (see `Input` wheel-notch `MAX_WHEEL_PENDING`).
+- **Do not let** an async/deferred sink diverge from its synchronous counterpart's configuration (timestamp format, level, etc.) -- carry the same settings through, as `Logger::enable_async_mode` does for the timestamp format.

docs/tests/README.md

@@ -313,6 +313,10 @@ The decoder header lives under `src/` (not the public include tree), so the test
 | `WheelPulseTest.SingleNotchPulsesThenGoesLow` | One queued notch reads pressed for exactly one cycle, then is forced low so the edge detector re-arms. |
 | `WheelPulseTest.TwoNotchesProduceTwoSeparatedPulses` | Two queued notches fire as two distinct Press edges separated by a forced low cycle, never one fused press. |
 | `WheelPulseTest.DirectionsAreIndependent` | Pending notches in different directions pulse on the same cycle without interfering. |
+| `WheelPulseTest.AddWheelNotchesAccumulatesBelowCap` | Freshly drained notches add to the pending backlog while it stays under the cap. |
+| `WheelPulseTest.AddWheelNotchesClampsRunawayBacklog` | A single huge burst and repeated bursts both saturate at `MAX_WHEEL_PENDING`, so a sustained fast scroll cannot queue notches without bound. |
+| `WheelPulseTest.AddWheelNotchesIgnoresNegativeCounts` | A corrupt negative count is ignored rather than driving the pending counter negative. |
+| `WheelPulseTest.CappedBacklogStillDrainsOnePulsePerNotch` | After the backlog saturates, each retained notch still drains as exactly one Press edge. |
 | `GamepadSuppressTest.BarePressIsNotSuppressed` | A trigger physically down with no active chord (`owned_now == 0`) is left untouched so the game still sees it. |
 | `GamepadSuppressTest.ActiveChordSuppressesTrigger` | A trigger claimed by an active chord is added to the clear mask. |
 | `GamepadSuppressTest.ModifierReleasedBeforeTriggerKeepsSuppressing` | Releasing the modifier a frame before the trigger does not leak a bare trigger: suppression latches to the trigger button's own lifetime. |

include/DetourModKit/async_logger.hpp

@@ -264,6 +264,13 @@ namespace DetourModKit
         size_t spin_backoff_iterations = DEFAULT_SPIN_BACKOFF_ITERATIONS;
         std::chrono::milliseconds block_timeout_ms{16};
         size_t block_max_spin_iterations{1000};
+        /**
+         * @brief strftime-style date/time format for the async sink.
+         * @details Kept in sync with the synchronous Logger by Logger::enable_async_mode so
+         *          both sinks emit identical timestamps; the trailing ".<ms>" is appended by
+         *          the writer, not this format.
+         */
+        std::string timestamp_format{"%Y-%m-%d %H:%M:%S"};
 
         [[nodiscard]] constexpr bool validate() const noexcept
         {

include/DetourModKit/config.hpp

@@ -459,6 +459,11 @@ namespace DetourModKit
          *       is logged and the watcher remains running. Tear the watcher
          *       down from a different thread, e.g. by posting the disable
          *       request to a deferred shutdown hook.
+         * @note A config change still inside the debounce window when this is called
+         *       fires one final reload (running the registered setters) during the
+         *       stop, so disabling auto-reload does not guarantee no further setter
+         *       invocation. Callers that require a hard stop should latch their own
+         *       guard around setter side effects.
          */
         void disable_auto_reload() noexcept;
 

include/DetourModKit/config_watcher.hpp

@@ -100,6 +100,10 @@ namespace DetourModKit
          *          which case the worker is detached (see
          *          StoppableWorker::shutdown()). Returns promptly
          *          (~100 ms bound) regardless.
+         * @note A change that arrived within the debounce window but had not yet
+         *       fired triggers one final @p on_reload callback during this stop, so
+         *       stop() does not guarantee that no further callback runs. Callers that
+         *       must observe no callback after stopping should latch their own flag.
          */
         void stop() noexcept;
 

include/DetourModKit/hook_manager.hpp

@@ -1116,7 +1116,13 @@ namespace DetourModKit
          */
         mutable std::shared_mutex m_mutator_gate;
 
-        [[nodiscard]] int &get_reentrancy_guard() noexcept
+        /**
+         * @brief Returns the thread-local reentrancy depth counter.
+         * @details Declared const so the const read-only query accessors can consult the
+         *          guard. The counter is thread-local and independent of object state, so
+         *          const is honest here.
+         */
+        [[nodiscard]] int &get_reentrancy_guard() const noexcept
         {
             thread_local int reentrancy_counter{0};
             return reentrancy_counter;
@@ -1134,6 +1140,27 @@ namespace DetourModKit
             ReentrancyGuard &operator=(ReentrancyGuard &&) = delete;
         };
 
+        /**
+         * @brief Acquires m_hooks_mutex shared, or returns a disengaged lock when reentrant.
+         * @details A with_* or try_with_* callback already holds m_hooks_mutex shared on
+         *          this thread and has bumped the reentrancy guard. Re-locking a non-recursive
+         *          std::shared_mutex from the same thread is undefined behavior (and can
+         *          deadlock if a writer is queued between the two acquisitions), so a const
+         *          query accessor invoked from inside such a callback must read under the
+         *          lock the callback already holds instead of taking a second shared lock.
+         *          When not reentrant (guard == 0) the returned lock owns m_hooks_mutex for
+         *          the caller's scope exactly as a direct shared_lock would.
+         * @return An engaged shared_lock when guard == 0, otherwise a disengaged one.
+         */
+        [[nodiscard]] std::shared_lock<std::shared_mutex> lock_hooks_shared_reentrant() const
+        {
+            if (get_reentrancy_guard() > 0)
+            {
+                return std::shared_lock<std::shared_mutex>{};
+            }
+            return std::shared_lock<std::shared_mutex>(m_hooks_mutex);
+        }
+
         std::string error_to_string(const safetyhook::InlineHook::Error &err) const;
         std::string error_to_string(const safetyhook::MidHook::Error &err) const;
 

include/DetourModKit/memory.hpp

@@ -234,13 +234,22 @@ namespace DetourModKit
          * @return The pointer-sized value at the address, or 0 if either the source
          *         address or the dereferenced value falls outside the user-mode
          *         window (min_valid, @ref USERSPACE_PTR_MAX).
+         * @note The lower bound is exclusive here, whereas plausible_userspace_ptr
+         *       treats the same bound as inclusive. The difference is intentional:
+         *       this function performs an unguarded dereference, so it is one address
+         *       more conservative and never blindly reads the boundary itself, while
+         *       plausible_userspace_ptr only screens a value arithmetically.
          */
         inline uintptr_t read_ptr_unchecked(uintptr_t base, ptrdiff_t offset,
                                             uintptr_t min_valid = 0x10000) noexcept
         {
             const auto src = base + static_cast<uintptr_t>(offset);
-            // Accept the source only inside the user-mode window
-            // (min_valid, USERSPACE_PTR_MAX). The ceiling rejects kernel-range and
+            // Accept the source only strictly inside the user-mode window
+            // (min_valid, USERSPACE_PTR_MAX). The floor is deliberately exclusive: this
+            // dereference is unguarded in release, so min_valid itself is treated as the
+            // first address NOT trusted for a blind read (one address more conservative
+            // than plausible_userspace_ptr, which is a pure no-deref pre-screen and so is
+            // inclusive at the same bound). The ceiling rejects kernel-range and
             // non-canonical sources; together with the floor it also subsumes any
             // pointer-arithmetic wraparound, because a ptrdiff_t offset is too small
             // to carry base back into this window (a non-negative offset cannot

src/async_logger.cpp

@@ -482,29 +482,35 @@ namespace DetourModKit
         if (m_shutdown_requested.load(std::memory_order_acquire))
         {
             std::lock_guard<std::mutex> lock(*m_log_mutex);
-            if (m_file_stream->is_open() && m_file_stream->good())
+            if (!m_file_stream->is_open() || !m_file_stream->good())
             {
-                const auto now = std::chrono::system_clock::now();
-                const auto time_t = std::chrono::system_clock::to_time_t(now);
-                std::tm tm_buf{};
+                // Stream already closed or failed during teardown: the message cannot be
+                // delivered, so report the drop rather than a false success.
+                return false;
+            }
+
+            const auto now = std::chrono::system_clock::now();
+            const auto time_t = std::chrono::system_clock::to_time_t(now);
+            std::tm tm_buf{};
 
 #if defined(_WIN32) || defined(_MSC_VER)
-                localtime_s(&tm_buf, &time_t);
+            localtime_s(&tm_buf, &time_t);
 #else
-                localtime_r(&time_t, &tm_buf);
+            localtime_r(&time_t, &tm_buf);
 #endif
 
-                const auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(
-                                    now.time_since_epoch()) %
-                                1000;
-                *m_file_stream << "[" << std::put_time(&tm_buf, "%Y-%m-%d %H:%M:%S")
-                               << "." << std::setfill('0') << std::setw(3) << ms.count()
-                               << std::setfill(' ') << "] "
-                               << "[" << std::setw(7) << std::left << log_level_to_string(level) << "] :: "
-                               << message << '\n';
-                m_file_stream->flush();
-            }
-            return true;
+            const auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(
+                                now.time_since_epoch()) %
+                            1000;
+            *m_file_stream << "[" << std::put_time(&tm_buf, m_config.timestamp_format.c_str())
+                           << "." << std::setfill('0') << std::setw(3) << ms.count()
+                           << std::setfill(' ') << "] "
+                           << "[" << std::setw(7) << std::left << log_level_to_string(level) << "] :: "
+                           << message << '\n';
+            m_file_stream->flush();
+
+            // Surface a write/flush failure through the no-throw delivery bool.
+            return m_file_stream->good();
         }
 
         LogMessage msg(level, message);
@@ -710,7 +716,7 @@ namespace DetourModKit
                                 msg.timestamp.time_since_epoch()) %
                             1000;
 
-            *m_file_stream << "[" << std::put_time(&cached_tm, "%Y-%m-%d %H:%M:%S")
+            *m_file_stream << "[" << std::put_time(&cached_tm, m_config.timestamp_format.c_str())
                            << "." << std::setfill('0') << std::setw(3) << ms.count()
                            << std::setfill(' ') << "] "
                            << "[" << std::setw(7) << std::left << log_level_to_string(msg.level) << "] :: "
@@ -806,7 +812,7 @@ namespace DetourModKit
             const auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(
                                 message.timestamp.time_since_epoch()) %
                             1000;
-            *m_file_stream << "[" << std::put_time(&tm_buf, "%Y-%m-%d %H:%M:%S")
+            *m_file_stream << "[" << std::put_time(&tm_buf, m_config.timestamp_format.c_str())
                            << "." << std::setfill('0') << std::setw(3) << ms.count()
                            << std::setfill(' ') << "] "
                            << "[" << std::setw(7) << std::left << log_level_to_string(message.level) << "] :: "

src/hook_manager.cpp

@@ -651,7 +651,7 @@ bool HookManager::is_target_already_hooked(uintptr_t target_address) const noexc
     {
         return false;
     }
-    std::shared_lock<std::shared_mutex> lock(m_hooks_mutex);
+    const auto lock = lock_hooks_shared_reentrant();
     for (const auto &[name, hook_ptr] : m_hooks)
     {
         if (hook_ptr->get_type() == HookType::Inline &&
@@ -1039,7 +1039,7 @@ std::size_t HookManager::disable_all_hooks()
 
 std::optional<HookStatus> HookManager::get_hook_status(std::string_view hook_id) const
 {
-    std::shared_lock<std::shared_mutex> lock(m_hooks_mutex);
+    const auto lock = lock_hooks_shared_reentrant();
     auto it = m_hooks.find(hook_id);
     if (it != m_hooks.end())
     {
@@ -1050,7 +1050,7 @@ std::optional<HookStatus> HookManager::get_hook_status(std::string_view hook_id)
 
 std::unordered_map<HookStatus, size_t> HookManager::get_hook_counts() const
 {
-    std::shared_lock<std::shared_mutex> lock(m_hooks_mutex);
+    const auto lock = lock_hooks_shared_reentrant();
     std::unordered_map<HookStatus, size_t> counts;
     counts[HookStatus::Active] = 0;
     counts[HookStatus::Disabled] = 0;
@@ -1063,7 +1063,7 @@ std::unordered_map<HookStatus, size_t> HookManager::get_hook_counts() const
 
 std::vector<std::string> HookManager::get_hook_ids(std::optional<HookStatus> status_filter) const
 {
-    std::shared_lock<std::shared_mutex> lock(m_hooks_mutex);
+    const auto lock = lock_hooks_shared_reentrant();
     std::vector<std::string> ids;
     ids.reserve(m_hooks.size());
     for (const auto &[name, hook_ptr] : m_hooks)
@@ -1261,7 +1261,27 @@ bool HookManager::apply_vmt_hook(std::string_view vmt_name, void *object)
                     {{std::format("HookManager: VMT hook '{}' not found for apply.", vmt_name), LogLevel::Warning}}};
         }
 
-        it->second.vmt_hook().apply(object);
+        // VmtHook::apply tracks the object in an internal container, so it can throw
+        // bad_alloc. Contain it here as every other SafetyHook call site does, so a
+        // failed apply returns false instead of unwinding out through the held locks.
+        try
+        {
+            it->second.vmt_hook().apply(object);
+        }
+        catch (const std::exception &e)
+        {
+            return {false,
+                    {{std::format("HookManager: Exception applying VMT hook '{}' to object {}: {}",
+                                  vmt_name, DetourModKit::Format::format_address(reinterpret_cast<uintptr_t>(object)), e.what()),
+                      LogLevel::Error}}};
+        }
+        catch (...)
+        {
+            return {false,
+                    {{std::format("HookManager: Unknown exception applying VMT hook '{}' to object {}.",
+                                  vmt_name, DetourModKit::Format::format_address(reinterpret_cast<uintptr_t>(object))),
+                      LogLevel::Error}}};
+        }
         return {true,
                 {{std::format("HookManager: VMT hook '{}' applied to object {}.",
                               vmt_name, DetourModKit::Format::format_address(reinterpret_cast<uintptr_t>(object))),
@@ -1353,7 +1373,7 @@ void HookManager::remove_all_vmt_hooks()
 
 std::vector<std::string> HookManager::get_vmt_hook_names() const
 {
-    std::shared_lock<std::shared_mutex> lock(m_hooks_mutex);
+    const auto lock = lock_hooks_shared_reentrant();
     std::vector<std::string> names;
     names.reserve(m_vmt_hooks.size());
     for (const auto &[name, entry] : m_vmt_hooks)

src/input.cpp

@@ -629,11 +629,7 @@ namespace DetourModKit
             if (m_has_wheel_bindings.load(std::memory_order_relaxed))
             {
                 const auto taken = detail::take_wheel_counts();
-                for (int dir = 0; dir < 4; ++dir)
-                {
-                    wheel_pulse.pending[static_cast<size_t>(dir)] +=
-                        taken[static_cast<size_t>(dir)];
-                }
+                detail::add_wheel_notches(wheel_pulse, taken);
                 wheel_pulse_mask = detail::step_wheel_pulse(wheel_pulse);
             }
 

src/input_intercept.cpp

@@ -367,6 +367,19 @@ namespace DetourModKit::detail
         return mask;
     }
 
+    void add_wheel_notches(WheelPulseState &state, const std::array<int, 4> &taken) noexcept
+    {
+        for (size_t dir = 0; dir < 4; ++dir)
+        {
+            const int add = taken[dir] > 0 ? taken[dir] : 0;
+            // pending is in [0, MAX_WHEEL_PENDING] by induction, so room is non-negative.
+            // Compare against room before adding so a large burst saturates rather than
+            // overflowing the int sum.
+            const int room = MAX_WHEEL_PENDING - state.pending[dir];
+            state.pending[dir] = (add >= room) ? MAX_WHEEL_PENDING : state.pending[dir] + add;
+        }
+    }
+
     uint16_t step_gamepad_suppress(GamepadSuppressState &state,
                                    uint16_t owned_now,
                                    uint16_t true_buttons,