fix(downloads): replace the 5-minute total request deadline with a stall timeout

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>
This commit is contained in:
2026-09-21 11:18:20 +02:00
co-authored by Claude Opus 5
parent 24d85f3738
commit e271874b1d
3 changed files with 151 additions and 3 deletions
+3
View File
@@ -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]
+145 -2
View File
@@ -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()
);
}
}