diff --git a/docs/decisions/2026-05-14-discord-integration-testing-strategy.md b/docs/decisions/2026-05-14-discord-integration-testing-strategy.md new file mode 100644 index 000000000..e1a8673f0 --- /dev/null +++ b/docs/decisions/2026-05-14-discord-integration-testing-strategy.md @@ -0,0 +1,119 @@ +# ADR: Discord Bot Integration Testing Strategy + +- **Date**: 2026-05-14 +- **Status**: Accepted +- **Deciders**: Lucas Santana +- **Tags**: testing, discord, voice, integration, ci + +--- + +## Context + +The Lucky Discord bot's test suite is entirely unit-tested with Jest mocks. All Discord.js, discord-player, and yt-dlp interactions are mocked at the boundary. This means voice channel joins, playback lifecycle, autoplay, and yt-dlp extraction are never exercised against real Discord infrastructure in CI. + +The question posed: **what is the right approach to test voice channel features, autoplay, and music features in a real Discord environment?** + +### Stack + +- discord.js v14 +- discord-player v7 + `@discordjs/opus` +- yt-dlp binary via a custom extractor service +- Node 22 / Alpine in production + +### Reliability gaps discovered during research + +1. **yt-dlp no-retry**: `service.ts` fires a single 30 s timeout then hard-kills with no retry. Network blips or CDN hiccups cause permanent playback failures. +2. **Lifecycle snapshot no-timeout**: `lifecycleHandlers.ts` calls `restoreSnapshot()` with no `Promise.race()` guard — a slow/hung DB call blocks the entire queue restore path. +3. **Backend test desert**: `packages/backend` has 9.3 k LOC but only 2 test files (bootstrap + one integration). Route and service logic is entirely uncovered. +4. **No Docker binary validation**: yt-dlp is bundled in the image but is never smoke-tested in CI after build — a broken binary reaches production silently. + +--- + +## Decision + +**DEFER** in-CI voice integration testing against real Discord infrastructure. + +Instead, invest in the four higher-ROI reliability improvements immediately, and treat a **post-deploy staging voice smoke test** (non-blocking) as the correct long-term integration gate if production incident volume justifies the cost. + +--- + +## Alternatives Considered + +### 1. Staging Bot in CI (Discord API calls against a real test guild) + +A dedicated bot token + test guild are provisioned. CI spins up the bot, joins a voice channel, plays a short clip, asserts events. + +**Rejected because:** +- Fundamentally flaky: voice UDP, yt-dlp downloads, Discord API rate limits, and CI runner network variance combine into a test that passes 90 % of the time at best. +- Discord.js maintainers themselves do not run real voice integration tests in CI ([confirmed by research](https://github.com/discordjs/discord.js/discussions)). +- Cost: a secret-bearing bot token in CI is a supply-chain risk; the test guild requires ongoing maintenance. +- The scenario most likely to catch real bugs (connection drop, stream stall) cannot be reliably triggered on demand. +- **Revisit if**: 3+ distinct production voice-join incidents in a quarter that would have been caught by this test. + +### 2. Protocol Replay / Captured Fixture Tests + +Record Discord WebSocket frames + UDP audio packets in a real session; replay them in unit tests against a fake Discord server. + +**Rejected because:** +- Engineering effort estimated at 2–3 sprints, largely infrastructure work with low direct feature value. +- Discord's gateway protocol is not public and changes without notice; fixtures would rot quarterly. +- The discord.js v14 ecosystem has no maintained capture/replay tooling. Building it from scratch is scope creep. +- **Revisit if**: a community library reaches stable release (check `discord-mock`, `discordeno` mock mode quarterly). + +### 3. Alternative Mock Libraries + +`discord.js-mock`, `@skyra/discord-components`, `discordeno` mock adapters. + +**Rejected because:** none maintains discord.js v14 compatibility as of 2026-05-14 (confirmed by npm + GitHub research). The bot's mock boundaries in Jest already achieve the same effect for unit tests. + +### 4. In-Process Audio Pipeline Tests (chosen subset) + +Test the audio processing chain (yt-dlp → FFmpeg → Opus encoder) without Discord connectivity, using local audio files. + +**Partially accepted**: already achievable within the current unit test framework by injecting a `file://` URL into the extractor. This is captured in PR #2 scope (yt-dlp retry logic). + +### 5. Post-Deploy Staging Voice Smoke Test (non-blocking) + +After a successful production deploy, a GitHub Actions `workflow_dispatch` (or post-deploy hook) runs `node scripts/smoke-voice.ts` against a staging guild. The step is marked `continue-on-error: true`. + +**Deferred, not rejected**: this is the correct integration approach if/when incident volume justifies the cost. It is explicitly listed in the plan (PR #3, optional). + +--- + +## The Plan (in priority order) + +| # | Change | Effort | PR target | +|---|--------|--------|-----------| +| 1 | Docker CI step: `yt-dlp --version` smoke inside built image | 0.25 h | after #848 | +| 2 | yt-dlp retry logic (3× exponential backoff: 30 s → 45 s → 60 s) | 2 h | release/v2.12.0 | +| 2 | `restoreSnapshot()` wrapped in `Promise.race(2 s)` with warn + empty-queue fallback | 1 h | same PR | +| 2 | Backend route test scaffolding (≥ 1 test per route group) | 3 h | same PR | +| 3 | Post-deploy staging voice smoke test (optional, non-blocking) | 3 h | separate PR | + +--- + +## Consequences + +### Positive +- Eliminates the two most likely production reliability gaps (yt-dlp blips, stuck restores) without adding CI flakiness. +- Backend test coverage gap begins to close. +- yt-dlp binary breakage is caught in CI before reaching production. +- No new secrets, bots, or Discord guilds to maintain in CI. + +### Negative +- Voice channel join/leave, playback, autoplay, and volume commands remain integration-untested in CI. +- A regression in discord-player or @discordjs/opus would not be caught until post-deploy or user report. + +### Neutral +- The decision is reversible: adding a staging bot integration test later is additive work, not a rewrite. +- Discord-player's own test suite covers the playback engine; we rely on that upstream coverage. + +--- + +## Revisit When + +- **3+ distinct production voice-join incidents** in a rolling 90-day window that a CI integration test would have caught → evaluate Staging Bot option. +- **CI voice flakiness falls below 0.5 %** (unlikely without a major ecosystem shift, but monitor quarterly). +- **A maintained discord.js v14 mock library reaches stable release** → evaluate Protocol Replay option. +- **Discord publishes a stable test guild / bot sandbox API** → re-evaluate entirely. +- **yt-dlp retry + lifecycle timeout changes reduce production incidents to zero for 2 releases** → the ROI argument for voice integration testing weakens further; record and defer indefinitely. diff --git a/packages/bot/src/handlers/player/lifecycleHandlers.ts b/packages/bot/src/handlers/player/lifecycleHandlers.ts index e03d1a056..f941920cf 100644 --- a/packages/bot/src/handlers/player/lifecycleHandlers.ts +++ b/packages/bot/src/handlers/player/lifecycleHandlers.ts @@ -49,11 +49,14 @@ export const setupLifecycleHandlers = (player: { if (ENVIRONMENT_CONFIG.MUSIC.SESSION_RESTORE_ENABLED) { const metadata = queue.metadata as QueueMetadata | undefined + const restoreDeadline = new Promise((resolve) => setTimeout(resolve, 2000)) - await musicSessionSnapshotService.restoreSnapshot( - queue, - metadata?.requestedBy ?? undefined, - ) + await Promise.race([ + musicSessionSnapshotService.restoreSnapshot(queue, metadata?.requestedBy ?? undefined), + restoreDeadline.then(() => { + infoLog({ message: `Snapshot restore timed out in ${queue.guild.name}, continuing with empty queue` }) + }), + ]) } musicWatchdogService.arm(queue) diff --git a/packages/bot/src/utils/music/ytdlpExtractor/service.ts b/packages/bot/src/utils/music/ytdlpExtractor/service.ts index 66b4bf1fc..f62e1b580 100644 --- a/packages/bot/src/utils/music/ytdlpExtractor/service.ts +++ b/packages/bot/src/utils/music/ytdlpExtractor/service.ts @@ -95,69 +95,69 @@ export class YtDlpExtractorService extends BaseExtractor { ] } - private setupProcessHandlers( - process: { - stdout?: { - on: (event: string, callback: (data: Buffer) => void) => void - } - stderr?: { - on: (event: string, callback: (data: Buffer) => void) => void - } - on: (event: string, callback: (code: number | null) => void) => void - kill: () => void - }, - resolve: (result: YtDlpExtractorResult) => void, - ): void { - let stdout = '' - let stderr = '' - - process.stdout?.on('data', (data: Buffer) => { - stdout += data.toString() - }) + private spawnYtDlp(query: string, timeoutMs: number): Promise { + return new Promise((resolve) => { + const args = this.buildYtDlpArgs(query) + const proc = spawn(this.options.executablePath, args, { + stdio: ['pipe', 'pipe', 'pipe'], + }) - process.stderr?.on('data', (data: Buffer) => { - stderr += data.toString() - }) + let stdout = '' + let stderr = '' + let settled = false - process.on('close', (code: number | null) => { - if (code === 0) { - const tracks = this.parseOutput(stdout) - resolve({ - success: true, - tracks, - }) - } else { - resolve({ - success: false, - error: stderr || `Process exited with code ${code}`, - }) + const settle = (result: YtDlpExtractorResult): void => { + if (settled) return + settled = true + clearTimeout(timer) + resolve(result) } - }) - process.on('error', (error: unknown) => { - resolve({ - success: false, - error: error instanceof Error ? error.message : 'Unknown error', + const timer = setTimeout(() => { + proc.kill() + settle({ success: false, error: 'yt-dlp timeout' }) + }, timeoutMs) + + proc.stdout?.on('data', (data: Buffer) => { stdout += data.toString() }) + proc.stderr?.on('data', (data: Buffer) => { stderr += data.toString() }) + + proc.on('close', (code: number | null) => { + if (code === 0) { + settle({ success: true, tracks: this.parseOutput(stdout) }) + } else { + settle({ success: false, error: stderr || `Process exited with code ${code}` }) + } }) - }) - setTimeout(() => { - process.kill() - resolve({ - success: false, - error: 'yt-dlp timeout', + proc.on('error', (error: unknown) => { + settle({ + success: false, + error: error instanceof Error ? error.message : 'Unknown error', + }) }) - }, this.options.timeout) + }) } private async executeYtDlp(query: string): Promise { - return new Promise((resolve) => { - const args = this.buildYtDlpArgs(query) - const process = spawn(this.options.executablePath, args, { - stdio: ['pipe', 'pipe', 'pipe'], - }) - this.setupProcessHandlers(process, resolve) - }) + const baseTimeout = this.options.timeout + const timeouts = [baseTimeout, Math.round(baseTimeout * 1.5), baseTimeout * 2] + + for (let attempt = 0; attempt < timeouts.length; attempt++) { + if (attempt > 0) { + debugLog({ message: `yt-dlp retry attempt ${attempt + 1} for query: ${query}` }) + } + + const result = await this.spawnYtDlp(query, timeouts[attempt]) + + if (result.success) return result + + const isLastAttempt = attempt === timeouts.length - 1 + if (isLastAttempt) return result + + debugLog({ message: `yt-dlp attempt ${attempt + 1} failed (${result.error}), retrying…` }) + } + + return { success: false, error: 'yt-dlp exhausted all retries' } } private parseOutput(_output: string): unknown[] { diff --git a/packages/bot/tests/utils/music/ytdlpExtractor.test.ts b/packages/bot/tests/utils/music/ytdlpExtractor.test.ts index dcf5994df..44f343091 100644 --- a/packages/bot/tests/utils/music/ytdlpExtractor.test.ts +++ b/packages/bot/tests/utils/music/ytdlpExtractor.test.ts @@ -1,12 +1,18 @@ import { spawn } from 'child_process' import { EventEmitter } from 'events' import { Readable } from 'stream' +import { YtDlpExtractorService } from '../../../src/utils/music/ytdlpExtractor/service' jest.mock('child_process') jest.mock('@lucky/shared/utils', () => ({ errorLog: jest.fn(), debugLog: jest.fn(), })) +jest.mock('discord-player', () => ({ + BaseExtractor: class MockBase { + constructor() {} + }, +})) const mockSpawn = spawn as jest.MockedFunction @@ -31,15 +37,6 @@ describe('YtDlpExtractorService', () => { describe('validate', () => { it('validates YouTube URLs', async () => { - jest.mock('discord-player', () => ({ - BaseExtractor: class MockBase { - constructor() {} - }, - })) - - const { YtDlpExtractorService } = - await import('../../../src/utils/music/ytdlpExtractor/service') - const extractor = new YtDlpExtractorService({} as any, {}) expect( @@ -54,6 +51,81 @@ describe('YtDlpExtractorService', () => { }) }) + describe('retry behavior', () => { + async function flushMicrotasks(n = 4) { + for (let i = 0; i < n; i++) await Promise.resolve() + } + + it('returns tracks on the first successful attempt', async () => { + const proc = createMockProcess() + mockSpawn.mockReturnValue(proc as any) + + const extractor = new YtDlpExtractorService({} as any, { timeout: 30000 }) + const handlePromise = extractor.handle('https://youtube.com/watch?v=test', {} as any) + + proc.emit('close', 0) + + const result = await handlePromise + expect(result.tracks).toEqual([]) + expect(mockSpawn).toHaveBeenCalledTimes(1) + }) + + it('retries after first failure and succeeds on second attempt', async () => { + const proc1 = createMockProcess() + const proc2 = createMockProcess() + mockSpawn.mockReturnValueOnce(proc1 as any).mockReturnValueOnce(proc2 as any) + + const extractor = new YtDlpExtractorService({} as any, { timeout: 30000 }) + const handlePromise = extractor.handle('https://youtube.com/watch?v=test', {} as any) + + proc1.emit('close', 1) + await flushMicrotasks() + + proc2.emit('close', 0) + + const result = await handlePromise + expect(result.tracks).toEqual([]) + expect(mockSpawn).toHaveBeenCalledTimes(2) + }) + + it('throws after all three attempts fail', async () => { + const procs = [createMockProcess(), createMockProcess(), createMockProcess()] + procs.forEach((p) => mockSpawn.mockReturnValueOnce(p as any)) + + const extractor = new YtDlpExtractorService({} as any, { timeout: 30000 }) + const handlePromise = extractor.handle('https://youtube.com/watch?v=test', {} as any) + + procs[0].emit('close', 1) + await flushMicrotasks() + procs[1].emit('close', 1) + await flushMicrotasks() + procs[2].emit('close', 1) + + await expect(handlePromise).rejects.toThrow() + expect(mockSpawn).toHaveBeenCalledTimes(3) + }) + + it('kills the process and retries on timeout', async () => { + const proc1 = createMockProcess() + const proc2 = createMockProcess() + mockSpawn.mockReturnValueOnce(proc1 as any).mockReturnValueOnce(proc2 as any) + + const extractor = new YtDlpExtractorService({} as any, { timeout: 1000 }) + const handlePromise = extractor.handle('https://youtube.com/watch?v=test', {} as any) + + jest.advanceTimersByTime(1000) + await flushMicrotasks() + + expect(proc1.kill).toHaveBeenCalled() + + proc2.emit('close', 0) + + const result = await handlePromise + expect(result.tracks).toEqual([]) + expect(mockSpawn).toHaveBeenCalledTimes(2) + }) + }) + describe('yt-dlp process integration', () => { it('spawns yt-dlp with bestaudio format', () => { const proc = createMockProcess()