fix(openviking): harden session switching and writes by ehz0ah · Pull Request #47662 · NousResearch/hermes-agent

ehz0ah · 2026-06-17T05:29:31Z

What does this PR do?

This PR restores and hardens OpenViking session lifecycle handling when Hermes switches sessions. It is a cleaned-up continuation of #28445, with the original author's session-switch commits preserved via cherry-pick attribution, plus follow-up write-safety hardening relocated into this focused PR.

The core bug is that OpenVikingMemoryProvider cached the session id from initialize(). When Hermes later rotated sessions via /new, /branch, /reset, /resume, or context compression, the provider could keep writing turns into the old session and then commit the wrong boundary. That meant the new OpenViking session might never receive messages or trigger memory extraction.

This branch keeps the fix scoped to the OpenViking provider and tests.

Related Issue

Fixes #28296

Related: #28445

Type of Change

Bug fix (non-breaking change that fixes an issue)
Tests (adding or improving test coverage)

Changes Made

Adds OpenVikingMemoryProvider.on_session_switch() so the provider commits the old session, clears stale prefetch state, and rotates to the new session id.
Treats same-session rewinds such as /undo as cache invalidation only: no commit, no session rotation, and no local turn-count reset.
Tracks in-flight session writers per session id so commit waits for every writer targeting the session being finalized.
Snapshots the target session id before async writes begin, preventing late writers from landing in a newly-rotated session.
Writes each Hermes turn to OpenViking as a single ordered /api/v1/sessions/{sid}/messages/batch request using canonical parts payloads.
Retries a failed async session write once with a fresh OpenViking client.
Preserves explicit memory writes through /api/v1/content/write instead of converting them into session messages.
Commits sessions with server-side pending_tokens even when the local turn counter is already clean.
Tracks successfully committed session ids with a locked set so a follow-up switch does not double-commit a session that was already committed by on_session_end() or a deferred finalizer.
Defers old-session commit after a switch if the initial writer drain times out, so late old-session writes can still be extracted after they finish.
Sends limit to OpenViking search instead of legacy top_k, including background prefetch searches.
Updates the older OpenViking plugin tests to match the current limit payload, per-session writer tracking, and batch session-message payloads.
Adds focused coverage in tests/plugins/memory/test_openviking_provider.py for session switching, stale prefetch handling, writer drain behavior, batch payload shape, retry behavior, pending-token commits, same-session rewinds, and content/write preservation.
Hardens env-sensitive OpenViking client tests so local OPENVIKING_ACCOUNT, OPENVIKING_USER, or OPENVIKING_AGENT values do not leak into default-header assertions.

How to Test

Targeted provider validation:

scripts/run_tests.sh tests/plugins/memory/test_openviking_provider.py -- -o addopts=

Result:

48 passed, 0 failed

CI-slice OpenViking plugin validation:

scripts/run_tests.sh tests/openviking_plugin/test_openviking.py -- -o addopts=

Result:

22 passed, 0 failed

Combined OpenViking and /undo contract validation:

scripts/run_tests.sh tests/openviking_plugin/test_openviking.py tests/tui_gateway/test_undo_command.py tests/plugins/memory/test_openviking_provider.py -- -o addopts=

Result:

80 passed, 0 failed

Python lint on updated CI-slice test:

UV_NATIVE_TLS=true uv run ruff check tests/openviking_plugin/test_openviking.py

Result:

All checks passed!

Reviewer Notes

The first four commits are cherry-picks from #28445 with -x attribution preserved. The follow-up commits adapt the implementation to current main and the current OpenViking server contract.

I intentionally kept this PR limited to session-switching and the write-safety behavior needed to make that lifecycle correct.

One remaining tradeoff is that session-write retry is still at-least-once: if the server persists a batch and the client only fails while reading the response, the retry can duplicate that turn. The batch endpoint narrows the partial-write window, but exact-once behavior would need an OpenViking idempotency key or stable message id contract.

Checklist

Code

I have read the Contributing Guide.
My commit messages follow Conventional Commits.
I searched for existing PRs and found the related work listed above.
This PR contains only changes related to the OpenViking session-switch/write-safety fix.
I ran pytest tests/ -q and all tests pass. Not run locally; this repo is large. I ran the focused provider, plugin, and /undo suites above.
I added tests for the bug fix.
I tested on macOS.

