Skip to content

fix(desktop): reclaim idle pool backend for foreground dials (#102281 follow-up) - #104871

Closed
bounce12340 wants to merge 3 commits into
NousResearch:mainfrom
bounce12340:cursor/foreground-idle-reclaim-477b
Closed

bounce12340 wants to merge 3 commits into
NousResearch:mainfrom
bounce12340:cursor/foreground-idle-reclaim-477b

Conversation

@bounce12340

Copy link
Copy Markdown

Summary

Related: #102281 (follow-up to #104139; issue remains closed — still repro after merge).

Test plan

  • npx vitest run --project electron electron/pool-eviction.test.ts (10 passed per draft)
  • Gateway lease/scope tests (20 passed per draft)
  • With maxBackends=3, fill idle residents then open another bot — oldest idle reclaimed
  • Active prompt turn on a resident must not be reclaimed

cursoragent and others added 3 commits September 7, 2026 08:33
Co-authored-by: Josh Tsai <bounce12340@users.noreply.github.com>
Co-authored-by: Josh Tsai <bounce12340@users.noreply.github.com>
Co-authored-by: Josh Tsai <bounce12340@users.noreply.github.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) labels Sep 7, 2026
@Enough1122

Copy link
Copy Markdown

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

Summary

Follow-up to #102281: a foreground dial may now reclaim a keepalive-fresh but idle pool backend instead of stalling on a full pool. Freshness alone no longer implies busy — the renderer publishes an explicit activeTurn lease per backend (touchBackend gains an options param; turn-lease acquire/release and secondary keepalives report it), and selectForegroundPoolEviction only selects spawned entries with activeTurn === false, LRU-first. The conservative LRU/idle-reaper path is unchanged.

Findings

  • Non-blocking, note: apps/desktop/electron/pool-eviction.ts:182 — the strict === false (not falsy) is the safety-critical choice: pre-existing entries that never published a lease are ineligible, so an unknown-activity backend can never be reclaimed out from under a prompt turn. Fail-closed default, correctly.
  • Non-blocking, note: apps/desktop/src/store/gateway.ts:232 — deriving per-scope activeTurn from the live turnLeases map for secondary keepalives keeps the main-process view consistent without a new subscription; the void ... .catch() fire-and-forget matches existing keepalive style.
  • Non-blocking, minor: gateway.ts:232 — key.slice(0, key.indexOf(' ')) yields slice(0, -1) if a lease key ever lacks the separator, silently mistargeting the scope. Keys are constructed with the separator today, but a split-with-fallback would be more defensive.
  • Positive: the two eviction tests pin both halves of the contract — an idle fresh resident is reclaimable, an older backend with an active turn is not.

Verdict

Looks good. The explicit lease cleanly separates "interested" from "busy", with the unknown state defaulting to protected. Safe to merge.

@bounce12340

Copy link
Copy Markdown
Author

Related / complementary work: #105390 (nftpoetrist) threads foreground spawnPriority through requestGatewayForAgent / retainGatewayForAgent so those dials aren't stuck on 'background'.

