From 83a3c7564172671327d65e414491fa380b1be291 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 5 Sep 2025 15:12:18 +1200 Subject: [PATCH 01/26] Implement process and instance isolation --- src/Sentry/GlobalSessionManager.cs | 2 +- src/Sentry/Internal/Http/CachingTransport.cs | 2 +- src/Sentry/Internal/InitCounter.cs | 23 ++++++++ src/Sentry/SentryOptions.cs | 31 ++++++++--- src/Sentry/SentrySdk.cs | 1 + .../Internals/Http/CachingTransportTests.cs | 2 +- .../Internals/InitCounterTests.cs | 55 +++++++++++++++++++ test/Sentry.Tests/SentryOptionsTests.cs | 42 ++++++++++++++ test/Sentry.Tests/SentrySdkTests.cs | 29 ++++++++++ 9 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 src/Sentry/Internal/InitCounter.cs create mode 100644 test/Sentry.Tests/Internals/InitCounterTests.cs diff --git a/src/Sentry/GlobalSessionManager.cs b/src/Sentry/GlobalSessionManager.cs index 35125817aa..9e56ae7331 100644 --- a/src/Sentry/GlobalSessionManager.cs +++ b/src/Sentry/GlobalSessionManager.cs @@ -36,7 +36,7 @@ public GlobalSessionManager( // TODO: session file should really be process-isolated, but we // don't have a proper mechanism for that right now. - _persistenceDirectoryPath = options.TryGetDsnSpecificCacheDirectoryPath(); + _persistenceDirectoryPath = options.TryGetIsolatedCacheDirectoryPath(); } // Take pause timestamp directly instead of referencing _lastPauseTimestamp to avoid diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index b5ed2cb34a..a48eb30cc6 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -75,7 +75,7 @@ private CachingTransport(ITransport innerTransport, SentryOptions options, bool : 0; // just in case MaxCacheItems is set to an invalid value somehow (shouldn't happen) _isolatedCacheDirectoryPath = - options.TryGetProcessSpecificCacheDirectoryPath() ?? + options.TryGetIsolatedCacheDirectoryPath() ?? throw new InvalidOperationException("Cache directory or DSN is not set."); // Sanity check: This should never happen in the first place. diff --git a/src/Sentry/Internal/InitCounter.cs b/src/Sentry/Internal/InitCounter.cs new file mode 100644 index 0000000000..57155a9dc8 --- /dev/null +++ b/src/Sentry/Internal/InitCounter.cs @@ -0,0 +1,23 @@ +namespace Sentry.Internal; + +/// +/// This is used internally to track the number of times the SDK has been initialised so that different +/// Hub instances get a different cache directory path, even if they're running in the same process. +/// +internal interface IInitCounter +{ + public int Count { get; } + public void Increment(); +} + +/// +internal class InitCounter : IInitCounter +{ + internal static InitCounter Instance { get; } = new(); + + private int _count; + + public int Count => Volatile.Read(ref _count); + + public void Increment() => Interlocked.Increment(ref _count); +} diff --git a/src/Sentry/SentryOptions.cs b/src/Sentry/SentryOptions.cs index df4e2937e3..d8f9b120f6 100644 --- a/src/Sentry/SentryOptions.cs +++ b/src/Sentry/SentryOptions.cs @@ -780,6 +780,24 @@ public IDiagnosticLogger? DiagnosticLogger /// public string? CacheDirectoryPath { get; set; } + internal Func ProcessIdResolver + { + set => _processIdResolver = value; + get + { + return _processIdResolver ?? DefaultResolver; + int? DefaultResolver() => ProcessInfo.Instance?.GetId(this); + } + } + private Func? _processIdResolver; + + internal IInitCounter InitCounter + { + get => _initCounter ?? Sentry.Internal.InitCounter.Instance; + set => _initCounter = value; + } + private IInitCounter? _initCounter; + /// /// The SDK will only capture HTTP Client errors if it is enabled. /// can be used to configure which requests will be treated as failed. @@ -1788,7 +1806,7 @@ internal void SetupLogging() } } - internal string? TryGetDsnSpecificCacheDirectoryPath() + internal string? TryGetIsolatedCacheDirectoryPath() { if (string.IsNullOrWhiteSpace(CacheDirectoryPath)) { @@ -1800,19 +1818,16 @@ internal void SetupLogging() { 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 - return Path.Combine(CacheDirectoryPath, "Sentry", Dsn.GetHashString()); + var processId = ProcessIdResolver.Invoke() ?? 0; + var stringBuilder = new StringBuilder().AppendJoin('_', Dsn.GetHashCode(), processId, InitCounter.Count); + return Path.Combine(CacheDirectoryPath, "Sentry", stringBuilder.ToString()); #endif } - internal string? TryGetProcessSpecificCacheDirectoryPath() - { - // In the future, this will most likely contain process ID - return TryGetDsnSpecificCacheDirectoryPath(); - } - internal static List GetDefaultInAppExclude() => [ "System", diff --git a/src/Sentry/SentrySdk.cs b/src/Sentry/SentrySdk.cs index eab7b9838f..6f15e9ee73 100644 --- a/src/Sentry/SentrySdk.cs +++ b/src/Sentry/SentrySdk.cs @@ -32,6 +32,7 @@ internal static IHub InitHub(SentryOptions options) { options.SetupLogging(); + options.InitCounter.Increment(); ProcessInfo.Instance ??= new ProcessInfo(options); // Locate the DSN diff --git a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs index 4eb29f4290..038c20c71e 100644 --- a/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs +++ b/test/Sentry.Tests/Internals/Http/CachingTransportTests.cs @@ -242,7 +242,7 @@ public async Task Handle_Malformed_Envelopes_Gracefully() { // Arrange var cacheDirectoryPath = - _options.TryGetProcessSpecificCacheDirectoryPath() ?? + _options.TryGetIsolatedCacheDirectoryPath() ?? throw new InvalidOperationException("Cache directory or DSN is not set."); var processingDirectoryPath = Path.Combine(cacheDirectoryPath, "__processing"); var fileName = $"{Guid.NewGuid()}.envelope"; diff --git a/test/Sentry.Tests/Internals/InitCounterTests.cs b/test/Sentry.Tests/Internals/InitCounterTests.cs new file mode 100644 index 0000000000..cc3f70a5f3 --- /dev/null +++ b/test/Sentry.Tests/Internals/InitCounterTests.cs @@ -0,0 +1,55 @@ +namespace Sentry.Tests.Internals; + +public class InitCounterTests +{ + [Fact] + public void Count_StartsAtZero() + { + // Arrange + var intCounter = new InitCounter(); + + // Act & Assert + Assert.Equal(0, intCounter.Count); + } + + [Fact] + public void Increment_IncreasesByOne() + { + // Arrange + var intCounter = new InitCounter(); + + // Act + intCounter.Increment(); + + // Assert + intCounter.Count.Should().Be(1); + } + + [Fact] + public async Task Increment_IsThreadsSafe() + { + // Arrange + const int incrementsPerTask = 1000; + const int numberOfTasks = 10; + var intCounter = new InitCounter(); + + // Act + // Spawn multiple threads to increment the counter then wait for all the threads to complete and verify the expected number of increments has been made + var tasks = new List(); + for (var i = 0; i < numberOfTasks; i++) + { + tasks.Add(Task.Run(() => + { + for (int j = 0; j < incrementsPerTask; j++) + { + intCounter.Increment(); + } + })); + } + + await Task.WhenAll(tasks); + + // Assert + Assert.Equal(incrementsPerTask * numberOfTasks, intCounter.Count); + } +} diff --git a/test/Sentry.Tests/SentryOptionsTests.cs b/test/Sentry.Tests/SentryOptionsTests.cs index 7d53c1c9b5..2058fac9cd 100644 --- a/test/Sentry.Tests/SentryOptionsTests.cs +++ b/test/Sentry.Tests/SentryOptionsTests.cs @@ -675,4 +675,46 @@ public void CachesInstallationId() installationId2.Should().Be(installationId1); logger.Received(0).Log(SentryLevel.Debug, "Resolved installation ID '{0}'.", null, Arg.Any()); } + + [Fact] + public void TryGetIsolatedCacheDirectoryPath_NullCacheDirectory_ReturnsNull() + { + var o = new SentryOptions { CacheDirectoryPath = null, Dsn = ValidDsn }; + Assert.Null(o.TryGetIsolatedCacheDirectoryPath()); + } + + [Fact] + public void TryGetIsolatedCacheDirectoryPath_MissingDsn_ReturnsNull() + { + var o = new SentryOptions { CacheDirectoryPath = "c:\\cache", Dsn = null }; + Assert.Null(o.TryGetIsolatedCacheDirectoryPath()); + } + + [Theory] + [InlineData(5, null)] + [InlineData(5, 7)] + public void TryGetIsolatedCacheDirectoryPath_UniqueForDsnInitCountAndProcessIdUnlessMobile(int initCount, int? processId) + { + // Arrange + var initCounter = Substitute.For(); + initCounter.Count.Returns(initCount); + var o = new SentryOptions + { + CacheDirectoryPath = "c:\\cache", + Dsn = ValidDsn, + InitCounter = initCounter, + ProcessIdResolver = () => processId + }; + + // Act + var path = o.TryGetIsolatedCacheDirectoryPath(); + + // Assert +#if IOS || ANDROID + var expectedFolder = "Sentry"; +#else + var expectedFolder = $"{ValidDsn.GetHashCode()}_{processId ?? 0}_{initCount}"; +#endif + path.Should().EndWith(expectedFolder); + } } diff --git a/test/Sentry.Tests/SentrySdkTests.cs b/test/Sentry.Tests/SentrySdkTests.cs index e63b97a20b..8fd903b1f3 100644 --- a/test/Sentry.Tests/SentrySdkTests.cs +++ b/test/Sentry.Tests/SentrySdkTests.cs @@ -188,6 +188,26 @@ public void Init_EmptyDsnDisabledDiagnostics_DoesNotLogWarning() } } + [Fact] + public void Init_IncrementsInitCount() + { + // Arrange + var initCount = new InitCounter(); + + // Act + using var first = SentrySdk.Init(o => + { + o.Dsn = ValidDsn; + o.AutoSessionTracking = false; + o.BackgroundWorker = Substitute.For(); + o.InitNativeSdks = false; + o.InitCounter = initCount; + }); + + // Assert + initCount.Count.Should().Be(1); + } + [Fact] public void Init_MultipleCalls_ReplacesHubWithLatest() { @@ -246,6 +266,11 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed(bool? testDel using var cacheDirectory = new TempDirectory(); var cachePath = cacheDirectory.Path; + // Force all instances to use the same cache path + var initCounter = Substitute.For(); + initCounter.Count.Returns(0); + Func processIdResolver = () => 1; + // Pre-populate cache var initialInnerTransport = Substitute.For(); var initialTransport = CachingTransport.Create( @@ -258,6 +283,8 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed(bool? testDel CacheDirectoryPath = cachePath, AutoSessionTracking = false, InitNativeSdks = false, + InitCounter = initCounter, + ProcessIdResolver = processIdResolver }, startWorker: false); await using (initialTransport) @@ -308,6 +335,8 @@ public async Task Init_WithCache_BlocksUntilExistingCacheIsFlushed(bool? testDel o.Debug = true; o.DiagnosticLogger = _logger; o.CacheDirectoryPath = cachePath; + o.InitCounter = initCounter; + o.ProcessIdResolver = processIdResolver; o.InitCacheFlushTimeout = initFlushTimeout; o.Transport = transport; o.AutoSessionTracking = false; From 5e2c12ecf9610d74997b410795ef16beefabca33 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Fri, 5 Sep 2025 23:55:50 +1200 Subject: [PATCH 02/26] Experimenting with named mutexes and semaphores --- .../Internal/CacheDirectoryCoordinator.cs | 99 +++++++++++++++++ src/Sentry/Internal/FileSystemBase.cs | 3 + src/Sentry/Internal/Http/CachingTransport.cs | 105 +++++++++++++----- src/Sentry/Internal/IFileSystem.cs | 1 + src/Sentry/Internal/Polyfills.cs | 30 +++++ src/Sentry/SentryOptions.cs | 22 ---- test/Sentry.Testing/FakeFileSystem.cs | 3 + test/Sentry.Tests/SentrySdkTests.cs | 10 +- 8 files changed, 218 insertions(+), 55 deletions(-) create mode 100644 src/Sentry/Internal/CacheDirectoryCoordinator.cs diff --git a/src/Sentry/Internal/CacheDirectoryCoordinator.cs b/src/Sentry/Internal/CacheDirectoryCoordinator.cs new file mode 100644 index 0000000000..70baca697f --- /dev/null +++ b/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}"; + } + + /// + /// Try to own this cache directory. + /// + /// How long to wait for the lock. + /// True if acquired; false otherwise. + 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()); + } +} diff --git a/src/Sentry/Internal/FileSystemBase.cs b/src/Sentry/Internal/FileSystemBase.cs index 60b474a668..30ada252e0 100644 --- a/src/Sentry/Internal/FileSystemBase.cs +++ b/src/Sentry/Internal/FileSystemBase.cs @@ -2,6 +2,9 @@ namespace Sentry.Internal; internal abstract class FileSystemBase : IFileSystem { + public IEnumerable EnumerateDirectories(string path, string searchPattern) => + Directory.EnumerateDirectories(path, searchPattern); + public IEnumerable EnumerateFiles(string path) => Directory.EnumerateFiles(path); public IEnumerable EnumerateFiles(string path, string searchPattern) => diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index a48eb30cc6..43ffb95e18 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/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(); diff --git a/src/Sentry/Internal/IFileSystem.cs b/src/Sentry/Internal/IFileSystem.cs index 366bf70583..9d6180ea8a 100644 --- a/src/Sentry/Internal/IFileSystem.cs +++ b/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 EnumerateDirectories(string path, string searchPattern); public IEnumerable EnumerateFiles(string path); public IEnumerable EnumerateFiles(string path, string searchPattern); public IEnumerable EnumerateFiles(string path, string searchPattern, SearchOption searchOption); diff --git a/src/Sentry/Internal/Polyfills.cs b/src/Sentry/Internal/Polyfills.cs index 2113687f8d..4666a1f568 100644 --- a/src/Sentry/Internal/Polyfills.cs +++ b/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 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 diff --git a/src/Sentry/SentryOptions.cs b/src/Sentry/SentryOptions.cs index d8f9b120f6..7c865f2f57 100644 --- a/src/Sentry/SentryOptions.cs +++ b/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 GetDefaultInAppExclude() => [ "System", diff --git a/test/Sentry.Testing/FakeFileSystem.cs b/test/Sentry.Testing/FakeFileSystem.cs index 0b6c3ad94a..370929fee2 100644 --- a/test/Sentry.Testing/FakeFileSystem.cs +++ b/test/Sentry.Testing/FakeFileSystem.cs @@ -6,6 +6,9 @@ internal class FakeFileSystem : IFileSystem { private readonly MockFileSystem _fileSystem = new(); + public IEnumerable EnumerateDirectories(string path, string searchPattern) => + _fileSystem.Directory.EnumerateDirectories(path, searchPattern); + public IEnumerable EnumerateFiles(string path) => _fileSystem.Directory.EnumerateFiles(path); public IEnumerable EnumerateFiles(string path, string searchPattern) => diff --git a/test/Sentry.Tests/SentrySdkTests.cs b/test/Sentry.Tests/SentrySdkTests.cs index 8fd903b1f3..8d9f6eeed5 100644 --- a/test/Sentry.Tests/SentrySdkTests.cs +++ b/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(); - initCounter.Count.Returns(0); - Func processIdResolver = () => 1; + initCounter.Count.Returns(1); + Func processIdResolver = () => 42; // Pre-populate cache var initialInnerTransport = Substitute.For(); @@ -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 From 7382caceedcef17b6d8c1cc0c4d394776f97063f Mon Sep 17 00:00:00 2001 From: Sentry Github Bot Date: Fri, 5 Sep 2025 12:14:55 +0000 Subject: [PATCH 03/26] Format code --- src/Sentry/Internal/CacheDirectoryCoordinator.cs | 5 +++-- src/Sentry/Internal/Http/CachingTransport.cs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Sentry/Internal/CacheDirectoryCoordinator.cs b/src/Sentry/Internal/CacheDirectoryCoordinator.cs index 70baca697f..bbe74730a2 100644 --- a/src/Sentry/Internal/CacheDirectoryCoordinator.cs +++ b/src/Sentry/Internal/CacheDirectoryCoordinator.cs @@ -61,7 +61,8 @@ public void Dispose() _disposed = true; if (_acquired) { - try { _semaphore.Release(); } + try + { _semaphore.Release(); } catch (Exception ex) { _logger?.LogError("Error releasing the cache directory semaphore.", ex); } } _semaphore.Dispose(); @@ -80,7 +81,7 @@ internal static class CacheDirectoryHelper internal static string? TryGetIsolatedCacheDirectoryPath(this SentryOptions options) { - if (GetBaseCacheDirectoryPath(options) is not {} baseCacheDir || string.IsNullOrWhiteSpace(options.Dsn)) + if (GetBaseCacheDirectoryPath(options) is not { } baseCacheDir || string.IsNullOrWhiteSpace(options.Dsn)) { return null; } diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index 43ffb95e18..1faacf2155 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -209,7 +209,7 @@ private void SalvageAbandonedCacheSessions() if (coordinator.TryAcquire(TimeSpan.FromMilliseconds(100))) { _options.LogDebug("Salvaging abandoned cache: {0}", dir); - foreach (var filePath in _fileSystem.EnumerateFiles (dir, "*.envelope", SearchOption.AllDirectories)) + 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); From ec60ca1c171332ed10fe035f69491db9a55c2f64 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 8 Sep 2025 10:38:13 +1200 Subject: [PATCH 04/26] Update CachingTransport.cs --- src/Sentry/Internal/Http/CachingTransport.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index 1faacf2155..3956c95750 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -5,14 +5,15 @@ namespace Sentry.Internal.Http; /// -/// A transport that caches envelopes to disk and sends them in the background. +/// Transport that caches envelopes to disk and sends them in the background. /// /// +/// /// Note although this class has a /// method, it doesn't implement as this caused /// a dependency issue with Log4Net in some situations. -/// -/// See https://github.com/getsentry/sentry-dotnet/issues/3178 +/// +/// See https://github.com/getsentry/sentry-dotnet/issues/3178 /// internal class CachingTransport : ITransport, IDisposable { From dcec5e0110d6783d2f19dbc633f08b57d1cbf8ef Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 8 Sep 2025 10:48:13 +1200 Subject: [PATCH 05/26] Update CacheDirectoryCoordinator.cs --- .../Internal/CacheDirectoryCoordinator.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/Sentry/Internal/CacheDirectoryCoordinator.cs b/src/Sentry/Internal/CacheDirectoryCoordinator.cs index bbe74730a2..1584d220e4 100644 --- a/src/Sentry/Internal/CacheDirectoryCoordinator.cs +++ b/src/Sentry/Internal/CacheDirectoryCoordinator.cs @@ -79,22 +79,27 @@ internal static class CacheDirectoryHelper ? null : Path.Combine(options.CacheDirectoryPath, "Sentry"); - internal static string? TryGetIsolatedCacheDirectoryPath(this SentryOptions options) + internal static string? GetIsolatedFolderName(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 + if (string.IsNullOrWhiteSpace(options.Dsn)) + { + return null; + } var processId = options.ProcessIdResolver.Invoke() ?? 0; stringBuilder.AppendJoin('_', options.Dsn.GetHashString(), processId, options.InitCounter.Count); #endif - return Path.Combine(baseCacheDir, stringBuilder.ToString()); + return stringBuilder.ToString(); } + + internal static string? TryGetIsolatedCacheDirectoryPath(this SentryOptions options) => + GetBaseCacheDirectoryPath(options) is not { } baseCacheDir + || GetIsolatedFolderName(options) is not { } isolatedFolderName + ? null + : Path.Combine(baseCacheDir, isolatedFolderName); } From 220884b454168926d72cf4e48d8a8d76f9bf5306 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 8 Sep 2025 12:35:29 +1200 Subject: [PATCH 06/26] Switched to a file lock --- .../Internal/CacheDirectoryCoordinator.cs | 100 ++++++++++++++---- src/Sentry/Internal/Http/CachingTransport.cs | 11 +- 2 files changed, 81 insertions(+), 30 deletions(-) diff --git a/src/Sentry/Internal/CacheDirectoryCoordinator.cs b/src/Sentry/Internal/CacheDirectoryCoordinator.cs index 1584d220e4..c069e20ac2 100644 --- a/src/Sentry/Internal/CacheDirectoryCoordinator.cs +++ b/src/Sentry/Internal/CacheDirectoryCoordinator.cs @@ -6,48 +6,85 @@ namespace Sentry.Internal; internal class CacheDirectoryCoordinator : IDisposable { private readonly IDiagnosticLogger? _logger; - private readonly Semaphore _semaphore; + private readonly object _gate = new(); + + private FileStream? _lockStream; + private readonly string _lockFilePath; + private bool _acquired; private bool _disposed; - private readonly Lock _gate = new(); - public CacheDirectoryCoordinator(string cacheDir, IDiagnosticLogger? logger) + public CacheDirectoryCoordinator(string cacheDir, IDiagnosticLogger? logger, IFileSystem fileSystem) { _logger = logger; - var mutexName = BuildMutexName(cacheDir); - _semaphore = new Semaphore(1, 1, mutexName); // Named mutexes allow interprocess locks on all platforms - } + _lockFilePath = $"{cacheDir}.lock"; - private static string BuildMutexName(string path) - { - var hash = path.GetHashString(); - // Global\ prefix allows cross-session visibility on Windows Terminal Services - return $"Global\\SentryCache{hash}"; + try + { + var baseDir = Path.GetDirectoryName(_lockFilePath); + if (!string.IsNullOrWhiteSpace(baseDir)) + { + // Not normally necessary, but just in case + fileSystem.CreateDirectory(baseDir); + } + } + catch (Exception ex) + { + _logger?.LogError("Failed to ensure lock directory exists for cache coordinator.", ex); + } } - /// - /// Try to own this cache directory. - /// - /// How long to wait for the lock. - /// True if acquired; false otherwise. public bool TryAcquire(TimeSpan timeout) { if (_acquired) { return true; } + lock (_gate) { if (_acquired) { return true; } - if (!_semaphore.WaitOne(timeout)) + + if (_disposed) { return false; } - _acquired = true; - return true; + + try + { + // Note that FileShare.None is implemented via advisory locks only on macOS/Linux... so it will stop + // other .NET processes from accessing the file but not other non-.NET processes. This should be fine + // in our case - we just want to avoid multiple instances of the SDK concurrently accessing the cache + _lockStream = new FileStream( + _lockFilePath, + FileMode.OpenOrCreate, + FileAccess.ReadWrite, + FileShare.None); + + _acquired = true; + return true; + } + catch (Exception ex) + { + _logger?.LogDebug("Unable to acquire cache directory lock", ex); + } + finally + { + if (!_acquired && _lockStream is not null) + { + try { _lockStream.Dispose(); } + catch + { + // Ignore + } + _lockStream = null; + } + } + + return false; } } @@ -56,16 +93,33 @@ 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); } + { + _lockStream?.Close(); + } + catch (Exception ex) + { + _logger?.LogError("Error releasing the cache directory file lock.", ex); + } + } + + try + { + _lockStream?.Dispose(); + } + catch (Exception ex) + { + _logger?.LogError("Error disposing cache lock stream.", ex); } - _semaphore.Dispose(); + _lockStream = null; } } } @@ -79,7 +133,7 @@ internal static class CacheDirectoryHelper ? null : Path.Combine(options.CacheDirectoryPath, "Sentry"); - internal static string? GetIsolatedFolderName(this SentryOptions options) + private static string? GetIsolatedFolderName(this SentryOptions options) { var stringBuilder = new StringBuilder(IsolatedCacheDirectoryPrefix); #if IOS || ANDROID diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs index 3956c95750..89c96fc18e 100644 --- a/src/Sentry/Internal/Http/CachingTransport.cs +++ b/src/Sentry/Internal/Http/CachingTransport.cs @@ -81,7 +81,7 @@ 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); + _cacheCoordinator = new CacheDirectoryCoordinator(_isolatedCacheDirectoryPath, options.DiagnosticLogger, options.FileSystem); if (!_cacheCoordinator.TryAcquire(TimeSpan.FromMilliseconds(100))) { throw new InvalidOperationException("Cache directory already locked."); @@ -99,18 +99,15 @@ private void Initialize(bool startWorker) // Restore any abandoned files from a previous session MoveUnprocessedFilesBackToCache(); - // Ensure directories exist - _fileSystem.CreateDirectory(_isolatedCacheDirectoryPath); _fileSystem.CreateDirectory(_processingDirectoryPath); - // Start a worker, if one is needed if (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); + _recycler = Task.Run(SalvageAbandonedCacheSessions); } else { @@ -206,7 +203,7 @@ private void SalvageAbandonedCacheSessions() } _options.LogDebug("found abandoned cache: {0}", dir); - using var coordinator = new CacheDirectoryCoordinator(dir, _options.DiagnosticLogger); + using var coordinator = new CacheDirectoryCoordinator(dir, _options.DiagnosticLogger, _options.FileSystem); if (coordinator.TryAcquire(TimeSpan.FromMilliseconds(100))) { _options.LogDebug("Salvaging abandoned cache: {0}", dir); @@ -324,7 +321,7 @@ private async Task ProcessCacheAsync(CancellationToken cancellation) // Make sure no files got stuck in the processing directory // See https://github.com/getsentry/sentry-dotnet/pull/3438#discussion_r1672524426 - if (_options.NetworkStatusListener is { Online: false } listener) + if (_options.NetworkStatusListener is { Online: false }) { MoveUnprocessedFilesBackToCache(); } From 180ff535d46f0af11eedfc9afe81840d9437805e Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 8 Sep 2025 13:07:37 +1200 Subject: [PATCH 07/26] Update Sentry.csproj --- src/Sentry/Sentry.csproj | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Sentry/Sentry.csproj b/src/Sentry/Sentry.csproj index f00de450ff..d753684857 100644 --- a/src/Sentry/Sentry.csproj +++ b/src/Sentry/Sentry.csproj @@ -13,7 +13,7 @@ $(TargetFrameworks);net9.0-ios18.0;net8.0-ios17.0 $(TargetFrameworks);net9.0-maccatalyst18.0;net8.0-maccatalyst17.0 - + @@ -83,7 +83,7 @@ - +