fix: share LCM storage across runtime clones - #202
100yenadmin wants to merge 4 commits into
Conversation
Imported upstream pr-conversation
@stephenschoettler maintainer review requested. The repository owner and recent merger of the latest releases/fixes, you appear to be the appropriate reviewer for this PR. Please review the _StorageBundle ownership/refcounting, run or enable CI, and advise whether the dependency on hermes-agent PR #79575 should be handled as-is or adjusted before merge. Imported upstream pr-conversation
Pre-review finding (likely blocker): Imported upstream pr-conversation
Additional pre-review blocker: this branch appears not to implement the explicit ContextEngine.create_runtime() / close() contract introduced by hermes-agent PR #79575. Once that core loader contract is active, the LCM engine may be rejected for lacking an isolated runtime factory, and inherited close() will not release the shared storage bundle. The plugin PR should implement the contract explicitly, likely mapping current clone/runtime construction to create_runtime() and _close_storage() to close(), with an integration test against the updated core loader.\n\nThe review also flagged a second major issue: _StorageBundle.release() stops if one helper close() raises, so later helpers can remain open. Close each helper independently while preserving and logging the first exception. Imported upstream pr-conversation
Final review found one remaining blocker at head 98b259a: repeated runtime.close() is not safely idempotent. A second close can invalidate shared storage, causing a still-live prototype to fail on subsequent database use. The fix needs per-runtime one-shot release semantics plus a regression test that closes the same runtime twice and then exercises the prototype/sibling runtime. A focused repair is underway; all other lifecycle, FD, portability, and test concerns passed. Imported upstream pr-conversation
Final independent approval review completed at head 95cafc4: no remaining blockers. Verified full pytest (2799 passed, 1 skipped, 12 xfailed), focused lifecycle/core (312 passed), FD hygiene (1 passed), compileall, and diff checks. Verified repeated close is idempotent; prototype and sibling remain usable; bundle refcount and helper close counts are correct; failed-construction cleanup, rebind, concurrency, optional helpers, runtime contract compatibility with hermes-agent PR #79575, portable bootstrap, and bounded FD growth all pass. Ready for maintainer review/merge. |
100yenadmin
added
active-continuation
|
Triage: this is one of three open PRs implementing the same architectural change — flagging so it is not reviewed in isolation. #197, #202 and #215 each introduce a shared, reference-counted SQLite storage bundle across LCM engine clones. All three necessarily relax the same documented guarantee — that a clone owns isolated storage — by removing the six To be clear, that is not test-hiding, and I checked before saying so: all three add replacement assertions for the new invariant (9, 8 and 11 respectively — clone-local session state, one idempotent lease per engine, owner-first shutdown). This is a legitimate contract change with coverage. The motivation is real and measured, from #197: ten retained clones cost 60 file descriptors and roughly 100 ms of duplicate SQLite helper construction, which risks descriptor exhaustion under parallel child-agent workloads. That is the defect tracked as #9. So the problem is not any one of these PRs — it is that there are three. Reviewing them independently would mean deciding the same architecture question three times, and merging any one of them makes the other two conflict against a moved contract. The maintainer action is to pick one design, land it, and close the other two with evidence. This needs an owner/architecture call, because it changes a documented guarantee for every embedder of the engine. It is not a call I will make unilaterally as maintainer. Recorded on #9 as the tracking issue; holding all three until it is decided. |
2 participants
Important
This executable LCM-X PR was recreated by the migration operator from the exact upstream commit head. GitHub did not transfer the original PR actor, dates, review objects, or approval state.
Source and attribution
95cafc4fe49a1c2ae4dca50f332b3d768cd3ec92jtstothard/hermes-lcm:feat/storage-bundle-lifecyclemainupstream/pr-506Falsedraft(a maintainer can mark it ready when current-head review is wanted)@coderabbitai ignore
Original commit authorship and history remain in the commits. Historical discussion and review text are imported below as attributed ordinary comments; they are not new approvals or change requests.
Original upstream PR description
Summary
_StorageBundlefor the SQLite helpers shared by cloned LCM runtimes.clone_for_agent()and release it during shutdown, while preserving per-agent session state.This is the distinct LCM storage-lifecycle half of the descriptor-lifecycle work. It is not the generic
SessionDBfix in hermes-agent#72822.Tracking
Verification
python3 -m pytest -q tests/test_fd_hygiene.py— 1 passedpython3 -m pytest -q tests/test_lcm_core.py— 308 passedpython3 -m compileall -q engine.py tests/test_fd_hygiene.py tests/test_lcm_core.py— passedgit diff --check— passedScope
Fresh branch from
origin/main; onlyengine.py,tests/conftest.py,tests/test_lcm_core.py, andtests/test_fd_hygiene.pyare included. No generated files or unrelated existing work are included.