From 48826af00b6fa9631391715dcd30c29669bcb3a1 Mon Sep 17 00:00:00 2001 From: 777genius Date: Sat, 16 May 2026 02:14:34 +0300 Subject: [PATCH] fix(team): harden MCP HTTP startup cleanup --- .../services/team/AgentTeamsMcpHttpServer.ts | 56 +++++++++++++------ .../OpenCodeManagedHostProcessCleanup.ts | 8 +-- .../team/AgentTeamsMcpHttpServer.test.ts | 52 +++++++++++++++++ .../OpenCodeManagedHostProcessCleanup.test.ts | 27 +++++++++ 4 files changed, 121 insertions(+), 22 deletions(-) diff --git a/src/main/services/team/AgentTeamsMcpHttpServer.ts b/src/main/services/team/AgentTeamsMcpHttpServer.ts index 443885a6..60c4ebc3 100644 --- a/src/main/services/team/AgentTeamsMcpHttpServer.ts +++ b/src/main/services/team/AgentTeamsMcpHttpServer.ts @@ -168,37 +168,57 @@ export class AgentTeamsMcpHttpServer { this.handle = null; } }; - child.once('exit', (code, signal) => { - clearIfCurrent(); - logger.warn( - `Agent Teams MCP HTTP server exited${typeof code === 'number' ? ` with code ${code}` : ''}${ - signal ? ` (${signal})` : '' - }` - ); - }); - child.once('error', (error) => { - clearIfCurrent(); - logger.warn( - `Agent Teams MCP HTTP server process error: ${ - error instanceof Error ? error.message : String(error) - }` - ); - }); child.stderr?.on('data', (chunk: Buffer) => { const text = chunk.toString('utf8').trim(); if (text) { logger.debug(`Agent Teams MCP HTTP stderr: ${text.slice(0, 1000)}`); } }); + this.child = child; + + let startupSettled = false; + const startupFailure = new Promise((_, reject) => { + child.once('exit', (code, signal) => { + clearIfCurrent(); + const codeSuffix = typeof code === 'number' ? ` with code ${code}` : ''; + const signalSuffix = signal ? ` (${signal})` : ''; + const message = `Agent Teams MCP HTTP server exited before startup completed${codeSuffix}${signalSuffix}`; + if (!startupSettled) { + reject(new Error(message)); + } + logger.warn(message); + }); + child.once('error', (error) => { + clearIfCurrent(); + const message = `Agent Teams MCP HTTP server process error: ${ + error instanceof Error ? error.message : String(error) + }`; + if (!startupSettled) { + reject(error instanceof Error ? error : new Error(message)); + } + logger.warn(message); + }); + }); try { - await waitForPort(MCP_HTTP_HOST, port, MCP_HTTP_READY_TIMEOUT_MS); + await Promise.race([ + waitForPort(MCP_HTTP_HOST, port, MCP_HTTP_READY_TIMEOUT_MS), + startupFailure, + ]); + if (this.child !== child) { + throw new Error('Agent Teams MCP HTTP server exited before startup completed'); + } } catch (error) { + startupSettled = true; + if (this.child === child) { + this.child = null; + this.handle = null; + } killProcessTree(child, 'SIGKILL'); throw error; } - this.child = child; + startupSettled = true; this.handle = { url: `http://${MCP_HTTP_HOST}:${port}${MCP_HTTP_ENDPOINT}`, port, diff --git a/src/main/services/team/opencode/bridge/OpenCodeManagedHostProcessCleanup.ts b/src/main/services/team/opencode/bridge/OpenCodeManagedHostProcessCleanup.ts index 3abb90ad..d684e77b 100644 --- a/src/main/services/team/opencode/bridge/OpenCodeManagedHostProcessCleanup.ts +++ b/src/main/services/team/opencode/bridge/OpenCodeManagedHostProcessCleanup.ts @@ -96,13 +96,13 @@ export async function cleanupManagedOpenCodeServeProcesses( } const details = await readDetails(row.pid); + const isManagedByWindowsCommand = + platform === 'win32' && isAppManagedWindowsOpenCodeServeCommand(row.command); const isManaged = - platform === 'win32' - ? isAppManagedWindowsOpenCodeServeCommand(row.command) || - Boolean(details && isManagedOpenCodeServeProcessDetails(details)) - : Boolean(details && isManagedOpenCodeServeProcessDetails(details)); + isManagedByWindowsCommand || Boolean(details && isManagedOpenCodeServeProcessDetails(details)); const hasRequiredDetailsMarkers = requiredDetailsMarkers.length === 0 || + (isManagedByWindowsCommand && details === null) || Boolean(details && processDetailsIncludeMarkers(details, requiredDetailsMarkers)); if (!isManaged || !hasRequiredDetailsMarkers) { result.candidates.push({ diff --git a/test/main/services/team/AgentTeamsMcpHttpServer.test.ts b/test/main/services/team/AgentTeamsMcpHttpServer.test.ts index 0801ef07..a964b49e 100644 --- a/test/main/services/team/AgentTeamsMcpHttpServer.test.ts +++ b/test/main/services/team/AgentTeamsMcpHttpServer.test.ts @@ -145,6 +145,58 @@ describe('AgentTeamsMcpHttpServer', () => { expect(spawnProcess).toHaveBeenCalledTimes(1); }); + it('fails startup promptly when the child exits before readiness', async () => { + const child = new FakeChildProcess(); + const server = new AgentTeamsMcpHttpServer({ + resolveLaunchSpec: async () => ({ + command: 'node', + args: ['mcp-server/dist/index.js'], + }), + allocatePort: async () => 41003, + spawnProcess: vi.fn(() => child as any), + waitForPort: vi.fn(() => { + child.emit('exit', 1, null); + return new Promise(() => { + // Keep readiness pending so startup resolves only through the child exit. + }); + }), + }); + + await expect(server.ensureStarted()).rejects.toThrow( + 'Agent Teams MCP HTTP server exited before startup completed with code 1' + ); + expect(hoisted.killProcessTreeMock).toHaveBeenCalledWith(child, 'SIGKILL'); + expect(vi.mocked(console.warn).mock.calls[0]?.join(' ')).toContain( + 'Agent Teams MCP HTTP server exited before startup completed with code 1' + ); + vi.mocked(console.warn).mockClear(); + }); + + it('does not return a handle if the child exits during readiness polling', async () => { + const child = new FakeChildProcess(); + const server = new AgentTeamsMcpHttpServer({ + resolveLaunchSpec: async () => ({ + command: 'node', + args: ['mcp-server/dist/index.js'], + }), + allocatePort: async () => 41004, + spawnProcess: vi.fn(() => child as any), + waitForPort: vi.fn(async () => { + await Promise.resolve(); + child.emit('exit', 0, null); + }), + }); + + await expect(server.ensureStarted()).rejects.toThrow( + 'Agent Teams MCP HTTP server exited before startup completed' + ); + expect(hoisted.killProcessTreeMock).toHaveBeenCalledWith(child, 'SIGKILL'); + expect(vi.mocked(console.warn).mock.calls[0]?.join(' ')).toContain( + 'Agent Teams MCP HTTP server exited before startup completed with code 0' + ); + vi.mocked(console.warn).mockClear(); + }); + it('waits for the HTTP health endpoint before marking the server ready', async () => { const child = new FakeChildProcess(); const port = await allocateLoopbackPort(); diff --git a/test/main/services/team/OpenCodeManagedHostProcessCleanup.test.ts b/test/main/services/team/OpenCodeManagedHostProcessCleanup.test.ts index 9561c40c..2b9ad444 100644 --- a/test/main/services/team/OpenCodeManagedHostProcessCleanup.test.ts +++ b/test/main/services/team/OpenCodeManagedHostProcessCleanup.test.ts @@ -403,6 +403,33 @@ describe('OpenCodeManagedHostProcessCleanup', () => { expect(result.diagnostics).toEqual([]); }); + it('does not require unreadable Windows details for app-managed command fallback cleanup', async () => { + const killProcess = vi.fn(); + + const result = await cleanupManagedOpenCodeServeProcesses({ + mode: 'force', + platform: 'win32', + requiredDetailsMarkers: ['CLAUDE_TEAM_APP_INSTANCE_ID=app-1'], + listProcessRows: () => + resolved([ + { + pid: 71629, + ppid: 86256, + command: + '"C:\\Users\\User\\AppData\\Roaming\\claude-agent-teams-ui\\data\\runtimes\\opencode\\versions\\1.14.48\\opencode-windows-x64\\opencode.exe" serve --hostname 127.0.0.1 --port 49914', + }, + ]), + readProcessDetails: () => resolved(null), + disposeServeHost: () => resolved(undefined), + isProcessAlive: () => false, + killProcess, + }); + + expect(killProcess).toHaveBeenCalledWith(71629); + expect(result.candidates[0]).toMatchObject({ pid: 71629, action: 'killed' }); + expect(result.diagnostics).toEqual([]); + }); + it('keeps app-managed Windows OpenCode serve processes while their parent is still alive', async () => { const killProcess = vi.fn();