From e21c145ac404544b242296b2ffd4f577896d5d64 Mon Sep 17 00:00:00 2001 From: Ismael Leon Date: Sat, 12 Sep 2026 00:38:18 -0600 Subject: [PATCH] Fix !lyrics, which never worked with the pinned lyricsgenius MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every lookup died in the client constructor: WARNING loopify.lyrics: Genius lookup failed for '...': Genius.__init__() got an unexpected keyword argument 'quiet' `quiet` belonged to lyricsgenius 2.x. The pinned 3.12.2 has no such argument and no `verbose` either — it prints nothing of its own, so there was never anything to silence and the argument is simply dropped rather than replaced. The TypeError happened while building the client, so it took out every query, and the command answered "Couldn't find lyrics for ..." — which reads like a failed search rather than a broken build. That is why it went unnoticed: the command appeared to work and simply never found anything. The bug predates the cleanup in #32. What changed is that #32 replaced `print(f"[Lyrics] Error: {e}")` with a real logger call, so the reason finally showed up in journalctl with a level and a logger name attached. Two related problems fixed alongside it: - No timeout was passed, so a lookup could pin an executor thread indefinitely — and a pinned executor thread delays the loop's shutdown, the ceiling #35 was about. It is now bounded at 10s (the library default is 5s). - A missing token built a client anyway, and lyricsgenius then falls back to $GENIUS_ACCESS_TOKEN and raises KeyError. `fetch` now gives up before that, which is also the honest answer for a user typing !lyrics on a deployment without a token. `skip_non_songs=True` and `remove_section_headers=False` match the library's current defaults, but stay explicit because the behaviour is relied on: Genius indexes tracklists and credits pages, and the `[Chorus]` markers are wanted in the embed. Tests assert both rather than trusting the defaults to hold. tests/test_lyrics_api.py is new — this module had no tests at all, which is exactly how a TypeError in a constructor lived here unnoticed. Nothing in them talks to Genius; they check that the installed library accepts what we pass, that a missing token is handled before it can raise, that the call is bounded, and that the blocking search never runs on the event loop. Verified against the real API from the server: "Bohemian Rhapsody" / "Queen" comes back with its section headers intact, and a nonsense query returns None with no traceback. Tests: 344, up from 332. Closes #37 --- services/lyrics_api.py | 40 +++++++-- tests/test_lyrics_api.py | 170 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 7 deletions(-) create mode 100644 tests/test_lyrics_api.py diff --git a/services/lyrics_api.py b/services/lyrics_api.py index 33b262e..50e4950 100644 --- a/services/lyrics_api.py +++ b/services/lyrics_api.py @@ -15,13 +15,30 @@ log = logging.getLogger("loopify.lyrics") +# Genius is reached from a thread, so a lookup that never returns pins that +# thread — and a pinned executor thread delays the event loop's shutdown. The +# library's own default is 5s; this trades a little patience for the occasional +# slow response while staying well inside the shutdown budget (see #35). +GENIUS_TIMEOUT = 10.0 + @lru_cache(maxsize=1) def _client() -> lyricsgenius.Genius: - """One client for the process — rebuilding it per query buys nothing.""" - genius = lyricsgenius.Genius(GENIUS_TOKEN, quiet=True, skip_non_songs=True) - genius.remove_section_headers = False - return genius + """ + One client for the process — rebuilding it per query buys nothing. + + There is deliberately no verbosity argument. ``quiet`` belonged to + lyricsgenius 2.x and raises ``TypeError`` on 3.x, which is what broke + ``!lyrics`` entirely; 3.x prints nothing of its own, so there is nothing left + to silence. The other two are passed because their behaviour is relied on, + not because the defaults differ. + """ + return lyricsgenius.Genius( + GENIUS_TOKEN, + skip_non_songs=True, # tracklists and credits pages aren't lyrics + remove_section_headers=False, # keep [Chorus] and friends in the embed + timeout=GENIUS_TIMEOUT, + ) async def fetch(title: str, artist: str = "", *, loop=None) -> Optional[dict]: @@ -29,13 +46,22 @@ async def fetch(title: str, artist: str = "", *, loop=None) -> Optional[dict]: Search Genius for lyrics. Returns a dict with ``title``, ``artist``, ``lyrics`` and ``url``, or None - when there is no match or Genius is unreachable — a missing lyric is never - a reason to take the bot down. + when there is no match, no token, or Genius is unreachable — a missing lyric + is never a reason to take the bot down. """ + if not GENIUS_TOKEN: + # Constructing a client without one makes lyricsgenius fall back to + # $GENIUS_ACCESS_TOKEN and raise KeyError. config.validate() already + # warned about this at startup. + log.debug("No GENIUS_TOKEN configured; skipping lookup for %r", title) + return None + loop = loop or asyncio.get_event_loop() def _search(): - return _client().search_song(title, artist) if artist else _client().search_song(title) + client = _client() + # An empty artist matches worse than no artist at all. + return client.search_song(title, artist) if artist else client.search_song(title) try: song = await loop.run_in_executor(None, _search) diff --git a/tests/test_lyrics_api.py b/tests/test_lyrics_api.py new file mode 100644 index 0000000..2360683 --- /dev/null +++ b/tests/test_lyrics_api.py @@ -0,0 +1,170 @@ +""" +The Genius client, built against the lyricsgenius that is actually installed. + +Issue #37: `!lyrics` had never worked with the pinned lyricsgenius. The client +was constructed with `quiet=True`, an argument from the 2.x series, so every +lookup died in the constructor with a TypeError and the command answered +"Couldn't find lyrics" — which reads like a failed search, not a broken build. + +Nothing here talks to Genius. The point is the wiring: that the constructor +accepts what we pass it, that a missing token is handled before it can raise, +and that the call is bounded. +""" + +import threading +from unittest.mock import MagicMock + +import lyricsgenius +import pytest + +from services import lyrics_api + +TOKEN = "not-a-real-token" + + +@pytest.fixture +def token(monkeypatch): + """ + A token present in the module, and a client cache cleared around it. + + The real cached function is held onto rather than looked up again on the way + out: tests that replace ``_client`` outright would otherwise have the + teardown reach for ``cache_clear`` on their own stand-in. + """ + cached = lyrics_api._client + cached.cache_clear() + monkeypatch.setattr(lyrics_api, "GENIUS_TOKEN", TOKEN) + yield TOKEN + cached.cache_clear() + + +def _recording_client(calls: list) -> MagicMock: + """A client whose ``search_song`` records the positional arguments it got.""" + def search_song(*args): + calls.append(args) + return FakeSong() + return MagicMock(search_song=search_song) + + +class FakeSong: + title = "Bohemian Rhapsody" + artist = "Queen" + lyrics = "Is this the real life?" + url = "https://genius.invalid/song" + + +# -- building the client ----------------------------------------------- + +def test_the_client_is_accepted_by_the_installed_lyricsgenius(token): + """ + The regression itself: this raised + ``TypeError: Genius.__init__() got an unexpected keyword argument 'quiet'``. + """ + assert isinstance(lyrics_api._client(), lyricsgenius.Genius) + + +def test_the_client_is_reused(token): + assert lyrics_api._client() is lyrics_api._client() + + +def test_section_headers_are_kept(token): + """`[Chorus]` and friends are wanted in the embed, so they stay.""" + assert lyrics_api._client().remove_section_headers is False + + +def test_non_songs_are_skipped(token): + """Genius indexes tracklists and credits pages; those are not lyrics.""" + assert lyrics_api._client().skip_non_songs is True + + +def test_the_lookup_is_bounded(token): + """ + An unbounded lookup pins an executor thread, and a pinned executor thread + delays the event loop's shutdown — the ceiling that #35 was about. + """ + assert 0 < lyrics_api._client().timeout <= 30 + + +# -- fetching ---------------------------------------------------------- + +async def test_a_missing_token_gives_up_before_building_a_client(monkeypatch): + """ + Without a token lyricsgenius falls back to $GENIUS_ACCESS_TOKEN and raises + KeyError, so the guard has to come first. config.validate() already warns + about this at startup; a user typing !lyrics should just get an answer. + """ + lyrics_api._client.cache_clear() + monkeypatch.setattr(lyrics_api, "GENIUS_TOKEN", None) + monkeypatch.setattr(lyrics_api, "_client", + MagicMock(side_effect=AssertionError("must not be built"))) + + assert await lyrics_api.fetch("Bohemian Rhapsody") is None + + +async def test_a_found_song_becomes_a_result(token, monkeypatch): + monkeypatch.setattr(lyrics_api, "_client", + lambda: MagicMock(search_song=lambda *a, **k: FakeSong())) + + result = await lyrics_api.fetch("Bohemian Rhapsody", "Queen") + + assert result == { + "title": "Bohemian Rhapsody", + "artist": "Queen", + "lyrics": "Is this the real life?", + "url": "https://genius.invalid/song", + } + + +async def test_no_match_is_not_an_error(token, monkeypatch): + monkeypatch.setattr(lyrics_api, "_client", + lambda: MagicMock(search_song=lambda *a, **k: None)) + + assert await lyrics_api.fetch("asdkjhaskdjh") is None + + +async def test_an_unreachable_genius_is_not_an_error(token, monkeypatch): + """A missing lyric is never a reason to take the bot down.""" + def explode(*args, **kwargs): + raise ConnectionError("genius is down") + + monkeypatch.setattr(lyrics_api, "_client", + lambda: MagicMock(search_song=explode)) + + assert await lyrics_api.fetch("Bohemian Rhapsody") is None + + +async def test_the_artist_is_used_when_given(token, monkeypatch): + calls = [] + monkeypatch.setattr(lyrics_api, "_client", lambda: _recording_client(calls)) + + await lyrics_api.fetch("Bohemian Rhapsody", "Queen") + + assert calls == [("Bohemian Rhapsody", "Queen")] + + +async def test_a_bare_title_does_not_pass_an_empty_artist(token, monkeypatch): + """Genius matches worse against an empty artist than against none at all.""" + calls = [] + monkeypatch.setattr(lyrics_api, "_client", lambda: _recording_client(calls)) + + await lyrics_api.fetch("Bohemian Rhapsody") + + assert calls == [("Bohemian Rhapsody",)] + + +async def test_the_search_never_runs_on_the_event_loop(token, monkeypatch): + """ + lyricsgenius is blocking and synchronous. Calling it on the loop would freeze + every other guild's playback for the length of an HTTP round trip. + """ + threads = [] + + def record(*args): + threads.append(threading.get_ident()) + return FakeSong() + + monkeypatch.setattr(lyrics_api, "_client", lambda: MagicMock(search_song=record)) + + await lyrics_api.fetch("Bohemian Rhapsody") + + assert threads and threads[0] != threading.get_ident(), "the blocking call ran on the event loop thread"