Skip to content

fix(honcho): stop background workers before exit - #72385

Open
clara-patchou-ai wants to merge 1 commit into
NousResearch:mainfrom
clara-patchou-ai:fix/fu20-honcho-clean-exit
Open

fix(honcho): stop background workers before exit#72385
clara-patchou-ai wants to merge 1 commit into
NousResearch:mainfrom
clara-patchou-ai:fix/fu20-honcho-clean-exit

Conversation

@clara-patchou-ai

Copy link
Copy Markdown

Summary

Fix intermittent Python exit 134 after a successful Honcho-backed CLI response by making the provider own and stop every background worker before interpreter finalization.

  • track provider- and session-manager-owned workers
  • reject new background work once shutdown begins
  • publish the session manager atomically after remote initialization
  • serialize save admission with shutdown and use a writer-owned FIFO flush barrier
  • preserve final flushing for session and exit write modes
  • retry failed manager cleanup while retaining ownership until it succeeds
  • derive CLI exit-watchdog bounds from provider shutdown budgets while preserving explicit operator overrides
  • add deterministic coverage for initialization/shutdown races, blocked writes, prefetch workers, late admission, timeout/retry, and watchdog integration

Root cause

HonchoMemoryProvider.shutdown() called flush_all() but never stopped HonchoSessionManager's daemon honcho-async-writer. Provider-owned initialization, prefetch, sync, and memory-write threads were also not comprehensively tracked. During fast fallback exits, Python could begin finalization while a daemon thread was still in Honcho/httpx/native TLS code, producing SIGABRT after the model response had already been printed.

The narrow manager.shutdown() fix was not enough: shutdown could race with session initialization and miss a manager published afterward; save admission could race the writer sentinel; synchronous flushing could race a live writer; and the fixed 30-second CLI exit watchdog could expire before timeout-aware Honcho cleanup completed. This change introduces terminal admission gates, atomic manager publication, writer-owned flush ordering, retained/retryable manager ownership, and provider-aware watchdog timing.

Reproduction and verification

Before this fix, the isolated memory-disabled fallback profile returned the expected response but aborted with exit 134 in 3 of 5 runs.

After the lifecycle fix:

  • 24 of 24 equivalent fallback runs returned the expected marker and exited 0
  • uv run --extra honcho --extra dev pytest -q tests/honcho_plugin
    • 450 passed; 4 cache-busting singleton/order failures outside the changed code
    • a clean origin/main worktree reproduced 3 failures from the same cache-busting test class (437 passed)
  • uv run --extra honcho --extra dev pytest -q tests/honcho_plugin/test_async_memory.py tests/honcho_plugin/test_session.py tests/agent/test_memory_provider.py tests/cli/test_cli_shutdown_memory_messages.py
    • 323 passed
  • Ruff checks over all changed Python files passed
  • git diff --check passed

The original provider-shutdown regression was verified RED before the fix and GREEN afterward.

Related work

This addresses the same shutdown/finalization family discussed in #60616, #37632, and #33485 and overlaps with ideas in #7627, #31664, #37635, and #58292. The distinguishing scope here is complete ownership across the async writer, manager prefetch workers, provider initialization, context/dialectic prefetch, sync, and explicit memory-write paths, including manager-publication and production-watchdog integration.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard 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 27, 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 manager-owned writer through the provider lifecycle. The core omission remains on current main: plugins/memory/honcho/__init__.py:1539-1548 flushes the manager without calling HonchoSessionManager.shutdown(), while plugins/memory/honcho/session.py:499-567 owns the async-writer sentinel and join.

Problems

  • cli.py:1134 makes the exit watchdog timeout-derived. With the PR fallback of 30 seconds (plugins/memory/honcho/__init__.py:306-315), one manager produces a 224-second provider hint (:317-337); agent/memory_manager.py:1156-1158 doubles retryable-provider budgets, and CLI adds 15 seconds. That makes normal cleanup 468 seconds and the signal watchdog 936 seconds. This defeats the watchdog's current bounded-cleanup role (cli.py:1169-1172) when Honcho is slow or wedged.

Suggested changes

  • Keep a hard bounded cleanup deadline independent of HTTP timeout, then add tests for unset and large configured Honcho timeouts alongside the worker-ownership tests.

This is an automated hermes-sweeper review.

Comment thread cli.py
try:
# Leave headroom for transcript finalization and the other bounded
# cleanup stages that run before memory-provider teardown.
timeout_s = max(timeout_s, float(hint()) + 15.0)

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.

This timeout becomes unbounded in practice. With the PR's 30s fallback, one manager computes 224s; MemoryManager doubles retryable-provider hints and this adds 15s, yielding a 468s cleanup watchdog (936s for the signal path). Keep an independent hard cap so a wedged Honcho backend cannot hold CLI exit for minutes.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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