perf(db): cache SQLite schema init and optimize descendant resolution - #39140
perf(db): cache SQLite schema init and optimize descendant resolution#39140sebastianlutycz wants to merge 3 commits into
Conversation
- Cache SQLite schema initialization status process-wide in hermes_state.py to prevent redundant column reconciliations and FTS probe queries on every SessionDB connection creation. This eliminates SQLite lock contention under high concurrent requests (e.g. status polls). Bypassed in pytest/unittest environments for mock compatibility. - Optimize the _session_latest_descendant query in hermes_cli/web_server.py using a recursive CTE (WITH RECURSIVE) to query only descendants of the target session. This avoids downloading and parsing all historical sessions in Python.
… Python fallback Why introduce Rust to the stack? - High-throughput streaming of terminal/PTY logs to WebSockets (e.g. during compilation, test execution) creates heavy CPU and thread-switching bottlenecks in Python due to the GIL and ThreadPoolExecutor. - Moving terminal I/O, resizing, and PTY process monitoring to a native Rust subprocess completely avoids blocking Python's event loop and reduces context-switching latency. - Future work can port other performance-critical components (such as text diffing, trajectory compression, and tokenizers) to Rust (via PyO3/Maturin) to maximize speed and stability. Implementation: - Introduced a lightweight Rust PTY bridge in pty_bridge_rust using portable-pty and tokio. It handles input, resizes (\x1b[8;<rows>;<cols>t), and outputs on separate native threads. - Added RustPtyBridge to hermes_cli/pty_bridge.py which communicates with the compiled Rust executable via pipes (stdin/stdout). - Integrated RustPtyBridge in hermes_cli/web_server.py. - Seamless Python PtyBridge (pure-Python fallback using ptyprocess) is maintained automatically if the Rust binary is missing or if running under a testing framework (e.g. pytest/unittest), ensuring 100% compatibility for standard Python-only environments and CI.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Small targeted DB perf + correctness fix: caches SQLite schema initialization per-path (skips redundant _init_schema() on subsequent SessionDB opens in the same process) and switches one descendant lookup to a recursive CTE.
Looks Good
- Thread-safe via
_db_init_lock. - Test-aware (
is_testingbypass) so pytest suites don't share state. - Recursive CTE replaces what was previously an in-memory loop; cleaner and more correct for deeper trees.
Reviewed by Hermes Agent
…ad EOF and using std::process::exit(0) to bypass tokio stdin shutdown bug
…escendant _session_latest_descendant fetched EVERY sessions row and built the parent->children tree in Python on each call. Replace with a recursive CTE that loads only the target session's descendant branch. Hand-applied from PR NousResearch#39140 (the schema-init cache and Rust PTY bridge parts of that PR are intentionally NOT salvaged here); main's function signature gained a db parameter since the PR was cut. (cherry picked from commit 8ed5e54)
The NousResearch#39140 CTE used UNION ALL, which recurses forever if a corrupted parent chain loops (a -> b -> a) — reproduced: query never returns. The old Python walk was cycle-safe via a seen-set. UNION dedups the working set and terminates. Regression test added and mutation-verified (UNION ALL hangs the test, UNION passes).
|
Partial salvage: your recursive-CTE descendant lookup landed on main as d7e4d94 via #60884 (rebase), with your authorship preserved — plus a follow-up switching UNION ALL -> UNION so a corrupted parent-chain cycle can't recurse forever (the old Python walk was cycle-safe via its seen-set; the CTE needed the dedup to match). Leaving this PR open for the remaining two parts: the SQLite schema-init cache (needs a design that doesn't sniff pytest in sys.modules — a process-level init registry keyed off the resolved path could work, but cross-process invalidation needs thought) and the Rust PTY bridge (a new native component — maintainer call). If you want to split those into separate PRs they'll be easier to land. |
…escendant _session_latest_descendant fetched EVERY sessions row and built the parent->children tree in Python on each call. Replace with a recursive CTE that loads only the target session's descendant branch. Hand-applied from PR NousResearch#39140 (the schema-init cache and Rust PTY bridge parts of that PR are intentionally NOT salvaged here); main's function signature gained a db parameter since the PR was cut. (cherry picked from commit 8ed5e54)
The NousResearch#39140 CTE used UNION ALL, which recurses forever if a corrupted parent chain loops (a -> b -> a) — reproduced: query never returns. The old Python walk was cycle-safe via a seen-set. UNION dedups the working set and terminates. Regression test added and mutation-verified (UNION ALL hangs the test, UNION passes).
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the performance work. The descendant-query portion has already landed as d7e4d94e2, with the necessary cycle correction in 1e2ad17afd.
Problems
hermes_cli/web_server.py:4484usesUNION ALL;1e2ad17afddemonstrates that a cyclic parent chain can recurse indefinitely. Main now usesUNIONand has coverage attests/hermes_cli/test_web_server.py:1330.hermes_cli/pty_bridge.py:299looks for an ignored/bin/hermes-pty-refactor, but this PR adds no build/install integration. A repository search of packaging, CI, Docker, and scripts finds no path that produces that binary.pty_bridge_rust/src/main.rs:97-111exits aftertry_wait()without draining the PTY reader, risking loss of trailing output. Its resize parsing at:69also assumes a pipe read is one complete resize frame.hermes_state.py:429disables the cache under pytest/unittest, and this PR changes no tests, leaving the new behavior unexercised.
Suggested changes
- Keep the already-salvaged CTE out of any follow-up.
- Rework the cache around current
SessionDB._init_schema()responsibilities (hermes_state.py:994) with explicit validity/invalidation semantics and multi-connection tests. - Add a supported native-helper distribution path plus framed I/O, EOF-drain/reap behavior, and integration tests before reconsidering the Rust bridge.
Automated hermes-sweeper review.
| """ | ||
| WITH RECURSIVE descendants(id, parent_session_id, started_at) AS ( | ||
| SELECT id, parent_session_id, started_at FROM sessions WHERE id = ? | ||
| UNION ALL |
There was a problem hiding this comment.
UNION ALL is unsafe here: a corrupted a -> b -> a parent cycle never reaches a fixed point. Main's follow-up 1e2ad17afd changed this to UNION and added a regression test; retain that deduplication in any salvage.
| if sys.platform.startswith("win"): | ||
| return False | ||
| # Locate binary relative to hermes-agent root directory | ||
| bin_path = Path(__file__).parent.parent / "bin" / "hermes-pty-refactor" |
There was a problem hiding this comment.
This checks an ignored /bin/hermes-pty-refactor artifact, but this PR contains no package, CI, Docker, or install step that builds or copies it there. Standard installs will therefore always take the Python fallback; add a supported distribution path before selecting this implementation.
| } | ||
| Ok(n) => { | ||
| let data = &buf[..n]; | ||
| if let Some(caps) = resize_re.captures(data) { |
There was a problem hiding this comment.
stdin is a byte stream, not a message-framed transport. A resize escape can be fragmented or coalesced with terminal input, causing this exact-match parser to forward control bytes into the TUI. Use explicit framing or buffered incremental parsing.
| break; | ||
| } | ||
| match child.try_wait() { | ||
| Ok(Some(_status)) => { |
There was a problem hiding this comment.
Breaking on child exit reaches std::process::exit(0) without waiting for the PTY reader thread to drain stdout. Large or trailing output can be lost; wait for reader EOF/drain and reap cleanly before helper exit.
|
|
||
| self._init_schema() | ||
| db_path_str = str(self.db_path.resolve()) | ||
| is_testing = "pytest" in sys.modules or "unittest" in sys.modules |
There was a problem hiding this comment.
Disabling the cache whenever pytest or unittest is imported prevents the new branch from being covered by normal tests. Prefer explicit cache reset/test fixtures and add multi-connection tests for path scoping and invalidation behavior.
…ck contention SessionDB.__init__ ran _init_schema on every connection construction. _init_schema calls executescript(SCHEMA_SQL) (15+ CREATE TABLE/INDEX), _reconcile_columns (PRAGMA + potential ALTER TABLE), and UPDATE messages SET active=1 — all acquiring a SQLite reserved lock. When multiple SessionDB instances were created concurrently (gateway + cron + channel directory), the DDL writes competed for the reserved lock, freezing the gateway. py-spy dumps consistently showed threads stuck in _init_schema/_connect_and_init across 3 separate freeze events. Add a process-level _initialized_dbs set guarded by _db_init_lock. After the first successful _init_schema for a given db_path, all subsequent connections skip the full DDL + reconciliation entirely. Bypassed in test environments (PYTEST_CURRENT_TEST) so test fixtures that mock connections still work. Same approach as upstream PR NousResearch#39140 (still open, mixed with unrelated Rust PTY + recursive CTE changes). This is the schema-cache part only. Tests: 339 state tests + 112 gateway/cron tests — all pass.
…escendant _session_latest_descendant fetched EVERY sessions row and built the parent->children tree in Python on each call. Replace with a recursive CTE that loads only the target session's descendant branch. Hand-applied from PR NousResearch#39140 (the schema-init cache and Rust PTY bridge parts of that PR are intentionally NOT salvaged here); main's function signature gained a db parameter since the PR was cut. (cherry picked from commit 8ed5e54)
The NousResearch#39140 CTE used UNION ALL, which recurses forever if a corrupted parent chain loops (a -> b -> a) — reproduced: query never returns. The old Python walk was cycle-safe via a seen-set. UNION dedups the working set and terminates. Regression test added and mutation-verified (UNION ALL hangs the test, UNION passes).
…escendant _session_latest_descendant fetched EVERY sessions row and built the parent->children tree in Python on each call. Replace with a recursive CTE that loads only the target session's descendant branch. Hand-applied from PR NousResearch#39140 (the schema-init cache and Rust PTY bridge parts of that PR are intentionally NOT salvaged here); main's function signature gained a db parameter since the PR was cut. (cherry picked from commit 8ed5e54)
The NousResearch#39140 CTE used UNION ALL, which recurses forever if a corrupted parent chain loops (a -> b -> a) — reproduced: query never returns. The old Python walk was cycle-safe via a seen-set. UNION dedups the working set and terminates. Regression test added and mutation-verified (UNION ALL hangs the test, UNION passes).
|
Thanks @sebastianlutycz for digging into these hot paths. The descendant-resolution optimization has since landed on main as a recursive CTE in |
…escendant _session_latest_descendant fetched EVERY sessions row and built the parent->children tree in Python on each call. Replace with a recursive CTE that loads only the target session's descendant branch. Hand-applied from PR NousResearch#39140 (the schema-init cache and Rust PTY bridge parts of that PR are intentionally NOT salvaged here); main's function signature gained a db parameter since the PR was cut. (cherry picked from commit 8ed5e54)
The NousResearch#39140 CTE used UNION ALL, which recurses forever if a corrupted parent chain loops (a -> b -> a) — reproduced: query never returns. The old Python walk was cycle-safe via a seen-set. UNION dedups the working set and terminates. Regression test added and mutation-verified (UNION ALL hangs the test, UNION passes).
…escendant _session_latest_descendant fetched EVERY sessions row and built the parent->children tree in Python on each call. Replace with a recursive CTE that loads only the target session's descendant branch. Hand-applied from PR NousResearch#39140 (the schema-init cache and Rust PTY bridge parts of that PR are intentionally NOT salvaged here); main's function signature gained a db parameter since the PR was cut. (cherry picked from commit 8ed5e54)
The NousResearch#39140 CTE used UNION ALL, which recurses forever if a corrupted parent chain loops (a -> b -> a) — reproduced: query never returns. The old Python walk was cycle-safe via a seen-set. UNION dedups the working set and terminates. Regression test added and mutation-verified (UNION ALL hangs the test, UNION passes).
This Pull Request introduces three key performance optimizations to the database and terminal streaming layers:
1. SQLite Schema Init Caching (
hermes_state.py)SessionDB.__init__executed_init_schema()(running DDLs and in-memory schema diffs) on every single connection instantiation. Under concurrent API requests (e.g. status polling), this caused database lock contention and blocked Python's single-threaded event loop._initialized_dbs) and thread lock (_db_init_lock) to skip schema reconciliation after first-time initialization.pytest/unittestenvironments to keep test connection mocking functional.2. Recursive Descendant Traversal (
web_server.py)_session_latest_descendantqueried all sessions in the database without filters, construction of the tree in Python on every invocation causing large memory/CPU load.WITH RECURSIVE) to load only the descendant branch of the target session.3. High-Performance Rust PTY Bridge (
hermes_cli/pty_bridge.py&pty_bridge_rust/)run_in_executorreads.ptyprocess) if the Rust binary is missing or on unsupported platforms.Verification
tests/hermes_cli/test_web_server.py): 227 passed.tests/test_hermes_state.py): 251 passed.