From bf3ca452b61d8f69c59204e77691c3f41b0ea696 Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Wed, 12 Aug 2026 00:13:53 -0400 Subject: [PATCH] fix(cli): harden managed service replacement --- .../src/commands/handlers/service/restart.ts | 2 +- .../cli/src/services/server-connection.ts | 2 +- packages/cli/src/services/service-config.ts | 11 ++ packages/cli/test/service.test.ts | 29 +++ packages/client/src/effect/service.ts | 67 ++----- packages/client/src/promise/service.ts | 113 ++++++------ packages/client/src/service.ts | 17 ++ packages/client/test/fixture/service.ts | 10 +- packages/client/test/promise-service.test.ts | 104 +++++++++-- packages/client/test/service.test.ts | 167 +++++++++++++++--- .../tui/src/component/migration-overlay.tsx | 14 +- 11 files changed, 397 insertions(+), 139 deletions(-) diff --git a/packages/cli/src/commands/handlers/service/restart.ts b/packages/cli/src/commands/handlers/service/restart.ts index fe306dcb02..baaee7abc1 100644 --- a/packages/cli/src/commands/handlers/service/restart.ts +++ b/packages/cli/src/commands/handlers/service/restart.ts @@ -8,7 +8,7 @@ import { ServiceConfig } from "../../../services/service-config" export default Runtime.handler( Commands.commands.service.commands.restart, Effect.fn("cli.service.restart")(function* () { - const options = yield* ServiceConfig.options() + const options = yield* ServiceConfig.options({ checkVersion: true }) yield* Service.stop(options) const transport = yield* Service.ensure(options) process.stdout.write(transport.url + EOL) diff --git a/packages/cli/src/services/server-connection.ts b/packages/cli/src/services/server-connection.ts index 785eeb89c7..786576f7bc 100644 --- a/packages/cli/src/services/server-connection.ts +++ b/packages/cli/src/services/server-connection.ts @@ -57,7 +57,7 @@ function managedService(options: EnsureOptions) { restart: () => Effect.gen(function* () { yield* Service.stop(options) - yield* Service.ensure(reconnectOptions) + yield* Service.ensure(options) }), } } diff --git a/packages/cli/src/services/service-config.ts b/packages/cli/src/services/service-config.ts index d348f390c2..375d590e74 100644 --- a/packages/cli/src/services/service-config.ts +++ b/packages/cli/src/services/service-config.ts @@ -5,6 +5,7 @@ import { Service } from "@opencode-ai/client/effect/service" import { Effect, FileSystem, Option, Schema } from "effect" import { randomBytes } from "crypto" import path from "path" +import semver from "semver" import { selfCommand } from "../util/process" // The CLI's service configuration file, plus the Service.EnsureOptions binding that @@ -104,10 +105,20 @@ export const options = Effect.fnUntraced(function* (input: { readonly checkVersi return { file, version: input.checkVersion ? OPENCODE_VERSION : undefined, + canReplace: (version: string | undefined) => canReplaceVersion(version), command: [...selfCommand(), "serve", "--service"], } }) +export function canReplaceVersion(serverVersion: string | undefined, clientVersion = OPENCODE_VERSION) { + if (serverVersion === undefined) return false + // Compare preview build numbers numerically rather than as semver prerelease strings. + const server = serverVersion.replace(/-(\d+)(?=(?:\.\d+)?$)/, ".$1") + const client = clientVersion.replace(/-(\d+)(?=(?:\.\d+)?$)/, ".$1") + if (!semver.valid(server) || !semver.valid(client)) return false + return semver.lt(server, client) +} + export const read = Effect.fn("cli.service-config.read")(function* () { const { fs, configFile, legacyConfigFile } = yield* paths if (legacyConfigFile) yield* migrateConfig(legacyConfigFile, configFile) diff --git a/packages/cli/test/service.test.ts b/packages/cli/test/service.test.ts index 3e29d6e443..a7c3a2cdf5 100644 --- a/packages/cli/test/service.test.ts +++ b/packages/cli/test/service.test.ts @@ -47,6 +47,35 @@ test("service filenames share release channels and identify preview channels", ( expect(ServiceConfig.versionBelongsToChannel("1.2.3", "preview-a")).toBe(false) }) +test("only newer clients replace managed service versions", () => { + expect(ServiceConfig.canReplaceVersion("0.0.0-next-17271", "0.0.0-next-17272")).toBe(true) + expect(ServiceConfig.canReplaceVersion("0.0.0-next-17272", "0.0.0-next-17271")).toBe(false) + expect(ServiceConfig.canReplaceVersion("0.0.0-next-17272", "0.0.0-next-17272")).toBe(false) + expect(ServiceConfig.canReplaceVersion(undefined, "0.0.0-next-17272")).toBe(false) + expect(ServiceConfig.canReplaceVersion("development-a", "development-b")).toBe(false) + expect(ServiceConfig.canReplaceVersion("development-b", "development-a")).toBe(false) +}) + +test("managed version replacement can never be mutual", () => { + const versions = [ + undefined, + "1.0.0", + "1.0.1", + "0.0.0-next-9999", + "0.0.0-next-15000", + "0.0.0-next-15000.1", + "0.0.0-next-15000.2", + "development-a", + "development-b", + ] + for (const left of versions) { + for (const right of versions) { + if (left === undefined || right === undefined) continue + expect(ServiceConfig.canReplaceVersion(left, right) && ServiceConfig.canReplaceVersion(right, left)).toBe(false) + } + } +}) + test("service config migrates from the hashed channel filename", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "opencode-service-config-migration-")) const legacy = path.join(root, ServiceConfig.legacyFilename("preview-a")!) diff --git a/packages/client/src/effect/service.ts b/packages/client/src/effect/service.ts index 733bbf8a7a..f2fe7faee6 100644 --- a/packages/client/src/effect/service.ts +++ b/packages/client/src/effect/service.ts @@ -2,7 +2,7 @@ import { ServiceStatus } from "@opencode-ai/protocol/groups/health" import { Effect, FileSystem, Option, Schedule, Schema } from "effect" import { homedir } from "node:os" import { join } from "node:path" -import type { DiscoverOptions, Endpoint, EnsureOptions, StopOptions } from "../service.js" +import { VersionMismatchError, type DiscoverOptions, type Endpoint, type EnsureOptions, type StopOptions } from "../service.js" import { contenderFailure, contenderFinished, @@ -56,7 +56,6 @@ const discoverLocal = Effect.fnUntraced(function* (options: DiscoverOptions) { export const ensure = Effect.fn("service.ensure")(function* (options: EnsureOptions = {}) { const timing = ensureTiming(options) const contenders = new Set() - let timeouts: { readonly info: Info; readonly count: number } | undefined let announced = false let lastSpawn = 0 let spawnDelay = timing.spawnDelay @@ -80,18 +79,6 @@ export const ensure = Effect.fn("service.ensure")(function* (options: EnsureOpti const registration = yield* registered(options.file, true, timing.requestTimeout) const info = registration.info const service = registration.service - if (registration.timedOut && info !== undefined) { - timeouts = { - info, - count: timeouts !== undefined && same(timeouts.info, info) ? timeouts.count + 1 : 1, - } - if (timeouts.count >= 3) { - yield* announce("missing") - yield* evict(info, options, timing) - timeouts = undefined - lastSpawn = Date.now() - spawnDelay - } - } else timeouts = undefined if (service !== undefined) { spawnDelay = timing.spawnDelay const compatible = !service.legacy && matchesVersion(service.version, options) @@ -99,8 +86,10 @@ export const ensure = Effect.fn("service.ensure")(function* (options: EnsureOpti if (compatible && service.state === "failed") return yield* Effect.fail(new Error("Background service failed to start")) if (compatible) return Option.none() + if (!service.legacy && options.canReplace?.(service.version) === false) + return yield* Effect.fail(new VersionMismatchError(options.version, service.version)) yield* announce("version-mismatch", service.version) - yield* kill(service, options, timing).pipe(Effect.ignore) + yield* kill(service, options, timing) lastSpawn = 0 return Option.none() } else if (lastSpawn === 0 && info !== undefined) lastSpawn = Date.now() @@ -135,6 +124,8 @@ export const ensure = Effect.fn("service.ensure")(function* (options: EnsureOpti export const stop = Effect.fn("service.stop")(function* (options: StopOptions = {}) { const existing = yield* find(options) if (existing !== undefined) yield* kill(existing, options, defaultEnsureTiming) + if (existing === undefined && (yield* read(options.file)) !== undefined) + return yield* Effect.fail(new Error("Background service is not responding; stop its process manually and try again")) }) function fallback() { @@ -254,9 +245,6 @@ const find = Effect.fnUntraced(function* (options: { readonly file?: string }) { const poll = (timing: EnsureTiming) => Schedule.max([Schedule.spaced(timing.stopPollInterval), Schedule.recurs(timing.stopPollAttempts)]) -const signal = (pid: number, name: NodeJS.Signals) => - Effect.try({ try: () => process.kill(pid, name), catch: (cause) => cause }).pipe(Effect.ignore) - const stopped = Effect.fnUntraced(function* (pid: number) { const running = yield* Effect.try({ try: () => process.kill(pid, 0), catch: () => false }).pipe( Effect.orElseSucceed(() => false), @@ -266,43 +254,25 @@ const stopped = Effect.fnUntraced(function* (pid: number) { }) function same(left: Info, right: Info) { - return left.id === right.id && left.version === right.version && left.url === right.url && left.pid === right.pid + return ( + left.id === right.id && + left.version === right.version && + left.url === right.url && + left.pid === right.pid && + left.password === right.password + ) } -const evict = Effect.fnUntraced(function* (info: Info, options: { readonly file?: string }, timing: EnsureTiming) { - const current = yield* read(options.file) - if (current === undefined || !same(current, info)) return - yield* signal(info.pid, "SIGTERM") - const done = yield* stopped(info.pid).pipe(Effect.retry(poll(timing)), Effect.option) - if (Option.isSome(done)) return - - const latest = yield* read(options.file) - if (latest === undefined || !same(latest, info)) return - yield* signal(info.pid, "SIGKILL") - yield* stopped(info.pid).pipe(Effect.retry(poll(timing))) -}) - -const kill = Effect.fnUntraced(function* ( - service: LocalService, - options: { readonly file?: string }, - timing: EnsureTiming, -) { +const kill = Effect.fnUntraced(function* (service: LocalService, _options: { readonly file?: string }, timing: EnsureTiming) { const requested = yield* requestStop(service, timing.requestTimeout) - if (requested === "rejected") return + if (requested === "rejected") return yield* Effect.fail(new Error("Background service rejected the stop request")) 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") + return yield* Effect.fail(new Error("Background service does not support authenticated stop requests")) } const done = yield* stopped(service.info.pid).pipe(Effect.retry(poll(timing)), 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(timing))) + return yield* Effect.fail(new Error("Background service accepted the stop request but did not exit")) }) const decodeStopResponse = Schema.decodeUnknownOption(ServiceStatus.StopResponse) @@ -317,7 +287,8 @@ const requestStop = Effect.fnUntraced(function* (service: LocalService, timeout signal: AbortSignal.timeout(timeout), }), ).pipe(Effect.option, Effect.map(Option.getOrUndefined)) - if (response === undefined || response.status === 404 || response.status === 405) return "unsupported" as const + if (response === undefined) return "rejected" as const + if (response.status === 404 || response.status === 405) return "unsupported" as const const body = yield* Effect.tryPromise(() => response.json()).pipe(Effect.option, Effect.map(Option.getOrUndefined)) const decoded = decodeStopResponse(body) if (!response.ok || Option.isNone(decoded) || !decoded.value.accepted) return "rejected" as const diff --git a/packages/client/src/promise/service.ts b/packages/client/src/promise/service.ts index d54da6c4e9..221d1e6206 100644 --- a/packages/client/src/promise/service.ts +++ b/packages/client/src/promise/service.ts @@ -1,7 +1,14 @@ import { readFile } from "node:fs/promises" import { homedir } from "node:os" import { join } from "node:path" -import type { DiscoverOptions, Endpoint, Info, EnsureOptions, StopOptions } from "../service.js" +import { + VersionMismatchError, + type DiscoverOptions, + type Endpoint, + type Info, + type EnsureOptions, + type StopOptions, +} from "../service.js" import { contenderFailure, contenderFinished, @@ -37,7 +44,6 @@ export async function ensure(options: EnsureOptions = {}): Promise { const timing = ensureTiming(options) const deadline = Date.now() + timing.promiseTimeout const contenders = new Set() - let timeouts: { readonly info: Info; readonly count: number } | undefined let announced = false let lastSpawn = 0 let spawnDelay = timing.spawnDelay @@ -61,19 +67,6 @@ export async function ensure(options: EnsureOptions = {}): Promise { while (true) { if (Date.now() >= deadline) throw new Error("Timed out waiting for the background service to start") const registration = await registered(options.file, true, timing.requestTimeout) - if (registration.timedOut && registration.info !== undefined) { - timeouts = { - info: registration.info, - count: timeouts !== undefined && same(timeouts.info, registration.info) ? timeouts.count + 1 : 1, - } - if (timeouts.count >= 3) { - announce("missing") - await evict(registration.info, options, timing) - timeouts = undefined - lastSpawn = Date.now() - spawnDelay - } - } else timeouts = undefined - if (registration.service !== undefined) { spawnDelay = timing.spawnDelay const service = registration.service @@ -81,8 +74,10 @@ export async function ensure(options: EnsureOptions = {}): Promise { if (compatible && service.state === "ready") return service.endpoint if (compatible && service.state === "failed") throw new Error("Background service failed to start") if (!compatible) { + if (!service.legacy && options.canReplace?.(service.version) === false) + throw new VersionMismatchError(options.version, service.version) announce("version-mismatch", service.version) - await kill(service, options, timing).catch(() => undefined) + await kill(service, options, timing) lastSpawn = 0 } } else { @@ -112,6 +107,8 @@ export async function ensure(options: EnsureOptions = {}): Promise { export async function stop(options: StopOptions = {}) { const existing = await find(options) if (existing !== undefined) await kill(existing, options, defaultEnsureTiming) + if (existing === undefined && (await read(options.file)) !== undefined) + throw new Error("Background service is not responding; stop its process manually and try again") } function fallback() { @@ -130,7 +127,15 @@ async function read(file?: string) { const text = await readFile(file ?? fallback(), "utf8").catch(() => undefined) if (text === undefined) return undefined try { - return JSON.parse(text) as Info + const value: unknown = JSON.parse(text) + if (typeof value !== "object" || value === null) return undefined + if (!("url" in value) || typeof value.url !== "string") return undefined + if (!("pid" in value) || !Number.isInteger(value.pid) || typeof value.pid !== "number" || value.pid <= 0) + return undefined + if ("id" in value && value.id !== undefined && typeof value.id !== "string") return undefined + if ("version" in value && value.version !== undefined && typeof value.version !== "string") return undefined + if ("password" in value && value.password !== undefined && typeof value.password !== "string") return undefined + return value as Info } catch { return undefined } @@ -172,7 +177,18 @@ async function probeResult(info: Info, allowLegacy = false, timeout = defaultEns if ("cause" in result) return { service: undefined, timedOut: signal.aborted } const response = result.value.response const body = result.value.body - if (body !== undefined && "version" in body && "pid" in body) { + if ( + typeof body === "object" && + body !== null && + "healthy" in body && + body.healthy === true && + "version" in body && + typeof body.version === "string" && + "pid" in body && + typeof body.pid === "number" && + Number.isInteger(body.pid) && + body.pid > 0 + ) { if (body.pid !== info.pid) return { service: undefined, timedOut: false } if (info.version !== undefined && body.version !== info.version) return { service: undefined, timedOut: false } return { @@ -186,7 +202,16 @@ async function probeResult(info: Info, allowLegacy = false, timeout = defaultEns timedOut: false, } } - if (!allowLegacy || body?.healthy !== true) return { service: undefined, timedOut: false } + if ( + !allowLegacy || + typeof body !== "object" || + body === null || + !("healthy" in body) || + body.healthy !== true || + "version" in body || + "pid" in body + ) + return { service: undefined, timedOut: false } return { service: { info, endpoint, state: "ready", legacy: true } satisfies LocalService, timedOut: false, @@ -203,12 +228,6 @@ async function find(options: { readonly file?: string }) { return (await registered(options.file, true)).service } -function signal(pid: number, name: NodeJS.Signals) { - try { - process.kill(pid, name) - } catch {} -} - function stopped(pid: number) { try { process.kill(pid, 0) @@ -227,36 +246,25 @@ async function waitUntilStopped(pid: number, timing: EnsureTiming) { } function same(left: Info, right: Info) { - return left.id === right.id && left.version === right.version && left.url === right.url && left.pid === right.pid + return ( + left.id === right.id && + left.version === right.version && + left.url === right.url && + left.pid === right.pid && + left.password === right.password + ) } -async function evict(info: Info, options: { readonly file?: string }, timing: EnsureTiming) { - const current = await read(options.file) - if (current === undefined || !same(current, info)) return - signal(info.pid, "SIGTERM") - if (await waitUntilStopped(info.pid, timing)) return - - const latest = await read(options.file) - if (latest === undefined || !same(latest, info)) return - signal(info.pid, "SIGKILL") - if (!(await waitUntilStopped(info.pid, timing))) throw new Error(`Server process ${info.pid} is still running`) -} - -async function kill(service: LocalService, options: { readonly file?: string }, timing: EnsureTiming) { +async function kill( + service: LocalService, + _options: { readonly file?: string }, + timing: EnsureTiming, +) { const requested = await requestStop(service, timing.requestTimeout) - if (requested === "rejected") return - if (requested === "unsupported") { - const current = await find(options) - if (current === undefined || !same(current.info, service.info)) return - signal(service.info.pid, "SIGTERM") - } + if (requested === "rejected") throw new Error("Background service rejected the stop request") + if (requested === "unsupported") throw new Error("Background service does not support authenticated stop requests") if (await waitUntilStopped(service.info.pid, timing)) return - - const latest = await find(options) - if (latest === undefined || !same(latest.info, service.info)) return - signal(service.info.pid, "SIGKILL") - if (!(await waitUntilStopped(service.info.pid, timing))) - throw new Error(`Server process ${service.info.pid} is still running`) + throw new Error("Background service accepted the stop request but did not exit") } async function requestStop(service: LocalService, timeout = defaultEnsureTiming.requestTimeout) { @@ -267,7 +275,8 @@ async function requestStop(service: LocalService, timeout = defaultEnsureTiming. body: JSON.stringify({ instanceID: service.info.id }), signal: AbortSignal.timeout(timeout), }).catch(() => undefined) - if (response === undefined || response.status === 404 || response.status === 405) return "unsupported" as const + if (response === undefined) return "rejected" as const + if (response.status === 404 || response.status === 405) return "unsupported" as const const body = (await response.json().catch(() => undefined)) as ServiceStopResponse | undefined if (!response.ok || body?.accepted !== true) return "rejected" as const return "accepted" as const diff --git a/packages/client/src/service.ts b/packages/client/src/service.ts index 65c3226643..50a342ae51 100644 --- a/packages/client/src/service.ts +++ b/packages/client/src/service.ts @@ -28,10 +28,27 @@ export type EnsureReason = "missing" | "version-mismatch" export type EnsureOptions = DiscoverOptions & { /** Service command and arguments. Defaults to `opencode serve --service`. */ readonly command?: ReadonlyArray + /** Decide whether a version-mismatched service may be replaced. Defaults to true. */ + readonly canReplace?: (version: string | undefined) => boolean /** Called once before spawning a new service process. */ readonly onStart?: (reason: EnsureReason, previousVersion?: string) => void } +/** A healthy service exists, but the caller's replacement policy protects it. */ +export class VersionMismatchError extends Error { + override readonly name = "VersionMismatchError" + + constructor( + readonly clientVersion: string | undefined, + readonly serverVersion: string | undefined, + ) { + super( + `Background service ${serverVersion ?? "unknown"} is newer than this client ${clientVersion ?? "unknown"}. ` + + "Run `opencode2 service restart` to activate this installed version.", + ) + } +} + /** Options used to stop the local OpenCode service. */ export type StopOptions = { /** Absolute registration file path. Defaults to the XDG state directory. */ diff --git a/packages/client/test/fixture/service.ts b/packages/client/test/fixture/service.ts index 4faf13e693..09dee01465 100644 --- a/packages/client/test/fixture/service.ts +++ b/packages/client/test/fixture/service.ts @@ -28,7 +28,7 @@ if (mode === "delayed" || mode === "delayed-failed" || mode === "coordinated" || let requests = 0 let version = "test" -if (mode === "old" || mode === "reject-stop") version = "old" +if (mode === "old" || mode === "reject-stop" || mode === "stop-hanging") version = "old" if (mode === "incompatible") version = "1.9.0" if (mode === "compatible" || mode === "delayed-compatible") version = "2.1.0-next.1" const id = crypto.randomUUID() @@ -40,7 +40,11 @@ const server = Bun.serve({ await appendFile(registration + ".stop-attempts", process.pid + "\n") return Response.json({ accepted: false }) } - if (pathname === "/api/service/stop" && mode === "graceful") { + if (pathname === "/api/service/stop" && mode === "stop-hanging") { + await appendFile(registration + ".stop-attempts", process.pid + "\n") + return new Promise(() => {}) + } + if (pathname === "/api/service/stop" && (mode === "graceful" || mode === "old")) { const body = await request.json() if (typeof body !== "object" || body === null || body.instanceID !== id) return Response.json({ accepted: false }) await writeFile(registration + ".stop", JSON.stringify(body)) @@ -63,7 +67,7 @@ const server = Bun.serve({ if (mode === "starting" && !(await Bun.file(registration + ".release").exists())) return Response.json({ healthy: true, version, pid: process.pid }, { status: 503 }) if (mode === "failed-owner") return Response.json({ healthy: true, version, pid: process.pid }, { status: 500 }) - if (mode === "starting" || mode === "graceful" || mode === "reject-stop") + if (mode === "starting" || mode === "graceful" || mode === "reject-stop" || mode === "stop-hanging") return Response.json({ healthy: true, version, pid: process.pid }) return Response.json({ healthy: true, version, pid: process.pid }) }, diff --git a/packages/client/test/promise-service.test.ts b/packages/client/test/promise-service.test.ts index 78d871ab5f..69fcde6eac 100644 --- a/packages/client/test/promise-service.test.ts +++ b/packages/client/test/promise-service.test.ts @@ -38,6 +38,50 @@ test("discovers a compatible registered service", async () => { expect(await Service.discover({ file: registration, version: (version) => version.startsWith("3.") })).toBeUndefined() }) +test("rejects malformed registrations without probing or signaling", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const malformed = [ + null, + [], + {}, + { url: "http://127.0.0.1:1" }, + { url: "http://127.0.0.1:1", pid: 0 }, + { url: "http://127.0.0.1:1", pid: -1 }, + { url: "http://127.0.0.1:1", pid: 1.5 }, + { url: "http://127.0.0.1:1", pid: "1" }, + { url: "http://127.0.0.1:1", pid: 1, id: 1 }, + ] + + for (const value of malformed) { + await Bun.write(registration, JSON.stringify(value)) + expect(await Service.discover({ file: registration })).toBeUndefined() + } +}) + +test("rejects primitive and partial modern health responses", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const bodies = [ + null, + 1, + "healthy", + [], + {}, + { healthy: false, version: "test", pid: process.pid }, + { healthy: true, version: null, pid: process.pid }, + { healthy: true, version: "test", pid: "1" }, + { healthy: true, version: "test" }, + { healthy: true, pid: process.pid }, + ] + + for (const body of bodies) { + using server = Bun.serve({ port: 0, fetch: () => Response.json(body) }) + await Bun.write(registration, JSON.stringify({ url: server.url.toString(), pid: process.pid })) + expect(await Service.discover({ file: registration })).toBeUndefined() + } +}) + test("ensures a missing service with native promises", async () => { const directory = await temp() const registration = join(directory, "service.json") @@ -100,7 +144,7 @@ test("reports a bounded contender stderr tail with native promises", async () => expect(error.message.length).toBeLessThan(9_000) }, 10_000) -test("evicts an unresponsive registered service before starting its replacement", async () => { +test("never evicts an unresponsive registered service automatically", async () => { const directory = await temp() const registration = join(directory, "service.json") const existing = Bun.spawn([process.execPath, fixture, registration, "hanging"], { @@ -111,19 +155,46 @@ test("evicts an unresponsive registered service before starting its replacement" await waitForFile(registration) const original = await Bun.file(registration).json() - const endpoint = await ensure({ + const options = { file: registration, version: "test", - command: [process.execPath, fixture, registration, "delayed", "10"], - }) - const replacement = await Bun.file(registration).json() + command: [process.execPath, fixture, registration, "record-start"], + } + const result = ensure(options) + await waitForLines(registration + ".requests", 3) - expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(3) - expect(await existing.exited).toBe(0) - expect(replacement.pid).not.toBe(original.pid) - expect(endpoint.url).toBe(replacement.url) - process.kill(replacement.pid, "SIGTERM") - await waitForExit(replacement.pid) + expect(existing.exitCode).toBe(null) + expect(await Bun.file(registration).json()).toEqual(original) + await expect(result).rejects.toThrow() +}) + +test("explicit native stop refuses to signal an unidentified unresponsive PID", async () => { + const registration = await setup("hanging") + const info = await Bun.file(registration).json() + + await expect(Service.stop({ file: registration })).rejects.toThrow("stop its process manually") + + expect(process.kill(info.pid, 0)).toBe(true) +}) + +test("a stale native client refuses to replace a newer service", async () => { + const registration = await setup("graceful") + const directory = await temp() + const contender = join(directory, "contender.json") + const info = await Bun.file(registration).json() + + await expect( + ensure({ + file: registration, + version: "old", + canReplace: () => false, + command: [process.execPath, fixture, contender, "record-start"], + }), + ).rejects.toThrow("Run `opencode2 service restart` to activate this installed version") + + expect(await Bun.file(contender + ".started").exists()).toBe(false) + expect(process.kill(info.pid, 0)).toBe(true) + expect(await Bun.file(registration).json()).toEqual(info) }) test("requests graceful stop of the exact service instance", async () => { @@ -156,3 +227,14 @@ async function waitForFile(file: string) { } throw new Error(`Timed out waiting for ${file}`) } + +async function waitForLines(file: string, count: number) { + for (let attempt = 0; attempt < 600; attempt++) { + const text = await Bun.file(file) + .text() + .catch(() => "") + if (text.trim().split("\n").length >= count) return + await Bun.sleep(5) + } + throw new Error(`Timed out waiting for ${count} lines in ${file}`) +} diff --git a/packages/client/test/service.test.ts b/packages/client/test/service.test.ts index 152f6cd15e..fb9d3fed5d 100644 --- a/packages/client/test/service.test.ts +++ b/packages/client/test/service.test.ts @@ -118,29 +118,39 @@ test("reports a failed registered service without spawning", async () => { expect(process.exitCode).toBe(null) }) -test("evicts an unresponsive registered service before starting its replacement", async () => { +test("never evicts an unresponsive registered service automatically", async () => { const directory = await temp() const registration = join(directory, "service.json") const existing = spawn(registration, "hanging") await waitForFile(registration) const original = await Bun.file(registration).json() - const endpoint = await run( + const controller = new AbortController() + const result = Effect.runPromise( ensure({ file: registration, version: "test", - command: [process.execPath, fixture, registration, "delayed", "10"], - }), + command: [process.execPath, fixture, registration, "record-start"], + }).pipe(Effect.provide(NodeFileSystem.layer)), + { signal: controller.signal }, ) - const replacement = await Bun.file(registration).json() + await waitForLines(registration + ".requests", 3) + controller.abort() + await result.catch(() => undefined) - expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(3) - expect(await existing.exited).toBe(0) - expect(replacement.pid).not.toBe(original.pid) - expect(endpoint.url).toBe(replacement.url) - expect(await health(endpoint.url)).toEqual({ healthy: true, version: "test", pid: replacement.pid }) - process.kill(replacement.pid, "SIGTERM") - await waitForExit(replacement.pid) + expect(existing.exitCode).toBe(null) + expect(await Bun.file(registration).json()).toEqual(original) +}) + +test("explicit stop refuses to signal an unidentified unresponsive PID", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "hanging") + await waitForFile(registration) + + await expect(run(Service.stop({ file: registration }))).rejects.toThrow("stop its process manually") + + expect(existing.exitCode).toBe(null) }) test("requests graceful stop of the exact service instance", async () => { @@ -161,25 +171,108 @@ test("does not spawn contenders while an incompatible service rejects replacemen const contender = join(directory, "contender.json") const existing = spawn(registration, "reject-stop") await waitForFile(registration) - const controller = new AbortController() - const starting = Effect.runPromise( + const starting = run( ensure({ file: registration, version: "test", command: [process.execPath, fixture, contender, "record-start"], - }).pipe(Effect.provide(NodeFileSystem.layer)), - { signal: controller.signal }, + }), ) - await waitForLines(registration + ".stop-attempts", 2) - controller.abort() - await starting.catch(() => undefined) + await expect(starting).rejects.toThrow("Background service rejected the stop request") expect(await Bun.file(contender + ".started").exists()).toBe(false) + expect((await Bun.file(registration + ".stop-attempts").text()).trim().split("\n")).toHaveLength(1) expect(existing.exitCode).toBe(null) }) -test("a legacy health response is still replaced", async () => { +test("does not signal a modern service when its stop request times out", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const contender = join(directory, "contender.json") + const existing = spawn(registration, "stop-hanging") + await waitForFile(registration) + const starting = run( + ensure({ + file: registration, + version: "test", + command: [process.execPath, fixture, contender, "record-start"], + }), + ) + + await expect(starting).rejects.toThrow("Background service rejected the stop request") + + expect(await Bun.file(contender + ".started").exists()).toBe(false) + expect((await Bun.file(registration + ".stop-attempts").text()).trim().split("\n")).toHaveLength(1) + expect(existing.exitCode).toBe(null) +}) + +test("explicit stop refuses to signal when a modern stop request times out", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const existing = spawn(registration, "stop-hanging") + await waitForFile(registration) + + await expect(run(Service.stop({ file: registration }))).rejects.toThrow("Background service rejected the stop request") + + expect(existing.exitCode).toBe(null) +}) + +test("a stale client refuses to replace a newer service", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const contender = join(directory, "contender.json") + const existing = spawn(registration, "graceful") + await waitForFile(registration) + const info = await Bun.file(registration).json() + + await expect( + run( + ensure({ + file: registration, + version: "old", + canReplace: () => false, + command: [process.execPath, fixture, contender, "record-start"], + }), + ), + ).rejects.toThrow("Run `opencode2 service restart` to activate this installed version") + + expect(await Bun.file(contender + ".started").exists()).toBe(false) + expect(existing.exitCode).toBe(null) + expect(await Bun.file(registration).json()).toEqual(info) +}) + +test("explicit restart can activate an installed downgrade", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const current = spawn(registration, "graceful") + await waitForFile(registration) + const before = await Bun.file(registration).json() + const options = { + file: registration, + version: "old", + canReplace: () => false, + command: [process.execPath, fixture, registration, "old"], + } + + await expect(run(ensure(options))).rejects.toThrow("Run `opencode2 service restart`") + expect(current.exitCode).toBe(null) + + await run(Service.stop({ file: registration })) + const endpoint = await run(ensure(options)) + const after = await Bun.file(registration).json() + + try { + expect(after.pid).not.toBe(before.pid) + expect(after.version).toBe("old") + expect(endpoint.url).toBe(after.url) + } finally { + process.kill(after.pid, "SIGTERM") + await waitForExit(after.pid) + } +}) + +test("refuses to signal a legacy service without authenticated stop", async () => { const directory = await temp() const registration = join(directory, "service.json") const existing = spawn(registration, "legacy") @@ -188,9 +281,9 @@ test("a legacy health response is still replaced", async () => { const starts: EnsureReason[] = [] const result = run(ensure({ file: registration, command: [], onStart: (reason) => starts.push(reason) })) - await expect(result).rejects.toThrow("Missing service command") + await expect(result).rejects.toThrow("does not support authenticated stop requests") expect(starts).toEqual(["version-mismatch"]) - await existing.exited + expect(existing.exitCode).toBe(null) }) test("waits for a slow winner while bounding lock probes", async () => { @@ -317,6 +410,36 @@ test("replaces an incompatible owner that appears during startup", async () => { } }) +test("concurrent current-version launchers converge on one replacement", async () => { + const directory = await temp() + const registration = join(directory, "service.json") + const old = spawn(registration, "old") + await waitForFile(registration) + + const endpoints = await Promise.all( + Array.from({ length: 20 }, (_, index) => { + const options = { + file: registration, + version: "test", + command: [process.execPath, fixture, registration, "coordinated"], + canReplace: (version: string | undefined) => version === "old", + } + return index % 2 === 0 ? run(ensure(options)) : import("../src/promise/service").then((mod) => mod.Service.ensure(options)) + }), + ) + const info = await Bun.file(registration).json() + + try { + expect(new Set(endpoints.map((endpoint) => endpoint.url))).toEqual(new Set([info.url])) + expect(info.version).toBe("test") + expect(old.exitCode).not.toBe(null) + expect(await health(info.url)).toEqual({ healthy: true, version: "test", pid: info.pid }) + } finally { + process.kill(info.pid, "SIGTERM") + await waitForExit(info.pid) + } +}) + function run(effect: Effect.Effect) { return Effect.runPromise(effect.pipe(Effect.provide(NodeFileSystem.layer))) } diff --git a/packages/tui/src/component/migration-overlay.tsx b/packages/tui/src/component/migration-overlay.tsx index 21943529af..fcd6b1166f 100644 --- a/packages/tui/src/component/migration-overlay.tsx +++ b/packages/tui/src/component/migration-overlay.tsx @@ -1,3 +1,4 @@ +import { ClientError } from "@opencode-ai/client" import { createSignal, onCleanup, onMount, Show } from "solid-js" import { useClient } from "../context/client" import { useTheme } from "../context/theme" @@ -18,7 +19,18 @@ export function MigrationOverlay() { await Bun.sleep(1_000) void (async () => { while (true) { - const status = await client.api.migration.v1.status({ signal: abort.signal }) + const result = await client.api.migration.v1.status({ signal: abort.signal }).then( + (status) => ({ status }), + (error: unknown) => ({ error }), + ) + if ("error" in result) { + if (result.error instanceof ClientError && result.error.reason === "Transport") { + await Bun.sleep(1_000) + continue + } + throw result.error + } + const status = result.status setProgress(status.status === "running" ? status.progress : undefined) if (status.status === "completed") return if (status.status === "error") throw new Error(status.error)