Publish reflector targets by copy-on-write, fix unstable proxy-entry key, gate with -race

[aszarama]

Four independent correctness fixes in the registration reflector, plus a race-detector gate in CI so they stay fixed. No new API surface, no feature.

First of a two-PR stack. #118 builds the per-request retargeting feature on top.

The rr.targets race

rr.targets is a plain map with no synchronization. An earlier assumption was that it is only written at render/startup — that is wrong. Start spawns a registration goroutine that retries with backoff, so getProxy can register entries long after Start has returned, while the reflector is already serving traffic. That races parseTargetUri's read on the ServeHTTP path.

A concurrent map read-and-write is a Go runtime throw, not a tolerable race. recover cannot catch it, so the agent process dies inside the customer's network. Reproduced on main with a probe that does what production does — ServeHTTP on one goroutine, ProxyURI on another:

WARNING: DATA RACE
Read at 0x00c0003ce030 by goroutine 17:
  ...parseTargetUri()  reflector.go:202
  ...ServeHTTP()       reflector.go:296
Previous write at 0x00c0003ce030 by goroutine 18:
  ...getProxy()        reflector.go:158
  ...ProxyURI()        reflector.go:264
FAIL  github.com/cortexapps/axon/server/snykbroker  0.612s

Fixed by publishing the map copy-on-write behind an atomic.Pointer, not by locking:

• readers (parseTargetUri, getUriForTarget) do a single atomic load and index a map that is never mutated after publication, so the request path synchronizes on nothing;
• getProxy builds a fresh copy and swaps it in under a CAS retry, so two concurrent registrations cannot lose an entry.

The access pattern is what makes this fit: writes happen at broker start, reads happen on every relayed request. Copy-on-write also makes the invariant structural — "the map is frozen once published" is the only state a reader can observe — rather than depending on every future caller remembering to take a lock.

Registration off HTTP handler goroutines

Two callers reached getProxy for reasons unrelated to starting the broker: the POST /__axon/broker/reregister handler and the 5-minute auto-register timer. Both only need to know whether registration changed.

getUrlAndToken is split in two. refreshTokenInfo re-registers and reports the change and touches nothing else; getUrlAndToken additionally registers the reflector's default entry, so it belongs to the start path alone. The reregister handler and the auto-register timer now use the former.

The reregister handler also hands its restart to the single restart consumer via requestRestart instead of calling Restart() inline, which removes a second goroutine that could otherwise enter Start() concurrently with the restart consumer. It now writes an explicit 200 as well, where before it fell through to an implicit one.

Response header set before publication

addResponseHeader ran after the entry was copied into the map, so the stored copy kept a nil responseHeaders map. It worked only because ModifyResponse closes over the pre-copy entry rather than reading the map value. Moved above the copy so the published entry carries the header.

Order-unstable proxyEntry.key()

key() hashed one |name=resolverKey suffix per header while ranging pe.headers in Go map order, so two newProxyEntry calls for the same URI and header set could hash differently and register duplicate entries. It only looked stable because hashCode memoizes within a single entry. accept.github.app.json rules carry two headers each, so this is reachable today. Header names are now sorted before hashing.

parseTargetUri linear scan

It ranged over every target calling key() per iteration, even though rr.targets is already keyed by exactly that value. Replaced with a direct lookup — equivalent, and O(1).

httpServer Start/Close

Start assigned h.server from inside the serve goroutine, so Close could read and write it concurrently. The server is now constructed before the goroutine is spawned, which gets a local reference.

This also removes a time.Sleep(100 * time.Millisecond) that was masking the race. Calling it out because it is a behaviour change rather than part of the race fix: Start no longer needs to wait, since the listener is already bound before it returns.

Worth more than it looks — this one fix is the sole cause of four failures on main under -race (TestBuildServeStack, TestProxyWithNoHeaders, TestHeaderOverwriting, TestGetProxyAndProxyURI), because so many tests start and close an httpServer.

Tests

TestConcurrentGetProxyAndParseTargetUri reproduces the map race: 16 goroutines registering distinct target URIs while 16 more resolve seeded hashes. Against a version that mutates the map in place it does not merely warn — it dies with a runtime throw. It also asserts the final map length, so a lost copy-on-write update fails it too.

TestRelayReRegisterDoesNotTouchReflector asserts the reregister endpoint registers no reflector entries, so the split above cannot quietly regress.

TestProxyEntryKeyStableAcrossHeaderOrder constructs 20 entries from the same three-header map to flush out iteration-order dependence.

Why the CI gate is scoped, not suite-wide

-race is not run in CI today, and the tree is nowhere near race-clean. Measured on main (golang:1.26): go test -race ./... fails 5 of 12 packages — cmd, server, server/handler, server/http, server/snykbroker — with 23 failing tests and 92 race warnings. The largest single source is supervisor.go (process lifecycle, ~137 stack-frame hits), followed by server/handler's manager and scheduled-entry. These are production-code races, and fixing them is a separate multi-package project that should not gate this PR.

Turning -race on suite-wide would just add a red build. Gating by package does not help either, since the reflector shares server/snykbroker with the supervisor races.

So make test-race gates at test granularity: the packages that are clean today (RACE_CLEAN_PKGS), plus the reflector suite alone within server/snykbroker via a -run regex generated from the test files, so new reflector tests are covered automatically and there is no hand-maintained list to rot.

It is green on the fix and red on the bug:

	reflector tests under -race
main	3 fail, 9 race warnings
this PR	35 pass, 0 races

Broaden RACE_CLEAN_PKGS as the pre-existing races get fixed — noted in the Makefile. I can file the supervisor / handler / server races as separate issues if useful.

Verification

Full suite green without -race; make test-race green; go vet clean; gofmt clean on every file touched. (server/snykbroker/ws_proxy_test.go and server/http/axon_handler.go already fail gofmt -l on main — pre-existing, left alone.)

One flake fixed while proving the gate

Turning the gate on surfaced a rare failure in the WebSocket tests: a tunnel goroutine logging through the zaptest logger after its test had returned, racing testing.tRunner marking the test done. It reproduced roughly once in eight runs.

This is the harness, not the agent — runTunnel does wait on both copy directions, and in production the logger outlives every tunnel. But a gate that flakes is not worth having, so the reflector test env now waits for ActiveConnections to reach zero as its last cleanup. The counter is decremented only after both copy goroutines have returned, so zero means none of them can log again.

A separate flake this PR does not fix

go test ./server/snykbroker/ dies with signal: killed and no --- FAIL about half the time under Docker, consistent with the OOM killer rather than any test failing. It is pre-existing and unrelated to this stack — measured at 10 runs each under identical container settings:

branch	pass	fail
main	5	5
this PR	6	4
#118 rebased on this PR	5	5

Flagging it because CI already runs make -C agent test, so it can redden builds independently of these changes. Worth its own issue.