fix(memory/honcho): align shutdown join timeout with Honcho HTTP timeout - #33701
Open
Pluviobyte wants to merge 1 commit into
Open
fix(memory/honcho): align shutdown join timeout with Honcho HTTP timeout#33701Pluviobyte wants to merge 1 commit into
Pluviobyte wants to merge 1 commit into
Conversation
HonchoMemoryProvider.shutdown joined the dialectic / prefetch daemon threads with a fixed timeout=5.0, but Honcho HTTP calls default to a 30s client timeout. When the join expired before the HTTP call returned, the daemon thread was still mid-call once CPython entered Py_FinalizeEx, intermittently aborting the interpreter with SIGABRT during otherwise-clean CLI shutdown (issue NousResearch#33485, hybrid recall mode). Extract the existing timeout resolution chain from get_honcho_client into a reusable resolve_http_timeout(config) helper, then use it from shutdown so the join window grows to match config.timeout / honcho config / HONCHO_TIMEOUT / _DEFAULT_HTTP_TIMEOUT. A 5s floor preserves the original grace period for users who configure a very short HTTP timeout (their HTTP call cannot outlast the floor anyway). Also map this contributor in scripts/release.py AUTHOR_MAP so the contributor-attribution check has a username for the noreply email. Fixes NousResearch#33485 Co-authored-by: Cursor <cursoragent@cursor.com>
Pluviobyte
force-pushed
the
fix/honcho-shutdown-join
branch
from
June 10, 2026 10:02
34fc24c to
7280e04
Compare
teknium1
reviewed
Jul 13, 2026
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for isolating the fixed 5s-versus-client-timeout mismatch. Current main still has the fixed t.join(timeout=5.0) in plugins/memory/honcho/__init__.py:1414-1417, while its client defaults unresolved HTTP calls to 30s in plugins/memory/honcho/client.py:209-215 and passes that resolved value at client.py:841-862.
Problems
tests/honcho_plugin/test_shutdown_join_timeout.pyonly checks mockedThread.joinarguments and mocked configuration. It does not execute client construction and provider shutdown through one resolved configuration path, so the claimed shared-resolution contract is not covered end to end.
Suggested changes
- Add an isolated-
HERMES_HOMEregression that captures the SDK constructor timeout viaget_honcho_clientand verifies provider shutdown derives the same value without a real 30-second wait.
This is an automated hermes-sweeper review.
| provider._sync_thread = sync | ||
| provider._manager = None # skip flush_all | ||
|
|
||
| provider.shutdown() |
Contributor
There was a problem hiding this comment.
This mock-only assertion proves the join argument derived by the helper, but not that SDK construction and shutdown use the same resolution path. Please add an isolated-HERMES_HOME regression that captures the timeout passed by get_honcho_client and verifies shutdown uses that identical value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
HonchoMemoryProvider.shutdownjoined the dialectic / prefetch daemon threads with a fixedtimeout=5.0, but the Honcho SDK HTTP client defaults to a 30s timeout (configurable viaHonchoClientConfig.timeout,honcho.timeout/requestTimeout, orHONCHO_TIMEOUT). When the join expired before a pending HTTP call returned, the daemon thread was still mid-call once CPython enteredPy_FinalizeEx, intermittently aborting the interpreter with SIGABRT during otherwise-clean shutdown. Issue #33485 isolated this torecallMode: hybridwhere the dialectic background worker is the most reliable trigger; the reporter confirmed extending the join window to 30s or shrinking the HTTP timeout to 3s eliminates the abort, which matches the timing analysis.This PR aligns the shutdown grace period with whatever timeout the Honcho client is actually using, while preserving a 5s floor:
get_honcho_clientinto a reusableresolve_http_timeout(config)helper so shutdown and the SDK kwargs read from the same source.HonchoMemoryProvider.shutdowntot.join(timeout=max(5.0, resolve_http_timeout(self._config)))for both_prefetch_threadand_sync_thread. The 5s floor preserves the original grace period when a user configures a very short HTTP timeout — the HTTP call cannot outlive that floor anyway, so the join is still bounded.importfailures and missing config so shutdown stays exception-free.Related Issue
Fixes #33485
Type of Change
Changes Made
plugins/memory/honcho/client.py— addedresolve_http_timeout(config)and routedget_honcho_clientthrough it so SDK and shutdown agree on the effective HTTP timeout.plugins/memory/honcho/__init__.py—HonchoMemoryProvider.shutdownnow waitsmax(5.0, resolve_http_timeout(self._config))before abandoning the dialectic / prefetch threads.tests/honcho_plugin/test_shutdown_join_timeout.py— new file with 10 regression tests covering explicit config timeout, hermes-cli fallback, default 30s, 5s floor, missing config, andresolve_http_timeoutchain semantics.scripts/release.py— added this contributor email toAUTHOR_MAPso the contributor-attribution check has a GitHub username for the noreply address.How to Test
recallMode: hybrid(the default) so the dialectic worker is exercised.Py_FinalizeEx. After this PR the join waits the full 30s, the worker exits cleanly, and the interpreter shuts down without abort.Regression-coverage check: temporarily reverting the
__init__.pychange makes 3 of the 4TestShutdownJoinTimeouttests fail (the one that already expects 5s still passes), confirming the new tests pin the bug rather than the fix.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A
Screenshots / Logs
Note on overlap: PRs #7627 (close httpx pool), #31664 (route oneshot through
HonchoSessionManager.shutdown), and #33543 (os._exitfrom oneshot to skip finalizers entirely) target the same SIGABRT class from different layers and are independent of this change. None of them adjusts the 5s join timeout, so this PR is complementary regardless of which lands first.Made with Cursor