diff --git a/docs/data-versioning-design.md b/docs/data-versioning-design.md new file mode 100644 index 0000000000..739224ec54 --- /dev/null +++ b/docs/data-versioning-design.md @@ -0,0 +1,51 @@ +# Data versioning design + +## What it is + +A **data version** is the short opaque label a query result carries so that a client asking for it again can be told "nothing has changed" instead of being sent the whole answer. On the wire it is an HTTP entity-tag: the response carries `ETag`, the client sends it back as `If-None-Match`, and a matching request is answered `304 Not Modified` with no body. + +One value type carries it end to end: `DataVersion` in `src/ServiceControl.Persistence/Infrastructure/DataVersion.cs`. Every persister produces one, `QueryStatsInfo.Version` carries it out of the persistence layer, and the Web API turns it into the header. It is a `readonly struct`, so `default` is a legitimate value and no variable of the type can be null. + +This is the primary (error) instance only. The audit instance still carries a `string ETag` on its own `QueryStatsInfo` and has not been converted. + +## The one rule + +**If a field the response renders can change without the version changing, a client caches that page for ever and nothing reveals it.** No log line, no exception, no failing test. + +The promise is scoped to **one URL**, because a client only ever sends a validator back to the URL that issued it. So what must never happen is one URL answering `304` when its own body would have differed. Two different URLs sharing a value is harmless: an HTTP cache is keyed on the whole URL. + +That scoping is what makes a backend's own token usable. RavenDB's result etag stands for the state of the index behind the query, so it moves on any write the query could see, but it says nothing about which page was asked for: every `/api/errors` URL shares one value, whatever the page, sort or filter. The EF Core persisters compose over the rows they returned, so theirs differ per page. Both satisfy the rule. + +## Making one + +| Factory | Use it for | +| --------------------------------------- | ----------------------------------------------------------------------------------------------- | +| `FromToken(string)` / `FromToken(long)` | a token the backend already produces, such as a RavenDB index etag or document change vector | +| `Compose(terms)` | named terms over aggregates, where an aggregate provably moves with the fields it stands in for | +| `OverRows(summary, rows, fields)` | a list the response renders row by row: summary terms for the whole set, plus one term per row | +| `Combine(instances)` | one version for a result gathered from several instances | +| `FromClient(header)` | a validator a caller sent back, in any shape an old or current instance might emit | + +`Compose` hashes its description through `DeterministicGuid.MakeId`, so the emitted tag is a GUID rather than the underlying values. + +Term names and every field inside a row are **length prefixed**. Without that, free user text carrying a delimiter could make two different results digest identically: a failure group titled `x.y` with an empty `Type` would collide with one titled `x` whose `Type` is `y`. `Format` accepts strings, `bool`, `DateTime` and `DateTimeOffset` (both by ticks) and anything `IFormattable` under the invariant culture, and **throws** on anything else, because a type whose `ToString` is not a documented function of its content would pin the version silently. + +`OverRows` names rows by position, so a caller whose query has no `ORDER BY` has to sort them first or the validator churns. + +## Absence + +`DataVersion.None` is `default`, and it **matches nothing, not even itself**. Two parties that both know nothing have not established that nothing changed, so an empty-string validator matching itself would answer `304` for every request. + +Absence propagates in the safe direction. `WithEtag` writes no header for `None`, so no header means no `If-None-Match`, which means the full body. `Combine` returns `None` as soon as any instance reports none, rather than quietly claiming to cover an instance it could not see. + +## Reaching the client + +`WithEtag` emits **every** tag weak, as `W/"…"`. Nothing here can promise the response bytes: response compression rewrites them without touching the tag, and no endpoint enables range processing, which is the one thing an exact validator would buy. RFC 9110 requires `If-None-Match` to use the weak comparison anyway, so the marking costs nothing. + +`NotModifiedStatusHttpHandler` turns a matching request into a `304`. It compares with `EntityTagHeaderValue.Compare(useStrongComparison: false)`, because `Equals` on that type compares strength as well as the tag and its own documentation says not to use it for this. `*` matches whenever a representation exists. + +## Across instances + +Scatter-gather endpoints merge one version per instance through `Combine`. It is keyed on instance id and sorted ordinally, so the composite is independent of the order instances answered in but still moves if two instances swap which validator they report. + +An API whose own instance holds none of the data drops its own empty result before aggregating, via `AggregateStatsFromRemotesOnly`. Left in, its version-less placeholder would take the whole composite to `None` and the endpoint would emit no tag at all. diff --git a/docs/eventlog-design.md b/docs/eventlog-design.md index 17c01c2110..b9b9f8208f 100644 --- a/docs/eventlog-design.md +++ b/docs/eventlog-design.md @@ -40,7 +40,7 @@ Timestamps are when the thing happened, so an item can land in the middle of the `IEventLogDataStore` has two methods, and its XML docs are the binding contract: - `Add(EventLogItem)` persists one item. **Identity is the store's to assign** and surfaces on `EventLogItemView.Id`. That makes `Id` opaque: a stable key within one store, not something to parse. -- `GetEventLogItems(PagingInfo, knownVersion)` returns a `QueryResult` carrying the page, the total count independent of paging, and an `ETag`. Two obligations: the `ETag` is surfaced **verbatim**, so whatever the client echoes back arrives here unchanged and can be compared, and it **must change when retention removes items**, not only when one is added, since nothing else tells a polling client its cached page has gone stale. +- `GetEventLogItems(PagingInfo)` returns a `QueryResult` carrying the page, the total count independent of paging, and an `ETag`. Two obligations: the `ETag` is surfaced **verbatim**, so whatever the client echoes back arrives here unchanged and can be compared, and it **must change when retention removes items**, not only when one is added, since nothing else tells a polling client its cached page has gone stale. ## Retention diff --git a/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj b/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj index 028c471d49..3090e7f929 100644 --- a/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj +++ b/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj @@ -51,9 +51,6 @@ - - - \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedErrorsController.cs b/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedErrorsController.cs index 2a6ce095f0..8f6053a386 100644 --- a/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedErrorsController.cs +++ b/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedErrorsController.cs @@ -5,6 +5,7 @@ using Infrastructure.WebApi; using Microsoft.AspNetCore.Mvc; using Operations; + using Persistence.Infrastructure; using Persistence.RavenDB; using Raven.Client.Documents; @@ -28,7 +29,7 @@ public async Task GetFailedErrorsCount(CancellationTok var count = await query.CountAsync(cancellationToken); - Response.WithEtag(stats.ResultEtag.ToString()); + Response.WithEtag(DataVersion.FromToken(stats.ResultEtag.ToString())); return new FailedErrorsCountReponse { Count = count }; } diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedMessageRetriesController.cs b/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedMessageRetriesController.cs index 705806daba..b2a432a342 100644 --- a/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedMessageRetriesController.cs +++ b/src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedMessageRetriesController.cs @@ -4,6 +4,7 @@ using System.Threading.Tasks; using Infrastructure.WebApi; using Microsoft.AspNetCore.Mvc; + using Persistence.Infrastructure; using Persistence.RavenDB; using Raven.Client.Documents; using ServiceControl.Recoverability; @@ -24,7 +25,7 @@ public async Task GetFailedMessageRetriesCount using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken); await session.Query().Statistics(out var stats).ToListAsync(cancellationToken); - Response.WithEtag(stats.ResultEtag.ToString()); + Response.WithEtag(DataVersion.FromToken(stats.ResultEtag.ToString())); return new FailedMessageRetriesCountReponse { Count = stats.TotalResults }; } diff --git a/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj b/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj index be951002d9..34de538dff 100644 --- a/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj +++ b/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj @@ -51,9 +51,6 @@ - - - diff --git a/src/ServiceControl.AcceptanceTests/WebApi/When_a_request_is_repeated_with_its_etag.cs b/src/ServiceControl.AcceptanceTests/WebApi/When_a_request_is_repeated_with_its_etag.cs index dbc86721c0..acc17d448e 100644 --- a/src/ServiceControl.AcceptanceTests/WebApi/When_a_request_is_repeated_with_its_etag.cs +++ b/src/ServiceControl.AcceptanceTests/WebApi/When_a_request_is_repeated_with_its_etag.cs @@ -2,18 +2,19 @@ namespace ServiceControl.AcceptanceTests.WebApi { using System.Net; using System.Net.Http; + using System.Net.Http.Json; using System.Threading.Tasks; using AcceptanceTesting; using NServiceBus.AcceptanceTesting; using NUnit.Framework; - using Recoverability.MessageRedirects; class When_a_request_is_repeated_with_its_etag : AcceptanceTest { - [TestCase("/api/customchecks", "GET", false)] - [TestCase("/api/redirects", "GET", true)] - [TestCase("/api/redirect", "HEAD", true)] - public async Task Should_answer_not_modified(string url, string method, bool seedARedirect) + [TestCase("/api/customchecks", "GET")] + [TestCase("/api/redirects", "GET")] + [TestCase("/api/redirect", "HEAD")] + [TestCase("/api/errors/queues/addresses", "GET")] + public async Task Should_answer_not_modified(string url, string method) { Answer issued = null; Answer repeated = null; @@ -21,15 +22,6 @@ public async Task Should_answer_not_modified(string url, string method, bool see await Define() .Done(async ctx => { - if (seedARedirect) - { - await this.Post("/api/redirects", new RedirectRequest - { - fromphysicaladdress = "endpointA@machine1", - tophysicaladdress = "endpointB@machine2" - }, status => status is not HttpStatusCode.Created); - } - // Internal custom checks re-report on a timer, so the validator can move between // the two requests. for (var attempt = 0; attempt < 5; attempt++) @@ -61,6 +53,42 @@ await Define() Assert.That(repeated.TotalCount, Is.Not.Null.And.EqualTo(issued.TotalCount), $"{method} {url} did not carry its Total-Count through to the 304"); } + [Test] + public async Task Should_answer_with_a_new_etag_once_the_data_moves() + { + Answer before = null; + Answer after = null; + + await Define() + .Done(async ctx => + { + before = await Ask("GET", "/api/redirects", ifNoneMatch: null); + + if (before.Etag == null) + { + return false; + } + + using var created = await HttpClient.PostAsJsonAsync("/api/redirects", new + { + FromPhysicalAddress = "SomeEndpoint@MACHINE", + ToPhysicalAddress = "OtherEndpoint@MACHINE" + }); + + created.EnsureSuccessStatusCode(); + + after = await Ask("GET", "/api/redirects", before.Etag); + + return true; + }) + .Run(); + + Assert.That(after.Status, Is.EqualTo(HttpStatusCode.OK), + "a redirect was added, so the client's validator is stale and it has to be sent the new list"); + Assert.That(after.Etag, Is.Not.Null.And.Not.EqualTo(before.Etag), + "the body changed, so the validator has to move with it or the next poll caches the stale list forever"); + } + async Task Ask(string method, string url, string ifNoneMatch) { using var response = await Send(method, url, ifNoneMatch); diff --git a/src/ServiceControl.Audit.Persistence.RavenDB/Extensions/RavenQueryStatisticsExtensions.cs b/src/ServiceControl.Audit.Persistence.RavenDB/Extensions/RavenQueryStatisticsExtensions.cs index a15ce9da6f..ac2429c37d 100644 --- a/src/ServiceControl.Audit.Persistence.RavenDB/Extensions/RavenQueryStatisticsExtensions.cs +++ b/src/ServiceControl.Audit.Persistence.RavenDB/Extensions/RavenQueryStatisticsExtensions.cs @@ -1,13 +1,18 @@ namespace ServiceControl.Audit.Persistence.RavenDB.Extensions { + using System.Globalization; using Auditing.MessagesView; using Raven.Client.Documents.Session; static class RavenQueryStatisticsExtensions { - public static QueryStatsInfo ToQueryStatsInfo(this QueryStatistics stats) - { - return new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults); - } + /// + /// RavenDB's result etag hashes the state of the index behind the query: every collection's last + /// document and tombstone etag, how far the index has processed, and its definition. It therefore + /// moves on any write the query could see, which is all a validator has to do, because a client only + /// ever sends one back on a request for the same URL. + /// + public static QueryStatsInfo ToQueryStatsInfo(this QueryStatistics stats) => + new(stats.ResultEtag?.ToString(CultureInfo.InvariantCulture) ?? string.Empty, stats.TotalResults); } -} \ No newline at end of file +} diff --git a/src/ServiceControl.Audit.Persistence.RavenDB/RavenAuditDataStore.cs b/src/ServiceControl.Audit.Persistence.RavenDB/RavenAuditDataStore.cs index 6c39ad103c..56ab14f906 100644 --- a/src/ServiceControl.Audit.Persistence.RavenDB/RavenAuditDataStore.cs +++ b/src/ServiceControl.Audit.Persistence.RavenDB/RavenAuditDataStore.cs @@ -25,7 +25,7 @@ public async Task> QuerySagaHistoryById(Guid input, Can .Statistics(out var stats) .SingleOrDefaultAsync(x => x.SagaId == input, token: cancellationToken); - return sagaHistory == null ? QueryResult.Empty() : new QueryResult(sagaHistory, new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults)); + return sagaHistory == null ? QueryResult.Empty() : new QueryResult(sagaHistory, stats.ToQueryStatsInfo()); } public async Task>> GetMessages(bool includeSystemMessages, PagingInfo pagingInfo, SortInfo sortInfo, DateTimeRange timeSentRange, CancellationToken cancellationToken = default) diff --git a/src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs b/src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs index 0267bb3779..682e618eb9 100644 --- a/src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs +++ b/src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs @@ -54,6 +54,7 @@ public async Task> GetAllMessages( } Response.WithTotalCount(result.QueryStats.TotalCount); + Response.WithEtag(result.QueryStats.ETag); return result.Results; } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/BodyStorage/BodyStorage.cs b/src/ServiceControl.Persistence.EFCore/Implementation/BodyStorage/BodyStorage.cs index d974cbb51f..02c93e381d 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/BodyStorage/BodyStorage.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/BodyStorage/BodyStorage.cs @@ -8,6 +8,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation.BodyStorage; using ServiceControl.Persistence.EFCore.DbContexts; using ServiceControl.Persistence.EFCore.Entities; using ServiceControl.Persistence.EFCore.Infrastructure; +using ServiceControl.Persistence.Infrastructure; /// /// Resolves a message body from wherever it was stored. @@ -28,12 +29,15 @@ public async Task TryFetch(string bodyId, CancellationToken c return MessageBodyResult.NotFound(); } - // Bodies are immutable per message, so the id is a stable ETag. - var uniqueMessageId = row.UniqueMessageId.ToString(); + // Ingestion updates the existing row rather than adding one, so the message id is unchanged + // and cannot serve as a version alone. LastModified is written on every upsert. + var version = DataVersion.Compose( + ("uniqueMessageId", row.UniqueMessageId), + ("lastModified", row.LastModified)); if (row.BodyStoredExternally) { - var external = await storagePersistence.ReadBody(uniqueMessageId, cancellationToken); + var external = await storagePersistence.ReadBody(row.UniqueMessageId.ToString(), cancellationToken); if (external == null) { @@ -46,7 +50,7 @@ public async Task TryFetch(string bodyId, CancellationToken c return MessageBodyResult.Empty(); } - return MessageBodyResult.Available(new MessageBodyStreamContent(external.Stream, external.ContentType, external.BodySize, uniqueMessageId)); + return MessageBodyResult.Available(new MessageBodyStreamContent(external.Stream, external.ContentType, external.BodySize, version)); } if (row.BodyText != null) @@ -62,7 +66,7 @@ public async Task TryFetch(string bodyId, CancellationToken c new MemoryStream(bytes, writable: false), row.BodyContentType ?? "text/plain", bytes.Length, - uniqueMessageId)); + version)); } if (row.BodySize == 0) @@ -98,7 +102,8 @@ public async Task TryFetch(string bodyId, CancellationToken c BodyText = message.BodyText, BodyStoredExternally = message.BodyStoredExternally, BodySize = message.BodySize, - BodyContentType = message.BodyContentType + BodyContentType = message.BodyContentType, + LastModified = message.LastModified }) .FirstOrDefaultAsync(cancellationToken); @@ -109,5 +114,6 @@ sealed class BodyRow public bool BodyStoredExternally { get; init; } public int BodySize { get; init; } public string? BodyContentType { get; init; } + public DateTime LastModified { get; init; } } } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs index af870ac38b..997680763f 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs @@ -4,6 +4,8 @@ namespace ServiceControl.Persistence.EFCore.Implementation; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using ServiceControl.Contracts.CustomChecks; +using ServiceControl.Operations; +using ServiceControl.Persistence.EFCore.Infrastructure; using ServiceControl.Persistence.Infrastructure; public class CustomCheckDataStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), ICustomChecksDataStore @@ -59,21 +61,31 @@ public Task>> GetStats(PagingInfo paging, string? _ => query }; - var page = await query + var checks = await query .OrderBy(c => c.ReportedAt) + .ThenBy(c => c.Id) .Skip(paging.Offset) .Take(paging.PageSize) + .Select(c => new CustomCheck + { + Id = c.Id.ToString(), + CustomCheckId = c.CustomCheckId, + Category = c.Category, + Status = c.Status, + ReportedAt = c.ReportedAt, + FailureReason = c.FailureReason, + OriginatingEndpoint = new EndpointDetails + { + Name = c.OriginatingEndpointName, + Host = c.OriginatingEndpointHost, + HostId = c.OriginatingEndpointHostId + } + }) .ToListAsync(token); - return new QueryResult>(page.Select(c => new CustomCheck - { - Id = c.Id.ToString(), - CustomCheckId = c.CustomCheckId, - Category = c.Category, - Status = c.Status, - ReportedAt = c.ReportedAt, - FailureReason = c.FailureReason - }).ToList(), new QueryStatsInfo("", page.Count, false)); + var totalCount = await query.CountAsync(token); + + return new QueryResult>(checks, checks.ToQueryStatsInfo(totalCount)); }, cancellationToken); public Task DeleteCustomCheck(Guid id, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (context, token) => await context.CustomChecks.AsNoTracking().Where(cc => cc.Id == id).ExecuteDeleteAsync(token), cancellationToken); diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/EventLogDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/EventLogDataStore.cs index db2f53e444..c3d962f8b3 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/EventLogDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/EventLogDataStore.cs @@ -5,6 +5,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation; using ServiceControl.EventLog; using ServiceControl.Persistence.Infrastructure; using ServiceControl.Persistence.EFCore.Entities; +using ServiceControl.Persistence.EFCore.Infrastructure; public class EventLogDataStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), IEventLogDataStore { @@ -25,35 +26,11 @@ public Task Add(EventLogItem logItem, CancellationToken cancellationToken = defa }, cancellationToken); public Task>> GetEventLogItems( - PagingInfo pagingInfo, string? knownVersion = null, CancellationToken cancellationToken = default) => + PagingInfo pagingInfo, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (dbContext, token) => { var query = dbContext.EventLogItems.AsNoTracking(); - // All three aggregates in one round trip. Grouping on a constant collapses the table to a - // single row, and an empty table yields no rows at all, hence the null coalescing. - var stats = await query - .GroupBy(_ => 1) - .Select(g => new - { - Total = g.LongCount(), - Newest = g.Max(e => (DateTime?)e.RaisedAt), - HighestId = g.Max(e => (long?)e.Id) - }) - .FirstOrDefaultAsync(token); - - var total = stats?.Total ?? 0; - var version = Version(total, stats?.Newest, stats?.HighestId); - var queryStats = new QueryStatsInfo(version, total, isStale: false); - - // The point of knownVersion. Everything above is index work. - // If the caller already has the latest version, skip the rest of the query. - // No database round trip is needed. No response body is needed. - if (knownVersion is not null && knownVersion == version) - { - return QueryResult>.Unchanged(queryStats); - } - var items = await query // The key breaks ties so that items sharing a RaisedAt cannot shuffle between // pages. IX_EventLogItems_RaisedAt_Id is declared in exactly this order. @@ -73,11 +50,8 @@ public Task>> GetEventLogItems( }) .ToListAsync(token); - return new QueryResult>(items, queryStats); - }, cancellationToken); + var total = await query.LongCountAsync(token); - // Synthesised version ID to be used for an ETag. The highest key is the monotonic term: identity - // values gap but never repeat, so an insert moves the version whatever its RaisedAt says. - static string Version(long total, DateTime? newest, long? highestId) => - DeterministicGuid.MakeId($"{total}|{newest?.Ticks ?? 0}|{highestId ?? 0}").ToString(); + return new QueryResult>(items, items.ToQueryStatsInfo(total)); + }, cancellationToken); } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryDataStore.cs index e74536a194..d434750028 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryDataStore.cs @@ -26,7 +26,7 @@ public Task GetFailedMessagesStats(string? status, string? modif .FilterByStatus(status) .FilterByLastModifiedRange(modified) .FilterByQueueAddress(queueAddress) - .ToQueryStatsInfo(token), cancellationToken); + .ToCountQueryStatsInfo("failures", token), cancellationToken); public Task>> GetFailedMessagesByEndpoint(string? status, string endpointName, string? modified, PagingInfo pagingInfo, SortInfo sortInfo, CancellationToken cancellationToken = default) => ExecuteWithDbContext((dbContext, token) => dbContext.FailedMessages diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryResults.cs b/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryResults.cs index c349a9a696..2907fbf2ee 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryResults.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/FailedMessageQueryResults.cs @@ -10,7 +10,7 @@ static class FailedMessageQueryResults { public static async Task>> ToPagedResult(this IQueryable source, PagingInfo pagingInfo, SortInfo sortInfo, CancellationToken cancellationToken = default) { - var stats = await source.ToQueryStatsInfo(cancellationToken); + var total = await source.LongCountAsync(cancellationToken); var entities = await source .Sort(sortInfo) @@ -19,6 +19,6 @@ public static async Task>> ToPagedResult(th IList results = [.. entities.Select(entity => entity.ToFailedMessageView())]; - return new QueryResult>(results, stats); + return new QueryResult>(results, entities.ToQueryStatsInfo(total)); } } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/GroupsDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/GroupsDataStore.cs index 66a80e3eb2..e642fa7823 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/GroupsDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/GroupsDataStore.cs @@ -13,7 +13,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation; public class GroupsDataStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), IGroupsDataStore { public Task> GetUnresolvedGroupsByClassifier(string classifier, string? classifierFilter, CancellationToken cancellationToken = default) => - ExecuteWithDbContext(async (dbContext, token) => + ExecuteWithDbContext>(async (dbContext, token) => { var groups = ByClassifier(dbContext, classifier); @@ -29,9 +29,15 @@ public Task> GetUnresolvedGroupsByClassifier(string clas return views; }, cancellationToken); - public Task> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default) => - ExecuteWithDbContext((dbContext, token) => MostRecent( - ByClassifier(dbContext, classifier).AggregateGroups(WithStatus(dbContext, FailedMessageStatus.Archived)), token), cancellationToken); + public Task>> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default) => + ExecuteWithDbContext(async (dbContext, token) => + { + var groups = ByClassifier(dbContext, classifier); + + var views = await MostRecent(groups.AggregateGroups(WithStatus(dbContext, FailedMessageStatus.Archived)), token); + + return new QueryResult>(views, views.ToQueryStatsInfo()); + }, cancellationToken); public Task> GetUnresolvedGroup(string groupId, string? status, string? modified, CancellationToken cancellationToken = default) => ExecuteWithDbContext((dbContext, token) => SingleGroup(dbContext, groupId, FailedMessageStatus.Unresolved, status, modified, token), cancellationToken); @@ -40,10 +46,12 @@ public Task> GetArchivedGroup(string groupId, stri ExecuteWithDbContext((dbContext, token) => SingleGroup(dbContext, groupId, FailedMessageStatus.Archived, status, modified, token), cancellationToken); public Task>> GetGroupErrors(string groupId, string? status, string? modified, SortInfo sortInfo, PagingInfo pagingInfo, CancellationToken cancellationToken = default) => - ExecuteWithDbContext((dbContext, token) => InGroup(dbContext, groupId, status, modified).ToPagedResult(pagingInfo, sortInfo, token), cancellationToken); + ExecuteWithDbContext((dbContext, token) => InGroup(dbContext, groupId, status, modified) + .ToPagedResult(pagingInfo, sortInfo, token), cancellationToken); public Task GetGroupErrorsCount(string groupId, string? status, string? modified, CancellationToken cancellationToken = default) => - ExecuteWithDbContext((dbContext, token) => InGroup(dbContext, groupId, status, modified).ToQueryStatsInfo(token), cancellationToken); + ExecuteWithDbContext((dbContext, token) => InGroup(dbContext, groupId, status, modified) + .ToCountQueryStatsInfo("failures", token), cancellationToken); public Task EditComment(string groupId, string comment, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (dbContext, token) => @@ -83,7 +91,8 @@ static async Task> SingleGroup(ServiceControlDbCon .FilterByLastModifiedRange(modified)) .ToListAsync(cancellationToken); - return new QueryResult(groups.FirstOrDefault()!, groups.ToQueryStatsInfo()); + return new QueryResult(groups.FirstOrDefault()!, + groups.ToQueryStatsInfo()); } static IQueryable WithStatus(ServiceControlDbContext dbContext, FailedMessageStatus status) => @@ -118,7 +127,7 @@ static async Task AttachComments(ServiceControlDbContext dbContext, IList> MostRecent(IQueryable groups, CancellationToken cancellationToken) => + static async Task> MostRecent(IQueryable groups, CancellationToken cancellationToken) => await groups .OrderByDescending(group => group.Last) .Take(FailureGroupQueries.MaxGroups) diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/MessagesViewQueryResults.cs b/src/ServiceControl.Persistence.EFCore/Implementation/MessagesViewQueryResults.cs index 006a69170f..50168b188f 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/MessagesViewQueryResults.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/MessagesViewQueryResults.cs @@ -10,7 +10,7 @@ static class MessagesViewQueryResults { public static async Task>> ToPagedMessagesResult(this IQueryable source, PagingInfo pagingInfo, SortInfo sortInfo, CancellationToken cancellationToken = default) { - var stats = await source.ToQueryStatsInfo(cancellationToken); + var total = await source.LongCountAsync(cancellationToken); var entities = await source .SortMessages(sortInfo) @@ -19,6 +19,6 @@ public static async Task>> ToPagedMessagesResult IList results = [.. entities.Select(entity => entity.ToMessagesView())]; - return new QueryResult>(results, stats); + return new QueryResult>(results, entities.ToQueryStatsInfo(total)); } } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/QueueAddressStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/QueueAddressStore.cs index be2448f662..74793d79ee 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/QueueAddressStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/QueueAddressStore.cs @@ -3,6 +3,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using ServiceControl.MessageFailures; +using ServiceControl.Persistence.EFCore.Infrastructure; using ServiceControl.Persistence.Infrastructure; public class QueueAddressStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), IQueueAddressStore @@ -20,8 +21,8 @@ public Task>> GetAddresses(PagingInfo pagingInfo }); var items = await query.Skip(pagingInfo.Offset).Take(pagingInfo.PageSize).ToListAsync(token); - var eTag = DeterministicGuid.MakeId($"{items.Count}|{string.Join(",", items.Select(x => x.PhysicalAddress))}").ToString(); + var addressCount = await query.CountAsync(token); - return new QueryResult>(items, new QueryStatsInfo(eTag, query.Count(), false)); + return new QueryResult>(items, items.ToQueryStatsInfo(addressCount)); }, cancellationToken); } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/RetryBatchStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/RetryBatchStore.cs index 0bbb8bb714..ed94045e9f 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/RetryBatchStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/RetryBatchStore.cs @@ -6,7 +6,6 @@ namespace ServiceControl.Persistence.EFCore.Implementation; using ServiceControl.Persistence.EFCore.DbContexts; using ServiceControl.Persistence.EFCore.Entities; using ServiceControl.Persistence.EFCore.Infrastructure; -using ServiceControl.Persistence.Infrastructure; public class RetryBatchStore(IServiceScopeFactory scopeFactory, IRetryBatchSqlDialect dialect) : DataStoreBase(scopeFactory), IRetryBatchStore { @@ -99,7 +98,7 @@ static Guid ParseBatchId(string batchId) => ? parsed : throw new ArgumentException($"'{batchId}' is not a retry batch id issued by this store.", nameof(batchId)); - public Task>> GetOrphanedBatches(string retrySessionId, CancellationToken cancellationToken = default) => + public Task GetOrphanedBatches(string retrySessionId, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (dbContext, token) => { var orphaned = await dbContext.RetryBatches @@ -109,9 +108,9 @@ public Task>> GetOrphanedBatches(string retrySessi var messageCounts = await CountMessages(dbContext, [.. orphaned.Select(batch => batch.Id)], token); - IList batches = [.. orphaned.Select(batch => batch.ToRetryBatch(messageCounts.GetValueOrDefault(batch.Id)))]; + IReadOnlyList batches = [.. orphaned.Select(batch => batch.ToRetryBatch(messageCounts.GetValueOrDefault(batch.Id)))]; - return new QueryResult>(batches, new QueryStatsInfo(string.Empty, batches.Count, false)); + return OrphanedBatches.Complete(batches); }, cancellationToken); public Task> GetAvailableBatchGroups(CancellationToken cancellationToken = default) => diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/RetryHistoryDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/RetryHistoryDataStore.cs index 1b94be8b5a..17db0a9281 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/RetryHistoryDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/RetryHistoryDataStore.cs @@ -4,11 +4,13 @@ namespace ServiceControl.Persistence.EFCore.Implementation; using Microsoft.Extensions.DependencyInjection; using ServiceControl.Persistence.EFCore.DbContexts; using ServiceControl.Persistence.EFCore.Entities; +using ServiceControl.Persistence.EFCore.Infrastructure; +using ServiceControl.Persistence.Infrastructure; using ServiceControl.Recoverability; public class RetryHistoryDataStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), IRetryHistoryDataStore { - public Task GetRetryHistory(CancellationToken cancellationToken = default) => + public Task> GetRetryHistory(CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (dbContext, token) => { var historicOperations = await dbContext.HistoricRetryOperations @@ -29,6 +31,10 @@ public Task GetRetryHistory(CancellationToken cancellationToken = var unacknowledgedOperations = await dbContext.UnacknowledgedRetryOperations .AsNoTracking() + // By the primary key, so the order is total. Without it the rows arrive in whatever order + // the server happens to produce, which leaves the body unstable under a stable validator. + .OrderBy(operation => operation.RequestId) + .ThenBy(operation => operation.RetryType) .Select(operation => new UnacknowledgedRetryOperation { RequestId = operation.RequestId, @@ -43,11 +49,13 @@ public Task GetRetryHistory(CancellationToken cancellationToken = }) .ToListAsync(token); - return new RetryHistory + var history = new RetryHistory { HistoricOperations = historicOperations, UnacknowledgedOperations = unacknowledgedOperations }; + + return new QueryResult(history, history.ToQueryStatsInfo()); }, cancellationToken); public Task RecordRetryOperationCompleted(string requestId, RetryType retryType, DateTime startTime, DateTime completionTime, diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/FailedMessageQueryFilters.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/FailedMessageQueryFilters.cs index 7a28dac79e..fbf47b97cc 100644 --- a/src/ServiceControl.Persistence.EFCore/Infrastructure/FailedMessageQueryFilters.cs +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/FailedMessageQueryFilters.cs @@ -1,7 +1,6 @@ namespace ServiceControl.Persistence.EFCore.Infrastructure; using System.Globalization; -using Microsoft.EntityFrameworkCore; using ServiceControl.MessageFailures; using ServiceControl.Persistence.EFCore.Entities; using ServiceControl.Persistence.Infrastructure; @@ -166,19 +165,6 @@ public static IQueryable SortMessages(this IQueryable Page(this IQueryable source, PagingInfo pagingInfo) => source.Skip(pagingInfo.Offset).Take(pagingInfo.Next); - public static async Task ToQueryStatsInfo(this IQueryable source, CancellationToken cancellationToken = default) - { - var stats = await source - .GroupBy(_ => 1) - .Select(group => new { Count = group.Count(), Latest = group.Max(message => (DateTime?)message.LastModified) }) - .SingleOrDefaultAsync(cancellationToken); - - var count = stats?.Count ?? 0; - var latest = stats?.Latest ?? DateTime.MinValue; - - return new QueryStatsInfo($"{count}-{latest.Ticks}", count, false); - } - static IOrderedQueryable OrderBy(this IQueryable source, System.Linq.Expressions.Expression> keySelector, bool descending) => descending ? source.OrderByDescending(keySelector).ThenByDescending(message => message.UniqueMessageId) diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/FailureGroupQueries.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/FailureGroupQueries.cs index 0767a7d4b2..3c21c23ece 100644 --- a/src/ServiceControl.Persistence.EFCore/Infrastructure/FailureGroupQueries.cs +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/FailureGroupQueries.cs @@ -1,7 +1,6 @@ namespace ServiceControl.Persistence.EFCore.Infrastructure; using ServiceControl.Persistence.EFCore.Entities; -using ServiceControl.Persistence.Infrastructure; using ServiceControl.Recoverability; static class FailureGroupQueries @@ -22,11 +21,4 @@ into aggregate First = aggregate.Min(message => message.FirstTimeOfFailure), Last = aggregate.Max(message => message.LastTimeOfFailure) }; - - public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection groups) - { - var latest = groups.Count == 0 ? DateTime.MinValue : groups.Max(group => group.Last); - - return new QueryStatsInfo($"{groups.Count}-{latest.Ticks}", groups.Count, false); - } } diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/QueryStatsInfoExtensions.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/QueryStatsInfoExtensions.cs new file mode 100644 index 0000000000..48c9d2dff7 --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/QueryStatsInfoExtensions.cs @@ -0,0 +1,60 @@ +namespace ServiceControl.Persistence.EFCore.Infrastructure; + +using Microsoft.EntityFrameworkCore; +using ServiceControl.Contracts.CustomChecks; +using ServiceControl.EventLog; +using ServiceControl.MessageFailures; +using ServiceControl.Persistence.EFCore.Entities; +using ServiceControl.Persistence.Infrastructure; +using ServiceControl.Recoverability; + +static class QueryStatsInfoExtensions +{ + public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection items, long totalCount) => + new QueryStatsInfo( + DataVersion.OverRows( + [("checks", totalCount)], + items, + check => [check.Id, check.CustomCheckId, check.Category, check.Status, check.ReportedAt, check.FailureReason]), + totalCount); + + public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection items, long totalCount) => + new QueryStatsInfo( + DataVersion.OverRows( + [("items", totalCount)], + items, + item => [item.Id, item.Description, item.Severity, item.RaisedAt, item.Category, item.EventType]), + totalCount); + + public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection items, long totalCount) => + new QueryStatsInfo( + DataVersion.OverRows( + [("messages", totalCount)], + items, + row => [row.UniqueMessageId, row.LastModified, row.Status, row.NumberOfProcessingAttempts]), + totalCount); + + // Used in a HEAD with no body. Total-Count is the whole response, so the count is the whole version. + public static async Task ToCountQueryStatsInfo(this IQueryable source, string name, CancellationToken cancellationToken = default) + { + var count = await source.LongCountAsync(cancellationToken); + + return new QueryStatsInfo(DataVersion.Compose([(name, count)]), count); + } + + public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection groups) => + new QueryStatsInfo( + DataVersion.OverRows( + [("groups", groups.Count)], + groups, + group => [group.Id, group.Title, group.Type, group.Count, group.Comment, group.First, group.Last]), + groups.Count); + + public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection items, long totalCount) => + new QueryStatsInfo( + DataVersion.OverRows( + [("addresses", totalCount)], + items, + address => [address.PhysicalAddress, address.FailedMessageCount]), + totalCount); +} diff --git a/src/ServiceControl.Persistence.EFCore/Infrastructure/RetryHistoryQueries.cs b/src/ServiceControl.Persistence.EFCore/Infrastructure/RetryHistoryQueries.cs new file mode 100644 index 0000000000..881aa7bd1f --- /dev/null +++ b/src/ServiceControl.Persistence.EFCore/Infrastructure/RetryHistoryQueries.cs @@ -0,0 +1,35 @@ +namespace ServiceControl.Persistence.EFCore.Infrastructure; + +using ServiceControl.Persistence.Infrastructure; +using ServiceControl.Recoverability; + +static class RetryHistoryQueries +{ + /// + /// Every field of every operation in both collections, plus each collection's count. + /// + public static QueryStatsInfo ToQueryStatsInfo(this RetryHistory history) => + new QueryStatsInfo(DataVersion.OverRows( + [("historic", history.HistoricOperations.Count), ("unacknowledged", history.UnacknowledgedOperations.Count)], + Rows(history), + row => row), + history.HistoricOperations.Count); + + static IEnumerable Rows(RetryHistory history) + { + // The leading marker keeps a historic row from ever digesting the same as an unacknowledged one. + foreach (var operation in history.HistoricOperations) + { + yield return ["historic", operation.RequestId, operation.RetryType, operation.StartTime, operation.CompletionTime, + operation.Originator, operation.Failed, operation.NumberOfMessagesProcessed]; + } + + // Rows are named by position, so both collections have to arrive in a deterministic order. Each is + // ordered by its query in RetryHistoryDataStore, historic by completion time and these by their key. + foreach (var operation in history.UnacknowledgedOperations) + { + yield return ["unacknowledged", operation.RequestId, operation.RetryType, operation.StartTime, operation.CompletionTime, + operation.Last, operation.Originator, operation.Classifier, operation.Failed, operation.NumberOfMessagesProcessed]; + } + } +} diff --git a/src/ServiceControl.Persistence.RavenDB/ErrorMessagesDataStore.cs b/src/ServiceControl.Persistence.RavenDB/ErrorMessagesDataStore.cs index a381261bd6..1ba1c9a5d5 100644 --- a/src/ServiceControl.Persistence.RavenDB/ErrorMessagesDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/ErrorMessagesDataStore.cs @@ -506,5 +506,7 @@ public async Task GetRetryPendingMessages(DateTime from, DateTime to, } record struct FailedMessageProjection(string UniqueMessageId); + + } } diff --git a/src/ServiceControl.Persistence.RavenDB/EventLogDataStore.cs b/src/ServiceControl.Persistence.RavenDB/EventLogDataStore.cs index e02ae48522..da0940a168 100644 --- a/src/ServiceControl.Persistence.RavenDB/EventLogDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/EventLogDataStore.cs @@ -29,7 +29,7 @@ await session.StoreAsync( } public async Task>> GetEventLogItems( - PagingInfo pagingInfo, string knownVersion = null, CancellationToken cancellationToken = default) + PagingInfo pagingInfo, CancellationToken cancellationToken = default) { using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken); var documents = await session @@ -41,13 +41,6 @@ public async Task>> GetEventLogItems( var queryStats = stats.ToQueryStatsInfo(); - // The validator comes off the query statistics, so the page cannot be - // skipped. Only the projection below is saved. - if (knownVersion is not null && knownVersion == queryStats.ETag) - { - return QueryResult>.Unchanged(queryStats); - } - // The id lives in document metadata rather than on the document var items = documents.ConvertAll(document => new EventLogItemView { diff --git a/src/ServiceControl.Persistence.RavenDB/Extensions/QueryResultConvert.cs b/src/ServiceControl.Persistence.RavenDB/Extensions/QueryResultConvert.cs index f9ea464930..230bb5297a 100644 --- a/src/ServiceControl.Persistence.RavenDB/Extensions/QueryResultConvert.cs +++ b/src/ServiceControl.Persistence.RavenDB/Extensions/QueryResultConvert.cs @@ -7,9 +7,7 @@ static class QueryResultConvert { public static QueryResult> ToQueryResult(this IList result, QueryStatistics stats) - where T : class - { - return new QueryResult>(result, stats.ToQueryStatsInfo()); - } + where T : class => + new(result, stats.ToQueryStatsInfo()); } -} \ No newline at end of file +} diff --git a/src/ServiceControl.Persistence.RavenDB/RavenAttachmentsBodyStorage.cs b/src/ServiceControl.Persistence.RavenDB/RavenAttachmentsBodyStorage.cs index 69a498fac0..98772b9496 100644 --- a/src/ServiceControl.Persistence.RavenDB/RavenAttachmentsBodyStorage.cs +++ b/src/ServiceControl.Persistence.RavenDB/RavenAttachmentsBodyStorage.cs @@ -4,6 +4,7 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; + using Persistence.Infrastructure; using Persistence.RavenDB; using Raven.Client.Documents; using Raven.Client.Documents.Session; @@ -72,7 +73,7 @@ async Task ResultForUniqueId(IAsyncDocumentSession session, s result.Stream, result.Details.ContentType, (int)result.Details.Size, - result.Details.ChangeVector)); + DataVersion.FromToken(result.Details.ChangeVector))); } } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs b/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs index f4e6e643d7..5ae267faf8 100644 --- a/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs @@ -57,7 +57,7 @@ public async Task>> GetStats(PagingInfo paging, s .Paging(paging) .ToListAsync(cancellationToken); - return new QueryResult>(results, new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults, stats.IsStale)); + return new QueryResult>(results, stats.ToQueryStatsInfo()); } public async Task DeleteCustomCheck(Guid id, CancellationToken cancellationToken = default) diff --git a/src/ServiceControl.Persistence.RavenDB/RavenQueryStatisticsExtensions.cs b/src/ServiceControl.Persistence.RavenDB/RavenQueryStatisticsExtensions.cs index fa5746ccfa..7e58d3ecdb 100644 --- a/src/ServiceControl.Persistence.RavenDB/RavenQueryStatisticsExtensions.cs +++ b/src/ServiceControl.Persistence.RavenDB/RavenQueryStatisticsExtensions.cs @@ -1,18 +1,16 @@ -namespace ServiceControl.Persistence +namespace ServiceControl.Persistence { using Raven.Client.Documents.Session; using ServiceControl.Persistence.Infrastructure; static class RavenQueryStatisticsExtensions { - public static QueryStatsInfo ToQueryStatsInfo(this QueryStatistics stats) - { - return new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults, stats.IsStale); - } + public static QueryStatsInfo ToQueryStatsInfo(this QueryStatistics stats) => + new(stats.ResultEtag is { } resultEtag ? DataVersion.FromToken(resultEtag) : DataVersion.None, + stats.TotalResults); - public static QueryStatsInfo ToQueryStatsInfo(this Raven.Client.Documents.Queries.QueryResult queryResult) - { - return new QueryStatsInfo(queryResult.ResultEtag.ToString(), queryResult.TotalResults, queryResult.IsStale); - } + public static QueryStatsInfo ToQueryStatsInfo(this Raven.Client.Documents.Queries.QueryResult queryResult) => + new(DataVersion.FromToken(queryResult.ResultEtag), + queryResult.TotalResults); } -} \ No newline at end of file +} diff --git a/src/ServiceControl.Persistence.RavenDB/Recoverability/GroupsDataStore.cs b/src/ServiceControl.Persistence.RavenDB/Recoverability/GroupsDataStore.cs index a1bbfcea58..d8f97782ad 100644 --- a/src/ServiceControl.Persistence.RavenDB/Recoverability/GroupsDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/Recoverability/GroupsDataStore.cs @@ -40,11 +40,12 @@ public async Task> GetUnresolvedGroupsByClassifier(strin return groups; } - public async Task> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default) + public async Task>> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default) { using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken); var groups = session .Query() + .Statistics(out var stats) .Where(v => v.Type == classifier); var results = await groups @@ -52,7 +53,7 @@ public async Task> GetArchivedGroupsByClassifier(string .Take(200) // only show 200 groups .ToListAsync(cancellationToken); - return results; + return new QueryResult>(results, stats.ToQueryStatsInfo()); } public async Task> GetUnresolvedGroup(string groupId, string status, string modified, CancellationToken cancellationToken = default) diff --git a/src/ServiceControl.Persistence.RavenDB/Recoverability/RetryHistoryDataStore.cs b/src/ServiceControl.Persistence.RavenDB/Recoverability/RetryHistoryDataStore.cs index b93e1c8df8..54685d77f7 100644 --- a/src/ServiceControl.Persistence.RavenDB/Recoverability/RetryHistoryDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/Recoverability/RetryHistoryDataStore.cs @@ -3,20 +3,32 @@ using System; using System.Threading; using System.Threading.Tasks; + using ServiceControl.Persistence.Infrastructure; using ServiceControl.Recoverability; class RetryHistoryDataStore(IRavenSessionProvider sessionProvider) : IRetryHistoryDataStore { const string DocumentId = "RetryOperations/History"; - public async Task GetRetryHistory(CancellationToken cancellationToken = default) + // Before the first operation completes there is no document and so no change vector, but an + // empty history still has to be cacheable. + static readonly DataVersion EmptyHistory = DataVersion.FromToken("no-retry-history"); + + public async Task> GetRetryHistory(CancellationToken cancellationToken = default) { using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken); var retryHistory = await session.LoadAsync(DocumentId, cancellationToken); + // GetChangeVectorFor throws for an entity the session is not tracking, so this relies on the + // session provider's default. Opening this one with NoTracking would turn the endpoint into a 500. + var version = retryHistory == null + ? EmptyHistory + : DataVersion.FromToken(session.Advanced.GetChangeVectorFor(retryHistory)); + retryHistory ??= new(); - return retryHistory; + return new QueryResult(retryHistory, + new QueryStatsInfo(version, retryHistory.HistoricOperations.Count)); } public async Task RecordRetryOperationCompleted(string requestId, RetryType retryType, DateTime startTime, DateTime completionTime, diff --git a/src/ServiceControl.Persistence.RavenDB/RetryDocumentDataStore.cs b/src/ServiceControl.Persistence.RavenDB/RetryDocumentDataStore.cs index 162565009e..3b67232127 100644 --- a/src/ServiceControl.Persistence.RavenDB/RetryDocumentDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/RetryDocumentDataStore.cs @@ -85,17 +85,16 @@ await session.StoreAsync(new RetryBatch return batchId; } - public async Task>> GetOrphanedBatches(string retrySessionId, CancellationToken cancellationToken = default) + public async Task GetOrphanedBatches(string retrySessionId, CancellationToken cancellationToken = default) { using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken); var orphanedBatches = await session .Query() - .Where(b => b.Status == RetryBatchStatus.MarkingDocuments && b.RetrySessionId != retrySessionId) .Statistics(out var stats) .ToListAsync(cancellationToken); - return orphanedBatches.Select(batch => batch.ToContract()).ToList().ToQueryResult(stats); + return new OrphanedBatches([.. orphanedBatches.Select(batch => batch.ToContract())], stats.IsStale); } public async Task> GetAvailableBatchGroups(CancellationToken cancellationToken = default) diff --git a/src/ServiceControl.Persistence.Tests.InMemory/PersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests.InMemory/PersistenceTestsContext.cs index e830dd8bbd..3f4a82dd7a 100644 --- a/src/ServiceControl.Persistence.Tests.InMemory/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.InMemory/PersistenceTestsContext.cs @@ -11,6 +11,7 @@ public class PersistenceTestsContext : IPersistenceTestsContext public PersistenceSettings PersistenceSettings { get; private set; } public string GenerateFailedMessageRecordId(string messageId) => throw new System.NotImplementedException(); public Task InsertFailedMessages(params FailedMessage[] messages) => throw new System.NotImplementedException(); + public void AdvanceClock(System.TimeSpan by) => throw new System.NotImplementedException(); public Task Setup(IHostApplicationBuilder hostBuilder) { diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs index 30bc5ae750..9fccdd71d6 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/PersistenceTestsContext.cs @@ -21,6 +21,8 @@ public partial class PersistenceTestsContext : IPersistenceTestsContext string databaseName; string bodyStoragePath; + public void AdvanceClock(TimeSpan by) => FakeTime.Advance(by); + public async Task Setup(IHostApplicationBuilder hostBuilder) { databaseName = $"sc_test_{Guid.NewGuid():n}"; diff --git a/src/ServiceControl.Persistence.Tests.RavenDB/PersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests.RavenDB/PersistenceTestsContext.cs index 13ae79e717..b973581ef9 100644 --- a/src/ServiceControl.Persistence.Tests.RavenDB/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.RavenDB/PersistenceTestsContext.cs @@ -81,6 +81,12 @@ public async Task InsertFailedMessages(params FailedMessage[] messages) public IRavenSessionProvider SessionProvider { get; private set; } + // Nothing to do: Raven versions come from document and index etags, which move on every write, so + // no test needs to push its clock. There is no hook for the server clock anyway. + public void AdvanceClock(TimeSpan by) + { + } + public Task CompleteDatabaseOperation() { DocumentStore.WaitForIndexing(); diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs index 5ebccb8177..e1ba34cf34 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests.SqlServer/PersistenceTestsContext.cs @@ -20,6 +20,8 @@ public partial class PersistenceTestsContext : IPersistenceTestsContext string databaseName; string bodyStoragePath; + public void AdvanceClock(TimeSpan by) => FakeTime.Advance(by); + public async Task Setup(IHostApplicationBuilder hostBuilder) { databaseName = $"sc_test_{Guid.NewGuid():n}"; diff --git a/src/ServiceControl.Persistence.Tests/BodyStorage/BodyVersionTests.cs b/src/ServiceControl.Persistence.Tests/BodyStorage/BodyVersionTests.cs new file mode 100644 index 0000000000..34ab459369 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/BodyStorage/BodyVersionTests.cs @@ -0,0 +1,100 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.IO; +using System.Text; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.Operations.BodyStorage; +using ServiceControl.Persistence.Infrastructure; + +[TestFixture] +class BodyVersionTests : IngestionTestBase +{ + static readonly DateTime FirstAttempt = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Utc); + + [Test] + public async Task Version_changes_when_a_later_attempt_carries_a_different_body() + { + var first = Failure("the original body", FirstAttempt); + + await Ingest(first); + await CompleteDatabaseOperation(); + + var (originalBody, before) = await Fetch(first.UniqueMessageIdString); + + // The same message fails again with a different body. Ingestion updates the existing row + // rather than adding one, so the message id is unchanged and cannot serve as a version. + AdvanceClock(TimeSpan.FromHours(8)); + await Ingest(Failure("a completely different body", FirstAttempt.AddHours(8), first)); + await CompleteDatabaseOperation(); + + var (replacedBody, after) = await Fetch(first.UniqueMessageIdString); + + using (Assert.EnterMultipleScope()) + { + Assert.That(originalBody, Is.EqualTo("the original body")); + Assert.That(replacedBody, Is.EqualTo("a completely different body"), "the stored body was replaced"); + Assert.That(before.HasValue, Is.True, "there was no version to move"); + Assert.That(after.Matches(before), Is.False, + "the body changed, so a client holding the old version must be sent the new body rather than a 304"); + } + } + + [Test] + public async Task Version_is_stable_while_the_body_is_not_rewritten() + { + var failure = Failure("the original body", FirstAttempt); + + await Ingest(failure); + await CompleteDatabaseOperation(); + + var (_, first) = await Fetch(failure.UniqueMessageIdString); + var (_, second) = await Fetch(failure.UniqueMessageIdString); + + Assert.That(second.Matches(first), Is.True, + "two reads with no write between them must let a client revalidate successfully"); + } + + [Test] + public async Task A_message_with_no_body_reads_back_as_empty_on_every_backend() + { + var failure = Failure(string.Empty, FirstAttempt); + + await Ingest(failure); + await CompleteDatabaseOperation(); + + var result = await BodyStorage.TryFetch(failure.UniqueMessageIdString); + + // Empty carries no content and so no version, whatever this task does. What is worth pinning + // is that all persistence seams agree it is Empty rather than NotFound or Unavailable, which is + // what decides whether the caller gets a no-body response or a 404. + Assert.That(result.State, Is.EqualTo(MessageBodyState.Empty)); + } + + async Task<(string Body, DataVersion Version)> Fetch(string bodyId) + { + var result = await BodyStorage.TryFetch(bodyId); + + Assert.That(result.State, Is.EqualTo(MessageBodyState.Available)); + + await using var stream = result.Content.Stream; + using var reader = new StreamReader(stream, Encoding.UTF8); + + return (await reader.ReadToEndAsync(), result.Content.Version); + } + + static IngestedFailure Failure(string body, DateTime attemptedAt, IngestedFailure sameMessageAs = null) + { + var identity = sameMessageAs ?? new IngestedFailure(); + + return new IngestedFailure + { + MessageId = identity.MessageId, + EndpointName = identity.EndpointName, + Body = Encoding.UTF8.GetBytes(body), + AttemptedAt = attemptedAt, + TimeOfFailure = attemptedAt + }; + } +} diff --git a/src/ServiceControl.Persistence.Tests/CustomCheckVersionTests.cs b/src/ServiceControl.Persistence.Tests/CustomCheckVersionTests.cs new file mode 100644 index 0000000000..3654501e64 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/CustomCheckVersionTests.cs @@ -0,0 +1,127 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Linq; +using System.Threading.Tasks; +using Contracts.CustomChecks; +using NUnit.Framework; +using ServiceControl.Operations; +using ServiceControl.Persistence.Infrastructure; + +[TestFixture] +class CustomCheckVersionTests : PersistenceTestBase +{ + static readonly DateTime ReportedAt = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Utc); + + [Test] + public async Task Version_changes_when_a_check_starts_failing() + { + await Report("Disk space", hasFailed: false); + + var before = await CustomChecks.GetStats(new PagingInfo()); + + await Report("Disk space", hasFailed: true); + + var after = await CustomChecks.GetStats(new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(1), "still one check"); + Assert.That(after.Results[0].Status, Is.EqualTo(Status.Fail), "and the body now reports it failing"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body changed, so the validator must too, or a revalidating client is shown a stale status"); + } + } + + [Test] + public async Task Version_changes_when_a_new_check_appears() + { + await Report("Disk space", hasFailed: false); + + var before = await CustomChecks.GetStats(new PagingInfo()); + + await Report("Queue length", hasFailed: false); + + var after = await CustomChecks.GetStats(new PagingInfo()); + + VersionAssert.Moved(before.QueryStats.Version, after.QueryStats.Version, + "a check appeared, so a revalidating client must not be told its page is current"); + } + + [Test] + public async Task Version_is_stable_while_nothing_changes() + { + await Report("Disk space", hasFailed: false); + + var first = await CustomChecks.GetStats(new PagingInfo()); + var second = await CustomChecks.GetStats(new PagingInfo()); + + VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version, + "nothing changed, so the validator has to stay put or conditional GET never pays off"); + } + + [Test] + public async Task An_empty_store_still_reports_a_version() + { + var result = await CustomChecks.GetStats(new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(result.Results, Is.Empty); + Assert.That(result.QueryStats.Version.HasValue, Is.True, + "an empty list is a representation like any other and has to be cacheable"); + } + } + + [Test] + public async Task Version_changes_when_the_reporting_endpoint_changes_under_an_unchanged_count() + { + var first = await Report("Disk space", hasFailed: false); + + var before = await CustomChecks.GetStats(new PagingInfo()); + + // OriginatingEndpoint has no version term of its own. It is covered because the row id is a + // deterministic hash of it, so swapping the endpoint swaps the id while everything else, the + // count included, stays exactly where it was. + await CustomChecks.DeleteCustomCheck(first); + await Report("Disk space", hasFailed: false, endpointName: "other-host"); + + var after = await CustomChecks.GetStats(new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(1), "still one check"); + Assert.That(after.QueryStats.TotalCount, Is.EqualTo(before.QueryStats.TotalCount), "and the total has not moved"); + Assert.That(after.Results.Single().Id, Is.Not.EqualTo(before.Results.Single().Id), "but it is a different check"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body reports a check from a different endpoint, so the validator has to move"); + } + } + + // Returns the row's deterministic id, which the backends could render differently. + async Task Report(string customCheckId, bool hasFailed, string endpointName = "test-host") + { + var detail = new CustomCheckDetail + { + Category = "test-category", + CustomCheckId = customCheckId, + HasFailed = hasFailed, + FailureReason = hasFailed ? "Testing" : null, + ReportedAt = ReportedAt, + OriginatingEndpoint = new EndpointDetails + { + Host = "localhost", + HostId = Guid.Parse("55D0800D-CC90-47C3-83EB-DDE292140C28"), + Name = endpointName + } + }; + + await CustomChecks.UpdateCustomCheckStatus(detail); + + await CompleteDatabaseOperation(); + + return detail.GetDeterministicId(); + } +} diff --git a/src/ServiceControl.Persistence.Tests/CustomChecksDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/CustomChecksDataStoreTests.cs index 04818e0417..364ed83b41 100644 --- a/src/ServiceControl.Persistence.Tests/CustomChecksDataStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/CustomChecksDataStoreTests.cs @@ -41,6 +41,40 @@ public async Task CustomChecks_load_from_data_store() } } + [Test] + public async Task Reported_endpoint_survives_a_round_trip() + { + await CustomChecks.UpdateCustomCheckStatus(new CustomCheckDetail + { + Category = "test-category", + CustomCheckId = "Test-Check", + HasFailed = true, + FailureReason = "Testing", + OriginatingEndpoint = new EndpointDetails + { + Host = "localhost", + HostId = Guid.Parse("55D0800D-CC90-47C3-83EB-DDE292140C28"), + Name = "test-host" + }, + }); + + await CompleteDatabaseOperation(); + + var stats = await CustomChecks.GetStats(new PagingInfo()); + + Assert.That(stats.Results, Has.Count.EqualTo(1)); + + var reported = stats.Results[0].OriginatingEndpoint; + + using (Assert.EnterMultipleScope()) + { + Assert.That(reported, Is.Not.Null, "the check came back with no reporting endpoint, so the page that renders its name cannot draw it"); + Assert.That(reported?.Name, Is.EqualTo("test-host")); + Assert.That(reported?.Host, Is.EqualTo("localhost")); + Assert.That(reported?.HostId, Is.EqualTo(Guid.Parse("55D0800D-CC90-47C3-83EB-DDE292140C28"))); + } + } + [Test] public async Task Storing_failed_custom_checks_returns_unchanged() { diff --git a/src/ServiceControl.Persistence.Tests/EFCore/ErrorIngestionTestBase.cs b/src/ServiceControl.Persistence.Tests/EFCore/ErrorIngestionTestBase.cs index e269c94761..24eaa2b615 100644 --- a/src/ServiceControl.Persistence.Tests/EFCore/ErrorIngestionTestBase.cs +++ b/src/ServiceControl.Persistence.Tests/EFCore/ErrorIngestionTestBase.cs @@ -14,7 +14,7 @@ namespace ServiceControl.Persistence.Tests; using ServiceControl.Persistence.EFCore.Infrastructure; using ServiceControl.Persistence.UnitOfWork; -abstract class ErrorIngestionTestBase : PersistenceTestBase +abstract class ErrorIngestionTestBase : IngestionTestBase { protected ErrorIngestionTestBase() => RegisterServices = services => services.AddSingleton(RecordedBodies); @@ -23,35 +23,6 @@ protected ErrorIngestionTestBase() => protected EFPersisterSettings EFSettings => (EFPersisterSettings)PersistenceSettings; - protected void AdvanceClock(TimeSpan by) => PersistenceTestsContext.FakeTime.Advance(by); - - protected async Task InBatch(Func record) - { - await using var unitOfWork = await UnitOfWorkFactory.StartNew(); - - await record(unitOfWork); - - await unitOfWork.Complete(TestContext.CurrentContext.CancellationToken); - } - - protected Task Ingest(params IngestedFailure[] failures) => - InBatch(async unitOfWork => - { - foreach (var failure in failures) - { - await unitOfWork.Recoverability.RecordFailedProcessingAttempt(failure.Context, failure.ProcessingAttempt, failure.Groups); - } - }); - - protected Task ConfirmRetry(params string[] uniqueMessageIds) => - InBatch(async unitOfWork => - { - foreach (var uniqueMessageId in uniqueMessageIds) - { - await unitOfWork.Recoverability.RecordSuccessfulRetry(uniqueMessageId); - } - }); - protected async Task GetFailedMessage(Guid uniqueMessageId) { var row = await Query(dbContext => dbContext.FailedMessages.AsNoTracking().SingleOrDefaultAsync(m => m.UniqueMessageId == uniqueMessageId)); diff --git a/src/ServiceControl.Persistence.Tests/EFCore/EventLogDataStoreEFTests.cs b/src/ServiceControl.Persistence.Tests/EFCore/EventLogDataStoreEFTests.cs index 34b98aada7..7516020d17 100644 --- a/src/ServiceControl.Persistence.Tests/EFCore/EventLogDataStoreEFTests.cs +++ b/src/ServiceControl.Persistence.Tests/EFCore/EventLogDataStoreEFTests.cs @@ -89,7 +89,7 @@ public async Task Version_changes_when_rows_are_deleted_behind_the_interface() using (Assert.EnterMultipleScope()) { Assert.That(after.QueryStats.TotalCount, Is.EqualTo(2)); - Assert.That(after.QueryStats.ETag, Is.Not.EqualTo(before.QueryStats.ETag), "a delete must invalidate the client's cached page"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, "a delete must invalidate the client's cached page"); } } @@ -120,7 +120,7 @@ public async Task Version_changes_when_a_retention_delete_and_a_backdated_insert { Assert.That(after.QueryStats.TotalCount, Is.EqualTo(before.QueryStats.TotalCount), "the setup only bites while the count is unchanged"); Assert.That(after.Results.Max(i => i.RaisedAt), Is.EqualTo(before.Results.Max(i => i.RaisedAt)), "and while the newest RaisedAt is unchanged"); - Assert.That(after.QueryStats.ETag, Is.Not.EqualTo(before.QueryStats.ETag), "the page changed, so the validator must too"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, "the page changed, so the validator must too"); } } diff --git a/src/ServiceControl.Persistence.Tests/EFCore/RetentionSweepTests.cs b/src/ServiceControl.Persistence.Tests/EFCore/RetentionSweepTests.cs index 35aa3d930a..6b5cf6e93f 100644 --- a/src/ServiceControl.Persistence.Tests/EFCore/RetentionSweepTests.cs +++ b/src/ServiceControl.Persistence.Tests/EFCore/RetentionSweepTests.cs @@ -250,7 +250,7 @@ public async Task Sweeping_event_log_items_changes_the_version() await Store(EventLogRow("expired", Now.AddDays(-15))); await Store(EventLogRow("fresh", Now.AddDays(-1))); - var versionBefore = (await EventLogDataStore.GetEventLogItems(new PagingInfo())).QueryStats.ETag; + var versionBefore = (await EventLogDataStore.GetEventLogItems(new PagingInfo())).QueryStats.Version; await RunRetentionSweep(); @@ -260,7 +260,26 @@ public async Task Sweeping_event_log_items_changes_the_version() { Assert.That(after.QueryStats.TotalCount, Is.EqualTo(1)); // The count term of the version exists precisely so that retention invalidates client caches. - Assert.That(after.QueryStats.ETag, Is.Not.EqualTo(versionBefore)); + Assert.That(after.QueryStats.Version.Matches(versionBefore), Is.False); + } + } + + [Test] + public async Task Sweeping_failed_messages_changes_the_version() + { + await SeedFailedMessage(FailedMessageStatus.Archived, Now.AddDays(-31)); + await SeedFailedMessage(FailedMessageStatus.Archived, Now.AddDays(-1)); + + var versionBefore = (await FailedMessageQueryStore.GetFailedMessagesStats(null, null, null)).Version; + + await RunRetentionSweep(); + + var after = await FailedMessageQueryStore.GetFailedMessagesStats(null, null, null); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.TotalCount, Is.EqualTo(1)); + Assert.That(after.Version.Matches(versionBefore), Is.False); } } diff --git a/src/ServiceControl.Persistence.Tests/EFCore/RetryBatchStoreTests.cs b/src/ServiceControl.Persistence.Tests/EFCore/RetryBatchStoreTests.cs index 6ab2896ae8..6c17ca5584 100644 --- a/src/ServiceControl.Persistence.Tests/EFCore/RetryBatchStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/EFCore/RetryBatchStoreTests.cs @@ -44,7 +44,7 @@ public async Task Does_not_report_batches_of_the_current_session_as_orphaned() var orphaned = await RetryBatchStore.GetOrphanedBatches(OtherSession); - Assert.That(orphaned.Results, Is.Empty); + Assert.That(orphaned.Batches, Is.Empty); } [Test] @@ -225,7 +225,7 @@ [.. Enumerable.Range(0, messageCount).Select(_ => Guid.NewGuid().ToString())], Noon, classifier: "Message Type"); - async Task> Orphaned() => (await RetryBatchStore.GetOrphanedBatches(OtherSession)).Results; + async Task> Orphaned() => (await RetryBatchStore.GetOrphanedBatches(OtherSession)).Batches; async Task> ClaimedBy(string batchId) { diff --git a/src/ServiceControl.Persistence.Tests/EFCore/RetryHistoryDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/EFCore/RetryHistoryDataStoreTests.cs index 410c480412..5c7c38e331 100644 --- a/src/ServiceControl.Persistence.Tests/EFCore/RetryHistoryDataStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/EFCore/RetryHistoryDataStoreTests.cs @@ -4,7 +4,6 @@ namespace ServiceControl.Persistence.Tests; using System.Linq; using System.Threading.Tasks; using NUnit.Framework; -using ServiceControl.Recoverability; class RetryHistoryDataStoreTests : ErrorIngestionTestBase { @@ -15,7 +14,7 @@ class RetryHistoryDataStoreTests : ErrorIngestionTestBase [Test] public async Task Returns_an_empty_history_when_nothing_has_completed() { - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; using (Assert.EnterMultipleScope()) { @@ -30,7 +29,7 @@ public async Task Records_a_completed_operation() await RecordCompleted("group-1", originator: "OrderPlaced failures", classifier: "Exception Type and Stack Trace", failed: true, numberOfMessagesProcessed: 3); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; var historic = history.HistoricOperations.Single(); var unacknowledged = history.UnacknowledgedOperations.Single(); @@ -64,7 +63,7 @@ public async Task Returns_the_newest_operations_first() await RecordCompleted("group-2", completionTime: Noon.AddHours(-1)); await RecordCompleted("group-3", completionTime: Noon.AddHours(1)); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; Assert.That(history.HistoricOperations.Select(operation => operation.RequestId), Is.EqualTo(new[] { "group-3", "group-1", "group-2" })); @@ -78,7 +77,7 @@ public async Task Keeps_only_the_newest_operations_up_to_the_depth() await RecordCompleted($"group-{minute}", completionTime: Noon.AddMinutes(minute), depth: 3); } - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; Assert.That(history.HistoricOperations.Select(operation => operation.RequestId), Is.EqualTo(new[] { "group-4", "group-3", "group-2" })); @@ -91,7 +90,7 @@ public async Task Breaks_ties_on_completion_time_by_the_order_recorded() await RecordCompleted("group-2", completionTime: Noon, depth: 2); await RecordCompleted("group-3", completionTime: Noon, depth: 2); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; Assert.That(history.HistoricOperations.Select(operation => operation.RequestId), Is.EqualTo(new[] { "group-3", "group-2" })); @@ -107,7 +106,7 @@ public async Task Applies_a_reduced_depth_to_operations_already_recorded() await RecordCompleted("group-4", completionTime: Noon.AddMinutes(4), depth: 2); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; Assert.That(history.HistoricOperations.Select(operation => operation.RequestId), Is.EqualTo(new[] { "group-4", "group-3" })); @@ -119,7 +118,7 @@ public async Task Keeps_no_history_when_the_depth_is_zero() await RecordCompleted("group-1"); await RecordCompleted("group-2", depth: 0); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; using (Assert.EnterMultipleScope()) { @@ -134,7 +133,7 @@ public async Task Does_not_wait_for_an_acknowledgement_of_message_retries(RetryT { await RecordCompleted("request-1", retryType); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; using (Assert.EnterMultipleScope()) { @@ -149,7 +148,7 @@ public async Task Replaces_the_pending_acknowledgement_when_a_group_is_retried_a await RecordCompleted("group-1", completionTime: Noon, numberOfMessagesProcessed: 3); await RecordCompleted("group-1", completionTime: Noon.AddHours(1), numberOfMessagesProcessed: 7); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; var unacknowledged = history.UnacknowledgedOperations.Single(); @@ -167,7 +166,7 @@ public async Task Keeps_the_pending_acknowledgements_of_other_retry_types_apart( await RecordCompleted("request-1", RetryType.FailureGroup); await RecordCompleted("request-1", RetryType.AllForEndpoint); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; Assert.That(history.UnacknowledgedOperations.Select(operation => operation.RetryType), Is.EquivalentTo(new[] { RetryType.FailureGroup, RetryType.AllForEndpoint })); @@ -180,7 +179,7 @@ public async Task Acknowledges_a_group_retry() var acknowledged = await RetryHistoryStore.AcknowledgeRetryGroup("group-1"); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; using (Assert.EnterMultipleScope()) { @@ -201,7 +200,7 @@ public async Task Does_not_acknowledge_an_operation_of_another_retry_type() var acknowledged = await RetryHistoryStore.AcknowledgeRetryGroup("SomeEndpoint"); - var history = await RetryHistoryStore.GetRetryHistory(); + var history = (await RetryHistoryStore.GetRetryHistory()).Results; using (Assert.EnterMultipleScope()) { @@ -210,6 +209,20 @@ public async Task Does_not_acknowledge_an_operation_of_another_retry_type() } } + [Test] + public async Task Unacknowledged_operations_are_read_in_key_order() + { + await RecordCompleted("group-c"); + await RecordCompleted("group-a"); + await RecordCompleted("group-b"); + await CompleteDatabaseOperation(); + + var history = (await RetryHistoryStore.GetRetryHistory()).Results; + + Assert.That(history.UnacknowledgedOperations.Select(operation => operation.RequestId), + Is.EqualTo(new[] { "group-a", "group-b", "group-c" })); + } + Task RecordCompleted(string requestId, RetryType retryType = RetryType.FailureGroup, DateTime? completionTime = null, string originator = "OrderPlaced failures", string classifier = "Exception Type and Stack Trace", bool failed = false, int numberOfMessagesProcessed = 1, int depth = DefaultDepth) diff --git a/src/ServiceControl.Persistence.Tests/EventLogDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/EventLogDataStoreTests.cs index 5b955d05d2..02c9481eda 100644 --- a/src/ServiceControl.Persistence.Tests/EventLogDataStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/EventLogDataStoreTests.cs @@ -96,18 +96,6 @@ public async Task Empty_store_returns_no_items() } } - [Test] - public async Task Empty_store_is_a_page_of_nothing_rather_than_not_modified() - { - var result = await EventLogDataStore.GetEventLogItems(new PagingInfo()); - - using (Assert.EnterMultipleScope()) - { - Assert.That(result.NotModified, Is.False, "an empty store still has a representation to return"); - Assert.That(result.Results, Is.Not.Null); - } - } - [Test] public async Task Page_size_limits_returned_items_but_not_the_total() { @@ -188,73 +176,8 @@ public async Task Version_is_stable_while_nothing_changes() Assert.That(secondRead, Is.EqualTo(firstRead)); } - [Test] - public async Task Matching_known_version_reports_not_modified() - { - await AddItems(3); - var version = await CurrentVersion(); - - var result = await EventLogDataStore.GetEventLogItems(new PagingInfo(), version); - - using (Assert.EnterMultipleScope()) - { - Assert.That(result.NotModified, Is.True, "a caller already holding the current version must be told so, not handed the page again"); - Assert.That(result.Results, Is.Null, "a not-modified result carries no page"); - } - } - - [Test] - public async Task Matching_known_version_still_reports_total_and_version() - { - await AddItems(3); - var version = await CurrentVersion(); - - var result = await EventLogDataStore.GetEventLogItems(new PagingInfo(), version); - - using (Assert.EnterMultipleScope()) - { - // The controller sets Total-Count and ETag on the 304, so neither may be dropped - // just because the page was not fetched. - Assert.That(result.QueryStats.TotalCount, Is.EqualTo(3)); - Assert.That(result.QueryStats.ETag, Is.EqualTo(version)); - } - } - - [Test] - public async Task Stale_known_version_returns_the_page() - { - await AddItems(2); - var staleVersion = await CurrentVersion(); - - await AddItems(1); - - var result = await EventLogDataStore.GetEventLogItems(new PagingInfo(), staleVersion); - - using (Assert.EnterMultipleScope()) - { - Assert.That(result.NotModified, Is.False); - Assert.That(result.Results, Has.Count.EqualTo(3)); - Assert.That(result.QueryStats.TotalCount, Is.EqualTo(3)); - Assert.That(result.QueryStats.ETag, Is.Not.EqualTo(staleVersion)); - } - } - - [Test] - public async Task Unrecognised_known_version_returns_the_page() - { - await AddItems(2); - - var result = await EventLogDataStore.GetEventLogItems(new PagingInfo(), "not-a-version-this-store-ever-issued"); - - using (Assert.EnterMultipleScope()) - { - Assert.That(result.NotModified, Is.False, "an unrecognised validator must be treated as a cache miss, never as a match"); - Assert.That(result.Results, Is.Not.Null); - } - } - - async Task CurrentVersion() => - (await EventLogDataStore.GetEventLogItems(new PagingInfo())).QueryStats.ETag; + async Task CurrentVersion() => + (await EventLogDataStore.GetEventLogItems(new PagingInfo())).QueryStats.Version; async Task AddItems(int count) { diff --git a/src/ServiceControl.Persistence.Tests/IPersistenceTestsContext.cs b/src/ServiceControl.Persistence.Tests/IPersistenceTestsContext.cs index 4128de9df9..886c86a0b4 100644 --- a/src/ServiceControl.Persistence.Tests/IPersistenceTestsContext.cs +++ b/src/ServiceControl.Persistence.Tests/IPersistenceTestsContext.cs @@ -1,6 +1,7 @@ #nullable enable namespace ServiceControl.Persistence.Tests; +using System; using System.Threading.Tasks; using MessageFailures; using Microsoft.Extensions.Hosting; @@ -15,6 +16,11 @@ public interface IPersistenceTestsContext Task CompleteDatabaseOperation(); + /// + /// Move the clock the persister stamps its own timestamps from + /// + void AdvanceClock(TimeSpan by); + PersistenceSettings PersistenceSettings { get; } string GenerateFailedMessageRecordId(string messageId); diff --git a/src/ServiceControl.Persistence.Tests/IngestedFailure.cs b/src/ServiceControl.Persistence.Tests/IngestedFailure.cs index 2de41ceb33..9f17cf7e6d 100644 --- a/src/ServiceControl.Persistence.Tests/IngestedFailure.cs +++ b/src/ServiceControl.Persistence.Tests/IngestedFailure.cs @@ -87,6 +87,7 @@ Dictionary BuildHeaders() MessageMetadata = new Dictionary { ["MessageId"] = MessageId, + ["MessageIntent"] = MessageIntent, ["MessageType"] = MessageType, ["TimeSent"] = TimeSent, ["ConversationId"] = ConversationId, diff --git a/src/ServiceControl.Persistence.Tests/IngestionTestBase.cs b/src/ServiceControl.Persistence.Tests/IngestionTestBase.cs new file mode 100644 index 0000000000..611381c938 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/IngestionTestBase.cs @@ -0,0 +1,39 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.Persistence.UnitOfWork; + +/// +/// Writes failures through the real ingestion path. +/// +abstract class IngestionTestBase : PersistenceTestBase +{ + protected async Task InBatch(Func record) + { + await using var unitOfWork = await UnitOfWorkFactory.StartNew(); + + await record(unitOfWork); + + await unitOfWork.Complete(TestContext.CurrentContext.CancellationToken); + } + + protected Task Ingest(params IngestedFailure[] failures) => + InBatch(async unitOfWork => + { + foreach (var failure in failures) + { + await unitOfWork.Recoverability.RecordFailedProcessingAttempt(failure.Context, failure.ProcessingAttempt, failure.Groups); + } + }); + + protected Task ConfirmRetry(params string[] uniqueMessageIds) => + InBatch(async unitOfWork => + { + foreach (var uniqueMessageId in uniqueMessageIds) + { + await unitOfWork.Recoverability.RecordSuccessfulRetry(uniqueMessageId); + } + }); +} diff --git a/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryAfterIngestionTests.cs b/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryAfterIngestionTests.cs index 44779e7deb..1b2ac294f4 100644 --- a/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryAfterIngestionTests.cs +++ b/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryAfterIngestionTests.cs @@ -6,7 +6,7 @@ namespace ServiceControl.Persistence.Tests; using ServiceControl.MessageFailures; using ServiceControl.Persistence.Infrastructure; -class FailedMessageQueryAfterIngestionTests : PersistenceTestBase +class FailedMessageQueryAfterIngestionTests : IngestionTestBase { [Test] public async Task Ingested_failure_is_returned_by_the_query_store() @@ -18,6 +18,7 @@ public async Task Ingested_failure_is_returned_by_the_query_store() }; await Ingest(failure); + await CompleteDatabaseOperation(); var result = await FailedMessageQueryStore.GetFailedMessages(null, null, null, new PagingInfo(), new SortInfo()); @@ -47,6 +48,7 @@ public async Task Ingestion_stores_the_failing_endpoint_address() var failure = new IngestedFailure { FailingEndpointAddress = "Sales@MACHINE" }; await Ingest(failure); + await CompleteDatabaseOperation(); var view = await FailedMessageQueryStore.GetLatestFailedMessageView(failure.UniqueMessageIdString); @@ -60,6 +62,7 @@ public async Task Ingested_failure_can_be_filtered_by_its_failing_endpoint_addre var other = new IngestedFailure { FailingEndpointAddress = "Billing@MACHINE" }; await Ingest(matching, other); + await CompleteDatabaseOperation(); var result = await FailedMessageQueryStore.GetFailedMessages(null, null, "Sales@MACHINE", new PagingInfo(), new SortInfo()); @@ -74,6 +77,7 @@ public async Task Ingested_failure_is_returned_by_id() var failure = new IngestedFailure(); await Ingest(failure); + await CompleteDatabaseOperation(); var message = await FailedMessageQueryStore.GetFailedMessage(failure.UniqueMessageIdString); @@ -97,6 +101,7 @@ public async Task Repeated_failures_are_counted_as_attempts() await Ingest(failure); await Ingest(secondAttempt); + await CompleteDatabaseOperation(); var view = await FailedMessageQueryStore.GetLatestFailedMessageView(failure.UniqueMessageIdString); var message = await FailedMessageQueryStore.GetFailedMessage(failure.UniqueMessageIdString); @@ -107,19 +112,4 @@ public async Task Repeated_failures_are_counted_as_attempts() Assert.That(message.ProcessingAttempts, Has.Count.EqualTo(2)); } } - - async Task Ingest(params IngestedFailure[] failures) - { - await using (var unitOfWork = await UnitOfWorkFactory.StartNew()) - { - foreach (var failure in failures) - { - await unitOfWork.Recoverability.RecordFailedProcessingAttempt(failure.Context, failure.ProcessingAttempt, failure.Groups); - } - - await unitOfWork.Complete(TestContext.CurrentContext.CancellationToken); - } - - await CompleteDatabaseOperation(); - } } diff --git a/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryDataStoreTests.cs index 057b693ff7..e03645f4d3 100644 --- a/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryDataStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/MessageFailures/FailedMessageQueryDataStoreTests.cs @@ -311,7 +311,7 @@ public async Task Reports_stats_matching_the_query() { Assert.That(stats.TotalCount, Is.EqualTo(1)); Assert.That(stats.TotalCount, Is.EqualTo(query.QueryStats.TotalCount)); - Assert.That(stats.ETag, Is.EqualTo(query.QueryStats.ETag)); + Assert.That(stats.Version.HasValue, Is.True, "the count endpoint still has to be cacheable"); } } @@ -323,7 +323,7 @@ public async Task Repeats_the_etag_while_nothing_changes() var first = await FailedMessageQueryStore.GetFailedMessagesStats(null, null, null); var second = await FailedMessageQueryStore.GetFailedMessagesStats(null, null, null); - Assert.That(second.ETag, Is.EqualTo(first.ETag)); + Assert.That(second.Version.Matches(first.Version), Is.True); } [Test] @@ -337,7 +337,7 @@ public async Task Changes_the_etag_when_the_set_changes() var after = await FailedMessageQueryStore.GetFailedMessagesStats(null, null, null); - Assert.That(after.ETag, Is.Not.EqualTo(before.ETag)); + Assert.That(after.Version.Matches(before.Version), Is.False); } diff --git a/src/ServiceControl.Persistence.Tests/MessagesViewVersionTests.cs b/src/ServiceControl.Persistence.Tests/MessagesViewVersionTests.cs new file mode 100644 index 0000000000..cb42a5744a --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/MessagesViewVersionTests.cs @@ -0,0 +1,173 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.CompositeViews.Messages; +using ServiceControl.Operations; +using ServiceControl.Persistence.Infrastructure; + +[TestFixture] +class MessagesViewVersionTests : IngestionTestBase +{ + [Test] + public async Task Version_changes_when_a_message_is_added() + { + await Ingest(new IngestedFailure()); + await CompleteDatabaseOperation(); + + var before = await AllMessages(); + + await Ingest(new IngestedFailure()); + await CompleteDatabaseOperation(); + + var after = await AllMessages(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(2), "the body now reports two messages"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body changed, so the validator must too"); + } + } + + [Test] + public async Task Version_changes_when_a_message_is_re_ingested_and_the_count_does_not_move() + { + var failure = new IngestedFailure(); + + await Ingest(failure); + await CompleteDatabaseOperation(); + + var before = await AllMessages(); + + AdvanceClock(TimeSpan.FromMinutes(5)); + + await Ingest(failure.NextAttempt(failure.AttemptedAt.AddMinutes(5))); + await CompleteDatabaseOperation(); + + var after = await AllMessages(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(1), "still one message"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the message the body reports has a later attempt on it, so the validator cannot stay put"); + } + } + + [Test] + public async Task Version_changes_when_the_endpoint_being_queried_gains_a_message() + { + await Ingest(ReceivedBy("Sales")); + await Ingest(ReceivedBy("Shipping")); + await CompleteDatabaseOperation(); + + var before = await MessagesFor("Sales"); + + await Ingest(ReceivedBy("Sales")); + await CompleteDatabaseOperation(); + + var after = await MessagesFor("Sales"); + + using (Assert.EnterMultipleScope()) + { + Assert.That(before.Results, Has.Count.EqualTo(1), "the Shipping message is not on this page"); + Assert.That(after.Results, Has.Count.EqualTo(2), "and the body now reports two"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body changed, so the validator must too"); + } + } + + [Test] + public async Task Version_is_stable_while_nothing_changes() + { + await Ingest(new IngestedFailure()); + await CompleteDatabaseOperation(); + + var first = await AllMessages(); + var second = await AllMessages(); + + VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version, + "nothing changed, so the validator has to stay put or conditional GET never pays off"); + } + + [Test] + public async Task Version_changes_when_the_total_moves_under_an_unchanged_page() + { + var shown = new IngestedFailure(); + + await Ingest(shown); + await CompleteDatabaseOperation(); + + var before = await MessagesViewStore.GetAllMessages(new PagingInfo(page: 1, pageSize: 1), new SortInfo(), includeSystemMessages: true); + + AdvanceClock(TimeSpan.FromMinutes(5)); + + await Ingest(new IngestedFailure()); + await CompleteDatabaseOperation(); + + var after = await MessagesViewStore.GetAllMessages(new PagingInfo(page: 1, pageSize: 1), new SortInfo(), includeSystemMessages: true); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(1), "still one row on the page"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "Total-Count went from one to two, and the body reports it, so the validator has to move"); + } + } + + [Test] + public async Task Version_changes_when_a_row_on_the_page_changes_status() + { + var archived = new IngestedFailure(); + + await Ingest(archived, new IngestedFailure()); + await CompleteDatabaseOperation(); + + var before = await AllMessages(); + + await FailedMessageLifecycleStore.MarkAsArchived(archived.UniqueMessageIdString); + await CompleteDatabaseOperation(); + + var after = await AllMessages(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(2), "both messages are still on the page"); + Assert.That(after.QueryStats.TotalCount, Is.EqualTo(before.QueryStats.TotalCount), "and the total has not moved"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body reports the new status, so a revalidating client must not be told its page is current"); + } + } + + [Test] + public async Task An_empty_store_still_reports_a_version() + { + var result = await AllMessages(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(result.Results, Is.Empty); + Assert.That(result.QueryStats.Version.HasValue, Is.True, + "an empty list is a representation like any other and has to be cacheable"); + } + } + + static IngestedFailure ReceivedBy(string endpointName) => + new() + { + EndpointName = endpointName, + ReceivingEndpoint = new EndpointDetails { Name = endpointName, Host = "ReceiverHost", HostId = Guid.NewGuid() } + }; + + Task>> AllMessages() => + MessagesViewStore.GetAllMessages(new PagingInfo(), new SortInfo(), includeSystemMessages: true); + + Task>> MessagesFor(string endpointName) => + MessagesViewStore.GetAllMessagesForEndpoint(endpointName, new PagingInfo(), new SortInfo(), includeSystemMessages: true); +} diff --git a/src/ServiceControl.Persistence.Tests/PersistenceTestBase.cs b/src/ServiceControl.Persistence.Tests/PersistenceTestBase.cs index d32c6db6a2..bd36998a56 100644 --- a/src/ServiceControl.Persistence.Tests/PersistenceTestBase.cs +++ b/src/ServiceControl.Persistence.Tests/PersistenceTestBase.cs @@ -83,6 +83,8 @@ protected async Task SeedFailedMessage(FailedMessage failedMessag return failedMessage; } + protected void AdvanceClock(TimeSpan by) => PersistenceTestsContext.AdvanceClock(by); + protected static async Task WaitUntil(Func> conditionChecker, string condition, TimeSpan timeout = default) { timeout = timeout == default ? TimeSpan.FromSeconds(10) : timeout; diff --git a/src/ServiceControl.Persistence.Tests/QueueAddressVersionTests.cs b/src/ServiceControl.Persistence.Tests/QueueAddressVersionTests.cs new file mode 100644 index 0000000000..79205d74dc --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/QueueAddressVersionTests.cs @@ -0,0 +1,116 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.Persistence.Infrastructure; + +[TestFixture] +class QueueAddressVersionTests : IngestionTestBase +{ + [Test] + public async Task Version_changes_when_a_queue_gains_a_failure_and_the_address_set_does_not() + { + await Ingest(Failure("SomeEndpoint@machine1")); + await CompleteDatabaseOperation(); + + var before = await QueueAddressStore.GetAddresses(new PagingInfo()); + + // A second, different message failing on the SAME queue. The address set is unchanged, so a + // validator built from the addresses alone cannot see this, and the client keeps a stale count. + await Ingest(Failure("SomeEndpoint@machine1")); + await CompleteDatabaseOperation(); + + var after = await QueueAddressStore.GetAddresses(new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(1), "still one address"); + Assert.That(after.Results[0].FailedMessageCount, Is.EqualTo(2), "and the body now reports two failures"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body changed, so the validator must too, or a revalidating client is served a stale count"); + } + } + + [Test] + public async Task Version_changes_when_a_message_moves_to_a_different_queue() + { + var first = Failure("SomeEndpoint@machine1"); + + await Ingest(first); + await CompleteDatabaseOperation(); + + var before = await QueueAddressStore.GetAddresses(new PagingInfo()); + + await Ingest(MovedTo("OtherEndpoint@machine2", first)); + await CompleteDatabaseOperation(); + + var after = await QueueAddressStore.GetAddresses(new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results, Has.Count.EqualTo(1), "still one address"); + Assert.That(after.Results[0].PhysicalAddress, Is.EqualTo("OtherEndpoint@machine2"), "and it is the new one"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body reports a different address under the same count, so the validator cannot stay put"); + } + } + + [Test] + public async Task Version_changes_when_a_new_address_appears() + { + await Ingest(Failure("SomeEndpoint@machine1")); + await CompleteDatabaseOperation(); + + var before = await QueueAddressStore.GetAddresses(new PagingInfo()); + + await Ingest(Failure("OtherEndpoint@machine2")); + await CompleteDatabaseOperation(); + + var after = await QueueAddressStore.GetAddresses(new PagingInfo()); + + VersionAssert.Moved(before.QueryStats.Version, after.QueryStats.Version, + "an address appeared, so a revalidating client must not be told its page is current"); + } + + [Test] + public async Task Version_is_stable_while_nothing_changes() + { + await Ingest(Failure("SomeEndpoint@machine1")); + await CompleteDatabaseOperation(); + + var first = await QueueAddressStore.GetAddresses(new PagingInfo()); + var second = await QueueAddressStore.GetAddresses(new PagingInfo()); + + VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version, + "nothing changed, so the validator has to stay put or conditional GET never pays off"); + } + + [Test] + public async Task An_empty_store_still_reports_a_version() + { + var result = await QueueAddressStore.GetAddresses(new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(result.Results, Is.Empty); + Assert.That(result.QueryStats.Version.HasValue, Is.True, + "an empty list is a representation like any other and has to be cacheable"); + } + } + + static IngestedFailure Failure(string failingEndpointAddress) => + new() { FailingEndpointAddress = failingEndpointAddress }; + + static IngestedFailure MovedTo(string failingEndpointAddress, IngestedFailure original) => + new() + { + MessageId = original.MessageId, + EndpointName = original.EndpointName, + FailingEndpointAddress = failingEndpointAddress, + AttemptedAt = original.AttemptedAt.AddHours(1), + TimeOfFailure = original.TimeOfFailure.AddHours(1) + }; +} diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/ArchivedGroupVersionTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/ArchivedGroupVersionTests.cs new file mode 100644 index 0000000000..22d2156ce7 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/Recoverability/ArchivedGroupVersionTests.cs @@ -0,0 +1,140 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Linq; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.MessageFailures; + +[TestFixture] +class ArchivedGroupVersionTests : PersistenceTestBase +{ + const string Classifier = "Exception Type and Stack Trace"; + + static readonly DateTime Oldest = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Utc); + static readonly DateTime Middle = new(2026, 8, 1, 13, 0, 0, DateTimeKind.Utc); + static readonly DateTime Newest = new(2026, 8, 1, 17, 0, 0, DateTimeKind.Utc); + + [Test] + public async Task Version_changes_when_group_counts_move_but_the_total_and_the_span_hold() + { + var shipping = NewGroup("Shipping"); + var billing = NewGroup("Billing"); + + var oldest = InGroup(shipping, Oldest); + var middle = InGroup(shipping, Middle); + var newest = InGroup(billing, Newest); + + await Insert(oldest, middle, newest); + await Archive(oldest, middle, newest); + + var before = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + + // The archived set keeps two groups, three messages, and the same earliest and latest failure. + // All that moves is how the three are split between the groups, from two and one to one and two. + var replacement = InGroup(billing, Middle); + await Insert(replacement); + await Archive(replacement); + _ = await FailedMessageLifecycleStore.UnArchiveMessages([middle.UniqueMessageIdString]); + await CompleteDatabaseOperation(); + + var after = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + + using (Assert.EnterMultipleScope()) + { + Assert.That(before.Results, Has.Count.EqualTo(2), "two archived groups to start with"); + Assert.That(after.Results, Has.Count.EqualTo(2), "and still two afterwards"); + Assert.That(after.Results.Sum(group => group.Count), Is.EqualTo(3), "still three archived messages between them"); + Assert.That(after.Results.Max(group => group.Last), Is.EqualTo(before.Results.Max(group => group.Last)), "and the latest failure has not moved"); + Assert.That(before.Results.Single(group => group.Title == "Shipping").Count, Is.EqualTo(2), "Shipping held two of them"); + Assert.That(after.Results.Single(group => group.Title == "Shipping").Count, Is.EqualTo(1), "and now holds one, so the split between the groups moved"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body reports a different count per group, so the validator cannot stay put"); + } + } + + [Test] + public async Task Version_changes_when_a_group_gains_a_message() + { + var group = NewGroup("Shipping"); + var first = InGroup(group, Oldest); + + await Insert(first); + await Archive(first); + + var before = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + + var second = InGroup(group, Newest); + await Insert(second); + await Archive(second); + + var after = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + + VersionAssert.Moved(before.QueryStats.Version, after.QueryStats.Version, + "the archived group gained a message, so its validator cannot stay put"); + } + + [Test] + public async Task Version_is_stable_while_nothing_changes() + { + var group = NewGroup("Shipping"); + var failure = InGroup(group, Oldest); + + await Insert(failure); + await Archive(failure); + + var first = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + var second = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + + VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version, + "nothing changed, so the validator has to stay put or conditional GET never pays off"); + } + + [Test] + public async Task A_classifier_with_nothing_archived_still_reports_a_version() + { + var result = await GroupsStore.GetArchivedGroupsByClassifier(Classifier); + + using (Assert.EnterMultipleScope()) + { + Assert.That(result.Results, Is.Empty); + Assert.That(result.QueryStats.Version.HasValue, Is.True); + } + } + + static FailedMessage.FailureGroup NewGroup(string title) => + new() { Id = Guid.NewGuid().ToString(), Title = title, Type = Classifier }; + + static IngestedFailure InGroup(FailedMessage.FailureGroup group, DateTime failedAt) => + new() + { + Groups = [group], + AttemptedAt = failedAt, + TimeOfFailure = failedAt, + TimeSent = failedAt.AddMinutes(-1) + }; + + async Task Archive(params IngestedFailure[] failures) + { + foreach (var failure in failures) + { + await FailedMessageLifecycleStore.MarkAsArchived(failure.UniqueMessageIdString); + } + + await CompleteDatabaseOperation(); + } + + async Task Insert(params IngestedFailure[] failures) + { + var messages = Array.ConvertAll(failures, failure => failure.ToFailedMessage()); + + foreach (var message in messages) + { + message.Id = PersistenceTestsContext.GenerateFailedMessageRecordId(message.UniqueMessageId); + } + + await PersistenceTestsContext.InsertFailedMessages(messages); + await CompleteDatabaseOperation(); + } +} diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/FailureGroupVersionTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/FailureGroupVersionTests.cs new file mode 100644 index 0000000000..df857929d3 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/Recoverability/FailureGroupVersionTests.cs @@ -0,0 +1,218 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Threading.Tasks; +using NUnit.Framework; +using ServiceControl.MessageFailures; +using ServiceControl.Persistence.Infrastructure; + +[TestFixture] +class FailureGroupVersionTests : PersistenceTestBase +{ + const string Classifier = "Exception Type and Stack Trace"; + + static readonly DateTime Oldest = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Utc); + static readonly DateTime Middle = new(2026, 8, 1, 13, 0, 0, DateTimeKind.Utc); + static readonly DateTime Newest = new(2026, 8, 1, 17, 0, 0, DateTimeKind.Utc); + + [Test] + public async Task Version_changes_when_a_group_loses_a_message_that_is_neither_its_oldest_nor_its_newest() + { + var group = NewGroup(); + var middle = InGroup(group, Middle); + + await Insert(InGroup(group, Oldest), middle, InGroup(group, Newest)); + + var before = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + // Archiving takes the message out of the unresolved group without removing the group, and + // the one chosen is neither the earliest nor the latest, so First and Last both stay put. + // Count is the only thing that moves, and Count is what the body reports. + await FailedMessageLifecycleStore.MarkAsArchived(middle.UniqueMessageIdString); + await CompleteDatabaseOperation(); + + var after = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + using (Assert.EnterMultipleScope()) + { + Assert.That(before.Results.Count, Is.EqualTo(3), "three messages to start with"); + Assert.That(after.Results.Count, Is.EqualTo(2), "and the body now reports two"); + Assert.That(after.Results.First, Is.EqualTo(before.Results.First), "the earliest failure is unchanged"); + Assert.That(after.Results.Last, Is.EqualTo(before.Results.Last), "and so is the latest"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body changed, so the validator must too, or a revalidating client is served a stale count"); + } + } + + [Test] + public async Task Version_changes_when_a_group_gains_a_message() + { + var group = NewGroup(); + + await Insert(InGroup(group, Oldest)); + + var before = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + await Insert(InGroup(group, Newest)); + + var after = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + VersionAssert.Moved(before.QueryStats.Version, after.QueryStats.Version, + "the group gained a message, so its validator cannot stay put"); + } + + [Test] + public async Task Version_changes_when_the_span_of_a_group_moves_but_its_count_does_not() + { + var group = NewGroup(); + var oldest = InGroup(group, Oldest); + + await Insert(oldest, InGroup(group, Middle), InGroup(group, Newest)); + + var before = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + // The count lands exactly where it started, so this is the case that proves First and Last + // are named. A version built from the count alone passes every other case in this fixture. + await FailedMessageLifecycleStore.MarkAsArchived(oldest.UniqueMessageIdString); + await Insert(InGroup(group, Newest.AddHours(4))); + + var after = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + using (Assert.EnterMultipleScope()) + { + Assert.That(after.Results.Count, Is.EqualTo(before.Results.Count), "still three messages"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "a different span of failures is being reported under the same count"); + } + } + + [Test] + public async Task Version_is_stable_while_nothing_changes() + { + var group = NewGroup(); + + await Insert(InGroup(group, Oldest)); + + var first = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + var second = await GroupsStore.GetUnresolvedGroup(group.Id, null, null); + + VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version, + "nothing changed, so the validator has to stay put or conditional GET never pays off"); + } + + [Test] + public async Task The_errors_in_a_group_report_a_version_that_moves_with_them() + { + var group = NewGroup(); + var middle = InGroup(group, Middle); + + await Insert(InGroup(group, Oldest), middle, InGroup(group, Newest)); + + var before = await GroupsStore.GetGroupErrors(group.Id, "unresolved", null, new SortInfo(), new PagingInfo()); + + await FailedMessageLifecycleStore.MarkAsArchived(middle.UniqueMessageIdString); + await CompleteDatabaseOperation(); + + var after = await GroupsStore.GetGroupErrors(group.Id, "unresolved", null, new SortInfo(), new PagingInfo()); + + using (Assert.EnterMultipleScope()) + { + Assert.That(before.Results, Has.Count.EqualTo(3), "three errors to start with"); + Assert.That(after.Results, Has.Count.EqualTo(2), "and the body now reports two"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the page lost a row, so the validator cannot stay put"); + } + } + + [Test] + public async Task The_error_count_of_a_group_reports_a_version_that_moves_with_it() + { + var group = NewGroup(); + var middle = InGroup(group, Middle); + + await Insert(InGroup(group, Oldest), middle, InGroup(group, Newest)); + + var before = await GroupsStore.GetGroupErrorsCount(group.Id, "unresolved", null); + + await FailedMessageLifecycleStore.MarkAsArchived(middle.UniqueMessageIdString); + await CompleteDatabaseOperation(); + + var after = await GroupsStore.GetGroupErrorsCount(group.Id, "unresolved", null); + + using (Assert.EnterMultipleScope()) + { + Assert.That(before.TotalCount, Is.EqualTo(3)); + Assert.That(after.TotalCount, Is.EqualTo(2), "the count the body reports has changed"); + Assert.That(before.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.Version.Matches(before.Version), Is.False); + } + } + + [Test] + public async Task An_archived_group_reports_a_version_that_moves_with_it() + { + var group = NewGroup(); + var oldest = InGroup(group, Oldest); + var middle = InGroup(group, Middle); + + await Insert(oldest, middle, InGroup(group, Newest)); + await FailedMessageLifecycleStore.MarkAsArchived(oldest.UniqueMessageIdString); + await FailedMessageLifecycleStore.MarkAsArchived(middle.UniqueMessageIdString); + await CompleteDatabaseOperation(); + + var before = await GroupsStore.GetArchivedGroup(group.Id, null, null); + + _ = await FailedMessageLifecycleStore.UnArchiveMessages([middle.UniqueMessageIdString]); + await CompleteDatabaseOperation(); + + var after = await GroupsStore.GetArchivedGroup(group.Id, null, null); + + using (Assert.EnterMultipleScope()) + { + Assert.That(before.Results.Count, Is.EqualTo(2), "two archived errors to start with"); + Assert.That(after.Results.Count, Is.EqualTo(1), "and the body now reports one"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False); + } + } + + [Test] + public async Task A_group_that_does_not_exist_still_reports_a_version() + { + var result = await GroupsStore.GetUnresolvedGroup("no-such-group", null, null); + + using (Assert.EnterMultipleScope()) + { + Assert.That(result.Results, Is.Null); + Assert.That(result.QueryStats.Version.HasValue, Is.True); + } + } + + static FailedMessage.FailureGroup NewGroup() => + new() { Id = Guid.NewGuid().ToString(), Title = "OrderPlaced", Type = Classifier }; + + static IngestedFailure InGroup(FailedMessage.FailureGroup group, DateTime failedAt) => + new() + { + Groups = [group], + AttemptedAt = failedAt, + TimeOfFailure = failedAt, + TimeSent = failedAt.AddMinutes(-1) + }; + + async Task Insert(params IngestedFailure[] failures) + { + var messages = Array.ConvertAll(failures, failure => failure.ToFailedMessage()); + + foreach (var message in messages) + { + message.Id = PersistenceTestsContext.GenerateFailedMessageRecordId(message.UniqueMessageId); + } + + await PersistenceTestsContext.InsertFailedMessages(messages); + await CompleteDatabaseOperation(); + } +} diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/GroupsDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/GroupsDataStoreTests.cs index 783fc0ff76..6502efbb4a 100644 --- a/src/ServiceControl.Persistence.Tests/Recoverability/GroupsDataStoreTests.cs +++ b/src/ServiceControl.Persistence.Tests/Recoverability/GroupsDataStoreTests.cs @@ -88,7 +88,7 @@ await Insert( InGroup(group).ToFailedMessage(), InGroup(group).ToFailedMessage(FailedMessageStatus.Archived)); - var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Single(); + var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Results.Single(); using (Assert.EnterMultipleScope()) { @@ -295,7 +295,7 @@ public async Task Leaves_archived_groups_without_their_comment() await Insert(InGroup(group).ToFailedMessage(FailedMessageStatus.Archived)); await EditComment(group.Id, "Only shown on the open group"); - var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Single(); + var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Results.Single(); Assert.That(view.Comment, Is.Null); } diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/RetryHistoryVersionTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/RetryHistoryVersionTests.cs new file mode 100644 index 0000000000..2d07d10630 --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/Recoverability/RetryHistoryVersionTests.cs @@ -0,0 +1,90 @@ +namespace ServiceControl.Persistence.Tests; + +using System; +using System.Threading.Tasks; +using NUnit.Framework; + +[TestFixture] +class RetryHistoryVersionTests : PersistenceTestBase +{ + const int DefaultDepth = 10; + + static readonly DateTime Noon = new(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc); + + [Test] + public async Task Version_changes_when_an_operation_is_acknowledged() + { + await RecordCompleted("group-1"); + await CompleteDatabaseOperation(); + + var before = await RetryHistoryStore.GetRetryHistory(); + + var acknowledged = await RetryHistoryStore.AcknowledgeRetryGroup("group-1"); + await CompleteDatabaseOperation(); + + var after = await RetryHistoryStore.GetRetryHistory(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(acknowledged, Is.True, "the premise: there was something to acknowledge"); + Assert.That(before.Results.UnacknowledgedOperations, Has.Count.EqualTo(1)); + Assert.That(after.Results.UnacknowledgedOperations, Is.Empty, "the body changed"); + Assert.That(after.Results.HistoricOperations, Has.Count.EqualTo(1), "and the historic half did not"); + Assert.That(before.QueryStats.Version.HasValue, Is.True, "there was no version to move"); + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "the body changed, so the validator must too, or a revalidating client keeps an operation it has dismissed"); + } + } + + [Test] + public async Task Version_changes_when_an_operation_completes() + { + await RecordCompleted("group-1"); + await CompleteDatabaseOperation(); + + var before = await RetryHistoryStore.GetRetryHistory(); + + await RecordCompleted("group-2", completionTime: Noon.AddHours(1)); + await CompleteDatabaseOperation(); + + var after = await RetryHistoryStore.GetRetryHistory(); + + VersionAssert.Moved(before.QueryStats.Version, after.QueryStats.Version, + "another operation completed, so a revalidating client must not keep the old history"); + } + + [Test] + public async Task Version_is_stable_while_nothing_changes() + { + await RecordCompleted("group-1"); + await CompleteDatabaseOperation(); + + var first = await RetryHistoryStore.GetRetryHistory(); + var second = await RetryHistoryStore.GetRetryHistory(); + + VersionAssert.Matches(first.QueryStats.Version, second.QueryStats.Version, + "nothing changed, so the validator has to stay put or conditional GET never pays off"); + } + + [Test] + public async Task An_empty_history_still_reports_a_version() + { + var result = await RetryHistoryStore.GetRetryHistory(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(result.Results.HistoricOperations, Is.Empty); + Assert.That(result.QueryStats.Version.HasValue, Is.True, + "an empty history is a representation like any other and has to be cacheable"); + } + } + + Task RecordCompleted(string requestId, DateTime? completionTime = null) + { + var completed = completionTime ?? Noon; + + return RetryHistoryStore.RecordRetryOperationCompleted(requestId, RetryType.FailureGroup, + completed.AddMinutes(-5), completed, "OrderPlaced failures", "Exception Type and Stack Trace", + messageFailed: false, numberOfMessagesProcessed: 1, completed.AddMinutes(-1), DefaultDepth); + } +} diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/ReturnToSenderDequeuerTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/ReturnToSenderDequeuerTests.cs index c73e7bbeba..d6e0473429 100644 --- a/src/ServiceControl.Persistence.Tests/Recoverability/ReturnToSenderDequeuerTests.cs +++ b/src/ServiceControl.Persistence.Tests/Recoverability/ReturnToSenderDequeuerTests.cs @@ -217,7 +217,7 @@ public Task TryFetch(string bodyId, CancellationToken cancell _ => throw new ArgumentOutOfRangeException(nameof(state), state, null) }); - static MessageBodyStreamContent Content(byte[] body) => new(new MemoryStream(body), "text/plain", body.Length, "etag"); + static MessageBodyStreamContent Content(byte[] body) => new(new MemoryStream(body), "text/plain", body.Length, DataVersion.FromToken("etag")); } } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.Tests/VersionAssert.cs b/src/ServiceControl.Persistence.Tests/VersionAssert.cs new file mode 100644 index 0000000000..25edc5bc5c --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/VersionAssert.cs @@ -0,0 +1,29 @@ +namespace ServiceControl.Persistence.Tests; + +using NUnit.Framework; +using ServiceControl.Persistence.Infrastructure; + +static class VersionAssert +{ + public static void Moved(DataVersion before, DataVersion after, string because) + { + using (Assert.EnterMultipleScope()) + { + Assert.That(before.HasValue, Is.True, "there was no version to move"); + Assert.That(after.HasValue, Is.True, "the version went missing rather than moving"); + Assert.That(after.Matches(before), Is.False, because); + } + } + + public static void Matches(DataVersion first, DataVersion second, string because) + { + using (Assert.EnterMultipleScope()) + { + Assert.That(first.HasValue, Is.True, "there was no version to hold"); + Assert.That(second.Matches(first), Is.True, because); + } + } + + public static bool Matches(this DataVersion one, DataVersion other) => + one.HasValue && other.HasValue && one.Equals(other); +} diff --git a/src/ServiceControl.Persistence/IBodyStorage.cs b/src/ServiceControl.Persistence/IBodyStorage.cs index 7d7715437f..187c3e8dc9 100644 --- a/src/ServiceControl.Persistence/IBodyStorage.cs +++ b/src/ServiceControl.Persistence/IBodyStorage.cs @@ -4,6 +4,7 @@ using System.IO; using System.Threading; using System.Threading.Tasks; + using ServiceControl.Persistence.Infrastructure; public interface IBodyStorage { @@ -47,5 +48,5 @@ public static MessageBodyResult Available(MessageBodyStreamContent content) MessageBodyStreamContent? ContentValue { get; } } - public sealed record MessageBodyStreamContent(Stream Stream, string ContentType, int BodySize, string Etag); + public sealed record MessageBodyStreamContent(Stream Stream, string ContentType, int BodySize, DataVersion Version); } \ No newline at end of file diff --git a/src/ServiceControl.Persistence/IEventLogDataStore.cs b/src/ServiceControl.Persistence/IEventLogDataStore.cs index b130161cfb..d7fa9eb579 100644 --- a/src/ServiceControl.Persistence/IEventLogDataStore.cs +++ b/src/ServiceControl.Persistence/IEventLogDataStore.cs @@ -27,20 +27,7 @@ public interface IEventLogDataStore /// Returns one page of event log items, newest RaisedAt first. /// /// Which page to return. - /// - /// The version the caller already holds, or null if it holds none. When it matches, the - /// result is and carries no page. - /// - /// - /// : the requested page, which may be empty. - /// : the number of items in the store, independent of the - /// page size, and populated even when nothing was modified. - /// : an opaque cache validator surfaced verbatim as the - /// ETag response header, so whatever a client echoes back arrives here as - /// . It must change when retention removes items, not only when - /// one is added, since nothing else tells a client its cached page is now wrong. - /// Task>> GetEventLogItems( - PagingInfo pagingInfo, string? knownVersion = null, CancellationToken cancellationToken = default); + PagingInfo pagingInfo, CancellationToken cancellationToken = default); } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence/IGroupsDataStore.cs b/src/ServiceControl.Persistence/IGroupsDataStore.cs index 5f389f10fe..2d1ffc547e 100644 --- a/src/ServiceControl.Persistence/IGroupsDataStore.cs +++ b/src/ServiceControl.Persistence/IGroupsDataStore.cs @@ -10,7 +10,7 @@ namespace ServiceControl.Persistence public interface IGroupsDataStore { Task> GetUnresolvedGroupsByClassifier(string classifier, string? classifierFilter, CancellationToken cancellationToken = default); - Task> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default); + Task>> GetArchivedGroupsByClassifier(string classifier, CancellationToken cancellationToken = default); Task> GetUnresolvedGroup(string groupId, string? status, string? modified, CancellationToken cancellationToken = default); Task> GetArchivedGroup(string groupId, string? status, string? modified, CancellationToken cancellationToken = default); diff --git a/src/ServiceControl.Persistence/IRetryBatchStore.cs b/src/ServiceControl.Persistence/IRetryBatchStore.cs index 3f037a3327..a21eb56e46 100644 --- a/src/ServiceControl.Persistence/IRetryBatchStore.cs +++ b/src/ServiceControl.Persistence/IRetryBatchStore.cs @@ -19,7 +19,7 @@ Task CreateBatch(string retrySessionId, string requestId, RetryType retr Task MoveBatchToStaging(string batchId, CancellationToken cancellationToken = default); - Task>> GetOrphanedBatches(string retrySessionId, CancellationToken cancellationToken = default); + Task GetOrphanedBatches(string retrySessionId, CancellationToken cancellationToken = default); Task> GetAvailableBatchGroups(CancellationToken cancellationToken = default); Task GetCurrentForwardingBatch(CancellationToken cancellationToken = default); diff --git a/src/ServiceControl.Persistence/IRetryHistoryDataStore.cs b/src/ServiceControl.Persistence/IRetryHistoryDataStore.cs index 5944bc65c5..cf9b195aa4 100644 --- a/src/ServiceControl.Persistence/IRetryHistoryDataStore.cs +++ b/src/ServiceControl.Persistence/IRetryHistoryDataStore.cs @@ -3,11 +3,12 @@ using System; using System.Threading; using System.Threading.Tasks; + using ServiceControl.Persistence.Infrastructure; using ServiceControl.Recoverability; public interface IRetryHistoryDataStore { - Task GetRetryHistory(CancellationToken cancellationToken = default); + Task> GetRetryHistory(CancellationToken cancellationToken = default); Task RecordRetryOperationCompleted(string requestId, RetryType retryType, DateTime startTime, DateTime completionTime, string? originator, string? classifier, bool messageFailed, int numberOfMessagesProcessed, DateTime lastProcessed, int retryHistoryDepth, CancellationToken cancellationToken = default); diff --git a/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs b/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs new file mode 100644 index 0000000000..85f89b2799 --- /dev/null +++ b/src/ServiceControl.Persistence/Infrastructure/DataVersion.cs @@ -0,0 +1,160 @@ +namespace ServiceControl.Persistence.Infrastructure +{ + using System; + using System.Collections.Generic; + using System.Diagnostics; + using System.Diagnostics.CodeAnalysis; + using System.Globalization; + using System.Linq; + + /// + /// An opaque version of a query result, sent to clients as an HTTP entity-tag. + /// + [DebuggerDisplay("{validator ?? \"None\",nq}")] + public readonly struct DataVersion : IEquatable + { + readonly string? validator; + + DataVersion(string validator) => this.validator = validator; + + public static readonly DataVersion None = default; + + [MemberNotNullWhen(true, nameof(validator))] + public bool HasValue => validator is not null; + + /// A version the backend made itself. + public static DataVersion FromToken(string token) => + string.IsNullOrEmpty(token) ? None : new DataVersion(token); + + public static DataVersion FromToken(long token) => + new(token.ToString(CultureInfo.InvariantCulture)); + + /// + /// A version over the query behind the page. Every field the response shows has to be covered by a + /// term, measured over the same filtered set, or a change to an uncovered one leaves a client holding + /// a stale page. + /// + public static DataVersion Compose(params (string Name, object? Value)[]? terms) => + terms is null || terms.Length == 0 + ? None + : new DataVersion(DeterministicGuid.MakeId(Describe(terms)).ToString()); + + /// + /// A version over a list the response renders row by row. covers whatever + /// the response says about the list as a whole, such as the total behind Total-Count when the rows are + /// only one page of it, and one term per row covers the rows themselves. Every field a row shows has + /// to appear in , and each is length prefixed, so no value can pose as a + /// different set of fields. Rows are named by position, so a caller whose query has no ORDER BY has to + /// sort them first. + /// + public static DataVersion OverRows((string Name, object? Value)[]? summary, IEnumerable rows, Func fields) + { + ArgumentNullException.ThrowIfNull(rows); + ArgumentNullException.ThrowIfNull(fields); + + var terms = new List<(string Name, object? Value)>(summary ?? []); + var row = 0; + + foreach (var item in rows) + { + terms.Add((FormattableString.Invariant($"row{row++}"), Row(fields(item)))); + } + + return Compose([.. terms]); + } + + /// + /// One version for a result gathered from several instances. Missing anywhere means missing overall. + /// Keyed on the instance, so a validator moving from one instance to another still moves the + /// composite. + /// + public static DataVersion Combine(IEnumerable<(string InstanceId, DataVersion Version)> versions) + { + ArgumentNullException.ThrowIfNull(versions); + + var reported = new List<(string InstanceId, string Validator)>(); + + foreach (var (instanceId, version) in versions) + { + if (!version.HasValue) + { + return None; + } + + reported.Add((instanceId, version.validator)); + } + + if (reported.Count == 0) + { + return None; + } + + return Compose([.. reported + .OrderBy(entry => entry.InstanceId, StringComparer.Ordinal) + .ThenBy(entry => entry.Validator, StringComparer.Ordinal) + .Select(entry => (entry.InstanceId, (object)entry.Validator))]); + } + + /// + /// A validator read back off the wire, in any shape an old or current instance might emit, so that + /// a scatter-gather can fold another instance's entity-tag into its own composite. + /// + public static DataVersion FromClient(string headerValue) + { + var value = headerValue?.Trim(); + + if (string.IsNullOrEmpty(value)) + { + return None; + } + + if (value.StartsWith("W/", StringComparison.Ordinal)) + { + value = value[2..]; + } + + // Only a matching pair. Stripping every quote would truncate a malformed header instead of + // treating it as the cache miss it is. + if (value.Length > 1 && value[0] == '"' && value[^1] == '"') + { + value = value[1..^1]; + } + + return FromToken(value); + } + + public bool Equals(DataVersion other) => + string.Equals(validator, other.validator, StringComparison.Ordinal); + + public override bool Equals(object? obj) => obj is DataVersion other && Equals(other); + + public override int GetHashCode() => validator?.GetHashCode(StringComparison.Ordinal) ?? 0; + + /// The validator unquoted, or an empty string for . + public override string ToString() => validator ?? string.Empty; + + static string Describe((string Name, object? Value)[] terms) => + string.Join("|", terms.Select(term => Encode(term.Name, Format(term.Value)))); + + static string Encode(string name, string value) => $"{name}:{Prefixed(value)}"; + + static string Row(object?[]? fields) => + fields is null ? string.Empty : string.Concat(fields.Select(field => Prefixed(Format(field)))); + + static string Prefixed(string value) => + $"{value.Length.ToString(CultureInfo.InvariantCulture)}:{value}"; + + static string Format(object? value) => value switch + { + null => string.Empty, + string text => text, + bool flag => flag.ToString(), + DateTime timestamp => timestamp.Ticks.ToString(CultureInfo.InvariantCulture), + DateTimeOffset timestamp => timestamp.UtcTicks.ToString(CultureInfo.InvariantCulture), + IFormattable formattable => formattable.ToString(null, CultureInfo.InvariantCulture), + // Any other ToString is not a documented function of the content, so it could pin the + // version while the data moves and cache a stale page forever. + _ => throw new ArgumentException($"A version term cannot be built from {value.GetType()}.", nameof(value)) + }; + } +} diff --git a/src/ServiceControl.Persistence/Infrastructure/QueryResult.cs b/src/ServiceControl.Persistence/Infrastructure/QueryResult.cs index be3e6b8377..519217f1c5 100644 --- a/src/ServiceControl.Persistence/Infrastructure/QueryResult.cs +++ b/src/ServiceControl.Persistence/Infrastructure/QueryResult.cs @@ -9,18 +9,14 @@ public class QueryResult(TOut? results, QueryStatsInfo queryStatsInfo) public string? InstanceId { get; set; } - public QueryStatsInfo QueryStats { get; } = queryStatsInfo; - /// - /// The caller already holds this version, so was never fetched and is - /// null. is still populated. + /// The result the scatter-gather got from its own instance. /// - public bool NotModified { get; private init; } + public bool IsLocalInstance { get; set; } - public static QueryResult Empty() => new(null, QueryStatsInfo.Zero); + public QueryStatsInfo QueryStats { get; } = queryStatsInfo; - public static QueryResult Unchanged(QueryStatsInfo queryStatsInfo) => - new(null, queryStatsInfo) { NotModified = true }; + public static QueryResult Empty() => new(null, QueryStatsInfo.Zero); public static implicit operator Task>(QueryResult instance) => Task.FromResult(instance); } diff --git a/src/ServiceControl.Persistence/Infrastructure/QueryStatsInfo.cs b/src/ServiceControl.Persistence/Infrastructure/QueryStatsInfo.cs index 91abfcef4b..0cf462e9ed 100644 --- a/src/ServiceControl.Persistence/Infrastructure/QueryStatsInfo.cs +++ b/src/ServiceControl.Persistence/Infrastructure/QueryStatsInfo.cs @@ -1,21 +1,19 @@ namespace ServiceControl.Persistence.Infrastructure { - public struct QueryStatsInfo + public readonly struct QueryStatsInfo { - public readonly string ETag; + public readonly DataVersion Version; public readonly long TotalCount; public readonly long HighestTotalCountOfAllTheInstances; - public readonly bool IsStale; - public QueryStatsInfo(string eTag, long totalCount, bool isStale, long? highestTotalCountOfAllTheInstances = null) + public QueryStatsInfo(DataVersion version, long totalCount, long? highestTotalCountOfAllTheInstances = null) { - ETag = eTag; + Version = version; TotalCount = totalCount; - IsStale = isStale; HighestTotalCountOfAllTheInstances = highestTotalCountOfAllTheInstances ?? totalCount; } - public static readonly QueryStatsInfo Zero = new QueryStatsInfo(string.Empty, 0, false); + public static readonly QueryStatsInfo Zero = new(DataVersion.None, 0); } -} \ No newline at end of file +} diff --git a/src/ServiceControl.Persistence/OrphanedBatches.cs b/src/ServiceControl.Persistence/OrphanedBatches.cs new file mode 100644 index 0000000000..5ee137fe6f --- /dev/null +++ b/src/ServiceControl.Persistence/OrphanedBatches.cs @@ -0,0 +1,11 @@ +namespace ServiceControl.Persistence +{ + using System.Collections.Generic; + + // An eventually-consistent store can miss batches its index has not caught up with yet, so the sweep + // keeps rechecking while that is possible. + public record OrphanedBatches(IReadOnlyList Batches, bool MightBeIncomplete) + { + public static OrphanedBatches Complete(IReadOnlyList batches) => new(batches, false); + } +} diff --git a/src/ServiceControl.Persistence/RetryHistory.cs b/src/ServiceControl.Persistence/RetryHistory.cs index fffdfdbd23..888c8ae376 100644 --- a/src/ServiceControl.Persistence/RetryHistory.cs +++ b/src/ServiceControl.Persistence/RetryHistory.cs @@ -20,11 +20,6 @@ public void AddToHistory(HistoricRetryOperation historicOperation, int historyDe .ToList(); } - public string GetHistoryOperationsUniqueIdentifier() - { - return string.Join(',', HistoricOperations.Select(x => x.RequestId)); - } - public void AddToUnacknowledged(UnacknowledgedRetryOperation unacknowledgedRetryOperation) { UnacknowledgedOperations.Add(unacknowledgedRetryOperation); diff --git a/src/ServiceControl.UnitTests/BodyStorage/MessageBodyResultTests.cs b/src/ServiceControl.UnitTests/BodyStorage/MessageBodyResultTests.cs index 5c2b04f97a..39adb49c2d 100644 --- a/src/ServiceControl.UnitTests/BodyStorage/MessageBodyResultTests.cs +++ b/src/ServiceControl.UnitTests/BodyStorage/MessageBodyResultTests.cs @@ -4,6 +4,7 @@ namespace ServiceControl.UnitTests.BodyStorage; using System.IO; using NUnit.Framework; using ServiceControl.Operations.BodyStorage; +using ServiceControl.Persistence.Infrastructure; [TestFixture] public class MessageBodyResultTests @@ -28,7 +29,7 @@ public void Body_is_not_accessible_without_content(MessageBodyState state) [Test] public void Body_is_accessible_when_available() { - var content = new MessageBodyStreamContent(Stream.Null, "text/plain", 1, "etag"); + var content = new MessageBodyStreamContent(Stream.Null, "text/plain", 1, DataVersion.FromToken("etag")); var result = MessageBodyResult.Available(content); Assert.That(result.Content, Is.SameAs(content)); diff --git a/src/ServiceControl.UnitTests/HeaderAssertions.cs b/src/ServiceControl.UnitTests/HeaderAssertions.cs index 4afb82970f..6b401cdc8f 100644 --- a/src/ServiceControl.UnitTests/HeaderAssertions.cs +++ b/src/ServiceControl.UnitTests/HeaderAssertions.cs @@ -2,6 +2,7 @@ { using System.Collections.Generic; using NUnit.Framework; + using ServiceControl.Persistence.Infrastructure; public static class HeaderAssertions { @@ -19,5 +20,8 @@ public static void AssertHeaderMissing(this IDictionary headers, { Assert.That(headers.ContainsKey(key), Is.False, $"Unexpected header [{key}] found."); } + + public static bool Matches(this DataVersion one, DataVersion other) => + one.HasValue && other.HasValue && one.Equals(other); } -} \ No newline at end of file +} diff --git a/src/ServiceControl.UnitTests/Infrastructure/DataVersionTests.cs b/src/ServiceControl.UnitTests/Infrastructure/DataVersionTests.cs new file mode 100644 index 0000000000..b5b47b6050 --- /dev/null +++ b/src/ServiceControl.UnitTests/Infrastructure/DataVersionTests.cs @@ -0,0 +1,272 @@ +namespace ServiceControl.UnitTests.Infrastructure; + +using System; +using NUnit.Framework; +using ServiceControl.Persistence.Infrastructure; + +[TestFixture] +public class DataVersionTests +{ + [Test] + public void None_never_matches_itself() + { + Assert.That(DataVersion.None.Matches(DataVersion.None), Is.False, + "two callers who both know nothing have not established that nothing changed, and matching here would answer 304 for every request"); + } + + [Test] + public void None_never_matches_a_real_version() + { + var real = DataVersion.FromToken("4611686018427387904"); + + using (Assert.EnterMultipleScope()) + { + Assert.That(DataVersion.None.Matches(real), Is.False); + Assert.That(real.Matches(DataVersion.None), Is.False); + } + } + + [Test] + public void Equality_stays_reflexive_so_the_struct_is_safe_in_collections() + { + // Matches carries the cache rule, Equals must not, or any dictionary or Distinct over it breaks. + Assert.That(DataVersion.None.Equals(DataVersion.None), Is.True); + } + + [Test] + public void An_identical_token_matches() + { + Assert.That(DataVersion.FromToken("abc").Matches(DataVersion.FromToken("abc")), Is.True); + } + + [Test] + public void A_different_token_does_not_match() + { + Assert.That(DataVersion.FromToken("abc").Matches(DataVersion.FromToken("abd")), Is.False); + } + + [Test] + public void An_empty_token_is_absent() + { + using (Assert.EnterMultipleScope()) + { + Assert.That(DataVersion.FromToken(null).HasValue, Is.False); + Assert.That(DataVersion.FromToken(string.Empty).HasValue, Is.False); + } + } + + [Test] + public void A_numeric_token_is_rendered_invariantly() + { + Assert.That(DataVersion.FromToken(4611686018427387904L).ToString(), Is.EqualTo("4611686018427387904")); + } + + [Test] + public void Compose_is_stable_across_calls() + { + var first = DataVersion.Compose(("total", 3L), ("highestId", 7L)); + var second = DataVersion.Compose(("total", 3L), ("highestId", 7L)); + + Assert.That(first.Matches(second), Is.True); + } + + [Test] + public void Compose_moves_when_any_term_value_moves() + { + var before = DataVersion.Compose(("total", 3L), ("highestId", 7L)); + var after = DataVersion.Compose(("total", 3L), ("highestId", 8L)); + + Assert.That(before.Matches(after), Is.False); + } + + [Test] + public void Compose_distinguishes_terms_that_bare_concatenation_would_collide() + { + var first = DataVersion.Compose(("a", 1L), ("b", 23L)); + var second = DataVersion.Compose(("a", 12L), ("b", 3L)); + + Assert.That(first.Matches(second), Is.False); + } + + [Test] + public void Compose_moves_when_a_term_goes_from_absent_to_present() + { + DateTime? absent = null; + DateTime? present = new DateTime(2026, 7, 31, 0, 0, 0, DateTimeKind.Utc); + + var empty = DataVersion.Compose(("newest", absent)); + var populated = DataVersion.Compose(("newest", present)); + + Assert.That(empty.Matches(populated), Is.False); + } + + [Test] + public void Compose_with_no_terms_is_absent() + { + Assert.That(DataVersion.Compose().HasValue, Is.False); + } + + [Test] + public void OverRows_moves_when_a_row_changes_under_an_unchanged_timestamp() + { + var at = new DateTime(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc); + + var before = Page(("a", at, "Unresolved")); + var after = Page(("a", at, "Archived")); + + Assert.That(after.Matches(before), Is.False, + "two writes to one row inside a single clock tick leave the timestamp identical, so the version cannot rest on it alone"); + } + + [Test] + public void OverRows_distinguishes_two_pages_of_one_set() + { + var at = new DateTime(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc); + + var firstPage = Page(("a", at, "Unresolved")); + var secondPage = Page(("b", at, "Unresolved")); + + Assert.That(secondPage.Matches(firstPage), Is.False, + "the two pages render different rows, so a client holding one must not be told the other is current"); + } + + [Test] + public void OverRows_holds_while_the_page_and_the_total_hold() + { + var at = new DateTime(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc); + + Assert.That(Page(("a", at, "Unresolved")).Matches(Page(("a", at, "Unresolved"))), Is.True); + } + + [Test] + public void OverRows_moves_when_only_the_total_moves() + { + var at = new DateTime(2026, 8, 1, 12, 0, 0, DateTimeKind.Utc); + var row = ("a", at, "Unresolved"); + + Assert.That(DataVersion.OverRows([("total", 9L)], [row], Fields) + .Matches(DataVersion.OverRows([("total", 2L)], [row], Fields)), Is.False, + "Total-Count is part of the response, so a client holding the old one must not be told it is current"); + } + + [Test] + public void OverRows_distinguishes_rows_that_bare_concatenation_would_collide() + { + var left = DataVersion.OverRows([("total", 1L)], [("ab", "c")], row => [row.Item1, row.Item2]); + var right = DataVersion.OverRows([("total", 1L)], [("a", "bc")], row => [row.Item1, row.Item2]); + + Assert.That(right.Matches(left), Is.False, + "fields inside a row are length prefixed, so no value can pose as a different split of the same text"); + } + + [Test] + public void OverRows_refuses_a_missing_row_source() + { + Assert.Throws(() => DataVersion.OverRows([("total", 0L)], null, _ => [])); + } + + [Test] + public void OverRows_refuses_a_missing_field_selector() + { + Assert.Throws(() => DataVersion.OverRows([("total", 0L)], [], null)); + } + + static DataVersion Page(params (string Id, DateTime At, string Status)[] rows) => + DataVersion.OverRows([("total", 2L)], rows, Fields); + + static object[] Fields((string Id, DateTime At, string Status) row) => [row.Id, row.At, row.Status]; + + [Test] + public void Combine_does_not_depend_on_the_order_instances_answered_in() + { + var a = DataVersion.FromToken("a"); + var b = DataVersion.FromToken("b"); + + Assert.That(DataVersion.Combine([("one", a), ("two", b)]) + .Matches(DataVersion.Combine([("two", b), ("one", a)])), Is.True); + } + + [Test] + public void Combine_moves_when_two_instances_swap_which_version_they_report() + { + var a = DataVersion.FromToken("a"); + var b = DataVersion.FromToken("b"); + + Assert.That(DataVersion.Combine([("one", a), ("two", b)]) + .Matches(DataVersion.Combine([("one", b), ("two", a)])), Is.False, + "both instances changed, so a composite that only looked at the set of validators would answer 304 over stale data"); + } + + [Test] + public void Combine_differs_from_every_instance_version_it_covers() + { + var a = DataVersion.FromToken("a"); + var b = DataVersion.FromToken("b"); + + var combined = DataVersion.Combine([("one", a), ("two", b)]); + + using (Assert.EnterMultipleScope()) + { + Assert.That(combined.Matches(a), Is.False); + Assert.That(combined.Matches(b), Is.False); + } + } + + [Test] + public void Combine_is_absent_when_any_instance_has_no_version() + { + var combined = DataVersion.Combine([("one", DataVersion.FromToken("a")), ("two", DataVersion.None)]); + + Assert.That(combined.HasValue, Is.False, + "a composite that ignored an instance would stop reporting that instance's changes"); + } + + [Test] + public void Combine_of_nothing_is_absent() + { + Assert.That(DataVersion.Combine([]).HasValue, Is.False); + } + + [Test] + public void Compose_distinguishes_a_term_value_that_carries_the_delimiters() + { + var forged = DataVersion.Compose(("one", "a|two:1:b")); + var genuine = DataVersion.Compose(("one", "a"), ("two", "b")); + + Assert.That(forged.Matches(genuine), Is.False, + "a value able to pose as a longer term list would let a peer pin the composite version"); + } + + [Test] + public void Compose_refuses_a_term_whose_text_is_not_derived_from_its_content() + { + Assert.That(() => DataVersion.Compose(("rows", new object())), Throws.ArgumentException, + "a type name is a constant, so the version would never move and clients would cache forever"); + } + + [TestCase("\"abc\"", TestName = "FromClient_reads_a_quoted_validator")] + [TestCase("W/\"abc\"", TestName = "FromClient_reads_a_weak_validator")] + [TestCase("abc", TestName = "FromClient_reads_an_unquoted_validator_from_an_older_instance")] + public void FromClient_yields_the_bare_validator(string headerValue) + { + Assert.That(DataVersion.FromClient(headerValue).ToString(), Is.EqualTo("abc")); + } + + [TestCase(null)] + [TestCase("")] + [TestCase(" ")] + [TestCase("\"\"")] + public void FromClient_treats_a_blank_validator_as_absent(string headerValue) + { + Assert.That(DataVersion.FromClient(headerValue).HasValue, Is.False, + "a caller holding nothing must be a cache miss, never a match"); + } + + [Test] + public void FromClient_leaves_a_malformed_validator_alone_rather_than_truncating_it() + { + // Stripping every quote would truncate this into something that might match by accident. + Assert.That(DataVersion.FromClient("\"abc").ToString(), Is.EqualTo("\"abc")); + } + +} diff --git a/src/ServiceControl.UnitTests/Infrastructure/WebApi/ConditionalGetTests.cs b/src/ServiceControl.UnitTests/Infrastructure/WebApi/ConditionalGetTests.cs index 8a2c1e945a..3c804c7f70 100644 --- a/src/ServiceControl.UnitTests/Infrastructure/WebApi/ConditionalGetTests.cs +++ b/src/ServiceControl.UnitTests/Infrastructure/WebApi/ConditionalGetTests.cs @@ -9,6 +9,7 @@ namespace ServiceControl.UnitTests.Infrastructure.WebApi; using Microsoft.AspNetCore.Routing; using NUnit.Framework; using ServiceControl.Infrastructure.WebApi; +using ServiceControl.Persistence.Infrastructure; [TestFixture] public class ConditionalGetTests @@ -18,7 +19,7 @@ public void Repeating_a_request_with_the_etag_just_issued_is_not_modified() { var httpContext = new DefaultHttpContext(); - httpContext.Response.WithEtag("4611686018427387904"); + httpContext.Response.WithEtag(DataVersion.FromToken("4611686018427387904")); httpContext.Request.Headers.IfNoneMatch = httpContext.Response.Headers.ETag; @@ -36,7 +37,7 @@ public void A_different_etag_still_returns_the_payload() { var httpContext = new DefaultHttpContext(); - httpContext.Response.WithEtag("4611686018427387904"); + httpContext.Response.WithEtag(DataVersion.FromToken("4611686018427387904")); httpContext.Request.Headers.IfNoneMatch = "\"something-else\""; var context = ResultExecuting(httpContext); @@ -51,7 +52,7 @@ public void The_emitted_etag_is_a_well_formed_entity_tag() { var httpContext = new DefaultHttpContext(); - httpContext.Response.WithEtag("4611686018427387904"); + httpContext.Response.WithEtag(DataVersion.FromToken("4611686018427387904")); // RFC 9110 requires an entity-tag to be a quoted string. GetTypedHeaders parses through // EntityTagHeaderValue and yields null for anything else. @@ -60,35 +61,88 @@ public void The_emitted_etag_is_a_well_formed_entity_tag() } [Test] - public void A_deterministic_etag_is_a_well_formed_entity_tag() + public void An_absent_data_version_emits_no_header() { var httpContext = new DefaultHttpContext(); - httpContext.Response.WithDeterministicEtag("any-non-empty-payload-signature"); + httpContext.Response.WithEtag(DataVersion.None); - Assert.That(httpContext.Response.GetTypedHeaders().ETag, Is.Not.Null); + Assert.That(httpContext.Response.Headers.ContainsKey("ETag"), Is.False, + "an empty entity-tag is well formed, so it would match itself and answer 304 for unrelated payloads"); } [Test] - public void The_emitted_etag_quotes_the_value_without_altering_it() + public void A_paged_endpoint_emits_the_store_version_rather_than_a_hash_of_it() { var httpContext = new DefaultHttpContext(); + var version = DataVersion.FromToken("4611686018427387904"); - httpContext.Response.WithEtag("4611686018427387904"); + httpContext.Response.WithQueryStatsAndPagingInfo( + new QueryStatsInfo(version, totalCount: 1), + new PagingInfo()); - Assert.That(httpContext.Response.Headers.ETag.ToString(), Is.EqualTo("\"4611686018427387904\"")); + Assert.That(httpContext.Response.Headers.ETag.ToString(), Does.Contain(version.ToString())); } - [TestCase(null)] - [TestCase("")] - public void A_call_site_with_nothing_to_validate_emits_no_etag_header(string value) + [Test] + public void Every_emitted_etag_is_marked_weak() { var httpContext = new DefaultHttpContext(); - httpContext.Response.WithEtag(value); + httpContext.Response.WithEtag(DataVersion.FromToken("4611686018427387904")); - Assert.That(httpContext.Response.Headers.ContainsKey("ETag"), Is.False, - "an empty entity-tag is well formed, so it would match itself and answer 304 for unrelated payloads"); + using (Assert.EnterMultipleScope()) + { + Assert.That(httpContext.Response.Headers.ETag.ToString(), Is.EqualTo("W/\"4611686018427387904\"")); + Assert.That(httpContext.Response.GetTypedHeaders().ETag.IsWeak, Is.True); + } + } + + [Test] + public void An_unmarked_validator_from_an_older_client_still_matches() + { + var httpContext = new DefaultHttpContext(); + + httpContext.Response.WithEtag(DataVersion.FromToken("4611686018427387904")); + httpContext.Request.Headers.IfNoneMatch = "\"4611686018427387904\""; + + var context = ResultExecuting(httpContext); + + new NotModifiedStatusHttpHandler().OnResultExecuting(context); + + Assert.That(context.Result, Is.InstanceOf(), + "weak comparison ignores the W/ prefix, which is what carries a client through the upgrade"); + } + + [Test] + public void A_wildcard_precondition_is_not_modified_when_a_representation_exists() + { + var httpContext = new DefaultHttpContext(); + + httpContext.Response.WithEtag(DataVersion.FromToken("4611686018427387904")); + httpContext.Request.Headers.IfNoneMatch = "*"; + + var context = ResultExecuting(httpContext); + + new NotModifiedStatusHttpHandler().OnResultExecuting(context); + + Assert.That(context.Result, Is.InstanceOf(), + "RFC 9110: * matches whenever a current representation exists"); + } + + [Test] + public void A_wildcard_precondition_is_ignored_when_there_is_no_validator() + { + var httpContext = new DefaultHttpContext(); + + httpContext.Request.Headers.IfNoneMatch = "*"; + + var context = ResultExecuting(httpContext); + + new NotModifiedStatusHttpHandler().OnResultExecuting(context); + + Assert.That(context.Result, Is.InstanceOf(), + "an endpoint that publishes no validator has nothing for a client to have cached"); } static ResultExecutingContext ResultExecuting(HttpContext httpContext) => diff --git a/src/ServiceControl.UnitTests/Recoverability/MessageRedirectVersionTests.cs b/src/ServiceControl.UnitTests/Recoverability/MessageRedirectVersionTests.cs new file mode 100644 index 0000000000..f3bf580901 --- /dev/null +++ b/src/ServiceControl.UnitTests/Recoverability/MessageRedirectVersionTests.cs @@ -0,0 +1,82 @@ +namespace ServiceControl.UnitTests.Operations +{ + using System; + using System.Collections.Generic; + using NUnit.Framework; + using ServiceControl.Infrastructure.WebApi; + using ServiceControl.Persistence.Infrastructure; + using ServiceControl.Persistence.MessageRedirects; + + [TestFixture] + public class MessageRedirectVersionTests + { + [Test] + public void From_address_changed_should_change_version() + { + var knownVersion = VersionOf(Redirects(Redirect(from: "old@machine"))); + + var moved = VersionOf(Redirects(Redirect(from: "new@machine"))); + + Assert.That(moved.Matches(knownVersion), Is.False); + } + + [Test] + public void To_address_changed_should_change_version() + { + var redirect = Redirect(to: "old@machine"); + var data = Redirects(redirect); + + var knownVersion = VersionOf(data); + + redirect.ToPhysicalAddress = "new@machine"; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Last_modified_changed_should_change_version() + { + var redirect = Redirect(); + var data = Redirects(redirect); + + var knownVersion = VersionOf(data); + + redirect.LastModified = redirect.LastModified.AddTicks(1); + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Changing_item_count_should_change_version() + { + var emptyVersion = VersionOf(Redirects()); + + var oneRedirect = Redirects(Redirect()); + + Assert.That(VersionOf(oneRedirect).Matches(emptyVersion), Is.False, + "an empty list is a representation like any other, so this compares two real versions rather than a version against nothing"); + } + + [Test] + public void An_empty_list_still_reports_a_version() + { + var version = VersionOf(Redirects()); + + Assert.That(version.HasValue, Is.True, + "a client watching a list that stays empty must be able to revalidate it rather than refetch the emptiness"); + } + + static DataVersion VersionOf(IReadOnlyList redirects) => + ResponseVersions.VersionOf(redirects, redirects.Count); + + static IReadOnlyList Redirects(params MessageRedirect[] redirects) => redirects; + + static MessageRedirect Redirect(string from = "sales@machine", string to = "sales@other") => + new() + { + FromPhysicalAddress = from, + ToPhysicalAddress = to, + LastModified = new DateTime(2026, 8, 19, 10, 0, 0, DateTimeKind.Utc) + }; + } +} diff --git a/src/ServiceControl.UnitTests/Recoverability/RetryGroupEtagHelperTests.cs b/src/ServiceControl.UnitTests/Recoverability/RetryGroupEtagHelperTests.cs deleted file mode 100644 index d0cc1cb0fd..0000000000 --- a/src/ServiceControl.UnitTests/Recoverability/RetryGroupEtagHelperTests.cs +++ /dev/null @@ -1,122 +0,0 @@ -namespace ServiceControl.UnitTests.Operations -{ - using System; - using NUnit.Framework; - using ServiceControl.Recoverability; - - [TestFixture] - public class RetryGroupEtagHelperTests - { - [Test] - public void Id_changed_should_change_etag() - { - var group = new GroupOperation { Id = "old" }; - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.Id = "new"; - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void Count_changed_should_change_etag() - { - var group = new GroupOperation { Count = 1 }; - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.Count = 2; - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void RetryStatus_changed_should_change_etag() - { - var group = new GroupOperation { OperationStatus = RetryState.Waiting.ToString() }; - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.OperationStatus = RetryState.Preparing.ToString(); - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void RetryProgress_changed_should_change_etag() - { - var group = new GroupOperation(); - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.OperationProgress = 0.01; - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void RetryStartTime_changed_should_change_etag() - { - var group = new GroupOperation(); - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.OperationStartTime = DateTime.UtcNow; - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void RetryCompletionTime_changed_should_change_etag() - { - var group = new GroupOperation(); - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.OperationCompletionTime = DateTime.UtcNow; - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void NeedUserAcknowledgement_changed_should_change_etag() - { - var group = new GroupOperation(); - var data = new[] { group }; - - var knownEtag = EtagHelper.CalculateEtag(data); - - group.NeedUserAcknowledgement = true; - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - - [Test] - public void Changing_item_count_should_change_etag() - { - var data = new GroupOperation[0]; - var knownEtag = EtagHelper.CalculateEtag(data); - - var group = new GroupOperation(); - data = new[] { group }; - - var newEtag = EtagHelper.CalculateEtag(data); - - Assert.That(newEtag, Is.Not.EqualTo(knownEtag)); - } - } -} \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/Recoverability/RetryGroupVersionTests.cs b/src/ServiceControl.UnitTests/Recoverability/RetryGroupVersionTests.cs new file mode 100644 index 0000000000..4ff7b4961d --- /dev/null +++ b/src/ServiceControl.UnitTests/Recoverability/RetryGroupVersionTests.cs @@ -0,0 +1,265 @@ +namespace ServiceControl.UnitTests.Operations +{ + using System; + using System.Collections.Generic; + using NUnit.Framework; + using ServiceControl.Infrastructure.WebApi; + using ServiceControl.Persistence.Infrastructure; + using ServiceControl.Recoverability; + + [TestFixture] + public class RetryGroupVersionTests + { + [Test] + public void Id_changed_should_change_version() + { + var group = new GroupOperation { Id = "old" }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.Id = "new"; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Count_changed_should_change_version() + { + var group = new GroupOperation { Count = 1 }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.Count = 2; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void RetryStatus_changed_should_change_version() + { + var group = new GroupOperation { OperationStatus = RetryState.Waiting.ToString() }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationStatus = RetryState.Preparing.ToString(); + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void RetryProgress_changed_should_change_version() + { + var group = new GroupOperation(); + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationProgress = 0.01; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void RetryStartTime_changed_should_change_version() + { + var group = new GroupOperation(); + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationStartTime = DateTime.UtcNow; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void RetryCompletionTime_changed_should_change_version() + { + var group = new GroupOperation(); + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationCompletionTime = DateTime.UtcNow; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void NeedUserAcknowledgement_changed_should_change_version() + { + var group = new GroupOperation(); + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.NeedUserAcknowledgement = true; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Comment_changed_should_change_version() + { + var group = new GroupOperation { Comment = "before" }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.Comment = "after"; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Title_changed_should_change_version() + { + var group = new GroupOperation { Title = "before" }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.Title = "after"; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Type_changed_should_change_version() + { + var group = new GroupOperation { Type = "before" }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.Type = "after"; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void First_changed_should_change_version() + { + var group = new GroupOperation(); + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.First = DateTime.UtcNow; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Last_changed_should_change_version() + { + var group = new GroupOperation(); + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.Last = DateTime.UtcNow; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void OperationFailed_changed_should_change_version() + { + var group = new GroupOperation { OperationFailed = false }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationFailed = true; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void OperationMessagesCompletedCount_changed_should_change_version() + { + var group = new GroupOperation { OperationMessagesCompletedCount = 1 }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationMessagesCompletedCount = 2; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void OperationRemainingCount_changed_should_change_version() + { + var group = new GroupOperation { OperationRemainingCount = 2 }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationRemainingCount = 1; + + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void A_message_completing_moves_the_version_even_when_the_rounded_progress_does_not() + { + // Progress is rounded to two decimals, so on a retry this big one message does not move it. + var group = new GroupOperation + { + Id = "retry-1", + OperationStatus = "Forwarding", + OperationProgress = 0.2, + OperationMessagesCompletedCount = 10_000, + OperationRemainingCount = 40_000 + }; + var data = new[] { group }; + + var knownVersion = VersionOf(data); + + group.OperationMessagesCompletedCount = 10_001; + group.OperationRemainingCount = 39_999; + + Assert.That(group.OperationProgress, Is.EqualTo(0.2), "the premise: the rounded percentage has not moved"); + Assert.That(VersionOf(data).Matches(knownVersion), Is.False); + } + + [Test] + public void Changing_item_count_should_change_version() + { + var emptyVersion = VersionOf(Array.Empty()); + + var oneGroup = new[] { new GroupOperation() }; + + Assert.That(VersionOf(oneGroup).Matches(emptyVersion), Is.False, + "an empty list is a representation like any other, so this compares two real versions rather than a version against nothing"); + } + + [Test] + public void A_title_carrying_a_delimiter_cannot_impersonate_the_next_field() + { + var carrying = VersionOf([new GroupOperation { Title = "Shipping.Exception", Type = string.Empty }]); + var split = VersionOf([new GroupOperation { Title = "Shipping", Type = "Exception" }]); + + Assert.That(carrying.Matches(split), Is.False, + "two groups a client can tell apart must not share a validator"); + } + + [Test] + public void Two_groups_cannot_digest_as_one_carrying_a_delimiter() + { + var two = VersionOf([new GroupOperation { Id = "a" }, new GroupOperation { Id = "b" }]); + var oneForging = VersionOf([new GroupOperation { Id = "a|row1:1:b" }]); + + Assert.That(oneForging.Matches(two), Is.False, + "a row able to pose as a longer row list would let user text pin the validator"); + } + + static DataVersion VersionOf(IReadOnlyList groups) => + ResponseVersions.VersionOf(groups, groups.Count); + } +} diff --git a/src/ServiceControl.UnitTests/ScatterGather/MessageView_ScatterGatherTest.cs b/src/ServiceControl.UnitTests/ScatterGather/MessageView_ScatterGatherTest.cs index 7f0c779e38..8fc1cf9225 100644 --- a/src/ServiceControl.UnitTests/ScatterGather/MessageView_ScatterGatherTest.cs +++ b/src/ServiceControl.UnitTests/ScatterGather/MessageView_ScatterGatherTest.cs @@ -39,7 +39,7 @@ QueryResult> GetPage(IEnumerable source, strin return new QueryResult>( pageOfResults, - new QueryStatsInfo(etag, allResults.Count, isStale: false)) + new QueryStatsInfo(DataVersion.FromToken(etag), allResults.Count)) { InstanceId = instanceId }; diff --git a/src/ServiceControl.UnitTests/ScatterGather/MessagesView_ScatterGather_DataFromBothInstances.cs b/src/ServiceControl.UnitTests/ScatterGather/MessagesView_ScatterGather_DataFromBothInstances.cs index 7a2dc205a9..69ac74e3b5 100644 --- a/src/ServiceControl.UnitTests/ScatterGather/MessagesView_ScatterGather_DataFromBothInstances.cs +++ b/src/ServiceControl.UnitTests/ScatterGather/MessagesView_ScatterGather_DataFromBothInstances.cs @@ -24,10 +24,13 @@ public void HasResults() [Test] public void ResultingETagIsDifferentFromBothInstanceSpecificETags() { - var resultingEtag = Results.QueryStats.ETag; + var resultingVersion = Results.QueryStats.Version; - Assert.That(resultingEtag, Is.Not.EqualTo(LocalETag), "Resulting etag should not equal local etag"); - Assert.That(resultingEtag, Is.Not.EqualTo(RemoteETag), "Resulting etag should not equal remote etag"); + Assert.Multiple(() => + { + Assert.That(resultingVersion.Matches(DataVersion.FromToken(LocalETag)), Is.False, "Resulting version should not equal local version"); + Assert.That(resultingVersion.Matches(DataVersion.FromToken(RemoteETag)), Is.False, "Resulting version should not equal remote version"); + }); } [Test] diff --git a/src/ServiceControl.UnitTests/ScatterGather/RemoteInstanceEtagTests.cs b/src/ServiceControl.UnitTests/ScatterGather/RemoteInstanceEtagTests.cs index e66b2e551e..eee9ae65db 100644 --- a/src/ServiceControl.UnitTests/ScatterGather/RemoteInstanceEtagTests.cs +++ b/src/ServiceControl.UnitTests/ScatterGather/RemoteInstanceEtagTests.cs @@ -3,10 +3,12 @@ namespace ServiceControl.UnitTests.ScatterGather using System.Net.Http; using CompositeViews.Messages; using NUnit.Framework; + using ServiceControl.Persistence.Infrastructure; [TestFixture] public class RemoteInstanceEtagTests { + [TestCase("W/\"4611686018427387904\"", TestName = "A_remote_etag_is_read_when_the_instance_marks_it_weak")] [TestCase("\"4611686018427387904\"", TestName = "A_remote_etag_is_read_when_the_instance_quotes_it")] [TestCase("4611686018427387904", TestName = "A_remote_etag_is_read_when_the_instance_predates_the_conditional_get_fix")] public void A_remote_etag_is_read(string asSentByTheRemoteInstance) @@ -14,8 +16,8 @@ public void A_remote_etag_is_read(string asSentByTheRemoteInstance) var response = new HttpResponseMessage(); response.Headers.TryAddWithoutValidation("ETag", asSentByTheRemoteInstance); - Assert.That(ScatterGatherApiBase.ReadEtag(response.Headers), Is.EqualTo("4611686018427387904"), - "a rolling upgrade runs both shapes side by side, so both have to be understood"); + Assert.That(ScatterGatherApiBase.ReadEtag(response.Headers).ToString(), Is.EqualTo("4611686018427387904"), + "a rolling upgrade runs all three shapes side by side, so all three have to be understood"); } [Test] @@ -23,7 +25,7 @@ public void An_instance_that_sends_no_etag_contributes_nothing() { var response = new HttpResponseMessage(); - Assert.That(ScatterGatherApiBase.ReadEtag(response.Headers), Is.Null); + Assert.That(ScatterGatherApiBase.ReadEtag(response.Headers).HasValue, Is.False); } } } diff --git a/src/ServiceControl.UnitTests/ScatterGather/ScatterGatherVersionTests.cs b/src/ServiceControl.UnitTests/ScatterGather/ScatterGatherVersionTests.cs new file mode 100644 index 0000000000..22eebc1e3f --- /dev/null +++ b/src/ServiceControl.UnitTests/ScatterGather/ScatterGatherVersionTests.cs @@ -0,0 +1,104 @@ +namespace ServiceControl.UnitTests.ScatterGather +{ + using System.Collections.Generic; + using System.Linq; + using System.Threading; + using System.Threading.Tasks; + using CompositeViews.Messages; + using Microsoft.Extensions.Logging.Abstractions; + using NUnit.Framework; + using Persistence.Infrastructure; + using ServiceBus.Management.Infrastructure.Settings; + + [TestFixture] + class ScatterGatherVersionTests + { + [Test] + public void A_composite_is_absent_when_an_instance_reports_no_version() + { + var api = new LocalAndRemoteApi(); + + var everyone = api.AggregateResults(Context(), [Page("local", "a"), Page("remote", "b")]); + var oneSilent = api.AggregateResults(Context(), [Page("local", "a"), Page("remote", null)]); + + using (Assert.EnterMultipleScope()) + { + Assert.That(everyone.QueryStats.Version.HasValue, Is.True, "every instance answered, so the composite covers them all"); + Assert.That(oneSilent.QueryStats.Version.HasValue, Is.False, + "a composite that ignored an instance would stop reporting that instance's changes"); + } + } + + [Test] + public void A_composite_moves_when_one_instance_moves() + { + var api = new LocalAndRemoteApi(); + + var before = api.AggregateResults(Context(), [Page("local", "a"), Page("remote", "b")]); + var after = api.AggregateResults(Context(), [Page("local", "a"), Page("remote", "c")]); + + Assert.That(after.QueryStats.Version.Matches(before.QueryStats.Version), Is.False, + "one instance reported new data, so the composite cannot stay put"); + } + + [Test] + public void A_remote_only_api_reports_the_version_of_the_instances_that_have_the_data() + { + var api = new RemoteOnlyApi(new Settings()); + + var composite = api.AggregateResults(Context(), [NoLocalData(), Page("remote", "b")]); + + Assert.That(composite.QueryStats.Version.HasValue, Is.True, + "the remote answered with a version, so discarding it would leave the response with no ETag at all"); + } + + [Test] + public void A_remote_only_api_still_covers_a_remote_configured_with_its_own_instance_id() + { + var settings = new Settings(); + var api = new RemoteOnlyApi(settings); + + var composite = api.AggregateResults(Context(), [NoLocalData(), Page(settings.InstanceId, "b")]); + + Assert.That(composite.QueryStats.Version.HasValue, Is.True, + "dropping a remote because it answers to the local instance id would promise coverage the composite does not have"); + } + + [Test] + public void A_remote_only_api_reports_no_version_when_no_remote_answered() + { + var api = new RemoteOnlyApi(new Settings()); + + var composite = api.AggregateResults(Context(), [NoLocalData()]); + + Assert.That(composite.QueryStats.Version.HasValue, Is.False, + "nothing reported a version, so there is nothing to promise a caller"); + } + + static ScatterGatherApiMessageViewContext Context() => new(new PagingInfo(), new SortInfo()); + + static QueryResult> Page(string instanceId, string validator) => + new([new MessagesView { MessageId = instanceId }], new QueryStatsInfo(DataVersion.FromToken(validator), 1)) + { + InstanceId = instanceId + }; + + // What ScatterGatherRemoteOnly.LocalQuery returns: no rows and no version. + static QueryResult> NoLocalData() => + new(null, QueryStatsInfo.Zero) { IsLocalInstance = true }; + + class LocalAndRemoteApi() : ScatterGatherApiMessageView( + null, null, null, null, NullLogger.Instance) + { + protected override Task>> LocalQuery(ScatterGatherApiMessageViewContext input, CancellationToken cancellationToken = default) => + throw new System.NotImplementedException(); + } + + class RemoteOnlyApi(Settings settings) : ScatterGatherRemoteOnly>( + settings, null, null, NullLogger.Instance) + { + protected override IList ProcessResults(ScatterGatherApiMessageViewContext input, QueryResult>[] results) => + [.. results.Where(result => result.Results is not null).SelectMany(result => result.Results)]; + } + } +} diff --git a/src/ServiceControl/CompositeViews/AuditCounts/GetAuditCountsForEndpointApi.cs b/src/ServiceControl/CompositeViews/AuditCounts/GetAuditCountsForEndpointApi.cs index dbf265b0b6..1455b6d8d6 100644 --- a/src/ServiceControl/CompositeViews/AuditCounts/GetAuditCountsForEndpointApi.cs +++ b/src/ServiceControl/CompositeViews/AuditCounts/GetAuditCountsForEndpointApi.cs @@ -3,13 +3,10 @@ using System.Collections.Generic; using System.Linq; using System.Net.Http; - using System.Threading; - using System.Threading.Tasks; using Api.Contracts; using Messages; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging; - using Persistence; using Persistence.Infrastructure; using ServiceBus.Management.Infrastructure.Settings; @@ -19,22 +16,17 @@ // gather approach here. public record AuditCountsForEndpointContext(PagingInfo PagingInfo, string Endpoint) : ScatterGatherContext(PagingInfo); + // The counts only ever live on an audit instance, so this instance has nothing of its own to add. public class GetAuditCountsForEndpointApi( - IMessagesViewDataStore dataStore, Settings settings, IHttpClientFactory httpClientFactory, IHttpContextAccessor httpContextAccessor, ILogger logger) - : ScatterGatherApi>(dataStore, settings, httpClientFactory, httpContextAccessor, logger) + : ScatterGatherRemoteOnly>(settings, httpClientFactory, httpContextAccessor, logger) { - static readonly IList Empty = new List(0).AsReadOnly(); - - protected override Task>> LocalQuery(AuditCountsForEndpointContext input, CancellationToken cancellationToken = default) => - // Will never be implemented on the primary instance - Task.FromResult(new QueryResult>(Empty, QueryStatsInfo.Zero)); - protected override IList ProcessResults(AuditCountsForEndpointContext input, QueryResult>[] results) => - results.SelectMany(r => r.Results) + results.Where(r => r.Results is not null) + .SelectMany(r => r.Results) .GroupBy(r => r.UtcDate) .Select(g => new AuditCount { diff --git a/src/ServiceControl/CompositeViews/Messages/GetMessages2Controller.cs b/src/ServiceControl/CompositeViews/Messages/GetMessages2Controller.cs index df6dec1155..6c1af26c31 100644 --- a/src/ServiceControl/CompositeViews/Messages/GetMessages2Controller.cs +++ b/src/ServiceControl/CompositeViews/Messages/GetMessages2Controller.cs @@ -66,6 +66,7 @@ public async Task> Messages( } Response.WithTotalCount(result.QueryStats.TotalCount); + Response.WithEtag(result.QueryStats.Version); return result.Results; } diff --git a/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs b/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs index e4de1674d7..1d2b31e954 100644 --- a/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs +++ b/src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs @@ -100,7 +100,7 @@ public async Task Get(string id, [FromQuery(Name = "instance_id") return NoContent(); } - Response.WithEtag(result.Content.Etag); + Response.WithEtag(result.Content.Version); return File(result.Content.Stream, result.Content.ContentType ?? "text/*"); } diff --git a/src/ServiceControl/CompositeViews/Messages/ScatterGatherApi.cs b/src/ServiceControl/CompositeViews/Messages/ScatterGatherApi.cs index d0444a2068..9d4b677fc5 100644 --- a/src/ServiceControl/CompositeViews/Messages/ScatterGatherApi.cs +++ b/src/ServiceControl/CompositeViews/Messages/ScatterGatherApi.cs @@ -20,19 +20,12 @@ interface IApi; // Non-generic, so statics live once rather than once per closed generic instantiation. public abstract class ScatterGatherApiBase { - internal static string ReadEtag(HttpResponseHeaders headers) - { - // Read raw rather than through headers.ETag. An instance predating the quoted validator - // sends a bare token, which EntityTagHeaderValue fails to parse and discards silently. - if (!headers.TryGetValues("ETag", out var values)) - { - return null; - } - - var etag = values.FirstOrDefault(); - - return etag?.Length > 1 && etag[0] == '"' && etag[^1] == '"' ? etag[1..^1] : etag; - } + // Read raw, not via headers.ETag: an older instance sends an unquoted tag that + // EntityTagHeaderValue cannot parse and drops without a word. + internal static DataVersion ReadEtag(HttpResponseHeaders headers) => + headers.TryGetValues("ETag", out var values) + ? DataVersion.FromClient(values.FirstOrDefault()) + : DataVersion.None; } public record ScatterGatherContext(PagingInfo PagingInfo); @@ -51,6 +44,7 @@ protected ScatterGatherApi(TDataStore store, Settings settings, IHttpClientFacto } protected TDataStore DataStore { get; } + Settings Settings { get; } IHttpClientFactory HttpClientFactory { get; } IHttpContextAccessor HttpContextAccessor { get; } @@ -86,6 +80,7 @@ async Task> LocalCall(TIn input, string instanceId, Cancellati { var result = await LocalQuery(input, cancellationToken); result.InstanceId = instanceId; + result.IsLocalInstance = true; return result; } @@ -103,14 +98,31 @@ internal QueryResult AggregateResults(TIn input, QueryResult[] resul protected abstract TOut ProcessResults(TIn input, QueryResult[] results); - protected virtual QueryStatsInfo AggregateStats(TIn input, IEnumerable> results, TOut processedResults) + protected virtual QueryStatsInfo AggregateStats(TIn input, IEnumerable> results, TOut processedResults) => + Aggregate(results); + + /// + /// For an API whose own instance holds none of the data. Its local result carries no version, and + /// reports as soon as one result + /// is missing one, which would leave every response with no ETag at all. + /// + protected static QueryStatsInfo AggregateStatsFromRemotesOnly(IEnumerable> results) => + Aggregate(results.Where(result => !result.IsLocalInstance)); + + static QueryStatsInfo Aggregate(IEnumerable> results) { - var infos = results.Select(x => x.QueryStats).ToArray(); + var reported = results.ToArray(); + + if (reported.Length == 0) + { + return QueryStatsInfo.Zero; + } + + var infos = reported.Select(x => x.QueryStats).ToArray(); return new QueryStatsInfo( - string.Concat(infos.OrderBy(x => x.ETag).Select(x => x.ETag)), + DataVersion.Combine(reported.Select(result => (result.InstanceId, result.QueryStats.Version))), infos.Sum(x => x.TotalCount), - isStale: infos.Any(x => x.IsStale), infos.Max(x => x.HighestTotalCountOfAllTheInstances) ); } @@ -191,11 +203,9 @@ static async Task> ParseResult(HttpResponseMessage responseMes totalCount = int.Parse(totalCounts.ElementAt(0)); } - // Unquoted, because AggregateStats concatenates it with the other instances' values and - // the result is re-tagged before it goes back on the wire. var etag = ReadEtag(responseMessage.Headers); - return new QueryResult(remoteResults, new QueryStatsInfo(etag, totalCount, isStale: false)); + return new QueryResult(remoteResults, new QueryStatsInfo(etag, totalCount)); } readonly ILogger logger; diff --git a/src/ServiceControl/CompositeViews/Messages/ScatterGatherRemoteOnly.cs b/src/ServiceControl/CompositeViews/Messages/ScatterGatherRemoteOnly.cs index 379070101b..f867a39694 100644 --- a/src/ServiceControl/CompositeViews/Messages/ScatterGatherRemoteOnly.cs +++ b/src/ServiceControl/CompositeViews/Messages/ScatterGatherRemoteOnly.cs @@ -1,5 +1,6 @@ namespace ServiceControl.CompositeViews.Messages { + using System.Collections.Generic; using System.Net.Http; using System.Threading; using System.Threading.Tasks; @@ -14,6 +15,9 @@ public abstract class ScatterGatherRemoteOnly(Settings settings, IHtt where TOut : class { protected sealed override Task> LocalQuery(TIn input, CancellationToken cancellationToken = default) => QueryResult.Empty(); + + protected sealed override QueryStatsInfo AggregateStats(TIn input, IEnumerable> results, TOut processedResults) => + AggregateStatsFromRemotesOnly(results); } public sealed class NoOpStore diff --git a/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs b/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs index 295b30ef90..75f221890b 100644 --- a/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs +++ b/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs @@ -25,8 +25,7 @@ public async Task> CustomChecks([FromQuery] PagingInfo paging { var stats = await checksDataStore.GetStats(pagingInfo, status, cancellationToken); - Response.WithPagingLinksAndTotalCount(pagingInfo, stats.QueryStats.TotalCount); - Response.WithEtag(stats.QueryStats.ETag); + Response.WithQueryStatsAndPagingInfo(stats.QueryStats, pagingInfo); return stats.Results; } diff --git a/src/ServiceControl/EventLog/EventLogApiController.cs b/src/ServiceControl/EventLog/EventLogApiController.cs index 824c5240bf..3d76ca4221 100644 --- a/src/ServiceControl/EventLog/EventLogApiController.cs +++ b/src/ServiceControl/EventLog/EventLogApiController.cs @@ -1,7 +1,6 @@ namespace ServiceControl.EventLog { using System.Collections.Generic; - using System.Net; using System.Threading; using System.Threading.Tasks; using Infrastructure.Auth; @@ -20,16 +19,9 @@ public class EventLogApiController(IEventLogDataStore logDataStore) : Controller [HttpGet] public async Task>> Items([FromQuery] PagingInfo pagingInfo, CancellationToken cancellationToken = default) { - // Passing knownVersion lets the persister skip work it would otherwise waste - var result = await logDataStore.GetEventLogItems(pagingInfo, Request.GetKnownVersion(), cancellationToken); + var result = await logDataStore.GetEventLogItems(pagingInfo, cancellationToken); - Response.WithPagingLinksAndTotalCount(pagingInfo, result.QueryStats.TotalCount); - Response.WithEtag(result.QueryStats.ETag); - - if (result.NotModified) - { - return StatusCode((int)HttpStatusCode.NotModified); - } + Response.WithQueryStatsAndPagingInfo(result.QueryStats, pagingInfo); return Ok(result.Results); } diff --git a/src/ServiceControl/Infrastructure/WebApi/HttpRequestExtensions.cs b/src/ServiceControl/Infrastructure/WebApi/HttpRequestExtensions.cs deleted file mode 100644 index f4210fde5a..0000000000 --- a/src/ServiceControl/Infrastructure/WebApi/HttpRequestExtensions.cs +++ /dev/null @@ -1,26 +0,0 @@ -namespace ServiceControl.Infrastructure.WebApi -{ - using System.Linq; - using Microsoft.AspNetCore.Http; - - static class HttpRequestExtensions - { - /// - /// The validator the caller already holds, unquoted so it can be compared against a store's - /// own version, or null if the caller holds none. - /// - /// Only meaningful for an endpoint that publishes its validator through - /// . An endpoint publishing through - /// WithDeterministicEtag hashes the validator on the way out, so what a client echoes - /// back cannot be compared with anything a store holds and this would never match. - /// - /// - public static string GetKnownVersion(this HttpRequest request) => - Unquote(request.Headers.IfNoneMatch.FirstOrDefault()); - - // Trimming every quote instead would turn a malformed header - // into a truncated value rather than into the cache miss it should be. - static string Unquote(string etag) => - etag?.Length > 1 && etag[0] == '"' && etag[^1] == '"' ? etag[1..^1] : etag; - } -} diff --git a/src/ServiceControl/Infrastructure/WebApi/HttpResponseExtensions.cs b/src/ServiceControl/Infrastructure/WebApi/HttpResponseExtensions.cs index 226148016a..04190dd0e5 100644 --- a/src/ServiceControl/Infrastructure/WebApi/HttpResponseExtensions.cs +++ b/src/ServiceControl/Infrastructure/WebApi/HttpResponseExtensions.cs @@ -13,35 +13,22 @@ static class HttpResponseExtensions { public static void WithTotalCount(this HttpResponse response, long totalCount) => response.WithHeader("Total-Count", totalCount.ToString(CultureInfo.InvariantCulture)); - public static void WithEtag(this HttpResponse response, StringValues value) + public static void WithEtag(this HttpResponse response, DataVersion version) { - var validator = value.ToString(); - - if (string.IsNullOrEmpty(validator)) + if (!version.HasValue) { return; } - // RFC 9110 requires an entity-tag to be a quoted string. Unquoted, EntityTagHeaderValue - // cannot parse it and NotModifiedStatusHttpHandler never matches a client's If-None-Match. - response.Headers.ETag = $"\"{validator}\""; + // Quotes are required by RFC 9110. Without them EntityTagHeaderValue cannot parse the tag and + // NotModifiedStatusHttpHandler never matches a client's If-None-Match. + response.Headers.ETag = $"W/\"{version}\""; } public static void WithQueryStatsInfo(this HttpResponse response, QueryStatsInfo queryStatsInfo) { response.WithTotalCount(queryStatsInfo.TotalCount); - response.WithEtag(queryStatsInfo.ETag); - } - - public static void WithDeterministicEtag(this HttpResponse response, string data) - { - if (string.IsNullOrEmpty(data)) - { - return; - } - - var guid = DeterministicGuid.MakeId(data); - response.WithEtag(guid.ToString()); + response.WithEtag(queryStatsInfo.Version); } static void WithHeader(this HttpResponse response, string header, StringValues value) => response.Headers.Append(header, value); @@ -103,14 +90,13 @@ static void AddLink(ICollection links, int page, string rel, string uriP public static void WithQueryStatsAndPagingInfo(this HttpResponse response, QueryStatsInfo queryStats, PagingInfo pagingInfo) { response.WithPagingLinksAndTotalCount(pagingInfo, queryStats.TotalCount, queryStats.HighestTotalCountOfAllTheInstances); - response.WithDeterministicEtag(queryStats.ETag); + response.WithEtag(queryStats.Version); } - public static void WithPagingLinksAndTotalCount(this HttpResponse response, - PagingInfo pagingInfo, long totalCount, long highestTotalCountOfAllInstances = 1) + public static void WithPagingLinksAndTotalCount(this HttpResponse response, PagingInfo pagingInfo, long totalCount, long? highestTotalCountOfAllInstances = null) { response.WithTotalCount(totalCount); - response.WithPagingLinks(pagingInfo, highestTotalCountOfAllInstances, totalCount); + response.WithPagingLinks(pagingInfo, highestTotalCountOfAllInstances ?? totalCount, totalCount); } } } \ No newline at end of file diff --git a/src/ServiceControl/Infrastructure/WebApi/NotModifiedStatusHttpHandler.cs b/src/ServiceControl/Infrastructure/WebApi/NotModifiedStatusHttpHandler.cs index 6554f17844..55cf5c0504 100644 --- a/src/ServiceControl/Infrastructure/WebApi/NotModifiedStatusHttpHandler.cs +++ b/src/ServiceControl/Infrastructure/WebApi/NotModifiedStatusHttpHandler.cs @@ -1,16 +1,30 @@ namespace ServiceControl.Infrastructure.WebApi { using System; + using System.Linq; using System.Net; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Headers; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; + using Microsoft.AspNetCore.Mvc.Infrastructure; class NotModifiedStatusHttpHandler : IResultFilter { - static bool IfNoneMatch(RequestHeaders requestHeaders, ResponseHeaders responseHeaders) => - responseHeaders.ETag != null && requestHeaders.IfNoneMatch.Contains(responseHeaders.ETag); + static bool IfNoneMatch(RequestHeaders requestHeaders, ResponseHeaders responseHeaders) + { + var current = responseHeaders.ETag; + + if (current is null) + { + return false; + } + + // EntityTagHeaderValue.Equals compares strength as well as tag, and its own documentation + // says not to use it for this. RFC 9110 requires If-None-Match to use weak comparison. + return requestHeaders.IfNoneMatch.Any(candidate => + candidate.Tag.Equals("*", StringComparison.Ordinal) || candidate.Compare(current, useStrongComparison: false)); + } static bool IfNotModifiedSince(DateTimeOffset? ifModifiedSince, DateTimeOffset? lastModified) => lastModified <= ifModifiedSince; @@ -23,8 +37,7 @@ public void OnResultExecuting(ResultExecutingContext context) return; } - var statusCode = context.HttpContext.Response.StatusCode; - if (statusCode is < 200 or > 299) + if (!IsSuccess(context)) { return; } @@ -37,10 +50,23 @@ public void OnResultExecuting(ResultExecutingContext context) if (ifNoneMatch || ifNotModifiedSince) { + // The replaced result never executes, so whatever it owned would never be released. + if (context.Result is FileStreamResult file) + { + context.HttpContext.Response.RegisterForDisposeAsync(file.FileStream); + } + context.Result = new StatusCodeResult((int)HttpStatusCode.NotModified); } } + // Response.StatusCode is still whatever the pipeline defaulted to, because the result that + // would set it has not executed yet. + static bool IsSuccess(ResultExecutingContext context) => + context.Result is IStatusCodeActionResult { StatusCode: { } statusCode } + ? statusCode is >= 200 and <= 299 + : context.HttpContext.Response.StatusCode is >= 200 and <= 299; + public void OnResultExecuted(ResultExecutedContext context) { // NOP diff --git a/src/ServiceControl/Infrastructure/WebApi/ResponseVersions.cs b/src/ServiceControl/Infrastructure/WebApi/ResponseVersions.cs new file mode 100644 index 0000000000..3ad86916a9 --- /dev/null +++ b/src/ServiceControl/Infrastructure/WebApi/ResponseVersions.cs @@ -0,0 +1,28 @@ +namespace ServiceControl.Infrastructure.WebApi; + +using System.Collections.Generic; +using ServiceControl.Persistence.Infrastructure; +using ServiceControl.Persistence.MessageRedirects; + +/// +/// Versions for responses a controller assembles itself, rather than getting from a store. Each takes the +/// total as well as the rows: it carries whatever the response says about the whole set, and it keeps an +/// empty list cacheable, because with no rows and no summary there would be no terms to compose and so no +/// validator at all. +/// +static class ResponseVersions +{ + internal static DataVersion VersionOf(IReadOnlyList page, int total) => + DataVersion.OverRows( + [("groups", total)], + page, + group => [group.Id, group.Title, group.Type, group.Count, group.Comment, group.First, group.Last, + group.OperationStatus, group.OperationFailed, group.OperationProgress, group.OperationMessagesCompletedCount, + group.OperationRemainingCount, group.OperationStartTime, group.OperationCompletionTime, group.NeedUserAcknowledgement]); + + internal static DataVersion VersionOf(IReadOnlyList page, int total) => + DataVersion.OverRows( + [("redirects", total)], + page, + redirect => [redirect.MessageRedirectId, redirect.ToPhysicalAddress, redirect.LastModified]); +} diff --git a/src/ServiceControl/MessageFailures/Api/ArchiveMessagesController.cs b/src/ServiceControl/MessageFailures/Api/ArchiveMessagesController.cs index 7c5ab51504..965bd9a8f2 100644 --- a/src/ServiceControl/MessageFailures/Api/ArchiveMessagesController.cs +++ b/src/ServiceControl/MessageFailures/Api/ArchiveMessagesController.cs @@ -1,6 +1,5 @@ namespace ServiceControl.MessageFailures.Api { - using System; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -48,11 +47,11 @@ await auditLog.AuditedOperation(user, MessageActionKind.Archive, Permissions.Err [HttpGet] public async Task GetArchiveMessageGroups(string classifier = "Exception Type and Stack Trace", CancellationToken cancellationToken = default) { - var results = await dataStore.GetArchivedGroupsByClassifier(classifier, cancellationToken); + var result = await dataStore.GetArchivedGroupsByClassifier(classifier, cancellationToken); - Response.WithDeterministicEtag(EtagHelper.CalculateEtag(results)); + Response.WithEtag(result.QueryStats.Version); - return Ok(results); + return Ok(result.Results); } [Authorize(Policy = Permissions.ErrorMessagesArchive)] @@ -77,7 +76,7 @@ public async Task> GetGroup(string groupId, strin { var result = await dataStore.GetArchivedGroup(groupId, status, modified, cancellationToken); - Response.WithEtag(result.QueryStats.ETag); + Response.WithEtag(result.QueryStats.Version); return result.Results == null ? NotFound() : result.Results; } diff --git a/src/ServiceControl/MessageRedirects/Api/MessageRedirectsController.cs b/src/ServiceControl/MessageRedirects/Api/MessageRedirectsController.cs index f3c8e61c0f..765e8b3d46 100644 --- a/src/ServiceControl/MessageRedirects/Api/MessageRedirectsController.cs +++ b/src/ServiceControl/MessageRedirects/Api/MessageRedirectsController.cs @@ -171,7 +171,7 @@ public async Task CountRedirects(CancellationToken cancellationToken = default) { var redirects = await store.GetRedirects(cancellationToken); - Response.WithDeterministicEtag(EtagHelper.CalculateEtag(redirects)); + Response.WithEtag(ResponseVersions.VersionOf(redirects, redirects.Count)); Response.WithTotalCount(redirects.Count); } @@ -182,21 +182,20 @@ public async Task> Redirects(string sort, stri { var redirects = await store.GetRedirects(cancellationToken); - var queryResult = redirects + var page = redirects .Sort(sort, direction) .Paging(pagingInfo) - .Select(r => new RedirectsQueryResult - ( - r.MessageRedirectId, - r.FromPhysicalAddress, - r.ToPhysicalAddress, - r.LastModified - )); - - Response.WithDeterministicEtag(EtagHelper.CalculateEtag(redirects)); - Response.WithPagingLinksAndTotalCount(pagingInfo, redirects.Count); - - return queryResult; + .ToList(); + + Response.WithQueryStatsAndPagingInfo(new QueryStatsInfo(ResponseVersions.VersionOf(page, redirects.Count), redirects.Count), pagingInfo); + + return page.Select(r => new RedirectsQueryResult + ( + r.MessageRedirectId, + r.FromPhysicalAddress, + r.ToPhysicalAddress, + r.LastModified + )); } public record RedirectsQueryResult(Guid MessageRedirectId, string FromPhysicalAddress, string ToPhysicalAddress, DateTime LastModified); diff --git a/src/ServiceControl/Monitoring/Web/EndpointsMonitoringController.cs b/src/ServiceControl/Monitoring/Web/EndpointsMonitoringController.cs index 8040bac1c9..0f65691ec4 100644 --- a/src/ServiceControl/Monitoring/Web/EndpointsMonitoringController.cs +++ b/src/ServiceControl/Monitoring/Web/EndpointsMonitoringController.cs @@ -68,7 +68,7 @@ public IList KnownEndpoints([FromQuery] PagingInfo pagingInf { var knownEndpoints = monitoring.GetKnownEndpoints(); - Response.WithQueryStatsAndPagingInfo(new QueryStatsInfo(string.Empty, knownEndpoints.Count, isStale: false), pagingInfo); + Response.WithPagingLinksAndTotalCount(pagingInfo, knownEndpoints.Count); return knownEndpoints; } diff --git a/src/ServiceControl/Recoverability/API/EtagHelper.cs b/src/ServiceControl/Recoverability/API/EtagHelper.cs deleted file mode 100644 index addd49b1ea..0000000000 --- a/src/ServiceControl/Recoverability/API/EtagHelper.cs +++ /dev/null @@ -1,55 +0,0 @@ -using System.Collections.Generic; -using System.Text; -using ServiceControl.Persistence.MessageRedirects; -using ServiceControl.Recoverability; - -static class EtagHelper -{ - internal static string CalculateEtag(IReadOnlyList redirects) - { - if (redirects.Count == 0) - { - return string.Empty; - } - - var data = new StringBuilder(); - foreach (var redirect in redirects) - { - data.Append($"{redirect.MessageRedirectId}.{redirect.ToPhysicalAddress}.{redirect.LastModified.Ticks}"); - } - - return data.ToString(); - } - - public static string CalculateEtag(GroupOperation[] groups) - { - if (groups.Length == 0) - { - return string.Empty; - } - - var data = new StringBuilder(); - foreach (var g in groups) - { - data.Append($"{g.Id}.{g.Count}.{g.OperationStatus}.{g.OperationProgress}.{g.OperationStartTime}.{g.OperationCompletionTime}.{g.NeedUserAcknowledgement}.{g.Comment}"); - } - - return data.ToString(); - } - - internal static string CalculateEtag(IList results) - { - if (results.Count == 0) - { - return string.Empty; - } - - var data = new StringBuilder(); - foreach (var g in results) - { - data.Append($"{g.Id}.{g.Count}"); - } - - return data.ToString(); - } -} \ No newline at end of file diff --git a/src/ServiceControl/Recoverability/API/FailureGroupsController.cs b/src/ServiceControl/Recoverability/API/FailureGroupsController.cs index 15d7db5b03..93dca14a47 100644 --- a/src/ServiceControl/Recoverability/API/FailureGroupsController.cs +++ b/src/ServiceControl/Recoverability/API/FailureGroupsController.cs @@ -67,7 +67,7 @@ public async Task GetAllGroups(string classifier = "Exception } var results = await fetcher.GetGroups(classifier, classifierFilter, cancellationToken); - Response.WithDeterministicEtag(EtagHelper.CalculateEtag(results)); + Response.WithEtag(ResponseVersions.VersionOf(results, results.Length)); return results; } @@ -100,9 +100,9 @@ public async Task GetRetryHistory(CancellationToken cancellationTo { var retryHistory = await retryStore.GetRetryHistory(cancellationToken); - Response.WithDeterministicEtag(retryHistory.GetHistoryOperationsUniqueIdentifier()); + Response.WithEtag(retryHistory.QueryStats.Version); - return retryHistory; + return retryHistory.Results; } [Authorize(Policy = Permissions.ErrorRecoverabilityGroupsView)] @@ -112,7 +112,7 @@ public async Task> GetGroup(string groupId, strin { var result = await store.GetUnresolvedGroup(groupId, status, modified, cancellationToken); - Response.WithEtag(result.QueryStats.ETag); + Response.WithEtag(result.QueryStats.Version); return result.Results == null ? NotFound() : result.Results; } diff --git a/src/ServiceControl/Recoverability/API/GroupFetcher.cs b/src/ServiceControl/Recoverability/API/GroupFetcher.cs index e72ea92458..949ad59444 100644 --- a/src/ServiceControl/Recoverability/API/GroupFetcher.cs +++ b/src/ServiceControl/Recoverability/API/GroupFetcher.cs @@ -21,7 +21,7 @@ public GroupFetcher(IGroupsDataStore store, IRetryHistoryDataStore retryStore, I public async Task GetGroups(string classifier, string classifierFilter, CancellationToken cancellationToken = default) { var dbGroups = await store.GetUnresolvedGroupsByClassifier(classifier, classifierFilter, cancellationToken); - var retryHistory = await retryStore.GetRetryHistory(cancellationToken); + var retryHistory = (await retryStore.GetRetryHistory(cancellationToken)).Results; var unacknowledgedRetries = retryHistory.GetUnacknowledgedByClassifier(classifier); var openRetryAcknowledgements = MapAcksToOpenGroups(dbGroups, unacknowledgedRetries); diff --git a/src/ServiceControl/Recoverability/Retrying/RetryDocumentManager.cs b/src/ServiceControl/Recoverability/Retrying/RetryDocumentManager.cs index 6c67c78268..143412ce3e 100644 --- a/src/ServiceControl/Recoverability/Retrying/RetryDocumentManager.cs +++ b/src/ServiceControl/Recoverability/Retrying/RetryDocumentManager.cs @@ -22,16 +22,16 @@ public async Task AdoptOrphanedBatches(CancellationToken cancellationToken { var orphanedBatches = await store.GetOrphanedBatches(RetrySessionId, cancellationToken); - logger.LogInformation("Found {OrphanedBatchCount} orphaned retry batches from previous sessions", orphanedBatches.Results.Count); + logger.LogInformation("Found {OrphanedBatchCount} orphaned retry batches from previous sessions", orphanedBatches.Batches.Count); // let's leave Task.Run for now due to sync sends - await Task.WhenAll(orphanedBatches.Results.Select(b => Task.Run(async () => + await Task.WhenAll(orphanedBatches.Batches.Select(b => Task.Run(async () => { logger.LogInformation("Adopting retry batch {BatchId} with {BatchMessageCount} messages", b.Id, b.MessageCount); await MoveBatchToStaging(b.Id, cancellationToken); }))); - foreach (var batch in orphanedBatches.Results) + foreach (var batch in orphanedBatches.Batches) { if (batch.RetryType != RetryType.MultipleMessages) { @@ -44,7 +44,7 @@ await Task.WhenAll(orphanedBatches.Results.Select(b => Task.Run(async () => return false; } - return orphanedBatches.QueryStats.IsStale || orphanedBatches.Results.Any(); + return orphanedBatches.MightBeIncomplete || orphanedBatches.Batches.Count > 0; } public virtual Task MoveBatchToStaging(string batchId, CancellationToken cancellationToken = default) => store.MoveBatchToStaging(batchId, cancellationToken);