Skip to content

fix(tui): release active-session lease immediately on session detach, not after orphan-reap delay (#68920) - #68947

Closed
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/68920-session-lease-leak
Closed

fix(tui): release active-session lease immediately on session detach, not after orphan-reap delay (#68920)#68947
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/68920-session-lease-leak

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Closes #68920

Problem

Desktop/TUI sessions leak active-session leases. When user does /new, the old session is detached with a 20s orphan-reap delay, but the lease stays active during that window. Multiple rapid /new commands exhaust max_concurrent_sessions.

Fix

Release the lease immediately on session detach, before the orphan-reap timer starts. _release_active_session_slot is idempotent.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/sessions Session lifecycle, resume, persistence, history needs-decision Awaiting maintainer decision before any implementation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 21, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related competing lease fixes for #68920: #68949 reclaims only stale owner-tagged leases before acquire, and #57059 changes lazy/idle lease timing. This PR releases at detach, so a maintainer should choose the intended lease lifecycle before merge.

webtecnica and others added 2 commits July 21, 2026 20:35
Previously _count_skills() only counted SKILL.md files inside the
profile's own skills/ directory, making the WebUI profile card show
a misleading low count (e.g. 0 for 'default', 30 for 'webtecnica')
even though the profile loaded 150+ skills from global + external
dirs.

Now it scans three sources:
1. Profile-specific skills/ dir (as before)
2. Global ~/.hermes/skills/ dir (via get_default_hermes_root)
3. External dirs from skills.external_dirs config

Deduplication by skill name (from YAML frontmatter) prevents double-
counting when the same skill exists in both global and profile dirs,
matching how scan_skill_commands() loads skills at runtime.

The cache is updated to key on all scanned directories and track
their combined mtime signatures.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the lease release to the disconnect lifecycle.

Problems

  • The new call is only in _close_sessions_for_transport (tui_gateway/server.py:907 in this PR), the WebSocket-disconnect path. Desktop New Chat instead clears its renderer-side active session (apps/desktop/src/app/session/hooks/use-session-actions/index.ts:310-314) and later creates a backend session on send (:371), so this diff does not yet demonstrate that it fixes the stated /new reproduction.
  • Current main already claims TUI capacity only on first prompt (tui_gateway/server.py:521-545, 11163-11164; e35c2f604), covering the rapid unsubmitted-draft mechanism described in [Bug]: Desktop/TUI sessions leak active-session leases — max_concurrent_sessions blocks new sessions over time #68920.
  • hermes_cli/profiles.py contains an unrelated skill-count change, and the PR has no detach/reconnect lease regression test.

Suggested changes

  • Split the profile-count change, then add a focused detach-before-orphan-reap test that also verifies resume plus next-turn reacquisition.
  • Either demonstrate that /new reaches this disconnect path or re-scope the PR to the WebSocket orphan-grace lease contract.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Two PRs address #68920 at different lifecycle points: #68947 releases leases on WebSocket detach, while #68949 reclaims same-process, owner-tagged leases after their sessions disappear from the live registry. The reported zombie-lease mechanism is now implemented on main by e35c2f6049633d3b6d897d63e637fc63b1b8b8c6, with reconciliation at tui_gateway/server.py:1023, reclamation at hermes_cli/active_sessions.py:390, and coverage at tests/hermes_cli/test_active_sessions.py:359, according to the automated close verdict.

Related pull requests

Duplicates

#68947 and #68949 are complementary, not strict duplicates: #68947 releases on clean WebSocket detach, whereas #68949 reclaims leases when release never ran; the current main implementation follows the live-registry reconciliation approach represented by #68949.

Suggested consolidation

Keep #68947 open with a salvage path, consistent with its automated keep-open review: rebase it onto current main, split out the unrelated hermes_cli/profiles.py changes, re-scope it to the WebSocket orphan-grace lease contract, and add the requested detach/resume/next-turn-reacquisition test. Leave #68949 closed as the author-superseded reference; no duplicate closure is needed between these complementary PRs.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I68920(["issue #68920 (closed)"])
    P68947["PR #68947 (open)"]
    P68947 -->|best fix| I68920
    class I68920 closed
    class P68947 open
    class P68947 best
    class P68947 target
    click I68920 "https://github.com/NousResearch/hermes-agent/issues/68920"
    click P68947 "https://github.com/NousResearch/hermes-agent/pull/68947"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 7 kB of issue/PR text, 4 kB of discussion (7 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@teknium1

Copy link
Copy Markdown
Contributor

Closing: this was fixed differently on main by e35c2f6 ('claim the cap slot on first turn, not on open') — lease acquisition moved to the first real turn, so parked tabs and rapid /new no longer hold slots at all and the 20s-window leak this PR targeted can no longer occur (#68920 was closed on the same basis). The branch also picked up unrelated hermes_cli/profiles.py changes. Thanks for the analysis @webtecnica — the diagnosis of the lease window was correct.

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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]: Desktop/TUI sessions leak active-session leases — max_concurrent_sessions blocks new sessions over time

4 participants