Implements sawWorkerFatal in RA InvocationEnd event.#6126
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
jasnell
left a comment
There was a problem hiding this comment.
Code Review (AI-generated by Claude)
Note: This review was generated by an AI assistant and may contain inaccuracies. Please use your judgment when evaluating these suggestions.
Overall this is a clean, well-scoped PR. The approach of injecting a JSG module for cross-boundary fatal error reporting follows established patterns in the pyodide integration. A few suggestions below, mostly around robustness of the fatal error path and preserving error context.
Summary of Findings
| Priority | Finding | Effort |
|---|---|---|
| Medium | Use IoContext::tryCurrent() instead of hasCurrent() + current() |
Trivial |
| Medium | Guard against missing IncomingRequest in getMetrics() |
Small |
| Medium | Pass error message through the reporting stack | Medium |
| Low | Clarify on_fatal upstream support |
Trivial |
| Low | Add basic test coverage | Medium |
See inline comments for details.
Questions
- Is there a corresponding Pyodide-side change that makes
Module.API.on_fatalactually get called? If this hook doesn't exist in the bundled Pyodide versions (0.26.0a2 / 0.28.2), the callback registration would be inert until that lands. - Is
sawWorkerFatal/setWorkerFatalalready implemented in the internalMetricsCollector::Request? The open-sourceRequestObserverWithTracerinserver.c++does not override it.
1cc6b4a to
2274bab
Compare
2274bab to
3d8c11a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6126 +/- ##
==========================================
- Coverage 70.61% 70.56% -0.06%
==========================================
Files 409 409
Lines 109295 109537 +242
Branches 18072 18039 -33
==========================================
+ Hits 77179 77291 +112
- Misses 21270 21440 +170
+ Partials 10846 10806 -40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds some observability for fatals coming out of Pyodide.