WIP/draft Fix passing task exceptions from boss to UI#6983
WIP/draft Fix passing task exceptions from boss to UI#6983rvykydal wants to merge 2 commits intorhinstaller:mainfrom
Conversation
After commit 18a2b60 moved RunInstallationTask to the Boss D-Bus module, non-critical errors (e.g. missing package) were no longer handled properly. The errorHandler in the Boss process has no UI set, so it re-raises exceptions instead of showing a dialog. The error stops the entire task queue and surfaces via Finish() as a generic "unknown error" dialog. Fix by adding a D-Bus error communication mechanism between Boss and UI. A _RemoteErrorUI proxy is set as errorHandler.ui during task queue execution in the Boss process. When an error occurs, it emits an ErrorRaised D-Bus signal and blocks. The UI spoke receives the signal, shows the appropriate dialog via its local errorHandler.ui, and calls AnswerError with the user's response. This unblocks the Boss task queue, which can then continue (ERROR_CONTINUE) or abort (ERROR_RAISE). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add showError to _RemoteErrorUI and a detail_type parameter to the ErrorRaised D-Bus signal so the UI can distinguish between yes/no questions (e.g. NonCriticalInstallationError) and fatal error dialogs (e.g. ScriptError). For fatal errors the UI shows an error dialog with an exit button, matching the behavior of _script_error_handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust mechanism for handling task exceptions and user interaction during the installation process. It enables the backend "boss" process to effectively communicate critical errors to both graphical and text-based user interfaces, allowing users to respond to prompts (e.g., continue or abort) and ensuring that the installation flow can react appropriately to these decisions. This change improves the overall stability and user experience by preventing unhandled exceptions from silently failing the installation and providing clear feedback to the user. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to propagate exceptions from background installation tasks in the 'boss' process to the UI for user interaction. It adds new D-Bus signals and methods for error reporting and response, and a proxy error handler (_RemoteErrorUI) to be used in the non-UI boss process. The changes correctly hook into the GUI and TUI progress screens to display error dialogs.
My review has identified a high-severity issue in the _RemoteErrorUI implementation that could lead to incorrect error handling behavior. I've also pointed out an opportunity to reduce code duplication between the GUI and TUI components for better maintainability. Overall, this is a good foundation for the intended feature, pending the suggested corrections.
| def showDetailedError(self, message, details, buttons=None): | ||
| full_message = message | ||
| if details: | ||
| full_message += "\n\n" + details | ||
| return self._task._show_dialog(full_message, "yesno") |
There was a problem hiding this comment.
The current implementation of showDetailedError always treats a detailed error as a "yes/no" question. This is not always correct. Some callers of showDetailedError (like _storage_install_handler in pyanaconda/errors.py) use it to display a fatal error message without expecting a user choice, while others (like _storage_reset_handler) use it to present a choice with custom buttons.
This implementation will incorrectly show a "yes/no" dialog for fatal errors and will not handle custom button scenarios correctly as it ignores the buttons parameter.
I suggest differentiating based on the presence of the buttons argument. If buttons are provided, it's likely a question that can be mapped to "yes/no". If not, it should be treated as a fatal error display, similar to showError.
| def showDetailedError(self, message, details, buttons=None): | |
| full_message = message | |
| if details: | |
| full_message += "\n\n" + details | |
| return self._task._show_dialog(full_message, "yesno") | |
| def showDetailedError(self, message, details, buttons=None): | |
| full_message = message | |
| if details: | |
| full_message += "\n\n" + details | |
| dialog_type = "yesno" if buttons else "error" | |
| return self._task._show_dialog(full_message, dialog_type) |
| def _on_error_raised(self, message, detail_type): | ||
| """Handle an error that needs user interaction. | ||
|
|
||
| Show the error message dialog and send the user's | ||
| answer back to the Boss process so the installation | ||
| task queue can continue or abort. | ||
|
|
||
| :param message: the error message to display | ||
| :param detail_type: "yesno" for a yes/no question, | ||
| "error" for a fatal error dialog | ||
| """ | ||
| if detail_type == "error": | ||
| errorHandler.ui.showError(message) | ||
| self._task_proxy.AnswerError(False) | ||
| else: | ||
| answer = errorHandler.ui.showYesNoQuestion(message) | ||
| self._task_proxy.AnswerError(answer) |
There was a problem hiding this comment.
The _on_error_raised method is identical to the one in pyanaconda/ui/gui/spokes/installation_progress.py. This code duplication could be avoided to improve maintainability.
Consider creating a mixin class in a shared location that contains this method, and have both ProgressSpoke classes (GUI and TUI) inherit from it.
For example, you could have a mixin like this:
from pyanaconda.errors import errorHandler
class InstallationProgressErrorMixin:
def _on_error_raised(self, message, detail_type):
"""Handle an error that needs user interaction."""
if detail_type == "error":
errorHandler.ui.showError(message)
self._task_proxy.AnswerError(False)
else:
answer = errorHandler.ui.showYesNoQuestion(message)
self._task_proxy.AnswerError(answer)Then, the ProgressSpoke classes could be defined as:
class ProgressSpoke(StandaloneSpoke, InstallationProgressErrorMixin): (for GUI)
class ProgressSpoke(StandaloneTUISpoke, InstallationProgressErrorMixin): (for TUI)
This would ensure that any future changes to this logic only need to be made in one place.
1 participant
This draft suggested by Claude could serve as a base for a solution.