test: synchronise every fixture-upstream capture a test goroutine reads

[amiddavid]

The defect class

A fixture handler runs on its httptest.Server's own goroutine. The HTTP round trip that
follows looks like it orders the handler's writes before the test goroutine's reads, but it is
not a happens-before edge the memory model recognises:

var forwarded []byte
upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
    forwarded, _ = io.ReadAll(r.Body)   // SERVER goroutine
    ...
}))
resp, _ := http.Post(srv.URL+"/...", ...)
resp.Body.Close()
gjson.GetBytes(forwarded, "tools")       // TEST goroutine — no edge between the two

make cover runs the suite with -race, so these are latent.

Audit

_test.go files swept	281 (every one in the repo)
closures running on another goroutine (handler bodies + go func)	95
of those, writing a variable the enclosing test also reads	46
qualifying — no mutex / channel / WaitGroup / atomic between the accesses	31 (in 11 files)
already correct, left alone	16

Method: an AST-ish scanner over every _test.go — locate each func literal whose parameters
include http.ResponseWriter or which is launched with go func, brace-match its body, collect
plain assignments and ++/-- to identifiers not declared inside it, then check whether the
enclosing test function reads that identifier outside the closure. Every hit was then read by
hand; the scanner's false positives (declaration-only "reads", writes that never leave the
handler) were discarded rather than fixed.

The 16 already-correct ones are worth naming, because they are the in-repo precedent:
proxy/tenancy_test.go's hostedFixture (guards seen/body with f.mu),
proxy/modes_test.go's captureUpstream (mu.Lock + a copying accessor), the
wg.Wait()-joined per-index slice fills in internal/extract/diag_test.go and
modes/modes_test.go, dash/sse_test.go's drained (never read outside its goroutine), and
the atomic.Int64 counters proxy/expandsplice_test.go already uses in four places.

Fixed here (29 writes, 11 files)

file	writes	shape used
internal/cheapmodel/cheapmodel_test.go	5	buffered channel
proxy/proxy_test.go	13	upstreamCapture (9) + atomic.Int64 (4)
proxy/adjudicatetool_test.go	5	upstreamCapture (3) + atomic.Int64 (2)
proxy/agentcompaction_test.go	2	upstreamCapture
proxy/expandsplice_test.go	5	upstreamCapture (3) + atomic.Int64 (2)
proxy/expandgate_test.go	1	upstreamCapture
proxy/dashexpand_test.go	1	atomic.Int64
proxy/keepalive_test.go	1	buffered channel
proxy/keepalive_wire_test.go	1	buffered channel (non-blocking receive)
proxy/prefixask_test.go	1	mutex + forwarded() accessor
proxy/tenancy_test.go	1	buffered channel (non-blocking receive)

Not fixed, deliberately

• proxy/conformance_test.go (×2) and proxy/counttokens_test.go (×1) — the three the
  reviewer found; owned by feat(dist): pure-Go releases, a cache preset, --idle-exit, and gateway conformance #141. Untouched here.
  - proxy/expandsplice_test.go:140 — calls is written in the handler but read only inside
  it, on rounds that the proxy issues sequentially. No cross-goroutine read, so not a finding.
  Withdrawn — this was wrong, and is fixed in the review round below.

Fix shapes, and why

Three, picked so the edge is visible where a reader is already looking:

1. upstreamCapture (new, proxy/proxy_test.go, package proxy_test) — a mutex-guarded
   []upstreamRound recording method, path, cloned header and body per round, modelled directly
   on hostedFixture. record() returns the 1-based round number, so a handler that must
   answer differently per round reads its own count from the return value and the captured
   counter disappears along with the captured body. This is where most of the sites are (bodies,
   headers, "what did round 2 carry").
2. atomic.Int64 for counter-only fixtures — carries its own edge, one-line change at the
   call site, and already the established shape in expandsplice_test.go.
3. Buffered channel for one-shot struct captures — cheapmodel, keepalive,
   keepalive_wire, tenancy. In the two tests where "no request arrived" is itself the
   assertion (keepalive_wire, tenancy), the receive is a non-blocking select, so that case
   stays observable as the zero value instead of becoming a deadlock.

Naming: the helper is upstreamCapture, deliberately not recordedRequest — #141 adds a
recordedRequest to proxy/ccbody_test.go in the same package. The two cannot collide at merge
in either ordering. The five internal-package (package proxy) files are a separate namespace
and use local fixes.

Detector output — honestly, it did not fire

-race does not report this shape, before or after the fix. Concretely, on the unmodified
tree:

$ go test -race -count=20 -run 'TestAnthropicBearerAuth|TestAnthropicDefaultAuthUsesAPIKey' ./internal/cheapmodel/
ok  	github.com/rossoctl/context-guru/internal/cheapmodel	1.139s

$ go test -race -count=10 -run '<the 15 affected proxy tests>' ./proxy/
ok  	github.com/rossoctl/context-guru/proxy	13.075s

So a revert-and-watch-it-fail verification is not available here, and I am not going to claim
one. What I did instead was establish that the detector was live and that the shape, not the
tests, is what it cannot see — with a standalone reproducer:

• the canonical pattern (handler assigns, test reads after the round trip): clean at
  -count=50;
• a harsher variant that assigns after the response is written and flushed, so the client can
  plausibly return first: clean at -count=200 with GORACE=history_size=7;
• a control in the same package — two concurrent requests, so two server goroutines write
  one variable — reported immediately:

WARNING: DATA RACE
Read at 0x00c000018468 by goroutine 50:
  ...racerepro.TestSanityTwoHandlersRace.func1()
      /tmp/cg-src-racesweep/racerepro/sanity_test.go:19 +0xce
  net/http.HandlerFunc.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2286 +0x47
Previous write at 0x00c000018468 by goroutine 47:
  ...racerepro.TestSanityTwoHandlersRace.func1()
      /tmp/cg-src-racesweep/racerepro/sanity_test.go:19 +0xe4
Goroutine 50 (running) created at:
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/go/src/net/http/httptest/server.go:341 +0xb2

The reproducer was scratch scaffolding and is not part of this branch.

Read-out: an in-process loopback round trip manufactures a happens-before edge through net/http's
shared internals (pooled bufio readers/writers and the atomics under them), which is enough to
blind TSan even though it is not a guarantee the memory model gives you. These are therefore
findings by inspection — unsynchronised per the memory model, not currently observable by the
detector. Practically that means low flake risk today and no promise about tomorrow: the edge is
an artefact of net/http's internals, not of anything the tests do, and it can change under us
with any Go release. The mutex, channel and atomic put the ordering in the test where it belongs.

Verification

• go vet ./proxy/ ./internal/cheapmodel/ — clean.
• gofmt -l proxy internal/cheapmodel — clean (empty).
• go build ./... — clean.
• go test -race ./proxy/ ./internal/cheapmodel/ — passing.

No behavioural change: every assertion reads the same bytes it read before, and the two
"nothing arrived" assertions are preserved by construction rather than by luck.

🤖 Generated with Claude Code

---

Review round (90b602b)

Six findings from the review, all addressed. Two changed a claim this PR was making rather than
just its code.

1. expandsplice_test.go:136 was a finding after all. The "not fixed, deliberately" entry
above said calls was read only inside the handler. It is not:
TestTheStreamedPrefixReachesTheClientBeforeTheExpandCall drives two upstream rounds and each is
served from a goroutine of the httptest.Server's own, so round 1's calls++ and round 2's
calls++ / if calls > 1 touch one variable from two goroutines — sequential, but ordered by
nothing the memory model promises. That -race stays quiet points at keep-alive reuse putting
both rounds on the same conn goroutine, which is net/http's business and not an invariant a test
may lean on. Converted to atomic.Int64 and the comment now gives the real reason. The audit
numbers above are corrected accordingly (31 qualifying, 29 fixed).

2. The stale comparative comment at :279. It read "atomic, unlike the counters in the tests
above" — a contrast this PR killed by making those counters atomics, and whose last plain int
was finding 1. Reworded to stand alone.

3. captureUpstream duplicated upstreamCapture. It is now implemented over it, via a new
bodies() accessor; the narrow signature stays because its fifteen call sites only want the
bodies in order, and sync drops out of modes_test.go. One place now holds the
synchronisation and the reasoning for it — which was the point of the helper.

4. The dead _ = hdr.Get("x-api-key"). Written as the assertion it was reaching for: an
OpenAI upstream must not receive Anthropic's header, least of all a copy of the key.

5. The lost failure diagnostic. served() restores the dump, and a String() on
upstreamRound makes it readable — %+v on a []byte field prints decimal byte values, which
defeats the point of dumping the body at all.

6. The nil dereference on the path the select exists to preserve. t.Errorf's argument was
evaluated unconditionally, so got.Header on a nil *http.Request panicked and took the test
binary down on exactly the "nothing arrived" outcome the non-blocking receive was added to keep
reportable. Pre-existing, but this PR is what made that path designed-for, so it is fixed here:
t.Fatal on nil before the header is read.

Verification of the round

The two findings that added real assertions were revert-verified, not assumed:

# x-api-key — OpenAI route's setKey mutated to also set the header
proxy_test.go:377: gateway sent x-api-key to an OpenAI upstream: "real-openai-key"

# nil guard — handler's `seen <- r.Clone(...)` dropped to simulate a request that never arrives
tenancy_test.go:517: single-tenant request never reached the upstream
#   ...where the pre-fix assertion instead panics inside t.Errorf's argument at tenancy_test.go:519

Both restored and passing afterwards. The atomic conversion is not revert-verifiable, for the
reason this PR already documents at length: -race does not fire on the shape before or after,
so it stays a finding by inspection.

gofmt -l proxy clean, go vet ./proxy/ clean, go build ./... clean,
go test -race ./proxy/ ./internal/cheapmodel/ passing.

One unrelated defect found, and filed not patched

Re-running the suite at -count=2 fails in TestExtractEconomicsAreExported and
TestExpandUnresolvedSeriesRender — both assert on process-global metrics counters they increment
themselves, so the proxy package cannot pass at -count>1. It reproduces identically on
unmodified origin/main, so it is not this branch's: filed as #192 with the cause and fix options
rather than carried here. Worth knowing while reviewing a concurrency change, since -count=N is
the instrument you would otherwise reach for.

[amiddavid]

Reviewed in a dedicated worktree at the PR head (999e1bf, parent == current origin/main, so no drift). Two passes: a hand review plus an independent automated sweep for the same defect class. Sound change and an unusually honest write-up — particular credit for saying plainly that -race does not fire on this shape rather than manufacturing a revert-and-fail story, and for proving the detector was live with a control. Nothing below blocks merge.

Claims I checked independently

My own scan of every _test.go for closures on another goroutine writing a variable the enclosing test reads agrees with the audit table:

• dash/sse_test.go:56 drained — declared outside, but genuinely read only inside the goroutine. Correctly left alone.
• proxy/counttokens_test.go:21 gotPath — real defect, deferred to #141. Verified #141 actually fixes it (gotPath, gotBody := rec.requestPath(), rec.forwarded()), so the deferral is covered rather than dropped on the floor.
• modes_test.go's captureUpstream, tenancy_test.go's hostedFixture, prefixask_test.go — guarded, correct.
• The only extra hit my scanner produced (adjudicatetool_test.go stream) is a false positive: the "stream": true map key.

Deadlock risk on the new blocking receives: there is no retry logic in internal/cheapmodel and none on the proxy's upstream path, so no size-1 channel can ever take a second send and wedge a handler goroutine behind srv.Close(). keepalive_test.go:723 is gated by an asserted status == 200 from a synchronous sendPing; the two cheapmodel receives by an asserted err == nil. keepalive_wire's non-blocking select is sound (k.dispatch = k.fire is inline, so the value is queued before sweep returns) and degrades to a clean failure rather than a false pass if that ever changes.

CI: build-test runs make cover → go test -race ./..., green at 10m22s, so all 11 files compile and pass under -race (and vet's copylocks would have caught a by-value upstreamCapture).

Findings that fall outside the diff hunks

1. proxy/expandsplice_test.go:136 — the "not fixed, deliberately" reasoning does not hold on this PR's own terms. The body says calls is "written in the handler but read only inside it, on rounds that the proxy issues sequentially. No cross-goroutine read." But TestTheStreamedPrefixReachesTheClientBeforeTheExpandCall drives two upstream rounds (round 1 serves head+tail; round 2 serves round2Answer, the source of the asserted ANSWERED), and each round is served from a httptest.Server goroutine. Handler-goroutine-1's calls++ and handler-goroutine-2's calls++ / if calls > 1 are therefore accesses to one variable from two goroutines — cross-goroutine, just never concurrent. By the memory-model criterion this PR applies everywhere else, the site qualifies.

One caveat against overstating it: TSan orders accesses by vector clock rather than by luck, so if there were genuinely no edge here -race would report it, and CI is green. That points at keep-alive connection reuse putting both rounds on the same conn goroutine — which is not a guarantee anyone gives you. So: either convert it (one line, atomic.Int64, already precedented ~200 lines below) or restate the justification as "same connection, therefore same goroutine", which is at least the true reason.

2. proxy/expandsplice_test.go:279-281 — this PR makes a pre-existing comment stale. It reads "atomic, unlike the counters in the tests above: the second round's connection is closed without a response, so there is no happens-before edge…". One of the two counters "above" is now an atomic too, so the contrast it draws is no longer true — and the remaining plain int above it is exactly the site in finding 1. Worth rewording here, since this PR is what invalidates it.

[amiddavid]

Duplicate capture helper in one package (quality, not correctness). proxy/modes_test.go:54 already has captureUpstream(t) (*httptest.Server, func() [][]byte) — mutex-guarded, 15 call sites, same package proxy_test. upstreamCapture is a strict superset of it (method / path / header / body, plus the round number), so the package now offers two overlapping ways to record an upstream and the next test picks arbitrarily.

The PR body cites captureUpstream as the in-repo precedent but does not say why it is not extended or reused. Either reimplement captureUpstream over upstreamCapture (its accessor becomes a one-liner over rounds), or leave a comment saying why both shapes exist.

[amiddavid]

Fixed in 90b602b — captureUpstream is now implemented over upstreamCapture rather than beside it:

func captureUpstream(t *testing.T) (*httptest.Server, func() [][]byte) {
	t.Helper()
	var up upstreamCapture
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		up.record(r)
		...
	}))
	t.Cleanup(srv.Close)
	return srv, up.bodies
}

with a new bodies() accessor on upstreamCapture. I kept the narrow func() [][]byte signature rather than exposing upstreamCapture to those call sites: all fifteen only ever want the bodies in order, and widening them would be churn for no reader's benefit. What mattered was the duplication you pointed at — the mutex, and the paragraph explaining why it is there, now exist in exactly one place, and sync drops out of modes_test.go's imports as a result.

[amiddavid]

Verified on 90b602b. captureUpstream is now the bodies-only view over upstreamCapture (return srv, up.bodies), sync drops out of modes_test.go, and the 15 call sites keep their signature. Aliasing is unchanged from the old accessor: both build a fresh outer slice and share the inner ones, and the handler allocates a fresh body per round, so no call site can observe a mutation it could not before. up.bodies as a method value on an addressable local binds &up correctly. Settled from my side.

[amiddavid]

Dead statement — it carries over the equally dead _ = gotXAPI, but the diff already rewrites this line, so it may as well go. Nothing reads an x-api-key assertion here; if the intent was "the gateway must not also send x-api-key", that is worth writing as a real assertion instead.

[amiddavid]

Fixed in 90b602b. Your reading of the intent was right, so it is written as a real assertion instead of deleted:

	// And only that one slot: an OpenAI upstream has no business receiving Anthropic's header,
	// least of all a copy of the key.
	if gotXAPI := hdr.Get("x-api-key"); gotXAPI != "" {
		t.Fatalf("gateway sent x-api-key to an OpenAI upstream: %q", gotXAPI)
	}

Revert-verified rather than assumed — mutating the OpenAI route's setKey to also set the header:

proxy_test.go:377: gateway sent x-api-key to an OpenAI upstream: "real-openai-key"

Restored, passes.

[amiddavid]

Verified on 90b602b. The dead statement is now a real assertion (x-api-key must be empty on an OpenAI upstream), which is strictly more than I asked for — the line went from asserting nothing to asserting the thing its presence hinted at. Settled from my side.

[amiddavid]

Minor diagnostic regression: the old message dumped the hits (got %d: %+v), so a failure told you what actually arrived. Now it prints only the count, which is the least informative part of the failure. A small accessor returning the recorded rounds (or %+v on up.round(1)/up.round(2)) would restore it.

[amiddavid]

Fixed in 90b602b via a served() accessor returning a copy of the rounds.

One thing worth recording, because the obvious fix was not enough: %+v over []upstreamRound renders the body []byte field as a list of decimal byte values, so the restored dump was ~2 KB of [123 34 109 101 ...] — arguably less useful than the bare count. So upstreamRound also gets a String():

func (r upstreamRound) String() string {
	return fmt.Sprintf("%s %s header=%v body=%s", r.method, r.path, r.header, r.body)
}

Checked by forcing the branch (n != 3), which now prints POST /inference/v1/chat/completions header=map[...] body={"messages":[...]} — readable, and it fixes %+v for every other site that dumps a round too.

[amiddavid]

Verified on 90b602b. served() plus String() on upstreamRound is the right shape — you are correct that a naive %+v restore would have been worse than the count, since %+v on the []byte body prints decimal byte values. Checked that the element-wise String() actually gets used: %+v on a []upstreamRound calls String() per element because the value receiver satisfies fmt.Stringer, so the dump renders as text. Settled from my side.

[amiddavid]

The select's stated purpose is to keep "nothing arrived" observable rather than turning it into a deadlock — but the assertion that consumes the zero value dereferences it:

if got == nil || got.Header.Get("Authorization") != "Bearer client-own-key" {
    t.Errorf("single-tenant pass-through changed: %v", got.Header.Get("Authorization"))
}

The got == nil || guard says nil is considered reachable, yet the t.Errorf argument is evaluated unconditionally, and got.Header on a nil *http.Request panics — taking down the whole test binary instead of reporting the failure. So on the one path the new select exists to preserve, the outcome is still not a clean failure.

To be fair: the t.Errorf line is pre-existing and unchanged — this is not introduced here. But the new select is what makes that path a designed-for outcome rather than a can't-happen, so it is the right patch to fix it in: hoist a nil-safe auth local, or t.Fatal on nil before the header check.

(The sibling non-blocking site, keepalive_wire_test.go:94, is fine — seen is a struct value, so the zero case is a nil body field and t.Fatals cleanly.)

[amiddavid]

Fixed in 90b602b, and you were right that this is the patch to fix it in — t.Fatal on nil before the header is read:

	select {
	case got = <-seen:
	default:
		t.Fatal("single-tenant request never reached the upstream")
	}
	if auth := got.Header.Get("Authorization"); auth != "Bearer client-own-key" {
		t.Errorf("single-tenant pass-through changed: %v", auth)
	}

Verified on the path in question, by dropping the handler's seen <- r.Clone(...) so nothing ever arrives. Before:

panic: runtime error: invalid memory address or nil pointer dereference
	github.com/rossoctl/context-guru/proxy.TestSingleTenantUnchanged(...)
		/tmp/cg-src-pr191-verify/proxy/tenancy_test.go:519
FAIL	github.com/rossoctl/context-guru/proxy	1.768s

After:

--- FAIL: TestSingleTenantUnchanged (0.25s)
    tenancy_test.go:517: single-tenant request never reached the upstream

So the outcome the select exists to preserve is now actually a reportable failure rather than a dead test binary.

[amiddavid]

Verified on 90b602b. t.Fatal in the default branch is the fix I wanted, and it does more than remove the panic: it makes the non-blocking select mean what its comment claims. "Nothing arrived" is now a named failure at the point of arrival rather than a zero value that has to survive being dereferenced two lines later. The got == nil || guard correctly goes away with it, since nil is unreachable past the select. Settled from my side.

[amiddavid]

Addressed in 90b602b. Thanks for the review — the two findings in the body were the valuable ones, because both were about a claim the PR was making rather than about its code.

Finding 1 — expandsplice_test.go:136. You are right and my "not fixed, deliberately" justification was wrong. I wrote that calls was read only inside the handler; it is written by round 1's handler goroutine and read by round 2's, which is a cross-goroutine access under exactly the criterion this PR applies everywhere else. I took the convert option rather than the restate one: -race's silence most likely means keep-alive reuse is putting both rounds on one conn goroutine, and "net/http happens to pool the connection" is not a property a test should depend on — it is the same class of accidental edge the whole PR is about, just arriving by a different route. It is now atomic.Int64 with the honest reason in the comment, and the audit table is corrected (31 qualifying, 29 fixed) with the withdrawn entry struck through rather than deleted, since it is part of the record.

Finding 2 — the stale comment at :279. Reworded to stand on its own terms. Worth noting it was stale in both halves: the counters "above" are now atomics, and the one plain int left among them was finding 1, so after that conversion there was no contrast left to draw at all.

The four inline findings are answered on their own threads. Two of them (the x-api-key assertion, the nil guard) added real assertions, so both were revert-verified rather than asserted to work — output on the threads.

One thing you will want to know before re-running the suite

Re-running at -count=2 gave me two failures that are not this branch's: TestExtractEconomicsAreExported and TestExpandUnresolvedSeriesRender both assert on process-global metrics counters they increment themselves (metrics.xReg, expand.Unresolved()), so the second run in a process reads the first's residue. Reproduces identically on unmodified origin/main, so I filed it as #192 with the cause and fix options rather than carrying it here.

It is not cosmetic in context: -count=N is the instrument for shaking out a race, and it is the one this PR's own write-up leans on. On the proxy package it currently fails for unrelated reasons, so a genuine intermittent failure would be hidden in noise that looks just like it.

Full -race suite at -count=1 is green (proxy 188s, internal/cheapmodel 1.6s), gofmt/vet/build clean.

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

[amiddavid]

CI green on 90b602b

check	result
build-test (make cover → go test -race ./...)	pass, 12m38s
purego	pass, 1m5s
trivy	pass, 13s
DCO	pass

So all eleven touched files compile and pass under -race in CI, including the reimplemented captureUpstream across its fifteen call sites.

The six findings, and where each one landed

#	finding	where	what changed
1	expandsplice_test.go:136 — the "not fixed, deliberately" reasoning does not hold	review body	calls → atomic.Int64; comment restates the real reason. Audit table corrected to 31 qualifying / 29 fixed, withdrawn entry struck through rather than deleted
2	expandsplice_test.go:279-281 — this PR made the comment stale	review body	reworded to stand alone (it was stale in both halves — the counters above are atomics now, and the one plain int among them was finding 1)
3	duplicate capture helper in one package	proxy_test.go:40	captureUpstream reimplemented over upstreamCapture via a new bodies(); sync drops out of modes_test.go
4	dead _ = hdr.Get("x-api-key")	proxy_test.go:355	written as the assertion it was reaching for; revert-verified
5	diagnostic regression — only the count survived	proxy_test.go:279	served() accessor plus a String() on upstreamRound — %+v on the []byte body rendered ~2 KB of decimal byte values, so the naive restore was less useful than the count it replaced
6	nil deref on the path the select exists to preserve	tenancy_test.go:514	t.Fatal on nil before the header read; revert-verified, and the pre-fix form demonstrated panicking

Detail and the revert-verification output are on the four inline threads and in the comment above; the PR body carries the corrected audit numbers.

Still open, deliberately

• feat(dist): pure-Go releases, a cache preset, --idle-exit, and gateway conformance #141's three sites (conformance_test.go ×2, counttokens_test.go ×1) — untouched, as before. You verified feat(dist): pure-Go releases, a cache preset, --idle-exit, and gateway conformance #141 actually fixes counttokens_test.go:21, so the deferral holds.
• test: proxy package cannot run at -count>1 — two tests assert on process-global metrics counters #192 — the -count>1 failures in TestExtractEconomicsAreExported / TestExpandUnresolvedSeriesRender. Reproduces on unmodified origin/main, so it is filed rather than carried here. Flagging it again only because -count=N is the instrument a reviewer reaches for on a change like this one, and on the proxy package it currently fails for reasons that have nothing to do with the change under review.

Nothing else from the review is outstanding. Ready for another look.

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

[amiddavid]

Re-reviewed at 90b602b. All six findings addressed; nothing new, and I have no remaining objection to merging. Per-thread confirmations are on the four inline threads; the two body findings and three notes follow.

Body finding 1 — expandsplice_test.go:136

Verified: calls is now atomic.Int64, and the replacement comment gives the real reason rather than the one I disputed — "which conn goroutine serves round 2 is net/http's business, not something the test may assume". That is the accurate framing: the point was never that a race is likely, it is that the test was relying on an invariant net/http does not offer it. Correcting the audit table to 31/29 with the withdrawn entry struck through rather than deleted is the right call — the reasoning that was wrong stays legible.

