diff --git a/.changeset/quick-services-restart.md b/.changeset/quick-services-restart.md new file mode 100644 index 0000000000..39aaeb359f --- /dev/null +++ b/.changeset/quick-services-restart.md @@ -0,0 +1,6 @@ +--- +"@opencode-ai/client": patch +"@opencode-ai/cli": patch +--- + +Recover explicitly restarted background services that still own the service lock but no longer answer health checks. diff --git a/docs/design/service-lifecycle.md b/docs/design/service-lifecycle.md index d4506521a9..dee854fad6 100644 --- a/docs/design/service-lifecycle.md +++ b/docs/design/service-lifecycle.md @@ -42,6 +42,7 @@ framework. | Lifecycle shell | Implemented; the owner binds and registers before application boot | | Failed-state latching | Implemented; deterministic boot failure stays bound and actionable | | Recovery diagnostics | Implemented; the TUI shows status instead of transport internals | +| Explicit recovery | Implemented; restart can replace an unchanged unresponsive owner | | Cross-platform validation | macOS runtime verified; Linux and Windows run in the unit-test matrix | ## Context @@ -453,13 +454,14 @@ After a bounded diagnostic threshold, the client may show: ```text The background service owns the service lock but is not responding. -Run `opencode service restart` to recover it. +[r] Restart service ``` -Only explicit `service restart` may perform destructive recovery. It verifies -the complete registration and process instance before signaling, waits for -graceful exit, re-checks identity before escalation, and refuses to kill a -process it cannot positively identify. +Only explicit `service restart` or the TUI restart action may perform +destructive recovery. It verifies that the complete registration is unchanged +before signaling, waits for graceful exit, and re-checks registration before +escalation. This deliberately accepts the narrow risk that a stale PID was +reused by an unrelated process; automatic reconnect never takes that risk. Automatic frozen-owner recovery is deferred. @@ -504,8 +506,11 @@ Automatic frozen-owner recovery is deferred. 1. Health fails, but the process still holds the service lock. 2. Contenders fail lock acquisition and exit. -3. Clients wait and eventually show explicit recovery guidance. -4. No TUI kills the owner automatically. +3. Clients wait and the TUI offers explicit restart. +4. The user presses `r`, or runs `opencode service restart`. +5. Restart rechecks the complete registration before each signal. +6. Once the process exits, normal election starts one replacement. +7. No TUI kills the owner automatically. ## TDD Verification @@ -609,6 +614,8 @@ was the observed incident cost. - Registration corruption cannot produce two owners. - A deleted registration heals without restarting the owner or any client. - An unresponsive owner is not killed without an explicit recovery command. +- Explicit restart replaces an unchanged unresponsive owner and returns only + when the replacement is ready. - Raw transport defects never escape to the terminal. ## Follow-ups @@ -617,7 +624,8 @@ was the observed incident cost. - Application protocol compatibility and automatic local TUI re-exec. - Durable execution recovery for provider attempts and tools. - Shell, sub-agent, permission, question, and background-job continuity. -- Automatic recovery for a positively identified frozen owner. +- Automatic recovery for a positively identified frozen owner without the + explicit-restart PID-reuse tradeoff. - Cold-boot concurrency limits and interaction-prioritized location loading. - A steward or socket-handoff architecture if zero-downtime replacement becomes a real requirement. diff --git a/packages/cli/src/commands/handlers/service/restart.ts b/packages/cli/src/commands/handlers/service/restart.ts index 7dc3e02c8f..36098657dc 100644 --- a/packages/cli/src/commands/handlers/service/restart.ts +++ b/packages/cli/src/commands/handlers/service/restart.ts @@ -9,8 +9,7 @@ export default Runtime.handler( Commands.commands.service.commands.restart, Effect.fn("cli.service.restart")(function* () { const options = yield* ServiceConfig.options() - yield* Service.stop(options, { targetVersion: options.version }) - const transport = yield* Service.start(options) + const transport = yield* Service.restart(options) process.stdout.write(transport.url + EOL) }), ) diff --git a/packages/cli/src/services/server.ts b/packages/cli/src/services/server.ts index b1cdce4cbb..27edbe1b5f 100644 --- a/packages/cli/src/services/server.ts +++ b/packages/cli/src/services/server.ts @@ -17,7 +17,7 @@ export type Args = { export type Resolved = { readonly endpoint: Service.Endpoint readonly reconnect?: (onStatus: (status: Service.Status) => void, signal: AbortSignal) => Promise - readonly reload?: () => Promise + readonly reload?: (signal?: AbortSignal) => Promise } export const resolve = Effect.fn("cli.server.resolve")(function* (args: Args) { @@ -53,13 +53,8 @@ export const resolve = Effect.fn("cli.server.resolve")(function* (args: Args) { Effect.runPromise(Service.start({ ...reconnectOptions, onStatus }).pipe(Effect.provide(NodeFileSystem.layer)), { signal, }), - reload: () => - Effect.runPromise( - Effect.gen(function* () { - yield* Service.stop(options, { targetVersion: options.version }) - yield* Service.start(options) - }).pipe(Effect.provide(NodeFileSystem.layer)), - ), + reload: (signal?: AbortSignal) => + Effect.runPromise(Service.restart(options).pipe(Effect.asVoid, Effect.provide(NodeFileSystem.layer)), { signal }), } satisfies Resolved }) diff --git a/packages/client/src/effect/service.ts b/packages/client/src/effect/service.ts index 86cbae6403..b079c0dd5c 100644 --- a/packages/client/src/effect/service.ts +++ b/packages/client/src/effect/service.ts @@ -199,6 +199,22 @@ export const stop = Effect.fn("service.stop")(function* (options: Options = {}, if (existing !== undefined) yield* kill(existing, options, metadata.targetVersion) }) +// Explicit recovery path: unlike stop(), restart may terminate an unchanged +// registered process that no longer answers authenticated health checks. +export const restart = Effect.fn("service.restart")(function* (options: StartOptions = {}) { + const existing = yield* registered(options.file, true) + if (existing.service !== undefined) { + const result = yield* kill(existing.service, options, options.version) + if (result === "rejected") return yield* Effect.fail(new Error("Background service rejected restart")) + if (result === "changed") { + const current = yield* read(options.file) + if (current !== undefined && same(current, existing.info)) + yield* terminate(existing.info, read(options.file), true) + } + } else if (existing.info !== undefined) yield* terminate(existing.info, read(options.file), true) + return yield* start(options) +}) + function fallback() { const state = process.env["XDG_STATE_HOME"] ?? join(homedir(), ".local", "state") return join(state, "opencode", "service.json") @@ -217,6 +233,7 @@ export const Info = Schema.Struct({ password: Schema.optional(Schema.String), }) export type Info = typeof Info.Type +const same = Schema.toEquivalence(Info) const decode = Schema.decodeUnknownEffect(Schema.fromJsonString(Info)) const decodeHealth = Schema.decodeUnknownOption(ServiceStatus.Health) @@ -305,27 +322,34 @@ const stopped = Effect.fnUntraced(function* (pid: number) { return yield* Effect.fail(new Error(`Server process ${pid} is still running`)) }) -function same(left: Info, right: Info) { - return left.id === right.id && left.version === right.version && left.url === right.url && left.pid === right.pid -} +const terminate = Effect.fnUntraced(function* ( + info: Info, + current: Effect.Effect, + graceful: boolean, +) { + if (graceful) { + const owner = yield* current + if (owner === undefined || !same(owner, info)) return "changed" as const + yield* signal(info.pid, "SIGTERM") + } + const done = yield* stopped(info.pid).pipe(Effect.retry(poll), Effect.option) + if (Option.isSome(done)) return "stopped" as const + + const latest = yield* current + if (latest === undefined || !same(latest, info)) return "changed" as const + yield* signal(info.pid, "SIGKILL") + yield* stopped(info.pid).pipe(Effect.retry(poll)) + return "stopped" as const +}) const kill = Effect.fnUntraced(function* (service: LocalService, options: Options, targetVersion?: string) { const requested = yield* requestStop(service, targetVersion) - if (requested === "rejected") return - if (requested === "unsupported") { - // A stale registration may point at a reused PID. Authenticate again - // immediately before the legacy signal fallback. - const current = yield* find(options) - if (current === undefined || !same(current.info, service.info)) return - yield* signal(service.info.pid, "SIGTERM") - } - const done = yield* stopped(service.info.pid).pipe(Effect.retry(poll), Effect.option) - if (Option.isSome(done)) return - - const latest = yield* find(options) - if (latest === undefined || !same(latest.info, service.info)) return - yield* signal(service.info.pid, "SIGKILL") - yield* stopped(service.info.pid).pipe(Effect.retry(poll)) + if (requested === "rejected") return "rejected" as const + return yield* terminate( + service.info, + registered(options.file, true).pipe(Effect.map((result) => result.service?.info)), + requested === "unsupported", + ) }) const decodeStopResponse = Schema.decodeUnknownOption(ServiceStatus.StopResponse) diff --git a/packages/client/test/fixture/service.ts b/packages/client/test/fixture/service.ts index 7b78ac190f..6e0e61e751 100644 --- a/packages/client/test/fixture/service.ts +++ b/packages/client/test/fixture/service.ts @@ -1,4 +1,4 @@ -import { appendFile, rename, writeFile } from "node:fs/promises" +import { appendFile, rename, rm, writeFile } from "node:fs/promises" const [registration, mode, delay] = process.argv.slice(2) if (registration === undefined || mode === undefined) throw new Error("Missing service fixture arguments") @@ -8,6 +8,35 @@ if (mode === "record-start") { process.exit(1) } if (mode === "signal") process.kill(process.pid, process.platform === "win32" ? "SIGTERM" : "SIGKILL") +if (mode === "unresponsive" || mode === "unresponsive-stubborn" || mode === "unresponsive-slow") { + const stalled = + mode === "unresponsive-slow" + ? Bun.serve({ + port: 0, + fetch: () => { + void writeFile(registration + ".health-request", "") + return new Promise(() => {}) + }, + }) + : undefined + await writeFile( + registration, + JSON.stringify({ + id: crypto.randomUUID(), + version: "test", + url: stalled?.url.toString() ?? "http://127.0.0.1:1", + pid: process.pid, + password: "secret", + }), + { mode: 0o600 }, + ) + process.on("SIGTERM", async () => { + if (mode === "unresponsive-stubborn") return + await rm(registration, { force: true }) + process.exit() + }) + await new Promise(() => {}) +} if (mode === "delayed" || mode === "delayed-failed" || mode === "coordinated") { await appendFile(registration + ".starts", process.pid + "\n") @@ -22,6 +51,7 @@ if (mode === "delayed" || mode === "delayed-failed" || mode === "coordinated") { } let requests = 0 +let stopDropped = false const version = mode === "old" || mode === "reject-stop" ? "old" : "test" const id = crypto.randomUUID() const server = Bun.serve({ @@ -32,6 +62,10 @@ const server = Bun.serve({ await writeFile(registration + ".stop-attempt", "") return Response.json({ accepted: false }) } + if (pathname === "/api/service/stop" && mode === "drop-stop") { + stopDropped = true + return new Promise(() => {}) + } if (pathname === "/api/service/stop" && mode === "graceful") { const body = await request.json() if (typeof body !== "object" || body === null || body.instanceID !== id) return Response.json({ accepted: false }) @@ -40,6 +74,7 @@ const server = Bun.serve({ return Response.json({ accepted: true }) } if (pathname !== "/api/health") return new Response(null, { status: 404 }) + if (stopDropped) return new Promise(() => {}) requests += 1 if (mode === "modern" && requests === 1) { await writeFile(registration + ".first-request", "") @@ -63,7 +98,7 @@ const server = Bun.serve({ }, { status: 503 }, ) - if (mode === "starting" || mode === "graceful" || mode === "reject-stop") + if (mode === "starting" || mode === "graceful" || mode === "reject-stop" || mode === "drop-stop") return Response.json({ healthy: true, version, pid: process.pid, instanceID: id, status: { type: "ready" } }) return Response.json({ healthy: true, version, pid: process.pid }) }, @@ -81,8 +116,9 @@ await writeFile( ) await rename(registration + ".tmp", registration) -function shutdown() { +async function shutdown() { server.stop(true) + await rm(registration, { force: true }) process.exit() } process.on("SIGTERM", shutdown) diff --git a/packages/client/test/service.test.ts b/packages/client/test/service.test.ts index 484baf7922..9e2fcdc4b2 100644 --- a/packages/client/test/service.test.ts +++ b/packages/client/test/service.test.ts @@ -11,8 +11,14 @@ const processes: Bun.Subprocess[] = [] const directories: string[] = [] afterEach(async () => { - processes.forEach((process) => process.kill("SIGTERM")) - await Promise.all(processes.splice(0).map((process) => process.exited)) + await Promise.all( + processes.splice(0).map(async (process) => { + process.kill("SIGTERM") + const exited = await Promise.race([process.exited.then(() => true), Bun.sleep(1_000).then(() => false)]) + if (!exited) process.kill("SIGKILL") + await process.exited + }), + ) await Promise.all(directories.splice(0).map((directory) => rm(directory, { recursive: true, force: true }))) }) @@ -89,6 +95,124 @@ test("requests graceful replacement of the exact service instance", async () => expect(await Bun.file(registration + ".stop").json()).toEqual({ instanceID: info.id, targetVersion: "next" }) }) +test("explicit restart replaces an unresponsive registered process", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "unresponsive") + await waitForFile(registration) + + const endpoint = await run( + Service.restart({ + file: registration, + version: "test", + command: [process.execPath, fixture, registration, "ready"], + }), + ) + await existing.exited + const info = await Bun.file(registration).json() + + try { + expect(endpoint.url).toBe(info.url) + expect(info.pid).not.toBe(existing.pid) + expect(await health(endpoint.url)).toMatchObject({ healthy: true, version: "test", pid: info.pid }) + } finally { + await run(Service.stop({ file: registration })) + } +}, 15_000) + +test("restart recovers when a healthy owner stops responding during shutdown", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "drop-stop") + await waitForFile(registration) + + const endpoint = await run( + Service.restart({ + file: registration, + version: "test", + command: [process.execPath, fixture, registration, "ready"], + }), + ) + await existing.exited + const info = await Bun.file(registration).json() + + try { + expect(endpoint.url).toBe(info.url) + expect(info.pid).not.toBe(existing.pid) + } finally { + await run(Service.stop({ file: registration })) + } +}, 15_000) + +test("restart fails when a responsive owner rejects shutdown", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "reject-stop") + await waitForFile(registration) + + await expect(run(Service.restart({ file: registration, version: "test" }))).rejects.toThrow( + "Background service rejected restart", + ) + expect(existing.exitCode).toBe(null) +}) + +test("ordinary stop never signals an unresponsive registered process", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "unresponsive") + await waitForFile(registration) + + await run(Service.stop({ file: registration })) + + expect(existing.exitCode).toBe(null) +}) + +test.skipIf(process.platform === "win32")( + "restart escalates when an unresponsive process ignores SIGTERM", + async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "unresponsive-stubborn") + await waitForFile(registration) + + const endpoint = await run( + Service.restart({ + file: registration, + version: "test", + command: [process.execPath, fixture, registration, "ready"], + }), + ) + await existing.exited + const info = await Bun.file(registration).json() + + try { + expect(endpoint.url).toBe(info.url) + expect(existing.signalCode).toBe("SIGKILL") + } finally { + await run(Service.stop({ file: registration })) + } + }, + 20_000, +) + +test("restart does not signal a process after registration changes", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "unresponsive-slow") + await waitForFile(registration) + const restarting = run( + Service.restart({ file: registration, version: "test", command: [] }), + ) + + await waitForFile(registration + ".health-request") + const replacement = spawn(registration, "ready") + await waitForRegistration(registration, replacement.pid) + const endpoint = await restarting + + expect(existing.exitCode).toBe(null) + expect(endpoint.url).toBe((await Bun.file(registration).json()).url) +}) + test("does not spawn contenders while an incompatible service rejects replacement", async () => { const directory = await temp() const registration = join(directory, "service.json") @@ -241,6 +365,17 @@ async function waitForFile(file: string) { throw new Error(`Timed out waiting for ${file}`) } +async function waitForRegistration(file: string, pid: number) { + for (let attempt = 0; attempt < 600; attempt++) { + const info = await Bun.file(file) + .json() + .catch(() => undefined) + if (info?.pid === pid) return + await Bun.sleep(5) + } + throw new Error(`Timed out waiting for registration from ${pid}`) +} + async function health(url: string) { return fetch(new URL("/api/health", url), { signal: AbortSignal.timeout(1_000) }).then((response) => response.json()) } diff --git a/packages/docs/build/client.mdx b/packages/docs/build/client.mdx index 70eed5104f..7960eb7ca3 100644 --- a/packages/docs/build/client.mdx +++ b/packages/docs/build/client.mdx @@ -115,6 +115,10 @@ Node application: a process. - `Service.start()` reuses a compatible service or starts one when needed. - `Service.stop()` stops the registered service. +- `Service.restart()` explicitly replaces the registered service, including + signaling an unchanged registered PID that no longer answers health checks. + Reserve it for deliberate user recovery; automatic reconnect should use + `Service.start()` to avoid the narrow stale-PID reuse risk. - `Service.headers(endpoint)` creates the authentication headers for a client. ```sh diff --git a/packages/tui/src/app.tsx b/packages/tui/src/app.tsx index 0611941a9e..ab32bace8f 100644 --- a/packages/tui/src/app.tsx +++ b/packages/tui/src/app.tsx @@ -140,7 +140,7 @@ export type TuiInput = { server: { endpoint: Service.Endpoint reconnect?: (onStatus: (status: Service.Status) => void, signal: AbortSignal) => Promise - reload?: () => Promise + reload?: (signal?: AbortSignal) => Promise } args: Args config: Config.Interface @@ -1121,7 +1121,7 @@ function App(props: { pair?: DialogPairCredentials }) { - + ) diff --git a/packages/tui/src/component/reconnecting.tsx b/packages/tui/src/component/reconnecting.tsx index 101e5d1775..3cf92dc510 100644 --- a/packages/tui/src/component/reconnecting.tsx +++ b/packages/tui/src/component/reconnecting.tsx @@ -1,11 +1,52 @@ import type { Service } from "@opencode-ai/client/effect" -import { Show } from "solid-js" +import { createSignal, onCleanup, Show } from "solid-js" +import { Keymap } from "../context/keymap" import { useTheme } from "../context/theme" +import { errorMessage } from "../util/error" import { Spinner } from "./spinner" -export function Reconnecting(props: { status?: Service.Status }) { +const restartCommand = "service.restart" + +export function Reconnecting(props: { status?: Service.Status; restart?: (signal?: AbortSignal) => Promise }) { const theme = useTheme().theme - const copy = () => reconnectingCopy(props.status) + const shortcuts = Keymap.useShortcuts() + const [restarting, setRestarting] = createSignal(false) + const [failure, setFailure] = createSignal() + let controller: AbortController | undefined + const copy = () => + restarting() + ? { loading: true, message: "Restarting background service..." } + : reconnectingCopy(props.status, shortcuts.get(restartCommand)) + + Keymap.createLayer(() => ({ + mode: "global", + priority: 1000, + commands: [ + { + id: restartCommand, + bind: "r", + title: "Restart service", + enabled: props.status?.type === "unresponsive" && !!props.restart, + run: () => { + if (!props.restart || restarting()) return + controller = new AbortController() + setFailure(undefined) + setRestarting(true) + void props + .restart(controller.signal) + .then( + () => setFailure(undefined), + (error) => { + setFailure(errorMessage(error)) + setRestarting(false) + }, + ) + }, + }, + ], + })) + + onCleanup(() => controller?.abort()) return ( + + {(message) => ( + + {message()} + + )} + ) } -export function reconnectingCopy(status?: Service.Status) { +export function reconnectingCopy(status?: Service.Status, restart = "r") { if (status?.type === "starting") return { loading: true, @@ -59,7 +107,7 @@ export function reconnectingCopy(status?: Service.Status) { return { loading: false, message: "Background service is not responding", - action: "Run `opencode service restart` to recover it.", + action: `[${restart}] Restart service`, } return { loading: true, message: "Waiting for background service..." } } diff --git a/packages/tui/src/context/client.tsx b/packages/tui/src/context/client.tsx index 6994f6a6f7..58308d0302 100644 --- a/packages/tui/src/context/client.tsx +++ b/packages/tui/src/context/client.tsx @@ -28,7 +28,7 @@ export const { use: useClient, provider: ClientProvider } = createSimpleContext( api: OpenCodeClient reconnect?: (onStatus: (status: Service.Status) => void, signal: AbortSignal) => Promise<{ api: OpenCodeClient }> // Stops and starts the managed service; present only in service mode. - reload?: () => Promise + reload?: (signal?: AbortSignal) => Promise }) => { const log = useLog({ component: "client" }) const abort = new AbortController() diff --git a/packages/tui/test/cli/tui/reconnecting-interaction.test.tsx b/packages/tui/test/cli/tui/reconnecting-interaction.test.tsx new file mode 100644 index 0000000000..3c51975389 --- /dev/null +++ b/packages/tui/test/cli/tui/reconnecting-interaction.test.tsx @@ -0,0 +1,73 @@ +/** @jsxImportSource @opentui/solid */ +import { testRender } from "@opentui/solid" +import { expect, test } from "bun:test" +import { ConfigProvider } from "../../../src/config" +import { Reconnecting } from "../../../src/component/reconnecting" +import { Keymap } from "../../../src/context/keymap" +import { ThemeProvider } from "../../../src/context/theme" +import { TestTuiContexts } from "../../fixture/tui-environment" +import { createTuiResolvedConfig } from "../../fixture/tui-runtime" + +function render(restart: (signal?: AbortSignal) => Promise) { + return testRender(() => ( + + + + Promise.resolve({}) }}> + + + + + + )) +} + +test("restarts an unresponsive service when r is pressed", async () => { + let restarts = 0 + let complete!: () => void + const restarting = new Promise((resolve) => { + complete = resolve + }) + const app = await render(() => { + restarts += 1 + return restarting + }) + app.renderer.start() + + try { + await app.waitForFrame((frame) => frame.includes("[r] Restart service")) + app.mockInput.pressKey("r") + await app.waitForFrame((frame) => frame.includes("Restarting background service...")) + app.mockInput.pressKey("r") + expect(restarts).toBe(1) + complete() + await Bun.sleep(0) + app.mockInput.pressKey("r") + expect(restarts).toBe(1) + } finally { + complete() + app.renderer.destroy() + } +}) + +test("cancels recovery when the reconnecting overlay unmounts", async () => { + let aborted = false + const app = await render( + (signal) => + new Promise((_, reject) => { + signal?.addEventListener("abort", () => { + aborted = true + reject(signal.reason) + }) + }), + ) + app.renderer.start() + + await app.waitForFrame((frame) => frame.includes("[r] Restart service")) + app.mockInput.pressKey("r") + await app.waitForFrame((frame) => frame.includes("Restarting background service...")) + app.renderer.destroy() + await Bun.sleep(0) + + expect(aborted).toBe(true) +}) diff --git a/packages/tui/test/cli/tui/reconnecting.test.ts b/packages/tui/test/cli/tui/reconnecting.test.ts index b8cca1ccc9..18ee7ef540 100644 --- a/packages/tui/test/cli/tui/reconnecting.test.ts +++ b/packages/tui/test/cli/tui/reconnecting.test.ts @@ -25,7 +25,7 @@ test("describes service status without transport diagnostics", () => { expect(reconnectingCopy({ type: "unresponsive" })).toEqual({ loading: false, message: "Background service is not responding", - action: "Run `opencode service restart` to recover it.", + action: "[r] Restart service", }) expect(JSON.stringify(reconnectingCopy())).not.toMatch(/Attempt|ECONNREFUSED|Event stream disconnected/) })