From 4fe7aa5819a3a196bfd525b82a6e8909b3964a46 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Thu, 6 Aug 2026 16:21:28 +0800 Subject: [PATCH 1/5] Add test coverage for notification data store, move raven ID out of persistence interface --- .../Editing/NotificationSettingsDocument.cs | 9 + .../Editing/NotificationsManager.cs | 15 +- ...ontrol.Persistence.Tests.PostgreSql.csproj | 1 + ...Control.Persistence.Tests.SqlServer.csproj | 1 + .../NotificationsDataStoreTests.cs | 167 ++++++++++++++++++ .../NotificationsSettings.cs | 2 - 6 files changed, 184 insertions(+), 11 deletions(-) create mode 100644 src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs create mode 100644 src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs diff --git a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs new file mode 100644 index 0000000000..7ef8ee17b5 --- /dev/null +++ b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs @@ -0,0 +1,9 @@ +namespace ServiceControl.Persistence.RavenDB.Editing; + +using Notifications; + +class NotificationsSettingsDocument +{ + public string Id { get; set; } + public EmailNotifications Email { get; set; } = new(); +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs index 52e41d6a44..9159e39b64 100644 --- a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs @@ -14,21 +14,18 @@ public async Task LoadSettings(TimeSpan? cacheTimeout = n { using var aggressivelyCacheFor = await Session.Advanced.DocumentStore.AggressivelyCacheForAsync(cacheTimeout ?? CacheTimeoutDefault); var settings = await Session - .LoadAsync(SingleDocumentId); + .LoadAsync(SingleDocumentId); - if (settings != null) + if (settings == null) { - return settings; + settings = new NotificationsSettingsDocument { Id = SingleDocumentId }; + await Session.StoreAsync(settings); } - settings = new NotificationsSettings + return new NotificationsSettings() { - Id = SingleDocumentId + Email = settings.Email }; - - await Session.StoreAsync(settings); - - return settings; } } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj b/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj index b7f3f20807..492db2ec1c 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj @@ -39,6 +39,7 @@ + diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj b/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj index 05b839198a..2e762eb089 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj +++ b/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj @@ -39,6 +39,7 @@ + diff --git a/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs new file mode 100644 index 0000000000..3a7907cd30 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs @@ -0,0 +1,167 @@ +namespace ServiceControl.Persistence.Tests; + +using System.Threading.Tasks; +using NUnit.Framework; + +class NotificationsDataStoreTests : PersistenceTestBase +{ + [Test, CancelAfter(30_000)] + public async Task LoadSettings_returns_defaults_when_no_settings_exist() + { + using var manager = await NotificationsStore.CreateNotificationsManager(); + + var settings = await manager.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(settings, Is.Not.Null); + Assert.That(settings.Email, Is.Not.Null); + Assert.That(settings.Email.Enabled, Is.False); + Assert.That(settings.Email.SmtpServer, Is.Null); + Assert.That(settings.Email.SmtpPort, Is.Null); + Assert.That(settings.Email.EnableTLS, Is.False); + Assert.That(settings.Email.From, Is.Null); + Assert.That(settings.Email.To, Is.Null); + Assert.That(settings.Email.AuthenticationAccount, Is.Null); + Assert.That(settings.Email.AuthenticationPassword, Is.Null); + } + } + + [Test, CancelAfter(30_000)] + public async Task SaveChanges_persists_email_settings_round_trip() + { + using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + + settings.Email.Enabled = true; + settings.Email.SmtpServer = "smtp.example.com"; + settings.Email.SmtpPort = 587; + settings.Email.EnableTLS = true; + settings.Email.From = "sc@example.com"; + settings.Email.To = "ops@example.com"; + settings.Email.AuthenticationAccount = "user"; + settings.Email.AuthenticationPassword = "p@ssw0rd"; + + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + var loaded = await verifyManager.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(loaded.Email.Enabled, Is.True); + Assert.That(loaded.Email.SmtpServer, Is.EqualTo("smtp.example.com")); + Assert.That(loaded.Email.SmtpPort, Is.EqualTo(587)); + Assert.That(loaded.Email.EnableTLS, Is.True); + Assert.That(loaded.Email.From, Is.EqualTo("sc@example.com")); + Assert.That(loaded.Email.To, Is.EqualTo("ops@example.com")); + Assert.That(loaded.Email.AuthenticationAccount, Is.EqualTo("user")); + Assert.That(loaded.Email.AuthenticationPassword, Is.EqualTo("p@ssw0rd")); + } + } + + [Test, CancelAfter(30_000)] + public async Task Toggling_enabled_is_persisted() + { + using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.Enabled = true; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + Assert.That(settings.Email.Enabled, Is.True); + + settings.Email.Enabled = false; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + var final = await verifyManager.LoadSettings(); + Assert.That(final.Email.Enabled, Is.False); + } + + [Test, CancelAfter(30_000)] + public async Task LoadSettings_returns_previously_saved_settings() + { + using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.SmtpServer = "configured.server"; + settings.Email.SmtpPort = 2525; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + using var manager2 = await NotificationsStore.CreateNotificationsManager(); + var loaded = await manager2.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(loaded.Email.SmtpServer, Is.EqualTo("configured.server")); + Assert.That(loaded.Email.SmtpPort, Is.EqualTo(2525)); + // Untouched fields keep their defaults + Assert.That(loaded.Email.Enabled, Is.False); + Assert.That(loaded.Email.EnableTLS, Is.False); + } + } + + [Test, CancelAfter(30_000)] + public async Task Updating_individual_fields_preserves_others() + { + using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.Enabled = true; + settings.Email.SmtpServer = "original.smtp"; + settings.Email.SmtpPort = 25; + settings.Email.EnableTLS = false; + settings.Email.From = "from@orig"; + settings.Email.To = "to@orig"; + settings.Email.AuthenticationAccount = "acct"; + settings.Email.AuthenticationPassword = "secret"; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.SmtpServer = "updated.smtp"; + settings.Email.EnableTLS = true; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + var loaded = await verifyManager.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + // Updated fields + Assert.That(loaded.Email.SmtpServer, Is.EqualTo("updated.smtp")); + Assert.That(loaded.Email.EnableTLS, Is.True); + // Preserved fields + Assert.That(loaded.Email.Enabled, Is.True); + Assert.That(loaded.Email.SmtpPort, Is.EqualTo(25)); + Assert.That(loaded.Email.From, Is.EqualTo("from@orig")); + Assert.That(loaded.Email.To, Is.EqualTo("to@orig")); + Assert.That(loaded.Email.AuthenticationAccount, Is.EqualTo("acct")); + Assert.That(loaded.Email.AuthenticationPassword, Is.EqualTo("secret")); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence/NotificationsSettings.cs b/src/ServiceControl.Persistence/NotificationsSettings.cs index 97bc8c4f55..8fd55a59e2 100644 --- a/src/ServiceControl.Persistence/NotificationsSettings.cs +++ b/src/ServiceControl.Persistence/NotificationsSettings.cs @@ -2,8 +2,6 @@ { public class NotificationsSettings { - public string Id { get; set; } - public EmailNotifications Email { get; set; } = new EmailNotifications(); } } \ No newline at end of file From 2323f66bddeff45fd86025bf5b54555a95af3d45 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Thu, 6 Aug 2026 17:15:44 +0800 Subject: [PATCH 2/5] Disable setting email field, update broken test --- .../When_email_notifications_are_enabled.cs | 10 +++------- .../NotificationsSettings.cs | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs index 20d1e6b30d..b1d88739a8 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs @@ -67,13 +67,9 @@ public async Task StartAsync(CancellationToken cancellationToken) using var notificationsManager = await notificationsDataStore.CreateNotificationsManager(); var settings = await notificationsManager.LoadSettings(); - settings.Email = new EmailNotifications - { - Enabled = true, - From = "YouServiceControl@particular.net", - To = "WhoeverMightBeConcerned@particular.net", - }; - + settings.Email.Enabled = true; + settings.Email.From = "YouServiceControl@particular.net"; + settings.Email.To = "WhoeverMightBeConcerned@particular.net"; await notificationsManager.SaveChanges(); } diff --git a/src/ServiceControl.Persistence/NotificationsSettings.cs b/src/ServiceControl.Persistence/NotificationsSettings.cs index 8fd55a59e2..159052a419 100644 --- a/src/ServiceControl.Persistence/NotificationsSettings.cs +++ b/src/ServiceControl.Persistence/NotificationsSettings.cs @@ -2,6 +2,6 @@ { public class NotificationsSettings { - public EmailNotifications Email { get; set; } = new EmailNotifications(); + public EmailNotifications Email { get; init; } = new EmailNotifications(); } } \ No newline at end of file From 0bc8ee0daddedfad46db7c4a38df676effaf177b Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Fri, 7 Aug 2026 10:00:17 +0800 Subject: [PATCH 3/5] Change IDataSessionManager to IAsyncDisposable --- .../When_email_notifications_are_enabled.cs | 2 +- .../EditFailedMessagesManager.cs | 3 ++- .../Implementation/NotificationsManager.cs | 3 ++- .../RavenTransactionalDataStore.cs | 6 ++++- .../NotificationsDataStoreTests.cs | 22 +++++++++---------- .../Recoverability/EditMessageTests.cs | 22 +++++++++---------- .../IDataSessionManager.cs | 2 +- .../EditFailedMessagesControllerAuditTests.cs | 6 ++--- .../Api/NotificationsController.cs | 8 +++---- .../Email/SendEmailNotificationHandler.cs | 2 +- .../Recoverability/Editing/EditHandler.cs | 2 +- 11 files changed, 40 insertions(+), 38 deletions(-) diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs index b1d88739a8..e797fe8edd 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs @@ -64,7 +64,7 @@ class SetupNotificationSettings(INotificationsDataStore notificationsDataStore) { public async Task StartAsync(CancellationToken cancellationToken) { - using var notificationsManager = await notificationsDataStore.CreateNotificationsManager(); + await using var notificationsManager = await notificationsDataStore.CreateNotificationsManager(); var settings = await notificationsManager.LoadSettings(); settings.Email.Enabled = true; diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs index 5a80ec2287..b6c5868923 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs @@ -20,9 +20,10 @@ public Task SetFailedMessageAsResolved() => public Task SaveChanges() => throw new NotImplementedException(); - public void Dispose() + public ValueTask DisposeAsync() { // Nothing to dispose yet GC.SuppressFinalize(this); + return ValueTask.CompletedTask; } } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs index 74dabd8071..4817701f34 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs @@ -10,9 +10,10 @@ public Task LoadSettings(TimeSpan? cacheTimeout = null) = public Task SaveChanges() => throw new NotImplementedException(); - public void Dispose() + public ValueTask DisposeAsync() { // Nothing to dispose yet GC.SuppressFinalize(this); + return ValueTask.CompletedTask; } } diff --git a/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs b/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs index 6f04ae065c..86138440e8 100644 --- a/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs @@ -8,6 +8,10 @@ abstract class AbstractSessionManager(IAsyncDocumentSession session) : IDataSess protected IAsyncDocumentSession Session { get; } = session; public Task SaveChanges() => Session.SaveChangesAsync(); - public void Dispose() => Session.Dispose(); + public ValueTask DisposeAsync() + { + Session.Dispose(); + return ValueTask.CompletedTask; + } } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs index 3a7907cd30..0c31cdc66e 100644 --- a/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs @@ -8,7 +8,7 @@ class NotificationsDataStoreTests : PersistenceTestBase [Test, CancelAfter(30_000)] public async Task LoadSettings_returns_defaults_when_no_settings_exist() { - using var manager = await NotificationsStore.CreateNotificationsManager(); + await using var manager = await NotificationsStore.CreateNotificationsManager(); var settings = await manager.LoadSettings(); @@ -30,7 +30,7 @@ public async Task LoadSettings_returns_defaults_when_no_settings_exist() [Test, CancelAfter(30_000)] public async Task SaveChanges_persists_email_settings_round_trip() { - using (var manager = await NotificationsStore.CreateNotificationsManager()) + await using (var manager = await NotificationsStore.CreateNotificationsManager()) { var settings = await manager.LoadSettings(); @@ -48,7 +48,7 @@ public async Task SaveChanges_persists_email_settings_round_trip() await CompleteDatabaseOperation(); - using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + await using var verifyManager = await NotificationsStore.CreateNotificationsManager(); var loaded = await verifyManager.LoadSettings(); using (Assert.EnterMultipleScope()) @@ -67,7 +67,7 @@ public async Task SaveChanges_persists_email_settings_round_trip() [Test, CancelAfter(30_000)] public async Task Toggling_enabled_is_persisted() { - using (var manager = await NotificationsStore.CreateNotificationsManager()) + await using (var manager = await NotificationsStore.CreateNotificationsManager()) { var settings = await manager.LoadSettings(); settings.Email.Enabled = true; @@ -76,7 +76,7 @@ public async Task Toggling_enabled_is_persisted() await CompleteDatabaseOperation(); - using (var manager = await NotificationsStore.CreateNotificationsManager()) + await using (var manager = await NotificationsStore.CreateNotificationsManager()) { var settings = await manager.LoadSettings(); Assert.That(settings.Email.Enabled, Is.True); @@ -87,7 +87,7 @@ public async Task Toggling_enabled_is_persisted() await CompleteDatabaseOperation(); - using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + await using var verifyManager = await NotificationsStore.CreateNotificationsManager(); var final = await verifyManager.LoadSettings(); Assert.That(final.Email.Enabled, Is.False); } @@ -95,7 +95,7 @@ public async Task Toggling_enabled_is_persisted() [Test, CancelAfter(30_000)] public async Task LoadSettings_returns_previously_saved_settings() { - using (var manager = await NotificationsStore.CreateNotificationsManager()) + await using (var manager = await NotificationsStore.CreateNotificationsManager()) { var settings = await manager.LoadSettings(); settings.Email.SmtpServer = "configured.server"; @@ -105,7 +105,7 @@ public async Task LoadSettings_returns_previously_saved_settings() await CompleteDatabaseOperation(); - using var manager2 = await NotificationsStore.CreateNotificationsManager(); + await using var manager2 = await NotificationsStore.CreateNotificationsManager(); var loaded = await manager2.LoadSettings(); using (Assert.EnterMultipleScope()) @@ -121,7 +121,7 @@ public async Task LoadSettings_returns_previously_saved_settings() [Test, CancelAfter(30_000)] public async Task Updating_individual_fields_preserves_others() { - using (var manager = await NotificationsStore.CreateNotificationsManager()) + await using (var manager = await NotificationsStore.CreateNotificationsManager()) { var settings = await manager.LoadSettings(); settings.Email.Enabled = true; @@ -137,7 +137,7 @@ public async Task Updating_individual_fields_preserves_others() await CompleteDatabaseOperation(); - using (var manager = await NotificationsStore.CreateNotificationsManager()) + await using (var manager = await NotificationsStore.CreateNotificationsManager()) { var settings = await manager.LoadSettings(); settings.Email.SmtpServer = "updated.smtp"; @@ -147,7 +147,7 @@ public async Task Updating_individual_fields_preserves_others() await CompleteDatabaseOperation(); - using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + await using var verifyManager = await NotificationsStore.CreateNotificationsManager(); var loaded = await verifyManager.LoadSettings(); using (Assert.EnterMultipleScope()) diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs index 1c571b413b..94eef1fa3d 100644 --- a/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs +++ b/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs @@ -76,7 +76,7 @@ public async Task Should_discard_edit_when_different_edit_already_exists() _ = await CreateAndStoreFailedMessage(failedMessageId); - using (var editFailedMessagesManager = await EditFailedMessagesStore.CreateEditFailedMessageManager()) + await using (var editFailedMessagesManager = await EditFailedMessagesStore.CreateEditFailedMessageManager()) { _ = await editFailedMessagesManager.GetFailedMessage(failedMessageId); await editFailedMessagesManager.SetCurrentEditingRequestId(previousEdit); @@ -88,7 +88,7 @@ public async Task Should_discard_edit_when_different_edit_already_exists() // Act await handler.Handle(message, new TestableMessageHandlerContext()); - using (var editFailedMessagesManagerAssert = await EditFailedMessagesStore.CreateEditFailedMessageManager()) + await using (var editFailedMessagesManagerAssert = await EditFailedMessagesStore.CreateEditFailedMessageManager()) { var failedMessage = await editFailedMessagesManagerAssert.GetFailedMessage(failedMessageId); var editId = await editFailedMessagesManagerAssert.GetCurrentEditingRequestId(failedMessageId); @@ -125,18 +125,16 @@ public async Task Should_dispatch_edited_message_when_first_edit() Assert.That(dispatchedMessage.Item1.Message.Headers["someKey"], Is.EqualTo("someValue")); } - using (var x = await EditFailedMessagesStore.CreateEditFailedMessageManager()) - { - var failedMessage2 = await x.GetFailedMessage(failedMessage.UniqueMessageId); - Assert.That(failedMessage2, Is.Not.Null, "Edited failed message"); + await using var x = await EditFailedMessagesStore.CreateEditFailedMessageManager(); + var failedMessage2 = await x.GetFailedMessage(failedMessage.UniqueMessageId); + Assert.That(failedMessage2, Is.Not.Null, "Edited failed message"); - var editId = await x.GetCurrentEditingRequestId(failedMessage2.UniqueMessageId); + var editId = await x.GetCurrentEditingRequestId(failedMessage2.UniqueMessageId); - using (Assert.EnterMultipleScope()) - { - Assert.That(failedMessage2.Status, Is.EqualTo(FailedMessageStatus.Resolved), "Failed message status"); - Assert.That(editId, Is.EqualTo(handlerContent.MessageId), "MessageId"); - } + using (Assert.EnterMultipleScope()) + { + Assert.That(failedMessage2.Status, Is.EqualTo(FailedMessageStatus.Resolved), "Failed message status"); + Assert.That(editId, Is.EqualTo(handlerContent.MessageId), "MessageId"); } } diff --git a/src/ServiceControl.Persistence/IDataSessionManager.cs b/src/ServiceControl.Persistence/IDataSessionManager.cs index fa6973ce03..b87bd91802 100644 --- a/src/ServiceControl.Persistence/IDataSessionManager.cs +++ b/src/ServiceControl.Persistence/IDataSessionManager.cs @@ -3,7 +3,7 @@ using System; using System.Threading.Tasks; - public interface IDataSessionManager : IDisposable + public interface IDataSessionManager : IAsyncDisposable { Task SaveChanges(); } diff --git a/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs b/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs index 76bd5f3056..bab51af05a 100644 --- a/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs +++ b/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs @@ -48,15 +48,13 @@ sealed class FakeEditFailedMessagesManager : IEditFailedMessagesManager { public string? CurrentEditingRequestId { get; set; } - public void Dispose() - { - } - public Task SaveChanges() => Task.CompletedTask; public Task GetFailedMessage(string failedMessageId) => Task.FromResult(null); public Task GetCurrentEditingRequestId(string failedMessageId) => Task.FromResult(CurrentEditingRequestId); public Task SetCurrentEditingRequestId(string editingMessageId) => Task.CompletedTask; public Task SetFailedMessageAsResolved() => Task.CompletedTask; + + public ValueTask DisposeAsync() => ValueTask.CompletedTask; } sealed class StubErrorMessageDataStore : IFailedMessageQueryDataStore, IEditFailedMessagesDataStore diff --git a/src/ServiceControl/Notifications/Api/NotificationsController.cs b/src/ServiceControl/Notifications/Api/NotificationsController.cs index f25c3433da..c12ae41447 100644 --- a/src/ServiceControl/Notifications/Api/NotificationsController.cs +++ b/src/ServiceControl/Notifications/Api/NotificationsController.cs @@ -19,7 +19,7 @@ public class NotificationsController(INotificationsDataStore store, Settings set [HttpGet] public async Task GetEmailNotificationsSettings() { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); return notificationsSettings.Email; @@ -30,7 +30,7 @@ public async Task GetEmailNotificationsSettings() [HttpPost] public async Task ToggleEmailNotifications(ToggleEmailNotifications request) { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); notificationsSettings.Email.Enabled = request.Enabled; @@ -45,7 +45,7 @@ public async Task ToggleEmailNotifications(ToggleEmailNotificatio [HttpPost] public async Task UpdateSettings(UpdateEmailNotificationsSettingsRequest request) { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); var emailSettings = notificationsSettings.Email; @@ -70,7 +70,7 @@ public async Task UpdateSettings(UpdateEmailNotificationsSettings [HttpPost] public async Task SendTestEmail() { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); try diff --git a/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs b/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs index 6c9c2c9550..55fd511d1b 100644 --- a/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs +++ b/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs @@ -16,7 +16,7 @@ public async Task Handle(SendEmailNotification message, IMessageHandlerContext c { NotificationsSettings notifications; - using (var manager = await store.CreateNotificationsManager()) + await using (var manager = await store.CreateNotificationsManager()) { notifications = await manager.LoadSettings(cacheTimeout); } diff --git a/src/ServiceControl/Recoverability/Editing/EditHandler.cs b/src/ServiceControl/Recoverability/Editing/EditHandler.cs index f0d3adbbcc..82a90b1fd7 100644 --- a/src/ServiceControl/Recoverability/Editing/EditHandler.cs +++ b/src/ServiceControl/Recoverability/Editing/EditHandler.cs @@ -24,7 +24,7 @@ public async Task Handle(EditAndSend message, IMessageHandlerContext context) { FailedMessage failedMessage; string editId; - using (var session = await store.CreateEditFailedMessageManager()) + await using (var session = await store.CreateEditFailedMessageManager()) { failedMessage = await session.GetFailedMessage(message.FailedMessageId); From caf1b047e20a9ab56f8f8bfc881b2039cfa04766 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Fri, 7 Aug 2026 10:56:00 +0800 Subject: [PATCH 4/5] Remove unused raven cache timeout override --- .../Implementation/NotificationsManager.cs | 2 +- .../Editing/NotificationsManager.cs | 6 +++--- src/ServiceControl.Persistence/INotificationsManager.cs | 3 +-- .../Notifications/Email/SendEmailNotificationHandler.cs | 3 +-- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs index 4817701f34..065a60b557 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs @@ -4,7 +4,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation; public class NotificationsManager : INotificationsManager { - public Task LoadSettings(TimeSpan? cacheTimeout = null) => + public Task LoadSettings() => throw new NotImplementedException(); public Task SaveChanges() => diff --git a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs index 9159e39b64..57e8d2b454 100644 --- a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs @@ -8,11 +8,11 @@ class NotificationsManager(IAsyncDocumentSession session) : AbstractSessionManager(session), INotificationsManager { const string SingleDocumentId = "NotificationsSettings/All"; - static readonly TimeSpan CacheTimeoutDefault = TimeSpan.FromMinutes(5); // Raven requires this to be at least 1 second + static readonly TimeSpan CacheTimeout = TimeSpan.FromMinutes(5); // Raven requires this to be at least 1 second - public async Task LoadSettings(TimeSpan? cacheTimeout = null) + public async Task LoadSettings() { - using var aggressivelyCacheFor = await Session.Advanced.DocumentStore.AggressivelyCacheForAsync(cacheTimeout ?? CacheTimeoutDefault); + using var aggressivelyCacheFor = await Session.Advanced.DocumentStore.AggressivelyCacheForAsync(CacheTimeout); var settings = await Session .LoadAsync(SingleDocumentId); diff --git a/src/ServiceControl.Persistence/INotificationsManager.cs b/src/ServiceControl.Persistence/INotificationsManager.cs index 6bdb21dd5b..673932d5ec 100644 --- a/src/ServiceControl.Persistence/INotificationsManager.cs +++ b/src/ServiceControl.Persistence/INotificationsManager.cs @@ -1,11 +1,10 @@ namespace ServiceControl.Persistence { - using System; using System.Threading.Tasks; using Notifications; public interface INotificationsManager : IDataSessionManager { - Task LoadSettings(TimeSpan? cacheTimeout = null); + Task LoadSettings(); } } \ No newline at end of file diff --git a/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs b/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs index 55fd511d1b..671491ed07 100644 --- a/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs +++ b/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs @@ -18,7 +18,7 @@ public async Task Handle(SendEmailNotification message, IMessageHandlerContext c await using (var manager = await store.CreateNotificationsManager()) { - notifications = await manager.LoadSettings(cacheTimeout); + notifications = await manager.LoadSettings(); } logger.LogInformation("Processing email notification. Subject: {Subject}, Body: {Body}", message.Subject, message.Body); @@ -85,7 +85,6 @@ public async Task Handle(SendEmailNotification message, IMessageHandlerContext c static readonly TimeSpan spinDelay = TimeSpan.FromSeconds(1); static readonly TimeSpan throttlingDelay = TimeSpan.FromSeconds(30); - static readonly TimeSpan cacheTimeout = TimeSpan.FromMinutes(5); public static RecoverabilityAction RecoverabilityPolicy(RecoverabilityConfig config, ErrorContext context) { From f2637f9989232d0695d88a5d5ee1f5126a120a79 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Fri, 7 Aug 2026 14:56:05 +0800 Subject: [PATCH 5/5] Use default instead of ValueTask.CompletedTask --- .../Implementation/EditFailedMessagesManager.cs | 7 +------ .../Implementation/NotificationsManager.cs | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs index b6c5868923..cb795e9b89 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs @@ -20,10 +20,5 @@ public Task SetFailedMessageAsResolved() => public Task SaveChanges() => throw new NotImplementedException(); - public ValueTask DisposeAsync() - { - // Nothing to dispose yet - GC.SuppressFinalize(this); - return ValueTask.CompletedTask; - } + public ValueTask DisposeAsync() => default; } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs index 065a60b557..b601155a89 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs @@ -10,10 +10,5 @@ public Task LoadSettings() => public Task SaveChanges() => throw new NotImplementedException(); - public ValueTask DisposeAsync() - { - // Nothing to dispose yet - GC.SuppressFinalize(this); - return ValueTask.CompletedTask; - } + public ValueTask DisposeAsync() => default; }