Skip to content

Serialize multicast table subscribe/unsubscribe - #771

Merged
puddly merged 1 commit into
devfrom
zigpy-bot/multicast-subscribe-lock
Oct 23, 2026
Merged

puddly merged 1 commit into
devfrom
zigpy-bot/multicast-subscribe-lock

Conversation

@zigpy-review-bot

Copy link
Copy Markdown
Collaborator

Summary

Multicast.subscribe() only records a group in its cache after the firmware write is awaited, but it claims a free table slot before that await. Two concurrent calls for the same group both miss the cache, each pop a slot and write it, and the second cache write overwrites the first slot reference. One multicast table slot is leaked per race, and the table is small (bellows asks for 16).

This became reachable with zigpy's subscribe_to_multicast_group() (zigpy/zigpy#1864) and the quirk-side API in zigpy/zha-device-handlers#5209: ZHA initializes devices concurrently, so several devices sharing a quirk (e.g. two BILRESA remotes) request the same group at the same time.

Changes

  • Guard Multicast.subscribe() and Multicast.unsubscribe() with one asyncio.Lock, so check, slot claim, firmware write and cache update are atomic with respect to each other. The already-subscribed early return then handles the second caller.
  • startup() is unchanged and does not hold the lock, so there is no re-entrancy.

Testing

  • Added test_subscribe_concurrent_same_group (two concurrent subscribes use one slot and unsubscribing frees it) and test_subscribe_unsubscribe_serialized. Both fail without the lock.
  • Full suite: 766 passed. pre-commit clean.

The subscription cache is only updated after the firmware write is awaited, so concurrent subscribe() calls for the same group (e.g. several devices initializing at once and requesting the same group) each popped a free slot and wrote it, leaking a table slot and overwriting the cached slot reference. Guard subscribe/unsubscribe with a lock.
@zigpy-review-bot zigpy-review-bot added the bugfix This PR fixes a bug label Oct 23, 2026
@codecov

codecov Bot commented Oct 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.59%. Comparing base (f747e95) to head (0215209).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #771   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files          64       64           
  Lines        4409     4417    +8     
=======================================
+ Hits         4391     4399    +8     
  Misses         18       18           

☔ View full report in Codecov by Harness.
📢 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.

@puddly
puddly merged commit 6210ab5 into dev Oct 23, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This PR fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants