diff --git a/ovos_core/intent_services/service.py b/ovos_core/intent_services/service.py index f1e84c74e21..323bdd20e1f 100644 --- a/ovos_core/intent_services/service.py +++ b/ovos_core/intent_services/service.py @@ -158,9 +158,15 @@ def __init__(self, bus, config=None, preload_pipelines=True, # ovos.intent.list / ovos.intent.describe pull-queries. self.intent_manifest: IntentManifest = IntentManifest(bus) - # connection SessionManager to the bus, - # this will sync default session across all components - SessionManager.connect_to_bus(self.bus) + # connect SessionManager to the bus, this will sync default session + # across all components. Guarded so the same bus does not get the + # five SessionManager handlers registered twice: in the monolith, + # SkillManager.__init__ runs first and connects the bus before this + # IntentService is constructed; in embedders that construct + # IntentService directly (without a SkillManager), this call site + # is the first to connect it. + if SessionManager.bus is not self.bus: + SessionManager.connect_to_bus(self.bus) self.bus.on(SpecMessage.UTTERANCE, self.handle_utterance) diff --git a/ovos_core/skill_manager.py b/ovos_core/skill_manager.py index 21ce46dcfb0..c61a9fe2e35 100644 --- a/ovos_core/skill_manager.py +++ b/ovos_core/skill_manager.py @@ -21,6 +21,7 @@ from ovos_bus_client.client import MessageBusClient from ovos_bus_client.message import Message +from ovos_bus_client.session import SessionManager from ovos_bus_client.util.scheduler import EventScheduler from ovos_config.config import Configuration from ovos_config.locations import get_xdg_config_save_path @@ -134,6 +135,16 @@ def __init__(self, bus: MessageBusClient, self.status.bind(self.bus) + # Connect SessionManager to the bus regardless of whether the intent + # service runs in this process: speak(wait=True)/wait_while_speaking + # depend on SessionManager.bus being set, and skills-only processes + # (enable_intent_service=False, e.g. --disable-intent-service) would + # otherwise never get it. Guarded so the monolith path (intent + # service enabled in this same process) does not register the five + # SessionManager bus handlers twice via IntentService.__init__. + if SessionManager.bus is not self.bus: + SessionManager.connect_to_bus(self.bus) + # init subsystems self.osm = SkillsStore(self.bus) if enable_installer else None self.event_scheduler = EventScheduler(self.bus, autostart=False) if enable_event_scheduler else None diff --git a/test/unittests/test_manager.py b/test/unittests/test_manager.py index d0ee7df999c..55422939538 100644 --- a/test/unittests/test_manager.py +++ b/test/unittests/test_manager.py @@ -3,6 +3,7 @@ from unittest.mock import MagicMock, patch from ovos_bus_client.message import Message +from ovos_bus_client.session import SessionManager from ovos_core.skill_manager import SkillManager @@ -10,8 +11,17 @@ class TestSkillManager(unittest.TestCase): def setUp(self): + SessionManager.bus = None self.bus = MagicMock() self.skill_manager = SkillManager(self.bus) + # SkillManager.__init__ now wires SessionManager.connect_to_bus(), + # which emits an "ovos.session.update_default" broadcast on + # construction; reset the mock so tests only observe emits from the + # code under test, not this setup side effect + self.bus.reset_mock() + + def tearDown(self): + SessionManager.bus = None def test_blacklist_property(self): blacklist = self.skill_manager.blacklist diff --git a/test/unittests/test_skill_manager.py b/test/unittests/test_skill_manager.py index 6dca310e00f..42011ef4c6d 100644 --- a/test/unittests/test_skill_manager.py +++ b/test/unittests/test_skill_manager.py @@ -23,6 +23,8 @@ from ovos_bus_client.message import Message from ovos_config import Configuration from ovos_config import LocalConf, DEFAULT_CONFIG +from ovos_bus_client.session import SessionManager +from ovos_spec_tools import SpecMessage from ovos_core.skill_manager import SkillManager from ovos_workshop.skill_launcher import SkillLoader @@ -69,10 +71,16 @@ class TestSkillManager(TestCase): def setUp(self): temp_dir = tempfile.mkdtemp() self.temp_dir = Path(temp_dir) + SessionManager.bus = None self.message_bus_mock = MessageBusMock() self._mock_log() self.skill_manager = SkillManager(self.message_bus_mock) self._mock_skill_loader_instance() + # SkillManager.__init__ now wires SessionManager.connect_to_bus(), + # which emits an "ovos.session.update_default" broadcast; drop that + # setup noise so tests only see messages emitted by the code under test + self.message_bus_mock.message_types = [] + self.message_bus_mock.message_data = [] def _mock_log(self): log_patch = patch(self.mock_package + 'LOG') @@ -81,6 +89,7 @@ def _mock_log(self): def tearDown(self): rmtree(str(self.temp_dir)) + SessionManager.bus = None def _mock_skill_loader_instance(self): self.skill_dir = self.temp_dir.joinpath('test_skill') @@ -99,6 +108,7 @@ def test_instantiate(self): # Ensure deferred_loading is explicitly False to isolate from other tests config = mock_config() config['skills']['use_deferred_loading'] = False + SessionManager.bus = None with patch.dict(Configuration._Configuration__patch, config): bus_mock = MessageBusMock() skill_manager = SkillManager(bus_mock) @@ -111,10 +121,19 @@ def test_instantiate(self): #'mycroft.skills.initialized', 'mycroft.skills.is_alive', 'mycroft.skills.is_ready', - 'mycroft.skills.all_loaded' + 'mycroft.skills.all_loaded', + # SessionManager.connect_to_bus() handlers - wired + # unconditionally so skills-only processes (no intent + # service) still get SessionManager.bus set + 'recognizer_loop:record_begin', + 'recognizer_loop:record_end', + 'recognizer_loop:audio_output_start', + 'recognizer_loop:audio_output_end', + SpecMessage.SESSION_SYNC, ] self.assertListEqual(expected_result, bus_mock.event_handlers) + SessionManager.bus = None def test_send_skill_list(self): @@ -432,9 +451,13 @@ class TestDeferredLoadingConfigFlag(TestCase): mock_package = 'ovos_core.skill_manager.' def setUp(self): + SessionManager.bus = None self.message_bus_mock = MessageBusMock() self._mock_log() + def tearDown(self): + SessionManager.bus = None + def _mock_log(self): log_patch = patch(self.mock_package + 'LOG') self.addCleanup(log_patch.stop) @@ -471,7 +494,12 @@ def test_connectivity_handlers_not_registered_when_deferred_loading_disabled(sel 'skillmanager.activate', 'mycroft.skills.is_alive', 'mycroft.skills.is_ready', - 'mycroft.skills.all_loaded' + 'mycroft.skills.all_loaded', + 'recognizer_loop:record_begin', + 'recognizer_loop:record_end', + 'recognizer_loop:audio_output_start', + 'recognizer_loop:audio_output_end', + SpecMessage.SESSION_SYNC, ] self.assertListEqual(expected_handlers, self.message_bus_mock.event_handlers) @@ -501,7 +529,12 @@ def test_connectivity_handlers_registered_when_deferred_loading_enabled(self): 'mycroft.gui.unavailable', 'mycroft.skills.is_alive', 'mycroft.skills.is_ready', - 'mycroft.skills.all_loaded' + 'mycroft.skills.all_loaded', + 'recognizer_loop:record_begin', + 'recognizer_loop:record_end', + 'recognizer_loop:audio_output_start', + 'recognizer_loop:audio_output_end', + SpecMessage.SESSION_SYNC, ] self.assertListEqual(expected_handlers, self.message_bus_mock.event_handlers) @@ -614,3 +647,55 @@ def test_run_uses_deferred_loading_when_enabled(self): skill_manager._mark_startup_complete_and_consume_deferred.assert_called() # Verify _load_new_skills is NOT called in deferred startup path (only in loop) skill_manager._load_new_skills.assert_not_called() + + +@patch.dict(Configuration._Configuration__patch, mock_config()) +class TestSkillManagerSessionManagerBus(TestCase): + """ + Regression test: SkillManager must wire SessionManager.connect_to_bus() + even when the intent service is disabled in this process (the default, + and the documented --disable-intent-service CLI path). Without this, + SessionManager.bus stays None in skills-only processes and + speak(wait=True)/SessionManager.wait_while_speaking silently no-op. + Mirrors the sibling fix/test in ovos-workshop#526 (SkillContainer). + """ + + def setUp(self): + SessionManager.bus = None + + def tearDown(self): + SessionManager.bus = None + + def test_connect_to_bus_with_intent_service_disabled(self): + bus = MessageBusMock() + SkillManager(bus, enable_intent_service=False) + self.assertIsNotNone(SessionManager.bus) + self.assertIs(SessionManager.bus, bus) + + def test_connect_to_bus_exactly_once_with_intent_service_enabled(self): + """ + Regression test: in the monolith (enable_intent_service=True), + SkillManager.__init__ connects SessionManager to the bus before + constructing IntentService, and IntentService.__init__ used to call + SessionManager.connect_to_bus() unconditionally. Same bus object on + both call sites means every standard monolith boot registered all + five SessionManager bus handlers twice. Assert exactly one handler + per topic is registered, regardless of which subsystem connects + first. + """ + bus = MessageBusMock() + SkillManager(bus, enable_intent_service=True, enable_file_watcher=False) + self.assertIsNotNone(SessionManager.bus) + self.assertIs(SessionManager.bus, bus) + for topic in ( + "recognizer_loop:record_begin", + "recognizer_loop:record_end", + "recognizer_loop:audio_output_start", + "recognizer_loop:audio_output_end", + SpecMessage.SESSION_SYNC, + ): + self.assertEqual( + bus.event_handlers.count(topic), 1, + f"expected exactly one handler for {topic}, got " + f"{bus.event_handlers.count(topic)}" + )