Replace flag based cleanup in bluetooth_device_connect with per branch cleanup

[bdraco]

What does this implement/fix?

Replaces the flag based cleanup in bluetooth_device_connect with explicit per branch cleanup; each except handler now releases its own resources, the finally clause is reduced to timeout_handle.cancel(), and the three booleans (connect_ok, timeout_expired, unhandled_exception) are gone. The behavior is pinned by test_bluetooth_device_connect_cleanup_contract, which already landed in #1761.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Code quality improvements to existing code or addition of tests
• Other

Related issue or feature (if applicable):

• fixes complexity: bluetooth_device_connect coordinates cleanup via three boolean flags across a 122-line try/except/finally #1754

Pull request in esphome (if applicable):

• esphome/esphome#

Checklist:

• The code change is tested and works locally.
• If api.proto was modified, a linked pull request has been made to esphome with the same changes.
• Tests have been added to verify that the new code works (under tests/ folder).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7647f3f) to head (e3d87b3).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1764   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         4183      4175    -8     
=========================================
- Hits          4183      4175    -8     

☔ 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.

[codspeed-hq]

Merging this PR will not alter performance

✅ 56 untouched benchmarks

---

_{Comparing finish-bluetooth-cleanup-refactor (e3d87b3) with main (7647f3f)}

[bdraco]

@esphbot review

[Copilot]

Pull request overview

Refactors APIClient.bluetooth_device_connect cleanup logic to be handled explicitly within each exception branch, removing the prior flag-based bookkeeping and simplifying the finally block, in line with the cleanup contract pinned by the existing tests.

Changes:

• Removes connect_ok / timeout_expired / unhandled_exception flags and associated finally-block conditionals.
• Moves unsub() and _bluetooth_disconnect_no_wait() cleanup into the relevant except branches (timeout/cancel/base-exception).
• Reduces finally to always cancel the timeout handle.

---

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

[esphbot]

@bdraco: @esphbot review

LGTM. Walked the truth table for all four outcomes and the new per-branch cleanup is behaviour-equivalent to the old flag-based version:

Branch	Old: unsub	Old: timeout_handle.cancel	Old: disconnect_no_wait	New
Success	–	✓	–	same
TimeoutError	✓ (except)	– (skipped)	– (uses guarded path)	unsub + cancel (no-op on fired handle) + guarded disconnect
CancelledError	✓ (finally)	✓	✓	same
BaseException	✓ (finally)	✓	✓	same

Two small side effects of the refactor, both wins:

1. After TimeoutError the new code calls timeout_handle.cancel() in finally. Old code skipped it because timeout_expired=True. asyncio.TimerHandle.cancel() on an already-fired handle is a documented no-op, so this is harmless.
2. If unsub() itself raises inside an except branch, timeout_handle.cancel() still runs (it's the only thing left in finally). In the old code the cancel was sequenced after unsub() in finally, so an unsub throw would skip the cancel.

test_bluetooth_device_connect_cleanup_contract (tests/test_client.py:4228) pins all five paths with explicit unsub / _bluetooth_disconnect_no_wait counts and a _message_handlers leak check, codecov shows 100% on the modified lines, CodSpeed shows no perf delta, and dropping the too-many-branches pylint disable now matches reality. Ship it.

[esphbot]

PR Review — Replace flag based cleanup in bluetooth_device_connect with per branch cleanup

Behaviour-preserving refactor. Walked the truth table for the four reachable states (success, TimeoutError, CancelledError, BaseException) and the new per-branch cleanup matches the old flag-based cleanup exactly. Two minor observations, neither blocking: (1) timeout_handle.cancel() now runs unconditionally in finally, including after a TimeoutError where the old code deliberately skipped it — TimerHandle.cancel() is a no-op on already-fired handles, so this is harmless and arguably more robust; (2) the new structure also makes timeout_handle.cancel() reachable when unsub() itself raises (the old code put unsub() before the cancel inside finally, so a throw aborted the cancel) — a small robustness win. test_bluetooth_device_connect_cleanup_contract (tests/test_client.py:4228) pins every branch with explicit unsub/_bluetooth_disconnect_no_wait counts and a handler-leak snapshot, codecov reports 100% coverage on the modified lines, and CodSpeed shows no perf delta. The dropped too-many-branches pylint disable now matches reality.

---

---

Checklist

• Per-branch cleanup equivalent to old flag-based cleanup across all 4 outcomes
• timeout_handle.cancel() safe when timeout already fired
• Test contract pins unsub + disconnect counts for every branch
• No resource leaks (handler-leak assertion in test)
• No new bare except / silent swallow
• Comments reflect current code, not historical rationale

---

Summary

Behaviour-preserving refactor. Walked the truth table for the four reachable states (success, TimeoutError, CancelledError, BaseException) and the new per-branch cleanup matches the old flag-based cleanup exactly. Two minor observations, neither blocking: (1) timeout_handle.cancel() now runs unconditionally in finally, including after a TimeoutError where the old code deliberately skipped it — TimerHandle.cancel() is a no-op on already-fired handles, so this is harmless and arguably more robust; (2) the new structure also makes timeout_handle.cancel() reachable when unsub() itself raises (the old code put unsub() before the cancel inside finally, so a throw aborted the cancel) — a small robustness win. test_bluetooth_device_connect_cleanup_contract (tests/test_client.py:4228) pins every branch with explicit unsub/_bluetooth_disconnect_no_wait counts and a handler-leak snapshot, codecov reports 100% coverage on the modified lines, and CodSpeed shows no perf delta. The dropped too-many-branches pylint disable now matches reality.

---

Automated review by Kōane3d87b3

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 08895025-37c9-411b-993b-169e7fb29590

📥 Commits

Reviewing files that changed from the base of the PR and between 7647f3f and e3d87b3.

📒 Files selected for processing (1)

• aioesphomeapi/client.py

---

Walkthrough

The PR refactors bluetooth_device_connect to eliminate flag-based error-handling coordination. TimeoutError, CancelledError, and BaseException handlers now manage cleanup explicitly within their branches. The lint suppression updates to reflect the reduced branch complexity, and the finally block is simplified to only cancel the timeout handle.

Changes

Bluetooth Connection Cleanup Refactor

Layer / File(s)	Summary
Method declaration and lint suppression update aioesphomeapi/client.py	Lint suppression on bluetooth_device_connect changes from too-many-branches to too-many-locals to reflect the refactored error-handling structure.
Error handling paths refactor aioesphomeapi/client.py	TimeoutError, CancelledError, and BaseException handlers now execute cleanup (unsubscribe, disconnect) explicitly within their branches instead of relying on shared flag-derived logic in the finally block. The finally block is simplified to unconditionally cancel the timeout handle.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch finish-bluetooth-cleanup-refactor

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}