fix(coding-agent): resolved EPERM errors when updating binary on Windows
- Switched to unique, timestamped backup file paths to avoid file lock collisions during binary replacement. - Implemented a best-effort strategy for backup deletion, allowing successful updates even if the previous process image remains locked. - Added a cleanup routine for sweeping stale backups, including legacy files, during each update attempt.
This commit is contained in:
can1357
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed `omp update` reporting `EPERM: operation not permitted, unlink '<binary>.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
|
||||
|
||||
@@ -370,13 +370,64 @@ async function unlinkIfExists(filePath: string): Promise<void> {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* 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<boolean> {
|
||||
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 `<binary>.<timestamp>.<pid>.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
|
||||
* `<binary>.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<void> {
|
||||
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 "<base>.bak" → empty middle; new "<base>.<timestamp>.<pid>.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<InstalledVersionVerification> {
|
||||
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`));
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user