From 0113d2bb9cab561a4b4b604c3226d378828b7c97 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Tue, 15 Sep 2026 08:20:49 +1000 Subject: [PATCH 01/14] Add failing test for package journal hang on abandoned holder On Windows, SystemSemaphoreManager.Acquire backs the package journal lock with a named Semaphore. A Semaphore has no notion of ownership, so when the holding Calamari process goes away without running the Releaser (killed mid-ApplyRetention, Tentacle restart, OOM) the count is never restored. The AbandonedMutexException handler in AcquireSemaphore can therefore never fire, and the unbounded WaitOne() waits forever - any concurrent deployment calling RegisterPackageUse or release-package-lock hangs with nothing but a single verbose line to show for it. This test asserts the behaviour we want - that Acquire recovers from an abandoned holder - so it fails until that is fixed. It abandons the holder through Acquire() itself rather than constructing a named Semaphore, which keeps it independent of the primitive and the name the implementation uses. Co-Authored-By: Claude Opus 5 --- .../WindowsSystemSemaphoreFixture.cs | 71 ++++++++++++++++++- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index b2baacefa2..36686b39c2 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -1,3 +1,8 @@ +using System; +using System.Runtime.Versioning; +using System.Threading; +using System.Threading.Tasks; +using Calamari.Common.Features.Processes.Semaphores; using Calamari.Testing.Helpers; using NUnit.Framework; @@ -5,6 +10,68 @@ namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores { [TestFixture] [Category(TestCategory.CompatibleOS.OnlyWindows)] + [SupportedOSPlatform("windows")] public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase - { } -} \ No newline at end of file + { + // On Windows, SystemSemaphoreManager backs Acquire() with a named Semaphore rather than a Mutex. + // A Semaphore has no notion of ownership, so when the holder goes away without running the Releaser + // the count is never restored, the AbandonedMutexException handler in AcquireSemaphore can never + // fire, and the unbounded WaitOne() waits forever. + + // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. + static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); + + // Held so the abandoned holder's handle is never closed or finalised, keeping the kernel object + // alive exactly as a second interested process would. Released in teardown. + IDisposable abandonedHolder; + + [TearDown] + public void ReleaseAbandonedHolder() + { + try + { + // Unblocks any waiter still stuck on the abandoned holder, so a failing test does not + // leave a permanently blocked thread pool thread behind. + abandonedHolder?.Dispose(); + } + catch (Exception) + { + // A Mutex can only be released by its owning thread, which has deliberately exited. + } + finally + { + abandonedHolder = null; + } + } + + [Test] + public void AcquireRecoversWhenTheHolderIsAbandoned() + { + var name = $"Octopus.Calamari.AbandonedHolder.{Guid.NewGuid():N}"; + var sut = new SystemSemaphoreManager(); + + // Take the semaphore on a thread that then exits without ever disposing the Releaser. This is + // what a Calamari process killed mid-ApplyRetention leaves behind: a holder that will never + // release. Going through Acquire() keeps this independent of which primitive and which name + // the implementation happens to use. + var holder = new Thread(() => abandonedHolder = sut.Acquire(name, "Another process is using the package journal")); + holder.Start(); + holder.Join(); + + // Acquire and release on the same thread: a Mutex can only be released by its owning thread, + // so handing the Releaser back to the test thread would throw regardless of the hang. + var acquire = Task.Run(() => + { + using (sut.Acquire(name, "Another process is using the package journal")) + { + } + }); + + Assert.That(acquire.Wait(RecoveryAllowance), + Is.True, + "Acquire() never returned after the holder was abandoned. A named Semaphore cannot signal " + + "abandonment, so the count stays at zero and the unbounded WaitOne() blocks forever. " + + "Using a Mutex on Windows would hand ownership to this waiter instead."); + } + } +} From 674bdb41add455a871bea76b0798916cd608e22d Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Tue, 15 Sep 2026 08:24:38 +1000 Subject: [PATCH 02/14] Use a Mutex on Windows so an abandoned lock can be recovered Replaces the named Semaphore in SystemSemaphoreManager with a named Mutex on Windows, which is what the *nix path already used. A Mutex has an owner, so when the holding thread or process terminates without releasing it the kernel signals the mutex as abandoned and hands ownership to the next waiter. The existing AbandonedMutexException handler - previously unreachable on Windows, because a Semaphore can never be abandoned - now does the job it was written for, and Acquire() recovers instead of blocking on its unbounded WaitOne(). This also corrects a naming inconsistency: the Windows path created the semaphore under the unprefixed name while building a `Global\` prefixed name only for the ACL, so two Calamari processes in different sessions took different locks and could write the journal concurrently. The lock is now consistently global on both platforms. The Everyone full-control ACL is kept (as MutexSecurity), so a lock taken by a Tentacle running as a service stays accessible to Calamari running under a different account, as does the Polly retry around creation. SystemSemaphoreManager is shared infrastructure, so WindowsX509CertificateStore gets the same recovery behaviour for its certificate store lock. Note this does not address the unbounded WaitOne() itself: a live process that holds the lock for a long time - ApplyRetention holds it across every package file deletion, each of which retries for up to a minute - will still stall other Calamari processes indefinitely. Co-Authored-By: Claude Opus 5 --- .../Semaphores/SystemSemaphoreManager.cs | 70 ++++++------------- .../WindowsSystemSemaphoreFixture.cs | 8 +-- 2 files changed, 24 insertions(+), 54 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 0e232ffd75..f6d3df441b 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -41,50 +41,17 @@ public SystemSemaphoreManager() public IDisposable Acquire(string name, string waitMessage) { - return OperatingSystem.IsWindows() - ? AcquireSemaphore(name, waitMessage) - : AcquireMutex(name, waitMessage); - } + var globalName = $@"Global\{name}"; - [SupportedOSPlatform("windows")] - IDisposable AcquireSemaphore(string name, string waitMessage) - { - var globalName = $"Global\\{name}"; - - //we try and create/acquire a global semaphore with some retry - //this is done to (hopefully) avoid situations where two instances of Calamari are trying to acquire the same semaphore + //we try and create/acquire a global mutex with some retry + //this is done to (hopefully) avoid situations where two instances of Calamari are trying to acquire the same mutex //this could happen in the case of parallel steps being executed on the same machine - var semaphore = semaphoreAcquisitionPipeline.Execute(() => new Semaphore(1,1, name)); - - //assign full control for all use - SetFullAccessControlForAllUsers(semaphore, globalName); - - try - { - if (!semaphore.WaitOne(initialWaitBeforeShowingLogMessage)) - { - log.Verbose(waitMessage); - semaphore.WaitOne(); - } - } - catch (AbandonedMutexException) - { - // We are now the owners of the mutex - // If a thread terminates while owning a mutex, the mutex is said to be abandoned. - // The state of the mutex is set to signaled and the next waiting thread gets ownership. - } + var mutex = semaphoreAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); - return new Releaser(() => - { - semaphore.Release(); - semaphore.Dispose(); - }); - } - - IDisposable AcquireMutex(string name, string waitMessage) - { - var globalName = $"Global\\{name}"; - var mutex = new Mutex(false, globalName); + //assign full control for all users, so that a lock taken by (say) a Tentacle running as a service + //is still accessible to Calamari running under a different account + if (OperatingSystem.IsWindows()) + SetFullAccessControlForAllUsers(mutex, globalName); try { @@ -96,9 +63,12 @@ IDisposable AcquireMutex(string name, string waitMessage) } catch (AbandonedMutexException) { - // We are now the owners of the mutex - // If a thread terminates while owning a mutex, the mutex is said to be abandoned. - // The state of the mutex is set to signaled and the next waiting thread gets ownership. + // We are now the owners of the mutex. + // If a thread or process terminates while owning a mutex, the mutex is said to be abandoned: + // the kernel signals it and hands ownership to the next waiter. This recovery is the reason a + // Mutex is used here rather than a Semaphore - a Semaphore has no notion of ownership, so a + // holder that died without releasing would leave its count at zero and block every later + // waiter forever. } return new Releaser(() => @@ -109,21 +79,21 @@ IDisposable AcquireMutex(string name, string waitMessage) } [SupportedOSPlatform("windows")] - void SetFullAccessControlForAllUsers(Semaphore semaphore, string name) + void SetFullAccessControlForAllUsers(Mutex mutex, string name) { - var semaphoreSecurity = new SemaphoreSecurity(); + var mutexSecurity = new MutexSecurity(); var everyone = new SecurityIdentifier(WellKnownSidType.WorldSid, null); - var rule = new SemaphoreAccessRule(everyone, SemaphoreRights.FullControl, AccessControlType.Allow); + var rule = new MutexAccessRule(everyone, MutexRights.FullControl, AccessControlType.Allow); - semaphoreSecurity.AddAccessRule(rule); + mutexSecurity.AddAccessRule(rule); try { - semaphore.SetAccessControl(semaphoreSecurity); + mutex.SetAccessControl(mutexSecurity); } catch (Exception e) { - log.Verbose($"Failed to set access controls on semaphore '{name}': {e.PrettyPrint()}"); + log.Verbose($"Failed to set access controls on mutex '{name}': {e.PrettyPrint()}"); } } diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index 36686b39c2..1913be2ab5 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -13,10 +13,10 @@ namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores [SupportedOSPlatform("windows")] public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase { - // On Windows, SystemSemaphoreManager backs Acquire() with a named Semaphore rather than a Mutex. - // A Semaphore has no notion of ownership, so when the holder goes away without running the Releaser - // the count is never restored, the AbandonedMutexException handler in AcquireSemaphore can never - // fire, and the unbounded WaitOne() waits forever. + // Acquire() must recover when the process holding the lock goes away without running the Releaser + // (killed mid-ApplyRetention, Tentacle restart, OOM). This is why the Windows path uses a named Mutex: + // an abandoned Mutex is signalled by the kernel and handed to the next waiter, whereas a Semaphore has + // no notion of ownership and would leave its count at zero, hanging every later waiter forever. // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); From f7faa3161911ee1882fcb4ab54d94200473ea243 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Tue, 15 Sep 2026 09:11:29 +1000 Subject: [PATCH 03/14] Tidy up after the Semaphore to Mutex switch Correct the retry log message, document on ISemaphoreFactory that the returned IDisposable must be disposed on the acquiring thread, and drop the test teardown that could never release the abandoned Mutex. Co-Authored-By: Claude Fable 5.1 --- .../Processes/Semaphores/ISemaphoreFactory.cs | 5 ++++ .../Semaphores/SystemSemaphoreManager.cs | 2 +- .../WindowsSystemSemaphoreFixture.cs | 29 ++++++------------- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs index 1240f3d196..642e29fd00 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs @@ -4,6 +4,11 @@ namespace Calamari.Common.Features.Processes.Semaphores { public interface ISemaphoreFactory { + /// + /// Acquires a machine-wide lock. The lock is backed by a named Mutex, so the returned + /// must be disposed on the same thread that called Acquire; + /// releasing from another thread (including after an await) throws. + /// IDisposable Acquire(string name, string waitMessage); } } \ No newline at end of file diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index f6d3df441b..7a88c4cc95 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -32,7 +32,7 @@ public SystemSemaphoreManager() Delay = TimeSpan.FromMilliseconds(50), OnRetry = args => { - log.Verbose($"Waiting {args.RetryDelay.TotalMilliseconds}ms before attempting to acquire the Semaphore again"); + log.Verbose($"Waiting {args.RetryDelay.TotalMilliseconds}ms before attempting to acquire the Mutex again"); return default; } }) diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index 1913be2ab5..620ed77083 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -21,27 +21,16 @@ public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); - // Held so the abandoned holder's handle is never closed or finalised, keeping the kernel object - // alive exactly as a second interested process would. Released in teardown. + // Held so the abandoned holder's handle is never closed or finalised during the test, keeping the + // kernel object alive exactly as a second interested process would. It cannot be released from here: + // a Mutex can only be released by its owning thread, which has deliberately exited. Dropping the + // reference in teardown lets the finaliser close the handle. IDisposable abandonedHolder; [TearDown] - public void ReleaseAbandonedHolder() + public void DropAbandonedHolder() { - try - { - // Unblocks any waiter still stuck on the abandoned holder, so a failing test does not - // leave a permanently blocked thread pool thread behind. - abandonedHolder?.Dispose(); - } - catch (Exception) - { - // A Mutex can only be released by its owning thread, which has deliberately exited. - } - finally - { - abandonedHolder = null; - } + abandonedHolder = null; } [Test] @@ -69,9 +58,9 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() Assert.That(acquire.Wait(RecoveryAllowance), Is.True, - "Acquire() never returned after the holder was abandoned. A named Semaphore cannot signal " - + "abandonment, so the count stays at zero and the unbounded WaitOne() blocks forever. " - + "Using a Mutex on Windows would hand ownership to this waiter instead."); + "Acquire() never returned after the holder was abandoned. The lock must be a named Mutex so " + + "that the kernel signals abandonment and hands ownership to this waiter; a Semaphore has no " + + "owner, so its count stays at zero and the unbounded WaitOne() blocks forever."); } } } From 425e8d9223cdc6ed1a5e211799692cc5b704a32e Mon Sep 17 00:00:00 2001 From: Luke Butters Date: Tue, 15 Sep 2026 10:33:37 +1000 Subject: [PATCH 04/14] Add test demonstrating Mutex-based lock release is thread-affine Acquire the lock in async code, hop onto a different thread-pool thread via Task.Delay/Task.Yield (what any real await after Acquire() does), then release from that thread and show it throws. This exercises the constraint already documented on ISemaphoreFactory.Acquire: a Windows Mutex can only be released by its owning thread, so holding the lock across an await is unsafe. Co-Authored-By: Claude Sonnet 5 --- .../WindowsSystemSemaphoreFixture.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index 620ed77083..b78fd41da3 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -62,5 +62,35 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() + "that the kernel signals abandonment and hands ownership to this waiter; a Semaphore has no " + "owner, so its count stays at zero and the unbounded WaitOne() blocks forever."); } + + [Test] + public async Task ReleasingFromADifferentThreadThanAcquiredThrows() + { + var name = $"Octopus.Calamari.CrossThreadRelease.{Guid.NewGuid():N}"; + var sut = new SystemSemaphoreManager(); + + // Acquire on whatever thread this async method happens to be running on right now - that thread + // becomes the Mutex's owner as far as the kernel is concerned. + var acquiringThread = Thread.CurrentThread.ManagedThreadId; + var releaser = sut.Acquire(name, "Another process is using the package journal"); + + // Hop onto other thread-pool threads via delay/yield, exactly what happens to any async method + // that awaits something after taking the lock. ConfigureAwait(false) lets the continuation land on + // whichever pool thread is free rather than being marshalled back, so this reliably changes threads. + int releasingThread; + do + { + await Task.Delay(1).ConfigureAwait(false); + await Task.Yield(); + releasingThread = Thread.CurrentThread.ManagedThreadId; + } while (releasingThread == acquiringThread); + + // Disposing here releases the Mutex from a thread other than the one that acquired it. A Mutex is + // owned by a specific thread (not the process), so the kernel rejects the release outright - this is + // exactly why ISemaphoreFactory.Acquire documents that the returned IDisposable must be disposed on + // the acquiring thread, and why async code cannot safely hold the lock across an await. + var ex = Assert.Throws(() => releaser.Dispose()); + Assert.That(ex.Message, Does.Contain("unsynchronized")); + } } } From e510d786376f1e9a94809fe633dca64d29febf42 Mon Sep 17 00:00:00 2001 From: Luke Butters Date: Tue, 15 Sep 2026 10:38:33 +1000 Subject: [PATCH 05/14] Acquire and release the Mutex on a dedicated owner thread A named Mutex can only be waited on and released by the thread that acquired it, but callers of ISemaphoreFactory.Acquire may reasonably dispose the returned IDisposable from a different thread than the one that called Acquire - most obviously, any async method that awaits something in between (as the previous commit's test demonstrated: ReleasingFromADifferentThreadThanAcquiredThrows). SystemSemaphoreManager now hides that constraint from callers: Acquire starts a dedicated background thread that performs the actual WaitOne()/ReleaseMutex() calls, and Acquire()/Dispose() just signal to and from it. The returned IDisposable can now be handed to, and disposed from, any thread. One consequence: the dedicated owner thread only exits once Dispose() signals it (or the process dies), so it's no longer possible to simulate an abandoned lock by having the *caller's* thread die - that thread was never the Mutex's real owner. Updated AcquireRecoversWhenTheHolderIsAbandoned to instead hold a raw named Mutex directly on the dying thread, modelling an external process/thread abandoning the lock, independent of SystemSemaphoreManager's own implementation. Co-Authored-By: Claude Sonnet 5 --- .../Processes/Semaphores/ISemaphoreFactory.cs | 7 +- .../Semaphores/SystemSemaphoreManager.cs | 93 +++++++++++++------ .../WindowsSystemSemaphoreFixture.cs | 54 +++++++---- 3 files changed, 104 insertions(+), 50 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs index 642e29fd00..faf0125793 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs @@ -5,9 +5,10 @@ namespace Calamari.Common.Features.Processes.Semaphores public interface ISemaphoreFactory { /// - /// Acquires a machine-wide lock. The lock is backed by a named Mutex, so the returned - /// must be disposed on the same thread that called Acquire; - /// releasing from another thread (including after an await) throws. + /// Acquires a machine-wide lock. The lock may be backed by a named Mutex, which can only be waited + /// on and released by the thread that acquired it; implementations are responsible for hiding that + /// constraint, so the returned is safe to dispose from any thread, including + /// after an await. /// IDisposable Acquire(string name, string waitMessage); } diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 7a88c4cc95..8f6ae7a6a7 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -1,4 +1,5 @@ using System; +using System.Runtime.ExceptionServices; using System.Runtime.Versioning; using System.Security.AccessControl; using System.Security.Principal; @@ -43,38 +44,76 @@ public IDisposable Acquire(string name, string waitMessage) { var globalName = $@"Global\{name}"; - //we try and create/acquire a global mutex with some retry - //this is done to (hopefully) avoid situations where two instances of Calamari are trying to acquire the same mutex - //this could happen in the case of parallel steps being executed on the same machine - var mutex = semaphoreAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); + // A Mutex can only be waited on and released by the thread that acquired it, but callers may + // dispose the returned IDisposable from a different thread than the one that called Acquire - + // most obviously, any async method that awaits something in between. So the actual WaitOne() and + // ReleaseMutex() calls happen on a dedicated thread that lives for exactly as long as the lock is + // held, and Acquire()/Dispose() just hand signals to and from it. + var acquired = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + Exception acquisitionFailure = null; - //assign full control for all users, so that a lock taken by (say) a Tentacle running as a service - //is still accessible to Calamari running under a different account - if (OperatingSystem.IsWindows()) - SetFullAccessControlForAllUsers(mutex, globalName); + var owner = new Thread(() => + { + Mutex mutex; + try + { + //we try and create/acquire a global mutex with some retry + //this is done to (hopefully) avoid situations where two instances of Calamari are trying to acquire the same mutex + //this could happen in the case of parallel steps being executed on the same machine + mutex = semaphoreAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); - try - { - if (!mutex.WaitOne(initialWaitBeforeShowingLogMessage)) - { - log.Verbose(waitMessage); - mutex.WaitOne(); - } - } - catch (AbandonedMutexException) - { - // We are now the owners of the mutex. - // If a thread or process terminates while owning a mutex, the mutex is said to be abandoned: - // the kernel signals it and hands ownership to the next waiter. This recovery is the reason a - // Mutex is used here rather than a Semaphore - a Semaphore has no notion of ownership, so a - // holder that died without releasing would leave its count at zero and block every later - // waiter forever. - } + //assign full control for all users, so that a lock taken by (say) a Tentacle running as a service + //is still accessible to Calamari running under a different account + if (OperatingSystem.IsWindows()) + SetFullAccessControlForAllUsers(mutex, globalName); + + try + { + if (!mutex.WaitOne(initialWaitBeforeShowingLogMessage)) + { + log.Verbose(waitMessage); + mutex.WaitOne(); + } + } + catch (AbandonedMutexException) + { + // We are now the owners of the mutex. + // If a thread or process terminates while owning a mutex, the mutex is said to be abandoned: + // the kernel signals it and hands ownership to the next waiter. This recovery is the reason a + // Mutex is used here rather than a Semaphore - a Semaphore has no notion of ownership, so a + // holder that died without releasing would leave its count at zero and block every later + // waiter forever. + } + } + catch (Exception ex) + { + acquisitionFailure = ex; + acquired.Set(); + return; + } + + acquired.Set(); + + release.Wait(); + + mutex.ReleaseMutex(); + mutex.Dispose(); + }) + { + IsBackground = true, + Name = $"Mutex owner for '{name}'" + }; + owner.Start(); + acquired.Wait(); + + if (acquisitionFailure != null) + ExceptionDispatchInfo.Capture(acquisitionFailure).Throw(); return new Releaser(() => { - mutex.ReleaseMutex(); - mutex.Dispose(); + release.Set(); + owner.Join(); }); } diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index b78fd41da3..0014af15b4 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -21,34 +21,38 @@ public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); - // Held so the abandoned holder's handle is never closed or finalised during the test, keeping the + // Held so the abandoned mutex's handle is never closed or finalised during the test, keeping the // kernel object alive exactly as a second interested process would. It cannot be released from here: // a Mutex can only be released by its owning thread, which has deliberately exited. Dropping the // reference in teardown lets the finaliser close the handle. - IDisposable abandonedHolder; + Mutex abandonedMutex; [TearDown] - public void DropAbandonedHolder() + public void DropAbandonedMutex() { - abandonedHolder = null; + abandonedMutex = null; } [Test] public void AcquireRecoversWhenTheHolderIsAbandoned() { var name = $"Octopus.Calamari.AbandonedHolder.{Guid.NewGuid():N}"; + var globalName = $@"Global\{name}"; var sut = new SystemSemaphoreManager(); - // Take the semaphore on a thread that then exits without ever disposing the Releaser. This is - // what a Calamari process killed mid-ApplyRetention leaves behind: a holder that will never - // release. Going through Acquire() keeps this independent of which primitive and which name - // the implementation happens to use. - var holder = new Thread(() => abandonedHolder = sut.Acquire(name, "Another process is using the package journal")); + // Simulate the lock being abandoned by some other process entirely (a Calamari process killed + // mid-ApplyRetention): take the raw named Mutex directly on a thread that then exits without ever + // releasing it. Acquire() now owns and releases its side of the lock on its own dedicated thread + // (see SystemSemaphoreManager), so it is no longer possible to simulate abandonment by having a + // caller's thread die - that thread was never the OS-level owner in the first place. + var holder = new Thread(() => + { + abandonedMutex = new Mutex(false, globalName); + abandonedMutex.WaitOne(); + }); holder.Start(); holder.Join(); - // Acquire and release on the same thread: a Mutex can only be released by its owning thread, - // so handing the Releaser back to the test thread would throw regardless of the hang. var acquire = Task.Run(() => { using (sut.Acquire(name, "Another process is using the package journal")) @@ -64,13 +68,12 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() } [Test] - public async Task ReleasingFromADifferentThreadThanAcquiredThrows() + public async Task ReleasingFromADifferentThreadThanAcquiredSucceeds() { var name = $"Octopus.Calamari.CrossThreadRelease.{Guid.NewGuid():N}"; var sut = new SystemSemaphoreManager(); - // Acquire on whatever thread this async method happens to be running on right now - that thread - // becomes the Mutex's owner as far as the kernel is concerned. + // Acquire on whatever thread this async method happens to be running on right now. var acquiringThread = Thread.CurrentThread.ManagedThreadId; var releaser = sut.Acquire(name, "Another process is using the package journal"); @@ -85,12 +88,23 @@ public async Task ReleasingFromADifferentThreadThanAcquiredThrows() releasingThread = Thread.CurrentThread.ManagedThreadId; } while (releasingThread == acquiringThread); - // Disposing here releases the Mutex from a thread other than the one that acquired it. A Mutex is - // owned by a specific thread (not the process), so the kernel rejects the release outright - this is - // exactly why ISemaphoreFactory.Acquire documents that the returned IDisposable must be disposed on - // the acquiring thread, and why async code cannot safely hold the lock across an await. - var ex = Assert.Throws(() => releaser.Dispose()); - Assert.That(ex.Message, Does.Contain("unsynchronized")); + // A raw Mutex can only be released by the thread that acquired it, so SystemSemaphoreManager + // acquires and releases on its own dedicated thread internally. Disposing here - from a thread + // other than the one that called Acquire() - must not throw. + Assert.DoesNotThrow(() => releaser.Dispose()); + + // And the lock must actually have been released: a second Acquire() should succeed promptly rather + // than hanging behind a lock nobody is going to release. + var reacquire = Task.Run(() => + { + using (sut.Acquire(name, "Another process is using the package journal")) + { + } + }); + + Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), + Is.True, + "Dispose() returned without actually releasing the mutex."); } } } From 847d1bf4a2eb4c08876185dedff7c74215b04390 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 11:03:23 +1000 Subject: [PATCH 06/14] Harden the Mutex owner thread and de-flake the cross-thread test Follow-ups from reviewing the dedicated owner thread added in #2159. Guard the release path. ReleaseMutex()/Dispose() run on our own thread rather than the caller's, so an exception there is unhandled and would terminate the process - a worse symptom than the hang this branch fixes. Releasing is now best effort and logs instead: a mutex we failed to release is abandoned, which the next waiter already recovers from. Dispose() still runs if ReleaseMutex() throws, so a failure doesn't leak the handle too. Dispose the two ManualResetEventSlims. Both waits block, so both allocate kernel handles that were only being reclaimed by the finaliser, once per acquisition. They are disposed after owner.Join() so the owner thread can never touch a disposed event, which in turn means a second Dispose() would have thrown ObjectDisposedException where it used to be harmless - so the Releaser is now idempotent. Dispose the Mutex on the acquisition failure path, where the handle was previously left to the finaliser. Nothing is released there, correctly: if WaitOne() threw we never owned it. ReleasingFromADifferentThreadThanAcquiredSucceeds now releases from an explicit thread instead of awaiting until a thread-pool continuation happens to land elsewhere. That loop was unbounded and had no timeout, so on a one-or-two core agent it could spin forever and hang CI rather than fail. Co-Authored-By: Claude Opus 5 --- .../Semaphores/SystemSemaphoreManager.cs | 40 ++++++++++++++++-- .../WindowsSystemSemaphoreFixture.cs | 41 +++++++++++-------- 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 8f6ae7a6a7..9cbce891bf 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -51,11 +51,11 @@ public IDisposable Acquire(string name, string waitMessage) // held, and Acquire()/Dispose() just hand signals to and from it. var acquired = new ManualResetEventSlim(false); var release = new ManualResetEventSlim(false); - Exception acquisitionFailure = null; + Exception? acquisitionFailure = null; var owner = new Thread(() => { - Mutex mutex; + Mutex? mutex = null; try { //we try and create/acquire a global mutex with some retry @@ -88,6 +88,9 @@ public IDisposable Acquire(string name, string waitMessage) } catch (Exception ex) { + //we never took the lock, so there is nothing to release - just + //close the handle rather than leaving it to the finaliser + mutex?.Dispose(); acquisitionFailure = ex; acquired.Set(); return; @@ -97,8 +100,24 @@ public IDisposable Acquire(string name, string waitMessage) release.Wait(); - mutex.ReleaseMutex(); - mutex.Dispose(); + //an unhandled exception here would terminate the process, because this + //is our own thread rather than the caller's. Releasing is best effort: + //if it fails the mutex is abandoned, which the next waiter recovers from. + try + { + try + { + mutex.ReleaseMutex(); + } + finally + { + mutex.Dispose(); + } + } + catch (Exception ex) + { + log.Verbose($"Failed to release the mutex '{globalName}': {ex.PrettyPrint()}"); + } }) { IsBackground = true, @@ -108,12 +127,25 @@ public IDisposable Acquire(string name, string waitMessage) acquired.Wait(); if (acquisitionFailure != null) + { + owner.Join(); + acquired.Dispose(); + release.Dispose(); ExceptionDispatchInfo.Capture(acquisitionFailure).Throw(); + } + + //guards against a caller disposing twice: the events are gone after the first time through + var released = 0; return new Releaser(() => { + if (Interlocked.Exchange(ref released, 1) != 0) + return; + release.Set(); owner.Join(); + acquired.Dispose(); + release.Dispose(); }); } diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index 0014af15b4..f09a334647 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -68,30 +68,39 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() } [Test] - public async Task ReleasingFromADifferentThreadThanAcquiredSucceeds() + public void ReleasingFromADifferentThreadThanAcquiredSucceeds() { var name = $"Octopus.Calamari.CrossThreadRelease.{Guid.NewGuid():N}"; var sut = new SystemSemaphoreManager(); - // Acquire on whatever thread this async method happens to be running on right now. - var acquiringThread = Thread.CurrentThread.ManagedThreadId; var releaser = sut.Acquire(name, "Another process is using the package journal"); - // Hop onto other thread-pool threads via delay/yield, exactly what happens to any async method - // that awaits something after taking the lock. ConfigureAwait(false) lets the continuation land on - // whichever pool thread is free rather than being marshalled back, so this reliably changes threads. - int releasingThread; - do - { - await Task.Delay(1).ConfigureAwait(false); - await Task.Yield(); - releasingThread = Thread.CurrentThread.ManagedThreadId; - } while (releasingThread == acquiringThread); + // Release from an explicit second thread rather than by hopping thread-pool threads after an await. + // The thread identity is then guaranteed to differ, with no dependency on how many cores the agent + // has - waiting for a pool continuation to land elsewhere can spin indefinitely on a small agent. + Exception releaseFailure = null; + var releasingThread = new Thread(() => + { + try + { + releaser.Dispose(); + } + catch (Exception ex) + { + releaseFailure = ex; + } + }); + releasingThread.Start(); + + Assert.That(releasingThread.Join(TimeSpan.FromSeconds(5)), Is.True, "Dispose() did not complete on the releasing thread."); // A raw Mutex can only be released by the thread that acquired it, so SystemSemaphoreManager - // acquires and releases on its own dedicated thread internally. Disposing here - from a thread - // other than the one that called Acquire() - must not throw. - Assert.DoesNotThrow(() => releaser.Dispose()); + // acquires and releases on its own dedicated thread internally. Disposing from another thread + // must not throw. + Assert.That(releaseFailure, + Is.Null, + "Disposing the releaser from a different thread threw. SystemSemaphoreManager must hide the " + + "Mutex's thread affinity from callers."); // And the lock must actually have been released: a second Acquire() should succeed promptly rather // than hanging behind a lock nobody is going to release. From 6de7fa0e9a5bcc710e40c328dcaa397f53c28f67 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 12:42:04 +1000 Subject: [PATCH 07/14] Trim comments in the semaphore manager and fixture Co-Authored-By: Claude Fable 5.1 --- .../Processes/Semaphores/ISemaphoreFactory.cs | 6 +-- .../Semaphores/SystemSemaphoreManager.cs | 32 +++++---------- .../WindowsSystemSemaphoreFixture.cs | 40 ++++--------------- 3 files changed, 19 insertions(+), 59 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs index faf0125793..70a83586d4 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs @@ -5,10 +5,8 @@ namespace Calamari.Common.Features.Processes.Semaphores public interface ISemaphoreFactory { /// - /// Acquires a machine-wide lock. The lock may be backed by a named Mutex, which can only be waited - /// on and released by the thread that acquired it; implementations are responsible for hiding that - /// constraint, so the returned is safe to dispose from any thread, including - /// after an await. + /// Acquires a machine-wide lock, backed by a Mutex. This will spawn a thread + /// to ensure the mutex is disposed on the same thread that created it. /// IDisposable Acquire(string name, string waitMessage); } diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 9cbce891bf..b36fc87dc1 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -44,27 +44,23 @@ public IDisposable Acquire(string name, string waitMessage) { var globalName = $@"Global\{name}"; - // A Mutex can only be waited on and released by the thread that acquired it, but callers may - // dispose the returned IDisposable from a different thread than the one that called Acquire - - // most obviously, any async method that awaits something in between. So the actual WaitOne() and - // ReleaseMutex() calls happen on a dedicated thread that lives for exactly as long as the lock is - // held, and Acquire()/Dispose() just hand signals to and from it. var acquired = new ManualResetEventSlim(false); var release = new ManualResetEventSlim(false); Exception? acquisitionFailure = null; + // A Mutex can only be waited on and released by the thread that acquired it var owner = new Thread(() => { Mutex? mutex = null; try { - //we try and create/acquire a global mutex with some retry - //this is done to (hopefully) avoid situations where two instances of Calamari are trying to acquire the same mutex - //this could happen in the case of parallel steps being executed on the same machine + // Wwe try and create/acquire a global semaphore mutex some retry + // to (hopefully) avoid situations where two instances of Calamari are trying to acquire the + // same mutex (e.g. parallel steps being executed on the same machine) mutex = semaphoreAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); - //assign full control for all users, so that a lock taken by (say) a Tentacle running as a service - //is still accessible to Calamari running under a different account + // Assign full control for all users, so that a lock taken by (say) a Tentacle running as a service + // is still accessible to Calamari running under a different account if (OperatingSystem.IsWindows()) SetFullAccessControlForAllUsers(mutex, globalName); @@ -78,18 +74,12 @@ public IDisposable Acquire(string name, string waitMessage) } catch (AbandonedMutexException) { - // We are now the owners of the mutex. - // If a thread or process terminates while owning a mutex, the mutex is said to be abandoned: - // the kernel signals it and hands ownership to the next waiter. This recovery is the reason a - // Mutex is used here rather than a Semaphore - a Semaphore has no notion of ownership, so a - // holder that died without releasing would leave its count at zero and block every later - // waiter forever. } } catch (Exception ex) { - //we never took the lock, so there is nothing to release - just - //close the handle rather than leaving it to the finaliser + // We never took the lock, so there is nothing to release - just + // close the handle rather than leaving it to the finaliser mutex?.Dispose(); acquisitionFailure = ex; acquired.Set(); @@ -100,9 +90,7 @@ public IDisposable Acquire(string name, string waitMessage) release.Wait(); - //an unhandled exception here would terminate the process, because this - //is our own thread rather than the caller's. Releasing is best effort: - //if it fails the mutex is abandoned, which the next waiter recovers from. + // An unhandled exception here would terminate the process - threads are great. try { try @@ -134,7 +122,7 @@ public IDisposable Acquire(string name, string waitMessage) ExceptionDispatchInfo.Capture(acquisitionFailure).Throw(); } - //guards against a caller disposing twice: the events are gone after the first time through + // Guards against a caller disposing twice: the events are gone after the first time through var released = 0; return new Releaser(() => diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index f09a334647..1e746458a1 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -14,17 +14,13 @@ namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase { // Acquire() must recover when the process holding the lock goes away without running the Releaser - // (killed mid-ApplyRetention, Tentacle restart, OOM). This is why the Windows path uses a named Mutex: - // an abandoned Mutex is signalled by the kernel and handed to the next waiter, whereas a Semaphore has - // no notion of ownership and would leave its count at zero, hanging every later waiter forever. + // (killed mid-ApplyRetention, Tentacle restart, OOM). + // Practically, this is why we use a mutex - a Semaphore won't cut it on Windows. // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); - // Held so the abandoned mutex's handle is never closed or finalised during the test, keeping the - // kernel object alive exactly as a second interested process would. It cannot be released from here: - // a Mutex can only be released by its owning thread, which has deliberately exited. Dropping the - // reference in teardown lets the finaliser close the handle. + // Held so the abandoned mutex's handle is never closed or finalised during the test Mutex abandonedMutex; [TearDown] @@ -40,11 +36,7 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() var globalName = $@"Global\{name}"; var sut = new SystemSemaphoreManager(); - // Simulate the lock being abandoned by some other process entirely (a Calamari process killed - // mid-ApplyRetention): take the raw named Mutex directly on a thread that then exits without ever - // releasing it. Acquire() now owns and releases its side of the lock on its own dedicated thread - // (see SystemSemaphoreManager), so it is no longer possible to simulate abandonment by having a - // caller's thread die - that thread was never the OS-level owner in the first place. + // Simulate the lock being abandoned by some other process entirely var holder = new Thread(() => { abandonedMutex = new Mutex(false, globalName); @@ -60,11 +52,7 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() } }); - Assert.That(acquire.Wait(RecoveryAllowance), - Is.True, - "Acquire() never returned after the holder was abandoned. The lock must be a named Mutex so " - + "that the kernel signals abandonment and hands ownership to this waiter; a Semaphore has no " - + "owner, so its count stays at zero and the unbounded WaitOne() blocks forever."); + Assert.That(acquire.Wait(RecoveryAllowance), Is.True, "Acquire() never returned after the holder was abandoned."); } [Test] @@ -75,9 +63,6 @@ public void ReleasingFromADifferentThreadThanAcquiredSucceeds() var releaser = sut.Acquire(name, "Another process is using the package journal"); - // Release from an explicit second thread rather than by hopping thread-pool threads after an await. - // The thread identity is then guaranteed to differ, with no dependency on how many cores the agent - // has - waiting for a pool continuation to land elsewhere can spin indefinitely on a small agent. Exception releaseFailure = null; var releasingThread = new Thread(() => { @@ -93,17 +78,8 @@ public void ReleasingFromADifferentThreadThanAcquiredSucceeds() releasingThread.Start(); Assert.That(releasingThread.Join(TimeSpan.FromSeconds(5)), Is.True, "Dispose() did not complete on the releasing thread."); + Assert.That(releaseFailure, Is.Null, "Disposing the releaser from a different thread threw."); - // A raw Mutex can only be released by the thread that acquired it, so SystemSemaphoreManager - // acquires and releases on its own dedicated thread internally. Disposing from another thread - // must not throw. - Assert.That(releaseFailure, - Is.Null, - "Disposing the releaser from a different thread threw. SystemSemaphoreManager must hide the " - + "Mutex's thread affinity from callers."); - - // And the lock must actually have been released: a second Acquire() should succeed promptly rather - // than hanging behind a lock nobody is going to release. var reacquire = Task.Run(() => { using (sut.Acquire(name, "Another process is using the package journal")) @@ -111,9 +87,7 @@ public void ReleasingFromADifferentThreadThanAcquiredSucceeds() } }); - Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), - Is.True, - "Dispose() returned without actually releasing the mutex."); + Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), Is.True, "Dispose() returned without actually releasing the mutex."); } } } From 409140d142789374bb0e12323e21a1a6c2212bd7 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 12:57:05 +1000 Subject: [PATCH 08/14] Run the abandonment and cross-thread release tests on all platforms The behaviours under test are properties of Mutex on every OS .NET supports, not just Windows, so the tests move into the shared fixture base. Co-Authored-By: Claude Fable 5.1 --- .../Processes/Semaphores/ISemaphoreFactory.cs | 3 +- .../Semaphores/SystemSemaphoreManager.cs | 9 +- .../Semaphores/SemaphoreFixtureBase.cs | 78 ++++++++++++++++- .../WindowsSystemSemaphoreFixture.cs | 85 +------------------ 4 files changed, 84 insertions(+), 91 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs index 70a83586d4..d37edbd864 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs @@ -5,8 +5,7 @@ namespace Calamari.Common.Features.Processes.Semaphores public interface ISemaphoreFactory { /// - /// Acquires a machine-wide lock, backed by a Mutex. This will spawn a thread - /// to ensure the mutex is disposed on the same thread that created it. + /// Acquires a machine-wide named lock. /// IDisposable Acquire(string name, string waitMessage); } diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index b36fc87dc1..6bbec2b952 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -54,9 +54,8 @@ public IDisposable Acquire(string name, string waitMessage) Mutex? mutex = null; try { - // Wwe try and create/acquire a global semaphore mutex some retry - // to (hopefully) avoid situations where two instances of Calamari are trying to acquire the - // same mutex (e.g. parallel steps being executed on the same machine) + // Create/acquire the global mutex with some retry, to (hopefully) avoid two instances of + // Calamari racing to create it (e.g. parallel steps on the same machine) mutex = semaphoreAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); // Assign full control for all users, so that a lock taken by (say) a Tentacle running as a service @@ -74,6 +73,7 @@ public IDisposable Acquire(string name, string waitMessage) } catch (AbandonedMutexException) { + // The previous owner died without releasing; the kernel has handed ownership to us } } catch (Exception ex) @@ -90,7 +90,8 @@ public IDisposable Acquire(string name, string waitMessage) release.Wait(); - // An unhandled exception here would terminate the process - threads are great. + // An unhandled exception here would terminate the process. Releasing is best effort: + // a failure leaves the mutex abandoned, which the next waiter recovers from. try { try diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs index 3ebf146ec1..a274c53f8b 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Threading; +using System.Threading.Tasks; using Calamari.Common.Features.Processes.Semaphores; using NUnit.Framework; @@ -8,6 +9,18 @@ namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores { public abstract class SemaphoreFixtureBase { + // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. + static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); + + // Held so the abandoned mutex handle is never closed or finalised during the test + Mutex abandonedMutex; + + [TearDown] + public void DropAbandonedMutex() + { + abandonedMutex = null; + } + [Test] public void SystemSemaphoreWaitsUntilFirstSemaphoreIsReleased() { @@ -20,13 +33,76 @@ public void SystemSemaphoreShouldIsolate() ShouldIsolate(new SystemSemaphoreManager()); } + // Acquire() must recover when the holder dies without running the Releaser (killed mid-ApplyRetention, + // Tentacle restart, OOM). + [Test] + public void AcquireRecoversWhenTheHolderIsAbandoned() + { + var name = $"Octopus.Calamari.AbandonedHolder.{Guid.NewGuid():N}"; + var globalName = $@"Global\{name}"; + var sut = new SystemSemaphoreManager(); + + // Simulate the lock being abandoned by some other process entirely + var holder = new Thread(() => + { + abandonedMutex = new Mutex(false, globalName); + abandonedMutex.WaitOne(); + }); + holder.Start(); + holder.Join(); + + var acquire = Task.Run(() => + { + using (sut.Acquire(name, "Another process is using the package journal")) + { + } + }); + + Assert.That(acquire.Wait(RecoveryAllowance), Is.True, "Acquire() never returned after the holder was abandoned."); + } + + [Test] + public void ReleasingFromADifferentThreadThanAcquiredSucceeds() + { + var name = $"Octopus.Calamari.CrossThreadRelease.{Guid.NewGuid():N}"; + var sut = new SystemSemaphoreManager(); + + var releaser = sut.Acquire(name, "Another process is using the package journal"); + + Exception releaseFailure = null; + var releasingThread = new Thread(() => + { + try + { + releaser.Dispose(); + } + catch (Exception ex) + { + releaseFailure = ex; + } + }); + releasingThread.Start(); + + Assert.That(releasingThread.Join(TimeSpan.FromSeconds(5)), Is.True, "Dispose() did not complete on the releasing thread."); + Assert.That(releaseFailure, Is.Null, "Disposing the releaser from a different thread threw."); + + var reacquire = Task.Run(() => + { + using (sut.Acquire(name, "Another process is using the package journal")) + { + } + }); + + Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), Is.True, "Dispose() returned without actually releasing the mutex."); + } + static void ShouldIsolate(ISemaphoreFactory semaphore) { var result = 0; var threads = new List(); for (var i = 0; i < 4; i++) - { + { threads.Add(new Thread(new ThreadStart(delegate { using (semaphore.Acquire("CalamariTest", "Another process is performing arithmetic, please wait")) diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs index 1e746458a1..81d1e5f69e 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs @@ -1,8 +1,3 @@ -using System; -using System.Runtime.Versioning; -using System.Threading; -using System.Threading.Tasks; -using Calamari.Common.Features.Processes.Semaphores; using Calamari.Testing.Helpers; using NUnit.Framework; @@ -10,84 +5,6 @@ namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores { [TestFixture] [Category(TestCategory.CompatibleOS.OnlyWindows)] - [SupportedOSPlatform("windows")] public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase - { - // Acquire() must recover when the process holding the lock goes away without running the Releaser - // (killed mid-ApplyRetention, Tentacle restart, OOM). - // Practically, this is why we use a mutex - a Semaphore won't cut it on Windows. - - // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. - static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); - - // Held so the abandoned mutex's handle is never closed or finalised during the test - Mutex abandonedMutex; - - [TearDown] - public void DropAbandonedMutex() - { - abandonedMutex = null; - } - - [Test] - public void AcquireRecoversWhenTheHolderIsAbandoned() - { - var name = $"Octopus.Calamari.AbandonedHolder.{Guid.NewGuid():N}"; - var globalName = $@"Global\{name}"; - var sut = new SystemSemaphoreManager(); - - // Simulate the lock being abandoned by some other process entirely - var holder = new Thread(() => - { - abandonedMutex = new Mutex(false, globalName); - abandonedMutex.WaitOne(); - }); - holder.Start(); - holder.Join(); - - var acquire = Task.Run(() => - { - using (sut.Acquire(name, "Another process is using the package journal")) - { - } - }); - - Assert.That(acquire.Wait(RecoveryAllowance), Is.True, "Acquire() never returned after the holder was abandoned."); - } - - [Test] - public void ReleasingFromADifferentThreadThanAcquiredSucceeds() - { - var name = $"Octopus.Calamari.CrossThreadRelease.{Guid.NewGuid():N}"; - var sut = new SystemSemaphoreManager(); - - var releaser = sut.Acquire(name, "Another process is using the package journal"); - - Exception releaseFailure = null; - var releasingThread = new Thread(() => - { - try - { - releaser.Dispose(); - } - catch (Exception ex) - { - releaseFailure = ex; - } - }); - releasingThread.Start(); - - Assert.That(releasingThread.Join(TimeSpan.FromSeconds(5)), Is.True, "Dispose() did not complete on the releasing thread."); - Assert.That(releaseFailure, Is.Null, "Disposing the releaser from a different thread threw."); - - var reacquire = Task.Run(() => - { - using (sut.Acquire(name, "Another process is using the package journal")) - { - } - }); - - Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), Is.True, "Dispose() returned without actually releasing the mutex."); - } - } + { } } From 09475bbcb77c8da90569de798ccc72184efb24b4 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 15:18:29 +1000 Subject: [PATCH 09/14] Log a warning when an abandoned lock is recovered The previous holder died inside the critical section, so whatever it was protecting may be inconsistent. Without this line the deployment log gave no hint that had happened. Co-Authored-By: Claude Fable 5.1 --- .../Features/Processes/Semaphores/SystemSemaphoreManager.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 6bbec2b952..8c53aff09f 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -74,6 +74,7 @@ public IDisposable Acquire(string name, string waitMessage) catch (AbandonedMutexException) { // The previous owner died without releasing; the kernel has handed ownership to us + log.Warn($"The lock '{name}' was abandoned by a previous process that exited without releasing it. Continuing, but anything it was protecting may have been left in an inconsistent state."); } } catch (Exception ex) From e7f796191017fc71ef63d87dbf3b76dbdfe90c47 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 15:35:37 +1000 Subject: [PATCH 10/14] Apply review feedback on comments and field naming Co-Authored-By: Claude Opus 5 --- .../Processes/Semaphores/ISemaphoreFactory.cs | 3 -- .../Semaphores/SystemSemaphoreManager.cs | 36 +++++++++---------- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs index d37edbd864..1240f3d196 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs @@ -4,9 +4,6 @@ namespace Calamari.Common.Features.Processes.Semaphores { public interface ISemaphoreFactory { - /// - /// Acquires a machine-wide named lock. - /// IDisposable Acquire(string name, string waitMessage); } } \ No newline at end of file diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 8c53aff09f..23068d62a9 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -16,28 +16,28 @@ public class SystemSemaphoreManager : ISemaphoreFactory { readonly ILog log; readonly int initialWaitBeforeShowingLogMessage; - readonly ResiliencePipeline semaphoreAcquisitionPipeline; + readonly ResiliencePipeline mutexAcquisitionPipeline; public SystemSemaphoreManager() { log = ConsoleLog.Instance; initialWaitBeforeShowingLogMessage = (int)TimeSpan.FromSeconds(3).TotalMilliseconds; - semaphoreAcquisitionPipeline = new ResiliencePipelineBuilder() - .AddRetry(new RetryStrategyOptions() - { - ShouldHandle = new PredicateBuilder().Handle(), - MaxRetryAttempts = 5, //means we'll wait for a max of around 250ms - BackoffType = DelayBackoffType.Linear, - UseJitter = true, - Delay = TimeSpan.FromMilliseconds(50), - OnRetry = args => - { - log.Verbose($"Waiting {args.RetryDelay.TotalMilliseconds}ms before attempting to acquire the Mutex again"); - return default; - } - }) - .Build(); + mutexAcquisitionPipeline = new ResiliencePipelineBuilder() + .AddRetry(new RetryStrategyOptions() + { + ShouldHandle = new PredicateBuilder().Handle(), + MaxRetryAttempts = 5, //means we'll wait for a max of around 250ms + BackoffType = DelayBackoffType.Linear, + UseJitter = true, + Delay = TimeSpan.FromMilliseconds(50), + OnRetry = args => + { + log.Verbose($"Waiting {args.RetryDelay.TotalMilliseconds}ms before attempting to acquire the Mutex again"); + return default; + } + }) + .Build(); } public IDisposable Acquire(string name, string waitMessage) @@ -56,7 +56,7 @@ public IDisposable Acquire(string name, string waitMessage) { // Create/acquire the global mutex with some retry, to (hopefully) avoid two instances of // Calamari racing to create it (e.g. parallel steps on the same machine) - mutex = semaphoreAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); + mutex = mutexAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); // Assign full control for all users, so that a lock taken by (say) a Tentacle running as a service // is still accessible to Calamari running under a different account @@ -79,7 +79,7 @@ public IDisposable Acquire(string name, string waitMessage) } catch (Exception ex) { - // We never took the lock, so there is nothing to release - just + // We never took the mutex, so there is nothing to release - just // close the handle rather than leaving it to the finaliser mutex?.Dispose(); acquisitionFailure = ex; From 3a53aa52d0255865ea2301448a9204e23cb6a4e5 Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 15:39:53 +1000 Subject: [PATCH 11/14] Move the release logic into the Releaser The double-dispose guard and the event/thread teardown now live in the type that owns them, rather than in a closure over locals in Acquire(). Co-Authored-By: Claude Opus 5 --- .../Semaphores/SystemSemaphoreManager.cs | 40 +++++++++---------- .../Semaphores/SemaphoreFixtureBase.cs | 21 ++++++++++ 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs index 23068d62a9..8151987a8a 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs @@ -116,27 +116,15 @@ public IDisposable Acquire(string name, string waitMessage) owner.Start(); acquired.Wait(); + var releaser = new Releaser(owner, acquired, release); + if (acquisitionFailure != null) { - owner.Join(); - acquired.Dispose(); - release.Dispose(); + releaser.Dispose(); ExceptionDispatchInfo.Capture(acquisitionFailure).Throw(); } - // Guards against a caller disposing twice: the events are gone after the first time through - var released = 0; - - return new Releaser(() => - { - if (Interlocked.Exchange(ref released, 1) != 0) - return; - - release.Set(); - owner.Join(); - acquired.Dispose(); - release.Dispose(); - }); + return releaser; } [SupportedOSPlatform("windows")] @@ -160,16 +148,28 @@ void SetFullAccessControlForAllUsers(Mutex mutex, string name) class Releaser : IDisposable { - readonly Action dispose; + readonly Thread owner; + readonly ManualResetEventSlim acquired; + readonly ManualResetEventSlim release; + int released; - public Releaser(Action dispose) + public Releaser(Thread owner, ManualResetEventSlim acquired, ManualResetEventSlim release) { - this.dispose = dispose; + this.owner = owner; + this.acquired = acquired; + this.release = release; } public void Dispose() { - dispose(); + // The events are gone after the first time through, so guard against a caller disposing twice + if (Interlocked.Exchange(ref released, 1) != 0) + return; + + release.Set(); + owner.Join(); + acquired.Dispose(); + release.Dispose(); } } } diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs index a274c53f8b..d98d39766c 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs @@ -96,6 +96,27 @@ public void ReleasingFromADifferentThreadThanAcquiredSucceeds() Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), Is.True, "Dispose() returned without actually releasing the mutex."); } + [Test] + public void DisposingTheReleaserTwiceIsANoOp() + { + var name = $"Octopus.Calamari.DoubleDispose.{Guid.NewGuid():N}"; + var sut = new SystemSemaphoreManager(); + + var releaser = sut.Acquire(name, "Another process is using the package journal"); + releaser.Dispose(); + + Assert.DoesNotThrow(() => releaser.Dispose(), "Disposing the releaser a second time threw."); + + var reacquire = Task.Run(() => + { + using (sut.Acquire(name, "Another process is using the package journal")) + { + } + }); + + Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), Is.True, "The second Dispose() interfered with the released mutex."); + } + static void ShouldIsolate(ISemaphoreFactory semaphore) { var result = 0; From 205101972a52c0c7c89ec0081f572bbb241ad4ef Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Wed, 16 Sep 2026 15:45:08 +1000 Subject: [PATCH 12/14] Rename Semaphore to NamedLock now the implementation uses a Mutex ISemaphoreFactory becomes INamedLockManager and SystemSemaphoreManager becomes MutexBasedNamedLockManager, with the namespace and test fixtures following. The lock name strings are unchanged. This is a breaking change for external consumers of the Octopus.Calamari.Common package that reference ISemaphoreFactory. Co-Authored-By: Claude Opus 5 --- .../Deployment/Journal/DeploymentJournal.cs | 16 ++++---- .../INamedLockManager.cs} | 4 +- .../MutexBasedNamedLockManager.cs} | 6 +-- .../Features/Scripting/ScriptExecutor.cs | 4 +- .../Journal/DeploymentJournalWriter.cs | 6 +-- .../Extensions/ApplicationDirectory.cs | 6 +-- .../DeploymentJournalVariableContributor.cs | 4 +- .../WindowsX509CertificateStore.cs | 18 ++++----- .../Fixtures/Deployment/CleanFixture.cs | 4 +- .../NamedLockFixtureBase.cs} | 40 +++++++++---------- .../NixNamedLockFixture.cs} | 4 +- .../WindowsNamedLockFixture.cs} | 4 +- .../PackageRetention/JournalFixture.cs | 14 +++---- ...ntlyUsedWithAgingSortPerformanceFixture.cs | 4 +- source/Calamari/Commands/CleanCommand.cs | 4 +- .../Calamari/Commands/DeployPackageCommand.cs | 6 +-- .../Commands/Java/DeployJavaArchiveCommand.cs | 6 +-- .../Commands/TransferPackageCommand.cs | 4 +- .../PackageRetention/Model/PackageJournal.cs | 20 +++++----- source/Calamari/Program.cs | 4 +- 20 files changed, 89 insertions(+), 89 deletions(-) rename source/Calamari.Common/Features/Processes/{Semaphores/ISemaphoreFactory.cs => NamedLocks/INamedLockManager.cs} (50%) rename source/Calamari.Common/Features/Processes/{Semaphores/SystemSemaphoreManager.cs => NamedLocks/MutexBasedNamedLockManager.cs} (98%) rename source/Calamari.Tests/Fixtures/Integration/Process/{Semaphores/SemaphoreFixtureBase.cs => NamedLocks/NamedLockFixtureBase.cs} (79%) rename source/Calamari.Tests/Fixtures/Integration/Process/{Semaphores/NixSystemSemaphoreFixture.cs => NamedLocks/NixNamedLockFixture.cs} (52%) rename source/Calamari.Tests/Fixtures/Integration/Process/{Semaphores/WindowsSystemSemaphoreFixture.cs => NamedLocks/WindowsNamedLockFixture.cs} (51%) diff --git a/source/Calamari.Common/Features/Deployment/Journal/DeploymentJournal.cs b/source/Calamari.Common/Features/Deployment/Journal/DeploymentJournal.cs index 2c6bd8584d..18d4e28e20 100644 --- a/source/Calamari.Common/Features/Deployment/Journal/DeploymentJournal.cs +++ b/source/Calamari.Common/Features/Deployment/Journal/DeploymentJournal.cs @@ -3,7 +3,7 @@ using System.IO; using System.Linq; using System.Xml.Linq; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; using Calamari.Common.Plumbing.Variables; @@ -12,16 +12,16 @@ namespace Calamari.Common.Features.Deployment.Journal { public class DeploymentJournal : IDeploymentJournal { - const string SemaphoreName = "Octopus.Calamari.DeploymentJournal"; + const string NamedLockName = "Octopus.Calamari.DeploymentJournal"; readonly ICalamariFileSystem fileSystem; - readonly ISemaphoreFactory semaphore; + readonly INamedLockManager namedLockManager; readonly IVariables variables; readonly ILog log; - public DeploymentJournal(ICalamariFileSystem fileSystem, ISemaphoreFactory semaphore, IVariables variables, ILog log) + public DeploymentJournal(ICalamariFileSystem fileSystem, INamedLockManager namedLockManager, IVariables variables, ILog log) { this.fileSystem = fileSystem; - this.semaphore = semaphore; + this.namedLockManager = namedLockManager; this.variables = variables; this.log = log; } @@ -30,7 +30,7 @@ public DeploymentJournal(ICalamariFileSystem fileSystem, ISemaphoreFactory semap internal void AddJournalEntry(JournalEntry entry) { - using (semaphore.Acquire(SemaphoreName, "Another process is using the deployment journal")) + using (namedLockManager.Acquire(NamedLockName, "Another process is using the deployment journal")) { var xElement = entry.ToXmlElement(); log.VerboseFormat("Adding journal entry:\n{0}", xElement.ToString()); @@ -40,7 +40,7 @@ internal void AddJournalEntry(JournalEntry entry) public List GetAllJournalEntries() { - using (semaphore.Acquire(SemaphoreName, "Another process is using the deployment journal")) + using (namedLockManager.Acquire(NamedLockName, "Another process is using the deployment journal")) { return Read().Select(element => new JournalEntry(element)).ToList(); } @@ -48,7 +48,7 @@ public List GetAllJournalEntries() public void RemoveJournalEntries(IEnumerable ids) { - using (semaphore.Acquire(SemaphoreName, "Another process is using the deployment journal")) + using (namedLockManager.Acquire(NamedLockName, "Another process is using the deployment journal")) { var elements = Read(); diff --git a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs b/source/Calamari.Common/Features/Processes/NamedLocks/INamedLockManager.cs similarity index 50% rename from source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs rename to source/Calamari.Common/Features/Processes/NamedLocks/INamedLockManager.cs index 1240f3d196..3d5491a9c3 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/ISemaphoreFactory.cs +++ b/source/Calamari.Common/Features/Processes/NamedLocks/INamedLockManager.cs @@ -1,8 +1,8 @@ using System; -namespace Calamari.Common.Features.Processes.Semaphores +namespace Calamari.Common.Features.Processes.NamedLocks { - public interface ISemaphoreFactory + public interface INamedLockManager { IDisposable Acquire(string name, string waitMessage); } diff --git a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs b/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs similarity index 98% rename from source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs rename to source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs index 8151987a8a..b381150650 100644 --- a/source/Calamari.Common/Features/Processes/Semaphores/SystemSemaphoreManager.cs +++ b/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs @@ -10,15 +10,15 @@ using Polly; using Polly.Retry; -namespace Calamari.Common.Features.Processes.Semaphores +namespace Calamari.Common.Features.Processes.NamedLocks { - public class SystemSemaphoreManager : ISemaphoreFactory + public class MutexBasedNamedLockManager : INamedLockManager { readonly ILog log; readonly int initialWaitBeforeShowingLogMessage; readonly ResiliencePipeline mutexAcquisitionPipeline; - public SystemSemaphoreManager() + public MutexBasedNamedLockManager() { log = ConsoleLog.Instance; initialWaitBeforeShowingLogMessage = (int)TimeSpan.FromSeconds(3).TotalMilliseconds; diff --git a/source/Calamari.Common/Features/Scripting/ScriptExecutor.cs b/source/Calamari.Common/Features/Scripting/ScriptExecutor.cs index 0994d63c08..4687c78f6a 100644 --- a/source/Calamari.Common/Features/Scripting/ScriptExecutor.cs +++ b/source/Calamari.Common/Features/Scripting/ScriptExecutor.cs @@ -2,7 +2,7 @@ using System.Collections.Generic; using System.IO; using Calamari.Common.Features.Processes; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; using Calamari.Common.Plumbing.Proxies; @@ -42,7 +42,7 @@ public CommandResult Execute(Script script, try { if (execution.CommandLineInvocation.Isolate) - using (new SystemSemaphoreManager() + using (new MutexBasedNamedLockManager() .Acquire("CalamariSynchronizeProcess", "Waiting for other process to finish executing script")) { diff --git a/source/Calamari.Common/Plumbing/Deployment/Journal/DeploymentJournalWriter.cs b/source/Calamari.Common/Plumbing/Deployment/Journal/DeploymentJournalWriter.cs index 45e6fac757..a2ddedafe9 100644 --- a/source/Calamari.Common/Plumbing/Deployment/Journal/DeploymentJournalWriter.cs +++ b/source/Calamari.Common/Plumbing/Deployment/Journal/DeploymentJournalWriter.cs @@ -2,7 +2,7 @@ using System.Linq; using Calamari.Common.Commands; using Calamari.Common.Features.Deployment.Journal; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; using Calamari.Common.Plumbing.Variables; @@ -31,8 +31,8 @@ public void AddJournalEntry(RunningDeployment deployment, bool wasSuccessful, st { if (deployment.SkipJournal) return; - var semaphore = new SystemSemaphoreManager(); - var journal = new DeploymentJournal(fileSystem, semaphore, deployment.Variables, log); + var namedLockManager = new MutexBasedNamedLockManager(); + var journal = new DeploymentJournal(fileSystem, namedLockManager, deployment.Variables, log); var hasPackages = !string.IsNullOrWhiteSpace(packageFile) || deployment.Variables.GetIndexes(PackageVariables.PackageCollection).Any(); diff --git a/source/Calamari.Common/Plumbing/Extensions/ApplicationDirectory.cs b/source/Calamari.Common/Plumbing/Extensions/ApplicationDirectory.cs index 3cd256b657..b277aeba86 100644 --- a/source/Calamari.Common/Plumbing/Extensions/ApplicationDirectory.cs +++ b/source/Calamari.Common/Plumbing/Extensions/ApplicationDirectory.cs @@ -1,7 +1,7 @@ using System; using System.IO; using Calamari.Common.Features.Packages; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Variables; @@ -9,7 +9,7 @@ namespace Calamari.Common.Plumbing.Extensions { public class ApplicationDirectory { - static readonly ISemaphoreFactory Semaphore = new SystemSemaphoreManager(); + static readonly INamedLockManager NamedLock = new MutexBasedNamedLockManager(); /// /// Returns the directory where the package will be installed. @@ -90,7 +90,7 @@ static string EnsureTargetPathExistsAndIsEmpty(string desiredTargetPath, ICalama { var target = desiredTargetPath; - using (Semaphore.Acquire("Octopus.Calamari.ExtractionDirectory", "Another process is finding an extraction directory, please wait...")) + using (NamedLock.Acquire("Octopus.Calamari.ExtractionDirectory", "Another process is finding an extraction directory, please wait...")) { for (var i = 1; fileSystem.DirectoryExists(target) || fileSystem.FileExists(target); i++) target = desiredTargetPath + "_" + i; diff --git a/source/Calamari.Common/Plumbing/Variables/DeploymentJournalVariableContributor.cs b/source/Calamari.Common/Plumbing/Variables/DeploymentJournalVariableContributor.cs index f9718bb7d1..95dc12a0bb 100644 --- a/source/Calamari.Common/Plumbing/Variables/DeploymentJournalVariableContributor.cs +++ b/source/Calamari.Common/Plumbing/Variables/DeploymentJournalVariableContributor.cs @@ -1,7 +1,7 @@ using System; using System.Linq; using Calamari.Common.Features.Deployment.Journal; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; @@ -15,7 +15,7 @@ public static void Contribute(ICalamariFileSystem fileSystem, IVariables variabl if (string.IsNullOrWhiteSpace(policySet)) return; - var journal = new DeploymentJournal(fileSystem, new SystemSemaphoreManager(), variables, log); + var journal = new DeploymentJournal(fileSystem, new MutexBasedNamedLockManager(), variables, log); Previous(variables, journal, policySet); PreviousSuccessful(variables, journal, policySet); } diff --git a/source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs b/source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs index a0f2103901..b1dcf3bfb2 100644 --- a/source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs +++ b/source/Calamari.Shared/Integration/Certificates/WindowsX509CertificateStore.cs @@ -10,7 +10,7 @@ using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Security.Principal; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Logging; using Calamari.Integration.Certificates.WindowsNative; using Org.BouncyCastle.Pkcs; @@ -24,8 +24,8 @@ namespace Calamari.Integration.Certificates public class WindowsX509CertificateStore : IWindowsX509CertificateStore { readonly ILog log; - public static readonly ISemaphoreFactory Semaphores = new SystemSemaphoreManager(); - public static readonly string SemaphoreName = nameof(WindowsX509CertificateStore); + public static readonly INamedLockManager NamedLocks = new MutexBasedNamedLockManager(); + public static readonly string NamedLockName = nameof(WindowsX509CertificateStore); const string IntermediateAuthorityStoreName = "CA"; public static readonly string RootAuthorityStoreName = "Root"; @@ -41,9 +41,9 @@ public WindowsX509CertificateStore(): this(ConsoleLog.Instance) } - private static IDisposable AcquireSemaphore() + private static IDisposable AcquireNamedLock() { - return Semaphores.Acquire(SemaphoreName, "Another process is working with the certificate store, please wait..."); + return NamedLocks.Acquire(NamedLockName, "Another process is working with the certificate store, please wait..."); } public string? FindCertificateStore(string thumbprint, StoreLocation storeLocation) @@ -67,7 +67,7 @@ private static IDisposable AcquireSemaphore() public void ImportCertificateToStore(byte[] pfxBytes, string password, StoreLocation storeLocation, string storeName, bool privateKeyExportable) { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { CertificateSystemStoreLocation systemStoreLocation; bool useUserKeyStore; @@ -96,7 +96,7 @@ public void ImportCertificateToStore(byte[] pfxBytes, string password, StoreLoca /// public void ImportCertificateToStore(byte[] pfxBytes, string password, string userName, string storeName, bool privateKeyExportable) { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { var account = new NTAccount(userName); var sid = (SecurityIdentifier) account.Translate(typeof(SecurityIdentifier)); @@ -125,7 +125,7 @@ public void AddPrivateKeyAccessRules(string thumbprint, StoreLocation storeLocat public void AddPrivateKeyAccessRules(string thumbprint, StoreLocation storeLocation, string storeName, ICollection privateKeyAccessRules) { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { var store = new X509Store(storeName, storeLocation); store.Open(OpenFlags.ReadWrite); @@ -152,7 +152,7 @@ public void AddPrivateKeyAccessRules(string thumbprint, StoreLocation storeLocat /// public void RemoveCertificateFromStore(string thumbprint, StoreLocation storeLocation, string storeName) { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { var store = new X509Store(storeName, storeLocation); store.Open(OpenFlags.ReadWrite); diff --git a/source/Calamari.Tests/Fixtures/Deployment/CleanFixture.cs b/source/Calamari.Tests/Fixtures/Deployment/CleanFixture.cs index 56e8e15a09..df4c55697a 100644 --- a/source/Calamari.Tests/Fixtures/Deployment/CleanFixture.cs +++ b/source/Calamari.Tests/Fixtures/Deployment/CleanFixture.cs @@ -3,7 +3,7 @@ using System.IO; using System.Xml.Linq; using Calamari.Common.Features.Deployment.Journal; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Commands; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Variables; @@ -41,7 +41,7 @@ public void SetUp() variables = new VariablesFactory(fileSystem, new SilentLog()).Create(new CommonOptions("test")); - deploymentJournal = new DeploymentJournal(fileSystem, new SystemSemaphoreManager(), variables, new SilentLog()); + deploymentJournal = new DeploymentJournal(fileSystem, new MutexBasedNamedLockManager(), variables, new SilentLog()); packagesDirectory = Path.Combine(Path.GetTempPath(), "CalamariTestPackages"); fileSystem.EnsureDirectoryExists(packagesDirectory); diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs b/source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/NamedLockFixtureBase.cs similarity index 79% rename from source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs rename to source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/NamedLockFixtureBase.cs index d98d39766c..02cc6a0aec 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/SemaphoreFixtureBase.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/NamedLockFixtureBase.cs @@ -2,14 +2,14 @@ using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using NUnit.Framework; -namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores +namespace Calamari.Tests.Fixtures.Integration.Process.NamedLocks { - public abstract class SemaphoreFixtureBase + public abstract class NamedLockFixtureBase { - // Must comfortably exceed the 3s initial wait inside SystemSemaphoreManager. + // Must comfortably exceed the 3s initial wait inside MutexBasedNamedLockManager. static readonly TimeSpan RecoveryAllowance = TimeSpan.FromSeconds(15); // Held so the abandoned mutex handle is never closed or finalised during the test @@ -22,15 +22,15 @@ public void DropAbandonedMutex() } [Test] - public void SystemSemaphoreWaitsUntilFirstSemaphoreIsReleased() + public void SecondNamedLockWaitsUntilFirstIsReleased() { - SecondSemaphoreWaitsUntilFirstSemaphoreIsReleased(new SystemSemaphoreManager()); + SecondWaitsUntilFirstIsReleased(new MutexBasedNamedLockManager()); } [Test] - public void SystemSemaphoreShouldIsolate() + public void NamedLockShouldIsolate() { - ShouldIsolate(new SystemSemaphoreManager()); + ShouldIsolate(new MutexBasedNamedLockManager()); } // Acquire() must recover when the holder dies without running the Releaser (killed mid-ApplyRetention, @@ -40,7 +40,7 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() { var name = $"Octopus.Calamari.AbandonedHolder.{Guid.NewGuid():N}"; var globalName = $@"Global\{name}"; - var sut = new SystemSemaphoreManager(); + var sut = new MutexBasedNamedLockManager(); // Simulate the lock being abandoned by some other process entirely var holder = new Thread(() => @@ -65,7 +65,7 @@ public void AcquireRecoversWhenTheHolderIsAbandoned() public void ReleasingFromADifferentThreadThanAcquiredSucceeds() { var name = $"Octopus.Calamari.CrossThreadRelease.{Guid.NewGuid():N}"; - var sut = new SystemSemaphoreManager(); + var sut = new MutexBasedNamedLockManager(); var releaser = sut.Acquire(name, "Another process is using the package journal"); @@ -100,7 +100,7 @@ public void ReleasingFromADifferentThreadThanAcquiredSucceeds() public void DisposingTheReleaserTwiceIsANoOp() { var name = $"Octopus.Calamari.DoubleDispose.{Guid.NewGuid():N}"; - var sut = new SystemSemaphoreManager(); + var sut = new MutexBasedNamedLockManager(); var releaser = sut.Acquire(name, "Another process is using the package journal"); releaser.Dispose(); @@ -117,7 +117,7 @@ public void DisposingTheReleaserTwiceIsANoOp() Assert.That(reacquire.Wait(TimeSpan.FromSeconds(5)), Is.True, "The second Dispose() interfered with the released mutex."); } - static void ShouldIsolate(ISemaphoreFactory semaphore) + static void ShouldIsolate(INamedLockManager namedLockManager) { var result = 0; var threads = new List(); @@ -126,7 +126,7 @@ static void ShouldIsolate(ISemaphoreFactory semaphore) { threads.Add(new Thread(new ThreadStart(delegate { - using (semaphore.Acquire("CalamariTest", "Another process is performing arithmetic, please wait")) + using (namedLockManager.Acquire("CalamariTest", "Another process is performing arithmetic, please wait")) { result = 1; Thread.Sleep(200); @@ -146,28 +146,28 @@ static void ShouldIsolate(ISemaphoreFactory semaphore) Assert.That(result, Is.EqualTo(3)); } - static void SecondSemaphoreWaitsUntilFirstSemaphoreIsReleased(ISemaphoreFactory semaphore) + static void SecondWaitsUntilFirstIsReleased(INamedLockManager namedLockManager) { AutoResetEvent autoEvent = new AutoResetEvent(false); - var threadTwoShouldGetSemaphore = true; + var threadTwoShouldGetTheLock = true; var threadOne = new Thread(() => { - using (semaphore.Acquire("Octopus.Calamari.TestSemaphore", "Another process has the semaphore...")) + using (namedLockManager.Acquire("Octopus.Calamari.TestNamedLock", "Another process has the lock...")) { - threadTwoShouldGetSemaphore = false; + threadTwoShouldGetTheLock = false; autoEvent.Set(); Thread.Sleep(200); - threadTwoShouldGetSemaphore = true; + threadTwoShouldGetTheLock = true; } }); var threadTwo = new Thread(() => { autoEvent.WaitOne(); - using (semaphore.Acquire("Octopus.Calamari.TestSemaphore", "Another process has the semaphore...")) + using (namedLockManager.Acquire("Octopus.Calamari.TestNamedLock", "Another process has the lock...")) { - Assert.That(threadTwoShouldGetSemaphore, Is.True); + Assert.That(threadTwoShouldGetTheLock, Is.True); } }); diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/NixSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/NixNamedLockFixture.cs similarity index 52% rename from source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/NixSystemSemaphoreFixture.cs rename to source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/NixNamedLockFixture.cs index cf91569434..6c8e9cbb7d 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/NixSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/NixNamedLockFixture.cs @@ -1,11 +1,11 @@ using Calamari.Testing.Helpers; using NUnit.Framework; -namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores +namespace Calamari.Tests.Fixtures.Integration.Process.NamedLocks { [TestFixture] [Category(TestCategory.CompatibleOS.OnlyNixOrMac)] - public class NixSystemSemaphoreFixture : SemaphoreFixtureBase + public class NixNamedLockFixture : NamedLockFixtureBase { } } \ No newline at end of file diff --git a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs b/source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/WindowsNamedLockFixture.cs similarity index 51% rename from source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs rename to source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/WindowsNamedLockFixture.cs index 81d1e5f69e..d88a373991 100644 --- a/source/Calamari.Tests/Fixtures/Integration/Process/Semaphores/WindowsSystemSemaphoreFixture.cs +++ b/source/Calamari.Tests/Fixtures/Integration/Process/NamedLocks/WindowsNamedLockFixture.cs @@ -1,10 +1,10 @@ using Calamari.Testing.Helpers; using NUnit.Framework; -namespace Calamari.Tests.Fixtures.Integration.Process.Semaphores +namespace Calamari.Tests.Fixtures.Integration.Process.NamedLocks { [TestFixture] [Category(TestCategory.CompatibleOS.OnlyWindows)] - public class WindowsSystemSemaphoreFixture : SemaphoreFixtureBase + public class WindowsNamedLockFixture : NamedLockFixtureBase { } } diff --git a/source/Calamari.Tests/Fixtures/PackageRetention/JournalFixture.cs b/source/Calamari.Tests/Fixtures/PackageRetention/JournalFixture.cs index 4b06e6c491..5a4181512b 100644 --- a/source/Calamari.Tests/Fixtures/PackageRetention/JournalFixture.cs +++ b/source/Calamari.Tests/Fixtures/PackageRetention/JournalFixture.cs @@ -2,7 +2,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Deployment.PackageRetention; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; @@ -40,7 +40,7 @@ public void Setup() Substitute.For(), Substitute.For(), Substitute.For>(), - Substitute.For() + Substitute.For() ); } @@ -169,7 +169,7 @@ public void WhenRetentionIsApplied_ThenPackageFileAndUsageAreRemoved() Substitute.For(), fileSystem, new []{ retentionAlgorithm }, - Substitute.For()); + Substitute.For()); thisJournal.RegisterPackageUse(packageOne, new ServerTaskId("Deployment-1"), 1000); thisJournal.ApplyRetention(); @@ -200,7 +200,7 @@ public void WhenRetentionIsAppliedAndCacheSpaceIsNotSufficient_ThenPackageFileAn Substitute.For(), fileSystem, new []{ retentionAlgorithm }, - Substitute.For()); + Substitute.For()); thisJournal.RegisterPackageUse(existingPackage, new ServerTaskId("Deployment-1"), 1 * 1024 * 1024); //Package is 1 MB thisJournal.ApplyRetention(); @@ -231,7 +231,7 @@ public void WhenRetentionIsAppliedAndCacheSpaceIsSufficientButDiskSpaceIsNot_The Substitute.For(), fileSystem, new []{ retentionAlgorithm }, - Substitute.For()); + Substitute.For()); thisJournal.RegisterPackageUse(existingPackage, new ServerTaskId("Deployment-1"), 1 * 1024 * 1024); //Package is 1 MB thisJournal.ApplyRetention(); @@ -265,7 +265,7 @@ public void WhenStaleLocksAreExpired_TheLocksAreRemoved() Substitute.For(), Substitute.For(), Substitute.For>(), - Substitute.For()); + Substitute.For()); testJournal.ExpireStaleLocks(TimeSpan.FromDays(14)); Assert.IsFalse(testJournalRepository.HasLock(thePackage)); @@ -301,7 +301,7 @@ public void OnlyStaleLocksAreExpired() Substitute.For(), Substitute.For(), Substitute.For>(), - Substitute.For()); + Substitute.For()); testJournal.ExpireStaleLocks(TimeSpan.FromDays(14)); Assert.IsFalse(testJournalRepository.HasLock(packageOne)); diff --git a/source/Calamari.Tests/Fixtures/PackageRetention/LeastFrequentlyUsedWithAgingSortPerformanceFixture.cs b/source/Calamari.Tests/Fixtures/PackageRetention/LeastFrequentlyUsedWithAgingSortPerformanceFixture.cs index cba128a0b1..0dbc0bad97 100644 --- a/source/Calamari.Tests/Fixtures/PackageRetention/LeastFrequentlyUsedWithAgingSortPerformanceFixture.cs +++ b/source/Calamari.Tests/Fixtures/PackageRetention/LeastFrequentlyUsedWithAgingSortPerformanceFixture.cs @@ -2,7 +2,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Deployment.PackageRetention; using Calamari.Common.Plumbing.Logging; using Calamari.Deployment.PackageRetention.Caching; @@ -42,7 +42,7 @@ static InMemoryJournalRepository SeedJournal() Substitute.For(), new TestCalamariPhysicalFileSystem(), Substitute.For>(), - new SystemSemaphoreManager()); + new MutexBasedNamedLockManager()); var serverTask = new ServerTaskId("ServerTasks-1"); for (var i = 0; i < 50000; i++) { diff --git a/source/Calamari/Commands/CleanCommand.cs b/source/Calamari/Commands/CleanCommand.cs index 049d962cc2..e9203e8329 100644 --- a/source/Calamari/Commands/CleanCommand.cs +++ b/source/Calamari/Commands/CleanCommand.cs @@ -1,7 +1,7 @@ using Calamari.Commands.Support; using Calamari.Common.Commands; using Calamari.Common.Features.Deployment.Journal; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; @@ -42,7 +42,7 @@ public override int Execute(string[] commandLineArguments) if (days <=0 && releases <= 0) throw new CommandException("A value must be provided for either --days or --releases"); - var deploymentJournal = new DeploymentJournal(fileSystem, new SystemSemaphoreManager(), variables, log); + var deploymentJournal = new DeploymentJournal(fileSystem, new MutexBasedNamedLockManager(), variables, log); var clock = new SystemClock(); var retentionPolicy = new RetentionPolicy(fileSystem, deploymentJournal, clock, log); diff --git a/source/Calamari/Commands/DeployPackageCommand.cs b/source/Calamari/Commands/DeployPackageCommand.cs index bc64582213..5be0ce41d5 100644 --- a/source/Calamari/Commands/DeployPackageCommand.cs +++ b/source/Calamari/Commands/DeployPackageCommand.cs @@ -11,7 +11,7 @@ using Calamari.Common.Features.EmbeddedResources; using Calamari.Common.Features.Packages; using Calamari.Common.Features.Processes; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Features.Scripting; using Calamari.Common.Features.StructuredVariables; using Calamari.Common.Features.Substitutions; @@ -101,8 +101,8 @@ public override int Execute(string[] commandLineArguments) featureClasses.Add(new NginxFeature(NginxServer.AutoDetect(), fileSystem, log)); } - var semaphore = new SystemSemaphoreManager(); - var journal = new DeploymentJournal(fileSystem, semaphore, variables, log); + var namedLockManager = new MutexBasedNamedLockManager(); + var journal = new DeploymentJournal(fileSystem, namedLockManager, variables, log); var conventions = new List { diff --git a/source/Calamari/Commands/Java/DeployJavaArchiveCommand.cs b/source/Calamari/Commands/Java/DeployJavaArchiveCommand.cs index ff035c3c03..a33992d5cf 100644 --- a/source/Calamari/Commands/Java/DeployJavaArchiveCommand.cs +++ b/source/Calamari/Commands/Java/DeployJavaArchiveCommand.cs @@ -11,7 +11,7 @@ using Calamari.Common.Features.Packages.Decorators; using Calamari.Common.Features.Packages.Java; using Calamari.Common.Features.Processes; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Features.Scripting; using Calamari.Common.Features.StructuredVariables; using Calamari.Common.Features.Substitutions; @@ -78,8 +78,8 @@ public override int Execute(string[] commandLineArguments) log.Info("Deploying: " + archiveFile); - var semaphore = new SystemSemaphoreManager(); - var journal = new DeploymentJournal(fileSystem, semaphore, variables, log); + var namedLockManager = new MutexBasedNamedLockManager(); + var journal = new DeploymentJournal(fileSystem, namedLockManager, variables, log); var jarTools = new JarTool(commandLineRunner, log, fileSystem, variables); var packageExtractor = new JarPackageExtractor(jarTools).WithExtractionLimits(log, variables); var embeddedResources = new AssemblyEmbeddedResources(); diff --git a/source/Calamari/Commands/TransferPackageCommand.cs b/source/Calamari/Commands/TransferPackageCommand.cs index 745a04555c..d0cc4075c5 100644 --- a/source/Calamari/Commands/TransferPackageCommand.cs +++ b/source/Calamari/Commands/TransferPackageCommand.cs @@ -3,7 +3,7 @@ using Calamari.Commands.Support; using Calamari.Common.Commands; using Calamari.Common.Features.Deployment.Journal; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Deployment.Journal; using Calamari.Common.Plumbing.Extensions; using Calamari.Common.Plumbing.FileSystem; @@ -37,7 +37,7 @@ public override int Execute(string[] commandLineArguments) if (packageFile == null) // required: true in the above call means it will throw rather than return null, but there's no way to tell the compiler that. And ! doesn't work in older frameworks throw new CommandException("Package File path could not be determined"); - var journal = new DeploymentJournal(fileSystem, new SystemSemaphoreManager(), variables, log); + var journal = new DeploymentJournal(fileSystem, new MutexBasedNamedLockManager(), variables, log); var conventions = new List { diff --git a/source/Calamari/Deployment/PackageRetention/Model/PackageJournal.cs b/source/Calamari/Deployment/PackageRetention/Model/PackageJournal.cs index 4025c03672..91cbcb3720 100644 --- a/source/Calamari/Deployment/PackageRetention/Model/PackageJournal.cs +++ b/source/Calamari/Deployment/PackageRetention/Model/PackageJournal.cs @@ -1,7 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Deployment.PackageRetention; using Calamari.Common.Plumbing.FileSystem; using Calamari.Common.Plumbing.Logging; @@ -16,26 +16,26 @@ public class PackageJournal : IManagePackageCache readonly IRetentionAlgorithm[] retentionAlgorithms; readonly ILog log; readonly ICalamariFileSystem fileSystem; - readonly ISemaphoreFactory semaphoreFactory; + readonly INamedLockManager namedLockManager; public PackageJournal(IJournalRepository journalRepository, ILog log, ICalamariFileSystem fileSystem, IEnumerable retentionAlgorithms, - ISemaphoreFactory semaphoreFactory) + INamedLockManager namedLockManager) { this.journalRepository = journalRepository; this.log = log; this.fileSystem = fileSystem; this.retentionAlgorithms = retentionAlgorithms.ToArray(); - this.semaphoreFactory = semaphoreFactory; + this.namedLockManager = namedLockManager; } public void RegisterPackageUse(PackageIdentity package, ServerTaskId deploymentTaskId, ulong packageSizeBytes) { try { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { journalRepository.Load(); journalRepository.Cache.IncrementCacheAge(); @@ -66,7 +66,7 @@ public void RegisterPackageUse(PackageIdentity package, ServerTaskId deploymentT public void RemoveAllLocks(ServerTaskId serverTaskId) { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { log.Verbose($"Releasing package locks for task {serverTaskId}"); journalRepository.Load(); @@ -79,7 +79,7 @@ public void ApplyRetention() { try { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { journalRepository.Load(); var packagesToRemove = retentionAlgorithms.SelectMany(algorithm => algorithm.GetPackagesToRemove(journalRepository.GetAllJournalEntries())); @@ -110,7 +110,7 @@ public void ExpireStaleLocks(TimeSpan timeBeforeExpiration) { try { - using (AcquireSemaphore()) + using (AcquireNamedLock()) { journalRepository.Load(); foreach (var entry in journalRepository.GetAllJournalEntries()) @@ -134,9 +134,9 @@ public void ExpireStaleLocks(TimeSpan timeBeforeExpiration) } } - IDisposable AcquireSemaphore() + IDisposable AcquireNamedLock() { - return semaphoreFactory.Acquire(nameof(PackageJournal), "Another process is using the package journal"); + return namedLockManager.Acquire(nameof(PackageJournal), "Another process is using the package journal"); } } } \ No newline at end of file diff --git a/source/Calamari/Program.cs b/source/Calamari/Program.cs index 91751d4ad5..5185c38d1d 100644 --- a/source/Calamari/Program.cs +++ b/source/Calamari/Program.cs @@ -13,7 +13,7 @@ using Calamari.Common; using Calamari.Common.Commands; using Calamari.Common.Features.Discovery; -using Calamari.Common.Features.Processes.Semaphores; +using Calamari.Common.Features.Processes.NamedLocks; using Calamari.Common.Plumbing.Commands; using Calamari.Common.Plumbing.Deployment.Journal; using Calamari.Common.Plumbing.Deployment.PackageRetention; @@ -119,7 +119,7 @@ protected override void ConfigureContainer(ContainerBuilder builder, CommonOptio .As() .SingleInstance(); - builder.RegisterInstance(new SystemSemaphoreManager()).As(); + builder.RegisterInstance(new MutexBasedNamedLockManager()).As(); TypeDescriptor.AddAttributes(typeof(ServerTaskId), new TypeConverterAttribute(typeof(TinyTypeTypeConverter))); From 3a20da5f6398156b11776d6243c656554cd602cc Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Thu, 17 Sep 2026 10:11:44 +1000 Subject: [PATCH 13/14] Move the owner-thread coordination into a TrackedThread helper The acquired/release events and the owner thread now live together in TrackedThread rather than being threaded through the Releaser by hand. No behaviour change. Co-Authored-By: Claude Fable 5.1 --- .../NamedLocks/MutexBasedNamedLockManager.cs | 191 +++++++++--------- 1 file changed, 100 insertions(+), 91 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs b/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs index b381150650..89b83e5c87 100644 --- a/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs +++ b/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs @@ -4,7 +4,6 @@ using System.Security.AccessControl; using System.Security.Principal; using System.Threading; -using Calamari.Common.Plumbing; using Calamari.Common.Plumbing.Extensions; using Calamari.Common.Plumbing.Logging; using Polly; @@ -42,89 +41,69 @@ public MutexBasedNamedLockManager() public IDisposable Acquire(string name, string waitMessage) { - var globalName = $@"Global\{name}"; - - var acquired = new ManualResetEventSlim(false); - var release = new ManualResetEventSlim(false); - Exception? acquisitionFailure = null; - - // A Mutex can only be waited on and released by the thread that acquired it - var owner = new Thread(() => - { - Mutex? mutex = null; - try - { - // Create/acquire the global mutex with some retry, to (hopefully) avoid two instances of - // Calamari racing to create it (e.g. parallel steps on the same machine) - mutex = mutexAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); - - // Assign full control for all users, so that a lock taken by (say) a Tentacle running as a service - // is still accessible to Calamari running under a different account - if (OperatingSystem.IsWindows()) - SetFullAccessControlForAllUsers(mutex, globalName); - - try - { - if (!mutex.WaitOne(initialWaitBeforeShowingLogMessage)) - { - log.Verbose(waitMessage); - mutex.WaitOne(); - } - } - catch (AbandonedMutexException) - { - // The previous owner died without releasing; the kernel has handed ownership to us - log.Warn($"The lock '{name}' was abandoned by a previous process that exited without releasing it. Continuing, but anything it was protecting may have been left in an inconsistent state."); - } - } - catch (Exception ex) - { - // We never took the mutex, so there is nothing to release - just - // close the handle rather than leaving it to the finaliser - mutex?.Dispose(); - acquisitionFailure = ex; - acquired.Set(); - return; - } - - acquired.Set(); - - release.Wait(); - - // An unhandled exception here would terminate the process. Releasing is best effort: - // a failure leaves the mutex abandoned, which the next waiter recovers from. - try - { - try - { - mutex.ReleaseMutex(); - } - finally - { - mutex.Dispose(); - } - } - catch (Exception ex) - { - log.Verbose($"Failed to release the mutex '{globalName}': {ex.PrettyPrint()}"); - } - }) - { - IsBackground = true, - Name = $"Mutex owner for '{name}'" - }; - owner.Start(); - acquired.Wait(); - - var releaser = new Releaser(owner, acquired, release); - - if (acquisitionFailure != null) - { - releaser.Dispose(); - ExceptionDispatchInfo.Capture(acquisitionFailure).Throw(); - } - - return releaser; + var trackedThread = new TrackedThread($"Mutex owner for '{name}'", + tracker => + { + var globalName = $@"Global\{name}"; + + Mutex? mutex = null; + try + { + // Create/acquire the global mutex with some retry, to (hopefully) avoid two instances of + // Calamari racing to create it (e.g. parallel steps on the same machine) + mutex = mutexAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); + + // Assign full control for all users, so that a lock taken by (say) a Tentacle running as a service + // is still accessible to Calamari running under a different account + if (OperatingSystem.IsWindows()) + SetFullAccessControlForAllUsers(mutex, globalName); + + try + { + if (!mutex.WaitOne(initialWaitBeforeShowingLogMessage)) + { + log.Verbose(waitMessage); + mutex.WaitOne(); + } + } + catch (AbandonedMutexException) + { + // The previous owner died without releasing; the kernel has handed ownership to us + log.Warn($"The lock '{name}' was abandoned by a previous process that exited without releasing it. Continuing, but anything it was protecting may have been left in an inconsistent state."); + } + } + catch (Exception ex) + { + // We never took the mutex, so there is nothing to release - just + // close the handle rather than leaving it to the finaliser + mutex?.Dispose(); + tracker.MarkAsErrored(ex); + return; + } + + tracker.HoldUntilDisposed(); + + // An unhandled exception here would terminate the process. Releasing is best effort: + // a failure leaves the mutex abandoned, which the next waiter recovers from. + try + { + try + { + mutex.ReleaseMutex(); + } + finally + { + mutex.Dispose(); + } + } + catch (Exception ex) + { + log.Verbose($"Failed to release the mutex '{globalName}': {ex.PrettyPrint()}"); + } + }); + + trackedThread.StartAndBlockUntilLockAcquired(); + return trackedThread; } [SupportedOSPlatform("windows")] @@ -146,18 +125,48 @@ void SetFullAccessControlForAllUsers(Mutex mutex, string name) } } - class Releaser : IDisposable + class TrackedThread : IDisposable { + readonly ManualResetEventSlim acquired = new(false); + readonly ManualResetEventSlim release = new(false); readonly Thread owner; - readonly ManualResetEventSlim acquired; - readonly ManualResetEventSlim release; + int released; + Exception? error; + + public TrackedThread(string name, Action threadStart) + { + owner = new Thread(() => threadStart(this)) + { + IsBackground = true, + Name = name + }; + } + + public void StartAndBlockUntilLockAcquired() + { + owner.Start(); + + // Wait for the owner thread to signal it has "entered", or errored + acquired.Wait(); + + if (error != null) + { + Dispose(); + ExceptionDispatchInfo.Capture(error).Throw(); + } + } + + public void HoldUntilDisposed() + { + acquired.Set(); + release.Wait(); + } - public Releaser(Thread owner, ManualResetEventSlim acquired, ManualResetEventSlim release) + public void MarkAsErrored(Exception ex) { - this.owner = owner; - this.acquired = acquired; - this.release = release; + error = ex; + acquired.Set(); } public void Dispose() From 9795c7ade6e0ab0aa4a1b71bf8deb181a493094f Mon Sep 17 00:00:00 2001 From: Steve Leigh Date: Thu, 17 Sep 2026 10:54:43 +1000 Subject: [PATCH 14/14] Minor cleanups, added verbose log messages to help diagnose locking. --- .../NamedLocks/MutexBasedNamedLockManager.cs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs b/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs index 89b83e5c87..d0f30c1424 100644 --- a/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs +++ b/source/Calamari.Common/Features/Processes/NamedLocks/MutexBasedNamedLockManager.cs @@ -15,14 +15,14 @@ public class MutexBasedNamedLockManager : INamedLockManager { readonly ILog log; readonly int initialWaitBeforeShowingLogMessage; - readonly ResiliencePipeline mutexAcquisitionPipeline; + readonly ResiliencePipeline mutexCreationPipeline; public MutexBasedNamedLockManager() { log = ConsoleLog.Instance; initialWaitBeforeShowingLogMessage = (int)TimeSpan.FromSeconds(3).TotalMilliseconds; - mutexAcquisitionPipeline = new ResiliencePipelineBuilder() + mutexCreationPipeline = new ResiliencePipelineBuilder() .AddRetry(new RetryStrategyOptions() { ShouldHandle = new PredicateBuilder().Handle(), @@ -32,7 +32,7 @@ public MutexBasedNamedLockManager() Delay = TimeSpan.FromMilliseconds(50), OnRetry = args => { - log.Verbose($"Waiting {args.RetryDelay.TotalMilliseconds}ms before attempting to acquire the Mutex again"); + log.Verbose($"Waiting {args.RetryDelay.TotalMilliseconds}ms before attempting to create the Mutex again"); return default; } }) @@ -41,6 +41,8 @@ public MutexBasedNamedLockManager() public IDisposable Acquire(string name, string waitMessage) { + log.Verbose($"Acquiring named lock for {name}"); + var trackedThread = new TrackedThread($"Mutex owner for '{name}'", tracker => { @@ -51,15 +53,20 @@ public IDisposable Acquire(string name, string waitMessage) { // Create/acquire the global mutex with some retry, to (hopefully) avoid two instances of // Calamari racing to create it (e.g. parallel steps on the same machine) - mutex = mutexAcquisitionPipeline.Execute(() => new Mutex(false, globalName)); + mutex = mutexCreationPipeline.Execute(() => new Mutex(false, globalName)); + // Assign full control for all users, so that a lock taken by (say) a Tentacle running as a service // is still accessible to Calamari running under a different account if (OperatingSystem.IsWindows()) + { SetFullAccessControlForAllUsers(mutex, globalName); + log.Verbose($"Calamari mutex configured to allow control for all users on '{name}'"); + } try { + log.Verbose($"Attempting to acquire mutex on Calamari '{name}'"); if (!mutex.WaitOne(initialWaitBeforeShowingLogMessage)) { log.Verbose(waitMessage); @@ -69,7 +76,7 @@ public IDisposable Acquire(string name, string waitMessage) catch (AbandonedMutexException) { // The previous owner died without releasing; the kernel has handed ownership to us - log.Warn($"The lock '{name}' was abandoned by a previous process that exited without releasing it. Continuing, but anything it was protecting may have been left in an inconsistent state."); + log.Warn($"The mutex '{name}' was abandoned by a previous process that exited without releasing it. Continuing, but anything it was protecting may have been left in an inconsistent state."); } } catch (Exception ex) @@ -81,6 +88,7 @@ public IDisposable Acquire(string name, string waitMessage) return; } + log.Verbose($"Acquired lock on Calamari for '{name}'"); tracker.HoldUntilDisposed(); // An unhandled exception here would terminate the process. Releasing is best effort: