Fix RegFree manifest-file race under parallel MSBuild

[johnml1135]

Parallel FieldWorks builds can now generate registration-free COM manifests without racing when multiple MSBuild workers target the same output file. RegFree.Execute() serializes each manifest's complete read-modify-write operation across processes while unrelated manifests remain independent.

The key review question is whether the lock is narrow enough to preserve build parallelism but broad enough to protect every manifest mutation. The mutex is keyed by the normalized manifest path and encloses the load, update, write, and delete paths. Its name uses a deterministic digest because .NET string hash codes are process-specific. An abandoned mutex is treated as acquired, matching the platform contract, so the existing task error handling can report any damaged manifest instead of leaking an unhandled exception.

Where to look

• RegFree.Execute() owns the mutex for the complete manifest transaction and releases it in finally.
• ManifestLockName() creates the same lock identity in every worker without globally serializing different manifests.
• AbandonedMutexException handling preserves ownership and lets normal manifest validation and error reporting continue.
• RegFreeConcurrencyTests runs 12 tasks against one manifest and verifies every result plus the final XML content.

Deliberately not here

• No global build lock or serialization between unrelated manifest files.
• No COM registration, registry workaround, retry loop, or change to manifest contents.

Verification

./test.ps1 -TestProject Build/Src/FwBuildTasks/FwBuildTasksTests -StartedBy agent passed locally: 146 managed tests passed, 3 skipped, and the repository script's 31 native smoke tests passed. The full FieldWorks suite was not rerun locally after the rebase; CI remains the full gate.

---

Reading this a year from now -- start here

This focused branch carried no Markdown research or working notes to preserve or delete. The decision record lives here because the source comments intentionally retain only the local concurrency invariants a maintainer needs while reading the code.

Decisions, and why

• The lock covers the entire manifest transaction, not only XmlWriter.Create(). A worker must not read another worker's partially updated state or overwrite changes made after its own read.
• The lock is path-specific. Builds can still generate different manifests concurrently.
• The normalized full path is converted to a deterministic MD5 digest for the mutex name. string.GetHashCode() was rejected because its value can differ between worker processes. MD5 is used only as a stable identifier, not for security.
• AbandonedMutexException is a successful acquisition signal with warning semantics. Continuing under ownership allows the existing XML load and task error handling to detect and report a manifest left incomplete by the previous holder.

Regression evidence

The original failure was an intermittent IOException while parallel workers wrote the same manifest during PR #964; rerunning the same CI job passed, which isolated the problem to timing rather than deterministic input.

The regression test compiles a COM-visible assembly, launches 12 concurrent RegFree.Execute() calls against one manifest, requires every call to succeed, loads the result as XML, and verifies the expected clrClass. During development it failed in 3 of 3 runs with the mutex removed and passed in 5 of 5 runs with the mutex restored. The final squashed commit passed the complete FwBuildTasksTests project locally: 146 passed and 3 skipped.

---

[github-actions]

NUnit Tests

    1 files  ±0      1 suites  ±0   11m 4s ⏱️ -24s
5 761 tests ±0  5 680 ✅ ±0  81 💤 ±0  0 ❌ ±0 
5 770 runs  ±0  5 689 ✅ ±0  81 💤 ±0  0 ❌ ±0 

Results for commit 8d57a97. ± Comparison against base commit 1674067.

♻️ This comment has been updated with latest results.

[johnml1135]

Code review (verified against the code at the PR head)

Overall: the mutex approach is sound and the regression test (12 concurrent Execute() calls, verified to actually fail without the fix) is convincing evidence. One correctness gap worth closing before merge:

MEDIUM — WaitOne() isn't wrapped for AbandonedMutexException, so a crashed prior holder turns a handled build error into an unhandled one

RegFree.cs:

using (var manifestLock = new Mutex(false, ManifestLockName(manifestFile)))
{
    manifestLock.WaitOne();
    try
    {
        ...
    }
    catch (Exception e)
    {
        Log.LogErrorFromException(e, true, true, null);
        return false;
    }
    finally
    {
        manifestLock.ReleaseMutex();
    }
}

WaitOne() sits outside the try. If a previous MSBuild worker holding this mutex is killed mid-write (a build cancellation, taskkill, OOM), the next waiter's WaitOne() throws AbandonedMutexException — and since that's before the try, it propagates out of Execute() as an unhandled exception instead of going through the existing Log.LogErrorFromException path, and ReleaseMutex() in the finally never runs either (the finally only guards the try block it's attached to). MSBuild will still fail the task, but with a raw unhandled-exception task failure instead of the clean, logged error this PR is otherwise adding.

Note that AbandonedMutexException actually means ownership was granted, so the fix is a catch around WaitOne() that treats it as a successful acquire (and probably logs that the manifest content from the crashed writer should be treated as suspect, since it could reflect a torn write) — not a return false. Worth comparing with how PR #979 handles the same category of mutex (AcquireProjectMutex there does exactly this: try { m_projectMutex.WaitOne(); } catch (AbandonedMutexException) { }).

_{Reviewed with Claude Code; findings verified against the code at the PR head. No code changed.}

[johnml1135]

Fixed in 73ea066: wrapped WaitOne() in a catch (AbandonedMutexException) that proceeds as a normal acquire (ownership is still granted on that exception), so a crashed prior holder now produces the intended logged build error instead of an unhandled task failure. Verified the existing RegFreeConcurrencyTests still passes.

[jasonleenaylor]

@jasonleenaylor reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on johnml1135).

[jasonleenaylor]

@copilot resolve the merge conflicts in this pull request

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.05%. Comparing base (1674067) to head (8d57a97).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #988   +/-   ##
=======================================
  Coverage   38.05%   38.05%           
=======================================
  Files        1499     1499           
  Lines      350117   350117           
  Branches    40233    40233           
=======================================
+ Hits       133226   133234    +8     
+ Misses     187607   187598    -9     
- Partials    29284    29285    +1     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasonleenaylor]

@jasonleenaylor reviewed 3 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on johnml1135).