diff --git a/packages/web/server/lib/opencode/lifecycle.js b/packages/web/server/lib/opencode/lifecycle.js index d01a9fd3..31f3341f 100644 --- a/packages/web/server/lib/opencode/lifecycle.js +++ b/packages/web/server/lib/opencode/lifecycle.js @@ -221,7 +221,7 @@ export const createOpenCodeLifecycleRuntime = (deps) => { // Drop it from the registry only once it has actually exited, so a child // that survived teardown stays eligible for the next run's reaper. if (Number.isInteger(pid) && hasChildProcessExited(child)) { - unregisterManagedProcess(pid); + await unregisterManagedProcess(pid); } } }; @@ -343,7 +343,7 @@ export const createOpenCodeLifecycleRuntime = (deps) => { // actual host (Electron sets OPENCHAMBER_RUNTIME='desktop'; the standalone // web CLI leaves it unset → 'web'; SSH remote → 'ssh-remote') rather than a // hardcoded label, matching the server's existing runtimeName convention. - registerManagedProcess({ + await registerManagedProcess({ pid: child.pid, ownerPid: process.pid, port, diff --git a/packages/web/server/lib/opencode/lifecycle.test.js b/packages/web/server/lib/opencode/lifecycle.test.js index 19dbe18d..39ebcdc7 100644 --- a/packages/web/server/lib/opencode/lifecycle.test.js +++ b/packages/web/server/lib/opencode/lifecycle.test.js @@ -6,6 +6,11 @@ const spawnMock = vi.fn(); vi.mock('node:child_process', () => ({ spawn: spawnMock, spawnSync: vi.fn(), + // `managed-process-registry.js` (imported transitively via lifecycle.js) + // calls `promisify(execFile)` at module load, so the mock must expose a + // function here. Lifecycle tests don't exercise the reaper path, so a plain + // stub is enough; the registry's best-effort writes are no-ops on errors. + execFile: vi.fn(), })); const { createOpenCodeLifecycleRuntime } = await import('./lifecycle.js'); diff --git a/packages/web/server/lib/opencode/managed-process-registry.js b/packages/web/server/lib/opencode/managed-process-registry.js index 2e225bce..a6b6927b 100644 --- a/packages/web/server/lib/opencode/managed-process-registry.js +++ b/packages/web/server/lib/opencode/managed-process-registry.js @@ -32,13 +32,26 @@ // been reparented to init/pid 1, or the recorded owner pid is dead. A // child still owned by a live instance is left untouched. // +// All filesystem and child-process operations here are ASYNCHRONOUS. The web +// server runs in-process inside the Electron main event loop (and other hosts), +// so any `spawnSync`/`*Sync` FS call blocks the single event loop — which also +// serves UI asset requests and realtime SSE traffic. The startup reaper can +// iterate several registry entries and, on Windows, each one spawns `tasklist` +// (100-500ms) and possibly `taskkill`; doing that synchronously stalls the +// whole process and is what caused the 1.13.3 `openchamber-ui://` lag +// regression (#1841). `execFile`/`fsp.*` keep the event loop responsive while +// the reaper waits on the kernel. +// // The VS Code extension cannot import this module (it does not bundle the web // package); it carries a parity implementation that reads/writes the SAME dir. -import fs from 'node:fs'; +import fsp from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; -import { spawnSync } from 'node:child_process'; +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; + +const execFileAsync = promisify(execFile); const resolveRegistryDir = () => { const override = process.env.OPENCHAMBER_MANAGED_PROCESS_REGISTRY; @@ -48,49 +61,53 @@ const resolveRegistryDir = () => { const entryFilePath = (pid) => path.join(resolveRegistryDir(), `${pid}.json`); -const writeEntryFile = (entry) => { +const writeEntryFile = async (entry) => { const dir = resolveRegistryDir(); try { - fs.mkdirSync(dir, { recursive: true }); + await fsp.mkdir(dir, { recursive: true }); const filePath = path.join(dir, `${entry.pid}.json`); const tmp = `${filePath}.tmp-${process.pid}`; - fs.writeFileSync(tmp, JSON.stringify(entry, null, 2)); - fs.renameSync(tmp, filePath); + await fsp.writeFile(tmp, JSON.stringify(entry, null, 2)); + await fsp.rename(tmp, filePath); } catch { // Best-effort: a failed registry write must never break spawn/shutdown. } }; -const readAllEntries = () => { +const readAllEntries = async () => { const dir = resolveRegistryDir(); let names = []; try { - names = fs.readdirSync(dir).filter((name) => name.endsWith('.json')); + names = await fsp.readdir(dir); } catch { return []; } const out = []; - for (const name of names) { + for (const name of names.filter((value) => value.endsWith('.json'))) { const filePath = path.join(dir, name); try { - const entry = JSON.parse(fs.readFileSync(filePath, 'utf8')); + const entry = JSON.parse(await fsp.readFile(filePath, 'utf8')); if (entry && Number.isInteger(entry.pid)) { out.push({ entry, filePath }); } else { - fs.rmSync(filePath, { force: true }); + await fsp.rm(filePath, { force: true }); } } catch { // Corrupt/partial file — drop it. - try { fs.rmSync(filePath, { force: true }); } catch {} + try { + await fsp.rm(filePath, { force: true }); + } catch { + // ignore + } } } return out; }; /** Record an OpenCode process WE spawned so a future run can reap it if orphaned. */ -export const registerManagedProcess = ({ pid, ownerPid, port, binary, runtime } = {}) => { +export const registerManagedProcess = async ({ pid, ownerPid, port, binary, runtime } = {}) => { if (!Number.isInteger(pid)) return; - writeEntryFile({ + await writeEntryFile({ pid, ownerPid: Number.isInteger(ownerPid) ? ownerPid : process.pid, port: Number.isInteger(port) ? port : null, @@ -101,11 +118,12 @@ export const registerManagedProcess = ({ pid, ownerPid, port, binary, runtime } }; /** Drop a pid from the registry (after we have killed/closed it ourselves). */ -export const unregisterManagedProcess = (pid) => { +export const unregisterManagedProcess = async (pid) => { if (!Number.isInteger(pid)) return; try { - fs.rmSync(entryFilePath(pid), { force: true }); + await fsp.rm(entryFilePath(pid), { force: true }); } catch { + // Best-effort: dropping a missing file is not an error. } }; @@ -123,14 +141,14 @@ const isPidAlive = (pid) => { const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); // Returns { ppid, command } for a live pid on Unix, or null if it can't be read. -const readUnixProcInfo = (pid) => { +const readUnixProcInfo = async (pid) => { try { - const result = spawnSync('ps', ['-p', String(pid), '-o', 'ppid=,command='], { + const { stdout } = await execFileAsync('ps', ['-p', String(pid), '-o', 'ppid=,command='], { encoding: 'utf8', timeout: 3000, windowsHide: true, }); - const line = (result.stdout || '').trim(); + const line = (stdout || '').trim(); if (!line) return null; const match = line.match(/^\s*(\d+)\s+(.*)$/); if (!match) return null; @@ -141,14 +159,14 @@ const readUnixProcInfo = (pid) => { }; // Windows image name for a pid (e.g. "opencode.exe"), or null. -const readWindowsImageName = (pid) => { +const readWindowsImageName = async (pid) => { try { - const result = spawnSync('tasklist', ['/FI', `PID eq ${pid}`, '/FO', 'CSV', '/NH'], { + const { stdout } = await execFileAsync('tasklist', ['/FI', `PID eq ${pid}`, '/FO', 'CSV', '/NH'], { encoding: 'utf8', timeout: 3000, windowsHide: true, }); - return (result.stdout || '').trim() || null; + return (stdout || '').trim() || null; } catch { return null; } @@ -167,15 +185,28 @@ const commandIdentifiesOurServer = (command, entry) => { const killOrphan = async (pid) => { if (process.platform === 'win32') { try { - spawnSync('taskkill', ['/PID', String(pid), '/T', '/F'], { stdio: 'ignore', timeout: 5000, windowsHide: true }); + await execFileAsync('taskkill', ['/PID', String(pid), '/T', '/F'], { + stdio: 'ignore', + timeout: 5000, + windowsHide: true, + }); } catch { + // Best-effort: a failed kill is not fatal (startup reaper is a backstop). } return; } const signalTree = (signal) => { - try { process.kill(-pid, signal); } catch {} - try { process.kill(pid, signal); } catch {} + try { + process.kill(-pid, signal); + } catch { + // process group may already be gone + } + try { + process.kill(pid, signal); + } catch { + // pid may already be gone + } }; signalTree('SIGTERM'); @@ -196,7 +227,7 @@ const processEntry = async (entry, { log }) => { const ownerGone = Number.isInteger(entry.ownerPid) && !isPidAlive(entry.ownerPid); if (process.platform === 'win32') { - const image = readWindowsImageName(entry.pid); + const image = await readWindowsImageName(entry.pid); const looksLikeOpencode = typeof image === 'string' && image.toLowerCase().includes('opencode'); // Windows lacks reliable reparent-to-1 semantics (job objects usually kill // children with the parent), so we reap only when the owner is provably dead @@ -209,7 +240,7 @@ const processEntry = async (entry, { log }) => { return false; } - const info = readUnixProcInfo(entry.pid); + const info = await readUnixProcInfo(entry.pid); // Can't verify identity (or it's not our server) → leave it alone. if (!info || !commandIdentifiesOurServer(info.command, entry)) return false; @@ -227,7 +258,7 @@ const processEntry = async (entry, { log }) => { * server. Returns { inspected, reaped }. */ export const reapOrphanedProcesses = async ({ log } = {}) => { - const records = readAllEntries(); + const records = await readAllEntries(); if (records.length === 0) return { inspected: 0, reaped: 0 }; let reaped = 0; @@ -243,7 +274,11 @@ export const reapOrphanedProcesses = async ({ log } = {}) => { log?.(`[lifecycle] reap check failed for pid ${entry.pid}: ${error?.message ?? error}`); } if (drop) { - try { fs.rmSync(filePath, { force: true }); } catch {} + try { + await fsp.rm(filePath, { force: true }); + } catch { + // best-effort + } } } diff --git a/packages/web/server/lib/opencode/managed-process-registry.test.mjs b/packages/web/server/lib/opencode/managed-process-registry.test.mjs new file mode 100644 index 00000000..7f1891c7 --- /dev/null +++ b/packages/web/server/lib/opencode/managed-process-registry.test.mjs @@ -0,0 +1,306 @@ +import { promisify } from 'node:util'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// Mocks must be in place before the module under test is imported, because the +// module calls `promisify(execFile)` at module-load time and binds `fsp.*` at +// call time. +// +// NOTE on `promisify.custom`: the real `child_process.execFile` carries a +// `[util.promisify.custom]` symbol so that `promisify(execFile)` resolves to +// `{ stdout, stderr }` (not the generic multi-arg array). A plain `vi.fn()` +// mock lacks that symbol, so `const { stdout } = await execFileAsync(...)` +// would destructure `undefined`. We attach the symbol to the mock so the +// promisified helper used by the module resolves to the same `{ stdout, +// stderr }` shape. + +const readdirMock = vi.fn(); +const readFileMock = vi.fn(); +const rmMock = vi.fn(); +const mkdirMock = vi.fn(); +const writeFileMock = vi.fn(); +const renameMock = vi.fn(); + +vi.mock('node:fs/promises', () => ({ + default: { + readdir: readdirMock, + readFile: readFileMock, + rm: rmMock, + mkdir: mkdirMock, + writeFile: writeFileMock, + rename: renameMock, + }, +})); + +// `execFileImpl` is the swappable per-test implementation; `execFileMock` is +// what the mocked module sees. `promisify(execFileMock)` returns the custom +// function, which delegates to `execFileImpl` with a (err, stdout, stderr) +// callback and resolves to `{ stdout, stderr }`. +const execFileImpl = vi.fn(); +const execFileMock = vi.fn(); +execFileMock[promisify.custom] = (cmd, args, opts) => + new Promise((resolve, reject) => { + execFileImpl(cmd, args, opts, (err, stdout, stderr) => + err ? reject(err) : resolve({ stdout: stdout ?? '', stderr: stderr ?? '' })); + }); + +vi.mock('node:child_process', () => ({ + execFile: execFileMock, +})); + +const { + registerManagedProcess, + unregisterManagedProcess, + reapOrphanedProcesses, +} = await import('./managed-process-registry.js'); + +const ORIGINAL_PLATFORM = Object.getOwnPropertyDescriptor(process, 'platform'); +const ORIGINAL_KILL = process.kill; +const killMock = vi.fn(); + +const setPlatform = (platform) => { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); +}; + +const restorePlatform = () => { + if (ORIGINAL_PLATFORM) { + Object.defineProperty(process, 'platform', ORIGINAL_PLATFORM); + } +}; + +const installKillMock = () => { + Object.defineProperty(process, 'kill', { value: killMock, configurable: true }); +}; + +const restoreKill = () => { + Object.defineProperty(process, 'kill', { value: ORIGINAL_KILL, configurable: true }); +}; + +// Helper to make given pids look alive on signal-0 (returns true); any other +// pid throws ESRCH (dead). Non-zero signals always "succeed" so `killOrphan`'s +// signalTree is inert under test. +const killAliveFor = (alivePids) => + killMock.mockImplementation((pid, signal) => { + if (signal === 0 || signal === undefined) { + if (alivePids.includes(pid)) return true; + const error = new Error('ESRCH'); + error.code = 'ESRCH'; + throw error; + } + return true; + }); + +// Configure `execFileImpl` with a (cmd, args, opts, cb) dispatcher. +const execFileYields = (dispatch) => + execFileImpl.mockImplementation((cmd, args, opts, cb) => dispatch(cmd, args, opts, cb)); + +beforeEach(() => { + readdirMock.mockReset(); + readFileMock.mockReset(); + rmMock.mockReset(); + mkdirMock.mockReset(); + writeFileMock.mockReset(); + renameMock.mockReset(); + execFileImpl.mockReset(); + killMock.mockReset(); + installKillMock(); +}); + +afterEach(() => { + restoreKill(); + restorePlatform(); +}); + +describe('reapOrphanedProcesses', () => { + it('returns zero counts when the registry directory is missing', async () => { + readdirMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); + + const result = await reapOrphanedProcesses(); + + expect(result).toEqual({ inspected: 0, reaped: 0 }); + expect(execFileImpl).not.toHaveBeenCalled(); + }); + + it('drops registry entries whose pid is already dead, without spawning anything', async () => { + readdirMock.mockResolvedValue(['99999.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 99999, ownerPid: 12345, port: 4096, binary: '/opencode', runtime: 'web' }), + ); + killMock.mockImplementation(() => { + const error = new Error('ESRCH'); + error.code = 'ESRCH'; + throw error; + }); + rmMock.mockResolvedValue(); + + const result = await reapOrphanedProcesses(); + + expect(result).toEqual({ inspected: 1, reaped: 0 }); + expect(rmMock).toHaveBeenCalledTimes(1); + expect(execFileImpl).not.toHaveBeenCalled(); + }); + + describe('on Windows', () => { + beforeEach(() => setPlatform('win32')); + + it('reaps an opencode image whose owner is gone', async () => { + readdirMock.mockResolvedValue(['777.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 777, ownerPid: 12345, port: 4096, binary: 'opencode.exe', runtime: 'desktop' }), + ); + // pid 777 alive, owner 12345 dead. + killAliveFor([777]); + execFileYields((cmd, _args, _opts, cb) => { + if (cmd === 'tasklist') return cb(null, 'opencode.exe', ''); + if (cmd === 'taskkill') return cb(null, '', ''); + cb(new Error(`unexpected cmd: ${cmd}`)); + }); + rmMock.mockResolvedValue(); + + const result = await reapOrphanedProcesses({ log: () => {} }); + + expect(result).toEqual({ inspected: 1, reaped: 1 }); + expect(execFileImpl).toHaveBeenCalledWith( + 'tasklist', + expect.any(Array), + expect.objectContaining({ windowsHide: true }), + expect.any(Function), + ); + expect(execFileImpl).toHaveBeenCalledWith( + 'taskkill', + expect.any(Array), + expect.objectContaining({ windowsHide: true }), + expect.any(Function), + ); + }); + + it('leaves a non-opencode image alone even if the owner is gone', async () => { + readdirMock.mockResolvedValue(['777.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 777, ownerPid: 12345, port: 4096, binary: 'opencode.exe', runtime: 'desktop' }), + ); + killAliveFor([777]); + execFileYields((_cmd, _args, _opts, cb) => cb(null, 'notepad.exe', '')); + rmMock.mockResolvedValue(); + + const result = await reapOrphanedProcesses({ log: () => {} }); + + expect(result).toEqual({ inspected: 1, reaped: 0 }); + const calls = execFileImpl.mock.calls.filter(([cmd]) => cmd === 'taskkill'); + expect(calls).toHaveLength(0); + }); + + it('leaves an opencode image whose owner is still alive', async () => { + readdirMock.mockResolvedValue(['777.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 777, ownerPid: 12345, port: 4096, binary: 'opencode.exe', runtime: 'desktop' }), + ); + // Both alive. + killAliveFor([777, 12345]); + execFileYields((_cmd, _args, _opts, cb) => cb(null, 'opencode.exe', '')); + + const result = await reapOrphanedProcesses({ log: () => {} }); + + expect(result).toEqual({ inspected: 1, reaped: 0 }); + const calls = execFileImpl.mock.calls.filter(([cmd]) => cmd === 'taskkill'); + expect(calls).toHaveLength(0); + }); + }); + + describe('on Unix', () => { + beforeEach(() => setPlatform('linux')); + + it('reaps a reparented opencode serve matching the recorded port', async () => { + readdirMock.mockResolvedValue(['777.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 777, ownerPid: 12345, port: 4096, binary: '/opencode', runtime: 'web' }), + ); + // pid 777 stays "alive"; killOrphan's signalTree is inert (mock returns + // true for non-zero signals), and its wait loop sees isPidAlive true so + // it exhausts the SIGTERM wait then sends SIGKILL and sleeps 300ms. + killAliveFor([777]); + execFileYields((cmd, _args, _opts, cb) => { + if (cmd === 'ps') return cb(null, '1 /usr/bin/opencode serve --port 4096\n', ''); + cb(new Error(`unexpected cmd: ${cmd}`)); + }); + rmMock.mockResolvedValue(); + + const result = await reapOrphanedProcesses({ log: () => {} }); + + expect(result).toEqual({ inspected: 1, reaped: 1 }); + }); + + it('leaves a process whose command is not our opencode serve', async () => { + readdirMock.mockResolvedValue(['777.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 777, ownerPid: 12345, port: 4096, binary: '/opencode', runtime: 'web' }), + ); + killAliveFor([777]); + execFileYields((cmd, _args, _opts, cb) => { + if (cmd === 'ps') return cb(null, '1 /some/other/binary serve\n', ''); + cb(new Error(`unexpected cmd: ${cmd}`)); + }); + + const result = await reapOrphanedProcesses({ log: () => {} }); + + expect(result).toEqual({ inspected: 1, reaped: 0 }); + }); + + it('leaves a process still owned by a live owner (not reparented)', async () => { + readdirMock.mockResolvedValue(['777.json']); + readFileMock.mockResolvedValue( + JSON.stringify({ pid: 777, ownerPid: 12345, port: 4096, binary: '/opencode', runtime: 'web' }), + ); + killAliveFor([777, 12345]); + execFileYields((cmd, _args, _opts, cb) => { + if (cmd === 'ps') return cb(null, '12345 /usr/bin/opencode serve --port 4096\n', ''); + cb(new Error(`unexpected cmd: ${cmd}`)); + }); + + const result = await reapOrphanedProcesses({ log: () => {} }); + + expect(result).toEqual({ inspected: 1, reaped: 0 }); + }); + }); +}); + +describe('registerManagedProcess', () => { + it('writes an entry file atomically via tmp + rename', async () => { + mkdirMock.mockResolvedValue(); + writeFileMock.mockResolvedValue(); + renameMock.mockResolvedValue(); + + await registerManagedProcess({ pid: 4242, ownerPid: 12345, port: 4096, binary: '/opencode', runtime: 'desktop' }); + + expect(mkdirMock).toHaveBeenCalledWith(expect.any(String), { recursive: true }); + expect(writeFileMock).toHaveBeenCalledWith( + expect.stringContaining('4242.json.tmp-'), + expect.any(String), + ); + expect(renameMock).toHaveBeenCalledWith( + expect.stringContaining('4242.json.tmp-'), + expect.stringContaining('4242.json'), + ); + }); + + it('is a no-op for a non-integer pid', async () => { + await registerManagedProcess({ pid: 'not-a-pid' }); + + expect(writeFileMock).not.toHaveBeenCalled(); + }); +}); + +describe('unregisterManagedProcess', () => { + it('removes the entry file', async () => { + rmMock.mockResolvedValue(); + + await unregisterManagedProcess(4242); + + expect(rmMock).toHaveBeenCalledWith(expect.stringContaining('4242.json'), { force: true }); + }); + + it('is a no-op for a non-integer pid', async () => { + await unregisterManagedProcess(undefined); + + expect(rmMock).not.toHaveBeenCalled(); + }); +});