perf: reuse session during converse matching

ovos_core/intent_services/converse_service.py

@@ -97,14 +97,18 @@ def _resolve_timeout() -> None:
     @property
     def active_skills(self):
         session = SessionManager.get()
-        return session.active_skills
+        return [
+            (handler["skill_id"], handler["activated_at"])
+            for handler in session.active_handlers
+        ]
 
     @active_skills.setter
     def active_skills(self, val):
         session = SessionManager.get()
-        session.active_skills = []
-        for skill_id, ts in val:
-            session.activate_skill(skill_id)
+        session.active_handlers = [
+            {"skill_id": skill_id, "activated_at": activated_at}
+            for skill_id, activated_at in val
+        ]
 
     @staticmethod
     def get_active_skills(message: Optional[Message] = None) -> List[str]:
@@ -115,7 +119,7 @@ def get_active_skills(message: Optional[Message] = None) -> List[str]:
             active_skills (list): ordered list of skill_ids
         """
         session = SessionManager.get(message)
-        return [skill[0] for skill in session.active_skills]
+        return [handler["skill_id"] for handler in session.active_handlers]
 
     def deactivate_skill(self, skill_id: str, source_skill: Optional[str] = None,
                          message: Optional[Message] = None) -> None:
@@ -272,16 +276,29 @@ def _converse_allowed(self, skill_id: str) -> bool:
             return False
         return True
 
-    def _collect_converse_skills(self, message: Message) -> List[str]:
+    def _collect_converse_skills(
+        self,
+        message: Message,
+        session: Optional[Session] = None,
+    ) -> List[str]:
         """use the messagebus api to determine which skills want to converse
 
         Individual skills respond to this request via the `can_converse` method"""
         skill_ids = []
         want_converse = []
-        session = SessionManager.get(message)
+        if session is None:
+            session = SessionManager.get(message)
+            active_skill_ids = self.get_active_skills(message)
+        else:
+            # ``match`` already folded this message snapshot into the live
+            # session. Reusing it avoids rebuilding and refolding the same
+            # snapshot while preserving direct callers of this helper.
+            active_skill_ids = [
+                handler["skill_id"] for handler in session.active_handlers
+            ]
 
         # note: this is sorted by priority already
-        active_skills = [skill_id for skill_id in self.get_active_skills(message)
+        active_skills = [skill_id for skill_id in active_skill_ids
                      if session.utterance_states.get(skill_id, UtteranceState.INTENT) == UtteranceState.INTENT]
         if not active_skills:
             return want_converse
@@ -319,14 +336,21 @@ def handle_ack(msg: Message) -> None:
             self.bus.remove("skill.converse.pong", handle_ack)
         return want_converse
 
-    def _check_converse_timeout(self, message: Message):
+    def _check_converse_timeout(
+        self,
+        message: Message,
+        session: Optional[Session] = None,
+    ) -> None:
         """ filter active skill list based on timestamps """
         timeouts = self.config.get("skill_timeouts") or {}
         def_timeout = self.config.get("timeout", 300)
-        session = SessionManager.get(message)
-        session.active_skills = [
-            skill for skill in session.active_skills
-            if time.time() - skill[1] <= timeouts.get(skill[0], def_timeout)]
+        if session is None:
+            session = SessionManager.get(message)
+        session.active_handlers = [
+            handler for handler in session.active_handlers
+            if time.time() - handler["activated_at"]
+            <= timeouts.get(handler["skill_id"], def_timeout)
+        ]
 
     def match(self, utterances: List[str], lang: str, message: Message) -> Optional[IntentHandlerMatch]:
         """
@@ -360,8 +384,12 @@ def match(self, utterances: List[str], lang: str, message: Message) -> Optional[
         utterances = flatten_list(utterances)
 
         # note: this is sorted by priority already
-        gr_skills = [skill_id for skill_id in self.get_active_skills(message)
-                     if session.utterance_states.get(skill_id, UtteranceState.INTENT) == UtteranceState.RESPONSE]
+        gr_skills = [
+            handler["skill_id"] for handler in session.active_handlers
+            if session.utterance_states.get(
+                handler["skill_id"], UtteranceState.INTENT
+            ) == UtteranceState.RESPONSE
+        ]
 
         # check if any skill wants to capture utterance for self.get_response method
         for skill_id in gr_skills:
@@ -378,10 +406,14 @@ def match(self, utterances: List[str], lang: str, message: Message) -> Optional[
             )
 
         # filter allowed skills
-        self._check_converse_timeout(message)
+        self._check_converse_timeout(message, session)
+        # Keep the message snapshot aligned with the filtered live session.
+        # Converse pings are derived from this message, so a stale snapshot
+        # must not be able to restore handlers that just expired.
+        message.context["session"] = session.serialize()
 
         # check if any skill wants to converse
-        for skill_id in self._collect_converse_skills(message):
+        for skill_id in self._collect_converse_skills(message, session):
             if skill_id in (session.blacklisted_skills or []):
                 LOG.debug(f"ignoring match, skill_id '{skill_id}' blacklisted by Session '{session.session_id}'")
                 continue

test/unittests/test_converse_service.py

@@ -18,8 +18,7 @@
 from unittest.mock import MagicMock, patch
 
 from ovos_bus_client.message import Message
-from ovos_bus_client.session import Session, SessionManager, UtteranceState
-from ovos_spec_tools import SpecMessage
+from ovos_bus_client.session import Session, UtteranceState
 from ovos_utils.fakebus import FakeBus
 from ovos_workshop.permissions import ConverseMode, ConverseActivationMode
 
@@ -225,27 +224,31 @@ def test_skills_within_default_timeout_stay(self):
         svc = _make_service()
         sess = Session("s")
         now = time.time()
-        sess.active_skills = [("skill_a", now - 10)]  # 10 s ago — within 300 s default
+        sess.active_handlers = [
+            {"skill_id": "skill_a", "activated_at": now - 10}
+        ]  # 10 s ago — within 300 s default
 
         with patch("ovos_core.intent_services.converse_service.SessionManager.get",
                    return_value=sess):
             svc._check_converse_timeout(Message("test"))
 
-        self.assertEqual(len(sess.active_skills), 1)
-        self.assertEqual(sess.active_skills[0][0], "skill_a")
+        self.assertEqual(len(sess.active_handlers), 1)
+        self.assertEqual(sess.active_handlers[0]["skill_id"], "skill_a")
 
     def test_skills_past_default_timeout_removed(self):
         """Skills older than the default timeout (300 s) are removed."""
         svc = _make_service()
         sess = Session("s")
         now = time.time()
-        sess.active_skills = [("old_skill", now - 400)]  # 400 s ago — beyond default
+        sess.active_handlers = [
+            {"skill_id": "old_skill", "activated_at": now - 400}
+        ]  # 400 s ago — beyond default
 
         with patch("ovos_core.intent_services.converse_service.SessionManager.get",
                    return_value=sess):
             svc._check_converse_timeout(Message("test"))
 
-        self.assertEqual(sess.active_skills, [])
+        self.assertEqual(sess.active_handlers, [])
 
     def test_per_skill_timeout_override_respected(self):
         """A per-skill timeout override takes precedence over the default."""
@@ -254,17 +257,65 @@ def test_per_skill_timeout_override_respected(self):
         sess = Session("s")
         now = time.time()
         # short_skill has a 5-second timeout; 10 seconds old → should be removed
-        sess.active_skills = [("short_skill", now - 10), ("long_skill", now - 10)]
+        sess.active_handlers = [
+            {"skill_id": "short_skill", "activated_at": now - 10},
+            {"skill_id": "long_skill", "activated_at": now - 10},
+        ]
 
         with patch("ovos_core.intent_services.converse_service.SessionManager.get",
                    return_value=sess):
             svc._check_converse_timeout(Message("test"))
 
-        remaining = [s[0] for s in sess.active_skills]
+        remaining = [handler["skill_id"] for handler in sess.active_handlers]
         self.assertNotIn("short_skill", remaining)
         self.assertIn("long_skill", remaining)
 
 
+# ---------------------------------------------------------------------------
+# active_skills compatibility property
+# ---------------------------------------------------------------------------
+
+class TestActiveSkillsCompatibility(unittest.TestCase):
+    """The legacy property remains a lossless view of canonical handlers."""
+
+    def test_getter_preserves_tuple_shape_order_and_timestamps(self):
+        svc = _make_service()
+        sess = Session("s")
+        sess.active_handlers = [
+            {"skill_id": "skill_a", "activated_at": 12.5},
+            {"skill_id": "skill_b", "activated_at": 7.25},
+        ]
+
+        with patch(
+            "ovos_core.intent_services.converse_service.SessionManager.get",
+            return_value=sess,
+        ):
+            active_skills = svc.active_skills
+
+        self.assertEqual(active_skills, [("skill_a", 12.5), ("skill_b", 7.25)])
+        self.assertTrue(all(isinstance(item, tuple) for item in active_skills))
+
+    def test_setter_preserves_order_and_original_timestamps(self):
+        svc = _make_service()
+        sess = Session("s")
+        legacy_skills = [("skill_a", 12.5), ("skill_b", 7.25)]
+
+        with patch(
+            "ovos_core.intent_services.converse_service.SessionManager.get",
+            return_value=sess,
+        ), patch.object(sess, "activate_skill") as activate_skill:
+            svc.active_skills = legacy_skills
+
+        activate_skill.assert_not_called()
+        self.assertEqual(
+            sess.active_handlers,
+            [
+                {"skill_id": "skill_a", "activated_at": 12.5},
+                {"skill_id": "skill_b", "activated_at": 7.25},
+            ],
+        )
+
+
 # ---------------------------------------------------------------------------
 # _activate_allowed / activate_skill
 # ---------------------------------------------------------------------------
@@ -510,6 +561,52 @@ def test_whitelist_mode_allows_whitelisted(self):
 class TestMatch(unittest.TestCase):
     """Tests for the top-level match() pipeline method."""
 
+    def test_message_session_is_folded_once_and_reused(self):
+        """One matcher call must operate on one live session snapshot."""
+        svc = _make_service()
+        sess = Session("s")
+        message = Message("test", context={})
+
+        with patch(
+            "ovos_core.intent_services.converse_service.SessionManager.get",
+            return_value=sess,
+        ) as get_session, patch.object(
+            svc, "_check_converse_timeout"
+        ) as check_timeout, patch.object(
+            svc, "_collect_converse_skills", return_value=[]
+        ) as collect_skills:
+            result = svc.match(["hello"], "en-US", message)
+
+        self.assertIsNone(result)
+        get_session.assert_called_once_with(message)
+        check_timeout.assert_called_once_with(message, sess)
+        collect_skills.assert_called_once_with(message, sess)
+
+    def test_expired_skill_is_not_refolded_before_poll(self):
+        """Timeout filtering survives until converse candidates are polled."""
+        svc = _make_service()
+        sess = Session("s")
+        sess.active_handlers = [
+            {"skill_id": "expired", "activated_at": time.time() - 400}
+        ]
+        message = Message("test", context={"session": sess.serialize()})
+
+        with patch(
+            "ovos_core.intent_services.converse_service.SessionManager.get",
+            return_value=sess,
+        ) as get_session, patch.object(
+            svc, "_collect_converse_skills", return_value=[]
+        ) as collect_skills:
+            result = svc.match(["hello"], "en-US", message)
+
+        self.assertIsNone(result)
+        self.assertEqual(sess.active_handlers, [])
+        self.assertEqual(
+            message.context["session"].get("active_handlers", []), []
+        )
+        get_session.assert_called_once_with(message)
+        collect_skills.assert_called_once_with(message, sess)
+
     def test_skill_in_response_state_captured_by_get_response(self):
         """A skill in RESPONSE state is matched as get_response, not converse."""
         svc = _make_service()