[miniflare] Add backend resources for email capture and storage - #15064
[miniflare] Add backend resources for email capture and storage#15064tpmmorris wants to merge 12 commits into
Conversation
🦋 Changeset detectedLatest commit: 2db167c The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| rawBase64: bytesToBase64(rawEmailBuffer), | ||
| }); | ||
|
|
||
| this.ctx.waitUntil( |
There was a problem hiding this comment.
🟡 Emails sent just before the dev session shuts down are never written to disk or logged
The on-disk copy of a sent email and its log line are queued to run in the background (this.ctx.waitUntil(...) at packages/miniflare/src/workers/email/send_email.worker.ts:370 and :451) instead of being finished before the send call returns, so a script that sends an email and then immediately shuts the local dev session down loses the saved message entirely.
Impact: Short-lived usages (for example sending through getPlatformProxy() and then disposing) no longer reliably produce the .eml/text/HTML/attachment files or the "send_email binding called..." log they used to.
Why the deferred work can be dropped
Before this change send() awaited every storeTempFile() call and logged before resolving, so by the time the caller's await env.SEND_EMAIL.send(...) returned the files existed. Now both branches resolve immediately after the in-workerd capture, deferring the loopback /core/store-temp-file writes and logging to ctx.waitUntil.
Miniflare#dispose() aborts, stops the loopback server and tears down workerd before drainEmailArtifactManager() runs (packages/miniflare/src/index.ts:3490-3496); drain() only awaits operations that already reached the Node side (packages/miniflare/src/plugins/email/artifacts.ts:86-89), so waitUntil work that has not yet issued its loopback request is simply discarded.
The test updates in this PR reflect the new asynchrony (the miniflare email specs now poll with vi.waitFor, and the get-platform-proxy e2e no longer asserts on the file), but callers that dispose right after sending have no way to wait.
Was this helpful? React with 👍 or 👎 to provide feedback.
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7fccbb7 to
494e214
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Please address the bugs highlighted by Devin
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
emily-shen
left a comment
emily-shen
left a comment
There was a problem hiding this comment.
just a quick first pass on the API surface
| "tags": ["Local Explorer"] | ||
| } | ||
| }, | ||
| "/email/routing": { |
There was a problem hiding this comment.
so because these have different semantics to the actual endpoints, we need to stick these in the local namespace. otherwise it will cause confusion between the 'real' api and this one
(https://developers.cloudflare.com/api/resources/email_routing/methods/get)
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "description": "Deliver the test email to this worker's email() handler, regardless of address-based routing." |
There was a problem hiding this comment.
what do you mean by address-based routing? the port? that won't hold because there can be multiple workers per port. you could either use the worker name as part of the path or make this required
| }, | ||
| "outcome": { | ||
| "type": "string", | ||
| "enum": ["ok", "exception"], |
There was a problem hiding this comment.
hmmm. as in you would want a 200 exception if the handler intentionally threw?
task failed successfully i guess 😅
There was a problem hiding this comment.
Yeah this is a weird situation because the message was sent successfully, just the worker itself didn't like it. This is a confusing way to express it but I felt using a non 200 status would make it seem like the send itself failed. I'll make it clearer in the docs whats actually happening, unless youd have a different preference to how its handled?
| "tags": ["Email"] | ||
| } | ||
| }, | ||
| "/email/routing/{email_id}": { |
There was a problem hiding this comment.
could this be a query param on GET /email/routing above
There was a problem hiding this comment.
The list and detail items have a different shape so i thought it would be appropriate to keep separate, if not I can make this change?
| "tags": ["Email"] | ||
| } | ||
| }, | ||
| "/email/sending/{email_id}": { |
There was a problem hiding this comment.
similarly to above, query param on the main list endpoint?
| "properties": { | ||
| "type": { | ||
| "type": "string", | ||
| "enum": ["received", "forward", "reply", "reject", "unhandled"], | ||
| "description": "The kind of event." | ||
| }, | ||
| "timestamp": { | ||
| "type": "string", | ||
| "description": "ISO 8601 timestamp of when the event occurred." | ||
| }, | ||
| "messageId": { | ||
| "type": "string", | ||
| "description": "Present on `forward`/`reply` events; correlates with the matching `forwards`/`replies` entry." | ||
| } |
There was a problem hiding this comment.
this could be a discriminated union to show that messageId is only available when the type is forward or reply
| }, | ||
| "headers": { | ||
| "type": "array", | ||
| "description": "Headers added to the forwarded message, as [key, value] pairs.", |
There was a problem hiding this comment.
why not an array of objects?
| description: | ||
| "Worker whose email() handler processed the message, if known.", | ||
| }, | ||
| from: { type: "string", description: "Envelope MAIL FROM address." }, |
There was a problem hiding this comment.
there's quite a lot of repetition in these objects with from/to/subject/messageId etc.
could you define an object like 'base_email' and extend that?
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const canContinue = candidate.hasMore && candidate.nextCursor !== undefined; | ||
| state[source] = canContinue ? candidate.nextCursor : null; | ||
| if (canContinue) { | ||
| hasMore = true; | ||
| } | ||
| if (items.length < options.query.per_page && canContinue) { | ||
| await getCandidate(source); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Email list in the inspector can offer a "next page" that turns out to be empty
When emails are combined from several dev instances, the list can be marked as having more results (hasMore = true at packages/miniflare/src/workers/local-explorer/resources/email.ts:264-266) even after the follow-up lookup finds nothing left to show, so the inspector offers another page that comes back empty.
Impact: Users paginating the Email tab can be shown a "load more" affordance that yields zero emails.
Mechanism: hasMore is latched from the consumed candidate before the refill attempt is evaluated
In listAggregatedEmails (packages/miniflare/src/workers/local-explorer/resources/email.ts:207-282), after an item is emitted the code sets hasMore = true whenever candidate.hasMore && candidate.nextCursor !== undefined. It then calls getCandidate(source) again; if that refill returns undefined (e.g. every remaining record on that source is filtered out by the worker query parameter, or the peer became unreachable), state[source] is set to null and no candidate is added. If no other source still has a candidate, the loop exits with candidates.size === 0 but hasMore still true, so a cursor is returned. The subsequent request resolves every source to null and returns items: [], has_more: false.
A correct signal would be derived after the refill attempt (i.e. hasMore = candidates.size > 0 plus any source with a non-null state that still has unexamined records).
Prompt for agents
In listAggregatedEmails (packages/miniflare/src/workers/local-explorer/resources/email.ts), `hasMore` is set to true as soon as a consumed candidate reports it could continue, before the follow-up getCandidate() call has established whether any further *matching* item actually exists for that source. When the refill finds nothing (all remaining records filtered out by the `worker` filter, or the peer stopped responding), the source's state is set to null and it is removed from `candidates`, yet `hasMore` stays true and a cursor is emitted. The client then fetches one more page that is guaranteed to be empty. Consider computing the final `hasMore` from the post-loop state instead: true only if `candidates.size > 0`, or if a source was left with a non-null cursor that was never re-probed (i.e. the loop stopped because the page filled up).
Was this helpful? React with 👍 or 👎 to provide feedback.
4 participants
Add backend resources for Email interaction within Local Explorer
Fixes #13648
Adds storage and capture methods for emails sent from/received by a worker using durable objects for storage (in line with the new 'Observability' tab), and cdn-cgi endpoints to mimic the sending of an email to a worker. Also records actions taken by the
email()handler (received,forwarded,replied,rejected,unhandled), so that they can be mapped and displayed in local explorer in a similar manner as the dash. The Email result interface has been updated to also include a list of events.