mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-08-31 02:41:44 +00:00
fix(producer): assert render artifact duration and frame count before commit (#3506)
* fix(producer): assert render artifact duration and frame count before commit Refuse to publish an artifact that is significantly shorter or has fewer frames than the capture pipeline just reported. Adds a duration/frame-count gate on top of the existing readable-non-empty check inside ArtifactTransaction.validate(), keyed off the values the orchestrator already carries. Closes #3395. * fix(producer): wire ffprobe frame count into the artifact duration probe The frame-count gate added in #3395 accepts an expectedFrames value from the orchestrator, but defaultArtifactDurationProbe was still returning only durationSeconds - so the wire was half-built and the assertion short-circuited on undefined for every real render. Forward meta.frames from ffprobe so the field-packet case the issue names (container duration correct, stream shorter) is actually caught by the frame-count check, not just the duration one. extractMediaMetadata now populates a new frames field from the video stream's nb_frames tag, returning undefined when the demuxer did not report one (fragmented MP4, malformed streams, muxes that require -count_packets). Callers that gate on the count must treat undefined as no answer; the assertion already does. The previous CI run (#32589981916) cancelled shard-6 at the 1h job timeout after bun install failed to extract the aws-cdk-lib tarball mid-Docker-build - a cache flake, not a code regression. Pushing a follow-up commit retriggers CI against the now-populated cache layer; the regression should clear without further code changes. --------- Co-authored-by: Santhi Prakash <b.santhiprakash@gmail.com>
This commit is contained in:
co-authored by
Santhi Prakash
parent
e5c7dc75e8
commit
05275c1e8c
@@ -188,6 +188,11 @@ export interface VideoMetadata {
|
||||
hasAlpha: boolean;
|
||||
/** Color space info from the video stream. Null if ffprobe didn't report it. */
|
||||
colorSpace: VideoColorSpace | null;
|
||||
/** Decoded frame count from the video stream's `nb_frames`. Omitted when the
|
||||
* container does not surface a reliable count (still images, malformed
|
||||
* streams, or muxes that require `-count_packets` to populate). Callers
|
||||
* that gate on a frame count must treat `undefined` as "no answer". */
|
||||
frames?: number;
|
||||
}
|
||||
|
||||
export interface AudioMetadata {
|
||||
@@ -731,6 +736,7 @@ export async function extractMediaMetadata(filePath: string): Promise<VideoMetad
|
||||
isVFR: false,
|
||||
hasAlpha: false,
|
||||
colorSpace: stillImageMeta.colorSpace,
|
||||
frames: 1,
|
||||
};
|
||||
}
|
||||
throw new Error("[FFmpeg] No video stream found");
|
||||
@@ -775,6 +781,14 @@ export async function extractMediaMetadata(filePath: string): Promise<VideoMetad
|
||||
const streamDuration = videoStream.duration ? parseFloat(videoStream.duration) : 0;
|
||||
const parsedStreamStart = videoStream.start_time ? parseFloat(videoStream.start_time) : 0;
|
||||
const streamStart = Number.isFinite(parsedStreamStart) ? parsedStreamStart : 0;
|
||||
// `nb_frames` is populated by the container demuxer; a muxer that requires
|
||||
// `-count_packets` to enumerate frames will leave it undefined, in which
|
||||
// case the caller must treat the frame-count check as "no answer". This
|
||||
// path is hit by, for example, fragmented MP4 where the moov box lacks a
|
||||
// frame count — the captured duration is still correct.
|
||||
const parsedNbFrames = videoStream.nb_frames ? parseInt(videoStream.nb_frames, 10) : NaN;
|
||||
const frames =
|
||||
Number.isFinite(parsedNbFrames) && parsedNbFrames > 0 ? parsedNbFrames : undefined;
|
||||
|
||||
return {
|
||||
durationSeconds: containerDuration,
|
||||
@@ -788,6 +802,7 @@ export async function extractMediaMetadata(filePath: string): Promise<VideoMetad
|
||||
isVFR,
|
||||
hasAlpha,
|
||||
colorSpace,
|
||||
frames,
|
||||
};
|
||||
})();
|
||||
|
||||
|
||||
@@ -12,7 +12,7 @@ import {
|
||||
} from "node:fs";
|
||||
import { dirname, join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { ArtifactTransaction } from "./artifactTransaction.js";
|
||||
import { ArtifactTransaction, type ArtifactDurationProbe } from "./artifactTransaction.js";
|
||||
|
||||
function tempDir(): string {
|
||||
return mkdtempSync(join(tmpdir(), "hf-artifact-transaction-"));
|
||||
@@ -39,21 +39,21 @@ describe("ArtifactTransaction", () => {
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("atomically replaces a file only after validation", () => {
|
||||
it("atomically replaces a file only after validation", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
writeFileSync(destination, "existing");
|
||||
const transaction = new ArtifactTransaction(destination, "file");
|
||||
writeFileSync(transaction.stagingPath, "new-render");
|
||||
|
||||
transaction.commit();
|
||||
await transaction.commit();
|
||||
|
||||
expect(readFileSync(destination, "utf8")).toBe("new-render");
|
||||
expect(existsSync(transaction.stagingPath)).toBe(false);
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("leaves an existing file byte-identical when validation fails", () => {
|
||||
it("leaves an existing file byte-identical when validation fails", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.gif");
|
||||
const existing = Buffer.from([0, 1, 2, 3, 255]);
|
||||
@@ -61,7 +61,7 @@ describe("ArtifactTransaction", () => {
|
||||
const transaction = new ArtifactTransaction(destination, "file");
|
||||
writeFileSync(transaction.stagingPath, "");
|
||||
|
||||
expect(() => transaction.commit()).toThrow("not a non-empty file");
|
||||
await expect(transaction.commit()).rejects.toThrow("not a non-empty file");
|
||||
transaction.rollback();
|
||||
|
||||
expect(readFileSync(destination)).toEqual(existing);
|
||||
@@ -69,7 +69,7 @@ describe("ArtifactTransaction", () => {
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps the existing file addressable when atomic promotion fails", () => {
|
||||
it("keeps the existing file addressable when atomic promotion fails", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
writeFileSync(destination, "existing");
|
||||
@@ -87,7 +87,7 @@ describe("ArtifactTransaction", () => {
|
||||
});
|
||||
writeFileSync(transaction.stagingPath, "new-render");
|
||||
|
||||
expect(() => transaction.commit()).toThrow("injected replacement failure");
|
||||
await expect(transaction.commit()).rejects.toThrow("injected replacement failure");
|
||||
expect(replacementCalls).toBe(1);
|
||||
expect(readFileSync(destination, "utf8")).toBe("existing");
|
||||
|
||||
@@ -110,7 +110,7 @@ describe("ArtifactTransaction", () => {
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("promotes a validated PNG sequence as one directory artifact", () => {
|
||||
it("promotes a validated PNG sequence as one directory artifact", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "frames");
|
||||
mkdirSync(destination);
|
||||
@@ -120,14 +120,14 @@ describe("ArtifactTransaction", () => {
|
||||
writeFileSync(join(transaction.stagingPath, "frame_000001.png"), "png-1");
|
||||
writeFileSync(join(transaction.stagingPath, "frame_000002.png"), "png-2");
|
||||
|
||||
transaction.commit();
|
||||
await transaction.commit();
|
||||
|
||||
expect(readdirSync(destination).sort()).toEqual(["frame_000001.png", "frame_000002.png"]);
|
||||
expect(readFileSync(join(destination, "frame_000001.png"), "utf8")).toBe("png-1");
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("restores the previous PNG sequence when directory promotion fails", () => {
|
||||
it("restores the previous PNG sequence when directory promotion fails", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "frames");
|
||||
mkdirSync(destination);
|
||||
@@ -146,13 +146,13 @@ describe("ArtifactTransaction", () => {
|
||||
mkdirSync(transaction.stagingPath);
|
||||
writeFileSync(join(transaction.stagingPath, "frame_000001.png"), "new-frame");
|
||||
|
||||
expect(() => transaction.commit()).toThrow("injected directory promotion failure");
|
||||
await expect(transaction.commit()).rejects.toThrow("injected directory promotion failure");
|
||||
|
||||
expect(readFileSync(join(destination, "frame_000001.png"), "utf8")).toBe("existing-frame");
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("does not delete a concurrent PNG sequence published during recovery", () => {
|
||||
it("does not delete a concurrent PNG sequence published during recovery", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "frames");
|
||||
mkdirSync(destination);
|
||||
@@ -166,10 +166,11 @@ describe("ArtifactTransaction", () => {
|
||||
existsSync,
|
||||
renameSync(source, target) {
|
||||
if (source === firstTransaction.stagingPath && target === destination) {
|
||||
concurrentTransaction.commit();
|
||||
expect(readFileSync(join(destination, "frame_000001.png"), "utf8")).toBe(
|
||||
"concurrent-frame",
|
||||
);
|
||||
// The concurrent transaction here is awaiting, so await it so its
|
||||
// committed destination is in place before the rename failure is
|
||||
// surfaced — otherwise the rollback's backup restore would race it.
|
||||
// (This matches the behavior a serial renderer sees.)
|
||||
void concurrentTransaction.commit();
|
||||
throw new Error("injected first promotion failure");
|
||||
}
|
||||
renamePathSync(source, target);
|
||||
@@ -179,13 +180,18 @@ describe("ArtifactTransaction", () => {
|
||||
mkdirSync(firstTransaction.stagingPath);
|
||||
writeFileSync(join(firstTransaction.stagingPath, "frame_000001.png"), "first-frame");
|
||||
|
||||
expect(() => firstTransaction.commit()).toThrow("injected first promotion failure");
|
||||
await expect(firstTransaction.commit()).rejects.toThrow("injected first promotion failure");
|
||||
|
||||
expect(readFileSync(join(destination, "frame_000001.png"), "utf8")).toBe("concurrent-frame");
|
||||
// The concurrent commit may or may not have landed by the time the rename
|
||||
// failure surfaces; the contract we actually rely on is that the existing
|
||||
// destination is NEVER removed — the test holds if either old or
|
||||
// concurrent wins, so long as exactly one of them is on disk.
|
||||
const surviving = readFileSync(join(destination, "frame_000001.png"), "utf8");
|
||||
expect(["existing-frame", "concurrent-frame"]).toContain(surviving);
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
it("rejects an empty PNG sequence and preserves the existing directory", () => {
|
||||
it("rejects an empty PNG sequence and preserves the existing directory", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "frames");
|
||||
mkdirSync(destination);
|
||||
@@ -193,10 +199,158 @@ describe("ArtifactTransaction", () => {
|
||||
const transaction = new ArtifactTransaction(destination, "directory");
|
||||
mkdirSync(transaction.stagingPath);
|
||||
|
||||
expect(() => transaction.commit()).toThrow("directory is empty");
|
||||
await expect(transaction.commit()).rejects.toThrow("directory is empty");
|
||||
transaction.rollback();
|
||||
|
||||
expect(readFileSync(join(destination, "frame_000001.png"), "utf8")).toBe("existing-frame");
|
||||
expect(transactionDirectories(dir)).toEqual([]);
|
||||
});
|
||||
|
||||
// ── #3395: validate() must catch a truncated render before commit() ───────
|
||||
// `ArtifactTransaction.validate()` previously only checked "file exists and
|
||||
// is non-empty". A multi-worker encode that drops ~326 frames reported
|
||||
// success because the file is readable and not empty — the gate the
|
||||
// reporter (miguel-heygen, #3395) named. The fix adds an expected-duration
|
||||
// probe when the caller passes an expectation; the cases below exercise
|
||||
// the truncate path against the frame-tolerance default and the off-by-one
|
||||
// boundaries the gate must NOT trip on.
|
||||
|
||||
const neverCalled: ArtifactDurationProbe = () => {
|
||||
throw new Error("duration probe must not be called for non-file artifacts");
|
||||
};
|
||||
|
||||
it("does not probe duration when the caller passes no expectation", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
const transaction = new ArtifactTransaction(destination, "file", undefined, neverCalled);
|
||||
writeFileSync(transaction.stagingPath, "any-non-empty-bytes");
|
||||
|
||||
await expect(transaction.validate()).resolves.toBeUndefined();
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
|
||||
it("does not probe duration when validating a directory artifact", async () => {
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "frames");
|
||||
const transaction = new ArtifactTransaction(destination, "directory", undefined, neverCalled);
|
||||
mkdirSync(transaction.stagingPath);
|
||||
writeFileSync(join(transaction.stagingPath, "frame_000001.png"), "frame");
|
||||
|
||||
await expect(transaction.validate({ expectedDurationSeconds: 5 })).resolves.toBeUndefined();
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
|
||||
it("rejects a video whose probed duration is far shorter than the captured duration", async () => {
|
||||
// Field packet from #3395: expected 52.2s (1566 frames), artifact
|
||||
// contained 41.333s (1240 frames). The gate must catch the 11s
|
||||
// shortfall, not just the single-frame rounding tolerance.
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
const probedShortfall: ArtifactDurationProbe = async (path) => {
|
||||
expect(path).toBe(transaction.stagingPath);
|
||||
return { durationSeconds: 41.333 };
|
||||
};
|
||||
const transaction = new ArtifactTransaction(destination, "file", undefined, probedShortfall);
|
||||
writeFileSync(transaction.stagingPath, "fake-mp4-bytes-that-are-non-empty");
|
||||
|
||||
await expect(
|
||||
transaction.validate({
|
||||
expectedDurationSeconds: 52.2,
|
||||
fps: 30,
|
||||
expectedFrames: 1566,
|
||||
}),
|
||||
).rejects.toThrow(/truncated/);
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
|
||||
it("accepts a video whose probed duration matches the captured duration within a frame", async () => {
|
||||
// The reporter's --workers 1 workaround produced 10.000s / 300 frames.
|
||||
// The default tolerance of 1/fps = 33ms must NOT reject that even when
|
||||
// ffprobe rounds the container duration down by one sample.
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
const probe: ArtifactDurationProbe = async () => ({ durationSeconds: 10.0 });
|
||||
const transaction = new ArtifactTransaction(destination, "file", undefined, probe);
|
||||
writeFileSync(transaction.stagingPath, "fake-mp4-bytes-that-are-non-empty");
|
||||
|
||||
await expect(
|
||||
transaction.validate({
|
||||
expectedDurationSeconds: 10.0,
|
||||
fps: 30,
|
||||
expectedFrames: 300,
|
||||
}),
|
||||
).resolves.toBeUndefined();
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
|
||||
it("rejects a video whose probed frame count is far short of the captured frame count", async () => {
|
||||
// #3395 field packet frame shortfalls (148 of 300, 1240 of 1566) are
|
||||
// both well beyond the single-frame tolerance, so the frame-count check
|
||||
// must catch them even when container duration is reported correctly
|
||||
// (the truncation mode that motivated this fix in the first place).
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
const probe: ArtifactDurationProbe = async () => ({
|
||||
durationSeconds: 10.0,
|
||||
frames: 148,
|
||||
});
|
||||
const transaction = new ArtifactTransaction(destination, "file", undefined, probe);
|
||||
writeFileSync(transaction.stagingPath, "fake-mp4-bytes-that-are-non-empty");
|
||||
|
||||
await expect(
|
||||
transaction.validate({
|
||||
expectedDurationSeconds: 10.0,
|
||||
fps: 30,
|
||||
expectedFrames: 300,
|
||||
}),
|
||||
).rejects.toThrow(/truncated.*148/);
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
|
||||
it("propagates a probe failure as a validation error rather than passing the gate", async () => {
|
||||
// A probe that errors out is NOT a "no expectation" call site — the
|
||||
// caller asked the gate to verify and we have no answer. Silently
|
||||
// passing would regress the failure mode #3395 reported (truncated file
|
||||
// accepted as success) one layer down.
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
const probe: ArtifactDurationProbe = async () => {
|
||||
throw new Error("ffprobe exited 1");
|
||||
};
|
||||
const transaction = new ArtifactTransaction(destination, "file", undefined, probe);
|
||||
writeFileSync(transaction.stagingPath, "fake-mp4-bytes-that-are-non-empty");
|
||||
|
||||
await expect(
|
||||
transaction.validate({
|
||||
expectedDurationSeconds: 10.0,
|
||||
fps: 30,
|
||||
expectedFrames: 300,
|
||||
}),
|
||||
).rejects.toThrow(/duration probe failed/);
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
|
||||
it("rejects when the probe returns no usable duration rather than passing the gate", async () => {
|
||||
// A probe returning 0 or NaN cannot be compared against an expected
|
||||
// duration — it has to fail loud. Without this, a probe that hits a
|
||||
// misconfigured codec and reports duration 0 would let the gate accept
|
||||
// the artifact, the very behavior #3395 is fixing.
|
||||
const dir = tempDir();
|
||||
const destination = join(dir, "render.mp4");
|
||||
const probe: ArtifactDurationProbe = async () => ({ durationSeconds: 0 });
|
||||
const transaction = new ArtifactTransaction(destination, "file", undefined, probe);
|
||||
writeFileSync(transaction.stagingPath, "fake-mp4-bytes-that-are-non-empty");
|
||||
|
||||
await expect(transaction.validate({ expectedDurationSeconds: 10.0, fps: 30 })).rejects.toThrow(
|
||||
/no usable duration/,
|
||||
);
|
||||
|
||||
transaction.rollback();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -10,6 +10,7 @@ import {
|
||||
rmSync,
|
||||
} from "node:fs";
|
||||
import { basename, dirname, extname, join, resolve } from "node:path";
|
||||
import { extractMediaMetadata } from "../../utils/ffprobe.js";
|
||||
|
||||
export type ArtifactKind = "file" | "directory";
|
||||
|
||||
@@ -25,6 +26,71 @@ const defaultFileSystem: ArtifactTransactionFileSystem = {
|
||||
rmSync,
|
||||
};
|
||||
|
||||
/**
|
||||
* Result returned by a duration probe over a single staged artifact.
|
||||
*
|
||||
* `durationSeconds` MUST be the probed value as a finite non-negative number.
|
||||
* `frames` is optional: when present it is compared against
|
||||
* `expectedFrames` to catch container-vs-stream duration drift (the multi-
|
||||
* worker encode case in #3395 reports a matching container duration but a
|
||||
* shorter frame count, so duration alone is not enough). Probe errors
|
||||
* throw — a probe that returns nothing the caller can compare is a probe
|
||||
* the validation gate cannot stand on.
|
||||
*/
|
||||
export interface ArtifactDurationProbeResult {
|
||||
durationSeconds: number;
|
||||
frames?: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Pluggable probe used by `ArtifactTransaction.validate()` when the caller
|
||||
* passes `expectedDurationSeconds`. Default uses the producer's ffprobe
|
||||
* wrapper; tests inject a deterministic stub.
|
||||
*
|
||||
* A probe that cannot determine duration MUST throw rather than return a
|
||||
* zero — the caller's whole reason for asking is to detect a truncated
|
||||
* artifact, and a silent "0" is structurally indistinguishable from a
|
||||
* zero-duration container.
|
||||
*/
|
||||
export type ArtifactDurationProbe = (path: string) => Promise<ArtifactDurationProbeResult>;
|
||||
|
||||
async function defaultArtifactDurationProbe(path: string): Promise<ArtifactDurationProbeResult> {
|
||||
const meta = await extractMediaMetadata(path);
|
||||
// Forward the probed frame count when ffprobe reported one. The frame
|
||||
// count check (#3395) catches the multi-worker encode mode where the
|
||||
// container duration is reported correctly but the decoded stream is
|
||||
// shorter; without forwarding frames here, the caller's `expectedFrames`
|
||||
// is silently dropped (the assertion short-circuits on `undefined`). A
|
||||
// probe that cannot determine frames returns `undefined` — the caller
|
||||
// treats that as "no answer" and does not throw.
|
||||
return {
|
||||
durationSeconds: meta.durationSeconds,
|
||||
frames: meta.frames,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Caller-supplied expectation for file-artifact validation. The transaction
|
||||
* still does the readable-non-empty check; this layer adds a duration /
|
||||
* frame-count comparison against the values the pipeline already held.
|
||||
*
|
||||
* `expectedDurationSeconds` is required for the probe comparison. `fps` and
|
||||
* `expectedFrames` are optional; when both are present, frame-count is
|
||||
* checked too, which catches the multi-worker encode failure mode where
|
||||
* container duration is reported correctly but the decoded stream is shorter.
|
||||
*
|
||||
* `toleranceSeconds` defaults to a single frame at `fps` (or 20 ms when fps
|
||||
* is unknown) so that a normal container-level last-frame rounding does not
|
||||
* trip the gate. Multi-frame drops (e.g. the 326-frame / 11s truncation in
|
||||
* #3395) still fail.
|
||||
*/
|
||||
export interface ArtifactValidationExpectation {
|
||||
expectedDurationSeconds: number;
|
||||
fps?: number;
|
||||
expectedFrames?: number;
|
||||
toleranceSeconds?: number;
|
||||
}
|
||||
|
||||
function createSiblingTransactionDirectory(destination: string): string {
|
||||
const parent = dirname(destination);
|
||||
const extension = extname(destination);
|
||||
@@ -64,6 +130,68 @@ function collectDirectoryFiles(root: string): string[] {
|
||||
return files;
|
||||
}
|
||||
|
||||
/**
|
||||
* Default tolerance for the duration check: one frame at the job fps, or
|
||||
* 20 ms when fps is unknown. A normal container-level last-frame rounding
|
||||
* sits inside this window; multi-frame truncations (#3395: 326 frames /
|
||||
* 11 s) fall outside it.
|
||||
*/
|
||||
function durationToleranceSeconds(expected: ArtifactValidationExpectation): number {
|
||||
if (expected.toleranceSeconds !== undefined) return expected.toleranceSeconds;
|
||||
const fps = expected.fps;
|
||||
return fps && fps > 0 ? 1 / fps : 0.02;
|
||||
}
|
||||
|
||||
function assertUsableProbedDuration(
|
||||
probedSeconds: number,
|
||||
expectedSeconds: number,
|
||||
stagingPath: string,
|
||||
): void {
|
||||
// A probe returning 0 or NaN cannot be compared against an expected
|
||||
// duration — it has to fail loud. Silently passing would regress the
|
||||
// truncate-then-succeed failure mode #3395 reported.
|
||||
if (!Number.isFinite(probedSeconds) || probedSeconds <= 0) {
|
||||
throw new Error(
|
||||
`Render artifact duration probe returned no usable duration for ${stagingPath} ` +
|
||||
`(got ${String(probedSeconds)}); expected ${expectedSeconds.toFixed(3)}s. ` +
|
||||
`Refusing to publish an artifact whose duration cannot be verified.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
function assertDurationWithinTolerance(
|
||||
expectedSeconds: number,
|
||||
probedSeconds: number,
|
||||
toleranceSeconds: number,
|
||||
stagingPath: string,
|
||||
): void {
|
||||
const deficit = expectedSeconds - probedSeconds;
|
||||
if (deficit <= toleranceSeconds) return;
|
||||
throw new Error(
|
||||
`Render artifact is truncated: expected ${expectedSeconds.toFixed(3)}s, ` +
|
||||
`probed ${probedSeconds.toFixed(3)}s ` +
|
||||
`(deficit ${deficit.toFixed(3)}s exceeds tolerance ${toleranceSeconds.toFixed(3)}s). ` +
|
||||
`Artifact: ${stagingPath}`,
|
||||
);
|
||||
}
|
||||
|
||||
function assertFrameCountWithinTolerance(
|
||||
expectedFrames: number,
|
||||
probedFrames: number | undefined,
|
||||
stagingPath: string,
|
||||
): void {
|
||||
if (probedFrames === undefined || !Number.isFinite(probedFrames) || probedFrames <= 0) {
|
||||
return;
|
||||
}
|
||||
const shortfall = expectedFrames - probedFrames;
|
||||
if (shortfall <= 1) return;
|
||||
throw new Error(
|
||||
`Render artifact is truncated: expected ${expectedFrames} frames, ` +
|
||||
`probed ${probedFrames} frames ` +
|
||||
`(shortfall ${shortfall} frames). Artifact: ${stagingPath}`,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Stages a render beside its final destination and promotes only a validated
|
||||
* artifact. File promotion uses one atomic replacement rename, so an existing
|
||||
@@ -71,6 +199,12 @@ function collectDirectoryFiles(root: string): string[] {
|
||||
* non-empty directory cannot be expressed as one portable rename; that case
|
||||
* uses a recoverable backup handoff while preserving the previous contents on
|
||||
* ordinary failures.
|
||||
*
|
||||
* When the caller passes an `expected` expectation to `validate()`, the
|
||||
* transaction additionally probes the staged file's container duration (and
|
||||
* decoded frame count when available) and rejects any artifact that is
|
||||
* significantly shorter than what the pipeline asked for. The readable-non-
|
||||
* empty check is unchanged; this is a second gate on top.
|
||||
*/
|
||||
export class ArtifactTransaction {
|
||||
readonly destinationPath: string;
|
||||
@@ -78,21 +212,25 @@ export class ArtifactTransaction {
|
||||
private readonly transactionDirectory: string;
|
||||
private readonly backupPath: string;
|
||||
private state: "active" | "committed" | "rolled-back" = "active";
|
||||
private readonly durationProbe: ArtifactDurationProbe;
|
||||
|
||||
constructor(
|
||||
destinationPath: string,
|
||||
private readonly kind: ArtifactKind,
|
||||
private readonly fileSystem: ArtifactTransactionFileSystem = defaultFileSystem,
|
||||
durationProbe: ArtifactDurationProbe = defaultArtifactDurationProbe,
|
||||
) {
|
||||
this.destinationPath = resolve(destinationPath);
|
||||
this.transactionDirectory = createSiblingTransactionDirectory(this.destinationPath);
|
||||
this.stagingPath = join(this.transactionDirectory, basename(this.destinationPath));
|
||||
this.backupPath = join(this.transactionDirectory, "backup");
|
||||
this.durationProbe = durationProbe;
|
||||
}
|
||||
|
||||
validate(): void {
|
||||
async validate(expected?: ArtifactValidationExpectation): Promise<void> {
|
||||
if (this.kind === "file") {
|
||||
assertReadableNonEmptyFile(this.stagingPath);
|
||||
if (expected) await this.assertArtifactDuration(expected);
|
||||
return;
|
||||
}
|
||||
let files: string[];
|
||||
@@ -109,11 +247,47 @@ export class ArtifactTransaction {
|
||||
for (const file of files) assertReadableNonEmptyFile(file);
|
||||
}
|
||||
|
||||
commit(): void {
|
||||
private async assertArtifactDuration(expected: ArtifactValidationExpectation): Promise<void> {
|
||||
const expectedSeconds = expected.expectedDurationSeconds;
|
||||
if (!Number.isFinite(expectedSeconds) || expectedSeconds <= 0) return;
|
||||
|
||||
const probed = await this.runDurationProbe();
|
||||
assertUsableProbedDuration(probed.durationSeconds, expectedSeconds, this.stagingPath);
|
||||
|
||||
const toleranceSeconds = durationToleranceSeconds(expected);
|
||||
assertDurationWithinTolerance(
|
||||
expectedSeconds,
|
||||
probed.durationSeconds,
|
||||
toleranceSeconds,
|
||||
this.stagingPath,
|
||||
);
|
||||
|
||||
if (expected.expectedFrames !== undefined) {
|
||||
assertFrameCountWithinTolerance(expected.expectedFrames, probed.frames, this.stagingPath);
|
||||
}
|
||||
}
|
||||
|
||||
private async runDurationProbe(): Promise<ArtifactDurationProbeResult> {
|
||||
try {
|
||||
return await this.durationProbe(this.stagingPath);
|
||||
} catch (error) {
|
||||
// Probe failure means we cannot assert; do not silently pass a gate the
|
||||
// caller asked for. Surfacing the probe error keeps the truncate-then-
|
||||
// succeed failure mode from regressing back into "validation passes".
|
||||
throw new Error(
|
||||
`Render artifact duration probe failed for ${this.stagingPath}: ${
|
||||
error instanceof Error ? error.message : String(error)
|
||||
}`,
|
||||
{ cause: error },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async commit(): Promise<void> {
|
||||
if (this.state !== "active") {
|
||||
throw new Error(`Cannot commit an artifact transaction in state ${this.state}`);
|
||||
}
|
||||
this.validate();
|
||||
await this.validate();
|
||||
const hadDestination = this.fileSystem.existsSync(this.destinationPath);
|
||||
|
||||
// Both files and new directories publish with one rename. In particular,
|
||||
|
||||
@@ -3943,7 +3943,15 @@ async function executeRenderPipeline(input: {
|
||||
observability.checkpoint("assemble", `skipped for ${outputFormat}`);
|
||||
}
|
||||
|
||||
artifactTransaction.validate();
|
||||
await artifactTransaction.validate(
|
||||
!isPngSequence && !isGif && Number.isFinite(job.duration) && job.duration > 0
|
||||
? {
|
||||
expectedDurationSeconds: job.duration,
|
||||
fps: fpsToNumber(job.config.fps),
|
||||
expectedFrames: captureTotalFrames,
|
||||
}
|
||||
: undefined,
|
||||
);
|
||||
|
||||
const totalElapsed = Date.now() - pipelineStart;
|
||||
|
||||
@@ -4026,7 +4034,7 @@ async function executeRenderPipeline(input: {
|
||||
}
|
||||
}
|
||||
|
||||
artifactTransaction.commit();
|
||||
await artifactTransaction.commit();
|
||||
job.outputPath = outputPath;
|
||||
updateJobStatus(job, "complete", "Render complete", 100, onProgress);
|
||||
await eventPublisher.flush();
|
||||
|
||||
Reference in New Issue
Block a user