Bug: Download queue permanently stuck at "Queued" — IsDownloading guard leaks in StartDownloadCycle

[wilsonbirch]

Reproduction steps

Observed in normal use: after downloads had been running for a while, every subsequently queued video stayed at Queued forever. Adding new downloads and pressing retry did nothing, with no error shown, until the app was fully restarted — after which the entire backlog downloaded normally.

Deterministic repro for one of the leak paths (the early return):

1. Start a playlist download
2. Delete that playlist while its PlaylistDownload record still exists
3. Queue any single video
4. The download cycle removes the stale playlist download and returns early → the queued video never starts, and every video queued afterwards also stays "Queued" until the app is restarted

Actual result

Downloads sit at "Queued" indefinitely: State: 0, Error: null, Progress: 0, no download speed. Confirmed by reading the record in downloads_ongoing/ while the app ignored it for hours. Nothing in the log at WARNING level; no "Started downloading cycle" line appears for subsequent triggers. Only an app restart recovers.

Expected result

Queued downloads start; a failure in one download or one stale playlist record does not disable downloading for the rest of the session.

Grayjay Version

Desktop, versionCode 17 (stable), Linux x86_64 (Flathub app.grayjay.Grayjay).

Not Flatpak-specific — cause is in Grayjay.ClientServer, verified against current master.

What plugins are you seeing the problem on?

All — the failure is in the download scheduler, before any plugin work.

Are you using a VPN?

No

Root cause

Grayjay.ClientServer/States/StateDownloads.cs → StartDownloadCycle() sets the static IsDownloading guard on entry, but the body is not protected by try/finally, so the flag can be left true forever:

1. Early return. In the downloading-playlists loop, when StatePlaylists.Get(playlistDownload.PlaylistID) returns null, the code calls RemoveDownloadingPlaylist(...) and then executes a bare return; — exiting with IsDownloading still true. (This return also looks like it should be continue: one stale playlist record aborts the whole cycle, including all the other playlists and every queued single video.)
2. Unhandled exception. Anything escaping the cycle body leaks the flag identically. Every call site is fire-and-forget (_ = StateDownloads.StartDownloadCycle(); in StateApp.cs:319, StatePlaylists.cs:81, plus DownloadController and DetailsController:619), so the faulted task is never observed and nothing is logged.

Once leaked, every future trigger hits if (IsDownloading) return; and silently no-ops — the queue is dead until restart.

Related but separate: download.Download(...) is awaited with no read timeout, so one stalled connection blocks the cycle indefinitely while holding the flag. Cancelling the stuck item removes its record but doesn't unblock the cycle thread. A finally doesn't fix that case (the thread is still parked), but it does stop every crash/early-exit path from bricking downloads for the session.

Suggested fix

Wrap the cycle body in try/finally and make the missing-playlist case continue. Diff shown with whitespace changes ignored (git show -w) — the real patch re-indents the block:

--- a/Grayjay.ClientServer/States/StateDownloads.cs
+++ b/Grayjay.ClientServer/States/StateDownloads.cs
@@ -192,7 +192,8 @@ namespace Grayjay.ClientServer.States
             }
             Logger.i(nameof(StateDownloads), "Started downloading cycle");
 
-
+            try
+            {
                 List<PlaylistDownload> playlistsToDownload = StateDownloads.GetDownloadingPlaylists();
                 foreach (var playlistDownload in playlistsToDownload)
                 {
@@ -202,7 +203,7 @@ namespace Grayjay.ClientServer.States
                         if(playlist == null)
                         {
                             StateDownloads.RemoveDownloadingPlaylist(playlistDownload.PlaylistID);
-                        return;
+                            continue;
                         }
 
                         await CheckOutdatedPlaylistVideos(playlist, playlistDownload);
@@ -239,8 +240,12 @@ namespace Grayjay.ClientServer.States
                 }
 
                 Logger.i(nameof(StateDownloads), "Ended downloading cycle");
+            }
+            finally
+            {
                 IsDownloading = false;
             }
+        }

Two follow-ups worth considering separately: read timeouts / stall detection on the download HTTP client so a dead connection can't park the cycle, and logging the exception from the fire-and-forget call sites so this class of failure isn't silent.

Happy to submit this as a merge request if useful — I understand development happens on gitlab.futo.org and self-registration there is closed, so let me know how you'd prefer to receive it. Disclosure: the change is mechanical and read-verified against master, but I have not built the solution locally to runtime-test it.

[wilsonbirch]

Cross-referencing two existing issues that appear to touch the same code path, in case it helps triage — I don't think either is a duplicate of this one:

• Improved handling of failed downloads #230 — a comment there asks for "the ability to cancel an in-progress download, so that the queue continues with the next". That is the second failure mode described above: StartDownloadCycle is parked inside await download.Download(...) with no read timeout, and cancelling only removes the record — the cycle thread stays blocked holding IsDownloading, so the queue does not continue. Read timeouts / cancellation plumbed into the download client would fix both that request and this half of the report.

• Bug: Failed Download Entry will not vanish #640 ("Failed Download Entry will not vanish") looks like a different root cause worth noting separately: in TryDownloadVideo, the error handler sets DownloadState.ERROR and adds the item to ignore, but the cleanup calls are commented out —

  //StateDownloads.RemoveDownload(currentVideo);
  //StateDownloads.PreventPlaylistDownload(currentVideo);

  so a failed entry is intentionally (?) left in the store with no UI path to clear it.