fix(media): delete the stored file on every delete path and surface storage failures

.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.

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<Database>,
 	id: string,
-): Promise<ApiResult<{ deleted: true; storageKey: string }>> {
+	storage?: Storage | null,
+): Promise<ApiResult<{ deleted: true; storageKey: string; storageDeleted: boolean }>> {
 	try {
 		const repo = new MediaRepository(db);
+		const notFound: ApiResult<never> = {
+			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 {

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),

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) })

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");
 	}

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<void> {
+		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<boolean> {
 		const row = await this.db
 			.selectFrom("_emdash_media_upload_attempts")
@@ -235,6 +257,7 @@ export class MediaRepository {
 	async deleteCompletedUploadAttempts(): Promise<number> {
 		const result = await this.db
 			.deleteFrom("_emdash_media_upload_attempts")
+			.where("status", "=", "active")
 			.where((eb) =>
 				eb.exists(
 					eb

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

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<boolean> {
-			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;
 		},
 	};
 }

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",

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<string>();
+		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 });

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<Database>;
+
+	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);
+	});
+});

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<EmDashConfig>;
+	/** 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: [],