refactor: audit-driven hygiene pass across modules and infra

Concurrency
- lolschedule: serialize subscriber Get→mutate→Put via state.subscribersMu;
  the single-slot list was previously losing writes under concurrent
  /lolschedule_subscribe.
- trading: PriceClient memoises its default *http.Client so /trade_stats
  reuses TLS connections across held tickers.

Observability
- server/log_middleware: defer the req log line and recover panics so a
  panicking cron handler still emits the structured req entry CloudWatch
  filters on for 5xx alerting.
- server/router (cron): inner recover with cron-name context captures the
  panicking job before the middleware's safety net does.
- telegram/webhook: rune-safe truncation in dispatch logs — Vietnamese,
  Korean, and emoji previews no longer ship as garbled bytes.
- lolschedule/api_client: same rune-safe fix for error-body log truncation.
- telegram/webhook: gate the post-recover WriteHeader(200) so a panicking
  handler that already touched w doesn't trigger superfluous-WriteHeader.

Correctness
- twentyq: clearGame error during solved-relaunch is logged instead of
  silently swallowed (was a permanent deadlock vector on KV failure).
- misc /mstats: KV read failure replies "Could not load stats. Try again
  later." to the user instead of returning into the dispatcher; matches the
  pattern other modules use.
- migrate_cf_data trading-audit-dump: surface f.Close error so a truncated
  JSONL never passes silently as a complete audit dump.

Operator ergonomics
- migrate_cf_data (all 4 subcommands): signal.NotifyContext for SIGINT /
  SIGTERM. Ctrl-C mid-Scan now propagates cleanly instead of leaving a
  half-converted DynamoDB table.
- ai/ratelimit: doc the Lambda-recycle memory bound to match keylock.Map
  so a future reviewer doesn't re-flag the unbounded map.

I/O-changing (user-approved)
- lolschedule daily push auto-prunes subscribers whose Telegram error
  matches a terminal marker (blocked / deactivated / chat gone). Transient
  errors keep the chat on the list. Subscribe message updated to mention
  the auto-cleanup.
- twentyq seed pool grown 50 → 178; repeat-collision threshold moves from
  ~9 plays to ~17 (birthday paradox).
- util /info flipped Public → Protected — chat/thread/sender IDs are no
  longer enumerable by every group member.
- cmd/server WriteTimeout 6min → 75s (cron 60s + 15s slack). No-op on
  Lambda; matters only for local non-Lambda runs.
- webhook + cron rejection paths drop response bodies (no fingerprintable
  text for internet scanners hitting the public Function URL). Status
  codes preserved for CloudWatch metrics; structured log lines carry the
  rejection reason for operator triage.

Tests added: TestTruncateRunes, TestRunDailyPush_PrunesDeadSubscribers,
TestIsTerminalSendError, TestInfo_DeniedToNonOwner,
TestInfo_DeniedToChannelMessageNoFrom, plus owner-allowed counterparts.
This commit is contained in:
tiennm99 committed 2026-05-16 13:35:00 +07:00
1 parent 16c9cce91d
commit 31f4e22c4d
19 files changed
+451 -61

No files matched your search

+11 -3
View File
@@ -20,6 +20,7 @@ import (
"net/http"
"net/url"
"time"
"unicode/utf8"
"github.com/tiennm99/miti99bot/internal/log"
"github.com/tiennm99/miti99bot/internal/storage"
@@ -259,13 +260,20 @@ func (c *Client) GetEventsCached(ctx context.Context, kv storage.KVStore, from,
return nil, fetchErr
}
// truncate clips a string to maxLen runes with "..." if cut. Keeps the log
// output bounded — lolesports occasionally returns multi-MB error pages.
// truncate clips a string to a rune-boundary prefix whose byte length is
// <= maxLen, appending "..." if cut. Keeps log output bounded — lolesports
// occasionally returns multi-MB error pages, and team/player names mix in
// Korean/Chinese characters that a raw byte slice would split mid-codepoint
// (producing replacement glyphs in CloudWatch).
func truncate(s string, maxLen int) string {
if len(s) <= maxLen {
return s
}
return s[:maxLen] + "..."
cut := maxLen
for cut > 0 && !utf8.RuneStart(s[cut]) {
cut--
}
return s[:cut] + "..."
}
// ErrEmptyResult is reserved for explicit "no events" scenarios where the
+70
View File
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"strings"
"time"
"github.com/go-telegram/bot"
@@ -13,6 +14,40 @@ import (
"github.com/tiennm99/miti99bot/internal/modules"
)
// terminalSendErrorMarkers are substrings of Telegram API errors that mean the
// chat will never accept messages again (blocked, deactivated, kicked, chat
// gone). Detecting these lets the daily-push handler prune the subscriber
// list so dead chats stop consuming the 30-msg/s global budget.
//
// String matching is fragile by nature, but the bot library surfaces these
// directly in err.Error() and Telegram has used the same wording for years.
// The false-negative path (we miss a new wording, dead chat lingers) is
// strictly safer than the false-positive path (we wrongly prune a live chat).
var terminalSendErrorMarkers = []string{
"bot was blocked by the user",
"user is deactivated",
"bot is not a member",
"chat not found",
"group chat was upgraded",
"have no rights to send",
"chat was deleted",
}
// isTerminalSendError reports whether err indicates the chat is permanently
// unreachable. Used by runDailyPush to drive auto-unsubscription.
func isTerminalSendError(err error) bool {
if err == nil {
return false
}
msg := err.Error()
for _, m := range terminalSendErrorMarkers {
if strings.Contains(msg, m) {
return true
}
}
return false
}
// dailyPushCronName is the cron route segment + EventBridge schedule key.
// Must match the regex in internal/server/router.go (^[a-z0-9_]{1,32}$).
const dailyPushCronName = "lolschedule_daily_push"
@@ -80,6 +115,7 @@ func runDailyPush(ctx context.Context, s *state, sender messageSender) error {
throttle := len(subs) > telegramRateLimitThreshold
var sent, failed int
var deadChats []int64
for i, chatID := range subs {
if throttle && i > 0 {
select {
@@ -95,14 +131,48 @@ func runDailyPush(ctx context.Context, s *state, sender messageSender) error {
}); err != nil {
log.Warn("lolschedule daily push send failed", "chat", chatID, "err", err)
failed++
if isTerminalSendError(err) {
deadChats = append(deadChats, chatID)
}
continue
}
sent++
}
// Best-effort prune. Failure here just leaves the dead chats in the list
// for tomorrow's push — same behaviour as before this code existed, so
// strictly an improvement even when the writes fail.
pruned := pruneDeadSubscribers(ctx, s, deadChats)
log.Info("lolschedule daily push complete",
"subscribers", len(subs),
"sent", sent,
"failed", failed,
"pruned", pruned,
"throttled", throttle)
return nil
}
// pruneDeadSubscribers removes chatIDs flagged as permanently unreachable.
// Serializes through state.subscribersMu so a concurrent /subscribe handler
// doesn't lose its write. Returns the number actually removed (idempotent if
// a subscriber unsubscribed between the failed send and this call).
func pruneDeadSubscribers(ctx context.Context, s *state, deadChats []int64) int {
if len(deadChats) == 0 {
return 0
}
s.subscribersMu.Lock()
defer s.subscribersMu.Unlock()
removed := 0
for _, chatID := range deadChats {
ok, err := removeSubscriber(ctx, s.kv, chatID)
if err != nil {
log.Warn("lolschedule prune dead subscriber failed", "chat", chatID, "err", err)
continue
}
if ok {
removed++
}
}
return removed
}
+81 -6
View File
@@ -14,19 +14,28 @@ import (
"github.com/tiennm99/miti99bot/internal/storage"
)
// fakeSender records every SendMessage call. errOn returns an error for the
// configured chat IDs; all others succeed.
// fakeSender records every SendMessage call. errOn returns a transient
// failure for the configured chat IDs; terminalErrOn returns a
// permanent-failure message string the dead-subscriber pruner recognises.
// All others succeed.
type fakeSender struct {
mu sync.Mutex
calls []bot.SendMessageParams
errOn map[int64]bool
mu sync.Mutex
calls []bot.SendMessageParams
errOn map[int64]bool
terminalErrOn map[int64]bool
}
func (f *fakeSender) SendMessage(_ context.Context, p *bot.SendMessageParams) (*models.Message, error) {
f.mu.Lock()
defer f.mu.Unlock()
f.calls = append(f.calls, *p)
if id, ok := p.ChatID.(int64); ok && f.errOn[id] {
id, ok := p.ChatID.(int64)
if ok && f.terminalErrOn[id] {
// String shape that isTerminalSendError matches; verifies the marker
// list works against a realistic Telegram error message.
return nil, errors.New("Forbidden: bot was blocked by the user")
}
if ok && f.errOn[id] {
return nil, errors.New("fakeSender: induced failure for chat " + chatIDString(id))
}
return &models.Message{}, nil
@@ -138,6 +147,72 @@ func TestRunDailyPush_PartialFailureContinues(t *testing.T) {
}
}
// TestRunDailyPush_PrunesDeadSubscribers locks in the auto-cleanup of chats
// that have permanently blocked the bot. Recoverable (transient) errors
// MUST NOT trigger removal — only terminal Telegram errors do.
func TestRunDailyPush_PrunesDeadSubscribers(t *testing.T) {
s := newTestState(t)
seedFreshCache(t, s.kv, nil)
chatIDs := []int64{100, 200, 300, 400}
for _, id := range chatIDs {
if _, err := addSubscriber(context.Background(), s.kv, id); err != nil {
t.Fatalf("addSubscriber %d: %v", id, err)
}
}
sender := &fakeSender{
// 200 hit a transient failure → keep on list. 400 is permanently blocked
// → prune from list.
errOn: map[int64]bool{200: true},
terminalErrOn: map[int64]bool{400: true},
}
if err := runDailyPush(context.Background(), s, sender); err != nil {
t.Fatalf("runDailyPush: %v", err)
}
remaining, err := listSubscribers(context.Background(), s.kv)
if err != nil {
t.Fatalf("listSubscribers: %v", err)
}
want := []int64{100, 200, 300} // 400 removed; 200 retained despite transient error
if len(remaining) != len(want) {
t.Fatalf("subscribers after prune: got %v, want %v", remaining, want)
}
for i, id := range want {
if remaining[i] != id {
t.Errorf("subscriber[%d]: got %d, want %d", i, remaining[i], id)
}
}
}
func TestIsTerminalSendError(t *testing.T) {
terminals := []string{
"Forbidden: bot was blocked by the user",
"Forbidden: user is deactivated",
"Bad Request: chat not found",
"Bad Request: group chat was upgraded to a supergroup chat",
}
for _, msg := range terminals {
if !isTerminalSendError(errors.New(msg)) {
t.Errorf("isTerminalSendError(%q) = false, want true", msg)
}
}
transients := []string{
"connection reset by peer",
"Too Many Requests: retry after 30",
"context deadline exceeded",
}
for _, msg := range transients {
if isTerminalSendError(errors.New(msg)) {
t.Errorf("isTerminalSendError(%q) = true, want false (transient)", msg)
}
}
if isTerminalSendError(nil) {
t.Error("isTerminalSendError(nil) = true, want false")
}
}
func TestDailyPushHandler_NilBot_ReturnsError(t *testing.T) {
s := newTestState(t)
deps := modules.Deps{KV: s.kv} // Bot intentionally nil
+11 -1
View File
@@ -2,6 +2,7 @@ package lolschedule
import (
"context"
"sync"
"time"
"github.com/go-telegram/bot"
@@ -19,6 +20,10 @@ type state struct {
// nowFn allows tests to inject a deterministic clock. Production code
// uses time.Now via the default zero-value.
nowFn func() time.Time
// subscribersMu serializes Get→mutate→Put on the single subscribers KV
// slot. Two concurrent /lolschedule_subscribe calls in the same
// millisecond would otherwise race and drop one append.
subscribersMu sync.Mutex
}
func (s *state) now() time.Time {
@@ -92,13 +97,16 @@ func (s *state) handleSubscribe(ctx context.Context, b *bot.Bot, update *models.
if msg == nil {
return nil
}
s.subscribersMu.Lock()
defer s.subscribersMu.Unlock()
added, err := addSubscriber(ctx, s.kv, msg.Chat.ID)
if err != nil {
return err
}
if added {
return chathelper.Reply(ctx, b, msg,
"✅ Subscribed. You'll get today's LoL schedule at 08:00 ICT (push activates with the cron rollout).")
"✅ Subscribed. You'll get today's LoL schedule at 08:00 ICT.\n"+
"If you block the bot, you'll be auto-unsubscribed on the next push.")
}
return chathelper.Reply(ctx, b, msg, "Already subscribed.")
}
@@ -109,6 +117,8 @@ func (s *state) handleUnsubscribe(ctx context.Context, b *bot.Bot, update *model
if msg == nil {
return nil
}
s.subscribersMu.Lock()
defer s.subscribersMu.Unlock()
removed, err := removeSubscriber(ctx, s.kv, msg.Chat.ID)
if err != nil {
return err
@@ -30,6 +30,11 @@ func listSubscribers(ctx context.Context, kv storage.KVStore) ([]int64, error) {
// addSubscriber appends chatID if absent. Returns true on first-add, false
// when already subscribed (idempotent).
//
// Concurrency: the list lives in a single KV slot, so a concurrent
// Get→mutate→Put from two chats subscribing in the same millisecond would
// drop one write. Callers MUST serialize through state.subscribersMu (or an
// equivalent module-scoped lock) before calling this.
func addSubscriber(ctx context.Context, kv storage.KVStore, chatID int64) (bool, error) {
ids, err := listSubscribers(ctx, kv)
if err != nil {
@@ -49,6 +54,9 @@ func addSubscriber(ctx context.Context, kv storage.KVStore, chatID int64) (bool,
// removeSubscriber drops chatID from the list. Returns true when removed,
// false when chatID wasn't present (idempotent).
//
// Concurrency: same single-slot Get→mutate→Put as addSubscriber; callers
// must hold state.subscribersMu.
func removeSubscriber(ctx context.Context, kv storage.KVStore, chatID int64) (bool, error) {
ids, err := listSubscribers(ctx, kv)
if err != nil {
+5 -1
View File
@@ -76,7 +76,11 @@ func mstatsCommand(deps modules.Deps) modules.Command {
text = fmt.Sprintf("last ping: %s",
time.UnixMilli(last.At).UTC().Format(time.RFC3339))
case err != nil && !errors.Is(err, storage.ErrNotFound):
return fmt.Errorf("misc /mstats: %w", err)
// User-visible reply mirrors how trading/wordle/loldle handle
// transient KV failures — returning the error here would leave
// the user with no reply at all.
log.Error("kv get failed", "module", "misc", "command", "mstats", "key", lastPingKey, "err", err)
text = "Could not load stats. Try again later."
}
return chathelper.Reply(ctx, b, update.Message, text)
},
+12 -1
View File
@@ -7,6 +7,7 @@ import (
"fmt"
"net/http"
"net/url"
"sync"
"time"
)
@@ -26,13 +27,23 @@ const kbsHTTPTimeout = 10 * time.Second
type PriceClient struct {
HTTP *http.Client
URL string
// defaultClient memoises the zero-value HTTP fallback so the transport's
// connection pool survives across FetchPrice calls — /trade_stats fans
// out per held ticker, and a fresh client per call means a fresh TLS
// handshake per ticker.
defaultOnce sync.Once
defaultClient *http.Client
}
func (c *PriceClient) httpClient() *http.Client {
if c.HTTP != nil {
return c.HTTP
}
return &http.Client{Timeout: kbsHTTPTimeout}
c.defaultOnce.Do(func() {
c.defaultClient = &http.Client{Timeout: kbsHTTPTimeout}
})
return c.defaultClient
}
func (c *PriceClient) baseURL() string {
+7 -1
View File
@@ -93,7 +93,13 @@ func (s *state) handleTwentyq(ctx context.Context, b *bot.Bot, update *models.Up
}
// Solved-but-lingering rounds → start fresh transparently (JS-parity).
if game != nil && game.Solved {
_ = clearGame(ctx, s.kv, subject)
// Best-effort delete: a hard failure here means the solved-state
// branch re-enters every call and the user is stuck. Log but keep
// going — the fresh-round write below will overwrite the slot
// regardless.
if err := clearGame(ctx, s.kv, subject); err != nil {
log.Warn("twentyq clearGame on solved-relaunch failed", "subject", subject, "err", err)
}
game = nil
}
+47 -8
View File
@@ -1,18 +1,57 @@
package twentyq
// seeds is the JS-parity seed list. Add nouns here — the LLM derives category
// + initial hint per round so no metadata table is needed.
// seeds is the seed pool. The LLM derives category + initial hint per round
// so no metadata table is needed. Keep entries:
// - single concrete nouns (no abstractions, no proper nouns, no plurals)
// - distinguishable to a player with general knowledge
// - ASCII-only so prompt-engineered locale traps don't bias the model
//
// Pool size matters because random selection is uniform without exclusion —
// at N=50 a power user sees a repeat within ~9 plays (birthday paradox); at
// N=200 the same threshold moves to ~17. This list is deliberately wide
// across categories to reduce thematic clustering when the model picks hints.
var seeds = []string{
// instruments
"guitar", "piano", "drum", "violin", "flute", "trumpet", "organ", "harmonica",
// animals
"elephant", "dolphin", "eagle", "kangaroo", "octopus", "penguin", "tiger", "horse", "snake", "owl",
"saxophone", "cello", "accordion", "banjo", "xylophone", "clarinet", "tambourine",
"harp", "ukulele", "bagpipes",
// animals — land
"elephant", "kangaroo", "tiger", "horse", "snake", "owl", "wolf", "rabbit",
"giraffe", "hedgehog", "rhinoceros", "panda", "cheetah", "gorilla", "raccoon",
"squirrel", "platypus", "bat", "armadillo", "sloth",
// animals — sea + air
"dolphin", "eagle", "octopus", "penguin", "shark", "whale", "starfish", "crab",
"jellyfish", "lobster", "parrot", "hummingbird", "flamingo", "ostrich", "seahorse",
// food
"pizza", "sushi", "burger", "ramen", "taco", "pho", "curry", "salad", "chocolate", "cheese",
"pizza", "sushi", "burger", "ramen", "taco", "pho", "curry", "salad",
"chocolate", "cheese", "pancake", "waffle", "donut", "croissant", "dumpling",
"sandwich", "lasagna", "popcorn", "kimchi", "biryani",
// fruits + vegetables
"banana", "pineapple", "watermelon", "strawberry", "mango", "avocado", "coconut",
"pomegranate", "broccoli", "carrot", "cucumber", "pumpkin", "garlic", "onion",
// vehicles
"bicycle", "car", "airplane", "boat", "train", "motorcycle", "helicopter", "submarine",
// sports
"bicycle", "car", "airplane", "boat", "train", "motorcycle", "helicopter",
"submarine", "tractor", "skateboard", "scooter", "rocket", "ambulance", "bulldozer",
// sports + games
"soccer", "basketball", "tennis", "swimming", "boxing", "golf", "chess", "skiing",
"badminton", "cricket", "rugby", "bowling", "surfing", "archery", "fencing",
// household items
"refrigerator", "microwave", "vacuum", "toaster", "kettle", "blender", "lamp", "sofa", "mirror", "broom",
"refrigerator", "microwave", "vacuum", "toaster", "kettle", "blender", "lamp",
"sofa", "mirror", "broom", "umbrella", "scissors", "telephone", "clock",
"hammer", "screwdriver", "ladder", "candle", "flashlight",
// nature + places
"mountain", "volcano", "desert", "glacier", "waterfall", "island", "forest",
"beach", "cave", "river",
// space
"telescope", "asteroid", "comet", "satellite", "galaxy",
// occupations
"firefighter", "astronaut", "chef", "carpenter", "librarian", "dentist",
"photographer", "scientist", "lifeguard", "magician",
// tools + tech
"camera", "compass", "binoculars", "keyboard", "headphones",
"microscope", "printer", "stapler",
// clothing
"sweater", "sneakers", "scarf", "helmet", "gloves", "raincoat",
// drinks
"coffee", "tea", "lemonade", "smoothie",
}
+19 -9
View File
@@ -28,8 +28,9 @@ func installUtil(t *testing.T, ownerID int64) *testutil.RecordingBot {
return rb
}
func TestInfo_PrivateChat(t *testing.T) {
rb := installUtil(t, 0)
func TestInfo_PrivateChat_OwnerAllowed(t *testing.T) {
// /info is Protected — sender must be the bot owner to get a reply.
rb := installUtil(t, 42)
rb.Bot.ProcessUpdate(context.Background(), testutil.NewPrivateMessage(42, "/info"))
got := rb.LastSent().Text()
@@ -40,8 +41,8 @@ func TestInfo_PrivateChat(t *testing.T) {
}
}
func TestInfo_GroupChat(t *testing.T) {
rb := installUtil(t, 0)
func TestInfo_GroupChat_OwnerAllowed(t *testing.T) {
rb := installUtil(t, 7)
rb.Bot.ProcessUpdate(context.Background(), testutil.NewGroupMessage(-100, 7, "/info"))
got := rb.LastSent().Text()
@@ -52,13 +53,22 @@ func TestInfo_GroupChat(t *testing.T) {
}
}
func TestInfo_ChannelMessageNoFrom(t *testing.T) {
func TestInfo_DeniedToNonOwner(t *testing.T) {
// Non-admin sender → Protected denies silently (no reply, no leak of
// the command's existence).
rb := installUtil(t, 999)
rb.Bot.ProcessUpdate(context.Background(), testutil.NewGroupMessage(-100, 7, "/info"))
if calls := rb.Sent(); len(calls) != 0 {
t.Errorf("non-owner /info replied: %+v", calls)
}
}
func TestInfo_DeniedToChannelMessageNoFrom(t *testing.T) {
// Channel posts have no From. Protected denies (no sender to check).
rb := installUtil(t, 0)
rb.Bot.ProcessUpdate(context.Background(), testutil.NewChannelMessage(-200, "/info"))
got := rb.LastSent().Text()
if !strings.Contains(got, "sender id: n/a") {
t.Errorf("info channel reply missing 'sender id: n/a'; got %q", got)
if calls := rb.Sent(); len(calls) != 0 {
t.Errorf("channel-post /info replied: %+v", calls)
}
}
+7 -2
View File
@@ -15,8 +15,13 @@ import (
// IDs, with "n/a" fallbacks. Used to debug bot routing in groups + topics.
func infoCommand() modules.Command {
return modules.Command{
Name: "info",
Visibility: modules.VisibilityPublic,
Name: "info",
// Protected (not Public) because the response exposes internal
// routing IDs — chat id, thread id, sender id. Useful for admins
// debugging group/topic routing; not something every group member
// should be able to enumerate. Non-admins see no response at all
// (Visibility denies are silent — see dispatcher.go:31).
Visibility: modules.VisibilityProtected,
Description: "Show chat id, thread id, and sender id (debug helper)",
Handler: func(ctx context.Context, b *bot.Bot, update *models.Update) error {
msg := update.Message