Fix scale and correctness defects, add TX(avg) and a per-host throughput breakdown

.github/workflows/go.yml

@@ -8,6 +8,11 @@ on:
     branches:
     - master
     - main
+
+# Least privilege: nothing in this job writes to the repository.
+permissions:
+  contents: read
+
 jobs:
   build:
     name: Build and test
@@ -31,6 +36,30 @@ jobs:
     - name: Vet
       run: go vet ./...
       shell: bash
+    # Built from source with this job's own Go toolchain rather than pulled as a
+    # release binary. golangci-lint refuses to run when the Go version it was
+    # built with is older than the module's go directive, and the published
+    # binaries lag Go 1.26 -- golangci-lint-action@v6 with "version: latest"
+    # installed a v1 build on Go 1.24, which could neither read the v2 config
+    # nor accept the module. Installing it here makes the two match by
+    # construction and pins an exact, tested version instead of a moving one.
+    - name: Lint
+      run: |
+        go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2
+        golangci-lint run --timeout 5m
+      shell: bash
     - name: Test
-      run: go test -race ./...
+      run: go test -race -count 1 -timeout 10m ./...
+      shell: bash
+    # The concurrency fixes are the kind that pass once and fail on the tenth
+    # run, and single-proc scheduling exposes different interleavings. The whole
+    # suite is repeated rather than a -run subset: a name pattern silently
+    # selects nothing when a test is renamed, and `go test -run` exits 0 in that
+    # case, so the gate would quietly stop testing anything. The full suite at
+    # -count 10 takes about 40s per GOMAXPROCS setting.
+    - name: Test repeatedly for flakiness
+      run: |
+        for procs in 1 4; do
+          GOMAXPROCS=$procs go test -race -count 10 -timeout 15m ./...
+        done
       shell: bash

.github/workflows/release.yml

@@ -25,7 +25,7 @@ jobs:
         go-version: ${{ matrix.go-version }}
         check-latest: true
     - name: Test
-      run: go test ./...
+      run: go test -race -count 1 -timeout 10m ./...
       shell: bash
     - name: Release
       uses: goreleaser/goreleaser-action@v6

.golangci.yml

@@ -1,27 +1,44 @@
-linters-settings:
-  golint:
-    min-confidence: 0
-
-  misspell:
-    locale: US
+# Migrated to the v2 schema. The previous config was pinned to
+# golangci-lint 1.20.0 and enabled deadcode, structcheck, golint and gosimple,
+# all of which have since been removed or folded into other linters -- so
+# `golangci-lint run` failed outright on any current release and the lint gate
+# documented in CLAUDE.md was never actually running.
+version: "2"
 
 linters:
-  disable-all: true
+  default: none
   enable:
-    - typecheck
-    - goimports
-    - misspell
     - govet
-    - golint
     - ineffassign
-    - gosimple
-    - deadcode
-    - structcheck
+    - misspell
+    - staticcheck
+    - unused
+    # Unchecked writes and closes are how the descriptor and connection leaks
+    # in this codebase stayed invisible.
+    - bodyclose
+    - errorlint
+    - unconvert
+    - usestdlibvars
+
+  settings:
+    misspell:
+      locale: US
+
+  exclusions:
+    presets:
+      - comments
+    rules:
+      # hperf's error strings are capitalized throughout and surface directly
+      # in the CLI output; that is a deliberate house style, not a defect.
+      - text: "error strings should not be capitalized"
+        linters:
+          - staticcheck
+
+formatters:
+  enable:
+    - gofmt
+    - goimports
 
 issues:
-  exclude-use-default: false
-  exclude:
-      - should have a package comment
-      - error strings should not be capitalized or end with punctuation or a newline
-service:
-  golangci-lint-version: 1.20.0 # use the fixed version to not introduce new linters unexpectedly
+  max-issues-per-linter: 0
+  max-same-issues: 0

CLAUDE.md

@@ -20,14 +20,23 @@ go install github.com/minio/hperf/cmd/hperf@latest
 
 ### Run tests
 ```bash
-go test ./...
+go test -race ./...
 ```
 
+CI runs the suite with `-race`, and runs the concurrency tests repeatedly at
+`GOMAXPROCS=1` and `4`, because the locking bugs in this codebase pass a single
+clean run and fail the tenth.
+
 ### Lint
 ```bash
 golangci-lint run
 ```
 
