From b4403ed3931c239b0012ea31926fcb445aa461e7 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Sat, 16 May 2026 10:49:11 -0700 Subject: [PATCH] Amazon download client: fix engine API calls in status methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_all_downloads` was calling `engine.get_all_records()` — a method that doesn't exist on DownloadEngine. Same story for `cancel_record` and `clear_completed`. The engine exposes `iter_records_for_source`, `get_record`, `update_record`, and `remove_record` — matching what every other streaming client (Deezer, HiFi, Qobuz, SoundCloud, Tidal, YouTube) already uses. With `get_all_downloads` silently returning `[]` on every call (the missing method raised, `except Exception: return []` swallowed it), the download monitor never saw Amazon records as complete — tasks stayed stuck at 0% even after the file had fully downloaded. Changes: - `get_all_downloads` → `iter_records_for_source('amazon')` - `get_download_status` → `get_record('amazon', id)`, no try/except - `cancel_download` → `get_record` check + `update_record` (Cancelled) + optional `remove_record` — same pattern as deezer/hifi/etc - `clear_all_completed_downloads` → iterate + `remove_record` for terminal states; returns True on no-engine (nothing to clear = success) - `_record_to_status` drops the `download_id` argument; reads `rec['id']` instead (worker stores `'id'` in every record — `iter_records_for_source` returns the full record dict) Tests updated to match: `iter_records_for_source` mock replaces `get_all_records`, cancel test verifies `update_record`+`remove_record`, clear test verifies only terminal-state records are removed, graceful-error test replaced with no-records boundary test (exception propagation is handled at the engine aggregator layer, not per-plugin). --- core/amazon_download_client.py | 56 ++++++++-------- tests/tools/test_amazon_download_client.py | 75 ++++++++++++++-------- 2 files changed, 76 insertions(+), 55 deletions(-) diff --git a/core/amazon_download_client.py b/core/amazon_download_client.py index 44c92474..76ba23ec 100644 --- a/core/amazon_download_client.py +++ b/core/amazon_download_client.py @@ -375,20 +375,16 @@ class AmazonDownloadClient(DownloadSourcePlugin): async def get_all_downloads(self) -> List[DownloadStatus]: if self._engine is None: return [] - try: - records = self._engine.get_all_records("amazon") - return [self._record_to_status(dl_id, rec) for dl_id, rec in records.items()] - except Exception: - return [] + return [ + self._record_to_status(record) + for record in self._engine.iter_records_for_source('amazon') + ] async def get_download_status(self, download_id: str) -> Optional[DownloadStatus]: if self._engine is None: return None - try: - rec = self._engine.get_record("amazon", download_id) - return self._record_to_status(download_id, rec) if rec is not None else None - except Exception: - return None + record = self._engine.get_record('amazon', download_id) + return self._record_to_status(record) if record is not None else None async def cancel_download( self, @@ -398,19 +394,21 @@ class AmazonDownloadClient(DownloadSourcePlugin): ) -> bool: if self._engine is None: return False - try: - return self._engine.cancel_record("amazon", download_id, remove=remove) - except Exception: + if self._engine.get_record('amazon', download_id) is None: return False + self._engine.update_record('amazon', download_id, {'state': 'Cancelled'}) + if remove: + self._engine.remove_record('amazon', download_id) + return True async def clear_all_completed_downloads(self) -> bool: if self._engine is None: - return False - try: - self._engine.clear_completed("amazon") return True - except Exception: - return False + terminal = {'Completed, Succeeded', 'Cancelled', 'Errored', 'Aborted'} + for record in list(self._engine.iter_records_for_source('amazon')): + if record.get('state') in terminal: + self._engine.remove_record('amazon', record['id']) + return True # ------------------------------------------------------------------ # Private helpers @@ -428,16 +426,16 @@ class AmazonDownloadClient(DownloadSourcePlugin): return path.with_name(f"{stem}_{uuid.uuid4().hex[:8]}{suffix}") @staticmethod - def _record_to_status(download_id: str, rec: Dict[str, Any]) -> DownloadStatus: + def _record_to_status(rec: Dict[str, Any]) -> DownloadStatus: return DownloadStatus( - id=download_id, - filename=str(rec.get("filename", "")), - username="amazon", - state=str(rec.get("state", "queued")), - progress=float(rec.get("progress", 0.0)), - size=int(rec.get("size", 0)), - transferred=int(rec.get("transferred", 0)), - speed=int(rec.get("speed", 0)), - time_remaining=rec.get("time_remaining"), - file_path=rec.get("file_path"), + id=str(rec.get('id', '')), + filename=str(rec.get('filename', '')), + username='amazon', + state=str(rec.get('state', 'queued')), + progress=float(rec.get('progress', 0.0)), + size=int(rec.get('size', 0)), + transferred=int(rec.get('transferred', 0)), + speed=int(rec.get('speed', 0)), + time_remaining=rec.get('time_remaining'), + file_path=rec.get('file_path'), ) diff --git a/tests/tools/test_amazon_download_client.py b/tests/tools/test_amazon_download_client.py index 47ea0715..2d1b70f9 100644 --- a/tests/tools/test_amazon_download_client.py +++ b/tests/tools/test_amazon_download_client.py @@ -325,6 +325,7 @@ class TestUniquePath: class TestRecordToStatus: def test_fields_mapped(self): rec = { + "id": "dl-001", "filename": "B1||Artist - Title", "state": "downloading", "progress": 0.5, @@ -334,7 +335,7 @@ class TestRecordToStatus: "time_remaining": 5, "file_path": "/tmp/track.flac", } - status = AmazonDownloadClient._record_to_status("dl-001", rec) + status = AmazonDownloadClient._record_to_status(rec) assert status.id == "dl-001" assert status.filename == "B1||Artist - Title" @@ -348,7 +349,8 @@ class TestRecordToStatus: assert status.file_path == "/tmp/track.flac" def test_defaults_for_missing_fields(self): - status = AmazonDownloadClient._record_to_status("dl-002", {}) + status = AmazonDownloadClient._record_to_status({}) + assert status.id == "" assert status.state == "queued" assert status.progress == 0.0 assert status.size == 0 @@ -716,22 +718,22 @@ class TestStatusInterface: def test_get_all_downloads_converts_records(self, tmp_path): client = _make_client(tmp_path) engine = MagicMock() - engine.get_all_records.return_value = { - "dl-001": { - "filename": "B1||A - T", - "state": "complete", - "progress": 1.0, - "size": 5_000_000, - "transferred": 5_000_000, - "speed": 0, - } - } + engine.iter_records_for_source.return_value = iter([{ + "id": "dl-001", + "filename": "B1||A - T", + "state": "complete", + "progress": 1.0, + "size": 5_000_000, + "transferred": 5_000_000, + "speed": 0, + }]) client._engine = engine statuses = run(client.get_all_downloads()) assert len(statuses) == 1 assert statuses[0].id == "dl-001" assert statuses[0].state == "complete" + engine.iter_records_for_source.assert_called_once_with('amazon') def test_get_download_status_returns_none_without_engine(self, tmp_path): client = _make_client(tmp_path) @@ -741,7 +743,8 @@ class TestStatusInterface: client = _make_client(tmp_path) engine = MagicMock() engine.get_record.return_value = { - "original_filename": "B1||A - T", + "id": "dl-001", + "filename": "B1||A - T", "state": "downloading", "progress": 0.7, "size": 10_000_000, @@ -752,9 +755,10 @@ class TestStatusInterface: status = run(client.get_download_status("dl-001")) assert status is not None + assert status.id == "dl-001" assert status.state == "downloading" assert status.progress == 0.7 - engine.get_record.assert_called_once_with("amazon", "dl-001") + engine.get_record.assert_called_once_with('amazon', 'dl-001') def test_get_download_status_miss(self, tmp_path): client = _make_client(tmp_path) @@ -771,36 +775,55 @@ class TestStatusInterface: def test_cancel_delegates_to_engine(self, tmp_path): client = _make_client(tmp_path) engine = MagicMock() - engine.cancel_record.return_value = True + engine.get_record.return_value = {'id': 'dl-001', 'state': 'downloading'} client._engine = engine result = run(client.cancel_download("dl-001", remove=True)) assert result is True - engine.cancel_record.assert_called_once_with("amazon", "dl-001", remove=True) + engine.update_record.assert_called_once_with('amazon', 'dl-001', {'state': 'Cancelled'}) + engine.remove_record.assert_called_once_with('amazon', 'dl-001') + + def test_cancel_returns_false_when_record_not_found(self, tmp_path): + client = _make_client(tmp_path) + engine = MagicMock() + engine.get_record.return_value = None + client._engine = engine - def test_clear_completed_returns_false_without_engine(self, tmp_path): + result = run(client.cancel_download("nonexistent")) + assert result is False + engine.update_record.assert_not_called() + + def test_clear_completed_returns_true_without_engine(self, tmp_path): client = _make_client(tmp_path) - assert run(client.clear_all_completed_downloads()) is False + assert run(client.clear_all_completed_downloads()) is True - def test_clear_completed_delegates_to_engine(self, tmp_path): + def test_clear_completed_removes_terminal_records(self, tmp_path): client = _make_client(tmp_path) engine = MagicMock() + engine.iter_records_for_source.return_value = iter([ + {'id': 'dl-001', 'state': 'Completed, Succeeded'}, + {'id': 'dl-002', 'state': 'InProgress, Downloading'}, + {'id': 'dl-003', 'state': 'Errored'}, + ]) client._engine = engine result = run(client.clear_all_completed_downloads()) assert result is True - engine.clear_completed.assert_called_once_with("amazon") + # Only terminal records removed + removed_ids = [c[0][1] for c in engine.remove_record.call_args_list] + assert 'dl-001' in removed_ids + assert 'dl-003' in removed_ids + assert 'dl-002' not in removed_ids - def test_status_methods_return_gracefully_on_engine_error(self, tmp_path): + def test_status_methods_no_records(self, tmp_path): + """Engine with no Amazon records returns empty/None gracefully.""" client = _make_client(tmp_path) engine = MagicMock() - engine.get_all_records.side_effect = Exception("engine crash") - engine.get_record.side_effect = Exception("engine crash") - engine.cancel_record.side_effect = Exception("engine crash") - engine.clear_completed.side_effect = Exception("engine crash") + engine.iter_records_for_source.return_value = iter([]) + engine.get_record.return_value = None client._engine = engine assert run(client.get_all_downloads()) == [] assert run(client.get_download_status("dl-001")) is None assert run(client.cancel_download("dl-001")) is False - assert run(client.clear_all_completed_downloads()) is False + assert run(client.clear_all_completed_downloads()) is True