feat(tui): surface the failure reason on failed downloads
Carry a human-readable reason on DownloadStatus::Failed so it flows through the existing status channel to the TUI. The download Info pane now shows an Error line for a failed download (connect failure, peer disconnect, truncated transfer, peer-reported upload failure, or the underlying transfer error). Failures without a known reason render as before.
This commit is contained in:
@@ -374,7 +374,7 @@ fn download_without_a_connection_resolves_failed() {
|
|||||||
.expect("download() should return a handle");
|
.expect("download() should return a handle");
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
receiver.recv_timeout(Duration::from_secs(1)),
|
receiver.recv_timeout(Duration::from_secs(1)),
|
||||||
Ok(DownloadStatus::Failed)
|
Ok(DownloadStatus::Failed(_))
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -398,7 +398,7 @@ fn fail_queued_downloads_notifies_receiver_and_store() {
|
|||||||
|
|
||||||
Client::fail_queued_downloads(&client.context, "peer");
|
Client::fail_queued_downloads(&client.context, "peer");
|
||||||
|
|
||||||
assert!(matches!(receiver.try_recv(), Ok(DownloadStatus::Failed)));
|
assert!(matches!(receiver.try_recv(), Ok(DownloadStatus::Failed(_))));
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
client
|
client
|
||||||
.context
|
.context
|
||||||
@@ -407,7 +407,7 @@ fn fail_queued_downloads_notifies_receiver_and_store() {
|
|||||||
.get_download_by_token(7)
|
.get_download_by_token(7)
|
||||||
.unwrap()
|
.unwrap()
|
||||||
.status,
|
.status,
|
||||||
DownloadStatus::Failed
|
DownloadStatus::Failed(_)
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1056,10 +1056,18 @@ impl Client {
|
|||||||
};
|
};
|
||||||
|
|
||||||
if failed {
|
if failed {
|
||||||
let _ = download.sender.send(DownloadStatus::Failed);
|
let reason = if peer_registered {
|
||||||
self.context
|
"Peer rejected the download request"
|
||||||
.write_safe()?
|
} else {
|
||||||
.update_download_with_status(token, DownloadStatus::Failed);
|
"Could not connect to the peer"
|
||||||
|
};
|
||||||
|
let _ = download
|
||||||
|
.sender
|
||||||
|
.send(DownloadStatus::Failed(Some(reason.to_string())));
|
||||||
|
self.context.write_safe()?.update_download_with_status(
|
||||||
|
token,
|
||||||
|
DownloadStatus::Failed(Some(reason.to_string())),
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok((download, download_receiver))
|
Ok((download, download_receiver))
|
||||||
@@ -1088,8 +1096,12 @@ impl Client {
|
|||||||
.map(|d| (d.token, d.sender.clone()))
|
.map(|d| (d.token, d.sender.clone()))
|
||||||
.collect();
|
.collect();
|
||||||
for (token, sender) in doomed {
|
for (token, sender) in doomed {
|
||||||
let _ = sender.send(DownloadStatus::Failed);
|
let reason = Some("Peer disconnected".to_string());
|
||||||
context.update_download_with_status(token, DownloadStatus::Failed);
|
let _ = sender.send(DownloadStatus::Failed(reason.clone()));
|
||||||
|
context.update_download_with_status(
|
||||||
|
token,
|
||||||
|
DownloadStatus::Failed(reason),
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1142,9 +1154,12 @@ impl Client {
|
|||||||
match client_context.write_safe() {
|
match client_context.write_safe() {
|
||||||
Ok(mut context) => {
|
Ok(mut context) => {
|
||||||
for token in failed_tokens {
|
for token in failed_tokens {
|
||||||
context
|
context.downloads.update_status(
|
||||||
.downloads
|
token,
|
||||||
.update_status(token, DownloadStatus::Failed);
|
DownloadStatus::Failed(Some(
|
||||||
|
"Peer reported the upload failed".to_string(),
|
||||||
|
)),
|
||||||
|
);
|
||||||
context.downloads.remove(token);
|
context.downloads.remove(token);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1326,9 +1341,12 @@ impl Client {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
let _ = download.sender.send(DownloadStatus::Failed);
|
let reason = Some(
|
||||||
|
e.to_string(),
|
||||||
|
);
|
||||||
|
let _ = download.sender.send(DownloadStatus::Failed(reason.clone()));
|
||||||
match client_context_clone.write_safe() {
|
match client_context_clone.write_safe() {
|
||||||
Ok(mut ctx) => ctx.update_download_with_status(download.token, DownloadStatus::Failed),
|
Ok(mut ctx) => ctx.update_download_with_status(download.token, DownloadStatus::Failed(reason)),
|
||||||
Err(e) => error!("[client] download failed write: {}", e),
|
Err(e) => error!("[client] download failed write: {}", e),
|
||||||
}
|
}
|
||||||
error!(
|
error!(
|
||||||
|
|||||||
@@ -149,7 +149,9 @@ pub fn collect_failed_tokens(
|
|||||||
d.username == username && filename.is_none_or(|f| d.filename == *f)
|
d.username == username && filename.is_none_or(|f| d.filename == *f)
|
||||||
})
|
})
|
||||||
.map(|d| {
|
.map(|d| {
|
||||||
let _ = d.sender.send(DownloadStatus::Failed);
|
let _ = d.sender.send(DownloadStatus::Failed(Some(
|
||||||
|
"Peer reported the upload failed".to_string(),
|
||||||
|
)));
|
||||||
d.token
|
d.token
|
||||||
})
|
})
|
||||||
.collect()
|
.collect()
|
||||||
@@ -301,7 +303,7 @@ mod tests {
|
|||||||
assert_eq!(tokens, vec![1]);
|
assert_eq!(tokens, vec![1]);
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
rx_match.try_recv().unwrap(),
|
rx_match.try_recv().unwrap(),
|
||||||
DownloadStatus::Failed
|
DownloadStatus::Failed(_)
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -212,7 +212,7 @@ fn handle_file_connection(
|
|||||||
match context.client_context.write_safe() {
|
match context.client_context.write_safe() {
|
||||||
Ok(mut ctx) => ctx.update_download_with_status(
|
Ok(mut ctx) => ctx.update_download_with_status(
|
||||||
failure_token,
|
failure_token,
|
||||||
DownloadStatus::Failed,
|
DownloadStatus::Failed(Some(e.to_string())),
|
||||||
),
|
),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
error!(
|
error!(
|
||||||
|
|||||||
@@ -114,7 +114,7 @@ impl Download {
|
|||||||
matches!(
|
matches!(
|
||||||
self.status,
|
self.status,
|
||||||
DownloadStatus::Completed
|
DownloadStatus::Completed
|
||||||
| DownloadStatus::Failed
|
| DownloadStatus::Failed(_)
|
||||||
| DownloadStatus::TimedOut
|
| DownloadStatus::TimedOut
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
@@ -159,7 +159,8 @@ pub enum DownloadStatus {
|
|||||||
total_bytes: u64,
|
total_bytes: u64,
|
||||||
},
|
},
|
||||||
Completed,
|
Completed,
|
||||||
Failed,
|
/// Failed, optionally with a human-readable reason.
|
||||||
|
Failed(Option<String>),
|
||||||
TimedOut,
|
TimedOut,
|
||||||
}
|
}
|
||||||
impl Transfer {
|
impl Transfer {
|
||||||
|
|||||||
@@ -614,7 +614,7 @@ fn a_file_downloads_from_a_peer_over_p_and_f_connections() {
|
|||||||
completed = true;
|
completed = true;
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
Ok(DownloadStatus::Failed | DownloadStatus::TimedOut) => {
|
Ok(DownloadStatus::Failed(_) | DownloadStatus::TimedOut) => {
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
_ => {}
|
_ => {}
|
||||||
@@ -862,7 +862,7 @@ fn a_file_downloads_from_a_peer_via_direct_connection() {
|
|||||||
completed = true;
|
completed = true;
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
Ok(DownloadStatus::Failed | DownloadStatus::TimedOut) => break,
|
Ok(DownloadStatus::Failed(_) | DownloadStatus::TimedOut) => break,
|
||||||
_ => {}
|
_ => {}
|
||||||
}
|
}
|
||||||
if client
|
if client
|
||||||
@@ -1021,7 +1021,7 @@ fn a_file_downloads_from_a_firewalled_peer_via_server_broker() {
|
|||||||
completed = true;
|
completed = true;
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
Ok(DownloadStatus::Failed | DownloadStatus::TimedOut) => break,
|
Ok(DownloadStatus::Failed(_) | DownloadStatus::TimedOut) => break,
|
||||||
_ => {}
|
_ => {}
|
||||||
}
|
}
|
||||||
if client
|
if client
|
||||||
@@ -1122,7 +1122,7 @@ fn two_real_clients_search_and_download() {
|
|||||||
completed = true;
|
completed = true;
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
Ok(DownloadStatus::Failed | DownloadStatus::TimedOut) => break,
|
Ok(DownloadStatus::Failed(_) | DownloadStatus::TimedOut) => break,
|
||||||
_ => {}
|
_ => {}
|
||||||
}
|
}
|
||||||
if leecher
|
if leecher
|
||||||
|
|||||||
@@ -231,7 +231,7 @@ impl MultiDownloadProgress {
|
|||||||
DownloadStatus::InProgress { .. } => "⧗",
|
DownloadStatus::InProgress { .. } => "⧗",
|
||||||
DownloadStatus::Paused { .. } => "⏸",
|
DownloadStatus::Paused { .. } => "⏸",
|
||||||
DownloadStatus::Completed => "✓",
|
DownloadStatus::Completed => "✓",
|
||||||
DownloadStatus::Failed => "✗",
|
DownloadStatus::Failed(_) => "✗",
|
||||||
DownloadStatus::TimedOut => "⏱",
|
DownloadStatus::TimedOut => "⏱",
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -272,7 +272,7 @@ impl MultiDownloadProgress {
|
|||||||
DownloadStatus::InProgress { .. } => warning_style(),
|
DownloadStatus::InProgress { .. } => warning_style(),
|
||||||
DownloadStatus::Paused { .. } => info_style(),
|
DownloadStatus::Paused { .. } => info_style(),
|
||||||
DownloadStatus::Completed => primary_style(),
|
DownloadStatus::Completed => primary_style(),
|
||||||
DownloadStatus::Failed | DownloadStatus::TimedOut => {
|
DownloadStatus::Failed(_) | DownloadStatus::TimedOut => {
|
||||||
error_style()
|
error_style()
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
@@ -346,7 +346,7 @@ pub fn render_download_stats(
|
|||||||
.filter(|d| {
|
.filter(|d| {
|
||||||
matches!(
|
matches!(
|
||||||
d.download.status,
|
d.download.status,
|
||||||
DownloadStatus::Failed | DownloadStatus::TimedOut
|
DownloadStatus::Failed(_) | DownloadStatus::TimedOut
|
||||||
)
|
)
|
||||||
})
|
})
|
||||||
.count();
|
.count();
|
||||||
|
|||||||
@@ -958,7 +958,7 @@ impl MainTui {
|
|||||||
};
|
};
|
||||||
if !matches!(
|
if !matches!(
|
||||||
entry.download.status,
|
entry.download.status,
|
||||||
DownloadStatus::Failed | DownloadStatus::TimedOut
|
DownloadStatus::Failed(_) | DownloadStatus::TimedOut
|
||||||
) {
|
) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
@@ -995,7 +995,7 @@ impl MainTui {
|
|||||||
!matches!(
|
!matches!(
|
||||||
entry.download.status,
|
entry.download.status,
|
||||||
DownloadStatus::Completed
|
DownloadStatus::Completed
|
||||||
| DownloadStatus::Failed
|
| DownloadStatus::Failed(_)
|
||||||
| DownloadStatus::TimedOut
|
| DownloadStatus::TimedOut
|
||||||
)
|
)
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -72,7 +72,7 @@ fn build_info_lines(
|
|||||||
}
|
}
|
||||||
DownloadStatus::Paused { .. } => ("Paused".to_string(), info_style()),
|
DownloadStatus::Paused { .. } => ("Paused".to_string(), info_style()),
|
||||||
DownloadStatus::Completed => ("Completed".to_string(), success_style()),
|
DownloadStatus::Completed => ("Completed".to_string(), success_style()),
|
||||||
DownloadStatus::Failed => ("Failed".to_string(), error_style()),
|
DownloadStatus::Failed(_) => ("Failed".to_string(), error_style()),
|
||||||
DownloadStatus::TimedOut => ("Timed out".to_string(), error_style()),
|
DownloadStatus::TimedOut => ("Timed out".to_string(), error_style()),
|
||||||
};
|
};
|
||||||
lines.push(label_value_styled("Status", status_text, status_style));
|
lines.push(label_value_styled("Status", status_text, status_style));
|
||||||
@@ -139,9 +139,20 @@ fn build_info_lines(
|
|||||||
lines.push(Line::from(""));
|
lines.push(Line::from(""));
|
||||||
push_progress_lines(&mut lines, *bytes_downloaded, *total_bytes);
|
push_progress_lines(&mut lines, *bytes_downloaded, *total_bytes);
|
||||||
}
|
}
|
||||||
DownloadStatus::Completed
|
DownloadStatus::Failed(reason) => {
|
||||||
| DownloadStatus::Failed
|
if let Some(reason) = reason {
|
||||||
| DownloadStatus::TimedOut => {}
|
lines.push(Line::from(""));
|
||||||
|
lines.push(Line::from(Span::styled(
|
||||||
|
format!("{:<LABEL_WIDTH$}", "Error"),
|
||||||
|
dimmed_style(),
|
||||||
|
)));
|
||||||
|
lines.push(Line::from(Span::styled(
|
||||||
|
reason.clone(),
|
||||||
|
error_style(),
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
DownloadStatus::Completed | DownloadStatus::TimedOut => {}
|
||||||
}
|
}
|
||||||
|
|
||||||
lines
|
lines
|
||||||
@@ -256,4 +267,52 @@ mod tests {
|
|||||||
assert_eq!(basename, "track.mp3");
|
assert_eq!(basename, "track.mp3");
|
||||||
assert_eq!(parent, "");
|
assert_eq!(parent, "");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn download_with_status(
|
||||||
|
status: DownloadStatus,
|
||||||
|
) -> soulseek_rs::types::Download {
|
||||||
|
let (sender, _rx) = std::sync::mpsc::channel();
|
||||||
|
soulseek_rs::types::Download {
|
||||||
|
username: "peer".to_string(),
|
||||||
|
filename: "song.mp3".to_string(),
|
||||||
|
token: 1,
|
||||||
|
size: 100,
|
||||||
|
download_directory: "~/dl".to_string(),
|
||||||
|
status,
|
||||||
|
sender,
|
||||||
|
queue_position: None,
|
||||||
|
metadata: soulseek_rs::types::DownloadMetadata::default(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn lines_to_text(lines: &[Line<'static>]) -> String {
|
||||||
|
lines
|
||||||
|
.iter()
|
||||||
|
.map(|l| {
|
||||||
|
l.spans
|
||||||
|
.iter()
|
||||||
|
.map(|s| s.content.as_ref())
|
||||||
|
.collect::<String>()
|
||||||
|
})
|
||||||
|
.collect::<Vec<_>>()
|
||||||
|
.join("\n")
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn failed_download_renders_its_error_reason() {
|
||||||
|
let download = download_with_status(DownloadStatus::Failed(Some(
|
||||||
|
"Peer disconnected".to_string(),
|
||||||
|
)));
|
||||||
|
let text = lines_to_text(&build_info_lines(&download));
|
||||||
|
assert!(text.contains("Error"), "missing Error label: {text}");
|
||||||
|
assert!(text.contains("Peer disconnected"), "missing reason: {text}");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn failed_download_without_reason_shows_no_error_line() {
|
||||||
|
let download = download_with_status(DownloadStatus::Failed(None));
|
||||||
|
let text = lines_to_text(&build_info_lines(&download));
|
||||||
|
assert!(text.contains("Failed"), "status should still show Failed");
|
||||||
|
assert!(!text.contains("Error"), "unexpected Error line: {text}");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -54,7 +54,7 @@ pub fn render_downloads_pane(
|
|||||||
DownloadStatus::InProgress { .. } => ("⧗", warning_style()),
|
DownloadStatus::InProgress { .. } => ("⧗", warning_style()),
|
||||||
DownloadStatus::Paused { .. } => ("⏸", info_style()),
|
DownloadStatus::Paused { .. } => ("⏸", info_style()),
|
||||||
DownloadStatus::Completed => ("✓", success_style()),
|
DownloadStatus::Completed => ("✓", success_style()),
|
||||||
DownloadStatus::Failed => ("✗", error_style()),
|
DownloadStatus::Failed(_) => ("✗", error_style()),
|
||||||
DownloadStatus::TimedOut => ("⏱", error_style()),
|
DownloadStatus::TimedOut => ("⏱", error_style()),
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -91,7 +91,7 @@ pub fn render_downloads_pane(
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
DownloadStatus::Completed => "Completed".to_string(),
|
DownloadStatus::Completed => "Completed".to_string(),
|
||||||
DownloadStatus::Failed => "Failed".to_string(),
|
DownloadStatus::Failed(_) => "Failed".to_string(),
|
||||||
DownloadStatus::TimedOut => "Timed out".to_string(),
|
DownloadStatus::TimedOut => "Timed out".to_string(),
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user