diff --git a/src/SQLInsight.Collectors/Concurrency/DeadlockCollector.cs b/src/SQLInsight.Collectors/Concurrency/DeadlockCollector.cs index 79dbab5..e338d1d 100644 --- a/src/SQLInsight.Collectors/Concurrency/DeadlockCollector.cs +++ b/src/SQLInsight.Collectors/Concurrency/DeadlockCollector.cs @@ -105,7 +105,8 @@ public sealed class DeadlockCollector : Collector return new DeadlockSnapshot { CapturedAt = context.Now, - NewDeadlocks = deadlocks, + // Our own deadlocks are skipped, but still move the read position past them. + NewDeadlocks = deadlocks.Where(d => !d.IsOwn).ToList(), LastEventTime = deadlocks.Count > 0 ? deadlocks[^1].Time : since, LastFileName = file, LastFileOffset = offset, @@ -130,7 +131,7 @@ public sealed class DeadlockCollector : Collector return new DeadlockSnapshot { CapturedAt = context.Now, - NewDeadlocks = deadlocks, + NewDeadlocks = deadlocks.Where(d => !d.IsOwn).ToList(), LastEventTime = deadlocks.Count > 0 ? deadlocks[^1].Time : since, }; } diff --git a/src/SQLInsight.Core/Data/Deadlocks.cs b/src/SQLInsight.Core/Data/Deadlocks.cs index 01fdaa6..95903f0 100644 --- a/src/SQLInsight.Core/Data/Deadlocks.cs +++ b/src/SQLInsight.Core/Data/Deadlocks.cs @@ -26,6 +26,16 @@ public sealed record DeadlockEvent /// The raw deadlock graph (XDL), for download and viewing in SSMS. public required string GraphXml { get; init; } + + /// + /// Every participant is SQLInsight Pro itself (monitoring or its own repository), e.g. when the repository lives on + /// a monitored instance. Not the customer's problem, so not reported. A deadlock with any other session is kept. + /// + public bool IsOwn => Processes.Count > 0 + && Processes.All(p => p.ClientApp?.StartsWith(OwnApplicationName, StringComparison.Ordinal) == true); + + /// Application Name prefix on all of SQLInsight Pro's connections ("SQLInsight Pro", "SQLInsight Pro (repository)"). + public const string OwnApplicationName = "SQLInsight Pro"; } public sealed record DeadlockProcess diff --git a/src/SQLInsight.Repository/InsightRepository.cs b/src/SQLInsight.Repository/InsightRepository.cs index 90a546c..e4cc68f 100644 --- a/src/SQLInsight.Repository/InsightRepository.cs +++ b/src/SQLInsight.Repository/InsightRepository.cs @@ -14,6 +14,23 @@ namespace SQLInsight.Repository; /// public sealed partial class InsightRepository(string connectionString) { + /// Application Name on repository connections, so SQLInsight's own sessions are recognisable (e.g. in deadlock graphs). + public const string ApplicationName = DeadlockEvent.OwnApplicationName + " (repository)"; + + private readonly string _connectionString = WithApplicationName(connectionString); + + /// Sets unless the connection string names an application itself. + public static string WithApplicationName(string connectionString) + { + var builder = new SqlConnectionStringBuilder(connectionString); + if (builder.ApplicationName == new SqlConnectionStringBuilder().ApplicationName) + { + builder.ApplicationName = ApplicationName; + } + + return builder.ConnectionString; + } + static InsightRepository() { // Dapper sends DateTime as the legacy datetime type (≈3 ms steps). Since SQL Server 2016, comparing @@ -26,11 +43,11 @@ public sealed partial class InsightRepository(string connectionString) // PascalCase: OPENJSON ... WITH matches property names case-sensitively. internal static readonly JsonSerializerOptions Json = new(JsonSerializerDefaults.General); - public Task MigrateAsync(CancellationToken cancellationToken = default) => SchemaMigrator.MigrateAsync(connectionString, cancellationToken); + public Task MigrateAsync(CancellationToken cancellationToken = default) => SchemaMigrator.MigrateAsync(_connectionString, cancellationToken); private async Task OpenAsync(CancellationToken cancellationToken) { - var connection = new SqlConnection(connectionString); + var connection = new SqlConnection(_connectionString); await connection.OpenAsync(cancellationToken).ConfigureAwait(false); return connection; } @@ -101,12 +118,43 @@ public sealed partial class InsightRepository(string connectionString) public async Task SaveRuleResultsAsync(int instanceId, IReadOnlyList results, DateTimeOffset evaluatedAt, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(results); + for (var attempt = 1; ; attempt++) + { + try + { + return await SaveRuleResultsOnceAsync(instanceId, results, evaluatedAt, cancellationToken).ConfigureAwait(false); + } + catch (SqlException ex) when (ex.Number == DeadlockVictim && attempt < 3) + { + // The whole transaction was rolled back, so running it again is safe. + await Task.Delay(TimeSpan.FromMilliseconds(250 * attempt), cancellationToken).ConfigureAwait(false); + } + } + } + + private const int DeadlockVictim = 1205; + + private async Task SaveRuleResultsOnceAsync(int instanceId, IReadOnlyList results, DateTimeOffset evaluatedAt, CancellationToken cancellationToken) + { var now = Utc(evaluatedAt); var opened = new List<(long, Finding)>(); var resolved = new List(); await using var connection = await OpenAsync(cancellationToken).ConfigureAwait(false); await using var transaction = (SqlTransaction)await connection.BeginTransactionAsync(cancellationToken).ConfigureAwait(false); + // Instances are monitored in parallel, and each saves all its rules in one transaction. While insight.Findings + // is small, SQL Server scans it, so two instances' transactions lock each other's rows in opposite order and + // deadlock. The agent is the only writer and a save takes milliseconds: take turns. + var lockResult = await connection.ExecuteScalarAsync(""" + DECLARE @result int; + EXEC @result = sp_getapplock @Resource = N'SQLInsight:Findings', @LockMode = N'Exclusive', @LockOwner = N'Transaction', @LockTimeout = 60000; + SELECT @result; + """, transaction: transaction).ConfigureAwait(false); + if (lockResult < 0) + { + throw new TimeoutException($"Kunne ikke få lås på fundene (sp_getapplock {lockResult})."); + } + foreach (var result in results) { await connection.ExecuteAsync(""" diff --git a/tests/SQLInsight.Collectors.Tests/DeadlockParserTests.cs b/tests/SQLInsight.Collectors.Tests/DeadlockParserTests.cs index dfecc82..62a2396 100644 --- a/tests/SQLInsight.Collectors.Tests/DeadlockParserTests.cs +++ b/tests/SQLInsight.Collectors.Tests/DeadlockParserTests.cs @@ -66,4 +66,21 @@ public class DeadlockParserTests Assert.Null(DeadlockParser.ParseEvent(evt)); } + + [Theory] + [InlineData("SQLInsight Pro (repository)", "SQLInsight Pro (repository)", true)] + [InlineData("SQLInsight Pro", "SQLInsight Pro (repository)", true)] + [InlineData("SQLInsight Pro (repository)", "Sales.Api", false)] + [InlineData("SQLInsight Pro (repository)", null, false)] + public void Deadlocks_between_sqlinsights_own_sessions_are_recognised(string first, string? second, bool own) + { + var deadlock = new SQLInsight.Core.Data.DeadlockEvent + { + Time = DateTimeOffset.UnixEpoch, + GraphXml = "", + Processes = [new() { Id = "p1", ClientApp = first }, new() { Id = "p2", ClientApp = second }], + }; + + Assert.Equal(own, deadlock.IsOwn); + } } diff --git a/tests/SQLInsight.IntegrationTests/RepositoryTests.cs b/tests/SQLInsight.IntegrationTests/RepositoryTests.cs index 1a84392..7a6831c 100644 --- a/tests/SQLInsight.IntegrationTests/RepositoryTests.cs +++ b/tests/SQLInsight.IntegrationTests/RepositoryTests.cs @@ -66,6 +66,46 @@ public class RepositoryTests(SqlServerFixture sql) : IAsyncLifetime Reason = outcome == RuleOutcome.NotMeasurable ? "Kræver Query Store" : null, }; + [Fact] + public async Task Parallel_saves_for_different_instances_do_not_deadlock() + { + // The agent monitors instances in parallel; each saves all its rules in one transaction. On a small Findings + // table this deadlocked (seen at a customer, where the repository sits on a monitored instance). + var a = await _repository.UpsertInstanceAsync("sql-a", Info, "SqlClient", false, Now); + var b = await _repository.UpsertInstanceAsync("sql-b", Info, "SqlClient", false, Now); + + async Task RunAsync(int instanceId) + { + for (var round = 0; round < 40; round++) + { + string[] subjects = round % 2 == 0 ? ["x", "y"] : ["y", "z"]; + var results = Enumerable.Range(1, 25).Select(r => + { + var ruleId = $"SQLI-PAR-{r:000}"; + return new RuleResult + { + RuleId = ruleId, + RuleTitle = "Parallel", + Outcome = RuleOutcome.Findings, + Findings = subjects.Select(s => Finding(s) with { RuleId = ruleId }).ToList(), + }; + }).ToList(); + await _repository.SaveRuleResultsAsync(instanceId, results, Now.AddMinutes(round)); + } + } + + await Task.WhenAll(RunAsync(a), RunAsync(b)); + + // Last round was odd: y and z active for 25 rules on both instances. + Assert.Equal(100, await ScalarAsync("SELECT COUNT(*) FROM insight.Findings WHERE ResolvedAt IS NULL")); + } + + [Theory] + [InlineData("Server=.;Database=SQLInsight;Integrated Security=true", "SQLInsight Pro (repository)")] + [InlineData("Server=.;Database=SQLInsight;Integrated Security=true;Application Name=Eget navn", "Eget navn")] + public void Repository_connections_are_named_unless_the_string_names_them(string connectionString, string expected) => + Assert.Equal(expected, new SqlConnectionStringBuilder(InsightRepository.WithApplicationName(connectionString)).ApplicationName); + [Fact] public async Task Findings_are_tracked_across_evaluations() {