feat(invalidation): report how many objects a purge actually removed - #104
Open
jensens wants to merge 2 commits into
Open
feat(invalidation): report how many objects a purge actually removed#104jensens wants to merge 2 commits into
jensens wants to merge 2 commits into
Conversation
vcl_purge answered synth(200) unconditionally, so a purge that removed nothing was indistinguishable from one that removed everything at every layer above Varnish — the shared root cause that let #93/#94/#95/#101 stay unnoticed for months. The count vmod_purge/vmod_xkey return was already computed (req.http.n-gone, landing only in the synth reason phrase); this wires it through as a stable, machine-readable signal instead of a status-line parse. vcl_synth now copies req.http.n-gone onto an X-Vinyl-Purged response header whenever it is set — the one place all four purge paths (soft/ hard URL purge in vcl_hit/vcl_miss, xkey purge/softpurge in vcl_recv) pass through on the way out, so one conditional covers all four. PodResult and BroadcastResult gain an ObjectsPurged *int. nil means unknown (header missing or unparseable) and is kept strictly distinct from a known 0 — post-#92 sharding, a correctly routed broadcast purge legitimately removes 0 objects on every pod but the URL's owner, and collapsing "unknown" into that same 0 would recreate the exact blindness this closes. HTTP status is untouched: 200 stays 200 either way. The aggregate is surfaced in the JSON body next to "succeeded" and omitted (not null/0) when unknown. vinyl_objects_purged_total (cache, namespace, type) is the new counter: it only advances on a known count, so a regression that stops the header arriving leaves it flat instead of climbing on fabricated zeros — that flat line while purges are being issued is the graphable signal that was missing. Tests drive a real httptest.Server for the header-parsing paths rather than hand-built structs, per this area's history of tests that pass while the feature is broken. Also verified against a real varnishd:8.0.2 running the generated VCL end to end (real objects, real purge, real HTTPBroadcaster, real JSON) — see the PR description for the transcript. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gression Review found two Important issues in 722fffa. 1. TestHandlePurge_UnknownObjectsPurged_MetricDoesNotAdvance asserted via testutil.ToFloat64(WithLabelValues(...)), which lazily creates the label combination at 0 on read — "Add never called" and "Add(0) called" were indistinguishable to it. Replaced with testutil.CollectAndCount, which only counts children the code under test actually touched. Falsified both ways: reverting the nil-guard to an unconditional Add(float64(n)) turns it red; restoring the guard turns it back green (see PR description for both transcripts). 2. aggregateObjectsPurged marks the sum "known" if any pod reported a count, so a regression that drops the header on only a subset of pods — the #101 shape, a broadcast-path defect hitting every pod the same way, reproducible by a rolling VCL push — just nudges the sum down a little with nothing making that visible. Added vinyl_objects_purged_unknown_total: counts individual pod responses that answered 2xx without a parseable count, kept deliberately separate from vinyl_objects_purged_total so "removed nothing" and "did not say" stay distinguishable facts. Restricted to purge/xkey (objectsPurgedCapable) since BAN never carries this header by design, not by regression — without that gate every ordinary BAN response would permanently count as "unknown" noise. Also falsified both the type-gate and the increment itself going missing. Also: corrected docs/metrics.md — ban can never appear as a `type` value on either objects-purged counter (copy-pasted from InvalidationTotal's convention, but ban never sets X-Vinyl-Purged); added a test asserting the JSON body renders an explicit "objectsPurged":0 for a genuine known zero, the positive counterpart to the existing omitted-when-unknown test. Fixed the test fixture (okResultWithObjectsPurged) to mark every pod known rather than just the first, and a goconst/gofmt cleanup (labelType constant) surfaced by golangci-lint along the way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #103.
Why
Four separate invalidation bugs — #93, #94, #95, #101 — went unnoticed for months. They had four different root causes and one thing in common: none of them produced a signal. A purge that removed nothing looked exactly like one that removed everything, at every layer. The operator reported success, the user saw success, and the stale object kept being served.
Those bugs are fixed. The blindness was not, and it would have hidden the next one just as well.
What this adds
The count already existed and was thrown away. After #94, the VCL computes it (
purge.soft()/purge.hard(), andxkey.purge()/xkey.softpurge()) and puts it in the synth reason phrase — butPodResultcarried only pod, status and error.X-Vinyl-Purgedresponse header, set from one place invcl_synthand gated onreq.http.n-gone, so it appears on all four purge paths and never leaks onto the 403 from an ACL check or the 400 from a malformed BAN. A header rather than the reason phrase because parsing a status line is a brittle contract.objectsPurgedonPodResultandBroadcastResult, surfaced in the JSON body.vinyl_objects_purged_totalandvinyl_objects_purged_unknown_total.Two distinctions the design turns on
Unknown is not zero. A missing or malformed header means the pod did not say; it does not mean nothing was removed. Both are
*int, soomitemptyomits only nil and a genuine zero still renders as"objectsPurged":0. Conflating the two would rebuild the blindness this exists to remove.Zero is not an error, so the HTTP status is untouched. 200 stays 200 whatever the count. After #92's sharding fix a URL lives on its owner pod, so a correct broadcast purge legitimately removes nothing on every other pod. A rule that treated zero as failure would report failure on every healthy purge. The signal is the aggregate: individual zeros are normal, a fleet-wide zero while purges are being issued is not.
Why there are two metrics rather than one
Review caught that a single summed counter is blind to the failure shape most likely to recur. A total collapse is obvious — the line goes flat. But a subset of pods dropping the header only lowers the sum slightly, and that subset shape is exactly what #101 was: a broadcast-path defect that a rolling VCL push would reproduce on some pods only.
So "removed nothing" and "did not say" are counted separately.
vinyl_objects_purged_unknown_totalincrements per pod that answered 2xx without a parseable count, gated to purge and xkey — BAN goes through the agent API and never carries the header, so counting it would be permanent noise.Testing
Given this area's history of tests that passed while the feature was broken, every mechanism here was falsified rather than merely asserted — the guard was broken, the test watched to fail, then restored:
testutil.ToFloat64lazily creates the label series at zero, so "never called" and "called with zero" were indistinguishable. It now usesCollectAndCountand goes red when the nil-guard is removed.httptest.Servers that set, withhold and malform the header, not against hand-built structs.Verified end to end against a live
varnishd:8.0.2running the real generated VCL, driven by the actual broadcaster:X-Vinyl-Purgedobserved as 1, 0 and 2 across the URL and xkey paths, and a re-purge correctly showing the soft pod still counting the object from grace while the hard pod reports zero.go test ./...,go vet ./..., golangci-lint v2.13.1 (0 issues) and bothhack/checks all clean. No API or CRD changes.