diff --git a/scripts/lib/opencode-live-preflight.mjs b/scripts/lib/opencode-live-preflight.mjs index a765c3ac..1fc7d734 100644 --- a/scripts/lib/opencode-live-preflight.mjs +++ b/scripts/lib/opencode-live-preflight.mjs @@ -4,6 +4,10 @@ import net from 'node:net'; import os from 'node:os'; import path from 'node:path'; +const CHILD_CLOSE_GRACE_MS = 3_000; +const CHILD_FORCE_CLOSE_GRACE_MS = 1_000; +const TASKKILL_TIMEOUT_MS = 5_000; + export async function preflightOpenCodeLiveEnvironment(input) { const repoRoot = input.repoRoot; const opencodeBin = process.env.OPENCODE_BIN?.trim() || '/opt/homebrew/bin/opencode'; @@ -139,44 +143,54 @@ async function canStartOpenCodeHost(opencodeBin, cwd, env) { } } -async function stopChild(child) { - if (child.exitCode != null || child.killed) { +async function stopChild(child, options = {}) { + const platform = options.platform ?? process.platform; + const killProcessTree = options.killProcessTree ?? taskkillProcessTree; + const closeGraceMs = options.closeGraceMs ?? CHILD_CLOSE_GRACE_MS; + const forceCloseGraceMs = options.forceCloseGraceMs ?? CHILD_FORCE_CLOSE_GRACE_MS; + + if (hasChildExited(child)) { return; } - if (process.platform === 'win32' && child.pid) { - await taskkillProcessTree(child.pid); + if (platform === 'win32' && child.pid) { + await killProcessTree(child.pid); + } else if (!child.killed) { + sendChildSignal(child, 'SIGTERM'); + } + + if (await waitForChildClose(child, closeGraceMs)) { return; } - return new Promise((resolve) => { - const timeout = setTimeout(() => { - if (child.exitCode == null) { - child.kill('SIGKILL'); - } - resolve(); - }, 3_000); - child.once('close', () => { - clearTimeout(timeout); - resolve(); - }); - child.kill('SIGTERM'); - }); + if (!hasChildExited(child)) { + sendChildSignal(child, 'SIGKILL'); + if (!(await waitForChildClose(child, forceCloseGraceMs))) { + child.stdout?.destroy(); + child.stderr?.destroy(); + child.unref?.(); + } + } } function taskkillProcessTree(pid) { return new Promise((resolve) => { let done = false; + let taskkill = null; const finish = () => { if (done) return; done = true; clearTimeout(timeout); resolve(); }; - const timeout = setTimeout(finish, 5_000); - timeout.unref?.(); + const timeout = setTimeout(() => { + if (taskkill) { + sendChildSignal(taskkill, 'SIGTERM'); + } + finish(); + }, TASKKILL_TIMEOUT_MS); try { - const taskkill = spawn( + taskkill = spawn( path.join(process.env.SystemRoot ?? 'C:\\Windows', 'System32', 'taskkill.exe'), ['/T', '/F', '/PID', String(pid)], { @@ -184,6 +198,7 @@ function taskkillProcessTree(pid) { windowsHide: true, } ); + taskkill.unref?.(); taskkill.once('error', finish); taskkill.once('close', finish); } catch { @@ -192,6 +207,36 @@ function taskkillProcessTree(pid) { }); } +function waitForChildClose(child, timeoutMs) { + if (hasChildExited(child)) { + return Promise.resolve(true); + } + + return new Promise((resolve) => { + let done = false; + const finish = (closed) => { + if (done) return; + done = true; + clearTimeout(timeout); + resolve(closed); + }; + const timeout = setTimeout(() => finish(false), timeoutMs); + child.once('close', () => finish(true)); + }); +} + +function hasChildExited(child) { + return child.exitCode != null || child.signalCode != null; +} + +function sendChildSignal(child, signal) { + try { + child.kill(signal); + } catch { + // Process may already be gone between liveness checks and the kill call. + } +} + function allocateLoopbackPort() { return new Promise((resolve, reject) => { const server = net.createServer(); @@ -224,3 +269,8 @@ function skip(reason) { function compactOutput(value) { return value.replace(/\s+/g, ' ').trim().slice(0, 1_200); } + +export const __opencodeLivePreflightTestHooks = { + stopChild, + taskkillProcessTree, +}; diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index f1f1946c..516f8047 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -152,10 +152,6 @@ import * as path from 'path'; import pidusage from 'pidusage'; import * as readline from 'readline'; -// pidusage's Windows gwmi fallback needs a non-zero cache window to finish its -// initial two-sample pass. maxage: 0 can recurse forever on Windows. -const RUNTIME_PIDUSAGE_OPTIONS = process.platform === 'win32' ? { maxage: 1_000 } : { maxage: 0 }; - import { ANTHROPIC_HELPER_MODE_COMPETING_AUTH_ENV_KEYS, type AnthropicTeamApiKeyHelperMaterial, @@ -576,6 +572,10 @@ import type { ToolCallMeta, } from '@shared/types'; +// pidusage's Windows gwmi fallback needs a non-zero cache window to finish its +// initial two-sample pass. maxage: 0 can recurse forever on Windows. +const RUNTIME_PIDUSAGE_OPTIONS = process.platform === 'win32' ? { maxage: 1_000 } : { maxage: 0 }; + const logger = createLogger('Service:TeamProvisioning'); const PREFLIGHT_DEBUG_LOG_PATH = path.join(os.tmpdir(), 'claude-team-preflight-debug.log'); diff --git a/test/main/services/team/TeamProvisioningService.test.ts b/test/main/services/team/TeamProvisioningService.test.ts index b8c8730b..80e3d67e 100644 --- a/test/main/services/team/TeamProvisioningService.test.ts +++ b/test/main/services/team/TeamProvisioningService.test.ts @@ -15453,9 +15453,9 @@ describe('TeamProvisioningService', () => { }); expect(spawnCli).toHaveBeenCalled(); - expect(progressUpdates[0]?.warnings).toEqual(expect.arrayContaining([ - expect.stringContaining('9 primary teammates'), - ])); + expect(progressUpdates[0]?.warnings).toEqual( + expect.arrayContaining([expect.stringContaining('9 primary teammates')]) + ); expect(progressUpdates[0]?.warnings?.join('\n')).toContain('Launches above 8 teammates'); }); diff --git a/test/scripts/opencodeLivePreflight.test.ts b/test/scripts/opencodeLivePreflight.test.ts new file mode 100644 index 00000000..0176d371 --- /dev/null +++ b/test/scripts/opencodeLivePreflight.test.ts @@ -0,0 +1,147 @@ +// @vitest-environment node + +import { EventEmitter } from 'events'; +import { promises as fs } from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { pathToFileURL } from 'url'; + +import { afterEach, describe, expect, it, vi } from 'vitest'; + +interface StopChildOptions { + platform?: string; + killProcessTree?: (pid: number) => Promise; + closeGraceMs?: number; + forceCloseGraceMs?: number; +} + +interface OpenCodeLivePreflightTestHooks { + __opencodeLivePreflightTestHooks: { + stopChild(child: FakeChild, options?: StopChildOptions): Promise; + taskkillProcessTree(pid: number): Promise; + }; +} + +const runOnPosix = process.platform === 'win32' ? it.skip : it; + +describe('opencode live preflight cleanup', () => { + let tempDir = ''; + const originalSystemRoot = process.env.SystemRoot; + const originalTaskkillArgsPath = process.env.FAKE_TASKKILL_ARGS_PATH; + + afterEach(async () => { + restoreEnvValue('SystemRoot', originalSystemRoot); + restoreEnvValue('FAKE_TASKKILL_ARGS_PATH', originalTaskkillArgsPath); + if (tempDir) { + await fs.rm(tempDir, { recursive: true, force: true }); + tempDir = ''; + } + }); + + it('waits for child close after Windows process-tree cleanup', async () => { + const { stopChild } = (await loadTestHooks()).__opencodeLivePreflightTestHooks; + const child = new FakeChild({ pid: 1234 }); + const killProcessTree = vi.fn(() => { + child.signalCode = 'SIGTERM'; + child.emit('close'); + return Promise.resolve(); + }); + + await stopChild(child, { + closeGraceMs: 5, + forceCloseGraceMs: 5, + killProcessTree, + platform: 'win32', + }); + + expect(killProcessTree).toHaveBeenCalledWith(1234); + expect(child.kill).not.toHaveBeenCalled(); + expect(child.stdout.destroy).not.toHaveBeenCalled(); + expect(child.unref).not.toHaveBeenCalled(); + }); + + it('detaches pipes when Windows process-tree cleanup and direct kill both fail to close', async () => { + const { stopChild } = (await loadTestHooks()).__opencodeLivePreflightTestHooks; + const child = new FakeChild({ pid: 5678 }); + const killProcessTree = vi.fn(() => Promise.resolve()); + + await stopChild(child, { + closeGraceMs: 1, + forceCloseGraceMs: 1, + killProcessTree, + platform: 'win32', + }); + + expect(killProcessTree).toHaveBeenCalledWith(5678); + expect(child.kill).toHaveBeenCalledWith('SIGKILL'); + expect(child.stdout.destroy).toHaveBeenCalled(); + expect(child.stderr.destroy).toHaveBeenCalled(); + expect(child.unref).toHaveBeenCalled(); + }); + + runOnPosix('invokes taskkill.exe with process-tree flags', async () => { + const { taskkillProcessTree } = (await loadTestHooks()).__opencodeLivePreflightTestHooks; + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'opencode-taskkill-test-')); + const system32Dir = path.join(tempDir, 'System32'); + const taskkillArgsPath = path.join(tempDir, 'taskkill-args.txt'); + + await fs.mkdir(system32Dir, { recursive: true }); + await writeExecutable(path.join(system32Dir, 'taskkill.exe'), fakeTaskkillScript()); + process.env.SystemRoot = tempDir; + process.env.FAKE_TASKKILL_ARGS_PATH = taskkillArgsPath; + + await taskkillProcessTree(4242); + + await expect(fs.readFile(taskkillArgsPath, 'utf8')).resolves.toBe('/T /F /PID 4242\n'); + }); +}); + +class FakeChild extends EventEmitter { + readonly kill = vi.fn(); + readonly stderr = { destroy: vi.fn() }; + readonly stdout = { destroy: vi.fn() }; + readonly unref = vi.fn(); + exitCode: number | null = null; + killed = false; + pid: number; + signalCode: string | null = null; + + constructor(input: { pid: number }) { + super(); + this.pid = input.pid; + this.kill.mockImplementation((signal: string) => { + this.killed = true; + return signal === 'SIGKILL'; + }); + } +} + +async function loadTestHooks(): Promise { + const moduleUrl = pathToFileURL( + path.join(process.cwd(), 'scripts/lib/opencode-live-preflight.mjs') + ).href; + return (await import(`${moduleUrl}?t=${Date.now()}`)) as OpenCodeLivePreflightTestHooks; +} + +async function writeExecutable(filePath: string, content: string): Promise { + await fs.writeFile(filePath, content, 'utf8'); + // eslint-disable-next-line sonarjs/file-permissions -- The taskkill fixture must be executable for child_process.spawn. + await fs.chmod(filePath, 0o755); +} + +function fakeTaskkillScript(): string { + return `#!/usr/bin/env node +const fs = require('node:fs'); + +fs.writeFileSync(process.env.FAKE_TASKKILL_ARGS_PATH, process.argv.slice(2).join(' ') + '\\n'); +process.exit(0); +`; +} + +function restoreEnvValue(name: string, value: string | undefined): void { + if (value === undefined) { + delete process.env[name]; + return; + } + process.env[name] = value; +}