isolate eval runtimes from terminal

This commit is contained in:
Colin Mason
2026-07-12 20:29:38 -04:00
parent edd959a381
commit c40ccdc684
13 changed files with 211 additions and 19 deletions
+4
View File
@@ -25,6 +25,10 @@
- Updated status event log to prioritize the most recent entries in the display window
- Updated the snapcompact shape preview transcript to use the compact scope format shown to models during compaction.
### Fixed
- Fixed eval cells launching interactive subprocesses in OMP's terminal session, which could steal the foreground process group and suspend the conversation with `zsh: suspended (tty input)`
### Removed
- Removed the `--downshift-boomerang` feature and its associated configuration setting
+6
View File
@@ -108,6 +108,7 @@ const TINY_WORKER_ARG = "__omp_worker_tiny_inference";
const STATS_SYNC_WORKER_ARG = "__omp_worker_stats_sync";
const TAB_WORKER_ARG = "__omp_worker_tab";
const JS_EVAL_WORKER_ARG = "__omp_worker_js_eval";
const JS_EVAL_PROCESS_ARG = "__omp_worker_js_eval_process";
const STT_WORKER_ARG = "__omp_worker_stt";
const TTS_WORKER_ARG = "__omp_worker_tts";
const MNEMOPI_EMBED_WORKER_ARG = "__omp_worker_mnemopi_embed";
@@ -156,6 +157,11 @@ async function runWorkerEntrypoint(arg: string | undefined): Promise<boolean> {
await import("./eval/js/worker-entry");
return true;
}
if (arg === JS_EVAL_PROCESS_ARG) {
const { startJsEvalProcess } = await import("./eval/js/process-entry");
await runIpcSubprocessWorker(startJsEvalProcess);
return true;
}
if (arg === STT_WORKER_ARG) {
const { startSttWorker } = await import("./stt/asr-worker");
await runIpcSubprocessWorker(startSttWorker);
@@ -2,7 +2,11 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test";
import { TempDir } from "@oh-my-pi/pi-utils";
import { Settings } from "../../config/settings";
import type { ToolSession } from "../../tools";
import { disposeAllVmContexts, setWorkerCloseTimeoutMsForTests } from "../js/context-manager";
import {
disposeAllVmContexts,
setJsEvalWorkerThreadForTests,
setWorkerCloseTimeoutMsForTests,
} from "../js/context-manager";
import { executeJs } from "../js/executor";
const originalWorker = globalThis.Worker;
@@ -181,7 +185,9 @@ function installFakeWorker(stats: FakeWorkerStats, behavior: FakeWorkerBehavior)
describe("JavaScript eval worker lifecycle", () => {
let restoreCloseTimeoutMs = 0;
let restoreWorkerThread = false;
beforeEach(() => {
restoreWorkerThread = setJsEvalWorkerThreadForTests(true);
// Shrink the graceful-close grace period so the "close acked but the worker
// never exits -> force terminate" contract is proven without a real 1s wait.
restoreCloseTimeoutMs = setWorkerCloseTimeoutMsForTests(1);
@@ -197,6 +203,7 @@ describe("JavaScript eval worker lifecycle", () => {
writable: true,
value: originalWorker,
});
setJsEvalWorkerThreadForTests(restoreWorkerThread);
});
it("exits a real worker on graceful close even with ref'ed user handles", async () => {
@@ -289,3 +296,37 @@ describe("JavaScript eval worker lifecycle", () => {
expect(stats.terminateCalls).toBe(1);
});
});
describe.skipIf(process.platform === "win32")("JavaScript eval process isolation", () => {
afterEach(async () => {
await disposeAllVmContexts();
});
it("runs spawned commands under an isolated POSIX session", async () => {
using tempDir = TempDir.createSync("@omp-js-process-isolation-");
const session = makeSession(tempDir.path());
const evalSessionId = `js-isolation:${crypto.randomUUID()}`;
const result = await executeJs(
[
`const child = Bun.spawn(["/bin/sh", "-c", 'sid=$(ps -o sid= -p $$); printf "%s %s\\n" "$sid" "$PPID"'], { stdout: "pipe" });`,
"return await new Response(child.stdout).text();",
].join("\n"),
{ cwd: tempDir.path(), sessionId: evalSessionId, session },
);
const [sessionId, parentProcessId] = result.output.trim().split(/\s+/).map(Number);
expect(parentProcessId).not.toBe(process.pid);
expect(sessionId).toBe(parentProcessId);
await executeJs("var saved = 41; function increment(value) { return value + 1; }", {
cwd: tempDir.path(),
sessionId: evalSessionId,
session,
});
const reused = await executeJs("return increment(saved);", {
cwd: tempDir.path(),
sessionId: evalSessionId,
session,
});
expect(reused.output.trim()).toBe("42");
});
});
@@ -3,9 +3,21 @@ import {
__resetWindowsConsoleProbeCache,
consoleAttachedViaTTY,
hostHasInheritableConsole,
shouldDetachKernel,
shouldHideKernelWindow,
} from "../py/spawn-options";
describe("shouldDetachKernel", () => {
it("starts POSIX kernels in a new session", () => {
expect(shouldDetachKernel("darwin")).toBe(true);
expect(shouldDetachKernel("linux")).toBe(true);
});
it("leaves Windows console inheritance to windowsHide", () => {
expect(shouldDetachKernel("win32")).toBe(false);
});
});
/**
* `shouldHideKernelWindow` decides whether the long-lived Python kernel
* subprocess is spawned with `windowsHide: true`. On Windows, Bun maps that
+2 -1
View File
@@ -13,7 +13,7 @@ import { $ } from "bun";
import { Settings } from "../../config/settings";
import { BaseKernel, getRemainingTimeMs, type KernelStartOptions } from "../kernel-base";
import type { KernelDisplayOutput } from "../py/display";
import { hostHasInheritableConsole, shouldHideKernelWindow } from "../py/spawn-options";
import { hostHasInheritableConsole, shouldDetachKernel, shouldHideKernelWindow } from "../py/spawn-options";
import { JULIA_PRELUDE } from "./prelude";
import RUNNER_SCRIPT from "./runner.jl" with { type: "text" };
import {
@@ -187,6 +187,7 @@ export class JuliaKernel extends BaseKernel<KernelExecuteOptions> {
[runtime.juliaPath, "--startup-file=no", "--history-file=no", "--color=no", "--project=@.", scriptPath],
{
cwd: options.cwd,
detached: shouldDetachKernel(process.platform),
env: spawnEnv,
stdin: "pipe",
stdout: "pipe",
@@ -1,6 +1,14 @@
import { logger, Snowflake, workerHostEntry } from "@oh-my-pi/pi-utils";
import {
createWorkerHandle,
createWorkerSubprocess,
resolveWorkerSpawnCmd,
workerEnvFromParent,
} from "../../subprocess/worker-client";
import type { ToolSession } from "../../tools";
import { ToolAbortError, ToolError } from "../../tools/tool-errors";
import { safeSend as safeSendIpc } from "../../utils/ipc";
import { shouldDetachKernel } from "../py/spawn-options";
import { callSessionTool, type JsStatusEvent } from "./tool-bridge";
import { WorkerCore } from "./worker-core";
// Coding-agent binary/bundle workers route through the CLI entrypoint with a
@@ -24,7 +32,7 @@ export interface VmRunState {
}
interface WorkerHandle {
mode: "worker" | "inline";
mode: "process" | "worker" | "inline";
send(msg: WorkerInbound): void;
onMessage(handler: (msg: WorkerOutbound) => void): () => void;
onError(handler: (error: Error) => void): () => void;
@@ -57,16 +65,18 @@ const resettingSessions = new Map<string, Promise<void>>();
// Worker startup (module-graph import + WorkerCore construction) is infrastructure
// cost, not user compute. Floor it independently of Bun's 5s default per-test timeout
// so a slow cold-start under load isn't aborted mid-init — terminating a still-
// initializing Bun worker triggers the same kind of terminate-race that motivates
// initializing eval runtime triggers the same kind of terminate-race that motivates
// avoiding `vm.runInContext` (see shared/indirect-eval.ts), here surfacing as a
// SIGILL/SIGSEGV. Callers that pass a larger per-cell budget still dominate.
const WORKER_INIT_TIMEOUT_MS = 15_000;
const WORKER_CLOSE_TIMEOUT_MS = 1_000;
const JS_EVAL_PROCESS_ARG = "__omp_worker_js_eval_process";
// Active graceful-close grace period before a worker that ack'd `close` but never
// emitted its `close` event is force-terminated. Defaults to the production floor;
// tests override it (and restore it) to exercise the close-timeout -> terminate
// path without a real wall-clock wait.
let workerCloseTimeoutMs: number = WORKER_CLOSE_TIMEOUT_MS;
let useWorkerThreadForTests = false;
/**
* Test-only seam: override the graceful-close grace period (ms). Returns the
@@ -79,6 +89,13 @@ export function setWorkerCloseTimeoutMsForTests(ms: number): number {
return previous;
}
/** Test-only seam for the legacy Worker lifecycle mocks. */
export function setJsEvalWorkerThreadForTests(enabled: boolean): boolean {
const previous = useWorkerThreadForTests;
useWorkerThreadForTests = enabled;
return previous;
}
export async function executeInVmContext(options: {
sessionKey: string;
sessionId: string;
@@ -144,9 +161,9 @@ export async function disposeAllVmContexts(): Promise<void> {
}
/**
* Smoke probe: spawn the JS eval worker through the worker-host entry and prove
* it answers the `init` handshake on a real worker thread (not the inline
* fallback). Catches the silent worker-load and init-message-drop regressions
* Smoke probe: spawn the JS evaluator through the worker-host entry and prove
* it answers the `init` handshake in a real isolated subprocess (not the inline
* fallback). Catches silent process-load and init-message regressions
* that otherwise strand every cell on the init timeout in a distribution build —
* the failure mode that motivated `installWorkerInbox`. Wired into
* `omp --smoke-test` so binary / source / tarball installs all exercise it.
@@ -163,8 +180,8 @@ export async function smokeTestJsEvalWorker(): Promise<void> {
};
try {
await initWorker(session, { cwd: process.cwd(), sessionId: "smoke" }, WORKER_INIT_TIMEOUT_MS);
if (worker.mode !== "worker") {
throw new Error("JS eval worker smoke fell back to the inline worker (real worker failed to start)");
if (worker.mode !== "process") {
throw new Error("JS eval worker smoke fell back from the isolated subprocess");
}
} finally {
await worker.terminate().catch(() => undefined);
@@ -237,10 +254,8 @@ async function acquireSession(sessionKey: string, snapshot: SessionSnapshot, tim
if (starting) return await starting;
const startup = (async (): Promise<JsSession> => {
// The message listener must be attached synchronously after `new Worker`:
// Bun drops messages posted before a listener exists, and WorkerCore emits
// `ready` from its constructor on load. `spawnJsWorker` + `initWorker` run with
// no intervening await, so `ready` can never race the attach.
// Attach the message listener before sending init. Both Bun Worker messages
// and subprocess IPC can arrive immediately after the evaluator loads.
const worker = spawnJsWorker();
const session: JsSession = {
sessionKey,
@@ -256,8 +271,8 @@ async function acquireSession(sessionKey: string, snapshot: SessionSnapshot, tim
try {
await initWorker(session, snapshot, readyTimeoutMs);
} catch (error) {
// Worker-thread crash/load failures surface asynchronously via the worker
// `error` event — after `spawnJsWorker`'s synchronous try/catch already
// Runtime crash/load failures surface asynchronously via the worker error
// callback — after `spawnJsWorker`'s synchronous try/catch already
// returned — so the only signal is the rejected handshake. Retry on the
// inline worker so a broken module graph fails fast instead of stalling
// every cell on the init timeout and then dying with exitCode 1.
@@ -480,6 +495,16 @@ async function raceWithTimeout<T>(promise: Promise<T>, timeoutMs: number, reason
}
function spawnJsWorker(): WorkerHandle {
if (!useWorkerThreadForTests) {
try {
return spawnJsProcess();
} catch (err) {
logger.warn("JS eval subprocess spawn failed; using inline JS eval worker (no sync-loop guard)", {
error: err instanceof Error ? err.message : String(err),
});
return spawnInlineWorker();
}
}
try {
const hostEntry = workerHostEntry();
const worker = hostEntry
@@ -494,6 +519,47 @@ function spawnJsWorker(): WorkerHandle {
}
}
function spawnJsProcess(): WorkerHandle {
const spawned = createWorkerSubprocess<WorkerOutbound>({
spawnCommand: resolveWorkerSpawnCmd(JS_EVAL_PROCESS_ARG),
env: workerEnvFromParent(),
exitLabel: "JS eval worker",
detached: shouldDetachKernel(process.platform),
reportCleanExit: true,
unref: false,
});
const base = createWorkerHandle<WorkerInbound, WorkerOutbound>(spawned, message =>
safeSendIpc(spawned.proc, message, "js-eval"),
);
return {
mode: "process",
send: message => base.send(message),
onMessage: handler => base.onMessage(handler),
onError: handler => base.onError(handler),
async close() {
const { promise, resolve } = Promise.withResolvers<boolean>();
let settled = false;
let timeout: NodeJS.Timeout | undefined;
let unsubscribe = (): void => {};
const finish = (value: boolean): void => {
if (settled) return;
settled = true;
if (timeout) clearTimeout(timeout);
unsubscribe();
resolve(value);
};
unsubscribe = base.onMessage(message => {
if (message.type !== "closed") return;
void base.terminate().finally(() => finish(true));
});
timeout = setTimeout(() => finish(false), workerCloseTimeoutMs);
base.send({ type: "close" });
return await promise;
},
terminate: () => base.terminate(),
};
}
function wrapBunWorker(worker: Worker): WorkerHandle {
return {
mode: "worker",
@@ -0,0 +1,16 @@
import { WorkerCore } from "./worker-core";
import type { WorkerInbound, WorkerOutbound } from "./worker-protocol";
/** Start the JavaScript evaluator inside a subprocess IPC transport. */
export function startJsEvalProcess(transport: {
send(message: WorkerOutbound): void;
onMessage(handler: (message: WorkerInbound) => void): () => void;
}): void {
new WorkerCore({
send: message => transport.send(message),
onMessage: handler => transport.onMessage(handler),
// The parent owns process lifetime and kills the subprocess after the
// WorkerCore `closed` acknowledgement has crossed IPC.
close: () => {},
});
}
+2 -1
View File
@@ -23,7 +23,7 @@ import {
resolveExplicitPythonRuntime,
resolvePythonRuntime,
} from "./runtime";
import { hostHasInheritableConsole, shouldHideKernelWindow } from "./spawn-options";
import { hostHasInheritableConsole, shouldDetachKernel, shouldHideKernelWindow } from "./spawn-options";
export type {
KernelExecuteOptions,
@@ -193,6 +193,7 @@ export class PythonKernel extends BaseKernel {
const proc = Bun.spawn([runtime.pythonPath, "-u", scriptPath], {
cwd: options.cwd,
detached: shouldDetachKernel(process.platform),
env: spawnEnv,
stdin: "pipe",
stdout: "pipe",
@@ -40,6 +40,19 @@ export function shouldHideKernelWindow(opts: {
return !opts.hostHasInheritableConsole;
}
/**
* Keep eval kernels outside the host's POSIX terminal session.
*
* User code can start an interactive shell which calls `tcsetpgrp(3)`. If the
* kernel shares OMP's session, that shell can replace OMP as the controlling
* terminal's foreground process group and the host is then stopped by SIGTTIN
* on its next stdin read. Bun implements `detached: true` with `setsid(2)` on
* POSIX, making the kernel a session leader with no controlling terminal.
*/
export function shouldDetachKernel(platform: NodeJS.Platform): boolean {
return platform !== "win32";
}
/**
* TTY-based fallback used when the Win32 console probe is unavailable.
*
+2 -1
View File
@@ -16,7 +16,7 @@ import { $ } from "bun";
import { Settings } from "../../config/settings";
import { BaseKernel, getRemainingTimeMs, type KernelRuntimeEnv, type KernelStartOptions } from "../kernel-base";
import type { KernelDisplayOutput } from "../py/display";
import { hostHasInheritableConsole, shouldHideKernelWindow } from "../py/spawn-options";
import { hostHasInheritableConsole, shouldDetachKernel, shouldHideKernelWindow } from "../py/spawn-options";
import { RUBY_PRELUDE } from "./prelude";
import RUNNER_SCRIPT from "./runner.rb" with { type: "text" };
import {
@@ -186,6 +186,7 @@ export class RubyKernel extends BaseKernel<KernelExecuteOptions> {
const proc = Bun.spawn([runtime.rubyPath, scriptPath], {
cwd: options.cwd,
detached: shouldDetachKernel(process.platform),
env: spawnEnv,
stdin: "pipe",
stdout: "pipe",
@@ -159,6 +159,12 @@ export function createWorkerSubprocess<Outbound>(options: {
spawnCommand: WorkerSpawnCommand;
env: Record<string, string>;
exitLabel: string;
/** Start the child as a new process-group/session leader where Bun supports it. */
detached?: boolean;
/** Treat exit code 0 as unexpected; eval cells can call process.exit(0). */
reportCleanExit?: boolean;
/** Whether an idle worker should stop keeping the parent event loop alive. */
unref?: boolean;
}): SpawnedSubprocess<Outbound> {
const inbound = new Set<(message: Outbound) => void>();
const errors = new Set<(error: Error) => void>();
@@ -175,6 +181,7 @@ export function createWorkerSubprocess<Outbound>(options: {
const proc = Bun.spawn({
cmd: options.spawnCommand.cmd,
cwd: options.spawnCommand.cwd,
detached: options.detached,
env: options.env,
stdin: "ignore",
stdout: "ignore",
@@ -186,7 +193,7 @@ export function createWorkerSubprocess<Outbound>(options: {
},
onExit(_proc, exitCode, signalCode) {
startStderrDrain();
if (exitCode === 0) return;
if (exitCode === 0 && !options.reportCleanExit) return;
// Swallow only the expected SIGKILL from `terminate()`; every other
// signal exit (SIGSEGV from a native fault, OOM SIGKILL, operator
// `kill -9`) is a real worker death that must fault in-flight
@@ -206,7 +213,7 @@ export function createWorkerSubprocess<Outbound>(options: {
// Don't keep the parent event loop alive on an idle worker; the dispose
// path calls `terminate()` explicitly. Bun's test runner starves IPC for
// unref'd subprocesses, so keep it referenced only under tests.
if (!isBunTestRuntime()) proc.unref();
if (!isBunTestRuntime() && options.unref !== false) proc.unref();
return { proc, inbound, errors, intentionalExit, stderrDrained: stderrDrained.promise };
}
@@ -67,6 +67,18 @@ describe.skipIf(!SHOULD_RUN)("python runner subprocess", () => {
}
});
it.skipIf(process.platform === "win32")("runs in its own POSIX session", async () => {
using tempDir = TempDir.createSync("@python-runner-session-isolation-");
const kernel = await PythonKernel.start({ cwd: tempDir.path() });
try {
const result = await executePythonWithKernel(kernel, "import os; print(os.getsid(0), os.getpid())");
const [sessionId, processId] = result.output.trim().split(/\s+/).map(Number);
expect(sessionId).toBe(processId);
} finally {
await kernel.shutdown();
}
});
it("cancels a long sleep via SIGINT within 500ms", async () => {
using tempDir = TempDir.createSync("@python-runner-cancel-");
const kernel = await PythonKernel.start({ cwd: tempDir.path() });
@@ -34,6 +34,18 @@ describe.skipIf(!SHOULD_RUN)("ruby runner subprocess", () => {
}
});
it.skipIf(process.platform === "win32")("runs in its own POSIX session", async () => {
using tempDir = TempDir.createSync("@ruby-runner-session-isolation-");
const kernel = await RubyKernel.start({ cwd: tempDir.path() });
try {
const result = await executeRubyWithKernel(kernel, 'puts "#{Process.getsid(0)} #{Process.pid}"', {});
const [sessionId, processId] = result.output.trim().split(/\s+/).map(Number);
expect(sessionId).toBe(processId);
} finally {
await kernel.shutdown();
}
});
it("keeps local variables across cells on one kernel", async () => {
using tempDir = TempDir.createSync("@ruby-runner-state-");
const kernel = await RubyKernel.start({ cwd: tempDir.path() });