diff --git a/src/servers/sidecar/claude/session-log.ts b/src/servers/sidecar/claude/session-log.ts index 1d7370ed..3337f3dc 100644 --- a/src/servers/sidecar/claude/session-log.ts +++ b/src/servers/sidecar/claude/session-log.ts @@ -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; }; // 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)); + }, }; } diff --git a/src/servers/sidecar/opencode/index.ts b/src/servers/sidecar/opencode/index.ts index e0437f9d..bb2aee96 100644 --- a/src/servers/sidecar/opencode/index.ts +++ b/src/servers/sidecar/opencode/index.ts @@ -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')); diff --git a/src/servers/sidecar/opencode/runner.test.ts b/src/servers/sidecar/opencode/runner.test.ts index d9045566..316917f5 100644 --- a/src/servers/sidecar/opencode/runner.test.ts +++ b/src/servers/sidecar/opencode/runner.test.ts @@ -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'; diff --git a/src/servers/sidecar/opencode/runner.ts b/src/servers/sidecar/opencode/runner.ts index 1ec9a881..ac5b0007 100644 --- a/src/servers/sidecar/opencode/runner.ts +++ b/src/servers/sidecar/opencode/runner.ts @@ -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; diff --git a/src/servers/sidecar/opencode/serve-sweep.test.ts b/src/servers/sidecar/opencode/serve-sweep.test.ts new file mode 100644 index 00000000..81c91a95 --- /dev/null +++ b/src/servers/sidecar/opencode/serve-sweep.test.ts @@ -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 -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 + }); +}); diff --git a/src/servers/sidecar/opencode/serve-sweep.ts b/src/servers/sidecar/opencode/serve-sweep.ts new file mode 100644 index 00000000..e55b399e --- /dev/null +++ b/src/servers/sidecar/opencode/serve-sweep.ts @@ -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 */ + } + } +}