fix: registry-first session resolution for stop write paths (mirrors #858) - #864
Closed
JarbasAl wants to merge 3 commits into
Closed
fix: registry-first session resolution for stop write paths (mirrors #858)#864JarbasAl wants to merge 3 commits into
JarbasAl wants to merge 3 commits into
Conversation
…p ping/pong (PIPELINE-1 §9.1.1) Applies the same round-correlation guard added to converse_service's handle_ack in #859 to the remaining poll rounds: fallback_service's ovos.skills.fallback.pong collector and stop_service's skill.stop.pong collector now discard pongs whose utterance_id or session mismatches the open round, standing down when the round carries no utterance_id (V0 back-compat). This closes the same late-answer-wins-wrong-round class of bug for fallback and stop, mirroring converse's fix. Note: common_query.py's phrase-string correlation (also flagged in the originating task) does not live in ovos-core -- that logic is in the separate ovos-common-query-pipeline-plugin repo and is out of scope here; left untouched. > 🤖 Auto-generated by Claude Sonnet 5 (claude-sonnet-5) via Claude Code — NOT human-reviewed. Verify before acting. Verified: new tests exercise the actual FallbackService/StopService ping-pong collectors against a FakeBus; red-before confirmed by reverting the source guard (test files kept) and re-running the new test classes, which failed exactly as expected; green after reapplying. Full unit suite (350 tests) passes on top of the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…858) Applies #858's fold-order discipline (ConverseService's _registry_session_for_write, not yet merged - reviewed at its PR head fix/registry-first-converse) to stop_service.py's 5 SessionManager.get call sites. Site-by-site classification: - get_active_skills (static) - read-only, no write - unchanged, but now accepts an optional `session=` param so callers already holding a resolved session don't re-fold (mirrors converse's chain-threading, case 4). - _collect_stop_skills - read-only (blacklisted_skills, and the #862 round-guard's session_id) - unchanged fold, but now accepts and threads `session=` from its caller instead of re-resolving, to avoid undoing a caller's registry-first write. - handle_stop_confirmation - read-only (utterance_states, is_active, is_speaking) - unchanged plain fold. - match_high / match_low - each does an incidental write with no wire echo (`sess.disable_response_mode(skill_id)`), reached by a message shared across every pipeline stage for this turn - switched to the new shared `registry_session_for_write` helper (ovos_core/intent_services/_session_fold.py) and thread that resolved session through their own get_active_skills/_collect_stop_skills calls so nothing re-folds and undoes the write. fallback_service.py's 2 SessionManager.get sites (_collect_fallback_skills, _fallback_range) were audited and are both read-only - no write ever happens through either `sess` reference - so neither needs the registry-first bypass. Left unchanged; this is a real finding, not skipped work: the write-path fix only applies where a write with no wire echo actually exists. > 🤖 Auto-generated by Claude Sonnet 5 (claude-sonnet-5) via Claude Code — NOT human-reviewed. Verify before acting. Verified: new TestFoldOrderRegistryFirstWrite tests drive match_high/ match_low against the real SessionManager registry with a message whose own session snapshot is stale relative to an incidental write already present on the live registry entry, and assert that write survives. Red-before confirmed by reverting only stop_service.py (patch-based, no git stash) and re-running the two new tests, which failed exactly as expected; reapplied and green. Full unit suite (352 tests) passes on top of this branch; test_stop_service.py alone is stable across 3 consecutive full-suite runs (one flaky failure seen once under full- suite load traced to timing in the pre-existing threaded ping/pong test pattern from #862/#859, unrelated to this change). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 |
Adversarial review found stop_service.py's handle_stop_confirmation and fallback_service.py's _collect_fallback_skills were misclassified as "read-only" in this PR's own site-by-site table. Neither echoes the resolved session onto the wire and neither is lifecycle entry, so their plain SessionManager.get(message) fold could full-replace the live registry entry with a stale message snapshot - undoing match_high/ match_low's disable_response_mode write and resurrecting deactivated skills (handle_stop_confirmation), or clobbering an earlier registry-first write made the same turn (_collect_fallback_skills). Both now resolve via this PR's own registry_session_for_write helper. fallback_service._fallback_range keeps its plain fold: its updated_session=sess is consumed by IntentService._dispatch_match (service.py:446) and stamped onto the outgoing message - a genuine wire-echo site, now documented as such instead of "n/a". Adds regression tests for both fixed sites, verified to fail against the unfixed source (patch-revert, not git stash) and pass after. Also adds registry isolation (setUp/tearDown popping session id "s") to test classes whose sessions self-register into the live SessionManager registry via Session.touch(), which the registry-first lookup now makes order-sensitive across the test file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JarbasAl
force-pushed
the
feat/utterance-id-consumers
branch
from
August 14, 2026 12:17
5c18560 to
8c2eb87
Compare
JarbasAl
added a commit
that referenced
this pull request
Aug 14, 2026
…ll (#881) Standing rule: every repo carries a version-stamped, newest-first log of what changed since the last stable release, reset at each stable. This adds it for ovos-core (since 2.1.1). Filled a docs gap along the way: converse-fallback.md described only the sequential per-skill converse request, not the OVOS-CONVERSE-1 broadcast contest poll added in #863. > 🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting. > Verified against current source: context-lock fix (#868), _LegacyStopBridge > presence and its removal note, registry-first session fold (#858), > converse broadcast poll (#863), round-correlation guard (#862). #864 and > #879 confirmed open/unmerged via gh pr view before being listed as > pending. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closed as superseded. The two write-path defects this PR fixed no longer exist on dev: the STOP-1 rewrite (#802) plus the PreDrainSnapshot refactor key pre-drain state per (session_id, skill_id) and pop it in the confirmation handler, so a stale
.stop.responsecan no longer fold an old session over the registry and revive a deactivated skill; session writes now travel exclusively throughMatch.updated_session. The remainingSessionManager.get(message)sites in the stop and fallback services are read-only (blacklist and fallback-range filtering), which is exactly the intended use. The round-guard commit this branch carried also landed independently as #862. Nothing here is left to merge.