Recover from AbandonedMutexException in GlobalMutex (Windows)

[myieye]

A bit of a gutsy PR perhaps.

Fixes LT-21834 and a related shutdown-time crash from WritingSystemManager.Save → GlobalWritingSystemRepository.TryGet → GlobalMutex.Lock().

WindowsGlobalMutexAdapter.Wait() now catches AbandonedMutexException, traces a warning, and proceeds. Why this feels like a safe change:

• It matches the .NET Mutex contract verbatim. Per Microsoft's AbandonedMutexException docs: "Whether or not the exception was thrown, the current thread owns the mutex, and must release it." Subsequent _mutex.ReleaseMutex() in Release() succeeds.
• No diagnostic value is lost by catching. AbandonedMutexException carries no information about who abandoned the mutex or why — propagating it just produced an unhandled crash. The Trace.TraceWarning preserves the only signal that actually exists ("an abandonment was observed here").
• One fix covers both reported stack traces. Both GlobalMutex.Lock() (the new shutdown crash) and GlobalMutex.InitializeAndLock() → Init(initiallyOwned: true) (LT-21834) funnel through Wait(). The Init stack shows the call frame because Wait() is JIT-inlined; the try/catch is preserved through inlining.

As far as I know/understand, the one real downside of this fix is that we/users will stop seeing crashes and errors and although those errors will likely not be useful for pinpointing bugs, they do indicate that there is presumably code out there somewhere that is not handling the mutex correctly.

---

This change is 

[github-actions]

Palaso Tests

     4 files  ±0       4 suites  ±0   10m 43s ⏱️ + 1m 5s
 5 105 tests +2   4 869 ✅ ±0  235 💤 +1  1 ❌ +1 
16 627 runs  +6  15 901 ✅ +1  725 💤 +4  1 ❌ +1 

For more details on these failures, see this check.

Results for commit b6f209e. ± Comparison against base commit ba68a4a.

♻️ This comment has been updated with latest results.

[myieye]

This is obviously somewhat hairy. I'm not actually up for this right now.

[imnasnainaec]

Should an issue be opened instead?

[myieye]

I added a comment to the Jira issue pointing at the PR.

[tombogle]

This problem just bit Roopa in testing P10. I'm going to reopen it, so we can pursue further evaluation with an eye to getting it merged.

[lyonsil]

Reviewed the AbandonedMutexException recovery. The core change is mechanically correct - for a single-handle WaitOne(), AME still grants ownership, so proceeding and later ReleaseMutex() balance cleanly, and reentrancy/recursion counts stay consistent.

Requesting changes on one point: catching AME keeps the app running (good) but lets callers proceed over on-disk state a dead holder may have left torn, which converts a crash into silent data loss in Sldr.GetLdmlFile and GlobalWritingSystemRepository (details inline). Making the two mutex-protected writes atomic closes that, and I think it belongs in this PR since this change is what makes those paths reachable.

The rest are smaller: the recovery silently also covers macOS/non-Windows and InitializeAndLock() (doc/scope), plus a few test-robustness items (env-var leak, unguarded worker WaitOne, inaccurate skip message).

[lyonsil]

Recovering from the abandonment is right, but proceeding over the protected resource isn't safe yet - this turns a crash into silent data loss on two paths.

An abandoned mutex means a prior holder died mid-critical-section, so the on-disk state it was writing may be torn. Because the two writers below are non-atomic, a recovered caller now consumes/overwrites that torn state instead of aborting:

• Sldr.GetLdmlFile: MoveTmpToCache re-serializes the cache file in place (Sldr.cs:731); a holder killed there leaves a truncated cache. The recovered caller then File.Copy(sldrCacheFilePath, dest, overwrite:true) (Sldr.cs:377) overwrites the caller's good LDML with the truncated file and returns FromCache.
• GlobalWritingSystemRepository.SaveDefinition deletes then rewrites in place (GWSR.cs:372/384); a torn file makes UpdateDefinitions move it aside and eventually base.Remove(id) (GWSR.cs:142), silently dropping a writing system.

The durable fix is to make the mutex-protected writes atomic (write to a temp, then atomic replace). Then a dead holder can only ever leave a torn temp; the primary file is always the complete old or new version, and this recovery becomes provably safe. RobustFile already has the atomic primitive - Replace(src, dest, backup) (wraps File.Replace), so use Replace-if-exists-else-Move (note: RobustFile.Move(..., overwrite:true) is delete-then-move, not atomic):

// GWSR.SaveDefinition - drop the File.Delete at 370-373
var tmp = writingSystemFilePath + ".tmp";
using (new FileModeOverride())
    ldmlDataMapper.Write(tmp, ws, oldData);
if (RobustFile.Exists(writingSystemFilePath))
    RobustFile.Replace(tmp, writingSystemFilePath, null);
else
    RobustFile.Move(tmp, writingSystemFilePath);

// Sldr.MoveTmpToCache - use a distinct temp suffix (".tmp" is already the download temp)
var tmp = sldrCacheFilePath + ".new";
using (var writer = XmlWriter.Create(tmp, writerSettings))
    element.WriteTo(writer);