Agreed that this conversion is not revert-verifiable, and saying so beats manufacturing a verification. Worth recording why, since it is the same reason the whole PR is unverifiable that way: TSan orders accesses by vector clock, not by timing luck, so a genuinely unordered pair would be reported deterministically. Green -race therefore says an edge did exist in practice — via connection reuse or net/http's shared internals — not that the detector got lucky. That is exactly the "findings by inspection" claim the PR opened with, and it survives the round trip intact.

Body finding 2 — expandsplice_test.go:279-281

Verified: reworded to stand on its own facts ("not even a completed round trip stands between the handler's write and the assertion's read") with no dependency on what the neighbouring tests happen to use. That also fixes the half of the staleness I had not separated out — the comment was wrong about the counters above it in both directions at once.

On #192

Confirmed independently, by inspection rather than by running it: TestExtractEconomicsAreExported calls metrics.RecordExtractionSuppressed / RecordExtractionCacheLookup against the process-global registry and then asserts cg_extract_calls_total{outcome="suppressed"} 1, so a second iteration in the same binary reads 2 and the substring match fails. proxy/promexport_components_test.go is untouched by this branch (last modified in 72eaee8), so filing it rather than patching it here is correct scoping — it is a main defect found on a feature branch.

It does bound what a concurrency review of this package can claim, and that cuts both ways: -count=N is the natural instrument for this defect class, and it is unavailable on ./proxy/ for reasons unrelated to any of these fixes. Reviewers of #192 should know that repeat-run coverage of the whole package is what it unblocks, not just those two tests.

One correction to the handoff note

The branch is not behind. origin/main is 51fcd91, which is this branch's parent — 3fabf51 is a local-only ref in the primary checkout, not on the remote. So there is nothing to rebase onto and no divergence to reconcile; CI is green against a current base, not a stale one.

Verification state at 90b602b

• build-test (make cover → go test -race ./...) pass, 12m38s; purego, trivy, DCO pass.
• Re-checked by hand: no reintroduced unsynchronised capture in the four touched files; modes_test.go's import list is consistent with its new body; %+v on []upstreamRound resolves through String(); got cannot be nil past the select in tenancy_test.go.
• Still no Go on this reviewer's box, so everything here is inspection plus CI, as before.

[amiddavid]

Your correction on the base is right, and my error was worse than a stale SHA — worth stating precisely, because it changes what #192 says rather than just how it is worded.

The remote's default branch is 51fcd91, which is this branch's parent, exactly as you say. 3fabf51 is not a divergent local commit ahead of it, as my note implied — it is a stale ancestor: a local branch in the primary checkout that had not been fetched for ten commits (3fabf51..51fcd91 spans #178, #183, #184, #185 among others). That is what misled me, and it is why the note was framed backwards: there was never anything to rebase onto, and no divergence.

The sharper consequence is for #192. Both tests it names were added after 3fabf51 — promexport_components_test.go and promexport_coverage_test.go do not exist in that tree at all, having arrived with #184's metrics/grafana work. So "this is on main today (3fabf51)" was not merely imprecise, it named a tree in which the defect cannot be observed.

The reproduction itself is unaffected, and I checked rather than assumed. The tree I ran against was built from the remote default-branch ref, which resolved to 51fcd91 — not from the stale local branch — and independently against this PR's head 999e1bf, whose parent is also 51fcd91. Both failed identically. So the finding and the cause hold; only the SHA quoted in the prose was wrong. #192's body is corrected, with the error and its scope recorded there rather than silently overwritten.

Noted on the two substantive points:

• Your framing of why green -race is not luck is better than mine and I will use it: TSan orders by vector clock, so a genuinely unordered pair is reported deterministically — green therefore says an edge did exist in practice, via connection reuse or net/http's internals, not that the detector missed it. That is a stronger statement of "findings by inspection" than the PR body currently makes, and it is the reason the atomic conversion is unverifiable by reverting rather than an excuse for it.
• Agreed on test: proxy package cannot run at -count>1 — two tests assert on process-global metrics counters #192's scope: what it unblocks is repeat-run coverage of the whole proxy package, not those two tests. I have not widened its title to claim more than was demonstrated, but the fix-options section is written against the package-level goal.

Nothing outstanding from my side. Thanks for catching the base — it was the one claim in the handoff I had taken from a local ref without checking it against the remote.

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

[amiddavid]

Correcting my own base note above, because it was wrong about the mechanism.

I wrote that 3fabf51 is "a local-only ref in the primary checkout, not on the remote." That is false. git merge-base --is-ancestor 3fabf51 51fcd91 succeeds: the commit is on the remote, as an ancestor of the default branch. It is a stale local branch, not an unpushed one — the opposite failure mode from the one I named, and someone reading my sentence later would have gone looking for an unpushed commit that does not exist.

The operative conclusion is unchanged and now rests on the right fact: origin/main is 51fcd91, this branch's parent, so there is nothing to rebase and no divergence.

Two figures in the handoff that reached #192 are also off, and since the range is what makes that argument work, it is worth having them exact:

• The gap is 78 commits (git rev-list --count 3fabf51..51fcd91), 62 of them first-parent PR merges, reaching back past feat(dash): per-model prices, prompt-cache savings, gate reasons, and a host dashboard #83 — not ten commits spanning fix(metrics): scope extraction accounting per component, and log every extraction call #178–fix(dash): let the invalid-config banner clear itself once the config is fixed #185.
• The two tests test: proxy package cannot run at -count>1 — two tests assert on process-global metrics counters #192 names were not added by feat(grafana): fill the two empty rows, graph the metrics nobody was watching, and alert on the observability path #184. TestExtractEconomicsAreExported came in with d9e2f24 (feat(dash): per-model prices, prompt-cache savings, gate reasons, and a host dashboard #83, promexport_components_test.go); TestExpandUnresolvedSeriesRender with 3ebc65d (fix(dash): report unpriced components in tokens, state the cache-frozen ceiling, and export the counters that only reached /stats #155, promexport_coverage_test.go).

Neither changes the conclusion — both files are absent at 3fabf51, so the tree named in #192's original body genuinely could not exhibit the defect, and the reproduction stands because it was run against the remote ref and against 999e1bf's parent, both 51fcd91. I have put the same two corrections on #192 so the issue's own record carries them.

[amiddavid]

Both corrections are mine to own, and both are right — I verified them rather than just accepting them:

• 78 commits (git rev-list --count 3fabf51..51fcd91 = 78). My "ten" came from reading a git log --oneline 3fabf51..51fcd91 | head as though it were the whole range. The | head was mine; the ten commits I then named are simply its tail, exactly as you say.
• d9e2f24 (feat(dash): per-model prices, prompt-cache savings, gate reasons, and a host dashboard #83) and 3ebc65d (fix(dash): report unpriced components in tokens, state the cache-frozen ceiling, and export the counters that only reached /stats #155), confirmed by git log --diff-filter=A on each path. The feat(grafana): fill the two empty rows, graph the metrics nobody was watching, and alert on the observability path #184 attribution was an inference from "the metrics/grafana work landed in that window" that I never checked against the paths.

#192's body is amended inline with the right figures, and the amendment records that both were wrong and why, so the issue's own record does not rely on a reader finding your comment.

One figure in the correction does not reconcile

"62 of them first-parent PR merges" does not match any measure I can find on that range:

measure	count
total commits	78
first-parent commits	60
first-parent subjects ending (#N)	57
actual merge commits (--merges)	3

The repo squash-merges, so "PR merges" here are non-merge first-parent commits ending in (#N) — 57. There are only 3 true merge commits in the range. So the figure is somewhere between 57 and 60 depending on what you meant, and 62 exceeds even the first-parent total.

Raising it only because it is the same class of error as mine and it is now in #192's permanent record next to figures I have just corrected — by your own standard that the range is what makes the argument work. It changes nothing: 78 is the number that carries the point, and the point survives at 57, 60 or 62.

Net state

Everything material is agreed and unchanged: the defect, its cause, the reproduction against 51fcd91 and 999e1bf, the absence of both files at 3fabf51, and all six review findings on this PR. Two things improved on the way through — your mechanism correction (stale ancestor, not unpushed commit) and your TSan framing, which is a better statement of the PR's own core claim than the body currently makes.

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

[amiddavid]

My 62 of them first-parent PR merges two comments up is wrong — it exceeds the first-parent total, so it could not have been a subset of anything. Exact on 3fabf51..51fcd91: 78 commits, 60 first-parent, 57 first-parent subjects ending (#N), 3 true merge commits. The repo squash-merges, so 57 is the right figure for PR landings by subject. Corrected in full on #192, where the same figure sits in the permanent record. Nothing else depends on it: 78 is what carries the stale-ancestor point, and that one is confirmed.