Skip to content

fix(desktop): release a native chat when you leave it, so its idle clock can start - #22801

Merged
brennanb2025 merged 2 commits into
mainfrom
brennanb2025/desktop-chat-unsubscribe-release
Sep 25, 2026
Merged

brennanb2025 merged 2 commits into
mainfrom
brennanb2025/desktop-chat-unsubscribe-release

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​84 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​83
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​28 $\color{#cf222e}{\Huge{\mathbf{−}}}$​33 $\color{#cf222e}{\Huge{\mathbf{−}}}$​5

ELI5

On desktop, when you leave a native chat (switch worktree or close its pane), the app tells the background process "I'm done reading this chat". That message was being ignored, so every chat you had ever opened stayed marked as "being viewed" until you reloaded or quit the app. Its agent process was never stopped when idle. Now leaving a chat really ends the read, and the idle clock can start.

What Changed

  • Before: on desktop, a chat you had viewed kept its provider process (Claude or Codex) running until the window reloaded or the app quit, however long it sat unused.
  • After: leaving a chat releases it. It is stopped after 30 idle minutes unless it still has work running (fix(native-chat): keep an idle chat alive while its subagents or background commands run #22794). Coming back re-attaches it and picks up where it left off, the same as any released chat.
  • Mechanism, src/main/ipc/runtime.ts: the runtime:subscribe handler dropped each stream's abort handle as soon as the RPC handler's promise settled. Most streaming handlers, including agentSession.subscribe, return right after setup and keep streaming until their signal aborts. So by the time the renderer sent runtime:unsubscribe, there was nothing left to abort. The handle now lives until runtime:unsubscribe or the window's retirement (reload, crash, close), which is when the renderer is actually done with the stream.
  • session.tabs.subscribeAll stopped listening for that abort once its first inventory was sent. It now routes the abort to its own cleanup for its whole life, so turning structured chat off no longer leaves a host listener running. The three chat streams (agentSession.subscribe, subscribeStatus, subscribeTurnCompletions) also return after setup, but they already listened for the abort for their whole life, so they needed no change.
  • Corrects the idle window stated in a comment in use-structured-agent-session-transport.ts (it said 15 s; it is 30 minutes).

Why

The bug was in the transport, not in any one method, so the fix is in the transport. Every desktop stream that ends by abort was affected the same way. A per-method unsubscribe RPC for chats would have added a second release path beside the abort that the handlers already wire up.

Orca's other transports reach the same outcome by a different mechanism. A paired desktop's remote subscription closes its own socket, which runs the stream's cleanup. On the mobile websocket, the per-request abort only covers setup; its streams end through the connection's cleanup or a per-method unsubscribe call. The desktop renderer is one long-lived connection, so it needs a per-stream end, and the stream's abort is the one every desktop handler already honors.

What this newly triggers: I audited every desktop stream that goes through this path (agentSession.subscribe, agentSession.subscribeStatus, agentSession.subscribeTurnCompletions, session.tabs.subscribeAll). Each one's cleanup key includes the per-subscription id (desktop-<uuid>), so ending one stream can never tear down another, including React StrictMode's unmount-and-remount. The chat streams use owned cleanup registrations, which ignore a stale release.

Linked Issue

No separate issue. Found while fixing #22794, which this depends on.

Merge order: #22794 before this PR. This PR now targets main directly and no longer carries #22794's commits. Each of these fixes something this PR would otherwise expose on every desktop chat you leave:

Visual Proof

Live Electron QA on an isolated dev instance (hidden window, CDP), macOS, real Claude Code sessions. The idle window was shortened to 45 s in the QA build only. "Before" builds swap in one file set from main at a time and restart the app. Process liveness comes from ps on the provider pid the app spawned.

Not visual on its own: the change is when the provider process stops after you leave a chat.

Build Leave the chat (switch worktree), then
Before (desktop unsubscribe from main) provider still ALIVE at +30, +60, +90 and +150 s
After provider ALIVE at +30 s, DEAD at +60 s (45 s QA window)

After, coming back to the stopped chat re-attaches it: history intact, and a new provider process answers the next send.

after-return

Testing

  • runtime-subscribe-lifecycle.test.ts: two new cases in which the dispatched handler returns right after setup, as real streaming handlers do. An explicit unsubscribe aborts it, and so does a navigation. The existing fake stream only settled on abort, which is why this was never caught. Both new cases fail without the fix.

  • session-tabs-inventory-rpc.test.ts: a set-up subscribeAll stream ends (host listener removed, end sent) when its transport aborts afterwards. Fails without the fix.

  • The existing structured-agent-session-hold.test.ts case "releases a desktop subscription when its renderer transport dies" already covers the host side: the abort releases the chat's hold and the idle clock evicts it.

  • 202 test files and 2,060 tests across IPC, session tabs, native chat and the structured session host pass. pnpm tc:node, pnpm tc:web, oxlint and check:code-quality:changed are clean.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Desktop only. A paired desktop viewing a remote host already closed its subscription socket on leave. The web client uses the websocket path, which is unchanged.
  • Mobile, not changed here: the phone multiplexes its streams over one socket and does not send agentSession.unsubscribe when it leaves a chat, so that stream's hold lasts until the socket drops (for example, when the app goes to the background). It is a separate client change and is not in this PR.
  • SSH: the release happens on the execution host that owns the provider process. No wire change.
  • Performance: fewer long-lived host listeners and provider processes on desktop. No new timers or polling.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status

Verdict: ready to merge in stack order (#22802, then #22794, then this PR). Live Electron QA for the stack is being attached separately. The review made no code changes. The head is still 634e2220c9.

Review loop. One adversarial review loop ran and came back clean without needing any changes, so no further loops ran. It confirmed the root cause and that the fix removes it rather than guarding around it. Before, the desktop runtime:subscribe handler dropped each stream's abort handle as soon as the handler finished setup, so a later runtime:unsubscribe had nothing to abort. Now the handle lives until the renderer unsubscribes or the window is retired, the same lifetime as the renderer's own listener.

Streams affected. The change reaches exactly four desktop streams: agentSession.subscribe, agentSession.subscribeStatus, agentSession.subscribeTurnCompletions and session.tabs.subscribeAll. They are the only ones reachable through window.api.runtime.subscribe. Each one, when unsubscribed after setup, releases only its own registration, because every cleanup key carries the per-subscription id. Double cleanup when the window is retired is deduplicated by the subscription registry.

Streams that end on their own. A stream that sends end or an error keeps its handle until the renderer unsubscribes. Every desktop caller unsubscribes in both cases, so the map stays bounded by the subscriptions that are still live. Dropping the handle on an error frame was considered and rejected. A handler can register a cleanup and then throw, and in that case the renderer's unsubscribe is the only thing that releases it.

Mobile, web and remote. Nothing changes for phone, paired-web or remote-host streams. No wire format and no persisted state changed.

Readiness checklist. Run at the start and the end of the review. Both passed with no P0 or P1 findings.

Validation.

  • Reverting runtime.ts fails both new lifecycle tests. Reverting session-tabs-inventory.ts fails the new inventory test.
  • 137 tests in 10 related test files pass, run by explicit path.
  • pnpm tc:node and oxlint on the changed files are clean.

PR description. Two sentences are corrected. The three chat streams also return right after setup, but they already honored a late abort. The other transports reach the same outcome by a different mechanism.

Non-blocking notes, not changed:

  • session.tabs.subscribeAll releases on abort without first checking that it still owns the cleanup it releases. Today this is safe, because re-registering an id runs the previous cleanup, which removes the old listener. The chat streams' owned-registration helper would make that explicit.
  • No test pins the error-frame path: a stream that ends with an error keeps its abort handle until the renderer unsubscribes. The behavior is correct; the test is missing.

Expected change users will notice. A desktop chat you leave now has its agent process stopped after 30 idle minutes, unless a turn, subagent or background command is still running. Coming back re-attaches it, with a short start-up delay. This relies on #22802 and #22794 being merged first.

@brennanb2025
brennanb2025 force-pushed the brennanb2025/chat-idle-background-work branch from 8bbedb6 to 55a9106 Compare September 25, 2026 11:11
@brennanb2025
brennanb2025 force-pushed the brennanb2025/desktop-chat-unsubscribe-release branch from 634e222 to 2c7ab99 Compare September 25, 2026 11:11
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Live Electron QA (whole stack)

Isolated dev instance (hidden window, driven over CDP), macOS, real Claude Code sessions. The idle window was shortened to 45 s in the QA build only. A "before" build swaps one fix's files back to main and restarts the app. Liveness is ps on the provider pid the app spawned. Screenshots are in each PR's Visual Proof.

Scenario Build Result
Leave a finished chat (switch worktree) stack head 2c7ab9991b provider ALIVE +30 s, DEAD +60 s. On return: history intact, new provider starts, the next send is answered. Pass
Same without #22801 provider still ALIVE at +30/+60/+90/+150 s (never released). Old behavior reproduced
Background sleep 150 left running, then leave stack head 2c7ab9991b provider ALIVE +30…+150 s while the command runs. Command finishes, provider DEAD by +180 s. On return: task done, new send answered. Pass
Same without #22794 provider and sleep both DEAD at +60 s. Chat shows "Background task stopped · didn't finish before the previous session ended". Old bug reproduced
Leave, idle stop, come back without #22802 "Claude couldn't restart: provider close unproven" and no new provider. An earlier run showed "Could not load conversation". Old bug reproduced

The scenario that crashes Claude mid-chat was not driven live; the #22802 integration tests cover it. Windows, Linux and SSH were not run live: the changes are host-side and not platform-specific, and no wire format changed.

@brennanb2025
brennanb2025 marked this pull request as ready for review September 25, 2026 11:20

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

This stacked PR (base: brennanb2025/chat-idle-background-work) makes a desktop runtime:unsubscribe actually end the stream it names, so leaving a native chat releases its provider hold and the idle clock can start.

  • src/main/ipc/runtime.ts — drops the .finally(stop) that deleted each stream's AbortController from senderSubscriptions when the dispatch promise settled. The controller now lives until runtime:unsubscribe aborts+deletes it or sender retirement aborts+clears the map, which is exactly what the streaming handlers listen to.
  • src/main/runtime/rpc/methods/session-tabs-inventory.ts — routes the transport abort to runtime.cleanupSubscription(subscriptionId) for the stream's whole life. Previously abortInventory only aborted the census controller and the abort listener was removed once setup finished, so session.tabs.subscribeAll kept its host listener. Listener removal now lives in the registered cleanup.
  • runtime-subscribe-lifecycle.test.ts / session-tabs-inventory-rpc.test.ts — new regression coverage: two runtime:subscribe cases where the handler settles right after setup (an explicit unsubscribe and a sender navigation must still abort), and a session.tabs.subscribeAll case where a later transport abort removes the host listener and emits end.
  • use-structured-agent-session-transport.ts — comment-only correction of the stated idle window.

I confirmed the new tests are not theatre: restoring the old .finally(stop) fails both runtime:subscribe cases, and restoring the old onTransportAbort = inventoryController.abort() fails the session-tabs case. The focused suites (runtime-subscribe-lifecycle, runtime, session-tabs-inventory-rpc, structured-agent-session-hold) pass, and npx oxlint is clean on the changed files.

I also traced the transports: the mobile/remote websocket dispatch controller only aborts on socket close/error (not on handler settle), so the new session-tabs abort path cannot prematurely end a mobile stream, and cleanup stays idempotent via RuntimeSubscriptionRegistry.cleanupAndWait plus owned registrations on the chat streams.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

The desktop runtime:subscribe handler dropped each stream's abort controller as soon as the method handler's promise settled. Most streaming handlers return right after setup and keep streaming until their signal aborts, so a later runtime:unsubscribe found nothing to abort. Those streams, and anything they hold, lived until the window reloaded or closed. For a native chat that meant the chat's history stream kept the session held after the user left it, and the idle release clock never started.

The controller now lives until runtime:unsubscribe or the sender's retirement, which is when the renderer is actually done with the stream.
…rts it

session.tabs.subscribeAll stopped listening for its transport's abort once the first inventory was sent, so a desktop unsubscribe left its host listener running until the window reloaded. The abort now routes to the stream's own cleanup for its whole life. Also corrects the idle window stated in the chat transport's comment.
@brennanb2025
brennanb2025 force-pushed the brennanb2025/chat-idle-background-work branch from 55a9106 to fecd6f5 Compare September 25, 2026 17:59
@brennanb2025
brennanb2025 force-pushed the brennanb2025/desktop-chat-unsubscribe-release branch from 2c7ab99 to d716636 Compare September 25, 2026 17:59
@brennanb2025
brennanb2025 changed the base branch from brennanb2025/chat-idle-background-work to main September 25, 2026 17:59
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 88f58bf8-7ce8-4457-a6c6-24132d49eca2

📥 Commits

Reviewing files that changed from the base of the PR and between 8009939 and d716636.

📒 Files selected for processing (5)
  • src/main/ipc/runtime-subscribe-lifecycle.test.ts
  • src/main/ipc/runtime.ts
  • src/main/runtime/rpc/methods/session-tabs-inventory-rpc.test.ts
  • src/main/runtime/rpc/methods/session-tabs-inventory.ts
  • src/renderer/src/components/native-chat/use-structured-agent-session-transport.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The runtime keeps subscription controllers registered after streaming dispatch settles, until unsubscribe or sender retirement. Session inventory subscriptions now use transport abort to clean up the runtime subscription. Tests cover both lifecycle paths. A renderer comment now describes session eviction in terms of the host’s idle window.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to d7166

The subscription cleanup changes are ready to merge after normal checks; no concrete remaining risk is identified.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to d7166

The change restores cleanup when a user leaves a chat without changing who may access the subscription. One failure path may retain subscriptions until the window is retired, which could affect the desktop app’s availability.

Retained concerns

  • Medium · reliability · inferred: A failed or rejected streaming dispatch can leave its controller in the desktop sender’s subscription map until explicit unsubscribe or sender retirement. Distinct IDs could accumulate in the shared main process for a long-lived window.
Security review details

Security Blast Radius

  • inferred — Accumulated failed-subscription controllers would be scoped to a sender’s long-lived window, while their memory cost falls on the shared desktop main process. The source does not establish a remote entrypoint for this path.

Security Findings and Attack Paths

  • inferred — A renderer already able to invoke desktop runtime subscriptions could submit distinct IDs for methods rejected by streaming dispatch. Those controllers would persist without unsubscribe or sender retirement; process-exhaustion feasibility has not been demonstrated.

Trust Boundaries and Controls

  • observed — The changed path does not grant another renderer control of a sender’s controller map. The main-frame check, sender-specific unsubscribe lookup, and connection-scoped inventory registration remain in place.

Resilience and Maintainability Implications

  • inferred — Inventory cleanup checks whether initialization completed before emitting end. An abort reentering cleanup between snapshots emission and initialization could omit that event, but whether a production transport can trigger that synchronous ordering is unresolved.

Hardening Proposals

  • proposed — Distinguish successfully established streams from rejected or failed dispatches so failed subscriptions release their controller without ending streams whose setup promise has merely settled.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed and explains the user impact, implementation, rationale, testing, and platform scope. However, it does not provide the required Linked Issue entry in the Fixes # format, … Add a valid linked issue using the required Fixes #<issue-number> format. Attach distinct before-and-after visual proof for the behavior change, or explain why the requirement does not apply. Update the checklist to reflect the actual lin…
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the main change: releasing a desktop native chat when the user leaves it so the idle timer can start.
Full details: Description check

Explanation

The description is detailed and explains the user impact, implementation, rationale, testing, and platform scope. However, it does not provide the required Linked Issue entry in the Fixes # format, and the required before-and-after visual proof is incomplete because only one image is attached. Several checklist items also remain unchecked despite reported validation.

Resolution

Add a valid linked issue using the required Fixes #&lt;issue-number&gt; format. Attach distinct before-and-after visual proof for the behavior change, or explain why the requirement does not apply. Update the checklist to reflect the actual lint, typecheck, test, and build results or state that CI will cover them.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@brennanb2025
brennanb2025 merged commit 01ed4b9 into main Sep 25, 2026
32 checks passed
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Visual proof: leaving a desktop chat now lets its agent process stop

Live runs in a hidden dev instance of Orca. The instance ran main with this fix, and for "before", the same build with only this PR's two files reverted. The QA build shortens the idle window to 45 seconds (it is 30 minutes in production). Each run opens a new Claude chat on one worktree, gets a reply, then leaves by switching to another worktree. It then checks whether that chat's Claude process is still running.

Time after leaving the chat Before (fix reverted) After (this PR)
+30 s running running
+60 s still running stopped
+90 s still running stopped
+150 s still running stopped

Before: the chat you left kept its Claude process running indefinitely, well past the idle window. The chat looks identical in both builds; the difference is only whether its process is released.

Before: chat replied, then left; its process was still running 150 s later

After: once the process has stopped, coming back to the chat still shows the full history, with no "Could not load conversation" error. Sending a new message starts a new process and gets a reply.

After: returning to the stopped chat shows its history

After: a new message in the stopped chat gets a reply

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant