From 1337425a9861cba4ec0d32dd622ddad2c870cd03 Mon Sep 17 00:00:00 2001 From: John Simons Date: Thu, 20 Aug 2026 10:24:02 +1000 Subject: [PATCH 1/2] Add acceptance tests for core ServicePulse API routes Adds coverage for the license, heartbeat stats, and failed message query routes as part of the Phase 4 review plan. Includes guidance in documentation on handling eventual consistency in search index assertions and modifies the settings infrastructure to allow overriding the heartbeat grace period in tests. --- docs/acceptance-test-review-plan.md | 32 +++- docs/writing-acceptance-tests.md | 32 ++++ .../When_the_license_is_requested.cs | 60 +++++++ .../When_heartbeat_stats_are_requested.cs | 66 ++++++++ .../When_failed_messages_are_queried.cs | 151 ++++++++++++++++++ .../Infrastructure/Settings/Settings.cs | 30 ++-- 6 files changed, 349 insertions(+), 22 deletions(-) create mode 100644 src/ServiceControl.AcceptanceTests/Licensing/When_the_license_is_requested.cs create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/When_heartbeat_stats_are_requested.cs create mode 100644 src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs diff --git a/docs/acceptance-test-review-plan.md b/docs/acceptance-test-review-plan.md index 009d664691..2e5b0a878c 100644 --- a/docs/acceptance-test-review-plan.md +++ b/docs/acceptance-test-review-plan.md @@ -12,7 +12,7 @@ That is the shape of the problem: these failures are silent. A test that asserts Phases 1, 2 and 3 are done. Phase 4 has closed the licensing block, 8 routes of the 40, on both of the branches those routes take. -What is left: 32 routes to cover, starting with the three nav-gating ones. +What is left: 27 routes needing a test, in seven groups, starting with notifications. Two more are recorded below as deliberately untested, and six are covered in MultiInstance only and need a home decision rather than a new test. ## Scope @@ -20,6 +20,8 @@ In scope: the functional areas of `ServiceControl.AcceptanceTests`, being `Recov Out of scope: `Security/*` (25 files across ForwardedHeaders, OpenIdConnect, Cors and Https), which carries a different risk model and deserves its own pass. Also out of scope: the audit instance, monitoring instance, and multi-instance suites, except where they already cover a route the primary suite misses. +One consequence of that boundary is worth stating, because this review already tripped over it once. `GET /api/my/routes` is the manifest every gated ServicePulse capability is resolved against, so it carries the risk of a nav item disappearing for all 85 routes at once. It lives in `Security/OpenIdConnect/When_my_routes_are_requested` and is therefore out of scope here, which is why the route list below originally recorded that risk as uncovered. Anything reasoning about gating has to read that file even though this review does not touch it. + Three artefacts anchor the work: - `src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` inventories 77 routes. It is **not** the whole primary-instance surface: the approval test behind it scans only `typeof(Program).Assembly` and `typeof(MyRoutesController).Assembly`. @@ -70,15 +72,31 @@ Harmless on its own, but it makes tests read as though they cover more than they The groups are sized to be one PR each and ordered by what breaks in ServicePulse if the route regresses. Each entry names the ServicePulse consumer where there is one, because that is what the test should assert: the contract the UI relies on, not a 200. -### Nav-gating routes +### Routes behind the main ServicePulse pages (done) + +These sit behind three of the most-used pages in ServicePulse. If one regresses, the page breaks on arrival. + +- [x] `GET /api/heartbeats/stats`: the Heartbeats page (`viewHeartbeats`), by `Monitoring/When_heartbeat_stats_are_requested` +- [x] `GET /api/messages2`: the audit messages page (`viewAuditMessages`), by `WebApi/When_failed_messages_are_queried` +- [x] `GET /api/license`: the Licence page (`viewLicense`), by `Licensing/When_the_license_is_requested` + +This tier was originally called "nav-gating", on the grounds that a regression here costs ServicePulse a whole section of its navigation. That is not how the gating works, and the distinction changes what these tests are for. + +ServicePulse decides which nav items to render with `canCall(ApiRoutes.viewHeartbeats)`, which reduces to `!shouldGate || store.routes.has(normalizeRouteKey(method, path))`. `store.routes` is a manifest fetched once from `GET /api/my/routes`, so a nav item appears when the route is *advertised in the manifest*, never because the route was called and answered. A route that throws on every request keeps its nav item and breaks the page behind it. Only removing the route, or changing the permission that filters it, takes the nav item away. The gating is fail-open besides: `shouldGate` requires `authEnabled && isAuthenticated && loaded`, so an install without OpenID Connect renders every nav item regardless. + +The manifest itself is covered, by `ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_my_routes_are_requested`, which asserts it is the projection of what the server actually enforces. See the note in [Scope](#scope): that file is out of scope for this review, which is how the risk came to be listed as uncovered here. + +So the three are not a journey and should not be written as one. Their arrangements have nothing in common: `license` needs none at all, `heartbeats/stats` needs endpoints registered in `IEndpointInstanceMonitoring` with heartbeats flowing, and `messages2` needs ingested failed messages. Nothing one call returns feeds the next, which is the property that earned the licensing block a single scenario. Three separate tests, filed in the area each belongs to. + +Writing them turned up three things worth keeping. + +`GET /api/license` answers **400** without a `clientName`. `LicenseController` is a `#nullable enable` file, so the non-nullable `string clientName` is inferred as required and the request is rejected before the action runs. The parameter exists only to label a marketing link, so the route that backs the Licence page rejects a caller who does not want one. ServicePulse always sends it, alongside `refresh=true`, which is why nothing has noticed. `When_the_license_is_requested` now covers both the answer ServicePulse gets and the 400, so the behaviour is pinned rather than latent. -If one of these regresses, ServicePulse loses a whole section of its navigation and nothing in the suite notices. +`GET /api/messages2` builds its paging from `page_size` alone. It is the only paged route that does not bind `PagingInfo` through `PagingInfoModelBinder`: it takes a bare `int pageSize` and hands it to `new PagingInfo(pageSize: pageSize)`, which takes the value as given. Omitting `page_size` therefore asks for a page of nothing and gets an empty list, where every other route falls back to 50. It also reads `page_size` rather than `per_page` and ignores `page` entirely, both of which match how ServicePulse calls it (`auditClient.ts` sends `page_size` and windows by date instead of paging), so the divergence is deliberate. Only the missing floor is a rough edge. -- [ ] `GET /api/heartbeats/stats`: gates the Heartbeats nav item (`viewHeartbeats`) -- [ ] `GET /api/messages2`: gates the audit messages nav item (`viewAuditMessages`) -- [ ] `GET /api/license`: gates the Licence nav item (`viewLicense`) +Search is not visible at the same moment on every persister. On SQL Server the full text index is populated asynchronously, so a `q=` search answers nothing for a while after the plain list already returns the message. A test that waits for ingestion on the unsearched list and then searches passes on RavenDB and PostgreSQL and fails on SQL Server, which is how this one first failed. The rule is now written down in [Writing acceptance tests](writing-acceptance-tests.md): wait on the path you are about to assert on. -`GET /api/licensing/report/available` gates the Throughput nav item and belongs to this tier too, but it is written with the rest of the licensing block below. `GET /api/connection` gates the Connections nav item and is covered in MultiInstance only, so it is a home decision rather than a new test. +`GET /api/licensing/report/available` backs the Throughput page and belongs to this tier too, but it is written with the rest of the licensing block below. `GET /api/connection` backs the Connections page and is covered in MultiInstance only, so it is a home decision rather than a new test. ### Licensing and throughput (done) diff --git a/docs/writing-acceptance-tests.md b/docs/writing-acceptance-tests.md index 913ece8b8c..f6e7f9e81a 100644 --- a/docs/writing-acceptance-tests.md +++ b/docs/writing-acceptance-tests.md @@ -109,6 +109,38 @@ Registering the double correctly is only half the job. If the assertion would al The suite runs against every persister. An assertion about how RavenDB happens to store or trim something says nothing about ServiceControl's behaviour. It also ends up on another persister's exclusion list, where it looks like a missing feature instead of a test that asks for too much. Assert what every persister has to do to be correct. +### The wait that does not cover what the assertion reads + +Wait on the path you are about to assert on, not on a cheaper one that looks equivalent. + +Persisters do not make a write visible everywhere at the same moment, and search is where they differ most. PostgreSQL indexes a `to_tsvector` expression with GIN, which is maintained inside the writing transaction, so a committed message is searchable at once. SQL Server's full text index is populated by a background process (`CHANGE_TRACKING AUTO`), so a `q=` search answers nothing for a while after the plain list already returns the same message. A test that waits for ingestion by listing messages and then searches therefore passes on PostgreSQL and RavenDB, and fails on SQL Server: + +```csharp +// Wrong: the list does not go through the search index, so the search below can run against +// an index that has not caught up. +.Do("Wait for the failures to be ingested", async _ => (await Query(string.Empty)).Count == 3) +.Do("Search", async _ => matches = await Query($"q={SearchTerm}")) + +// Right: the wait runs the same query the assertion depends on. +.Do("Wait for the search index to catch up", async _ => +{ + matches = await Query($"q={SearchTerm}"); + return matches.Count == 2; +}) +``` + +This cuts across [the assertion that could not have failed](#the-assertion-that-could-not-have-failed), so be deliberate about where the wait stops and the assertion starts. + +Wait for the loosest condition that makes the query answerable, not for the answer you expect. `Count == 2` never advances when a search matches three, so the interesting regression, matching too much, is reported as a 90 second timeout rather than as the assertion that would have named the extra row. `Count >= 2` advances as soon as there is enough to judge and lets the assertion do the judging, which turns that same regression into a failure in a few seconds reading `Extra (1): DeliveryFailed`. + +Whatever the wait cannot avoid holding, record on the scenario context, because the runner prints the context when a scenario does not finish while a `TimeoutException` on its own says only that 90 seconds passed: + +```csharp +ctx.SearchMatched = string.Join(", ", TypesIn(matchingTerm)); +``` + +A stalled run then reads as the step it stopped on, from `Advancing from X to Y`, plus what that step kept seeing, which separates an index that never populated from one that matched the wrong thing. + ### The setup that nothing reads Headers put into a dictionary nothing reads, constants nothing compares against, fixtures left behind after an assertion was deleted. Each one is harmless by itself. Together they make a test look like it covers more than it does, which is how everything above gets through review. diff --git a/src/ServiceControl.AcceptanceTests/Licensing/When_the_license_is_requested.cs b/src/ServiceControl.AcceptanceTests/Licensing/When_the_license_is_requested.cs new file mode 100644 index 0000000000..a56527625d --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Licensing/When_the_license_is_requested.cs @@ -0,0 +1,60 @@ +namespace ServiceControl.AcceptanceTests.Licensing +{ + using System.Net; + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceControl.Licensing; + + class When_the_license_is_requested : AcceptanceTest + { + [Test] + public async Task Should_report_the_instance_and_where_to_extend_the_trial() + { + LicenseInfo license = null; + + await Define() + .Done(async _ => + { + var result = await this.TryGet($"/api/license?refresh=true&clientName={ClientName}"); + license = result.Item; + return result.HasResult; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(license.InstanceName, Is.EqualTo(Settings.InstanceName), + "ServicePulse labels the license page with the instance it is talking to"); + + Assert.That(license.LicenseExtensionUrl, Does.StartWith("https://particular.net/extend-your-trial"), + "With no MassTransit connector reporting in, the license page offers the trial extension rather than the connector link"); + + Assert.That(license.LicenseExtensionUrl, Does.Contain($"p={ClientName}"), + "The link has to carry the caller through, or Particular cannot tell which product the request came from"); + } + } + + [Test] + public async Task Should_reject_a_request_that_names_no_client() + { + HttpStatusCode status = default; + + await Define() + .Done(async _ => + { + using var response = await this.GetRaw("/api/license"); + status = response.StatusCode; + return true; + }) + .Run(); + + Assert.That(status, Is.EqualTo(HttpStatusCode.BadRequest)); + } + + const string ClientName = "servicepulse"; + + class Context : ScenarioContext; + } +} diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/When_heartbeat_stats_are_requested.cs b/src/ServiceControl.AcceptanceTests/Monitoring/When_heartbeat_stats_are_requested.cs new file mode 100644 index 0000000000..52dee69f51 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/When_heartbeat_stats_are_requested.cs @@ -0,0 +1,66 @@ +namespace ServiceControl.AcceptanceTests.Monitoring +{ + using System; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceBus.Management.Infrastructure.Settings; + + class When_heartbeat_stats_are_requested : AcceptanceTest + { + [Test] + public async Task Should_count_a_heartbeating_endpoint_as_active() + { + HeartbeatStats stats = null; + + await Define() + .WithEndpoint() + .Done(async _ => + { + var result = await this.TryGet("/api/heartbeats/stats", found => found.Active > 0); + stats = result.Item; + return result.HasResult; + }) + .Run(); + + Assert.That(stats.Failing, Is.Zero, + "An endpoint still sending heartbeats must not also be counted against the failing tile"); + } + + [Test] + public async Task Should_count_an_endpoint_past_its_grace_period_as_failing() + { + HeartbeatStats stats = null; + + // Short enough that the endpoint below is past it by the time the monitor first looks, + // however recently it last reported. + SetSettings = settings => settings.HeartbeatGracePeriod = TimeSpan.FromMilliseconds(1); + + await Define() + .WithEndpoint() + .Done(async _ => + { + var result = await this.TryGet("/api/heartbeats/stats", found => found.Failing > 0); + stats = result.Item; + return result.HasResult; + }) + .Run(); + + Assert.That(stats.Active, Is.Zero, + "An endpoint counted as failing must have left the active tile, or the two tiles double count it"); + } + + record HeartbeatStats(int Active, int Failing); + + class Context : ScenarioContext; + + public class HeartbeatingEndpoint : EndpointConfigurationBuilder + { + public HeartbeatingEndpoint() => + EndpointSetup(c => c.SendHeartbeatTo(Settings.DEFAULT_INSTANCE_NAME)); + } + } +} diff --git a/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs b/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs new file mode 100644 index 0000000000..7b62cde46f --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs @@ -0,0 +1,151 @@ +namespace ServiceControl.AcceptanceTests.WebApi +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using CompositeViews.Messages; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NServiceBus.AcceptanceTesting.Customization; + using NUnit.Framework; + using Conventions = NServiceBus.AcceptanceTesting.Customization.Conventions; + + class When_failed_messages_are_queried : AcceptanceTest + { + [Test] + public async Task Should_filter_by_endpoint_and_by_search_term() + { + List forBilling = null; + List matchingTerm = null; + List forBillingMatchingTerm = null; + + await Define() + .WithEndpoint(b => b.When(async (bus, _) => + { + await bus.Send(new InvoiceFailed { Description = $"Invoice for the {SearchTerm} run" }); + await bus.Send(new LabelFailed { Description = $"Label for the {SearchTerm} run" }); + await bus.Send(new DeliveryFailed { Description = "Delivery booked for the same day" }); + })) + .WithEndpoint(b => b.DoNotFailOnErrorMessages()) + .WithEndpoint(b => b.DoNotFailOnErrorMessages()) + .Do("Wait for all three failures to be ingested", async _ => + (await Query(string.Empty)).Count == 3) + .Do("Wait for the search index to catch up", async ctx => + { + matchingTerm = await Query($"q={SearchTerm}"); + + ctx.SearchMatched = string.Join(", ", TypesIn(matchingTerm)); + + return matchingTerm.Count >= 2; + }) + .Do("Query the list the way ServicePulse filters it", async _ => + { + forBilling = await Query($"endpoint_name={BillingEndpoint}"); + forBillingMatchingTerm = await Query($"endpoint_name={BillingEndpoint}&q={SearchTerm}"); + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(TypesIn(forBilling), Is.EquivalentTo(new[] { NameOf() }), + "endpoint_name has to drop the other endpoint's failures, not just keep this endpoint's"); + + Assert.That(TypesIn(matchingTerm), Is.EquivalentTo(new[] { NameOf(), NameOf() }), + $"q has to match the bodies carrying '{SearchTerm}' across endpoints, and drop the one without it"); + + Assert.That(TypesIn(forBillingMatchingTerm), Is.EquivalentTo(new[] { NameOf() }), + "Supplying both narrows to the intersection rather than applying whichever filter is read last"); + + Assert.That(ReceiversIn(forBillingMatchingTerm), Is.EquivalentTo(new[] { BillingEndpoint })); + } + } + + async Task> Query(string filter) + { + var result = await this.TryGetMany($"/api/messages2?page_size=50&{filter}"); + + return result.Items; + } + + static IEnumerable TypesIn(IEnumerable messages) => + messages.Select(message => message.MessageType); + + static IEnumerable ReceiversIn(IEnumerable messages) => + messages.Select(message => message.ReceivingEndpoint.Name).Distinct(); + + static string NameOf() => typeof(T).FullName; + + const string SearchTerm = "overnight"; + + static string BillingEndpoint => Conventions.EndpointNamingConvention(typeof(Billing)); + + class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + + public string SearchMatched { get; set; } + } + + public class Sender : EndpointConfigurationBuilder + { + public Sender() => + EndpointSetup(c => + { + var routing = c.ConfigureRouting(); + routing.RouteToEndpoint(typeof(InvoiceFailed), typeof(Billing)); + routing.RouteToEndpoint(typeof(LabelFailed), typeof(Shipping)); + routing.RouteToEndpoint(typeof(DeliveryFailed), typeof(Shipping)); + }); + } + + public class Billing : EndpointConfigurationBuilder + { + public Billing() => EndpointSetup(c => c.NoRetries()); + + [Handler] + public class InvoiceFailedHandler : IHandleMessages + { + public Task Handle(InvoiceFailed message, IMessageHandlerContext context) => + throw new Exception("Simulated exception"); + } + } + + public class Shipping : EndpointConfigurationBuilder + { + public Shipping() => EndpointSetup(c => c.NoRetries()); + + [Handler] + public class LabelFailedHandler : IHandleMessages + { + public Task Handle(LabelFailed message, IMessageHandlerContext context) => + throw new Exception("Simulated exception"); + } + + [Handler] + public class DeliveryFailedHandler : IHandleMessages + { + public Task Handle(DeliveryFailed message, IMessageHandlerContext context) => + throw new Exception("Simulated exception"); + } + } + + public class InvoiceFailed : ICommand + { + public string Description { get; set; } + } + + public class LabelFailed : ICommand + { + public string Description { get; set; } + } + + public class DeliveryFailed : ICommand + { + public string Description { get; set; } + } + } +} diff --git a/src/ServiceControl/Infrastructure/Settings/Settings.cs b/src/ServiceControl/Infrastructure/Settings/Settings.cs index 3850d65b52..8455370966 100644 --- a/src/ServiceControl/Infrastructure/Settings/Settings.cs +++ b/src/ServiceControl/Infrastructure/Settings/Settings.cs @@ -79,6 +79,7 @@ public Settings( NotificationsFilter = SettingsReader.Read(SettingsRootNamespace, "NotificationsFilter"); RemoteInstances = GetRemoteInstances().ToArray(); TimeToRestartErrorIngestionAfterFailure = GetTimeToRestartErrorIngestionAfterFailure(); + HeartbeatGracePeriod = GetHeartbeatGracePeriod(); DisableExternalIntegrationsPublishing = SettingsReader.Read(SettingsRootNamespace, "DisableExternalIntegrationsPublishing", false); TrackInstancesInitialValue = SettingsReader.Read(SettingsRootNamespace, "TrackInstancesInitialValue", true); ShutdownTimeout = SettingsReader.Read(SettingsRootNamespace, "ShutdownTimeout", ShutdownTimeout); @@ -175,21 +176,7 @@ public string InstanceId public string Hostname { get; private set; } public string VirtualDirectory => SettingsReader.Read(SettingsRootNamespace, "VirtualDirectory", string.Empty); - public TimeSpan HeartbeatGracePeriod - { - get - { - try - { - return TimeSpan.Parse(SettingsReader.Read(SettingsRootNamespace, "HeartbeatGracePeriod", "00:00:40")); - } - catch (Exception ex) - { - logger.LogError(ex, "HeartbeatGracePeriod settings invalid. Defaulting HeartbeatGracePeriod to '00:00:40'"); - return TimeSpan.FromSeconds(40); - } - } - } + public TimeSpan HeartbeatGracePeriod { get; set; } public string TransportType { get; set; } public string PersistenceType { get; private set; } @@ -369,6 +356,19 @@ TimeSpan GetErrorRetentionPeriod() return result; } + TimeSpan GetHeartbeatGracePeriod() + { + try + { + return TimeSpan.Parse(SettingsReader.Read(SettingsRootNamespace, "HeartbeatGracePeriod", "00:00:40")); + } + catch (Exception ex) + { + logger.LogError(ex, "HeartbeatGracePeriod settings invalid. Defaulting HeartbeatGracePeriod to '00:00:40'"); + return TimeSpan.FromSeconds(40); + } + } + TimeSpan GetTimeToRestartErrorIngestionAfterFailure() { string message; From 6ca2f678fc1f16ba41c08224039269c74948f7a4 Mon Sep 17 00:00:00 2001 From: John Simons Date: Thu, 20 Aug 2026 16:51:54 +1000 Subject: [PATCH 2/2] Add Phase 4 journey-based acceptance tests for recoverability and monitoring Implements the first five journeys of the Phase 4 review plan, covering failed message triage, batch archival and restoration, pending retry resolution, and notification configuration. These tests shift from a per-route model to scenario-based journeys that simulate real operator workflows. This change also fixes a defect in the endpoint settings API where a missing default row could cause UI failures and renames an incorrectly titled pending retry test to reflect its actual behavior. --- docs/acceptance-test-review-plan.md | 272 ------------------ ...hen_a_failing_custom_check_is_dismissed.cs | 83 ++++++ ...When_email_notifications_are_configured.cs | 223 ++++++++++++++ .../When_endpoint_tracking_is_configured.cs | 98 +++++++ ...failing_endpoint_is_triaged_and_retried.cs | 229 +++++++++++++++ ...pending_retry_is_resolved_by_timeframe.cs} | 8 +- .../When_deleted_messages_are_restored.cs | 208 ++++++++++++++ ...n_pending_retries_are_resolved_by_queue.cs | 170 +++++++++++ .../When_failed_messages_are_queried.cs | 16 ++ .../When_the_configuration_page_is_read.cs | 105 +++++++ .../When_the_edit_and_retry_flag_is_read.cs | 53 ++++ ...it_counts_for_an_endpoint_are_requested.cs | 90 ++++++ .../Messages/GetMessagesController.cs | 4 +- .../Web/EndpointsSettingsController.cs | 11 +- 14 files changed, 1284 insertions(+), 286 deletions(-) delete mode 100644 docs/acceptance-test-review-plan.md create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/When_endpoint_tracking_is_configured.cs create mode 100644 src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_failing_endpoint_is_triaged_and_retried.cs rename src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/{When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs => When_a_pending_retry_is_resolved_by_timeframe.cs} (91%) create mode 100644 src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_deleted_messages_are_restored.cs create mode 100644 src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_pending_retries_are_resolved_by_queue.cs create mode 100644 src/ServiceControl.AcceptanceTests/WebApi/When_the_configuration_page_is_read.cs create mode 100644 src/ServiceControl.AcceptanceTests/WebApi/When_the_edit_and_retry_flag_is_read.cs create mode 100644 src/ServiceControl.MultiInstance.AcceptanceTests/Auditing/When_audit_counts_for_an_endpoint_are_requested.cs diff --git a/docs/acceptance-test-review-plan.md b/docs/acceptance-test-review-plan.md deleted file mode 100644 index 2e5b0a878c..0000000000 --- a/docs/acceptance-test-review-plan.md +++ /dev/null @@ -1,272 +0,0 @@ -# Primary instance acceptance test review - -A review of the primary instance's acceptance tests for assertion correctness, prioritised by what ServicePulse actually calls. - -## Why this is worth doing - -Two tests in this suite were recently found to be testing nothing at all. Both registered a test double with `AddSingleton()` while the code under test injects `IEnumerable`, so the double was never resolved and never ran. Both compiled, both passed, and both had passed for years. - -That is the shape of the problem: these failures are silent. A test that asserts nothing and a test that asserts something unfalsifiable both look identical to CI. The purpose of this review is to find the rest of them before the EF persistence work starts leaning on this suite as its safety net. - -## Where this has got to - -Phases 1, 2 and 3 are done. Phase 4 has closed the licensing block, 8 routes of the 40, on both of the branches those routes take. - -What is left: 27 routes needing a test, in seven groups, starting with notifications. Two more are recorded below as deliberately untested, and six are covered in MultiInstance only and need a home decision rather than a new test. - -## Scope - -In scope: the functional areas of `ServiceControl.AcceptanceTests`, being `Recoverability`, `Monitoring`, `EventLogs` and `WebApi`. That is 71 `When_*.cs` files. - -Out of scope: `Security/*` (25 files across ForwardedHeaders, OpenIdConnect, Cors and Https), which carries a different risk model and deserves its own pass. Also out of scope: the audit instance, monitoring instance, and multi-instance suites, except where they already cover a route the primary suite misses. - -One consequence of that boundary is worth stating, because this review already tripped over it once. `GET /api/my/routes` is the manifest every gated ServicePulse capability is resolved against, so it carries the risk of a nav item disappearing for all 85 routes at once. It lives in `Security/OpenIdConnect/When_my_routes_are_requested` and is therefore out of scope here, which is why the route list below originally recorded that risk as uncovered. Anything reasoning about gating has to read that file even though this review does not touch it. - -Three artefacts anchor the work: - -- `src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` inventories 77 routes. It is **not** the whole primary-instance surface: the approval test behind it scans only `typeof(Program).Assembly` and `typeof(MyRoutesController).Assembly`. -- `src/Particular.LicensingComponent.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` inventories the other 8, served under the `api/licensing` prefix, via a separate approval test scanning `typeof(ThroughputCollector).Assembly`. Anything reasoning about "the API surface" has to read both files or it will silently miss the throughput and licensing endpoints. -- `src/composables/apiRoutes.ts` in ServicePulse maps each gated UI capability to the route behind it and names the ServiceControl controller for each. It describes itself as the only place coupling ServicePulse to ServiceControl's route surface. - -That makes 85 routes in total. - -## Defect patterns - -Each of these was found in the current suite. The value of naming them is that each becomes a repeatable check to run over all 71 files, rather than a one-off fix. - -### 1. Test double registered against the wrong service type (silent) - -The double is registered as its concrete type, so the collection the production code injects never contains it. The test still passes, having exercised none of the behaviour it names. Found three times: `CounterEnricher` in `When_errors_with_same_uniqueid_are_imported`, `FailOnceEnricher` in `When_single_message_fails_in_batch`, and `CriticalErrorCustomCheck` in `When_a_critical_error_is_triggered`, which phase 2 turned up. All three are fixed. - -**Detect:** read how the collaborator takes its dependency before registering anything, since a service resolved as a collection is added to rather than replaced. Phase 2 audited all seven registrations in the suite and settled on writing the practice down in [Writing acceptance tests](writing-acceptance-tests.md) rather than building a check, because the fault is a test that passes without its own setup taking effect and a convention test would catch one shape of that. - -### 2. No assertion, failure surfaces only as a timeout (diagnostics) - -Seven files contain no `Assert` at all. Five use the `Do("step", …)` sequence helper, which logs `Advancing from X to Y` on each transition, so a regression there is diagnosable from the console output. That is a deliberate and acceptable pattern. The other two gated on a single `Done` predicate and reported a regression as a bare timeout with nothing to read. - -**Detect:** files with a `[Test]` and zero `Assert.` occurrences, minus those using the `Sequence` helper. Outside `ExternalIntegration` this is now clear: eight files have no assertion, seven of them driven by steps, and `ErrorImportPerformanceTests` records its count on the context, which the runner prints when a scenario does not finish. - -### 3. Assertion restates the condition the scenario already waited on (unfalsifiable) - -`When_a_invalid_id_is_sent_to_retry` ended with `Assert.That(context.Done, Is.True)` after `.Done(ctx => ctx.Done)`. That assertion could not fail: if the flag were false the scenario would have timed out first. The real subject of the test, that posting a retry for a non-existent id does not break subsequent batches, was never asserted, and the response of the invalid POST was never inspected. - -**Detect:** a mechanical grep finds candidates of the shape `Assert.That(context.Flag, Is.True)`. Nine were confirmed and fixed across the suite. The two that remain were read and are sound, because a flag set by a message handler while the scenario waits for something else can fail: `When_failed_message_searched_by_body_content`, whose `Done` returns true either way, and `When_single_message_fails_in_batch`, whose new assertion is falsifiable and was proven so. The grep is a starting point, not a verdict. - -### 4. Assertions coupled to one persister's internals (portability) - -Tests that assert RavenDB implementation details rather than the contract both persisters offer. The multi-attempt tests asserted Raven's ten-attempt trimming and its full attempt history, neither of which EF provides by design. These read as EF gaps when they are really over-specified tests, and they are the main reason the EF exclusion list looks longer than the real feature gap. - -**Detect:** the `` blocks in the two EF acceptance csprojs are the existing inventory. Each entry is either a real gap, a settled design difference, or an over-specified test, and the comments do not currently distinguish them. - -### 5. Setup computed but never asserted (rot) - -Fixtures, headers, constants and context properties that exist to support an assertion that was removed or never written. `Recoverability/MessageFailures` held eight: a `Retried` flag written by a handler and read by nothing in four tests whose subject is that a retry happened, two `FromAddress` and two `LocalAddress` captures, each with a constructor parameter that existed only to feed them. Thirteen `Console.WriteLine` calls in eleven files were the same thing in another form, printing either nothing identifying or what the neighbouring assertion message already says. - -Harmless on its own, but it makes tests read as though they cover more than they do, which is how the first two patterns survive review. - -**Detect:** per-file reading. Context properties with a setter and no read outside the scenario are the strongest signal. - -## The routes that need tests - -40 of the 85 routes were called by no acceptance test in any suite when this list was confirmed, route by route, against the two approved route lists and the test sources. The licensing block has since been covered, leaving 32. This is the whole list rather than a sample of it, and ticking every box here is what closes phase 4. - -The groups are sized to be one PR each and ordered by what breaks in ServicePulse if the route regresses. Each entry names the ServicePulse consumer where there is one, because that is what the test should assert: the contract the UI relies on, not a 200. - -### Routes behind the main ServicePulse pages (done) - -These sit behind three of the most-used pages in ServicePulse. If one regresses, the page breaks on arrival. - -- [x] `GET /api/heartbeats/stats`: the Heartbeats page (`viewHeartbeats`), by `Monitoring/When_heartbeat_stats_are_requested` -- [x] `GET /api/messages2`: the audit messages page (`viewAuditMessages`), by `WebApi/When_failed_messages_are_queried` -- [x] `GET /api/license`: the Licence page (`viewLicense`), by `Licensing/When_the_license_is_requested` - -This tier was originally called "nav-gating", on the grounds that a regression here costs ServicePulse a whole section of its navigation. That is not how the gating works, and the distinction changes what these tests are for. - -ServicePulse decides which nav items to render with `canCall(ApiRoutes.viewHeartbeats)`, which reduces to `!shouldGate || store.routes.has(normalizeRouteKey(method, path))`. `store.routes` is a manifest fetched once from `GET /api/my/routes`, so a nav item appears when the route is *advertised in the manifest*, never because the route was called and answered. A route that throws on every request keeps its nav item and breaks the page behind it. Only removing the route, or changing the permission that filters it, takes the nav item away. The gating is fail-open besides: `shouldGate` requires `authEnabled && isAuthenticated && loaded`, so an install without OpenID Connect renders every nav item regardless. - -The manifest itself is covered, by `ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_my_routes_are_requested`, which asserts it is the projection of what the server actually enforces. See the note in [Scope](#scope): that file is out of scope for this review, which is how the risk came to be listed as uncovered here. - -So the three are not a journey and should not be written as one. Their arrangements have nothing in common: `license` needs none at all, `heartbeats/stats` needs endpoints registered in `IEndpointInstanceMonitoring` with heartbeats flowing, and `messages2` needs ingested failed messages. Nothing one call returns feeds the next, which is the property that earned the licensing block a single scenario. Three separate tests, filed in the area each belongs to. - -Writing them turned up three things worth keeping. - -`GET /api/license` answers **400** without a `clientName`. `LicenseController` is a `#nullable enable` file, so the non-nullable `string clientName` is inferred as required and the request is rejected before the action runs. The parameter exists only to label a marketing link, so the route that backs the Licence page rejects a caller who does not want one. ServicePulse always sends it, alongside `refresh=true`, which is why nothing has noticed. `When_the_license_is_requested` now covers both the answer ServicePulse gets and the 400, so the behaviour is pinned rather than latent. - -`GET /api/messages2` builds its paging from `page_size` alone. It is the only paged route that does not bind `PagingInfo` through `PagingInfoModelBinder`: it takes a bare `int pageSize` and hands it to `new PagingInfo(pageSize: pageSize)`, which takes the value as given. Omitting `page_size` therefore asks for a page of nothing and gets an empty list, where every other route falls back to 50. It also reads `page_size` rather than `per_page` and ignores `page` entirely, both of which match how ServicePulse calls it (`auditClient.ts` sends `page_size` and windows by date instead of paging), so the divergence is deliberate. Only the missing floor is a rough edge. - -Search is not visible at the same moment on every persister. On SQL Server the full text index is populated asynchronously, so a `q=` search answers nothing for a while after the plain list already returns the message. A test that waits for ingestion on the unsearched list and then searches passes on RavenDB and PostgreSQL and fails on SQL Server, which is how this one first failed. The rule is now written down in [Writing acceptance tests](writing-acceptance-tests.md): wait on the path you are about to assert on. - -`GET /api/licensing/report/available` backs the Throughput page and belongs to this tier too, but it is written with the rest of the licensing block below. `GET /api/connection` backs the Connections page and is covered in MultiInstance only, so it is a home decision rather than a new test. - -### Licensing and throughput (done) - -All eight `api/licensing` routes, covered by one scenario: `ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_non_broker_transport`. The open question about whether this needed its own test project is answered: `LicensingComponent` is in `ServiceControlMainInstance.Components`, so the acceptance host already starts it, and both the Raven and EF persisters register an `ILicensingDataStore`, so the test needs no exclusions. - -The scenario arranges throughput the way it really arrives, by dispatching the message a monitoring instance sends to the throughput queue, then walks the ServicePulse throughput page: check a report is possible, review where the numbers come from, correct the queue that is not an NServiceBus endpoint, redact the customer name, download the report, and assert the redaction and the correction both survive into the file that gets sent to Particular. - -- [x] `GET /api/licensing/report/available`: gates the Throughput nav item (`viewThroughput`) -- [x] `POST /api/licensing/settings/masks/update`: `manageThroughput` -- [x] `GET /api/licensing/settings/masks`: throughput masking settings -- [x] `GET /api/licensing/settings/test`: connection test on the throughput settings page -- [x] `GET /api/licensing/settings/info`: throughput settings summary -- [x] `GET /api/licensing/report/file`: downloads the throughput report -- [x] `GET /api/licensing/endpoints`: endpoint throughput list -- [x] `POST /api/licensing/endpoints/update`: saves per-endpoint user indicators - -Writing it surfaced a domain rule no route-level test would have reached: a report counts only complete days, so throughput recorded for today does not make one available. The first version of the scenario reported today's numbers and sat on a 90-second timeout. That rule is now stated in the test's arrangement. - -Each route is covered on both of its branches, by one scenario each. The suite runs on LearningTransport, which registers no `IBrokerThroughputQuery`, so `ThroughputCollector` receives null for it and takes the branch only Learning and MSMQ take. RabbitMQ, Azure Service Bus, SQS, SQL Server and PostgreSQL all register one, and on that branch `report/available` requires broker-sourced throughput rather than any throughput, `settings/test` runs a broker connection test, `settings/info` returns the broker's settings, and the report carries a `ReportMethod` of `Broker` rather than `ServiceControl`. The test names say which branch each one covers. - -- [x] Cover the broker path for the eight `api/licensing` routes - -`When_creating_a_usage_report_on_a_broker_transport` covers the branch every production install except MSMQ takes. It registers a fake `IBrokerThroughputQuery` through `CustomizeHostBuilderBeforeServiceControl`, because `AddLicensingComponent` decides whether to start `BrokerThroughputCollectorHostedService` by checking whether a query is registered, and it decides that while `AddServiceControl` runs. It also replaces the collector registration with one whose `DelayStart` is zero, since the production value of 40 seconds is longer than a scenario should take. - -Beyond the branch itself it asserts the grouping: a broker queue and a monitored endpoint whose names differ only by the sanitized character have to end up as one endpoint in the report, or a customer's usage is counted twice. Making the fake's `SanitizeEndpointName` an identity function fails that assertion, so it is doing real work. - -Getting the two scenarios to pass together turned up a fault in the suite that had nothing to do with them. `RavenPersisterSettings.ThroughputDatabaseName` defaults to a fixed name, and the Raven acceptance storage configuration set only `DatabaseName`, so throughput and licensing data from every acceptance test shared one database while everything else was isolated per test. The persistence tests already set it per test; the acceptance ones now do the same, and clean it up. Two things made this hard to see: the symptom looked like queue interference, because the two tests' endpoints differ only by the character the broker sanitizes and so merged into one grouped endpoint, and the `MonitoringService` bug fixed separately was losing endpoints at the same time. The EF suites were never affected, since throughput lives in their per-test database. - -### Notifications - -- [ ] `GET /api/notifications/email`: `viewNotifications` -- [ ] `POST /api/notifications/email`: `manageNotifications` -- [ ] `POST /api/notifications/email/test`: `testNotifications` -- [ ] `POST /api/notifications/email/toggle`: the enable/disable switch - -### Actions ServicePulse offers on messages and groups - -- [ ] `PATCH/POST /api/errors/archive`: `deleteMessage`, the batch delete. `PATCH /api/errors/{id}/archive` is well covered, so the single-message route passing has been standing in for a batch route no test calls -- [ ] `POST /api/recoverability/groups/{id}/errors/unarchive`: `restoreGroup` -- [ ] `DELETE /api/customchecks/{id}`: `dismissCustomCheck`. Same shape as heartbeats: `GET /api/customchecks` is covered and the dismiss route beside it is not -- [ ] `DELETE /api/recoverability/unacknowledgedgroups/{id}`: dismissing completed group operations - -### Endpoint settings - -- [ ] `PATCH /api/endpointssettings/{name}`: `manageEndpointSettings` -- [ ] `GET /api/endpointssettings`: the settings list the PATCH edits - -### Failed message and group queries - -- [ ] `GET /api/errors/summary`: failed-message summary counts -- [ ] `GET /api/recoverability/history`: retry history panel -- [ ] `POST /api/recoverability/groups/{id}/comment`: group comments -- [ ] `DELETE /api/recoverability/groups/{id}/comment`: group comments -- [ ] `GET /api/recoverability/groups/id/{groupId}`: single group. The archive twin, `GET /api/archive/groups/id/{groupId}`, is covered -- [ ] `GET /api/endpoints/{name}/errors`: failed messages filtered to one endpoint - -### Retry and resolve routes - -- [ ] `POST /api/errors/queues/{queueAddress}/retry`: retry everything for a queue -- [ ] `PATCH /api/pendingretries/queues/resolve`: resolve pending retries by queue -- [ ] `PATCH /api/errors/{from}...{to}/unarchive`: unarchive by date range - -### Audit-backed message queries - -These need an audit instance, so decide whether MultiInstance is the right home before adding them to the primary suite. - -- [ ] `GET /api/messages/search`: the `?q=` form. The `/search/{keyword}` form is covered -- [ ] `GET /api/endpoints/{endpoint}/messages/search`: the same, scoped to an endpoint -- [ ] `GET /api/endpoints/{endpoint}/audit-count`: audit counts per endpoint - -### Configuration surface - -- [ ] `GET /api/configuration`: only `/api/configuration/remotes` is covered today -- [ ] `GET /api/instance-info`: same action as `/api/configuration`, separate route -- [ ] `GET /api/edit/config`: the edit-and-retry feature flag ServicePulse reads before offering edit -- [ ] `GET /api/license/details` -- [ ] `POST /api/license/detailsUpload` - -### Deliberately untested - -No ServicePulse journey reaches these, and ServicePulse issues no HEAD request at all, so no scenario in this plan will cover them. Recorded here so that they stay a decision rather than an oversight: if a consumer for them turns up, they need tests. - -- `HEAD /api/errors`: the count behind failed-message paging -- `HEAD /api/recoverability/groups/{id}/errors`: the count behind group paging - -`HEAD /api/redirect` is the exception and is already covered, by `When_a_request_is_repeated_with_its_etag`. - -### Covered only in MultiInstance - -Not gaps, but not in the primary suite either. Decide deliberately whether MultiInstance is the right home before duplicating any of them. - -- [ ] `GET /api/connection`: gates the Connections nav item (`viewConnections`) -- [ ] `GET /api/endpoints/known`: known endpoints list, covered by `When_endpoint_known_to_audit_instance` -- [ ] `GET /api/conversations/{id}`: sequence diagram -- [ ] `GET /api/sagas/{id}`: saga diagram -- [ ] `GET /api/endpoints/{endpoint}/messages` -- [ ] `GET /api/endpoints/{endpoint}/messages/search/{keyword}` - -Worth being precise about the heartbeats entry, because several others are the same shape. Heartbeat *ingestion* is well covered: six tests drive endpoints starting up, going quiet, and being marked monitored. What no test calls is `/api/heartbeats/stats`, the endpoint ServicePulse's heartbeats page actually reads. The plumbing is tested; the contract on top of it is not. - -## The work - -### Phase 1: confirm the gap list properly (done) - -The original table was grep-based, and a route reached through a helper, a constant or an interpolated base path would have been missed. The list above replaces it, built from both approved route lists rather than from greps and checked route by route against the primary and MultiInstance sources. Reading only the ServiceControl route list is what hid the licensing routes in the first place, so both were read. - -Two details mattered while confirming it. The approved lists carry the action-level template only, so they hold two rows both reading `GET /configuration`, one on `RootController` under `api` and one on `AuthenticationController` under `api/authentication`; the controller-level `[Route]` has to be folded in or the two merge. And the verb has to be tracked separately from the path, because ServicePulse gates `GET` and `POST` on `/api/notifications/email` as two different capabilities. - -Six entries changed as a result. `GET /api/endpoints/known` is not a gap: MultiInstance covers it. The other five were gaps the grep missed, and four of them gate a ServicePulse capability: `GET /api/license` and `GET /api/connection` gate nav items, and `DELETE /api/customchecks/{id}`, `PATCH/POST /api/errors/archive` and `POST /api/recoverability/groups/{id}/errors/unarchive` are actions the UI offers. - -No tooling came out of this phase, deliberately. A test that scans the suite's source to police its own coverage is a second thing to maintain and gets stale in its own way; the list above is the artefact, and new routes are a review-time concern. - -### Phase 2: the silent-registration class (done) - -The whole suite registers services from a test in seven places, so the audit was exhaustive rather than a sample. Five were correct. Two were the known `IEnrichImportedErrorMessages` cases, already fixed. One was new: - -`When_a_critical_error_is_triggered` registered `CriticalErrorCustomCheck` as its own concrete type, with a comment saying it overrode the production registration to shorten the check interval. It did not. The check is registered with `TryAddEnumerable` against `ICustomCheck` and consumed through `GetServices()`, so the test's registration was never resolved and the check ran on its 60-second production interval. The test passed either way, about a minute slower than intended. It now removes the production registration explicitly and re-adds the check against `ICustomCheck`, and runs in six seconds. - -One registration is worth knowing about even though it is correct. `When_a_retry_fails_to_be_sent` substitutes a `FakeReturnToSender` by re-registering `ReturnToSender`, which works only because `CustomizeHostBuilder` runs after all production registration and `ReturnToSenderDequeuer` resolves a single instance rather than a collection. That is a real distinction, not a detail: the same move against a collection adds a second implementation and leaves the production one running. - -No harness check came out of this phase. The underlying fault is a test written so that it could pass without its own setup taking effect, and a convention test policing registrations would catch one shape of that while leaving the rest. The practice is written down instead, in [Writing acceptance tests](writing-acceptance-tests.md), which covers registering against the injected abstraction, replacing a production registration so that it fails loudly if production moves, and asserting on evidence the double actually ran. - -### Phase 3: sweep the 71 files, one area per PR - -Read for the five patterns above, area by area, so each PR stays reviewable and the diff maps to one owner's mental model. Fix what is cheap to fix in the same PR; raise anything that changes what a test means as its own change with the reasoning written down. - -- [x] `Recoverability/MessageFailures`, 22 files, the densest area and the one the EF work touches most. - - One unfalsifiable assertion, `When_a_invalid_id_is_sent_to_retry`, which asserted the flag its own `Done` predicate had already waited for. It now asserts what the test is named for: retrying an id that does not exist answers `202 Accepted` rather than rejecting, and the scenario still completes, so the batch behind it kept moving. The retry loop it used to sit behind is gone. It was there to wait for an API that is never not ready: the instance is started while the component runner is created, before any endpoint starts, and ServiceControl has no code path that answers 503. The loop also caught every non-success alike, so a genuine rejection would have spun rather than failed. - - One test reporting failure as a bare timeout, `ErrorImportPerformanceTests`. The count now goes on the scenario context, which the runner prints when a scenario does not finish, so a failure says how many of the 100 messages arrived. - - Dead setup in six files: `Retried` in four, `FromAddress` in two, `LocalAddress` in two, each written by a handler and read by nothing, along with the `ReceiveAddresses` parameter that only existed to feed them. `Retried` was the misleading one, sitting in tests whose subject is that a retry happened while `RetryCount` did the actual work. - - Nothing found for the persister-coupling pattern: no file in this area is excluded from the EF suites. The two registrations here were already settled in phase 2. - - Three tests moved to the `Do` sequence helper, which is where the area's readability was worst. `When_a_retry_for_a_failed_message_is_successful` held a four-step sequence inside a single `Done` predicate five times over, re-entered on every poll, with a `RetryIssued` guard to stop the retry firing repeatedly. As steps that guard is unnecessary, though the flag itself stays because the handler reads it to decide whether to throw. `When_a_failed_message_is_pending_retry` and `When_a_invalid_id_is_sent_to_retry` had the same shape spread across chained endpoint `When` clauses. A stalled run now names the step it stopped on rather than only the elapsed time. - - Thirteen `Console.WriteLine` calls went, across eleven files. Six were a bare "Message Handled" in a handler, which carries no identity and fires on every delivery, so in tests turning on how many times a message was handled it cannot tell the first attempt from the retry. Worse, they sat next to the counter that does answer that, and the runner already prints the context on failure. The rest either narrated a step that throws with detail when it fails, or dumped state next to an assertion whose message says the same thing. They read as debugging left in place rather than diagnostics anyone chose. - - Not every test wants this. A test that sends a message, waits for one thing and asserts reads worse as a sequence, which is most of `When_a_message_has_failed`. A step also has no `bus`, so anything that sends has to stay an endpoint `When`. - - Converting to steps exposed a second tautology underneath the first. Four of these tests polled until the message was `Resolved` and then asserted it was `Resolved`, which cannot fail: if the status never changes the scenario times out and the assertion never runs. Three were dropped, since the step named "Wait for it to be resolved" reports the same failure with the same precision. The event log test keeps its assertions because they check the description and the related message id, which the poll does not. -- [x] `Recoverability/*` root, `Groups`, `MessageRedirects`, and `Monitoring/*` and `EventLogs`: swept together, since the mechanical passes cover both in one go and the findings were thin. - - `When_single_message_fails_in_batch` was the last test in the suite with no assertion and no sequence to read on failure, and it is one of the two that started this review. Nothing in it checked that its double ran, so the original bug would still pass today. It now asserts the enricher threw, which is not gated on by the scenario: registering the double as its concrete type again fails the test in six seconds with "The enricher never threw, so nothing in the batch failed and the test proved nothing". The imported count also goes on the context so a timeout says how far ingestion got. - - Two more unfalsifiable assertions, both fixed by making the sequence explicit and dropping the restatement: `When_a_message_without_a_correlationid_header_is_retried` asserted the flag its own `Done` waited for, and `MessageRedirects/When_a_message_is_retried` asserted `Received` after `Done(ctx => ctx.Received)`. - - One dead context property, `EmailDropPath`, written by the scenario while the test read the local it was copied from. - - Nothing for the registration or persister-coupling patterns. The four registrations here were settled in phase 2, and every EF exclusion in these areas is in `ExternalIntegration`. -- [x] `Recoverability/ExternalIntegration` and `Monitoring/ExternalIntegration`: reviewed and brought into shape, still excluded from the EF suites until the EF external-integration work lands. - - Six unfalsifiable assertions, the densest pocket in the suite. Every one restated the flag its own `Done` had waited for. Two files already carried a real assertion underneath, so the restatement simply went: `When_a_custom_check_fails` checks the type name external subscribers bind to, and `When_encountered_an_error` checks that the faulty publisher actually ran, which is the point of registering one. - - The other three had nothing else, so removing the restatement would have left them asserting nothing at all. They now assert the same contract their sibling does, the `EnclosedMessageTypes` header of the published event, which is what an external subscriber binds to and what a rename would silently break. Renaming the expected type fails them with the old and new names side by side. - - These are single-wait tests, so none became a sequence. Nothing found for the registration, timeout or rot patterns. - -### Phase 4: work through the route list - -Straight down the list in "The routes that need tests", one PR per group, in the order the groups are given. The phase is done when every box is ticked. - -Each test asserts the contract its consumer relies on rather than just a 200: the shape ServicePulse reads, the status it branches on, the effect the action has on the next request. A test that only proves the route is routable would leave the same gap in a different form. - -## What this plan does not do - -It does not keep the route list current by machine. The list is a snapshot taken during phase 1, and a route added after that will not appear in it on its own. Catching those is a review-time concern: a new controller action arrives with the PR that adds it, which is where the test for it belongs. - -It does not review the `Security/*` tests, the audit instance, or the monitoring instance. - -It does not treat persister parity as a workstream in its own right. It appears only as one defect pattern, on the grounds that most of the current EF exclusions are over-specified tests rather than missing features, which is itself a claim worth confirming during phase 3. diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs new file mode 100644 index 0000000000..11ebf3f150 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs @@ -0,0 +1,83 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System; + using System.Linq; + using System.Threading; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NServiceBus.CustomChecks; + using NUnit.Framework; + using ServiceBus.Management.Infrastructure.Settings; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheck; + using CheckStatus = global::ServiceControl.Persistence.Status; + + class When_a_failing_custom_check_is_dismissed : AcceptanceTest + { + [Test] + public async Task Should_come_back_while_the_check_is_still_failing() + { + CustomCheckView dismissed = null; + CustomCheckView returned = null; + + await Define() + .WithEndpoint() + .Do("Wait for the check to report a failure", async ctx => + { + var checks = await this.TryGetMany("/api/customchecks", + check => check.CustomCheckId == CheckId && check.Status == CheckStatus.Fail); + + dismissed = checks.HasResult ? checks.Items.Single() : null; + + return dismissed != null; + }) + .Do("Dismiss it from the page", async _ => + await this.Delete($"/api/customchecks/{WithoutPrefix(dismissed.Id)}")) + .Do("Wait until it has gone", async _ => + { + var checks = await this.TryGetMany("/api/customchecks"); + + return checks.Items.All(check => check.CustomCheckId != CheckId); + }) + .Do("Wait for the next report from the endpoint", async _ => + { + var checks = await this.TryGetMany("/api/customchecks", + check => check.CustomCheckId == CheckId && check.Status == CheckStatus.Fail); + + returned = checks.HasResult ? checks.Items.Single() : null; + + return returned != null; + }) + .Done(_ => true) + .Run(); + + Assert.That(returned.FailureReason, Is.EqualTo(dismissed.FailureReason), + "Dismissing a check that is still failing cannot silence it for good, or a real failure disappears from the page for as long as it lasts"); + } + + static string WithoutPrefix(string id) => + id.StartsWith(DocumentPrefix, StringComparison.OrdinalIgnoreCase) ? id[DocumentPrefix.Length..] : id; + + const string DocumentPrefix = "CustomChecks/"; + const string CheckId = "DismissedCheck"; + + class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + } + + public class Checked : EndpointConfigurationBuilder + { + public Checked() => + EndpointSetup(c => c.ReportCustomChecksTo(Settings.DEFAULT_INSTANCE_NAME, TimeSpan.FromSeconds(1))); + + class FailingCheck() : CustomCheck(CheckId, "Testing", TimeSpan.FromSeconds(1)) + { + public override Task PerformCheck(CancellationToken cancellationToken = default) => + Task.FromResult(CheckResult.Failed("Still failing")); + } + } + } +} diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs new file mode 100644 index 0000000000..15c915b4e1 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs @@ -0,0 +1,223 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System; + using System.IO; + using System.Linq; + using System.Net; + using System.Net.Http; + using System.Text; + using System.Threading; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NServiceBus.CustomChecks; + using NUnit.Framework; + using ServiceBus.Management.Infrastructure.Settings; + using ServiceControl.Notifications; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheck; + using CheckStatus = global::ServiceControl.Persistence.Status; + + class When_email_notifications_are_configured : AcceptanceTest + { + [Test] + public async Task Should_gate_notifications_on_the_settings_the_page_saved() + { + var emailDropPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + Directory.CreateDirectory(emailDropPath); + + SetSettings = settings => + { + settings.NotificationsFilter = $"{SilencedCheck}#{WatchedCheck}"; + settings.EmailDropFolder = emailDropPath; + }; + + EmailNotifications initial = null; + EmailNotifications saved = null; + EmailNotifications enabled = null; + HttpResponseMessage testEmail = null; + string[] delivered = null; + + try + { + await Define() + .WithEndpoint() + .Do("Read the settings the notifications page opens with", async _ => + { + initial = await this.TryGet("/api/notifications/email"); + + return initial != null; + }) + .Do("Save an SMTP server to send through", async _ => + { + await this.Post("/api/notifications/email", new + { + smtp_server = SmtpServer, + smtp_port = SmtpPort, + from = From, + to = To, + enable_tls = false + }); + + saved = await this.TryGet("/api/notifications/email"); + }) + .Do("Check the server before relying on it", async _ => + { + // Nothing is listening on that port, and the route sends through real SMTP even + // when EmailDropFolder is set, so this is the answer a wrong server produces. + testEmail = await HttpClient.PostAsync("/api/notifications/email/test", null); + }) + .Do("Let a check fail while notifications are still off", async ctx => + { + var checks = await this.TryGetMany("/api/customchecks", + check => check.CustomCheckId == SilencedCheck && check.Status == CheckStatus.Fail); + + return checks.HasResult; + }) + .Do("Switch notifications on", async _ => + { + await this.Post("/api/notifications/email/toggle", new { enabled = true }); + + enabled = await this.TryGet("/api/notifications/email"); + }) + .Do("Let the other check fail now they are on", async ctx => + { + ctx.WatchedCheckFails = true; + + var emails = await EmailsOnceDelivered(emailDropPath); + + delivered = emails; + + return delivered != null; + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(initial.Enabled, Is.False, + "Notifications start off, so saving a server cannot be what turns them on"); + + Assert.That(saved.SmtpServer, Is.EqualTo(SmtpServer)); + Assert.That(saved.SmtpPort, Is.EqualTo(SmtpPort)); + Assert.That(saved.From, Is.EqualTo(From)); + Assert.That(saved.To, Is.EqualTo(To), + "The page reads its own form back from this route, so what was saved has to come back"); + + Assert.That(saved.Enabled, Is.False, + "Saving a server must not switch notifications on behind the operator"); + + Assert.That(testEmail.StatusCode, Is.EqualTo(HttpStatusCode.InternalServerError), + "A server that cannot be reached has to be reported, not silently accepted"); + + Assert.That(testEmail.Headers.TryGetValues("X-Particular-Reason", out var reason) ? reason.Single() : null, + Is.EqualTo("Error sending test email notification"), + "ServicePulse shows this header rather than the status code alone"); + + Assert.That(enabled.Enabled, Is.True); + + Assert.That(delivered, Has.All.Contains(WatchedCheck), + "Only the check that failed after the switch may produce an email"); + + Assert.That(delivered, Has.None.Contains(SilencedCheck), + "The check that failed while notifications were off must never be delivered, or the switch is decorative"); + + Assert.That(delivered.Single(), Does.Contain($"From: {From}").And.Contain($"To: {To}"), + "The email goes to the addresses saved through the API, not to whatever was in the store"); + } + } + finally + { + Directory.Delete(emailDropPath, recursive: true); + } + } + + // SmtpClient creates the file in the pickup folder before writing to it, so an email is only + // readable once the blank line that terminates its headers has arrived. + static async Task EmailsOnceDelivered(string emailDropPath) + { + var files = Directory.EnumerateFiles(emailDropPath).ToArray(); + + if (files.Length == 0) + { + return null; + } + + var contents = new string[files.Length]; + + for (var i = 0; i < files.Length; i++) + { + string[] lines; + + try + { + lines = await File.ReadAllLinesAsync(files[i]); + } + catch (IOException) + { + return null; + } + + var endOfHeaders = Array.IndexOf(lines, string.Empty); + + if (endOfHeaders < 0) + { + return null; + } + + contents[i] = string.Join(Environment.NewLine, lines[..endOfHeaders]) + Environment.NewLine + DecodedBody(lines[(endOfHeaders + 1)..]); + } + + return contents; + } + + // The notification body is sent base64 encoded, so the check that caused it is only legible + // once it is decoded. + static string DecodedBody(string[] lines) + { + try + { + return Encoding.UTF8.GetString(Convert.FromBase64String(string.Concat(lines))); + } + catch (FormatException) + { + return string.Join(Environment.NewLine, lines); + } + } + + const string SilencedCheck = "SilencedCheck"; + const string WatchedCheck = "WatchedCheck"; + const string SmtpServer = "localhost"; + const int SmtpPort = 25252; + const string From = "servicecontrol@particular.net"; + const string To = "oncall@particular.net"; + + public class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + + public bool WatchedCheckFails { get; set; } + } + + public class EndpointWithCustomChecks : EndpointConfigurationBuilder + { + public EndpointWithCustomChecks() => + EndpointSetup(c => c.ReportCustomChecksTo(Settings.DEFAULT_INSTANCE_NAME, TimeSpan.FromSeconds(1))); + + class AlwaysFailingCheck() : CustomCheck(SilencedCheck, "Testing", TimeSpan.FromSeconds(1)) + { + public override Task PerformCheck(CancellationToken cancellationToken = default) => + Task.FromResult(CheckResult.Failed("Failing before notifications were switched on")); + } + + class EventuallyFailingCheck(Context scenarioContext) : CustomCheck(WatchedCheck, "Testing", TimeSpan.FromSeconds(1)) + { + public override Task PerformCheck(CancellationToken cancellationToken = default) => + Task.FromResult(scenarioContext.WatchedCheckFails + ? CheckResult.Failed("Failing after notifications were switched on") + : CheckResult.Pass); + } + } + } +} diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/When_endpoint_tracking_is_configured.cs b/src/ServiceControl.AcceptanceTests/Monitoring/When_endpoint_tracking_is_configured.cs new file mode 100644 index 0000000000..29db77686d --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/When_endpoint_tracking_is_configured.cs @@ -0,0 +1,98 @@ +namespace ServiceControl.AcceptanceTests.Monitoring +{ + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceBus.Management.Infrastructure.Settings; + using ServiceControl.Monitoring; + using Conventions = NServiceBus.AcceptanceTesting.Customization.Conventions; + + class When_endpoint_tracking_is_configured : AcceptanceTest + { + [Test] + public async Task Should_read_and_change_tracking_for_one_endpoint_and_for_the_default() + { + List initial = null; + List afterEndpointChange = null; + List afterDefaultChange = null; + + await Define() + .WithEndpoint() + .Do("Read the settings the heartbeats page opens with", async _ => + { + initial = (await this.TryGetMany("/api/endpointssettings")).Items; + + return initial.Count > 0; + }) + .Do("Stop tracking instances of the endpoint that redeploys", async _ => + { + await this.Patch($"/api/endpointssettings/{TrackedEndpoint}", new { track_instances = false }); + + var settings = await this.TryGetMany("/api/endpointssettings", + setting => setting.Name == TrackedEndpoint && !setting.TrackInstances); + + afterEndpointChange = settings.HasResult + ? (await this.TryGetMany("/api/endpointssettings")).Items + : null; + + return afterEndpointChange != null; + }) + .Do("Change the default every other endpoint inherits", async _ => + { + await this.Patch("/api/endpointssettings", new { track_instances = false }); + + var settings = await this.TryGetMany("/api/endpointssettings", + setting => setting.Name == string.Empty && !setting.TrackInstances); + + afterDefaultChange = settings.HasResult + ? (await this.TryGetMany("/api/endpointssettings")).Items + : null; + + return afterDefaultChange != null; + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(Default(initial)?.TrackInstances, Is.True, + "The page reads its default from the row with an empty name, so one has to be there before anything is saved"); + + Assert.That(afterEndpointChange.Single(setting => setting.Name == TrackedEndpoint).TrackInstances, Is.False, + "Turning tracking off for one endpoint has to come back off"); + + Assert.That(Default(afterEndpointChange), Is.Not.Null, + "The page reads the default row with a non-null assertion, so saving one endpoint must not take it away"); + + Assert.That(Default(afterEndpointChange).TrackInstances, Is.True, + "Changing one endpoint is not changing the default"); + + Assert.That(Default(afterDefaultChange).TrackInstances, Is.False); + + Assert.That(afterDefaultChange.Select(setting => setting.Name), Is.EquivalentTo(afterEndpointChange.Select(setting => setting.Name)), + "Changing the default edits the row that is already there rather than adding another"); + } + } + + static SettingsData Default(IEnumerable settings) => + settings.SingleOrDefault(setting => setting.Name == string.Empty); + + static string TrackedEndpoint => Conventions.EndpointNamingConvention(typeof(Tracked)); + + class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + } + + public class Tracked : EndpointConfigurationBuilder + { + public Tracked() => + EndpointSetup(c => c.SendHeartbeatTo(Settings.DEFAULT_INSTANCE_NAME)); + } + } +} diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_failing_endpoint_is_triaged_and_retried.cs b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_failing_endpoint_is_triaged_and_retried.cs new file mode 100644 index 0000000000..befe3ac671 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_failing_endpoint_is_triaged_and_retried.cs @@ -0,0 +1,229 @@ +namespace ServiceControl.AcceptanceTests.Recoverability.Groups +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceControl.MessageFailures; + using ServiceControl.MessageFailures.Api; + using ServiceControl.Persistence; + using ServiceControl.Recoverability; + using Conventions = NServiceBus.AcceptanceTesting.Customization.Conventions; + + class When_a_failing_endpoint_is_triaged_and_retried : AcceptanceTest + { + [Test] + public async Task Should_narrow_to_the_group_annotate_it_and_clear_it_once_retried() + { + Dictionary> summary = null; + List broken = null; + FailureGroupView group = null; + GroupOperation annotated = null; + GroupOperation corrected = null; + GroupOperation cleared = null; + RetryHistory afterRetry = null; + RetryHistory afterDismissal = null; + + await Define() + .WithEndpoint(b => b.When(async bus => + { + await bus.SendLocal(new BrokenCommand()); + await bus.SendLocal(new BrokenCommand()); + }).DoNotFailOnErrorMessages()) + .WithEndpoint(b => b.When(bus => bus.SendLocal(new UnrelatedCommand())).DoNotFailOnErrorMessages()) + .Do("Wait for all three failures to be grouped", async ctx => + { + var failures = await this.TryGetMany("/api/errors?per_page=50"); + + var forBroken = failures.Items.Where(failure => failure.ReceivingEndpoint.Name == BrokenEndpoint).ToArray(); + + if (failures.Items.Count != 3 || forBroken.Length != 2) + { + return false; + } + + // A failure belongs to one group per classifier, in no guaranteed order, so the + // group has to be picked by the classifier the groups list defaults to rather + // than by position: the persisters order them differently. + var message = await this.TryGet($"/api/errors/{forBroken[0].Id}", + found => found.FailureGroups.Any(group => group.Type == DefaultClassifier)); + + ctx.GroupId = message.HasResult + ? message.Item.FailureGroups.Single(group => group.Type == DefaultClassifier).Id + : null; + + return ctx.GroupId != null; + }) + .Do("Read the summary the page opens with", async _ => + { + summary = await this.TryGet>>("/api/errors/summary"); + }) + .Do("Narrow the list to the endpoint that broke", async _ => + { + broken = (await this.TryGetMany($"/api/endpoints/{BrokenEndpoint}/errors?per_page=50")).Items; + }) + .Do("Open the group behind those failures", async ctx => + { + var opened = await this.TryGet($"/api/recoverability/groups/id/{ctx.GroupId}"); + + group = opened.Item; + + return opened.HasResult; + }) + .Do("Leave a note on the group while the fix is in flight", async ctx => + { + await this.Post($"/api/recoverability/groups/{ctx.GroupId}/comment?comment={Uri.EscapeDataString(FirstNote)}"); + + annotated = await NoteOn(ctx.GroupId, FirstNote); + + return annotated != null; + }) + .Do("Correct the note once the cause is known", async ctx => + { + await this.Post($"/api/recoverability/groups/{ctx.GroupId}/comment?comment={Uri.EscapeDataString(SecondNote)}"); + + corrected = await NoteOn(ctx.GroupId, SecondNote); + + return corrected != null; + }) + .Do("Remove the note", async ctx => + { + await this.Delete($"/api/recoverability/groups/{ctx.GroupId}/comment"); + + cleared = await NoteOn(ctx.GroupId, null); + + return cleared != null; + }) + .Do("Retry the group now the fix is out", async ctx => + { + ctx.FixDeployed = true; + + await this.Post($"/api/recoverability/groups/{ctx.GroupId}/errors/retry"); + }) + .Do("Wait for the retry to finish and report itself", async ctx => + { + var history = await this.TryGet("/api/recoverability/history", + found => found.UnacknowledgedOperations.Any(operation => operation.RequestId == ctx.GroupId)); + + afterRetry = history.Item; + + return history.HasResult; + }) + .Do("Dismiss the completed operation", async ctx => + { + await this.Delete($"/api/recoverability/unacknowledgedgroups/{ctx.GroupId}"); + + var history = await this.TryGet("/api/recoverability/history", + found => !found.UnacknowledgedOperations.Any(operation => operation.RequestId == ctx.GroupId)); + + afterDismissal = history.Item; + + return history.HasResult; + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(summary[FailedMessageSummaryKeys.Endpoints], Is.EquivalentTo(new Dictionary + { + [BrokenEndpoint] = 2, + [UnrelatedEndpoint] = 1 + }), "The summary counts failures per endpoint, which is how the page shows where the trouble is"); + + Assert.That(broken.Select(failure => failure.ReceivingEndpoint.Name), Is.EquivalentTo(new[] { BrokenEndpoint, BrokenEndpoint }), + "Narrowing to an endpoint has to leave out the other endpoint's failure, not just include this one's"); + + Assert.That(group.Count, Is.EqualTo(2), + "The group holds only the broken endpoint's failures, so the other endpoint's failure grouped separately"); + + Assert.That(annotated.Comment, Is.EqualTo(FirstNote)); + + Assert.That(corrected.Comment, Is.EqualTo(SecondNote), + "Posting a second note replaces the first rather than adding to it"); + + Assert.That(cleared.Comment, Is.Null, + "Deleting the note clears it, or an operator cannot withdraw what they wrote"); + + var completed = afterRetry.UnacknowledgedOperations.Single(operation => operation.RequestId == group.Id); + + Assert.That(completed.RetryType, Is.EqualTo(RetryType.FailureGroup), + "Only a FailureGroup operation can be dismissed through the unacknowledgedgroups route"); + + Assert.That(completed.NumberOfMessagesProcessed, Is.EqualTo(2), + "The banner tells the operator how many messages the retry moved"); + + Assert.That(afterDismissal.HistoricOperations.Select(operation => operation.RequestId), Does.Contain(group.Id), + "Dismissing clears the banner without erasing the retry from the history panel"); + } + } + + // The comment is surfaced on the groups list rather than by the single group route, which + // returns the title and counts only. Both persisters agree, and ServicePulse reads the note + // from the list too. + async Task NoteOn(string groupId, string expected) + { + var groups = await this.TryGetMany("/api/recoverability/groups", + candidate => candidate.Id == groupId && candidate.Comment == expected); + + return groups.HasResult ? groups.Items.Single() : null; + } + + const string DefaultClassifier = "Exception Type and Stack Trace"; + + const string FirstNote = "Waiting on the fix"; + const string SecondNote = "Caused by the bad release"; + + static string BrokenEndpoint => Conventions.EndpointNamingConvention(typeof(Broken)); + static string UnrelatedEndpoint => Conventions.EndpointNamingConvention(typeof(Unrelated)); + + public class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + + public string GroupId { get; set; } + + public bool FixDeployed { get; set; } + } + + public class Broken : EndpointConfigurationBuilder + { + public Broken() => EndpointSetup(c => c.NoRetries()); + + [Handler] + public class BrokenCommandHandler(Context scenarioContext) : IHandleMessages + { + public Task Handle(BrokenCommand message, IMessageHandlerContext context) + { + if (!scenarioContext.FixDeployed) + { + throw new Exception("Simulated exception"); + } + + return Task.CompletedTask; + } + } + } + + public class Unrelated : EndpointConfigurationBuilder + { + public Unrelated() => EndpointSetup(c => c.NoRetries()); + + [Handler] + public class UnrelatedCommandHandler : IHandleMessages + { + public Task Handle(UnrelatedCommand message, IMessageHandlerContext context) => + throw new Exception("Simulated exception in an endpoint that is not being triaged"); + } + } + + public class BrokenCommand : ICommand; + + public class UnrelatedCommand : ICommand; + } +} diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_timeframe.cs similarity index 91% rename from src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs rename to src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_timeframe.cs index 10066fb9ca..4d2c5462af 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_timeframe.cs @@ -14,7 +14,7 @@ using ServiceControl.MessageFailures; using ServiceControl.MessageFailures.Api; - class When_a_pending_retry_is_resolved_by_queue_and_timeframe : AcceptanceTest + class When_a_pending_retry_is_resolved_by_timeframe : AcceptanceTest { [Test] public async Task Should_succeed() => @@ -44,7 +44,6 @@ await Define() { await this.Patch("/api/pendingretries/resolve", new { - queueaddress = ctx.FromAddress, from = DateTime.UtcNow.AddHours(-1).ToString("o"), to = DateTime.UtcNow.ToString("o") }); @@ -71,15 +70,13 @@ public Failing() => [Handler] public class MyMessageHandler( Context scenarioContext, - IReadOnlySettings settings, - ReceiveAddresses receiveAddresses) + IReadOnlySettings settings) : IHandleMessages { public Task Handle(MyMessage message, IMessageHandlerContext context) { if (scenarioContext.Step == 0) { - scenarioContext.FromAddress = receiveAddresses.MainReceiveAddress; scenarioContext.UniqueMessageId = DeterministicGuid.MakeId(context.MessageId, settings.EndpointName()).ToString(); throw new Exception("Simulated Exception"); } @@ -95,7 +92,6 @@ public class Context : ScenarioContext, ISequenceContext { public string UniqueMessageId { get; set; } public int RetryCount { get; set; } - public string FromAddress { get; set; } public int Step { get; set; } } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_deleted_messages_are_restored.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_deleted_messages_are_restored.cs new file mode 100644 index 0000000000..7613cbd6b6 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_deleted_messages_are_restored.cs @@ -0,0 +1,208 @@ +namespace ServiceControl.AcceptanceTests.Recoverability.MessageFailures +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceControl.MessageFailures; + using ServiceControl.MessageFailures.Api; + using ServiceControl.Recoverability; + using Conventions = NServiceBus.AcceptanceTesting.Customization.Conventions; + + class When_deleted_messages_are_restored : AcceptanceTest + { + [Test] + public async Task Should_restore_a_deleted_selection_and_a_deleted_group() + { + string[] afterSelectionDeleted = null; + string[] afterRangeRestored = null; + string[] afterGroupDeleted = null; + string[] afterGroupRestored = null; + FailedMessage afterSelection = null; + FailedMessage afterGroup = null; + + var context = await Define() + .WithEndpoint(b => b.When(async bus => + { + await bus.SendLocal(new BrokenCommand()); + await bus.SendLocal(new BrokenCommand()); + await bus.SendLocal(new BrokenCommand()); + }).DoNotFailOnErrorMessages()) + .WithEndpoint(b => b.When(bus => bus.SendLocal(new UnrelatedCommand())).DoNotFailOnErrorMessages()) + .Do("Wait for all four failures to be grouped", async ctx => + { + var failures = await this.TryGetMany("/api/errors?per_page=50"); + + var broken = failures.Items.Where(failure => failure.ReceivingEndpoint.Name == BrokenEndpoint).ToArray(); + + if (failures.Items.Count != 4 || broken.Length != 3) + { + return false; + } + + var message = await this.TryGet($"/api/errors/{broken[0].Id}", + found => found.FailureGroups.Any(group => group.Type == DefaultClassifier)); + + if (!message.HasResult) + { + return false; + } + + ctx.GroupId = message.Item.FailureGroups.Single(group => group.Type == DefaultClassifier).Id; + ctx.Unrelated = failures.Items.Single(failure => failure.ReceivingEndpoint.Name == UnrelatedEndpoint).Id; + ctx.Deleted = broken.Take(2).Select(failure => failure.Id).ToArray(); + ctx.Kept = broken[2].Id; + + return true; + }) + .Do("Delete two of the three failures", async ctx => + await this.Patch("/api/errors/archive", ctx.Deleted.ToList())) + .Do("Wait until those two are gone", async ctx => + { + afterSelectionDeleted = await StatusesOnceArchived(ctx.Deleted); + + if (afterSelectionDeleted == null) + { + return false; + } + + afterSelection = await this.TryGet($"/api/errors/{ctx.Unrelated}"); + + return true; + }) + .Do("Restore everything deleted in the window", async _ => + { + var from = DateTime.UtcNow.AddHours(-1).ToString(Iso8601); + var to = DateTime.UtcNow.AddHours(1).ToString(Iso8601); + + await this.Patch($"/api/errors/{from}...{to}/unarchive"); + }) + .Do("Wait until they are back", async ctx => + { + afterRangeRestored = await StatusesOnceUnresolved(ctx.Deleted); + + return afterRangeRestored != null; + }) + .Do("Delete the whole group instead", async ctx => + await this.Post($"/api/recoverability/groups/{ctx.GroupId}/errors/archive")) + .Do("Wait until the whole group is gone", async ctx => + { + afterGroupDeleted = await StatusesOnceArchived([.. ctx.Deleted, ctx.Kept]); + + if (afterGroupDeleted == null) + { + return false; + } + + afterGroup = await this.TryGet($"/api/errors/{ctx.Unrelated}"); + + var archived = await this.TryGet($"/api/archive/groups/id/{ctx.GroupId}", + group => group.Count == 3); + + return archived.HasResult; + }) + .Do("Restore the whole group", async ctx => + await this.Post($"/api/recoverability/groups/{ctx.GroupId}/errors/unarchive")) + .Do("Wait until the whole group is back", async ctx => + { + afterGroupRestored = await StatusesOnceUnresolved([.. ctx.Deleted, ctx.Kept]); + + return afterGroupRestored != null; + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(afterSelectionDeleted, Has.All.EqualTo(nameof(FailedMessageStatus.Archived))); + + Assert.That(afterRangeRestored, Has.All.EqualTo(nameof(FailedMessageStatus.Unresolved)), + "Restoring by range has to bring back everything the selection deleted"); + + Assert.That(afterGroupRestored, Has.All.EqualTo(nameof(FailedMessageStatus.Unresolved)), + "Restoring a group has to bring back every message the group delete took, not only the ones deleted individually"); + + Assert.That(afterSelection.Status, Is.EqualTo(FailedMessageStatus.Unresolved), + "Deleting a selection must reach only the ids it was given"); + + Assert.That(afterGroup.Status, Is.EqualTo(FailedMessageStatus.Unresolved), + "Deleting a group must reach only that group, and the other endpoint's failure grouped separately"); + } + } + + async Task StatusesOnceArchived(IReadOnlyCollection ids) => await StatusesOnce(ids, FailedMessageStatus.Archived); + + async Task StatusesOnceUnresolved(IReadOnlyCollection ids) => await StatusesOnce(ids, FailedMessageStatus.Unresolved); + + async Task StatusesOnce(IReadOnlyCollection ids, FailedMessageStatus expected) + { + var statuses = new List(); + + foreach (var id in ids) + { + var message = await this.TryGet($"/api/errors/{id}", found => found.Status == expected); + + if (!message.HasResult) + { + return null; + } + + statuses.Add(message.Item.Status.ToString()); + } + + return [.. statuses]; + } + + const string DefaultClassifier = "Exception Type and Stack Trace"; + const string Iso8601 = "yyyy-MM-ddTHH:mm:ssZ"; + + static string BrokenEndpoint => Conventions.EndpointNamingConvention(typeof(Broken)); + static string UnrelatedEndpoint => Conventions.EndpointNamingConvention(typeof(Unrelated)); + + class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + + public string GroupId { get; set; } + + public string[] Deleted { get; set; } + + public string Kept { get; set; } + + public string Unrelated { get; set; } + } + + public class Broken : EndpointConfigurationBuilder + { + public Broken() => EndpointSetup(c => c.NoRetries()); + + [Handler] + public class BrokenCommandHandler : IHandleMessages + { + public Task Handle(BrokenCommand message, IMessageHandlerContext context) => + throw new Exception("Simulated exception"); + } + } + + public class Unrelated : EndpointConfigurationBuilder + { + public Unrelated() => EndpointSetup(c => c.NoRetries()); + + [Handler] + public class UnrelatedCommandHandler : IHandleMessages + { + public Task Handle(UnrelatedCommand message, IMessageHandlerContext context) => + throw new Exception("Simulated exception on an endpoint nothing here selects"); + } + } + + public class BrokenCommand : ICommand; + + public class UnrelatedCommand : ICommand; + } +} diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_pending_retries_are_resolved_by_queue.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_pending_retries_are_resolved_by_queue.cs new file mode 100644 index 0000000000..a337d91468 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_pending_retries_are_resolved_by_queue.cs @@ -0,0 +1,170 @@ +namespace ServiceControl.AcceptanceTests.Recoverability.MessageFailures +{ + using System; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NServiceBus.Features; + using NUnit.Framework; + using ServiceControl.MessageFailures; + using ServiceControl.MessageFailures.Api; + using Conventions = NServiceBus.AcceptanceTesting.Customization.Conventions; + + class When_pending_retries_are_resolved_by_queue : AcceptanceTest + { + [Test] + public async Task Should_resolve_only_the_queue_it_was_given() + { + FailedMessage billingAfterResolve = null; + FailedMessage shippingAfterResolve = null; + + await Define() + .WithEndpoint(b => b.When(bus => bus.SendLocal(new BillingCommand())).DoNotFailOnErrorMessages()) + .WithEndpoint(b => b.When(bus => bus.SendLocal(new ShippingCommand())).DoNotFailOnErrorMessages()) + .Do("Wait for both queues to have a failure", async ctx => + { + var failures = await this.TryGetMany("/api/errors?per_page=50"); + + if (failures.Items.Count != 2) + { + return false; + } + + var billing = failures.Items.Single(failure => failure.ReceivingEndpoint.Name == BillingEndpoint); + var shipping = failures.Items.Single(failure => failure.ReceivingEndpoint.Name == ShippingEndpoint); + + ctx.BillingQueue = billing.QueueAddress; + ctx.ShippingQueue = shipping.QueueAddress; + ctx.BillingId = billing.Id; + ctx.ShippingId = shipping.Id; + + // Set only once both have failed, so neither original failure is skipped. + ctx.FixDeployed = true; + + return true; + }) + .Do("Retry everything on both queues", async ctx => + { + await this.Post($"/api/errors/queues/{ctx.BillingQueue}/retry"); + await this.Post($"/api/errors/queues/{ctx.ShippingQueue}/retry"); + }) + .Do("Wait until both retries are left pending", async ctx => + { + // The endpoints suppress the notification that would tell ServiceControl the retry + // was handled, which is what leaves a retry pending in production too. + var billing = await this.TryGet($"/api/errors/{ctx.BillingId}", + message => message.Status == FailedMessageStatus.RetryIssued); + + var shipping = await this.TryGet($"/api/errors/{ctx.ShippingId}", + message => message.Status == FailedMessageStatus.RetryIssued); + + return billing.HasResult && shipping.HasResult; + }) + .Do("Resolve the pending retries on one queue only", async ctx => + { + await this.Patch("/api/pendingretries/queues/resolve", new + { + queueaddress = ctx.BillingQueue, + from = DateTime.UtcNow.AddHours(-1).ToString("o"), + to = DateTime.UtcNow.AddHours(1).ToString("o") + }); + + var billing = await this.TryGet($"/api/errors/{ctx.BillingId}", + message => message.Status == FailedMessageStatus.Resolved); + + billingAfterResolve = billing.Item; + + return billing.HasResult; + }) + .Do("Look at the queue that was not named", async ctx => + { + shippingAfterResolve = await this.TryGet($"/api/errors/{ctx.ShippingId}"); + }) + .Done(_ => true) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(billingAfterResolve.Status, Is.EqualTo(FailedMessageStatus.Resolved)); + + Assert.That(shippingAfterResolve.Status, Is.EqualTo(FailedMessageStatus.RetryIssued), + "Resolving by queue has to leave another queue's pending retry alone, which is the only thing separating this route from the timeframe one"); + } + } + + static string BillingEndpoint => Conventions.EndpointNamingConvention(typeof(Billing)); + static string ShippingEndpoint => Conventions.EndpointNamingConvention(typeof(Shipping)); + + public class Context : ScenarioContext, ISequenceContext + { + public int Step { get; set; } + + public bool FixDeployed { get; set; } + + public string BillingQueue { get; set; } + + public string ShippingQueue { get; set; } + + public string BillingId { get; set; } + + public string ShippingId { get; set; } + } + + public class Billing : EndpointConfigurationBuilder + { + public Billing() => + EndpointSetup(c => + { + c.DisableFeature(); + c.NoRetries(); + c.NoOutbox(); + }); + + [Handler] + public class BillingCommandHandler(Context scenarioContext) : IHandleMessages + { + public Task Handle(BillingCommand message, IMessageHandlerContext context) + { + if (!scenarioContext.FixDeployed) + { + throw new Exception("Simulated exception"); + } + + return Task.CompletedTask; + } + } + } + + public class Shipping : EndpointConfigurationBuilder + { + public Shipping() => + EndpointSetup(c => + { + c.DisableFeature(); + c.NoRetries(); + c.NoOutbox(); + }); + + [Handler] + public class ShippingCommandHandler(Context scenarioContext) : IHandleMessages + { + public Task Handle(ShippingCommand message, IMessageHandlerContext context) + { + if (!scenarioContext.FixDeployed) + { + throw new Exception("Simulated exception"); + } + + return Task.CompletedTask; + } + } + } + + public class BillingCommand : ICommand; + + public class ShippingCommand : ICommand; + } +} diff --git a/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs b/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs index 7b62cde46f..93db48638c 100644 --- a/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs +++ b/src/ServiceControl.AcceptanceTests/WebApi/When_failed_messages_are_queried.cs @@ -21,6 +21,8 @@ public async Task Should_filter_by_endpoint_and_by_search_term() List forBilling = null; List matchingTerm = null; List forBillingMatchingTerm = null; + List searchRoute = null; + List endpointSearchRoute = null; await Define() .WithEndpoint(b => b.When(async (bus, _) => @@ -46,6 +48,11 @@ await Define() forBilling = await Query($"endpoint_name={BillingEndpoint}"); forBillingMatchingTerm = await Query($"endpoint_name={BillingEndpoint}&q={SearchTerm}"); }) + .Do("Query the same two things through the search routes beside it", async _ => + { + searchRoute = await Paged($"/api/messages/search?q={SearchTerm}"); + endpointSearchRoute = await Paged($"/api/endpoints/{BillingEndpoint}/messages/search?q={SearchTerm}"); + }) .Done(_ => true) .Run(); @@ -57,6 +64,12 @@ await Define() Assert.That(TypesIn(matchingTerm), Is.EquivalentTo(new[] { NameOf(), NameOf() }), $"q has to match the bodies carrying '{SearchTerm}' across endpoints, and drop the one without it"); + Assert.That(TypesIn(searchRoute), Is.EquivalentTo(TypesIn(matchingTerm)), + "messages/search and messages2 run the same search, so ServicePulse gets the same answer whichever it asks"); + + Assert.That(TypesIn(endpointSearchRoute), Is.EquivalentTo(new[] { NameOf() }), + "Scoping the search route to an endpoint has to drop the other endpoint's match"); + Assert.That(TypesIn(forBillingMatchingTerm), Is.EquivalentTo(new[] { NameOf() }), "Supplying both narrows to the intersection rather than applying whichever filter is read last"); @@ -64,6 +77,9 @@ await Define() } } + async Task> Paged(string url) => + (await this.TryGetMany($"{url}&per_page=50")).Items; + async Task> Query(string filter) { var result = await this.TryGetMany($"/api/messages2?page_size=50&{filter}"); diff --git a/src/ServiceControl.AcceptanceTests/WebApi/When_the_configuration_page_is_read.cs b/src/ServiceControl.AcceptanceTests/WebApi/When_the_configuration_page_is_read.cs new file mode 100644 index 0000000000..f9a6ffe0b1 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/WebApi/When_the_configuration_page_is_read.cs @@ -0,0 +1,105 @@ +namespace ServiceControl.AcceptanceTests.WebApi +{ + using System.IO; + using System.IO.Compression; + using System.Net; + using System.Net.Http; + using System.Text.Json; + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using Particular.LicensingComponent.Contracts; + + class When_the_configuration_page_is_read : AcceptanceTest + { + [Test] + public async Task Should_report_the_instance_the_same_way_from_both_of_its_routes() + { + string configuration = null; + string instanceInfo = null; + + await Define() + .Done(async _ => + { + configuration = await Body("/api/configuration"); + instanceInfo = await Body("/api/instance-info"); + + return true; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(configuration, Is.EqualTo(instanceInfo), + "Both routes are the same action, and nothing would notice if they drifted apart"); + + Assert.That(configuration, Does.Contain(Settings.InstanceName), + "The configuration page names the instance it is describing"); + } + } + + [Test] + public async Task Should_accept_licensed_endpoint_details_and_report_none_without_the_licence_for_them() + { + HttpStatusCode upload = default; + HttpStatusCode read = default; + string reported = null; + + await Define() + .Done(async _ => + { + using var uploaded = await HttpClient.PostAsync("/api/license/detailsUpload", Compressed(new LicensedEndpointDetails + { + LicenseId = "a-licence-this-instance-does-not-hold", + ServiceEndDate = "2027-01-01" + })); + + upload = uploaded.StatusCode; + + using var details = await this.GetRaw("/api/license/details"); + + read = details.StatusCode; + reported = await details.Content.ReadAsStringAsync(); + + return true; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(upload, Is.EqualTo(HttpStatusCode.OK), + "The upload is how a customer supplies the endpoint details their licence covers"); + + Assert.That(read, Is.EqualTo(HttpStatusCode.NoContent), + "Details are only reported back on an Endpoint Size licence carrying endpoint metadata, and the page hides the section on this answer"); + + Assert.That(reported, Is.Empty, + "A 204 carries no body, so nothing here is for ServicePulse to parse"); + } + } + + static MultipartFormDataContent Compressed(LicensedEndpointDetails details) + { + var buffer = new MemoryStream(); + + using (var brotli = new BrotliStream(buffer, CompressionMode.Compress, leaveOpen: true)) + { + JsonSerializer.Serialize(brotli, details); + } + + var file = new ByteArrayContent(buffer.ToArray()); + + return new MultipartFormDataContent { { file, "file", "details.json.br" } }; + } + + async Task Body(string url) + { + using var response = await this.GetRaw(url); + + return await response.Content.ReadAsStringAsync(); + } + + class Context : ScenarioContext; + } +} diff --git a/src/ServiceControl.AcceptanceTests/WebApi/When_the_edit_and_retry_flag_is_read.cs b/src/ServiceControl.AcceptanceTests/WebApi/When_the_edit_and_retry_flag_is_read.cs new file mode 100644 index 0000000000..5cb2ff189e --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/WebApi/When_the_edit_and_retry_flag_is_read.cs @@ -0,0 +1,53 @@ +namespace ServiceControl.AcceptanceTests.WebApi +{ + using System.Net; + using System.Net.Http; + using System.Net.Http.Json; + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using ServiceControl.MessageFailures.Api; + + class When_the_edit_and_retry_flag_is_read : AcceptanceTest + { + [TestCase(true)] + [TestCase(false)] + public async Task Should_agree_with_whether_the_edit_route_answers(bool editingAllowed) + { + SetSettings = settings => settings.AllowMessageEditing = editingAllowed; + + EditConfigurationModel config = null; + HttpStatusCode editStatus = default; + + await Define() + .Done(async _ => + { + config = await this.TryGet("/api/edit/config"); + + using var edit = await HttpClient.PostAsync("/api/edit/does-not-exist", + JsonContent.Create(new EditMessageModel { MessageBody = "{}" }, options: SerializerOptions)); + + editStatus = edit.StatusCode; + + return config != null; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(config.Enabled, Is.EqualTo(editingAllowed), + "ServicePulse offers the Edit button on this flag alone"); + + Assert.That(editStatus == HttpStatusCode.NotFound, Is.Not.EqualTo(editingAllowed), + "The flag has to agree with the route: offering Edit while the route refuses every edit is worse than not offering it"); + + Assert.That(config.LockedHeaders, Does.Contain(Headers.MessageId), + "The page greys out the headers it is told not to let anyone change"); + } + } + + class Context : ScenarioContext; + } +} diff --git a/src/ServiceControl.MultiInstance.AcceptanceTests/Auditing/When_audit_counts_for_an_endpoint_are_requested.cs b/src/ServiceControl.MultiInstance.AcceptanceTests/Auditing/When_audit_counts_for_an_endpoint_are_requested.cs new file mode 100644 index 0000000000..bc419db5ac --- /dev/null +++ b/src/ServiceControl.MultiInstance.AcceptanceTests/Auditing/When_audit_counts_for_an_endpoint_are_requested.cs @@ -0,0 +1,90 @@ +namespace ServiceControl.MultiInstance.AcceptanceTests.Auditing +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using NServiceBus.AcceptanceTesting.Customization; + using ServiceControl.Audit.Auditing; + using TestSupport; + using Conventions = NServiceBus.AcceptanceTesting.Customization.Conventions; + + class When_audit_counts_for_an_endpoint_are_requested : AcceptanceTest + { + [Test] + public async Task Should_come_from_the_audit_instance_only() + { + List counted = null; + List unknownEndpoint = null; + + await Define() + .WithEndpoint(b => b.When((bus, _) => bus.Send(new MyMessage()))) + .WithEndpoint() + .Done(async _ => + { + var forReceiver = await this.TryGetMany( + $"/api/endpoints/{ReceiverEndpoint}/audit-count", + count => count.Count > 0, + instanceName: ServiceControlInstanceName); + + if (!forReceiver.HasResult) + { + return false; + } + + counted = forReceiver.Items; + + unknownEndpoint = (await this.TryGetMany( + $"/api/endpoints/AnEndpointThatNeverRan/audit-count", + instanceName: ServiceControlInstanceName)).Items; + + return true; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(counted.Sum(count => count.Count), Is.GreaterThan(0), + "The primary instance holds no audit data of its own, so a count here can only have come from the audit instance"); + + Assert.That(counted.Select(count => count.UtcDate), Is.Unique, + "Counts are merged per day across instances, which is what the endpoint's audit chart plots"); + + Assert.That(counted, Has.All.Matches(count => count.UtcDate.Date == count.UtcDate), + "Each point is a whole day"); + + Assert.That(unknownEndpoint, Is.Empty, + "An endpoint that processed nothing has nothing to plot"); + } + } + + static string ReceiverEndpoint => Conventions.EndpointNamingConvention(typeof(ReceiverRemote)); + + public class Context : ScenarioContext; + + public class Sender : EndpointConfigurationBuilder + { + public Sender() => + EndpointSetup(c => + c.ConfigureRouting().RouteToEndpoint(typeof(MyMessage), typeof(ReceiverRemote))); + } + + public class ReceiverRemote : EndpointConfigurationBuilder + { + public ReceiverRemote() => EndpointSetup(c => { }); + + [Handler] + public class MyMessageHandler : IHandleMessages + { + public Task Handle(MyMessage message, IMessageHandlerContext context) => Task.CompletedTask; + } + } + + public class MyMessage : ICommand; + } +} diff --git a/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs b/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs index e4de1674d7..21a775bf82 100644 --- a/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs +++ b/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs @@ -155,7 +155,7 @@ public async Task> Search([FromQuery] PagingInfo pagingInfo, string endpoint, string q, CancellationToken cancellationToken = default) { QueryResult> result = await endpointApi.Execute( - new SearchEndpointContext(pagingInfo, sortInfo, endpoint, q), Request.GetEncodedPathAndQuery(), cancellationToken); + new SearchEndpointContext(pagingInfo, sortInfo, Keyword: q, Endpoint: endpoint), Request.GetEncodedPathAndQuery(), cancellationToken); Response.WithQueryStatsAndPagingInfo(result.QueryStats, pagingInfo); return result.Results; @@ -168,7 +168,7 @@ public async Task> SearchByKeyword([FromQuery] PagingInfo pa [FromQuery] SortInfo sortInfo, string endpoint, string keyword, CancellationToken cancellationToken = default) { QueryResult> result = await endpointApi.Execute( - new SearchEndpointContext(pagingInfo, sortInfo, endpoint, keyword), Request.GetEncodedPathAndQuery(), cancellationToken); + new SearchEndpointContext(pagingInfo, sortInfo, Keyword: keyword, Endpoint: endpoint), Request.GetEncodedPathAndQuery(), cancellationToken); Response.WithQueryStatsAndPagingInfo(result.QueryStats, pagingInfo); return result.Results; diff --git a/src/ServiceControl/Monitoring/Web/EndpointsSettingsController.cs b/src/ServiceControl/Monitoring/Web/EndpointsSettingsController.cs index ebcbac29f4..01050ced57 100644 --- a/src/ServiceControl/Monitoring/Web/EndpointsSettingsController.cs +++ b/src/ServiceControl/Monitoring/Web/EndpointsSettingsController.cs @@ -34,19 +34,18 @@ public async IAsyncEnumerable Endpoints([EnumeratorCancellation] C { await using IAsyncEnumerator enumerator = dataStore.GetAllEndpointSettings(cancellationToken).GetAsyncEnumerator(cancellationToken); - bool noResults = true; + bool hasDefault = false; while (await enumerator.MoveNextAsync()) { - noResults = false; + hasDefault |= enumerator.Current.Name == string.Empty; yield return new SettingsData { -#pragma warning disable IDE0055 - Name = enumerator.Current.Name, TrackInstances = enumerator.Current.TrackInstances -#pragma warning restore IDE0055 + Name = enumerator.Current.Name, + TrackInstances = enumerator.Current.TrackInstances }; } - if (noResults) + if (!hasDefault) { yield return new SettingsData { Name = string.Empty, TrackInstances = settings.TrackInstancesInitialValue }; }