docs(desktop): add PTY supervisor implementation plan#803
Closed
Kitenite wants to merge 1 commit into
Closed
Conversation
Add RFC document outlining the architecture for splitting the terminal daemon into two processes (daemon + supervisor) to allow daemon updates without killing running shell processes. The plan includes: - Architecture diagrams and responsibility split - 4 implementation phases - IPC protocol definition - Daemon upgrade flow - File changes summary and effort estimate - Tradeoffs vs enhanced cold restore alternative
Contributor
📝 WalkthroughWalkthroughImplements a two-process PTY architecture by introducing a Supervisor process that owns PTYs and sessions, while the Daemon becomes a stateless proxy. Includes new supervisor subsystem, terminal-host reorganization, and phased integration plan. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Daemon as Daemon<br/>(Stateless Proxy)
participant Supervisor as Supervisor<br/>(PTY Owner)
App->>Daemon: Create Session Request
Daemon->>Supervisor: Forward to Supervisor via IPC
Supervisor->>Supervisor: Create PTY<br/>& Session
Supervisor-->>Daemon: Session Handle
Daemon-->>App: Session Handle
App->>Daemon: Write to Terminal
Daemon->>Supervisor: Proxy Write Operation
Supervisor->>Supervisor: Write to PTY
Supervisor-->>Daemon: Operation Complete
Daemon-->>App: Response
Note over Daemon,Supervisor: Daemon restarts<br/>without data loss
Daemon->>Supervisor: Reconnect & Sync
Supervisor-->>Daemon: Buffered Operations
Daemon-->>App: Resume Stream
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/docs/PTY_SUPERVISOR_PLAN.md`:
- Around line 33-49: Add explicit IPC access-control requirements to the plan:
specify socket file ownership and restrictive permissions/umask for the
supervisor socket (~/.superset/pty-supervisor.sock) and terminal-host socket
(~/.superset/terminal-host.sock), require credential verification (validate peer
UID/PID) on accept, and define an optional shared secret or signed handshake
exchanged by SupervisorClient <-> Terminal Host Daemon before any session
operations; document where checks occur (PTY Supervisor, Session,
HeadlessEmulator, pty-subprocess lifecycle and node-pty PTY creation) and
failure-handling policy (reject/reset connection and log security events).
- Around line 33-41: The document currently lists Unix socket paths
(~/.superset/terminal-host.sock and ~/.superset/pty-supervisor.sock) for the
Terminal Host Daemon / SupervisorClient but doesn’t mention Windows; update the
PTY_SUPERVISOR_PLAN to explicitly call out Windows transport by adding a note
that on Windows the daemon and SupervisorClient will use named pipes (e.g.,
\\.\pipe\superset\terminal-host and \\.\pipe\superset\pty-supervisor or a
documented app-specific pipe namespace), include any path/namespace conventions
and permission/authentication considerations, and mention fallback behavior or
detection logic so the implementation (Terminal Host Daemon and
SupervisorClient) can choose the appropriate transport per-OS.
- Around line 14-24: The fenced code blocks in PTY_SUPERVISOR_PLAN.md are
missing language identifiers (tripping MD040); update each triple-backtick fence
that contains the ASCII diagrams and listings (e.g., the blocks starting with
"CURRENT:/NEW:" diagram, the large Terminal/Daemon/PTY diagram, the
apps/desktop/src/main/pty-supervisor/ tree, the
apps/desktop/src/main/terminal-host/ tree, and the numbered upgrade flow block)
by adding "text" (or another appropriate language) immediately after the opening
``` so each fenced block becomes ```text; ensure every fenced block in the
ranges flagged (lines with the diagrams and trees) is updated consistently.
🧹 Nitpick comments (1)
apps/desktop/docs/PTY_SUPERVISOR_PLAN.md (1)
35-38: Handshake should include an explicit protocol version.
The architecture mentions protocol versioning, but the IPChelloonly carriesdaemonVersion. Add aprotocolVersion(or feature flags) so mixed versions can negotiate safely.Suggested doc update
type SupervisorRequest = - | { type: "hello"; daemonPid: number; daemonVersion: string } + | { type: "hello"; daemonPid: number; daemonVersion: string; protocolVersion: number } | { type: "createSession"; sessionId: string; workspaceId: string; paneId: string; cwd: string; shell: string; env: Record<string, string>; cols: number; rows: number } | { type: "attachSession"; sessionId: string } | { type: "detachSession"; sessionId: string } | { type: "write"; sessionId: string; data: string } | { type: "resize"; sessionId: string; cols: number; rows: number } | { type: "getSnapshot"; sessionId: string } | { type: "killSession"; sessionId: string; signal?: string } | { type: "listSessions" } type HelloResponse = { supervisorPid: number; supervisorVersion: string; + protocolVersion: number; sessions: SessionInfo[]; // Existing sessions for reconnection }Also applies to: 122-139
Comment on lines
+14
to
+24
| ``` | ||
| CURRENT: | ||
| App → Daemon → pty-subprocess → PTY | ||
| ↓ | ||
| (dies on update, kills PTYs) | ||
|
|
||
| NEW: | ||
| App → Daemon → Supervisor → pty-subprocess → PTY | ||
| ↓ ↓ | ||
| (can restart) (stays alive, PTYs survive) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add fenced code block languages to satisfy markdownlint (MD040).
This is flagged by static analysis; add text (or a more specific language if applicable) to each fenced block.
Suggested fix
-```
+```text
CURRENT:
App → Daemon → pty-subprocess → PTY
↓
(dies on update, kills PTYs)
NEW:
App → Daemon → Supervisor → pty-subprocess → PTY
↓ ↓
(can restart) (stays alive, PTYs survive)
-```
+```
-```
+```text
┌─────────────────────────────────────────────────────────────┐
│ Electron App │
│ └── TerminalHostClient │
└──────────────┬──────────────────────────────────────────────┘
│ Unix socket: ~/.superset/terminal-host.sock
┌──────────────▼──────────────────────────────────────────────┐
│ Terminal Host Daemon ← Can restart on app update │
│ - Client authentication │
│ - Protocol versioning │
│ - Event routing │
│ └── SupervisorClient │
└──────────────┬──────────────────────────────────────────────┘
│ Unix socket: ~/.superset/pty-supervisor.sock
┌──────────────▼──────────────────────────────────────────────┐
│ PTY Supervisor ← Rarely restarts │
│ - Session lifecycle │
│ - HeadlessEmulator state │
│ - pty-subprocess management │
│ └── Session │
│ └── pty-subprocess │
│ └── node-pty PTY │
└─────────────────────────────────────────────────────────────┘
-```
+```
-```
+```text
apps/desktop/src/main/pty-supervisor/
├── index.ts # Entry point, socket server
├── supervisor.ts # Session management (from terminal-host.ts)
├── session.ts # Session class (from terminal-host/session.ts)
└── types.ts # Supervisor IPC protocol
-```
+```
-```
+```text
apps/desktop/src/main/terminal-host/
├── index.ts # Simplified - proxy only
└── supervisor-client.ts # NEW - connection to supervisor
-```
+```
-```
+```text
1. App v2 launches, detects daemon v1 running
2. App sends "shutdown" to daemon v1
┌────────┐ ┌──────────┐ ┌────────────┐
│ App v2 │────▶│ Daemon v1│ │ Supervisor │
└────────┘ └────┬─────┘ └────────────┘
│ shutdown │
▼ │ (stays alive)
💀 │
3. App spawns daemon v2, connects to existing supervisor
┌────────┐ ┌──────────┐ ┌────────────┐
│ App v2 │────▶│ Daemon v2│────▶│ Supervisor │
└────────┘ └──────────┘ └────────────┘
4. Daemon v2 sends "hello", gets session list
5. Daemon v2 attaches to sessions, gets snapshots
6. App resubscribes - terminals continue seamlessly
(Running shells never noticed anything)
-```
+```Also applies to: 28-51, 66-72, 85-89, 154-176
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@apps/desktop/docs/PTY_SUPERVISOR_PLAN.md` around lines 14 - 24, The fenced
code blocks in PTY_SUPERVISOR_PLAN.md are missing language identifiers (tripping
MD040); update each triple-backtick fence that contains the ASCII diagrams and
listings (e.g., the blocks starting with "CURRENT:/NEW:" diagram, the large
Terminal/Daemon/PTY diagram, the apps/desktop/src/main/pty-supervisor/ tree, the
apps/desktop/src/main/terminal-host/ tree, and the numbered upgrade flow block)
by adding "text" (or another appropriate language) immediately after the opening
``` so each fenced block becomes ```text; ensure every fenced block in the
ranges flagged (lines with the diagrams and trees) is updated consistently.
Comment on lines
+33
to
+49
| │ Unix socket: ~/.superset/terminal-host.sock | ||
| ┌──────────────▼──────────────────────────────────────────────┐ | ||
| │ Terminal Host Daemon ← Can restart on app update │ | ||
| │ - Client authentication │ | ||
| │ - Protocol versioning │ | ||
| │ - Event routing │ | ||
| │ └── SupervisorClient │ | ||
| └──────────────┬──────────────────────────────────────────────┘ | ||
| │ Unix socket: ~/.superset/pty-supervisor.sock | ||
| ┌──────────────▼──────────────────────────────────────────────┐ | ||
| │ PTY Supervisor ← Rarely restarts │ | ||
| │ - Session lifecycle │ | ||
| │ - HeadlessEmulator state │ | ||
| │ - pty-subprocess management │ | ||
| │ └── Session │ | ||
| │ └── pty-subprocess │ | ||
| │ └── node-pty PTY │ |
Contributor
There was a problem hiding this comment.
Define explicit IPC access-control requirements between daemon and supervisor.
Right now the plan mentions “client authentication” but doesn’t spell out how the supervisor socket is protected. This is a security risk (any local process could attach to or write into PTYs if permissions are lax). Please add explicit requirements: socket file permissions/owner/umask, credential verification (uid/pid), and/or a shared token or signed handshake.
Also applies to: 117-150
🤖 Prompt for AI Agents
In `@apps/desktop/docs/PTY_SUPERVISOR_PLAN.md` around lines 33 - 49, Add explicit
IPC access-control requirements to the plan: specify socket file ownership and
restrictive permissions/umask for the supervisor socket
(~/.superset/pty-supervisor.sock) and terminal-host socket
(~/.superset/terminal-host.sock), require credential verification (validate peer
UID/PID) on accept, and define an optional shared secret or signed handshake
exchanged by SupervisorClient <-> Terminal Host Daemon before any session
operations; document where checks occur (PTY Supervisor, Session,
HeadlessEmulator, pty-subprocess lifecycle and node-pty PTY creation) and
failure-handling policy (reject/reset connection and log security events).
Comment on lines
+33
to
+41
| │ Unix socket: ~/.superset/terminal-host.sock | ||
| ┌──────────────▼──────────────────────────────────────────────┐ | ||
| │ Terminal Host Daemon ← Can restart on app update │ | ||
| │ - Client authentication │ | ||
| │ - Protocol versioning │ | ||
| │ - Event routing │ | ||
| │ └── SupervisorClient │ | ||
| └──────────────┬──────────────────────────────────────────────┘ | ||
| │ Unix socket: ~/.superset/pty-supervisor.sock |
Contributor
There was a problem hiding this comment.
Call out Windows transport explicitly.
The socket paths are Unix‑specific; please add a note about Windows named pipes (and any path convention) so the plan is unambiguous across platforms.
🤖 Prompt for AI Agents
In `@apps/desktop/docs/PTY_SUPERVISOR_PLAN.md` around lines 33 - 41, The document
currently lists Unix socket paths (~/.superset/terminal-host.sock and
~/.superset/pty-supervisor.sock) for the Terminal Host Daemon / SupervisorClient
but doesn’t mention Windows; update the PTY_SUPERVISOR_PLAN to explicitly call
out Windows transport by adding a note that on Windows the daemon and
SupervisorClient will use named pipes (e.g., \\.\pipe\superset\terminal-host and
\\.\pipe\superset\pty-supervisor or a documented app-specific pipe namespace),
include any path/namespace conventions and permission/authentication
considerations, and mention fallback behavior or detection logic so the
implementation (Terminal Host Daemon and SupervisorClient) can choose the
appropriate transport per-OS.
Collaborator
Author
|
This is likely too complicated and the supervisor still has too much responsibility that would be updated over time. Best if we just restart the daemon or do rolling. |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Context
When the app updates, the terminal daemon restarts, killing all PTYs and shell processes. This plan documents an architecture where:
Test plan
Summary by CodeRabbit
Note: This PR contains primarily internal planning documentation with no immediate user-facing changes or feature updates.
✏️ Tip: You can customize this high-level summary in your review settings.