From b4f722641da91d0387ac79a9c2e6b244bec38d4d Mon Sep 17 00:00:00 2001 From: = Date: Tue, 8 Sep 2026 15:08:33 +0200 Subject: [PATCH 1/2] fix: keep the connection loop alive through connect faults and purges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a per-server connection loop could die and never come back — nothing re-arms a dead loop, so the server stayed offline until the process restarted. A throwing socket source ended the loop. Only OperationCanceledException was caught around ConnectAsync; anything else reached the outer catch (Exception), which logs and returns, leaving the half-open socket undisposed. Treat it as the Unreachable it is: log with the exception, dispose, back off, retry on the same loop. A guild purge deleted RustServer rows out from under running loops. ServerRemovalService already stops the socket before the row delete and says why; GuildPurgeService never touched the supervisor, so every loop in the guild faulted on the connection-state foreign key at its next status write and leaked its socket. It now stops each connection first, through a new IServerConnectionStopper seam (Features.Workspace cannot reference Features.Connections, so this mirrors the IRustServerQuery pattern). ConnectionStore.UpsertStatusAsync also refuses to insert a status row for a server that no longer exists, which covers every status write in that race rather than only the NoCredentials one. Co-Authored-By: Claude Opus 5 (1M context) --- .../Connections/IServerConnectionStopper.cs | 14 ++++ .../ConnectionServiceCollectionExtensions.cs | 1 + .../Supervisor/ConnectionSupervisor.cs | 17 +++++ .../Supervisor/IConnectionSupervisor.cs | 9 +-- .../Teardown/GuildPurgeService.cs | 14 +++- .../Connections/ConnectionStore.cs | 12 +++ .../ConnectionRegistrationTests.cs | 4 + .../ConnectionSupervisorTests.cs | 76 ++++++++++++++++--- .../Teardown/GuildPurgeServiceTests.cs | 52 ++++++++++++- .../Connections/ConnectionStoreTests.cs | 20 +++++ 10 files changed, 199 insertions(+), 20 deletions(-) create mode 100644 src/RustPlusBot.Abstractions/Connections/IServerConnectionStopper.cs diff --git a/src/RustPlusBot.Abstractions/Connections/IServerConnectionStopper.cs b/src/RustPlusBot.Abstractions/Connections/IServerConnectionStopper.cs new file mode 100644 index 00000000..2a80007c --- /dev/null +++ b/src/RustPlusBot.Abstractions/Connections/IServerConnectionStopper.cs @@ -0,0 +1,14 @@ +namespace RustPlusBot.Abstractions.Connections; + +/// +/// Stops the live socket for one server (implemented by the connection supervisor). Exists so layers that +/// delete a server's rows can shut its connection loop down first without depending on the connections +/// feature: a loop still running when its RustServer row disappears faults on the status row's foreign key. +/// +public interface IServerConnectionStopper +{ + /// Stops the connection for one server, if running. Returns once the loop has ended. + /// The owning guild snowflake. + /// The server. + Task StopAsync(ulong guildId, Guid serverId); +} diff --git a/src/RustPlusBot.Features.Connections/ConnectionServiceCollectionExtensions.cs b/src/RustPlusBot.Features.Connections/ConnectionServiceCollectionExtensions.cs index 3955392b..2112005b 100644 --- a/src/RustPlusBot.Features.Connections/ConnectionServiceCollectionExtensions.cs +++ b/src/RustPlusBot.Features.Connections/ConnectionServiceCollectionExtensions.cs @@ -28,6 +28,7 @@ public static IServiceCollection AddConnections(this IServiceCollection services services.AddSingleton(sp => sp.GetRequiredService()); services.AddSingleton(); services.AddSingleton(sp => sp.GetRequiredService()); + services.AddSingleton(sp => sp.GetRequiredService()); services.AddSingleton(sp => sp.GetRequiredService()); services.AddScoped(); services.AddScoped(); diff --git a/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs b/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs index 41f19dd1..be4a838f 100644 --- a/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs +++ b/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs @@ -483,6 +483,11 @@ public async Task SetClanMotdAsync( [LoggerMessage(Level = LogLevel.Error, Message = "Connection loop for server {ServerId} faulted.")] private static partial void LogLoopFaulted(ILogger logger, Exception exception, Guid serverId); + [LoggerMessage( + Level = LogLevel.Warning, + Message = "Socket for server {ServerId} threw while connecting; treating as unreachable and retrying.")] + private static partial void LogConnectThrew(ILogger logger, Exception exception, Guid serverId); + [LoggerMessage(Level = LogLevel.Error, Message = "Stored token for credential {CredentialId} is unreadable.")] private static partial void LogUnreadableToken(ILogger logger, Exception exception, Guid credentialId); @@ -526,6 +531,18 @@ await PublishStatusAsync(key, ConnectionStatus.Connecting, null, p.CredentialId, await connection.DisposeAsync().ConfigureAwait(false); throw; } +#pragma warning disable CA1031 // Broad catch is intentional: see below — a throwing source must not end the loop. + catch (Exception ex) +#pragma warning restore CA1031 + { + // A source that throws instead of reporting an outcome is still just an unreachable + // server. Letting it escape reaches the outer catch and ENDS the loop, and nothing + // re-arms a dead loop, so the server would stay offline until the process restarts. + // Fall through as Unreachable: the branch below disposes the socket, publishes the + // status and backs off, exactly as for a reported failure. + LogConnectThrew(logger, ex, key.Server); + outcome = SocketConnectOutcome.Unreachable; + } if (outcome == SocketConnectOutcome.AuthRejected) { diff --git a/src/RustPlusBot.Features.Connections/Supervisor/IConnectionSupervisor.cs b/src/RustPlusBot.Features.Connections/Supervisor/IConnectionSupervisor.cs index 6457f328..907a9c18 100644 --- a/src/RustPlusBot.Features.Connections/Supervisor/IConnectionSupervisor.cs +++ b/src/RustPlusBot.Features.Connections/Supervisor/IConnectionSupervisor.cs @@ -1,7 +1,9 @@ +using RustPlusBot.Abstractions.Connections; + namespace RustPlusBot.Features.Connections.Supervisor; /// Owns the live Rust+ sockets — one per (guild, server). -internal interface IConnectionSupervisor +internal interface IConnectionSupervisor : IServerConnectionStopper { /// Starts a connection for every server that has a non-Invalid credential (called once at startup). /// A cancellation token. @@ -13,11 +15,6 @@ internal interface IConnectionSupervisor /// A cancellation token. Task EnsureConnectionAsync(ulong guildId, Guid serverId, CancellationToken cancellationToken = default); - /// Stops the connection for one server, if running. - /// The owning guild snowflake. - /// The server. - Task StopAsync(ulong guildId, Guid serverId); - /// Cancels and disposes every connection (called on shutdown). Task StopAllAsync(); } diff --git a/src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs b/src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs index e9a0c438..d4dd2761 100644 --- a/src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs +++ b/src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs @@ -1,4 +1,5 @@ using Microsoft.EntityFrameworkCore; +using RustPlusBot.Abstractions.Connections; using RustPlusBot.Features.Workspace.Reconciler; using RustPlusBot.Persistence; using RustPlusBot.Persistence.Servers; @@ -10,11 +11,13 @@ namespace RustPlusBot.Features.Workspace.Teardown; /// Server management (RemoveAsync cascades all per-server rows). /// Removes provisioned Discord channels/categories/messages. /// Held across the whole purge to block concurrent reconciliation. +/// Stops each server's connection loop before its row is deleted. internal sealed class GuildPurgeService( BotDbContext context, IServerService servers, WorkspaceTeardownService teardown, - IProvisioningLock provisioningLock) : IGuildPurgeService + IProvisioningLock provisioningLock, + IServerConnectionStopper connections) : IGuildPurgeService { /// public async Task PurgeGuildAsync(ulong guildId, CancellationToken cancellationToken = default) @@ -30,10 +33,15 @@ public async Task PurgeGuildAsync(ulong guildId, CancellationToken cancellationT // 2) Remove each server; the RustServer FK cascade clears its per-server rows // (connection state, command/map settings, switches, alarms, storage monitors, credentials). + // Stop the socket BEFORE each row delete, exactly as ServerRemovalService does for a single + // server: a connection loop still running when its RustServer row goes away faults on the + // connection-state foreign key at its next status write, and leaks its socket for the life of + // the process. StopAsync joins the loop, so it is finished before the delete lands. var known = await servers.ListAsync(guildId, cancellationToken).ConfigureAwait(false); - foreach (var server in known) + foreach (var serverId in known.Select(server => server.Id)) { - await servers.RemoveAsync(guildId, server.Id, cancellationToken).ConfigureAwait(false); + await connections.StopAsync(guildId, serverId).ConfigureAwait(false); + await servers.RemoveAsync(guildId, serverId, cancellationToken).ConfigureAwait(false); } // 3) Delete guild-keyed rows that have no cascade FK to RustServer (event subscriptions, diff --git a/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs b/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs index fae04d57..9f18a6ac 100644 --- a/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs +++ b/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs @@ -42,6 +42,18 @@ public async Task UpsertStatusAsync( if (existing is null) { + // The row is FK'd to RustServers with ON DELETE CASCADE, so a deleted server takes its status + // row with it. A connection loop still running at that moment would insert a fresh row against + // the missing parent and die on the constraint violation. A server that is gone has no status + // to record: report "no change" rather than faulting the caller. + var serverExists = await context.RustServers + .AnyAsync(s => s.Id == serverId && s.GuildId == guildId, cancellationToken) + .ConfigureAwait(false); + if (!serverExists) + { + return false; + } + context.ConnectionStates.Add(new ConnectionState { RustServerId = serverId, diff --git a/tests/RustPlusBot.Features.Connections.Tests/ConnectionRegistrationTests.cs b/tests/RustPlusBot.Features.Connections.Tests/ConnectionRegistrationTests.cs index 568020d0..3079625c 100644 --- a/tests/RustPlusBot.Features.Connections.Tests/ConnectionRegistrationTests.cs +++ b/tests/RustPlusBot.Features.Connections.Tests/ConnectionRegistrationTests.cs @@ -1,6 +1,7 @@ using Discord.WebSocket; using Microsoft.Extensions.DependencyInjection; using NSubstitute; +using RustPlusBot.Abstractions.Connections; using RustPlusBot.Abstractions.Credentials; using RustPlusBot.Abstractions.Events; using RustPlusBot.Abstractions.Time; @@ -39,6 +40,9 @@ public async Task Services_Resolve() Assert.NotNull(provider.GetRequiredService()); Assert.NotNull(provider.GetRequiredService()); + // The workspace purge stops connections through this seam; an unregistered stopper + // breaks GuildPurgeService at resolve time, not at compile time. + Assert.NotNull(provider.GetRequiredService()); await using var scope = provider.CreateAsyncScope(); Assert.NotNull(scope.ServiceProvider.GetRequiredService()); Assert.NotNull(scope.ServiceProvider.GetRequiredService()); diff --git a/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs b/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs index ba7875f5..40af531a 100644 --- a/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs +++ b/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs @@ -1080,13 +1080,13 @@ public async Task Heartbeat_AuthRejected_MidWindow_FailsOverAndReconnects() } /// - /// A socket library that throws while connecting (rather than reporting an outcome) must not take the - /// host down, and must leave the supervisor able to start the server again. The loop itself ends — that - /// is the documented contract of the outer catch — so the regression this pins is that the failure is - /// LOGGED and CONTAINED rather than silently swallowed or propagated. + /// A socket library that throws while connecting (rather than reporting an outcome) must be treated as + /// the Unreachable it is: back off and try again on the SAME loop. Ending the loop strands the server — + /// nothing re-arms a dead one (EnsureConnectionAsync fires only on registration, a credential change, or + /// a button press), so it stays offline until the process restarts. /// [Fact] - public async Task Faulting_connect_is_logged_and_leaves_the_supervisor_restartable() + public async Task Faulting_connect_is_retried_by_the_same_loop() { var source = new FakeRustSocketSource(); source.LastConnectionSetup = c => @@ -1100,16 +1100,72 @@ public async Task Faulting_connect_is_logged_and_leaves_the_supervisor_restartab await using var h = CreateHarness(source); var (serverId, _, _) = await SeedAsync(h.Provider); + // Started ONCE: the recovery has to come from the loop itself, not a second EnsureConnectionAsync. + await h.Supervisor.EnsureConnectionAsync(10UL, serverId); + + var state = await WaitForStateAsync( + h.Provider, serverId, s => s.Status == ConnectionStatus.Connected && s.PlayerCount == 9); + Assert.NotNull(state); + } + + /// + /// The socket created for a connect attempt that throws must still be disposed: the loop holds the only + /// reference, so a retry storm against a broken server would otherwise leak one half-open WebSocket per + /// attempt, forever. + /// + [Fact] + public async Task Faulting_connect_disposes_the_socket_it_created() + { + var source = new FakeRustSocketSource(); + FakeRustSocketSource.FakeConnection? faulted = null; + source.LastConnectionSetup = c => + { + if (source.CreateCount == 1) + { + c.ConnectFault = new InvalidOperationException("socket library faulted"); + faulted = c; + } + }; + source.EnqueueHeartbeat(HeartbeatResult.Ok(9)); + await using var h = CreateHarness(source); + var (serverId, _, _) = await SeedAsync(h.Provider); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); await h.Supervisor.EnsureConnectionAsync(10UL, serverId, cts.Token); - await WaitForLogAsync(h, LogLevel.Error, "faulted", cts.Token); + await WaitUntilAsync(() => faulted is { DisposeCount: > 0 }, cts.Token); - // The second attempt gets a healthy socket: nothing about the fault is sticky. + Assert.Equal(1, faulted!.DisposeCount); + } + + /// + /// A throwing socket library is a defect in the library or its wrapper, not a normal unreachable server. + /// The retry keeps the bot alive but must not hide it: the exception has to reach the log. + /// + [Fact] + public async Task Faulting_connect_is_logged() + { + var source = new FakeRustSocketSource(); + source.LastConnectionSetup = c => + { + if (source.CreateCount == 1) + { + c.ConnectFault = new InvalidOperationException("socket library faulted"); + } + }; + source.EnqueueHeartbeat(HeartbeatResult.Ok(9)); + await using var h = CreateHarness(source); + var (serverId, _, _) = await SeedAsync(h.Provider); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); await h.Supervisor.EnsureConnectionAsync(10UL, serverId, cts.Token); - var state = await WaitForStateAsync( - h.Provider, serverId, s => s.Status == ConnectionStatus.Connected && s.PlayerCount == 9); - Assert.NotNull(state); + await WaitForLogAsync(h, LogLevel.Warning, "threw while connecting", cts.Token); + + // The exception itself must ride along: the message alone says a socket misbehaved, not how. + var record = h.Logs.Records.Single(r => r.Level == LogLevel.Warning + && r.Message.Contains("threw while connecting", + StringComparison.Ordinal)); + Assert.IsType(record.Exception); } /// diff --git a/tests/RustPlusBot.Features.Workspace.Tests/Teardown/GuildPurgeServiceTests.cs b/tests/RustPlusBot.Features.Workspace.Tests/Teardown/GuildPurgeServiceTests.cs index 26c896e1..992b7a57 100644 --- a/tests/RustPlusBot.Features.Workspace.Tests/Teardown/GuildPurgeServiceTests.cs +++ b/tests/RustPlusBot.Features.Workspace.Tests/Teardown/GuildPurgeServiceTests.cs @@ -1,6 +1,7 @@ using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; using NSubstitute; +using RustPlusBot.Abstractions.Connections; using RustPlusBot.Domain.Connections; using RustPlusBot.Domain.Credentials; using RustPlusBot.Domain.Entities; @@ -92,7 +93,9 @@ public async Task PurgeGuild_RemovesTargetGuildRows_AndLeavesOtherGuildIntact() .Returns(noCategories); var provisioningLock = new ProvisioningLock(); var teardown = new WorkspaceTeardownService(gateway, store, provisioningLock); - var service = new GuildPurgeService(context, new ServerService(context), teardown, provisioningLock); + var service = new GuildPurgeService( + context, new ServerService(context), teardown, provisioningLock, + Substitute.For()); await service.PurgeGuildAsync(1); @@ -111,4 +114,51 @@ public async Task PurgeGuild_RemovesTargetGuildRows_AndLeavesOtherGuildIntact() Assert.Single(await context.GuildSettings.Where(g => g.GuildId == 2).ToListAsync()); Assert.Single(await context.FcmRegistrations.Where(f => f.GuildId == 2).ToListAsync()); } + + /// + /// The purge deletes each RustServer row, and the connection loop for that server may still be running. + /// It must be stopped FIRST: a live loop whose server row has vanished faults on the foreign key the + /// moment it writes its next status, and its socket stays open for the life of the process. + /// + [Fact] + public async Task PurgeGuild_StopsEachServersConnection_WhileItsRowStillExists() + { + var connection = new SqliteConnection("DataSource=:memory:"); + await connection.OpenAsync(); + await using var _ = connection; + await using var context = NewContext(connection); + + var server = new RustServer + { + GuildId = 1, Name = "A", Ip = "a", Port = 1 + }; + context.RustServers.Add(server); + await context.SaveChangesAsync(); + + var gateway = Substitute.For(); + var store = Substitute.For(); + IReadOnlyList noCategories = []; + store.GetAllCategoriesAsync(Arg.Any(), Arg.Any()).Returns(noCategories); + var provisioningLock = new ProvisioningLock(); + var teardown = new WorkspaceTeardownService(gateway, store, provisioningLock); + + // Records whether the server row was still present at the moment the stop was requested — the + // ordering is the whole point, so asserting the call happened is not enough. + var rowPresentAtStop = new List(); + var stopper = Substitute.For(); + stopper.StopAsync(Arg.Any(), Arg.Any()).Returns(call => + { + var id = call.ArgAt(1); + rowPresentAtStop.Add(context.RustServers.AsNoTracking().Any(s => s.Id == id)); + return Task.CompletedTask; + }); + + var service = new GuildPurgeService( + context, new ServerService(context), teardown, provisioningLock, stopper); + + await service.PurgeGuildAsync(1); + + await stopper.Received(1).StopAsync(1UL, server.Id); + Assert.Equal([true], rowPresentAtStop); + } } diff --git a/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs b/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs index a4a84abc..da09363d 100644 --- a/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs +++ b/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs @@ -1,4 +1,5 @@ using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; using NSubstitute; using RustPlusBot.Abstractions.Time; using RustPlusBot.Domain.Connections; @@ -72,6 +73,25 @@ public async Task UpsertStatus_InsertsThenReportsChangeOnlyWhenDifferent() Assert.Equal(6, state.PlayerCount); } + /// + /// A connection loop can still be running when its server row is deleted (a guild purge racing the + /// loop). The status row is FK'd to RustServers, so inserting one for a server that is gone throws a + /// constraint violation and kills the loop. A deleted server has no status to record: write nothing. + /// + [Fact] + public async Task UpsertStatus_ForAServerThatIsGone_WritesNothingAndReportsNoChange() + { + var (store, context, conn) = Create(); + await using var _ = conn; + await using var __ = context; + + var changed = await store.UpsertStatusAsync( + 10UL, Guid.NewGuid(), ConnectionStatus.NoCredentials, null, null); + + Assert.False(changed); + Assert.Empty(await context.ConnectionStates.ToListAsync()); + } + [Fact] public async Task GetActiveCredential_ReturnsTheActiveOne() { From 6d226eec1c7a66f0a43fe5a922bc5ef810b64662 Mon Sep 17 00:00:00 2001 From: = Date: Tue, 8 Sep 2026 15:23:14 +0200 Subject: [PATCH 2/2] fix: keep stops quiet and close the status-insert race Addresses review feedback on the loop-resilience changes. A socket library that unwinds cancellation as something other than an OperationCanceledException (a client disposed underneath the connect) was being reported as Unreachable and logged as a connect failure. It is a stop, not a connectivity problem: dispose and end the loop quietly. The existence check before inserting a connection-state row and the insert itself are two round trips, so the server could still be deleted in between and the foreign-key violation would fault the caller anyway. The insert now handles DbUpdateException by re-checking the parent: gone means "no change", anything else still surfaces. Co-Authored-By: Claude Opus 5 (1M context) --- .../Supervisor/ConnectionSupervisor.cs | 9 ++ .../Connections/ConnectionStore.cs | 31 ++++- .../ConnectionSupervisorTests.cs | 34 +++++ .../Fakes/FakeRustSocketSource.cs | 16 ++- .../Connections/ConnectionStoreTests.cs | 121 ++++++++++++++++++ 5 files changed, 204 insertions(+), 7 deletions(-) diff --git a/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs b/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs index be4a838f..42c6a15e 100644 --- a/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs +++ b/src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs @@ -531,6 +531,15 @@ await PublishStatusAsync(key, ConnectionStatus.Connecting, null, p.CredentialId, await connection.DisposeAsync().ConfigureAwait(false); throw; } + catch (Exception) when (ct.IsCancellationRequested) + { + // Stopping. A socket library may unwind cancellation as something other than an + // OperationCanceledException (a client disposed underneath the connect, say). Dispose + // and end the loop as the cancellation it is — reporting the server unreachable and + // warning about a connect failure here would be noise pointing at the wrong problem. + await connection.DisposeAsync().ConfigureAwait(false); + return; + } #pragma warning disable CA1031 // Broad catch is intentional: see below — a throwing source must not end the loop. catch (Exception ex) #pragma warning restore CA1031 diff --git a/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs b/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs index 9f18a6ac..eda066be 100644 --- a/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs +++ b/src/RustPlusBot.Persistence/Connections/ConnectionStore.cs @@ -46,15 +46,12 @@ public async Task UpsertStatusAsync( // row with it. A connection loop still running at that moment would insert a fresh row against // the missing parent and die on the constraint violation. A server that is gone has no status // to record: report "no change" rather than faulting the caller. - var serverExists = await context.RustServers - .AnyAsync(s => s.Id == serverId && s.GuildId == guildId, cancellationToken) - .ConfigureAwait(false); - if (!serverExists) + if (!await ServerExistsAsync().ConfigureAwait(false)) { return false; } - context.ConnectionStates.Add(new ConnectionState + var added = context.ConnectionStates.Add(new ConnectionState { RustServerId = serverId, GuildId = guildId, @@ -63,7 +60,26 @@ public async Task UpsertStatusAsync( ActiveCredentialId = activeCredentialId, UpdatedAt = clock.UtcNow, }); - await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + + try + { + await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + } + catch (DbUpdateException) + { + // The check above and this insert are two round trips, so the server can still be deleted + // in between. Drop the doomed entity (it would be retried by the next SaveChanges on this + // context) and re-check: only a vanished parent is expected here, so anything else — a + // genuine store failure — must still surface to the caller. + added.State = EntityState.Detached; + if (await ServerExistsAsync().ConfigureAwait(false)) + { + throw; + } + + return false; + } + return true; } @@ -80,6 +96,9 @@ public async Task UpsertStatusAsync( existing.UpdatedAt = clock.UtcNow; await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); return true; + + Task ServerExistsAsync() => context.RustServers + .AnyAsync(s => s.Id == serverId && s.GuildId == guildId, cancellationToken); } /// diff --git a/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs b/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs index 40af531a..88e1ab0b 100644 --- a/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs +++ b/tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs @@ -1168,6 +1168,40 @@ public async Task Faulting_connect_is_logged() Assert.IsType(record.Exception); } + /// + /// A socket library may unwind cancellation as something other than an OperationCanceledException — a + /// client disposed underneath the connect throwing ObjectDisposedException, say. That is a stop, not a + /// connectivity failure: the loop must end quietly, disposing its socket, without reporting the server + /// unreachable or logging a connect warning that would send someone hunting a network problem. + /// + [Fact] + public async Task Connect_unwinding_as_a_non_cancellation_exception_on_stop_ends_quietly() + { + var source = new FakeRustSocketSource + { + LastConnectionSetup = c => + { + c.BlockConnectUntilCancelled = true; + c.ConnectFaultOnCancel = new ObjectDisposedException("socket"); + } + }; + await using var h = CreateHarness(source); + var (serverId, _, _) = await SeedAsync(h.Provider); + + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + await h.Supervisor.EnsureConnectionAsync(10UL, serverId, cts.Token); + await WaitUntilAsync(() => source.LastConnection is not null, cts.Token); + var connecting = source.LastConnection!; + + // StopAsync joins the loop, so once it returns no further log or dispose can race these asserts. + await h.Supervisor.StopAsync(10UL, serverId).WaitAsync(TimeSpan.FromSeconds(30), cts.Token); + + Assert.Equal(1, connecting.DisposeCount); + Assert.DoesNotContain( + h.Logs.Records, + r => r.Message.Contains("threw while connecting", StringComparison.Ordinal)); + } + /// /// Stopping a server whose connect is still in flight must complete and must dispose the half-open /// socket. A leaked socket here accumulates one live WebSocket per stop/start cycle. diff --git a/tests/RustPlusBot.Features.Connections.Tests/Fakes/FakeRustSocketSource.cs b/tests/RustPlusBot.Features.Connections.Tests/Fakes/FakeRustSocketSource.cs index df591435..96f3c91a 100644 --- a/tests/RustPlusBot.Features.Connections.Tests/Fakes/FakeRustSocketSource.cs +++ b/tests/RustPlusBot.Features.Connections.Tests/Fakes/FakeRustSocketSource.cs @@ -252,6 +252,13 @@ internal sealed class FakeConnection(SocketConnectOutcome outcome, FakeRustSocke /// public bool BlockConnectUntilCancelled { get; set; } + /// + /// When set together with , the cancelled connect unwinds by + /// throwing THIS instead of — models a socket library that + /// surfaces cancellation as something else (a disposed client throwing ObjectDisposedException, say). + /// + public Exception? ConnectFaultOnCancel { get; set; } + /// When set, throws this instead of answering. public Exception? TeamInfoFault { get; set; } @@ -408,7 +415,14 @@ public async Task ConnectAsync(TimeSpan timeout, Cancellat if (BlockConnectUntilCancelled) { - await Task.Delay(Timeout.Infinite, cancellationToken).ConfigureAwait(false); + try + { + await Task.Delay(Timeout.Infinite, cancellationToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (ConnectFaultOnCancel is not null) + { + throw ConnectFaultOnCancel; + } } return outcome; diff --git a/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs b/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs index da09363d..e908740c 100644 --- a/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs +++ b/tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs @@ -1,5 +1,6 @@ using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using NSubstitute; using RustPlusBot.Abstractions.Time; using RustPlusBot.Domain.Connections; @@ -11,6 +12,17 @@ namespace RustPlusBot.Persistence.Tests.Connections; public sealed class ConnectionStoreTests { + private static BotDbContext NewContext(SqliteConnection connection, IInterceptor interceptor) + { + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .AddInterceptors(interceptor) + .Options; + var context = new BotDbContext(options); + context.Database.Migrate(); + return context; + } + private static (ConnectionStore Store, BotDbContext Context, SqliteConnection Conn) Create() { var (context, connection) = SqliteContextFixture.Create(); @@ -92,6 +104,88 @@ public async Task UpsertStatus_ForAServerThatIsGone_WritesNothingAndReportsNoCha Assert.Empty(await context.ConnectionStates.ToListAsync()); } + /// + /// The existence check and the insert are two separate round trips, so the server row can still be + /// deleted in between. The foreign-key violation that follows must not escape: it would fault the + /// connection loop that called in, which is the very outcome the check exists to prevent. + /// + [Fact] + public async Task UpsertStatus_WhenTheServerIsDeletedMidSave_WritesNothingAndReportsNoChange() + { + var connection = new SqliteConnection("DataSource=:memory:"); + await connection.OpenAsync(); + await using var _ = connection; + + var deleter = new InterferingWriteInterceptor((ctx, ct) => + ctx.Database.ExecuteSqlRawAsync("DELETE FROM RustServers", ct)); + await using var context = NewContext(connection, deleter); + + var server = new RustServer + { + GuildId = 10UL, Name = "S", Ip = "1.1.1.1", Port = 28015 + }; + context.RustServers.Add(server); + await context.SaveChangesAsync(); + + // Armed only now: the seed above must survive, and the delete must land between the store's + // existence check and its insert. + deleter.Arm(); + var clock = Substitute.For(); + clock.UtcNow.Returns(DateTimeOffset.UnixEpoch); + var store = new ConnectionStore(context, clock); + + var changed = await store.UpsertStatusAsync( + 10UL, server.Id, ConnectionStatus.Unreachable, null, null); + + Assert.False(changed); + Assert.Empty(await context.ConnectionStates.ToListAsync()); + } + + /// + /// Only a vanished parent may be swallowed. Any other write failure — here a status row inserted + /// concurrently, colliding on the primary key — is a real store problem and must reach the caller + /// rather than being reported as "nothing changed". + /// + [Fact] + public async Task UpsertStatus_WhenTheSaveFailsForAnotherReason_Throws() + { + var connection = new SqliteConnection("DataSource=:memory:"); + await connection.OpenAsync(); + await using var _ = connection; + + var serverId = Guid.Empty; + var conflicter = new InterferingWriteInterceptor(async (ctx, ct) => + { + // A second context over the SAME connection, so the row lands before the outer insert runs. + var options = new DbContextOptionsBuilder() + .UseSqlite(ctx.Database.GetDbConnection()) + .Options; + await using var other = new BotDbContext(options); + other.ConnectionStates.Add(new ConnectionState + { + RustServerId = serverId, GuildId = 10UL, Status = ConnectionStatus.Connected + }); + await other.SaveChangesAsync(ct); + }); + await using var context = NewContext(connection, conflicter); + + var server = new RustServer + { + GuildId = 10UL, Name = "S", Ip = "1.1.1.1", Port = 28015 + }; + context.RustServers.Add(server); + await context.SaveChangesAsync(); + serverId = server.Id; + + conflicter.Arm(); + var clock = Substitute.For(); + clock.UtcNow.Returns(DateTimeOffset.UnixEpoch); + var store = new ConnectionStore(context, clock); + + await Assert.ThrowsAsync(() => + store.UpsertStatusAsync(10UL, serverId, ConnectionStatus.Unreachable, null, null)); + } + [Fact] public async Task GetActiveCredential_ReturnsTheActiveOne() { @@ -192,4 +286,31 @@ public async Task GetStatesForGuild_ReturnsOnlyThatGuildsStates() Assert.Equal(serverA.Id, Assert.Single(states).RustServerId); Assert.Empty(await store.GetStatesForGuildAsync(30UL)); } + + /// + /// Runs from inside SaveChanges, once armed — the seam for reproducing a + /// concurrent write that lands between a caller's read and its own SaveChanges. + /// + /// The interfering write, given the saving context. + private sealed class InterferingWriteInterceptor(Func action) + : SaveChangesInterceptor + { + private bool _armed; + + public void Arm() => _armed = true; + + public override async ValueTask> SavingChangesAsync( + DbContextEventData eventData, + InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (_armed && eventData.Context is BotDbContext context) + { + _armed = false; + await action(context, cancellationToken).ConfigureAwait(false); + } + + return result; + } + } }