Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions packages/bot/src/handlers/player/errorHandlers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,22 @@ import { setupErrorHandlers } from './errorHandlers'

const debugLogMock = jest.fn()
const errorLogMock = jest.fn()
const warnLogMock = jest.fn()
const analyzeYouTubeErrorMock = jest.fn()
const logYouTubeErrorMock = jest.fn()
const recordFailureMock = jest.fn()
const recordSuccessMock = jest.fn()
const providerFromTrackMock = jest.fn()
const createErrorEmbedMock = jest.fn(() => ({ type: 'error' }))

jest.mock('@lucky/shared/utils', () => ({
debugLog: (...args: unknown[]) => debugLogMock(...args),
errorLog: (...args: unknown[]) => errorLogMock(...args),
warnLog: (...args: unknown[]) => warnLogMock(...args),
}))

jest.mock('../../utils/general/embeds', () => ({
createErrorEmbed: (...args: unknown[]) => createErrorEmbedMock(...args),
}))

jest.mock('../../utils/music/youtubeErrorHandler', () => ({
Expand Down Expand Up @@ -466,4 +473,77 @@ describe('setupErrorHandlers', () => {
}),
)
})

it('sends Discord error embed when YouTube recovery finds no tracks', async () => {
const { queueHandlers } = createPlayerWithHandlers()
providerFromTrackMock.mockReturnValue('youtube')
analyzeYouTubeErrorMock.mockReturnValue({ isParserError: false })

const channelSendMock = jest.fn().mockResolvedValue({})
const queue = {
guild: { id: 'guild-notify', name: 'Guild Notify' },
metadata: {
requestedBy: { id: 'user-1' },
channel: { id: 'ch-1', send: channelSendMock },
},
currentTrack: {
url: 'https://example.com/current',
title: 'Unavailable Song',
requestedBy: { id: 'user-1' },
},
player: { search: jest.fn().mockResolvedValue({ tracks: [] }) },
insertTrack: jest.fn(),
node: { skip: jest.fn() },
}

;(queueHandlers.playerError as PlayerErrorHandler)(
queue as any,
new Error('Could not extract stream'),
)
await flushPromises()

expect(channelSendMock).toHaveBeenCalledWith(
expect.objectContaining({ embeds: expect.any(Array) }),
)
expect(queue.node.skip).toHaveBeenCalled()
expect(warnLogMock).toHaveBeenCalledWith(
expect.objectContaining({
message: expect.stringContaining(
'YouTube recovery found nothing',
),
}),
)
})

it('sends Discord error embed when no requestedBy user', async () => {
const { queueHandlers } = createPlayerWithHandlers()
providerFromTrackMock.mockReturnValue('youtube')
analyzeYouTubeErrorMock.mockReturnValue({ isParserError: false })

const channelSendMock = jest.fn().mockResolvedValue({})
const queue = {
guild: { id: 'guild-no-user', name: 'Guild No User' },
metadata: {
requestedBy: null,
channel: { id: 'ch-1', send: channelSendMock },
},
currentTrack: {
url: 'https://example.com/current',
title: 'Song No User',
requestedBy: null,
},
player: { search: jest.fn() },
insertTrack: jest.fn(),
node: { skip: jest.fn() },
}

;(queueHandlers.playerError as PlayerErrorHandler)(
queue as any,
new Error('Could not extract stream'),
)
await flushPromises()

expect(channelSendMock).toHaveBeenCalled()
expect(queue.node.skip).toHaveBeenCalled()
})
})
50 changes: 48 additions & 2 deletions packages/bot/src/handlers/player/errorHandlers.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { QueryType, type GuildQueue } from 'discord-player'
import type { User } from 'discord.js'
import { errorLog, debugLog } from '@lucky/shared/utils'
import type { TextChannel, User } from 'discord.js'
import { errorLog, debugLog, warnLog } from '@lucky/shared/utils'
import { createErrorEmbed } from '../../utils/general/embeds'
import {
analyzeYouTubeError,
logYouTubeError,
Expand All @@ -20,6 +21,27 @@ type PlayerEvents = {

interface IQueueMetadata {
requestedBy?: User | null
channel?: TextChannel | null
}

async function notifyChannelStreamFailed(
queue: GuildQueue,
trackTitle: string,
): Promise<void> {
const channel = (queue.metadata as IQueueMetadata)?.channel
if (!channel) return
try {
await channel.send({
embeds: [
createErrorEmbed(
'⚠️ Could not play track',
`**${trackTitle}** could not be streamed from any source. It may be unavailable in your region or not on SoundCloud. Skipping to next track.`,
),
],
})
} catch {
// non-critical — don't crash if we can't notify
}
}

function toErrorDetails(error: unknown): {
Expand Down Expand Up @@ -202,6 +224,11 @@ async function recoverFromStreamExtractionError(
(queue.metadata as IQueueMetadata).requestedBy ??
undefined
if (!requestedByUser) {
warnLog({
message: 'Stream failed, skipping — no requestedBy to search with',
data: { title: currentTrack.title, guildId: queue.guild.id },
})
await notifyChannelStreamFailed(queue, currentTrack.title)
queue.node.skip()
return
Comment on lines +227 to 233

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Skip is blocked by channel notification I/O in recovery paths.

await notifyChannelStreamFailed(...) runs before queue.node.skip(). If Discord send is slow/fails late, recovery is delayed and playback can stall longer than needed.

⚡ Proposed fix (skip first, notify best-effort)
-        await notifyChannelStreamFailed(queue, currentTrack.title)
-        queue.node.skip()
+        queue.node.skip()
+        void notifyChannelStreamFailed(queue, currentTrack.title)
@@
-        await notifyChannelStreamFailed(queue, currentTrack.title)
-        queue.node.skip()
+        queue.node.skip()
+        void notifyChannelStreamFailed(queue, currentTrack.title)
@@
-        await notifyChannelStreamFailed(queue, currentTrack.title)
-        queue.node.skip()
+        queue.node.skip()
+        void notifyChannelStreamFailed(queue, currentTrack.title)
@@
-                if (failedTrack) {
-                    await notifyChannelStreamFailed(queue, failedTrack.title)
-                }
-                queue.node.skip()
+                queue.node.skip()
+                if (failedTrack) {
+                    void notifyChannelStreamFailed(queue, failedTrack.title)
+                }
#!/bin/bash
# Verify all call sites where notification is awaited before skip.
rg -n -P --type ts 'await\s+notifyChannelStreamFailed\(queue,\s*[^)]*\)\s*\n\s*queue\.node\.skip\(\)' -C 1

As per coding guidelines, "Set timeouts and handle aborts/cancellations; avoid dangling requests in API/network code."

Also applies to: 242-248, 270-277, 334-338

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/bot/src/handlers/player/errorHandlers.ts` around lines 227 - 233,
The notifyChannelStreamFailed call is awaited before skipping which blocks
recovery; change the order to call queue.node.skip() immediately (use
queue.node.skip() before notifyChannelStreamFailed), then fire
notifyChannelStreamFailed as a best-effort non-blocking operation (do not
await), wrapping it in a try/catch and apply a short timeout/AbortSignal to
bound network I/O; update all identical sites (the blocks around
notifyChannelStreamFailed, queue.node.skip, warnLog at the referenced ranges) so
skip is immediate and notification failures are handled/logged but do not
prevent playback recovery.

}
Expand All @@ -212,6 +239,11 @@ async function recoverFromStreamExtractionError(
})

if (!searchResult || searchResult.tracks.length === 0) {
warnLog({
message: 'Stream failed, YouTube recovery found nothing — skipping',
data: { title: currentTrack.title, guildId: queue.guild.id },
})
await notifyChannelStreamFailed(queue, currentTrack.title)
queue.node.skip()
return
}
Expand All @@ -229,8 +261,18 @@ async function recoverFromStreamExtractionError(
providerHealthService.recordSuccess(providerFromTrack(currentTrack))
debugLog({
message: 'Successfully recovered from stream extraction error',
data: {
title: currentTrack.title,
alternativeUrl: alternativeTrack.url,
},
})
} else {
warnLog({
message:
'Stream failed, all YouTube alternatives already in queue — skipping',
data: { title: currentTrack.title, guildId: queue.guild.id },
})
await notifyChannelStreamFailed(queue, currentTrack.title)
queue.node.skip()
}
}
Expand Down Expand Up @@ -289,6 +331,10 @@ const handlePlayerError = async (
'Failed to recover from stream extraction error:',
recoveryError,
)
const failedTrack = queue.currentTrack
if (failedTrack) {
await notifyChannelStreamFailed(queue, failedTrack.title)
}
queue.node.skip()
}
}
Expand Down
25 changes: 23 additions & 2 deletions packages/bot/src/handlers/player/playerFactory.bridge.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,28 @@ const fakeStream = { on: jest.fn() } as unknown as Readable

function makeSpawnSuccess() {
const stdout = new PassThrough()
const stderr = new PassThrough()
const proc = Object.assign(new EventEmitter(), {
stdout,
stderr,
kill: jest.fn(),
})
setImmediate(() => stdout.emit('data', Buffer.from('audio')))
return proc
}

function makeSpawnError(code = 1) {
function makeSpawnError(code = 1, stderrText = '') {
const stdout = new PassThrough()
const stderr = new PassThrough()
const proc = Object.assign(new EventEmitter(), {
stdout,
stderr,
kill: jest.fn(),
})
setImmediate(() => proc.emit('close', code))
setImmediate(() => {
if (stderrText) stderr.emit('data', Buffer.from(stderrText))
proc.emit('close', code)
})
return proc
}

Expand Down Expand Up @@ -298,10 +305,24 @@ describe('streamViaYtDlp', () => {
).rejects.toThrow(/exited with code 1/)
})

it('includes first stderr line in rejection message for diagnostics', async () => {
const proc = makeSpawnError(
1,
'ERROR: Video unavailable: This video is not available in your country',
)
spawnMock.mockReturnValue(proc)

await expect(
streamViaYtDlp('https://youtube.com/watch?v=test'),
).rejects.toThrow(/Video unavailable/)
})

it('rejects when spawn emits error', async () => {
const stdout = new PassThrough()
const stderr = new PassThrough()
const proc = Object.assign(new EventEmitter(), {
stdout,
stderr,
kill: jest.fn(),
})
spawnMock.mockReturnValue(proc)
Expand Down
29 changes: 26 additions & 3 deletions packages/bot/src/handlers/player/playerFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,11 @@
reject(new Error('yt-dlp: timed out waiting for stream start'))
}, 15_000)

// Collect stderr so failures include yt-dlp's reason (bot detection,
// geo-restriction, unavailable video, etc.) in the error message.
const stderrChunks: Buffer[] = []
proc.stderr!.on('data', (chunk: Buffer) => stderrChunks.push(chunk))

Check warning on line 174 in packages/bot/src/handlers/player/playerFactory.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This assertion is unnecessary since it does not change the type of the expression.

See more on https://sonarcloud.io/project/issues?id=LucasSantana-Dev_Lucky&issues=AZ15w0B8y10cRilHSC_l&open=AZ15w0B8y10cRilHSC_l&pullRequest=546

proc.stdout!.once('data', (firstChunk: Buffer) => {
clearTimeout(timeout)
// The `once('data')` callback consumes the first chunk from the
Expand All @@ -187,7 +192,9 @@
proc.once('close', (code) => {
clearTimeout(timeout)
if (code && code !== 0) {
reject(new Error(`yt-dlp exited with code ${code}`))
const stderr = Buffer.concat(stderrChunks).toString().trim()
const reason = stderr ? ` — ${stderr.split('\n')[0]}` : ''
reject(new Error(`yt-dlp exited with code ${code}${reason}`))
}
})
})
Expand Down Expand Up @@ -276,13 +283,29 @@
errorLog({
message: 'Bridge: all stages exhausted',
error: coreError,
data: { title: track.title, coreTitle },
data: {
title: track.title,
cleanedTitle,
coreTitle,
url: track.url,
stages: [
'yt-dlp',
'soundcloud-full',
'soundcloud-title',
'soundcloud-core',
],
},
})
}
} else {
errorLog({
message: 'Bridge: all stages exhausted',
data: { title: track.title },
data: {
title: track.title,
cleanedTitle,
url: track.url,
stages: ['yt-dlp', 'soundcloud-full', 'soundcloud-title'],
},
})
}

Expand Down