Skip to content

Commit 2553af0

Browse files
johnsimonsjohnsimons
committed
Fix schemas stranded in shared database when tests run multiple scenarios
Tests that run more than one scenario call CustomizeSettings multiple times, creating a new schema each time, but the old single-field approach only tracked the most recent. When Cleanup ran, earlier schemas were left behind in the shared database. Switching to ConcurrentBag accumulates every schema and body storage path created across all scenarios, so Cleanup drains and removes all of them. The one-shot cleanupStarted guard is also removed, since iterating the bag is already idempotent. Schema setup and teardown on SQL Server also now retries on deadlock (error 1205), which occurs when many tests concurrently hit the system catalogs with DDL. DropSchema additionally switches to READ UNCOMMITTED to avoid taking shared locks on catalog reads that were themselves entering deadlocks.
1 parent 041a47a commit 2553af0

3 files changed

Lines changed: 59 additions & 37 deletions

File tree

‎src/ServiceControl.AcceptanceTests.PostgreSql/AcceptanceTestStorageConfiguration.cs‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
namespace ServiceControl.AcceptanceTests.PostgreSql;
22

33
using System;
4+
using System.Collections.Concurrent;
45
using System.IO;
56
using System.Threading;
67
using System.Threading.Tasks;
@@ -16,11 +17,17 @@ public class AcceptanceTestStorageConfiguration : IAcceptanceTestStorageConfigur
1617

1718
public async Task CustomizeSettings(Settings settings, CancellationToken cancellationToken = default)
1819
{
19-
schema = $"sc_at_{Guid.NewGuid():n}";
20+
var schema = $"sc_at_{Guid.NewGuid():n}";
21+
var bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName;
22+
2023
connectionString = await PostgreSqlSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false);
2124
await TestSchema.Create(connectionString, schema, cancellationToken).ConfigureAwait(false);
2225

23-
bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName;
26+
// A test that runs more than one scenario comes back through here, and the runner cleans up
27+
// after each one. Recording everything created, rather than keeping only the most recent,
28+
// is what stops the earlier schema being stranded in the shared database.
29+
schemas.Add(schema);
30+
bodyStoragePaths.Add(bodyStoragePath);
2431

2532
settings.PersisterSpecificSettings = new PostgreSqlPersisterSettings
2633
{
@@ -33,23 +40,16 @@ public async Task CustomizeSettings(Settings settings, CancellationToken cancell
3340

3441
public async Task Cleanup(CancellationToken cancellationToken = default)
3542
{
36-
if (Interlocked.Exchange(ref cleanupStarted, 1) != 0)
37-
{
38-
return;
39-
}
40-
4143
try
4244
{
43-
if (connectionString == null || schema == null)
45+
while (schemas.TryTake(out var schema))
4446
{
45-
return;
47+
await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false);
4648
}
47-
48-
await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false);
4949
}
5050
finally
5151
{
52-
if (bodyStoragePath != null)
52+
while (bodyStoragePaths.TryTake(out var bodyStoragePath))
5353
{
5454
try
5555
{
@@ -77,8 +77,7 @@ public void Dispose()
7777
}
7878
}
7979

80+
readonly ConcurrentBag<string> schemas = [];
81+
readonly ConcurrentBag<string> bodyStoragePaths = [];
8082
string connectionString;
81-
string schema;
82-
string bodyStoragePath;
83-
int cleanupStarted;
84-
}
83+
}

‎src/ServiceControl.AcceptanceTests.SqlServer/AcceptanceTestStorageConfiguration.cs‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
namespace ServiceControl.AcceptanceTests.SqlServer;
22

33
using System;
4+
using System.Collections.Concurrent;
45
using System.IO;
56
using System.Threading;
67
using System.Threading.Tasks;
@@ -16,11 +17,17 @@ public class AcceptanceTestStorageConfiguration : IAcceptanceTestStorageConfigur
1617

