rest: trustedProxies cache relies on a conventional once-write invariant — consider making it structural

[SyniRon]

This was generated by AI — surfaced during the #194 swarm review (follow-up to #190).

Observation

rest/clientip.go keeps the parsed trusted-proxy set in a package-level var trustedProxies []*net.IPNet, written once by InitTrustedProxies() at startup and read thereafter by clientIP. The "write once before any listener serves, read-only after" invariant is enforced only by call ordering + a comment, not by the type — any code in package rest could reassign it, and the read/write are unsynchronized.

Why it's benign today

• The composition root wires InitTrustedProxies() before serving (and servers/server_test.go has a source-level guard pinning that ordering and the fatal-on-malformed behavior).
• A read before init yields nil → trust-nothing → the safe direction.
• Boot is single-threaded, so there is no real race in the current lifecycle.

Possible direction (for triage — low priority)

Per the type-design review, wrap the cache in atomic.Pointer[[]*net.IPNet] behind an unexported accessor (~5 lines), turning the once-write/read-after lifecycle from a documented convention into a structural one and removing the theoretical race. Only pays off if a concurrent writer ever appears on this surface (e.g. the deferred per-key rate-limiting work in PRD #112).

Design context: ADR 0005 (docs/adr/0005-trusted-proxy-client-ip.md). Surfaced by the type-design review on PR #194. needs-triage.

[SyniRon]

This was generated by AI during triage.

Agent Brief

Category: refactor (structure without functional change)
Summary: Make the trusted-proxy cache's once-write/read-after lifecycle structural (atomic pointer + unexported accessor) instead of convention-only.

Current behavior:
The parsed trusted-proxy set lives in a package-level mutable slice in the rest package, written once by InitTrustedProxies at startup and read thereafter by clientIP (the two 401 log sites). The "write once before any listener serves, read-only after" invariant is enforced only by call ordering plus a comment — any code in the package could reassign it, and the read/write are unsynchronized. It is benign today: the composition root wires InitTrustedProxies before serving (with a source-level wiring guard pinning the ordering and the fatal-on-malformed behavior), a read before init yields nil → trust-nothing (the safe direction), and boot is single-threaded so there is no real race.

Desired behavior:
The once-write/read-after lifecycle is enforced by the type, not by convention. The trusted set is published atomically by InitTrustedProxies and read atomically by clientIP through an unexported accessor, removing the theoretical data race and making "you can't observe a half-written set" structural. Behavior is otherwise identical: empty/unset still trusts nothing, a non-empty malformed value is still a fatal startup error, and resolveClientIP stays pure (no globals).

Key interfaces:

• The package-level trusted-proxy cache — change from a plain mutable slice to an atomic pointer (atomic.Pointer over the slice type) read/written through an unexported accessor.
• InitTrustedProxies — still reads/parses TRUSTED_PROXIES once and returns an error on a non-empty malformed value; now publishes via an atomic store rather than a direct assignment.
• clientIP — still the thin ambient wrapper over resolveClientIP; now loads the set via the atomic accessor. resolveClientIP itself stays pure and unchanged.

Acceptance criteria:

• The trusted-proxy set is stored and published atomically; no unsynchronized package-level mutable slice remains for it.
• Empty/unset TRUSTED_PROXIES still trusts nothing (resolveClientIP ignores forwarding headers).
• A non-empty malformed TRUSTED_PROXIES is still a startup error the composition root makes fatal.
• A read before init still yields the safe trust-nothing direction.
• resolveClientIP remains pure and exhaustively table-testable; existing client-IP tests pass (adjust only the test helper that pokes the cache directly, if needed).
• The existing composition-root wiring guard (InitTrustedProxies before any listener serves, fatal on malformed) still holds.
• go test ./... and the race detector (go test -race) are green for the rest and servers packages.

Out of scope:

• The deferred per-key rate-limiting work in PRD PRD: Goodbye gRPC — plain net/http rewrite with DB indexing, de-caching, TDD contract suite, and observability #112 — this issue only hardens the cache; it does not add a concurrent writer.
• Any change to ADR 0005's client-IP resolution rule or the forwarding-header trust logic.
• The composition-root call ordering or the fatal-on-malformed behavior — only how the value is stored/published changes.

---

Dependency / sequencing (graph around #134):

• Independent of Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134 — no hard edge either direction. Benign today; the payoff (race-safety) has no trigger until a concurrent writer exists, which is the deferred per-key rate-limiting work, not Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134.
• Touches the same composition-root / trusted-proxy surface Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134 rebuilds (the single-listener wiring; the wiring guard already carries a CROSS-ISSUE #134 note). Lowest priority of the three. Can land before Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134 (cheap; Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134's rebuild then builds on the structural version) — but it does not gate Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134 and Phase 3 cutover: single listener, datastore type-swap, Keycloak removal, deliberate breaks live #134 does not gate it.