Skip to content

Change Pyodide code to ensure all fatals are reported.#6334

Merged
dom96 merged 1 commit intomainfrom
dominik/report-all-fatals-py
Mar 18, 2026
Merged

Change Pyodide code to ensure all fatals are reported.#6334
dom96 merged 1 commit intomainfrom
dominik/report-all-fatals-py

Conversation

@dom96
Copy link
Contributor

@dom96 dom96 commented Mar 16, 2026

The code for this is here: https://github.com/pyodide/pyodide/blob/0.28.2/src/core/error_handling.ts#L92. The on_fatal method is only called when the fatal happens for the first time. This change should ensure it gets called even if the fatal occurred already.

@dom96 dom96 requested review from hoodmane and ryanking13 March 16, 2026 17:21
@dom96 dom96 requested review from a team as code owners March 16, 2026 17:21
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This change patches Pyodide's fatal_error to invoke API.on_fatal(e) in the recursive-call path, ensuring all fatal errors are reported rather than only the first one.

Issues found (ranked by severity):

  1. [MEDIUM] Missing try/catch around on_fatal in the recursive path. In the original non-recursive path, API.on_fatal(e) is called inside a try { ... } catch block. But in this patch, the call is unguarded. If on_fatal itself throws, the return statement won't execute, and fatal_error will continue into the rest of its body, which could cause an infinite recursion or other unexpected behavior.

These comments were generated by an AI code review assistant and may contain errors. Please verify all suggestions before applying.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 16, 2026

Review posted successfully on PR #6334.

Summary: The PR patches Pyodide's fatal_error function to call API.on_fatal(e) during recursive fatal errors (not just the first one). I found one medium-severity issue: the on_fatal call in the new code path is unguarded, unlike the original non-recursive path where it's wrapped in a try/catch. If on_fatal throws, the return won't execute, potentially causing further problems. I posted a suggestion to wrap the call in a try/catch.

github run

ask-bonk bot added a commit that referenced this pull request Mar 16, 2026
Co-authored-by: dom96 <dom96@users.noreply.github.com>
@ask-bonk ask-bonk bot requested a review from a team as a code owner March 16, 2026 17:23
@dom96 dom96 force-pushed the dominik/report-all-fatals-py branch from 72ed3b2 to 10d9ae6 Compare March 16, 2026 17:36
@dom96 dom96 force-pushed the dominik/report-all-fatals-py branch from 10d9ae6 to 34a6e49 Compare March 16, 2026 17:51
@hoodmane
Copy link
Contributor

Running Build Python Runtime workflow on this, we need tests to pass after that workflow pushes an update to this branch.

@dom96 dom96 closed this Mar 17, 2026
@dom96 dom96 reopened this Mar 17, 2026
@dom96 dom96 force-pushed the dominik/report-all-fatals-py branch from 6159738 to 2d87e14 Compare March 17, 2026 14:43
@dom96 dom96 force-pushed the dominik/report-all-fatals-py branch from 2d87e14 to 3057025 Compare March 17, 2026 21:12
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@7143daf). Learn more about missing BASE report.
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6334   +/-   ##
=======================================
  Coverage        ?   70.80%           
=======================================
  Files           ?      420           
  Lines           ?   112589           
  Branches        ?    18447           
=======================================
  Hits            ?    79719           
  Misses          ?    21869           
  Partials        ?    11001           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dom96 dom96 merged commit 580f4b0 into main Mar 18, 2026
22 of 23 checks passed
@dom96 dom96 deleted the dominik/report-all-fatals-py branch March 18, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants