Merge pull request #3828 from c020627/fix-dingtalk-media-tmp-cleanup

fix(dingtalk): clean up outbound media temp files

channel/dingtalk/dingtalk_channel.py

@@ -25,7 +25,7 @@ from bridge.reply import Reply, ReplyType
 from channel.chat_channel import ChatChannel
 from channel.chat_message import safe_filename
 from common import state_dir
-from common.media_download import MAX_FILE_BYTES, MediaTooLargeError, download_to_file
+from common.media_download import MAX_FILE_BYTES, MediaTooLargeError, download_to_file, remove_download
 from channel.dingtalk.dingtalk_message import DingTalkMessage
 from channel.dingtalk.dingtalk_stream_card import (
     DingTalkCardStreamer,
@@ -449,6 +449,7 @@ class DingTalkChanel(ChatChannel, dingtalk_stream.ChatbotHandler):
             file_path = file_path[7:]
         
         # 如果是 HTTP URL，先下载（带字节上限，防止超大响应耗尽内存/磁盘）
+        temp_file = None
         if file_path.startswith("http://") or file_path.startswith("https://"):
             try:
                 import uuid
@@ -458,7 +459,10 @@ class DingTalkChanel(ChatChannel, dingtalk_stream.ChatbotHandler):
                     safe_filename(unquote(os.path.basename(urlparse(file_path).path)))
                     or f"media_{uuid.uuid4()}"
                 )
