fix(miniflare): Fix local scheduled handler returning exception when assets is enabled#13704
fix(miniflare): Fix local scheduled handler returning exception when assets is enabled#13704ktKongTong wants to merge 1 commit into
exception when assets is enabled#13704Conversation
🦋 Changeset detectedLatest commit: 5990537 The changes in this PR will be included in the next version bump. 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
|
exception when assets is enabled
| async scheduled(controller: ScheduledController) { | ||
| await this.env.USER_WORKER.scheduled({ | ||
| scheduledTime: new Date(controller.scheduledTime), | ||
| cron: controller.cron, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 RPCProxyWorker discards scheduled outcome, silently swallowing user worker errors
The scheduled handler calls this.env.USER_WORKER.scheduled(...) but ignores the returned FetcherScheduledResult. The Fetcher.scheduled() API doesn't throw on handler failures — it returns { outcome: "exception" } (as evidenced by packages/miniflare/src/workers/core/scheduled.ts:14-15 which checks result.outcome). Because the RPCProxyWorker's handler completes without throwing, workerd reports the outer dispatch as { outcome: "ok" }. This means when assets are enabled and the user worker's scheduled handler throws an exception, handleScheduled will return a 200 "ok" response instead of a 500 "exception" response.
| async scheduled(controller: ScheduledController) { | |
| await this.env.USER_WORKER.scheduled({ | |
| scheduledTime: new Date(controller.scheduledTime), | |
| cron: controller.cron, | |
| }); | |
| } | |
| async scheduled(controller: ScheduledController) { | |
| const result = await this.env.USER_WORKER.scheduled({ | |
| scheduledTime: new Date(controller.scheduledTime), | |
| cron: controller.cron, | |
| }); | |
| if (result.outcome !== "ok") { | |
| throw new Error(`User worker scheduled handler returned: ${result.outcome}`); | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
petebacondarwin
left a comment
There was a problem hiding this comment.
Is this the best fix? It feels like there should be a more general purpose solution that would cover all the other non-fetch entry-points too.
| fetch() { | ||
| return new Response(scheduledRun); | ||
| }, | ||
| scheduled() { |
There was a problem hiding this comment.
we should check that we are passing through the correct parameters here.
|
@ktKongTong - would you like to work on this PR further? |
|
@petebacondarwin Thanks for the ping. I’d still like to continue with this PR. Sorry for the delay. I’ll take another look and update the PR. |
|
I spent some time testing this against the current implementation. The simple forwarding approach in this PR does trigger the user worker’s scheduled() handler, but it doesn’t behave like a transparent scheduled dispatch. Since the call goes through the assets RPC proxy, // user worker
//...
scheduled(controller) {
controller.noRetry();
}the // expect
{
"outcome": "ok",
"noRetry": true
}
//actual
{
"outcome": "ok",
"noRetry": false
}I can only observe the expected result if the proxy explicitly yield back to the runtime, which feels fragile. async scheduled(controller: ScheduledController) {
await this.env.USER_WORKER.scheduled({
cron: controller.cron,
scheduledTime: new Date(controller.scheduledTime),
noRetry: () => controller.noRetry(),
});
await new Promise<void>((resolve) => { setTimeout(resolve, 0) })
}so I create another PR: #14177, which bypass the assets' rpc-proxy worker for scheduled handler. |
Fixes #9882
add scheduled handler for asset's rpc-proxy worker
A picture of a cute animal (not mandatory, but encouraged)