mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-03 03:03:24 -04:00
fix(cli): use standalone script for service restart after update (#17225)
The updater was previously attempting to restart the service using the installed codebase, which could be in an inconsistent state during the update process. This caused the service to stall when the updater deleted its own files before the restart could complete. Changes: - restart-helper.ts: new module that writes a platform-specific restart script to os.tmpdir() before the update begins (Linux systemd, macOS launchctl, Windows schtasks). - update-command.ts: prepares the restart script before installing, then uses it for service restart instead of the standard runDaemonRestart. - restart-helper.test.ts: 12 tests covering all platforms, custom profiles, error cases, and shell injection safety. Review feedback addressed: - Use spawn(detached: true) + unref() so restart script survives parent process termination (Greptile). - Shell-escape profile values using single-quote wrapping to prevent injection via OPENCLAW_PROFILE (Greptile). - Reject unsafe batch characters on Windows. - Self-cleanup: scripts delete themselves after execution (Copilot). - Add tests for write failures and custom profiles (Copilot). Fixes #17225
This commit is contained in:
committed by
Peter Steinberger
parent
a62ff19a66
commit
b1d5c71609
213
src/cli/update-cli/restart-helper.test.ts
Normal file
213
src/cli/update-cli/restart-helper.test.ts
Normal file
@@ -0,0 +1,213 @@
|
||||
import { spawn, type ChildProcess } from "node:child_process";
|
||||
import fs from "node:fs/promises";
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { prepareRestartScript, runRestartScript } from "./restart-helper.js";
|
||||
|
||||
vi.mock("node:child_process", () => ({
|
||||
spawn: vi.fn(),
|
||||
}));
|
||||
|
||||
describe("restart-helper", () => {
|
||||
const originalPlatform = process.platform;
|
||||
const originalGetUid = process.getuid;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.resetAllMocks();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
Object.defineProperty(process, "platform", { value: originalPlatform });
|
||||
process.getuid = originalGetUid;
|
||||
});
|
||||
|
||||
describe("prepareRestartScript", () => {
|
||||
it("creates a systemd restart script on Linux", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "linux" });
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "default",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
expect(scriptPath!.endsWith(".sh")).toBe(true);
|
||||
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
expect(content).toContain("#!/bin/sh");
|
||||
expect(content).toContain("systemctl --user restart 'openclaw-gateway.service'");
|
||||
// Script should self-cleanup
|
||||
expect(content).toContain('rm -f "$0"');
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("creates a launchd restart script on macOS", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "darwin" });
|
||||
process.getuid = () => 501;
|
||||
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "default",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
expect(scriptPath!.endsWith(".sh")).toBe(true);
|
||||
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
expect(content).toContain("#!/bin/sh");
|
||||
expect(content).toContain("launchctl kickstart -k 'gui/501/ai.openclaw.gateway'");
|
||||
expect(content).toContain('rm -f "$0"');
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("creates a schtasks restart script on Windows", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "win32" });
|
||||
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "default",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
expect(scriptPath!.endsWith(".bat")).toBe(true);
|
||||
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
expect(content).toContain("@echo off");
|
||||
expect(content).toContain('schtasks /End /TN "OpenClaw Gateway"');
|
||||
expect(content).toContain('schtasks /Run /TN "OpenClaw Gateway"');
|
||||
// Batch self-cleanup
|
||||
expect(content).toContain('del "%~f0"');
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("uses custom profile in service names", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "linux" });
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "production",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
expect(content).toContain("openclaw-gateway-production.service");
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("uses custom profile in macOS launchd label", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "darwin" });
|
||||
process.getuid = () => 502;
|
||||
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "staging",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
expect(content).toContain("gui/502/ai.openclaw.staging");
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("uses custom profile in Windows task name", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "win32" });
|
||||
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "production",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
expect(content).toContain('schtasks /End /TN "OpenClaw Gateway (production)"');
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("returns null for unsupported platforms", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "aix" });
|
||||
const scriptPath = await prepareRestartScript({});
|
||||
expect(scriptPath).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when script creation fails", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "linux" });
|
||||
const writeFileSpy = vi
|
||||
.spyOn(fs, "writeFile")
|
||||
.mockRejectedValueOnce(new Error("simulated write failure"));
|
||||
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "default",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeNull();
|
||||
writeFileSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("escapes single quotes in profile names for shell scripts", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "linux" });
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "it's-a-test",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeTruthy();
|
||||
const content = await fs.readFile(scriptPath!, "utf-8");
|
||||
// Single quotes should be escaped with '\'' pattern
|
||||
expect(content).not.toContain("it's");
|
||||
expect(content).toContain("it'\\''s");
|
||||
|
||||
if (scriptPath) {
|
||||
await fs.unlink(scriptPath);
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects unsafe batch profile names on Windows", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "win32" });
|
||||
const scriptPath = await prepareRestartScript({
|
||||
OPENCLAW_PROFILE: "test&whoami",
|
||||
});
|
||||
|
||||
expect(scriptPath).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("runRestartScript", () => {
|
||||
it("spawns the script as a detached process on Linux", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "linux" });
|
||||
const scriptPath = "/tmp/fake-script.sh";
|
||||
const mockChild = { unref: vi.fn() };
|
||||
vi.mocked(spawn).mockReturnValue(mockChild as unknown as ChildProcess);
|
||||
|
||||
await runRestartScript(scriptPath);
|
||||
|
||||
expect(spawn).toHaveBeenCalledWith("/bin/sh", [scriptPath], {
|
||||
detached: true,
|
||||
stdio: "ignore",
|
||||
});
|
||||
expect(mockChild.unref).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("uses cmd.exe on Windows", async () => {
|
||||
Object.defineProperty(process, "platform", { value: "win32" });
|
||||
const scriptPath = "C:\\Temp\\fake-script.bat";
|
||||
const mockChild = { unref: vi.fn() };
|
||||
vi.mocked(spawn).mockReturnValue(mockChild as unknown as ChildProcess);
|
||||
|
||||
await runRestartScript(scriptPath);
|
||||
|
||||
expect(spawn).toHaveBeenCalledWith("cmd.exe", ["/c", scriptPath], {
|
||||
detached: true,
|
||||
stdio: "ignore",
|
||||
});
|
||||
expect(mockChild.unref).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
118
src/cli/update-cli/restart-helper.ts
Normal file
118
src/cli/update-cli/restart-helper.ts
Normal file
@@ -0,0 +1,118 @@
|
||||
import { spawn } from "node:child_process";
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import {
|
||||
resolveGatewayLaunchAgentLabel,
|
||||
resolveGatewaySystemdServiceName,
|
||||
resolveGatewayWindowsTaskName,
|
||||
} from "../../daemon/constants.js";
|
||||
|
||||
/**
|
||||
* Shell-escape a string for embedding in single-quoted shell arguments.
|
||||
* Replaces every `'` with `'\''` (end quote, escaped quote, resume quote).
|
||||
* For batch scripts, validates against special characters instead.
|
||||
*/
|
||||
function shellEscape(value: string): string {
|
||||
return value.replace(/'/g, "'\\''");
|
||||
}
|
||||
|
||||
/** Validates a string is safe for embedding in a batch (cmd.exe) script. */
|
||||
function isBatchSafe(value: string): boolean {
|
||||
// Reject characters that have special meaning in batch: & | < > ^ % " ` $
|
||||
return /^[A-Za-z0-9 _\-().]+$/.test(value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Prepares a standalone script to restart the gateway service.
|
||||
* This script is written to a temporary directory and does not depend on
|
||||
* the installed package files, ensuring restart capability even if the
|
||||
* update process temporarily removes or corrupts installation files.
|
||||
*/
|
||||
export async function prepareRestartScript(
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
): Promise<string | null> {
|
||||
const tmpDir = os.tmpdir();
|
||||
const timestamp = Date.now();
|
||||
const platform = process.platform;
|
||||
|
||||
let scriptContent = "";
|
||||
let filename = "";
|
||||
|
||||
try {
|
||||
if (platform === "linux") {
|
||||
const serviceName = resolveGatewaySystemdServiceName(env.OPENCLAW_PROFILE);
|
||||
const escaped = shellEscape(`${serviceName}.service`);
|
||||
filename = `openclaw-restart-${timestamp}.sh`;
|
||||
scriptContent = `#!/bin/sh
|
||||
# Standalone restart script — survives parent process termination.
|
||||
# Wait briefly to ensure file locks are released after update.
|
||||
sleep 1
|
||||
systemctl --user restart '${escaped}'
|
||||
# Self-cleanup
|
||||
rm -f "$0"
|
||||
`;
|
||||
} else if (platform === "darwin") {
|
||||
const label = resolveGatewayLaunchAgentLabel(env.OPENCLAW_PROFILE);
|
||||
const escaped = shellEscape(label);
|
||||
// Fallback to 501 if getuid is not available (though it should be on macOS)
|
||||
const uid = process.getuid ? process.getuid() : 501;
|
||||
filename = `openclaw-restart-${timestamp}.sh`;
|
||||
scriptContent = `#!/bin/sh
|
||||
# Standalone restart script — survives parent process termination.
|
||||
# Wait briefly to ensure file locks are released after update.
|
||||
sleep 1
|
||||
launchctl kickstart -k 'gui/${uid}/${escaped}'
|
||||
# Self-cleanup
|
||||
rm -f "$0"
|
||||
`;
|
||||
} else if (platform === "win32") {
|
||||
const taskName = resolveGatewayWindowsTaskName(env.OPENCLAW_PROFILE);
|
||||
if (!isBatchSafe(taskName)) {
|
||||
return null;
|
||||
}
|
||||
filename = `openclaw-restart-${timestamp}.bat`;
|
||||
scriptContent = `@echo off
|
||||
REM Standalone restart script — survives parent process termination.
|
||||
REM Wait briefly to ensure file locks are released after update.
|
||||
timeout /t 2 /nobreak >nul
|
||||
schtasks /End /TN "${taskName}"
|
||||
schtasks /Run /TN "${taskName}"
|
||||
REM Self-cleanup
|
||||
del "%~f0"
|
||||
`;
|
||||
} else {
|
||||
return null;
|
||||
}
|
||||
|
||||
const scriptPath = path.join(tmpDir, filename);
|
||||
await fs.writeFile(scriptPath, scriptContent, { mode: 0o755 });
|
||||
return scriptPath;
|
||||
} catch {
|
||||
// If we can't write the script, we'll fall back to the standard restart method
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Executes the prepared restart script as a **detached** process.
|
||||
*
|
||||
* The script must outlive the CLI process because the CLI itself is part
|
||||
* of the service being restarted — `systemctl restart` / `launchctl
|
||||
* kickstart -k` will terminate the current process tree. Using
|
||||
* `spawn({ detached: true })` + `unref()` ensures the script survives
|
||||
* the parent's exit.
|
||||
*
|
||||
* Resolves immediately after spawning; the script runs independently.
|
||||
*/
|
||||
export async function runRestartScript(scriptPath: string): Promise<void> {
|
||||
const isWindows = process.platform === "win32";
|
||||
const file = isWindows ? "cmd.exe" : "/bin/sh";
|
||||
const args = isWindows ? ["/c", scriptPath] : [scriptPath];
|
||||
|
||||
const child = spawn(file, args, {
|
||||
detached: true,
|
||||
stdio: "ignore",
|
||||
});
|
||||
child.unref();
|
||||
}
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
} from "../../commands/doctor-completion.js";
|
||||
import { doctorCommand } from "../../commands/doctor.js";
|
||||
import { readConfigFileSnapshot, writeConfigFile } from "../../config/config.js";
|
||||
import { resolveGatewayService } from "../../daemon/service.js";
|
||||
import {
|
||||
channelToNpmTag,
|
||||
DEFAULT_GIT_CHANNEL,
|
||||
@@ -34,6 +35,7 @@ import { formatCliCommand } from "../command-format.js";
|
||||
import { installCompletion } from "../completion-cli.js";
|
||||
import { runDaemonRestart } from "../daemon-cli.js";
|
||||
import { createUpdateProgress, printResult } from "./progress.js";
|
||||
import { prepareRestartScript, runRestartScript } from "./restart-helper.js";
|
||||
import {
|
||||
DEFAULT_PACKAGE_NAME,
|
||||
ensureGitCheckout,
|
||||
@@ -388,6 +390,7 @@ async function maybeRestartService(params: {
|
||||
shouldRestart: boolean;
|
||||
result: UpdateRunResult;
|
||||
opts: UpdateCommandOptions;
|
||||
restartScriptPath?: string | null;
|
||||
}): Promise<void> {
|
||||
if (params.shouldRestart) {
|
||||
if (!params.opts.json) {
|
||||
@@ -396,7 +399,14 @@ async function maybeRestartService(params: {
|
||||
}
|
||||
|
||||
try {
|
||||
const restarted = await runDaemonRestart();
|
||||
let restarted = false;
|
||||
if (params.restartScriptPath) {
|
||||
await runRestartScript(params.restartScriptPath);
|
||||
restarted = true;
|
||||
} else {
|
||||
restarted = await runDaemonRestart();
|
||||
}
|
||||
|
||||
if (!params.opts.json && restarted) {
|
||||
defaultRuntime.log(theme.success("Daemon restarted successfully."));
|
||||
defaultRuntime.log("");
|
||||
@@ -566,6 +576,18 @@ export async function updateCommand(opts: UpdateCommandOptions): Promise<void> {
|
||||
const { progress, stop } = createUpdateProgress(showProgress);
|
||||
const startedAt = Date.now();
|
||||
|
||||
let restartScriptPath: string | null = null;
|
||||
if (shouldRestart) {
|
||||
try {
|
||||
const loaded = await resolveGatewayService().isLoaded({ env: process.env });
|
||||
if (loaded) {
|
||||
restartScriptPath = await prepareRestartScript(process.env);
|
||||
}
|
||||
} catch {
|
||||
// Ignore errors during pre-check; fallback to standard restart
|
||||
}
|
||||
}
|
||||
|
||||
const result = switchToPackage
|
||||
? await runPackageInstallUpdate({
|
||||
root,
|
||||
@@ -638,6 +660,7 @@ export async function updateCommand(opts: UpdateCommandOptions): Promise<void> {
|
||||
shouldRestart,
|
||||
result,
|
||||
opts,
|
||||
restartScriptPath,
|
||||
});
|
||||
|
||||
if (!opts.json) {
|
||||
|
||||
Reference in New Issue
Block a user