if (RobustFile.Exists(sldrCacheFilePath))
    RobustFile.Replace(tmp, sldrCacheFilePath, null);
else
    RobustFile.Move(tmp, sldrCacheFilePath);
File.Delete(filePath);

Two caveats with File.Replace: source/dest must be on the same volume (fine - sibling temp), and it copies the destination's ACLs onto the replacement, so please verify the FileModeOverride "775" group-write intent survives a replace on the Linux multi-user path.

Since this recovery is what makes those torn-state paths reachable, I think both writer fixes belong in this PR rather than a follow-up - otherwise the change ships a crash-to-silent-corruption swap, while the PR description frames the only downside as lost diagnostics. Making the two writes atomic here keeps the recovery and closes the corruption paths as one coherent change.

[lyonsil]

This also changes behavior on macOS and other non-Windows/non-Linux platforms, not just Windows. ExplicitGlobalMutexAdapter (selected for the else branch at line 58) extends WindowsGlobalMutexAdapter and inherits this Wait(), and it uses a real named Mutex that can throw AbandonedMutexException. So the recovery applies there too. That's fine functionally, but the PR title/CHANGELOG scope it to "(Windows)" and the new test skips every non-Windows platform (see the Assert.Ignore comment) - so the macOS path is silently changed and untested. Please widen the CHANGELOG wording (see the suggestion on the changelog bullet) and the test's skip message.

[lyonsil]

The fixture's env var is process-global and never restored, and this new guard leans on that. The [TestFixture(true)]/[TestFixture(false)] constructor sets/clears SIL_CORE_MAKE_GLOBAL_MUTEX_LOCAL_ONLY (lines 13-23) with no [OneTimeTearDown], so whichever fixture is constructed last leaves the value applied for the rest of the process - any later test that news up a GlobalMutex silently gets a different adapter. Recommend restoring the prior value in a teardown, and having this test read the fixture's own localOnly ctor arg (store it in a field) rather than re-reading the leaked env var.

[lyonsil]

Unguarded WaitOne() on a fixed OS-global name can crash the whole test process under concurrent runs. The name libpalaso-test-abandoned-mutex lives in the Windows session namespace, so two concurrent same-session test processes (e.g. CI running the net462/net48/net8.0 targets in parallel) can contend: process A's worker abandons it, and process B's worker's raw.WaitOne() returns WAIT_ABANDONED -> AbandonedMutexException on this background thread with no handler -> B's process terminates (not a clean test failure). Suggest a per-run unique name (append a GUID) or wrapping the worker's WaitOne() in a try/catch.

[lyonsil]

This skip reason is inaccurate for macOS and hides a coverage gap. "AbandonedMutexException only surfaces through the Windows named Mutex; flock and Monitor adapters cannot reach it" isn't quite true - on macOS the adapter is ExplicitGlobalMutexAdapter : WindowsGlobalMutexAdapter, a real named Mutex that runs this exact patched Wait() and can throw AME. The test only skips it because the raw new Mutex(false, name) here omits the Global\ prefix that ExplicitGlobalMutexAdapter adds. Please correct the message so a maintainer doesn't conclude the fix's code path can't run off Windows, and consider covering the macOS path.

[lyonsil]

The catch is in the shared Wait(), so it also covers InitializeAndLock() (via Init(initiallyOwned:true) -> Wait()), and it applies wherever a named Mutex is used, not only Windows. Suggest:

Suggested change

- [SIL.Core] `GlobalMutex.Lock()` (Windows) now recovers from `AbandonedMutexException` instead of propagating it. Per the .NET Mutex contract, the calling thread owns the mutex despite the exception; the prior behavior crashed callers (FieldWorks LT-21834 and a related shutdown crash) with no diagnostic gain. A `Trace.TraceWarning` is emitted on recovery.

- [SIL.Core] `GlobalMutex` (Windows, and macOS/other platforms that use a named Mutex) now recovers from `AbandonedMutexException` in `Lock()` and `InitializeAndLock()` instead of propagating it. Per the .NET Mutex contract, the calling thread owns the mutex despite the exception; the prior behavior crashed callers (FieldWorks LT-21834 and a related shutdown crash) with no diagnostic gain. A `Trace.TraceWarning` is emitted on recovery.

[lyonsil]

The trace actually fires for abandonment by any holder, including the cross-process/shutdown case that's the real motivation - not just in-process self-abandonment. Minor reword:

Suggested change

	// Per the .NET Mutex contract this thread owns the mutex despite the exception, so
	// we proceed and release as normal. AME carries no information about who abandoned
	// the mutex or why, so propagating it would just crash the host with no diagnostic
	// value. We still trace so self-abandonment by a thread in *this* process stays visible.
	// Per the .NET Mutex contract this thread owns the mutex despite the exception, so
	// we proceed and release as normal. AME carries no information about who abandoned
	// the mutex or why, so propagating it would just crash the host with no diagnostic
	// value. We still trace so any abandonment (this process or another) stays visible.