Skip to content

fix(coordinator): wrap audio-mute shellouts in spawn_blocking [中]#391

Merged
appergb merged 2 commits into
betafrom
fix/audit-async-hygiene
May 10, 2026
Merged

fix(coordinator): wrap audio-mute shellouts in spawn_blocking [中]#391
appergb merged 2 commits into
betafrom
fix/audit-async-hygiene

Conversation

@appergb
Copy link
Copy Markdown
Collaborator

@appergb appergb commented May 9, 2026

User description

Summary

`AudioMuteGuard::activate()` shells out to `osascript` on macOS (~100–300 ms AppleScript runtime startup) and to `wpctl`/`pactl` on Linux (similar). Both go through synchronous `std::process::Command`. `acquire_recording_mute` was called inline from the async `begin_session` path, so the tokio worker thread was blocked on the shellout for every dictation start. Net effect: every macOS dictation absorbed ~150 ms of osascript startup before `Recorder::start` could run.

Change

  • `acquire_recording_mute` → async. Existing Mutex/holders/activate body wrapped in `tokio::task::spawn_blocking`. The `parking_lot` Mutex never crosses an await — it lives entirely inside the blocking task.
  • `release_recording_mute` stays a sync entry point because some call sites are on plain `std::thread` workers (the recorder error monitor uses `std::thread::Builder::new().spawn`, not tokio). It now delegates to `spawn_blocking` when a `tokio::runtime::Handle::try_current` is available, and falls back to running synchronously when not — the fallback case is already on a non-async OS thread, so blocking there blocks nothing.
  • `start_recorder_for_starting` → async, to thread `acquire_recording_mute().await` up. Three call sites (Bailian, generic Volcengine, the `start_recorder_and_enter_listening` helper) get matching `.await?`.

Why one PR

Originally scoped as "async hygiene" covering 3.2.3 (sync inserter), 3.2.4 (audio-mute), and 3.2.5 (Linux probe sleep). Trimmed to just 3.2.4 because:

  • 3.2.3 (`inserter.insert`) is mostly Linux/Windows perf — macOS path uses `CGEventPost`, fast in practice. Touches different files. Defer.
  • 3.2.5 is Linux-only 120 ms perf. Defer.
  • 3.2.4 is the user-visible one (every macOS dictation feels ~150 ms snappier).

Audit linkage

Audit ID 3.2.4 (CONFIRMED 中). 3.2.3 + 3.2.5 tracked separately. See `docs/audit-2026-05-10-validated.md` (local).

Test plan

  • `cargo test --lib` — 183/183 pass.
  • `cargo check --lib` clean.
  • CI build — to be verified.
  • Manual measurement: dictate on macOS with `muteDuringRecording: true`. Before fix: log shows ~150 ms gap between `hotkey pressed` and `Recorder::start` Ok. After: gap should drop to <20 ms (osascript runs in parallel on the blocking pool). To be done after merge.

PR Type

Bug fix, Enhancement


Description

  • Wrap audio-mute shell-outs in spawn_blocking to avoid blocking tokio worker threads.

  • Convert start_recorder_for_starting to async to integrate with new async mute.

  • Update three call sites in begin_session with .await? for async recorder start.

  • Adapt release_recording_mute to offload mute restoration when a tokio handle is present.


Diagram Walkthrough

flowchart LR
    A["begin_session (async)"] -- ".await" --> B["start_recorder_for_starting (async)"]
    B -- ".await" --> C["acquire_recording_mute (async)"]
    C -- "spawn_blocking" --> D["AudioMuteGuard::activate() (osascript / wpctl)"]
    E["release_recording_mute (sync)"] -- "if tokio handle" --> F["spawn_blocking (drop guard)"]
    E -- "no handle" --> G["synchronous drop"]
Loading

File Walkthrough

Relevant files
Enhancement
dictation.rs
Asyncify dictation recording start to integrate non-blocking mute

openless-all/app/src-tauri/src/coordinator/dictation.rs

  • Make start_recorder_for_starting an async function.
  • Await acquire_recording_mute inside start_recorder_for_starting.
  • Add .await? to start_recorder_for_starting calls in begin_session for
    Bailian and Volcengine paths, and in
    start_recorder_and_enter_listening.
  • No additional logic changes.
+5/-5     
resources.rs
Offload audio mute shell-outs to blocking thread pool       

openless-all/app/src-tauri/src/coordinator/resources.rs

  • acquire_recording_mute moved to async and wraps
    AudioMuteGuard::activate() in tokio::task::spawn_blocking.
  • Explicit error logging added for spawn_blocking panic (instead of
    silently dropping).
  • release_recording_mute remains sync but delegates to spawn_blocking if
    a tokio runtime handle is available; otherwise runs inline for
    non‑tokio threads.
  • Extensive doc comments added explaining the reasoning and thread
    safety.
