perf: reuse session during converse matching - #846
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConverse state now uses ChangesConverse session reuse
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Match
participant Session
participant TimeoutCheck
participant SkillCollector
participant ConverseBus
Match->>Session: load session once
Match->>TimeoutCheck: filter active handlers using session
Match->>SkillCollector: collect converse skills using session
SkillCollector->>ConverseBus: poll converse candidates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ovos_core/intent_services/converse_service.py`:
- Around line 348-352: Update the _check_converse_timeout call path so the
filtered live session is serialized back into message.context["session"] before
message.forward() emits converse pings. Ensure _handle_activate cannot re-fold
the original snapshot and restore expired handlers, while preserving the
existing filtering in session.active_handlers and downstream
_collect_converse_skills behavior.
- Around line 100-109: Update the active_skills property getter to return
immutable (skill_id, activated_at) tuples, and change its setter to rebuild
session.active_handlers directly from the supplied tuples without calling
Session.activate_skill. Preserve each original activated_at value and the input
ordering exactly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: adc95211-cb01-47ab-a4c7-d36602841fa1
📒 Files selected for processing (2)
ovos_core/intent_services/converse_service.pytest/unittests/test_converse_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unittests/test_converse_service.py
Summary
Architecture
This is an internal state-reuse refactor. It does not skip converse, reorder intent pipelines, change message topics, alter public protocol behavior, or add traffic-manipulation hooks. A message snapshot is folded into the canonical live Session once; every stage in that matcher invocation then uses that same object.
Besides the latency reduction, this preserves timeout filtering. Previously, a later SessionManager.get(message) could refold the original wire snapshot after an expired skill had been removed, restoring stale state before polling.
Canary evidence
The exact-source 32-process canary compared three 400-client runs before and after this head. Every run delivered 400/400 semantically verified skill replies; persona and LLM escalation were disabled.
Stable observations (discarding the first post-rollout noisy run from both versions):
The canary image is composed only from explicit review heads, publishes SBOM/provenance, and verifies the single-fold source shape at build time.
Validation
A local all-in-one test process reported four order-dependent no-skill subtest failures after 360 passes; the same no-skill file passes in a clean process (2 tests and 4 subtests), and the official isolated Ovoscope job passes. This is retained here for transparent reproducibility rather than attributed to this change.
Summary by CodeRabbit
Bug Fixes
Tests