From d1e5c3dc33ef0d43d021662674e1a7cd5e43eecd Mon Sep 17 00:00:00 2001 From: Jeff Mettel Date: Thu, 30 Jul 2026 16:33:39 -0500 Subject: [PATCH] fix(openviking): re-arm the commit guard after in-place compression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_committed_session_ids` is a permanent per-sid latch, and `_session_needs_commit` checks it before the turn counter by design — a racing sync_turn can re-increment `_turn_count` after commit+reset, so the guard must win to stop a double-commit. That is correct for a session being left behind. It is wrong for one that keeps its id. `compress_context()` commits before rewriting the transcript in both modes, and with `compression.in_place: true` (the default) `on_session_switch` receives the same id and does not rotate. The latch then rejects every later commit for a still-live session — the next compression, /new, normal session end, startup recovery — so every post-compression turn is silently never extracted. Rotation mode is unaffected because a fresh child id is minted and starts clean, which is what confirms the latch's intent was only ever to dedupe the departing id. Clear the latch when compression completes without rotation. Turns arriving after that point are genuinely new, and this is a defined moment rather than a race. The rotation path is untouched, so the old id stays latched and its _finalize_session_async still dedupes against the compression commit. Fixes #74695 Co-Authored-By: Claude Opus 5 (1M context) --- plugins/memory/openviking/__init__.py | 26 +++++++++ .../memory/test_openviking_provider.py | 53 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index e59f9467ac236..c732de975cac7 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -2716,6 +2716,18 @@ class OpenVikingMemoryProvider(MemoryProvider): with self._committed_session_lock: self._committed_session_ids.add(sid) + def _clear_session_committed(self, sid: str) -> None: + """Re-arm the commit guard for a session that is still live. + + A permanent per-sid latch is correct for a session being left behind: + it dedupes that id's ``_finalize_session_async`` against the commit + compression already performed. In-place compression keeps the *same* + id, so the latch would otherwise reject every later commit for a + session that is still accumulating turns (#74695). + """ + with self._committed_session_lock: + self._committed_session_ids.discard(sid) + def _pending_session_dir(self) -> Optional[Path]: if not self._hermes_home: return None @@ -4182,6 +4194,20 @@ class OpenVikingMemoryProvider(MemoryProvider): self._profile_prefetched_sessions.discard(old_session_id) self._profile_prefetched_sessions.discard(new_id) + if not rotate and old_session_id: + # In-place compression (the default) keeps the same session id. + # compress_context() has just committed it, latching the guard — + # but the session is still live, so every later commit for it + # (the next compression, /new, normal session end, startup + # recovery) would be rejected and post-compression turns would + # never be extracted. Re-arm the guard now that compression has + # finished; turns arriving after this point are genuinely new. + # + # Rotation mode is untouched: there a fresh child id is minted + # and the old id stays latched, which is what dedupes its + # _finalize_session_async against this same commit. + self._clear_session_committed(old_session_id) + if not rotate: # Same-session rewind (/undo) or no-op rotation: no commit and no # counter reset. diff --git a/tests/plugins/memory/test_openviking_provider.py b/tests/plugins/memory/test_openviking_provider.py index 2b8d92e61d84b..709ffd5aaec7e 100644 --- a/tests/plugins/memory/test_openviking_provider.py +++ b/tests/plugins/memory/test_openviking_provider.py @@ -993,3 +993,56 @@ def test_prefetch_sends_contract_safe_memory_context_payload(monkeypatch): assert "target_uri" not in payload + + +def test_in_place_compression_rearms_commit_guard(): + """Post-compression turns must still be committable (#74695). + + ``compress_context()`` commits before rewriting the transcript, which + latches the per-sid guard. In-place mode (the default) keeps the SAME sid, + so the latch then rejected every later commit for a still-live session — + the next compression, /new, normal session end and startup recovery all + silently did nothing, and post-compression turns were never extracted. + """ + provider = _make_provider_with_session("sid-123", turn_count=4) + provider._ensure_client = lambda: True + + # Compression commits the live session, latching the guard. + provider._mark_session_committed("sid-123") + assert provider._session_needs_commit("sid-123", 4) is False + + # In-place compression: same id in, no rotation. + provider.on_session_switch("sid-123", reason="compression") + + # The session is still live, so new turns must be committable again. + assert provider._has_committed_session("sid-123") is False + assert provider._session_needs_commit("sid-123", 2) is True + + +def test_rotating_compression_keeps_old_session_latched(): + """Rotation mode must keep the guard, which dedupes the old id's finalize. + + With ``compression.in_place: false`` a fresh child id is minted. The old id + stays committed so its ``_finalize_session_async`` does not double-commit + what compression already committed — the behavior the guard exists for. + """ + provider = _make_provider_with_session("old-sid", turn_count=4) + provider._ensure_client = lambda: True + provider._finalize_session_async = MagicMock() + + provider._mark_session_committed("old-sid") + provider.on_session_switch("new-sid", reason="compression") + + assert provider._has_committed_session("old-sid") is True + assert provider._session_needs_commit("old-sid", 4) is False + + +def test_undo_rewind_does_not_rearm_commit_guard(): + """Only compression re-arms; a same-session /undo must not.""" + provider = _make_provider_with_session("sid-123", turn_count=4) + provider._ensure_client = lambda: True + + provider._mark_session_committed("sid-123") + provider.on_session_switch("sid-123", rewound=True) + + assert provider._has_committed_session("sid-123") is True