From 5848751c6bc57e4a21e282ecda7f2f0ffc1ea03b Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 28 Sep 2026 15:19:59 +0700 Subject: [PATCH] refactor(server): share env parsing, shutdown and score capping helpers Collapse the three env parsers into one generic helper, the duplicated graceful-shutdown block into shutdownServer, and the score cap into pointsFor. Behaviour is unchanged. --- server/cmd/noitu-server/main.go | 64 ++++++++++++------------ server/internal/dictionary/store.go | 8 +-- server/internal/dictionary/store_test.go | 26 ++-------- server/internal/game/engine.go | 46 ++++++----------- server/internal/game/engine_test.go | 8 --- server/internal/game/state.go | 4 +- 6 files changed, 56 insertions(+), 100 deletions(-) diff --git a/server/cmd/noitu-server/main.go b/server/cmd/noitu-server/main.go index dbbec7d..7d487b4 100644 --- a/server/cmd/noitu-server/main.go +++ b/server/cmd/noitu-server/main.go @@ -145,15 +145,20 @@ func run() error { 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() - } + _ = shutdownServer(debugSrv) + return shutdownServer(srv) +} - shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownGrace) +// shutdownServer stops srv gracefully, giving in-flight requests up to +// shutdownGrace. A nil srv — the debug listener when it is not configured — +// is a no-op. +func shutdownServer(srv *http.Server) error { + if srv == nil { + return nil + } + ctx, cancel := context.WithTimeout(context.Background(), shutdownGrace) defer cancel() - return srv.Shutdown(shutdownCtx) + return srv.Shutdown(ctx) } // newDebugServer builds the expvar listener, or nil when NOITU_DEBUG_ADDR is @@ -222,19 +227,12 @@ func loadConfig() config { } } -// envInt falls back loudly, like envDuration. Zero means "use the built-in -// default", so it is what an unset or invalid value becomes. +// envInt falls back loudly, like envDuration. Zero is a real value — for the +// limits it configures it means "use the built-in default" — so only a +// negative or unparseable one is rejected. func envInt(key string, fallback int) int { - raw := strings.TrimSpace(os.Getenv(key)) - if raw == "" { - return fallback - } - n, err := strconv.Atoi(raw) - if err != nil || n < 0 { - slog.Warn("ignoring invalid integer", "key", key, "value", raw, "using", fallback) - return fallback - } - return n + return envParsed(key, fallback, "ignoring invalid integer", strconv.Atoi, + func(n int) bool { return n >= 0 }) } func env(key, fallback string) string { @@ -249,32 +247,32 @@ func env(key, fallback string) string { // 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 == "" { - 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 + return envParsed(key, fallback, "ignoring invalid duration", time.ParseDuration, + func(d time.Duration) bool { return d > 0 }) } // 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 { + return envParsed(key, fallback, "ignoring invalid duration", time.ParseDuration, + func(d time.Duration) bool { return d >= 0 }) +} + +// envParsed reads key through parse, returning fallback when it is unset or +// blank, and — with a warning, so a misconfiguration is visible rather than +// silently ignored — when it fails to parse or is not valid. +func envParsed[T any](key string, fallback T, invalidMsg string, parse func(string) (T, error), valid func(T) bool) T { 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) + v, err := parse(raw) + if err != nil || !valid(v) { + slog.Warn(invalidMsg, "key", key, "value", raw, "using", fallback) return fallback } - return d + return v } // envList returns nil for an unset variable, which coder/websocket reads as diff --git a/server/internal/dictionary/store.go b/server/internal/dictionary/store.go index 32ac0cc..7f95faa 100644 --- a/server/internal/dictionary/store.go +++ b/server/internal/dictionary/store.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "iter" + "math/rand/v2" "net/url" "os" "slices" @@ -27,8 +28,6 @@ import ( "strings" "unicode" - "math/rand/v2" - "golang.org/x/text/unicode/norm" _ "modernc.org/sqlite" ) @@ -98,7 +97,7 @@ func Open(path string) (*Store, error) { return nil, fmt.Errorf("dictionary not found at %s — run 'make fetch-dict && make dict' first: %w", path, err) } - db, err := sql.Open("sqlite", dsn(path)) + db, err := sql.Open("sqlite", DSN(path, true)) if err != nil { return nil, fmt.Errorf("open dictionary: %w", err) } @@ -148,9 +147,6 @@ func Open(path string) (*Store, error) { return s, nil } -// dsn is the read-only URI the store opens with. -func dsn(path string) string { return DSN(path, true) } - // DSN builds a SQLite URI for path. The path must be escaped: SQLite reads // '#' as a URI fragment delimiter, so a bare path containing one silently // opens a different (usually nonexistent) file and reports a confusing schema diff --git a/server/internal/dictionary/store_test.go b/server/internal/dictionary/store_test.go index fc91346..da7da7a 100644 --- a/server/internal/dictionary/store_test.go +++ b/server/internal/dictionary/store_test.go @@ -269,20 +269,13 @@ func TestResolveUnknown(t *testing.T) { } } -// nearMissFixtureAt builds a tiny dictionary purpose-built for NearMiss: one +// nearMissFixture builds a tiny dictionary purpose-built for NearMiss: one // word with a unique stripped form, one ambiguous pair that shares a stripped // form with each other, and nothing else that could coincidentally collide. -func nearMissFixtureAt(tb testing.TB, dir string) string { +func nearMissFixture(tb testing.TB) *Store { tb.Helper() - path := filepath.Join(dir, "nearmiss.db") - db, err := sql.Open("sqlite", "file:"+path) - if err != nil { - tb.Fatal(err) - } - defer func() { _ = db.Close() }() - - data := fixtureSchema + ` + path := writeDB(tb, fixtureSchema+` INSERT INTO meta VALUES ('source_license','CC BY-SA 4.0'),('word_count','4'),('meaning_count','0'); INSERT INTO words VALUES ('bình yên','bình','yên',2), @@ -292,17 +285,8 @@ INSERT INTO words VALUES ('ngữ nghĩa','ngữ','nghĩa',2), ('ngừ nghĩa','ngừ','nghĩa',2); INSERT INTO syllables VALUES ('bình',1),('an',1),('ngữ',1),('ngừ',1); -` - if _, err := db.Exec(data); err != nil { - tb.Fatal(err) - } - return path -} - -func nearMissFixture(tb testing.TB) *Store { - tb.Helper() - - store, err := Open(nearMissFixtureAt(tb, tb.TempDir())) +`) + store, err := Open(path) if err != nil { tb.Fatalf("Open: %v", err) } diff --git a/server/internal/game/engine.go b/server/internal/game/engine.go index 29ee707..53a9441 100644 --- a/server/internal/game/engine.go +++ b/server/internal/game/engine.go @@ -298,44 +298,30 @@ func (e *Engine) pointsFor(syllables int, link string, now time.Time) (int, []Po {Kind: PointKindSpeed, Value: e.speedPoints(now)}, {Kind: PointKindRarity, Value: e.rarityPoints(link)}, } - parts = capParts(parts) - total := 0 - for _, p := range parts { - total += p.Value + // Anything over maxPointsPerWord is trimmed from the end: rarity first, + // then speed, then syllables. Base and the chain term never need touching + // to make room — the chain term is itself capped at chainBonusWords words + // (10 base + 2*15 chain = 40 at most), well under the cap — so the loop + // always finds enough in the later terms and stops before reaching them. + for i, overflow := len(parts)-1, sumParts(parts)-maxPointsPerWord; i >= 0 && overflow > 0; i-- { + cut := min(parts[i].Value, overflow) + parts[i].Value -= cut + overflow -= cut } - return total, parts + + // A PointPart exists only for a term that actually contributed. + parts = slices.DeleteFunc(parts, func(p PointPart) bool { return p.Value <= 0 }) + return sumParts(parts), parts } -// capParts trims a word's score down to maxPointsPerWord when the terms -// pointsFor computed add up to more, and drops whatever term that leaves at -// zero — a PointPart exists only for a term that actually contributed. -// -// Trimmed from the end: rarity first, then speed, then syllables, then chain. -// Base and the chain term never need touching to make room — the chain term -// is itself capped at chainBonusWords words (10 base + 2*15 chain = 40 at -// most), well under the cap — so the loop always finds enough in the later -// terms and stops before reaching them. -func capParts(parts []PointPart) []PointPart { +// sumParts is the total a word's score breakdown adds up to. +func sumParts(parts []PointPart) int { total := 0 for _, p := range parts { total += p.Value } - if overflow := total - maxPointsPerWord; overflow > 0 { - for i := len(parts) - 1; i >= 0 && overflow > 0; i-- { - cut := min(parts[i].Value, overflow) - parts[i].Value -= cut - overflow -= cut - } - } - - kept := parts[:0] - for _, p := range parts { - if p.Value > 0 { - kept = append(kept, p) - } - } - return kept + return total } // speedPoints pays for the share of the turn the player left on the clock. diff --git a/server/internal/game/engine_test.go b/server/internal/game/engine_test.go index 1f6ecd1..41390ab 100644 --- a/server/internal/game/engine_test.go +++ b/server/internal/game/engine_test.go @@ -477,14 +477,6 @@ func TestSubmitScoringPartsAreTrimmedAtTheCap(t *testing.T) { } } -func sumParts(parts []PointPart) int { - sum := 0 - for _, p := range parts { - sum += p.Value - } - return sum -} - func TestPointKindStrings(t *testing.T) { seen := map[string]bool{} for k := PointKind(0); k < NumPointKinds; k++ { diff --git a/server/internal/game/state.go b/server/internal/game/state.go index de42bb3..c27ac1b 100644 --- a/server/internal/game/state.go +++ b/server/internal/game/state.go @@ -198,7 +198,7 @@ type State struct { Over bool Winner PlayerID EndReason EndReason - // Standings is the final table, best first. Meaningless while the game is - // in play, for the same reason Winner is. + // Standings is the final table, best first. Nil while the game is in + // play, when it would be meaningless for the same reason Winner is. Standings []Standing }