From ad6ac8bbe47c0261c3a50362d4b2722f2c8a5a84 Mon Sep 17 00:00:00 2001 From: Ingwannu Date: Sat, 29 Aug 2026 14:35:54 +0000 Subject: [PATCH] fix(service): harden stable systemd launcher contracts --- .../content/docs/reference/cli/lifecycle.md | 8 ++- src/service.ts | 48 +++++++++----- structure/04_transports-and-sidecars.md | 22 +++++++ tests/service.test.ts | 63 ++++++++++++++++--- 4 files changed, 113 insertions(+), 28 deletions(-) diff --git a/docs-site/src/content/docs/reference/cli/lifecycle.md b/docs-site/src/content/docs/reference/cli/lifecycle.md index 9ba0650063..97182382f9 100644 --- a/docs-site/src/content/docs/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/reference/cli/lifecycle.md @@ -231,12 +231,14 @@ 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 +On Linux, the systemd unit invokes the first regular, executable `ocx` file 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. +resolving. Source checkouts without an `ocx` launcher keep the previous direct Bun + CLI form. A +trusted `OPENCODEX_BUN_PATH` selected before Bun starts is preserved through the shim; package-local +bundled Bun paths are deliberately rediscovered after upgrades instead of being pinned in the unit. 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 diff --git a/src/service.ts b/src/service.ts index c9e79873fb..4dfb0de6d7 100644 --- a/src/service.ts +++ b/src/service.ts @@ -7,16 +7,16 @@ */ 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 { accessSync, chmodSync, constants as fsConstants, existsSync, mkdirSync, mkdtempSync, readFileSync, rmdirSync, statSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; -import { dirname, isAbsolute, join, posix, resolve, win32 } from "node:path"; +import { delimiter, 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"; import { stripGrokConfig } from "./grok/inject"; import { isWslRuntime, resolveCodexHomeDir, type CodexHomeDeps } from "./codex/home"; import { BUN_RUNTIME_PATH_ENV, BUN_RUNTIME_SOURCE_ENV, durableBunRuntime } from "./lib/bun-runtime"; -import type { BunRuntimeSource } from "./lib/bun-runtime"; +import type { BunRuntimeSource, DurableBunRuntime } from "./lib/bun-runtime"; import { isProcessAlive, stopProxy } from "./lib/process-control"; import { serviceApiTokenFilePath } from "./lib/service-secrets"; import { tokenCollidesWithAdmin } from "./lib/admin-secrets"; @@ -56,14 +56,13 @@ const TASK = "opencodex-proxy"; export type ServiceBackend = "scheduler" | "native"; -function cliEntry(): { bun: string; bunRuntimeSource: BunRuntimeSource; cli: string } { +function cliEntry(runtime: DurableBunRuntime = durableBunRuntime()): { bun: string; bunRuntimeSource: BunRuntimeSource; cli: string } { // Bake the bundled Bun (npm global prefix, survives `ocx update`) rather than // a transient system Bun, so launchd/systemd/schtasks keep resolving even if a // standalone Bun is later removed. The CLI entry lives at src/cli/index.ts. // // Path and provenance come from ONE resolution so the marker can never describe a // different binary than the one actually baked. - const runtime = durableBunRuntime(); return { bun: runtime.path, bunRuntimeSource: runtime.source, cli: join(import.meta.dir, "cli", "index.ts") }; } @@ -86,14 +85,26 @@ function cliEntry(): { bun: string; bunRuntimeSource: BunRuntimeSource; cli: str * 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 { +export function stableLauncherEntry(deps: { + env?: NodeJS.ProcessEnv; + isExecutableFile?: (path: string) => boolean; + pathDelimiter?: string; +} = {}): string | null { const env = deps.env ?? process.env; - const exists = deps.exists ?? existsSync; - const entries = (env.PATH ?? "").split(":"); + const isExecutableFile = deps.isExecutableFile ?? ((path: string): boolean => { + try { + if (!statSync(path).isFile()) return false; + accessSync(path, fsConstants.X_OK); + return true; + } catch { + return false; + } + }); + const entries = (env.PATH ?? "").split(deps.pathDelimiter ?? delimiter); for (const entry of entries) { if (!entry || !isAbsolute(entry)) continue; const candidate = join(entry, "ocx"); - if (exists(candidate)) return candidate; + if (isExecutableFile(candidate)) return candidate; } return null; } @@ -2588,13 +2599,14 @@ function unitPath(): string { export function buildUnit( proxyEnv: { name: string; value: string }[] = resolvedProxyEnv(), - deps: { launcher?: string | null } = {}, + deps: { launcher?: string | null; runtime?: DurableBunRuntime } = {}, ): 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 runtime = deps.runtime ?? durableBunRuntime(); + const { bun, bunRuntimeSource, cli } = cliEntry(runtime); + // Discovery belongs to installSystemd(), which resolves once and passes the same value to + // both the unit and install state. Keeping this builder explicit makes tests and diagnostics + // independent of the host PATH. + const launcher = deps.launcher ?? null; const log = logPath(); const path = process.env.PATH ?? "/usr/local/bin:/usr/bin:/bin"; const codexHome = systemdEnvironmentAssignment("CODEX_HOME", process.env.CODEX_HOME?.trim()); @@ -2606,6 +2618,12 @@ export function buildUnit( systemdEnvironmentAssignment(BUN_RUNTIME_SOURCE_ENV, bunRuntimeSource), systemdEnvironmentAssignment(BUN_RUNTIME_PATH_ENV, bun), ]), + // A launcher normally resolves the current package's bundled Bun after every upgrade. + // Preserve only a proof-bound shell override; otherwise writing a package-local path here + // would recreate the version-manager pin that the launcher mode exists to remove. + launcher && runtime.source === "override" + ? systemdEnvironmentAssignment(runtime.overrideEnv, runtime.path) + : null, systemdEnvironmentAssignment("PATH", path), codexHome, codexSqliteHome, diff --git a/structure/04_transports-and-sidecars.md b/structure/04_transports-and-sidecars.md index 150f7b781a..eff93add16 100644 --- a/structure/04_transports-and-sidecars.md +++ b/structure/04_transports-and-sidecars.md @@ -17,6 +17,28 @@ existing task. Explicit `ocx service install` remains the operator-owned registr - 다른 대안 대신 이 방식을 선택한 이유: Saved state can be stale and unconditional repair breaks first install, while a boolean cannot represent the exact uncertainty that must fail closed. - 장점, 단점 및 영향: Existing services avoid UAC and registration churn, invalid input performs no status I/O, and uncertain Windows hosts require one explicit status/installation decision instead of risking a destructive guess. +## Linux stable service launcher + +Systemd installation resolves the first absolute `ocx` PATH candidate that is both a regular file +and executable, keeps that path lexical so a version-manager shim remains an indirection, and +records the same single resolution in the unit and service state. Unit construction never performs +PATH discovery itself: callers provide either the resolved launcher or an explicit direct Bun/CLI +fallback, keeping diagnostics and tests independent of the host PATH. + +Launcher mode omits the package-local Bun provenance pair because an upgrade may delete that +versioned tree. The only runtime path carried through the launcher is a pre-Bun, proof-bound +`OPENCODEX_BUN_PATH` whose durable runtime source is `override`; bundled and process fallbacks are +rediscovered by the current launcher. The API-auth token remains file-backed and is loaded only by +the service shell at start. + +[Decision Log] +- 목적과 의도: Keep systemd services upgrade-stable without losing an explicitly trusted Bun override or accepting a non-executable PATH placeholder. +- 기존 구현 및 제약 조건: Version managers replace package trees but retain lexical shims; Bun dotenv makes ambient override values untrustworthy unless the Node launcher already stamped matching runtime provenance. +- 검토한 주요 대안: Bake the package Bun and CLI forever; resolve the shim target; accept the first existing PATH entry; drop every runtime override in launcher mode; or preserve only a proof-bound override. +- 선택한 방식: Require a regular executable lexical launcher, resolve it once during installation, preserve only `durableBunRuntime().source === "override"`, and keep token loading in the existing file-backed shell preamble. +- 다른 대안 대신 이 방식을 선택한 이유: Resolving or pinning package paths recreates upgrade restart loops, existence-only selection can name a directory or non-executable file, and dropping a trusted override silently changes an operator's runtime. +- 장점, 단점 및 영향: Mise/asdf-style upgrades keep working and explicit Bun selection survives; source installs still use the direct pair, while a removed or non-executable launcher requires `ocx service repair`. + ## Provider diagnostic outbound safety Provider connection tests and live model discovery share the GET-only provider outbound wrapper. diff --git a/tests/service.test.ts b/tests/service.test.ts index 04cd75519f..2d07f179c0 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -1,12 +1,12 @@ 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 { execFileSync } from "node:child_process"; import { tmpdir } from "node:os"; -import { isAbsolute, join, posix, win32 } from "node:path"; +import { delimiter, isAbsolute, join, posix, win32 } from "node:path"; import * as serviceModule from "../src/service"; import { saveConfig } from "../src/config"; import { windowsEnvIndirectBatchValue } from "../src/lib/win-paths"; -import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml, deriveWindowsServiceDiagnostic, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceArgs, parseServiceInstallState, planServiceCommand, prepareServiceInstall, probeServiceInstallation, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, resolveServiceListenPort, runLaunchctl, selectServiceSubcommand, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, systemdNeedsDaemonReload, systemdServiceInstallCleanupOps, uninstallSystemd, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy } from "../src/service"; +import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml, deriveWindowsServiceDiagnostic, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceArgs, parseServiceInstallState, planServiceCommand, prepareServiceInstall, probeServiceInstallation, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, resolveServiceListenPort, runLaunchctl, selectServiceSubcommand, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, stableLauncherEntry, systemdNeedsDaemonReload, systemdServiceInstallCleanupOps, uninstallSystemd, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy } from "../src/service"; import type { ServiceDiagnostic } from "../src/service"; import { definitionCarriesCredential, resolvedProxyEnv, writeServiceDefinitionFile } from "../src/service"; import { buildWinswXml } from "../src/lib/winsw"; @@ -98,6 +98,41 @@ describe("service listen-port bake", () => { }); describe("systemd service unit", () => { + test("stable launcher discovery skips invalid PATH candidates and keeps the lexical executable", () => { + const first = join(TEST_DIR, "first"); + const second = join(TEST_DIR, "second"); + const probes: string[] = []; + const result = stableLauncherEntry({ + env: { PATH: [first, second].join(delimiter) }, + isExecutableFile: candidate => { + probes.push(candidate); + return candidate === join(second, "ocx"); + }, + }); + + expect(probes).toEqual([join(first, "ocx"), join(second, "ocx")]); + expect(result).toBe(join(second, "ocx")); + }); + + test("stable launcher discovery requires a regular executable file", () => { + if (process.platform === "win32") return; + const root = mkdtempSync(join(tmpdir(), "ocx-launcher-path-")); + const directoryEntry = join(root, "directory-entry"); + const nonExecutableEntry = join(root, "non-executable-entry"); + const executableEntry = join(root, "executable-entry"); + for (const entry of [directoryEntry, nonExecutableEntry, executableEntry]) mkdirSync(entry); + mkdirSync(join(directoryEntry, "ocx")); + writeFileSync(join(nonExecutableEntry, "ocx"), "#!/bin/sh\nexit 0\n", { mode: 0o644 }); + writeFileSync(join(executableEntry, "ocx"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }); + try { + expect(stableLauncherEntry({ + env: { PATH: [directoryEntry, nonExecutableEntry, executableEntry].join(delimiter) }, + })).toBe(join(executableEntry, "ocx")); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + test("bare service installs only when absent and otherwise selects no-admin repair", async () => { expect(normalizeServiceSubcommand()).toBe("install"); expect(normalizeServiceSubcommand("restart")).toBe("repair"); @@ -816,6 +851,8 @@ describe("launchd service plist", () => { const launched = buildUnit(resolvedProxyEnv(), { launcher: "/opt/shims/ocx" }); expect(launched).not.toContain("OCX_BUN_RUNTIME_SOURCE"); expect(launched).not.toContain("OCX_BUN_RUNTIME_PATH"); + expectTextToContainPath(launched, process.execPath); + expect(launched).toContain("OPENCODEX_BUN_PATH="); expect(buildWindowsServiceScript()).toContain('set "OCX_BUN_RUNTIME_SOURCE=override"'); } finally { if (inheritedOverride === undefined) delete process.env.OPENCODEX_BUN_PATH; @@ -884,7 +921,10 @@ describe("launchd service plist", () => { // 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 }); + const unit = buildUnit(resolvedProxyEnv({}), { + launcher, + runtime: { path: "/opt/opencodex/versioned/bun", source: "bundled", overrideEnv: "OPENCODEX_BUN_PATH" }, + }); expect(unit).toContain(launcher); expect(unit).toContain("start --port"); @@ -892,6 +932,8 @@ describe("launchd service plist", () => { // 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("OPENCODEX_BUN_PATH"); + expect(unit).not.toContain("/opt/opencodex/versioned/bun"); expect(unit).not.toContain("cli/index.ts"); // The token still comes from the file at start, never from the unit (#2107). expectTextToContainPath(unit, serviceApiTokenFilePath()); @@ -908,8 +950,9 @@ describe("launchd service plist", () => { 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"); + const v1 = join(root, "installs", "2.35.0 package's"); + const v2 = join(root, "installs", "2.36.0 package's"); + const quoteForSh = (value: string): string => `'${value.replaceAll("'", "'\"'\"'")}'`; mkdirSync(shimDir, { recursive: true }); mkdirSync(v1, { recursive: true }); mkdirSync(v2, { recursive: true }); @@ -917,20 +960,20 @@ describe("launchd service plist", () => { 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 }); + writeFileSync(shim, `#!/bin/sh\nexec ${quoteForSh(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"); + expect(execFileSync(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 }); + writeFileSync(shim, `#!/bin/sh\nexec ${quoteForSh(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"); + expect(execFileSync(shim, ["start", "--port", "1"], { encoding: "utf8" })).toContain("V2"); rmSync(root, { recursive: true, force: true }); });