Skip to content

fix(desktop): latch dead-session 4001s off approval.pending and goal polls - #98568

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98554
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98554

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

When a runtime id is reaped, tui_gateway's _sess_nowait rejects every session-scoped RPC with 4001 "session not found" — a terminal condition for that id. The process.list status-stack poll already latches the dead id off (#94219), but two other pollers never received the guard:

  • store/prompts.tsreplayPendingApproval (approval.pending): no try/catch and no latch; callers swallow the rejection and it is re-driven on every gateway event.
  • store/goals.tsrefreshSessionGoal (slash.exec goal status): a bare catch {} with no latch, so it re-asks on every refresh trigger.

The result is a window whose runtime was reaped streaming rejected RPCs for the rest of its life (measured in the issue: 1.6 rejections/sec sustained, worst single id 694 rejections), with a user-visible symptom of the composer losing focus mid-keystroke while the rejected-RPC error path runs.

This PR extracts the existing latch from composer-status.ts into a dependency-free store/session-gone.ts and routes all three pollers through the same gone-set:

  • One shared set means one clear path: resetBackgroundPollingGuard is already called on every fresh-runtime rebind (use-gateway-boot.ts, composer/status-stack/index.tsx, store/gateway.ts), so a rebind resumes all three pollers and none can latch off permanently.
  • The module has no imports from ./goals, ./prompts or ./composer-status, so there is no import cycle (composer-status.ts already imports ./goals).
  • composer-status.ts re-exports isSessionGoneForBackgroundPolling and resetBackgroundPollingGuard so existing import sites are untouched.
  • Transient failures (timeouts, transport blips) still retry and rethrow exactly as before — only the terminal 4001 class latches.

Related Issue

Fixes #98554

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • apps/desktop/src/store/session-gone.ts (new): the shared gone-latch — isSessionGone, markSessionGone, isSessionGoneForBackgroundPolling, resetBackgroundPollingGuard, moved verbatim from composer-status.ts.
  • apps/desktop/src/store/composer-status.ts: inline latch replaced by the shared module; helpers re-exported so external importers are unchanged.
  • apps/desktop/src/store/goals.ts: refreshSessionGoal skips latched ids and latches on a 4001 instead of silently retrying forever.
  • apps/desktop/src/store/prompts.ts: replayPendingApproval skips latched ids, catches 4001 to latch, and rethrows anything transient.
  • apps/desktop/src/store/prompts.test.ts: regression tests — 4001 latches off re-drives, transient errors still throw and retry, a runtime rebind resumes polling.
  • apps/desktop/src/store/goals.test.ts: regression tests — 4001 latches off refresh triggers, transient errors do not latch.

How to Test

  1. cd apps/desktop && npx vitest run src/store/prompts.test.ts src/store/goals.test.ts src/store/composer-status.test.ts
    Observed result: 47 passed (3 files), including the 5 new dead-session-guard tests.
  2. npx vitest run (full desktop suite)
    Observed result: 8715 passed / 1 failed — the single failure is electron/managed-ssh-update.test.ts ("POSIX managed launcher executes the updater command…"), which fails identically on a clean upstream/main checkout (it mis-creates macOS /var/folders temp paths as workspace-relative directories), i.e. pre-existing and unrelated to this change.
  3. npx tsc -p . --noEmit && npx tsc -p tsconfig.electron.json --noEmit && npx tsc -p tsconfig.e2e.json --noEmit — all clean.
  4. npx eslint src/ electron/ — clean.

Manual shape (from the issue): open a Desktop session, let its runtime be reaped (WS drop past the orphan-reap grace), leave the window open — after this fix the gateway log shows one rejection per dead id and then silence, and typing in that window is no longer interrupted.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — N/A (desktop TypeScript change; full desktop vitest suite run instead, see How to Test)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (darwin 25.4.0, arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure TypeScript store logic, no platform APIs touched
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…polls (NousResearch#98554)

A reaped runtime rejects every session-scoped RPC with 4001 forever. The
process.list status-stack poll already latches the dead id off (NousResearch#94219),
but approval.pending (replayPendingApproval, re-driven on every gateway
event) and the goal-status slash poll (refreshSessionGoal) kept hammering
a dead id for the life of the window.

Extract the latch into a dependency-free store/session-gone.ts so all
three pollers share one gone-set and one clear path (any runtime rebind
resets all of them via the existing resetBackgroundPollingGuard callers).
composer-status.ts re-exports the helpers so existing import sites are
untouched. Transient failures (timeouts, transport blips) still retry and
rethrow exactly as before.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 30, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

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

PR #98568 — fix(desktop): latch dead-session 4001s off approval.pending and goal polls

Summary: Extracts the goneSessions latch into a new shared apps/desktop/src/store/session-gone.ts so process.list (status stack), approval.pending (replayPendingApproval), and goal status (refreshSessionGoal) share one Set and one clear path. A reaped runtime's 4001 "session not found" (tui_gateway._sess_nowait) is terminal for that id, so each poller now checks isSessionGone(sid) before the wire and calls markSessionGone(sid) on a 4001-shaped error. composer-status.ts re-exports isSessionGoneForBackgroundPolling/resetBackgroundPollingGuard for compat. Tests pin latch vs transient and the resume path.

Non-blocking observations:

  • Sharing the set (session-gone.ts:29) fixes the single-clear-path bug: composer-status.ts previously held the only latch, so approval.pending and goal kept hammering (observed steady 4001 stream per reaped window). Now a single resetBackgroundPollingGuard(sid) clears all three pollers at once — crucial when a fresh runtime rebinds (prompts.test.ts:254 pins this).
  • isSessionGoneForBackgroundPolling preserves the JsonRpcGatewayError.code===4001 strict check with message-regex fallback only when no numeric code (session-gone.ts:43-50). This avoids misclassifying unrelated "session not found" substrings when a typed error is available, while still catching unwrapped errors where the frame structure was lost — correct per feat(tui-gateway): seq-stamped event replay for lossless desktop WS reconnect #94219.
  • Early-exit order is correct: if (!sid || !gateway || isSessionGone(sid)) return (goals.ts:162, composer-status.ts:69) and if (isSessionGone(sessionId)) return before gateway.request in prompts.ts:286 ensures no wire call for a latched id. replayPendingApproval still throws transient errors (prompts.ts:303) so callers keep retry semantics; refreshSessionGoal swallows transients as before (goals.ts:180) — both intentional differences pinned by tests.
  • Re-export (composer-status.ts:62) keeps existing importers untouched; new direct imports in goals.ts/prompts.ts avoid a cycle (comment in session-gone.ts:324-326 notes no imports from consumers).
  • resetBackgroundPollingGuard() with no arg clears all (session-gone.ts:365-373) — only used in tests, safe.

Verdict: Correct shared-latch extraction closing the #94219 fallout for the two remaining pollers. No blocking issues.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — glad the shared-latch extraction reads correctly. I intentionally kept the strict JsonRpcGatewayError.code===4001 check with the message-regex fallback only for unwrapped errors, and pinned the throw-vs-swallow asymmetry between replayPendingApproval and refreshSessionGoal in tests, so it's good to see those called out as correct.

@OutThisLife

Copy link
Copy Markdown
Contributor

Superseded by #99891.

That salvage keeps latching approval.pending and goal status off a dead runtime, and:

  • puts the latch in the existing runtime-gone module instead of a new session-gone.ts
  • shares the same 4001 / session-not-found helper as process.list

You're credited via Co-authored-by. Thanks for extending the gone-latch past process.list.

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

Desktop: approval.pending and goal-status polls never latch off a reaped runtime (4001 loop) — composer loses focus while typing

4 participants