fix(v2/httpserver): synchronize Addr/ProbeAddr with Start writes

Summary

httpserver.Server.Start writes the bound listener fields (s.ln, s.probeLn) at v2/httpserver/server.go:234,241 without any synchronisation against concurrent reads through the public Addr() / ProbeAddr() accessors. The accessors are documented for cross-goroutine use ("Use this after starting with Addr :0 to discover the bound port"), so callers reasonably expect them to be race-safe — yet go test -race flags two distinct races whenever a caller spawns go app.Run(ctx) and then polls the accessors to discover the bound ephemeral ports:

  1. Interface-field assignment race. The two-word write of the net.Listener iface header to s.probeLn races with the nil-check read in ProbeAddr() (server.go:217).
  2. TCPListener byte-read race. (*net.TCPListener).Addr() reads bytes that are still being written by net.ListenConfig.Listen in Start.

Sample race trace (matches the trace in the AR work item):

WARNING: DATA RACE
Write at 0x00c0003b2270 by goroutine 11:
  ...httpserver.(*Server).Start()      server.go:234
Previous read at 0x00c0003b2270 by goroutine 12:
  ...httpserver.(*Server).Addr()       server.go:207

Fix

Guard the two listener fields with a sync.Mutex:

  • Start now publishes both listeners under the lock after both Listen calls succeed. Readers therefore never observe a half-initialised state (e.g. the app listener bound but the probe still nil) either.
  • Addr() and ProbeAddr() take the same lock around the nil-check and the Addr() call.

The lock is uncontended in production (one writer in Start, occasional post-Start readers from operators/tests) and adds no measurable overhead. http.Server keeps its own listener reference internally, so Shutdown is unaffected.

This matches the upstream-resolution sketch in the work item.

Test plan

  • TestServer_Addr_ProbeAddr_RaceFree added — polls Addr() / ProbeAddr() from a separate goroutine while Start runs. Reproduces both races under -race against master (verified) and passes after the fix.
  • go test -race ./... (full v2 module) passes.
  • ./scripts/test.sh passes.
  • ./scripts/golangci-lint.sh reports 0 issues for both modules.

Downstream unblock

Surfaced by gitlab-org/ops/artifact-registry#100 (closed), whose S01 Step 6 lifecycle integration tests currently skip under -race pending this fix. Once this lands in a v2 patch release, AR can bump its go.mod pin and drop the testing.RaceEnabled skip in cmd/artifact-registry/main_test.go.

  • gitlab-org/labkit v2/httpserver/server.go:213-232 — the previously unsynchronised accessors
  • gitlab-org/labkit v2/httpserver/server.go:234-262 — the writer in Start
Edited by Suleimi Ahmed

Merge request reports

Loading
Loading