Skip to content

Comments

Fix race condition: await remove_all_listeners() in on_shutdown#311

Merged
NodeJSmith merged 3 commits intomainfrom
309-await-remove-all-listeners
Feb 24, 2026
Merged

Fix race condition: await remove_all_listeners() in on_shutdown#311
NodeJSmith merged 3 commits intomainfrom
309-await-remove-all-listeners

Conversation

@NodeJSmith
Copy link
Owner

Summary

  • Bus.remove_all_listeners() returns an asyncio.Task but three on_shutdown methods (ServiceWatcher, StateProxy, AppHandler) called it without await, creating a race condition where listeners could survive shutdown if the task bucket was cancelled first
  • Added await to all three call sites and fixed the test fixture mock to return an awaitable task

Closes #309

ServiceWatcher, StateProxy, and AppHandler called Bus.remove_all_listeners()
without awaiting the returned Task, creating a race condition where listeners
could survive shutdown if the task bucket was cancelled first.
Copilot AI review requested due to automatic review settings February 24, 2026 22:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a race condition in shutdown handlers where Bus.remove_all_listeners() was called without await, potentially allowing listeners to survive shutdown if the task bucket was cancelled before cleanup completed.

Changes:

  • Added await to three remove_all_listeners() calls in on_shutdown() methods
  • Fixed test fixture to return an awaitable Task instead of a plain Mock

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/hassette/core/state_proxy.py Added await to remove_all_listeners() call in on_shutdown()
src/hassette/core/service_watcher.py Added await to remove_all_listeners() call in on_shutdown()
src/hassette/core/app_handler.py Added await to remove_all_listeners() call in on_shutdown()
tests/integration/test_state_proxy.py Updated mock to return awaitable Task via asyncio.ensure_future()
CHANGELOG.md Added entry documenting the fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@NodeJSmith NodeJSmith merged commit e832950 into main Feb 24, 2026
6 checks passed
@NodeJSmith NodeJSmith deleted the 309-await-remove-all-listeners branch February 24, 2026 22:29
@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.99%. Comparing base (9d0147c) to head (4cfda51).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #311   +/-   ##
=======================================
  Coverage   79.99%   79.99%           
=======================================
  Files         134      134           
  Lines        9531     9531           
  Branches      943      943           
=======================================
  Hits         7624     7624           
- Misses       1538     1539    +1     
+ Partials      369      368    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Await remove_all_listeners() in on_shutdown methods

1 participant