fix(api-client, react-api-client, app, robot-server): support multiple recovery policies during a run #16950
Conversation
mjhuff
changed the title
mjhuff
force-pushed
the
support-multiple-ignore-all-policies
branch
2 times, most recently
from
4fc6904 to
be544dc
Compare
mjhuff
force-pushed
the
support-multiple-ignore-all-policies
branch
2 times, most recently
from
9153f5d to
91ed2a5
Compare
We need to do this because it's carrying its own state now.
| # todo(mm, 2024-11-22): Storing the list of error recovery rules is outside the | ||
| # responsibilities of this class. It's also clunky for us to store it like this | ||
| # because we need to remember to clear it whenever the current run changes. | ||
| # This error recovery mapping stuff probably belongs in RunOrchestratorStore. |
There was a problem hiding this comment.
I tried doing this, but then we gotta move the call to error_recovery_mapping.create_error_recovery_policy_from_rules(), too, and it was feeling a bit too churny for 8.2.0.
| return RunDataManager( | ||
| run_orchestrator_store=run_orchestrator_store, | ||
| run_store=run_store, | ||
| error_recovery_setting_store=error_recovery_setting_store, | ||
| task_runner=task_runner, | ||
| runs_publisher=runs_publisher, | ||
| ) | ||
| run_data_manager = _run_data_manager_accessor.get_from(app_state) | ||
|
|
||
| if run_data_manager is None: | ||
| run_data_manager = RunDataManager( | ||
| run_orchestrator_store=run_orchestrator_store, | ||
| run_store=run_store, | ||
| error_recovery_setting_store=error_recovery_setting_store, | ||
| task_runner=task_runner, | ||
| runs_publisher=runs_publisher, | ||
| ) | ||
| _run_data_manager_accessor.set_on(app_state, run_data_manager) | ||
|
|
||
| return run_data_manager |
There was a problem hiding this comment.
RunDataManager needs to be a singleton now that it has some state of its own that needs to carry across different HTTP requests.
In general, this kind of change is not trivially safe. Before, the RunDataManager was picking up fresh values of task_runner, run_orchestrator_store, etc. on every HTTP request, and now it's not, and it's possible for that to affect behavior. I don't see anything where it would matter in this case, but double-check my thinking.
mjhuff
requested review from
ncdiehl11,
sfoster1 and
a team
and removed request for
a team and
ncdiehl11
|
Tested on a protocol with different kinds of ignorable errors, confirmed it's working: 
|
Overview
Closes RQA-3670
We need to support the client ignoring several different classes of errors in a single run, which means the app needs to know the current recovery policy and be able to modify. This PR adds the necessary infrastructure to support that.
Test Plan and Hands on Testing
Changelog
On the server, add a new
GET /runs/{id}/errorRecoveryPolicyendpoint to complement the existingPUTendpoint. This turns out to be a little annoying because, so far, we have not actually been preserving that information, and we didn't have a super clean place to add it. This PR does it in a quick and dirty way.On the client, use that to let the "ignore all errors of this type" button do a read-modify-write sequence, where it reads the current errors we're ignoring, appends the new one to the list, and writes the new list.
Review requests
See comments below.
Risk assessment
Medium? See comment below about
RunDataManagerbeing a singleton now.