From 92c5bc0f5b7e20149c364347e2879707afbdf547 Mon Sep 17 00:00:00 2001 From: L42y <423300@gmail.com> Date: Wed, 23 Sep 2026 07:16:12 +0000 Subject: [PATCH] fix(cli): make systemd npm updates restart-safe --- .../src/__tests__/update-global-extra.test.ts | 113 +++++++++- apps/cli/src/core/supervisor/systemd.ts | 7 +- apps/cli/src/core/update-glue.ts | 13 +- apps/cli/src/core/update.ts | 201 ++++++++++++++++-- docs/development/local-dev-isolation.md | 5 +- docs/development/portable-node-runtime.md | 8 + 6 files changed, 324 insertions(+), 23 deletions(-) diff --git a/apps/cli/src/__tests__/update-global-extra.test.ts b/apps/cli/src/__tests__/update-global-extra.test.ts index 1298da5a89..733c967447 100644 --- a/apps/cli/src/__tests__/update-global-extra.test.ts +++ b/apps/cli/src/__tests__/update-global-extra.test.ts @@ -1,5 +1,6 @@ +import { execFileSync } from "node:child_process"; import { EventEmitter } from "node:events"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -210,6 +211,116 @@ describe("global update helpers", () => { ); }); + it("runs managed Linux installs in a transient unit with rollback boundaries", async () => { + if (process.platform !== "linux") return; + spawnSyncMock.mockReturnValueOnce({ status: 0, stdout: "null", stderr: "" }); + const child = new FakeChild(); + registrySpawnMock.mockReturnValueOnce({ child }); + const { installGlobalSpec } = await import("../core/update.js"); + + const installing = installGlobalSpec("latest", { managed: true }); + const [command, args] = registrySpawnMock.mock.calls[0] as [string, string[]]; + expect(command).toBe("systemd-run"); + expect(args).toEqual(expect.arrayContaining(["--wait", "--collect", "--pipe", "--unit"])); + expect(args).toContain("first-tree-update.service"); + const script = args[args.indexOf("-c") + 1]; + expect(script).toContain("systemctl"); + expect(script).toContain("stop 'first-tree.service'"); + expect(script).toContain("start 'first-tree.service'"); + expect(script).toContain("first-tree-update-backup"); + expect(() => execFileSync("/bin/sh", ["-n", "-c", script], { encoding: "utf8" })).not.toThrow(); + + child.stdout.emit("data", Buffer.from("+ first-tree@1.2.3\n")); + child.emit("exit", 0, null); + await expect(installing).resolves.toEqual({ ok: true, mode: "global", installedVersion: "1.2.3" }); + }); + + it("rolls back the moved npm tree before restarting after a failed managed install", async () => { + if (process.platform !== "linux") return; + spawnSyncMock.mockReturnValueOnce({ status: 0, stdout: "null", stderr: "" }); + const child = new FakeChild(); + registrySpawnMock.mockReturnValueOnce({ child }); + const { installGlobalSpec } = await import("../core/update.js"); + const installing = installGlobalSpec("latest", { managed: true }); + const [, args] = registrySpawnMock.mock.calls[0] as [string, string[]]; + const script = args[args.indexOf("-c") + 1]; + + const root = mkdtempSync(join(tmpdir(), "ft-managed-update-rollback-")); + try { + const prefix = join(root, "prefix"); + const packageRoot = join(prefix, "lib", "node_modules"); + const packageDir = join(packageRoot, "first-tree"); + const binDir = join(prefix, "bin"); + const fakeBin = join(root, "fake-bin"); + const logPath = join(root, "systemctl.log"); + const fakeNpm = join(fakeBin, "npm"); + mkdirSync(packageDir, { recursive: true }); + mkdirSync(binDir, { recursive: true }); + mkdirSync(fakeBin, { recursive: true }); + writeFileSync(join(packageDir, "sentinel"), "old-package"); + writeFileSync(join(binDir, "first-tree"), "old-bin"); + writeFileSync(join(binDir, "ft"), "old-alias"); + writeFileSync(join(fakeBin, "systemctl"), '#!/bin/sh\nprintf \'%s\\n\' "$*" >> "$SYSTEMCTL_LOG"\nexit 0\n', { + mode: 0o755, + }); + writeFileSync( + fakeNpm, + [ + "#!/bin/sh", + `root=${JSON.stringify(packageRoot)}`, + `prefix=${JSON.stringify(prefix)}`, + 'if [ "$1" = "root" ]; then printf \'%s\\n\' "$root"; exit 0; fi', + 'if [ "$1" = "prefix" ]; then printf \'%s\\n\' "$prefix"; exit 0; fi', + 'mkdir -p "$root/first-tree"', + "printf 'partial\\n' > \"$root/first-tree/partial\"", + "exit 42", + ].join("\n"), + { mode: 0o755 }, + ); + + const env = { + ...process.env, + PATH: `${fakeBin}:${process.env.PATH ?? ""}`, + SYSTEMCTL_LOG: logPath, + }; + expect(() => + execFileSync( + "/bin/sh", + ["-c", script, "first-tree-npm-update", fakeNpm, "install", "-g", "first-tree@latest"], + { env }, + ), + ).toThrow(); + expect(readFileSync(join(packageDir, "sentinel"), "utf8")).toBe("old-package"); + expect(() => readFileSync(join(packageDir, "partial"))).toThrow(); + expect(readFileSync(join(binDir, "first-tree"), "utf8")).toBe("old-bin"); + expect(readFileSync(join(binDir, "ft"), "utf8")).toBe("old-alias"); + expect(readFileSync(logPath, "utf8")).toMatch(/stop first-tree\.service/); + expect(readFileSync(logPath, "utf8")).toMatch(/start first-tree\.service/); + } finally { + rmSync(root, { recursive: true, force: true }); + } + + child.emit("exit", 0, null); + await expect(installing).resolves.toEqual({ ok: true, mode: "global", installedVersion: null }); + }); + + it("refuses a managed install when systemd-run cannot be started", async () => { + if (process.platform !== "linux") return; + spawnSyncMock.mockReturnValueOnce({ status: 0, stdout: "null", stderr: "" }); + const spawnError = Object.assign(new Error("systemd-run not found"), { code: "ENOENT" }); + registrySpawnMock.mockImplementationOnce(() => { + throw spawnError; + }); + const { installGlobalSpec } = await import("../core/update.js"); + + await expect(installGlobalSpec("latest", { managed: true })).resolves.toMatchObject({ + ok: false, + retryable: false, + reasonCode: "systemd_update_runner_unavailable", + reason: expect.stringContaining("systemd-run is unavailable"), + }); + }); + it("prefers sibling npm, tolerates empty engine stdout, and keeps non-semver installed labels", async () => { existsSyncMock.mockImplementation( (path: unknown) => String(path).endsWith("/npm") || String(path).endsWith("\\npm.cmd"), diff --git a/apps/cli/src/core/supervisor/systemd.ts b/apps/cli/src/core/supervisor/systemd.ts index 0690103d1d..2a1cfe09b0 100644 --- a/apps/cli/src/core/supervisor/systemd.ts +++ b/apps/cli/src/core/supervisor/systemd.ts @@ -109,9 +109,10 @@ export function renderSystemdUnit( // Restart policy split: // - on-failure → operator-issued `systemctl stop` (clean exit 0) really stops. // - SuccessExitStatus=0 makes that explicit. - // - RestartForceExitStatus=75 keeps the self-update path working: the - // UpdateManager exits 75 after `npm i -g`, systemd sees it as a - // "must restart" signal and brings up the new binary. + // - RestartForceExitStatus=75 remains the fallback self-update path for + // portable installs and environments that do not use the managed Linux + // npm handoff. Managed Linux npm updates run in a transient unit that + // stops this service before touching its global package tree. // StartLimit* caps a crash storm (10 failures in 5 min → systemd holds back). // Normal client diagnostics go through the rotating NDJSON `client.log` when // FIRST_TREE_SERVICE_MODE=1; journald is only the supervisor fallback for diff --git a/apps/cli/src/core/update-glue.ts b/apps/cli/src/core/update-glue.ts index 2b5428be9c..f30462200c 100644 --- a/apps/cli/src/core/update-glue.ts +++ b/apps/cli/src/core/update-glue.ts @@ -78,9 +78,10 @@ function createInstallOutputLog(log: UpdateLogger | undefined): ((chunk: string) * Build the command-layer `executeUpdate` callback. * * `managed=true` means a process supervisor (launchd / systemd / Docker - * `restart`) is expected to relaunch us after `process.exit` — the callback - * installs the new bits and exits with `SELF_RESTART_EXIT_CODE` so the - * relaunch picks up the new binary. + * `restart`) owns the daemon lifecycle. Portable updates and non-Linux + * managed paths install the new bits and exit with `SELF_RESTART_EXIT_CODE`; + * managed Linux npm updates hand off to a transient systemd unit that stops + * and starts the service around the install. * * `managed=false` means the process is running standalone (e.g. manual * `client start`, `login --no-start`, CI without a supervisor). @@ -176,9 +177,13 @@ export function createExecuteUpdate({ ? `Switching portable ${channelConfig.binName} to ${targetVersion}...` : `Running \`npm install -g ${pkgSpec}@${targetVersion}\`...`, ); + const installOptions = { + managed, + ...(installOutput ? { output: installOutput } : {}), + }; const result = isPortable ? await installPortableSpec(targetVersion) - : await installGlobalSpec(targetVersion, installOutput ? { output: installOutput } : undefined); + : await installGlobalSpec(targetVersion, installOptions); if (!result.ok) { emit("warn", `Install failed: ${result.reason}`); recordUpdateAttempt({ diff --git a/apps/cli/src/core/update.ts b/apps/cli/src/core/update.ts index 36edbe8653..024e4d8463 100644 --- a/apps/cli/src/core/update.ts +++ b/apps/cli/src/core/update.ts @@ -34,6 +34,7 @@ import { const NPM_INSTALL_TIMEOUT_MS = 5 * 60 * 1000; /** Short metadata probe used only to catch guaranteed npm-mode engine mismatch. */ const NPM_METADATA_TIMEOUT_MS = 10 * 1000; +const SYSTEMD_UPDATE_RUNNER = "systemd-run"; export type InstallMode = "global" | "npx" | "source" | "portable"; export type VersionLookupFailureCode = "server_url_not_configured"; @@ -171,12 +172,173 @@ export type ExecuteUpdateResult = export type InstallGlobalSpecOptions = { output?: (chunk: string) => void; + /** Run the install outside the supervisor cgroup when the daemon is managed. */ + managed?: boolean; }; function writeInstallOutput(options: InstallGlobalSpecOptions | undefined, chunk: string): void { (options?.output ?? print.line)(chunk); } +function shellQuote(value: string): string { + return `'${value.replace(/'/g, "'\\''")}'`; +} + +function systemdManagerArgs(): string[] { + return process.getuid?.() === 0 ? [] : ["--user"]; +} + +function systemdUpdateUnitName(): string { + const serviceUnit = channelConfig.serviceUnitFile.replace(/\.service$/u, ""); + return `${serviceUnit}-update.service`; +} + +/** + * Keep the npm tree quiescent while npm reifies it. The service stop is part + * of the transient unit rather than the daemon process, so systemd can kill + * the daemon's `systemd-run` client without killing this worker. The old + * package and bin links are moved aside first; a failed or interrupted npm + * run is rolled back before the service is started again. + */ +function systemdUpdateScript(): string { + const systemctl = ["systemctl", ...systemdManagerArgs()].map(shellQuote).join(" "); + const packageName = shellQuote(PACKAGE_NAME ?? ""); + const binName = shellQuote(channelConfig.binName); + const aliasName = shellQuote(channelConfig.aliasName); + const serviceUnit = shellQuote(channelConfig.serviceUnitFile); + + return [ + "set -u", + "service_stopped=0", + "install_succeeded=0", + "rollback_safe=1", + "had_package=0", + "had_bin=0", + "had_alias=0", + "package_backup=", + "bin_backup=", + "alias_backup=", + "package_root=", + "prefix=", + "package_dir=", + "bin_dir=", + "bin_path=", + "alias_path=", + 'path_exists() { [ -e "$1" ] || [ -L "$1" ]; }', + 'remove_path() { if path_exists "$1"; then rm -rf -- "$1" || return 1; fi; }', + "restore_path() {", + ' original="$1"', + ' backup="$2"', + ' had="$3"', + ' remove_path "$original" || return 1', + ' if [ "$had" -eq 1 ]; then', + ' mv -- "$backup" "$original" || return 1', + " fi", + "}", + "rollback() {", + " rollback_status=0", + ' if [ -n "$package_dir" ]; then restore_path "$package_dir" "$package_backup" "$had_package" || rollback_status=1; fi', + ' if [ -n "$bin_path" ]; then restore_path "$bin_path" "$bin_backup" "$had_bin" || rollback_status=1; fi', + ' if [ -n "$alias_path" ]; then restore_path "$alias_path" "$alias_backup" "$had_alias" || rollback_status=1; fi', + ' return "$rollback_status"', + "}", + "finish() {", + " status=$?", + ' if [ "$install_succeeded" -ne 1 ]; then', + " rollback || rollback_safe=0", + " else", + ' remove_path "$package_backup" || true', + ' remove_path "$bin_backup" || true', + ' remove_path "$alias_backup" || true', + " fi", + ' if [ "$service_stopped" -eq 1 ] && { [ "$install_succeeded" -eq 1 ] || [ "$rollback_safe" -eq 1 ]; }; then', + ` ${systemctl} start ${serviceUnit} >/dev/null 2>&1 || start_status=$?`, + " start_status=$" + "{start_status:-0}", + ' if [ "$status" -eq 0 ] && [ "$start_status" -ne 0 ]; then status=$start_status; fi', + " fi", + ' exit "$status"', + "}", + "trap finish EXIT", + "trap 'exit 143' HUP INT TERM", + `npm_command="$1"; shift; package_name=${packageName}; bin_name=${binName}; alias_name=${aliasName}`, + `${systemctl} stop ${serviceUnit}`, + "stop_status=$?", + 'if [ "$stop_status" -ne 0 ]; then exit "$stop_status"; fi', + "service_stopped=1", + 'package_root=$("$npm_command" root --global 2>/dev/null) || exit 1', + 'prefix=$("$npm_command" prefix --global 2>/dev/null) || exit 1', + 'case "$package_root" in /*) ;; *) exit 1 ;; esac', + 'case "$prefix" in /*) ;; *) exit 1 ;; esac', + 'package_dir="$package_root/$package_name"', + 'bin_dir="$prefix/bin"', + 'bin_path="$bin_dir/$bin_name"', + 'alias_path="$bin_dir/$alias_name"', + 'package_backup="$package_dir.first-tree-update-backup.$$"', + 'bin_backup="$bin_path.first-tree-update-backup.$$"', + 'alias_backup="$alias_path.first-tree-update-backup.$$"', + 'if path_exists "$package_backup" || path_exists "$bin_backup" || path_exists "$alias_backup"; then package_dir=; bin_path=; alias_path=; exit 1; fi', + 'if path_exists "$package_dir"; then if mv -- "$package_dir" "$package_backup"; then had_package=1; else package_dir=; bin_path=; alias_path=; exit 1; fi; fi', + 'if path_exists "$bin_path"; then if mv -- "$bin_path" "$bin_backup"; then had_bin=1; else bin_path=; alias_path=; exit 1; fi; fi', + 'if path_exists "$alias_path"; then if mv -- "$alias_path" "$alias_backup"; then had_alias=1; else alias_path=; exit 1; fi; fi', + '"$npm_command" "$@"', + "install_status=$?", + 'if [ "$install_status" -ne 0 ]; then exit "$install_status"; fi', + "install_succeeded=1", + "exit 0", + ].join("\n"); +} + +function systemdUpdateArgs(npm: ReturnType): string[] { + const environment = [ + "PATH", + "HOME", + "FIRST_TREE_HOME", + "NPM_CONFIG_PREFIX", + "NPM_CONFIG_USERCONFIG", + "NPM_CONFIG_REGISTRY", + "NPM_CONFIG_CACHE", + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "NO_PROXY", + "http_proxy", + "https_proxy", + "all_proxy", + "no_proxy", + "NODE_EXTRA_CA_CERTS", + "NODE_OPTIONS", + ] + .map((name) => { + const value = process.env[name]; + return value === undefined ? null : `--setenv=${name}=${value}`; + }) + .filter((value): value is string => value !== null); + + return [ + ...systemdManagerArgs(), + "--unit", + systemdUpdateUnitName(), + "--collect", + "--wait", + "--pipe", + "--quiet", + "--service-type=oneshot", + `--property=TimeoutStartSec=${NPM_INSTALL_TIMEOUT_MS / 1000}s`, + ...environment, + "--", + "/bin/sh", + "-c", + systemdUpdateScript(), + "first-tree-npm-update", + npm.command, + ...npm.args, + ]; +} + +function isMissingCommandError(err: unknown): boolean { + return typeof err === "object" && err !== null && "code" in err && (err as { code?: unknown }).code === "ENOENT"; +} + /** * Validate an npm install spec (the part after `@` in `@`). We * accept either a known dist-tag string (`latest`, `alpha`, …) or an exact @@ -736,18 +898,25 @@ export async function installGlobalSpec( writeInstallOutput(options, ` [update] ${prefixFailure.reason}\n`); return prefixFailure; } + const useSystemdRunner = options?.managed === true && process.platform === "linux"; return new Promise((resolvePromise) => { const npm = resolveNpmInvocation(["install", "-g", `${PACKAGE_NAME}@${spec}`]); - // Bug 4: route the subprocess through ChildProcessRegistry so it is - // tracked and reaped by the lifecycle shutdown hook, AND give it a - // 5-minute hard timeout (network blip on the registry used to block - // the main process for 60s+ with no escalation). Failures are mapped - // through the error taxonomy so UpdateManager knows whether to retry. + const command = useSystemdRunner ? SYSTEMD_UPDATE_RUNNER : npm.command; + const args = useSystemdRunner ? systemdUpdateArgs(npm) : npm.args; + // Route the wrapper through ChildProcessRegistry so it is tracked and + // reaped by the lifecycle shutdown hook, AND give it a 5-minute hard + // timeout. In managed Linux mode the wrapper creates a transient unit; + // that unit stops the daemon, performs the install, rolls back on + // failure, and starts the daemon only after the package tree is complete. + // The transient worker therefore survives the supervisor killing this + // daemon's wrapper during the stop phase. let child: ChildProcess; try { - ({ child } = getChildProcessRegistry().spawn(npm.command, npm.args, { + ({ child } = getChildProcessRegistry().spawn(command, args, { category: "npm-install", - label: `npm install -g ${PACKAGE_NAME}@${spec}`, + label: useSystemdRunner + ? `systemd-run npm install -g ${PACKAGE_NAME}@${spec}` + : `npm install -g ${PACKAGE_NAME}@${spec}`, timeoutMs: NPM_INSTALL_TIMEOUT_MS, stdio: ["ignore", "pipe", "pipe"], shell: npm.shell, @@ -755,12 +924,15 @@ export async function installGlobalSpec( } catch (err) { const message = err instanceof Error ? err.message : String(err); const classification = classify(err, { source: "update" }); + const runnerUnavailable = useSystemdRunner && isMissingCommandError(err); resolvePromise({ ok: false, mode: "global", - reason: message, - retryable: classification.kind === ERROR_KINDS.TRANSIENT, - reasonCode: classification.reasonCode, + reason: runnerUnavailable + ? "Cannot run a managed npm update safely because systemd-run is unavailable; install the portable CLI or update manually." + : message, + retryable: runnerUnavailable ? false : classification.kind === ERROR_KINDS.TRANSIENT, + reasonCode: runnerUnavailable ? "systemd_update_runner_unavailable" : classification.reasonCode, }); return; } @@ -777,12 +949,15 @@ export async function installGlobalSpec( child.on("error", (err) => { const message = err instanceof Error ? err.message : String(err); const classification = classify(err, { source: "update" }); + const runnerUnavailable = useSystemdRunner && isMissingCommandError(err); resolvePromise({ ok: false, mode: "global", - reason: message, - retryable: classification.kind === ERROR_KINDS.TRANSIENT, - reasonCode: classification.reasonCode, + reason: runnerUnavailable + ? "Cannot run a managed npm update safely because systemd-run is unavailable; install the portable CLI or update manually." + : message, + retryable: runnerUnavailable ? false : classification.kind === ERROR_KINDS.TRANSIENT, + reasonCode: runnerUnavailable ? "systemd_update_runner_unavailable" : classification.reasonCode, }); }); diff --git a/docs/development/local-dev-isolation.md b/docs/development/local-dev-isolation.md index 596431552d..ff55cb7a4d 100644 --- a/docs/development/local-dev-isolation.md +++ b/docs/development/local-dev-isolation.md @@ -159,8 +159,9 @@ After the installer completes successfully, sign in with the staging binary: installs, upgrading either (for example, via `first-tree-staging upgrade`) follows that channel's configured server target and updates only that channel's portable prefix. Existing legacy npm-mode installs retain their - machine-wide global npm update behavior. Dev is immune because its - source-checkout install mode short-circuits the upgrade path. + machine-wide global npm update behavior; managed Linux daemons perform that + update in a transient unit while the daemon is stopped. Dev is immune + because its source-checkout install mode short-circuits the upgrade path. ## Tearing down a dev install diff --git a/docs/development/portable-node-runtime.md b/docs/development/portable-node-runtime.md index b2f30c74a4..c9473e55ef 100644 --- a/docs/development/portable-node-runtime.md +++ b/docs/development/portable-node-runtime.md @@ -102,3 +102,11 @@ range, the update fails before install with guidance to either: If npm metadata cannot be read, the CLI falls back to the existing npm install path and classifies npm's own error output. `EBADENGINE` remains a permanent operator-action failure. + +On Linux, an npm-mode daemon managed by systemd performs automatic updates in a +transient `systemd-run` unit outside the daemon's service cgroup. The worker +stops the daemon before npm reifies the global package tree, moves the current +package and channel links aside for rollback, and starts the daemon only after +the install completes. This prevents a supervisor restart from killing npm +halfway through a reify and booting a daemon against a partial `node_modules` +tree. Foreground and manually invoked npm updates retain the direct npm path.