From 438d4ca616ebc46bd86562b769ba8f587ac184b0 Mon Sep 17 00:00:00 2001 From: Gravirei <147187533+Gravirei@users.noreply.github.com> Date: Fri, 30 Oct 2026 05:54:03 +0600 Subject: [PATCH] fix(update): restore getLatestVersion in published build (#2430) * fix(update): restore getLatestVersion in published build Remove the utils/autoUpdater stub block from no-telemetry-plugin.ts that replaced getLatestVersion() with a no-op returning null, causing 'openclaude update' to fail with 'Failed to check for updates'. The real implementation runs 'npm view' (5s timeout) with a direct registry.npmjs.org HTTP fallback. The 9 remaining telemetry/analytics stubs are preserved. Fixes #2424 * fix(update): pass selected channel version and close lock race - Pass latestVersion to installGlobalPackage in the standalone global CLI path so stable-channel installs install the resolved version instead of @latest. - Check isAutoUpdaterDisabled before getLatestVersion in the background AutoUpdater callback so CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC suppresses lookups at mount and on the 30-minute timer. - Make acquireLock atomic: verify owning PID is alive before stale recovery, and write to a unique temp file then rename instead of stat/unlink/write, closing the TOCTOU race that admitted two owners. Co-Authored-By: Claude * fix(update): preserve exclusive locks and package manager ownership * test(update): use a file URL for the Node preload * fix(update): retain uncertain installer ownership and emulate Windows commands * fix: preserve safe updater recovery after failed termination * fix(update): retain ownership after installer signal termination * test(update): preserve lifecycle worker on Windows * fix(test): bound updater fixture readiness wait --------- Co-authored-by: Gravirei Co-authored-by: Claude Co-authored-by: jatmn --- README.md | 8 + scripts/fixtures/auto-updater-disabled.ts | 70 ++++++++ scripts/fixtures/update-recovery.ts | 66 +++++++ scripts/fixtures/updater-preload.mjs | 143 +++++++++++++++ scripts/fixtures/updater-process.mjs | 47 +++++ scripts/no-telemetry-plugin.ts | 18 -- scripts/updater.integration.test.ts | 204 ++++++++++++++++++++++ src/cli/update.ts | 11 +- src/commands/update/update.test.ts | 15 ++ src/commands/update/update.tsx | 14 +- src/components/AutoUpdater.test.ts | 24 +++ src/components/AutoUpdater.tsx | 9 +- src/utils/autoUpdater.ts | 112 +++++------- 13 files changed, 644 insertions(+), 97 deletions(-) create mode 100644 scripts/fixtures/auto-updater-disabled.ts create mode 100644 scripts/fixtures/update-recovery.ts create mode 100644 scripts/fixtures/updater-preload.mjs create mode 100644 scripts/fixtures/updater-process.mjs create mode 100644 scripts/updater.integration.test.ts diff --git a/README.md b/README.md index e02585c15..b2a16747f 100644 --- a/README.md +++ b/README.md @@ -179,6 +179,14 @@ npm view @twigpine/openclaude dist-tags npm install -g @twigpine/openclaude@latest ``` +**Update-lock recovery:** Global updates use `.update.lock` in the OpenClaude +configuration directory (`OPENCLAUDE_CONFIG_DIR`, or `~/.openclaude`). Existing +locks are never removed automatically, even when old, to avoid overlapping +package-manager installations. If an update was interrupted, first confirm that +all OpenClaude update processes and their package-manager children have stopped; +then remove only `.update.lock` from that directory and retry `openclaude update`. +Do not remove the lock while an update is running. + ### Start ```bash diff --git a/scripts/fixtures/auto-updater-disabled.ts b/scripts/fixtures/auto-updater-disabled.ts new file mode 100644 index 000000000..597895cbf --- /dev/null +++ b/scripts/fixtures/auto-updater-disabled.ts @@ -0,0 +1,70 @@ +// Isolated module mocks keep hook/callback probes out of the shared test runner. +import { mock } from 'bun:test' +import * as React from 'react' +import { withMockMacro } from '../../src/test/mockMacro.js' + +const effects: Array<() => void> = [] +const pending: Promise[] = [] +let interval: () => Promise +let disabled = true +let latest = 0 +let maximum = 0 +mock.module('react', () => ({ + ...React, + useState: (initial: unknown) => [initial, () => {}], + useRef: (current: unknown) => ({ current }), + useEffect: (effect: () => void) => { effects.push(effect) }, + useCallback: (callback: () => Promise) => () => { + const result = callback() + pending.push(result) + return result + }, +})) +mock.module('usehooks-ts', () => ({ useInterval: (callback: () => Promise) => { interval = callback } })) +mock.module('../../src/hooks/useUpdateNotification.js', () => ({ useUpdateNotification: () => null })) +mock.module('../../src/ink.js', () => ({ Box: () => null, Text: () => null })) +mock.module('../../src/services/analytics/index.js', () => ({ logEvent: () => {} })) +mock.module('../../src/utils/debug.js', () => ({ logForDebugging: () => {} })) +mock.module('../../src/utils/config.js', () => ({ isAutoUpdaterDisabled: () => disabled, getGlobalConfig: () => ({}) })) +mock.module('../../src/utils/settings/settings.js', () => ({ getInitialSettings: () => ({ autoUpdatesChannel: 'stable' }) })) +mock.module('../../src/utils/doctorDiagnostic.js', () => ({ getCurrentInstallationType: async () => 'npm-global' })) +mock.module('../../src/utils/localInstaller.js', () => ({ localInstallationExists: async () => false, installOrUpdateClaudePackage: async () => 'success' })) +mock.module('../../src/utils/nativeInstaller/index.js', () => ({ removeInstalledSymlink: async () => {} })) +mock.module('../../src/utils/autoUpdater.js', () => ({ + UPDATE_RECOVERY_MESSAGE: 'Lock recovery required', + getLatestVersion: async () => { latest++; return '0.33.0' }, + getMaxVersion: async () => { maximum++; return undefined }, + installGlobalPackage: async () => { throw new Error('Unexpected automatic install') }, + shouldSkipVersion: () => false, +})) + +try { + const { AutoUpdater } = await import('../../src/components/AutoUpdater.js') + await withMockMacro({ VERSION: '0.33.0', PACKAGE_URL: '@twigpine/openclaude' }, async () => { + AutoUpdater({ isUpdating: false, onChangeIsUpdating: () => {}, onAutoUpdaterResult: () => {}, autoUpdaterResult: null, showSuccessMessage: false, verbose: false }) + effects.forEach(effect => effect()) + await Promise.all(pending) + const initial = { latest, maximum } + await interval!() + const timer = { latest, maximum } + disabled = false + await interval!() + const text = (node: React.ReactNode): string => { + if (Array.isArray(node)) return node.map(text).join('') + if (typeof node === 'string') return node + return React.isValidElement<{ children?: React.ReactNode }>(node) ? text(node.props.children) : '' + } + const notice = (status: 'termination_uncertain' | 'install_failed') => text(AutoUpdater({ + isUpdating: false, onChangeIsUpdating: () => {}, onAutoUpdaterResult: () => {}, + autoUpdaterResult: { version: '0.34.0', status }, showSuccessMessage: false, verbose: false, + })) + const recovery = notice('termination_uncertain') + const ordinary = notice('install_failed') + console.log(JSON.stringify({ initial, timer, enabled: { latest, maximum }, + recoveryNotice: recovery.includes('Lock recovery required') && !recovery.includes('npm i -g'), + ordinaryNotice: ordinary.includes('npm i -g @twigpine/openclaude@latest'), + })) + }) +} finally { + mock.restore() +} diff --git a/scripts/fixtures/update-recovery.ts b/scripts/fixtures/update-recovery.ts new file mode 100644 index 000000000..e9ee8b78a --- /dev/null +++ b/scripts/fixtures/update-recovery.ts @@ -0,0 +1,66 @@ +// Isolated consumer probe: real command callbacks/JSX, injected hooks and installer result. +import { mock } from 'bun:test' +import * as React from 'react' +import { withMockMacro } from '../../src/test/mockMacro.js' + +let state: unknown +let settled = () => {} +let status: 'termination_uncertain' | 'install_failed' = 'termination_uncertain' +let element: React.ReactElement | undefined +const effects: Array<() => void> = [] +mock.module('react', () => ({ ...React, + useState: () => [state, (next: unknown) => { + state = next + if (next && typeof next === 'object' && 'type' in next && next.type === 'error') settled() + }], + useRef: (current: unknown) => ({ current }), + useEffect: (effect: () => void) => { effects.push(effect) }, +})) +mock.module('../../src/ink.js', () => ({ Box: () => null, Text: () => null, + render: async (node: React.ReactElement) => { element = node; return { unmount() {} } }, +})) +mock.module('../../src/components/design-system/StatusIcon.js', () => ({ StatusIcon: () => null })) +mock.module('../../src/utils/debug.js', () => ({ logForDebugging() {} })) +mock.module('../../src/utils/config.js', () => ({ getGlobalConfig: () => ({ installMethod: 'global' }) })) +mock.module('../../src/utils/globalPackageManager.js', () => ({ detectGlobalPackageManager: async () => 'npm' })) +mock.module('../../src/utils/localInstaller.js', () => ({ installOrUpdateClaudePackage: async () => 'success' })) +mock.module('../../src/utils/nativeDistribution.js', () => ({ hasNativeDistribution: () => false })) +mock.module('../../src/utils/nativeInstaller/index.js', () => ({ installLatest: async () => ({}), removeInstalledSymlink: async () => {} })) +mock.module('../../src/utils/updateStrategy.js', () => ({ resolveUpdateStrategy: async () => ({ action: 'npm', method: 'global' }) })) +mock.module('../../src/utils/autoUpdater.js', () => ({ getLatestVersion: async () => '0.34.0', + installGlobalPackage: async () => status, UPDATE_RECOVERY_MESSAGE: 'Lock recovery required', +})) + +function text(node: React.ReactNode): string { + if (Array.isArray(node)) return node.map(text).join('') + if (typeof node === 'string') return node + return React.isValidElement<{ children?: React.ReactNode }>(node) ? text(node.props.children) : '' +} + +try { + await withMockMacro({ PACKAGE_URL: '@twigpine/openclaude', DISPLAY_VERSION: '0.33.0' }, async () => { + const { call } = await import('../../src/commands/update/update.js') + const results = [] + for (const next of ['termination_uncertain', 'install_failed'] as const) { + status = next + state = { type: 'checking' } + effects.length = 0 + const changed = new Promise(resolve => { settled = resolve }) + let completed = (_message: string) => {} + const completion = new Promise(resolve => { completed = resolve }) + await call(message => completed(message), undefined, '0.34.0') + if (!element || typeof element.type !== 'function') throw new Error('Update component was not rendered') + const component = element.type as React.FunctionComponent + await component(element.props) + effects[0]!() + await changed + effects.length = 0 + const rendered = text(await component(element.props)) + effects.at(-1)!() + const message = await completion + results.push({ status, recovery: rendered.includes('Lock recovery required'), + manual: rendered.includes('npm install -g'), recoveryOnDone: message.includes('Lock recovery required') }) + } + console.log(JSON.stringify(results)) + }) +} finally { mock.restore() } diff --git a/scripts/fixtures/updater-preload.mjs b/scripts/fixtures/updater-preload.mjs new file mode 100644 index 000000000..e51994f57 --- /dev/null +++ b/scripts/fixtures/updater-preload.mjs @@ -0,0 +1,143 @@ +// External commands are intercepted; updater code and lock/permission filesystem +// operations run unchanged in the built CLI under Node. +import childProcess from 'node:child_process' +import fs from 'node:fs' +import promises from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import http from 'node:http' +import https from 'node:https' +import { createRequire, syncBuiltinESMExports } from 'node:module' +import { fileURLToPath } from 'node:url' + +const root = process.env.UPDATER_TEST_ROOT +const owner = process.env.UPDATER_TEST_OWNER || 'npm' +const log = event => fs.appendFileSync(path.join(root, 'events'), JSON.stringify(event) + '\n') +os.homedir = () => path.join(root, 'home') +process.argv[1] = owner === 'bun' + ? path.join(root, 'npm/bun/install/global/node_modules/@twigpine/openclaude/cli.mjs') + : path.join(root, 'npm/node_modules/@twigpine/openclaude/cli.mjs') + +// Exercise the real Windows parser even on POSIX; this preload is process-local. +let windowsParse +if (process.env.UPDATER_TEST_WINDOWS_WRAPPERS) { + fs.mkdirSync(path.join(root, 'bin'), { recursive: true }) + fs.writeFileSync(path.join(root, 'bin/bun.exe'), '') + const platform = Object.getOwnPropertyDescriptor(process, 'platform') + try { + Object.defineProperty(process, 'platform', { value: 'win32' }) + windowsParse = createRequire(import.meta.url)('cross-spawn')._parse + } finally { + Object.defineProperty(process, 'platform', platform) + } +} + +// A missed command must fail the test instead of reaching a live registry. +http.request = https.request = () => { throw new Error('Unexpected updater HTTP request') } + +const setTimeout = globalThis.setTimeout +let shortenInstallTimeout = false +globalThis.setTimeout = (callback, delay, ...args) => { + if (shortenInstallTimeout && delay === 600_000) { + shortenInstallTimeout = false + return setTimeout(callback, 500, ...args) + } + return setTimeout(callback, delay, ...args) +} + +const spawn = childProcess.spawn +childProcess.spawn = (file, args = [], options = {}) => { + const availability = /which (npm|bun)$/.exec([file, ...args].join(' ')) + if (windowsParse && (file === 'npm' || file === 'bun')) { + const pathExt = process.env.PATHEXT + try { + // Bun ships a native .exe, so its Windows route must stay shell-free. + if (file === 'bun') process.env.PATHEXT = '.exe' + const parsed = windowsParse(file, args, file === 'bun' + ? { ...options, env: { ...process.env, PATH: path.join(root, 'bin') } } + : options) + log({ type: 'parsed-command', file: parsed.command, args: parsed.args, resolved: parsed.file }) + file = parsed.command + args = parsed.args + options = { ...parsed.options, env: options.env } + } finally { + if (pathExt === undefined) delete process.env.PATHEXT + else process.env.PATHEXT = pathExt + } + } else if (windowsParse && availability) { + file = 'cmd.exe' + args = ['/d', '/s', '/c', `"where.exe ${availability[1]}"`] + } + log({ type: 'command', file, args }) + if (path.win32.basename(file).toLowerCase() === 'cmd.exe' && args.includes('/c')) { + // Only fixture-owned, whitespace-free arguments are admitted below. Decode + // cmd quoting/caret layers; do not execute or interpret arbitrary shell input. + const words = args.at(-1).replace(/["^]/g, '').trim().split(/\s+/) + if (['npm', 'bun', 'where.exe'].includes(words[0])) [file, ...args] = words + } + const command = [file, ...args].join(' ') + let output = '' + let code = 0 + let install = false + if (/(?:which|where\.exe) (npm|bun)$/.test(command)) { + output = path.join(root, 'bin', command.endsWith('bun') ? 'bun' : 'npm') + } else if (file === 'npm' && args[0] === 'view') { + output = process.env.UPDATER_TEST_VERSION || '0.34.0' + log({ type: 'lookup', args }) + } else if (file === 'npm' && args.includes('prefix')) { + output = path.join(root, 'npm-prefix') + } else if (file === 'npm' && args[0] === 'root') { + output = path.join(root, 'npm/node_modules') + } else if (file === 'bun' && args[0] === 'pm') { + output = path.join(root, 'npm/bun/bin') + } else if ((file === 'npm' && args[0] === 'install') || (file === 'bun' && args[0] === 'add')) { + install = true + log({ type: 'install', file, args, pid: process.pid }) + code = Number(process.env.UPDATER_TEST_INSTALL_EXIT || 0) + shortenInstallTimeout = process.env.UPDATER_TEST_UNCERTAIN?.startsWith('timeout') + } else { + code = 1 // Never run incidental diagnostics or completion commands on the host. + } + const mode = install + ? process.env.UPDATER_TEST_UNCERTAIN?.replace('-kill-failure', '') || (process.env.UPDATER_TEST_HOLD ? 'hold' : 'result') + : 'result' + const child = spawn(process.execPath, [ + fileURLToPath(new URL('./updater-process.mjs', import.meta.url)), mode, String(code), output, + ], { ...options, shell: false, windowsVerbatimArguments: false }) + if (install && mode.startsWith('SIG')) { + // Signal only the manager after its independent lifecycle worker is active. + child.stdout.once('data', () => child.kill(mode)) + } + if (install && process.env.UPDATER_TEST_UNCERTAIN?.endsWith('-kill-failure')) { + // Exercise Node's real synchronous kill-error event before the wrapper's + // timeout/output-limit result settles, without signaling a foreign process. + child._handle.kill = () => { + log({ type: 'kill-error', args: [] }) + return process.binding('uv').UV_EPERM + } + } + return child +} + +const writeFile = promises.writeFile +let firstLockWrite = true +promises.writeFile = async (file, ...args) => { + if (String(file).includes('.update.lock') && firstLockWrite) { + firstLockWrite = false + if (process.env.UPDATER_TEST_BARRIER) { + fs.writeFileSync(path.join(root, `ready-${process.pid}`), '') + const deadline = Date.now() + 8000 + while (fs.readdirSync(root).filter(name => name.startsWith('ready-')).length < 2) { + if (Date.now() > deadline) throw new Error('Lock contenders did not reach the barrier') + await new Promise(resolve => setTimeout(resolve, 5)) + } + } + if (process.env.UPDATER_TEST_MISSING_DIRECTORY) { + // The CLI normally initializes config before installation. Exercise the + // installer's missing-directory retry without removing that configuration. + throw Object.assign(new Error('Simulated missing config directory'), { code: 'ENOENT' }) + } + } + return writeFile(file, ...args) +} +syncBuiltinESMExports() diff --git a/scripts/fixtures/updater-process.mjs b/scripts/fixtures/updater-process.mjs new file mode 100644 index 000000000..f2ee31841 --- /dev/null +++ b/scripts/fixtures/updater-process.mjs @@ -0,0 +1,47 @@ +// Package-manager stand-in. Lifecycle work deliberately outlives its manager. +import { spawn } from 'node:child_process' +import fs from 'node:fs' +import path from 'node:path' +import { fileURLToPath } from 'node:url' + +const [mode, status, output] = process.argv.slice(2) +const root = process.env.UPDATER_TEST_ROOT +const released = () => fs.existsSync(path.join(root, 'release')) +// Cover startup too: cleanup can remove the heartbeat before we observe it. +// Do not keep completed result-mode commands alive just for this safety timer. +const deadline = setTimeout(() => { process.exit(124) }, 12_000).unref() + +function hold(tick) { + const timer = setInterval(() => { + if (released()) { + clearInterval(timer) + clearTimeout(deadline) + if (mode === 'lifecycle') fs.writeFileSync(path.join(root, 'lifecycle-exited'), '') + process.exitCode = Number(status) + } else { + tick?.() + } + }, 10) +} + +if (mode === 'result') { + fs.writeSync(1, output) + process.exitCode = Number(status) +} else if (mode === 'lifecycle') { + fs.writeFileSync(path.join(root, 'lifecycle-ready'), '') + hold(() => fs.appendFileSync(path.join(root, 'heartbeat'), 'x')) +} else { + if (mode === 'timeout' || mode === 'output-limit' || mode.startsWith('SIG')) { + spawn(process.execPath, [fileURLToPath(import.meta.url), 'lifecycle', '0'], { + stdio: 'ignore', env: process.env, + // Avoid the manager's kill-on-exit job on Windows; retain POSIX grouping. + detached: process.platform === 'win32', + }) + while (!fs.existsSync(path.join(root, 'heartbeat'))) { + await new Promise(resolve => setTimeout(resolve, 5)) + } + if (mode === 'output-limit') fs.writeSync(1, Buffer.alloc(1_100_000, 'x')) + if (mode.startsWith('SIG')) fs.writeSync(1, 'lifecycle started\n') + } + hold() +} diff --git a/scripts/no-telemetry-plugin.ts b/scripts/no-telemetry-plugin.ts index 8aa9368d2..34e32d862 100644 --- a/scripts/no-telemetry-plugin.ts +++ b/scripts/no-telemetry-plugin.ts @@ -7,7 +7,6 @@ * Analytics and telemetry modules have been replaced at the source level and * no longer need build-time stubs. This plugin now only covers: * - * - Auto-updater (phones home to GCS + npm) * - Plugin fetch telemetry * - Transcript / feedback sharing * - Internal employee logging @@ -22,23 +21,6 @@ import type { BunPlugin } from 'bun' // Module path (relative to src/, without extension) → stub source const stubs: Record = { - // ─── Auto-updater (phones home to GCS + npm) ────────────────── - - 'utils/autoUpdater': ` -export async function assertMinVersion() {} -export async function getMaxVersion() { return undefined; } -export async function getMaxVersionMessage() { return undefined; } -export function shouldSkipVersion() { return true; } -export function getLockFilePath() { return '/tmp/openclaude-update.lock'; } -export async function checkGlobalInstallPermissions() { return { hasPermissions: false, npmPrefix: null }; } -export async function getLatestVersion() { return null; } -export async function getNpmDistTags() { return { latest: null, stable: null }; } -export async function getLatestVersionFromGcs() { return null; } -export async function getGcsDistTags() { return { latest: null, stable: null }; } -export async function getVersionHistory() { return []; } -export async function installGlobalPackage() { return 'success'; } -`, - // ─── Plugin fetch telemetry (not the marketplace itself) ─────── 'utils/plugins/fetchTelemetry': ` diff --git a/scripts/updater.integration.test.ts b/scripts/updater.integration.test.ts new file mode 100644 index 000000000..e9dbec9b6 --- /dev/null +++ b/scripts/updater.integration.test.ts @@ -0,0 +1,204 @@ +import { describe, expect, test } from 'bun:test' +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, utimesSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, win32 } from 'node:path' +import { pathToFileURL } from 'node:url' + +const repo = join(import.meta.dir, '..') +const cli = join(repo, 'dist/cli.mjs') +const currentVersion = JSON.parse(readFileSync(join(repo, 'package.json'), 'utf8')).version as string +const targetVersion = `${Number(currentVersion.split('.')[0]) + 1}.0.0` +const preload = join(import.meta.dir, 'fixtures/updater-preload.mjs') + +type Event = { type: string; file?: string; args: string[]; pid?: number; resolved?: string } + +function fixture(owner = 'npm') { + if (!existsSync(cli)) throw new Error('Run bun run build before updater integration tests') + const root = mkdtempSync(join(tmpdir(), 'updater-integration-')) + for (const name of ['home', 'config', 'npm/bun/bin', ...(owner === 'npm' ? ['npm-prefix'] : [])]) { + mkdirSync(join(root, name), { recursive: true }) + } + writeFileSync(join(root, 'config/settings.json'), JSON.stringify({ autoUpdatesChannel: 'stable' })) + const children: ReturnType[] = [] + return { + root, + lock: join(root, 'config/.update.lock'), + events: (): Event[] => existsSync(join(root, 'events')) + ? readFileSync(join(root, 'events'), 'utf8').trim().split('\n').filter(Boolean).map(line => JSON.parse(line)) + : [], + start(extraEnv: Record = {}) { + const child = Bun.spawn(['node', '--import', pathToFileURL(preload).href, cli, 'update'], { + cwd: root, + env: { + ...process.env, + NODE_OPTIONS: '', NODE_ENV: 'production', NO_COLOR: '1', CI: '1', + OPENCLAUDE_CONFIG_DIR: join(root, 'config'), + BUN_INSTALL: join(root, 'npm/bun'), + UPDATER_TEST_ROOT: root, UPDATER_TEST_OWNER: owner, UPDATER_TEST_VERSION: targetVersion, + ...extraEnv, + }, + stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', timeout: 15_000, + }) + children.push(child) + return (async () => ({ + code: await child.exited, + stdout: await new Response(child.stdout).text(), + stderr: await new Response(child.stderr).text(), + }))() + }, + async close() { + writeFileSync(join(root, 'release'), '') + for (const child of children) { + await Promise.race([child.exited, Bun.sleep(500)]) + if (child.exitCode === null) child.kill() + await child.exited + } + if (existsSync(join(root, 'lifecycle-ready'))) { + await waitFor(() => existsSync(join(root, 'lifecycle-exited'))) + } + rmSync(root, { recursive: true, force: true }) + }, + } +} + +async function waitFor(predicate: () => boolean) { + const deadline = Date.now() + 10_000 + while (!predicate()) { + if (Date.now() >= deadline) throw new Error('Timed out waiting for updater readiness') + await Bun.sleep(10) + } +} + +// Child deadlines are 15 seconds; allow time for barriers, assertions and teardown. +describe('published updater under Node', () => { + test('bounds fixture startup when cleanup removes the readiness directory', async () => { + const root = mkdtempSync(join(tmpdir(), 'updater-readiness-')) + rmSync(root, { recursive: true }) + // The lifecycle worker cannot publish a heartbeat after cleanup. The manager + // must reach its own 12-second deadline before this 15-second outer watchdog. + const child = Bun.spawn(['node', join(import.meta.dir, 'fixtures/updater-process.mjs'), 'timeout', '0'], { + cwd: repo, + env: { ...process.env, NODE_OPTIONS: '', UPDATER_TEST_ROOT: root }, + stdin: 'ignore', stdout: 'ignore', stderr: 'ignore', timeout: 15_000, + }) + try { + expect(await child.exited).toBe(124) + } finally { + if (child.exitCode === null) child.kill() + await child.exited + rmSync(root, { recursive: true, force: true }) + } + }, 20_000) + + test('reports the installed version as current without installing', async () => { + const f = fixture() + try { + const result = await f.start({ UPDATER_TEST_VERSION: currentVersion }) + expect(result.code).toBe(0) + expect(result.stdout).toContain(`up to date (${currentVersion})`) + expect(f.events().filter(e => e.type === 'lookup')).toHaveLength(1) + expect(f.events().filter(e => e.type === 'install')).toHaveLength(0) + expect(existsSync(f.lock)).toBe(false) + } finally { await f.close() } + }, 20_000) + + test.each(['npm', 'bun'].flatMap(owner => [false, true].map(windows => ({ owner, windows }))))('installs the selected stable version with $owner (Windows wrappers: $windows) and releases the lock', async ({ owner, windows }) => { + const f = fixture(owner) + try { + for (let attempt = 0; attempt < 2; attempt++) { + const result = await f.start(windows ? { UPDATER_TEST_WINDOWS_WRAPPERS: '1' } : {}) + expect(result, JSON.stringify(result)).toMatchObject({ code: 0 }) + expect(result.stdout).toContain(`to version ${targetVersion}`) + expect(existsSync(f.lock)).toBe(false) + } + const events = f.events() + expect(events.filter(e => e.type === 'lookup').every(e => e.args.includes('@twigpine/openclaude@stable'))).toBe(true) + expect(events.filter(e => e.type === 'install').map(e => [e.file, e.args])).toEqual([ + [owner, [owner === 'npm' ? 'install' : 'add', '-g', `@twigpine/openclaude@${targetVersion}`]], + [owner, [owner === 'npm' ? 'install' : 'add', '-g', `@twigpine/openclaude@${targetVersion}`]], + ]) + if (owner === 'bun') expect(existsSync(join(f.root, 'npm-prefix'))).toBe(false) + if (windows) { + const spec = `@twigpine/openclaude@${targetVersion}` + const dispatched = events.filter(e => e.type === 'command' && e.args.join(' ').includes(spec)) + const isCmd = (e: Event) => win32.basename(e.file || '').toLowerCase() === 'cmd.exe' + expect(dispatched).toHaveLength(2) + expect(dispatched.every(e => owner === 'npm' ? isCmd(e) : e.file === 'bun')).toBe(true) + expect(events.filter(e => e.type === 'command' && isCmd(e) && e.args.join(' ').includes('@twigpine/openclaude@stable'))).toHaveLength(2) + // On Windows, production cross-spawn has already wrapped npm before + // interception. Only the POSIX simulation generates parser metadata. + if (process.platform !== 'win32') { + const parsed = events.filter(e => e.type === 'parsed-command' && e.args.join(' ').includes(spec)) + expect(parsed).toHaveLength(2) + expect(dispatched.map(e => [e.file, e.args])).toEqual(parsed.map(e => [e.file, e.args])) + if (owner === 'bun') expect(parsed.every(e => e.resolved === join(f.root, 'bin/bun.exe'))).toBe(true) + } + } + } finally { await f.close() } + }, 40_000) + + test.each([false, true])('allows one simultaneous installer (missing-directory retry: %s)', async missingDirectory => { + const f = fixture() + try { + const env = { UPDATER_TEST_BARRIER: '1', UPDATER_TEST_HOLD: '1', ...(missingDirectory ? { UPDATER_TEST_MISSING_DIRECTORY: '1' } : {}) } + const first = f.start(env) + const second = f.start(env) + await waitFor(() => f.events().some(e => e.type === 'install')) + // A loser must exit while the winner is still holding the install open. + const loser = await Promise.race([first, second]) + expect(loser.code).toBe(1) + expect(f.events().filter(e => e.type === 'install')).toHaveLength(1) + writeFileSync(join(f.root, 'release'), '') + const results = await Promise.all([first, second]) + expect(results.map(r => r.code).sort()).toEqual([0, 1]) + expect(existsSync(f.lock)).toBe(false) + expect(readdirSync(join(f.root, 'config')).some(name => name.includes('.update.lock'))).toBe(false) + } finally { await f.close() } + }, 40_000) + + test.each(['fresh-live', 'aged-live', 'dead', 'malformed'])('preserves an existing %s lock against two contenders', async state => { + const f = fixture() + try { + const pid = state === 'malformed' ? '' : state === 'dead' ? '2147483647' : String(process.pid) + writeFileSync(f.lock, pid) + if (state !== 'fresh-live') utimesSync(f.lock, new Date(0), new Date(0)) + const results = await Promise.all([f.start(), f.start()]) + expect(results.map(r => r.code)).toEqual([1, 1]) + expect(results.every(r => r.stderr.includes('Could not acquire the update lock'))).toBe(true) + expect(f.events().filter(e => e.type === 'install')).toHaveLength(0) + expect(readFileSync(f.lock, 'utf8')).toBe(pid) + } finally { await f.close() } + }, 40_000) + + test('releases after a failed install so another update can run', async () => { + const f = fixture() + try { + expect((await f.start({ UPDATER_TEST_INSTALL_EXIT: '1' })).code).toBe(1) + expect(existsSync(f.lock)).toBe(false) + expect((await f.start()).code).toBe(0) + } finally { await f.close() } + }, 40_000) + + test.each(['timeout', 'output-limit', 'timeout-kill-failure', 'output-limit-kill-failure', 'SIGTERM', 'SIGKILL'])('retains the lock while lifecycle work survives an installer %s', async failure => { + const f = fixture() + try { + const first = await f.start({ UPDATER_TEST_UNCERTAIN: failure }) + expect(first.code, first.stderr).toBe(1) + expect(first.stderr).toContain('Installer may still be running') + expect(first.stderr).toContain('README update-lock recovery instructions') + expect(first.stderr).not.toContain('npm install') + const heartbeat = () => existsSync(join(f.root, 'heartbeat')) ? readFileSync(join(f.root, 'heartbeat'), 'utf8').length : 0 + const before = heartbeat() + await waitFor(() => heartbeat() > before) + if (failure.endsWith('-kill-failure')) expect(f.events().some(e => e.type === 'kill-error')).toBe(true) + expect(existsSync(f.lock)).toBe(true) + const install = f.events().filter(e => e.type === 'install') + expect(install).toHaveLength(1) + expect(readFileSync(f.lock, 'utf8')).toBe(String(install[0]!.pid)) + const second = await f.start() + expect(second.code).toBe(1) + expect(second.stderr).toContain('Could not acquire the update lock') + expect(f.events().filter(e => e.type === 'install')).toHaveLength(1) + } finally { await f.close() } + }, 40_000) +}) diff --git a/src/cli/update.ts b/src/cli/update.ts index 426635ccf..6da40a590 100644 --- a/src/cli/update.ts +++ b/src/cli/update.ts @@ -4,6 +4,7 @@ import { getLatestVersion, type InstallStatus, installGlobalPackage, + UPDATE_RECOVERY_MESSAGE, } from 'src/utils/autoUpdater.js' import { regenerateCompletionCache } from 'src/utils/completionCache.js' import { @@ -410,7 +411,7 @@ export async function update() { status = await installOrUpdateClaudePackage(channel) } else { logForDebugging('update: Calling installGlobalPackage() for global update') - status = await installGlobalPackage() + status = await installGlobalPackage(latestVersion) } logForDebugging(`update: Installation status: ${status}`) @@ -439,6 +440,10 @@ export async function update() { } await gracefulShutdown(1) break + case 'termination_uncertain': + process.stderr.write(`Error: ${UPDATE_RECOVERY_MESSAGE}\n`) + await gracefulShutdown(1) + break case 'install_failed': process.stderr.write('Error: Failed to install update\n') if (useLocalUpdate) { @@ -453,9 +458,9 @@ export async function update() { break case 'in_progress': process.stderr.write( - 'Error: Another instance is currently performing an update\n', + 'Error: Could not acquire the update lock\n', ) - process.stderr.write('Please wait and try again later\n') + process.stderr.write('Wait for any active update to finish. For interrupted updates, see the README update-lock recovery instructions.\n') await gracefulShutdown(1) break } diff --git a/src/commands/update/update.test.ts b/src/commands/update/update.test.ts index 10cb2dbd8..abc2180bc 100644 --- a/src/commands/update/update.test.ts +++ b/src/commands/update/update.test.ts @@ -66,3 +66,18 @@ describe('removeStaleNativeLauncherForNpmUpdate', () => { expect(removed).toBe(1) }) }) + +test('retained-lock errors show recovery in the view and completion without a manual-install bypass', async () => { + const child = Bun.spawn([process.execPath, 'run', `${import.meta.dir}/../../../scripts/fixtures/update-recovery.ts`], { + cwd: `${import.meta.dir}/../../..`, env: { ...process.env, NODE_ENV: 'production' }, + stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', timeout: 15_000, + }) + const [code, stdout, stderr] = await Promise.all([ + child.exited, new Response(child.stdout).text(), new Response(child.stderr).text(), + ]) + expect(code, stderr).toBe(0) + expect(JSON.parse(stdout)).toEqual([ + { status: 'termination_uncertain', recovery: true, manual: false, recoveryOnDone: true }, + { status: 'install_failed', recovery: false, manual: true, recoveryOnDone: false }, + ]) +}, 20_000) diff --git a/src/commands/update/update.tsx b/src/commands/update/update.tsx index e2fd89f3c..bf9ab4ee8 100644 --- a/src/commands/update/update.tsx +++ b/src/commands/update/update.tsx @@ -5,6 +5,7 @@ import { Box, render, Text } from '../../ink.js' import { getLatestVersion, installGlobalPackage, + UPDATE_RECOVERY_MESSAGE, } from '../../utils/autoUpdater.js' import { getGlobalConfig, @@ -45,7 +46,7 @@ type UpdateState = | { type: 'up-to-date'; version: string } | { type: 'updating'; version: string; via: string } | { type: 'success'; version: string; via: string } - | { type: 'error'; message: string } + | { type: 'error'; message: string; requiresLockRecovery?: boolean } export function PackageManagerUpdateGuidance({ manager, @@ -198,11 +199,14 @@ function Update({ onDone, force, target }: UpdateProps): React.ReactNode { 'Insufficient permissions for the install. Re-run with the right permissions (e.g. sudo) or fix your install directory ownership.', }) break + case 'termination_uncertain': + setState({ type: 'error', message: UPDATE_RECOVERY_MESSAGE, requiresLockRecovery: true }) + break case 'in_progress': setState({ type: 'error', message: - 'Another update is already in progress. Try again in a moment.', + 'Could not acquire the update lock. Wait for any active update to finish; for interrupted updates, see the README update-lock recovery instructions.', }) break default: @@ -316,11 +320,11 @@ function Update({ onDone, force, target }: UpdateProps): React.ReactNode { Update failed {state.message} - + {!state.requiresLockRecovery && You can update manually, e.g. npm install -g {PACKAGE_URL}@latest - + } )} @@ -347,7 +351,7 @@ function terminalDoneMessage(state: UpdateState): { case 'no-package-manager': return { message: 'No supported package manager found', delay: 3000 } case 'error': - return { message: 'OpenClaude update failed', delay: 4000 } + return { message: state.requiresLockRecovery ? UPDATE_RECOVERY_MESSAGE : 'OpenClaude update failed', delay: 4000 } default: return { message: '', delay: 0 } } diff --git a/src/components/AutoUpdater.test.ts b/src/components/AutoUpdater.test.ts index 406f3d2f0..d51458d4b 100644 --- a/src/components/AutoUpdater.test.ts +++ b/src/components/AutoUpdater.test.ts @@ -46,3 +46,27 @@ describe('shouldRemoveInstalledSymlinkForNpmUpdate', () => { expect(shouldRemoveInstalledSymlinkForNpmUpdate(undefined, true)).toBe(true) }) }) + +test('disabled initial and timer callbacks skip version lookups, with an enabled control', async () => { + // The isolated child exercises the component callbacks with mocked hook + // scheduling and network boundaries; it does not mount the terminal UI. + const child = Bun.spawn([ + process.execPath, 'run', + `${import.meta.dir}/../../scripts/fixtures/auto-updater-disabled.ts`, + ], { + cwd: `${import.meta.dir}/../..`, + env: { ...process.env, NODE_ENV: 'production' }, + stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', timeout: 8_000, + }) + const [code, stdout, stderr] = await Promise.all([ + child.exited, new Response(child.stdout).text(), new Response(child.stderr).text(), + ]) + expect(code, stderr).toBe(0) + expect(JSON.parse(stdout)).toEqual({ + initial: { latest: 0, maximum: 0 }, + timer: { latest: 0, maximum: 0 }, + enabled: { latest: 1, maximum: 1 }, + recoveryNotice: true, + ordinaryNotice: true, + }) +}, 10_000) // Longer than the child deadline to allow assertion and cleanup. diff --git a/src/components/AutoUpdater.tsx b/src/components/AutoUpdater.tsx index 3e57ad9ce..2b8835425 100644 --- a/src/components/AutoUpdater.tsx +++ b/src/components/AutoUpdater.tsx @@ -4,7 +4,7 @@ import { type AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, logEve import { useInterval } from 'usehooks-ts'; import { useUpdateNotification } from '../hooks/useUpdateNotification.js'; import { Box, Text } from '../ink.js'; -import { type AutoUpdaterResult, getLatestVersion, getMaxVersion, type InstallStatus, installGlobalPackage, shouldSkipVersion } from '../utils/autoUpdater.js'; +import { type AutoUpdaterResult, getLatestVersion, getMaxVersion, type InstallStatus, installGlobalPackage, shouldSkipVersion, UPDATE_RECOVERY_MESSAGE } from '../utils/autoUpdater.js'; import { getAutoUpdaterNpmMethod, shouldRemoveInstalledSymlinkForNpmUpdate } from '../utils/autoUpdaterRouting.js'; import { getGlobalConfig, isAutoUpdaterDisabled } from '../utils/config.js'; import { logForDebugging } from '../utils/debug.js'; @@ -57,8 +57,12 @@ export function AutoUpdater({ } const currentVersion = MACRO.VERSION; const channel = getInitialSettings()?.autoUpdatesChannel ?? 'latest'; - let latestVersion = await getLatestVersion(channel); const isDisabled = isAutoUpdaterDisabled(); + if (isDisabled) { + logForDebugging('AutoUpdater: Auto-updater disabled, skipping check'); + return; + } + let latestVersion = await getLatestVersion(channel); // Check if max version is set (server-side kill switch for auto-updates) const maxVersion = await getMaxVersion(); @@ -189,6 +193,7 @@ export function AutoUpdater({ : autoUpdaterResult?.status === 'success' && showSuccessMessage && updateSemver && ✓ Update installed · Restart to apply } + {autoUpdaterResult?.status === 'termination_uncertain' && {UPDATE_RECOVERY_MESSAGE}} {(autoUpdaterResult?.status === 'install_failed' || autoUpdaterResult?.status === 'no_permissions') && ✗ Auto-update failed · Try openclaude doctor or{' '} diff --git a/src/utils/autoUpdater.ts b/src/utils/autoUpdater.ts index 39bead0b5..c4adee3d8 100644 --- a/src/utils/autoUpdater.ts +++ b/src/utils/autoUpdater.ts @@ -57,6 +57,10 @@ export type InstallStatus = | 'no_permissions' | 'install_failed' | 'in_progress' + | 'termination_uncertain' + +export const UPDATE_RECOVERY_MESSAGE = + 'Installer may still be running. Wait until all updater and package-manager processes have stopped, then follow the README update-lock recovery instructions.' export type AutoUpdaterResult = { version: string | null @@ -188,9 +192,6 @@ export function shouldSkipVersion(targetVersion: string): boolean { return shouldSkip } -// Lock file for auto-updater to prevent concurrent updates -const LOCK_TIMEOUT_MS = 5 * 60 * 1000 // 5 minute timeout for locks - /** * Get the path to the lock file * This is a function to ensure it's evaluated at runtime after test setup @@ -206,74 +207,28 @@ export function getLockFilePath(): string { async function acquireLock(): Promise { const fs = getFsImplementation() const lockPath = getLockFilePath() - - // Check for existing lock: 1 stat() on the happy path (fresh lock or ENOENT), - // 2 on stale-lock recovery (re-verify staleness immediately before unlink). + // Exclusive creation is the ownership boundary. Never reclaim an existing + // lock: a separate liveness/age check followed by unlink can remove a new + // owner's lock, and a slow installation may still be active at any age. try { - const stats = await fs.stat(lockPath) - const age = Date.now() - stats.mtimeMs - if (age < LOCK_TIMEOUT_MS) { - return false - } - // Lock is stale, remove it before taking over. Re-verify staleness - // immediately before unlinking to close a TOCTOU race: if two processes - // both observe the stale lock, A unlinks + writes a fresh lock, then B - // would unlink A's fresh lock and both believe they hold it. A fresh - // lock has a recent mtime, so re-checking staleness makes B back off. - try { - const recheck = await fs.stat(lockPath) - if (Date.now() - recheck.mtimeMs < LOCK_TIMEOUT_MS) { - return false - } - await fs.unlink(lockPath) - } catch (err) { - if (!isENOENT(err)) { - logError(err as Error) - return false - } - } - } catch (err) { - if (!isENOENT(err)) { - logError(err as Error) - return false - } - // ENOENT: no lock file, proceed to create one - } - - // Create lock file atomically with O_EXCL (flag: 'wx'). If another process - // wins the race and creates it first, we get EEXIST and back off. - // Lazy-mkdir the config dir on ENOENT. - try { - await writeFile(lockPath, `${process.pid}`, { - encoding: 'utf8', - flag: 'wx', - }) + await writeFile(lockPath, `${process.pid}`, { encoding: 'utf8', flag: 'wx' }) return true } catch (err) { - const code = getErrnoCode(err) - if (code === 'EEXIST') { - return false - } - if (code === 'ENOENT') { + if (isENOENT(err)) { try { - // fs.mkdir from getFsImplementation() is always recursive:true and - // swallows EEXIST internally, so a dir-creation race cannot reach the - // catch below — only writeFile's EEXIST (true lock contention) can. await fs.mkdir(getClaudeConfigHomeDir()) - await writeFile(lockPath, `${process.pid}`, { - encoding: 'utf8', - flag: 'wx', - }) + await writeFile(lockPath, `${process.pid}`, { encoding: 'utf8', flag: 'wx' }) return true - } catch (mkdirErr) { - if (getErrnoCode(mkdirErr) === 'EEXIST') { - return false + } catch (retryError) { + if (getErrnoCode(retryError) !== 'EEXIST') { + logError(retryError as Error) } - logError(mkdirErr as Error) return false } } - logError(err as Error) + if (getErrnoCode(err) !== 'EEXIST') { + logError(err as Error) + } return false } } @@ -297,9 +252,11 @@ async function releaseLock(): Promise { } } -async function getInstallationPrefix(): Promise { +async function getInstallationPrefix( + packageManager: 'npm' | 'bun', +): Promise { // Run from home directory to avoid reading project-level .npmrc/.bunfig.toml - const isBun = env.isRunningWithBun() + const isBun = packageManager === 'bun' let prefixResult: Awaited> | null = null if (isBun) { @@ -320,12 +277,14 @@ async function getInstallationPrefix(): Promise { return prefixResult.stdout.trim() } -export async function checkGlobalInstallPermissions(): Promise<{ +export async function checkGlobalInstallPermissions( + packageManager: 'npm' | 'bun' = env.isRunningWithBun() ? 'bun' : 'npm', +): Promise<{ hasPermissions: boolean npmPrefix: string | null }> { try { - const prefix = await getInstallationPrefix() + const prefix = await getInstallationPrefix(packageManager) if (!prefix) { return { hasPermissions: false, npmPrefix: null } } @@ -521,7 +480,7 @@ export async function installGlobalPackage( ): Promise { if (!(await acquireLock())) { logError( - new AutoUpdaterError('Another process is currently installing an update'), + new AutoUpdaterError(`Update lock already exists or could not be created: ${getLockFilePath()}`), ) // Log the lock contention logEvent('tengu_auto_updater_lock_contention', { @@ -532,6 +491,7 @@ export async function installGlobalPackage( return 'in_progress' } + let releaseLockOnCompletion = true try { await removeClaudeAliasesFromShellConfigs() @@ -568,7 +528,7 @@ To fix this issue: // for those managers. pnpm/yarn manage their own global store, so we let the // install command itself surface any permission error there. if (packageManager === 'npm' || packageManager === 'bun') { - const { hasPermissions } = await checkGlobalInstallPermissions() + const { hasPermissions } = await checkGlobalInstallPermissions(packageManager) if (!hasPermissions) { return 'no_permissions' } @@ -586,6 +546,19 @@ To fix this issue: getGlobalInstallArgs(packageManager, packageSpec), { cwd: homedir() }, ) + // Early executor errors and signal-terminated managers do not establish that + // lifecycle children stopped. The executor reports signal names in error. + // Keep ownership for the documented manual recovery procedure. + if ( + installResult.error === 'maxBuffer exceeded' || + installResult.error?.startsWith('Command timed out after ') || + installResult.error?.startsWith('kill ') || + installResult.error?.startsWith('SIG') + ) { + releaseLockOnCompletion = false + logForDebugging('Auto-updater: installer termination uncertain; retaining update lock') + return 'termination_uncertain' + } if (installResult.code !== 0) { const error = new AutoUpdaterError( `Failed to install new version of claude: ${installResult.stdout} ${installResult.stderr}`, @@ -602,8 +575,9 @@ To fix this issue: return 'success' } finally { - // Ensure we always release the lock - await releaseLock() + if (releaseLockOnCompletion) { + await releaseLock() + } } }