Implement process and instance isolation #4498

jamescrosswell · 2025-09-05T03:15:08Z

Resolves #2033, Resolves #1067

Errors when initializing Sentry twice with caching enabled #2033
Offline caching should support concurrent process instances #1067
Auto Session Tracking should only be enabled by default on Mobile until total process isolation is supported sentry-unity#264

Todo

We're seeing an error in some entirely unrelated code on Windows ARM64. This is happening for other PRs as well, so unrelated to this PR.

GetRuntime_AllProperties failing on WinArm64 #4712

Interprocess Locking

This turns out to be surprisingly difficult.

Something like a named Mutex could be used for this. However Mutexes are thread affine so they need to be released on the same thread that acquired them. Other than setting aside a dedicated thread for the lifetime of the Hub, there's no easy way to make that work.

A named Semaphore would be another option, but these aren't supported on macOS.

In the end I went with the "lock file" solution that @lucas-zimerman suggested. Keeping a lock file specific to each cache directory and opening this with FileShare.None gives us a kind of file based named mutex that we can use to avoid having multiple instances of the Hub trying to process files from the same cache directory.

…dotnet into isolated-cache

codecov · 2025-09-11T22:46:54Z

Codecov Report

❌ Patch coverage is 77.16049% with 37 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (version6@522f80e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/Sentry/Internal/Http/CachingTransport.cs	70.73%	19 Missing and 5 partials ⚠️
src/Sentry/Internal/CacheDirectoryCoordinator.cs	81.96%	9 Missing and 2 partials ⚠️
src/Sentry/Internal/ReadOnlyFilesystem.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             version6    #4498   +/-   ##
===========================================
  Coverage            ?   73.36%           
===========================================
  Files               ?      482           
  Lines               ?    17567           
  Branches            ?     3473           
===========================================
  Hits                ?    12888           
  Misses              ?     3826           
  Partials            ?      853

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bruno-garcia · 2025-10-05T23:20:37Z

@sentry review

src/Sentry/Internal/CacheDirectoryCoordinator.cs

seer-by-sentry · 2025-10-05T23:25:28Z

src/Sentry/Internal/Http/CachingTransport.cs

+        {
+            throw new InvalidOperationException("Cache directory already locked.");
+        }


The cache coordinator acquisition failure throws an InvalidOperationException, but this could make it impossible to create multiple SDK instances in some scenarios. Consider making this non-fatal and falling back to a non-cached transport or using a different isolation strategy.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

question: Could this be a severe issue?

But was should happen if this fails?
I believe the code path comes from creating a SentryClient via the Hub.

But we don't have too much control in this CachingTransport-.ctor here.

Hmmm ...
I wonder if we should avoid trying to Acquire the Lock-File in the .ctor,
but instead in the Initialize ... or the Worker-Task ... but then the error may happen quite late.

🤔 something is troubling me but I can't quite put my finger on it just yet 🤔

This is how the SDK currently works... so I don't believe it's an issue. If it is, I think we could address in a separate PR.

sentry-dotnet/src/Sentry/Internal/Http/CachingTransport.cs

Lines 81 to 83 in ee30f23

_isolatedCacheDirectoryPath =

options.TryGetIsolatedCacheDirectoryPath() ??

throw new InvalidOperationException("Cache directory or DSN is not set.");

seer-by-sentry · 2025-10-05T23:25:29Z

src/Sentry/Internal/ReadWriteFileSystem.cs

+    /// <remarks>
+    /// This method can throw all of the same exceptions that the <see cref="FileStream"/> constructor can throw. The
+    /// caller is responsible for handling those exceptions.
+    /// </remarks>
+    public override bool TryCreateLockFile(string path, out Stream fileStream)
+    {
+        // 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
+        fileStream = new FileStream(
+            path,
+            FileMode.OpenOrCreate,
+            FileAccess.ReadWrite,
+            FileShare.None);
+        return true;
+    }


The TryCreateLockFile method can throw all FileStream constructor exceptions but always returns true. This is inconsistent with the interface design pattern where 'Try' methods typically return false on failure rather than throwing. Consider catching exceptions and returning false, or rename the method to indicate it can throw.

Suggested change

/// <remarks>

/// This method can throw all of the same exceptions that the <see cref="FileStream"/> constructor can throw. The

/// caller is responsible for handling those exceptions.

/// </remarks>

public override bool TryCreateLockFile(string path, out Stream fileStream)

{

// 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

fileStream = new FileStream(

path,

FileMode.OpenOrCreate,

FileAccess.ReadWrite,

FileShare.None);

return true;

}

public override bool TryCreateLockFile(string path, out Stream fileStream)

{

try

{

fileStream = new FileStream(

path,

FileMode.OpenOrCreate,

FileAccess.ReadWrite,

FileShare.None);

return true;

}

catch

{

fileStream = Stream.Null;

return false;

}

}

_{Did we get this right? 👍 / 👎 to inform future reviews.}

This is consistent with the behaviour for CreateFileStream and makes it easier to surface errors to callers... so I'd rather stick with the API we have.

src/Sentry/SentrySdk.cs

src/Sentry/Internal/CacheDirectoryCoordinator.cs

Flash0ver · 2025-11-03T20:26:04Z

test/Sentry.Tests/HubTests.cs

+        if (options.Transport is CachingTransport cachingTransport)
+        {
+            cachingTransport.Dispose(); // Release cache lock so that the cacheDirectory can be removed


question: Don't we need to do this for every test?

The method CapturesEventWithContextKey_Implementation does it too?

I think we only need to do it when options.CacheDirectoryPath is set... it's just to ensure the cache dir gets cleaned up. Most tests aren't using a cache dir.

Flash0ver · 2025-11-03T20:27:15Z

test/Sentry.Tests/SentryClientTests.cs

        var cachingTransport = Assert.IsType<CachingTransport>(_fixture.SentryOptions.Transport);
        _ = Assert.IsType<FakeTransport>(cachingTransport.InnerTransport);
+
+        cachingTransport.Dispose(); // Release cache lock so that the cacheDirectory can be removed


question: don't we need to do this on a Test-Class level instead, so that it applies to every Test-Method?

It's only necessary because we set CacheDirectoryPath on line 1628.

It's a bit messy but only really a problem in tests, I think... SDK users won't need to worry about this.

Flash0ver · 2025-11-03T20:37:58Z

src/Sentry/Internal/Http/CachingTransport.cs

+        {
+            throw new InvalidOperationException("Cache directory already locked.");
+        }


question: Could this be a severe issue?

But was should happen if this fails?
I believe the code path comes from creating a SentryClient via the Hub.

But we don't have too much control in this CachingTransport-.ctor here.

Hmmm ...
I wonder if we should avoid trying to Acquire the Lock-File in the .ctor,
but instead in the Initialize ... or the Worker-Task ... but then the error may happen quite late.

🤔 something is troubling me but I can't quite put my finger on it just yet 🤔

src/Sentry/Internal/Http/CachingTransport.cs

Flash0ver · 2025-11-03T20:46:59Z

src/Sentry/Internal/Http/CachingTransport.cs

+
+            _options.LogDebug("found abandoned cache: {0}", dir);
+            using var coordinator = new CacheDirectoryCoordinator(dir, _options.DiagnosticLogger, _options.FileSystem);
+            if (!coordinator.TryAcquire())


question: Could we run into a concurrency issue?

E.g.

this recycler has started and tries to acquire a lock-file

in the .ctor we have also acquired a lock-file already, in a directory below

now we try to move files that contain a lock-file that we still have a reference via Stream on

Essentially now retrying 3 times without any effect?
But I may misunderstand something there 🤔

The lock files sit in the parent/root cache directory. The isolated cache folders are all children of that root folder, so none of the operations on the isolated cache folders should affect the locks. I've added a code comment to make this clearer:

sentry-dotnet/src/Sentry/Internal/CacheDirectoryCoordinator.cs

Lines 22 to 23 in ada094b

// Note this creates a lock file in the cache directory's parent directory... not in the cache directory itself

_lockFilePath = $"{cacheDir}.lock";

github-actions · 2025-11-05T01:04:49Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

### Features

- Implement process and instance isolation ([#4498](https://github.com/getsentry/sentry-dotnet/pull/4498))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description or adding a skip-changelog label.

Generated by 🚫 dangerJS against 2be400d

jamescrosswell · 2025-11-05T03:30:06Z

When I changed the merge target, it didn't automatically trigger a build, so I triggered this manually:
https://github.com/getsentry/sentry-dotnet/actions/runs/19088039748

All green 🟢

jamescrosswell · 2025-11-05T03:38:33Z

@Flash0ver I think the only remaining piece of feedback to address (not sure what happened to it) is how we handle upgrading from previous SDK versions, if people have files in their cache when they do so (since we're changing the cache directory).

Options:

Deal with it via upgrade docs, in the Breaking Changes section of the changelog
Modify the code to automatically import envelopes from the cache root directory (we could maybe remove that code in a future v7 release).

I think since this could potentially result in lost client data, it's worth opting for door number 2 here... but only if we're broadly happy with the PR aside from that.

I addressed all the review feedback and changed the merge target to version 6.

cursor · 2025-11-07T00:27:29Z

src/Sentry/Internal/Http/CachingTransport.cs

+        {
+            return ResultMigrationLockNotAcquired;
+        }
+


Bug: Race-Condition in Migration Locking Mechanism

The MigrateVersion5Cache method checks if the marker file exists before acquiring the migration lock, creating a race condition. Multiple processes could check the marker file simultaneously, all see it doesn't exist, then compete for the lock. The marker file check needs to happen after acquiring the lock to ensure atomicity of the check-and-migrate operation. Without this, concurrent processes may incorrectly attempt migration or return misleading status codes.

While technically possible, worst case, two threads run the migration one after the other (and the second thread does nothing).

The current design will be more optimal 99.999% of the time (i.e. when not upgrading), since it avoids unnecessarily creating and deleting lock files for the root cache directory on every run.

…dotnet into isolated-cache

Flash0ver · 2025-11-12T02:46:50Z

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

+        _options.FileSystem.WriteAllTextToFile(procFile, "dummy content");
+
+        // Act
+        transport.MigrateVersion5Cache(CancellationToken.None);


suggestion: Assert that the result is ResultMigrationOk

Flash0ver · 2025-11-12T02:47:07Z

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

+    }
+
+    [Fact]
+    public async Task MigrateVersion5Cache_MovesEnvelopesFromBaseAndProcessing()


question: Should we add a test for ResultMigrationNoBaseCacheDir?

Flash0ver · 2025-11-12T02:55:11Z

src/Sentry/Internal/Http/CachingTransport.cs

+    /// In version 5, there were no isolated subdirectories.
+    /// </summary>
+    /// <param name="cancellationToken"></param>
+    /// <returns>An integer return code to facillitate testing</returns>


Suggested change

/// <returns>An integer return code to facillitate testing</returns>

/// <returns>An integer return code to facilitate testing.</returns>

Flash0ver · 2025-11-12T03:00:58Z

src/Sentry/Internal/Http/CachingTransport.cs

+            return Task.CompletedTask;
+        }
+
+        // Stop worker and wait until it finishes


thought: This comment is confusing to me ... unless I misunderstand something 😟:

We don't wait until the stopped worker is finished here ... but the caller is awaiting it's completion/cancellation.

Flash0ver · 2025-11-12T03:07:05Z

test/Sentry.Tests/SentryOptionsTests.cs

+    public void GetIsolatedFolderName_MissingDsn_UniqueForInitcount()
+    {
+        var o = new SentryOptions { CacheDirectoryPath = "c:\\cache", Dsn = null };
+        var folder =o.GetIsolatedFolderName();


nitpick

Suggested change

var folder =o.GetIsolatedFolderName();

var folder = o.GetIsolatedFolderName();

Flash0ver · 2025-11-12T03:08:33Z

CHANGELOG.md

+### Features
+
+- Implement process and instance isolation so that multiple instances of the Sentry SDK can be instantiated inside the same process when using the Caching Transport ([#4498](https://github.com/getsentry/sentry-dotnet/pull/4498))


suggestion: merge into already existing Unreleased Features section

jamescrosswell and others added 21 commits September 5, 2025 15:12

Implement process and instance isolation

83a3c75

Experimenting with named mutexes and semaphores

5e2c12e

Format code

7382cac

Update CachingTransport.cs

ec60ca1

Update CacheDirectoryCoordinator.cs

dcec5e0

Switched to a file lock

220884b

Update Sentry.csproj

180ff53

Format code

03c998c

Fixed tests

71ed31b

Update CachingTransportTests.cs

d5dc55a

Merge branch 'isolated-cache' of https://github.com/getsentry/sentry-…

3ae4d96

…dotnet into isolated-cache

Update CHANGELOG.md

0046933

.

d6dd817

Fixed mobile tests

90ede6b

Format code

c26c5bd

Windows tests

39be526

Create CacheDirectoryCoordinatorTests.cs

c85161f

Update SentryOptionsTests.cs

536d9e2

Merge branch 'isolated-cache' of https://github.com/getsentry/sentry-…

477361b

…dotnet into isolated-cache

Added tests for SalvageAbandonedCacheSessions

ee30f23

Update CacheDirectoryCoordinatorTests.cs

cd0848d

jamescrosswell marked this pull request as ready for review September 11, 2025 22:25

jamescrosswell requested a review from Flash0ver as a code owner September 11, 2025 22:25

This comment was marked as outdated.

Sign in to view

jamescrosswell added 2 commits September 12, 2025 10:30

Merge branch 'main' into isolated-cache

b763698

Update CHANGELOG.md

a7b0656

jamescrosswell added 2 commits September 19, 2025 22:26

Merge branch 'main' into isolated-cache

78f1a3a

Update CHANGELOG.md

29fa252

seer-by-sentry bot reviewed Oct 5, 2025

View reviewed changes

src/Sentry/Internal/CacheDirectoryCoordinator.cs Outdated Show resolved Hide resolved

seer-by-sentry bot reviewed Oct 5, 2025

View reviewed changes

src/Sentry/Internal/CacheDirectoryCoordinator.cs Outdated Show resolved Hide resolved

seer-by-sentry bot reviewed Oct 5, 2025

View reviewed changes

src/Sentry/SentrySdk.cs Outdated Show resolved Hide resolved

Flash0ver reviewed Nov 3, 2025

View reviewed changes

jamescrosswell added 2 commits November 5, 2025 13:51

Review feedback

ada094b

Merge remote-tracking branch 'origin/version6' into isolated-cache

efaa83e

jamescrosswell changed the base branch from main to version6 November 5, 2025 01:04

jamescrosswell requested a review from Flash0ver November 5, 2025 03:30

Added auto migration from v5

3730654

cursor bot reviewed Nov 7, 2025

View reviewed changes

getsentry-bot and others added 4 commits November 7, 2025 00:42

Format code

81a06f9

Fix tests for windows

af4821a

Merge branch 'isolated-cache' of https://github.com/getsentry/sentry-…

8f72bf3

…dotnet into isolated-cache

Merge branch 'version6' into isolated-cache

1003333

Flash0ver reviewed Nov 12, 2025

View reviewed changes

Flash0ver approved these changes Nov 12, 2025

View reviewed changes

Review feedback

2be400d

jamescrosswell merged commit 570eae6 into version6 Nov 12, 2025
14 of 15 checks passed

jamescrosswell deleted the isolated-cache branch November 12, 2025 08:28

This was referenced Nov 18, 2025

ref: cleanup unused code after merge #4748

Merged

chore: fix vulnerable packages #4757

Merged

	_isolatedCacheDirectoryPath =
	options.TryGetIsolatedCacheDirectoryPath() ??
	throw new InvalidOperationException("Cache directory or DSN is not set.");

	// Note this creates a lock file in the cache directory's parent directory... not in the cache directory itself
	_lockFilePath = $"{cacheDir}.lock";

	/// <returns>An integer return code to facillitate testing</returns>
	/// <returns>An integer return code to facilitate testing.</returns>

	var folder =o.GetIsolatedFolderName();
	var folder = o.GetIsolatedFolderName();

		### Features

		- Implement process and instance isolation so that multiple instances of the Sentry SDK can be instantiated inside the same process when using the Caching Transport ([#4498](https://github.com/getsentry/sentry-dotnet/pull/4498))

Uh oh!

Implement process and instance isolation #4498

Implement process and instance isolation #4498

Uh oh!

Conversation

jamescrosswell commented Sep 5, 2025 • edited by bitsandfoxes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Interprocess Locking

Uh oh!

This comment was marked as outdated.

Uh oh!

codecov bot commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bruno-garcia commented Oct 5, 2025

Uh oh!

Uh oh!

Uh oh!

seer-by-sentry bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seer-by-sentry bot Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions and example for changelog

Uh oh!

jamescrosswell commented Nov 5, 2025

Uh oh!

jamescrosswell commented Nov 5, 2025

Uh oh!

cursor bot Nov 7, 2025

Choose a reason for hiding this comment

Bug: Race-Condition in Migration Locking Mechanism

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jamescrosswell commented Sep 5, 2025 •

edited by bitsandfoxes

Loading

codecov bot commented Sep 11, 2025 •

edited

Loading

jamescrosswell Nov 4, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading