Fix #2291: fix(hermes-adapter): widen _ACTIVE_CLIENTS key with owner_id to prevent intra-pr - #2293
Closed
Memtensor-AI wants to merge 2 commits into
Closed
Conversation
…or#2291) When the Hermes gateway hosts multiple concurrent sessions (email threads, cron jobs, subagents) in one process, each session gets its own MemTensorProvider instance. The module-level _ACTIVE_CLIENTS singleton in bridge_client.py was keyed by (agent, no_viewer, runtime_home) — one slot per process — so every new provider's MemosBridgeClient closed whichever client held that slot, even when the slot belonged to a different, healthy session's provider. The gateway process oscillated between 2+ bridge.mjs children replaced every 2-3 seconds. Widen the singleton key to (agent, no_viewer, runtime_home, owner_id) so concurrent provider instances coexist. Callers pass owner_id=f"provider-{id(self)}"; the anonymous fallback is f"anon-{id(self)}" and stays unique for the client's lifetime. The issue MemTensor#1910 guarantee (a single provider does not keep spawning bridges per turn) is preserved: re-registration under the same owner_id still reaps the previous holder. - bridge_client.py: key widened tuple[str, bool, str] -> tuple[str, bool, str, str]; owner_id: str | None added to __init__; _singleton_owner field set from owner_id or f"anon-{id(self)}"; key construction in _register_active / _unregister_active updated. - __init__.py: both construction sites (initialize() and _reconnect_bridge()) in shared and legacy modes pass owner_id=f"provider-{id(self)}". - tests/python/test_bridge_client.py: adds regression tests covering distinct-owner coexistence, same-owner reap, anonymous-fallback isolation; existing MemTensor#1910 tests updated to pass explicit owner_id. Tests: 126 unit tests pass (python3 -m unittest discover -s tests/python). Related: MemTensor#1910, MemTensor#1927, MemTensor#1985
Memtensor-AI
added
ai:generated
6 tasks
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2293 ✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 2 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. |
…-key test Address open-code-review findings on PR MemTensor#2293: * bridge_client.py: raise a warnings.warn when MemosBridgeClient is constructed without owner_id. The anon-<id(self)> fallback is unique per instance, which silently disables the MemTensor#1910 reap-previous guard. Warning forces callers to be explicit about opting out. * test_bridge_client.py: replace assertIsNot(key_first, key_second) with assertNotEqual. Two freshly constructed tuples are always distinct objects in CPython, so the identity check was vacuously true and would still pass if both keys carried identical values. Value comparison is what the regression test needs.
Collaborator
Author
✅ Automated Test Results: PASSEDAll tests passed (114/114 executed, 1 skipped). memos_github_open_source/smoke: 1/1, memos_local_plugin/changed-repo-python: 68/68, memos_python_core/changed-repo-python: 45 passed, 1 skipped. Duration: 9s [advisory, non-gating] AI-generated tests on branch test/auto-gen-5daf587c668e1502-20260828033828: 33/33 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
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
3 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 #2291. Widens the module-level
_ACTIVE_CLIENTSkey inapps/memos-local-plugin/adapters/hermes/memos_provider/bridge_client.pyfrom(agent, no_viewer, runtime_home)to(agent, no_viewer, runtime_home, owner_id)so concurrentMemTensorProviderinstances in one Hermes gateway process (email threads, cron jobs, subagents) stop reaping each other's bridge subprocesses.Changes:
bridge_client.py: addsowner_id: str | None = NonetoMemosBridgeClient.__init__; storesself._singleton_owner = owner_id or f"anon-{id(self)}"; widens_ACTIVE_CLIENTStype annotation todict[tuple[str, bool, str, str], MemosBridgeClient];_register_active/_unregister_activebuild the four-tuple key.adapters/hermes/memos_provider/__init__.py: both provider construction sites (initialize()and_reconnect_bridge()) in shared and legacy bridge modes now passowner_id=f"provider-{id(self)}".tests/python/test_bridge_client.py: adds three regression tests — distinct owners coexist without displacement, same-owner replacement still reaps predecessor (preserves Bridge process leak: every turn spawns new bridge.cjs, 4+ processes accumulated #1910 guarantee), anonymous fallback yields unique keys per instance. Updates the two existing Bridge process leak: every turn spawns new bridge.cjs, 4+ processes accumulated #1910-era tests to pass explicit owner ids.Verification:
python3 -m unittest discover -s tests/python→ 126 tests pass, 0 failed, 0 errored. Full log excerpt: "Ran 126 tests in 17.707s — OK". No production tests removed; only two pre-existing #1910 unit tests widened to assert on the four-tuple key.Expected steady state after deploy: daemon + 1 bridge per host process = 3 total instead of 2 bridges churning every 2-3s; reconnect counts approach zero. Related history: #1910 (bridge process leak, now defended alongside #2291), #1927 (double-initialize), #1985 (HTTP transport, the eventual architectural end-state).
Related Issue (Required): Fixes #2291
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests are pending.
Checklist
@WeiminLee please review this PR.
Reviewer Checklist