From 8223f40b16c8d5766b8c2c8cdf060be1584d418e Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 21 Sep 2026 01:17:52 +0700 Subject: [PATCH] feat(server): counters, readiness, drain mode and a version stamp 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. --- .github/dependabot.yml | 37 ++++ .github/workflows/ci.yml | 28 ++- .github/workflows/proto.yml | 7 +- Dockerfile | 9 +- Makefile | 22 ++- README.md | 29 ++- docs/deployment.md | 113 ++++++++++-- ...er-260921-0027-server-ops-observability.md | 168 ++++++++++++++++++ server/cmd/build-dictionary/dump.go | 2 +- server/cmd/build-dictionary/main.go | 18 +- server/cmd/build-dictionary/main_test.go | 8 +- server/cmd/noitu-server/main.go | 110 +++++++++++- server/internal/dictionary/store.go | 10 +- server/internal/dictionary/store_test.go | 4 +- server/internal/vietnamese/normalize.go | 10 +- server/internal/vietnamese/normalize_test.go | 51 ++++++ .../fuzz/FuzzNormalize/d3ebd1ce3935d20e | 2 + server/internal/wsapi/codec_test.go | 44 +++++ server/internal/wsapi/drain_test.go | 154 ++++++++++++++++ server/internal/wsapi/hub.go | 61 ++++++- server/internal/wsapi/limits_test.go | 2 +- server/internal/wsapi/metrics.go | 82 +++++++++ server/internal/wsapi/metrics_test.go | 70 ++++++++ server/internal/wsapi/multiplayer_test.go | 4 +- server/internal/wsapi/nickname_test.go | 64 +++++++ server/internal/wsapi/room.go | 88 ++++++++- server/internal/wsapi/server.go | 58 +++++- server/internal/wsapi/session.go | 9 + server/internal/wsapi/wsapi_test.go | 6 +- 29 files changed, 1204 insertions(+), 66 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 plans/reports/fullstack-developer-260921-0027-server-ops-observability.md create mode 100644 server/internal/vietnamese/testdata/fuzz/FuzzNormalize/d3ebd1ce3935d20e create mode 100644 server/internal/wsapi/codec_test.go create mode 100644 server/internal/wsapi/drain_test.go create mode 100644 server/internal/wsapi/metrics.go create mode 100644 server/internal/wsapi/metrics_test.go create mode 100644 server/internal/wsapi/nickname_test.go diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..d7dafc7 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,37 @@ +# One dependency update ecosystem per manifest the project actually has, so +# an upgrade is proposed rather than left to be noticed by chance. See +# docs/deployment.md and README.md for the "moving tag over exact pin" +# preference this exists to serve: a bot is the mechanism that rule assumes. +version: 2 +updates: + - package-ecosystem: gomod + directory: /server + schedule: + interval: weekly + groups: + go-minor-patch: + update-types: [minor, patch] + + - package-ecosystem: npm + directory: /web + schedule: + interval: weekly + groups: + web-minor-patch: + update-types: [minor, patch] + + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + groups: + actions-minor-patch: + update-types: [minor, patch] + + - package-ecosystem: docker + directory: / + schedule: + interval: weekly + groups: + docker-minor-patch: + update-types: [minor, patch] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a1c7e86..5867455 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,7 @@ name: ci on: push: - branches: [main] + branches: [main, dev] pull_request: release: types: [published] @@ -33,6 +33,28 @@ jobs: run: go vet ./... working-directory: server + # gofmt -l lists files that are not gofmt-clean; -d would only show the + # diff. Non-empty output is the failure signal, so it decides the exit + # code itself rather than leaning on the emptiness of a report nobody + # reads. + - name: Format check + run: | + unformatted="$(gofmt -l .)" + if [ -n "$unformatted" ]; then + echo "not gofmt-clean:" + echo "$unformatted" + exit 1 + fi + working-directory: server + + # Default linters only: this is a signal every PR has to pass, not a + # style debate, so nothing beyond golangci-lint's own defaults is + # enabled here. + - name: Lint + uses: golangci/golangci-lint-action@v9 + with: + working-directory: server + # -race because the whole transport layer is goroutines and timers, and a # data race there is exactly the kind of defect that passes without it. - name: Test @@ -57,6 +79,10 @@ jobs: run: npm run check working-directory: web + - name: Lint + run: npm run lint + working-directory: web + # npm test builds first, so this also proves the bundle compiles and # carries no wordlist. - name: Test diff --git a/.github/workflows/proto.yml b/.github/workflows/proto.yml index c3939ca..3581962 100644 --- a/.github/workflows/proto.yml +++ b/.github/workflows/proto.yml @@ -7,7 +7,7 @@ name: proto on: push: - branches: [main] + branches: [main, dev] pull_request: permissions: @@ -22,9 +22,12 @@ jobs: # buf breaking compares against main, which needs real history. fetch-depth: 0 + # A moving version rather than an exact pin, per house rule: updates + # arrive on the next run, and a breaking major would surface here before + # it could surprise a contributor's own machine. - uses: bufbuild/buf-setup-action@v1 with: - version: 1.69.0 + version: latest - uses: actions/setup-go@v5 with: diff --git a/Dockerfile b/Dockerfile index d7372d4..153b1d7 100644 --- a/Dockerfile +++ b/Dockerfile @@ -18,13 +18,20 @@ RUN npm run build # --- the binary ------------------------------------------------------------- FROM golang:1.25-alpine AS build +# What GET /version answers and the startup log line carries. .dockerignore +# deliberately keeps .git out of the build context — a stale copy should +# never ship — so git describe cannot run in here; a caller that wants a real +# version passes it in, the way `make image` does. Unset, this defaults to +# "dev", which is honest about an unstamped build. +ARG VERSION=dev + WORKDIR /src/server COPY server/go.mod server/go.sum ./ RUN go mod download COPY server/ ./ # CGO_ENABLED=0 is what makes a distroless static image possible, and it works # because the SQLite driver is pure Go. -RUN CGO_ENABLED=0 go build -trimpath -ldflags='-s -w' -o /out/noitu-server ./cmd/noitu-server +RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w -X main.version=${VERSION}" -o /out/noitu-server ./cmd/noitu-server RUN CGO_ENABLED=0 go build -trimpath -o /out/build-dictionary ./cmd/build-dictionary # --- the dictionary --------------------------------------------------------- diff --git a/Makefile b/Makefile index 43de5fc..7414fb4 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,12 @@ FIXTURE_WORDS := testdata/fixture-words.txt FIXTURE_DB := data/fixture.db SERVER_BIN := noitu-server -.PHONY: help fetch-dict dict fixture-dict proto proto-check server web web-dev test test-go test-web test-e2e run clean +# What GET /version answers and what the startup log line carries. "dev" when +# there is no tag and no git history at all (a shallow clone, an extracted +# tarball) — the same fallback -ldflags leaves unstamped Go code with anyway. +VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) + +.PHONY: help fetch-dict dict fixture-dict proto proto-check server image web web-dev web-lint test test-go test-web test-e2e run clean help: @echo "fetch-dict download the current Wiktionary tiếng Việt dump (~61 MB) into data/" @@ -22,9 +27,11 @@ help: @echo "fixture-dict build the small test dictionary — no download needed" @echo "proto regenerate the Go and JS wire types from proto/" @echo "proto-check lint the schema and verify the committed output is in sync" - @echo "server build the Go server binary" + @echo "server build the Go server binary, stamped with VERSION" + @echo "image build the container image, stamped with VERSION" @echo "web build the SvelteKit frontend" @echo "web-dev run the frontend dev server, proxying /ws to a local server" + @echo "web-lint lint the frontend" @echo "test run all tests" @echo "test-e2e run the Playwright suite against the fixture dictionary" @echo "run build and run the server locally" @@ -78,7 +85,13 @@ web/node_modules: web/package.json web/package-lock.json @touch web/node_modules server: - cd server && CGO_ENABLED=0 go build -o ../$(SERVER_BIN) ./cmd/noitu-server + cd server && CGO_ENABLED=0 go build -ldflags "-X main.version=$(VERSION)" -o ../$(SERVER_BIN) ./cmd/noitu-server + +# The Dockerfile cannot run git describe itself — .dockerignore deliberately +# keeps .git out of the build context, so a stale copy never ships — so this +# is the one place VERSION reaches it, as a build-arg. +image: + docker build --build-arg VERSION=$(VERSION) -t noitu:$(VERSION) -t noitu:latest . web: web/node_modules cd web && npm run build @@ -89,6 +102,9 @@ web: web/node_modules web-dev: web/node_modules cd web && npm run dev +web-lint: web/node_modules + cd web && npm run lint + test: test-go test-web test-go: diff --git a/README.md b/README.md index 93371d2..8e4496d 100644 --- a/README.md +++ b/README.md @@ -151,6 +151,11 @@ Every Vietnamese string lives in `web/src/lib/i18n/vi.js`, including the map fro `room_not_found` and never prose. A test walks the generated enums and fails when a value has no message, so a schema change cannot quietly ship an untranslated screen. +The rules — the chain, the clock, a dead end, elimination, scoring, room codes and reconnect +grace — are written out once on `/rules`, a single anchored page linked from the landing +screen and from a small "Luật chơi" link beside the connection badge in the game header and +the lobby. It is not a button: reading the rules is not a way to start playing. + The countdown is drawn against the server's clock, estimated from the `Ping`/`Pong` round trip, and settles 300ms early so the ring never claims more time than the server allows. @@ -192,13 +197,17 @@ Configuration is environment-only; every variable has a working default. | `NOITU_TRUSTED_PROXIES` | *(unset)* | Comma-separated proxy addresses or CIDRs whose `X-Forwarded-For` is believed. Unset keys limiters on the socket peer | | `NOITU_MAX_ROOMS` | `1000` | Ceiling on live rooms across the process; a creator past it is told `server_full` | | `NOITU_MAX_CONNECTIONS` | `2000` | Ceiling on open WebSockets; the next upgrade gets HTTP 503 | +| `NOITU_DEBUG_ADDR` | *(unset)* | A separate listen address for `GET /debug/vars` (expvar). Unset means the operational counters are not exposed anywhere | +| `NOITU_DRAIN_TIMEOUT` | `0s` | How long a shutdown waits for live *games* (not lobbies) to finish before ending them anyway. `0s` is today's behaviour: end them immediately | An invalid duration or count is logged and ignored rather than silently changing the rules of the game. -Endpoints: `GET /ws` (Protobuf over binary WebSocket frames), `GET /healthz`, -and — when `NOITU_WEB_DIR` is set — the frontend on everything else, with -unknown paths falling back to `index.html` because deep links are client routes. +Endpoints: `GET /ws` (Protobuf over binary WebSocket frames), `GET /healthz` +(liveness), `GET /readyz` (readiness — 503 while draining), `GET /version` +(plain text), and — when `NOITU_WEB_DIR` is set — the frontend on everything +else, with unknown paths falling back to `index.html` because deep links are +client routes. ### Smoke-testing without a frontend @@ -227,9 +236,11 @@ dev-only URL to get wrong. |---|---| | `fetch-dict` | Download the current upstream Wiktionary export (~62 MB) into `data/` | | `dict` | Derive `data/noitu.db` from the upstream export | -| `server` | Build the Go server binary | +| `server` | Build the Go server binary, stamped with the version `git describe` reports | +| `image` | Build the container image, stamped the same way | | `web` | Build the SvelteKit frontend to static assets | | `web-dev` | Run the frontend dev server, proxying `/ws` to a local server | +| `web-lint` | Lint the frontend | | `proto` | Regenerate the Go and JS wire types from `proto/` (needs `buf`) | | `proto-check` | Lint the schema and verify the committed generated code is in sync | | `fixture-dict` | Build the small test dictionary, no download needed | @@ -254,11 +265,17 @@ cd server && go run ./cmd/build-dictionary --dump ../data/viwiktionary-latest-pa cd server && go vet ./... && go test ./... -race # server -cd server && CGO_ENABLED=0 go build -o ../noitu-server ./cmd/noitu-server +cd server && CGO_ENABLED=0 go build -ldflags "-X main.version=$(git describe --tags --always --dirty)" -o ../noitu-server ./cmd/noitu-server + +# image +docker build --build-arg VERSION="$(git describe --tags --always --dirty)" -t noitu:latest . # web cd web && npm ci && npm run build +# web-lint +cd web && npm run lint + # test-web (npm test builds first, then checks the bundle carries no wordlist) cd web && npm run check && npm test @@ -292,7 +309,7 @@ dictionary. See [`docs/deployment.md`](./docs/deployment.md) for configuration, reverse-proxy requirements, and what a restart costs. ```sh -docker build -t noitu:latest . +make image # stamps the image with `git describe`; see docs/deployment.md docker run -p 8080:8080 noitu:latest ``` diff --git a/docs/deployment.md b/docs/deployment.md index c474edd..a11ef0a 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -1,9 +1,9 @@ # Deployment The whole game is one binary. It serves the WebSocket API, the built frontend, -and a health check, and it reads a single database file at startup. The -supported shape is the container image behind a reverse proxy that terminates -TLS. +and its own health, readiness and version endpoints, and it reads a single +database file at startup. The supported shape is the container image behind a +reverse proxy that terminates TLS. ## Configuration @@ -21,6 +21,8 @@ so the image runs with nothing set. | `NOITU_TRUSTED_PROXIES` | *(unset)* | Comma-separated proxy addresses or CIDRs whose `X-Forwarded-For` is believed. Unset keys limiters on the socket peer | | `NOITU_MAX_ROOMS` | `1000` | Ceiling on live rooms across the process; a creator past it is told `server_full` | | `NOITU_MAX_CONNECTIONS` | `2000` | Ceiling on open WebSockets; the next upgrade gets HTTP 503 | +| `NOITU_DEBUG_ADDR` | *(unset)* | Separate listen address for `GET /debug/vars` (expvar counters). Unset means the counters exist in the process but nothing serves them | +| `NOITU_DRAIN_TIMEOUT` | `0s` | How long a shutdown waits for live games to finish before ending them anyway; see "Draining on deploy" below | An invalid duration or count is logged and ignored rather than silently changing the rules of the game. @@ -50,10 +52,13 @@ permissive direction lets any page open a socket as one of your players. ## The image ```sh -docker build -t noitu:latest . +docker build --build-arg VERSION="$(git describe --tags --always --dirty)" -t noitu:latest . docker run -p 8080:8080 noitu:latest ``` +`make image` runs the same build with `VERSION` filled in for you; see +"Version" below. + The build downloads the ~62 MB upstream Wiktionary export in a builder stage and derives the ~2 MB database the game uses. Only the derived file is copied into the final image, so the upstream export never ships. The result is a @@ -63,6 +68,20 @@ Passing `--build-arg FIXTURE_DICT=1` builds the same image against the checked-in word sample instead. It produces a playable but tiny dictionary and exists so the image can be tested without the download; do not ship it. +### Version + +`GET /version` answers with the build's version, and the same string opens +the startup log line — the fastest way to confirm a deploy actually replaced +the running process rather than restarted the old one. It comes from +`-X main.version=...` at link time, populated from `git describe --tags +--always --dirty`. `make server` runs that command directly; the Dockerfile +cannot — `.dockerignore` deliberately keeps `.git` out of the build context, +so a stale copy never ships in the image — so it takes the version as the +`VERSION` build-arg instead, which `make image` supplies. Building the image +directly with `docker build .` and no `--build-arg VERSION=...` reports +`dev`, which is an honest answer for an unstamped build rather than a wrong +one. + ### What travels with the data The derived wordlist is CC BY-SA 4.0 while the code is Apache-2.0, so the image @@ -144,25 +163,91 @@ client past it is disconnected rather than throttled. The defaults are generous for one binary on a small host; lower them if memory is tight, because a room is a goroutine and an engine held for up to its idle window. -## Health check +## Observability -`GET /healthz` returns 200 once the dictionary has loaded. It does not report -on live games, so it is a liveness check rather than a readiness one. +Set `NOITU_DEBUG_ADDR` to a second listen address — one that is not the one +players reach — to expose `GET /debug/vars` there: standard-library +[`expvar`](https://pkg.go.dev/expvar), zero extra dependencies, a JSON object +of process counters refreshed on every write. It is never mounted on the +public address, unset or not, so leaving `NOITU_DEBUG_ADDR` unset is the same +as not having it. The counters, all prefixed `noitu_`: connections open and +total; rooms live and total, each split `bot`/`pvp`; games started and +finished the same way; words submitted, accepted, and rejected by reason; +eliminations by reason; chat lines; join attempts refused, by whether it was +the rate limit, an unknown code, or a full room; bot moves by difficulty; and +resumes attempted versus succeeded. None of it is read by the game itself — +it is a second write next to a decision already made, not an input to one. + +Every rejected word also gets one structured log line at `Info`, +`word_rejected`, carrying `reason`, `word`, `link` (the syllable it had to +start with), `mode` (`bot`/`pvp`) and `room`. `word` is never the raw text a +player typed — it is normalized the same way the engine matches it (NFC, +lowercase, single-spaced) and capped at 64 runes — so the line is safe to +collect and is exactly the corpus-review question this project has open: +which words players type that the dictionary does not have. Nothing else a +player types is logged: not chat, not a nickname, not an accepted word. + +## Health and readiness + +`GET /healthz` returns 200 once the dictionary has loaded, for the rest of the +process's life. It does not report on live games, so it is a liveness check +rather than a readiness one, and it never moves — a proxy or orchestrator +using it to decide whether to kill the process must not see it fail during a +drain, because the process is still correctly finishing the games it has. ```sh curl -fsS https://noitu.example/healthz ``` -A post-deploy check worth having is a real socket open, because the health -check passes whether or not the proxy forwards upgrades. Opening the site and -starting a game against the bot is the shortest version of that. +`GET /readyz` is the readiness check: 200 while the server is accepting new +rooms, 503 once it has started draining (see below). Point a load balancer's +"stop sending me new traffic" check here and its "restart me" check at +`/healthz`; pointing both at the same endpoint defeats the reason there are +two. + +A post-deploy check worth having beyond either is a real socket open, because +both health checks pass whether or not the proxy forwards upgrades. Opening +the site and starting a game against the bot is the shortest version of that. + +## Draining on deploy + +Rooms are in memory, so a restart has always ended every live game — but a +plain `kill` used to do that the instant the signal arrived, which is why +"deploy when the game is quiet" was the only advice this document had. +`SIGTERM` now runs a short sequence first: + +1. The server stops accepting new rooms. A creator past this point is told + `server_restarting` — the same UI key a live shutdown sends everyone else — + rather than `server_full`, because unlike a full room this one is never + coming back. +2. `GET /readyz` flips to 503, so a load balancer that checks it stops routing + new players here. +3. The server waits up to `NOITU_DRAIN_TIMEOUT` for every room with a game + *actually running* to finish on its own turn clock. A room sitting in its + lobby does not count — it has no game a restart would cost, and waiting for + one would make every deploy sit out somebody's abandoned tab. +4. Once every game has finished, or the timeout passes, every player still + connected is told the server is restarting and the process shuts down as + it always did. + +Each step logs the room and live-game count, so "did the deploy actually +wait, and for what" is answered from the log rather than guessed at. + +The default, `NOITU_DRAIN_TIMEOUT=0s`, is today's behaviour: nothing waits, +every live game ends immediately. Setting it to something like `60s` turns +"deploy when the game is quiet" into "deploy whenever, and the games in +flight get up to a minute to finish before they are cut off anyway" — the +turn clock already bounds how long any one game can take, so a timeout a +little over `NOITU_TURN_LIMIT` covers the common case of a handful of games +mid-turn. ## What a restart costs -Rooms are in memory. A restart ends every live game, and players are told the -server is restarting rather than being left waiting. Deploy when the game is -quiet, or accept that the games in flight are lost — there is no session -persistence, by design, in this version. +A restart with `NOITU_DRAIN_TIMEOUT` unset, or a signal harder than `SIGTERM`, +ends every live game immediately and tells players the server is restarting +rather than leaving them waiting. There is no session persistence, by design, +in this version — a game that does not finish inside the drain window is +simply lost. ## Updating the dictionary diff --git a/plans/reports/fullstack-developer-260921-0027-server-ops-observability.md b/plans/reports/fullstack-developer-260921-0027-server-ops-observability.md new file mode 100644 index 0000000..9aa4b17 --- /dev/null +++ b/plans/reports/fullstack-developer-260921-0027-server-ops-observability.md @@ -0,0 +1,168 @@ +# 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. diff --git a/server/cmd/build-dictionary/dump.go b/server/cmd/build-dictionary/dump.go index 12bc6f3..d60150a 100644 --- a/server/cmd/build-dictionary/dump.go +++ b/server/cmd/build-dictionary/dump.go @@ -76,7 +76,7 @@ func readDump(path string) (map[string]entry, map[string][]sense, map[rejectReas if err != nil { return nil, nil, nil, stats, prov, fmt.Errorf("read dump: %w", err) } - defer f.Close() + defer func() { _ = f.Close() }() info, err := f.Stat() if err != nil { // A provenance row must be right or absent, never a plausible zero. diff --git a/server/cmd/build-dictionary/main.go b/server/cmd/build-dictionary/main.go index dcd6f77..cd9183d 100644 --- a/server/cmd/build-dictionary/main.go +++ b/server/cmd/build-dictionary/main.go @@ -230,7 +230,7 @@ func verify(path string, minWords int, requireCoverage bool) error { if err != nil { return err } - defer db.Close() + defer func() { _ = db.Close() }() checks := []struct { desc string @@ -369,7 +369,7 @@ func write(path string, words map[string]entry, meanings map[string][]sense, ali committed := false defer func() { if !committed { - os.Remove(tmp) + _ = os.Remove(tmp) } }() @@ -399,7 +399,7 @@ func writeTo(path string, words map[string]entry, meanings map[string][]sense, a if err != nil { return fmt.Errorf("create output: %w", err) } - defer db.Close() + defer func() { _ = db.Close() }() schema := ` CREATE TABLE words ( @@ -441,13 +441,13 @@ CREATE TABLE meta (key TEXT PRIMARY KEY, value TEXT NOT NULL); if err != nil { return err } - defer tx.Rollback() + defer func() { _ = tx.Rollback() }() insertWord, err := tx.Prepare(`INSERT INTO words (word, first, last, syllables) VALUES (?, ?, ?, ?)`) if err != nil { return err } - defer insertWord.Close() + defer func() { _ = insertWord.Close() }() outDegree := make(map[string]int) for _, e := range words { @@ -466,7 +466,7 @@ CREATE TABLE meta (key TEXT PRIMARY KEY, value TEXT NOT NULL); if err != nil { return err } - defer insertSyllable.Close() + defer func() { _ = insertSyllable.Close() }() for syllable, degree := range outDegree { if _, err := insertSyllable.Exec(syllable, degree); err != nil { return fmt.Errorf("insert syllable %q: %w", syllable, err) @@ -477,7 +477,7 @@ CREATE TABLE meta (key TEXT PRIMARY KEY, value TEXT NOT NULL); if err != nil { return err } - defer insertAlias.Close() + defer func() { _ = insertAlias.Close() }() for variant, canonical := range aliases { if _, err := insertAlias.Exec(variant, canonical); err != nil { return fmt.Errorf("insert alias %q: %w", variant, err) @@ -488,7 +488,7 @@ CREATE TABLE meta (key TEXT PRIMARY KEY, value TEXT NOT NULL); if err != nil { return err } - defer insertMeaning.Close() + defer func() { _ = insertMeaning.Close() }() meaningCount := 0 for word, senses := range meanings { if _, isWord := words[word]; !isWord { @@ -509,7 +509,7 @@ CREATE TABLE meta (key TEXT PRIMARY KEY, value TEXT NOT NULL); if err != nil { return err } - defer insertMeta.Close() + defer func() { _ = insertMeta.Close() }() meta := [][2]string{ {"source_url", src.url}, {"source_license", src.license}, diff --git a/server/cmd/build-dictionary/main_test.go b/server/cmd/build-dictionary/main_test.go index 918ecfc..85ab8ee 100644 --- a/server/cmd/build-dictionary/main_test.go +++ b/server/cmd/build-dictionary/main_test.go @@ -38,7 +38,7 @@ func openOut(t *testing.T, path string) *sql.DB { if err != nil { t.Fatal(err) } - t.Cleanup(func() { db.Close() }) + t.Cleanup(func() { _ = db.Close() }) return db } @@ -123,7 +123,7 @@ func TestBuildWritesMeanings(t *testing.T) { if err != nil { t.Fatal(err) } - defer rows.Close() + defer func() { _ = rows.Close() }() var got []sense for rows.Next() { var ord int @@ -269,7 +269,7 @@ func TestWordListCarriesTabSeparatedMeanings(t *testing.T) { if err != nil { t.Fatal(err) } - defer rows.Close() + defer func() { _ = rows.Close() }() type row struct { word string ord int @@ -326,7 +326,7 @@ func brokenDB(t *testing.T, extraSQL string) string { if err != nil { t.Fatal(err) } - defer db.Close() + defer func() { _ = db.Close() }() if _, err := db.Exec(extraSQL); err != nil { t.Fatal(err) } diff --git a/server/cmd/noitu-server/main.go b/server/cmd/noitu-server/main.go index 9b9fac0..196c89a 100644 --- a/server/cmd/noitu-server/main.go +++ b/server/cmd/noitu-server/main.go @@ -5,6 +5,7 @@ package main import ( "context" "errors" + "expvar" "log/slog" "net/http" "os" @@ -28,8 +29,20 @@ const ( // arrives. Rooms are told separately and immediately, so this only covers // the HTTP side. shutdownGrace = 10 * time.Second + + // drainPollInterval is how often waitForGamesToFinish checks whether the + // last live game has ended. Short enough that a drain does not overrun its + // timeout by more than a blink, cheap enough to poll at all — the + // alternative is a channel the hub would need to fan out to every room. + drainPollInterval = 200 * time.Millisecond ) +// version is the build the process is running. The default here is what +// `go run` or a build with no -ldflags reports; a real build sets it with +// -X main.version=$(git describe --tags --always --dirty), from the Makefile +// server target or the Dockerfile's VERSION build-arg. +var version = "dev" + type config struct { addr string dbPath string @@ -40,6 +53,8 @@ type config struct { trustedProxies []string maxRooms int maxConnections int + debugAddr string + drainTimeout time.Duration } func main() { @@ -81,6 +96,7 @@ func run() error { TrustedProxies: cfg.trustedProxies, MaxRooms: cfg.maxRooms, MaxConnections: cfg.maxConnections, + Version: version, }) srv := &http.Server{ @@ -89,13 +105,23 @@ func run() error { ReadHeaderTimeout: 10 * time.Second, } + debugSrv := newDebugServer(cfg.debugAddr) + errc := make(chan error, 1) go func() { - slog.Info("listening", "addr", cfg.addr, "turn_limit", cfg.turnLimit, "web_dir", cfg.webDir) + slog.Info("listening", "addr", cfg.addr, "turn_limit", cfg.turnLimit, "web_dir", cfg.webDir, "version", version) if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { errc <- err } }() + if debugSrv != nil { + go func() { + slog.Info("debug endpoint listening", "addr", cfg.debugAddr) + if err := debugSrv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { + slog.Error("debug endpoint exited", "err", err) + } + }() + } select { case err := <-errc: @@ -103,16 +129,74 @@ func run() error { case <-ctx.Done(): } + // Draining is the first half of shutdown: stop seating new rooms and flip + // /readyz unhealthy, so a load balancer stops sending this instance new + // traffic while the games it already has finish on their own turn clock. + // A creator refused during this window gets server_restarting rather than + // server_full — the room is not coming back, unlike a full one. + slog.Info("draining", "rooms", api.RoomCount(), "live_games", api.LiveGameCount()) + api.StartDraining() + waitForGamesToFinish(api, cfg.drainTimeout) + // Tell players why before the sockets go, rather than dropping them and // leaving the UI to guess. - slog.Info("shutting down") + slog.Info("shutting down", "rooms", api.RoomCount(), "live_games", api.LiveGameCount()) api.Shutdown() + if debugSrv != nil { + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownGrace) + _ = debugSrv.Shutdown(shutdownCtx) + cancel() + } + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownGrace) defer cancel() return srv.Shutdown(shutdownCtx) } +// newDebugServer builds the expvar listener, or nil when NOITU_DEBUG_ADDR is +// unset. It is a separate *http.Server on its own address rather than a route +// on the public mux: /debug/vars is an operator surface, and the two must not +// be reachable from the same port a player's browser talks to. +func newDebugServer(addr string) *http.Server { + if addr == "" { + return nil + } + mux := http.NewServeMux() + mux.Handle("/debug/vars", expvar.Handler()) + return &http.Server{Addr: addr, Handler: mux, ReadHeaderTimeout: 10 * time.Second} +} + +// waitForGamesToFinish blocks until every room's game has ended or timeout +// passes, whichever is first. timeout <= 0 returns immediately, which is +// today's behaviour: rooms are told the server is restarting and torn down +// with whatever they were doing. +// +// Only games count, not lobbies: a room nobody has started a game in has +// nothing a restart costs, and waiting for it would make every deploy sit out +// somebody's abandoned tab for the full timeout. +func waitForGamesToFinish(api *wsapi.Server, timeout time.Duration) { + if timeout <= 0 { + return + } + deadline := time.NewTimer(timeout) + defer deadline.Stop() + ticker := time.NewTicker(drainPollInterval) + defer ticker.Stop() + + for { + if api.LiveGameCount() == 0 { + return + } + select { + case <-deadline.C: + slog.Info("drain timed out with games still running", "live_games", api.LiveGameCount()) + return + case <-ticker.C: + } + } +} + func loadConfig() config { return config{ addr: env("NOITU_ADDR", defaultAddr), @@ -124,6 +208,8 @@ func loadConfig() config { trustedProxies: envList("NOITU_TRUSTED_PROXIES"), maxRooms: envInt("NOITU_MAX_ROOMS", 0), maxConnections: envInt("NOITU_MAX_CONNECTIONS", 0), + debugAddr: env("NOITU_DEBUG_ADDR", ""), + drainTimeout: envNonNegDuration("NOITU_DRAIN_TIMEOUT", 0), } } @@ -150,7 +236,9 @@ func env(key, fallback string) string { } // envDuration falls back loudly. A typo in a timing variable would otherwise -// silently change the rules of the game. +// silently change the rules of the game. Zero is rejected along with anything +// unparseable, which is right for every duration except the drain timeout — +// see envNonNegDuration. func envDuration(key string, fallback time.Duration) time.Duration { raw := strings.TrimSpace(os.Getenv(key)) if raw == "" { @@ -164,6 +252,22 @@ func envDuration(key string, fallback time.Duration) time.Duration { return d } +// envNonNegDuration is envDuration with zero accepted as a real value rather +// than a trigger for the fallback: NOITU_DRAIN_TIMEOUT=0 means "do not wait", +// which is a deliberate choice an operator can make explicitly, not a typo. +func envNonNegDuration(key string, fallback time.Duration) time.Duration { + raw := strings.TrimSpace(os.Getenv(key)) + if raw == "" { + return fallback + } + d, err := time.ParseDuration(raw) + if err != nil || d < 0 { + slog.Warn("ignoring invalid duration", "key", key, "value", raw, "using", fallback) + return fallback + } + return d +} + // envList returns nil for an unset variable, which coder/websocket reads as // same-origin only. func envList(key string) []string { diff --git a/server/internal/dictionary/store.go b/server/internal/dictionary/store.go index 8c44b30..f5e6220 100644 --- a/server/internal/dictionary/store.go +++ b/server/internal/dictionary/store.go @@ -94,7 +94,7 @@ func Open(path string) (*Store, error) { if err != nil { return nil, fmt.Errorf("open dictionary: %w", err) } - defer db.Close() + defer func() { _ = db.Close() }() if err := db.Ping(); err != nil { return nil, fmt.Errorf("open dictionary: %w", err) @@ -193,7 +193,7 @@ func (s *Store) loadSyllables(db *sql.DB) error { if err != nil { return fmt.Errorf("load syllables: %w", err) } - defer rows.Close() + defer func() { _ = rows.Close() }() for rows.Next() { var syllable string @@ -214,7 +214,7 @@ func (s *Store) loadWords(db *sql.DB) error { if err != nil { return fmt.Errorf("load words: %w", err) } - defer rows.Close() + defer func() { _ = rows.Close() }() for rows.Next() { var word, first, last string @@ -236,7 +236,7 @@ func (s *Store) loadAliases(db *sql.DB) error { if err != nil { return fmt.Errorf("load aliases: %w", err) } - defer rows.Close() + defer func() { _ = rows.Close() }() for rows.Next() { var variant, canonical string @@ -256,7 +256,7 @@ func (s *Store) loadMeanings(db *sql.DB) error { if err != nil { return fmt.Errorf("load meanings: %w", err) } - defer rows.Close() + defer func() { _ = rows.Close() }() for rows.Next() { var word string diff --git a/server/internal/dictionary/store_test.go b/server/internal/dictionary/store_test.go index b570147..0a695b5 100644 --- a/server/internal/dictionary/store_test.go +++ b/server/internal/dictionary/store_test.go @@ -38,7 +38,7 @@ func fixtureAt(tb testing.TB, dir string) string { if err != nil { tb.Fatal(err) } - defer db.Close() + defer func() { _ = db.Close() }() data := fixtureSchema + ` INSERT INTO meta VALUES ('source_license','CC BY-SA 4.0'),('word_count','7'),('meaning_count','3'); @@ -87,7 +87,7 @@ func writeDB(tb testing.TB, sqlText string) string { if err != nil { tb.Fatal(err) } - defer db.Close() + defer func() { _ = db.Close() }() if _, err := db.Exec(sqlText); err != nil { tb.Fatal(err) } diff --git a/server/internal/vietnamese/normalize.go b/server/internal/vietnamese/normalize.go index 708aea5..a2e9e4a 100644 --- a/server/internal/vietnamese/normalize.go +++ b/server/internal/vietnamese/normalize.go @@ -36,7 +36,15 @@ var ErrEmpty = errors.New("vietnamese: empty input") // non-breaking spaces that IMEs and copy-paste routinely introduce. func Normalize(raw string) (string, []string, error) { composed := norm.NFC.String(raw) - lowered := strings.ToLower(composed) + // Composed a second time after lowering: case-folding a base rune can + // enable a composition that only exists for its lowercase form — "Y" plus + // a combining ring above has no precomposed codepoint, but "y" plus the + // same ring does (U+1E99) — so lowering the already-composed string can + // hand back something that is no longer NFC. Fuzzing found this on + // synthetic input; Vietnamese text never hits it (the language has no such + // case-asymmetric diacritic), but the guarantee is meant to hold for + // whatever a player actually types. + lowered := norm.NFC.String(strings.ToLower(composed)) // strings.Fields splits on every unicode.IsSpace rune, which covers tabs and // U+00A0 non-breaking spaces as well as ordinary spaces. diff --git a/server/internal/vietnamese/normalize_test.go b/server/internal/vietnamese/normalize_test.go index 0addc03..6d85294 100644 --- a/server/internal/vietnamese/normalize_test.go +++ b/server/internal/vietnamese/normalize_test.go @@ -2,7 +2,10 @@ package vietnamese import ( "errors" + "strings" "testing" + + "golang.org/x/text/unicode/norm" ) func TestNormalize(t *testing.T) { @@ -71,6 +74,54 @@ func TestNormalizeEmpty(t *testing.T) { } } +// FuzzNormalize guards the one function two processes have to agree on +// byte-for-byte: the corpus builder normalizes with it at build time, the +// server normalizes with it on every submission, and a divergence between +// them would silently unmatch a word the dictionary actually has. The +// invariants are exactly what the package doc promises — NFC, lowercase, +// single-spaced — plus no panic on arbitrary input, and that the returned +// word and syllables always agree with each other. +func FuzzNormalize(f *testing.F) { + seeds := []string{ + "pháp luật", "PHÁP LUẬT", "Pháp Luật", " pháp luật ", "pháp luật", + "pháp\tluật", "pháp luật", "pháp\nluật", "vô tuyến điện", + "công nghiệp hóa hiện", "pháp", "", " ", "\t\n", " ", + "ngôn ngữ", "ngôn ngữ", + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, raw string) { + word, syllables, err := Normalize(raw) + if err != nil { + if !errors.Is(err, ErrEmpty) { + t.Fatalf("Normalize(%q) returned an unexpected error: %v", raw, err) + } + if word != "" || syllables != nil { + t.Fatalf("Normalize(%q) returned ErrEmpty but non-empty output: %q %v", raw, word, syllables) + } + return + } + + if !norm.NFC.IsNormalString(word) { + t.Fatalf("Normalize(%q) = %q is not NFC", raw, word) + } + if strings.ToLower(word) != word { + t.Fatalf("Normalize(%q) = %q is not fully lowercase", raw, word) + } + if strings.Join(strings.Fields(word), " ") != word { + t.Fatalf("Normalize(%q) = %q is not single-spaced and trimmed", raw, word) + } + if len(syllables) == 0 { + t.Fatalf("Normalize(%q) returned no error but no syllables", raw) + } + if strings.Join(syllables, " ") != word { + t.Fatalf("Normalize(%q): word %q does not match its own syllables %v", raw, word, syllables) + } + }) +} + func TestHasEnoughSyllables(t *testing.T) { tests := []struct { syllables []string diff --git a/server/internal/vietnamese/testdata/fuzz/FuzzNormalize/d3ebd1ce3935d20e b/server/internal/vietnamese/testdata/fuzz/FuzzNormalize/d3ebd1ce3935d20e new file mode 100644 index 0000000..988d8b7 --- /dev/null +++ b/server/internal/vietnamese/testdata/fuzz/FuzzNormalize/d3ebd1ce3935d20e @@ -0,0 +1,2 @@ +go test fuzz v1 +string("00000000Y̊") diff --git a/server/internal/wsapi/codec_test.go b/server/internal/wsapi/codec_test.go new file mode 100644 index 0000000..0b5ea10 --- /dev/null +++ b/server/internal/wsapi/codec_test.go @@ -0,0 +1,44 @@ +package wsapi + +import ( + "errors" + "testing" + + "github.com/coder/websocket" + "google.golang.org/protobuf/proto" +) + +// FuzzDecode guards the one function that turns arbitrary bytes off the wire +// into a message the rest of the server trusts. The invariant Decode's own +// comment promises is narrower than "never panics" — a non-binary frame is +// always ErrNotBinary, and anything else either fails cleanly or comes back +// as a real message — but a panic here would take the reader goroutine down +// with a client that sent nothing but noise, so the fuzzer is left free to +// find one if it can. +func FuzzDecode(f *testing.F) { + for _, m := range clientVariants() { + raw, err := proto.Marshal(m) + if err != nil { + f.Fatalf("seed marshal: %v", err) + } + f.Add(int(websocket.MessageBinary), raw) + } + f.Add(int(websocket.MessageText), []byte("not protobuf")) + f.Add(int(websocket.MessageBinary), []byte{}) + f.Add(int(websocket.MessageBinary), []byte{0xff, 0xff, 0xff, 0x01}) + f.Add(0, []byte{0x01, 0x02}) + + f.Fuzz(func(t *testing.T, typ int, raw []byte) { + msg, err := Decode(websocket.MessageType(typ), raw) + + if websocket.MessageType(typ) != websocket.MessageBinary { + if !errors.Is(err, ErrNotBinary) { + t.Fatalf("Decode(typ=%d, ...) err = %v, want ErrNotBinary", typ, err) + } + return + } + if err == nil && msg == nil { + t.Fatalf("Decode(%q) returned neither an error nor a message", raw) + } + }) +} diff --git a/server/internal/wsapi/drain_test.go b/server/internal/wsapi/drain_test.go new file mode 100644 index 0000000..240fe21 --- /dev/null +++ b/server/internal/wsapi/drain_test.go @@ -0,0 +1,154 @@ +package wsapi + +import ( + "io" + "net/http" + "testing" + "time" + + noituv1 "github.com/tiennm99dev/noitu/server/gen/noitu/v1" +) + +// TestReadyzFlipsOnDrain: /healthz is liveness and never moves; /readyz is +// what a load balancer should stop trusting once the process has started +// shutting down. +func TestReadyzFlipsOnDrain(t *testing.T) { + api, url := newTestServer(t, chainDict(), Config{}) + httpURL := "http" + url[len("ws"):] + + resp, err := http.Get(httpURL + "/readyz") + if err != nil { + t.Fatalf("GET /readyz: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("before draining: status = %d, want 200", resp.StatusCode) + } + + api.StartDraining() + + resp, err = http.Get(httpURL + "/readyz") + if err != nil { + t.Fatalf("GET /readyz: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusServiceUnavailable { + t.Errorf("while draining: status = %d, want 503", resp.StatusCode) + } + + resp, err = http.Get(httpURL + "/healthz") + if err != nil { + t.Fatalf("GET /healthz: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("healthz while draining: status = %d, want 200 (liveness must not move)", resp.StatusCode) + } +} + +// TestDrainRefusesNewRooms: a creator past the drain point is told the server +// is restarting, not that it is full — a full room might free up, a draining +// one never will. +func TestDrainRefusesNewRooms(t *testing.T) { + api, url := newTestServer(t, chainDict(), Config{}) + api.StartDraining() + + c := dial(t, url) + c.hello("Người thử") + c.send(&noituv1.ClientMessage{Payload: &noituv1.ClientMessage_CreateRoom{CreateRoom: &noituv1.CreateRoom{}}}) + + if got := c.await("error").GetError().GetCode(); got != "server_restarting" { + t.Errorf("error code = %q, want server_restarting", got) + } +} + +// TestLiveGameCountTracksGamesNotLobbies is the invariant a drain waits on: +// an empty room, or one sitting in its lobby, costs a restart nothing, and +// must not be counted the way a running game is. +func TestLiveGameCountTracksGamesNotLobbies(t *testing.T) { + api, url := newTestServer(t, chainDict(), Config{TurnLimit: 200 * time.Millisecond}) + + host, guest, _ := pvpLobby(t, url) + if got := api.LiveGameCount(); got != 0 { + t.Fatalf("a lobby with nobody playing counted as %d live games, want 0", got) + } + + guest.setReady(true) + host.await("room_state") + host.startGame() + _, waits, _ := awaitLead(t, host, guest) + + if got := api.LiveGameCount(); got != 1 { + t.Fatalf("live games = %d after starting, want 1", got) + } + + // Nobody submits anything: the lead's own turn runs out the clock, which + // is enough to end a two-seat game. + waits.await("game_over") + + settle() + if got := api.LiveGameCount(); got != 0 { + t.Errorf("live games = %d after the game ended, want 0", got) + } +} + +// TestVersionEndpoint: GET /version answers with exactly the string the +// server was configured with, in plain text, so a deploy check can diff it +// against what was just built without parsing anything. +func TestVersionEndpoint(t *testing.T) { + _, url := newTestServer(t, chainDict(), Config{Version: "v1.2.3-test"}) + httpURL := "http" + url[len("ws"):] + + resp, err := http.Get(httpURL + "/version") + if err != nil { + t.Fatalf("GET /version: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if got := string(body); got != "v1.2.3-test" { + t.Errorf("/version = %q, want %q", got, "v1.2.3-test") + } +} + +// TestVersionEndpointDefaultsWhenUnset: a test server, like a plain `go run`, +// never sets Config.Version, and /version must still answer something rather +// than an empty body a deploy check would misread as broken. +func TestVersionEndpointDefaultsWhenUnset(t *testing.T) { + _, url := newTestServer(t, chainDict(), Config{}) + httpURL := "http" + url[len("ws"):] + + resp, err := http.Get(httpURL + "/version") + if err != nil { + t.Fatalf("GET /version: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if got := string(body); got != defaultVersion { + t.Errorf("/version = %q, want the default %q", got, defaultVersion) + } +} + +// TestDebugVarsNotOnPublicMux: expvar's counters are process-global, but +// /debug/vars is only ever supposed to answer on the separate address +// NOITU_DEBUG_ADDR names. The public mux this test dials must never carry it. +func TestDebugVarsNotOnPublicMux(t *testing.T) { + _, url := newTestServer(t, chainDict(), Config{}) + httpURL := "http" + url[len("ws"):] + + resp, err := http.Get(httpURL + "/debug/vars") + if err != nil { + t.Fatalf("GET /debug/vars: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusNotFound { + t.Errorf("/debug/vars on the public mux = %d, want 404 (it must live on NOITU_DEBUG_ADDR alone)", resp.StatusCode) + } +} diff --git a/server/internal/wsapi/hub.go b/server/internal/wsapi/hub.go index b299798..8d3d705 100644 --- a/server/internal/wsapi/hub.go +++ b/server/internal/wsapi/hub.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "errors" "sync" + "sync/atomic" "time" "github.com/tiennm99dev/noitu/server/internal/bot" @@ -27,6 +28,10 @@ var ( errRoomNotFound = errors.New("wsapi: no such room") errNoRoomCode = errors.New("wsapi: could not allocate a room code") errServerFull = errors.New("wsapi: room limit reached") + // errDraining is returned instead of errServerFull once the process has + // started shutting down, so the creator is told to come back rather than + // to wait — a full room fills back up, a draining one never will. + errDraining = errors.New("wsapi: server draining") ) // defaultMaxRooms bounds live rooms across the whole process when nothing else @@ -55,6 +60,19 @@ type hub struct { sessions map[string]*session // by resume token joinLimiter *keyedLimiter + + // draining refuses every new room once set, so a creator is told to come + // back rather than being seated in a room the shutdown below is about to + // end anyway. Read and written from outside the hub's own goroutine (there + // isn't one), so it is atomic rather than mutex-guarded. + draining atomic.Bool + + // liveGames counts rooms with a game actually running, as opposed to + // sitting in their lobby. Draining waits for this to reach zero rather + // than for the room count to, because an empty lobby has nothing a + // restart costs and waiting for it would make every deploy sit out + // somebody's abandoned tab. + liveGames atomic.Int64 } func newHub(ctx context.Context, dict Dictionary, turnLimit, graceFor, idleFor time.Duration, maxRooms int) *hub { @@ -113,7 +131,7 @@ func (h *hub) resumable(token string) (*session, bool) { // checked by reading run(), rather than by reasoning about which writes // happened to precede a `go` statement. func (h *hub) startBotRoom(s *session, difficulty bot.Difficulty) error { - r, err := h.newRegisteredRoom() + r, err := h.newRegisteredRoom(roomModeBot) if err != nil { return err } @@ -125,7 +143,7 @@ func (h *hub) startBotRoom(s *session, difficulty bot.Difficulty) error { // creator is seated, so a client can never receive the code before the seat // behind it exists. func (h *hub) createRoom(s *session) error { - r, err := h.newRegisteredRoom() + r, err := h.newRegisteredRoom(roomModePvP) if err != nil { return err } @@ -149,13 +167,24 @@ func (h *hub) joinRoom(code string, s *session) error { } // newRegisteredRoom allocates a code, registers the room and starts it. -func (h *hub) newRegisteredRoom() (*room, error) { +// +// mode is known here, before the room goroutine has processed a single input, +// because it is the hub method called — startBotRoom or createRoom — that +// decides it. Counting the room live from this point rather than from +// handleCreate/handleStartBot is deliberately generous: a room that fails to +// seat its creator still held a goroutine and a registry entry for a moment, +// and the gauge should say so. +func (h *hub) newRegisteredRoom(mode string) (*room, error) { + if h.draining.Load() { + return nil, errDraining + } + code, err := h.reserveCode() if err != nil { return nil, err } - r := newRoom(h, code, h.turnLimit, h.graceFor, h.idleFor) + r := newRoom(h, code, h.turnLimit, h.graceFor, h.idleFor, mode) // The ceiling is checked under the same lock that registers the room, so // two creators racing for the last slot cannot both get it. @@ -168,6 +197,9 @@ func (h *hub) newRegisteredRoom() (*room, error) { h.rooms[code] = r h.mu.Unlock() + metrics.roomsTotal.Add(mode, 1) + metrics.roomsLive.Add(mode, 1) + go r.run() return r, nil } @@ -179,6 +211,27 @@ func (h *hub) roomCount() int { return len(h.rooms) } +// startDraining stops the hub from seating any new room. Existing rooms are +// untouched here — telling them to stop is the caller's job, once it has +// decided how long to wait for the ones with a game running. +func (h *hub) startDraining() { h.draining.Store(true) } + +// isDraining reports whether startDraining has been called, which is what +// /readyz answers with. +func (h *hub) isDraining() bool { return h.draining.Load() } + +// liveGameCount is how many rooms currently have a game running, as opposed +// to sitting in their lobby. +func (h *hub) liveGameCount() int64 { return h.liveGames.Load() } + +// gameStarted and gameFinished keep liveGameCount accurate. A room calls +// gameStarted when its engine is built and gameFinished exactly once for +// every gameStarted — including when the room is cancelled mid-game rather +// than finishing normally, which is why room.run's teardown carries its own +// call rather than relying on broadcastGameOver alone. +func (h *hub) gameStarted() { h.liveGames.Add(1) } +func (h *hub) gameFinished() { h.liveGames.Add(-1) } + // evict removes a finished room. Called by the room goroutine as it exits, so // a code is reusable the moment its game is done. func (h *hub) evict(code string) { diff --git a/server/internal/wsapi/limits_test.go b/server/internal/wsapi/limits_test.go index f37b2e6..b55ce30 100644 --- a/server/internal/wsapi/limits_test.go +++ b/server/internal/wsapi/limits_test.go @@ -37,7 +37,7 @@ func TestTypedWordIsSanitizedBeforeItIsEchoed(t *testing.T) { lead, waits, start := pvpGame(t, url) // A zero-width joiner and a bidi override inside an otherwise legal word. - lead.submit("b‍ ‮c", start.GetTurnSeq()) + lead.submit("b\u200d \u202ec", start.GetTurnSeq()) played := waits.await("turn_update").GetTurnUpdate().GetPlayed() if played.GetWord() != "b c" { diff --git a/server/internal/wsapi/metrics.go b/server/internal/wsapi/metrics.go new file mode 100644 index 0000000..54065a0 --- /dev/null +++ b/server/internal/wsapi/metrics.go @@ -0,0 +1,82 @@ +package wsapi + +import "expvar" + +// metricSet is the process's answer to "how is the game actually being +// played" — the question plans/reports/brainstormer-260921-0016 names as +// unanswerable with nothing but a dozen scattered slog calls. It is exposed +// at GET /debug/vars, but only when the operator opts in by setting +// NOITU_DEBUG_ADDR to a separate listen address: see cmd/noitu-server/main.go. +// Nothing here is read anywhere in the game logic — a counter that fed a +// decision back into play would need its own tests for correctness, not just +// for visibility. +// +// expvar's registry is a process-wide singleton by design (see the stdlib +// package doc), so these are package-level rather than server-instance +// fields: a second *Server in the same process — every wsapi test — shares +// one set of counters rather than fighting to register a second one under the +// same name. +type metricSet struct { + connectionsOpen *expvar.Int + connectionsTotal *expvar.Int + + // roomsLive and roomsTotal are keyed "bot" or "pvp". + roomsLive *expvar.Map + roomsTotal *expvar.Map + + // gamesStarted and gamesFinished are keyed the same way. + gamesStarted *expvar.Map + gamesFinished *expvar.Map + + wordsSubmitted *expvar.Int + wordsAccepted *expvar.Int + // wordsRejected is keyed by game.RejectReason.String(), which is also + // what the word_rejected log line's reason field carries — one reason + // vocabulary rather than two. + wordsRejected *expvar.Map + + // eliminations is keyed by the wire GameEndReason string, since that is + // the one place "the reconnect window ran out" is told apart from "they + // resigned" — both are game.EndResigned to the engine underneath. + eliminations *expvar.Map + + chatLines *expvar.Int + + // joinsRefused is keyed by the same UI error code the client is sent — + // "too_many_attempts", "room_not_found" or "room_full" — so a reader + // checking this counter against the frontend copy is checking against the + // same string, not a second name for it. + joinsRefused *expvar.Map + + // botMoves is keyed by bot.Difficulty.String(). + botMoves *expvar.Map + + resumesAttempted *expvar.Int + resumesSucceeded *expvar.Int +} + +// metrics is the one instance every call site writes through. Built at +// package init rather than lazily, so every counter exists — reading at zero +// — from the moment the process starts, whether or not NOITU_DEBUG_ADDR is +// ever set. +var metrics = newMetricSet() + +func newMetricSet() *metricSet { + return &metricSet{ + connectionsOpen: expvar.NewInt("noitu_connections_open"), + connectionsTotal: expvar.NewInt("noitu_connections_total"), + roomsLive: expvar.NewMap("noitu_rooms_live"), + roomsTotal: expvar.NewMap("noitu_rooms_total"), + gamesStarted: expvar.NewMap("noitu_games_started"), + gamesFinished: expvar.NewMap("noitu_games_finished"), + wordsSubmitted: expvar.NewInt("noitu_words_submitted"), + wordsAccepted: expvar.NewInt("noitu_words_accepted"), + wordsRejected: expvar.NewMap("noitu_words_rejected"), + eliminations: expvar.NewMap("noitu_eliminations"), + chatLines: expvar.NewInt("noitu_chat_lines"), + joinsRefused: expvar.NewMap("noitu_joins_refused"), + botMoves: expvar.NewMap("noitu_bot_moves"), + resumesAttempted: expvar.NewInt("noitu_resumes_attempted"), + resumesSucceeded: expvar.NewInt("noitu_resumes_succeeded"), + } +} diff --git a/server/internal/wsapi/metrics_test.go b/server/internal/wsapi/metrics_test.go new file mode 100644 index 0000000..3a9c694 --- /dev/null +++ b/server/internal/wsapi/metrics_test.go @@ -0,0 +1,70 @@ +package wsapi + +import ( + "expvar" + "testing" + + noituv1 "github.com/tiennm99dev/noitu/server/gen/noitu/v1" +) + +// mapValue reads one key of an expvar.Map as an int64, 0 for a key never +// written. The counters under test are shared package state — every wsapi +// test that ever ran in this process wrote to them — so every assertion below +// is a delta across one call, never an absolute value. +func mapValue(m *expvar.Map, key string) int64 { + v := m.Get(key) + if v == nil { + return 0 + } + iv, ok := v.(*expvar.Int) + if !ok { + return 0 + } + return iv.Value() +} + +// TestMetricsCountWordSubmissions covers both directions handleSubmit can +// go: an engine rejection counted under its game.RejectReason string, and an +// accepted move counted separately. wordsSubmitted rises either way, which is +// what makes it the denominator for an acceptance rate. +func TestMetricsCountWordSubmissions(t *testing.T) { + _, url := newTestServer(t, chainDict(), Config{}) + c := dial(t, url) + c.hello("Người thử") + c.send(&noituv1.ClientMessage{Payload: &noituv1.ClientMessage_StartBotGame{ + StartBotGame: &noituv1.StartBotGame{Difficulty: noituv1.Difficulty_DIFFICULTY_EASY}, + }}) + started := c.await("game_started").GetGameStarted() + + submittedBefore := metrics.wordsSubmitted.Value() + acceptedBefore := metrics.wordsAccepted.Value() + rejectedBefore := mapValue(metrics.wordsRejected, "not in dictionary") + + c.submit("khong co", started.GetTurnSeq()) + c.await("move_rejected") + + if got, want := metrics.wordsSubmitted.Value(), submittedBefore+1; got != want { + t.Errorf("wordsSubmitted after a rejection = %d, want %d", got, want) + } + if got, want := metrics.wordsAccepted.Value(), acceptedBefore; got != want { + t.Errorf("wordsAccepted moved on a rejection: %d, want unchanged at %d", got, want) + } + if got, want := mapValue(metrics.wordsRejected, "not in dictionary"), rejectedBefore+1; got != want { + t.Errorf(`wordsRejected["not in dictionary"] = %d, want %d`, got, want) + } + + submittedBefore = metrics.wordsSubmitted.Value() + acceptedBefore = metrics.wordsAccepted.Value() + + // The turn never moved on the rejection above, so the same turn_seq still + // answers the current position. + c.submit("b c", started.GetTurnSeq()) + c.await("turn_update") + + if got, want := metrics.wordsSubmitted.Value(), submittedBefore+1; got != want { + t.Errorf("wordsSubmitted after an acceptance = %d, want %d", got, want) + } + if got, want := metrics.wordsAccepted.Value(), acceptedBefore+1; got != want { + t.Errorf("wordsAccepted = %d, want %d", got, want) + } +} diff --git a/server/internal/wsapi/multiplayer_test.go b/server/internal/wsapi/multiplayer_test.go index 212d96a..094a9df 100644 --- a/server/internal/wsapi/multiplayer_test.go +++ b/server/internal/wsapi/multiplayer_test.go @@ -464,8 +464,8 @@ func TestSeveralReconnectWindowsRunAtOnce(t *testing.T) { clients, _ := roomOf(t, url, 3) host, second, third := clients[0], clients[1], clients[2] - second.conn.CloseNow() - third.conn.CloseNow() + _ = second.conn.CloseNow() + _ = third.conn.CloseNow() // Both seats are held first, then both are freed. The owner is the one // still here to watch it happen. diff --git a/server/internal/wsapi/nickname_test.go b/server/internal/wsapi/nickname_test.go new file mode 100644 index 0000000..db2a80f --- /dev/null +++ b/server/internal/wsapi/nickname_test.go @@ -0,0 +1,64 @@ +package wsapi + +import ( + "strings" + "testing" + "unicode" + "unicode/utf8" +) + +// FuzzSanitizeText guards the filter every player-supplied string crosses +// before it is shown to a stranger: a nickname, a chat line and a submitted +// word all go through it. The invariants are the ones sanitizeText's own +// comment promises — no control or format character survives, no rune count +// over the cap, no run of combining marks longer than the cap allows — plus +// the one every caller depends on implicitly: it never panics on arbitrary +// input, however malformed. +// +// Bounds are fixed at the nickname's rather than fuzzed themselves: every real +// call site passes a compile-time constant, so a bound the fuzzer chooses +// would be exercising a contract sanitizeText was never asked to keep. +func FuzzSanitizeText(f *testing.F) { + seeds := []string{ + "Minh", + "Nguyễn Thuý", + "", + " \t\n ", + "Mi\x00nh\x07", + "Mi\u200bnh\u200d", + "Minh\u202e", + " Minh Nguyen ", + "Minh\nNguyen", + strings.Repeat("a", 40), + strings.Repeat("ữ", maxNicknameRunes), + "Minh" + strings.Repeat(string(stackingMark), 50), + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, raw string) { + got := sanitizeText(raw, maxNicknameRunes, maxNicknameMarks) + + if n := utf8.RuneCountInString(got); n > maxNicknameRunes { + t.Fatalf("sanitizeText(%q) kept %d runes, want at most %d", raw, n, maxNicknameRunes) + } + + marks := 0 + for _, r := range got { + switch { + case unicode.IsControl(r), unicode.Is(unicode.Cf, r): + t.Fatalf("sanitizeText(%q) = %q kept a control/format character %U", raw, got, r) + case !unicode.IsPrint(r): + t.Fatalf("sanitizeText(%q) = %q kept a non-printing character %U", raw, got, r) + case unicode.Is(unicode.Mn, r), unicode.Is(unicode.Me, r): + marks++ + if marks > maxNicknameMarks { + t.Fatalf("sanitizeText(%q) = %q stacked %d combining marks, want at most %d", raw, got, marks, maxNicknameMarks) + } + default: + marks = 0 + } + } + }) +} diff --git a/server/internal/wsapi/room.go b/server/internal/wsapi/room.go index 34eb6fd..1e46573 100644 --- a/server/internal/wsapi/room.go +++ b/server/internal/wsapi/room.go @@ -6,12 +6,23 @@ import ( "log/slog" "math/rand/v2" "slices" + "sync/atomic" "time" noituv1 "github.com/tiennm99dev/noitu/server/gen/noitu/v1" "github.com/tiennm99dev/noitu/server/internal/bot" "github.com/tiennm99dev/noitu/server/internal/dictionary" "github.com/tiennm99dev/noitu/server/internal/game" + "github.com/tiennm99dev/noitu/server/internal/vietnamese" +) + +// roomModeBot and roomModePvP are the two values a room's mode ever takes. +// They double as the label under which every mode-keyed metric and the +// word_rejected log line group their counts, so a reader checking one against +// the other is checking against the same string everywhere. +const ( + roomModeBot = "bot" + roomModePvP = "pvp" ) // botPlayerID is the seat the bot occupies. It is a normal player to the @@ -228,6 +239,20 @@ type room struct { graceFor time.Duration idleFor time.Duration + // mode is roomModeBot or roomModePvP, fixed at creation. It is the label + // every mode-keyed metric and the word_rejected log line use, kept as its + // own field rather than re-derived from strategy == nil so the hub can set + // it before the room goroutine has seated anyone or built an engine. + mode string + + // liveCounted mirrors whether this room's game is the one hub.liveGames is + // currently counting. Atomic rather than plain, because drain reads + // hub.liveGameCount() from outside the room goroutine while this flips on + // the goroutine itself; the CompareAndSwap in run's teardown is what + // guarantees exactly one hub.gameFinished() per hub.gameStarted() even when + // the room is cancelled mid-game instead of finishing normally. + liveCounted atomic.Bool + seats [maxPlayers]*seat // owner is the seat that may start a game and free the other one. It is a @@ -280,7 +305,7 @@ type Dictionary interface { Meanings(word string) []dictionary.Sense } -func newRoom(h *hub, code string, turnLimit, graceFor, idleFor time.Duration) *room { +func newRoom(h *hub, code string, turnLimit, graceFor, idleFor time.Duration, mode string) *room { if idleFor <= 0 { idleFor = defaultIdleWindow } @@ -295,6 +320,7 @@ func newRoom(h *hub, code string, turnLimit, graceFor, idleFor time.Duration) *r turnLimit: turnLimit, graceFor: graceFor, idleFor: idleFor, + mode: mode, } } @@ -332,6 +358,15 @@ func (r *room) send(msg any) bool { func (r *room) run() { defer r.cancel() defer r.hub.evict(r.code) + defer metrics.roomsLive.Add(r.mode, -1) + // Catches a room cancelled with a game still running — drain forcing the + // last stragglers closed, or a shutdown mid-game — which never reaches + // broadcastGameOver's own decrement. + defer func() { + if r.liveCounted.CompareAndSwap(true, false) { + r.hub.gameFinished() + } + }() // Whatever ended the room — everybody leaving, the idle window, a server // shutdown — the connections still seated in it must stop pointing here. // A session that keeps a dead room would answer every later action with @@ -570,6 +605,7 @@ func (r *room) handleStartBot(m startBotInput) { func (r *room) handleJoin(m joinInput) { free := r.freeSeat() if free < 0 || !r.occupied() { + metrics.joinsRefused.Add("room_full", 1) m.sess.send(errorMsg("room_full")) return } @@ -770,6 +806,10 @@ func (r *room) beginGame() error { } } + metrics.gamesStarted.Add(r.mode, 1) + r.hub.gameStarted() + r.liveCounted.Store(true) + state := r.engine.Snapshot() for _, s := range r.seats { r.sendGameStarted(s, state) @@ -817,8 +857,11 @@ func (r *room) handleSubmit(m submitInput) { // The rejection carries the server's sequence, not the client's stale one, // so the client can resynchronise from the refusal instead of having to // wait for the next turn update to discover where the game actually is. + metrics.wordsSubmitted.Add(1) + if m.turnSeq != r.turnSeq { r.sendTo(m.player, moveRejectedMsg(noituv1.RejectReason_REJECT_REASON_NOT_YOUR_TURN, m.word, r.turnSeq)) + r.recordRejection(game.ReasonNotYourTurn, m.word) return } @@ -832,11 +875,13 @@ func (r *room) handleSubmit(m submitInput) { move, reason := r.engine.Submit(m.player, word, time.Now()) if reason != game.ReasonNone { r.sendTo(m.player, moveRejectedMsg(RejectReason(reason), word, m.turnSeq)) + r.recordRejection(reason, word) // A rejection for an expired turn also took this player out of the // game, and everybody has to be told which. r.applyEliminations(before) return } + metrics.wordsAccepted.Add(1) // An accepted move never ends a game: a dead end is left for whoever // inherits it, which is what Submit's own comment explains. @@ -845,6 +890,38 @@ func (r *room) handleSubmit(m submitInput) { r.maybeScheduleBot() } +// recordRejection counts one rejected submission and logs it at Info. +// +// This is the corpus feedback loop the improvement report calls the input to +// every decision about the dictionary: which words players actually type that +// the game does not accept, and why. The word logged is never the raw typed +// text — it is normalized the same way the engine would have matched it +// (NFC, lowercase, single-spaced) and capped, so the line is useful for corpus +// review without ever logging what a player literally typed into the box. +func (r *room) recordRejection(reason game.RejectReason, raw string) { + metrics.wordsRejected.Add(reason.String(), 1) + + // sanitizeText first: raw may be the untouched client payload (the + // not-your-turn path never reaches the sanitizer below it in + // handleSubmit), and Normalize alone does not drop control or format + // characters. + word, _, err := vietnamese.Normalize(sanitizeText(raw, maxWordRunes, maxNicknameMarks)) + if err != nil { + word = "" + } + if runes := []rune(word); len(runes) > maxWordRunes { + word = string(runes[:maxWordRunes]) + } + + slog.Info("word_rejected", + "reason", reason.String(), + "word", word, + "link", r.engine.Current(), + "mode", r.mode, + "room", r.code, + ) +} + // handleBotMove applies what the worker chose. func (r *room) handleBotMove(m botMoveInput) { if r.engine == nil || r.engine.Over() { @@ -855,6 +932,7 @@ func (r *room) handleBotMove(m botMoveInput) { if m.turnSeq != r.turnSeq || r.engine.Turn() != botPlayerID { return } + metrics.botMoves.Add(r.strategy.Difficulty().String(), 1) now := time.Now() before := r.mark() @@ -1032,6 +1110,7 @@ func (r *room) broadcastElimination(id game.PlayerID, suggestions []string) { name = out.nickname } reason := r.wireEndReason(id) + metrics.eliminations.Add(reason.String(), 1) for _, s := range r.seats { if s == nil || s.sess == nil { @@ -1067,6 +1146,11 @@ func (r *room) wireEndReason(p game.PlayerID) noituv1.GameEndReason { // broadcastGameOver reports the result from each seat's point of view. func (r *room) broadcastGameOver(state game.State) { + metrics.gamesFinished.Add(r.mode, 1) + if r.liveCounted.CompareAndSwap(true, false) { + r.hub.gameFinished() + } + // The reason the game ended is the reason the last player went out, which // with two seats is the only elimination there was. reason := noituv1.GameEndReason_GAME_END_REASON_UNSPECIFIED @@ -1233,6 +1317,7 @@ func (r *room) handleResume(m resumeInput) { // Accepted. Only now is the old connection finished: its token is spent and // its socket is either gone or about to be, and leaving it registered would // let a third connection claim the same seat. + metrics.resumesSucceeded.Add(1) m.sess.attach(r, string(m.player)) if m.prior != nil { m.sess.hub.unregister(m.prior.resumeToken) @@ -1304,6 +1389,7 @@ func (r *room) handleChat(m chatInput) { if len(r.chat) > chatHistoryLimit { r.chat = r.chat[len(r.chat)-chatHistoryLimit:] } + metrics.chatLines.Add(1) for _, s := range r.seats { if s == nil || s.sess == nil { diff --git a/server/internal/wsapi/server.go b/server/internal/wsapi/server.go index 2fc6da8..d5f18a8 100644 --- a/server/internal/wsapi/server.go +++ b/server/internal/wsapi/server.go @@ -40,8 +40,16 @@ type Config struct { // default. MaxConnections caps open sockets the same way. MaxRooms int MaxConnections int + // Version is what GET /version answers and what the startup log line + // carries. Empty falls back to defaultVersion, which is what a plain + // `go run` or a test server — nothing built with -ldflags — reports. + Version string } +// defaultVersion is what an unstamped build reports: a local `go run` or a +// test server, neither of which passes -X main.version through -ldflags. +const defaultVersion = "dev" + // defaultMaxConnections bounds open WebSockets when nothing else is set. Each // one is three goroutines and an outbox; the number is generous for one // binary and small next to what the host can hold. @@ -49,10 +57,11 @@ const defaultMaxConnections = 2000 // Server wires the hub to an HTTP mux. type Server struct { - hub *hub - mux *http.ServeMux - cancel context.CancelFunc - cfg Config + hub *hub + mux *http.ServeMux + cancel context.CancelFunc + cfg Config + version string proxies []netip.Prefix maxConns int64 @@ -63,11 +72,17 @@ type Server struct { func NewServer(ctx context.Context, dict Dictionary, cfg Config) *Server { ctx, cancel := context.WithCancel(ctx) + version := cfg.Version + if version == "" { + version = defaultVersion + } + s := &Server{ hub: newHub(ctx, dict, cfg.TurnLimit, cfg.GraceFor, cfg.IdleFor, cfg.MaxRooms), mux: http.NewServeMux(), cancel: cancel, cfg: cfg, + version: version, proxies: parsePrefixes(cfg.TrustedProxies), maxConns: int64(cfg.MaxConnections), } @@ -80,6 +95,23 @@ func NewServer(ctx context.Context, dict Dictionary, cfg Config) *Server { w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("ok")) }) + // /readyz is a readiness check, distinct from /healthz above: it fails + // while draining even though the process is still alive and still + // finishing the games it already has, which is exactly the state a load + // balancer should stop sending new traffic to. + s.mux.HandleFunc("GET /readyz", func(w http.ResponseWriter, _ *http.Request) { + if s.hub.isDraining() { + http.Error(w, "draining", http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ready")) + }) + s.mux.HandleFunc("GET /version", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(s.version)) + }) s.mountStatic() go s.sweepLimiters(ctx) @@ -94,6 +126,20 @@ func (s *Server) Shutdown() { s.cancel() } +// StartDraining stops the server from seating any new room and flips +// /readyz to unhealthy. Existing games are untouched — a caller decides +// separately, via LiveGameCount, how long to wait before calling Shutdown. +func (s *Server) StartDraining() { s.hub.startDraining() } + +// RoomCount is how many rooms — lobbies and running games alike — are live +// right now, for the log lines a drain or shutdown writes. +func (s *Server) RoomCount() int { return s.hub.roomCount() } + +// LiveGameCount is how many of those rooms have a game actually running. +// Draining waits for this, not for RoomCount, because an empty lobby has +// nothing a restart costs. +func (s *Server) LiveGameCount() int64 { return s.hub.liveGameCount() } + func (s *Server) handleWS(w http.ResponseWriter, r *http.Request) { // Refused before the upgrade, so a client that is over the line is told // so in HTTP terms it can read, and never costs a socket. @@ -114,6 +160,10 @@ func (s *Server) handleWS(w http.ResponseWriter, r *http.Request) { return } + metrics.connectionsOpen.Add(1) + metrics.connectionsTotal.Add(1) + defer metrics.connectionsOpen.Add(-1) + sess := newSession(s.hub.ctx, conn, s.hub, s.clientIP(r)) sess.run() diff --git a/server/internal/wsapi/session.go b/server/internal/wsapi/session.go index 17b194f..38502c6 100644 --- a/server/internal/wsapi/session.go +++ b/server/internal/wsapi/session.go @@ -453,10 +453,12 @@ func (s *session) dispatch(msg *noituv1.ClientMessage) error { case *noituv1.ClientMessage_JoinRoom: if !s.hub.joinLimiter.allow(s.remoteIP, time.Now()) { + metrics.joinsRefused.Add("too_many_attempts", 1) s.send(errorMsg("too_many_attempts")) return nil } if err := s.hub.joinRoom(p.JoinRoom.GetRoomCode(), s); err != nil { + metrics.joinsRefused.Add("room_not_found", 1) s.send(errorMsg("room_not_found")) } @@ -519,6 +521,12 @@ func roomCreateError(sessionID string, err error) *noituv1.ServerMessage { if errors.Is(err, errServerFull) { return errorMsg("server_full") } + if errors.Is(err, errDraining) { + // The same key Shutdown sends to everyone already seated: a room + // refused for this reason will not open a moment later the way a full + // one might, so the client is told the same thing either way. + return errorMsg("server_restarting") + } slog.Error("open room", "session", sessionID, "err", err) return errorMsg("room_start_failed") } @@ -583,6 +591,7 @@ func (s *session) handleHello(h *noituv1.Hello) error { // Welcome and then silence has nothing to render and no reason to stop // waiting. func (s *session) resumeFrom(prior *session) { + metrics.resumesAttempted.Add(1) r, id := prior.currentRoom() if r == nil { s.send(errorMsg("game_already_over")) diff --git a/server/internal/wsapi/wsapi_test.go b/server/internal/wsapi/wsapi_test.go index 29815f9..dd61b77 100644 --- a/server/internal/wsapi/wsapi_test.go +++ b/server/internal/wsapi/wsapi_test.go @@ -355,7 +355,9 @@ func TestTheFirstTurnIsDrawn(t *testing.T) { // Started here rather than over a pair of sockets: a series long enough to // tell a draw from a fixed lead is far more starts than a lobby's rate // limiter allows, and none of what is being checked is on the wire. - r := &room{dict: chainDict(), turnLimit: time.Second} + // beginGame reports to the hub's live-game gauge, so this hand-built room + // needs one even though nothing here reads it back. + r := &room{hub: &hub{}, dict: chainDict(), turnLimit: time.Second} r.seats[0] = &seat{id: "p1"} r.seats[1] = &seat{id: "p2"} @@ -383,7 +385,7 @@ func TestABotGameOpensWithTheHuman(t *testing.T) { if err != nil { t.Fatalf("bot.New: %v", err) } - r := &room{dict: chainDict(), turnLimit: time.Second, strategy: strategy} + r := &room{hub: &hub{}, dict: chainDict(), turnLimit: time.Second, strategy: strategy} r.seats[0] = &seat{id: "p1"} r.seats[1] = &seat{id: botPlayerID}