fix: harden profiler, hook manager, and logger

[tkhquang]

Summary

Addresses all v3.1.0 audit findings. Both release blockers and six strong recommendations are resolved with local fixes; no public API changes beyond the two appended enum values already scoped for v3.1.0.

Changes

• Profiler: replace seqlock RMW with fetch_add(1, acq_rel) on open and close, eliminating stale-publish races under concurrent wrap collisions.
• HookManager: destructor now acquires m_mutator_gate before clearing maps, drains in-flight readers, and pins the module while leaking maps (not Hook objects) when running under loader lock.
• Logger: s_leaked_loggers is now an append-only vector so repeated shutdown cycles in one process do not drop prior handles.
• Scanner: prologue-fallback hit counting is actually consulted via count_pattern_hits_bounded; ambiguous candidates (hits > cap) are rejected with ResolveError::NoMatch.
• InputManager: update_binding_combos cardinality mismatch is now logged at Warning level (was Debug) so silent rejections are visible.
• CMakeLists: -O2 downgrade is scoped to src/scanner.cpp under $<$<CONFIG:Release>:-O2> only, restoring -O3 on the rest of the tree.
• ScopedProfile: now compile-time literal-only via array-reference template constructor, preventing dangling std::string::c_str() UB on export.

Test plan

• Build succeeds on MinGW and MSVC presets (Debug and Release).
• ConcurrentRecordAndExport_NoTornReads passes under TSan.
• LateShutdown_DrainsReadersBeforeClearingMaps passes under ASan.
• PrologueFallbackRejectsAmbiguousTail passes.
• Three static_assert checks in test_profiler.cpp confirm ScopedProfile literal-only enforcement.
• Manual smoke test of an existing mod confirms no API breakage.

Summary by CodeRabbit

• New Features

  • Compiler-enforced string-literal requirement for DMK_PROFILE_SCOPE macro names, ensuring captured profiler data references static memory only.

• Bug Fixes

  • Improved shutdown robustness with enhanced concurrent reader draining and exclusive cleanup serialization.
  • Refined profiler sequence counter protocol to prevent counter rollback under producer races.

• Documentation

  • Updated profiler ring-buffer synchronization and string-literal behavior documentation.
  • Clarified shutdown memory cleanup expectations and async-logger singleton lifecycle.

• Chores

  • Input combo cardinality mismatch reporting upgraded from Debug to Warning log level.

[coderabbitai]

Warning

Rate limit exceeded

@tkhquang has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 28 minutes and 9 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 9 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a3fa1e4f-2e32-4e28-a691-d2fb36a40c62

📥 Commits

Reviewing files that changed from the base of the PR and between db61a4a and c5dfb0c.

📒 Files selected for processing (8)

• README.md
• docs/hot-reload/README.md
• include/DetourModKit/profiler.hpp
• src/logger.cpp
• src/profiler.cpp
• tests/test_hook_manager.cpp
• tests/test_profiler.cpp
• tests/test_scanner.cpp

📝 Walkthrough

Walkthrough

The PR updates shutdown semantics and thread-safety documentation for HookManager, Logger, and Profiler while implementing targeted changes: HookManager gains a loader-lock-aware destructor fallback, Logger retains multiple leaked AsyncLogger instances in a vector, Profiler shifts to monotonic sequence-counter increments with fetch_add, and ScopedProfile enforces compile-time string-literal requirements. The build system applies targeted -O2 optimization to scanner.cpp instead of globally downgrading Release builds.

Changes

Cohort / File(s)	Summary
Documentation Updates AGENTS.md, README.md, include/DetourModKit.hpp, include/DetourModKit/async_logger.hpp, include/DetourModKit/config.hpp, src/async_logger.cpp	Updated thread-safety semantics, shutdown behavior, and cleanup guarantees in architecture documentation and Doxygen comments. Clarifies StringPool singleton intentional leak, loader-lock safety during shutdown, and async-logger lifetime across hot-reload scenarios.
HookManager Shutdown Redesign src/hook_manager.cpp	Restructured destructor with conditional early-return on m_shutdown_called, added loader-lock detection branch that pins module and leaks maps to avoid deadlock, and normal path now serializes with mutators via exclusive m_mutator_gate while draining readers under shared m_hooks_mutex before clearing maps.
Logger Leak Retention Strategy src/logger.cpp	Changed from storing single prior AsyncLogger shared_ptr to appending multiple instances into static std::vector during detachment, preserving handles to prevent use-after-free when detached writer threads access old logger state during hot-reload.
Profiler Sequence Protocol & ScopedProfile Constraint include/DetourModKit/profiler.hpp, src/profiler.cpp	Replaced odd/even load-based sequence tracking with unconditional monotonic fetch_add(1) on both open and close to prevent counter rollback under producer races. Modified ScopedProfile constructor to template on const char (&)[N] to enforce compile-time string-literal requirement and reject function-local/heap strings.
Build System Optimization Targeting CMakeLists.txt	Replaced global Release -O3 → -O2 downgrade with targeted source-property assignment of -O2 exclusively to src/scanner.cpp, allowing remaining codebase to retain full Release-level optimization.
Logging Severity Adjustment src/input.cpp	Updated cardinality mismatch reporting in InputPoller::update_combos and InputManager::update_binding_combos from Logger::debug to Logger::warning while preserving early-return/ignore behavior.
Test Coverage Expansion tests/test_hook_manager.cpp, tests/test_profiler.cpp, tests/test_scanner.cpp	Added concurrent shutdown serialization test for reader draining, multi-writer wrap-around profiler stress test with export validation, and cascade prologue-fallback ambiguity regression test to guard against arbitrary match selection.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant HM as HookManager
    participant Gate as m_mutator_gate
    participant HMutex as m_hooks_mutex
    participant OSL as OS / Loader Lock

    rect rgba(255, 200, 100, 0.5)
    Note over App,OSL: Normal Shutdown Path (No Loader Lock)
    App->>HM: ~HookManager() destructor
    HM->>HM: Check m_shutdown_called
    alt Already shutdown
        HM-->>App: Return early
    else Shutdown in progress
        HM->>Gate: Acquire exclusive lock
        HM->>HM: Set m_shutdown_called = true
        HM->>Gate: Release exclusive lock
        HM->>HMutex: Acquire shared lock
        Note over HM: Drain in-flight readers
        HM->>HMutex: Release shared lock
        HM->>HMutex: Acquire exclusive lock
        HM->>HM: Clear hook maps
        HM->>HMutex: Release exclusive lock
        HM-->>App: Return safely
    end
    end

    rect rgba(200, 150, 255, 0.5)
    Note over App,OSL: Fallback Path (Loader Lock Detected)
    App->>HM: ~HookManager() during DLL unload
    HM->>OSL: Detect loader lock engaged
    HM->>HM: Pin current module
    HM->>HM: Set m_shutdown_called
    HM->>HM: Leak hook/VMT maps to static vector
    HM-->>App: Return without cleanup (deadlock avoidance)
    Note over OSL: OS releases leaked memory at process exit
    end

Loading

sequenceDiagram
    participant User as User Code
    participant SP as ScopedProfile
    participant Prof as Profiler
    participant Slot as Ring-Buffer Slot

    rect rgba(100, 200, 100, 0.5)
    Note over User,Slot: Old Protocol (Odd/Even with Loaded Seq)
    User->>SP: Create ScopedProfile("func")
    SP->>Prof: record_open(name)
    Prof->>Slot: Load seq (relaxed)
    Prof->>Slot: Store seq | 1 (odd)
    Note over Prof,Slot: Thread race: another producer fetches new seq
    User->>User: Do work
    User->>SP: ~ScopedProfile()
    SP->>Prof: record_close()
    Prof->>Slot: Load old seq (now stale!)
    Prof->>Slot: Store seq | 0 (even)
    Note over Prof,Slot: Potential rollback if concurrent fetch_add occurred
    end

    rect rgba(100, 150, 255, 0.5)
    Note over User,Slot: New Protocol (Monotonic fetch_add)
    User->>SP: Create ScopedProfile("func")
    SP->>Prof: record_open(name)
    Prof->>Slot: fetch_add(1, acq_rel)
    Note over Prof,Slot: Unconditional atomic increment, no rollback possible
    User->>User: Do work
    User->>SP: ~ScopedProfile()
    SP->>Prof: record_close()
    Prof->>Slot: fetch_add(1, release)
    Note over Prof,Slot: Sequence always advances, races between producers safe
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

The PR contains intricate concurrency and lifetime-management changes (HookManager loader-lock fallback, Logger AsyncLogger retention vector, Profiler sequence protocol shift) that span multiple core subsystems, mixed with documentation and test additions requiring careful reasoning about thread-safety implications and shutdown semantics.

Possibly related PRs

• PR #56: Modifies the same Profiler and ScopedProfile constructor signature for string-literal enforcement, directly overlapping with this PR's profiler implementation changes.
• PR #52: Updates HookManager shutdown to a two-phase disable-then-clear sequence and modifies async-logger leak handling to avoid destruction races, sharing core shutdown logic patterns with this PR.
• PR #26: Originally added the thread-safety entries to AGENTS.md that this PR expands with revised shutdown and concurrency semantics.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: harden profiler, hook manager, and logger' directly corresponds to the primary changes documented in the PR objectives, which focus on hardening these three components to address audit findings and prevent races/UB.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

tests/test_scanner.cpp (1)

2014-2069: Test correctly validates the uniqueness-ceiling guard.

Seeding five identical trampoline-shaped sequences (E9 + disp32 + 8-byte literal tail) into a PAGE_EXECUTE_READ buffer and asserting ResolveError::NoMatch precisely exercises the hits > kPrologueFallbackMaxHits branch in scan_candidates_hooked_prologue. The tail bytes (A5 B6 C7 D8 E9 FA 0B 1C) are distinctive enough to avoid cross-test aliasing.

Minor nit (optional): this test rolls its own VirtualAlloc/VirtualFree instead of reusing the ExecBuffer RAII helper a few tests above. If the intermediate ASSERT_TRUE(VirtualProtect(...)) at line 2046 fires, raw leaks until process exit. Not a correctness bug, just an RAII inconsistency with the surrounding tests; an ExecBuffer-style guard (or a small scope-exit) would make the cleanup path uniform.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_scanner.cpp` around lines 2014 - 2069, The test
ScannerCascade::PrologueFallbackRejectsAmbiguousTail uses manual
VirtualAlloc/VirtualFree/VirtualProtect and can leak 'raw' if VirtualProtect
ASSERT_TRUE fails; replace the manual allocation with the existing ExecBuffer
RAII helper (or wrap raw in a scope-exit guard) so allocation and protection
changes are always undone on failure, i.e., remove direct calls to
VirtualAlloc/VirtualFree/VirtualProtect and use ExecBuffer (or a small
RAII/protector) to allocate, set PAGE_EXECUTE_READ, and restore/free in its
destructor to ensure deterministic cleanup.

tests/test_hook_manager.cpp (1)

2436-2446: Optional: assert the ordering invariant explicitly.

The comment states "it must not return while the reader holds shared_lock", but the test never checks that before flipping reader_may_return. Adding a pre-release assertion on shutdown_returned makes the invariant a hard test failure rather than relying solely on the reader_observed_valid indirect check. Purely optional.

♻️ Proposed strengthener

     std::this_thread::sleep_for(std::chrono::milliseconds(20));
+    // Shutdown must still be blocked because the reader holds shared_lock.
+    EXPECT_FALSE(shutdown_returned.load(std::memory_order_acquire));
     reader_may_return.store(true, std::memory_order_release);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_hook_manager.cpp` around lines 2436 - 2446, Add an explicit
