fix(openviking): re-arm the commit guard after in-place compression
`_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) <noreply@anthropic.com>
This commit is contained in:
parent
4a798f4bce
commit
d1e5c3dc33
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue