fix(update): serialize target swap under a per-target lock
Two overlapping `omp update` runs now share the target only for the swap + stale-artifact sweep, guarded by withFileLock. This closes the rollback race the unique-temp fix left open: a concurrent run's sweep could delete a live run's .bak before its failed verification rolled it back. The download stays outside the lock (unique temp path, safe to overlap). Adds a regression covering a failed verification overlapping a successful update.
This commit is contained in:
can1357
@@ -12,6 +12,7 @@ import { Transform } from "node:stream";
|
||||
import { pipeline } from "node:stream/promises";
|
||||
import { $env, $which, APP_NAME, compareVersions, isEnoent, VERSION } from "@oh-my-pi/pi-utils";
|
||||
import chalk from "@oh-my-pi/pi-utils/chalk";
|
||||
import { withFileLock } from "@oh-my-pi/pi-utils/file-lock";
|
||||
import { $ } from "bun";
|
||||
import { theme } from "../modes/theme/theme";
|
||||
import { isTimeoutError, withTimeoutSignal } from "../utils/fetch-timeout";
|
||||
@@ -1396,16 +1397,22 @@ export async function updateViaBinaryAt(
|
||||
});
|
||||
console.log(chalk.dim(`Verified ${asset.digest}`));
|
||||
|
||||
console.log(chalk.dim("Installing update..."));
|
||||
await replaceBinaryForUpdate({
|
||||
targetPath,
|
||||
tempPath,
|
||||
backupPath,
|
||||
expectedVersion,
|
||||
verifyInstalledVersion: options.verifyInstalledVersion ?? verifyInstalledVersion,
|
||||
// Serialize the target swap and stale-artifact sweep per target so two
|
||||
// overlapping `omp update` runs never replace the same binary concurrently
|
||||
// or reclaim each other's live backup/temp files. The download above writes
|
||||
// to a unique temp path and is safe to overlap; only the swap is shared.
|
||||
await withFileLock(targetPath, async () => {
|
||||
console.log(chalk.dim("Installing update..."));
|
||||
await replaceBinaryForUpdate({
|
||||
targetPath,
|
||||
tempPath,
|
||||
backupPath,
|
||||
expectedVersion,
|
||||
verifyInstalledVersion: options.verifyInstalledVersion ?? verifyInstalledVersion,
|
||||
});
|
||||
// Reclaim backups from earlier updates whose owning process has since exited.
|
||||
await sweepStaleUpdateArtifacts(targetPath);
|
||||
});
|
||||
// Reclaim backups from earlier updates whose owning process has since exited.
|
||||
await sweepStaleUpdateArtifacts(targetPath);
|
||||
printVerifiedVersion(expectedVersion);
|
||||
console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`));
|
||||
}
|
||||
@@ -1464,65 +1471,69 @@ export async function updateViaShimTakeover(
|
||||
fetchImpl: options.fetchImpl,
|
||||
});
|
||||
console.log(chalk.dim(`Verified ${asset.digest}`));
|
||||
|
||||
console.log(chalk.dim(`Installing ${APP_NAME}.exe beside the script launcher...`));
|
||||
await fs.promises.rename(tempPath, exePath);
|
||||
// Retire the shims so PATH resolution lands on the new exe. Renamed, not
|
||||
// deleted: restorable on verification failure, and Windows permits
|
||||
// renaming a batch file that is still executing. A shim that cannot be
|
||||
// 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 = `${attempt}.bak`;
|
||||
const retired: Array<{ launcher: string; backup: string }> = [];
|
||||
const forwarded: Array<{ launcher: string; original: string }> = [];
|
||||
const stuck: string[] = [];
|
||||
for (const ext of ["", ".cmd", ".ps1", ".bat"]) {
|
||||
const launcher = path.join(launcherDir, `${APP_NAME}${ext}`);
|
||||
const backup = `${launcher}.${backupSuffix}`;
|
||||
try {
|
||||
await fs.promises.rename(launcher, backup);
|
||||
retired.push({ launcher, backup });
|
||||
} catch (err) {
|
||||
if (isEnoent(err)) continue;
|
||||
// Serialize the launcher swap and artifact sweep so two overlapping updates
|
||||
// never retire the same shims or reclaim a live run's backup before its
|
||||
// verification can roll it back.
|
||||
await withFileLock(exePath, async () => {
|
||||
console.log(chalk.dim(`Installing ${APP_NAME}.exe beside the script launcher...`));
|
||||
await fs.promises.rename(tempPath, exePath);
|
||||
// Retire the shims so PATH resolution lands on the new exe. Renamed, not
|
||||
// deleted: restorable on verification failure, and Windows permits
|
||||
// renaming a batch file that is still executing. A shim that cannot be
|
||||
// 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 = `${attempt}.bak`;
|
||||
const retired: Array<{ launcher: string; backup: string }> = [];
|
||||
for (const ext of ["", ".cmd", ".ps1", ".bat"]) {
|
||||
const launcher = path.join(launcherDir, `${APP_NAME}${ext}`);
|
||||
const backup = `${launcher}.${backupSuffix}`;
|
||||
try {
|
||||
const original = await Bun.file(launcher).text();
|
||||
await Bun.write(launcher, SHIM_FORWARDERS[ext]);
|
||||
forwarded.push({ launcher, original });
|
||||
} catch {
|
||||
stuck.push(launcher);
|
||||
await fs.promises.rename(launcher, backup);
|
||||
retired.push({ launcher, backup });
|
||||
} catch (err) {
|
||||
if (isEnoent(err)) continue;
|
||||
try {
|
||||
const original = await Bun.file(launcher).text();
|
||||
await Bun.write(launcher, SHIM_FORWARDERS[ext]);
|
||||
forwarded.push({ launcher, original });
|
||||
} catch {
|
||||
stuck.push(launcher);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Verify the exe by its explicit path: $which cached the shim path when
|
||||
// the update target was resolved, and the shim was just renamed away, so
|
||||
// a PATH re-resolution here would test a file that no longer exists.
|
||||
const verify = options.verifyBinary ?? verifyBinaryAtPath;
|
||||
const verification = await verify(exePath, expectedVersion);
|
||||
if (!verification.ok) {
|
||||
for (const { launcher, backup } of retired) {
|
||||
try {
|
||||
await fs.promises.rename(backup, launcher);
|
||||
} catch {}
|
||||
// Verify the exe by its explicit path: $which cached the shim path when
|
||||
// the update target was resolved, and the shim was just renamed away, so
|
||||
// a PATH re-resolution here would test a file that no longer exists.
|
||||
const verify = options.verifyBinary ?? verifyBinaryAtPath;
|
||||
const verification = await verify(exePath, expectedVersion);
|
||||
if (!verification.ok) {
|
||||
for (const { launcher, backup } of retired) {
|
||||
try {
|
||||
await fs.promises.rename(backup, launcher);
|
||||
} catch {}
|
||||
}
|
||||
for (const { launcher, original } of forwarded) {
|
||||
try {
|
||||
await Bun.write(launcher, original);
|
||||
} catch {}
|
||||
}
|
||||
await unlinkIfExists(exePath);
|
||||
throw new Error(
|
||||
`${formatVerificationFailure(verification, expectedVersion)}; restored previous ${APP_NAME} launcher`,
|
||||
);
|
||||
}
|
||||
for (const { launcher, original } of forwarded) {
|
||||
try {
|
||||
await Bun.write(launcher, original);
|
||||
} catch {}
|
||||
for (const { backup } of retired) {
|
||||
await removeBackupBestEffort(backup);
|
||||
}
|
||||
await unlinkIfExists(exePath);
|
||||
throw new Error(
|
||||
`${formatVerificationFailure(verification, expectedVersion)}; restored previous ${APP_NAME} launcher`,
|
||||
);
|
||||
}
|
||||
for (const { backup } of retired) {
|
||||
await removeBackupBestEffort(backup);
|
||||
}
|
||||
// Reclaim exe backups and retired-shim leftovers from earlier attempts.
|
||||
for (const ext of [".exe", "", ".cmd", ".ps1", ".bat"]) {
|
||||
await sweepStaleUpdateArtifacts(path.join(launcherDir, `${APP_NAME}${ext}`));
|
||||
}
|
||||
// Reclaim exe backups and retired-shim leftovers from earlier attempts.
|
||||
for (const ext of [".exe", "", ".cmd", ".ps1", ".bat"]) {
|
||||
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).`));
|
||||
}
|
||||
|
||||
@@ -1105,12 +1105,16 @@ describe("update-cli concurrent binary updates", () => {
|
||||
});
|
||||
}
|
||||
|
||||
// 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 () => {
|
||||
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 });
|
||||
|
||||
async function prepare(): Promise<{ dir: string; targetPath: string }> {
|
||||
const loadedTheme = await getThemeByName("dark");
|
||||
if (!loadedTheme) throw new Error("theme unavailable");
|
||||
setThemeInstance(loadedTheme);
|
||||
@@ -1118,6 +1122,16 @@ describe("update-cli concurrent binary updates", () => {
|
||||
const dir = await makeTempDir();
|
||||
const targetPath = path.join(dir, "omp");
|
||||
await Bun.write(targetPath, "old binary");
|
||||
return { dir, targetPath };
|
||||
}
|
||||
|
||||
// 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 () => {
|
||||
const { dir, targetPath } = await prepare();
|
||||
|
||||
const aWroteFirstChunk = Promise.withResolvers<void>();
|
||||
const letAFinish = Promise.withResolvers<void>();
|
||||
@@ -1139,13 +1153,6 @@ describe("update-cli concurrent binary updates", () => {
|
||||
}
|
||||
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,
|
||||
@@ -1165,4 +1172,39 @@ describe("update-cli concurrent binary updates", () => {
|
||||
const residue = (await fs.readdir(dir)).filter(name => name.endsWith(".new"));
|
||||
expect(residue).toEqual([]);
|
||||
});
|
||||
|
||||
// Regression: a failed verification must still roll back its own backup even
|
||||
// when another update completes while it is held. The per-target lock
|
||||
// serializes the swap + sweep, so the concurrent run's sweep cannot reclaim
|
||||
// the live backup before the rollback renames it back.
|
||||
it("rolls back its backup when verification fails while another update runs", async () => {
|
||||
const { dir, targetPath } = await prepare();
|
||||
|
||||
const enteredVerify = Promise.withResolvers<void>();
|
||||
const releaseVerify = Promise.withResolvers<void>();
|
||||
const failingVerify = async () => {
|
||||
enteredVerify.resolve();
|
||||
await releaseVerify.promise;
|
||||
return { ok: false, actual: "0.0.0", path: targetPath };
|
||||
};
|
||||
|
||||
const runA = updateViaBinaryAt(targetPath, version, {
|
||||
binaryName,
|
||||
fetchImpl: fastFetch,
|
||||
verifyInstalledVersion: failingVerify,
|
||||
});
|
||||
await enteredVerify.promise;
|
||||
const runB = updateViaBinaryAt(targetPath, version, {
|
||||
binaryName,
|
||||
fetchImpl: fastFetch,
|
||||
verifyInstalledVersion: verify,
|
||||
});
|
||||
releaseVerify.resolve();
|
||||
await expect(runA).rejects.toThrow(/still reports 0\.0\.0 \(expected 999\.0\.0\)/);
|
||||
await runB;
|
||||
|
||||
expect(await Bun.file(targetPath).bytes()).toEqual(new Uint8Array(payload));
|
||||
const residue = (await fs.readdir(dir)).filter(name => name.endsWith(".bak") || name.endsWith(".new"));
|
||||
expect(residue).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user