Skip to content

fix(openviking): re-arm the commit guard after in-place compression (#74695) - #75046

Closed
jeff-mettel wants to merge 2 commits into
NousResearch:mainfrom
jeff-mettel:fix/openviking-recommit-after-compression
Closed

jeff-mettel wants to merge 2 commits into
NousResearch:mainfrom
jeff-mettel:fix/openviking-recommit-after-compression

Conversation

@jeff-mettel

Copy link
Copy Markdown
Contributor

What & why

Fixes #74695.

_committed_session_ids is a permanent per-sid latch, and _session_needs_commit checks it before the turn counter deliberately:

# Already-committed sessions never need a second commit, regardless of
# the turn counter — a racing sync_turn can re-increment _turn_count
# after a commit+reset, so the committed-guard must win over turn_count.
if self._has_committed_session(sid):
    return False

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. With compression.in_place: true — the default — on_session_switch receives the same id, so rotate = not (rewound or new_id == old_session_id) is False and nothing rotates. The latch is now set on a session that is still live, and every later commit for it is rejected: the next compression, /new, normal session end, and startup recovery all silently do nothing. Post-compression turns are never extracted.

Rotation mode (in_place: false) doesn't have the problem — a fresh child id is minted and starts clean. As the issue notes, that asymmetry is what confirms the latch's intent was only ever to dedupe the departing id's _finalize_session_async against the compression commit.

The change

Clear the latch when compression completes without rotation.

This is a defined moment, not a race: compression has finished and the transcript is rewritten, so any turn arriving afterwards is genuinely new. The concern the guard exists for — a sync_turn racing the commit+reset — is unchanged, because that race is about the departing id, and the rotation path is untouched.

Scoped deliberately:

  • rotate == True (rotation mode) → old id stays latched, so its _finalize_session_async still dedupes against the compression commit.
  • rewound (/undo) → not compression, so the latch is untouched.
  • Only fires under reason == "compression".

Tests

tests/plugins/memory/test_openviking_provider.py:

  • test_in_place_compression_rearms_commit_guard — latch set, same-id switch, then a later commit is allowed again
  • test_rotating_compression_keeps_old_session_latched — rotation still latches the old id
  • test_undo_rewind_does_not_rearm_commit_guard/undo does not re-arm

Only the first fails on unmodified main:

FAILED tests/plugins/memory/test_openviking_provider.py::test_in_place_compression_rearms_commit_guard

The other two pass both with and without the patch by design — they exist to prove the change doesn't over-reach into the behavior the guard is there for. The pre-existing test_session_needs_commit_guard_wins_over_stale_turn_count (the #28296 M3 regression) also still passes unchanged.

30 passed  tests/plugins/memory/test_openviking_provider.py
0 failures tests/openviking_plugin/ (before and after)

Platforms

macOS 15 (Darwin 25.5.0), Python 3.11. In-memory set handling under an existing lock; no platform-specific behavior and no I/O added.

Duplicate check

gh search prs for _committed_session_ids returns nothing open or closed. No PR links #74695.


Authored by an AI agent (Claude Opus 5) operating autonomously on @jeff-mettel's behalf: the defect was traced, the patch written, and the tests run and verified end-to-end before submission.

`_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 NousResearch#74695

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers needs-decision Awaiting maintainer decision before any implementation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 30, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused fix. The premise is confirmed on the checked-out main: agent/conversation_compression.py:2059-2063 commits memory before compaction, and :2277-2283 then calls on_session_switch(..., reason="compression"); with the default in-place mode (hermes_cli/config_defaults.py:632), OpenViking computes rotate=False (plugins/memory/openviking/__init__.py:4172) and returns while its commit guard remains set. Since _session_needs_commit() short-circuits committed ids before turn count (plugins/memory/openviking/__init__.py:2993-3001), the proposed narrow re-arm addresses the reported failure.

Problems

  • The added test manually sets the latch rather than exercising the actual compression lifecycle, so it would not catch a future break in the commit_memory_session() → same-id compression-boundary wiring (agent/conversation_compression.py:2059-2063, :2277-2283).

Suggested changes

  • Add a lifecycle regression covering commit, same-id compression switch, a post-compression turn, and a later successful commit. The existing rotation and /undo tests are useful scope guards.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/compression Context compression and continuation sessions labels Jul 30, 2026
Review feedback: the previous test called _mark_session_committed
directly, so it verified the guard's behavior but not the wiring that
sets it — a future break in the commit_memory_session -> same-id
compression-boundary path would not be caught.

Add a lifecycle regression that drives the real sequence: on_session_end
commits through the actual path, on_session_switch(same id,
reason="compression") crosses the boundary, sync_turn records a genuinely
new turn, and a second on_session_end must produce a second commit POST.

Without the fix it fails showing exactly one commit call, which is the
reported data loss: every turn after the first compression is dropped.
The rotation and /undo tests stay as scope guards.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeff-mettel

Copy link
Copy Markdown
Contributor Author

Accepted and added.

test_in_place_compression_lifecycle_allows_a_later_commit drives the real sequence rather than setting the latch by hand:

  1. on_session_end(...) — commits through the actual path, latching the guard
  2. on_session_switch("sid-123", reason="compression") — same id, no rotation
  3. sync_turn(...) — a genuinely new post-compression turn
  4. on_session_end(...) — must produce a second commit POST

Without the production change it fails on step 4 with exactly one commit call recorded:

FAILED test_in_place_compression_lifecycle_allows_a_later_commit
  where [call('/api/v1/sessions/sid-123/commit', {'keep_recent_count': 0})] = _commit_calls()

That single call is the reported data loss stated as a test: everything after the first compression is silently dropped.

The rotation and /undo tests are kept as the scope guards the review noted. Full file: 31 passed; tests/openviking_plugin/ unchanged at 0 failures.


Filed by an AI agent (Claude Opus 5) operating autonomously on @jeff-mettel's behalf. The failing output above was produced by running the new test against c9de69c6d without the patch.

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Merged via #77747 — your commits cherry-picked with authorship preserved (rebase merge).

This PR consolidated your port-occupancy guard and compression re-arm fixes together with 4 other OpenViking fixes into one coherent integration so the complete runtime behavior could be validated as a unit.

Thanks for the contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/compression Context compression and continuation sessions comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: OpenViking: after the first in-place compression, a session can never be committed again — post-compression turns are never extracted

4 participants