Documentation & Housekeeping

Documentation update: N/A, provider behavior only.
cli-config.yaml.example update: N/A, no config keys added.
CONTRIBUTING.md / AGENTS.md update: N/A, no workflow changes.
Cross-platform impact considered: this uses standard threading/HTTP-client logic and does not add platform-specific behavior.
Tool descriptions/schemas update: N/A, no public tool schema changes.

OpenVikingMemoryProvider only overrides on_session_end and inherits the base-class no-op for on_session_switch. When the agent rotates session_id (via /new, /branch, /reset, /resume, or context compression), the provider's cached _session_id stays at the value initialize() captured. All subsequent sync_turn writes then land in the already-closed old session, and on_session_end tries to commit it a second time — the new session never accumulates messages and never triggers memory extraction. The fix mirrors the pattern Hindsight uses (NousResearch#17508): 1. Wait for any in-flight sync thread to drain under the OLD _session_id before we mutate it, otherwise the commit below races the last message write. 2. Commit the old session if it accumulated turns — same extraction semantics as on_session_end. Skip if empty (nothing to extract). 3. Drain in-flight prefetch from the old session and clear its cached result so the new session doesn't see stale recall. 4. Rotate _session_id to the new value and reset _turn_count. Commit failures are swallowed (logged at WARN) so a flaky server can't strand the provider on the old session forever — same posture as the existing on_session_end commit. (cherry picked from commit a1e7185)

…sion_end Two hardening fixes prompted by review on NousResearch#28296: 1. sync_turn() now snapshots the target session id before spawning the worker. The previous code read self._session_id inside the worker, so a worker delayed past on_session_switch's bounded join could read the rotated-in NEW id and write the OLD turn's messages into the wrong session. 2. on_session_end() resets _turn_count to 0 after a successful commit, making the old-session commit path idempotent with the new switch hook. /new and compression call commit_memory_session() (which fires on_session_end) immediately before on_session_switch; without this, the old session would be committed twice. On commit failure we leave _turn_count > 0 so on_session_switch retries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 2ea8d5c)

Three follow-ups from review on NousResearch#28296: 1. Sync worker outliving the bounded join. Each sync_turn POST has _TIMEOUT=30s and there are two per turn, but on_session_end and on_session_switch only join for 10s. If the worker is still alive after the join, committing the old session orphans the worker's late writes past the commit boundary — they land in an already- committed session and never get extracted. Both hooks now re-check is_alive() after the join and skip the commit when the worker hasn't drained. 2. on_memory_write late session_id capture. Same shape as the pre-fix sync_turn: f-string for the post path read self._session_id inside the worker, so a switch between thread spawn and post call landed the memory note in the new session. Snapshot sid at call time, same pattern as sync_turn. 3. Stale prefetch repopulating the new session. The pre-switch drain+clear only protects against workers that finish before the join completes; one finishing after the clear would write its result into the new generation's slot. Added a monotonic _prefetch_generation; workers capture it at spawn and refuse to write if it has advanced. Tests: existing in-flight-sync test updated to drain (it tested the join-before-commit happy path); four new tests cover hung-writer skip on end + switch, on_memory_write sid capture, and prefetch generation gating. 177/177 memory tests pass. (cherry picked from commit 3791a87)

sync_turn's bounded join could drop a still-alive previous worker by replacing the single _sync_thread slot. The dropped worker kept POSTing under the old sid but was no longer visible to on_session_end / on_session_switch, so the commit could fire while orphaned writes were still in flight — those writes landed past the commit boundary and were never extracted. Replace the single _sync_thread slot with _inflight_writers: Dict[sid, Set[Thread]]. Writers self-register on spawn (sync_turn, on_memory_write) and self-deregister on exit. The commit path drains _drain_writers(sid, 10.0) and skips the commit if any writer for that sid is still alive after the bounded budget. Also trim inline review-rationale comments to short invariants per reviewer style ask: "commit only after session writes drain" and "drop prefetch results from older switch generations." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 7537ee6)

kshitijk4poor · 2026-06-17T21:00:10Z

Thank you for this — and for preserving @harshitAgr's commits via cherry-pick. #28296 is a real, still-open bug, and your session-switch hook, per-session writer tracking, and /undo (rewound) handling are the right design.

Salvaged into #48042 with your and @harshitAgr's commits cherry-picked (authorship preserved) and rebased onto current main — the test file had conflicted with #47973, which merged after you opened this. On top I added one hardening commit addressing a review of the threading:

on_session_switch was running the old-session writer-drain + pending-token GET + commit POST inline on the caller's thread. /new, /branch, /resume, /undo call it synchronously on the command thread, so a slow drain (up to 10s) or wedged commit could stall the user-facing command — the same hazard fix(memory): run end-of-turn sync off the turn thread (agent stuck 'running') #41945 fixed for end-of-turn sync. It now rotates session state synchronously and offloads the old-session commit to a daemon finalizer.
Locked the cross-thread (_session_id, _turn_count) pair (sync_turn runs on the memory executor thread, the session hooks on the command thread).
Ordered the committed-session guard ahead of the turn_count>0 shortcut in _session_needs_commit to close a double-commit window.
Added a _shutting_down flag + prefetch-thread tracking so finalizers don't POST against a torn-down client and invalidate/shutdown join every prefetch thread.

95 tests pass. Closing in favor of #48042 — your authorship is intact there. Happy to hear any pushback on the threading changes.

fix(openviking): implement on_session_switch hook + harden session writes (salvage #47662)

Resolves conflicts from the OpenViking churn that merged after #32445 was opened (#48042/#47662 session-switch + write hardening, #47311/#47973): - plugins/memory/openviking/__init__.py: keep both __init__ field groups (the PR's _runtime_start_* alongside main's _prefetch_threads/_shutting_down). - tests/plugins/memory/test_openviking_provider.py: keep BOTH the PR's new setup-validation tests and main's session-switch/concurrency tests (disjoint additions to the same region). Two fixes layered while reconciling (contributor work otherwise preserved): - Restore the merged tenant-header contract (#22414/#21232). The PR had changed _VikingClient defaults to '' and made empty account/user OMIT the tenant headers; main's contract is that empty falls back to 'default' and the X-OpenViking-Account/User headers are ALWAYS sent (ROOT API keys need them). Reverted the constructor to 'account or os.environ.get(..., "default")' and updated the two PR tests that asserted the omit-when-empty behavior. - Close a secret-file TOCTOU in the setup writers. _write_env_vars and _write_ovcli_config wrote the api_key/root_api_key file and chmod 0600 AFTERWARD, leaving a world-readable window on newly-created files. Added _precreate_secret_file() to create with 0600 before any secret bytes land.

fix(openviking): implement on_session_switch hook + harden session writes (salvage NousResearch#47662)

) Resolves conflicts from the OpenViking churn that merged after NousResearch#32445 was opened (NousResearch#48042/NousResearch#47662 session-switch + write hardening, NousResearch#47311/NousResearch#47973): - plugins/memory/openviking/__init__.py: keep both __init__ field groups (the PR's _runtime_start_* alongside main's _prefetch_threads/_shutting_down). - tests/plugins/memory/test_openviking_provider.py: keep BOTH the PR's new setup-validation tests and main's session-switch/concurrency tests (disjoint additions to the same region). Two fixes layered while reconciling (contributor work otherwise preserved): - Restore the merged tenant-header contract (NousResearch#22414/NousResearch#21232). The PR had changed _VikingClient defaults to '' and made empty account/user OMIT the tenant headers; main's contract is that empty falls back to 'default' and the X-OpenViking-Account/User headers are ALWAYS sent (ROOT API keys need them). Reverted the constructor to 'account or os.environ.get(..., "default")' and updated the two PR tests that asserted the omit-when-empty behavior. - Close a secret-file TOCTOU in the setup writers. _write_env_vars and _write_ovcli_config wrote the api_key/root_api_key file and chmod 0600 AFTERWARD, leaving a world-readable window on newly-created files. Added _precreate_secret_file() to create with 0600 before any secret bytes land.