Skip to content
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
/**
* @vitest-environment jsdom
*/

/**
* Unit tests for usePtyProcess
*
* Covers the guard that prevents PTY creation when a terminal has exited
* naturally (status === 'exited'), which was causing an infinite
* mount → create PTY → unmount → mount loop via the TerminalGrid
* pendingCleanup grace-period mechanism.
*/
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { renderHook, act } from '@testing-library/react';
import { useRef } from 'react';
import { usePtyProcess } from '../usePtyProcess';

const mockCreateTerminal = vi.fn();
const mockDestroyTerminal = vi.fn();
const mockRestoreTerminalSession = vi.fn();

vi.stubGlobal('window', {
electronAPI: {
createTerminal: mockCreateTerminal,
destroyTerminal: mockDestroyTerminal,
restoreTerminalSession: mockRestoreTerminalSession,
},
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

/** Mutable terminal state controlled per test */
let mockTerminalStatus: string = 'idle';
let mockIsRestored: boolean = false;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

const mockSetTerminalStatus = vi.fn();
const mockGetTerminal = vi.fn();

vi.mock('../../../stores/terminal-store', () => ({
useTerminalStore: Object.assign(vi.fn(), {
getState: () => ({
terminals: [
{
id: 'term-1',
status: mockTerminalStatus,
isRestored: mockIsRestored,
isCLIMode: false,
cwd: '/test',
},
],
setTerminalStatus: mockSetTerminalStatus,
getTerminal: mockGetTerminal,
}),
}),
}));
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const DEFAULT_OPTIONS = {
terminalId: 'term-1',
cwd: '/test',
projectPath: '/test',
cols: 80,
rows: 24,
skipCreation: false,
} as const;

describe('usePtyProcess — exited-terminal guard (#fix/terminal-exit-pty-recreation-loop)', () => {
beforeEach(() => {
vi.clearAllMocks();
mockTerminalStatus = 'idle';
mockIsRestored = false;
mockCreateTerminal.mockResolvedValue({ success: true });
mockDestroyTerminal.mockResolvedValue({ success: true });
});

it('does not call createTerminal when terminal status is exited (natural exit)', async () => {
mockTerminalStatus = 'exited';

await act(async () => {
renderHook(() => usePtyProcess(DEFAULT_OPTIONS));
});

expect(mockCreateTerminal).not.toHaveBeenCalled();
});

it('does not call createTerminal on repeated mounts when status remains exited', async () => {
mockTerminalStatus = 'exited';

for (let i = 0; i < 3; i++) {
await act(async () => {
const { unmount } = renderHook(() => usePtyProcess(DEFAULT_OPTIONS));
unmount();
});
}

expect(mockCreateTerminal).not.toHaveBeenCalled();
});
Comment thread
luiggibcn marked this conversation as resolved.

it('allows PTY creation when status is exited but isRecreatingRef is true (worktree switch)', async () => {
mockTerminalStatus = 'exited';

await act(async () => {
renderHook(() => {
const isRecreatingRef = useRef(true);
return usePtyProcess({ ...DEFAULT_OPTIONS, isRecreatingRef });
});
});

expect(mockCreateTerminal).toHaveBeenCalledTimes(1);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it('calls createTerminal when terminal status is idle', async () => {
mockTerminalStatus = 'idle';

await act(async () => {
renderHook(() => usePtyProcess(DEFAULT_OPTIONS));
});

expect(mockCreateTerminal).toHaveBeenCalledTimes(1);
expect(mockCreateTerminal).toHaveBeenCalledWith(
expect.objectContaining({ id: 'term-1', cwd: '/test', cols: 80, rows: 24 }),
);
});

it('calls createTerminal when terminal status is running (e.g. reconnect)', async () => {
mockTerminalStatus = 'running';

await act(async () => {
renderHook(() => usePtyProcess(DEFAULT_OPTIONS));
});

expect(mockCreateTerminal).toHaveBeenCalledTimes(1);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

it('does not call createTerminal when skipCreation is true', async () => {
mockTerminalStatus = 'idle';

await act(async () => {
renderHook(() => usePtyProcess({ ...DEFAULT_OPTIONS, skipCreation: true }));
});

expect(mockCreateTerminal).not.toHaveBeenCalled();
});
});
24 changes: 24 additions & 0 deletions apps/desktop/src/renderer/components/terminal/usePtyProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@ interface UsePtyProcessOptions {
onError?: (error: string) => void;
}

/**
* Manages the PTY process lifecycle for a terminal instance.
*
* Creates and tracks the underlying PTY process, handling:
* - Initial creation once xterm has measured valid dimensions (`skipCreation`)
* - Session restoration for terminals persisted across hot-reloads
* - Deliberate recreation after worktree switches (`isRecreatingRef`)
* - Retry logic when dimensions are not yet ready during recreation
*
* @returns `prepareForRecreate` / `resetForRecreate` callbacks used by
* `Terminal.tsx` to coordinate controlled PTY destruction and recreation.
*/
export function usePtyProcess({
terminalId,
cwd,
Expand Down Expand Up @@ -140,6 +152,18 @@ export function usePtyProcess({
const alreadyRunning = terminalState?.status === 'running' || terminalState?.status === 'claude-active';
const isRestored = terminalState?.isRestored;

// Do not create a new PTY for a terminal that has exited naturally.
// When the shell exits (e.g. user types "exit"), the terminal is kept in the DOM
// for a short grace period (pendingCleanup). During that period the component can
// mount/unmount rapidly — without this guard each mount would spin up a new PTY,
// causing an infinite mount → create PTY → unmount → mount loop.
// Deliberate recreation (worktree switch) sets isRecreatingRef before exiting, so
// it is excluded from this guard.
if (terminalState?.status === 'exited' && !isRecreatingRef?.current) {
debugLog(`[usePtyProcess] Skipping PTY creation for terminal: ${terminalId} - terminal has exited naturally (status=exited)`);
return;
}

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.

medium

This guard is a great addition to prevent the infinite loop. However, it could be made more robust by also checking if terminalState exists. If the terminal has been removed from the store, we should skip PTY creation to avoid spawning orphaned processes.

Suggested change
if (terminalState?.status === 'exited' && !isRecreatingRef?.current) {
debugLog(`[usePtyProcess] Skipping PTY creation for terminal: ${terminalId} - terminal has exited naturally (status=exited)`);
return;
}
if (!terminalState || (terminalState.status === 'exited' && !isRecreatingRef?.current)) {
debugLog(`[usePtyProcess] Skipping PTY creation for terminal: ${terminalId} - ${!terminalState ? 'terminal not found' : 'terminal has exited naturally'}`);
return;
}


debugLog(`[usePtyProcess] Starting PTY creation for terminal: ${terminalId}`);
debugLog(`[usePtyProcess] Terminal ${terminalId} state: isRestored=${isRestored}, status=${terminalState?.status}`);
debugLog(`[usePtyProcess] Terminal ${terminalId} dimensions for PTY: cols=${cols}, rows=${rows}`);
Expand Down
Loading