From 2cd8982931384553316d71c6d6d2c2967b045e6c Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 26 Sep 2026 18:05:46 -0700 Subject: [PATCH 1/3] docs(plan): a plain terminal view survives a same-id respawn (#1281) Co-Authored-By: Claude Opus 5.5 --- ...-09-26-terminal-attach-survives-respawn.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 docs/plans/2026-09-26-terminal-attach-survives-respawn.md diff --git a/docs/plans/2026-09-26-terminal-attach-survives-respawn.md b/docs/plans/2026-09-26-terminal-attach-survives-respawn.md new file mode 100644 index 000000000..134577173 --- /dev/null +++ b/docs/plans/2026-09-26-terminal-attach-survives-respawn.md @@ -0,0 +1,25 @@ +# A plain terminal view survives a same-id respawn (#1281 residual, #1283 item 3 for terminals) + +## Problem +#1281 is two halves. The agent raw-PTY half shipped in `2ab60d45`, and #1311's review (`6cf6f054`) made each renderer page own its agent-PTY attaches, which covers #1283 item 3 for agents. The plain-terminal half is still open, and its plan (`2026-09-25-pty-attach-survives-wake.md`) recorded it as a residual. +- `SessionManager.terminalAttached` is a Set that `cleanupSessionState` deletes when the shell exits. +- `TerminalLeaf` attaches once per sessionId (effect deps `[sessionId]`), after `ensureSessionLive`. +- A same-id respawn under a still-mounted leaf buffers the new shell's bytes but never forwards them: the xterm stays on the dead shell until a remount. The respawn happens through `recover` with `recoverTmuxName`, from a delivery, a control call, or a wake. +- There is no terminal detach at all, so the flag could not be kept past exit without leaking one id per closed terminal. +- `session:terminal-attach` ignores the sender, so a reloaded or crashed renderer could never release a reference either (#1283 item 3 for terminals). + +## Evidence +- Source: `sessionManager.ts` (`terminalAttached` add in `attachTerminal`; delete in `cleanupSessionState`; data forwarding gated on `has`), `ipc/session.ts` (`session:terminal-attach` handler), `TerminalLeaf.tsx` (attach in the `[sessionId]` effect, no detach in its cleanup). +- The agent-half fix and its tests (`sessionManager.ptyAttachWake.test.ts`, `ipc/session.test.ts` "raw PTY attach ownership (#1311)") are the recorded model this copies. + +## Decisions (defaults) +- **The attach describes the VIEW, not the process.** `terminalAttached` becomes a count, which process cleanup no longer touches. A new `detachTerminal` releases one reference. +- **A missing session still takes a reference.** A leaf can mount while its shell is down, and the respawn then forwards. A kind mismatch (an agent id) takes none, and the IPC layer records only references main took. +- **Renderer ownership:** the IPC ownership table for agent-PTY attaches becomes one generic per-page table, keyed by view type and session. Terminal attach and detach use it with the page document, like agent PTYs. A reload or destroy releases a page's terminal references too; a stale page's late detach is ignored. +- **TerminalLeaf detaches in its effect cleanup** when its attach resolved; a late attach after unmount detaches itself, as `AgentInlineTerminal` does. +- **No terminal resize restore:** plain terminals have none today, so none is added. + +## Tests +- **Manager:** a still-attached terminal view keeps forwarding across exit and a same-id respawn, and its detach ends forwarding. Red on main. +- **IPC:** terminal attaches are released on reload and destroy; a stale page's detach is ignored; a kind-mismatch attach takes no reference. +- **Renderer:** TerminalLeaf detaches on unmount, and a late attach after unmount releases itself. From a3b84f29c5fd8ea56024009c83d94dbcb57c3d02 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 26 Sep 2026 18:09:51 -0700 Subject: [PATCH 2/3] fix(terminal): a mounted terminal keeps its live output across a same-id shell respawn The shell's view reference was a manager flag cleared when the shell exited, so a still-mounted leaf froze on the dead shell after a same-id respawn. It is now a view-scoped count with a detachTerminal release, owned per renderer page like agent PTY attaches (one shared ownership table), and TerminalLeaf detaches on unmount. Co-Authored-By: Claude Opus 5.5 --- .../2026-09-25-pty-attach-survives-wake.md | 2 +- src/main/ipc/session.test.ts | 52 +++++++ src/main/ipc/session.ts | 145 +++++++++++------- ...ssionManager.terminalAttachRespawn.test.ts | 81 ++++++++++ .../sessionManager.terminalReplay.test.ts | 3 +- src/main/sessionManager.ts | 53 +++++-- src/preload/api/session.ts | 9 +- .../TerminalLeaf.retention.renderer.test.tsx | 32 ++++ .../src/workspace/tile-tree/TerminalLeaf.tsx | 15 +- 9 files changed, 323 insertions(+), 69 deletions(-) create mode 100644 src/main/sessionManager.terminalAttachRespawn.test.ts diff --git a/docs/plans/2026-09-25-pty-attach-survives-wake.md b/docs/plans/2026-09-25-pty-attach-survives-wake.md index 453041b7b..ee8ef725c 100644 --- a/docs/plans/2026-09-25-pty-attach-survives-wake.md +++ b/docs/plans/2026-09-25-pty-attach-survives-wake.md @@ -10,7 +10,7 @@ The attach count and restore size describe the renderer's VIEW, not the process. Process cleanup leaves them alone, and the view's own `detachAgentPty` (on unmount, including pane close) remains the only release. ## Residuals (not in this PR) -- Plain terminals: `terminalAttached` has no detach call at all and is only cleared by cleanup. Keeping it would leak an id per closed terminal, so the terminal half needs a renderer detach first. Recorded on #1281. +- Plain terminals: `terminalAttached` has no detach call at all and is only cleared by cleanup. Keeping it would leak an id per closed terminal, so the terminal half needs a renderer detach first. Recorded on #1281. Done in `2026-09-26-terminal-attach-survives-respawn.md`. - A renderer reload or crash never runs the leaf cleanup (#1283 item 3). That predates this change, but it now also survives a process exit. ## Test diff --git a/src/main/ipc/session.test.ts b/src/main/ipc/session.test.ts index 4781e2744..79d8cc4a4 100644 --- a/src/main/ipc/session.test.ts +++ b/src/main/ipc/session.test.ts @@ -332,6 +332,58 @@ describe('raw PTY attach ownership (#1311)', () => { }) }) +// #1281 / #1283 item 3 (terminal half): a plain terminal's view reference now +// outlives the shell too, so it gets the same page ownership as the agent PTY +// above. Before, the manager flag had no release at all, and a reloaded or +// crashed renderer could never give one back. +describe('terminal attach ownership (#1281)', () => { + function register(id: number) { + const detached: string[] = [] + const manager = Object.assign(new EventEmitter(), { + attachTerminal: (sessionId: string) => (sessionId === 'agent-pane' ? null : 'replay'), + detachTerminal: (sessionId: string) => { detached.push(sessionId) }, + detachAgentPty: vi.fn(), + }) + registerSessionIpc(manager as never, {} as never, new SessionFeedTap(manager as never)) + const sender = Object.assign(new EventEmitter(), { id }) + return { + detached, + sender, + attach: (sessionId: string, document: string) => harness.handlers.get('session:terminal-attach')!({ sender }, sessionId, document), + detach: (sessionId: string, document: string) => harness.handlers.get('session:terminal-detach')!({ sender }, sessionId, document), + announce: (document: string) => harness.handlers.get('session:screen-document')!({ sender }, document), + } + } + + it('releases a page\'s terminal views when it reloads or dies, and ignores its late detach', () => { + const page = register(6161) + page.announce('doc-1') + expect(page.attach('shell', 'doc-1')).toBe('replay') + // Not a terminal: main took no reference, so the reload releases none. + expect(page.attach('agent-pane', 'doc-1')).toBe('') + page.announce('doc-2') + expect(page.detached).toEqual(['shell']) + // The dead page's queued detach must not take the new page's reference. + page.attach('shell', 'doc-2') + page.detach('shell', 'doc-1') + expect(page.detached).toEqual(['shell']) + page.sender.emit('destroyed') + expect(page.detached).toEqual(['shell', 'shell']) + }) + + it('passes a leaf\'s detach through once per attach, and a stale page gets the replay without a reference', () => { + const page = register(6262) + page.announce('doc') + page.attach('shell', 'doc') + page.detach('shell', 'doc') + page.detach('shell', 'doc') + expect(page.detached).toEqual(['shell']) + // A page that already reloaded away takes nothing that could leak. + expect(page.attach('shell', 'old-doc')).toBe('') + page.sender.emit('destroyed') + expect(page.detached).toEqual(['shell']) + }) +}) // #1267 (steering q22 at the source): session:spawn relayed the raw provider // exception over IPC, where every renderer surface had to remember not to diff --git a/src/main/ipc/session.ts b/src/main/ipc/session.ts index 767b303f3..94c137474 100644 --- a/src/main/ipc/session.ts +++ b/src/main/ipc/session.ts @@ -221,15 +221,6 @@ export function registerSessionIpc( return manager.getSessionKind(sessionId) }) - // Terminal attach/replay. Called once by TerminalLeaf on mount. - // Returns the full buffered output of the session so far AND flips - // the manager's "attached" flag so subsequent PTY data events - // broadcast live. See SessionManager.terminalBuffers for the race - // being fixed. - ipcMain.handle('session:terminal-attach', (_evt, sessionId: string) => { - return manager.attachTerminal(sessionId) - }) - // Snapshot for a renderer that restored after the last change event. The // monitor emits on change only, so without this a reload would show every // busy shell idle until its foreground moved again. Filtered to the caller's @@ -243,30 +234,57 @@ export function registerSessionIpc( ) }) - // Agent PTY attach/replay. DebugPanel uses this for Claude - // and Codex panes when the user asks to see the raw underlying TUI - // as an xterm terminal. Kept separate from terminal-attach because - // plain terminal panes and agent panes have different primary - // renderers and different live IPC channels. + // Raw views (agent PTY xterms and plain-terminal leaves) are OWNED by the + // calling renderer page (#1311 review for agents, #1283 item 3 for + // terminals), the same lifecycle as the screen leases below. Main's attach + // counts outlive the process (a same-id wake keeps a mounted view live, + // #1281), so a renderer that reloads or crashes without running its leaf + // cleanup would otherwise pin a count forever: bytes forwarded to nobody, + // and for agents a restore size applied to some later process. Each + // renderer's outstanding attaches are released when it is destroyed or + // loads a new document. // - // Attaches are OWNED by the calling renderer page (#1311 review), the same - // lifecycle as the screen leases below. The attach count now outlives the - // provider process (a same-id wake keeps a mounted view live), so a - // renderer that reloads or crashes without running its leaf cleanup would - // otherwise pin a count forever: raw PTY bytes forwarded to nobody, and a - // restore size applied to some later process. Each renderer's outstanding - // attaches are released when it is destroyed or loads a new document. - const agentPtyAttaches = new Map>() - // Each renderer's current page document, so only a real reload releases. - const agentPtyDocuments = new Map() - const releaseAgentPtyAttaches = (owner: number): void => { - const owned = agentPtyAttaches.get(owner) - if (!owned) return - agentPtyAttaches.delete(owner) - for (const [sessionId, count] of owned) { - for (let i = 0; i < count; i += 1) manager.detachAgentPty(sessionId) + // WHY one table per view type from one factory, rather than the agent-only + // table this used to be plus a copy for terminals: the terminal half of + // #1281 needs exactly the same take / give-back-only-what-you-hold / + // release-on-reload rules, and two hand-written copies are how one of them + // drifts. `detach` is the manager call one reference is worth. + const rawViewOwnership = (detach: (sessionId: string) => void) => { + const byOwner = new Map>() + return { + take(owner: number, sessionId: string): void { + const owned = byOwner.get(owner) ?? new Map() + owned.set(sessionId, (owned.get(sessionId) ?? 0) + 1) + byOwner.set(owner, owned) + }, + /** Give back one reference this owner holds; one it does not hold is ignored. */ + give(owner: number, sessionId: string): void { + const owned = byOwner.get(owner) + const count = owned?.get(sessionId) ?? 0 + if (count === 0) return + if (count === 1) owned!.delete(sessionId) + else owned!.set(sessionId, count - 1) + if (owned!.size === 0) byOwner.delete(owner) + detach(sessionId) + }, + releaseOwner(owner: number): void { + const owned = byOwner.get(owner) + if (!owned) return + byOwner.delete(owner) + for (const [sessionId, count] of owned) { + for (let i = 0; i < count; i += 1) detach(sessionId) + } + }, } } + const agentPtyViews = rawViewOwnership(sessionId => manager.detachAgentPty(sessionId)) + const terminalViews = rawViewOwnership(sessionId => manager.detachTerminal(sessionId)) + // Each renderer's current page document, so only a real reload releases. + const rawViewDocuments = new Map() + const releaseRawViews = (owner: number): void => { + agentPtyViews.releaseOwner(owner) + terminalViews.releaseOwner(owner) + } // Is this call from the renderer's CURRENT page? #1311 round 2 (review A): // a reload can keep the webContents id, so the sender alone cannot tell // the dead page's queued detach from the live page's; without the page @@ -275,39 +293,61 @@ export function registerSessionIpc( // once per load, the same token the screen leases carry). A renderer that // has not announced yet adopts the caller's document, which its own // announcement then confirms. - const isCurrentAgentPtyDocument = (owner: number, document: string): boolean => { - const current = agentPtyDocuments.get(owner) + const isCurrentRawViewDocument = (owner: number, document: string): boolean => { + const current = rawViewDocuments.get(owner) if (current === undefined) { - agentPtyDocuments.set(owner, document) + rawViewDocuments.set(owner, document) return true } return current === document } + + // Agent PTY attach/replay. DebugPanel uses this for Claude + // and Codex panes when the user asks to see the raw underlying TUI + // as an xterm terminal. Kept separate from terminal-attach because + // plain terminal panes and agent panes have different primary + // renderers and different live IPC channels. ipcMain.handle('session:agent-pty-attach', (evt, sessionId: string, document: string) => { const owner = watchLeaseOwner(evt.sender) // A dead page's attach must take no reference: nothing would release it, // because that page's release already ran. - if (!isCurrentAgentPtyDocument(owner, document)) return null + if (!isCurrentRawViewDocument(owner, document)) return null const buffer = manager.attachAgentPty(sessionId) // null = no backend, and main took no reference; nothing to own. if (buffer === null) return buffer - const owned = agentPtyAttaches.get(owner) ?? new Map() - owned.set(sessionId, (owned.get(sessionId) ?? 0) + 1) - agentPtyAttaches.set(owner, owned) + agentPtyViews.take(owner, sessionId) return buffer }) ipcMain.handle('session:agent-pty-detach', (evt, sessionId: string, document: string) => { // Only a reference the CURRENT page holds. After a reload released the // old page's references, its late detach must not take the new page's. - if (agentPtyDocuments.get(evt.sender.id) !== document) return - const owned = agentPtyAttaches.get(evt.sender.id) - const count = owned?.get(sessionId) ?? 0 - if (count === 0) return - if (count === 1) owned!.delete(sessionId) - else owned!.set(sessionId, count - 1) - if (owned!.size === 0) agentPtyAttaches.delete(evt.sender.id) - manager.detachAgentPty(sessionId) + if (rawViewDocuments.get(evt.sender.id) !== document) return + agentPtyViews.give(evt.sender.id, sessionId) + }) + + // Terminal attach/replay. Called once by TerminalLeaf per session id. + // Returns the full buffered output of the session so far AND takes a view + // reference so subsequent PTY data events broadcast live. See + // SessionManager.terminalBuffers for the race being fixed. Same page + // ownership as the agent PTY above (#1283 item 3): the reference used to be + // a manager flag no renderer could release, cleared only by the shell's + // exit, which is what froze a mounted leaf across a same-id respawn (#1281). + ipcMain.handle('session:terminal-attach', (evt, sessionId: string, document: string) => { + const owner = watchLeaseOwner(evt.sender) + // A dead page gets the replay for nothing: no reference, since nothing + // would release it. '' keeps the renderer contract a plain string. + if (!isCurrentRawViewDocument(owner, document)) return '' + const buffer = manager.attachTerminal(sessionId) + // null = not a terminal: main took no reference, so there is none to own. + if (buffer === null) return '' + terminalViews.take(owner, sessionId) + return buffer + }) + + ipcMain.handle('session:terminal-detach', (evt, sessionId: string, document: string) => { + if (rawViewDocuments.get(evt.sender.id) !== document) return + terminalViews.give(evt.sender.id, sessionId) }) // #762. Live `session:screen` frames are forwarded only while a renderer @@ -327,8 +367,8 @@ export function registerSessionIpc( leaseOwnersWatched.add(owner) sender.once('destroyed', () => { screenInterest.dropOwner(owner) - releaseAgentPtyAttaches(owner) - agentPtyDocuments.delete(owner) + releaseRawViews(owner) + rawViewDocuments.delete(owner) leaseOwnersWatched.delete(owner) }) } @@ -339,11 +379,12 @@ export function registerSessionIpc( ipcMain.handle('session:screen-document', (evt, document: string): void => { const owner = watchLeaseOwner(evt.sender) screenInterest.enterDocument(owner, document) - // A NEW document is a page load: the previous page's raw PTY attaches - // died with it. A re-announce of the live document changes nothing. - const previous = agentPtyDocuments.get(owner) - agentPtyDocuments.set(owner, document) - if (previous !== undefined && previous !== document) releaseAgentPtyAttaches(owner) + // A NEW document is a page load: the previous page's raw views (agent + // PTY and terminal attaches) died with it. A re-announce of the live + // document changes nothing. + const previous = rawViewDocuments.get(owner) + rawViewDocuments.set(owner, document) + if (previous !== undefined && previous !== document) releaseRawViews(owner) }) ipcMain.handle('session:screen-lease', (evt, sessionId: string, document: string): void => { screenInterest.acquire(watchLeaseOwner(evt.sender), sessionId, document) diff --git a/src/main/sessionManager.terminalAttachRespawn.test.ts b/src/main/sessionManager.terminalAttachRespawn.test.ts new file mode 100644 index 000000000..78d5ebbfb --- /dev/null +++ b/src/main/sessionManager.terminalAttachRespawn.test.ts @@ -0,0 +1,81 @@ +import { EventEmitter } from 'node:events' +import { expect, it, vi } from 'vitest' + +// #1281 (terminal half): a mounted TerminalLeaf attaches once per session id, +// but process cleanup cleared the attach when the shell exited. A same-id +// respawn under the still-mounted leaf (recover from a delivery, a control +// call, a wake) then buffered the new shell's bytes and never forwarded them: +// the xterm stayed on the dead shell until a remount. The agent half of the +// same bug is sessionManager.ptyAttachWake.test.ts. +const terminals = vi.hoisted(() => ({ created: [] as EventEmitter[] })) +vi.mock('@shared/runtime/terminalSession.js', () => ({ + TerminalSession: class FakeTerminalSession extends EventEmitter { + constructor() { super(); terminals.created.push(this) } + async start(): Promise { this.emit('started') } + async stop(): Promise {} + write(): void {} + resize(): void {} + }, +})) +vi.mock('@main/workspaceDirectory.js', () => ({ + MissingWorkspaceDirectoryError: class extends Error {}, + assertWorkspaceDirectoryExists: vi.fn(async () => {}), +})) +vi.mock('@main/setup/toolchain.js', () => ({ getToolPath: () => '/usr/bin/true' })) +vi.mock('@main/performance/PerformanceService.js', () => ({ + performanceService: { mark: vi.fn(), record: vi.fn(), error: vi.fn(), metric: vi.fn(), span: () => ({ end: vi.fn(), fail: vi.fn() }) }, +})) +vi.mock('@main/storage/feedDebugLog.js', () => ({ forgetFeedDebugSession: vi.fn() })) + +async function managerWithShell() { + const { SessionManager } = await import('./sessionManager') + // No tmux: a direct PTY terminal, the case every machine without tmux runs. + const manager = new SessionManager({ isAvailable: () => false, getBinary: () => null } as never) + const forwarded: string[] = [] + manager.on('terminal-data', event => forwarded.push(event.data)) + const { sessionId } = await manager.spawn({ kind: 'terminal', cwd: '/tmp/project' }) + return { manager, forwarded, sessionId } +} + +it('keeps forwarding to a still-attached terminal view after its shell exits and respawns under the same id', async () => { + const { manager, forwarded, sessionId } = await managerWithShell() + manager.attachTerminal(sessionId) + const first = terminals.created.at(-1)! + first.emit('data', 'old shell') + first.emit('exit', { exitCode: 0 }) + + await manager.recover({ sessionId, kind: 'terminal', cwd: '/tmp/project' }) + const second = terminals.created.at(-1)! + expect(second).not.toBe(first) + second.emit('data', 'fresh shell') + expect(forwarded).toEqual(['old shell', 'fresh shell']) + + // The view's own detach still ends forwarding. + manager.detachTerminal(sessionId) + second.emit('data', 'after detach') + expect(forwarded).not.toContain('after detach') + await manager.kill(sessionId) +}) + +it('forgets the view when the pane detaches while its shell is down', async () => { + const { manager, forwarded, sessionId } = await managerWithShell() + manager.attachTerminal(sessionId) + terminals.created.at(-1)!.emit('exit', { exitCode: 0 }) + manager.detachTerminal(sessionId) + await manager.recover({ sessionId, kind: 'terminal', cwd: '/tmp/project' }) + terminals.created.at(-1)!.emit('data', 'nobody is watching') + expect(forwarded).toEqual([]) + await manager.kill(sessionId) +}) + +// Two views of one shell (a lane and a second window's lane): one closing +// must not cut the other off. +it('keeps forwarding while any view of the shell is still attached', async () => { + const { manager, forwarded, sessionId } = await managerWithShell() + manager.attachTerminal(sessionId) + manager.attachTerminal(sessionId) + manager.detachTerminal(sessionId) + terminals.created.at(-1)!.emit('data', 'still watched') + expect(forwarded).toEqual(['still watched']) + await manager.kill(sessionId) +}) diff --git a/src/main/sessionManager.terminalReplay.test.ts b/src/main/sessionManager.terminalReplay.test.ts index b37cd9972..943e8da31 100644 --- a/src/main/sessionManager.terminalReplay.test.ts +++ b/src/main/sessionManager.terminalReplay.test.ts @@ -40,8 +40,9 @@ it('a remounted shell terminal gets the alternate screen and mouse mode its evic for (let written = 0; written < 300 * 1024; written += frame.length) shell.emit('data', frame) const replay = manager.attachTerminal(sessionId) + expect(replay).not.toBeNull() const terminal = new Terminal({ cols: 120, rows: 36, allowProposedApi: true }) - await new Promise(done => terminal.write(replay, done)) + await new Promise(done => terminal.write(replay!, done)) expect(terminal.buffer.active.type).toBe('alternate') expect(terminal.modes.mouseTrackingMode).toBe('vt200') await manager.kill(sessionId) diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 96dec9f75..3bad5c2f1 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -808,7 +808,13 @@ export class SessionManager extends EventEmitter { // cappedTextBuffer.ts — the string version copied the whole cap on every // chunk and retained sliced-string parents (#726). private readonly terminalBuffers = new Map() - private readonly terminalAttached = new Set() + // How many renderer views of each shell are attached. A count, not a flag, + // because two lanes (or two windows) can show one shell and one closing must + // not cut the other off; and it describes the VIEWS, not the process, so + // process cleanup leaves it alone (#1281, see cleanupSessionState). + // detachTerminal is the only release, and the IPC layer releases a + // renderer page's references when it reloads or is destroyed (#1283). + private readonly terminalAttachCounts = new Map() // Shell activity producer (#865). Emits on a channel of its own, NOT // 'process-state': the remote tap and the recorder subscribe to @@ -1093,7 +1099,14 @@ export class SessionManager extends EventEmitter { this.sessionSizes.delete(sessionId) if (kind === 'terminal') { this.terminalBuffers.delete(sessionId) - this.terminalAttached.delete(sessionId) + // NOT terminalAttachCounts (#1281, terminal half): a TerminalLeaf stays + // mounted while its shell exits and is respawned under the SAME id (a + // delivery, a control call or a wake goes through recover without + // remounting it), and it attaches only once per id. Deleting the count + // here stopped forwarding to it: the xterm stayed on the dead shell + // while keystrokes reached the new one unseen. The agent PTY count + // below is kept for the same reason. The leaf's own detachTerminal (on + // unmount, pane close included) is the release. this.terminalForeground.untrack(sessionId) } else { this.agentPtyBuffers.delete(sessionId) @@ -3585,7 +3598,7 @@ export class SessionManager extends EventEmitter { // replayed when the renderer calls attachTerminal. See the // block comment on terminalBuffers for why this is // race-free. - if (this.terminalAttached.has(sessionId)) { + if ((this.terminalAttachCounts.get(sessionId) ?? 0) > 0) { this.emit('terminal-data', { sessionId, data }) } }) @@ -3731,16 +3744,17 @@ export class SessionManager extends EventEmitter { * see the queue logic in TerminalLeaf. * 3. Subsequent live events write directly to xterm. * - * Returns '' if the session doesn't exist or isn't a terminal — - * silently safe so a stale attach call on a dead session doesn't - * error. + * Returns the replay buffer and takes one view reference, or NULL when no + * reference was taken (the id is not a terminal). A session that does not + * exist right now DOES take a reference and returns '': a TerminalLeaf can + * mount while its shell is down, and a later respawn under the same id must + * forward to it (#1281). The IPC layer records only references main took, + * so a renderer's detach can never release one it does not hold. */ - attachTerminal(sessionId: string): string { + attachTerminal(sessionId: string): string | null { const entry = this.sessions.get(sessionId) if (!entry) { - // Caller is asking to attach to a session that's already gone. - // Silent empty-string is fine here; any TerminalLeaf that mounts - // for a dead session will simply see an empty xterm. + this.terminalAttachCounts.set(sessionId, (this.terminalAttachCounts.get(sessionId) ?? 0) + 1) return '' } if (entry.kind !== 'terminal') { @@ -3755,17 +3769,30 @@ export class SessionManager extends EventEmitter { `[SessionManager] attachTerminal called on non-terminal session`, { sessionId, kind: entry.kind }, ) - return '' + return null } // replay(), not read(): the modes the evicted bytes set come first (#843). const buffer = this.terminalBuffers.get(sessionId)?.replay() ?? '' - // Flip the attach flag in the SAME synchronous block as reading + // Take the reference in the SAME synchronous block as reading // the buffer. JavaScript is single-threaded and event emission // can only happen on a later tick, so nothing can sneak in. - this.terminalAttached.add(sessionId) + this.terminalAttachCounts.set(sessionId, (this.terminalAttachCounts.get(sessionId) ?? 0) + 1) return buffer } + /** + * Release one terminal view reference (a TerminalLeaf unmounting, or the + * IPC layer releasing a reloaded or destroyed renderer's references). The + * last one stops live forwarding; the buffer keeps accumulating for the + * next attach's replay. Works whether or not a shell exists right now, + * because the reference outlives the process (#1281). + */ + detachTerminal(sessionId: string): void { + const attachCount = this.terminalAttachCounts.get(sessionId) ?? 0 + if (attachCount > 1) this.terminalAttachCounts.set(sessionId, attachCount - 1) + else this.terminalAttachCounts.delete(sessionId) + } + /** * Agent PTY attach/replay entry point. * diff --git a/src/preload/api/session.ts b/src/preload/api/session.ts index 6477a2786..659e94f5e 100644 --- a/src/preload/api/session.ts +++ b/src/preload/api/session.ts @@ -120,8 +120,15 @@ export const sessionApi = { * and attach response are silently missed. See TerminalLeaf.tsx * for the queue pattern. */ + // Both carry this page's document, like the agent PTY calls below: main + // owns each terminal view reference per page, releases a reloaded page's, + // and ignores a dead page's late detach (#1281 / #1283). attachTerminal: (sessionId: string): Promise => - ipcRenderer.invoke('session:terminal-attach', sessionId), + ipcRenderer.invoke('session:terminal-attach', sessionId, screenLeaseDocument), + + /** Release this leaf's view of a shell (TerminalLeaf unmount). */ + detachTerminal: (sessionId: string): Promise => + ipcRenderer.invoke('session:terminal-detach', sessionId, screenLeaseDocument), /** * Attach this renderer to an agent runtime's raw PTY terminal. diff --git a/src/renderer/src/workspace/tile-tree/TerminalLeaf.retention.renderer.test.tsx b/src/renderer/src/workspace/tile-tree/TerminalLeaf.retention.renderer.test.tsx index ecf879624..eb11badc9 100644 --- a/src/renderer/src/workspace/tile-tree/TerminalLeaf.retention.renderer.test.tsx +++ b/src/renderer/src/workspace/tile-tree/TerminalLeaf.retention.renderer.test.tsx @@ -176,4 +176,36 @@ describe('TerminalLeaf retention', () => { expect(resize).toHaveBeenCalledTimes(2) expect(resize).toHaveBeenLastCalledWith('shell-1', 120, 40) }) + + // #1281 / #1283 (terminal half): main keeps a shell's view reference across + // the shell's exit and a same-id respawn, so the leaf's unmount is what + // releases it. Without the release main forwards the shell's bytes to + // nobody until the page reloads. + it('releases its view of the shell when it unmounts', async () => { + const view = render( + {}} workspace={workspace} showStatusMode={false} />, + ) + await act(async () => { + attach.resolve('') + await attach.promise + }) + expect(window.api.detachTerminal).not.toHaveBeenCalled() + view.unmount() + expect(window.api.detachTerminal).toHaveBeenCalledExactlyOnceWith('shell-1') + }) + + it('releases an attach that lands after the leaf is gone', async () => { + const view = render( + {}} workspace={workspace} showStatusMode={false} />, + ) + // Let the mount's wake settle so the attach is in flight. + await act(async () => { await Promise.resolve() }) + view.unmount() + expect(window.api.detachTerminal).not.toHaveBeenCalled() + await act(async () => { + attach.resolve('late replay') + await attach.promise + }) + expect(window.api.detachTerminal).toHaveBeenCalledExactlyOnceWith('shell-1') + }) }) diff --git a/src/renderer/src/workspace/tile-tree/TerminalLeaf.tsx b/src/renderer/src/workspace/tile-tree/TerminalLeaf.tsx index 59ed8ed13..76a11ab81 100644 --- a/src/renderer/src/workspace/tile-tree/TerminalLeaf.tsx +++ b/src/renderer/src/workspace/tile-tree/TerminalLeaf.tsx @@ -223,6 +223,11 @@ export function TerminalLeaf({ let resizeObserver: ResizeObserver | null = null let resizeFrame: number | null = null let disposed = false + // Whether main holds a view reference for this leaf (#1281 / #1283). + // Released in the cleanup below; an attach that resolves after the leaf + // is gone releases itself instead, or main would forward this shell's + // bytes to nobody until the page reloads. + let attached = false let attachedBackfillDone = false let lastCols = 0 let lastRows = 0 @@ -470,7 +475,11 @@ export function TerminalLeaf({ // The cleanup below may have disposed the term already // (transient remount, rapid tab switch). Check before // touching it. - if (disposed || termRef.current !== term) return + if (disposed || termRef.current !== term) { + void window.api.detachTerminal?.(sessionId).catch(() => undefined) + return + } + attached = true const liveTerm = term if (!liveTerm) return // Replay the buffer, then drain any live events that arrived @@ -560,6 +569,10 @@ export function TerminalLeaf({ return () => { disposed = true + // The view is gone, so main may stop forwarding to it. Main keeps the + // reference across a shell exit and a same-id respawn (#1281), so this + // is the release; a pane close, a remount or a tab switch all land here. + if (attached) void window.api.detachTerminal?.(sessionId).catch(() => undefined) offUserInput?.() if (resizeFrame !== null) cancelAnimationFrame(resizeFrame) resizeObserver?.disconnect() From cb76010c95b969ea5bf6e116bd1cd8e0617acb52 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Sat, 26 Sep 2026 18:28:38 -0700 Subject: [PATCH 3/3] test(terminal): pin the attach-while-shell-down path and import the manager once Round-1 review of #1333: the absent-session reference had no test, and a cold per-test import of the manager hit the default timeout under load. Co-Authored-By: Claude Opus 5.5 --- ...ssionManager.terminalAttachRespawn.test.ts | 27 ++++++++++++++++--- src/main/sessionManager.ts | 3 ++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/main/sessionManager.terminalAttachRespawn.test.ts b/src/main/sessionManager.terminalAttachRespawn.test.ts index 78d5ebbfb..ad560d9c7 100644 --- a/src/main/sessionManager.terminalAttachRespawn.test.ts +++ b/src/main/sessionManager.terminalAttachRespawn.test.ts @@ -27,8 +27,12 @@ vi.mock('@main/performance/PerformanceService.js', () => ({ })) vi.mock('@main/storage/feedDebugLog.js', () => ({ forgetFeedDebugSession: vi.fn() })) +// Imported once at module scope (#1333 review a): a cold import of the +// manager's graph took ~4 s inside the first test and hit the default 5 s +// timeout on a contended machine. Collection time is not test time. +const { SessionManager } = await import('./sessionManager') + async function managerWithShell() { - const { SessionManager } = await import('./sessionManager') // No tmux: a direct PTY terminal, the case every machine without tmux runs. const manager = new SessionManager({ isAvailable: () => false, getBinary: () => null } as never) const forwarded: string[] = [] @@ -68,8 +72,10 @@ it('forgets the view when the pane detaches while its shell is down', async () = await manager.kill(sessionId) }) -// Two views of one shell (a lane and a second window's lane): one closing -// must not cut the other off. +// Two views of one shell in the same renderer (a lane and a retained +// Spotlight copy): one closing must not cut the other off. Two WINDOWS cannot +// both receive a shell's bytes: the window router gives a session one owner +// (#1333 review c), so this is about views within the receiving renderer. it('keeps forwarding while any view of the shell is still attached', async () => { const { manager, forwarded, sessionId } = await managerWithShell() manager.attachTerminal(sessionId) @@ -79,3 +85,18 @@ it('keeps forwarding while any view of the shell is still attached', async () => expect(forwarded).toEqual(['still watched']) await manager.kill(sessionId) }) + +// #1333 review (a, b, c; surviving mutant): a leaf can attach while its shell +// is down (it exited between the leaf's wake and its attach reaching main). +// That attach must still take a reference, or the same-id respawn buffers its +// bytes and the leaf stays frozen: the bug this file is about, one step +// earlier. +it('forwards a respawned shell to a view that attached while the shell was down', async () => { + const { manager, forwarded, sessionId } = await managerWithShell() + terminals.created.at(-1)!.emit('exit', { exitCode: 0 }) + expect(manager.attachTerminal(sessionId)).toBe('') + await manager.recover({ sessionId, kind: 'terminal', cwd: '/tmp/project' }) + terminals.created.at(-1)!.emit('data', 'after absent attach') + expect(forwarded).toEqual(['after absent attach']) + await manager.kill(sessionId) +}) diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 3bad5c2f1..3d463350c 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -809,7 +809,8 @@ export class SessionManager extends EventEmitter { // chunk and retained sliced-string parents (#726). private readonly terminalBuffers = new Map() // How many renderer views of each shell are attached. A count, not a flag, - // because two lanes (or two windows) can show one shell and one closing must + // because two views in the receiving renderer (a lane and a retained + // Spotlight copy) can show one shell and one closing must // not cut the other off; and it describes the VIEWS, not the process, so // process cleanup leaves it alone (#1281, see cleanupSessionState). // detachTerminal is the only release, and the IPC layer releases a