+`.golangci.yml` uses the v2 schema. It was previously pinned to golangci-lint
+1.20.0 and enabled linters that no longer exist, so this command failed outright
+and the gate never ran - if it starts erroring on a config version again, that is
+what happened.
+
 ### Build Docker image
 ```bash
 docker build -t hperf:latest .
@@ -59,31 +68,45 @@ docker build -t hperf:latest .
 
 ### Test Types
 
-- **RequestTest** (latency command): Sends fixed-size HTTP PUT requests with configurable delay between requests. Measures TTFB (Time To First Byte), RMS (Round-trip time), and tracks per-request latency
-- **StreamTest** (bandwidth command): Sends continuous HTTP streams with configurable concurrency. Measures throughput (TX bytes/sec) with multiple concurrent connections
+- **RequestTest** (`latency` and `requests` commands): Sends fixed-size HTTP PUT requests with configurable delay between requests. Measures TTFB (Time To First Byte), RMS (Round-trip time), and tracks per-request latency. `latency` is a fixed probe (payload 1000, concurrency 1, 200 ms delay) and does not register those flags; `requests` registers them and honours them. Both share `runLatency`
+- **StreamTest** (`bandwidth` command): Sends continuous HTTP streams with configurable concurrency. Measures throughput (TX bytes/sec) with multiple concurrent connections. Payload and buffer are pinned to 32000, so only `--concurrency` is tunable
+
+Note a stream request never completes - it ends only when the test is cancelled -
+so anything derived from request completion (RMS, and a completion-based request
+count) is meaningless for a bandwidth test.
 
 ### Critical Implementation Details
 
 **Server IP handling**: Servers need `--real-ip` flag when `--address` differs from external IP (or is a wildcard). Without this, servers report the bind address in stats and cannot recognize themselves in the host list (`isSelfHost` in server/server.go).
 
 **Address handling**: All host entries pass through `shared.NormalizeHost` in `ParseHosts`, which strips brackets and canonicalizes IP literals, so one spelling reaches the wire, the self filters and the output. Use `shared.URLHostPort` when building a URL (it percent-encodes an IPv6 zone as RFC 6874 requires), `shared.HostOnly` to drop a port for display, and `shared.SameHost` to compare hosts - never substring matching, which used to make `--real-ip 10.0.0.1` swallow the peer `10.0.0.10`. The server binds with `fiber.NetworkTCP`, so a wildcard bind is dual-stack.
 
-**Data persistence**: Test results are saved to `--storage-path` (default: current directory + `/hperf-tests/`). Each data point is JSON with a prefix byte (0=DataPoint, 1=ErrorPoint) followed by newline. Files are named by test ID.
+**Data persistence**: Test results are saved to `--storage-path` (default: current directory + `/hperf-tests/`). Each data point is JSON with a prefix byte (0=DataPoint, 1=ErrorPoint) followed by newline. Files are named `<testID>.<index>`. Test IDs are validated by `shared.ValidateTestID` before touching the filesystem: they are a filename component supplied by whichever client asked for the test, so an unvalidated ID could escape `--storage-path`, and an empty one made the cleanup glob match every saved test. Globs that select a test's files must be anchored with the separator (`id + ".*"`, never `id + "*"`).
+
+**Server locking contract**: written out above `type test` in server/server.go and worth reading before touching that file. In short: `testLock` guards only the package-level `tests` slice; `t.M` guards `errors`, `errMap`, `DPS` and `cons`; `t.endedAt` carries liveness so callers can test it without `t.M`; `netPerfReader.m` guards only the TTFB/RMS watermarks and `hasStats` is atomic. The three locks are never held together, iterators snapshot under one lock and then work unlocked, and no blocking work (socket write, disk write, print) happens under `t.M` — `AddError` takes `t.M`, so persisting under it would deadlock as well as stall.
+
+**Websocket peers**: never retain the `*websocket.Conn` that gofiber hands a handler. gofiber/contrib pools that wrapper and nils its embedded conn when the handler returns, so a stored wrapper later writes into an unrelated client's socket. Wrap it in `wsPeer` (server/server.go), which holds the inner non-pooled `fasthttp/websocket.Conn`, serializes writes on its own leaf mutex, and applies a write deadline so a stalled client cannot stall the test. Control frames deliberately bypass that mutex.
+
+**Per-connection memory**: `fiber.Config.ReadBufferSize`/`WriteBufferSize` are allocated per concurrent connection, not once. A full mesh opens `(hosts-1) x concurrency` inbound connections per server, so these constants multiply by thousands — they are 64 KiB (`serverReadBufferSize`), measured at ~90 KiB RSS per connection. The only hard floor is that the largest inbound request header must fit or fasthttp answers 431; `--payload-size` is a body size and is streamed, never buffered whole.
+
+**Concurrency model**: Each server maintains a semaphore channel per remote host (`concurrency chan int`) limiting concurrent requests. Workers pull from this channel, send requests, then return the slot (`startPerformanceReader`/`sendRequestToHost`). The channel starts full, so any direct call to `sendRequestToHost` must take a slot first or its deferred return blocks. Every response body is drained and closed on all paths — net/http cannot release a connection whose body is still open.
+
+**Stats collection**: `pollHostStats` publishes an immutable `hostStats` snapshot through an `atomic.Pointer`, so `generateDataPoints` reads memory, CPU and drop counters without locking and without a nil dereference before the first poll. `cpu.Percent` blocks for a second by design, which is why it lives on its own goroutine.
 
-**Concurrency model**: Each server maintains a semaphore channel per remote host (`concurrency chan int`) limiting concurrent requests. Workers pull from this channel, send requests, then return the slot (server/server.go:700-723).
+**Dropped packets**: reported as a delta since the test started, covering receive *and* transmit drops, scoped to the interface resolved from `--real-ip`. `-1` means no usable counter, which is distinct from zero. The transmit column is the one that matters for a saturating sender.
 
-**Stats collection**: Separate goroutine collects system stats (memory, CPU, dropped packets from `/proc/net/dev`) every second (server/server.go:260-287). Stats are locked with mutexes when updating high/low watermarks.
+**Throughput reporting**: the client aggregates incrementally in `client/aggregate.go`, updated per incoming data point under `responseLock` and read once per tick — never by rescanning the accumulated slice. `TX(max)`, `TX(min)` and `TX(avg)` summarize one population (per-flow, per-second rates), so `min <= avg <= max` holds. A single flow is ~1/(hosts-1) of a host's aggregate; that is the definition, not a bug, and it is why comparing `TX(max)` to a NIC counter mismatches by roughly the host count. `generateDataPoints` divides bytes by the *measured* elapsed window, so a slow sampling loop reduces the number of samples but does not bias the rate.
 
 ## Key Configuration Parameters
 
 - `--hosts`: Supports ellipsis patterns (`10.10.1.{2...10}`), comma-separated lists, or file input (`file:/path/to/hosts`)
-- `--id`: Test identifier for start/stop/listen/download operations. Auto-generated from Unix timestamp if not provided
+- `--id`: Test identifier for start/stop/listen/download operations. Auto-generated from Unix timestamp if not provided (for `bandwidth`, `latency` and `requests`). Letters, digits, `-`, `_` and `.` only, max 64 characters; validated server-side before it becomes a filename
 - `--port`: Server port (default: 9010)
-- `--concurrency`: Concurrent requests per host (default: 2 × GOMAXPROCS)
+- `--concurrency`: Concurrent requests per host (default: 2 × GOMAXPROCS). Registered on `bandwidth` and `requests` only
 - `--duration`: Test duration in seconds (default: 30)
-- `--buffer-size`: Network buffer size in bytes (default: 32000)
-- `--payload-size`: HTTP payload size in bytes (default: 1000000)
-- `--request-delay`: Delay between requests in milliseconds (default: 0)
+- `--buffer-size`: Network buffer size in bytes (default: 32000). Registered on `requests` only
+- `--payload-size`: HTTP payload size in bytes (default: 1000000). Registered on `requests` only
+- `--request-delay`: Delay between requests in milliseconds (default: 0). Registered on `requests` only
 - `--save`: Save test results on server for later retrieval (default: true)
 - `--ip-family`: Address family used when resolving hostnames in `--hosts`: `auto`, `4` or `6` (default: auto)
 - `--dns-server`: Resolve hostnames in `--hosts` through this DNS server
@@ -96,7 +119,8 @@ docker build -t hperf:latest .
 - System metrics: shirou/gopsutil for CPU/memory stats
 - UI: charmbracelet/lipgloss for terminal styling
 - The codebase filters servers from testing themselves: see `filterSelf` in client/client.go and `isSelfHost` in server/server.go
+- `httpServer` in server/server.go is a package-level `fiber.New` singleton, so only one server can run per process. That is why there is no in-process multi-server test; end-to-end mesh testing needs separate processes or containers
 
 ## Helm Deployment
 
-Helm chart located in `helm/hperf/` for Kubernetes deployments. Current version: 5.0.6. Includes StatefulSet, Service, ServiceAccount, and Job templates for bandwidth/latency tests.
+Helm chart located in `helm/hperf/` for Kubernetes deployments. Current version: 5.2.0. Includes StatefulSet, Service, ServiceAccount, and Job templates for bandwidth/latency tests. The chart version must be bumped whenever a template changes, per the note in `Chart.yaml`.