OPS-004. bot.run() qui rend la main signifie que la connexion est perdue pour de bon : discord.py se reconnecte seul pour tout ce qui est recuperable. Ce qui se passait ensuite, c'etait rien. Le fil se terminait, bot_thread restait non nul donc start_bot n'en relancerait jamais un autre, et l'application web continuait a servir des pages pendant que toutes les notifications et tous les rappels quotidiens s'etaient arretes. La vague E/F avait fait la moitie visibilite (OPS-012), pas la moitie reprise. La panne pouvait durer des semaines. Deux fins sont distinguees, parce que reessayer ne sert que pour l'une. Un jeton rejete ou un intent privilegie manquant est une erreur de configuration : boucler dessus ne fait que marteler le point de connexion de Discord, ce qui est la maniere d'obtenir une limitation ou un bannissement. Le reste est traite comme une panne et reessaye avec une temporisation exponentielle, plafonnee a cinq minutes, tant que le processus vit. La temporisation se reinitialise apres une connexion qui a dure. Sinon un bot qui tourne un mois puis decroche attend cinq minutes avant son premier essai, fort d'un incident depuis longtemps termine. Deux consequences de conception. Une instance neuve a chaque tentative : discord.py ferme le client quand run() rend la main, et un client ferme ne se reconnecte pas -- le reutiliser transforme une reprise en fil qui tourne sur une exception. Et donc la file de messages passe au niveau module, sinon chaque redemarrage emporterait les notifications en attente. Co-Authored-By: Claude Opus 5 <[email protected]>
351 lines
13 KiB
Python
351 lines
13 KiB
Python
"""The bot's on-disk state and what /health says about it.
|
|
|
|
OPS-005/006/007/012. Four defects that shared a shape: the bot could stop
|
|
working correctly, and nothing anywhere said so.
|
|
|
|
- a truncated pending file was logged and then silently replaced by an
|
|
empty dict, so every in-flight Discord reaction stopped having an
|
|
effect;
|
|
- the file was written in place, so any interruption produced exactly
|
|
that truncated file;
|
|
- entries were never removed unless someone reacted, so the file grew
|
|
for ever;
|
|
- when the bot thread died, the site kept serving pages and every
|
|
notification stopped, invisibly.
|
|
|
|
These tests drive the bot object directly. Nothing here talks to Discord.
|
|
"""
|
|
|
|
import json
|
|
import logging
|
|
import os
|
|
import time
|
|
|
|
import pytest
|
|
|
|
from app import discord_bot
|
|
from app.discord_bot import PENDING_MAX_AGE_DAYS, TeamTryoutsBot
|
|
|
|
|
|
@pytest.fixture
|
|
def bot(monkeypatch, tmp_path):
|
|
"""A bot whose pending file lives in a throwaway directory."""
|
|
pending = tmp_path / 'discord_pending.json'
|
|
monkeypatch.setattr(discord_bot, 'PENDING_FILE', str(pending))
|
|
instance = TeamTryoutsBot.__new__(TeamTryoutsBot)
|
|
instance.pending_requests = {}
|
|
return instance, pending
|
|
|
|
|
|
class TestSaving:
|
|
def test_a_saved_file_reloads(self, bot):
|
|
instance, pending = bot
|
|
instance.pending_requests = {123: {'type': 'one_on_one', 'id': 5}}
|
|
instance._save_pending()
|
|
|
|
instance.pending_requests = {}
|
|
instance._load_pending()
|
|
|
|
assert instance.pending_requests == {123: {'type': 'one_on_one', 'id': 5}}
|
|
|
|
def test_no_temporary_file_is_left_behind(self, bot):
|
|
instance, pending = bot
|
|
instance.pending_requests = {1: {'type': 'one_on_one', 'id': 1}}
|
|
instance._save_pending()
|
|
|
|
leftovers = [name for name in os.listdir(pending.parent) if name.startswith('.pending-')]
|
|
assert leftovers == []
|
|
|
|
def test_the_previous_content_survives_a_failed_write(self, bot, monkeypatch):
|
|
"""Writing in place was the whole problem: an interruption left a
|
|
half-written file that the loader could not read."""
|
|
instance, pending = bot
|
|
instance.pending_requests = {1: {'type': 'one_on_one', 'id': 1}}
|
|
instance._save_pending()
|
|
|
|
def _explode(*args, **kwargs):
|
|
raise OSError('disk full')
|
|
|
|
monkeypatch.setattr(discord_bot.tempfile, 'NamedTemporaryFile', _explode)
|
|
instance.pending_requests = {2: {'type': 'one_on_one', 'id': 2}}
|
|
instance._save_pending()
|
|
|
|
assert json.loads(pending.read_text(encoding='utf-8')) == {
|
|
'1': {'type': 'one_on_one', 'id': 1}
|
|
}
|
|
|
|
|
|
class TestLoading:
|
|
def test_a_missing_file_starts_empty(self, bot):
|
|
instance, _pending = bot
|
|
instance._load_pending()
|
|
assert instance.pending_requests == {}
|
|
|
|
def test_a_truncated_file_is_moved_aside_not_overwritten(self, bot, caplog):
|
|
"""Losing the mapping is bad; losing it *and* the evidence is worse."""
|
|
instance, pending = bot
|
|
pending.write_text('{"123": {"type": "one_on', encoding='utf-8')
|
|
|
|
instance._load_pending()
|
|
|
|
assert instance.pending_requests == {}
|
|
quarantined = [name for name in os.listdir(pending.parent) if '.corrupt-' in name]
|
|
assert len(quarantined) == 1
|
|
assert not pending.exists()
|
|
|
|
def test_a_json_document_that_is_not_an_object_is_refused(self, bot):
|
|
instance, pending = bot
|
|
pending.write_text('[1, 2, 3]', encoding='utf-8')
|
|
|
|
instance._load_pending()
|
|
|
|
assert instance.pending_requests == {}
|
|
assert any('.corrupt-' in name for name in os.listdir(pending.parent))
|
|
|
|
def test_stale_entries_are_dropped(self, bot):
|
|
instance, pending = bot
|
|
old = time.time() - (PENDING_MAX_AGE_DAYS + 1) * 86400
|
|
pending.write_text(
|
|
json.dumps(
|
|
{
|
|
'1': {'type': 'one_on_one', 'id': 1, 'created_at': old},
|
|
'2': {'type': 'one_on_one', 'id': 2, 'created_at': time.time()},
|
|
}
|
|
),
|
|
encoding='utf-8',
|
|
)
|
|
|
|
instance._load_pending()
|
|
|
|
assert list(instance.pending_requests) == [2]
|
|
|
|
def test_the_purge_is_written_back(self, bot):
|
|
instance, pending = bot
|
|
old = time.time() - (PENDING_MAX_AGE_DAYS + 1) * 86400
|
|
pending.write_text(
|
|
json.dumps({'1': {'type': 'one_on_one', 'id': 1, 'created_at': old}}),
|
|
encoding='utf-8',
|
|
)
|
|
|
|
instance._load_pending()
|
|
|
|
assert json.loads(pending.read_text(encoding='utf-8')) == {}
|
|
|
|
def test_an_entry_without_a_timestamp_is_kept(self, bot):
|
|
"""Records written before this field existed. Dropping them would
|
|
be guessing at their age."""
|
|
instance, pending = bot
|
|
pending.write_text(json.dumps({'1': {'type': 'one_on_one', 'id': 1}}), encoding='utf-8')
|
|
|
|
instance._load_pending()
|
|
|
|
assert list(instance.pending_requests) == [1]
|
|
|
|
|
|
class TestHealthReporting:
|
|
def test_the_status_shape_is_stable(self, monkeypatch):
|
|
"""An external probe reads these keys; they are the contract.
|
|
|
|
`reminders_scheduled` joined them with ARCH-008. The other four can
|
|
all read green while the 18:00 job never got scheduled — the thread
|
|
is alive, the gateway is up, and no reminder is ever sent. Adding a
|
|
key is safe for a probe that looks at the ones it knows; removing or
|
|
renaming one is not.
|
|
"""
|
|
monkeypatch.setattr(discord_bot, 'bot_thread', None)
|
|
monkeypatch.setattr(discord_bot, 'bot_instance', None)
|
|
|
|
status = discord_bot.bot_status()
|
|
|
|
assert set(status) == {
|
|
'configured',
|
|
'running',
|
|
'connected',
|
|
'pending',
|
|
'reminders_scheduled',
|
|
}
|
|
assert status['running'] is False
|
|
assert status['connected'] is False
|
|
assert status['reminders_scheduled'] is False
|
|
|
|
def test_a_dead_thread_reads_as_not_running(self, monkeypatch):
|
|
class DeadThread:
|
|
@staticmethod
|
|
def is_alive():
|
|
return False
|
|
|
|
monkeypatch.setattr(discord_bot, 'bot_thread', DeadThread())
|
|
monkeypatch.setattr(discord_bot, 'bot_instance', None)
|
|
|
|
assert discord_bot.bot_status()['running'] is False
|
|
|
|
def test_health_reports_the_bot_when_it_is_enabled(self, app):
|
|
"""The suite runs with the bot disabled, so this builds an app that
|
|
claims otherwise rather than starting anything."""
|
|
app.config['ENABLE_DISCORD_BOT'] = True
|
|
|
|
payload = app.test_client().get('/health').get_json()
|
|
|
|
assert 'discord_bot' in payload
|
|
assert payload['status'] == 'healthy'
|
|
|
|
def test_health_stays_quiet_when_the_bot_is_off(self, app, client):
|
|
payload = client.get('/health').get_json()
|
|
|
|
assert 'discord_bot' not in payload
|
|
|
|
|
|
class TestTheSupervisorRestartsTheBot:
|
|
"""OPS-004 — nothing used to restart it.
|
|
|
|
`bot.run()` returning means the connection is gone for good; discord.py
|
|
reconnects on its own for anything recoverable. The thread then ended,
|
|
`bot_thread` stayed non-None so `start_bot` would never start another,
|
|
and the web application kept serving pages while every notification and
|
|
every daily reminder had stopped. The outage could last weeks.
|
|
"""
|
|
|
|
@pytest.fixture
|
|
def supervisor(self, monkeypatch):
|
|
"""Drive supervise_bot without a network, a token or a real client.
|
|
|
|
Returns a recorder holding the sequence of fake bots that were built
|
|
and how long the supervisor waited between them.
|
|
"""
|
|
|
|
class Recorder:
|
|
def __init__(self):
|
|
self.built = []
|
|
self.waits = []
|
|
self.outcomes = []
|
|
|
|
recorder = Recorder()
|
|
|
|
class FakeBot:
|
|
def __init__(self, outcome):
|
|
self.outcome = outcome
|
|
|
|
def run(self, _token):
|
|
# Returning is what discord.py does on a clean disconnection;
|
|
# raising is the other half of what the supervisor separates.
|
|
if isinstance(self.outcome, Exception):
|
|
raise self.outcome
|
|
|
|
def fake_get_bot(flask_app=None):
|
|
outcome = recorder.outcomes.pop(0) if recorder.outcomes else None
|
|
bot = FakeBot(outcome)
|
|
recorder.built.append(bot)
|
|
return bot
|
|
|
|
def fake_wait(seconds):
|
|
recorder.waits.append(seconds)
|
|
# False means "not asked to stop", so the loop continues. The
|
|
# test bounds the run by emptying `outcomes` and setting the
|
|
# event, never by sleeping.
|
|
return recorder.stop_after_waits <= len(recorder.waits)
|
|
|
|
recorder.stop_after_waits = 1
|
|
monkeypatch.setattr(discord_bot, 'get_bot', fake_get_bot)
|
|
monkeypatch.setattr(discord_bot, 'DISCORD_BOT_TOKEN', 'token-not-real')
|
|
monkeypatch.setattr(discord_bot._stop_bot, 'wait', fake_wait)
|
|
monkeypatch.setattr(discord_bot, 'bot_instance', None)
|
|
return recorder
|
|
|
|
def test_a_disconnection_builds_a_new_bot(self, supervisor, monkeypatch):
|
|
"""A fresh instance, not the old one: discord.py closes the client
|
|
when run() returns and a closed client will not log in again."""
|
|
supervisor.stop_after_waits = 2
|
|
|
|
discord_bot.supervise_bot()
|
|
|
|
assert len(supervisor.built) == 2, 'the supervisor did not try again'
|
|
assert supervisor.built[0] is not supervisor.built[1]
|
|
|
|
def test_the_wait_grows_between_attempts(self, supervisor):
|
|
supervisor.stop_after_waits = 4
|
|
|
|
discord_bot.supervise_bot()
|
|
|
|
assert supervisor.waits == [5, 10, 20, 40]
|
|
|
|
def test_the_wait_is_capped(self, supervisor):
|
|
supervisor.stop_after_waits = 12
|
|
|
|
discord_bot.supervise_bot()
|
|
|
|
assert max(supervisor.waits) == discord_bot.BOT_RESTART_MAX_DELAY_SECONDS
|
|
assert supervisor.waits[-1] == discord_bot.BOT_RESTART_MAX_DELAY_SECONDS
|
|
|
|
def test_a_rejected_token_is_not_retried(self, supervisor, caplog):
|
|
"""Retrying a bad token accomplishes nothing except hammering the
|
|
login endpoint, which is how an application gets rate-limited."""
|
|
from discord import LoginFailure
|
|
|
|
supervisor.outcomes = [LoginFailure('Improper token has been passed.')]
|
|
supervisor.stop_after_waits = 99
|
|
|
|
discord_bot.supervise_bot()
|
|
|
|
assert supervisor.waits == [], 'a configuration error must not be retried'
|
|
assert len(supervisor.built) == 1
|
|
|
|
def test_a_rejected_token_says_it_is_a_configuration_error(self, supervisor, caplog):
|
|
from discord import LoginFailure
|
|
|
|
supervisor.outcomes = [LoginFailure('Improper token has been passed.')]
|
|
|
|
package_logger = logging.getLogger('app')
|
|
previous = package_logger.propagate
|
|
package_logger.propagate = True
|
|
try:
|
|
with caplog.at_level(logging.ERROR, logger='app.discord_bot'):
|
|
discord_bot.supervise_bot()
|
|
finally:
|
|
package_logger.propagate = previous
|
|
|
|
assert 'configuration error' in caplog.text
|
|
assert 'not an outage' in caplog.text
|
|
|
|
def test_an_unexpected_error_is_retried(self, supervisor):
|
|
"""The other branch: anything that is not a login refusal is treated
|
|
as an outage."""
|
|
supervisor.outcomes = [RuntimeError('gateway exploded')]
|
|
supervisor.stop_after_waits = 2
|
|
|
|
discord_bot.supervise_bot()
|
|
|
|
assert len(supervisor.built) == 2
|
|
|
|
def test_a_connection_that_lasted_resets_the_backoff(self, supervisor, monkeypatch):
|
|
"""Otherwise a bot that ran for a month and then dropped waits five
|
|
minutes before its first retry, on the strength of an incident that
|
|
is long over."""
|
|
clock = {'now': 0.0}
|
|
|
|
def fake_monotonic():
|
|
# Every run() call is deemed to have lasted longer than the
|
|
# reset threshold.
|
|
clock['now'] += discord_bot.BOT_RESTART_RESET_SECONDS + 1
|
|
return clock['now']
|
|
|
|
monkeypatch.setattr(discord_bot.time, 'monotonic', fake_monotonic)
|
|
supervisor.stop_after_waits = 3
|
|
|
|
discord_bot.supervise_bot()
|
|
|
|
assert supervisor.waits == [5, 5, 5], 'the back-off should not have grown'
|
|
|
|
|
|
class TestTheQueueSurvivesARestart:
|
|
def test_it_is_shared_by_every_instance(self, monkeypatch):
|
|
"""A queue living on the bot object would be discarded with it on
|
|
each restart, taking every pending notification along."""
|
|
monkeypatch.setattr(discord_bot, 'bot_instance', None)
|
|
first = discord_bot.get_bot()
|
|
monkeypatch.setattr(discord_bot, 'bot_instance', None)
|
|
second = discord_bot.get_bot()
|
|
|
|
assert first is not second
|
|
assert first.message_queue is second.message_queue
|
|
assert first.message_queue is discord_bot.message_queue
|