This PR (#104871) is the other half of the #102281 follow-up: when the pool is already full of keepalive-fresh idle residents, reclaim the oldest idle (activeTurn === false) backend so a later foreground dial can proceed.

The other author notes no line-level overlap with this change. Happy for maintainers to review both together — they can coexist, or we can follow guidance on how to split/merge.

@austinpickett

Copy link
Copy Markdown
Collaborator

Superseded by #113280.

Your trigger point (a foreground dial at activeCount >= poolMaxBackends() before coordinator.request) and the "keepalive-fresh is not busy" diagnosis are the core of that PR and you are credited as co-author; what changed is that idleness is now proven by the backend itself (/api/health/idle: running sessions, running cron jobs, prompts waiting on a human, fail-closed on null) rather than by the renderer's activeTurn, concurrent foreground dials share one retirement behind a fence with an identity recheck after every await, and the evicted scope parks instead of redialing into the slot it just freed. Thanks @bounce12340.

OutThisLife pushed a commit that referenced this pull request Sep 17, 2026
… an admission fence

A foreground bot open against a full 3-slot local pool waited 30s for a slot
and timed out (production: 7,122 slot waits, 6,305 timeouts, ~790 cancelled
starts in 10 days on a 17-profile host). Nothing could free a slot: a child's
hard lease lives until the child exits (main.ts child 'exit' handler,
teardownFailedLocalBackend, stopPoolBackend after the bounded SIGTERM->SIGKILL
exit), while LRU eviction and the idle reaper key off lastActiveAt, which the
renderer refreshes every 60s for every open socket. A bot-tile-pinned resident
is keepalive-fresh forever. Occupied is not busy.

New `electron/pool-retire.ts` (pure, DI-testable like pool-spawn-coordinator):
a foreground dial that finds `activeCount >= poolMaxBackends()` may retire ONE
resident, under these rules:

* Proof is the backend's. Each LRU candidate is probed over
  `/api/health/idle` (running sessions + running cron jobs + prompts waiting on
  a human). Only `true` is idle; `false` and `null` (older runtime 404, probe
  error, unreadable ledger) are busy. The renderer-published `activeTurn`
  lease is an early skip, never the proof; entries no longer initialise it to
  false, so a fresh backend is not evictable by default.
* Admission fence: concurrent foreground dials share one retirement and one
  probe; the coordinator hands the freed slot to the ticket that queued first.
* Identity recheck (`pool.get(key) === entry`, no new turn lease) after every
  await, and a re-probe immediately before the stop.
* The waiter's `coordinator.request()` is issued BEFORE the stop so it sits at
  the queue head; the slot is released only by the retired child's real exit
  through the existing stopPoolBackend -> releaseLocalBackendSlot path.
* `hermes:pool:retiring` is broadcast to every window before the SIGTERM so
  the renderer parks the scope instead of redialing into the vacated slot.

main.ts grows only the trigger point, the HTTP probe, the broadcast and the
retirer construction. Kept from #104871 with credit: the trigger point before
`coordinator.request`, the `touchBackend(profile, options)` IPC widening
(preload.ts / global.d.ts), and the LRU-among-eligible selector shape.

Tests (pool-retire.test.ts): fence with two concurrent tickets over a real
LocalBackendSpawnCoordinator (one probe, one SIGTERM, slot granted only after
the simulated exit, exactly one waiter served); identity recheck aborts on a
swapped entry or a lease published after the probe; idle null / cron-running
ineligible with fall-through to the queue; renderer activeTurn:false loses to
a backend re-probe.

Co-authored-by: bounce12340 <128559392+bounce12340@users.noreply.github.com>
OutThisLife pushed a commit that referenced this pull request Sep 17, 2026
When main retires a pooled backend for a foreground open, the renderer entry
riding it is still wantOpen: the socket's 'closed' state ran
scheduleReconnect -> reconnectSecondary -> openSecondary('background') and
immediately re-queued a spawn for the slot the retirement had just freed. The
wake/focus nudge (reconnectSecondaryGateways) re-arms parked entries too, so
even a stall-parked scope would have redialed on the next window focus.

Listen for `hermes:pool:retiring` in use-gateway-boot and route it through a
new `parkSecondariesForRetiredBackend(poolKey)`: every scope riding that
child (bare profile or `conn:local::<profile>`; both ride one local child)
gets wantOpen=false, its reconnect timer cleared, and a sticky `retiredByPool`
flag that the nudge skips. The entry stays, so bot tiles keep their card
without a live socket. Only an explicit open of the scope (rearmSecondary,
reached from ensureGatewayForAgent / openGatewayForAgent /
ensureGatewayForProfile / ensureActiveGatewayOpen) clears the flag and dials
again.

Turn-lease reporting (shape from #104871): retainGatewayForSessionTurn and
touchSecondaryGateways publish `activeTurn` per scope through the widened
touchBackend IPC. On release the SCOPE's lease state is reported, not the
single lease's, so a second session on the same backend keeps it marked.

Test (gateway-connection-lifecycle.test.ts): a retired local scope does not
redial from the nudge across ~400s of fake time while an unrelated remote
scope is untouched; an explicit open re-arms it and the nudge treats it
normally afterwards. RED on base: the nudge redialed the retired scope.

Co-authored-by: bounce12340 <128559392+bounce12340@users.noreply.github.com>
OutThisLife pushed a commit that referenced this pull request Sep 17, 2026
… an admission fence

A foreground bot open against a full 3-slot local pool waited 30s for a slot
and timed out (production: 7,122 slot waits, 6,305 timeouts, ~790 cancelled
starts in 10 days on a 17-profile host). Nothing could free a slot: a child's
hard lease lives until the child exits (main.ts child 'exit' handler,
teardownFailedLocalBackend, stopPoolBackend after the bounded SIGTERM->SIGKILL
exit), while LRU eviction and the idle reaper key off lastActiveAt, which the
renderer refreshes every 60s for every open socket. A bot-tile-pinned resident
is keepalive-fresh forever. Occupied is not busy.

New `electron/pool-retire.ts` (pure, DI-testable like pool-spawn-coordinator):
a foreground dial that finds `activeCount >= poolMaxBackends()` may retire ONE
resident, under these rules:

* Proof is the backend's. Each LRU candidate is probed over
  `/api/health/idle` (running sessions + running cron jobs + prompts waiting on
  a human). Only `true` is idle; `false` and `null` (older runtime 404, probe
  error, unreadable ledger) are busy. The renderer-published `activeTurn`
  lease is an early skip, never the proof; entries no longer initialise it to
  false, so a fresh backend is not evictable by default.
* Admission fence: concurrent foreground dials share one retirement and one
  probe; the coordinator hands the freed slot to the ticket that queued first.
* Identity recheck (`pool.get(key) === entry`, no new turn lease) after every
  await, and a re-probe immediately before the stop.
* The waiter's `coordinator.request()` is issued BEFORE the stop so it sits at
  the queue head; the slot is released only by the retired child's real exit
  through the existing stopPoolBackend -> releaseLocalBackendSlot path.
* `hermes:pool:retiring` is broadcast to every window before the SIGTERM so
  the renderer parks the scope instead of redialing into the vacated slot.

main.ts grows only the trigger point, the HTTP probe, the broadcast and the
retirer construction. Kept from #104871 with credit: the trigger point before
`coordinator.request`, the `touchBackend(profile, options)` IPC widening
(preload.ts / global.d.ts), and the LRU-among-eligible selector shape.

Tests (pool-retire.test.ts): fence with two concurrent tickets over a real
LocalBackendSpawnCoordinator (one probe, one SIGTERM, slot granted only after
the simulated exit, exactly one waiter served); identity recheck aborts on a
swapped entry or a lease published after the probe; idle null / cron-running
ineligible with fall-through to the queue; renderer activeTurn:false loses to
a backend re-probe.

Co-authored-by: bounce12340 <128559392+bounce12340@users.noreply.github.com>
OutThisLife pushed a commit that referenced this pull request Sep 17, 2026
When main retires a pooled backend for a foreground open, the renderer entry
riding it is still wantOpen: the socket's 'closed' state ran
scheduleReconnect -> reconnectSecondary -> openSecondary('background') and
immediately re-queued a spawn for the slot the retirement had just freed. The
wake/focus nudge (reconnectSecondaryGateways) re-arms parked entries too, so
even a stall-parked scope would have redialed on the next window focus.

Listen for `hermes:pool:retiring` in use-gateway-boot and route it through a
new `parkSecondariesForRetiredBackend(poolKey)`: every scope riding that
child (bare profile or `conn:local::<profile>`; both ride one local child)
gets wantOpen=false, its reconnect timer cleared, and a sticky `retiredByPool`
flag that the nudge skips. The entry stays, so bot tiles keep their card
without a live socket. Only an explicit open of the scope (rearmSecondary,
reached from ensureGatewayForAgent / openGatewayForAgent /
ensureGatewayForProfile / ensureActiveGatewayOpen) clears the flag and dials
again.

Turn-lease reporting (shape from #104871): retainGatewayForSessionTurn and
touchSecondaryGateways publish `activeTurn` per scope through the widened
touchBackend IPC. On release the SCOPE's lease state is reported, not the
single lease's, so a second session on the same backend keeps it marked.

Test (gateway-connection-lifecycle.test.ts): a retired local scope does not
redial from the nudge across ~400s of fake time while an unrelated remote
scope is untouched; an explicit open re-arms it and the nudge treats it
normally afterwards. RED on base: the nudge redialed the retired scope.

Co-authored-by: bounce12340 <128559392+bounce12340@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants