diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 9d052e374..1ed9f3c18 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -12,6 +12,7 @@ import { Transform } from "node:stream"; import { pipeline } from "node:stream/promises"; import { $env, $which, APP_NAME, compareVersions, isEnoent, VERSION } from "@oh-my-pi/pi-utils"; import chalk from "@oh-my-pi/pi-utils/chalk"; +import { withFileLock } from "@oh-my-pi/pi-utils/file-lock"; import { $ } from "bun"; import { theme } from "../modes/theme/theme"; import { isTimeoutError, withTimeoutSignal } from "../utils/fetch-timeout"; @@ -1396,16 +1397,22 @@ export async function updateViaBinaryAt( }); console.log(chalk.dim(`Verified ${asset.digest}`)); - console.log(chalk.dim("Installing update...")); - await replaceBinaryForUpdate({ - targetPath, - tempPath, - backupPath, - expectedVersion, - verifyInstalledVersion: options.verifyInstalledVersion ?? verifyInstalledVersion, + // Serialize the target swap and stale-artifact sweep per target so two + // overlapping `omp update` runs never replace the same binary concurrently + // or reclaim each other's live backup/temp files. The download above writes + // to a unique temp path and is safe to overlap; only the swap is shared. + await withFileLock(targetPath, async () => { + console.log(chalk.dim("Installing update...")); + await replaceBinaryForUpdate({ + targetPath, + tempPath, + backupPath, + expectedVersion, + verifyInstalledVersion: options.verifyInstalledVersion ?? verifyInstalledVersion, + }); + // Reclaim backups from earlier updates whose owning process has since exited. + await sweepStaleUpdateArtifacts(targetPath); }); - // Reclaim backups from earlier updates whose owning process has since exited. - await sweepStaleUpdateArtifacts(targetPath); printVerifiedVersion(expectedVersion); console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); } @@ -1464,65 +1471,69 @@ export async function updateViaShimTakeover( fetchImpl: options.fetchImpl, }); console.log(chalk.dim(`Verified ${asset.digest}`)); - - console.log(chalk.dim(`Installing ${APP_NAME}.exe beside the script launcher...`)); - await fs.promises.rename(tempPath, exePath); - // Retire the shims so PATH resolution lands on the new exe. Renamed, not - // deleted: restorable on verification failure, and Windows permits - // renaming a batch file that is still executing. A shim that cannot be - // renamed (held open without delete sharing) is rewritten in place as a - // forwarder to the exe — write and rename take different Windows locks, - // so one can succeed where the other fails. - const backupSuffix = `${attempt}.bak`; - const retired: Array<{ launcher: string; backup: string }> = []; const forwarded: Array<{ launcher: string; original: string }> = []; const stuck: string[] = []; - for (const ext of ["", ".cmd", ".ps1", ".bat"]) { - const launcher = path.join(launcherDir, `${APP_NAME}${ext}`); - const backup = `${launcher}.${backupSuffix}`; - try { - await fs.promises.rename(launcher, backup); - retired.push({ launcher, backup }); - } catch (err) { - if (isEnoent(err)) continue; + // Serialize the launcher swap and artifact sweep so two overlapping updates + // never retire the same shims or reclaim a live run's backup before its + // verification can roll it back. + await withFileLock(exePath, async () => { + console.log(chalk.dim(`Installing ${APP_NAME}.exe beside the script launcher...`)); + await fs.promises.rename(tempPath, exePath); + // Retire the shims so PATH resolution lands on the new exe. Renamed, not + // deleted: restorable on verification failure, and Windows permits + // renaming a batch file that is still executing. A shim that cannot be + // renamed (held open without delete sharing) is rewritten in place as a + // forwarder to the exe — write and rename take different Windows locks, + // so one can succeed where the other fails. + const backupSuffix = `${attempt}.bak`; + const retired: Array<{ launcher: string; backup: string }> = []; + for (const ext of ["", ".cmd", ".ps1", ".bat"]) { + const launcher = path.join(launcherDir, `${APP_NAME}${ext}`); + const backup = `${launcher}.${backupSuffix}`; try { - const original = await Bun.file(launcher).text(); - await Bun.write(launcher, SHIM_FORWARDERS[ext]); - forwarded.push({ launcher, original }); - } catch { - stuck.push(launcher); + await fs.promises.rename(launcher, backup); + retired.push({ launcher, backup }); + } catch (err) { + if (isEnoent(err)) continue; + try { + const original = await Bun.file(launcher).text(); + await Bun.write(launcher, SHIM_FORWARDERS[ext]); + forwarded.push({ launcher, original }); + } catch { + stuck.push(launcher); + } } } - } - // Verify the exe by its explicit path: $which cached the shim path when - // the update target was resolved, and the shim was just renamed away, so - // a PATH re-resolution here would test a file that no longer exists. - const verify = options.verifyBinary ?? verifyBinaryAtPath; - const verification = await verify(exePath, expectedVersion); - if (!verification.ok) { - for (const { launcher, backup } of retired) { - try { - await fs.promises.rename(backup, launcher); - } catch {} + // Verify the exe by its explicit path: $which cached the shim path when + // the update target was resolved, and the shim was just renamed away, so + // a PATH re-resolution here would test a file that no longer exists. + const verify = options.verifyBinary ?? verifyBinaryAtPath; + const verification = await verify(exePath, expectedVersion); + if (!verification.ok) { + for (const { launcher, backup } of retired) { + try { + await fs.promises.rename(backup, launcher); + } catch {} + } + for (const { launcher, original } of forwarded) { + try { + await Bun.write(launcher, original); + } catch {} + } + await unlinkIfExists(exePath); + throw new Error( + `${formatVerificationFailure(verification, expectedVersion)}; restored previous ${APP_NAME} launcher`, + ); } - for (const { launcher, original } of forwarded) { - try { - await Bun.write(launcher, original); - } catch {} + for (const { backup } of retired) { + await removeBackupBestEffort(backup); } - await unlinkIfExists(exePath); - throw new Error( - `${formatVerificationFailure(verification, expectedVersion)}; restored previous ${APP_NAME} launcher`, - ); - } - for (const { backup } of retired) { - await removeBackupBestEffort(backup); - } - // Reclaim exe backups and retired-shim leftovers from earlier attempts. - for (const ext of [".exe", "", ".cmd", ".ps1", ".bat"]) { - await sweepStaleUpdateArtifacts(path.join(launcherDir, `${APP_NAME}${ext}`)); - } + // Reclaim exe backups and retired-shim leftovers from earlier attempts. + for (const ext of [".exe", "", ".cmd", ".ps1", ".bat"]) { + await sweepStaleUpdateArtifacts(path.join(launcherDir, `${APP_NAME}${ext}`)); + } + }); for (const { launcher } of forwarded) { console.log(chalk.dim(`Converted ${launcher} to a forwarder (it could not be removed).`)); } diff --git a/packages/coding-agent/test/update-cli.test.ts b/packages/coding-agent/test/update-cli.test.ts index c3a1e278f..f658dd3b3 100644 --- a/packages/coding-agent/test/update-cli.test.ts +++ b/packages/coding-agent/test/update-cli.test.ts @@ -1105,12 +1105,16 @@ describe("update-cli concurrent binary updates", () => { }); } - // Regression for #8434: two overlapping `omp update` runs must not share a - // temp path. Run A downloads slowly and only finishes after run B has fully - // installed. With the old fixed `.new` temp name, B's pre-download - // unlink deleted A's temp file, so A's chmod failed with ENOENT even though - // its size + digest passed. Unique temp paths keep the two runs independent. - it("lets an overlapping slow run install after a fast run completes, instead of failing chmod with ENOENT", async () => { + const fastFetch = async (input: string | URL | Request): Promise => { + const requestUrl = String(input); + if (requestUrl.startsWith("https://api.github.com/")) return metadata(); + if (requestUrl === url) return new Response(payload); + throw new Error(`Unexpected request: ${requestUrl}`); + }; + + const verify = async () => ({ ok: true, actual: version }); + + async function prepare(): Promise<{ dir: string; targetPath: string }> { const loadedTheme = await getThemeByName("dark"); if (!loadedTheme) throw new Error("theme unavailable"); setThemeInstance(loadedTheme); @@ -1118,6 +1122,16 @@ describe("update-cli concurrent binary updates", () => { const dir = await makeTempDir(); const targetPath = path.join(dir, "omp"); await Bun.write(targetPath, "old binary"); + return { dir, targetPath }; + } + + // Regression for #8434: two overlapping `omp update` runs must not share a + // temp path. Run A downloads slowly and only finishes after run B has fully + // installed. With the old fixed `.new` temp name, B's pre-download + // unlink deleted A's temp file, so A's chmod failed with ENOENT even though + // its size + digest passed. Unique temp paths keep the two runs independent. + it("lets an overlapping slow run install after a fast run completes, instead of failing chmod with ENOENT", async () => { + const { dir, targetPath } = await prepare(); const aWroteFirstChunk = Promise.withResolvers(); const letAFinish = Promise.withResolvers(); @@ -1139,13 +1153,6 @@ describe("update-cli concurrent binary updates", () => { } throw new Error(`Unexpected request: ${requestUrl}`); }; - const fastFetch = async (input: string | URL | Request): Promise => { - const requestUrl = String(input); - if (requestUrl.startsWith("https://api.github.com/")) return metadata(); - if (requestUrl === url) return new Response(payload); - throw new Error(`Unexpected request: ${requestUrl}`); - }; - const verify = async () => ({ ok: true, actual: version, path: targetPath }); const runA = updateViaBinaryAt(targetPath, version, { binaryName, @@ -1165,4 +1172,39 @@ describe("update-cli concurrent binary updates", () => { const residue = (await fs.readdir(dir)).filter(name => name.endsWith(".new")); expect(residue).toEqual([]); }); + + // Regression: a failed verification must still roll back its own backup even + // when another update completes while it is held. The per-target lock + // serializes the swap + sweep, so the concurrent run's sweep cannot reclaim + // the live backup before the rollback renames it back. + it("rolls back its backup when verification fails while another update runs", async () => { + const { dir, targetPath } = await prepare(); + + const enteredVerify = Promise.withResolvers(); + const releaseVerify = Promise.withResolvers(); + const failingVerify = async () => { + enteredVerify.resolve(); + await releaseVerify.promise; + return { ok: false, actual: "0.0.0", path: targetPath }; + }; + + const runA = updateViaBinaryAt(targetPath, version, { + binaryName, + fetchImpl: fastFetch, + verifyInstalledVersion: failingVerify, + }); + await enteredVerify.promise; + const runB = updateViaBinaryAt(targetPath, version, { + binaryName, + fetchImpl: fastFetch, + verifyInstalledVersion: verify, + }); + releaseVerify.resolve(); + await expect(runA).rejects.toThrow(/still reports 0\.0\.0 \(expected 999\.0\.0\)/); + await runB; + + expect(await Bun.file(targetPath).bytes()).toEqual(new Uint8Array(payload)); + const residue = (await fs.readdir(dir)).filter(name => name.endsWith(".bak") || name.endsWith(".new")); + expect(residue).toEqual([]); + }); });