smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(update): unique temp path for concurrent self-updates

Browse Source 
Two overlapping `omp update` runs shared the fixed `<binary>.new` temp
path. downloadVerifiedBinary unlinks the target before writing, so the
second run's unlink deleted the first run's still-downloading temp file;
the first kept writing to its open fd (size and digest still passed),
then chmod hit the missing path and aborted with ENOENT.

Give the temp path the same unique per-attempt suffix the backup path
already uses (pid, timestamp, and a new process-local counter that also
covers same-millisecond, same-process collisions). Generalize the stale
backup sweep to also reclaim orphaned `.new` temp files, age-gated by the
download window so a concurrent run's in-progress temp is never deleted.

Fixes #8434
This commit is contained in:
roboomp
2026-08-13 15:17:41 +00:00
parent 326d24bd40
commit 1d6d35bd24
3 changed files with 158 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/test/update-cli.test.ts
+97 -6
View File
@@ -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([]);
});
});