diff --git a/.changeset/media-delete-storage-cleanup.md b/.changeset/media-delete-storage-cleanup.md new file mode 100644 index 0000000000..d0a2b8f63c --- /dev/null +++ b/.changeset/media-delete-storage-cleanup.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Media deletion no longer leaves files behind. The MCP `media_delete` tool and the plugin `ctx.media.delete()` API now remove the stored file as well as the record, matching the admin API. When the storage delete fails, `DELETE /_emdash/api/media/:id` reports `storageDeleted: false` instead of a plain success, and the periodic cleanup retries the file deletion until it succeeds. diff --git a/packages/core/src/api/handlers/media.ts b/packages/core/src/api/handlers/media.ts index f173ea2d49..2fb6a69229 100644 --- a/packages/core/src/api/handlers/media.ts +++ b/packages/core/src/api/handlers/media.ts @@ -8,6 +8,8 @@ import { MediaRepository, type MediaItem } from "../../database/repositories/med import { InvalidCursorError } from "../../database/repositories/types.js"; import type { Database } from "../../database/types.js"; import { isValidFocalPointUpdate, type FocalPointUpdate } from "../../media/focal-point.js"; +import { removeUploadAttempt } from "../../media/upload-attempts.js"; +import type { Storage } from "../../storage/types.js"; import type { ApiResult } from "../types.js"; const FOREIGN_KEY_VIOLATION_RE = /foreign key constraint failed/i; @@ -270,29 +272,38 @@ function isForeignKeyViolation(error: unknown): boolean { } /** - * Delete media item + * Delete a media item and its stored object. + * + * The object is registered for cleanup before the row is removed, so when + * the storage delete fails the cleanup sweep retries it; `storageDeleted` + * tells the caller whether the object is already gone. */ export async function handleMediaDelete( db: Kysely, id: string, -): Promise> { + storage?: Storage | null, +): Promise> { try { const repo = new MediaRepository(db); + const notFound: ApiResult = { + success: false, + error: { code: "NOT_FOUND", message: `Media item not found: ${id}` }, + }; + + const media = await repo.findById(id); + if (!media) return notFound; + + if (storage) await repo.trackStorageKeyForCleanup(media.id, media.storageKey); const storageKey = await repo.deleteWithStorageKey(id); + if (!storageKey) return notFound; - if (!storageKey) { - return { - success: false, - error: { - code: "NOT_FOUND", - message: `Media item not found: ${id}`, - }, - }; - } + const storageDeleted = storage + ? await removeUploadAttempt(storage, repo, storageKey, { allowUntracked: true }) + : false; return { success: true, - data: { deleted: true, storageKey }, + data: { deleted: true, storageKey, storageDeleted }, }; } catch { return { diff --git a/packages/core/src/api/openapi/document.ts b/packages/core/src/api/openapi/document.ts index 15127cb17d..0f5463eee9 100644 --- a/packages/core/src/api/openapi/document.ts +++ b/packages/core/src/api/openapi/document.ts @@ -21,7 +21,12 @@ import { adminCommentListResponseSchema, publicCommentListResponseSchema, } from "../schemas/comments.js"; -import { apiErrorSchema, deleteResponseSchema, successEnvelope } from "../schemas/common.js"; +import { + apiErrorSchema, + deleteResponseSchema, + mediaDeleteResponseSchema, + successEnvelope, +} from "../schemas/common.js"; import { contentCompareResponseSchema, contentAuthorsResponseSchema, @@ -1014,7 +1019,7 @@ function buildMediaPaths(maxUploadSize: number) { responses: { "200": { description: "Deleted", - content: { [JSON_CONTENT]: { schema: successEnvelope(deleteResponseSchema) } }, + content: { [JSON_CONTENT]: { schema: successEnvelope(mediaDeleteResponseSchema) } }, }, ...authErrors, ...standardErrors(404, 500), diff --git a/packages/core/src/api/schemas/common.ts b/packages/core/src/api/schemas/common.ts index 1d6556922d..ba9be767aa 100644 --- a/packages/core/src/api/schemas/common.ts +++ b/packages/core/src/api/schemas/common.ts @@ -98,6 +98,11 @@ export const deleteResponseSchema = z.object({ deleted: z.literal(true) }).meta( id: "DeleteResponse", }); +/** Media delete response: `storageDeleted` is false when the stored file survived and is retried by cleanup */ +export const mediaDeleteResponseSchema = deleteResponseSchema + .extend({ storageDeleted: z.boolean() }) + .meta({ id: "MediaDeleteResponse" }); + /** Standard count response */ export const countResponseSchema = z .object({ count: z.number().int().min(0) }) diff --git a/packages/core/src/astro/routes/api/media/[id].ts b/packages/core/src/astro/routes/api/media/[id].ts index 7148ec9a9a..10eb112267 100644 --- a/packages/core/src/astro/routes/api/media/[id].ts +++ b/packages/core/src/astro/routes/api/media/[id].ts @@ -13,9 +13,6 @@ import { apiError, apiSuccess, handleError, unwrapResult } from "#api/error.js"; import { handleMediaUsageSummaries } from "#api/handlers/media-usage.js"; import { isParseError, parseBody, parseQuery } from "#api/parse.js"; import { mediaGetQuery, mediaUpdateBody } from "#api/schemas.js"; -import { MediaRepository } from "#db/repositories/media.js"; -import { removeUploadAttempt } from "#media/upload-attempts.js"; - export const prerender = false; /** @@ -142,27 +139,20 @@ export const DELETE: APIRoute = async ({ params, locals }) => { ); if (ownerDenied) return ownerDenied; - // Delete from database — site-settings cache invalidation happens - // in `EmDashRuntime.handleMediaDelete` so MCP/plugin paths inherit it. + // Storage deletion and site-settings cache invalidation happen in + // `EmDashRuntime.handleMediaDelete` so the MCP tool inherits them. const result = await emdash.handleMediaDelete(id); if (!result.success) return unwrapResult(result); if ( typeof result.data !== "object" || result.data === null || - !("storageKey" in result.data) || - typeof result.data.storageKey !== "string" + !("storageDeleted" in result.data) || + typeof result.data.storageDeleted !== "boolean" ) { return apiError("MEDIA_DELETE_ERROR", "Failed to delete media", 500); } - if (emdash.storage) { - const repo = new MediaRepository(emdash.db); - await removeUploadAttempt(emdash.storage, repo, result.data.storageKey, { - allowUntracked: true, - }); - } - - return apiSuccess({ deleted: true }); + return apiSuccess({ deleted: true, storageDeleted: result.data.storageDeleted }); } catch (error) { return handleError(error, "Failed to delete media", "MEDIA_DELETE_ERROR"); } diff --git a/packages/core/src/database/repositories/media.ts b/packages/core/src/database/repositories/media.ts index 573bd42a3c..11a9014939 100644 --- a/packages/core/src/database/repositories/media.ts +++ b/packages/core/src/database/repositories/media.ts @@ -196,6 +196,28 @@ export class MediaRepository { .execute(); } + /** + * Register a stored object for cleanup before its media row is removed, + * so a failed storage delete is retried by the cleanup sweep instead of + * leaving the object unreferenced and unreachable. + */ + async trackStorageKeyForCleanup(mediaId: string, storageKey: string): Promise { + const now = new Date().toISOString(); + await this.db + .insertInto("_emdash_media_upload_attempts") + .values({ + media_id: mediaId, + storage_key: storageKey, + status: "cleanup", + created_at: now, + updated_at: now, + }) + .onConflict((oc) => + oc.column("storage_key").doUpdateSet({ status: "cleanup", updated_at: now }), + ) + .execute(); + } + async hasUploadAttempt(storageKey: string): Promise { const row = await this.db .selectFrom("_emdash_media_upload_attempts") @@ -235,6 +257,7 @@ export class MediaRepository { async deleteCompletedUploadAttempts(): Promise { const result = await this.db .deleteFrom("_emdash_media_upload_attempts") + .where("status", "=", "active") .where((eb) => eb.exists( eb diff --git a/packages/core/src/emdash-runtime.ts b/packages/core/src/emdash-runtime.ts index 50a35fad0f..dd295e2fca 100644 --- a/packages/core/src/emdash-runtime.ts +++ b/packages/core/src/emdash-runtime.ts @@ -3436,7 +3436,7 @@ export class EmDashRuntime { } async handleMediaDelete(id: string) { - const result = await handleMediaDelete(this.db, id); + const result = await handleMediaDelete(this.db, id, this.storage); // Same reasoning as `handleMediaUpdate`: if the deleted media row // was referenced by a setting, the cached resolved URL now points // at a 404. Invalidation is unconditional on success — cheaper than diff --git a/packages/core/src/plugins/context.ts b/packages/core/src/plugins/context.ts index f3ca01cb36..e80eb09821 100644 --- a/packages/core/src/plugins/context.ts +++ b/packages/core/src/plugins/context.ts @@ -8,6 +8,7 @@ import type { Kysely } from "kysely"; import { ulid } from "ulidx"; +import { handleMediaDelete } from "../api/handlers/media.js"; import { ContentRepository } from "../database/repositories/content.js"; import { EntryLockRepository } from "../database/repositories/entry-locks.js"; import { MediaRepository } from "../database/repositories/media.js"; @@ -672,16 +673,16 @@ export function createMediaAccessWithWrite( }, async delete(id: string): Promise { - const deleted = await mediaRepo.delete(id); + const result = await handleMediaDelete(db, id, storage); // Plugins can delete media that's referenced by site settings // (`logo`, `favicon`, `seo.defaultOgImage`); the worker-scoped // resolved-URL cache must be dropped or it will keep serving // 404s. Matches the invalidation in // `EmDashRuntime.handleMediaDelete`. - if (deleted) { + if (result.success) { invalidateSiteSettingsCache(); } - return deleted; + return result.success; }, }; } diff --git a/packages/core/tests/integration/astro/media-stream-upload.test.ts b/packages/core/tests/integration/astro/media-stream-upload.test.ts index c732e2d6be..1903129861 100644 --- a/packages/core/tests/integration/astro/media-stream-upload.test.ts +++ b/packages/core/tests/integration/astro/media-stream-upload.test.ts @@ -1151,7 +1151,7 @@ describe("streamed media upload fallback", () => { handleMediaDelete: async (id: string) => { startDelete?.(); await allowDelete; - return handleMediaDelete(db, id); + return handleMediaDelete(db, id, storage); }, }, user: { @@ -1181,6 +1181,63 @@ describe("streamed media upload fallback", () => { expect(storage.objects.size).toBe(0); }); + it("reports a failed storage delete and leaves the object reachable for cleanup", async () => { + const repo = new MediaRepository(db); + const media = await repo.create({ + filename: "photo.png", + mimeType: "image/png", + size: 3, + storageKey: "photo.png", + authorId: "user-1", + }); + const storage = streamingStorage(); + storage.objects.set(media.storageKey, new Uint8Array([1, 2, 3])); + storage.delete.mockRejectedValueOnce(new Error("bucket unavailable")); + + const response = await deleteMedia({ + params: { id: media.id }, + locals: { + emdash: { + db, + storage, + handleMediaGet: (id: string) => handleMediaGet(db, id), + handleMediaDelete: (id: string) => handleMediaDelete(db, id, storage), + }, + user: { id: "user-1", email: "test@example.com", name: "Test User", role: 30 }, + }, + } as unknown as APIContext); + + expect(response.status).toBe(200); + const body = (await response.json()) as { data: unknown }; + expect(body.data).toEqual({ deleted: true, storageDeleted: false }); + expect(await repo.findById(media.id)).toBeNull(); + expect(storage.objects.has(media.storageKey)).toBe(true); + + await runSystemCleanup(db, storage); + + expect(storage.objects.size).toBe(0); + expect(await repo.hasUploadAttempt(media.storageKey)).toBe(false); + }); + + it("keeps an object marked for cleanup tracked while its media row still exists", async () => { + const repo = new MediaRepository(db); + const media = await repo.create({ + filename: "photo.png", + mimeType: "image/png", + size: 3, + storageKey: "photo.png", + authorId: "user-1", + }); + const storage = streamingStorage(); + storage.objects.set(media.storageKey, new Uint8Array([1, 2, 3])); + await repo.trackStorageKeyForCleanup(media.id, media.storageKey); + + await runSystemCleanup(db, storage); + + expect(await repo.hasUploadAttempt(media.storageKey)).toBe(true); + expect(storage.objects.size).toBe(1); + }); + it("rejects a non-owner without media:edit_any", async () => { const pending = await new MediaRepository(db).createPending({ filename: "photo.png", diff --git a/packages/core/tests/integration/mcp/media.test.ts b/packages/core/tests/integration/mcp/media.test.ts index a9615b094d..239a41efd9 100644 --- a/packages/core/tests/integration/mcp/media.test.ts +++ b/packages/core/tests/integration/mcp/media.test.ts @@ -19,6 +19,7 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { MediaRepository } from "../../../src/database/repositories/media.js"; import type { Database } from "../../../src/database/types.js"; +import type { Storage } from "../../../src/storage/types.js"; import { connectMcpHarness, extractJson, @@ -350,6 +351,36 @@ describe("media_delete", () => { expect(got.isError).toBe(true); }); + it("removes the stored file along with the record", async () => { + const objects = new Set(); + const storage = { + delete: async (key: string) => { + objects.delete(key); + }, + } as unknown as Storage; + const item = await new MediaRepository(db).create({ + filename: "photo.png", + mimeType: "image/png", + size: 3, + storageKey: "media/photo.png", + authorId: ADMIN_ID, + }); + objects.add(item.storageKey); + harness = await connectMcpHarness({ + db, + userId: ADMIN_ID, + userRole: Role.ADMIN, + runtimeOptions: { storage }, + }); + + const result = await harness.client.callTool({ + name: "media_delete", + arguments: { id: item.id }, + }); + expect(result.isError, extractText(result)).toBeFalsy(); + expect(objects.size).toBe(0); + }); + it("AUTHOR cannot delete another user's media", async () => { const id = await seedMedia(db, { authorId: OTHER_AUTHOR_ID }); harness = await connectMcpHarness({ db, userId: AUTHOR_ID, userRole: Role.AUTHOR }); diff --git a/packages/core/tests/integration/runtime/plugin-media-enrich.test.ts b/packages/core/tests/integration/runtime/plugin-media-enrich.test.ts index 682bd89046..1ef84b1c23 100644 --- a/packages/core/tests/integration/runtime/plugin-media-enrich.test.ts +++ b/packages/core/tests/integration/runtime/plugin-media-enrich.test.ts @@ -83,3 +83,36 @@ describe("plugin ctx.media.upload — metadata enrichment", () => { expect(row?.blurhash).toBeNull(); }); }); + +describe("plugin ctx.media.delete", () => { + let db: Kysely; + + beforeEach(async () => { + db = await setupTestDatabase(); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + it("removes the stored file along with the record", async () => { + const storage = fakeStorage(); + const media = createMediaAccessWithWrite(db, undefined, storage); + const uploaded = await media.upload( + "data.bin", + "application/octet-stream", + new Uint8Array([1, 2, 3, 4]).buffer, + ); + expect(await storage.exists(uploaded.storageKey)).toBe(true); + + expect(await media.delete(uploaded.mediaId)).toBe(true); + + expect(await storage.exists(uploaded.storageKey)).toBe(false); + expect(await new MediaRepository(db).findById(uploaded.mediaId)).toBeNull(); + }); + + it("returns false for an unknown id", async () => { + const media = createMediaAccessWithWrite(db, undefined, fakeStorage()); + expect(await media.delete("missing")).toBe(false); + }); +}); diff --git a/packages/core/tests/utils/mcp-runtime.ts b/packages/core/tests/utils/mcp-runtime.ts index 46ce6fb8dc..f35371e5be 100644 --- a/packages/core/tests/utils/mcp-runtime.ts +++ b/packages/core/tests/utils/mcp-runtime.ts @@ -25,6 +25,7 @@ import { createMcpServer } from "../../src/mcp/server.js"; import { createHookPipeline } from "../../src/plugins/hooks.js"; import type { ResolvedPlugin } from "../../src/plugins/types.js"; import { invalidateUrlPatternCache } from "../../src/query.js"; +import type { Storage } from "../../src/storage/types.js"; // --------------------------------------------------------------------------- // Auth-injecting transport @@ -96,6 +97,8 @@ export interface TestRuntimeOptions { plugins?: ResolvedPlugin[]; /** Optional partial config override. Default: empty config. */ config?: Partial; + /** Optional storage adapter for media tools. Default: none. */ + storage?: Storage | null; } /** @@ -135,7 +138,7 @@ export function createTestRuntime( return new EmDashRuntime({ db, - storage: null, + storage: opts.storage ?? null, configuredPlugins: plugins, sandboxedPlugins: new Map(), sandboxedPluginEntries: [],