diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 14334fddd..b7eda0e71 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `omp update` aborting with `chmod ENOENT` on `.new` when two update runs overlapped: the download temp path is now unique per attempt (pid, timestamp, and a process-local counter), so concurrent runs no longer delete each other's temp file, and orphaned temp files are reclaimed alongside stale backups ([#8434](https://github.com/can1357/oh-my-pi/issues/8434)). + ## [17.3.0] - 2026-08-13 ### Breaking Changes diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index af5ba846c..9d052e374 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -987,8 +987,8 @@ async function unlinkIfExists(filePath: string): Promise { * 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. + * is reclaimed by {@link sweepStaleUpdateArtifacts} on the next update once it + * is no longer in use. Returns whether the file is gone. */ async function removeBackupBestEffort(filePath: string): Promise { try { @@ -1000,16 +1000,21 @@ async function removeBackupBestEffort(filePath: string): Promise { } /** - * Best-effort removal of binary-update backups left by earlier runs. + * Best-effort removal of binary-update leftovers from 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. + * Each self-update writes to `...new` and moves the + * previous executable to `...bak` before swapping the + * new one in. On Windows a backup cannot be deleted while the updating process + * is alive (it is the running process image), so it is left for a later run to + * reclaim once its owning process has exited. A `.new` temp file only survives + * a hard kill mid-download; it is reaped once older than the download window, + * which a live download cannot exceed without timing out and cleaning up after + * itself — so a concurrent run's in-progress temp is never deleted. Legacy + * fixed `.bak` / `.new` names (from before suffixes were made + * unique) are matched too, so users upgrading from a buggy release get the + * orphaned files cleaned up. */ -export async function sweepStaleBackups(targetPath: string): Promise { +export async function sweepStaleUpdateArtifacts(targetPath: string): Promise { const dir = path.dirname(targetPath); const base = path.basename(targetPath); let entries: string[]; @@ -1018,13 +1023,28 @@ export async function sweepStaleBackups(targetPath: string): Promise { } catch { return; } + const now = Date.now(); 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 (!entry.startsWith(`${base}.`)) continue; + const suffix = entry.endsWith(".bak") ? ".bak" : entry.endsWith(".new") ? ".new" : undefined; + if (!suffix) continue; + // Legacy "" → empty middle; new ".." + // → dot-separated numeric run. Anything else is an unrelated file. + const middle = entry.slice(base.length + 1, entry.length - suffix.length); if (middle.length > 0 && !/^\d+(\.\d+)*$/.test(middle)) continue; - await removeBackupBestEffort(path.join(dir, entry)); + const full = path.join(dir, entry); + if (suffix === ".new") { + // A temp file may belong to a concurrent update still downloading, so + // only reap ones older than the download window. + let mtimeMs: number; + try { + mtimeMs = (await fs.promises.stat(full)).mtimeMs; + } catch { + continue; + } + if (now - mtimeMs < BINARY_DOWNLOAD_TIMEOUT_MS) continue; + } + await removeBackupBestEffort(full); } } @@ -1334,6 +1354,11 @@ async function updateViaMise(expectedVersion: string, force: boolean): Promise { const binaryName = options.binaryName ?? getBinaryName(); - const tempPath = `${targetPath}.new`; - // 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`; + // Unique per attempt so two overlapping `omp update` runs never share a temp + // or backup path. A fixed temp name (`.new`) let the second run's + // pre-download unlink delete the first run's still-downloading temp file; the + // first kept writing to its open fd (size + digest still passed), then chmod + // hit the missing path and the update aborted (issue #8434). The backup needs + // the same uniqueness: a stale backup from an earlier update may still be + // locked (the previous process image on Windows), so a fixed name would force + // the move-aside rename to overwrite it. pid, timestamp, and a process-local + // counter keep two updates started in the same millisecond from colliding. + const attempt = `${Date.now()}.${process.pid}.${updateAttemptSeq++}`; + const tempPath = `${targetPath}.${attempt}.new`; + const backupPath = `${targetPath}.${attempt}.bak`; const asset = await getReleaseBinaryAsset(expectedVersion, binaryName, options.fetchImpl, options.githubToken); console.log(chalk.dim(`Downloading ${binaryName}…`)); await downloadVerifiedBinary({ @@ -1374,7 +1405,7 @@ export async function updateViaBinaryAt( verifyInstalledVersion: options.verifyInstalledVersion ?? verifyInstalledVersion, }); // Reclaim backups from earlier updates whose owning process has since exited. - await sweepStaleBackups(targetPath); + await sweepStaleUpdateArtifacts(targetPath); printVerifiedVersion(expectedVersion); console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); } @@ -1421,7 +1452,8 @@ export async function updateViaShimTakeover( const binaryName = options.binaryName ?? getBinaryName(); const launcherDir = path.dirname(shimPath); const exePath = path.join(launcherDir, `${APP_NAME}.exe`); - const tempPath = `${exePath}.new`; + const attempt = `${Date.now()}.${process.pid}.${updateAttemptSeq++}`; + const tempPath = `${exePath}.${attempt}.new`; const asset = await getReleaseBinaryAsset(expectedVersion, binaryName, options.fetchImpl, options.githubToken); console.log(chalk.dim(`Downloading ${binaryName}…`)); await downloadVerifiedBinary({ @@ -1441,7 +1473,7 @@ export async function updateViaShimTakeover( // 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 = `${Date.now()}.${process.pid}.bak`; + const backupSuffix = `${attempt}.bak`; const retired: Array<{ launcher: string; backup: string }> = []; const forwarded: Array<{ launcher: string; original: string }> = []; const stuck: string[] = []; @@ -1489,7 +1521,7 @@ export async function updateViaShimTakeover( } // Reclaim exe backups and retired-shim leftovers from earlier attempts. for (const ext of [".exe", "", ".cmd", ".ps1", ".bat"]) { - await sweepStaleBackups(path.join(launcherDir, `${APP_NAME}${ext}`)); + 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 608071d8b..3e0345a1f 100644 --- a/packages/coding-agent/test/update-cli.test.ts +++ b/packages/coding-agent/test/update-cli.test.ts @@ -27,7 +27,7 @@ import { resolveReleaseRename, resolveUpdateMethodForTest, shouldForceBinaryUpdate, - sweepStaleBackups, + sweepStaleUpdateArtifacts, updateViaBinaryAt, updateViaShimTakeover, } from "@oh-my-pi/pi-coding-agent/cli/update-cli"; @@ -758,7 +758,8 @@ describe("update-cli release binary integrity", () => { expect(metadataAuthorizations).toEqual(["Bearer test-token"]); expect(await Bun.file(targetPath).text()).toBe(installed); expect((await fs.stat(targetPath)).mode & 0o777).toBe(0o755); - expect(await Bun.file(`${targetPath}.new`).exists()).toBe(false); + const newResidue = (await fs.readdir(dir)).filter(name => name.endsWith(".new")); + expect(newResidue).toEqual([]); } finally { if (previousGitHubToken === undefined) delete Bun.env.GITHUB_TOKEN; else Bun.env.GITHUB_TOKEN = previousGitHubToken; @@ -869,26 +870,41 @@ describe("update-cli binary replacement on locked backups", () => { }); }); -describe("update-cli stale backup sweep", () => { - it("reclaims timestamped and legacy backups while leaving unrelated .bak files", async () => { +describe("update-cli stale update artifact sweep", () => { + it("reclaims timestamped and legacy backups and orphaned temps while sparing in-progress temps and unrelated 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. + // Orphaned temp files from a hard-killed download: reaped once older than + // the download window. Legacy fixed name and timestamped name both count. + const stale = new Date(Date.now() - 60 * 60 * 1000); + await Bun.write(`${targetPath}.new`, "legacy temp"); + await fs.utimes(`${targetPath}.new`, stale, stale); + await Bun.write(`${targetPath}.1700000000000.4242.new`, "timestamped temp"); + await fs.utimes(`${targetPath}.1700000000000.4242.new`, stale, stale); + // Must survive: a fresh temp still belongs to a concurrent, in-progress + // download (unique per attempt), plus foreign basenames and non-numeric + // middle segments. + await Bun.write(`${targetPath}.9999999999999.7.new`, "in-progress temp"); await Bun.write(path.join(dir, "notes.bak"), "keep me"); await Bun.write(`${targetPath}.config.bak`, "keep me too"); + await Bun.write(`${targetPath}.config.new`, "keep me three"); - await sweepStaleBackups(targetPath); + await sweepStaleUpdateArtifacts(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(`${targetPath}.new`).exists()).toBe(false); + expect(await Bun.file(`${targetPath}.1700000000000.4242.new`).exists()).toBe(false); + expect(await Bun.file(`${targetPath}.9999999999999.7.new`).exists()).toBe(true); expect(await Bun.file(path.join(dir, "notes.bak")).exists()).toBe(true); expect(await Bun.file(`${targetPath}.config.bak`).exists()).toBe(true); + expect(await Bun.file(`${targetPath}.config.new`).exists()).toBe(true); }); }); @@ -1071,3 +1087,78 @@ describe("update-cli script-shim takeover", () => { } }); }); + +describe("update-cli concurrent binary updates", () => { + const version = "999.0.0"; + const binaryName = "omp-linux-x64"; + const url = `https://github.com/can1357/oh-my-pi/releases/download/v${version}/${binaryName}`; + const payload = Buffer.alloc(2048, 0x41); + const digest = `sha256:${createHash("sha256").update(payload).digest("hex")}`; + + function metadata(): Response { + return Response.json({ + tag_name: `v${version}`, + draft: false, + prerelease: false, + assets: [{ name: binaryName, state: "uploaded", size: payload.byteLength, digest, browser_download_url: url }], + }); + } + + // 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 () => { + vi.spyOn(console, "log").mockImplementation(() => {}); + const dir = await makeTempDir(); + const targetPath = path.join(dir, "omp"); + await Bun.write(targetPath, "old binary"); + + const aWroteFirstChunk = Promise.withResolvers(); + const letAFinish = Promise.withResolvers(); + const slowFetch = 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( + new ReadableStream({ + async start(controller) { + controller.enqueue(payload.subarray(0, 1024)); + aWroteFirstChunk.resolve(); + await letAFinish.promise; + controller.enqueue(payload.subarray(1024)); + controller.close(); + }, + }), + ); + } + 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, + fetchImpl: slowFetch, + verifyInstalledVersion: verify, + }); + await aWroteFirstChunk.promise; + await updateViaBinaryAt(targetPath, version, { + binaryName, + fetchImpl: fastFetch, + verifyInstalledVersion: verify, + }); + letAFinish.resolve(); + await runA; + + expect(await Bun.file(targetPath).bytes()).toEqual(new Uint8Array(payload)); + const residue = (await fs.readdir(dir)).filter(name => name.endsWith(".new")); + expect(residue).toEqual([]); + }); +});