terminal: keep the pty alive while its tab is open, whatever the view does
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.
This commit is contained in:
+13
-2
@@ -140,7 +140,18 @@ export function Sidebar(props: { ctx: Context }) {
|
|||||||
const fullscreen = state !== undefined && window.innerWidth - state.width < 8
|
const fullscreen = state !== undefined && window.innerWidth - state.width < 8
|
||||||
|
|
||||||
const actions: WorkbenchActions = useMemo(() => ({
|
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) => {
|
activateTab: (paneId, tabId) => {
|
||||||
sidebarStore.reduce(s => ({
|
sidebarStore.reduce(s => ({
|
||||||
...s,
|
...s,
|
||||||
@@ -167,7 +178,7 @@ export function Sidebar(props: { ctx: Context }) {
|
|||||||
resizeSplit: (splitId, index, deltaFrac) => {
|
resizeSplit: (splitId, index, deltaFrac) => {
|
||||||
sidebarStore.reduce(s => ({ ...s, splits: resizeSplit(s.splits, splitId, index, deltaFrac) }))
|
sidebarStore.reduce(s => ({ ...s, splits: resizeSplit(s.splits, splitId, index, deltaFrac) }))
|
||||||
},
|
},
|
||||||
}), [])
|
}), [sessionId, cwd])
|
||||||
|
|
||||||
if (state === undefined || sessionId === undefined) {
|
if (state === undefined || sessionId === undefined) {
|
||||||
return (
|
return (
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ import { FitAddon } from '@xterm/addon-fit'
|
|||||||
import 'xterm/css/xterm.css'
|
import 'xterm/css/xterm.css'
|
||||||
import { t } from './locales.ts'
|
import { t } from './locales.ts'
|
||||||
import type { SessionScope } from './api.ts'
|
import type { SessionScope } from './api.ts'
|
||||||
|
import { sidebarStore } from './state.ts'
|
||||||
import css from './sidebar.module.css'
|
import css from './sidebar.module.css'
|
||||||
|
|
||||||
/** How many consecutive unreasoned failures before showing the error banner. */
|
/** 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)
|
window.clearTimeout(retry)
|
||||||
observer.disconnect()
|
observer.disconnect()
|
||||||
inputSub.dispose()
|
inputSub.dispose()
|
||||||
// Tell the host the owning tab is gone so it releases the terminal
|
// The close frame tells the host the owning tab is GONE (immediate
|
||||||
// quota immediately (best effort; a dropped socket gets the grace
|
// quota release). A bare unmount — conversation switch, re-render,
|
||||||
// period on the host side instead).
|
// page unload — leaves the tab open, so the socket drop alone hands
|
||||||
if (socket !== null && socket.readyState === WebSocket.OPEN) {
|
// 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.send(JSON.stringify({ type: 'close' }))
|
||||||
}
|
}
|
||||||
socket?.close()
|
socket?.close()
|
||||||
|
|||||||
@@ -114,6 +114,11 @@ export const api = {
|
|||||||
call<{ ok: true }>('git.checkout', scopePayload(scope, { branch })),
|
call<{ ok: true }>('git.checkout', scopePayload(scope, { branch })),
|
||||||
gitLog: (scope: SessionScope, signal?: AbortSignal) =>
|
gitLog: (scope: SessionScope, signal?: AbortSignal) =>
|
||||||
call<GitLogEntry[]>('git.log', scopePayload(scope, {}), signal),
|
call<GitLogEntry[]>('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). */
|
/** Absolute URL of the media route for one path (images only). */
|
||||||
|
|||||||
@@ -125,6 +125,11 @@ export function allLeaves(node: SplitNode): SidebarLeaf[] {
|
|||||||
return node.children.flatMap(allLeaves)
|
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. */
|
/** Replace a leaf with a split of it plus a fresh empty leaf. */
|
||||||
export function splitLeafAt(node: SplitNode, paneId: string, dir: 'row' | 'col'): SplitNode {
|
export function splitLeafAt(node: SplitNode, paneId: string, dir: 'row' | 'col'): SplitNode {
|
||||||
const fresh: SidebarLeaf = { kind: 'leaf', id: uid('pane'), tabs: [], active: null }
|
const fresh: SidebarLeaf = { kind: 'leaf', id: uid('pane'), tabs: [], active: null }
|
||||||
@@ -506,6 +511,21 @@ export class SidebarStore {
|
|||||||
this.notify()
|
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). */
|
/** Apply a pure reducer (returns the next state). */
|
||||||
reduce(reducer: (state: SidebarState) => SidebarState): void {
|
reduce(reducer: (state: SidebarState) => SidebarState): void {
|
||||||
const sessionId = this.snapshot.sessionId
|
const sessionId = this.snapshot.sessionId
|
||||||
|
|||||||
@@ -204,6 +204,16 @@ function buildApi(ctx: Context, ptyManager: PtyManager): Record<string, ApiMetho
|
|||||||
const rev = requireString(payload, 'rev')
|
const rev = requireString(payload, 'rev')
|
||||||
return { content: await git.show(cwd, rev, path) }
|
return { content: await git.show(cwd, rev, path) }
|
||||||
},
|
},
|
||||||
|
// Release a terminal immediately. The WebSocket close frame already does
|
||||||
|
// this while the socket is open; this route covers the tab-close that
|
||||||
|
// happens while the socket is down (reconnect loop), so a closed tab can
|
||||||
|
// never hold the per-session quota until the reconnect grace expires.
|
||||||
|
'pty.close': (payload) => {
|
||||||
|
const sessionId = requireString(payload, 'sessionId')
|
||||||
|
const tab = requireString(payload, 'tab')
|
||||||
|
ptyManager.close(`${sessionId}:${tab}`)
|
||||||
|
return { ok: true }
|
||||||
|
},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+14
-4
@@ -46,6 +46,11 @@ export interface SidebarPty {
|
|||||||
key: string
|
key: string
|
||||||
sessionId: string
|
sessionId: string
|
||||||
tabId: 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
|
pty: nodePty.IPty
|
||||||
/** Output accumulated since spawn (bounded; head dropped when over the limit). */
|
/** Output accumulated since spawn (bounded; head dropped when over the limit). */
|
||||||
transcript: string
|
transcript: string
|
||||||
@@ -79,9 +84,13 @@ export class PtyManager {
|
|||||||
/**
|
/**
|
||||||
* Open (or reuse) the terminal for a session/tab key. A handle whose
|
* Open (or reuse) the terminal for a session/tab key. A handle whose
|
||||||
* process already exited is replaced with a fresh spawn (reconnecting a
|
* process already exited is replaced with a fresh spawn (reconnecting a
|
||||||
* dead terminal must yield a live shell, not an input sink). Reopening
|
* dead terminal must yield a live shell, not an input sink), and so is a
|
||||||
* also cancels any pending scheduled close (a reconnect within the grace
|
* live handle whose spawn cwd differs from the now-authoritative one (the
|
||||||
* window keeps the process alive).
|
* 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 sessionId - conversation id.
|
||||||
* @param tabId - client tab id.
|
* @param tabId - client tab id.
|
||||||
* @param cwd - initial working directory (the session's cwd).
|
* @param cwd - initial working directory (the session's cwd).
|
||||||
@@ -94,7 +103,7 @@ export class PtyManager {
|
|||||||
const key = `${sessionId}:${tabId}`
|
const key = `${sessionId}:${tabId}`
|
||||||
this.cancelClose(key)
|
this.cancelClose(key)
|
||||||
const existing = this.sessions.get(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)
|
if (existing !== undefined) this.close(key)
|
||||||
// Zombie cleanup: a session's exited handles (shell closed, tab dropped
|
// Zombie cleanup: a session's exited handles (shell closed, tab dropped
|
||||||
// on an old host without the close frame) must not eat the quota.
|
// on an old host without the close frame) must not eat the quota.
|
||||||
@@ -108,6 +117,7 @@ export class PtyManager {
|
|||||||
key,
|
key,
|
||||||
sessionId,
|
sessionId,
|
||||||
tabId,
|
tabId,
|
||||||
|
cwd,
|
||||||
pty: nodePty.spawn(this.shell, [], {
|
pty: nodePty.spawn(this.shell, [], {
|
||||||
name: 'xterm-256color',
|
name: 'xterm-256color',
|
||||||
cols: Math.max(2, Math.floor(cols)),
|
cols: Math.max(2, Math.floor(cols)),
|
||||||
|
|||||||
+37
-6
@@ -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 () => {
|
it('lists the repository root level', async () => {
|
||||||
const listing = await listDirectory(process.cwd(), 1000)
|
const listing = await listDirectory(process.cwd(), 1000)
|
||||||
expect(listing.entries.some(entry => entry.name === 'src' && entry.isDir)).toBe(true)
|
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')!
|
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 body = Buffer.from(JSON.stringify(payload))
|
||||||
const req = {
|
const req = {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
url: '/sidebar/api/session.cwd',
|
url: `/sidebar/api/${method}`,
|
||||||
headers: { host: '127.0.0.1:3080' },
|
headers: { host: '127.0.0.1:3080' },
|
||||||
[Symbol.asyncIterator]: async function* () { yield body },
|
[Symbol.asyncIterator]: async function* () { yield body },
|
||||||
} as never
|
} 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 () => {
|
it('uses the client summary cwd while the session is detached', async () => {
|
||||||
const route = mount()
|
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.ok).toBe(true)
|
||||||
expect(result.value?.cwd).toBe('/tmp/summary-cwd')
|
expect(result.value?.cwd).toBe('/tmp/summary-cwd')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('falls back to the process cwd with no summary cwd', async () => {
|
it('falls back to the process cwd with no summary cwd', async () => {
|
||||||
const route = mount()
|
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.ok).toBe(true)
|
||||||
expect(result.value?.cwd).toBe(process.cwd())
|
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,
|
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.ok).toBe(true)
|
||||||
expect(result.value?.cwd).toBe('/attached-cwd')
|
expect(result.value?.cwd).toBe('/attached-cwd')
|
||||||
})
|
})
|
||||||
|
|
||||||
it('rejects a non-absolute client cwd', async () => {
|
it('rejects a non-absolute client cwd', async () => {
|
||||||
const route = mount()
|
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.ok).toBe(false)
|
||||||
expect(result.error?.message).toMatch(/invalid working directory/)
|
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)
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
+23
-1
@@ -3,7 +3,8 @@ import { compareEntries, parentOf, rootLabel, requireAbsolute } from '../src/fs-
|
|||||||
import { parseLogLines, parsePorcelainZ } from '../src/git.ts'
|
import { parseLogLines, parsePorcelainZ } from '../src/git.ts'
|
||||||
import {
|
import {
|
||||||
activateTab, closeTab, makeDefaultState, moveTab, moveTabToEdge, openTab,
|
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'
|
} from '../src/client/state.ts'
|
||||||
import { extOf, languageKeyForExt } from '../src/client/lang.ts'
|
import { extOf, languageKeyForExt } from '../src/client/lang.ts'
|
||||||
import { producedForClosing, resolveSidebarPath, selectProducedFiles } from '../src/client/produced-files.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)
|
const after = activateTab(s, leaf.id, tabId)
|
||||||
expect((after.splits as { active: string | null }).active).toBe(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<SplitNode, { kind: 'split' }>
|
||||||
|
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', () => {
|
describe('pty helpers', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user