fix: address Copilot review on PR #38 (atomicity, N+1, wording, purge lock)

- DatabaseMaintenanceService: wipe in a single transaction with
  defer_foreign_keys so an interruption rolls back instead of leaving a
  partially-cleared DB; throw on an unexpected table identifier instead of
  silently skipping it (and reporting a misleading success).
- DiagnosticsModule /status: replace the per-server GetStateAsync N+1 with a
  single IConnectionStore.GetStatesForGuildAsync bulk read + dictionary lookup.
- MaintenanceModule: clearer danger-off message for the operator-facing /admin
  group instead of "Developer commands are disabled".
- GuildPurgeService: hold the per-guild provisioning lock across the whole
  purge (teardown + domain deletes) so concurrent reconciliation — including
  the self-heal triggered by teardown deleting channels — cannot re-provision
  into a half-purged guild. Adds a lock-free WorkspaceTeardownService core.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: HandyS11

src/RustPlusBot.Features.Commands/Modules/DiagnosticsModule.cs

@@ -47,6 +47,8 @@ public async Task StatusAsync()
             var connections = scope.ServiceProvider.GetRequiredService<IConnectionStore>();
 
             var known = await servers.ListAsync(Context.Guild.Id).ConfigureAwait(false);
+            var states = (await connections.GetStatesForGuildAsync(Context.Guild.Id).ConfigureAwait(false))
+                .ToDictionary(state => state.RustServerId);
 
             var embed = new EmbedBuilder()
                 .WithTitle("Bot status")
@@ -61,11 +63,10 @@ public async Task StatusAsync()
             // Cap server fields so the embed stays under Discord's 25-field limit (4 header fields above).
             foreach (var server in known.Take(20))
             {
-                var state = await connections.GetStateAsync(Context.Guild.Id, server.Id).ConfigureAwait(false);
-                var line = state is null
-                    ? "unknown"
-                    : string.Create(CultureInfo.InvariantCulture,
-                        $"{state.Status} · {state.PlayerCount?.ToString(CultureInfo.InvariantCulture) ?? "?"} players");
+                var line = states.TryGetValue(server.Id, out var state)
+                    ? string.Create(CultureInfo.InvariantCulture,
+                        $"{state.Status} · {state.PlayerCount?.ToString(CultureInfo.InvariantCulture) ?? "?"} players")
+                    : "unknown";
                 embed.AddField(server.Name, line);
             }
 

src/RustPlusBot.Features.Commands/Modules/MaintenanceModule.cs

@@ -30,7 +30,9 @@ public async Task ResetDatabaseAsync(
 
         if (!options.Value.EnableDangerCommands)
         {
-            await RespondAsync("Developer commands are disabled.", ephemeral: true).ConfigureAwait(false);
+            await RespondAsync(
+                "Dangerous maintenance commands are disabled. Set `Workspace:EnableDangerCommands` to enable them.",
+                ephemeral: true).ConfigureAwait(false);
             return;
         }
 

src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs

@@ -1,4 +1,5 @@
 using Microsoft.EntityFrameworkCore;
+using RustPlusBot.Features.Workspace.Reconciler;
 using RustPlusBot.Persistence;
 using RustPlusBot.Persistence.Servers;
 
@@ -8,16 +9,24 @@ namespace RustPlusBot.Features.Workspace.Teardown;
 /// <param name="context">The bot database context.</param>
 /// <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>
 internal sealed class GuildPurgeService(
     BotDbContext context,
     IServerService servers,
-    IWorkspaceTeardownService teardown) : IGuildPurgeService
+    WorkspaceTeardownService teardown,
+    IProvisioningLock provisioningLock) : IGuildPurgeService
 {
     /// <inheritdoc />
     public async Task PurgeGuildAsync(ulong guildId, CancellationToken cancellationToken = default)
     {
-        // 1) Delete provisioned Discord channels/categories/messages (Discord side + records).
-        await teardown.ResetGuildAsync(guildId, cancellationToken).ConfigureAwait(false);
+        // Hold the per-guild provisioning lock across the ENTIRE purge. Otherwise reconciliation could
+        // interleave after the teardown step — in particular the self-heal that fires when teardown
+        // deletes channels — and re-provision resources or add rows into a half-purged guild.
+        using var handle = await provisioningLock.AcquireAsync(guildId, cancellationToken).ConfigureAwait(false);
+
+        // 1) Delete provisioned Discord channels/categories/messages (Discord side + records). Use the
+        //    lock-free core since we already hold the lock (ResetGuildAsync would deadlock re-acquiring).
+        await teardown.ResetGuildCoreAsync(guildId, cancellationToken).ConfigureAwait(false);
 
         // 2) Remove each server; the RustServer FK cascade clears its per-server rows
         //    (connection state, command/map settings, switches, alarms, storage monitors, credentials).

src/RustPlusBot.Features.Workspace/Teardown/WorkspaceTeardownService.cs

@@ -24,6 +24,19 @@ public async Task RemoveServerAsync(ulong guildId, Guid serverId, CancellationTo
     public async Task ResetGuildAsync(ulong guildId, CancellationToken cancellationToken = default)
     {
         using var handle = await provisioningLock.AcquireAsync(guildId, cancellationToken).ConfigureAwait(false);
+        await ResetGuildCoreAsync(guildId, cancellationToken).ConfigureAwait(false);
+    }
+
+    /// <summary>
+    /// Deletes the guild's provisioned resources and records WITHOUT taking the provisioning lock.
+    /// The caller MUST already hold the guild's <see cref="IProvisioningLock"/> — <see cref="GuildPurgeService"/>
+    /// uses this so it can hold the lock across the wider purge. External callers use <see cref="ResetGuildAsync"/>.
+    /// </summary>
+    /// <param name="guildId">The guild to reset.</param>
+    /// <param name="cancellationToken">A cancellation token.</param>
+    /// <returns>A task.</returns>
+    internal async Task ResetGuildCoreAsync(ulong guildId, CancellationToken cancellationToken = default)
+    {
         var categories = await store.GetAllCategoriesAsync(guildId, cancellationToken).ConfigureAwait(false);
         foreach (var category in categories)
         {

src/RustPlusBot.Persistence/Connections/ConnectionStore.cs

@@ -18,6 +18,15 @@ public sealed class ConnectionStore(BotDbContext context, IClock clock) : IConne
         context.ConnectionStates
             .SingleOrDefaultAsync(s => s.GuildId == guildId && s.RustServerId == serverId, cancellationToken);
 
+    /// <inheritdoc />
+    public async Task<IReadOnlyList<ConnectionState>> GetStatesForGuildAsync(
+        ulong guildId,
+        CancellationToken cancellationToken = default) =>
+        await context.ConnectionStates
+            .Where(s => s.GuildId == guildId)
+            .ToListAsync(cancellationToken)
+            .ConfigureAwait(false);
+
     /// <inheritdoc />
     public async Task<bool> UpsertStatusAsync(
         ulong guildId,

src/RustPlusBot.Persistence/Connections/IConnectionStore.cs

@@ -16,6 +16,14 @@ public interface IConnectionStore
     /// <returns>The connection state, or null.</returns>
     Task<ConnectionState?> GetStateAsync(ulong guildId, Guid serverId, CancellationToken cancellationToken = default);
 
+    /// <summary>Gets every server's connection state for a guild in a single query.</summary>
+    /// <param name="guildId">Owning Discord guild snowflake.</param>
+    /// <param name="cancellationToken">A cancellation token.</param>
+    /// <returns>The guild's connection states (empty if none).</returns>
+    Task<IReadOnlyList<ConnectionState>> GetStatesForGuildAsync(
+        ulong guildId,
+        CancellationToken cancellationToken = default);
+
     /// <summary>
     /// Upserts the connection state. Returns true only when the persisted (status, player count, active
     /// credential) actually changed, so callers can skip a redundant #info refresh.

src/RustPlusBot.Persistence/Maintenance/DatabaseMaintenanceService.cs

@@ -16,35 +16,34 @@ public async Task ClearAllAsync(CancellationToken cancellationToken = default)
             .Distinct(StringComparer.Ordinal)
             .ToList();
 
-        // Keep one connection open across every statement so the FK pragma persists
-        // (with per-statement connections the pragma would reset before the DELETEs).
-        await context.Database.OpenConnectionAsync(cancellationToken).ConfigureAwait(false);
-        try
+        // Wipe every table in one transaction so an interruption rolls back rather than leaving the
+        // database partially cleared. defer_foreign_keys defers FK enforcement to commit time (and
+        // resets itself when the transaction ends), so tables can be cleared in any order — once every
+        // table is empty the commit-time check has nothing to violate. This also avoids leaving a
+        // connection-level foreign_keys pragma toggled off on a pooled connection.
+        var transaction = await context.Database.BeginTransactionAsync(cancellationToken).ConfigureAwait(false);
+        await using (transaction.ConfigureAwait(false))
         {
-            await context.Database.ExecuteSqlRawAsync("PRAGMA foreign_keys = OFF", cancellationToken)
+            await context.Database.ExecuteSqlRawAsync("PRAGMA defer_foreign_keys = ON", cancellationToken)
                 .ConfigureAwait(false);
 
             foreach (var table in tables)
             {
-                // Deletion order is deliberately irrelevant: PRAGMA foreign_keys = OFF (above) suspends
-                // FK enforcement for the wipe, so do not "fix" this by adding a topological sort.
-                // Table names come from the EF model (never user input); the identifier guard keeps
-                // the raw statement demonstrably injection-safe for the Sonar gate.
+                // Table names come from the EF model (never user input). Fail loud on an unexpected
+                // identifier rather than silently skipping it and reporting a misleading success; the
+                // guard also keeps the raw statement demonstrably injection-safe for the Sonar gate.
                 if (!IsSafeIdentifier(table!))
                 {
-                    continue;
+                    throw new InvalidOperationException(
+                        string.Create(CultureInfo.InvariantCulture,
+                            $"Refusing to clear table with an unexpected identifier: '{table}'."));
                 }
 
                 var sql = string.Create(CultureInfo.InvariantCulture, $"DELETE FROM \"{table}\"");
                 await context.Database.ExecuteSqlRawAsync(sql, cancellationToken).ConfigureAwait(false);
             }
 
-            await context.Database.ExecuteSqlRawAsync("PRAGMA foreign_keys = ON", cancellationToken)
-                .ConfigureAwait(false);
-        }
-        finally
-        {
-            await context.Database.CloseConnectionAsync().ConfigureAwait(false);
+            await transaction.CommitAsync(cancellationToken).ConfigureAwait(false);
         }
     }
 

tests/RustPlusBot.Features.Workspace.Tests/Teardown/GuildPurgeServiceTests.cs

@@ -8,9 +8,13 @@
 using RustPlusBot.Domain.Guilds;
 using RustPlusBot.Domain.Servers;
 using RustPlusBot.Domain.Switches;
+using RustPlusBot.Domain.Workspace;
+using RustPlusBot.Features.Workspace.Gateway;
+using RustPlusBot.Features.Workspace.Reconciler;
 using RustPlusBot.Features.Workspace.Teardown;
 using RustPlusBot.Persistence;
 using RustPlusBot.Persistence.Servers;
+using RustPlusBot.Persistence.Workspace;
 
 namespace RustPlusBot.Features.Workspace.Tests.Teardown;
 
@@ -79,12 +83,20 @@ public async Task PurgeGuild_RemovesTargetGuildRows_AndLeavesOtherGuildIntact()
         });
         await context.SaveChangesAsync();
 
-        var teardown = Substitute.For<IWorkspaceTeardownService>();
-        var service = new GuildPurgeService(context, new ServerService(context), teardown);
+        // Real teardown over fake Discord I/O, sharing the lock the purge holds. An empty category set
+        // makes the teardown core a no-op on channels while still proving it runs under the held lock.
+        var gateway = Substitute.For<IWorkspaceGateway>();
+        var store = Substitute.For<IWorkspaceStore>();
+        IReadOnlyList<ProvisionedCategory> noCategories = [];
+        store.GetAllCategoriesAsync(Arg.Any<ulong>(), Arg.Any<CancellationToken>())
+            .Returns(noCategories);
+        var provisioningLock = new ProvisioningLock();
+        var teardown = new WorkspaceTeardownService(gateway, store, provisioningLock);
+        var service = new GuildPurgeService(context, new ServerService(context), teardown, provisioningLock);
 
         await service.PurgeGuildAsync(1);
 
-        await teardown.Received(1).ResetGuildAsync(1, Arg.Any<CancellationToken>());
+        await store.Received(1).GetAllCategoriesAsync(1, Arg.Any<CancellationToken>());
         Assert.Empty(await context.RustServers.Where(s => s.GuildId == 1).ToListAsync());
         Assert.Empty(await context.SmartSwitches.ToListAsync());
         Assert.Empty(await context.ConnectionStates.ToListAsync());

tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs

@@ -147,4 +147,30 @@ public async Task ListConnectableServers_ExcludesAllInvalidServers()
         var after = await store.ListConnectableServersAsync();
         Assert.DoesNotContain((10UL, serverId), after);
     }
+
+    [Fact]
+    public async Task GetStatesForGuild_ReturnsOnlyThatGuildsStates()
+    {
+        var (store, context, conn) = Create();
+        await using var _ = conn;
+        await using var __ = context;
+
+        var serverA = new RustServer
+        {
+            GuildId = 10UL, Name = "A", Ip = "1.1.1.1", Port = 28015
+        };
+        var serverB = new RustServer
+        {
+            GuildId = 20UL, Name = "B", Ip = "2.2.2.2", Port = 28015
+        };
+        context.RustServers.AddRange(serverA, serverB);
+        await context.SaveChangesAsync();
+        await store.UpsertStatusAsync(10UL, serverA.Id, ConnectionStatus.Connected, 5, null);
+        await store.UpsertStatusAsync(20UL, serverB.Id, ConnectionStatus.Connecting, null, null);
+
+        var states = await store.GetStatesForGuildAsync(10UL);
+
+        Assert.Equal(serverA.Id, Assert.Single(states).RustServerId);
+        Assert.Empty(await store.GetStatesForGuildAsync(30UL));
+    }
 }