dtourolle/jellytau
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 40 Wiki Activity

fix(downloads): stop libraries mixing, make pause/resume real, reap partials, end bitrate corruption

Browse Source 
Four defects behind "downloads still flaky", each with its own cause.

Libraries mixed their media (DR-167). Cached items carry no link back to their
library — library_id and parent_id are NULL on every row — so the library branch
of get_downloaded_items matched `EXISTS (SELECT 1 FROM libraries WHERE id = ?)`,
which asserts only that the library exists and never constrains the item to it.
Opening any downloaded library listed every downloaded top-level item on the
server: films under Music, albums under TV. The query deciding which libraries
appear already had the right rule, so the two disagreed about the same question;
that collection_type <-> item_type mapping is now one constant used by both.

Pause and resume did nothing (DR-168). pause_download wrote status = 'paused'
and stopped there — no cancellation existed anywhere in the download stack, so
the streaming task ran on and overwrote the row with completed/failed when it
finished. The row flicked to "paused" and undid itself. resume_download had the
mirror defect: it flipped the row to 'pending' without pumping, and the pump is
not a poller, so a resumed download sat until some unrelated event pumped the
queue. Adds a per-download stop flag the worker reads between chunks and on
retry, returning Stopped — not retryable, not recorded as a failure, and the
.part file is kept because that is what the resume continues from. Registering
returns a fresh flag so a resumed download does not inherit the pause that
stopped it. Cancel and clear_stale_downloads signal it too, so neither deletes a
file still being written.

Partial files were never reaped (DR-169). The worker named its sidecar with
with_extension("part"), which replaces: movie.mp4 became movie.part. Every
cleanup path deleted "{file_path}.part" — movie.mp4.part. They never matched, so
the partial of every cancelled or failed download stayed on disk forever,
invisible to disk-usage totals because no row pointed at it. One partial_path
helper now serves the writer and the cleaners.

Bitrate downloads corrupted themselves (DR-170). Only `original` asks for
Static=true; every other rung requests a transcode, which Jellyfin serves
chunked with no Content-Length and cannot byte-seek — it ignores Range and
answers 200 with the whole stream, not 206 with the tail. The worker sent the
header whenever a .part existed and appended the body regardless, so each retry
concatenated another full copy onto what was on disk. The file grew past its
real size and would not play, which is why bitrate downloads stayed broken after
the videoBitRate casing fix corrected the request. resume_offset now lets the
response decide: append only on 206, otherwise truncate and take it from the top.

docs/requirements.md also carries DR-171/UT-166, written by a parallel session
working in the same tree; its code lands separately.
This commit is contained in:
dtourolle
2026-08-15 23:52:02 +02:00
parent d49d027020
commit a5535f2941
5 changed files with 422 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src-tauri/src/commands/download/mod.rs
+94 -21
View File
@@ -845,7 +845,19 @@ pub async fn get_downloads(
Ok(DownloadsResponse { downloads, stats })
}
/// Pause a download
/// Pause a download.
///
/// Writing `status = 'paused'` is only half of it, and used to be all of it: the
/// streaming task knew nothing about the row and kept running, then overwrote it
/// with `completed`/`failed` when it finished. The row flicked to "paused" and
/// undid itself — the reported "pause does not work". Signalling the worker is
/// what actually stops the bytes; it leaves the `.part` file in place so
/// [`resume_download`] can continue from it.
///
/// A queued (not yet started) download has no worker to signal, and the status
/// write alone is enough — the pump skips anything that is not `pending`.
///
/// TRACES: UR-055 | DR-168
#[tauri::command]
#[specta::specta]
pub async fn pause_download(
@@ -858,19 +870,34 @@ pub async fn pause_download(
};
let query = Query::with_params(
"UPDATE downloads SET status = 'paused' WHERE id = ? AND status = 'downloading'",
"UPDATE downloads SET status = 'paused' WHERE id = ? AND status IN ('downloading', 'pending')",
vec![QueryParam::Int64(download_id)],
);
db_service.execute(query).await.map_err(|e| e.to_string())?;
let was_running = crate::download::stop::signal(download_id);
info!(
"[pause] Download {} paused (in flight: {})",
download_id, was_running
);
Ok(())
}
/// Resume a paused download
/// Resume a paused download.
///
/// Flipping the row back to `pending` is likewise not enough on its own: the
/// pump is not a poller, it runs when something calls it, so a resumed download
/// sat untouched until some unrelated event happened to pump the queue. That is
/// the other half of "resume does not work".
///
/// TRACES: UR-055 | DR-168
#[tauri::command]
#[specta::specta]
pub async fn resume_download(
app: tauri::AppHandle,
db: State<'_, DatabaseWrapper>,
download_manager: State<'_, DownloadManagerWrapper>,
download_id: i64,
) -> Result<(), String> {
let db_service = {
@@ -879,11 +906,22 @@ pub async fn resume_download(
};
let query = Query::with_params(
"UPDATE downloads SET status = 'pending' WHERE id = ? AND status = 'paused'",
"UPDATE downloads SET status = 'pending', error_message = NULL WHERE id = ? AND status IN ('paused', 'failed')",
vec![QueryParam::Int64(download_id)],
);
db_service.execute(query).await.map_err(|e| e.to_string())?;
// Drop any stale stop flag before the pump can start this id again, or the
// resumed run would read the pause that stopped it and halt immediately.
crate::download::stop::clear(download_id);
let active_downloads = {
let manager = download_manager.0.lock().map_err(|e| e.to_string())?;
manager.get_active_downloads()
};
pump_download_queue(app, db_service, active_downloads).await;
Ok(())
}
@@ -923,6 +961,13 @@ pub async fn cancel_download(
.await
.map_err(|e| e.to_string())?;
// Stop the worker if this download is actually running. Without this the
// task keeps streaming into a `.part` file whose `downloads` row has just
// been deleted — bytes with nothing pointing at them, and the file below is
// removed while still being written to. (DR-168)
crate::download::stop::signal(download_id);
crate::download::stop::clear(download_id);
// Unregister from download manager (in case it was active)
{
let manager = download_manager.0.lock().map_err(|e| e.to_string())?;
@@ -934,10 +979,12 @@ pub async fn cancel_download(
);
}
// Delete partial file if exists
// Delete the partial file, and any completed file, if present. Both go
// through `partial_path` so this cannot drift from what the worker writes —
// it did, and every cancelled download leaked its partial. (DR-169)
if let Some(path) = file_path {
let partial_path = format!("{}.part", path);
let _ = std::fs::remove_file(&partial_path); // Ignore errors
let target = std::path::PathBuf::from(&path);
let _ = std::fs::remove_file(crate::download::worker::partial_path(&target));
}
Ok(())
@@ -1244,8 +1291,6 @@ pub async fn enqueue_video_downloads(
download_ids: Vec<i64>,
target_dir: String,
) -> Result<(), String> {
use crate::repository::MediaRepository;
let repo = repository.0.get(&handle).ok_or("Repository not found")?;
let db_service = {
@@ -1270,10 +1315,12 @@ pub async fn enqueue_video_downloads(
}
};
// Build the transcode URL (pure URL builder, no server round-trip).
let stream_url = repo
.as_ref()
.get_video_download_url(&item_id, &quality, None);
// Build the download URL, resolving the source's audio codec first so a
// track this device cannot decode is re-encoded on the way down rather
// than saved as a silent file (DR-167).
let stream_url =
crate::repository::resolve_video_download_url(repo.as_ref(), &item_id, &quality, None)
.await;
let update_query = Query::with_params(
"UPDATE downloads SET status = 'pending', stream_url = ?, target_dir = ? WHERE id = ?",
@@ -1530,7 +1577,11 @@ fn spawn_download_worker(
let _ = progress_app.emit("download-event", event);
};
let result = worker.download(&task, on_progress).await;
// Registering returns a fresh flag, so a download resumed after a pause
// does not inherit the stop that ended its previous run. (DR-168)
let stop_flag = crate::download::stop::register(download_id);
let result = worker.download(&task, &stop_flag, on_progress).await;
crate::download::stop::clear(download_id);
// Free the slot before pumping so the next download can take it.
if let Ok(mut active) = active_downloads.lock() {
@@ -1601,6 +1652,17 @@ fn spawn_download_worker(
Err(e) => error!(" Completed event emit failed: {:?}", e),
}
}
// A pause or cancel is not a failure. The row already says `paused`
// (or the row is gone, for a cancel), and overwriting that with
// `failed` is what made a pause look like an error and stranded the
// download outside the resumable set. The `.part` file is deliberately
// left alone — it is what the resume continues from. (DR-168)
Err(e) if e.is_stopped() => {
info!(
"[pump] Download {} stopped by request; partial file kept for resume",
download_id
);
}
Err(e) => {
error!("Download failed: {:?}", e);
@@ -1848,17 +1910,26 @@ pub async fn clear_stale_downloads(
Arc::new(database.service())
};
// Get file paths for stale downloads (pending/paused/failed)
// Ids as well as paths: a stale row may still have a worker attached (a
// 'downloading' row that was paused mid-flight is 'paused' here), and
// deleting the row without stopping the task leaves it writing to a file we
// are about to remove. (DR-168)
let file_query = Query::with_params(
"SELECT file_path FROM downloads WHERE user_id = ? AND status IN ('pending', 'paused', 'failed')",
"SELECT id, file_path FROM downloads WHERE user_id = ? AND status IN ('pending', 'paused', 'failed')",
vec![QueryParam::String(user_id.clone())],
);
let file_paths: Vec<String> = db_service
.query_many(file_query, |row| row.get(0))
let stale: Vec<(i64, String)> = db_service
.query_many(file_query, |row| Ok((row.get(0)?, row.get(1)?)))
.await
.map_err(|e| e.to_string())?;
for (id, _) in &stale {
crate::download::stop::signal(*id);
crate::download::stop::clear(*id);
}
let file_paths: Vec<String> = stale.into_iter().map(|(_, path)| path).collect();
// Delete all pending, paused, and failed downloads (but keep completed ones)
let delete_query = Query::with_params(
"DELETE FROM downloads WHERE user_id = ? AND status IN ('pending', 'paused', 'failed')",
@@ -1870,10 +1941,12 @@ pub async fn clear_stale_downloads(
.await
.map_err(|e| e.to_string())?;
// Delete any partial files
// Delete any partial files, via the shared helper so this cannot drift from
// what the worker actually writes. (DR-169)
for path in file_paths {
let _ = std::fs::remove_file(&path);
let _ = std::fs::remove_file(format!("{}.part", path));
let target = std::path::PathBuf::from(&path);
let _ = std::fs::remove_file(&target);
let _ = std::fs::remove_file(crate::download::worker::partial_path(&target));
}
Ok(deleted_count as i64)