From ffc70e411c63fab744a2cf489c410c4fae777ba1 Mon Sep 17 00:00:00 2001 From: Ismael Leon Date: Fri, 11 Sep 2026 21:50:13 -0600 Subject: [PATCH] Retry the login so a DNS blip no longer kills the bot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 2026-09-11 at 06:18 UTC an unattended-upgrade replaced glibc — which *is* the DNS resolver — and needrestart restarted the bot in the middle of that window. `getaddrinfo("discord.com")` returned EAI_AGAIN, the error propagated out of `main()`, and the process exited 1. Two starts died that way before a third one landed; each one sent an OnFailure alert to Discord. `Client.start()` is `login()` followed by `connect()`, and only the first half was exposed. `connect(reconnect=True)` already catches `aiohttp.ClientError`, `OSError`, `GatewayNotFound` and friends and retries with its own backoff, so a gateway that drops mid-song recovers on its own. `login()` has no retry at all, which is why a few seconds without a resolver was fatal. So `main()` now calls the two halves separately: the login is retried with backoff (5s, 15s, 30s, 60s, 120s), and the gateway is handed to discord.py untouched rather than wrapped in a second retry policy. Only failures that waiting can fix are retried. A `LoginFailure` or a `PrivilegedIntentsRequired` is a deployment mistake that will fail identically forever, and retrying it invites a rate-limit on the login endpoint on top; a 4xx is ours and stays ours. DNS, refused connections, timeouts and Discord's own 5xx are retried. After the delays run out the last error is re-raised, so a lasting outage still reaches systemd instead of leaving a bot that looks alive to `systemctl` and never trips OnFailure. Retrying the login is not as simple as calling it twice, and one of the tests here found out why. `HTTPClient.static_login` builds a new `aiohttp.ClientSession` on every call and abandons the previous one, so a naive retry leaks a session per attempt. Closing it is not enough either: the session is handed the HTTPClient's connector with aiohttp's default `connector_owner=True`, so closing the session closes that shared connector — and because `ClientSession.closed` reports the connector's state, every later session built on it is born closed and the next attempt dies with "Session is closed" instead of retrying. The connector is cleared alongside the session so a fresh pair is built. That bug was invisible to the tests using a fake HTTP client; it only appeared in the one that drives a real `discord.Client` with real aiohttp sessions against a resolver patched to fail. That test also asserts the invariant that matters on a 768 MB box: no aiohttp session is left open behind the retries. Verified end to end against the real API: with DNS made to fail twice and then recover, the login retries and gets through, and an invalid token is then rejected on the first attempt without a retry. Tests: 319 passing, up from 293. Closes #33 --- main.py | 4 +- tests/test_startup.py | 304 ++++++++++++++++++++++++++++++++++++++++++ utils/startup.py | 99 ++++++++++++++ 3 files changed, 406 insertions(+), 1 deletion(-) create mode 100644 tests/test_startup.py create mode 100644 utils/startup.py diff --git a/main.py b/main.py index a1e371f..8d394f2 100644 --- a/main.py +++ b/main.py @@ -8,6 +8,7 @@ from config import COGS, COMMAND_PREFIX, DISCORD_TOKEN from utils import errors from utils.help import build as build_help +from utils.startup import start as start_bot config.configure_logging() config.log_runtime() @@ -59,7 +60,8 @@ async def main(): log.info("Loaded cog: %s", cog) except Exception: log.exception("Failed to load cog: %s", cog) - await bot.start(DISCORD_TOKEN) + # Not bot.start(): the login half of it needs retrying. + await start_bot(bot, DISCORD_TOKEN) if __name__ == "__main__": diff --git a/tests/test_startup.py b/tests/test_startup.py new file mode 100644 index 0000000..7018bbb --- /dev/null +++ b/tests/test_startup.py @@ -0,0 +1,304 @@ +""" +Logging in survives a DNS outage. + +On 2026-09-11 an unattended-upgrade replaced glibc — which *is* the DNS +resolver — and needrestart restarted the bot in the middle of that window. +`getaddrinfo("discord.com")` returned EAI_AGAIN, `bot.start()` propagated it, +and the process exited 1. systemd restarted it twice before one attempt landed. + +Only the login half needs this. `Client.connect(reconnect=True)` already catches +`aiohttp.ClientError` and retries with backoff on its own; `Client.login()` has +no retry at all, which is why the failure was fatal. +""" + +import asyncio +import socket +from unittest.mock import MagicMock + +import aiohttp +import discord +import pytest +from aiohttp.client_exceptions import ClientConnectorDNSError + +from utils.startup import LOGIN_RETRY_DELAYS, login_with_retry, start + +TOKEN = "not-a-real-token" + + +def dns_failure() -> ClientConnectorDNSError: + """The exact exception that killed the bot on 2026-09-11 at 06:18 UTC.""" + return ClientConnectorDNSError( + MagicMock(host="discord.com", port=443, ssl=True), + socket.gaierror(-3, "Temporary failure in name resolution"), + ) + + +def http_error(status: int) -> discord.HTTPException: + response = MagicMock() + response.status = status + response.reason = "because" + return discord.HTTPException(response, {"message": "nope", "code": 0}) + + +class FakeHTTP: + """Stands in for ``bot.http``, recording how often its session was closed.""" + + def __init__(self) -> None: + self.closes = 0 + + async def close(self) -> None: + self.closes += 1 + + +class FakeBot: + """ + A client whose ``login`` replays a scripted list of outcomes. + + An entry that is an exception is raised; anything else counts as a + successful login. + """ + + def __init__(self, *outcomes) -> None: + self._outcomes = list(outcomes) + self.attempts = 0 + self.http = FakeHTTP() + self.calls: list[str] = [] + self.connect_kwargs: dict = {} + + async def login(self, token: str) -> None: + assert token == TOKEN + self.attempts += 1 + self.calls.append("login") + outcome = self._outcomes.pop(0) if self._outcomes else None + if isinstance(outcome, BaseException): + raise outcome + + async def connect(self, **kwargs) -> None: + self.calls.append("connect") + self.connect_kwargs = kwargs + + +@pytest.fixture +def slept(): + """Replaces the real sleep, recording the delays that were waited.""" + delays: list[float] = [] + + async def _sleep(seconds: float) -> None: + delays.append(seconds) + + _sleep.delays = delays + return _sleep + + +# -- the incident ------------------------------------------------------ + +async def test_a_dns_failure_is_retried_until_it_succeeds(slept): + bot = FakeBot(dns_failure(), dns_failure(), None) + + await login_with_retry(bot, TOKEN, sleep=slept) + + assert bot.attempts == 3, "it must keep trying while DNS is down" + + +async def test_a_dns_failure_no_longer_kills_the_process(slept): + """The regression itself: one blip used to propagate out of main().""" + bot = FakeBot(dns_failure(), None) + await login_with_retry(bot, TOKEN, sleep=slept) + + +async def test_it_waits_between_attempts_with_growing_delays(slept): + bot = FakeBot(dns_failure(), dns_failure(), dns_failure(), None) + + await login_with_retry(bot, TOKEN, sleep=slept) + + assert slept.delays == list(LOGIN_RETRY_DELAYS[:3]) + assert slept.delays == sorted(slept.delays), "delays must back off, not shrink" + + +async def test_the_stale_session_is_closed_before_each_retry(slept): + """ + ``HTTPClient.static_login`` builds a fresh ``aiohttp.ClientSession`` every + call and drops the previous one on the floor. Without closing it, each retry + leaks a session — which on a 768 MB box is exactly the kind of slow leak the + project brief forbids. + """ + bot = FakeBot(dns_failure(), dns_failure(), None) + + await login_with_retry(bot, TOKEN, sleep=slept) + + assert bot.http.closes == 2, "one close per discarded session" + + +async def test_nothing_is_closed_when_the_first_attempt_works(slept): + bot = FakeBot(None) + + await login_with_retry(bot, TOKEN, sleep=slept) + + assert bot.http.closes == 0 + assert slept.delays == [] + + +# -- what must NOT be retried ------------------------------------------ + +async def test_a_bad_token_fails_immediately(slept): + """Waiting cannot fix a wrong token, and retrying risks a login rate-limit.""" + bot = FakeBot(discord.LoginFailure("Improper token has been passed.")) + + with pytest.raises(discord.LoginFailure): + await login_with_retry(bot, TOKEN, sleep=slept) + + assert bot.attempts == 1 + assert slept.delays == [] + + +async def test_a_client_side_http_error_is_not_retried(slept): + """A 4xx is our fault and stays our fault.""" + bot = FakeBot(http_error(403)) + + with pytest.raises(discord.HTTPException): + await login_with_retry(bot, TOKEN, sleep=slept) + + assert bot.attempts == 1 + + +async def test_a_discord_outage_is_retried(slept): + """A 5xx is Discord's problem and usually passes.""" + bot = FakeBot(http_error(503), None) + + await login_with_retry(bot, TOKEN, sleep=slept) + + assert bot.attempts == 2 + + +# -- giving up --------------------------------------------------------- + +async def test_it_gives_up_and_reraises_when_the_outage_outlasts_the_retries(slept): + """ + systemd is still the backstop. Retrying forever would leave a bot that looks + alive to `systemctl` while never being usable, and never trips OnFailure. + """ + last = dns_failure() + bot = FakeBot(*[dns_failure() for _ in LOGIN_RETRY_DELAYS], last) + + with pytest.raises(ClientConnectorDNSError) as caught: + await login_with_retry(bot, TOKEN, sleep=slept) + + assert caught.value is last, "the final failure is what the operator sees" + assert bot.attempts == len(LOGIN_RETRY_DELAYS) + 1 + assert len(slept.delays) == len(LOGIN_RETRY_DELAYS) + + +async def test_the_retry_budget_is_survivable_but_not_endless(): + """Long enough to outlast a package upgrade, short enough to surface.""" + assert 60 <= sum(LOGIN_RETRY_DELAYS) <= 600 + assert len(LOGIN_RETRY_DELAYS) >= 3 + + +# -- transient classification ------------------------------------------- + +@pytest.mark.parametrize("error", [ + dns_failure(), + OSError(101, "Network is unreachable"), + asyncio.TimeoutError(), + discord.GatewayNotFound(), + http_error(500), + http_error(502), +]) +async def test_transient_failures_are_retried(slept, error): + bot = FakeBot(error, None) + await login_with_retry(bot, TOKEN, sleep=slept) + assert bot.attempts == 2 + + +@pytest.mark.parametrize("error", [ + discord.LoginFailure("bad token"), + discord.PrivilegedIntentsRequired(shard_id=None), + http_error(400), + http_error(401), + TypeError("expected token to be a str"), +]) +async def test_permanent_failures_are_raised_at_once(slept, error): + bot = FakeBot(error) + with pytest.raises(type(error)): + await login_with_retry(bot, TOKEN, sleep=slept) + assert bot.attempts == 1 + + +# -- handing off to the gateway ---------------------------------------- + +async def test_start_logs_in_before_connecting(slept): + bot = FakeBot(None) + + await start(bot, TOKEN, sleep=slept) + + assert bot.calls == ["login", "connect"] + + +async def test_start_lets_discord_py_own_the_gateway_reconnects(slept): + """ + ``connect(reconnect=True)`` already retries the gateway with its own + backoff. Wrapping it in ours as well would stack two retry policies on the + same failure and double every wait. + """ + bot = FakeBot(None) + + await start(bot, TOKEN, sleep=slept) + + assert bot.connect_kwargs == {"reconnect": True} + + +async def test_start_retries_the_login_before_connecting(slept): + bot = FakeBot(dns_failure(), None) + + await start(bot, TOKEN, sleep=slept) + + assert bot.calls == ["login", "login", "connect"] + + +async def test_start_never_connects_when_the_login_never_lands(slept): + bot = FakeBot(*[dns_failure() for _ in range(len(LOGIN_RETRY_DELAYS) + 1)]) + + with pytest.raises(ClientConnectorDNSError): + await start(bot, TOKEN, sleep=slept) + + assert "connect" not in bot.calls + + +# -- against a real client, with real aiohttp sessions ------------------ + +async def test_a_real_login_retry_leaves_no_unclosed_session(monkeypatch): + """ + The leak check, with nothing faked but DNS. + + Every ``static_login`` opens a real ``aiohttp.ClientSession``. This drives + three real attempts against a resolver that always fails and asserts that no + session is left open behind them — the invariant the FakeHTTP test can only + describe. + """ + import gc + import socket as socket_module + + def no_dns(*args, **kwargs): + raise socket_module.gaierror(-3, "Temporary failure in name resolution") + + monkeypatch.setattr(socket_module, "getaddrinfo", no_dns) + + def live_sessions() -> set[int]: + return {id(o) for o in gc.get_objects() + if isinstance(o, aiohttp.ClientSession) and not o.closed} + + before = live_sessions() + client = discord.Client(intents=discord.Intents.none()) + try: + with pytest.raises(aiohttp.ClientError): + await login_with_retry(client, TOKEN, delays=(0.0, 0.0), sleep=_no_wait) + finally: + await client.http.close() # the final attempt's session + await client.close() + + gc.collect() + assert live_sessions() - before == set(), "a retry left an aiohttp session open" + + +async def _no_wait(seconds: float) -> None: + """A sleep that does not.""" diff --git a/utils/startup.py b/utils/startup.py new file mode 100644 index 0000000..bde5e44 --- /dev/null +++ b/utils/startup.py @@ -0,0 +1,99 @@ +""" +Logging in to Discord without dying on a passing network failure. + +``Client.start()`` is ``login()`` followed by ``connect()``. Only the first half +needs anything from us: ``connect(reconnect=True)`` already catches +``aiohttp.ClientError``, ``OSError`` and friends and retries with its own +backoff, so a gateway that drops mid-song recovers on its own. ``login()`` has +no retry whatsoever, so a resolver that is down for the few seconds the bot +happens to be starting takes the whole process with it. + +That is not hypothetical: on 2026-09-11 an unattended-upgrade replaced glibc — +which *is* the DNS resolver — and needrestart restarted the bot inside that +window. Two starts died on ``EAI_AGAIN`` before a third one landed. +""" + +import asyncio +import logging + +import aiohttp +import discord + +log = logging.getLogger("loopify.startup") + +# Waits between login attempts, in seconds. The total has to outlast a package +# upgrade restarting the resolver, but stay short enough that a deployment which +# is genuinely broken still fails and trips systemd's OnFailure alert rather +# than sitting there looking alive. +LOGIN_RETRY_DELAYS = (5.0, 15.0, 30.0, 60.0, 120.0) + + +def is_transient(error: BaseException) -> bool: + """ + Whether waiting could plausibly fix this. + + A wrong token or a missing privileged intent is a deployment mistake: it + will fail identically forever, and hammering the login endpoint over it + invites a rate-limit on top. Discord's own 5xx, a refused connection and a + name that will not resolve are all things that pass. + """ + if isinstance(error, (discord.LoginFailure, discord.PrivilegedIntentsRequired)): + return False + if isinstance(error, discord.HTTPException): + return error.status >= 500 + return isinstance(error, (OSError, aiohttp.ClientError, + asyncio.TimeoutError, discord.GatewayNotFound)) + + +async def _discard_session(http) -> None: + """ + Release the aiohttp session a failed login left behind. + + Two details make this less obvious than it looks: + + * ``HTTPClient.static_login`` builds a new ``ClientSession`` on every call + and abandons the previous one, so without closing it each retry leaks a + session. + * That session is handed the HTTPClient's connector, and aiohttp's default + ``connector_owner=True`` means closing the session closes the *shared* + connector too. ``ClientSession.closed`` is then True for every later + session built on it, so the next attempt dies with "Session is closed" + rather than retrying. Clearing the connector makes ``static_login`` build + a fresh pair. + """ + await http.close() + http.connector = discord.utils.MISSING + + +async def login_with_retry(bot, token: str, *, + delays: tuple[float, ...] = LOGIN_RETRY_DELAYS, + sleep=asyncio.sleep) -> None: + """ + Log ``bot`` in, retrying while the failure looks like weather. + + Raises the last error once the delays run out, so a lasting outage still + reaches systemd instead of being swallowed. + """ + for delay in (*delays, None): # the trailing None is the last attempt + try: + await bot.login(token) + return + except Exception as error: + if delay is None or not is_transient(error): + raise + await _discard_session(bot.http) + log.warning("Login failed (%s: %s) — retrying in %.0fs", + type(error).__name__, error, delay) + await sleep(delay) + + +async def start(bot, token: str, **retry_options) -> None: + """ + Bring the bot online: a retried login, then discord.py's own gateway loop. + + The split matters. ``connect(reconnect=True)`` already retries gateway drops + with its own backoff, so it is handed over untouched — wrapping it here too + would stack two retry policies on one failure. + """ + await login_with_retry(bot, token, **retry_options) + await bot.connect(reconnect=True)