Review feedback

Author: jamescrosswell

src/Sentry/GlobalSessionManager.cs

@@ -33,10 +33,7 @@ public GlobalSessionManager(
         _clock = clock ?? SystemClock.Clock;
         _persistedSessionProvider = persistedSessionProvider
                                     ?? (filePath => Json.Load(_options.FileSystem, filePath, PersistedSessionUpdate.FromJson));
-
-        // TODO: session file should really be process-isolated, but we
-        // don't have a proper mechanism for that right now.
-        _persistenceDirectoryPath = options.TryGetIsolatedCacheDirectoryPath();
+        _persistenceDirectoryPath = options.GetIsolatedCacheDirectoryPath();
     }
 
     // Take pause timestamp directly instead of referencing _lastPauseTimestamp to avoid

src/Sentry/Internal/CacheDirectoryCoordinator.cs

@@ -7,33 +7,20 @@ internal class CacheDirectoryCoordinator : IDisposable
 {
     private readonly IDiagnosticLogger? _logger;
     private readonly IFileSystem _fileSystem;
-    private readonly object _gate = new();
+    private readonly Lock _lock = new();
 
     private Stream? _lockStream;
     private readonly string _lockFilePath;
 
-    private bool _acquired;
-    private bool _disposed;
+    private volatile bool _acquired;
+    private volatile bool _disposed;
 
     public CacheDirectoryCoordinator(string cacheDir, IDiagnosticLogger? logger, IFileSystem fileSystem)
     {
         _logger = logger;
         _fileSystem = fileSystem;
+        // Note this creates a lock file in the cache directory's parent directory... not in the cache directory itself
         _lockFilePath = $"{cacheDir}.lock";
-
-        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);
-        }
     }
 
     public bool TryAcquire()
@@ -43,7 +30,7 @@ public bool TryAcquire()
             return true;
         }
 
