Experimenting with named mutexes and semaphores

Author: jamescrosswell

src/Sentry/Internal/CacheDirectoryCoordinator.cs

@@ -0,0 +1,99 @@
+using Sentry.Extensibility;
+using Sentry.Internal.Extensions;
+
+namespace Sentry.Internal;
+
+internal class CacheDirectoryCoordinator : IDisposable
+{
+    private readonly IDiagnosticLogger? _logger;
+    private readonly Semaphore _semaphore;
+    private bool _acquired;
+    private bool _disposed;
+    private readonly Lock _gate = new();
+
+    public CacheDirectoryCoordinator(string cacheDir, IDiagnosticLogger? logger)
+    {
+        _logger = logger;
+        var mutexName = BuildMutexName(cacheDir);
+        _semaphore = new Semaphore(1, 1, mutexName); // Named mutexes allow interprocess locks on all platforms
+    }
+
+    private static string BuildMutexName(string path)
+    {
+        var hash = path.GetHashString();
+        // Global\ prefix allows cross-session visibility on Windows Terminal Services
+        return $"Global\\SentryCache{hash}";
+    }
+
+    /// <summary>
+    /// Try to own this cache directory.
+    /// </summary>
+    /// <param name="timeout">How long to wait for the lock.</param>
+    /// <returns>True if acquired; false otherwise.</returns>
+    public bool TryAcquire(TimeSpan timeout)
+    {
+        if (_acquired)
+        {
+            return true;
+        }
+        lock (_gate)
+        {
+            if (_acquired)
+            {
+                return true;
+            }
+            if (!_semaphore.WaitOne(timeout))
+            {
+                return false;
+            }
+            _acquired = true;
+            return true;
+        }
+    }
+
+    public void Dispose()
+    {
+        lock (_gate)
+        {
+            if (_disposed)
+                return;
+
+            _disposed = true;
+            if (_acquired)
+            {
+                try { _semaphore.Release(); }
+                catch (Exception ex) { _logger?.LogError("Error releasing the cache directory semaphore.", ex); }
+            }
+            _semaphore.Dispose();
+        }
+    }
+}
+
+internal static class CacheDirectoryHelper
+{
+    public const string IsolatedCacheDirectoryPrefix = "isolated_";
+
+    internal static string? GetBaseCacheDirectoryPath(this SentryOptions options) =>
+        string.IsNullOrWhiteSpace(options.CacheDirectoryPath)
+            ? null
+            : Path.Combine(options.CacheDirectoryPath, "Sentry");
+
+    internal static string? TryGetIsolatedCacheDirectoryPath(this SentryOptions options)
+    {
+        if (GetBaseCacheDirectoryPath(options) is not {} baseCacheDir || string.IsNullOrWhiteSpace(options.Dsn))
+        {
+            return null;
+        }
+
+        var stringBuilder = new StringBuilder(IsolatedCacheDirectoryPrefix);
+#if IOS || ANDROID
+        // On iOS or Android the app is already sandboxed, so there's no risk of sending data to another Sentry's DSN.
+        // However, users may still initiate the SDK multiple times within the process, so we need an InitCounter
+        stringBuilder.Append(options.InitCounter.Count);
+#else
+        var processId = options.ProcessIdResolver.Invoke() ?? 0;
+        stringBuilder.AppendJoin('_', options.Dsn.GetHashString(), processId, options.InitCounter.Count);
+#endif
+        return Path.Combine(baseCacheDir, stringBuilder.ToString());
+    }
+}

src/Sentry/Internal/FileSystemBase.cs

@@ -2,6 +2,9 @@ namespace Sentry.Internal;
 
 internal abstract class FileSystemBase : IFileSystem
 {
+    public IEnumerable<string> EnumerateDirectories(string path, string searchPattern) =>
+        Directory.EnumerateDirectories(path, searchPattern);
+
     public IEnumerable<string> EnumerateFiles(string path) => Directory.EnumerateFiles(path);
 
     public IEnumerable<string> EnumerateFiles(string path, string searchPattern) =>

src/Sentry/Internal/Http/CachingTransport.cs

@@ -22,9 +22,11 @@ internal class CachingTransport : ITransport, IDisposable
     private readonly ITransport _innerTransport;
     private readonly SentryOptions _options;
     private readonly bool _failStorage;
-    private readonly string _isolatedCacheDirectoryPath;
     private readonly int _keepCount;
 
+    private readonly CacheDirectoryCoordinator _cacheCoordinator;
+    private readonly string _isolatedCacheDirectoryPath;
+
     // When a file is getting processed, it's moved to a child directory
     // to avoid getting picked up by other threads.
     private readonly string _processingDirectoryPath;
@@ -45,6 +47,7 @@ internal class CachingTransport : ITransport, IDisposable
 
     private readonly CancellationTokenSource _workerCts = new();
     private Task _worker = null!;
+    private Task? _recycler = null;
 
     private ManualResetEventSlim? _initCacheResetEvent = new();
     private ManualResetEventSlim? _preInitCacheResetEvent = new();
@@ -77,6 +80,11 @@ private CachingTransport(ITransport innerTransport, SentryOptions options, bool
         _isolatedCacheDirectoryPath =
             options.TryGetIsolatedCacheDirectoryPath() ??
             throw new InvalidOperationException("Cache directory or DSN is not set.");
+        _cacheCoordinator = new CacheDirectoryCoordinator(_isolatedCacheDirectoryPath, options.DiagnosticLogger);
+        if (!_cacheCoordinator.TryAcquire(TimeSpan.FromMilliseconds(100)))
+        {
+            throw new InvalidOperationException("Cache directory already locked.");
+        }
 
         // Sanity check: This should never happen in the first place.
         // We check for `DisableFileWrite` before creating the CachingTransport.
@@ -99,6 +107,9 @@ private void Initialize(bool startWorker)
         {
             _options.LogDebug("Starting CachingTransport worker.");
             _worker = Task.Run(CachedTransportBackgroundTaskAsync);
+
+            // Start a recycler in the background so as not to block initialisation
+            // _recycler = Task.Run(SalvageAbandonedCacheSessions);
         }
         else
         {
@@ -178,6 +189,36 @@ private async Task CachedTransportBackgroundTaskAsync()
         _options.LogDebug("CachingTransport worker stopped.");
     }
 
+    private void SalvageAbandonedCacheSessions()
+    {
+        if (_options.GetBaseCacheDirectoryPath() is not { } baseCacheDir)
+        {
+            return;
+        }
+
+        const string searchPattern = $"{CacheDirectoryHelper.IsolatedCacheDirectoryPrefix}*";
+        foreach (var dir in _fileSystem.EnumerateDirectories(baseCacheDir, searchPattern))
+        {
+            if (string.Equals(dir, _isolatedCacheDirectoryPath, StringComparison.OrdinalIgnoreCase))
+            {
+                continue; // Ignore the current cache directory
+            }
+
+            _options.LogDebug("found abandoned cache: {0}", dir);
+            using var coordinator = new CacheDirectoryCoordinator(dir, _options.DiagnosticLogger);
+            if (coordinator.TryAcquire(TimeSpan.FromMilliseconds(100)))
+            {
+                _options.LogDebug("Salvaging abandoned cache: {0}", dir);
+                foreach (var filePath in _fileSystem.EnumerateFiles (dir, "*.envelope", SearchOption.AllDirectories))
+                {
+                    var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath));
+                    _options.LogDebug("Moving abandoned file back to cache: {0} to {1}.", filePath, destinationPath);
+                    MoveFileWithRetries(filePath, destinationPath, "abandoned");
+                }
+            }
+        }
+    }
+
     private void MoveUnprocessedFilesBackToCache()
     {
         // Processing directory may already contain some files left from previous session
@@ -194,42 +235,46 @@ private void MoveUnprocessedFilesBackToCache()
         {
             var destinationPath = Path.Combine(_isolatedCacheDirectoryPath, Path.GetFileName(filePath));
             _options.LogDebug("Moving unprocessed file back to cache: {0} to {1}.", filePath, destinationPath);
+            MoveFileWithRetries(filePath, destinationPath, "unprocessed");
+        }
+    }
 
-            const int maxAttempts = 3;
-            for (var attempt = 1; attempt <= maxAttempts; attempt++)
+    private void MoveFileWithRetries(string filePath, string destinationPath, string moveType)
+    {
+        const int maxAttempts = 3;
+        for (var attempt = 1; attempt <= maxAttempts; attempt++)
+        {
+            try
             {
-                try
+                _fileSystem.MoveFile(filePath, destinationPath);
+                break;
+            }
+            catch (Exception ex)
+            {
+                if (!_fileSystem.FileExists(filePath))
                 {
-                    _fileSystem.MoveFile(filePath, destinationPath);
+                    _options.LogDebug(
+                        "Failed to move {2} file back to cache (attempt {0}), " +
+                        "but the file no longer exists so it must have been handled by another process: {1}",
+                        attempt, filePath, moveType);
                     break;
                 }
-                catch (Exception ex)
-                {
-                    if (!_fileSystem.FileExists(filePath))
-                    {
-                        _options.LogDebug(
-                            "Failed to move unprocessed file back to cache (attempt {0}), " +
-                            "but the file no longer exists so it must have been handled by another process: {1}",
-                            attempt, filePath);
-                        break;
-                    }
 
-                    if (attempt < maxAttempts)
-                    {
-                        _options.LogDebug(
-                            "Failed to move unprocessed file back to cache (attempt {0}, retrying.): {1}",
-                            attempt, filePath);
-
-                        Thread.Sleep(200); // give a small bit of time before retry
-                    }
-                    else
-                    {
-                        _options.LogError(ex,
-                            "Failed to move unprocessed file back to cache (attempt {0}, done.): {1}", attempt, filePath);
-                    }
+                if (attempt < maxAttempts)
+                {
+                    _options.LogDebug(
+                        "Failed to move {2} file back to cache (attempt {0}, retrying.): {1}",
+                        attempt, filePath, moveType);
 
-                    // note: we do *not* want to re-throw the exception
+                    Thread.Sleep(200); // give a small bit of time before retry
                 }
+                else
+                {
+                    _options.LogError(ex,
+                        "Failed to move {2} file back to cache (attempt {0}, done.): {1}", attempt, filePath, moveType);
+                }
+
+                // note: we do *not* want to re-throw the exception
             }
         }
     }
@@ -560,7 +605,9 @@ public async ValueTask DisposeAsync()
         _workerSignal.Dispose();
         _workerCts.Dispose();
         _worker.Dispose();
+        _recycler?.Dispose();
         _cacheDirectoryLock.Dispose();
+        _cacheCoordinator.Dispose();
         _preInitCacheResetEvent?.Dispose();
         _initCacheResetEvent?.Dispose();
 

src/Sentry/Internal/IFileSystem.cs

@@ -10,6 +10,7 @@ internal interface IFileSystem
     // Note: This is not comprehensive.  If you need other filesystem methods, add to this interface,
     // then implement in both Sentry.Internal.FileSystem and Sentry.Testing.FakeFileSystem.
 
+    public IEnumerable<string> EnumerateDirectories(string path, string searchPattern);
     public IEnumerable<string> EnumerateFiles(string path);
     public IEnumerable<string> EnumerateFiles(string path, string searchPattern);
     public IEnumerable<string> EnumerateFiles(string path, string searchPattern, SearchOption searchOption);

src/Sentry/Internal/Polyfills.cs

@@ -1,5 +1,9 @@
 // Polyfills to bridge the missing APIs in older targets.
 
+#if !NET9_0_OR_GREATER
+global using Lock = object;
+#endif
+
 #if NETFRAMEWORK || NETSTANDARD2_0
 namespace System
 {
@@ -9,6 +13,32 @@ internal static class HashCode
         public static int Combine<T1, T2, T3>(T1 value1, T2 value2, T3 value3) => (value1, value2, value3).GetHashCode();
     }
 }
+
+internal static partial class PolyfillExtensions
+{
+    public static StringBuilder AppendJoin(this StringBuilder builder, char separator, params object?[] values)
+    {
+        if (values.Length == 0)
+        {
+            return builder;
+        }
+
+        if (values[0] is {} value)
+        {
+            builder.Append(value);
+        }
+
+        for (var i = 1; i < values.Length; i++)
+        {
+            builder.Append(separator);
+            if (values[i] is {} nextValue)
+            {
+                builder.Append(nextValue);
+            }
+        }
+        return builder;
+    }
+}
 #endif
 
 #if NETFRAMEWORK

src/Sentry/SentryOptions.cs

@@ -1806,28 +1806,6 @@ internal void SetupLogging()
         }
     }
 
-    internal string? TryGetIsolatedCacheDirectoryPath()
-    {
-        if (string.IsNullOrWhiteSpace(CacheDirectoryPath))
-        {
-            return null;
-        }
-
-        // DSN must be set to use caching
-        if (string.IsNullOrWhiteSpace(Dsn))
-        {
-            return null;
-        }
-
-#if IOS || ANDROID // on iOS or Android the app is already sandboxed so there's no risk of sending data from 1 app to another Sentry's DSN
-        return Path.Combine(CacheDirectoryPath, "Sentry");
-#else
-        var processId = ProcessIdResolver.Invoke() ?? 0;
-        var stringBuilder = new StringBuilder().AppendJoin('_', Dsn.GetHashCode(), processId, InitCounter.Count);
-        return Path.Combine(CacheDirectoryPath, "Sentry", stringBuilder.ToString());
-#endif
-    }
-
     internal static List<StringOrRegex> GetDefaultInAppExclude() =>
     [
         "System",

test/Sentry.Testing/FakeFileSystem.cs

@@ -6,6 +6,9 @@ internal class FakeFileSystem : IFileSystem
 {
     private readonly MockFileSystem _fileSystem = new();
 
+    public IEnumerable<string> EnumerateDirectories(string path, string searchPattern) =>
+        _fileSystem.Directory.EnumerateDirectories(path, searchPattern);
+
     public IEnumerable<string> EnumerateFiles(string path) => _fileSystem.Directory.EnumerateFiles(path);
 
     public IEnumerable<string> EnumerateFiles(string path, string searchPattern) =>

test/Sentry.Tests/SentrySdkTests.cs

@@ -268,8 +268,8 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed(bool? testDel
 
         // Force all instances to use the same cache path
         var initCounter = Substitute.For<IInitCounter>();
-        initCounter.Count.Returns(0);
-        Func<int?> processIdResolver = () => 1;
+        initCounter.Count.Returns(1);
+        Func<int?> processIdResolver = () => 42;
 
         // Pre-populate cache
         var initialInnerTransport = Substitute.For<ITransport>();
@@ -369,8 +369,10 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed(bool? testDel
         finally
         {
             // cleanup to avoid disposing/deleting the temp directory while the cache worker is still running
-            var cachingTransport = (CachingTransport)options!.Transport;
-            await cachingTransport!.StopWorkerAsync();
+            if (options!.Transport is CachingTransport cachingTransport)
+            {
+                await cachingTransport!.StopWorkerAsync();
+            }
         }
     }
 #endif