PR #84375: fix(twitch): evict registry entry even when disconnectAll throws (#83886)

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

Description (problem / solution / changelog)

Fixes #83886.

removeClientManager in extensions/twitch/src/client-manager-registry.ts:75 is the public teardown entry point — plugin.ts:208 calls it when an account is stopped or reconfigured. The body is:

const entry = registry.get(accountId);
if (!entry) return;
await entry.manager.disconnectAll();
registry.delete(accountId);

If disconnectAll() rejects (a quit() call on a broken socket, an underlying tmi.js exception, etc.), registry.delete(accountId) is never reached. The entry survives in the module-level Map. The next getOrCreateClientManager(accountId, …) short-circuits on the cache branch (if (existing) return existing.manager) and hands back the broken / partially-disconnected manager — silent stale-state hazard on top of the original disconnect failure.

Changes

• extensions/twitch/src/client-manager-registry.ts: wrap disconnectAll in try { … } finally { registry.delete; logger.info }. The finally block is unconditional, so the registry is cleaned up whether the disconnect succeeded or threw. The original rejection still propagates to the caller (finally doesn't suppress it).
• extensions/twitch/src/client-manager-registry.test.ts: NEW colocated test file. Mocks TwitchClientManager at the constructor boundary so disconnectAll can be driven to resolve or reject without standing up a real @twurple/chat client. Four cases — happy path eviction, no-op when no entry exists, regression: throwing disconnectAll still evicts, and end-to-end: after a failed remove, getOrCreateClientManager constructs a fresh manager (not the stale one). Uses vi.resetModules() to start each test with an empty module-level registry, so the test file is self-contained and doesn't depend on a separate test-only export.

Diff stat: 2 files, +105 / -6. Behavior change is strictly safer: any case that previously resolved still does identically; the previously-broken throwing-disconnect path now leaves the registry in a clean state.

Real behavior proof

• Behavior or issue addressed: Sanitized issue evidence — removeClientManager is the sole caller-facing teardown for registry, and registry is the cache layer that getOrCreateClientManager short-circuits through. The eviction must be atomic with the teardown intent, not gated on a successful disconnect.

• Real environment tested: Local Node 22.x. Probe at /tmp/probe_83886.mjs does both halves of the proof. (a) Parses the patched client-manager-registry.ts and verifies (i) removeClientManager wraps disconnectAll in try { … } finally { … }, (ii) registry.delete(accountId) is inside the finally block, (iii) no remaining unguarded delete-after-disconnectAll path. (b) Replays the registry semantics in pure JS for four scenarios — buggy shape (throwing disconnectAll → registry retains 'acc-1', confirming the #83886 symptom), patched shape (throwing disconnectAll → registry evicts 'acc-1' AND error still propagates to caller), patched happy path (clean disconnect still evicts), and patched no-op (missing key returns without throw).

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

• Evidence after fix:

PASS: removeClientManager now wraps disconnectAll in try { … } finally { … }
PASS: registry.delete(accountId) is called inside the finally block (always runs)
PASS: no remaining unguarded delete-after-disconnectAll path
PASS: replay (buggy): disconnectAll throws → registry retains 'acc-1' (confirms #83886)
PASS: replay (patched): disconnectAll throws → registry evicts 'acc-1' AND error still propagates
PASS: replay (patched, happy path): clean disconnectAll → entry evicted as expected
PASS: replay (patched, missing key): no-op, no throw

ALL CASES PASS

• Observed result after fix: A removeClientManager(accountId) call where the underlying client's disconnectAll throws still removes the entry from the module-level registry. The next getOrCreateClientManager(accountId, …) call gets a fresh manager bound to whatever logger the caller passed, instead of the stale failed-disconnect instance. Production cache-hit behavior within a healthy session (same accountId, no remove) is unchanged.

• What was not tested: A live @twurple/chat client with a real broken WebSocket — the included regression test mocks disconnectAll at the boundary, which is the same shape the broader test suite uses for this module (twitch-client.test.ts mocks @twurple/chat at the constructor level).

Audit (per CLAUDE rules — all 5 steps)

• Existing-helper check: No helper introduced — try { … } finally { … } is the canonical primitive for "always run cleanup regardless of failure." The registry.delete call is the same operation the buggy path was attempting; only the placement moves. PASS
• Shared-helper caller check: removeClientManager has 1 production caller (extensions/twitch/src/plugin.ts:208, account-stop / account-reconfigure path). The new behavior is strictly safer: any case that previously resolved still does so; the previously-broken throwing-disconnect path now cleans up correctly. PASS
• Broader-fix rival scan: gh pr list --search '83886 in:title,body' and gh pr list --search 'removeClientManager twitch' return no open PRs. The issue timeline cross-references issue #83887 (which is a sibling clawpatch finding that I shipped via #84244 — not a competing PR). PASS
• Recent-merge audit: git log --oneline -5 -- extensions/twitch/src/client-manager-registry.ts shows e1061a8b46 test(live): tolerate provider drift in release checks — unrelated. PASS
• Prototype-pollution scan: N/A — Map.delete and try/finally on locally-bound values.

Changed files

• extensions/twitch/src/client-manager-registry.test.ts (added, +97/-0)
• extensions/twitch/src/client-manager-registry.ts (modified, +12/-6)