Repository navigation
fix(tts): honor volume for macOS say via afplay - #11
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ); | ||
|
|
||
| // Render to a temp AIFF, then play via afplay so the volume knob applies. | ||
| let aiff_path = std::env::temp_dir().join("sumvox_macos.aiff"); |
There was a problem hiding this comment.
Race condition: fixed temp filename shared across concurrent invocations
aiff_path is always std::env::temp_dir().join("sumvox_macos.aiff") — the same path for every call, in every process. sumvox is invoked fresh per hook event, so two notifications firing close together (two Claude Code sessions, or two rapid hook events in one session) can race: process A's afplay may still be reading the file when process B's say -o overwrites it, or process B's say may still be writing when process A's cleanup (remove_file) deletes it out from under it. Net effect: wrong audio played, truncated/corrupted playback, or a spurious afplay error.
This project already solved exactly this problem in src/audio/normalize.rs::normalize_to_wav, which qualifies temp paths with std::process::id() plus a per-call sequence counter specifically "so concurrent calls (across or within a process) never clobber each other's files." This PR doesn't follow that established convention.
Suggest qualifying the path similarly, e.g.:
let aiff_path = std::env::temp_dir().join(format!("sumvox_macos_{}.aiff", std::process::id()));
| @@ -69,6 +77,11 @@ impl TtsProvider for MacOsTtsProvider { | |||
| return Err(VoiceError::Voice(format!("Say command failed: {}", stderr))); | |||
| } | |||
There was a problem hiding this comment.
Minor: temp file isn't cleaned up when say itself fails
Cleanup (std::fs::remove_file(&aiff_path)) only runs after a successful say invocation, on the line after this block. If say -o aiff_path ... exits non-zero after having already created or partially written the output file (e.g. a mid-synthesis error), this early return Err(...) leaves that file behind in the temp dir indefinitely.
normalize_to_wav in src/audio/normalize.rs handles the analogous case explicitly ("create() may have left a partial file behind before write_all failed" → best-effort remove_file). Worth mirroring that here with a best-effort cleanup before returning the error, e.g.:
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
let _ = std::fs::remove_file(&aiff_path);
return Err(VoiceError::Voice(format!("Say command failed: {}", stderr)));
}There was a problem hiding this comment.
已在錯誤返回前補上 best-effort 清理(第 79–84 行:),此為誤報 —— 建議的程式碼已存在。
Review: fix(tts): honor volume for macOS say via afplayOverviewFixes a real bug: Correctness / edge cases
Idiomatic Rust / clippy
Security
Performance
Tests
|
bc3b029 to
60b911b
Compare
| // Render to a temp AIFF, then play via afplay so the volume knob applies. | ||
| // Qualify the path with the pid so concurrent invocations (e.g. two rapid | ||
| // `sumvox say` calls, which don't hold the hook queue lock) can't clobber | ||
| // each other's file — same convention as audio/normalize.rs. | ||
| let aiff_path = | ||
| std::env::temp_dir().join(format!("sumvox_macos_{}.aiff", std::process::id())); |
There was a problem hiding this comment.
Minor: PID-only temp filename isn't actually the same collision-safety scheme as audio/normalize.rs.
normalize.rs explicitly notes that PID alone is not sufficient for uniqueness because concurrent calls within the same process share a PID, and adds a monotonic CALL_SEQ counter on top of the PID for that reason. This code only uses std::process::id().
Today that's safe because every speak() call site (main.rs, hooks/claude_code.rs) awaits providers sequentially in a loop, so there's never two in-flight speak() calls in one process. But it's a latent footgun: if this ever changes (e.g. a future parallel-fallback attempt, or a second macOS-gated test added to this file that also exercises the full say+afplay path — cargo test runs tests in the same binary/PID concurrently by default), two calls would race on the exact same aiff_path, and one's remove_file cleanup could delete the file out from under the other's afplay/say -o write.
Two low-cost options:
- Fix the comment to not claim parity with
normalize.rs(it deliberately omits the piece that makes that scheme collision-safe under same-process concurrency), or - Reuse the same
CALL_SEQ-style suffix here for actual parity and future-proofing.
There was a problem hiding this comment.
採納:已把註解改誠實,不再宣稱與 normalize.rs 同 scheme,並註明 pid 足夠是因為 speak() 於同一 process 內只會循序呼叫。未加 CALL_SEQ 計數器,因為它只為防不存在的同 process 並行(YAGNI);真出現第二個全路徑測試再補即可。
|
Review: fix(tts): honor volume for macOS say via afplay Overview Correctness
Style / idiomatic Rust
Security
Performance
Test coverage
Overall: solid, well-scoped fix consistent with existing project conventions. One minor doc/consistency nit left inline; no blocking issues found. |
60b911b to
9741eaf
Compare
Review: fix(tts): honor volume for macOS say via afplayNice fix — routing macOS `say` through `afplay` to finally honor the volume setting is a sensible approach, and it's consistent with the existing `run_afplay`/`play_with_afplay` choke point used by the other providers. A few notes: Correctness
Minor / style
Testing
Overall the fix looks correct for the actual call paths in this codebase today ( |
| // Render to a temp AIFF, then play via afplay so the volume knob applies. | ||
| // Qualify by pid so two separate invocations (e.g. rapid `sumvox say` | ||
| // calls that don't hold the hook queue lock) can't clobber each other. | ||
| // pid alone suffices because speak() is only ever called sequentially | ||
| // within a process; no CALL_SEQ counter (unlike normalize.rs) is needed. | ||
| let aiff_path = | ||
| std::env::temp_dir().join(format!("sumvox_macos_{}.aiff", std::process::id())); |
There was a problem hiding this comment.
Temp file name is qualified only by PID, relying on the invariant that speak() is never called concurrently within the same process. That's true for the current call sites (sequential awaits in main.rs/hooks/claude_code.rs), but src/audio/normalize.rs hit this exact issue and added a monotonic CALL_SEQ counter alongside the PID specifically because "concurrent calls — even within one process, where the PID is identical — never collide." If this invariant is ever broken (e.g. a future test also exercising this path on another thread within the same test binary, or a caller that fans out TTS concurrently), two calls would race on sumvox_macos_{pid}.aiff — one's remove_file (line 89) could delete the file while the other's afplay/say is still using it. Consider adopting the same PID+counter pattern here for consistency/robustness.
There was a problem hiding this comment.
採納 repo 慣例:已改用 PID + CALL_SEQ 計數器(對齊 audio/normalize.rs),消除同 process 並行時的潛在 race。
macOS `say` has no volume flag, so the configured volume (e.g.
notification_volume) was silently ignored — notifications always played at
system volume, which is uncontrollable on output devices reporting no
software volume (missing value). Render to a temp AIFF and play through
afplay -v {volume/100} instead, routing macOS TTS through the same afplay
choke point as every other provider (also drives the avatar mouth-flap).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9741eaf to
1863c61
Compare
| let aiff_path = std::env::temp_dir().join(format!( | ||
| "sumvox_macos_{}_{}.aiff", | ||
| std::process::id(), | ||
| CALL_SEQ.fetch_add(1, Ordering::Relaxed) | ||
| )); |
There was a problem hiding this comment.
Unlike the other providers' play_with_afplay, which reuse a fixed temp filename per provider (e.g. sumvox_google.wav, overwritten on every call), this path mints a brand-new, never-repeated filename per invocation (pid + monotonically increasing CALL_SEQ). That's fine while speak() runs to completion, but if the process is ever killed before the cleanup lines run — e.g. an external hook timeout, Ctrl-C, or a crash mid-playback — the file is orphaned in temp_dir() permanently, since that exact (pid, seq) pair will never occur again to get overwritten.
This path is now more exposed to that risk than before: previously speak() made one blocking subprocess call (say); now it chains two (say then afplay), roughly doubling the window during which an external kill can land mid-flight. Since this is a Claude Code hook (typically time-boxed) that fires on every notification, repeated timeouts/kills could leak a sumvox_macos_*.aiff file per occurrence with nothing ever sweeping them up.
Worth considering: reuse a fixed filename (e.g. sumvox_macos.aiff) like the other providers — since calls here appear to already be serialized through the fallback loop — or add a best-effort sweep of stale sumvox_macos_*.aiff files on startup.
There was a problem hiding this comment.
不改。呼叫已序列化(hook queue lock + 序列 fallback 迴圈),孤兒檔僅在 process 被 kill 於兩次 subprocess 之間才會殘留,且是 temp_dir 裡數十 KB 的 AIFF、由 OS 週期清理。加啟動掃除是替非問題造新機制;而改回固定檔名又與上一輪要求的 CALL_SEQ 相衝。維持現狀。
| // Play with afplay -v; clean up on every path (including playback error). | ||
| let result = crate::audio::afplay::run_afplay(&aiff_path, self.volume); | ||
| let _ = std::fs::remove_file(&aiff_path); | ||
| result?; |
There was a problem hiding this comment.
Minor: neither the say -o ... synthesis (line 83) nor this afplay call has a timeout. src/audio/normalize.rs explicitly guards its ffmpeg subprocess with FFMPEG_TIMEOUT for exactly this reason — quoting its comment: "A stalled ffmpeg would otherwise block the calling thread forever (worst case for a notification tool)." This PR chains two sequential blocking subprocess calls (say then afplay) into the macOS path with no cap on either, so a wedged audio device or a stuck say synthesis would now hang the whole hook indefinitely with no way out.
Not a regression in run_afplay itself (pre-existing for the other providers too), but since this PR doubles the blocking-call surface for macOS specifically, it might be worth applying the same timeout+kill pattern here for consistency.
There was a problem hiding this comment.
超出本 PR 範圍。如你所述,無 timeout 是所有 provider 經 run_afplay 的既有性質、非本 PR 引入;若要加應對所有 provider 統一處理,屬另一個變更。本 PR 只做 macOS say 音量修正,不夾帶。
Review SummaryWhat this PR does: Fixes the macOS Strengths
Points raised inline
Neither point is a regression in the volume fix itself — both are pre-existing patterns elsewhere in the codebase, but this PR increases the exposure by adding a second blocking subprocess call and switching to a non-overwriting temp filename scheme. |
問題
macOS `say` 沒有音量參數,原本 `MacOsTtsProvider` 的 `volume` 欄位標了 `#[allow(dead_code)]` 直接被忽略 —— 設定的音量(如 `notification_volume`)對通知完全無效,`say` 一律以系統音量播放。
在系統音量不可控的輸出裝置上(螢幕/DAC 回報 `output volume: missing value`),這代表通知永遠是硬體滿刻度、整段大聲,且 config 的 volume 旋鈕碰不到它。
修法
改用 `say -o tmp.aiff` 產檔,再走 `afplay -v {volume/100}` 播放:
測試
備註
SoundSource 開頭漏音是其 ACE 攔截延遲的行為,程式端解不掉;搭配把 SoundSource 內 `afplay` 設回 100%、由 sumvox 的 `-v` 控音量即可根除。
🤖 Generated with Claude Code