-                temp_file = os.path.join(str(state_dir.tmp_dir()), file_name)
+                # A generated unique name, not the URL's: the tmp dir also holds
+                # inbound attachments, so two downloads of the same URL would
+                # overwrite each other under a name taken from the URL.
+                temp_file = state_dir.tmp_file("dingtalk_media", os.path.splitext(file_name)[1])
                 try:
                     download_to_file(
                         file_path, temp_file, MAX_FILE_BYTES,
@@ -502,6 +506,11 @@ class DingTalkChanel(ChatChannel, dingtalk_stream.ChatbotHandler):
         except Exception as e:
             logger.error(f"[DingTalk] Error uploading media: {e}")
             return None
+        finally:
+            # A remote download belongs to this call to clean up, whatever the
+            # upload did with it -- the way the weixin / wecom_bot senders do.
+            if temp_file:
+                remove_download(temp_file)
     
     def send_image_with_media_id(self, access_token: str, media_id: str, incoming_message, is_group: bool) -> bool:
         """
@@ -1026,31 +1035,43 @@ class DingTalkChanel(ChatChannel, dingtalk_stream.ChatbotHandler):
 
             amr_path = voice_path
             duration_ms = 0
-            if not voice_path.lower().endswith((".amr", ".ogg")):
-                try:
-                    from voice.audio_convert import any_to_amr
-                    amr_path = os.path.splitext(voice_path)[0] + ".amr"
-                    duration_ms = int(any_to_amr(voice_path, amr_path) or 0)
-                except Exception as e:
-                    logger.error(f"[DingTalk] Failed to convert voice to amr: {e}")
-                    self.reply_text("抱歉，语音转码失败", incoming_message)
+            # The amr this send creates is ours to clean up; the TTS source
+            # beside it belongs to voice synthesis and is left in place, the
+            # way the weixin / wecom_bot senders leave a local source alone.
+            temp_files = []
+            try:
+                if not voice_path.lower().endswith((".amr", ".ogg")):
+                    try:
+                        from voice.audio_convert import any_to_amr
+                        amr_path = os.path.splitext(voice_path)[0] + ".amr"
+                        # Register before converting: pydub opens the target
+                        # file before it spawns ffmpeg, so a conversion that
+                        # dies partway leaves a half-written amr behind too.
+                        temp_files.append(amr_path)
+                        duration_ms = int(any_to_amr(voice_path, amr_path) or 0)
+                    except Exception as e:
+                        logger.error(f"[DingTalk] Failed to convert voice to amr: {e}")
+                        self.reply_text("抱歉，语音转码失败", incoming_message)
+                        return
+
+                media_id = self.upload_media(amr_path, media_type="voice")
+                if not media_id:
+                    logger.error("[DingTalk] Failed to upload voice media")
+                    self.reply_text("抱歉，语音上传失败", incoming_message)
                     return
 
-            media_id = self.upload_media(amr_path, media_type="voice")
-            if not media_id:
-                logger.error("[DingTalk] Failed to upload voice media")
-                self.reply_text("抱歉，语音上传失败", incoming_message)
-                return
-
-            msg_param = {
-                "mediaId": media_id,
-                "duration": str(duration_ms or 1000),
-            }
-            success = self._send_file_message(
-                access_token, incoming_message, "sampleAudio", msg_param, isgroup
-            )
-            if not success:
-                self.reply_text("抱歉，语音发送失败", incoming_message)
+                msg_param = {
+                    "mediaId": media_id,
+                    "duration": str(duration_ms or 1000),
+                }
+                success = self._send_file_message(
+                    access_token, incoming_message, "sampleAudio", msg_param, isgroup
+                )
+                if not success:
+                    self.reply_text("抱歉，语音发送失败", incoming_message)
+            finally:
+                for path in temp_files:
+                    remove_download(path)
             return
 
         # 处理文本消息

tests/test_dingtalk_bounded_media_download.py

@@ -2,9 +2,14 @@
 """DingTalk upload_media() must cap remote downloads (cf. #3300, Feishu).
 
 It used a bare requests.get + f.write, so an oversized URL filled memory/disk.
-Pin it to the shared download_to_file helper.
+Pin it to the shared download_to_file helper -- and, a download being this
+call's to make, to cleaning it up again on every way out, the way the
+weixin / wecom_bot senders do. The voice send converts the TTS output to amr
+beside it; that amr is transient too, including the half-written one a failed
+conversion leaves behind.
 """
-from tests.test_dingtalk_streaming_cards import _bare_channel
+from bridge.reply import Reply, ReplyType
+from tests.test_dingtalk_streaming_cards import _bare_channel, _context
 
 
 class FakeResp:
@@ -80,12 +85,28 @@ def test_http_url_over_size_limit_rejected(monkeypatch, tmp_path):
 
 
 def test_http_url_query_string_stays_out_of_the_file_name(monkeypatch, tmp_path):
+    from channel.dingtalk import dingtalk_channel as mod
+
     get = FakeResp(content=b"<media data>")
     post = FakeResp(json_data={"errcode": 0, "media_id": "mid-3"})
     _stub(monkeypatch, get, post, tmp_path)
+    real_download = mod.download_to_file
+    seen = {}
+
+    def recording_download(url, path, *a, **k):
+        seen["path"] = path
+        return real_download(url, path, *a, **k)
+
+    monkeypatch.setattr(mod, "download_to_file", recording_download)
     url = "https://cdn.example/dir/%E6%8A%A5%E5%91%8A.pdf?token=secret&x=a/b"
     assert _channel().upload_media(url, "file") == "mid-3"
-    assert [p.name for p in tmp_path.iterdir()] == ["报告.pdf"]
+    # The download target is a generated unique name, so nothing from the URL
+    # -- query string included -- ever becomes part of a path.
+    assert "?" not in seen["path"]
+    assert "token" not in seen["path"]
+    assert "secret" not in seen["path"]
+    # A download belongs to this call: nothing is left in the tmp dir.
+    assert [p.name for p in tmp_path.iterdir()] == []
 
 
 def test_local_file_url_still_uploaded(monkeypatch, tmp_path):
@@ -109,3 +130,63 @@ def test_http_url_download_is_guarded(monkeypatch, tmp_path):
     monkeypatch.setattr(mod, "download_to_file", download_to_file)
     assert _channel().upload_media("https://cdn.example/a.pdf", "file") is None
     assert seen.get("guarded") is True
+
+
+def test_http_url_download_is_cleaned_up_when_the_upload_fails(monkeypatch, tmp_path):
+    get = FakeResp(content=b"<media data>")
+    post = FakeResp(json_data={"errcode": 40001, "errmsg": "invalid token"})
+    _stub(monkeypatch, get, post, tmp_path)
+    assert _channel().upload_media("https://cdn.example/ok.mp4", "video") is None
+    assert [p.name for p in tmp_path.iterdir()] == []
+
+
+def test_voice_amr_is_cleaned_up_after_the_send(monkeypatch, tmp_path):
+    from voice import audio_convert
+
+    voice = tmp_path / "tts.mp3"
+    voice.write_bytes(b"<mp3 data>")
+    amr = tmp_path / "tts.amr"
+
+    def fake_convert(src, dst):
+        # pydub opens the target, then spawns ffmpeg into it.
+        with open(dst, "wb") as f:
+            f.write(b"#!AMR\n")
+        return 1.5
+
+    monkeypatch.setattr(audio_convert, "any_to_amr", fake_convert)
+    _stub(monkeypatch, FakeResp(), FakeResp(json_data={"errcode": 0, "media_id": "mid-v"}), tmp_path)
+    ch = _channel()
+    ch._send_file_message = lambda *a, **k: True
+    ch.reply_text = lambda *a, **k: None
+    ch.send(Reply(ReplyType.VOICE, str(voice)), _context())
+
+    assert not amr.exists()
+    # The TTS source belongs to voice synthesis, not to this send.
+    assert [p.name for p in tmp_path.iterdir()] == ["tts.mp3"]
+
+
+def test_voice_amr_is_cleaned_up_when_the_conversion_fails(monkeypatch, tmp_path):
+    from voice import audio_convert
+
+    voice = tmp_path / "tts.mp3"
+    voice.write_bytes(b"<mp3 data>")
+    amr = tmp_path / "tts.amr"
+
+    def failing_convert(src, dst):
+        # pydub opens the target before it spawns ffmpeg: a conversion that
+        # dies partway still leaves a half-written amr behind.
+        with open(dst, "wb") as f:
+            f.write(b"#!AMR\nhalf")
+        raise RuntimeError("ffmpeg died")
+
+    monkeypatch.setattr(audio_convert, "any_to_amr", failing_convert)
+    _stub(monkeypatch, FakeResp(), FakeResp(), tmp_path)
+    ch = _channel()
+    ch._send_file_message = lambda *a, **k: True
+    texts = []
+    ch.reply_text = lambda content, incoming: texts.append(content)
+    ch.send(Reply(ReplyType.VOICE, str(voice)), _context())
+
+    assert not amr.exists()
+    assert texts == ["抱歉，语音转码失败"]
+    assert [p.name for p in tmp_path.iterdir()] == ["tts.mp3"]