fix: harden and de-slop the merged contribution batch
Follow-ups promised on merge, plus review findings on the batch itself: - chat: task-tool output now respects the 512KiB render cap; quick-open icon is visible at rest on coarse pointers and reachable by keyboard (row keydown no longer swallows inner-button Enter/Space); composer inline-code decoration drops the metric-shifting padding; a btw fork send carries only the boundary instruction, never the promotion notice - sync: cascade revert/unrevert aborts busy descendants, busy state is read from every child store at the moment of use; rule 9 documents redo clearing all descendant revert markers - electron: renderer recovery keeps memory-eviction (a valid render-process-gone reason) and both windows share one attachRendererRecovery helper - vscode: process registry is a thin re-export of the web module (provider-env-aliases precedent) with ordered register/unregister writes and an awaited close - server/cli: managed-process registry takes injectable deps (fixes the unreaped-orphans ReferenceError), corrupt settings errors name the file, getWorktrees test restores console.warn - tests: module-mock harnesses removed (AgentsSidebar, SettingsView mobile focus — behaviors stay live but uncovered, accepted trade), QuestionMarkdown asserts rendered DOM - i18n: German gains the debug-panel request keys, Japanese/German drop removed worktree keys, Ukrainian unit spacing fixed - changelog: Copilot AI Credits entries (main + VS Code)
This commit is contained in:
Bohdan Triapitsyn
@@ -0,0 +1,13 @@
|
||||
export function registerManagedProcess(entry: {
|
||||
pid?: number;
|
||||
ownerPid?: number;
|
||||
port?: number | null;
|
||||
binary?: string | null;
|
||||
runtime?: string;
|
||||
}): Promise<void>;
|
||||
|
||||
export function unregisterManagedProcess(pid?: number): Promise<void>;
|
||||
|
||||
export function reapOrphanedProcesses(options?: {
|
||||
log?: (message: string) => void;
|
||||
}): Promise<{ inspected: number; reaped: number }>;
|
||||
@@ -51,7 +51,7 @@ import path from 'node:path';
|
||||
import { execFile } from 'node:child_process';
|
||||
import { promisify } from 'node:util';
|
||||
|
||||
const execFileAsync = promisify(execFile);
|
||||
const defaultExecFileAsync = promisify(execFile);
|
||||
|
||||
const resolveRegistryDir = () => {
|
||||
const override = process.env.OPENCHAMBER_MANAGED_PROCESS_REGISTRY;
|
||||
@@ -61,72 +61,6 @@ const resolveRegistryDir = () => {
|
||||
|
||||
const entryFilePath = (pid) => path.join(resolveRegistryDir(), `${pid}.json`);
|
||||
|
||||
const writeEntryFile = async (entry) => {
|
||||
const dir = resolveRegistryDir();
|
||||
try {
|
||||
await fsp.mkdir(dir, { recursive: true });
|
||||
const filePath = path.join(dir, `${entry.pid}.json`);
|
||||
const tmp = `${filePath}.tmp-${process.pid}`;
|
||||
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 = async () => {
|
||||
const dir = resolveRegistryDir();
|
||||
let names = [];
|
||||
try {
|
||||
names = await fsp.readdir(dir);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
const out = [];
|
||||
for (const name of names.filter((value) => value.endsWith('.json'))) {
|
||||
const filePath = path.join(dir, name);
|
||||
try {
|
||||
const entry = JSON.parse(await fsp.readFile(filePath, 'utf8'));
|
||||
if (entry && Number.isInteger(entry.pid)) {
|
||||
out.push({ entry, filePath });
|
||||
} else {
|
||||
await fsp.rm(filePath, { force: true });
|
||||
}
|
||||
} catch {
|
||||
// Corrupt/partial file — drop it.
|
||||
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 = async ({ pid, ownerPid, port, binary, runtime } = {}) => {
|
||||
if (!Number.isInteger(pid)) return;
|
||||
await writeEntryFile({
|
||||
pid,
|
||||
ownerPid: Number.isInteger(ownerPid) ? ownerPid : process.pid,
|
||||
port: Number.isInteger(port) ? port : null,
|
||||
binary: typeof binary === 'string' ? binary : null,
|
||||
runtime: typeof runtime === 'string' ? runtime : 'web',
|
||||
startedAt: new Date().toISOString(),
|
||||
});
|
||||
};
|
||||
|
||||
/** Drop a pid from the registry (after we have killed/closed it ourselves). */
|
||||
export const unregisterManagedProcess = async (pid) => {
|
||||
if (!Number.isInteger(pid)) return;
|
||||
try {
|
||||
await fsp.rm(entryFilePath(pid), { force: true });
|
||||
} catch {
|
||||
// Best-effort: dropping a missing file is not an error.
|
||||
}
|
||||
};
|
||||
|
||||
const isPidAlive = (pid) => {
|
||||
if (!Number.isInteger(pid)) return false;
|
||||
try {
|
||||
@@ -140,38 +74,6 @@ 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 = async (pid) => {
|
||||
try {
|
||||
const { stdout } = await execFileAsync('ps', ['-p', String(pid), '-o', 'ppid=,command='], {
|
||||
encoding: 'utf8',
|
||||
timeout: 3000,
|
||||
windowsHide: true,
|
||||
});
|
||||
const line = (stdout || '').trim();
|
||||
if (!line) return null;
|
||||
const match = line.match(/^\s*(\d+)\s+(.*)$/);
|
||||
if (!match) return null;
|
||||
return { ppid: Number.parseInt(match[1], 10), command: match[2] };
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
// Windows image name for a pid (e.g. "opencode.exe"), or null.
|
||||
const readWindowsImageName = async (pid) => {
|
||||
try {
|
||||
const { stdout } = await execFileAsync('tasklist', ['/FI', `PID eq ${pid}`, '/FO', 'CSV', '/NH'], {
|
||||
encoding: 'utf8',
|
||||
timeout: 3000,
|
||||
windowsHide: true,
|
||||
});
|
||||
return (stdout || '').trim() || null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
const commandIdentifiesOurServer = (command, entry) => {
|
||||
if (typeof command !== 'string') return false;
|
||||
const lower = command.toLowerCase();
|
||||
@@ -182,105 +84,218 @@ const commandIdentifiesOurServer = (command, entry) => {
|
||||
return true;
|
||||
};
|
||||
|
||||
const killOrphan = async (pid) => {
|
||||
if (process.platform === 'win32') {
|
||||
/**
|
||||
* Build the registry API over injectable filesystem and child-process
|
||||
* dependencies. Production callers use the default instance exported below;
|
||||
* tests pass their own `fs`/`execFileAsync` instead of mocking node builtins.
|
||||
*/
|
||||
export const createManagedProcessRegistry = ({ fs = fsp, execFileAsync = defaultExecFileAsync } = {}) => {
|
||||
const writeEntryFile = async (entry) => {
|
||||
const dir = resolveRegistryDir();
|
||||
try {
|
||||
await execFileAsync('taskkill', ['/PID', String(pid), '/T', '/F'], {
|
||||
stdio: 'ignore',
|
||||
timeout: 5000,
|
||||
windowsHide: true,
|
||||
});
|
||||
await fs.mkdir(dir, { recursive: true });
|
||||
const filePath = path.join(dir, `${entry.pid}.json`);
|
||||
const tmp = `${filePath}.tmp-${process.pid}`;
|
||||
await fs.writeFile(tmp, JSON.stringify(entry, null, 2));
|
||||
await fs.rename(tmp, filePath);
|
||||
} catch {
|
||||
// Best-effort: a failed kill is not fatal (startup reaper is a backstop).
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
const signalTree = (signal) => {
|
||||
try {
|
||||
process.kill(-pid, signal);
|
||||
} catch {
|
||||
// process group may already be gone
|
||||
}
|
||||
try {
|
||||
process.kill(pid, signal);
|
||||
} catch {
|
||||
// pid may already be gone
|
||||
// Best-effort: a failed registry write must never break spawn/shutdown.
|
||||
}
|
||||
};
|
||||
|
||||
signalTree('SIGTERM');
|
||||
for (let waited = 0; waited < 1500 && isPidAlive(pid); waited += 150) {
|
||||
await sleep(150);
|
||||
}
|
||||
if (isPidAlive(pid)) {
|
||||
signalTree('SIGKILL');
|
||||
await sleep(300);
|
||||
}
|
||||
};
|
||||
|
||||
// Decide+act on a single registry entry. Returns true if it was reaped.
|
||||
const processEntry = async (entry, { log }) => {
|
||||
// Dead pid → nothing to do (caller drops the file).
|
||||
if (!isPidAlive(entry.pid)) return false;
|
||||
|
||||
const ownerGone = Number.isInteger(entry.ownerPid) && !isPidAlive(entry.ownerPid);
|
||||
|
||||
if (process.platform === 'win32') {
|
||||
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
|
||||
// AND the image still looks like opencode.
|
||||
if (looksLikeOpencode && ownerGone) {
|
||||
await killOrphan(entry.pid);
|
||||
log?.(`[lifecycle] reaped orphaned OpenCode pid ${entry.pid} (owner ${entry.ownerPid} gone)`);
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
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;
|
||||
|
||||
const orphaned = info.ppid === 1 || ownerGone;
|
||||
if (!orphaned) return false; // still owned by a live instance
|
||||
|
||||
await killOrphan(entry.pid);
|
||||
log?.(`[lifecycle] reaped orphaned OpenCode pid ${entry.pid} (reparented/owner gone)`);
|
||||
return true;
|
||||
};
|
||||
|
||||
/**
|
||||
* Kill any genuinely-orphaned OpenCode processes WE previously spawned, and
|
||||
* prune their registry files. Safe to call at startup before spawning a new
|
||||
* server. Returns { inspected, reaped }.
|
||||
*/
|
||||
export const reapOrphanedProcesses = async ({ log } = {}) => {
|
||||
const records = await readAllEntries();
|
||||
if (records.length === 0) return { inspected: 0, reaped: 0 };
|
||||
|
||||
let reaped = 0;
|
||||
for (const { entry, filePath } of records) {
|
||||
let drop = false;
|
||||
const readAllEntries = async () => {
|
||||
const dir = resolveRegistryDir();
|
||||
let names = [];
|
||||
try {
|
||||
const wasReaped = await processEntry(entry, { log });
|
||||
if (wasReaped) reaped += 1;
|
||||
// Drop the file when the process is gone (reaped now, or already dead);
|
||||
// keep it only while the process is still alive and owned by a live owner.
|
||||
drop = wasReaped || !isPidAlive(entry.pid);
|
||||
} catch (error) {
|
||||
log?.(`[lifecycle] reap check failed for pid ${entry.pid}: ${error?.message ?? error}`);
|
||||
names = await fs.readdir(dir);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
if (drop) {
|
||||
const out = [];
|
||||
for (const name of names.filter((value) => value.endsWith('.json'))) {
|
||||
const filePath = path.join(dir, name);
|
||||
try {
|
||||
await fsp.rm(filePath, { force: true });
|
||||
const entry = JSON.parse(await fs.readFile(filePath, 'utf8'));
|
||||
if (entry && Number.isInteger(entry.pid)) {
|
||||
out.push({ entry, filePath });
|
||||
} else {
|
||||
await fs.rm(filePath, { force: true });
|
||||
}
|
||||
} catch {
|
||||
// best-effort
|
||||
// Corrupt/partial file — drop it.
|
||||
try {
|
||||
await fs.rm(filePath, { force: true });
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return out;
|
||||
};
|
||||
|
||||
return { inspected: records.length, reaped };
|
||||
/** Record an OpenCode process WE spawned so a future run can reap it if orphaned. */
|
||||
const registerManagedProcess = async ({ pid, ownerPid, port, binary, runtime } = {}) => {
|
||||
if (!Number.isInteger(pid)) return;
|
||||
await writeEntryFile({
|
||||
pid,
|
||||
ownerPid: Number.isInteger(ownerPid) ? ownerPid : process.pid,
|
||||
port: Number.isInteger(port) ? port : null,
|
||||
binary: typeof binary === 'string' ? binary : null,
|
||||
runtime: typeof runtime === 'string' ? runtime : 'web',
|
||||
startedAt: new Date().toISOString(),
|
||||
});
|
||||
};
|
||||
|
||||
/** Drop a pid from the registry (after we have killed/closed it ourselves). */
|
||||
const unregisterManagedProcess = async (pid) => {
|
||||
if (!Number.isInteger(pid)) return;
|
||||
try {
|
||||
await fs.rm(entryFilePath(pid), { force: true });
|
||||
} catch {
|
||||
// Best-effort: dropping a missing file is not an error.
|
||||
}
|
||||
};
|
||||
|
||||
// Returns { ppid, command } for a live pid on Unix, or null if it can't be read.
|
||||
const readUnixProcInfo = async (pid) => {
|
||||
try {
|
||||
const { stdout } = await execFileAsync('ps', ['-p', String(pid), '-o', 'ppid=,command='], {
|
||||
encoding: 'utf8',
|
||||
timeout: 3000,
|
||||
windowsHide: true,
|
||||
});
|
||||
const line = (stdout || '').trim();
|
||||
if (!line) return null;
|
||||
const match = line.match(/^\s*(\d+)\s+(.*)$/);
|
||||
if (!match) return null;
|
||||
return { ppid: Number.parseInt(match[1], 10), command: match[2] };
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
// Windows image name for a pid (e.g. "opencode.exe"), or null.
|
||||
const readWindowsImageName = async (pid) => {
|
||||
try {
|
||||
const { stdout } = await execFileAsync('tasklist', ['/FI', `PID eq ${pid}`, '/FO', 'CSV', '/NH'], {
|
||||
encoding: 'utf8',
|
||||
timeout: 3000,
|
||||
windowsHide: true,
|
||||
});
|
||||
return (stdout || '').trim() || null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
const killOrphan = async (pid) => {
|
||||
if (process.platform === 'win32') {
|
||||
try {
|
||||
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 {
|
||||
// process group may already be gone
|
||||
}
|
||||
try {
|
||||
process.kill(pid, signal);
|
||||
} catch {
|
||||
// pid may already be gone
|
||||
}
|
||||
};
|
||||
|
||||
signalTree('SIGTERM');
|
||||
for (let waited = 0; waited < 1500 && isPidAlive(pid); waited += 150) {
|
||||
await sleep(150);
|
||||
}
|
||||
if (isPidAlive(pid)) {
|
||||
signalTree('SIGKILL');
|
||||
await sleep(300);
|
||||
}
|
||||
};
|
||||
|
||||
// Decide+act on a single registry entry. Returns true if it was reaped.
|
||||
const processEntry = async (entry, { log }) => {
|
||||
// Dead pid → nothing to do (caller drops the file).
|
||||
if (!isPidAlive(entry.pid)) return false;
|
||||
|
||||
const ownerGone = Number.isInteger(entry.ownerPid) && !isPidAlive(entry.ownerPid);
|
||||
|
||||
if (process.platform === 'win32') {
|
||||
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
|
||||
// AND the image still looks like opencode.
|
||||
if (looksLikeOpencode && ownerGone) {
|
||||
await killOrphan(entry.pid);
|
||||
log?.(`[lifecycle] reaped orphaned OpenCode pid ${entry.pid} (owner ${entry.ownerPid} gone)`);
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
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;
|
||||
|
||||
const orphaned = info.ppid === 1 || ownerGone;
|
||||
if (!orphaned) return false; // still owned by a live instance
|
||||
|
||||
await killOrphan(entry.pid);
|
||||
log?.(`[lifecycle] reaped orphaned OpenCode pid ${entry.pid} (reparented/owner gone)`);
|
||||
return true;
|
||||
};
|
||||
|
||||
/**
|
||||
* Kill any genuinely-orphaned OpenCode processes WE previously spawned, and
|
||||
* prune their registry files. Safe to call at startup before spawning a new
|
||||
* server. Returns { inspected, reaped }.
|
||||
*/
|
||||
const reapOrphanedProcesses = async ({ log } = {}) => {
|
||||
const records = await readAllEntries();
|
||||
if (records.length === 0) return { inspected: 0, reaped: 0 };
|
||||
|
||||
let reaped = 0;
|
||||
for (const { entry, filePath } of records) {
|
||||
let drop = false;
|
||||
try {
|
||||
const wasReaped = await processEntry(entry, { log });
|
||||
if (wasReaped) reaped += 1;
|
||||
// Drop the file when the process is gone (reaped now, or already dead);
|
||||
// keep it only while the process is still alive and owned by a live owner.
|
||||
drop = wasReaped || !isPidAlive(entry.pid);
|
||||
} catch (error) {
|
||||
log?.(`[lifecycle] reap check failed for pid ${entry.pid}: ${error?.message ?? error}`);
|
||||
}
|
||||
if (drop) {
|
||||
try {
|
||||
await fs.rm(filePath, { force: true });
|
||||
} catch {
|
||||
// best-effort
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return { inspected: records.length, reaped };
|
||||
};
|
||||
|
||||
return { registerManagedProcess, unregisterManagedProcess, reapOrphanedProcesses };
|
||||
};
|
||||
|
||||
const defaultRegistry = createManagedProcessRegistry();
|
||||
|
||||
export const registerManagedProcess = defaultRegistry.registerManagedProcess;
|
||||
export const unregisterManagedProcess = defaultRegistry.unregisterManagedProcess;
|
||||
export const reapOrphanedProcesses = defaultRegistry.reapOrphanedProcesses;
|
||||
|
||||
@@ -1,18 +1,9 @@
|
||||
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.
|
||||
import { createManagedProcessRegistry } from './managed-process-registry.js';
|
||||
|
||||
// The registry takes its filesystem and child-process helpers as dependencies,
|
||||
// so these tests inject fakes instead of mocking node builtins.
|
||||
const readdirMock = vi.fn();
|
||||
const readFileMock = vi.fn();
|
||||
const rmMock = vi.fn();
|
||||
@@ -20,8 +11,13 @@ const mkdirMock = vi.fn();
|
||||
const writeFileMock = vi.fn();
|
||||
const renameMock = vi.fn();
|
||||
|
||||
vi.mock('node:fs/promises', () => ({
|
||||
default: {
|
||||
// `execFileImpl` is the swappable per-test implementation, called with the same
|
||||
// (cmd, args, opts, cb) shape the callback-style `execFile` uses; the injected
|
||||
// `execFileAsync` adapts it to the `{ stdout, stderr }` promise the module awaits.
|
||||
const execFileImpl = vi.fn();
|
||||
|
||||
const { registerManagedProcess, unregisterManagedProcess, reapOrphanedProcesses } = createManagedProcessRegistry({
|
||||
fs: {
|
||||
readdir: readdirMock,
|
||||
readFile: readFileMock,
|
||||
rm: rmMock,
|
||||
@@ -29,29 +25,12 @@ vi.mock('node:fs/promises', () => ({
|
||||
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');
|
||||
execFileAsync: (cmd, args, opts) =>
|
||||
new Promise((resolve, reject) => {
|
||||
execFileImpl(cmd, args, opts, (err, stdout, stderr) =>
|
||||
err ? reject(err) : resolve({ stdout: stdout ?? '', stderr: stderr ?? '' }));
|
||||
}),
|
||||
});
|
||||
|
||||
const ORIGINAL_PLATFORM = Object.getOwnPropertyDescriptor(process, 'platform');
|
||||
const ORIGINAL_KILL = process.kill;
|
||||
|
||||
Reference in New Issue
Block a user