Skip to content

fix(desktop): profile-switch slot-cap regression (slot-cap half of #102163) - #102463

Closed
Finn763 wants to merge 1 commit into
NousResearch:mainfrom
Finn763:fix/102163-descr
Closed

Finn763 wants to merge 1 commit into
NousResearch:mainfrom
Finn763:fix/102163-descr

Conversation

@Finn763

@Finn763 Finn763 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Addresses #102163 (slot-cap half only) — [Bug]: Profile switching regression in v0.21.0 — 4th profile timed out waiting for a free local slot. Minimal diff, single shared guard, no new deps.

Scope note

  • This PR fixes the 3-slot-cap symptom via demand displacement (stalest RUNNING pool backend is stopped so the queued spawn converges instead of timing out).
  • The session-ownership-lock symptom (Session X already has a live owner, hermes_cli/active_sessions.py) is intentionally untouched — [Bug]: Profile switching regression in v0.21.0 — slot limit 3 enforced + session ownership lock #102163 stays open for that half.
  • Tradeoff, as reviewed: displacing the stalest running backend can interrupt a background profile mid-turn. Intended: a foreground switch outranks background pressure; the session persists and reopening respawns.

Evidence

  • Repro: local repro shows before→after.
  • Related suites pass (see worktree 102163).

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 3, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

PR #102463 — demand-displace stalest pool backend instead of queue-timeout on profile switch

Verdict: Looks good. Right prioritization (interactive profile switch outranks background pool pressure) with a tightly scoped victim policy; the documented single-displacement limit keeps the blast radius small.

What the change does

  • apps/desktop/electron/pool-eviction.ts:84-117selectSlotDisplacementVictim: stalest lastActiveAt among entries holding a child process, never self (exclude), never starting/queued/descriptor entries; missing timestamps sort oldest (?? 0).
  • apps/desktop/electron/main.ts:18-41,50-52displaceStalestPoolBackendForSlot stops the victim before the waiter acquires; no-op when no running victim (genuine capacity pressure still surfaces the queue timeout).
  • apps/desktop/electron/pool-eviction.test.ts:65-74 — stalest-running picked, self excluded, self-only → null.

Non-blocking

  1. Integration gap: only the pure selector is unit-tested; the main.ts orchestration (stop-then-acquire ordering, log line, no-op passthrough) has no test. A fast switch A→B→C→D regression test at the coordinator level would lock the actual [Bug]: Profile switching regression in v0.21.0 — slot limit 3 enforced + session ownership lock #102163 symptom (4th spawn timing out) rather than the helper.
  2. main.ts:38 — after stopPoolBackend(victim) there is no explicit wait for the slot release before spawnRequest.acquired resolves; correctness relies on the coordinator observing the freed slot. If stop is asynchronous far beyond the acquire tick, the waiter could still time out in a slow-teardown corner. Consider asserting/documenting the ordering guarantee the coordinator provides.
  3. Tie-breaking uses strict <, so equal lastActiveAt keeps the first-iterated entry — deterministic per insertion order, fine. And killing a running backend drops its in-memory turn state (session persists, reopening respawns — stated); a user with unsent composer text on the victim profile could lose it. Acceptable for an explicit switch, noting the tradeoff.
  4. The ponytail comment appropriately scopes the single-displacement limit; agree a drain loop should wait for observed need.

Tests: Selector unit test covers the policy core; see point 1 for the integration level. Good.

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

This fixes the slot-cap half of #102163, but the title and body say "Fixes"/"Closes" the whole issue, and the second symptom isn't touched.

#102163 reports two separate regressions:

  • The 3-slot cap (Local backend start for "X" timed out while waiting for a free slot). This diff handles that: selectSlotDisplacementVictim and displaceStalestPoolBackendForSlot stop the stalest running backend when a spawn queues behind full slots.

  • The session-ownership lock (Session X already has a live owner (desktop, pid Y, running 10m). Only one surface at a time may run a session). The issue says this fires "even with 2–3 profiles active", below the cap, so displacement never runs in that case. The lock lives in a separate path, hermes_cli/active_sessions.py (session_already_owned_message, backed by runtime/active_sessions.json), and none of the three changed files (all under apps/desktop/electron/) touch it.

If this merges with "Closes #102163", GitHub closes the P1 while the ownership-lock half is still open. I'd scope the wording to Addresses #102163 and leave the issue open for the ownership-lock symptom.

One more thing worth a second look: displaceStalestPoolBackendForSlot stops the stalest running backend even if that profile is mid-turn. lastActiveAt being oldest only means least-recently-active, which can still be an in-flight agent. A foreground switch can interrupt another profile's active work. If that tradeoff is intended it's worth saying so in the body.

…search#102163

Slot-cap half only: demand-displace stalest running pool backend when a
spawn queues behind full slots. Session-ownership lock half
(hermes_cli/active_sessions.py) intentionally untouched — issue stays open
for that symptom. Displacement may interrupt a background profile mid-turn;
foreground switch outranks it, session persists and reopening respawns.
@Finn763 Finn763 changed the title fix: [Bug]: Profile switching regression in v0.21.0 — slot limit 3 enforced + session ownership lock (#102163) fix(desktop): profile-switch slot-cap regression (slot-cap half of #102163) Sep 6, 2026
@Finn763

Finn763 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — you were right on both points. Scoped this PR to the slot-cap half: title/body now say Addresses #102163, ownership-lock half (hermes_cli/active_sessions.py) stays open on the issue, and the mid-turn interruption tradeoff is documented in the body (intended: foreground switch outranks background, session persists). Commit message amended accordingly (new head f07e858).

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

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) 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.

5 participants