Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 727d7d9 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
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR restarts the workerd process when it crashes after a successful startup, while preserving the existing fail-fast behavior for startup crashes.
Issues (ranked by severity):
-
Unhandled promise rejection on restart failure — The
.catch()handler re-throws the error, but the promise is voided (void this.#runtimeMutex...). This means a failed restart will produce an unhandled promise rejection, which crashes Node.js by default (since Node 15). You should either log the error instead of re-throwing, or store the promise somewhere it can be observed. -
TODO(NOW)left in code — This looks like a development reminder that should be resolved before merging. -
Flaky restart test —
setTimeout(resolve, 0)is almost certainly not enough time for the full restart cycle (mutex acquisition → config assembly → workerd spawn → port ready). This will likely be flaky in CI. Consider pollingmf.readyormf.dispatchFetchwith retries instead. -
Typo — "goit" → "got".
| // can still test async initialisation failures, without test failures | ||
| // telling us to `dispose()` the instance. | ||
| maybeInstanceRegistry?.delete(this); | ||
| throw e; |
There was a problem hiding this comment.
Re-throwing inside .catch() on a voided promise creates an unhandled promise rejection, which crashes Node.js by default. Consider logging the error instead, or not re-throwing:
| throw e; | |
| this.#log.error(e); |
| onWorkerdCrashRestart: () => { | ||
| // workerd crashed after successful startup. Try to restart | ||
| // workerd. I copied this from the constructor. | ||
| // TODO(NOW): What do we do when it fails?? |
There was a problem hiding this comment.
This TODO(NOW) should be resolved before merging — it looks like a development note.
| ).rejects.toThrow(); | ||
|
|
||
| // Wait for workerd to reset | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); |
There was a problem hiding this comment.
setTimeout(resolve, 0) is unlikely to be enough for the full restart cycle (mutex + config assembly + workerd spawn + port listening). This will probably be flaky in CI. Consider awaiting mf.ready instead, which should resolve once the restart completes, or polling dispatchFetch with retries.
7127ca5 to
211410d
Compare
|
I'm Bonk, and I've done a quick review of your PR. This PR restarts the workerd process when it crashes after a successful startup, while preserving the existing fail-fast behavior for startup crashes. I posted a review with 4 issues (ranked by severity):
|
|
File: Issue: Missing body / insufficient description The changeset has only a title with no body paragraph. The README guidelines require:
The current description — "Miniflare now restarts workerd if it crashes in a handler (but not if it crashes at startup)" — is a reasonable title, but it lacks context such as:
A revised changeset might look like: ---
"miniflare": patch
---
Miniflare now restarts workerd if it crashes in a handler (but not if it crashes at startup)
Previously, if workerd crashed while handling a request, Miniflare would not recover and subsequent requests would fail. Miniflare now detects mid-handler crashes and automatically restarts workerd so that local development can continue without needing to manually restart the dev server. Startup crashes are still surfaced as fatal errors.Other checks (pass):
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| void this.#runtimeMutex | ||
| .runWith(() => this.#assembleAndUpdateConfig()) | ||
| .catch((e) => { | ||
| // If initialisation failed, attempting to `dispose()` this instance | ||
| // will too. Therefore, remove from the instance registry now, so we | ||
| // can still test async initialisation failures, without test failures | ||
| // telling us to `dispose()` the instance. | ||
| maybeInstanceRegistry?.delete(this); | ||
| throw e; | ||
| }); |
There was a problem hiding this comment.
🔴 Unhandled promise rejection when crash restart fails
The onWorkerdCrashRestart callback copies the error-handling pattern from the constructor (line 1081-1090) but applies it incorrectly. In the constructor, the promise chain (including the throw e in .catch()) is assigned to this.#initPromise, which is later await-ed in #waitForReady, so the rejection is handled. In the crash restart callback, the same .catch((e) => { throw e }) is used, but the resulting promise is void-ed — no handler is ever attached to it. When the restart fails (e.g., workerd crashes again at startup), the throw e produces a rejected promise with no rejection handler, triggering an unhandledRejection event in Node.js, which by default crashes the process (Node.js 15+). The throw e should be removed or replaced with error logging, since there is no consumer to propagate the error to.
| void this.#runtimeMutex | |
| .runWith(() => this.#assembleAndUpdateConfig()) | |
| .catch((e) => { | |
| // If initialisation failed, attempting to `dispose()` this instance | |
| // will too. Therefore, remove from the instance registry now, so we | |
| // can still test async initialisation failures, without test failures | |
| // telling us to `dispose()` the instance. | |
| maybeInstanceRegistry?.delete(this); | |
| throw e; | |
| }); | |
| void this.#runtimeMutex | |
| .runWith(() => this.#assembleAndUpdateConfig()) | |
| .catch((e) => { | |
| // If initialisation failed, attempting to `dispose()` this instance | |
| // will too. Therefore, remove from the instance registry now, so we | |
| // can still test async initialisation failures, without test failures | |
| // telling us to `dispose()` the instance. | |
| maybeInstanceRegistry?.delete(this); | |
| this.#log.error("Failed to restart workerd after crash:", e); | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| } else { | ||
| // workerd is now listening. Watch for unexpected exits so we can | ||
| // restart. | ||
| const currentProcess = this.#process; | ||
| currentProcess?.once("exit", () => { | ||
| if (this.#process !== currentProcess) { | ||
| // We goit here because dispose() set this.#process to | ||
| // undefined before sending SIGKILL | ||
| return; | ||
| } | ||
| if (abortSignal.aborted) { | ||
| return; | ||
| } | ||
| // Crash: clear stale #process and notify the caller. | ||
| this.#process = undefined; | ||
| options.onWorkerdCrashRestart?.(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Early return in VSCode inspector path skips crash handler registration
When running inside a VSCode Debug Terminal with an inspector socket enabled, if the bootloader path is not found, updateConfig returns early at packages/miniflare/src/runtime/index.ts:279 (return ports). This bypasses the crash handler registration at lines 313-329, meaning workerd crashes after startup will not trigger automatic restarts in this scenario. The exit handler registration should be moved before the VSCode inspector block, or the early return should be refactored to fall through to the exit handler registration.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // We got here because dispose() set this.#process to | ||
| // undefined before sending SIGKILL | ||
| return; |
There was a problem hiding this comment.
Could we just get dispose to remove this handler instead?
|
@hoodmane this is still failing tests, and so I've marked it as draft for now |
The added tests require cloudflare/workerd#6382 for abortIsolate().
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
4a4048f to
727d7d9
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
@hoodmane - this PR has become pretty stale. Shall we just close it until you have time to look into it further? |
|
@matthewp is going to take this on. Closing this one. |
If workerd crashes while starting up, exit as before. If workerd crash happens in a handler, try to restart workerd.
I'm not really sure whether the added logic is correct if restarting workerd fails. On the other hand, starting it the first time worked so hopefully restarting it will work?
The added tests require cloudflare/workerd#6382 for abortIsolate().
cc @penalosa