Repository navigation
Remove old device on DB worker during re-interview - #1899
TheJulianJES wants to merge 3 commits into
Conversation
`_device_reinterviewed` awaited `PersistingListener._remove_device()` directly, outside the worker queue. Its forced commit could therefore land between two statements of another handler, committing that handler's partial writes. Add `PersistingListener.remove_device()`, which queues the removal like any other event but waits until the worker has run it and raises its exception to the caller. The re-interview keeps its guarantee that a failed delete aborts it, and the delete now runs in order with every other database write. Errors that nobody waits for, including a waited handler whose caller was cancelled, are logged as before. If the worker stops, every waiting caller gets a `RuntimeError` instead of waiting forever, and the unfinished jobs are marked done so that `shutdown()` does not hang on them.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1899 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 61 61
Lines 12773 12797 +24
=======================================
+ Hits 12711 12735 +24
Misses 62 62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Events enqueued after the worker stops can still make database shutdown hang.
Review effort: Balanced
Findings: 1
What changed in this PR
Serializes re-interview device deletion through the database worker while preserving error propagation.
Changes:
- Adds queued, awaitable database operations.
- Updates re-interview deletion to use the worker.
- Adds worker failure and ordering tests.
| File | Description |
|---|---|
zigpy/application.py |
Uses queued device removal. |
zigpy/appdb.py |
Adds awaitable queue results and cleanup. |
tests/test_application.py |
Updates re-interview mocks. |
tests/test_appdb.py |
Tests ordering, errors, cancellation, and shutdown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`running` stays true if the worker stops on its own, so events enqueued afterwards would never be marked done and `shutdown()` would hang in `join()`. Discard them, as `remove_device()` already refuses to wait.
An unknown handler name raised outside the worker's error handling and stopped the worker. Since events are discarded once the worker is gone, every later write would then be dropped with only a debug message. Look the handler up inside the error handling, and log an error if the worker stops while the listener is still running. Each queue item now marks itself done and fails its waiting caller in its own `finally`, which replaces the `in_progress` flag. A new test checks that the blocked handler's partial write is not committed while `remove_device()` waits, the bug the queued removal fixes.
zigpy-review-bot
left a comment
There was a problem hiding this comment.
No blocking findings. The re-interview delete now runs on the DB worker in queue order, and its errors still reach the caller. zigpy-review-bot wrote these commits, so this review is a COMMENT and not an approval. The approval should come from a maintainer.
Verified (6 checks)
- Full suite at e196bd2: 1358 passed. zha's suite (current dev) with this branch's zigpy installed editable: 1458 passed.
test_remove_device_waits_for_queued_workfails whenremove_device()is changed back to call_remove_device()directly (assert not remove_task.done()), so it guards the bug this PR fixes.- Worker accounting: every item taken off the queue gets exactly one
task_done(). The item being run gets it in the innerfinally, and the items behind it get it in the drain loop. A cancellation that lands inget()takes no item and calls notask_done().CancelledErrorisn't caught byexcept Exception, so a stopping worker still reaches_fail_pending_result()for the waiting caller. - A cancelled caller cancels its future, so
result.done()is true and the handler's error goes to the normal logging (pinned bytest_remove_device_cancelled_caller_error_is_logged). - Ordering:
old_device.remove_listener()runs before the delete is queued, so no new device-level events for the old device arrive after that. Events already queued for it run before the delete, and the delete then removes their rows. The old direct call could interleave with them instead. - Nothing downstream touches
_remove_device,_callback_handlers, or the queue's item shape (grep of zha, zha-device-handlers, bellows, zigpy-znp, zigpy-deconz, and HA's zha component). No listener event is namedremove_device, so the new public method can't be dispatched as a listener callback by accident.
One optional note inline.
| # Set up a mock DB listener | ||
| db_listener = MagicMock() | ||
| db_listener._remove_device = AsyncMock() | ||
| db_listener.shutdown = AsyncMock() |
There was a problem hiding this comment.
Optional: these db_listener.shutdown = AsyncMock() lines (here, L1940 and L1999) do nothing on this branch alone, because the app fixture doesn't shut the app down yet. They start to matter once #1900 adds await app.shutdown() to the fixture teardown. Without them the tests still pass, but ControllerApplication.shutdown() awaits a plain MagicMock and logs "Failed to disconnect from database". A short comment, or a line in the PR body, would keep someone from removing them as dead code in the meantime.
DRAFT.
ControllerApplication._device_reinterviewed()deleted the old device by awaitingPersistingListener._remove_device()directly, outside the database worker queue. Since #1835, that path ends with a forced commit, so it can land between two statements of whatever handler the worker is running (e.g._neighbors_updated'sDELETEandINSERT), committing that handler's partial writes. This came up in the #1835 review rounds.The direct call was deliberate (#1789): the fire-and-forget
device_removed()only enqueues the delete, while the re-interview needs it done before_finalize_device()enqueues the save of the new device. A queued delete already runs before that save, but a failed one would only be logged, and the save would then go ahead on top of the stale rows. So the re-interview still has to wait for the delete and see its error. Copilot's review on #1789 suggested enqueueing and awaiting completion, which is what this does.Changes
PersistingListener.remove_device(): it queues_remove_devicelike any other event, then waits until the worker has run it. An exception from the handler is raised to the caller instead of being logged by the worker, so a failed delete still aborts the re-interview, as before. If the caller was cancelled in the meantime, the error is logged like any other.enqueue()passesNone, so fire-and-forget events are handled as before, with one exception: once the worker task has stopped,enqueue()discards them, because nothing would mark them done andshutdown()would hang injoin().RuntimeErrorinstead of waiting forever. This covers both the job being run and any queued behind it. Those jobs are also marked done, so a latershutdown()doesn't hang injoin().remove_device()also raisesRuntimeErrorup front if the listener is shutting down or the worker is gone._device_reinterviewed()awaitsremove_device().The delete now runs in order after every write queued before it, including pending writes for the old device, which it then removes. The new device's save is still enqueued only after the delete has finished.
One limitation is unchanged: if the re-interview is cancelled after the delete has been queued, the delete still runs and the new device is never saved. The old direct call behaved the same way, because the
DELETEkept running on aiosqlite's thread and was committed by the next commit.Testing: new tests cover
remove_device():shutdown()still completes afterwards, while a normal shutdown is not logged as an unexpected stop.Another test, with two cases, pins the logging of fire-and-forget handler errors, which had no test before, and one more checks that an unknown handler name is logged and doesn't stop the worker. Each test fails when the behaviour it covers is removed, and the waits on
remove_device()and on the worker have timeouts. Full suite: 1358 passed.