Refactor and improve data versioning / etags

docs/data-versioning-design.md

@@ -0,0 +1,54 @@
+# 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  |
+| `OverRows(summary, rows)`               | the same, for rows that declare their own fields by implementing `IVersionedRow`                |
+| `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` digests its terms and emits the result as a GUID, so the tag reveals nothing about the values behind it. The terms go into the hash one at a time rather than being joined into a string first, so the largest thing ever held in memory is a single row rather than the whole page.
+
+Every term's value, and every field inside a row, is **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`. Term names are not prefixed, because they are literals in the code rather than anything a user can type. `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 means there is no version to offer. Two parties that both know nothing have not established that nothing changed, so **absence must never answer `304`**: an empty validator that matched itself would serve a cached page for every request for ever.
+
+`None` means "no answer", not "no rows", and the difference matters. A query that found nothing still produces a real version, because a list always contributes a summary term and `Compose` over `[("messages", 0)]` is as good a validator as any other. So an empty page is cacheable, and a client watching something that stays empty gets its `304`. What produces `None` is a question that was never answered: a store that has no token of its own to offer, a remote instance that timed out or refused the call, a response whose `ETag` header was absent or unparseable.
+
+`Combine` returns `None` as soon as any instance reports none, and that is why: an instance reporting none is one whose data could not be seen at all, so no promise can be made about it. It is not an instance reporting that it is empty, which would come with a version like anything else.
+
+## 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 result before aggregating, via `AggregateStatsFromRemotesOnly`. That instance is not a source for the query and never ran one, so its placeholder is a non-participant rather than an instance that went quiet. Left in, it would take the composite to `None` on every request, and the endpoint would never emit a tag at all.

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
 

src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj

@@ -51,9 +51,6 @@
     <Compile Remove="..\ServiceControl.AcceptanceTests\Recoverability\ExternalIntegration\When_a_reedit_solves_a_failed_msg.cs" />
     <Compile Remove="..\ServiceControl.AcceptanceTests\Recoverability\ExternalIntegration\When_encountered_an_error.cs" />
     <Compile Remove="..\ServiceControl.AcceptanceTests\Recoverability\When_edited_message_fails_to_process.cs" />
-
-    <!-- The EF custom-check query does not provide an ETag. Addressed by a separate PR. -->
-    <Compile Remove="..\ServiceControl.AcceptanceTests\WebApi\When_a_request_is_repeated_with_its_etag.cs" />
   </ItemGroup>
 
 </Project>

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<FailedErrorsCountReponse> 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 };
         }

src/ServiceControl.AcceptanceTests.RavenDB/Recoverability/MessageFailures/FailedMessageRetriesController.cs

@@ -4,9 +4,9 @@
     using System.Threading.Tasks;
     using Infrastructure.WebApi;
     using Microsoft.AspNetCore.Mvc;
+    using Persistence.Infrastructure;
     using Persistence.RavenDB;
     using Raven.Client.Documents;
-    using ServiceControl.Recoverability;
 
     public class FailedMessageRetriesCountReponse
     {
@@ -24,7 +24,7 @@ public async Task<FailedMessageRetriesCountReponse> GetFailedMessageRetriesCount
             using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken);
             await session.Query<FailedMessageRetry>().Statistics(out var stats).ToListAsync(cancellationToken);
 
-            Response.WithEtag(stats.ResultEtag.ToString());
+            Response.WithEtag(DataVersion.FromToken(stats.ResultEtag.ToString()));
 
             return new FailedMessageRetriesCountReponse { Count = stats.TotalResults };
         }

src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj

@@ -51,9 +51,6 @@
     <Compile Remove="..\ServiceControl.AcceptanceTests\Recoverability\ExternalIntegration\When_a_reedit_solves_a_failed_msg.cs" />
     <Compile Remove="..\ServiceControl.AcceptanceTests\Recoverability\ExternalIntegration\When_encountered_an_error.cs" />
     <Compile Remove="..\ServiceControl.AcceptanceTests\Recoverability\When_edited_message_fails_to_process.cs" />
-
-    <!-- The EF custom-check query does not provide an ETag. Addressed by a separate PR. -->
-    <Compile Remove="..\ServiceControl.AcceptanceTests\WebApi\When_a_request_is_repeated_with_its_etag.cs" />
   </ItemGroup>
 
 </Project>

src/ServiceControl.AcceptanceTests/WebApi/When_a_request_is_repeated_with_its_etag.cs

@@ -2,34 +2,47 @@ 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/eventlogitems", "GET")]
+        [TestCase("/api/redirects", "GET")]
+        [TestCase("/api/redirect", "HEAD")]
+        [TestCase("/api/errors/queues/addresses", "GET")]
+        [TestCase("/api/errors", "GET")]
+        [TestCase("/api/errors", "HEAD")]
+        [TestCase("/api/messages", "GET")]
+        [TestCase("/api/messages/search?q=anything", "GET")]
+        [TestCase("/api/messages/search/anything", "GET")]
+        [TestCase("/api/messages2?page_size=10", "GET")]
+        [TestCase("/api/endpoints/no-such-endpoint/messages", "GET")]
+        [TestCase("/api/endpoints/no-such-endpoint/errors", "GET")]
+        [TestCase("/api/endpoints/no-such-endpoint/messages/search?q=anything", "GET")]
+        [TestCase("/api/endpoints/no-such-endpoint/messages/search/anything", "GET")]
+        [TestCase("/api/errors/groups", "GET")]
+        [TestCase("/api/endpoints", "GET")]
+        [TestCase("/api/heartbeats/stats", "GET")]
+        [TestCase("/api/recoverability/classifiers", "GET")]
+        [TestCase("/api/recoverability/groups", "GET")]
+        [TestCase("/api/recoverability/history", "GET")]
+        // A group that does not exist still answers, with an empty page and a validator of its own.
+        [TestCase("/api/recoverability/groups/no-such-group/errors", "GET")]
+        [TestCase("/api/recoverability/groups/no-such-group/errors", "HEAD")]
+        [TestCase("/api/conversations/no-such-conversation", "GET")]
+        public async Task Should_answer_not_modified(string url, string method)
         {
             Answer issued = null;
             Answer repeated = null;
 
             await Define<Context>()
                 .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 +74,42 @@ await Define<Context>()
             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<Context>()
+                .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<Answer> Ask(string method, string url, string ifNoneMatch)
         {
             using var response = await Send(method, url, ifNoneMatch);

src/ServiceControl.Audit.Persistence.RavenDB/Extensions/RavenQueryStatisticsExtensions.cs

@@ -1,13 +1,12 @@
 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);
-        }
+        public static QueryStatsInfo ToQueryStatsInfo(this QueryStatistics stats) =>
+            new(stats.ResultEtag?.ToString(CultureInfo.InvariantCulture) ?? string.Empty, stats.TotalResults);
     }
-}
+}

src/ServiceControl.Audit.Persistence.RavenDB/RavenAuditDataStore.cs

@@ -25,7 +25,7 @@ public async Task<QueryResult<SagaHistory>> QuerySagaHistoryById(Guid input, Can
                     .Statistics(out var stats)
                     .SingleOrDefaultAsync(x => x.SagaId == input, token: cancellationToken);
 
-            return sagaHistory == null ? QueryResult<SagaHistory>.Empty() : new QueryResult<SagaHistory>(sagaHistory, new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults));
+            return sagaHistory == null ? QueryResult<SagaHistory>.Empty() : new QueryResult<SagaHistory>(sagaHistory, stats.ToQueryStatsInfo());
         }
 
         public async Task<QueryResult<IList<MessagesView>>> GetMessages(bool includeSystemMessages, PagingInfo pagingInfo, SortInfo sortInfo, DateTimeRange timeSentRange, CancellationToken cancellationToken = default)

src/ServiceControl.Audit/Auditing/MessagesView/GetMessages2Controller.cs

@@ -54,6 +54,7 @@ public async Task<IList<MessagesView>> GetAllMessages(
         }
 
         Response.WithTotalCount(result.QueryStats.TotalCount);
+        Response.WithEtag(result.QueryStats.ETag);
 
         return result.Results;
     }

src/ServiceControl.Persistence.EFCore/EntityConfigurations/EventLogItemConfiguration.cs

@@ -18,9 +18,6 @@ public void Configure(EntityTypeBuilder<EventLogItemEntity> builder)
         builder.Property(e => e.RelatedTo).IsRequired();
         builder.Property(e => e.Category).IsRequired().HasMaxLength(ColumnLengths.ShortTextLength);
         builder.Property(e => e.EventType).IsRequired().HasMaxLength(ColumnLengths.ShortTextLength);
-        // Every read is "order by RaisedAt descending" plus paging. The key is included as a
-        // tiebreaker so that items sharing a RaisedAt do not shuffle between pages, and so that
-        // MAX(RaisedAt) for the ETag is an index seek.
         builder.HasIndex(e => new { e.RaisedAt, e.Id }).IsDescending();
     }
 }