From 074971820f7274355f042955424e9b370a01376d Mon Sep 17 00:00:00 2001 From: levivilet <72156503+levivilet@users.noreply.github.com> Date: Wed, 14 Oct 2026 21:17:13 +0000 Subject: [PATCH] fix: bound concurrent devcontainer filesystem processes --- .github/workflows/ci.yml | 3 + .github/workflows/pr.yml | 3 + .../ContainerFileSystem.ts | 41 +++++++++- .../test/ContainerFileSystem.e2e.ts | 80 +++++++++++++++++++ .../test/ContainerFileSystem.test.ts | 72 +++++++++++++++++ 5 files changed, 197 insertions(+), 2 deletions(-) create mode 100644 packages/devcontainer-node/test/ContainerFileSystem.e2e.ts create mode 100644 packages/devcontainer-node/test/ContainerFileSystem.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c54d5b0..0344f27 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,6 +68,9 @@ jobs: timeout-minutes: 30 run: npm run e2e:devcontainer if: matrix.os == 'ubuntu-24.04' + - name: Container filesystem process lifecycle regression + run: node --test packages/devcontainer-node/test/ContainerFileSystem.e2e.ts + if: matrix.os == 'ubuntu-24.04' - name: Devcontainer direct test URLs and reloads timeout-minutes: 15 run: npm run e2e:local-url --prefix packages/e2e diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 66ac921..d391e3d 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -69,6 +69,9 @@ jobs: timeout-minutes: 30 run: npm run e2e:devcontainer if: matrix.os == 'ubuntu-24.04' + - name: Container filesystem process lifecycle regression + run: node --test packages/devcontainer-node/test/ContainerFileSystem.e2e.ts + if: matrix.os == 'ubuntu-24.04' - name: Devcontainer direct test URLs and reloads timeout-minutes: 15 run: npm run e2e:local-url --prefix packages/e2e diff --git a/packages/devcontainer-node/src/parts/ContainerFileSystem/ContainerFileSystem.ts b/packages/devcontainer-node/src/parts/ContainerFileSystem/ContainerFileSystem.ts index 84f2137..98ff9ce 100644 --- a/packages/devcontainer-node/src/parts/ContainerFileSystem/ContainerFileSystem.ts +++ b/packages/devcontainer-node/src/parts/ContainerFileSystem/ContainerFileSystem.ts @@ -27,7 +27,10 @@ done`, writeFile: 'cat > "$1"', } -export const run = async (options: Options): Promise => { +const runOperation = async ( + options: Options, + runProcess: typeof RunProcess.runProcess, +): Promise => { const { containerCli = 'docker', containerId, @@ -42,7 +45,7 @@ export const run = async (options: Options): Promise => { if (!script) { throw new Error(`Unsupported container file operation: ${operation}`) } - const result = await RunProcess.runProcess({ + const result = await runProcess({ args: [ 'exec', '-i', @@ -81,3 +84,37 @@ export const run = async (options: Options): Promise => { } return result.stdout } + +// File searches and language services can request hundreds of files at once. +// Bound Docker exec processes across containers while allowing terminal and +// lifecycle commands to proceed independently. +const maxProcesses = 8 + +export const createContainerFileSystem = ( + runProcess: typeof RunProcess.runProcess, +) => { + let activeProcesses = 0 + const waiting: (() => void)[] = [] + return async (options: Options): Promise => { + if (activeProcesses < maxProcesses) { + activeProcesses++ + } else { + await new Promise((resolve) => { + waiting.push(resolve) + }) + } + try { + return await runOperation(options, runProcess) + } finally { + const next = waiting.shift() + if (next) { + // Transfer the slot directly so a new request cannot overtake the queue. + next() + } else { + activeProcesses-- + } + } + } +} + +export const run = createContainerFileSystem(RunProcess.runProcess) diff --git a/packages/devcontainer-node/test/ContainerFileSystem.e2e.ts b/packages/devcontainer-node/test/ContainerFileSystem.e2e.ts new file mode 100644 index 0000000..ad1a851 --- /dev/null +++ b/packages/devcontainer-node/test/ContainerFileSystem.e2e.ts @@ -0,0 +1,80 @@ +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { test } from 'node:test' +import * as ContainerFileSystem from '../src/parts/ContainerFileSystem/ContainerFileSystem.ts' +import * as RunProcess from '../src/parts/RunProcess/RunProcess.ts' + +const children = (): string[] => { + return readFileSync(`/proc/self/task/${process.pid}/children`, 'utf8') + .trim() + .split(/\s+/) + .filter(Boolean) +} + +void test( + 'concurrent container file operations bound and reap Docker children', + { timeout: 120_000 }, + async () => { + const started = await RunProcess.runProcess({ + args: [ + 'run', + '-d', + '--entrypoint', + 'sleep', + 'mcr.microsoft.com/devcontainers/base:ubuntu24.04', + 'infinity', + ], + command: 'docker', + }) + assert.ok(!('errorMessage' in started), JSON.stringify(started)) + assert.equal(started.exitCode, 0, started.stderr) + const containerId = started.stdout.trim() + let peak = 0 + const sample = () => { + peak = Math.max(peak, children().length) + } + const timer = setInterval(sample, 5) + try { + for (let cycle = 0; cycle < 3; cycle++) { + const results = await Promise.allSettled( + Array.from({ length: 128 }, (_, index) => + ContainerFileSystem.run({ + containerId, + operation: 'readFile', + path: index % 16 === 0 ? '/missing-file' : '/etc/os-release', + remoteWorkspaceFolder: '/', + }), + ), + ) + for (const [index, result] of results.entries()) { + if (index % 16 === 0) { + assert.equal(result.status, 'rejected') + assert.equal(result.reason.code, 'ENOENT') + } else { + assert.equal(result.status, 'fulfilled') + assert.match( + Buffer.from(String(result.value), 'base64').toString(), + /Ubuntu/, + ) + } + } + assert.deepEqual( + children(), + [], + 'completed operations must leave no Docker processes or zombies', + ) + } + assert.ok(peak > 0, 'must observe the real Docker processes') + assert.ok( + peak <= 8, + `expected at most eight Docker children, observed ${peak}`, + ) + } finally { + clearInterval(timer) + await RunProcess.runProcess({ + args: ['rm', '-f', containerId], + command: 'docker', + }) + } + }, +) diff --git a/packages/devcontainer-node/test/ContainerFileSystem.test.ts b/packages/devcontainer-node/test/ContainerFileSystem.test.ts new file mode 100644 index 0000000..03ef62f --- /dev/null +++ b/packages/devcontainer-node/test/ContainerFileSystem.test.ts @@ -0,0 +1,72 @@ +import { expect, jest, test } from '@jest/globals' +import type { + RunProcessOptions, + RunProcessResult, +} from '../src/parts/RunProcess/RunProcess.ts' +import { createContainerFileSystem } from '../src/parts/ContainerFileSystem/ContainerFileSystem.ts' + +const options = { + containerId: 'container', + operation: 'readFile', + path: '/workspace/file', + remoteWorkspaceFolder: '/workspace', +} +const success = { exitCode: 0, stderr: '', stdout: 'content' } + +test('bounds simultaneous container file processes and drains queued reads', async () => { + const runProcess = + jest.fn<(options: RunProcessOptions) => Promise>() + const run = createContainerFileSystem(runProcess) + const started: string[] = [] + const pending: ((result: RunProcessResult) => void)[] = [] + let active = 0 + let peak = 0 + runProcess.mockImplementation(async ({ args }) => { + started.push(args.at(-1)!) + active++ + peak = Math.max(peak, active) + const result = await new Promise((resolve) => { + pending.push(resolve) + }) + active-- + return result + }) + const paths = Array.from({ length: 32 }, (_, i) => `/workspace/file${i}`) + const reads = paths.map((path) => run({ ...options, path })) + try { + // Each completion admits the next waiting file operation. + for (let i = 0; i < paths.length; i++) { + await new Promise((resolve) => setImmediate(resolve)) + pending.shift()!(success) + } + expect(await Promise.all(reads)).toEqual(paths.map(() => 'content')) + expect(started).toEqual(paths) + expect(peak).toBeLessThanOrEqual(8) + } finally { + runProcess.mockReset() + } +}) + +test('failed operations release their capacity for subsequent reads', async () => { + const runProcess = + jest.fn<(options: RunProcessOptions) => Promise>() + const run = createContainerFileSystem(runProcess) + runProcess.mockImplementation(async () => ({ + exitCode: 44, + stderr: '', + stdout: '', + })) + const results = await Promise.allSettled( + Array.from({ length: 24 }, () => run(options)), + ) + expect(results).toHaveLength(24) + for (const result of results) { + expect(result).toMatchObject({ + reason: { code: 'ENOENT' }, + status: 'rejected', + }) + } + runProcess.mockResolvedValue(success) + expect(await run(options)).toBe('content') + runProcess.mockReset() +})