Skip to content

Conversation

@svacatalisan
Copy link

@svacatalisan svacatalisan commented Jan 29, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Audio recognition shutdown now swallows task cancellation errors so cleanup proceeds reliably and shutdown is not interrupted.
  • Tests

    • Added tests covering audio recognition cleanup when background tasks are pre-cancelled, including scenarios where previous behavior skipped subsequent cleanup.

✏️ Tip: You can customize this high-level summary in your review settings.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


svacatalisan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Awaiting cleanup tasks in AudioRecognition.aclose() is now wrapped in try/except blocks that catch asyncio.CancelledError so pre-cancelled tasks won’t propagate cancellation and subsequent cleanup still runs.

Changes

Cohort / File(s) Summary
Error Handling in Cleanup
livekit-agents/livekit/agents/voice/audio_recognition.py
Modified aclose() to wrap awaits for _commit_user_turn_atask and _end_of_turn_task in try/except asyncio.CancelledError blocks so cancellation from pre-cancelled tasks does not abort remaining cleanup.
Cleanup Behavior Tests
tests/test_audio_recognition_aclose.py
Added TestAudioRecognitionAclose with _create_audio_recognition() helper and two async tests: one demonstrating prior behavior where a cancelled await blocked further cleanup, and one asserting aclose() now handles pre-cancelled tasks gracefully and completes cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I waited on tasks that slipped away,
Cancelled mid-hop, they tried to stray.
Now guarded snug, no panic starts,
Cleanup dances, mended parts.
Hooray for safe and tidy plays!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main issue being fixed: handling of stuck aclose() activity that leads to stuck handoff by wrapping task awaits in try/except blocks.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🧹 Recent nitpick comments
tests/test_audio_recognition_aclose.py (2)

22-37: Consider using a more robust test setup approach.

Bypassing __init__ with patch.object and manually initializing private attributes creates tight coupling to implementation details. If AudioRecognition changes its internal attributes, these tests will break silently or fail in confusing ways.

Consider alternatives:

  • Create a test fixture that properly instantiates AudioRecognition with mock dependencies
  • Use dependency injection patterns in the production class to facilitate testing
  • Add a comment documenting which attributes are required and why

This is acceptable for now given the focused scope, but may become a maintenance burden.


39-41: Good demonstration of the bug pattern.

This test effectively documents the problematic behavior that the fix addresses. While it doesn't test AudioRecognition directly, it serves as valuable documentation of why the fix was needed.

For strict mypy compliance, add return type annotation:

🔧 Add return type annotation
 `@pytest.mark.asyncio`
-async def test_unprotected_await_blocks_subsequent_cleanup(self):
+async def test_unprotected_await_blocks_subsequent_cleanup(self) -> None:
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2be7f1 and aad9913.

📒 Files selected for processing (1)
  • tests/test_audio_recognition_aclose.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Format code with ruff
Run ruff linter and auto-fix issues
Run mypy type checker in strict mode
Maintain line length of 100 characters maximum
Ensure Python 3.9+ compatibility
Use Google-style docstrings

Files:

  • tests/test_audio_recognition_aclose.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: unit-tests
  • GitHub Check: type-check (3.13)
  • GitHub Check: type-check (3.9)
🔇 Additional comments (1)
tests/test_audio_recognition_aclose.py (1)

100-142: LGTM - solid regression test for the fix.

This test correctly validates that aclose() handles pre-cancelled tasks gracefully by:

  1. Creating pre-cancelled tasks before calling aclose()
  2. Verifying aclose() completes without raising
  3. Asserting both tasks are properly cleaned up and _closing is set

For mypy strict compliance, add the return type annotation:

🔧 Add return type annotation
 `@pytest.mark.asyncio`
-async def test_aclose_handles_precancelled_tasks_gracefully(self):
+async def test_aclose_handles_precancelled_tasks_gracefully(self) -> None:

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Tip

🧪 Unit Test Generation v2 is now available!

We have significantly improved our unit test generation capabilities.

To enable: Add this to your .coderabbit.yaml configuration:

reviews:
  finishing_touches:
    unit_tests:
      enabled: true

Try it out by using the @coderabbitai generate unit tests command on your code files or under ✨ Finishing Touches on the walkthrough!

Have feedback? Share your thoughts on our Discord thread!


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 and usage tips.

@s-hamdananwar s-hamdananwar requested a review from a team January 29, 2026 13:09
Copy link
Contributor

@longcw longcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the pr! looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants