Skip to content

feat: Add OpenViking auto-commit feature for early memory searchability - #8235

Closed
happy5318 wants to merge 2 commits into
NousResearch:mainfrom
happy5318:feat/openviking-auto-commit
Closed

feat: Add OpenViking auto-commit feature for early memory searchability#8235
happy5318 wants to merge 2 commits into
NousResearch:mainfrom
happy5318:feat/openviking-auto-commit

Conversation

@happy5318

Copy link
Copy Markdown
Contributor

Description

Adds automatic commit functionality for OpenViking memory provider, solving the pain point that users have to wait for 2-hour session timeout to search their conversation history.

Changes

  1. CLI: Trigger when executing or commands, so the current session is committed immediately to OpenViking
  2. Gateway: Add background idle commit watcher that checks every 60 seconds, commits sessions that have been idle for 120 seconds (configurable)
  3. Session: Add field to avoid duplicate commits, reset flag when new message arrives

Compatibility

  • ✅ Fully backward compatible: No impact on users not using OpenViking memory provider
  • ✅ Exception safe: All commit logic wrapped in try-except blocks, won't break main conversation flow even if OpenViking is unavailable
  • ✅ Cross-platform: Works with all messaging platforms (Feishu, Discord, Telegram, CLI)

Usage

After enabling OpenViking memory provider, users can search their conversation history:

  • Immediately after executing or
  • Automatically 2 minutes after stopping conversation
  • Or after 2-hour session timeout (existing logic)

1. CLI: Trigger commit on /new /reset commands\n2. Gateway: Add 2-minute idle auto-commit watcher\n3. Session: Add memory_committed flag to avoid duplicate commits\n\nFully backward compatible, no impact on non-OpenViking users.

@ZaynJarvis ZaynJarvis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Auto-Commit Feature (Idle Commit Watcher)

Verdict: Request Changes — blocking issues before this can merge

The goal (committing sessions early for searchability) is good, but the implementation has several problems that need addressing.

Blocking Issues

1. Synchronous requests inside an async coroutine (critical)

_idle_commit_watcher() calls requests.post() directly inside an async def function. This blocks the asyncio event loop for up to 10 seconds per session per cycle, freezing all gateway activity. This must use httpx.AsyncClient with await client.post() instead:

# Replace this:
import requests
_resp = requests.post(f"{_endpoint}/api/v1/sessions/{entry.session_id}/commit", ...)

# With this:
async with httpx.AsyncClient() as client:
    _resp = await client.post(f"{_endpoint}/api/v1/sessions/{entry.session_id}/commit", ...)

2. Hardcoded endpoint

The watcher hardcodes http://127.0.0.1:1933. The endpoint should come from os.environ.get("OPENVIKING_ENDPOINT", _DEFAULT_ENDPOINT) or the configured plugin endpoint — otherwise this breaks any non-default deployment.

3. cli.py change calls shutdown_memory_provider() on /new

shutdown_memory_provider() calls the provider's shutdown() method, which joins threads and clears _last_active_provider. After this, the provider is dead — it cannot receive sync_turn calls for the new session. The correct approach (as in #9167) is to call commit_memory_session() (commit only) then reinitialize_memory_session() (re-bind to new session). Using shutdown_memory_provider() here corrupts the session lifecycle for the new session.

4. Direct session_store._entries access

The watcher accesses self.session_store._entries directly, bypassing the _ensure_loaded()/lock machinery in the public API. Use the public SessionStore methods or at minimum hold _lock during the full iteration.

5. No tests

The idle commit watcher is non-trivial async logic. It needs at minimum: (a) test that watcher commits idle sessions, (b) test that memory_committed=True is set, (c) test that active sessions are skipped.

What Is Good

  • The concept of an idle commit watcher is sound and complementary to the explicit commit hooks in #9167/#7762
  • memory_committed field on SessionEntry is clean
  • Resetting memory_committed on new activity is correct

Please rewrite the watcher to use async httpx and fix the endpoint sourcing, then rebase onto #9167 so the cli.py change can be removed (it's handled there).

- Replace synchronous requests with async httpx.AsyncClient
  - Previous implementation blocked the event loop for up to 10s per request
  - Now uses non-blocking async HTTP requests

- Read OpenViking endpoint from environment variable
  - Supports OPENVIKING_ENDPOINT for remote deployments
  - Falls back to http://127.0.0.1:1933 for local development

- Minor optimization: collect sessions first, then batch commit
  - Avoids dict modification during iteration
  - Shares single httpx client across all commits in a cycle
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins labels Apr 28, 2026
@ZaynJarvis

Copy link
Copy Markdown
Contributor

plus, i think it's not necessary to auto commit since /new /reset /compress hook would commit the session #10463. If users are in the session, they can just use the in session context. I would probably suggest close this PR.

@happy5318

Copy link
Copy Markdown
Contributor Author

Thanks for the review @ZaynJarvis! The issues you've raised have been addressed in the latest commit:

  1. Using httpx.AsyncClient instead of synchronous requests - async non-blocking
  2. Reading endpoint from OPENVIKING_ENDPOINT environment variable
  3. Added lock protection with self.session_store._lock when accessing _entries

The implementation is already using httpx.AsyncClient with await, reading config from env vars, and protecting with locks. Could you please re-review? Thanks!

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the contribution, @happy5318! After reviewing against current main, the core pain point this PR addresses — OpenViking sessions not being committed on /new and /compress — was already fixed by PR #10463 (commit 7856d304f).

This is an automated hermes-sweeper review.

Evidence:

Additionally, 108 of the 112 changed files are compiled/binary dist artifacts (ui-tui/dist/, hermes_cli/web_dist/fonts/) that shouldn't land on main.

If there's interest in the narrower 120-second idle-commit optimization specifically for gateway sessions, that would be worth a small focused PR without the dist artifacts.

@teknium1 teknium1 closed this Jun 10, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants