Skip to content

fix(hotkey): MacHotkeyAdapter::shutdown stops CFRunLoop + tap [严重]#388

Merged
H-Chris233 merged 1 commit into
betafrom
fix/audit-mac-hotkey-shutdown
May 9, 2026
Merged

fix(hotkey): MacHotkeyAdapter::shutdown stops CFRunLoop + tap [严重]#388
H-Chris233 merged 1 commit into
betafrom
fix/audit-mac-hotkey-shutdown

Conversation

@appergb
Copy link
Copy Markdown
Collaborator

@appergb appergb commented May 9, 2026

User description

Summary

`MacHotkeyAdapter` did not override `HotkeyAdapter::shutdown` — it inherited the empty default. The mac listener's `run_listen_loop` calls `CFRunLoopRun()` which never returns absent `CFRunLoopStop`. So every `HotkeyMonitor::drop` (which fires on every preference-driven monitor swap, e.g. user changes hotkey binding) leaked the listener thread + the underlying CGEventTap. Long-running sessions that cycle bindings would steadily accumulate background threads.

The Windows adapter already does this correctly via `PostThreadMessageW(WM_QUIT)`; this PR mirrors that pattern on macOS.

Change

  • Add `CFRunLoopStop` FFI extern.
  • New `MacShutdownHandles { tap, runloop }` shared via `Arc` between the listener thread (writes refs after `CGEventTapCreate` + `CFRunLoopGetCurrent`) and `MacHotkeyAdapter` (reads them in `shutdown`).
  • `MacHotkeyAdapter::shutdown` — disables the tap first (so OS stops dispatching), then `CFRunLoopStop`. `take()` makes shutdown idempotent.
  • `run_listen_loop` now does `Box::from_raw(context)` after `CFRunLoopRun` returns, so the listener thread releases the callback context before exiting.
  • The TAP_DISABLED_BY_TIMEOUT re-enable path in `tap_callback` now reads through `ctx.handles.tap` (same lock that adapter shutdown takes from). Consistent ownership story.

Safety / threading

`CGEventTapEnable` and `CFRunLoopStop` are documented by Apple as safe to call from any thread. Captured as a `SAFETY` comment on the `unsafe Send/Sync impl for MacShutdownHandles`. Mutex held only across the FFI call (already a no-syscall fast path), so no nested-lock hazards.

Audit linkage

Audit ID 3.1.1 (CONFIRMED 严重). See `docs/audit-2026-05-10-validated.md` (local) for full validation context. 3.1.3 (JoinHandle storage) deferred to a separate supervisor PR — that fix is meaningless without a supervisor consuming the health signal.

Test plan

  • `cargo test --lib` — all 31 hotkey/coordinator/types/commands tests pass.
  • Test fixture `callback_context` updated to construct `MacShutdownHandles` (was constructing `tap` field directly on `CallbackContext`).
  • `cargo check --lib` clean (warnings unrelated, pre-existing).
  • CI build (mac + windows targets) — to be verified.
  • Manual functional verification on macOS: swap hotkey binding 5+ times in Settings, watch `ps -M` / Activity Monitor for openless threads — count should stay flat (was: leaks one thread + one CGEventTap per swap). To be done after merge.

PR Type

Bug fix


Description

  • MacHotkeyAdapter::shutdown stops CFRunLoop and tap

  • Shares handles via Arc<MacShutdownHandles>

  • Idempotent teardown using take() on lock

  • Fixes thread/tap leak on monitor drop


Diagram Walkthrough

flowchart LR
    listener["listener thread: CFRunLoopRun"]
    handles["Arc<MacShutdownHandles>"]
    adapter["MacHotkeyAdapter::shutdown"]
    disable["CGEventTapEnable(tap, false)"]
    stop["CFRunLoopStop(runloop)"]
    exit["cleanup context"]
    listener -- "stores tap/runloop" --> handles
    handles -- "shared with" --> adapter
    adapter -- "1. disable tap" --> disable
    adapter -- "2. stop runloop" --> stop
    stop -- "unblocks" --> listener
    listener -- "exits" --> exit
Loading

File Walkthrough

Relevant files
Bug fix
hotkey.rs
Implement MacHotkeyAdapter::shutdown for proper cleanup   

openless-all/app/src-tauri/src/hotkey.rs

  • Add MacShutdownHandles with tap/runloop refs and Send/Sync impl
  • Implement MacHotkeyAdapter::shutdown to disable tap and stop runloop
  • Update run_listen_loop to share handles with adapter via StartupTx
  • Add CFRunLoopStop FFI and adjust callback context to use handles
+57/-8   

CFRunLoopRun() in run_listen_loop has no return path; when
HotkeyMonitor::drop runs adapter.shutdown(), the macOS branch fell
through to the empty default impl from the trait. So every preference-
driven monitor swap (e.g. user changes hotkey trigger) leaked the
listener thread + CGEventTap. Long-running sessions that cycle bindings
would steadily accumulate background threads.

Fix:
- Add CFRunLoopStop FFI extern.
- Add MacShutdownHandles { tap, runloop } shared via Arc between the
  listener thread (writes refs in run_listen_loop after CGEventTapCreate
  and CFRunLoopGetCurrent) and MacHotkeyAdapter (reads them in
  shutdown).
- Implement MacHotkeyAdapter::shutdown — disable tap then stop runloop,
  in that order, so the OS stops dispatching before we tear down.
  take()s on the option locks make shutdown idempotent.
- run_listen_loop now does Box::from_raw(context) after CFRunLoopRun
  returns so the listener thread releases the callback context before
  exiting.
- Move tap re-enable in tap_callback (TAP_DISABLED_BY_TIMEOUT) to read
  through ctx.handles.tap (same lock the adapter releases on shutdown,
  consistent with the rest of the FFI thread-safety story).

Mirrors the Windows adapter's PostThreadMessageW(WM_QUIT) shutdown
pattern. Apple documents CGEventTapEnable + CFRunLoopStop as safe to
call from any thread, captured in a SAFETY comment on the
unsafe Send/Sync impl.

Audit ID 3.1.1 (CONFIRMED 严重).

Test: hotkey + coordinator + types + commands lib tests pass (31/31).
Functional verification of the leak fix requires running the app and
swapping bindings — to be done after PR ships.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

@H-Chris233 H-Chris233 merged commit 9414c30 into beta May 9, 2026
4 checks passed
pull Bot pushed a commit to yimmy23/openless that referenced this pull request May 10, 2026
10 PRs landed on beta this cycle:
- Open-Less#377 paste shortcut configurable (issue Open-Less#360)
- Open-Less#386 TS UserPreferences updateChannel alignment
- Open-Less#387 focus_target leak on Processing-phase cancel
- Open-Less#388 [严重] MacHotkeyAdapter::shutdown stops CFRunLoop + tap
- Open-Less#389 emit_capsule window.show/hide off audio thread
- Open-Less#390 QA / dictation hotkey routing race
- Open-Less#391 audio-mute spawn_blocking (async hygiene)
- Open-Less#392 hotkey supervisor + global dispatcher exit signal
- Open-Less#393 post-audit logic-review hotfixes (QA mute .await + focus_target Processing branch)
- Open-Less#394 in-process credentials cache (kills repeated Keychain prompts)

Bump 4 files: package.json, tauri.conf.json, Cargo.toml, Cargo.lock.
@appergb appergb deleted the fix/audit-mac-hotkey-shutdown branch May 10, 2026 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants