chore(fix): ensure reconnection to terminal after container restart (#17076)

chore(fix): fix terminal freeze due to race conditions

packages/renderer/src/lib/container/ContainerDetailsTerminal.spec.ts

@@ -19,6 +19,7 @@
 import '@testing-library/jest-dom/vitest';
 
 import { render, screen, waitFor } from '@testing-library/svelte';
+import { Terminal } from '@xterm/xterm';
 import { get } from 'svelte/store';
 import { beforeEach, expect, test, vi } from 'vitest';
 
@@ -174,7 +175,247 @@ test('terminal active/ restarts connection after stopping and starting a contain
   await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(3));
 });
 
-test('terminal active/ restarts connection after restarting a container', async () => {
+test('terminal reconnects via scheduleReconnect when immediate reconnect fails during restart', async () => {
+  vi.useFakeTimers();
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  let onEndCallback: () => void = () => {};
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Simulate restart: the exec dies while the store still says RUNNING (due to debounce).
+  // The immediate reconnect in receiveEndCallback fails because the container is mid-restart.
+  shellInContainerMock.mockRejectedValueOnce(new Error('container is restarting'));
+
+  onEndCallback();
+
+  // The rejected reconnect triggers scheduleReconnect
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+
+  // Restore mock for the next call to succeed
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  // Advance time to trigger the scheduled reconnect (2s delay)
+  await vi.advanceTimersByTimeAsync(2000);
+
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(3));
+
+  // Clean up component before restoring real timers to clear internal reconnect timer
+  renderObject.unmount();
+  vi.useRealTimers();
+});
+
+test('scheduleReconnect retries when restartTerminal fails inside the timer callback', async () => {
+  vi.useFakeTimers();
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  let onEndCallback: () => void = () => {};
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Make the next TWO calls fail so scheduleReconnect's own catch path is exercised
+  shellInContainerMock.mockRejectedValueOnce(new Error('first failure'));
+  shellInContainerMock.mockRejectedValueOnce(new Error('second failure'));
+
+  // Shell ends → receiveEndCallback → restartTerminal fails (call #2) → scheduleReconnect
+  onEndCallback();
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+
+  // Advance timer → scheduleReconnect fires → restartTerminal fails again (call #3)
+  // → the catch inside scheduleReconnect re-calls scheduleReconnect (lines 80-81)
+  await vi.advanceTimersByTimeAsync(2000);
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(3));
+
+  // Restore mock so the re-scheduled timer succeeds
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  // Advance again → the re-scheduled timer fires → restartTerminal succeeds (call #4)
+  await vi.advanceTimersByTimeAsync(2000);
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(4));
+
+  renderObject.unmount();
+  vi.useRealTimers();
+});
+
+test('user input is forwarded to the container via shellInContainerSend', async () => {
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  // Spy on Terminal.open to capture the instance via mock.contexts
+  const openSpy = vi.spyOn(Terminal.prototype, 'open');
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      _onEnd: () => void,
+    ) => {
+      return sendCallbackId;
+    },
+  );
+  vi.mocked(window.shellInContainerSend).mockResolvedValue(undefined);
+
+  render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Retrieve the Terminal instance that called open()
+  const terminalInstance = openSpy.mock.contexts[0];
+  expect(terminalInstance).toBeDefined();
+
+  // Fire the onData event through xterm's internal emitter to simulate user typing
+  /* eslint-disable @typescript-eslint/no-explicit-any */
+  // biome-ignore lint/suspicious/noExplicitAny: accessing xterm internals for testing
+  (terminalInstance as any)._core._onData.fire('test input');
+  /* eslint-enable @typescript-eslint/no-explicit-any */
+
+  await waitFor(() => expect(window.shellInContainerSend).toHaveBeenCalledWith(sendCallbackId, 'test input'));
+
+  openSpy.mockRestore();
+});
+
+test('receiveEndCallback schedules reconnect when container is not running', async () => {
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  let onEndCallback: () => void = () => {};
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Container stops — exec ends while state is not RUNNING.
+  // receiveEndCallback's else branch calls scheduleReconnect.
+  container.state = 'EXITED';
+  await renderObject.rerender({ container: container, screenReaderMode: true });
+  onEndCallback();
+
+  // No immediate reconnect since container is not RUNNING
+  expect(shellInContainerMock).toHaveBeenCalledTimes(1);
+
+  // Container comes back — $effect detects EXITED → RUNNING and reconnects
+  container.state = 'RUNNING';
+  await renderObject.rerender({ container: container, screenReaderMode: true });
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+});
+
+test('receiveEndCallback reconnects successfully and resizes terminal', async () => {
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  let onEndCallback: () => void = () => {};
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Shell ends while container is running — receiveEndCallback calls restartTerminal
+  onEndCallback();
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+
+  // Verify shellInContainerResize was called for the reconnect (not just the initial connection)
+  expect(window.shellInContainerResize).toHaveBeenCalledTimes(2);
+});
+
+test('receiveEndCallback reconnect ignores first data chunk to avoid prompt duplication', async () => {
   const container: ContainerInfoUI = {
     id: 'myContainer',
     state: 'RUNNING',
@@ -195,41 +436,235 @@ test('terminal active/ restarts connection after restarting a container', async
     ) => {
       onDataCallback = onData;
       onEndCallback = onEnd;
-      // return a callback id
       return sendCallbackId;
     },
   );
 
-  // render the component with a terminal
   const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
 
-  // wait shellInContainerMock is called
-  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalled());
-
-  // write some data on the terminal
-  onDataCallback(Buffer.from('hello\nworld'));
-
-  // wait 1s
-  await waitFor(() => renderObject.container.querySelector('div[aria-live="assertive"]'));
-
-  // check the content
+  // Write initial data
+  onDataCallback(Buffer.from('prompt$ '));
   await waitFor(() => {
-    // search a div having aria-live="assertive" attribute
-    const terminalLinesLiveRegion = renderObject.container.querySelector('div[aria-live="assertive"]');
-    expect(terminalLinesLiveRegion).toHaveTextContent('hello world');
+    const region = renderObject.container.querySelector('div[aria-live="assertive"]');
+    expect(region).toHaveTextContent('prompt$');
   });
 
-  container.state = 'RESTARTING';
+  // Shell ends while running — receiveEndCallback routes through restartTerminal,
+  // which sets ignoreFirstData = true before reopening the shell
+  onEndCallback();
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
 
+  // The reconnected shell sends the initial prompt chunk — it should be ignored
+  onDataCallback(Buffer.from('prompt$ '));
+
+  // Send real user output after the ignored chunk
+  onDataCallback(Buffer.from('ls\nfile1 file2\nprompt$ '));
+
+  await waitFor(() => {
+    const region = renderObject.container.querySelector('div[aria-live="assertive"]');
+    expect(region).toHaveTextContent('prompt$ ls file1 file2 prompt$');
+  });
+});
+
+test('$effect schedules reconnect when restartTerminal fails', async () => {
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      _onEnd: () => void,
+    ) => {
+      return sendCallbackId;
+    },
+  );
+
+  const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Container goes to EXITED
+  container.state = 'EXITED';
   await renderObject.rerender({ container: container, screenReaderMode: true });
 
+  // Container returns to RUNNING — $effect fires restartTerminal, but it fails
+  shellInContainerMock.mockRejectedValueOnce(new Error('not ready yet'));
   container.state = 'RUNNING';
-
   await renderObject.rerender({ container: container, screenReaderMode: true });
 
+  // $effect's catch calls scheduleReconnect — wait for the failed attempt
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+
+  // Restore mock for the scheduled retry to succeed
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      _onEnd: () => void,
+    ) => {
+      return sendCallbackId;
+    },
+  );
+
+  // scheduleReconnect will fire after 2s — wait for it with real timers
+  await waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(3), { timeout: 5000 });
+});
+
+test('concurrent receiveEndCallback calls do not create duplicate connections', async () => {
+  vi.useFakeTimers();
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  let onEndCallback: () => void = () => {};
+  let resolveShell: ((id: number) => void) | undefined;
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Make the next shellInContainer call hang so we can test the guard
+  shellInContainerMock.mockImplementation(
+    () =>
+      new Promise<number>(resolve => {
+        resolveShell = resolve;
+      }),
+  );
+
+  // First exec dies — receiveEndCallback starts a reconnect (now in flight)
   onEndCallback();
 
+  // Second exec end fires while the first reconnect is still pending —
+  // the reconnecting guard prevents a duplicate, but schedules a safety-net reconnect
+  onEndCallback();
+
+  // Resolve the pending reconnect
+  resolveShell?.(sendCallbackId);
+
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+
+  // Only 2 calls so far: initial + one reconnect. The second onEndCallback was blocked
+  // by the reconnecting guard but scheduled a safety-net timer.
   expect(shellInContainerMock).toHaveBeenCalledTimes(2);
+
+  // Restore mock for the safety-net timer's reconnect
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  // Advance past the safety-net timer (2s)
+  await vi.advanceTimersByTimeAsync(2000);
+
+  // The safety-net reconnect fires — call #3
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(3));
+
+  renderObject.unmount();
+  vi.useRealTimers();
+});
+
+test('receiveEndCallback during reconnect schedules safety-net retry to prevent freeze', async () => {
+  vi.useFakeTimers();
+  const container: ContainerInfoUI = {
+    id: 'myContainer',
+    state: 'RUNNING',
+    engineId: 'podman',
+  } as unknown as ContainerInfoUI;
+
+  let onEndCallback: () => void = () => {};
+  let resolveShell: ((id: number) => void) | undefined;
+
+  const sendCallbackId = 12345;
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  const renderObject = render(ContainerDetailsTerminal, { container, screenReaderMode: true });
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(1));
+
+  // Make reconnect hang so we can simulate the race
+  shellInContainerMock.mockImplementation(
+    () =>
+      new Promise<number>(resolve => {
+        resolveShell = resolve;
+      }),
+  );
+
+  // Exec dies → receiveEndCallback → restartTerminal starts (reconnecting = true)
+  onEndCallback();
+
+  // The new session's onEnd fires while the reconnect is still in flight —
+  // this is the race condition: the session being established is already dead.
+  // With the fix, this schedules a safety-net reconnect instead of being dropped.
+  onEndCallback();
+
+  // The in-progress reconnect completes — but the session it established is "dead"
+  resolveShell?.(sendCallbackId);
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(2));
+
+  // Restore mock so the safety-net reconnect succeeds
+  shellInContainerMock.mockImplementation(
+    async (
+      _engineId: string,
+      _containerId: string,
+      _onData: (data: Buffer) => void,
+      _onError: (error: string) => void,
+      onEnd: () => void,
+    ) => {
+      onEndCallback = onEnd;
+      return sendCallbackId;
+    },
+  );
+
+  // Advance past the 2s safety-net timer
+  await vi.advanceTimersByTimeAsync(2000);
+
+  // The safety-net reconnect creates a fresh session (call #3) — terminal recovers
+  await vi.waitFor(() => expect(shellInContainerMock).toHaveBeenCalledTimes(3));
+
+  renderObject.unmount();
+  vi.useRealTimers();
 });
 
 test('prompt is not duplicated after restoring terminal from containerTerminals store', async () => {

packages/renderer/src/lib/container/ContainerDetailsTerminal.svelte

@@ -5,6 +5,7 @@ import { TerminalSettings } from '@podman-desktop/core-api/terminal';
 import { EmptyScreen } from '@podman-desktop/ui-svelte';
 import { FitAddon } from '@xterm/addon-fit';
 import { SerializeAddon } from '@xterm/addon-serialize';
+import type { IDisposable } from '@xterm/xterm';
 import { Terminal } from '@xterm/xterm';
 import { onDestroy, onMount } from 'svelte';
 import { router } from 'tinro';
@@ -29,18 +30,58 @@ let terminalContent: string = '';
 let serializeAddon: SerializeAddon;
 let lastState = $state('');
 let containerState = $derived(container.state);
+let reconnectTimer: ReturnType<typeof setTimeout> | undefined;
+let onDataDisposable: IDisposable | undefined;
+let reconnecting = false;
+
+function registerInputHandler(callbackId: number): void {
+  onDataDisposable?.dispose();
+  onDataDisposable = shellTerminal?.onData(data => {
+    window.shellInContainerSend(callbackId, data).catch((error: unknown) => console.log(String(error)));
+  });
+}
 
 $effect(() => {
-  if (lastState === 'STARTING' && containerState === 'RUNNING') {
-    restartTerminal().catch((err: unknown) => console.error('Error restarting terminal', err));
+  if (lastState !== '' && lastState !== 'RUNNING' && containerState === 'RUNNING') {
+    restartTerminal().catch((err: unknown) => {
+      console.error('Error restarting terminal', err);
+      scheduleReconnect();
+    });
   }
   lastState = container.state;
 });
 
 async function restartTerminal(): Promise<void> {
-  ignoreFirstData = true;
-  await executeShellIntoContainer();
-  window.dispatchEvent(new Event('resize'));
+  if (reconnecting) return;
+  reconnecting = true;
+  try {
+    clearReconnectTimer();
+    ignoreFirstData = true;
+    await executeShellIntoContainer();
+    window.dispatchEvent(new Event('resize'));
+  } finally {
+    reconnecting = false;
+  }
+}
+
+function clearReconnectTimer(): void {
+  if (reconnectTimer) {
+    clearTimeout(reconnectTimer);
+    reconnectTimer = undefined;
+  }
+}
+
+function scheduleReconnect(): void {
+  if (reconnectTimer) return;
+  reconnectTimer = setTimeout(() => {
+    reconnectTimer = undefined;
+    if (container.state === 'RUNNING') {
+      restartTerminal().catch((err: unknown) => {
+        console.error('Error restarting terminal', err);
+        scheduleReconnect();
+      });
+    }
+  }, 2000);
 }
 
 // update current route scheme
@@ -63,17 +104,20 @@ function createDataCallback(): (data: Buffer) => void {
 }
 
 function receiveEndCallback(): void {
-  // need to reopen a new terminal if container is running
-  if (sendCallbackId && containerState === 'RUNNING') {
-    window
-      .shellInContainer(container.engineId, container.id, createDataCallback(), () => {}, receiveEndCallback)
-      .then(id => {
-        sendCallbackId = id;
-        shellTerminal?.onData(async data => {
-          await window.shellInContainerSend(id, data);
-        });
-      })
-      .catch((err: unknown) => console.error(`Error opening terminal for container ${container.id}`, err));
+  if (!sendCallbackId) return;
+
+  if (reconnecting) {
+    scheduleReconnect();
+    return;
+  }
+
+  if (containerState === 'RUNNING') {
+    restartTerminal().catch((err: unknown) => {
+      console.error(`Error opening terminal for container ${container.id}`, err);
+      scheduleReconnect();
+    });
+  } else {
+    scheduleReconnect();
   }
 }
 
@@ -91,12 +135,7 @@ async function executeShellIntoContainer(): Promise<void> {
     receiveEndCallback,
   );
   await window.shellInContainerResize(callbackId, shellTerminal.cols, shellTerminal.rows);
-  // pass data from xterm to container
-  shellTerminal?.onData(data => {
-    window.shellInContainerSend(callbackId, data).catch((error: unknown) => console.log(String(error)));
-  });
-
-  // store it
+  registerInputHandler(callbackId);
   sendCallbackId = callbackId;
 }
 
@@ -164,8 +203,9 @@ onMount(async () => {
 });
 
 onDestroy(() => {
+  clearReconnectTimer();
+  onDataDisposable?.dispose();
   terminalContent = serializeAddon.serialize();
-  // register terminal for reusing it
   registerTerminal({
     engineId: container.engineId,
     containerId: container.id,