fix(cloudflare): resolve AI Search bindings per request - #2404
Conversation
🦋 Changeset detectedLatest commit: 1f874f4 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: |
There was a problem hiding this comment.
The approach is correct. Caching the cloudflare:workers module import at isolate scope is unsafe because that module is request-scoped on Workers; the resolved env object can leak across requests. Removing the global Symbol.for(...) cache so the plugin re-imports it per call fixes that. The removed waitUntil wrapper is also redundant: I traced the hook dispatch in packages/core/src/emdash-runtime.ts and confirmed that content hooks are invoked inside after() (packages/core/src/after.ts), which already hands the promise to the adapter's waitUntil on Cloudflare. So indexing work remains background-extended without the plugin duplicating the effort.
The diff is clean and focused, the changeset uses the right package and describes the user-visible effect, and no stale exports or helpers are left behind.
The one issue is the new tests. They assert that info() and namespace.get() are called twice across two operations, but that's also true of the pre-fix code — the old code cached the module import, not the binding or instance. Because the Vitest mock returns a static fakeEnv, a reintroduced module-level import cache would still pass these tests. A regression test for request-scoped bindings should make import("cloudflare:workers") return distinct env snapshots per call and assert the second operation reads from the second snapshot.
There was a problem hiding this comment.
Pull request overview
This PR fixes AI Search behavior in the Cloudflare Worker runtime by ensuring Cloudflare bindings are resolved fresh (rather than being cached at the isolate level) and by relying on EmDash’s existing deferred-hook lifetime management instead of duplicating waitUntil usage inside the plugin.
Changes:
- Removed isolate-level caching for
import("cloudflare:workers")in the AI Search plugin so bindings can be resolved per operation/request. - Removed the plugin’s internal
waitUntilanchoring for index sync operations (EmDash core already defers/anchors content hooks viaafter()). - Added tests and a changeset documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cloudflare/src/plugins/ai-search.ts | Drops isolate-level caching of cloudflare:workers and removes redundant waitUntil anchoring for content hook index sync. |
| packages/cloudflare/tests/plugins/ai-search-indexing.test.ts | Adds assertions intended to verify fresh binding-backed instance resolution across operations/route checks. |
| .changeset/fresh-ai-search-request-bindings.md | Patch changeset for @emdash-cms/cloudflare describing the binding-resolution fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
7b6d09d to
4ee8d7f
Compare
There was a problem hiding this comment.
The approach is still correct: resolving cloudflare:workers per request instead of caching the module import at isolate scope is the right fix for request-scoped binding leakage, and the waitUntil wrapper is indeed redundant because the runtime already defers content hooks through after().
I re-checked the new regression test against the prior review feedback and it now validates the actual failure mode: vi.doMock swaps the env snapshot between the two simulated requests, and the second indexing operation is asserted to write into the second request's binding. If the old isolate-level module cache were reintroduced, the second operation would reuse the first request's env and the assertion would fail.
I also verified:
- No stale references to
WORKERS_MODULE_KEY,cfWaitUntil, orwaitForSyncremain. - All content hooks in
packages/core/src/emdash-runtime.tsare still dispatched insideafter(), so index/deindex work keeps being handed to the host's lifetime extender without the plugin duplicating it. - The changeset is still package-appropriate and user-facing.
No blocking issues. The previous needs_fixing test concern is resolved; the rest of the diff is clean.
khoinguyenpham04
left a comment
There was a problem hiding this comment.
pls replace this with a workerd test using withEnv() and the same plugin/module instance across two calls. It must fail on main and pass here. Also, do avoid vi.doMock(), since that doesn’t match Workers behavior.
e8fbe96 to
1f874f4
Compare
There was a problem hiding this comment.
The approach is still correct: resolving cloudflare:workers per request instead of caching the module import at isolate scope fixes request-scoped binding leakage, and removing the plugin-level waitUntil wrapper is safe because EmDash's runtime already defers content hooks through after().
I re-checked the PR head statically and confirmed:
- No stale references to
WORKERS_MODULE_KEY,cfWaitUntil,waitForSync,loadWorkersModule, orgetCloudflareEnvremain inpackages/cloudflare. envis imported directly from"cloudflare:workers"per AGENTS.md convention, and binding resolution is now synchronous and request-scoped.packages/core/src/emdash-runtime.tsstill dispatchescontent:afterSave,content:afterDelete, and the publish/schedule/restore hooks insideafter(), so lifetime extension happens via the runtime rather than the plugin duplicating it.- The new
packages/cloudflare/tests/workerd/ai-search-request-scope.test.tsexercises the actual regression: it creates one plugin instance, swaps theenvsnapshot across twowithEnvblocks, and asserts each simulated request writes to its own binding. If the old isolate-level module cache were reintroduced, the second request would reuse the first request's env and the assertion would fail. - The changeset targets the right package with a user-facing description, and
@cloudflare/vitest-pool-workersis appropriately added to devDependencies with the catalog version bump.
No blocking issues. The prior needs_fixing test concern is resolved and the rest of the diff remains clean.
What does this PR do?
Fixes AI Search operations across consecutive Worker requests by resolving request-scoped Cloudflare bindings each time. It removes isolate-level caching of the
cloudflare:workersmodule and removes complexity.Also removes the unneeded waitUntil, since EmDash already does this for the hooks.
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; this is a non-visual runtime fix.