fix(bot): une reaction qui echoue le dit, au lieu de se taire
Chaque gestionnaire de discord_bot.py enveloppait tout son corps dans un except Exception qui journalisait et poursuivait. Un refus de la base, une boite de reception fermee et une faute de frappe dans ce module produisaient la meme ligne, et la personne qui avait clique n'apprenait rien dans aucun des trois cas. Le defaut que l'audit citait en exemple : dans handle_attendance_confirm, le commit et le message "Your attendance has been confirmed!" etaient dans le meme bloc protege. Si le commit levait, rien n'etait envoye et rien n'etait signale. La reaction devenait indiscernable d'un bot arrete. Trois familles, trois reponses. La base refuse : rollback, l'entree pending est conservee pour que la reaction reste reessayable, et la personne est prevenue que rien n'a ete enregistre. Discord est injoignable : apres un commit c'est du meilleur effort, un DM qui rebondit ne defait pas une decision prise. Tout le reste est un defaut et remonte, jusqu'a on_error, qui est ajoute parce que discord.py journalise sur le logger 'discord' que configure_logging ne collecte pas. Trouve en appliquant : deux reactions sur le meme message passent toutes deux le test d'appartenance puis s'attendent sur deux await, et la perdante levait un KeyError qui se lisait comme une erreur sans consequence ; un fetch_channel en echec renvoyait sans un mot, donc un clic sans effet et sans trace ; un start_scheduler en echec supprime tous les rappels a jamais et laissait trois cles de /health au vert, d'ou reminders_scheduled. L'ordre des etapes apres le commit est desormais fixe : oublier l'entree pending avant les messages, sinon un DM rebondi laisse une demande deja approuvee reactivable une seconde fois. Les tests ont ete verifies par mutation du code de production. La deuxieme mutation a trouve une faiblesse dans le test lui-meme, qui ne regardait que le premier message emis. QUA-004 (roadmap) / ARCH-008 (constats). Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
GGThed
+567
-292
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,430 @@
|
||||
"""Three families of failure, and the three different answers they get.
|
||||
|
||||
ARCH-008 / QUA-004. Every handler in `discord_bot.py` used to wrap its
|
||||
whole body in one `except Exception` that logged a line and returned, so
|
||||
these three all looked the same from the outside and two of them looked the
|
||||
same from the inside:
|
||||
|
||||
- the database refused the write — nothing happened, and the person who
|
||||
clicked has to be told, because a silent reaction is indistinguishable
|
||||
from a bot that is not running;
|
||||
- Discord could not be reached — after a commit, the thing is recorded
|
||||
whether or not the message got through, and must stay recorded;
|
||||
- something else went wrong — a defect, which has to reach a traceback
|
||||
rather than be flattened into "Error handling attendance confirmation".
|
||||
|
||||
The coroutines are driven with `asyncio.run` rather than pytest-asyncio,
|
||||
which the project does not depend on — same convention as
|
||||
`test_discord_delivery.py`.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import logging
|
||||
from datetime import date, time
|
||||
|
||||
import pytest
|
||||
from discord import HTTPException
|
||||
from sqlalchemy.exc import OperationalError
|
||||
|
||||
from app import discord_bot
|
||||
from app.discord_bot import TeamTryoutsBot
|
||||
|
||||
|
||||
class FakeChannel:
|
||||
"""The DM channel a reaction came from. Records what it was told to say."""
|
||||
|
||||
def __init__(self, raises=None):
|
||||
self.raises = raises
|
||||
self.sent = []
|
||||
|
||||
async def send(self, message):
|
||||
if self.raises is not None:
|
||||
raise self.raises
|
||||
self.sent.append(message)
|
||||
return object()
|
||||
|
||||
|
||||
class FakeReactor:
|
||||
"""The Discord account that clicked. Only its snowflake is ever read."""
|
||||
|
||||
def __init__(self, uid):
|
||||
self.id = uid
|
||||
|
||||
|
||||
def http_error():
|
||||
class _Response:
|
||||
status = 500
|
||||
reason = 'test'
|
||||
|
||||
return HTTPException(_Response(), 'Internal Server Error')
|
||||
|
||||
|
||||
def database_error():
|
||||
"""A refusal that is not an IntegrityError.
|
||||
|
||||
Deliberately: the handlers catch `SQLAlchemyError`, and picking the
|
||||
subclass a constraint violation produces would let a narrower catch
|
||||
pass this suite while still dropping a lost connection on the floor.
|
||||
"""
|
||||
return OperationalError('SELECT 1', {}, Exception('server closed the connection'))
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def logs(caplog):
|
||||
"""Let the bot's records reach caplog.
|
||||
|
||||
`configure_logging` sets propagate = False on the 'app' logger, and
|
||||
caplog's handler sits on the root. Without this the assertions below
|
||||
read an empty log and pass for the wrong reason — the trap recorded in
|
||||
the wave G notes.
|
||||
"""
|
||||
package_logger = logging.getLogger('app')
|
||||
previous = package_logger.propagate
|
||||
package_logger.propagate = True
|
||||
caplog.set_level(logging.INFO, logger='app.discord_bot')
|
||||
yield caplog
|
||||
package_logger.propagate = previous
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def bot():
|
||||
"""A bot with the state the reaction handlers touch, and nothing else.
|
||||
|
||||
`__new__` because the constructor builds a real discord.py client.
|
||||
`_save_pending` is replaced: the real one writes `discord_pending.json`
|
||||
at the repository root.
|
||||
"""
|
||||
instance = TeamTryoutsBot.__new__(TeamTryoutsBot)
|
||||
instance.flask_app = None
|
||||
instance.pending_requests = {}
|
||||
instance._user_cache = {}
|
||||
instance.reminders_scheduled = False
|
||||
instance.saves = 0
|
||||
|
||||
def _save():
|
||||
instance.saves += 1
|
||||
|
||||
instance._save_pending = _save
|
||||
return instance
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def participation(db, make_user):
|
||||
"""A player on a match, plus the pending message asking them to confirm.
|
||||
|
||||
Returns (participation row id, the player's Discord snowflake).
|
||||
"""
|
||||
from app.models import Match, MatchParticipant, Tryout
|
||||
|
||||
snowflake = '4242'
|
||||
player_id = make_user('player', discord_user_id=snowflake)
|
||||
admin_id = make_user('admin')
|
||||
|
||||
tryout = Tryout(title='Sélection', game='valorant', date=date(2026, 9, 1), created_by=admin_id)
|
||||
db.session.add(tryout)
|
||||
db.session.flush()
|
||||
|
||||
match = Match(
|
||||
title='Finale',
|
||||
date=date(2026, 9, 2),
|
||||
start_time=time(18, 0),
|
||||
created_by=admin_id,
|
||||
tryout_id=tryout.id,
|
||||
match_type='intra',
|
||||
)
|
||||
db.session.add(match)
|
||||
db.session.flush()
|
||||
|
||||
row = MatchParticipant(match_id=match.id, player_id=player_id)
|
||||
db.session.add(row)
|
||||
db.session.commit()
|
||||
return row.id, snowflake
|
||||
|
||||
|
||||
def _pending(bot, message_id, row_id):
|
||||
bot.pending_requests[message_id] = {
|
||||
'type': 'schedule_addition',
|
||||
'id': row_id,
|
||||
'event_type': 'match',
|
||||
}
|
||||
|
||||
|
||||
def _confirmed(row_id):
|
||||
from app.models import MatchParticipant
|
||||
|
||||
return MatchParticipant.query.get(row_id).attendance_confirmed
|
||||
|
||||
|
||||
class TestTheDatabaseRefusedTheWrite:
|
||||
def test_the_happy_path_records_the_confirmation(self, bot, db, participation, logs):
|
||||
"""The premise of every test below. Without it they could all pass
|
||||
against a handler that never records anything at all."""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
channel = FakeChannel()
|
||||
|
||||
asyncio.run(bot.handle_attendance_confirm(FakeReactor(snowflake), 7, row_id, channel))
|
||||
|
||||
assert _confirmed(row_id) is True
|
||||
assert channel.sent == ["✅ Your attendance has been confirmed!"]
|
||||
assert 7 not in bot.pending_requests
|
||||
|
||||
def test_a_refused_commit_tells_the_player_instead_of_going_quiet(
|
||||
self, bot, db, participation, monkeypatch, logs
|
||||
):
|
||||
"""The defect ARCH-008 was written about.
|
||||
|
||||
The success message sat inside the same blanket handler as the
|
||||
commit, so a refused write produced no message at all — the reaction
|
||||
looked ignored, which is what a stopped bot looks like too.
|
||||
"""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
channel = FakeChannel()
|
||||
|
||||
def refuse():
|
||||
raise database_error()
|
||||
|
||||
monkeypatch.setattr(db.session, 'commit', refuse)
|
||||
|
||||
asyncio.run(bot.handle_attendance_confirm(FakeReactor(snowflake), 7, row_id, channel))
|
||||
|
||||
assert channel.sent, 'silence is the one answer this must never give'
|
||||
assert 'Nothing was recorded' in channel.sent[0]
|
||||
# Every message, not just the first: an earlier version of this test
|
||||
# looked at sent[0] alone, and a handler that reported the refusal
|
||||
# and then sent "✅ Your attendance has been confirmed!" anyway
|
||||
# passed it.
|
||||
assert not any("✅" in message for message in channel.sent), (
|
||||
'a failed write must not be followed by a success message'
|
||||
)
|
||||
|
||||
def test_a_refused_commit_leaves_the_row_untouched(
|
||||
self, bot, db, participation, monkeypatch, logs
|
||||
):
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
|
||||
def refuse():
|
||||
raise database_error()
|
||||
|
||||
monkeypatch.setattr(db.session, 'commit', refuse)
|
||||
asyncio.run(bot.handle_attendance_confirm(FakeReactor(snowflake), 7, row_id, FakeChannel()))
|
||||
|
||||
monkeypatch.undo()
|
||||
assert _confirmed(row_id) is False, (
|
||||
'the handler must roll back; leaving the change pending in the '
|
||||
'session hands it to whatever commits next'
|
||||
)
|
||||
|
||||
def test_a_refused_commit_keeps_the_reaction_answerable(
|
||||
self, bot, db, participation, monkeypatch, logs
|
||||
):
|
||||
"""Dropping the pending entry on a failed write would make the
|
||||
second attempt do nothing, silently, forever."""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
|
||||
def refuse():
|
||||
raise database_error()
|
||||
|
||||
monkeypatch.setattr(db.session, 'commit', refuse)
|
||||
asyncio.run(bot.handle_attendance_confirm(FakeReactor(snowflake), 7, row_id, FakeChannel()))
|
||||
|
||||
assert 7 in bot.pending_requests
|
||||
|
||||
def test_the_refusal_reaches_the_log_with_its_traceback(
|
||||
self, bot, db, participation, monkeypatch, logs
|
||||
):
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
|
||||
def refuse():
|
||||
raise database_error()
|
||||
|
||||
monkeypatch.setattr(db.session, 'commit', refuse)
|
||||
with logs.at_level(logging.ERROR, logger='app.discord_bot'):
|
||||
asyncio.run(
|
||||
bot.handle_attendance_confirm(FakeReactor(snowflake), 7, row_id, FakeChannel())
|
||||
)
|
||||
|
||||
assert any(record.exc_info for record in logs.records), (
|
||||
'a database refusal is diagnosed from the driver error, which only '
|
||||
'the traceback carries'
|
||||
)
|
||||
|
||||
def test_a_declined_match_that_cannot_be_deleted_says_so(
|
||||
self, bot, db, participation, monkeypatch, logs
|
||||
):
|
||||
"""The decline path deletes a row, which is exactly what a foreign
|
||||
key refuses. Same answer as the confirm path."""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
channel = FakeChannel()
|
||||
|
||||
def refuse():
|
||||
raise database_error()
|
||||
|
||||
monkeypatch.setattr(db.session, 'commit', refuse)
|
||||
asyncio.run(bot.handle_attendance_decline(FakeReactor(snowflake), 7, row_id, channel))
|
||||
|
||||
assert channel.sent and 'Nothing was recorded' in channel.sent[0]
|
||||
assert 7 in bot.pending_requests
|
||||
|
||||
|
||||
class TestDiscordCouldNotBeReached:
|
||||
def test_a_bounced_message_does_not_undo_the_record(self, bot, db, participation, logs):
|
||||
"""The confirmation is committed before the message goes out. A DM
|
||||
that bounces costs the player a message, not their attendance."""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
|
||||
asyncio.run(
|
||||
bot.handle_attendance_confirm(
|
||||
FakeReactor(snowflake), 7, row_id, FakeChannel(raises=http_error())
|
||||
)
|
||||
)
|
||||
|
||||
assert _confirmed(row_id) is True
|
||||
assert 7 not in bot.pending_requests, (
|
||||
'the answer is durable, so the message must not stay answerable'
|
||||
)
|
||||
|
||||
def test_a_bounced_message_is_reported(self, bot, db, participation, logs):
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
|
||||
with logs.at_level(logging.ERROR, logger='app.discord_bot'):
|
||||
asyncio.run(
|
||||
bot.handle_attendance_confirm(
|
||||
FakeReactor(snowflake), 7, row_id, FakeChannel(raises=http_error())
|
||||
)
|
||||
)
|
||||
|
||||
assert 'Could not answer a reaction' in logs.text
|
||||
|
||||
|
||||
class TestAnythingElseIsADefect:
|
||||
def test_an_unexpected_failure_propagates(self, bot, db, participation, monkeypatch, logs):
|
||||
"""It used to be caught here and logged as though it were a delivery
|
||||
problem. Letting it out is what puts it in front of on_error."""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
|
||||
def boom(event_type, reference_id):
|
||||
raise ZeroDivisionError('a defect, not an outage')
|
||||
|
||||
monkeypatch.setattr(TeamTryoutsBot, '_attendance_record', staticmethod(boom))
|
||||
|
||||
with pytest.raises(ZeroDivisionError):
|
||||
asyncio.run(
|
||||
bot.handle_attendance_confirm(FakeReactor(snowflake), 7, row_id, FakeChannel())
|
||||
)
|
||||
|
||||
def test_on_error_records_it_under_this_application_s_logger(self, bot, logs):
|
||||
"""discord.py logs escaped event exceptions on the `discord` logger,
|
||||
which logging_config does not wire to any of this application's
|
||||
files. That is where these used to go."""
|
||||
with logs.at_level(logging.ERROR, logger='app.discord_bot'):
|
||||
try:
|
||||
raise ZeroDivisionError('boom')
|
||||
except ZeroDivisionError:
|
||||
asyncio.run(bot.on_error('on_raw_reaction_add'))
|
||||
|
||||
assert 'on_raw_reaction_add' in logs.text
|
||||
assert 'defect' in logs.text
|
||||
assert any(record.exc_info for record in logs.records)
|
||||
|
||||
|
||||
class TestTheQueueBoundary:
|
||||
def test_queueing_never_raises_into_the_request_that_caused_it(self, monkeypatch, logs):
|
||||
"""This runs in a Flask request thread, after the work it announces
|
||||
is committed. A notification must not turn a finished page into a
|
||||
500 — but it must not swallow the reason either."""
|
||||
|
||||
def no_bot():
|
||||
raise RuntimeError('the bot could not be built')
|
||||
|
||||
monkeypatch.setattr(discord_bot, 'get_bot', no_bot)
|
||||
|
||||
with logs.at_level(logging.ERROR, logger='app.discord_bot'):
|
||||
queued = discord_bot.send_schedule_notification(1, 'match', 'Finale', 'd', 't', 2)
|
||||
|
||||
assert queued is False
|
||||
assert any(record.exc_info for record in logs.records)
|
||||
|
||||
def test_a_queued_notification_reaches_the_queue(self, bot, monkeypatch):
|
||||
"""The premise of the test above: the same call succeeds normally."""
|
||||
from queue import Queue
|
||||
|
||||
bot.message_queue = Queue()
|
||||
monkeypatch.setattr(discord_bot, 'get_bot', lambda: bot)
|
||||
|
||||
assert discord_bot.send_schedule_notification(1, 'match', 'Finale', 'd', 't', 2) is True
|
||||
assert bot.message_queue.get_nowait()['type'] == 'schedule_addition'
|
||||
|
||||
def test_an_unknown_item_type_is_named_and_dropped(self, bot, logs):
|
||||
with logs.at_level(logging.ERROR, logger='app.discord_bot'):
|
||||
asyncio.run(bot._dispatch({'type': 'invented', 'data': {}}))
|
||||
|
||||
assert 'invented' in logs.text
|
||||
|
||||
|
||||
class TestTheSchedulerSaysWhetherItStarted:
|
||||
class FailingScheduler:
|
||||
@staticmethod
|
||||
def add_job(*args, **kwargs):
|
||||
raise RuntimeError('no event loop')
|
||||
|
||||
class WorkingScheduler:
|
||||
def __init__(self):
|
||||
self.started = False
|
||||
|
||||
def add_job(self, *args, **kwargs):
|
||||
pass
|
||||
|
||||
def start(self):
|
||||
self.started = True
|
||||
|
||||
def test_a_scheduler_that_will_not_start_is_visible_from_outside(self, bot, logs):
|
||||
"""The other four /health keys all read green in this state: the
|
||||
thread is alive, the gateway is up, and no reminder is ever sent."""
|
||||
from zoneinfo import ZoneInfo
|
||||
|
||||
bot.scheduler = self.FailingScheduler()
|
||||
bot.timezone = ZoneInfo('America/Toronto')
|
||||
|
||||
with logs.at_level(logging.ERROR, logger='app.discord_bot'):
|
||||
asyncio.run(bot.start_scheduler())
|
||||
|
||||
assert bot.reminders_scheduled is False
|
||||
assert 'No reminder will be sent' in logs.text
|
||||
|
||||
def test_a_scheduler_that_starts_says_so(self, bot, logs):
|
||||
from zoneinfo import ZoneInfo
|
||||
|
||||
bot.scheduler = self.WorkingScheduler()
|
||||
bot.timezone = ZoneInfo('America/Toronto')
|
||||
|
||||
asyncio.run(bot.start_scheduler())
|
||||
|
||||
assert bot.scheduler.started is True
|
||||
assert bot.reminders_scheduled is True
|
||||
|
||||
|
||||
class TestTwoReactionsOnOneMessage:
|
||||
def test_the_second_one_finds_nothing_to_do(self, bot, db, participation, logs):
|
||||
"""A confirm and a decline arrive together; both got past the
|
||||
membership check in on_raw_reaction_add before either finished. The
|
||||
loser used to raise KeyError inside the blanket handler, where it
|
||||
read as an error with no consequence."""
|
||||
row_id, snowflake = participation
|
||||
_pending(bot, 7, row_id)
|
||||
reactor = FakeReactor(snowflake)
|
||||
|
||||
asyncio.run(bot.handle_attendance_confirm(reactor, 7, row_id, FakeChannel()))
|
||||
second = FakeChannel()
|
||||
asyncio.run(bot.handle_attendance_decline(reactor, 7, row_id, second))
|
||||
|
||||
assert second.sent == [], 'the second reaction must not answer twice'
|
||||
assert _confirmed(row_id) is True
|
||||
@@ -143,15 +143,29 @@ class TestLoading:
|
||||
|
||||
class TestHealthReporting:
|
||||
def test_the_status_shape_is_stable(self, monkeypatch):
|
||||
"""An external probe reads these keys; they are the contract."""
|
||||
"""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'}
|
||||
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:
|
||||
|
||||
Reference in New Issue
Block a user