From 7af7b1c9f90a1eaad03773aa91ca68bb1dc57e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Chalifour?= Date: Mon, 13 Jul 2026 14:50:45 +0200 Subject: [PATCH] fix(updater): make CLI updates resilient and actionable Prevent automatic update checks from interrupting successful commands when configuration, cache persistence, or release discovery fails. Keep atomic temporary files beside their destinations so writes and replacements stay on the same filesystem. Verify downloaded native binaries before replacing the current executable, preserve rollback behavior, and route source checkouts through the detected package manager while recommending the concise self-update command. Simplify human-facing update output, reuse cached availability between daily network checks, show notices after help, and make --clear-cache perform an immediate refresh. Add regression coverage for failure containment, installation routing, binary verification, cached notices, and command output. --- cli/src/cli.ts | 4 + cli/src/commands/update.ts | 2 +- cli/src/lib/config-files.ts | 49 +++++---- cli/src/lib/updater-config.ts | 1 - cli/src/lib/updater.ts | 197 ++++++++++++++++++---------------- cli/tests/updater.test.ts | 161 +++++++++++++++++++++++---- 6 files changed, 281 insertions(+), 133 deletions(-) diff --git a/cli/src/cli.ts b/cli/src/cli.ts index a2af3af..eca1aa0 100644 --- a/cli/src/cli.ts +++ b/cli/src/cli.ts @@ -165,6 +165,10 @@ async function bootstrap(): Promise { if (earlyExit?.id === "help") { const [command, parent] = await resolveSubCommandForUsage(main, rawArgs); await showAltertableUsage(command, parent); + await maybeShowUpdateNotice({ + context: getCliContext(), + commandName: resolveTopLevelCommandName(rawArgs) ?? "help", + }); process.exit(EXIT_SUCCESS); } diff --git a/cli/src/commands/update.ts b/cli/src/commands/update.ts index aac546c..530d17e 100644 --- a/cli/src/commands/update.ts +++ b/cli/src/commands/update.ts @@ -83,7 +83,7 @@ async function runUpdateCommand(args: UpdateCommandArgs, sink: OutputSink): Prom return; } - if ((interval || args["clear-cache"]) && !args.install && !args["target-version"]) { + if (interval && !args["clear-cache"] && !args.install && !args["target-version"]) { await writeStatus(sink); return; } diff --git a/cli/src/lib/config-files.ts b/cli/src/lib/config-files.ts index f770aa8..0ddc9c2 100644 --- a/cli/src/lib/config-files.ts +++ b/cli/src/lib/config-files.ts @@ -1,12 +1,15 @@ import { randomBytes } from "node:crypto"; -import { chmodSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; -import { tmpdir } from "node:os"; +import { chmodSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; function trim(value: string): string { return value.trim(); } +function tempSiblingPath(filePath: string): string { + return join(dirname(filePath), `.altertable-kv-${randomBytes(8).toString("hex")}`); +} + export function configDir(): string { const override = process.env.ALTERTABLE_CONFIG_HOME; if (override) { @@ -48,7 +51,7 @@ export function kvGet(filePath: string, key: string): string { export function kvSet(filePath: string, key: string, value: string): void { mkdirSync(dirname(filePath), { recursive: true }); - const tmpPath = join(tmpdir(), `altertable-kv-${randomBytes(8).toString("hex")}`); + const tmpPath = tempSiblingPath(filePath); let found = false; let lines: string[] = []; @@ -77,25 +80,29 @@ export function kvSet(filePath: string, key: string, value: string): void { output.push(`${key}=${value}`); } - writeFileSync( - tmpPath, - output - .filter((line, index, array) => { - if (index === array.length - 1 && line === "") { - return false; - } - return true; - }) - .join("\n") + (output.length > 0 ? "\n" : ""), - { mode: 0o600 }, - ); - renameSync(tmpPath, filePath); + try { + writeFileSync( + tmpPath, + output + .filter((line, index, array) => { + if (index === array.length - 1 && line === "") { + return false; + } + return true; + }) + .join("\n") + (output.length > 0 ? "\n" : ""), + { mode: 0o600 }, + ); + renameSync(tmpPath, filePath); + } finally { + rmSync(tmpPath, { force: true }); + } } export function kvUnset(filePath: string, key: string): void { try { const lines = readFileSync(filePath, "utf8").split("\n"); - const tmpPath = join(tmpdir(), `altertable-kv-${randomBytes(8).toString("hex")}`); + const tmpPath = tempSiblingPath(filePath); const output: string[] = []; for (const line of lines) { const eqIndex = line.indexOf("="); @@ -105,8 +112,12 @@ export function kvUnset(filePath: string, key: string): void { } output.push(line); } - writeFileSync(tmpPath, output.join("\n"), { mode: 0o600 }); - renameSync(tmpPath, filePath); + try { + writeFileSync(tmpPath, output.join("\n"), { mode: 0o600 }); + renameSync(tmpPath, filePath); + } finally { + rmSync(tmpPath, { force: true }); + } } catch { // file does not exist } diff --git a/cli/src/lib/updater-config.ts b/cli/src/lib/updater-config.ts index 208ebc8..f23bd15 100644 --- a/cli/src/lib/updater-config.ts +++ b/cli/src/lib/updater-config.ts @@ -111,7 +111,6 @@ export const UpdaterConfig = { }, commands: { selfUpdate: "altertable update --install", - packageManagerUpdate: "altertable update --install --install-method package-manager", }, } as const; diff --git a/cli/src/lib/updater.ts b/cli/src/lib/updater.ts index c9f93e0..9e9accf 100644 --- a/cli/src/lib/updater.ts +++ b/cli/src/lib/updater.ts @@ -1,6 +1,5 @@ import { existsSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { chmodSync } from "node:fs"; -import { tmpdir } from "node:os"; import { basename, dirname, join } from "node:path"; import { createHash, randomBytes } from "node:crypto"; import { spawnSync } from "node:child_process"; @@ -13,8 +12,8 @@ import { CliError, HttpError, NetworkError } from "@/lib/errors.ts"; import { hasObjectKey } from "@/lib/object.ts"; import type { OutputSink } from "@/lib/runtime.ts"; import { getOutputSink } from "@/lib/runtime.ts"; -import { terminalMetadata } from "@/ui/terminal/styles.ts"; -import { document, rows, section, text, type DisplayRow } from "@/ui/document.ts"; +import { terminalAccent, terminalMetadata, terminalSuccess } from "@/ui/terminal/styles.ts"; +import { document, rows, section, type DisplayRow } from "@/ui/document.ts"; import { renderDocumentText } from "@/ui/renderers/terminal.ts"; import { UpdaterInstallManagers, @@ -240,9 +239,13 @@ function updaterStateFile(): string { function writeJsonFileAtomic(filePath: string, data: unknown): void { mkdirSync(dirname(filePath), { recursive: true }); - const tmpPath = join(tmpdir(), `altertable-update-${randomBytes(8).toString("hex")}`); - writeFileSync(tmpPath, `${JSON.stringify(data, null, 2)}\n`, { mode: 0o600 }); - renameSync(tmpPath, filePath); + const tmpPath = tempSiblingPath(filePath, "update"); + try { + writeFileSync(tmpPath, `${JSON.stringify(data, null, 2)}\n`, { mode: 0o600 }); + renameSync(tmpPath, filePath); + } finally { + rmSync(tmpPath, { force: true }); + } try { chmodSync(filePath, 0o600); } catch { @@ -796,13 +799,7 @@ function resolveInstallMethodForInstallation( if (installation.kind === UpdaterInstallationKind.nativeBinary) { return UpdaterInstallMethod.githubBinary; } - if (installation.kind === UpdaterInstallationKind.packageManager) { - return UpdaterInstallMethod.packageManager; - } - throw new CliError("Automatic CLI install is not supported for this checkout.", { - details: - "Source checkouts should be updated with git, or pass --install-method package-manager to install the published package globally.", - }); + return UpdaterInstallMethod.packageManager; } function spawnOutputText(value: unknown): string { @@ -819,20 +816,21 @@ export function verifyInstalledCliVersion( expectedVersion: string, command = "altertable", spawnImpl: SpawnSyncImpl = spawnSync, + description = "Installed altertable", ): string { const result = spawnImpl(command, ["--version"], { encoding: "utf8" }); if (result.error) { - throw new CliError(`Unable to verify installed altertable version.`, { cause: result.error }); + throw new CliError(`Unable to verify ${description.toLowerCase()} version.`, { + cause: result.error, + }); } if (result.status !== 0) { - throw new CliError( - `Installed altertable version check failed with exit code ${result.status ?? 1}.`, - ); + throw new CliError(`${description} version check failed with exit code ${result.status ?? 1}.`); } const installedVersion = normalizeVersion(spawnOutputText(result.stdout).trim()); if (compareVersions(installedVersion, expectedVersion) !== 0) { throw new CliError( - `Installed altertable version is ${installedVersion || "unknown"}, expected ${normalizeVersion( + `${description} version is ${installedVersion || "unknown"}, expected ${normalizeVersion( expectedVersion, )}.`, ); @@ -936,18 +934,27 @@ export async function installGitHubBinaryRelease( timeoutMs, fetchImpl, }); - replaceFileAtomically(targetPath, tempPath); + try { + const verifiedVersion = + options.verify === false + ? release.version + : verifyInstalledCliVersion( + release.version, + tempPath, + options.spawnImpl, + "Downloaded altertable", + ); + replaceFileAtomically(targetPath, tempPath); - const verifiedVersion = - options.verify === false - ? release.version - : verifyInstalledCliVersion(release.version, targetPath, options.spawnImpl); - return { - installed_version: release.version, - verified_version: verifiedVersion, - method: UpdaterInstallMethod.githubBinary, - target_path: targetPath, - }; + return { + installed_version: release.version, + verified_version: verifiedVersion, + method: UpdaterInstallMethod.githubBinary, + target_path: targetPath, + }; + } finally { + rmSync(tempPath, { force: true }); + } } export function runInstallPlan(plan: InstallPlan, options: RunInstallPlanOptions = {}): string { @@ -1015,18 +1022,10 @@ export function recommendedInstallCommand( version: string, installation: CurrentInstallation = detectCurrentInstallation(), ): string { - try { - const method = resolveInstallMethodForInstallation( - UpdaterConfig.defaults.installMethod, - installation, - ); - if (method === UpdaterInstallMethod.githubBinary) { - return UpdaterConfig.commands.selfUpdate; - } - } catch { - return UpdaterConfig.commands.packageManagerUpdate; + if (installation.kind === UpdaterInstallationKind.packageManager) { + return createInstallPlan(version).display; } - return createInstallPlan(version).display; + return UpdaterConfig.commands.selfUpdate; } export async function checkForUpdate(options: FetchLatestOptions = {}): Promise { @@ -1072,6 +1071,29 @@ export function shouldRunAutomaticUpdateCheck(options: { state?: UpdateState; now?: Date; stderrIsTTY?: boolean; +}): boolean { + if (!shouldShowAutomaticUpdateNotice(options)) { + return false; + } + + const interval = getUpdateCheckInterval(); + const state = options.state ?? readUpdateState(); + if (!state.last_checked_at) { + return true; + } + + const previous = Date.parse(state.last_checked_at); + if (Number.isNaN(previous)) { + return true; + } + const now = options.now ?? new Date(); + return now.getTime() - previous >= intervalMs(interval); +} + +function shouldShowAutomaticUpdateNotice(options: { + context: CliContext; + commandName?: string; + stderrIsTTY?: boolean; }): boolean { if (isJsonOutput(options.context) || options.context.debug) { return false; @@ -1097,73 +1119,62 @@ export function shouldRunAutomaticUpdateCheck(options: { if (interval === "never") { return false; } + return true; +} - const state = options.state ?? readUpdateState(); - if (!state.last_checked_at) { - return true; - } - - const previous = Date.parse(state.last_checked_at); - if (Number.isNaN(previous)) { - return true; - } - const now = options.now ?? new Date(); - return now.getTime() - previous >= intervalMs(interval); +function writeAutomaticUpdateNotice(sink: OutputSink, latestVersion: string): void { + sink.writeMetadata([ + "", + terminalMetadata(`Update available: altertable ${latestVersion} (current ${VERSION}).`), + terminalMetadata(`Run ${recommendedInstallCommand(latestVersion)} to install it.`), + ]); } export async function maybeShowUpdateNotice(options: AutomaticNoticeOptions): Promise { - if (!shouldRunAutomaticUpdateCheck(options)) { - return; - } - - const sink = options.sink ?? getOutputSink(); try { - const result = await checkForUpdate({ - source: options.source, - fetchImpl: options.fetchImpl, - timeoutMs: options.timeoutMs ?? UpdaterConfig.timeoutsMs.automatic, - }); - if (!result.update_available) { + if (!shouldShowAutomaticUpdateNotice(options)) { return; } - sink.writeMetadata([ - "", - terminalMetadata( - `Update available: altertable ${result.latest_version} (current ${result.current_version}).`, - ), - terminalMetadata(`Run ${result.install_command} or ${UpdaterConfig.commands.selfUpdate}.`), - ]); - } catch (error) { - const message = error instanceof Error ? error.message : "Unknown update check error."; + const sink = options.sink ?? getOutputSink(); const previous = readUpdateState(); - tryWriteUpdateState({ - ...previous, - last_checked_at: new Date().toISOString(), - last_error: message, - }); + let latestVersion = previous.latest_version; + + if (shouldRunAutomaticUpdateCheck({ ...options, state: previous })) { + try { + const result = await checkForUpdate({ + source: options.source, + fetchImpl: options.fetchImpl, + timeoutMs: options.timeoutMs ?? UpdaterConfig.timeoutsMs.automatic, + }); + latestVersion = result.latest_version; + } catch (error) { + const message = error instanceof Error ? error.message : "Unknown update check error."; + tryWriteUpdateState({ + ...previous, + last_checked_at: new Date().toISOString(), + last_error: message, + }); + } + } + + if (latestVersion && compareVersions(latestVersion, VERSION) > 0) { + writeAutomaticUpdateNotice(sink, latestVersion); + } + } catch { + // Automatic update notices must never interrupt the command being run. } } export function formatUpdateResult(result: UpdateCheckResult): string { if (!result.update_available) { - return `altertable is up to date (${result.current_version}).`; - } - - return renderDocumentText( - document( - section( - text([ - `altertable ${result.latest_version} is available (current ${result.current_version}).`, - ]), - rows([ - { label: "Install:", value: result.install_command }, - { label: "Release:", value: result.release_url, linkifyUrls: true }, - ]), - ), - ), - { labelWidth: "Release:".length }, - ); + return `${terminalSuccess("Congrats!")} You're already on the latest version of altertable ${terminalMetadata(`(which is v${normalizeVersion(result.current_version)})`)}`; + } + + return [ + `A new version of altertable is available: v${normalizeVersion(result.latest_version)} ${terminalMetadata(`(current v${normalizeVersion(result.current_version)})`)}`, + `Run ${terminalAccent(result.install_command)} to install it.`, + ].join("\n"); } export function formatUpdateStatus(interval: UpdateCheckInterval, state: UpdateState): string { diff --git a/cli/tests/updater.test.ts b/cli/tests/updater.test.ts index 0a8a4da..924be84 100644 --- a/cli/tests/updater.test.ts +++ b/cli/tests/updater.test.ts @@ -7,6 +7,7 @@ import { join, resolve } from "node:path"; import { createHash } from "node:crypto"; import { buildMainCommand, resolveTopLevelCommandName } from "@/cli.ts"; import { CLI_PACKAGE_METADATA } from "@/package-metadata.ts"; +import { VERSION } from "@/version.ts"; import { checkForUpdate, compareVersions, @@ -343,6 +344,7 @@ describe("binary self-update", () => { }); test("recommends install commands from installation origin", () => { + process.env.ALTERTABLE_UPDATE_INSTALLER = "npm"; expect( recommendedInstallCommand(UPDATE_TEST_VERSION, { kind: "native-binary", @@ -354,7 +356,19 @@ describe("binary self-update", () => { kind: "package-manager", executablePath: "/usr/local/lib/node_modules/@altertable/cli/dist/cli.js", }), - ).toContain(`${UpdaterConfig.packageName}@${UPDATE_TEST_VERSION}`); + ).toBe(`npm install -g ${UpdaterConfig.packageName}@${UPDATE_TEST_VERSION}`); + expect( + recommendedInstallCommand(UPDATE_TEST_VERSION, { + kind: "source", + executablePath: "/repo/cli/src/cli.ts", + }), + ).toBe("altertable update --install"); + expect( + recommendedInstallCommand(UPDATE_TEST_VERSION, { + kind: "unknown", + executablePath: "/custom/altertable", + }), + ).toBe("altertable update --install"); }); test("parses and verifies SHA-256 checksums", () => { @@ -426,29 +440,33 @@ describe("binary self-update", () => { expect(readFileSync(targetPath, "utf8")).toBe(originalBinary); }); - test("auto install rejects source checkouts but explicit package-manager can run", async () => { + test("verifies a downloaded GitHub binary before replacing the target", async () => { + const targetPath = join(testHome, "altertable"); + const originalBinary = "#!/bin/sh\necho 1.0.0\n"; + const unexpectedBinary = "#!/bin/sh\necho 9.9.9\n"; + writeFileSync(targetPath, originalBinary, { mode: 0o755 }); + try { - await installCliUpdate(UPDATE_TEST_VERSION, { - verify: false, - installation: { - kind: "source", - executablePath: "/repo/cli/src/cli.ts", - reason: "test", - }, - spawnImpl: (() => { - throw new Error("should not spawn"); - }) as unknown as typeof import("node:child_process").spawnSync, + await installGitHubBinaryRelease(UPDATE_TEST_VERSION, { + targetPath, + platform: "linux-x64", + fetchImpl: githubBinaryFetch({ + binary: unexpectedBinary, + checksum: sha256(unexpectedBinary), + }), }); - throw new Error("Expected automatic install to reject source checkouts."); + throw new Error("Expected downloaded binary verification to fail."); } catch (error) { expect(error).toBeInstanceOf(Error); - expect((error as Error).message).toContain("Automatic CLI install is not supported"); + expect((error as Error).message).toContain("Downloaded altertable version is 9.9.9"); } + expect(readFileSync(targetPath, "utf8")).toBe(originalBinary); + }); + test("auto install uses the package manager for source checkouts", async () => { process.env.ALTERTABLE_UPDATE_INSTALLER = "npm"; const commands: string[] = []; const result = await installCliUpdate(UPDATE_TEST_VERSION, { - method: "package-manager", verify: false, stdio: "pipe", installation: { @@ -563,6 +581,19 @@ describe("automatic update checks", () => { }); }); + test("does not reject when automatic check policy cannot read config", async () => { + const invalidConfigHome = join(testHome, "not-a-directory"); + writeFileSync(invalidConfigHome, ""); + process.env.ALTERTABLE_CONFIG_HOME = invalidConfigHome; + + await maybeShowUpdateNotice({ + context: { debug: false, json: false, agent: false }, + commandName: "context", + stderrIsTTY: true, + fetchImpl: jsonFetch({ version: UPDATE_TEST_VERSION }), + }); + }); + test("writes automatic notices to metadata output", async () => { const stderr: string[] = []; const runtime = createCliRuntime({ debug: false, json: false, agent: false }); @@ -582,13 +613,103 @@ describe("automatic update checks", () => { expect(stderr.join("\n")).toContain(`Update available: altertable ${UPDATE_TEST_VERSION}`); expect(stderr.join("\n")).toContain("altertable update --install"); }); + + test("writes cached update notices without refreshing release metadata", async () => { + await checkForUpdate({ fetchImpl: jsonFetch({ version: UPDATE_TEST_VERSION }) }); + const stderr: string[] = []; + let fetchCalls = 0; + + await maybeShowUpdateNotice({ + context: { debug: false, json: false, agent: false }, + commandName: "help", + stderrIsTTY: true, + fetchImpl: (async () => { + fetchCalls += 1; + throw new Error("cached notices should not fetch"); + }) as unknown as typeof fetch, + sink: { + json: false, + debug: false, + writeStderr() {}, + writeJson() {}, + writeRaw() {}, + writeHuman() {}, + writeMetadata(lines) { + stderr.push(...lines); + }, + }, + }); + + expect(fetchCalls).toBe(0); + expect(stderr.join("\n")).toContain(`Update available: altertable ${UPDATE_TEST_VERSION}`); + }); + + test("falls back to a cached notice when a scheduled refresh fails", async () => { + await checkForUpdate({ fetchImpl: jsonFetch({ version: UPDATE_TEST_VERSION }) }); + const state = readUpdateState(); + const stderr: string[] = []; + + await maybeShowUpdateNotice({ + context: { debug: false, json: false, agent: false }, + commandName: "context", + stderrIsTTY: true, + now: new Date(Date.parse(state.last_checked_at ?? "") + 24 * 60 * 60 * 1000), + fetchImpl: (async () => { + throw new Error("network unavailable"); + }) as unknown as typeof fetch, + sink: { + json: false, + debug: false, + writeStderr() {}, + writeJson() {}, + writeRaw() {}, + writeHuman() {}, + writeMetadata(lines) { + stderr.push(...lines); + }, + }, + }); + + expect(stderr.join("\n")).toContain(`Update available: altertable ${UPDATE_TEST_VERSION}`); + expect(readUpdateState().last_error).toBe("Unable to check for CLI updates."); + }); }); describe("update command", () => { test("reports explicit target version without network", async () => { const output = await runUpdateCommand(["update", "--target-version", UPDATE_TEST_VERSION]); - expect(output.stdout[0]).toContain(`altertable ${UPDATE_TEST_VERSION} is available`); + expect(output.stdout[0]).toBe( + [ + `A new version of altertable is available: v${UPDATE_TEST_VERSION} (current v${VERSION})`, + `Run ${UpdaterConfig.commands.selfUpdate} to install it.`, + ].join("\n"), + ); + expect(output.stdout[0]).not.toContain("Install:"); + expect(output.stdout[0]).not.toContain("Release:"); + }); + + test("reports the latest installed version like Bun", async () => { + const output = await runUpdateCommand(["update", "--target-version", VERSION]); + + expect(output.stdout[0]).toBe( + `Congrats! You're already on the latest version of altertable (which is v${VERSION})`, + ); + }); + + test("clear-cache forces a fresh update check", async () => { + const originalFetch = globalThis.fetch; + globalThis.fetch = jsonFetch({ version: UPDATE_TEST_VERSION }); + try { + const output = await runUpdateCommand(["update", "--clear-cache"]); + + expect(output.stdout[0]).toContain( + `A new version of altertable is available: v${UPDATE_TEST_VERSION}`, + ); + expect(readUpdateState().latest_version).toBe(UPDATE_TEST_VERSION); + } finally { + globalThis.fetch = originalFetch; + } }); test("configures automatic check interval", async () => { @@ -608,9 +729,11 @@ describe("update command", () => { UPDATE_TEST_VERSION, ]); - expect(output.stdout[0]).toContain(`altertable ${UPDATE_TEST_VERSION} is available`); - expect(output.stdout[0]).toContain( - `Release: ${releaseUrlForSource(source, UPDATE_TEST_VERSION)}`, + expect(output.stdout[0]).toBe( + [ + `A new version of altertable is available: v${UPDATE_TEST_VERSION} (current v${VERSION})`, + `Run ${UpdaterConfig.commands.selfUpdate} to install it.`, + ].join("\n"), ); } });