[miniflare] Surface workerd fatal crash reports instead of filtering them out - #14989
Conversation
…them out When workerd crashes, kj writes a banner (e.g. `*** std::terminate() called with no exception`) followed by a `stack:` line straight to stderr. The stack line matched the hex-stack pattern used to filter ordinary workerd noise, so it was discarded and the crash was left with no context at all. Keep the `stack:` line and the missing-$LLVM_SYMBOLIZER notice when they follow a fatal crash banner, and report the whole thing at `error` level. The wrapped log handler is now created once per stream rather than once per chunk, since the banner and its stack trace can arrive in separate `data` events. Also warn when the runtime is restarted after a crash. Recovery was silent, which made a crash look like an unexplained dev server restart; the crash count distinguishes a one-off from a restart loop.
🦋 Changeset detectedLatest commit: 843e6da 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 |
|
I've completed a thorough review. The changes are well-designed:
No logic bugs, security issues, backward-compatibility violations, or incorrect API behavior found. The edge cases I considered (a non-crash structured log whose message begins with LGTM |
@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: |
|
✅ All changesets look good |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
3b07ae1 to
843e6da
Compare
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Describe your change...
fixtures/dev-registryhas been failing on Windows CI at a high rate. Across 19 recenttest-and-checkruns where the fixture actually executed (it is often a turbo cache hit), the Windows fixtures job failed on it 7 times (37%), and a workerd crash was present in 11 (58%). macOS and Linux: 0.The failure is always the same test —
Dev Registry: vite dev <-> vite dev > supports exported handler fetch over service binding— and alwaysTest timed out in 50000mson both attempts. Every failing log contains this in exactly one of thevite devchild processes:That is workerd aborting,
#handleWorkerdCrashrespawning it, and the Vite plugin'sunsafeHandleRuntimeRestartrestarting the dev server — which then callssetOptions()and respawns workerd a second time. It costs ~30s per crash, which is what eats the test budget. (Notably it always hits the first-started session in the test, i.e. the one on port 5173, whichever config that happens to be.)The problem is that the crash carries no diagnostic information, so there is nothing to act on.
messageClassifiers.isInternaldiscards anything matching/stack:( (0|[a-f\d]{4,})){3,}/— a pattern added for Windows hex stacks — and kj emits the crash stack as a separate line immediately after the banner. So the stack was being thrown away.This PR fixes that, ahead of fixing the crash itself:
*** std::terminate(),*** Fatal uncaught,*** Received signal,*** Uncaught exception) and keep thestack:line and missing-$LLVM_SYMBOLIZERnotice that follow one, reporting the whole report aterrorlevel. Address-in-use and access-violation handling keep priority, so their existing messages are unchanged.dataevents.Before / after, from
test/index.spec.ts:This is a user-facing improvement in its own right, and it has already done its job here. With a temporary diagnostic on top (since reverted) the Windows fixtures job captured the crash for the first time:
Two things fall straight out of that, neither of which was visible before:
BaseThreadInitThunk/RtlUserThreadStartwith CRT and ntdll unwind frames on top, i.e. a C++ exception escaping a workerd worker thread, and it contains a repeated frame (…c416twice) so something re-enters.setRegistrypushes; later sessions boot with the registry already baked into their config. That asymmetry is the live lead.Note on reading the excerpt above: the test helper accumulates stdout and stderr into separate buffers and the diagnostic printed them one after the other, so the relative order of the Vite line and the crash lines there is an artifact of concatenation, not real interleaving. (This change is what moved the banner from stdout to stderr, by reclassifying it to
error.) Ordering has to be read from pre-change logs, where both appear on stdout — and those show the first crash arriving before any Vite restart, with theserver restarted.in between being the crash recovery itself.Follow-up work will fix the crash and reduce the recovery cost (one crash currently costs two workerd spawns plus a full Vite restart, ~30s, with no backoff).
A picture of a cute animal (not mandatory, but encouraged)
Note
This is a contribution from an AI agent: OpenCode, claude-opus-5.