From 747836f0388b7672c51315702d3a3e397ecf79a7 Mon Sep 17 00:00:00 2001 From: John Simons Date: Fri, 14 Aug 2026 14:28:43 +1000 Subject: [PATCH] Address cancellation and DateTimeOffset analyzer debt in persistence and tests Refines cancellation logic in persistence loops to ensure OperationCanceledException is correctly surfaced rather than swallowed. Resolves DateTimeOffset debt in acceptance and persistence tests and uses pragmas to document where cancellation tokens cannot be added due to external extension points. Adds new test coverage to ensure long-running operations in every persistence dialect respect cancellation. --- .../.editorconfig | 4 - .../.editorconfig | 8 -- .../DispatchRawMessages.cs | 4 + .../EndpointTemplates/DefaultServerBase.cs | 4 + .../IConfigureEndpointTestExecution.cs | 2 + .../ConfigureEndpointInMemoryPersistence.cs | 2 + .../ConfigureEndpointLearningTransport.cs | 2 + .../ScenarioWithEndpointBehaviorExtensions.cs | 4 + .../Sequence.cs | 4 + .../.editorconfig | 10 -- .../ExternalIntegrationAcceptanceTest.cs | 2 +- ...age_groups_are_sorted_by_a_web_api_call.cs | 2 +- ..._errors_with_same_uniqueid_are_imported.cs | 8 +- ...ing_failed_message_with_missing_headers.cs | 2 + .../.editorconfig | 4 - .../.editorconfig | 4 - .../RavenFailedAuditStorage.cs | 5 +- .../.editorconfig | 11 -- .../IndexSetupTests.cs | 2 + .../.editorconfig | 7 +- .../AuditCountingTests.cs | 4 +- .../AuditTests.cs | 10 +- .../FailedAuditStorageTests.cs | 62 +++++++++ .../.editorconfig | 4 - .../Auditing/ImportFailedAudits.cs | 4 +- src/ServiceControl.Config/.editorconfig | 9 -- .../Commands/SelectPathCommand.cs | 2 + .../Framework/Command.cs | 2 + .../Commands/AwaitableAbstractCommand.cs | 2 + .../Commands/AwaitableDelegateCommand.cs | 4 + .../Framework/Commands/IAsyncCommand.cs | 2 + .../.editorconfig | 6 - .../FailedErrorImportDataStore.cs | 13 +- .../FailedErrorImportDataStore.cs | 8 +- .../.editorconfig | 4 - .../.editorconfig | 4 - .../.editorconfig | 4 - .../.editorconfig | 4 - .../.editorconfig | 6 - .../EFCore/FailedErrorImportTests.cs | 30 ----- .../FailedErrorImportCancellationTests.cs | 104 +++++++++++++++ .../ArchiveCancellationTests.cs | 123 ++++++++++++++++++ src/ServiceControl.UnitTests/.editorconfig | 5 - 43 files changed, 357 insertions(+), 150 deletions(-) create mode 100644 src/ServiceControl.Persistence.Tests/FailedErrorImportCancellationTests.cs create mode 100644 src/ServiceControl.Persistence.Tests/Recoverability/ArchiveCancellationTests.cs diff --git a/src/Particular.LicensingComponent.UnitTests/.editorconfig b/src/Particular.LicensingComponent.UnitTests/.editorconfig index 0a6ae3dffe..ca5ad8bd2e 100644 --- a/src/Particular.LicensingComponent.UnitTests/.editorconfig +++ b/src/Particular.LicensingComponent.UnitTests/.editorconfig @@ -2,8 +2,4 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Cancellation analyzer debt. These fire because Particular.Analyzers is no longer pinned to 0.9.0. -# They are scheduled work, not accepted exceptions: remove a line once this project has no -# violations of that rule left, and never add a rule back to this list. dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.AcceptanceTesting/.editorconfig b/src/ServiceControl.AcceptanceTesting/.editorconfig index cc18ee5f5b..196d6e44a8 100644 --- a/src/ServiceControl.AcceptanceTesting/.editorconfig +++ b/src/ServiceControl.AcceptanceTesting/.editorconfig @@ -2,11 +2,3 @@ # Justification: Usage is from test projects dotnet_diagnostic.CA2007.severity = none - -# These types sit on NServiceBus.AcceptanceTesting's extension points, which drive them without a -# CancellationToken: IEndpointSetupTemplate, IComponentBehavior/ComponentRunner, and the scenario -# Done/When callbacks whose delegate shapes the framework fixes. A token added here could only ever -# be CancellationToken.None. Everything else in this project takes and forwards one. -[{EndpointTemplates/*.cs,InfrastructureConfig/*.cs,ScenarioWithEndpointBehaviorExtensions.cs,Sequence.cs,DispatchRawMessages.cs}] -dotnet_diagnostic.PS0013.severity = none -dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.AcceptanceTesting/DispatchRawMessages.cs b/src/ServiceControl.AcceptanceTesting/DispatchRawMessages.cs index 28e6d4f13d..1eb779c1c0 100644 --- a/src/ServiceControl.AcceptanceTesting/DispatchRawMessages.cs +++ b/src/ServiceControl.AcceptanceTesting/DispatchRawMessages.cs @@ -9,6 +9,8 @@ using NServiceBus.Features; using NServiceBus.Transport; +#pragma warning disable PS0013 +#pragma warning disable PS0018 public abstract class DispatchRawMessages : Feature where TContext : ScenarioContext { @@ -79,4 +81,6 @@ protected override Task OnStop(IMessageSession session, CancellationToken cancel ScenarioContext scenarioContext; } } +#pragma warning restore PS0018 +#pragma warning restore PS0013 } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTesting/EndpointTemplates/DefaultServerBase.cs b/src/ServiceControl.AcceptanceTesting/EndpointTemplates/DefaultServerBase.cs index a045a88345..1bf50cb8f0 100644 --- a/src/ServiceControl.AcceptanceTesting/EndpointTemplates/DefaultServerBase.cs +++ b/src/ServiceControl.AcceptanceTesting/EndpointTemplates/DefaultServerBase.cs @@ -16,7 +16,11 @@ protected DefaultServerBase() : this(new ConfigureEndpointLearningTransport()) { } +#pragma warning disable PS0013 +#pragma warning disable PS0018 public virtual async Task GetConfiguration(RunDescriptor runDescriptor, EndpointCustomizationConfiguration endpointCustomizations, Func configurationBuilderCustomization) +#pragma warning restore PS0018 +#pragma warning restore PS0013 { var endpointConfiguration = new EndpointConfiguration(endpointCustomizations.EndpointName); diff --git a/src/ServiceControl.AcceptanceTesting/EndpointTemplates/IConfigureEndpointTestExecution.cs b/src/ServiceControl.AcceptanceTesting/EndpointTemplates/IConfigureEndpointTestExecution.cs index 62fb5f8200..057438879b 100644 --- a/src/ServiceControl.AcceptanceTesting/EndpointTemplates/IConfigureEndpointTestExecution.cs +++ b/src/ServiceControl.AcceptanceTesting/EndpointTemplates/IConfigureEndpointTestExecution.cs @@ -8,6 +8,7 @@ /// Provide a mechanism in acceptance tests for transports and persistences /// to configure an endpoint for a test and then clean up afterwards. /// +#pragma warning disable PS0018 public interface IConfigureEndpointTestExecution { /// @@ -32,4 +33,5 @@ public interface IConfigureEndpointTestExecution /// An async Task. Task Cleanup(); } +#pragma warning restore PS0018 } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointInMemoryPersistence.cs b/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointInMemoryPersistence.cs index 5371e3a13a..6c7c4ac999 100644 --- a/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointInMemoryPersistence.cs +++ b/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointInMemoryPersistence.cs @@ -5,6 +5,7 @@ using NServiceBus; using NServiceBus.AcceptanceTesting.Support; +#pragma warning disable PS0018 public class ConfigureEndpointInMemoryPersistence : IConfigureEndpointTestExecution { public Task Configure(string endpointName, EndpointConfiguration configuration, RunSettings settings, PublisherMetadata publisherMetadata) @@ -17,4 +18,5 @@ public Task Cleanup() => // Nothing required for in-memory persistence Task.CompletedTask; } +#pragma warning restore PS0018 } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointLearningTransport.cs b/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointLearningTransport.cs index 4f637aa023..1879947431 100644 --- a/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointLearningTransport.cs +++ b/src/ServiceControl.AcceptanceTesting/InfrastructureConfig/ConfigureEndpointLearningTransport.cs @@ -7,6 +7,7 @@ using NServiceBus.AcceptanceTesting.Support; using NUnit.Framework; +#pragma warning disable PS0018 public class ConfigureEndpointLearningTransport : ITransportIntegration { public ConfigureEndpointLearningTransport() @@ -48,4 +49,5 @@ public Task Cleanup() public string TypeName => "LearningTransport"; } +#pragma warning restore PS0018 } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTesting/ScenarioWithEndpointBehaviorExtensions.cs b/src/ServiceControl.AcceptanceTesting/ScenarioWithEndpointBehaviorExtensions.cs index 8f02d3ea2e..55bc34b8df 100644 --- a/src/ServiceControl.AcceptanceTesting/ScenarioWithEndpointBehaviorExtensions.cs +++ b/src/ServiceControl.AcceptanceTesting/ScenarioWithEndpointBehaviorExtensions.cs @@ -8,6 +8,8 @@ using NServiceBus.AcceptanceTesting; using NServiceBus.AcceptanceTesting.Support; +#pragma warning disable PS0013 +#pragma warning disable PS0018 public static class ScenarioWithEndpointBehaviorExtensions { public static IScenarioWithEndpointBehavior Done(this IScenarioWithEndpointBehavior endpointBehavior, Func> func) where TContext : ScenarioContext @@ -158,4 +160,6 @@ public override async Task Stop(CancellationToken cancellationToken = default) CancellationTokenSource tokenSource; } } +#pragma warning restore PS0018 +#pragma warning restore PS0013 } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTesting/Sequence.cs b/src/ServiceControl.AcceptanceTesting/Sequence.cs index b7b4505658..7b28d12334 100644 --- a/src/ServiceControl.AcceptanceTesting/Sequence.cs +++ b/src/ServiceControl.AcceptanceTesting/Sequence.cs @@ -4,6 +4,8 @@ using System.Collections.Generic; using System.Threading.Tasks; +#pragma warning disable PS0013 +#pragma warning disable PS0018 class Sequence where TContext : ISequenceContext { @@ -55,4 +57,6 @@ public async Task Continue(TContext context) List>> steps = []; List stepNames = []; } +#pragma warning restore PS0018 +#pragma warning restore PS0013 } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests/.editorconfig b/src/ServiceControl.AcceptanceTests/.editorconfig index a32084aea3..da44eb13fb 100644 --- a/src/ServiceControl.AcceptanceTests/.editorconfig +++ b/src/ServiceControl.AcceptanceTests/.editorconfig @@ -2,15 +2,5 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Timezone debt: these DateTime values are Kind=Unspecified or Local, so the implicit cast to -# DateTimeOffset uses the build agent's offset. Each needs individual review before removing. -dotnet_diagnostic.PS0022.severity = none - -# Justification: these are the test helpers the NServiceBus.AcceptanceTesting scenario API drives. -# It calls Done/When callbacks and IComponentBehavior/ComponentRunner without a CancellationToken, -# so a token added here could only ever be CancellationToken.None at every call site, which tests -# nothing. Tests that genuinely exercise cancellation use [Test, CancelAfter(...)] with the token -# NUnit injects, and forward that. Helpers reachable with a real token do take one. dotnet_diagnostic.PS0013.severity = none dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/ExternalIntegration/ExternalIntegrationAcceptanceTest.cs b/src/ServiceControl.AcceptanceTests/Recoverability/ExternalIntegration/ExternalIntegrationAcceptanceTest.cs index 2e0bf56a04..de147c7301 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/ExternalIntegration/ExternalIntegrationAcceptanceTest.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/ExternalIntegration/ExternalIntegrationAcceptanceTest.cs @@ -43,7 +43,7 @@ protected override TransportOperations CreateMessage(Context context) OutgoingMessage CreateTransportMessage(string messageId) { - var date = new DateTime(2015, 9, 20, 0, 0, 0); + var date = new DateTimeOffset(2015, 9, 20, 0, 0, 0, TimeSpan.Zero); var msg = new OutgoingMessage(messageId, new Dictionary { {Headers.MessageId, messageId}, diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_message_groups_are_sorted_by_a_web_api_call.cs b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_message_groups_are_sorted_by_a_web_api_call.cs index ed87bf8420..732d455cdb 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_message_groups_are_sorted_by_a_web_api_call.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_message_groups_are_sorted_by_a_web_api_call.cs @@ -108,7 +108,7 @@ protected override TransportOperations CreateMessage(MyContext context) OutgoingMessage CreateTransportMessage(int i) { - var date = new DateTime(2015, 9 + i, 20 + i, 0, 0, 0); + var date = new DateTimeOffset(2015, 9 + i, 20 + i, 0, 0, 0, TimeSpan.Zero); var messageId = $"{i}{MessageId}"; var msg = new OutgoingMessage(messageId, new Dictionary { diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs index af9b323433..c84d19b878 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs @@ -59,7 +59,7 @@ public async Task The_import_should_deduplicate_on_TimeOfFailure() using (Assert.EnterMultipleScope()) { Assert.That(attempts, Has.Count.EqualTo(1)); - Assert.That(attempts[^1].AttemptedAt, Is.EqualTo(context.FailureTime)); + Assert.That(attempts[^1].AttemptedAt, Is.EqualTo(context.FailureTime.UtcDateTime)); } } @@ -88,12 +88,12 @@ protected override TransportOperations CreateMessage(MyContext context) { var messageId = Guid.NewGuid().ToString(); context.UniqueId = DeterministicGuid.MakeId(messageId, "Error.SourceEndpoint").ToString(); - context.FailureTime = new DateTime(2020, 09, 05, 13, 20, 00, 0, DateTimeKind.Utc); + context.FailureTime = new DateTimeOffset(2020, 09, 05, 13, 20, 00, 0, TimeSpan.Zero); return new TransportOperations([.. GetMessages(context.UniqueId, context.FailureTime)]); } - IEnumerable GetMessages(string uniqueId, DateTime failureTime) + IEnumerable GetMessages(string uniqueId, DateTimeOffset failureTime) { for (var i = 0; i < NumberOfDuplicates; i++) { @@ -125,7 +125,7 @@ class MyContext : ScenarioContext { public string UniqueId { get; set; } - public DateTime FailureTime { get; set; } + public DateTimeOffset FailureTime { get; set; } public int IngestedCount => receivedMessages.Count; diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_ingesting_failed_message_with_missing_headers.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_ingesting_failed_message_with_missing_headers.cs index 6196d97536..0230387ca6 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_ingesting_failed_message_with_missing_headers.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_ingesting_failed_message_with_missing_headers.cs @@ -73,7 +73,9 @@ public async Task TimeSent_should_not_be_casted() var context = await Define(c => { c.AddMinimalRequiredHeaders(); +#pragma warning disable PS0022 c.Headers.Add(Headers.TimeSent, DateTimeOffsetHelper.ToWireFormattedString(sentTime)); +#pragma warning restore PS0022 }) .WithEndpoint() .Done(async c => await TryGetFailureFromApi(c)) diff --git a/src/ServiceControl.Audit.AcceptanceTests.RavenDB/.editorconfig b/src/ServiceControl.Audit.AcceptanceTests.RavenDB/.editorconfig index 0a6ae3dffe..ca5ad8bd2e 100644 --- a/src/ServiceControl.Audit.AcceptanceTests.RavenDB/.editorconfig +++ b/src/ServiceControl.Audit.AcceptanceTests.RavenDB/.editorconfig @@ -2,8 +2,4 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Cancellation analyzer debt. These fire because Particular.Analyzers is no longer pinned to 0.9.0. -# They are scheduled work, not accepted exceptions: remove a line once this project has no -# violations of that rule left, and never add a rule back to this list. dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.Audit.AcceptanceTests/.editorconfig b/src/ServiceControl.Audit.AcceptanceTests/.editorconfig index 0a6ae3dffe..ca5ad8bd2e 100644 --- a/src/ServiceControl.Audit.AcceptanceTests/.editorconfig +++ b/src/ServiceControl.Audit.AcceptanceTests/.editorconfig @@ -2,8 +2,4 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Cancellation analyzer debt. These fire because Particular.Analyzers is no longer pinned to 0.9.0. -# They are scheduled work, not accepted exceptions: remove a line once this project has no -# violations of that rule left, and never add a rule back to this list. dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.Audit.Persistence.RavenDB/RavenFailedAuditStorage.cs b/src/ServiceControl.Audit.Persistence.RavenDB/RavenFailedAuditStorage.cs index d99e9b6fc5..183907a9ed 100644 --- a/src/ServiceControl.Audit.Persistence.RavenDB/RavenFailedAuditStorage.cs +++ b/src/ServiceControl.Audit.Persistence.RavenDB/RavenFailedAuditStorage.cs @@ -30,9 +30,10 @@ public async Task ProcessFailedMessages( try { stream = await session.Advanced.StreamAsync(query, cancellationToken); - while (!cancellationToken.IsCancellationRequested && - await stream.MoveNextAsync()) + while (await stream.MoveNextAsync()) { + cancellationToken.ThrowIfCancellationRequested(); + FailedTransportMessage transportMessage = stream.Current.Document.Message; var localSession = session; diff --git a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/.editorconfig b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/.editorconfig index 174320291d..ca5ad8bd2e 100644 --- a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/.editorconfig +++ b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/.editorconfig @@ -2,15 +2,4 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Cancellation analyzer debt. These fire because Particular.Analyzers is no longer pinned to 0.9.0. -# They are scheduled work, not accepted exceptions: remove a line once this project has no -# violations of that rule left, and never add a rule back to this list. -# The remaining sites are private test helpers that already close over the fixture's -# TestTimeoutCancellationToken, so they need a parameter added rather than a token found. dotnet_diagnostic.PS0018.severity = none - -# Justification: the polling loop swallows OperationCanceledException on purpose, because querying an -# index at the moment it updates throws one. That is not cancellation, so there is no token to filter on. -[IndexSetupTests.cs] -dotnet_diagnostic.PS0020.severity = none diff --git a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/IndexSetupTests.cs b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/IndexSetupTests.cs index b31c3288a9..9ec30788d6 100644 --- a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/IndexSetupTests.cs +++ b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/IndexSetupTests.cs @@ -122,10 +122,12 @@ async Task WaitForIndexDefinitionUpdate(IndexStats oldStats) return newStats; } } +#pragma warning disable PS0020 catch (OperationCanceledException) { // keep going since we can get this if we query right when the update happens } +#pragma warning restore PS0020 await Task.Delay(TimeSpan.FromMilliseconds(100), TestTimeoutCancellationToken); } diff --git a/src/ServiceControl.Audit.Persistence.Tests/.editorconfig b/src/ServiceControl.Audit.Persistence.Tests/.editorconfig index d9eedc8bed..c11a245166 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/.editorconfig +++ b/src/ServiceControl.Audit.Persistence.Tests/.editorconfig @@ -2,12 +2,7 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none -dotnet_diagnostic.PS0004.severity = none # A parameter of type CancellationToken on a private delegate or method should be required -dotnet_diagnostic.PS0018.severity = none # Add a CancellationToken parameter +dotnet_diagnostic.PS0018.severity = none # Justification: Tests don't support cancellation and don't need to forward IMessageHandlerContext.CancellationToken dotnet_diagnostic.NSB0002.severity = suggestion - -# Timezone debt: these DateTime values are Kind=Unspecified or Local, so the implicit cast to -# DateTimeOffset uses the build agent's offset. Each needs individual review before removing. -dotnet_diagnostic.PS0022.severity = none diff --git a/src/ServiceControl.Audit.Persistence.Tests/AuditCountingTests.cs b/src/ServiceControl.Audit.Persistence.Tests/AuditCountingTests.cs index e067bd33b9..d8d02540fc 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/AuditCountingTests.cs +++ b/src/ServiceControl.Audit.Persistence.Tests/AuditCountingTests.cs @@ -15,7 +15,7 @@ class AuditCountingTests : PersistenceTestFixture [Test] public async Task ShouldCountAuditedMessages() { - var today = DateTime.UtcNow.Date; + var today = new DateTimeOffset(DateTime.UtcNow.Date, TimeSpan.Zero); var yesterday = today.AddDays(-1); var weekBefore = yesterday.AddDays(-7); @@ -67,7 +67,7 @@ string ScrubDates(string input) }, ScrubDates); } - ProcessedMessage MakeMessage(string processingEndpoint, DateTime processedAt, bool systemMessage) + ProcessedMessage MakeMessage(string processingEndpoint, DateTimeOffset processedAt, bool systemMessage) { var messageId = Guid.NewGuid().ToString(); var messageType = "MyMessageType"; diff --git a/src/ServiceControl.Audit.Persistence.Tests/AuditTests.cs b/src/ServiceControl.Audit.Persistence.Tests/AuditTests.cs index 0aa2983656..96d2fc6b37 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/AuditTests.cs +++ b/src/ServiceControl.Audit.Persistence.Tests/AuditTests.cs @@ -169,7 +169,7 @@ public async Task Deduplicates_messages_in_same_batch() var unitOfWork = await StartAuditUnitOfWork(1); var messageId = "duplicatedId"; var processingEndpoint = "endpoint"; - var processingStarted = DateTime.UtcNow; + var processingStarted = DateTimeOffset.UtcNow; var processedMessage = MakeMessage(messageId: messageId, processingEndpoint: processingEndpoint, processingStarted: processingStarted); var duplicatedMessage = MakeMessage(messageId: messageId, processingEndpoint: processingEndpoint, processingStarted: processingStarted); @@ -190,7 +190,7 @@ public async Task Deduplicates_messages_in_different_batches() { var messageId = "duplicatedId"; var processingEndpoint = "endpoint"; - var processingStarted = DateTime.UtcNow; + var processingStarted = DateTimeOffset.UtcNow; var processedMessage = MakeMessage(messageId: messageId, processingEndpoint: processingEndpoint, processingStarted: processingStarted); var unitOfWork1 = await StartAuditUnitOfWork(1); @@ -215,7 +215,7 @@ public async Task Does_not_deduplicate_with_different_processing_started_header( var unitOfWork = await StartAuditUnitOfWork(1); var messageId = "duplicatedId"; var processingEndpoint = "endpoint"; - var processingStarted = DateTime.UtcNow; + var processingStarted = DateTimeOffset.UtcNow; var duplicatedProcessingStarted = processingStarted.AddSeconds(5); var processedMessage = MakeMessage(messageId: messageId, processingEndpoint: processingEndpoint, processingStarted: processingStarted); @@ -255,7 +255,7 @@ ProcessedMessage MakeMessage( MessageIntent intent = MessageIntent.Send, string conversationId = null, string processingEndpoint = null, - DateTime? processingStarted = null, + DateTimeOffset? processingStarted = null, string messageType = null ) { @@ -284,7 +284,7 @@ ProcessedMessage MakeMessage( { Headers.ProcessingEndpoint, processingEndpoint }, { Headers.MessageIntent, intent.ToString() }, { Headers.ConversationId, conversationId }, - { Headers.ProcessingStarted, DateTimeOffsetHelper.ToWireFormattedString(processingStarted ?? DateTime.UtcNow) }, + { Headers.ProcessingStarted, DateTimeOffsetHelper.ToWireFormattedString(processingStarted ?? DateTimeOffset.UtcNow) }, { Headers.EnclosedMessageTypes, messageType } }; diff --git a/src/ServiceControl.Audit.Persistence.Tests/FailedAuditStorageTests.cs b/src/ServiceControl.Audit.Persistence.Tests/FailedAuditStorageTests.cs index 6e3a3b2b7c..c450ff6e25 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/FailedAuditStorageTests.cs +++ b/src/ServiceControl.Audit.Persistence.Tests/FailedAuditStorageTests.cs @@ -1,5 +1,6 @@ namespace ServiceControl.Audit.Persistence.Tests { + using System; using System.Threading; using System.Threading.Tasks; using NUnit.Framework; @@ -45,5 +46,66 @@ await FailedAuditStorage.ProcessFailedMessages(async (transportMessage, markComp Assert.That(numFailures, Is.EqualTo(0)); } } + + [Test] + public async Task Processing_throws_when_the_token_is_already_cancelled() + { + await FailedAuditStorage.SaveFailedAuditImport(new FailedAuditImport()); + + await configuration.CompleteDBOperation(); + + var processed = 0; + + Assert.That( + async () => await FailedAuditStorage.ProcessFailedMessages( + async (transportMessage, markComplete, token) => + { + processed++; + await markComplete(token); + }, + new CancellationToken(true)), + Throws.InstanceOf()); + + await configuration.CompleteDBOperation(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(processed, Is.EqualTo(0)); + Assert.That(await FailedAuditStorage.GetFailedAuditsCount(), Is.EqualTo(1), "the pending import must survive"); + } + } + + [Test] + public async Task Processing_stops_at_the_message_that_cancels() + { + await FailedAuditStorage.SaveFailedAuditImport(new FailedAuditImport()); + await FailedAuditStorage.SaveFailedAuditImport(new FailedAuditImport()); + await FailedAuditStorage.SaveFailedAuditImport(new FailedAuditImport()); + + await configuration.CompleteDBOperation(); + + using var source = new CancellationTokenSource(); + var processed = 0; + + Assert.That( + async () => await FailedAuditStorage.ProcessFailedMessages( + async (transportMessage, markComplete, token) => + { + processed++; + await markComplete(token); + await source.CancelAsync(); + }, + source.Token), + Throws.InstanceOf(), + "an interrupted replay must not return as though it had completed"); + + await configuration.CompleteDBOperation(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(processed, Is.EqualTo(1)); + Assert.That(await FailedAuditStorage.GetFailedAuditsCount(), Is.EqualTo(3), "an interrupted replay must not commit its deletes"); + } + } } } \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/.editorconfig b/src/ServiceControl.Audit.UnitTests/.editorconfig index 0a6ae3dffe..ca5ad8bd2e 100644 --- a/src/ServiceControl.Audit.UnitTests/.editorconfig +++ b/src/ServiceControl.Audit.UnitTests/.editorconfig @@ -2,8 +2,4 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Cancellation analyzer debt. These fire because Particular.Analyzers is no longer pinned to 0.9.0. -# They are scheduled work, not accepted exceptions: remove a line once this project has no -# violations of that rule left, and never add a rule back to this list. dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.Audit/Auditing/ImportFailedAudits.cs b/src/ServiceControl.Audit/Auditing/ImportFailedAudits.cs index c8aaf835ca..eda805d9cf 100644 --- a/src/ServiceControl.Audit/Auditing/ImportFailedAudits.cs +++ b/src/ServiceControl.Audit/Auditing/ImportFailedAudits.cs @@ -55,9 +55,9 @@ await failedAuditStore.ProcessFailedMessages( succeeded++; logger.LogDebug("Successfully re-imported failed audit message {MessageId}", transportMessage.Id); } - catch (OperationCanceledException e) when (cancellationToken.IsCancellationRequested) + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - logger.LogInformation(e, "Cancelled re-importing failed audit message {MessageId}", transportMessage.Id); + throw; } catch (Exception e) { diff --git a/src/ServiceControl.Config/.editorconfig b/src/ServiceControl.Config/.editorconfig index 6181205b06..9e40cf6683 100644 --- a/src/ServiceControl.Config/.editorconfig +++ b/src/ServiceControl.Config/.editorconfig @@ -5,12 +5,3 @@ dotnet_diagnostic.CA2007.severity = none # Use auto property - should be fixed in an independent PR dotnet_diagnostic.IDE0032.severity = suggestion - -# System.Windows.Input.ICommand.Execute(object) returns void and supplies no CancellationToken, -# so the async command chain built on it has no token to accept or forward. A token added here -# could only ever be CancellationToken.None. Command bodies themselves do take tokens: they are -# wired through ReactiveCommand.CreateFromTask(Func), which supplies a -# real one. -[{Framework/Commands/*.cs,Framework/Command.cs,Commands/SelectPathCommand.cs}] -dotnet_diagnostic.PS0013.severity = none -dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.Config/Commands/SelectPathCommand.cs b/src/ServiceControl.Config/Commands/SelectPathCommand.cs index 7d6f50c0ac..9fe11df6d9 100644 --- a/src/ServiceControl.Config/Commands/SelectPathCommand.cs +++ b/src/ServiceControl.Config/Commands/SelectPathCommand.cs @@ -12,7 +12,9 @@ class AwaitableSelectPathCommand : AwaitableAbstractCommand readonly CommonOpenFileDialog dlg; readonly Func setPath; +#pragma warning disable PS0013 public AwaitableSelectPathCommand(Func setPath, string title = null, bool isFolderPicker = false, IEnumerable filters = null, string defaultPath = "") +#pragma warning restore PS0013 { dlg = new CommonOpenFileDialog { diff --git a/src/ServiceControl.Config/Framework/Command.cs b/src/ServiceControl.Config/Framework/Command.cs index 4ca134039e..41730690e1 100644 --- a/src/ServiceControl.Config/Framework/Command.cs +++ b/src/ServiceControl.Config/Framework/Command.cs @@ -26,6 +26,7 @@ public static ICommand Create(Action executeMethod, Func canEx return new DelegateCommand(executeMethod, canExecuteMethod); } +#pragma warning disable PS0013 public static IAsyncCommand Create(Func executeMethod) { return new AwaitableDelegateCommand(_ => executeMethod()); @@ -45,5 +46,6 @@ public static IAsyncCommand Create(Func executeMethod, Func(executeMethod, canExecuteMethod); } +#pragma warning restore PS0013 } } \ No newline at end of file diff --git a/src/ServiceControl.Config/Framework/Commands/AwaitableAbstractCommand.cs b/src/ServiceControl.Config/Framework/Commands/AwaitableAbstractCommand.cs index 5f7c025895..51f111b701 100644 --- a/src/ServiceControl.Config/Framework/Commands/AwaitableAbstractCommand.cs +++ b/src/ServiceControl.Config/Framework/Commands/AwaitableAbstractCommand.cs @@ -31,7 +31,9 @@ void System.Windows.Input.ICommand.Execute(object parameter) ((ICommand)this).Execute((T)parameter); } +#pragma warning disable PS0018 public abstract Task ExecuteAsync(T obj); +#pragma warning restore PS0018 protected virtual void OnExecuting() { diff --git a/src/ServiceControl.Config/Framework/Commands/AwaitableDelegateCommand.cs b/src/ServiceControl.Config/Framework/Commands/AwaitableDelegateCommand.cs index fceff09df6..80732daf24 100644 --- a/src/ServiceControl.Config/Framework/Commands/AwaitableDelegateCommand.cs +++ b/src/ServiceControl.Config/Framework/Commands/AwaitableDelegateCommand.cs @@ -3,6 +3,8 @@ using System; using System.Threading.Tasks; +#pragma warning disable PS0013 +#pragma warning disable PS0018 class AwaitableDelegateCommand : AwaitableDelegateCommand, IAsyncCommand { public AwaitableDelegateCommand(Func executeMethod, Func canExecuteMethod = null) : base(executeMethod, canExecuteMethod) @@ -39,4 +41,6 @@ public async Task ExecuteAsync(T parameter) readonly Func executeMethod; } +#pragma warning restore PS0018 +#pragma warning restore PS0013 } \ No newline at end of file diff --git a/src/ServiceControl.Config/Framework/Commands/IAsyncCommand.cs b/src/ServiceControl.Config/Framework/Commands/IAsyncCommand.cs index ef8b4a4ed9..61b70dd715 100644 --- a/src/ServiceControl.Config/Framework/Commands/IAsyncCommand.cs +++ b/src/ServiceControl.Config/Framework/Commands/IAsyncCommand.cs @@ -4,10 +4,12 @@ public interface IAsyncCommand : IAsyncCommand; +#pragma warning disable PS0018 public interface IAsyncCommand : IRaiseCanExecuteChanged, System.Windows.Input.ICommand { Task ExecuteAsync(T obj); bool CanExecute(T obj); } +#pragma warning restore PS0018 } \ No newline at end of file diff --git a/src/ServiceControl.MultiInstance.AcceptanceTests/.editorconfig b/src/ServiceControl.MultiInstance.AcceptanceTests/.editorconfig index 3eca2918ad..da44eb13fb 100644 --- a/src/ServiceControl.MultiInstance.AcceptanceTests/.editorconfig +++ b/src/ServiceControl.MultiInstance.AcceptanceTests/.editorconfig @@ -2,11 +2,5 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Justification: these are the test helpers the NServiceBus.AcceptanceTesting scenario API drives. -# It calls Done/When callbacks and IComponentBehavior/ComponentRunner without a CancellationToken, -# so a token added here could only ever be CancellationToken.None at every call site, which tests -# nothing. Tests that genuinely exercise cancellation use [Test, CancelAfter(...)] with the token -# NUnit injects, and forward that. Helpers reachable with a real token do take one. dotnet_diagnostic.PS0013.severity = none dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/FailedErrorImportDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/FailedErrorImportDataStore.cs index bfdfbbc09f..de424f3d74 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/FailedErrorImportDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/FailedErrorImportDataStore.cs @@ -74,8 +74,10 @@ public async Task ProcessFailedErrorImports(Func(); await using var stream = await session.Advanced.StreamAsync(query, cancellationToken); - while (!cancellationToken.IsCancellationRequested && await stream.MoveNextAsync()) + while (await stream.MoveNextAsync()) { + cancellationToken.ThrowIfCancellationRequested(); + var transportMessage = stream.Current.Document.Message; try { @@ -44,9 +46,9 @@ public async Task ProcessFailedErrorImports(Func callback shapes, which is why it is not done -# yet. Remove a line once this project has no violations of that rule left. dotnet_diagnostic.PS0013.severity = none dotnet_diagnostic.PS0018.severity = none diff --git a/src/ServiceControl.Persistence.Tests/EFCore/FailedErrorImportTests.cs b/src/ServiceControl.Persistence.Tests/EFCore/FailedErrorImportTests.cs index 59d4143502..259aa61e0e 100644 --- a/src/ServiceControl.Persistence.Tests/EFCore/FailedErrorImportTests.cs +++ b/src/ServiceControl.Persistence.Tests/EFCore/FailedErrorImportTests.cs @@ -4,13 +4,10 @@ namespace ServiceControl.Persistence.Tests; using System.Collections.Generic; using System.Linq; using System.Text; -using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.DependencyInjection; using NServiceBus; using NUnit.Framework; using ServiceControl.Operations; -using ServiceControl.Persistence; using ServiceControl.Persistence.EFCore.Entities; using ServiceControl.Persistence.Infrastructure; @@ -164,33 +161,6 @@ public async Task Replays_across_multiple_pages() } } - [Test] - public async Task Stops_replaying_when_cancelled() - { - await Store( - Import("native-1", BaseTime), - Import("native-2", BaseTime.AddSeconds(1)), - Import("native-3", BaseTime.AddSeconds(2))); - - using var cts = new CancellationTokenSource(); - var replayed = new List(); - - await FailedImportStore.ProcessFailedErrorImports( - (message, _) => - { - replayed.Add(message.Id); - cts.Cancel(); - return Task.CompletedTask; - }, - cts.Token); - - using (Assert.EnterMultipleScope()) - { - Assert.That(replayed, Has.Count.EqualTo(1)); - Assert.That(await FailedImportStore.QueryContainsFailedImports(), Is.True); - } - } - [Test] public async Task Retention_sweep_does_not_touch_failed_imports() { diff --git a/src/ServiceControl.Persistence.Tests/FailedErrorImportCancellationTests.cs b/src/ServiceControl.Persistence.Tests/FailedErrorImportCancellationTests.cs new file mode 100644 index 0000000000..d70a0b301e --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/FailedErrorImportCancellationTests.cs @@ -0,0 +1,104 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Collections.Generic; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using NServiceBus; +using NUnit.Framework; +using ServiceControl.Operations; + +[TestFixture] +class FailedErrorImportCancellationTests : PersistenceTestBase +{ + [Test, CancelAfter(60_000)] + public async Task Replay_stops_at_the_message_that_cancels() + { + await StoreImport("native-1"); + await StoreImport("native-2"); + await StoreImport("native-3"); + await CompleteDatabaseOperation(); + + using var source = new CancellationTokenSource(); + var replayed = new List(); + + Assert.That( + async () => await FailedImportStore.ProcessFailedErrorImports( + (message, _) => + { + replayed.Add(message.Id); + source.Cancel(); + return Task.CompletedTask; + }, + source.Token), + Throws.InstanceOf(), + "an interrupted replay must not return as though it had completed"); + + await CompleteDatabaseOperation(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(replayed, Has.Count.EqualTo(1), "the loop should stop once the token is cancelled"); + Assert.That( + await FailedImportStore.QueryContainsFailedImports(), + Is.True, + "the imports that were never replayed must still be pending"); + } + } + + [Test, CancelAfter(60_000)] + public async Task Replay_throws_when_the_token_is_already_cancelled() + { + await StoreImport("native-1"); + await CompleteDatabaseOperation(); + + var replayed = new List(); + + Assert.That( + async () => await FailedImportStore.ProcessFailedErrorImports( + (message, _) => + { + replayed.Add(message.Id); + return Task.CompletedTask; + }, + new CancellationToken(true)), + Throws.InstanceOf()); + + await CompleteDatabaseOperation(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(replayed, Is.Empty, "nothing should be replayed under a cancelled token"); + Assert.That( + await FailedImportStore.QueryContainsFailedImports(), + Is.True, + "the pending import must survive so a later run can replay it"); + } + } + + Task StoreImport(string nativeId) + { + var headers = new Dictionary + { + [Headers.MessageId] = nativeId, + [Headers.ProcessingEndpoint] = "Sales", + ["NServiceBus.ExceptionInfo.ExceptionType"] = "System.InvalidOperationException", + ["NServiceBus.ExceptionInfo.Message"] = "Something went wrong" + }; + + var failure = new FailedErrorImport + { + Id = FailedErrorImport.DeriveKey(headers, nativeId).ToString(), + Message = new FailedTransportMessage + { + Id = nativeId, + Headers = headers, + Body = Encoding.UTF8.GetBytes("1") + }, + ExceptionInfo = "Import failed" + }; + + return FailedImportStore.StoreFailedErrorImport(failure); + } +} diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/ArchiveCancellationTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/ArchiveCancellationTests.cs new file mode 100644 index 0000000000..2fdfea6432 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/Recoverability/ArchiveCancellationTests.cs @@ -0,0 +1,123 @@ +namespace ServiceControl.Persistence.Tests.Recoverability; + +using System; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; +using NUnit.Framework; +using ServiceControl.Infrastructure.DomainEvents; +using ServiceControl.MessageFailures; +using ServiceControl.Recoverability; + +[TestFixture] +class ArchiveCancellationTests : PersistenceTestBase +{ + const string Classifier = "Exception Type and Stack Trace"; + + static readonly DateTime Noon = new(2026, 7, 22, 12, 0, 0, DateTimeKind.Utc); + + readonly CancelOnFirstBatch events = new(); + + public ArchiveCancellationTests() => + RegisterServices = services => services.AddSingleton(events); + + [Test, CancelAfter(60_000)] + public async Task Archiving_completes_when_a_cancelled_run_is_retried() + { + var group = NewGroup(); + await Insert(InGroup(group), InGroup(group), InGroup(group)); + + Assert.That( + async () => await ArchiveMessages.ArchiveAllInGroup(group.Id, cancellationToken: events.Token), + Throws.InstanceOf(), + "cancelling mid-operation should surface rather than be swallowed"); + + await AssertNoUnresolvedMessagesIn(group); + + await ArchiveMessages.ArchiveAllInGroup(group.Id); + + await AssertNoUnresolvedMessagesIn(group); + } + + [Test, CancelAfter(60_000)] + public async Task Cancelled_archiving_leaves_the_batch_it_had_already_committed() + { + var group = NewGroup(); + await Insert(InGroup(group), InGroup(group)); + + Assert.That( + async () => await ArchiveMessages.ArchiveAllInGroup(group.Id, cancellationToken: events.Token), + Throws.InstanceOf()); + + Assert.That(events.BatchesArchived, Is.EqualTo(1), "exactly one batch should have been archived before cancelling"); + + await AssertNoUnresolvedMessagesIn(group); + } + + async Task AssertNoUnresolvedMessagesIn(FailedMessage.FailureGroup group) + { + await CompleteDatabaseOperation(); + + var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null); + + Assert.That( + groups.Select(view => view.Id), + Does.Not.Contain(group.Id), + "every message in the group should have left the Unresolved status"); + } + + static FailedMessage.FailureGroup NewGroup() => + new() { Id = Guid.NewGuid().ToString(), Title = "OrderPlaced", Type = Classifier }; + + static IngestedFailure InGroup(FailedMessage.FailureGroup group) => + new() + { + Groups = [group], + AttemptedAt = Noon, + TimeOfFailure = Noon, + TimeSent = Noon.AddMinutes(-1) + }; + + async Task Insert(params IngestedFailure[] failures) + { + var messages = failures.Select(failure => failure.ToFailedMessage()).ToArray(); + + foreach (var message in messages) + { + message.Id = PersistenceTestsContext.GenerateFailedMessageRecordId(message.UniqueMessageId); + } + + await PersistenceTestsContext.InsertFailedMessages(messages); + await CompleteDatabaseOperation(); + } + + /// + /// Cancels as soon as a batch has been archived. Both persisters raise + /// only after the batch and its progress have been + /// committed, so this interrupts the operation at a batch boundary rather than mid-write. + /// Cancellation is honoured on the way in, the way the real implementation does, so the token is + /// observed on the next event the archiver raises. + /// + sealed class CancelOnFirstBatch : IDomainEvents + { + readonly CancellationTokenSource source = new(); + + public CancellationToken Token => source.Token; + + public int BatchesArchived { get; private set; } + + public Task Raise(T domainEvent, CancellationToken cancellationToken = default) where T : IDomainEvent + { + cancellationToken.ThrowIfCancellationRequested(); + + if (domainEvent is FailedMessageGroupBatchArchived) + { + BatchesArchived++; + source.Cancel(); + } + + return Task.CompletedTask; + } + } +} diff --git a/src/ServiceControl.UnitTests/.editorconfig b/src/ServiceControl.UnitTests/.editorconfig index 062de7d0db..ca5ad8bd2e 100644 --- a/src/ServiceControl.UnitTests/.editorconfig +++ b/src/ServiceControl.UnitTests/.editorconfig @@ -2,9 +2,4 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none - -# Cancellation analyzer debt. These fire because Particular.Analyzers is no longer pinned to 0.9.0. -# They are scheduled work, not accepted exceptions: remove a line once this project has no -# violations of that rule left, and never add a rule back to this list. -dotnet_diagnostic.PS0013.severity = none dotnet_diagnostic.PS0018.severity = none