1718
public async Task CustomizeSettings(Settings settings, CancellationToken cancellationToken = default)
1819
{
19-
schema = $"sc_at_{Guid.NewGuid():n}";
20+
var schema = $"sc_at_{Guid.NewGuid():n}";
21+
var bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName;
22+
2023
connectionString = await SqlServerSharedContainer.GetConnectionStringAsync(cancellationToken).ConfigureAwait(false);
2124
await TestSchema.Create(connectionString, schema, cancellationToken).ConfigureAwait(false);
2225

23-
bodyStoragePath = Directory.CreateTempSubdirectory("sc_at_bodies_").FullName;
26+
// A test that runs more than one scenario comes back through here, and the runner cleans up
27+
// after each one. Recording everything created, rather than keeping only the most recent,
28+
// is what stops the earlier schema being stranded in the shared database.
29+
schemas.Add(schema);
30+
bodyStoragePaths.Add(bodyStoragePath);
2431

2532
settings.PersisterSpecificSettings = new SqlServerPersisterSettings
2633
{
@@ -33,23 +40,16 @@ public async Task CustomizeSettings(Settings settings, CancellationToken cancell
3340

3441
public async Task Cleanup(CancellationToken cancellationToken = default)
3542
{
36-
if (Interlocked.Exchange(ref cleanupStarted, 1) != 0)
37-
{
38-
return;
39-
}
40-
4143
try
4244
{
43-
if (connectionString == null || schema == null)
45+
while (schemas.TryTake(out var schema))
4446
{
45-
return;
47+
await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false);
4648
}
47-
48-
await TestSchema.Drop(connectionString, schema, cancellationToken).ConfigureAwait(false);
4949
}
5050
finally
5151
{
52-
if (bodyStoragePath != null)
52+
while (bodyStoragePaths.TryTake(out var bodyStoragePath))
5353
{
5454
try
5555
{
@@ -77,8 +77,7 @@ public void Dispose()
7777
}
7878
}
7979

80+
readonly ConcurrentBag<string> schemas = [];
81+
readonly ConcurrentBag<string> bodyStoragePaths = [];
8082
string connectionString;
81-
string schema;
82-
string bodyStoragePath;
83-
int cleanupStarted;
8483
}

‎src/ServiceControl.Persistence.Tests.SqlServer/TestSchema.cs‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
namespace ServiceControl.Persistence.Tests;
22

3+
using System;
34
using System.Threading;
45
using System.Threading.Tasks;
56
using Microsoft.Data.SqlClient;
@@ -14,15 +15,33 @@ public static Task Create(string connectionString, string schema, CancellationTo
1415
public static Task Drop(string connectionString, string schema, CancellationToken cancellationToken = default) =>
1516
Execute(connectionString, DropSchemaSql, schema, cancellationToken);
1617

18+
// Tests share one database now, so a schema being set up or torn down contends on the system
19+
// catalogs with every other test's CREATE and DROP, and with the transport tests, which CI points
20+
// at the same server. SQL Server settles that by picking a victim and asking it to try again,
21+
// which is what error 1205 means.
22+
const int DeadlockVictim = 1205;
23+
const int MaxAttempts = 5;
24+
1725
static async Task Execute(string connectionString, string sql, string schema, CancellationToken cancellationToken)
1826
{
19-
await using var connection = new SqlConnection(connectionString);
20-
await connection.OpenAsync(cancellationToken);
27+
for (var attempt = 1; ; attempt++)
28+
{
29+
try
30+
{
31+
await using var connection = new SqlConnection(connectionString);
32+
await connection.OpenAsync(cancellationToken);
2133

22-
await using var command = connection.CreateCommand();
23-
command.CommandText = sql;
24-
command.Parameters.AddWithValue("@schema", schema);
25-
await command.ExecuteNonQueryAsync(cancellationToken);
34+
await using var command = connection.CreateCommand();
35+
command.CommandText = sql;
36+
command.Parameters.AddWithValue("@schema", schema);
37+
await command.ExecuteNonQueryAsync(cancellationToken);
38+
return;
39+
}
40+
catch (SqlException e) when (e.Number == DeadlockVictim && attempt < MaxAttempts)
41+
{
42+
await Task.Delay(TimeSpan.FromMilliseconds(100 * attempt), cancellationToken);
43+
}
44+
}
2645
}
2746

2847
// CREATE SCHEMA has to be the only statement in its batch, and EXEC will not take a
@@ -36,6 +55,11 @@ IF SCHEMA_ID(@schema) IS NULL
3655
""";
3756

3857
const string DropSchemaSql = """
58+
-- Only this test's own schema is read, and no other session creates or drops anything in it,
59+
-- so a dirty read cannot be wrong here. It does mean the catalog reads take no shared locks,
60+
-- which is what put them in a deadlock with other tests' DDL.
61+
SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;
62+
3963
DECLARE @sql nvarchar(max) = N'';
4064
4165
SELECT @sql = @sql + N'DROP FULLTEXT INDEX ON ' + QUOTENAME(s.name) + N'.' + QUOTENAME(t.name) + N';'

0 commit comments

Comments
 (0)