diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 86605dfb5..4e00def9a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `omp update` reporting `EPERM: operation not permitted, unlink '.bak'` on Windows when self-replacing a standalone binary, even though the new binary had already been installed. The backed-up old executable is still the running process image and cannot be unlinked until the process exits, so the post-verify backup cleanup is now best-effort, backups use a unique per-attempt name, and stale backups are swept on the next update ([#845](https://github.com/can1357/oh-my-pi/issues/845)). + ## [16.0.7] - 2026-06-18 ### Fixed diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 1d70e3255..9add11fdc 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -370,13 +370,64 @@ async function unlinkIfExists(filePath: string): Promise { } } +/** + * Remove a backup binary without letting the removal abort a completed update. + * + * On Windows the executable that was just moved aside is still mapped as the + * running process image, so unlinking it fails with EPERM/EACCES until this + * process exits (issue #845). The replacement and verification already + * succeeded by the time we get here, so every error is swallowed; the leftover + * is reclaimed by {@link sweepStaleBackups} on the next update once it is no + * longer in use. Returns whether the file is gone. + */ +async function removeBackupBestEffort(filePath: string): Promise { + try { + await fs.promises.unlink(filePath); + return true; + } catch (err) { + return isEnoent(err); + } +} + +/** + * Best-effort removal of binary-update backups left by earlier runs. + * + * Each self-update moves the previous executable to `...bak` + * before swapping the new one in. On Windows that backup cannot be deleted + * while the updating process is alive, so it is left for a later run to reclaim + * once its owning process has exited. Also matches the legacy fixed + * `.bak` name produced before backups were timestamped, so users + * upgrading from a buggy release get the orphaned file cleaned up. + */ +export async function sweepStaleBackups(targetPath: string): Promise { + const dir = path.dirname(targetPath); + const base = path.basename(targetPath); + let entries: string[]; + try { + entries = await fs.promises.readdir(dir); + } catch { + return; + } + for (const entry of entries) { + if (!entry.startsWith(`${base}.`) || !entry.endsWith(".bak")) continue; + // Legacy ".bak" → empty middle; new "...bak" + // → dot-separated numeric run. Anything else is an unrelated *.bak file. + const middle = entry.slice(base.length + 1, entry.length - ".bak".length); + if (middle.length > 0 && !/^\d+(\.\d+)*$/.test(middle)) continue; + await removeBackupBestEffort(path.join(dir, entry)); + } +} + /** * Atomically replace the installed binary and roll back if version verification fails. */ export async function replaceBinaryForUpdate(options: BinaryReplacementOptions): Promise { let backupReady = false; try { - await unlinkIfExists(options.backupPath); + // `backupPath` is unique per attempt (see updateViaBinaryAt), so this rename + // never has to overwrite — or unlink — a possibly-locked leftover from an + // earlier run. Renaming the running executable itself is permitted on + // Windows; only deleting its still-mapped image is not. await fs.promises.rename(options.targetPath, options.backupPath); backupReady = true; await fs.promises.rename(options.tempPath, options.targetPath); @@ -389,7 +440,10 @@ export async function replaceBinaryForUpdate(options: BinaryReplacementOptions): } backupReady = false; - await unlinkIfExists(options.backupPath); + // Swap done and verified. On Windows the backup is still the running + // process image and cannot be unlinked until this process exits, so a + // failure here must NOT fail an otherwise-successful update. + await removeBackupBestEffort(options.backupPath); return verification; } catch (err) { if (backupReady) { @@ -517,7 +571,11 @@ async function updateViaBinaryAt(targetPath: string, expectedVersion: string): P const url = `https://github.com/${REPO}/releases/download/${tag}/${binaryName}`; const tempPath = `${targetPath}.new`; - const backupPath = `${targetPath}.bak`; + // Unique per attempt: a stale backup from an earlier update may still be + // locked (it is the previous process image on Windows), and a fixed name + // would force the move-aside rename to overwrite it. pid + timestamp keeps + // two forced updates in the same millisecond from colliding. + const backupPath = `${targetPath}.${Date.now()}.${process.pid}.bak`; console.log(chalk.dim(`Downloading ${binaryName}…`)); const response = await fetch(url, { redirect: "follow" }); @@ -535,6 +593,8 @@ async function updateViaBinaryAt(targetPath: string, expectedVersion: string): P expectedVersion, verifyInstalledVersion, }); + // Reclaim backups from earlier updates whose owning process has since exited. + await sweepStaleBackups(targetPath); printVerifiedVersion(expectedVersion); console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); } diff --git a/packages/coding-agent/test/update-cli.test.ts b/packages/coding-agent/test/update-cli.test.ts index 73e7ada2b..ad58002de 100644 --- a/packages/coding-agent/test/update-cli.test.ts +++ b/packages/coding-agent/test/update-cli.test.ts @@ -1,4 +1,5 @@ -import { afterEach, describe, expect, it } from "bun:test"; +import { afterEach, describe, expect, it, spyOn } from "bun:test"; +import * as nodeFs from "node:fs"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -9,6 +10,7 @@ import { buildMiseUpgradeArgs, replaceBinaryForUpdate, resolveUpdateMethodForTest, + sweepStaleBackups, } from "@oh-my-pi/pi-coding-agent/cli/update-cli"; const tempDirs: string[] = []; @@ -181,3 +183,68 @@ describe("update-cli binary replacement", () => { expect(await Bun.file(backupPath).exists()).toBe(false); }); }); + +describe("update-cli binary replacement on locked backups", () => { + it("treats an EPERM on backup cleanup as a successful, completed update", async () => { + // Regression: on Windows the binary moved aside during the swap is still + // the running process image, so unlinking it throws EPERM. That cleanup + // failure must not turn a verified swap into "Update failed" (issue #845). + const dir = await makeTempDir(); + const targetPath = path.join(dir, "omp.exe"); + const tempPath = `${targetPath}.new`; + const backupPath = `${targetPath}.1700000000000.4242.bak`; + await Bun.write(targetPath, "old binary"); + await Bun.write(tempPath, "new binary"); + + const realUnlink = nodeFs.promises.unlink.bind(nodeFs.promises); + const spy = spyOn(nodeFs.promises, "unlink").mockImplementation(async (p: nodeFs.PathLike) => { + if (String(p) === backupPath) { + const err = new Error(`EPERM: operation not permitted, unlink '${p}'`) as NodeJS.ErrnoException; + err.code = "EPERM"; + throw err; + } + return realUnlink(p); + }); + try { + const result = await replaceBinaryForUpdate({ + targetPath, + tempPath, + backupPath, + expectedVersion: "15.1.8", + verifyInstalledVersion: async () => ({ ok: true, actual: "15.1.8", path: targetPath }), + }); + expect(result.ok).toBe(true); + } finally { + spy.mockRestore(); + } + + // New binary is installed and the temp consumed even though the locked + // backup survives; the next run's sweep reclaims it once it is unlocked. + expect(await Bun.file(targetPath).text()).toBe("new binary"); + expect(await Bun.file(tempPath).exists()).toBe(false); + expect(await Bun.file(backupPath).text()).toBe("old binary"); + }); +}); + +describe("update-cli stale backup sweep", () => { + it("reclaims timestamped and legacy backups while leaving unrelated .bak files", async () => { + const dir = await makeTempDir(); + const targetPath = path.join(dir, "omp.exe"); + await Bun.write(targetPath, "current binary"); + await Bun.write(`${targetPath}.bak`, "legacy backup"); + await Bun.write(`${targetPath}.1700000000000.4242.bak`, "timestamped backup"); + await Bun.write(`${targetPath}.1800000000000.99.bak`, "another backup"); + // Must survive: foreign basename and a non-numeric middle segment. + await Bun.write(path.join(dir, "notes.bak"), "keep me"); + await Bun.write(`${targetPath}.config.bak`, "keep me too"); + + await sweepStaleBackups(targetPath); + + expect(await Bun.file(targetPath).exists()).toBe(true); + expect(await Bun.file(`${targetPath}.bak`).exists()).toBe(false); + expect(await Bun.file(`${targetPath}.1700000000000.4242.bak`).exists()).toBe(false); + expect(await Bun.file(`${targetPath}.1800000000000.99.bak`).exists()).toBe(false); + expect(await Bun.file(path.join(dir, "notes.bak")).exists()).toBe(true); + expect(await Bun.file(`${targetPath}.config.bak`).exists()).toBe(true); + }); +});