fix(studio-server): freeze still appears whole, so a slow extraction cannot break its undo (#4982)

* fix(studio-server): freeze still appears whole, so a slow extraction cannot break its undo

The freeze route reserved the still's final name with an empty file before
ffmpeg ran. When extraction outlasted the history's quiet time, history
committed that empty file as an outside change, and Cmd+Z then restored a
0-byte PNG. ffmpeg now writes under an atomic temp name the history skips,
and the still is published with createFileAtomically (EEXIST still refuses
a taken name).

* fix(core): a direct exclusive write that fails midway leaves no partial file

Without hard links, createFileAtomically writes the destination directly.
A full disk then left a partial file behind (a corrupt upload or freeze
still); it is now removed, and a name another writer holds is never
touched. Freeze tests: the slow-extraction test leaves a half-written temp
file while history commits, and concurrent extractions meet at a barrier
instead of a real timer.

* fix(core): a quota reported only at close still removes the partial file

* test(core): the direct write closes once and keeps its write error when cleanup fails

* test(core): a failed direct write closes its descriptor exactly once

* refactor(studio-server): freeze names carry what their comments said
This commit is contained in:
Miguel Ángel
2026-10-04 04:16:02 +00:00
committed by GitHub
parent a8dcdd109a
commit 4e118824b3
6 changed files with 275 additions and 102 deletions
+61
View File
@@ -187,6 +187,8 @@ describe("createFileAtomically", () => {
return fs.writeFileSync(path, ...args);
},
chmodSync: fs.chmodSync,
openSync: fs.openSync,
closeSync: fs.closeSync,
linkSync: (from: fs.PathLike, to: fs.PathLike) => {
expect(readFileSync(from, "utf-8")).toBe("complete html");
return fs.linkSync(from, to);
@@ -230,6 +232,8 @@ describe("createFileAtomically", () => {
return {
writeFileSync: fs.writeFileSync,
chmodSync: fs.chmodSync,
openSync: fs.openSync,
closeSync: fs.closeSync,
linkSync: () => {
throw Object.assign(new Error(code), { code });
},
@@ -250,6 +254,61 @@ describe("createFileAtomically", () => {
},
);
it("removes the partial file a direct write leaves on a full disk, keeping the error", () => {
const dir = tempDir();
const file = join(dir, "index.html");
const closed: number[] = [];
const operations = {
...failingLink("EXDEV"),
writeFileSync: (target: fs.PathOrFileDescriptor, ...args: any[]) => {
if (typeof target !== "number") return fs.writeFileSync(target, ...args);
fs.writeFileSync(target, "ht");
throw Object.assign(new Error("ENOSPC"), { code: "ENOSPC" });
},
closeSync: (fd: number) => {
closed.push(fd);
fs.closeSync(fd);
throw Object.assign(new Error("EIO"), { code: "EIO" });
},
};
expect(() => createFileAtomically(file, "html", operations)).toThrow("ENOSPC");
expect(fs.readdirSync(dir)).toEqual([]);
// Unlinking an open file succeeds, so only the count shows the descriptor was not leaked.
expect(closed).toHaveLength(1);
});
it("removes the file when a network share reports its quota only at close", () => {
const dir = tempDir();
const file = join(dir, "index.html");
const closed: number[] = [];
const operations = {
...failingLink("EXDEV"),
closeSync: (fd: number) => {
closed.push(fd);
fs.closeSync(fd);
throw Object.assign(new Error("EDQUOT"), { code: "EDQUOT" });
},
};
expect(() => createFileAtomically(file, "html", operations)).toThrow("EDQUOT");
expect(fs.readdirSync(dir)).toEqual([]);
// A second close could hit a descriptor another request has reused since.
expect(closed).toHaveLength(1);
});
it("keeps another writer's file when the direct write finds the name taken", () => {
const dir = tempDir();
const file = join(dir, "index.html");
writeFileSync(file, "theirs");
expect(() => createFileAtomically(file, "html", failingLink("EXDEV"))).toThrow(
expect.objectContaining({ code: "EEXIST" }),
);
expect(readFileSync(file, "utf-8")).toBe("theirs");
expect(fs.readdirSync(dir)).toEqual(["index.html"]);
});
it("propagates any other link failure without writing the destination", () => {
const dir = tempDir();
const file = join(dir, "index.html");
@@ -265,6 +324,8 @@ describe("createFileAtomically", () => {
const operations = {
writeFileSync: fs.writeFileSync,
chmodSync: fs.chmodSync,
openSync: fs.openSync,
closeSync: fs.closeSync,
linkSync: fs.linkSync,
unlinkSync: () => {
throw Object.assign(new Error("EBUSY"), { code: "EBUSY" });
+30 -3
View File
@@ -4,6 +4,7 @@ import { basename, dirname, isAbsolute, join, resolve, sep } from "node:path";
import { realpath } from "./safePath.js";
type SiblingFileSystem = Pick<typeof fs, "writeFileSync" | "chmodSync" | "unlinkSync">;
type DirectFileSystem = SiblingFileSystem & Pick<typeof fs, "openSync" | "closeSync">;
// Codes a volume without hard links (FAT, exFAT, some network shares) answers link() with.
// EISDIR: libuv maps Windows ERROR_INVALID_FUNCTION (FAT/exFAT refusing a link) to it; nodejs/node#65817.
@@ -41,15 +42,14 @@ export function replaceFileAtomically(
export function createFileAtomically(
filePath: string,
content: string | Uint8Array,
operations: SiblingFileSystem & Pick<typeof fs, "linkSync"> = fs,
operations: DirectFileSystem & Pick<typeof fs, "linkSync"> = fs,
): void {
publishSibling(filePath, content, undefined, operations, (tempPath) => {
try {
operations.linkSync(tempPath, filePath);
} catch (error) {
if (!NO_HARD_LINKS.has(errorCode(error))) throw error;
// Without hard links, keep today's direct exclusive write.
operations.writeFileSync(filePath, content, { flag: "wx" });
createDirectly(filePath, content, operations);
}
try {
operations.unlinkSync(tempPath);
@@ -59,6 +59,33 @@ export function createFileAtomically(
});
}
/** Without hard links: an exclusive write; a partial file it made (a full disk) is removed, a taken name never. */
function createDirectly(
filePath: string,
content: string | Uint8Array,
operations: DirectFileSystem,
): void {
const fd = operations.openSync(filePath, "wx");
let open = true;
try {
operations.writeFileSync(fd, content);
open = false;
operations.closeSync(fd);
} catch (error) {
try {
if (open) operations.closeSync(fd);
} catch {
// Preserve the write error; cleanup is best effort.
}
try {
operations.unlinkSync(filePath);
} catch {
// Preserve the write error; cleanup is best effort.
}
throw error;
}
}
/** The file a write to `filePath` lands on: folder links and file links followed as the system does. */
export function resolveWritePath(filePath: string): string {
let path = resolve(filePath);
@@ -61,8 +61,10 @@ describe("freezeFrameMediaTime", () => {
});
describe("freezeExtractArgs", () => {
it("seeks before the input and writes one frame into the file the route claimed", () => {
expect(freezeExtractArgs("/p/a.mp4", 7.2004, "/p/assets/freeze/a-3200.png")).toEqual([
it("seeks before the input and writes one PNG frame to an output of any name", () => {
expect(
freezeExtractArgs("/p/a.mp4", 7.2004, "/p/assets/freeze/a-3200.png.hf0a1b2c.tmp"),
).toEqual([
"-y",
"-ss",
"7.2",
@@ -70,7 +72,13 @@ describe("freezeExtractArgs", () => {
"/p/a.mp4",
"-frames:v",
"1",
"/p/assets/freeze/a-3200.png",
"-c:v",
"png",
"-f",
"image2",
"-update",
"1",
"/p/assets/freeze/a-3200.png.hf0a1b2c.tmp",
]);
});
});
@@ -40,7 +40,22 @@ export function freezeFrameMediaTime(input: {
}
export function freezeExtractArgs(src: string, mediaTime: number, output: string): string[] {
return ["-y", "-ss", String(round3(mediaTime)), "-i", src, "-frames:v", "1", output];
return [
"-y",
"-ss",
String(round3(mediaTime)),
"-i",
src,
"-frames:v",
"1",
"-c:v",
"png",
"-f",
"image2",
"-update",
"1",
output,
];
}
export const randomStillToken = (): string => randomBytes(4).toString("hex");
@@ -1,14 +1,26 @@
import { afterEach, describe, expect, it } from "vitest";
import { Hono } from "hono";
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
import {
existsSync,
mkdirSync,
mkdtempSync,
readdirSync,
readFileSync,
rmSync,
writeFileSync,
} from "node:fs";
import { tmpdir } from "node:os";
import { basename, dirname, join } from "node:path";
import { isAtomicTempPath } from "@hyperframes/core/atomic-file";
import { registerFreezeFrameRoutes, type FrameExtractor } from "./freezeFrame";
import { fileContentVersion } from "../helpers/fileVersion";
import { openProjectHistory, type ProjectHistory } from "../history";
import { stubAdapter } from "./stubAdapter.test-helpers";
const tempDirs: string[] = [];
afterEach(() => {
const histories: ProjectHistory[] = [];
afterEach(async () => {
for (const history of histories.splice(0)) await history.close();
for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true });
});
@@ -36,14 +48,21 @@ function setup(
return { dir, post };
}
function pngWriter(calls: string[][]): FrameExtractor {
return async (args) => {
calls.push(args);
writeFileSync(args.at(-1) ?? "", "png");
return { ok: true };
};
}
const stillOf = (output: string) => output.replace(/\.hf[0-9a-f]{6}\.tmp$/, "");
const freezeFiles = (dir: string) => readdirSync(join(dir, "assets/freeze"));
describe("freeze-frame route", () => {
it("extracts the frame under the playhead and writes split + still in one write", async () => {
const calls: string[][] = [];
const { dir, post } = setup(async (args) => {
calls.push(args);
writeFileSync(args.at(-1) ?? "", "png");
return { ok: true };
});
const { dir, post } = setup(pngWriter(calls));
const res = await post({
path: "index.html",
expectedVersion: fileContentVersion(html),
@@ -54,25 +73,49 @@ describe("freeze-frame route", () => {
await res.json();
expect(res.status).toBe(200);
const output = calls[0]?.at(-1) ?? "";
expect(calls[0]).toEqual([
"-y",
"-ss",
"2.5",
"-i",
join(dir, "media/talk.mp4"),
"-frames:v",
"1",
output,
]);
expect(dirname(output)).toBe(join(dir, "assets/freeze"));
expect(basename(output)).toMatch(/^talk-[0-9a-f]{10}-2500-[0-9a-f]{8}\.png$/);
expect(body.imageSrc).toBe(`assets/freeze/${basename(output)}`);
expect(body.stillPath).toBe(`assets/freeze/${basename(output)}`);
expect(calls[0]?.slice(0, 5)).toEqual(["-y", "-ss", "2.5", "-i", join(dir, "media/talk.mp4")]);
expect(isAtomicTempPath(output)).toBe(true);
const still = stillOf(output);
expect(dirname(still)).toBe(join(dir, "assets/freeze"));
expect(basename(still)).toMatch(/^talk-[0-9a-f]{10}-2500-[0-9a-f]{8}\.png$/);
expect(body.imageSrc).toBe(`assets/freeze/${basename(still)}`);
expect(body.stillPath).toBe(`assets/freeze/${basename(still)}`);
expect(readFileSync(still, "utf-8")).toBe("png");
expect(freezeFiles(dir)).toEqual([basename(still)]);
expect(body.before).toBe(html);
expect(readFileSync(join(dir, "index.html"), "utf-8")).toBe(body.after);
expect(body.after).toContain('id="talk-freeze"');
});
it("keeps the still out of history until it is whole, so undo removes it after a slow extraction", async () => {
let history: ProjectHistory | undefined;
const { dir, post } = setup(async (args) => {
// ffmpeg outlasting the watcher's quiet time: history commits the project with the still half written.
writeFileSync(args.at(-1) ?? "", "pn");
await history?.flush();
writeFileSync(args.at(-1) ?? "", "png");
return { ok: true };
});
const historyRoot = mkdtempSync(join(tmpdir(), "hf-freeze-history-"));
tempDirs.push(historyRoot);
history = await openProjectHistory({ projectDir: dir, historyRoot });
histories.push(history);
const res = await post({
path: "index.html",
expectedVersion: fileContentVersion(html),
target: { id: "talk" },
playhead: 2.5,
});
const { stillPath = "" }: { stillPath?: string } = await res.json();
expect(res.status).toBe(200);
const you = { kind: "person" as const, name: "You" };
const claimed = await history.claim(you, "Freeze frame", ["index.html", stillPath]);
expect(await history.undo(claimed?.id ?? "", { who: you })).toMatchObject({ ok: true });
expect(existsSync(join(dir, stillPath))).toBe(false);
expect(readFileSync(join(dir, "index.html"), "utf-8")).toBe(html);
expect(history.list().map((entry) => entry.who.kind)).not.toContain("outside");
});
it("refuses a stale version without extracting", async () => {
const calls: string[][] = [];
const { post } = setup(async (args) => {
@@ -89,30 +132,15 @@ describe("freeze-frame route", () => {
expect(calls).toEqual([]);
});
it("leaves the file untouched, and no partial still, when extraction fails", async () => {
let still = "";
it.each([
["extraction fails after a partial write", "partial", false],
["ffmpeg succeeds without writing a frame", null, true],
] as const)("leaves the file untouched, and no still, when %s", async (_, written, ok) => {
let output = "";
const { dir, post } = setup(async (args) => {
still = args.at(-1) ?? "";
writeFileSync(still, "partial");
return { ok: false, error: "boom" };
});
const res = await post({
path: "index.html",
expectedVersion: fileContentVersion(html),
target: { id: "talk" },
playhead: 1,
});
expect(res.status).toBe(500);
expect(readFileSync(join(dir, "index.html"), "utf-8")).toBe(html);
expect(still).not.toBe("");
expect(existsSync(still)).toBe(false);
});
it("refuses, leaving nothing behind, when ffmpeg succeeds without writing a frame", async () => {
let still = "";
const { dir, post } = setup(async (args) => {
still = args.at(-1) ?? "";
return { ok: true };
output = args.at(-1) ?? "";
if (written) writeFileSync(output, written);
return ok ? { ok } : { ok, error: "boom" };
});
const res = await post({
path: "index.html",
@@ -122,8 +150,8 @@ describe("freeze-frame route", () => {
});
expect(res.status).toBe(500);
expect(readFileSync(join(dir, "index.html"), "utf-8")).toBe(html);
expect(still).not.toBe("");
expect(existsSync(still)).toBe(false);
expect(output).not.toBe("");
expect(freezeFiles(dir)).toEqual([]);
});
it("removes the still it extracted when the page changed before the write", async () => {
@@ -142,20 +170,16 @@ describe("freeze-frame route", () => {
});
expect(res.status).toBe(409);
expect(still).not.toBe("");
expect(existsSync(still)).toBe(false);
expect(freezeFiles(dir)).toEqual([]);
});
it("keeps a traversal clip id inside assets/freeze, for the ffmpeg output and the still's src", async () => {
const evil = html.replace('id="talk"', 'id="../../../../outside/frame"');
const calls: string[][] = [];
const { dir, post } = setup(
async (args) => {
calls.push(args);
writeFileSync(args.at(-1) ?? "", "png");
return { ok: true };
},
{ path: "scenes/a.html", html: evil.replace('src="media/', 'src="../media/') },
);
const { dir, post } = setup(pngWriter(calls), {
path: "scenes/a.html",
html: evil.replace('src="media/', 'src="../media/'),
});
const res = await post({
path: "scenes/a.html",
expectedVersion: fileContentVersion(evil.replace('src="media/', 'src="../media/')),
@@ -164,7 +188,7 @@ describe("freeze-frame route", () => {
});
const body: { imageSrc?: string; after?: string; stillPath?: string } = await res.json();
expect(res.status).toBe(200);
const output = calls[0]?.at(-1) ?? "";
const output = stillOf(calls[0]?.at(-1) ?? "");
// The history names project files from the project root, not from the page that shows the still.
expect(body.stillPath).toBe(`assets/freeze/${basename(output)}`);
expect(basename(output)).toMatch(/^____________outside_frame-[0-9a-f]{10}-2500-/);
@@ -183,22 +207,32 @@ describe("freeze-frame route", () => {
<video id="${b}" class="clip" src="media/b.mp4" data-start="0" data-duration="6" data-track-index="1"></video>
</div>`;
function writingExtractor(outputs: string[]): FrameExtractor {
/** Each extraction writes only once `together` of them have started, so concurrent requests overlap. */
function writingExtractor(outputs: string[], together = 1): FrameExtractor {
let allStarted = () => {};
const started = new Promise<void>((resolve) => (allStarted = resolve));
return async (args) => {
const output = args.at(-1) ?? "";
outputs.push(output);
await new Promise((resolve) => setTimeout(resolve, 5));
try {
writeFileSync(output, `frame of ${args[4]} #${outputs.length}`, {
flag: args.includes("-y") ? "w" : "wx",
});
return { ok: true };
} catch (error) {
return { ok: false, error: String(error) };
}
outputs.push(stillOf(output));
if (outputs.length >= together) allStarted();
await started;
writeFileSync(output, `frame of ${args[4]} #${outputs.length}`);
return { ok: true };
};
}
/** Freezes talk, puts the page back, and freezes it again: the first still's bytes and the second reply. */
async function freezeTalkTwice(
post: (body: unknown) => Promise<Response>,
dir: string,
outputs: string[],
) {
expect((await freeze(post, dir, "talk")).status).toBe(200);
const first = readFileSync(outputs[0] ?? "", "utf-8");
writeFileSync(join(dir, "index.html"), html);
return { first, second: await freeze(post, dir, "talk") };
}
function freeze(post: (body: unknown) => Promise<Response>, dir: string, id: string) {
return post({
path: "index.html",
@@ -232,17 +266,15 @@ describe("freeze-frame route", () => {
it("writes a new still when the same clip is frozen again at the same time", async () => {
const outputs: string[] = [];
const { dir, post } = setup(writingExtractor(outputs));
expect((await freeze(post, dir, "talk")).status).toBe(200);
const first = readFileSync(outputs[0] ?? "", "utf-8");
writeFileSync(join(dir, "index.html"), html);
expect((await freeze(post, dir, "talk")).status).toBe(200);
const { first, second } = await freezeTalkTwice(post, dir, outputs);
expect(second.status).toBe(200);
expect(outputs[1]).not.toBe(outputs[0]);
expect(readFileSync(outputs[0] ?? "", "utf-8")).toBe(first);
});
it("gives concurrent requests distinct stills and keeps only the one the page uses", async () => {
const outputs: string[] = [];
const { dir, post } = setup(writingExtractor(outputs));
const { dir, post } = setup(writingExtractor(outputs, 2));
const results = await Promise.all([freeze(post, dir, "talk"), freeze(post, dir, "talk")]);
expect(results.map((res) => res.status).sort()).toEqual([200, 409]);
expect(new Set(outputs).size).toBe(2);
@@ -254,23 +286,21 @@ describe("freeze-frame route", () => {
it("keeps the winner's still when two requests race for the same name", async () => {
const outputs: string[] = [];
const { dir, post } = setup(writingExtractor(outputs), undefined, () => "fixed");
const { dir, post } = setup(writingExtractor(outputs, 2), undefined, () => "fixed");
const results = await Promise.all([freeze(post, dir, "talk"), freeze(post, dir, "talk")]);
expect(results.map((res) => res.status).sort()).toEqual([200, 409]);
expect(outputs).toHaveLength(1);
expect(existsSync(outputs[0] ?? "")).toBe(true);
expect(new Set(outputs).size).toBe(1);
expect(freezeFiles(dir)).toEqual([basename(outputs[0] ?? "")]);
expect(readFileSync(join(dir, "index.html"), "utf-8")).toContain(basename(outputs[0] ?? ""));
});
it("refuses, without extracting, when the still's name is already taken", async () => {
it("refuses, keeping the first still, when the still's name is already taken", async () => {
const outputs: string[] = [];
const { dir, post } = setup(writingExtractor(outputs), undefined, () => "fixed");
expect((await freeze(post, dir, "talk")).status).toBe(200);
const first = readFileSync(outputs[0] ?? "", "utf-8");
writeFileSync(join(dir, "index.html"), html);
expect((await freeze(post, dir, "talk")).status).toBe(409);
expect(outputs).toHaveLength(1);
const { first, second } = await freezeTalkTwice(post, dir, outputs);
expect(second.status).toBe(409);
expect(readFileSync(outputs[0] ?? "", "utf-8")).toBe(first);
expect(freezeFiles(dir)).toEqual([basename(outputs[0] ?? "")]);
expect(readFileSync(join(dir, "index.html"), "utf-8")).toBe(html);
});
});
@@ -1,11 +1,15 @@
import { execFile } from "node:child_process";
import { closeSync, openSync, readFileSync, rmSync, statSync } from "node:fs";
import { dirname, join, relative, sep } from "node:path";
import { readFileSync, rmSync, statSync } from "node:fs";
import { basename, dirname, join, relative, sep } from "node:path";
import type { Hono } from "hono";
import { findFfBinary } from "@hyperframes/parsers/ff-binaries";
import type { StudioApiAdapter } from "../types.js";
import { mkdirWithinProject, pinWithinProject } from "../helpers/safePath.js";
import { replaceFileAtomically } from "@hyperframes/core/atomic-file";
import {
atomicTempPath,
createFileAtomically,
replaceFileAtomically,
} from "@hyperframes/core/atomic-file";
import { backupPathForResponse, snapshotBeforeWrite } from "../helpers/backupJournal.js";
import {
createWriteToken,
@@ -80,9 +84,18 @@ function readExpected(absPath: string, expectedVersion: string): { content: stri
: { error: "file conflict", status: 409 };
}
function claimStillNameBeforeExtracting(path: string): boolean {
function readFrame(path: string): Buffer | null {
try {
closeSync(openSync(path, "wx"));
return readFileSync(path);
} catch (error) {
if ((error as NodeJS.ErrnoException).code === "ENOENT") return null;
throw error;
}
}
function publishStill(path: string, frame: Buffer): boolean {
try {
createFileAtomically(path, frame);
return true;
} catch (error) {
if ((error as NodeJS.ErrnoException).code === "EEXIST") return false;
@@ -90,6 +103,27 @@ function claimStillNameBeforeExtracting(path: string): boolean {
}
}
async function extractThenPublishWhole(
extract: FrameExtractor,
mediaPath: string,
mediaTime: number,
imagePath: string,
): Promise<Failure | null> {
const unseenByHistory = atomicTempPath(imagePath);
try {
const extracted = await extract(freezeExtractArgs(mediaPath, mediaTime, unseenByHistory));
const frame = extracted.ok ? readFrame(unseenByHistory) : null;
if (!frame?.length) {
const reason = extracted.ok ? "no frame at this time" : (extracted.error ?? "ffmpeg failed");
return { error: `Could not extract the frame: ${reason}`, status: 500 };
}
if (publishStill(imagePath, frame)) return null;
return { error: `freeze still already exists: ${basename(imagePath)}`, status: 409 };
} finally {
rmSync(unseenByHistory, { force: true });
}
}
async function extractStill(
projectDir: string,
absPath: string,
@@ -107,15 +141,13 @@ async function extractStill(
if (!imagePath || dirname(imagePath) !== freezeDir) {
return { error: `forbidden freeze path: ${fileName}`, status: 403 };
}
if (!claimStillNameBeforeExtracting(imagePath))
return { error: `freeze still already exists: ${fileName}`, status: 409 };
const extracted = await tools.extract(freezeExtractArgs(mediaPath, source.mediaTime, imagePath));
const partialOrNoFrame = !extracted.ok || statSync(imagePath).size === 0;
if (partialOrNoFrame) {
rmSync(imagePath, { force: true });
const reason = extracted.ok ? "no frame at this time" : (extracted.error ?? "ffmpeg failed");
return { error: `Could not extract the frame: ${reason}`, status: 500 };
}
const failed = await extractThenPublishWhole(
tools.extract,
mediaPath,
source.mediaTime,
imagePath,
);
if (failed) return failed;
const depth = relative(projectDir, fileDir).split(sep).filter(Boolean).length;
const stillPath = `${FREEZE_DIR.join("/")}/${fileName}`;
return { imageSrc: `${"../".repeat(depth)}${stillPath}`, stillPath, imagePath };