From ed4eaf6f46bd52281ed4445e64e9a43665396be2 Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 22 Jul 2026 19:05:07 +0000 Subject: [PATCH] fix(cli): harden service lifecycle edge cases Co-Authored-By: Paperclip --- cli/src/__tests__/install-command.test.ts | 14 +++++++++++++- cli/src/__tests__/service-health-check.test.ts | 8 +++++++- cli/src/__tests__/service-manager.test.ts | 14 ++++++++++++++ cli/src/checks/service-health-check.ts | 13 ++++--------- cli/src/commands/service.ts | 16 +++++++--------- cli/src/commands/uninstall.ts | 17 ++++++++++++++++- cli/src/services/service-manager.ts | 2 +- cli/src/utils/health-url.ts | 10 ++++++++++ 8 files changed, 72 insertions(+), 22 deletions(-) create mode 100644 cli/src/utils/health-url.ts diff --git a/cli/src/__tests__/install-command.test.ts b/cli/src/__tests__/install-command.test.ts index 4a1d932e34c..4653e3ab8c7 100644 --- a/cli/src/__tests__/install-command.test.ts +++ b/cli/src/__tests__/install-command.test.ts @@ -79,8 +79,20 @@ describe("managed install commands", () => { const userData = path.join(process.env.PAPERCLIP_HOME!, "instances", "default", "keep.txt"); fs.mkdirSync(path.dirname(userData), { recursive: true }); fs.writeFileSync(userData, "keep"); - await uninstallCommand(); + const uninstallService = vi.fn(async () => { + expect(fs.existsSync(paths.shimPath)).toBe(true); + }); + await uninstallCommand({ + detectServiceManager: vi.fn(async () => ({ + supported: true as const, + manager: { + status: vi.fn(async () => ({ installed: true, active: true })), + uninstall: uninstallService, + } as never, + })), + }); + expect(uninstallService).toHaveBeenCalledOnce(); expect(fs.existsSync(paths.cliRoot)).toBe(false); expect(fs.existsSync(paths.shimPath)).toBe(false); expect(fs.readFileSync(userData, "utf8")).toBe("keep"); diff --git a/cli/src/__tests__/service-health-check.test.ts b/cli/src/__tests__/service-health-check.test.ts index 0703e3e3f07..f765373279b 100644 --- a/cli/src/__tests__/service-health-check.test.ts +++ b/cli/src/__tests__/service-health-check.test.ts @@ -5,6 +5,7 @@ import { describe, expect, it, vi } from "vitest"; import { serviceHealthChecks } from "../checks/service-health-check.js"; import { resolveRestartExpectedVersion } from "../commands/service.js"; import type { PaperclipConfig } from "../config/schema.js"; +import { buildLocalHealthUrl } from "../utils/health-url.js"; import { packageVersion } from "../version.js"; const config = { @@ -40,11 +41,16 @@ function managerFixture(active = true) { } describe("service health doctor checks", () => { - it("uses the bare package version when restart health has no prior server version", () => { + it("uses the current CLI version unless an expected restart version is explicit", () => { expect(resolveRestartExpectedVersion(null)).toBe(packageVersion); expect(resolveRestartExpectedVersion("1.2.3")).toBe("1.2.3"); }); + it("brackets configured IPv6 hosts in health URLs", () => { + expect(buildLocalHealthUrl("::1", 3100)).toBe("http://[::1]:3100/api/health"); + expect(buildLocalHealthUrl("::", 3100)).toBe("http://127.0.0.1:3100/api/health"); + }); + it("passes for a current, active, healthy service", async () => { const manager = managerFixture(); const results = await serviceHealthChecks(config, { diff --git a/cli/src/__tests__/service-manager.test.ts b/cli/src/__tests__/service-manager.test.ts index 1d0a0bafbbd..37775609072 100644 --- a/cli/src/__tests__/service-manager.test.ts +++ b/cli/src/__tests__/service-manager.test.ts @@ -104,6 +104,20 @@ describe("launchd lifecycle", () => { expect(calls).toContain(`launchctl bootout gui/${process.getuid?.() ?? 0}/ing.paperclip.paperclipai.team-a`); expect(calls.some((call) => call.includes("launchctl kill"))).toBe(false); }); + + it("disables login startup when uninstalled", async () => { + const userHome = await temporaryDirectory(); + const calls: string[] = []; + const runner: CommandRunner = async (command, args) => { + calls.push([command, ...args].join(" ")); + return { stdout: "", stderr: "" }; + }; + const manager = new LaunchdServiceManager("team-a", runner, path.join(userHome, ".paperclip"), path.join(userHome, ".local/bin/paperclipai"), userHome); + + await manager.uninstall(); + + expect(calls).toContain(`launchctl disable gui/${process.getuid?.() ?? 0}/ing.paperclip.paperclipai.team-a`); + }); }); describe("single-writer guard", () => { diff --git a/cli/src/checks/service-health-check.ts b/cli/src/checks/service-health-check.ts index b8fa905e9b7..47d62586a77 100644 --- a/cli/src/checks/service-health-check.ts +++ b/cli/src/checks/service-health-check.ts @@ -6,6 +6,7 @@ import { detectServiceManager, type ServiceManagerDetection, } from "../services/service-manager.js"; +import { buildLocalHealthUrl } from "../utils/health-url.js"; import type { CheckResult } from "./index.js"; type HealthResult = { ok: boolean; version: string | null; error?: string }; @@ -14,17 +15,11 @@ type ServiceCheckDependencies = { probe: (config: PaperclipConfig) => Promise; }; -function healthUrl(config: PaperclipConfig): string { - const configuredHost = config.server.host?.trim(); - const host = !configuredHost || configuredHost === "0.0.0.0" || configuredHost === "::" - ? "127.0.0.1" - : configuredHost; - return `http://${host}:${config.server.port}/api/health`; -} - async function probeHealth(config: PaperclipConfig): Promise { try { - const response = await fetch(healthUrl(config), { signal: AbortSignal.timeout(2_000) }); + const response = await fetch(buildLocalHealthUrl(config.server.host, config.server.port), { + signal: AbortSignal.timeout(2_000), + }); const body = (await response.json()) as { status?: unknown; serverVersion?: unknown; diff --git a/cli/src/commands/service.ts b/cli/src/commands/service.ts index e9312b4f8e6..c2100bc258f 100644 --- a/cli/src/commands/service.ts +++ b/cli/src/commands/service.ts @@ -6,6 +6,7 @@ import { packageVersion } from "../version.js"; import { readConfig, resolveConfigPath } from "../config/store.js"; import { resolvePaperclipHomeDir, resolvePaperclipInstanceId } from "../config/home.js"; import { detectServiceManager, type ServiceManager, type ServiceStatus } from "../services/service-manager.js"; +import { buildLocalHealthUrl } from "../utils/health-url.js"; type CommonOptions = { instance?: string; json?: boolean }; type HealthResult = { ok: boolean; serverVersion: string | null; error?: string }; @@ -26,10 +27,7 @@ async function resolveManager(opts: CommonOptions): Promise { @@ -53,11 +51,11 @@ async function waitForHealth(instanceId: string, expectedVersion: string | null, throw new Error(`Paperclip service did not become healthy${expectedVersion ? ` at version ${expectedVersion}` : ""}: ${last.error ?? `reported ${last.serverVersion ?? "no version"}`}`); } -export function resolveRestartExpectedVersion(serverVersion: string | null): string { - return serverVersion ?? packageVersion; +export function resolveRestartExpectedVersion(expectedVersion: string | null | undefined): string { + return expectedVersion ?? packageVersion; } -async function writeHotRestartIntent(status: ServiceStatus, instanceId: string, drainRequired: boolean): Promise<{ requestedAt: string; previousVersion: string }> { +async function writeHotRestartIntent(status: ServiceStatus, instanceId: string, drainRequired: boolean): Promise<{ requestedAt: string }> { if (!status.pid) throw new Error(`Cannot restart ${status.serviceName}: supervisor did not report a server pid.`); const health = await probeHealth(instanceId); const homeDir = resolvePaperclipHomeDir(); @@ -72,7 +70,7 @@ async function writeHotRestartIntent(status: ServiceStatus, instanceId: string, drainRequired, requestedByRunId: process.env.PAPERCLIP_RUN_ID?.trim() || null, }, null, 2)}\n`, "utf8"); - return { requestedAt, previousVersion: resolveRestartExpectedVersion(health.serverVersion) }; + return { requestedAt }; } async function waitForRestartReport(requestedAt: string, timeoutMs = 10_000): Promise { @@ -97,7 +95,7 @@ export async function restartManagedService(input: { instanceId?: string; expect const before = await detection.manager.status(); const intent = await writeHotRestartIntent(before, instanceId, input.waitForDrain ?? false); await detection.manager.restart(); - const health = await waitForHealth(instanceId, input.expectedVersion ?? intent.previousVersion); + const health = await waitForHealth(instanceId, resolveRestartExpectedVersion(input.expectedVersion)); return { status: await detection.manager.status(), health, report: await waitForRestartReport(intent.requestedAt) }; } diff --git a/cli/src/commands/uninstall.ts b/cli/src/commands/uninstall.ts index 4677a363c21..f624d4617c7 100644 --- a/cli/src/commands/uninstall.ts +++ b/cli/src/commands/uninstall.ts @@ -6,8 +6,23 @@ import { removeManagedShim, resolveInstallStorePaths, } from "../install-store.js"; +import { resolvePaperclipInstanceId } from "../config/home.js"; +import { detectServiceManager } from "../services/service-manager.js"; + +type UninstallDependencies = { + detectServiceManager: typeof detectServiceManager; +}; + +export async function uninstallCommand( + dependencies: Partial = {}, +): Promise { + const detect = dependencies.detectServiceManager ?? detectServiceManager; + const detection = await detect({ instanceId: resolvePaperclipInstanceId() }); + if (detection.supported) { + const status = await detection.manager.status(); + if (status.installed || status.active) await detection.manager.uninstall(); + } -export async function uninstallCommand(): Promise { const paths = resolveInstallStorePaths(); const shimRemoved = removeManagedShim(paths); fs.rmSync(paths.cliRoot, { recursive: true, force: true }); diff --git a/cli/src/services/service-manager.ts b/cli/src/services/service-manager.ts index c20f80efbb4..7b010832305 100644 --- a/cli/src/services/service-manager.ts +++ b/cli/src/services/service-manager.ts @@ -230,7 +230,7 @@ export class LaunchdServiceManager implements ServiceManager { async uninstall(): Promise { await this.runner("launchctl", ["bootout", `${this.domain}/${this.serviceName}`]).catch(() => undefined); - await this.runner("launchctl", ["enable", `${this.domain}/${this.serviceName}`]).catch(() => undefined); + await this.runner("launchctl", ["disable", `${this.domain}/${this.serviceName}`]).catch(() => undefined); await fs.rm(this.definitionPath, { force: true }); } async start(): Promise { await this.install({ startNow: true, startOnLogin: true }); } diff --git a/cli/src/utils/health-url.ts b/cli/src/utils/health-url.ts new file mode 100644 index 00000000000..12f20312ffa --- /dev/null +++ b/cli/src/utils/health-url.ts @@ -0,0 +1,10 @@ +export function buildLocalHealthUrl(host: string | undefined, port: number): string { + const configuredHost = host?.trim(); + const reachableHost = !configuredHost || configuredHost === "0.0.0.0" || configuredHost === "::" + ? "127.0.0.1" + : configuredHost; + const urlHost = reachableHost.includes(":") && !reachableHost.startsWith("[") + ? `[${reachableHost}]` + : reachableHost; + return `http://${urlHost}:${port}/api/health`; +}