Port workload corruption repair to main from release/10.0.2xx#52758
Port workload corruption repair to main from release/10.0.2xx#52758
Conversation
This ports the fixes from PR #52700 (backport of #52434) to main branch, including: - Workload repair can now recover from corrupt workload sets - Added WorkloadManifestCorruptionRepairer to detect and repair missing manifests - Updated WorkloadInstallerFactory to use the repairer - Added proper error messages for corrupt workload sets - Copilot improvements: clarified nullability check, removed extra blank lines, removed unused import Resolves workload corruption issue where manifests are removed by package managers. Co-authored-by: nagilson <23152278+nagilson@users.noreply.github.com>
Co-authored-by: nagilson <23152278+nagilson@users.noreply.github.com>
|
This is also porting the fixes (namely whitespace) copilot had made into the 10.0.2xx backport PR that I closed because it wasn't needed, so we should merge this even if there is automatic flow into main from 10.0.2xx |
|
@dotnet/dotnet-cli PTAL at approving this PR - it is a backport from copilot so I cant approve but changes LGTM and are approved by tactics |
There was a problem hiding this comment.
Pull request overview
Ports workload-manifest corruption repair support to main, allowing dotnet workload repair (and update) to recover when workload set manifest files are missing (e.g., removed by OS package managers).
Changes:
- Added a CLI-side corruption repairer that reinstalls missing workload-set manifests via a transaction.
- Extended
SdkDirectoryWorkloadManifestProviderwith corruption detection/handling hooks and a new failure-mode enum. - Added/updated tests and localized strings for the new “missing manifests” error path.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Workload/Update/GivenDotnetWorkloadUpdate.cs | Wires manifest provider into mocks and adds a new update test covering missing-manifest repair. |
| test/dotnet.Tests/CommandTests/Workload/Search/MockWorkloadResolver.cs | Adds support for returning a manifest provider from the mock resolver. |
| test/dotnet.Tests/CommandTests/Workload/Restore/GivenDotnetWorkloadRestore.cs | Whitespace/style tweak only. |
| test/dotnet.Tests/CommandTests/Workload/Repair/GivenDotnetWorkloadRepair.cs | Adds a new repair test for corrupt workload sets (missing manifests). |
| test/dotnet.Tests/CommandTests/Workload/Install/MockPackWorkloadInstaller.cs | Implements RepairWorkloads and writes minimal manifest files to disk when installing manifests (enables provider-based detection). |
| test/dotnet.Tests/CommandTests/Workload/Install/CorruptWorkloadSetTestHelper.cs | New helper to set up a corrupt workload set scenario (missing manifests). |
| test/Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs | Updates expected exception type/message for missing-manifest workload set failures. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/IWorkloadManifestProvider.cs | Introduces ManifestCorruptionFailureMode enum to control behavior (repair/throw/ignore). |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/IWorkloadManifestCorruptionRepairer.cs | New interface used by the provider to invoke repair from the CLI layer. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs | Adds missing-manifest detection, repair hook integration, and improved error messaging. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/Strings.resx | Adds resolver-layer resource string for “workload set has missing manifests”. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.cs.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.de.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.es.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.fr.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.it.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.ja.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.ko.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.pl.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.pt-BR.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.ru.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.tr.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.zh-Hans.xlf | Localization entry for new resolver-layer string. |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/xlf/Strings.zh-Hant.xlf | Localization entry for new resolver-layer string. |
| src/Cli/dotnet/Commands/Workload/WorkloadManifestCorruptionRepairer.cs | New CLI component that checks for missing manifests and reinstalls them. |
| src/Cli/dotnet/Commands/Workload/WorkloadHistoryRecorder.cs | Sets provider corruption failure mode to Ignore for history snapshotting. |
| src/Cli/dotnet/Commands/Workload/Repair/WorkloadRepairCommand.cs | Minor cleanup (reuse SdkFeatureBand variable). |
| src/Cli/dotnet/Commands/Workload/Install/WorkloadInstallerFactory.cs | Attaches corruption repairer to the manifest provider during installer creation. |
| src/Cli/dotnet/Commands/Workload/Install/FileBasedInstaller.cs | Adds a new using (currently appears unnecessary). |
| src/Cli/dotnet/Commands/CliCommandStrings.resx | Adds CLI-layer resource string for “workload set has missing manifests”. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.cs.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.de.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.es.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.fr.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.it.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ja.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ko.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.pl.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.pt-BR.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.ru.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.tr.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hans.xlf | Localization entry for new CLI-layer string. |
| src/Cli/dotnet/Commands/xlf/CliCommandStrings.zh-Hant.xlf | Localization entry for new CLI-layer string. |
| _checked = true; | ||
|
|
||
| if (failureMode == ManifestCorruptionFailureMode.Ignore) | ||
| { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
_checked is set to true before the Ignore early-return. If the first call uses ManifestCorruptionFailureMode.Ignore, subsequent calls (e.g., later switching to Repair/Throw) will never re-check or repair. Consider moving the Ignore check before setting _checked, or track the strongest requested failure mode so later calls can still perform repair/throw.
| _checked = true; | |
| if (failureMode == ManifestCorruptionFailureMode.Ignore) | |
| { | |
| return; | |
| } | |
| if (failureMode == ManifestCorruptionFailureMode.Ignore) | |
| { | |
| return; | |
| } | |
| _checked = true; |
| throw new InvalidOperationException(string.Format(CliCommandStrings.WorkloadSetHasMissingManifests, workloadSet.Version)); | ||
| } | ||
|
|
||
| _reporter.WriteLine($"Repairing workload set {workloadSet.Version}..."); |
There was a problem hiding this comment.
The message "Repairing workload set {version}..." is user-facing and currently hardcoded. Consider moving it to CliCommandStrings (and XLF) for localization consistency with the rest of the workload command output.
| _reporter.WriteLine($"Repairing workload set {workloadSet.Version}..."); | |
| _reporter.WriteLine(string.Format(CliCommandStrings.RepairingWorkloadSet, workloadSet.Version)); |
| using Microsoft.NET.Sdk.WorkloadManifestReader; | ||
| using static Microsoft.NET.Sdk.WorkloadManifestReader.IWorkloadManifestProvider; | ||
|
|
There was a problem hiding this comment.
using static Microsoft.NET.Sdk.WorkloadManifestReader.IWorkloadManifestProvider; appears unused here and may trigger CS8019 under warnings-as-errors. Please remove it to keep the test project warning-free.
| { | ||
| var testDirectory = testAssetsManager.CreateTestDirectory(identifier: userLocal ? "userlocal" : "default").Path; | ||
| var dotnetRoot = Path.Combine(testDirectory, "dotnet"); | ||
| var userProfileDir = Path.Combine(testDirectory, "user-profile"); | ||
| sdkFeatureVersion = "6.0.100"; |
There was a problem hiding this comment.
CreateTestDirectory uses the caller member name to derive a deterministic folder. Since this helper calls it directly, multiple test methods that use SetupCorruptWorkloadSet will share the same directory (same caller + same identifier), which can cause cross-test interference/flakiness (e.g., one test repairs manifests and a later test no longer sees corruption). Consider threading the test name through (e.g., add a [CallerMemberName] string testName = "" parameter and pass it to CreateTestDirectory(testName: ...), or include an additional per-test unique identifier).
|
|
||
| using System.Collections.Concurrent; | ||
| using System.Text.Json; | ||
| using Microsoft.DotNet.Cli.Commands.Workload; |
There was a problem hiding this comment.
using Microsoft.DotNet.Cli.Commands.Workload; looks unused in this file (types are either in the current namespace subtree or referenced via other usings). With warnings-as-errors enabled, this may fail the build; please remove the unnecessary using.
| using Microsoft.DotNet.Cli.Commands.Workload; |
| using ManifestReaderTests; | ||
| using Microsoft.DotNet.Cli.Commands.Workload; | ||
| using Microsoft.DotNet.Cli.Commands.Workload.Install; | ||
| using Microsoft.DotNet.Cli.NuGetPackageDownloader; | ||
| using Microsoft.DotNet.Cli.Utils; | ||
| using Microsoft.NET.Sdk.WorkloadManifestReader; |
There was a problem hiding this comment.
Several using directives in this new helper appear unused (e.g., ManifestReaderTests, Microsoft.DotNet.Cli.NuGetPackageDownloader, Microsoft.DotNet.Cli.Utils, and possibly Microsoft.DotNet.Cli.Commands.Workload.Install). With warnings-as-errors enabled, please remove any unused usings to avoid CS8019 build breaks.
| using Microsoft.DotNet.Cli; | ||
| using Microsoft.DotNet.Cli.Commands.Workload; | ||
| using Microsoft.NET.Sdk.Localization; | ||
| using Microsoft.DotNet.Cli.Commands; |
There was a problem hiding this comment.
using Microsoft.DotNet.Cli.Commands; appears to be unused in this file. With warnings-as-errors enabled, this will fail the build; please remove it (or reference a type from that namespace if it was intended).
| using Microsoft.DotNet.Cli.Commands; |
| private readonly IInstaller _workloadInstaller; | ||
| private readonly IWorkloadResolver _workloadResolver; | ||
| private readonly SdkFeatureBand _sdkFeatureBand; | ||
| private readonly string _dotnetPath; | ||
| private readonly string _userProfileDir; | ||
| private readonly INuGetPackageDownloader? _packageDownloader; | ||
| private readonly PackageSourceLocation? _packageSourceLocation; | ||
| private readonly VerbosityOptions _verbosity; | ||
|
|
There was a problem hiding this comment.
_sdkFeatureBand and _dotnetPath are assigned in the constructor but never used, which will generate warnings (likely treated as errors in this repo). Either remove these fields/constructor parameters, or use them (e.g., to scope the repair/diagnostics) so the build stays clean.
Ports PR #52700 (backport of #52434) from release/10.0.2xx to main, enabling
workload repairto recover from corrupt workload sets where package managers have removed manifest files.Implementation
HasMissingManifests()to detect corruption, throws descriptive error when manifests missing outside repair contextCode clarity improvements
Includes copilot review suggestions from #52700:
provider == null || !provider.HasMissingManifests(workloadSet)replaces nullable operator patternTest coverage
GivenMissingManifestsInWorkloadSetModeRepairReinstallsManifests: Validates repair command reinstalls missing manifestsGivenMissingManifestsInWorkloadSetModeUpdateReinstallsManifests: Validates update command triggers repairCorruptWorkloadSetTestHelper: Test infrastructure for simulating corruption scenariosWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/repos/dotnet/sdk/pulls/52700/commits/usr/bin/curl curl -s REDACTED(http block)If you need me to access, download, or install something from one of these locations, you can either:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.