Skip to content

fix(tui_gateway): stop isolated turns fencing out the parent's session lease - #103737

Closed
jakobbjelver wants to merge 1 commit into
NousResearch:mainfrom
jakobbjelver:fix/compute-host-lease-borrow
Closed

jakobbjelver wants to merge 1 commit into
NousResearch:mainfrom
jakobbjelver:fix/compute-host-lease-borrow

Conversation

@jakobbjelver

Copy link
Copy Markdown
Contributor

Summary

Closes #101416 (and explains the lease accumulation several reporters saw alongside it).

With dashboard.turn_isolation: true (dashboard process isolation), every new desktop session is a lazy session (no in-process agent yet), so _session_uses_compute_host routes its turns to the compute-host child process. The parent claims the session's active-session lease during prompt.submit (_ensure_active_session_slot, before routing), but the child's freshly built session record (_build_server_session → _init_session) never carried a lease — so _admit_prompt_turn re-claimed from the child's pid and was fenced out by the parent's own registry entry: _is_same_writer correctly requires the same pid AND the same live_session_id.

That is the bug in #101416 — not a WS-reconnect sid change (the reporter's hypothesis). Confirmed against live logs (agent.log, timestamps within 200ms):

16:00:18,782  lease created: 20260905_160018_e43c60 (pid 1038 = dashboard process)
16:00:18,885  agent build on thread 'compute-host-turn_0'
16:00:18,975  INFO hermes_cli.active_sessions: Refused active session 20260905_160018_e43c60: already held by pid=1038 surface=desktop
16:00:18,976  INFO tui_gateway.server: Refusing turn ... SESSION_NOT_OWNED

Every retry stacked another unreclaimable lease: the failed claim leaves the parent's entry in runtime/active_sessions.json, and _prune_dead only reclaims leases of dead processes — the dashboard pid never dies. Resumed/old sessions kept working because their in-process agent bypasses the compute host entirely, which is why the bug reads as "new sessions only".

Fix (3 small pieces, no behavior change outside isolation)

  1. compute_host_bridge._compute_host_turn_frame — the parent vouches on the frame: parent_owns_active_session_lease, derived from the session's actual lease state (single source of truth; is not None so an inert token doesn't double-vouch).
  2. compute_host.ComputeHost._run_real_turn — the child calls the new server._install_borrowed_lease immediately after resolving its session record, before the turn pipeline runs.
  3. session_lifecycle._install_borrowed_lease — installs an inert ActiveSessionLease(enabled=False, released=True) borrowed token. The slot is real and owned upstream, so the child must neither claim a second one nor be able to release/transfer the parent's: release_active_session/transfer_active_session are already no-ops on enabled=False, and released=True keeps the state inert if anything force-releases.

Design notes:

  • No exclusivity relaxation. _is_same_writer and the fail-closed refusal are untouched; the registry-level fence is covered by an explicit test.
  • Fail-closed preserved. Without the vouch (parent predates the field), the child keeps the legacy self-claim path and any conflict still fails closed with the visible refusal — never a silent second writer.
  • Teardown-safe. Child-side close paths (_finalize_session/_other_runtime_lease_guard, orphan-reap, compression transfer) no-op or sweep only the child's own pid — which never writes registry entries — so the parent's slot survives every child lifecycle.

Test plan

New tests/tui_gateway/test_compute_host_borrowed_lease.py (10 tests) drives the real child path (_build_server_session → _init_session → _run_prompt_submit → _admit_prompt_turn) with only the turn body stubbed, against the conftest-sandboxed registry:

  • THE bug(desktop): 'Session already has a live owner' on every new session creation #101416 repro, fixed: parent holds the lease → child turn admitted (turn.started/message.start), no refusal, registry untouched (1 entry, parent's lease id)
  • Negative control: no vouch + held slot → the fail-closed refusal still fires (no silent double-writer)
  • Fallback: no vouch + unowned session → child self-claims and runs (legacy path preserved)
  • Registry: same-pid/different-live_session_id still fences; inert borrow cannot release/transfer the parent's slot; borrow ordering guard (installed before _run_prompt_submit)

Verified the tests catch the bug: with the fix stashed, the repro test fails exactly as production did; with the fix, it passes.

scripts/run_tests.sh (CI-parity runner): 109 files, 1021 tests, 0 failures — includes test_active_sessions.py, test_active_session_exclusivity.py, all compute-host suites. ruff check clean on touched files; formatting left as-is on pre-existing files to keep the diff minimal (CI treats format as advisory).

Tested on macOS (Apple Silicon), Python 3.13, plus live-verified against a containerized deployment (image nousresearch/hermes-agent, single desktop surface): with the fix present, new isolated sessions run; without it, they refuse.

…n lease

With dashboard.turn_isolation enabled, every NEW desktop session routes its
turns through the compute-host child process. The parent claims the session's
active-session lease in prompt.submit, but the child's freshly built session
record carried no lease — so _admit_prompt_turn re-claimed from the child's
pid and was fenced out by the parent's own registry entry (_is_same_writer
requires the same pid AND the same live_session_id). Result: 'Session ...
already has a live owner (desktop, pid N, running 0m)' on the first message
of every new session, plus one unreclaimable lease leaked per attempt (the
owner pid is the immortal dashboard process, so _prune_dead never reclaims
it).

Fix: the parent vouches on the turn frame (parent_owns_active_session_lease,
derived from the session's actual lease state) and the child installs an
inert borrow — ActiveSessionLease(enabled=False, released=True) — before the
turn pipeline runs. Admission sees the slot as held upstream; the child can
never release or transfer the parent's slot; _is_same_writer and the
fail-closed refusal are untouched. Without the vouch (parent predates the
field) the child keeps the legacy self-claim path.

Closes NousResearch#101416

@andrexibiza andrexibiza 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.

Reviewed exact head 360292689549830cc086ff2ec018909ce8e176c5 against current main@9dd6634c5635321cf38840cc30e9b51226689128, including all four changed files, the current active-session registry/transfer/finalize implementation, the compute-host completion path, compression/session-id rotation, issue #101416 and sibling #101415, merged exclusivity foundation #99719/#94595, and the earlier open isolation carrier #101501. This branch is one commit ahead / 14 commits behind current main from merge base 370ad21deac86c109de243114fe007e75cb9516b; those 14 landing-edge commits do not touch this PR's four files, so I did not find a current merge-edge code collision.

The production diagnosis here is real, and the narrow first-turn fix is pointed in the right direction. In particular, installing the borrowed token before _run_prompt_submit preserves the existing admission chokepoint, and the no-vouch negative control correctly keeps the conflict path fail-closed. The current regression also meaningfully pins the reported #101416 failure rather than weakening _is_same_writer. I would not merge this head yet, though, because the ownership handoff stops one layer too early: two lifecycle paths can still reopen the single-writer defect class that #99719 made unconditional.

Blocker 1 — the canonical parent lease is still released while an isolated child may be writing

The PR's teardown argument only covers what the child's borrowed token can do. The real registry authority lives in the serving process, and its close path is unchanged.

tui_gateway/session_lifecycle.py::_teardown_popped_session waits only for session["_run_thread"]. An isolated compute-host turn does not run on that parent-side thread; the work is in the child process. For an explicit session.close / tui_close, teardown therefore reaches _finalize_session without a child-completion receipt, and _finalize_session releases the serving process's real active_session_lease on the normal non-automatic-close path.

At that instant the child can still be inside _run_prompt_submit, but its ActiveSessionLease(enabled=False, released=True) is intentionally non-authoritative. A second backend can now acquire the same stored session id and run concurrently. That is the exact double-writer state #99719 (salvaging Futahua's #94595 work) closed: two writers can reason from histories that do not contain each other's work.

The UI/RPC close is allowed to be bounded; ownership is not allowed to expire merely because that bound elapsed. The canonical enabled lease needs to remain held until there is a correlated turn.end / turn.error settlement or observed child death (or the child is deterministically terminated and death is observed). A detached/deferred owner record is one valid shape, provided the orphan/reaper path also treats that detached lease as live authority. Please add a registry-level regression showing: close returns while the isolated child is still live; a distinct writer remains refused; only after child settlement/death can the session be acquired again.

This is not speculative architecture work: Bergmann89's earlier #101501 was reviewed for this exact hole and its current head carries the detached/deferred canonical-lease lifetime plus turn-done/child-death release. That PR is now far behind current main and non-mergeable, so I am not arguing that it should mechanically land first; I am saying this current-layout carrier should preserve that already-developed ownership contract and credit rather than regress to the narrower handoff.

Blocker 2 — released=True makes compression escape the borrow and split authority across A/B

The borrowed token's exact state matters here. hermes_cli.active_sessions.transfer_active_session() returns False immediately when lease.released is true. The token installed by this PR is deliberately released=True.

When an isolated child compresses and rotates stored session id A -> B, session_compression._sync_session_key_after_compress() calls _transfer_active_session_slot(...). With this borrowed token:

  1. transfer_active_session(borrow, session_id=B) returns False because borrow.released is True.
  2. _transfer_active_session_slot sees track_liveness=False on the borrow and falls through to its reserve-before-release fallback.
  3. That fallback calls _claim_active_session_slot(B, ...) from the compute-host child process, creating a real registry lease for B under the child pid.
  4. The child then reports session_key=B in turn.end; _compute_host_adopt_frame_meta updates the parent's in-memory session_key to B, but nothing transfers the parent's real enabled lease, which is still keyed on A.

So the PR body's claim that child-side compression transfer is harmless/no-op and the child never writes a registry entry is not true on the current implementation. Authority is now split: parent owns A; child owns B.

There is a worse second-order failure after child restart. _compute_host_turn_frame vouches with only session.get("active_session_lease") is not None. After the rotation the parent still has some lease object, but that lease proves ownership of A, while the parent session now names B. A replacement compute-host child can therefore receive parent_owns_active_session_lease=True for B and install a borrowed B token without performing a registry claim. Once another process touches the B registry it can prune the dead prior-child lease and acquire B while the replacement child is already writing under the false vouch. That restores the silent second-writer window.

The current integration test cannot catch this because its fixture explicitly replaces _sync_session_key_after_compress with a no-op. The missing test is the real A→B path across the process boundary, including a child-restart control.

The authority needs to stay singular. Before the child publishes/continues under B, it should propose the rotation to the canonical lease owner, and the serving process should atomically transfer the real enabled lease A→B and acknowledge that commit. The admission proof also needs to be qualified — at minimum lease_id + exact admitted session_id + generation/nonce — rather than a bare boolean derived from the existence of any lease object. A stale A lease must never authorize a borrow for B. Again, #101501's current head contains an earlier implementation of this exact child-proposes / owner-transfers / owner-acks shape and generation-bound admission; that prior work should be preserved/credited if this PR becomes the current carrier.

Interlock / attribution / landing order

I would classify the graph this way:

  • #99719 is the merged canonical single-writer foundation, salvaging Futahua/#94595 with authorship preserved. Its (pid, live_session_id) fence and fail-closed coordination are invariants to preserve, not something this PR should relax.
  • #101416 is the live bug this PR reproduces. Jakob's Sep 5 compute-host-child diagnosis is materially useful and corrects the original WS-reconnect hypothesis.
  • #101415 is adjacent/resolved same-symptom work (self-orphan cleanup), not evidence that same-owner fencing should be weakened.
  • #101501 (Bergmann89) predates this PR and overlaps the isolated-turn admission handoff, but also contains the two ownership layers missing here: deferred canonical-lease lifetime and owner-side compression re-anchor. It is stale against today's landing graph, so #103737 can reasonably become the current-layout carrier — but only as an explicit superseding/current-port relationship that preserves Bergmann89's contribution, not as if the broader design were new or unnecessary.

Do not merge both overlapping admission implementations independently. Pick the landing carrier, preserve the prior credit, and retain the complete invariant.

Verification gate

Exact head 360292689549830cc086ff2ec018909ce8e176c5 currently has CI, Docker Build/Test/Publish, and Nix flake check all concluded action_required; the CI run currently exposes zero jobs. The local scripts/run_tests.sh result reported in the PR is useful evidence, but there is no hosted exact-head green receipt yet. Because this PR has one surviving commit, that one commit must be green before it satisfies the repository acceptance bar.

The core repro and the admission-order regression are good work, and the current-main shape is much easier to land than the older carrier. Close the two authority/lifetime holes, preserve the prior isolation work's attribution, and get the exact head green; then this can be a strong current carrier for #101416. 🚀

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 5, 2026
teknium1 added a commit that referenced this pull request Sep 17, 2026
… compression rotation

The borrowed token from #103737 was released=True, so when the child compressed
and rotated the stored id A->B, transfer_active_session() returned False and
_transfer_active_session_slot fell through to a REAL registry claim under the
child pid: parent owned A, child owned B (split authority). A bare-boolean vouch
then let a replacement child borrow B on the strength of the stale A lease.

- The borrow is enabled=False but NOT released: release() is a no-op and
  transfer only retargets the token locally, so the child never writes the
  registry.
- The parent vouches with {lease_id, session_id} and the child installs the
  borrow only when that session_id is the admitted stored id.
- The parent re-anchors its real lease A->B where it already adopts the child's
  rotated session_key (_compute_host_adopt_frame_meta: turn.end and compress
  acks), so authority stays singular and keyed on the live continuation.

Owner-side re-anchor + qualified admission identity follow the design in
Bergmann89's #101501.

Co-authored-by: Bergmann89 <info@bergmann89.de>
Co-authored-by: Jakob Bjelvér <jakobbjelver@gmail.com>
teknium1 added a commit that referenced this pull request Sep 17, 2026
Replaces the 383-line suite from #103737 with four invariants: the real
child turn path admits under the parent's lease (registry unchanged) and still
fails closed on a vouch for another stored id; a child-side A->B rotation
never claims and the parent re-anchors its real lease; session.close keeps the
lease refusing a distinct writer until the isolated turn settles.
teknium1 added a commit that referenced this pull request Sep 17, 2026
… compression rotation

The borrowed token from #103737 was released=True, so when the child compressed
and rotated the stored id A->B, transfer_active_session() returned False and
_transfer_active_session_slot fell through to a REAL registry claim under the
child pid: parent owned A, child owned B (split authority). A bare-boolean vouch
then let a replacement child borrow B on the strength of the stale A lease.

- The borrow is enabled=False but NOT released: release() is a no-op and
  transfer only retargets the token locally, so the child never writes the
  registry.
- The parent vouches with {lease_id, session_id} and the child installs the
  borrow only when that session_id is the admitted stored id.
- The parent re-anchors its real lease A->B where it already adopts the child's
  rotated session_key (_compute_host_adopt_frame_meta: turn.end and compress
  acks), so authority stays singular and keyed on the live continuation.

Owner-side re-anchor + qualified admission identity follow the design in
Bergmann89's #101501.

Co-authored-by: Bergmann89 <info@bergmann89.de>
Co-authored-by: Jakob Bjelvér <jakobbjelver@gmail.com>
teknium1 added a commit that referenced this pull request Sep 17, 2026
Replaces the 383-line suite from #103737 with four invariants: the real
child turn path admits under the parent's lease (registry unchanged) and still
fails closed on a vouch for another stored id; a child-side A->B rotation
never claims and the parent re-anchors its real lease; session.close keeps the
lease refusing a distinct writer until the isolated turn settles.
@teknium1

Copy link
Copy Markdown
Collaborator

Landed on main via #113974 with your commit cherry-picked as-is (b3973490654, authorship preserved) — thank you @jakobbjelver, the compute-host-child diagnosis was exactly right. On top of it, #113974 closes the two blockers from @andrexibiza's review: the borrow is now enabled=False but not released, the vouch is {lease_id, session_id} and the parent re-anchors its real lease on A→B rotation (ecefbfa6e1c); session.close holds the canonical lease until the isolated turn settles or the child dies (baeebe4a60f, following @Bergmann89's #101501 design, co-authored). Live-proven with a real compute-host child (base fires the refusal, head runs with one registry entry; close-while-live keeps a foreign acquire refused; SIGKILL releases). Closing this carrier as landed.

@teknium1 teknium1 closed this Sep 17, 2026
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/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists 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): 'Session already has a live owner' on every new session creation

4 participants