diff --git a/docs-site/src/content/docs/reference/cli/lifecycle.md b/docs-site/src/content/docs/reference/cli/lifecycle.md index 930368c91c..9ba0650063 100644 --- a/docs-site/src/content/docs/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/reference/cli/lifecycle.md @@ -231,6 +231,17 @@ interrupted package update removed either file, it logs one `installation is inc stops instead of retrying the same missing executable every five seconds. Reinstall opencodex, then run `ocx service repair` to refresh the task with the restored package paths. +On Linux, the systemd unit invokes the stable `ocx` executable found on `PATH` at install time +rather than the Bun and CLI paths inside the installed package tree. Version managers such as +**mise** and **asdf** install into a versioned directory and delete the old one on upgrade, which +used to leave the unit pointing at files that no longer existed — systemd then restart-looped while +still reporting the service as installed. A shim path survives the upgrade, so the unit keeps +resolving. Source checkouts without an `ocx` launcher keep the previous direct Bun + CLI form. + +Units installed before this change still carry the old versioned paths and cannot migrate +themselves — once the old executable is deleted, no opencodex code runs to fix it. Run +`ocx service repair` once after upgrading; subsequent version changes need no action. + | Subcommand | Action | | --- | --- | | none | Install and start when absent; otherwise refresh and restart the existing service without re-registering it. | diff --git a/src/service.ts b/src/service.ts index 4c4967d6de..c9e79873fb 100644 --- a/src/service.ts +++ b/src/service.ts @@ -9,7 +9,7 @@ import { execFileSync, execSync, spawnSync } from "node:child_process"; import { findLiveProxy, proxyIdentityAt, SERVICE_STOP_LIVENESS } from "./server/proxy-liveness"; import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmdirSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; -import { dirname, join, posix, resolve, win32 } from "node:path"; +import { dirname, isAbsolute, join, posix, resolve, win32 } from "node:path"; import { expandUserPath, getConfigDir, loadConfig } from "./config"; import { readPid, removePid, removeRuntimePort, verifyPidIdentity } from "./config/process-state"; import { restoreNativeCodex, restoreNativeCodexAsync } from "./codex/inject"; @@ -67,6 +67,37 @@ function cliEntry(): { bun: string; bunRuntimeSource: BunRuntimeSource; cli: str return { bun: runtime.path, bunRuntimeSource: runtime.source, cli: join(import.meta.dir, "cli", "index.ts") }; } +/** + * The stable `ocx` launcher to bake into a systemd unit, or null to fall back to the + * Bun + CLI pair. + * + * `cliEntry()` resolves both of its paths from `import.meta.dir`, so they point INSIDE + * the installed package tree. Under a version manager that tree is a versioned directory: + * `~/.local/share/mise/installs/npm-opencodex/2.35.0/...`. An upgrade installs 2.36.0 and + * deletes 2.35.0, after which the unit's `exec ` cannot resolve, and + * `Restart=on-failure` turns that into a restart loop (#2898). The shim in + * `~/.local/share/mise/shims/ocx` survives the upgrade and dispatches to whatever version + * is current, so it is the durable thing to name. + * + * Deliberately LEXICAL. Resolving the symlink would write the versioned target back into + * the unit and reintroduce the bug — the indirection is the entire point. + * + * Only an absolute path is accepted. A bare `ocx` would be re-resolved through `PATH` on + * every restart, which turns a service definition into a PATH-hijacking surface; naming + * one validated absolute file keeps the target fixed at install time. + */ +function stableLauncherEntry(deps: { env?: NodeJS.ProcessEnv; exists?: (path: string) => boolean } = {}): string | null { + const env = deps.env ?? process.env; + const exists = deps.exists ?? existsSync; + const entries = (env.PATH ?? "").split(":"); + for (const entry of entries) { + if (!entry || !isAbsolute(entry)) continue; + const candidate = join(entry, "ocx"); + if (exists(candidate)) return candidate; + } + return null; +} + function plistPath(): string { return join(homedir(), "Library", "LaunchAgents", `${LABEL}.plist`); } @@ -155,6 +186,14 @@ export interface ServiceInstallState { /** Baked at install; lets status flag paths gone stale after npm prefix/nvm moves. */ bunPath?: string; cliPath?: string; + /** + * Linux only. The stable `ocx` launcher the unit actually invokes, when one was found. + * Present means `bunPath`/`cliPath` are provenance for the install, NOT what systemd + * runs — so staleness must be judged against THIS path instead. A version-manager + * upgrade replaces the directory those two point into while the launcher survives, and + * checking the old pair would report a stale service that is in fact healthy. + */ + launcherPath?: string; /** v2: which Windows backend was chosen at install; absent (v1/legacy) means scheduler. */ backend?: ServiceBackend; winswVersion?: string; @@ -167,7 +206,7 @@ export function parseServiceInstallState(value: unknown): ServiceInstallState | if (state.version !== 1 && state.version !== 2) return null; if (typeof state.codexHome !== "string" || state.codexHome.length === 0) return null; if (typeof state.opencodexHome !== "string" || state.opencodexHome.length === 0) return null; - for (const key of ["bunPath", "cliPath", "winswVersion", "winswSha256"] as const) { + for (const key of ["bunPath", "cliPath", "launcherPath", "winswVersion", "winswSha256"] as const) { if (state[key] !== undefined && (typeof state[key] !== "string" || state[key].length === 0)) return null; } if (state.version === 1) { @@ -178,7 +217,7 @@ export function parseServiceInstallState(value: unknown): ServiceInstallState | return state as unknown as ServiceInstallState; } -function writeServiceInstallState(backend: ServiceBackend = "scheduler"): void { +function writeServiceInstallState(backend: ServiceBackend = "scheduler", launcherPath?: string | null): void { const { bun, cli } = cliEntry(); const state: ServiceInstallState = { version: 2, @@ -186,6 +225,7 @@ function writeServiceInstallState(backend: ServiceBackend = "scheduler"): void { opencodexHome: currentOpenCodexHome(), bunPath: bun, cliPath: cli, + ...(launcherPath ? { launcherPath } : {}), backend, ...(backend === "native" ? { winswVersion: WINSW_VERSION, winswSha256: WINSW_SHA256 } : {}), }; @@ -501,6 +541,17 @@ function buildServiceShellCommand(bun: string, cli: string, port = resolveServic return `if [ -f ${shellQuote(tokenFile)} ]; then OPENCODEX_API_AUTH_TOKEN="$(cat ${shellQuote(tokenFile)})"; export OPENCODEX_API_AUTH_TOKEN; fi; exec ${shellQuote(bun)} ${shellQuote(cli)} start --port ${port}`; } +/** + * The same command shape, launched through a stable `ocx` executable instead of an + * explicit Bun + CLI pair. The token-file preamble is identical and deliberately shared + * in form: the service still reads the token from disk at start and never carries it in + * the unit. + */ +function buildServiceLauncherShellCommand(launcher: string, port = resolveServiceListenPort()): string { + const tokenFile = serviceApiTokenFilePath(); + return `if [ -f ${shellQuote(tokenFile)} ]; then OPENCODEX_API_AUTH_TOKEN="$(cat ${shellQuote(tokenFile)})"; export OPENCODEX_API_AUTH_TOKEN; fi; exec ${shellQuote(launcher)} start --port ${port}`; +} + /** * The `--port ` actually baked into the installed launchd plist, or null when it * cannot be read. macOS only — named for launchd rather than "service" so no caller @@ -2507,6 +2558,14 @@ function uninstallWindows(): void { */ export function bakedServicePathsDiagnostic(): string | null { const state = readServiceInstallState(); + // A launcher install runs the launcher, not the baked pair, so the pair's existence says + // nothing about whether the service can start. Judging the recorded launcher is both + // necessary (a deleted launcher IS stale) and sufficient (a replaced version directory + // is not, which is exactly what #2898 made routine). + if (state?.launcherPath) { + if (existsSync(state.launcherPath)) return null; + return `STALE baked paths (missing: ${state.launcherPath}) — run 'ocx service repair' to re-bake`; + } if (!state?.bunPath || !state?.cliPath) return null; const missing = [state.bunPath, state.cliPath].filter(path => !existsSync(path)); if (missing.length === 0) return null; @@ -2527,8 +2586,15 @@ function unitPath(): string { return join(unitDir(), `${TASK}.service`); } -export function buildUnit(proxyEnv: { name: string; value: string }[] = resolvedProxyEnv()): string { +export function buildUnit( + proxyEnv: { name: string; value: string }[] = resolvedProxyEnv(), + deps: { launcher?: string | null } = {}, +): string { const { bun, bunRuntimeSource, cli } = cliEntry(); + // A stable launcher replaces the versioned pair entirely: baking OCX_BUN_RUNTIME_PATH + // alongside it would pin the runtime to the directory the upgrade deletes, which is the + // defect being fixed. The launcher resolves the current package's Bun itself. + const launcher = deps.launcher !== undefined ? deps.launcher : stableLauncherEntry(); const log = logPath(); const path = process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin"; const codexHome = systemdEnvironmentAssignment("CODEX_HOME", process.env.CODEX_HOME?.trim()); @@ -2536,15 +2602,17 @@ export function buildUnit(proxyEnv: { name: string; value: string }[] = resolved const opencodexHome = systemdEnvironmentAssignment("OPENCODEX_HOME", process.env.OPENCODEX_HOME?.trim()); const envLines = [ systemdEnvironmentAssignment("OCX_SERVICE", "1"), - systemdEnvironmentAssignment(BUN_RUNTIME_SOURCE_ENV, bunRuntimeSource), - systemdEnvironmentAssignment(BUN_RUNTIME_PATH_ENV, bun), + ...(launcher ? [] : [ + systemdEnvironmentAssignment(BUN_RUNTIME_SOURCE_ENV, bunRuntimeSource), + systemdEnvironmentAssignment(BUN_RUNTIME_PATH_ENV, bun), + ]), systemdEnvironmentAssignment("PATH", path), codexHome, codexSqliteHome, opencodexHome, ...proxyEnv.map(({ name, value }) => systemdEnvironmentAssignment(name, value)), ].filter((line): line is string => Boolean(line)).join("\n"); - const command = `${buildServiceShellCommand(bun, cli)} >> ${shellQuote(log)} 2>&1`; + const command = `${launcher ? buildServiceLauncherShellCommand(launcher) : buildServiceShellCommand(bun, cli)} >> ${shellQuote(log)} 2>&1`; return `[Unit] Description=OpenCodex Proxy Server After=network-online.target @@ -2602,11 +2670,14 @@ function installSystemd(): void { recordOwnedConfigPath(getConfigDir(), serviceStatePath()); if (!existsSync(getConfigDir())) mkdirSync(getConfigDir(), { recursive: true }); writeServiceApiTokenFile(); - writeServiceDefinitionFile(unitPath(), buildUnit(), "utf8"); + // Resolve ONCE and reuse: the unit and the install state must agree about what is + // launched, or the staleness check would validate a path the unit does not run. + const launcher = stableLauncherEntry(); + writeServiceDefinitionFile(unitPath(), buildUnit(resolvedProxyEnv(), { launcher }), "utf8"); sh("systemctl --user daemon-reload"); sh(`systemctl --user enable ${TASK}`); sh(`systemctl --user restart ${TASK}`); - writeServiceInstallState(); + writeServiceInstallState("scheduler", launcher); } /** * Whether systemd's in-memory unit differs from the file on disk. diff --git a/tests/service.test.ts b/tests/service.test.ts index 1144845815..04cd75519f 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, spyOn, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { execSync } from "node:child_process"; import { tmpdir } from "node:os"; import { isAbsolute, join, posix, win32 } from "node:path"; import * as serviceModule from "../src/service"; @@ -300,7 +301,7 @@ describe("systemd service unit", () => { // The write goes through writeServiceDefinitionFile so the unit lands 0600: it can carry a // proxy credential (#2107). What this test pins is the ORDER — write, then reload. - const writeAt = installSystemd.indexOf('writeServiceDefinitionFile(unitPath(), buildUnit(), "utf8")'); + const writeAt = installSystemd.indexOf("writeServiceDefinitionFile(unitPath(), buildUnit("); const reloadAt = installSystemd.indexOf("systemctl --user daemon-reload"); const enableAt = installSystemd.indexOf("systemctl --user enable"); const restartAt = installSystemd.indexOf("systemctl --user restart"); @@ -310,6 +311,15 @@ describe("systemd service unit", () => { expect(enableAt).toBeLessThan(restartAt); expect(installSystemd).not.toContain("ocx service install"); expect(installSystemd).not.toContain("process.exit(1)"); + + // #2898: the unit and the recorded install state must agree about WHAT is launched, so + // the launcher is resolved once and handed to both. Resolving twice would let the + // staleness check validate a path the unit does not run. + const resolveAt = installSystemd.indexOf("stableLauncherEntry()"); + expect(resolveAt).toBeGreaterThan(-1); + expect(resolveAt).toBeLessThan(writeAt); + expect(installSystemd).toContain("writeServiceInstallState(\"scheduler\", launcher)"); + expect(installSystemd.match(/stableLauncherEntry\(\)/g)).toHaveLength(1); }); }); @@ -798,7 +808,14 @@ describe("launchd service plist", () => { const trustedPlist = buildPlist(); expect(trustedPlist).toContain("OCX_BUN_RUNTIME_SOURCEoverride"); expectTextToContainPath(trustedPlist, process.execPath); - expect(buildUnit()).toContain('Environment="OCX_BUN_RUNTIME_SOURCE=override"'); + // The systemd unit stamps the pair only when it BAKES that pair. A stable-launcher + // install runs `ocx` and lets it resolve the current package's Bun, so stamping a + // path there would pin the runtime to the directory a version upgrade deletes + // (#2898) — the opposite of what #848 asks for. Assert both modes explicitly. + expect(buildUnit(resolvedProxyEnv(), { launcher: null })).toContain('Environment="OCX_BUN_RUNTIME_SOURCE=override"'); + const launched = buildUnit(resolvedProxyEnv(), { launcher: "/opt/shims/ocx" }); + expect(launched).not.toContain("OCX_BUN_RUNTIME_SOURCE"); + expect(launched).not.toContain("OCX_BUN_RUNTIME_PATH"); expect(buildWindowsServiceScript()).toContain('set "OCX_BUN_RUNTIME_SOURCE=override"'); } finally { if (inheritedOverride === undefined) delete process.env.OPENCODEX_BUN_PATH; @@ -862,6 +879,62 @@ describe("launchd service plist", () => { } }); + // #2898. A version manager installs OpenCodex under a versioned directory and deletes the + // old one on upgrade; the baked Bun and CLI both live there. The shim does not move, so the + // unit has to name the shim and nothing from inside the version directory. + test("a stable launcher install names the launcher and bakes no versioned path", () => { + const launcher = "/home/u/.local/share/mise/shims/ocx"; + const unit = buildUnit(resolvedProxyEnv({}), { launcher }); + + expect(unit).toContain(launcher); + expect(unit).toContain("start --port"); + // The versioned pair must be absent from BOTH the command and the environment: either one + // pins the service to a directory the next upgrade removes. + expect(unit).not.toContain("OCX_BUN_RUNTIME_PATH"); + expect(unit).not.toContain("OCX_BUN_RUNTIME_SOURCE"); + expect(unit).not.toContain("cli/index.ts"); + // The token still comes from the file at start, never from the unit (#2107). + expectTextToContainPath(unit, serviceApiTokenFilePath()); + expect(unit).toContain("OPENCODEX_API_AUTH_TOKEN"); + + // Without a launcher the unit keeps the previous shape, so source checkouts are unaffected. + const direct = buildUnit(resolvedProxyEnv({}), { launcher: null }); + expect(direct).toContain("cli/index.ts"); + expect(direct).toContain("OCX_BUN_RUNTIME_PATH"); + }); + + // The scenario itself, executed rather than asserted: retarget the shim the way an upgrade + // does, delete the old version, and check the generated command still reaches live code. + test("the generated launcher command follows a retargeted shim after the old version is gone", () => { + const root = mkdtempSync(join(tmpdir(), "ocx-shim-")); + const shimDir = join(root, "shims"); + const v1 = join(root, "installs", "2.35.0"); + const v2 = join(root, "installs", "2.36.0"); + mkdirSync(shimDir, { recursive: true }); + mkdirSync(v1, { recursive: true }); + mkdirSync(v2, { recursive: true }); + writeFileSync(join(v1, "ocx"), "#!/bin/sh\necho V1 \"$@\"\n", { mode: 0o755 }); + writeFileSync(join(v2, "ocx"), "#!/bin/sh\necho V2 \"$@\"\n", { mode: 0o755 }); + + const shim = join(shimDir, "ocx"); + writeFileSync(shim, `#!/bin/sh\nexec ${join(v1, "ocx")} "\$@"\n`, { mode: 0o755 }); + + // stableLauncherEntry finds the shim lexically from PATH — not its versioned target. + const found = buildUnit(resolvedProxyEnv({}), { launcher: shim }); + expect(found).toContain(shim); + expect(found).not.toContain(v1); + + expect(execSync(`sh -c ${JSON.stringify(`${shim} start --port 1`)}`, { encoding: "utf8" })).toContain("V1"); + + // The upgrade: shim retargeted, old version removed. + writeFileSync(shim, `#!/bin/sh\nexec ${join(v2, "ocx")} "\$@"\n`, { mode: 0o755 }); + rmSync(v1, { recursive: true, force: true }); + expect(existsSync(join(v1, "ocx"))).toBe(false); + expect(execSync(`sh -c ${JSON.stringify(`${shim} start --port 1`)}`, { encoding: "utf8" })).toContain("V2"); + + rmSync(root, { recursive: true, force: true }); + }); + // The relative case is why the resolve() is there at all: a service unit has no meaningful // working directory, so a relative home must still be made absolute. test("still absolutizes a relative sqlite home", () => { @@ -1816,6 +1889,54 @@ describe("service diagnostics", () => { } }); + // #2898: a version manager (mise, asdf) installs OpenCodex into a VERSIONED directory and + // deletes the old one on upgrade. The baked Bun and CLI both live in that directory, so the + // unit's `exec ` stops resolving and Restart=on-failure restart-loops. + // When the install went through a stable launcher, the launcher is what systemd runs, so it + // is the only path whose absence means anything — and the replaced version directory must + // NOT be reported as stale. + test("a launcher install judges staleness by the launcher, not the replaced version dir", () => { + const oldOpenCodexHome = process.env.OPENCODEX_HOME; + const stateDir = join(TEST_DIR, "launcher-paths-home"); + try { + process.env.OPENCODEX_HOME = stateDir; + mkdirSync(stateDir, { recursive: true }); + const statePath = join(stateDir, "service-state.json"); + const launcher = join(import.meta.dir, "service.test.ts"); + const removedVersionDir = join(stateDir, "installs", "2.35.0"); + + // The upgrade case: version directory gone, launcher intact. Healthy. + writeFileSync(statePath, JSON.stringify({ + version: 2, + codexHome: stateDir, + opencodexHome: stateDir, + bunPath: join(removedVersionDir, "bun"), + cliPath: join(removedVersionDir, "cli", "index.ts"), + launcherPath: launcher, + backend: "scheduler", + }), "utf8"); + expect(bakedServicePathsDiagnostic()).toBeNull(); + + // A launcher that is itself gone is genuinely stale, and names the launcher. + const missingLauncher = join(stateDir, "shims", "ocx"); + writeFileSync(statePath, JSON.stringify({ + version: 2, + codexHome: stateDir, + opencodexHome: stateDir, + bunPath: join(import.meta.dir, "service.test.ts"), + cliPath: join(import.meta.dir, "service.test.ts"), + launcherPath: missingLauncher, + backend: "scheduler", + }), "utf8"); + const diagnostic = bakedServicePathsDiagnostic(); + expect(diagnostic).toContain("STALE baked paths"); + expect(diagnostic).toContain(missingLauncher); + } finally { + if (oldOpenCodexHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = oldOpenCodexHome; + } + }); + test("direct service status prints the diagnostics line", async () => { const service = await readText("src/service.ts"); const statusCase = service.slice(service.indexOf('case "status":'), service.indexOf('case "uninstall":'));