Handle non-critical installation errors in GUI/TUI progress spokes

[rvykydal]

This draft suggested by Claude could serve as a base for a solution.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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

• D-Bus Error Signaling: Introduced a new D-Bus signal, ErrorRaised, within the CategoryReportTaskInterface to enable the backend 'boss' process to notify the UI about errors requiring user interaction.
• Remote Error UI Proxy: Implemented a _RemoteErrorUI class in the boss process that acts as a proxy for the errorHandler.ui, translating standard UI dialog calls into D-Bus signals for remote handling.
• UI Error Handling Integration: Updated both the GUI and TUI installation_progress spokes to listen for the ErrorRaised D-Bus signal, display appropriate error dialogs to the user, and send the user's response back to the boss process via a new AnswerError D-Bus method.
• Blocking Mechanism for User Response: Utilized threading.Event to implement a blocking mechanism, allowing the boss process to pause execution after emitting an error signal until the UI provides a response, ensuring synchronous user interaction.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

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.

[github-actions]

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

[KKoukiou]

/kickstart-tests --testtype smoke

[rvykydal]

The PR is missing unit tests, which is also apparent from codecov check.

[rvykydal]

I wonder if we should remove the 'Category' from some names and make them more generic (like CategoryReportTaskInterface -> ReportTaskInterface) with the change.

[rvykydal]

Other than the 2 comments (the tests are required I think) looks good to me.

[M4rtinK]

Looks good to me in general, but have some question/suggestions that I think should be addressed, but I don't mind that happening in a followup PR.

[M4rtinK]

Any reason this is named starting with an _ ? IF so, the reason should be noted in a code comment or at least in the commit message.

[KKoukiou]

So that it's not visible as an API outside of the module. Added comment.

[M4rtinK]

IIRC we used to handle this by scoping what gets exported from the module instead with __all__: https://github.com/rhinstaller/anaconda/blob/main/pyanaconda/modules/users/installation.py

__all__ = [
    "ConfigureRootPasswordSSHLoginTask",
    "CreateGroupsTask",
    "CreateUsersTask",
    "SetRootPasswordTask",
    "SetSshKeysTask",
]

Or do you mean something else ?

I checked & we currently have only 2 other classes that start their name with _ so I wondered if we might have a better way already in use:

$ git grep "class _"
pyanaconda/anaconda_logging.py:58:class _AnacondaLogFixer:
pyanaconda/payload/manager.py:49:class _PayloadManager(Runnable, ProgressReporter):

[M4rtinK]

This looks kinda ugly - do I guess correctly that the idea is that we do currently only use the new inter-process error dialog during the installation phase, so we temporarily replace it when the task queue is running ?

Still does not seem like a nice solution & should be at least better documented if we keep it for now.

IMHO better would be to either use the IPC error dialog always or use a new variable in errorHandler ("ui_process_interface" ?) so we don't have to do this dance.

[KKoukiou]

Put an extended comment inline.

[M4rtinK]

Also I assume we have plans how to wire this in to the Web UI as well, right ?

The Web UI will have the same use case where the backend needs to ask the user & block processing in the meantime.

[KKoukiou]

Also I assume we have plans how to wire this in to the Web UI as well, right ?

The Web UI will have the same use case where the backend needs to ask the user & block processing in the meantime.

It's already wired - and tests are passing rhinstaller/anaconda-webui#1325

[KKoukiou]

/kickstart-tests --testtype smoke

[rvykydal]

Still looks good to me.
Regarding the codecov check, not 100% sure if the report is relevant but IIRC we are testing the interfaces somehow (might be wrong). I think someone looking at existing tests could get the answer.

[KKoukiou]

Still looks good to me. Regarding the codecov check, not 100% sure if the report is relevant but IIRC we are testing the interfaces somehow (might be wrong). I think someone looking at existing tests could get the answer.

Added test_error_raised_and_respond_to_error to test_install_category.py, following the existing CategoryChanged pattern. Let's see codecov now.

[KKoukiou]

/kickstart-tests --testtype smoke