Fix the possible nullref on completing failed results

[JanKrivanek]

Fixes AB#2172446

Context

The OM unification (#10172) unified check for readiness of BuildSubmission - it newly skipped checking of BuiltRequest not being null - expecting that during submitting the submission the BuiltResult is eventaully set.
This was covering the happy path, however result created from exception might be requested too early - before BuiltRequest is attached (but afet IsStarted was set). So we reintroduced the nun-null check

Changes Made

The removed BuiltRequest null check was reintroduced - it was just pulled to the internals of the BuildSubmission type.

[rainersigwald]

I don't see how this fixes the null ref, can you elaborate on that please?

[JanKrivanek]

I don't see how this fixes the null ref, can you elaborate on that please?

Sure! Happy to clear that out!

I'll use one example (out of 5 usages) of unification of completing submission with exception:

Formerly:

msbuild/src/Build/BackEnd/BuildManager/BuildManager.cs

Lines 818 to 834 in a481b83

	foreach (BuildSubmission submission in _buildSubmissions.Values)
	{
	if (submission.BuildRequest != null)
	{
	BuildResult result = new BuildResult(submission.BuildRequest, new BuildAbortedException());
	_resultsCache.AddResult(result);
	submission.CompleteResults(result);
	}
	}
	foreach (GraphBuildSubmission submission in _graphBuildSubmissions.Values)
	{
	if (submission.IsStarted)
	{
	submission.CompleteResults(new GraphBuildResult(submission.SubmissionId, new BuildAbortedException()));
	}
	}

After refactor:

msbuild/src/Build/BackEnd/BuildManager/BuildManager.cs

Lines 807 to 817 in 1b1df9e

	foreach (BuildSubmissionBase submission in _buildSubmissions.Values)
	{
	if (submission.IsStarted)
	{
	BuildResultBase buildResult = submission.CompleteResultsWithException(new BuildAbortedException());
	if (buildResult is BuildResult result)
	{
	_resultsCache!.AddResult(result);
	}
	}
	}

The code within if was unified properly, however the conditioning wasn't. For graph build there is no change. For non-graph build the IsStarted is set when the submission is just being executed - but the BuildRequest might not have yet been attached. This can lead to unexpected null down in the result creation code, when the result is attempted to be asynchronously finished with exception.
There are 3 possible ways of fixing:

• The IsStarted for non-graph submission would be set only after BuildRequest is guaranteed to be set (there are however multiple codepaths)
• The IsStarted for non-graph submission reflect the state of the BuildRequest being set
• The asynchronous finalization of BuildSubmission with exception would use a lock that is being used in ExecuteSubmission when altering the BuildSubmission

The 2nd option is least risky as it's the smallest codechange plus reflects 1:1 the behavior before refactoring (where the condition checked explicitly the BuildRequest not being null)