From a712dd00e76526f2231f4817e792e0ca339992fb Mon Sep 17 00:00:00 2001 From: Hridayesh Date: Thu, 30 Jul 2026 14:23:29 +0900 Subject: [PATCH 1/2] fix(core): don't create pending media rows when storage can't pre-sign The upload-url route created the pending media record before asking storage for a signed URL. Adapters that cannot pre-sign -- local storage, and R2 accessed through a Worker binding -- throw NOT_SUPPORTED at that point, which the catch turns into a 501 so the client falls back to direct upload. The record was already committed by then. Those rows are invisible (findMany defaults to status='ready' and the list query exposes no status filter) and are only removed by cleanupPendingUploads(), which nothing schedules, so the table grew by one dead row per upload attempt on every such deployment. Ask storage for the URL first; the record is created only once it is known that one can be issued. --- .../media-signed-upload-pending-rows.md | 5 + .../src/astro/routes/api/media/upload-url.ts | 26 +++-- .../api/media-upload-url-pending.test.ts | 101 ++++++++++++++++++ 3 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 .changeset/media-signed-upload-pending-rows.md create mode 100644 packages/core/tests/integration/api/media-upload-url-pending.test.ts diff --git a/.changeset/media-signed-upload-pending-rows.md b/.changeset/media-signed-upload-pending-rows.md new file mode 100644 index 0000000000..d2f6526614 --- /dev/null +++ b/.changeset/media-signed-upload-pending-rows.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes the media table filling up with hidden, unusable `pending` records — one per upload attempt — when storage cannot create signed upload URLs, which is always the case for local storage and for R2 accessed through a Worker binding. diff --git a/packages/core/src/astro/routes/api/media/upload-url.ts b/packages/core/src/astro/routes/api/media/upload-url.ts index edfa95ff2e..ea30e689c4 100644 --- a/packages/core/src/astro/routes/api/media/upload-url.ts +++ b/packages/core/src/astro/routes/api/media/upload-url.ts @@ -103,6 +103,24 @@ export const POST: APIRoute = async ({ request, locals }) => { const ext = path.extname(body.filename) || ""; const storageKey = `${id}${ext}`; + // Get signed upload URL from storage. + // + // This must happen BEFORE the pending record is created. Adapters that + // cannot pre-sign -- local storage, and R2 accessed through a Worker + // binding -- throw NOT_SUPPORTED here, which the catch below turns into a + // 501 so the client falls back to direct upload. Creating the record first + // meant every such request committed a `status='pending'` row with no + // object behind it: invisible in the library (findMany defaults to + // `status='ready'`, and the list query exposes no status filter) and only + // ever removed by cleanupPendingUploads(), which nothing schedules. On + // those setups the table grew by one dead row per upload attempt. + const signedUrl = await emdash.storage.getSignedUploadUrl({ + key: storageKey, + contentType: body.contentType, + size: body.size, + expiresIn: 3600, // 1 hour + }); + // Create pending media record with content hash const mediaItem = await repo.createPending({ filename: body.filename, @@ -113,14 +131,6 @@ export const POST: APIRoute = async ({ request, locals }) => { authorId: user?.id, }); - // Get signed upload URL from storage - const signedUrl = await emdash.storage.getSignedUploadUrl({ - key: storageKey, - contentType: body.contentType, - size: body.size, - expiresIn: 3600, // 1 hour - }); - const response: UploadUrlResponse = { uploadUrl: signedUrl.url, method: signedUrl.method, diff --git a/packages/core/tests/integration/api/media-upload-url-pending.test.ts b/packages/core/tests/integration/api/media-upload-url-pending.test.ts new file mode 100644 index 0000000000..6c51f6ea60 --- /dev/null +++ b/packages/core/tests/integration/api/media-upload-url-pending.test.ts @@ -0,0 +1,101 @@ +/** + * The signed-upload endpoint must not leave a `pending` media row behind when + * storage cannot pre-sign. + * + * `POST /_emdash/api/media/upload-url` is the first call the admin's + * `uploadMedia()` makes; it falls back to direct multipart upload when the + * endpoint answers 501. Two shipped adapters can never pre-sign and always + * throw NOT_SUPPORTED — local storage (`LocalStorage.getSignedUploadUrl`) and + * R2 accessed through a Worker binding (`R2Storage.getSignedUploadUrl`) — so + * on those setups the 501 fallback is the *normal* path, taken on every single + * upload. + * + * The route used to create the pending record before asking storage for the + * URL, so each of those attempts committed a `status='pending'` row with no + * object behind it. The rows are invisible (`findMany` defaults to + * `status='ready'` and the list query exposes no status filter) and are only + * removed by `cleanupPendingUploads()`, which nothing schedules — so the table + * grew by one dead row per upload attempt, indefinitely. + */ +import { Role } from "@emdash-cms/auth"; +import type { APIContext } from "astro"; +import type { Kysely } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { POST as requestUploadUrl } from "../../../src/astro/routes/api/media/upload-url.js"; +import type { DatabaseSchema } from "../../../src/database/types.js"; +import { EmDashStorageError } from "../../../src/storage/types.js"; +import type { Storage } from "../../../src/storage/types.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; + +/** Storage that behaves like local storage / an R2 binding: it cannot pre-sign. */ +function storageThatCannotPresign(): Storage { + return { + getSignedUploadUrl() { + throw new EmDashStorageError( + "Local storage does not support signed upload URLs. Upload files directly through the API.", + "NOT_SUPPORTED", + ); + }, + } as unknown as Storage; +} + +function callRoute(db: Kysely, storage: Storage) { + const request = new Request("http://localhost:4321/_emdash/api/media/upload-url", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filename: "photo.png", contentType: "image/png", size: 1024 }), + }); + + return requestUploadUrl({ + request, + url: new URL(request.url), + params: {}, + locals: { + emdash: { db, storage, config: {} }, + user: { id: "admin-1", role: Role.ADMIN }, + }, + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- minimal stub for tests + } as unknown as APIContext); +} + +async function countMedia(db: Kysely, status: string): Promise { + const rows = await db.selectFrom("media").select("id").where("status", "=", status).execute(); + return rows.length; +} + +describe("POST /_emdash/api/media/upload-url with storage that cannot pre-sign", () => { + let db: Kysely; + + beforeEach(async () => { + db = await setupTestDatabase(); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + it("answers 501 NOT_SUPPORTED so the client falls back to direct upload", async () => { + const response = await callRoute(db, storageThatCannotPresign()); + + expect(response.status).toBe(501); + const body = (await response.json()) as { error: { code: string } }; + expect(body.error.code).toBe("NOT_SUPPORTED"); + }); + + it("does not create a pending media row", async () => { + await callRoute(db, storageThatCannotPresign()); + + expect(await countMedia(db, "pending")).toBe(0); + }); + + it("does not accumulate rows across repeated attempts", async () => { + for (let i = 0; i < 5; i++) { + // oxlint-disable-next-line no-await-in-loop -- sequential on purpose: the point is that repeats don't accumulate + await callRoute(db, storageThatCannotPresign()); + } + + const all = await db.selectFrom("media").select("id").execute(); + expect(all).toHaveLength(0); + }); +}); From 3425382c0f5cb5c26ffd4ebde11dc53f1e21ebed Mon Sep 17 00:00:00 2001 From: Hridayesh Date: Thu, 30 Jul 2026 14:40:37 +0900 Subject: [PATCH 2/2] docs: drop review narrative from the upload-url comments AGENTS.md scopes comments to a future reader of the code: not PR descriptions, not narrative about the previous behaviour, not references to the reporting issue. Keep the one thing a future reader would get wrong -- that the call order is load-bearing -- and drop the rest, which lives in the commit message and changeset. --- .../src/astro/routes/api/media/upload-url.ts | 13 ++---------- .../api/media-upload-url-pending.test.ts | 20 +------------------ 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/packages/core/src/astro/routes/api/media/upload-url.ts b/packages/core/src/astro/routes/api/media/upload-url.ts index ea30e689c4..6d07ea2a8c 100644 --- a/packages/core/src/astro/routes/api/media/upload-url.ts +++ b/packages/core/src/astro/routes/api/media/upload-url.ts @@ -103,17 +103,8 @@ export const POST: APIRoute = async ({ request, locals }) => { const ext = path.extname(body.filename) || ""; const storageKey = `${id}${ext}`; - // Get signed upload URL from storage. - // - // This must happen BEFORE the pending record is created. Adapters that - // cannot pre-sign -- local storage, and R2 accessed through a Worker - // binding -- throw NOT_SUPPORTED here, which the catch below turns into a - // 501 so the client falls back to direct upload. Creating the record first - // meant every such request committed a `status='pending'` row with no - // object behind it: invisible in the library (findMany defaults to - // `status='ready'`, and the list query exposes no status filter) and only - // ever removed by cleanupPendingUploads(), which nothing schedules. On - // those setups the table grew by one dead row per upload attempt. + // Get the signed upload URL before creating the pending row, so adapters + // that cannot pre-sign don't leave an orphaned `pending` record. const signedUrl = await emdash.storage.getSignedUploadUrl({ key: storageKey, contentType: body.contentType, diff --git a/packages/core/tests/integration/api/media-upload-url-pending.test.ts b/packages/core/tests/integration/api/media-upload-url-pending.test.ts index 6c51f6ea60..8f78595ecc 100644 --- a/packages/core/tests/integration/api/media-upload-url-pending.test.ts +++ b/packages/core/tests/integration/api/media-upload-url-pending.test.ts @@ -1,22 +1,4 @@ -/** - * The signed-upload endpoint must not leave a `pending` media row behind when - * storage cannot pre-sign. - * - * `POST /_emdash/api/media/upload-url` is the first call the admin's - * `uploadMedia()` makes; it falls back to direct multipart upload when the - * endpoint answers 501. Two shipped adapters can never pre-sign and always - * throw NOT_SUPPORTED — local storage (`LocalStorage.getSignedUploadUrl`) and - * R2 accessed through a Worker binding (`R2Storage.getSignedUploadUrl`) — so - * on those setups the 501 fallback is the *normal* path, taken on every single - * upload. - * - * The route used to create the pending record before asking storage for the - * URL, so each of those attempts committed a `status='pending'` row with no - * object behind it. The rows are invisible (`findMany` defaults to - * `status='ready'` and the list query exposes no status filter) and are only - * removed by `cleanupPendingUploads()`, which nothing schedules — so the table - * grew by one dead row per upload attempt, indefinitely. - */ +/** The signed-upload endpoint must not leave a pending media row behind when storage cannot pre-sign. */ import { Role } from "@emdash-cms/auth"; import type { APIContext } from "astro"; import type { Kysely } from "kysely";