diff --git a/backend/app/channels/discord.py b/backend/app/channels/discord.py index d81a71fd6..75acc9064 100644 --- a/backend/app/channels/discord.py +++ b/backend/app/channels/discord.py @@ -205,10 +205,14 @@ class DiscordChannel(Channel): return False try: - fp = open(str(attachment.actual_path), "rb") # noqa: SIM115 - file = self._discord_module.File(fp, filename=attachment.filename) - send_future = asyncio.run_coroutine_threadsafe(target.send(file=file), self._discord_loop) - await asyncio.wrap_future(send_future) + # Keep the file handle open only for the duration of the upload: discord.py + # reads ``fp`` while ``target.send`` runs on ``_discord_loop``; once that + # future resolves the bytes are consumed, so closing here is safe and avoids + # leaking the handle on both the success and failure paths. + with open(str(attachment.actual_path), "rb") as fp: + file = self._discord_module.File(fp, filename=attachment.filename) + send_future = asyncio.run_coroutine_threadsafe(target.send(file=file), self._discord_loop) + await asyncio.wrap_future(send_future) logger.info("[Discord] file uploaded: %s", attachment.filename) return True except Exception: diff --git a/backend/tests/test_discord_channel.py b/backend/tests/test_discord_channel.py index 0cebce5af..ecbad5de9 100644 --- a/backend/tests/test_discord_channel.py +++ b/backend/tests/test_discord_channel.py @@ -2,13 +2,17 @@ from __future__ import annotations +import asyncio +import builtins +import threading from types import SimpleNamespace +from unittest.mock import patch import pytest from app.channels.discord import DiscordChannel from app.channels.manager import CHANNEL_CAPABILITIES -from app.channels.message_bus import InboundMessageType, MessageBus +from app.channels.message_bus import InboundMessageType, MessageBus, OutboundMessage, ResolvedAttachment from app.channels.service import _CHANNEL_REGISTRY @@ -86,3 +90,124 @@ async def test_discord_bot_mention_known_command_routes_as_command() -> None: assert inbound.text == "/help" assert inbound.msg_type == InboundMessageType.COMMAND assert inbound.topic_id == "456" + + +# --------------------------------------------------------------------------- +# send_file file-handle lifecycle +# --------------------------------------------------------------------------- + + +def _start_bg_loop() -> tuple[asyncio.AbstractEventLoop, threading.Thread]: + """Spin up a real background event loop, mirroring ``DiscordChannel._discord_loop``. + + ``send_file`` schedules work onto ``_discord_loop`` via + ``run_coroutine_threadsafe`` and awaits the result with ``wrap_future``, so a + real running loop is the most faithful way to exercise that path. + """ + loop = asyncio.new_event_loop() + ready = threading.Event() + + def _runner() -> None: + loop.call_soon(ready.set) + loop.run_forever() + + thread = threading.Thread(target=_runner, daemon=True) + thread.start() + ready.wait() + return loop, thread + + +def _stop_bg_loop(loop: asyncio.AbstractEventLoop, thread: threading.Thread) -> None: + loop.call_soon_threadsafe(loop.stop) + thread.join(timeout=5) + loop.close() + + +def _build_send_file_channel(bg_loop: asyncio.AbstractEventLoop) -> DiscordChannel: + channel = DiscordChannel(bus=MessageBus(), config={"bot_token": "token"}) + channel._discord_loop = bg_loop + channel._discord_module = SimpleNamespace(File=lambda fp, filename=None: fp) + + async def _noop(*_args, **_kwargs): + return None + + channel._stop_typing = _noop + return channel + + +def _tracking_open(): + """Wrap ``builtins.open`` to record every handle it returns.""" + handles: list = [] + real_open = builtins.open + + def _open(path, *args, **kwargs): + handle = real_open(path, *args, **kwargs) + handles.append(handle) + return handle + + return handles, _open + + +async def _noop_coro(*_args, **_kwargs): + return None + + +def _resolve_to(target): + async def _resolve_target(_msg): + return target + + return _resolve_target + + +@pytest.mark.asyncio +async def test_send_file_closes_file_handle(tmp_path) -> None: + """The file handle opened for upload is closed once send_file returns (success path).""" + bg_loop, bg_thread = _start_bg_loop() + try: + channel = _build_send_file_channel(bg_loop) + target = SimpleNamespace(send=_noop_coro) + channel._resolve_target = _resolve_to(target) + + path = tmp_path / "upload.txt" + path.write_bytes(b"hello") + att = ResolvedAttachment("/mnt/user-data/outputs/upload.txt", path, "upload.txt", "text/plain", 5, False) + msg = OutboundMessage(channel_name="discord", chat_id="c1", thread_id="t1", text="t") + + handles, tracking_open = _tracking_open() + with patch("builtins.open", tracking_open): + result = await channel.send_file(msg, att) + + assert result is True + assert len(handles) == 1 + assert handles[0].closed is True + finally: + _stop_bg_loop(bg_loop, bg_thread) + + +@pytest.mark.asyncio +async def test_send_file_closes_handle_when_send_fails(tmp_path) -> None: + """The file handle is still closed when target.send raises (failure path).""" + bg_loop, bg_thread = _start_bg_loop() + try: + channel = _build_send_file_channel(bg_loop) + + async def _failing_send(*, file=None): + raise RuntimeError("upload failed") + + target = SimpleNamespace(send=_failing_send) + channel._resolve_target = _resolve_to(target) + + path = tmp_path / "upload.txt" + path.write_bytes(b"hello") + att = ResolvedAttachment("/mnt/user-data/outputs/upload.txt", path, "upload.txt", "text/plain", 5, False) + msg = OutboundMessage(channel_name="discord", chat_id="c1", thread_id="t1", text="t") + + handles, tracking_open = _tracking_open() + with patch("builtins.open", tracking_open): + result = await channel.send_file(msg, att) + + assert result is False + assert len(handles) == 1 + assert handles[0].closed is True + finally: + _stop_bg_loop(bg_loop, bg_thread)