Add support for 'canceling' status for variant analysis

[charisk]

Introduces a new 'canceling' status for variant analyses which is used to show whether we're in the middle of stoping the analysis.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[shati-patel]

I had one question inline, but looks good otherwise! 🖼️

Is it worth adding a storybook example for the "canceling" status? 📖

[shati-patel]

🚢

[shati-patel]

Oh wait, a CHANGELOG entry would indeed be helpful!

[charisk]

Oh wait, a CHANGELOG entry would indeed be helpful!

Cool, I'll update. BTW I won't merge until we hear back from Product. Also I'll wait in case there are more reviews.

[robertbrignull]

I like the approach, but I've seen a few things in the code that don't look quite right to me. Although I haven't tried manually running the code to verify things. Have you manually tested this?

[robertbrignull]

I'm not sure adding this code to onVariantAnalysisUpdated is going to do what we want.

As far as I can tell this method is only called from within cancelVariantAnalysis. Once when we set the status to Canceling and once more if we want to set it back to InProgress.

So it won't help when we get updates from the monitoring job which will try to set the status to InProgress. It will also hinder us because it'll undo the change we're trying to make in the catch block of cancelVariantAnalysis.

I think instead we might want to add this logic to mapUpdatedVariantAnalysis?

What do you think? Have I understood things correctly?

[charisk]

I think you're right. I did a last minute refactor here and didn't think through all the cases I had in mind on Friday.

I'm also having some trouble with testing manually atm - I seem to be testing against older versions of the code somehow. I'm either being forgetful with builds or the watcher/build is a bit off.

I'll move some logic to mapUpdatedVariantAnalysis.

[robertbrignull]

Oops, I missed originally that we pass onVariantAnalysisUpdated into variantAnalysisMonitor.onVariantAnalysisChange, so it was called in other places. But the point still stands about when we want to set that status back when cancelation fails, so the other changes you made are still the right thing.

[charisk]

@shati-patel @robertbrignull this is now ready to review again. While testing I noticed a small bug around the "stats" area so I fixed that (see f0ea640).

I've also done the following manual tests:

• Run a normal variant analysis
• Run a variant analysis and press "Stop query"
• Change the gh-actions-api-client.ts to always throw an error so I can simulate error when canceling, run a variant analysis and press "Stop query".

[charisk]

Note that I had to pass this as a separate arg instead of adding more items to Pick because we want to allow for it to be undefined for the case where we map a variant analysis submission to a variant analysis entity (L30).

I don't love how this function is shaping up in terms of args (specially with the Pick<>) but we could tidy that up a bit separately.

[robertbrignull]

Maybe we should rewrite the exposed interface of this file. Right now we have

mapVariantAnalysis(VariantAnalysisSubmission, ApiVariantAnalysis): VariantAnalysis
mapUpdatedVariantAnalysis(Pick<...>, ApiVariantAnalysis): VariantAnalysis

where mapVariantAnalysis depends on mapUpdatedVariantAnalysis. We could rewrite mapUpdatedVariantAnalysis to be

mapUpdatedVariantAnalysis(VariantAnalysis, ApiVariantAnalysis): VariantAnalysis

and then have a third un-exported method that can have Pick or other ugly things for its args, but it'll be an internal function and not exposed outside the file.

[charisk]

Yeah that's the sort of thing I had in mind (although I'm still not into using Pick and would rather avoid it). Let's play around that in a separate PR.

[robertbrignull]

Could you explain why we call getVariantAnalysisStatus(variantAnalysis.id) to fetch the status from the variant analysis manager instead of using the variantAnalysis.status value we already have?

[charisk]

Because the variantAnalysis.status we already have is one we got from the API so it won't necessarily have the "true" status (which is "canceling"). I should probably at least add a code comment to explain this.

[robertbrignull]

Hmm, I agree this is necessary for now, but I think it's not ideal. I'd like to look at changing it in a followup tech-debt issue.

In the code here, variantAnalysisSummary comes from the API, but the variantAnalysis variable we have here is of the internal type and has gone through mapping / processing. The problem is that it's a variable we keep within this _monitorVariantAnalysis function so it doesn't see any changes that come from outside the monitor. Previously this was fine because the monitor was the only thing that changed the variant analysis state and then it just fed data back to the manager. But now changes can come from other places and the monitor doesn't see these changes and tries to overwrite them.

Adding in getVariantAnalysisStatus works right now, but only because the status field is the only bit that's changing. It could cause problems if any other fields change in other places.

We could make it more general and fetch the whole variant analysis from the manager. We could also rewrite the monitor so it can track any changes by listening to the onVariantAnalysisStatusUpdated event from the manager.

[charisk]

Yeah agree it's an area that could do with improvement. IMO it goes hand in hand with #3405 (comment).

[charisk]

I've added a comment. I would like to make some incremental improvements around this and the mapping functions in separate PRs, if that's okay.

[robertbrignull]

I've also done the following manual tests:

• Run a normal variant analysis
• Run a variant analysis and press "Stop query"
• Change the gh-actions-api-client.ts to always throw an error so I can simulate error when canceling, run a variant analysis and press "Stop query".

Thanks. I've done some manual testing and it's working correctly in all the cases I tried.

[shati-patel]

Poked it again, and things still work for me! Thanks for fixing the bug in the "Stats" header too ⚡

I'll leave @robertbrignull to re-review + approve, since he had more comments than I did 😛

[robertbrignull]

I've left one optional comment on the comment, but I think the code looks fine. I had no other comments other than what I said earlier and we've decided to defer any changes there to followup issues.