pre-release assertion to enforce the ordering invariant: before calling
reader_may_return.store(true, std::memory_order_release) in the test, assert
that shutdown_returned.load() is false (e.g.,
EXPECT_FALSE(shutdown_returned.load())); this makes the requirement "must not
return while the reader holds shared_lock" a hard check instead of relying
solely on reader_observed_valid, referencing the existing test variables
reader_may_return and shutdown_returned.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/logger.cpp`:
- Around line 211-216: The branch that only retains the detached AsyncLogger
when local_logger.use_count() == 1 is unsafe; instead, whenever
detail::is_loader_lock_held() is true and local_logger is non-null, move
local_logger into the static s_leaked_loggers to keep the object alive
regardless of use_count(). Update the block around local_logger and
detail::is_loader_lock_held() so it no longer checks use_count() before
emplacing std::move(local_logger) into s_leaked_loggers, ensuring
AsyncLogger::shutdown()'s detached writer thread cannot be destroyed while other
threads still hold temporaries.

In `@tests/test_profiler.cpp`:
- Around line 501-569: The test currently only validates JSON envelope from
export_chrome_json() and therefore cannot detect per-slot sequence rollbacks;
add a test-only way to observe ProfileSample::sequence (or equivalent slot
sequence counters) and assert each slot's sequence is strictly non-decreasing
across reads. Modify Profiler to expose a test-only accessor (e.g., a method
named snapshot_slot_sequences() or get_slot_sequence(size_t) guarded by `#ifdef`
TESTING or an `#if` defined unit-test build flag, or mark the test as a friend)
that atomically reads each slot's sequence value from the ring buffer, then
update ConcurrentRecord_WrapsBuffer_ReaderObservesNoSequenceRollback to
repeatedly call that accessor and verify for each slot that successive reads do
not decrease (store any violation into rollback_detected); keep existing writers
and stop logic but replace the JSON check with these direct sequence snapshots
so the test will fail if any slot counter rolls back.
- Around line 483-493: The header comment in profiler.hpp incorrectly claims
const char (&)[N] enforces string-literal/static-storage semantics; either
strengthen the API or correct the docs—do not change the tests. Update
profiler.hpp's documentation for ScopedProfile to state that the array-reference
constructor accepts any array (including local automatic arrays) and therefore
may produce a dangling pointer if given a non-static array, and recommend safe
alternatives (e.g., pass a compile-time literal constant, use a static const
char array, or use an API that accepts std::string_view with documented lifetime
requirements or a factory that copies the string). Reference the ScopedProfile
constructor and its array-reference overload in the comment so callers see the
exact symbol whose semantics were clarified.

---

Nitpick comments:
In `@tests/test_hook_manager.cpp`:
- Around line 2436-2446: Add an explicit pre-release assertion to enforce the
ordering invariant: before calling reader_may_return.store(true,
std::memory_order_release) in the test, assert that shutdown_returned.load() is
false (e.g., EXPECT_FALSE(shutdown_returned.load())); this makes the requirement
"must not return while the reader holds shared_lock" a hard check instead of
relying solely on reader_observed_valid, referencing the existing test variables
reader_may_return and shutdown_returned.

In `@tests/test_scanner.cpp`:
- Around line 2014-2069: The test
ScannerCascade::PrologueFallbackRejectsAmbiguousTail uses manual
VirtualAlloc/VirtualFree/VirtualProtect and can leak 'raw' if VirtualProtect
ASSERT_TRUE fails; replace the manual allocation with the existing ExecBuffer
RAII helper (or wrap raw in a scope-exit guard) so allocation and protection
changes are always undone on failure, i.e., remove direct calls to
VirtualAlloc/VirtualFree/VirtualProtect and use ExecBuffer (or a small
RAII/protector) to allocate, set PAGE_EXECUTE_READ, and restore/free in its
destructor to ensure deterministic cleanup.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d307992c-1f84-4797-9a28-2a8d23791aa9

📥 Commits

Reviewing files that changed from the base of the PR and between f0cc945 and db61a4a.

📒 Files selected for processing (15)

• AGENTS.md
• CMakeLists.txt
• README.md
• include/DetourModKit.hpp
• include/DetourModKit/async_logger.hpp
• include/DetourModKit/config.hpp
• include/DetourModKit/profiler.hpp
• src/async_logger.cpp
• src/hook_manager.cpp
• src/input.cpp
• src/logger.cpp
• src/profiler.cpp
• tests/test_hook_manager.cpp
• tests/test_profiler.cpp
• tests/test_scanner.cpp