Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
namespace RustPlusBot.Abstractions.Connections;

/// <summary>
/// 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.
/// </summary>
public interface IServerConnectionStopper
{
/// <summary>Stops the connection for one server, if running. Returns once the loop has ended.</summary>
/// <param name="guildId">The owning guild snowflake.</param>
/// <param name="serverId">The server.</param>
Task StopAsync(ulong guildId, Guid serverId);
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public static IServiceCollection AddConnections(this IServiceCollection services
services.AddSingleton<IChatSender>(sp => sp.GetRequiredService<ConnectionSupervisor>());
services.AddSingleton<IBotTeamChatSender, BotTeamChatSender>();
services.AddSingleton<IRustServerQuery>(sp => sp.GetRequiredService<ConnectionSupervisor>());
services.AddSingleton<IServerConnectionStopper>(sp => sp.GetRequiredService<ConnectionSupervisor>());
services.AddSingleton<IAfkState>(sp => sp.GetRequiredService<ConnectionSupervisor>());
services.AddScoped<IServerRemovalService, ServerRemovalService>();
services.AddScoped<ServerResolver>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,11 @@ public async Task<bool> 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);

Expand Down Expand Up @@ -526,6 +531,27 @@ 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
{
// 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;
}
Comment on lines +543 to +554

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6d226ee. A catch (Exception) when (ct.IsCancellationRequested) now sits ahead of the broad catch: it disposes the socket and ends the loop quietly, matching the OperationCanceledException path, so no Unreachable status or connect warning is produced for what is really a stop.

Covered by Connect_unwinding_as_a_non_cancellation_exception_on_stop_ends_quietly, which parks a connect until cancellation and then throws ObjectDisposedException. It failed on the warning assertion before the change.


if (outcome == SocketConnectOutcome.AuthRejected)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using RustPlusBot.Abstractions.Connections;

namespace RustPlusBot.Features.Connections.Supervisor;

/// <summary>Owns the live Rust+ sockets — one per (guild, server).</summary>
internal interface IConnectionSupervisor
internal interface IConnectionSupervisor : IServerConnectionStopper
{
/// <summary>Starts a connection for every server that has a non-Invalid credential (called once at startup).</summary>
/// <param name="cancellationToken">A cancellation token.</param>
Expand All @@ -13,11 +15,6 @@ internal interface IConnectionSupervisor
/// <param name="cancellationToken">A cancellation token.</param>
Task EnsureConnectionAsync(ulong guildId, Guid serverId, CancellationToken cancellationToken = default);

/// <summary>Stops the connection for one server, if running.</summary>
/// <param name="guildId">The owning guild snowflake.</param>
/// <param name="serverId">The server.</param>
Task StopAsync(ulong guildId, Guid serverId);

/// <summary>Cancels and disposes every connection (called on shutdown).</summary>
Task StopAllAsync();
}
14 changes: 11 additions & 3 deletions src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.EntityFrameworkCore;
using RustPlusBot.Abstractions.Connections;
using RustPlusBot.Features.Workspace.Reconciler;
using RustPlusBot.Persistence;
using RustPlusBot.Persistence.Servers;
Expand All @@ -10,11 +11,13 @@ namespace RustPlusBot.Features.Workspace.Teardown;
/// <param name="servers">Server management (RemoveAsync cascades all per-server rows).</param>
/// <param name="teardown">Removes provisioned Discord channels/categories/messages.</param>
/// <param name="provisioningLock">Held across the whole purge to block concurrent reconciliation.</param>
/// <param name="connections">Stops each server's connection loop before its row is deleted.</param>
internal sealed class GuildPurgeService(
BotDbContext context,
IServerService servers,
WorkspaceTeardownService teardown,
IProvisioningLock provisioningLock) : IGuildPurgeService
IProvisioningLock provisioningLock,
IServerConnectionStopper connections) : IGuildPurgeService
{
/// <inheritdoc />
public async Task PurgeGuildAsync(ulong guildId, CancellationToken cancellationToken = default)
Expand All @@ -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);
Comment on lines 40 to +44

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one — the missing token is deliberate, and adding it would reintroduce the bug this PR fixes.

ConnectionSupervisor.StopAsync takes CancellationToken.None on the gate on purpose, with the reason in the code: teardown must still acquire the gate after StopAllAsync has cancelled _shutdown, otherwise the connection is never stopped. ServerRemovalService follows the same rule — it has a token in scope and deliberately does not pass one to StopAsync.

A cancellable stop here would mean a purge that is cancelled mid-flight leaves a live socket attached to a server row that is about to be deleted: exactly the fault this PR closes. The stop is also bounded rather than open-ended — it cancels the loop token and joins, and parked polls unwind on cancellation (Team_poll_parked_in_a_request_does_not_wedge_teardown pins that).

}

// 3) Delete guild-keyed rows that have no cascade FK to RustServer (event subscriptions,
Expand Down
35 changes: 33 additions & 2 deletions src/RustPlusBot.Persistence/Connections/ConnectionStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,16 @@ public async Task<bool> UpsertStatusAsync(

if (existing is null)
{
context.ConnectionStates.Add(new ConnectionState
// 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.
if (!await ServerExistsAsync().ConfigureAwait(false))
{
return false;
}

var added = context.ConnectionStates.Add(new ConnectionState
{
RustServerId = serverId,
GuildId = guildId,
Expand All @@ -51,7 +60,26 @@ public async Task<bool> 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;
}

Expand All @@ -68,6 +96,9 @@ public async Task<bool> UpsertStatusAsync(
existing.UpdatedAt = clock.UtcNow;
await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false);
return true;

Task<bool> ServerExistsAsync() => context.RustServers
.AnyAsync(s => s.Id == serverId && s.GuildId == guildId, cancellationToken);
}

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -39,6 +40,9 @@ public async Task Services_Resolve()

Assert.NotNull(provider.GetRequiredService<IConnectionSupervisor>());
Assert.NotNull(provider.GetRequiredService<IBotTeamChatSender>());
// The workspace purge stops connections through this seam; an unregistered stopper
// breaks GuildPurgeService at resolve time, not at compile time.
Assert.NotNull(provider.GetRequiredService<IServerConnectionStopper>());
await using var scope = provider.CreateAsyncScope();
Assert.NotNull(scope.ServiceProvider.GetRequiredService<IConnectionStore>());
Assert.NotNull(scope.ServiceProvider.GetRequiredService<IServerRemovalService>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1080,13 +1080,69 @@ public async Task Heartbeat_AuthRejected_MidWindow_FailsOverAndReconnects()
}

/// <summary>
/// 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.
/// </summary>
[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 =>
{
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);

// 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);
}

/// <summary>
/// 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.
/// </summary>
[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 WaitUntilAsync(() => faulted is { DisposeCount: > 0 }, cts.Token);

Assert.Equal(1, faulted!.DisposeCount);
}

/// <summary>
/// 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.
/// </summary>
[Fact]
public async Task Faulting_connect_is_logged()
{
var source = new FakeRustSocketSource();
source.LastConnectionSetup = c =>
Expand All @@ -1102,14 +1158,48 @@ public async Task Faulting_connect_is_logged_and_leaves_the_supervisor_restartab

using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30));
await h.Supervisor.EnsureConnectionAsync(10UL, serverId, cts.Token);
await WaitForLogAsync(h, LogLevel.Error, "faulted", cts.Token);

// The second attempt gets a healthy socket: nothing about the fault is sticky.
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<InvalidOperationException>(record.Exception);
}

/// <summary>
/// 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.
/// </summary>
[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!;

var state = await WaitForStateAsync(
h.Provider, serverId, s => s.Status == ConnectionStatus.Connected && s.PlayerCount == 9);
Assert.NotNull(state);
// 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));
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@ internal sealed class FakeConnection(SocketConnectOutcome outcome, FakeRustSocke
/// </summary>
public bool BlockConnectUntilCancelled { get; set; }

/// <summary>
/// When set together with <see cref="BlockConnectUntilCancelled"/>, the cancelled connect unwinds by
/// throwing THIS instead of <see cref="OperationCanceledException"/> — models a socket library that
/// surfaces cancellation as something else (a disposed client throwing ObjectDisposedException, say).
/// </summary>
public Exception? ConnectFaultOnCancel { get; set; }

/// <summary>When set, <see cref="GetTeamInfoAsync"/> throws this instead of answering.</summary>
public Exception? TeamInfoFault { get; set; }

Expand Down Expand Up @@ -408,7 +415,14 @@ public async Task<SocketConnectOutcome> 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;
Expand Down
Loading
Loading