From d1cdca3324fb7871b7a51e89464261d885253766 Mon Sep 17 00:00:00 2001 From: Menghuan1918 Date: Sat, 8 Aug 2026 17:24:08 +0800 Subject: [PATCH] terminal: keep the pty alive while its tab is open, whatever the view does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The close frame was sent on every unmount, so switching conversations (or refreshing) killed the shell even though the tab was still open; coming back spawned a fresh terminal. Now the unmount close frame only fires when the tab is truly gone (checked against the per-session store state), so a bare unmount hands the process to the host's reconnect grace and the next connect reattaches the SAME shell with its transcript. Closing a tab releases the quota immediately: the socket-open path keeps the close frame, and a new pty.close API covers the socket-down (reconnect loop) case that previously left a 30s zombie holding the quota. The host also records the spawn cwd and respawns when a reconnect carries a different authoritative cwd — the page-load hydrate race can attach the real session cwd after the first connect fell back to the process cwd, and a shell in the wrong directory must not linger. --- src/client/Sidebar.tsx | 15 +++++++++++-- src/client/TerminalView.tsx | 13 +++++++---- src/client/api.ts | 5 +++++ src/client/state.ts | 20 +++++++++++++++++ src/index.ts | 10 +++++++++ src/pty-manager.ts | 18 ++++++++++++---- tests/smoke.spec.ts | 43 +++++++++++++++++++++++++++++++------ tests/unit.spec.ts | 24 ++++++++++++++++++++- 8 files changed, 131 insertions(+), 17 deletions(-) diff --git a/src/client/Sidebar.tsx b/src/client/Sidebar.tsx index 285a301..d7b8a07 100644 --- a/src/client/Sidebar.tsx +++ b/src/client/Sidebar.tsx @@ -140,7 +140,18 @@ export function Sidebar(props: { ctx: Context }) { const fullscreen = state !== undefined && window.innerWidth - state.width < 8 const actions: WorkbenchActions = useMemo(() => ({ - closeTab: (paneId, tabId) => { sidebarStore.reduce(s => closeTab(s, paneId, tabId)) }, + closeTab: (paneId, tabId) => { + // A closed terminal releases its pty immediately — including when its + // socket is mid-reconnect, where the unmount close frame never reaches + // the host and the process would hold the quota until the grace ends. + const current = sidebarStore.getSnapshot().state + const leaf = current === undefined ? undefined : leafWithTab(current.splits, tabId) + const tab = leaf?.tabs.find(candidate => candidate.id === tabId) + sidebarStore.reduce(s => closeTab(s, paneId, tabId)) + if (sessionId !== undefined && tab?.type === 'terminal') { + void api.ptyClose({ sessionId, cwd }, tabId).catch(() => { /* the host may already have released it */ }) + } + }, activateTab: (paneId, tabId) => { sidebarStore.reduce(s => ({ ...s, @@ -167,7 +178,7 @@ export function Sidebar(props: { ctx: Context }) { resizeSplit: (splitId, index, deltaFrac) => { sidebarStore.reduce(s => ({ ...s, splits: resizeSplit(s.splits, splitId, index, deltaFrac) })) }, - }), []) + }), [sessionId, cwd]) if (state === undefined || sessionId === undefined) { return ( diff --git a/src/client/TerminalView.tsx b/src/client/TerminalView.tsx index ea24f03..f572c63 100644 --- a/src/client/TerminalView.tsx +++ b/src/client/TerminalView.tsx @@ -14,6 +14,7 @@ import { FitAddon } from '@xterm/addon-fit' import 'xterm/css/xterm.css' import { t } from './locales.ts' import type { SessionScope } from './api.ts' +import { sidebarStore } from './state.ts' import css from './sidebar.module.css' /** How many consecutive unreasoned failures before showing the error banner. */ @@ -125,10 +126,14 @@ export function TerminalView(props: { scope: SessionScope; tabId: string }) { window.clearTimeout(retry) observer.disconnect() inputSub.dispose() - // Tell the host the owning tab is gone so it releases the terminal - // quota immediately (best effort; a dropped socket gets the grace - // period on the host side instead). - if (socket !== null && socket.readyState === WebSocket.OPEN) { + // The close frame tells the host the owning tab is GONE (immediate + // quota release). A bare unmount — conversation switch, re-render, + // page unload — leaves the tab open, so the socket drop alone hands + // the process to the host's reconnect grace: switching back or + // refreshing reattaches the SAME shell instead of respawning one. + // (The host respawns on its own when the authoritative cwd changed.) + if (!sidebarStore.tabOpen(scope.sessionId, tabId) + && socket !== null && socket.readyState === WebSocket.OPEN) { socket.send(JSON.stringify({ type: 'close' })) } socket?.close() diff --git a/src/client/api.ts b/src/client/api.ts index 36057f8..8492a04 100644 --- a/src/client/api.ts +++ b/src/client/api.ts @@ -114,6 +114,11 @@ export const api = { call<{ ok: true }>('git.checkout', scopePayload(scope, { branch })), gitLog: (scope: SessionScope, signal?: AbortSignal) => call('git.log', scopePayload(scope, {}), signal), + /** Release a terminal's process immediately (tab closed; the WS close frame + * may be unreachable while the socket is down, so the host also accepts + * this explicit route). */ + ptyClose: (scope: SessionScope, tab: string) => + call<{ ok: true }>('pty.close', scopePayload(scope, { tab })), } /** Absolute URL of the media route for one path (images only). */ diff --git a/src/client/state.ts b/src/client/state.ts index 78b23a4..1783729 100644 --- a/src/client/state.ts +++ b/src/client/state.ts @@ -125,6 +125,11 @@ export function allLeaves(node: SplitNode): SidebarLeaf[] { return node.children.flatMap(allLeaves) } +/** Whether a tab exists anywhere in a state tree (any pane). */ +export function tabOpenIn(state: SidebarState, tabId: string): boolean { + return allLeaves(state.splits).some(leaf => leaf.tabs.some(tab => tab.id === tabId)) +} + /** Replace a leaf with a split of it plus a fresh empty leaf. */ export function splitLeafAt(node: SplitNode, paneId: string, dir: 'row' | 'col'): SplitNode { const fresh: SidebarLeaf = { kind: 'leaf', id: uid('pane'), tabs: [], active: null } @@ -506,6 +511,21 @@ export class SidebarStore { this.notify() } + /** + * Whether a tab still exists in its session's state. Views use this on + * unmount to tell "the tab was closed" (release the terminal now) from + * "the tree re-rendered / the conversation switched" (the tab is still + * open — keep the terminal alive through the host's reconnect grace). + * Checks the session's own map entry (the current snapshot may already + * point at another session when a conversation switch unmounts the old + * one's tabs). + */ + tabOpen(sessionId: string, tabId: string): boolean { + const state = this.bySession.get(sessionId) + ?? (this.snapshot.sessionId === sessionId ? this.snapshot.state : undefined) + return state !== undefined && tabOpenIn(state, tabId) + } + /** Apply a pure reducer (returns the next state). */ reduce(reducer: (state: SidebarState) => SidebarState): void { const sessionId = this.snapshot.sessionId diff --git a/src/index.ts b/src/index.ts index 9ef7464..6f63733 100644 --- a/src/index.ts +++ b/src/index.ts @@ -204,6 +204,16 @@ function buildApi(ctx: Context, ptyManager: PtyManager): Record { + const sessionId = requireString(payload, 'sessionId') + const tab = requireString(payload, 'tab') + ptyManager.close(`${sessionId}:${tab}`) + return { ok: true } + }, } } diff --git a/src/pty-manager.ts b/src/pty-manager.ts index 6322a03..9e84e04 100644 --- a/src/pty-manager.ts +++ b/src/pty-manager.ts @@ -46,6 +46,11 @@ export interface SidebarPty { key: string sessionId: string tabId: string + /** The working directory the process was SPAWNED with (a reconnect that + * resolves a different authoritative cwd respawns instead of reusing — + * the page-load hydrate race can attach the real cwd after the first + * connect, and a shell in the wrong directory must not linger). */ + cwd: string pty: nodePty.IPty /** Output accumulated since spawn (bounded; head dropped when over the limit). */ transcript: string @@ -79,9 +84,13 @@ export class PtyManager { /** * Open (or reuse) the terminal for a session/tab key. A handle whose * process already exited is replaced with a fresh spawn (reconnecting a - * dead terminal must yield a live shell, not an input sink). Reopening - * also cancels any pending scheduled close (a reconnect within the grace - * window keeps the process alive). + * dead terminal must yield a live shell, not an input sink), and so is a + * live handle whose spawn cwd differs from the now-authoritative one (the + * first connect of a page load can arrive before the session hydrates, so + * it fell back to the process cwd — reconnecting with the real cwd must + * restart the shell in the right directory). Reopening also cancels any + * pending scheduled close (a reconnect within the grace window keeps the + * process alive). * @param sessionId - conversation id. * @param tabId - client tab id. * @param cwd - initial working directory (the session's cwd). @@ -94,7 +103,7 @@ export class PtyManager { const key = `${sessionId}:${tabId}` this.cancelClose(key) const existing = this.sessions.get(key) - if (existing !== undefined && !existing.exited) return existing + if (existing !== undefined && !existing.exited && existing.cwd === cwd) return existing if (existing !== undefined) this.close(key) // Zombie cleanup: a session's exited handles (shell closed, tab dropped // on an old host without the close frame) must not eat the quota. @@ -108,6 +117,7 @@ export class PtyManager { key, sessionId, tabId, + cwd, pty: nodePty.spawn(this.shell, [], { name: 'xterm-256color', cols: Math.max(2, Math.floor(cols)), diff --git a/tests/smoke.spec.ts b/tests/smoke.spec.ts index 10e78b9..ede59f8 100644 --- a/tests/smoke.spec.ts +++ b/tests/smoke.spec.ts @@ -119,6 +119,25 @@ describe('host plugin smoke', () => { } }) + it('pty manager: reopening with a different cwd respawns in the new directory', async () => { + const manager = new PtyManager(defaultShell(), 3) + try { + const first = manager.open('s4', 't1', process.cwd(), 80, 24) + // The hydrate race: the first connect fell back to the process cwd, + // the reconnect carries the session's real cwd — the shell must move. + const second = manager.open('s4', 't1', '/tmp', 80, 24) + expect(second).not.toBe(first) + expect(second.cwd).toBe('/tmp') + expect(manager.keysOf('s4')).toHaveLength(1) + // A same-cwd reconnect reattaches without respawning. + const third = manager.open('s4', 't1', '/tmp', 80, 24) + expect(third).toBe(second) + expect(manager.keysOf('s4')).toHaveLength(1) + } finally { + manager.disposeAll() + } + }) + it('lists the repository root level', async () => { const listing = await listDirectory(process.cwd(), 1000) expect(listing.entries.some(entry => entry.name === 'src' && entry.isDir)).toBe(true) @@ -148,11 +167,15 @@ describe('session cwd resolution over the API route', () => { return routes.find(route => route.path === '/sidebar/api')! } - const invoke = async (route: SidebarWebRoute, payload: unknown): Promise<{ ok: boolean; value?: { cwd: string }; error?: { message: string } }> => { + const invoke = async ( + route: SidebarWebRoute, + method: string, + payload: unknown, + ): Promise<{ ok: boolean; value?: { cwd: string }; error?: { message: string } }> => { const body = Buffer.from(JSON.stringify(payload)) const req = { method: 'POST', - url: '/sidebar/api/session.cwd', + url: `/sidebar/api/${method}`, headers: { host: '127.0.0.1:3080' }, [Symbol.asyncIterator]: async function* () { yield body }, } as never @@ -167,14 +190,14 @@ describe('session cwd resolution over the API route', () => { it('uses the client summary cwd while the session is detached', async () => { const route = mount() - const result = await invoke(route, { sessionId: 's-detached', cwd: '/tmp/summary-cwd' }) + const result = await invoke(route, 'session.cwd', { sessionId: 's-detached', cwd: '/tmp/summary-cwd' }) expect(result.ok).toBe(true) expect(result.value?.cwd).toBe('/tmp/summary-cwd') }) it('falls back to the process cwd with no summary cwd', async () => { const route = mount() - const result = await invoke(route, { sessionId: 's-unknown' }) + const result = await invoke(route, 'session.cwd', { sessionId: 's-unknown' }) expect(result.ok).toBe(true) expect(result.value?.cwd).toBe(process.cwd()) }) @@ -185,15 +208,23 @@ describe('session cwd resolution over the API route', () => { get: (id) => id === 's-attached' ? { header: { cwd: '/attached-cwd' } } : undefined, }, }) - const result = await invoke(route, { sessionId: 's-attached', cwd: '/tmp/summary-cwd' }) + const result = await invoke(route, 'session.cwd', { sessionId: 's-attached', cwd: '/tmp/summary-cwd' }) expect(result.ok).toBe(true) expect(result.value?.cwd).toBe('/attached-cwd') }) it('rejects a non-absolute client cwd', async () => { const route = mount() - const result = await invoke(route, { sessionId: 's-detached', cwd: 'relative/path' }) + const result = await invoke(route, 'session.cwd', { sessionId: 's-detached', cwd: 'relative/path' }) expect(result.ok).toBe(false) expect(result.error?.message).toMatch(/invalid working directory/) }) + + it('pty.close releases a terminal key (and rejects a missing tab)', async () => { + const route = mount() + const result = await invoke(route, 'pty.close', { sessionId: 's-pty', tab: 't1' }) + expect(result.ok).toBe(true) + const missing = await invoke(route, 'pty.close', { sessionId: 's-pty' }) + expect(missing.ok).toBe(false) + }) }) diff --git a/tests/unit.spec.ts b/tests/unit.spec.ts index 44c1ebf..fe3ebbe 100644 --- a/tests/unit.spec.ts +++ b/tests/unit.spec.ts @@ -3,7 +3,8 @@ import { compareEntries, parentOf, rootLabel, requireAbsolute } from '../src/fs- import { parseLogLines, parsePorcelainZ } from '../src/git.ts' import { activateTab, closeTab, makeDefaultState, moveTab, moveTabToEdge, openTab, - resizeSplit, sanitizeState, splitPane, toggleExpanded, type SidebarState, type SplitNode, + resizeSplit, sanitizeState, splitPane, tabOpenIn, toggleExpanded, + type SidebarState, type SplitNode, } from '../src/client/state.ts' import { extOf, languageKeyForExt } from '../src/client/lang.ts' import { producedForClosing, resolveSidebarPath, selectProducedFiles } from '../src/client/produced-files.ts' @@ -175,6 +176,27 @@ describe('sidebar state', () => { const after = activateTab(s, leaf.id, tabId) expect((after.splits as { active: string | null }).active).toBe(tabId) }) + + it('tabOpenIn: a tab is open until it is truly closed, wherever it lives', () => { + let s = state() + const leaf = s.splits as { id: string; tabs: { id: string }[] } + const explorerId = leaf.tabs[0]!.id + expect(tabOpenIn(s, explorerId)).toBe(true) + // Moving the tab to another pane keeps it open. + s = splitPane(s, 'row') + const split = s.splits as Extract + const paneA = split.children[0] as { id: string; tabs: { id: string }[] } + const paneB = split.children[1] as { id: string } + s = moveTab(s, paneA.id, explorerId, paneB.id) + expect(tabOpenIn(s, explorerId)).toBe(true) + // Closing it removes it from the whole tree. + const target = s.splits as { id: string; tabs: { id: string }[] } + s = closeTab(s, target.id, explorerId) + expect(tabOpenIn(s, explorerId)).toBe(false) + // A terminal tab added later is open too. + s = openTab(s, { id: 'terminal:9', type: 'terminal', title: 'Terminal 9' }) + expect(tabOpenIn(s, 'terminal:9')).toBe(true) + }) }) describe('pty helpers', () => {