Stop SQLInsight deadlocking itself, and stop reporting its own deadlocks
At a customer whose repository sits on a monitored instance, the agent reported 12 deadlocks that were its own: two instances' monitors save findings in parallel, one transaction each, and while insight.Findings is small SQL Server scans it, so the two lock each other's rows. - SaveRuleResultsAsync takes an exclusive application lock for its transaction (the agent is the only writer; a save takes milliseconds) and retries if chosen as deadlock victim anyway - repository connections are named "SQLInsight Pro (repository)" - deadlocks where every participant is SQLInsight Pro are not reported (the read position still moves past them); any other session keeps it The new integration test (two instances saving 25 rules x 40 rounds in parallel) failed 2 of 2 without the fix and passed 5 of 5 with it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
e508b0c702
commit
25a67cb98b
5 changed files with 120 additions and 4 deletions
|
|
@ -105,7 +105,8 @@ public sealed class DeadlockCollector : Collector<DeadlockSnapshot>
|
|||
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<DeadlockSnapshot>
|
|||
return new DeadlockSnapshot
|
||||
{
|
||||
CapturedAt = context.Now,
|
||||
NewDeadlocks = deadlocks,
|
||||
NewDeadlocks = deadlocks.Where(d => !d.IsOwn).ToList(),
|
||||
LastEventTime = deadlocks.Count > 0 ? deadlocks[^1].Time : since,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,6 +26,16 @@ public sealed record DeadlockEvent
|
|||
|
||||
/// <summary>The raw deadlock graph (XDL), for download and viewing in SSMS.</summary>
|
||||
public required string GraphXml { get; init; }
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public bool IsOwn => Processes.Count > 0
|
||||
&& Processes.All(p => p.ClientApp?.StartsWith(OwnApplicationName, StringComparison.Ordinal) == true);
|
||||
|
||||
/// <summary>Application Name prefix on all of SQLInsight Pro's connections ("SQLInsight Pro", "SQLInsight Pro (repository)").</summary>
|
||||
public const string OwnApplicationName = "SQLInsight Pro";
|
||||
}
|
||||
|
||||
public sealed record DeadlockProcess
|
||||
|
|
|
|||
|
|
@ -14,6 +14,23 @@ namespace SQLInsight.Repository;
|
|||
/// </summary>
|
||||
public sealed partial class InsightRepository(string connectionString)
|
||||
{
|
||||
/// <summary>Application Name on repository connections, so SQLInsight's own sessions are recognisable (e.g. in deadlock graphs).</summary>
|
||||
public const string ApplicationName = DeadlockEvent.OwnApplicationName + " (repository)";
|
||||
|
||||
private readonly string _connectionString = WithApplicationName(connectionString);
|
||||
|
||||
/// <summary>Sets <see cref="ApplicationName"/> unless the connection string names an application itself.</summary>
|
||||
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<int> MigrateAsync(CancellationToken cancellationToken = default) => SchemaMigrator.MigrateAsync(connectionString, cancellationToken);
|
||||
public Task<int> MigrateAsync(CancellationToken cancellationToken = default) => SchemaMigrator.MigrateAsync(_connectionString, cancellationToken);
|
||||
|
||||
private async Task<SqlConnection> 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<FindingChanges> SaveRuleResultsAsync(int instanceId, IReadOnlyList<RuleResult> 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<FindingChanges> SaveRuleResultsOnceAsync(int instanceId, IReadOnlyList<RuleResult> results, DateTimeOffset evaluatedAt, CancellationToken cancellationToken)
|
||||
{
|
||||
var now = Utc(evaluatedAt);
|
||||
var opened = new List<(long, Finding)>();
|
||||
var resolved = new List<ResolvedFinding>();
|
||||
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<int>("""
|
||||
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("""
|
||||
|
|
|
|||
|
|
@ -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 = "<deadlock />",
|
||||
Processes = [new() { Id = "p1", ClientApp = first }, new() { Id = "p2", ClientApp = second }],
|
||||
};
|
||||
|
||||
Assert.Equal(own, deadlock.IsOwn);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<int>("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()
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue