Second-pass review of the 54-commit self-correcting audit. Each item below
was confirmed by reading the surrounding source (and, where practical, the
pre-fix code) before being changed; regression tests are included for every
behavioral fix.
Concurrency:
- eventbus: Bus.Subscribe called wg.Add with no synchronization against a
concurrent Bus.Stop's wg.Wait, a real "WaitGroup misuse" panic risk (e.g. a
Telegram-bot settings save racing panel shutdown/restart). Stop now flips a
mu-guarded `stopped` flag before waiting, and Subscribe checks it under the
same lock, so Add and Wait can no longer race.
Security:
- login_limiter: evictForRoom's fallback eviction picked an arbitrary map
key, including ones still under an active cooldown - an attacker flooding
/login with fresh usernames could evict their own (or anyone's) blocked
record and reset the lockout. The fallback now skips actively-blocked
records, only falling back to an unconditional evict if the map is
somehow entirely full of active blocks (preserves the hard memory cap).
Subscription-endpoint panics (reachable by any client hitting /sub):
- internal/sub/service.go: applyPathAndHostParams/Obj (ws/httpupgrade/xhttp
with no path settings object) and the TLS alpn readers in three places
used unchecked type assertions - exactly the bug class abab7cd0 patched
elsewhere in the same switch statements, just not these call sites.
- internal/sub/json_service.go, clash_service.go: the externalProxy loops
in the JSON and Clash generators used unchecked assertions on a
legacy/admin-supplied field (missing "port", non-object entry, etc.).
- internal/sub/json_service.go: realityData's shortId/serverName selection
could assert a non-string array element.
Other correctness:
- client_traffic.go: ResetAllTraffics (touched by 3eb214d0) still skipped
clearing NodeClientTraffic node-sync baselines, unlike its sibling reset
paths in the same file - a node's next sync would re-add pre-reset delta
on top of the freshly-zeroed counter.
- inbound_traffic.go: the traffic-tick tx's Commit/Rollback errors were
silently discarded; now logged so a backend-level commit failure (e.g. an
aborted Postgres tx from a best-effort helper) doesn't masquerade as a
successful tick.
- outbound_subscription.go: the new subscriptionFetchClient doc comment was
wedged between fetchAndStore's existing comment and fetchAndStore itself,
leaving fetchAndStore undocumented and the comment describing the wrong
function.
Convention cleanup:
- Removed narrative // comments added by the audit that violate this repo's
no-inline-comment rule (mostly narrating the specific bug/fix rather than
a lasting contract, and mostly on new Test functions, which this repo's
existing tests never comment) - calibrated against this exact codebase's
own pre-existing comment style so legitimate godoc-style doc comments
were left alone.
SendMsgToTgbotDeleteAfter spawns a goroutine that, after the display delay,
deleted the transient message and then unconditionally cleared the chat's
conversation state. Every caller that ends a wizard step already clears the
state synchronously, so that call was redundant — and harmful: if within
the delay the user advanced to the next step (a callback sets a fresh
awaiting_* state), the late goroutine wiped it, and the user's next message
fell through unrecognized, silently dropping their input.
Move the delayed deletion into deleteMessageAfterDelay, which only removes
the message and no longer touches the conversation state. Guard
deleteMessageTgBot against a nil bot so the deletion path is unit-testable.
claude[bot]