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

@appergbappergb commented May 9, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented May 9, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/appergb/openless/commit/1889b7912b9f2c57f4463102561373f27c9b5af9)

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
CollaboratorAuthor

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

@claude

claudeBot commented May 9, 2026

Copy link
Copy Markdown

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),状态同样一致,但完全没有日志

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

ifletErr(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
CollaboratorAuthor

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
Contributor

@appergb
appergb merged commit 400097a into betaMay 10, 2026
4 checks passed
@appergb
appergb deleted the fix/audit-async-hygiene branch May 10, 2026 00:22
pullBot 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.
pullBot 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 freeto 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

@appergb