fix(core): defer afterDelete and afterUnpublish hooks so Workers doesn't cancel them - #1588
Conversation
🦋 Changeset detectedLatest commit: 6633f93 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 |
@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-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.
This is the right fix for the right problem. afterDelete and afterUnpublish hook dispatch was the only remaining lifecycle cleanup path that dispatched on a bare promise, so on Cloudflare Workers it could be canceled as soon as the HTTP response went out. Wrapping both in after() matches the existing afterSave / afterPublish pattern exactly, which is the documented way to defer bookkeeping past the response and hand the lifetime to waitUntil.
I read the full diff, the after() implementation, the existing runAfterSaveHooks / runAfterPublishHooks implementations, and the new test. The code changes mirror the established pattern closely: trusted hook awaited with try/catch, sandboxed plugins collected into tasks and awaited via Promise.allSettled, and the entry-point comments updated to reflect deferred execution. The test directly verifies the scheduling contract by mocking after() and asserting the hook handler is not run inline but is flushed from the deferred queue, and it fails on the pre-fix behavior.
No blocking issues, no AGENTS.md convention violations, no SQL/Tailwind/Lingui concerns, and the changeset is present and correctly names the emdash package. LGTM.
There was a problem hiding this comment.
Pull request overview
This PR fixes a Cloudflare Workers runtime-lifetime bug where content:afterDelete and content:afterUnpublish plugin hooks could be canceled when the HTTP response completes, by deferring those hook dispatchers through the existing after() mechanism (which integrates with Workers waitUntil when available).
Changes:
- Wrap
runAfterDeleteHooksandrunAfterUnpublishHooksinafter()and align their trusted/sandboxed hook execution structure with the existingafterSave/afterPublishimplementations. - Add an integration test that mocks
after()to assert delete/unpublish hook work is scheduled (deferred) rather than executed inline. - Add a patch changeset documenting the Workers-specific fix and user impact.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/emdash-runtime.ts | Defers after-delete and after-unpublish hook dispatch via after() and awaits sandboxed hook tasks with Promise.allSettled inside the deferred work. |
| packages/core/tests/integration/runtime/after-hooks-deferred.test.ts | Adds coverage that pins the “deferred via after()” contract for delete and unpublish lifecycle hooks. |
| .changeset/defer-delete-unpublish-hooks.md | Documents the patch-level fix for dropped lifecycle hooks on Cloudflare Workers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Re-review of #1588. This is the same fix I previously approved: runAfterDeleteHooks and runAfterUnpublishHooks now wrap their hook dispatch in after(), mirroring the existing runAfterSaveHooks / runAfterPublishHooks pattern so Cloudflare Workers’ waitUntil can keep the work alive past the HTTP response.
I re-read the diff, the full emdash-runtime.ts implementation around all four lifecycle dispatchers, the after() implementation, and the new after-hooks-deferred.test.ts. The changes remain consistent with the established deferred pattern: trusted hooks are awaited with try/catch, sandboxed plugins are collected into tasks and awaited via Promise.allSettled, and the entry-point comments now correctly say "deferred past the response via after()". I also checked that no other bare-promise hook dispatches remain in emdash-runtime.ts.
The test directly verifies the scheduling contract by mocking after() and asserting the handlers are not run inline but are flushed from the deferred queue. The changeset is present and names the emdash package.
No blocking issues and no AGENTS.md violations. The prior approval stands; LGTM.
|
@marcusbellamyshaw-cell can you resolve the conflicts |
…n't cancel them runAfterDeleteHooks and runAfterUnpublishHooks dispatched plugin hooks fire-and-forget. On Cloudflare Workers a promise not handed to waitUntil is canceled when the response returns, so plugin cleanup work (R2 deletes, search-index removal) is killed mid-flight -- and a half-completed plugin-storage write can wedge the isolate and hang later admin requests. Wrap both dispatchers in after(), exactly as runAfterSaveHooks and runAfterPublishHooks already do. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7259317 to
4208c78
Compare
github-actions
Bot
added
review/needs-rereview
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
|
Bumping this — all checks are green (last run 2026-07-02), just needs a maintainer look before the stale-bot auto-closes it. Happy to rebase/update if anything's changed upstream. |
ascorbic
added
bot:review
There was a problem hiding this comment.
Re-review of #1588. This is the same narrowly-scoped bug fix I previously approved, and the changes remain sound.
Approach: Deferring content:afterDelete hook dispatch through after() so it survives the Cloudflare Workers response boundary is the right fix for the reported symptom (plugin cleanup work killed mid-flight). The new implementation of runAfterDeleteHooks mirrors the existing runAfterSaveHooks shape: trusted hooks awaited with try/catch, sandboxed plugins collected into tasks and awaited via Promise.allSettled, all wrapped in after().
runAfterUnpublishHooks already delegates to the shared runDeferredContentHook() helper, which is already wrapped in after(); the PR just corrects the stale "fire-and-forget" comment above the call site in handleContentUnpublish.
What I re-checked:
- Full
packages/core/src/emdash-runtime.tsaround all four lifecycle dispatchers and theafter()implementation. - New integration test
packages/core/tests/integration/runtime/after-hooks-deferred.test.ts— it directly verifies scheduling throughafter()rather than just asserting the handler eventually ran. - Changeset (
emdashpatch, present, observable wording). - AGENTS.md conventions: no new logged-out queries, no UI strings, no SQL interpolation, no new indexes or migrations, comments are explanatory rather than reviewer-facing.
Still open (non-blocking): media:afterUpload in handleMediaCreate is still dispatched as a bare promise with .catch(), so plugin work in that hook is subject to the same Workers cancellation. That’s pre-existing and outside this PR’s scope, but worth a follow-up to keep the lifecycle-hook behavior consistent.
No blocking issues. Prior approval stands; LGTM with the one suggestion noted below.
Findings
-
[suggestion]
packages/core/src/emdash-runtime.ts:3139-3141media:afterUploadis still dispatched as a bare promise with.catch(), so any real work a plugin does there can be canceled the moment Workers returns the response. It has the same lifecycle shape ascontent:afterDelete/content:afterUnpublishand should be deferred throughafter()for consistency.if (result.success && this.hooks.hasHooks("media:afterUpload")) { const item = result.data.item; const mediaItem: MediaItem = { id: item.id, filename: item.filename, mimeType: item.mimeType, size: item.size, url: `/media/${item.id}/${item.filename}`, createdAt: item.createdAt, }; after(async () => { try { await this.hooks.runMediaAfterUpload(mediaItem); } catch (err) { console.error("EmDash afterUpload hook error:", err); } }); }
github-actions
Bot
added
review/approved
What does this PR do?
runAfterDeleteHooksandrunAfterUnpublishHooksdispatch plugin hooks fire-and-forget (a bare promise with.catch()), unlikerunAfterSaveHooksandrunAfterPublishHooks, which already wrap their dispatch inafter().On Cloudflare Workers a promise that isn't handed to the host's lifetime extender (
waitUntil, whichafter()wraps) is canceled the moment the HTTP response is returned. So any real work a plugin does incontent:afterDelete/content:afterUnpublish— deleting uploaded files from R2, removing search-index entries, plugin-storage writes — gets killed mid-flight. In practice a half-completed plugin-storage write duringafterDeletecould wedge the storage backend for the isolate and hang every subsequent authenticated admin request (observed as the admin "Move to trash" action freezing the content list).The fix wraps both dispatchers in
after(), identical to the existingafterSave/afterPublishhandling — trusted hook in a try/catch, sandboxed plugins collected intotasksand awaited viaPromise.allSettled.Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (0 diagnostics)pnpm testpasses (full core suite: 4071 passed; the only failures are pre-existing Windows-env issues — file-URL/tar/file-SQLite — that fail identically on a cleanmain)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
New test
tests/integration/runtime/after-hooks-deferred.test.tsdrives a real soft-delete and unpublish throughEmDashRuntimeand asserts the hook work is scheduled viaafter()rather than run inline-and-abandoned. It fails on stockmain(the handler runs inline) and passes with this change.