fix: share SQLite storage across LCM engine clones - #197
100yenadmin wants to merge 4 commits into
Conversation
Address review feedback on the shared-storage fix: - Add test_clone_churn_does_not_leak_fds: verifies clone churn and live concurrent clones add zero file descriptors and that shutdown returns the process to its FD baseline (uses /proc/self/fd or /dev/fd). - Add test_concurrent_clone_churn_is_thread_safe: 10 threads x 10 clone/shutdown cycles against one prototype, asserting no errors, the prototype lease survives, and final close happens exactly once. - Document the acquire/release reference-counting lifecycle inline in _SharedStorage.
Imported upstream pr-conversation
PR ReviewStrengthsThis is a comprehensive fix that addresses the root cause of the FD leak:
Suggested Enhancements1. Add FD leak regression testDirect test that verifies FD count doesn't grow under clone churn: def test_clone_does_not_leak_fds(self, tmp_path):
from pathlib import Path
import os
baseline = len(list(Path(f"/proc/{os.getpid()}/fd").iterdir()))
prototype = self._engine(tmp_path)
clones = [prototype.clone_for_agent() for _ in range(10)]
after_clones = len(list(Path(f"/proc/{os.getpid()}/fd").iterdir()))
for clone in clones:
clone.shutdown()
prototype.shutdown()
after_shutdown = len(list(Path(f"/proc/{os.getpid()}/fd").iterdir()))
assert after_clones - baseline < 5 # Small increase acceptable
assert after_shutdown == baseline # Clean return to baselineWhy: Demonstrates fix prevents FD exhaustion under real clone churn patterns. 2. Add inline lifecycle commentsDocument reference counting flow in def acquire(self) -> "_SharedStorage":
# Increment reference count, prevent double-acquire on closed storage
with self._lock:
if self._closed or self._closing:
raise RuntimeError("cannot acquire closed LCM storage")
self._owners += 1
return self
def release(self) -> None:
# Decrement reference count, trigger close when last owner releases
with self._lock:
if self._owners <= 0:
return
self._owners -= 1
if self._owners:
return # Still shared, keep alive
self._closing = True # Last owner releasing, trigger teardownWhy: Future maintainers can trace ownership lifecycle quickly. 3. Add concurrent stress test (optional)Multi-threaded clone churn test to verify locks prevent race conditions: def test_concurrent_clone_churn(self, tmp_path):
prototype = self._engine(tmp_path)
barrier = threading.Barrier(11)
errors = []
def worker():
barrier.wait()
try:
for _ in range(10):
clone = prototype.clone_for_agent()
clone.shutdown()
except Exception as e:
errors.append(e)
threads = [threading.Thread(target=worker) for _ in range(10)]
for t in threads:
t.start()
barrier.wait()
for t in threads:
t.join()
prototype.shutdown()
assert errors == []Why: Verifies thread safety under high-concurrency scenarios. 4. Handle SQLite close errors specificallyCatch for helper in (self.store, self.dag, self.lifecycle, ...):
close = getattr(helper, "close", None)
if callable(close):
try:
close()
except sqlite3.OperationalError as exc:
# WAL checkpoint can fail if connection state inconsistent
failures.append(exc)
except BaseException as exc:
failures.append(exc)Why: Better error reporting and debugging for SQLite-specific issues. VerdictReady to approve with these enhancements. High-quality fix with minimal additions needed. The FD regression test (suggestion #1) is the highest-value addition - it directly demonstrates that the issue is fixed and provides a regression guard for future changes. ContextThis PR addresses issue #463 (file descriptor leak on macOS). I encountered the same issue on Linux and confirmed:
Our local verification confirms the fix is correct. PR #501 is more comprehensive than our stopgap patch and is ready for merge with the suggested enhancements. Imported upstream pr-conversation
Thanks for the detailed review and the Linux confirmation, @jtstothard. Pushed d4eeb4c addressing the feedback: 1. FD leak regression test — added 2. Lifecycle comments — added inline comments documenting the acquire/release reference-counting flow in 3. Concurrent stress test — added 4. SQLite-specific close handling — left as-is intentionally: the close loop already catches Verification: full |
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. |
|
Thanks @grantjayy for measuring the clone storage cost and for the FD-leak tests. This mirror now conflicts with main, and it's a resource optimization rather than a fix for broken behavior, so we're closing it to keep the queue focused on verified defects. If clone FD or initialization cost is hurting a real deployment on current main (v0.24.1 or later), please open an issue with the measurements or refile a rebased PR against main. |
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
4aeacebd6bbbb7688fea3669dc611b6dc3a3473egrantjayy/hermes-lcm:fix/shared-sqlite-clone-storage-cleanmainupstream/pr-501Falsedraft(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
LCM engine clones currently construct a fresh set of SQLite-backed helpers. A clone therefore pays database initialization cost again and retains another set of SQLite file descriptors even though it is serving the same logical database as its prototype.
This change gives each clone family one reference-counted storage bundle while keeping mutable engine/session/model state clone-local. Independently constructed engines still own separate bundles, but bundles resolving to the same canonical database path share one in-process reentrant lock for helper construction, operations, backup/commit, and final teardown.
Fixes #463.
Hermes-Session:
20260804_101014_c509f5f7Why this is needed
Hermes creates engine clones for child agents. Before this change, ten retained clones added 60 file descriptors on the measured macOS checkout and spent roughly 100 ms constructing duplicate SQLite helper sets. That multiplication raises the risk of descriptor exhaustion and repeated schema/WAL setup under parallel child-agent workloads.
The ownership boundary is the important part of the fix:
This is complementary to #470's cleanup work. It does not replace #486's cross-process FTS bootstrap locking; the lock introduced here is process-local.
Implementation
_SharedStoragebundle for message store, summary DAG, lifecycle state, optional assertion store, and optional query-view store.clone_for_agent()and preserve clone-local runtime/model state.scripts/measure_clone_storage.pyfor reproducible before/after measurements against a selected checkout.Reproduced benchmark
Environment: macOS, Python from the active Hermes environment, 25 samples, 10 retained clones. The benchmark imports each selected checkout using
LCM_BENCH_REPO_ROOT.Commands:
Commits:
6b7dbb1074b9d8bd117bc092a9d77f2be5b5b8300d304d2Results:
+60→05.402319 ms→0.166021 ms101.970458 ms→1.952215 ms6.731890 ms→7.287487 msRaw local receipts:
/private/tmp/lcm-storage-benchmark-final-base-20260804.json2bcd2a9430f2d63a5b4f7ef00a9386157a9475e27825d935c44e03d3d3c09c06/private/tmp/lcm-storage-benchmark-final-feature-20260804.jsonba969980fa6087ded140a811b1bcf91a2b725e9f64aae536f1484f7cc84a7626Verification
Green feature/affected gates:
tests/test_lcm_core.py::TestLCMEngineSharedStorage: 9 passedtests/test_lcm_engine.py: 751 passed, 1 skippedtests/test_lcm_core.pyexcluding one independently reproduced upstream failure: 316 passedtests/test_packaging_install.py: 38 passed in the grouped run before the known isolated baseline failure was reproducedtests/test_assertion_lifecycle_tools.py tests/test_assertion_store.py tests/test_query_view_store.py: 49 passedtests/test_lcm_core.py::TestLifecycleStateStore: 19 passedtests/test_assertion_store.py: 20 passedtests/test_query_view_store.py: 21 passedgit diff --check: passed074b9d8: PASS; 133 affected storage tests passed locallyFull-suite result:
mainfor pre-existing or load-sensitive tests. The observed failures were reproduced against pristine6b7dbb1, including strict wall-clock embedding deadlines, macOS/varversus/private/varcontainment assertions, trajectory token/chunk expectations, provider-routing behavior, and an isolated packaging registration test.Scope
This PR intentionally does not: