test: proxy package cannot run at -count>1 — two tests assert on process-global metrics counters

[amiddavid]

Summary

go test -count=N with N > 1 cannot pass on the proxy package. Two tests assert on
process-global metrics counters that they themselves increment, so the second run in the same
process reads the first run's residue and the assertions fail. This is on main today
(51fcd91), independent of any open PR.

Reproduce

$ go test -race -count=2 -run "TestExtractEconomicsAreExported|TestExpandUnresolvedSeriesRender" ./proxy/
--- FAIL: TestExtractEconomicsAreExported (0.00s)
    promexport_components_test.go:78: missing from /metrics:
          cg_extract_calls_total{outcome="avoided"} 1
    promexport_components_test.go:78: missing from /metrics:
          cg_extract_calls_total{outcome="suppressed"} 1
    promexport_components_test.go:78: missing from /metrics:
          cg_extract_gate_declines_total{reason="gate:not_worth_it"} 1
--- FAIL: TestExpandUnresolvedSeriesRender (0.00s)
    promexport_coverage_test.go:162: /metrics is missing the line "cg_expand_unresolved_total{reason=\"malformed\"} 0"
    promexport_coverage_test.go:162: /metrics is missing the line "cg_expand_unresolved_total{reason=\"missing\"} 0"
FAIL

-count=1 (what CI runs, via make cover) is green, which is why this has stayed invisible.

Cause

Neither test's state lives in the metrics.NewAggregator() it constructs.

1. proxy/promexport_components_test.go:63 — TestExtractEconomicsAreExported.
   metrics.RecordExtractionSuppressed / RecordExtractionCacheLookup write through
   metrics.xFor(component), which reads and lazily fills the package-global registry
   xReg (metrics/extract.go:112). The agg passed to New is not consulted. Run 2 finds the
   run-1 counters still there, renders ... 2, and the exact-string strings.Contains(body, "... 1")
   assertions miss.

2. proxy/promexport_coverage_test.go:155 — TestExpandUnresolvedSeriesRender. The test's
   whole point is that both reasons render at zero before anything breaks, then it increments
   them itself to prove the values move. The values come from expand.Unresolved()
   (proxy/promexport.go:478), also process-global — deliberately so, per the comment there,
   because sourcing them off Snapshot would export a hard-wired 0 forever. So the 0 assertion
   is only true on the first run in a process.

The production behaviour is correct in both cases: a proxy is one process with one lifetime, and
process-global counters are the right shape for it. The defect is that the tests read that
global state without owning it.

Why it matters beyond tidiness

-count=N is the tool for shaking out a flake or a race, and it is exactly what a reviewer reaches
for on a concurrency change — #191's own write-up leans on -count=20 and -count=200 runs. On
the proxy package that instrument is unavailable: any -count>1 run fails for reasons unrelated
to whatever is being investigated, so a real intermittent failure is hidden in noise that looks
identical to it. Found while re-running #191's suite at -count=2, where these two failures had to
be chased down and cleared before the actual change could be trusted.

Severity: low impact on shipped behaviour, moderate on the ability to diagnose concurrency bugs.

Fix options

A. Reset the globals per test (smallest). Export test-only resets — metrics.ResetExtractionForTest()
and an equivalent for expand's unresolved counters — and call them at the top of each test (or via
t.Cleanup). Two or three lines per test, no production change. Downside: a test-only export on a
production package, and it stays a footgun for the next test that touches these counters — nothing
makes the requirement visible.

