Refactor and improve data versioning / etags by warwickschroeder · Pull Request #5794 · Particular/ServiceControl

warwickschroeder · 2026-08-19T16:07:19Z

Context

An HTTP server can tell a client "nothing has changed since you last asked" instead of resending the whole answer. It does that by stamping each response with a short opaque label, an entity-tag, and the client sends that label back on its next request. If the label still matches, the server answers 304 Not Modified with no body at all.

Before this branch, ServiceControl built those labels ad hoc: loose strings threaded through the persistence layer, plus a helper (EtagHelper) that glued a few fields together with a StringBuilder. Different stores disagreed about what an absent label looked like, and the empty string was used to mean "no label", which is dangerous because the empty string matches itself.

Size

76% tests and docs, 24% production code, by lines changed across the whole PR. Even before this PR the test coverage for ETags and data versioning was lacking heavily. By adding these tests, several real defects were descovered and resolved.

Design

See docs/data-versioning-design.md

What changed

Shared

One type for a version. DataVersion composes from a backend token, named terms, per-row terms, or several instances combined. Terms are length-prefixed, so user text containing a delimiter cannot make two different results digest identically. Replaces EtagHelper and WithDeterministicEtag, which hashed on the way out so the store never recognised its own version coming back. Rule and factory guide in docs/data-versioning-design.md.
Archived groups and retry history return a version. Both returned bare lists, so the controller invented one. Retry history's named only the historic request ids, so acknowledging an operation changed the body without moving the validator and a dismissed operation came back.
Correct conditional GET. If-None-Match now uses RFC 9110 weak comparison rather than EntityTagHeaderValue.Equals, which compares strength too. The header is read through typed headers, so a comma-separated list is no longer treated as one malformed value, and * is handled. The 304 decision reads the action result's status code, not Response.StatusCode, which is not set yet at that point. A FileStreamResult replaced by a 304 is registered for disposal.
Multi-instance results combine properly. DataVersion.Combine over instance and version pairs replaces sorting and concatenating raw etags, and reports nothing when any instance did not supply one, so a response never claims to cover data it could not version.
Endpoints with no honest version say so. The known-endpoints list is built in memory with no store version behind it, so it now states that rather than emitting an empty validator that can never match.

RavenDB

Paged and filtered reads name their query. The version was the index etag alone, so page 2 could carry page 1's validator. It now covers the index etag, the total, the page, sort and filters, and the id of every row rendered.
Counts name their filters too. GetGroupErrorsCount returned the bare index etag, so the unresolved count and the archived count of one group shared a validator.
The overload that omitted the terms is gone. ToPagedQueryStatsInfo and QueryResultConvert now require an id selector and the query terms, so a new call site cannot quietly skip them.

EF Core

Paged and filtered reads name their query, through the same shared QueryNarrowing.Terms as Raven. Applied in the custom check, queue address, failure group and failed message query helpers.
Custom checks had no validator at all. The store returned an empty string, so ServicePulse re-downloaded the list on every poll and the acceptance test covering this was excluded on both EF backends.
Message bodies revalidate. The validator was the message id, on the assumption a body never changes. Ingestion upserts the existing row, so a replaced body kept the old validator. It now covers the id and the row's last-modified stamp.
Retry history rows are ordered. Rows are named by position, so an unordered query versioned row order rather than data: historic operations now order by completion time, unacknowledged ones by key.

Not about data versioning

Custom checks dropped their originating endpoint. The projection never selected the endpoint name, host or host id, so ServicePulse showed a check with no endpoint against it. EF only, found while verifying the above.
Custom checks reported the page size as the total count. The paging links were wrong on every page. Same query, same fix.
QueryStatsInfo.Fresh. EF reads cannot be stale, and the alternative was repeating isStale: false at every new call site.
GetAuditCountsForEndpointApi moved to ScatterGatherRemoteOnly. It had to source its version from remotes only, since its own instance holds none of the data. That removed a never-implemented local query and an unused store dependency.
Ingestion and clock test helpers moved to the shared test project. The new store tests need to ingest data and control time on every backend, not just one.
Etag acceptance test re-enabled on SQL Server and PostgreSQL. It was excluded with a note deferring it to a separate PR. This is that PR.

Outside the error instance

Audit RavenDB persister. All five paged message queries returned the bare index etag, so two pages, two searches or two endpoints shared a validator. /api/messages combines the primary's validator with audit's, so fixing only the primary would leave that endpoint wrong. Audit keeps its own string ETag type; only the composition changed, mirroring the primary's formatting including ticks-precision timestamps.

Test coverage

The version type. Every factory, collisions, precision, missing instances.
HTTP. Weak comparison, wildcards, malformed and unknown validators, non-success responses, stream disposal.
Stores. Per-endpoint version tests, plus a conformance suite proving two queries never share a version, on RavenDB, SQL Server and PostgreSQL. Half its cases return nothing, where only the query terms separate them.
Message bodies. Moves when the body is replaced, holds when it is not.
Audit. The same conformance shape over pages, searches and endpoints.
End to end. Etag acceptance tests on all three backends.

…ceptance test. - Add tests for CustomCheck data versioning

- Add tests for archived groups data versioning

…ng an operation changed the body without moving it and clients kept an operation they had dismissed. Live on both persisters. Each backend now versions its own way, as the other stores do: EF composes every field of both collections, RavenDB uses the document change vector. Also extracts the inline composers from QueueAddressStore and CustomCheckDataStore, and renames EtagHelper to ResponseVersions.

johnsimons · 2026-08-20T23:46:48Z

-        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(


@warwickschroeder I haven't done a thorough review yet, but this one seems wrong. We don't need to add another column to this table, as we previously mentioned; bodies are immutable. In other words, the body would only change if a different UniqueMessageId was issued.

@johnsimons from what I'm reading, it doesnt seem to be immutable. The failed message is set via an upsert, which also includes the body text.

It looks like if an already retried message is edited and retried again, the body would be updated due to it checking for the originals header?

An edit creates a brand new message.
I am 100% sure that bodies are immutable.
If we are updating the body as part of an upsert, we should not.

I just reviewed the upsert, I think because we don't know whether it is going to be an insert or update, we still need to send the body regardless, but we could skip updating the body if it is an update.

Ok, I can look into doing that so it is 100% immutable. Then I can remove the additional field.

johnsimons

I'm halfway through reviewing this PR.
One thing that isn't sitting well with me is that I don't think the data storage layer should need to be aware of ETags.

Imagine an internal service that needs data from two different data storage implementations. That service doesn't care about ETags. The only thing that really cares about them is the HTTP layer.
So in my opinion, the data stores shouldn't have to return QueryResult at all. They should return just the data, and something sitting between the controller/action and the data storage should be responsible for adding the ETag information.

I can understand how this separation can be hard to achieve given RavenDB mixes both worlds.

johnsimons · 2026-08-21T01:03:53Z

-        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(


An edit creates a brand new message.
I am 100% sure that bodies are immutable.
If we are updating the body as part of an upsert, we should not.

johnsimons · 2026-08-21T01:06:30Z

-        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(


I just reviewed the upsert, I think because we don't know whether it is going to be an insert or update, we still need to send the body regardless, but we could skip updating the body if it is an update.

johnsimons · 2026-08-21T01:49:35Z

+            // No index etag means no version at all, as on the primary side. The rows here carry only ids,
+            // so the etag is the only term covering a change to a field a row renders; without it a
+            // validator would stand still while that field moved.
+            if (stats.ResultEtag is not { } resultEtag)


Would Raven ever return a null for this?

It is nullable so possibly, and if it is null we should be sending a DataVersion.None rather than a null or empty string. This entire file has been refactored quite a bit to simplify however.

johnsimons · 2026-08-21T02:24:14Z


-            return new QueryResult<IList<RetryBatch>>(batches, new QueryStatsInfo(string.Empty, batches.Count, false));
+            // No version: orphaned batches are consumed by the retry session, never by a caching client.
+            return new QueryResult<IList<RetryBatch>>(batches, QueryStatsInfo.Fresh(DataVersion.None, batches.Count));


This is what we talked about @warwickschroeder, this one is used for internal consumption so, it is not required

johnsimons · 2026-08-21T02:26:20Z

+    /// check id, so naming Id covers all three, and the host string is written once on insert and never
+    /// updated.
+    /// </summary>
+    public static QueryStatsInfo ToQueryStatsInfo(this IReadOnlyCollection<CustomCheck> page, long totalCount, params (string Name, object? Value)[] query) =>


This is only referenced in one place. Do we really need to have a class on its own?

Its following the pattern of FailureGroupQueries, but I could consolidate all of these helper functions into a single class?

I've consolidated all these into an extensions file.

johnsimons · 2026-08-21T02:30:50Z

                .FilterByLastModifiedRange(modified)
                .FirstOrDefaultAsync(cancellationToken);

-            return new QueryResult<FailureGroupView>(document, stats.ToQueryStatsInfo());


i don't entirely understand why we need to change Raven, given Raven has its own etag buildt into it?

Reverted to just using ravens ResultEtag

rbev · 2026-08-21T02:57:38Z

+            Aggregate(results);
+
+        /// <summary>
+        /// For an API whose own instance holds none of the data. Its local result carries no version, and


I'm not clear on why this is treated differently.

If a remote instance has no data and forces it to none why should a local instance be excluded from that?

rbev · 2026-08-21T03:10:19Z


-            return new QueryResult<IList<CustomCheck>>(results, new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults, stats.IsStale));
+            return new QueryResult<IList<CustomCheck>>(results,
+                stats.ToPagedQueryStatsInfo(results, check => check.Id, ("status", status), ("page", paging.Page), ("pageSize", paging.PageSize)));


Shouldn't this just return stats.ResultsEtag in the version struct?

Reverted. No longer taking paging into account.

rbev · 2026-08-21T03:13:00Z

-            return new QueryStatsInfo($"{stats.ResultEtag}", stats.TotalResults, stats.IsStale);
-        }
+        /// <summary>
+        /// For a paged query. The index etag covers whether the data moved, and the row ids cover which


I'm confused as to the reason this needs to be done, are you saying the etag in raven is based on the whole table?
My understanding is that it was already aggregated for you from the result set it returned

You are right, I'm going to revert this. I was thinking too much from a "backend" PoW and forgot about what these versions would be used for and who will be using it. An identical etag for page 1 and page 2 in ServicePulse is fine as they have different URLs, and SP doesnt do anything with etags iteself either. This may not be the case for other clients depending on how they consume and use the etag, but we dont have those, so ill simplify.

rbev · 2026-08-21T03:27:55Z

+    }
+
+    /// <summary>Nothing changed, so a caller holding the earlier version still holds the current one.</summary>
+    public static void Held(DataVersion first, DataVersion second, string because)


Should this be VersionAssert.Matches()? "Held" seems like how this assertion is used rather than what it's asserts

rbev · 2026-08-21T03:29:03Z

+    /// Two different queries, answered at the same instant. Neither describes the other, so a caller
+    /// holding one must never be told the other is current.
+    /// </summary>
+    public static void Distinct(DataVersion one, DataVersion other, string because)


This is the same code as Moved with different messages, does it need to be it's own assertion?

rbev · 2026-08-21T03:31:03Z

+    /// The page, ordering and filters a read was narrowed by, expressed as version terms.
+    /// <para>
+    /// A version over a list normally tells two queries apart by the rows it returns. A query that matches
+    /// nothing returns no rows and so contributes no terms, which leaves every empty view of the same data


I thought that blank results were meant to return an DataVersion.None anyway so that they weren't flagged as unchanged.

Removed the entire file. No longer needed, as discussed in another comment

rbev · 2026-08-21T03:40:20Z

            return new QueryResult<IList<MessagesView>>(
                pageOfResults,
-                new QueryStatsInfo(etag, allResults.Count, isStale: false))
+                QueryStatsInfo.Fresh(DataVersion.FromToken(etag), allResults.Count))


Given the "isStale" flag is exposing a ravenDb concept directly could it be replaced by returning ETags correcly? I know that the EF implementation always just returns false.

Or is it used downstream for something else?

internalautomation Bot assigned warwickschroeder Aug 19, 2026

warwickschroeder added 29 commits August 20, 2026 12:31

Add DataVersion, one representation for a persisted query result version

7bc096d

Carry the message body version as a DataVersion

c40cc22

Retype the query result validator as DataVersion

0e84986

Emit the store version verbatim on every endpoint

6e23697

Make the ingestion and clock test helpers available to every backend

bf13604

Fix merge issues

d788c9a

Move the message body version when the stored body is replaced

a8bc1fc

Mark derived validators weak and compare them per RFC 9110

8e32ddb

Derive projected versions from the rows they report on

fc2b93c

- Cover a paged endpoint and the empty case in the conditional GET ac…

5a0fa1d

…ceptance test. - Add tests for CustomCheck data versioning

- Fix Groups data versioning

869a04b

- Add tests for archived groups data versioning

Clarify some comments

8686d3f

Add data version tests for messages view

3b9c32d

Add data version tests for message redirects

5e3f032

Clean after review

563836b

Fix versioning paged results

cd3d0b2

Fix versioning issue for message view

da50d71

Fix custom checks versioning

5af79d1

Improve and add tests

924cde1

Remove the unused strong tag

f9c7498

Refactor to use shared OverRows function

233bed7

Fix ordering for retry history

e24e5b5

Cleanup

aac3aa4

Abstract away the IsStale boolean for EF

74211ec

Use paged data versioning for eventlogs

aed683c

add data version design doc

a103b2a

Changes from review

e1e3348

Fix after rebase

7f21c13

warwickschroeder force-pushed the warwick/data-ver branch from 31a46fe to 7f21c13 Compare August 20, 2026 05:01

Final review changes

53244b5

warwickschroeder marked this pull request as ready for review August 20, 2026 14:18

warwickschroeder requested review from abparticular, johnsimons and rbev August 20, 2026 14:18

johnsimons reviewed Aug 20, 2026

View reviewed changes

johnsimons reviewed Aug 21, 2026

View reviewed changes

rbev reviewed Aug 21, 2026

View reviewed changes

Uh oh!

Conversation

warwickschroeder commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Size

Design

What changed

Shared

RavenDB

EF Core

Not about data versioning

Outside the error instance

Test coverage

Uh oh!

Choose a reason for hiding this comment

Uh oh!

warwickschroeder Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsimons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

warwickschroeder commented Aug 19, 2026 •

edited

Loading

warwickschroeder Aug 21, 2026 •

edited

Loading