Skip to content

fix(honcho): drain context prefetch threads during shutdown - #69070

Open
bigstar0920 wants to merge 1 commit into
NousResearch:mainfrom
bigstar0920:fix/honcho-provider-shutdown
Open

fix(honcho): drain context prefetch threads during shutdown#69070
bigstar0920 wants to merge 1 commit into
NousResearch:mainfrom
bigstar0920:fix/honcho-provider-shutdown

Conversation

@bigstar0920

Copy link
Copy Markdown
Contributor

Summary

  • route HonchoMemoryProvider.shutdown() through HonchoSessionManager.shutdown() so manager-owned workers are stopped, not only flushed
  • atomically block new honcho-context-prefetch registrations while shutdown snapshots and joins in-flight requests
  • keep context-prefetch workers non-daemon so CPython cannot finalize underneath a native/socket stack if a request outlives the bounded join
  • add regression coverage for the async writer, active context prefetch drain, and post-shutdown prefetch rejection

Reproduction and root cause

On current main (d8bf3df255) with Honcho enabled, a supported single-query CLI invocation printed the complete answer and then exited via SIGABRT (134 / signal 6):

hermes chat -Q --toolsets skills -q 'Reply with exactly ...'
response marker printed: yes
exit: 134

The coredump showed the main thread in Py_FinalizeEx while another thread was still in socket receive / Pydantic response handling. A cleanup-time thread snapshot identified that worker as:

honcho-context-prefetch -> get_prefetch_context -> Honcho SDK -> httpx socket read

The provider already received the CLI shutdown hook, but it called manager.flush_all() rather than manager.shutdown(). Also, prefetch_context() created a fire-and-forget daemon thread without retaining a handle, so no shutdown path could join it.

Changing only flush_all() to shutdown() was insufficient until the context-prefetch thread itself was tracked. With this patch, the same live Honcho-backed CLI smoke test prints the expected marker and exits 0. A post-cleanup thread snapshot no longer contains honcho-context-prefetch.

Existing reports and PRs

Verification

179 passed in 3.85s
ruff check: passed
git diff --check: passed
live Honcho-backed chat -q smoke: expected marker present, exit 0

Focused test set:

tests/test_honcho_shutdown.py
tests/test_honcho_session_context.py
tests/test_honcho_startup_fail_open.py
tests/test_honcho_client_concurrency.py
tests/test_honcho_client_config.py
tests/plugins/memory/test_honcho_config_schema.py
tests/honcho_plugin/test_session.py

Scope and behavior

No configuration or public API changes. Long-running interactive/gateway behavior remains asynchronous. During provider shutdown, new context-prefetch work is rejected and existing work receives a bounded 10-second join. If a network request outlives that bound, the non-daemon worker prevents unsafe interpreter finalization and completes according to the existing HTTP timeout instead of being force-unwound through native frames.

@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 area/memory Memory subsystem: store, providers, sync, background reviews sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 22, 2026

@teknium1 teknium1 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.

Thanks for tracing the provider-to-manager shutdown gap; current main still starts honcho-context-prefetch as an untracked daemon in plugins/memory/honcho/session.py:689-702, while provider shutdown only calls flush_all() at plugins/memory/honcho/__init__.py:1539-1547.

Problems

  • tests/test_honcho_shutdown.py:35 assumes async-manager construction starts _async_thread. Current main commit bd1a850fa2 intentionally starts it lazily via _ensure_async_writer() in plugins/memory/honcho/session.py:548-567, so this regression test needs updating during salvage.
  • The new tracking covers only prefetch_context(). The first-turn path independently creates daemon honcho-base-first and calls the same get_prefetch_context() at plugins/memory/honcho/__init__.py:729-746; provider shutdown does not retain or join that thread at :1539-1547. This leaves the same finalization class reachable on first-turn context fetches.

Suggested changes

  • Preserve the lazy-writer behavior and make the test enqueue work before asserting writer state.
  • Bring honcho-base-first under the same shutdown ownership, with coverage for that path.

Automated hermes-sweeper review.

provider._manager = manager

assert manager._async_thread is not None
assert manager._async_thread.is_alive()

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.

Current main starts the async writer lazily in _ensure_async_writer() after save() enqueues work (bd1a850fa2), so a newly constructed manager has _async_thread is None. Enqueue a save before asserting writer lifecycle, otherwise this regression test fails when salvaged.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

3 participants