B. Assert relative, not absolute. Read the counter before acting and assert on the delta.
Keeps the globals private and is repeat-safe by construction. Downside: TestExpandUnresolvedSeriesRender
genuinely wants to assert the literal 0 line — that a zero-valued series is rendered at all
rather than omitted (Grafana's "No data" reading as healthy). A delta assertion cannot express
that, so this one needs a reset regardless.

C. Make the counters injectable. Give xReg and expand's counters a per-instance home reachable
from the Options/aggregator the handler already takes, with the global as the default. This is the
structurally right answer and makes the whole family testable without ceremony — and it is a
production refactor across metrics/extract.go and the expand path, well beyond the size of the
problem being solved.

Recommendation: A for expand (the zero-render assertion needs it), B where a delta says the same
thing — then -count=N works on the package without a production refactor. Happy to take a
different call.

Not fixed here

Reported rather than patched: it is a main defect found while reviewing a branch, so it belongs on
its own branch off main rather than riding along in #191, whose diff is test synchronisation.

Assisted-By: Claude Opus 5 (1M context) noreply@anthropic.com

---

Correction to the base SHA (this issue originally said 3fabf51). The default branch on the
remote is 51fcd91. 3fabf51 is a stale ancestor of it — a local branch in one checkout that was
78 commits behind — and both tests named here were added after it (TestExtractEconomicsAreExported
by d9e2f24/#83, TestExpandUnresolvedSeriesRender by 3ebc65d/#155), so they do not exist in that
tree at all. The reproduction above is unaffected:
it was run against a tree built from the remote default-branch ref, which resolved to 51fcd91,
plus independently against #191's head 999e1bf, whose parent is also 51fcd91. Only the SHA quoted
in the text was wrong. Caught by the reviewer on #191.

(Amended: the note above first said "ten commits" and attributed both tests to #184. Both figures
were mine and both were wrong — the gap is 78 commits (git rev-list --count 3fabf51..51fcd91), and
the tests came from #83 and #155. The "ten" was a truncated git log | head read as the whole range;
the #184 attribution was an inference I never checked. Corrected inline above; the conclusion is
unchanged, since both files are absent at 3fabf51 either way. Caught by the reviewer on #191.)

[amiddavid]

Two corrections to the base-tree note in this issue, from the reviewer side of #191. Neither touches the finding — the defect and its reproduction stand — but the range is what makes the "wrong tree named" argument work, so it should be exact.

1. The gap is 78 commits, not ten. 3fabf51 is an ancestor of 51fcd91 by git rev-list --count 3fabf51..51fcd91 = 78 commits, 62 of them first-parent PR merges, reaching back past #83. The correction note describes it as ten commits spanning #178, #183, #184 and #185; that is the tail of the range, not the range.

2. Neither named test came from #184. They predate it by a lot:

test	file	added by
TestExtractEconomicsAreExported	proxy/promexport_components_test.go	d9e2f24 (#83)
TestExpandUnresolvedSeriesRender	proxy/promexport_coverage_test.go	3ebc65d (#155)

What is unaffected: both files are absent at 3fabf51 (git cat-file -e 3fabf51:<path> fails for each), so the original body did name a tree in which the defect cannot be observed — the point the correction was making is right, just attributed to the wrong commits. And the reproduction is independent of all of this, having been run against the remote default-branch ref and against 999e1bf's parent, both 51fcd91.

I also confirmed the defect itself by inspection rather than by re-running it: TestExtractEconomicsAreExported calls metrics.RecordExtractionSuppressed / RecordExtractionCacheLookup against the process-global registry and then asserts cg_extract_calls_total{outcome="suppressed"} 1, so the second iteration in a -count=2 binary reads 2 and the substring match fails. Same shape for the expand.Unresolved() assertion.

One consequence worth stating in the issue for whoever picks it up: what this unblocks is repeat-run coverage of the whole ./proxy/ package, not just these two tests. -count=N is the natural instrument for the concurrency defect class #191 is about, and it is currently unavailable there for reasons unrelated to any of that work.

[amiddavid]

Correcting my own figure, raised by the #191 work session and confirmed here.

"62 of them first-parent PR merges" is wrong, and wrong in the way that should have caught my eye before I posted it: 62 exceeds the first-parent total, so it could not have been a subset of anything. It came from eyeballing git log --first-parent output rather than counting it — the same class of error as the "ten commits" I corrected, which makes it a poor thing to have put next to that correction.

Exact, on 3fabf51..51fcd91:

figure	command	value
commits	git rev-list --count	78
first-parent commits	git rev-list --count --first-parent	60
first-parent subjects ending (#N)	git log --first-parent --format=%s | grep -cE '\(#[0-9]+\)$'	57
true merge commits	git rev-list --count --merges	3

The repo squash-merges, so the 57 plus those 3 merge commits are the 60 first-parent landings; 57 is the right number if "PR merges" means subjects carrying a PR reference. Nothing else in either record depends on it — 78 is the figure that carries the point about 3fabf51 being a stale ancestor, and it is confirmed.

[amiddavid]

A third instance was added in #188 and is fixed in #203

Recording it so this issue's "two tests" stays accurate.

TestAnExhaustedRewindReserveNeitherStampsUnbackedMarkersNorHidesItself (added in #188) asserted snap.StashMissing != 0 absolutely, while TestStatsPublishesDanglingReplaysFromTheLiveCounter — added in the same PR, below it in the same file — deliberately drives that process-wide counter non-zero. Passed on run 1, failed on run 2. #203 makes it baseline-relative.

After #203, -count=2 on the proxy package fails only on the two tests this issue names:

--- FAIL: TestExtractEconomicsAreExported
--- FAIL: TestExpandUnresolvedSeriesRender

Two observations worth having here

The convention already existed and was applied inconsistently within one file. The same file's other assertions were already baseline-relative — the StashRefusals delta, and the live-counter comparison. So this was not a missing rule; it was one assertion written absolutely next to several written relatively. That is the more likely failure mode than nobody knowing the rule, which argues for a mechanical check over documentation.

The failure is invisible in CI. make test runs at the default -count=1, so a new absolute assertion on a process-global counter passes CI and only breaks for whoever next runs -count>1 — which is exactly how this issue's two instances survived, and how a third arrived on top of them. Anything that fixes this issue properly probably wants a CI job at -count=2 for the proxy package, otherwise instance four is a matter of time.

Also worth noting for whoever picks this up: the pattern to look for is an assertion comparing a process-wide counter (offload.StashRefusals, offload.StashMissing, adjudicate.StrayAnswered, expand.Unresolved, the metrics package aggregates) against a literal rather than against a value captured earlier in the same test.