fix: tolerate removing unregistered record listener - #1783
Conversation
Sorry, something went wrong.
Demonstrates GHSA-5pv9-xcmm-gqc7: set.remove() raises KeyError, which the except ValueError clause cannot catch.
self.listeners is a set, so .remove() raises KeyError, never ValueError — the existing except clause could not catch the only realistic failure mode. Switch to the set-native discard idiom so removing a listener that was never registered (e.g. during teardown / reconnect sequences) no longer breaks the shutdown flow.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #1783 +/- ##
==========================================
+ Coverage 99.77% 99.80% +0.02%
==========================================
Files 33 33
Lines 3536 3534 -2
Branches 498 499 +1
==========================================
- Hits 3528 3527 -1
+ Misses 5 4 -1
Partials 3 3 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
bdraco
left a comment
There was a problem hiding this comment.
this should still log debug if listener was never there
Sorry, something went wrong.
Sorry, something went wrong.
Rebase with requested adjustmentsBranch koan/fix-remove-listener-keyerror was rebased onto master and review feedback was applied. Stats2 files changed, 16 insertions(+), 4 deletions(-)
CI statusCI will be checked asynchronously. Automated by Kōan |
Sorry, something went wrong.
What
Switch RecordManager.async_remove_listener from set.remove() + except ValueError to set.discard(), so removing a listener that was never registered no longer raises.
Why
self.listeners is a set, so .remove(listener) raises KeyError on a missing element — not ValueError. The existing except ValueError clause could not catch the only realistic failure mode of that line, so the documented "log and continue" intent was effectively dead code. A teardown / reconnect path that double-removes a listener propagates KeyError out of Zeroconf.async_remove_listener and breaks the surrounding shutdown.
How
Use set.discard() (the set-native "remove if present" idiom) gated by if listener in self.listeners, so async_notify_all() only fires when something actually changed. The try / except is gone.
Testing
Added tests/test_handlers.py::test_async_remove_listener_missing_does_not_raise in a separate commit against the unfixed code first, so the before/after is explicit. Full suite (SKIP_CYTHON=1 pytest): 393 passed.
Quality Report
Changes: 2 files changed, 16 insertions(+), 4 deletions(-)
Code scan: clean
Tests: passed (4 PASSED)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline