From fe7298689a568e0ad8b7fec21355012648b18528 Mon Sep 17 00:00:00 2001 From: Estrella Pan Date: Thu, 2 Jul 2026 11:57:35 +0200 Subject: [PATCH] fix(review): tighten qB login check, propagate delete failures end-to-end, close cache race Post-review fixes for the batch-3.2.8 branch: - auth() and the setup wizard again require a positive success marker on 200 responses ('Ok.' body) so a proxy or non-qB service answering 200 + HTML is not mistaken for a login; 204 stays the qB >= 5.2 success. - delete_rule no longer wraps a failed torrent deletion into a 200 reply, and the WebUI torrents-delete endpoint reports the failure instead of answering 'Torrents deleted' unconditionally. - The close-on-failed-auth teardown now lives once in DownloadClient.__aenter__ (client.logout() before raising) instead of being copy-pasted into each concrete client. - search_all() records a cache generation before querying and skips the cache write if an invalidation landed in between, so a stale snapshot can no longer overwrite a newer invalidation. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014w1Z6Nxy6XTRgkFXqPr9Zh --- backend/src/module/api/downloader.py | 10 ++++++-- backend/src/module/api/setup.py | 9 +++---- backend/src/module/database/bangumi.py | 12 +++++++--- .../downloader/client/aria2_downloader.py | 4 ---- .../module/downloader/client/qb_downloader.py | 13 +++++----- .../src/module/downloader/download_client.py | 4 ++++ backend/src/module/manager/torrent.py | 8 +++++++ backend/src/test/conftest.py | 2 +- backend/src/test/test_api_downloader.py | 23 +++++++++++++++++- backend/src/test/test_download_client.py | 13 ++++++++++ backend/src/test/test_qb_downloader.py | 24 +++++++++++-------- backend/src/test/test_setup.py | 16 +++++++++++++ 12 files changed, 107 insertions(+), 31 deletions(-) diff --git a/backend/src/module/api/downloader.py b/backend/src/module/api/downloader.py index 88712355..ac7a90dd 100644 --- a/backend/src/module/api/downloader.py +++ b/backend/src/module/api/downloader.py @@ -53,8 +53,14 @@ async def resume_torrents(req: TorrentHashesRequest): async def delete_torrents(req: TorrentDeleteRequest): hashes = "|".join(req.hashes) async with DownloadClient() as client: - await client.delete_torrent(hashes, delete_files=req.delete_files) - return {"msg_en": "Torrents deleted", "msg_zh": "种子已删除"} + ok = await client.delete_torrent(hashes, delete_files=req.delete_files) + if not ok: + return { + "status": False, + "msg_en": "Failed to delete torrents", + "msg_zh": "删除种子失败", + } + return {"status": True, "msg_en": "Torrents deleted", "msg_zh": "种子已删除"} @router.post("/torrents/tag", dependencies=[Depends(get_current_user)]) diff --git a/backend/src/module/api/setup.py b/backend/src/module/api/setup.py index 92c879b7..fec53455 100644 --- a/backend/src/module/api/setup.py +++ b/backend/src/module/api/setup.py @@ -162,10 +162,11 @@ async def test_downloader(req: TestDownloaderRequest): data={"username": req.username, "password": req.password}, ) # qBittorrent < 5.2 answers 200 + "Ok."; >= 5.2 answers 204 with - # an empty body on success (#1044). - if ( - login_resp.status_code in (200, 204) - and "fails" not in login_resp.text.lower() + # an empty body on success (#1044). Keep the positive body check + # for 200 so a proxy answering 200 + HTML is not reported as a + # working login. + if login_resp.status_code == 204 or ( + login_resp.status_code == 200 and "ok" in login_resp.text.lower() ): return TestResultResponse( success=True, diff --git a/backend/src/module/database/bangumi.py b/backend/src/module/database/bangumi.py index 92f550d0..30998340 100644 --- a/backend/src/module/database/bangumi.py +++ b/backend/src/module/database/bangumi.py @@ -72,13 +72,17 @@ _bangumi_cache: list[Bangumi] | None = None _bangumi_cache_time: float = 0 _BANGUMI_CACHE_TTL: float = 300.0 # 5 minutes - extended from 60s to reduce DB queries _bangumi_cache_lock = threading.Lock() +# Bumped on every invalidation so a search_all() that was already past the +# cache check cannot overwrite a newer invalidation with its stale snapshot. +_bangumi_cache_gen = 0 def _invalidate_bangumi_cache(): - global _bangumi_cache, _bangumi_cache_time + global _bangumi_cache, _bangumi_cache_time, _bangumi_cache_gen with _bangumi_cache_lock: _bangumi_cache = None _bangumi_cache_time = 0 + _bangumi_cache_gen += 1 class BangumiDatabase: @@ -376,6 +380,7 @@ class BangumiDatabase: and (now - _bangumi_cache_time) < _BANGUMI_CACHE_TTL ): return _bangumi_cache + gen_at_query = _bangumi_cache_gen statement = select(Bangumi) result = self.session.execute(statement) bangumis = list(result.scalars().all()) @@ -384,8 +389,9 @@ class BangumiDatabase: for b in bangumis: self.session.expunge(b) with _bangumi_cache_lock: - _bangumi_cache = bangumis - _bangumi_cache_time = now + if _bangumi_cache_gen == gen_at_query: + _bangumi_cache = bangumis + _bangumi_cache_time = now return bangumis def search_id(self, _id: int) -> Optional[Bangumi]: diff --git a/backend/src/module/downloader/client/aria2_downloader.py b/backend/src/module/downloader/client/aria2_downloader.py index d68e52b8..dfa78a5f 100644 --- a/backend/src/module/downloader/client/aria2_downloader.py +++ b/backend/src/module/downloader/client/aria2_downloader.py @@ -49,10 +49,6 @@ class Aria2Downloader: ) await asyncio.sleep(5) times += 1 - # Failed auth: close the client here or its connection pool leaks, - # because DownloadClient.__aenter__ raises before __aexit__ can run. - await self._client.aclose() - self._client = None return False async def logout(self): diff --git a/backend/src/module/downloader/client/qb_downloader.py b/backend/src/module/downloader/client/qb_downloader.py index bbf229dc..cff3c6ee 100644 --- a/backend/src/module/downloader/client/qb_downloader.py +++ b/backend/src/module/downloader/client/qb_downloader.py @@ -48,8 +48,13 @@ class QbDownloader: data={"username": self.username, "password": self.password}, ) # qBittorrent < 5.2 answers 200 + "Ok." / "Fails."; - # qBittorrent >= 5.2 answers 204 with an empty body on success (#1044). - if resp.status_code in (200, 204) and "fails" not in resp.text.lower(): + # qBittorrent >= 5.2 answers 204 with an empty body on success + # (#1044). Keep the positive body check for 200 so a proxy or + # non-qB service answering 200 + HTML is not mistaken for a + # successful login. + if ( + resp.status_code == 200 and resp.text.startswith("Ok") + ) or resp.status_code == 204: return True elif resp.status_code == 403: logger.error("Login refused by qBittorrent Server") @@ -82,10 +87,6 @@ class QbDownloader: else: logger.error(f"Unknown error: {e}") break - # Failed auth: DownloadClient.__aenter__ raises before __aexit__ can run, - # so the client must be closed here or its connection pool leaks. - await self._client.aclose() - self._client = None return False async def logout(self): diff --git a/backend/src/module/downloader/download_client.py b/backend/src/module/downloader/download_client.py index 8e06f960..7278ed95 100644 --- a/backend/src/module/downloader/download_client.py +++ b/backend/src/module/downloader/download_client.py @@ -52,6 +52,10 @@ class DownloadClient(TorrentPath): if not self.authed: await self.auth() if not self.authed: + # __aexit__ never runs when we raise here, so close the + # concrete client's connection pool now or it leaks on every + # failed connect (#1043). + await self.client.logout() raise ConnectionError("Download client authentication failed") else: logger.error("[Downloader] Already authed.") diff --git a/backend/src/module/manager/torrent.py b/backend/src/module/manager/torrent.py index 34f2e971..219ccd1f 100644 --- a/backend/src/module/manager/torrent.py +++ b/backend/src/module/manager/torrent.py @@ -58,6 +58,14 @@ class TorrentManager(Database): torrent_message = None if file: torrent_message = await self.delete_torrents(data, client) + if torrent_message.status_code == 500: + return ResponseModel( + status_code=500, + status=False, + msg_en=f"Deleted rule for {data.official_title}, " + "but deleting its torrents failed.", + msg_zh=f"已删除 {data.official_title} 规则，但删除种子失败。", + ) logger.info(f"[Manager] Delete rule for {data.official_title}") return ResponseModel( status_code=200, diff --git a/backend/src/test/conftest.py b/backend/src/test/conftest.py index fa856fd7..61c13c73 100644 --- a/backend/src/test/conftest.py +++ b/backend/src/test/conftest.py @@ -222,5 +222,5 @@ def mock_download_client(): ] client.pause_torrent.return_value = None client.resume_torrent.return_value = None - client.delete_torrent.return_value = None + client.delete_torrent.return_value = True return client diff --git a/backend/src/test/test_api_downloader.py b/backend/src/test/test_api_downloader.py index 44d4f512..92680851 100644 --- a/backend/src/test/test_api_downloader.py +++ b/backend/src/test/test_api_downloader.py @@ -62,7 +62,7 @@ def mock_download_client(): ] client.pause_torrent.return_value = None client.resume_torrent.return_value = None - client.delete_torrent.return_value = None + client.delete_torrent.return_value = True return client @@ -433,3 +433,24 @@ class TestAutoTagTorrents: assert data["unmatched_count"] == 1 assert len(data["unmatched"]) == 1 mock_download_client.add_tag.assert_not_called() + + +class TestDeleteTorrentsFailure: + def test_delete_failure_is_reported(self, authed_client, mock_download_client): + """A rejected qB delete must not be reported as success (#1046).""" + mock_download_client.delete_torrent.return_value = False + with patch("module.api.downloader.DownloadClient") as MockClient: + MockClient.return_value.__aenter__ = AsyncMock( + return_value=mock_download_client + ) + MockClient.return_value.__aexit__ = AsyncMock(return_value=False) + + response = authed_client.post( + "/api/v1/downloader/torrents/delete", + json={"hashes": ["abc123"], "delete_files": True}, + ) + + assert response.status_code == 200 + data = response.json() + assert data["status"] is False + assert data["msg_en"] == "Failed to delete torrents" diff --git a/backend/src/test/test_download_client.py b/backend/src/test/test_download_client.py index 2ce43af9..bc10e2e5 100644 --- a/backend/src/test/test_download_client.py +++ b/backend/src/test/test_download_client.py @@ -48,6 +48,19 @@ class TestAuth: await download_client.auth() assert download_client.authed is False + async def test_aenter_closes_client_on_failed_auth( + self, download_client, mock_qb_client + ): + """__aenter__ must close the concrete client's pool before raising, + because __aexit__ never runs on a failed connect (leak fix, #1043).""" + mock_qb_client.auth.return_value = False + + with pytest.raises(ConnectionError): + async with download_client: + pass + + mock_qb_client.logout.assert_awaited_once() + # --------------------------------------------------------------------------- # init_downloader diff --git a/backend/src/test/test_qb_downloader.py b/backend/src/test/test_qb_downloader.py index 81ee209f..40d7ed72 100644 --- a/backend/src/test/test_qb_downloader.py +++ b/backend/src/test/test_qb_downloader.py @@ -584,27 +584,31 @@ class TestAuthQb52Compat: assert result is False - async def test_auth_closes_client_on_failure(self): - """A failed auth must aclose() the httpx client (leak fix).""" + async def test_auth_returns_false_on_200_html_body(self): + """A 200 + HTML page (proxy, wrong service) is not a successful login.""" qb = QbDownloader( host="localhost:8080", username="admin", password="pass", ssl=False ) mock_client = AsyncMock() mock_resp = MagicMock() - mock_resp.status_code = 403 - mock_resp.text = "Forbidden" + mock_resp.status_code = 200 + mock_resp.text = "Sign in" mock_client.post = AsyncMock(return_value=mock_resp) - with patch( - "module.downloader.client.qb_downloader.httpx.AsyncClient", - return_value=mock_client, + with ( + patch( + "module.downloader.client.qb_downloader.httpx.AsyncClient", + return_value=mock_client, + ), + patch( + "module.downloader.client.qb_downloader.asyncio.sleep", + new_callable=AsyncMock, + ), ): - result = await qb.auth() + result = await qb.auth(retry=1) assert result is False - mock_client.aclose.assert_awaited_once() - assert qb._client is None # --------------------------------------------------------------------------- diff --git a/backend/src/test/test_setup.py b/backend/src/test/test_setup.py index f386e07b..d342a21a 100644 --- a/backend/src/test/test_setup.py +++ b/backend/src/test/test_setup.py @@ -373,3 +373,19 @@ class TestTestDownloaderHardening: assert data["success"] is False assert "secret-detail-xyz" not in data["message_en"] assert "secret-detail-xyz" not in data["message_zh"] + + def test_login_rejects_200_html_body(self, client, mock_first_run): + """A proxy answering 200 + HTML to the login POST is not a success.""" + from unittest.mock import MagicMock + + get_resp = MagicMock(text="qBittorrent WebUI") + login_resp = MagicMock( + status_code=200, text="portal" + ) + cls_patch = self._mock_client(get_resp=get_resp, login_resp=login_resp) + try: + response = self._post(client) + finally: + cls_patch.stop() + assert response.status_code == 200 + assert response.json()["success"] is False