Skip to content

fix: bound OVOSSkill._wait_response so get_response can't hang forever - #514

Merged
JarbasAl merged 1 commit into
devfrom
fix/wait-response-deadline
Aug 11, 2026
Merged

fix: bound OVOSSkill._wait_response so get_response can't hang forever#514
JarbasAl merged 1 commit into
devfrom
fix/wait-response-deadline

Conversation

@JarbasAl

Copy link
Copy Markdown
Member

🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting. Marked below which claims were checked against current source (this PR's own diff/tests) vs. cited from prior field evidence (re-checked by re-reading the linked issue, not re-run).

Bug

OVOSSkill._wait_response() (ovos_workshop/skills/ovos.py) busy-polls
while not ans: time.sleep(0.1) with no deadline while it waits for the
killable background thread (_real_wait_response) to populate
self.__validated_responses[session_id]. If that thread never finishes —
e.g. the bus/TTS handshake it waits on inside __get_response never
completes — the calling thread blocks forever instead of returning the
documented "no response" (None) outcome.

Field evidence (verified against this repo's current dev source, not
just cited): OpenVoiceOS/ovos-skill-alerts#138 "Update 3" captured a live
py-spy stack of exactly this call chain hanging:

handle_reschedule_alert (ask_yesno)
  -> ask_yesno (ovos_workshop/skills/ovos.py)
  -> get_response
  -> _wait_response -> while not ans: time.sleep(0.1)

hanging a CI job to its 30-minute job kill instead of exiting cleanly.

Fix

Add a deadline to the busy-wait, derived from the same budget the
background thread already spends per attempt rather than an arbitrary
constant:

  • skills.get_response_timeout (config, default 20s) — the bound
    __get_response itself already enforces per polling attempt.
  • + 15s — the wait_while_speaking() ceiling already used elsewhere in
    this same file (speak_dialog(..., wait=True)) for a reprompt.
  • multiplied by the number of retry attempts (num_retries + 1, or 2 when
    num_retries is -1, matching this method's own docstring: "if the user
    doesn't respond and this is -1 this will only retry once").

When the deadline is hit, ans is set to None, which is get_response's
existing documented "no response"/aborted outcome — no new return value,
no new bus message type. The normal path (background thread answers
quickly) and the abort path (__responses/__validated_responses set to
None externally, e.g. mycroft.skills.abort_question) are both
unchanged; only the previously-infinite stall case now returns within a
bounded time.

Red -> green proof

New regression test:
test/unittests/test_wait_response_deadline.py::TestWaitResponseDeadline::test_wait_response_returns_none_when_background_thread_never_answers

It patches _real_wait_response to simulate exactly the stuck-thread case
(initializes __validated_responses[session_id] = [], as the real thread
does on start, then never sets a final value — the thread just "hangs"),
with get_response_timeout patched down to 0.2s so the test runs fast.

  • Before the fix: hangs at ovos_workshop/skills/ovos.py:1902 inside
    the unbounded while not ans: time.sleep(0.1) loop; run with
    pytest --timeout=20 fails with Failed: Timeout (>15.0s) from pytest-timeout (the test's own @pytest.mark.timeout marker firing,
    proving the loop never exits on its own).
  • After the fix: passes in ~15.2s, asserting ans is None and
    elapsed < 25.

Full suite

pytest test/unittests (throwaway uv venv, Python 3.14): 550 passed,
1 failed
in ~69s. The 1 failure —
test/unittests/skills/test_base.py::TestOVOSSkill::test_ask_yesno — is
pre-existing on a clean checkout of dev (unrelated: the
ovos-solver-yes-no-plugin isn't installed in this env, so
ask_yesno('nope') falls back to the heuristic engine, which returns the
raw utterance 'nope' instead of the normalized 'no') and is untouched
by this change — verified by running the same test file against dev
before creating this branch.

Scope

ovos-workshop#513 (open, touches decorators/killable.py) is unrelated
and untouched by this PR — no overlap, no rebase taken on it.

No new bus message types. No behavior change on the normal/abort paths.

_wait_response() busy-polled `while not ans: time.sleep(0.1)` with no
deadline while waiting on the killable background thread
(_real_wait_response) to populate __validated_responses. If that thread
never finishes (eg. the bus/TTS handshake it waits on inside
__get_response never completes), the caller blocks indefinitely instead
of returning the documented "no response" (None) outcome.

Add a deadline derived from the same budget the background thread itself
uses per attempt: skills.get_response_timeout (default 20s) plus the 15s
wait_while_speaking() ceiling already used elsewhere in this file for a
reprompt, multiplied by the number of retry attempts. Normal-path and
abort-path (externally set None) behavior is unchanged; only the
previously-infinite stall case now returns None within a bounded time.

Field evidence: OpenVoiceOS/ovos-skill-alerts#138 "Update 3" captured a
live py-spy stack of ask_yesno -> get_response -> _wait_response blocked
forever in exactly this loop, hanging a CI job to its 30-minute kill.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@JarbasAl, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bcf7022e-c994-4814-9351-76e1da089eaa

📥 Commits

Reviewing files that changed from the base of the PR and between 7aaa4c5 and 2ef46ed.

📒 Files selected for processing (2)
  • ovos_workshop/skills/ovos.py
  • test/unittests/test_wait_response_deadline.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the fix label Aug 11, 2026
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Processing sequence 0x4F564F53 complete! 🦾

I've aggregated the results of the automated checks for this PR below.

📋 Repo Health

Ensuring the project's documentation is healthy. 📚

✅ All required files present.

Latest Version: 9.3.3a1

ovos_workshop/version.py — Version file
README.md — README
LICENSE — License file
pyproject.toml — pyproject.toml
⚠️ setup.py — setup.py
CHANGELOG.md — Changelog
ovos_workshop/version.py has valid version block markers

⚖️ License Check

Navigating the maze of open-source compliance. 🧩

✅ No license violations found.

Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed.

🔍 Lint

Analysis complete! Check out the details below. 📊

ruff: issues found — see job log

🔒 Security (pip-audit)

Ensuring our certificates are valid and trustworthy. 📜

✅ No known vulnerabilities found (74 packages scanned).

🔨 Build Tests

The blueprints match the build! 📐

✅ All versions pass

Python Build Install Tests
3.10
3.11
3.12
3.13
3.14

Your automated companion in the OpenVoiceOS journey. 🤝

@JarbasAl
JarbasAl marked this pull request as ready for review August 11, 2026 14:15
@JarbasAl
JarbasAl merged commit ff07c5c into dev Aug 11, 2026
12 checks passed
@JarbasAl
JarbasAl deleted the fix/wait-response-deadline branch August 11, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant