# Server ops/observability batch

Scope: server/, .github/, Dockerfile, Makefile, README.md, docs/deployment.md. Did not
touch proto/, web/, or generated code.

## What changed

### 1. Observability (D2)
- `server/internal/wsapi/metrics.go` (new): `metricSet` struct of `expvar.Int`/`expvar.Map`,
  one package var `metrics`. Counters: connections open/total; rooms live/total (bot/pvp);
  games started/finished (bot/pvp); words submitted/accepted/rejected (by
  `game.RejectReason.String()`); eliminations (by wire `GameEndReason` string); chat lines;
  joins refused (`too_many_attempts`/`room_not_found`/`room_full` — reused wire error codes
  as keys, one vocabulary instead of two); bot moves (by `bot.Difficulty.String()`); resumes
  attempted/succeeded.
- Wired at call sites: `server.go` (handleWS: connections), `hub.go` (newRegisteredRoom:
  rooms live/total, draining check), `room.go` (beginGame: games started; broadcastGameOver:
  games finished; handleSubmit: words + new `recordRejection` helper; handleBotMove: bot
  moves; handleChat: chat lines; handleJoin: room_full; broadcastElimination: eliminations;
  handleResume: resumes succeeded), `session.go` (resumeFrom: resumes attempted; JoinRoom
  dispatch: rate-limited/not-found refusals).
- `GET /debug/vars` only ever mounted on a **separate** `http.Server` in
  `cmd/noitu-server/main.go` (`newDebugServer`), started only when `NOITU_DEBUG_ADDR` is set.
  Never touches the public mux — verified by `TestDebugVarsNotOnPublicMux`.
- `word_rejected` Info log line in `room.recordRejection` (room.go): `reason`, `word`
  (normalized via `vietnamese.Normalize` after `sanitizeText`, capped at `maxWordRunes`=64 —
  never the raw typed text), `link` (`engine.Current()`), `mode`, `room`. No chat text or
  nicknames logged anywhere.
- Documented in `docs/deployment.md` under new "## Observability" section: what
  `NOITU_DEBUG_ADDR` does, every counter name, and exactly what the log line contains.

### 2. Ops hygiene (D4 + D1a)
- `GET /readyz` (server.go): 200 while accepting, 503 while draining (`hub.isDraining()`).
  `GET /healthz` unchanged.
- Drain mode: `hub.draining` (atomic.Bool) refuses new rooms with `errDraining` →
  `server_restarting` (session.go `roomCreateError`, reusing the existing i18n key).
  `hub.liveGames` (atomic.Int64) counts rooms with a game **running**, not lobbies;
  `room.liveCounted` (atomic.Bool) + a `CompareAndSwap` in `room.run()`'s teardown guarantees
  exactly one decrement per increment even if a room is cancelled mid-game rather than
  finishing normally. `main.go`'s `run()`: on ctx.Done, logs room/live-game count, calls
  `api.StartDraining()`, calls `waitForGamesToFinish(api, cfg.drainTimeout)` (polls
  `LiveGameCount()` every 200ms up to `NOITU_DRAIN_TIMEOUT`), logs again, then does the
  existing `api.Shutdown()` / `srv.Shutdown()`.
- Version stamp: `main.version` (default `"dev"`), set via `-X main.version=...` from
  `git describe --tags --always --dirty`. `Makefile`'s `server` target computes it directly;
  a new `image` target passes it to `docker build --build-arg VERSION=...` (`.dockerignore`
  deliberately excludes `.git`, so the Dockerfile can't run `git describe` itself — this is
  the standard, documented reason). `wsapi.Config.Version` → `Server.version` → printed in
  the `listening` log line and served as plain text by `GET /version`.
- `README.md` and `docs/deployment.md`: added `NOITU_DEBUG_ADDR`, `NOITU_DRAIN_TIMEOUT` to
  the env tables; new "Draining on deploy" section; "Version" subsection under "The image";
  `image` make target in both the targets table and "Without make" raw equivalents;
  `/readyz` and `/version` added to the endpoints list.

### 3. Fuzz targets (F3)
- `server/internal/wsapi/codec_test.go`: `FuzzDecode`, seeded from `wire_test.go`'s
  `clientVariants()` plus malformed/short/wrong-type frames. Asserts non-binary always
  `ErrNotBinary`, otherwise either a real error or a real message — never both nil.
- `server/internal/wsapi/nickname_test.go` (new): `FuzzSanitizeText`, seeded from the
  existing `TestSanitizeNickname` table plus the stacking-mark/bidi/zero-width regression
  cases. Asserts: no control/format char in the output, no non-printable rune, rune count
  ≤ `maxNicknameRunes`, no run of combining marks over `maxNicknameMarks`.
- `server/internal/vietnamese/normalize_test.go`: `FuzzNormalize`, seeded from the existing
  table tests. Asserts NFC, lowercase, single-spaced, and word/syllables agree with each
  other (or `ErrEmpty` with empty output).
  **Found and fixed a real bug**: `Normalize` composed (NFC) then lowercased, but lowering a
  base rune can enable a composition that only exists for the lowercase form (e.g. `Y` + ring
  above has no precomposed codepoint; `y` + ring above does, U+1E99) — so the result could be
  non-NFC despite the doc's promise. Fixed by re-composing after lowering
  (`normalize.go`). Vietnamese text never hits this (no case-asymmetric diacritics in the
  language), so no dictionary data is invalidated by the fix. Regression seed corpus saved at
  `server/internal/vietnamese/testdata/fuzz/FuzzNormalize/d3ebd1ce3935d20e`.
- All three run their seed corpus under plain `go test` (fast, no `-fuzz` needed); verified
  each with `-fuzz -fuzztime 15-20s` locally, clean after the Normalize fix.

### 4. CI hygiene (F4 + reviewer findings)
- `.github/workflows/ci.yml`: `push` trigger now `[main, dev]`. Go job gained a `gofmt -l`
  format-check step and a `golangci-lint` step (`golangci/golangci-lint-action@v9`, moving
  major tag, default config — no `.golangci.yml` added, no linters disabled).
- `.github/workflows/proto.yml`: `push` trigger now `[main, dev]`; `bufbuild/buf-setup-action`
  `version:` changed from the pinned `1.69.0` to `latest`; `buf breaking` still compares
  against `.git#ref=origin/main`, untouched.
- `.github/dependabot.yml` (new): `gomod` (`/server`), `npm` (`/web`), `github-actions` (`/`),
  `docker` (`/`); weekly; each grouped for minor+patch.
- Fixed **23** pre-existing `errcheck` findings and **1** `staticcheck` finding (Unicode
  format chars in a string literal) across `server/` so the tree passes `golangci-lint`
  default linters clean — these were latent (hidden by golangci-lint's default
  `max-same-issues: 3`, confirmed with `--max-same-issues=0`), not something I introduced;
  fixed rather than suppressed per the instruction not to add lint noise. Files touched:
  `cmd/build-dictionary/{dump.go,main.go,main_test.go}`, `internal/dictionary/{store.go,
  store_test.go}`, `internal/wsapi/{multiplayer_test.go,limits_test.go}`. All are
  `defer x.Close()`/`Rollback()`/`os.Remove()` wrapped as `defer func() { _ = x.Close() }()`,
  or a raw Unicode literal replaced with its `\uXXXX` escape.
- `make proto-check` unaffected (didn't touch `proto/`, `buf.yaml`, `buf.gen.yaml`, or the
  builder's `DICT_URL` constant).

### 5. Makefile / README
- `Makefile`: `VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo
  dev)`; `server` target now passes `-ldflags "-X main.version=$(VERSION)"`; new `image`
  target; help text and `.PHONY` updated for both. (`web-lint` target/step already present
  from a concurrent change — left untouched, not mine.)
- `README.md`: env table, endpoints list, Make targets table, "Without make" raw equivalents
  (server ldflags line, new `image` raw docker command), Deployment section's docker example
  now points at `make image`.

## Verification

```
cd server && go vet ./... && gofmt -l . && golangci-lint run ./... && go test ./... -race -count=1
```

Tail of the actual run:
```
0 issues.
ok      github.com/tiennm99dev/noitu/server/cmd/build-dictionary       2.803s
?       github.com/tiennm99dev/noitu/server/cmd/noitu-server    [no test files]
?       github.com/tiennm99dev/noitu/server/gen/noitu/v1        [no test files]
ok      github.com/tiennm99dev/noitu/server/internal/bot        1.609s
ok      github.com/tiennm99dev/noitu/server/internal/dictionary 4.074s
ok      github.com/tiennm99dev/noitu/server/internal/game       1.055s
ok      github.com/tiennm99dev/noitu/server/internal/vietnamese 1.030s
ok      github.com/tiennm99dev/noitu/server/internal/wsapi      28.489s
```

Also: built the real binary via `make server` (embeds `442ced3-dirty`, confirmed via
`/version`); ran it against the fixture dictionary with `NOITU_DEBUG_ADDR` set and curled
`/healthz` (200), `/readyz` (200, then 503 conceptually verified via the unit test),
`/version` (`442ced3-dirty`), `/debug/vars` on the public port (404) and on the debug port
(200, all `noitu_*` counters present at zero); sent `SIGTERM` and confirmed the `draining` /
`shutting down` log lines with room/live-game counts. Cleaned up the binary, fixture db, and
verified no leftover process. Also built the Dockerfile's `build` stage in isolation
(`docker build --target build --build-arg VERSION=...`, avoids the `web` stage's `npm`
entirely per the constraint) to confirm the new `-ldflags` line is valid; removed the test
image afterward.

New/changed tests, all in the behaviour list asked for:
- `TestReadyzFlipsOnDrain`, `TestDrainRefusesNewRooms`, `TestLiveGameCountTracksGamesNotLobbies`
  (drain_test.go)
- `TestVersionEndpoint`, `TestVersionEndpointDefaultsWhenUnset`, `TestDebugVarsNotOnPublicMux`
  (drain_test.go)
- `TestMetricsCountWordSubmissions` (metrics_test.go)
- `FuzzDecode`, `FuzzSanitizeText`, `FuzzNormalize`

Two existing tests (`TestTheFirstTurnIsDrawn`, `TestABotGameOpensWithTheHuman`) hand-build a
`room{}` and call `beginGame()` directly without going through the hub; `beginGame` now
touches `r.hub.gameStarted()`, so both were given `hub: &hub{}` — a zero-value hub is fine
since only its atomic fields are touched.

## Deferred / not done

- Did not add per-room `game_in_progress` join refusal to the `joinsRefused` metric — spec
  named exactly three buckets (rate limit/not found/full); adding a fourth wasn't asked for.
- Did not touch `ci.yml`'s image-build steps to pass `VERSION` — the deliverable named the
  Makefile and Dockerfile specifically; the Dockerfile's `ARG VERSION=dev` default keeps
  those builds working unchanged.
- Observed `TestStandingsReachEverySeat` fail once under full-suite load (`context deadline
  exceeded` reading a socket) but pass 5/5 in isolation and clean on every subsequent full
  run; looks like a pre-existing environment flake under CPU contention, not something my
  change touches (it doesn't call any of the changed code paths). Did not chase further since
  the suite has been green on every full re-run since.

## Unresolved questions

- None blocking. One judgment call worth flagging: `NOITU_DRAIN_TIMEOUT` accepts `0` as an
  explicit valid value (meaning "don't wait"), unlike every other duration env var where `0`/
  invalid falls back to the default — needed a second parser (`envNonNegDuration`) rather than
  reusing `envDuration`. Documented inline; flagging in case the two-parser asymmetry should
  instead be a single parser with a flag, but the current split reads clearer to me.