fix: data race in the /metrics handler

[nisay759]

fix: data race in the /metrics handler

Source: fix/metrics-race-and-session-count
Target: dev
Commit: 27faf5c
Diffstat: 2 files changed, 104 insertions(+), 2 deletions(-)

---

Summary

metricsHandler assigns h.options.Stats — a *Metrics shared by every protocol server — to a local variable. That copies the pointer, not the struct, so each request writes into the one shared struct and reads its counters non-atomically while the protocol servers update them with atomic adds.

The fix snapshots the counters into a local value using atomic loads, so the shared struct is never written and the encoded numbers are self-consistent.

Scope is deliberately narrow: only the handler's read path changes. The session counters themselves are untouched — the snapshot simply reads Sessions and SessionsTotal alongside the protocol counters.

---

The bug

Options.Stats is a *Metrics (pkg/server/server.go:133). The handler began with:

interactMetrics := h.options.Stats     // pointer copy, NOT a value copy
interactMetrics.Cache = GetCacheMetrics(h.options)
interactMetrics.Cpu = GetCpuMetrics()
interactMetrics.Memory = GetMemoryMetrics()
interactMetrics.Network = GetNetworkMetrics()

Because interactMetrics is the same pointer, those four assignments mutate the shared struct.
Two distinct races follow:

• Write/write — two concurrent /metrics requests assign .Cache, .Cpu, .Memory and .Network on the same struct.
• Read/write — the JSON encoder reads the counter fields non-atomically while the protocol servers issue atomic.AddUint64 / atomic.AddInt64 (dns_server.go, http_server.go, smtp_server.go, ftp_server.go, smb_server.go, and eleven call sites in ldap_server.go).

The fix

interactMetrics := Metrics{
    Dns:           atomic.LoadUint64(&h.options.Stats.Dns),
    Ftp:           atomic.LoadUint64(&h.options.Stats.Ftp),
    Http:          atomic.LoadUint64(&h.options.Stats.Http),
    Ldap:          atomic.LoadUint64(&h.options.Stats.Ldap),
    Smb:           atomic.LoadUint64(&h.options.Stats.Smb),
    Smtp:          atomic.LoadUint64(&h.options.Stats.Smtp),
    Sessions:      atomic.LoadInt64(&h.options.Stats.Sessions),
    SessionsTotal: atomic.LoadInt64(&h.options.Stats.SessionsTotal),
}

Every writer was confirmed to use atomic adds, so the loads pair correctly.

---

Impact

Confined to observability — nothing gates on these counters, and the interaction-capture pipeline is untouched. But /metrics is exactly the endpoint an operator scrapes on an interval, so the racy path is the one under sustained concurrent load in production, and Sessions is the only in-app signal of how many clients a server is carrying.

---

Test plan

New file pkg/server/metrics_race_test.go (88 lines, 2 tests):

• TestMetricsHandlerDoesNotMutateSharedStats — asserts the handler leaves
  options.Stats.Cache/Cpu/Memory/Network nil. A deterministic check that fails without -race.
• TestMetricsHandlerConcurrent — four goroutines updating counters against four goroutines hitting metricsHandler. Intended for -race.

Both were checked against the unfixed handler, confirming they fail for the right reasons rather than passing vacuously:

Test	Result without the fix
TestMetricsHandlerDoesNotMutateSharedStats	FAIL — Expected nil, but got: &storage.CacheMetrics{...}
TestMetricsHandlerConcurrent	FAIL — race detected during execution of test

With the fix applied:

go build ./...        ok
go vet ./...          ok
go test ./...         ok
go test -race ./...   ok — 0 races

go.mod / go.sum unchanged.

---

Reviewer notes

CI would not have caught this. .github/workflows/build-test.yml:33 has a step named "Race Condition Tests" that runs:

- name: Race Condition Tests
  run: go build -race ./...

That only compiles with the detector enabled — no test is ever executed under -race. Changing it to go test -race ./... would make the step do what its name claims. Left out of this PR as unrelated, but recommended as a follow-up.

Misc note

The code was produced using Claude AI

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4af73b02-7872-45fe-8dd1-5989c233b4d8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}