Repository navigation
Stop the topology scan loop even during a scheduled scan - #1904
zigpy-review-bot wants to merge 1 commit into
Conversation
`_scan_loop` catches `CancelledError` so that a manual scan, which cancels the running scheduled scan, does not end the loop. It also caught the cancellation of the loop task itself: if `stop_periodic_scans()` was called while a scheduled scan was running, the loop kept scanning. Re-raise when the loop task itself is being cancelled. The `test_periodic_scan_priority` flake (#1171) also hits this path: a timing race let the loop start a fourth scan just as the second manual scan finished, and stopping the loop then didn't end it. The test now drives each scan with an event instead of sleeps and checks that a manual scan doesn't end the loop. A new test covers stopping during a scheduled scan.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1904 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 61 61
Lines 12773 12775 +2
=======================================
+ Hits 12711 12713 +2
Misses 62 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
Cancellation behavior is correctly differentiated and covered by focused regression tests.
0 open findings
What changed in this PR
Fixes topology scan-loop cancellation while preserving manual scan preemption.
Changes:
- Distinguishes loop cancellation from scheduled-scan cancellation.
- Replaces timing-sensitive tests with event-controlled scans.
- Adds coverage for stopping an active scheduled scan.
| File | Description |
|---|---|
zigpy/topology.py |
Correctly terminates a cancelled scan loop. |
tests/test_topology.py |
Adds deterministic cancellation and priority tests. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zigpy-review-bot
left a comment
There was a problem hiding this comment.
No findings. The fix is correct and minimal, and the tests pin both halves of the behavior. (COMMENT only, since this PR is bot-authored.)
The new branch re-raises only when the loop task itself is being cancelled (current_task().cancelling() > 0). A manual scan() cancels only the inner _scan_task, so the loop's own count stays at 0 and the loop keeps going as before. Task.cancelling() needs Python 3.11, which matches requires-python = ">=3.11". Dropping CancelledError from the second except clause changes nothing, because the first clause already catches every CancelledError.
Out of scope here, but the same swallow-every-CancelledError pattern is in catching_coro (zigpy/util.py:302). It wraps one-shot tasks, so it can't cause a stuck loop like this one. It's worth fixing in a separate PR.
Verified (6 checks)
- Full suite at 1302bd2: 1350 passed. CI: all 22 checks green.
- Mutation, dev's
topology.pywith the PR's tests:test_stop_periodic_scans_during_scheduled_scanfails (assert Falseon_scan_loop_task.cancelled(), bounded by the 5 s wait rather than hanging).test_periodic_scan_prioritypasses. - Mutation, unconditional
raisein theCancelledErrorhandler:test_periodic_scan_priorityfails atassert not topology._scan_loop_task.done(), so the "manual scan doesn't end the loop" half is pinned too. - Both new tests: 25/25 consecutive passes. No sleeps decide the outcome; the remaining
sleep(0.1)/sleep(0.05)only give a buggy loop the chance to misbehave. - Race check: if
stop_periodic_scans()runs after_scan_taskfinishes but before the loop resumes,Task.cancel()sets_must_cancel. TheCancelledErroris then thrown atawait self._scan_taskwithcancelling() == 1, so it re-raises as well. - Callers:
ControllerApplication.initialize(start_periodic_scans) andshutdown(stop_periodic_scans,zigpy/application.py:594). The loop now actually stops at shutdown. Downstream, HA's ZHA websocket API only callstopology.scan()(a manual scan), and that behavior is unchanged. zha doesn't touch the loop API. No public API change. - Copilot (sol + luna) had already reviewed this exact head before the PR was opened and had no findings, so I didn't run it again.
stop_periodic_scans()doesn't stop the topology scan loop if it is called while a scheduled scan is running._scan_loopcatchesCancelledErroraroundawait self.scan(), so that a manual scan, which cancels the running scheduled scan, doesn't end the loop. That handler also catches the cancellation of the loop task itself, so the loop keeps going. Ondev, afterstop_periodic_scans()during a scheduled scan, the loop task is still running. The same applies toControllerApplication.shutdown(), which stops the periodic scans the same way, and tostart_periodic_scans()during a scheduled scan, which would leave two loops running (today it's only called once, at startup).The old
test_periodic_scan_priorityflake (#1171,assert 4 == 3), which just failed CI on #1900, also hits this bug. The fourth call is a timing race in the test: when the loop's timer fired just as the second manual scan finished, the loop started another scheduled scan before the test stopped it. Because of the bug, the test'sstop_periodic_scans()then didn't even end the loop. The CI log shows a lingeringTopology._scan_looptask, still "cancelling", at teardown.Changes
_scan_loopre-raises theCancelledErrorwhen its own task is being cancelled (asyncio.current_task().cancelling()). A cancelled scheduled scan still just logs and keeps the loop going. The secondexceptclause, which also listedCancelledError, is reduced toexcept Exception; the first clause already handles everyCancelledError.test_periodic_scan_prioritydrives each mocked scan with an event instead of 0.5 s and 0.15 s sleeps, so it no longer depends on timing. It also checks that the loop keeps running when a manual scan preempts the scheduled one, and that it ends afterstop_periodic_scans(). Re-raising everyCancelledErrorinstead fails this test.test_stop_periodic_scans_during_scheduled_scanstops the loop during a scheduled scan. It fails ondev. Both tests release their mocked scans and cancel the loop until it's done in afinally, so a regression fails them instead of hanging the test session.Testing: full suite passes (1350), and both tests passed 20 runs in a row.