diff --git a/internal/keylock/keylock.go b/internal/keylock/keylock.go new file mode 100644 index 0000000..86e2666 --- /dev/null +++ b/internal/keylock/keylock.go @@ -0,0 +1,33 @@ +// Package keylock serialises compound operations that target the same key +// (typically a chat / user / subject identifier) across goroutines. +// +// Why a separate package: every game module needs a per-subject mutex to +// turn KVStore's single-op atomicity into safe Get→mutate→Put. The bot +// dispatcher runs each Telegram update in its own goroutine, and the JS +// source's Cloudflare Workers serialisation is not a property Go inherits. +// +// Trade-off: the underlying sync.Map grows unboundedly with distinct keys +// (~32 B each). At 1M keys that's ~32 MB — acceptable for the lifetime of +// a Cloud Run instance, which restarts well before reaching that scale. +// Eviction is a Phase 11 concern, not a v1 one. +package keylock + +import "sync" + +// Map gives each string key its own mutex, lazily created. Zero value is +// usable; do not copy after first use (sync.Map is non-copyable). +type Map struct { + m sync.Map // key: string → val: *sync.Mutex +} + +// Acquire locks the per-key mutex and returns its Unlock as a func so the +// caller can `defer m.Acquire(key)()` at the top of a critical section. +// +// Distinct keys never block each other; same-key callers run serially in the +// order Acquire was called. +func (m *Map) Acquire(key string) func() { + v, _ := m.m.LoadOrStore(key, &sync.Mutex{}) + mu := v.(*sync.Mutex) + mu.Lock() + return mu.Unlock +} diff --git a/internal/keylock/keylock_test.go b/internal/keylock/keylock_test.go new file mode 100644 index 0000000..b9ef68a --- /dev/null +++ b/internal/keylock/keylock_test.go @@ -0,0 +1,73 @@ +package keylock + +import ( + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestMap_DistinctKeysDoNotBlock asserts the per-key fan-out: two acquires on +// different keys must run in parallel even while one is held. We arrange a +// goroutine that holds key "a" for a beat, then a second goroutine acquires +// "b" — if Acquire blocked across keys, the second goroutine couldn't finish +// before the first releases. We give it a generous timeout to avoid flakes +// on slow CI hardware while still failing if locks are global. +func TestMap_DistinctKeysDoNotBlock(t *testing.T) { + var m Map + holdA := make(chan struct{}) + releasedA := make(chan struct{}) + bDone := make(chan struct{}) + + go func() { + release := m.Acquire("a") + close(holdA) + <-time.After(50 * time.Millisecond) + release() + close(releasedA) + }() + + go func() { + <-holdA + release := m.Acquire("b") + release() + close(bDone) + }() + + select { + case <-bDone: + // Good: b acquired while a was held. + case <-time.After(40 * time.Millisecond): + t.Fatal("Acquire(\"b\") blocked while Acquire(\"a\") was held — keys are not independent") + } + <-releasedA +} + +// TestMap_SameKeySerialises asserts mutual exclusion on a shared key under +// concurrent contention. If acquires interleaved, the counter would race. +func TestMap_SameKeySerialises(t *testing.T) { + var m Map + var counter int64 + const goroutines = 32 + const itersEach = 100 + + var wg sync.WaitGroup + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < itersEach; j++ { + release := m.Acquire("shared") + // Read-modify-write that would race without serialisation. + v := atomic.LoadInt64(&counter) + atomic.StoreInt64(&counter, v+1) + release() + } + }() + } + wg.Wait() + + if got := atomic.LoadInt64(&counter); got != int64(goroutines*itersEach) { + t.Errorf("counter = %d, want %d (lost updates → mutex didn't serialise)", got, goroutines*itersEach) + } +} diff --git a/internal/modules/wordle/handlers.go b/internal/modules/wordle/handlers.go index 9f7a2e5..b4f42c9 100644 --- a/internal/modules/wordle/handlers.go +++ b/internal/modules/wordle/handlers.go @@ -3,6 +3,7 @@ package wordle import ( "context" "fmt" + "math" "strconv" "strings" "time" @@ -10,6 +11,7 @@ import ( "github.com/go-telegram/bot" "github.com/go-telegram/bot/models" + "github.com/tiennm99/miti99bot-go/internal/keylock" "github.com/tiennm99/miti99bot-go/internal/storage" ) @@ -19,7 +21,7 @@ type state struct { kv storage.KVStore words []string set map[string]struct{} - locks subjectLocks // per-subject mutex; serialises Get→mutate→Put + locks keylock.Map // per-subject mutex; serialises Get→mutate→Put } // subjectFor mirrors JS getSubject: per-user in DMs, per-chat in groups, @@ -125,7 +127,7 @@ func (s *state) handleWordle(ctx context.Context, b *bot.Bot, update *models.Upd if subject == "" { return reply(ctx, b, msg, "Cannot identify chat.") } - defer s.locks.acquire(subject)() + defer s.locks.Acquire(subject)() arg := argAfterCommand(msg.Text) g, err := s.getOrInit(ctx, subject) @@ -197,7 +199,7 @@ func (s *state) handleNew(ctx context.Context, b *bot.Bot, update *models.Update if subject == "" { return reply(ctx, b, msg, "Cannot identify chat.") } - defer s.locks.acquire(subject)() + defer s.locks.Acquire(subject)() prelude := "" prior, err := loadGame(ctx, s.kv, subject) @@ -229,7 +231,7 @@ func (s *state) handleGiveup(ctx context.Context, b *bot.Bot, update *models.Upd if subject == "" { return reply(ctx, b, msg, "Cannot identify chat.") } - defer s.locks.acquire(subject)() + defer s.locks.Acquire(subject)() g, err := s.getOrInit(ctx, subject) if err != nil { return err @@ -266,7 +268,10 @@ func (s *state) handleStats(ctx context.Context, b *bot.Bot, update *models.Upda } winRate := 0 if stats.Played > 0 { - winRate = int(float64(stats.Wins) / float64(stats.Played) * 100) + // math.Round matches JS Math.round (round half away from zero for + // positive inputs); int(...) would truncate 66.66 to 66 where JS + // shows 67. + winRate = int(math.Round(float64(stats.Wins) / float64(stats.Played) * 100)) } scope := "group" if msg.Chat.Type == models.ChatTypePrivate { diff --git a/internal/modules/wordle/locks.go b/internal/modules/wordle/locks.go deleted file mode 100644 index 941f1e7..0000000 --- a/internal/modules/wordle/locks.go +++ /dev/null @@ -1,31 +0,0 @@ -package wordle - -import "sync" - -// subjectLocks serialises Get → mutate → Put compound operations on a -// GameState by per-subject mutex. -// -// Why: the KVStore guarantees atomicity on a single op (Get, Put, Delete) but -// not on a load-then-save sequence. With the bot dispatcher running each -// Telegram update in its own goroutine, two concurrent /wordle calls for the -// same chat (groups can have many active users) can interleave and silently -// drop one player's guess. The Cloudflare Workers source the JS bot ran in -// happens to serialise this for free; Go + Firestore does not. -// -// Implementation: one *sync.Mutex per subject, lazily created via sync.Map. -// The map grows unboundedly with distinct subjects but each entry is ~32 B, -// so 1M chats costs ~32 MB — acceptable for v1. Sharded eviction is a Phase -// 11 concern. -type subjectLocks struct { - m sync.Map // key: subject string → val: *sync.Mutex -} - -// acquire locks the per-subject mutex and returns the unlock func. Callers -// must `defer s.acquire(subject)()` at the top of any handler that reads, -// mutates, then writes the same KV record. -func (s *subjectLocks) acquire(subject string) func() { - v, _ := s.m.LoadOrStore(subject, &sync.Mutex{}) - mu := v.(*sync.Mutex) - mu.Lock() - return mu.Unlock -}