From c3d77669abc84fb51434da327accd39707d8f831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Thu, 6 Aug 2026 16:03:51 +0200 Subject: [PATCH 1/2] Ensure cache cleanup after shutdown failures Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../MSBuildCachePluginBaseTests.cs | 157 ++++++++++++++++++ src/Common/MSBuildCachePluginBase.cs | 103 ++++++++++-- 2 files changed, 248 insertions(+), 12 deletions(-) diff --git a/src/Common.Tests/MSBuildCachePluginBaseTests.cs b/src/Common.Tests/MSBuildCachePluginBaseTests.cs index d7e6d56..ae27d66 100644 --- a/src/Common.Tests/MSBuildCachePluginBaseTests.cs +++ b/src/Common.Tests/MSBuildCachePluginBaseTests.cs @@ -5,10 +5,16 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.IO; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; using BuildXL.Cache.ContentStore.Hashing; using DotNet.Globbing; using Microsoft.Build.Construction; using Microsoft.Build.Execution; +using Microsoft.Build.Experimental.ProjectCache; +using Microsoft.MSBuildCache.Caching; +using Microsoft.MSBuildCache.Fingerprinting; using Microsoft.MSBuildCache.Tests.Mocks; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -154,4 +160,155 @@ private static NodeContext CreateNode(string projectFileRelativePath, IReadOnlyL Array.Empty(), null, new HashSet(StringComparer.OrdinalIgnoreCase)); + + [TestMethod] + [DoNotParallelize] +#pragma warning disable CA2000 // Ownership is transferred to the plugin; the finally block disposes only if EndBuildAsync did not. + public async Task EndBuildAsyncDisposesCacheClientAndPreservesExceptionWhenAsynchronousPublishingFails() + { + FaultingCacheClient cacheClient = new("publishing"); + TestPlugin plugin = new(); + SetCacheClient(plugin, cacheClient); + + try + { + AggregateException exception = await Assert.ThrowsExactlyAsync( + () => plugin.EndBuildAsync(NullPluginLogger.Instance, CancellationToken.None)); + + Assert.AreSame(cacheClient.ShutdownFailure, exception); + Assert.IsTrue(cacheClient.DisposeCalled, "The cache client must be disposed after shutdown fails."); + } + finally + { + if (!cacheClient.DisposeCalled) + { + await plugin.DisposeAsync(); + } + } + } +#pragma warning restore CA2000 + + [TestMethod] + [DoNotParallelize] +#pragma warning disable CA2000 // Ownership is transferred to the plugins; the finally block avoids releasing a shared lock twice. + public async Task EndBuildAsyncReleasesProcessAndDirectoryLocksWhenAsynchronousMaterializationFails() + { + string cacheRoot = Path.Combine(Path.GetTempPath(), "MSBuildCacheTests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(cacheRoot); + + PluginSettings settings = new() + { + RepoRoot = cacheRoot, + LocalCacheRootPath = cacheRoot, + }; + + FaultingCacheClient cacheClient = new("materialization"); + TestPlugin firstPlugin = new(); + TestPlugin secondPlugin = new(); + bool firstPluginAcquiredLocks = false; + bool secondPluginAcquiredLocks = false; + + try + { + firstPluginAcquiredLocks = TryAcquireLock(firstPlugin, settings); + Assert.IsTrue(firstPluginAcquiredLocks, "The first plugin must acquire both locks for the test to be valid."); + SetCacheClient(firstPlugin, cacheClient); + + AggregateException exception = await Assert.ThrowsExactlyAsync( + () => firstPlugin.EndBuildAsync(NullPluginLogger.Instance, CancellationToken.None)); + Assert.AreSame(cacheClient.ShutdownFailure, exception); + + secondPluginAcquiredLocks = TryAcquireLock(secondPlugin, settings); + Assert.IsTrue( + secondPluginAcquiredLocks, + "A subsequent plugin must be able to reacquire both the process-wide semaphore and cache directory lock."); + } +#pragma warning restore CA2000 + finally + { + if (firstPluginAcquiredLocks && !cacheClient.DisposeCalled) + { + await firstPlugin.DisposeAsync(); + } + + if (secondPluginAcquiredLocks) + { + await secondPlugin.DisposeAsync(); + } + + Directory.Delete(cacheRoot, recursive: true); + } + } + + private static void SetCacheClient(TestPlugin plugin, ICacheClient cacheClient) + { + FieldInfo cacheClientField = typeof(MSBuildCachePluginBase).GetField( + "_cacheClient", + BindingFlags.Instance | BindingFlags.NonPublic)!; + cacheClientField.SetValue(plugin, cacheClient); + } + + private static bool TryAcquireLock(TestPlugin plugin, PluginSettings settings) + { + MethodInfo tryAcquireLockMethod = typeof(MSBuildCachePluginBase).GetMethod( + "TryAcquireLock", + BindingFlags.Instance | BindingFlags.NonPublic)!; + return (bool)tryAcquireLockMethod.Invoke(plugin, [settings, NullPluginLogger.Instance])!; + } + + private sealed class TestPlugin : MSBuildCachePluginBase + { + protected override HashType HashType => HashType.Murmur; + + protected override Task CreateCacheClientAsync(PluginLoggerBase logger, CancellationToken cancellationToken) + => throw new NotSupportedException(); + } + + private sealed class FaultingCacheClient : ICacheClient + { + private readonly Task _backgroundOperation; + + public FaultingCacheClient(string operationName) + { + IOException backgroundFailure = new($"Asynchronous {operationName} failed."); + _backgroundOperation = Task.FromException(backgroundFailure); + ShutdownFailure = new AggregateException(backgroundFailure); + } + + public bool DisposeCalled { get; private set; } + + public AggregateException ShutdownFailure { get; } + + public Task AddNodeAsync( + NodeContext nodeContext, + PathSet? pathSet, + IReadOnlyCollection outputPaths, + Func, NodeBuildResult> nodeBuildResultBuilder, + CancellationToken cancellationToken) + => throw new NotSupportedException(); + + public Task<(PathSet?, NodeBuildResult?)> GetNodeAsync( + NodeContext nodeContext, + bool materializeOutputs, + CancellationToken cancellationToken) + => throw new NotSupportedException(); + + public async Task ShutdownAsync(CancellationToken cancellationToken) + { + try + { + await _backgroundOperation; + } + catch (IOException) + { + throw ShutdownFailure; + } + } + + public ValueTask DisposeAsync() + { + DisposeCalled = true; + return default; + } + } } diff --git a/src/Common/MSBuildCachePluginBase.cs b/src/Common/MSBuildCachePluginBase.cs index 4bfb2c4..9304a02 100644 --- a/src/Common/MSBuildCachePluginBase.cs +++ b/src/Common/MSBuildCachePluginBase.cs @@ -12,6 +12,7 @@ using System.Linq; using System.Reflection; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Text; using System.Text.Json; using System.Threading; @@ -144,21 +145,58 @@ public virtual async ValueTask DisposeAsync() { GC.SuppressFinalize(this); - if (_cacheClient != null) + List exceptions = new(); + + ICacheClient? cacheClient = _cacheClient; + _cacheClient = null; + if (cacheClient != null) { - await _cacheClient.DisposeAsync(); + await CaptureExceptionAsync(cacheClient.DisposeAsync, exceptions); } - ContentHasher?.Dispose(); + IContentHasher? contentHasher = ContentHasher; + ContentHasher = null; + if (contentHasher != null) + { + CaptureException(contentHasher.Dispose, exceptions); + } - if (_fileAccessRepository is IDisposable fileAccessRepositoryDisposable) + if (_fileAccessRepository != null) { - fileAccessRepositoryDisposable.Dispose(); + try + { + _fileAccessRepository.Dispose(); + } + catch (Exception ex) + { + exceptions.Add(ex); + } } + _fileAccessRepository = null; _outputProducer.Clear(); - _localCacheDirectoryLock?.Dispose(); - _singlePluginInstanceMutex?.Release(); + + if (_localCacheDirectoryLock != null) + { + try + { + _localCacheDirectoryLock.Dispose(); + } + catch (Exception ex) + { + exceptions.Add(ex); + } + } + + _localCacheDirectoryLock = null; + SemaphoreSlim? singlePluginInstanceMutex = _singlePluginInstanceMutex; + _singlePluginInstanceMutex = null; + if (singlePluginInstanceMutex != null) + { + CaptureException(() => singlePluginInstanceMutex.Release(), exceptions); + } + + ThrowIfAny(exceptions); } protected virtual string? GetBuildId() @@ -357,16 +395,20 @@ public override Task EndBuildAsync(PluginLoggerBase logger, CancellationToken ca private async Task EndBuildInnerAsync(PluginLoggerBase logger, CancellationToken cancellationToken) { - if (_cacheClient is not null) + List exceptions = new(); + + ICacheClient? cacheClient = _cacheClient; + if (cacheClient is not null) { - await _cacheClient.ShutdownAsync(cancellationToken); + await CaptureExceptionAsync(() => new ValueTask(cacheClient.ShutdownAsync(cancellationToken)), exceptions); } - await DisposeAsync(); + await CaptureExceptionAsync(DisposeAsync, exceptions); + _pluginLogger = null; + + ThrowIfAny(exceptions); LogCacheStats(logger); - - _pluginLogger = null; } public override Task GetCacheResultAsync(BuildRequestData buildRequest, PluginLoggerBase logger, CancellationToken cancellationToken) @@ -1370,6 +1412,43 @@ private static void TimeAndLog( } } + private static void CaptureException(Action action, List exceptions) + { + try + { + action(); + } + catch (Exception ex) + { + exceptions.Add(ex); + } + } + + private static async ValueTask CaptureExceptionAsync(Func action, List exceptions) + { + try + { + await action(); + } + catch (Exception ex) + { + exceptions.Add(ex); + } + } + + private static void ThrowIfAny(List exceptions) + { + if (exceptions.Count == 1) + { + ExceptionDispatchInfo.Capture(exceptions[0]).Throw(); + } + + if (exceptions.Count > 1) + { + throw new AggregateException(exceptions); + } + } + private static Task TimeAndLogAsync(PluginLoggerBase logger, Func innerAsync, CancellationToken cancellationToken, string? context = null, [CallerMemberName] string memberName = "") => TimeAndLogAsync(logger, async () => { await innerAsync(); return 0; }, cancellationToken, context, memberName); } From 8459aef51c50e55c4059ff45c1ce9ee1b9be42bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20Lev=C3=A9?= Date: Fri, 7 Aug 2026 01:01:40 +0200 Subject: [PATCH 2/2] Narrow shutdown cleanup handling Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Common/MSBuildCachePluginBase.cs | 115 ++++++--------------------- 1 file changed, 23 insertions(+), 92 deletions(-) diff --git a/src/Common/MSBuildCachePluginBase.cs b/src/Common/MSBuildCachePluginBase.cs index 9304a02..d7981ec 100644 --- a/src/Common/MSBuildCachePluginBase.cs +++ b/src/Common/MSBuildCachePluginBase.cs @@ -12,7 +12,6 @@ using System.Linq; using System.Reflection; using System.Runtime.CompilerServices; -using System.Runtime.ExceptionServices; using System.Text; using System.Text.Json; using System.Threading; @@ -145,58 +144,21 @@ public virtual async ValueTask DisposeAsync() { GC.SuppressFinalize(this); - List exceptions = new(); - - ICacheClient? cacheClient = _cacheClient; - _cacheClient = null; - if (cacheClient != null) + if (_cacheClient != null) { - await CaptureExceptionAsync(cacheClient.DisposeAsync, exceptions); + await _cacheClient.DisposeAsync(); } - IContentHasher? contentHasher = ContentHasher; - ContentHasher = null; - if (contentHasher != null) - { - CaptureException(contentHasher.Dispose, exceptions); - } + ContentHasher?.Dispose(); - if (_fileAccessRepository != null) + if (_fileAccessRepository is IDisposable fileAccessRepositoryDisposable) { - try - { - _fileAccessRepository.Dispose(); - } - catch (Exception ex) - { - exceptions.Add(ex); - } + fileAccessRepositoryDisposable.Dispose(); } - _fileAccessRepository = null; _outputProducer.Clear(); - - if (_localCacheDirectoryLock != null) - { - try - { - _localCacheDirectoryLock.Dispose(); - } - catch (Exception ex) - { - exceptions.Add(ex); - } - } - - _localCacheDirectoryLock = null; - SemaphoreSlim? singlePluginInstanceMutex = _singlePluginInstanceMutex; - _singlePluginInstanceMutex = null; - if (singlePluginInstanceMutex != null) - { - CaptureException(() => singlePluginInstanceMutex.Release(), exceptions); - } - - ThrowIfAny(exceptions); + _localCacheDirectoryLock?.Dispose(); + _singlePluginInstanceMutex?.Release(); } protected virtual string? GetBuildId() @@ -395,18 +357,24 @@ public override Task EndBuildAsync(PluginLoggerBase logger, CancellationToken ca private async Task EndBuildInnerAsync(PluginLoggerBase logger, CancellationToken cancellationToken) { - List exceptions = new(); - - ICacheClient? cacheClient = _cacheClient; - if (cacheClient is not null) + try { - await CaptureExceptionAsync(() => new ValueTask(cacheClient.ShutdownAsync(cancellationToken)), exceptions); + if (_cacheClient is not null) + { + await _cacheClient.ShutdownAsync(cancellationToken); + } + } + finally + { + try + { + await DisposeAsync(); + } + finally + { + _pluginLogger = null; + } } - - await CaptureExceptionAsync(DisposeAsync, exceptions); - _pluginLogger = null; - - ThrowIfAny(exceptions); LogCacheStats(logger); } @@ -1412,43 +1380,6 @@ private static void TimeAndLog( } } - private static void CaptureException(Action action, List exceptions) - { - try - { - action(); - } - catch (Exception ex) - { - exceptions.Add(ex); - } - } - - private static async ValueTask CaptureExceptionAsync(Func action, List exceptions) - { - try - { - await action(); - } - catch (Exception ex) - { - exceptions.Add(ex); - } - } - - private static void ThrowIfAny(List exceptions) - { - if (exceptions.Count == 1) - { - ExceptionDispatchInfo.Capture(exceptions[0]).Throw(); - } - - if (exceptions.Count > 1) - { - throw new AggregateException(exceptions); - } - } - private static Task TimeAndLogAsync(PluginLoggerBase logger, Func innerAsync, CancellationToken cancellationToken, string? context = null, [CallerMemberName] string memberName = "") => TimeAndLogAsync(logger, async () => { await innerAsync(); return 0; }, cancellationToken, context, memberName); }