fix(channels): close Discord file handle after upload (#3561)

send_file opened the attachment with a bare open() and never closed it,
leaking a file descriptor on every Discord file delivery. The handle was
also leaked on the failure path: when target.send raised, the except
branch logged and returned without closing fp. The "# noqa: SIM115"
suppressed the lint warning instead of fixing it.

Wrap open() in a with statement that stays open for the full upload —
the discord client reads fp while target.send runs on _discord_loop, and
once that future resolves the bytes are consumed, so closing here is
safe. This closes the handle on both the success and exception paths and
matches how telegram and feishu already handle their file uploads.

Adds regression tests asserting the handle is closed after send_file on
both the success and failure paths.

Refs #3544
This commit is contained in:
hataa
2026-06-13 23:27:17 +08:00
committed by GitHub
parent d23eac227f
commit 1783da42f4
2 changed files with 134 additions and 5 deletions
+8 -4
View File
@@ -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:
+126 -1
View File
@@ -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)