fix(core): defer fatal handlers outside event dispatch#268
fix(core): defer fatal handlers outside event dispatch#268RtlZeroMemory merged 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDefers certain fatal errors that occur inline during event handling until the onEvent stack unwinds, adds a flush mechanism to run deferred fatals after event processing completes, and adds two tests verifying deferred fatal behavior and that fatal handlers run after event-depth unwinds. Changes
Sequence Diagram(s)sequenceDiagram
participant Emitter as EventEmitter
participant Scheduler as Scheduler
participant App as createApp
participant Fatal as FatalHandler
Emitter->>App: emit(text event)
App->>Scheduler: mark isExecuting / inEventHandlerDepth++
App->>App: run onEvent handler (throws)
App->>App: fatalNowOrEnqueue(fatal)
alt canFailFastInline && inEventHandlerDepth > 0
App-->>App: store deferredInlineFatal
else canFailFastInline && inEventHandlerDepth == 0
App->>Fatal: doFatal (run fatal handlers)
else
App->>Scheduler: enqueueFatal (fallback)
end
App->>Scheduler: inEventHandlerDepth-- (unwind)
App->>App: flushDeferredInlineFatal()
App->>Fatal: execute deferred fatal handlers (dispose allowed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92dfdfb106
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/core/src/app/createApp.ts
Outdated
| const canFailFastInline = | ||
| scheduler.isExecuting && !inRender && !inCommit && inEventHandlerDepth === 0; | ||
| if (canFailFastInline) { |
There was a problem hiding this comment.
Preserve fail-fast when fatal is raised in event handlers
Gating inline fatal dispatch on inEventHandlerDepth === 0 means an onEvent throw now queues fatal for the next turn instead of faulting immediately; while that queued item waits, processTurn continues handling the rest of the current turn (including additional coalesced eventBatch items and commitUpdates/tryRenderOnce) because state is still Running. In turns that already contain multiple queued items, this allows post-error events and state changes to run before the fatal transition, which breaks the previous fail-fast behavior for terminal user-code errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in eeb8e16. Fatal raised during event handlers now defers only until inEventHandlerDepth unwinds, then faults in the same turn before queued updates can commit. Added regression coverage in packages/core/src/app/__tests__/onEventHandlers.test.ts for same-turn queued updater execution and fatal-handler dispose() after onEvent throws.
Summary
Why
CodeRabbit flagged a real runtime hole in merged PR #265: fatal handlers could run while , causing app API calls from fatal handlers to trip re-entrancy guards and get swallowed.
Validation
Summary by CodeRabbit
Bug Fixes
Tests