-        lock (_gate)
+        lock (_lock)
         {
             if (_acquired)
             {
@@ -57,6 +44,11 @@ public bool TryAcquire()
 
             try
             {
+                var baseDir = Path.GetDirectoryName(_lockFilePath);
+                if (!string.IsNullOrWhiteSpace(baseDir))
+                {
+                    _fileSystem.CreateDirectory(baseDir);
+                }
                 _acquired = _fileSystem.TryCreateLockFile(_lockFilePath, out _lockStream);
                 return _acquired;
             }
@@ -84,13 +76,13 @@ public bool TryAcquire()
 
     public void Dispose()
     {
-        lock (_gate)
+        if (_disposed)
         {
-            if (_disposed)
-            {
-                return;
-            }
+            return;
+        }
 
+        lock (_lock)
+        {
             _disposed = true;
 
             if (_acquired)
@@ -140,12 +132,13 @@ internal static class CacheDirectoryHelper
             return null;
         }
         var processId = options.ProcessIdResolver.Invoke() ?? 0;
-        stringBuilder.AppendJoin('_', options.Dsn.GetHashString(), processId, options.InitCounter.Count);
+        stringBuilder.AppendJoin('_', options.Dsn.GetHashString(), processId.ToString(),
+            options.InitCounter.Count.ToString());
 #endif
         return stringBuilder.ToString();
     }
 
-    internal static string? TryGetIsolatedCacheDirectoryPath(this SentryOptions options) =>
+    internal static string? GetIsolatedCacheDirectoryPath(this SentryOptions options) =>
         GetBaseCacheDirectoryPath(options) is not { } baseCacheDir
         || GetIsolatedFolderName(options) is not { } isolatedFolderName
             ? null

src/Sentry/Internal/Http/CachingTransport.cs

@@ -79,7 +79,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.TryGetIsolatedCacheDirectoryPath() ??
+            options.GetIsolatedCacheDirectoryPath() ??
             throw new InvalidOperationException("Cache directory or DSN is not set.");
         _cacheCoordinator = new CacheDirectoryCoordinator(_isolatedCacheDirectoryPath, options.DiagnosticLogger, options.FileSystem);
         if (!_cacheCoordinator.TryAcquire())
@@ -207,7 +207,7 @@ internal void SalvageAbandonedCacheSessions(CancellationToken cancellationToken)
                 continue; // Ignore the current cache directory
             }
 
-            _options.LogDebug("found abandoned cache: {0}", dir);
+            _options.LogDebug("Found abandoned cache: {0}", dir);
             using var coordinator = new CacheDirectoryCoordinator(dir, _options.DiagnosticLogger, _options.FileSystem);
             if (!coordinator.TryAcquire())
             {

src/Sentry/SentrySdk.cs

@@ -30,9 +30,10 @@ static partial class SentrySdk
 
     internal static IHub InitHub(SentryOptions options)
     {
+        options.InitCounter.Increment();
+
         options.SetupLogging();
 
-        options.InitCounter.Increment();
         ProcessInfo.Instance ??= new ProcessInfo(options);
 
         // Locate the DSN

test/Sentry.Tests/HubTests.cs

@@ -2203,7 +2203,7 @@ await transport.Received(1)
                 Arg.Any<CancellationToken>());
         if (options.Transport is CachingTransport cachingTransport)
         {
-            cachingTransport.Dispose(); // Release cache lock so that the cacheDirectory can be removed
+            await cachingTransport.DisposeAsync(); // Release cache lock so that the cacheDirectory can be removed
         }
     }
 

test/Sentry.Tests/Internals/CacheDirectoryCoordinatorTests.cs

@@ -6,18 +6,22 @@ public class CacheDirectoryCoordinatorTests : IDisposable
 
     public void Dispose()
     {
-        if (_fixture.FileSystem.FileExists(_fixture.CacheDirectoryPath))
-        {
-            _fixture.FileSystem.DeleteDirectory(_fixture.CacheDirectoryPath);
-        }
+       _fixture.CacheRoot.Dispose();
     }
 
     private class Fixture
     {
+        public readonly TempDirectory CacheRoot = new();
         public ReadWriteFileSystem FileSystem { get; } = new();
-        public string CacheDirectoryPath { get; } = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"));
 
-        public CacheDirectoryCoordinator GetSut() => new(CacheDirectoryPath, null, FileSystem);
+        public string CacheDirectoryPath { get; private set; }
+        private readonly string _isolatedDirectory = Guid.NewGuid().ToString("N");
+
+        public CacheDirectoryCoordinator GetSut()
+        {
+            CacheDirectoryPath = Path.Combine(CacheRoot.Path, _isolatedDirectory);
+            return new(CacheDirectoryPath, null, FileSystem);
+        }
     }
 
     [Fact]

test/Sentry.Tests/Internals/Http/CachingTransportTests.cs

@@ -243,7 +243,7 @@ public async Task Handle_Malformed_Envelopes_Gracefully()
     {
         // Arrange
         var cacheDirectoryPath =
-            _options.TryGetIsolatedCacheDirectoryPath() ??
+            _options.GetIsolatedCacheDirectoryPath() ??
             throw new InvalidOperationException("Cache directory or DSN is not set.");
         var processingDirectoryPath = Path.Combine(cacheDirectoryPath, "__processing");
         var fileName = $"{Guid.NewGuid()}.envelope";
@@ -624,7 +624,7 @@ public async Task DoesntWriteSentAtHeaderToCacheFile()
         await transport.SendEnvelopeAsync(envelope);
 
         // Assert
-        var isolatedCacheDir = _options.TryGetIsolatedCacheDirectoryPath();
+        var isolatedCacheDir = _options.GetIsolatedCacheDirectoryPath();
         var filePath = _options.FileSystem
             .EnumerateFiles(isolatedCacheDir!, "*", SearchOption.AllDirectories)
             .Single();
@@ -640,7 +640,7 @@ public async Task SalvageAbandonedCacheSessions_MovesEnvelopesFromOtherIsolatedD
         using var innerTransport = new FakeTransport();
         await using var transport = CachingTransport.Create(innerTransport, _options, startWorker: false);
 
-        var currentIsolated = _options.TryGetIsolatedCacheDirectoryPath()!; // already validated during creation
+        var currentIsolated = _options.GetIsolatedCacheDirectoryPath()!; // already validated during creation
         var baseCacheDir = Directory.GetParent(currentIsolated)!.FullName;
 
         // Create two abandoned isolated cache directories with envelope files (including in nested folder)
@@ -694,7 +694,7 @@ public async Task SalvageAbandonedCacheSessions_IgnoresCurrentDirectory()
         // Arrange
         using var innerTransport = new FakeTransport();
         await using var transport = CachingTransport.Create(innerTransport, _options, startWorker: false);
-        var currentIsolated = _options.TryGetIsolatedCacheDirectoryPath()!;
+        var currentIsolated = _options.GetIsolatedCacheDirectoryPath()!;
 
         var currentFile = Path.Combine(currentIsolated, "current.envelope");
         if (_options.FileSystem.CreateFileForWriting(currentFile, out var stream))
@@ -717,7 +717,7 @@ public async Task SalvageAbandonedCacheSessions_SkipsDirectoriesWithActiveLock()
         using var innerTransport = new FakeTransport();
         await using var transport = CachingTransport.Create(innerTransport, _options, startWorker: false);
 
-        var currentIsolated = _options.TryGetIsolatedCacheDirectoryPath()!;
+        var currentIsolated = _options.GetIsolatedCacheDirectoryPath()!;
         var baseCacheDir = Directory.GetParent(currentIsolated)!.FullName;
 
         // Create an abandoned directory that matches the search pattern and acquire its lock

test/Sentry.Tests/SentryOptionsTests.cs

@@ -680,7 +680,7 @@ public void CachesInstallationId()
     public void TryGetIsolatedCacheDirectoryPath_NullCacheDirectory_ReturnsNull()
     {
         var o = new SentryOptions { CacheDirectoryPath = null, Dsn = ValidDsn };
-        Assert.Null(o.TryGetIsolatedCacheDirectoryPath());
+        Assert.Null(o.GetIsolatedCacheDirectoryPath());
     }
 
 #if IOS || ANDROID