Skip to content

Fix conditional GET across the API - #5662

Merged
warwickschroeder merged 1 commit into
masterfrom
warwick/etag-fix
Jul 30, 2026
Merged

Fix conditional GET across the API#5662
warwickschroeder merged 1 commit into
masterfrom
warwick/etag-fix

Conversation

@warwickschroeder

Copy link
Copy Markdown
Contributor

ServicePulse polls data from ServiceControl as often as 5s. ServiceControl was built to answer "nothing has changed", but the ETag it sends was a bare token and RFC 9110 requires a quoted string. NotModifiedStatusHttpHandler compares typed header values, an unquoted token fails to parse as EntityTagHeaderValue and yields null, so the comparison never matched and every conditional request got a full payload.

What changed

  • WithEtag quotes the validator, in ServiceControl and in ServiceControl.Audit, which has its own copy of the same code and the same defect. All 37 ETag call sites funnel through it.
  • WithEtag emits no header when there is nothing to validate. An empty validator quoted becomes ETag: "", which is well formed and therefore matches itself, so quoting alone would have answered 304 to a client holding an unrelated payload.
  • Two body endpoints now use WithEtag instead of assigning Response.Headers.ETag directly. Bodies are the largest payloads on the API and immutable once written.
  • ScatterGatherApi reads a remote instance's ETag from the raw header. The typed accessor returns null for an unquoted value, so every remote validator was being discarded.
  • The in-memory audit persister supplies a stable body validator. It returned string.Empty for a body in metadata and a fresh Guid.NewGuid() per fetch for one in an attachment. Both can never match. It now uses the body id.

Impact assessment

  • Reviewed against ServiceControl iteselt, ServicePulse, and ServiceInsight. Nothing breaks.
  • Mixed-version clusters are safe. Instances read each other's validators raw and accept both shapes.

Tests

  • ConditionalGetTests, 7 tests in ServiceControl.UnitTests and 7 in ServiceControl.Audit.UnitTests: the 304 round trip, the mismatch guard, both validator shapes, and the empty case.
  • When_a_request_is_repeated_with_its_etag, 3 cases: /api/customchecks, GET /api/redirects and HEAD /api/redirect end to end through Kestrel, including that Total-Count survives the 304. The HEAD verb is covered nowhere else.
  • When_a_message_body_is_requested_twice: the audit body endpoint

@johnsimons johnsimons left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
But I noticed the monitoring instance does not use etags 😞

{
internal static string ReadEtag(HttpResponseHeaders headers)
{
// Read raw rather than through headers.ETag. An instance predating the quoted validator

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this matter too much?
Once the instance updates a new read will happen and the etag will get updated to the quoted version. It seems like bothering with re-implementing this read and handling quoted/unquoted values is a marginal optimisation at best especially if it was being silently ignored previously anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It protects a small window when an error instance is upgraded and an audit has not. Without this check a 304 would be sent to the client rather than a 200 even if audit has new events it needs to share. It can probably be cleaned up in a couple releases time i think, but adding it is harmless.

@warwickschroeder
warwickschroeder merged commit 7a41dd5 into master Jul 30, 2026
37 checks passed
@warwickschroeder
warwickschroeder deleted the warwick/etag-fix branch July 30, 2026 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants