fix(honcho): bound local session cache growth - #71463
Conversation
HonchoSession.messages grew forever -- add_message() appended every turn and _flush_session() only marked entries synced, never trimmed them. HonchoSessionManager's four caches (_cache, _peers_cache, _sessions_cache, _context_cache) had no eviction path besides an explicit /new reset. A long-lived channel that is never manually reset accumulates both for the gateway's entire uptime. Honcho is the durable source of truth (get_or_create() already re-fetches history from Honcho on a cache miss), so both bounds are safe: evicting an idle local entry only costs one extra Honcho round-trip next time that key is used. - Trim already-synced messages beyond a retention window right after a successful flush; unsynced messages are never touched. - Add a rate-limited idle-TTL sweep triggered opportunistically from get_or_create(), so no new background task/watcher wiring is needed. Fixes NousResearch#71461
48a207a to
d967329
Compare
|
Rebased this PR onto
|
|
Rebased this PR onto
|
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real current-main retention problem: add_message() still appends indefinitely and _flush_session() only marks entries synced (plugins/memory/honcho/session.py:45-54,421-458).
Problems
- The new cap is not a hard bound.
_trim_synced_messages()stops at the first unsynced entry (plugins/memory/honcho/session.py:497in this PR); the added test intentionally retains entries 10–299 after placing an unsynced message at index 10 (tests/test_honcho_session_cache_bounds.py:64-77). A persistent failed sync can therefore still grow the local list without limit. - The sweep pops
_context_cacheunder_cache_lock(plugins/memory/honcho/session.py:401in this PR), but normal context-cache writes/pops are guarded by_prefetch_cache_lockon main (plugins/memory/honcho/session.py:708-718). Please use that lock for sweep removal and cover the concurrent prefetch case.
Suggested changes
- Specify a bounded failure-mode policy for unsynced messages, or narrow the claimed guarantee to successfully persisted history and test that contract.
- Synchronize context-cache eviction with the existing prefetch-cache lock.
Automated hermes-sweeper review.
| continue | ||
| del self._cache[key] | ||
| self._sessions_cache.pop(session.honcho_session_id, None) | ||
| self._context_cache.pop(key, None) |
There was a problem hiding this comment.
_context_cache is otherwise accessed under _prefetch_cache_lock (set_context_result() and pop_context_result() on main). Please acquire that lock for this removal as well; a background prefetch can otherwise race an idle sweep and repopulate stale context after eviction.
| negligible in practice. | ||
| """ | ||
| excess = len(session.messages) - _SESSION_MESSAGE_RETENTION | ||
| while excess > 0 and session.messages and session.messages[0].get("_synced"): |
There was a problem hiding this comment.
This stops at the first unsynced item, so it does not enforce the advertised retention bound: the added test's unsynced item at index 10 leaves 290 entries. Please define and test the intended persistent-sync-failure policy, or narrow the bound claim to histories that have successfully flushed.
What does this PR do?
Bounds the two unbounded-growth sources in the Honcho local session cache described in #71461:
HonchoSession.messages(never trimmed) andHonchoSessionManager's four caches (_cache,_peers_cacheuntouched/_sessions_cache/_context_cache, no eviction path except an explicit/new). Together these cause monotonic RSS growth on any gateway with a long-lived channel that's never manually reset — distinct from the_agent_cacheleak already fixed for #48287.This is scoped to
plugins/memory/honcho/session.pyonly — no changes togateway/run.pyor its watcher loop, sinceget_or_create()already re-fetches from Honcho (the durable source of truth) on a cache miss, so the fix can be entirely opportunistic/self-contained rather than needing new background-task wiring.Related Issue
Fixes #71461
Type of Change
Changes Made
plugins/memory/honcho/session.py:_SESSION_MESSAGE_RETENTION/_SESSION_IDLE_TTL_SECONDS/_SESSION_SWEEP_INTERVAL_SECONDSconstants.HonchoSessionManager._trim_synced_messages()(new, static): drops already-synced messages beyond the retention cap after a successful flush; never touches unsynced messages, even ones stuck mid-list from a failed sync.HonchoSessionManager._sweep_idle_sessions_locked()/_maybe_sweep_idle_sessions()(new): idle-TTL eviction across_cache/_sessions_cache/_context_cache, rate-limited and triggered opportunistically fromget_or_create()— no new background task needed._peers_cacheis deliberately left alone: it's keyed by distinct peer/user id, not by session, so its cardinality is bounded by user count rather than by uptime — it isn't part of this leak._flush_session()now calls_trim_synced_messages()right after a successful sync.get_or_create()now calls_maybe_sweep_idle_sessions()on entry.tests/test_honcho_session_cache_bounds.py(new): 7 tests covering trim-caps-length, trim-never-drops-unsynced (including the stuck-mid-list case), trim-is-noop-under-cap, sweep-evicts-stale-across-all-caches, sweep-keeps-fresh, sweep-is-rate-limited, and a threading test guarding against theRLockreentrancy this design depends on (sweep is triggered from inside a method that also holds the lock).How to Test
pytest tests/test_honcho_session_cache_bounds.py -v— all 7 new tests pass.pytest tests/test_honcho_session_context.py tests/test_honcho_client_concurrency.py tests/test_honcho_startup_fail_open.py tests/test_honcho_client_config.py -v— confirms no regression in the existing 29 Honcho tests.HonchoSession, calladd_messagein a loop past_SESSION_MESSAGE_RETENTION, call_flush_session—len(session.messages)stays bounded instead of growing forever. Similarly, age a cached session'supdated_atpast_SESSION_IDLE_TTL_SECONDSand call_maybe_sweep_idle_sessions()— it's evicted.Checklist
Code
fix(honcho):)pytest tests/ -qand all tests pass — I ran the full Honcho-related test files (36 tests, all passing, listed above); the full repo-wide suite timed out collecting in my local environment (missing some optional dependency groups) rather than failing, so I couldn't confirm a full green run locally — deferring to CI for that. Happy to chase down the collection hang separately if it's not just an environment gap on my end.Documentation & Housekeeping
Screenshots / Logs