fix(media-use): clean failed asset reservations (#2627)

* fix(media-use): clean failed asset reservations

* test(media-use): pin installer guidance exactly

skills-manifest.json

@@ -46,7 +46,7 @@
       "files": 10
     },
     "media-use": {
-      "hash": "f15fce08a493d70e",
+      "hash": "5399c0f111ff5619",
       "files": 139
     },
     "motion-graphics": {

skills/media-use/scripts/lib/heygen-cli.mjs

@@ -6,14 +6,16 @@ import { track } from "./telemetry.mjs";
 // needs — so anything below this can't authenticate for free usage at all.
 export const HEYGEN_MIN_VERSION = "0.3.0";
 // Free-usage path is OAuth (`--oauth` → subscription/free credits); `--api-key`
-// bills API credits, so the onboarding steers to OAuth.
+// bills API credits, so the onboarding steers to OAuth. Keep pipe-to-shell
+// installer text out of the runtime module; the docs are the safer source of
+// truth for platform-specific setup.
 export const HEYGEN_INSTALL_COMMAND =
-  "curl -fsSL https://static.heygen.ai/cli/install.sh | bash && heygen auth login --oauth";
+  "Install the CLI from https://developers.heygen.com/cli, then run: heygen auth login --oauth";
 export const HEYGEN_AUTH_COMMAND = "heygen auth login --oauth";
 export const HEYGEN_UPDATE_COMMAND = "heygen update";
 export const HEYGEN_CLIENT_SOURCE_ARGV = ["--headers", "X-HeyGen-Client-Source: media-use"];
 
-export const HEYGEN_NOT_FOUND_MESSAGE = `media-use: heygen CLI not found — it's the free path for bgm/image/voice/avatar-video. Install: ${HEYGEN_INSTALL_COMMAND}`;
+export const HEYGEN_NOT_FOUND_MESSAGE = `media-use: heygen CLI not found — it's the free path for bgm/image/voice/avatar-video. ${HEYGEN_INSTALL_COMMAND}`;
 export const HEYGEN_NOT_AUTHENTICATED_MESSAGE = `media-use: heygen CLI not authenticated (free usage) — run: ${HEYGEN_AUTH_COMMAND}`;
 export const HEYGEN_OUTDATED_MESSAGE = `media-use: heygen CLI is outdated — run: ${HEYGEN_UPDATE_COMMAND}  (need >= v${HEYGEN_MIN_VERSION})`;
 

skills/media-use/scripts/lib/lut-preset-provider.mjs

@@ -1,6 +1,6 @@
 import { existsSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs";
 import { join } from "node:path";
-import { allocateId } from "./manifest.mjs";
+import { withReservedFile, withReservedFileSync } from "./manifest.mjs";
 import { freezeUrl } from "./freeze.mjs";
 import { tokenOverlap } from "./match.mjs";
 import { buildCube } from "./cube-build.mjs";
@@ -183,19 +183,27 @@ export async function freezeLibraryLut(match, { projectDir, type, localOnly = fa
   // to deterministic buildCube params when offline (--local-only) or if the
   // download/validation fails, so resolution is never blocked on the network.
   if (match.url && !localOnly) {
-    const { id, localPath } = allocateId(projectDir, type, ".cube");
-    const fullPath = join(projectDir, localPath);
-    const tmpPath = `${fullPath}.tmp`;
     try {
-      // Download + validate at a .tmp path, then atomically rename. A crash
-      // (SIGKILL/OOM) between write and validate can't orphan an invalid .cube
-      // at the final path — only a validated cube is ever renamed into place.
-      await freezeUrl(match.url, tmpPath);
-      assertValidCubeFile(tmpPath, `downloaded library LUT ${match.id} failed validation`);
-      renameSync(tmpPath, fullPath);
-      return libraryRecord(match, { id, localPath, fullPath, via: "url" });
+      return await withReservedFile(
+        projectDir,
+        type,
+        ".cube",
+        async ({ id, localPath, fullPath }) => {
+          const tmpPath = `${fullPath}.tmp`;
+          try {
+            // Download + validate at a .tmp path, then atomically rename. A crash
+            // (SIGKILL/OOM) between write and validate can't orphan an invalid .cube
+            // at the final path — only a validated cube is ever renamed into place.
+            await freezeUrl(match.url, tmpPath);
+            assertValidCubeFile(tmpPath, `downloaded library LUT ${match.id} failed validation`);
+            renameSync(tmpPath, fullPath);
+            return libraryRecord(match, { id, localPath, fullPath, via: "url" });
+          } finally {
+            rmSync(tmpPath, { force: true });
+          }
+        },
+      );
     } catch (err) {
-      rmSync(tmpPath, { force: true });
       if (!match.params) {
         throw new Error(`failed to freeze library LUT ${match.id}: ${err.message}`);
       }
@@ -204,26 +212,25 @@ export async function freezeLibraryLut(match, { projectDir, type, localOnly = fa
   }
 
   if (match.params) {
-    const { id, localPath } = allocateId(projectDir, type, ".cube");
-    const fullPath = join(projectDir, localPath);
-    const tmpPath = `${fullPath}.tmp`;
-    try {
-      const cube = buildCube(match.params);
-      assertValidCubeText(cube, `invalid library LUT ${match.id}`);
-      // Write + validate at .tmp, then atomic rename — same no-orphan guarantee
-      // as the url path above.
-      writeFileSync(tmpPath, cube);
-      assertValidCubeFile(tmpPath, `invalid frozen LUT ${localPath}`);
-      renameSync(tmpPath, fullPath);
-    } catch (err) {
-      rmSync(tmpPath, { force: true });
-      throw err;
-    }
-    return libraryRecord(match, {
-      id,
-      localPath,
-      fullPath,
-      via: match.url ? "params-fallback" : "params",
+    return withReservedFileSync(projectDir, type, ".cube", ({ id, localPath, fullPath }) => {
+      const tmpPath = `${fullPath}.tmp`;
+      try {
+        const cube = buildCube(match.params);
+        assertValidCubeText(cube, `invalid library LUT ${match.id}`);
+        // Write + validate at .tmp, then atomic rename — same no-orphan guarantee
+        // as the url path above.
+        writeFileSync(tmpPath, cube);
+        assertValidCubeFile(tmpPath, `invalid frozen LUT ${localPath}`);
+        renameSync(tmpPath, fullPath);
+        return libraryRecord(match, {
+          id,
+          localPath,
+          fullPath,
+          via: match.url ? "params-fallback" : "params",
+        });
+      } finally {
+        rmSync(tmpPath, { force: true });
+      }
     });
   }
 

skills/media-use/scripts/lib/lut-preset-provider.test.mjs

@@ -1,5 +1,5 @@
 import { strict as assert } from "node:assert";
-import { mkdtempSync, rmSync, existsSync, readFileSync } from "node:fs";
+import { mkdtempSync, rmSync, existsSync, readFileSync, readdirSync } from "node:fs";
 import { join } from "node:path";
 import { tmpdir } from "node:os";
 import { test } from "node:test";
@@ -128,3 +128,33 @@ test("url library entries respect localOnly and freeze through fetch", async ()
     rmSync(projectDir, { recursive: true, force: true });
   }
 });
+
+test("failed URL library freeze releases its reservation", async () => {
+  const projectDir = mkdtempSync(join(tmpdir(), "mu-lut-failure-"));
+  const originalFetch = globalThis.fetch;
+  try {
+    globalThis.fetch = async () => ({
+      ok: true,
+      headers: { get: () => "12" },
+      body: [Buffer.from("not a cube\n")],
+    });
+    await assert.rejects(
+      freezeLibraryLut(
+        {
+          kind: "library",
+          id: "broken-cdn-look",
+          description: "Broken CDN look",
+          tags: ["broken"],
+          intensity: 1,
+          url: "https://example.com/broken.cube",
+        },
+        { projectDir, type: "lut" },
+      ),
+      /failed to freeze library LUT/,
+    );
+    assert.deepStrictEqual(readdirSync(join(projectDir, ".media/luts")), []);
+  } finally {
+    globalThis.fetch = originalFetch;
+    rmSync(projectDir, { recursive: true, force: true });
+  }
+});

skills/media-use/scripts/lib/manifest.mjs

@@ -190,3 +190,40 @@ export function allocateId(projectDir, type, ext) {
     return { id, localPath };
   });
 }
+
+function reservedFile(projectDir, type, ext) {
+  const allocation = allocateId(projectDir, type, ext);
+  return { ...allocation, fullPath: join(projectDir, allocation.localPath) };
+}
+
+function rollbackReservation(reservation) {
+  rmSync(reservation.fullPath, { force: true });
+}
+
+// A reservation is committed only when populate returns a non-null value.
+// Throwing/rejecting or returning null means no usable asset was produced, so
+// the placeholder must be released. Keeping this transaction beside allocateId
+// prevents individual provider/cache/LUT paths from forgetting the rollback.
+export function withReservedFileSync(projectDir, type, ext, populate) {
+  const reservation = reservedFile(projectDir, type, ext);
+  try {
+    const result = populate(reservation);
+    if (result == null) rollbackReservation(reservation);
+    return result;
+  } catch (error) {
+    rollbackReservation(reservation);
+    throw error;
+  }
+}
+
+export async function withReservedFile(projectDir, type, ext, populate) {
+  const reservation = reservedFile(projectDir, type, ext);
+  try {
+    const result = await populate(reservation);
+    if (result == null) rollbackReservation(reservation);
+    return result;
+  } catch (error) {
+    rollbackReservation(reservation);
+    throw error;
+  }
+}

skills/media-use/scripts/lib/manifest.test.mjs

@@ -1,5 +1,13 @@
 import { strict as assert } from "node:assert";
-import { mkdtempSync, rmSync, readFileSync, writeFileSync, mkdirSync, existsSync } from "node:fs";
+import {
+  mkdtempSync,
+  rmSync,
+  readFileSync,
+  writeFileSync,
+  mkdirSync,
+  existsSync,
+  readdirSync,
+} from "node:fs";
 import { join } from "node:path";
 import { tmpdir } from "node:os";
 import {
@@ -9,6 +17,7 @@ import {
   findByEntity,
   nextId,
   allocateId,
+  withReservedFileSync,
   normalizePrompt,
   manifestPath,
   mediaDir,
@@ -164,6 +173,38 @@ function runTests() {
     cleanup();
   });
 
+  test("failed reservation rollback preserves another completed reservation", () => {
+    setup();
+    const kept = withReservedFileSync(tmp, "bgm", ".wav", (reservation) => {
+      writeFileSync(reservation.fullPath, "completed asset");
+      return reservation;
+    });
+
+    assert.throws(
+      () =>
+        withReservedFileSync(tmp, "bgm", ".wav", () => {
+          throw new Error("populate failed");
+        }),
+      /populate failed/,
+    );
+
+    assert.equal(kept.id, "bgm_001");
+    assert.equal(readFileSync(kept.fullPath, "utf8"), "completed asset");
+    assert.deepStrictEqual(readdirSync(typeDirPath(tmp, "bgm")), ["bgm_001.wav"]);
+    assert.equal(allocateId(tmp, "bgm", ".wav").id, "bgm_002");
+    cleanup();
+  });
+
+  test("empty reservation result releases the placeholder", () => {
+    setup();
+    assert.equal(
+      withReservedFileSync(tmp, "image", ".jpg", () => null),
+      null,
+    );
+    assert.deepStrictEqual(readdirSync(typeDirPath(tmp, "image")), []);
+    cleanup();
+  });
+
   test("findByEntity matches case-insensitively", () => {
     setup();
     appendRecord(tmp, makeRecord({ entity: "GitHub", type: "icon" }));

skills/media-use/scripts/resolve.mjs

@@ -4,7 +4,14 @@ import { spawnSync } from "node:child_process";
 import { existsSync, statSync, writeFileSync, renameSync, rmSync } from "node:fs";
 import { resolve, join, extname, basename } from "node:path";
 import { parseArgs } from "node:util";
-import { appendRecord, findByPrompt, findByEntity, nextId, allocateId } from "./lib/manifest.mjs";
+import {
+  appendRecord,
+  findByPrompt,
+  findByEntity,
+  nextId,
+  withReservedFile,
+  withReservedFileSync,
+} from "./lib/manifest.mjs";
 import { regenerateIndex } from "./lib/index-gen.mjs";
 import { cacheGet, cacheGetByEntity, importFromCache, cachePut } from "./lib/cache.mjs";
 import { runCapability, listTypes, providerMatches, providerNamesFor } from "./lib/registry.mjs";
@@ -326,10 +333,8 @@ async function run() {
   const cacheHit = forced ? null : cacheGet(intent, type);
   if (cacheHit) {
     const ext = extname(cacheHit.cached_path);
-    const { id, localPath } = allocateId(projectDir, type, ext);
-    const imported = localizeImportedRecord(
-      importFromCache(cacheHit, projectDir, id, localPath),
-      localPath,
+    const imported = withReservedFileSync(projectDir, type, ext, ({ id, localPath }) =>
+      localizeImportedRecord(importFromCache(cacheHit, projectDir, id, localPath), localPath),
     );
     if (imported) {
       appendRecord(projectDir, imported);
@@ -342,10 +347,11 @@ async function run() {
     const entityCacheHit = cacheGetByEntity(entity);
     if (entityCacheHit && typesMatch(entityCacheHit.type, type)) {
       const ext = extname(entityCacheHit.cached_path);
-      const { id, localPath } = allocateId(projectDir, type, ext);
-      const imported = localizeImportedRecord(
-        importFromCache(entityCacheHit, projectDir, id, localPath),
-        localPath,
+      const imported = withReservedFileSync(projectDir, type, ext, ({ id, localPath }) =>
+        localizeImportedRecord(
+          importFromCache(entityCacheHit, projectDir, id, localPath),
+          localPath,
+        ),
       );
       if (imported) {
         appendRecord(projectDir, imported);
@@ -456,17 +462,21 @@ async function run() {
   // 5. freeze + register (atomic id+file reservation so concurrent resolves
   // can't collide on an id during the download — MU-23)
   const ext = searchResult.ext || extFromUrl(searchResult.url || "") || defaultExt(type);
-  const { id, localPath } = allocateId(projectDir, type, ext);
-  const fullPath = join(projectDir, localPath);
-
-  if (searchResult.localPath) {
-    freezeLocalFile(searchResult.localPath, fullPath);
-  } else if (searchResult.url) {
-    await freezeUrl(searchResult.url, fullPath);
-  } else {
-    console.error("error: provider returned no url or localPath");
-    process.exit(1);
-  }
+  const { id, localPath, fullPath } = await withReservedFile(
+    projectDir,
+    type,
+    ext,
+    async (reservation) => {
+      if (searchResult.localPath) {
+        freezeLocalFile(searchResult.localPath, reservation.fullPath);
+      } else if (searchResult.url) {
+        await freezeUrl(searchResult.url, reservation.fullPath);
+      } else {
+        throw new Error("provider returned no url or localPath");
+      }
+      return reservation;
+    },
+  );
 
   const record = {
     id,
@@ -545,32 +555,32 @@ function freezeGeneratedLut(
     validationErrorPrefix = "generated LUT failed validation",
   },
 ) {
-  const { id, localPath } = allocateId(projectDir, type, ".cube");
-  const fullPath = join(projectDir, localPath);
-  const tmpPath = `${fullPath}.tmp`;
-  try {
-    // Write + validate at .tmp, then atomic rename, so a crash between write and
-    // validate can't leave an invalid .cube at the final path.
-    writeFileSync(tmpPath, buildCube(params));
-    const check = validateCubeFile(tmpPath);
-    if (!check.ok) throw new Error(check.error);
-    renameSync(tmpPath, fullPath);
-  } catch (err) {
-    rmSync(tmpPath, { force: true });
-    throw new Error(`${validationErrorPrefix}: ${err.message}`);
-  }
-  return {
-    id,
-    localPath,
-    fullPath,
-    lut: { src: localPath, intensity: 1 },
-    source: "generated",
-    description,
-    metadata: {
-      provider: "cube_lut.builder",
-      provenance: { params },
-    },
-  };
+  return withReservedFileSync(projectDir, type, ".cube", ({ id, localPath, fullPath }) => {
+    const tmpPath = `${fullPath}.tmp`;
+    try {
+      // Write + validate at .tmp, then atomic rename, so a crash between write and
+      // validate can't leave an invalid .cube at the final path.
+      writeFileSync(tmpPath, buildCube(params));
+      const check = validateCubeFile(tmpPath);
+      if (!check.ok) throw new Error(check.error);
+      renameSync(tmpPath, fullPath);
+    } catch (err) {
+      rmSync(tmpPath, { force: true });
+      throw new Error(`${validationErrorPrefix}: ${err.message}`);
+    }
+    return {
+      id,
+      localPath,
+      fullPath,
+      lut: { src: localPath, intensity: 1 },
+      source: "generated",
+      description,
+      metadata: {
+        provider: "cube_lut.builder",
+        provenance: { params },
+      },
+    };
+  });
 }
 
 function exitError(message, status = 1) {
@@ -818,10 +828,16 @@ async function ingest(src) {
     process.exit(2);
   }
   const ext = extname(isUrl ? new URL(src).pathname : src) || defaultExt(type);
-  const { id, localPath } = allocateId(projectDir, type, ext);
-  const fullPath = join(projectDir, localPath);
-  if (isUrl) await freezeUrl(src, fullPath);
-  else freezeLocalFile(resolve(src), fullPath);
+  const { id, localPath, fullPath } = await withReservedFile(
+    projectDir,
+    type,
+    ext,
+    async (reservation) => {
+      if (isUrl) await freezeUrl(src, reservation.fullPath);
+      else freezeLocalFile(resolve(src), reservation.fullPath);
+      return reservation;
+    },
+  );
   if (type === "lut" || type === "grade") {
     try {
       const check = validateCubeFile(fullPath);
@@ -1135,10 +1151,8 @@ async function reuseGlobal(shaArg) {
     process.exit(2);
   }
   const ext = extname(rec.cached_path || "") || defaultExt(type);
-  const { id, localPath } = allocateId(projectDir, type, ext);
-  const imported = localizeImportedRecord(
-    importFromCache(rec, projectDir, id, localPath),
-    localPath,
+  const imported = withReservedFileSync(projectDir, type, ext, ({ id, localPath }) =>
+    localizeImportedRecord(importFromCache(rec, projectDir, id, localPath), localPath),
   );
   if (!imported) {
     console.error(`error: cache entry for "${shaArg}" is incomplete or missing on disk`);

skills/media-use/scripts/resolve.test.mjs

@@ -12,10 +12,11 @@ import {
 import { join } from "node:path";
 import { tmpdir } from "node:os";
 import { createServer } from "node:http";
-import { execFileSync, spawnSync } from "node:child_process";
+import { execFileSync, spawn, spawnSync } from "node:child_process";
 import { appendRecord, readManifest } from "./lib/manifest.mjs";
 import { regenerateIndex } from "./lib/index-gen.mjs";
 import { getProvider } from "./lib/providers.mjs";
+import { HEYGEN_NOT_FOUND_MESSAGE } from "./lib/heygen-cli.mjs";
 import { freezeLocalFile } from "./lib/freeze.mjs";
 import { cachePut, cacheGet, importFromCache } from "./lib/cache.mjs";
 import { validateCubeFile } from "./lib/cube-validate.mjs";
@@ -78,6 +79,26 @@ function spawnResolve(args, opts = {}) {
   });
 }
 
+function spawnResolveAsync(args, opts = {}) {
+  const { env, ...rest } = opts;
+  return new Promise((resolve, reject) => {
+    const child = spawn(process.execPath, [RESOLVE_CLI, ...args], {
+      cwd: REPO_ROOT,
+      env: { ...process.env, DO_NOT_TRACK: "1", ...env },
+      stdio: ["ignore", "pipe", "pipe"],
+      ...rest,
+    });
+    let stdout = "";
+    let stderr = "";
+    child.stdout.setEncoding("utf8");
+    child.stderr.setEncoding("utf8");
+    child.stdout.on("data", (chunk) => (stdout += chunk));
+    child.stderr.on("data", (chunk) => (stderr += chunk));
+    child.once("error", reject);
+    child.once("close", (status, signal) => resolve({ status, signal, stdout, stderr }));
+  });
+}
+
 function makeFrame(dir, name, color) {
   const out = join(dir, name);
   execFileSync(
@@ -140,7 +161,8 @@ test("bundled SFX resolve without HeyGen on PATH", () => {
   const parsed = JSON.parse(result.stdout);
   assert.equal(parsed.ok, true);
   assert.equal(parsed.provenance.provider, "bundled.sfx");
-  assert.match(parsed.advisory?.message ?? "", /Install: curl -fsSL/);
+  assert.equal(parsed.advisory?.message, HEYGEN_NOT_FOUND_MESSAGE);
+  assert.equal(parsed.advisory.message.includes("| bash"), false);
   assert.ok(existsSync(join(tmp, parsed.path)));
   cleanup();
 });
@@ -224,7 +246,10 @@ test("human bundled fallback prints the install hint once", () => {
     env: { HOME: tmp, PATH: tmp },
   });
   assert.equal(result.status, 0, result.stderr);
-  assert.equal(result.stderr.match(/Install: curl -fsSL/g)?.length, 1);
+  assert.equal(
+    result.stderr.match(/Install the CLI from https:\/\/developers\.heygen\.com\/cli/g)?.length,
+    1,
+  );
   assert.match(result.stdout, /resolved sfx_001/);
   cleanup();
 });
@@ -409,6 +434,61 @@ test("freezeLocalFile creates parent dirs and copies", () => {
   cleanup();
 });
 
+test("failed remote freeze removes its reserved placeholder", async () => {
+  setup();
+  const server = createServer((_req, res) => {
+    res.writeHead(503);
+    res.end("unavailable");
+  });
+  await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve));
+  const port = server.address().port;
+  const binDir = writeFakeHeygen(
+    `printf '%s\\n' '{"data":[{"id":"asset.jpg","url":"http://127.0.0.1:${port}/asset.jpg"}]}'`,
+  );
+
+  try {
+    const result = await spawnResolveAsync(
+      [
+        "--type",
+        "image",
+        "--intent",
+        "download failure",
+        "--provider",
+        "heygen",
+        "--project",
+        tmp,
+        "--json",
+      ],
+      { env: { HOME: tmp, PATH: binDir } },
+    );
+
+    assert.equal(result.status, 1, result.stderr);
+    assert.deepStrictEqual(readdirSync(join(tmp, ".media/images")), []);
+    assert.deepStrictEqual(readManifest(tmp), []);
+  } finally {
+    await new Promise((resolve) => server.close(resolve));
+    cleanup();
+  }
+});
+
+test("failed URL ingest removes its reserved placeholder", () => {
+  setup();
+  const result = spawnResolve([
+    "--from",
+    "https://example.invalid/unavailable.jpg",
+    "--type",
+    "image",
+    "--project",
+    tmp,
+    "--json",
+  ]);
+
+  assert.equal(result.status, 1, result.stderr);
+  assert.deepStrictEqual(readdirSync(join(tmp, ".media/images")), []);
+  assert.deepStrictEqual(readManifest(tmp), []);
+  cleanup();
+});
+
 // --- adopt existing assets ---
 
 test("--adopt registers existing assets/ files", () => {