diff --git a/app/tests/test_download_queue.py b/app/tests/test_download_queue.py index f11c3a6..036a67b 100644 --- a/app/tests/test_download_queue.py +++ b/app/tests/test_download_queue.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio import copy import os import re @@ -311,6 +312,47 @@ async def test_start_pending_moves_to_queue(dq_env): with patch.object(DownloadQueue, "_DownloadQueue__start_download", AsyncMock()): await dq.start_pending([url]) assert not dq.pending.exists(url) + # It is in the queue now and starts on its own, so it must not keep + # advertising the Start button the UI draws for 'pending' (#1081), and the + # client has to be told before the concurrency slot frees up. + assert dq.queue.get(url).info.status == "queued" + assert notifier.updated.await_args[0][0].status == "queued" + + +@pytest.mark.asyncio +async def test_queued_download_is_not_offered_as_startable(dq_env): + """A download waiting on a MAX_CONCURRENT_DOWNLOADS slot used to sit at + 'pending', which is the status the UI draws a Start button for — but + start_pending has nothing to do for an item already in the queue, so the + button silently did nothing and still reported success (#1081). + """ + notifier = AsyncMock() + dq_env.MAX_CONCURRENT_DOWNLOADS = "1" + dq = DownloadQueue(dq_env, notifier) + released = asyncio.Event() + + def fake_extract(self, url, *_args, **_kwargs): + return {"_type": "video", "id": url[-1], "title": f"Video {url[-1]}", + "url": url, "webpage_url": url} + + async def blocking_start(self, notifier_, executor=None): + await released.wait() + + first, second = "https://example.com/v1", "https://example.com/v2" + with patch.object(DownloadQueue, "_DownloadQueue__extract_info", fake_extract), \ + patch("ytdl.Download.start", blocking_start), \ + patch("ytdl.Download.close", lambda self: None): + for url in (first, second): + await dq.add(url, "video", "auto", "any", "best", "", "", 0, auto_start=True) + await asyncio.sleep(0) + + # The first holds the only slot; the second is waiting behind it. + assert dq.queue.get(second).info.status == "queued" + assert not dq.pending.exists(second) + + released.set() + + await asyncio.sleep(0) @pytest.mark.asyncio @@ -1200,7 +1242,7 @@ async def test_probe_scheduled_starts_when_live(dq_env): assert url not in dq._scheduled_probe_at assert download.info.live_status == "is_live" - assert download.info.status == "pending" + assert download.info.status == "queued" start_mock.assert_called_once_with(download) @@ -1345,7 +1387,7 @@ async def test_probe_recovers_after_transient_then_starts(dq_env): assert url not in dq._scheduled_probe_at assert url not in dq._scheduled_probe_failures - assert download.info.status == "pending" + assert download.info.status == "queued" # Placeholder error/msg cleared now that a real download is starting. assert download.info.error is None assert download.info.msg is None diff --git a/app/ytdl.py b/app/ytdl.py index 2774465..1abf7da 100644 --- a/app/ytdl.py +++ b/app/ytdl.py @@ -495,6 +495,12 @@ class DownloadInfo: self.folder = folder self.custom_name_prefix = custom_name_prefix self.msg = self.percent = self.speed = self.eta = None + # 'pending' means "waiting for the user to press Start" — an item added + # with auto_start=False, sitting in self.pending. A download that is in + # self.queue waiting for a MAX_CONCURRENT_DOWNLOADS slot is 'queued' + # instead: it starts on its own and there is nothing to press. Keeping + # both under one name left the UI showing a Start button that silently + # did nothing (#1081). self.status = "pending" self.size = None self.timestamp = time.time_ns() @@ -1484,7 +1490,7 @@ class DownloadQueue: return self._unregister_scheduled(url) - info.status = 'pending' + info.status = 'queued' # Clear the "scheduled to start at ..." placeholder now that the stream # is live and a real download is about to begin. info.error = None @@ -1499,7 +1505,7 @@ class DownloadQueue: def _force_start_scheduled(self, download: Download) -> None: self._unregister_scheduled(download.info.url) - download.info.status = 'pending' + download.info.status = 'queued' download.info.error = None download.info.msg = None bg_tasks.create_task(self.__start_download(download), name="start_download") @@ -1649,6 +1655,7 @@ class DownloadQueue: if is_upcoming: await self._schedule_upcoming_download(download) else: + download.info.status = 'queued' await self.queue.put(download) bg_tasks.create_task(self.__start_download(download), name="start_download") else: @@ -2159,7 +2166,13 @@ class DownloadQueue: if getattr(dl.info, 'live_status', None) == 'is_upcoming': await self._schedule_upcoming_download(dl) else: + dl.info.status = 'queued' await self.queue.put(dl) + # Tell the client it moved out of 'pending' now, not when a + # slot frees up: with MAX_CONCURRENT_DOWNLOADS saturated the + # wait is unbounded, and until this lands the row still + # offers the Start button it has already outgrown. + await self.notifier.updated(dl.info) bg_tasks.create_task(self.__start_download(dl), name="start_download") continue if self.queue.exists(id): diff --git a/ui/src/app/app.html b/ui/src/app/app.html index e6e67d2..a31db81 100644 --- a/ui/src/app/app.html +++ b/ui/src/app/app.html @@ -728,6 +728,9 @@ @if (download.value.live_status === 'is_live' && download.value.status !== 'scheduled') { LIVE } + @if (download.value.status === 'queued') { + Queued + } @if (download.value.status === 'scheduled') { diff --git a/ui/src/app/app.spec.ts b/ui/src/app/app.spec.ts index fbec516..d59ead5 100644 --- a/ui/src/app/app.spec.ts +++ b/ui/src/app/app.spec.ts @@ -544,4 +544,57 @@ describe('App', () => { }); }); + // Issue #1081: a download waiting for a concurrency slot ('queued') starts on + // its own, so it must not offer the Start button that a 'pending' row — one + // added with auto-start off — legitimately has. + describe('queued rows do not offer a dead Start button (#1081)', () => { + const queueEntry = (status: string): Download => ({ + id: 'vid1', + title: 'Test', + url: 'https://example.com/v', + download_type: 'video', + quality: 'best', + format: 'any', + folder: '', + custom_name_prefix: '', + playlist_item_limit: 0, + status, + msg: '', + percent: 0, + speed: 0, + eta: 0, + filename: '', + checked: false, + } as Download); + + const render = (status: string) => { + const fixture = TestBed.createComponent(App); + downloads.queue.set('https://example.com/v', queueEntry(status)); + downloads.queueChanged.next(); + fixture.detectChanges(); + return fixture; + }; + + it('hides Start for a queued row but keeps it for a pending one', () => { + const queued = render('queued'); + expect( + (queued.nativeElement as HTMLElement).querySelector('[aria-label="Start download for Test"]') + ).toBeNull(); + + downloads.queue.clear(); + + const pending = render('pending'); + expect( + (pending.nativeElement as HTMLElement).querySelector('[aria-label="Start download for Test"]') + ).not.toBeNull(); + }); + + it('says why the row is idle and counts it as queued', () => { + const fixture = render('queued'); + expect((fixture.nativeElement as HTMLElement).textContent).toContain('Queued'); + expect(fixture.componentInstance.queuedDownloads).toBe(1); + expect(fixture.componentInstance.activeDownloads).toBe(0); + }); + }); + }); diff --git a/ui/src/app/app.ts b/ui/src/app/app.ts index 2fe3062..b624172 100644 --- a/ui/src/app/app.ts +++ b/ui/src/app/app.ts @@ -1728,7 +1728,7 @@ export class App implements AfterViewInit, OnInit, OnDestroy { speed += download.speed || 0; } else if (download.status === 'preparing' || download.status === 'postprocessing') { active++; - } else if (download.status === 'pending' || download.status === 'scheduled') { + } else if (download.status === 'queued' || download.status === 'pending' || download.status === 'scheduled') { queued++; } });