diff --git a/AGENTS.md b/AGENTS.md index 604c1a1931..c85ab85220 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,6 +14,13 @@ Minimal, high-signal guidance for coding agents in this repository. - Native C++ must build before managed projects (enforced by `FieldWorks.proj` + `build.ps1`). - FieldWorks uses registration-free COM; do not register COM globally and do not add registry hacks. - Keep localization in `.resx`; do not hardcode translatable UI strings. +- Comments: default terse — a one-line `///` summary on public members, and + inline `//` only where the code isn't self-evident. Expand (a block comment + or ``) only when a maintainer could reasonably get it wrong: + non-obvious rationale, concurrency/ordering hazards, interop/platform quirks + and workarounds, a deliberate anti-obvious choice, or bug-driven behavior + (cite the `LT-####`). Narrative that isn't load-bearing belongs in the + commit/PR, not inline. ## Context model diff --git a/Build/Src/FwBuildTasks/FwBuildTasksTests/RegFreeConcurrencyTests.cs b/Build/Src/FwBuildTasks/FwBuildTasksTests/RegFreeConcurrencyTests.cs new file mode 100644 index 0000000000..7746525856 --- /dev/null +++ b/Build/Src/FwBuildTasks/FwBuildTasksTests/RegFreeConcurrencyTests.cs @@ -0,0 +1,131 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using System.CodeDom.Compiler; +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; +using System.Threading.Tasks; +using System.Xml; +using Microsoft.CSharp; +using NUnit.Framework; +using SIL.FieldWorks.Build.Tasks; + +namespace SIL.FieldWorks.Build.Tasks.FwBuildTasksTests +{ + /// + /// Regression coverage for the RegFree task's cross-process manifest-file race: several EXE + /// projects (FieldWorks, LCMBrowser, UnicodeCharEditor, GenerateHCConfig, + /// ComManifestTestHost) each import RegFree.targets and generate manifests for the same + /// shared managed assemblies into the same $(OutDir); under a parallel MSBuild build, two of + /// them can race to read/write the exact same manifest file, throwing an IOException that + /// fails the whole build (observed in CI). These tests exercise multiple concurrent + /// executions targeting the identical output path in-process, which + /// reproduces the same file-sharing race the mutex fix in RegFree.cs (see + /// ManifestLockName) is meant to serialize. + /// + [TestFixture] + public sealed class RegFreeConcurrencyTests + { + private const string AsmNamespace = "urn:schemas-microsoft-com:asm.v1"; + + [Test] + public void Execute_ConcurrentInvocationsTargetingSameManifest_AllSucceedAndProduceValidXml() + { + var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(tempDir); + var assemblyPath = Path.Combine(tempDir, "RegFreeConcurrencyTestAssembly.dll"); + var manifestPath = Path.Combine(tempDir, "Shared.manifest"); + + try + { + CompileComVisibleAssembly(assemblyPath); + + // Simulates several EXE projects' MSBuild nodes each independently generating a + // manifest for the same shared assembly into the same output path at once. + const int concurrentInvocations = 12; + var results = new bool[concurrentInvocations]; + var tasks = new Task[concurrentInvocations]; + for (var i = 0; i < concurrentInvocations; i++) + { + var index = i; + tasks[index] = Task.Run(() => + { + var regFree = new RegFree + { + BuildEngine = new FwBuildTasks.TestBuildEngine(), + Executable = assemblyPath, + Output = manifestPath, + ManagedAssemblies = new Microsoft.Build.Utilities.TaskItem[] + { + new Microsoft.Build.Utilities.TaskItem(assemblyPath) + }, + Platform = "x64" + }; + results[index] = regFree.Execute(); + }); + } + + Task.WaitAll(tasks); + + Assert.That(results, Has.All.True, + "every concurrent RegFree.Execute() call must succeed - none should fail with the " + + "manifest-file-in-use IOException this test guards against"); + + Assert.That(File.Exists(manifestPath), Is.True, "the shared manifest file must exist after all writers finish"); + + // A corrupted/interleaved concurrent write would produce truncated or malformed XML; + // loading it here proves the final file is a single, complete, well-formed write. + var doc = new XmlDocument(); + Assert.DoesNotThrow(() => doc.Load(manifestPath), + "the manifest file must be well-formed XML, not truncated or interleaved by a concurrent write race"); + + var ns = new XmlNamespaceManager(doc.NameTable); + ns.AddNamespace("asmv1", AsmNamespace); + Assert.That(doc.SelectSingleNode("//asmv1:clrClass", ns), Is.Not.Null, + "the manifest must still contain the expected clrClass content after the concurrent writes"); + } + finally + { + if (Directory.Exists(tempDir)) + { + Directory.Delete(tempDir, true); + } + } + } + + private static void CompileComVisibleAssembly(string outputPath) + { + const string source = @"using System.Runtime.InteropServices; +[assembly: ComVisible(true)] +[assembly: Guid(""6A2C9E1D-6C8B-4B77-9C3E-6F6B6B2C9E1D"")] +namespace RegFreeConcurrencyTestAssembly +{ + [ComVisible(true)] + [Guid(""7B3DAF2E-7D9C-4C88-AD4F-7A7C7C3DAF2E"")] + [ProgId(""RegFreeConcurrency.SampleClass"")] + public class SampleComClass + { + } +}"; + var provider = new CSharpCodeProvider(); + var parameters = new CompilerParameters + { + GenerateExecutable = false, + OutputAssembly = outputPath, + CompilerOptions = "/target:library" + }; + parameters.ReferencedAssemblies.Add(typeof(object).Assembly.Location); + parameters.ReferencedAssemblies.Add(typeof(GuidAttribute).Assembly.Location); + + var results = provider.CompileAssemblyFromSource(parameters, source); + if (results.Errors.HasErrors) + { + var message = string.Join(Environment.NewLine, results.Errors.Cast().Select(e => e.ToString())); + throw new InvalidOperationException($"Failed to compile test assembly:{Environment.NewLine}{message}"); + } + } + } +} diff --git a/Build/Src/FwBuildTasks/RegFree.cs b/Build/Src/FwBuildTasks/RegFree.cs index c329e2eef4..14eed7c3d0 100644 --- a/Build/Src/FwBuildTasks/RegFree.cs +++ b/Build/Src/FwBuildTasks/RegFree.cs @@ -20,7 +20,10 @@ using System.Diagnostics; using System.IO; using System.Linq; +using System.Security.Cryptography; using System.Security.Principal; +using System.Text; +using System.Threading; using System.Windows.Forms; using System.Xml; using Microsoft.Build.Framework; @@ -181,185 +184,222 @@ public override bool Execute() ? Executable + ".manifest" : Output; - try + // Under a parallel MSBuild build, several EXE projects can process the same shared + // manifest in different worker processes at once and race to read/write it. This + // cross-process mutex serializes invocations targeting the same manifest file; + // invocations for different files are unaffected and still run fully in parallel. + using (var manifestLock = new Mutex(false, ManifestLockName(manifestFile))) { - var doc = new XmlDocument { PreserveWhitespace = true }; - - // Try to load existing manifest, or create empty document if it doesn't exist - if (File.Exists(manifestFile)) + try { - using (XmlReader reader = new XmlTextReader(manifestFile)) - { - if (reader.MoveToElement()) - doc.ReadNode(reader); - } + manifestLock.WaitOne(); } - else + catch (AbandonedMutexException) { - // Create a minimal valid XML document if manifest doesn't exist - Log.LogMessage( - MessageImportance.Low, - "\tCreating new manifest file {0}", - manifestFile - ); + // Prior holder crashed mid-write; .NET still grants us ownership here. } - - // Process all DLLs using direct type library parsing (no registry redirection needed) - var creator = new RegFreeCreator(doc, Log); - if (ExcludedClsids != null) + try { - creator.AddExcludedClsids(GetFilesFrom(ExcludedClsids)); - } + var doc = new XmlDocument { PreserveWhitespace = true }; - // Remove non-existing files from the list - itemsToProcess.RemoveAll(item => !File.Exists(item.ItemSpec)); + // Try to load existing manifest, or create empty document if it doesn't exist + if (File.Exists(manifestFile)) + { + using (XmlReader reader = new XmlTextReader(manifestFile)) + { + if (reader.MoveToElement()) + doc.ReadNode(reader); + } + } + else + { + // Create a minimal valid XML document if manifest doesn't exist + Log.LogMessage( + MessageImportance.Low, + "\tCreating new manifest file {0}", + manifestFile + ); + } - string assemblyName = Path.GetFileNameWithoutExtension(manifestFile); - if (assemblyName.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)) - { - assemblyName = Path.GetFileNameWithoutExtension(assemblyName); - } - Debug.Assert(assemblyName != null); - // The C++ test programs won't run if an assemblyIdentity element exists. - //if (assemblyName.StartsWith("test")) - // assemblyName = null; - string assemblyVersion = null; - try - { - assemblyVersion = FileVersionInfo.GetVersionInfo(Executable).FileVersion; - } - catch - { - // just ignore - } - if (string.IsNullOrEmpty(assemblyVersion)) - { - assemblyVersion = "1.0.0.0"; - } - else - { - // Ensure version has exactly 4 numeric parts for manifest compliance (Major.Minor.Build.Revision) - // Some assemblies might have 3-part versions (e.g. 1.1.0) or non-numeric suffixes - // (e.g. 9.3.5.local_20260119) which are invalid in SxS manifests. - // Always sanitize to extract only the leading digits from each part. - var parts = assemblyVersion.Split('.'); - var newParts = new string[4]; - for (int i = 0; i < 4; i++) + // Process all DLLs using direct type library parsing (no registry redirection needed) + var creator = new RegFreeCreator(doc, Log); + if (ExcludedClsids != null) + { + creator.AddExcludedClsids(GetFilesFrom(ExcludedClsids)); + } + + // Remove non-existing files from the list + itemsToProcess.RemoveAll(item => !File.Exists(item.ItemSpec)); + + string assemblyName = Path.GetFileNameWithoutExtension(manifestFile); + if (assemblyName.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)) + { + assemblyName = Path.GetFileNameWithoutExtension(assemblyName); + } + Debug.Assert(assemblyName != null); + // The C++ test programs won't run if an assemblyIdentity element exists. + //if (assemblyName.StartsWith("test")) + // assemblyName = null; + string assemblyVersion = null; + try { - // Simple parsing to ensure we only get numbers - string part = "0"; - if (i < parts.Length) + assemblyVersion = FileVersionInfo.GetVersionInfo(Executable).FileVersion; + } + catch + { + // just ignore + } + if (string.IsNullOrEmpty(assemblyVersion)) + { + assemblyVersion = "1.0.0.0"; + } + else + { + // Ensure version has exactly 4 numeric parts for manifest compliance (Major.Minor.Build.Revision) + // Some assemblies might have 3-part versions (e.g. 1.1.0) or non-numeric suffixes + // (e.g. 9.3.5.local_20260119) which are invalid in SxS manifests. + // Always sanitize to extract only the leading digits from each part. + var parts = assemblyVersion.Split('.'); + var newParts = new string[4]; + for (int i = 0; i < 4; i++) { - // Take only the leading digits from each version part - var digits = new string(parts[i].TakeWhile(char.IsDigit).ToArray()); - if (!string.IsNullOrEmpty(digits)) - part = digits; + // Simple parsing to ensure we only get numbers + string part = "0"; + if (i < parts.Length) + { + // Take only the leading digits from each version part + var digits = new string(parts[i].TakeWhile(char.IsDigit).ToArray()); + if (!string.IsNullOrEmpty(digits)) + part = digits; + } + newParts[i] = part; } - newParts[i] = part; + assemblyVersion = string.Join(".", newParts); } - assemblyVersion = string.Join(".", newParts); - } - XmlElement root = creator.CreateExeInfo(assemblyName, assemblyVersion, Platform); + XmlElement root = creator.CreateExeInfo(assemblyName, assemblyVersion, Platform); - foreach (string fileName in GetFilesFrom(ManagedAssemblies)) - { - Log.LogMessage( - MessageImportance.Low, - "\tProcessing managed assembly {0}", - Path.GetFileName(fileName) - ); - creator.ProcessManagedAssembly(root, fileName); - } + foreach (string fileName in GetFilesFrom(ManagedAssemblies)) + { + Log.LogMessage( + MessageImportance.Low, + "\tProcessing managed assembly {0}", + Path.GetFileName(fileName) + ); + creator.ProcessManagedAssembly(root, fileName); + } - foreach (var item in itemsToProcess) - { - string fileName = item.ItemSpec; - if (NoTypeLib.Count(f => f.ItemSpec == fileName) != 0) - continue; - - string server = item.GetMetadata("Server"); - if (string.IsNullOrEmpty(server)) - server = null; - - Log.LogMessage( - MessageImportance.Low, - "\tProcessing library {0}", - Path.GetFileName(fileName) - ); - - // Process type library directly (no registry redirection needed) - creator.ProcessTypeLibrary(root, fileName, server); - } + foreach (var item in itemsToProcess) + { + string fileName = item.ItemSpec; + if (NoTypeLib.Count(f => f.ItemSpec == fileName) != 0) + continue; + + string server = item.GetMetadata("Server"); + if (string.IsNullOrEmpty(server)) + server = null; + + Log.LogMessage( + MessageImportance.Low, + "\tProcessing library {0}", + Path.GetFileName(fileName) + ); + + // Process type library directly (no registry redirection needed) + creator.ProcessTypeLibrary(root, fileName, server); + } - // Process classes and interfaces from HKCR (where COM is already registered) - creator.ProcessClasses(root); - creator.ProcessInterfaces(root); + // Process classes and interfaces from HKCR (where COM is already registered) + creator.ProcessClasses(root); + creator.ProcessInterfaces(root); - foreach (string fragmentName in GetFilesFrom(Fragments)) - { - Log.LogMessage( - MessageImportance.Low, - "\tAdding fragment {0}", - Path.GetFileName(fragmentName) - ); - creator.AddFragment(root, fragmentName); - } + foreach (string fragmentName in GetFilesFrom(Fragments)) + { + Log.LogMessage( + MessageImportance.Low, + "\tAdding fragment {0}", + Path.GetFileName(fragmentName) + ); + creator.AddFragment(root, fragmentName); + } - foreach (string fragmentName in GetFilesFrom(AsIs)) - { - Log.LogMessage( - MessageImportance.Low, - "\tAdding as-is fragment {0}", - Path.GetFileName(fragmentName) - ); - creator.AddAsIs(root, fragmentName); - } + foreach (string fragmentName in GetFilesFrom(AsIs)) + { + Log.LogMessage( + MessageImportance.Low, + "\tAdding as-is fragment {0}", + Path.GetFileName(fragmentName) + ); + creator.AddAsIs(root, fragmentName); + } - foreach (string assemblyFileName in GetFilesFrom(DependentAssemblies)) - { - Log.LogMessage( - MessageImportance.Low, - "\tAdding dependent assembly {0}", - Path.GetFileName(assemblyFileName) - ); - creator.AddDependentAssembly(root, assemblyFileName); - } + foreach (string assemblyFileName in GetFilesFrom(DependentAssemblies)) + { + Log.LogMessage( + MessageImportance.Low, + "\tAdding dependent assembly {0}", + Path.GetFileName(assemblyFileName) + ); + creator.AddDependentAssembly(root, assemblyFileName); + } - if (!HasRegFreeContent(doc)) - { - Log.LogMessage( - MessageImportance.Low, - "\tNo registration-free content found for {0}; manifest will not be emitted.", - Path.GetFileName(manifestFile) - ); - if (File.Exists(manifestFile)) - File.Delete(manifestFile); - return true; - } + if (!HasRegFreeContent(doc)) + { + Log.LogMessage( + MessageImportance.Low, + "\tNo registration-free content found for {0}; manifest will not be emitted.", + Path.GetFileName(manifestFile) + ); + if (File.Exists(manifestFile)) + File.Delete(manifestFile); + return true; + } + + var settings = new XmlWriterSettings + { + OmitXmlDeclaration = false, + NewLineOnAttributes = false, + NewLineChars = Environment.NewLine, + Indent = true, + IndentChars = "\t", + }; + using (XmlWriter writer = XmlWriter.Create(manifestFile, settings)) + { + doc.WriteContentTo(writer); + } - var settings = new XmlWriterSettings + // Note: No unregistration needed - we never registered anything! + // Direct type library parsing doesn't touch the registry. + } + catch (Exception e) { - OmitXmlDeclaration = false, - NewLineOnAttributes = false, - NewLineChars = Environment.NewLine, - Indent = true, - IndentChars = "\t", - }; - using (XmlWriter writer = XmlWriter.Create(manifestFile, settings)) + Log.LogErrorFromException(e, true, true, null); + return false; + } + finally { - doc.WriteContentTo(writer); + manifestLock.ReleaseMutex(); } - - // Note: No unregistration needed - we never registered anything! - // Direct type library parsing doesn't touch the registry. } - catch (Exception e) + return true; + } + + /// ------------------------------------------------------------------------------------ + /// + /// Cross-process mutex name for the given manifest path. + /// is deliberately not used: .NET randomizes string hash codes per process, so different + /// worker processes could hash the same path differently and defeat the lock. MD5 is a + /// stable fingerprint here (not security-sensitive), deterministic across processes. + /// + /// ------------------------------------------------------------------------------------ + private static string ManifestLockName(string manifestFile) + { + var normalizedPath = Path.GetFullPath(manifestFile).ToUpperInvariant(); + using (var md5 = MD5.Create()) { - Log.LogErrorFromException(e, true, true, null); - return false; + var hash = md5.ComputeHash(Encoding.UTF8.GetBytes(normalizedPath)); + return "FwRegFreeManifest_" + BitConverter.ToString(hash).Replace("-", ""); } - return true; } private static bool HasRegFreeContent(XmlDocument doc)