stop orphaning opencode turns and serves on restart
B8, both halves. They share index.ts, so they share a commit. In-flight turns: `opencode run` is spawned, not supervised, so pm2 restart officer-opencode left every turn ALIVE — reparented, still spending tokens, still writing files as the agent, with the only reader of its stdout gone. The transcript stopped mid-tool-call, which reads as the agent hanging. stopAllOpenCodeTurns kills them and settles each synchronously, because the caller is about to process.exit and nothing waiting on proc.exited would ever run. Settling writes a reason, so a reload after a restart explains itself instead of trailing off. Turns are stopped BEFORE the connection is destroyed — that write travels over it — and the flush is bounded, since losing the explanation is bad but hanging the restart is worse. Stale serves: the sweep read /proc, so it was a no-op on macOS and orphaned serves piled up, one per unclean exit, each holding a port. Added a pidfile sweep alongside it. A pid we wrote ourselves needs no cwd guard to prove it is ours, which is the part ps cannot answer portably (macOS would need lsof), and a serve started by hand is never in the file. The guard checks command AND subcommand: matching the word serve anywhere in the line would sweep a running turn whose prompt merely mentioned it. Fixtures are real ps output from both machines, not invented. Split into serve-sweep.ts because index.ts spawns a serve at module scope, so a test importing it would start one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -29,6 +29,11 @@ export type SessionLogStore = {
|
||||
push: (sessionId: string, event: ChatEvent, durable?: boolean) => void;
|
||||
/** Forget a session's buffer and cursor chain (on kill / clear-session). */
|
||||
drop: (sessionId: string) => void;
|
||||
/**
|
||||
* Settle every commit queued so far. For shutdown: `push` returns immediately and the durable write
|
||||
* happens on a queue, so exiting without this drops whatever had not reached Postgres yet.
|
||||
*/
|
||||
flush: () => Promise<void>;
|
||||
};
|
||||
|
||||
// The durable store, behind an interface so the ordering guarantee below can be tested against a writer
|
||||
@@ -117,5 +122,12 @@ export function createSessionLogStore(
|
||||
drop(sessionId) {
|
||||
logs.delete(sessionId);
|
||||
},
|
||||
|
||||
async flush() {
|
||||
// Await the tails as they stand now. A `push` racing this one is not waited for — which is the
|
||||
// honest contract: this exists for shutdown, where the alternative is exiting on an unresolved
|
||||
// write and losing the row that explains why the turn ended.
|
||||
await Promise.allSettled([...logs.values()].map((log) => log.tail));
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -1,11 +1,12 @@
|
||||
import { mkdirSync, readdirSync, readFileSync, realpathSync } from 'node:fs';
|
||||
import { mkdirSync, readdirSync, readFileSync, realpathSync, unlinkSync, writeFileSync } from 'node:fs';
|
||||
import { homedir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { DATA_PATH } from '../../data-path';
|
||||
import { createSidecarConnector } from '../connect';
|
||||
import { sweepRecordedServe } from './serve-sweep';
|
||||
import { createSessionLogStore } from '../claude/session-log';
|
||||
import type { SidecarCommand, SidecarEvent } from '../protocol';
|
||||
import { runOpenCodeTurn, killOpenCodeTurn, listRunningOpenCodeTurns } from './runner';
|
||||
import { runOpenCodeTurn, killOpenCodeTurn, listRunningOpenCodeTurns, stopAllOpenCodeTurns } from './runner';
|
||||
|
||||
// The OpenCode sidecar (officer-opencode). Same philosophy as officer-claude: a singleton process that
|
||||
// OWNS its runtime — here, an `opencode serve` — registers with the API server, and answers commands. It
|
||||
@@ -95,8 +96,11 @@ mkdirSync(SERVE_CWD, { recursive: true });
|
||||
// anchors the agent's own file operations, not merely the process cwd, and the anchor survives a
|
||||
// multi-step turn including a write (docs/opencode-phase0-review.md). Nothing replaces this.
|
||||
|
||||
// Sweep any orphaned serve from a previous unclean exit (scoped strictly to SERVE_CWD) so we never
|
||||
// end up with two serves for this directory.
|
||||
// Sweep any orphaned serve from a previous unclean exit so we never end up with two for this directory.
|
||||
// Two mechanisms, deliberately: the pidfile is portable and precise, the /proc scan is the Linux-only
|
||||
// backstop for a serve whose pidfile was lost (killed -9 mid-write, or predating the pidfile entirely).
|
||||
const SERVE_PID_FILE = join(SERVE_CWD, 'serve.pid');
|
||||
sweepRecordedServe(SERVE_PID_FILE);
|
||||
sweepStaleServes(SERVE_CWD);
|
||||
|
||||
const port = getFreePort();
|
||||
@@ -109,6 +113,14 @@ const serve = Bun.spawn([OPENCODE_BIN, 'serve', '--port', String(port), '--hostn
|
||||
stderr: 'inherit',
|
||||
});
|
||||
|
||||
// Recorded before the health check, not after: an unhealthy serve is exactly the kind that gets left
|
||||
// behind, and it still needs sweeping next time.
|
||||
try {
|
||||
writeFileSync(SERVE_PID_FILE, String(serve.pid));
|
||||
} catch (err) {
|
||||
console.error('[opencode] could not record the serve pid; sweeping will fall back to /proc', err);
|
||||
}
|
||||
|
||||
if (!(await waitHealthy(baseUrl, HEALTH_TIMEOUT_MS))) {
|
||||
console.error('[opencode] serve failed its health check');
|
||||
try {
|
||||
@@ -189,7 +201,26 @@ const sessionLog = createSessionLogStore((d) =>
|
||||
|
||||
// ── Graceful shutdown ──
|
||||
|
||||
function shutdown(signal: string) {
|
||||
// PM2 sends SIGTERM and follows with SIGKILL shortly after, so everything below is on a budget. The
|
||||
// flush is bounded rather than awaited outright: losing the explanation is bad, hanging the restart is
|
||||
// worse, and an unbounded await on a wedged Postgres would do exactly that.
|
||||
const SHUTDOWN_FLUSH_MS = 1_000;
|
||||
|
||||
let shuttingDown = false;
|
||||
|
||||
async function shutdown(signal: string) {
|
||||
if (shuttingDown) return; // SIGINT after SIGTERM must not re-enter and cut the flush short
|
||||
shuttingDown = true;
|
||||
|
||||
// Before the connection goes: killing a turn produces an event, and that event's durable write travels
|
||||
// over this socket. Tearing it down first would stop every turn silently — the exact outcome this is
|
||||
// here to prevent.
|
||||
const stopped = stopAllOpenCodeTurns(`The OpenCode sidecar restarted (${signal}), so this turn stopped.`);
|
||||
if (stopped > 0) {
|
||||
console.log(`[opencode] ${signal} received, stopping ${stopped} in-flight turn(s)...`);
|
||||
await Promise.race([sessionLog.flush(), Bun.sleep(SHUTDOWN_FLUSH_MS)]);
|
||||
}
|
||||
|
||||
console.log(`[opencode] ${signal} received, stopping serve...`);
|
||||
connection.destroy();
|
||||
try {
|
||||
@@ -197,8 +228,15 @@ function shutdown(signal: string) {
|
||||
} catch {
|
||||
/* already gone */
|
||||
}
|
||||
// We killed it ourselves, so the record has done its job. Leaving it would make the next start check a
|
||||
// pid that is either gone or, worse, reused.
|
||||
try {
|
||||
unlinkSync(SERVE_PID_FILE);
|
||||
} catch {
|
||||
/* never written, or already gone */
|
||||
}
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
process.on('SIGTERM', () => shutdown('SIGTERM'));
|
||||
process.on('SIGINT', () => shutdown('SIGINT'));
|
||||
process.on('SIGTERM', () => void shutdown('SIGTERM'));
|
||||
process.on('SIGINT', () => void shutdown('SIGINT'));
|
||||
|
||||
@@ -3,7 +3,13 @@ import { mkdtempSync, writeFileSync, chmodSync, rmSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import type { RunnerMessage } from './runner';
|
||||
import { killOpenCodeTurn, listRunningOpenCodeTurns, mapRunLine, runOpenCodeTurn } from './runner';
|
||||
import {
|
||||
killOpenCodeTurn,
|
||||
listRunningOpenCodeTurns,
|
||||
mapRunLine,
|
||||
runOpenCodeTurn,
|
||||
stopAllOpenCodeTurns,
|
||||
} from './runner';
|
||||
|
||||
// The first tests on the OpenCode path, which had none.
|
||||
//
|
||||
@@ -168,6 +174,31 @@ describe('runOpenCodeTurn — a second turn on a live session', () => {
|
||||
expect(messages.some((m) => m.type === 'opencode:event' && m.event.type === 'stopped')).toBe(true);
|
||||
});
|
||||
|
||||
it('stops every in-flight turn on shutdown, and says why in the transcript', async () => {
|
||||
// `opencode run` is spawned, not supervised, so a sidecar restart used to leave these alive:
|
||||
// reparented, still spending tokens and still writing files, with nothing reading their output.
|
||||
const messages: RunnerMessage[] = [];
|
||||
const emit = (m: RunnerMessage) => messages.push(m);
|
||||
|
||||
runOpenCodeTurn({ sessionKey: 'sess-x', prompt: 'a', cwd: stubDir }, CONFIG, emit);
|
||||
runOpenCodeTurn({ sessionKey: 'sess-y', prompt: 'b', cwd: stubDir }, CONFIG, emit);
|
||||
expect(listRunningOpenCodeTurns()).toHaveLength(2);
|
||||
|
||||
const stopped = stopAllOpenCodeTurns('sidecar restarted');
|
||||
|
||||
// Synchronous on purpose: the caller is about to call process.exit, so nothing that waits for
|
||||
// `proc.exited` would ever run. A turn killed that way just trails off mid-tool-call.
|
||||
expect(stopped).toBe(2);
|
||||
expect(listRunningOpenCodeTurns()).toEqual([]);
|
||||
const errors = messages.filter((m) => m.type === 'opencode:event' && m.event.type === 'error');
|
||||
expect(errors).toHaveLength(2);
|
||||
|
||||
// And the late exits must not add a second, worse ending on top of the one just written.
|
||||
await Bun.sleep(500);
|
||||
expect(messages.filter((m) => m.type === 'opencode:event' && m.event.type === 'error')).toHaveLength(2);
|
||||
expect(messages.some((m) => m.type === 'opencode:event' && m.event.type === 'stopped')).toBe(false);
|
||||
});
|
||||
|
||||
it('still reports a turn that dies on its own, rather than swallowing every exit', async () => {
|
||||
// The guard must not overreach: an ordinary failure is still an error the user needs to see.
|
||||
const sessionKey = 'sess-solo';
|
||||
|
||||
@@ -44,6 +44,15 @@ type RunHandle = {
|
||||
* stop button a no-op, and orphaned a process nothing could reach.
|
||||
*/
|
||||
superseded: boolean;
|
||||
/**
|
||||
* End this turn from outside the closure that owns it, with a reason.
|
||||
*
|
||||
* `killOpenCodeTurn` can kill a process and let `proc.exited` do the rest, because it has time.
|
||||
* Shutdown does not: the sidecar is about to call `process.exit`, so nothing asynchronous will ever
|
||||
* run again and a turn killed that way would simply stop mid-sentence, leaving a transcript that
|
||||
* trails off. Settling synchronously is what puts the explanation in the log before we go.
|
||||
*/
|
||||
finish: (event: ChatEvent) => void;
|
||||
};
|
||||
|
||||
// One turn per sessionKey; a new turn supersedes any stale process for that key.
|
||||
@@ -97,7 +106,9 @@ export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig,
|
||||
stderr: 'pipe',
|
||||
});
|
||||
|
||||
const handle: RunHandle = { proc, killedByUser: false, superseded: false };
|
||||
// `finish` is a placeholder for the few synchronous lines until the real one below exists — it closes
|
||||
// over `handle`, so the two cannot both be defined first. Nothing can call it in between.
|
||||
const handle: RunHandle = { proc, killedByUser: false, superseded: false, finish: () => {} };
|
||||
running.set(sessionKey, handle);
|
||||
|
||||
let done = false;
|
||||
@@ -128,6 +139,7 @@ export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig,
|
||||
};
|
||||
|
||||
const finish = (event: ChatEvent) => settle(event);
|
||||
handle.finish = finish;
|
||||
|
||||
// ── Watchdogs ──
|
||||
const hardTimer = setTimeout(() => {
|
||||
@@ -253,6 +265,35 @@ export function listRunningOpenCodeTurns(): { sessionKey: string }[] {
|
||||
return Array.from(running.keys()).map((sessionKey) => ({ sessionKey }));
|
||||
}
|
||||
|
||||
/**
|
||||
* Kill every turn this process is running, because the process itself is going away.
|
||||
*
|
||||
* A turn is a child of this sidecar only in the bookkeeping sense: `opencode run` is spawned, not
|
||||
* supervised, so `pm2 restart officer-opencode` used to leave every in-flight turn ALIVE — reparented,
|
||||
* still spending tokens, and still writing files as the agent, while the only reader of its stdout had
|
||||
* exited. The turn's output went nowhere and the transcript simply stopped mid-tool-call, which is
|
||||
* indistinguishable from the agent hanging.
|
||||
*
|
||||
* Both halves matter. Killing the children stops the invisible work; settling them synchronously writes
|
||||
* a reason into the transcript, so a reload after a restart explains itself instead of trailing off.
|
||||
* Returns how many were stopped, so the caller can skip the flush wait when there were none.
|
||||
*/
|
||||
export function stopAllOpenCodeTurns(message: string): number {
|
||||
const handles = [...running.values()];
|
||||
for (const handle of handles) {
|
||||
// Suppress the exit handler's own error: this death is accounted for, and `finish` below is the
|
||||
// account. Without it a late `proc.exited` would be a second, less accurate ending.
|
||||
handle.killedByUser = true;
|
||||
try {
|
||||
handle.proc.kill();
|
||||
} catch {
|
||||
/* already gone */
|
||||
}
|
||||
handle.finish({ type: 'error', message });
|
||||
}
|
||||
return handles.length;
|
||||
}
|
||||
|
||||
export function killOpenCodeTurn(sessionKey: string): void {
|
||||
const handle = running.get(sessionKey);
|
||||
if (!handle) return;
|
||||
|
||||
@@ -0,0 +1,44 @@
|
||||
import { describe, expect, it } from 'bun:test';
|
||||
import { looksLikeOpenCodeServe } from './serve-sweep';
|
||||
|
||||
// The guard on the pidfile sweep, which is the portable replacement for a `/proc` scan that was a no-op
|
||||
// on macOS — so orphaned serves accumulated there, one per unclean exit, each holding a port.
|
||||
//
|
||||
// The strings below are REAL `ps -p <pid> -o command=` output, captured from the live serve on both
|
||||
// machines rather than invented: macOS 26 and alpha (Linux). Both print the absolute binary path with no
|
||||
// leading whitespace, which is the thing worth pinning — a format change is what would silently turn the
|
||||
// sweep back into a no-op.
|
||||
|
||||
const MACOS = '/Users/pastilhas/.opencode/bin/opencode serve --port 55565 --hostname 127.0.0.1';
|
||||
const LINUX = '/home/pastilhas/.opencode/bin/opencode serve --port 33637 --hostname 127.0.0.1';
|
||||
|
||||
describe('looksLikeOpenCodeServe', () => {
|
||||
it('recognises the real serve command line on both platforms', () => {
|
||||
expect(looksLikeOpenCodeServe(MACOS)).toBe(true);
|
||||
expect(looksLikeOpenCodeServe(LINUX)).toBe(true);
|
||||
expect(looksLikeOpenCodeServe(`${LINUX}\n`)).toBe(true); // ps leaves the trailing newline on
|
||||
});
|
||||
|
||||
it('refuses a pid the OS handed to something else entirely', () => {
|
||||
// The whole reason the check exists: the recorded process died and its number was reused, so
|
||||
// SIGTERM would hit a bystander.
|
||||
expect(looksLikeOpenCodeServe('/Applications/Xcode.app/Contents/MacOS/Xcode')).toBe(false);
|
||||
expect(looksLikeOpenCodeServe('')).toBe(false);
|
||||
expect(looksLikeOpenCodeServe(' ')).toBe(false);
|
||||
});
|
||||
|
||||
it('never matches an `opencode run`, whatever the prompt says', () => {
|
||||
// A turn is not a serve, and this is the sharp edge: matching the word `serve` anywhere in the line
|
||||
// would sweep a running turn whose PROMPT merely mentioned it. Killing a live turn to tidy up a
|
||||
// stale server is the worst outcome available here.
|
||||
expect(looksLikeOpenCodeServe('/home/p/.opencode/bin/opencode run --format json "how do I serve this"')).toBe(
|
||||
false,
|
||||
);
|
||||
expect(looksLikeOpenCodeServe('/home/p/.opencode/bin/opencode run --format json serve')).toBe(false);
|
||||
});
|
||||
|
||||
it('is not fooled by a path that merely contains the word', () => {
|
||||
expect(looksLikeOpenCodeServe('/opt/opencode/server/bin/helper --port 1')).toBe(false);
|
||||
expect(looksLikeOpenCodeServe('/usr/bin/serve --port 1')).toBe(false); // no `opencode` in the command
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,61 @@
|
||||
import { readFileSync, unlinkSync } from 'node:fs';
|
||||
|
||||
// Killing an `opencode serve` that outlived the sidecar that started it.
|
||||
//
|
||||
// Its own file rather than part of `index.ts` for one practical reason: `index.ts` spawns a serve and
|
||||
// dials the API at module scope, so importing it from a test would start a real one. This has no side
|
||||
// effects until called.
|
||||
|
||||
/**
|
||||
* Does this pid still look like the serve we started?
|
||||
*
|
||||
* The pidfile's one real hazard is pid REUSE: the recorded process died, the OS handed its number to
|
||||
* something unrelated, and we would then SIGTERM an innocent bystander — possibly the user's editor.
|
||||
* Checking the command line first makes that impossible, and costs one `ps`.
|
||||
*
|
||||
* Separated from the `ps` call so it can be tested against real output rather than an invented format.
|
||||
*/
|
||||
export function looksLikeOpenCodeServe(psOutput: string): boolean {
|
||||
// Command and subcommand, not "contains the word serve". Both platforms print
|
||||
// `/abs/path/to/opencode serve --port N --hostname 127.0.0.1` with no leading whitespace (verified on
|
||||
// macOS 26 and on alpha). Searching the whole line for `serve` would match an `opencode run` whose
|
||||
// PROMPT happened to contain the word — and a run is precisely what must not be swept.
|
||||
const [command, subcommand] = psOutput.trim().split(/\s+/);
|
||||
return !!command && command.includes('opencode') && subcommand === 'serve';
|
||||
}
|
||||
|
||||
/**
|
||||
* Kill the serve this sidecar started last time, if it outlived us.
|
||||
*
|
||||
* The `/proc` sweep in `index.ts` is Linux-only and therefore a no-op on the Mac, where orphaned serves
|
||||
* simply accumulated — one per unclean exit, each holding a port. This is the portable half, and it is
|
||||
* also the more precise one: a pid we wrote down ourselves needs no cwd guard to prove it is ours, which
|
||||
* is the part `ps` cannot answer portably (macOS would need `lsof`). An `opencode serve` the user
|
||||
* started by hand is never in this file and so is never touched.
|
||||
*/
|
||||
export function sweepRecordedServe(pidFile: string): void {
|
||||
let recorded: number;
|
||||
try {
|
||||
recorded = Number(readFileSync(pidFile, 'utf8').trim());
|
||||
} catch {
|
||||
return; // no pidfile — first run, or a clean exit removed it
|
||||
}
|
||||
if (!Number.isInteger(recorded) || recorded <= 0) return;
|
||||
|
||||
try {
|
||||
const ps = Bun.spawnSync(['ps', '-p', String(recorded), '-o', 'command=']);
|
||||
if (!ps.success) return; // pid is gone; nothing to sweep
|
||||
if (!looksLikeOpenCodeServe(ps.stdout.toString())) return; // pid was reused — not ours to kill
|
||||
|
||||
process.kill(recorded, 'SIGTERM');
|
||||
console.log(`[opencode] swept recorded serve pid ${recorded}`);
|
||||
} catch {
|
||||
/* vanished mid-check, or no permission — leave it to the /proc sweep on Linux */
|
||||
} finally {
|
||||
try {
|
||||
unlinkSync(pidFile);
|
||||
} catch {
|
||||
/* already gone */
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user