Fix duplicate runtime pack crash in dotnet test device deployment

[jonathanpeppers]

Fixes the dotnet test crash reported in dotnet/android#12254.

The bug

dotnet test against a device project (e.g. Android) failed with only a generic message:

Running the ComputeRunArguments target to discover run commands failed for this project. Fix the errors and warnings and run again.

The real error was visible only in a binlog:

Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(411,5): error MSB4018:
The "ResolveFrameworkReferences" task failed unexpectedly.
System.ArgumentException: An item with the same key has already been added. Key: Microsoft.NETCore.App
   at Microsoft.NET.Build.Tasks.ResolveFrameworkReferences.ExecuteCore()

Root cause

SolutionAndProjectUtility.DeployAndGetRunProperties built DeployToDevice and then ComputeRunArguments against the same ProjectInstance.

MSBuild <Output ItemName="..."/> appends, and ProjectInstance.Build() is a self-contained BuildManager session that does not reset instance state between calls. So every target shared by both builds contributed its items a second time:

1. ProcessFrameworkReferences appended 4 more @(TargetingPack) items (4 → 8).
2. ResolveFrameworkReferences's GetPackageDirectory Items="@(TargetingPack)" appended its 8 outputs onto the 4 @(ResolvedTargetingPack) items already present → 12, containing Microsoft.NETCore.App three times.
3. ResolveFrameworkReferences.ExecuteCore()'s ResolvedTargetingPacks.ToDictionary(tp => tp.ItemSpec, ...) threw.

Confirmed from the CI binlog — three MSBuild submissions on the same project, the third reusing the second's instance:

#	Entry point	ResolvedTargetingPack seen / added
1	ComputeAvailableDevices (own instance, via RunCommandSelector)	4 / 4
2	DeployToDevice	4 / 4
3	ComputeRunArguments — same instance as #2	12 💥

dotnet run does not hit this because it never shares an instance across builds — #52046 fixed exactly this class of bug in RunCommandSelector.OpenProjectIfNeeded, and ComputeRunArguments is built on a separately loaded instance in RunCommand.

The fix

Deploy against a fresh ProjectInstance so its item state cannot leak into the ComputeRunArguments build, matching dotnet run:

// Create a fresh ProjectInstance for each build operation
// to avoid accumulating state (existing item groups) from previous builds
if (!project.DeepCopy().Build([Constants.DeployToDevice], loggers))

ComputeRunArguments still builds the original instance, which is what RunProperties.FromProject and EnvironmentVariablesToMSBuild.ReadFromItems read from afterwards. ProjectInstance.DeepCopy() is the ProjectInstance-level analogue of Project.CreateProjectInstance() used by dotnet run (both are snapshots of an already-evaluated project, not re-evaluations); it also preserves the @(RuntimeEnvironmentVariable) items that EnvironmentVariablesToMSBuild.AddAsItems adds before the deploy build. dotnet watch uses DeepCopy() for the same reason.

Test coverage

Following the same approach as #52046, there is no bespoke regression test — instead DotnetTestDevices.csproj now runs the real ResolveFrameworkReferences task, which makes the existing device tests catch the bug.

Two asset changes are needed. The first mirrors DotnetRunDevices.csproj exactly:

<!-- ResolveFrameworkReferences mimics Android -->
<Target Name="DeployToDevice" DependsOnTargets="ResolveFrameworkReferences">

The second has no counterpart in #52046, and is required. dotnet run's accumulating pair was ComputeAvailableDevices + DeployToDevice (both through RunCommandSelector), whereas dotnet test's pair is DeployToDevice + ComputeRunArguments. The SDK's ComputeRunArguments target is empty with no DependsOnTargets, so without a hook the second build runs nothing and nothing accumulates:

<!-- Unlike dotnet run, dotnet test builds DeployToDevice first and ComputeRunArguments second,
     so ResolveFrameworkReferences has to run in the second build too to mimic Android. -->
<Target Name="_ComputeRunArgumentsDependsOnResolveFrameworkReferences"
        BeforeTargets="ComputeRunArguments"
        DependsOnTargets="ResolveFrameworkReferences" />

This is faithful to Android: the CI binlog shows the real ComputeRunArguments submission running the full framework-reference chain.

Verified red → green

All three combinations were measured against GivenDotnetTestSelectsDevice:

Test asset	CLI	Result
DeployToDevice DependsOnTargets only, no ComputeRunArguments hook	unfixed	29 passed — bug not caught
Both asset changes	unfixed	13 failed, 16 passed
Both asset changes	fixed	29 passed

The 13 tests that the DotnetTestDevices.csproj change causes to fail without the CLI fix, and which the SolutionAndProjectUtility.cs change fixes:

• ItRunsTestsWithSpecifiedDevice("test-device-1")
• ItRunsTestsWithSpecifiedDevice("test-device-2")
• ItAutoSelectsSingleDevice
• ItAutoSelectsSingleDevicePerTfm
• ItRunsWithDeviceAndFramework
• ItRunsDeviceProjectsInSolution
• ItAcceptsDeviceViaMSBuildProperty
• ItPassesEnvironmentVariablesToBuildDeployAndRunArgumentsTargets
• ItCallsDeployToDeviceTargetWhenDeviceIsSpecified
• ItCallsDeployToDeviceTargetWhenDeviceIsAutoSelected
• ItCallsDeployToDeviceTargetEvenWithNoBuild
• ItDeploysBeforeComputingRunArguments
• ItDeploysEveryTargetFramework

GivenDotnetRunSelectsDevice also passes 23/23.

End-to-end validation

Validated on a real Android device (dotnet new androidtest against SDK 11.0.100-preview.7.26376.106 with the released Microsoft.Android.Sdk.Windows 37.0.0-preview.6.59, physical device attached):

	Before	After
dotnet test	fails in ~10s with the ComputeRunArguments message	gets past ComputeRunArguments, deploys the APK, and runs am instrument on device

logcat confirms MSTest actually starts on device. Note there is a separate, unrelated downstream failure once the test host is running (NotSupportedException: Running tests in any of the provided sources is not supported for the selected platform) which reproduces against the released Android workload and is being tracked on the dotnet/android side — this PR fixes the item-accumulation crash, it does not claim dotnet test fully succeeds end-to-end on Android yet.

Notes

• ResolveFrameworkReferences is deliberately not hardened to tolerate duplicates. De-duplicating there would mask doubled item state that every downstream @(ResolvedRuntimePack) / @(ResolvedTargetingPack) consumer would still see, so a recurrence should keep failing loudly.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
2 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Fixes a dotnet test crash during device deployment (e.g., Android) caused by reusing the same ProjectInstance for multiple MSBuild Build() calls, which accumulates item state and can introduce duplicate runtime pack entries.

Changes:

• Build DeployToDevice using a fresh ProjectInstance (DeepCopy()) to prevent item accumulation between DeployToDevice and ComputeRunArguments.
• Update the DotnetTestDevices test asset to execute ResolveFrameworkReferences during both deploy and run-argument computation, so existing device tests regress this failure mode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
test/TestAssets/TestProjects/DotnetTestDevices/DotnetTestDevices.csproj	Makes the test asset mimic Android by running ResolveFrameworkReferences during DeployToDevice and before ComputeRunArguments.
src/Cli/dotnet/Commands/Test/MTP/SolutionAndProjectUtility.cs	Avoids ProjectInstance state leakage by running the deploy build on project.DeepCopy() before building ComputeRunArguments.

[Evangelink]

Reviewed this over three passes — fix correctness, test asset and coverage, and repo-wide consistency — and re-validated every finding against MSBuild's own source and the device tests before writing it up. The fix is correct. Approving. Everything below is non-blocking.

What I verified

DeepCopy() is the right primitive here. Checked against the MSBuild this repo actually consumes (18.10.0-1.26363.117, eng/Version.Details.xml): DeepCopy() calls DeepCopy(_isImmutable) and the clone keeps the mutability of the original. Every ProjectInstance that reaches DeployAndGetRunProperties comes from ProjectInstance.FromFile(...) with ProjectOptions that never set ProjectInstanceSettings, so it is always mutable — there is no path where DeepCopy() degenerates into returning this and Build() then trips VerifyThrowNotImmutable. The copy constructor deep-clones properties, items and global properties while sharing the immutable _targets, Toolset, TaskRegistry and ProjectRootElementCache, and _hostServices is preserved, so the deploy build itself behaves exactly as it did before. Neither DeepCopy overload carries [RequiresUnreferencedCode]/[RequiresDynamicCode], so no new trim/AOT surface, and this file is not in AotSourceFiles.props.

The ordering relative to AddAsItems matters, and it is right. EnvironmentVariablesToMSBuild.AddAsItems runs before the copy, so @(RuntimeEnvironmentVariable) is carried into the deploy build. That is pinned by ItPassesEnvironmentVariablesToBuildDeployAndRunArgumentsTargets, which asserts FOO=BAR appears in the DeployToDevice output — it would fail if the copy ever stopped carrying items. Worth noting because a re-evaluation (CreateProjectInstance/LoadProject) instead of DeepCopy() would have silently dropped them.

The state-flow concern doesn't hold up. The obvious risk is that something a real device SDK's DeployToDevice sets no longer reaches ComputeRunArguments. But dotnet run computes run arguments on an instance that never ran DeployToDevice (RunCommand.EvaluateProject loads its own instance; deploy happens on RunCommandSelector.OpenProjectIfNeeded's), and RunProperties.FromProject reads only RunCommand, RunArguments, RunWorkingDirectory, RuntimeIdentifier, DefaultAppHostRuntimeIdentifier and TargetFrameworkVersion. If any of those depended on a prior deploy, dotnet run would already be broken on Android/iOS. This moves dotnet test onto the same contract rather than off it. dotnet watch reaches for DeepCopy() for the same reason (EvaluationResult.cs, CompilationHandler.cs).

No double deploy, no residual accumulation. TrySelectDevice only builds ComputeAvailableDevices; TryDeployToDevice is reachable only from RunCommand. GetProjectProperties creates a fresh ProjectInstance per TFM, so ComputeRunArguments is built at most once per instance and the nine ContainSingle deploy assertions in the device tests still hold.

The asset change is genuinely required. ComputeRunArguments is <Target Name="ComputeRunArguments" /> — empty, no DependsOnTargets, and that is the only definition in the repo — so the BeforeTargets + DependsOnTargets hook really is the only way to pull ResolveFrameworkReferences into the second build. I also re-verified the BeforeTargets+DependsOnTargets ordering empirically on a scratch project rather than taking it on trust.

CI

Both red work items are unrelated to this change:

• dotnet.Tests.dll.10 (Windows FullFramework, exit 2) — 30 failures, all in RunFileTests_BuildOptions / RunFileTests_CscOnlyAndApi, i.e. dotnet run file.cs, on the "binary log option was specified but build will be skipped because output is up to date" mismatch. That console log has zero hits for DeployToDevice, ResolveFrameworkReferences, MSB4018 or same key has already been added, and every device test passed in the same build (ItDeploysBeforeComputingRunArguments, ItCallsDeployToDeviceTarget*, ItDeploysEveryTargetFramework, ItAutoSelectsSingleDevicePerTfm, ...). Same signature as #53869.
• dotnet-new.IntegrationTests.dll.1 (Linux, exit 7) — hang-dump timeout with CommonTemplatesTests.FeaturesSupport still running, SIGKILL (137), 224/224 recovered results passed. Same exit-code-7 hang class as #55494 / #55477 / #55430 / #55258, and dotnet-new.IntegrationTests also failed on main in build 1529734 earlier the same day.

One trap for anyone else looking: the AzDO run-level failedTests: 0 on this pipeline is wrong. Run 42131574 reports 0 but actually has 26 distinct failures — the Helix exit codes and TRX files are the ground truth.

Nits

The title says "duplicate runtime pack", but the throw is on the first line of ResolveFrameworkReferences.ExecuteCore — ResolvedTargetingPacks.ToDictionary(tp => tp.ItemSpec, ...) — i.e. targeting packs, which the description itself gets right. Worth fixing since the title is the searchable part.

I did consider the alternative of collapsing this into one submission, project.Build([DeployToDevice, ComputeRunArguments], loggers). MSBuild would dedupe the shared targets so it would avoid the crash too, but it costs the distinction between RunCommandDeployFailed and RunCommandEvaluationExceptionBuildFailed (pinned by ItFailsWhenDeployToDeviceTargetFails) and still shares every other deploy-side mutation with the compute build. Agree with the shape you picked.

Optional and low priority: three commands now hand-roll "fresh instance for the next build" — dotnet watch via DeepCopy, dotnet run via CreateProjectInstance, and now this. A small ProjectInstanceExtensions.BuildFresh(...) with a doc comment describing the accumulation hazard would make it harder for the next person to reintroduce.

Also agree with leaving ResolveFrameworkReferences unhardened — de-duplicating there would hide doubled item state that every downstream @(ResolvedTargetingPack) consumer would still see.

[Evangelink]

This hook is what carries the whole regression guard, and nothing asserts that it exists. If someone later tidies up this empty target, ResolveFrameworkReferences stops running in the ComputeRunArguments build, every device test still passes, and the protection is gone with no signal. The comment helps, but a comment isn't a failing test.

ItDeploysBeforeComputingRunArguments already reads msbuild-dotnet-test.binlog through the AssertTargetInBinlog helper, so pinning it is cheap:

AssertTargetInBinlog(binlogPath, "ResolveFrameworkReferences", targets =>
    targets.Should().HaveCount(2, "ResolveFrameworkReferences must run in both the DeployToDevice build and the ComputeRunArguments build"));

To be precise about what that buys, since it's easy to overclaim: it guards the asset (deleting this hook takes the count 2 → 1). It does not discriminate fixed CLI from unfixed — before the fix the count is also 2, the second one just fails. Reverting DeepCopy() is already caught by these tests crashing outright. Worth confirming the exact count on a real run before committing to the number.

[jonathanpeppers]

If someone later tidies up this empty target ... every device test still passes

So, if someone deleted this target, tests would start failing. I think that is good, and the comment should help someone understand why it exists.

[Evangelink]

I checked this and I think it's the other way around: deleting that target makes the tests pass, not fail.

The PR description already measured exactly that configuration:

Test asset	CLI	Result
DeployToDevice DependsOnTargets only, no ComputeRunArguments hook	unfixed	29 passed — bug not caught
Both asset changes	unfixed	13 failed
Both asset changes	fixed	29 passed

Row 1 is the "someone deleted the target" state, and it's green even against the unfixed CLI. It's green against the fixed CLI too, so the deletion is invisible in both directions.

That follows from the mechanism described in the PR body: the SDK's ComputeRunArguments is empty with no DependsOnTargets. Remove the hook and the second build runs nothing, so nothing accumulates and the duplicate Microsoft.NETCore.App key never occurs. The hook is what creates the failure condition — it isn't something the failure condition depends on.

And nothing else pins it: GivenDotnetTestSelectsDevice.cs never mentions ResolveFrameworkReferences, the asset's ComputeRunArguments targets only touch @(RuntimeEnvironmentVariable), and ItDeploysBeforeComputingRunArguments only asserts DeployToDevice.EndTime <= ComputeRunArguments.StartTime.

So the comment does its job of explaining why the target is there, but there's no failing signal if it goes away — which is why I'd still like a cheap assertion on the target count.