Skip to content

Clean up raw device callbacks after quirk wrapping#1795

Draft
TheJulianJES wants to merge 5 commits intozigpy:devfrom
TheJulianJES:tjj/fix_quirked_device_leak
Draft

Clean up raw device callbacks after quirk wrapping#1795
TheJulianJES wants to merge 5 commits intozigpy:devfrom
TheJulianJES:tjj/fix_quirked_device_leak

Conversation

@TheJulianJES
Copy link
Copy Markdown
Contributor

@TheJulianJES TheJulianJES commented Mar 19, 2026

DRAFT.

AI summary

When device_initialized() applies a quirk via get_device(), the raw Device object is replaced by a new BaseCustomDevice instance. However, the raw device's registered callback listeners (notably the PollControl check-in listener from Device.__init__) were never cleaned up, leaving an orphaned _req_listeners entry keyed by the dead raw device object.

This removes the raw device's _req_listeners key entirely when quirk wrapping creates a new instance. When no quirk matches (same object returned), nothing is changed.

Why _req_listeners.pop() instead of device.on_remove()

device_initialized() runs inside the device's own _initialize_task:

schedule_initialize()
  └─ _initialize_task = create_task(initialize())
       └─ initialize() → _initialize()
            ├─ _discover()
            └─ device_initialized(self)  ← here

create_task() adds the task to self._tasks and registers self._tasks.remove as a done callback. on_remove() calls self._tasks.clear(), emptying the set. When _initialize_task completes moments later, its done callback fires self._tasks.remove(task) on the now-empty set, raising KeyError.

_req_listeners.pop(device, None) directly removes the orphaned listener entry without touching tasks, avoiding this issue.

Related

Related PR that added the PollControl check-in listener:

Found here:

@TheJulianJES TheJulianJES requested a review from Copilot March 19, 2026 04:03
Copy link
Copy Markdown
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 addresses a lifecycle issue in ControllerApplication.device_initialized(): when a device is “quirk wrapped” (raw Device replaced by a new BaseCustomDevice instance), callbacks registered on the raw device were not being cleaned up, leading to stale request-listener registrations.

Changes:

  • Replace in-place reassignment with quirked = zigpy.quirks.get_device(device) and store quirked into app.devices.
  • Call device.on_remove() on the raw device when quirk wrapping returns a new object.
  • Add tests covering the quirk-wrapping vs non-wrapping initialization paths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
zigpy/application.py Cleans up raw device callbacks when quirk wrapping replaces the device instance.
tests/test_application.py Adds regression tests for raw-device cleanup behavior during initialization.

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

Comment thread zigpy/application.py Outdated
Comment thread zigpy/application.py Outdated
Comment thread tests/test_application.py
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.52%. Comparing base (190043d) to head (d76c322).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1795   +/-   ##
=======================================
  Coverage   99.52%   99.52%           
=======================================
  Files          64       64           
  Lines       12930    12932    +2     
=======================================
+ Hits        12868    12870    +2     
  Misses         62       62           

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

@TheJulianJES TheJulianJES requested a review from Copilot March 19, 2026 04:19
Copy link
Copy Markdown
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 addresses a lifecycle/memory-leak issue during device initialization when quirk matching replaces the raw Device instance with a new (quirked) device object, ensuring callback listeners registered on the raw device don’t remain orphaned in the application’s request-listener registry.

Changes:

  • Update ControllerApplication.device_initialized() to remove the raw device’s _req_listeners entry when quirk wrapping returns a different object.
  • Adjust initialization flow to consistently use the quirked device for app.devices, DB listener attachment, and device_initialized event emission.
  • Add tests covering both the quirk-replacement cleanup case and the no-quirk (same object) case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
zigpy/application.py Removes raw-device _req_listeners entry when quirk wrapping replaces the instance; switches subsequent initialization steps to the quirked device.
tests/test_application.py Adds regression tests asserting _req_listeners key removal on quirk replacement and no change when no quirk is applied.

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

Comment thread tests/test_application.py
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.

2 participants