From a68a295d23fd30d704d5459a3d246270c64ad7df Mon Sep 17 00:00:00 2001 From: David Federman Date: Mon, 3 Aug 2026 12:47:33 -0700 Subject: [PATCH] Make late file access warnings configurable Add a focused setting that logs allowlisted late file accesses as high-importance messages while preserving warning behavior by default. Cover the production repository path and MSBuild warn-as-error policy. Copilot-Session: e30d7b9b-de7d-4db4-a221-cc1312fa09ff --- README.md | 1 + .../FileAccess/FileAccessRepositoryTests.cs | 136 ++++++++++++++ src/Common.Tests/PluginSettingsTests.cs | 6 + src/Common.Tests/WarningPolicyTests.cs | 169 ++++++++++++++++++ src/Common/FileAccess/FileAccessRepository.cs | 10 +- src/Common/PluginLoggerExtensions.cs | 22 +++ src/Common/PluginSettings.cs | 2 + .../Microsoft.MSBuildCache.Common.targets | 2 + 8 files changed, 344 insertions(+), 4 deletions(-) create mode 100644 src/Common.Tests/FileAccess/FileAccessRepositoryTests.cs create mode 100644 src/Common.Tests/WarningPolicyTests.cs create mode 100644 src/Common/PluginLoggerExtensions.cs diff --git a/README.md b/README.md index 1875b49..c2e3778 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,7 @@ These settings are common across all plugins, although different implementations | `$(MSBuildCacheAsyncCacheMaterialization)` | `bool` | true | Whether files are materialized on disk asynchronously from the cache as opposed to delaying the completion signal to MSBuild until publishing is complete. Note: Materialization will be awaited when a depending project requires the files and before the overall build completes. | | `$(MSBuildCacheAllowFileAccessAfterProjectFinishProcessPatterns)` | `Glob[]` | `\**\vctip.exe` | Processes to allow file accesses after the project which launched it completes, ie accesses by a detached process. Note: these accesses will not be considered for caching. | | `$(MSBuildCacheAllowFileAccessAfterProjectFinishFilePatterns)` | `Glob[]` | | Files to allow to be accessed by a process launched by a project after the project completes, ie accesses by a detached process. Note: these accesses will not be considered for caching. | +| `$(MSBuildCacheLogAllowFileAccessAfterProjectFinishMatchesAsMessages)` | `bool` | false | Whether late file accesses matching `$(MSBuildCacheAllowFileAccessAfterProjectFinishProcessPatterns)` or `$(MSBuildCacheAllowFileAccessAfterProjectFinishFilePatterns)` are logged as messages instead of warnings. This can keep these diagnostics from being promoted by `/warnaserror`; other warnings remain unaffected. | | `$(MSBuildCacheAllowProcessCloseAfterProjectFinishProcessPatterns)` | `Glob[]` | `\**\mspdbsrv.exe` | Processes to allow to exit after the project which launched it completes, ie detached processes. | | `$(MSBuildCacheGlobalPropertiesToIgnore)` | `string[]` | `CurrentSolutionConfigurationContents; ShouldUnsetParentConfigurationAndPlatform; BuildingInsideVisualStudio; BuildingSolutionFile; SolutionDir; SolutionExt; SolutionFileName; SolutionName; SolutionPath; _MSDeployUserAgent`, as well as all proeprties related to plugin settings | The list of global properties to exclude from consideration by the cache | | `$(MSBuildCacheGetResultsForUnqueriedDependencies)` | `bool` | false | Whether to try and query the cache for dependencies if they have not previously been requested. This option can help in cases where the build isn't done in graph order, or if some projects are skipped. | diff --git a/src/Common.Tests/FileAccess/FileAccessRepositoryTests.cs b/src/Common.Tests/FileAccess/FileAccessRepositoryTests.cs new file mode 100644 index 0000000..a95ab67 --- /dev/null +++ b/src/Common.Tests/FileAccess/FileAccessRepositoryTests.cs @@ -0,0 +1,136 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Collections.Generic; +using System.IO; +using Microsoft.Build.Execution; +using Microsoft.Build.Experimental.FileAccess; +using Microsoft.MSBuildCache.FileAccess; +using Microsoft.MSBuildCache.Tests.Mocks; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.MSBuildCache.Tests.FileAccess; + +[TestClass] +public sealed class FileAccessRepositoryTests +{ +#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Justification: Always set by MSTest. + public TestContext TestContext { get; set; } +#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. + + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public void FilePatternMatchUsesConfiguredLogLevel(bool logAsMessage) + { + string testDirectory = CreateTestDirectory(); + string lateAccessPath = Path.Combine(testDirectory, "late-access.txt"); + PluginSettings settings = CreateSettings( + testDirectory, + new Dictionary + { + [nameof(PluginSettings.AllowFileAccessAfterProjectFinishFilePatterns)] = lateAccessPath, + [nameof(PluginSettings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages)] = logAsMessage.ToString(), + }); + + MockPluginLogger logger = new(); + using FileAccessRepository repository = new(logger, settings); + NodeContext nodeContext = CreateNodeContext(testDirectory); + + _ = repository.FinishProject(nodeContext); + repository.AddFileAccess(nodeContext, CreateFileAccess(lateAccessPath)); + + Assert.HasCount(1, logger.LogEntries); + Assert.AreEqual(logAsMessage ? PluginLogLevel.Message : PluginLogLevel.Warning, logger.LogEntries[0].LogLevel); + } + + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public void ProcessPatternMatchUsesConfiguredLogLevel(bool logAsMessage) + { + string testDirectory = CreateTestDirectory(); + string processPath = Path.Combine(testDirectory, "detached.exe"); + PluginSettings settings = CreateSettings( + testDirectory, + new Dictionary + { + [nameof(PluginSettings.AllowFileAccessAfterProjectFinishProcessPatterns)] = processPath, + [nameof(PluginSettings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages)] = logAsMessage.ToString(), + }); + + MockPluginLogger logger = new(); + using FileAccessRepository repository = new(logger, settings); + NodeContext nodeContext = CreateNodeContext(testDirectory); + + repository.AddFileAccess(nodeContext, CreateProcessAccess(processPath)); + _ = repository.FinishProject(nodeContext); + repository.AddFileAccess(nodeContext, CreateFileAccess(Path.Combine(testDirectory, "other.txt"))); + + Assert.HasCount(1, logger.LogEntries); + Assert.AreEqual(logAsMessage ? PluginLogLevel.Message : PluginLogLevel.Warning, logger.LogEntries[0].LogLevel); + } + + private string CreateTestDirectory() + { + string testDirectory = Path.Combine( + TestContext.TestResultsDirectory!, + nameof(FileAccessRepositoryTests), + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(testDirectory); + return testDirectory; + } + + private static PluginSettings CreateSettings(string repoRoot, Dictionary settings) + => PluginSettings.Create( + settings, + NullPluginLogger.Instance, + repoRoot, + supportsProbeAndEnumerationCapture: true); + + private static NodeContext CreateNodeContext(string testDirectory) + { + string projectPath = Path.Combine(testDirectory, "p.proj"); + File.WriteAllText(projectPath, ""); + ProjectInstance projectInstance = new(projectPath); + + return new NodeContext( + testDirectory, + projectInstance, + Array.Empty(), + "p", + new Dictionary(), + Array.Empty(), + referenceAssemblyRelativePath: null, + new HashSet(StringComparer.OrdinalIgnoreCase)); + } + + private static FileAccessData CreateFileAccess(string path) + => new( + ReportedFileOperation.CreateFile, + RequestedAccess.Read, + processId: 123, + id: 0, + correlationId: 0, + error: 1, + DesiredAccess.GENERIC_READ, + FlagsAndAttributes.FILE_ATTRIBUTE_NORMAL, + path, + processArgs: null, + isAnAugmentedFileAccess: false); + + private static FileAccessData CreateProcessAccess(string path) + => new( + ReportedFileOperation.Process, + RequestedAccess.None, + processId: 123, + id: 0, + correlationId: 0, + error: 0, + desiredAccess: 0, + flagsAndAttributes: 0, + path, + processArgs: null, + isAnAugmentedFileAccess: false); +} diff --git a/src/Common.Tests/PluginSettingsTests.cs b/src/Common.Tests/PluginSettingsTests.cs index 27615ad..ccbdfb0 100644 --- a/src/Common.Tests/PluginSettingsTests.cs +++ b/src/Common.Tests/PluginSettingsTests.cs @@ -274,6 +274,12 @@ public void AllowFileAccessAfterProjectFinishFilePatternsSupportsMachineLocalCat Assert.IsFalse(patterns.Any(pattern => pattern.IsMatch(@"X:\Repo\src\Program.cs"))); } + [TestMethod] + public void LogAllowFileAccessAfterProjectFinishMatchesAsMessagesSetting() + => TestBoolSetting( + nameof(PluginSettings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages), + pluginSettings => pluginSettings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages); + [TestMethod] [DynamicData(nameof(GlobTestCases), DynamicDataDisplayName = nameof(GetTestCaseDisplayName))] public void AllowProcessCloseAfterProjectFinishProcessPatternsSetting(GlobTestCase testCase) diff --git a/src/Common.Tests/WarningPolicyTests.cs b/src/Common.Tests/WarningPolicyTests.cs new file mode 100644 index 0000000..7b72553 --- /dev/null +++ b/src/Common.Tests/WarningPolicyTests.cs @@ -0,0 +1,169 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +#if !NETFRAMEWORK +using System.Diagnostics; +using System.IO; +using System.Security; +#endif +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Build.Execution; +using Microsoft.Build.Experimental.ProjectCache; +#if !NETFRAMEWORK +using Microsoft.VisualStudio.TestTools.UnitTesting; +#endif + +namespace Microsoft.MSBuildCache.Tests; + +public sealed class WarningPolicyTestPlugin : ProjectCachePluginBase +{ + internal const string DiagnosticMessage = "Allowlisted late file access diagnostic"; + + public override Task BeginBuildAsync(CacheContext context, PluginLoggerBase logger, CancellationToken cancellationToken) + { + PluginSettings settings = PluginSettings.Create( + context.PluginSettings, + logger, + Environment.CurrentDirectory, + supportsProbeAndEnumerationCapture: true); + logger.LogWarningOrMessage(DiagnosticMessage, settings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages); + return Task.CompletedTask; + } + + public override Task GetCacheResultAsync( + BuildRequestData buildRequest, + PluginLoggerBase logger, + CancellationToken cancellationToken) + => Task.FromResult(CacheResult.IndicateNonCacheHit(CacheResultType.CacheNotApplicable)); + + public override Task EndBuildAsync(PluginLoggerBase logger, CancellationToken cancellationToken) + => Task.CompletedTask; +} + +#if !NETFRAMEWORK +[TestClass] +public sealed class WarningPolicyTests +{ +#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Justification: Always set by MSTest. + public TestContext TestContext { get; set; } +#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. + + [TestMethod] + public async Task DefaultDiagnosticIsVisibleAsWarning() + { + BuildInvocationResult result = await RunBuildAsync(logAsMessage: false, warnAsError: false, emitUnrelatedWarning: false); + + StringAssert.Contains(result.Output, "Build succeeded.", StringComparison.Ordinal); + StringAssert.Contains(result.Output, WarningPolicyTestPlugin.DiagnosticMessage, StringComparison.Ordinal); + } + + [TestMethod] + public async Task WarnAsErrorEscalatesDefaultDiagnostic() + { + BuildInvocationResult result = await RunBuildAsync(logAsMessage: false, warnAsError: true, emitUnrelatedWarning: false); + + StringAssert.Contains(result.Output, "Build FAILED.", StringComparison.Ordinal); + StringAssert.Contains(result.Output, WarningPolicyTestPlugin.DiagnosticMessage, StringComparison.Ordinal); + } + + [TestMethod] + public async Task MessageSettingAvoidsWarnAsErrorEscalation() + { + BuildInvocationResult result = await RunBuildAsync(logAsMessage: true, warnAsError: true, emitUnrelatedWarning: false); + + StringAssert.Contains(result.Output, "Build succeeded.", StringComparison.Ordinal); + StringAssert.Contains(result.Output, WarningPolicyTestPlugin.DiagnosticMessage, StringComparison.Ordinal); + } + + [TestMethod] + public async Task MessageSettingDoesNotWeakenOtherWarnings() + { + BuildInvocationResult result = await RunBuildAsync(logAsMessage: true, warnAsError: true, emitUnrelatedWarning: true); + + StringAssert.Contains(result.Output, "Build FAILED.", StringComparison.Ordinal); + StringAssert.Contains(result.Output, "Unrelated warning", StringComparison.Ordinal); + } + + private async Task RunBuildAsync(bool logAsMessage, bool warnAsError, bool emitUnrelatedWarning) + { + string testDirectory = Path.Combine( + TestContext.TestResultsDirectory!, + nameof(WarningPolicyTests), + TestContext.TestName!, + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(testDirectory); + + string pluginPath = SecurityElement.Escape(typeof(WarningPolicyTestPlugin).Assembly.Location)!; + string commonTargetsPath = SecurityElement.Escape(FindCommonTargetsPath())!; + string projectPath = Path.Combine(testDirectory, "test.proj"); + string projectContents = + $""" + + + {pluginPath} + {logAsMessage} + + + + + + + """; + await File.WriteAllTextAsync(projectPath, projectContents); + + ProcessStartInfo startInfo = new() + { + FileName = Environment.GetEnvironmentVariable("DOTNET_HOST_PATH") ?? "dotnet", + WorkingDirectory = testDirectory, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + }; + startInfo.ArgumentList.Add("msbuild"); + startInfo.ArgumentList.Add(projectPath); + startInfo.ArgumentList.Add("-graphBuild"); + startInfo.ArgumentList.Add("-maxCpuCount:1"); + startInfo.ArgumentList.Add("-nodeReuse:false"); + startInfo.ArgumentList.Add("-verbosity:normal"); + if (warnAsError) + { + startInfo.ArgumentList.Add("-warnAsError"); + } + + if (emitUnrelatedWarning) + { + startInfo.ArgumentList.Add("-property:EmitUnrelatedWarning=true"); + } + + using Process process = Process.Start(startInfo)!; + Task standardOutput = process.StandardOutput.ReadToEndAsync(); + Task standardError = process.StandardError.ReadToEndAsync(); + await process.WaitForExitAsync(); + + string output = await standardOutput + await standardError; + return new BuildInvocationResult(output); + } + + private static string FindCommonTargetsPath() + { + DirectoryInfo? directory = new FileInfo(typeof(WarningPolicyTestPlugin).Assembly.Location).Directory; + while (directory != null) + { + string candidatePath = Path.Combine(directory.FullName, "src", "Common", "build", "Microsoft.MSBuildCache.Common.targets"); + if (File.Exists(candidatePath)) + { + return candidatePath; + } + + directory = directory.Parent; + } + + throw new InvalidOperationException("Could not find Microsoft.MSBuildCache.Common.targets."); + } + + private readonly record struct BuildInvocationResult(string Output); +} +#endif diff --git a/src/Common/FileAccess/FileAccessRepository.cs b/src/Common/FileAccess/FileAccessRepository.cs index f3b3eb5..99967f5 100644 --- a/src/Common/FileAccess/FileAccessRepository.cs +++ b/src/Common/FileAccess/FileAccessRepository.cs @@ -263,15 +263,17 @@ public void AddFileAccess(FileAccessData fileAccessData) if (processMatch != null) { - _logger.LogWarning( + _logger.LogWarningOrMessage( $"File access reported from process after the project finished, but process matched {nameof(_pluginSettings.AllowFileAccessAfterProjectFinishProcessPatterns)} `{processMatch}`. " + - $"This may lead to incorrect caching. Node Id: {_nodeContext.Id}, Process Id: {fileAccessData.ProcessId} ProcessPath: `{processName}` File Path: `{path}`"); + $"This may lead to incorrect caching. Node Id: {_nodeContext.Id}, Process Id: {fileAccessData.ProcessId} ProcessPath: `{processName}` File Path: `{path}`", + _pluginSettings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages); } else if (fileMatch != null) { - _logger.LogWarning( + _logger.LogWarningOrMessage( $"File access reported from process after the project finished, but file path matched {nameof(_pluginSettings.AllowFileAccessAfterProjectFinishFilePatterns)} `{fileMatch}`. " + - $"This may lead to incorrect caching. Node Id: {_nodeContext.Id}, Process Id: {fileAccessData.ProcessId} ProcessPath: `{processName}` File Path: `{path}`"); + $"This may lead to incorrect caching. Node Id: {_nodeContext.Id}, Process Id: {fileAccessData.ProcessId} ProcessPath: `{processName}` File Path: `{path}`", + _pluginSettings.LogAllowFileAccessAfterProjectFinishMatchesAsMessages); } else { diff --git a/src/Common/PluginLoggerExtensions.cs b/src/Common/PluginLoggerExtensions.cs new file mode 100644 index 0000000..477de53 --- /dev/null +++ b/src/Common/PluginLoggerExtensions.cs @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.Build.Experimental.ProjectCache; +using Microsoft.Build.Framework; + +namespace Microsoft.MSBuildCache; + +internal static class PluginLoggerExtensions +{ + public static void LogWarningOrMessage(this PluginLoggerBase logger, string message, bool logAsMessage) + { + if (logAsMessage) + { + logger.LogMessage(message, MessageImportance.High); + } + else + { + logger.LogWarning(message); + } + } +} diff --git a/src/Common/PluginSettings.cs b/src/Common/PluginSettings.cs index 9a8a051..4195e70 100644 --- a/src/Common/PluginSettings.cs +++ b/src/Common/PluginSettings.cs @@ -98,6 +98,8 @@ public string LocalCacheRootPath public IReadOnlyCollection AllowFileAccessAfterProjectFinishFilePatterns { get; init; } = Array.Empty(); + public bool LogAllowFileAccessAfterProjectFinishMatchesAsMessages { get; init; } + public IReadOnlyCollection AllowProcessCloseAfterProjectFinishProcessPatterns { get; init; } = Array.Empty(); public IReadOnlyList GlobalPropertiesToIgnore { get; init; } = Array.Empty(); diff --git a/src/Common/build/Microsoft.MSBuildCache.Common.targets b/src/Common/build/Microsoft.MSBuildCache.Common.targets index 485ec72..6e63e51 100644 --- a/src/Common/build/Microsoft.MSBuildCache.Common.targets +++ b/src/Common/build/Microsoft.MSBuildCache.Common.targets @@ -18,6 +18,7 @@ $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheAsyncCacheMaterialization $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheAllowFileAccessAfterProjectFinishProcessPatterns $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheAllowFileAccessAfterProjectFinishFilePatterns + $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheLogAllowFileAccessAfterProjectFinishMatchesAsMessages $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheAllowProcessCloseAfterProjectFinishProcessPatterns $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheGlobalPropertiesToIgnore $(MSBuildCacheGlobalPropertiesToIgnore);MSBuildCacheGetResultsForUnqueriedDependencies @@ -43,6 +44,7 @@ $(MSBuildCacheAsyncCacheMaterialization) $(MSBuildCacheAllowFileAccessAfterProjectFinishProcessPatterns) $(MSBuildCacheAllowFileAccessAfterProjectFinishFilePatterns) + $(MSBuildCacheLogAllowFileAccessAfterProjectFinishMatchesAsMessages) $(MSBuildCacheAllowProcessCloseAfterProjectFinishProcessPatterns) $(MSBuildCacheGlobalPropertiesToIgnore) $(MSBuildCacheGetResultsForUnqueriedDependencies)