Skip to content

fix(state): compute message_count from messages table (#39734) - #39896

Closed
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:auto-fix/issue-39734
Closed

fix(state): compute message_count from messages table (#39734)#39896
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:auto-fix/issue-39734

Conversation

@kyssta-exe

Copy link
Copy Markdown
Contributor

Fixes #39734. The denormalized sessions.message_count column can lag behind the actual messages table after a partial write, crash, or race condition, causing session search/browse to report message_count=0 and empty preview for sessions that actually had persisted messages. This computes message_count via a correlated subquery on the messages table in list_sessions_rich and _get_session_rich_row, overriding the stale denormalized value.

…s_rich (NousResearch#39734)

The denormalized sessions.message_count column can lag behind the actual
messages table after a partial write, crash, or race condition. This caused
session search/browse to report message_count=0 and empty preview for
sessions that actually had persisted messages.

Compute message_count via a correlated subquery on the messages table in
list_sessions_rich and _get_session_rich_row, overriding the stale
denormalized value. This ensures the browse/read APIs always agree with
direct message table counts and FTS discovery.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 5, 2026
@kyssta-exe

Copy link
Copy Markdown
Contributor Author

Stale — oldest open PR, no merge activity for weeks.

@kyssta-exe kyssta-exe closed this Jul 14, 2026
@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting a real session-history consistency concern. The current implementation needs adjustment before this can preserve current SessionDB semantics.

Problems

  • The added COUNT(*) in hermes_state.py:1761 counts inactive rows. Current main specifies that message_count tracks only the live active set in hermes_state.py:3980-3984; this would inflate compacted and rewound sessions.
  • The Python override happens after list_sessions_rich applies s.message_count >= ? at hermes_state.py:3327-3329. The TUI project tree calls this with min_message_count=1 at tui_gateway/server.py:11116-11122, so a stale-zero session would still be filtered out.
  • No regression test accompanies the change. tests/test_hermes_state.py:3685-3719 covers rich-list previews but not stale counters or inactive rows.

Suggested changes

  • Derive the count from active messages and use that same definition in filtering/counting paths.
  • Add stale-counter regressions for active rows, compacted/inactive rows, and min_message_count=1.

Automated hermes-sweeper review.

Comment thread hermes_state.py
ORDER BY m.timestamp, m.id LIMIT 1),
''
) AS _preview_raw,
(SELECT COUNT(*) FROM messages m3 WHERE m3.session_id = s.id) AS _computed_message_count,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COUNT(*) includes soft-archived and rewound rows. On current main, message_count is explicitly the live active = 1 count (hermes_state.py:3980-3984), so this needs an active-row predicate to avoid inflating compacted sessions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: session search/browse can hide recently submitted prompt as zero-message session

3 participants