mirror of
https://github.com/ZHANGTIANYAO1/teamspeak-music-bot.git
synced 2026-10-02 04:52:50 +08:00
fix: harden bot lifecycle, validate HTTP inputs, make YouTube truly optional
Major bug fixes and corner-case hardening across the backend, plus a
comprehensive feature test suite. All 94 unit tests + 51 integration
tests pass against a local TS3 server.
Lifecycle & state consistency
-----------------------------
- Bug A: startBot() now wraps connect() in a 15s deadline. A hung TS
handshake no longer blocks the /start HTTP call forever; the failing
instance is torn down and the caller gets a clean 500.
- Bug B: executeCommand rejects audio-dispatching commands (play, add,
next, skip, prev, playlist, album, fm) when the bot is disconnected.
Config-only commands (vol, mode, clear, stop, queue, now, lyrics)
still work so the UI stays usable while offline.
- Bug C: the tsClient 'disconnected' handler always clears player state
now, even when connect() never completed. A separate disconnectEmitted
flag guards duplicate external event emission. Previously an orphaned
connect attempt that idle-timed-out would leave playing=true forever.
- resolveAndPlay re-checks this.connected AFTER the URL-resolve await so
a stop() during the network call can't spawn ffmpeg on a disconnected
bot.
- connect() throws if disconnect() fired during the handshake await,
preventing a concurrent stop from being overwritten by a late connected
flag flip.
- startBot always disconnects the outgoing BotInstance before creating
a replacement, covering the mid-handshake case where isConnected()
still returned false but the library client was live.
- startBot now reuses the stored identity so server groups granted to
the bot survive restarts (was regenerating a fresh UID each time).
WebSocket reliability
---------------------
- BotManager extends EventEmitter and emits 'botInstance' whenever a
new instance is created. websocket.ts listens and re-attaches its
stateChange / connected / disconnected listeners immediately, fixing
the bug where player-bar UI never updated until manual refresh.
- attachedBots map now stores the BotInstance reference and detaches
stale listeners when the instance is replaced. Safety-net interval
(5s) also reconciles to catch anything missed.
- removeBot emits 'botInstanceRemoved' -> WS broadcasts a new
{type:"botRemoved", botId} message. Client drops the bot from its
local store instead of showing it as permanently offline.
HTTP input validation
---------------------
- /volume rejects non-number, NaN, Infinity, and out-of-range values
with a proper 400 instead of a 200 OK wrapping a usage-text string.
- /mode rejects anything not in {seq, loop, random, rloop} with 400.
- /seek rejects NaN / Infinity / negative (previously NaN slipped
through typeof==="number" and poisoned seekOffset).
- /play-at validates index < queue.size() BEFORE stopping current
playback (was silently killing the current song on invalid input).
- /play, /add, /playlist, /play-by-id, /add-by-id, /play-playlist
all honour platform=youtube now (previously fell through to netease
and silently played the wrong platform).
YouTube made truly optional
---------------------------
- Lazy checkYtDlpAvailable() runs `yt-dlp --version` once, caches only
positive results so users can install yt-dlp mid-run and have it
picked up without a restart.
- getAuthStatus() returns loggedIn=false with nickname
"YouTube (yt-dlp not installed)" when the binary is missing. UI can
grey out YouTube instead of silently returning empty searches.
- findYtDlp() picks .exe on win32 and bare binary elsewhere.
- /auth/status?platform=youtube now routes to the YouTube provider
instead of falling through to NetEase and leaking the NetEase
user's nickname + avatar.
- /auth/cookie rejects platform=youtube with 400 instead of clobbering
the NetEase cookie entry.
- README documents yt-dlp install paths (bin/ local vs PATH) and adds
a dedicated "Optional: YouTube source" section.
Bot Selector UI
---------------
- New power button in each row of the dropdown with play-state-aware
styling: disabled + wait-cursor during API call, green highlight when
connected, greys out when the bot is offline.
- Dropdown always visible when >=1 bot exists, bigger font + padding.
Queue correctness
-----------------
- PlayQueue.remove(current) now decrements currentIndex so next() in
sequential mode advances to the shifted song. Previously removing
the currently-playing track silently skipped the next track because
current() falsely reported it as active and next() then incremented
past it.
Vote-skip hardening
-------------------
- cmdVote: needed threshold is Math.max(1, ceil(users/2)) so a single
voter in an empty channel can't unanimously pass a vote with
needed=0.
- resolveAndPlay clears voteSkipUsers on every new track load so votes
can't leak across songs via cmdPlay/cmdPlaylist/cmdAlbum/cmdFm paths.
cmdAdd parity
-------------
- cmdAdd auto-plays the newly-added song if the player was idle,
matching /api/player/:id/add-by-id behaviour. Previously add'ing to
an empty queue on a connected+idle bot silently enqueued without
starting playback.
Test suite
----------
- scripts/test_full_feature.py — 51 tests across 10 groups exercising
every HTTP endpoint, WebSocket broadcasts, all music providers, bot
lifecycle, disconnected-state corners, seek validation, input
validation, and the main race conditions. Captures and restores the
target bot's initial state. Resilient to TS3 anti-flood via retry
with exponential backoff. Runs against a real local TS3 server.
- scripts/test_rapid_cycle.py — Bugs A/B/C regressions
- scripts/test_corner_cases.py — disconnect-during-connect race, config
commands while disconnected, etc.
- scripts/test_more_corners.py — resolveAndPlay race, seek NaN
- scripts/test_power_button.py — E2E for the new power button
- scripts/test_bot_remove.py — E2E for WS botRemoved broadcast
- scripts/test_playbar.py — player bar auto-show regression (updated
to restore bot state on exit)
- scripts/test_multibot.py — two-bot concurrent playback monitor
- src/audio/queue.test.ts — 4 new vitest cases for remove() edge cases
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
6e828b9c2d
commit
4643f70f4a
21 files changed
+2305
-134
No files matched your search
+77
-8
@@ -57,6 +57,7 @@ export class BotInstance extends EventEmitter {
|
||||
private config: BotConfig;
|
||||
private logger: Logger;
|
||||
private connected = false;
|
||||
private disconnectEmitted = false;
|
||||
private voteSkipUsers = new Set<string>();
|
||||
private isAdvancing = false;
|
||||
|
||||
@@ -108,28 +109,42 @@ export class BotInstance extends EventEmitter {
|
||||
});
|
||||
|
||||
this.tsClient.on("disconnected", () => {
|
||||
// Avoid duplicate event if disconnect() was called explicitly
|
||||
// (it already set connected = false and emitted "disconnected").
|
||||
if (!this.connected) return;
|
||||
// Always reset local state — covers the case where connect() never
|
||||
// completed (hanging handshake → 60s library idle timeout) and
|
||||
// this.connected was never flipped to true. Previously this handler
|
||||
// short-circuited on !this.connected, leaving player stuck as "playing".
|
||||
this.connected = false;
|
||||
this.player.stop();
|
||||
// Only emit externally once per lifecycle so clients don't see a
|
||||
// duplicate "disconnected" after an explicit disconnect() call.
|
||||
if (this.disconnectEmitted) return;
|
||||
this.disconnectEmitted = true;
|
||||
this.emit("disconnected");
|
||||
});
|
||||
}
|
||||
|
||||
async connect(): Promise<void> {
|
||||
this.disconnectEmitted = false;
|
||||
await this.tsClient.connect();
|
||||
// Race guard: if disconnect() was called while the handshake was
|
||||
// awaiting, don't flip connected back to true — that would leave the
|
||||
// bot in an inconsistent state (externally "connected" but the tsClient
|
||||
// has already been torn down).
|
||||
if (this.disconnectEmitted) {
|
||||
throw new Error("Connect aborted by concurrent disconnect");
|
||||
}
|
||||
this.connected = true;
|
||||
this.emit("connected");
|
||||
}
|
||||
|
||||
disconnect(): void {
|
||||
this.player.stop();
|
||||
// Set connected = false BEFORE tsClient.disconnect() so that the
|
||||
// "disconnected" event handler (setupTsEvents) won't emit a duplicate.
|
||||
this.connected = false;
|
||||
if (!this.disconnectEmitted) {
|
||||
this.disconnectEmitted = true;
|
||||
this.emit("disconnected");
|
||||
}
|
||||
this.tsClient.disconnect();
|
||||
this.emit("disconnected");
|
||||
}
|
||||
|
||||
private async handleTextMessage(msg: TS3TextMessage): Promise<void> {
|
||||
@@ -170,6 +185,24 @@ export class BotInstance extends EventEmitter {
|
||||
cmd: ParsedCommand,
|
||||
msg?: TS3TextMessage
|
||||
): Promise<string | null> {
|
||||
// Reject commands that would push audio when the bot isn't connected:
|
||||
// otherwise ffmpeg spawns and voice goes to a half-initialized or
|
||||
// torn-down TS client, leaving player.state="playing" on a disconnected
|
||||
// bot. Config-only commands (vol, mode, clear, stop, queue, now) are
|
||||
// still allowed so the UI stays usable while the bot is offline.
|
||||
const AUDIO_COMMANDS = new Set([
|
||||
"play",
|
||||
"add",
|
||||
"next",
|
||||
"skip",
|
||||
"prev",
|
||||
"playlist",
|
||||
"album",
|
||||
"fm",
|
||||
]);
|
||||
if (!this.connected && AUDIO_COMMANDS.has(cmd.name)) {
|
||||
throw new Error("Bot is not connected to TeamSpeak");
|
||||
}
|
||||
switch (cmd.name) {
|
||||
case "play":
|
||||
return this.cmdPlay(cmd);
|
||||
@@ -235,6 +268,14 @@ export class BotInstance extends EventEmitter {
|
||||
|
||||
/** Resolve URL for a song and start playing it. Skips to next if URL fails. */
|
||||
async resolveAndPlay(song: QueuedSong): Promise<boolean> {
|
||||
if (!this.connected) {
|
||||
this.logger.warn({ songId: song.id, name: song.name }, "resolveAndPlay called on disconnected bot — skipping");
|
||||
return false;
|
||||
}
|
||||
// Clear any accumulated skip votes — every fresh track starts with a
|
||||
// clean slate, regardless of which code path loaded it (cmdPlay,
|
||||
// cmdPlaylist, cmdAlbum, cmdFm, trackEnd auto-advance, etc.).
|
||||
this.voteSkipUsers.clear();
|
||||
const provider = this.getProviderFor(song.platform);
|
||||
try {
|
||||
const url = await provider.getSongUrl(song.id);
|
||||
@@ -242,6 +283,18 @@ export class BotInstance extends EventEmitter {
|
||||
this.logger.warn({ songId: song.id, name: song.name }, "No URL available, skipping");
|
||||
return false;
|
||||
}
|
||||
// Re-check connection state AFTER the network round-trip — the URL
|
||||
// resolve can take multiple seconds and the user may have called stop
|
||||
// during that window. Without this, we'd spawn ffmpeg on a
|
||||
// disconnected bot and land back in the same "connected=false but
|
||||
// playing=true" inconsistency that Bug C was about.
|
||||
if (!this.connected) {
|
||||
this.logger.warn(
|
||||
{ songId: song.id, name: song.name },
|
||||
"bot disconnected during URL resolve — aborting playback",
|
||||
);
|
||||
return false;
|
||||
}
|
||||
song.url = url;
|
||||
this.player.play(url);
|
||||
this.database.addPlayHistory({
|
||||
@@ -288,7 +341,20 @@ export class BotInstance extends EventEmitter {
|
||||
return `No results found for: ${cmd.args}`;
|
||||
|
||||
const song = result.songs[0];
|
||||
const wasIdle = this.player.getState() === "idle";
|
||||
this.queue.add({ ...song, platform: provider.platform });
|
||||
|
||||
// If nothing was playing, start this newly-added song immediately.
|
||||
// Matches /api/player/:id/add-by-id behavior so both add paths feel
|
||||
// the same to the user (add to idle bot → plays now).
|
||||
if (wasIdle) {
|
||||
this.queue.playAt(this.queue.size() - 1);
|
||||
this.player.resetFailures();
|
||||
await this.resolveAndPlay(this.queue.current()!);
|
||||
this.emit("stateChange");
|
||||
return `Now playing: ${song.name} - ${song.artist}`;
|
||||
}
|
||||
|
||||
this.emit("stateChange");
|
||||
return `Added to queue: ${song.name} - ${song.artist} (position ${this.queue.size()})`;
|
||||
}
|
||||
@@ -440,8 +506,11 @@ export class BotInstance extends EventEmitter {
|
||||
if (!msg) return "Vote can only be used in TeamSpeak";
|
||||
this.voteSkipUsers.add(msg.invokerUid);
|
||||
const clients = await this.tsClient.getClientsInChannel();
|
||||
const totalUsers = clients.length - 1;
|
||||
const needed = Math.ceil(totalUsers / 2);
|
||||
const totalUsers = clients.length - 1; // exclude the bot itself
|
||||
// At least 1 vote is always required — otherwise a single voter in an
|
||||
// otherwise empty channel (or a transient clients.length=1 race) could
|
||||
// unanimously "win" with needed=0.
|
||||
const needed = Math.max(1, Math.ceil(totalUsers / 2));
|
||||
const votes = this.voteSkipUsers.size;
|
||||
|
||||
if (votes >= needed) {
|
||||
|
||||
Reference in new issue
Block a user