fix(media): delete the stored file on every delete path and surface storage failures - #3067
fix(media): delete the stored file on every delete path and surface storage failures#3067swissky wants to merge 2 commits into
Conversation
…torage failures The media delete handler now removes the object from storage itself, so the MCP media_delete tool deletes the file as its description promises. Before the row is removed the storage key is registered in the upload attempts table, so a failed storage delete is retried by the cleanup sweep instead of leaving an unreferenced, still-public object. The API response reports storageDeleted so callers learn when the object survived.
🦋 Changeset detectedLatest commit: 6934481 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
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: centralising storage deletion in handleMediaDelete and reusing the existing cleanup sweep lets a failed storage delete retry later while preserving the row-first ordering that protects the upload race. The REST and MCP paths now correctly remove the stored object and surface storageDeleted, and the tests reproduce the failure and cleanup scenarios.
The remaining gap is the plugin-facing delete path. packages/core/src/plugins/context.ts still deletes only the media row and never touches the stored object or the cleanup marker, so plugins calling ctx.media.delete() will leave orphan files behind — exactly the bug this PR is fixing for REST and MCP. That path should share the same cleanup pattern (or delegate to the handler) before merge.
Otherwise the diff is clean: no new logged-out queries, no SQL injection surface, the OpenAPI schema is additive, and the changeset is acceptable as user-facing release notes.
Findings
-
[needs fixing]
packages/core/src/plugins/context.ts:674-684The plugin-facing
media.delete()still deletes only the database row:async delete(id: string): Promise<boolean> { const deleted = await mediaRepo.delete(id); // ... if (deleted) { invalidateSiteSettingsCache(); } return deleted; },
It never deletes the stored object and never registers the
cleanupmarker, so a plugin delete leaves an orphan file behind — the same failure mode #3049 fixes for the admin API and MCP. The tests for the site-settings cache even call this out as one of the three media delete paths (packages/core/tests/unit/settings/settings.test.ts).It should share the same cleanup pattern as the handler, e.g. by delegating to
handleMediaDelete(and adding the import from../api/handlers/media.js):async delete(id: string): Promise<boolean> { const result = await handleMediaDelete(db, id, storage ?? null); if (result.success && result.data?.deleted) { invalidateSiteSettingsCache(); return true; } return false; },This reuses the row-first delete order, storage failure handling, and retry marker rather than recreating the bug for plugin callers.
|
Addressed the review: |
github-actions
Bot
added
size/L
review/needs-rereview
There was a problem hiding this comment.
Re-review of the media-delete fix.
The plugin-facing gap from the last review is resolved: packages/core/src/plugins/context.ts now delegates ctx.media.delete() to handleMediaDelete, so plugin callers share the same row-first delete order, storage-failure handling, and cleanup-marker retry as the REST and MCP paths. The new tests reproduce the failed-storage-delete and cleanup-marker scenarios, and the OpenAPI response change is additive.
The approach is sound—centralising storage deletion in handleMediaDelete and reusing the existing cleanup sweep means a failed storage delete is retried on every cron tick instead of being lost, while preserving the row-first ordering that protects the upload race.
One real delete path still has the original bug. packages/core/src/media/local-runtime.ts implements the local media provider's delete() and is called by DELETE /_emdash/api/media/providers/:providerId/:itemId. It still deletes the storage object directly, swallows any storage error, deletes the row without registering a cleanup marker, and does so in storage-before-row order. That leaves the same orphan-file failure mode this PR is fixing, and the site-settings cache tests explicitly list local-runtime.delete() as one of the three media-delete paths. It should share the same cleanup pattern, e.g. by delegating to handleMediaDelete.
Otherwise the diff is clean: no new logged-out queries, no SQL-injection surface, the changeset is useful release-facing prose, and comments are acceptable.
Findings
-
[needs fixing]
packages/core/src/media/local-runtime.ts:130The local media provider's
delete()still handles deletion manually: it callsstorage.delete()directly, swallows any error, then deletes the row, with nocleanupmarker and no retry path. This is the same failure mode #3049 fixes for the admin API, MCP tool, and plugin API—if the storage delete fails, the row is gone and the object is never found again. The site-settings cache tests also call this out as one of the three media-delete paths.It should share the centralized handler so a failed local delete is retried by the cleanup sweep:
async delete(id: string) { const result = await handleMediaDelete(resolveDb(), id, storage); if (result.success) { invalidateSiteSettingsCache(); } },This reuses the row-first ordering, storage-failure reporting, and
cleanupmarker that the other delete paths now use. Be sure to add the import forhandleMediaDeletefrom../api/handlers/media.js.
github-actions
Bot
added
review/awaiting-author
1 participant
What does this PR do?
Fixes the first half of #3049: a media delete could report
{ deleted: true }while the object was never removed from storage, and because the row was already gone nothing could ever find the object again.handleMediaDelete(db, id, storage)now owns storage deletion.EmDashRuntime.handleMediaDeletepasses its storage adapter, so the MCPmedia_deletetool removes the file too (its description already promised that; it only deleted the row). The REST route just forwards the result._emdash_media_upload_attemptswith statuscleanup(MediaRepository.trackStorageKeyForCleanup, an upsert on the primary key). The existing cleanup sweep (runSystemCleanup→findUploadAttemptsForCleanup→removeUploadAttempt) already retries rows in that state once the media row is gone, so a failed storage delete is retried on every cron tick instead of being lost.deleteCompletedUploadAttemptsskipscleanuprows so a cron tick landing between the upsert and the row delete cannot reap the marker.DELETE /_emdash/api/media/:idresponds{ deleted: true, storageDeleted: boolean }(OpenAPIMediaDeleteResponse). Additive; the admin client only checks the envelope.Not changed: the public file route still serves any key that exists in storage (second half of the issue). Gating it on a
mediarow adds a query to a logged-out route; with deletes now retried until the object is gone, I'd leave that for a maintainer decision.Tests (all fail before the fix): route reports
storageDeleted: falsewhen storage rejects, the object stays reachable and the nextrunSystemCleanupremoves it; MCPmedia_deleteremoves the stored object; acleanup-marked key survives the sweep while its media row still exists.Refs #3049 (leaves the file-route question open, see above)
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Not applicable (no UI change). i18n and Discussion items are n/a: no admin strings, bug fix.