fix(team): ensure MCP config cleanup on spawn failures

- Added logic to remove generated MCP config files when team creation or launch fails due to spawn errors.
- Updated tests to verify that MCP config files are correctly removed in failure scenarios, ensuring no leftover configurations persist.
- Enhanced error handling in the TeamProvisioningService to maintain clean state during provisioning operations.
This commit is contained in:
iliya 2026-03-29 01:53:16 +02:00
parent e1f0d61dc5
commit 93f160d731
3 changed files with 490 additions and 1 deletions

View file

@ -3203,6 +3203,10 @@ export class TeamProvisioningService {
const tasksDir = path.join(getTasksBasePath(), request.teamName);
await fs.promises.rm(teamDir, { recursive: true, force: true }).catch(() => {});
await fs.promises.rm(tasksDir, { recursive: true, force: true }).catch(() => {});
if (run.mcpConfigPath) {
await this.mcpConfigBuilder.removeConfigFile(run.mcpConfigPath).catch(() => {});
run.mcpConfigPath = null;
}
this.runs.delete(runId);
this.provisioningRunByTeam.delete(request.teamName);
throw error;
@ -3639,6 +3643,10 @@ export class TeamProvisioningService {
stdio: ['pipe', 'pipe', 'pipe'],
});
} catch (error) {
if (run.mcpConfigPath) {
await this.mcpConfigBuilder.removeConfigFile(run.mcpConfigPath).catch(() => {});
run.mcpConfigPath = null;
}
this.runs.delete(runId);
this.provisioningRunByTeam.delete(request.teamName);
await this.restorePrelaunchConfig(request.teamName);

View file

@ -1,9 +1,41 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import * as fs from 'fs';
import Module from 'module';
import * as os from 'os';
import * as path from 'path';
const hoisted = vi.hoisted(() => ({
electronState: {
isPackaged: false,
version: '9.9.9-test',
},
execFileMock: vi.fn(
(
_file: string,
_args: readonly string[],
_options:
| { encoding?: string; timeout?: number }
| ((error: Error | null, stdout: string, stderr: string) => void),
callback?: (error: Error | null, stdout: string, stderr: string) => void
) => {
const cb = typeof _options === 'function' ? _options : callback;
cb?.(null, '/mock/node', '');
}
),
}));
let mockHomeDir = '';
type ModuleLoad = (request: string, parent: NodeModule | undefined, isMain: boolean) => unknown;
const moduleInternal = Module as unknown as { _load: ModuleLoad };
const originalModuleLoad = moduleInternal._load;
vi.mock('child_process', async (importOriginal) => {
const actual = await importOriginal<typeof import('child_process')>();
return {
...actual,
execFile: hoisted.execFileMock,
};
});
vi.mock('@main/utils/pathDecoder', async (importOriginal) => {
const actual = await importOriginal<typeof import('@main/utils/pathDecoder')>();
@ -20,15 +52,72 @@ describe('TeamMcpConfigBuilder', () => {
const createdPaths: string[] = [];
const createdDirs: string[] = [];
let tempAppData: string;
let originalResourcesPath: string | undefined;
function setPackagedMode(isPackaged: boolean, version = '9.9.9-test'): void {
hoisted.electronState.isPackaged = isPackaged;
hoisted.electronState.version = version;
}
function setResourcesPath(resourcesPath: string | undefined): void {
Object.defineProperty(process, 'resourcesPath', {
value: resourcesPath,
configurable: true,
writable: true,
});
}
function createPackagedServerBundle(baseDir: string, body = '// packaged server'): string {
const dir = path.join(baseDir, 'mcp-server');
fs.mkdirSync(dir, { recursive: true });
fs.writeFileSync(path.join(dir, 'index.js'), body);
fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: 'agent-teams-mcp' }));
return dir;
}
function readGeneratedServer(configPath: string): { command?: string; args?: string[] } | undefined {
const raw = fs.readFileSync(configPath, 'utf8');
const parsed = JSON.parse(raw) as {
mcpServers?: Record<string, { command?: string; args?: string[] }>;
};
return parsed.mcpServers?.['agent-teams'];
}
function expectNodeEntry(server: { command?: string; args?: string[] } | undefined, entry: string): void {
expect(server?.args).toEqual([entry]);
expect(server?.command).toMatch(/(^node$|[\\/]node(?:\.exe)?$)/);
}
beforeEach(() => {
originalResourcesPath = (process as NodeJS.Process & { resourcesPath?: string }).resourcesPath;
tempAppData = fs.mkdtempSync(path.join(os.tmpdir(), 'team-mcp-appdata-'));
createdDirs.push(tempAppData);
moduleInternal._load = ((request, parent, isMain) => {
if (request === 'electron') {
return {
app: {
get isPackaged() {
return hoisted.electronState.isPackaged;
},
getVersion: () => hoisted.electronState.version,
getPath: () => '/mock/electron-user-data',
},
};
}
return originalModuleLoad(request, parent, isMain);
}) as ModuleLoad;
setAppDataBasePath(tempAppData);
setPackagedMode(false);
setResourcesPath(undefined);
hoisted.execFileMock.mockClear();
});
afterEach(() => {
setAppDataBasePath(null);
setPackagedMode(false);
setResourcesPath(originalResourcesPath);
moduleInternal._load = originalModuleLoad;
vi.restoreAllMocks();
for (const filePath of createdPaths.splice(0)) {
try {
fs.rmSync(filePath, { force: true });
@ -321,4 +410,125 @@ describe('TeamMcpConfigBuilder', () => {
// Should not throw when directory doesn't exist
await builder.gcStaleConfigs();
});
// ── Packaged copy / fallback ──
it('packaged mode reuses an existing valid stable copy', async () => {
setPackagedMode(true, '1.2.3');
setResourcesPath(tempAppData);
const stableDir = path.join(tempAppData, 'mcp-server', '1.2.3');
fs.mkdirSync(stableDir, { recursive: true });
fs.writeFileSync(path.join(stableDir, 'index.js'), '// stable copy');
fs.writeFileSync(path.join(stableDir, 'package.json'), JSON.stringify({ name: 'stable' }));
const builder = new TeamMcpConfigBuilder();
const configPath = await builder.writeConfigFile();
createdPaths.push(configPath);
expectNodeEntry(readGeneratedServer(configPath), path.join(stableDir, 'index.js'));
});
it('packaged mode copies the MCP server from resourcesPath into userData', async () => {
setPackagedMode(true, '2.0.0');
const resourcesDir = fs.mkdtempSync(path.join(os.tmpdir(), 'team-mcp-resources-'));
createdDirs.push(resourcesDir);
createPackagedServerBundle(resourcesDir, '// copied server');
setResourcesPath(resourcesDir);
const builder = new TeamMcpConfigBuilder();
const configPath = await builder.writeConfigFile();
createdPaths.push(configPath);
const stableDir = path.join(tempAppData, 'mcp-server', '2.0.0');
expect(fs.existsSync(path.join(stableDir, 'index.js'))).toBe(true);
expect(fs.existsSync(path.join(stableDir, 'package.json'))).toBe(true);
expectNodeEntry(readGeneratedServer(configPath), path.join(stableDir, 'index.js'));
});
it('packaged mode heals a partial stable copy and rebuilds it from resourcesPath', async () => {
setPackagedMode(true, '3.0.0');
const stableDir = path.join(tempAppData, 'mcp-server', '3.0.0');
fs.mkdirSync(stableDir, { recursive: true });
fs.writeFileSync(path.join(stableDir, 'index.js'), '// partial copy only');
const resourcesDir = fs.mkdtempSync(path.join(os.tmpdir(), 'team-mcp-resources-'));
createdDirs.push(resourcesDir);
createPackagedServerBundle(resourcesDir, '// healed server');
setResourcesPath(resourcesDir);
const builder = new TeamMcpConfigBuilder();
const configPath = await builder.writeConfigFile();
createdPaths.push(configPath);
expect(fs.readFileSync(path.join(stableDir, 'index.js'), 'utf8')).toContain('healed server');
expect(fs.existsSync(path.join(stableDir, 'package.json'))).toBe(true);
expect(readGeneratedServer(configPath)?.args).toEqual([path.join(stableDir, 'index.js')]);
});
it('packaged mode falls back to resourcesPath when stable copy creation fails', async () => {
setPackagedMode(true, '4.0.0');
const resourcesDir = fs.mkdtempSync(path.join(os.tmpdir(), 'team-mcp-resources-'));
createdDirs.push(resourcesDir);
createPackagedServerBundle(resourcesDir, '// fallback server');
setResourcesPath(resourcesDir);
vi.spyOn(fs.promises, 'copyFile').mockRejectedValueOnce(new Error('copy failed'));
const builder = new TeamMcpConfigBuilder();
const configPath = await builder.writeConfigFile();
createdPaths.push(configPath);
expectNodeEntry(readGeneratedServer(configPath), path.join(resourcesDir, 'mcp-server', 'index.js'));
});
it('packaged mode uses the winner stable copy when atomic rename loses the race', async () => {
setPackagedMode(true, '5.0.0');
const resourcesDir = fs.mkdtempSync(path.join(os.tmpdir(), 'team-mcp-resources-'));
createdDirs.push(resourcesDir);
createPackagedServerBundle(resourcesDir, '// race source');
setResourcesPath(resourcesDir);
const stableDir = path.join(tempAppData, 'mcp-server', '5.0.0');
const originalRename = fs.promises.rename.bind(fs.promises);
vi.spyOn(fs.promises, 'rename').mockImplementation(async (from, to) => {
if (to === stableDir) {
fs.mkdirSync(stableDir, { recursive: true });
fs.writeFileSync(path.join(stableDir, 'index.js'), '// winner copy');
fs.writeFileSync(path.join(stableDir, 'package.json'), JSON.stringify({ name: 'winner' }));
const err = new Error('EEXIST') as NodeJS.ErrnoException;
err.code = 'EEXIST';
throw err;
}
return originalRename(from, to);
});
const builder = new TeamMcpConfigBuilder();
const configPath = await builder.writeConfigFile();
createdPaths.push(configPath);
expect(fs.readFileSync(path.join(stableDir, 'index.js'), 'utf8')).toContain('winner copy');
expectNodeEntry(readGeneratedServer(configPath), path.join(stableDir, 'index.js'));
});
it('packaged mode falls back to workspace source when resourcesPath bundle is missing', async () => {
setPackagedMode(true, '6.0.0');
const resourcesDir = fs.mkdtempSync(path.join(os.tmpdir(), 'team-mcp-resources-'));
createdDirs.push(resourcesDir);
setResourcesPath(resourcesDir);
const builder = new TeamMcpConfigBuilder();
const configPath = await builder.writeConfigFile();
createdPaths.push(configPath);
expect(readGeneratedServer(configPath)).toEqual({
command: 'pnpm',
args: [
'--dir',
path.join(process.cwd(), 'mcp-server'),
'exec',
'tsx',
path.join(process.cwd(), 'mcp-server', 'src', 'index.ts'),
],
});
});
});

View file

@ -1,16 +1,55 @@
import type { ChildProcess } from 'child_process';
import { EventEmitter } from 'events';
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
const hoisted = vi.hoisted(() => ({
paths: {
claudeRoot: '',
teamsBase: '',
tasksBase: '',
projectsBase: '',
},
}));
let tempClaudeRoot = '';
let tempTeamsBase = '';
let tempTasksBase = '';
let tempProjectsBase = '';
vi.mock('@main/services/team/ClaudeBinaryResolver', () => ({
ClaudeBinaryResolver: { resolve: vi.fn() },
}));
vi.mock('@main/services/team/TeamTaskReader', () => ({
TeamTaskReader: class {
async getTasks() {
return [];
}
},
}));
vi.mock('@main/utils/childProcess', () => ({
spawnCli: vi.fn(),
killProcessTree: vi.fn(),
}));
vi.mock('@main/utils/pathDecoder', async (importOriginal) => {
const actual = await importOriginal<typeof import('@main/utils/pathDecoder')>();
return {
...actual,
getAutoDetectedClaudeBasePath: () => hoisted.paths.claudeRoot,
getClaudeBasePath: () => hoisted.paths.claudeRoot,
getHomeDir: () => hoisted.paths.claudeRoot,
getProjectsBasePath: () => hoisted.paths.projectsBase,
getTasksBasePath: () => hoisted.paths.tasksBase,
getTeamsBasePath: () => hoisted.paths.teamsBase,
};
});
import { TeamProvisioningService } from '@main/services/team/TeamProvisioningService';
import { ClaudeBinaryResolver } from '@main/services/team/ClaudeBinaryResolver';
import { spawnCli } from '@main/utils/childProcess';
@ -30,9 +69,47 @@ function createFakeChild(exitCode: number): ChildProcess {
return child;
}
function createRunningChild() {
return Object.assign(new EventEmitter(), {
pid: 12345,
stdin: {
writable: true,
write: vi.fn(() => true),
end: vi.fn(),
},
stdout: new EventEmitter(),
stderr: new EventEmitter(),
kill: vi.fn(),
});
}
describe('TeamProvisioningService', () => {
beforeEach(() => {
vi.clearAllMocks();
tempClaudeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'claude-team-provisioning-'));
tempTeamsBase = path.join(tempClaudeRoot, 'teams');
tempTasksBase = path.join(tempClaudeRoot, 'tasks');
tempProjectsBase = path.join(tempClaudeRoot, 'projects');
hoisted.paths.claudeRoot = tempClaudeRoot;
hoisted.paths.teamsBase = tempTeamsBase;
hoisted.paths.tasksBase = tempTasksBase;
hoisted.paths.projectsBase = tempProjectsBase;
fs.mkdirSync(tempTeamsBase, { recursive: true });
fs.mkdirSync(tempTasksBase, { recursive: true });
fs.mkdirSync(tempProjectsBase, { recursive: true });
});
afterEach(() => {
vi.useRealTimers();
try {
fs.rmSync(tempClaudeRoot, { recursive: true, force: true });
} catch {
// ignore temp cleanup failures
}
hoisted.paths.claudeRoot = '';
hoisted.paths.teamsBase = '';
hoisted.paths.tasksBase = '';
hoisted.paths.projectsBase = '';
});
describe('warmup', () => {
@ -53,4 +130,198 @@ describe('TeamProvisioningService', () => {
expect(spawnCli).toHaveBeenCalled();
});
});
it('removes generated MCP config when createTeam spawn fails synchronously', async () => {
allowConsoleLogs();
vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue('/mock/claude');
vi.mocked(spawnCli).mockImplementation(() => {
throw new Error('spawn EINVAL');
});
const mcpConfigBuilder = {
writeConfigFile: vi.fn(async () => '/mock/mcp-config-create.json'),
removeConfigFile: vi.fn(async () => {}),
};
const membersMetaStore = {
writeMembers: vi.fn(async () => {}),
};
const teamMetaStore = {
writeMeta: vi.fn(async () => {}),
deleteMeta: vi.fn(async () => {}),
};
const svc = new TeamProvisioningService(
undefined,
undefined,
membersMetaStore as any,
undefined,
mcpConfigBuilder as any,
teamMetaStore as any
);
(svc as any).buildProvisioningEnv = vi.fn(async () => ({
env: { ANTHROPIC_API_KEY: 'test' },
authSource: 'anthropic_api_key',
}));
(svc as any).pathExists = vi.fn(async () => false);
await expect(
svc.createTeam(
{
teamName: 'cleanup-team',
cwd: tempClaudeRoot,
members: [{ name: 'alice' }],
},
() => {}
)
).rejects.toThrow('spawn EINVAL');
expect(mcpConfigBuilder.writeConfigFile).toHaveBeenCalledWith(tempClaudeRoot);
expect(mcpConfigBuilder.removeConfigFile).toHaveBeenCalledWith('/mock/mcp-config-create.json');
expect(teamMetaStore.deleteMeta).toHaveBeenCalledWith('cleanup-team');
});
it('removes generated MCP config when launchTeam spawn fails synchronously', async () => {
allowConsoleLogs();
const teamName = 'launch-cleanup-team';
const teamDir = path.join(tempTeamsBase, teamName);
fs.mkdirSync(teamDir, { recursive: true });
fs.writeFileSync(
path.join(teamDir, 'config.json'),
JSON.stringify({
name: teamName,
projectPath: tempClaudeRoot,
members: [{ name: 'team-lead', agentType: 'team-lead' }, { name: 'alice' }],
}),
'utf8'
);
vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue('/mock/claude');
vi.mocked(spawnCli).mockImplementation(() => {
throw new Error('launch spawn EINVAL');
});
const mcpConfigBuilder = {
writeConfigFile: vi.fn(async () => '/mock/mcp-config-launch.json'),
removeConfigFile: vi.fn(async () => {}),
};
const restorePrelaunchConfig = vi.fn(async () => {});
const svc = new TeamProvisioningService(
undefined,
undefined,
undefined,
undefined,
mcpConfigBuilder as any
);
(svc as any).buildProvisioningEnv = vi.fn(async () => ({
env: { ANTHROPIC_API_KEY: 'test' },
authSource: 'anthropic_api_key',
}));
(svc as any).resolveLaunchExpectedMembers = vi.fn(async () => ({
members: [{ name: 'alice' }],
source: 'members-meta',
warning: undefined,
}));
(svc as any).normalizeTeamConfigForLaunch = vi.fn(async () => {});
(svc as any).assertConfigLeadOnlyForLaunch = vi.fn(async () => {});
(svc as any).updateConfigProjectPath = vi.fn(async () => {});
(svc as any).restorePrelaunchConfig = restorePrelaunchConfig;
(svc as any).pathExists = vi.fn(async () => false);
await expect(svc.launchTeam({ teamName, cwd: tempClaudeRoot }, () => {})).rejects.toThrow(
'launch spawn EINVAL'
);
expect(mcpConfigBuilder.writeConfigFile).toHaveBeenCalledWith(tempClaudeRoot);
expect(mcpConfigBuilder.removeConfigFile).toHaveBeenCalledWith('/mock/mcp-config-launch.json');
expect(restorePrelaunchConfig).toHaveBeenCalledWith(teamName);
});
it('regenerates a missing --mcp-config before auth-failure respawn', async () => {
vi.useFakeTimers();
allowConsoleLogs();
vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue('/mock/claude');
const firstChild = createRunningChild();
const secondChild = createRunningChild();
vi.mocked(spawnCli)
.mockImplementationOnce(() => firstChild as any)
.mockImplementationOnce(() => secondChild as any);
const mcpConfigBuilder = {
writeConfigFile: vi
.fn()
.mockResolvedValueOnce('/missing/original-mcp-config.json')
.mockResolvedValueOnce('/regenerated/mcp-config.json'),
removeConfigFile: vi.fn(async () => {}),
};
const membersMetaStore = {
writeMembers: vi.fn(async () => {}),
};
const teamMetaStore = {
writeMeta: vi.fn(async () => {}),
deleteMeta: vi.fn(async () => {}),
};
const svc = new TeamProvisioningService(
undefined,
undefined,
membersMetaStore as any,
undefined,
mcpConfigBuilder as any,
teamMetaStore as any
);
(svc as any).buildProvisioningEnv = vi.fn(async () => ({
env: { ANTHROPIC_API_KEY: 'test' },
authSource: 'anthropic_api_key',
}));
(svc as any).pathExists = vi.fn(async () => false);
(svc as any).startFilesystemMonitor = vi.fn();
(svc as any).stopFilesystemMonitor = vi.fn();
(svc as any).startStallWatchdog = vi.fn();
(svc as any).stopStallWatchdog = vi.fn();
(svc as any).attachStdoutHandler = vi.fn();
(svc as any).attachStderrHandler = vi.fn();
const { runId } = await svc.createTeam(
{
teamName: 'retry-team',
cwd: tempClaudeRoot,
members: [{ name: 'alice' }],
},
() => {}
);
const run = (svc as any).runs.get(runId);
expect(run).toBeTruthy();
const mcpFlagIdx = run.spawnContext.args.indexOf('--mcp-config');
expect(mcpFlagIdx).toBeGreaterThanOrEqual(0);
run.spawnContext.args[mcpFlagIdx + 1] = path.join(tempClaudeRoot, 'deleted-mcp-config.json');
run.mcpConfigPath = run.spawnContext.args[mcpFlagIdx + 1];
run.authRetryInProgress = true;
const respawnPromise = (svc as any).respawnAfterAuthFailure(run);
await vi.advanceTimersByTimeAsync(2000);
await respawnPromise;
expect(mcpConfigBuilder.writeConfigFile).toHaveBeenNthCalledWith(2, tempClaudeRoot);
expect(run.spawnContext.args[mcpFlagIdx + 1]).toBe('/regenerated/mcp-config.json');
expect(run.mcpConfigPath).toBe('/regenerated/mcp-config.json');
expect(vi.mocked(spawnCli)).toHaveBeenNthCalledWith(
2,
'/mock/claude',
run.spawnContext.args,
expect.objectContaining({
cwd: tempClaudeRoot,
stdio: ['pipe', 'pipe', 'pipe'],
})
);
expect(run.child).toBe(secondChild);
if (run.timeoutHandle) {
clearTimeout(run.timeoutHandle);
run.timeoutHandle = null;
}
});
});