From e271874b1da3ec5c5f8c84636eb3a0a13aa404d4 Mon Sep 17 00:00:00 2001
From: Duncan Tourolle <duncan@tourolle.paris>
Date: Mon, 21 Sep 2026 11:18:20 +0200
Subject: [PATCH] fix(downloads): replace the 5-minute total request deadline
 with a stall timeout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The download worker built its HTTP client with `Client::timeout(300s)`, which
in reqwest is a total deadline that runs until the response body has finished.
Every transfer longer than five minutes was cut off mid-body as "error
decoding response body" and retried. A transcode ignores `Range`, so each
retry restarted from byte zero, met the same deadline, and after three
attempts the download failed — no feature film at transcode speed ever
completed on a device whose audio must be re-encoded, and a large direct copy
limped through in five-minute slices with a backoff between each.

A connect timeout plus a read timeout that resets on every chunk catches a
dead connection without capping how long a healthy transfer may run.

Red first: a loopback server dribbling a body three times longer than the
timeout failed with the old client (and burned the whole retry budget) and
passes now; a second test hangs the socket and shows the stall is still
detected.

DR-289, UT-251.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 docs/requirements.md             |   4 +-
 src-tauri/Cargo.toml             |   3 +
 src-tauri/src/download/worker.rs | 147 ++++++++++++++++++++++++++++++-
 3 files changed, 151 insertions(+), 3 deletions(-)

diff --git a/docs/requirements.md b/docs/requirements.md
index 2bee7565c..1088040db 100644
--- a/docs/requirements.md
+++ b/docs/requirements.md
@@ -490,6 +490,7 @@ Internal architecture, components, and application logic.
 | DR-198 | The webview runs under a real Content-Security-Policy, and the asset protocol is scoped to the one directory it still serves. `csp` was `null`, which disables CSP entirely: any script that reached the web layer — through a future `{@html}`, a dependency, or a devtools paste — would have inherited the whole IPC surface, and with it the user's session. `script-src 'self'` (Tauri injects a nonce for SvelteKit's inline bootstrap script at build time, so no `'unsafe-inline'` is needed) plus `object-src`/`frame-src 'none'` and `base-uri 'self'` is the part that is genuinely restrictive. `img-src`/`media-src`/`connect-src` cannot be: the Jellyfin origin is typed in by the user at run time and is commonly plain `http` on a LAN, so they allow `http:`/`https:` — a wide grant for *data*, but one that still bars `file:`, `filesystem:` and scripting schemes, and leaves `script-src` untouched. `style-src` keeps `'unsafe-inline'` because Svelte compiles `style="…"` attributes (including `app.html`'s `display: contents` wrapper) into markup; this is safe only while no `<style>` element survives into `index.html`, since a nonce there would make Tauri's injection outrank — and therefore void — `'unsafe-inline'`. `worker-src blob:` and `media-src blob:` are hls.js: it demuxes in a worker built from a blob and attaches MSE through `URL.createObjectURL`. `asset:` and `http://asset.localhost` are the same protocol under the two naming schemes `convertFileSrc` emits (custom scheme on Linux/macOS, `http` host on Windows/Android); `ipc:`/`http://ipc.localhost` is the invoke transport, which would otherwise be blocked by `connect-src`. A run-time CSP naming the server origin exactly was rejected: Tauri computes the header from immutable config when it serves the HTML, so it would mean rebuilding config and reloading the webview on every server change, for a policy the user can already point anywhere. The asset-protocol scope narrows from `$APPDATA/**` to `$APPDATA/thumbnails/**` — since DR-137 moved downloaded media to the loopback server, `imageCache` is the only `convertFileSrc` caller left, so the database and the encrypted-token fallback file no longer sit inside the grant | Security | UR-012, UR-071 | Done |
 | DR-287 | Authentication uses only the spellings Jellyfin 12.0 leaves enabled. 12.0 disables `X-Emby-Authorization`, `X-Emby-Token`, `X-MediaBrowser-Token`, the `Emby` scheme and the `api_key` **query parameter** by default, and a migration (`DisableLegacyAuthorization`) turns them off on upgraded servers too — so a client using them stops working against an upgraded server rather than degrading. This is not a version branch: `Authorization` with the `MediaBrowser` scheme, and `ApiKey` as a query parameter, are ungated on *both* generations, and the header value this app already built was always the correct one. So the fix is a rename at 21 header sites and 28 query sites, not a capability flag. The query-parameter spelling is load-bearing rather than cosmetic: stream URLs are handed to mpv, ExoPlayer and the webview's `<video>`, none of which can set a header, so `ApiKey` is the only way a player authenticates at all. A structural test refuses any deprecated spelling reaching a request builder, because the failure is silent until a server upgrades | Security | UR-085 | Proposed |
 | DR-288 | A type-filtered listing states `Recursive` explicitly. Jellyfin 12.0 defaults it to true when the parent is a library folder and `IncludeItemTypes` is set, where 10.11 returned immediate children — the identical request, a different result set, with nothing in the response to say which rule applied. Sending the value the client actually wants makes both generations agree, and the value sent is the one that shipped rather than the new server-side default, so this is a compatibility fix and not a silent behaviour change | Repository | UR-085 | Proposed |
+| DR-289 | The download worker's HTTP client carries a **read** timeout and a connect timeout, never a total request timeout. reqwest's `Client::timeout` is a deadline that runs until the body has finished, and it was set to five minutes: every transfer longer than that was cut off mid-body as "error decoding response body" and retried. A transcode ignores `Range`, so each retry restarted from byte zero, met the same deadline, and after three attempts the download failed — no feature film at transcode speed ever completed on a device whose audio must be re-encoded, and a large direct copy limped through in five-minute slices with a backoff between each. A read timeout resets on every chunk, so it still catches a dead connection without capping how long a healthy transfer may run | Downloads | UR-071 | Done |
 
 ---
 
@@ -568,7 +569,7 @@ Internal architecture, components, and application logic.
 | UR-068 | - | DR-119 |
 | UR-069 | - | DR-113, DR-114, DR-120 |
 | UR-070 | - | DR-121, DR-122 |
-| UR-071 | IR-032 | DR-123, DR-124, DR-125, DR-126, DR-127, DR-128, DR-133, DR-134, DR-135, DR-136, DR-137, DR-138, DR-170, DR-171, DR-180, DR-198, DR-199 |
+| UR-071 | IR-032 | DR-123, DR-124, DR-125, DR-126, DR-127, DR-128, DR-133, DR-134, DR-135, DR-136, DR-137, DR-138, DR-170, DR-171, DR-180, DR-198, DR-199, DR-289 |
 | UR-072 | - | DR-156 |
 | UR-073 | - | DR-158 |
 | UR-074 | - | DR-162, DR-177, DR-181 |
@@ -836,6 +837,7 @@ Internal architecture, components, and application logic.
 | UT-248 | Narrowing the library clause does not starve the libraries that do have landing pages: music, movies and TV each still list their own media and none of the others | DR-277 | Done |
 | UT-249 | Opening an individual collection still lists its own children: a BoxSet's members are matched by the stored `parent_id`, not by the library clause, so narrowing that clause did not empty collections | DR-277 | Done |
 | UT-250 | Two libraries of the same collection type are not interchangeable: seeded through the real cache write path, a "TV" and a "Shows" library each list their own series and not the other's | DR-278 | Done |
+| UT-251 | A download that runs longer than the stall timeout completes as long as bytes keep arriving, and one whose connection goes silent is given up on promptly as a network error — driven through the real `reqwest` client against a loopback socket, because the defect was the client's configuration | DR-289 | Done |
 ### Integration Tests
 
 | Test ID | Test Description | Traces To | Status |
diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml
index f445aafff..cb24a435b 100644
--- a/src-tauri/Cargo.toml
+++ b/src-tauri/Cargo.toml
@@ -150,6 +150,9 @@ ndk-context = "0.1"
 
 [dev-dependencies]
 tempfile = "3.24.0"
+# `net` for the loopback server in `download::worker::timeout_tests`; reqwest
+# enables it transitively, but a test must not depend on that.
+tokio = { version = "1", features = ["net"] }
 wiremock = "0.6.5"
 
 [features]
diff --git a/src-tauri/src/download/worker.rs b/src-tauri/src/download/worker.rs
index 4665605bf..f4ab4e72a 100644
--- a/src-tauri/src/download/worker.rs
+++ b/src-tauri/src/download/worker.rs
@@ -18,11 +18,40 @@ pub struct DownloadWorker {
     max_retries: u32,
 }
 
+/// How long a transfer may go without receiving a single byte before it is
+/// treated as dead and retried. Generous because a transcode download waits on
+/// ffmpeg, which pauses when Jellyfin throttles it.
+const STALL_TIMEOUT: Duration = Duration::from_secs(60);
+
+/// How long to wait for the TCP/TLS handshake before giving up.
+const CONNECT_TIMEOUT: Duration = Duration::from_secs(30);
+
 impl DownloadWorker {
     pub fn new() -> Self {
+        Self::with_stall_timeout(STALL_TIMEOUT, true)
+    }
+
+    /// Build a worker whose HTTP client gives up on a transfer that receives
+    /// nothing for `stall`. `https_only` is relaxed only by tests, which serve
+    /// from a loopback socket.
+    ///
+    /// The timeouts are a *connect* timeout and a *read* timeout — never
+    /// `Client::timeout`. That one is a total deadline that runs until the body
+    /// has finished, and it was set to five minutes: every download longer than
+    /// that was cut off mid-body with "error decoding response body", then
+    /// retried. A transcode ignores `Range`, so each retry restarted from byte
+    /// zero, ran into the same five minutes, and after three attempts the
+    /// download failed — which is why no feature film at transcode speed ever
+    /// completed on a device that needs the audio re-encoded. A read timeout
+    /// resets on every chunk, so it catches a dead connection without putting a
+    /// ceiling on how long a healthy transfer may run.
+    ///
+    /// TRACES: UR-071 | DR-289 | UT-251
+    fn with_stall_timeout(stall: Duration, https_only: bool) -> Self {
         let client = reqwest::Client::builder()
-            .timeout(Duration::from_secs(300)) // 5 minute timeout
-            .https_only(true)
+            .connect_timeout(CONNECT_TIMEOUT)
+            .read_timeout(stall)
+            .https_only(https_only)
             .build()
             .expect("Failed to create HTTP client");
 
@@ -436,3 +465,117 @@ mod tests {
         assert!(!DownloadError::Network("timeout".to_string()).is_stopped());
     }
 }
+
+/// Transfers that outlive the timeout. These drive the real `reqwest` client
+/// against a loopback socket because the defect lived in how that client was
+/// configured, not in any code of ours a mock could stand in for.
+#[cfg(test)]
+mod timeout_tests {
+    use super::*;
+    use tokio::net::TcpListener;
+
+    /// Serve one HTTP/1.1 response of `chunks` bodies of `chunk_len` bytes,
+    /// pausing `gap` between them. `hang_after` chunks, the server stops sending
+    /// and never closes — a stalled connection.
+    async fn dribbling_server(
+        chunks: usize,
+        chunk_len: usize,
+        gap: Duration,
+        hang_after: Option<usize>,
+    ) -> String {
+        let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
+        let addr = listener.local_addr().unwrap();
+        tokio::spawn(async move {
+            let (mut sock, _) = listener.accept().await.unwrap();
+            // Drain the request head; we answer the same thing regardless.
+            let mut buf = [0u8; 4096];
+            let _ = tokio::io::AsyncReadExt::read(&mut sock, &mut buf).await;
+            let head = format!(
+                "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n",
+                chunks * chunk_len
+            );
+            if sock.write_all(head.as_bytes()).await.is_err() {
+                return;
+            }
+            let body = vec![b'x'; chunk_len];
+            for i in 0..chunks {
+                if hang_after == Some(i) {
+                    // Hold the socket open forever without writing.
+                    tokio::time::sleep(Duration::from_secs(3600)).await;
+                }
+                // The client hanging up (as it does once it times out) is not
+                // the server's failure to report.
+                if sock.write_all(&body).await.is_err() || sock.flush().await.is_err() {
+                    return;
+                }
+                tokio::time::sleep(gap).await;
+            }
+        });
+        format!("http://{}/file.bin", addr)
+    }
+
+    /// A download that takes longer than the timeout but never stalls must
+    /// finish. The worker set `Client::timeout`, which in reqwest is a *total*
+    /// deadline covering the body, so every transfer longer than five minutes —
+    /// any film at transcode speed — was cut off with "error decoding response
+    /// body", retried from byte zero (a transcode ignores `Range`), and cut off
+    /// again until the retry budget ran out.
+    ///
+    /// TRACES: UR-071 | DR-289 | UT-251
+    #[tokio::test]
+    async fn test_download_longer_than_the_stall_timeout_completes_when_bytes_keep_flowing() {
+        let stall = Duration::from_millis(400);
+        // 12 chunks × 100 ms ≈ 1.2 s of transfer, three times the stall timeout,
+        // with every gap comfortably inside it.
+        let url = dribbling_server(12, 1024, Duration::from_millis(100), None).await;
+        let dir = tempfile::tempdir().unwrap();
+        let task = DownloadTask {
+            url,
+            target_path: dir.path().join("file.bin"),
+        };
+
+        let worker = DownloadWorker::with_stall_timeout(stall, false);
+        let result = worker
+            .download(&task, &AtomicBool::new(false), |_, _| {})
+            .await;
+
+        let result = result
+            .unwrap_or_else(|e| panic!("a transfer that never stalls must not time out: {e:?}"));
+        assert_eq!(result.bytes_downloaded, 12 * 1024);
+        assert!(task.target_path.exists());
+    }
+
+    /// The converse: a connection that goes silent is still given up on, so
+    /// dropping the total deadline did not turn a dead wifi link into a download
+    /// that hangs forever with no retry.
+    ///
+    /// TRACES: UR-071 | DR-289 | UT-251
+    #[tokio::test]
+    async fn test_download_that_stalls_is_given_up_on() {
+        let stall = Duration::from_millis(300);
+        let url = dribbling_server(4, 1024, Duration::from_millis(10), Some(2)).await;
+        let dir = tempfile::tempdir().unwrap();
+        let task = DownloadTask {
+            url,
+            target_path: dir.path().join("file.bin"),
+        };
+
+        let worker = DownloadWorker::with_stall_timeout(stall, false);
+        // `download()` retries with 5 s/15 s/45 s backoff; a single attempt is
+        // what proves the stall is detected.
+        let started = std::time::Instant::now();
+        let result = worker
+            .try_download(&task, &AtomicBool::new(false), &|_, _| {})
+            .await;
+
+        assert!(
+            matches!(result, Err(DownloadError::Network(_))),
+            "a stalled transfer must fail as a network error: {result:?}"
+        );
+        assert!(
+            started.elapsed() < Duration::from_secs(5),
+            "the stall must be detected promptly, took {:?}",
+            started.elapsed()
+        );
+    }
+}