Fix conditional GET across the API - #5662
Open
warwickschroeder wants to merge 1 commit into
Open
Conversation
johnsimons
approved these changes
Jul 29, 2026
johnsimons
left a comment
Member
There was a problem hiding this comment.
LGTM
But I noticed the monitoring instance does not use etags 😞
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ServicePulse polls data from ServiceControl as often as 5s. ServiceControl was built to answer "nothing has changed", but the
ETagit sends was a bare token and RFC 9110 requires a quoted string.NotModifiedStatusHttpHandlercompares typed header values, an unquoted token fails to parse asEntityTagHeaderValueand yieldsnull, so the comparison never matched and every conditional request got a full payload.What changed
WithEtagquotes the validator, inServiceControland inServiceControl.Audit, which has its own copy of the same code and the same defect. All 37 ETag call sites funnel through it.WithEtagemits no header when there is nothing to validate. An empty validator quoted becomesETag: "", which is well formed and therefore matches itself, so quoting alone would have answered304to a client holding an unrelated payload.WithEtaginstead of assigningResponse.Headers.ETagdirectly. Bodies are the largest payloads on the API and immutable once written.ScatterGatherApireads a remote instance's ETag from the raw header. The typed accessor returnsnullfor an unquoted value, so every remote validator was being discarded.string.Emptyfor a body in metadata and a freshGuid.NewGuid()per fetch for one in an attachment. Both can never match. It now uses the body id.Impact assessment
Tests
ConditionalGetTests, 7 tests inServiceControl.UnitTestsand 7 inServiceControl.Audit.UnitTests: the304round 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/redirectsandHEAD /api/redirectend to end through Kestrel, including thatTotal-Countsurvives the304. The HEAD verb is covered nowhere else.When_a_message_body_is_requested_twice: the audit body endpoint