fix: wake word mic-busy retry with actionable error messages - #74996
fix: wake word mic-busy retry with actionable error messages#74996tomekdot wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve wake-word startup reliability and UX by adding configurable retry/backoff behavior when the microphone is busy, and by replacing the previous opaque “already owned” errors with more actionable guidance. This fits into Hermes’s voice/wake-word subsystem (tools/wake_word.py) and its default configuration surface (hermes_cli/config_defaults.py).
Changes:
- Adds wake-word mic-busy retry configuration (
retry_on_busy,retry_interval,retry_max_attempts) and retry loop behavior instart_listening(). - Expands the cross-process mic lock error message to suggest likely culprits and a config-based workaround.
- Extends default config values for
wake_wordwith the new retry keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/wake_word.py | Adds retry config + retry loop and updates mic ownership error messaging. |
| hermes_cli/config_defaults.py | Adds default values for the new wake-word retry configuration keys. |
Comments suppressed due to low confidence (1)
tools/wake_word.py:1203
- Auto-retry doesn’t apply to the most common mic-busy case:
_acquire_machine_lock()raisesWakeWordInUsebeforelock_handleis assigned, and that exception currently bypasses the retry loop. As a result, cross-process ownership still fails immediately instead of backing off.
# No retry needed — proceed to acquire lock and build detector
# Acquire lock and build detector (inside the retry loop)
lock_handle = _acquire_machine_lock()
try:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Sleep outside the detector_lock so other threads can proceed | ||
| if retry_on and (_detector_owner is not owner): | ||
| if retry_max and attempt > retry_max: | ||
| raise WakeWordInUse( | ||
| "Wake-word microphone is already owned and retry limit " | ||
| f"({retry_max}) reached. Run: hermes config set " | ||
| "wake_word.enabled false" | ||
| ) | ||
| time.sleep(retry_interval) | ||
| continue |
| # Resolve retry config | ||
| _cfg = config if config is not None else load_wake_word_config() | ||
| retry_on = bool(_cfg.get("retry_on_busy", _RETRY_ON_BUSY_DEFAULT)) | ||
| retry_interval = max(1, int(_cfg.get("retry_interval", _RETRY_INTERVAL_SECONDS))) | ||
| retry_max = max(0, int(_cfg.get("retry_max_attempts", _RETRY_MAX_ATTEMPTS_DEFAULT))) |
434991d to
e78f0c2
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for improving a real wake-word startup failure path. The current implementation needs correction before the retry behavior can satisfy the PR's stated cross-process/device-busy goal.
Problems
tools/wake_word.py:1205calls_acquire_machine_lock()before thetrywhoseWakeWordInUsehandler starts at line 1220. A held machine lock therefore raises out immediately; it never retries. This confirms the existing review observation.WakeWordDetector.start()turns microphone-open failures intoRuntimeError(tools/wake_word.py:923-928), but the new retry branch catches onlyWakeWordInUse(tools/wake_word.py:1220). Audio-backend busy failures therefore still fail immediately.- The new retry test at
tests/tools/test_wake_word.py:648covers only the in-process_detectorpath. The pre-existing cross-process lock fixture at lines 603-621 is not used to validate retry.
Suggested changes
- Retry the machine-lock failure explicitly and add a real held-lock/release test.
- Define and test the retryable device-open error boundary separately from non-retryable startup errors.
- Reconcile waiting behavior with the sticky-ownership contract in
website/docs/user-guide/features/wake-word.md:150-154, and restore the test file's original line endings.
Automated hermes-sweeper review.
| raise WakeWordInUse( | ||
| "Wake-word microphone is already owned by another " | ||
| "surface in this process (e.g. desktop app + CLI)." | ||
| ) | ||
| lock_handle = _acquire_machine_lock() |
There was a problem hiding this comment.
_acquire_machine_lock() can itself raise WakeWordInUse, but it is evaluated before the try at line 1206. That bypasses the retry handler below, so the cross-process lock case still fails immediately. Move acquisition into retry handling and cover a held machine lock in a test.
| ) | ||
| _detector = detector | ||
| _detector_owner = owner | ||
| _detector_file_lock = lock_handle | ||
| detector.start() | ||
| return detector | ||
| except WakeWordInUse: |
There was a problem hiding this comment.
This catches only WakeWordInUse, whereas WakeWordDetector.start() wraps microphone-open failures as RuntimeError (tools/wake_word.py:923-928). The actual audio-device-busy path therefore is not retried; classify and retry only the relevant startup failure.
| assert isinstance(defaults, dict) | ||
|
|
||
|
|
||
| def test_start_listening_retries_when_detector_held(monkeypatch, tmp_path): |
There was a problem hiding this comment.
This covers only a pre-populated in-process _detector. Add a test using the existing cross-process machine-lock fixture so the retry path verifies the failure raised by _acquire_machine_lock().
|
Thanks @teknium1 for the detailed review. You're right on both points:
I'm not able to keep iterating on this PR right now, so I'll leave it as-is and let the maintainers pick it up — feel free to apply the fixes or close it if a better approach lands on main. Thanks again! |
|
Thanks for the detailed reviews — both points were real bugs. Pushed a fixup that addresses each: Copilot / hermes-sweeper #1 — cross-process lock never retried hermes-sweeper #2 — device-busy failures never retried hermes-sweeper #3 — test coverage + sticky-ownership contract
Verified locally: all 9 retry tests + the pre-existing I couldn't see any CI runs on this PR ( |
When the microphone is already held by another process or Hermes instance, wake word initialization previously failed immediately with the opaque message 'Wake-word microphone is already owned.' This commit: - Adds auto-retry with configurable interval and max attempts (wake_word.retry_on_busy / retry_interval / retry_max_attempts) - Provides actionable error messages that name likely culprits (Teams, Discord, Zoom, other Hermes instances) and suggest hermes config set wake_word.enabled false as a workaround - Logs retry attempts so users can see what is happening - Adds unit tests for retry behavior (held-by-other, max retries, retry disabled)
…h non-retryable boundary Address review feedback (Copilot + hermes-sweeper) on the mic-busy retry: - Move _acquire_machine_lock() inside the retryable try so the cross-process mic lease (the most common busy case) actually retries instead of failing immediately outside the loop. - Treat a transient device-busy mic-open failure as retryable via a new private _MicBusy error; hard failures (no device / missing deps) stay non-retryable RuntimeError so we never retry forever on a real error. - Add _is_mic_busy_error() to classify the retryable/non-retryable error boundary explicitly, as the review requested. - Clear half-built detector state on retry so the next loop re-acquires the lock rather than hitting the idempotent early-return. - Document the retry keys and reconcile with the sticky-ownership contract. - Add real cross-process lock-busy + device-busy retry tests.
94c86df to
f13b5f9
Compare
|
Note for maintainers: the PR UI shows
A force-push / re-review usually clears the cached state. If it still shows conflicting after a refresh, let me know and I'll dig further. |
Problem
When the microphone is already held by another process or Hermes instance, wake word initialization fails immediately with the opaque message:
This gives users no indication of what to do next.
Solution
This PR adds:
1. Auto-retry with configurable backoff
When the mic is busy, now retries every 10s (configurable) instead of failing immediately. This handles the common case where Desktop GUI holds the mic briefly at startup.
Config options in
wake_word:retry_on_busy(default:true)retry_interval(default:10seconds)retry_max_attempts(default:0= unlimited)2. Actionable error messages
When retry is exhausted, the error message now:
hermes config set wake_word.enabled falseas a workaroundTesting
Files changed
tools/wake_word.py— retry logic + better error messageshermes_cli/config_defaults.py— new retry config defaults