Merge PR #8435: fix(update): unique temp path for concurrent self-updates (@roboomp)

packages/coding-agent/CHANGELOG.md

@@ -8,6 +8,7 @@
 - Fixed Pi extension contexts omitting the runtime `mode`, which made documented TUI guards silently disable extension UI ([#8419](https://github.com/can1357/oh-my-pi/issues/8419)).
 - Fixed extension-registered tool names being rejected by `--tools` before extension discovery, preventing least-privilege sessions from allowlisting plugin tools ([#8421](https://github.com/can1357/oh-my-pi/issues/8421)).
 - Fixed `omp plugin install` failing with `The object can not be cloned.` for legacy Pi extensions whose tool schemas embed or spread omptype builders through the legacy-typebox shim (e.g. `pi-subagents`). `Type.Unsafe` now lowers nested schema builders to plain wire JSON and reconstructs spread schemas from their copied self-reference before serializing ([#8420](https://github.com/can1357/oh-my-pi/issues/8420)).
+- Fixed `omp update` aborting with `chmod ENOENT` on `<binary>.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
 

packages/coding-agent/src/cli/update-cli.ts

@@ -987,8 +987,8 @@ async function unlinkIfExists(filePath: string): Promise<void> {
  * 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<boolean> {
 	try {
@@ -1000,16 +1000,21 @@ async function removeBackupBestEffort(filePath: string): Promise<boolean> {
 }
 
 /**
- * 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 `<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.
+ * Each self-update writes to `<binary>.<timestamp>.<pid>.new` and moves the
+ * previous executable to `<binary>.<timestamp>.<pid>.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 `<binary>.bak` / `<binary>.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<void> {
+export async function sweepStaleUpdateArtifacts(targetPath: string): Promise<void> {
 	const dir = path.dirname(targetPath);
 	const base = path.basename(targetPath);
 	let entries: string[];
@@ -1018,13 +1023,28 @@ export async function sweepStaleBackups(targetPath: string): Promise<void> {
 	} catch {
 		return;
 	}
+	const now = Date.now();
 	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 (!entry.startsWith(`${base}.`)) continue;
+		const suffix = entry.endsWith(".bak") ? ".bak" : entry.endsWith(".new") ? ".new" : undefined;
+		if (!suffix) continue;
+		// Legacy "<base><suffix>" → empty middle; new "<base>.<timestamp>.<pid><suffix>"
+		// → 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<v
 	await printVerification(expectedVersion);
 }
 
+// Monotonic within this process so two updates started in the same millisecond
+// (same pid, same `Date.now()`) still get distinct temp/backup paths. Kept
+// numeric so the artifact sweep's `\d+(\.\d+)*` matcher still reclaims them.
+let updateAttemptSeq = 0;
+
 /**
  * Download a release binary to a target path, replacing an existing file.
  */
@@ -1348,12 +1373,18 @@ export async function updateViaBinaryAt(
 	} = {},
 ): Promise<void> {
 	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 (`<binary>.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).`));

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 `<binary>.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<void>();
+		const letAFinish = Promise.withResolvers<void>();
+		const slowFetch = async (input: string | URL | Request): Promise<Response> => {
+			const requestUrl = String(input);
+			if (requestUrl.startsWith("https://api.github.com/")) return metadata();
+			if (requestUrl === url) {
+				return new Response(
+					new ReadableStream<Uint8Array>({
+						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<Response> => {
+			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([]);
+	});
+});