fix: delete unpublished skill blobs when publish fails

convex/httpApiV1.handlers.test.ts

@@ -8474,6 +8474,46 @@ describe("httpApiV1 handlers", () => {
     expect(publishVersionForUser).not.toHaveBeenCalled();
   });
 
+  it("publish multipart deletes stored blobs when owner resolution fails", async () => {
+    vi.mocked(requireApiTokenUser).mockResolvedValueOnce({
+      userId: "users:1",
+      user: { handle: "p" },
+    } as never);
+    const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
+      if (isRateLimitArgs(args)) return okRate();
+      throw new Error("Publisher not found");
+    });
+    const form = new FormData();
+    form.set(
+      "payload",
+      JSON.stringify({
+        slug: "demo",
+        displayName: "Demo",
+        ownerHandle: "@missing",
+        version: "1.0.0",
+        changelog: "",
+        acceptLicenseTerms: true,
+        tags: ["latest"],
+      }),
+    );
+    form.append("files", new Blob(["hello"], { type: "text/plain" }), "SKILL.md");
+    const store = vi.fn().mockResolvedValue("storage:1");
+    const remove = vi.fn().mockResolvedValue(undefined);
+    const response = await __handlers.publishSkillV1Handler(
+      makeCtx({ runMutation, storage: { store, delete: remove } }),
+      new Request("https://example.com/api/v1/skills", {
+        method: "POST",
+        headers: { Authorization: "Bearer clh_test" },
+        body: form,
+      }),
+    );
+    expect(response.status).toBe(400);
+    expect(await response.text()).toMatch(/publisher not found/i);
+    expect(publishVersionForUser).not.toHaveBeenCalled();
+    expect(store).toHaveBeenCalledTimes(1);
+    expect(remove).toHaveBeenCalledWith("storage:1");
+  });
+
   it("publish json rejects omitted license terms", async () => {
     vi.mocked(requireApiTokenUser).mockResolvedValueOnce({
       userId: "users:1",
@@ -8515,11 +8555,12 @@ describe("httpApiV1 handlers", () => {
       userId: "users:1",
       user: { handle: "p" },
     } as never);
-    vi.mocked(publishVersionForUser).mockResolvedValueOnce({
-      skillId: "s",
-      versionId: "v",
-      embeddingId: "e",
-    } as never);
+    vi.mocked(publishVersionForUser).mockImplementationOnce(
+      async (_ctx, _userId, _args, options) => {
+        options?.onFilesPersisted?.();
+        return { skillId: "s", versionId: "v", embeddingId: "e" } as never;
+      },
+    );
     const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
       if (isRateLimitArgs(args)) return okRate();
       if (args.ownerHandle === "me") return { publisherId: "publishers:me" };
@@ -8587,11 +8628,12 @@ describe("httpApiV1 handlers", () => {
       userId: "users:1",
       user: { handle: "p" },
     } as never);
-    vi.mocked(publishVersionForUser).mockResolvedValueOnce({
-      skillId: "s",
-      versionId: "v",
-      embeddingId: "e",
-    } as never);
+    vi.mocked(publishVersionForUser).mockImplementationOnce(
+      async (_ctx, _userId, _args, options) => {
+        options?.onFilesPersisted?.();
+        return { skillId: "s", versionId: "v", embeddingId: "e" } as never;
+      },
+    );
     const runMutation = vi.fn(async (_mutation: unknown, args: Record<string, unknown>) => {
       if (isRateLimitArgs(args)) return okRate();
       if (args.ownerHandle === "openclaw") return { publisherId: "publishers:openclaw" };
@@ -8625,10 +8667,54 @@ describe("httpApiV1 handlers", () => {
       expect.anything(),
       "users:1",
       expect.not.objectContaining({ ownerHandle: expect.anything() }),
-      { ownerPublisherId: "publishers:openclaw" },
+      { ownerPublisherId: "publishers:openclaw", onFilesPersisted: expect.any(Function) },
     );
   });
 
+  it.each([false, true])(
+    "multipart cleanup respects persisted files after failure (%s)",
+    async (persisted) => {
+      vi.mocked(requireApiTokenUser).mockResolvedValueOnce({
+        userId: "users:1",
+        user: { handle: "p" },
+      } as never);
+      vi.mocked(publishVersionForUser).mockImplementationOnce(
+        async (_ctx, _userId, _args, options) => {
+          if (persisted) options?.onFilesPersisted?.();
+          throw new Error(persisted ? "followup failed" : "insert rejected");
+        },
+      );
+      const remove = vi.fn();
+      const form = new FormData();
+      form.set(
+        "payload",
+        JSON.stringify({
+          slug: "demo",
+          displayName: "Demo",
+          version: "1.0.0",
+          acceptLicenseTerms: true,
+          tags: ["latest"],
+        }),
+      );
+      form.append("files", new Blob(["# Demo\n"]), "SKILL.md");
+      const response = await __handlers.publishSkillV1Handler(
+        makeCtx({
+          runMutation: vi.fn().mockResolvedValue(okRate()),
+          storage: { store: vi.fn().mockResolvedValue("storage:request"), delete: remove },
+        }),
+        new Request("https://example.com/api/v1/skills", {
+          method: "POST",
+          headers: { Authorization: "Bearer clh_test" },
+          body: form,
+        }),
+      );
+      expect(response.status).toBe(400);
+      expect(await response.text()).toContain(persisted ? "followup failed" : "insert rejected");
+      if (persisted) expect(remove).not.toHaveBeenCalled();
+      else expect(remove).toHaveBeenCalledExactlyOnceWith("storage:request");
+    },
+  );
+
   it("publish multipart rejects omitted license terms", async () => {
     vi.mocked(requireApiTokenUser).mockResolvedValueOnce({
       userId: "users:1",
@@ -8652,8 +8738,10 @@ describe("httpApiV1 handlers", () => {
       }),
     );
     form.append("files", new Blob(["hello"], { type: "text/plain" }), "SKILL.md");
+    const store = vi.fn().mockResolvedValue("storage:1");
+    const remove = vi.fn().mockResolvedValue(undefined);
     const response = await __handlers.publishSkillV1Handler(
-      makeCtx({ runMutation, storage: { store: vi.fn().mockResolvedValue("storage:1") } }),
+      makeCtx({ runMutation, storage: { store, delete: remove } }),
       new Request("https://example.com/api/v1/skills", {
         method: "POST",
         headers: { Authorization: "Bearer clh_test" },
@@ -8663,6 +8751,8 @@ describe("httpApiV1 handlers", () => {
     expect(response.status).toBe(400);
     expect(await response.text()).toMatch(/license terms must be accepted/i);
     expect(publishVersionForUser).not.toHaveBeenCalled();
+    expect(store).toHaveBeenCalledTimes(1);
+    expect(remove).toHaveBeenCalledWith("storage:1");
   });
 
   it("publish rejects explicit license refusal", async () => {
@@ -8709,11 +8799,12 @@ describe("httpApiV1 handlers", () => {
       userId: "users:1",
       user: { handle: "p" },
     } as never);
-    vi.mocked(publishVersionForUser).mockResolvedValueOnce({
-      skillId: "s",
-      versionId: "v",
-      embeddingId: "e",
-    } as never);
+    vi.mocked(publishVersionForUser).mockImplementationOnce(
+      async (_ctx, _userId, _args, options) => {
+        options?.onFilesPersisted?.();
+        return { skillId: "s", versionId: "v", embeddingId: "e" } as never;
+      },
+    );
     const runMutation = vi.fn().mockResolvedValue(okRate());
     const store = vi.fn().mockResolvedValue("storage:1");
     const form = new FormData();

convex/httpApiV1.shared.test.ts

@@ -5,10 +5,12 @@ import type { Id } from "./_generated/dataModel";
 import type { ActionCtx } from "./_generated/server";
 import {
   formatUserFacingErrorMessage,
+  parseMultipartPublish,
   parseMultipartSkillScan,
   resolveTagsBatch,
   softDeleteErrorToResponse,
 } from "./httpApiV1/shared";
+import { MAX_PUBLISH_FILE_BYTES } from "./lib/publishLimits";
 
 function makeCtx() {
   return {
@@ -161,4 +163,74 @@ describe("http API v1 shared helpers", () => {
     ).rejects.toThrow("update is not valid for uploaded scans");
     expect(store).not.toHaveBeenCalled();
   });
+
+  it("deletes stored publish blobs when the payload fails after upload", async () => {
+    const form = new FormData();
+    form.set(
+      "payload",
+      JSON.stringify({
+        displayName: "Demo",
+        version: "1.0.0",
+        changelog: "",
+        tags: ["latest"],
+      }),
+    );
+    form.append("files", new Blob(["# Demo"], { type: "text/markdown" }), "SKILL.md");
+    const request = new Request("https://clawhub.ai/api/v1/skills", {
+      method: "POST",
+      body: form,
+    });
+    const store = vi.fn().mockResolvedValue("storage:1");
+    const remove = vi.fn().mockResolvedValue(undefined);
+    const ctx = {
+      storage: {
+        store,
+        delete: remove,
+      },
+    } as unknown as ActionCtx;
+
+    await expect(parseMultipartPublish(ctx, request)).rejects.toThrow(/slug/i);
+    expect(store).toHaveBeenCalledTimes(1);
+    expect(remove).toHaveBeenCalledWith("storage:1");
+  });
+
+  it("does not store publish files when a later part exceeds the size limit", async () => {
+    const form = new FormData();
+    form.set(
+      "payload",
+      JSON.stringify({
+        slug: "demo",
+        displayName: "Demo",
+        version: "1.0.0",
+        changelog: "",
+        acceptLicenseTerms: true,
+        tags: ["latest"],
+      }),
+    );
+    form.append("files", new Blob(["# Demo"], { type: "text/markdown" }), "SKILL.md");
+    form.append(
+      "files",
+      new File([new Uint8Array(MAX_PUBLISH_FILE_BYTES + 1)], "big.bin", {
+        type: "application/octet-stream",
+      }),
+    );
+    const request = new Request("https://clawhub.ai/api/v1/skills", {
+      method: "POST",
+      body: form,
+    });
+    const store = vi.fn().mockResolvedValue("storage:1");
+    const remove = vi.fn().mockResolvedValue(undefined);
+    const ctx = {
+      storage: {
+        store,
+        delete: remove,
+      },
+    } as unknown as ActionCtx;
+
+    await expect(parseMultipartPublish(ctx, request)).rejects.toThrow(
+      'File "big.bin" exceeds 10MB limit',
+    );
+    expect(store).not.toHaveBeenCalled();
+    expect(remove).not.toHaveBeenCalled();
+  });
 });

convex/httpApiV1/shared.ts

@@ -386,6 +386,13 @@ function toFileLike(entry: FormDataEntryValue): FileLikeEntry | null {
   return entry as FileLikeEntry;
 }
 
+export async function deleteStoredMultipartFiles(
+  ctx: ActionCtx,
+  files: Array<{ storageId: Id<"_storage"> }>,
+) {
+  await Promise.allSettled(files.map((file) => ctx.storage.delete(file.storageId)));
+}
+
 export async function parseMultipartPublish(
   ctx: ActionCtx,
   request: Request,
@@ -402,6 +409,14 @@ export async function parseMultipartPublish(
     throw new Error("Invalid JSON payload");
   }
 
+  const fileEntries = form
+    .getAll("files")
+    .map((entry) => toFileLike(entry))
+    .filter((file): file is FileLikeEntry => Boolean(file))
+    .filter((file) => !isMacJunkPath(file.name));
+  const oversized = fileEntries.find((file) => file.size > MAX_PUBLISH_FILE_BYTES);
+  if (oversized) throw new Error(getPublishFileSizeError(oversized.name));
+
   const files: Array<{
     path: string;
     size: number;
@@ -410,44 +425,47 @@ export async function parseMultipartPublish(
     contentType?: string;
   }> = [];
 
-  for (const entry of form.getAll("files")) {
-    const file = toFileLike(entry);
-    if (!file) continue;
-    const path = file.name;
-    if (isMacJunkPath(path)) continue;
-    const size = file.size;
-    if (size > MAX_PUBLISH_FILE_BYTES) {
-      throw new Error(getPublishFileSizeError(path));
+  try {
+    for (const file of fileEntries) {
+      const path = file.name;
+      const size = file.size;
+      const contentType = file.type || undefined;
+      const buffer = new Uint8Array(await file.arrayBuffer());
+      const sha256 = await sha256Hex(buffer);
+      const storageId = await ctx.storage.store(file as Blob);
+      files.push({ path, size, storageId, sha256, contentType });
     }
-    const contentType = file.type || undefined;
-    const buffer = new Uint8Array(await file.arrayBuffer());
-    const sha256 = await sha256Hex(buffer);
-    const storageId = await ctx.storage.store(file as Blob);
-    files.push({ path, size, storageId, sha256, contentType });
-  }
 
-  const forkOf = payload.forkOf && typeof payload.forkOf === "object" ? payload.forkOf : undefined;
-  const hasAcceptLicenseTerms = Object.prototype.hasOwnProperty.call(payload, "acceptLicenseTerms");
-  const body = {
-    slug: payload.slug,
-    displayName: payload.displayName,
-    ...(typeof payload.ownerHandle === "string" ? { ownerHandle: payload.ownerHandle } : {}),
-    ...(typeof payload.sourceOwnerHandle === "string"
-      ? { sourceOwnerHandle: payload.sourceOwnerHandle }
-      : {}),
-    ...(typeof payload.migrateOwner === "boolean" ? { migrateOwner: payload.migrateOwner } : {}),
-    version: payload.version,
-    changelog: typeof payload.changelog === "string" ? payload.changelog : "",
-    ...(hasAcceptLicenseTerms ? { acceptLicenseTerms: payload.acceptLicenseTerms } : {}),
-    tags: Array.isArray(payload.tags) ? payload.tags : undefined,
-    ...(Array.isArray(payload.categories) ? { categories: payload.categories } : {}),
-    ...(Array.isArray(payload.topics) ? { topics: payload.topics } : {}),
-    ...(payload.source ? { source: payload.source } : {}),
-    files,
-    ...(forkOf ? { forkOf } : {}),
-  };
+    const forkOf =
+      payload.forkOf && typeof payload.forkOf === "object" ? payload.forkOf : undefined;
+    const hasAcceptLicenseTerms = Object.prototype.hasOwnProperty.call(
+      payload,
+      "acceptLicenseTerms",
+    );
+    const body = {
+      slug: payload.slug,
+      displayName: payload.displayName,
+      ...(typeof payload.ownerHandle === "string" ? { ownerHandle: payload.ownerHandle } : {}),
+      ...(typeof payload.sourceOwnerHandle === "string"
+        ? { sourceOwnerHandle: payload.sourceOwnerHandle }
+        : {}),
+      ...(typeof payload.migrateOwner === "boolean" ? { migrateOwner: payload.migrateOwner } : {}),
+      version: payload.version,
+      changelog: typeof payload.changelog === "string" ? payload.changelog : "",
+      ...(hasAcceptLicenseTerms ? { acceptLicenseTerms: payload.acceptLicenseTerms } : {}),
+      tags: Array.isArray(payload.tags) ? payload.tags : undefined,
+      ...(Array.isArray(payload.categories) ? { categories: payload.categories } : {}),
+      ...(Array.isArray(payload.topics) ? { topics: payload.topics } : {}),
+      ...(payload.source ? { source: payload.source } : {}),
+      files,
+      ...(forkOf ? { forkOf } : {}),
+    };
 
-  return parsePublishBody(body);
+    return parsePublishBody(body);
+  } catch (error) {
+    await deleteStoredMultipartFiles(ctx, files);
+    throw error;
+  }
 }
 
 export async function parseMultipartSkillScan(
@@ -508,7 +526,7 @@ export async function parseMultipartSkillScan(
       files.push({ path, size, storageId, sha256, contentType });
     }
   } catch (error) {
-    await Promise.allSettled(files.map((file) => ctx.storage.delete(file.storageId)));
+    await deleteStoredMultipartFiles(ctx, files);
     throw error;
   }