Merge pull request #1854 from bashrusakh/fix/1841-async-reaper

fix(web): make managed process reaper async

packages/web/server/lib/opencode/lifecycle.js

@@ -361,7 +361,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);
       }
     }
   };
@@ -514,7 +514,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,

packages/web/server/lib/opencode/lifecycle.test.js

@@ -8,6 +8,11 @@ const recordStartupPerformanceMock = vi.fn();
 vi.mock('node:child_process', () => ({
   spawn: spawnMock,
   spawnSync: spawnSyncMock,
+  // `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(),
 }));
 vi.mock('./startup-performance.js', () => ({
   recordStartupPerformance: recordStartupPerformanceMock,

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
+      }
     }
   }
 

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();
+  });
+});