PR #84195: fix(twitch): guard onMessage cleanup against newer handler (#83888)

• Repository: openclaw/openclaw
• Author: hclsys
• State: open | merged: False
• Link: https://github.com/openclaw/openclaw/pull/84195

Description (problem / solution / changelog)

Fixes #83888.

TwitchClientManager.onMessage (extensions/twitch/src/twitch-client.ts:197) registers a handler under getAccountKey(account) and returns a cleanup closure that captures key. The closure calls this.messageHandlers.delete(key) unconditionally. If a second onMessage(account, h2) call overwrites the first handler — which messageHandlers.set silently does — and the first caller's cleanup then fires, it evicts the live h2. The account is left with no handler and the next inbound chat message — and every one after — is silently dropped. The bug is invisible to existing tests because none register two handlers for the same account key and then exercise the first cleanup.

Changes

• extensions/twitch/src/twitch-client.ts: guard the cleanup closure with if (this.messageHandlers.get(key) === handler) { this.messageHandlers.delete(key); }. The lone-handler case is unchanged; the only difference is that a stale cleanup is now a no-op instead of yanking a newer handler out from under a second caller.
• extensions/twitch/src/twitch-client.test.ts: two new regression cases under the existing describe("onMessage", …) block. (i) Register h1 → register h2 (overwrites) → invoke cleanup1 → assert h2 is still the registered handler. (ii) Register h_solo → invoke cleanup_solo → assert the entry is removed (no regression on the single-handler path).

Diff stat: 2 files, +32 / -1.

Real behavior proof

• Behavior or issue addressed: messageHandlers is the per-key Map<string, (msg) => void> at twitch-client.ts:~70-80; getAccountKey(account) derives a stable {bot}:{channel} string used for both set and the captured cleanup key. The new identity check makes the cleanup closure idempotent w.r.t. unrelated overwrites — only the exact registered handler is removed.

• Real environment tested: Local Node 22.x. Probe at /tmp/probe_83888.mjs does both halves of the proof. (a) Parses the patched twitch-client.ts and verifies the cleanup closure contains this.messageHandlers.get(key) === handler and that the set(key, handler) precedes the guarded cleanup. (b) Replays the buggy and patched shapes in pure JS — registers two handlers for the same key, invokes the first cleanup, dispatches a message, and asserts: pre-fix shape silently drops the dispatch (confirms #83888's exact symptom: "Inbound messages are silently dropped"), patched shape preserves the second handler and delivers the dispatch, and the single-handler regression case still removes the lone entry on cleanup.

• Exact steps or command run after this patch: node /tmp/probe_83888.mjs

• Evidence after fix:

PASS: cleanup closure guards delete with identity check (this.messageHandlers.get(key) === handler)
PASS: handler is registered via set() before the guarded cleanup closure is returned
PASS: replay (buggy): cleanup1 deleted the live handler — message silently dropped (confirms #83888)
PASS: replay (patched): cleanup1 saw mismatched handler and skipped — h2 still receives dispatch
PASS: replay (patched, sole handler): cleanup of the only registered handler still removes it — no regression

ALL CASES PASS

• Observed result after fix: Stale cleanups from earlier onMessage registrations are now no-ops when a newer handler is in place. Inbound chat messages for the account continue to reach the newest registered handler. Single-handler unregister behavior is unchanged.

• What was not tested: Live tmi.js Twitch chat connection — that requires an OAuth token + channel and is outside the scope of a handler-registration race fix. The new vitest cases drive the exact same public boundary the original test ("should replace existing handler for same account") uses, plus the cleanup invocation and a dispatch-equivalent identity assertion.

Audit (per CLAUDE rules — all 5 steps)

• Existing-helper check: No helper is needed for a Map.get(key) === value identity check — this is the canonical pattern for handler-table deregistration. PASS
• Shared-helper caller check: onMessage has one production caller (extensions/twitch/src/monitor.ts:266, monitorTwitchProvider's unregisterHandler = clientManager.onMessage(...)) plus six test sites. The new behavior is strictly a superset: same outcome on a single-handler timeline, no eviction of a newer handler on stale cleanup. PASS
• Broader-fix rival scan: gh pr list --search '83888 in:title,body' and gh pr list --search 'onMessage cleanup in:title,body' return no open PRs. PASS
• Recent-merge audit: git log --oneline -5 -- extensions/twitch/src/twitch-client.ts shows 4f4d108639 chore(lint): remove underscore-dangle allow list (#83542) and e1061a8b46 test(live): tolerate provider drift in release checks — unrelated to handler registration. PASS
• Prototype-pollution scan: N/A — single Map.get + Map.delete call.

Changed files

• extensions/twitch/src/twitch-client.test.ts (modified, +24/-0)
• extensions/twitch/src/twitch-client.ts (modified, +8/-1)