From ba63b4605f2fdca8b9fc4e91fac6f13e9f6baa56 Mon Sep 17 00:00:00 2001 From: lidge-jun Date: Sat, 29 Aug 2026 22:45:03 +0900 Subject: [PATCH] fix(service): launch the systemd unit through the stable ocx executable A version manager installs OpenCodex into a versioned directory and deletes the old one on upgrade. Both paths baked into the systemd unit - the bundled Bun and the CLI entry - are computed from import.meta.dir and therefore live inside that directory, so after a mise upgrade /bin/sh still starts but "exec " cannot resolve, and Restart=on-failure turns it into a restart loop while systemd still reports the unit as installed. installSystemd now resolves the stable ocx executable lexically from PATH and bakes that instead. A mise/asdf/npm shim survives the upgrade and dispatches to whatever version is current. The resolution is deliberately lexical: resolving the symlink would write the versioned target back into the unit and recreate the bug. Only an absolute path is accepted, so the unit never re-resolves a bare name through PATH at restart. Source checkouts with no launcher keep the previous direct Bun + CLI form. OCX_BUN_RUNTIME_PATH and OCX_BUN_RUNTIME_SOURCE are omitted in launcher mode - stamping them would pin the runtime to the directory the upgrade deletes, which is the defect. The #848 provenance invariant still holds where the pair is actually baked, and the test now asserts both modes explicitly. ServiceInstallState records launcherPath, and bakedServicePathsDiagnostic judges staleness against the launcher when one is present. Without that, a healthy service was reported STALE after every upgrade, since the recorded pair pointed into the replaced directory. State files lacking the field are unaffected. The launcher and the state are resolved once and shared, so the diagnostic can never validate a path the unit does not run. --- .../content/docs/reference/cli/lifecycle.md | 11 ++ src/service.ts | 89 +++++++++++-- tests/service.test.ts | 125 +++++++++++++++++- 3 files changed, 214 insertions(+), 11 deletions(-) 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":'));