Fix concurrent tool installation race conditions with named mutex #51834
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -272,26 +272,53 @@ protected void DownloadTool( | |
| string? targetFramework, | ||
| VerbosityOptions verbosity) | ||
| { | ||
| // Use a named mutex to serialize concurrent installations of the same tool package | ||
| string mutexName = GetToolInstallMutexName(packageId, packageVersion); | ||
| using var mutex = new Mutex(false, mutexName); | ||
|
|
||
| try | ||
| { | ||
| // Wait for the mutex with a reasonable timeout | ||
| if (!mutex.WaitOne(TimeSpan.FromMinutes(5))) | ||
| { | ||
| throw new ToolPackageException(string.Format(CliStrings.ToolInstallationTimeout, packageId, packageVersion)); | ||
| } | ||
|
||
|
|
||
| if (!IsPackageInstalled(packageId, packageVersion, packageDownloadDir.Value)) | ||
| { | ||
| DownloadAndExtractPackage(packageId, nugetPackageDownloader, packageDownloadDir.Value, packageVersion, packageSourceLocation, includeUnlisted: givenSpecificVersion, verbosity: verbosity); | ||
| } | ||
|
|
||
| CreateAssetFile(packageId, packageVersion, packageDownloadDir, Path.Combine(assetFileDirectory.Value, ToolPackageInstance.AssetsFileName), _runtimeJsonPath, verbosity, targetFramework); | ||
|
|
||
| // Also download RID-specific package if needed | ||
| if (ResolveRidSpecificPackage(packageId, packageVersion, packageDownloadDir, assetFileDirectory, verbosity) is PackageId ridSpecificPackage) | ||
| { | ||
| if (!IsPackageInstalled(ridSpecificPackage, packageVersion, packageDownloadDir.Value)) | ||
| { | ||
| DownloadAndExtractPackage(ridSpecificPackage, nugetPackageDownloader, packageDownloadDir.Value, packageVersion, packageSourceLocation, includeUnlisted: true, verbosity: verbosity); | ||
| } | ||
|
|
||
| CreateAssetFile(ridSpecificPackage, packageVersion, packageDownloadDir, Path.Combine(assetFileDirectory.Value, ToolPackageInstance.RidSpecificPackageAssetsFileName), _runtimeJsonPath, verbosity, targetFramework); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| mutex.ReleaseMutex(); | ||
| } | ||
|
Comment on lines
+312
to
315
There was a problem hiding this comment. The Example: bool mutexAcquired = false;
try
{
if (!mutex.WaitOne(TimeSpan.FromMilliseconds(50)))
{
// ...
mutexAcquired = mutex.WaitOne(TimeSpan.FromMinutes(5));
}
else
{
mutexAcquired = true;
}
if (mutexAcquired)
{
// ... installation logic ...
}
}
finally
{
if (mutexAcquired)
{
mutex.ReleaseMutex();
}
} |
||
| } | ||
|
|
||
| private static string GetToolInstallMutexName(PackageId packageId, NuGetVersion packageVersion) | ||
| { | ||
| // Create a mutex name in the format: tool-install-{packageId}-{packageVersion} | ||
| // Replace characters that are invalid in mutex names with underscores | ||
| string safeName = $"tool-install-{packageId}-{packageVersion.ToNormalizedString()}" | ||
| .Replace('/', '_') | ||
| .Replace('\\', '_'); | ||
|
|
||
| return safeName; | ||
| } | ||
|
Comment on lines
+318
to
+327
There was a problem hiding this comment. The mutex name sanitization only replaces
Package IDs and versions could contain other problematic characters (e.g., Example: private static string GetToolInstallMutexName(PackageId packageId, NuGetVersion packageVersion)
{
string baseName = $"tool-install-{packageId}-{packageVersion.ToNormalizedString()}";
// If the name is too long or contains problematic characters, use a hash
if (baseName.Length > 200 || !IsValidMutexName(baseName))
{
using var sha256 = SHA256.Create();
var hash = Convert.ToBase64String(sha256.ComputeHash(Encoding.UTF8.GetBytes(baseName)))
.Replace('/', '_')
.Replace('+', '-');
return $"tool-install-{hash}";
}
return baseName.Replace('/', '_').Replace('\\', '_');
} |
||
|
|
||
| public bool TryGetDownloadedTool( | ||
| PackageId packageId, | ||
| NuGetVersion packageVersion, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about the scenario where a computer running Windows has two Jenkins agents installed as services with separate user accounts and separate file-system directories, and the agents run
dotnet tool installon the same tool in parallel. Then this code will construct the samemutexNamestring in both processes, and because all services run in session 0, they will attempt to open the same mutex object; but because they have separate user accounts, the DACL of the mutex might not allow the second open.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'd suggest including a hash of the directory path where the tool is going to be installed.