From a58e0bbd5ad27c8d2436d4990b11903010c16acb Mon Sep 17 00:00:00 2001 From: levivilet <72156503+levivilet@users.noreply.github.com> Date: Sun, 4 Oct 2026 18:33:49 +0000 Subject: [PATCH 1/3] fix: clean up owned container terminal sessions --- .../src/parts/CommandMap/CommandMap.ts | 2 + .../ContainerTerminal/ContainerTerminal.ts | 28 ++++- .../ContainerTerminalSession.ts | 101 ++++++++++++++++++ .../test/ContainerTerminal.test.ts | 38 ++++--- .../test/ContainerTerminalSession.test.ts | 40 +++++++ packages/e2e/helpers/testContainerTerminal.ts | 17 +++ .../e2e/src/devcontainer.terminal-disposal.ts | 13 +++ .../DevContainerCommands.ts | 4 + .../RegisterCommands/RegisterCommands.ts | 4 + 9 files changed, 232 insertions(+), 15 deletions(-) create mode 100644 packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts create mode 100644 packages/devcontainer-worker/test/ContainerTerminalSession.test.ts create mode 100644 packages/e2e/src/devcontainer.terminal-disposal.ts diff --git a/packages/devcontainer-worker/src/parts/CommandMap/CommandMap.ts b/packages/devcontainer-worker/src/parts/CommandMap/CommandMap.ts index 5d260ff..0fc6c08 100644 --- a/packages/devcontainer-worker/src/parts/CommandMap/CommandMap.ts +++ b/packages/devcontainer-worker/src/parts/CommandMap/CommandMap.ts @@ -1,6 +1,7 @@ import { setDockerPath } from '@lvce-editor/devcontainer-node/devcontainer-cli' import * as ContainerFileSystem from '../ContainerFileSystem/ContainerFileSystem.ts' import * as ContainerTerminal from '../ContainerTerminal/ContainerTerminal.ts' +import * as ContainerTerminalSession from '../ContainerTerminalSession/ContainerTerminalSession.ts' import * as DevContainer from '../DevContainer/DevContainer.ts' import * as DevContainerCommandType from '../DevContainerCommandType/DevContainerCommandType.ts' import * as HandleElectronMessagePort from '../HandleElectronMessagePort/HandleElectronMessagePort.ts' @@ -8,6 +9,7 @@ import * as HandleNodeMessagePort from '../HandleNodeMessagePort/HandleNodeMessa import * as HandleWebSocket from '../HandleWebSocket/HandleWebSocket.ts' export const commandMap = { + 'DevContainer.disposeTerminal': ContainerTerminalSession.dispose, 'DevContainer.getTerminalSpawnOptions': ContainerTerminal.getSpawnOptions, [DevContainerCommandType.DevContainerDetect]: DevContainer.detect, [DevContainerCommandType.DevContainerExec]: DevContainer.exec, diff --git a/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts b/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts index a776a91..fa44621 100644 --- a/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts +++ b/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts @@ -1,9 +1,21 @@ import { getDevcontainerCliPath } from '@lvce-editor/devcontainer-node/devcontainer-cli' import { posix } from 'node:path' +import * as ContainerTerminalSession from '../ContainerTerminalSession/ContainerTerminalSession.ts' import * as ContainerUri from '../ContainerUri/ContainerUri.ts' import * as DevContainerState from '../DevContainerState/DevContainerState.ts' -export const getSpawnOptions = async (workspaceUri: string, cwd = '') => { +export const getSpawnOptions = async ( + workspaceUri: string, + cwd = '', + createSession: ( + id: string, + cli: string, + user?: string, + ) => Promise<{ + directory: string + token: string + }> = ContainerTerminalSession.create, +) => { const workspace = ContainerUri.parse(workspaceUri) const location = ContainerUri.parse(cwd || workspaceUri) if (location.id !== workspace.id) { @@ -19,6 +31,11 @@ export const getSpawnOptions = async (workspaceUri: string, cwd = '') => { ) { throw new Error('Devcontainer is not running') } + const session = await createSession( + state.containerId, + state.containerCli || 'docker', + state.remoteUser, + ) // The host PTY runs the CLI, which applies remoteUser, remoteEnv and the // user's environment probe. The shell and its working directory are remote. return { @@ -31,14 +48,21 @@ export const getSpawnOptions = async (workspaceUri: string, cwd = '') => { state.containerCli || 'docker', '--container-id', state.containerId, + 'env', + `LVCE_TERMINAL_SESSION=${session.token}`, 'sh', '-c', - 'cd -- "$1" && exec "${SHELL:-/bin/sh}" -il', + ContainerTerminalSession.shellScript, 'devcontainer-terminal', posix.join(state.remoteWorkspaceFolder, location.path), + session.directory, ], command: process.execPath, cwd: workspaceFolder, + disposeCommand: { + args: [session.token], + command: 'devcontainer.disposeTerminal', + }, env: { ELECTRON_RUN_AS_NODE: '1' }, } } diff --git a/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts b/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts new file mode 100644 index 0000000..82af75b --- /dev/null +++ b/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts @@ -0,0 +1,101 @@ +import { execFile } from 'node:child_process' +import { randomUUID } from 'node:crypto' +import { promisify } from 'node:util' + +interface Session { + containerCli: string + containerId: string + directory: string + remoteUser?: string + token: string +} + +const execFileAsync = promisify(execFile) + +export const run = async (session: Session, script: string) => { + const user = session.remoteUser ? ['--user', session.remoteUser] : [] + await execFileAsync( + session.containerCli, + [ + 'exec', + ...user, + session.containerId, + 'sh', + '-c', + script, + 'lvce-terminal-session', + session.directory, + session.token, + ], + { timeout: 10_000 }, + ) +} + +export const createSessionManager = (execute = run) => { + const sessions = new Map() + const closing = new Map>() + const create = async ( + containerId: string, + containerCli: string, + remoteUser?: string, + ) => { + const token = randomUUID() + const session: Session = { + containerCli, + containerId, + directory: `/tmp/lvce-terminal-${token}`, + remoteUser, + token, + } + await execute(session, 'umask 077; mkdir -- "$1"') + sessions.set(token, session) + return session + } + + const finishDispose = async ( + session: Session, + token: string, + ): Promise => { + try { + await execute(session, cleanupScript) + } finally { + closing.delete(token) + } + } + const dispose = (token: string): Promise => { + const pending = closing.get(token) + if (pending) return pending + const session = sessions.get(token) + if (!session) return Promise.resolve() + sessions.delete(token) + const task = finishDispose(session, token) + closing.set(token, task) + return task + } + return { create, dispose } +} + +// Rename closes the startup gate before inspecting the atomically published +// owner. A late CLI launch cannot publish its PID into the removed directory. +// The nonce in the process environment also prevents killing a reused PID. +export const cleanupScript = [ + 'set -e', + 'mv -- "$1" "$1.closed" 2>/dev/null || exit 0', + 'pid=', + 'if [ -f "$1.closed/pid" ]; then read -r pid < "$1.closed/pid"; fi', + 'case "$pid" in ""|*[!0-9]*) pid=0;; esac', + 'if [ "$pid" -gt 1 ] && [ -r "/proc/$pid/environ" ] && tr "\\000" "\\n" < "/proc/$pid/environ" | grep -Fxq "LVCE_TERMINAL_SESSION=$2"; then kill -HUP -"$pid" 2>/dev/null || true; fi', + 'rm -r -- "$1.closed"', +].join('; ') + +const manager = createSessionManager() +export const { create } = manager +export const { dispose } = manager + +export const shellScript = [ + 'set -e', + 'cd -- "$1"', + 'printf "%s\\n" "$$" > "$2/pid.tmp"', + 'mv -- "$2/pid.tmp" "$2/pid"', + 'exec "${SHELL:-/bin/sh}" -il', +].join('; ') diff --git a/packages/devcontainer-worker/test/ContainerTerminal.test.ts b/packages/devcontainer-worker/test/ContainerTerminal.test.ts index 6ce0dae..792c4a6 100644 --- a/packages/devcontainer-worker/test/ContainerTerminal.test.ts +++ b/packages/devcontainer-worker/test/ContainerTerminal.test.ts @@ -1,7 +1,15 @@ -import { afterEach, expect, test } from '@jest/globals' +import { afterEach, expect, jest, test } from '@jest/globals' import * as ContainerTerminal from '../src/parts/ContainerTerminal/ContainerTerminal.ts' +import * as ContainerTerminalSession from '../src/parts/ContainerTerminalSession/ContainerTerminalSession.ts' import * as DevContainerState from '../src/parts/DevContainerState/DevContainerState.ts' +const createSession = jest.fn(async () => ({ + directory: '/tmp/lvce-terminal-test-token', + token: 'test-token', +})) +const getOptions = (uri: string, cwd = '') => + ContainerTerminal.getSpawnOptions(uri, cwd, createSession) + const workspaceFolder = '/host/project with spaces' const workspaceUri = 'devcontainers:///abc123' const connect = () => @@ -17,10 +25,14 @@ afterEach(() => DevContainerState.reset()) test('launches a PTY through devcontainer exec with the configured environment and container id', async () => { connect() - const options = await ContainerTerminal.getSpawnOptions(workspaceUri) + const options = await getOptions(workspaceUri) expect(options.command).toBe(process.execPath) expect(options.env).toEqual({ ELECTRON_RUN_AS_NODE: '1' }) expect(options.cwd).toBe(workspaceFolder) + expect(options.disposeCommand).toEqual({ + args: ['test-token'], + command: 'devcontainer.disposeTerminal', + }) expect(options.args).toEqual([ expect.stringContaining('devcontainer.js'), 'exec', @@ -30,38 +42,38 @@ test('launches a PTY through devcontainer exec with the configured environment a '/custom/podman', '--container-id', 'abc123', + 'env', + 'LVCE_TERMINAL_SESSION=test-token', 'sh', '-c', - 'cd -- "$1" && exec "${SHELL:-/bin/sh}" -il', + ContainerTerminalSession.shellScript, 'devcontainer-terminal', '/workspaces/project', + '/tmp/lvce-terminal-test-token', ]) }) test('opens an Explorer directory and passes special characters as a positional argument', async () => { connect() - const options = await ContainerTerminal.getSpawnOptions( + const options = await getOptions( workspaceUri, `${workspaceUri}/src/a%20b%3B%24x`, ) - expect(options.args.at(-1)).toBe('/workspaces/project/src/a b;$x') + expect(options.args.at(-2)).toBe('/workspaces/project/src/a b;$x') }) test('refuses a terminal in another container', async () => { connect() await expect( - ContainerTerminal.getSpawnOptions( - workspaceUri, - 'devcontainers:///def456/src', - ), + getOptions(workspaceUri, 'devcontainers:///def456/src'), ).rejects.toThrow('different workspace') }) test('refuses host directories in a container workspace', async () => { connect() - await expect( - ContainerTerminal.getSpawnOptions(workspaceUri, 'file:///host'), - ).rejects.toThrow('Invalid devcontainer URI') + await expect(getOptions(workspaceUri, 'file:///host')).rejects.toThrow( + 'Invalid devcontainer URI', + ) }) test('does not launch a host shell for a stopped container', async () => { @@ -70,7 +82,7 @@ test('does not launch a host shell for a stopped container', async () => { ...DevContainerState.get(workspaceFolder)!, status: 'stopped', }) - await expect(ContainerTerminal.getSpawnOptions(workspaceUri)).rejects.toThrow( + await expect(getOptions(workspaceUri)).rejects.toThrow( 'Devcontainer is not running', ) }) diff --git a/packages/devcontainer-worker/test/ContainerTerminalSession.test.ts b/packages/devcontainer-worker/test/ContainerTerminalSession.test.ts new file mode 100644 index 0000000..d0a16a1 --- /dev/null +++ b/packages/devcontainer-worker/test/ContainerTerminalSession.test.ts @@ -0,0 +1,40 @@ +import { expect, jest, test } from '@jest/globals' +import * as Session from '../src/parts/ContainerTerminalSession/ContainerTerminalSession.ts' + +test('disposal retains its owned container and user and is idempotent', async () => { + const execute = jest.fn(async (_session: unknown, _script: string) => {}) + const manager = Session.createSessionManager(execute) + const first = await manager.create( + 'first-container', + '/custom/podman', + 'node', + ) + const other = await manager.create('other-container', 'docker') + execute.mockClear() + await manager.dispose(first.token) + expect(execute).toHaveBeenCalledWith(first, Session.cleanupScript) + expect(first).toMatchObject({ + containerCli: '/custom/podman', + containerId: 'first-container', + remoteUser: 'node', + }) + await manager.dispose(first.token) + expect(execute).toHaveBeenCalledTimes(1) + await manager.dispose(other.token) + expect(execute).toHaveBeenCalledTimes(2) +}) + +test('concurrent disposal waits for the same cleanup operation', async () => { + const execute = jest.fn(async (_session: unknown, _script: string) => {}) + const manager = Session.createSessionManager(execute) + const session = await manager.create('container', 'docker') + execute.mockClear() + const gate = Promise.withResolvers() + execute.mockImplementationOnce(() => gate.promise) + const first = manager.dispose(session.token) + const second = manager.dispose(session.token) + expect(second).toBe(first) + expect(execute).toHaveBeenCalledTimes(1) + gate.resolve() + await first +}) diff --git a/packages/e2e/helpers/testContainerTerminal.ts b/packages/e2e/helpers/testContainerTerminal.ts index eda7995..3dbaf75 100644 --- a/packages/e2e/helpers/testContainerTerminal.ts +++ b/packages/e2e/helpers/testContainerTerminal.ts @@ -13,6 +13,7 @@ export const testContainerTerminal = async ( Workspace, }: Parameters[0], label = 'Dev Containers: Reopen in Container', + verifyDisposal = false, ): Promise => { const localUri = await getWorkspaceUri({ Command }, 'reopen') await Workspace.setPath(localUri) @@ -77,6 +78,22 @@ export const testContainerTerminal = async ( ['/container-workspace/sub folder/marker.txt'], 'subfolder-created', ) + if (verifyDisposal) { + await Command.execute( + 'Terminals.sendText', + 'echo $$ > terminal-shell-pid; printf "dispose-%s\\n" ready\r', + ) + await expect(terminal).toContainText('dispose-ready') + await Command.execute('Terminals.killTerminal') + await Devcontainer.shouldHaveExecOutput( + 'sh', + [ + '-c', + 'pid=$(cat "/container-workspace/sub folder/terminal-shell-pid"); state=$(awk \'{print $3}\' "/proc/$pid/stat" 2>/dev/null || true); if [ -z "$state" ] || [ "$state" = Z ]; then printf disposed; else printf alive; fi', + ], + 'disposed', + ) + } } finally { await Workspace.setPath(localUri) await Devcontainer.remove() diff --git a/packages/e2e/src/devcontainer.terminal-disposal.ts b/packages/e2e/src/devcontainer.terminal-disposal.ts new file mode 100644 index 0000000..b8d41f2 --- /dev/null +++ b/packages/e2e/src/devcontainer.terminal-disposal.ts @@ -0,0 +1,13 @@ +import type { Test } from '@lvce-editor/test-worker' +import { testContainerTerminal } from '../helpers/testContainerTerminal.ts' + +// Requires the renderer's disposeCommand support in the published server. +export const skip = 1 + +export const test: Test = async (context) => { + await testContainerTerminal( + context, + 'Dev Containers: Reopen in Container', + true, + ) +} diff --git a/packages/extension/src/parts/DevContainerCommands/DevContainerCommands.ts b/packages/extension/src/parts/DevContainerCommands/DevContainerCommands.ts index dbe383e..318fc26 100644 --- a/packages/extension/src/parts/DevContainerCommands/DevContainerCommands.ts +++ b/packages/extension/src/parts/DevContainerCommands/DevContainerCommands.ts @@ -152,3 +152,7 @@ export const installDocker = async (): Promise => { export const getTerminalSpawnOptions = (workspaceUri: string, cwd = '') => { return Rpc.invoke('DevContainer.getTerminalSpawnOptions', workspaceUri, cwd) } + +export const disposeTerminal = (token: string) => { + return Rpc.invoke('DevContainer.disposeTerminal', token) +} diff --git a/packages/extension/src/parts/RegisterCommands/RegisterCommands.ts b/packages/extension/src/parts/RegisterCommands/RegisterCommands.ts index c28af7c..ae4c4c7 100644 --- a/packages/extension/src/parts/RegisterCommands/RegisterCommands.ts +++ b/packages/extension/src/parts/RegisterCommands/RegisterCommands.ts @@ -3,6 +3,10 @@ import * as BuildError from '../BuildError/BuildError.ts' import * as DevContainerCommands from '../DevContainerCommands/DevContainerCommands.ts' const commands = [ + { + execute: DevContainerCommands.disposeTerminal, + id: 'devcontainer.disposeTerminal', + }, { execute: DevContainerCommands.getTerminalSpawnOptions, id: 'devcontainer.getTerminalSpawnOptions', From d3c1029b389503abbcee525a8bf049995f450fc1 Mon Sep 17 00:00:00 2001 From: levivilet <72156503+levivilet@users.noreply.github.com> Date: Sun, 4 Oct 2026 18:36:14 +0000 Subject: [PATCH 2/3] fix: keep container command execution private --- .../parts/ContainerTerminalSession/ContainerTerminalSession.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts b/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts index 82af75b..42ca50e 100644 --- a/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts +++ b/packages/devcontainer-worker/src/parts/ContainerTerminalSession/ContainerTerminalSession.ts @@ -12,7 +12,7 @@ interface Session { const execFileAsync = promisify(execFile) -export const run = async (session: Session, script: string) => { +const run = async (session: Session, script: string) => { const user = session.remoteUser ? ['--user', session.remoteUser] : [] await execFileAsync( session.containerCli, From 442ab13ecd38f12025b4975a381a625827ce2f5b Mon Sep 17 00:00:00 2001 From: levivilet <72156503+levivilet@users.noreply.github.com> Date: Sun, 4 Oct 2026 18:40:51 +0000 Subject: [PATCH 3/3] fix: keep session preparation errors recoverable --- .../ContainerTerminal/ContainerTerminal.ts | 27 +++++++++++++++---- .../test/ContainerTerminal.test.ts | 12 +++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts b/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts index fa44621..115cba8 100644 --- a/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts +++ b/packages/devcontainer-worker/src/parts/ContainerTerminal/ContainerTerminal.ts @@ -31,11 +31,28 @@ export const getSpawnOptions = async ( ) { throw new Error('Devcontainer is not running') } - const session = await createSession( - state.containerId, - state.containerCli || 'docker', - state.remoteUser, - ) + let session: { directory: string; token: string } + try { + session = await createSession( + state.containerId, + state.containerCli || 'docker', + state.remoteUser, + ) + } catch (error) { + // Keep preparation failures in the terminal's normal startup-error UI. + // The message is an argument to fixed code, never executable source. + return { + args: [ + '--eval', + 'console.error(process.argv[1]); process.exitCode = 1', + error instanceof Error ? error.message : String(error), + ], + command: process.execPath, + cwd: workspaceFolder, + disposeCommand: undefined, + env: { ELECTRON_RUN_AS_NODE: '1' }, + } + } // The host PTY runs the CLI, which applies remoteUser, remoteEnv and the // user's environment probe. The shell and its working directory are remote. return { diff --git a/packages/devcontainer-worker/test/ContainerTerminal.test.ts b/packages/devcontainer-worker/test/ContainerTerminal.test.ts index 792c4a6..0fda312 100644 --- a/packages/devcontainer-worker/test/ContainerTerminal.test.ts +++ b/packages/devcontainer-worker/test/ContainerTerminal.test.ts @@ -62,6 +62,18 @@ test('opens an Explorer directory and passes special characters as a positional expect(options.args.at(-2)).toBe('/workspaces/project/src/a b;$x') }) +test('preparation failures use the normal terminal error path without evaluating the message', async () => { + connect() + const message = 'Docker is unavailable; $(unexpected) "quoted"' + createSession.mockRejectedValueOnce(new Error(message)) + const options = await getOptions(workspaceUri) + expect(options.command).toBe(process.execPath) + expect(options.env).toEqual({ ELECTRON_RUN_AS_NODE: '1' }) + expect(options.args.at(-1)).toBe(message) + expect(options.args[1]).not.toContain(message) + expect(options.disposeCommand).toBeUndefined() +}) + test('refuses a terminal in another container', async () => { connect() await expect(