Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(app): Support "resume from recovery assuming false positive" action #16601

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Oct 24, 2024

Closes EXEC-791

Overview

See #16556 and the linked ticket for details concerning the reasoning for implementing these changes.

When skipping a step in Error Recovery, the FE essentially calls two commands: handleIgnoringErrorKind, which deals with updating the recovery policy, and skipFailedCommand, which does the skipping. By using isAssumeFalsePositiveResumeKind, we can conditionally determine which policy ifMatch criteria to use, and we can determine which error recovery resume kind to dispatch to the server.

Test Plan and Hands on Testing

Review requests

  • The new model for determining policy and resume type actions is now predicated on the errorKind as utilized in isAssumeFalsePositiveResumeKind. See the linked ticket, which leads me to think this line of thinking is true. This assumption gets us through 8.2, but is it reasonably sufficient going forward? I think it is, but curious if anyone else has thoughts.

Risk assessment

mediumish. This effectively impacts every route in some way, but it should only directly change the behavior of tip handling error flows.

@mjhuff mjhuff requested a review from a team October 24, 2024 20:22
@mjhuff mjhuff requested a review from a team as a code owner October 24, 2024 20:22
@mjhuff mjhuff requested review from jerader and removed request for a team and jerader October 24, 2024 20:22
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

lgtm!

@mjhuff mjhuff added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Oct 24, 2024
@mjhuff
Copy link
Contributor Author

mjhuff commented Oct 24, 2024

Will merge after #16556 merges

@SyntaxColoring
Copy link
Contributor

  • The new model for determining policy and resume type actions is now predicated on the errorKind as utilized in isAssumeFalsePositiveResumeKind. See the linked ticket, which leads me to think this line of thinking is true. This assumption gets us through 8.2, but is it reasonably sufficient going forward? I think it is, but curious if anyone else has thoughts.

How does this handle the case where:

  1. A pickUpTip command fails
  2. You enter error recovery mode
  3. You select "Retry picking up tip"

We want the client to POST a retrying fixit command and then POST resume-from-recovery, but from skimming the code, it seems like the errorKind predicate will make it POST resume-from-recovery-assuming-false-positive instead?

@mjhuff
Copy link
Contributor Author

mjhuff commented Oct 24, 2024

How does this handle the case where...

All of the retry flows currently utilize retryFailedCommand, sometimes followed by a resumeRun, which always issues resumeRunFromRecovery unconditionally. While the handleIgnoringErrorKind is wired up to resumeRun, there's currently no "ignore error and retry step" path - this existed at one point in earlier iterations of ER but effectively never occurs currently, so we are covered here.

If it's helpful, we can test your specific flow to confirm this is true in practice before merging!

EDIT: Ran the above sequence of steps, can confirm we are issuing resume-from-recovery.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Tested and all is well!

@mjhuff mjhuff removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Oct 28, 2024
@mjhuff mjhuff merged commit 8a66ea3 into edge Oct 28, 2024
36 checks passed
@mjhuff mjhuff deleted the app_support-resume-from-recovery-assuming-false-positive branch October 28, 2024 14:29
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.

3 participants