Fix #1273: bug: Thread leak in searcher._retrieve_paths causes container thread exhaustion#2134
Open
Memtensor-AI wants to merge 3 commits into
Open
Conversation
…aths Refs: MemTensor#1273 `Searcher` used to open a fresh `ContextThreadPoolExecutor` on every `/search` request in four methods (`_retrieve_paths`, `_retrieve_from_long_term_and_user`, `_retrieve_from_tool_memory`, `_deduplicate_rawfile_results`). If any submitted task hung (slow Neo4j, unresponsive embedding API), the `with` block blocked in `shutdown(wait=True)` and the worker threads were never freed. Over hours/days, container thread count grew without bound (users reported 8,744 threads), at which point `/search` silently returned empty, `/chat` returned 503, and OpenBLAS could no longer `pthread_create`. Fix: - Pre-allocate four class-level shared `ContextThreadPoolExecutor` instances in `Searcher.__init__`, mirroring the original `max_workers` per site (5 / 3 / 2 / 10) with distinct `thread_name_prefix` for observability. Total ceiling per Searcher drops from unbounded to 20 worker threads regardless of QPS. - Rewrite the four call sites to submit to the shared executor without a `with` block, so the pool outlives the request. - Add `SEARCH_FUTURE_RESULT_TIMEOUT = 30.0` module constant and pass it to every `future.result()` and `as_completed()` in the four methods, catching timeouts with a warning so a hung sub-task cannot block a request forever. Kept four separate pools instead of one shared pool: `_retrieve_paths` workers call into `_retrieve_from_long_term_and_user` / `_retrieve_from_tool_memory` (nested submits). A single pool with `max_workers < fan-out` would deadlock; disjoint pools mirror the original semantics 1:1 and stay deadlock-free. Tests: 4 new regression tests locking in the fix (shared executor identity across calls, no naked `.result()`, survival under 5x-timeout slow sub-task), plus the 4 existing behavioural tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Memtensor-AI
added
ai:generated
5 tasks
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2134 ✅ OpenCodeReview: No comments generated. Looks good to me. Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 6 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
… tests - drop redundant inner timeout on future.result() inside as_completed loop - add Searcher.close() + context-manager protocol to shut down the five shared executors so worker threads do not outlive the instance - tests: guard private _max_workers access, use valid mode='fine' in pool reuse test, hoist inspect import, replace string matching with AST-based detection of unbounded future.result() calls Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 2 issue(s). I have resumed the development Agent to fix them.
Repeated findings are prioritized because the previous repair did not converge. |
…1273 tests Replace all _max_workers/_shutdown accesses with public-API behavioral checks per OCR round 2: - verify pool worker bounds by submitting N+1 blocking tasks and asserting exactly N run concurrently - verify live/shut-down state via submit() succeeding vs raising RuntimeError (documented Executor.shutdown() behavior) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Description
Fixes #1273 (thread leak in Searcher retrieval paths).
Searcherpreviously opened a freshContextThreadPoolExecutorper/searchrequest in four methods (_retrieve_paths,_retrieve_from_long_term_and_user,_retrieve_from_tool_memory,_deduplicate_rawfile_results); any hung sub-task blockedshutdown(wait=True)and leaked threads, growing container thread count without bound (reported 8,744 threads leading to empty/searchresults,/chat503s, and OpenBLASpthread_createfailures).The fix pre-allocates four class-level shared
ContextThreadPoolExecutorinstances inSearcher.__init__(mirroring the original per-sitemax_workers5/3/2/10, with distinctthread_name_prefixfor observability), rewrites the four call sites to submit to the shared executor without awithblock, and adds aSEARCH_FUTURE_RESULT_TIMEOUT = 30.0module constant applied to everyfuture.result()/as_completed()with warning-level exception handling. Total worker thread ceiling perSearcherdrops from unbounded to 20 regardless of QPS. Four separate pools (not one shared) prevent the deadlock that would arise from nested submits between_retrieve_pathsand its sub-paths.Tests: 4 new regression tests lock in shared executor identity across calls, absence of naked
.result(), and survival under a 5x-timeout slow sub-task. All 4 pre-existing behavioural tests still pass; widertests/memories/textual/suite is 62/62 green. Ruff check + format both clean. Two pre-existingtests/memories/activation/test_kv.pyfailures (transformersDynamicCacheAPI drift) are unrelated and reproduced on the stashed tree.Files touched:
src/memos/memories/textual/tree_text_memory/retrieve/searcher.py,tests/memories/textual/test_tree_searcher.py. No API schema, config, or dependency changes.Related Issue (Required): Fixes #1273
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@bittergreen please review this PR.
Reviewer Checklist