mirror of
https://github.com/tiennm99/noitu.git
synced 2026-10-16 12:15:35 +00:00
tiennm99
8223f40b16
expvar counters for connections, rooms, games, submissions by rejection reason, eliminations, chat, joins and bot moves, served on a separate debug address so they never sit on the public mux, plus one structured word_rejected log line per refused word carrying the normalized word and its link. GET /readyz flips to 503 while draining; SIGTERM stops new rooms, waits up to NOITU_DRAIN_TIMEOUT for live games, then shuts down. GET /version and the startup log carry the build's git describe. Fuzz targets for the frame decoder, the text sanitizer and Vietnamese normalization; the last one found that composing before lowercasing could leave a non-NFC result, now recomposed after lowering. CI runs on dev as well as main, gates gofmt and golangci-lint, tracks the buf major instead of an exact pin, and dependabot watches every ecosystem. The lint findings that had been hidden by the default per-issue cap are fixed.
169 lines
11 KiB
Markdown
169 lines
11 KiB
Markdown
# 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.
|