fix: restore idempotent double-registration for intent-topic wrapped handlers - #421
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The automated sentinels have completed their watch. 💂♂️I've aggregated the results of the automated checks for this PR below. 🔍 LintBeep boop! Standard processing sub-routine complete. 🦾 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)Ensuring our cross-site scripting defenses are up. 🛡️ ✅ No known vulnerabilities found (47 packages scanned). 🏷️ Release PreviewI've checked the 'Legal' section for the release. ⚖️ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
📋 Repo HealthThe repo's annual physical is complete! 🩺 ✅ All required files present. Latest Version: ✅ 📊 CoverageTesting the limits! Here's the coverage breakdown. 📏 ✅ 85.3% total coverage Files below 80% coverage (5 files)
Full report: download the 🔨 Build TestsI tried building your changes, and here's what happened! 🔨 ✅ All versions pass
⚖️ License CheckChecking for any potential license conflicts. ⚔️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. Keeping things running like clockwork 🕰️ |
Registering the same handler twice on one guarded topic used to be harmless because pyee keys its listener OrderedDict by the handler object, so an equal bound method collapsed onto one slot. Both the intent-topic bridge and the namespace-migration (is_migrated) branch of the mirror guard started wrapping every registration in a fresh closure, and pyee saw a new object each time, so the same registration fired twice instead of once. FakeBus.on/AsyncFakeBus.on already reused the existing wrapper for a repeat (msg_type, handler) pair; once() bypassed the mirror guard entirely, so a handler bound via once() to both spellings of a mirrored dispatch fired twice, and a later on() of the same handler stacked a second, independent listener because once() never recorded itself in _dedup_registrations. once() now goes through the same guard-selection and wrapper-reuse machinery as on(), self-cleaning its bookkeeping once pyee auto-removes the fired listener. Adds regression coverage for once() on both intent spellings, once() followed by on() of the same handler, duplicate on() registration on the legacy intent spelling, and duplicate on() registration on a migrated-namespace topic (recognizer_loop:utterance) -- the last pins the chosen 1-fire behaviour rather than baseline's 2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
afd75ff to
99f0c7f
Compare
1 participant
Registering the same handler twice on the same intent topic used to be harmless. pyee keys its listener OrderedDict by the handler object itself, so two
on()calls with an equal bound method collapsed onto one slot and the handler only fired once.The intent-topic bridge in
FakeBus.on/AsyncFakeBus.onbroke that by wrapping every intent-topic registration in a fresh closure. pyee then saw two distinct wrapper objects for what was meant to be one subscription, and the handler fired twice for a single dispatch. This surfaced in ovos-workshop astest_dual_registration_does_not_double_firefailing with2 != 1.This restores the old behavior:
on()now reuses the existing wrapper for a repeat(msg_type, handler)pair instead of minting a new one, so re-registering re-adds the same closure pyee already knows.remove()already worked off the same per-handler registration map so it needed no change. Genuinely different handlers on the same topic still both fire, and non-intent topics are untouched. Added 6 regression tests covering same-handler-twice-plus-off, same-handler-both-spellings, two-different-handlers, and the non-intent-topic control, for bothFakeBusandAsyncFakeBus; the full ovos-utils suite (955 passed, 1 skipped) and the ovos-workshop reproducer both pass with the fix.