+70/-26 

AudioMuteGuard::activate() shells out to osascript on macOS (~100–300 ms
AppleScript runtime startup) and to wpctl/pactl on Linux (similar cost).
Both implementations execute synchronously through std::process::Command.
acquire_recording_mute and release_recording_mute were called from the
async begin_session path, so the tokio worker thread was blocked for the
full shellout duration on every dictation start and stop.

Fix:
- Make acquire_recording_mute async; wrap the existing
  Mutex/holders/activate body in tokio::task::spawn_blocking. The
  parking_lot Mutex never crosses an await — it lives entirely inside
  the blocking task. Caller (start_recorder_for_starting) awaits.
- release_recording_mute stays a sync entry point (some call sites are
  on plain std::thread workers, e.g. the recorder error monitor) but
  delegates to spawn_blocking when a tokio Handle::try_current is
  available. Falls back to running synchronously when there's no tokio
  context — that case is already on a non-async OS thread, so blocking
  there blocks nothing.
- start_recorder_for_starting becomes async to thread the
  acquire_recording_mute().await up. Three call sites (Bailian, generic
  Volcengine, the start_recorder_and_enter_listening helper) get
  matching .await? sequels.

Net effect: dictation-start latency on macOS no longer absorbs the
~100–300 ms osascript startup before Recorder::start can run; the work
runs on a tokio blocking pool thread instead. Same on Linux. Windows
path uses native COM (IAudioEndpointVolume::SetMute), already fast —
spawn_blocking is a small extra hop but harmless.

Audit ID 3.2.4 (CONFIRMED 中). Audit IDs 3.2.3 (sync inserter.insert)
and 3.2.5 (Linux-only 120 ms thread::sleep in permission probe) deferred
to follow-up — both are smaller-impact perf concerns and don't share
code with the audio-mute path.

Test: 183/183 lib tests pass. Manual verification (start dictation,
measure time-to-first-mic-capture before/after) requires the running
app and is expected to drop ~150 ms on macOS.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

PR Reviewer Guide 🔍

(Review updated until commit 1889b79)

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

@appergb
Copy link
Copy Markdown
Collaborator Author

appergb commented May 9, 2026

@claude opus 4.7 请审核这个 PR,确保它和前文的 PR 对应,且没有其他问题。

@claude
Copy link
Copy Markdown

claude Bot commented May 9, 2026

Claude finished @appergb's task in 2m 8s —— View job


PR #391 代码审核

  • 初始化任务列表
  • 读取变更文件 (dictation.rs, resources.rs)
  • 检查 git diff,与 PR 描述对比
  • 审查自动审核器标记的问题 (JoinHandle 错误处理)
  • 全面代码审核,检查其他潜在问题
  • 发布审核结果

总体评估

代码改动与 PR 描述完全对应,逻辑正确,Audit 3.2.4 的目标(解除 tokio worker 在 shellout 上的阻塞)已实现。以下是具体发现:


问题一:acquire_recording_mute 静默吞掉 task panic(中等)

resources.rs 第 127–144 行:

let _ = tokio::task::spawn_blocking(move || {
    // ...
})
.await;

let _ = 丢弃了 Result<(), JoinError>。当 blocking task 内部 panic 时(比如 AudioMuteGuard::activate 中发生不期而遇的 panic),JoinError 被静默丢弃,没有任何日志输出,用户音频未被静音,调用方却照常继续。

状态一致性分析:

  • Err 路径(activate() 返回 Err):已在闭包内 log::warn + 提前 returnholders 不增加 → 与修改前行为相同 ✓
  • Panic 路径:holders 也不会增加(panic 打断了 saturating_add),状态同样一致,但完全没有日志

建议改为(可观测,不影响控制流):

if let Err(e) = tokio::task::spawn_blocking(move || {
    // ...
})
.await
{
    log::warn!("[audio-mute] spawn_blocking task failed: {e:?}");
}

Fix this →


其余检查项(均无问题)

owner: &'static str 参数收窄spawn_blocking 要求闭包 'static,所有现有调用方均传字面量("dictation""qa"),编译正确,设计合理。

release_recording_mute 的 fire-and-forget — 文档注释已说明原因:调用方(cancel_sessionend_session、recorder error monitor)不需要等待静音恢复完成。并发正确性:parking_lot Mutex 保证了 acquire/release 的闭包互斥,无论调度顺序如何,holders 计数始终一致。

start_recorder_for_starting 三处调用方 — 已全部更新:

  • begin_session Bailian 分支(dictation.rs:228)→ .await?
  • begin_session Volcengine 分支(dictation.rs:327)→ .await?
  • start_recorder_and_enter_listening(dictation.rs:548)→ .await?
  • Windows/macOS Local 分支经过 start_recorder_and_enter_listening 间接覆盖 ✓

acquire/release 配对start_recorder_for_startingRecorder::start 失败时(dictation.rs:480)仍调用 release_recording_mute,配对正确。

Windows COM 路径spawn_blocking 对本已很快的 COM 调用增加了一次线程池跳转,代价极小,PR 描述中已说明"harmless",可接受。

与 PR 描述对应性 — 代码变更与 PR Summary 中的三条变更说明(acquire_recording_mute async、release_recording_mute 同步兜底、start_recorder_for_starting async 传递)完全吻合。Audit ID 3.2.3、3.2.5 明确标注为 defer,未出现在本次改动中。


结论

一个实质性建议(问题一,中等优先级):将 let _ = 改为 if let Err(e) = 以保留 panic 可观测性。其余逻辑无误,可以合并。

Addresses pr_agent "Possible Issue" review on PR #391. The original
`let _ = tokio::task::spawn_blocking(...).await` silently swallowed
JoinError, which means a panic inside AudioMuteGuard::activate() (or
the parking_lot lock) would leave the user with system audio leaking
during recording — and zero [audio-mute] log to explain why.

Match the JoinResult and log::error! the panic. State (holders/guard)
stays consistent under panic — activate panic means no guard stored
and holders not incremented; lock panic can't reach the mutate. The
fix is purely about observability.
@appergb
Copy link
Copy Markdown
Collaborator Author

appergb commented May 10, 2026

pr_agent "Possible Issue" 已修

1889b79:把 let _ = spawn_blocking(...).await 换成显式 match,在 JoinError 路径上 log::error! 记录 panic + owner + 「mute did not activate」语义。

State 一致性补充说明:原本 panic 路径下 holders/guard 也是一致的(activate 抛 → guard 没存 / holders 没增;lock 抛 → 进不到 mutate);这次修的是可观察性 —— 用户碰到「录音时系统声漏出」找不到任何 [audio-mute] 日志没法 debug 的问题。

@github-actions
Copy link
Copy Markdown

Persistent review updated to latest commit 1889b79

@appergb appergb merged commit 400097a into beta May 10, 2026
4 checks passed
@appergb appergb deleted the fix/audit-async-hygiene branch May 10, 2026 00:22
pull Bot pushed a commit to yimmy23/openless that referenced this pull request May 10, 2026
Two real bugs surfaced by the 2026-05-10 end-to-end logic review of
beta after the 8-PR audit-fix wave landed.

1. coordinator.rs:2313 — `acquire_recording_mute(inner, "qa");` was
   missing `.await`. PR Open-Less#391 (`6171df61`) made the function `async fn`
   and updated the dictation call site at coordinator/dictation.rs:451,
   but the QA call site was missed. Effect: when a user has
   `mute_during_recording = true` and triggers QA via Right Option, the
   returned Future is dropped on the next line, `spawn_blocking` is
   never scheduled, holders never increments, and system audio is NOT
   muted (e.g. YouTube playback continues into the QA recording). Both
   the matching `release_recording_mute(inner, "qa")` calls become
   no-ops (early return at holders == 0). The compiler was emitting
   `unused_must_use` for this site (verified before this commit).
   Fix: add `.await`.

2. coordinator/dictation.rs:843-849 — PR Open-Less#387 (`ce82fcd9`) was framed
   as "clear focus_target on cancel regardless of phase", but the only
   code path that gained the unconditional clear was
   `finish_cancel_session_state`. cancel_session deliberately skips
   that helper for Processing (so end_session can drive its own
   teardown), and end_session's "ASR-finished, cancelled" exit at
   dictation.rs:843-849 set phase=Idle but never touched focus_target.
   Result: cancel-during-Processing leaves a stale `usize` focus slot
   in state.focus_target until the next begin_session_state overwrites
   it. Today the leak is bounded (no documented reader between cancel
   and next begin), but it violates PR Open-Less#387's stated contract and is a
   silent footgun for any future reader of focus_target on that
   interval. Fix: in the cancelled-after-ASR exit branch, take the
   state lock once and clear both phase and focus_target together.

Source: docs/logic-review-2026-05-10.md (subagent end-to-end review).
185/185 lib tests pass; cargo check clean (unused_must_use gone).
Manual verification checklist for both fixes in the review doc.
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.
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.

1 participant