cwilliams5/review-criticals

Enter planning mode. Two-phase audit: first inventory every Critical hold in the codebase, then evaluate each for necessity and scope. Use maximum parallelism — spawn explore agents for independent file groups.

Why This Matters

Critical "On" prevents timer, hotkey, and callback interruption — it's the only synchronization primitive in AHK v2. But every Critical hold blocks the main thread. Keyboard hooks, timer callbacks, and WinEvent callbacks all queue up behind it. Critical sections accumulate over time as defenses against bugs, but the conditions that required them can change: a refactor moves shared state into a single producer, a Map iteration gets replaced with a snapshot copy, yet the Critical stays.

This is the mirror of review-race-conditions:

• review-race-conditions: "where should Critical exist but doesn't?" (additive)
• review-criticals: "where does Critical exist but shouldn't, or is wider than needed?" (subtractive)

Phase 1 — Inventory

Find every Critical "On" / Critical "Off" pair in src/core/ and src/gui/ (plus src/shared/ files they include). For each hold, document:

| Function | File:Lines | Protected Code Summary | Hold Duration | Shared State Touched | |----------|-----------|----------------------|---------------|---------------------| | _WEH_WinEventProc | winevent_hook.ahk:196+ | Check eligibility, upsert window, update MRU | Medium (~50-200μs) | gWS_* store globals |

Hold Duration estimates:

• Trivial (~1-5μs): Single assignment, counter increment, flag set
• Short (~5-50μs): Few property accesses, simple if/else, Map Has+Set
• Medium (~50-500μs): Loop over small collection, multiple DllCalls, string building
• Long (~500μs+): Loop over unbounded collection, file I/O, large data structure rebuild

Also note any Critical "On" without a matching Critical "Off" on every exit path (early returns, continues). These are bugs regardless of whether the Critical is necessary — but that's review-race-conditions territory. Just flag them as cross-references.

Phase 2 — Evaluate Each Hold

For each Critical section, answer one question: What specific race would occur if this Critical were removed?

This is not "is this a good idea" — it's "name the exact interleaving." What interrupt source fires, at what point in the protected code, modifying what shared state, causing what corruption?

Classification

Still necessary — A concrete race exists:

• Name the interrupt source (which timer, hotkey, or callback)
• Name the interleaving point (between which two operations)
• Name the corruption (stale read, lost write, partial state, collection modified during iteration)
• Verdict: keep as-is

Narrowable — The Critical protects more than it needs to:

• The hold spans N lines but only M lines (M < N) contain the actual shared state access
• Work before or after the critical shared state access could be moved outside
• Computations, local variable setup, string formatting, DllCalls that read (not write) local state — these don't need interrupt protection
• Verdict: shrink the scope, move non-critical work outside

Stale — The condition that required this Critical no longer exists:

• The protected variable is no longer shared (only one callback type accesses it now)
• The dangerous interleaving was eliminated by a refactor (e.g., Map iteration replaced with snapshot copy that happens outside Critical)
• The function is no longer called from an interruptible context
• Verdict: remove

Defensive but unverifiable — Can't name a specific race, but:

• The pattern looks like it could race in theory
• The code has a comment explaining the Critical but the explanation references old architecture
• Similar code elsewhere uses Critical for the same pattern
• Verdict: flag for human review with honest "I cannot name the specific race" assessment

Known Safe — Do NOT Flag

These Critical sections have been deliberately designed, tested, and documented. Do not propose removing, narrowing, or questioning them:

• Critical "On" through the entire GUI_OnInterceptorEvent handler including GDI+ rendering (~16ms). This was previously narrowed and reverted — releasing before render causes partial glass background, window mapping corruption, and stale projection data. See keyboard-hooks.md "CRITICAL: Critical Sections During Rendering (Context-Dependent)." No internal abort points — unsafe to interrupt. This is the most important Critical section in the codebase.
• Critical "On" in _INT_Alt_Down, _INT_Alt_Up, _INT_Tab_Down, _INT_Tab_Up, _INT_Tab_Decide, _INT_Ctrl_Down, _INT_Escape_Down — all hotkey callbacks require Critical to prevent callback-interrupting-callback corruption.
• Critical "On" in _Anim_FrameLoop — covers _Anim_UpdateTweens() → GUI_Repaint() → DComp sync. Phase 1 (#177) deliberately moved the long blocking wait (compositor clock / waitable swap chain) OUTSIDE Critical so the AHK message pump runs during the wait. The Critical section is only held during the render + present + commit sequence. This is the intended design.
• Critical "Off" before _GUI_ShowOverlayWithFrozen() in _GUI_GraceTimerFired — must release before the 1-2s first paint or Windows' LowLevelHooksTimeout (~300ms) silently removes hooks (#303). This is safe because the show path has 3 RACE FIX abort points that detect gGUI_State != "ACTIVE". This is the deliberate opposite of GUI_OnInterceptorEvent — context-dependent rule, see keyboard-hooks.md.
• Async activation event buffer (gGUI_EventBuffer) — Critical around buffer push/pop is necessary by design.
• Flight recorder FR_Record() — if it uses Critical, it's protecting the ring buffer write pointer. Pre-allocated, intentional.

If you encounter these, classify as "Still necessary — known safe, documented" and move on. Spend your tokens on the holds that aren't on this list.

Explore Strategy

Split by file group (run in parallel):

• Producer callbacks — src/core/winevent_hook.ahk, komorebi_sub.ahk, komorebi_state.ahk, komorebi_lite.ahk, mru_lite.ahk, winenum_lite.ahk
• GUI hot path — src/gui/gui_state.ahk, gui_interceptor.ahk, gui_input.ahk, gui_data.ahk, gui_paint.ahk
• GUI support — src/gui/gui_overlay.ahk, gui_pump.ahk, gui_workspace.ahk, gui_win.ahk, gui_monitor.ahk
• Shared infrastructure — src/shared/window_list.ahk, blacklist.ahk, ipc_pipe.ahk, stats.ahk

Use query_timers.ps1 to cross-reference — timer callbacks are the primary interrupt source for non-hotkey Critical sections. Use query_mutations.ps1 <globalName> to see which functions mutate a given global — reveals whether all writers are inside Critical sections without reading full files.

Validation

After explore agents report back, validate every finding yourself. Removing or narrowing a Critical section is high-risk — a wrong call creates a race condition that may only manifest under specific timing.

For each candidate:

1. Cite evidence: "I verified by reading file.ahk lines X–Y" with the protected code quoted. Show what's inside the Critical boundaries.
2. Name the race (or admit you can't): For "still necessary" — state the specific interleaving. For "stale" or "narrowable" — explain why the race no longer exists or why the moved code is safe outside Critical. For "defensive but unverifiable" — be honest that you can't name the race.
3. Check interrupt sources: Use query_timers.ps1 and grep for SetTimer / Hotkey( to identify what could actually interrupt this function. If nothing can interrupt it (function is only called from within an already-Critical context), the Critical may be redundant.
4. Check callers: A Critical section might look unnecessary in isolation but be required because callers invoke it from non-Critical contexts. Use query_function_visibility.ps1 to check all call sites.
5. Counter-argument: "What timing scenario would make removing/narrowing this Critical dangerous?" — Even if you can't name one, consider: could a future change re-introduce the race? Is the Critical cheap enough that the safety margin is worth it?

Plan Format

Section 1 — Inventory:

| # | Function | File:Lines | Hold Duration | Shared State | Classification | |---|----------|-----------|---------------|-------------|----------------| | 1 | _WEH_WinEventProc | winevent_hook.ahk:196+ | Medium | gWS_* | Still necessary | | 2 | KSub_CacheFocusedHwnds | komorebi_state.ahk | Long | gKS_Cache, gWS_* | Narrowable | | 3 | _OldHelper | some_file.ahk:30-35 | Trivial | gFoo (no longer shared) | Stale |

Total: N Critical sections. X still necessary, Y narrowable, Z stale, W unverifiable.

Section 2 — Narrowable holds (recommended changes):

| Function | File:Lines | Current Scope | Proposed Scope | Lines Moved Out | Est. Time Saved | Risk | |----------|-----------|--------------|---------------|----------------|----------------|------| | KSub_CacheFocusedHwnds | komorebi_state.ahk | 60 lines | 15 lines | JSON parse (before), log write (after) | ~100μs | Low — moved work is purely local |

Section 3 — Stale holds (recommended removal):

| Function | File:Lines | Why Stale | Evidence | |----------|-----------|----------|---------| | _OldHelper | some_file.ahk:30-35 | gFoo is only written by this function now (confirmed via query_global_ownership.ps1) | No other writers, no timer/hotkey accesses this path |

Section 4 — Defensive/unverifiable (human review needed):

| Function | File:Lines | What It Protects | Why Unverifiable | Recommendation | |----------|-----------|-----------------|-----------------|----------------| | _SomeFunc | file.ahk:90-95 | gBar increment | Only one known interrupt source, but pattern suggests historical race | Keep — trivial duration, safety margin worth it |

Order by hold duration within each section (longest first — the most blocking holds are the highest value targets for narrowing/removal).

Ignore any existing plans — create a fresh one.