fix(core): don't create pending media rows when storage can't pre-sign - #2284
fix(core): don't create pending media rows when storage can't pre-sign#2284Hridayesh13 wants to merge 2 commits into
Conversation
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.
🦋 Changeset detectedLatest commit: 3425382 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
|
All contributors have signed the CLA ✍️ ✅ |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for the right problem: moving the storage getSignedUploadUrl() call before repo.createPending() prevents the route from leaving orphaned pending rows on adapters that cannot pre-sign, while preserving the existing 501 NOT_SUPPORTED fallback so the admin client still switches to direct upload. The change is minimal, well-scoped, and matches the architecture.
I read the full diff, the route, MediaRepository, the RBAC permission map, and the changeset. The authorization check (media:upload), error envelope shape, SQL-safety, and changeset all look correct. The new integration test exercises observable behavior (no row is written, repeated calls don't accumulate, 501 still returned) rather than asserting implementation details.
The only issues are two AGENTS.md comment-discipline violations: both the route and the new test file paste PR-style narrative into code comments, including adapter names and a reference to the broader issue's design question. These should be trimmed to one-sentence explanations.
| // 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 |
There was a problem hiding this comment.
[needs fixing] This comment block is PR narrative pasted into the source: it restates the bug, names internal adapters and cleanupPendingUploads(), and walks through the rejected implementation. AGENTS.md says comments must not be PR descriptions, justification, narrative, or issue/PR references. The ordering constraint is genuinely non-obvious, so a one-line "why" is fine — trim the rest.
| // 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 | |
| // Get the signed upload URL before creating the pending row, so adapters | |
| // that cannot pre-sign don't leave an orphaned `pending` record. |
There was a problem hiding this comment.
Fixed in 3425382 — trimmed to your suggested two lines. Kept the ordering note since that is the one thing a future reader could undo by accident.
| /** | ||
| * 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 |
There was a problem hiding this comment.
[needs fixing] The file-level JSDoc is a copy of the PR description: it narrates the bug, compares adapters, and refers to the original issue's open design question. AGENTS.md says comments must not be PR descriptions, narrative, or issue references — that context belongs in the PR/commit message, not evergreen test comments. Keep a single sentence, or delete the block since the describe title already states the purpose.
| /** | |
| * 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 | |
| /** The signed-upload endpoint must not leave a pending media row behind when storage cannot pre-sign. */ |
There was a problem hiding this comment.
Fixed in 3425382 — replaced with your suggested one-liner.
|
I have read the CLA Document and I hereby sign the CLA |
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.
github-actions
Bot
added
review/needs-rereview
khoinguyenpham04
added
the
bot:review
There was a problem hiding this comment.
This is still the right fix for the right problem. Moving getSignedUploadUrl() ahead of repo.createPending() stops the route from leaving hidden pending rows on adapters that cannot pre-sign, while keeping the existing 501 NOT_SUPPORTED fallback intact so the admin client falls back to direct upload. I re-read the route, MediaRepository, the storage and RBAC types, the changeset, and the related upload-widening tests.
Verification summary:
- Authorization check (
media:upload) is unchanged and correct. - Error envelope shape and SQL safety are unchanged.
- The changeset is user-facing and present-tense.
- The new integration test exercises observable behavior (no row written, repeated calls don't accumulate, 501 still returned) rather than asserting implementation details.
- The route comment from the prior review has been trimmed to a concise one-sentence explanation of the ordering constraint.
One AGENTS.md comment-discipline issue remains: the test helper comment still pastes PR-specific adapter names into the code. I'd like that trimmed before merge.
| 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. */ |
There was a problem hiding this comment.
[needs fixing] This comment still embeds PR-specific adapter names (local storage / an R2 binding). That context belongs in the PR description, not in a comment that future readers will see long after the adapter list changes. Trim it to a generic, one-sentence description of what the helper does.
| /** Storage that behaves like local storage / an R2 binding: it cannot pre-sign. */ | |
| /** Storage adapter that does not support pre-signed upload URLs. */ |
github-actions
Bot
added
review/awaiting-author
|
Closing this — #2273 landed while it was in review and supersedes it.
That makes this a rewrite rather than a rebase — the integration test here asserts "no row written, repeated calls don't accumulate, 501 still returned", and all three of those are now wrong by design. Thanks for the review passes — the comment-discipline notes were fair and I applied them in 3425382 (the last one, on the test helper, is moot now). Happy to open a fresh, narrow PR covering the new contract — that the |
2 participants
What does this PR do?
POST /_emdash/api/media/upload-urlcreates the pending media record before asking storage for a signed URL:Adapters that cannot pre-sign throw
NOT_SUPPORTEDon that second call. For R2 accessed through a Worker binding this is unconditional and permanent — a property of the adapter, not a misconfiguration (packages/cloudflare/src/storage/r2.ts) — and local storage behaves the same way. The route catches it and correctly answers501, the admin correctly falls back to direct upload, and the user-visible flow works. But the insert has already committed, and nothing rolls it back.Those rows are invisible:
findMany()defaults tostatus='ready'and the list query exposes nostatusparameter, so they never appear in the library or through the API.cleanupPendingUploads()exists but I could not find anything scheduling it. In practice themediatable grows by one dead row per upload attempt, forever, on every such deployment. On our install 11 accumulated during one short debugging session before we noticed — and only because we were instrumenting something else.This PR moves the
getSignedUploadUrl()call abovecreatePending(), so the record is written only once it is known a URL can be issued. The 501 response and the client fallback are unchanged.This is the "possible side effect" noted at the end of #2256 — that issue reports the same endpoint from the user-facing angle (the failed request in DevTools). This PR fixes only the row-accumulation half; the question that issue raises, whether the admin should skip the probe altogether for adapters known not to support pre-signing, is a separate design call and is left open.
Refs #2256
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runScreenshots / test output
packages/core/tests/integration/api/media-upload-url-pending.test.tscovers the endpoint against a storage adapter that cannot pre-sign: it still answers 501, it writes no row, and repeated attempts do not accumulate. It detects a revert — with the fix backed out and everything else unchanged:With the fix applied:
Note the first assertion passes either way — that is deliberate, it pins the 501 contract this PR must not change.
Full suites on this branch's base (
668184f):Two notes on the shared gates, both reproduced on a clean checkout of
main@668184fwith no changes applied, i.e. pre-existing and unrelated to this PR:pnpm typecheckfails inpackages/core—src/plugins/context.ts(1212,24): error TS2345: Argument of type '"cache:purge"' is not assignable to parameter of type 'PluginCapability'.pnpm lintreports 1 warning (and--deny-warningsturns it into a failure) —typescript(no-unnecessary-type-assertion)atpackages/cloudflare/src/sandbox/bridge.ts:1210.Happy to rebase once those are sorted if it makes CI easier to read.
AI-generated code disclosure