fix(state): dedup list_sessions_rich's recursive chain CTE (UNION not UNION ALL) - #61201
Open
pierrenode wants to merge 1 commit into
Open
Conversation
… UNION ALL) list_sessions_rich's order_by_last_active=True path builds a recursive "chain" CTE that walks compression-continuation edges forward via child.parent_session_id = cur_id, using UNION ALL. A corrupted parent chain that loops (a -> b -> a) never terminates: UNION ALL keeps re-deriving the same (root_id, cur_id) pairs forever since it doesn't dedup the working set. This is the same class of bug just fixed in the sibling _session_latest_descendant query (1e2ad17, "NousResearch#39140 CTE ... recurses forever if a corrupted parent chain loops") — a separate, pre-existing recursive CTE in this file that fix didn't touch. list_sessions_rich is used far more broadly (dashboard, CLI, TUI, ACP adapter, ~20 call sites), so a corrupted/cyclic compression chain reaching this path would hang whichever caller invoked it, not just one endpoint. Fix: UNION instead of UNION ALL. A legitimate acyclic chain never produces a duplicate (root_id, cur_id) pair (parent_session_id is single-valued per row), so this is a no-op for correct data and only changes behavior for the corrupted-cycle case. Tests: tests/test_hermes_state.py -- builds a 2-session cycle (cyc-a <-> cyc-b, both end_reason='compression', mutually pointing at each other via parent_session_id) and asserts list_sessions_rich(..., order_by_last_active=True, include_children=True) returns instead of hanging. Mutation-verified empirically, not just by code inspection: reverting the fix and running the new test with a bounded 8s timeout confirms it actually hangs (times out) against the pre-fix code; a raw-SQL reproduction against the exact cyclic fixture data independently confirmed the same UNION ALL query never returns. With the fix, the same test passes in under 0.1s. Full hermes_state.py (340), session-listing/filters/archiving (166), and web_server.py (366) suites pass.
tonydwb
reviewed
Jul 9, 2026
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Looks Good
- Fixes duplicate results in
list_sessions_richby changingUNIONtoUNION ALLin the recursive CTE - Scoped SQLite query fix, no side effects
- No security concerns
Contributor
|
Thanks for the focused regression fix. Current main still has the recursive Sibling recursive session traversals already use Automated hermes-sweeper review. |
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
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.
Summary
list_sessions_rich'sorder_by_last_active=Truepath builds a recursivechainCTE that walks compression-continuation edges forward viachild.parent_session_id = cur_id, usingUNION ALL. A corrupted parent chain that loops (a -> b -> a) never terminates:UNION ALLkeeps re-deriving the same(root_id, cur_id)pairs forever since it doesn't dedup the working set.This is the same class of bug just fixed in the sibling
_session_latest_descendantquery (1e2ad17, "#39140 CTE ... recurses forever if a corrupted parent chain loops") — a separate, pre-existing recursive CTE in this file that fix didn't touch.list_sessions_richis used far more broadly (dashboard, CLI, TUI, ACP adapter, ~20 call sites), so a corrupted/cyclic compression chain reaching this path would hang whichever caller invoked it, not just one endpoint.Fix
UNIONinstead ofUNION ALL. A legitimate acyclic chain never produces a duplicate(root_id, cur_id)pair (parent_session_idis single-valued per row), so this is a no-op for correct data and only changes behavior for the corrupted-cycle case.Tests
tests/test_hermes_state.py— builds a 2-session cycle (cyc-a <-> cyc-b, bothend_reason='compression', mutually pointing at each other viaparent_session_id) and assertslist_sessions_rich(..., order_by_last_active=True, include_children=True)returns instead of hanging.Test plan
UNION ALLquery never returns. With the fix, the same test passes in under 0.1s.hermes_state.pysuite (340 tests) passeshermes_cli/web_server.pyfull suite (366 tests) passesruff checkclean on both changed files