diff --git a/packages/bot/src/handlers/player/soundcloudMatcher.ts b/packages/bot/src/handlers/player/soundcloudMatcher.ts index df90fb35d..1ef38c355 100644 --- a/packages/bot/src/handlers/player/soundcloudMatcher.ts +++ b/packages/bot/src/handlers/player/soundcloudMatcher.ts @@ -38,7 +38,7 @@ export async function streamViaSoundCloud( scStream = await playdl.stream(match.url) } catch (err) { throw new Error( - `SoundCloud: stream creation failed for "${match.name}" — ${(err as Error).message}`, + `SoundCloud: stream creation failed for "${match.name}" — ${err instanceof Error ? err.message : String(err)}`, { cause: err }, ) } diff --git a/packages/bot/src/handlers/player/streamBridge.spec.ts b/packages/bot/src/handlers/player/streamBridge.spec.ts index ff224254d..79bf453fb 100644 --- a/packages/bot/src/handlers/player/streamBridge.spec.ts +++ b/packages/bot/src/handlers/player/streamBridge.spec.ts @@ -125,6 +125,10 @@ describe('streamViaYtDlp – URL validation', () => { describe('streamViaYtDlp – process lifecycle', () => { const validUrl = 'https://www.youtube.com/watch?v=abc123' + afterEach(() => { + jest.useRealTimers() + }) + it('resolves with a PassThrough stream when stdout emits first chunk', async () => { const proc = makeFakeProc() mockSpawn.mockReturnValue(proc) diff --git a/packages/bot/src/lastfm/lastFmApi.spec.ts b/packages/bot/src/lastfm/lastFmApi.spec.ts index 28c4b1f38..c7dcbca43 100644 --- a/packages/bot/src/lastfm/lastFmApi.spec.ts +++ b/packages/bot/src/lastfm/lastFmApi.spec.ts @@ -22,6 +22,7 @@ import { parseArtists, getTrackMetadata, __resetMetadataCacheForTests, + LastFmSessionExpiredError, } from './lastFmApi' const getSessionKeyMock = @@ -233,6 +234,29 @@ describe('lastFmApi', () => { }) }) + describe('LastFmSessionExpiredError', () => { + it('creates error with default message', () => { + const error = new LastFmSessionExpiredError() + expect(error).toBeInstanceOf(Error) + expect(error.message).toBe( + 'Last.fm session key has expired (error code 9)', + ) + expect(error.name).toBe('LastFmSessionExpiredError') + }) + + it('creates error with custom message', () => { + const customMsg = 'Custom expiry message' + const error = new LastFmSessionExpiredError(customMsg) + expect(error.message).toBe(customMsg) + expect(error.name).toBe('LastFmSessionExpiredError') + }) + + it('is instanceof Error', () => { + const error = new LastFmSessionExpiredError() + expect(error instanceof Error).toBe(true) + }) + }) + describe('getTopTracks', () => { it('returns mapped tracks on success', async () => { fetchMock.mockResolvedValueOnce({ @@ -517,6 +541,17 @@ describe('lastFmApi', () => { expect(isLastFmInvalidSessionError(error)).toBe(true) }) + + it('detects LastFmSessionExpiredError instances regardless of message', () => { + expect( + isLastFmInvalidSessionError(new LastFmSessionExpiredError()), + ).toBe(true) + expect( + isLastFmInvalidSessionError( + new LastFmSessionExpiredError('totally different message'), + ), + ).toBe(true) + }) }) describe('getTagTopTracks', () => { diff --git a/packages/bot/src/lastfm/lastFmApi.ts b/packages/bot/src/lastfm/lastFmApi.ts index 76f47fb2e..c117938ab 100644 --- a/packages/bot/src/lastfm/lastFmApi.ts +++ b/packages/bot/src/lastfm/lastFmApi.ts @@ -11,6 +11,17 @@ import { debugLog } from '@lucky/shared/utils/general/log' const API_BASE = 'https://ws.audioscrobbler.com/2.0/' +/** + * Error thrown when Last.fm session key has expired (error code 9). + * Caller should re-authenticate the user. + */ +export class LastFmSessionExpiredError extends Error { + constructor(message = 'Last.fm session key has expired (error code 9)') { + super(message) + this.name = 'LastFmSessionExpiredError' + } +} + function getApiConfig(): { apiKey: string; secret: string } | null { const apiKey = process.env.LASTFM_API_KEY const secret = process.env.LASTFM_API_SECRET @@ -83,6 +94,9 @@ async function signedPost( message?: string } if (data.error) { + if (data.error === 9) { + throw new LastFmSessionExpiredError() + } throw new Error( `Last.fm ${method}: ${data.error} - ${data.message ?? ''}`, ) @@ -331,6 +345,7 @@ export async function scrobble( } export function isLastFmInvalidSessionError(error: unknown): boolean { + if (error instanceof LastFmSessionExpiredError) return true if (!(error instanceof Error)) return false const message = error.message.toLowerCase() diff --git a/packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts b/packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts index a517d56dc..e2b386b0e 100644 --- a/packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts +++ b/packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts @@ -1,76 +1,188 @@ -import { jest } from '@jest/globals' -import { hasGenreTag, createArtistTagFetcher } from './artistTagCache' +import { describe, expect, it, jest, beforeEach } from '@jest/globals' +import { + createArtistTagFetcher, + hasGenreTag, + type ArtistTagFetcher, +} from './artistTagCache' jest.mock('../../../lastfm', () => ({ getArtistTopTags: jest.fn(), })) -describe('hasGenreTag', () => { - it('returns false when tags array is empty', () => { - expect(hasGenreTag([], ['rock', 'pop'])).toBe(false) - }) +const { getArtistTopTags } = require('../../../lastfm') - it('returns true when a tag matches a genre variant', () => { - expect(hasGenreTag(['sertanejo', 'country'], ['sertanejo'])).toBe(true) +describe('createArtistTagFetcher', () => { + beforeEach(() => { + jest.clearAllMocks() }) - it('returns false when no tags match', () => { - expect(hasGenreTag(['rock', 'indie'], ['sertanejo', 'forró'])).toBe(false) + it('returns empty array for undefined artist', async () => { + const fetcher = createArtistTagFetcher() + const result = await fetcher(undefined) + expect(result).toEqual([]) }) - it('is case-insensitive', () => { - expect(hasGenreTag(['Sertanejo'], ['sertanejo'])).toBe(true) - expect(hasGenreTag(['sertanejo'], ['SERTANEJO'])).toBe(true) + it('returns empty array for empty string artist', async () => { + const fetcher = createArtistTagFetcher() + const result = await fetcher('') + expect(result).toEqual([]) }) - it('trims whitespace from tags and variants', () => { - expect(hasGenreTag([' sertanejo '], ['sertanejo'])).toBe(true) + it('returns empty array for whitespace-only artist', async () => { + const fetcher = createArtistTagFetcher() + const result = await fetcher(' ') + expect(result).toEqual([]) }) - it('returns false when variants array is empty', () => { - expect(hasGenreTag(['rock'], [])).toBe(false) + it('fetches tags from Last.fm for valid artist', async () => { + const tags = ['rock', 'alternative', 'indie'] + getArtistTopTags.mockResolvedValue(tags) + + const fetcher = createArtistTagFetcher() + const result = await fetcher('The Beatles') + + expect(getArtistTopTags).toHaveBeenCalledWith('The Beatles') + expect(result).toEqual(tags) }) -}) -describe('createArtistTagFetcher', () => { - it('returns an empty array for undefined artist', async () => { + it('caches results for same artist (case-insensitive)', async () => { + const tags = ['pop', 'electronic'] + getArtistTopTags.mockResolvedValue(tags) + const fetcher = createArtistTagFetcher() - const result = await fetcher(undefined) - expect(result).toEqual([]) + + const result1 = await fetcher('Daft Punk') + const result2 = await fetcher('daft punk') + const result3 = await fetcher('DAFT PUNK') + + expect(getArtistTopTags).toHaveBeenCalledTimes(1) + expect(result1).toEqual(tags) + expect(result2).toEqual(tags) + expect(result3).toEqual(tags) }) - it('returns an empty array for empty string artist', async () => { + it('coalesces concurrent requests for same artist', async () => { + const tags = ['country', 'folk'] + getArtistTopTags.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => resolve(tags), 50) + }), + ) + const fetcher = createArtistTagFetcher() - const result = await fetcher('') - expect(result).toEqual([]) + + const [result1, result2, result3] = await Promise.all([ + fetcher('Willie Nelson'), + fetcher('Willie Nelson'), + fetcher('Willie Nelson'), + ]) + + expect(getArtistTopTags).toHaveBeenCalledTimes(1) + expect(result1).toEqual(tags) + expect(result2).toEqual(tags) + expect(result3).toEqual(tags) }) - it('returns an empty array for whitespace-only artist', async () => { + it('returns empty array when Last.fm fails', async () => { + getArtistTopTags.mockRejectedValue(new Error('API error')) + const fetcher = createArtistTagFetcher() - const result = await fetcher(' ') + const result = await fetcher('Unknown Artist') + expect(result).toEqual([]) }) - it('fetches and caches tags for a known artist', async () => { - const { getArtistTopTags } = require('../../../lastfm') - ;(getArtistTopTags as jest.Mock).mockResolvedValue(['rock', 'indie']) + it('uses spotify fallback when Last.fm returns empty and fallback provided', async () => { + getArtistTopTags.mockResolvedValue([]) + const spotifyFallback = jest + .fn<(artist: string) => Promise>() + .mockResolvedValue(['pop', 'latin']) - const fetcher = createArtistTagFetcher() - const result1 = await fetcher('Radiohead') - const result2 = await fetcher('radiohead') + const fetcher = createArtistTagFetcher(spotifyFallback) + const result = await fetcher('Bad Bunny') - expect(result1).toEqual(['rock', 'indie']) - expect(result2).toEqual(['rock', 'indie']) - expect((getArtistTopTags as jest.Mock).mock.calls.length).toBe(1) + expect(getArtistTopTags).toHaveBeenCalledWith('Bad Bunny') + expect(spotifyFallback).toHaveBeenCalledWith('Bad Bunny') + expect(result).toEqual(['pop', 'latin']) }) - it('returns empty array when Last.fm fetch fails', async () => { - const { getArtistTopTags } = require('../../../lastfm') - ;(getArtistTopTags as jest.Mock).mockRejectedValue(new Error('API down')) + it('does not use spotify fallback when Last.fm has tags', async () => { + getArtistTopTags.mockResolvedValue(['rock', 'metal']) + const spotifyFallback = jest + .fn<(artist: string) => Promise>() + .mockResolvedValue(['pop']) - const fetcher = createArtistTagFetcher() - const result = await fetcher('Some Artist') + const fetcher = createArtistTagFetcher(spotifyFallback) + const result = await fetcher('Metallica') + + expect(spotifyFallback).not.toHaveBeenCalled() + expect(result).toEqual(['rock', 'metal']) + }) + + it('handles spotify fallback errors gracefully', async () => { + getArtistTopTags.mockResolvedValue([]) + const spotifyFallback = jest + .fn<(artist: string) => Promise>() + .mockRejectedValue(new Error('Spotify API error')) + + const fetcher = createArtistTagFetcher(spotifyFallback) + const result = await fetcher('Unknown') + + expect(result).toEqual([]) + }) + + it('converts spotify fallback non-array result to empty array', async () => { + getArtistTopTags.mockResolvedValue([]) + const spotifyFallback = jest + .fn<(artist: string) => Promise>() + .mockResolvedValue(null as unknown as string[]) + + const fetcher = createArtistTagFetcher(spotifyFallback) + const result = await fetcher('Artist') expect(result).toEqual([]) }) + + it('caches fallback results same as Last.fm results', async () => { + getArtistTopTags.mockResolvedValue([]) + const spotifyFallback = jest + .fn<(artist: string) => Promise>() + .mockResolvedValue(['genre1', 'genre2']) + + const fetcher = createArtistTagFetcher(spotifyFallback) + + const result1 = await fetcher('Artist') + const result2 = await fetcher('Artist') + + expect(spotifyFallback).toHaveBeenCalledTimes(1) + expect(result1).toEqual(result2) + }) +}) + +describe('hasGenreTag', () => { + it('returns false when tags array is empty', () => { + expect(hasGenreTag([], ['rock', 'pop'])).toBe(false) + }) + + it('returns true when a tag matches a genre variant', () => { + expect(hasGenreTag(['sertanejo', 'country'], ['sertanejo'])).toBe(true) + }) + + it('returns false when no tags match', () => { + expect(hasGenreTag(['rock', 'indie'], ['sertanejo', 'forró'])).toBe(false) + }) + + it('is case-insensitive', () => { + expect(hasGenreTag(['Sertanejo'], ['sertanejo'])).toBe(true) + expect(hasGenreTag(['sertanejo'], ['SERTANEJO'])).toBe(true) + }) + + it('trims whitespace from tags and variants', () => { + expect(hasGenreTag([' sertanejo '], ['sertanejo'])).toBe(true) + }) + + it('returns false when variants array is empty', () => { + expect(hasGenreTag(['rock'], [])).toBe(false) + }) }) diff --git a/packages/bot/src/utils/music/autoplay/artistTagCache.ts b/packages/bot/src/utils/music/autoplay/artistTagCache.ts index 8ea3491c8..175881067 100644 --- a/packages/bot/src/utils/music/autoplay/artistTagCache.ts +++ b/packages/bot/src/utils/music/autoplay/artistTagCache.ts @@ -9,15 +9,21 @@ export function hasGenreTag(tags: string[], genreVariants: string[]): boolean { } /** - * Per-pass de-duplicating Last.fm artist-tag fetcher. The replenisher creates - * one of these per autoplay pass and threads it through every candidate - * collector so we only ask Last.fm for each artist once even when the same - * artist surfaces in Spotify, Last.fm seeds, and genre searches. + * Per-pass de-duplicating artist-tag fetcher. The replenisher creates one per + * autoplay pass and threads it through every candidate collector, so each + * artist is only looked up once even when it surfaces in multiple sources. + * + * When a `spotifyFallback` is provided and Last.fm returns no tags (i.e. the + * user has not linked Last.fm), the fetcher calls the fallback and caches its + * result. This lets the cross-locale veto fire even for artists whose + * title/author carry no Spanish text markers (e.g. "Marcos Witt", "Alex Zurdo"). * * Each call returns a Promise so concurrent in-flight requests for the same * artist coalesce automatically. */ -export function createArtistTagFetcher(): ArtistTagFetcher { +export function createArtistTagFetcher( + spotifyFallback?: (artist: string) => Promise, +): ArtistTagFetcher { const cache = new Map>() return async (artist) => { @@ -26,7 +32,17 @@ export function createArtistTagFetcher(): ArtistTagFetcher { if (!key) return [] const cached = cache.get(key) if (cached) return cached - const pending = getArtistTopTags(artist).catch(() => []) + const pending = getArtistTopTags(artist) + .catch(() => []) + .then(async (tags) => { + if (tags.length === 0 && spotifyFallback) { + const result = await Promise.resolve( + spotifyFallback(artist), + ).catch(() => []) + return Array.isArray(result) ? result : [] + } + return tags + }) cache.set(key, pending) return pending } diff --git a/packages/bot/src/utils/music/autoplay/replenisher.spec.ts b/packages/bot/src/utils/music/autoplay/replenisher.spec.ts index 7968f1f27..58005a67f 100644 --- a/packages/bot/src/utils/music/autoplay/replenisher.spec.ts +++ b/packages/bot/src/utils/music/autoplay/replenisher.spec.ts @@ -17,6 +17,7 @@ jest.mock('@lucky/shared/services', () => ({ }, spotifyLinkService: { getValidAccessToken: jest.fn(), + getByDiscordId: jest.fn(), }, premiumService: { isPremium: jest.fn(), @@ -322,4 +323,20 @@ describe('replenishQueue', () => { }), ) }) + + it('passes Spotify genre fallback to createArtistTagFetcher when token is available', async () => { + const queue = createGuildQueue({ + currentTrack: createTrack({ requestedBy: { id: 'user-123' } as import('discord.js').User }), + }) + const { spotifyLinkService } = require('@lucky/shared/services') + spotifyLinkService.getValidAccessToken.mockResolvedValue('test-token') + + const { createArtistTagFetcher } = require('./artistTagCache') + + await replenishQueue(queue) + + expect(createArtistTagFetcher).toHaveBeenCalledWith( + expect.any(Function), + ) + }) }) diff --git a/packages/bot/src/utils/music/autoplay/replenisher.ts b/packages/bot/src/utils/music/autoplay/replenisher.ts index 1be21d740..503ea802b 100644 --- a/packages/bot/src/utils/music/autoplay/replenisher.ts +++ b/packages/bot/src/utils/music/autoplay/replenisher.ts @@ -10,6 +10,7 @@ import { } from '@lucky/shared/services' import { getArtistPopularity, + getArtistGenres, } from '../../../spotify/spotifyApi' import { detectSessionMood, type SessionMood } from './sessionMood' import { getRecentSkipCount } from '../../../handlers/player/trackHandlers' @@ -230,11 +231,21 @@ async function _replenishQueue( : null const replenishCount = replenishCounters.get(guildId) ?? 0 - // Tag-driven genre context (Phase 2). One Last.fm artist-tag cache for - // the whole pass — every candidate collector reuses it, plus the - // current track + recent history. Falls through to no-op when Last.fm - // is not configured. - const getArtistTags: ArtistTagFetcher = createArtistTagFetcher() + // Tag-driven genre context (Phase 2). One artist-tag cache for the + // whole pass — every candidate collector reuses it. When Last.fm is not + // linked the fetcher falls back to Spotify genre strings so the + // cross-locale veto can still reject Spanish gospel tracks whose + // title/author carry no Spanish text markers. + const spotifyToken = requestedBy?.id + ? await Promise.resolve( + spotifyLinkService.getValidAccessToken(requestedBy.id), + ).catch(() => null) + : null + const getArtistTags: ArtistTagFetcher = createArtistTagFetcher( + spotifyToken + ? (artist) => getArtistGenres(spotifyToken, artist) + : undefined, + ) const currentTrackTags = await getArtistTags(currentTrack.author) const sessionGenreFamilies = await detectSessionGenreFamilies( historyTracks, diff --git a/packages/bot/src/utils/music/queueManipulation.spec.ts b/packages/bot/src/utils/music/queueManipulation.spec.ts index 785b7bf6e..73acc2389 100644 --- a/packages/bot/src/utils/music/queueManipulation.spec.ts +++ b/packages/bot/src/utils/music/queueManipulation.spec.ts @@ -3546,6 +3546,7 @@ describe('queueManipulation — multi-user VC blend', () => { const sharedMocks = jest.requireMock('@lucky/shared/services') as any sharedMocks.spotifyLinkService.getValidAccessToken .mockResolvedValueOnce('token-for-current') + .mockResolvedValueOnce(null) // spotifyToken for artist tag fetcher .mockResolvedValueOnce(null) // collectSpotifyRecommendationCandidates .mockResolvedValueOnce('token-for-enrich')