fix: keep the connection loop alive through connect faults and purges - #90
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are still race/cancellation-edge cases in the new guards/exception handling that can reintroduce loop-ending faults or misleading operational behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves resilience of per-server connection supervision by preventing connection loops from dying permanently due to (1) socket connect-time exceptions and (2) guild purges deleting server rows while loops are still running.
Changes:
- Treat exceptions thrown during
ConnectAsyncas anUnreachableoutcome (with warning logging) so the same loop backs off and retries instead of terminating. - Stop each server’s connection loop before deleting its
RustServerrow during guild purge, via a newIServerConnectionStopperabstraction. - Add/adjust tests to cover connect-fault retry/disposal/logging and purge ordering, and add a guard to avoid inserting connection-state rows for deleted servers.
File summaries
| File | Description |
|---|---|
| tests/RustPlusBot.Persistence.Tests/Connections/ConnectionStoreTests.cs | Adds a test ensuring upsert to a deleted server writes nothing and reports no change. |
| tests/RustPlusBot.Features.Workspace.Tests/Teardown/GuildPurgeServiceTests.cs | Adds a test ensuring purge stops connections before deleting server rows. |
| tests/RustPlusBot.Features.Connections.Tests/ConnectionSupervisorTests.cs | Replaces prior contract test with retry/dispose/logging tests for connect-time exceptions. |
| tests/RustPlusBot.Features.Connections.Tests/ConnectionRegistrationTests.cs | Asserts IServerConnectionStopper resolves from DI to protect purge wiring. |
| src/RustPlusBot.Persistence/Connections/ConnectionStore.cs | Adds server-existence check before inserting a new ConnectionState. |
| src/RustPlusBot.Features.Workspace/Teardown/GuildPurgeService.cs | Stops each server connection before deleting the server row during purge. |
| src/RustPlusBot.Features.Connections/Supervisor/IConnectionSupervisor.cs | Makes supervisor implement IServerConnectionStopper and removes duplicate StopAsync declaration. |
| src/RustPlusBot.Features.Connections/Supervisor/ConnectionSupervisor.cs | Adds warning log + broad catch around ConnectAsync to keep the loop alive. |
| src/RustPlusBot.Features.Connections/ConnectionServiceCollectionExtensions.cs | Registers IServerConnectionStopper backed by ConnectionSupervisor. |
| src/RustPlusBot.Abstractions/Connections/IServerConnectionStopper.cs | Introduces the new seam for stopping a server connection loop from other layers. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #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; | ||
| } |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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).
| // 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 |
There was a problem hiding this comment.
Fixed in 6d226ee. UpsertStatusAsync now wraps the insert: on DbUpdateException it detaches the doomed entity and re-checks the parent, returning false only when the server is really gone and rethrowing otherwise, so a genuine store failure still surfaces.
Two tests, both watched failing first: UpsertStatus_WhenTheServerIsDeletedMidSave_... uses a SaveChangesInterceptor to delete the row between the check and the insert (it threw SQLite Error 19 before), and UpsertStatus_WhenTheSaveFailsForAnotherReason_Throws pins the rethrow with a concurrent primary-key collision.
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) <noreply@anthropic.com>
Two ways a per-server connection loop could die and never come back. Nothing re-arms a dead loop —
EnsureConnectionAsyncfires only on server registration, a credential change, or a button press, andStartAllAsynconly at boot — so an affected server stayed offline until the process restarted.Both were found while reviewing #89 and deliberately left out of it as behaviour changes.
A throwing socket source ended the loop
RunAsynccaught onlyOperationCanceledExceptionaroundConnectAsync. Anything else reached the outercatch (Exception), which logs and returns — killing the loop and leaking the half-open socket, which no other reference points at.It is now treated as the
Unreachableit is: logged at Warning with the exception attached, then it falls through to the existing Unreachable branch, which disposes, publishes the status and backs off.RustPlusSocketSourcealready maps failures toUnreachable, but only under awhen (!cancellationToken.IsCancellationRequested)filter, so a connect failure racing a cancellation still escapes.A guild purge deleted server rows out from under running loops
ServerRemovalServicestops the socket before the row delete and its comment names this exact hazard.GuildPurgeService(/purge, dev-gated) never touched the supervisor: every loop in the guild kept running, faulted on the connection-state foreign key at its next status write, and leaked its socket for the life of the process.Features.Workspacecannot referenceFeatures.Connections, so this goes through a newIServerConnectionStopperinAbstractions.Connections— the seam patternIRustServerQueryalready uses.ConnectionStore.UpsertStatusAsyncrefuses to insert a status row for a server that no longer exists. This covers every status write in the race, not just theNoCredentialsone — a loop mid-cycle would equally fault publishingConnectingorUnreachable.Tests
Each was written first and watched fail for the right reason:
UpsertStatus_ForAServerThatIsGoneSqliteException: SQLite Error 19(FK constraint)PurgeGuild_StopsEachServersConnection_WhileItsRowStillExistsExpected: [True], Actual: [False]with the stop wired after the deleteServices_ResolveNo service for type 'IServerConnectionStopper'Faulting_connect_is_retried_by_the_same_loopFaulting_connect_disposes_the_socket_it_createdFaulting_connect_is_loggedFaulting_connect_is_logged_and_leaves_the_supervisor_restartableis replaced: it pinned the old contract ("the loop itself ends"), which is the behaviour being fixed.Full suite: 1658 passed, 20 projects.
Not addressed here
source.Create(...)remains outside the try — a throwing factory would still end the loop, though no socket exists yet to leak.RunAsync(a DB outage duringPublishStatusAsync, say) still end the loop. This removes the FK case from that class without restructuring the loop.🤖 Generated with Claude Code